Repository navigation
fix(reborn): classify run-wait states — stop forever-hangs and gate-parked-run kills - #5250
henrypark133 wants to merge 12 commits into
Conversation
Three wait/poll loops hand-rolled partial predicates over TurnStatus on top of is_terminal(), so they mishandled the four "parked" Blocked* states (which never self-advance). Add one canonical, exhaustive classifier — TurnStatus::wait_class() -> RunWaitClass — next to is_terminal(), and route the three sites through it. Verified bugs (each has a regression test proven red on the unpatched code, then green after the fix): 1. ironclaw_reborn_composition openai_compat_serve wait_for_response_completion was an unbounded loop that only returned on a finalized message or a Failed/Cancelled projection. A run parked on a gate emits neither, so it spun until the outer 30s HTTP timeout and returned a permanent retryable 503 with no gate info. Now it detects the run-matched GatePrompt projection and returns Incomplete promptly. 2. ironclaw_reborn_composition runtime wait_for_terminal polled is_terminal() with a 180s bound, then cancel_run(Timeout) — which destroyed a run legitimately parked on an approval/auth/resource gate (and all its descendants). It now classifies via wait_class() and returns a ParkedAwaitingUser run to the caller instead of killing it. wait_for_terminal_or_gate is now a thin alias since the gate-aware behavior is no longer test-only. 3. ironclaw_reborn subagent completion_observer gated observes_state/ observes_event on is_terminal(), so a child subagent that parked on its own gate was skipped and resume_parent never fired — the parent hung in BlockedDependentRun forever. The observer now reacts to a parked-awaiting-user child, synthesizes a Failed child outcome (preserving the gate store's terminal-only invariant), and resumes the parent with a failed result the model can surface. is_terminal() keeps its exact meaning; a test pins its equivalence to the terminal wait classes. Guardrail added to the ironclaw_turns spec. Follow-up (owned by a concurrent change): migrate slack_delivery.rs's hand-rolled status predicate onto wait_class(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finish completion_observer import (TurnTimestamp via crate root), the openai_compat unbounded-wait bound, and the gate-aware wait routing, plus their tests, on top of the RunWaitClass classifier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds ChangesRun wait classification and parked-run handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a canonical run-wait classification system via TurnStatus::wait_class() to resolve multiple bugs where wait and poll loops would either hang indefinitely or prematurely cancel runs parked on user-resolvable gates. These changes are applied to the subagent completion observer, the OpenAI-compatible response waiter, and the runtime's terminal waiter. Feedback on the implementation identifies a critical bug in run_parked_on_gate_in_events where a run that starts and immediately parks within the same poll drain window would have its parked state incorrectly marked as superseded. A chronological check using event positions is suggested to ensure the latest state is correctly identified.
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.
![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: 84be82c3a4
ℹ️ 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: 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_reborn_composition/src/runtime.rs (1)
1522-1535: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring:
send_user_messageno longer always returns a terminal state.
wait_for_terminalnow returnsRunWaitClass::ParkedAwaitingUserruns (the test pinsreply.status == BlockedApproval), so this public method can hand back a non-terminalBlocked*reply. The doc still says it "wait[s] for the run to reach a terminal state," which now misleads callers readingreply.status/failure_category.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."📝 Suggested doc update
- /// Submit a user message into the conversation, wait for the run to - /// reach a terminal state, and return the assistant reply read back - /// from the session thread service. + /// Submit a user message into the conversation, wait until the run + /// reaches a terminal state *or* parks on a user-resolvable gate + /// (approval/auth/resource), and return the assistant reply read back + /// from the session thread service. A gate-parked reply carries the + /// `Blocked*` status (and the run stays live for later resolution); + /// callers must not assume `status` is terminal.🤖 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/runtime.rs` around lines 1522 - 1535, The doc comment for send_user_message in runtime.rs is stale because the method can now return a non-terminal Blocked* reply via wait_for_terminal and RunWaitClass::ParkedAwaitingUser, so update or remove the sentence claiming it always waits for a terminal state. Keep the rest of the public contract aligned with current behavior, especially the notes about reply.status and failure_category, and make sure the comment accurately describes the possible blocked approval outcome.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/openai_compat_serve.rs`:
- Around line 814-828: The gate-check logic in openai_compat_serve’s prompt/park
handling currently treats any matching RunStatus found in the entire drain
window as superseding, which can incorrectly drop the prompt when
ProjectionSnapshot already contains a pre-park running status. Update the
supersession check so only status changes that occur after the prompt are
considered, using the existing run_status_present_in_state and submitted_run_id
logic but narrowing the scan to post-prompt events rather than any(event...).
Keep the fix localized to this gate evaluation path so the prompt always wins
over stale pre-park status.
In `@crates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rs`:
- Around line 204-228: The new gate-parking regression test only covers the
prompt envelope path and misses the case where a running status arrives in the
same drain window before the gate prompt. Update the tests around
OpenAiResponsesThreadProjectionReader::wait_for_response_completion to use a
StaticProjectionStream containing both run_status_envelope(..., "running") and
gate_prompt_envelope(run_id), so the order-insensitive superseded behavior is
exercised. Keep the assertion that the projection surfaces as
OpenAiResponseStatus::Incomplete and that the wait returns promptly, which will
lock in the fix for the run_parked_on_gate_in_events path.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1522-1535: The doc comment for send_user_message in runtime.rs is
stale because the method can now return a non-terminal Blocked* reply via
wait_for_terminal and RunWaitClass::ParkedAwaitingUser, so update or remove the
sentence claiming it always waits for a terminal state. Keep the rest of the
public contract aligned with current behavior, especially the notes about
reply.status and failure_category, and make sure the comment accurately
describes the possible blocked approval outcome.
🪄 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: 9c66020b-17fe-4450-8816-174fcc484f5b
📒 Files selected for processing (9)
crates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn_composition/src/openai_compat_serve.rscrates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/tests/runtime_gate_wait.rscrates/ironclaw_turns/CLAUDE.mdcrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/status.rsdocs/plans/2026-06-25-run-wait-class.md
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix run-wait classification so parked states do not hang, mis-timeout, or deadlock waiters across response serving, runtime waiting, and subagent completion.
Stats: 2 findings posted (from 5 raw reviewer findings after manual validation and live-thread dedupe) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Existing unresolved gate-ordering comments on openai_compat_serve.rs are still valid and were not reposted here.
Tests
- Medium Parked child state path is not exercised (
crates/ironclaw_reborn/src/subagent/completion_observer.rs:675-681, confidence 86) — anchor: crates/ironclaw_reborn/src/subagent/completion_observer.rs:675
The new observe_committed_state branch for a child parked on a user gate is only covered through observe_committed_event. A state-only delivery still needs to synthesize the failed child event and resume the parent, but there is no regression test for that path, so a future divergence between the event and state observers could slip through. - Medium Stale gate-prompt suppression is untested (
crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:795-827, confidence 79) — anchor: crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:795
The new run_parked_on_gate_in_events helper is supposed to ignore an earlier GatePrompt or AuthPrompt when the same drain window also contains a later non-parked RunStatus. There is no test for that superseded-prompt case, so a future edit could wrongly return Incomplete for a run that has already resumed.
|
🚅 Deployed to the ironclaw-pr-5250 environment in ironclaw-ci-preview
|
run_parked_on_gate_in_events used an order-insensitive any() to detect a superseding RunStatus. When the first wait poll (after_cursor: None) drains both an earlier `running` ProjectionUpdate and the later GatePrompt in one batch, the stale running status was treated as resumption, the prompt was dropped, and the poller spun until the outer HTTP timeout instead of returning Incomplete. Use rposition to compare positions: a RunStatus only supersedes a prompt when it appears AFTER the latest matching prompt. Add an ordering regression test ([running, gate_prompt] in one window) that times out (RED) on the old logic and returns Incomplete with the fix. Addresses PR #5250 review (gemini/codex/coderabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 804c8bca15
ℹ️ 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".
Two regression tests requested in PR #5250 review: - openai_responses_wait_ignores_stale_gate_prompt_after_run_resumes: the supersede branch of run_parked_on_gate_in_events — a GatePrompt followed by a later running RunStatus in one drain window must keep the caller polling (asserts a short timeout fires), never surface a spurious Incomplete for a run that already resumed past the gate. - blocking_child_parked_on_gate_resumes_parent_with_failure_from_state: state-path parity for the parked-on-user-gate child handling. The event path was already covered; this drives observe_committed_state with a BlockedApproval child and asserts the same outcome (parent resumed, failed-child result written, child goal cleaned up) so the two observer delivery paths cannot silently diverge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add an exhaustive run-wait classifier and update waiters so parked runs no longer hang or get killed.
Stats: 2 findings (from 3 raw, 2 after validation/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Existing unresolved review threads already cover the parked-child terminalization issue on completion_observer.rs:785, so I did not duplicate that here. The performance reviewer raised a possible polling scan concern, but I dropped it after checking the filesystem-backed thread service uses the assistant-run lookup index before falling back to a scan.
tests
-
Medium Blocked-dependent runs are not covered by the runtime wait test (
crates/ironclaw_reborn_composition/src/runtime.rs:1886-1912, confidence 72) — anchor:crates/ironclaw_reborn_composition/src/runtime.rs:1907
The runtime integration tests cover a genuinely running timeout and a user-resolvable approval gate, but not theParkedAwaitingRunbranch that should stay on the timeout/cancel path. -
Medium Auth-prompt supersede ordering is untested (
crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:803-833, confidence 64) — anchor:crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:811
The ordering regression test coversGatePrompt, while the newly sharedAuthPromptbranch could still regress independently.
| RunWaitClass::TerminalSuccess | ||
| | RunWaitClass::TerminalFailure | ||
| | RunWaitClass::ParkedAwaitingUser => return Ok(state), | ||
| RunWaitClass::Running | RunWaitClass::ParkedAwaitingRun => {} |
There was a problem hiding this comment.
Medium — Blocked-dependent runs are not covered by the runtime wait test.
The new wait_class() branch for RunWaitClass::ParkedAwaitingRun is only covered by the classifier unit test. The runtime integration test proves Running still times out and BlockedApproval returns early, but it does not exercise a BlockedDependentRun through send_user_message/wait_for_terminal. If that status were accidentally treated as user-parked or terminal at this caller, this PR would not catch it.
Fix: Add a runtime_gate_wait integration case that drives a BlockedDependentRun through the runtime waiter and asserts it stays on the timeout/cancel path rather than surfacing as a user gate.
There was a problem hiding this comment.
Investigated — this one can't be driven through the integration harness in the current tree, so I did not fabricate a test. The only production path that produces BlockedDependentRun is SubagentSpawnCapabilityPort (a blocking spawn_subagent), and spawn_subagent is disabled system-wide via the capability deny filter (every test in tests/reborn_subagent_spawn_e2e.rs is #[ignore = "TEMP(disable-spawn-subagents)"]). So send_user_message/wait_for_terminal cannot reach BlockedDependentRun end-to-end. The classification itself IS locked: TurnStatus::wait_class() is an exhaustive match (a new variant fails the build) with a unit test, and parked_on_user_gate_classification_covers_all_three_user_gates asserts BlockedDependentRun is ParkedAwaitingRun (not ParkedAwaitingUser). Happy to add the runtime-caller integration case as part of re-enabling spawn_subagent — flagging rather than forcing a fake test.
…er-call API Post-merge with main (#5145 capability activity lifecycle): three test constructors needed updating against struct/API changes main introduced: - TurnRunState gained `blocked_activity_id` — added `: None` to the new parked-child state-path test literal. - AuthPromptView gained `invocation_id` — added `: None` to the auth-prompt envelope test helper. - register_provider_tool_call now takes RegisterProviderToolCallRequest — wrapped the runtime_gate_wait integration test's ProviderToolCall via ::new(). Pure test-side adaptation to merged upstream contracts; no production behavior change. CI's all-features build (PR merged with main) caught these; local default-feature build did not surface them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix run-wait state classification so parked runs are handled correctly and wait paths do not hang forever.
Stats: 2 findings (from 6 raw, 2 after validation/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Bugs
- Medium Retrieve path drops the parked-run classification (
crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:713-730, confidence 84) — anchor:crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:520
read_response()callsread_projected_response_status(), but then keeps only.statusand discardsparked_on_gate. A response parked on approval/auth/resource still reportsin_progresson retrieve while the wait path reportsincomplete, so polling clients can keep treating a user-action-required run as normally in flight.
Tests
- Medium Resource-gated runs are not exercised end to end (
crates/ironclaw_reborn_composition/src/runtime.rs:1942-1948, confidence 79) — anchor:crates/ironclaw_reborn_composition/src/runtime.rs:1942
The runtime wait classifier now treatsBlockedResourceas user-resolvable, but the caller-level regression coverage only drives approval/auth parked states. A resource-parked run could regress back to timeout/cancel behavior without a caller-level test failing.
Notes: I dropped the stale/duplicate AuthPrompt ordering and blocked-child lifecycle items because live unresolved threads already cover them, and I dropped the resource-exhaustion candidate after verifying parked runs still keep the turn coordinator active-run lock.
| /// states never self-advance, so a waiter that only checks `is_terminal()` | ||
| /// would poll a gate-parked run until `poll_settings.max_total` and then | ||
| /// **kill** it with a `Timeout` cancel — destroying a run that was | ||
| /// correctly waiting on the user. A [`RunWaitClass::ParkedAwaitingUser`] |
There was a problem hiding this comment.
Medium — Resource-gated runs are not exercised end to end.
The new runtime wait logic treats BlockedResource as a user-resolvable parked state, but the regression coverage only drives approval/auth parked states. A resource-parked run could regress back to timeout/cancel behavior without a caller-level test failing.
Fix: Add tests::runtime_gate_wait::gate_parked_run_is_surfaced_not_killed_on_resource_gate covering a run parked on BlockedResource returning BlockedResource promptly.
There was a problem hiding this comment.
Same situation — can't be driven E2E without new production code, so no fabricated test. RuntimeCapabilityOutcome::ResourceBlocked is never produced by production DefaultHostRuntime::invoke_capability; only test mocks emit it, so build_reborn_runtime has no path that parks a real run on BlockedResource. The classification is covered at unit level (wait_class() exhaustive match + parked_on_user_gate_classification_covers_all_three_user_gates asserts BlockedResource -> ParkedAwaitingUser). I'll add the runtime-caller integration case once a production resource-gate path exists; flagging rather than forcing a fake test.
Bug #3's fix synthesized a Failed event to unblock the parent when a child subagent parked on a user gate, but only wrote the subagent gate store — the child's real run stayed Blocked* in the turn-state store. Because Blocked* keeps the same-thread active lock (keeps_active_lock = !is_terminal) and leaves the gate user-resolvable, the child leaked its lock and a later approval could resume an orphaned child whose parent already moved on (PR #5250 review, codex P1 split-brain). Cancel the real child run (SanitizedCancelReason::Policy) after handle_terminal so it reaches terminal Cancelled — releasing the lock and invalidating the gate. Ordering is deliberate: the synthetic Failed runs first and gate-store terminal records are first-write-wins, so the parent keeps the descriptive failure and the cancel's Cancelled terminal is harmlessly ignored there. Cancel is best-effort (parent already resumed) and logs at debug on failure. This is the minimal correct lock/gate release. The real run's terminal still carries only a generic SanitizedCancelReason category, not the gate diagnostic; the proper fix (a coordinator fail-with-reason transition) is tracked as a near-term follow-up in docs/plans/2026-06-25-run-wait-class.md. Low urgency: subagents are currently disabled, so this observer path is latent. Both parked-child regression tests (event + state path) now assert the child run is cancelled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn/src/subagent/completion_observer.rs`:
- Around line 755-763: The follow-up cancel still uses only
state.actor/event.owner_user_id, so ownerless parked-child cases can skip the
real cancel even though handle_terminal() can recover the missing owner. Update
observe_committed_state() and the terminal path around
handle_terminal()/cancel_parked_child_run()/cancel_run() so the recovered owner
is threaded through to the cancel step, or recovered before invoking it.
Preserve the recovered actor from the child/gate state and use that value for
the cleanup call instead of the possibly missing original metadata.
- Around line 522-574: In cancel_parked_child_run, do not swallow every
coordinator.cancel_run failure: only ignore expected convergence races like
Conflict or InvalidTransition, and propagate real backend/state errors so the
observer can retry instead of silently succeeding. Update the cancel_run error
handling in completion_observer.rs to branch on the error type around the
coordinator.cancel_run call, using the existing cancel_parked_child_run path and
its callers so unexpected failures bubble up through the observer flow. Keep the
existing debug logging for ignored races, but ensure non-ignorable errors return
with context rather than allowing the observer to complete successfully.
🪄 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: 9a045b0d-f0b8-46e1-85a7-98bc92a72dab
📒 Files selected for processing (2)
crates/ironclaw_reborn/src/subagent/completion_observer.rsdocs/plans/2026-06-25-run-wait-class.md
…paths PR #5250 review (henrypark133) found the Responses retrieve path dropped the parked-run classification, and the same bug class exists on the Chat Completions waiter: 1. read_response (GET retrieve) read read_projected_response_status() but kept only .status, discarding parked_on_gate. A run parked on a gate emits no terminal RunStatus, so retrieve reported in_progress forever while the wait path reported incomplete — a polling client never learned the run needs the user. Fixed: retrieve now maps parked_on_gate -> Incomplete, matching the wait path. 2. read_chat_completion_projection was an unbounded poll for a finalized assistant message; a run parked on a gate never finalizes, so it spun until the outer HTTP timeout (a misleading retryable 503 — the original Bug #1 symptom, still live on the chat surface). Fixed: the chat reader now drains the projection and, on a parked run, returns a bounded non-retryable 409 Conflict (Chat Completions has no `incomplete` status) instead of spinning. Regression tests added for both surfaces (retrieve -> Incomplete, chat -> 409/non-retryable). Together with the existing wait-path tests, all three OpenAI-compatible read surfaces now have explicit parked-run coverage so the "one surface drops the parked classification" divergence cannot regress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #5250 review on the parked-child cancel (cancel_parked_child_run): - Propagate real cancel failures (coderabbit): the cancel no longer swallows every error. Only convergence races (Conflict / InvalidTransition, plus an already-terminal run that cancel_run reports as Ok) are ignored — another actor already moved the run, releasing the lock. A real backend/state error now propagates so the observer retries (the path is idempotent) instead of silently leaving the child Blocked* with a held lock and a resolvable gate. - Recover the actor for ownerless deliveries (coderabbit): when the observed event/state carries no actor (the cases handle_terminal recovers internally), the cancel now recovers the authoritative actor from the child run's own state via get_run_state, so the orphaned child is still terminalized rather than skipped. Tests: parked_child_cancel_backend_failure_propagates and parked_child_cancel_recovers_actor_when_event_owner_missing cover the two arms. Also adds openai_responses_wait_ignores_stale_auth_prompt_after_run_resumes, mirroring the gate-supersede test for the AuthPrompt branch (henrypark133). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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_reborn/src/subagent/completion_observer.rs`:
- Around line 536-570: In completion_observer::completion_observer.rs, the
parked-child cancel path is swallowing failure by returning Ok(()) when the
coordinator is missing or the run state has no actor, which leaves the child
lock/gate uncleared without retry; update this branch to fail loudly by
returning a typed TurnError::Unavailable (or equivalent) from the
coordinator/actor-missing cases, and if needed recover the actor from the gate
or run record before giving up so the cancel can be authorized and retried.
🪄 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: 2b1b2156-f3c8-4fb8-af95-2e72a931bdec
📒 Files selected for processing (3)
crates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn_composition/src/openai_compat_serve.rscrates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rs
The parked-child cancel (cancel_parked_child_run) was invoked unconditionally after handle_terminal in observe_committed_state/event. But the observer also fires for a MAIN (non-subagent) run parking on an approval/auth/resource gate — observes_* keys on the parked status alone. handle_terminal no-ops for such a run (no gate record, no parent), but the cancel still ran, so a main run parked on a gate was Cancelled instead of surfaced to the user — a severe regression in the just-added cancel. Caught by the existing integration test gate_parked_run_is_surfaced_not_killed_on_send (asserts BlockedApproval; was getting Cancelled). handle_terminal now returns whether it handled the event as a tracked subagent child (Result<bool>); the cancel only fires when it returns true. Main runs and any non-subagent run are left untouched. Tests: - main_run_parked_on_gate_is_not_cancelled_by_observer locks the observer-level invariant (no cancel/resume for a non-subagent parked run). - The two parked-child robustness tests (cancel-failure propagation, ownerless actor recovery) were reworked onto a real subagent-child scaffold (build_parked_child_scenario) since the cancel now correctly requires a tracked child. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #5250 review (coderabbit): the cancel's two remaining early returns (coordinator not bound, run state carries no actor) returned Ok(()), which acknowledges cleanup that did not happen — the child stays Blocked* with its lock held and the path never retries. Both now return TurnError::Unavailable so the failure surfaces, consistent with the fail-loud rule and with handle_terminal's own coordinator-missing behavior. (The genuine no-op case — the run record already gone, ScopeNotFound — still returns Ok.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix run-wait state classification so waiters and observers stop hanging and killing parked runs incorrectly.
Stats: 2 findings (from 4 candidate findings after live-thread/prior-rationale suppression) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Conventions
- Medium Normal send path returns gate state without gate outcome (
crates/ironclaw_reborn_composition/src/runtime.rs:1973, confidence 93) — anchor:crates/ironclaw_reborn_composition/src/runtime.rs:1585
send_user_message()is still documented as waiting for a terminal run and returning a final assistant reply, but the shared waiter now returns user-gated states too. That makes the normal production send path return a blockedAssistantReplywithout thegate_refcarried by the typed gate-aware outcome.
Tests
- Medium OpenAI retrieve lacks an auth-parked regression (
crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:793, confidence 78) — anchor:crates/ironclaw_reborn_composition/src/openai_compat_serve.rs:788
read_response()now mapsparked_on_gatetoIncomplete, but retrieve only has the GatePrompt regression while the wait path has explicit AuthPrompt coverage.
Suppressed after prior-thread review
- Parked-child split terminalization: already documented as an interim shape with a tracked follow-up for a diagnostic-preserving coordinator transition, and prior discussion accepted that tradeoff.
- Parked-child actor fallback: current stores reconstruct real run state with a non-optional actor, and the prior actor-recovery/fail-loud thread was addressed with sound reasoning.
- Existing unresolved
BlockedDependentRun/BlockedResourceruntime E2E requests: kept suppressed because the author explained why those cannot be driven through the current production integration harness.
PR #5250 review: after Bug #2, wait_for_terminal returns ParkedAwaitingUser states, so the production send_user_message could return AssistantReply { status: BlockedApproval/BlockedAuth/BlockedResource } with no gate_ref — the caller saw a parked status but could not resolve or surface the gate from the return value, and the doc still claimed a terminal-only contract. AssistantReply now carries gate_ref: Option<GateRef>, populated from the run state. send_user_message_internal enforces the blocked-reason invariant (a ParkedAwaitingUser status MUST have a gate_ref, else it errors) so a parked reply always carries one; terminal replies carry None. Docs on both send_user_message and AssistantReply updated to describe the gate-aware contract. The integration test gate_parked_run_is_surfaced_not_killed_on_send now also asserts the surfaced reply carries gate_ref. Note: this leaves AssistantReply (now status+gate_ref) overlapping the test-only RebornTurnDriveOutcome::BlockedOnGate; unifying onto one send path/return type (and migrating the QA-trace recorder off the enum) is a follow-up, kept out of this run-wait PR because it rewrites trace-recording test infra. Also adds openai_responses_retrieve_returns_incomplete_when_run_parks_on_auth_gate (retrieve auth-parked coverage, mirroring the wait path). 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 (1)
crates/ironclaw_reborn_composition/src/test_support.rs (1)
32-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t let this helper fabricate gate-parked replies without a
gate_ref.
AssistantReplynow treatsBlockedApproval/BlockedAuth/BlockedResourceas a gated contract, but this helper still accepts anyTurnStatusand always buildsgate_ref: None. That lets tests create a statesend_user_message()now rejects, which can hide regressions in the parked-run surface. Either add agate_refparameter or assert that user-gated statuses are not passed here.Minimal guard
pub fn assistant_reply_without_text_for_test( status: TurnStatus, failure_category: Option<&str>, ) -> AssistantReply { + assert!( + !matches!( + status, + TurnStatus::BlockedApproval | TurnStatus::BlockedAuth | TurnStatus::BlockedResource + ), + "parked replies in tests must provide a gate_ref" + ); AssistantReply { conversation: ConversationId( ThreadId::new("test-assistant-reply").expect("static test thread id"), // safety: static test helper id is a valid thread id literal. ), run_id: TurnRunId::new(), status, failure_category: failure_category.map(str::to_owned), text: None, gate_ref: 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_reborn_composition/src/test_support.rs` around lines 32 - 44, assistant_reply_without_text_for_test currently allows any TurnStatus while always setting gate_ref to None, which can fabricate invalid gate-parked AssistantReply states. Update this helper in test_support.rs to either accept an explicit gate_ref for gated statuses or reject BlockedApproval, BlockedAuth, and BlockedResource at the helper boundary. Keep the contract aligned with AssistantReply and send_user_message by referencing assistant_reply_without_text_for_test and the gated TurnStatus variants when making the check.
🤖 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/test_support.rs`:
- Around line 32-44: assistant_reply_without_text_for_test currently allows any
TurnStatus while always setting gate_ref to None, which can fabricate invalid
gate-parked AssistantReply states. Update this helper in test_support.rs to
either accept an explicit gate_ref for gated statuses or reject BlockedApproval,
BlockedAuth, and BlockedResource at the helper boundary. Keep the contract
aligned with AssistantReply and send_user_message by referencing
assistant_reply_without_text_for_test and the gated TurnStatus variants when
making the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf5eae50-051c-4ca0-9f1c-533d16d5a489
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/test_support.rscrates/ironclaw_reborn_composition/tests/runtime_gate_wait.rs
|
Closing as stale — no activity in over three weeks. The branch is untouched; reopen if this is still needed. |
This branch was successfully deployed
1 participant
Problem
Code that waits/polls for a run to reach a terminal state hand-rolled partial predicates (mostly
TurnStatus::is_terminal()), which omit the four parked-awaiting-user states (BlockedApproval,BlockedAuth,BlockedResource,BlockedDependentRun). A run parked on one of those never becomes terminal, so the waiters mishandle it. A read-only audit found three genuine bugs from this one missing concept — each worse than the Slack-delivery timeout that surfaced it:openai_compat_serve::wait_for_response_completion— HTTP handler hangs forever. An unbounded loop whose status mapper falls through (_ => None) on a parked run → the/v1/responseshandler never returns.ironclaw_reborn::runtime::wait_for_terminal— kills a run parked on a gate. Waitsis_terminal(), 180s, then issuescancel_run(Timeout)→ a run legitimately waiting on the user's approval/auth is cancelled out from under them.subagent::completion_observer— parent hangs forever when a child subagent parks on its own gate (theis_terminal()filter never firesresume_parent).runtime::wait_for_terminal_or_gate(an exhaustive match) was the one correct waiter — this PR makes its discipline the shared default.Fix — one exhaustive classifier, then minimal per-site changes
TurnStatus::wait_class()(ironclaw_turns/src/status.rs): an exhaustive match (no_arm) sorting every status intoRunWaitClass { Running, ParkedAwaitingUser, ParkedAwaitingRun, TerminalSuccess, TerminalFailure }.is_terminal()keeps its single meaning (lock release); the classifier answers the different question every waiter actually asks. The exhaustive match is the guardrail: a newTurnStatusvariant becomes a compile error at this one classifier, not a silent forever-hang at the call sites.openai_compat_serve): bound the wait and classify a parked run → returnincompleteinstead of hanging; status mapper made exhaustive/typed.runtime::wait_for_terminal): route the production message path through gate-aware waiting so aParkedAwaitingUserrun is surfaced, not cancelled.completion_observer): react toParkedAwaitingUserso a parked child surfaces to the parent instead of deadlocking it.ironclaw_turns/CLAUDE.md: waiting on a run state must go throughwait_class(); never hand-roll a partialis_terminal()predicate.Tests (green)
wait_class_matches_expected_classification,parked_states_are_never_terminal,is_terminal_equals_terminal_wait_classes— pass.openai_responses_wait_returns_incomplete_when_run_parks_on_gate+..._on_auth_gate(--features openai-compat-beta) — pass (a parked run returns promptly instead of hanging).runtime_gate_waitintegration tests — pass.completion_observertests — pass.cargo clippyon the touched crates: clean (the pre-existingfactory.rsdead-code warnings are unrelated/not in this diff).Coordination with open PRs
subagent_prompt_port.rs, notcompletion_observer.rsor the park path, so Onboarding: show Telegram in channel selection and auto-install bundled channel #3 does not duplicate it. Kept; will rebase cleanly. (Confirmed viagh pr diff 5170.)ironclaw_reborn_http_kit) also touchesopenai_compat_serve.rs; site Move whatsapp channel source to channels-src/ for consistency #1's change is intentionally surgical (bound the loop + classify) to rebase over that refactor.wait_class()is a documented follow-up.Note
Built off
4f28febbe; needs a rebase onto currentmain(now carries #5204/#5207/#5163). No expected file conflicts beyond the #5170/#5137 coordination above.🤖 Generated with Claude Code