009 human readable errors impl#304
Draft
haseebrabbani wants to merge 9 commits into
Draft
Conversation
) Implements the server side of feature 009-human-readable-errors. - GuardianError gains user_message() — a short, sanitized, end-user-safe sentence per variant, with the connectivity-vs-internal split, plus retryable(). The diagnostic Display string is now logged, never returned. - HTTP error body is reshaped to { code, message, meta } (meta carries retryable + retry_after_secs/missing_permissions/paused_*); legacy `success` and `error` fields are removed. - gRPC Status conversion generalized: Status.message is the user-safe message and Status.details carries the identical { code, message, meta } object for every variant (was AccountPaused-only). - rate-limit middleware and the configure endpoint reuse the canonical message/envelope; dead api/http.rs ErrorResponse removed. - OpenAPI ApiErrorResponse/ApiErrorMeta updated and docs/openapi*.json regenerated; tests reshaped + SC-001/SC-002/SC-005 coverage added.
Per @zeljkoX's PR #292 review, gRPC errors now surface as a tonic::Status (Status.code unchanged, Status.message = the user-safe message, Status.details = the identical { code, message, meta } object) instead of in-band success=false / error_code fields on the response messages. All 9 service handlers' error arms now return Err(Status::from(e)); the generalized From<GuardianError> impl attaches the details. gRPC integration tests updated to expect Status errors with parsed details. Follow-ups (noted, out of this change): the now-vestigial success/error_code fields remain on the proto success responses (clean-up alongside HTTP ConfigureResponse), and the pre-service Status::invalid_argument auth-metadata guards still return developer-facing messages.
…179) - ClientError gains guardian_code(), user_message(), guardian_meta(), and is_not_found() — parsed from the gRPC Status.details { code, message, meta } object (feature 009), with user_message() falling back to Status.message. - miden-multisig-client get_deltas() now detects "no new deltas" via is_not_found() (gRPC NotFound) instead of substring-matching the message, which broke when gRPC errors moved from in-band to Status.
GuardianHttpError now parses the reshaped error body (feature 009) and exposes code, userMessage (safe to display), and meta (snake_case mapped to camelCase). The replay-retry branches on the stable `authentication_failed` code instead of the now-sanitized "Replay attack" body text. Contract tests updated to the new shape + accessor coverage.
…179) parseErrorBody and parseErrorResponse now read the user-safe `message` (was `error`) and the structured `meta` block (retry_after_secs / missing_permissions / paused_* / retryable), and no longer require/expect `success`. GuardianOperatorHttpErrorData drops `success`, renames `error` → `message`; the wire type (GuardianErrorResponse) is reshaped to { code, message, meta }. Tests updated to the new shape; meta.retryable is now surfaced for every code.
…#179) User Story 3: when Guardian is reached, errors carry the server's stable code + user-safe message (via GuardianHttpError); when it is not reached (connection refused, DNS, timeout, or a proxy 5xx with no Guardian body), toUserFacingError() classifies the failure and returns a friendly connectivity message instead of raw "Failed to fetch" text. Mirrors the 0xMiden/wallet connectivity-classify.ts heuristic. Re-exports GuardianHttpError/GuardianErrorMeta for wallet consumers.
…r) (#179) Follows the operator-client error reshape (feature 009): the user-safe message is now `message`, not `error`. code/pausedAt/pausedReason remain.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/guardian-operator-client/src/types.ts (1)
20-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign
retryabledocs with the flattened meta contract.
parseErrorResponsenow surfacesmeta.retryableregardless of error code, but this public field doc still says it is absent for every non-permission code. That can lead callers to ignore retryability on rate-limit/data-unavailable errors.Suggested doc update
- * Feature 006-operator-authz FR-016: explicit retryability flag. - * `false` for permission denials (the contract pins this); absent - * for every other code so existing parsers see no change. + * Explicit retryability flag flattened from `meta.retryable` when supplied + * by the feature 009 error envelope. Permission denials and account-paused + * rejections pin this to `false`; retryable server failures surface `true`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/guardian-operator-client/src/types.ts` around lines 20 - 38, Update the `retryable` field documentation in `types.ts` so it matches the flattened meta contract exposed by `parseErrorResponse`: it should describe `retryable` as a surfaced `meta.retryable` value for any error code, not something absent for all non-permission errors. Keep the references to `parseErrorResponse` and the `retryable` property in `OperatorError` aligned, and adjust the comment to mention that callers may see retryability on rate-limit or data-unavailable responses as well as permission denials.
🧹 Nitpick comments (2)
packages/miden-multisig-client/src/connectivity.ts (1)
67-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline implementation comments violate the multisig SDK rule.
Lines 72-73 are inline
// ...comments inside thetoUserFacingErrorimplementation body. The rationale is valuable, so move it into the function's doc comment (which already exists above) rather than inlining it.As per coding guidelines: "Do not add inline comments (
// ...) in implementation code in multisig SDKs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/miden-multisig-client/src/connectivity.ts` around lines 67 - 83, The inline implementation comments inside toUserFacingError violate the multisig SDK no-inline-comments rule; move that rationale into the existing function doc comment above toUserFacingError and remove the // comments from the function body. Keep the behavior in toUserFacingError unchanged, but preserve the explanation there via documentation rather than inline comments.Source: Coding guidelines
crates/server/src/error.rs (1)
343-356: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReconsider marking non-transient internal faults as
retryable.
ConfigurationErrorandSigningErrorare typically deterministic/persistent rather than transient. Surfacingmeta.retryable = truefor them invites clients to retry failures that cannot succeed on retry, which can amplify load during an outage (retry storms). If the intent is purely to match the "please try again" copy, that's defensible — but consider scopingretryableto genuinely transient faults (storage/network/RPC/rate-limit/data-unavailable) and keeping configuration/signing faults non-retryable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/server/src/error.rs` around lines 343 - 356, Update GuardianError::retryable in error.rs to stop classifying deterministic internal faults as retryable; remove ConfigurationError and SigningError from the matches! list (or otherwise narrow retryable to transient cases only) so meta.retryable is reserved for genuinely temporary failures like storage, network, RPC, rate limits, and data availability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/miden-multisig-client/src/client/account.rs`:
- Around line 398-400: The `get_delta_since` error handling in `AccountClient`
is treating all transport-level NotFound errors the same, which can mask a
missing-account case from `resolve_account`. Update the match branch around
`Err(e) if e.is_not_found()` to distinguish by the stable Guardian error code
(`delta_not_found` versus `account_not_found`) instead of `is_not_found()`, so
only the no-newer-deltas case returns `Ok(())` and the account-missing path is
handled separately.
In `@crates/server/src/testing/integration/proposals_grpc.rs`:
- Around line 305-308: The `proposal_not_found` gRPC error check in
`proposals_grpc.rs` only verifies `code` and `message`, so add an assertion on
the parsed `status.details()` JSON to require `meta.retryable` as well. Update
the existing test around `serde_json::from_slice`, `details["code"]`, and
`details["message"]` so it also checks that `details["meta"]["retryable"]` is
present and has the expected boolean value.
- Around line 367-370: The integration assertion in the proposals gRPC test is
too loose because it only checks that details["code"] is a string; update the
test around the status.details JSON parsing to pin the expected stable
machine-readable error code explicitly. Use the existing status/details
assertions in proposals_grpc.rs and replace the generic type check with a strict
equality check against the known code so regressions in the client-facing
contract are caught.
In `@packages/guardian-operator-client/src/http.test.ts`:
- Line 164: The error fixtures in http.test.ts are missing the required
meta.retryable field for the new wire contract, so update each affected error
object to include a meta payload with retryable set appropriately. Use the
existing fixture groups around the referenced error expectations in the test
file to add meta.retryable wherever meta is absent or incomplete, keeping the
asserted { code, message, meta } shape consistent across the named cases.
In `@packages/guardian-operator-client/src/http.ts`:
- Around line 281-284: The HTTP error message construction in the error class
within http.ts is appending the raw response body when parsing fails, which
should not be exposed in Error.message. Update the constructor that builds the
Guardian operator HTTP error so it only includes the safe parsed display string
from data.message in the default message, while preserving the raw body
separately on the error object for diagnostics. Adjust the message formatting
logic around the status/statusText composition to remove the body fallback
entirely.
- Around line 1072-1109: The strict error parser in the response handling path
is still allowing `meta.retryable` to be absent, which can hide wire contract
drift and leave retry classification undefined. Update the parsing logic around
`record.meta` so `meta` and `meta.retryable` are required in this strict path,
and throw a `GuardianOperatorContractError` when either is missing or malformed.
Use the existing `meta`, `retryableRaw`, and `retryable` handling in the error
parser to keep the validation aligned with the rest of the contract checks.
---
Outside diff comments:
In `@packages/guardian-operator-client/src/types.ts`:
- Around line 20-38: Update the `retryable` field documentation in `types.ts` so
it matches the flattened meta contract exposed by `parseErrorResponse`: it
should describe `retryable` as a surfaced `meta.retryable` value for any error
code, not something absent for all non-permission errors. Keep the references to
`parseErrorResponse` and the `retryable` property in `OperatorError` aligned,
and adjust the comment to mention that callers may see retryability on
rate-limit or data-unavailable responses as well as permission denials.
---
Nitpick comments:
In `@crates/server/src/error.rs`:
- Around line 343-356: Update GuardianError::retryable in error.rs to stop
classifying deterministic internal faults as retryable; remove
ConfigurationError and SigningError from the matches! list (or otherwise narrow
retryable to transient cases only) so meta.retryable is reserved for genuinely
temporary failures like storage, network, RPC, rate limits, and data
availability.
In `@packages/miden-multisig-client/src/connectivity.ts`:
- Around line 67-83: The inline implementation comments inside toUserFacingError
violate the multisig SDK no-inline-comments rule; move that rationale into the
existing function doc comment above toUserFacingError and remove the // comments
from the function body. Keep the behavior in toUserFacingError unchanged, but
preserve the explanation there via documentation rather than inline comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 815079a2-e416-471c-b0a3-8404a0a2c7a9
📒 Files selected for processing (28)
crates/client/src/error.rscrates/miden-multisig-client/src/client/account.rscrates/server/src/api/dashboard.rscrates/server/src/api/grpc.rscrates/server/src/api/http.rscrates/server/src/error.rscrates/server/src/middleware/rate_limit.rscrates/server/src/openapi.rscrates/server/src/testing/integration/auth_grpc.rscrates/server/src/testing/integration/error_envelope_http.rscrates/server/src/testing/integration/proposals_grpc.rsdocs/openapi-client.jsondocs/openapi-dashboard.jsondocs/openapi-evm.jsondocs/openapi.jsonexamples/operator-smoke-web/src/App.tsxpackages/guardian-client/src/http.test.tspackages/guardian-client/src/http.tspackages/guardian-client/src/index.tspackages/guardian-operator-client/src/http.test.tspackages/guardian-operator-client/src/http.tspackages/guardian-operator-client/src/server-types.tspackages/guardian-operator-client/src/types.tspackages/miden-multisig-client/src/connectivity.test.tspackages/miden-multisig-client/src/connectivity.tspackages/miden-multisig-client/src/index.tsspeckit/features/009-human-readable-errors/plan.mdspeckit/features/009-human-readable-errors/tasks.md
Comment on lines
+398
to
400
| Err(e) if e.is_not_found() => { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'fn get_delta_since' -A30 crates
rg -nP 'DeltaNotFound|AccountNotFound|StateNotFound' -A3 crates/server/src/error.rs
rg -nP 'fn code\b' -A40 crates/server/src/error.rs | rg -n 'not_found|delta|account'Repository: OpenZeppelin/guardian
Length of output: 17079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the client-side delta handling and the server-side error path.
sed -n '360,430p' crates/miden-multisig-client/src/client/account.rs
echo '---'
sed -n '1,220p' crates/server/src/services/get_delta_since.rs
echo '---'
rg -n 'resolve_account|AccountNotFound|DeltaNotFound|guardian_code|is_not_found' crates/server/src -A4 -B4Repository: OpenZeppelin/guardian
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the server's gRPC error conversion and any helper the client can branch on.
sed -n '140,230p' crates/server/src/error.rs
echo '---'
rg -n 'guardian_code|error_code|tonic::Status|metadata' crates/miden-multisig-client crates/client crates/server/src -A3 -B3Repository: OpenZeppelin/guardian
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the server's gRPC error conversion and any helper the client can branch on.
sed -n '140,230p' crates/server/src/error.rs
echo '---'
rg -n 'guardian_code|error_code|tonic::Status|metadata' crates/miden-multisig-client crates/client crates/server/src -A3 -B3Repository: OpenZeppelin/guardian
Length of output: 50378
Differentiate delta_not_found from other NotFound errors.
get_delta_since can return both DeltaNotFound (no newer canonical deltas) and AccountNotFound from resolve_account; both become gRPC NotFound, so this branch also hides a missing-account error. Switch on the stable Guardian error code (delta_not_found vs account_not_found) instead of the transport-level not-found signal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/miden-multisig-client/src/client/account.rs` around lines 398 - 400,
The `get_delta_since` error handling in `AccountClient` is treating all
transport-level NotFound errors the same, which can mask a missing-account case
from `resolve_account`. Update the match branch around `Err(e) if
e.is_not_found()` to distinguish by the stable Guardian error code
(`delta_not_found` versus `account_not_found`) instead of `is_not_found()`, so
only the no-newer-deltas case returns `Ok(())` and the account-missing path is
handled separately.
Comment on lines
+305
to
+308
| let details: serde_json::Value = | ||
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | ||
| assert_eq!(details["code"], "proposal_not_found"); | ||
| assert!(details["message"].is_string()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required meta.retryable field here.
The new envelope requires meta.retryable; this test would still pass if Status.details() regressed to only { code, message }.
Suggested assertion
assert_eq!(details["code"], "proposal_not_found");
assert!(details["message"].is_string());
+ assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let details: serde_json::Value = | |
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | |
| assert_eq!(details["code"], "proposal_not_found"); | |
| assert!(details["message"].is_string()); | |
| let details: serde_json::Value = | |
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | |
| assert_eq!(details["code"], "proposal_not_found"); | |
| assert!(details["message"].is_string()); | |
| assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/server/src/testing/integration/proposals_grpc.rs` around lines 305 -
308, The `proposal_not_found` gRPC error check in `proposals_grpc.rs` only
verifies `code` and `message`, so add an assertion on the parsed
`status.details()` JSON to require `meta.retryable` as well. Update the existing
test around `serde_json::from_slice`, `details["code"]`, and
`details["message"]` so it also checks that `details["meta"]["retryable"]` is
present and has the expected boolean value.
Comment on lines
+367
to
+370
| let details: serde_json::Value = | ||
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | ||
| assert!(details["code"].is_string()); | ||
| assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin the stable error code instead of only checking its type.
Line 369 allows any string code, so it won’t catch regressions in the machine-readable contract clients branch on.
Suggested tightening
- assert!(details["code"].is_string());
+ assert_eq!(details["code"], "authentication_failed");
assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let details: serde_json::Value = | |
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | |
| assert!(details["code"].is_string()); | |
| assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); | |
| let details: serde_json::Value = | |
| serde_json::from_slice(status.details()).expect("Status.details is JSON"); | |
| assert_eq!(details["code"], "authentication_failed"); | |
| assert_eq!(details["meta"]["retryable"], serde_json::Value::Bool(false)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/server/src/testing/integration/proposals_grpc.rs` around lines 367 -
370, The integration assertion in the proposals gRPC test is too loose because
it only checks that details["code"] is a string; update the test around the
status.details JSON parsing to pin the expected stable machine-readable error
code explicitly. Use the existing status/details assertions in proposals_grpc.rs
and replace the generic type check with a strict equality check against the
known code so regressions in the client-facing contract are caught.
| success: false, | ||
| code: 'invalid_limit', | ||
| error: 'limit must be at most 500, got 9999', | ||
| message: 'limit must be at most 500, got 9999', |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include required meta.retryable in error fixtures.
The new wire type requires meta.retryable, but these fixtures either omit meta or include meta without retryable. Add it so tests lock the actual { code, message, meta } contract and catch server/client drift.
Example fixture shape
- body: { code, message: `${code} message` },
+ body: { code, message: `${code} message`, meta: { retryable: false } },
- meta: { retry_after_secs: 60 },
+ meta: { retry_after_secs: 60, retryable: true },Also applies to: 483-484, 670-670, 694-694, 717-717, 1191-1191, 1207-1207, 1220-1221, 1255-1255, 1347-1347, 1471-1471, 1541-1541, 1689-1690, 1872-1873, 1896-1896
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/guardian-operator-client/src/http.test.ts` at line 164, The error
fixtures in http.test.ts are missing the required meta.retryable field for the
new wire contract, so update each affected error object to include a meta
payload with retryable set appropriately. Use the existing fixture groups around
the referenced error expectations in the test file to add meta.retryable
wherever meta is absent or incomplete, keeping the asserted { code, message,
meta } shape consistent across the named cases.
Comment on lines
281
to
284
| super( | ||
| `Guardian operator HTTP error ${status}: ${statusText}${ | ||
| data ? ` - ${data.error}` : body ? ` - ${body}` : '' | ||
| data ? ` - ${data.message}` : body ? ` - ${body}` : '' | ||
| }`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not append unparsed response bodies to Error.message.
data.message is the safe display string, but the body fallback can still expose raw backend/transport text when parsing fails. Keep the raw body on the field for diagnostics, but do not include it in the default message.
Suggested fix
super(
`Guardian operator HTTP error ${status}: ${statusText}${
- data ? ` - ${data.message}` : body ? ` - ${body}` : ''
+ data ? ` - ${data.message}` : ''
}`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| super( | |
| `Guardian operator HTTP error ${status}: ${statusText}${ | |
| data ? ` - ${data.error}` : body ? ` - ${body}` : '' | |
| data ? ` - ${data.message}` : body ? ` - ${body}` : '' | |
| }`, | |
| super( | |
| `Guardian operator HTTP error ${status}: ${statusText}${ | |
| data ? ` - ${data.message}` : '' | |
| }`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/guardian-operator-client/src/http.ts` around lines 281 - 284, The
HTTP error message construction in the error class within http.ts is appending
the raw response body when parsing fails, which should not be exposed in
Error.message. Update the constructor that builds the Guardian operator HTTP
error so it only includes the safe parsed display string from data.message in
the default message, while preserving the raw body separately on the error
object for diagnostics. Adjust the message formatting logic around the
status/statusText composition to remove the body fallback entirely.
Comment on lines
+1072
to
1109
| const metaRaw = record.meta; | ||
| let meta: Record<string, unknown> = {}; | ||
| if (metaRaw !== undefined) { | ||
| if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) { | ||
| throw new GuardianOperatorContractError( | ||
| 'error response', | ||
| 'meta must be an object when present', | ||
| ); | ||
| } | ||
| meta = metaRaw as Record<string, unknown>; | ||
| } | ||
|
|
||
| const retryAfterValue = meta.retry_after_secs; | ||
| let retryAfterSecs: number | undefined; | ||
| if (retryAfterValue !== undefined) { | ||
| if (typeof retryAfterValue !== 'number' || !Number.isInteger(retryAfterValue)) { | ||
| throw new GuardianOperatorContractError( | ||
| 'error response', | ||
| 'retry_after_secs must be an integer when present', | ||
| 'meta.retry_after_secs must be an integer when present', | ||
| ); | ||
| } | ||
| retryAfterSecs = retryAfterValue; | ||
| } | ||
|
|
||
| // Optional stable machine-readable error code added by feature | ||
| // `005-operator-dashboard-metrics` and required for the dashboard | ||
| // error taxonomy (FR-028). Older servers may omit it; tolerate that | ||
| // by leaving the field undefined. | ||
| const codeValue = record.code; | ||
| let code: string | undefined; | ||
| if (codeValue !== undefined) { | ||
| if (typeof codeValue !== 'string') { | ||
| // `meta.retryable` is always present on the new wire shape; surface it | ||
| // whenever it is a boolean (the server pins `false` for permission denials | ||
| // and account-paused rejections). | ||
| let retryable: boolean | undefined; | ||
| const retryableRaw = meta.retryable; | ||
| if (retryableRaw !== undefined) { | ||
| if (typeof retryableRaw !== 'boolean') { | ||
| throw new GuardianOperatorContractError( | ||
| 'error response', | ||
| 'code must be a string when present', | ||
| 'meta.retryable must be a boolean when present', | ||
| ); | ||
| } | ||
| code = mapDashboardErrorCode(codeValue) ?? undefined; | ||
| retryable = retryableRaw; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require meta.retryable in the strict error parser.
The reviewed wire type makes meta.retryable required, but this parser accepts missing meta and leaves retryable undefined. That silently accepts server contract drift and loses retry classification.
Suggested fix
const metaRaw = record.meta;
- let meta: Record<string, unknown> = {};
- if (metaRaw !== undefined) {
- if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) {
- throw new GuardianOperatorContractError(
- 'error response',
- 'meta must be an object when present',
- );
- }
- meta = metaRaw as Record<string, unknown>;
+ if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) {
+ throw new GuardianOperatorContractError(
+ 'error response',
+ 'meta must be an object',
+ );
}
+ const meta = metaRaw as Record<string, unknown>;
@@
- let retryable: boolean | undefined;
const retryableRaw = meta.retryable;
- if (retryableRaw !== undefined) {
- if (typeof retryableRaw !== 'boolean') {
- throw new GuardianOperatorContractError(
- 'error response',
- 'meta.retryable must be a boolean when present',
- );
- }
- retryable = retryableRaw;
+ if (typeof retryableRaw !== 'boolean') {
+ throw new GuardianOperatorContractError(
+ 'error response',
+ 'meta.retryable must be a boolean',
+ );
}
+ const retryable = retryableRaw;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const metaRaw = record.meta; | |
| let meta: Record<string, unknown> = {}; | |
| if (metaRaw !== undefined) { | |
| if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'meta must be an object when present', | |
| ); | |
| } | |
| meta = metaRaw as Record<string, unknown>; | |
| } | |
| const retryAfterValue = meta.retry_after_secs; | |
| let retryAfterSecs: number | undefined; | |
| if (retryAfterValue !== undefined) { | |
| if (typeof retryAfterValue !== 'number' || !Number.isInteger(retryAfterValue)) { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'retry_after_secs must be an integer when present', | |
| 'meta.retry_after_secs must be an integer when present', | |
| ); | |
| } | |
| retryAfterSecs = retryAfterValue; | |
| } | |
| // Optional stable machine-readable error code added by feature | |
| // `005-operator-dashboard-metrics` and required for the dashboard | |
| // error taxonomy (FR-028). Older servers may omit it; tolerate that | |
| // by leaving the field undefined. | |
| const codeValue = record.code; | |
| let code: string | undefined; | |
| if (codeValue !== undefined) { | |
| if (typeof codeValue !== 'string') { | |
| // `meta.retryable` is always present on the new wire shape; surface it | |
| // whenever it is a boolean (the server pins `false` for permission denials | |
| // and account-paused rejections). | |
| let retryable: boolean | undefined; | |
| const retryableRaw = meta.retryable; | |
| if (retryableRaw !== undefined) { | |
| if (typeof retryableRaw !== 'boolean') { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'code must be a string when present', | |
| 'meta.retryable must be a boolean when present', | |
| ); | |
| } | |
| code = mapDashboardErrorCode(codeValue) ?? undefined; | |
| retryable = retryableRaw; | |
| } | |
| const metaRaw = record.meta; | |
| if (typeof metaRaw !== 'object' || metaRaw === null || Array.isArray(metaRaw)) { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'meta must be an object', | |
| ); | |
| } | |
| const meta = metaRaw as Record<string, unknown>; | |
| const retryAfterValue = meta.retry_after_secs; | |
| let retryAfterSecs: number | undefined; | |
| if (retryAfterValue !== undefined) { | |
| if (typeof retryAfterValue !== 'number' || !Number.isInteger(retryAfterValue)) { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'meta.retry_after_secs must be an integer when present', | |
| ); | |
| } | |
| retryAfterSecs = retryAfterValue; | |
| } | |
| // `meta.retryable` is always present on the new wire shape; surface it | |
| // whenever it is a boolean (the server pins `false` for permission denials | |
| // and account-paused rejections). | |
| const retryableRaw = meta.retryable; | |
| if (typeof retryableRaw !== 'boolean') { | |
| throw new GuardianOperatorContractError( | |
| 'error response', | |
| 'meta.retryable must be a boolean', | |
| ); | |
| } | |
| const retryable = retryableRaw; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/guardian-operator-client/src/http.ts` around lines 1072 - 1109, The
strict error parser in the response handling path is still allowing
`meta.retryable` to be absent, which can hide wire contract drift and leave
retry classification undefined. Update the parsing logic around `record.meta` so
`meta` and `meta.retryable` are required in this strict path, and throw a
`GuardianOperatorContractError` when either is missing or malformed. Use the
existing `meta`, `retryableRaw`, and `retryable` handling in the error parser to
keep the validation aligned with the rest of the contract checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issue #179 — wallet users now see short, sanitized, human-readable error messages instead of raw backend/transport errors. Implements the error-contract reshape agreed in the spec review (#292): a single
{ code, message, meta }object, identical on the HTTP body and the gRPCStatus.details, with the diagnosticDisplaystring logged server-side only.Spec:
speckit/features/009-human-readable-errors/(spec.md,plan.md,tasks.md).Wire contract
{ "code": "authorization_failed", "message": "You're not an authorized signer for this account.", "meta": { "retryable": false } }code— stable machine code (existing vocabulary, unchanged). Branch + i18n key.message— short, user-safe sentence; always present; safe to display. Wording not stable.meta—retryable(always) +retry_after_secs/missing_permissions/paused_at/paused_reasonwhen they apply.success, noerror). gRPC:Status.codeunchanged,Status.message=message,Status.details= this object, for every error.What changed (bottom-up)
crates/server):GuardianError::user_message()(sanitized per-variant, connectivity-vs-internal split) +retryable(); reshaped HTTP envelope;Displayis logged, not returned; gRPC errors now ride ontonic::Status(all 9 handlers); rate-limit +configureuse the user-safe message; OpenAPIApiErrorResponse/ApiErrorMetaregenerated.ClientError::{guardian_code, user_message, guardian_meta, is_not_found}parsed fromStatus.details; multisigget_deltasbranches onis_not_found()(not message text).guardian-client:GuardianHttpErrorparses{ code, message, meta }; replay-retry keys on theauthentication_failedcode.guardian-operator-client: parsers + types reshaped to{ code, message, meta }.miden-multisig-client:toUserFacingError()/isLikelyNetworkError()connectivity classifier for codeless transport failures (User Story 3 — the 0xMiden/walletconnectivity-classify.tspattern).operator-smoke-webreadsdata.message.Testing
cargo check --workspace --all-targets,cargo clippy,cargo fmtall clean. Tests: error (45),error_envelope_http(8), dashboard (73),*_grpcintegration, guardian-client (41), miden-multisig-client (110).tsc --noEmitclean on all 3 packages): guardian-client vitest (49), operator-client vitest (83), miden-multisig-client connectivity (6).Security note
Today's raw
errorstrings leak account IDs, commitments, and raw RPC text to clients;messageis sanitized by construction and the diagnostic detail is now logged-only — closing that disclosure gap.miden-multisig-clientand the examples consume the published@openzeppelin/guardian-client. The newGuardianHttpErrorfields only exist once guardian-client is published (or linked in CI). This PR was verified locally by building guardian-client and linking it.Deferred follow-ups (tracked in
tasks.mdT014–T016)success/error_codefrom gRPC success responses + migrate the HTTPconfigureerror path onto the envelope (completes a strict "drop success everywhere").Status::invalid_argumentauth-metadata guards throughGuardianError.Closes #179.
Summary by CodeRabbit
New Features
code,message, andmetaformat across the app and API clients.Bug Fixes