fix: pre-landing review fixes (GST-942, pass 1)

- an imported rm( counts like fs.rm(;
- a recursive delete a formatter split across lines raises security,
  read over the file's changed lines joined (SECURITY_JOINED).

Repin lib/review-risk.ts under no-loss replay GST-942 (no tier change
over 100 main commits).

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/gst-942-rm-recursive.md

@@ -1,3 +1,3 @@
 ### Fixed
 
-- **A recursive delete keeps the security class however its arguments are written (GST-942).** `fs.rmSync(path.join(dir, "x"), { recursive: true })`, a `)` inside a comment before `recursive: true`, and the async `fs.rm`/`fs.promises.rm` all slipped past the rule, because it stopped reading at the first `)`. The review depth could then drop to light on real recursive-delete code. The rule now reads up to 500 characters of the line after `rmSync(` or `.rm(`, still in linear time.
+- **A recursive delete keeps the security class however its arguments are written (GST-942).** `fs.rmSync(path.join(dir, "x"), { recursive: true })`, a `)` inside a comment before `recursive: true`, `fs.rm`/`fs.promises.rm`/an imported `rm`, and a call a formatter split across lines all slipped past the rule, because it stopped reading at the first `)` and read one line at a time. The review depth could then drop to light on real recursive-delete code. The rule now reads up to 500 characters after `rmSync(` or `rm(`, across the file's changed lines, still in linear time.

docs/review-policy.md

@@ -228,7 +228,7 @@ line that can run code (a script or `foreignObject` element, an event handler, a
 rules are coarse on purpose and parse no import clause or call arguments;
 `test/review-risk-linear.test.ts` holds each of them under 100 ms on
 200,000-character hostile lines, its anchor followed by a long run of the
-characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` or `.rm(` call counts as security when `recursive: true` follows within 500 characters on the same line, whatever the arguments hold (a `path.join(...)` first argument or a `)` in a comment included, GST-942), and on its own when more than 500 characters come before the first `)` after it. **data** is user data: a path naming pii, gdpr, ccpa, privacy,
+characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` or `rm(` call (`fs.rm`, `fs.promises.rm` or an imported `rm`) counts as security when `recursive: true` follows within 500 characters, whatever the arguments hold (a `path.join(...)` first argument or a `)` in a comment included) and also when a formatter split the call across lines, which is read over the file's changed lines joined (GST-942); a call with more than 500 characters before the first `)` after it counts on its own. **data** is user data: a path naming pii, gdpr, ccpa, privacy,
 personal, account, customer, cookie, persistence, storage or database, or a line
 that drops a table or column, deletes rows or bulk-deletes records. Auth scope,
 migrations and deleted code paths were already gate class. `RISK_RULES` hashes

lib/review-risk.ts

@@ -55,9 +55,11 @@ export const SECURITY_LINE = [
   /\b(?:includes|startsWith|endsWith|indexOf)\s*\(\s*['"`]\.\.[/\\]{0,2}['"`]|[!=]==?\s*['"`]\.\.['"`]|['"`]\.\.['"`]\s*[!=]==?|\\\.\\\./,
   /(?<![.\w])eval\s*\(/, /(?:^|[;&|])\s*eval\s+["'$]/, /\bnew\s+Function\s*\(/,
   /\b(?:spawn|spawnSync|execFile|execFileSync)\s*\(\s*['"](?:\/bin\/)?(?:ba|z)?sh['"]/, /\bBun\.spawn(?:Sync)?\s*\(\s*\[\s*['"](?:\/bin\/)?(?:ba|z)?sh['"]/,
-  /\brmSync\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\.rm\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\brm\s+-(?=[a-z]*r)(?=[a-z]*f)[a-z]*[rf]\b/,
+  /\brmSync\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\brm\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\brm\s+-(?=[a-z]*r)(?=[a-z]*f)[a-z]*[rf]\b/,
   /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/,
 ];
+/** A recursive delete split across lines (a formatter puts `recursive: true` on its own line), read over a file's changed lines joined. */
+export const SECURITY_JOINED = [/\brm(?:Sync)?\s*\([\s\S]{0,500}?recursive\s*:\s*true/];
 export const SVG_PATH = /\.svg$/i;
 /** SVG text that can run code, read decoded and again with whitespace removed (java&#9;script: is javascript:); a file's changed lines are read joined, so a split attribute still counts. */
 export const SVG_ACTIVE = [/<(?:[\w.-]+:)?(?:script|foreignObject)\b/i, /(?:^|[\s"'/])on[a-z]+\s*=/i, /(?:java|vb)script:|data:text\/html/i];
@@ -81,7 +83,7 @@ const namesMoney = (t: readonly string[], words: readonly string[]) => t.some(x
 export const RISK_RULES = createHash('sha256').update(JSON.stringify({
   RISK_CLASSES, MONEY_PATH_TOKENS, AUTH_PATH_TOKENS, AUTH_PATH_PREFIXES, KEY_NAME, SECURITY_PATH_TOKENS, DATA_PATH_TOKENS,
   MONEY_LINE_TOKENS, MONEY_PAIRS,
-  patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, DATA_LINE, SVG_PATH: [SVG_PATH], SVG_ACTIVE }).map(([k, v]) => [k, v.map(r => '/' + r.source + '/' + r.flags)]),
+  patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, SECURITY_JOINED, DATA_LINE, SVG_PATH: [SVG_PATH], SVG_ACTIVE }).map(([k, v]) => [k, v.map(r => '/' + r.source + '/' + r.flags)]),
 })).digest('hex').slice(0, 16);
 
 export const tokens = (p: string) => p.replace(/([a-z0-9])([A-Z])/g, '$1 $2').toLowerCase().split(/[^a-z0-9]+/).filter(Boolean);
@@ -113,6 +115,7 @@ export function matchRisk(i: RiskInput): Map<RiskClass, string> {
       if (!why.has('security') && SECURITY_LINE.some(r => r.test(line))) hit('security', 'security line: ' + p);
       if (!why.has('data') && DATA_LINE.some(r => r.test(line))) hit('data', 'data line: ' + p);
     }
+    if (!why.has('security') && [i.added?.[p], i.removed?.[p]].some(l => l && SECURITY_JOINED.some(r => r.test(l.join('\n'))))) hit('security', 'security lines: ' + p);
   }
   return new Map(RISK_CLASSES.filter(c => why.has(c)).map(c => [c, why.get(c)!]));
 }

test/review-packets-removed.test.ts

@@ -57,7 +57,7 @@ const NO_LOSS_REPLAYS = {
   'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)",
   'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged",
   'GST-850': "GST-850 dispatch-superset no-loss check: on the 26 GSTA-167 no-loss units the branch plan (wider security and money classes, write-up text in the batched diff) drops no reviewer role main dispatches, so no saved blocker can be lost, and over the last 100 main commits no commit with a gate or risk class drops depth (test/review-risk-routing.test.ts, test/review-risk-linear.test.ts); reviewer text is unchanged",
-  'GST-942': "GST-942 dispatch-superset no-loss check: the rmSync rule no longer stops at the first ) and a matching .rm( rule is added, so a recursive delete raises security on more lines and never on fewer; over the last 100 first-parent main commits to 3d99c2249 (62,531 changed lines) the rules match all 170 lines the old rule matched plus 19 more, and no commit changes depth, class or risk class; reviewer text is unchanged",
+  'GST-942': "GST-942 dispatch-superset no-loss check: the rmSync rule no longer stops at the first ), a matching rm( rule is added, and both read a file's changed lines joined for a delete split across lines, so a recursive delete raises security on more diffs and never on fewer; over the last 100 first-parent main commits to 3d99c2249 (62,531 changed lines) the line rules match all 170 lines the old rule matched plus 19 more, and no commit changes depth, class or risk class; reviewer text is unchanged",
   'GST-911-redos': "GST-911 ReDoS fix verdict-identity no-loss check (wave review-e7b551ebd865): SVG_REFUSED's url() entry, rewritten to run in linear time, refuses exactly the strings #3838's pattern refused (test/review-risk-redos.test.ts: 60,000 seeded strings and the hand-picked edges, and every SVG blob in the repository's history, with zero differences); SVG_MARKUP now refuses a DOCTYPE naming any document but W3C's SVG DTD (test/review-planner-misreads.test.ts), which only keeps more SVGs code and so drops no reviewer; GST-911's no-loss replay holds unchanged; reviewer text is unchanged",
   'GST-911': "GST-911 no-loss replay (GSTA-249/13): the planner over the last 100 first-parent main commits to f5cb6ec1e, origin/main's code against this branch, gate scan fixed at 0, changes no commit's depth, class, risk classes, targeted classes or SCOPE_AUTH; the three misreads (a drawing-only SVG, a session-named data file, a bare base) are proven on fixtures (test/review-planner-misreads.test.ts), where every SVG that can run script stays code with the security class; reviewer text is unchanged",
   'R51': "R51 risk-tier routing no-loss re-score (GSTA-167): the saved panel findings (349 records over 24 of the 26 no-loss units) re-scored against the branch's plan lose no blocker whose only finders are roles it drops, and every gate, security, user-data and money fixture keeps main's reviewers plus the full panel (test/review-targeted.test.ts, test/review-risk-routing.test.ts); reviewer text is unchanged",
@@ -84,7 +84,7 @@ const REVIEWER_BRIEFS: Record<string, { sha: string; replay: Replay }> = {
     'review/specialists/targeted.md': { sha: '7f92a0d0a8319c3c0e076fb3b1a4689c45120b2be2c14dfc7d4cb03aafa9d6a5', replay: 'GST-257' },
     'scripts/resolvers/fix-check.ts': { sha: '712bf5b0c439e96bac32f19cbe25394dabb4c94aa66600100bfde9b100e15a08', replay: '#3638' },
     'lib/review-depth.ts': { sha: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' },
-    'lib/review-risk.ts': { sha: '262ae8a14cda820c3ea0e508045229c2e36993c0fdfd3a4d51a8834a915de94f', replay: 'GST-942' },
+    'lib/review-risk.ts': { sha: 'd8845fdb7b4cffe73d400b367c1fdf7df03ca155f2e579ed5c9833d4e9d09094', replay: 'GST-942' },
 };
 
 describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => {

test/review-risk-redos.test.ts

@@ -5,7 +5,7 @@
  */
 import { describe, expect, test } from 'bun:test';
 import { SVG_REFUSED, svgDrawing } from '../lib/review-depth';
-import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_LINE, matchRisk } from '../lib/review-risk';
+import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_JOINED, SECURITY_LINE, matchRisk } from '../lib/review-risk';
 
 const L = 100_000;
 const ms = (f: () => unknown) => { const t = performance.now(); f(); return performance.now() - t; };
@@ -18,7 +18,7 @@ describe('linear time on hostile input', () => {
   //   or on a line of repeated anchors; seam=every pattern in SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE and SECRET_PATH
   const hostile = [
     run('rm -', 'r'), run('rm -', 'f'), run('rm -r', 'r'), run('rm -f', 'a'), run('rm\t-', 'r'), run('rm ', ' '), run('rm -', 'r', '1'), run('rm -', 'rf', '_'), run('', 'rm -r '),
-    run('', 'rmSync('), run('rmSync(', ' '), run('rmSync(', 'recursive '), run('rmSync(', 'recursive: '), run('', '.rm('), run('.rm(', ' '), run('.rm(', 'recursive: '),
+    run('', 'rmSync('), run('rmSync(', ' '), run('rmSync(', 'recursive '), run('rmSync(', 'recursive: '), run('', '.rm('), run('.rm(', ' '), run('.rm(', 'recursive: '), run('', 'rm('), run('rm(', ' '), run('rmSync(', ' \n'),
     run('shell', ' '), run('shell:', ' '), run('shell=', ' '), run('', '; '), run(';', ' '), run('; eval', ' '), run('eval', ' '), run('', ';eval '),
     run('execSync', ' '), run('child_process.exec', ' '), run('os.system', ' '), run('new', ' '), run('new Function', ' '),
     run('spawn(', ' '), run('Bun.spawn(', ' '), run('Bun.spawn([', ' '), run('innerHTML', ' '),
@@ -27,7 +27,7 @@ describe('linear time on hostile input', () => {
     run('', 'a_'), run('', 'aU'), run('a', 'A'), run('spend', '_'), run('new Anthropic', ' '), run('.messages.create', ' '),
     run('$', 'A_'), run('${', 'A_'), run('', '$A_'), run('process.env.', 'A_'), run('process.env[', ' '), run('os.environ[', ' '), run('os.getenv(', ' '), run('ENV[', ' '),
   ];
-  test.each(Object.entries({ SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE, SECRET_PATH: [SECRET_PATH] }))('every %s pattern finishes under 100 ms on each 100k-character hostile line', (_n, patterns) => {
+  test.each(Object.entries({ SECURITY_LINE, SECURITY_JOINED, DATA_LINE, SECRET_LINE, MONEY_LINE, SECRET_PATH: [SECRET_PATH] }))('every %s pattern finishes under 100 ms on each 100k-character hostile line', (_n, patterns) => {
     const slow = patterns.flatMap(r => hostile.map(([label, s]) => [r.source.slice(0, 50), label, Math.round(ms(() => r.test(s)))] as const)).filter(([, , t]) => t >= 100);
     expect(slow).toEqual([]);
   }, 120_000);
@@ -76,9 +76,18 @@ describe('the rm rule matches what R51 matched', () => {
     //   ) inside the first argument or a comment, or skips the async fs.rm; seam=matchRisk on one added line
     const recursive = ['fs.rmSync(path.join(dir, "x"), { recursive: true, force: true });', 'fs.rmSync(dir, /* ) */ { recursive: true });',
       'fs.rmSync(path.resolve(root, name), { recursive: true });', 'await fs.promises.rm(path.join(dir, "x"), { recursive: true });',
-      'await fs.rm(dir, { recursive: true })'];
+      'await fs.rm(dir, { recursive: true })', "await rm(dir, { recursive: true, force: true }); // import { rm } from 'node:fs/promises'",
+      'fs.rm(' + 'a'.repeat(600) + ', { force: true })'];
     expect(recursive.filter(l => !security(l))).toEqual([]);
-    expect(['fs.rm(path.join(dir, "x"), { force: true });', 'const rm = (a) => a; rm(x, { recursive: true });', 'farm(x, { recursive: true });'].filter(security)).toEqual([]);
+    expect(['fs.rm(path.join(dir, "x"), { force: true });', 'farm(x, { recursive: true });', 'confirm(x, { recursive: true });'].filter(security)).toEqual([]);
+  });
+  test('a recursive delete a formatter split across lines raises security, read over the joined lines (GST-942 review)', () => {
+    // Value: protects=a Prettier-formatted rmSync or fs.promises.rm keeps the security class; fails_when=the delete rules read
+    //   only one line at a time; seam=matchRisk over a file's added lines
+    const split = (lines: string[]) => matchRisk({ codePaths: ['src/clean.ts'], added: { 'src/clean.ts': lines }, removed: {}, scopeAuth: false }).has('security');
+    expect(split(['  fs.rmSync(path.join(root, "build-output-cache"), {', '    recursive: true,', '    force: true,', '  });'])).toBe(true);
+    expect(split(['  await fs.promises.rm(', '    path.join(root, "cache"),', '    { recursive: true },', '  );'])).toBe(true);
+    expect(split(['  fs.rmSync(path.join(root, "file"), {', '    force: true,', '  });'])).toBe(false);
   });
 });