docs(fix-issue): tier the workers, fit the skill in context, and enforce both in CI - #1776
Merged
Conversation
Restore explicit per-worker capability selection, lost when the skill was rewritten provider-agnostically. Express it as a LIGHT/MID/HIGH tier contract with a harness mapping table (Claude Code haiku/sonnet/opus, Codex luna/terra/sol) so the rule binds regardless of which harness runs the skill, and pin each worker role to a fixed tier. Separate the two economy levers the skill previously conflated: delegate to keep bulk output out of the coordinator, choose the tier to control cost. The coordinator may now run trivial deterministic assertions itself instead of spawning for a hash comparison. Consolidate the review gates from six mandatory reviewer spawns to two: a post-diagnosis gate that also challenges the domain classification, and a pre-ready gate that combines the code review with the final aggregate review. Extra aggregate reviews are trigger-only and receive evidence links, not inlined evidence. Make CI the default owner of the full test suite, with local runs targeted. Move conditionally-relevant procedure out of the always-loaded SKILL.md into references/: autonomous mode, environment and hooks, generated-artifact gates, regression locks and the xfail rule, and push/merge/cleanup.
The autonomous-mode reference warned that a long unattended run is where an unset tier silently bills the top model, then left the two assignments below that warning untiered. Name them: MID for the verifier, HIGH for the visual reviewer.
The PR API lags a push by seconds and reports the previous SHA, so the origin check fired a false "lost commit" alarm during this branch's own second push. Query the ref directly and re-query once before treating a mismatch as real.
Contributor
|
Render preview: no visual changes detected. All renders match |
A dry-run plan eval against issue #1765 (a theme-threading defect with no rendered symptom) surfaced that the diagnostic protocol assumed every issue is a geometry bug. Step 3 demanded "render the example, inspect coordinates, restate as a numeric claim", the classification offered only "authoring mistake" or "engine bug", and the post-diagnosis gate was scripted to challenge a numeric claim that cannot exist for such an issue. - Step 3 now carries two claim forms, geometry and non-geometry, and adds a third classification bucket for input-independent structural defects. - An issue that already names its own root cause gets a MID confirm-or-refute brief instead of a HIGH re-derivation. Independent confirmation stays mandatory. - Step 5 says explicitly when to skip: no layout property to guard, no validator. - The gate-coverage ratchet trigger is now literal. It scans src/nf_metro/layout/routing/ only, so a change to engine.py cannot trip it and needs no defensive check. The guard-trace golden is a separate trigger. - The writer tier rule resolves a diff spanning both HIGH and MID directories. - Visual review is sized to the verdict: a LIGHT worker may report a literal "no visual changes", but any delta at all gets HIGH eyes on every changed render.
An audit for duplicated expensive work found the draft-PR push specified in three separate steps. A push runs the full test matrix and renders the whole gallery on both the PR branch and the base, so it is the costliest action in the workflow, and a coordinator reading linearly could pay for it three times. - Step 8 now owns the single push and draft-PR creation. Step 10 verifies it landed and edits the body instead of re-creating. Step 11 only flips the draft to ready. - Accepted fixes batch into one push per round rather than one push per fix. - The local before/after sweep and the CI render-diff compute the same corpus diff; pick one, never both for the same SHA. - Hooks run on the changed files, not `--all-files`, which sweeps the repo with a cold mypy over all of src/. - The verifier's mypy cache moves outside the per-SHA artifact directory, since a fresh cache per candidate made every verification run cold. - Step 8's probe_layout/inspect_layout reading is labelled as the candidate-SHA after-state, so it does not read as a repeat of Step 3's before-state.
Cut the per-spawn coordinator output that persists in the transcript and is re-read on every later turn. - Add references/worker-contract.md holding everything invariant across spawns: the one-writer rule, read-only discipline, the six-item return schema, evidence-by-path, and what a worker does when the work exceeds its briefed tier. Briefs now carry only the task-specific fields plus a pointer, instead of restating the contract each time. - Evidence arrives as a path plus the one figure carrying the verdict. Pasted coordinate dumps, render analysis, and test logs were landing in coordinator context permanently, which defeats the delegation the skill is built on. - The ledger keeps only its live slice in context (current gate, open blockers, accepted SHA, active assignments) and appends settled rows to a file. It was the one item growing every turn. - State that the coordinator does not read src/ at all. It does no substantive diagnosis or implementation, so the large layout modules belong only in worker contexts. - Halve the frontmatter description, which loads every session for every skill whether or not this one is used. All trigger phrases retained.
CLAUDE.md loads into every subagent, so a worker running a lint check received the full Astro/Starlight build description: the Vite plugin, the cache directory, the gallery generator's outputs, the gh-pages deployment. The section was 3.1KB of a 12.9KB file. Reduce it to the structural facts plus pointers. Nothing is lost: the flag list is in `scripts/serve_docs.sh --help`, the `<Metro>` component options are documented more fully in docs/contributing.mdx, the branch-preview workflow is in the serve-docs skill, and the render pipeline is in website/src/lib/render-metro.mjs.
…test Running it by default stays the rule. Skipping requires all four conditions to hold and must be declared: no new function, class, branch, or code path; no shared helper, dispatch table, or multi-caller site touched; under roughly 20 lines of non-test production code; and no production code added by a later step. A small diff that introduces a branch or threads an argument through three call sites does not qualify, and weighing whether it qualifies means it does not.
…e compaction cap Two dry-plan evals (issues #1765 and #1563) and a review of Claude Code's skill mechanics drove this. Per-role agent types. `.claude/agents/` now defines one type per role with its tier and tool set in frontmatter. Spawn by role name and pass the model explicitly: the spawn-time model is verified to take precedence, and the definition's model is a safety net only, since the fallback direction is not verified here. Read-only roles carry no Edit/Write tool, making read-only structural rather than advisory. The investigator and verifier may instead use the built-in Explore type, the only one that loads neither CLAUDE.md, verified by self-report as saving roughly 5k tokens per spawn. Diagnosis can now conclude there is no defect. The classification offered only authoring mistake, engine bug, or structural defect, all of which presuppose a bug; issue #1563 exists specifically to be closed if the mirrored shape routes acceptably. "Not a bug" is now a first-class outcome held to the same evidential standard, and the post-diagnosis gate certifies whether a defect exists, not only which kind it is. Also: the single-push rule states its own exception, since the routing ratchets skip off Python 3.11 and surface only in CI; the writer is one continuing worker rather than a fresh spawn per step, so it keeps the large layout modules it already read; the writer tier distinguishes geometry-affecting changes from structural ones in the same directories; the stated-cause carve-out is bounded to single-site claims; the brief template gains a DECIDE field for options a worker must surface rather than pick; fixture parametrisation carves out structural defects; a writer's own draft renders are not a spawn; and the regression-lock grep warns about parent issue numbers. Split SKILL.md from 42KB to 18KB (311 lines). After auto-compaction Claude Code re-attaches at most 5,000 tokens per skill, so the previous body would have been silently truncated mid-run. Procedure detail moves to references/; SKILL.md keeps the worker contract, the tier tables, the gates, cost discipline, and a twelve-step spine. Verified by sentence-level diff that no rule was dropped.
An independent review against the Claude Code docs found the prose had been restructured carefully and the executable content had not been re-tested. Cost and blast radius: - fix-issue-writer had no tools allowlist, so the most expensive worker in the run inherited every MCP tool schema, plus Agent, letting it spawn untiered children and break the one-writer rule. Every definition now carries an explicit allowlist in the documented comma-separated form. - Definitions gain `effort`. The writer persists across steps by design, so a top-tier model was regenerating goldens and typing commit messages; mechanical re-briefs now drop to `effort: low` without losing its context. - The eco-merge assessor moves from MID to HIGH: it gates shipping code CI has not verified. Gates that could not be executed as briefed: - The visual reviewer had no way to get renders into context. Read takes filesystem paths, the preview embeds SVG, and WebFetch returns markdown. Added a curl-and-rasterise block, since Bash already covers it. - The verifier's command block asserted against a frozen checkout that nothing created, and never cd'd anywhere, while the worker least licensed to improvise was the one told not to choose different commands. - Local renders omitted --no-chrome-css, which CLAUDE.md already warns aborts cairosvg. - The simplify worker was pointed at the unqualified skill, and both candidates apply fixes and need Agent. Corrected mechanics claims: - Plan skips CLAUDE.md too, Explore is one-shot and cannot be re-briefed, it runs its own system prompt rather than the role definition, and it inherits the session model. The invented tool list is gone. - CLAUDE_CODE_SUBAGENT_MODEL outranks the per-invocation model, so the full four-step resolution order is now stated. Truncation keeps the FIRST 5,000 tokens of a skill, and the twelve-step spine was last. Reordered so procedure and the tier contract come first and only the rationale is at risk, and added the instruction to re-invoke after compaction. Two roles that had no agent type, both needing the Skill tool, now have one.
…budget A second independent review checked the first round's fixes against the live preview and the repo, and found three that were present but non-functional. The visual gate could not see anything. The render-fetch block globbed for `.svg` files on the preview site, but `scripts/build_render_diff.py` inlines every render through `_inline_svg` and writes a single multi-megabyte `index.html`; there are no fetchable SVGs, and the inlined markup carries `var()` and `light-dark()` that cairosvg cannot parse. The block would have produced an empty file list, a no-op loop, and a reviewer giving a confident verdict having seen nothing. It now reads the changed stems from the page's diff-entry anchors and re-renders those locally at both SHAs with --no-chrome-css, and explicitly forbids reading the preview page itself. The environment recipe built an env that could not import nf_metro: the hand-written package list omitted `lark`, a hard runtime dependency, and `coverage`, which the gate ratchet needs. It now installs from pyproject. The verifier could not run its own block: no env activation, so ruff, mypy and pytest were off PATH, and without `set -e` the frozen-SHA and clean-tree assertions returned non-zero and execution continued, so the guarantees the verifier exists to provide were not enforced. The SHA comparison also required a full 40-character SHA and now compares rev-parse output on both sides. Corrected claims: `effort` is fixed for the life of a definition, there is no per-invocation effort, so the persistent writer cannot be dialled down for mechanical re-briefs; the retained context is worth more than the thinking tokens that effort moves. `permissionMode` is ignored when the parent runs in auto mode, so the structural lever for read-only is the per-subagent `hooks` field. Making the writer persistent requires `SendMessage` to its agent ID, which was never stated. Also: the eco-merge tier contradiction survived in merge-and-cleanup.md; the simplifier held unrestricted Agent while being told not to write, pointed at a skill that applies fixes, and never named the qualified `pinin4fjords:simplify`; the post-diagnosis gate claimed to review aggregate progress at Step 3, before anything is aggregated; the hook command activated the wrong env, never cd'd, and word-split its file list. CLAUDE.md's editable-install claim is corrected rather than left contradicting the skill, and environment.md notes that render-topologies brings its own env. Measured with tiktoken rather than estimated: SKILL.md is 4,882 tokens, with the twelve-step spine at 1,217 and the tier contract at 3,373, so the procedure sits well inside the 5,000-token re-attachment cap and only a pointer paragraph is near it. All 17 shell blocks pass `bash -n`.
…en read A third review executed the pipelines instead of reading them. Three defects failed silently, which is worse than failing loudly. The render loop consumed $ART from a previous Bash call. Shell state does not persist between calls, a rule this skill states itself, so the worktree path became /base, git worktree add failed, every stem fell through the resolution guard, and the mandatory visual gate produced zero images while reporting nothing. The block is now self-contained and aborts on an unset ART. Stem resolution missed 7 of 38 real stems. The corpus spans examples/, tests/fixtures/ and tests/fixtures/hash_seed_determinism/, and gallery ids can carry a pipeline_ prefix the source file does not, so a glob rooted at examples/ skipped them under `|| continue` while the prose demanded no silent skips. Resolution now runs against git ls-files across the whole tree with the prefix stripped as a fallback, and names every unresolved stem. Executed against the live preview for PR 1743: 38 stems extracted, 38 resolved, against 31 before. Executing it also surfaced a defect no review found: some corpus fixtures abort by design at head, and under `set -euo pipefail` one aborting fixture killed the whole sweep. Verified with seed_41, which aborts on bundle curves at this SHA. Renders are now guarded per stem, stderr is kept per stem, and a stem that renders on base but fails on the candidate is called out as the regression it is rather than skipped. The Step 3 tooling could not import nf_metro at all: no PYTHONPATH on probe_layout, inspect_layout, explain or info. Worse, with a non-editable install they would have silently diagnosed the installed snapshot instead of the worktree under test, so the numeric claim the whole run rests on would have been about the wrong code. Step 5 named only _guard_*; routing invariants are check_* registered in CHECK_REGISTRY via a GuardSpec whose tier decides how much golden churn a change causes across hundreds of committed traces. Both are now documented. Measured, and correcting my own earlier figure: SKILL.md is 4,619 tokens with 381 headroom, and the twelve-step spine ends at 1,565 cumulative, not the 1,217 I reported. That number excluded the file head. Moved the model-resolution order and the effort/read-only mechanics into agent-types.md to buy the margin. All 17 shell blocks pass both bash -n and zsh -n.
…it the checks An independent review of the validation methodology measured what I had not: the restructure read 14,817 tokens on a normal run against the baseline's 9,397, a 58% increase. I had shrunk the always-loaded file, reported that as the win, and grown the total. The reference table even said seven of eleven references load on a normal run; I never did the arithmetic on my own sentence. The axis was wrong. References are now split by owner, not by step number. The coordinator reads four files and is told explicitly not to read the others, because it is forbidden from acting on them and reading them puts worker-facing bytes in the context that is re-read every turn. procedure.md, which mixed both, becomes coordinator.md and writer-steps.md. Coordinator-resident text is now 7,903 tokens, 16% below the baseline, with 10,091 tokens of worker-facing material read in contexts that are discarded at handoff. Closed the merge-assessor gap rather than documenting it again: the role no longer carries `Skill`, so it cannot reach a procedure ending in `gh pr merge --admin`. The eco-merge safelist is inlined, extended with the repo-specific paths a ratchet or the render-diff reads. Capability removal rather than a `permissionMode` that auto mode ignores, or a hooks schema I have not verified. Fixed the trigger collision. nf-metro-layout-fix advertises the same invariant-test-first, runtime-validator and gallery-vetting loop, neither skill mentioned the other, and roughly half of "fix the kink in #1234" would have landed on it. Both descriptions now state the boundary: issue-driven work goes to fix-issue, a bad render with no issue goes to layout-fix. Its stale `src/nf_metro/routing/` path is corrected, and the tier table's path strings now match the real nesting. Removed the self-contradiction on tier resolution. SKILL.md claimed an unset parameter lands on the session's top tier while agent-types.md gave the documented order in which it lands on the definition. Both could not hold, and which one does is the thing never tested; the rule now points at the order and names the environment variable that overrides every tier here. scripts/check_skill.py commits the verification instead of leaving it in a transcript: link resolution, spine cross-references, tier naming, owner-split accounting, agent-definition agreement with the tier table, bash and zsh syntax, and the token budget. It found three defects on first run, including two untiered assignments and an owner mismatch, all fixed here.
…s the SHA A review that executed every block found that `set -euo pipefail` is inert in this harness: the Bash tool runs zsh and evals the block, so ERREXIT never fires. Verified directly - `set -euo pipefail; false; echo SURVIVED` prints SURVIVED, exit 0. Both of the verifier's mandatory guards were therefore decorative, and running the block against a wrong SHA on a dirty tree reached the end reporting success. Every guard now fails itself through a `die` helper, the block ends with an explicit `VERIFY OK` line, and an absent line is a failure whatever the exit status looked like. Proven by execution: the SHA guard and the dirty-tree guard both now abort. The visual gate could review the wrong tree. Nothing checked that the preview was built from the candidate SHA, and this skill's own `[skip ci]` default guarantees staleness after any post-Step-8 push. On the branch used for testing the preview was eight days older than the head, and in a quarter-sample of the ids missing from its stem list, four rendered differently and five aborted on the candidate but not the base - all invisible to the procedure. The sweep now proves provenance from the workflow run's headSha before trusting the stem list, uses `curl -fsS` so a 404 fails instead of silently saving an error page into an empty stem list, and says to enumerate the corpus rather than fall back to a stale one. All three gate-ratchet commands failed as written for want of PYTHONPATH in the prescribed env, and the third emitted 27 subprocess errors that look exactly like the gate failures the file forbids hand-editing away. Also: `nf_metro info --json` was missing both PYTHONPATH and its required argument; the gallery row lives in scripts/gallery.yaml, not GALLERY_ENTRIES, which is derived from it, and the lock grep now looks there; the force-push guard is a Claude Code PreToolUse matcher, not the git pre-push hook this file claimed; the worktree needed --no-track, since otherwise the branch takes main as upstream and a bare push suggests `git push origin HEAD:main`; and the LIGHT verifier is no longer asked to assess a full render. check_skill.py gains a check that no shell guard depends on `set -e`, which is what let this class of defect through. It caught two more on first run. One reviewer claim was fabricated and is not encoded: gallery.yaml has no render_only.expected_aborts key, and no allow-list of aborting fixtures exists anywhere in the repo. Aborts are judged by comparing both sides instead.
… money A cost audit over 45 real runs of this workflow (25,942 main-thread turns, 62,715 subagent turns, 466 spawns, from the per-request usage blocks) found the skill was optimising terms worth under 1% while ignoring the two that dominate. Measured structure: cache reads are 68% of spend, the HIGH writer's turns are 45%, and the coordinator re-reading its accumulated context is 26%. Two levers added, neither previously present: - The writer hands off at roughly 150 turns. Cost grows as turns^1.28; spawns in the 150-300 band averaged $25 while the 46 that ran past 300 averaged $62. This directly qualifies the previous unconditional "one continuing worker" rule, which is right for a 100-turn writer and expensive for a 500-turn one. Worth about $35 a run, roughly 8x the tiering saving. - Do not hold a large context idle across a CI run. 468 turns, 1.8% of all turns, were 9.8% of total spend: full re-encodes averaging 358k tokens at the 1-hour cache premium, caused by idle gaps busting the cache. Worth about $15 a run. Three of my own claims were contradicted by measurement and are corrected rather than quietly dropped: coordinator `src/` reads are under 1% of a run, not "the largest avoidable cost"; naming a tier is 5.7%, not "the single largest lever", and much of the per-spawn gap between tiers is turns rather than price per token; and the Explore substitution is worth about $0.24 a run, so it is a curiosity that no longer spends resident tokens arguing for itself. Push and `[skip ci]` policy moves to merge-and-cleanup.md, where the push lives: it is governance, not economy, and it was filed under cost discipline. It also now states its own cost, since a `[skip ci]` push invalidates the render preview and forces a local corpus sweep. Separately, ~/.claude/skills/session-economy/analyze.py priced Opus at $15/$75. Opus 5 is $5/$25, verified against the claude-api skill's pricing table, so every dollar figure that tool has produced for this project was inflated threefold. Corrected, with Fable added and the Sonnet 5 introductory rate noted.
A mechanics review executed every block and found the Step 8 provenance check, added two commits ago, wrong in the commonest case and in the dangerous one. Verified against the workflows: when a PR has no visual changes, build_render_diff.py returns early, pr-renders.yml sets has_changes=false, and every deploy step in pr-render-publish.yml is gated on it being true. So the page is never published and my curl died telling the reviewer to wait for something that would never arrive - on a clean run, which is the common one. In the other direction, pr-render-publish.yml deploys with keep_files: true and only cleans up on PR close, so comparing the workflow's headSha passes while the page fetched belongs to an earlier push. The gate now reads the sticky comment first and treats "no visual changes detected" as the verdict it is, then proves the page is this run's by matching its own nf-metro-render-run marker against the run id, with both SHAs normalised through git rev-parse. Two guards were still decorative. `test -z "$(git status --porcelain)"` passes when git itself fails, because it inspects stdout only - verified in a non-repo directory - so both clean-tree assertions now capture and test separately. And two verifiers on one candidate SHA derived the same worktree path and collided, which also blocked any re-run after a mid-block failure; the path is now process-unique and the block prunes and checks before adding. The origin check was prose telling the reader to compare two printed SHAs. It is now a self-failing test, which is what the rest of the skill requires. Fixed from the same review: agent-types.md, a coord-owned file, still claimed the merge-assessor holds Skill after that grant was removed, and pointed hardening at the one role already safe; the Step 4 lock grep still named build_gallery.py, which contains zero issue references while gallery.yaml holds 110; the golden gate covers every root _discover_fixtures walks, not just examples/topologies; the largest files are the routing handlers and invariants, not the trio named; and nobody owned materialising a reproducer that exists only inside a details fold, which is where a real trace got stuck - the diagnostician now does it. check_skill.py was demonstrated to pass eight distinct mutations. It now joins shell continuations, matches bracket guards and undefined `die`, validates repo paths in commands, checks agent definitions against the role table both ways, catches orphaned references, and fails rather than skips when tiktoken is absent. The role table names the agent type per role so that check is exact rather than heuristic. Mutation-tested: 9 of 9 now caught, from 1 of 9. SKILL.md holds a declared 10% margin under the re-attachment cap, enforced by the checker, since o200k is only a proxy for Claude's tokenizer. Gate and writer-discipline prose moved to coordinator.md: still resident for the coordinator, but out of the capped file. 4,274 tokens, 726 headroom. The measured cost figures are now attributed as a one-off measurement on one machine rather than presented as reproducible, since they do not meet the evidence bar this skill imposes on its own workers.
…8% term Two round-two reviews, both executing rather than reading. The Step 8 chain was broken by `| tail -1`. The sticky comment body is multi-line and its last LINE is the sticky HTML marker, so both greps missed and the block fell through as if deltas existed, then 404'd on a page that is never published for a clean run. Reproduced on live PR 1776: the body says "no visual changes detected", `tail -1` yields the marker, grep misses. Now selects the last matching comment whole (`last | .body`), and the repaired chain returns the right verdict when executed against that PR. Three companion defects in the same chain: `$$` in the artifact path made it unreachable between Bash calls, two blocks never created the directory they wrote into, and the fallback enumeration raised TypeError because `render_only` mixes lists of strings with lists of dicts. Cost, from a second audit over 115,383 usage blocks: my own advice was the largest remaining leak. "Read the big layout files in wide slices, generously" holds 205k tokens resident, which is about $30 in cache reads over a 300-turn top-tier spawn, and 68% of spend is re-reading accumulated context. It now says to grep to the function and read 10-25k around it. Second: forks. A fork inherits the parent's whole context and ignores the `model` parameter, so every tier rule here is void inside one; six fork spawns averaged $220 each. One line forbids them. Corrected my own numbers rather than leaving them flattering: "8x the tiering saving" was a category error and is ~1.1x; "one extra preamble costs cents" is $1.50-3.00; "1.8% of all turns" divided a both-thread numerator by a main-thread denominator and is under 1%; most of those re-encodes were at the 5-minute premium, not the 1-hour one; and the Explore substitution costs a few dollars a run once resident re-reads are counted, not $0.24. The 150-turn handoff rule was also unimplementable and had a losing branch below 178 turns: the trigger moves to 200 and the writer now reports turns used as item 7 of the handoff schema, since it is the only party that can see them. Also: the golden gate covers every root `_discover_fixtures` walks, and a fixture under `tests/` reds it, which matters because Step 4 puts tests there; the red message quoted was not the one the test emits; `_VALIDATE_DEFAULT = False`, so a runtime validator does not fire in an ordinary render and the skill said otherwise; D-delta narrowing was the only uncapped loop and now stops at two rounds; and a truncated sentence had lost the pointer to `pr-chain-vet`. check_skill.py was shown to miss 9 of 13 mutations outside my own set. It now checks prose tool claims against the definitions, prose repo paths, effort values, per-role required tools, non-failing `if` guards, `$ART` writes without mkdir, owner-table membership, and broader spawn phrasings, while skipping re-briefs so it stops flagging them. Re-tested: 12 of 12 caught, including the three that survived the first hardening pass. One reviewer claim was wrong and is not acted on: `run-tutorial` does exist, as a project-scoped skill in training-studios-config, so the CLAUDE.md pointer is live.
A third cost audit computed the resident-context integral across 586 subagent spawns and the lever I promoted two commits ago is wrong by two orders of magnitude. Bash output is 74.5% of resident cost ($1,062 across the corpus, 36k calls); reading the three big layout modules is 2.0% ($29 in a month, $0.05 a spawn). Worse, 571 of 573 reads of those files already passed an explicit limit, so "read the region" described behaviour that was already universal - and my "10-25k around it" was 14-36x the median window actually used, against a median function of 28 lines. Following it literally would have read more, not less. So: a general rule about keeping bulk command output out of every context, which is the measured lever and 37x the one it replaces; the region-reading advice demoted to a clause and moved to worker-contract.md where the audience actually is, calibrated to 1-3k tokens; and an explicit warning against under-reading, since reading costs $0.05 a spawn while a wrong fix costs a narrowing round plus a CI cycle, roughly $50. That warning names docs/dev/inter_section_dispatch.mdx, because routing/core.py is a first-match dispatcher and a handler read in isolation cannot tell you whether it fires. Other honest corrections from the same audit: - The idle-context rule reaches about a third of the cost it sits beside. Only 35% of those re-encodes follow an idle gap; the rest happen within a minute of the previous turn for reasons not visible in the transcripts. It now says so. - The LIGHT tier is unvalidated: no historical spawn in this repo ran on that model. Four roles are assigned it on no evidence, so early LIGHT spawns are labelled an experiment to re-route rather than a settled default. - The turn trigger is unobservable by either party - no tool reports a turn count - so the schema now says the writer's figure is an estimate. - The fork multiple is ~4x a comparable named worker; the $220 mean was a property of the parent model those forks inherited. - The owner split is stated as organisational rather than a saving: the resident difference is pennies a session while the corpus more than doubled. The Explore figure disagreed between two files ($0.24 versus "a few dollars") and nothing caught it. Both are now the measured $1.65, and check_skill.py gained a numeric-consistency check so the same quantity cannot carry two figures again. SKILL.md 4,470 tokens, 530 headroom, real margin restored.
… merge ruling A third mechanics review executed every block and found the visual gate still returning a wrong answer on the path that matters. `runid` was assigned in the provenance block and compared in the next one, which is a separate Bash call, so it was always empty and every delta-carrying PR got a false "stale preview" - routing it into the corpus-wide local sweep the same file forbids. It is now derived where it is used, with an empty-result guard, because `gh run list` exits 0 when a branch has no runs and an empty value otherwise reaches the comparison and dies misleadingly. Three more faults in the same chain, all the same underlying mistake of trusting state or prose instead of a discriminator: - The sticky selector matched on the words "Render preview", so a human comment saying "the Render preview looks fine" would be chosen as the latest match. It now anchors on the sticky marker, and guards the case where jq prints a literal "null" and exits 0. - There are five sticky wordings, not four. `pr-render-publish.yml:173` emits "was not generated because a prerequisite check or the render job failed", which both greps missed; the chain then waited for a page that CI had already failed to build. That is now a distinct failure, not a wait. - Two blocks called `python`, which is not on PATH unactivated, and block 1 then reported the resulting empty SHA as a candidate mismatch. Both now activate. Also: my guard used `a && ! b`, which zsh rejects outright - and this harness is zsh, so it was a parse error rather than a weak check. Verified directly and split into two guards. The merge rule contradicted a standing ruling. It said never escalate to `--admin`; the recorded feedback says that when checks are green and the only blocker is an out-of-date branch, "merge" is standing authority to admin-merge that PR, and that merging the base in to satisfy the policy is the CI waste the ruling exists to prevent. The exception is now stated, with never-squash intact. check_skill.py gained guards for the classes this round exposed: `$$` in a shared artifact path (the exact regression fixed last round, previously unguarded), a read-only role holding Edit or Write, one-line `if` guards that never exit, and paths under docs, .github and .claude which were outside its scope. Its numeric check was firing false conflicts across paragraphs and is now sentence-scoped. Guard detection accepts `continue` and `break`, since a loop guard need not exit.
Eight review rounds produced the same class of defect over and over, always in the same file, always found by someone executing rather than reading: tail -1 on a multi-line body, $$ in a path shared across Bash calls, runid scoped to the wrong block, python not on PATH, and `a && ! b` which zsh rejects outright. Each was declared fixed and the next round found another. The root cause was structural, not carelessness. A twenty-block bash chain embedded in markdown is unverifiable: there is no program, so nothing can catch a variable scoped to the wrong block, `set -e` does not fire under the harness's eval, and `bash -n` sees only fragments. The whole class becomes impossible once the chain is a script, because a script is one process. scripts/visual_preview.sh, scripts/render_pairs.sh and scripts/verify_candidate.sh replace those chains. Each takes named arguments, each guards its own failures, and each carries --self-test that exercises its parsing against fixtures. check_skill.py now parses every script in bash and zsh, runs shellcheck over it, runs its self-test, asserts every script is referenced, and cross-checks the sticky-comment wordings the preview script greps for against what pr-render-publish.yml actually emits - so a rename of both the grep and its own fixture cannot pass. Mutation-tested against the three classes that blocked the last three rounds: a variable referenced but never assigned is caught by shellcheck, a grep string the workflow never emits is caught by the workflow cross-check, and `a && ! b` is caught by zsh -n. Six of six mutation classes caught, where prose caught none of these three. Token effect, which is the other half of the point: visual-review.md falls from 4,133 to 2,056 tokens and environment.md from 1,924 to 1,333. Those are worker-facing files read fresh on every run, so that is 2,668 tokens off each affected spawn, and the logic now lives in scripts that are executed rather than loaded into any context at all.
The scripts are committed executable (mode 100755) and run from a fresh worktree, verified. But the documented invocation is a relative path, so a worker whose cwd is elsewhere would fail; both call sites now cd first.
…cy contract An external review found the two material blockers, both confirmed: nothing invoked check_skill.py, and tiktoken was not a declared dependency. The PR's whole thesis is that this workflow should be mechanically protected against drift, while its guarantees were unenforced and its token gate failed before it could measure anything. - New `skill-selfcheck` CI job runs the checker from the repo root, installing shellcheck and zsh so the shell and script checks are real rather than skipped. - New `skill-checks` extra declares pyyaml and tiktoken, so the token budget is a checkable invariant rather than a dependency the script hopes is present. - Without the extra the checker now warns on a byte estimate calibrated to the measured 4.13 bytes/token instead of hard-failing, since that figure is too imprecise to gate on. CI is the enforcing path; both branches are tested. The review also caught that render_pairs.sh reimplemented gallery interpretation in shell rather than reusing what the repo already owns. It resolved output names by searching for a matching basename, which guesses at data gallery.yaml states outright and gets duplicate basenames, ids containing regex metacharacters, and every entry whose output differs from its id wrong. New corpus_map.py emits `output_name<TAB>source_path` using the same group semantics as build_gallery.py, and the shell now does an exact awk lookup with no pattern matching. Writing it as a real resolver with a self-test immediately caught two wrong assumptions the grep version could never have surfaced: pipelines follow their gallery entry's source_dir rather than always examples/, and nextflow conversions source from examples/ rather than tests/fixtures/nextflow/. All 291 mapped sources now verify to exist. Push policy reworded, since "the one and only push in the run" followed by a stated exception for CI-only failures was a contradiction a reader could take either way. The invariant is one CI-triggering push per candidate round, with further pushes only for findings CI surfaced that could not be reproduced locally - in practice the routing ratchets off Python 3.11.
…face Both follow-up findings confirmed by execution. The positional repository argument was read from argv[1], so `corpus_map.py REPO` silently used the cwd and `render_pairs.sh --repo DIR` did not honour DIR unless the caller already happened to be inside it. Reproduced from /tmp, where it raised rather than resolving. Argument parsing is now a named function with its own self-test cases, so the shape is asserted rather than assumed. The docstring claimed missing sources are skipped as build_gallery.py skips them. They are not: the map emits them and render_pairs.sh reports them as UNRESOLVED. That is the better behaviour for a review gate - a missing source should be loud, not invisible - so the docstring now says what the code does, and the self-test's existence assertion means the two only diverge when something is genuinely wrong. The deeper point in the finding was that this is the class the self-check machinery exists to catch, and it did not. check_skill.py only globbed *.sh, so the Python helper's self-test never ran. It now runs every scripts/*.py self-test, from a foreign cwd with an explicit repo argument, which is precisely the invocation that exposes a positional-argument bug. Mutation-tested: reading the argument from the wrong index, removing the self-test, and pointing a group at the wrong source directory are all caught now.
pinin4fjords
changed the title
…itions Adding ten `.claude/agents/` definitions quietly made the primary spawn instruction Claude Code specific. The LIGHT/MID/HIGH mapping to Codex's tiers survived, but "spawn by role name and pass the model" assumes definitions exist, and on a harness without them the first half simply fails with no stated alternative. The contract is the tier table and always was: name the role from it and pass that tier's model by whatever means the harness offers. The definitions are a Claude Code optimisation that makes the tier structural rather than remembered, not the thing the rule depends on. agent-types.md is now labelled as harness-specific at the top, since agent definitions, Explore/Plan, effort, permissionMode, hooks, SendMessage resumption and the CLAUDE_CODE_SUBAGENT_MODEL precedence are all this harness's mechanics and none of them port. Trimmed three restatements to hold the token margin: the Explore trade-off, the tools rationale and the read-only caveat each say once what they said twice.
Adding ten agent definitions made the skill substantially harness-specific, and a one-line Codex tier mapping was left implying otherwise. Measured: about 40 references to Claude Code mechanics across 9 of the 14 skill files - agent definitions, Explore/Plan, effort, SendMessage resumption, the CLAUDE_CODE_SUBAGENT_MODEL precedence, the post-compaction re-attachment cap and the .claude layout itself. A Codex run would not get through the first spawn. So the scope is now stated where the tier contract is: this skill does not run elsewhere as written. The doctrine does port - the two levers, the tiers, one writer with independent readers, the gates, diagnose-before-fix - but porting it means re-implementing the enforcement for that harness, not reading these files as-is. The tier mapping stays as a note for whoever does that, rather than as a claim that it already works. Dropped the "read the region" bullet from SKILL.md to hold the token margin. Two reviews pointed out it was worker-facing advice, measured at $0.05 a spawn, sat resident in the coordinator's file, and described what workers already do; worker-contract.md carries the version its audience actually reads.
I proposed replacing tiktoken with a bytes-per-token constant, on the grounds that a foreign tokenizer cannot give precision against a Claude-token cap. Measuring first showed that wrong: bytes/token is 3.37 on the shell scripts and 4.31 on prose, a 28% spread, so a constant calibrated on prose would under-count a code-heavier file by around 20% and silently consume the entire margin. tiktoken tracks content shape, which is the property that matters here, so it stays. What was actually wrong was the description, not the choice. The checker now says o200k is OpenAI's encoding rather than Claude's, that the correct instrument is the count_tokens endpoint but it needs network and credentials a hermetic CI gate should not require, and that the 10% margin exists to absorb the encoding bias rather than as arbitrary padding. Its output reports "~4490 tokens by o200k proxy" against "a 5000 Claude-token cap" so nobody reads it as measured. The local fallback stays a warning and not a gate, with the reason stated: the same constant that makes it usable for this file makes it unsafe as a threshold.
Reading the authoritative guidance changed the answer materially: tiktoken undercounts Claude tokens by 15-20%, and more on code. Every figure I have quoted this session was that much low, and the 10% margin I was defending was smaller than the bias it was supposed to absorb. SKILL.md at 4,490 proxy tokens projected to about 5,388 Claude tokens, so it was already over the 5,000 re-attachment cap and the compaction-survival claim was false. The checker now prefers the correct instrument, count_tokens, which is the only thing that knows Claude's tokenizer, pinned to the model in use. Where no SDK or credential is available it falls back to tiktoken but scales the reading by the documented 20% and gates on a working ceiling of 4,750, keeping 5% slack because the 15-20% band is itself a range. It no longer reports a proxy reading as if it were a measurement. Both paths are exercised, and the gate was confirmed to bite by pushing the file over the ceiling. Getting under the real cap needed real cuts, not compression alone: the brief template and the cost-discipline rules move to coordinator.md, which the coordinator always reads anyway, so they stay resident while leaving the file the cap applies to. SKILL.md is 3,572 proxy tokens, projecting to ~4,286 Claude tokens against a 4,750 ceiling. The skill-checks extra now declares anthropic as well, and the CI job passes ANTHROPIC_API_KEY through if the repo has one. It is optional: without it the fallback is conservative rather than wrong.
Tested in a fresh session on this branch: the ten agent types resolve, and spawning one with no `model` parameter runs the definition's model rather than the session's. That was the one claim resting on documentation alone, and it holds. So the double declaration goes. The skill asked for the role name *and* the model, belt and braces, because the fallback direction was unverified. An external review pointed out that this leaves two things to keep in sync and nothing able to check the spawn site; with the fallback confirmed, the definition is the single source and naming the role is sufficient. Two cases still need the model explicitly, and the skill says which: a generic type has no definition to fall back on, and `CLAUDE_CODE_SUBAGENT_MODEL` overrides every level, so it is worth checking once at session start.
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.
What this is for
The
fix-issueskill runs an nf-metro issue end to end by delegating to 8-15 subagents. Four things were wrong with how it did that, and this PR fixes them.It spent more than it needed to. Nothing said which model each delegated worker should use, so every one of them ran on the session's model. A worker whose whole job is running
ruff checkand reporting the exit code was costing the same as one doing layout diagnosis.It was too big to survive a long session. After an auto-compaction, Claude Code re-attaches only the first 5,000 tokens of a skill.
SKILL.mdwas ~11,300, so well over half of it was silently dropped mid-run, taking the later steps and the cost rules with it.Some of its instructions did not work when run. The verifier's "the tree must be clean" check passed on a dirty tree. The visual-review step could review renders from the wrong commit, or report a clean run as a failure. These were readable and plausible and simply did not do what they said.
Nothing checked any of it. A future edit could break the tier table, a reference link or a shell command with all of CI staying green.
What changed
Worker tiers are explicit.
SKILL.mdgains a role-to-tier table: LIGHT for mechanical work, MID for bounded reasoning, HIGH for judgment where being wrong wastes the run. Forks are forbidden, because a fork ignores the model parameter and inherits the parent's whole context.Ten agent definitions in
.claude/agents/(a third of this diff). Each is a small file declaring one role's model, tool allowlist and effort, so Claude Code resolves them instead of the skill asking a coordinator to remember. For example, the verifier:Two consequences worth reviewing. The tier stops being advice: it is a property of the role, and
check_skill.pyfails if a definition and the table disagree. And read-only becomes partly structural, since those roles hold noEditorWriteat all. They do holdBash, so it is a backstop rather than a boundary, and the skill says so.SKILL.mdis a spine, not a manual. 4,470 tokens with the procedure and tier contract first, so a truncation loses rationale rather than instructions. Detail moved toreferences/, split by owner: the coordinator reads four files, everything else is named in a worker's brief and read in that worker's context, which is discarded afterwards.The fragile shell chains are real scripts.
visual_preview.sh,render_pairs.shandverify_candidate.shreplace multi-block command sequences that could not be verified as prose. Each guards its own failures and carries--self-test.corpus_map.pyresolves gallery output names fromgallery.yamlinstead of guessing at them.Diagnosis covers more than geometry. Structural defects with no rendered symptom get their own classification and evidence form, and "not a bug" is a first-class outcome held to the same evidential standard.
Cost rules point at what measurement says is expensive: keeping bulk command output out of every context, handing the writer off before its context makes each turn expensive, and not parking a large context across a CI wait.
nf-metro-layout-fixgets a boundary. It advertised the same invariant-test-first and gallery-vetting workflow, neither skill mentioned the other, and roughly half of "fix the kink in feat(gallery): list seqinspector showcase render on the nf-core pipelines page #1234" would have landed on it instead. Both descriptions now state the split: issue-driven work goes tofix-issue, a bad render with no issue behind it goes tolayout-fix. Its stalesrc/nf_metro/routing/path is corrected while there.CLAUDE.mdsheds the Astro build detail that every subagent was loading and that lives in the docs and theserve-docsskill.What now enforces it
A
skill-selfcheckCI job runscheck_skill.py, which validates link resolution, agent-definition and tier-table agreement, reference ownership, shell parsing in both bash and zsh, shellcheck, every script's self-test, repo paths named in commands and prose, and the token budget. The budget is measured with Anthropic'scount_tokenswhen a credential is available, since only that knows Claude's tokenizer; otherwise it falls back to tiktoken scaled by the documented 15-20% undercount and gates on a 4,750 working ceiling. Theskill-checksextra declares both, so the check cannot silently skip.What savings to expect
All figures are Claude tokens, and all dollar figures are list prices projected from measuring 45-90 historical runs of this workflow on one machine. Nothing below is observed on the revised skill, because it has not been run end to end yet. On a subscription none of these dollars are billed as cash; treat them as relative weight, not an invoice.
Static, per spawn or per session:
SKILL.mdCLAUDE.md, loaded by every subagentvisual-review.md, read per run with deltasenvironment.md, read by any command-running workerThe
SKILL.mdfigure is a correctness fix, not a saving: at 11,276 tokens against a 5,000-token post-compaction re-attachment cap, more than half the file was being silently dropped mid-run. Coordinator-resident is flat. The always-on footprint is close to a wash once the agent definitions are counted. The genuine static win is about 3,000 tokens off each worker spawn that reads those two references, plus 546 off every spawn from theCLAUDE.mdtrim: call it 8,000-15,000 tokens per session depending on spawn mix, worth $1-3 once re-reads are counted.Behavioural, which is where the money is:
forkBest guess for a typical session: roughly $40-60 saved on a mean run of about $300, so 15-20%, with the median run being cheaper and saving proportionally less. In tokens that is on the order of 80-120M cache-read tokens against a measured median of ~230M per session, since cache reads are 68% of spend.
Two honest caveats. The bulk-output rule is excluded from that range because its delta is unmeasured, and it plausibly exceeds all the others combined. And run cost varies enormously: median $123, mean $300, p90 $732, so a percentage is more meaningful than a dollar figure.
What has actually been exercised
Executed, not read:
verify_candidate.shend to end against a real SHA: worktree add, both guards, ruff, ruff format, mypy over 118 files, targeted pytest, clean-tree assertions,VERIFY OKvisual_preview.shagainst live PR docs(fix-issue): tier the workers, fit the skill in context, and enforce both in CI #1776, returning the correctVERDICT: no visual changesrender_pairs.shon real stems including ones an earlier basename resolver silently skipped;seed_41aborts at head and the loop survives itcorpus_map.py --self-testfrom a foreign cwd: all 304 mapped sources exist--self-test; shellcheck clean;bash -nandzsh -non every scriptcheck_skill.pymutation-tested against 21 defect classes, including every class that had previously shippedset -eis inert here, zsh rejectsa && ! b,Exploreloads neither CLAUDE.md, and a spawn-time model overrides an agent definition'smain: checker, all self-tests, and the corpus map against the enlarged 304-entry corpusNot exercised, and worth knowing before merge:
gh issue viewto a ready PR through this skill. The maintainer intends to exercise this in real usage rather than as a pre-merge test.The model fallback whenTested and confirmed in a fresh session on this branch: the ten types resolve, and spawning one with nomodelis omitted rests on documentation.modelparameter runs the definition's model rather than the session's. The skill no longer asks for both, only for the role name, with the model still required when spawning a generic type.hooksas a structural read-only guard, which is documented as a known gap rather than implemented.Reviewing this
CI covers it:
skill-selfcheckruns the validator, and lint, format andrender-diff are green. No application code is touched, so there is no render or
layout impact and the render-diff reports no visual changes.
The two things most worth a human eye are the ten agent definitions in
.claude/agents/(are those the right tiers and tool sets for those jobs?) andthe honesty of the unexercised list above.
🤖 Generated with Claude Code