fix(reborn): surface approval-gate denial to model instead of cancelling the run - #4954
Merged
Merged
Conversation
…ing the run Approval-gate denial in Reborn cancelled the run (deny_gate / replay_denied_gate -> cancel_run), so the model never learned the user declined and the next trigger re-issued the same approval-gated capability and re-blocked — the same loop class #4944 removed for auth gates. Mirror #4944 for approval gates: denial now RESUMES the parked run carrying a denial disposition; the capability stage converts ONLY the approval-gated call into a model-visible non-retryable Authorization failure ("approval gate denied by user", SameCallRetryConstraint:: Forbidden) and the loop continues. Unrelated parallel calls are unaffected. Per the maintainability review of the plan, this unifies rather than duplicates the #4944 plumbing: - ironclaw_turns: AuthResumeDisposition -> GateResumeDisposition (one gate-agnostic enum); ResumeTurnRequest/TurnRunRecord/TurnRunState/ AgentLoopDriverResumeRequest field auth_resume_disposition -> resume_disposition. Serde key pinned to "auth_resume_disposition" (rename attr) so persisted run records still deserialize; legacy-key round-trip test added. - ironclaw_agent_loop: PendingApprovalResume gains a disposition field; the auth denied short-circuit in CapabilityStage::process is extracted into ONE shared short_circuit_denied_resume helper used by both the auth and approval paths (no second copy). - ironclaw_product_workflow: approval deny_gate / replay_denied_gate resume instead of cancel; ResolveApprovalInteractionResponse::Denied (CancelRunResponse) -> Resumed(ResumeTurnResponse); idempotent replay guarded by terminal run status. - ironclaw_reborn: PlannedDriver::resume stamps the disposition onto the pending resume that is set (auth or approval). Decisions (plan docs/plans/2026-06-15-reborn-approval-deny-continue.md): both Denied and Cancelled continue (consistent with #4944, no Cancel variant). The extension_install/extension_search missing-observation gap is a separate PR; the user-visible extension-install loop is only fully closed when both land. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplaces approval-gate cancel-on-deny flow with resume-with-denial-disposition flow, unifying auth and approval gates under ChangesGate Denial → Resume + Continue
Sequence Diagram(s)sequenceDiagram
participant ApprovalService
participant TurnCoordinator
participant PlannedDriver
participant CapabilityStage
rect rgba(200, 80, 80, 0.5)
note over ApprovalService,TurnCoordinator: Deny path (new)
ApprovalService->>TurnCoordinator: resume_turn(resume_disposition=GateResumeDisposition::Denied)
TurnCoordinator-->>ApprovalService: ResumeTurnResponse(Queued)
ApprovalService-->>ApprovalService: return Resumed (not Denied)
end
rect rgba(80, 120, 200, 0.5)
note over PlannedDriver,CapabilityStage: Executor denial short-circuit
PlannedDriver->>PlannedDriver: load checkpoint, stamp resume_disposition onto pending slot
PlannedDriver->>CapabilityStage: process(state with disposition=Denied)
CapabilityStage->>CapabilityStage: short_circuit_denied_resume — partition by denied capability
CapabilityStage->>CapabilityStage: synthesize Authorization/Forbidden via handle_capability_error
CapabilityStage-->>PlannedDriver: TurnDone (all denied) or Remaining (partial)
end
rect rgba(80, 180, 80, 0.5)
note over ApprovalService,TurnCoordinator: Idempotent replay
ApprovalService->>TurnCoordinator: get_run_state
TurnCoordinator-->>ApprovalService: TurnRunState{resume_disposition=Some(Denied)}
ApprovalService-->>ApprovalService: return Resumed idempotently (no new resume_turn call)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
size: XL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da4fe8d6d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_turns/src/memory.rs (1)
1919-1944:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject denial dispositions on non-auth/non-approval gates.
resume_dispositionis now stamped for any resumable gate, butGateResumeDisposition::Deniedonly has auth/approval semantics. With the defaultAnyBlockedGate, a resource-gate resume can persistSome(Denied), making downstream driver state look like a user-declined authorization/approval gate. Validate the actual blocked status before storing the marker.Suggested guard
if record.gate_ref.as_ref() != Some(&request.gate_resolution_ref) { return Err(TurnError::InvalidRequest { reason: "gate resolution reference mismatch".to_string(), }); } + if request.resume_disposition.is_some() + && !matches!( + record.status, + TurnStatus::BlockedApproval | TurnStatus::BlockedAuth + ) + { + return Err(TurnError::InvalidRequest { + reason: "resume disposition is only valid for approval or auth gates" + .to_string(), + }); + } let now = Utc::now(); record.status = TurnStatus::Queued; record.resume_disposition = request.resume_disposition.clone();Add a regression that blocks a resource gate and verifies
resume_turnrejectsSome(GateResumeDisposition::Denied).As per coding guidelines, “Add a regression test with every bug fix.”
🤖 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_turns/src/memory.rs` around lines 1919 - 1944, The code currently allows `GateResumeDisposition::Denied` to be stored for any resumable gate including resource gates, but this disposition should only be valid for auth and approval gates. After the existing validation checks (resumable_status, actor match, and gate_ref match), add a guard condition that rejects requests with `Some(GateResumeDisposition::Denied)` if the current record status is `TurnStatus::BlockedResource`, returning an appropriate error to prevent invalid state. Additionally, add a regression test that blocks a resource gate and verifies that attempting to resume with `Some(GateResumeDisposition::Denied)` is correctly rejected by the resume_turn method.Source: Coding guidelines
crates/ironclaw_turns/tests/checkpoint_state_store_contract.rs (1)
738-879:⚠️ Potential issue | 🟡 MinorAdd value-bearing serde round-trip tests for resume_disposition enum migration. Both tests only verify the None/missing-key default path; they lack coverage of actual enum values over the legacy wire format. Per the enum migration guideline (preserve every historical value and add round-trip deserialization tests):
crates/ironclaw_turns/tests/checkpoint_state_store_contract.rs#L738-L879: Add a case where"auth_resume_disposition": "denied"is present in the JSON and assertruns[0].resume_disposition == Some(GateResumeDisposition::Denied).crates/ironclaw_turns/tests/agent_loop_host_contract.rs#L3848-L3890: Add the same value-bearing legacy-key case forTurnRunState.🤖 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_turns/tests/checkpoint_state_store_contract.rs` around lines 738 - 879, In crates/ironclaw_turns/tests/checkpoint_state_store_contract.rs (lines 738-879), extend the turn_persistence_snapshot_legacy_run_defaults_resume_disposition_to_none test to add a second test case (or extend this test) that explicitly sets "auth_resume_disposition": "denied" in the JSON, then deserializes and asserts that runs[0].resume_disposition equals Some(GateResumeDisposition::Denied) to verify that actual enum values round-trip correctly over the legacy wire format. Apply the same fix to crates/ironclaw_turns/tests/agent_loop_host_contract.rs (lines 3848-3890) for the TurnRunState test, adding a value-bearing case for the "denied" enum value instead of only testing the None/missing-key default path.Source: Coding guidelines
crates/ironclaw_product_workflow/tests/reborn_services_contract.rs (1)
3784-3810:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename the test to match deny→resume behavior.
The test name still says
returns_cancelled, but Line 3810 now assertsRebornResolveGateResponse::Resumed(_). Please rename the test (and any nearby wording) to keep the contract unambiguous.As per coding guidelines, "When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change."
🤖 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_product_workflow/tests/reborn_services_contract.rs` around lines 3784 - 3810, The test function `approval_gate_denial_uses_approval_interaction_service_and_returns_cancelled` has a name that contradicts its actual behavior, as the assertion on line 3810 verifies `RebornResolveGateResponse::Resumed(_)` rather than a cancelled response. Rename the test function to accurately reflect that approval gate denial returns a resumed response instead of cancelled, and update any associated comments or docstrings to match the correct behavior.Source: Coding guidelines
🤖 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_agent_loop/src/executor/capabilities.rs`:
- Around line 1034-1035: The short_circuit_denied_resume function is missing the
required arch-exempt annotation comment for its clippy allow directive. Add a
comment line immediately before or after the
#[allow(clippy::too_many_arguments)] attribute in the format // arch-exempt:
too_many_args, <reason>, plan `#NNNN`, where <reason> explains why the function
needs multiple arguments and `#NNNN` is the relevant tracking issue number.
In `@crates/ironclaw_product_workflow/src/approval_interaction/service.rs`:
- Around line 391-402: The condition checking state.resume_disposition.is_some()
treats any GateResumeDisposition value as a denial replay, but it should
explicitly match only the Denied disposition. Change the condition to check if
state.resume_disposition equals the Denied variant specifically, rather than
checking for any Some value, to ensure the replay path is tied to the Denied
case explicitly as the contract is gate-agnostic.
In `@crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs`:
- Around line 1789-1791: The comment on line 1790 in the test function
replay_denied_gate_returns_stale_when_get_run_state_errors() states that the
error path returns StaleGate, but the actual assertions on lines 1813-1816
verify that ProductWorkflowError::Transient is returned instead. Update the
comment to correctly reflect that the tested behavior returns
ProductWorkflowError::Transient rather than StaleGate, ensuring the documented
intent matches the actual test contract.
In `@crates/ironclaw_turns/src/store.rs`:
- Around line 468-482: The test
`turn_run_record_resume_disposition_defaults_to_none_when_absent` does not
exercise the actual `TurnRunRecord` serde contract. Instead of only testing
`Option<GateResumeDisposition>` and the enum in isolation, create a real
`TurnRunRecord` JSON object, remove the `resume_disposition` field entirely,
deserialize it, and assert that `resume_disposition` defaults to `None`
(verifying the `#[serde(default)]` attribute works). Additionally, create
another `TurnRunRecord` JSON with the legacy field name
`auth_resume_disposition` set to `"denied"`, deserialize it, and assert that the
value correctly maps to `resume_disposition`, confirming backward compatibility
with the legacy field rename still works.
In `@docs/plans/2026-06-15-reborn-approval-deny-continue.md`:
- Line 162: Replace the typo in the test plan at line 162 where it references
`ApprovalResumeDisposition::Denied`. Change `ApprovalResumeDisposition` to
`GateResumeDisposition` to match the actual enum name that is unified in Step 1
(Carrier) as mentioned in the document.
---
Outside diff comments:
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 3784-3810: The test function
`approval_gate_denial_uses_approval_interaction_service_and_returns_cancelled`
has a name that contradicts its actual behavior, as the assertion on line 3810
verifies `RebornResolveGateResponse::Resumed(_)` rather than a cancelled
response. Rename the test function to accurately reflect that approval gate
denial returns a resumed response instead of cancelled, and update any
associated comments or docstrings to match the correct behavior.
In `@crates/ironclaw_turns/src/memory.rs`:
- Around line 1919-1944: The code currently allows
`GateResumeDisposition::Denied` to be stored for any resumable gate including
resource gates, but this disposition should only be valid for auth and approval
gates. After the existing validation checks (resumable_status, actor match, and
gate_ref match), add a guard condition that rejects requests with
`Some(GateResumeDisposition::Denied)` if the current record status is
`TurnStatus::BlockedResource`, returning an appropriate error to prevent invalid
state. Additionally, add a regression test that blocks a resource gate and
verifies that attempting to resume with `Some(GateResumeDisposition::Denied)` is
correctly rejected by the resume_turn method.
In `@crates/ironclaw_turns/tests/checkpoint_state_store_contract.rs`:
- Around line 738-879: In
crates/ironclaw_turns/tests/checkpoint_state_store_contract.rs (lines 738-879),
extend the
turn_persistence_snapshot_legacy_run_defaults_resume_disposition_to_none test to
add a second test case (or extend this test) that explicitly sets
"auth_resume_disposition": "denied" in the JSON, then deserializes and asserts
that runs[0].resume_disposition equals Some(GateResumeDisposition::Denied) to
verify that actual enum values round-trip correctly over the legacy wire format.
Apply the same fix to crates/ironclaw_turns/tests/agent_loop_host_contract.rs
(lines 3848-3890) for the TurnRunState test, adding a value-bearing case for the
"denied" enum value instead of only testing the None/missing-key default path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b2c3cf1-f517-41ec-8c62-26f86c4839f3
📒 Files selected for processing (42)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_product_workflow/src/approval_interaction/service.rscrates/ironclaw_product_workflow/src/approval_interaction/types.rscrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/workflow.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/product_workflow_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn/tests/planned_driver_e2e.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/driver.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rsdocs/plans/2026-06-15-reborn-approval-deny-continue.mdtests/support/reborn/harness.rs
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Make Reborn approval-gate denial resume the run and surface a non-retryable model-visible failure instead of cancelling and looping.
Stats: 7 findings (from 9 raw, 8 after overlap dedup, 7 after validation filtering) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
performance
- Medium Denied replay races with runner completion (
crates/ironclaw_product_workflow/src/approval_interaction/service.rs:384-401, confidence 82) — anchor: crates/ironclaw_product_workflow/src/approval_interaction/service.rs:384
The denied replay path derives idempotency from the current run state instead of replaying through resume_turn. After the first Deny resumes the run, a transport retry with the same idempotency key can arrive after the runner has already completed the run; this branch then returns StaleGate instead of the original successful ResumeTurnResponse, so the observable result depends on runner timing. Also flagged by: maintainability/Low.
tests
- Medium Approval denial resume error path is untested (
crates/ironclaw_product_workflow/src/approval_interaction/service.rs:316-330, confidence 75) — anchor: crates/ironclaw_product_workflow/src/approval_interaction/service.rs:316
deny_gate now resumes the parked run instead of cancelling it and propagates resume_turn failures through map_approval_resume_error, but the approval interaction contract tests cover successful denial resumes and replay get_run_state failures, not a resume_turn failure on this new denial path. - Medium Legacy denied resume marker lacks snapshot coverage (
crates/ironclaw_turns/src/store.rs:196-201, confidence 75) — anchor: crates/ironclaw_turns/src/store.rs:196
TurnRunRecord now exposes resume_disposition while preserving the legacy auth_resume_disposition wire key. Existing coverage checks the field defaulting to None in a snapshot and unit-level enum round-tripping, but not a full persisted snapshot carrying auth_resume_disposition: "denied". A snapshot-level rename regression could still drop a persisted denial marker.
conventions
- Medium Missing arch-exempt for too_many_arguments allow (
crates/ironclaw_agent_loop/src/executor/capabilities.rs:1034-1034, confidence 100) — anchor: .claude/rules/architecture.md:43
The diff adds #[allow(clippy::too_many_arguments)] without the required // arch-exempt: too_many_args, , plan #NNNN annotation on the line above it. The architecture rule explicitly flags new too_many_arguments allows that lack that annotation. - Medium Approval deny test copies the production branch (
crates/ironclaw_reborn/src/planned_driver.rs:1290-1295, confidence 75) — anchor: .claude/rules/testing.md:49
The new approval-deny stamping test applies the same conditional assignment inline instead of driving PlannedDriver::resume, so it can still pass if the actual resume wrapper stops stamping pending_approval_resume. The repo testing rule requires caller-level coverage when wrapper logic can silently drop a computed input before a side effect.
local-patterns
- Low Field-name note describes the wrong enclosing scope (
crates/ironclaw_agent_loop/src/state.rs:155-158, confidence 75) — anchor: crates/ironclaw_agent_loop/src/state.rs:156 changed comment
The updated comment says PendingAuthResume::disposition is short because the enclosing type scopes it to gate-resume context, but the enclosing type is still auth-specific. That makes the naming rationale stale where readers compare auth versus approval pending-resume fields.
maintainability
- Medium Gate disposition is stamped through a comment-only invariant (
crates/ironclaw_reborn/src/planned_driver.rs:169-175, confidence 75) — anchor: crates/ironclaw_reborn/src/planned_driver.rs:169
The driver receives a gate-agnostic resume disposition and compensates by stamping it onto both pending auth and pending approval slots, relying on the comment that only one slot can exist. That makes the resume boundary depend on a hidden invariant instead of carrying the gate identity that was already known when the run was resumed.
…slot only Review round 1 fixes: - planned_driver: the denial disposition was stamped onto BOTH pending_auth_resume and pending_approval_resume on a comment-only "one slot at a time" invariant. GateStage deliberately preserves a pending auth resume when a non-auth gate blocks mid-re-dispatch, so both slots can be set at once; stamping both corrupted an unrelated auth resume. Now stamps only the pending slot whose gate_ref matches the blocking gate (state.last_gate). Adds a regression test asserting the auth slot stays None when the approval gate is denied, plus an end-to-end resume() drive. - approval replay: match GateResumeDisposition::Denied explicitly rather than is_some(), keeping the gate-agnostic carrier tied to denial. - tests: real TurnRunRecord struct-level serde test (legacy auth_resume_disposition key → resume_disposition) + snapshot-level legacy denied-marker test; new deny-path resume-error test asserting the record is denied and the run is never cancelled on resume failure. - arch-exempt annotation on short_circuit_denied_resume's too_many_arguments allow (plan #4954); stale comments/typos fixed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2026-06-15-reborn-approval-deny-continue.md (1)
85-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStep 2: field name inconsistency with Step 1.
Line 85 references
approval_resume_dispositionbut Step 1 establishes the unified field name asresume_disposition: Option<GateResumeDisposition>(line 68). The actual service code (context snippet 2) confirms it'sresume_disposition. Fix to maintain consistency within the plan.🔧 Proposed fix
- marks the approval record `Denied` (unchanged durable write), then - `resume_turn(precondition = BlockedApprovalGate, - approval_resume_disposition = Some(Denied))`, returning + resume_disposition = Some(Denied))`, returning `ResolveApprovalInteractionResponse::Resumed(ResumeTurnResponse)`.🤖 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 `@docs/plans/2026-06-15-reborn-approval-deny-continue.md` at line 85, Change the field name reference on line 85 from `approval_resume_disposition` to `resume_disposition` to maintain consistency with the unified field name established in Step 1 (line 68), which defines the field as `resume_disposition: Option<GateResumeDisposition>`. This aligns the documentation with the actual service code structure.
🤖 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_product_workflow/tests/approval_interaction_contract.rs`:
- Around line 1830-1874: The test
`deny_marks_pending_gate_denied_then_propagates_resume_error_without_cancelling`
currently only asserts call counts (denial_count, resumption_count,
cancellation_count) which do not verify the call order. To ensure the denial
write happens before resume_turn as the test comment claims, add a shared
ordered trace or event log to the resolver and coordinator fixtures that records
when deny and resume_turn methods are invoked, then update the assertions to
verify that the denial event occurs strictly before the resumption event in the
recorded sequence. This will lock the contract against regressions where call
order changes while counts remain the same.
In `@crates/ironclaw_reborn/src/planned_driver.rs`:
- Around line 261-269: The current if-else if structure silently picks the first
matching slot when both pending_auth_resume and pending_approval_resume have the
same gate_ref matching last_gate, allowing an approval denial to be incorrectly
written to the auth slot. Before the if-else if block that sets
pending.disposition, add a check that detects when both
state.pending_auth_resume and state.pending_approval_resume have matching
gate_ref values equal to last_gate, and fail closed by returning an error or
asserting when this ambiguous condition occurs. This ensures the denial
attribution boundary is preserved and only one slot receives the disposition for
any given gate.
---
Outside diff comments:
In `@docs/plans/2026-06-15-reborn-approval-deny-continue.md`:
- Line 85: Change the field name reference on line 85 from
`approval_resume_disposition` to `resume_disposition` to maintain consistency
with the unified field name established in Step 1 (line 68), which defines the
field as `resume_disposition: Option<GateResumeDisposition>`. This aligns the
documentation with the actual service code structure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 966fffa7-443b-48d2-9c67-21da5827d30f
📒 Files selected for processing (8)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_product_workflow/src/approval_interaction/service.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rsdocs/plans/2026-06-15-reborn-approval-deny-continue.md
… (auth + approval) Review finding #7: the denied-gate replay paths derived idempotency from current run state (TurnStatus::is_terminal() guard) rather than replaying through resume_turn. After the first Deny resumed the run, a transport retry with the same idempotency key arriving after the runner completed returned StaleGate/StaleAuth instead of the original ResumeTurnResponse — the observable result depended on runner timing. resume_turn is idempotent by key (memory.rs:665 returns the cached Result from resume_idempotency before the precondition check). Both the approval (replay_denied_gate) and auth (resume_denied_auth replay arm) paths now replay through resume_turn with the same key, deleting the terminal-guard branching: a retried key replays the original response regardless of run state; a genuinely stale request with a fresh key still errors via the precondition. Auth and approval kept symmetric. FakeTurnCoordinator now models resume idempotency by key so the replay tests are meaningful; terminal-guard assertions re-framed around same-key replay vs fresh-key stale, plus an explicit idempotent-replay test on both services. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…re-resume order Review round 2 (both Major): - planned_driver stamp_resume_disposition: the if/else-if silently stamped the auth slot if both pending slots matched last_gate. At the denial- attribution boundary that could misattribute an approval denial. Now an explicit 4-way match fails closed on the ambiguous (true, true) case (warn + stamp neither). Test added. - approval_interaction_contract deny-resume-error test: asserted only aggregate call counts, which pass even if call order regressed. Added a shared ordered trace across the resolver (deny) and coordinator (resume_turn) fakes and assert deny is recorded strictly before resume. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_product_workflow/tests/approval_interaction_contract.rs`:
- Around line 1953-1955: The assertion on line 1954 only compares run_id values
between the first and second responses, but the test intent is to verify that
the full ResumeTurnResponse payload is identical (same cached result). To
strengthen this assertion, change it to compare the complete first and second
Resumed response objects or payloads directly (not just extracted run_id
fields), ensuring that fields like status and event_cursor are also validated.
This prevents regressions in other response fields from passing undetected.
In `@crates/ironclaw_product_workflow/tests/auth_interaction_contract.rs`:
- Around line 866-961: The test
idempotent_auth_deny_replay_returns_same_resumed_response_as_first_deny
currently only verifies that run_id matches between the first and second
responses, but the test name and comment promise that the entire resumed
response should be identical. Enhance the assertions at the end of the test to
compare the full ResolveAuthInteractionResponse::Resumed payload between
first_response and second_response, including not just run_id but also status
and event_cursor fields. This ensures that regressions in replayed status or
event_cursor values are caught.
In `@crates/ironclaw_reborn/src/planned_driver.rs`:
- Around line 285-291: Change the `tracing::warn!` call on line 288 in the
planned_driver.rs file to `tracing::debug!` to prevent REPL/TUI corruption. This
is an internal fail-closed diagnostic for an impossible state that should not
emit warn-level logs; keep internal diagnostics at debug level unless they are
routed through a user-facing status surface per the coding guidelines.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b930e6e-153e-4e3a-a12d-3784b97891a2
📒 Files selected for processing (5)
crates/ironclaw_product_workflow/src/approval_interaction/service.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn/src/planned_driver.rs
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Change Reborn approval-gate denials to resume the run and surface a model-visible non-retryable authorization failure instead of cancelling.
Stats: 4 findings (from 6 raw, 4 after filtering/dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Current head reviewed: f99e8c79c6a34a524dadd9fe8beff4759e41754f.
Bugs
- High Denied approval resumes the run but the WebUI still treats it as terminal (
crates/ironclaw_product_workflow/src/approval_interaction/service.rs:316-327, confidence 88) — anchor:crates/ironclaw_product_workflow/src/approval_interaction/service.rs:316
This path now resolves approval Deny/Cancel by callingresume_turnwithGateResumeDisposition::Deniedand RebornServices maps the result toRebornResolveGateResponse::Resumed, so the run continues. The WebUIresolveGatehandler still computesshouldContinueProcessingonly forapprovedorcredential_provided, then clearsactiveRunfor denied/cancelled.
Tests
-
Medium Approval gate checkpoint never asserts disposition starts empty (
crates/ironclaw_agent_loop/src/executor/gates.rs:81-87, confidence 76) — anchor:crates/ironclaw_agent_loop/src/executor/gates.rs:81
GateStagenow writesPendingApprovalResume { disposition: None }for the first parked approval checkpoint, but the executor approval-block test only checks the resume token and approval_request_id. -
Medium Denied-approval short circuit lacks a no-matching-call test (
crates/ironclaw_agent_loop/src/executor/capabilities.rs:188-213, confidence 72) — anchor:crates/ironclaw_agent_loop/src/executor/capabilities.rs:188
The approval-deny short circuit explicitly clears stale denied state even when partitioning finds no matching capability calls, then falls through with the untouched batch. Existing executor tests cover one denied call and a mixed matching/unrelated batch, but not the no-match case.
Maintainability
- Medium Denied-resume helper carries more control flow than it removes (
crates/ironclaw_agent_loop/src/executor/capabilities.rs:1011-1068, confidence 82) — anchor:crates/ironclaw_agent_loop/src/executor/capabilities.rs:1011
The shared helper now introducesDeniedResumeOutcome, boxesLoopExecutionState, and clonescapability_batchto report the empty-remaining case back throughcompleted_turn, making the abstraction fairly heavy for two callers.
…losed log to debug Review round 3 (straightforward): - idempotent deny-replay tests (auth + approval) now assert full ResumeTurnResponse payload equality, not just run_id. - stamp_resume_disposition ambiguous-dual-slot diagnostic: warn! -> debug! (REPL/TUI logging rule — internal fail-closed diagnostics use debug!). - executor: assert the first approval BeforeBlock checkpoint carries pending_approval_resume.disposition == None before any denial. - executor: denied-approval short-circuit no-matching-call test (denied X, model emits only Y -> X not surfaced, Y dispatches, pending cleared). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (3)
crates/ironclaw_reborn/src/planned_driver.rs (1)
251-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale
warn!comments after the debug downgrade.Line 288 now emits
debug!, but Lines 251-253 and Line 1636 still describe the ambiguous case as warning. That stale contract can invite reintroducingwarn!despite the REPL/TUI invariant. As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”Proposed fix
-/// in practice), the function stamps NEITHER and emits a `warn!` — failing +/// in practice), the function stamps NEITHER and emits a `debug!` — failing /// closed rather than misattributing the denial. @@ - // Call with the ambiguous state — should warn and stamp neither. + // Call with the ambiguous state — should log at debug and stamp neither.Also applies to: 288-288, 1636-1637
🤖 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_reborn/src/planned_driver.rs` around lines 251 - 253, The documentation and comments describing the ambiguous case behavior are now stale after the debug downgrade. In crates/ironclaw_reborn/src/planned_driver.rs, update the docstring at lines 251-253 to accurately state that the function emits `debug!` (not `warn!`) when both slots carry the same gate_ref, and similarly update the comment at lines 1636-1637 to reflect the same behavior. Ensure all three locations (the docstring anchor, the debug statement at line 288, and the trailing comment) consistently describe that `debug!` is emitted for the ambiguous case.Source: Coding guidelines
crates/ironclaw_product_workflow/tests/auth_interaction_contract.rs (1)
929-957: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake auth replay cache hits observable.
A replay with the wrong key can still pass because cache miss creates the same
ResumeTurnResponseshape as the seeded cache. Seed from the first response where available, then setresume_error; cache hits still succeed, cache misses fail. As per coding guidelines, “Add a regression test with every bug fix.”Proposed test tightening
coordinator2.seed_resume_cache( run_id, IdempotencyKey::new("idem-auth-deny").unwrap(), - ResumeTurnResponse { - run_id, - status: TurnStatus::Queued, - event_cursor: EventCursor(41), - }, + first_resumed.clone(), ); + coordinator2.set_resume_error(TurnError::Unavailable { + reason: "fresh replay must hit idempotency cache".to_string(), + }); @@ coordinator.seed_resume_cache( run_id, IdempotencyKey::new("auth-action-replay-denied").unwrap(), ResumeTurnResponse { run_id, status: TurnStatus::Queued, event_cursor: EventCursor(41), }, ); + coordinator.set_resume_error(TurnError::Unavailable { + reason: "fresh replay must hit idempotency cache".to_string(), + }); @@ coordinator.seed_resume_cache( run_id, IdempotencyKey::new("auth-action-replay-denied-terminal").unwrap(), ResumeTurnResponse { run_id, status: TurnStatus::Queued, event_cursor: EventCursor(41), }, ); + coordinator.set_resume_error(TurnError::Unavailable { + reason: "fresh replay must hit idempotency cache".to_string(), + });Also applies to: 1115-1154, 1246-1282
🤖 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_product_workflow/tests/auth_interaction_contract.rs` around lines 929 - 957, The test does not distinguish between cache hits and cache misses because both return the same ResumeTurnResponse shape. Modify the seeded response in the seed_resume_cache call for the first Deny response to include a resume_error field (or equivalent distinguishing field) so that when the second request with the same idempotency_key is made, the cached response will have this error marker. Update the assertion at the end of the second response check to verify that the returned response includes this error marker, ensuring it proves the cache was actually hit. Apply the same modification pattern to the other affected test locations at lines 1115-1154 and 1246-1282 where similar cache hit verification is needed.Source: Coding guidelines
crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs (1)
1766-1790: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake cache misses fail in the replay tests.
These tests claim idempotent replay, but
FakeTurnCoordinator::resume_turnreturns the same fresh payload on cache miss, so a wrong idempotency key can still pass Lines 1953-1954. Armresume_errorafter the cache is seeded/created; cache hits bypass it, cache misses fail. As per coding guidelines, “Add a regression test with every bug fix.”Proposed test tightening
coordinator.seed_resume_cache( run_id, IdempotencyKey::new("replay-denied-idempotent").expect("idempotency"), cached_response.clone(), ); + coordinator.set_resume_error(TurnError::Unavailable { + reason: "fresh replay must hit idempotency cache".to_string(), + }); @@ // Simulate the transition: run left BlockedApproval and the gate is now Denied. coordinator.set_status(TurnStatus::Queued); + coordinator.set_resume_error(TurnError::Unavailable { + reason: "fresh replay must hit idempotency cache".to_string(), + }); // ── Second call: replay Deny with SAME key, gate now Denied ───────────────Also applies to: 1901-1954
🤖 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_product_workflow/tests/approval_interaction_contract.rs` around lines 1766 - 1790, After calling coordinator.seed_resume_cache() to populate the cache, arm the resume_error on the coordinator to fail subsequent calls that miss the cache. This allows cache hits to return the cached result (the expected idempotent behavior), while cache misses from incorrect idempotency keys will properly fail. This pattern should be applied at all affected test sites to ensure idempotent replay validation is robust and cannot pass with wrong idempotency keys.Source: Coding guidelines
🤖 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_product_workflow/tests/approval_interaction_contract.rs`:
- Around line 1766-1790: After calling coordinator.seed_resume_cache() to
populate the cache, arm the resume_error on the coordinator to fail subsequent
calls that miss the cache. This allows cache hits to return the cached result
(the expected idempotent behavior), while cache misses from incorrect
idempotency keys will properly fail. This pattern should be applied at all
affected test sites to ensure idempotent replay validation is robust and cannot
pass with wrong idempotency keys.
In `@crates/ironclaw_product_workflow/tests/auth_interaction_contract.rs`:
- Around line 929-957: The test does not distinguish between cache hits and
cache misses because both return the same ResumeTurnResponse shape. Modify the
seeded response in the seed_resume_cache call for the first Deny response to
include a resume_error field (or equivalent distinguishing field) so that when
the second request with the same idempotency_key is made, the cached response
will have this error marker. Update the assertion at the end of the second
response check to verify that the returned response includes this error marker,
ensuring it proves the cache was actually hit. Apply the same modification
pattern to the other affected test locations at lines 1115-1154 and 1246-1282
where similar cache hit verification is needed.
In `@crates/ironclaw_reborn/src/planned_driver.rs`:
- Around line 251-253: The documentation and comments describing the ambiguous
case behavior are now stale after the debug downgrade. In
crates/ironclaw_reborn/src/planned_driver.rs, update the docstring at lines
251-253 to accurately state that the function emits `debug!` (not `warn!`) when
both slots carry the same gate_ref, and similarly update the comment at lines
1636-1637 to reflect the same behavior. Ensure all three locations (the
docstring anchor, the debug statement at line 288, and the trailing comment)
consistently describe that `debug!` is emitted for the ambiguous case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96af8e2a-67a1-4ead-b8a0-96e9b1555a9b
📒 Files selected for processing (4)
crates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_reborn/src/planned_driver.rs
… resume Review round 3 (design + High): - WebUiGateResolution: the approval card sends `denied`, the auth cards send `cancelled`, and both are now treated identically (resume the run and surface the decision to the model). Run termination is a separate control (the X -> cancelRun route), not a gate resolution. Collapsed the two equivalent variants into one `Declined` (serde aliases "denied"/"cancelled" keep the wire stable; no JS change). Facade maps Declined -> Deny for auth, approval, and the generic fallback. - #6 WebUI desync (High): useChat.resolveGate kept processing only for approved/credential_provided, dropping processing + activeRun on denied/cancelled — but those now resume the run. resolveGate now always keeps processing/activeRun; the terminal run_status SSE event clears it and the X/cancelRun path remains the only stop. Fixes the latent auth-cancelled desync from #4944. assets.rs assertion + useChat tests updated. - #7 helper weight: short_circuit_denied_resume no longer returns the DeniedResumeOutcome enum / boxes LoopExecutionState / clones the batch. It returns ControlFlow<TurnCompletedStep, (state, remaining_calls)>; the completed_turn/empty-remaining tail moved to the two call sites. Heavy per-denied-call failure synthesis stays shared (one helper). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (3)
docs/plans/2026-06-15-reborn-approval-deny-continue.md (1)
113-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistency: "Proposed change" Step 5 does not match "Decisions" Q3.
Step 5 proposes keeping both
DeniedandCancelledenum variants mapping to the same decision. However, "Decisions" Q3 (lines 141–151) clearly states the variants were unified into a singleDeclinedvariant with serde aliases. The PR objectives confirm: "WebUiGateResolutionenum collapses... into a singleDeclinedvariant."Update Step 5 to reflect the unified-variant decision to avoid confusing future readers about the actual approach taken.
Suggested fix for Step 5
4. **Response type.** `ResolveApprovalInteractionResponse` gains `Resumed(ResumeTurnResponse)`; `Denied(CancelRunResponse)` is removed (mirrors `#4944` L-b deleting `DenialResumed`). Callers in `workflow.rs` / `reborn_services.rs` collapse to the resumed path. -5. **Cancel vs Deny.** Keep both `WebUiGateResolution::Denied` and - `::Cancelled` mapping to `ApprovalInteractionDecision::Deny` → - resume + continue, matching `#4944`'s final decision and the user's - stated intent ("approval cancel also doing the same"). No new - `Cancel` decision variant. +5. **Cancel vs Deny (unified).** Unify `WebUiGateResolution::Denied` and + `::Cancelled` into a single `Declined` variant with serde aliases + `"denied"` and `"cancelled"` for wire stability. This maps to + `ApprovalInteractionDecision::Deny` → resume + continue, matching + `#4944`'s final decision and the user's stated intent ("approval + cancel also doing the same"). No new `Cancel` decision variant.🤖 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 `@docs/plans/2026-06-15-reborn-approval-deny-continue.md` around lines 113 - 117, Step 5 currently describes keeping both `WebUiGateResolution::Denied` and `::Cancelled` as separate variants that map to the same decision, but the "Decisions" Q3 section clarifies that these variants were actually unified into a single `Declined` variant with serde aliases for backward compatibility. Update Step 5 to accurately describe the unified-variant approach (collapsing both into one `Declined` variant with aliases) rather than the dual-variant mapping approach, ensuring consistency with the actual decision documented in the "Decisions" section and the PR objectives.crates/ironclaw_product_workflow/src/webui_inbound.rs (1)
540-551:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept canonical
"declined"in gate-resolution parsing.
WebUiGateResolutionnow canonicalizes decline to"declined", butparse_gate_resolutionrejects that token and only accepts legacy"denied"/"cancelled". This creates a wire-contract mismatch and can fail valid canonical requests.Suggested patch
fn parse_gate_resolution( resolution: Option<String>, always: Option<bool>, credential_ref: Option<String>, ) -> Result<WebUiGateResolution, WebUiInboundValidationError> { let resolution = required_text("resolution", resolution, 64, TextMode::Token)?; match resolution.as_str() { "approved" => Ok(WebUiGateResolution::Approved { always: always.unwrap_or(false), }), - "denied" | "cancelled" => Ok(WebUiGateResolution::Declined), + "declined" | "denied" | "cancelled" => Ok(WebUiGateResolution::Declined), "credential_provided" => Ok(WebUiGateResolution::CredentialProvided { credential_ref: required_text( "credential_ref", credential_ref, CREDENTIAL_REF_MAX_BYTES, TextMode::Token, )?, }), _ => Err(WebUiInboundValidationError::new( "resolution", WebUiInboundValidationCode::InvalidValue, )), } }🤖 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_product_workflow/src/webui_inbound.rs` around lines 540 - 551, The parse_gate_resolution function currently only accepts the legacy tokens "denied" and "cancelled" for declined gate resolutions, but the canonical form is now "declined". Update the match pattern in parse_gate_resolution to also accept "declined" alongside the legacy options so that all three tokens ("denied", "cancelled", and "declined") map to the WebUiGateResolution::Declined result, eliminating the wire-contract mismatch.crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
1047-1062:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPut the denial reason in the model-visible observation.
Line 1052 leaves
CapabilityFailure.safe_summaryempty, while Line 1061 puts"auth/approval gate denied by user"only in the planner summary. That misses this PR’s contract that the denial is visible to the model as the refusal text; passplanner_summaryinto the synthesized failure so theAuthorizationobservation carries the user-denied reason.Proposed fix
let failure = ironclaw_turns::run_profile::CapabilityFailure { error_kind: CapabilityFailureKind::Authorization, - // Intentionally empty: model-visible text comes from - // `model_visible_capability_failure_observation` and the - // planner summary from `from_trusted_static` below. - safe_summary: String::new(), + // Keep the planner summary and model-visible observation aligned + // so the model sees the explicit user denial, not a blank + // authorization failure. + safe_summary: planner_summary.to_string(), detail: None, };🤖 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_agent_loop/src/executor/capabilities.rs` around lines 1047 - 1062, The CapabilityFailure struct created in the authorization denial case has its safe_summary field left empty, which means the model-visible observation generated by model_visible_capability_failure_observation does not contain the denial reason. Instead, the planner_summary containing the denial message is only added to the CapabilityErrorSummary. To fix this, populate the safe_summary field of the CapabilityFailure struct with the planner_summary value so that the denial reason becomes part of the model-visible observation that gets passed to model_visible_capability_failure_observation.
🤖 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_agent_loop/src/executor/capabilities.rs`:
- Around line 1047-1062: The CapabilityFailure struct created in the
authorization denial case has its safe_summary field left empty, which means the
model-visible observation generated by
model_visible_capability_failure_observation does not contain the denial reason.
Instead, the planner_summary containing the denial message is only added to the
CapabilityErrorSummary. To fix this, populate the safe_summary field of the
CapabilityFailure struct with the planner_summary value so that the denial
reason becomes part of the model-visible observation that gets passed to
model_visible_capability_failure_observation.
In `@crates/ironclaw_product_workflow/src/webui_inbound.rs`:
- Around line 540-551: The parse_gate_resolution function currently only accepts
the legacy tokens "denied" and "cancelled" for declined gate resolutions, but
the canonical form is now "declined". Update the match pattern in
parse_gate_resolution to also accept "declined" alongside the legacy options so
that all three tokens ("denied", "cancelled", and "declined") map to the
WebUiGateResolution::Declined result, eliminating the wire-contract mismatch.
In `@docs/plans/2026-06-15-reborn-approval-deny-continue.md`:
- Around line 113-117: Step 5 currently describes keeping both
`WebUiGateResolution::Denied` and `::Cancelled` as separate variants that map to
the same decision, but the "Decisions" Q3 section clarifies that these variants
were actually unified into a single `Declined` variant with serde aliases for
backward compatibility. Update Step 5 to accurately describe the unified-variant
approach (collapsing both into one `Declined` variant with aliases) rather than
the dual-variant mapping approach, ensuring consistency with the actual decision
documented in the "Decisions" section and the PR objectives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ecce1cb6-649c-429f-9012-293bf7293522
📒 Files selected for processing (8)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/webui_inbound_contract.rscrates/ironclaw_webui_v2_static/src/assets.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjsdocs/plans/2026-06-15-reborn-approval-deny-continue.md
henrypark133
commented
Jun 16, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Change Reborn approval-gate denial handling so a denial resumes the parked run with a denial disposition instead of cancelling the run, letting the model observe the refusal and continue without re-triggering the same gated capability.
Stats: 1 finding (from 4 raw, 1 after validation/dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Maintainability
- Medium Collapse the denied-approval replay into one resume helper (
crates/ironclaw_product_workflow/src/approval_interaction/service.rs:357-380, confidence 78) — anchor:crates/ironclaw_product_workflow/src/approval_interaction/service.rs:357
deny_gateandreplay_denied_gatenow both build the sameResumeTurnRequest, map the same turn errors, and return the sameResumedshape. The only real difference is the one-offresolver.denyside effect in the first-time path, so splitting the resume logic into two methods leaves readers chasing two near-identical flows for one outcome.
…replay Review (Medium): deny_gate and replay_denied_gate built an identical ResumeTurnRequest, mapped the same errors, and returned the same Resumed shape — the only difference was deny_gate's one-off resolver.deny side effect. Extracted a shared resume_denied(request, run_id) helper; deny_gate performs the durable denial then delegates to it, and replay_denied_gate calls it directly. Removes the duplicated request construction / path handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 17, 2026
This was referenced Jun 17, 2026
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…ing the run (nearai#4954) * fix(reborn): surface approval-gate denial to model instead of cancelling the run Approval-gate denial in Reborn cancelled the run (deny_gate / replay_denied_gate -> cancel_run), so the model never learned the user declined and the next trigger re-issued the same approval-gated capability and re-blocked — the same loop class nearai#4944 removed for auth gates. Mirror nearai#4944 for approval gates: denial now RESUMES the parked run carrying a denial disposition; the capability stage converts ONLY the approval-gated call into a model-visible non-retryable Authorization failure ("approval gate denied by user", SameCallRetryConstraint:: Forbidden) and the loop continues. Unrelated parallel calls are unaffected. Per the maintainability review of the plan, this unifies rather than duplicates the nearai#4944 plumbing: - ironclaw_turns: AuthResumeDisposition -> GateResumeDisposition (one gate-agnostic enum); ResumeTurnRequest/TurnRunRecord/TurnRunState/ AgentLoopDriverResumeRequest field auth_resume_disposition -> resume_disposition. Serde key pinned to "auth_resume_disposition" (rename attr) so persisted run records still deserialize; legacy-key round-trip test added. - ironclaw_agent_loop: PendingApprovalResume gains a disposition field; the auth denied short-circuit in CapabilityStage::process is extracted into ONE shared short_circuit_denied_resume helper used by both the auth and approval paths (no second copy). - ironclaw_product_workflow: approval deny_gate / replay_denied_gate resume instead of cancel; ResolveApprovalInteractionResponse::Denied (CancelRunResponse) -> Resumed(ResumeTurnResponse); idempotent replay guarded by terminal run status. - ironclaw_reborn: PlannedDriver::resume stamps the disposition onto the pending resume that is set (auth or approval). Decisions (plan docs/plans/2026-06-15-reborn-approval-deny-continue.md): both Denied and Cancelled continue (consistent with nearai#4944, no Cancel variant). The extension_install/extension_search missing-observation gap is a separate PR; the user-visible extension-install loop is only fully closed when both land. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): address PR nearai#4954 review — stamp denial on matching gate slot only Review round 1 fixes: - planned_driver: the denial disposition was stamped onto BOTH pending_auth_resume and pending_approval_resume on a comment-only "one slot at a time" invariant. GateStage deliberately preserves a pending auth resume when a non-auth gate blocks mid-re-dispatch, so both slots can be set at once; stamping both corrupted an unrelated auth resume. Now stamps only the pending slot whose gate_ref matches the blocking gate (state.last_gate). Adds a regression test asserting the auth slot stays None when the approval gate is denied, plus an end-to-end resume() drive. - approval replay: match GateResumeDisposition::Denied explicitly rather than is_some(), keeping the gate-agnostic carrier tied to denial. - tests: real TurnRunRecord struct-level serde test (legacy auth_resume_disposition key → resume_disposition) + snapshot-level legacy denied-marker test; new deny-path resume-error test asserting the record is denied and the run is never cancelled on resume failure. - arch-exempt annotation on short_circuit_denied_resume's too_many_arguments allow (plan nearai#4954); stale comments/typos fixed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): route denied-gate replay through resume_turn idempotency (auth + approval) Review finding #7: the denied-gate replay paths derived idempotency from current run state (TurnStatus::is_terminal() guard) rather than replaying through resume_turn. After the first Deny resumed the run, a transport retry with the same idempotency key arriving after the runner completed returned StaleGate/StaleAuth instead of the original ResumeTurnResponse — the observable result depended on runner timing. resume_turn is idempotent by key (memory.rs:665 returns the cached Result from resume_idempotency before the precondition check). Both the approval (replay_denied_gate) and auth (resume_denied_auth replay arm) paths now replay through resume_turn with the same key, deleting the terminal-guard branching: a retried key replays the original response regardless of run state; a genuinely stale request with a fresh key still errors via the precondition. Auth and approval kept symmetric. FakeTurnCoordinator now models resume idempotency by key so the replay tests are meaningful; terminal-guard assertions re-framed around same-key replay vs fresh-key stale, plus an explicit idempotent-replay test on both services. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): fail closed on ambiguous dual-slot stamp; lock deny-before-resume order Review round 2 (both Major): - planned_driver stamp_resume_disposition: the if/else-if silently stamped the auth slot if both pending slots matched last_gate. At the denial- attribution boundary that could misattribute an approval denial. Now an explicit 4-way match fails closed on the ambiguous (true, true) case (warn + stamp neither). Test added. - approval_interaction_contract deny-resume-error test: asserted only aggregate call counts, which pass even if call order regressed. Added a shared ordered trace across the resolver (deny) and coordinator (resume_turn) fakes and assert deny is recorded strictly before resume. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): strengthen replay/checkpoint coverage; downgrade fail-closed log to debug Review round 3 (straightforward): - idempotent deny-replay tests (auth + approval) now assert full ResumeTurnResponse payload equality, not just run_id. - stamp_resume_disposition ambiguous-dual-slot diagnostic: warn! -> debug! (REPL/TUI logging rule — internal fail-closed diagnostics use debug!). - executor: assert the first approval BeforeBlock checkpoint carries pending_approval_resume.disposition == None before any denial. - executor: denied-approval short-circuit no-matching-call test (denied X, model emits only Y -> X not surfaced, Y dispatches, pending cleared). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): unify gate Declined resolution; keep WebUI processing on resume Review round 3 (design + High): - WebUiGateResolution: the approval card sends `denied`, the auth cards send `cancelled`, and both are now treated identically (resume the run and surface the decision to the model). Run termination is a separate control (the X -> cancelRun route), not a gate resolution. Collapsed the two equivalent variants into one `Declined` (serde aliases "denied"/"cancelled" keep the wire stable; no JS change). Facade maps Declined -> Deny for auth, approval, and the generic fallback. - #6 WebUI desync (High): useChat.resolveGate kept processing only for approved/credential_provided, dropping processing + activeRun on denied/cancelled — but those now resume the run. resolveGate now always keeps processing/activeRun; the terminal run_status SSE event clears it and the X/cancelRun path remains the only stop. Fixes the latent auth-cancelled desync from nearai#4944. assets.rs assertion + useChat tests updated. - #7 helper weight: short_circuit_denied_resume no longer returns the DeniedResumeOutcome enum / boxes LoopExecutionState / clones the batch. It returns ControlFlow<TurnCompletedStep, (state, remaining_calls)>; the completed_turn/empty-remaining tail moved to the two call sites. Heavy per-denied-call failure synthesis stays shared (one helper). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): share denied-approval resume between deny_gate and replay Review (Medium): deny_gate and replay_denied_gate built an identical ResumeTurnRequest, mapped the same errors, and returned the same Resumed shape — the only difference was deny_gate's one-off resolver.deny side effect. Extracted a shared resume_denied(request, run_id) helper; deny_gate performs the durable denial then delegates to it, and replay_denied_gate calls it directly. Removes the duplicated request construction / path handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 23, 2026
5 tasks
33 tasks
29 tasks
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.
Problem
Approval-gate denial in Reborn cancelled the run (
deny_gate/replay_denied_gate→cancel_run), so the model never learned the userdeclined and the next trigger re-issued the same approval-gated capability and
re-blocked — the same loop class #4944 removed for auth gates. User-visible
symptom (extension-install flow): pressing Deny/Cancel "didn't return anything
from the LLM and just put me in the same loop."
Fix
Mirror #4944 for approval gates: denial now resumes the parked run carrying a
denial disposition. The capability stage converts only the approval-gated
call into a model-visible non-retryable
Authorizationfailure ("approval gate denied by user",SameCallRetryConstraint::Forbidden) and the loop continues.Unrelated parallel calls in the same batch are unaffected (partition-by-
capability-id, batch policy recomputed after partition — the guarantees #4944's
review established).
Unify, don't duplicate (per plan review — maintainability lens)
ironclaw_turns—AuthResumeDisposition→GateResumeDisposition(onegate-agnostic enum); field
auth_resume_disposition→resume_dispositiononResumeTurnRequest/TurnRunRecord/TurnRunState/AgentLoopDriverResumeRequest.Serde key pinned to
"auth_resume_disposition"(rename attr) so persisted runrecords still deserialize; legacy-key round-trip test added.
ironclaw_agent_loop—PendingApprovalResumegains adisposition; theauth denied short-circuit in
CapabilityStage::processis extracted into oneshared
short_circuit_denied_resumehelper used by both auth and approvalpaths (no second copy).
ironclaw_product_workflow— approvaldeny_gate/replay_denied_gateresume instead of cancel;
ResolveApprovalInteractionResponse::Denied( CancelRunResponse)→Resumed(ResumeTurnResponse); idempotent replay guardedby terminal run status.
ironclaw_reborn—PlannedDriver::resumestamps the disposition onto thepending resume that is set (auth or approval).
Decisions
Plan:
docs/plans/2026-06-15-reborn-approval-deny-continue.md(reviewed inplan-mode by approach / local-patterns / maintainability lenses).
DeniedandCancelledcontinue, consistent with fix(reborn): surface auth-gate denial to model instead of re-prompt loop #4944. NoCanceldecision variant.extension_install/extension_searchmissing-observation gap is aseparate PR2. The user-visible extension-install loop is only fully closed
when both land — gate the rollout so deny-continue cannot produce a new blind
loop on its own.
Test plan
reborn → composition).
PendingApprovalResumedenied-disposition checkpoint round-trip; caller-level denied-approval
short-circuit fails only the matching call while an unrelated parallel call
proceeds (agent_loop); parked-gate deny resumes + idempotent terminal-run
replay guard +
get_run_stateerror path (product_workflow); preconditionstamping (reborn).
cargo fmt --allclean ·cargo check --workspace --all-featuresclean ·cargo clippy --all --benches --tests --examples --all-featureszerowarnings ·
cargo check -p ironclaw_reborn_composition --features slack-v2-host-betaclean ·cargo test --workspacegreen (one pre-existingflaky SQLite-pool test
identity_files_not_in_list_from_secondary_scope,file untouched by this branch, passes in isolation).
🤖 Generated with Claude Code