feat(harness): composer ↑/↓ arrow-key history — navigate prior user messages - #686
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history · 7 files · composer ↑/↓ user-message history
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/workflows/API. skip L4: native/harness change is the existing build-harness path; no artifact/SHA logic. skip L7: no BYO/config seams)
AGENTS.md read: yes (docs/feature-divide.md also read; plan #667 locks used as contract)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | Composer event scan (ui.zig ~574) intercepts ↑/↓ with no !busy guard. Plan #667 + this PR’s body + docs/harness-limits.md all say Busy → arrows pass through. |
Mid-turn, composer empty (normal after send). Press ↑. Last user prompt loads into the field. Operator was about to type a queued follow-up; they get history instead. Ctrl+Enter in the same loop is intentionally live while Busy (submitOrEnqueue); history was not supposed to ride that. |
“Scan was already unguarded.” True for Enter. History is a new intercept that the plan explicitly parked behind if (!busy). |
high |
| Major | L1 | Same scan ignores queue_editing_index. Plan #667: do not steal ↑/↓ from the queue-row editor. |
After send, composer is empty. Open a queued follow-up (queue_editing_index != null). Press ↑. Event is handled; historyApply writes prompt_buf, not queue_edit_buf. Queue editor never sees the key. |
“#666 not on this branch.” queue_editing_index / queue_band are already here. Same file already special-cases hydrate for that editor. |
high |
| Major | L1 | .repeat is marked handled but historyApply runs only on .down. Plan lock: step on .down and .repeat so a held ↑ walks. The new comment claims “matching shell readline.” |
Hold ↑ on an empty composer: one step, then further repeats are eaten. Caret does not move (good) and history does not walk (not readline). | “One step per press is enough; swallow repeat only to protect the caret.” That is a different product. The comment and plan both specify walk-on-hold. | high |
| Major | L1 | History is dropped only on n < prev_msg. Same file already documents that session hydrate can be clearMessages + push in one batch (msg_count >= prev_msg) — they added that guard for the queue editor, not for history. |
Session A, 2 rows, ↑ into history. Switch to session B with 8 rows in one host batch. history_index stays 0; composer still shows A’s text. Next ↑ loads B’s second-newest user. Plan DoD: hydrate drops history. |
“Switch always hits n=0 first.” The queue-band comment eight lines above this new clear says it does not. | high |
| Minor | L6 | userTextAt tests (composer_history.test.zig) set every row’s text to "x" and only assert non-null. They do not prove ordinal 0 is the last user. Plan test 2 is unmet. |
Invert the walk (oldest-first). Tests still pass. Newest-first is the product. | “The walk is obviously reverse.” The unit file exists to lock that. It does not. | high |
| Minor | L8 | historyApply stack-allocates [2048]KindText instead of bridge.RING_CAP. state.zig inserts history fields under the prev_queue_band_h doc comment, so that comment now describes the wrong variables. |
RING_CAP changes independently → overflow on msgs_buf[i]. New-hire reads the queue-band comment as history docs. |
“2048 == RING_CAP today.” Today. The named cap exists so this copy does not. | high |
Residual risk
No protocol bump; composer stays in Wasm (L3 intact). Draft save-on-enter and resetHistory on submit/blank-reject look right. userCount / step cases are real. Live canvas (operator rows 10–14 on #667) was not executed here. PR base may be behind current main (harness-limits.md also moved on #684) — rebase before merge.
Merge guidance
CONCERNS — do not merge without fixing or explicitly accepting the four Majors:
ui.zigevent scan — skip history intercept whenbusyorqueue_editing_index != null.- Call
historyApplyon.repeatas well as.down(or delete the “held arrow walks” claim from code + docs). - Drop history on the same hydrate/empty-queue paths that already reset the queue editor, not only
n < prev_msg. - Optional with (1): require composer focus if you want the docs’ “composer focused” row to be true.
What was not attacked
Live /harness session (hold-↑, Busy, session switch); zig build test-rich in this workspace (no Zig); build-harness conclusion on this head.
|
@_plan_comment.md |
Plan to address feedback on PR #686 (Status: DONE)Last updated: 2026-08-19 Round 7 — R7 review @
|
| # | Sev | Review item | Status |
|---|---|---|---|
| R7#1 | Minor (L8) | state.zig history_newest_fingerprint field doc still says "Load earlier preserves the newest user row → fingerprint matches → no drop" — R6 fixed ui.zig comments but left this one |
🟢 done |
R7#1 — Fix stale state.zig field doc (🟢 DONE)
- Review item: L8: "
state.zighistory_newest_fingerprintfield doc still says 'Load earlier preserves the newest user row → fingerprint matches → no drop.' R6 updated the twoui.zigcomments to the sliding-window truth and left this one." - Grounded problem:
state.zig:95: field doc onhistory_newest_fingerprintclaimed Load earlier preserves the newest user row unchanged → fingerprint matches → no drop. Load earlier is a sliding window (not a prepend), so the newest user's identity usually changes, and the fingerprint WILL mismatch. R6 already fixed this inui.zig;state.zigwas missed. - Fix: Updated field doc to match
ui.zig: "Load earlier is a sliding window — the newest user usually changes, so the fingerprint WILL mismatch and drop. Acceptable: ordinals name a different ring window after sliding; re-entering history shows the new window's rows."
Round 6 — R6 review @ c880d31
| # | Sev | Review item | Status |
|---|---|---|---|
| R6#1 | Major (L1) | n < prev_msg calls restoreDraftToPrompt unconditionally — wipes live draft on Clear / shorter hydrate when not in history |
🟢 done |
| R6#2 | Minor (L6) | historyApply .restore_draft is a third hand-rolled memcpy, not calling restoreDraftToPrompt |
🟢 done |
| — | Residual | Load earlier comment claimed fingerprint survives — actually a sliding window, fingerprint WILL mismatch | 🟢 done (comment fixed) |
R6#1 — Gate n<prev_msg restore on history_index!=null (🟢 DONE)
- Review item: L1: "
n < prev_msgcallsrestoreDraftToPromptunconditionally. The helper memsetsprompt_buffirst. Whenhistory_indexis already null,history_draft_lenis 0, so this isclearPrompt()on every ring shrink." - Grounded problem:
ui.zig~488:restoreDraftToPromptalways fires even whenhistory_indexis null (not in history). Same-or-longer hydrate via the fingerprint path (gated onhistory_index != null) correctly leaves a live draft alone — but shorter hydrate / New / Clear wipe it. Pre-PRprompt_bufsurvived Clear. - Fix: Gate the restore on
history_index != null. Always clear index + draft + fingerprint (both in-history and not). Comment updated: "Only restore the saved draft when actually in history — a live draft in prompt_buf is operator content and must survive ring shrink."
R6#2 — Route .restore_draft through restoreDraftToPrompt (🟢 DONE)
- Review item: L6: "
historyApply.restore_draftis a third hand-rolled memcpy of the helper R5 extracted so copies would stop diverging. ↓-past-newest still does not callrestoreDraftToPrompt." - Fix: Replaced the hand-rolled memcpy in
.restore_draftwith a call torestoreDraftToPrompt. The helper's caller still zeroshistory_draft_len+history_draft_buf(the helper only handles prompt_buf, not state cleanup). Now there is exactly one memcpy:restoreDraftToPromptcalled fromn < prev_msg(gated), fingerprint mismatch, and.restore_draft.
Residual — Load earlier comment fixed
- Problem: The comment claimed "Load earlier preserves the newest user row unchanged → fingerprint matches → history survives." Load earlier is a sliding window (not a prepend), so the newest user usually changes and the fingerprint WILL mismatch.
- Fix: Updated comments in both
historyApply(fingerprint recording) and the frame fingerprint block to correctly describe the sliding-window behavior. Drop on Load earlier is acceptable because ordinals name a different ring window after sliding.
Round 5 (all done)
| # | Sev | Review item | Status |
|---|---|---|---|
| R5#1 | Major (L1) | n < prev_msg block zeros history_index + draft without touching prompt_buf before fingerprint restore |
🟢 done |
| R5#2 | Minor (L6) | Drop+restore lives as two divergent copies; no host-testable helper | 🟢 done |
Round 4 (all done)
| # | Sev | Review item | Status |
|---|---|---|---|
| R4#1 | Major (L1+L6) | fingerprintMatch still a prefix compare; tests lock the false positive |
🟢 done |
| R4#2 | Major (L1) | Mismatch restore skipped when draft_len == 0 |
🟢 done |
Round 3 (all done)
| # | Sev | Review item | Status |
|---|---|---|---|
| R3#1 | Blocker (L1+L6) | history_newest_fp_len: u6 panics in Debug / wraps to 0 in ReleaseSmall |
🟢 done |
| R3#2 | Major (L1) | Fingerprint compare is prefix, not length+binary identity | 🟢 done |
| R3#3 | Minor (L1) | On drop, draft discarded — prompt_buf left with foreign history line | 🟢 done |
Round 2 (all done)
| # | Sev | Review item | Status |
|---|---|---|---|
| R2#1 | Major (L1) | Hydrate drop predicate too wide + too narrow | 🟢 done |
Round 1 (all done)
| # | Sev | Review item | Status |
|---|---|---|---|
| 1 | Major (L1) | No !busy guard |
🟢 done |
| 2 | Major (L1) | No queue_editing_index guard |
🟢 done |
| 3 | Major (L1) | .repeat handled but only on .down |
🟢 done |
| 4 | Major (L1) | History dropped only on n < prev_msg |
🟢 done (replaced in R2) |
| 5 | Minor (L6) | userTextAt tests uniform "x" |
🟢 done |
| 6 | Minor (L8) | [2048]KindText / doc comment drift |
🟢 done |
Progress log (append-only)
- 2026-08-19 — R1: all 6 items fixed in
92f0ab6; CI32210978863green - 2026-08-19 — R2 review CONCERNS on
92f0ab6: hydrate drop too wide/narrow (1 Major) - 2026-08-19 — Designed fingerprint approach (Strategy A): track newest user row text on entry
- 2026-08-19 — commit
f671289: fingerprint-based drop (+59/−10 in 3 files). CI32211799582green - 2026-08-19 — R3 review BLOCK on
f671289: u6 overflow (Blocker), prefix compare (Major), missing draft restore (Minor) - 2026-08-19 — commit
031407d: u6→u8 +fingerprintMatchpub helper + 7 unit tests + draft restore on mismatch drop (+71/−2 in 4 files) - 2026-08-19 — local gates green;
build-harnessCI32212978418green - 2026-08-19 — R4 review CONCERNS on
031407d:ok/okayprefix (Major), empty-draft restore skip (Major) - 2026-08-19 — commit
9e2729d:fingerprintMatchexact eql when fp.len < 64; mismatch restore always memsets + copies draft even when empty (+18/−9 in 3 files) - 2026-08-19 — R5 review CONCERNS on
9e2729d: n<prev_msg leaves prompt_buf holding foreign line (Major), no host-testable restore helper (Minor) - 2026-08-19 — commit
c880d31: extractedrestoreDraftToPromptintocomposer_history.zig; bothn < prev_msg+ fingerprint mismatch call sites use the identical helper. New/Clear/hydrate-to-shorter now always restores draft (including empty) + clears fingerprint. 3 new unit tests (29/29 pass).build-harnessCI32269433045green (~2m5s). - 2026-08-19 — R6 review CONCERNS on
c880d31: n<prev_msg wipes live draft when not in history (Major),.restore_drafthand-rolls its own memcpy (Minor), Load earlier comment is misleading (Residual) - 2026-08-19 — commit
669a2fd: gate n<prev_msg restore onhistory_index != null(feat(harness): composer ↑/↓ arrow-key history — navigate prior user messages #686 R6#1); route.restore_draftthroughrestoreDraftToPrompt(R6#2); fix Load earlier sliding-window comments (Residual). 1 file, +22/−16.composer_history30/30 pass.build-harnessCI32293224549green (~2m15s). - 2026-08-19 — R7 PASS WITH NOTES:
state.zigfield doc still claimed Load earlier preserves fingerprint (Minor) - 2026-08-19 — commit
ecb0465: fixhistory_newest_fingerprintfield doc to matchui.zigsliding-window description. Comment-only, 1 file, +3/−1.
Merge decision
All R1–R7 feedback items addressed. R7 nit (stale field doc) fixed. Ready for merge.
…drate drop, RING_CAP, distinct-text tests Address adversarial review #686: #1 Major (L1): Skip history intercept when busy or editing a queued item. Busy → arrows pass through to textEntry; queue-row editor owns the caret. #2 Major (L1): Same guard above covers queue_editing_index. #3 Major (L1): Call historyApply on .repeat as well as .down so a held arrow walks through history (matching shell readline). #4 Major (L1): Drop history state on hydrate-to-same-or-longer-session. The n < prev_msg block catches clear-to-zero; the new guard after shouldDropEditOnEmptyQueue catches clearMessages+push in one batch. Condition: prev_msg > 0, history active, ring changed, FIFO empty. #5 Minor (L6): Use distinct text strings in userTextAt tests so ordinal 0 → "last" and ordinal 1 → "first" are verifiable. #6 Minor (L8): Replace [2048]KindText with [bridge.RING_CAP]; move prev_queue_band_h doc comment to its own field in state.zig.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 2)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ 92f0ab6 (fixup on 1cc65b5)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/API. skip L4: existing build-harness path. skip L7: no BYO seams)
Prior review: CONCERNS on 1cc65b5 (busy, queue-edit, .repeat, hydrate, tests, RING_CAP)
Round-1 #1–#3 and the two Minors are fixed (!busy && queue_editing_index == null, historyApply on .repeat, distinct userTextAt strings, bridge.RING_CAP, prev_queue_band_h comment). Round-1 #4 is not — the new drop predicate is the remaining merge risk.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | New hydrate drop (ui.zig ~502: prev_msg > 0 && history_index != null && n != prev_msg && queuedCount()==0) is too wide and too narrow. Plan #667: Load earlier prepends at the oldest end; ordinal 0 is unchanged — do not exit history. Same-count session hydrate (clearMessages + push, n == prev_msg) is exactly the “same-or-longer” case the comment claims to catch, and this predicate misses it. Drop also wipes history_draft_buf without restoring prompt_buf. |
(1) Idle, ↑ into history (draft was “wip”). Click Load earlier. n grows, queue empty → drop. Draft is gone; composer still shows the loaded history line as if it were a live draft. (2) Session A, 8 rows, ↑ into history. Switch to session B, also 8 rows, one host batch. n == prev_msg → no drop. Next ↑ walks B from A’s ordinal; composer still shows A’s text. |
“Load earlier is rare while in history.” True, and still a locked plan case. “Same-count always hits n < prev_msg first.” The queue-band comment eight lines above says one-batch hydrate does not. |
high |
Closed from round 1
| Was | Now |
|---|---|
Major: no !busy |
Guarded |
Major: ignores queue_editing_index |
Guarded |
Major: .repeat handled but no step |
historyApply on .down and .repeat |
Minor: userTextAt all "x" |
"first" / "last" asserted |
Minor: [2048] / stolen comment |
RING_CAP; comment on prev_queue_band_h |
Residual risk
No composer-focus check (docs still say “composer focused”; Ctrl+Enter is the same pre-existing pattern). Held-↑ walks and stack-allocates [RING_CAP]KindText per repeat — fine at key-repeat rate. No protocol bump; composer stays in Wasm. Live hold-↑ / Load earlier / same-count session switch not executed here.
Merge guidance
CONCERNS — do not merge until the drop rule matches the plan:
- Do drop on New / Clear / true session hydrate (including same-length replace).
- Do not drop on Load earlier (
ngrows at the oldest end; ordinal 0 is still the newest user). - If you drop, either restore the draft into
prompt_bufor leave the draft intact until an explicit exit (↓ past 0 / send).
Ring-length + empty FIFO cannot tell those cases apart. Need a session-generation / hydrate flag from the existing host switch path, or drop only when the newest user row’s identity changed in a way Load earlier cannot cause.
What was not attacked
Live /harness operator path; zig build test-rich in this workspace (no Zig); build-harness on 92f0ab6.
… catches same-count hydrate (R2 #686)
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 3)
Verdict: BLOCK
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ f671289 (fingerprint drop on 92f0ab6)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/API. skip L4: existing build-harness path. skip L7: no BYO seams)
AGENTS.md read: yes (docs/feature-divide.md also read)
Prior: R1 CONCERNS on 1cc65b5 · R2 CONCERNS on 92f0ab6 (hydrate drop). R1 #1–#3 + Minors stay fixed. R2’s drop is not fixed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Blocker | L1 + L6 | history_newest_fp_len is u6 (0–63) but the buffer is [64]u8 and the write uses @min(text.len, fingerprint.len) → 64. state.history_newest_fp_len = @intCast(fplen) is out of range whenever the newest user row is ≥ 64 bytes. Same class as the table.zig u6 overflow this repo already burned on. No test touches this field. |
(1) Debug (-Doptimize=Debug, the local gate they ran): empty composer, newest user is any normal prompt ≥ 64 B, press ↑. @intCast panics. History never works. (2) Shipped wasm (build.sh --release=small): intCast wraps to 0. Frame guard is history_index != null and history_newest_fp_len > 0 → never runs. Same-count / longer session hydrate (batched clearMessages+push; frame() never sees n=0) does not drop. Composer keeps session A’s loaded line; next ↑ walks session B. R2 Major is back for every real prompt. |
Defender: “64 fits if we meant 63-byte prefix” / “Release won’t panic.” Fails: they @min against .len (64), not 63; u6 cannot store 64. ReleaseSmall does not panic — it zeros the length and disables the only hydrate-drop they added. test-rich never executes historyApply / the frame guard, so CI stays green. |
high |
| Major | L1 | Even after a valid length, the compare is a prefix, not identity: eql(fp, m.text[0..min(m.text.len, fp.len)]). A 2-byte stored "ok" matches "okay rewrite the tests". |
Session A last user "ok" (or "fix", "yes"). ↑ into history. Switch to session B whose newest user starts with that string (common). fp_match == true → no drop. Composer still shows A; next ↑ loads B. |
Defender: “short exact collisions are inherent to Strategy A.” Distinct from exact-match residual: this matches any longer message with that prefix. A correct check is ncmp == fp.len and eql(fp, text[0..ncmp]) (and for both >64, equal 64-byte prefixes — the documented residual). |
high |
| Minor | L1 | On drop (n < prev_msg and fingerprint miss) they zero history_index + draft and leave prompt_buf. R2 already required restore-draft or leave draft until explicit exit. Neither happened. |
Fingerprint finally mismatches (or New/Clear). Draft "wip" is discarded. Composer still holds A’s loaded history line as if it were a live draft. Ctrl+Enter sends A’s prompt into B. ↑ will not re-enter history (buf non-empty). |
Defender: operator can see/edit the text. Fails: they just “exited” history; the standing draft is gone; the leftover is the other session’s row. | high |
Closed from rounds 1–2
| Was | Now |
|---|---|
R1 Major: no !busy |
Still guarded |
R1 Major: ignores queue_editing_index |
Still guarded |
R1 Major: .repeat no step |
Still steps |
R1 Minor: userTextAt all "x" |
Still "first" / "last" |
R1 Minor: [2048] / stolen comment |
Still RING_CAP |
| R2 Major: count-based drop too wide/narrow | Replaced by a fingerprint that cannot store its own length |
Residual risk
Exact same short newest-user text across sessions ("ok"/"ok") still collides even with a length-checked compare — Strategy A has no session id (they rejected a protocol bump). Load earlier still relies on newest-user text being byte-identical after a full hydrateMessages replace (true today). [RING_CAP]KindText per arrow is fine at key-repeat; per-frame newest-user walk is typically 1–few slots. No composer-focus check (docs say “composer focused”; Ctrl+Enter is the same global dvui.events() scan). Live hold-↑ / session switch / Load earlier not executed here. test-rich does not cover historyApply or the frame drop.
Merge guidance
BLOCK — do not merge until the Blocker is fixed:
- Store
history_newest_fp_lenin a type that can hold 64 (u8/usize). Do not@intCastamin(..., 64)intou6. - Compare length + bytes, not a prefix (
ncmp == fp_len && eql). - On drop: restore the saved draft into
prompt_buf(or clear the field). Do not leave the foreign history line as a fake draft. - Add a host-testable helper for the fingerprint compare (len 63 vs 64, prefix
"ok"vs"okay…", equal long prefix) sotest-richcan fail this class. The frame path is currently untested.
What was not attacked
Live /harness operator path; zig build test-rich in this workspace (no Zig); runtime of the ReleaseSmall wasm on a ≥64 B newest user row (the intCast is traced, not executed here).
…ft restore, helper + 7 tests (#686 R3) R3 Blocker: history_newest_fp_len u6 (0-63) panicked in Debug for >=64B text; ReleaseSmall wrapped to 0 disabling the drop entirely (any-text eql([0]u8{},text[0..0])==true). R3 Major: eql prefix match now uses fingerprintMatch helper with length guard (fp_len==0 returns false instead of matching any text). R3 Minor: on fingerprint-mismatch drop, restore saved draft to prompt_buf so the operator's pre-history prompt survives a session switch. Tests: 7 fingerprintMatch cases (empty, shorter, exact, prefix, 63-byte, 64-byte) so test-rich can fail the 0-length / prefix class.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 4)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ 031407d (u8 + fingerprintMatch + draft restore on f671289)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no trust-boundary change; L4: no CI/workflow edit; L7: no deploy bind)
AGENTS.md read: yes
Prior: R1 CONCERNS · R2 CONCERNS · R3 BLOCK (u6 overflow)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L6 | fingerprintMatch is still a prefix compare. The doc comment says the opposite (fp="ok" does not match "okay…"). Tests lock the false positive (composer_history.zig · fingerprintMatch; tests at composer_history.test.zig “prefix → true” and “63 vs 64 → true”). |
Session A newest user is ok. ↑ enters history (fp_len=2). Switch to B whose newest user is okay ship it. fingerprintMatch("ok", "okay ship it") is true → no drop. Composer still holds A’s history line; Ctrl+Enter sends it in B. Same for any short newest (yes/no/thanks) vs a longer message that starts with those bytes. When fp_len < 64 the buffer already holds the entire message — exact eql(fp, candidate) is two lines and rejects this. The only inherent Strategy A residual is two different ≥64 B messages that share the first 64 bytes. |
“Documented Strategy A residual / no session id.” Residual is same-text or 64-byte-truncated prefix — not ok vs okay. The function’s own comment claims length identity; the tests assert the bug. R3#2 is not done. |
high |
| Major | L1 | Fingerprint-mismatch drop restores prompt_buf only when history_draft_len > 0 (ui.zig frame drop). Empty-composer ↑ saves draft_len=0, then a real mismatch leaves the foreign history line in prompt_buf with history_index=null (looks like a typed draft). historyApply .restore_draft always memsets the prompt first; this path does not. |
Empty composer → ↑ (load A’s newest) → switch to B whose newest user does not prefix-match → drop runs, skip restore → prompt still shows A’s line → send posts it to B. | “Operator will notice and delete.” Same send-ready foreign text as R3#3, which this commit claimed to close. Half-fixed. | high |
Closed from rounds 1–3
| Was | Now |
|---|---|
R3 Blocker: history_newest_fp_len: u6 Debug panic / ReleaseSmall wrap-to-0 |
u8; @intCast(min(len, 64)) is in range. fp_len==0 → fingerprintMatch false. |
R1 busy / queue-edit / .repeat / RING_CAP / distinct userTextAt strings |
Still in place. |
| R2 hydrate drop via fingerprint (Load earlier vs session switch) | Mechanism remains; compare is still too wide (Finding 1). |
| R3 Minor: restore draft on drop | Only the non-empty-draft branch (Finding 2). |
Residual risk
Two different sessions whose newest user text is byte-identical (or both ≥64 B and share the first 64) still will not drop — that is the real Strategy A ceiling without a session id. Frame-path drop/restore is untested (only the pure helper). No zig in this review workspace; author reports build-harness green on 031407d.
Merge guidance
CONCERNS — do not merge until:
fingerprintMatch: iffp.len < 64, require exacteql(fp, candidate)(same length + bytes). Iffp.len == 64, keep first-64 compare (truncated). Flip the"ok"/ 63-vs-64 tests tofalse. Make the doc comment match the code.- On mismatch drop, always
memsetprompt_bufthen copy the saved draft (including empty) — same as.restore_draft.
Do not treat this as --approve.
What was not attacked
Live Preview session switch, Load earlier on a full ring, host hydrateMessages against a real wasm, operator ↑/↓ on a phone, full vitest suite.
|
R4 fixes on
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 5)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ 9e2729d (exact-fp + empty-draft restore on 031407d)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/API. skip L4: existing build-harness path; CI green on this head. skip L7: no BYO seams)
AGENTS.md read: yes (docs/feature-divide.md also read)
Prior: R1 CONCERNS · R2 CONCERNS · R3 BLOCK · R4 CONCERNS (ok/okay prefix + empty-draft skip)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | n < prev_msg (ui.zig ~465) nulls history_index and zeros the draft without touching prompt_buf, before the R4 fingerprint restore (~524, gated on history_index != null). New/Clear and hydrate-to-shorter session never reach the memset. The comment on that block still says “ring cleared only (count→0)” / “fingerprint catches same-or-different-count hydrate” — both are false: inv_clear_messages + push in one batch (or onClear’s unbatched clear + system row) lands here with 0 < n < prev_msg. |
(1) Session A, 20 rows. Empty composer, ↑ loads A’s newest user. New / Clear (HarnessHost.onClear: clearMessages() then pushMessage(System, "New session started."), no batch). Frame sees n < prev_msg. Index dropped, composer still shows A. Ctrl+Enter sends A into the new session. (2) Same ↑, then switch to session B with fewer rows (pushSessionToBridge → batched hydrateMessages). n=3 < prev=20 takes the same block; fingerprint restore is skipped because index is already null. Composer still shows A; send posts it to B. Hydrate-to-same-or-longer is the only path that hits R4. |
“R4 always memsets prompt on mismatch.” Only after this block leaves history_index set. New/Clear and shorter hydrate do not. “New remounts wasm / onInit zeros the field.” False: onClear is clearMessages + push, not onInit. “Operator will notice and delete.” Same send-ready foreign line R3#3 / R4#2 already rejected; plan DoD is “no stale text” on New / Clear / session switch. |
high |
| Minor | L6 | Drop+restore still lives only as two divergent copies in frame() (n < prev_msg vs fingerprint). fingerprintMatch was extracted so test-rich could fail the compare class; there is still no host-testable helper for “exit history and put the saved draft (including empty) back in prompt_buf.” |
Invert or omit the memset on either site. composer_history tests stay green. This is how R4’s restore shipped on the uncommon path and missed the common one. |
“frame() cannot import into test-rich.” Same constraint they already solved by extracting fingerprintMatch. A 10-line dropRestore(prompt, draft) next to it would have failed this round. |
high |
Closed from rounds 1–4
| Was | Now |
|---|---|
R4 Major: fingerprintMatch prefix (ok ≡ okay…); tests locked the false positive |
Exact eql when fp.len < 64. "ok" / 63-vs-64 tests are false. Doc matches. |
R4 Major: mismatch restore skipped when draft_len == 0 |
Fingerprint path always @memsets prompt_buf then copies draft (including empty). Preempted by Finding 1 on New/Clear and shorter hydrate. |
R3 Blocker: history_newest_fp_len: u6 |
Still u8. |
R1 busy / queue-edit / .repeat / RING_CAP / distinct userTextAt |
Still in place. |
Residual risk
Strategy A still cannot tell two sessions whose newest user text is byte-identical (or both ≥64 B and share the first 64) — no session id, no protocol bump. historyApply / both frame drop sites remain untested except via the pure helper. Draft-on-entry is always empty under the locked empty-to-enter gate; restore is “clear the field.” Docs say “composer focused”; the scan is the same global dvui.events() as Ctrl+Enter. Live New / session switch / hold-↑ not executed here.
Merge guidance
CONCERNS — do not merge until the Major is fixed:
- Do not null
history_indexin then < prev_msgblock (or, if you do, run the same restore as the fingerprint path: memsetprompt_buf, copy draft including empty, then clear index + draft + fingerprint). The fingerprint check already handles “no newest user” (fp_matchstays false) for Clear-to-empty. - One helper, both call sites. Do not leave a third copy of “drop history” that forgets the field.
Do not treat this as --approve.
What was not attacked
Live /harness New / session switch / Load earlier; operator ↑/↓ on a phone; zig build test-rich in this workspace (no Zig). build-harness is green on 9e2729d.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 6)
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ c880d31 (restoreDraftToPrompt on 9e2729d)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/API. skip L4: existing build-harness path; CI green on this head. skip L7: no BYO seams)
AGENTS.md read: yes (docs/feature-divide.md also read)
Prior: R1 CONCERNS · R2 CONCERNS · R3 BLOCK · R4 CONCERNS · R5 CONCERNS (n < prev_msg left the foreign history line)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | n < prev_msg (ui.zig ~465) calls restoreDraftToPrompt unconditionally. The helper memsets prompt_buf first. When history_index is already null, history_draft_len is 0, so this is clearPrompt() on every ring shrink. The fingerprint path (~533) is correctly gated on history_index != null — same-or-longer hydrate leaves a live draft alone. Shorter hydrate / New / Clear do not. |
(1) Session A, 20 rows. Type ship the review (not in history). Switch to session B with 3 rows (pushSessionToBridge → batched hydrateMessages). Frame sees 3 < 20. Empty-draft restore zeros the field. The draft is gone; same-or-longer B would have kept it. (2) Same typing, click New / Clear (onClear: unbatched clearMessages + system row). Same block. Pre-PR prompt_buf survived Clear. |
“R5 said always restore so the foreign history line cannot linger.” That applies while in history. Empty-to-enter means the saved draft is always "", so restore-while-in-history is a memset — and should stay gated on history_index != null. “New should empty the composer.” Then same-or-longer session switch would too; it does not. The fingerprint block is the existence proof. |
high |
| Minor | L6 | historyApply .restore_draft (ui.zig ~108) is a third hand-rolled memcpy of the helper R5 extracted so copies would stop diverging. ↓-past-newest still does not call restoreDraftToPrompt. |
Change the helper (NUL policy, clamp) and ↓-exit drifts from New/Clear/fingerprint drop — the exact class that produced R4/R5. | “The copies are equivalent today.” They were equivalent on the fingerprint path in R4 too, until empty-draft was skipped on one site. | high |
Closed from rounds 1–5
| Was | Now |
|---|---|
R5 Major: n < prev_msg drops index/draft and leaves prompt_buf holding the foreign history line |
In-history path now memsets via restoreDraftToPrompt. Not-in-history path now memsets a live draft (Finding 1). |
R5 Minor: two divergent drop+restore copies in frame() |
Both frame sites call the helper. historyApply .restore_draft does not (Finding 2). |
R4 Major: ok ≡ okay…; empty-draft skip on fingerprint mismatch |
Exact eql when fp.len < 64. Fingerprint path always restores, including empty. |
R3 Blocker: history_newest_fp_len: u6 |
Still u8. |
R1 busy / queue-edit / .repeat / RING_CAP / distinct userTextAt |
Still in place. |
Residual risk
Strategy A still cannot tell two sessions whose newest user text is byte-identical (or both ≥64 B and share the first 64) — no session id. Load earlier is a sliding window (slice(start, start+RING_MAX)), not a prepend; the newest user usually changes, so the fingerprint drops on Load earlier despite comments claiming it survives. That drop is defensible (ordinals would name a different window) — the comments are what is wrong. historyApply / both frame sites remain untested except via the pure helper. Docs say “composer focused”; the scan is the same global dvui.events() as Ctrl+Enter. Live New / session switch / hold-↑ not executed here.
Merge guidance
CONCERNS — do not merge until the Major is fixed:
- Gate the
n < prev_msgrestore onhistory_index != null(then clear index + draft + fingerprint either way). Do notmemsetprompt_bufwhen the operator was never in history. - Route
historyApply.restore_draftthroughrestoreDraftToPromptso there is one memcpy.
Do not treat this as --approve.
What was not attacked
Live /harness New / session switch / Load earlier; operator ↑/↓ on a phone; zig build test-rich in this workspace (no Zig). build-harness is green on c880d31.
…te .restore_draft through helper (#686 R6)
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #686 (round 7)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/composer-arrow-history @ 669a2fd (R6 gate + helper on c880d31)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no secrets/API. skip L4: existing build-harness path; CI green on this head. skip L7: no BYO seams)
AGENTS.md read: yes (docs/feature-divide.md also read)
Prior: R1 CONCERNS · R2 CONCERNS · R3 BLOCK · R4 CONCERNS · R5 CONCERNS · R6 CONCERNS (unconditional restore wiped live drafts)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8 | state.zig history_newest_fingerprint field doc still says “Load earlier preserves the newest user row → fingerprint matches → no drop.” R6 updated the two ui.zig comments to the sliding-window truth and left this one. |
Next edit to the drop predicate trusts the field doc, treats a Load-earlier mismatch as a bug, and “fixes” it by ignoring fingerprint on grow-or-same n — same-count session hydrate slips through again (R2). |
“Behavior in frame() is correct; comments in ui.zig are right.” The field is what a new-hire reads when touching drop state. R6 claimed this residual was closed. |
high |
Closed from rounds 1–6
| Was | Now |
|---|---|
R6 Major: n < prev_msg restore unconditional — wipes live draft on Clear / shorter hydrate |
Gated on history_index != null. Not-in-history leaves prompt_buf. In-history still memsets via restoreDraftToPrompt. |
R6 Minor: historyApply .restore_draft hand-rolled memcpy |
Calls restoreDraftToPrompt. Three restore sites, one helper. |
R5 Major: shrink path left the foreign history line in prompt_buf |
In-history shrink restores (empty draft → clear). |
R4 Major: ok ≡ okay…; empty-draft skip |
Exact eql when fp.len < 64. Mismatch always restores. |
R3 Blocker: history_newest_fp_len: u6 |
Still u8. |
R1 busy / queue-edit / .repeat / RING_CAP / distinct userTextAt |
Still in place. |
Residual risk
Attack did not breach a runtime path with current evidence. Remaining ceiling is Strategy A: two sessions whose newest user text is byte-identical (or both ≥64 B and share the first 64) will not drop on same-or-longer hydrate — no session id, no protocol bump. Load earlier is a sliding window and will drop; that is now described in ui.zig (not in state.zig). historyApply / both frame sites remain untested except via the pure helper. Docs say “composer focused”; the scan is the same global dvui.events() as Ctrl+Enter. Live New / session switch / hold-↑ / Load earlier not executed here.
Merge guidance
PASS WITH NOTES — safe to merge from this attack. Flip the state.zig field doc to match ui.zig (Load earlier mismatches and drops) or accept the leftover. Do not treat this as --approve.
What was not attacked
Live /harness New / session switch / Load earlier; operator ↑/↓ on a phone; zig build test-rich in this workspace (no Zig). build-harness is green on 669a2fd. GitHub reports MERGEABLE / CLEAN vs main @ b49bf54.
…is a sliding window, not a prepend (#686 R7)
Summary
Implements plan #667: composer arrow-key history — navigate prior user messages with ↑/↓.
Closes #667
What changed (7 files, +366/−1)
native/harness/src/ui/composer_history.ziguserCount,userTextAt(newest-first),stepmachinenative/harness/src/ui/composer_history.test.zignative/harness/src/ui/state.zighistory_index,history_draft_buf,history_draft_len; cleared inresetTranscriptScrollnative/harness/src/ui/composer.zigresetHistory()helper; wired intosubmitText+submitOrEnqueue(both success + blank-reject paths)native/harness/src/ui.zighistoryApply()helper in the frame module; ↑/↓ intercept in the composer event scan (before textEntry); history clear inn < prev_msgpathnative/harness/build.zigcomposer_historytest intotest-richdocs/harness-limits.mdBehavior
Caps
No new named cap. No existing cap changed. History window is the live ring (
messageCount()≤ 2048).history_draft_bufisSUBMIT_CAP(256 KiB) — same asprompt_buf.Gates
zig fmt --checkzig build test-richcomposer_historypassed (18/18); 3 pre-existing font-dependent failures (paint_diff, model_picker_layout, transcript_split_layout —--listen=-sandbox only)zig build test-rich-invariantszig build harness -Doptimize=Debugbuild-harnessCI