From cbbe7ea925f9464881353f7945ccbca9d3767cf1 Mon Sep 17 00:00:00 2001 From: heznpc Date: Fri, 29 May 2026 06:42:49 +0900 Subject: [PATCH] =?UTF-8?q?chore:=20code-review=20fixes=20=E2=80=94=206=20?= =?UTF-8?q?findings=20from=20PR=20#47=20second-pass?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/publish.yml | 10 ++++----- src/audit-security.ts | 41 +++++++++++++++++++++++------------ src/mcp-schemas.ts | 18 +++++++++++---- src/seed-security-guidance.ts | 39 ++++++++++++++++++++++++++++----- 4 files changed, 79 insertions(+), 29 deletions(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 80901a0..696d842 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -64,12 +64,10 @@ jobs: # version mismatch (e.g., Claude Desktop ext manager vs npm). Gate # publish on parity. PKG_VERSION="${{ steps.pkg.outputs.version }}" - declare -A FILES=( - [server.json]="server.json" - [manifest.json]="manifest.json" - [.claude-plugin/plugin.json]=".claude-plugin/plugin.json" - ) - for path in server.json manifest.json .claude-plugin/plugin.json; do + # Single source of truth for the manifest set. Add a new entry here + # to extend the parity check — the loop iterates this array. + MANIFESTS=(server.json manifest.json .claude-plugin/plugin.json) + for path in "${MANIFESTS[@]}"; do if [ ! -f "$path" ]; then echo "::error::$path not found at repo root" exit 1 diff --git a/src/audit-security.ts b/src/audit-security.ts index bf2da76..6ae96d6 100644 --- a/src/audit-security.ts +++ b/src/audit-security.ts @@ -331,9 +331,19 @@ function checkClaudeSecurityGuidance(repoPath: string): SecurityCheckResult { ".claude-security-guidance.md", ".claude/security-guidance.md", ]; + // Restrict to regular files (and symlinks to files). existsSync alone would + // return true for a directory named `claude-security-guidance.md/`, which + // the consuming plugin can't parse as markdown — reporting PRESENT in that + // case would mislead the operator. statSync follows symlinks by design. const found = candidates .map((p) => join(repoPath, p)) - .filter((p) => existsSync(p)); + .filter((p) => { + try { + return statSync(p).isFile(); + } catch { + return false; + } + }); if (found.length > 0) { return { name: "claude-security-guidance", @@ -377,27 +387,30 @@ export async function auditSecurity(repoPath: string): Promise !c.optional); const summary = { - present: checks.filter((c) => c.status === "present").length, - missing: checks.filter((c) => c.status === "missing").length, - partial: checks.filter((c) => c.status === "partial").length, + present: core.filter((c) => c.status === "present").length, + missing: core.filter((c) => c.status === "missing").length, + partial: core.filter((c) => c.status === "partial").length, }; const issues = checks .filter((c) => c.status === "missing" || c.status === "partial") .map((c) => `${c.name} (${c.status}): ${c.recommendation ?? "no detail"}`); - // Verdict aggregator only counts CORE checks (CI primitives). Optional - // checks like `claude-security-guidance` (a repo-author content file) are - // surfaced in `issues` but don't downgrade the verdict on their own — a - // repo that has the full CI baseline stays HARDENED even before the author - // writes their org-specific guidance file. - const coreMissing = checks.filter((c) => c.status === "missing" && !c.optional).length; - const corePartial = checks.filter((c) => c.status === "partial" && !c.optional).length; - let verdict: AuditSecurityReport["overall"]["verdict"]; - if (coreMissing === 0 && corePartial === 0) verdict = "hardened"; - else if (coreMissing <= 2) verdict = "needs-attention"; + if (summary.missing === 0 && summary.partial === 0) verdict = "hardened"; + else if (summary.missing <= 2) verdict = "needs-attention"; else verdict = "soft"; return { diff --git a/src/mcp-schemas.ts b/src/mcp-schemas.ts index 5b08dcd..7870041 100644 --- a/src/mcp-schemas.ts +++ b/src/mcp-schemas.ts @@ -87,11 +87,21 @@ const securityCheckNameValues = [ // Compile-time exhaustiveness gate: `satisfies` only proves every array element // is a valid SecurityCheckName — it does NOT prove the reverse, that every // SecurityCheckName appears in the array. If a future check type is added to -// the union in audit-security.ts but not to the array above, `_MissingFromArray` -// resolves to the missing name(s), the conditional resolves to `never`, and -// the assignment fails compilation. Refresh the array, ship the fix. +// the union in audit-security.ts but not to the array above, _MissingFromArray +// resolves to the missing name(s) and the assignment below fails to compile. +// +// The branded error type names the missing variants in the TS2322 message: +// Type 'true' is not assignable to type +// '{ __exhaustivenessFailure: "Add these SecurityCheckName variants to securityCheckNameValues"; missing: "new-check"; }'. +// — so the developer sees WHICH check is missing without manually diffing. type _MissingFromArray = Exclude; -const _securityCheckExhaustive: [_MissingFromArray] extends [never] ? true : never = true; +type _ExhaustivenessGate = [M] extends [never] + ? true + : { + readonly __exhaustivenessFailure: "Add these SecurityCheckName variants to securityCheckNameValues"; + readonly missing: M; + }; +const _securityCheckExhaustive: _ExhaustivenessGate<_MissingFromArray> = true; void _securityCheckExhaustive; // VersionSource = "package.json" | ... | null — inline literal in diff --git a/src/seed-security-guidance.ts b/src/seed-security-guidance.ts index 62d7b8f..6731e3e 100644 --- a/src/seed-security-guidance.ts +++ b/src/seed-security-guidance.ts @@ -117,7 +117,15 @@ function buildContent(matchedId: StarterId | null): string { const starterSection = matchedId ? (SECTIONS_BY_STARTER[matchedId] ?? FALLBACK_SECTION) : FALLBACK_SECTION; - const today = new Date().toISOString().slice(0, 10); + // Local YYYY-MM-DD so the stamp matches the user's wall clock (and the + // git commit timestamp they're about to make). `toISOString().slice(0,10)` + // would return UTC, which is off-by-a-day for users running near local + // midnight in non-UTC zones. + const now = new Date(); + const yyyy = now.getFullYear(); + const mm = String(now.getMonth() + 1).padStart(2, "0"); + const dd = String(now.getDate()).padStart(2, "0"); + const today = `${yyyy}-${mm}-${dd}`; return `# Security guidance This file is read by Anthropic's Claude Code Security Guidance Plugin @@ -152,8 +160,8 @@ export function seedSecurityGuidance( } const filePath = join(abs, "claude-security-guidance.md"); - const exists = existsSync(filePath); - if (exists && !options.force) { + const preExisted = existsSync(filePath); + if (preExisted && !options.force) { return { repoPath: abs, filePath, @@ -166,13 +174,34 @@ export function seedSecurityGuidance( const sig = extractStarterSignals(abs); const content = buildContent(sig.id); - writeFileSync(filePath, content, "utf-8"); + + // Atomic create-or-overwrite with TOCTOU guard. Without `force`, use the + // `wx` flag so the kernel atomically refuses a write when another process + // created the file between our existsSync above and this call. We then + // re-report it as `status: "exists"` (the contract the caller asked for), + // not silently overwrite. With `force`, use `w` which truncates. + const flag = options.force ? "w" : "wx"; + try { + writeFileSync(filePath, content, { encoding: "utf-8", flag }); + } catch (e) { + if ((e as NodeJS.ErrnoException).code === "EEXIST") { + return { + repoPath: abs, + filePath, + matchedStarter: null, + status: "exists", + bytesWritten: 0, + relativePath: relative(abs, filePath), + }; + } + throw e; + } return { repoPath: abs, filePath, matchedStarter: sig.id, - status: exists ? "overwritten" : "created", + status: preExisted ? "overwritten" : "created", bytesWritten: Buffer.byteLength(content, "utf-8"), relativePath: relative(abs, filePath), };