Handler: retire notify-only, make the wrap-up durable, and unhook the comments from a missing spec - #73
Merged
Merged
Conversation
`notifyOnly` meant "tell me, never act": every pause escalated without spending a judge call, and a park wake notified instead of typing `continue`. Nothing in the product could turn it on. It was only ever sent as `defaultNotifyOnly`, which is read from handler-config.json — a file the bridge and app only ever read and never write. Hand-editing JSON was the sole path in, so the mode was unreachable by construction. Two engine branches go with it, and `outputSnippet()` behind them, which had no other production caller. `defaultNotifyOnly` was handler-config.json's only setting, so the schema, its v1 migration, the loader, the engine's config cache and `cfg()` go too; config.ts survives on ActivityRecord and appendActivity, which is what a later judge or personality picker actually needs from it. No message type is added or removed, so the checkout-variable sets are untouched. The accepted trade, stated rather than denied: anyone who did hand-edit that file has `notifyOnly: true` persisted in their handler-session record. Their parked sessions will now inject `continue` into a terminal they had opted out of auto-driving, and every pause will spend a judge call. Pre-v1, with no users, that is worth more than a compatibility shim for a mode the UI never exposed. One test is replaced rather than deleted: the notify-only sub-case was the only thing asserting that a standing `guard_blocked` report is not a pending question, and the park-wake gate reads exactly that predicate. It comes back as a park-wake equivalent.
The wrap-up was composed from three private helpers, spent on one push, and thrown away. The `wrapped_up` activity row kept only the goal, which the app renders nowhere. That left the phone-was-off case with nothing at all: handler-activity.jsonl is write-only, `handler:activity` is not a REPLAY_TYPE, and HandlerState is never persisted — so an app that reconnected after the disarm had no feed to go back to, which is exactly when the summary is read. A WrapUpRecord now persists in handler-wrapups.json and replays on handler:status, which is already a REPLAY_TYPE. One module owns content selection so the push and the stored copy cannot drift: buildWrapUp decides which items and which caps, and the push body and the activity detail are two thin renderings of its output. The undo count is the one thing that must never be frozen. It is an argument to the push renderer, not a field on the record, and the app re-derives it: an undo taken after the wrap-up, or a re-arm retiring the offers, would each make a stored count permanently wrong on a card whose whole job is to be read later. The blocked count and its reasons ARE frozen, deliberately — they die with the session when disarm drops it. Freeze what dies with the session, never freeze what outlives it. Sized for the wire it rides: handler:status is emitted twice per handler event, held by reference in the replay cache, and crosses the relay to a phone, so item text is clipped at 120 chars and five records are kept — a worst case of roughly 22K chars per frame rather than the 190K the uncapped shape allowed. The push loses its "see the activity feed" tail, because that feed is not durable and pointing at it was the bug. Wrap-ups survive a re-arm. Snapshots are retired because each pins a stash, a backup ref and a trash copy and owes a release(); a wrap-up owns no resource, so retiring one would keep that cost and drop its reason — losing the previous session's report is the failure this change exists to fix.
The bridge replays a WrapUpRecord on handler:status; this is the surface that reads it. The Handler screen gains a pinned Wrap-up section between Sessions and Undo, and a project whose only remaining artifact is a wrap-up no longer renders as "Handler is off" — the morning after, that is the whole screen. The undo count is derived at the call site from the terminal's snapshots and passed to the card, mirroring the bridge's own renderer taking it as an argument. Neither side of the wire is given a field it could freeze the count into, which is the only reason the number is still true when the card is finally read. Sans, not mono: a wrap-up is the user's own prose plus chrome labels, never a path or a command. The outcome labels are a local switch rather than a reuse of the activity-row labels — those are keyed on activity decisions (`item_done`), not wire outcome statuses (`done`), and bridging them would be a second hand-mirror to keep in lockstep for four words.
Roughly 180 comments pointed at numbered sections of a handler spec — `§4.3`, `spec §5.2` — that is not in this repository and is not in its history. A reader who followed one found nothing, and the tag was doing the work a sentence should have been doing. Where the prose already carried the reason, the tag is simply gone. Where the section number WAS the justification, the reason is written in from the code that enforces it and names the symbol a reader can grep: `§5.4` becomes instruction-scoped authorization and the file that implements it, `§5.3` becomes the HARD floor tier, `§2.2` becomes the terminal states. The references that resolve are untouched: bridge/src/e2e/ and packages/ cite docs/protocol/e2e-handshake.md by named section and that file exists, and two more cite RFCs. A path plus a named section is a working pointer; a bare number pointing at nothing is not, and that is the whole distinction being applied here. No spec document was written to make the numbers resolve. Reconstructing one from the code would invent a source of truth that never existed and leave 180 comments citing a document written after them.
The same dangling references ran through test names — `describe("§2.2 allTerminal is the wrap-up predicate")`, `group('quick choices (§4.6)')`. A test name is read in failure output, where a section number from a missing document is worth less than nothing: it names an authority the reader cannot consult instead of the behaviour that broke.
Renamed to describe the behaviour, and the comments inside these files got the same treatment as the source. Pass counts are unchanged — 3159 bridge, 3087 app.
Six conflicts, all where #71 improved the same code HF-3 deleted and HF-6 replaced. Two of them were live risks rather than textual noise, because the auto-merge would have kept our side and silently reverted a shipped fix: The push named what a guard refused; our composer only counted it. #71's point stands and is now the composer's: a count reads the same whether the guard stopped something trivial or the one thing the session existed to do, and the push is the only channel reaching a phone whose app was not running when the activity rows went out. `blockedClause` renders the record's reasons, capped at two for the lock screen while the card keeps three. The undo clause belonged AHEAD of the reports; ours put it last. OS surfaces truncate the tail, and of the two only the undo expires — the reports keep on the wrap-up card, while the offer to undo is gone once the user stops looking for it. Restored, and pinned by a test, which nothing did before. Kept from #71 otherwise: the activity-log rotation and its three tests, and the injective `JSON.stringify([tier, matched])` Set key that replaced the NUL separator. Its rotation comment claimed the jsonl was the only durable copy of what a wrap-up describes — true when written, and no longer, so it now says what is still true: those rows are the only record of the session decision by decision. Thirteen `notifyOnly: false` arguments arrived in tests #71 added. They merged clean, so nothing flagged them against a field HF-3 removed; stripped by hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three Handler follow-ups from the HF review: retire
notifyOnly, give thewrap-up summary a surface that survives the session, and stop citing a spec that
does not exist. Five commits, each independently revertible.
HF-3 —
notifyOnlygoes (6a32806b)notifyOnlymeant "tell me, never act": every pause escalated without spending ajudge call, and a park wake notified instead of typing
continue.Nothing in the product could turn it on. It was only ever sent as
defaultNotifyOnly, read fromhandler-config.json— a file the bridge and apponly ever read, never write. Hand-editing JSON was the sole path in, so the
mode was unreachable by construction.
Two engine branches go with it, and
outputSnippet()behind them, which had noother production caller.
defaultNotifyOnlywas that config file's only setting,so the schema, its v1 migration, the loader, the engine's config cache and
cfg()go too.config.tssurvives onActivityRecordandappendActivity—which is what a later judge or personality picker actually needs from it.
The accepted trade, stated rather than denied. Anyone who did hand-edit
that file has
notifyOnly: truepersisted in their handler-session record. Theirparked sessions will now inject
continueinto a terminal they had opted out ofauto-driving, and every pause will spend a judge call. Pre-v1, with no users,
that is worth more than a compatibility shim for a mode the UI never exposed.
One test is replaced rather than deleted: the notify-only sub-case was the only
thing asserting that a standing
guard_blockedreport is not a pending question,and the park-wake gate reads exactly that predicate. It returns as a park-wake
equivalent.
Not done here, deliberately: the escalate-only empty state. Re-aiming the deleted
copy at
escalate_onlysessions is new product behaviour, and it belongs on itsown terms rather than inside a deletion.
HF-6 — the wrap-up outlives its session (
bcaf544a,43bfb37c)The morning-after summary was composed from three private helpers, spent on one
push notification, and thrown away. The
wrapped_upactivity row kept only thegoal, which the app rendered nowhere.
That left the phone-was-off case with nothing at all:
handler-activity.jsonliswrite-only,
handler:activityis not aREPLAY_TYPE, andHandlerStateisnever persisted — so an app that reconnected after the disarm had no feed to go
back to. Which is exactly when the summary is read.
A
WrapUpRecordnow persists inhandler-wrapups.jsonand replays onhandler:status, already aREPLAY_TYPE. One module owns content selection sothe push body and the stored copy cannot drift. The app gains a pinned Wrap-up
section, and a project whose only remaining artifact is a wrap-up no longer
renders as "Handler is off".
The undo count is never frozen. It is an argument to the push renderer, not a
field on the record, and the app re-derives it. An undo taken after the wrap-up,
or a re-arm retiring the offers, would each make a stored count permanently wrong
on a card whose whole job is to be read later. The blocked count and its reasons
are frozen, deliberately — they die with the session when disarm drops it.
Freeze what dies with the session, never freeze what outlives it.
Sized for the wire it rides.
handler:statusis emitted twice per handlerevent, held by reference in the replay cache, and crosses the relay to a phone.
Item text is clipped at 120 chars and five records are kept: a worst case of
~22K chars per frame, against the ~190K an uncapped shape allowed. The push
loses its "see the activity feed" tail, because that feed is not durable and
pointing at it was the bug.
Wrap-ups survive a re-arm. Snapshots are retired because each pins a stash, a
backup ref and a trash copy and owes a
release(); a wrap-up owns no resource,so retiring one keeps that cost and drops its reason.
HF-11 — comments stop citing a spec nobody can open (
b98f0651,dcd36dcb)~180 comments and 29 test names pointed at numbered sections of a handler spec —
§4.3,spec §5.2— that is not in this repository and is not in its history.Where the prose already carried the reason, the tag is simply gone. Where the
section number was the justification, the reason is written in from the code
that enforces it, naming a symbol a reader can grep:
§5.4becomesinstruction-scoped authorization and the file implementing it,
§5.3becomes theHARD floor tier,
§2.2becomes the terminal states.The 19 references that resolve are untouched:
bridge/src/e2e/andpackages/citedocs/protocol/e2e-handshake.mdby named section and that fileexists, plus two RFC citations. A path with a named section is a working pointer;
a bare number pointing at nothing is not.
No spec document was written to make the numbers resolve. Reconstructing one from
the code would invent a source of truth that never existed and leave ~180
comments citing a document written after them.
Review notes
An audit pass over the comment rewrites caught three fabricated invariants —
comments asserting rules the code does not have. Each was verified against the
source and corrected before commit:
session_row.dartclaimed the row body is the only affordance a stoppedsession has. There is a kebab Start at
:678whose handler comment says it isthe same intent.
session-store.tsinvented a bound of "one disarmed session per upgrade".sessionPathis per-project-per-terminal, so an upgrade disarms every armedsession.
handler_reply_sheet.dartclaimed the safety floor is the one class of blockthat always routes through a human.
quickChoicesForreturnsundefinedforevery
guard_blocked; what is unique to the hard floor is that nothing lifts it.Worth stating plainly: a comment-only change is invisible to every test here.
All 3159 bridge and 3087 app tests pass with those three false claims in the
tree, which is why they were hunted for directly.
Gates
bun run --filter antgrid-bridge test— 3159 pass, 14 skip, 0 failcd app && flutter test— 3087 passcd app && flutter analyze— No issues foundbun run typecheck— clean across all five TS workspacesnpm run check:font-tokens— OKeverything else CRLF, none mixed.
Not verified:
evals/tests/handler.test.tswas extended for HF-3 but theeval suite was not run — it spawns real agents, relays and PTYs. Nothing yet
proves a wrap-up record crosses a real relay into
HandlerState.Unreviewed on a device: the wrap-up card renders every outcome group plus a
blocked line plus the undo line — up to a 7-line row, the tallest on the screen.
A deliberate call over a disclosure widget, but nobody has seen it on a phone.