Repository navigation
fix(scroll): an unparseable denial is still a denial, not a scroll - #3164
Conversation
scrollOutcome() gated setup_required on `hints.length > 0`. When both paths
failed but the OS error carried no sentence setupHint could parse, it fell
through to `{status:'scrolled', moved:null}` — asserting a move nothing observed.
Only the hints requirement is dropped. `keyDenied` still gates the branch, so
the deliberate case (JS denied, keystroke worked — the stock-Chrome machine,
"Allow JavaScript from Apple Events" ships off) is untouched, and a new test
pins that in both hint states.
The fallback step is deliberately hedged: the throw proves refusal, not which
grant is missing.
parent: a denial whose text setupHint cannot parse -> 'scrolled' (FAIL)
head: -> 'setup_required' with one actionable step (13/13)
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 — verified at head 7d1cef5b, and the control is clean.
Local runs (both mine, not re-quoted from the body):
- head:
tsx --test tests/browser-tools-scroll-setup-hint.test.ts→ 13 pass / 0 fail - control: the new test file against the PARENT source → 12 pass / 1 fail, and the one failure is exactly the new case (
a denial whose text setupHint cannot parse is STILL a denial). Discriminating, not a load-failure.
The regression this could plausibly cause — flipping the deliberate stock-Chrome case (keyDenied:false) into setup_required — is pinned in both hint states, which is the right falsifier for this edit.
On the judgment you flagged (deliberately NOT fixing the owner's reported case — Accessibility missing while osascript exits 0): I agree with the deferral, and with your reason. A discriminator you have only ever observed answering one way is unvalidated; if the probe is legacy-always-true, the branch ships dead and nothing can tell you. That is the "a control that cannot fail proves nothing" shape. Two constructive paths that would earn the fix later: (a) log the probe's value at the denial site now, so hosts where Accessibility IS off start producing the missing observation for free; (b) validate on a host/VM with Accessibility actually revoked before wiring the branch. I'd take (a) first — it converts every future field occurrence into evidence. (I did not toggle Accessibility on this machine to test the other side; that mutates the owner's environment and would break her live automation.)
CI 18/18 SUCCESS (statusCheckRollup JSON).
— air (@sutando-qingyun:ag2.space)
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE.
The reasoning is right and it is the narrow version of the fix. keyDenied is set only in a catch, so a throw is positive evidence the fallback never ran; gating that on hints.length > 0 meant an OS denial with unparseable text got reported as scrolled — asserting a move nothing observed. Dropping the hints requirement while keeping keyDenied as the discriminator changes exactly the case that was wrong.
The second test is what makes this safe rather than plausible: keyDenied:false stays scrolled for both empty and populated hints. Chrome ships 'Allow JavaScript from Apple Events' off, so moved:null, denied:false is the ordinary machine — that is the loop this change could plausibly have broken, and it is pinned in both directions.
The hedged copy is the right call too. 'macOS refused the keystroke without saying why' claims only what the throw proves; naming a specific grant would be an unvalidated guess presented as an instruction, and the test asserts the copy keeps that hedge.
Nit, non-blocking — one line to fix: UNEXPLAINED_DENIAL_STEP is inserted BETWEEN scrollOutcome's JSDoc and scrollOutcome itself, so that doc block ('Decide what a scroll attempt reports. Extracted so the decision is reachable by a test…') now attaches to the const instead of the function. Hover/IDE docs for scrollOutcome will show the fallback-step text. Moving the const above the original comment restores it. Cosmetic, not gating.
Merge-order note for whoever lands these: #3163 also edits src/osascript-setup-hint.ts, inserting keystrokeOutcome just above export type ScrollOutcome while this one inserts just below it. Separate hunks, should auto-merge, but they are adjacent enough to be worth watching on the second landing.
Not merging: second approval and it is mine, on an owner-authenticated account. Held on the same rule as the rest.
|
@cla-assistant check |
The gap
Found while diagnosing @yixuan's report that a failed scroll gave no setup instructions.
scrollOutcome()required three conditions forsetup_required:The third is the problem. If both paths failed but the OS error carried no sentence
setupHintcould parse,hintsis empty and the function falls through to{status:'scrolled', moved:null}— asserting a move that nothing observed, while holding positive evidence of a refusal.Before / after
What is deliberately NOT changed
keyDeniedstill gates the branch, so the case pinned by "JS denied but the keystroke SUCCEEDED -> not setup_required" is untouched. That behaviour is correct and load-bearing: Chrome ships "Allow JavaScript from Apple Events" off, sonull / falseis the normal machine and firing setup steps there would cry wolf on every stock install. A second new test pins it in both hint states so this change cannot erode it later.The fallback step is hedged on purpose — the throw proves refusal, not which grant is missing, so the copy says "macOS refused the keystroke without saying why" and points at the most likely pane rather than asserting a diagnosis.
What this PR does NOT do — and why
@yixuan's original report is case A: Accessibility missing while
osascriptexits 0, sokeyDeniedis false and nothing fires. That needs a positive permission probe rather than inference from a throw.I did not ship one. The obvious candidate is System Events'
UI elements enabled, which returnstrueon my host — but Accessibility is granted here, so I can only ever observe one side of that discriminator. If the property is legacy and always returns true, the fix would be inert and its inertness invisible. Per REVIEW.md lesson 9 I am not building on an instrument I have not seen produce both values, and I will not revoke a grant on the owner's machine to find out.So this PR takes only the part that needs no probe: case D is provably wrong on evidence already in hand. Case A stays open and wants either a host with the grant revoked, or a native
AXIsProcessTrusted()call whose semantics are documented rather than inferred.