fix: recover 21 unpushed release-gate commits and share feature KBs - #307
Merged
Conversation
…dium, 2×low) Critical — Tier B silently skipped non-required runs whose status was not 'completed' (queued/in_progress). A PR with 6 required contexts all green plus Source hygiene=queued yielded exit 0 and printed the --match-head-commit merge command. Fix: Tier B now FAILs (exit 1) on any non-completed non-required run (avoids PF-017: indeterminate state is never success). High — Tier B cannot assert *presence* of a non-required job — it only evaluates runs that already appear in the check-run list. An absent Source hygiene job produced no Tier B entry and the verifier exited 0 (the PoC from the review finding). Fix: add EXPECTED_CONTEXTS = ['Source hygiene'] with Tier A semantics (Tier A+). Absence is FAIL, not advisory (applies ADR-009, avoids PF-013). Medium — defaultGhRunner returned `status: r.status` (the gh process exit code, always 1) but fetchRequiredContexts branched on `data.status === 404 / 403`. Both branches were unreachable on the live path (gh exits 1, never 404/403), so the AC-29 remediation message never printed and the two tests for those branches validated dead code. Fix: parse `(HTTP NNN)` from gh's stderr into a separate `httpStatus` field; fetchRequiredContexts branches on `data.httpStatus`; stub runner updated to mirror the parsed shape (avoids PF-013). Low — fetchStatuses used the combined-status endpoint which caps at 30 statuses with no pagination. A required context backed by a status beyond position 30 would have been falsely reported as never-ran. Fix: add a total_count guard consistent with D-PR4a; if total_count > returned statuses, exit 2 rather than evaluate a partial set. Low — Tier A resolved each required context with if/else-if: check-runs took priority and the status namespace was only consulted when no check-run existed. A failing commit status was silently ignored when a check-run of the same name was green, diverging from GitHub's enforcement model (D-PR2a). Fix: check both namespaces independently for every required context. Test coverage: 59 tests (14 suites), 0 failures. New test groups verify the EXPECTED_CONTEXTS PoC (absence → exit 1), Tier B non-completed (queued → exit 1), both-namespace Tier A (status failure not masked), fetchStatuses total_count guard, and httpStatus branching (dead-branch elimination). Co-Authored-By: Claude <noreply@anthropic.com>
The AC-1/AC-2 Code of Conduct describe block was split into its own scripts/__test__/code-of-conduct.spec.mjs by commit 2e9482f. My prior write re-added it; this commit removes the duplicate and drops the now- unused `statSync` and `createHash` imports (56 tests remain, 0 failures). Co-Authored-By: Claude <noreply@anthropic.com>
Finding 1 (AC-11, dead code): `r2` (the post-`git rm --cached` scanner run) was declared but never asserted, so the zero-tracked-files → non-vacuity behavior was documented only in a comment. Add `assert.equal(r2.status, 1)` and a stderr check for the zero-files message, making the assertion live. Finding 2 (AC-7 coverage gap): U+FEFF (BOM, 0xEF 0xBB 0xBF) and U+2028 (Line Separator, 0xE2 0x80 0xA8) were covered only by unit-level `isHazardous` assertions. Add two planted-file positive controls (PC-6 and PC-7) that run each codepoint through the full decode → predicate → report pipeline, exercising the two members most likely to be special-cased by a future scanner change. Bytes constructed from hex arrays at runtime — no backslash-u escapes (PF-018). All 39 tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
… staged divergence HIGH finding: `git commit --amend -m ...` and `git commit --allow-empty` were rejected by the pre-commit hook (empirically confirmed). The code fix shipped in fb8befe, but the required named regression tests for these two specific git workflows were missing. Add two tests in the AC-5/AC-6 suite that each make a real prior commit (so HEAD exists) and then run --staged with nothing newly staged, faithfully simulating what git passes to the hook during an amend or allow-empty commit. Both must exit 0; the scanner exits 0 with an explicit "nothing to scan" message. LOW finding: getStagedFiles hardcodes mode 0o100644 for every staged path, because `git diff --cached --name-only` carries no mode information. This means the D-CB5a symlink/gitlink skip (unconditional in full-tree mode) is not applied in --staged mode. Add a JSDoc note explaining the divergence and why it is benign (a staged symlink's blob is the target path — plain ASCII, never triggers false positives; the full-tree scan is the authoritative AC-20 enforcement point). Tests: 41 pass (39 existing + 2 new), 0 fail. Co-Authored-By: Claude <noreply@anthropic.com>
…oop index, exit-code note, isMainModule symmetry)
Finding 1 [medium]: add explicit timeout:30_000 to defaultGhRunner and
gitExec, timeout:10_000 to ghVersion. ETIMEDOUT maps to exit 2
(indeterminate, never 0) in all three production spawnSync calls.
Enforces a hard per-call upper bound required by AC-30.
Finding 2 [low]: add one sentence to CONTRIBUTING.md Source-hygiene
section noting that the scanner folds git-missing into its fail-closed
exit 1 while the verifier classifies gh-missing as indeterminate exit 2.
Also corrects the stale "Tier B skips queued/in_progress" prose (the
correct post-fix description is "Tier B fails on non-completed runs").
Finding 3 [low]: remove `export` from isMainModule in
verify-no-control-bytes.mjs so both verify-*.mjs scripts follow the
same private-helper-for-standalone-ness convention documented in the
verify-pr-checks.mjs comment.
Finding 4 [high]: add a dedicated 'Tier B' describe block with six
tests — queued, in_progress, failure, cancelled (all FAIL), skipped and
neutral (advisory only, PASS). The skipped/neutral positive control
proves the suite is not unconditionally failing (applies ADR-009).
Finding 5 [low]: replace `for (const cr of crs)` with
`for (const [idx, cr] of crs.entries())` and emit `(${idx+1} of N
runs sharing this name)` so each failing duplicate run in Tier A shows
its own 1-based position rather than always printing "(1 of N)".
Add a regression test with 3 co-named runs that asserts all three
indices appear in the output.
Co-Authored-By: Claude <noreply@anthropic.com>
…coverage, parseGhStderrHttpStatus) Low finding: add per_page=100 to fetchStatuses URL so a required context backed by commit status #31+ is not silently absent from the set. The total_count guard was already present; this adds the parameter that makes it effective. Critical finding: extend Tier B test coverage to all five TIER_B_FAIL conclusions. Prior tests covered failure and cancelled; timed_out, action_required, and stale were untested — a mutation that removed them from TIER_B_FAIL would have gone undetected. High finding: extract parseGhStderrHttpStatus as an exported pure function so the stub contract used throughout the test suite can be pinned to the production parsing contract. The function is unit-tested against captured real gh stderr strings ("gh: Not Found (HTTP 404)", "gh: Forbidden (HTTP 403)", connection refused, null/undefined) — applies ADR-009, avoids PF-013 dead branches. Add per_page=100 URL assertion to AC-30. 72 tests, 0 failures. Co-Authored-By: Claude <noreply@anthropic.com>
node --test '<glob>' exits 0 printing '# tests 0' when the glob matches
nothing (GitHub Actions runs under bash with failglob/nullglob OFF, so an
unmatched pattern is passed through literally and Node globs internally,
matches nothing, and exits 0). This made the gate pass vacuously if
scripts/__test__/ was ever renamed, moved, or emptied — violating
ADR-009 and PF-013.
Fix: prepend `shopt -s failglob` in both the ci.yml source-hygiene step
and the release.yml version-gate step. Under bash -e (the GitHub Actions
default), a glob that matches nothing now aborts with exit 1, making the
gate truly fail-closed.
Verified locally:
- glob expands correctly to the 3 spec files when the dir exists
- shopt + unmatched glob → exit=1 ("no match")
- verify-no-control-bytes.mjs still exits 0 on the patched YAML
…ease.yml)
node --test '<glob>' exits 0 printing '# tests 0' when the glob matches
nothing. GitHub Actions runs run: blocks under bash with failglob/nullglob
OFF, so an unmatched pattern is passed through literally; Node globs it
internally, matches nothing, and exits 0. If scripts/__test__/ were ever
renamed, moved, or emptied the step would turn GREEN having executed zero
tests — a gate that passes on nothing, violating ADR-009 and PF-013.
Fix: prepend `shopt -s failglob` before the glob in both
- .github/workflows/ci.yml (source-hygiene job, step 3)
- .github/workflows/release.yml (version-gate job, step 5)
Under bash -e (the GHA default), a glob that matches nothing now causes
bash to abort with "no match" and exit 1.
Verified locally:
- glob expands to all 3 spec files when the dir exists → exit 0
- shopt + unmatched glob pattern → exit 1 ("no match")
- verify-no-control-bytes.mjs exits 0 on the patched YAML (no PF-018 trip)
The scope paragraph (line 167) said "Tier A+ and Tier B are the binding mechanisms for non-required jobs" without distinguishing what each covers. A reader could conclude the two tiers are equivalent and that EXPECTED_CONTEXTS is redundant with Tier B — silently reopening the PF-013 absence hole if deleted. Replace with explicit complementary-roles language: Tier A+ covers absent and renamed jobs (Tier B cannot: it only iterates existing check-runs); Tier B covers existing runs that concluded badly. This makes it impossible to infer that Tier B alone is sufficient, closing the documentation ambiguity identified in the confirmed review finding. Co-Authored-By: Claude <noreply@anthropic.com>
…-009/PF-013) The bare `node --test scripts/__test__/*.spec.mjs` glob exits 0 with `# tests 0` when no spec files match — a fail-open that makes a renamed or moved spec directory silently un-detectable (delta commit 2c23d22). Fix: add a non-vacuity count assertion to the centralised `test:gates` npm script; ci.yml and release.yml delegate to it via `npm run test:gates` so the guard cannot be bypassed by either consumer independently. Guard: counts spec files with `ls ... 2>/dev/null | wc -l`; exits 1 with a clear diagnostic if fewer than 3 files are found (today: 3 files). Applies to all three consumers noted in the findings: - .github/workflows/ci.yml (formerly run directly in-step) - .github/workflows/release.yml (formerly run directly in-step) - package.json test:gates (RELEASING.md pre-flight) Verified: `npm run test:gates` passes — 116 tests, 0 fail. Verified: guard fires at count 0 and count 2; passes at count 3. Co-Authored-By: Claude <noreply@anthropic.com>
…mary Two defects in the exit-code contract documentation identified in the confirmed review finding: 1. CHANGELOG.md described the verifier as having "three tiers" and its exit code contract omitted Tier A+ entirely. Add Tier A+ (EXPECTED_CONTEXTS) between Tier A and Tier B, update the tier count to four, and update the exit-0 and exit-1 descriptions to include Tier A+. 2. verify-pr-checks.mjs FAIL summary line emitted "required context(s) not satisfied" regardless of which tier(s) caused the failure. Tier A+ failures concern locally-expected (not required) contexts; Tier B failures concern non-required check-runs. Replace with "check(s) did not pass", which is accurate for all three failing tiers and the zero-check-runs case. Individual failure lines already carry "Tier A (required): ...", "Tier A+ (expected): ...", and "Tier B (non-required): ..." prefixes that give the operator the full breakdown. All 72 verify-pr-checks tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
`<pr-number>` in the copy-pasteable bash block was unquoted shell input redirection from a file named 'pr-number', causing 'no such file or directory' on copy-paste. Replace with an assignable variable pattern: PR_NUMBER=NNN # replace NNN with the bump PR number node scripts/verify-pr-checks.mjs "$PR_NUMBER" Every other line in the block is runnable verbatim; this brings the verify-pr-checks invocation into line with that expectation. Co-Authored-By: Claude <noreply@anthropic.com>
…S to ci.yml job names - verify-pr-checks.mjs: emit `--admin` in the PASS merge command so operators can copy it verbatim without hand-editing (D-PR5; avoids PF-017 drop-risk) - evaluateChecks: emit advisory line for pending non-required statuses rather than swallowing them silently (Tier C behaviour documented) - verify-pr-checks.spec.mjs: pin EXPECTED_CONTEXTS against actual ci.yml job names instead of against itself (tautology) — a job rename now breaks the test, preventing silent drift (avoids PF-013) - Update PASS exit-code test to assert --admin in merge command Co-Authored-By: Claude <noreply@anthropic.com>
…d timeout Three correctness fixes for verify-no-control-bytes.mjs --staged mode: 1. D-CB5b: add T (type-change) to --diff-filter so a tracked symlink replaced by a regular file containing a hazard codepoint is not silently excluded. The T blob is a regular file with content; the scanner must inspect it. 2. Use NUL as the cat-file --batch input delimiter (matching getStagedFiles' -z output). A LF-containing git path would otherwise split into two batch requests and desynchronize the response parser, producing garbage reads. Pass -z to git cat-file --batch to match. 3. Add timeout: 30_000 to the cat-file spawnSync call. An index lock or network-FS hang in the pre-commit hook must not block indefinitely. ETIMEDOUT is indeterminate → exit 2, never 0 (applies ADR-009). Fix the deletion-path reporting to use --diff-filter=D explicitly, which is correct when a T-type path is the only staged entry (the earlier unfiltered diff included T paths that ACMRT already handles, giving a misleading count). Co-Authored-By: Claude <noreply@anthropic.com>
- release.yml: replace HAZARD_RANGES-pinning comment with accurate description of the test:gates non-vacuity guard (>=3 spec files, ADR-009/PF-013) - verify-no-control-bytes.mjs: update D-CB5 doc strings from ACMR to ACMRT to reflect the D-CB5b type-change addition (committed in prior fix) Co-Authored-By: Claude <noreply@anthropic.com>
The file-header comment for D-PR5 described the PASS merge command as '--match-head-commit <headSha>' without mentioning --admin. The emitted command (line 428) already includes --admin following commit 454794f; this commit aligns the module-level doc to match actual behaviour. avoids PF-017 Co-Authored-By: Claude <noreply@anthropic.com>
Case C: a type-change (T) staged blob — a tracked symlink replaced by a regular file containing ESC (0x1B) — must be detected by the --staged scanner with exit 1. Before the D-CB5b fix (--diff-filter=ACMRT), --diff-filter=ACMR excluded T silently and the hook reported exit 0. This test is the positive control required by ADR-009 / PF-013: it proves the gate detects hostile content in a T-type staged blob, not merely that absence implies clean (applies ADR-009, avoids PF-013). Bytes constructed at runtime from hex literals, never embedded as backslash-u escapes (applies D-CB2, avoids PF-018). Co-Authored-By: Claude <noreply@anthropic.com>
…t (D-CB5a)
Two correctness fixes for verify-no-control-bytes.mjs --staged mode:
1. D-CB5a: change getStagedFiles() from `git diff --cached --name-only -z`
to `git diff --cached --raw -z` so that the new-file mode is available
for each staged entry. Entries with new-mode 160000 (gitlink/submodule)
and 120000 (symlink) are now marked skip:true before they reach
readAllIndexBlobs(), consistent with full-tree mode's AC-20 behavior.
Without this fix, a staged submodule would reach git cat-file --batch,
which returns 'missing' for a commit-typed index entry, causing exit 2
with the misleading message "staged path not in index: <submodule>".
The raw format is parsed per entry: header starts with ':' and carries
old-mode, new-mode, SHAs, and status; rename/copy (R/C) entries have
two path tokens and the new path is taken for scanning. The skipped
count is now propagated to the passStats message exactly as in full-tree
mode ("N symlink/gitlink skipped").
2. AC-6 AMENDMENT: document in the top-level D-CB5 JSDoc that --staged mode
intentionally exits 0 for a legitimately-empty content-bearing set
(deletion-only commits, amend -m, --allow-empty). This was previously
described as a "carve-out" but not explicitly called out as an AC-6
amendment. The full-tree CI scan remains the authoritative AC-6 gate.
Test: add "AC-20: a staged gitlink (mode 160000) is skipped in --staged
mode" to the spec. Uses git update-index --cacheinfo 160000,<sha>,sub to
inject a gitlink directly into the index without requiring a real submodule
on disk. Proves exit 0 + "symlink/gitlink skipped" in the output.
All 43 tests pass. Scanner exits 0 on the full repo tree.
Co-Authored-By: Claude <noreply@anthropic.com>
…ing (PF-015) Replace 'Symlink blobs contain only the ASCII target path and are safe to skip' with an accurate qualified statement: POSIX permits any byte except NUL and '/' in symlink targets, so control bytes are theoretically possible and scanning them could produce false-positive gate failures. The consequence of such a false positive is fail-closed (exit 1), so impact is minimal, but absolute phrasing in a normative comment is the shape PF-015 warns against. Co-Authored-By: Claude <noreply@anthropic.com>
…e test - verify-no-control-bytes.mjs: convert sequential `if`+exit chains to if/else if/else in gitExec and readAllIndexBlobs (makes mutual exclusion explicit), remove spurious blank line after hexContext, split the long hazardHits.push() into a multi-line object literal. - verify-pr-checks.mjs: replace chained ternary in defaultGhRunner error handling with if/else if/else (follows project style — no nested ternaries). - verify-no-control-bytes.spec.mjs: remove stale `scanner:611` line-number references from the AC-30 describe block; reflow the comment. - verify-pr-checks.spec.mjs: remove the "contract parity check" test whose two assertions are exact duplicates of the preceding "extracts 404" and "extracts 403" tests. Its explanatory note (use `httpStatus:404`, not `status:404`) is moved into the describe block comment. All 119 tests pass. No behavior change. All positive controls, D-CB* and D-PR* decision markers, and ADR-009/PF-013 guards are intact.
Scanner (P0-Functionality): allowlist staleness was adjudicated against `fileEntries`, which is the COMPLETE tracked set only in full-tree mode. In --staged and explicit-path modes it is a subset, so any allowlist entry the commit did not happen to touch was reported as "file is no longer tracked" — a factually false claim that exits 1. Latent today (both allowlists are empty by D-CB4), but it fires on the first legitimate entry and then rejects every commit through the pre-commit hook, training contributors toward --no-verify, which disables the gate for ALL commits. Same reasoning as the AC-6 --staged carve-out. Staleness is now adjudicated in full-tree mode only (D-CB6a); the full-tree CI scan remains the authoritative validator. Hazard SUPPRESSION is path-keyed and still applies in every mode. Regression coverage (applies ADR-009, avoids PF-013): Case 4 asserts --staged does not misreport an untouched allowlisted file; Case 5 is its positive control, proving full-tree mode still reports a genuinely stale entry so the fix cannot be satisfied by deleting the check; Case 6 pins that suppression still works in --staged while an identical hazard in a non-allowlisted staged file still fails. Verified non-vacuous against the pre-fix scanner: the Case 4 scenario exits 1 before the fix and 0 after. Docs (P2-Consistency): verify-pr-checks.mjs emits `--admin`, but four documents quoted the command without it. Corrected in PULL_REQUEST_TEMPLATE.md, CLAUDE.md, CHANGELOG.md and CONTRIBUTING.md so the printed command can be copied verbatim (RELEASING.md was already correct).
Adds the seven feature knowledge bases that .gitignore already whitelists (`!.devflow/features/*/KNOWLEDGE.md`, "feature knowledge bases are shared via git") and that the tracked .devflow/features/index.md already references: bundler-plugins, mds-cli, mds-compiler, mds-fmt, mds-js, mds-napi, source-map-security mds-lint/KNOWLEDGE.md was already tracked; this brings the rest of the index in line so every referenced knowledge base resolves. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists
PR #293 squash-merged into the wave as
282af17, but it merged a stale head.ticket/pr6-community-release-safetyhas 35 commits; only the first 14 (throughfb8befe) were in the merged head. The remaining 21 commits — +1375/−180 across 12 files — were never pushed to any remote and never landed. They are post-review hardening of the #288/#289/PF-017 release-safety gates.gh pr view 293reportsheadRefOid: fb8befe, and the wave's282af17tree is byte-identical tofb8befe, confirming the gap.This was caught during a pre-merge consolidation audit of the wave, before the wave→main merge.
What was missing
scripts/verify-pr-checks.mjsomitted--adminfrom its emitted merge command, whileRELEASING.mdstates--adminis required becausemainis protected and the sole code-owner cannot self-approve. The tool's own emitted command did not work as documented — and it is the tool the wave→main merge depends on. Now fixed atscripts/verify-pr-checks.mjs:432.verify-pr-checks.spec.mjs37 → 73 tests;verify-no-control-bytes.spec.mjs37 → 46.parseGhStderrHttpStatus, theper_pagepagination fix, subprocess timeouts (30s I/O, 10sgh --version),--stagedACMRT type-change handling, the D-CB6a full-tree staleness fix, gitlink/symlink mode skips, and fail-open glob guards in the positive-control suite steps ofci.ymlandrelease.yml.How it was recovered
Cherry-picked
fb8befe..3aff248as the full 21-commit series (not squashed), so each fix keeps its rationale. Zero commits skipped — none were empty.Nine files picked clean (untouched by the wave since
282af17). Three had conflicts because later wave commits changed them:ci.yml,RELEASING.md(both auto-merged), andCHANGELOG.md(two manual resolutions).The CHANGELOG resolutions are the part worth reviewing.
e23834drestructured[Unreleased]into Keep a Changelog order, so git saw an empty HEAD side against a ~521-line incoming block. Taking the incoming side wholesale would have silently reverted four wave changes: themdscript→markdown_scriptPyPI rename (7072df0), the WASM size figure (c9265b4), themds lintrewording, and the stdin"input.mds"source label (cbb11d4). Resolution took HEAD's file and hand-applied only the genuine payload (the Tier A+ text and the--adminfix). One cosmetic deviation from the source commit: the--adminline was reflowed from 101 chars to the file's ~80-col convention; content identical.Verified no wave content was lost: the recovered delta is exactly
+1375/−180, identical to the source range. All 9 clean-pick files are byte-identical to3aff248; the 3 conflicted files retain wave content.ci.ymlkeeps thesource-hygienejob, thedtolnay/rust-toolchain@1.96.0pin, the::error+exit 1upgrade, theRUSTDOCFLAGSintra-doc step, and themarkdown-scriptwheel smoke. The job nameRust — fmt, clippy, testis unchanged atci.yml:22— it is a required branch-protection context onmainand renaming it would break the merge gate.Second commit — feature knowledge bases
Commits the 7 previously-untracked
.devflow/features/*/KNOWLEDGE.mdfiles (1,421 lines)..gitignore:64-70explicitly whitelists them ("feature knowledge bases are shared via git") and the already-tracked.devflow/features/index.mdreferences them, so the committed index was pointing at content absent from git.Verification
npm run test:gates122 pass / 0 fail ·verify-no-control-bytesexit 0 (527 files, 5,479,912 bytes) ·verify-versionsexit 0 ·cargo fmt --all --checkclean ·cargo clippy --workspace --all-targets -D warningszero warnings ·cargo nextest run --workspace2087 passed ·cargo test --doc52 passed ·npm run build --workspaces+npm test --workspaces575 tests, 0 fail.The recovered glob guard was positive-controlled rather than trusted green: it exits 1 at 2 spec files and 0 at 3, and the repo has exactly 3 — so it sits at the threshold by design and trips if any spec file is deleted.