From 0c27ecb17d591ddb1aead7bf0acc961635b4ae41 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 16 May 2026 01:29:56 +0900 Subject: [PATCH] docs(adr): write prompt-async-gate ADR Documents the reservation-based duplicate-injection guard introduced in v4.2.0. Covers context (Issue #4012 race window), decision (Symbol token, post-dispatch hold, dispatch timeout, shared runner, prefix-tightened release), consequences (caller-side release discipline, AST audit strengthens enforcement), and references. Closes M11 --- docs/reference/prompt-async-gate-rfc.md | 415 +++++++++--------------- 1 file changed, 147 insertions(+), 268 deletions(-) diff --git a/docs/reference/prompt-async-gate-rfc.md b/docs/reference/prompt-async-gate-rfc.md index cff01355c..1bfc888df 100644 --- a/docs/reference/prompt-async-gate-rfc.md +++ b/docs/reference/prompt-async-gate-rfc.md @@ -1,332 +1,211 @@ -# ADR: Prompt Async Gate +# ADR: prompt-async-gate - reservation-based duplicate-injection guard ## Status -Accepted for v4.2.0. - -This decision applies to every production route that sends an internal message -through an OpenCode session API. - -The mandated implementation is `src/shared/prompt-async-gate.ts`. - -The root `AGENTS.md` invariant named "Internal message injection is dangerous" -is the policy authority for this ADR. - -The static audit `src/shared/prompt-async-route-audit.test.ts` enforces the -production side of this decision. - -Route-specific tests must still prove behavior for each internal message path. +Proposed (introduced in v4.2.0) ## Context -Issue 4012 reported duplicate streaming output after internal message injection. +Issue #4012 reported duplicate streaming output after OMO injected an +internal message into a live OpenCode session. -The visible symptom was repeated assistant output in a live parent session. +The user-visible failure was two assistant bubbles streaming the same +continuation. -The underlying failure mode was a race between OpenCode session state and OMO -continuation hooks. +The root race was not one hook making one bad decision. Multiple internal +routes could observe the same idle, completion, or error edge and each decide +that the parent session needed a wake or recovery prompt. -OMO has several routes that can decide to wake or continue a session: +The most important race window was: -- background task completion notifications +1. OpenCode emitted a `session.idle` event. +2. OMO started an `isSessionActive` HTTP poll. +3. OpenCode was still pacing the streaming animation for the previous answer. +4. The poll observed an inactive or idle-looking session. +5. OMO injected a continuation prompt. +6. A second hook observed the same edge and injected again. +7. The user saw two assistant bubbles. + +The historical race site was visible in the built bundle at +`dist/index.js:69665-69680`. That code checked session activity before sending +an internal prompt, but the check and the prompt were not protected by a +shared reservation. + +OpenCode's `prompt_async` route contributed to the failure mode because it has +fire-and-forget semantics. `session.promptAsync` can resolve before the prompt +is durably accepted by the target session. A later `session.error` event can +still arrive for the same attempt, so the caller can believe dispatch finished +while a recovery hook still treats the session as eligible for retry. + +OMO has 13+ internal hook callers that can inject prompts, including: + +- background task parent wakes - runtime fallback retries -- team mailbox delivery -- recovery continuations -- CLI run resume paths -- Claude Code hook delivery -- sync and background subagent prompts +- model suggestion retries +- team mailbox live delivery +- session recovery continuations +- todo continuation resumes +- CLI run resumes +- Claude Code hook injections +- sync subagent prompts +- background subagent prompts -These routes can observe the same idle, completion, or error edge. +Route-local guards cannot close this race. Each route can be correct in +isolation and still collide with another route in the same process. -Without a shared gate, two routes can dispatch the same internal prompt into the -same parent session. - -OpenCode also exposes a subtle durability gap. - -`session.promptAsync` can return before the prompt is durably accepted by the -target session. - -A later `session.error` event can still arrive for the same attempt. - -That means a route can think it finished while another hook still sees the -session as eligible for recovery. - -The old pattern was unsafe: - -```ts -await client.session.promptAsync({ - path: { id: sessionID }, - body: { text: message }, -}) -``` - -The unsafe properties were: - -1. No per-session reservation before dispatch. -2. No shared active-session check. -3. No post-dispatch hold for late failures. -4. No timeout around a hung dispatch. -5. No central log trail for skipped or failed dispatches. -6. No static audit that could block new raw prompt routes. - -Local guards inside each feature were not enough. - -Different hooks can run in the same process and see different snapshots of -session state. - -They need one shared reservation map keyed by session ID. - -The root `AGENTS.md` now states the invariant: - -```text -Treat every session.prompt / session.promptAsync call as a write to shared -session state. Production code may call them only inside -src/shared/prompt-async-gate.ts. -``` - -This ADR records the architecture behind that invariant. +The root `AGENTS.md` now records the governing invariant in the section +"Internal message injection is dangerous": production code may call +`session.prompt` or `session.promptAsync` only inside +`src/shared/prompt-async-gate.ts`. Every other route must use the shared gate. ## Decision -All production internal message injection must go through -`src/shared/prompt-async-gate.ts`. +Create `src/shared/prompt-async-gate.ts` as the single production owner of raw +OpenCode prompt dispatch. -The module exports two gate functions: +The gate coordinates callers with a module-global reservation map: ```ts -export async function promptAsyncAfterSessionIdle(args: { - client: PromptAsyncClient - sessionID: string - input: TInput - source: string - settleMs?: number - postDispatchHoldMs?: number - dispatchTimeoutMs?: number - checkStatus?: boolean -}): Promise - -export async function promptAfterSessionIdle(args: { - client: PromptClient - sessionID: string - input: TInput - source: string - settleMs?: number - postDispatchHoldMs?: number - dispatchTimeoutMs?: number - checkStatus?: boolean -}): Promise +const reservations = new Map() ``` -The gate returns a discriminated result instead of throwing for expected races: +The map is keyed by `sessionID`. A reservation records the source that claimed +the session, an expiration time, and a `Symbol(source)` token. The token gives +each reservation identity beyond its text source. -```ts -export type PromptAsyncGateResult = - | { status: "dispatched"; response: unknown } - | { status: "active" } - | { status: "reserved"; reservedBy: string } - | { status: "unavailable" } - | { status: "failed"; error: unknown } -``` - -Callers must treat `active` and `reserved` as successful suppression. - -They mean another actor owns the session or the user is already active. - -They are not retry signals by default. - -Every call must provide a stable `source` string. - -The source identifies the route that reserved the session. - -Recommended source format: +Every caller supplies a stable `source` string such as: ```ts const source = `background-agent:${taskID}` ``` -The reservation flow is: +The shared flow is: 1. Prune expired reservations. -2. Reject if the session already has an active reservation. -3. Reserve the session before waiting or dispatching. -4. Wait for idle settle time. -5. Check current session status unless the caller opted out for a proven reason. -6. Dispatch through `session.promptAsync` or `session.prompt`. -7. Keep a short post-dispatch hold after an attempted dispatch. -8. Release only after the hold expires or through an intentional recovery path. +2. Reserve the session before waiting or dispatching. +3. Wait for the idle settle period. +4. Poll session activity unless the route has a proven opt-out. +5. Dispatch through the selected OpenCode prompt API. +6. Keep the reservation during the post-dispatch hold. +7. Release after the hold or through an explicit recovery path. -The default timing constants are part of the decision: +The reservation is taken before the activity poll so that two hooks cannot both +enter the poll-dispatch window. + +The default post-dispatch hold is exported as: ```ts export const DEFAULT_PROMPT_ASYNC_POST_DISPATCH_HOLD_MS = 250 +``` + +`postDispatchHoldMs` defaults to 250 ms. The gate holds the reservation briefly +after the dispatch attempt even when dispatch throws synchronously or returns a +failed result. This closes the AGENTS.md hazard where `promptAsync` returns +before durable acceptance and a late OpenCode error races with retry logic. + +The default dispatch timeout is 30 seconds: + +```ts export const DEFAULT_PROMPT_DISPATCH_TIMEOUT_MS = 30_000 ``` -The post-dispatch hold is required because `promptAsync` returning does not prove -that all related OpenCode events have drained. +`dispatchTimeoutMs` wraps the underlying `session.promptAsync` or +`session.prompt` call with `Promise.race`. A hung OpenCode API call must fail +closed instead of holding a reservation forever. -The dispatch timeout is required because a stuck OpenCode API call must not hold -the reservation forever. - -The timeout is a circuit breaker, not a synchronization primitive. - -Callers must not set `postDispatchHoldMs: 0`. - -The static audit rejects that pattern. - -If a caller needs custom behavior, it must add a route-specific regression test -that proves duplicate dispatch cannot occur. - -The gate owns the raw prompt calls: +Both public gate helpers delegate to one internal runner: ```ts -const promptAsync = client.session?.promptAsync - -if (typeof promptAsync !== "function") { - return { status: "unavailable" } -} - -return dispatchAfterSessionIdle({ - sessionName: "promptAsync", - client, - sessionID, - input, - source, - settleMs, - postDispatchHoldMs, - dispatchTimeoutMs, - checkStatus: args.checkStatus !== false, - dispatch: (dispatchInput) => promptAsync(dispatchInput), -}) +dispatchAfterSessionIdle(args) ``` -Production code outside this module must not access these APIs directly: +`promptAsyncAfterSessionIdle` passes a `session.promptAsync` dispatcher. +`promptAfterSessionIdle` passes a `session.prompt` dispatcher. Sharing the +runner keeps reservation, hold, timeout, logging, and active-session behavior +identical for async and sync prompt routes. + +The public gate result is a discriminated union. Callers must treat `active` +and `reserved` as successful suppression, not automatic retry signals. A route +that changed optimistic task or loop state before dispatch owns restoring that +state when the gate returns `failed`, `unavailable`, or a skipped status that +requires rollback. + +The gate exposes `releasePromptAsyncReservation` for intentional recovery +paths. Prefix release is deliberately tight: ```ts -client.session.prompt(...) -client.session.promptAsync(...) -client["session"]["promptAsync"](...) -const { promptAsync } = client.session -Reflect.apply(client.session.promptAsync, client.session, [input]) -``` - -Type guards may check that `promptAsync` exists when the eventual dispatch still -routes through the shared gate. - -The allowlist in the audit must stay small and justified. - -The gate also exposes reservation release helpers for intentional recovery: - -```ts -releasePromptAsyncReservation(sessionID, { - reservedBy: "model-suggestion-retry", -}) - releasePromptAsyncReservation(sessionID, { reservedByPrefix: "runtime-fallback:", }) ``` -Prefix release is allowed only for prefixes that end with `:`. +`reservedByPrefix` must end in `:`. This prevents broad releases such as +`runtime` matching unrelated sources. Exact source release remains available +for callers that know the full reservation source. -This prevents broad accidental releases such as `runtime` matching unrelated -sources. - -Release helpers exist for rollback and retry flows. - -They must not be used as a normal cleanup path after dispatch. +Raw prompt calls outside the gate are blocked by +`src/shared/prompt-async-route-audit.test.ts`. The audit uses the TypeScript +Compiler API rather than regex so it catches destructuring, bracket access, +optional chaining, and aliased or cast access patterns. ## Consequences -Positive consequences: +### Positive -- Duplicate internal dispatches collapse to one reservation winner. -- Late `session.error` events no longer trigger immediate duplicate retries. -- Internal message routes share logging and result semantics. -- Tests can reason about a single gate instead of many ad hoc guards. -- New raw prompt routes are blocked by a static audit. -- Retry flows can release only their own reservation source. -- Hung dispatches fail closed through a timeout. +- Duplicate internal prompt injection now has one reservation winner per + session. +- The post-dispatch hold closes the AGENTS.md "returns before durably + accepted" hazard even when dispatch errors synchronously. +- Dispatch timeout prevents a stuck OpenCode call from holding the gate forever. +- 13+ internal hook callers share one result model and one safety primitive. +- The AST-based audit from HIGH-5 catches more bypass shapes than the prior + regex audit. +- Route-specific tests can focus on route behavior while the shared gate tests + reservation semantics. -Negative consequences: +### Negative -- Internal prompt injection has a small default latency from idle settling. -- A post-dispatch hold can delay a legitimate retry by 250 ms. -- Callers must handle `PromptAsyncGateResult` instead of assuming dispatch. -- Tests that mock session APIs may need to model reservation state. -- Any new route must add route-specific duplicate-injection coverage. +- Caller-side retry logic that releases and retries must call + `releasePromptAsyncReservation` explicitly when the original prompt did not + durably reach the server. `src/shared/model-suggestion-retry.ts` is the + reference case. +- 13+ wiring sites each need to be conscious of the gate result. Treating + `reserved` as a failure can create noisy retries. +- A valid retry can be delayed by the default 250 ms post-dispatch hold. +- The reservation map is process-local. It protects OMO hooks in the current + plugin process, not every possible OpenCode process. -Operational consequences: +### Migration -- CI green is not enough for race fixes tied to issue 4012. -- Maintainers must re-run the documented reproducer against the fix commit. -- Logs containing `[prompt-async-gate]` are the first place to inspect when a - wake, retry, or recovery message does not appear. +Existing `session.prompt` and `session.promptAsync` callers must route through +`promptAfterSessionIdle` or `promptAsyncAfterSessionIdle`. -Testing consequences: +The AST-based audit fails CI if a raw prompt call is added without an allowlist +entry. Any allowlist entry must explain why the raw access is not a dispatch +route or why it is still gate-routed. -- `src/shared/prompt-async-gate.test.ts` covers gate behavior. -- `src/shared/prompt-async-route-audit.test.ts` blocks raw production prompt - routes. -- Route owners must add regression tests for the specific trigger they wire. -- Tests must not rely on sleeping to wait for the post-dispatch hold. +New internal message routes must include duplicate-injection regression tests +for their trigger. Static policy alone is not enough. -Design constraints that remain open: +### Future work -- The reservation map is process-local. -- Cross-process OpenCode sessions still rely on the session API and event stream. -- The gate does not deduplicate different semantic prompts for the same session. -- The gate prevents concurrent injection, not incorrect caller intent. - -Rejected alternatives: - -1. Keep route-local guards. - - This failed because hooks observe the same edge from different modules. - -2. Disable recovery on any recent prompt event. - - This would hide valid recovery paths and lose task state. - -3. Use a global fixed delay after every dispatch. - - A delay without a reservation does not prevent another route from entering. - -4. Treat `promptAsync` success as durable acceptance. - - Issue 4012 showed that later OpenCode errors can still arrive. - -5. Allow raw prompt calls with code review discipline. - - The risk is architectural, so the invariant needs an automated audit. - -Migration rule: - -```ts -const result = await promptAsyncAfterSessionIdle({ - client, - sessionID, - input, - source: "runtime-fallback:retry", -}) - -if (result.status === "failed") { - restoreOptimisticState() -} -``` - -The caller owns any optimistic task or loop state it changed before dispatch. - -If dispatch is skipped, unavailable, or failed, the caller must restore state -when needed. +- Replace prefix-tightened release with full Symbol-token-based release + ownership. This is the HIGH-7 deferred work. +- Define same-source concurrent caller handling. Some routes may need collapse + semantics by source rather than by session only. +- Add dispatch metrics for observability, including reservation win, reserved + skip, active skip, timeout, and failed dispatch counts. +- Consider cross-process coordination if OpenCode exposes a durable session + lock or idempotency key. ## References -- Issue 4012: https://github.com/code-yeongyu/oh-my-openagent/issues/4012 -- Introduction PR 4034: https://github.com/code-yeongyu/oh-my-openagent/pull/4034 -- Hardening commit: `b333a5280` `fix(prompt-async-gate): add dispatch timeout, shared runner, harden prefix release` -- Test commit: `f93d7297c` `test(prompt-async-gate): cover dispatch timeout and post-dispatch error hold` -- Retry release commit: `ff1b15d53` `fix(model-suggestion-retry): release reservation before retry attempt` -- Root invariant: `AGENTS.md`, section `Internal message injection is dangerous` -- Implementation: `src/shared/prompt-async-gate.ts` -- Static audit: `src/shared/prompt-async-route-audit.test.ts` +- Issue #4012: duplicate streaming output and two assistant bubbles. +- PR #4034: introduction of `prompt-async-gate`. +- PR #3866 -> PR #4053: schema-compatible synthetic tool results for + post-compaction recovery, related to safe recovery dispatch. +- Root `AGENTS.md`: section "Internal message injection is dangerous". +- `.sisyphus/rules/test-discipline.md`: forbids `setTimeout(resolve, N)` and + `await sleep(N)` in tests unless time itself is the system under test. +- Implementation: `src/shared/prompt-async-gate.ts`. +- Audit: `src/shared/prompt-async-route-audit.test.ts`.