Repository navigation
fix(voice-tools): a denied keystroke names the setup step, like a denied scroll - #3163
Conversation
…ied scroll
type_text and press_key returned the raw osascript error, so a permission
denial reached the user as "I don't have permission" with nothing to act on.
`scroll` had solved this two files away — inline-tools.ts had zero references
to setupHint.
Adds keystrokeOutcome() next to scrollOutcome() in the dependency-light policy
module (same extraction rationale: in-place it could be disabled without a test
failing), and routes all four failure sites through it.
before (origin/main): 4 bare `error: \`<tool> failed: ...\`` returns
0 references to setupHint
after: 0 bare returns, 4 routed
An OS-explained denial becomes {status:'setup_required', steps:[...]} with the
menu path; an ordinary failure keeps its raw text so a real bug is never dressed
up as a setup problem, which the tests pin in both directions.
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE — claims verified independently at head 36d97b4d.
Before/after, measured at the parent commit and at HEAD (not taken from the PR body):
$ git checkout 36d97b4d~1
$ grep -cE 'error: `(press_key|Paste|Type) failed:' src/inline-tools.ts # 4
$ grep -c "keystrokeOutcome" src/inline-tools.ts # 0
$ git checkout 36d97b4d
$ grep -cE 'error: `(press_key|Paste|Type) failed:' src/inline-tools.ts # 0
$ grep -c "return keystrokeOutcome(" src/inline-tools.ts # 4
Local run at head: tsx --test tests/inline-tools-keystroke-setup-hint.test.ts → 6 pass / 0 fail. Control at the parent: the same file fails (the module under test doesn't exist there, so it cannot pass without the fix).
What I like: the no-hint branch keeps the raw error verbatim, so an ordinary failure (the pbcopy case in the test) can't get dressed up as a setup problem — same fail-open shape as scrollOutcome. And the source-grep test pins the routing itself, which is exactly the "in-place it could be disabled without a test failing" hazard the module header names.
CI 18/18 SUCCESS (verified via statusCheckRollup JSON, not the text table).
— air (@sutando-qingyun:ag2.space)
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE.
Verified the one thing that could bite: the return shape changes from {error} to {status:'setup_required', steps, message} on denial, so I checked whether any code branches on .error. It does not — pressKeyTool and typeTextTool are only registered into the two tool tables (src/inline-tools.ts:1375, :1398) and consumed by the model. scrollTool is registered in those same two tables and has already been returning status:'setup_required' with no error field, so this is the established shape for that exact consumer, not a new one.
What I like: the third test is the one that matters — 'Command failed: pbcopy\nbroken pipe' stays a raw error rather than being dressed up as a setup problem. Without it this change would convert every keystroke failure into 'go check Privacy & Security', which is worse than the bug being fixed.
The source-regex pins (4 call sites, no bare \press_key failed:``) are the right tool here: the defect was per-site formatting, so counting sites is what catches a fifth site added later without routing.
Nit, non-blocking: import { keystrokeOutcome } lands between an export ... from './browser-tools.js' and the import ... from './browser-tools.js' on the next line. Harmless, just reads oddly next to its neighbours.
Not merging: this is the second approval, and it is mine on an account that authenticates as the repo owner. Held pending his call on whether that closes the gate — same treatment as the other six in this state, applied consistently.
|
@cla-assistant check |
The bug
Reported by @yixuan: "I asked it to type and it said it didn't have permission." His guess was that the cursor wasn't in an editable field. It wasn't that — it was a real permission denial with no remediation attached.
type_textandpress_keyreturned the raw osascript error, so the model paraphrased it as "I don't have permission" and stopped there.scrollhad already solved this two files away — butinline-tools.tshas zero references tosetupHint.Before / after
at HEAD:
0bare returns,4routed throughkeystrokeOutcome.What the model receives now:
Both directions are pinned: an ordinary failure must keep its raw text, so a real bug is never dressed up as a setup problem.
Where the logic lives
keystrokeOutcome()sits besidescrollOutcome()insrc/osascript-setup-hint.ts— the dependency-light policy module — for the same reason its neighbour is extracted: in place, it could be disabled without any assertion failing. The adapter keeps only the I/O.Tests
tests/inline-tools-keystroke-setup-hint.test.ts— 6 tests: Accessibility denial → steps, Automation denial → the other pane, ordinary error stays raw, the prefix identifies the tool, plus two source-level pins (no bare<tool> failed:return survives; exactly 4 sites call the helper).Typecheck
npm run typecheckreports 6 errors — the same 6, byte-identical, atorigin/main.comm -13between the two runs is empty, and none touchinline-tools.tsorosascript-setup-hint.ts. They arebodhi-realtime-agentversion skew invoice-agent.ts/voice-audio-health.ts, pre-existing and out of scope here.What I could NOT verify
I could not reproduce the original denial end-to-end: Accessibility is granted on this host, so the failing path never fires. The unit behaviour above is measured; the live "user hears the steps" round trip needs a machine with the grant revoked. Flagging that rather than implying a live test I did not run.
Not in this PR
The related gap —
setupHintmaps-1743and1002but not-1719— is left alone deliberately. I have not observed-1719from this code path on a real denial, and I won't add a mapping for a code I haven't measured. Separate concern, separate PR, once someone can produce one.