fix(ask): a wizard_ask timeout is not a decline - #1233
Conversation
A timeout and a dismissal both resolved every field to __cancelled__, and the tool description told the agent to read either as "the user declined". The flows that raise askTimeoutMs park on a question while the user runs a production build, triggers a test error, and checks Error Tracking. When that work outlasts the timer, the agent concluded the user had said no and unwound the demo code the user was still verifying. A timeout now answers with its own sentinel, __timed_out__, and the tool result carries shared guidance: keep waiting by asking again, and never revert a change already applied. An unanswered ask was already refunded on both caps, so re-asking is free. Also replaces the analytics `timed_out` proxy (duration >= timeout) with the actual race outcome. Generated-By: PostHog Desktop Task-Id: 3bcaf0e1-9c4f-4f30-b9a7-f897885bb1d2
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 1 should fix, 1 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 left for you |
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| 'Ask the same question again to keep waiting; a timed-out call costs nothing ' + | ||
| 'against your call budget. If a later ask comes back as "__cancelled__", the ' + | ||
| 'user dismissed it and you may fall back. If two more asks time out, finish the ' + | ||
| 'run, leave every change in place, and report the step as unverified.'; |
There was a problem hiding this comment.
Timeout retries have no enforced limit
Why we think it's a valid issue
- Checked:
ASK_TIMED_OUT_NOTEatsrc/lib/wizard-tools/tools.ts:406-415, the cap policyevaluateAskCapattools.ts:488-522, the accounting increateAskAccountingattools.ts:555-596, both facade refund paths, the configured timeout values, and every turn or wall-clock bound in the repo. - Found: The premise holds on both caps.
refundrolls back the run total and the per-subject run (tools.ts:581-587), and both facades call it wheneverisFullyCancelled(answers)is true —src/lib/wizard-tools/mcp.ts:756-758andsrc/lib/agent/runner/harness/pi/tools.ts:388.isFullyCancelledaccepts either sentinel (src/lib/wizard-ask-bridge.ts:114-118), so a fully timed-out ask costs nothing onmaxQuestionsor on the adjacency run. - Found: The adjacency guard cannot catch a loop either.
adjacencyNudgedlatches to true the first time it fires (tools.ts:568-571), andevaluateAskCaponly nudges when that flag is false (tools.ts:503), so it fires at most once per run. - Found: No host-side turn or duration bound exists on the runs that mount this tool.
maxTurnsappears exactly once in the repo, atsrc/lib/agent/mcp-prompt-streaming.ts:297— a separate one-off prompt surface, not the pi or anthropic wizard run. Neither harness sets a turn cap or a run deadline. The "stop after two more" limit attools.ts:414-415is prose only. - Found: The wait per iteration is long.
askTimeoutMsis 30 minutes in both programs this PR targets (src/lib/programs/error-tracking-upload-source-maps/index.ts:99,src/lib/programs/self-driving/index.ts:74). - Impact: Confirmed as an unenforced limit: the change converts a wait the timer used to bound into one bounded only by the agent obeying the note. The worst outcome is a local CLI process idling in the user's own terminal at one LLM turn per 30 minutes. No data loss, no corruption, no server-side resource held, and two escapes stay open — Esc dismisses the overlay and returns
__cancelled__, which the same text tells the agent to treat as a decline, and Ctrl-C ends the run. - Priority: Lowered to
consider. Triggering the loop needs an agent that follows "ask the same question again" while ignoring "if two more asks time out, finish the run" from the same paragraph, so the risk is symmetric with the feature working at all. The suggested fix adds per-subject retry state, a terminal result, ask rejection, and tests on both facades — a large mechanism for a bounded, user-escapable idle. Real and worth recording, not worth blocking on.
Issue description
ASK_TIMED_OUT_NOTE says to stop after two more timeouts, but no code enforces this limit. Timeout calls are refunded from both existing caps. An agent can repeat 30-minute waits without a hard bound. This can keep the CLI and agent session alive indefinitely.
Suggested fix
Track timeout retries per subject outside the refunded call cap. After three timeouts, return a terminal unverified result and reject further identical asks. Test both facades against this host-side limit.
Prompt to fix with AI (copy-paste)
## Context
@src/lib/wizard-tools/tools.ts#L412-415
<issue_description>
`ASK_TIMED_OUT_NOTE` says to stop after two more timeouts, but no code enforces this limit. Timeout calls are refunded from both existing caps. An agent can repeat 30-minute waits without a hard bound. This can keep the CLI and agent session alive indefinitely.
</issue_description>
<issue_validation>
- **Checked:** `ASK_TIMED_OUT_NOTE` at `src/lib/wizard-tools/tools.ts:406-415`, the cap policy `evaluateAskCap` at `tools.ts:488-522`, the accounting in `createAskAccounting` at `tools.ts:555-596`, both facade refund paths, the configured timeout values, and every turn or wall-clock bound in the repo.
- **Found:** The premise holds on both caps. `refund` rolls back the run total and the per-subject run (`tools.ts:581-587`), and both facades call it whenever `isFullyCancelled(answers)` is true — `src/lib/wizard-tools/mcp.ts:756-758` and `src/lib/agent/runner/harness/pi/tools.ts:388`. `isFullyCancelled` accepts either sentinel (`src/lib/wizard-ask-bridge.ts:114-118`), so a fully timed-out ask costs nothing on `maxQuestions` or on the adjacency run.
- **Found:** The adjacency guard cannot catch a loop either. `adjacencyNudged` latches to true the first time it fires (`tools.ts:568-571`), and `evaluateAskCap` only nudges when that flag is false (`tools.ts:503`), so it fires at most once per run.
- **Found:** No host-side turn or duration bound exists on the runs that mount this tool. `maxTurns` appears exactly once in the repo, at `src/lib/agent/mcp-prompt-streaming.ts:297` — a separate one-off prompt surface, not the pi or anthropic wizard run. Neither harness sets a turn cap or a run deadline. The "stop after two more" limit at `tools.ts:414-415` is prose only.
- **Found:** The wait per iteration is long. `askTimeoutMs` is 30 minutes in both programs this PR targets (`src/lib/programs/error-tracking-upload-source-maps/index.ts:99`, `src/lib/programs/self-driving/index.ts:74`).
- **Impact:** Confirmed as an unenforced limit: the change converts a wait the timer used to bound into one bounded only by the agent obeying the note. The worst outcome is a local CLI process idling in the user's own terminal at one LLM turn per 30 minutes. No data loss, no corruption, no server-side resource held, and two escapes stay open — Esc dismisses the overlay and returns `__cancelled__`, which the same text tells the agent to treat as a decline, and Ctrl-C ends the run.
- **Priority:** Lowered to `consider`. Triggering the loop needs an agent that follows "ask the same question again" while ignoring "if two more asks time out, finish the run" from the same paragraph, so the risk is symmetric with the feature working at all. The suggested fix adds per-subject retry state, a terminal result, ask rejection, and tests on both facades — a large mechanism for a bounded, user-escapable idle. Real and worth recording, not worth blocking on.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track timeout retries per subject outside the refunded call cap. After three timeouts, return a terminal unverified result and reject further identical asks. Test both facades against this host-side limit.
</potential_solution>
There was a problem hiding this comment.
Escalating this one rather than implementing it — the gap is real, but choosing the shape of the bound needs product context I shouldn't settle unattended.
I verified all of it against the current code and everything holds: a timed-out ask is refunded on both caps, the adjacency nudge latches so it can only fire once per run, neither harness sets a turn or wall-clock bound, and a repo-wide search finds no timeout counter anywhere. The "stop after two more" limit really is prose only.
What makes this a decision rather than a mechanical fix is the scoping question, not the size. A timeout budget can be per-subject or per-run, a real answer or a dismissal may or may not reset it, and the third timeout can either return a terminal result or reject the next ask — those behave differently for real flows. This tool has already been burned by exactly that choice once: the batching guard had to be changed from run-wide to per-subject because the warehouse flow legitimately walks 5–8 sources one call at a time, and a run-wide count killed it. A run-wide timeout budget would reintroduce that failure against the flows most likely to park on a long wait, which is the opposite of what this PR is for.
So what a human needs to decide: is host-side enforcement wanted at all here, and if so, is the budget per subject or per run, and what resets it? My read is that it belongs with the follow-up the PR description already flags — the host-side guard that refuses deletions after a timeout. Both are the same question of whether the wizard defends against an agent that ignores the note, and deciding them together will produce a more coherent answer than bolting on a retry counter now.
One update worth knowing: the commandment fix on this PR (f5109d1, the separate thread about the no-re-ask conflict) now tells the agent to follow the guidance the timed-out result carries, where the commandment previously said nothing about the bound at all. That strengthens the prose side slightly. It is still prose, so it does not close what you're pointing at.
No code changed for this thread.
`WIZARD_COMMANDMENTS` is assembled first in every run and told the agent, without condition, not to call `wizard_ask` again for the same fields once it returns. A fully timed-out return is not an answer, so that prohibition contradicted the retry this PR's tool description and `ASK_TIMED_OUT_NOTE` depend on — the agent held both instructions in one context and could stop after the first timeout. Scope the commandment to what it was protecting against: real answers and `__cancelled__` still may not be re-asked; a return where every answer is `__timed_out__` must be. The bound on how long to keep waiting stays in `ASK_TIMED_OUT_NOTE` rather than being restated here. Generated-By: PostHog Desktop Task-Id: c201be22-071e-461e-93c9-7f84a2c7bc55
The timer callback cancelled the host overlay before resolving its own promise. On the real TUI path that is not a no-op: `cancelPendingQuestion` fills every field with `__cancelled__` and resolves the pending `showQuestion` promise synchronously, so the dismissal settled first, queued its `Promise.race` reaction first, and won. The bridge returned `__cancelled__`, `isFullyTimedOut` was false, and both facades dropped `unanswered_reason` and `ASK_TIMED_OUT_NOTE` — leaving a real timeout indistinguishable from a decline, which is the defect this all exists to fix. Resolve with the timeout sentinel first, then dismiss the overlay, so the cleanup can no longer overtake the answer. Contain a throwing `cancelQuestion` too: the answer is already settled by then, and an injected callback should not take the run down from inside a timer. The existing timeout test could not catch this — it paired a never-resolving `showQuestion` with a no-op `cancelQuestion`, so the store's synchronous resolve was never exercised. The new test models what the store actually does, and fails on the previous ordering. Generated-By: PostHog Desktop Task-Id: c201be22-071e-461e-93c9-7f84a2c7bc55
Problem
askTimeoutMs. When it does, the wizard reverts the demo code the user is in the middle of verifying, and the run ends as if the user had refused it.__cancelled__, so the agent could not tell "the user is still working" from "the user said no".Changes
TIMED_OUT_SENTINEL(__timed_out__) replaces__cancelled__when the timer wins the race. A dismissal is unchanged.ASK_TIMED_OUT_NOTE: the user is probably still working, ask again to keep waiting, and do not undo, revert, or delete anything already applied. Re-asking is already free — an unanswered ask is refunded on both the per-run and per-subject caps.formatAskResult, so the contract cannot drift between them.vaultSensitiveAnswersgeneralises its guard to both sentinels, so a timed-out secret field is still never vaulted.timed_outdimension now reports the real race outcome instead of theduration >= timeoutproxy.What the agent receives when the user walks off mid-verification:
{ "answers": { - "verified": "__cancelled__" + "verified": "__timed_out__" }, + "unanswered_reason": "timeout", + "note": "... This is a timeout, not a decline. ... Do NOT undo, revert, or + delete any change you already applied ... Ask the same question + again to keep waiting; a timed-out call costs nothing ..." }Note
Out of scope for this repository. The report's two dead-end verification surfaces —
RealtimeCheckIndicatorandsourceMapsStepLogic, plus the missing onboarding instrumentation — live inposthog/posthog, not here. Nothing in this repo renders them. This PR fixes only the wizard-side defect the report raises under ticket #71566.Test plan
pnpm build && pnpm test && pnpm lint— 2687 tests pass, 0 lint errors.showQuestionthat never resolves) through the actual piwizard_asktool. Before:{"answers":{"verified":"__cancelled__"}}and nothing else. After: the payload in the diff above. The host overlay is still dismissed on timeout in both cases, so a later ask is not rejected as a duplicate.TIMED_OUT_SENTINEL, and still refunds the slot;Agent context
askTimeoutMsagain, or dropping the timer. The timer bounds one wait and is what dismisses a stuck overlay so later asks are not rejected as duplicates; the destructive part was the ambiguous answer, so the fix is the answer.{answers, outcome}object from the bridge. It changes the signature at every call site and fixture for information the answers can carry themselves.isFully*predicates into one classification helper — it would touch three unrelatedisFullyCancelledcall sites for no behaviour change.Created with PostHog Desktop from this inbox report.