refactor(engine): output inversion batch 3 — the long tail loses its terminal imports (#429) - #458
Merged
Merged
Conversation
Batch 3 of #429: skills.zig, skill_docs.zig, review.zig, imagegen.zig and agent_prompt.zig go from 1/2/1/1/2 banned imports to zero. Only one of the seven was an actual terminal write, so the batch splits three ways. Four of the seven were approvals.zig, and none of them wanted the approval gate: skills and skill_docs want the settings-file path, imagegen wants confinedPath/noSymlinkEscape, review wants readOnlyAllowed. approvals.zig is already terminal-free — it is on the ratchet's list because it is the CONSENT surface, the mutable allow-list a human grows by answering y/n. So the pure half moves to harness_policy.zig (settings location, path confinement, the command classifiers) and approvals.zig keeps the session state. A pure move: every decl is re-exported unqualified, so method bodies and `Approvals.x` call sites are untouched, and a new test asserts the re-exports ARE the aliases rather than a second copy — a security predicate that drifts is a hole. agent_prompt.zig is the real inversion. prompt() now gathers the session into one `.prompt_ready` event and emits it; the palette, the badge frame and the #209 width budget move to agent_prompt_render.zig behind TuiSink, the same shape agent_stream_render/agent_tool_render have. One new variant and its payload types: prompt_ready: PromptStatus — what the status line SAYS: model, provider, cwd, the mode badges, and the meters as values (CostMeter distinguishes off / flat-rate / unpriced / a real figure; ContextMeter carries tokens, window and the compaction threshold). No widths, no colors, no assembled segments — which badges survive a narrow pane is a rendering decision. A presentation pulse: the wire has never had a prompt, because a --json client drives its own turns, and the emit site still returns early on json_mode rather than relying on a silent sink. ReasoningEffort and PrivacyTier are the vocabulary's own enums rather than main.zig's and learning_privacy's, so a transport-split sink can read an event without importing the engine; the emit site maps with exhaustive switches and a test pins the two sets in step. skill_docs.zig gets a relocation, not an event pair, and deliberately: its ansi use was the `/skills` command surface, whose other half (the companion rows) is printed inline by commands_session.zig. Routing one half of one command through a contract with no wire shape for it, leaving the other half inline, would manufacture exactly the divergence #422 exists to remove. The command layer is frontend territory, so handleRemove/handleAdd/printSection move whole to skill_docs_render.zig and skill_docs.zig goes back to being the skill subsystem its own header describes. commands_session.zig stays at 600 lines by repointing its existing import rather than adding one. One behavioral note: promptLine swallows a write error where prompt() used to propagate it, because a sink emit returns void. Every other TuiSink branch already behaves that way, and the only path there is a terminal that vanished mid-prompt. Proven byte-identical by a golden before/after harness: a scripted-model PTY session at three pane widths (160/60/34), capturing the status line with and without usage, the `/skills` catalog, and remove/add/unknown — 40 artifacts, raw bytes and rendered text, all identical between the pre- and post-change binaries. Capture windows open only after a turn has settled, so no spinner frame is inside one and every .raw is deterministic (verified by two runs of the same binary agreeing). The layout is additionally pinned in-tree: the renderer's tests assert the whole line byte-for-byte at four widths, including the #209 shed order and the rule that the cache badge never floats free of the context meter. tier 1 green; 1051 tests pass. Co-Authored-By: Codegraff <blackfloofie@codegraff.com>
The instrument that proved batch 3 behavior-preserving lived only in a session scratchpad, which made its verdict unreproducible the moment that session ended. Three separate issues depend on it — #429's remaining batches ride the same bar, #430's acceptance is golden byte-identity in both modes, and #431's attach-parity eval is this harness pointed at two transports — so it moves to scripts/eval/golden/ in the shape scripts/eval/live-ab/ established: - both arms' binaries come from GRAFF_EVAL_BEFORE / GRAFF_EVAL_AFTER, the same variables the live-ab harness uses, resolved lazily so capturing one arm needs only that arm's variable, and failing with an actionable message rather than a traceback. No path in the file is absolute: ROOT derives from __file__ and the shared PTY driver is imported through it; - the determinism self-check is a first-class step and gates everything after it. The same binary is captured twice and must agree before any before/after diff is believed, because PTY capture has several ways not to be stable and a diff from an unstable instrument cannot be attributed; - the session script is data (STEPS x WIDTHS), so the next batch adds a window rather than rewriting the driver; - runs/ is gitignored. Only the instrument ships. The README writes down the capture-window race that cost real time here: a window opened at `cursor = len(raw)` straight after wait_for_literal can fall on either side of readline's 12 setup bytes, and the failure is stable per run, so one arm can look self-consistent while the two arms differ by a prefix with byte-identical content behind it. Every step now settles before its window opens. The known gaps are documented too rather than left to be rediscovered: no NO_COLOR arm (this build draws no prompt under it), no --json arm (these surfaces have no wire shape), and no approval prompt (#430's, not this). Verified end-to-end on batch 3's two binaries: self-check green, 40 artifacts byte-identical, exit 0. Also verified it FAILS correctly — a tampered artifact and a deleted one are both reported, exit 1 — since an instrument that always says "identical" is worse than none. zig build test green; tier 1 green. Co-Authored-By: Codegraff <blackfloofie@codegraff.com>
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
1 participant
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.
Batch 3 of #429 (part of epic #422). Five files, zero banned imports remaining in each.
skills.zigskill_docs.zigreview.zigimagegen.zigagent_prompt.zigThe finding that shaped the batch, and that the epic should absorb
Four of the seven offending imports were
approvals.zig, and not one of them was a terminal write.approvals.zigis already a pure std leaf. It is on the ban list because it is the consent surface (the mutable allow-list a human grows by answering y/n), not because it drags in the terminal. What these files actually wanted was policy:settings_pathfor skills/skill_docs,confinedPath/noSymlinkEscapefor imagegen,readOnlyAllowedfor review.So the pure half is split into
harness_policy.zigas a move+alias, every decl re-exported unqualified so method bodies and allApprovals.xcall sites elsewhere are untouched, with a test asserting the re-exports really are aliases rather than a second copy.This means batch 1 is not at zero either.
exec,tools,edit_verifyandagent_tool_gatestill importapprovals, andagent_toolsstill importsterm. They can now switch toharness_policyfor free. Worth reflecting in the epic's ratchet, which currently counts a policy import as if it were terminal coupling.One new event
prompt_ready: PromptStatus, carryingCostMeter(off / flat-rate / unpriced / a figure),ContextMeter, and vocabulary-ownedReasoningEffort/PrivacyTierenums, so a transport-split sink need not import the engine. Exhaustive switches at the emit site, with a test pinning both enum sets in step. It is a presentation pulse: the wire never had a prompt, andprompt()still returns early onjson_moderather than relying on a silent sink.skill_docs.zigdeliberately got a relocation, not events. Itsansiuse is the/skillscommand surface, whose other half (companion rows) is printed inline bycommands_session.zig. Routing one half of one command through a contract with no wire shape, while the other half stays inline, would manufacture exactly the divergence #422 exists to remove.Byte-identity: verified, with a harness that had to be built
There is no committed golden harness. The phrase traces to a CHANGELOG entry describing an ad-hoc before/after run; nothing under
scripts/implements it. So one was built: a scripted-model PTY session at three pane widths (160/60/34) capturing the status line with and without usage, the/skillscatalog, and remove/add/unknown paths — 20 windows, raw bytes plus rendered text, 40 artifacts.diff -rq before after: no differences. All 40 artifacts byte-identical, re-verified afterzig fmtrewrote a file.ESC[?2004h ESC[6n, readline's terminal setup) landing inside one capture window. Diagnosed as a capture-window race, fixed with a settle before opening each window, after which both binaries matched exactly. Recorded rather than waved away.Tests
zig build testexit 0, 1051 passing.scripts/eval-tier1.shgreen: fmt, 600-line ceiling, reachability (1016 declared tests compiled in), build, suite, 9 named invariants, SDK sync.commands_session.zigstays at exactly 600 — its existing import line was repointed rather than a new one added.Behavioural delta worth flagging
promptLineswallows a write error whereprompt()used to propagate it, because a sinkemitreturnsvoid. Every otherTuiSinkbranch already behaves this way and the only path there is a terminal that vanished mid-prompt, but it is a real, if pathological, difference.Not covered
NO_COLOR rendering has no PTY golden: with
NO_COLOR=1this build never draws the interactive prompt over a PTY at all (pre-existing, unrelated to this change). Covered instead by in-tree tests that zeroansi.styleand assert the whole line byte-for-byte at four widths.--jsonwas not exercised live; no new event is durable (asserted by test) andprompt()returns early injson_mode, so the wire is untouched by construction rather than by observation.The
anyOfoutput this batch saw on baseline runs is the display artifact diagnosed in #444 (PR #456), not a real failure.