Skip to content

fix(approval): a desktop approval rereads the app's window, not the whole screen - #1778

Merged
abrichr merged 16 commits into
mainfrom
claude/approval-form-bands
Nov 11, 2026
Merged

abrichr merged 16 commits into
mainfrom
claude/approval-form-bands

Conversation

@abrichr

@abrichr abrichr commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

Problem

On a desktop run, approval mode compares the form after the person approves it with the form they saw. Without a DOM, that comparison was an OCR digest of the whole screen. On the hosted Windows governed run (#1697), the taskbar clock changed between the approval and the save, so every signed approval ended HALTED (each_item: form_changed_after_approval).

Change

  • win_agent. A new typed read, window-bounds (POST /window/bounds, capability window_bounds_v1), returns the visible frame of a window, given its hwnd and pid. It answers None when the window is gone, has changed process, or is hidden.
  • WindowsBackend. approval_form_region(exclude_step) returns that frame for the one window the run's verified steps were in, apart from the save step. It answers None when no step was verified, when steps were seen in two windows, or when the agent's answer is doubtful.
  • Replayer. It reads the approval's form state inside that window, clipped to the frame. The save step is left out, so the request and the recheck ask about the same window. Backends without the hook read the whole screen, as before.
  • Form state. In region mode, words keep OCR reading order instead of being sorted, so values that trade places change the digest. The window frame is part of the digest, so a moved or resized window halts the save. A window that reads nothing gives None, which the approval path treats as a refusal.

Review round 1 (the row-band version) asked for changes: fields with no click point, wrapped lines, the patient banner and scrolled fields fell outside the rows, and sorted words hid swapped values. Reading the whole app window, in reading order, addresses each of these.

Tests

tests/test_approval_form_window.py uses real OCR on rendered frames:

  • a clock outside the window leaves the digest unchanged (and in whole-screen mode it changes the digest);
  • each of these inside the window changes the digest: a banner, a field, a wrapped line, a far field, and swapped values;
  • a moved window changes the digest;
  • a blank window gives None;
  • the replayer passes the save step id, clips the window to the frame, and falls back to the whole screen when the window is missing, unknown, raises, or is off the frame;
  • the Windows backend refuses a doubtful window in each of 7 ways.

tests/test_win_agent_server.py covers the agent's window-bounds read.

Part of #1697.

Review record (independent adversarial reviewer)

  • Round 1: CHANGES REQUESTED (row-band version). Fields without a click point, wrapped values, the banner and scrolled fields fell outside the rows, and sorted words hid swapped values. The PR was reworked to read the app window.
  • Round 2: CHANGES REQUESTED.
    • A form in another window of the app was not read. The agent now answers None when any window of the app reaches past the frame.
    • Captures were never cleared. run() now clears them.
  • Round 3: APPROVE.
  • Round 4: APPROVE of a blocker-only fix found on hosted Windows. A resumed run always halted; it now names the window from the earlier steps' restored points, and that choice is not kept as a capture.

@abrichr abrichr changed the title fix(approval): a desktop approval rereads the item's field rows, not the whole screen fix(approval): a desktop approval rereads the app's window, not the whole screen Nov 3, 2026
@abrichr
abrichr marked this pull request as ready for review November 3, 2026 23:40
@abrichr
abrichr enabled auto-merge (squash) November 4, 2026 00:22
replayer.run clears the verified windows first, then main's at-rest
handling. The window-forgetting test sets the checkpoint key that main's
run now reads, since it builds the Replayer without __init__.
@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Review (Grok), head 24ab796: BLOCK

I read the diff and the CI logs for this head. The approach is sound: reading only the app window, in OCR reading order, with the window frame in the digest, does fix the false form_changed_after_approval caused by the taskbar clock. Three things need fixing before it can merge.

  1. Safety-ordering regression (P1). Replayer.run() now calls self.backend.clear_verified_windows() as its first statement, before the draft and fail-closed refusal checks. The tests that assert the backend is not touched before a refused run now fail, 10 in total across shards 2 and 3 (job 126743302603 has 7 failed / 5170 passed, job 126743302594 has 3 failed / 4119 passed). All 10 fail with "AssertionError: the backend was used (clear_verified_windows) before the check":

    • tests/test_jit_draft_execution.py: test_the_replayer_refuses_a_draft_and_its_copy, test_an_agent_cannot_attend_a_draft_run (5 params), test_emit_mcp_labels_a_draft_and_its_server_refuses_it
    • tests/test_jit_fail_closed.py: test_deleting_the_draft_marker_does_not_free_a_draft, test_a_replay_on_another_app_is_refused, test_a_replay_without_an_independent_readback_is_refused

    Please move the clear to after the refusal checks pass, just before the first step runs, rather than relaxing those tests. Clearing it in a finally at the end of run() would also stop stale captures carrying over.

  2. mypy (lint job 126743302255). windows_backend.py:937 reports "Set comprehension has incompatible type Set[tuple[int, int] | None]; expected Set[tuple[int, int]]". The None in captures check is right; build the set as Optional first, then narrow it.

  3. Merge conflict. GitHub reports the branch as CONFLICTING with main, so it needs to be merged with current main (not rebased, since it is already pushed) and CI re-run.

Not checked: I did not run the real-OCR window tests locally or try it on a Windows host. This verdict rests on the diff and the CI logs. I will re-review when a new head is pushed.

# Conflicts:
#	packages/flow/CHANGELOG.md
@abrichr

abrichr commented Nov 8, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

BLOCK remains at 8b36670. The two code blockers from Grok's earlier review are still present after the main merge.

Replayer.run still reads self.backend.clear_verified_windows before the draft and fail-closed refusal checks (replayer.py:1650). The exact-head hosted CI reproduces all ten backend-before-check failures: three fail-closed cases and seven draft-execution cases. Move this backend work behind the existing refusal boundary; retain the refusal assertions. The lint job also still reports the Optional capture-set assignment at windows_backend.py:937.

I checked the current source and exact-head hosted logs; I did not rerun unchanged probes or native Windows tests. This is a generic safety-ordering defect, with no per-app engine change requested. Held-out real-app evidence is not established by this review.

Review by Codex.

Replayer.run cleared the backend's captured windows before the draft and
fail-closed checks, so a refused run touched the backend. The clear now
runs in _run_unsealed_steps next to the qualification guard reset, after
every refusal check. Narrow the resumed-run capture set to non-None for
mypy.
@abrichr
abrichr marked this pull request as draft November 9, 2026 15:29
@abrichr

abrichr commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Parked (coordinator, 2026-11-09). We're focusing on verification at the system of record (wire, database and API read-back, zero-config verifiers generated from captured traffic, and the agent cache) instead of hardening screen-reading heuristics such as OCR, pixels, fonts and UI label guards. This PR stays as a draft and gets no further review rounds unless it fixes a case where OpenAdapt takes an unsafe action. If so, comment with that case and it will be reprioritized. The branch is kept.

abrichr added a commit that referenced this pull request Nov 10, 2026
Main's draft and fail-closed tests require that a refused run never
touches the backend. The run now forgets earlier captures only after the
refusal checks, as #1778 does, and the form-window tests match #1778's:
a refused run keeps the captures and sends nothing.
@abrichr
abrichr marked this pull request as ready for review November 10, 2026 23:45
@abrichr

abrichr commented Nov 10, 2026

Copy link
Copy Markdown
Member Author

Finished and ready at 4294954.

  • Grok/Codex P1 (backend touched before the refusal checks): clear_verified_windows() runs after the draft and fail-closed refusals. The 10 backend-before-check tests in test_jit_draft_execution.py and test_jit_fail_closed.py pass unchanged.
  • mypy at windows_backend.py: mypy is clean (682 files).
  • Merged origin/main, which was conflicting. The resumed-run path now uses main's window_root_at (the private _root_window_at copy is gone). The CHANGELOG keeps main's 2.7.1 entries plus this PR's line.
  • Size budget: replayer.py was at its budget, so the window-region logic moved to approval_mode/form_state.approval_window_regions and _row_dates moved to runtime/row_dates.py (re-exported). replayer.py is 20138 lines and its budget is lowered to 20138. A new test drives Replayer._approval_form_state end to end.
  • Local, Python 3.12: approval-window, win_agent, jit and approval tests 956 passed; ruff, ruff format, the generality check and the size ratchet are clean.

@abrichr

abrichr commented Nov 11, 2026

Copy link
Copy Markdown
Member Author

New head 17d8379 (after 0b3c85c, which rebuilt llms-full.txt for the changelog line).

Guard: window-bounds now returns window: None unless the virtual desktop starts at (0, 0) (GetSystemMetrics SM_XVIRTUALSCREEN/SM_YVIRTUALSCREEN, the origin mss monitors[0] uses). It also returns None when the origin can't be read. With a monitor left of or above the primary, approval_form_region therefore gets no frame, and the approval reads the whole screen. New test test_window_bounds_refuses_a_virtual_desktop_not_at_zero covers origins (-1920, 0), (0, -1080) and (-1280, -200), plus an origin that can't be read. win_agent and approval-window tests: 171 passed; mypy clean.

@abrichr

abrichr commented Nov 11, 2026

Copy link
Copy Markdown
Member Author

Cross-vendor review relayed from Grok (internal inbox, 2026-11-11). Round 2: APPROVE at 0b3c85c. The P1 is fixed, because clear_verified_windows() now runs after the refusal checks. No path returns a region that misses part of the approved form. Every doubt falls back to the stricter whole-screen read, and the size-budget refactor preserves behavior. Delta check: confirmed at 17d8379. The only change is the virtual-desktop origin guard in win_agent/server.py: window-bounds returns None unless SM_X/YVIRTUALSCREEN is (0,0), and an unreadable origin also returns None. A new test covers three negative origins and one unreadable layout. 12 window_bounds tests pass, and hosted CI has 32 passes and 0 failures. A live multi-monitor Windows box was not run.

@abrichr
abrichr merged commit 6ca9b3f into main Nov 11, 2026
57 checks passed
@abrichr
abrichr deleted the claude/approval-form-bands branch November 11, 2026 02:51
abrichr added a commit that referenced this pull request Nov 11, 2026
…p reports without failing the job

#1778 is on main, so its files take main's version. What remains is this
PR's own work: the hosted governed Windows test, the move of
_pipeline_safe_verifier to runtime/effects/pipeline_safety.py (replayer
budget lowered to 20095), and the evidence and workflow wiring.

The governed run is a continue-on-error step with a 10-minute timeout,
after the no-skips check, so a stop before the save prints halts and
form_state_probe without failing windows replay. The changelog line
duplicated #1778's and is dropped; llms-full.txt is rebuilt.
abrichr added a commit that referenced this pull request Nov 11, 2026
…l, a hold and a resume) (#1804)

* fix(approval): a desktop approval rereads the item's field rows, not the whole screen (#1697)

* Read an approval's form in the app's window, in reading order

* Format

* Read the whole screen when an app window reaches past the frame; forget captures per run

* A resumed run names the approval window from the earlier steps' points

* Run the approval-window tests on hosted Windows too

* test: a governed Windows run on a hosted runner, Standard with an approval, a hold and a resume (#1697)

* Prove the hold and the person's approval; keep the approver key out of the run

* The run reads the approver roster; only the person holds the passphrase

* test: record which form-state read the held approval and its resume used

The hosted resume halts with form_changed_after_approval. On failure the
governed test now recomputes the request's digest from its own frame and
from the resumed save's frame, over the whole screen and over the app
window, and records the window lookup under the earlier steps' points.
The approval request files join the uploaded evidence.

* fix: clear captured windows after the run's refusal checks

Main's draft and fail-closed tests require that a refused run never
touches the backend. The run now forgets earlier captures only after the
refusal checks, as #1778 does, and the form-window tests match #1778's:
a refused run keeps the captures and sends nothing.

* test: say why the approved governed run stopped

On a nonzero exit the hosted test now records each halted step's
category and error, and recomputes the request's form-state digest from
the save's frame, ahead of the output tail in the assertion message.

* test: print why a resumed governed run stopped

The first non-gating run passed the approval and the hold but the resume
halted, and the message showed neither the halt reason nor the whole probe.
The resume now records halts too, and both phases print halts and
form_state_probe in full before asserting.

* ci: gate the Windows governed run on its record writes

The governed step stays continue-on-error for the #2189 false stop. A new
if: always() step reads the evidence the test now records (the SQLite rows
each phase left, its outcome and approval) and fails the job on a sibling or
wrong-record write, a held run that wrote, or a target write without an
approved VERIFIED report. CAPABILITIES says the governed run is non-gating
and drops the RDP-session claim, whose evidence is not public.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant