fix(guards): close fail-open and latent-test defects found by a repo-wide sweep

[kyle-sexton]

Summary

A repo-wide simplification sweep across 132 files in 14 groups, plus fixes for seven defects the sweep uncovered — three fail-open guards, three latent test bugs that could not fail, and a counting bug. Every applied diff was checked by a separate fresh-context agent whose only job was to construct a counterexample disproving behavior preservation.

Nothing is deferred. Every finding is either applied here or closed in-branch with a stated reason; no issues were filed.

The simplification yield is deliberately small: 9 of 14 groups changed nothing. This codebase is mature, and the honest output of a simplification pass over it is a short diff and a long findings list. The defect fixes are worth more than the simplification.

Scope

288 files in the repo, 132 swept. 156 excluded, by class:

Excluded	N	Why
Fixture / test data	82	fixtures/** and .github/scripts/fixtures/** are deliberately broken inputs (bad/, no-shell/, unterminated-expression/). Their content is the assertion.
Agent & enforcement config	40	.github/workflows/**, .claude/**, CODEOWNERS, dependabot.
Markdown	16	No docs flag on this run.
Upstream-managed	13	Synced from melodic-software/standards; a local edit is reverted by the next sync PR.
Byte-pinned	2	*.pinned.yml must stay byte-identical to a region of an excluded workflow, so editing one side alone breaks the equality test with no way to repair the other.

Two of the upstream-managed exclusions are traps worth naming: .gitleaks.toml and .editorconfig-checker.json have never been modified by the sync bot, so a git-history heuristic marks them safe to edit. They are not. They were identified from the mapping tables the sync engine renders into its own commit bodies.

Fix

Fail-open guards

1. managed-files-guard/run.sh passed when git failed. mapfile -t changed < <(git diff …) discards git's exit status — process substitution is not reaped, so mapfile returns 0 regardless. An unfetched or bogus BASE_REF/HEAD_REF produced an empty change list, and the guard printed "No managed-file hand-edits in this diff" and exited 0, passing precisely when it could not see the diff. Now captured with if ! changed_paths="$(git diff …)", the shape the dest-paths call in the same file already used.

Scenario	before	after
bad ref	rc=0 "no hand-edits"	rc=1 "failed to diff"
managed file edited	rc=1, hit reported	rc=1, hit reported
nothing managed edited	rc=0	rc=0

2. eol-renormalize checked only the first line of a multi-line pathspec. read -ra stops at the first newline. The sibling actionlint, biome and editorconfig actions all use read -r -d '' -a … || true; this one had diverged. Measured: multi-line input yielded 1 element, now 2.

3. probe-billing-usage truncated its own report. main().then((code) => process.exit(code)) abandons pending stdout writes, and this script's entire output is a JSON report. Measured on a 400,011-byte payload: process.exit() delivered 131,072 bytes (the pipe buffer); process.exitCode delivers all of it.

Latent test bugs — each one could not fail

4. gitleaks/scan.test.sh used return inside a top-level for loop, so its *) default arm could never fail the test. Forced against scratch copies: HEAD exits 2 with return: can only `return' from a function; the fix exits 1 with the intended message.

5. osv-scanner-pin.test.cjs had an assertion conditioned on the same value it then checked, so it passed if either wording was present and could not detect the guard's mismatch message being reworded. Proof, with the message reworded consistently in both the workflow and osv-scan-guard.sh: the old assertion gives pass 5 / fail 0, the corrected one pass 4 / fail 1.

6. Invoke-CompositeRunPssa.test.ps1 printed $custom.Output on failure in the path-guard loop, where $outside.Output is what that case produced — a failure would dump the previous case's output.

Counting bug

7. claude-lane-incident keyed unattributed observations on observations.indexOf(observation), which returns the first matching index, so two entries sharing an object reference collapsed to one key and the incident body under-reported blast radius. Now keyed on the true index from .entries(), which also removes an O(n²) scan.

Regression coverage for the two silent fixes

Both #3 and #7 were bugs that produced no failure signal, and both initially shipped without a test — raised in review, and correct. Each new test was checked in both directions: it passes against the fixed source and fails against the pre-fix source.

probe-billing-usage.test.cjs is a new file; that script previously had no test at all. Its regression guard asserts against the source rather than reproducing a truncated report, and that is deliberate: measured, the report embeds only endpoint paths, statuses and fixed strings, so a stub gh emitting 500 KB of stderr still yields a 1030-byte report on both the fixed and pre-fix versions. The truncation is latent, arriving the moment anything variable-length enters the report. A test that could only fail after that future change would not guard this fix.

Verification

Union pass across every ecosystem the run touched, on the final branch:

Check	Result
node --test	744 pass / 0 fail / 0 skipped (740 pre-sweep baseline, +4 new regression tests)
render.cjs --check (gates the required selector-contract job)	exit 0
Bash suites	9 pass / 0 real failures / 2 environmental
YAML parse	68/68 valid
biome at CI's exact scope	clean
PowerShell parse	no errors
File modes / CR bytes vs main	unchanged / 0

Each applied diff went through a dedicated refutation agent asked to produce a concrete counterexample. All returned NOT REFUTED, with the work shown:

• codegen engine — 1,645-case argv fuzz; byte-identical rendered output.
• shellcheck/run.sh — 40+ element shapes with a positive control proving the harness could detect divergence. Established the removed guard was dead code: on a bash old enough for it to matter, mapfile -d aborts first and both versions die identically.
• select-runner — 61-row element matrix, executed against the rendered workflow copy, not just the source.
• workflow-yaml — ~1.5M differential cases including exhaustive enumeration of all 299,593 strings of length 0–6 over a targeted alphabet, and branch instrumentation confirming the divergent case was genuinely exercised (~48,700 hits) rather than merely unvisited.
• test groups — assertion counts and kinds compared per file; the two shell suites produce byte-identical runtime case sets.

Later cleanups carried their own proofs: a differential over 0x00-0x7F plus 390 patterns for the regex-escape fold, key-order and value-identity checks over 2000 randomized maps for the Map rebuild, and a shellcheck shim capturing full argv plus file bytes across every tracked composite action (499 lines of extracted script, identical).

Honest gaps

• shellcheck, shfmt, actionlint, biome, lefthook and PSScriptAnalyzer are not installed in the sweep environment. This cost a CI cycle: the first run failed SC2016 on a jq_edit test helper, because ShellCheck recognises $urn as a jq variable only when the filter is a direct argument to jq — routing it through a shell function loses that. Fixed by inlining that one call site rather than suppressing the check.
• Roughly 46 of the 132 swept files (35%) have no behavior test behind them. Twelve action.yml files are touched only by generic repo-wide sweeps that assert nothing about their inputs, outputs, or step semantics. For those, the refutation pass and the union pass are the only checks that ran.
• PSScriptAnalyzer and Pester never ran locally; no claim is made that they passed.

Related

No linked issue.

Correction to an earlier version of this description

An earlier revision claimed comment-hygiene forbids issue references while five files carry them, and called it a rule/code disagreement. That was wrong. Running the repo's own chp::scan_text over all six files reports them clean, with a positive control confirming the scanner does flag TODO, FIXME, HACK, closes #123, owner/repo#12, GH-45 and issue 77. The ruleset targets closing-keyword+#N, owner/repo#N (requires the slash), GH-N and issue N; a bare (#200) or repo#399 is deliberately outside it. The header prose is a loose gloss — the patterns are the policy, and CI has been green on those files throughout. Nothing to change.

Findings closed rather than applied

Every remaining finding was worked in-branch. These were closed because applying them would be wrong, not merely unnecessary:

• The spawnSync triage longhand in security-review-absent-mitigate.cjs stays. status is null for ENOENT, signal, maxBuffer and timeout, so collapsing that triage is exactly how a missing binary comes to pass a guard.
• claude-lane-incident-write-gate.cjs's two audits stay separate; the file's stated thesis is deliberate redundancy between two independent mechanisms that cross-check each other.
• install-release.sh's two checksum calls gate different things (one the cache write, one the copy).
• A nonempty() helper in release-tag-drift.sh would introduce five new SC2310 findings — the repo already carries # shellcheck disable=SC2310 at five sites for exactly that shape. Trading four duplicated fragments for five suppressions is a net loss.
• The osv-scanner-pin.json fields no code reads are a human-readable audit record for a supply-chain pin.
• The dotnet setup fallback and the PowerShell NUL-reassemble idiom cannot be shared without a new action directory or a new .psm1 and a changed load contract for a consumer-facing action.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J3DMJtbY5RhnHEVAAFsnee

[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-08-30T22:13:02.902389Z	87bcb45	PR opened

ℹ️ 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 3m 21s —— View job

---

Review summary

Reviewed all 21 changed files: the four defect fixes against the specific failure modes described in the PR body, and the remaining simplification/test-refactor diffs for behavior preservation.

The four defect fixes are correct.

• managed-files-guard/run.sh — capturing via if ! changed_paths="$(git diff …)" (matching the existing dest-paths call in the same file) genuinely fixes the swallowed-exit-status bug; the added empty-diff short-circuit ([[ -z "$changed_paths" ]]) correctly avoids the mapfile <<<"" phantom-empty-element trap. → .github/actions/managed-files-guard/run.sh#L43-L62
• eol-renormalize/action.yml — read -r -d '' -a paths now matches the identical pattern already used in actionlint, biome, and editorconfig (verified by direct comparison), correctly fixing the first-line-only truncation. → .github/actions/eol-renormalize/action.yml#L29-L34
• claude-lane-incident.cjs and probe-billing-usage.cjs — both correct; left inline notes on each.

Simplification diffs preserve behavior. Spot-checked the riskiest ones directly rather than trusting the PR's own claims:

• render.cjs parseArgs — the removed arg !== "--check" filter was genuinely dead: --check already fails !arg.startsWith("-").
• workflow-yaml.cjs splitFlowEntries — the removed conditional push is provably redundant given the function's own trailing .filter((entry) => entry.trim() !== "").
• approval-agent-guardrails.cjs parseProtectedPaths — Set-based dedup preserves the original's ordering/precedence semantics (defaults first, first-occurrence-wins).
• claude-review-outcome-wiring.test.cjs — laneSource now reads through workflowsDir, confirmed to be byte-identical to the path it replaced (path.join(repositoryRoot, ".github", "workflows")).
• The select-runner.test.cjs / standards-sync-*.test.cjs fixture-hoisting diffs are mechanical; spot-checked several call sites' overrides against their originals and found no dropped or altered fixture keys.
• pulumi-deploy-guard/guard.test.sh jq_edit helper — checked argument reordering (jq_edit file filter --arg ... → reassembled as jq --arg ... filter file) reproduces the original invocation shape exactly, including the one call with --arg.

Finding (left as 2 inline comments): the four defect fixes ship with no regression tests. claude-lane-incident.cjs has an existing, extensive tallyObservations test file but no case for the unattributed-observation dedup path this bug lived in. probe-billing-usage.cjs has no test file at all. The other two (managed-files-guard, eol-renormalize) also have no test harness, which is a larger, pre-existing structural gap the PR body already discloses honestly under "Honest gaps" — I did not re-flag those two separately since adding first-time coverage for a composite-action shell script is a bigger lift than extending an existing .test.cjs. Given these are exactly the bugs being fixed (not incidental simplifications), a regression test is the difference between "fixed" and "fixed until the next refactor."

No correctness issues found in the fixes or the simplification diffs themselves.

[claude]

Claude finished @kyle-sexton's task in 3m 4s —— View job

---

Security review — PR #530

• Gather context (PR diff, changed files, CLAUDE.md)
• Review each changed file for security-relevant issues
• Post findings

Note: the /review:security-review skill failed to load in this session (it errored on invocation with no further detail), so this is a manual security review of the full diff against origin/main rather than the automated lane. Repo CLAUDE.md files are present but empty — no repo-specific review instructions to layer in.

Summary

No new vulnerabilities introduced by this diff. I read all 21 changed files (git diff origin/main...HEAD) with particular attention to the guard/security-relevant scripts, and independently re-derived correctness for the four defect fixes the PR describes.

The four fixes — verified

1. managed-files-guard/run.sh#L43-62 — genuine fail-open fix. mapfile -t changed < <(git diff …) discards git's exit status because process substitution isn't reaped by mapfile; a bad BASE_REF/HEAD_REF silently produced an empty change list and the guard exited 0. The new if ! changed_paths="$(git diff …)" shape correctly propagates the failure and exits 1. This is the most security-relevant fix in the set — it closes a path where the guard against unauthorized sync-manifest-managed file edits could be bypassed simply by starving it of a valid diff.
2. claude-lane-incident.cjs#L217-L228 — observations.indexOf(observation) returns the first matching index for reference-equal objects, collapsing distinct unattributed observations into one dedup key and undercounting incident blast radius. Keying on .entries() index is correct and removes the O(n²) scan as a side benefit. Confirmed this is a real under-reporting bug in an incident-monitoring script, not just a style change.
3. eol-renormalize/action.yml and probe-billing-usage.cjs — correctness fixes (multi-line pathspec truncation via read -ra; stdout truncation via process.exit() racing pending async writes on a >64KB JSON report). Neither is exploitable — worst case is a false-negative EOL check or a truncated billing report — but both are legitimate fixes, verified by tracing the actual bash read/Node stdout-drain semantics.

Everything else in the diff

The remaining 17 files are dead-code removal, redundant-guard removal, and test-fixture hoisting — all mechanical and behavior-preserving on inspection:

• approval-agent-guardrails.cjs parseProtectedPaths (protects approval-bypass paths) — rewritten but semantically identical dedup/ordering; traced both the raw == null/raw === 0 edge cases and the Set-based defaults-first ordering.
• security-review-absent-mitigate.cjs — the needsRulesets hoist calls resolveDefaultBranch/collectRulesetDetails under the exact same combined condition as before, just computed once instead of twice.
• shellcheck/run.sh — the ${arr[@]+"${arr[@]}"} → "${arr[@]}" simplification is safe because normal_files/extra_files are always initialized to () before use, so there's no unset-array case under set -u in any bash version.
• select-runner.cjs/.yml, render.cjs, workflow-yaml.cjs, render-compose.cjs — cosmetic/message-formatting or provably-redundant-filter simplifications with no security surface.
• Remaining files are test-only (.test.cjs/.test.sh) fixture/helper refactors.

No command-injection, credential-handling, or privilege-escalation issues found. Subprocess calls I checked (probe-billing-usage.cjs → spawnSync("gh", [...])) use argv arrays, not shell strings. git diff/git add --renormalize invocations pass caller input through env vars or quoted variables, consistent with the rest of the guard fleet.
· branch

[github-actions]

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

[github-actions]

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

[claude]

Claude finished @kyle-sexton's task in 1m 8s —— View job

---

Security review — PR #530

• Gather context (PR diff, prior review state, relevance gate)
• Determine scope: commits since last review
• Review changed files for security-relevant issues
• Post findings

Skip gate: no new security-relevant surface since the last review.

A successful security review already ran on this PR at head 87bcb45. The relevance gate scopes re-review to commits since that SHA, and the only commit since then is a3f92f2, which touches a single file: .github/actions/pulumi-deploy-guard/guard.test.sh.

That change inlines one jq_edit call as a direct jq --arg urn "$urn_one" '...' "$state" >"$state.tmp"; mv -- "$state.tmp" "$state" (a ShellCheck-motivated restructuring, per the commit message) instead of routing it through the shared helper. I read the surrounding test harness: $state is derived from a mktemp -d directory the script creates and controls itself (.github/actions/pulumi-deploy-guard/guard.test.sh#L4-6), not external or untrusted input, and the mv -- guards against a leading-dash filename. This is test-fixture-only code exercised in this repo's own CI, with no change to guard.sh's runtime logic, and it carries no injection, trust-boundary, or credential-handling surface.

Per the skill's skip gate (§"This head already has a successful security review that still applies"), the prior review at 87bcb45 — which covered all 21 files including the fail-open fix in managed-files-guard/run.sh, the dedup fix in claude-lane-incident.cjs, and the rest of the diff — still applies. No new findings.
· branch

[claude]

Claude finished @kyle-sexton's task in 1m 20s —— View job

---

Security review — PR #530

• Gather context (PR diff, prior review state)
• Attempt /review:security-review skill (failed to load — see note)
• Review changed files for security-relevant issues
• Post findings

Note: the /review:security-review skill failed to load in this session again (errored on invocation with no further detail, same as the earlier attempt on this PR), so this is a manual review of the diff, as before.

Scope

Prior reviews on this PR covered head 87bcb45 (full diff) and a3f92f2 (single test-fixture file, no findings). This pass covers the delta since then: a3f92f2..794f194 — commit 794f194, 17 files.

Summary

No new vulnerabilities introduced. The commit is the "clear the deferred inventory" pass: mostly dead-code removal, .at(-1)/.toSorted() modernization, and duplication hoists, plus a few genuine bug fixes. I read every file in the delta; two are worth calling out for security relevance, both fine on inspection:

1. osv-scan-guard.sh#L32-L48 and its byte-pinned twin in osv-scanner.yml — this is the workflow-command escaping used before emitting ::error file=...::message annotations built from OSV finding data, so it's on the injection-relevant path. escape_property now composes escape_data via v="$(escape_data "$1")" instead of duplicating the substitution chain. Traced it: escape_data encodes %, CR, and LF first, so its output can contain no literal newline — meaning the command-substitution's trailing-newline-stripping has nothing to strip, and escape_property's two additional substitutions (:, ,) run on the same encoded value the old inlined version produced. Order-of-encoding is preserved (% first, so the %3A/%2C output isn't re-escaped). Behavior-preserving, not a regression.
2. check-run-reconcile.cjs#L125 — matchRefPattern's regex-special-char escape set was reshaped from two checks ("\\^$.|()+[{}".includes(ch) || ch === "]" || ch === "/") into one string ("\\^$.|()+[]{}/".includes(ch)). This function turns a required-check ref pattern into a regex used for branch-protection matching, so a dropped character here would be a real gap. Diffed the two character sets by hand: both are exactly \ ^ $ . | ( ) + [ ] { } /, just reordered/consolidated. No character was dropped.

Everything else in the delta is non-security-relevant: .gitignore addition, set -euo pipefail added to two already-safe inline bash blocks (dotnet-format, markdown actions — no new untrusted-input handling), test-file fixture refactors (gitleaks/scan.test.sh, Invoke-CompositeRunPssa.test.ps1, osv-scanner-pin.test.cjs, the two standards-sync-*.test.cjs files), and mechanical simplifications in claude-lane-outcome/classify.cjs, claude-lane-incident.cjs, change-detection/match.sh, comment-hygiene/scan-tree.sh, composite-run-shellcheck.sh that don't touch trust boundaries.

One item worth flagging as a genuine improvement, not a defect: probe-billing-usage.cjs's ghApi now throws a distinct, clearer error when spawnSync itself fails to launch (result.error set, e.g. missing gh binary) instead of falling through to the generic status !== 0 branch. This doesn't change fail-closed behavior — the old code already threw in that case too (status is null !== 0) — it only improves the error message. Noted in the commit body as intentionally preserving the status !== 0 check.

No command-injection, credential-handling, privilege-escalation, or fail-open issues found in this delta.
· branch