feat(bridge): bind approvals to the prompt they were decided against#29
feat(bridge): bind approvals to the prompt they were decided against#29Optic00 wants to merge 1 commit into
Conversation
Collie already binds an approval to the dialog the operator tapped: entryGuard re-reads the pane, re-derives the model and compares it before sending. That check is entirely client-side, so the bridge enforces nothing. POST /api/pane/:id/keys and /reply accept any keystroke with no precondition, which means a client that skips the guard writes blind, and the guard's own window still contains a full round trip from the phone to the bridge. This moves the decisive check behind the API without moving the dialog grammar. Both endpoints accept an optional expected_prompt: the region the client verified. When present, the bridge does one fresh pane.read immediately before writing and requires that region to still be there. On drift it answers 409 with code "prompt_changed" and writes nothing; the client maps that onto the existing "changed" path, so the UI refuses and refreshes as it already does. When the field is absent nothing changes, so existing clients are unaffected. The comparison is line-based over a normalised region. Only trailing padding and blank lines are ignored: leading indentation and internal alignment survive, because in a displayed diff or command they are content rather than redraw noise, and erasing them would let two different edits compare equal. The match must also end within a few lines of the tail. Both bounds are measured against the committed fixtures rather than guessed: at most one normalised line follows a real match, so a replaced prompt pushes a stale region well outside the window and is refused. The read depth for the check is derived from the expectation rather than from COLLIE_READ_LINES. The client reads 600 lines while that setting legally goes down to 1, so a small value would make every approval fail permanently. preview-select and multi-select gain an additive regionSignature: their existing signatures deliberately normalise the pointer and checkboxes out, so they are identities rather than verbatim screen text and the bridge cannot find them. The existing fields and every client comparison are untouched. In the multi-step flows only the first write after each guard is bound. Those macros change the dialog on purpose, so binding a later step to the region captured before the first one would reject every valid run. The later steps stay protected by the identity re-check the macros already perform. This is a mitigation, not a guarantee. The re-read and the send are two separate herdr RPCs, so a TOCTOU window remains by construction; it shrinks from seconds of poll, push and human latency to the few milliseconds between two local calls. Closing it completely needs a conditional input primitive in herdr, which does not exist today. A pair of contract tests pins both halves to the same committed regions, since the client derives them from parsed text while the bridge matches raw text and nothing else couples the two. Either side drifting turns one of them red.
|
Holding this one for now, but not because it is wrong. #28 and #30 are merged and shipped in 0.15.0; this needs one change first. What I checked. It merges clean on main, 358 backend and 1054 web tests pass, both typechecks clean. I negative-controlled the contract test in both directions rather than taking it on trust: breaking the bridge normalizer fails 21 tests, and perturbing the client region derivation fails 5. So the silent-drift failure mode you identified, where every approval starts returning "prompt changed" and looks like the feature working, is genuinely pinned by the fixtures. That was the right thing to worry about and the right way to handle it. The change I want first. Two smaller notes, neither blocking. On scope. Post-#28 the only writers are allowlisted devices running our own PWA, so the threat this addresses is narrower than it was when you opened the PR. It is still worth having, and your honesty about the remaining TOCTOU window is appreciated. If you want to raise the conditional-input primitive upstream in herdr, that seems worth asking for and would let this become a guarantee rather than a mitigation. Rebase on main when you pick it back up; the merge is clean today but 0.15.0 moved a few of the same files. |
2 participants
Where this starts
Collie already binds an approval to the dialog the operator tapped.
entryGuardre-reads the pane,re-derives the model, and compares it before sending, with the comments explaining that herdr's
revisionis a stub so the content re-derivation is the load-bearing check. That is a carefuldesign and this PR does not replace it.
What it adds is enforcement.
POST /api/pane/:id/keysand/replyaccept any keystroke with noprecondition, so the guard is advisory: a client that skips it writes blind, and the guard's own
window still contains a full round trip from the phone to the bridge. This moves the decisive check
behind the API without moving the dialog grammar out of the client.
The wire
Both endpoints accept an optional
expected_prompt: the region the client verified.POST /api/pane/:id/keys { "keys": ["1"], "expected_prompt": "…the dialog region the UI rendered…" }Absent, nothing changes at all, so existing clients and the nav tray are unaffected. Present, the
bridge does one fresh
pane.readimmediately before writing and requires the region to still bethere. On drift it answers
409withcode: "prompt_changed"and writes nothing. The client mapsthat onto the existing
"changed"result, so the UI refuses and refreshes exactly as it does today,with no new surface.
What the comparison covers, and why
This is the decision the feature lives or dies on, so here is the reasoning.
The region is compared as a contiguous run of lines, after stripping ANSI, collapsing runs of
whitespace, and dropping blank lines. The match must end within the tail of the read.
Wider, for example hashing the whole pane, and a spinner or a clock invalidates every approval until
users switch the feature off. Narrower and it stops seeing real changes. A terminal redraw changes
padding, column alignment and blank-line layout without changing the question, so normalising those
away is exactly the noise that must not count, while every word and every option label still does.
Line-based rather than substring, because a raw substring match can straddle line boundaries and
match text that was never one block on screen.
Against the committed pane fixtures: a legitimate approval passes, a re-padded redraw still passes,
and an answered, replaced or scrolled-away prompt is refused.
Why the bridge does not gain a grammar
The dialog grammar under
web/src/lib/harness/is around 2000 lines and pluggable per harness.Duplicating it into the bridge to compute a hash server-side would cross that design and be far
harder to review, so the client sends the region text it verified and the bridge checks content it
does not need to understand.
preview-selectandmulti-selectgain an additiveregionSignature. Their existing signaturesdeliberately normalise the pointer and the checkbox glyphs out, which makes them stable identities
for the client's own comparison but not verbatim screen text, so the bridge cannot find them. The
existing fields, their computation, and every client comparison are untouched.
There is deliberately no
expected_blockedflag.agent_statusis not carried bypane.read, onlyby
session.snapshot, so checking it would cost a second RPC before the write and widen the verywindow this exists to shrink. The region check already subsumes it: if the exact prompt text is
still on screen, that prompt is still what the pane shows.
Multi-step flows
preview-actionandmulti-select-actiondrive the dialog through several writes. Only the firstwrite after each guard carries an expectation. Those macros change the dialog on purpose, so binding
a later step to the region captured before the first one would reject every valid run. The later
steps stay protected by the identity re-check the macros already perform before each write. This is
commented at each site so it does not get "fixed" later.
Honest limitation
This is a mitigation, not a guarantee. The re-read and the send are two separate herdr RPCs, so a
TOCTOU window remains by construction. It shrinks from seconds of poll interval, push delivery and
human reaction time to the few milliseconds between two local calls. That removes the human-latency
portion, which is where essentially all of the real risk lives, but it does not close the window.
Closing it properly needs a conditional input primitive in herdr,
send_keyswith a preconditionrejected atomically server-side. That does not exist today. I am happy to raise it upstream there if
you think it is worth asking for.
Tests
bun test ./bridge: 219 before, 260 after.cd web && bun run test: 910 before, 943 after. Bothtypechecks clean.
The web suite has 17 pre-existing failures on my machine, in
use-display-prefs.test.tsandagent-chat.test.tsx, which reproduce on an unmodifiedmainhere and look environmental(
localStorage is not available because --localstorage-file was not provided). Untouched by thisPR, and CI is green on
main, so I assume they are local to my setup.One pair of tests is worth pointing at. The client derives the region from text it has already
parsed ANSI away from, while the bridge matches against the raw
pane.read, and nothing in the typesystem couples the two. If either normalisation drifts, every legitimate approval starts coming back
"prompt changed", which looks like the feature working. So
prompt-binding-regions.jsonpins theexact region each detector produces for each fixture: the web test proves the detectors still
produce them byte-for-byte, the bridge test proves the bridge still finds them in the raw text, and
a completeness check fails if a new dialog fixture appears without a pinned region. I verified the
pair actually fires by breaking each side on purpose.
Note for reviewing
web/src/lib/multi-select-action.tsshows up as a binary diff. That is pre-existing: the filecontains a NUL byte used as a cache-key joiner, on
mainas well.git diff --textrenders it.