Repository navigation
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
repr and str show [sealed] for sealed fields. Cipher(cls) checks the class again and finds the credentials, with no request and no HTTP client. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Credentials are found without building an HTTP client, and each refusal names the variable or file to fix. Redaction reads the class's own dict, covers rich, and runs no descriptor that may do work. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
repr and str show [sealed] for sealed fields. Cipher(cls) checks the class again and finds the credentials, with no request. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5b792d5 to
11ee027
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation — commit reviewed: 11ee027aa1b1c5fbce339a6a0c2a23c1017d0a23. Host: Claude Code (scheduled routine). Model: claude-sonnet-5-5. Skills: code-review (medium, performed inline), review-cross-check (done manually: no existing review threads; only changeset-bot and CodeRabbit-skipped notices), review-post.
Blocking issues: none.
Findings: 1 inline comment (non-blocking, medium confidence) on hand_written in _redact.py.
Not reviewed in depth: Rust engine/refusal/plan changes and test files were skimmed from the file list only; tests were not run.
Generated by Claude Code
| filename = code.co_filename | ||
| return ( | ||
| not filename.startswith(_ATTRS_GENERATED) | ||
| and os.path.realpath(filename) not in _GENERATOR_FILES |
There was a problem hiding this comment.
[code-review, medium confidence] hand_written can misclassify a hand-written method as generated, so the decorator silently drops the user's code instead of raising declaration_repr_present.
A user's __repr__ wrapped in reprlib.recursive_repr() (a common idiom) has a wrapper whose __code__.co_filename is reprlib.py. recursive_repr copies __module__ from the wrapped function, so the __module__ == cls.__module__ check passes. The filename then matches _GENERATOR_FILES, and the method is treated as generated and replaced without the refusal the docs promise.
Suggested fix: unwrap via __wrapped__ (or compare __qualname__ / __code__.co_firstlineno against the class's own source) before deciding. Also add a test with a hand-written @reprlib.recursive_repr() __repr__.
Source: code-review (medium)
Generated by Claude Code
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes (1 of 8 review job(s) failed)
infracode — claude (claude-opus-5-5)
Nothing in the infrastructure part of this change needs to be fixed before merge. This PR changes no Terraform, CloudFormation, mise tasks, CI workflows or observability configs. Its only build-config changes are a new stack-auth path dependency in languages/python/Cargo.toml, crates/core/Cargo.toml and one line in languages/python/Cargo.lock.
I checked three things:
- CI path filter.
tests-python.ymlalready listspackages/stack-auth/**, becausestack-kms'sprofilefeature pulls in that crate. A change tostack-authalready reruns the Python suite. httpfeature stays off. The new dependency isdefault-features = false.stack-encryptis built withdefault-features = false, features = ["dynamic"], anddynamicdoes not turn onstack-auth/httporstack-kms/http. Cargo's feature unification does not compile reqwest or TLS into the wheel, so the comments inCargo.tomlandcredentials.rsare correct.- Lock file.
stack-authwas already in the workspace'sCargo.lock, throughstack-kms. The diff adds only the new edge fromstackencrypt-core, socargo ... --lockedstill resolves.
I found no infrastructure issues.
python — claude (claude-opus-5-5)
Nothing must change before merging. Two small follow-ups can wait: hand_written can replace some hand-written methods with no error, and Pydantic's devtools hook still prints plaintext.
I did not repeat the open thread on reprlib.recursive_repr. The first finding below is a different input with a related cause.
| Severity | Finding | Location |
|---|---|---|
| Follow-up | A hand-written method with no __code__, or from another module, is replaced with no error |
languages/python/python/stackencrypt/_redact.py:73 |
| Optional | Pydantic's __pretty__ (devtools) still prints sealed fields |
languages/python/python/stackencrypt/_redact.py:60 |
I could not run the toolchain: this checkout has no .venv, and Pydantic and SQLAlchemy are not installed. I ran hand_written and install alone on Python 3.12, outside the package. They gave the results below. Rust files were out of scope.
rust — claude (claude-opus-5-5)
No Rust change has to be fixed before merging. The two follow-ups are both about the messages a Python user reads. The new Rust code does not use unsafe, has no panic sites, and holds no lock across an .await. The no-request transport keeps stack-auth's http feature off, as the PR description says. The one gap is that cargo clippy and cargo test were not run, because this environment has no Rust toolchain.
| Severity | Finding | Location |
|---|---|---|
| Fix in a follow-up | Refusal::of joins the message and the help with a space only, so the text reads as one broken sentence. This now applies to plan refusals too. |
languages/python/crates/core/src/refusal.rs:24 |
| Fix in a follow-up | When a profile exists but its token has no region or client id, the user sees "no credentials were found" | languages/python/crates/core/src/credentials.rs:47 |
| Optional | The default message is cloned on every refusal, even when a mapped message replaces it | languages/python/crates/core/src/credentials.rs:37 |
test-gap — claude (claude-opus-5-5)
Two coverage gaps should be fixed before merging: repr on a real SQLAlchemy instance with a column that is not loaded, and a Cipher built from a valid stash auth login profile. Both are the main promises of this PR, and today only a stand-in or the refusal path is tested. The other four comments are optional and can wait.
The tests that exist are clear and follow the repo's pytest style. The sketches below use the same helpers (declared, refusal, the credentials fixture, SECRET). I could not run the sketches here, because no .venv is built in this checkout.
Not repeated here: the existing review thread on hand_written and reprlib.recursive_repr already asks for a test of a hand-written @reprlib.recursive_repr() __repr__.
Out of scope, noted only: hand_written returns False for a method whose __module__ differs from the class's module. So a __repr__ defined in a helper module and assigned in the class body (__repr__ = helpers.show) is replaced with no error. This is the same kind of misclassification as the recursive_repr thread.
infracode — codex (gpt-5.6-terra)
This pull request contains no infrastructure-as-code changes. No infrastructure review findings.
python — codex (gpt-5.6-terra)
Redaction must avoid custom attribute access before merging. The remaining issue can wait only after this access is made storage-only.
| Severity | Finding | Location |
|---|---|---|
| request-changes | Reading __dict__ can run user code and trigger I/O. |
languages/python/python/stackencrypt/_redact.py:122 |
Focused tests were not run because this checkout has no Python or pytest executable.
rust — codex (gpt-5.6-terra)
I found no Rust issues that require changes before merging. The Rust credential detection builds a no-request transport and only performs local credential and profile validation.
| Severity | Findings |
|---|---|
| — | None |
Other findings not posted as comments
languages/python/crates/core/src/credentials.rs:56— Optional: the Python messages forInvalidCrn,RegionandInvalidAccessKeyare not checked by any test.languages/python/crates/core/src/credentials.rs:29— Change before merge: no test builds aCipherfrom a validstash auth loginprofile.languages/python/tests/test_cipher.py:151— Optional: this test is named "names the name", but it does not check which field is named.languages/python/python/stackencrypt/_redact.py:122— Change before merge:getattr(instance, "__dict__")runs a custom__getattribute__method.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 6 found, 3 posted |
| claude | claude-opus-5-5 | rust | 3 found, 3 posted |
| claude | claude-opus-5-5 | python | 2 found, 2 posted |
| claude | claude-opus-5-5 | infracode | 0 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | failed |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | python | 1 found, 0 posted |
| codex | gpt-5.6-terra | infracode | 0 found, 0 posted |
Plain language: the reader test did not run; only the word-and-shape check was applied.
Stack: position 7 of 15 (#1405, #1406, #1414, #1415, #1433, #1437, *️⃣ #1453, #1499, #1509, #1510, #1511, #1513, #1516, #1517, #1582). *️⃣ marks this pull request.
Context loaded: the description, 2 linked issue(s) and 4 discussion entries.
| """ | ||
| method = vars(cls).get(name) | ||
| code = getattr(method, "__code__", None) | ||
| if code is None or getattr(method, "__module__", None) != cls.__module__: |
There was a problem hiding this comment.
Fix in a follow-up: hand_written treats a method as generated when it has no __code__ or comes from another module, so the decorator replaces the user's code with no error.
Impact: model replaces a hand-written __str__ or __repr__ without raising declaration_repr_present. The user's formatting code stops running, and nothing tells them. This is a different input from the open reprlib.recursive_repr thread. The root cause is related: the check accepts a method as generated unless it proves the method hand-written.
Evidence: I ran _redact.hand_written on Python 3.12:
class C:
__str__ = functools.partialmethod(lambda self, x: x, "s")
hand_written(C, "__str__") # False
These cases also return False: a callable object, a staticmethod, and __repr__ = helpers.field_repr imported from another module of the same project.
Fix: Treat a method as generated only when a known generator made it. Treat everything else in vars(cls) as hand-written:
def hand_written(cls: type, name: str) -> bool:
method = vars(cls).get(name)
if method is None:
return False
method = inspect.unwrap(method) if callable(method) else method
code = getattr(method, "__code__", None)
if code is None:
return True # partialmethod, callable object, staticmethod, ...
filename = code.co_filename
return not (
filename.startswith(_ATTRS_GENERATED)
or os.path.realpath(filename) in _GENERATOR_FILES
or _is_namedtuple_repr(cls, method)
)Add one test per case: partialmethod, an imported function, and staticmethod.
Found by 1 model: claude
| # attrs compiles its methods under this file name. | ||
| _ATTRS_GENERATED = "<attrs generated" | ||
|
|
||
| METHODS = ("__repr__", "__str__", "__rich_repr__") |
There was a problem hiding this comment.
Optional: Pydantic's __pretty__ hook, which devtools' debug() uses, still prints every sealed field in plaintext.
Impact: devtools.debug(user) on a declared Pydantic model shows the plaintext of each sealed field. The module docstring lists the print paths it covers: repr, str and rich. It does not list devtools. The user may think every print path is covered. Pydantic's Representation base class builds __pretty__, __repr_str__ and __rich_repr__ from one method, __repr_args__.
Fix: On a Pydantic class, also replace __repr_args__, so every Pydantic print hook shows the same output:
def __repr_args__(self: object) -> Iterator[Tuple[str, object]]:
return __rich_repr__(self)
methods = (__repr__, __str__, __rich_repr__)
if hasattr(cls, "__repr_args__"):
setattr(cls, "__repr_args__", __repr_args__)If this is out of scope, add devtools to the docstring's list of print paths that still show plaintext.
Found by 1 model: claude
| /// The code and message of `error`, with no field or reason. | ||
| pub fn of(error: &dyn Diagnostic) -> Self { | ||
| let message = match error.help() { | ||
| Some(help) => format!("{error} {help}"), |
There was a problem hiding this comment.
Fix in a follow-up: Refusal::of joins the engine message and its help with a single space, so the Python message has no full stop between two sentences.
Impact: Most engine messages do not end with punctuation. A Python user then reads one broken sentence. This also changes the plan refusals that plan::check raises at the class statement. Before this PR they used error.to_string() only. Now the help is added, and some of that help names the Rust/JSON plan format, not the Python declaration.
Evidence: UntypedIndex becomes record plan field "x" has a term output and no declared type Declare the field's "type" ("string", "int64", ...): its index terms derive from that type. Auth variants that credentials::message does not map, such as TokenExpired or InvalidToken, are joined the same way. The doc comment on message says "A caller whose engine help names a Rust API writes its own message", but plan.rs does not write its own message.
Fix: Add a full stop when the message does not end with one. Also decide whether plan refusals should carry the engine help. If not, keep field/reason from Refusal::of and set message: error.to_string() in plan::refusal.
let text = error.to_string();
let message = match error.help() {
Some(help) if text.ends_with(['.', '!', '?']) => format!("{text} {help}"),
Some(help) => format!("{text}. {help}"),
None => text,
};Found by 1 model: claude
| /// no variable at all. `None` keeps the engine's message and help. | ||
| fn message(error: &AuthError) -> Option<String> { | ||
| let text = match error { | ||
| AuthError::NotAuthenticated(_) => "no credentials were found. Set CS_CLIENT_ACCESS_KEY \ |
There was a problem hiding this comment.
Fix in a follow-up: NotAuthenticated does not always mean that no credentials exist, but the new message says "no credentials were found".
Impact: A user who ran stash auth login can still get this message. The message then tells them to do the thing they already did. They have no hint that the profile file is the problem.
Evidence: packages/stack-auth/src/device_session_strategy.rs (build_from_workspace_store) returns AuthError::NotAuthenticated in two more cases. One is a stored token with no region(). The other is a stored token with no client_id(). In both cases detect found the profile and loaded it.
Fix: Write a message that covers both cases. Or name the profile in the message, the same way the Store arm already does.
AuthError::NotAuthenticated(_) => "no usable credentials were found. Set CS_CLIENT_ACCESS_KEY \
and CS_WORKSPACE_CRN, or log in again with `stash auth login` (the profile is in \
~/.cipherstash, or in CS_CONFIG_PATH when set)."
.to_owned(),Found by 1 model: claude
| .map_err(|error| { | ||
| let refused = Refusal::of(&error); | ||
| Refusal { | ||
| message: message(&error).unwrap_or(refused.message.clone()), |
There was a problem hiding this comment.
Optional: unwrap_or(refused.message.clone()) clones the default message every time, also when message(&error) returns a replacement.
Impact: This is a small, needless allocation on a cold path. The bigger cost is that the closure is harder to read than it has to be.
Fix: Move the message out of refused instead of cloning it.
.map_err(|error| {
let mut refused = Refusal::of(&error);
if let Some(text) = message(&error) {
refused.message = text;
}
refused
})Found by 1 model: claude
| return UNSET | ||
| except Exception: # noqa: BLE001 A repr never raises. | ||
| return UNAVAILABLE | ||
| return UNSET |
There was a problem hiding this comment.
Change before merge: no test runs repr on a real SQLAlchemy instance whose column is not loaded, which is the main reason _value exists.
Impact: The docstring says a SQLAlchemy column that is not loaded must run no SELECT. The only test for this uses a hand-written __getattr__, and that test passes through the except AttributeError: return UNSET branch at line 128. It never reaches this line, where a non-storage descriptor such as SQLAlchemy's InstrumentedAttribute returns UNSET. If someone changes this branch to call getattr, every repr of an expired or detached row runs a query or raises DetachedInstanceError, and no test fails. The MappedUser test in KINDS builds a transient instance with every column set, so it reads only __dict__.
Evidence: tests/test_redaction.py has test_a_passthrough_value_is_read_without_running_a_descriptor, which uses a plain class. No test uses a Session.
Fix: Add a test to tests/test_redaction.py that commits a MappedUser to an in-memory SQLite database and counts the SQL statements while repr runs:
from sqlalchemy import create_engine, event
from sqlalchemy.orm import Session
def test_an_expired_sqlalchemy_column_runs_no_select():
engine = create_engine("sqlite://")
Base.metadata.create_all(engine)
statements = []
event.listen(engine, "before_cursor_execute", lambda *args: statements.append(args[2]))
with Session(engine) as session:
user = MappedUser(email=SECRET, age=42)
session.add(user)
session.commit() # expires every attribute
statements.clear()
assert repr(user) == "MappedUser(email=<sealed>, age=<unset>)"
assert statements == []
# Detached: the read would raise, and repr must not.
assert repr(user) == "MappedUser(email=<sealed>, age=<unset>)"The test expects repr to show <unset> for the expired column and to send no statement to the database.
Found by 1 model: claude
| setattr(cls, DECLARATION_ATTRIBUTE, Declaration(table=table, fields=fields)) | ||
| methods: Tuple[Callable[..., Any], ...] = () | ||
| if redact: | ||
| _check_no_hand_written_methods(cls) |
There was a problem hiding this comment.
Optional: a hand-written __rich_repr__ is refused by the code, but no test checks it.
Impact: METHODS has three names. The tests check the refusal for __repr__ and __str__ only. If __rich_repr__ is dropped from METHODS, or hand_written stops matching it, the decorator replaces the user's __rich_repr__ with no error. The user's code then never runs, and nothing tells them.
Evidence: tests/test_redaction.py has test_a_hand_written_repr_is_refused and test_a_hand_written_str_is_refused. No test defines __rich_repr__.
Fix: Replace the two tests with one test over all three names:
@pytest.mark.parametrize("name", ["__repr__", "__str__", "__rich_repr__"])
def test_a_hand_written_method_is_refused(name):
def method(self):
return "plaintext"
namespace = {
"__module__": __name__,
"__annotations__": {"email": Annotated[str, TextEq]},
name: method,
}
User = dataclasses.dataclass(type("User", (), namespace))
error = pytest.raises(DeclarationError, stackencrypt.model("users"), User).value
assert error.code == "declaration_repr_present"
assert name in str(error)The test expects each of the three methods, written on the class, to raise declaration_repr_present and to name the method.
Found by 1 model: claude
| ), | ||
| default=None, | ||
| ) | ||
| if changed is not None: |
There was a problem hiding this comment.
Optional: check_unchanged is tested only for an added annotation and a replaced __repr__.
Impact: The code compares the union of the old and new hint names, so it also catches a changed hint and a removed hint. It also checks __str__ and __rich_repr__. No test covers these cases. If the comparison is changed to loop over hints only, a deleted field passes Cipher with no error. If the zip with _redact.METHODS gets out of order, a replaced __str__ is not found.
Evidence: tests/test_cipher.py has test_an_annotation_added_after_decoration_is_refused and test_a_replaced_repr_is_refused only.
Fix: Add two parametrized tests to tests/test_cipher.py:
@pytest.mark.parametrize(
"change",
[
lambda hints: hints.__setitem__("age", Annotated[str, Passthrough]),
lambda hints: hints.pop("age"),
],
ids=["changed", "removed"],
)
def test_a_changed_or_removed_hint_is_refused(credentials, change):
User = declared()
change(User.__annotations__)
error = refusal(lambda: Cipher(User))
assert error.code == "declaration_class_changed"
assert error.reason == "fields"
assert error.field == "age"
@pytest.mark.parametrize("name", ["__str__", "__rich_repr__"])
def test_a_replaced_str_or_rich_repr_is_refused(credentials, name):
User = declared()
setattr(User, name, lambda self: self.email)
error = refusal(lambda: Cipher(User))
assert error.reason == "methods"
assert f"{name} replaced" in str(error)The tests expect each change to raise declaration_class_changed with the right reason, and with the right field for a hint.
Found by 1 model: claude
Summary
A declared class no longer prints plaintext:
repr,strand rich's__rich_repr__show<sealed>for every sealed field.Cipher(User)now exists. It checks that the class is declared and unchanged, then finds the credentials the way every SDK does, with no request.encrypt,decryptandquerycome in later tickets.Changes
Redaction (
_redact.py).@stackencrypt.modelreplaces__repr__,__str__and__rich_repr__. A passthrough field shows its value, a sealed field shows<sealed>, and anIgnorefield is left out. A value is read from the instance's__dict__or a storage slot only, so a SQLAlchemy column that is not loaded runs no SELECT. A generated method is replaced. A hand-written one is refused withdeclaration_repr_present, unless the decorator is givenredact=False.Cipher(_cipher.py).Cipher(cls)refuses an undeclared class, a subclass of a declared class, or a non-class withdeclaration_not_decorated. A class changed after decoration isdeclaration_class_changed, with reasonmethods,fieldsorconstructor. Then it calls the engine for the credentials. A refusal is anAuthenticationErrorwith the engine's code, such asstack_auth_not_authenticated.Credentials (
crates/core/src/credentials.rs). stack-auth'sAutoStrategyreadsCS_CLIENT_ACCESS_KEYandCS_WORKSPACE_CRN, then the profilestash auth loginwrites. It is given a transport that sends nothing, so no HTTP client is built and thehttpfeature stays off. Each refusal names the variable or file to fix.Placeholder. Python prints
<sealed>, not Go's[sealed].[sealed]is a valid tag in rich's markup, sorich.print(repr(user))would show the field as empty. The ADR and the spec record it.Verification
NamedTupleand SQLAlchemy.cargo test --locked, clippy,cargo fmt --check, mypy strict, and pyright are clean.pnpm run test:scripts: 83 of 83 files pass.Related
Closes #1388. Part of #1341.
Review notes
_redact.hand_writtenand_value, thencheck_unchangedin_declaration.py.dataclasses.asdict,model_dumpandvarsstill return plaintext, by design.Ignorefield underAnnotatedloads as one object, not a list. That is a spec problem, noted on Python SDK: core call over psycopg, on EQL v4 and Stack Encrypt #1341.🤖 Generated with Claude Code