Uh oh!
There was an error while loading. Please reload this page.
[Air #1474] Render gate: anchor the agy prompt marker to the cursor row and marker palette - #1491
Conversation
… and marker palette `AGY_MARKER = /^> /` treated any `> `-prefixed line as agy's composer. Measured against real agy 1.1.13 under a PTY, that is not a hypothetical looseness: agy echoes every submitted turn into the transcript as `> <message>` (palette-4), its slash-menu selection cursor is also `> ` in palette-12 and renders BELOW the composer, and the trust dialog's selected option is `> Yes, I trust this folder`. Since `findMarkerRow` is last-match-wins, the menu item and the turn echo won the marker scan over the composer — so the classifier bounded and scanned the wrong region on ordinary screens. Adds two optional, per-app `GateProfile` anchors, both set only on agy and both measured: `markerRequiresCursorRow` (the marker row must hold the cursor — only the live input row does) and `markerFgPalette` (the marker glyph's own color, 12, which separates the composer from the palette-4 turn echo). Also relaxes the marker separator to `\s|$`: agy's no-hint mode renders the empty composer as a bare `>`, which `/^> /` never matched, so the gate held every message to an agy in that mode forever. Replaces the three synthesized agy fixtures with seven real, sanitized captures (idle / bare-marker / draft / menu / trust / turn-echo / torn mid-repaint frame). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er thread Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohidmakhdoomi
commented
Aug 18, 2026
Test status, stated precisely
The All 3 failures are in They are pre-existing and unrelated to this PR. Verified rather than asserted: I reverted both changed source files to the branch base (141b493), rebuilt, and re-ran that file — identical 3 failures. Files restored afterwards (all committed, so lossless; working tree verified clean against HEAD, unit tests re-run green). I did not skip or annotate them as flaky, because they are neither mine nor intermittent — they reproduce consistently. Flagging for a maintainer instead. Plausibly environmental: this machine is running four-plus concurrent builder sessions plus Tower, and all three failures are PTY-spawn-through-the-API. |
mohidmakhdoomi
commented
Aug 18, 2026
Architect integration review — 3-way CMAP (risk tier: High — render-gate classifier, false-CLEAN direction)Verdicts: gemini APPROVE · codex APPROVE · claude COMMENT ("merge it, with follow-ups") — all HIGH confidence, zero blocking correctness issues. Architect read concurs: the two positive-evidence anchors ( Independently verified across the reviews: viewport-relative Requested before the gate (one item)
Follow-ups (this PR or noted for later — builder's call, say which)
For the maintainer
Parked for maintainer approval + merge; we are not maintainers. |
…low-ups CMAP requested item: the agy anchors are the first classifier input that depends on CURSOR STATE, and the cursor is the one dimension where the transient replay path and the long-lived SessionScreen mirror could diverge. Adds a per-fixture parity case to the production-mirror-path suite — same bytes through both paths, same verdict — fed twice, once in production-sized chunks and once in 7-byte chunks that deliberately split the cursor-positioning CSI across feed() calls, since a mis-parsed cursor is now a verdict change rather than a cosmetic one. Follow-ups taken in-PR rather than deferred: - arch.md: "marker present" now means more than a text match for agy — records the two anchors and their fail-toward-HOLD direction. - Commits the capture + sanitization harness (codev/air-1474-captures/) so re-measuring against a future agy starts from a script, not the PR description. Verified by re-deriving four committed fixtures byte-identically from the raw captures. - Fixes the sanitizer's leak check, which reported `/home/` on every run because the placeholder path itself starts with it — a check that always fires checks nothing. Now matches paths that are not the placeholder, and refuses to write on a leak. - Cosmetic: required-fixtures test title listed three agy states while requiring seven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohidmakhdoomi
commented
Aug 18, 2026
CMAP items addressed — all four taken in-PR, none deferredRequested before the gate ✅agy case in the production-mirror-path suite ( Each fixture is fed twice: once in production-sized 64 KiB chunks, and once in 7-byte chunks that deliberately split escape sequences across Follow-ups — taken, not deferred
One thing the harness work turned upThe sanitizer's leak check was self-defeating: it tested for the bare prefixes To be clear about what this did and didn't mean: the committed fixtures were always clean — I verified that independently with a direct grep before the first push, and the byte-identical re-derivation confirms it again. The defect was in the check, not in the fixtures. Ready for the |
…, PR parked) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
waleedkadous
commented
Sep 3, 2026
Thanks for the patience on this one — the review queue is moving now. This PR is showing as CONFLICTING against main after this week's merges in the same area (serializer convergence #1492, delivery verification #1573/#1584, self-attesting frames #1574, and the recipient/reply-hint changes to message-format.ts and tower-routes.ts). Could you rebase onto current main when you get a chance? I'll review the moment it's green — the four non-conflicting PRs in your list are in review right now. |
Resolves the one conflict in render-gate.ts: main added the exported `bufferLines()` helper (#1573 echo verification) directly above `findMarkerRow`, whose doc and signature this branch rewrote for the cursor-row/palette anchors. Both are kept — they are adjacent, not competing. Typecheck clean; render-gate suite 66/66. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…alarm Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohidmakhdoomi
commented
Sep 3, 2026
Brought up to current |
Retrospective at codev/reviews/1482-f1-tower-vs-pty-dimension-dive.md, plus the governance routing it describes. The Summary leads with the user-visible harm rather than the mechanism: the owner starvation notice offered `afx interrupt` for EVERY hold, and for a `user-text` hold that is advice to interrupt a person mid-draft. The dims fix and the detail column are how that becomes possible to tell apart, not the point in themselves. Both gaps the human approved knowingly are stated in the review rather than left for a reviewer to find: the dashboard popover still renders a bare `busy` (reverted deliberately — no worktree.devCommand, no Playwright here, and the project requires a browser check), and evidence item 3 is covered by test rather than induced live because send.ts constructs `new TowerClient()` with no port and so reaches only the live Tower on 4100. Also called out for the maintainer: the beyond-plan 409 RESIZE_DROPPED route change and CronDeliveryResult gaining `detail`; the v18-vs-v17 migration collision surface (in two places — the constant and the pinned source assertion that reads it back); and the conflict surface with the two parked PRs, #1486 (commands/inbox.ts, commands/send.ts, tower-routes.ts) and #1491 (render-gate.ts, render-gate.test.ts). **Governance routed COLD only.** Both hot files sit exactly at their caps (10 facts / 10 lessons) and nothing here justified displacing an existing entry; the hot map already routes readers to "Invariants & Constraints", so the tiering works without growing the always-injected tier. - arch.md: new invariant #10, "Terminal dimensions must be earned, not assumed" (requested/applied/outstanding, 409 vs 404, WELCOME adoption, and why the gate depends on it); and the Spec 1313 mailbox section's response vocabulary extended with the v18 `detail` column and why it carries no CHECK constraint. - lessons-learned.md: three Architecture entries (a failure boolean is only as honest as its callers; two-state models fail at the third state — the missing bit was OUTSTANDING, not DIFFERENT; commit derived state only after what it describes is confirmed) and three Process entries (read the ignore rule's own reason before `git add -f`; report the boundary of a gap, not the instance you tripped over; mutation-check a test written to close someone else's finding). Full suite green on this tree: 278 files passed / 3 skipped, 5495 tests passed / 48 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
waleedkadous
left a comment
There was a problem hiding this comment.
Really thorough work — seven real agy captures, per-fixture mirror-vs-transient parity tests (including the 7-byte chunking that splits cursor CSIs), and a failure direction that stays HOLD rather than deliver. The render-gate change itself is approve-quality and claude's lane approved it outright; the one thing I'd like fixed before merge is in the capture tooling, because it's a leak-safety guarantee that currently isn't one:
codev/air-1474-captures/sanitize.pywrites the output before running its leak check (theopen(dst, …).write(data)at ~line 137 precedes the# Leak checkblock). If identifiers survive, it raises only after the unsafe fixture is already on disk — contradicting its own "REFUSING to sanitize" message and creating an accidental-commit risk. Run every check before writing, or deletedston failure. Relatedly, its path regex misses/Users/…, so a macOS capture would pass the check while leaking a username (the committed fixtures are clean; this is about the next capture).
Two small things you might consider alongside, neither blocking:
buf.cursorYis baseY-relative in xterm's public contract; the code relies onviewportY === baseY.buf.baseY + buf.cursorY - buf.viewportYremoves that assumption for free, and it's now load-bearing for delivery.- Idle-session drift is silent —
onLivenessgates on recent output, so a re-themed agy sitting idle holds forever with no alarm, the same shape as the bare->bug this fixes. A follow-up on profile-drift observability (classifier-stuck escalation for idle sessions, or anafx doctorcheck that classifies each live mirror once) would close that.
Also noting markerFgPalette will want to accept multiple colors before agy ships a truecolor theme — follow-up territory.
…hrough baseY Addresses the PR #1491 maintainer review. BLOCKING — sanitize.py wrote the output file before checking it for leaks, so a capture that still carried an identifier landed on disk and the "REFUSING to sanitize" message was false by the time it printed. Now every check runs before anything reaches the filesystem, chosen over delete-on-failure: a guarantee that depends on cleanup running is weaker than one that never creates the file. PATH_RE also gains /Users/, without which a macOS capture sanitizes cleanly while leaking a username — the prefixes the scrubber rewrites and the prefixes the leak check inspects have to be one list. Writing the --selftest exposed a second defect in the predicate: same-length substitution truncates a cwd shorter than the placeholder (/home/ab/x becomes /home/agent/p), which failed startswith() and read as a leak, making every short-cwd capture unsanitizable. is_placeholder() now accepts prefixes of the placeholder. All seven committed fixtures re-verified clean under it. The selftest runs the refusal path for real — it stubs out the redaction, pushes a leaking capture through, and stats the output path — and CI runs the selftest (air-1474-sanitize-ordering.test.ts, skipped where python3 is absent). NON-BLOCKING — classifyBuffer read buf.cursorY alongside top = buf.viewportY, assuming viewportY === baseY. Expected to be hygiene; measured as a real false clean. With the viewport scrolled up, the unconverted row lands on a stale composer still in scrollback — empty, palette-12, rule beneath it — so the region bounds and classifies `empty`: CLEAN, while the live composer is off-view holding a draft. Verified against the built old code ({clean: true, detail: 'empty'}) and the fix (no-composer-marker). baseY + cursorY - top removes it. Both new tests mutation-checked: revert either fix and the matching test fails. pnpm test: 276 files / 5479 tests pass, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mohidmakhdoomi
commented
Sep 3, 2026
Thanks — the blocking item was right, and chasing it turned up a second defect in the same file. Blocking: Writing the self-check exposed a second defect, in the predicate I had already "fixed" once. Same-length substitution truncates a cwd shorter than the placeholder, so Per your note about proving rather than asserting: Non-blocking, The regression test pins the rigging itself — that the viewport is genuinely off the bottom, that the two conventions disagree, and that the row the old reading picked really was a marker+rule pair — so it fails loudly rather than quietly becoming a tautology if xterm changes. Mutation-checked the same way. Non-blocking, not done here, and I agree it is a real gap: silent idle-session profile drift.
|
waleedkadous
left a comment
There was a problem hiding this comment.
Re-verified: sanitize_file now finds leaks before any write and refuses with nothing on disk, backed by a self-test that stats the path; /Users/… added to PATH_RE so the scrub set and the leak-check set are one list; and the short-cwd false-positive you found while writing the self-test is exactly the kind of second defect a real check surfaces. CI 7/7 on 8d9d666. Approving and merging.
# Conflicts: # codev/resources/arch.md
Uh oh!
There was an error while loading. Please reload this page.
Implements #1474.
What was actually wrong
AGY_MARKER = /^> /(gate-profiles.ts:72) treated any line starting with>as agy'scomposer prompt, and
findMarkerRowis last-match-wins. The issue called this "the weakest link"in an empirically-derived profile. Measured against a real, authenticated agy 1.1.13 under a
PTY at 110×32, it is worse than a theoretical looseness — non-composer
>rows are routine:>row the old marker picked>never matched/^> //)Three findings drove the design:
>line (SGR 34;1 → palette 4).Non-composer
>rows accumulate one per conversation turn.>, also palette-12, and renders BELOW thecomposer — so it won the marker scan outright. A color anchor alone does not separate
these; the cursor row does.
>, which right-trims to">"and nevermatched
/^> /— so the gate held every message to an agy in that mode forever. Apre-existing false-HOLD, fixed here, since the issue's ask is that the marker identify the
actual composer.
Markdown blockquotes turned out to render as
│, not>— so the issue's "quoted output" riskarrives via the turn echo rather than via blockquotes.
Honest scope of the defect. I could not reproduce an actual false-CLEAN from the captures: on
every mis-bounded frame the wrong region still happened to contain counted text, so the verdict
landed
busyanyway. What is demonstrated is that the classifier bounds the wrong region onreal screens, and that getting
busyout of a wrong region is luck rather than a guarantee. Thecorruption risk is latent, not observed — stated plainly rather than overclaimed.
The change
Two optional, per-app
GateProfileanchors — profile data, in keeping with the spike'sconstraint 9 — set only on agy, both measured:
markerRequiresCursorRow: true— the marker row must hold the buffer cursor. Only the liveinput row does; menu rows, dialog options and transcript echoes never do.
markerFgPalette: 12— the marker glyph's own color, which separates the composer from thepalette-4 turn echo.
Plus the marker separator relaxed to
/^>(\s|$)/for the bare->mode. claude/codex set neitheranchor and are byte-for-byte unaffected (pinned by a test).
Every failure direction is toward HOLD: a row that fails an anchor is simply not a marker, so
drift yields
no-composer-marker→ held and re-checked, never a false clean. Sustained holdsalready escalate through the mailbox liveness telemetry (
recordStreak).Verdict changes on the real captures
agy-idle.cleanagy-draft.busyagy-menu.busy/typed in the composer)agy-trust.busyagy-torn-echo.busyagy-turn-echo.cleanagy-baremarker.cleanCost of the tightening, measured
Sweeping every byte-prefix of a real stream showed 70 CLEAN frames before vs 9 after — alarming
until you notice byte-prefix sampling cuts mid-repaint, which production never does. Sampling
the way production actually classifies (wall-clock, against a live agy driven through boot → idle
→ streaming → settled, classifying the mirror every 200 ms) the two profiles are identical:
No measurable false-HOLD cost, and the bare-
>mode goes from permanently held to deliverable.Fixtures
The issue asked for captured real-agy fixtures across states, and agy is authenticated in this
environment, so the three synthesized Phase-3 fixtures are replaced by seven real captures
(idle / bare-marker / draft / menu / trust / turn-echo / torn frame). agy's banner embeds the
account email and session cwd; both are replaced with same-length placeholders so the rendered
screen stays byte-for-byte equivalent — no attribute is retouched. Each file is 1.7–8.7 KB, so
they are committed plain rather than gzipped (gzip is used for the multi-hundred-KB claude
replays). The torn-frame fixture is real bytes cut mid-repaint, which is the tear shape #1361
documents for the adopt seed.
Tests
55 pass in
render-gate.test.ts. New coverage: per-fixture verdicts for all seven states; thereason for each (a mis-bounded region that returns
busyis not the same as a correctly-boundedone); synthetic branch tests for each anchor independently, including the palette-12 dialog option
with the cursor on it — the one shape the anchors cannot reject, which pins that the occupancy
count still catches it; and a guard that claude/codex are unaffected by cursor position.
porch check 1474: build ✓ (13.1s), tests ✓ (28.7s).Notes for the reviewer
buffer.cursorYis viewport-relative, the same conventionisGhostCursorCellalready relies on. True wheneverviewportY === baseY(no manual scrollback),which holds on both gate paths.
>option.The trust dialog does not (measured), and the occupancy count plus the region-end guard both still
catch it — but it is the seam to watch if agy adds dialogs.
🤖 Generated with Claude Code