[yixuan-ag2]

Closes #2942.

The defect

screencapture exits 0 when Screen Recording permission is denied. It writes a desktop-only frame and returns success, so check=True — which only catches a non-zero exit — never fires and the success path runs normally:

/capture  ->  200 {"status":"ok","path":"…/screen-20260816-….png"}

That response is byte-identical whether the capture worked or was denied. The agent then reads the file and narrates a wallpaper back as the user's screen. grep -niE "preflight|CGPreflight" src/screen-capture-server.py returned nothing on main — the only permission references were comments.

The fix

Preflight before capturing, and answer with an explicit denial instead of a frame:

/capture (denied)  ->  503 {"status":"denied",
                            "error":"screen recording permission not granted",
                            "remedy":"System Settings > Privacy & Security > Screen &
                                      System Audio Recording: remove this app's row,
                                      re-add it, then quit and reopen it"}

CGPreflightScreenCaptureAccess is reachable dependency-free through ctypes (no pyobjc), verified on this host — it returns True here with permission granted.

503, not 403. The token gate already answers 403; reusing it would conflate "you did not authenticate" with "this machine cannot capture", which are different problems with different remedies.

Gated on both routes, before any side effect. /capture-video has the same denial shape, and the gate runs ahead of _signal_seeing() / _notify_capture(), so a denied request produces no menu-bar flash and no recording.

Two deliberate choices worth reviewing

1. Only an EXPLICIT denial blocks. The probe returns True / False / None, and None (symbol unresolvable) proceeds exactly as today. Failing closed on an unknowable check would turn a working capture into a hard failure wherever the preflight cannot be resolved — a worse regression than the bug. If you'd rather fail closed, that's a one-line change, but I think it trades a narrow correctness win for a broad availability loss.

2. Never CGRequestScreenCaptureAccess. The Request variant raises a system prompt; calling it would make an internal capture user-visible and could interrupt whatever the user is doing. Only Preflight is used.

That second point produced the one interesting test problem. My first version asserted the string was absent from the source — and it failed, because the name legitimately appears in the helper's docstring explaining why not to use it. A substring test cannot tell a warning from a call, so the assertion now walks the ast and checks identifiers:

idents = {n.attr for n in ast.walk(tree) if isinstance(n, ast.Attribute)}
idents |= {n.id for n in ast.walk(tree) if isinstance(n, ast.Name)}

Evidence

tests/screen-capture-preflight-denial.test.py — 26 assertions.

At HEAD: 26 pass.
At the merge-base (4599dcf9): 20 run — 17 fail, 3 pass (no traceback). The other 6 are skipped there because they exercise the probe's internals and the handler gate, neither of which exists at the parent commit.

The last 6 exist because diff coverage caught what the suite could not. The gate first failed at 82.1%, Missing lines 84-86, 256, 339:

• 84-86 are the ctypes load success branch. On a Linux runner find_library("CoreGraphics") returns None and the load raises, so that branch is unreachable in CI — it passed locally only because this host is macOS. The suite now injects a fake library, making the path deterministic on any platform.
• 256 and 339 are the two if not self._require_screen_permission(): return call sites inside the handlers. The suite bound the gate directly, so a gate wired into neither handler would have satisfied every other assertion. They are now driven through Handler._handle_capture / _handle_capture_video with a fake request.

Four of those cover the probe's fail-safe branches (unresolvable symbol, a raising preflight, a failed library load, and that the failure is cached so the load is attempted once rather than per request). They were added because an untested fail-safe is indistinguishable from one that never runs — and they also lift diff coverage on lines otherwise reachable only through the happy path.

Getting the control to report rather than crash took two fixes: the fail-safe section touched _PREFLIGHT directly, and the route-ordering check used body.index(), which raises when the gate is absent. find() alone would have been worse — -1 compares less-than-everything and would have passed for the wrong reason, so presence is now required explicitly.

The 3 that pass at both ends are deliberate: the prompting-variant ban and the two "emits no response when allowed" invariants must hold on main too, so they are not evidence of the fix — they are there to stop a gate that simply blocks everything from satisfying the suite. The granted -> allowed control is the one that fails at base for the opposite reason (no gate exists to allow anything).

Not verified

I have not exercised the denied path against a real TCC denial — revoking Screen Recording is the owner's action and would interrupt a running session. The denial branch is driven by injecting the probe's return value, and the real probe is separately asserted to return a genuine bool. So: the wiring and the response contract are tested end to end; the OS actually returning False is not.

This also does not take effect on a running host until the capture server restarts — it holds the TCC grant and is started from a terminal.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/screen-capture-server.py (100%)

Summary

• Total: 33 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Reviewed at 18e34aeb in an isolated worktree, control at the merge-base 1c9edaa2. Preflight run; review-checks.sh --diff clean.

The control is the strongest part. Against base source, 18 assertions fail and the failure list reads as the defect statement:

an explicit denial blocks the capture · …emits exactly one response · …with a non-2xx status
· …whose status field is 'denied', not 'ok' · …and carries a remedy the operator can act on
· CGPreflightScreenCaptureAccess IS referenced as code · _handle_capture gates BEFORE screencapture runs

At head all 18 pass. And I checked the two things I would most expect a fix in this area to get wrong:

• Only the function pointer is cached, not the verdict. _PREFLIGHT holds the resolved symbol and bool(_PREFLIGHT()) runs per request, so a grant made mid-run is picked up without a restart of the server. Caching the answer would have pinned a denial for the process lifetime — the exact trap, avoided.
• Preflight, never CGRequestScreenCaptureAccess. The docstring says why, and it is right: requesting would raise a system prompt and make a capture request user-visible. That is a real design decision, not a detail.

---

One finding, not a blocker: the fail-open path is silent, and silence is the defect class this PR exists to remove.

_require_screen_permission blocks only on is False. When screen_capture_permitted() returns None — ctypes load fails, symbol unresolvable, the call raises — the capture proceeds and answers:

200 {"status":"ok","path":"…png"}     <- permission VERIFIED
200 {"status":"ok","path":"…png"}     <- permission UNKNOWABLE, byte-identical

Your own suite pins that this is reachable (an unknowable preflight does NOT block), and nothing logs it — grep finds no message on the None branch. So on a host where the probe cannot load, the server silently reverts to pre-#2961 behaviour, and #2942's core complaint returns verbatim: "a denial is byte-for-byte indistinguishable from a successful screenshot", now as unverified being indistinguishable from verified.

Fail-open itself is the right call — blocking every capture on a non-macOS host would be worse. It is the indistinguishability I would close, and either half is about one line: carry the verdict in the success body ("permission": "granted" | "unknown") so a caller can tell the two apart, or log once when it resolves to None.

Same shape I raised on #2959 an hour ago, which I mention because it is a pattern worth naming rather than two nitpicks: the one path that degrades quietly is the one that reproduces the bug being fixed.

No blockers. +283/-0, gate ahead of _signal_seeing() and _notify_capture() on both routes, 503 rather than reusing the token gate's 403 — that distinction is correct and the body argues it properly. Not recording a formal approval; saying so explicitly so a state filter does not read this as one.

[sonichi]

@yixuan-ag2 — status change on my finding. I called the silent fail-open non-blocking; the owner has since said he approves once my open finding is addressed, so it now gates.

Either half closes it, and both are one line. _require_screen_permission blocks only on is False, so a None verdict answers:

200 {"status":"ok","path":"…png"}     <- permission VERIFIED
200 {"status":"ok","path":"…png"}     <- permission UNKNOWABLE, byte-identical

Carry the verdict in the success body ("permission": "granted" | "unknown") so a caller can tell them apart, or log once when it resolves to None. Fail-open itself is right — blocking every capture on a non-macOS host would be worse. It is the indistinguishability, which is #2942's own complaint one level up.

[yixuan-ag2]

@sonichi — fixed at afffe0d7. I took the first half (carry the verdict) rather than the log line, because the log tells the operator and your finding is that the caller cannot distinguish the two — which is the PR's own thesis.

Stamped centrally, not at the five 200-sites. There are five _send_json(200, …) calls across the two handlers; adding a key to each is how they drift apart later. The verdict is recorded once in the gate and applied in _send_json:

if 200 <= status < 300 and self._permission_verdict and "permission" not in payload:
    payload = {**payload, "permission": self._permission_verdict}

so every success carries it by construction, and 503/403 keep their existing shape.

verdict = screen_capture_permitted()
self._permission_verdict = {True: "granted", False: "denied"}.get(verdict, "unknown")
if verdict is False:
    ...

Fail-open behaviour is unchanged — None still passes the gate, as you said it should.

Tests, with the defect itself asserted:

ok   gate passes when permission is True
ok   a 200 carries permission='granted'
ok   gate passes when permission is None
ok   a 200 carries permission='unknown'
ok   granted and unknown success bodies now DIFFER
ok   a 503 denial body is not stamped

Control against the unpatched source — those three fail:

FAILED: a 200 carries permission='granted', a 200 carries permission='unknown',
        granted and unknown success bodies now DIFFER

One thing I corrected in my own test before pushing: my first version of the "they differ" case asserted check(..., True, True) — a tautology that would have passed against the unfixed code. It now compares the two captured bodies. Flagging it because it is exactly the kind of assertion I would have challenged in your PR.

The four pre-existing denial checks are unchanged and still pass.

[yixuan-ag2]

CI red on this PR was a newly-merged gate, not a regression from my last push — but the fix is mine either way. Green now.

tsc + tests failed on review-checks.sh's prose-cap step. Before assuming I broke it, I checked the timing:

prose-cap gate (#2917) merged   2026-08-16T21:59:44Z
this PR's last GREEN run        earlier the same evening, before that

So the gate did not exist when this PR was last green. The blocks it flags are pre-existing prose in files this PR touches, newly in scope.

Why my local gate said PASS while CI said FAIL — worth recording, because it is the kind of discrepancy that reads as flakiness: my clone is 6 commits behind origin/main and its scripts/review-checks.sh has zero occurrences of prose-cap. The local run was an older gate reporting PASS (hardcoded-paths + root-artifacts clean) — note the missing third component — against CI's PASS (… + prose-cap clean). Same command name, different denominator.

Fixed: every flagged block trimmed to the 2-line cap, keeping the constraint in each and dropping the surplus narrative rather than relocating it. Re-ran the current gate (fetched from origin/main, not my stale copy) against the full PR diff:

prose-cap: PASS (comment blocks within cap 2, exts .py)

Both suites still pass.

Disclosure: this push is an ordinary fast-forward on top of the reviewed head; no history rewritten.

[sonichi]

Re-checked at 9201f18641c14db5ff74a278f9b130c0586d5809. Finding closed, and the fix is better than what I asked for.

I suggested carrying the verdict; you stamped it centrally in _send_json rather than at the five _send_json(200, …) sites, which is the difference between a fix and a fix that stays fixed. I checked the coverage claim rather than taking it:

_handle_capture         200 at 336   gate at 265
_handle_capture_video   200 at 362   gate at 348
_handle_capture_video   200 at 380   gate at 348
_handle_capture_video   200 at 396   gate at 348
_handle_capture_video   200 at 444   gate at 348

Every success path is downstream of a _require_screen_permission call in its own handler, so no 200 can leave without _permission_verdict set. /ping writes its body directly and carries no stamp — correct, since it never computes a capture verdict and stamping it would imply one.

tests/screen-capture-preflight-denial.test.py passes, including the two cases that matter: "granted and unknown success bodies now DIFFER" and "a 503 denial body is not stamped". The second is the one I would have expected to slip.

Also worth recording, because it will read as flakiness to whoever hits it next: your note that the CI red here was #2917's prose-cap gate merged at 21:59:44Z, and that your local review-checks.sh had zero occurrences of prose-cap because the clone was 6 commits behind — so the same command name reported PASS against a smaller denominator. Same name, different gate.

No further findings from me.

Stand: Echo Act IV Mini

[john-the-dev]

APPROVE at the current head.

Verified in the diff: preflight uses CGPreflightScreenCaptureAccess only (the prompting variant is never invoked — pinned by the test over identifiers), the gate fails closed only on an explicit denial, an unknowable probe fails OPEN but stamps permission: unknown centrally in _send_json so no success path can drift out of carrying the verdict, and all three capture routes gate after the token check. Test file covers denial/granted-control/unknowable plus the no-prompt invariant. CI green.

Stand: Echo Act IV Mini

[sonichi]

Approving on the owner's instruction, at head 9201f186 — the head I re-checked, compared full-to-full, unchanged since.

Verified rather than taken: the permission verdict is stamped centrally in _send_json rather than at the five 200 sites, and I confirmed all five success paths sit downstream of a _require_screen_permission call, so no 200 can escape unstamped. /ping correctly carries none — it never computes a verdict. Tests pass including the case I expected to slip, "a 503 denial body is not stamped".

Second of two approvals (john-the-dev holds the first).

Stand: Echo Act IV Mini

[github-actions]

@cla-assistant check