refactor: behavior-preserving simplification sweep (all 74 groups)

[kyle-sexton]

No related issue: a repo-wide behavior-preserving simplification sweep run on request, not tracking a filed defect. The defects it uncovered are listed under Related so they can be filed separately.

Summary

A whole-repository code-tidying:batch-simplify sweep. The 3,451-file universe was classified down
to 1,190 code files, grouped into 74 dependency-ordered groups, and worked group by group. All 74
groups are complete, plus a Phase 6.5 resolution wave for the cross-group deferrals no single group
was permitted to make.

Every group ran as a pair: a simplifier that applied changes, then a fresh-context refutation
verifier that had not seen the simplifier's reasoning and whose job was to break the claim that
behavior was preserved. A verdict of "looks fine" counted as a failed verification. The passing
verdict is "I could not construct a counterexample, and here is what I tried."

The diff is deliberately small relative to the files read: 150 code files, net −11 lines. It is
close to line-neutral on purpose, and the arithmetic is worth stating rather than hiding. Removals
dominated through the group waves; the resolution wave then added lines back, because the two
changes it shipped introduce shared helpers, and because several refutations are now recorded as
comments at the site so the next reader does not re-derive the same false saving. Lines removed was
never the goal.

Several groups read their whole file list and changed nothing at all, and each is reported as a
result rather than a gap. What the sweep declined to touch is recorded alongside what it changed.

This branch has been merged with main twice and reconciled each time. While it ran, main
absorbed a separate repo-wide tidy sweep, first in part and then in full. The second reconciliation
was far smaller than the first (15 overlapping files against 69, 10 conflicts against 52), which is
what convergence between two independent sweeps looks like.

Fix

Simplifications applied: duplicated logic folded into single helpers, dead stores and unread
accumulators removed, single-use bindings inlined, repeated derivations hoisted, and duplicated
fixture setup deduped in suites. Every plugin whose shipped files changed carries a patch bump and a
specific changelog entry.

One real bug fixed, found while claiming not to change behavior.
plugins/planning/scripts/check-open-questions.sh looked up deferred question ids with
printf | grep -qE under set -uo pipefail. grep -q exits as soon as it matches, printf is then
killed by SIGPIPE, and pipefail promotes the pipeline to 141, which the enclosing if ! reads as
"id absent". A question that was present got reported missing and the script died ungradeable.

It is a race against the 64 KB pipe buffer, not a size threshold: printf only takes SIGPIPE if
it still has data to write when grep exits. Measured at 15 runs per size, id on the section's first
line, counting runs where the pipeline returned nonzero: 2/15 at 64 KB, 7/15 at 100 KB, then 15/15
from 128 KB. So small Briefs passed, mid-sized ones failed intermittently, and the failure only
became reliable once a plan grew. No test covered it. The builtin [[ =~ ]] reads the string
directly and cannot SIGPIPE.

The intermittent band is the part worth naming: a registered question reported missing only sometimes
reads as a transient and invites a re-run rather than an investigation. An earlier revision of this
PR described the onset as "~100 KB, hard 141 from ~250 KB" on the strength of single-shot probes; the
100 KB probe had landed on the passing side of a coin flip. Corrected here, in the source comment,
and in the changelog entry.

One deliberate behavior change, stated plainly. In plugins/firecrawl/.../update.sh the
SKILL.md not found checkout guard moved after the --help arm, so update.sh --help outside a
plugin checkout now prints help and exits 0 where it previously errored and exited 2. This was
originally logged as behavior-preserving, which was self-contradicting. It is kept because the family
convention supports it: of the six sibling update.sh scripts, three carry both constructs and all
three put --help first. Firecrawl was the only outlier.

The single most useful result produced no diff at all

One group's candidate was to replace three blocking gates' per-field printf | jq | tr payload reads
with the shared hook::jq_fields helper that ten sibling hooks already use. Spawn counts, measured
with strace, improved on every path (the claim gate went 28/21 to 23/12 execve/clone on a matching
call), so against the hook-budget contract it was a clear win.

A 449-payload differential killed it. Three delta classes, two of which turn a blocking gate
permissive:

1. $() strips trailing newlines and hook::jq_fields does not, so a .cwd ending in \n stops
   resolving a relative worktree or --body-file target and the gate goes rc 2 to rc 0.
2. On two concatenated JSON documents the old form joins both .tool_input.command values and
   blocks, while the helper rejects the over-count with rc 2, which the || exit 0 converts to a
   permit. rc 2 to rc 0 again.

affected-tests.sh --run returned 0 across all seven suites with those weakened gates in place,
and every lint gate passed too. Only the differential caught it, on the three files where a false
negative matters most, since these gates are what stop a --no-verify or an out-of-containment
worktree. The change was reverted; the deferrals are the handoff.

A second near-miss of the same shape, caught in the resolution wave

The formatter-hooks cluster was sent to collapse printf | jq -R . | jq -s . into one
jq -R -s 'split("\n")…': one fewer process per fire on an always-on hook. It is not equivalent.
Slurp mode decodes the whole stream as one string, so a truncated UTF-8 lead byte immediately before
a newline absorbs that newline into a single U+FFFD and merges two findings into one array
element; line mode splits on the raw byte first. Minimal case:

printf 'a\xe2\nb\n' | jq -R . | jq -s .              →  ["a<fffd>","b"]
printf 'a\xe2\nb\n' | jq -R -s 'split("\n")…'        →  ["a<fffd>b"]

Same for \xf0; not for \xc3. The fold was refuted (28 mismatches over 231 unit cases) and a
"tried and wrong" comment now sits at each site. What shipped instead is the safe half: the findings
encode is gated on hook::telemetry_enabled, since its only consumer returns immediately when the
sink is unwired. Measured with strace -f -e trace=clone,clone3,fork,vfork,execve, three reps, on
the default unwired path: jq executions 3→1 for bash-format and biome-format, 5→3 for
actionlint; with the sink wired, unchanged. typos-format needed no gate and took only the
rationale sentence its siblings carry.

Reverted or corrected after verification

No test suite caught any of these:

Reverted or corrected	Why
claude-ops hook via hook::jq_field	Behavior-identical in every input class, but strace measured one extra fork per fire on an always-on hook. The commit's own "same spawn count" claim was false.
markdown-format DIGEST_FILE="" removal	With the write forced to fail, the status leaving the block goes 0 to 1 and the variable is left unset rather than empty.
docs-hygiene head -1 removal	Justified by "emits at most one line", which verification falsified: jq -r prints a newline-bearing string across lines.
knowledge processLesson guard hoist	Converted a crash on malformed course.json into a silently skipped lesson and exit 0. 416 of 3,000 adversarial cases diverged.
knowledge generate-manifests.js, all three edits	Reverted on a reviewer's P1. Nothing covers the file, so the edits could not be validated. See below.
kindle-dedrm comment	A comment this sweep wrote said an unreachable host prints 000. It prints 000000.
instruction-placement spawn claim	A comment asserted a saving that measurement contradicted.
guardrails changelog claim	The release entry said "no hook file changed". It was false when written. Rewritten to claim only what is true.
planning SIGPIPE measurement	The comment and changelog both cited a size threshold. It is a race; re-measured at 15 runs per size (above).

The docs-hygiene and knowledge decisions turned on the same tiebreaker in opposite directions:
this repo's fail-loud doctrine. One change was kept because it made failure louder, the other
reverted because it made failure quieter.

On generate-manifests.js specifically, since a reviewer caught what this PR's own description
had understated. scripts/affected-tests.sh reported the file UNMAPPED, which AGENTS.md treats as an
error rather than "nothing to run", so the sweep's edits to its frame-classification and manifest-path
logic bypassed the required validation. Neither prescribed remedy applied: the file exports nothing,
no module imports it, and no suite names it (it is a standalone ESM script exposed only as the
manifests npm script), so there is no co-located or referencing suite to point at; and a
scripts/affected-tests-no-suite.txt entry would have been a false claim, because that file's header
defines an entry as a claim that a named non-shell lane covers the class, and the course-digest vitest
lane covers the directory while reaching none of this file. The three edits were cosmetic and worth
four lines, so the file went back to its origin/main state. --explain over all 150 changed code
files now exits 0.

The Phase 6.5 resolution wave, and why most of it is declines

27 of the sweep's 273 deferrals were CROSS-GROUP: fixes needing files from more than one group's
list, which is exactly what no single group was allowed to do. A resolution wave took them in
clusters, each agent holding the whole cluster. Most came back declined, and a decline with a
recorded reason is a result.

The allowed-tools-pairing.test.sh cluster refuted its own deferral. Three groups had
independently reported that the per-skill allowlist branch is "dead in four copies, live in one". It
is not dead. expected_granted is called and its result immediately tested
(expected="$(expected_granted "$skill")" then if [[ -n "$expected" ]]), so the four *)-only
copies are a guarded optional taking a no-op path, which is a different claim. Planting a real
arm into the supposedly-dead case fires the check
(FAIL: tidy: granted set drifted from the allowlist, rc=1); substituting the true value returns 0.
The extension point is reachable, functional and discriminating. Corroborating: da70c871, the
competing sweep that landed on main and also ran fresh-context verifiers, touched these exact lines
to reindent them and kept the function.

The watch/ tempDirs cluster was billed as "the cheapest genuine win" and is a decline — the
four blocks are byte-identical, but sharing them across four ES modules needs a module to import, and
creating files was forbidden. What it found instead is worth more: two sibling suites
(render-quality-audit, render-triage-log) clean up with for (const dir of tempDirs.splice(0))
where the other four use for (const dir of tempDirs) { … }; tempDirs.length = 0;. Those are not
equivalent on the throw path. rmSync's force: true suppresses ENOENT but not EBUSY/EPERM, and
splice(0) empties the array before the loop body runs, so a throw on the first entry leaks every
later directory permanently while the other form leaves them queued for the next afterEach to
retry. The shorter form that reads as a free one-line saving is the buggier one.

One cluster produced a fix. repo-hygiene's two batch orchestrators each carried their own copy
of the skip-list match loop, the SKIP_HITS initialization and the unmatched-skip report loop; those
become batch_skip_match, batch_reset_skip_hits and batch_report_unmatched_skips in
lib/batch-common.sh. Two rows stayed declined on evidence rather than caution: adopting the shared
batch_resolve_repos was tested against a real git repository whose directory name contains a
literal backslash, where the inline loop resolves it and the shared helper folds the name to a
non-existent path and reports it invalid; and the --repos-from/--skip-from contract text lives in
each usage() heredoc, making it program output rather than comment. The SKIP_HITS init was
moved rather than dropped because its positive control proved it live: storing a different value
there makes every UnmatchedSkip line disappear (229 mismatches).

What the merges revealed

Three resolutions would have shipped broken on a naive merge:

• restart-consumer.sh and telemetry-upsert.sh each auto-merged into an inconsistent state
  outside any conflict marker: a dangling call to a renamed function, and two surviving definitions
  of the same function. Neither is visible as a conflict; both were caught by grepping every
  identifier after resolving.
• observer.py is the one place where preferring main's side would have been wrong. The sibling
  lines had already auto-merged from this branch, so main's conflict half would have deleted a
  .strip(). Nothing in the 73-test suite distinguishes the two, so it would have merged green.

fingerprint.mjs was treated as the highest risk, because this run had already measured that its
suite does not pin the algorithm. A 226-document corpus emitting 3,390 values per variant across
three window sizes confirms the resolution reproduces the merge base, this branch, and main
byte-for-byte, with both negative controls firing at 218/226 and 226/226.

Convergence was measured, not assumed. Every changelog entry was rewritten from the post-merge
diff rather than restored, because an entry claiming work main has since published is false. That
check rewrote fourteen entries after the first merge and more after the second.

Verification

Per group: the repo's own scripts/affected-tests.sh --run, shellcheck, shfmt -d,
check-shell-portability.sh, check-silent-skips.sh, check-hook-exec-form.sh,
check-cross-plugin-source-drift.sh --check and check-purged-em-dashes.sh, plus each ecosystem's
own lane (vitest, tsc --noEmit, the pinned run-ruff.sh, Pester).

Because the suites here often do not cover what was changed, each group also built a differential
harness comparing a pinned parent revision against the edited one, with negative controls (plant
a bug, confirm the harness detects it) and, for removed assignments, positive controls (put a
sentinel where the dead store was and confirm still-zero mismatches). That is not ceremony: one
assignment that looked exactly like a dead store produced 18 mismatches across 722 cases under
its positive control, proving it was a live default carrier, and it was re-plumbed instead of
deleted. Always-on hooks had spawn counts measured with strace rather than asserted.

Two refinements the later groups added, both after a harness proved blind:

• A second comparison channel when the change touches setup rather than output. The linear
  adapter's negative control (one field added to a GraphQL query) was invisible to rc, stdout and
  stderr; only an instrumented request-body log caught it. Same for the watch/ cleanup: with
  rmSync disabled outright the suites still reported 4 files / 15 tests passed, and only a
  leftover-directory check saw the leak.
• A generalized positive control when a diff removes no assignment: perturb what the change
  introduced instead. A comment relocation, for instance, is proved unobservable by relocating and
  rewording it again and confirming still-zero mismatches.

The Windows Pester lane does run here, and that changed how two groups were verified. pwsh 7.6.5 and Pester 6.1.0 are installed. Invoke-Pester over machine-health's tests/windows gives
TOTAL=448 PASSED=323 FAILED=124 SKIPPED=1; the 124 failures are Windows-only cmdlets absent on
Linux, and the set is deterministic (run twice, byte-identical). Both machine-health test groups
were held to a clean diff against that baseline. Two caveats a reviewer should have:
scripts/affected-tests.sh returns exit 3 for these files (Pester lane selected and NOT RUN), and
CI has no Pester lane at all — zero invocations in .github/workflows — so tests/windows is
unrun in CI and the differential is the only evidence for those groups. A green test-windows check
does not cover them. Of four negative-control plants against one of those groups, two were not
detected, which is the honest measure of how much the suite covers those particular files; the gap
was closed by driving the dot-source paths directly.

The final union verification, run serially over the changed code files, with every lane the repo
has. affected-tests.sh --run exits 3, not 0, because 107 non-shell suites are selected and NOT
RUN; each was then run directly:

lane	result
shell (affected-tests.sh)	269 suites, 7,637 assertions, 0 failures
Python (15 suites)	965 tests + 360 subtests, 0 failures
Node .mjs (3 suites)	544 + 22 + 40 checks, 0 failures
Pester (13 suites)	448 tests, byte-identical to the recorded baseline
vitest, video-digest	72 files / 514 tests, 1 known flake (below)
vitest, course-digest	10 files / 91 tests
ai-briefing build	41 tests
knowledge vendor repo-analysis	10 tests

Repo-wide static gates all rc=0: check-shell-portability.sh --all (1,593 shell files),
check-cross-plugin-source-drift.sh --check, check-silent-skips.sh, check-hook-exec-form.sh,
check-purged-em-dashes.sh, and all four check-changelog-parity.sh modes.

Serially is load-bearing: the runner is deliberately sequential, and two concurrent invocations in
this shared checkout each produced a single non-reproducing failure in a different unrelated gate
suite, both of which pass standalone and neither of which reads any file this PR touches.

The one failure in that table is acquisition/acquire.test.js > listWorkDirFiles, which recursively
walks the machine's shared /tmp under a 5s default timeout. It passes 8/8 at --testTimeout=60000.
Aptly, it failed here because this container's /tmp had filled with the sweep's own scratch
directories, which is precisely the "slower the longer a machine lives" property listed below.

A note on this PR's own CI history, since it is a worked example of the thing the sweep kept finding.
While the PR was a draft, test-linux and hook-utils reported success in about twelve seconds
and test-windows reported skipped, because ci.yml gates every heavy lane on
github.event.pull_request.draft != true. Three green test lanes that had executed nothing. Marking
it ready is what got them their first real run.

Related

Defects and coverage gaps the sweep measured but could not fix within a behavior-preserving
remit. This is the part worth filing:

• Nothing covers generate-manifests.js. Not "it maps to zero shell suites" — it exports nothing,
  no module imports it, and no suite names it, so the course-digest vitest lane reaches none of it
  despite covering the directory. affected-tests.sh correctly calls it UNMAPPED. Closing this needs
  either a real test, which means exporting classifyLesson and is a source change beyond a
  behavior-preserving remit, or an owner decision about the script's coverage. An earlier revision of
  this description asserted the vitest lane covered it; that was wrong.
• work-items' github list-items.sh skips the gh-version ladder its siblings use. :37
  requests --json …,issueType,blockedBy,… unconditionally, while common.sh:229
wit_gh_issue_view_json_fields() gates exactly those names behind wit_gh_has_native_surface,
  citing work-items: create-item is blocked by the gh>=2.94 sub-issue guard even when no gated flag is passed #3598/fix(work-items): gate gh 2.94 on native-surface flags only #3639 and noting that cloud images ship gh 2.45. So on an older gh, get-item and
  create-item degrade to null/0 while list-items should fail outright. Two qualifications:
  gh issue list --json and gh issue view --json accept different field sets, so the 2.94 threshold
  may not transfer exactly, and only a modern gh was available here, so the failing case was not run.
  Out of remit either way, since fixing it changes behavior on old gh.
• Two watch/ suites leak temp directories on a throwing rmSync (the splice(0) finding above).
• Restart-StoppedService.ps1's ShouldProcess gate is untested. Replacing it with if ($true)
  breaks no test, and under -WhatIf the original skips Start-Service while the mutant actually
  restarts the service. No test anywhere passes -WhatIf or -Confirm.
• The provenance fingerprint algorithm is unpinned. Changing the shingle join separator or the
  window bound leaves the suite 40/40 green while altering every fingerprint the plugin has emitted.
  Reconfirmed during the merge by two independent negative controls.
• check-cross-plugin-source-drift.sh cannot see intra-plugin drift. It iterates
  for plugin_dir in plugins/*/ and compares across plugins, so five byte-identical file pairs
  inside knowledge (.npmrc, tsconfig.json, register-hook.mjs, resolve-hook.mjs,
  vitest.config.ts) are unprotected. Confirmed with cmp on all five; the gate exits 0.
• A machine-health suite's affected-tests mapping lost its redundancy. drive-root-baseline.jsonc
  maps to its Pester suite by textual reference; that basename appeared twice in the test file and now
  appears once, in a docstring. Still mapped, but a future docstring edit would silently unmap it.
• acquire.test.js recursively walks the machine's shared /tmp while asserting only that the
  result is an array. Flaky by construction, and slower the longer a machine lives.
• miro's MCP wire protocol is untested: 14.72% statement coverage, and 19 of 31 planted
  mutations survive, including every one in response.ts, stderr.ts and miro-client.ts.
• A shipped Windows CRLF fix has no test: dropping tr -d '\r' from provenance's
  list-corpus.sh config reader is invisible to its suite.
• go-format.test.sh skips entirely without goimports, giving an always-on hook zero coverage
  while still reporting success. Confirmed here: the binary is absent, the suite prints
  SKIP: no goimports binary and exits 0.
• watching/models.js fans affected-tests.sh out to about 200 suites on a basename match.
• hook::jq_fields fails with jq: Argument list too long on an actionlint run producing ~120
  findings (lib/hook-utils.sh:1963). Pre-existing and identical before and after this PR, so no
  differential signal, but a real degradation on large workflows. It lives in the synced
  lib/hook-utils.sh, so fixing it is a canonical-plus-17-copies change.
• plugins/knowledge/vendor/repo-analysis/repo-analysis.test.js has a real suite and no CI lane.
• allowScripts in the root package.json is consumed by nothing in the repo. It occurs exactly
  once, with no script, workflow, gate or lavamoat tooling reading it, and it has drifted behind the
  pinned claude-code version twice.
• repo-hygiene ships bash 4+ constructs (local -n, local -A, mapfile) while
  check-shell-portability.sh reasons about GNU-vs-BSD userland rather than bash version, so they
  pass every gate here and would fail on stock macOS bash 3.2.
• Two --help handlers disagree with their siblings: worktree-claim.sh exits 2 rather than 0,
  and worktree-create.sh documents exit codes only up to 4 while exit 5 exists.
• shfmt is not a CI gate (ci.yml marks it optional; gated cases skip rather than fail), so the
  repo carries a long tail of non-conforming shell including lib/hook-utils.sh. Worth knowing
  before reading any "brings the tree into conformance" claim. One consequence:
  repo-hygiene/scripts/allowed-tools-pairing.test.sh is the one copy of five that da70c871 did not
  reformat, so the bash-format hook will reflow it on the next edit and drag 16 unrelated lines into
  whatever diff touches it next.

Deferred items are recorded per group with an explicit ground. Of 273 recovered from the run's
transcripts: HUMAN-DECISION 185, PROTECTED 50, CROSS-GROUP 27, TOO-LARGE 11. HUMAN-DECISION at 68% is
high for a fix-first contract; sampling the rows, most are genuine (paired doc-and-code changes,
documented boilerplate conventions, style calls with no behavior content), but a subset is really
"declined on judgment" wearing a softer label, and a few paired doc-and-code items are CROSS-GROUP
mislabeled.

One structural note for whoever runs the next sweep: agents were given a scoped file list but not
a scoped workspace, so isolation rested on discipline rather than structure. That produced two
near-misses in this run (a git stash that swept up two sibling agents' in-flight work, and a shared
scratch directory whose contents a sibling overwrote mid-comparison). Give each agent its own git
worktree.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XsxC7nPL8mhm3JXL1rrjNJ

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-04T18:20:52.967684Z	b61fcfc	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 2m 48s —— View job

---

Code review in progress

Skip-gate: PR is open, ready for review (not draft), and the diff (223 files, +2466/-1658) is substantive, so a full pass applies.

• Gather context (PR body, full diff vs origin/main)
• Run skip-gate checks
• Review changed files for correctness / maintainability issues — fanned out across 6 parallel review passes: always-on hooks, misc shell scripts, knowledge/course-digest JS+Python, knowledge/video-digest JS, remaining Python/test scripts, machine-health PowerShell
• Post inline findings (if any)
• Post final summary

[claude]

Claude finished @kyle-sexton's task in 2m 36s —— View job

---

Security review in progress

• Gather context (PR diff, changed files, repo instructions)
• Confirm no .github/workflows files changed (Actions security surface unaffected)
• Fan out security hunters across hooks/guardrails, work-items/source-control, and knowledge-extraction file groups
• Adversarially verify any surviving candidates
• Post findings (or "no issues" if clean)

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[github-actions]

Last security-reviewed head: b61fcfc35e0da55f808188fe87e5f9e72e5bcdcb. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b61fcfc35e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".