feat(question): flag-gated external-result rework (PR A) - #764
Merged
Conversation
Add optional `reason: z.enum(["aborted", "shutdown", "tool_failure"])` to durable ToolStateError. Backward-compatible: existing stored sessions decode unchanged with `reason: undefined`. Future commits (writer wiring, renderer branches) consume the field; this commit only widens the schema so old data still parses and new writers have a slot. Part of #756 PR A (question tool external-result rework, v10 spec).
Tag "ExternalResultError" with reason union "aborted" | "shutdown". Will be raised by ctx.externalResult when the Deferred is aborted by the surrounding turn (ctx.abort fires) or by session destroy (onSessionDestroyed hook). The runner narrows the Tool wrapper's catchAll so this error survives as a typed failure rather than being defectified, and the processor's failToolCall reads `.reason` to populate the durable ToolStateError.reason field added in the previous commit. No consumers yet; pure new module. Part of #756 PR A.
…hine State machine: pending → resolved (tombstone, 30s TTL) → cleaned. Exports: register, lookup, resolveIfPending, failIfPending, hasPending, onSessionDestroyed. Cleanup is on-lookup (no timer thread); the clock is injectable for tests. Semantic split (addresses v10 round-3 P2 #4): onSessionDestroyed fires only on server shutdown / session delete and rejects pending Deferreds with `ExternalResultError({reason: "shutdown"})`. User turn aborts will flow through ctx.abort → failIfPending with reason: "aborted" — wired in a later commit. The two paths never overlap; reason is always "aborted" XOR "shutdown" XOR completed-normally. Registry semantics (addresses v10 round-2 P2 #1): hasPending counts only pending entries (tombstones excluded); resolveIfPending returns a discriminated outcome ("resolved" / "already_resolved" / "not_found") that the route maps to 200 / 409 / 404. Within the 30s tombstone window a retry of the same user gesture sees the deterministic 409 instead of a 404 race. No consumers yet (route, ctx.externalResult, session-destroy hook all come in later commits). Part of #756 PR A.
Replace the blanket `Effect.orDie` in `wrap()` at tool.ts:131 with a narrow `Effect.catch` that lets `ExternalResult.Error` propagate as a typed failure, while every other typed error continues to defectify — matching the prior `.orDie` behavior so existing tool error paths are unchanged (verified across 305 tool/* tests). The typed `ExternalResult.Error` needs to survive the wrapper because the processor's failToolCall will (in a later commit) read its `.reason` and persist it as `ToolStateError.reason`. With the prior `.orDie`, the typed error was wrapped in a Die cause and the writer could only inspect a generic defect string. Part of #756 PR A.
Tools that suspend on a user response can statically declare `externalResult: true` on their `Def`. The runner / renderer / dock can then scope behavior to only externalResult tools (preparing-state placeholder before the registry registers the Deferred; question dock projects only over tool parts whose tool def declares this flag). Declaration only; the consumer wiring (the metadata flag on the running tool part, and the renderer / dock logic that reads it) is added in subsequent commits when ctx.externalResult is plumbed through. Part of #756 PR A.
The new optional `ctx.externalResult({ inputSnapshot })` method
registers a Deferred in the external-result registry keyed by
(sessionID, messageID, callID), flips the running tool part's
`metadata.externalResultReady` flag to true (so the renderer
transitions from "preparing..." placeholder to active input
controls), wires the AbortSignal so a turn cancel rejects the
Deferred with `ExternalResultError({reason: "aborted"})`, and
suspends `execute` until the Deferred settles via:
- POST /session/.../tool/respond (later commit) — `{kind: "submitted",
value}` or `{kind: "dismissed"}` on the success channel.
- ctx.abort firing — typed failure with reason "aborted".
- ExternalResult.onSessionDestroyed (later commit) — typed failure
with reason "shutdown".
Property is optional on Tool.Context so non-suspending tools (the
vast majority) don't need to provide a stub. The question tool will
opt in by declaring `externalResult: true` on its Def and calling
this method from a flag-gated branch (later commit). Implementation
lives in the resolveTools context factory at prompt.ts:709, captured
in the closure with the EffectBridge shape from `run.promise`.
Part of #756 PR A.
`failToolCall` now reads `error.reason` from `ExternalResult.Error` and persists it as `ToolStateError.reason` on the durable tool part. Mapping is narrow on purpose: - `error instanceof ExternalResult.Error` → write `reason` (one of "aborted" | "shutdown"). These are the only typed reasons emitted in PR A. - `error instanceof Question.RejectedError` (legacy) → leave `reason` undefined so the renderer's substring fallback at tool.tsx:59,74 continues to fire for legacy "dismissed" / "interrupted" copy. - Any other thrown/defect → also leave `reason` undefined. The renderer's existing generic-error copy applies. Net: flag-off remains bit-for-bit identical to today's renderer output (no error part written during flag-off carries a reason); only the new ExternalResult.Error path writes a typed value. Part of #756 PR A.
The LLM provider silent-timeout path used to re-arm rather than abort when blockers.hasAwaitingQuestion returned true (legacy path; protects the user from being killed mid-question-answering). PR A adds the new path's guard alongside it: ExternalResult.hasPending(sessionID). Either path returning true re-arms. After PR A merges and the flag is on by default in PR B, the legacy hasAwaitingQuestion call (and the blockers namespace) goes away; hasPending becomes the sole guard. Until then, both coexist to keep both flag-off (legacy) and flag-on (new) sessions safe. Part of #756 PR A.
New server route for resolving a pending external-result Deferred.
The body is a discriminated union on `kind`:
- `{ kind: "submit", messageID, callID, payload }` — payload is
passed through as-is to the Deferred's resolved value as
`{ kind: "submitted", value: payload }`. Tool-specific validation
(label membership, count, multi-select bounds) will live in the
question tool's submit handler once flag-on lands; the route's
contract today is structural.
- `{ kind: "dismiss", messageID, callID }` — Deferred resolves with
`{ kind: "dismissed" }`.
Failure mode order matches v10:
422 parse → 404 lookup → 409 already_resolved → 200 resolve
Status codes 409/404 distinguish tombstone (recent resolve, within
30s TTL — second client / retry) from absent (never registered or
TTL elapsed). The renderer can surface 409 as "answered from
another device" and 404 as "session restarted, please retry".
Part of #756 PR A.
Wires ExternalResult.onSessionDestroyed into the existing
Session.clearPendingInteractions path that already tears down the
Question/Permission/SessionBlocker services on session delete and
session archive. Pending external-result Deferreds get rejected
with ExternalResultError({reason: "shutdown"}); tombstones are
dropped.
Per v10 semantic split (round-3 P2 #4): this hook does NOT fire on
turn abort or status transitions. Turn aborts route through
ctx.abort → failIfPending with reason: "aborted". So `reason:
"aborted"` and `reason: "shutdown"` never overlap.
Part of #756 PR A.
Adds `PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT` env flag (dynamic getter
on Flag namespace; default off). When on, the question tool's
execute uses `ctx.externalResult` to suspend on the new registry's
Deferred; when off, the legacy `Question.ask` path runs unchanged
(bit-for-bit identical to pre-PR-A).
Flag-on success-channel discriminant:
- `{kind: "dismissed"}` → tool returns `metadata: { answers: [],
dismissed: true }` (renderer keys dismiss on `metadata.dismissed
=== true`, not on `answers.length`).
- `{kind: "submitted", value}` → tool reads `value.answers` and
formats the same user-facing string as the legacy path.
ExternalResultError (turn abort, session shutdown) is NOT caught
here — it propagates as a typed failure through the Tool wrapper
(now narrowed via Effect.catch) to the writer, which records
`ToolStateError.reason`. The legacy branch's `.pipe(Effect.orDie)`
is removed since the wrapper now handles defectification of all
non-ExternalResult typed errors equivalently.
Declares `externalResult: true` statically so the renderer / dock
can scope behavior (the "preparing..." placeholder while the
registry registers the Deferred, the inline timeline marker
during running). The flag-off behavior is unaffected by the
declaration.
Part of #756 PR A.
Updates tool.tsx render switch with the v10 two-surface rendering rule (D1 = B, dock projection): - Flag-on question running → thin marker "↓ Pending question — answer below" inline in the timeline. No submit/dismiss controls (those live in the dock). Detection: the new path writes `metadata.externalResultReady` when ctx.externalResult registers. - Flag-on question completed with `metadata.dismissed === true` → "Skipped by user" friendly copy (keys on the explicit dismissed flag, NOT on `answers.length`, since all-blank submit is a legitimate non-dismiss case). - Flag-on question error with `state.reason === "aborted" | "shutdown"` → friendly copy via typed branch (no substring match needed). - Legacy substring fallback on `state.error` for parts written before the new path (reason undefined) is preserved. The `hideQuestion` memo now hides only when the new path is NOT active (flag-off legacy continues to hide running questions because the dock is the only render surface). Flag-on running surfaces the marker. i18n: adds `ui.messagePart.questions.pendingMarker` to en and zh. Part of #756 PR A.
The flag-on path needs a dock data source that reads running question parts from sync.data.message and routes submissions through POST /session/:sessionID/tool/respond. Both depend on SDK regeneration, so the full wire-up lands in PR B. Document the gap inline so reviewers know the dock is intentionally empty when flag is on, and PR A's E2E exercises the route directly via fetch.
The recovery clock's missingRunning snapshot heuristic detects a running question tool part with no matching sync.data.question entry and arms a halt timer. With PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT on, the new path never writes sync.data.question (the tool part itself is the source of truth, and the external-result registry manages the lifecycle). Without this skip, the recovery clock would arm on every flag-on session, retry, and escalate to halt — reintroducing the exact bug this rework targets. Detection key: state.metadata.externalResultReady, written by ctx.externalResult once the registry has registered the Deferred. This keeps the gate at the part level so mixed legacy + flag-on sessions during a rolling deploy remain safe.
…4/409/422
Cover the route round-trip end to end: register a Deferred via
ExternalResult.register, hit the route through the real instance
router, assert HTTP status and that the Deferred resolves with the
expected discriminated value. Five paths:
- submit: 200, Deferred -> {kind:"submitted", value}
- dismiss: 200, Deferred -> {kind:"dismissed"}
- unknown (sessionID,messageID,callID): 404
- double-submit inside tombstone TTL: second call 409
- malformed body (kind missing/wrong): 4xx
Abort path is covered by external-result-registry.test.ts via
failIfPending; ctx.externalResult abort wiring lives in prompt.ts and
is exercised by session-level tests in PR B once SDK regen lands.
…ss renderable Two crosscheck findings on PR A: - question.ts: the new POST /:sessionID/tool/respond accepts payload as z.unknown(), so a malformed submit (missing/non-array answers) would crash formatAnswers with TypeError. Validate the shape at the tool boundary and coerce malformed input to empty answer slots, semantically equivalent to a dismiss with all slots skipped. Same failure surface as legacy all-blank. - tool.tsx: the completed-dismiss Match branch gated on newQuestionPath(), which reads metadata.externalResultReady on the tool part state. That flag is only present in the running-state metadata; the writer replaces state on completion so the branch was unreachable. metadata.dismissed === true is unique to the new path (legacy dismiss routes through the error branch), so we can drop the newQuestionPath gate without ambiguity.
Contributor
📝 WalkthroughWalkthroughAdds an external-result execution path for the question tool (flag-gated), including Context API, question decoder, server submit/dismiss route, session/timeout integration, frontend rendering changes, extended error metadata, and comprehensive tests. ChangesExternal Result Tool Execution
Sequence DiagramsequenceDiagram
participant ToolExec as Tool Execution
participant ExtResult as ExternalResult Registry
participant ServerRoute as POST /session/:sessionID/tool/respond
participant Client as Client/UI
ToolExec->>ExtResult: register(sessionID, messageID, callID)
ExtResult-->>ToolExec: Deferred (awaiting)
ToolExec->>ToolExec: suspend
Client->>ServerRoute: submit/dismiss payload
ServerRoute->>ExtResult: resolveIfPending(sessionID,messageID,callID)
ExtResult-->>ToolExec: resolve/reject Deferred
ToolExec->>ToolExec: resume with outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session/blockers/question-fallback.test.ts, packages/app/src/pages/session/blockers/question-fallback.ts, packages/app/src/pages/session/blockers/use-session-blockers.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Astro-Han
force-pushed
the
claude/question-tool-flagged
branch
from
May 19, 2026 09:53
2c08a55 to
e67819d
Compare
![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 new "external-result" mechanism for tool calls, specifically for the question tool. It adds a new API endpoint /:sessionID/tool/respond to handle user submissions or dismissals, updates the session processor to manage pending external results via Deferreds, and modifies the UI to display pending markers and handle specific error reasons like aborts or shutdowns. Feedback identifies an undefined variable run in the abortHandler and a type mismatch in the test payload for the new response route.
Perf delta summaryComparator: pass
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/session/prompt.ts (1)
320-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve soft-cancel semantics for external-result questions.
This adds a second “awaiting user” state, but
cancel({ mode: "soft" })still only checksSessionBlocker.hasAwaitingQuestion(). With the flag on, a soft cancel now aborts a pending external-result question instead of leaving it open for the user to answer. Please gate soft cancel onExternalResult.hasPending(sessionID)too, the same way the silent-timeout path does.Suggested fix
const mode = options?.mode ?? "hard" yield* elog.info("cancel", { sessionID, mode }) - if (mode === "soft" && (yield* blockers.hasAwaitingQuestion(sessionID))) { + if ( + mode === "soft" && + ((yield* blockers.hasAwaitingQuestion(sessionID)) || ExternalResult.hasPending(sessionID)) + ) { yield* elog.info("cancel ignored", { sessionID, mode, reason: "awaiting_question" }) return false }Also applies to: 736-791
🤖 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 `@packages/opencode/src/session/prompt.ts` around lines 320 - 329, The soft-cancel path in SessionPrompt.cancel currently only checks blockers.hasAwaitingQuestion(sessionID) and will abort external-result questions; update the soft cancel guard to check both blockers.hasAwaitingQuestion(sessionID) and ExternalResult.hasPending(sessionID) (i.e., only treat as "awaiting user" if either blocker is true) so that soft cancels do not abort pending ExternalResult questions, and apply the identical change to the other cancel/silent-timeout handling block that mirrors this logic; keep elog.info logging consistent when skipping the cancel.
🧹 Nitpick comments (1)
packages/opencode/test/server/tool-respond-route.test.ts (1)
145-159: ⚡ Quick winPin malformed-body responses to the contract status (422).
This test currently passes on any 4xx, so a regression away from the intended validation status could slip through unnoticed. Consider asserting
422directly.Suggested change
- expect(res.status).toBeGreaterThanOrEqual(400) - expect(res.status).toBeLessThan(500) + expect(res.status).toBe(422)🤖 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 `@packages/opencode/test/server/tool-respond-route.test.ts` around lines 145 - 159, The test for malformed request bodies currently accepts any 4xx; change the assertions in the test inside tool-respond-route.test.ts (the test that builds app via Server.Default().app and creates a session via Session.Service.create) to require the specific contract validation status 422 instead of the range checks—replace expect(res.status).toBeGreaterThanOrEqual(400) / toBeLessThan(500) with a single expect(res.status).toBe(422) so regressions that return other 4xx codes are caught.
🤖 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 `@packages/opencode/src/server/instance/session.ts`:
- Around line 467-499: The route currently accepts body.payload as z.unknown()
and resolves the deferred immediately; change the handler so that after
ExternalResult.lookup({ sessionID, messageID, callID }) and before calling
AppRuntime.runPromise(ExternalResult.resolveIfPending(...)) you validate
body.payload against the registered inputSnapshot for that pending external call
(the same validation logic used in question.ts), returning c.json({ error:
"...validation_error..." }, 422) on failure; only when validation succeeds
proceed to call ExternalResult.resolveIfPending({ sessionID, messageID, callID,
value }) (and keep dismiss handling unchanged). Ensure you reference
messageID/callID to fetch the inputSnapshot used for validation and preserve the
existing response branches for outcome values ("resolved", "already_resolved",
"no_pending_tool_call").
In `@packages/opencode/src/session/session.ts`:
- Around line 589-595: The call to ExternalResult.onSessionDestroyed(sessionID)
must run even when InstanceState is missing because clearPendingInteractions()
currently bails out on missing InstanceState and prevents ExternalResult
shutdown cleanup; move the yield* ExternalResult.onSessionDestroyed(sessionID)
invocation to execute before the InstanceState guard (or split the guard so
clearPendingInteractions/legacy cleanup remains gated but
ExternalResult.onSessionDestroyed always runs), ensuring pending ExternalResult
waiters are rejected with the "shutdown" durable reason on session
delete/archive; update the function containing clearPendingInteractions(), the
InstanceState check, and any code paths related to remove() to preserve existing
legacy gating while guaranteeing ExternalResult.onSessionDestroyed(sessionID)
always executes.
In `@packages/ui/src/components/message-part/parts/tool.tsx`:
- Around line 26-35: The memo hideQuestion triggers a TDZ because createMemo
evaluates immediately but partMetadata is declared later; move the declarations
of isQuestion, isQuestionRunning, newQuestionPath, and hideQuestion so they come
after the partMetadata definition. Specifically, locate the partMetadata symbol
(where it's initialized) and relocate the functions isQuestion,
isQuestionRunning, newQuestionPath and the createMemo call for hideQuestion to
follow that declaration, ensuring the createMemo callback calls partMetadata
only after partMetadata exists.
---
Outside diff comments:
In `@packages/opencode/src/session/prompt.ts`:
- Around line 320-329: The soft-cancel path in SessionPrompt.cancel currently
only checks blockers.hasAwaitingQuestion(sessionID) and will abort
external-result questions; update the soft cancel guard to check both
blockers.hasAwaitingQuestion(sessionID) and ExternalResult.hasPending(sessionID)
(i.e., only treat as "awaiting user" if either blocker is true) so that soft
cancels do not abort pending ExternalResult questions, and apply the identical
change to the other cancel/silent-timeout handling block that mirrors this
logic; keep elog.info logging consistent when skipping the cancel.
---
Nitpick comments:
In `@packages/opencode/test/server/tool-respond-route.test.ts`:
- Around line 145-159: The test for malformed request bodies currently accepts
any 4xx; change the assertions in the test inside tool-respond-route.test.ts
(the test that builds app via Server.Default().app and creates a session via
Session.Service.create) to require the specific contract validation status 422
instead of the range checks—replace
expect(res.status).toBeGreaterThanOrEqual(400) / toBeLessThan(500) with a single
expect(res.status).toBe(422) so regressions that return other 4xx codes are
caught.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 81920945-7e92-4e5d-ae25-681c5c63624c
📒 Files selected for processing (21)
packages/app/src/pages/session/blockers/question-fallback.test.tspackages/app/src/pages/session/blockers/question-fallback.tspackages/app/src/pages/session/blockers/use-session-blockers.tspackages/core/src/flag/flag.tspackages/opencode/src/server/instance/session.tspackages/opencode/src/session/llm.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/external-result.tspackages/opencode/src/tool/question.tspackages/opencode/src/tool/tool.tspackages/opencode/test/server/tool-respond-route.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/tool/external-result-registry.test.tspackages/opencode/test/tool/external-result.test.tspackages/opencode/test/tool/tool-define.test.tspackages/ui/src/components/message-part/parts/tool.tsxpackages/ui/src/i18n/en.tspackages/ui/src/i18n/zh.ts
Soft cancel (Stop button / Escape) only checked the legacy hasAwaitingQuestion blocker; with PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT on, pending external-result questions slipped through and got aborted. Mirror the silent-timeout re-arm in llm.ts:486 by OR-ing ExternalResult.hasPending(sessionID) into the same gate. Reported by CodeRabbit on PR #764.
Solid createMemo evaluates eagerly at component init. hideQuestion's callback synchronously calls newQuestionPath() -> partMetadata(), but partMetadata was declared on a later line so the const binding was in its temporal dead zone. Any running question rendering through this component path hit ReferenceError at mount. Reorder the helper declarations so partMetadata exists before the createMemo runs. Reported by CodeRabbit on PR #764.
clearPendingInteractions early-returns when InstanceState is missing because the legacy Question/Permission/Blocker services depend on instance context. But ExternalResult's registry is module-level and remove() explicitly supports broken-session cleanup paths that have no InstanceState. With the guard order swapped, pending external-result Deferreds for those sessions never received the 'shutdown' reason. Run ExternalResult.onSessionDestroyed before the guard so the shutdown path is unconditional; legacy clearers stay gated below. Reported by CodeRabbit on PR #764.
The route accepted any payload and resolved the Deferred immediately, so a malformed POST became a successful tool result with all answers silently coerced to empty. Three review rounds converged on the same load-bearing question: should the route allow external POSTs to fabricate a completed tool result, or only accept decoder-validated responses? The answer is decoder-validated. The route stays tool-agnostic; tools that suspend on ctx.externalResult may register a response decoder that the route runs before resolving. Decoder failure returns 422 with structured error + details and leaves the registry entry pending so the client can correct and resubmit. Wiring: - external-result.ts: ResponseDecoder + DecodeResult types; PendingEntry carries an optional decoder; lookup() exposes it. - tool.ts: Context.externalResult accepts decoder; re-export types. - prompt.ts: externalResult primitive forwards decoder to register. - session.ts route: submit branch runs lookup.decoder before resolveIfPending; 422 on failure (Deferred untouched). - question.ts: questionDecoder enforces the legacy 4 reply-time rules (count, trim, single-select multi-answer, custom:false label membership). Snapshot self-check for duplicate option labels runs before ctx.externalResult — that rule is about the LLM's prompt, not the user's submission. Tests: - question-decoder.test.ts: rule-by-rule coverage. - tool-respond-route.test.ts: 422 + retry success on corrected payload; decoderless tool still forwards raw payload; dismiss skips decoder; malformed outer body still 400 (zod validator, distinct contract). Reported on PR #764 by CodeRabbit (P2 outside-diff), with the same shape earlier flagged by crosscheck Codex P1 + Claude P1.
Owner
Author
|
Addressing the outside-diff finding and the nitpick from the CodeRabbit review: Outside-diff: soft-cancel guard for Nitpick: pin malformed-body test to 422 — Partially adopted. The test that sends a malformed outer body ( Round-up of dispositions:
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Adds three source-text assertions to ensure `partMetadata` is declared before `newQuestionPath` and before the eager `hideQuestion = createMemo(...)` call. Anyone who reorders the declarations back will flunk this test. Source-text matches existing ui-package convention (see button-states.test.ts and undefined-tokens.test.ts) and avoids the runtime cost of a browser-conditions render — happydom alone resolves solid-js/web to its server build, where router-touching components throw notSup. Covers the regression fixed in e327368.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/ui/src/components/message-part/parts/tool.tsx (1)
109-126: ⚡ Quick winCombine duplicate error reason branches.
The
abortedandshutdownbranches render identical JSX with the same "interrupted" message. Combine them into a single condition to eliminate duplication.♻️ Proposed refactor
- if (isQuestion() && reason === "aborted") { - return ( - <div style="width: 100%; display: flex; justify-content: flex-end;"> - <span class="text-body text-fg-weak cursor-default"> - {i18n.t("ui.messagePart.questions.interrupted")} - </span> - </div> - ) - } - if (isQuestion() && reason === "shutdown") { + if (isQuestion() && (reason === "aborted" || reason === "shutdown")) { return ( <div style="width: 100%; display: flex; justify-content: flex-end;"> <span class="text-body text-fg-weak cursor-default"> {i18n.t("ui.messagePart.questions.interrupted")} </span> </div> ) }🤖 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 `@packages/ui/src/components/message-part/parts/tool.tsx` around lines 109 - 126, The two identical branches checking reason === "aborted" and reason === "shutdown" should be merged into a single condition; update the component in message-part/parts/tool.tsx so that the existing JSX is returned when isQuestion() && (reason === "aborted" || reason === "shutdown") instead of duplicating the same block twice, removing the redundant branch and keeping the single span that renders i18n.t("ui.messagePart.questions.interrupted").
🤖 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 `@packages/opencode/src/session/prompt.ts`:
- Around line 758-771: The updateToolCall callback only sets externalResultReady
when match.state.status === "running", which misses parts still in "pending";
change the status check in input.processor.updateToolCall (the callback using
callID and match) to treat "pending" as reachable too (e.g., update when
match.state.status === "running" || match.state.status === "pending" or check
for !== "completed"/terminal states), and keep the existing metadata merge logic
so metadata: { ...existing, externalResultReady: true } is written for both
running and pending parts.
In `@packages/opencode/src/tool/question.ts`:
- Around line 90-97: The membership check fails when option labels contain
incidental whitespace because validLabels is built from raw q.options while
answer entries were trimmed earlier; update the validation in the block where
q.custom === false (using variables validLabels, q.options, answer) to normalize
option labels the same way answers are normalized (e.g., trim each option.label
before inserting into validLabels) so comparisons use the same normalized form
and the details object can still include the original or normalized labels as
appropriate.
In `@packages/opencode/test/server/tool-respond-route.test.ts`:
- Around line 11-12: The tests currently use a manual runtime shim (run,
AppRuntime.runPromise) and Instance.provide; migrate each test to the repo
pattern by replacing ad-hoc run(...) and Instance.provide usage with testEffect
+ Effect.gen and provideTmpdirInstance: define const it = testEffect(...) with
the same required layers (e.g., Session.defaultLayer, Config.defaultLayer), wrap
each test body as provideTmpdirInstance(() => Effect.gen(function* () { ... }),
{ git: true }), convert awaited run(Service.use(...)) calls into yield*
expressions inside Effect.gen using Service.pipe/Effect.flatMap, and remove the
run helper and manual Instance.provide blocks entirely to avoid unsafe casting.
---
Nitpick comments:
In `@packages/ui/src/components/message-part/parts/tool.tsx`:
- Around line 109-126: The two identical branches checking reason === "aborted"
and reason === "shutdown" should be merged into a single condition; update the
component in message-part/parts/tool.tsx so that the existing JSX is returned
when isQuestion() && (reason === "aborted" || reason === "shutdown") instead of
duplicating the same block twice, removing the redundant branch and keeping the
single span that renders i18n.t("ui.messagePart.questions.interrupted").
🪄 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: CHILL
Plan: Pro Plus
Run ID: 619d669f-965b-4142-acbb-63dc78a64201
📒 Files selected for processing (9)
packages/opencode/src/server/instance/session.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/external-result.tspackages/opencode/src/tool/question.tspackages/opencode/src/tool/tool.tspackages/opencode/test/server/tool-respond-route.test.tspackages/opencode/test/tool/question-decoder.test.tspackages/ui/src/components/message-part/parts/tool.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/server/instance/session.ts
LLM-supplied option labels may carry incidental whitespace. The decoder trims answers (so " yes " becomes "yes") but used raw option labels in the membership set, producing a 422 the client can never recover from. Normalize both sides.
ctx.externalResult's writer only matched "running", while the adjacent ctx.metadata helper accepts pending+running and upgrades pending to running. Today's stream order flips parts to running before execute() runs, so the asymmetry is not observable; matching the two keeps the external-result writer correct under future re-orderings.
Both error reasons render the same `ui.messagePart.questions.interrupted` copy. Merge the two `if` branches into one disjunction so the two paths cannot drift in future copy edits.
Resolves one content conflict in packages/opencode/src/session/prompt.ts cancel() — dev added a `source` log field; this branch added an ExternalResult.hasPending guard alongside hasAwaitingQuestion. The merged form keeps both: `source` is threaded through info logs, and the soft-cancel OR-guard now reads `(hasAwaitingQuestion || hasPending)`.
This was referenced May 19, 2026
Astro-Han
added a commit
that referenced
this pull request
May 20, 2026
…l lifecycle (#772) ## Summary First-principles refactor of the question tool that collapses the parallel `Question.ask`/`Question.recover` state machine into a single tool-call lifecycle driven by `ctx.externalResult` (the primitive shipped in #764). The question tool now suspends on a Deferred, the dock submits via `POST /session/:sessionID/tool/respond`, and every legacy concept that propped up the old design — bridge events, recovery clock, fallback refetch, blocker namespace, soft cancel, and the gating flag — is removed. ## Why PR A introduced `ctx.externalResult` behind `PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT` to validate the new primitive without disturbing the legacy path. PR B closes the loop: switch the dock to the new selector, gate-flip ON, delete the legacy path. Three originally-planned PRs (route switch, legacy delete, follow-on cleanup) are merged into one so the dev branch never carries both implementations simultaneously. ## Related Issue No issue — direct follow-on to #764 (PR A). ## Human Review Status Pending ## Review Focus - `packages/opencode/src/tool/question.ts` — the inline decoder and the snapshot-level duplicate-label guard - `packages/app/src/pages/session/blockers/running-external-result-question.ts` — message-stream selector and the dock/sidebar tree-walk (parent session page surfaces a child agent question; matches `sessionPermissionRequest` semantics) - `packages/app/src/pages/layout.tsx` — background question OS notification reattached to `message.part.updated`; dedup pruned on transition out of `running`; suppression walks ancestors (matches the dock) - `packages/opencode/src/server/instance/external-result.ts` — new `GET /external-result` route that joins each pending `ctx.externalResult` Deferred with its session and message+part snapshot. Brief retry covers the register / processor.updateToolCall race window. - `packages/app/src/context/global-sync/bootstrap.ts` — `hydratePendingExternalResults` writes the trio into session/message/part stores during the slow bootstrap phase so parent-page reload / cold-open still surfaces a child agent's pending question. Fetch failures swallowed to avoid the project-level reloadFailed toast. - `packages/app/src/pages/session/composer/session-question-dock.tsx` — 404/409/422 toast routing and the void-promise rejection swallow - `packages/opencode/src/session/prompt.ts` — `cancel()` signature collapse (no more `mode: "soft" | "hard"`), and the externalResult abort handler now uses `ExternalResult.abortPendingSync` so the registry tombstone lands synchronously when the abort signal fires - `packages/opencode/src/tool/external-result.ts` — new `abortPendingSync` helper plus reordered tombstone-before-yield in `resolveIfPending` / `failIfPending` so the pending → resolved transition wins races against any concurrently scheduled Effect - `packages/app/src/pages/session.tsx`, `submit.ts`, `use-session-commands.tsx` — every abort call now passes only `{sessionID, source}` - `packages/core/src/flag/flag.ts` — `PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT` is gone entirely - E2E specs in `packages/app/e2e/session/session-composer-dock.spec.ts` — five legacy-recovery tests deleted; surviving question tests drive the dock through the real tool runner via `seedSessionQuestion` ## Risk Notes - The `/question`, `/blocker`, `/session/:id/question`, `/__e2e/ask`, and `/__e2e/publish-asked` routes are deleted. Any external client still calling them will 404. There is no known consumer outside this repo. - `session.abort` no longer accepts `mode=soft`. Callers that passed `mode=soft` will get a 400 from the query validator. Internal callers (and the SDK) are updated; the abort renderer diagnostic no longer carries `mode`. - Stage 9 (capability split + lint enforcement + 4 isolation fixtures + CI job) from the original 11-stage plan was dropped after discussion: the only background timers that needed gating were deleted in Stage 6, so the lint rule would guard non-existent code. Will land separately when real background code returns. ## How To Verify ```text bun --cwd packages/sdk/js run typecheck ok bun --cwd packages/core run typecheck ok bun --cwd packages/opencode run typecheck ok bun --cwd packages/app run typecheck ok bun --cwd packages/ui run typecheck ok cd packages/opencode && bun test 2786 pass / 0 fail cd packages/app && bun test 1116 pass / 0 fail cd packages/core && bun test 55 pass / 1 unrelated fail (cross-spawn cwd, pre-existing) cd packages/ui && bun test 552 pass / 4 unrelated fail (icon-button size, pre-existing) Fresh-eyes crosscheck round 2: Codex 0 findings; Claude 0 confirmed P0/P1 (both flagged P1s were self-marked as non-issues by the reviewer) External GPT review surfaced 2 P1s in dock/notification glue (parent page missing child agent question; background question OS notification dropped). Fixed in 0a90193 / 6ed20d8 / 4b439d6 / 06e09a9; round-2 crosscheck on the fixes returned 0 P0/P1 from both reviewers. Second external GPT review surfaced 1 P1: pending child-agent questions stayed invisible across parent-page reload / cold-open because Stage 6 dropped question.asked from the SSE replay buffer and the new message.part.updated path is not replayable. Fixed in 974241b by adding GET /external-result + a bootstrap hydrate phase that rebuilds the (session, message, part) trio. Round-2 crosscheck on the fix: Codex 0 findings; Claude 0 P0/P1 (remaining P2/P3 were nit-level or PR-scope-external). Third external GPT review surfaced 1 P2 (race between abort signal and /tool/respond) and 1 P3 (silent skip when part-flush exceeds the 150ms retry window); review verdict was mergeable with both non-blocking. P2 fixed in e255aa4 by hoisting the registry tombstone out of the microtask queue (sync `abortPendingSync` helper) and reordering tombstone-before-yield in `resolveIfPending` / `failIfPending`. P3 closed without code change: skipped entries remain in the registry so the next hydrate cycle or live SSE recovers the dock; adding a warn for a never-observed path is preventive noise. ``` ## Screenshots or Recordings No visible UI surface changed — the dock still renders the same way; only its submit path moved from `/session/:id/question/:id/reply` to `/session/:id/tool/respond`. ## Checklist - [ ] **Type label** — this PR carries exactly one of \`bug\`, \`enhancement\`, \`task\`, \`documentation\`. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this. - [ ] **Routing labels** — this PR carries at least one of \`app\`, \`ui\`, \`platform\`, \`harness\`, \`ci\`. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this. - [ ] **Priority label** — this PR carries exactly one of \`P0\`, \`P1\`, \`P2\`, \`P3\`. The priority-triage bot suggests one on PR open. Confirm or override, then tick this. - [x] Human Review Status above is set to \`Pending\`, \`Approved by @<reviewer>\`, or \`Not required: <reason>\` (default is \`Pending\`; "not required" is restricted to bot-authored low-risk PRs). - [x] I linked the related issue, or stated in Summary why there is no issue. - [x] I described the review focus and any meaningful risks. - [x] I replaced the example block in How To Verify with the real verification steps and the key result for each. - [x] I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope. - [ ] **(conditional)** I manually checked visible UI or copy changes when needed, with screenshots or recordings. Leave unticked only if no visible UI or copy changed. - [ ] **(conditional)** I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes. Leave unticked only if no platform/packaging surface was touched. - [x] **(conditional)** I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant. - [x] I reviewed the final diff for unrelated changes and suspicious dependency changes. - [x] I am targeting \`dev\`, and my PR title and commit messages use Conventional Commits in English. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Child question docks now survive hard page reloads via persistent external-result hydration. * **Bug Fixes** * Simplified abort behavior to improve reliability and reduce confusing abort modes. * Submit/dismiss flows for question docks handle common HTTP errors with clearer toasts and swallow transient failures. * **Refactor** * Notifications for pending tool-question events improved and deduped to reduce noise. * Session blocker/recovery behaviors streamlined (less noisy auto-heal activity). * **Documentation** * Added localized error strings for session-question error states. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/Astro-Han/pawwork/pull/772?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/Astro-Han/pawwork/pull/772?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR A of the question-tool rework (issue #756): introduce the `ctx.externalResult` tool primitive, registry, route, and renderer branches behind `PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT` (default off). Legacy path is bit-for-bit unchanged. Question tool now declares `externalResult: true` and flag-gates to the new path inside execute.
The new path: a tool's execute suspends on a Deferred registered with `ExternalResult.register`; a POST `/session/:sessionID/tool/respond` route resolves the Deferred with the user's submitted payload (submit or dismiss); turn abort routes through `ctx.abort` → `failIfPending` with typed reason `aborted`; session destroy routes through `onSessionDestroyed` with typed reason `shutdown`. The processor's `failToolCall` persists `ToolStateError.reason` so the renderer can show typed copy ("Interrupted") instead of the legacy substring fallback.
PR B (not in this PR): SDK regen, dock data source switch to message-stream selector, legacy state machine deletion.
Why
Closes the 7-fix recurrence on question-tool cancel UX (#419 and seven follow-up PRs). Root cause was a parallel state machine (`sync.data.question` + blockers + recovery clock) layered alongside the normal tool-call lifecycle; every fix patched one corner and broke another. The first-principles rework collapses the question lifecycle into the standard tool-call path: the running tool part itself is the source of truth, the registry holds only the suspend/resume Deferred, the renderer keys on `state.status` + typed `ToolStateError.reason`.
Design doc with the v10 architecture, decision history, and three rounds of GPT Pro adversarial review lives in `docs/architecture/2026-05-19-question-as-tool-call.md` (referenced in issue #756). PR A is the flag-gated implementation slice; PR B finishes the deletion.
Related Issue
#756
Human Review Status
Pending
Review Focus
Risk Notes
The new path is fully behind `PAWWORK_QUESTION_TOOL_EXTERNAL_RESULT` (default off). Flag-off path runs the legacy code with no functional change. Flag-on UX has a known scope gap: the dock data source still reads `sync.data.blocker` + `sync.data.question`, which the new path never populates, so the dock will be empty when the flag is on. Submission round-trip is API-only until PR B regenerates the SDK and rewires the dock. This is documented inline in `use-session-blockers.ts` and exercised end-to-end by the route fetch test.
No platform/packaging/updater surface touched. No migration or data shape changes (`ToolStateError.reason` is optional). Two-surface non-ambiguity (one input surface active at a time) is preserved by the existing dock + inline timeline rule.
How To Verify
Screenshots or Recordings
No visible UI under the default flag-off state. Flag-on inline marker is a `text-fg-weak` pendingMarker label rendered to the right of the timeline; completed-dismissed renders as a similar `text-fg-weak` Skipped label. Both are intentionally minimal until PR B activates the dock.
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests