fix(tui): answer a tool approval prompt only once - #1349
Conversation
The approval queue resolves its head on every answer, so a second answer from an already-settled prompt consumed the next queued request and resolved it unseen - approving a tool the user was never shown. ToolConfirmation now answers at most once per request across all three paths into the slot (select, Escape, formatter-crash auto-cancel), and resets when a new toolCall arrives so a reused instance stays answerable. Mirrors the answeredRef guard in question-prompt.tsx. Follow-up to Nano-Collective#1159.
nc-review: nothing to raise@addyCooks — nothing to raise from the automated review. Adds a one-answer-per-request guard in ToolConfirmation that mirrors the established 🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with |
The coverage gate failed on a 0.01% drop. Two things were behind it, and both are in the tests rather than the guard. The reset test mounted a Harness with an `onCancel` it never called: it answered both requests with Enter, so the callback body was dead. It now answers the reused instance with Escape instead, which is the better test as well as the covered one — Escape is the path that was double-firing, so answering through it proves the reset re-opens every answer route rather than only the one the first request happened to use. The per-tool validator's catch had no test at all. A validator that throws now has one, and it pins the distinction the guard depends on: a formatter crash auto-cancels, a validation error must not, so the prompt stays up and the single allowed answer is still the user's to spend. tool-confirmation.tsx goes from 17 uncovered lines to 10 and from 92.58% to 96.02%; the spec is fully covered. Together the two files move from 442/459 to 562/572, so the branch now adds headroom rather than nudging the gate.
2 participants
Follow-up to #1159,
from @will-lamerton's review note on the merged PR.
Description
The approval queue resolves its head on every answer (
queueRef.current.shift()?.resolve(result)), and nothing ties a result to the request that was on screen. So a second answer from an already-settled prompt consumed the next queued request and resolved it with the previous answer, without that request ever being shown approving a tool the user was never asked about. Before #1159 the single-resolver code nulled the resolver, so a repeat answer was a no-op.It is not reachable today: the
key={toolCall.id}props added in #1159 unmount the prompt as the queue advances. That is the problem the queue's safety depends on twokeyprops in a different file, with nothing in the code saying so.ToolConfirmationnow answers at most once per request, covering all three paths into the slot (the select prompt, Escape, and the formatter-crash auto-cancel), and resets when a newtoolCallarrives so a reused instance stays answerable instead of being silently blocked. This mirrors theansweredRef+previousQuestionguard inquestion-prompt.tsxthe reason the question slot was already safe.One deliberate deviation: the review suggested the guard live in
useHandlerQueue. I put it in the component instead, because the hook's answer callbacks receive only a result with no request identity, so the hook cannot distinguish a repeat answer for request A from a legitimate answer for B. The component is where both double-fire windows actually are. Happy to move it if you would rather change the callback signature to carry identity.26 lines of source across one component, plus tests and a changeset.
Type of Change
Changeset
pnpm changeset) describing this change for the changelogTesting
Automated Tests
.spec.ts/tsxfilespnpm test:allcompletes successfully)Manual Testing
Checklist
No issue for this one: it came from your review note on #1159.
Happy to file one if you would prefer it tracked.