refactor(question): collapse Question.ask state machine into tool-call lifecycle - #772
Conversation
Stage 6 of PR B. Tool runner now owns the question lifecycle via ctx.externalResult, so the client-side recovery state machine is dead code. Drop the bootstrap.question.list + bootstrap.blocker.list fan-outs, the `question` / `blocker` slots from State, the event reducer cases for question.asked / question.replied / question.rejected / session.blocker.upserted / session.blocker.removed, and the sidebar's sessionQuestionRequest / sessionQuestionBlockerRequest helpers. Replace the sidebar's "asking" pip with anyDescendantExternalResultQuestion, which walks the session tree and reuses the dock's external-result selector. Composer state drops the now-unused halt option, since recovery no longer needs to abort sessions.
Stage 7 of PR B. With the question tool now resolved via ctx.externalResult, abort no longer needs a "soft" / "hard" distinction — the runner always interrupts the same way, and ExternalResult.abort propagates a typed failure to any suspended tool. Drop `mode` from: - the POST /session/:sessionID/abort query schema - SessionPrompt.cancel options and the exported wrapper - InterruptMeta (runner) - MessageV2 assistant abort diagnostics schema - Export.Snapshot diagnostics.aborts entries - the renderer sessionAbortDiagnosticEvent helper and all callers (submit.ts, keydown.ts, session.tsx, use-session-commands.tsx, use-session-page-diagnostics.ts) The cancel reason collapses from "soft_cancel" / "hard_cancel" to "cancel". SDK regenerated. Tests updated.
Stage 8 of PR B. The question tool now always uses ctx.externalResult; no second branch survives the legacy delete, so the flag has nothing to gate. Drop the Flag declaration and the dynamic getter; update the question tool comment to remove the stale flag reference.
… hooks
Stage 10 of PR B. The /question/__e2e/ask and /question/__e2e/publish-asked
server hooks were deleted with the legacy Question route in Stage 5, and
sdk.question.list / sdk.question.reply were dropped in the SDK regen in
Stage 5. Rewrite the e2e helpers to drive the dock through the real tool
runner instead:
- seedSessionQuestion now polls session messages for a running `question`
tool part with state.metadata.externalResultReady === true, returning
{messageID, callID} the dock submit path uses
- clearSessionDockSeed drops the sdk.question.reject loop; the question
tool's Deferred is rejected naturally when withDockSession removes the
session
- Five legacy-recovery tests in session-composer-dock.spec.ts are deleted
(replay/fallback-refresh/blocker/stale.asked/stale.blocker) since the
recovery state machine they exercised no longer exists
- The overflow test now arms llm.toolMatch + seedSessionQuestion in place
of the deleted /question/__e2e/ask hook
- Drop unused helpers e2eAskQuestion, e2ePublishQuestionAsked,
e2ePublishQuestionBlocker, waitForQuestionSeed
Crosscheck P3 fixes: - Dock submit/escape paths used `void reply()` / `void reject()`, which let mutateAsync's rethrow after onError surface as an unhandled promise rejection. Catch and discard at the call site — onError already produced the toast. - The dock selector accepted a running question part whose snapshot was missing or had an empty `state.input.questions` array. The dock would render with zero questions and submit `payload.answers: []`, which the server decoder rejects as a count mismatch. Skip those parts until the snapshot arrives intact.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
📝 WalkthroughWalkthroughReplace question/blocker flows with external-result hydration and message-part lookup, remove abort mode (use source-only aborts), rewire SessionQuestionDock to session.toolRespond, delete backend question/blocker services, and update tests and diagnostics. ChangesExternal-Result Migration and Schema Changes
Abort Mode Removal and Diagnostic Simplification
Question Dock Refactoring to External-Result Flow
Backend Question/Blocker Service Removal
Tool execution: Question & Plan external-result
E2E and Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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/components/prompt-input/keydown.ts, packages/app/src/components/prompt-input/submit.test.ts, packages/app/src/components/prompt-input/submit.ts, packages/app/src/context/global-sync/bootstrap.test.ts, packages/app/src/context/global-sync/bootstrap.ts, packages/app/src/context/global-sync/child-store.ts, packages/app/src/context/global-sync/event-reducer.test.ts, packages/app/src/context/global-sync/event-reducer.ts, packages/app/src/context/global-sync/session-cache.test.ts, packages/app/src/context/global-sync/session-cache.ts, packages/app/src/context/global-sync/types.ts, packages/app/src/context/renderer-diagnostics.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/layout.tsx, packages/app/src/pages/layout/sidebar-items.tsx, packages/app/src/pages/session.tsx, 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/question-reconcile.test.ts, packages/app/src/pages/session/blockers/question-reconcile.ts, packages/app/src/pages/session/blockers/question-recovery-chain.test.ts, packages/app/src/pages/session/blockers/question-recovery-clock.test.ts, packages/app/src/pages/session/blockers/question-recovery-clock.ts, packages/app/src/pages/session/blockers/question-recovery-reverify.test.ts, packages/app/src/pages/session/blockers/question-recovery-reverify.ts, packages/app/src/pages/session/blockers/question-recovery-snapshot.test.ts, packages/app/src/pages/session/blockers/question-recovery-snapshot.ts, packages/app/src/pages/session/blockers/question-refetch-runner.test.ts, packages/app/src/pages/session/blockers/question-refetch-runner.ts, packages/app/src/pages/session/blockers/request-tree.test.ts, packages/app/src/pages/session/blockers/request-tree.ts, packages/app/src/pages/session/blockers/running-external-result-question.ts, packages/app/src/pages/session/blockers/use-session-blockers.test.ts, packages/app/src/pages/session/blockers/use-session-blockers.ts, packages/app/src/pages/session/composer/session-composer-state.ts, packages/app/src/pages/session/composer/session-question-dock.tsx, packages/app/src/pages/session/use-session-commands.tsx, packages/app/src/pages/session/use-session-page-diagnostics.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.
![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 refactors the question tool to use an externalResult primitive, replacing the legacy Question.ask flow. It removes the dedicated Question and SessionBlocker services, along with their associated API routes and tests. The frontend and SDK have been updated to support this new tool response mechanism, and the session abort logic has been simplified by removing the soft/hard mode distinction. I have no feedback to provide as there were no review comments.
Perf delta summaryComparator: pass
|
Astro-Han
added
the
task
…nt question The new external-result dock selector only scanned the active session's messages, so a child agent calling question() left the parent session page without an answer dock — even though the sidebar pip (which already walks descendants) signalled "asking". Mirrors sessionPermissionRequest, which has walked the tree since #419. - Rename `anyDescendantExternalResultQuestion` -> `findDescendantExternalResultQuestion` and return the matching `DockQuestionRequest` (sessionID points at the session that actually owns the running part, so /tool/respond hits the right Deferred). - `useSessionBlockers.questionRequest` now uses the walker. - Sidebar `isAsking` reuses the same walker (=== undefined). - Tests: parent surfaces own request first, falls back to child request, walks grandchildren, returns undefined when no descendant has one.
PR B removed the legacy `question.asked` / `question.replied` listeners
from layout.tsx along with the rest of the legacy Question namespace,
but left the i18n keys orphaned and dropped the system notification for
background-session questions. Reattach it to the new event source: when
a running question tool part's metadata.externalResultReady flips true,
emit a notification for sessions the user is not currently viewing.
- Listen for `message.part.updated`; require part.tool === "question",
state.status === "running", metadata.externalResultReady === true.
- Dedup by `${directory}:${sessionID}:${partID}`; drop on
`message.part.removed`. The set keeps a single ready transition from
re-notifying when the part receives further updates.
- Suppress notification when the question's session (or its parent) is
the one currently visible — same rule permission notifications use,
consistent with the dock now walking the session tree.
- Reuse `settings.notifications.agent()` gate.
…une dedup on settle Round-1 crosscheck flagged two follow-ups on the question notification: 1. dock now walks descendants from the open session, so a grandchild question still fires an OS notification even though it is visible in-app. Replace the 1-level `parentID === currentSession` check with an ancestor walk: any ancestor matching currentSession suppresses. 2. The dedup set previously only shrank on `message.part.removed`, which is not always fired when a question part settles. Prune the entry when the part transitions out of `running` (completed, error, or dismissed), so the set cannot grow unbounded. Same change applies the ancestor walk to permission notifications too, mirroring `sessionPermissionRequest`'s tree-walk semantics (already in place since #419 for the dialog). Test naming: previous fixture asserted `externalResultReady: false` but the test name claimed "preparing window" (missing key). Renamed and added a separate case that omits the metadata key entirely.
github-actions
Bot
removed
the
task
Astro-Han
added
the
task
github-actions
Bot
removed
the
task
Astro-Han
added
task
Stage 6 dropped question.asked / replied / rejected / session.blocker.* from the SSE replay buffer because the new metadata.externalResultReady flag rides on message.part.updated, which is intentionally not in the buffer (high-volume streaming event). That left no recovery path on parent-page reload or cold-open: child session messages never hydrate through the existing route refresh (which only syncs routeSessionID), and message.part.updated alone cannot rebuild a missing child session or message in the store. A child agent's pending question dock would vanish across any reload past the SSE cursor. Add ExternalResult.list() on the server side, expose it through a new GET /external-result route that joins each pending entry with its session and message+part snapshot, and hydrate the trio during the directory bootstrap slow phase. Brief retry inside the route covers the register / processor.updateToolCall race so a reload caught in the millisecond window between Deferred registration and part-row flush still gets the part. Bootstrap fetch swallows transient errors so a backend hiccup does not surface the project-level reloadFailed toast. Adds bun unit coverage on ExternalResult.list() and the hydrate helper, plus an E2E that creates a parent + child session, seeds a question on the child, hard-reloads the parent route (fresh SSE, no cursor), and asserts the dock returns and submit resolves the child question.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/tool/plan.ts (1)
30-60: 💤 Low valueDecoder only validates first question's options - this is intentional but could be fragile.
The
planExitDecoderat line 53-58 only validates answers againstparams.questions[0]. This is correct for the current single-question plan exit prompt, but if additional questions were ever added to the snapshot, subsequent answers would bypass label validation.Consider adding a comment or assertion to make this single-question assumption explicit.
📝 Suggested comment
function planExitDecoder(payload: unknown, snapshot: unknown): DecodeResult { const params = snapshot as PlanExitSnapshot | null | undefined if (!params || !Array.isArray(params.questions)) { return { ok: false, error: "internal_snapshot_invalid" } } + // PlanExitTool always emits exactly one question; this decoder validates + // only the first row's labels. If this changes, update validation below. if (payload === null || typeof payload !== "object") {🤖 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/tool/plan.ts` around lines 30 - 60, The decoder planExitDecoder currently only validates labels against params.questions[0] (see params.questions[0] usage) which assumes a single-question snapshot; make that assumption explicit by either adding a clear comment above planExitDecoder explaining the single-question contract or adding a runtime assertion that params.questions.length === 1 (or throw an "internal_snapshot_invalid" if it's not) so future changes to questions won't silently bypass validation for subsequent answers; refer to the variables params.questions, trimmed, and validLabels when placing the comment/assertion.packages/opencode/src/server/instance/external-result.ts (1)
63-70: 💤 Low valueConsider logging when part remains missing after retries.
The retry loop handles the register→updateToolCall race, but if the part is still missing after 3 attempts, the entry is silently skipped. This could make debugging difficult if parts are consistently missing for unexpected reasons.
🔧 Suggested improvement
part = message.parts.find((p) => p.type === "tool" && p.callID === snap.callID) } - if (!part) continue + if (!part) { + log.warn("external-result pending hydrate: part not found after retries", { + sessionID: snap.sessionID, + messageID: snap.messageID, + callID: snap.callID, + }) + continue + } out.push({ session, message: message.info, part })🤖 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/server/instance/external-result.ts` around lines 63 - 70, The retry loop fetching MessageV2.get({ sessionID, messageID }) retries finding the tool part (message.parts.find(... callID === snap.callID)) but silently continues if part is still missing; add a log entry after the final check (before the "continue") that records the failure, including sessionID, messageID, snap.callID and number of attempts, using the project's logging facility (e.g., processLogger or logger) so missing-part races are visible during debugging.
🤖 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.
Nitpick comments:
In `@packages/opencode/src/server/instance/external-result.ts`:
- Around line 63-70: The retry loop fetching MessageV2.get({ sessionID,
messageID }) retries finding the tool part (message.parts.find(... callID ===
snap.callID)) but silently continues if part is still missing; add a log entry
after the final check (before the "continue") that records the failure,
including sessionID, messageID, snap.callID and number of attempts, using the
project's logging facility (e.g., processLogger or logger) so missing-part races
are visible during debugging.
In `@packages/opencode/src/tool/plan.ts`:
- Around line 30-60: The decoder planExitDecoder currently only validates labels
against params.questions[0] (see params.questions[0] usage) which assumes a
single-question snapshot; make that assumption explicit by either adding a clear
comment above planExitDecoder explaining the single-question contract or adding
a runtime assertion that params.questions.length === 1 (or throw an
"internal_snapshot_invalid" if it's not) so future changes to questions won't
silently bypass validation for subsequent answers; refer to the variables
params.questions, trimmed, and validLabels when placing the comment/assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8d2d7bc-c727-42ad-ac94-d78a21e8a31b
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (79)
packages/app/e2e/actions.tspackages/app/e2e/session/session-composer-dock.spec.tspackages/app/src/components/prompt-input/keydown.tspackages/app/src/components/prompt-input/submit.test.tspackages/app/src/components/prompt-input/submit.tspackages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/bootstrap.tspackages/app/src/context/global-sync/child-store.tspackages/app/src/context/global-sync/event-reducer.test.tspackages/app/src/context/global-sync/event-reducer.tspackages/app/src/context/global-sync/session-cache.test.tspackages/app/src/context/global-sync/session-cache.tspackages/app/src/context/global-sync/types.tspackages/app/src/context/renderer-diagnostics.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/layout.tsxpackages/app/src/pages/layout/sidebar-items.tsxpackages/app/src/pages/session.tsxpackages/app/src/pages/session/blockers/question-fallback.test.tspackages/app/src/pages/session/blockers/question-fallback.tspackages/app/src/pages/session/blockers/question-reconcile.test.tspackages/app/src/pages/session/blockers/question-reconcile.tspackages/app/src/pages/session/blockers/question-recovery-chain.test.tspackages/app/src/pages/session/blockers/question-recovery-clock.test.tspackages/app/src/pages/session/blockers/question-recovery-clock.tspackages/app/src/pages/session/blockers/question-recovery-reverify.test.tspackages/app/src/pages/session/blockers/question-recovery-reverify.tspackages/app/src/pages/session/blockers/question-recovery-snapshot.test.tspackages/app/src/pages/session/blockers/question-recovery-snapshot.tspackages/app/src/pages/session/blockers/question-refetch-runner.test.tspackages/app/src/pages/session/blockers/question-refetch-runner.tspackages/app/src/pages/session/blockers/request-tree.test.tspackages/app/src/pages/session/blockers/request-tree.tspackages/app/src/pages/session/blockers/running-external-result-question.tspackages/app/src/pages/session/blockers/use-session-blockers.test.tspackages/app/src/pages/session/blockers/use-session-blockers.tspackages/app/src/pages/session/composer/session-composer-state.tspackages/app/src/pages/session/composer/session-question-dock.tsxpackages/app/src/pages/session/use-session-commands.tsxpackages/app/src/pages/session/use-session-page-diagnostics.tspackages/core/src/flag/flag.tspackages/opencode/src/effect/app-runtime.tspackages/opencode/src/effect/runner.tspackages/opencode/src/question/index.tspackages/opencode/src/server/event-replay.tspackages/opencode/src/server/instance/blocker.tspackages/opencode/src/server/instance/external-result.tspackages/opencode/src/server/instance/index.tspackages/opencode/src/server/instance/question.tspackages/opencode/src/server/instance/session.tspackages/opencode/src/server/routes/instance/question.tspackages/opencode/src/session/blocker.tspackages/opencode/src/session/export.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/session/tool-failure.tspackages/opencode/src/tool/external-result.tspackages/opencode/src/tool/plan.tspackages/opencode/src/tool/question.tspackages/opencode/src/tool/registry.tspackages/opencode/test/question/question.test.tspackages/opencode/test/question/schema.test.tspackages/opencode/test/server/event-replay.test.tspackages/opencode/test/server/global-event-replay.test.tspackages/opencode/test/server/pending-interaction-routes.test.tspackages/opencode/test/server/session-actions.test.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/llm.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/session/pending-interaction-lifecycle.test.tspackages/opencode/test/session/prompt-effect.test.tspackages/opencode/test/session/snapshot-tool-race.test.tspackages/opencode/test/session/tool-failure.test.tspackages/opencode/test/tool/external-result-registry.test.tspackages/ui/src/components/message-part/tools/question.tsx
💤 Files with no reviewable changes (39)
- packages/app/src/pages/session/blockers/question-refetch-runner.ts
- packages/opencode/test/session/pending-interaction-lifecycle.test.ts
- packages/opencode/test/server/pending-interaction-routes.test.ts
- packages/opencode/src/session/tool-failure.ts
- packages/app/src/pages/session/blockers/question-recovery-snapshot.ts
- packages/app/src/pages/session/blockers/question-recovery-reverify.test.ts
- packages/app/src/pages/session/blockers/question-refetch-runner.test.ts
- packages/app/src/pages/session/blockers/question-reconcile.ts
- packages/opencode/src/session/blocker.ts
- packages/opencode/src/server/event-replay.ts
- packages/opencode/src/server/routes/instance/question.ts
- packages/app/src/pages/session/blockers/question-recovery-chain.test.ts
- packages/opencode/src/effect/runner.ts
- packages/opencode/test/question/question.test.ts
- packages/app/src/pages/session/blockers/question-fallback.test.ts
- packages/app/src/pages/session/blockers/question-fallback.ts
- packages/opencode/src/server/instance/blocker.ts
- packages/app/src/pages/session/blockers/question-recovery-clock.ts
- packages/opencode/test/session/tool-failure.test.ts
- packages/app/src/pages/session/blockers/question-recovery-reverify.ts
- packages/opencode/src/session/session.ts
- packages/opencode/src/session/message-v2.ts
- packages/opencode/src/server/instance/question.ts
- packages/opencode/src/effect/app-runtime.ts
- packages/opencode/src/tool/registry.ts
- packages/app/src/pages/session/blockers/question-recovery-clock.test.ts
- packages/app/src/context/global-sync/session-cache.ts
- packages/app/src/pages/session/blockers/question-recovery-snapshot.test.ts
- packages/opencode/test/session/llm.test.ts
- packages/app/src/pages/session/use-session-page-diagnostics.ts
- packages/app/src/pages/session/blockers/question-reconcile.test.ts
- packages/opencode/src/session/export.ts
- packages/app/src/context/renderer-diagnostics.ts
- packages/opencode/src/server/instance/session.ts
- packages/opencode/test/session/snapshot-tool-race.test.ts
- packages/core/src/flag/flag.ts
- packages/app/src/context/global-sync/child-store.ts
- packages/app/src/context/global-sync/event-reducer.ts
- packages/app/src/context/global-sync/types.ts
|
@coderabbitai Acknowledged both nitpicks; neither will be acted on in this PR.
No code changes; closing both threads. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Resolving both threads now. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 21
I wasn't able to locate the original inline comments via the API to resolve them programmatically (they may already be resolved or scoped to a different review session), but consider both threads closed on my end. |
Upstream history (anomalyco/opencode) is no longer a shared ancestor of PawWork's dev branch, so `git merge` from upstream is physically blocked by no-common-ancestor regardless of attributes. The driver only fires for intra-PawWork 3-way merges, where it silently drops dev's PawWork-internal changes when feature branches pull dev forward (most recently lost #765's detectSubmitOwnership additions from PR B's dev merge, leaving #765's new tests in the worktree without the symbols they import). The carve-out goal — preserve PawWork UI when re-anchoring on upstream opencode — now lives wherever the next anchor sync ends up: a one-off checkout of the listed paths back to HEAD, reviewed as part of that intentional event. No silent path-based override remains. - .gitattributes: drop the pawwork-keep-ours block (LF pinning kept) - packages/ui/script/verify-merge-driver.sh: delete (validates a driver that no longer exists)
Resolved SDK gen conflicts by regenerating from the merged OpenAPI source. SDK now exposes: - externalResult.list (from PR B) - RateLimit, command-inline, draft-isolation surfaces from dev Legacy question/blocker routes stay deleted (PR B's intent preserved). typecheck + bun test pass on app/opencode/core/ui/sdk.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Third external review came back as mergeable — no P0/P1. Two non-blocking items, both noted as "do not block this PR" by the reviewer themselves. Acknowledged without code changes; reasoning below. P2 — abort vs This is the same race surfaced in Round 1 and disclosed in this PR's Risk Notes ("A pre-existing race condition in PR A's PR B does not change PR A's P3 — The current 3 × 50ms retry already covers the observed register→updateToolCall flush race. The remaining "exceeds 150ms" path is theoretical: PawWork DB writes flush sub-millisecond in practice, and no test has observed a miss after the retry. If the race ever exceeds the window, the entry stays in Adding a warn for a never-fired path adds noise without observability value; if we ever see this in practice we'd add a counter and longer backoff together. Closing the suggestion without changes. |
PR A wired the externalResult abort handler via run.promise(failIfPending), which posts the registry tombstone behind a microtask. A concurrently scheduled /tool/respond ran its own Effect through the same microtask queue, and its Effect.gen tick committed Deferred.succeed before the abort Effect could mark the entry resolved. Net effect: a question the user intended to abort completed with their last-typed answer. Closed the race in two places: - Add `ExternalResult.abortPendingSync` (synchronous transition of the Map entry pending → resolved tombstone, returning the Deferred so the caller can schedule Deferred.fail asynchronously). The registry now reflects the post-abort state the moment the AbortSignal fires, before any other Effect tick runs. - Reorder `entries.set` to land before the Deferred yield inside `resolveIfPending` and `failIfPending`. Either end of the race commits the tombstone synchronously on its Effect tick 1, so whichever Effect schedules first wins deterministically and the loser sees state==="resolved" on its own first read. `prompt.ts`'s abort handler switches from `run.promise(failIfPending(...))` to `abortPendingSync` + a follow-up `run.promise(Deferred.fail(deferred,...))`. The Deferred propagation is unchanged; only the registry transition was hoisted out of the microtask queue. 5 new tests cover: sync transition + returned Deferred identity, ok:false on missing/resolved entries, respond-after-abort returns already_resolved, Deferred.fail surfaces as aborted error to the awaiter, and the inverse race where respond commits the tombstone before a racing abort. Closes the "out of scope, follow-up" disclaimer in this PR's Risk Notes. 2829 pass / 0 fail across opencode tests.
|
Reopened P2 in scope after offline discussion (no PR C is planned, so the "follow-up" excuse no longer holds). Fixed in e255aa4. Root cause: the abort handler at Two ordering fixes, applied together:
5 new race tests in PR body Risk Notes section updated to drop the disclaimer. For P3: closed without code change. Rationale already documented in the prior comment — skipped entries stay in the registry, so the next hydrate cycle or live SSE restores the dock; adding a warn for a path that has not fired in tests is preventive noise without observability value. |
|
Third round review verdict acknowledged: mergeable, no P0/P1. Three non-blocking items addressed below. P2 — Declined. PlanExitTool is gated by The only path where the LLM can actually call Same reasoning as Stage 9 drop in the PR plan: don't write defensive code for an unreachable path. If PawWork ever enables plan mode for the desktop client, the gate and the selector get relaxed in the same PR. P2 — Status confirmation, not a finding. Removed intentionally in The carve-out's only intended scenario was upstream-opencode sync. PawWork's If we ever re-anchor on upstream-opencode in the future, the carve-out goal moves to a one-off P3 — Declined again (same call as the second-round response). The 3 × 50ms retry covers observed timings; PawWork DB writes flush sub-millisecond in practice. Skipped entries remain in |
1 participant
Summary
First-principles refactor of the question tool that collapses the parallel
Question.ask/Question.recoverstate machine into a single tool-call lifecycle driven byctx.externalResult(the primitive shipped in #764). The question tool now suspends on a Deferred, the dock submits viaPOST /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.externalResultbehindPAWWORK_QUESTION_TOOL_EXTERNAL_RESULTto 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 guardpackages/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; matchessessionPermissionRequestsemantics)packages/app/src/pages/layout.tsx— background question OS notification reattached tomessage.part.updated; dedup pruned on transition out ofrunning; suppression walks ancestors (matches the dock)packages/opencode/src/server/instance/external-result.ts— newGET /external-resultroute that joins each pendingctx.externalResultDeferred with its session and message+part snapshot. Brief retry covers the register / processor.updateToolCall race window.packages/app/src/context/global-sync/bootstrap.ts—hydratePendingExternalResultswrites 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 swallowpackages/opencode/src/session/prompt.ts—cancel()signature collapse (no moremode: "soft" | "hard"), and the externalResult abort handler now usesExternalResult.abortPendingSyncso the registry tombstone lands synchronously when the abort signal firespackages/opencode/src/tool/external-result.ts— newabortPendingSynchelper plus reordered tombstone-before-yield inresolveIfPending/failIfPendingso the pending → resolved transition wins races against any concurrently scheduled Effectpackages/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_RESULTis gone entirelypackages/app/e2e/session/session-composer-dock.spec.ts— five legacy-recovery tests deleted; surviving question tests drive the dock through the real tool runner viaseedSessionQuestionRisk Notes
/question,/blocker,/session/:id/question,/__e2e/ask, and/__e2e/publish-askedroutes are deleted. Any external client still calling them will 404. There is no known consumer outside this repo.session.abortno longer acceptsmode=soft. Callers that passedmode=softwill get a 400 from the query validator. Internal callers (and the SDK) are updated; the abort renderer diagnostic no longer carriesmode.How To Verify
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/replyto/session/:id/tool/respond.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation