feat(harness): protocol v23 reasoning-effort picker + persist - #902
Conversation
In-canvas effort menu next to the model picker. Host pushes this model's Gateway values; the operator pick rides reserved meta.reasoningEffort and is sent live on POST /api/turns. Default/restore never auto-selects max.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
implement-plan #898 — protocol v22 effort picker + persist.
Not merged. Next: |
Protocol-bump PRs failed durable-turn int: wait was off and fetch always took latest main Wasm, so REQUIRED_FNS fail-closed on the new v23 exports. int-durable now pins HARNESS_COMMIT_SHA to the PR head (github.sha on pull_request is the ephemeral merge commit) and HARNESS_PR_NUMBER so fetch-harness downloads harness-wasm-pr-N from the commit-matched build-harness run. Host-only follow-ups reuse that PR artifact. Vercel latest is unchanged: still main-only production name harness-wasm.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #902
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/reasoning-effort · 39 files · protocol v23 effort picker + persist + int-durable wait
Lenses run: L1 L2 L3 L4 L5 L6 L8 L9 (skip L7: no new single-tenant/runtime bind)
AGENTS.md read: yes · feature-divide.md · SECURITY.md
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | HarnessHost applySessionReasoning persist seam is writeLocalSession, not writeLocalSessionMeta. Model-pick fold already locks Meta (PR #618 re-run 7) because writeLocalSession snaps ringWindowStartRef to latest while Wasm may still show a Load-earlier page. Poll path: hasPendingModelChange → applySessionReasoning(sessionRef.current) uses the full writer. |
1. Load earlier. 2. Stored reasoningEffort is high. 3. Switch model to one whose Gateway list does not include high (or is empty / GLM). 4. applySessionReasoning persist-drops the sticky via writeLocalSession. 5. Next Send sees needSnap === false, paints the turn onto the stale historical ring. Transcript and SessionStore diverge. |
Defender: persist only fires on drop/re-default, not every model change. Fail: that is the common GLM / list-mismatch path the plan requires. Defender: adopt rebuilds the ring. Fail: the poll callback is not adopt; it is the 150 ms model-change path. | high |
| Minor | L1+L9 | ui.zig line-1 effort ellipsis second pass ignores the model trigger that is still painted. effort_budget = budget - spinner_w - trigger_overhead - effort_gap — no reserved model width. Plan #898 already flagged “second trigger can overflow ~390px”. Isolated reasoning_picker_layout tests never paint both triggers. |
Narrow ~390 px canvas, long model id + effort token (provider-default / anthropic/… + low). First pass squeezes the model (correctly reserving effort). Second pass re-expands effort as if the model picker is gone. Line 1 overflows the 32 px row / clips line 2. |
Defender: effort labels are short (low). Fail: provider-default is 16 ASCII chars plus 32 px trigger overhead, and the formula is unconditionally wrong whenever both pickers exist. |
high |
| Minor | L1+L9 | selectedReasoningLabel falls back to index 0 when unset; reasoning_picker.paint is static (no commit) at count == 1. Host defaultEffortFromOptions leaves max-only / xhigh-only / provider-default-only lists unset (never auto max). |
Gateway list is [max]. Host shows static max, operator cannot open a menu to commit it, getSelectedReasoning() is null, POST omits reasoning, server resolveAgentReasoning may still send product low. UI says max, inference is not. |
Defender: max-only Gateway lists may not exist today. Fail: DoD is “max is listable”; count==1 static makes a single NEVER_AUTO value unlistable. The label fallback is what makes the lie visible. |
medium |
| Minor | L4 | int-durable “wait for commit-matched harness-wasm-pr-N” did not run on the SHA that added it. listCommitPaths(HEAD) is per-commit; Actions pull_request path filters are vs merge-base, so build-harness still ran on 2e6315f while fetch logged sha=2e6315f does not touch harness paths — skip wait and reused artifact 9747356832 from run 33362959801 (06:10, earlier PR wasm). |
Trailing host-only commit on a protocol-bump PR: int-durable skip-waits; if the previous PR artifact is missing (cancel-in-progress of the zig run) it falls back to main harness-wasm (v21) and fail-closes. The wait path this PR claims is unproven by the green check. |
Defender: host-only HEAD should reuse PR wasm; this SHA did not change Zig; 06:10 artifact is v23. Fail: GitHub PR path filter ≠ per-commit path list, so zig still rebuilds while int-durable does not wait for that run. Green did not exercise HARNESS_WAIT_MS=720000. Vercel is not poisoned (HARNESS_PR_NUMBER unset). |
high |
Residual risk
Wasm was not executed in this review (no zig build test-rich here). Real-Wasm int on this HEAD used an earlier PR artifact, not the in-flight 2e6315f compile. Palette/TEAL copy of the model picker looks honest; no dual-chat / secret-in-Wasm breach. RESERVED_META_KEYS append + drop-to-unset matches the closed allowlist. Protocol skip v22 is documented.
Merge guidance
- CONCERNS: do not merge until the Major Load-earlier snap is fixed (
applySessionReasoning→writeLocalSessionMeta, plus a HarnessHost wiring lock like the model-pick one). - Minors: ~390 px budget formula and max-only commit path should land in the same turn; int-durable skip-wait is acceptable only if a later harness SHA actually waits (or wait keys off “PR still has harness files”, not HEAD-only).
What was not attacked
Live Gateway catalogs, self-hosted zig build test-rich on this workspace, Production Vercel fetch, operator click-through of the dvui menu, fork-PR runner (job if: already skips forks).
applySessionReasoning now uses writeLocalSessionMeta so a model switch that drops a sticky effort cannot snap a Load-earlier ring window. Line-1 effort ellipsis reserves the model trigger still painted. count==1 unset is a menu so a NEVER_AUTO-only Gateway list (max) is listable. int-durable waits on a PR host-only HEAD because Actions pull_request path filters are vs merge-base.
|
Follow-up
Targeted vitest: 4 files / 57 passed. Zig layout tests updated; they run on |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #902
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/reasoning-effort · 39 files · protocol v23 effort picker + persist + int-durable wait · HEAD 32b847c (re-run after first-pass fixes)
Lenses run: L1 L2 L3 L4 L5 L6 L8 L9 (skip L7: no new single-tenant/runtime bind)
AGENTS.md read: yes · feature-divide.md · SECURITY.md
First-pass CONCERNS (2e6315f) closed on this SHA:
| Prior | Status |
|---|---|
Major L1 writeLocalSession snap |
Fixed — poll persist is writeLocalSessionMeta; source lock in harnessHostReasoningPersist.test.ts |
| Minor L1+L9 effort ellipsis ignored model trigger | Fixed — second pass reserves painted model width |
| Minor L4 int-durable skip-wait | Fixed + proven — run 33365908033 logged waiting for build-harness.yml on 32b847c artifact=harness-wasm-pr-902 and installed commit-matched 9748324832 |
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1+L9 | bridge.selectedReasoningLabel still falls back to index 0 when unset; reasoning_picker highlights i == view.selected even when has_selection is false. applySessionReasoning + defaultEffortFromOptions leave NEVER_AUTO-only lists ([max], [xhigh], [provider-default], [max,xhigh]) with getSelectedReasoning() === null. Count==1 unset is now a menu (first-pass fix) so the value is clickable, but the closed trigger already paints the NEVER_AUTO token in teal_accent as if committed. |
1. Gateway list for this id is [max] (or any NEVER_AUTO-only set). 2. Boot/restore: host does not auto-select. 3. Trigger shows max. 4. Operator Sends without opening the menu. 5. POST omits { reasoning }. Server resolveAgentReasoning with a non-empty NEVER_AUTO-only options list also returns undefined (does not fall through to product low — that path is empty-options / GLM only). Inference is provider-default. UI said max. |
Defender: max-only Gateway catalogs may not exist today; operator can open the menu and commit. Fail: DoD is “max is listable, never the restore/default”; the fallback label is what makes the unset state indistinguishable from a committed pick, so the listable menu is not used. provider-default is also NEVER_AUTO and can be a singleton. |
medium |
| Minor | L6 | No unit row locks the NEVER_AUTO-only unset path. applySessionReasoning tests cover [low,high,max] default-low and stored-max restore; they never call ['max'] / ['xhigh','max'] and assert getSelectedReasoning() is null. Isolated reasoning_picker_layout “count 1 unset” passes short_label = "max" and only checks the tag exists. |
A later change that auto-selects index 0 (or re-introduces the label fallback as “selection”) stays green. Combined model+effort ~390 px budget still has no test that paints both triggers. | Defender: Zig push does not auto-select plus host default tests on mixed lists. Fail: the DoD-critical NEVER_AUTO-only case is exactly the untested one; mixed-list default-low cannot catch it. |
high |
Residual risk
Wasm layout of both pickers on a live ~390 px canvas was not executed here (host dvui tests isolate the effort trigger). max-only Gateway catalogs are unconfirmed in production; if they never appear, the honesty Minor is latent. Protocol v22 remains an unused hole (documented; no other open PR claims it). Palette copy of the model picker is TEAL; no dual-chat / secret-in-Wasm / runner-on-fork breach. RESERVED_META_KEYS append + drop-to-unset matches the closed allowlist.
Merge guidance
- PASS WITH NOTES: first-pass Major is gone; int-durable actually waited on this SHA. Safe to merge from this attack if the honesty Minor is accepted.
- Should still land: unset trigger must not look like a committed NEVER_AUTO value; add
applySessionReasoning(['max'])→ null.
What was not attacked
Live Gateway catalogs, operator click-through of the dvui menu on a phone, Production Vercel fetch, fork-PR runner (if: already skips forks), zig build test-rich in this workspace (self-hosted only).
adversarial-review #902 re-run: NEVER_AUTO-only lists stay unselected. selectedReasoningLabel no longer falls back to index 0; the closed trigger shows "effort" until the operator commits; menu highlight requires has_selection. Host test locks applySessionReasoning(['max']).
|
Follow-up
Targeted vitest: 2 files / 70 passed. Zig layout/bridge tests run on |
Summary
Protocol v23 in-canvas reasoning-effort picker (plan #898 / parent #896 phase 2).
Status bar line 1, next to the model menu: host pushes this model's Gateway
reasoningOptionsinto Wasm (not stuffed into the id catalog). Hidden when the list is empty (GLM today). Operator pick persists as reservedmeta.reasoningEffortand is sent live onPOST /api/turns{ reasoning }fromgetSelectedReasoning(). Host computes the default viadefaultEffortFromOptions—maxis listable if Gateway lists it, never the restore/default.Merged
maininto this branch after #901. Concurrent F21 claimed the next protocol slot as v22 (enqueue-visibility residual on #815), so this PR retargets 21 → 23. Merge conflict inHarnessHost: persist the F21reconciledsnapshot (queue restore/give-up), then fold livegetSelectedReasoning()the same way asselectedModel.Layers
reasoning_picker.zig+reasoning_catalog.zig; protocol v23 exports; 32 px trigger; ellipsize on ~390 pxreasoningById→ push on selection change; restore/pending fold; live POSTRESERVED_META_KEYS+=reasoningEffort;sanitizeReasoningEffortdrop-to-unset (never 400)DoD
HARNESS_PROTOCOL_VERSION/PROTOCOL_VERSION)maxlistable, never default/restore auto-selectmeta.reasoningEffortonRESERVED_META_KEYS+ persist + live POSTfeature-divide,session-model,agent-stream,AGENTS.md)build-harness(self-hosted runner — not compiled in this workspace)Verification
npm run typecheck— greenreasoning_catalog,reasoning_picker_layout,bridge.zigv23 rows) ship in this PR; they run on the runner withbuild-harness.harness.wasmuntilbuild-harnessproduces the v23 artifact.Caps
Zig
MAX_REASONING_EFFORT_LEN = 32parity-locked to TSREASONING_EFFORT_MAX_BYTES. Protocol 21→23 is not a cap. No existing cap changed.Fixes #898
Refs #896