Repository navigation
feat(golang): every failure a guest reports is a Diagnostic that unwraps to its sentinel - #1110
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe shared Go guest package now retrieves optional error details after guest-call failures and returns diagnostics that unwrap to existing sentinel errors. The auth and encrypt packages expose diagnostic types. Tests and documentation cover diagnostic content, sentinel matching, and compatibility with guests that omit the export. ChangesGuest error diagnostics
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GuestCall as guest.Call
participant Diagnose as Exports.diagnose
participant LastError as se_last_error
participant Memory as Guest memory
GuestCall->>Diagnose: Pass PackedResult error
Diagnose->>LastError: Request error details
LastError-->>Diagnose: Return packed detail location and length
Diagnose->>Memory: Copy and wipe detail bytes
Diagnose-->>GuestCall: Return Diagnostic or status sentinel
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in this change after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Existing error-kind checks and compatibility with older runtimes are preserved. Diagnostic retrieval has cleanup and failure-containment controls. No sensitive-data disclosure was established, but verification of every transport-error path remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 73.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 23 files. (1 skipped: 1 unsupported.)
Comment |
8faaf44 to
d18bfbc
Compare
d18bfbc to
6056eab
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 |
2705d06 to
20b9c50
Compare
20b9c50 to
8e95495
Compare
8e95495 to
8968ef1
Compare
8968ef1 to
f6bb5db
Compare
245bb8d to
f2c025d
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. |
631e755 to
fbdb56f
Compare
05ea7c1 to
d21aa96
Compare
d21aa96 to
9e38186
Compare
|
Rebased onto the new #1104 (no conflicts) and addressed the review in 9e38186. Every inline thread has a reply. The two findings in the review body that had no inline comment:
|
9e38186 to
2ea9a96
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
I examined the Go error design in this PR. The design is correct for errors.Is and errors.As. All guest calls go through guest.Call, so each status that a guest reports goes through diagnose. diagnose never returns a typed-nil *Diagnostic: it returns the *Diagnostic or the bare kind. Each %w wrap in encrypt and auth keeps the Diagnostic in the chain. I did not find a == comparison on a sentinel in non-test code.
I did not repeat the earlier comments. The author fixed those in 9e38186.
Findings:
- Medium (design / compatibility):
Diagnostic.Error()returns only the Rust message. The kind text (cipherstash: ...) is not in the error text now. See the inline comment. - Low (test gap): No test covers the
context.WithoutCancelindiagnose. - Nit (API): A caller outside this module cannot make a
Diagnosticthat unwraps to a sentinel. This makes fakes in caller tests difficult. - Nit (stale comment, not in the diff): The comment on
authHTTPStatus.wrapinauth/transport.go(line 61) gives the example text"cipherstash: auth transport failed: HTTP 403". The test now expects"Server error: 403: HTTP 403". Update the example.
2ea9a96 to
968a754
Compare
968a754 to
48b5a31
Compare
|
On the review's other points:
The stack is restacked: #1096 → #1103 → #1104 → this PR. Generated by Claude Code |
48b5a31 to
bdd7d6d
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-review at bdd7d6d. The branch was rebased. The PR's main commit (80d68a7) has the same Go changes as 2ea9a96. The three new Go commits fix my earlier findings.
Status of the earlier findings:
- Medium,
Error()without the kind text: fixed (fab9419).Error()returns the sentinel text, then the Rust message. ADiagnosticwith nokindreturns onlyMessage.diagnoseno longer has the special case for an unknown status.TestAnUnknownStatusIsStillInternalstill checks that the status number is in the text. - Low, test gap for
context.WithoutCancel: fixed (dd3b708). The new test usesWithCloseOnContextDone(true)and a cancelled context, and it checks for noErrTrap. - Nit, no constructor for a fake: fixed (bdd7d6d). The doc comment shows how to wrap a sentinel with a
Diagnostic. - Nit, stale comment on
authHTTPStatus.wrap: fixed (fab9419).
New findings: two nits only. Two doc lines in encrypt still say that Error() returns Message. The auth doc was updated, but these two were not. See the inline comments. I found no new problems in the Go code.
…aps to its sentinel A Go caller could check an error's kind with errors.Is and nothing more: the guest handed back a status number, and errors.As had no type to target. #1100 gave both guests se_last_error, which hands over the full error behind a failed export. This reads it. guest.Call fetches it after any non-zero status and returns a *Diagnostic: Code, Message, Help, URL, Severity, Fields and Causes, with Unwrap returning the sentinel the status already mapped to, so every errors.Is check keeps working and Error() is the Rust message. The buffer is wiped on both sides once read. A guest with no se_last_error, nothing recorded, or bytes that do not decode into an error with a message all give the bare sentinel, as before; a trap fetching it also wraps ErrTrap, so the caller closes the instance as after any trap. encrypt and auth expose the type as Diagnostic (and Cause) by alias, as they do the sentinels. ExpectedKeyset and FoundKeyset read a foreign-keyset refusal's two keysets, and Field and Reason a refused plan, record or value's field and reason. Tests: a hand-assembled guest drives each missing or broken detail and checks the wipe; each ZeroKMS outcome, each engine refusal reachable through the public API, and each profile-store kind asserts its code; a guest with se_last_error unbound fails with the bare sentinel; the foreign-keyset refusal carries both ids, hermetically and live. The README's Errors section explains errors.Is for the kind and errors.As for the detail, and the rule for what an error may contain. Closes #1101 Claude-Session: https://claude.ai/code/session_01URtfKsTToFUCRwq3g7gCUf
Error returned the Rust message alone, so a guest failure's text lost the "cipherstash: ..." prefix it had before: a log line no longer said where it came from, and a caller matching on the old text stopped matching. Error now returns the sentinel's text, then the Rust message. The status number of an unknown status is in that prefix, so diagnose no longer wraps one specially.
The real guests close their module when the context is done. The stub runtime now does too, and a test calls diagnose under a cancelled context: it fails with a trap if the fetch stops using context.WithoutCancel.
325ccd4 to
58c7802
Compare
4 participants
Summary
Go callers can now inspect detailed Rust runtime errors with
errors.As, while existingerrors.Ischecks continue to identify the same error kinds. Bothencryptandauthexpose a*Diagnosticcontaining a stable code, message, help, structured fields, and causes. The runtime reads these details through these_last_errorexport added in #1104 and falls back to the previous plain error when details are unavailable or invalid.This is the third of three PRs for #1098, completing the path from Rust error details to the Go API.
Changes
DiagnosticandCausetypes, exposed as aliases from bothencryptandauth.Error()returns the Rust message, andUnwrap()returns the existing sentinel error used byerrors.Is.se_last_errorafter a nonzero status, decodes the details, and wipes both the host copy and runtime buffer. The export is optional for compatibility with older runtimes.ErrTrap, so the client closes the runtime instance while preserving the original error kind.errors.Isfor the kind anderrors.Asfor details, documents allowed error contents, and removes stale comments saying only status numbers cross the boundary.Verification
The existing PR description reports the following checks. This description edit did not rerun them or check current CI status.
CGO_ENABLED=0 go test ./...fromlanguages/golangpassed against freshly built authentication and all four encryption runtimes.GOARCH=386tests passed forinternal/...andauth/...;go vet ./...andgofmt -lwere clean.golangci-lint run ./...with pinned version 2.14.0 reported 0 issues. A compatible binary was installed because the image's Go 1.25-built binary could not check this module.errors.Isassertions passed unchanged, and the README example compiled in a temporary test.8faaf44: Go lint, WebAssembly/runtime checks, macOS/Windows bindings, and Go live tests passed. The original description reported CI pending at rebased headd18bfbc; only two base dependency files changed. This is historical status, not a claim about current CI.TestLiveForeignKeysetIsRefusedBeforeRetrieval, which requires credentials. A deterministic test checks both keyset identifiers on every PR.Related
Review notes
internal/guest/diagnostic.go, then the integration ininternal/guest/call.go.: HTTP 403suffix because the Rust error does not always contain the status. Some messages therefore repeat it, such asServer error: 403: HTTP 403.Summary by CodeRabbit
New Features
Documentation