fix(reborn): stale gate projection rows in WebUI stream - #5297
hanakannzashi wants to merge 12 commits into
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
scope: docs
|
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
WalkthroughBlocked gate projection now suppresses stale gate rows from persisted run state, and chat handling now scopes processing, sends, and gate UI to the active thread. ChangesBlocked gate read-model hardening
Chat thread-scoped state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ironclaw-pr-5297 environment in ironclaw-ci-preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_reborn_composition/src/projection/turn_events.rs (1)
505-513: 🩺 Stability & Availability | 🟡 MinorCorrect the cross-layer guarantee comment on
BlockedExternalTool.Lines 505–507 claim "External-tool gates are not user-clickable prompts; the OpenAI Responses surface reads them via its own projection path." This overstates exclusivity. Verification shows
gate_projection_itememits aProductProjectionItem::GateforBlockedExternalToolvia the standard projection (mapped toProductGateKind::Genericwith static body text). The comment correctly notes there is no prompt payload, but it misleadingly implies no generic gate row is emitted and that the Responses surface alone owns the projection.Per coding guidelines: "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent."
Fix options (pick one):
- Soften the comment to reflect reality: "External-tool gates have no user-clickable prompt payload; the OpenAI Responses surface may handle them via its own path, but the generic projection still emits a gate row."
- Enforce the intended exclusivity by adding an early return in
gate_projection_itemforExternalTool(if the design intent is truly to suppress the generic row).Recommend
#1unless the generic row is confirmed to cause UI drift.Additional consistency check: Ensure the WebUI correctly distinguishes
Genericgates from actionable prompt gates when rendering, or add a filter if external-tool gates should be hidden from the standard gate list.🤖 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_composition/src/projection/turn_events.rs` around lines 505 - 513, The comment around `TurnStatus::BlockedExternalTool` overstates what the projection does and should be softened. Update the note in `turn_events::gate_projection_item`/the surrounding `BlockedExternalTool` match arm to say there is no user-clickable prompt payload, but the generic projection still emits a `ProductProjectionItem::Gate` (with `ProductGateKind::Generic`) and the Responses surface may handle it via its own path. If the intent is true exclusivity, enforce that in `gate_projection_item` with an early return and add matching tests; otherwise keep the code and adjust the comment only.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_reborn_composition/src/projection/turn_events.rs`:
- Around line 505-513: The comment around `TurnStatus::BlockedExternalTool`
overstates what the projection does and should be softened. Update the note in
`turn_events::gate_projection_item`/the surrounding `BlockedExternalTool` match
arm to say there is no user-clickable prompt payload, but the generic projection
still emits a `ProductProjectionItem::Gate` (with `ProductGateKind::Generic`)
and the Responses surface may handle it via its own path. If the intent is true
exclusivity, enforce that in `gate_projection_item` with an early return and add
matching tests; otherwise keep the code and adjust the comment only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c13a734-542d-4c89-96f1-4bfc47e7858a
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/projection/turn_events.rs
think-in-universe
changed the title
|
@claude review |
|
Code Review Summary - PR 5297 Found 9 issues requiring attention before merge. CRITICAL
HIGH
MEDIUM
Summary Most urgent: (1) Cursor comparison logic allows stale gates, (2) Error handling on approval_gate_prompt fails at runtime, (3) Stringly-typed status field violates type-driven design rule. These issues block merge. Please address and re-request review. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_composition/src/projection/tests/turn_stream.rs (1)
697-704: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not codify an allow-always prompt without context.
This test wires no approval store, so
approval_contextis absent. The fallback prompt can exist, butallow_alwaysshould fail closed just like the projection row below.As per coding guidelines, “Fail closed for auth, approvals, trust…” applies here.
Proposed test adjustment
&& prompt.gate_ref == gate_ref.as_str() && prompt.approval_context.is_none() - && prompt.allow_always + && !prompt.allow_always🤖 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_composition/src/projection/tests/turn_stream.rs` around lines 697 - 704, The GatePrompt assertion in turn_stream test currently expects allow_always to be true even when approval_context is absent, which should fail closed. Update the match on ProductOutboundPayload::GatePrompt so the fallback prompt is still accepted, but allow_always is asserted false when no approval store/context is present, matching the projection behavior and the auth/approval fail-closed rule.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/projection/turn_events.rs (1)
518-535: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed on
allow_alwayswhen approval context is missing.Line 532 enables the “always allow” affordance from the gate-ref shape alone. If
approval_prompt_lookup()returns noapproval_context, the transientGatePromptcan advertiseallow_always: truewhile the projection row correctly disables it at Line 805. That is approval UI drift at a user boundary.As per coding guidelines, “Fail closed for auth, approvals, trust…” applies here.
Proposed fix
let lookup = approval_prompt_lookup(approval_requests, gate_ref, owner_user_id, &event.scope).await; + let allow_always = is_approval_gate_ref(gate_ref.as_str()) && lookup.context.is_some(); gate_prompt_with_context( event, gate_ref_string, "Approval required", - is_approval_gate_ref(gate_ref.as_str()), + allow_always, lookup.context, lookup.invocation_id, )🤖 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_composition/src/projection/turn_events.rs` around lines 518 - 535, The approval prompt currently enables the “always allow” affordance based only on the gate-ref shape, which can drift from the persisted projection when approval context is missing. Update approval_gate_prompt and the gate_prompt_with_context call site so allow_always is only true when approval_prompt_lookup returns a real approval_context, and otherwise fail closed by forcing it off; use the approval_prompt_lookup result and the is_approval_gate_ref / gate_prompt_with_context path to locate the change.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_reborn_composition/src/projection/tests/turn_stream.rs`:
- Around line 697-704: The GatePrompt assertion in turn_stream test currently
expects allow_always to be true even when approval_context is absent, which
should fail closed. Update the match on ProductOutboundPayload::GatePrompt so
the fallback prompt is still accepted, but allow_always is asserted false when
no approval store/context is present, matching the projection behavior and the
auth/approval fail-closed rule.
In `@crates/ironclaw_reborn_composition/src/projection/turn_events.rs`:
- Around line 518-535: The approval prompt currently enables the “always allow”
affordance based only on the gate-ref shape, which can drift from the persisted
projection when approval context is missing. Update approval_gate_prompt and the
gate_prompt_with_context call site so allow_always is only true when
approval_prompt_lookup returns a real approval_context, and otherwise fail
closed by forcing it off; use the approval_prompt_lookup result and the
is_approval_gate_ref / gate_prompt_with_context path to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7fc00d6-9377-4b18-b287-35cea7e3eaef
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/projection/tests/turn_stream.rscrates/ironclaw_reborn_composition/src/projection/turn_events.rs
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useChat.js`:
- Around line 404-408: The send path still uses the mounted thread state for its
remaining guards, so target-thread sends can be blocked by the currently open
thread. Update the guard logic in send to scope every gating check to the
resolved target thread, using targetRunThreadId alongside threadId and
activeRunBlocksSend. Make sure the early return/null behavior only applies when
the target thread itself is gated or processing, not just the active UI thread.
🪄 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: 3c5f4dac-dfc1-43a6-bd5e-0954f835601f
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (7)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useSSE.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.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.mjs
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)
400-414: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope
submitBusyRefto the same thread contract.Line 412 still uses a global submit latch. After this change, a successful
send(..., { threadId: otherThread })can pass Lines 400-414, setsubmitBusyRef.current = true, and never clear it because this hook only receivesonRunSettledfor the mounted thread. The next send from the current chat will returnnullforever.Suggested fix
- if ( - submitBusyRef.current || + if ( + (sendTargetsCurrentThread && submitBusyRef.current) || (sendTargetsCurrentThread && isProcessingRef.current) || activeRunBlocksSend ) { return null; } … - submitBusyRef.current = true; + if (shouldRenderInCurrentThread) { + submitBusyRef.current = true; + } … - } else if (!response?.run_id) { + } else if (!response?.run_id || !shouldRenderInCurrentThread) { submitBusyRef.current = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 400 - 414, The send flow in useChat.js still treats submitBusyRef as a global latch, which can get stuck when send(..., { threadId: otherThread }) succeeds and the mounted thread never clears it. Update the send/submitBusyRef logic so it is scoped to the same target thread contract as sendTargetsCurrentThread and activeRunBlocksSend, and only blocks or resets submits for the matching thread handled by this hook’s onRunSettled path.
🤖 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 400-414: The send flow in useChat.js still treats submitBusyRef as
a global latch, which can get stuck when send(..., { threadId: otherThread })
succeeds and the mounted thread never clears it. Update the send/submitBusyRef
logic so it is scoped to the same target thread contract as
sendTargetsCurrentThread and activeRunBlocksSend, and only blocks or resets
submits for the matching thread handled by this hook’s onRunSettled path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33bee3da-fa83-475d-b9ad-439d6fb7e127
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
github-actions
Bot
added
size: L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useChat.js`:
- Around line 417-430: The send guard in useChat still treats any current-thread
destination as blocked when isProcessingRef is true, which causes sends to be
dropped even though the composer is enabled for the activeRun/thread mismatch
case. Update the send eligibility logic around activeRunBlocksSend and
processingBlocksSend so it only falls back to threadId before a run has a thread
identity, and once activeRunRef.current has a threadId, compare against the
destination thread identity instead. Add or extend a useChat-level test to cover
the half-state where isProcessing is true but activeRun.threadId differs from
activeThreadId, verifying send is not blocked for the current composer state.
🪄 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: e82043ba-ace2-4076-b2c9-35103a095b4d
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/hooks/useChat.js`:
- Around line 417-430: The send guard in useChat still treats any current-thread
destination as blocked when isProcessingRef is true, which causes sends to be
dropped even though the composer is enabled for the activeRun/thread mismatch
case. Update the send eligibility logic around activeRunBlocksSend and
processingBlocksSend so it only falls back to threadId before a run has a thread
identity, and once activeRunRef.current has a threadId, compare against the
destination thread identity instead. Add or extend a useChat-level test to cover
the half-state where isProcessing is true but activeRun.threadId differs from
activeThreadId, verifying send is not blocked for the current composer state.
🪄 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: e82043ba-ace2-4076-b2c9-35103a095b4d
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
🛑 Comments failed to post (1)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
417-430: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
isProcessingstill blocks a current-thread send after the UI says it is sendable.
chat.jsnow explicitly keeps the composer enabled for{ isProcessing: true, activeRun.threadId !== activeThreadId }, but this guard still blocks any send whose destination resolves tothreadId. In that half-state, clicking send on the current thread still returnsnull, so the submit is silently dropped even though the composer is enabled. Only the pre-run_idwindow should fall back tothreadId; onceactiveRun.threadIdexists, this check needs to follow the destination run’s thread identity. The matching hook-level regression is missing too — the newchat.test.mjscase only asserts props.Suggested fix
- const processingBlocksSend = - isProcessingRef.current && - Boolean(sendTargetThreadId) && - sendTargetThreadId === threadId; + const processingBlocksSend = + isProcessingRef.current && + Boolean(sendTargetThreadId) && + (!activeRunForSend?.threadId + ? sendTargetThreadId === threadId + : activeRunForSend.threadId === sendTargetThreadId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const sendTargetThreadId = targetThreadId || threadId; const activeRunForSend = activeRunRef.current; const activeRunBlocksSend = Boolean(activeRunForSend) && Boolean(sendTargetThreadId) && activeRunForSend.threadId === sendTargetThreadId; const processingBlocksSend = isProcessingRef.current && Boolean(sendTargetThreadId) && (!activeRunForSend?.threadId ? sendTargetThreadId === threadId : activeRunForSend.threadId === sendTargetThreadId); if ( submitBusyRef.current || processingBlocksSend || activeRunBlocksSend🤖 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 417 - 430, The send guard in useChat still treats any current-thread destination as blocked when isProcessingRef is true, which causes sends to be dropped even though the composer is enabled for the activeRun/thread mismatch case. Update the send eligibility logic around activeRunBlocksSend and processingBlocksSend so it only falls back to threadId before a run has a thread identity, and once activeRunRef.current has a threadId, compare against the destination thread identity instead. Add or extend a useChat-level test to cover the half-state where isProcessing is true but activeRun.threadId differs from activeThreadId, verifying send is not blocked for the current composer state.
|
@claude review |
Code Review ResultsFound Issues
No Issues Found
Summary: The PR correctly implements the documented stale-gate suppression rule (CLAUDE.md lines 50-53) with proper cursor/status/gate-identity validation. Five minor issues remain: observability of the timing window between fetch and validation, sourceThreadId propagation guarantee, test consolidation, minor allocations, and documentation clarity on defensive vs. primary checks. |
This branch was successfully deployed
2 participants
Summary
Fixes #5218. Part of #5200.
Testing
Notes