fix(review): reject empty arrays in isNonEmptyStringArray + honor injected roots on config failure

.claude/skills/_devkit/review-roots.mjs

@@ -1,8 +1,15 @@
 import { readFileSync } from 'node:fs';
 import { isAbsolute, win32 } from 'node:path';
 
+// `length > 0` is load-bearing, not belt-and-braces: `[].every(...)` is vacuously TRUE, so without it
+// this predicate accepts `[]` — contradicting its own name. The correctness checklist then takes
+// `sourceExtensions: []` as valid, `exts.some(...)` is false for every path, and the reviewer passes
+// having examined ZERO files. A gate that silently verifies nothing is the one failure mode devkit
+// refuses to ship; an empty list must fall through to the caller's default.
 export const isNonEmptyStringArray = (value) =>
-  Array.isArray(value) && value.every((entry) => typeof entry === 'string' && entry.length > 0);
+  Array.isArray(value) &&
+  value.length > 0 &&
+  value.every((entry) => typeof entry === 'string' && entry.length > 0);
 
 /** Normalize trusted repository-relative roots so selector and Git pathspec readers agree. */
 export function normalizeReviewRoots(value, name) {

.claude/skills/correctness/scripts/checklist.mjs

@@ -58,16 +58,25 @@ const log = console.log;
 // never silently no-ops). A PRESENT but invalid value warns loudly and is ignored (the other
 // roots still count), rather than crashing the git call into an empty pass-through.
 function unionRoots() {
+  // Injected roots are read BEFORE the config, and the config's failure paths fall through to `{}`
+  // rather than returning early. A review run carries its effective topology in the environment; the
+  // old order returned `['.']` the moment guard.config.json was missing or malformed, discarding an
+  // explicit injected scope and silently widening the reviewer to every staged file. The env is the
+  // more authoritative source here, so it must not be gated behind the less authoritative one.
+  // (resolveReviewRoots in _devkit/review-roots.mjs already orders it this way — this is the local
+  // union catching up to the shared helper.)
+  const injectedBackend = parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS');
+  const injectedFrontend = parseInjectedReviewRoots('DEVKIT_REVIEW_FRONTEND_ROOTS');
   let c;
   try {
     c = JSON.parse(readFileSync('guard.config.json', 'utf-8'));
   } catch {
-    return ['.'];
+    c = {};
   }
-  if (!c || typeof c !== 'object') return ['.'];
+  if (!c || typeof c !== 'object') c = {};
   const review = typeof c.review === 'object' && c.review !== null ? c.review : {};
-  const backend = parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS') ?? review.backendRoots;
-  const frontend = parseInjectedReviewRoots('DEVKIT_REVIEW_FRONTEND_ROOTS') ?? review.frontendRoots;
+  const backend = injectedBackend ?? review.backendRoots;
+  const frontend = injectedFrontend ?? review.frontendRoots;
   const roots = new Set();
   for (const [label, value] of [
     ['scanRoots', c.scanRoots],

.cursor/skills/_devkit/review-roots.mjs

@@ -1,8 +1,15 @@
 import { readFileSync } from 'node:fs';
 import { isAbsolute, win32 } from 'node:path';
 
+// `length > 0` is load-bearing, not belt-and-braces: `[].every(...)` is vacuously TRUE, so without it
+// this predicate accepts `[]` — contradicting its own name. The correctness checklist then takes
+// `sourceExtensions: []` as valid, `exts.some(...)` is false for every path, and the reviewer passes
+// having examined ZERO files. A gate that silently verifies nothing is the one failure mode devkit
+// refuses to ship; an empty list must fall through to the caller's default.
 export const isNonEmptyStringArray = (value) =>
-  Array.isArray(value) && value.every((entry) => typeof entry === 'string' && entry.length > 0);
+  Array.isArray(value) &&
+  value.length > 0 &&
+  value.every((entry) => typeof entry === 'string' && entry.length > 0);
 
 /** Normalize trusted repository-relative roots so selector and Git pathspec readers agree. */
 export function normalizeReviewRoots(value, name) {

.cursor/skills/correctness/scripts/checklist.mjs

@@ -58,16 +58,25 @@ const log = console.log;
 // never silently no-ops). A PRESENT but invalid value warns loudly and is ignored (the other
 // roots still count), rather than crashing the git call into an empty pass-through.
 function unionRoots() {
+  // Injected roots are read BEFORE the config, and the config's failure paths fall through to `{}`
+  // rather than returning early. A review run carries its effective topology in the environment; the
+  // old order returned `['.']` the moment guard.config.json was missing or malformed, discarding an
+  // explicit injected scope and silently widening the reviewer to every staged file. The env is the
+  // more authoritative source here, so it must not be gated behind the less authoritative one.
+  // (resolveReviewRoots in _devkit/review-roots.mjs already orders it this way — this is the local
+  // union catching up to the shared helper.)
+  const injectedBackend = parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS');
+  const injectedFrontend = parseInjectedReviewRoots('DEVKIT_REVIEW_FRONTEND_ROOTS');
   let c;
   try {
     c = JSON.parse(readFileSync('guard.config.json', 'utf-8'));
   } catch {
-    return ['.'];
+    c = {};
   }
-  if (!c || typeof c !== 'object') return ['.'];
+  if (!c || typeof c !== 'object') c = {};
   const review = typeof c.review === 'object' && c.review !== null ? c.review : {};
-  const backend = parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS') ?? review.backendRoots;
-  const frontend = parseInjectedReviewRoots('DEVKIT_REVIEW_FRONTEND_ROOTS') ?? review.frontendRoots;
+  const backend = injectedBackend ?? review.backendRoots;
+  const frontend = injectedFrontend ?? review.frontendRoots;
   const roots = new Set();
   for (const [label, value] of [
     ['scanRoots', c.scanRoots],

.devkit/skills-manifest.json

@@ -1,39 +1,39 @@
 {
-  "devkitRef": "v0.35.2",
-  "generatedAt": "2026-07-18T03:27:27.833Z",
+  "devkitRef": "v0.37.1",
+  "generatedAt": "2026-07-21T12:07:22.124Z",
   "targets": [
     "claude",
     "cursor"
   ],
   "files": {
-    "_devkit/review-roots.mjs": "3dc00e6ba4c3f8b21d05a9b9926057bf0377cb66436cbddbd55dceffa3c599e0",
     "_devkit/review-roots.d.mts": "3c2cdf231cd93a60c1ff0e7ef0eaf17a01f873398917e32fe4e092f947b47bc4",
-    "dup-detection/SKILL.md": "fdc2e37d8a278f82931818807b4d5ee91907e379131be7a07532e79e3822cbda",
-    "parallel-neon-migration/SKILL.md": "b2c2d90ff77fe8bd4eb6e2dda074dd0c8b5adfc1b179ac136689445a4d2c981e",
-    "frontend-accessibility/scripts/checklist.mjs": "8adf873b5f5e40e1f77f82b87b55b5206700b8d2866361f0dd3d881b7851d247",
-    "frontend-accessibility/SKILL.md": "d950e1ddc87ec9a090936f05447d2298b25fb5e12e315345d2d9c02912c513a9",
-    "feature-critique/SKILL.md": "f40fae2d934d851c3ba9847a924c0d43bf4661d640d0a83e9bf3f9121b42429f",
-    "decisions/SKILL.md": "c85736e2b4ff81af0b299c2a7455e7ef6d5c8e81b47200c7f1d65833d5ecef4a",
-    "upstream-sync/scripts/sync.mjs": "218aa7ad7e96f280f281a38f99919f2ce55eace902ed95b1119a0099ddbb3c76",
-    "upstream-sync/scripts/verify-merge.mjs": "afb2977ab8b7a96fac2e93e29a68ae432508ba7e63d03c580fcf03229fcd0676",
-    "upstream-sync/SKILL.md": "1007c8377ac15393dcc9db5384d80bd38487c85aa465921ae0b49ba5a91bad70",
-    "backend-performance/scripts/checklist.mjs": "efba9596aad753a23c45b735f41433ca8368d360ec10fb535bca8cf7dbce07e1",
+    "_devkit/review-roots.mjs": "e7d506cdc755ecb41cd311c27be5ef425ec3b13114fbed6978bd0d661f1f2c11",
+    "api-security/SKILL.md": "5916bc98f288a0b7269ec9df416692509c449274026afc93013388c40c13415e",
+    "api-security/scripts/checklist.mjs": "6707ed9f9deb45d50be430ef1e6bcaa61fd5809db5cb163b5e4fce1eb695aa89",
     "backend-performance/SKILL.md": "5ff0858b34ffc91d677976dd279f27714d934aa5062f0a3f6b38088e7bd10ddb",
-    "testing/SKILL.md": "1816d2bbdb38b1269fd4355b0853bfdf951a84b7ec416c5c271825eb20cc2b64",
+    "backend-performance/scripts/checklist.mjs": "efba9596aad753a23c45b735f41433ca8368d360ec10fb535bca8cf7dbce07e1",
     "brainstorming/SKILL.md": "faa2fd9ea4ac99b0e92934c931701c25a85767935526810baa9c462b632855e0",
-    "structure-governance/references/walls.md": "9986c487a0a2cdf06adad88ea1261657c0c6945179aa4c8b15d7cef086cd540d",
-    "structure-governance/references/fixing-lint-errors.md": "44ad9ba30d747bac4fac27d2d2a8f2ef788b5894db77df70d6c4f3359c7a1fc8",
-    "structure-governance/SKILL.md": "21fb605d1b6fadd81358da7dbe078c634b60115234b427dae4ae47ce8df30f04",
-    "correctness/scripts/checklist.mjs": "ed0ae0d16ac93fbf069fb427e994d96a8fa2c2dd63342f3eec06abaa60651850",
+    "commit-guard/SKILL.md": "69fe86ef83dd22006fec3b1b7990747c69455b6368699c86a579977603e4bdbb",
+    "commit-guard/scripts/checklist.mjs": "49b8397ad2f305be08d4569d354f454aeb166ec5c62467de22f5eac7a4c3f538",
     "correctness/SKILL.md": "16fca786d4b2fa85c2588aee13d14f4cf6a71a30014cbcc24993f526f67770c7",
-    "using-devkit/SKILL.md": "106a7a2388d9c887e83c8a9ac820bcba61649a8a440cc9c090e13af26e17cacb",
-    "frontend-performance/scripts/checklist.mjs": "17eaf8adc34e213ff7b72396f2274560ccfbf8f4570bf327263d9d28a6239b19",
+    "correctness/scripts/checklist.mjs": "ac92581106621ad02a270f14a259fd5854ff32be363191a0ce9bbfd3f283e481",
+    "decisions/SKILL.md": "c85736e2b4ff81af0b299c2a7455e7ef6d5c8e81b47200c7f1d65833d5ecef4a",
+    "dup-detection/SKILL.md": "fdc2e37d8a278f82931818807b4d5ee91907e379131be7a07532e79e3822cbda",
+    "feature-critique/SKILL.md": "f40fae2d934d851c3ba9847a924c0d43bf4661d640d0a83e9bf3f9121b42429f",
+    "frontend-accessibility/SKILL.md": "d950e1ddc87ec9a090936f05447d2298b25fb5e12e315345d2d9c02912c513a9",
+    "frontend-accessibility/scripts/checklist.mjs": "8adf873b5f5e40e1f77f82b87b55b5206700b8d2866361f0dd3d881b7851d247",
     "frontend-performance/SKILL.md": "6697c6583485c5eea8168437c2faee3f81f9051a0c9f85e739278d13c1c6b628",
-    "api-security/scripts/checklist.mjs": "6707ed9f9deb45d50be430ef1e6bcaa61fd5809db5cb163b5e4fce1eb695aa89",
-    "api-security/SKILL.md": "5916bc98f288a0b7269ec9df416692509c449274026afc93013388c40c13415e",
-    "commit-guard/scripts/checklist.mjs": "49b8397ad2f305be08d4569d354f454aeb166ec5c62467de22f5eac7a4c3f538",
-    "commit-guard/SKILL.md": "69fe86ef83dd22006fec3b1b7990747c69455b6368699c86a579977603e4bdbb",
+    "frontend-performance/scripts/checklist.mjs": "17eaf8adc34e213ff7b72396f2274560ccfbf8f4570bf327263d9d28a6239b19",
+    "frontend-security/SKILL.md": "b4f097efd4076ebf530d59b2042def53bb51d975c50e6dc12da698d882fe630f",
     "frontend-security/scripts/checklist.mjs": "86926016f1db574bdc3fca5372ccd343c822e309f03a96160d09c0469409638d",
-    "frontend-security/SKILL.md": "b4f097efd4076ebf530d59b2042def53bb51d975c50e6dc12da698d882fe630f"
+    "parallel-neon-migration/SKILL.md": "b2c2d90ff77fe8bd4eb6e2dda074dd0c8b5adfc1b179ac136689445a4d2c981e",
+    "structure-governance/SKILL.md": "21fb605d1b6fadd81358da7dbe078c634b60115234b427dae4ae47ce8df30f04",
+    "structure-governance/references/fixing-lint-errors.md": "44ad9ba30d747bac4fac27d2d2a8f2ef788b5894db77df70d6c4f3359c7a1fc8",
+    "structure-governance/references/walls.md": "9986c487a0a2cdf06adad88ea1261657c0c6945179aa4c8b15d7cef086cd540d",
+    "testing/SKILL.md": "1816d2bbdb38b1269fd4355b0853bfdf951a84b7ec416c5c271825eb20cc2b64",
+    "upstream-sync/SKILL.md": "1007c8377ac15393dcc9db5384d80bd38487c85aa465921ae0b49ba5a91bad70",
+    "upstream-sync/scripts/sync.mjs": "218aa7ad7e96f280f281a38f99919f2ce55eace902ed95b1119a0099ddbb3c76",
+    "upstream-sync/scripts/verify-merge.mjs": "afb2977ab8b7a96fac2e93e29a68ae432508ba7e63d03c580fcf03229fcd0676",
+    "using-devkit/SKILL.md": "65a1e81491f3843e445376a38dc7cfb4e4dacbe151efd0547f03e97d17500e04"
   }
 }

gate-engine/review/__tests__/review-roots.test.mts

@@ -0,0 +1,184 @@
+/**
+ * skills/_devkit/review-roots.mjs — the shared root-validation module every reviewer checklist
+ * imports. It is the single choke point that stops an absolute path, a `..` traversal, or a
+ * pathspec-magic string from reaching a `git diff -- <pathspec>` call, and the single place that
+ * decides when a reviewer falls back to scanning everything.
+ *
+ * The tests live HERE and not beside the module on purpose: `skills/**` is excluded from the vitest
+ * include globs (repo-coupled helper scripts must not redden devkit's run), and everything under
+ * skills/_devkit/ is PROJECTED into consumer repos by `devkit sync-skills` — a test file there would
+ * ship into every consumer's .claude/.cursor tree as dead weight their runner might pick up.
+ */
+import { describe, expect, it } from 'vitest';
+import {
+  isNonEmptyStringArray,
+  normalizeReviewRoots,
+  parseInjectedReviewRoots,
+  toGitPathspecs,
+} from '../../../skills/_devkit/review-roots.mjs';
+
+// Save exactly the keys THIS call mutates, derived from the argument — not a hardcoded list. With a
+// fixed list, the first test to pass a key outside it (DEVKIT_REVIEW_FRONTEND_ROOTS, say) would set
+// that var and never restore it, leaking into every later test in the file. Deriving the set makes
+// the helper correct by construction for any key a future test reaches for.
+const withEnv = <T,>(env: Record<string, string | undefined>, fn: () => T): T => {
+  const saved = Object.fromEntries(Object.keys(env).map((k) => [k, process.env[k]]));
+  try {
+    for (const [k, v] of Object.entries(env)) {
+      if (v === undefined) delete process.env[k];
+      else process.env[k] = v;
+    }
+    return fn();
+  } finally {
+    for (const [k, v] of Object.entries(saved)) {
+      if (v === undefined) delete process.env[k];
+      else process.env[k] = v;
+    }
+  }
+};
+
+describe('isNonEmptyStringArray', () => {
+  // The regression this module was fixed for: `[].every()` is vacuously true, so an empty array
+  // passed a predicate whose NAME promises otherwise. Downstream that made the correctness
+  // checklist match zero files and pass having reviewed nothing.
+  it('REJECTS an empty array (a gate must never verify nothing)', () => {
+    expect(isNonEmptyStringArray([])).toBe(false);
+  });
+
+  it('accepts a populated array of non-empty strings', () => {
+    expect(isNonEmptyStringArray(['ts', 'tsx'])).toBe(true);
+  });
+
+  it.each([
+    ['an empty string entry', ['ts', '']],
+    ['a non-string entry', ['ts', 42]],
+    ['a nested array', [['ts']]],
+    ['null', null],
+    ['undefined', undefined],
+    ['a string, not an array', 'ts'],
+    ['an object', { 0: 'ts' }],
+  ])('rejects %s', (_label, value) => {
+    expect(isNonEmptyStringArray(value)).toBe(false);
+  });
+});
+
+describe('normalizeReviewRoots — path containment', () => {
+  it.each([
+    ['an absolute posix path', ['/etc/passwd']],
+    ['an absolute windows path', ['C:\\Windows']],
+    ['a parent traversal', ['../secrets']],
+    ['a traversal mid-path', ['src/../../etc']],
+    ['a backslash traversal', ['src\\..\\..\\etc']],
+    ['a null byte', ['src\0evil']],
+    ['a pathspec-magic root', [':(exclude)src']],
+    ['an empty string', ['']],
+    ['a whitespace-only string', ['   ']],
+    ['a non-string entry', [42]],
+    ['an empty list', []],
+    ['a non-array', 'src'],
+  ])('THROWS on %s', (_label, value) => {
+    expect(() => normalizeReviewRoots(value, 'scanRoots')).toThrow(/scanRoots/);
+  });
+
+  it('normalizes and de-duplicates equivalent spellings', () => {
+    expect(normalizeReviewRoots(['./src', 'src/', 'src'], 'scanRoots')).toEqual(['src']);
+  });
+
+  it('collapses a bare "." to the scan-all sentinel', () => {
+    expect(normalizeReviewRoots(['.'], 'scanRoots')).toEqual(['.']);
+  });
+
+  it('converts windows separators to posix', () => {
+    expect(normalizeReviewRoots(['src\\main\\lib'], 'scanRoots')).toEqual(['src/main/lib']);
+  });
+
+  it('trims surrounding whitespace', () => {
+    expect(normalizeReviewRoots(['  src/main  '], 'scanRoots')).toEqual(['src/main']);
+  });
+
+  it('names the offending key in the error, so a consumer can find it', () => {
+    expect(() => normalizeReviewRoots(['/abs'], 'review.backendRoots')).toThrow(
+      /review\.backendRoots/,
+    );
+  });
+});
+
+describe('toGitPathspecs', () => {
+  // Without :(top,literal) a root containing glob or magic characters would be reinterpreted by git.
+  it('forces every real root to a top-anchored LITERAL pathspec', () => {
+    expect(toGitPathspecs(['src', 'socket-server'])).toEqual([
+      ':(top,literal)src',
+      ':(top,literal)socket-server',
+    ]);
+  });
+
+  it('leaves "." bare — it is the scan-all sentinel, not a path', () => {
+    expect(toGitPathspecs(['.'])).toEqual(['.']);
+  });
+
+  it('handles a mixed list', () => {
+    expect(toGitPathspecs(['.', 'src'])).toEqual(['.', ':(top,literal)src']);
+  });
+});
+
+describe('parseInjectedReviewRoots', () => {
+  it('returns null outside review mode — a stray env var cannot re-scope a normal commit', () => {
+    expect(
+      withEnv({ DEVKIT_RUN_MODE: undefined, DEVKIT_REVIEW_BACKEND_ROOTS: '["src"]' }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toBeNull();
+  });
+
+  it('returns null when the var is unset in review mode', () => {
+    expect(
+      withEnv({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_BACKEND_ROOTS: undefined }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toBeNull();
+  });
+
+  it('parses and normalizes an injected list in review mode', () => {
+    expect(
+      withEnv({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_BACKEND_ROOTS: '["./src","src/"]' }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toEqual(['src']);
+  });
+
+  it('THROWS on unparseable JSON rather than silently scanning everything', () => {
+    expect(() =>
+      withEnv({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_BACKEND_ROOTS: '{not json' }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toThrow(/must be a JSON string array/);
+  });
+
+  it('THROWS on an injected traversal — the env is trusted-ish, not unchecked', () => {
+    expect(() =>
+      withEnv({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_BACKEND_ROOTS: '["../../etc"]' }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toThrow(/DEVKIT_REVIEW_BACKEND_ROOTS/);
+  });
+
+  it('THROWS on an injected empty array', () => {
+    expect(() =>
+      withEnv({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_BACKEND_ROOTS: '[]' }, () =>
+        parseInjectedReviewRoots('DEVKIT_REVIEW_BACKEND_ROOTS'),
+      ),
+    ).toThrow(/DEVKIT_REVIEW_BACKEND_ROOTS/);
+  });
+});
+
+describe('end-to-end: config value → git pathspec', () => {
+  it('a hostile root never reaches git', () => {
+    expect(() => toGitPathspecs(normalizeReviewRoots(['../../etc'], 'scanRoots'))).toThrow();
+  });
+
+  it('a legitimate topology survives intact', () => {
+    expect(
+      toGitPathspecs(normalizeReviewRoots(['src/main', './socket-server'], 'scanRoots')),
+    ).toEqual([':(top,literal)src/main', ':(top,literal)socket-server']);
+  });
+});

skills/_devkit/review-roots.mjs

@@ -1,8 +1,15 @@
 import { readFileSync } from 'node:fs';
 import { isAbsolute, win32 } from 'node:path';
 
+// `length > 0` is load-bearing, not belt-and-braces: `[].every(...)` is vacuously TRUE, so without it
+// this predicate accepts `[]` — contradicting its own name. The correctness checklist then takes
+// `sourceExtensions: []` as valid, `exts.some(...)` is false for every path, and the reviewer passes
+// having examined ZERO files. A gate that silently verifies nothing is the one failure mode devkit
+// refuses to ship; an empty list must fall through to the caller's default.
 export const isNonEmptyStringArray = (value) =>
-  Array.isArray(value) && value.every((entry) => typeof entry === 'string' && entry.length > 0);
+  Array.isArray(value) &&
+  value.length > 0 &&
+  value.every((entry) => typeof entry === 'string' && entry.length > 0);
 
 /** Normalize trusted repository-relative roots so selector and Git pathspec readers agree. */
 export function normalizeReviewRoots(value, name) {