chore: code-review fixes — 6 findings from PR #47 second-pass#48
Closed
heznpc wants to merge 1 commit into
Closed
Conversation
All findings actionable from the extra-high-recall /code-review of PR #47. Severity ordering matches the review verdict. 1. audit-security.ts (medium): summary↔verdict asymmetry The summary counts included optional checks (e.g. claude-security-guidance) while the verdict aggregator filtered them out. Result: a repo without the guidance file showed 'summary.missing=1, verdict=hardened' simultaneously, so a CI gate written as 'fail if summary.missing > 0' fired false-positive. Now summary counts CORE checks only (matching verdict semantics), and 'summary.missing === 0 ⇔ verdict === "hardened"' is a stable invariant. Optional checks remain visible in checks[] and issues[]. 2. publish.yml (low): dead 'declare -A FILES' associative array The array was built but the loop iterated three hardcoded literals. A future maintainer extending FILES would get a silent skip. Replaced with a plain bash array MANIFESTS=(...) that the loop actually iterates. 3. seed-security-guidance.ts (low): TOCTOU on existsSync→writeFileSync Two concurrent invocations both seeing existsSync()=false could both write — silently breaking the 'force: false' contract. Now we let the kernel adjudicate via writeFileSync({ flag: 'wx' }) and catch EEXIST to report status:'exists' (the contract the caller asked for) instead of silently overwriting. 4. audit-security.ts (low): existsSync doesn't reject directories A repo with a directory named 'claude-security-guidance.md/' would be reported PRESENT, but the consuming plugin can't parse a directory as markdown. Now uses statSync(p).isFile() with a try/catch fallback. 5. seed-security-guidance.ts (low): UTC date in generated file stamp new Date().toISOString().slice(0,10) returned UTC, so users running near local midnight in non-UTC zones got an off-by-a-day stamp that didn't match their git commit timestamp. Verified live: local date 2026-05-29 / UTC 2026-05-28 → file now stamps 2026-05-29 (was 05-28). Switched to local year/month/day components. 6. mcp-schemas.ts (info): cryptic exhaustiveness gate error When a new SecurityCheckName variant was added without extending securityCheckNameValues, tsc emitted 'Type true is not assignable to never' with no hint about which variant was missing. Replaced the conditional with a branded error type that names the missing variant in the TS2322 message: __exhaustivenessFailure + missing: M. Verification: - npm run build (tsc) clean - npm test: 97/97 pass - audit_security on this repo: 'Overall: HARDENED (8 present 0 partial 0 missing)' (pre-fix would have shown '0 partial 1 missing' for the optional guidance-file check while still reading HARDENED — asymmetry resolved) - seed-security-guidance smoke: CREATED → EXISTS (no force) → OVERWRITTEN (with --force); generated date stamp matches local calendar.
Member
Author
|
Closing: superseded and regressive vs main (downgrades fetch-metadata v3.1.0->v2.3.0, reverts publish.yml cleanup, deletes deploy-setup skill). The 9th-check wiring it carried is already on main via other PRs. |
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.
Summary
All 6 actionable findings from the extra-high-recall
/code-reviewof PR #47, ranked by severity.src/audit-security.ts:380summarynow counts CORE checks only (matchingverdictsemantics).summary.missing === 0 ⇔ verdict === "hardened"is now a stable invariant; a CI gate written asjq .summary.missingno longer disagrees with the verdict..github/workflows/publish.yml:67declare -A FILES=(...)whose entries the for-loop never read. Replaced withMANIFESTS=(...)that the loop actually iterates — extending the parity check is now a single-edit operation.src/seed-security-guidance.ts:154writeFileSync({ flag: 'wx' })for the non-forcepath so the kernel adjudicates the race. CatchEEXISTand reportstatus: "exists"(the contract the caller asked for) instead of silently overwriting.src/audit-security.ts:334existsSync→statSync(p).isFile()with try/catch. A repo with a directory namedclaude-security-guidance.md/no longer reports PRESENT (the plugin can't read a directory as markdown).src/seed-security-guidance.ts:1202026-05-29(was2026-05-28).src/mcp-schemas.ts:93__exhaustivenessFailure + missing: M. Saves the developer from diffing the union vs the array.Out-of-scope (intentional, documented in review)
softis the worst status (least hardening), not the best. Severity ordering reads correctly.as unknown as Record<string, unknown>insrc/index.tsaudit handlers: unavoidable SDK ↔ AuditReport bridge (covered in/simplify).writeFileSyncnomodeargument: the generated file is meant to be committed and world-readable; default 0o644 is the intent.Test plan
npm run buildcleannpm test— 97/97 passaudit-securityon this repo reports8 present 0 partial 0 missing+verdict: HARDENED(pre-fix would have shown0 partial 1 missingbecause the optional guidance file is absent here, while still reading HARDENED — the asymmetry this PR fixes).seed-security-guidance:CREATED → EXISTS (no force) → OVERWRITTEN (with --force); generated date stamp matches local calendar.🔗 Methodology limit (documented at end of
/code-review):semgrep/trufflehognot executed locally, so this PR doesn't add coverage for finding patterns those tools surface. Trust CI.