diff --git a/changelog.d/gst-961-money-paths.md b/changelog.d/gst-961-money-paths.md new file mode 100644 index 000000000..b594119c2 --- /dev/null +++ b/changelog.d/gst-961-money-paths.md @@ -0,0 +1,8 @@ +### Fixed + +- **A docs or data PR no longer gets a money reader because a file name says "cost", and reading a `cost_usd` field no longer counts as money code (GST-961, GST-962).** On gstack-evals#138, a results write-up planned as docs at light depth still dispatched a second, targeted money reader, because `wiki/.../cost-gstack-full.tsv` has "cost" in its name. On gstack-evals#139, an analysis module that only sums and prints per-trial `cost_usd` raised the money class. A data file that no code reads now never counts as money, by its name or its rows, and a `_usd` or `Usd` name counts only when it says cap, limit, max, budget, spend, price, fee, bill or payment, or when a `<` or `>` compares it. Spend caps, budgets, billing paths and paid-provider calls still count. Every other class still reads unread data files. Over the last 100 main commits, no commit's depth, class, risk classes or targeted classes changed. + +### Tests + +- `test/review-targeted.test.ts`: #138's file list plans no targeted reader, and the same file read by code still plans money. Neither a `cost_usd` read nor a `cost_usd` report names money. Positive controls: caps, budgets, limits, spend checks, comparisons, `spend_cap`, a billing path, a provider client, and a secret in an unread data file. +- `test/review-risk-redos.test.ts`: `usdMoney` stays under 250 ms on 100,000-character lines of `_usd` names, comparisons and member chains. diff --git a/docs/review-policy.md b/docs/review-policy.md index bc1bee4fc..063abd5cb 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -385,7 +385,7 @@ and matching is token-bounded: `costume` never reads as cost. | Class | Signal | |---|---| -| money | a path segment naming cost, spend, budget, ledger, billing, price, fee or invoice; or a changed line with an identifier ending in `_usd` or `Usd`, any `spend_cap` spelling, or a call to a paid provider `lib/pricing.ts` prices | +| money | a path segment naming cost, spend, budget, ledger, billing, price, fee or invoice; or a changed line with an identifier ending in `_usd` or `Usd` whose name says cap, limit, max, budget, ceiling, allowance, spend, price, fee, bill or payment, or that a `<` or `>` compares (reading or reporting `cost_usd` is not money, GST-962); any `spend_cap` spelling; or a call to a paid provider `lib/pricing.ts` prices. A data file no code reads (the same consumer scan the depth uses) never counts as money by its name or its rows (GST-961); every other class still reads it | | gate | a hunk the gate scan tagged: a relaxed threshold, a skipped test, a loosened check | | auth | `SCOPE_AUTH` from `gstack-diff-scope`, on a code path with an auth, session, jwt, permission or role token in its name; a data file named for sessions (`recovered-sessions.jsonl`) outside a config location does not set `SCOPE_AUTH`, and `SCOPE_AUTH` fails safe: any name token containing auth sets it (`auth.ts`, `/auth/`, `oauth`, `useAuth.ts`, `nextauthmiddleware.ts`) except the authorship words `author`, `authors`, `authored`, `authoring`, `authorship`, `authority`, `authorities`, `coauthor` and `coauthors` | | secret | a secrets, credentials, keychain or token path, or a changed line that reads a key-named environment variable (`OPENAI_API_KEY`, `GH_TOKEN`) or a credential store | diff --git a/lib/review-depth.ts b/lib/review-depth.ts index c337ebd98..0afe9091c 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -245,7 +245,7 @@ export interface ClassResult { class: DiffClass; reasons: string[]; errors: Dept const sanitize = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').slice(0, 200); const isOutput = (i: ClassInput, p: string) => !!i.registry?.outputs.length && matchesAny(i.registry.outputs, p); -const unconsumedData = (i: ClassInput, p: string) => Array.isArray(i.consumed) && !i.consumed.includes(p) && (dataCandidate(p, i.root) || (!!i.drawings?.includes(p) && svgCandidate(p, i.root))); +export const unconsumedData = (i: Pick, p: string) => Array.isArray(i.consumed) && !i.consumed.includes(p) && (dataCandidate(p, i.root) || (!!i.drawings?.includes(p) && svgCandidate(p, i.root))); const pathClass = (i: ClassInput, p: string): PathClass => (i.excluded?.has(p) || (i.stampOnly ?? []).includes(p) || unconsumedData(i, p) ? 'docs' : classifyPath(p, i.root)); /** The diff's class, raised to gate by any gate signal; `errors` lists every input that fails the depth closed; `runtimeLines` counts code-class lines outside the base registry's outputs. */ diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 1f2132522..1633ca192 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -22,12 +22,23 @@ export const MONEY_PATH_TOKENS = ['cost', 'costs', 'spend', 'spending', 'budget' export const MONEY_PAIRS: readonly (readonly [string, readonly string[]])[] = [ ['usage', ['token', 'tokens', 'cost', 'costs', 'spend', 'billing', 'price', 'pricing', 'budget']], ['proxy', ['billing', 'bill']], ['client', ['cost', 'costs', 'charge', 'charges', 'billing', 'price', 'pricing', 'spend']], ]; -/** Identifiers ending in _usd, Usd or cap_usd, any spend_cap spelling, and paid-provider call sites (the providers lib/pricing.ts prices). */ -export const MONEY_LINE = [ - /\b[A-Za-z0-9_]*_usd\b/i, /\b[a-z][A-Za-z0-9]*Usd\b/, /\bspend[_-]?cap\b/i, +/** Identifiers ending in _usd or Usd: money only beside a word in MONEY_USD_WORDS or in a comparison (usdMoney). */ +export const MONEY_USD = [/\b[A-Za-z0-9_]*_usd\b/i, /\b[a-z][A-Za-z0-9]*Usd\b/]; +const MONEY_USD_ALL = MONEY_USD.map(r => new RegExp(r.source, r.flags + 'g')); +/** What makes a _usd identifier money (GST-962: reading or reporting gstack-evals#139's per-trial cost_usd is not money code). */ +export const MONEY_USD_WORDS = ['cap', 'caps', 'limit', 'limits', 'max', 'maximum', 'budget', 'budgets', 'ceiling', 'allowance', 'spend', 'spent', 'spending', 'price', 'prices', 'pricing', 'fee', 'fees', 'bill', 'billing', 'paid', 'payment']; +/** Any spend_cap spelling and paid-provider call sites (the providers lib/pricing.ts prices). */ +export const PAID_MONEY_LINE = [ + /\bspend[_-]?cap\b/i, /\bapi\.(?:anthropic|openai|voyageai)\.com\b/, /\bgenerativelanguage\.googleapis\.com\b/, /\bopenrouter\.ai\b/, /\bnew\s+(?:Anthropic|OpenAI|AnthropicBedrock|AnthropicVertex)\s*\(/, /\.(?:messages|chat\.completions|responses|embeddings)\.create\s*\(/, ]; +export const MONEY_LINE = [...MONEY_USD, ...PAID_MONEY_LINE]; +/** A _usd identifier whose name says cap, budget, limit, spend or price, or that a `<` or `>` compares; never one an arrow reads. */ +export const usdMoney = (line: string) => MONEY_USD_ALL.some(r => [...line.matchAll(r)].some(m => + tokens(m[0]).some(x => MONEY_USD_WORDS.includes(x)) + || /^\s{0,20}[<>]/.test(line.slice(m.index + m[0].length, m.index + m[0].length + 21)) + || /(?]=?\s{0,20}(?:[\w$]{1,100}\.){0,10}$/.test(line.slice(Math.max(0, m.index - 300), m.index)))); export const AUTH_PATH_TOKENS = ['auth', 'authn', 'authz', 'oauth', 'oauth2', 'jwt', 'session', 'sessions', 'permission', 'permissions', 'role', 'roles']; export const AUTH_PATH_PREFIXES = ['authenticat', 'authoriz']; export const SECRET_PATH = /(^|\/)(secrets?|credentials?|keychain|token[s]?)([._/-]|$)/i; @@ -82,7 +93,7 @@ const namesMoney = (t: readonly string[], words: readonly string[]) => t.some(x /** A hash of the patterns that decide the risk classes, not of this file's bytes; lib/review-depth.ts folds it into DEPTH_RULES. */ 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, + MONEY_LINE_TOKENS, MONEY_PAIRS, MONEY_USD_WORDS, 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); @@ -95,6 +106,8 @@ export interface RiskInput { removed?: Record; /** gstack-diff-scope's SCOPE_AUTH: the auth class needs it and an auth token in the path. */ scopeAuth: boolean; + /** Code paths no code reads (an unconsumed data file or drawing): nothing runs them, so neither their name nor their rows name money (GST-961). */ + dataPaths?: readonly string[]; } /** The risk classes the code paths touch, the first reason per class, in RISK_CLASSES order. Nothing fails closed here: input errors are the caller's. */ @@ -105,12 +118,13 @@ export function matchRisk(i: RiskInput): Map { const t = tokens(p); if (i.scopeAuth && t.some(x => AUTH_PATH_TOKENS.includes(x) || AUTH_PATH_PREFIXES.some(a => x.startsWith(a)))) hit('auth', 'auth path: ' + p); if (SECRET_PATH.test(p)) hit('secret', 'secret path: ' + p); - if (namesMoney(t, MONEY_PATH_TOKENS)) hit('money', 'money path: ' + p); + const money = !i.dataPaths?.includes(p); + if (money && namesMoney(t, MONEY_PATH_TOKENS)) hit('money', 'money path: ' + p); if (t.some(x => SECURITY_PATH_TOKENS.includes(x))) hit('security', 'security path: ' + p); if (t.some(x => DATA_PATH_TOKENS.includes(x))) hit('data', 'data path: ' + p); if (SVG_PATH.test(p) && [i.added?.[p], i.removed?.[p]].some(l => l && svgActive(l.join('\n')))) hit('security', 'active SVG lines: ' + p); for (const line of [...(i.added?.[p] ?? []), ...(i.removed?.[p] ?? [])]) { - if (!why.has('money') && (MONEY_LINE.some(r => r.test(line)) || namesMoney(tokens(line), MONEY_LINE_TOKENS))) hit('money', 'money line: ' + p); + if (money && !why.has('money') && (PAID_MONEY_LINE.some(r => r.test(line)) || usdMoney(line) || namesMoney(tokens(line), MONEY_LINE_TOKENS))) hit('money', 'money line: ' + p); if (!why.has('secret') && SECRET_LINE.some(r => r.test(line))) hit('secret', 'secret line: ' + p); 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); diff --git a/lib/review-targeted.ts b/lib/review-targeted.ts index 7d71db88a..6f540a277 100644 --- a/lib/review-targeted.ts +++ b/lib/review-targeted.ts @@ -20,8 +20,8 @@ import { spawnSync } from 'node:child_process'; import { createHash } from 'node:crypto'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { classifyPath } from './review-depth'; -import { AUTH_PATH_PREFIXES, AUTH_PATH_TOKENS, KEY_NAME, MONEY_LINE, MONEY_LINE_TOKENS, MONEY_PAIRS, MONEY_PATH_TOKENS, SECRET_LINE, SECRET_PATH, matchRisk } from './review-risk'; +import { classifyPath, unconsumedData, type ClassInput } from './review-depth'; +import { AUTH_PATH_PREFIXES, AUTH_PATH_TOKENS, KEY_NAME, MONEY_LINE, MONEY_LINE_TOKENS, MONEY_PAIRS, MONEY_PATH_TOKENS, MONEY_USD_WORDS, SECRET_LINE, SECRET_PATH, matchRisk } from './review-risk'; export const TARGETED_CLASSES = ['money', 'gate', 'auth', 'secret'] as const; export type TargetedClass = (typeof TARGETED_CLASSES)[number]; @@ -31,7 +31,7 @@ export const secretLine = (line: string) => SECRET_LINE.some(r => r.test(line)); /** A hash of the patterns that decide the classes, not of this file's bytes: a comment edit keeps it, a pattern edit changes it. */ export const TARGETED_RULES = createHash('sha256').update(JSON.stringify({ - TARGETED_CLASSES, MONEY_PATH_TOKENS, MONEY_LINE_TOKENS, MONEY_PAIRS, AUTH_PATH_TOKENS, AUTH_PATH_PREFIXES, KEY_NAME, + TARGETED_CLASSES, MONEY_PATH_TOKENS, MONEY_LINE_TOKENS, MONEY_PAIRS, MONEY_USD_WORDS, AUTH_PATH_TOKENS, AUTH_PATH_PREFIXES, KEY_NAME, patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); @@ -47,6 +47,9 @@ export interface TargetedInput { root?: string | readonly string[]; /** Collector failures (a diff git could not list). */ errors?: readonly { code: string; input: string }[]; + /** The collector's consumer scan and drawings (ClassInput): a data file no code reads names no money. */ + consumed?: ClassInput['consumed']; + drawings?: ClassInput['drawings']; } export interface TargetedResult { classes: TargetedClass[]; reasons: string[]; errors: string[]; rules: string } @@ -61,7 +64,7 @@ export function targetedClasses(i: TargetedInput): TargetedResult { if (errors.length) return { classes: [...TARGETED_CLASSES], reasons: errors.map(e => `fail-closed: ${e}`), errors, rules: TARGETED_RULES }; const why = new Map(); if ((i.gateTagged ?? 0) > 0) why.set('gate', `gate scan tagged ${i.gateTagged} hunk(s)`); - for (const [c, reason] of matchRisk({ codePaths: code, added: i.added, removed: i.removed, scopeAuth: s!.SCOPE_AUTH === 'true' })) if ((TARGETED_CLASSES as readonly string[]).includes(c)) why.set(c as TargetedClass, reason); + for (const [c, reason] of matchRisk({ codePaths: code, dataPaths: code.filter(p => unconsumedData(i, p)), added: i.added, removed: i.removed, scopeAuth: s!.SCOPE_AUTH === 'true' })) if ((TARGETED_CLASSES as readonly string[]).includes(c)) why.set(c as TargetedClass, reason); const classes = TARGETED_CLASSES.filter(c => why.has(c)); return { classes, reasons: classes.map(c => why.get(c)!), errors: [], rules: TARGETED_RULES }; } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index c1cf4fc10..5c7c8201f 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -74,6 +74,7 @@ const NO_LOSS_REPLAYS = { 'GST-967': "GST-967 dispatch-superset no-loss check (test/review-depth.test.ts, 'GST-967 dispatch-superset no-loss check'): over 1,440 planReview cases the branch plans exactly what main planned at 1022f4f except the 71 gate-class stops before Step 11, which add only Step 11's adversarial, outside and targeted voices, so no reviewer main dispatches is dropped or re-efforted; the brief bodies lib/review-brief.ts cuts from the ship sections hash to main's; only the lead's repeat-decision and Step 11 instructions changed; reviewer text is unchanged", 'GST-508+GST-464+GST-967': "wave review-53cfb5bfe011 merge of main 68410828f into #3852: ship/sections/review-army.md and adversarial.md are main's GST-967 bytes plus exactly the GST-508+GST-464 lines (the merged file minus main equals the wave minus fc292e22f, line for line), so GST-967's dispatch-superset no-loss check and GST-508's no-loss replay both hold; merged-head checks in GStack Minder drive handoffs/GSTA-202/merge-6841-checks.md", 'GST-508+R51+GST-911+GST-967': "wave review-53cfb5bfe011 merge of main 68410828f into #3852: lib/review-depth.ts is main's GST-967 bytes plus exactly the GST-508+R51+GST-911 lines (the merged file minus main equals the wave minus fc292e22f, line for line); GST-967's dispatch-superset no-loss check (test/review-depth.test.ts) passes on the merged file, and the GST-508+R51+GST-911 checks are rerun at the merged head (GStack Minder drive handoffs/GSTA-202/merge-6841-checks.md)", + 'GST-961+962': "GST-961+962 verdict no-loss replay: a data file no code reads (the depth's consumer scan) names no money by its path or rows, and a _usd or Usd name is money only when it says cap, limit, max, budget, spend, price, fee, bill or payment or a < or > compares it; over the last 100 first-parent main commits to bf2bcf14b, main's code against this branch, gate scan fixed at 0, no commit changes depth, class, risk classes or targeted classes (~/.capy/work/gst898/replay961.ts on GSTA-249's machine); the 76 changed lines that no longer read as money lines are cost reads and reports (paid-report, eval-plan projections, autoplan estimates), tests, docs and the old pattern source; lib/review-depth.ts only exports unconsumedData; reviewer text is unchanged", 'GST-940': "GST-940 verdict no-loss check: svgDrawing reads a style element's opening tag as quoted pairs with ASCII whitespace, walks its body over the raw text, refuses any & in it, reads comments as CSS and refuses a non-ASCII element or attribute name; on 62,805 unique SVG files (21,658 with a style element) it gives the same verdict as main at 4bbad2422 on every file, and the planner over the last 100 first-parent main commits to 4bbad2422 changes no commit's depth, class or risk classes; the only verdicts it changes are the GST-940 cases in test/review-planner-misreads.test.ts; rerun on the merge with main at 8f08cb76a (GST-967 merged), with the same results; rerun on the merge with main at 5f705f367 (#3852 and #3864 merged), with the same results", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; @@ -96,8 +97,8 @@ const REVIEWER_BRIEFS: Record = { 'review/specialists/testing.md': { sha: 'd6cc2bef7be562387d0c0a22fefe646dff9c295f1d34d8453a6cb496584150e4', replay: '#3638' }, 'review/specialists/targeted.md': { sha: '7f92a0d0a8319c3c0e076fb3b1a4689c45120b2be2c14dfc7d4cb03aafa9d6a5', replay: 'GST-257' }, 'scripts/resolvers/fix-check.ts': { sha: '712bf5b0c439e96bac32f19cbe25394dabb4c94aa66600100bfde9b100e15a08', replay: '#3638' }, - 'lib/review-depth.ts': { sha: '25050a7a61f81df649d1df0d0237dbf961671356395317c6a527741f5bb56877', replay: 'GST-940' }, - 'lib/review-risk.ts': { sha: '08464b78a723ee463ca3a28d0abbc4eb8f4f512192061c703743707b66b5a311', replay: 'GST-942' }, + 'lib/review-depth.ts': { sha: '894ef79782232ae8e2b13a25d57d87264125ff6ae5aeb70813c4b11c8cd59a4d', replay: 'GST-961+962' }, + 'lib/review-risk.ts': { sha: 'ac023de9073705a31f864bc9dc9a58262c325185375dff9049c917fbe8d2be76', replay: 'GST-961+962' }, }; /** diff --git a/test/review-risk-redos.test.ts b/test/review-risk-redos.test.ts index 4b9982fcb..dc606b92e 100644 --- a/test/review-risk-redos.test.ts +++ b/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_JOINED, SECURITY_LINE, matchRisk } from '../lib/review-risk'; +import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_JOINED, SECURITY_LINE, matchRisk, usdMoney } from '../lib/review-risk'; const L = 100_000; const ms = (f: () => unknown) => { const t = performance.now(); f(); return performance.now() - t; }; @@ -37,6 +37,11 @@ describe('linear time on hostile input', () => { expect(ms(() => { hit = security(line); })).toBeLessThan(250); expect(hit).toBe(false); }, 60_000); + test('usdMoney (GST-962) returns in under 250 ms on each 100k-character line of _usd names, comparisons and member chains', () => { + const lines = [run('', 'x_usd '), run('', 'aUsd '), run('', '> a.x_usd '), run('', '=> t.cost_usd '), run('>', ' '), run('> ', 'a.', 'x_usd'), run('', 'a_', 'usd'), run('', '<'), run('', 'cost_usd>')]; + const slow = lines.map(([label, s]) => [label, Math.round(ms(() => usdMoney(s)))] as const).filter(([, t]) => t >= 250); + expect(slow).toEqual([]); + }, 60_000); }); describe('the rm rule matches what R51 matched', () => {