fix(reborn): stabilize tool activity display and gate resume flows - #4978
Conversation
Emit denied capability activity as a durable failed activity, surface denied gates back into the model loop, and preserve live WebUI activity across gate resolution/history refresh.
|
This PR was not deployed automatically as @hanakannzashi does not have access to the Railway project. In order to get automatic PR deploys, please add @hanakannzashi to your workspace on Railway. |
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDenied capability resumes now emit correlated authorization failures. Activity projections carry stable ordering metadata, WebUI tool-state/history flows preserve terminal tool entries, and NEAR AI tool-message flattening is now opt-in. ChangesDenied authorization failure, activity ordering, and WebUI reconciliation
NEAR AI tool-message flattening made opt-in
Sequence Diagram(s)sequenceDiagram
participant CapabilityStage
participant short_circuit_denied_resume
participant LoopProgressEvent
participant Model
CapabilityStage->>CapabilityStage: denied_activity_id = pending.activity_id_for_resume()
CapabilityStage->>short_circuit_denied_resume: denied_activity_id
short_circuit_denied_resume->>LoopProgressEvent: emit CapabilityActivityFailed(Authorization)
short_circuit_denied_resume->>Model: handle_capability_error
sequenceDiagram
participant useChat
participant failGateToolActivity
participant useChatEvents
participant locallyResolvedGatesRef
useChat->>failGateToolActivity: denied resolution
failGateToolActivity->>useChat: append authorization failure tool card
useChat->>locallyResolvedGatesRef: record local resolution
useChatEvents->>locallyResolvedGatesRef: check stale gate before restoring
useChatEvents->>useChatEvents: settleTerminalRunAfterResolvedPrompt()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
|
github-actions
Bot
added
size: XL
![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 implements support for user-denied approval gates in the agent loop execution, surfacing a model-visible Authorization failure rather than cancelling the run. It also introduces activity ordering (activity_order) to ensure stable rendering of tool activities in the WebUI, and adds frontend state management to track and display synthetic gate activities and denied gates. The review feedback highlights several opportunities to improve robustness in the frontend JavaScript code by defensively using optional chaining to prevent potential TypeError exceptions when handling activity lists, message indices, and history refreshes.
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.
…ny-gate-activity # Conflicts: # crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.js # crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js # crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs # crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useHistory.test.mjs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/tests.rs (1)
5540-5554:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the stale doc comment to match approval-deny semantics.
Line 5540 describes an auth-deny (
pending_auth_resume) path, but the test at Line 5556 exercises approval-deny viapending_approval_resume. Please update the comment to match the test behavior to avoid misleading future maintenance.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_agent_loop/src/executor/tests.rs` around lines 5540 - 5554, The doc comment for the test starting at line 5540 describes an auth-deny scenario with `pending_auth_resume` and `disposition = Some(Denied)`, but the actual test exercises an approval-deny path using `pending_approval_resume`. Update the comment to accurately reflect the approval-deny semantics that the test actually validates, replacing references to auth-deny terminology with approval-deny terminology to keep the documentation consistent with the test implementation.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 125-168: The pending_approval_resume state is being cleared at
line 128 before handle_capability_error() can snapshot it for the retry guard,
causing the handler to lose context that this was a denied resume. Additionally,
the partition at lines 129-131 matches only by capability_id, potentially
failing multiple sibling calls with the same capability when only one should be
denied. Move the state.pending_approval_resume = None assignment to after the
for loop completes, so handle_capability_error() can still access it during
processing. Update the partition predicate to match both the denied_cap_id and
the denied_activity_id (parsed from the resume token) to ensure only the
specific denied invocation is failed, not all calls with the same capability_id.
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 604-609: The approval-deny contract test assertion at line 3812
still expects RebornResolveGateResponse::Cancelled(_), but the code now returns
a queued ResumeTurnResponse indicating a resumed turn. Update the assertion at
line 3812 and the surrounding test code in the range 3785-3814 to expect and
validate the correct response type that matches the new approval-deny resume
path behavior instead of the cancelled response, ensuring the test actually
drives the real caller behavior and validates the UI/model-visible resume
outcome.
In `@crates/ironclaw_reborn_composition/src/projection/runtime_replay.rs`:
- Around line 125-140: The issue is that capability_activities is sorted in
ascending order (oldest first), but the current code uses
.take(activity_payloads) to select the first N items, which discards the newest
activities and violates the invariant that terminal/completed activities must
remain visible. To fix this in the runtime_payload_candidates() function,
reverse the selection to keep the tail (newest items) instead of the head—either
by sorting in descending order before truncating, or by using the
select_nth_unstable_by pattern from
compare_capability_activities_for_output_window() as a reference model. The same
fix applies to append_activity_replay_candidates() which has the analogous issue
with selecting from an unsorted transitions iterator without prioritizing
terminal activities.
In `@crates/ironclaw_reborn/src/planned_driver.rs`:
- Around line 169-173: The code silently ignores the
`approval_resume_disposition` when `pending_approval_resume` is not present,
which violates the fail-closed requirement for approvals. When
`request.approval_resume_disposition` is Some but
`initial.pending_approval_resume` is None, this mismatch should be treated as an
error condition rather than being silently ignored. Add error handling that
returns an error or halts execution when this condition is detected, ensuring
that an explicit user denial cannot be lost due to a missing or invalid
checkpoint.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/tests.rs`:
- Around line 5540-5554: The doc comment for the test starting at line 5540
describes an auth-deny scenario with `pending_auth_resume` and `disposition =
Some(Denied)`, but the actual test exercises an approval-deny path using
`pending_approval_resume`. Update the comment to accurately reflect the
approval-deny semantics that the test actually validates, replacing references
to auth-deny terminology with approval-deny terminology to keep the
documentation consistent with the test implementation.
🪄 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: ae8d8f49-4dc2-4c94-8d85-163518cb136c
📒 Files selected for processing (61)
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_event_projections/src/runtime_projection.rscrates/ironclaw_product_adapters/src/outbound.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/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_driver_host/port_adapters.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.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/live_progress.rscrates/ironclaw_reborn_composition/src/projection/runtime_replay.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/projection/tests/runtime_stream.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/ids.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/run_profile/host.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.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/activity-run.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/activity-run.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useHistory.test.mjs
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)
crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js (1)
622-631: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueInconsistent return type:
falsevsnullfor missing state.Line 623 returns
falsewhen!runId, but lines 625 and 630 returnnull. Callers check truthiness so it works, but the mixed types obscure intent and could confuse future maintenance.function locallyResolvedStateForRun(locallyResolvedGatesRef, runId) { - if (!runId) return false; + if (!runId) return null; const resolved = locallyResolvedGatesRef?.current;🤖 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_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js` around lines 622 - 631, The function locallyResolvedStateForRun has inconsistent return types across different code paths: it returns false when !runId is true (line 623), but returns null when resolved state is missing or no matching entry is found (lines 625 and 630). To fix this, change the return statement on line 623 to return null instead of false, ensuring all code paths that fail to find a valid state return the same consistent type.
🤖 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_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js`:
- Around line 335-348: The call to settleSuccessfulRunAfterResolvedPrompt at
useChatEvents.js lines 335-348 is passing parameters onRunCompleted and
completedRunsRef that don't exist in scope, causing a naming mismatch. In
useChatEvents.js lines 335-348, change the parameters passed to
settleSuccessfulRunAfterResolvedPrompt from onRunCompleted to onRunSettled and
from completedRunsRef to settledRunsRef. In useChatEvents.js lines 546-570,
rename the settleSuccessfulRunAfterResolvedPrompt helper function parameters and
all internal references from onRunCompleted to onRunSettled and from
completedRunsRef to settledRunsRef to match. In useChatEvents.test.mjs at line
673, change the test assertion from harness.completedRuns to harness.settledRuns
and ensure it validates the expected shape with runId and success fields.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs`:
- Around line 622-674: The test assertion at the end of the test "useChatEvents:
parent completion after resumed auth cancel clears typing and refetches"
references harness.completedRuns which does not exist on the harness object.
Replace harness.completedRuns with harness.settledRuns in the final
assert.deepEqual call to match the actual property exposed by the test harness,
ensuring the test correctly validates the expected behavior of settled runs
after a parent completion event.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js`:
- Around line 622-631: The function locallyResolvedStateForRun has inconsistent
return types across different code paths: it returns false when !runId is true
(line 623), but returns null when resolved state is missing or no matching entry
is found (lines 625 and 630). To fix this, change the return statement on line
623 to return null instead of false, ensuring all code paths that fail to find a
valid state return the same consistent type.
🪄 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: a5e8bbaf-1b0a-4e47-a63d-f9ec87966e58
📒 Files selected for processing (4)
crates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js (1)
331-348:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSettle every terminal parent run after a resumed prompt, not only success.
When the locally resolved prompt run differs from the terminal parent
runId, this branch marks the parent status stale. Today only successful parent terminals are handled;failed,cancelled, orrecovery_requiredhitcontinue, leaving typing active and skipping timeline refetch/error UI. This breaks the resumed auth-cancel/deny completion path. Add a caller-level regression for a failed/recovery parent terminal after a resumed local gate.🐛 Proposed direction
- if ( - SUCCESS_RUN_STATUSES.has(status) && - activeResolvedPromptState?.outcome === "resumed" - ) { - settleSuccessfulRunAfterResolvedPrompt({ + if (activeResolvedPromptState?.outcome === "resumed") { + settleTerminalRunAfterResolvedPrompt({ runId, activePromptRunId: activeRunRef?.current?.runId, + success: SUCCESS_RUN_STATUSES.has(status), + status, + failureCategory, + failureSummary, + setMessages, setIsProcessing, setPendingGate, setActiveRun, onRunSettled, settledRunsRef, @@ -function settleSuccessfulRunAfterResolvedPrompt({ +function settleTerminalRunAfterResolvedPrompt({ runId, activePromptRunId, + success, + status, + failureCategory, + failureSummary, + setMessages, setIsProcessing, setPendingGate, setActiveRun, @@ - settleRun(settledRunsRef, onRunSettled, runId, true); + settleRun(settledRunsRef, onRunSettled, runId, success); + if (status === "failed" || status === "recovery_required") { + appendRunFailureMessage(setMessages, { + runId, + status, + failureCategory, + failureSummary, + }); + } }Also applies to: 546-567
🤖 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_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js` around lines 331 - 348, The current condition only handles successful parent run terminals using SUCCESS_RUN_STATUSES when the actively resolved prompt state outcome is resumed, but it should handle all terminal parent statuses (including failed, cancelled, and recovery_required) to properly settle the parent run and update the UI. Replace the SUCCESS_RUN_STATUSES check with a broader condition that captures all terminal status types, ensuring that settleSuccessfulRunAfterResolvedPrompt (or equivalent settlement logic) is called for any terminal parent run after a resumed prompt, not just successful ones, to prevent typing from remaining active and ensure timeline refetch and error UI are properly displayed.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_reborn_composition/src/projection/display_preview.rs`:
- Around line 215-228: The has_pending_input_for_activity() function currently
returns true if any pending input exists for the entire run, rather than
checking if the pending input specifically belongs to the given activity. You
need to scope the pending check to the specific activity by validating that
pending input refs belong to this activity's InvocationId or activity identifier
before returning true. Instead of just checking if refs exist for the run_id,
verify that the pending refs are associated with this particular activity (e.g.,
by matching activity identifiers or invocation IDs). The same fix also needs to
be applied at the other location around lines 274-286 which has the same scoping
issue.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.js`:
- Around line 331-348: The current condition only handles successful parent run
terminals using SUCCESS_RUN_STATUSES when the actively resolved prompt state
outcome is resumed, but it should handle all terminal parent statuses (including
failed, cancelled, and recovery_required) to properly settle the parent run and
update the UI. Replace the SUCCESS_RUN_STATUSES check with a broader condition
that captures all terminal status types, ensuring that
settleSuccessfulRunAfterResolvedPrompt (or equivalent settlement logic) is
called for any terminal parent run after a resumed prompt, not just successful
ones, to prevent typing from remaining active and ensure timeline refetch and
error UI are properly displayed.
🪄 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: 9dab8ad8-44e8-4120-b316-f4f60c99144c
📒 Files selected for processing (6)
crates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/runtime_replay.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs
a7b149a to
0bd9fe7
Compare
github-actions
Bot
added
scope: agent
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
✅ Code review approval for head acc571ac39baf5fb5cd9e87a8e93b06c99d9e0e7.
I reviewed the approval-deny activity ordering changes across the runtime projection, Reborn composition projection, WebUI event handling, and targeted tests. The gate/activity flow now keeps denied capability activity visible and preserves durable activity ordering through replay.
I found one CI compile issue after the new CapabilityActivityProjection::first_cursor field was added: crates/ironclaw_event_streams/tests/event_stream_manager_contract/support/builders.rs still used the old fixture shape. I pushed acc571ac3 to add the fixture field.
Local verification:
node --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjsnode --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.test.mjsnode --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.test.mjscargo test -p ironclaw_event_streams --test event_stream_manager_contractcargo check -p ironclaw_event_streams --testscargo test -p ironclaw_reborn_composition projection::tests::runtime_stream -- --nocapture
The replacement CI run is queued, so I am not posting final human-review guidance yet.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
66-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resolveGateOutcomemisclassifies fallback responses as cancelled.Line 70 uses
response?.already_terminal !== undefined, which is true for bothtrueandfalse. That can force"cancelled"and skipsetActiveRun(...)at Line 476 even when the run continues, leaving live gate/run state inconsistent.Suggested fix
function resolveGateOutcome(response) { if (response?.outcome) return response.outcome; const status = String(response?.status || "").toLowerCase(); if (status === "queued" || status === "running") return "resumed"; - if (status === "cancelled" || response?.already_terminal !== undefined) { + if (status === "cancelled" || response?.already_terminal === true) { return "cancelled"; } return null; }Also applies to: 468-482
🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 66 - 74, The resolveGateOutcome function incorrectly checks if response?.already_terminal is defined using !== undefined, which evaluates to true for both true and false values, causing false positives where non-terminal responses are classified as "cancelled". Fix this by changing the condition to explicitly check if already_terminal is true (either using === true or relying on truthiness), so that only responses with already_terminal actually set to true trigger the "cancelled" outcome and prevent the run state inconsistency issue.
♻️ Duplicate comments (1)
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
141-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve and match the specific parked denied resume.
The call sites clear
pending_*_resumebeforehandle_capability_error()snapshots resume-origin state at Lines 774-783, so the retry guard can treat a denied resume as a fresh call. The helper also partitions bycapability_idonly, so one denied gate can fail every same-capability sibling while emittingCapabilityActivityFailedfor only the first viadenied_activity_id.take(). Keep the pending resume available or pass an explicit resume-origin flag, and match the parked call by full resume identity (capability_id,input_ref,surface_version, or activity id where available); if no call matches, fail loud instead of clearing and continuing. This is the same failure mode noted in the previous review, still visible after the helper extraction. As per coding guidelines, Rust code must “Fail closed for auth, approvals, trust…” and the repo invariant says “Fail loud” rather than silently poisoning downstream state.Also applies to: 185-194, 1051-1059
🤖 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 141 - 150, The code is clearing state.pending_auth_resume before calling short_circuit_denied_resume, which prevents proper matching of the specific parked denied resume. Instead of clearing pending_auth_resume upfront, preserve it and pass it explicitly to short_circuit_denied_resume (or keep it available). Modify the logic to match the denied resume by its full identity using capability_id, input_ref, surface_version, or activity_id rather than partitioning by capability_id only, to prevent one denied gate from affecting sibling capabilities. If no matching parked call is found, fail loudly with an error instead of silently clearing the state and continuing, per the requirement to "Fail closed for auth" and "Fail loud" rather than poisoning downstream state. This applies to all three call sites at lines 141-150, 185-194, and 1051-1059.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_reborn_composition/src/projection/display_preview.rs`:
- Around line 121-123: The early return in the error handling of
self.pending.lock() causes silent failures when the lock is poisoned, making all
subsequent record_input calls no-ops and losing preview input state invisibly.
Instead of returning early when the lock acquisition fails in the `else` clause,
log an error message to make the failure visible to developers, adhering to the
"fail loud" principle and ensuring state-update failures are flagged rather than
silently ignored.
---
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 66-74: The resolveGateOutcome function incorrectly checks if
response?.already_terminal is defined using !== undefined, which evaluates to
true for both true and false values, causing false positives where non-terminal
responses are classified as "cancelled". Fix this by changing the condition to
explicitly check if already_terminal is true (either using === true or relying
on truthiness), so that only responses with already_terminal actually set to
true trigger the "cancelled" outcome and prevent the run state inconsistency
issue.
---
Duplicate comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 141-150: The code is clearing state.pending_auth_resume before
calling short_circuit_denied_resume, which prevents proper matching of the
specific parked denied resume. Instead of clearing pending_auth_resume upfront,
preserve it and pass it explicitly to short_circuit_denied_resume (or keep it
available). Modify the logic to match the denied resume by its full identity
using capability_id, input_ref, surface_version, or activity_id rather than
partitioning by capability_id only, to prevent one denied gate from affecting
sibling capabilities. If no matching parked call is found, fail loudly with an
error instead of silently clearing the state and continuing, per the requirement
to "Fail closed for auth" and "Fail loud" rather than poisoning downstream
state. This applies to all three call sites at lines 141-150, 185-194, and
1051-1059.
🪄 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: 4df1cb71-1884-4568-bb59-ce03893035fa
📒 Files selected for processing (34)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_event_projections/src/lib.rscrates/ironclaw_event_projections/src/runtime_projection.rscrates/ironclaw_event_projections/tests/replay_projection_contract.rscrates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/nearai_chat.rscrates/ironclaw_llm/src/nearai_tool_message_flattening.rscrates/ironclaw_loop_support/tests/compaction_task_contract.rscrates/ironclaw_product_adapters/src/outbound.rscrates/ironclaw_reborn_composition/src/projection.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/runtime_replay.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview_runtime.rscrates/ironclaw_reborn_composition/src/projection/tests/runtime_stream.rscrates/ironclaw_reborn_composition/src/projection/tests/turn_stream.rscrates/ironclaw_reborn_composition/src/projection/turn_events.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_threads/src/capability_display_preview.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_threads/tests/session_thread_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_schema_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/gates.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.test.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.test.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjssrc/agent/dispatcher.rs
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
✅ Code review approval for head 5a29a86994e9dc5126f298a8ae7e9a53992c3ea7.
I addressed the unresolved review thread about silently dropping display-preview input capture on a poisoned lock. record_input now logs and recovers the poisoned pending guard with into_inner() instead of returning early. I also updated the display-preview runtime tests to match this PR's current activity-before-preview replay contract.
Verified locally:
cargo test -p ironclaw_reborn_composition projection::tests::display_preview -- --nocapturecargo test -p ironclaw_reborn_composition projection::tests::runtime_stream -- --nocapturegit diff --check
CI has restarted for the new head, so I am still holding final human-review guidance until the check rollup is green.
|
✅ Code review approval for head I reviewed the new commit
I did not find actionable issues. The update makes completed-preview cursor holding conditional on an actual pending input for the run, recovers display-preview mutex poisoning instead of silently dropping previews, interleaves activity/preview replay candidates to preserve cursor semantics, and fixes I am not formally approving the PR. CI is still pending on this head, so I am not posting final human-review guidance yet. |
think-in-universe
changed the title
|
@claude review |
Code Review: PR #4978Reviewed against root CLAUDE.md, ironclaw_agent_loop/CLAUDE.md, and ironclaw_turns/run_profile/CLAUDE.md. Found 2 Issues:
No Issues Found:
|
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 (1)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
466-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard denied tool-failure marking by normalized gate outcome.
failGateToolActivity(...)runs for every"denied"click, even when the server reports a stale/terminal gate (already_terminal: true→"cancelled"). That can reintroduce the exact state-drift this PR is fixing (terminal activity overwritten by synthetic auth failure).Suggested fix
- if (resolution === "denied") { - failGateToolActivity(setMessages, pendingGate, toolActivityStateRef); - } const outcome = resolveGateOutcome(response); + if (resolution === "denied" && outcome === "resumed") { + failGateToolActivity(setMessages, pendingGate, toolActivityStateRef); + }Also applies to: 470-477
🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 466 - 469, The failGateToolActivity function is being called based on the raw resolution value "denied" before the gate outcome is normalized through resolveGateOutcome. This can cause the function to incorrectly mark an activity as failed when the server actually indicates a stale/terminal gate (already_terminal: true, which normalizes to "cancelled"). Move the failGateToolActivity call to occur after the normalized outcome is calculated, and guard it by checking that the normalized outcome from resolveGateOutcome(response) is "denied" rather than checking the raw resolution value.
🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 466-469: The failGateToolActivity function is being called based
on the raw resolution value "denied" before the gate outcome is normalized
through resolveGateOutcome. This can cause the function to incorrectly mark an
activity as failed when the server actually indicates a stale/terminal gate
(already_terminal: true, which normalizes to "cancelled"). Move the
failGateToolActivity call to occur after the normalized outcome is calculated,
and guard it by checking that the normalized outcome from
resolveGateOutcome(response) is "denied" rather than checking the raw resolution
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 982d52c6-f52a-471f-9757-c4ba251bf2c0
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/runtime_replay.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
|
Human final-review guidance for current head Current status:
Suggested human review focus:
|
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
❌ Code review request changes for head 0309973c55d54d92e88973f3f34af230dc54349c.
I reviewed the new commit 0309973c (Fix stale gate deny activity marking). I found one Medium issue: the new already_terminal: true path avoids synthesizing a failed activity, but the caller still unconditionally marks the UI as processing after the terminal response.
I am not formally requesting changes via GitHub review state and I am not approving the PR. CI is running again on this head, so no final human-review guidance yet.
hanakannzashi
changed the title
|
✅ Code review approval for head I reviewed the new commit
The change addresses the prior terminal-gate finding: |
…ny-gate-activity # Conflicts: # crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js # crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
|
✅ Code review update for current head I reviewed the new merge commit
I found no new PR-scoped issues. The previous Medium CI is still pending and the |
|
✅ Reviewed new head I focused on the runtime replay candidate ordering and the caller-level CI is still pending on this head, so this is not ready for human final review yet. |
|
✅ Human final-review guidance for current head Current status:
Please focus final human review on:
|
…earai#4978) * fix(webui-v2): ignore stale denied gate projections * fix(reborn): keep denied gate activity visible Emit denied capability activity as a durable failed activity, surface denied gates back into the model loop, and preserve live WebUI activity across gate resolution/history refresh. * fix(reborn): stabilize webui tool activity state * Fix WebUI tool activity ordering * Move tool activity ordering into projections * Fix approval deny resume edge cases * Fix auth-cancel resume and live completion state * Fix resumed prompt terminal settlement * Fix Reborn tool results and activity ordering * Fix denied capability activity replay status * Make pending resume activity ids explicit * Fix WebUI gate outcome and preview cursor handling * Fix stale gate deny activity marking * Fix terminal gate processing state * Fix streaming of activity metadata with pending previews --------- Co-authored-by: think-in-universe <46699230+think-in-universe@users.noreply.github.com>
2 participants
Summary
CapabilityActivityFailedmilestones.activity_idexplicitly in checkpoint state; deny replay now consumes that field, with legacy resume-token fallback only for older checkpoints.activity_order/ first activity cursor, then have WebUI live/history rendering consume that canonical order.CapabilityActivityProjection::first_cursorfield.Tests
node --test crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChatEvents.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/tool-activity-state.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/message-groups.test.mjs crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useHistory.test.mjscargo test -p ironclaw_product_adapters --lib capability_activitycargo test -p ironclaw_webui_v2 --test webui_v2_schema_contractcargo test -p ironclaw_reborn_composition projection::tests::runtime_stream::webui_event_stream_delivers_prior_completed_activity_before_pending_approval_previewcargo test -p ironclaw_product_workflow --test auth_interaction_contract denied_auth_without_flow_record_resumes_parked_auth_run_with_denial_dispositioncargo test -p ironclaw_agent_loop pending_auth_resume -- --nocapturecargo test -p ironclaw_agent_loop denied_approval_resume -- --nocapturecargo test -p ironclaw_agent_loop denied_auth_resume -- --nocapturecargo test -p ironclaw_reborn deny_disposition -- --nocapturecargo clippy -p ironclaw_agent_loop --all-features --all-targets -- -D warningscargo clippy -p ironclaw_reborn --all-features --all-targets -- -D warningscargo clippy -p ironclaw_event_streams --all-features --all-targets -- -D warningscargo fmt --all -- --checkgit diff --checkFixes #4977
Closes #4762
Closes #4764
Closes #4983
Closes #4853