docs: reborn error recoverability audit + remediation plan - #5383
Conversation
Maps every reborn run error to recoverable / run-borking, analyzes PR #4841 coverage, and lays out the path to the two-bucket end state (SecurityStop | Retriable | Explainable). Headline finding: the host_runtime disposition layer intends no capability failure to abort, but the recovery strategy aborts on Dispatcher/InvalidOutput/Unknown — re-bucketing that class makes "model called a nonexistent tool" and malformed-output failures recoverable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new planning document, ChangesReborn Error Recoverability Audit Plan
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Review note: Documentation-only change (+240/-0, one file). No code, no exported/public entity changes, nothing to gate against sandbox/trust/secrets/egress/migration invariants — nothing for clippy/rustfmt/cargo-deny/check_no_panics to catch here either. Recommend confirming referenced code-location appendix (§ code locations) still matches current file/module paths before merging, since stale pointers in an audit doc silently rot. 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 |
github-actions
Bot
added
scope: docs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive audit and remediation plan for the Reborn Error Recoverability initiative, detailing error classification, identifying gaps, and outlining a target architecture to route failures into three distinct lanes (SecurityStop, Retriable, and Explainable). The review feedback suggests correcting a variable name in a documented code snippet and advises using length-prefixed encoding with domain-separated digests when designing deterministic identifiers for the RunFailureReason taxonomy.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| ### Building blocks (dependency order) | ||
|
|
||
| 1. **`RunFailureReason` taxonomy** — wire-stable, user-facing, distinct from internal `LoopFailureKind`; carries `{lane, retry_policy, user_message, correlation_id}`. (#4841's `FailureExplanationProvider` + `safe_summary` category is most of this.) |
There was a problem hiding this comment.
When designing the wire-stable RunFailureReason taxonomy or generating any deterministic identifiers/hashes from multiple string components (such as combining lane, retry_policy, user_message, and correlation_id), please ensure you use an injective length-prefixed encoding combined with a domain-separated collision-resistant digest rather than simple concatenation with a delimiter. This eliminates the risk of separator-collision attacks or accidental collisions.
References
- Derive deterministic identifiers from multiple components using length-prefixed components combined with a domain-separated collision-resistant digest, rather than raw string concatenation, to prevent accidental or malicious collisions.
| | Site | Condition | Current | Fix | | ||
| |---|---|---|---| | ||
| | `crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs:108,213` (via `outbound_delivery_host_error`, :575) | model picks bad/nonexistent `target_id` (InvalidRequest/NotFound), forbidden, conflict, rate-limited, transient-unavailable | maps **all** `RebornServicesErrorCode` → `Err` → terminal | mirror `project_service_outcome`: InvalidRequest/NotFound→`Failed{InvalidInput}`; Unauthenticated/Forbidden→`Denied`; Conflict→`Failed{OperationFailed}`; RateLimited→`Failed{Resource}`; Unavailable→`Failed{Unavailable}`; only Internal→`Err` | | ||
| | `outbound_delivery.rs:223` | model-supplied `target_id` interpolated into `safe_summary` | `format!("set delivery target to {target_id}")` — a delimiter in the id trips validation → terminal | fixed host-authored string; id travels in `output` | |
There was a problem hiding this comment.
There is a minor typo in the documented code snippet. In crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs:223, the actual variable interpolated is target_summary rather than target_id.
| | `outbound_delivery.rs:223` | model-supplied `target_id` interpolated into `safe_summary` | `format!("set delivery target to {target_id}")` — a delimiter in the id trips validation → terminal | fixed host-authored string; id travels in `output` | | |
| | outbound_delivery.rs:223 | model-supplied target_id interpolated into safe_summary | format!("set delivery target to {target_summary}") — a delimiter in the id trips validation → terminal | fixed host-authored string; id travels in output | |
|
🚅 Deployed to the ironclaw-pr-5383 environment in ironclaw-ci-preview
|
This branch was successfully deployed
1 participant
What
A findings + remediation-plan document (no code changes) mapping every reborn-binary run error to recoverable vs run-borking, analyzing what PR #4841 already covers, and laying out the path to the agreed two-bucket end state: a security-related error stops the run, otherwise everything is user-explainable or retriable.
Doc:
docs/plans/2026-06-28-reborn-error-recoverability-audit.md. Builds ondocs/plans/2026-06-12-reborn-no-borking-failures.md/ PR #4841.Highlights
capability_failure_disposition) intends that no capability failure ever aborts (onlyModelVisibleToolError/RetrySameCall), yet the recovery strategy aborts on thePermanentclass.Dispatcher/InvalidOutput/Unknownare dispositioned recoverable by host_runtime but mapped toPermanent → Abortbycapability_error_class. SinceUnknownCapability/UnknownProvider→InvalidOutput, "the model called a nonexistent tool" currently kills the run instead of becoming a model-visible tool error. Re-bucketing that class is the single highest-leverage fix.outbound_deliverymaps every service error to a terminalErrand interpolatestarget_idinto asafe_summary; the sandbox-plan path turns bad model input into a terminalInvalidInvocation; approval-lease expiry hard-borks.Status
Discovery is complete for the core spine and the synthetic/local_dev handler scope. Two sweeps remain (flagged in §6.4): host_runtime/dispatcher variant-by-variant, and the tool-backend/extension layer. Two product decisions are open (§8): side-effect retry for no-checkpoint runs, and whether pre-run/ingress failures join the same taxonomy.
Opening as draft for discussion — this is a plan, not an implementation.
🤖 Generated with Claude Code