chore: code-review fixes — 6 findings from PR #47 second-pass

.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

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<AuditSecurityRepo
     checkClaudeSecurityGuidance(abs),
   ];
 
+  // `summary` counts CORE checks only — the same set the verdict aggregator
+  // uses below. This keeps `summary.missing === 0 ⇔ verdict === "hardened"`
+  // as a stable invariant, so a CI gate written as
+  // `if [ $(... | jq .summary.missing) -ne 0 ]` agrees with the verdict.
+  //
+  // Optional checks (e.g. `claude-security-guidance`, a repo-author content
+  // file rather than a CI primitive) are not part of the CI quality bar.
+  // Their per-entry status is still visible in `checks[]` and is surfaced in
+  // `issues[]` when missing/partial — they're not hidden, they just don't
+  // show up in the headline counts callers gate on.
+  const core = checks.filter((c) => !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 {

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<SecurityCheckName, (typeof securityCheckNameValues)[number]>;
-const _securityCheckExhaustive: [_MissingFromArray] extends [never] ? true : never = true;
+type _ExhaustivenessGate<M> = [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

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),
   };