Add durable recovery telemetry and close WS7 retry gaps - #6844
Conversation
📝 WalkthroughWalkthroughThis PR adds durable ChangesDurable recovery pipeline
Related regression and cleanup changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ModelStage
participant CheckpointStage
participant HostProgressPort
participant DurableMilestoneSink
participant ReplayProjection
ModelStage->>CheckpointStage: emit recovery metadata
CheckpointStage->>HostProgressPort: append FailureRecovered
HostProgressPort->>DurableMilestoneSink: publish milestone
DurableMilestoneSink->>ReplayProjection: replay durable event
ReplayProjection-->>ModelStage: projected recovery timeline entry
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
size: XL
🔎 Review · PR #6844
3 actionable findings →The recovery telemetry path has durability/exactly-once gaps, the revised JWT detector misses valid tokens, and removal of the public retry classifiers leaves an architecture snapshot test failing. Automatic · PR opened · attempt 1 of 3 · completed in 2m 58s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Review complete · PR #6844
The recovery telemetry path has durability/exactly-once gaps, the revised JWT detector misses valid tokens, and removal of the public retry classifiers leaves an architecture snapshot test failing.
Findings
- 🔴 High · Recovery events use a best-effort, non-idempotent progress path —
crates/ironclaw_agent_loop/src/executor/model.rs:266-275
Details are attached to the relevant diff. - 🔴 High · Valid JWTs with whitespace in the protected header bypass scanning —
crates/ironclaw_safety/src/leak_detector.rs:558-564
Details are attached to the relevant diff. - 🟠 Medium · Update the enforced composition public-surface snapshot —
crates/ironclaw_reborn_composition/src/lib.rs:120
Details are attached to the relevant diff.
Validation and technical details
- Inspected the complete trusted comparison refs/ironloop/base...refs/ironloop/head across all 29 changed files and traced executor progress through runner milestones, durable events, projections, hooks, product summaries, and tests.
- The repository codebase graph reported MISSING; review therefore used crate-local guidance, targeted ripgrep searches, and live source verification.
- Confirmed programmatically that the current composition pub-use extraction does not match docs/plans/composition-pubuse.snapshot; the stale snapshot still contains the removed failure_lane export.
- Constructed a valid JSON JWT header containing whitespace, base64url-encoded it to an eyA-prefixed compact token, and confirmed the new regex does not match it.
cargois unavailable in the review environment, so Rust test execution could not be repeated;git diff --checkcompleted successfully.- Base:
main - Head:
codex/ws7-error-recoverabilityate4c12d0 - Run:
0748c67d-a351-492e-8e16-8f890a87ac71
|
🚅 Deployed to the ironclaw-pr-6844 environment in ironclaw-ci-preview
|
e4c12d0 to
f501f69
Compare
github-actions
Bot
added
scope: dependencies
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 Review · PR #6844
GitHub request failed IronLoop could not complete a required GitHub request. Automatic · PR opened · attempt 1 of 3 · failed after 2m 21s Failure details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ironclaw_event_projections/src/lib.rs`:
- Around line 1713-1715: Before constructing the projection’s TimelineEntry,
re-sanitize event.recovery_stage, event.recovery_class, and
event.recovery_disposition with the sanitizer owned by the events component
rather than copying them directly. Add a regression covering a custom durable
backend that supplies unsafe sentinel recovery labels, and verify the published
projection contains only sanitized values.
In `@crates/ironclaw_safety/src/leak_detector.rs`:
- Around line 611-618: Update the bare_jwt regex in the LeakPattern definition
to remove the trailing word boundary or otherwise consume the full base64url
signature, including a final “-”, so matching and redaction never truncate the
token. Add a regression test in the leak detector test suite covering a JWT
whose signature ends with “-” and assert the entire token is redacted.
🪄 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 Plus
Run ID: 4632915e-baec-408a-bdae-e651d0bcdcda
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.tomlcrates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/checkpoint.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/model.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_event_projections/src/lib.rscrates/ironclaw_event_projections/src/runtime_projection.rscrates/ironclaw_event_projections/tests/replay_projection_contract.rscrates/ironclaw_event_streams/tests/event_stream_manager_contract/support/builders.rscrates/ironclaw_events/src/runtime_event.rscrates/ironclaw_events/tests/durable_log_contract.rscrates/ironclaw_hooks/src/dispatch/lifecycle_owner.rscrates/ironclaw_loop_host/tests/compaction_task_contract.rscrates/ironclaw_product/src/projection/tests/failure_explanation.rscrates/ironclaw_runner/src/failure_classification.rscrates/ironclaw_runner/src/failure_summary.rscrates/ironclaw_runner/src/lib.rscrates/ironclaw_runner/src/loop_driver_host/port_adapters.rscrates/ironclaw_runner/src/loop_driver_host/tests.rscrates/ironclaw_runner/src/milestone_events.rscrates/ironclaw_runner/src/planned_driver.rscrates/ironclaw_runner/tests/loop_milestone_event_projection.rscrates/ironclaw_safety/src/leak_detector.rscrates/ironclaw_turns/src/run_profile/host/mod.rscrates/ironclaw_turns/src/run_profile/host/progress.rscrates/ironclaw_turns/src/run_profile/milestones.rscrates/ironclaw_turns/src/run_profile/mod.rsdocs/reborn/contracts/events.mdtests/reborn_failure_retry_resume_e2e.rstests/support/reborn_parity_qa/binary_e2e.rs
💤 Files with no reviewable changes (2)
- crates/ironclaw_runner/src/failure_classification.rs
- crates/ironclaw_runner/src/lib.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.74% — 314336 / 366607 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
CI follow-up resolved in Local verification:
Final PR checks on head |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_events/src/runtime_event.rs (1)
123-128: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftUse recovery-specific domain types instead of
String.
failure_recoveredaccepts arbitrary strings and stores them in publicOption<String>fields; bounded sanitization cannot enforce valid stage/class/disposition semantics at callers. Define enums/newtypes inironclaw_events, with wire compatibility mapping unknown legacy values tounclassified.As per coding guidelines, “Use strong types and enums for known domain shapes; keep raw strings at external boundaries.”
Also applies to: 710-723
🤖 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/ironclaw_events/src/runtime_event.rs` around lines 123 - 128, Replace the public String-based recovery fields and the failure_recovered inputs with recovery-specific enums or newtypes in ironclaw_events for stage, class, and disposition. Update failure_recovered and related serialization/deserialization to use these strong types, while mapping unknown legacy wire values to the unclassified variant. Preserve wire compatibility and the existing retried/model_visible semantics.Source: Coding guidelines
crates/ironclaw_safety/src/leak_detector.rs (1)
611-618: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound bare-JTT candidates before decoding.
bare_jwtuses unbounded quantifiers, and each match is decoded and parsed byhas_json_web_token_header. A hostile input can force excessive allocation/CPU in the safety boundary (crates/**/*.rs: safety path requires explicit bounds on user-controlled strings and fail-closed). Returnfalsefor oversized candidates so the match is still treated as blocked/redacted rather than skipped.🤖 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/ironclaw_safety/src/leak_detector.rs` around lines 611 - 618, Bound the three base64url segments in the bare_jwt LeakPattern regex to explicit maximum lengths, and update has_json_web_token_header to return false for candidates exceeding those bounds before decoding or parsing. Preserve oversized matches as blocked/redacted rather than skipping them.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_events/src/runtime_event.rs`:
- Around line 123-128: Replace the public String-based recovery fields and the
failure_recovered inputs with recovery-specific enums or newtypes in
ironclaw_events for stage, class, and disposition. Update failure_recovered and
related serialization/deserialization to use these strong types, while mapping
unknown legacy wire values to the unclassified variant. Preserve wire
compatibility and the existing retried/model_visible semantics.
In `@crates/ironclaw_safety/src/leak_detector.rs`:
- Around line 611-618: Bound the three base64url segments in the bare_jwt
LeakPattern regex to explicit maximum lengths, and update
has_json_web_token_header to return false for candidates exceeding those bounds
before decoding or parsing. Preserve oversized matches as blocked/redacted
rather than skipping them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fb5a6a9-c9dc-48f9-acc0-8566b14e28ad
📒 Files selected for processing (5)
crates/ironclaw_event_projections/src/lib.rscrates/ironclaw_event_projections/tests/replay_projection_contract.rscrates/ironclaw_events/src/lib.rscrates/ironclaw_events/src/runtime_event.rscrates/ironclaw_safety/src/leak_detector.rs
|
Follow-up on CodeRabbit outside-diff review 4808372998: the oversized JWT allocation finding was valid and is fixed in commit 6819ff9. Validation now rejects allocation/JSON parsing above 64 KiB while treating the complete three-segment match as sensitive, so it fails closed and redacts the full token. The regex intentionally remains unbounded so it consumes the complete base64url run instead of recreating a tail leak. The recovery-domain-type suggestion does not fit the live ownership contract and received no code change: LoopRecoveryStage, LoopRecoveryClass, and LoopRecoveryDisposition are already strongly typed from the canonical loop through LoopHostMilestone in ironclaw_turns; the runner converts them to closed-vocabulary strings only at the lower, row-shaped RuntimeEvent substrate boundary documented in docs/reborn/contracts/events.md. Making ironclaw_events depend upward on ironclaw_turns or defining mirror enums would violate the dependency and shared-vocabulary rules. Verification: 279 ironclaw_safety tests passed; focused zero-warning clippy, fmt, and diff checks passed. |
* Add durable recovery telemetry and remove dead retry policy * test(reborn): cover durable recovery events end to end * test(events): initialize recovery projection fields * fix(recovery): make telemetry replay-stable * fix(recovery): harden projected telemetry redaction * fix(safety): bound JWT header validation
This branch was successfully deployed
1 participant
Summary
RecoveryOutcomeapplication site.event_id.failure_laneandretry_dispositionpolicies, remove only the zero-production-caller parallel classification module, and exercise automatic retry behavior through the runner.Change Type
The dependency change is test-only: the root integration harness consumes the existing event-projection crate.
Linked Issue
Related #6284. Follow-up to #5965.
Validation
cargo fmt --all -- --check-D warningsfor every touched cratecargo buildcargo test --features integration9dfa55801,f2a547742, and6819ff961.Test Strategy
User behavior:
Recovered model and capability failures contribute a durable recovery-rate numerator without terminating the run or duplicating a capability side effect. A recovery cannot proceed when its durable event append fails. Dotted package names such as
com.fasterxml.jacksonno longer poison compaction, while valid JWTs whose header JSON starts with whitespace remain redacted.Risk areas:
Tests added or updated:
HostManagedLoopProgressPort→ typed milestone; milestone → stable durable runtime event identity; recovery → final projection.What the tests prove:
event_id; a different recovery sequence produces a different identity.COUNT(DISTINCT event_id)rather than claiming a cross-store transaction.sequenceand old runtime events without recovery fields remain readable.eyJprefix.Commands run:
Security Impact
The compaction leak detector now recognizes bare JWTs by decoding the first base64url segment as a JSON object with an
algfield. This avoids false positives for Java/package-like dotted strings without depending on the common-but-not-requiredeyJprefix. JWT header validation also fails closed above 64 KiB before decode allocation or JSON parsing. No authentication, authorization, network, file-access, tool-execution, or sandbox policy changes are introduced.Reborn Trust-Boundary Checklist
Database Impact
None. No migrations, SQL schema, or persistence backend selection changed. Runtime event JSON gains additive optional recovery fields; the loop/milestone sequence has a compatibility default.
Blast Radius
Touches turn progress vocabulary, checkpointed loop state, model/capability recovery application, runner milestone projection, durable event identity, compaction secret scanning, and failure-summary projection. Tests cover every consuming seam. No new production dependency edge was added by the review fixes.
Rollback Plan
Revert
6819ff961,f2a547742,9dfa55801, and the preceding WS7 commits (579db8790,f501f6907,000120759). Previously persisted runtime events remain readable because the additions are compatible/defaulted. This removes the recovery numerator and restores the prior scanner behavior; no data migration is required.Review Follow-Through
The multi-review found and fixed:
FailureRecovered;Epic reconciliation evidence:
CompactionUnavailablecatch-all defect was retracted: [EPIC] error-recoverability endgame — the model recovers from 100% of the errors it sees #6284 (comment)failure_lane()was vacuous was retracted: [EPIC] error-recoverability endgame — the model recovers from 100% of the errors it sees #6284 (comment)Those retracted claims were verified against current production call sites and intentionally received no behavior churn.
Review track: C (security/runtime/DB/CI)