Repository navigation
feat(stack-encrypt)!: codes, help and fields on every error in stack-encrypt, stack-kms, stack-auth and stack-profile - #1103
Conversation
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 52 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (7)
📒 Files selected for processing (41)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe PR adds structured diagnostic codes, help text, and payloads across Rust crates. It also changes selected error messages to omit input or wrapped-error text, adds field and reason details to dynamic encryption errors, and updates related tests and bindings. ChangesShared Error Diagnostics
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~55 minutes Change: Feature Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established for this change. It is ready for normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 5 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Most coding requirements in [
Comment |
🦋 Changeset detectedLatest commit: e3400e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 106 | 3 | 77 | 0 |
Surviving mutants: add a test that fails on each before merging
packages/stack-auth/src/error.rs:117:9: replace RequestError::is_no_transport -> bool with false
packages/stack-auth/src/error.rs:124:9: replace RequestError::message -> &'static str with ""
packages/stack-auth/src/error.rs:124:9: replace RequestError::message -> &'static str with "xyzzy"
5249770 to
b52dac4
Compare
b52dac4 to
7e8e00f
Compare
|
Rebased onto #1094 at 6efc22b, which fixes the second cipherstash-bot review there (index-only fields and nil interface passthroughs now round-trip). Two conflicts came up, in |
444bb02 to
f59cc2b
Compare
f59cc2b to
fe489bc
Compare
39f2813 to
b9b082d
Compare
|
Rebased onto the new #1096, with no conflicts and no new commits. Go tests pass locally against freshly built guests. |
|
Red on b9b082d: CI tooling, not this PR's code. Fixed by #1111. Four checks fail on b9b082d: All four were re-run once and failed the same way. The tool pins in The cause is a mise cache key shared across runner images. The Fix: #1111 ( Generated by Claude Code |
…kept list The same change as #1103 makes for the stack crates: ERROR_CODES was read only by its own test. The test now builds every GuestError through a match with no wildcard, so a new variant fails to compile until it has a row, and checks each code is in this crate's namespace and snake_case. Refs #1100 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
0dfb025 to
0c7ccd7
Compare
6126f19 to
4c081d1
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
4c081d1 to
a2cca12
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The first comment is a policy break: ciphertext or index bytes can get into a message and the payload through the Go guest.
| #[derive(Debug, thiserror::Error, miette::Diagnostic)] | ||
| #[error("Token store error: {0}")] | ||
| #[diagnostic(transparent)] | ||
| pub struct StoreError(pub stack_profile::ProfileError); |
There was a problem hiding this comment.
StoreError has no #[source]. thiserror does not use the tuple field 0 as the source. Thus AuthError::Store(..).source() returns None. I checked this on the PR head: AuthError::from(ProfileError::Json(..)) displays Token store error: JSON error: unexpected data at line 1 column 3, with source = None.
This PR removes the parser and I/O text from the ProfileError messages and says that it "remains available through source()". Through the auth path, that is not true: the serde_json::Error and io::Error cannot be reached from an AuthError.
Fix: pub struct StoreError(#[source] pub stack_profile::ProfileError);. The message then shows the profile error two times in a full chain report. If that is a problem, remove {0} from the #[error] text.
There was a problem hiding this comment.
Fixed in 05c2c2a: StoreError(#[source] pub ProfileError). A new test builds AuthError::from(ProfileError::Json(..)) and follows source() down to the serde_json::Error.
I kept {0} in the message. The TypeScript binding shows only the message, so without it a store failure would read just "Token store error". As a result, a full chain report shows the profile error twice; the doc on StoreError says so.
Generated by Claude Code
| #[diagnostic( | ||
| code(stack_auth::request_error), | ||
| help( | ||
| "The auth server could not be reached, or its response could not be read. Check the network path to it; the transport's error is this error's source." | ||
| ) | ||
| )] |
There was a problem hiding this comment.
The help text is incorrect for one of the errors that this type carries. A build without http returns RequestError(NoTransport) from transport::resolve when you do not set a transport. That error sends no request, but the help tells the user to "check the network path". The actionable text ("give one with .transport(..)") was in the message before this PR. Now it is only in the source, which a binding does not show.
Fix: add a variant or a separate error type for the missing transport, with its own code and help. Alternatively, keep the NoTransport text in the message, because it is fixed text from this crate.
There was a problem hiding this comment.
Fixed in 05c2c2a. RequestError now implements Diagnostic by hand. When the boxed error is NoTransport:
- the message is the fixed no-transport text, which says to pass
.transport(..); - the code is
stack_auth::no_transport; - the help says to pass a transport or build with
http.
The legacy code stays REQUEST_ERROR, so the TypeScript and Go mappings do not change. I kept the tuple struct's shape because host transports, including the Go auth guest's, build it directly. The network help no longer tells the reader to look at source(). The no-http test now checks the message, code and help.
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 4 review job(s) failed)
One change is needed before merging: add a regression test for the INVALID_TOKEN message change in stack-auth, because the changeset promises it to TypeScript users. All other findings are follow-ups or optional, and can wait.
The reviews found no message, help text or payload in the diff that carries a secret. Two follow-ups matter most to callers. TypeScript users of @cipherstash/auth get no detail about network failures. StackCipher::builder().init() reports every setup failure with the one code stack_encrypt::config.
Findings checked and not kept
describe_json_errortests only the syntax category: not kept.a_json_error_does_not_quote_the_fileparses"marker-token"as au8, which is a data error, not a syntax error. The four-arm mapping has little else to test.- No test excludes the body and headers from the
FailureResponsepayload: not kept. The encrypt guest leak test in #1104 sends a marker response body throughstack_kms::classify_response, and searches every cause for it.
Other findings not posted as comments
- Fix in a follow-up: no test checks the miette codes of
DeviceClientError. A new variant with no#[diagnostic(code(..))], or with a code that differs from theAuthErrorit converts into, passes every test. Thevariants!macro inerror.rscan build one row per variant.packages/stack-auth/src/device_client.rs:43 - Fix in a follow-up: no test checks the
ProfileErrorpayload keysfilename,workspace_idandcolumn. Tests checkpathandio_kind, and #1110 checksline. A table test over all variants would check each key.packages/stack-profile/src/error.rs:70 - Optional:
refusaldrops the originalPlanError. AReason::Refusederror then has no cause, and it loses fields such as the index name ofIndexNotDeclared. This PR already changes these variant shapes, so a cause field costs least now.packages/stack-encrypt/src/dynamic/record.rs:507 - Optional: the stack-auth
every_variantlist is written by hand. A newAuthErrorvariant that reuses a frozen code gets no miette code check. Thevariants!macro would make a missing row fail to compile.packages/stack-auth/src/error.rs:1493 - Optional:
query()reportsNoSuchFieldwith the requested name, andNotATarget, but its tests check onlyError::Plan { .. }.packages/stack-encrypt/src/dynamic/record.rs:1143 - Optional:
ServerError::refusedhas no test for a JSON body with noerror_description, or with a blank one. Both must give the status alone.packages/stack-auth/src/error.rs:395 - Optional: the stack-kms tests check one payload field,
request_kindonKeysetNotFound. Therequest_kindof retrieve and generate failures, the key counts,statusandenv_varare not checked.packages/stack-kms/src/errors.rs:9 - Optional: the help text of
StackKmsBuilderError::InvalidEndpointuses{env_var}, and no test reads that help.packages/stack-kms/src/builder.rs:45 - Optional:
check_treegivesRepeatedKeyfor a repeated key inside a sealed value, but the source and record tests check only the variant.packages/stack-encrypt/src/dynamic/record.rs:1519
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 4 found, 3 posted |
| claude | claude-opus-5-5 | rust | 5 found, 5 posted |
| codex | gpt-5.6-terra | test-gap | 3 found, 0 posted |
| codex | gpt-5.6-terra | rust | failed |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 3 comment(s) had a problem that stopped the reader acting; it rewrote 3.
Stack: position 6 of 8 (#1090, #1093, #1094, #1095, #1096, *️⃣ #1103, #1104, #1110). *️⃣ marks this pull request.
Context loaded: the description, 4 linked issue(s) and 9 discussion entries.
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Before merging, add one regression test: a JWT whose claims do not decode must be reported without the token's text. All other findings can wait for a follow-up or are optional. The code itself shows no defect that a caller can reach today. The gaps are one doc that does not agree with the code, one policy exception in LabelError, and missing tests for fields and reasons that Go callers read.
Other findings not posted as comments
- Optional: the
request_kindpayload value isformat!("{:?}", error.kind)ofzerokms_protocol::ViturRequestErrorKind, a type thatcipherstash-suiteowns. A rename there changes a value that Go callers compare, and nothing in this repository fails. A localmatchwith no wildcard would make that a compile error.stack-profile'sio_kindhas the same problem withstd::io::ErrorKind. (packages/stack-kms/src/errors.rs:10,packages/stack-profile/src/error.rs:73) - Optional:
is_code_ofis a test helper, butstack-auth,stack-kmsandstack-encryptre-exportstack_profile::diagnostic, so it is public API of four published crates. Add#[doc(hidden)]. (packages/stack-profile/src/diagnostic.rs:139) - Optional: no test checks that each
DeviceClientErrorvariant has the same code as theAuthErrorit converts into, which its doc promises.DeviceClientErroris also not in any test that builds one of every variant. (packages/stack-auth/src/device_client.rs:43) - Optional:
describe_json_errorhas a direct test only for theDatacategory, andProfileError::payload()has payload tests only forIo,JsonandNotFound(in the doctest). TheIo,SyntaxandEofdescriptions, and thefilenameandworkspace_idfields, are not checked. (packages/stack-profile/src/diagnostic.rs:124,packages/stack-profile/src/error.rs:70) - Optional:
Error::InvalidEndpoint(endpoint from the token'sservicesclaim) andStackKmsBuilderError::InvalidEndpoint(endpoint from an environment variable) share the codestack_kms::invalid_endpoint. A caller can tell them apart only by whetherenv_varis in the payload. (packages/stack-kms/src/errors.rs:612,packages/stack-kms/src/builder.rs:44) - Optional: neither wrapper of
InvalidEndpointreturns its payload, so theschemefield ofInvalidEndpoint::Schemeis not in the top-level payload. (packages/stack-kms/src/errors.rs:633) - Optional: a
dynamic::Error::Termraised throughIndexSpec'sIndex<Value>reaches the caller insidecrate::Error::Other, so it reportsstack_encrypt::otherand notstack_encrypt::dynamic_term. Only Rust callers that runIndex<Value>directly get this. (packages/stack-encrypt/src/cipher.rs:196) - Optional:
ServerError::refusedcopies the auth server'serror_descriptioninto the message with no length limit, and no test covers an empty, whitespace-only or non-string description. (packages/stack-auth/src/error.rs:395) - Optional:
context()refusals are checked only by variant: no test checksReason::EmptyContextorReason::ContextNotUtf8fromcontext()itself. (packages/stack-encrypt/src/dynamic/context.rs:98) - Optional:
check_fieldadds the field name to anError::Term(record.rs:1607), but no test reaches that path throughcheck_sourceorencrypt.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 7 found, 5 posted |
| claude | claude-opus-5-5 | rust | 5 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 2 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 6 comment(s) had a problem that stopped the reader acting; it rewrote 6.
Stack: position 6 of 8 (#1090, #1093, #1094, #1095, #1096, *️⃣ #1103, #1104, #1110). *️⃣ marks this pull request.
Context loaded: the description, 4 linked issue(s) and 9 discussion entries.
b19f477 to
a9586c5
Compare
…tain, and codes for ProfileError Go callers get a status number when a guest call fails, and nothing else (#1098). Carrying more across means every error in stack-profile, stack-auth, stack-kms and stack-encrypt needs a stable code, help and structured fields, and a written rule for what those may hold. This is the first of the four crates, and the one the other three depend on, so the shared pieces live here. `ErrorPayload: miette::Diagnostic` gives an error's structured fields (`payload()`, a serde_json map shaped like stack-auth's `AuthErrorKind::payload`). Its docs carry the rule: keyset ids, counts, field names and the like are allowed; plaintext, key material, tokens, ciphertext and term bytes, and raw context values never are; context descriptors, another library's message and ZeroKMS response bodies are left out by default, each with its reason. `diagnostic::is_code_of` checks a code's shape (`crate::snake_case_name`), and `describe_json_error` renders a serde_json error without quoting input. `ProfileError` derives `miette::Diagnostic` with a `stack_profile::*` code on every variant, listed in `ERROR_CODES` and pinned by a test that builds every variant. `Io` names the I/O error's kind and `Json` gives the error kind, line and column instead of the parser's message: serde_json quotes the value it refused, and `auth.json` holds tokens. Both wrapped errors are still the source. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
Every stack-auth error now carries a path-style miette code (`stack_auth::invalid_crn`), the one source of truth #1099 settles on for what crosses a binding. The frozen codes stay exactly as they were: `error_code()`, the `INVALID_CRN`-style strings and `from_error_code` are the `type` of the published TypeScript `AuthFailure`, and two of them are what the token service sends back. A test builds every `AuthError` variant and pins its frozen code and its miette code side by side, so the two cannot drift; another checks every code an error here can produce is in the new crate-level `ERROR_CODES`. `InvalidAccessKey` gets its own four codes. `StoreError` is diagnostic-transparent: a profile failure that comes through the auth path carries `stack_profile::not_found` and the profile error's help, the same as one straight from the store, while its frozen code stays `STORE_ERROR`. `DeviceClientError` carries the code of the `AuthError` each variant converts into. `AuthError` implements the shared `ErrorPayload` with the fields `AuthErrorKind::payload` already gives. Messages follow the rule now written next to `ErrorPayload`: `RequestError` no longer repeats the transport's message (it can carry a URL with its query string) and makes it the source instead; a token whose claims do not decode no longer quotes the decoder's message; a failed device binding gives ZeroKMS's status, not its response body. Help is added where a caller can act. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
stack-kms already derived `miette::Diagnostic` but gave no codes and no help, so a ZeroKMS failure reached a binding as its message alone. Every error here now has a `stack_kms::*` code (`stack_kms::keyset_not_found`), listed in `ERROR_CODES` and pinned by a test that builds every variant; the top-level `Error` and `StackKmsBuilderError` forward the code of what they carry. Each implements the shared `ErrorPayload`: a failed ZeroKMS request gives its request kind and a count mismatch both counts, and nothing from a response body. `KeysetNotFound`'s help says what its source has always said, that ZeroKMS answers 404 for an unknown client too. Messages follow the rule next to `ErrorPayload`: `FailedRetrieval` keeps ZeroKMS's per-key reason on the variant but out of the message; `GenerateIv` and `ConnectionInitError` no longer repeat another library's message (it is the source); and `InvalidEndpoint` no longer echoes the URL it refused, which can still carry credentials or a query string at the point those checks run. `diagnostic` and `ErrorPayload` are re-exported here, so stack-encrypt reaches them through this crate. The stack-kms fuzz lockfile also gains the `sha2` dependency stack-auth already declares, which it was missing. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…input errors name the field stack-encrypt was the one crate in the chain with no miette support: its errors were plain thiserror enums with no codes and no help, and the dynamic module's input errors were unit variants that could not say which field was wrong. A Go caller got "malformed input" for a bad plan, a record that did not fit it, an empty context and an over-long one alike. Every error type here now derives `miette::Diagnostic` with a `stack_encrypt::*` code, listed in `ERROR_CODES` and pinned by a test that builds every variant: `Error`, `PlanError`, `LabelError`, `LeafBytesError`, `sem::TermError`, `sem::TermBytesError`, and with `dynamic`, `dynamic::Error` and `dynamic::TargetError`. Help is added where a caller can act (`ForeignKeyset`, `DescriptorTooLong`, `EmptyTermText`, the plan refusals), and each implements the shared `ErrorPayload`: both keyset ids of a `ForeignKeyset`, the field and expected type of a `FieldType`, a descriptor's length against its limit. `Error::Kms`, `Term` and `Plan` forward the code of what they carry; `Kms` is now transparent outright, so a ZeroKMS keyset-not-found reads as itself. BREAKING CHANGE: `dynamic::Error::Context`, `Plan`, `Source` and `Record` are struct variants carrying `field: Option<String>` and a `Reason` from the new fixed `dynamic::Reason` enum (`MissingContext`, `DuplicateOutput`, `FieldMissing`, `NoCiphertextNode`, ...), and `Term` gains `field`. Every site that raised one now names the field it knows, plan builder and engine refusals included; `in_field` names it from the caller's side for functions that never see one. `Error::Kms` loses its message prefix. `ContextMismatch` gives the stored context's length and part count rather than its descriptor, which a context field can fill with customer data, and `TermError::Prf` and `MatchPositionOutOfRange` stop repeating another library's message and a value read from term bytes. The Go guest's status mapping matches the new shapes; its numbers are unchanged. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…s two 36-arm matches CI on the codes PR failed four ways; this answers each. The payload impls added for #1099 had no test that read their fields, so cargo-mutants could replace any of them with an empty map unnoticed, and CRAP scored TargetError::payload, PlanError::payload and refusal() as untested. A table in codes.rs now pins one variant per arm of every payload impl, a table in record.rs pins every arm of refusal(), and device_client gains a test that the Server payload is the status and never the body. Reason::as_str and its Display were 36-arm matches, and CRAP is never below cyclomatic complexity, so no test could clear them. Reason is now declared through a macro from one list of variant, snake_case name and phrase; the enum, ALL and the lookup table all come from it, so a reason cannot be added without both strings, and both methods index the table. Every name, phrase and the ALL order are unchanged. The no-http transport test read the hint from the message, which is now fixed text; it reads it from the source. The publish dry-run adds stack-profile and stack-auth so stack-kms and stack-encrypt build against the tree's versions of them rather than the older ones on crates.io. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…ever the body The Go binding's test for a 403 from the edge in front of the auth server showed the access-key and OIDC refreshers putting the whole response body into ServerError's message: "Server error: 403: <html>...nginx CSAK...</html>". A body from the edge is an HTML page, and any body may echo the credential the request carried, so under the rule on ErrorPayload it never belongs in a message. With se_last_error that message now crosses into every binding. ServerError::refused builds the message from the status and, when the body is the auth server's JSON error, its error_description, which the rule allows. ServerError::unparseable replaces serde_json's message, which can quote the body, with where the JSON broke. The refresh-lock join failure no longer repeats tokio's error text. The body is still logged at debug level where it was before. The test that asserted the body was in the message now asserts it is not. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
Explain error details and migration steps in plain language. Refs #1099
Show developers how to inspect codes, help, and payload fields, with an example and guidance for safely reporting underlying errors. Refs #1099
…of codes Each of stack-profile, stack-auth, stack-kms and stack-encrypt carried a public ERROR_CODES list that only its own tests read: a test built every variant, required its code to be listed, and required every listed code to be produced. Adding or renaming a code meant editing the list too, and the list still could not catch a variant missing from the test's rows. The lists are gone. Each crate's test builds one of every variant and checks its code is in the crate's namespace and snake_case, or, for a variant that wraps another crate's error, in that crate's. The rows are written `pattern => value` through a small test macro whose patterns are the arms of a match with no wildcard, so a variant without a row fails to compile, and each row must build the variant its pattern names. That found two StackKmsBuilderError variants, Auth and InvalidConfig, the old test never built. AuthError::ERROR_CODES, the frozen list the TypeScript bindings and the Go auth guest read, is unchanged. A renamed code now shows only as the changed #[diagnostic(code(...))] line in review. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
… kind and position TargetError::Stored's reason goes into the payload as well as the message, and the Go encrypt guest filled it with serde_json's own message. serde_json quotes the value it refused, and the stored EQL value holds ciphertext and index terms, so both could carry them. The guest's convert, and the test resolver in dynamic/record.rs that showed the same pattern, now write describe_json_error's kind, line and column. The reason field's doc states the rule. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…rt says so StoreError had no #[source], and thiserror does not take a tuple field as one, so AuthError::Store(..).source() was None. The profile error, and the parser or I/O error under it, could not be reached from an AuthError, though ProfileError drops their text from its message on the promise that source() keeps it. A build without `http` whose strategy has no transport returned a RequestError whose help said to check the network path, though nothing was sent; what to do was only in the source, which a binding does not show. RequestError now implements Diagnostic by hand: that case has its own message, code (stack_auth::no_transport) and help, all fixed text, and keeps the legacy code REQUEST_ERROR. The network help no longer points at a source() that TypeScript cannot read. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
PlanError::IndexOptions put `declared` and `asked` in the payload as Rust Debug text, which is no format: a Go caller cannot parse it, and it changes whenever MatchOptions gains a field. Its test built the expected value with the same format!, so it could not notice. They are now the index as a plan writes it: the key, or the object of all four match options. stack-kms's request_kind and stack-profile's io_kind were Debug text of another crate's enum. Both are now written out: request_kind by a match with no wildcard, so a new zerokms-protocol kind fails to compile rather than reaching a caller unseen; io_kind by a table of the kinds a profile store meets, `Other` for the rest. Both keep the spellings callers already compare. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…haracter LabelError's doc called a label schema, and so let Reserved quote the character it refused, in its message and as a `character` payload field. But context_field builds a label from a record's own field, and the policy in stack_profile::diagnostic says a context can hold customer data and is reported by length and parts, never content. Reserved now names the segment by position only, like the other variants. The doc says why, and that `found` is still on the variant for a caller in the same process. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
…T and carries the builder's fields StackCipher::builder().init() reports every setup failure as Error::Config. Its help listed four variables but not CS_ZEROKMS_HOST, so a bad endpoint was sent to check the others, and its payload was empty though the builder error inside it has fields (the variable at fault). The help now names all five and points at the message, and the payload is the builder error's. The code stays stack_encrypt::config: the box keeps the enum's shape the same with and without `http`, and no binding reaches this path (the Go guest builds its cipher with an explicit key source). Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
… read The review found error details a caller reads that no test checked, so a swapped reason or a dropped key would pass: - a JWT whose claims do not decode quotes nothing from the token, the behaviour the @cipherstash/auth changeset promises; - the help on REQUEST_ERROR, INVALID_GRANT, INVALID_WORKSPACE_ID, ALREADY_CONSUMED and STORE_ERROR; - the field and reason of a target field's misfit EQL node, of a query on an unknown or non-target field, and of three refused plans (two fields under one identity, an empty context, a repeated match option); - stack-kms's scheme, status, key-count and env_var fields, and the builder help naming its variable. IndexSpec::from_value's doc said a repeated match option is UnknownOutput; the code says RepeatedKey, which is right, and the doc and the test now say so too. is_code_of is a test helper that four crates' tests need public: it is hidden from the docs so it is not part of their API. Refs #1099 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
a9586c5 to
e3400e2
Compare
4 participants
Summary
Rust errors now provide stable codes, actionable help, and structured details across encryption, key management, authentication, and local login storage. This gives applications enough information to explain failures and handle specific cases, such as a value encrypted with another keyset, without parsing message text. The PR also defines what error details may contain and removes several messages that could expose credentials or customer data.
This is the first of three PRs adding detailed errors to Go: this PR defines the Rust details, #1104 transports them across the runtime boundary, and #1110 exposes them to Go callers.
Changes
ErrorPayloadand diagnostic helpers instack-profile, the shared dependency of all four crates. Errors expose structured fields such as keyset identifiers, field names, expected types, operation kinds, counts, and lengths.stack_encrypt::foreign_keyset. Wrapped errors can pass through the underlying code. Each crate's tests build one of every variant and check that its code is in the crate's namespace andsnake_case. There is no hand-kept list of codes to update: the test rows go through a match with no wildcard, so a variant without a row fails to compile. Tests also check structured fields. Existing authentication code strings, including the frozenAuthError::ERROR_CODESlist the TypeScript bindings read, remain unchanged.source(). Key-management errors now pass through without the previous message prefix.@cipherstash/auth, updates the encryption changelog and lockfiles, and includes all five dependent crates in the publication dry-run.Verification
The existing PR description reports the following checks. This description edit did not rerun them or check current CI status.
cargo fmt --all --check;cargo clippy --workspace --all-targets --all-features -- -D warnings; thewasm:no-http-testloop for authentication, key management, and encryption; authentication and encryptioncargo testruns with default and all features; and rustdoc with-D warnings. All passed.eql-bindings --features stack-encryptandeql-encryption-tests, and the trybuilduisuite passed. A script confirmed all 35 reason names, phrases, and their list order were unchanged after the rewrite.5249770: Formatting, clippy, nextest, documentation, the five-crate publication dry-run, complexity/coverage checks, mutation testing (61 caught, 0 missed), WebAssembly and Go checks, macOS/Windows bindings, and Go live tests passed. Initial publication, no-HTTP transport, coverage/complexity, and mutation failures were fixed before that run.b52dac4; the tree changed only in the base's two JavaScript dependency files. This is historical status, not a claim about current CI.d485be6c):cargo fmt --all --check, workspace clippy with-D warnings, rustdoc with-D warnings, and the four crates' unit tests with default, all, and no default features passed locally. Deleting a row was checked to fail compilation, and a row that builds the wrong variant was checked to fail the test. The new check found twoStackKmsBuilderErrorvariants,AuthandInvalidConfig, that the old test never built; they now have rows. One stack-kms builder test failed once in a multi-crate threadedcargo testrun, which is the environment-variable race noted inAGENTS.md. It passed single-threaded and on five repeated threaded runs. nextest was not available locally. CI on this commit has not been checked, and the coverage and test jobs are expected to fail on the shared tool cache until the stack is rebased onto main with fix(ci): key the mise cache by runner image #1111.Related
Review notes
stack-profileandstack-authmust be version-bumped, with dependent workspace requirements updated, before or alongside publishingstack-kmsandstack-encrypt. The published 0.43.0 dependencies lack the new helpers. The dry-run checks packaging together; it does not prove release order. Version bumps are deferred to the repository's manual Rust release PRs.Error::OtherandTargetError::Othermessages remain visible to preserve EQL's unsupported-payload error. Implementations are responsible for their content. Configuration errors retain the key-management builder's message. The policy also allows profile paths, authentication-server descriptions, and fixed library messages such as URL parse errors.STORE_ERRORcode. TypeScript network errors now use fixed text and help; refused exchanges and device binding failures omit raw response bodies. ZeroKMS is CipherStash's key-management service.#[diagnostic(code(...))]line in review. Go callers may branch on codes, so treat a rename as an API change.Error::Other; changing that requires a separate EQL change and release. Error documentation URLs remain unset until per-code pages exist.Summary by CodeRabbit