From f254cebf1200c20cd48af4d2d44fd032c0192aad Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 03:13:34 +0000 Subject: [PATCH 1/9] test(review): planner misreads: SVG chart as runtime, session data as auth, bare base in diff-scope and receipt depth (GST-911, GST-884, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-planner-misreads.test.ts | 236 +++++++++++++++++++++++++++ 1 file changed, 236 insertions(+) create mode 100644 test/review-planner-misreads.test.ts diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts new file mode 100644 index 000000000..f85f066ae --- /dev/null +++ b/test/review-planner-misreads.test.ts @@ -0,0 +1,236 @@ +/** + * Three /ship planner misreads from GST-911 (gstack-evals#132, a results write-up) and GST-884: + * a plotting-script SVG counted as runtime code, a data file named for sessions read as auth code, and + * gstack-diff-scope (plus the receipt depth) resolving a bare base name, so a stale local main or a tag + * named main or origin/main moved the diff. Every real signal stays: an SVG that can run code is + * security, session code is auth, and a credential reference is a secret wherever it is. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { collectCommit, collectLive } from '../lib/review-collect'; +import { depthOfCommitInput } from '../lib/review-depth'; +import { targetedClasses } from '../lib/review-targeted'; +import { receiptDepthAtHead, receiptDepthVerdict } from '../lib/ship-receipt-depth'; + +const BIN = path.join(import.meta.dir, '..', 'bin'); +const SCOPE = path.join(BIN, 'gstack-diff-scope'); +const dirs: string[] = []; +afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); }); + +function fixture() { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'planner-misreads-'))); + dirs.push(root); + const work = path.join(root, 'work'); + const env = { ...process.env, GSTACK_STATE_ROOT: path.join(root, 'state'), GIT_AUTHOR_NAME: 't', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t', GIT_CONFIG_GLOBAL: path.join(root, 'gitconfig'), GIT_CONFIG_NOSYSTEM: '1' }; + fs.writeFileSync(path.join(root, 'gitconfig'), '[commit]\n\tgpgsign = false\n[init]\n\tdefaultBranch = main\n'); + const sh = (cwd: string, c: string) => { const r = spawnSync('bash', ['-c', c], { cwd, env, encoding: 'utf8', timeout: 60_000 }); if (r.status !== 0) throw new Error(`${c}: ${r.stderr}`); return r.stdout.trim(); }; + sh(root, 'git init -q --bare origin.git && git clone -q origin.git work 2>/dev/null'); + const write = (files: Record) => { for (const [p, text] of Object.entries(files)) { const f = path.join(work, p); fs.mkdirSync(path.dirname(f), { recursive: true }); fs.writeFileSync(f, text); } }; + const commit = (files: Record) => { write(files); sh(work, 'git add -A && git commit -qm change'); return sh(work, 'git rev-parse HEAD'); }; + write({ 'README.md': '# evals\n', 'src/run.ts': 'export const run = () => 1;\n' }); + sh(work, 'git add -A && git commit -qm base && git push -q origin main && git checkout -qb feat'); + const base = sh(work, 'git rev-parse HEAD'); + const scope = (args: string[]) => { + const r = spawnSync('bash', [SCOPE, ...args], { cwd: work, env, encoding: 'utf8', timeout: 30_000 }); + return { status: r.status, out: r.stdout, vars: Object.fromEntries(r.stdout.split('\n').filter(l => /^SCOPE_\w+=/.test(l)).map(l => l.split('=') as [string, string])) }; + }; + return { root, work, env, sh, write, commit, base, scope }; +} + +/** A matplotlib-style chart: XML declaration, DOCTYPE, RDF metadata, CSS, markers by internal reference, a clip path. */ +const chart = (points: number, extra = '') => [ + '', + '', + '', + ' ', + ' ', + ' ', + ' ', + ' 2026-10-31T12:00:00', + ' image/svg+xml', + ' ', + ' ', + ' Matplotlib v3.9.2, https://matplotlib.org/', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + ...Array.from({ length: points }, (_, i) => ` `), + ' ', + ' cost per run & pass rate', + ' ', + ...(extra ? [extra] : []), + ' ', + ' ', + ' ', + ' ', + ' ', + ' ', + '', + '', +].join('\n'); + +const plan = (fx: ReturnType, head: string) => { + const c = collectCommit(fx.work, fx.base, head); + const d = depthOfCommitInput({ ...c, gateTagged: 0 }); + return { class: d.class, depth: d.depth, runtime: d.runtimeLines, risk: d.risk }; +}; + +describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is security', () => { + // Value: protects=a results write-up whose chart is an SVG plans like the same chart as a PNG (docs, light, no runtime lines); + // fails_when=an SVG with only drawing elements, internal references and CSS counts as runtime code; seam=collectCommit + depthOfCommitInput, collectLive + test('a plotting-script SVG no code names plans as docs, light, runtime 0, like the PNG', () => { + const fx = fixture(); + const svg = plan(fx, fx.commit({ 'results/2026-10-31/chart.svg': chart(600), 'results/2026-10-31/README.md': '# Results\n\n![chart](chart.svg)\n' })); + expect(svg).toEqual({ class: 'docs', depth: 'light', runtime: 0, risk: [] }); + fx.sh(fx.work, 'git rm -q results/2026-10-31/chart.svg && git commit -qm drop'); + const png = plan(fx, fx.commit({ 'results/2026-10-31/chart.png': '\x89PNG\r\n\x1a\n\0\0\0\rIHDR' })); + expect(png).toEqual({ class: 'docs', depth: 'light', runtime: 0, risk: [] }); + fx.write({ 'results/live/chart.svg': chart(40) }); + const live = collectLive(fx.work, 'main'); + expect(depthOfCommitInput({ ...live, gateTagged: 0 }).runtimeLines).toBe(0); + }, 60_000); + + // Value: protects=an SVG that can execute script stays code and gets the security class (full depth), however it spells the script; + // fails_when=a script element, a namespaced script, an event handler, a javascript: URL (plain or entity-encoded) or a + // foreignObject plans as docs or loses the security class; seam=collectCommit + depthOfCommitInput + test.each([ + ['a script element', ' '], + ['a namespaced script', ' window.x = 42'], + ['an event handler', ' '], + ['a javascript: URL', ' x'], + ['an entity-encoded javascript: URL', ' x'], + ['a foreignObject', '
x
'], + ])('%s stays code with the security class', (_name, line) => { + const fx = fixture(); + expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) }))).toMatchObject({ class: 'code', depth: 'full', risk: ['security'] }); + }, 60_000); + + // Value: protects=only an SVG made of drawing elements is data: anything that loads, links or declares outside that set stays code; + // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs; seam=collectCommit + depthOfCommitInput + test.each([ + ['an external image', ' '], + ['a CSS import', ' '], + ['a CSS url() to another document', ' '], + ['an animation', ' '], + ['a stylesheet processing instruction', ''], + ['an entity declaration', ' ]>'], + ])('%s keeps the SVG code', (_name, line) => { + const fx = fixture(); + expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); + }, 60_000); + + // Value: protects=a passive edit to an SVG that already carries a script is still code, because the whole file decides, not the + // changed lines; and an SVG code names stays code like any consumed data file; fails_when=either plans as docs; seam=collectCommit + test('an edit inside a scripted SVG, and an SVG code names, stay code', () => { + const fx = fixture(); + fx.commit({ 'assets/live.svg': chart(5, ' ') }); + fx.sh(fx.work, 'git checkout -q main && git merge -q --ff-only feat && git checkout -q feat'); + const scripted = { ...fx, base: fx.sh(fx.work, 'git rev-parse HEAD') }; + expect(plan(scripted, fx.commit({ 'assets/live.svg': chart(6, ' ') })).class).toBe('code'); + const named = plan(fx, fx.commit({ 'assets/icon.svg': chart(3), 'src/icon.ts': "export const icon = new URL('./icon.svg', import.meta.url);\n" })); + expect(named.class).toBe('code'); + expect(named.runtime).toBeGreaterThan(3); + }, 60_000); +}); + +describe('GST-911 misread 2: a data file named for sessions is not auth', () => { + // Value: protects=a data file whose name says session never sets SCOPE_AUTH, while session code and session config still do; + // fails_when=recovered-sessions.jsonl sets SCOPE_AUTH, or session.ts, a session store, a sessions controller or config/sessions.json stop setting it; + // seam=bin/gstack-diff-scope + test('gstack-diff-scope: session data files are not auth; session code and config are', () => { + for (const [file, auth] of [ + ['results/recovered-sessions.jsonl', 'false'], ['data/sessions.csv', 'false'], ['logs/session-costs.json', 'false'], ['shots/session.PNG', 'false'], + ['src/auth/session.ts', 'true'], ['lib/session-store.ts', 'true'], ['app/controllers/sessions_controller.rb', 'true'], ['config/sessions.json', 'true'], + ['.github/session.json', 'true'], ['app/.session.json', 'true'], ['session.config.json', 'true'], ['data/oauth-tokens.json', 'true'], + ] as const) { + const fx = fixture(); + fx.commit({ [file]: '{"a": 1}\n', 'src/run.ts': 'export const run = () => 2;\n' }); + expect([file, fx.scope(['main']).vars.SCOPE_AUTH]).toEqual([file, auth]); + } + }, 120_000); + + // Value: protects=the planner adds no auth reader and no gate class for a session-named data file, and a credential in it is still a secret; + // fails_when=targetedClasses returns auth for recovered-sessions.jsonl next to a code change, classifyDiff raises it to gate through SCOPE_AUTH, + // or the secret class misses an env key reference in that file; seam=gstack-diff-scope + collectCommit + targetedClasses + depthOfCommitInput + test('the planner: no auth class and no gate for session rows; real session code is auth; a key reference is a secret', () => { + const fx = fixture(); + const rows = Array.from({ length: 20 }, (_, i) => `{"run": ${i}, "tokens": ${1000 + i}}`).join('\n') + '\n'; + const run = (files: Record) => { + const head = fx.commit(files); + const c = collectCommit(fx.work, fx.base, head); + const scope = fx.scope(['main', '--head', head]).vars; + return { targeted: targetedClasses({ ...c, scope, gateTagged: 0 }).classes, class: depthOfCommitInput({ ...c, scope, gateTagged: 0 }).class }; + }; + expect(run({ 'results/recovered-sessions.jsonl': rows, 'src/run.ts': 'export const run = () => 3;\n' })).toEqual({ targeted: [], class: 'code' }); + expect(run({ 'lib/session-store.ts': 'export const cookie = (sid: string) => `sid=${sid}; HttpOnly`;\n' })).toEqual({ targeted: ['auth'], class: 'gate' }); + expect(run({ 'results/recovered-sessions.jsonl': rows + '{"key": "${OPENAI_API_KEY}"}\n' }).targeted).toEqual(['auth', 'secret']); + }, 120_000); +}); + +describe('GST-884: gstack-diff-scope and the receipt depth read origin/ by its full ref name', () => { + function staleFixture() { + const fx = fixture(); + fx.sh(fx.root, 'git clone -q origin.git other && cd other && mkdir -p db/migrate && echo "create table t;" > db/migrate/001.sql && git add -A && git commit -qm upstream && git push -q origin main'); + fx.sh(fx.work, 'git fetch -q origin && git checkout -q -B feat origin/main && echo "a{}" > styles.css && git add -A && git commit -qm feat'); + return fx; + } + + // Value: protects=a local main behind origin/main never adds upstream files to the scope; fails_when=gstack-diff-scope diffs against the + // stale local main and reports SCOPE_MIGRATIONS for a migration the branch only inherited; seam=bin/gstack-diff-scope + test('a stale local main scopes the same as a fresh one', () => { + const fx = staleFixture(); + expect(fx.sh(fx.work, 'git merge-base main HEAD')).not.toBe(fx.sh(fx.work, 'git merge-base origin/main HEAD')); + const stale = fx.scope(['main']); + fx.sh(fx.work, 'git branch -f main origin/main'); + const fresh = fx.scope(['main']); + expect(fresh.vars).toMatchObject({ SCOPE_FRONTEND: 'true', SCOPE_MIGRATIONS: 'false' }); + expect(stale.out).toBe(fresh.out); + }, 60_000); + + // Value: protects=a tag named main or origin/main cannot stand in for the branch and hide the change; fails_when=gstack-diff-scope resolves + // the base by its short name while a tag of that name exists at HEAD (an empty scope); seam=bin/gstack-diff-scope + for (const [tag, arg] of [['main', 'main'], ['origin/main', 'main'], ['origin/main', 'origin/main']] as const) { + test(`a tag named ${tag} at HEAD does not shadow the base ${arg}`, () => { + const fx = staleFixture(); + fx.sh(fx.work, 'git branch -f main origin/main'); + const before = fx.scope([arg]); + fx.sh(fx.work, `git tag '${tag}' HEAD`); + const after = fx.scope([arg]); + expect(before.vars.SCOPE_FRONTEND).toBe('true'); + expect(after.out).toBe(before.out); + const head = fx.sh(fx.work, 'git rev-parse HEAD'); + expect(fx.scope([arg, '--head', head]).out).toBe(before.out); + }, 60_000); + } + + // Value: protects=the receipt's depth and the queue's recomputation diff against refs/remotes/origin/, never a tag named origin/main; + // fails_when=receiptDepthAtHead or receiptDepthVerdict rev-parse the short name origin/; seam=lib/ship-receipt-depth.ts + test('a tag named origin/main does not move the receipt depth base', () => { + const fx = staleFixture(); + const origin = fx.sh(fx.work, 'git rev-parse refs/remotes/origin/main'); + const head = fx.sh(fx.work, 'git rev-parse HEAD'); + fx.sh(fx.work, "git tag 'origin/main' HEAD && git tag main HEAD"); + const stub = path.join(fx.root, 'stub-bin'); + fs.mkdirSync(stub); + fs.writeFileSync(path.join(stub, 'gstack-diff-scope'), '#!/bin/sh\nexit 0\n', { mode: 0o755 }); + fs.writeFileSync(path.join(stub, 'gstack-gate-diff'), '#!/bin/sh\nexit 1\n', { mode: 0o755 }); + expect(receiptDepthAtHead(fx.work, 'main', stub, head).base).toBe(origin); + const verdict = receiptDepthVerdict(fx.work, 'origin/main', { gstack_version: '1.0.0', review_depth: { depth: 'full', gate_scan: { tagged: 0, head } } } as any, head); + expect(verdict.line).toContain(`inputs=base@${origin.slice(0, 12)}`); + expect(verdict.line).toContain('class=code'); + }, 60_000); +}); From 8b034914cc37e621b84d0c03dc824f087bd13ea8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 03:58:06 +0000 Subject: [PATCH 2/9] fix(review): a drawing-only SVG is data and a scripted one is security; session data files are not auth; diff-scope and the receipt depth read origin/ by full ref name (GST-911, GST-884) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 53 ++++++++++++++++++++--- bin/gstack-review-plan | 2 +- changelog.d/gst-911-planner-misreads.md | 8 ++++ docs/review-policy.md | 20 ++++++--- lib/review-collect.ts | 31 +++++++++++--- lib/review-depth.ts | 57 ++++++++++++++++++++++--- lib/review-risk.ts | 15 ++++++- lib/ship-preflight.ts | 2 +- lib/ship-receipt-depth.ts | 7 ++- test/review-packets-removed.test.ts | 5 ++- 10 files changed, 169 insertions(+), 31 deletions(-) create mode 100644 changelog.d/gst-911-planner-misreads.md diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index 3ec81cf09..d7da8ad86 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -37,6 +37,30 @@ BASE="${1:-$(_default_base)}" HEAD_REF="" if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi +# The base as a commit (GST-884, mirroring lib/gate-diff/git.ts resolveBase from +# GST-883): a branch name reads refs/remotes/ (origin/main as given), then +# refs/remotes// that shares history with the head (origin first), +# then refs/heads/, all by full ref name, so a stale local main never widens +# the scope and a tag named main or origin/main cannot stand in for the branch. +# A sha, a relative ref (HEAD~1, main^2, @{u}) or a full refs/ name resolves as +# given. Prints nothing when nothing resolves. +_is_branch_name() { + case "$1" in refs/*|*~*|*^*|*@\{*|HEAD|*_HEAD) return 1 ;; esac + [[ ! "$1" =~ ^[0-9a-f]{7,64}$ ]] +} +_commit() { git rev-parse --verify -q "$1^{commit}" 2>/dev/null; } +_resolve_base() { + local ref="$1" head="$2" sha remote + if ! _is_branch_name "$ref"; then _commit "$ref"; return; fi + sha=$(_commit "refs/remotes/$ref") && { echo "$sha"; return; } + for remote in $(git remote 2>/dev/null | grep -x origin; git remote 2>/dev/null | grep -vx origin); do + sha=$(_commit "refs/remotes/$remote/$ref") || continue + git merge-base "$sha" "$head" >/dev/null 2>&1 && { echo "$sha"; return; } + done + sha=$(_commit "refs/heads/$ref") && { echo "$sha"; return; } + _commit "$ref" +} + FRONTEND=false BACKEND=false PROMPTS=false @@ -53,6 +77,19 @@ AUTH=false # the root-anchored globs alone (#3335). TEST_PATH_RE='(^|/)(tests?|spec|__tests__)/|(^|/)tests?\.py$|(^|/)test_[^/]+\.py$|_test\.(go|py|rb|ts|js|exs|dart)$|\.(test|spec)\.[jt]sx?$|_spec\.rb$|Test\.(java|kt)$' +# Data files (lib/review-depth.ts DATA_PATH) outside the config locations its +# DATA_CONFIG and agent-prose rules keep code: .github/, config/, dotfiles, +# *.config.*, .gstack/. Case-insensitive, as DATA_PATH is. +DATA_FILE_RE='\.(json|jsonl|ndjson|csv|tsv|png|jpe?g|gif|webp|bmp|ico|avif)$' +DATA_CONFIG_RE='(^|/)\.github/|(^|/)config/|(^|/)\.[^/]+$|(^|/)[^/]+\.config\.[^/]+$|^\.gstack/' +_session_data() { + local r=1 + shopt -s nocasematch + if [[ "$1" =~ $DATA_FILE_RE ]] && [[ ! "$1" =~ $DATA_CONFIG_RE ]]; then r=0; fi + shopt -u nocasematch + return "$r" +} + _print_flags() { echo "SCOPE_FRONTEND=$FRONTEND" echo "SCOPE_BACKEND=$BACKEND" @@ -68,7 +105,8 @@ _print_flags() { # Base reachability (#2526): a shallow CI checkout or an unfetched ref makes # `git diff` return an empty list ā€” all-false with exit 0, a green that means # "we could not look". Distinguish it before diffing. -if ! git rev-parse --verify -q "${BASE}^{commit}" >/dev/null 2>&1; then +BASE_SHA=$(_resolve_base "$BASE" "${HEAD_REF:-HEAD}" || true) +if [ -z "$BASE_SHA" ]; then _print_flags echo "SCOPE_ERROR=no_base" echo "# base ref '${BASE}' is not resolvable ā€” shallow checkout or missing fetch. Run: git fetch origin ${BASE}" @@ -91,16 +129,16 @@ if [ -n "$HEAD_REF" ] || [ "${2:-}" = "--head" ]; then echo "# --head '${HEAD_REF}' is not a commit in this repository" exit 2 fi - if ! git merge-base "${BASE}" "${HEAD_REF}" >/dev/null 2>&1; then + if ! git merge-base "${BASE_SHA}" "${HEAD_REF}" >/dev/null 2>&1; then _print_flags echo "SCOPE_ERROR=no_merge_base" echo "# '${BASE}' and '${HEAD_REF}' share no merge base" exit 2 fi - _collect < <(git diff -z "${BASE}...${HEAD_REF}" --name-only 2>/dev/null) + _collect < <(git diff -z "${BASE_SHA}...${HEAD_REF}" --name-only 2>/dev/null) else # Committed diff vs base (merge-base form; two-dot fallback when no merge base). -_collect < <(git diff -z "${BASE}...HEAD" --name-only 2>/dev/null || git diff -z "${BASE}" --name-only 2>/dev/null || true) +_collect < <(git diff -z "${BASE_SHA}...HEAD" --name-only 2>/dev/null || git diff -z "${BASE_SHA}" --name-only 2>/dev/null || true) # Working-tree changes (staged + unstaged). `git diff HEAD` fails on a repo # with no commits; tolerated. _collect < <(git diff -z HEAD --name-only 2>/dev/null || true) @@ -190,9 +228,12 @@ for f in ${FILES_LIST[@]+"${FILES_LIST[@]}"}; do *.graphql|*.gql|openapi.*|swagger.*) m_api=true ;; esac - # Auth: authentication, authorization, sessions, permissions + # Auth: authentication, authorization, sessions, permissions. A data file + # named for sessions (recovered-sessions.jsonl, GST-911) holds rows, not + # session handling, so it is not auth; session config still is. case "$f" in - *auth*|*session*|*jwt*|*oauth*|*permission*|*role*) m_auth=true ;; + *auth*|*jwt*|*oauth*|*permission*|*role*) m_auth=true ;; + *session*) _session_data "$f" || m_auth=true ;; esac # Backend: code that isn't a frontend component/view file. Includes ESM/CJS diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 93f6aee5e..56031d8e8 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -149,7 +149,7 @@ function main(): number { const gateScan = { tagged: gateTagged, head }; if (has('--json')) { const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; - process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed }) + '\n'); + process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed, drawings: collected.drawings }) + '\n'); return EXIT.ok; } const out = [`REVIEW_CLASS: ${plan.class} (${plan.reasons.slice(0, 3).join('; ') || 'no signals'})`, reviewDepthLine(plan), `GATE_SCAN: ${gateTagged === null ? 'unavailable' : `tagged=${gateTagged}`} head=${head}`, diff --git a/changelog.d/gst-911-planner-misreads.md b/changelog.d/gst-911-planner-misreads.md new file mode 100644 index 000000000..b3944c884 --- /dev/null +++ b/changelog.d/gst-911-planner-misreads.md @@ -0,0 +1,8 @@ +### Fixed + +- **/ship no longer reviews a results write-up as if it were gate code.** Three planner misreads on gstack-evals#132 (GST-911) pushed a docs PR to the five-reader full panel. A chart SVG made of drawing elements now counts as data, like the same chart as a PNG; an SVG with a script, `foreignObject`, event handler or `javascript:` URL stays code and now gets the security class. A data file named for sessions (`recovered-sessions.jsonl`) no longer sets the auth scope, so it adds no auth reader and no gate class; session code and session config still do. +- **`gstack-diff-scope` and the receipt depth read `origin/` by its full ref name (GST-884).** A local `main` behind `origin/main` no longer adds upstream files to the scope, and a tag named `main` or `origin/main` can no longer stand in for the branch. + +### Tests + +- `test/review-planner-misreads.test.ts`: a 600-point matplotlib-style chart plans docs, light, runtime 0 (before: code, 636 runtime lines); six ways an SVG can run script keep it code with the security class, and six other non-drawing constructs keep it code; session data files leave `SCOPE_AUTH` false while session code and config set it; a stale local `main` and tags named `main` or `origin/main` leave the scope and the receipt base unchanged. diff --git a/docs/review-policy.md b/docs/review-policy.md index 1d4e3612b..e3662dc64 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -181,7 +181,12 @@ follows that file, four files deep. Precedence: agent prose (`.gstack/**`), a te fixture path, a gate path, a config file (`.github/**`, anything under a `config/` directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named -consumer each keep the path code, and YAML, TOML and SVG are always code. More than +consumer each keep the path code, and YAML and TOML are always code. An SVG counts +as data only when its whole text at the head is a drawing (GST-911): SVG drawing +elements and RDF metadata, CSS with no import and no `url()` outside the file, every +`href` a same-file `#` reference or an inline raster image, no event handler and no +entity declaration, read after character references are decoded; anything else, a +script or `foreignObject` included, keeps it code. More than 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed` and fails the depth closed. Only `classifyDiff` applies the rule (`lib/review-collect.ts` finds the consumers, `gstack-ship-preflight`'s `RUNTIME_LINES` reads the same @@ -203,15 +208,18 @@ added and deleted lines; any class makes the depth full (`risk:` in [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code)'s matchers. **security** is a trust-boundary path (trust, redact, sanitize, neutralize, consent, escape) or line (`shell: true`, `execSync(`, `eval`, `new Function(`, -raw HTML sinks, a shell run through argv, recursive deletes). **data** is user data: a path naming pii, gdpr, ccpa, privacy, +raw HTML sinks, a shell run through argv, recursive deletes), or a changed SVG +line that can run code (a script or `foreignObject` element, an event handler, a +`javascript:` URL). **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 the patterns and `DEPTH_RULES` includes it. -**UI text and SVGs** stay code (light unless a risk class applies). A docs path -for UI copy, locale values and chart SVGs waits for an allowlist classifier: three -rounds of R51 review found bypasses in line-pattern versions of both. +**UI text** stays code (light unless a risk class applies). A docs path for UI +copy and locale values waits for an allowlist classifier: three rounds of R51 +review found bypasses in line-pattern versions. Chart SVGs use the drawing +allowlist above, which reads the whole file rather than changed lines. **Batched checklists (the lean ensemble, GSTA-167 part 2).** One specialist subagent reads Testing and Security on every code or gate diff, plus @@ -330,7 +338,7 @@ and matching is token-bounded: `costume` never reads as cost. |---|---| | 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 | | 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 | +| 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` | | 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 | An input the classifier cannot read (the diff, `gstack-diff-scope`, the gate diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7fbfdae12..66fa33910 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -12,7 +12,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { GENERATED_PATH, parseRegistry } from './regen'; import { RESULT_CODES } from './result-codes'; -import { classifyPath, dataCandidate, type ClassInput, type DepthError, type PathLines, type RegistryInfo } from './review-depth'; +import { classifyPath, dataCandidate, svgCandidate, svgDrawing, type ClassInput, type DepthError, type PathLines, type RegistryInfo } from './review-depth'; export interface Collected extends ClassInput { perPath: PathLines[]; @@ -28,6 +28,7 @@ export interface Collected extends ClassInput { mergeBase: string | null; untracked: string[]; consumed: string[] | 'all'; + drawings: string[]; } function git(root: string, args: string[]): { ok: boolean; status: number | null; out: string; err: string } { @@ -106,7 +107,7 @@ export const CONSUMER_SCAN_DEPTH = 4; * a collector error, which fails the depth closed. */ export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[]): string[] | 'all' { - const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot)).sort(); + const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot) || svgCandidate(p, classRoot)).sort(); if (candidates.length > CONSUMER_SCAN_CAP) return 'all'; const base = (p: string) => p.slice(p.lastIndexOf('/') + 1); const named = new Map>(); @@ -146,9 +147,20 @@ export function scanConsumers(root: string, paths: readonly string[], revs: read return candidates.filter(p => consumed.has(p)); } -function result(diff: ReturnType, extra: Pick): Collected { +/** The largest SVG whose text is read for the drawing check; a larger one stays code. */ +export const DRAWING_READ_CAP = 8 * 1024 * 1024; + +/** The changed SVG candidates whose whole text, read by `read` (null when it cannot be read), is a drawing (lib/review-depth.ts svgDrawing). */ +function drawingsAmong(perPath: readonly PathLines[], classRoot: string | readonly string[], read: (p: string) => Buffer | null): string[] { + return perPath.filter(e => !e.binary && svgCandidate(e.path, classRoot)).map(e => e.path).filter(p => { + const buf = read(p); + return !!buf && buf.length <= DRAWING_READ_CAP && !buf.includes(0) && svgDrawing(buf.toString('utf8')); + }).sort(); +} + +function result(diff: ReturnType, extra: Pick & { drawings?: string[] }): Collected { const paths = diff.perPath.map(e => e.path).sort(); - return { ...diff, ...extra, paths, gateTagged: null, diffLines: diff.perPath.reduce((n, e) => n + e.lines, 0) }; + return { ...diff, drawings: [], ...extra, paths, gateTagged: null, diffLines: diff.perPath.reduce((n, e) => n + e.lines, 0) }; } /** The range `baseSha`...`headSha` from git objects alone; the registry is the base commit's, never the head's. */ @@ -165,7 +177,11 @@ export function collectCommit(root: string, baseSha: string, headSha: string): C const tree = git(root, ['ls-tree', '-r', '--name-only', '-z', headSha]); const skillDirs = [...new Set(tree.out.split('\0').flatMap(p => /^([^/]+)\/SKILL\.md(\.tmpl)?$/.exec(p)?.[1] ?? []))].sort(); const consumed = scanConsumers(root, [...diff.perPath.map(e => e.path), ...diff.deleted], [headSha, mergeBase], false, skillDirs, errors); - return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [], consumed }); + const drawings = drawingsAmong(diff.perPath, skillDirs, p => { + const blob = spawnSync('git', ['cat-file', 'blob', `${headSha}:${p}`], { cwd: root, timeout: 60_000, maxBuffer: DRAWING_READ_CAP + 1 }); + return blob.status === 0 && !blob.error ? blob.stdout : null; + }); + return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [], consumed, drawings }); } /** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ @@ -193,5 +209,8 @@ export function collectLive(root: string, base: string): Collected { if (!binary) diff.added[p] = text.split('\n'); } const consumed = scanConsumers(root, [...diff.perPath.map(e => e.path), ...diff.deleted], [mergeBase], true, root, errors); - return result(diff, { root, registry, errors, mergeBase, untracked, consumed }); + const drawings = drawingsAmong(diff.perPath, root, p => { + try { return fs.statSync(path.join(root, p)).size <= DRAWING_READ_CAP ? fs.readFileSync(path.join(root, p)) : null; } catch { return null; } + }); + return result(diff, { root, registry, errors, mergeBase, untracked, consumed, drawings }); } diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 714e44183..78af654ac 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -13,7 +13,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { ResultCodeName } from './result-codes'; import { matchesAny } from './ship-policy'; -import { matchRisk, RISK_RULES, type RiskClass } from './review-risk'; +import { decodeCharRefs, matchRisk, RISK_RULES, SVG_PATH, svgActive, type RiskClass } from './review-risk'; export type DiffClass = 'docs' | 'tests' | 'code' | 'gate'; export type PathClass = 'docs' | 'tests' | 'code'; @@ -58,7 +58,7 @@ const GATE_PATH = [ /^lib\/review-(depth|collect|scope|targeted|risk)\.ts$/, /^review\/specialists\/targeted\.md$/, /^bin\/gstack-review-plan$/, /^lib\/ship-queue[^/]*\.ts$/, /^lib\/regen\.ts$/, /^\.gstack\/generated\.json$/, /^ship\/sections\/(review-army|adversarial)\.md\.tmpl$/, /^scripts\/resolvers\/review-army\.ts$/, ]; -/** Data files: JSON, CSV, TSV and raster images. YAML, TOML and SVG are never data here: deploy configs, API specs and executable SVG stay code. */ +/** Data files: JSON, CSV, TSV and raster images. YAML and TOML are never data here: deploy configs and API specs stay code. An SVG is data only as a drawing (svgDrawing). */ export const DATA_PATH = /\.(json|jsonl|ndjson|csv|tsv|png|jpe?g|gif|webp|bmp|ico|avif)$/i; /** Data-shaped files tools read by convention, with no code naming them: always code. */ const DATA_CONFIG = [ @@ -66,10 +66,50 @@ const DATA_CONFIG = [ /(^|\/)(package|package-lock|npm-shrinkwrap|composer|deno|jsr|tsconfig[^/]*|jsconfig[^/]*|manifest|app|vercel|netlify|firebase|renovate|turbo|nx|lerna|biome|angular|nest-cli|lint-staged|components|pyrightconfig|cdk|serverless|appsettings[^/]*)\.json$/i, ]; +/** + * The SVG elements a chart is drawn with, lowercased, under any namespace prefix: shapes, text, paint servers, clip paths, + * filters (feImage loads a document, so it is left out), CSS, and the RDF metadata plotting libraries write. No script, + * foreignObject, link, animation or embedded document. + */ +const SVG_ELEMENTS = new Set([ + 'svg', 'g', 'defs', 'symbol', 'use', 'path', 'rect', 'circle', 'ellipse', 'line', 'polyline', 'polygon', 'text', 'tspan', 'textpath', 'title', 'desc', + 'metadata', 'clippath', 'mask', 'pattern', 'marker', 'lineargradient', 'radialgradient', 'stop', 'style', 'image', 'filter', + 'feblend', 'fecolormatrix', 'fecomponenttransfer', 'fecomposite', 'feconvolvematrix', 'fediffuselighting', 'fedisplacementmap', 'fedistantlight', + 'fedropshadow', 'feflood', 'fefunca', 'fefuncb', 'fefuncg', 'fefuncr', 'fegaussianblur', 'femerge', 'femergenode', 'femorphology', 'feoffset', + 'fepointlight', 'fespecularlighting', 'fespotlight', 'fetile', 'feturbulence', + 'rdf', 'work', 'agent', 'type', 'date', 'format', 'creator', 'description', 'license', +]); +/** Markup a drawing may hold besides elements: comments, CDATA, the XML declaration, and a DOCTYPE with no internal subset. */ +const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg\b[^>[]*>)/i; +/** What a drawing never holds: a named entity beyond the five XML ones, a CSS import, a url() to another document, a CSS expression or binding. */ +const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; +/** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ +const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+/=\s]*$)/i; + +/** + * Whether an SVG's whole text is a drawing (GST-911): every element in SVG_ELEMENTS, no event handler, every href or src a + * same-document reference or an inline raster image, and nothing SVG_REFUSED names, read after character references are + * decoded. Anything else, an SVG that can run code included, stays code. + */ +export function svgDrawing(text: string): boolean { + const t = decodeCharRefs(text); + if (SVG_REFUSED.some(r => r.test(t)) || t.split('\n').some(svgActive)) return false; + for (let at = t.indexOf('<'); at !== -1; at = t.indexOf('<', at + 1)) { + if (SVG_MARKUP.test(t.slice(at, at + 512))) continue; + const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); + if (!tag || !SVG_ELEMENTS.has(tag[1]!.toLowerCase())) return false; + } + for (const m of t.matchAll(/(?]+))/g)) { + const local = m[1]!.slice(m[1]!.lastIndexOf(':') + 1).toLowerCase(); + if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(m[2] ?? m[3] ?? m[4] ?? ''))) return false; + } + return true; +} + /** A hash of the constants and patterns that decide a depth, not of this file's bytes: a comment edit keeps it, a threshold edit changes it. */ export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ - RISK_RULES, SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, BATCH_SCOPED, SCOPED_RANGE_LINES, ROLE_EFFORT, - patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), + RISK_RULES, SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, BATCH_SCOPED, SCOPED_RANGE_LINES, ROLE_EFFORT, SVG_ELEMENTS: [...SVG_ELEMENTS], + patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); /** @@ -97,6 +137,11 @@ export function dataCandidate(p: string, root?: string | readonly string[]): boo return DATA_PATH.test(p) && classifyPath(p, root) === 'code' && ![...AGENT_PROSE, ...GATE_PATH, ...DATA_CONFIG].some(r => r.test(p)); } +/** An SVG the data rule may move to docs once the collector reads its whole text as a drawing (ClassInput.drawings); never a data file another file is named through. */ +export function svgCandidate(p: string, root?: string | readonly string[]): boolean { + return SVG_PATH.test(p) && classifyPath(p, root) === 'code' && ![...AGENT_PROSE, ...GATE_PATH, ...DATA_CONFIG].some(r => r.test(p)); +} + /** Whether a range needs the gate scan: any path that is not docs by the path-only rule. The data rule never skips it. */ export function gateScanNeeded(paths: readonly string[], root?: string | readonly string[]): boolean { return paths.some(p => classifyPath(p, root) !== 'docs'); @@ -131,6 +176,8 @@ export interface ClassInput { * (lib/review-collect.ts). A candidate not listed counts as docs; 'all' or no scan keeps every data file code. */ consumed?: readonly string[] | 'all'; + /** The SVG candidates (svgCandidate) whose whole text the collector read as a drawing (svgDrawing); any other SVG stays code. */ + drawings?: readonly string[]; /** Added and deleted lines per path (lib/review-collect.ts): the risk classes read them; without them no line signal applies. */ added?: Record; removed?: Record; @@ -139,7 +186,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); +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))); const pathClass = (i: ClassInput, p: string): PathClass => ((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 c1ed328a1..522c11d62 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -6,7 +6,8 @@ * money, auth and secret are the targeted pass's matchers (GST-257). security is a trust-boundary path or line * (a redaction, sanitizer, consent or trust module; a shell, eval or raw HTML sink); data is user data (a path * naming personal or stored data, or a line that drops or bulk-deletes it). Path and identifier signals are - * token-bounded: costume never names cost. + * token-bounded: costume never names cost. A changed SVG line that can run code (a script or foreignObject element, an + * event handler, a javascript: URL; character references decoded) is security (GST-911). */ import { createHash } from 'node:crypto'; @@ -40,6 +41,15 @@ const SECURITY_LINE = [ /\brmSync\s*\([^)]*recursive\s*:\s*true/, /\brm\s+-(?:[a-z]*r[a-z]*f|[a-z]*f[a-z]*r)\b/, /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/, ]; +export const SVG_PATH = /\.svg$/i; +/** An SVG line that can run code, read decoded and again with whitespace removed (java script: is javascript:). */ +export const SVG_ACTIVE = [/<(?:[\w.-]+:)?(?:script|foreignObject)\b/i, /(?:^|[\s"'/])on[a-z]+\s*=/i, /(?:java|vb)script:|data:text\/html/i]; +/** XML character references decoded (j and j are j); an out-of-range one decodes to nothing. */ +export const decodeCharRefs = (s: string) => s.replace(/&#(x[0-9a-f]{1,8}|\d{1,10});?/gi, (_, n: string) => { + const c = n[0] === 'x' || n[0] === 'X' ? parseInt(n.slice(1), 16) : Number(n); + return c <= 0x10ffff ? String.fromCodePoint(c) : ''; +}); +export const svgActive = (line: string) => { const d = decodeCharRefs(line); const squeezed = d.replace(/[\s\0-\x1f]+/g, ''); return SVG_ACTIVE.some(r => r.test(d) || r.test(squeezed)); }; const DATA_PATH_TOKENS = ['pii', 'gdpr', 'ccpa', 'privacy', 'personal', 'account', 'accounts', 'customer', 'customers', 'cookie', 'cookies', 'persistence', 'storage', 'database']; const DATA_LINE = [ /\bDELETE\s+FROM\b/, /\b(?:drop\s+(?:table|column|database|schema)|truncate\s+table)\b/i, /\bdelete\s+from\s+[\w."`]+\s*(?:where\b|;|$|["'`)])/i, @@ -49,7 +59,7 @@ const DATA_LINE = [ /** 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, - patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, DATA_LINE }).map(([k, v]) => [k, v.map(r => '/' + r.source + '/' + r.flags)]), + 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)]), })).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); @@ -78,6 +88,7 @@ export function matchRisk(i: RiskInput): Map { if (!why.has('money') && MONEY_LINE.some(r => r.test(line))) 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('security') && SVG_PATH.test(p) && svgActive(line)) hit('security', 'active SVG line: ' + p); if (!why.has('data') && DATA_LINE.some(r => r.test(line))) hit('data', 'data line: ' + p); } } diff --git a/lib/ship-preflight.ts b/lib/ship-preflight.ts index 519ee5475..405a4eda8 100644 --- a/lib/ship-preflight.ts +++ b/lib/ship-preflight.ts @@ -117,7 +117,7 @@ function merge(cwd: string, base: string, r: Report): boolean { let runtime = 'unknown'; try { const j = JSON.parse(plan.stdout); - const c = classifyDiff({ paths: j.paths, perPath: j.per_path, stampOnly: j.stamp_only, registry: j.registry, consumed: j.consumed, root: cwd, gateTagged: 0 }); + const c = classifyDiff({ paths: j.paths, perPath: j.per_path, stampOnly: j.stamp_only, registry: j.registry, consumed: j.consumed, drawings: j.drawings, root: cwd, gateTagged: 0 }); runtime = String(c.runtimeLines ?? 'unknown'); } catch { /* unknown */ } r.add(`RUNTIME_LINES: ${runtime} (gstack-ship-budget check --runtime-lines at the Step 9 and 12 boundaries)`); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index ad8933cfb..4452321e9 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -40,6 +40,9 @@ function git(root: string, args: string[]): { ok: boolean; out: string } { return { ok: r.status === 0, out: (r.stdout ?? '').trim() }; } +/** A base as a commit; origin/ reads refs/remotes/origin/ by its full name, so a tag named origin/main cannot stand in for it (GST-884). */ +const baseCommit = (root: string, ref: string) => git(root, ['rev-parse', '--verify', '--quiet', `${/^origin\//.test(ref) ? `refs/remotes/${ref}` : ref}^{commit}`]); + /** gstack-diff-scope on the committed range `base`...`head` (`--head`): the same signals at write, carry and the queue. */ function scopeOfRange(root: string, binDir: string, base: string, head: string): Record { const sr = runScriptSync(path.join(binDir, 'gstack-diff-scope'), [base, '--head', head], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024 }); @@ -58,7 +61,7 @@ function scopeOfRange(root: string, binDir: string, base: string, head: string): */ export function receiptDepthAtHead(repoRoot: string, base: string, binDir: string, head: string): ReceiptDepth { const branch = base.replace(/^origin\//, ''); - const baseSha = git(repoRoot, ['rev-parse', '--verify', '--quiet', `origin/${branch}^{commit}`]); + const baseSha = baseCommit(repoRoot, `origin/${branch}`); const baseRev = baseSha.ok ? baseSha.out : branch; const c = collectCommit(repoRoot, baseRev, head); const scope = scopeOfRange(repoRoot, binDir, baseRev, head); @@ -156,7 +159,7 @@ export interface DepthVerdict { ok: boolean; line: string; message?: string } /** The head's committed range against the base, read once for the depth verdict and the targeted line. */ interface HeadInputs { baseSha: { ok: boolean; out: string }; c: ReturnType; scope: Record } function headInputs(root: string, baseRef: string, head: string): HeadInputs { - const baseSha = git(root, ['rev-parse', '--verify', '--quiet', `${baseRef}^{commit}`]); + const baseSha = baseCommit(root, baseRef); const baseRev = baseSha.ok ? baseSha.out : baseRef; return { baseSha, c: collectCommit(root, baseRev, head), scope: scopeOfRange(root, BIN_DIR, baseRev, head) }; } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 747723390..77ab96def 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -56,6 +56,7 @@ const NO_LOSS_REPLAYS = { 'G1': "GSTA-167 variant G Arm G1: live no-loss replay of both prose voices with part 1's prompts, 2 passes, normalizer v2, paired rule against pooled C' (parts 1 and 3; specialists at medium as in C and C2)", '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-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", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; @@ -78,8 +79,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: 'd649569a163c03476c6e0c5ea295f5d02088f96aab85fbc7d558f2ac2b27006a', replay: 'R51' }, - 'lib/review-risk.ts': { sha: 'dbef7475d467e17340b21a2bacb52a16a7ccb1ee66291900ed2d36dfb5cfb7f2', replay: 'R51' }, + 'lib/review-depth.ts': { sha: '7352b9e6e4e9f44c6e94f56cbb908d9a16b2ff536548770470af6169ffabccf7', replay: 'GST-911' }, + 'lib/review-risk.ts': { sha: 'c1b1baec718798df9e7629d641a864dc2125455f51339535f7c95057e0d83fcd', replay: 'GST-911' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { From d8227c100dff7faca2ed61427c953ae2d9cc8bce Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:22:06 +0000 Subject: [PATCH 3/9] fix(review): the SVG drawing check refuses CSS escapes and reads zero-padded character references; the security class reads a file's changed SVG lines joined (GST-911 review pass 1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 3 ++- lib/review-depth.ts | 9 ++++++--- lib/review-risk.ts | 13 +++++++------ test/review-packets-removed.test.ts | 4 ++-- test/review-planner-misreads.test.ts | 14 +++++++++++--- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index e3662dc64..be7396557 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -183,7 +183,8 @@ directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named consumer each keep the path code, and YAML and TOML are always code. An SVG counts as data only when its whole text at the head is a drawing (GST-911): SVG drawing -elements and RDF metadata, CSS with no import and no `url()` outside the file, every +elements and RDF metadata, CSS with no import, no `url()` outside the file and no +backslash escape, every `href` a same-file `#` reference or an inline raster image, no event handler and no entity declaration, read after character references are decoded; anything else, a script or `foreignObject` included, keeps it code. More than diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 78af654ac..5044c3b0a 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -81,8 +81,11 @@ const SVG_ELEMENTS = new Set([ ]); /** Markup a drawing may hold besides elements: comments, CDATA, the XML declaration, and a DOCTYPE with no internal subset. */ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg\b[^>[]*>)/i; -/** What a drawing never holds: a named entity beyond the five XML ones, a CSS import, a url() to another document, a CSS expression or binding. */ -const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; +/** + * What a drawing never holds: a named entity beyond the five XML ones, a backslash (a CSS escape such as @\69mport), a CSS + * import, a url() to another document, a CSS expression or binding. + */ +const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; /** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+/=\s]*$)/i; @@ -93,7 +96,7 @@ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+ */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); - if (SVG_REFUSED.some(r => r.test(t)) || t.split('\n').some(svgActive)) return false; + if (SVG_REFUSED.some(r => r.test(t)) || svgActive(t)) return false; for (let at = t.indexOf('<'); at !== -1; at = t.indexOf('<', at + 1)) { if (SVG_MARKUP.test(t.slice(at, at + 512))) continue; const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 522c11d62..2c21875a6 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -42,14 +42,15 @@ const SECURITY_LINE = [ /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/, ]; export const SVG_PATH = /\.svg$/i; -/** An SVG line that can run code, read decoded and again with whitespace removed (java script: is javascript:). */ +/** SVG text that can run code, read decoded and again with whitespace removed (java 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]; -/** XML character references decoded (j and j are j); an out-of-range one decodes to nothing. */ -export const decodeCharRefs = (s: string) => s.replace(/&#(x[0-9a-f]{1,8}|\d{1,10});?/gi, (_, n: string) => { - const c = n[0] === 'x' || n[0] === 'X' ? parseInt(n.slice(1), 16) : Number(n); +/** XML character references decoded, leading zeros included (j, j and j are j); an out-of-range one decodes to nothing. */ +export const decodeCharRefs = (s: string) => s.replace(/&#(?:[xX]0*([0-9a-fA-F]+)|0*(\d+));?/g, (_, hex: string | undefined, dec: string | undefined) => { + const digits = hex ?? dec!; + const c = digits.length > 8 ? Infinity : hex !== undefined ? parseInt(hex, 16) : Number(dec); return c <= 0x10ffff ? String.fromCodePoint(c) : ''; }); -export const svgActive = (line: string) => { const d = decodeCharRefs(line); const squeezed = d.replace(/[\s\0-\x1f]+/g, ''); return SVG_ACTIVE.some(r => r.test(d) || r.test(squeezed)); }; +export const svgActive = (text: string) => { const d = decodeCharRefs(text); const squeezed = d.replace(/[\s\0-\x1f]+/g, ''); return SVG_ACTIVE.some(r => r.test(d) || r.test(squeezed)); }; const DATA_PATH_TOKENS = ['pii', 'gdpr', 'ccpa', 'privacy', 'personal', 'account', 'accounts', 'customer', 'customers', 'cookie', 'cookies', 'persistence', 'storage', 'database']; const DATA_LINE = [ /\bDELETE\s+FROM\b/, /\b(?:drop\s+(?:table|column|database|schema)|truncate\s+table)\b/i, /\bdelete\s+from\s+[\w."`]+\s*(?:where\b|;|$|["'`)])/i, @@ -84,11 +85,11 @@ export function matchRisk(i: RiskInput): Map { if (t.some(x => MONEY_PATH_TOKENS.includes(x))) 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))) 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('security') && SVG_PATH.test(p) && svgActive(line)) hit('security', 'active SVG line: ' + p); if (!why.has('data') && DATA_LINE.some(r => r.test(line))) hit('data', 'data line: ' + p); } } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 77ab96def..d84691081 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -79,8 +79,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: '7352b9e6e4e9f44c6e94f56cbb908d9a16b2ff536548770470af6169ffabccf7', replay: 'GST-911' }, - 'lib/review-risk.ts': { sha: 'c1b1baec718798df9e7629d641a864dc2125455f51339535f7c95057e0d83fcd', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: 'd3b269015a8d97b4cf5654c0dacc5bea7562b96e3c1c3c6fc9442ef4bb336637', replay: 'GST-911' }, + 'lib/review-risk.ts': { sha: 'df618e8aef3d641e8bf2073d8dca170740434acbb69c04555c4166df047f566c', replay: 'GST-911' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index f85f066ae..2edcf5e72 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -105,8 +105,8 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se }, 60_000); // Value: protects=an SVG that can execute script stays code and gets the security class (full depth), however it spells the script; - // fails_when=a script element, a namespaced script, an event handler, a javascript: URL (plain or entity-encoded) or a - // foreignObject plans as docs or loses the security class; seam=collectCommit + depthOfCommitInput + // fails_when=a script element, a namespaced script, an event handler, a javascript: URL (plain, entity-encoded or zero-padded) or a + // foreignObject plans as docs or loses the security class, or a handler or URL split across lines loses it; seam=collectCommit + depthOfCommitInput test.each([ ['a script element', ' '], ['a namespaced script', ' window.x = 42'], @@ -114,13 +114,17 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['a javascript: URL', ' x'], ['an entity-encoded javascript: URL', ' x'], ['a foreignObject', '
x
'], + ['a zero-padded javascript: URL', ' x'], + ['an event handler split across lines', ' '], + ['a javascript: URL split across lines', ' x'], ])('%s stays code with the security class', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) }))).toMatchObject({ class: 'code', depth: 'full', risk: ['security'] }); }, 60_000); // Value: protects=only an SVG made of drawing elements is data: anything that loads, links or declares outside that set stays code; - // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs; seam=collectCommit + depthOfCommitInput + // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs, + // including through a CSS escape or a zero-padded character reference (review pass 1 findings); seam=collectCommit + depthOfCommitInput test.each([ ['an external image', ' '], ['a CSS import', ' '], @@ -128,6 +132,10 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['an animation', ' '], ['a stylesheet processing instruction', ''], ['an entity declaration', ' ]>'], + ['a CSS-escaped import', ' '], + ['a CSS-escaped url()', ' '], + ['a zero-padded character reference import', ' '], + ['a zero-padded hex character reference url()', ' '], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); From 10af2e1edb11623cacc388c300253f5e4ceef5ec Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:33:26 +0000 Subject: [PATCH 4/9] test(review): a stray quote cannot hide an external href in a drawing SVG (GST-911 review pass 2, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-planner-misreads.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index 2edcf5e72..15bda38b1 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -124,7 +124,8 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se // Value: protects=only an SVG made of drawing elements is data: anything that loads, links or declares outside that set stays code; // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs, - // including through a CSS escape or a zero-padded character reference (review pass 1 findings); seam=collectCommit + depthOfCommitInput + // including through a CSS escape or a zero-padded character reference (review pass 1 findings) or behind a stray quote that + // pairs with a later one (review pass 2 finding); seam=collectCommit + depthOfCommitInput test.each([ ['an external image', ' '], ['a CSS import', ' '], @@ -136,6 +137,10 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['a CSS-escaped url()', ' '], ['a zero-padded character reference import', ' '], ['a zero-padded hex character reference url()', ' '], + ['an external image after a stray quote in text', " z=''"], + ['an external use after a stray quote in a comment', " "], + ['an external xlink:href after a stray quote in a desc', " z=''"], + ['an external image after a stray href in text', " href='#'"], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); From 4b3abead1fec9f7c05ab91444cf75349f6463e5d Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:33:26 +0000 Subject: [PATCH 5/9] fix(review): the SVG drawing check reads every name= without consuming its value, so a stray quote cannot hide an external href (GST-911 review pass 2) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 5 +++-- test/review-packets-removed.test.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 5044c3b0a..7acf15c43 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -92,7 +92,8 @@ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+ /** * Whether an SVG's whole text is a drawing (GST-911): every element in SVG_ELEMENTS, no event handler, every href or src a * same-document reference or an inline raster image, and nothing SVG_REFUSED names, read after character references are - * decoded. Anything else, an SVG that can run code included, stays code. + * decoded. Every name= in the text is read, so a stray quote cannot hide a later attribute inside its value; an unquoted value + * stops at the next =, which keeps the scan linear. Anything else, an SVG that can run code included, stays code. */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); @@ -102,7 +103,7 @@ export function svgDrawing(text: string): boolean { const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); if (!tag || !SVG_ELEMENTS.has(tag[1]!.toLowerCase())) return false; } - for (const m of t.matchAll(/(?]+))/g)) { + for (const m of t.matchAll(/(?=]+)))/g)) { const local = m[1]!.slice(m[1]!.lastIndexOf(':') + 1).toLowerCase(); if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(m[2] ?? m[3] ?? m[4] ?? ''))) return false; } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index d84691081..9019a9b01 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -79,7 +79,7 @@ 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: 'd3b269015a8d97b4cf5654c0dacc5bea7562b96e3c1c3c6fc9442ef4bb336637', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: 'ff4c888f75ff9b8b898a96060eb758d1aad836f2cecdbe13d2408ab406a6f432', replay: 'GST-911' }, 'lib/review-risk.ts': { sha: 'df618e8aef3d641e8bf2073d8dca170740434acbb69c04555c4166df047f566c', replay: 'GST-911' }, }; From 25563a8e5b2386449daab7e79169d5b2e983e592 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:49:55 +0000 Subject: [PATCH 6/9] test(review): CSS in a drawing SVG holds only declarations and url(#id): image-set(), image(), element() keep it code (GST-911 Step 11, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-planner-misreads.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index 15bda38b1..4e0f51743 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -125,7 +125,8 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se // Value: protects=only an SVG made of drawing elements is data: anything that loads, links or declares outside that set stays code; // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs, // including through a CSS escape or a zero-padded character reference (review pass 1 findings) or behind a stray quote that - // pairs with a later one (review pass 2 finding); seam=collectCommit + depthOfCommitInput + // pairs with a later one (review pass 2 finding), or any CSS beyond declarations and url(#id) (Step 11 finding: image-set()); + // seam=collectCommit + depthOfCommitInput test.each([ ['an external image', ' '], ['a CSS import', ' '], @@ -141,6 +142,13 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['an external use after a stray quote in a comment', " "], ['an external xlink:href after a stray quote in a desc', " z=''"], ['an external image after a stray href in text', " href='#'"], + ['a CSS image-set() in a style element', ' '], + ['a CSS image-set() in a style attribute', " "], + ['a CSS image() with a string', ' '], + ['a CSS element() reference', ' '], + ['a CSS import of a bare string', ' '], + ['a style element import', ' '], + ['a style attribute cross-fade()', ' '], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); From 0f61362c1e4603062e0c5db3c8f1625001b622f8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:50:44 +0000 Subject: [PATCH 7/9] fix(review): a drawing SVG's CSS holds declarations only, no @ rule and no function but url(#id), which closes image-set(), image(), element() and every other CSS load (GST-911 Step 11) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-911-planner-misreads.md | 2 +- docs/review-policy.md | 5 +++-- lib/review-depth.ts | 13 ++++++++++--- test/review-packets-removed.test.ts | 2 +- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/changelog.d/gst-911-planner-misreads.md b/changelog.d/gst-911-planner-misreads.md index b3944c884..5bf07a9d7 100644 --- a/changelog.d/gst-911-planner-misreads.md +++ b/changelog.d/gst-911-planner-misreads.md @@ -5,4 +5,4 @@ ### Tests -- `test/review-planner-misreads.test.ts`: a 600-point matplotlib-style chart plans docs, light, runtime 0 (before: code, 636 runtime lines); six ways an SVG can run script keep it code with the security class, and six other non-drawing constructs keep it code; session data files leave `SCOPE_AUTH` false while session code and config set it; a stale local `main` and tags named `main` or `origin/main` leave the scope and the receipt base unchanged. +- `test/review-planner-misreads.test.ts`: a 600-point matplotlib-style chart plans docs, light, runtime 0 (before: code, 636 runtime lines); six ways an SVG can run script keep it code with the security class, and other non-drawing constructs (external images, any CSS beyond declarations and `url(#id)`, animations, entity declarations) keep it code; session data files leave `SCOPE_AUTH` false while session code and config set it; a stale local `main` and tags named `main` or `origin/main` leave the scope and the receipt base unchanged. diff --git a/docs/review-policy.md b/docs/review-policy.md index be7396557..92b003634 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -183,8 +183,9 @@ directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named consumer each keep the path code, and YAML and TOML are always code. An SVG counts as data only when its whole text at the head is a drawing (GST-911): SVG drawing -elements and RDF metadata, CSS with no import, no `url()` outside the file and no -backslash escape, every +elements and RDF metadata, CSS made of declarations only (no `@` rule and no +function but a same-file `url(#id)`, which closes `@import`, `image-set()`, +`image()`, `element()` and every other CSS load), no backslash escape, every `href` a same-file `#` reference or an inline raster image, no event handler and no entity declaration, read after character references are decoded; anything else, a script or `foreignObject` included, keeps it code. More than diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 7acf15c43..16ff3a3b2 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -86,18 +86,24 @@ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg\b[^>[]*>)/i; * import, a url() to another document, a CSS expression or binding. */ const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; +/** A drawing's CSS, a style element's text or a style attribute's value, holds declarations only: no @ rule and no function but url(#id). */ +const SVG_STYLE = /<(?:[A-Za-z_][\w.-]*:)?style\b[^>]*>([\s\S]*?)(?:<\/(?:[A-Za-z_][\w.-]*:)?style\s*>|$)/gi; +const SVG_CSS_REF = /url\(\s*(['"]?)#[\w.:-]*\1\s*\)/gi; +const SVG_CSS_REFUSED = /[@(]/; /** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+/=\s]*$)/i; /** * Whether an SVG's whole text is a drawing (GST-911): every element in SVG_ELEMENTS, no event handler, every href or src a - * same-document reference or an inline raster image, and nothing SVG_REFUSED names, read after character references are + * same-document reference or an inline raster image, CSS of declarations only (SVG_STYLE), and nothing SVG_REFUSED names, read after character references are * decoded. Every name= in the text is read, so a stray quote cannot hide a later attribute inside its value; an unquoted value * stops at the next =, which keeps the scan linear. Anything else, an SVG that can run code included, stays code. */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); if (SVG_REFUSED.some(r => r.test(t)) || svgActive(t)) return false; + const cssDrawing = (css: string) => !SVG_CSS_REFUSED.test(css.replace(SVG_CSS_REF, '')); + for (const m of t.matchAll(SVG_STYLE)) if (!cssDrawing(m[1]!)) return false; for (let at = t.indexOf('<'); at !== -1; at = t.indexOf('<', at + 1)) { if (SVG_MARKUP.test(t.slice(at, at + 512))) continue; const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); @@ -105,7 +111,8 @@ export function svgDrawing(text: string): boolean { } for (const m of t.matchAll(/(?=]+)))/g)) { const local = m[1]!.slice(m[1]!.lastIndexOf(':') + 1).toLowerCase(); - if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(m[2] ?? m[3] ?? m[4] ?? ''))) return false; + const value = m[2] ?? m[3] ?? m[4] ?? ''; + if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(value)) || (local === 'style' && !cssDrawing(value))) return false; } return true; } @@ -113,7 +120,7 @@ export function svgDrawing(text: string): boolean { /** A hash of the constants and patterns that decide a depth, not of this file's bytes: a comment edit keeps it, a threshold edit changes it. */ export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ RISK_RULES, SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, BATCH_SCOPED, SCOPED_RANGE_LINES, ROLE_EFFORT, SVG_ELEMENTS: [...SVG_ELEMENTS], - patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), + patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF], SVG_STYLE: [SVG_STYLE], SVG_CSS_REF: [SVG_CSS_REF], SVG_CSS_REFUSED: [SVG_CSS_REFUSED] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); /** diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 9019a9b01..b02be6c3b 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -79,7 +79,7 @@ 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: 'ff4c888f75ff9b8b898a96060eb758d1aad836f2cecdbe13d2408ab406a6f432', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: '7c4e2673d6d7613a94a493665bd566e6c34958bb0b72374e1552f2223a1da2bd', replay: 'GST-911' }, 'lib/review-risk.ts': { sha: 'df618e8aef3d641e8bf2073d8dca170740434acbb69c04555c4166df047f566c', replay: 'GST-911' }, }; From 6ad88b7feff866d195ef3cc9b4a69f4bf8b9fe0f Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 05:26:18 +0000 Subject: [PATCH 8/9] test(review): a style element's CSS is read past comments and CDATA, unclosed style tags are refused in linear time, and every drawing verdict holds at 1 MB and 8 MB (GST-911, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-planner-misreads.test.ts | 34 +++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index 4e0f51743..80873fe3e 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -11,7 +11,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { collectCommit, collectLive } from '../lib/review-collect'; -import { depthOfCommitInput } from '../lib/review-depth'; +import { depthOfCommitInput, svgDrawing } from '../lib/review-depth'; import { targetedClasses } from '../lib/review-targeted'; import { receiptDepthAtHead, receiptDepthVerdict } from '../lib/ship-receipt-depth'; @@ -149,6 +149,8 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['a CSS import of a bare string', ' '], ['a style element import', ' '], ['a style attribute cross-fade()', ' '], + ['a style element whose hides in a comment', ' -->image-set("https://x.test/p.png" 1x)}'], + ['a style element whose hides in CDATA', ' */ svg{fill:image-set("https://x.test/p.png" 1x)} ]]>'], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); @@ -166,6 +168,36 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se expect(named.class).toBe('code'); expect(named.runtime).toBeGreaterThan(3); }, 60_000); + + // Value: protects=the drawing check reads a file up to the collector's 8 MB cap in linear time; fails_when=a run of unclosed + // style tags (480 KB) takes over 100 ms or reads as a drawing (review pass notes on #3838); seam=svgDrawing + test('a 480 KB run of unclosed ' + fill('') + '')).toBe(true); + expect(svgDrawing(H + '')).toBe(true); + for (const svg of [ + H + fill('') + '', + H + '', + H + '', + H + ' */ ' + fill('rect{fill:#000} ') + 'svg{fill:' + bad + '} ]]>', + H + ' -->svg{fill:' + bad + '}', + H + '', + H + "z='" + fill('abc ') + "'", + H + fill(' { From 26d71ad689dbf9d9e740919f81cb7d2826b99fe9 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 05:27:20 +0000 Subject: [PATCH 9/9] fix(review): walk a style element's CSS with indexOf, dropping comments and reading CDATA, so no hidden ends it early and unclosed style tags refuse in linear time (GST-911) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 38 +++++++++++++++++++++++++---- test/review-packets-removed.test.ts | 2 +- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 16ff3a3b2..e1c2b00ac 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -87,7 +87,8 @@ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg\b[^>[]*>)/i; */ const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; /** A drawing's CSS, a style element's text or a style attribute's value, holds declarations only: no @ rule and no function but url(#id). */ -const SVG_STYLE = /<(?:[A-Za-z_][\w.-]*:)?style\b[^>]*>([\s\S]*?)(?:<\/(?:[A-Za-z_][\w.-]*:)?style\s*>|$)/gi; +const SVG_STYLE_OPEN = /<(?:[A-Za-z_][\w.-]*:)?style\b/gi; +const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style\s*>/i; const SVG_CSS_REF = /url\(\s*(['"]?)#[\w.:-]*\1\s*\)/gi; const SVG_CSS_REFUSED = /[@(]/; /** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ @@ -95,15 +96,42 @@ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+ /** * Whether an SVG's whole text is a drawing (GST-911): every element in SVG_ELEMENTS, no event handler, every href or src a - * same-document reference or an inline raster image, CSS of declarations only (SVG_STYLE), and nothing SVG_REFUSED names, read after character references are + * same-document reference or an inline raster image, CSS of declarations only, and nothing SVG_REFUSED names, read after character references are * decoded. Every name= in the text is read, so a stray quote cannot hide a later attribute inside its value; an unquoted value - * stops at the next =, which keeps the scan linear. Anything else, an SVG that can run code included, stays code. + * stops at the next =, which keeps the scan linear. A style element's CSS is walked with indexOf to its closing tag, dropping + * comments and reading CDATA as CSS, so neither can hide a closing tag; any other markup inside it, or a style element that never + * closes, refuses. Anything else, an SVG that can run code included, stays code. */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); if (SVG_REFUSED.some(r => r.test(t)) || svgActive(t)) return false; const cssDrawing = (css: string) => !SVG_CSS_REFUSED.test(css.replace(SVG_CSS_REF, '')); - for (const m of t.matchAll(SVG_STYLE)) if (!cssDrawing(m[1]!)) return false; + for (let at = 0; ;) { + SVG_STYLE_OPEN.lastIndex = at; + const open = SVG_STYLE_OPEN.exec(t); + if (!open) break; + let i = t.indexOf('>', open.index) + 1; + if (!i) return false; + let css = ''; + for (;;) { + const lt = t.indexOf('<', i); + if (lt === -1) return false; + css += t.slice(i, lt); + if (t.startsWith('', lt + 4) + 3; + if (i === 2) return false; + } else if (t.startsWith('', lt + 9); + if (end === -1) return false; + css += t.slice(lt + 9, end); + i = end + 3; + } else if (SVG_STYLE_CLOSE.test(t.slice(lt, lt + 256))) { + at = lt + 1; + break; + } else return false; + } + if (!cssDrawing(css)) return false; + } for (let at = t.indexOf('<'); at !== -1; at = t.indexOf('<', at + 1)) { if (SVG_MARKUP.test(t.slice(at, at + 512))) continue; const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); @@ -120,7 +148,7 @@ export function svgDrawing(text: string): boolean { /** A hash of the constants and patterns that decide a depth, not of this file's bytes: a comment edit keeps it, a threshold edit changes it. */ export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ RISK_RULES, SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, BATCH_SCOPED, SCOPED_RANGE_LINES, ROLE_EFFORT, SVG_ELEMENTS: [...SVG_ELEMENTS], - patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF], SVG_STYLE: [SVG_STYLE], SVG_CSS_REF: [SVG_CSS_REF], SVG_CSS_REFUSED: [SVG_CSS_REFUSED] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), + patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF], SVG_STYLE_OPEN: [SVG_STYLE_OPEN], SVG_STYLE_CLOSE: [SVG_STYLE_CLOSE], SVG_CSS_REF: [SVG_CSS_REF], SVG_CSS_REFUSED: [SVG_CSS_REFUSED] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); /** diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index b02be6c3b..e16cb6361 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -79,7 +79,7 @@ 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: '7c4e2673d6d7613a94a493665bd566e6c34958bb0b72374e1552f2223a1da2bd', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: 'b0c86b19ed13959c1cda8c1d2a2066b3b899473b855f06c0d9680e66660fb698', replay: 'GST-911' }, 'lib/review-risk.ts': { sha: 'df618e8aef3d641e8bf2073d8dca170740434acbb69c04555c4166df047f566c', replay: 'GST-911' }, };