feat(harness): str_replace L2 visible EMBER/TEAL bands, pin-open during turn - #716
Conversation
…ng turn, cached paint Fix #714 / plan #715. #692 painted ink-only bands (textLayout covered the box fill; *_surface is near-black on teal_bg). Use ember_border/teal_border fills, .background=false on inner layouts, split sides at decode, cache_layout + mono addText, and pin L0/L2 open while Busy via thinking_collapse membership.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Plan #715 implemented on this branch. Zig gates are CI-only ( Operator smoke after Preview wasm: in-flight |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #716
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/str-replace-l2-fix @ 3e59a42 · 3 files · str_replace L2 visible bands + active-turn pin + decode-time sides
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no trust-boundary / emit / redact / protocol change. skip L4: no workflow edit; zig → harness.wasm already ran test-rich + wasm. skip L7: no deploy bind)
AGENTS.md read: yes · docs/feature-divide.md yes (paint stays in-canvas; no DOM chat)
Findings
No surviving Blocker / Major after self-refutation. Host addToolStart still emits detail: '' while running, so L2 pin + cache_layout only engage on the complete (ok/fail) payload — sides_stable is true on first band paint. Pin helpers match thinking_collapse.State.isActiveTurnFull (cleared on Busy→ready/err). HashMap is not written while pinned, so turn-end collapses. Item.str_replace_sides is slot-cached at decode; slices die with Decoded. Palette stays on named tokens (ember_border / teal_border fills, muted rims, ember_text / teal_text ink). build-harness is green.
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1 | native/harness/src/ui/toolrun.zig · paintToolRun reuses (src, item_base+2) for the no-detail textLayout and the has-detail expander. This PR makes that type swap the default path: L0 is pinned open while str_replace is still running (static label), then the same id becomes an expander the frame detail lands. |
In-flight str_replace: L0 open, L1 is textLayout at +2. Tool completes → expander mounts at the same (paintToolRun src, +2). First completed frame can steal persist/hit data from the textLayout slot (wrong caret, one-frame dead hit target on the L1 row). L2 body still paints (l2_expanded is computed before the widget). |
Defender: mutually exclusive in one frame; dvui may reset on kind change; operator-opened L0 during a running exec already did this. Fails because both widgets take the caller src (not their own @src()) + the same extra, and this PR makes the swap the happy path for every in-flight str_replace, not a rare manual open. Distinct extras (keep +2 expander, static label on a free slot) close it. |
medium |
| Minor | L8 | docs/harness-limits.md Tool-run Two-level expand still ends “Clicking a row toggles; second click collapses; per-item isolation.” Header (default) still says default-collapsed with no exception. Open state (same table) says L0/L2 are pinned for in-turn str_replace and click cannot collapse. |
Next harness edit follows Header / Two-level, treats the pin as a regression, and “fixes” it by writing the HashMap again — #714 reappears (collapsed / click-to-see). |
Open state row is accurate. Fails as living-docs contradiction: two sentences in the same table describe opposite click policy. The toggle sentence needs the pin exception (or a pointer at Open state). | high |
| Nit | L6 | native/harness/src/rich/toolrun.zig · ui id packing is unique… still uses slot_max = 4 while paint mounts +5..+8. |
A later extra at +5 is already used; the test will not catch a new alias inside 5..8. 200·1024+8 < 1000003 still holds, so today’s extras do not collide. |
#692 Nit fixed the paint comment (+0..+8); the packing test was not updated. Math is still safe. Documentation-of-the-lock only. |
high |
Residual risk
Attack did not breach a runtime path with current evidence.
- Band fills (
ember_border#3a1e18/teal_border#152528onteal_bg#050a0c) are still a small lift. Visibility now hangs on fill-through (.background = false) plusember_muted/teal_muted1px rims. If Preview still reads ink-only, the plan’s escalation isember_muted/teal_mutedfill — still palette-only. Operator checklist in the PR body is unchecked. - Side bodies are mono
addText(plan-locked to kill the#692stall).→/ emoji / CJK in the replaced text will tofu; the else-branch still usesaddTextMixed. Restoring mixed-face on sides reopens the stall. - Empty-new TEAL strip still depends on dvui honoring
min_size_content.h = fontMono().lineHeight()on a box whose childaddTexts"". Splitter tests locknew.len==0; they do not paint. cache_layoutis keyed onstatus != .running, not decode-cache hit. Host never streamsstr_replacedetail while running, so the first band paint is already committed. A future host that grows a completed item’s detail in place would trip DVUI’s append-only-prefix assert.- No 390 px wrap / no-h-gutter proof in CI. Preview wasm not exercised in this review.
Merge guidance
PASS WITH NOTES — safe to merge from this attack after the operator smoke (in-flight pin + visible bands, canvas stays interactive, turn-end collapse, click-during-Busy cannot keep it closed, delete strip, exec/read_file still one unpinned block). Minors are should-fix-soon (distinct +2 extras; docs toggle sentence). Nit optional.
zig → harness.wasm is green (test-rich includes src/rich/toolrun.zig). Do not treat this as --approve.
What was not attacked
Live Preview paint, restored-session L2 after refresh, Copy of the still-sentinel payload, dvui testing-backend layout of the empty new box / +2 type swap, full vitest (N/A — no TS), DO runner beyond the already-green build-harness job.
|
Landed the adversarial-review notes on
Zig gates are CI ( |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #716
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/str-replace-l2-fix @ f1d7e95 · 3 files · str_replace L2 visible bands + active-turn pin + decode-time sides (re-review after 3e59a42 notes)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no trust-boundary / emit / redact / protocol change. skip L4: no workflow edit; zig → harness.wasm green on this SHA — test-rich + wasm. skip L7: no deploy bind)
AGENTS.md read: yes · docs/feature-divide.md yes (paint stays in-canvas; no DOM chat)
Findings
No surviving Blocker / Major after self-refutation. Prior review on 3e59a42 (L1 +2 persist reuse, L8 Header/Two-level vs Open-state contradiction, L6 slot_max = 4) is closed on this SHA: no-detail L1 is extra +9, expander stays +2; Header / Two-level point at the Busy-turn pin; packing test slot_max = 9.
Host addToolStart still emits detail: '' while running, so L2 pin + cache_layout only engage on the complete (ok/fail) payload — sides_stable is true on first band paint (widgets +4/+6/+8 did not exist while running; DVUI byte_heights empty → no CacheLayoutError). Pin helpers match thinking_collapse.State.isActiveTurnFull (physical-slot range, cleared on Busy→ready/err, including a same-frame tryPromoteQueued). HashMap is not written while pinned, so turn-end collapses L2. Item.str_replace_sides is slot-cached at decode; slices die with Decoded. Palette stays on named tokens (ember_border / teal_border fills, muted rims, ember_text / teal_text ink). build-harness is green.
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L8 | native/harness/src/ui/toolrun.zig · paintToolRun calls state.thinking_collapse_state.isActiveTurnFull with no call-site note that the module is turn-membership (physical slot range), not thinking-specific. Plan #715 locked “Comment at the call site. Do not rename.” |
Next harness edit treats the thinking_* name as a layering bug and deletes the import / pin — #714 collapsed L0/L2 returns. |
Defender: Open state in docs/harness-limits.md and toolRunL0PinnedOpen already state the policy. Fails only as a plan-locked comment, not a runtime hole. |
high |
Residual risk
Attack did not breach a runtime path with current evidence.
- Band fills (
ember_border#3a1e18/teal_border#152528onteal_bg#050a0c) are still a small lift. Visibility now hangs on fill-through (.background = false) plusember_muted/teal_muted1px rims. If Preview still reads ink-only, the plan’s escalation isember_muted/teal_mutedfill — still palette-only. Operator checklist in the PR body is unchecked. - Side bodies are mono
addText(plan-locked to kill the#692stall).→/ emoji / CJK in the replaced text will tofu; the else-branch still usesaddTextMixed. Restoring mixed-face on sides reopens the stall. - Empty-new TEAL strip still depends on dvui honoring
min_size_content.h = fontMono().lineHeight()on a box whose childaddTexts"". Splitter tests locknew.len==0; they do not paint. cache_layoutis keyed onstatus != .running, not decode-cache hit (markdown is stricter: hit only). Host never streamsstr_replacedetail while running, and side widgets mount only once detail exists, so the first band paint has no priorbyte_heights. A future host that grows a completed item’s detail in place, or a ring-index shift that reuses(src, extra)with different text, wouldlogError(CacheLayoutError)and drop the height cache for a frame — not a panic.- Click-during-pin: dvui
expandertoggles the passed pointer and paints the caret before the pin re-assertsexpanded = true. List stays open the same frame; caret can flicker one frame. Same pattern asui/thinking.zig. Click cannot keep it closed. - A 200-item rolled group that contains one
str_replacestays L0-open (all L1 rows) for the rest of the Busy turn. Plan-locked; L2 still only thestr_replaceitem(s). - No 390 px wrap / no-h-gutter proof in CI. Preview wasm not exercised in this review.
Merge guidance
PASS WITH NOTES — safe to merge from this attack after the operator smoke (in-flight pin + visible bands, canvas stays interactive, turn-end collapse, click-during-Busy cannot keep it closed, delete strip, exec/read_file still one unpinned block). Nit optional.
zig → harness.wasm is green on f1d7e95 (test-rich includes src/rich/toolrun.zig). Do not treat this as --approve.
What was not attacked
Live Preview paint, restored-session L2 after refresh, Copy of the still-sentinel payload, dvui testing-backend layout of the empty new box / click-during-pin caret, full vitest (N/A — no TS), DO runner beyond the already-green build-harness job.
|
Addressing the re-review (f1d7e95) Nit (L8): added a call-site comment at No planning comment warranted this round — single Nit, obvious localized doc fix (skill §0). Zig gates local + |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #716
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/str-replace-l2-fix @ da654c6 · 3 files · str_replace L2 visible bands + active-turn pin + decode-time sides (re-review after 3e59a42 / f1d7e95 notes)
Lenses run: L1, L3, L5, L6, L8, L9 (skip L2: no trust-boundary / emit / redact / protocol change. skip L4: no workflow edit; zig → harness.wasm green on this SHA. skip L7: no deploy bind)
AGENTS.md read: yes · docs/feature-divide.md yes (paint stays in-canvas; no DOM chat)
Findings
No surviving Blocker / Major / Minor / Nit after self-refutation.
Prior notes on this PR are closed on da654c6: L1 +2 persist reuse (static label is +9, expander stays +2); L8 Header / Two-level vs Open-state contradiction; L6 packing slot_max = 9; L8 call-site note that isActiveTurnFull is turn-membership, not thinking-only.
Traced on this SHA:
- Decode assigns
Item.str_replace_sidesfrom a real encoded line (unescapepath); slices point into owneddetail;execstays null. Paint reads the field; it does not re-split. - L0 pin is name-only (
has_str_replace); L2 pin is active +str_replace+has_detail. HashMap is not written while pinned → turn-end collapse with empty maps. isActiveTurnFull(slot, messageHead(), RING_CAP)uses the physical slot frommessageSlotAt, same asthinking.zig. Idle/slot == null→ not pinned.- Host still emits empty
detailwhilerunning, so L2 widgets+4/+6/+8first mount on the complete payload withcache_layouttrue and empty DVUIbyte_heights. - Fills are named tokens (
ember_border/teal_border, muted rims,ember_text/teal_textink). InnertextLayout.background = false. Side bodies are monoaddText. EMBER is removed-line semantics (AGENTS exception), not error chrome. - New
test-richrows cover decode sides, L0/L2 pin matrix, and extras+0..+9.build-harnessis green.
Residual risk
Attack did not breach a runtime path with current evidence.
- Band fills (
ember_border#3a1e18/teal_border#152528onteal_bg#050a0c) are still a small lift. Visibility hangs on fill-through plus 1px muted rims. If Preview still reads ink-only, the plan’s escalation isember_muted/teal_mutedfill — palette-only. Operator checklist in the PR body is unchecked. - Side bodies are mono
addText(plan-locked to kill the#692stall).→/ emoji / CJK in replaced text will tofu; the else-branch still usesaddTextMixed. Restoring mixed-face on sides reopens the stall. - Empty-new TEAL strip still depends on dvui honoring
min_size_content.h = fontMono().lineHeight()on a box whose childaddTexts"". Splitter tests locknew.len==0; they do not paint. - Click-during-pin: expander toggles the pointer and paints the caret before the pin re-asserts
expanded = true. List stays open the same frame; caret can flicker one frame. Same pattern asui/thinking.zig. Click cannot keep it closed. - A 200-item rolled group that contains one
str_replacestays L0-open (all L1 rows) for the rest of the Busy turn. Plan-locked; L2 still only thestr_replaceitem(s). cache_layoutis keyed onstatus != .running, not decode-cache hit. A future host that grows a completed item’s detail in place wouldlogError(CacheLayoutError)for a frame — not a panic.- No 390 px wrap / no-h-gutter proof in CI. Preview wasm not exercised in this review.
Merge guidance
PASS WITH NOTES — safe to merge from this attack after the operator smoke (in-flight pin + visible bands, canvas stays interactive, turn-end collapse, click-during-Busy cannot keep it closed, delete strip, exec/read_file still one unpinned block). Nothing left in the table.
zig → harness.wasm is green on da654c6. Do not treat this as --approve.
What was not attacked
Live Preview paint, restored-session L2 after refresh, Copy of the still-sentinel payload, dvui testing-backend layout of the empty new box / click-during-pin caret, full vitest (N/A — no TS), DO runner beyond the already-green build-harness job.
Summary
Implements plan #715 (source #714).
#692shippedstr_replaceL2 side boxes, but the operator still saw an ink-only old side, a collapsed expander, and a stalled canvas if they opened it. This PR:ember_border/teal_borderfills (visible againstteal_bg) with muted rimstextLayout.background = falseso the box fill shows through (house markdown/diff already did this; feat(harness): str_replace L2 TEAL/EMBER side bands #692 did not)str_replace(even while still running); pins L2 once detail exists; collapses when the turn ends (reusethinking_collapse_state.isActiveTurnFull)Item.str_replace_sides, slot-cached). Committed sides usecache_layout+ monoaddText— no per-frameaddTextMixedon the bodiesNo protocol bump. No TS emit/preview/cap change.
Files
native/harness/src/rich/toolrun.zig—Item.str_replace_sides, decode assignment,toolRunL0PinnedOpen/strReplaceL2PinnedOpen+ tests (decode encoded line + pin matrix)native/harness/src/ui/toolrun.zig— fill tokens,.background = false,cache_layout, pin (no HashMap write while pinned)docs/harness-limits.md— Tool-run L2 / Painter / Open state (timeless)Verification
tsc --noEmitnode_modules; no TS delta)vitest run --changedzig build test-rich/test-rich-invariants/harness -Doptimize=Debugzigin this agent workspacebuild-harnessNew Zig tests (plan matrix): decode encoded
str_replacestores sides /execstays null; L0 pin on name-only; L2 pin requires active+name+detail; existing 8 splitter cases unchanged.Test plan (operator, after Preview wasm)
str_replace: L0 open while running; L2 open once detail exists; two visibly colored bands (not ink-only)exec/read_fileL2 still oneteal_textblock; not auto-openedstr_replacecollapsed; expanding still splitsFixes #715
Fixes #714
Refs #691
Refs #687