From 4aa052350b9e26ac990a6eda1a86675e75b9738b Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:30:53 +0000 Subject: [PATCH 01/35] fix(gate-diff): read a bare base as origin/ in gate-diff and diff-scope (#3650) gstack-gate-diff and gstack-diff-scope resolved a bare base such as main to the local branch whenever it shared history with HEAD. A local main that is behind origin/main (a linked worktree, or a Capy clone, where local main never moves) then set the merge base, so /ship's gate scan and specialist scope also read commits already upstream. Both now prefer origin/ when it resolves, the base /ship fetches and merges against. Fixes #3650 Co-authored-by: exGeni <135786749+exGeni@users.noreply.github.com> Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 7 +++++ bin/gstack-gate-diff | 2 +- changelog.d/gsta246-gate-diff-origin-base.md | 1 + lib/gate-diff/git.ts | 27 ++++++++++---------- test/diff-scope.test.ts | 19 ++++++++++++++ test/gate-diff-cli.test.ts | 26 +++++++++++++++++++ 6 files changed, 68 insertions(+), 14 deletions(-) create mode 100644 changelog.d/gsta246-gate-diff-origin-base.md diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index 3ec81cf09..83f7ba620 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -34,6 +34,13 @@ _default_base() { echo "main" } BASE="${1:-$(_default_base)}" +# A bare branch name reads its remote-tracking twin when there is one: /ship +# merges against origin/, and a linked worktree or a Capy clone never +# moves the local branch, so a stale one widens the scope (#3650). +case "$BASE" in + */*|*[~^:@]*) ;; + *) git rev-parse --verify -q "refs/remotes/origin/${BASE}^{commit}" >/dev/null 2>&1 && BASE="origin/${BASE}" ;; +esac HEAD_REF="" if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi diff --git a/bin/gstack-gate-diff b/bin/gstack-gate-diff index 0a1af715c..d09b9bf7c 100755 --- a/bin/gstack-gate-diff +++ b/bin/gstack-gate-diff @@ -148,7 +148,7 @@ function main(argv: string[]): number { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); return 2; } - if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : `no local branch ${baseRef}`}; base is ${base.ref}`); + if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : base.local === 'differs' ? `local ${baseRef} is not ${base.ref}` : `no local branch ${baseRef}`}; base is ${base.ref}`); const mb = mergeBase(base.sha, head, root); if (!mb) { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); diff --git a/changelog.d/gsta246-gate-diff-origin-base.md b/changelog.d/gsta246-gate-diff-origin-base.md new file mode 100644 index 000000000..7f68b21e3 --- /dev/null +++ b/changelog.d/gsta246-gate-diff-origin-base.md @@ -0,0 +1 @@ +- `gstack-gate-diff` and `gstack-diff-scope` now read a bare base name such as `main` as `origin/main` when that ref exists, the base /ship fetches and merges against. Before, a local `main` that was behind `origin/main` (a linked worktree, or a Capy clone, where local `main` never moves) set the merge base, so the gate scan and the specialist scope also read commits already on `origin/main`. One /ship run in #3650 listed a `.github/workflows/ci.yml` hunk from upstream and dispatched an extra performance reviewer. `gstack-gate-diff` says so on stderr (`local main is not origin/main; base is origin/main`). An explicit `origin/main` or commit is unchanged (#3650). diff --git a/lib/gate-diff/git.ts b/lib/gate-diff/git.ts index a9d4f07c0..ee60115b9 100644 --- a/lib/gate-diff/git.ts +++ b/lib/gate-diff/git.ts @@ -85,22 +85,23 @@ export function isShallow(cwd: string): boolean { /** * The base as a ref that resolves here and, when `head` is given, shares - * history with it: the ref itself, else its remote-tracking branch - * (`origin/main` for `main`). A clone that checked out only the feature - * branch has no local `main`, and the merge-base against `origin/main` is the - * same commit; a stale local `main` that shares no history with HEAD (a - * fetched-over clone) yields to the twin too, so the fix line's fetch into the - * tracking ref is a fix and not a loop. + * history with it: for a branch name its remote-tracking branch first + * (`origin/main` for `main`), the base /ship merges against, else the ref + * itself. A local `main` that is missing, behind (a linked worktree or a Capy + * clone never moves it, #3650) or shares no history with HEAD (a fetched-over + * clone) yields to the twin, so the fix line's fetch into the tracking ref is + * a fix and not a loop. */ -export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' } | null { +export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'differs' } | null { const related = (sha: string) => !head || mergeBase(sha, head, cwd) !== null; const direct = resolvesToCommit(ref, cwd); - if (direct && related(direct)) return { ref, sha: direct }; - if (!isBranchName(ref)) return direct ? { ref, sha: direct } : null; - for (const remote of remoteNames(cwd)) { - const tracking = `${remote}/${ref}`; - const sha = resolvesToCommit(tracking, cwd); - if (sha && related(sha)) return { ref: tracking, sha, local: direct ? 'unrelated' : 'missing' }; + if (isBranchName(ref)) { + for (const remote of remoteNames(cwd)) { + const tracking = `${remote}/${ref}`; + const sha = resolvesToCommit(tracking, cwd); + if (!sha || !related(sha)) continue; + return sha === direct ? { ref: tracking, sha } : { ref: tracking, sha, local: !direct ? 'missing' : related(direct) ? 'differs' : 'unrelated' }; + } } return direct ? { ref, sha: direct } : null; } diff --git a/test/diff-scope.test.ts b/test/diff-scope.test.ts index e33469c1a..67c01b06a 100644 --- a/test/diff-scope.test.ts +++ b/test/diff-scope.test.ts @@ -330,6 +330,25 @@ describe('exit-code contract (#2526)', () => { }); }); +describe('a bare base name reads origin/ when it exists (#3650)', () => { + test('a local main behind origin/main does not widen the scope to upstream commits', () => { + const dir = createRepo(['README2.md']); + const git = (...args: string[]) => spawnSync('git', args, { cwd: dir, stdio: 'pipe', timeout: 5000 }).stdout.toString().trim(); + const seed = git('rev-parse', 'main'); + git('checkout', '-q', 'main'); + writeFileSync(join(dir, 'service.py'), 'print(1)\n'); + git('add', '.'); + git('commit', '-qm', 'upstream backend change'); + git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + git('checkout', '-q', 'feature/test'); + git('rebase', '-q', 'origin/main'); + git('branch', '-f', 'main', seed); + expect(git('diff', '--name-only', 'main...HEAD').split('\n').sort()).toEqual(['README2.md', 'service.py']); + const scope = runScope(dir); + expect([scope.SCOPE_BACKEND, scope.SCOPE_DOCS]).toEqual(['false', 'true']); + }); +}); + describe('uncommitted work is visible (#2299)', () => { test('uncommitted change on a branch with no commits sets the scope', () => { const dir = mkdtempSync(join(tmpdir(), 'diff-scope-dirty-')); diff --git a/test/gate-diff-cli.test.ts b/test/gate-diff-cli.test.ts index 3a8545e09..ce8aace18 100644 --- a/test/gate-diff-cli.test.ts +++ b/test/gate-diff-cli.test.ts @@ -153,6 +153,32 @@ describe('gstack-gate-diff arguments and exit contract', () => { // fetches from it; fails_when=remoteNames or resolveBase hard-code origin, or noBaseFix falls back to the literal 'origin' // while a real remote exists; why_new=every existing fixture has either an origin remote or none; seam=none; // no_claim=nothing about the order when origin and another remote both carry the branch. + // Value: protects=a bare base name scans from the same merge base /ship merges against (origin/) when the local + // branch is related but behind (#3650); fails_when=resolveBase prefers any related local branch, so a stale local main + // widens the scan to commits already upstream; why_new=the stale-main test above covers only unrelated history; + // seam=none; no_claim=nothing about a local base that is ahead of its remote-tracking twin. + test('a related local main that is behind origin/main yields to origin/main (#3650)', () => { + const src = repo({ fixture: 'rh1-skip', commit: true }); + const seed = gitIn(src, ['rev-parse', 'main']).trim(); + gitIn(src, ['checkout', '-q', 'main']); + fs.mkdirSync(path.join(src, '.github', 'workflows'), { recursive: true }); + fs.writeFileSync(path.join(src, '.github', 'workflows', 'ci.yml'), 'on: push\njobs: {}\n'); + gitIn(src, ['add', '-A']); + gitIn(src, ['commit', '-q', '-m', 'upstream ci change']); + gitIn(src, ['checkout', '-q', 'feature']); + gitIn(src, ['rebase', '-q', 'main']); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-gate-diff-behind-')); + repos.push(dir); + gitIn(dir, ['clone', '-q', '--branch', 'feature', `file://${src}`, '.']); + gitIn(dir, ['branch', '-f', 'main', seed]); + const bare = run(dir, ['main']); + const remote = run(dir, ['origin/main']); + expect([bare.status, remote.status]).toEqual([0, 0]); + expect(summary(bare.stdout).candidate).toBe(summary(remote.stdout).candidate); + expect(bare.stdout).toBe(remote.stdout); + expect(bare.stderr).toContain('gstack-gate-diff: local main is not origin/main; base is origin/main'); + }); + test('a clone whose only remote is upstream resolves the base to upstream/main, and the fix line fetches from upstream', () => { const src = repo({ fixture: 'rh1-skip', commit: true }); const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-gate-diff-upstream-')); From 87b7c922e07e7371185787b58c792b980d898ac8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:45:45 +0000 Subject: [PATCH 02/35] fix: pre-landing review fixes (diff-scope reads slash-named bases through origin too) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 8 ++++---- test/diff-scope.test.ts | 27 +++++++++++++++++++++------ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index 83f7ba620..d481cbb8a 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -37,10 +37,10 @@ BASE="${1:-$(_default_base)}" # A bare branch name reads its remote-tracking twin when there is one: /ship # merges against origin/, and a linked worktree or a Capy clone never # moves the local branch, so a stale one widens the scope (#3650). -case "$BASE" in - */*|*[~^:@]*) ;; - *) git rev-parse --verify -q "refs/remotes/origin/${BASE}^{commit}" >/dev/null 2>&1 && BASE="origin/${BASE}" ;; -esac +_NOT_BRANCH='^[0-9a-f]{7,64}$|[~^]|@\{|^[A-Z_]*HEAD$|^refs/' +if [[ ! "$BASE" =~ $_NOT_BRANCH ]] && git rev-parse --verify -q "refs/remotes/origin/${BASE}^{commit}" >/dev/null 2>&1; then + BASE="origin/${BASE}" +fi HEAD_REF="" if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi diff --git a/test/diff-scope.test.ts b/test/diff-scope.test.ts index 67c01b06a..f9d31df84 100644 --- a/test/diff-scope.test.ts +++ b/test/diff-scope.test.ts @@ -331,22 +331,37 @@ describe('exit-code contract (#2526)', () => { }); describe('a bare base name reads origin/ when it exists (#3650)', () => { - test('a local main behind origin/main does not widen the scope to upstream commits', () => { + function behindBase(base: string) { const dir = createRepo(['README2.md']); const git = (...args: string[]) => spawnSync('git', args, { cwd: dir, stdio: 'pipe', timeout: 5000 }).stdout.toString().trim(); const seed = git('rev-parse', 'main'); - git('checkout', '-q', 'main'); + git('checkout', '-q', '-B', base, 'main'); writeFileSync(join(dir, 'service.py'), 'print(1)\n'); git('add', '.'); git('commit', '-qm', 'upstream backend change'); - git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + git('update-ref', `refs/remotes/origin/${base}`, 'HEAD'); git('checkout', '-q', 'feature/test'); - git('rebase', '-q', 'origin/main'); - git('branch', '-f', 'main', seed); - expect(git('diff', '--name-only', 'main...HEAD').split('\n').sort()).toEqual(['README2.md', 'service.py']); + git('rebase', '-q', `origin/${base}`); + git('branch', '-f', base, seed); + expect(git('diff', '--name-only', `${base}...HEAD`).split('\n').sort()).toEqual(['README2.md', 'service.py']); + return { dir, git }; + } + const scopeAt = (dir: string, base: string) => Object.fromEntries(spawnSync('bash', [SCRIPT, base], { cwd: dir, stdio: 'pipe', timeout: 5000 }).stdout.toString().trim().split('\n').map(l => l.split('='))); + + test('a local main behind origin/main does not widen the scope to upstream commits', () => { + const { dir } = behindBase('main'); const scope = runScope(dir); expect([scope.SCOPE_BACKEND, scope.SCOPE_DOCS]).toEqual(['false', 'true']); }); + + test('a slash-named base reads its remote-tracking twin too, and HEAD or a sha stays as given', () => { + const { dir, git } = behindBase('release/x'); + expect(scopeAt(dir, 'release/x').SCOPE_BACKEND).toBe('false'); + git('update-ref', 'refs/remotes/origin/HEAD', git('rev-parse', 'release/x')); + expect(scopeAt(dir, 'HEAD').SCOPE_DOCS).toBe('false'); + git('update-ref', `refs/remotes/origin/${git('rev-parse', 'release/x')}`, 'HEAD'); + expect(scopeAt(dir, git('rev-parse', 'release/x')).SCOPE_BACKEND).toBe('true'); + }); }); describe('uncommitted work is visible (#2299)', () => { From 2a1f123d9eec9719a5aab475cc2fe7b896bbedba Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:54:11 +0000 Subject: [PATCH 03/35] fix: Step 11 review fixes (a fork branch, a colliding tag or another remote never replaces the base) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 21 ++++++++++++++------- lib/gate-diff/git.ts | 34 +++++++++++++++++++++------------- test/diff-scope.test.ts | 18 ++++++++++++++++++ test/gate-diff-cli.test.ts | 27 +++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 20 deletions(-) diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index d481cbb8a..6bfef3252 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -34,15 +34,22 @@ _default_base() { echo "main" } BASE="${1:-$(_default_base)}" -# A bare branch name reads its remote-tracking twin when there is one: /ship -# merges against origin/, and a linked worktree or a Capy clone never -# moves the local branch, so a stale one widens the scope (#3650). -_NOT_BRANCH='^[0-9a-f]{7,64}$|[~^]|@\{|^[A-Z_]*HEAD$|^refs/' -if [[ ! "$BASE" =~ $_NOT_BRANCH ]] && git rev-parse --verify -q "refs/remotes/origin/${BASE}^{commit}" >/dev/null 2>&1; then - BASE="origin/${BASE}" -fi HEAD_REF="" if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi +# A bare branch name reads origin's tracking branch, by its full ref, when that +# shares history with HEAD: /ship merges against origin/, and a linked +# worktree or a Capy clone never moves the local branch, so a stale one widens +# the scope (#3650). A remote-qualified name is taken as given. +_NOT_BRANCH='^[0-9a-f]{7,64}$|[~^]|@\{|^[A-Z_]*HEAD$|^refs/' +_QUALIFIED=false +for _remote in $(git remote 2>/dev/null); do + case "$BASE" in "$_remote"/*) _QUALIFIED=true ;; esac +done +if [[ ! "$BASE" =~ $_NOT_BRANCH ]] && [ "$_QUALIFIED" = false ] \ + && git rev-parse --verify -q "refs/remotes/origin/${BASE}^{commit}" >/dev/null 2>&1 \ + && git merge-base "refs/remotes/origin/${BASE}" "${HEAD_REF:-HEAD}" >/dev/null 2>&1; then + BASE="refs/remotes/origin/${BASE}" +fi FRONTEND=false BACKEND=false diff --git a/lib/gate-diff/git.ts b/lib/gate-diff/git.ts index ee60115b9..cac2f298d 100644 --- a/lib/gate-diff/git.ts +++ b/lib/gate-diff/git.ts @@ -85,23 +85,31 @@ export function isShallow(cwd: string): boolean { /** * The base as a ref that resolves here and, when `head` is given, shares - * history with it: for a branch name its remote-tracking branch first - * (`origin/main` for `main`), the base /ship merges against, else the ref - * itself. A local `main` that is missing, behind (a linked worktree or a Capy - * clone never moves it, #3650) or shares no history with HEAD (a fetched-over - * clone) yields to the twin, so the fix line's fetch into the tracking ref is - * a fix and not a loop. + * history with it. For a bare branch name that is origin's tracking branch + * (`refs/remotes/origin/main` for `main`), the base /ship merges against, + * else the related local branch, else another remote's tracking branch. A + * local `main` that is behind (a linked worktree or a Capy clone never moves + * it, #3650), missing or unrelated to HEAD (a fetched-over clone) yields to the + * twin, so the fix line's fetch into the tracking ref is a fix and not a loop. + * A remote-qualified name (`upstream/main`) is taken as given, and the twin is + * read by its full ref, so a fork's branch or a tag cannot stand in for it. */ export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'differs' } | null { const related = (sha: string) => !head || mergeBase(sha, head, cwd) !== null; const direct = resolvesToCommit(ref, cwd); - if (isBranchName(ref)) { - for (const remote of remoteNames(cwd)) { - const tracking = `${remote}/${ref}`; - const sha = resolvesToCommit(tracking, cwd); - if (!sha || !related(sha)) continue; - return sha === direct ? { ref: tracking, sha } : { ref: tracking, sha, local: !direct ? 'missing' : related(direct) ? 'differs' : 'unrelated' }; - } + const remotes = remoteNames(cwd); + if (!isBranchName(ref) || remotes.some(remote => ref.startsWith(`${remote}/`))) return direct ? { ref, sha: direct } : null; + const twin = (remote: string) => { + const sha = resolvesToCommit(`refs/remotes/${remote}/${ref}`, cwd); + if (!sha || !related(sha)) return null; + return sha === direct ? { ref: `${remote}/${ref}`, sha } : { ref: `${remote}/${ref}`, sha, local: !direct ? 'missing' as const : related(direct) ? 'differs' as const : 'unrelated' as const }; + }; + const origin = remotes.includes('origin') ? twin('origin') : null; + if (origin) return origin; + if (direct && related(direct)) return { ref, sha: direct }; + for (const remote of remotes) { + const other = twin(remote); + if (other) return other; } return direct ? { ref, sha: direct } : null; } diff --git a/test/diff-scope.test.ts b/test/diff-scope.test.ts index f9d31df84..05192b07e 100644 --- a/test/diff-scope.test.ts +++ b/test/diff-scope.test.ts @@ -362,6 +362,24 @@ describe('a bare base name reads origin/ when it exists (#3650)', () => { git('update-ref', `refs/remotes/origin/${git('rev-parse', 'release/x')}`, 'HEAD'); expect(scopeAt(dir, git('rev-parse', 'release/x')).SCOPE_BACKEND).toBe('true'); }); + + test('a colliding tag, a fork branch over an explicit remote base, or an unrelated twin never replaces the base (#3650 review)', () => { + const { dir, git } = behindBase('main'); + const upstreamCommit = git('rev-parse', 'origin/main'); + git('tag', 'origin/main', 'HEAD'); + expect([scopeAt(dir, 'main').SCOPE_BACKEND, scopeAt(dir, 'main').SCOPE_DOCS]).toEqual(['false', 'true']); + git('remote', 'add', 'upstream', 'file:///nonexistent-upstream'); + git('update-ref', 'refs/remotes/upstream/main', git('rev-parse', 'main')); + git('update-ref', 'refs/remotes/origin/upstream/main', 'HEAD'); + expect(scopeAt(dir, 'upstream/main').SCOPE_BACKEND).toBe('true'); + git('tag', '-d', 'origin/main'); + git('checkout', '-q', '--orphan', 'island'); + git('commit', '-q', '--allow-empty', '-m', 'unrelated'); + git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + git('checkout', '-q', 'feature/test'); + expect(scopeAt(dir, 'main').SCOPE_BACKEND).toBe('true'); + expect(upstreamCommit).not.toBe(''); + }); }); describe('uncommitted work is visible (#2299)', () => { diff --git a/test/gate-diff-cli.test.ts b/test/gate-diff-cli.test.ts index ce8aace18..315bfccda 100644 --- a/test/gate-diff-cli.test.ts +++ b/test/gate-diff-cli.test.ts @@ -179,6 +179,33 @@ describe('gstack-gate-diff arguments and exit contract', () => { expect(bare.stderr).toContain('gstack-gate-diff: local main is not origin/main; base is origin/main'); }); + // Value: protects=a ref a contributor controls (a fork's origin/upstream/main branch, a tag named origin/main, another + // remote's main) never replaces the base the caller named or a related local base, so it cannot hide a gate edit; + // fails_when=resolveBase rewrites remote-qualified names, resolves the twin by its short name, or lets any remote beat a + // related local base; why_new=the cases above have one remote and no colliding refs; seam=none; no_claim=nothing about + // a remote's HEAD symref. + test('a fork branch, a colliding tag or another remote never replaces the base (#3650 review)', () => { + const src = repo({ fixture: 'rh1-skip', commit: true }); + const seed = gitIn(src, ['rev-parse', 'main']).trim(); + const clone = (remote: string) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-gate-diff-refs-')); + repos.push(dir); + gitIn(dir, ['clone', '-q', '-o', remote, '--branch', 'feature', `file://${src}`, '.']); + return dir; + }; + const named = clone('upstream'); + gitIn(named, ['remote', 'add', 'origin', 'file:///nonexistent-fork']); + gitIn(named, ['update-ref', 'refs/remotes/origin/upstream/main', 'HEAD']); + expect(summary(run(named, ['upstream/main']).stdout).tagged).toBe('RH-1:1'); + const tagged = clone('origin'); + gitIn(tagged, ['tag', 'origin/main', 'HEAD']); + expect(summary(run(tagged, ['main']).stdout).tagged).toBe('RH-1:1'); + const fork = clone('zfork'); + gitIn(fork, ['branch', '-f', 'main', seed]); + gitIn(fork, ['update-ref', 'refs/remotes/zfork/main', 'HEAD']); + expect(summary(run(fork, ['main']).stdout).tagged).toBe('RH-1:1'); + }); + test('a clone whose only remote is upstream resolves the base to upstream/main, and the fix line fetches from upstream', () => { const src = repo({ fixture: 'rh1-skip', commit: true }); const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-gate-diff-upstream-')); From 3675e472744b9b7d2848798fb55a0737447dbd89 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:58:14 +0000 Subject: [PATCH 04/35] docs(changelog): describe the fork, tag and remote guards (#3650) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gsta246-gate-diff-origin-base.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/gsta246-gate-diff-origin-base.md b/changelog.d/gsta246-gate-diff-origin-base.md index 7f68b21e3..9ee4db95d 100644 --- a/changelog.d/gsta246-gate-diff-origin-base.md +++ b/changelog.d/gsta246-gate-diff-origin-base.md @@ -1 +1 @@ -- `gstack-gate-diff` and `gstack-diff-scope` now read a bare base name such as `main` as `origin/main` when that ref exists, the base /ship fetches and merges against. Before, a local `main` that was behind `origin/main` (a linked worktree, or a Capy clone, where local `main` never moves) set the merge base, so the gate scan and the specialist scope also read commits already on `origin/main`. One /ship run in #3650 listed a `.github/workflows/ci.yml` hunk from upstream and dispatched an extra performance reviewer. `gstack-gate-diff` says so on stderr (`local main is not origin/main; base is origin/main`). An explicit `origin/main` or commit is unchanged (#3650). +- `gstack-gate-diff` and `gstack-diff-scope` now read a bare base name such as `main` as origin's tracking branch (`refs/remotes/origin/main`) when it exists and shares history with HEAD. That's the base /ship fetches and merges against. Before, a local `main` that was behind `origin/main` set the merge base. That happens in a linked worktree, and on a Capy machine, where local `main` never moves. So the gate scan and the specialist scope also read commits already on `origin/main`. One /ship run in #3650 listed a `.github/workflows/ci.yml` hunk from upstream and dispatched an extra performance reviewer. `gstack-gate-diff` now says `local main is not origin/main; base is origin/main` on stderr when that happens. A base you name with its remote (`upstream/main`), a commit or `HEAD` is used as given. A tag named `origin/main` or a fork's `origin/upstream/main` branch can't stand in for the base, and another remote's branch only replaces a local base that is missing or unrelated (#3650). From 56ced9d1c126e89df3962ba385991fea55efe5f2 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:16:58 +0000 Subject: [PATCH 05/35] fix(retro): keep retro history in the state root, shared across worktrees (#3749) /retro saved snapshots under .context/retros inside the working tree, so a run in a linked worktree saw none of the main checkout's history, reported "First retro recorded", and wrote a snapshot that git worktree remove deletes. gstack-retro-metrics --history [--next ] now resolves the history directory as /projects//retros (the slug is keyed on the git common dir, so every worktree shares it), moves any legacy .context/retros from this worktree and the main checkout into it, lists prior snapshots and prints the next snapshot path. Step 12/13 and the gbrain prior-retros query use it. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-retro-metrics | 49 +++++++ docs/skills.md | 2 +- retro/SKILL.md | 32 ++--- retro/SKILL.md.tmpl | 32 ++--- ...ession-3749-retro-history-worktree.test.ts | 130 ++++++++++++++++++ 5 files changed, 202 insertions(+), 43 deletions(-) create mode 100644 test/regression-3749-retro-history-worktree.test.ts diff --git a/bin/gstack-retro-metrics b/bin/gstack-retro-metrics index 40bb4f43c..11c30f4d4 100755 --- a/bin/gstack-retro-metrics +++ b/bin/gstack-retro-metrics @@ -28,6 +28,16 @@ # --since midnight-aligned ISO date ("2026-03-11T00:00:00") or a relative # expression ("24 hours ago"). Default: "7 days ago". # --until optional window end (compare mode's prior window). +# +# gstack-retro-metrics --history [--next ] +# +# Retro history for this project (#3749): prints RETRO_HISTORY_DIR (the +# state root's projects//retros, shared by every worktree of the +# repository) and one RETRO_PRIOR line per snapshot, newest first. Moves any +# legacy .context/retros/*.json from this worktree and the main checkout +# into it first: identical copies collapse, a same-name snapshot that +# differs takes the next free number for its date. --next also prints +# RETRO_FILE, the next unused -.json path. GSTACK_BUN_CONFIG=/dev/null; case "${OSTYPE:-}" in msys*|cygwin*) if command -v cygpath >/dev/null 2>&1; then GSTACK_BUN_CONFIG=NUL; fi ;; esac @@ -39,6 +49,8 @@ while [ $# -gt 0 ]; do --base) BASE="$2"; shift 2 ;; --since) SINCE="$2"; shift 2 ;; --until) UNTIL="$2"; shift 2 ;; + --history) HISTORY=1; shift ;; + --next) NEXT_DATE="$2"; shift 2 ;; *) shift ;; esac done @@ -51,6 +63,43 @@ _BIN="$_SCRIPT_DIR" gstack_state_root_select _GH="$_gstack_sr_root" +if [ -n "${HISTORY:-}" ]; then + _SLUG=$("$_BIN/gstack-slug" --get SLUG 2>/dev/null || true) + if [ -z "$_SLUG" ]; then + echo "RETRO_HISTORY_ERROR: could not resolve the project slug (gstack-slug failed). fix: reinstall with ./setup or /gstack-upgrade" + exit 0 + fi + _HD="$_GH/projects/$_SLUG/retros" + mkdir -p "$_HD" || { echo "RETRO_HISTORY_ERROR: cannot create $_HD"; exit 0; } + _TOP=$(git rev-parse --show-toplevel 2>/dev/null || true) + _MAIN=$(git worktree list --porcelain 2>/dev/null | sed -n '1s/^worktree //p') + _MOVED=0 + for _TREE in "$_TOP" "$_MAIN"; do + [ -n "$_TREE" ] && [ -d "$_TREE/.context/retros" ] || continue + for _F in "$_TREE"/.context/retros/*.json; do + [ -f "$_F" ] || continue + _B=$(basename "$_F") + _DEST="$_HD/$_B" + if [ -e "$_DEST" ] && cmp -s "$_F" "$_DEST"; then + rm -f "$_F"; continue + fi + _N=1 + while [ -e "$_DEST" ]; do _N=$((_N + 1)); _DEST="$_HD/${_B%-*}-$_N.json"; done + mv "$_F" "$_DEST" && _MOVED=$((_MOVED + 1)) + done + rmdir "$_TREE/.context/retros" 2>/dev/null || true + done + echo "RETRO_HISTORY_DIR: $_HD" + [ "$_MOVED" -gt 0 ] && echo "RETRO_HISTORY_MIGRATED: $_MOVED from .context/retros" + ls -t "$_HD"/*.json 2>/dev/null | sed 's/^/RETRO_PRIOR: /' + if [ -n "${NEXT_DATE:-}" ]; then + _N=1 + while [ -e "$_HD/$NEXT_DATE-$_N.json" ]; do _N=$((_N + 1)); done + echo "RETRO_FILE: $_HD/$NEXT_DATE-$_N.json" + fi + exit 0 +fi + echo "RETRO_METRICS_PROTO: 1" if ! git rev-parse --git-dir >/dev/null 2>&1; then diff --git a/docs/skills.md b/docs/skills.md index dde699409..e56183572 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -1115,7 +1115,7 @@ Claude: Week of Mar 1: 47 commits (3 contributors), 3.2k LOC, 38% tests, 12 PRs, [Top 3 team wins, 3 things to improve, 3 habits for next week] ``` -It saves a JSON snapshot to `.context/retros/` so the next run can show trends. +It saves a JSON snapshot under `~/.gstack/projects//retros/`, shared by every worktree of the repository, so the next run can show trends from any checkout. An older `.context/retros/` is moved there on the first run. --- diff --git a/retro/SKILL.md b/retro/SKILL.md index 0b71c2dfd..f8d286db1 100644 --- a/retro/SKILL.md +++ b/retro/SKILL.md @@ -18,8 +18,8 @@ gbrain: context_queries: - id: prior-retros kind: filesystem - # /retro writes .context/retros/*.json (repo-local; see Step 13). - glob: ".context/retros/*.json" + # /retro writes its history here, shared by every worktree (Step 13). + glob: "{gstack_state_root}/projects/{repo_slug}/retros/*.json" sort: mtime_desc limit: 5 render_as: "## Prior retros for this project" @@ -504,13 +504,16 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Before saving the new snapshot, check for prior retro history: +Check prior history, shared by every worktree of this repo (substitute the session-reminder date for ``): ```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -ls -t .context/retros/*.json 2>/dev/null +_RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics" +[ -x "$_RM" ] || _RM=".claude/skills/gstack/bin/gstack-retro-metrics" +"$_RM" --history --next "" ``` +`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. + **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` Last Now Delta @@ -530,20 +533,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -```bash -mkdir -p .context/retros -``` - -Determine the next unused sequence number for today (substitute the session-reminder date for ``): -```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -today="" -next=1 -while [ -e ".context/retros/${today}-${next}.json" ]; do next=$((next + 1)); done -# Save as .context/retros/${today}-${next}.json -``` - -Use the Write tool to save the JSON file with this schema: +Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema: ```json { "date": "2026-03-08", @@ -945,7 +935,7 @@ When the user runs `/retro compare` (or `/retro compare 14d`): - Never compare teammates against each other negatively. Each person's section stands on its own. - Size the report to the window and the team: every section carries specific evidence, and a section with nothing to say is cut, not padded - Use markdown tables and code blocks for data, prose for narrative -- Output the narrative directly to the conversation; the only report file is the JSON snapshot (`.context/retros/` for repo retros, `$GSTACK_STATE_ROOT/retros/` for global) +- Output the narrative directly to the conversation; the only report file is the JSON snapshot (`$GSTACK_STATE_ROOT/projects//retros/` for repo retros, `$GSTACK_STATE_ROOT/retros/` for global) ## Important Rules @@ -957,4 +947,4 @@ When the user runs `/retro compare` (or `/retro compare 14d`): - Treat merge commits as PR boundaries - Do not read CLAUDE.md or unrelated docs — this skill is self-contained; the CHANGELOG and optional inputs explicitly named above are exceptions - On first run (no prior retros), skip saved-history comparisons gracefully; explicit `compare` mode still computes its prior window -- **Global mode:** Does NOT require being inside a git repo. Saves snapshots to `~/.gstack/retros/` (not `.context/retros/`). Gracefully skip AI tools that aren't installed. Only compare against prior global retros with the same window value. If streak hits 365d cap, display as "365+ days". +- **Global mode:** Does NOT require being inside a git repo. Saves snapshots to `~/.gstack/retros/` (not the per-project history). Gracefully skip AI tools that aren't installed. Only compare against prior global retros with the same window value. If streak hits 365d cap, display as "365+ days". diff --git a/retro/SKILL.md.tmpl b/retro/SKILL.md.tmpl index 32dd21b7a..61c04cc3b 100644 --- a/retro/SKILL.md.tmpl +++ b/retro/SKILL.md.tmpl @@ -22,8 +22,8 @@ gbrain: context_queries: - id: prior-retros kind: filesystem - # /retro writes .context/retros/*.json (repo-local; see Step 13). - glob: ".context/retros/*.json" + # /retro writes its history here, shared by every worktree (Step 13). + glob: "{gstack_state_root}/projects/{repo_slug}/retros/*.json" sort: mtime_desc limit: 5 render_as: "## Prior retros for this project" @@ -349,13 +349,16 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Before saving the new snapshot, check for prior retro history: +Check prior history, shared by every worktree of this repo (substitute the session-reminder date for ``): ```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -ls -t .context/retros/*.json 2>/dev/null +_RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics" +[ -x "$_RM" ] || _RM=".claude/skills/gstack/bin/gstack-retro-metrics" +"$_RM" --history --next "" ``` +`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. + **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` Last Now Delta @@ -375,20 +378,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -```bash -mkdir -p .context/retros -``` - -Determine the next unused sequence number for today (substitute the session-reminder date for ``): -```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -today="" -next=1 -while [ -e ".context/retros/${today}-${next}.json" ]; do next=$((next + 1)); done -# Save as .context/retros/${today}-${next}.json -``` - -Use the Write tool to save the JSON file with this schema: +Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema: ```json { "date": "2026-03-08", @@ -766,7 +756,7 @@ When the user runs `/retro compare` (or `/retro compare 14d`): - Never compare teammates against each other negatively. Each person's section stands on its own. - Size the report to the window and the team: every section carries specific evidence, and a section with nothing to say is cut, not padded - Use markdown tables and code blocks for data, prose for narrative -- Output the narrative directly to the conversation; the only report file is the JSON snapshot (`.context/retros/` for repo retros, `$GSTACK_STATE_ROOT/retros/` for global) +- Output the narrative directly to the conversation; the only report file is the JSON snapshot (`$GSTACK_STATE_ROOT/projects//retros/` for repo retros, `$GSTACK_STATE_ROOT/retros/` for global) ## Important Rules @@ -778,4 +768,4 @@ When the user runs `/retro compare` (or `/retro compare 14d`): - Treat merge commits as PR boundaries - Do not read CLAUDE.md or unrelated docs — this skill is self-contained; the CHANGELOG and optional inputs explicitly named above are exceptions - On first run (no prior retros), skip saved-history comparisons gracefully; explicit `compare` mode still computes its prior window -- **Global mode:** Does NOT require being inside a git repo. Saves snapshots to `~/.gstack/retros/` (not `.context/retros/`). Gracefully skip AI tools that aren't installed. Only compare against prior global retros with the same window value. If streak hits 365d cap, display as "365+ days". +- **Global mode:** Does NOT require being inside a git repo. Saves snapshots to `~/.gstack/retros/` (not the per-project history). Gracefully skip AI tools that aren't installed. Only compare against prior global retros with the same window value. If streak hits 365d cap, display as "365+ days". diff --git a/test/regression-3749-retro-history-worktree.test.ts b/test/regression-3749-retro-history-worktree.test.ts new file mode 100644 index 000000000..96ee42589 --- /dev/null +++ b/test/regression-3749-retro-history-worktree.test.ts @@ -0,0 +1,130 @@ +/** + * Regression for #3749: /retro kept its history in .context/retros/ inside the + * working tree, so a run in a linked worktree (Conductor, Claude Code) found + * none of the main checkout's snapshots, reported "First retro recorded", and + * wrote its own snapshot where `git worktree remove` deletes it. History now + * lives in the state root, keyed by project slug (shared by every worktree of + * a repository), and an existing .context/retros/ is migrated on first use. + * + * Runs the real Step 12 bash block (history and the Step 13 snapshot path) from the generated + * retro/SKILL.md with the gstack install pointed at this checkout. + */ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SKILL = fs.readFileSync(path.join(ROOT, 'retro', 'SKILL.md'), 'utf-8'); +const TMPL = fs.readFileSync(path.join(ROOT, 'retro', 'SKILL.md.tmpl'), 'utf-8'); + +function bashBlockAfter(heading: string, contains: string): string { + const start = SKILL.indexOf(heading); + if (start < 0) throw new Error(`retro/SKILL.md: heading not found: ${heading}`); + const end = SKILL.indexOf('\n### ', start + heading.length); + const re = /```bash\n([\s\S]*?)```/g; + re.lastIndex = start; + for (let m = re.exec(SKILL); m && m.index < end; m = re.exec(SKILL)) { + if (m[1].includes(contains)) { + return m[1].replaceAll('$HOME/.claude/skills/gstack', ROOT).replaceAll('~/.claude/skills/gstack', ROOT); + } + } + throw new Error(`retro/SKILL.md: no bash block containing ${contains} under ${heading}`); +} + +const LOAD = bashBlockAfter('### Step 12: Load History & Compare', 'retro').replace('', '2026-11-01'); + +let tmp: string; +let home: string; + +function sh(script: string, cwd: string) { + const env: Record = {}; + for (const [k, v] of Object.entries(process.env)) if (v !== undefined) env[k] = v; + for (const k of ['GSTACK_STATE_ROOT', 'GSTACK_STATE_DIR', 'GSTACK_PROJECT_SLUG', 'CLAUDE_PLUGIN_DATA', 'CLAUDE_PROJECT_DIR']) delete env[k]; + const r = spawnSync('bash', ['-c', script], { cwd, env: { ...env, HOME: tmp, GSTACK_HOME: home }, encoding: 'utf-8', timeout: 30_000 }); + return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1 }; +} + +function git(cwd: string, ...args: string[]) { + const r = spawnSync('git', args, { cwd, encoding: 'utf-8', timeout: 30_000 }); + if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`); +} + +function snapshot(dir: string, name: string, body: object): string { + fs.mkdirSync(dir, { recursive: true }); + const file = path.join(dir, name); + fs.writeFileSync(file, JSON.stringify(body)); + return file; +} + +/** A main checkout with one commit (fixture-local identity) and a linked worktree under it. */ +function layout() { + const main = path.join(tmp, 'widget'); + fs.mkdirSync(main); + git(main, 'init', '-q', '-b', 'main'); + git(main, 'remote', 'add', 'origin', 'https://github.com/acme/widget.git'); + git(main, 'config', 'user.name', 'Fixture'); + git(main, 'config', 'user.email', 'fixture@example.com'); + git(main, 'commit', '-q', '--allow-empty', '-m', 'init'); + git(main, 'worktree', 'add', '-q', '-b', 'demo', path.join(main, '.claude', 'worktrees', 'demo')); + const wt = fs.realpathSync(path.join(main, '.claude', 'worktrees', 'demo')); + const slug = sh(`'${ROOT}/bin/gstack-slug' --get SLUG`, main).stdout.trim(); + expect(slug).not.toBe(''); + return { main, wt, history: path.join(home, 'projects', slug, 'retros') }; +} + +beforeEach(() => { + tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'retro-3749-'))); + home = path.join(tmp, '.gstack'); +}); + +afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +describe('#3749 retro history is shared across linked worktrees', () => { + test('Step 12 in a linked worktree finds the main checkout\'s earlier snapshot', () => { + const { main, wt, history } = layout(); + snapshot(path.join(main, '.context', 'retros'), '2026-10-25-1.json', { date: '2026-10-25', window: '7d' }); + + const r = sh(LOAD, wt); + expect(r.status).toBe(0); + expect(r.stdout).toContain(path.join(history, '2026-10-25-1.json')); + expect(fs.existsSync(path.join(history, '2026-10-25-1.json'))).toBe(true); + expect(fs.existsSync(path.join(main, '.context', 'retros'))).toBe(false); + }); + + test('the snapshot path from a linked worktree is outside the worktree, and the main checkout sees it', () => { + const { main, wt, history } = layout(); + const save = sh(LOAD, wt); + expect(save.status).toBe(0); + const file = save.stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; + expect(file).toBe(path.join(history, '2026-11-01-1.json')); + expect(file!.startsWith(wt)).toBe(false); + fs.writeFileSync(file!, JSON.stringify({ date: '2026-11-01', window: '7d' })); + + git(main, 'worktree', 'remove', '--force', wt); + expect(sh(LOAD, main).stdout).toContain(file!); + expect(sh(LOAD, main).stdout).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`); + }); + + test('migration keeps every snapshot: duplicates collapse, same-name differing snapshots get the next number', () => { + const { main, wt, history } = layout(); + for (const tree of [main, wt]) snapshot(path.join(tree, '.context', 'retros'), '2026-10-18-1.json', { date: '2026-10-18', window: '7d' }); + snapshot(path.join(main, '.context', 'retros'), '2026-10-25-1.json', { date: '2026-10-25', from: 'main' }); + snapshot(path.join(wt, '.context', 'retros'), '2026-10-25-1.json', { date: '2026-10-25', from: 'worktree' }); + + const r = sh(LOAD, wt); + expect(r.status).toBe(0); + expect(fs.readdirSync(history).sort()).toEqual(['2026-10-18-1.json', '2026-10-25-1.json', '2026-10-25-2.json']); + const froms = ['2026-10-25-1.json', '2026-10-25-2.json'].map((f) => JSON.parse(fs.readFileSync(path.join(history, f), 'utf-8')).from).sort(); + expect(froms).toEqual(['main', 'worktree']); + for (const tree of [main, wt]) expect(fs.existsSync(path.join(tree, '.context', 'retros'))).toBe(false); + }); + + test('the gbrain prior-retros query reads the same state-root history', () => { + expect(TMPL).toContain('glob: "{gstack_state_root}/projects/{repo_slug}/retros/*.json"'); + expect(TMPL).not.toContain('glob: ".context/retros/*.json"'); + }); +}); From a14511156ceaa6a9435a282013af7f40900ff955 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:33:06 +0000 Subject: [PATCH 06/35] fix(retro): make history migration safe against symlinks and concurrent runs (#3749 review) Review pass 1 findings: a .context/retros that resolves outside its worktree (or to the shared history itself) is left alone, so migration can no longer delete the history it points at; moves are no-clobber with retry; a copy identical to any same-date snapshot collapses; RETRO_FILE is reserved as {} so overlapping runs in two worktrees never share a path; --next must be a YYYY-MM-DD date; a missing option value no longer loops forever. Step 12 names RETRO_HISTORY_ERROR instead of reading it as a stale helper. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-retro-metrics | 52 +++++++++++++------ retro/SKILL.md | 4 +- retro/SKILL.md.tmpl | 4 +- ...ession-3749-retro-history-worktree.test.ts | 28 ++++++++-- 4 files changed, 66 insertions(+), 22 deletions(-) diff --git a/bin/gstack-retro-metrics b/bin/gstack-retro-metrics index 11c30f4d4..d49abd3cb 100755 --- a/bin/gstack-retro-metrics +++ b/bin/gstack-retro-metrics @@ -35,9 +35,12 @@ # state root's projects//retros, shared by every worktree of the # repository) and one RETRO_PRIOR line per snapshot, newest first. Moves any # legacy .context/retros/*.json from this worktree and the main checkout -# into it first: identical copies collapse, a same-name snapshot that -# differs takes the next free number for its date. --next also prints -# RETRO_FILE, the next unused -.json path. +# into it first: a copy identical to a stored snapshot of the same date +# collapses, a same-name snapshot that differs takes the next free number +# for its date, and a .context/retros that resolves outside its worktree +# is left alone. --next also reserves the next unused +# -.json (written as {} so a concurrent run cannot take it) and +# prints it as RETRO_FILE. GSTACK_BUN_CONFIG=/dev/null; case "${OSTYPE:-}" in msys*|cygwin*) if command -v cygpath >/dev/null 2>&1; then GSTACK_BUN_CONFIG=NUL; fi ;; esac @@ -46,11 +49,11 @@ SINCE="7 days ago" UNTIL="" while [ $# -gt 0 ]; do case "$1" in - --base) BASE="$2"; shift 2 ;; - --since) SINCE="$2"; shift 2 ;; - --until) UNTIL="$2"; shift 2 ;; + --base) BASE="$2"; shift 2 || break ;; + --since) SINCE="$2"; shift 2 || break ;; + --until) UNTIL="$2"; shift 2 || break ;; --history) HISTORY=1; shift ;; - --next) NEXT_DATE="$2"; shift 2 ;; + --next) NEXT_DATE="$2"; shift 2 || break ;; *) shift ;; esac done @@ -71,21 +74,37 @@ if [ -n "${HISTORY:-}" ]; then fi _HD="$_GH/projects/$_SLUG/retros" mkdir -p "$_HD" || { echo "RETRO_HISTORY_ERROR: cannot create $_HD"; exit 0; } + if [ -n "${NEXT_DATE:-}" ]; then + case "$NEXT_DATE" in + [0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]) ;; + *) echo "RETRO_HISTORY_ERROR: --next takes a YYYY-MM-DD date, got '$NEXT_DATE'"; exit 0 ;; + esac + fi + _HDR=$(cd -P "$_HD" && pwd) _TOP=$(git rev-parse --show-toplevel 2>/dev/null || true) _MAIN=$(git worktree list --porcelain 2>/dev/null | sed -n '1s/^worktree //p') _MOVED=0 for _TREE in "$_TOP" "$_MAIN"; do [ -n "$_TREE" ] && [ -d "$_TREE/.context/retros" ] || continue - for _F in "$_TREE"/.context/retros/*.json; do - [ -f "$_F" ] || continue + _SRC=$(cd -P "$_TREE/.context/retros" 2>/dev/null && pwd) || continue + _TR=$(cd -P "$_TREE" 2>/dev/null && pwd) || continue + case "$_SRC" in "$_TR"/*) ;; *) continue ;; esac + [ "$_SRC" != "$_HDR" ] || continue + for _F in "$_SRC"/*.json; do + [ -f "$_F" ] && [ ! -L "$_F" ] || continue _B=$(basename "$_F") + _DUP="" + for _E in "$_HD/$_B" "$_HD/${_B%-*}"-*.json; do + [ -f "$_E" ] && cmp -s "$_F" "$_E" && { _DUP=1; break; } + done + if [ -n "$_DUP" ]; then rm -f "$_F"; continue; fi _DEST="$_HD/$_B" - if [ -e "$_DEST" ] && cmp -s "$_F" "$_DEST"; then - rm -f "$_F"; continue - fi _N=1 - while [ -e "$_DEST" ]; do _N=$((_N + 1)); _DEST="$_HD/${_B%-*}-$_N.json"; done - mv "$_F" "$_DEST" && _MOVED=$((_MOVED + 1)) + while [ -e "$_F" ] && [ "$_N" -le 1000 ]; do + [ -e "$_DEST" ] || mv -n "$_F" "$_DEST" 2>/dev/null + [ -e "$_F" ] || { _MOVED=$((_MOVED + 1)); break; } + _N=$((_N + 1)); _DEST="$_HD/${_B%-*}-$_N.json" + done done rmdir "$_TREE/.context/retros" 2>/dev/null || true done @@ -94,7 +113,10 @@ if [ -n "${HISTORY:-}" ]; then ls -t "$_HD"/*.json 2>/dev/null | sed 's/^/RETRO_PRIOR: /' if [ -n "${NEXT_DATE:-}" ]; then _N=1 - while [ -e "$_HD/$NEXT_DATE-$_N.json" ]; do _N=$((_N + 1)); done + until ( set -C; printf '{}\n' > "$_HD/$NEXT_DATE-$_N.json" ) 2>/dev/null; do + _N=$((_N + 1)) + [ "$_N" -le 1000 ] || { echo "RETRO_HISTORY_ERROR: cannot reserve a snapshot file in $_HD"; exit 0; } + done echo "RETRO_FILE: $_HD/$NEXT_DATE-$_N.json" fi exit 0 diff --git a/retro/SKILL.md b/retro/SKILL.md index f8d286db1..256ffb2d0 100644 --- a/retro/SKILL.md +++ b/retro/SKILL.md @@ -512,7 +512,7 @@ _RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics" "$_RM" --history --next "" ``` -`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` @@ -533,7 +533,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema: +Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema: ```json { "date": "2026-03-08", diff --git a/retro/SKILL.md.tmpl b/retro/SKILL.md.tmpl index 61c04cc3b..1ec7941f3 100644 --- a/retro/SKILL.md.tmpl +++ b/retro/SKILL.md.tmpl @@ -357,7 +357,7 @@ _RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics" "$_RM" --history --next "" ``` -`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` @@ -378,7 +378,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema: +Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema: ```json { "date": "2026-03-08", diff --git a/test/regression-3749-retro-history-worktree.test.ts b/test/regression-3749-retro-history-worktree.test.ts index 96ee42589..0e4a16a2c 100644 --- a/test/regression-3749-retro-history-worktree.test.ts +++ b/test/regression-3749-retro-history-worktree.test.ts @@ -105,8 +105,9 @@ describe('#3749 retro history is shared across linked worktrees', () => { fs.writeFileSync(file!, JSON.stringify({ date: '2026-11-01', window: '7d' })); git(main, 'worktree', 'remove', '--force', wt); - expect(sh(LOAD, main).stdout).toContain(file!); - expect(sh(LOAD, main).stdout).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`); + const fromMain = sh(LOAD, main).stdout; + expect(fromMain).toContain(file!); + expect(fromMain).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`); }); test('migration keeps every snapshot: duplicates collapse, same-name differing snapshots get the next number', () => { @@ -117,12 +118,33 @@ describe('#3749 retro history is shared across linked worktrees', () => { const r = sh(LOAD, wt); expect(r.status).toBe(0); - expect(fs.readdirSync(history).sort()).toEqual(['2026-10-18-1.json', '2026-10-25-1.json', '2026-10-25-2.json']); + expect(fs.readdirSync(history).sort()).toEqual(['2026-10-18-1.json', '2026-10-25-1.json', '2026-10-25-2.json', '2026-11-01-1.json']); const froms = ['2026-10-25-1.json', '2026-10-25-2.json'].map((f) => JSON.parse(fs.readFileSync(path.join(history, f), 'utf-8')).from).sort(); expect(froms).toEqual(['main', 'worktree']); for (const tree of [main, wt]) expect(fs.existsSync(path.join(tree, '.context', 'retros'))).toBe(false); }); + test('a .context/retros symlinked to the shared history is left alone, so migration cannot delete it', () => { + const { wt, history } = layout(); + const kept = snapshot(history, '2026-10-25-1.json', { date: '2026-10-25', window: '7d' }); + fs.mkdirSync(path.join(wt, '.context'), { recursive: true }); + fs.symlinkSync(history, path.join(wt, '.context', 'retros')); + + const r = sh(LOAD, wt); + expect(r.status).toBe(0); + expect(fs.readFileSync(kept, 'utf-8')).toContain('2026-10-25'); + expect(r.stdout).toContain(`RETRO_PRIOR: ${kept}`); + }); + + test('RETRO_FILE is reserved, so two runs in different worktrees never get the same path', () => { + const { main, wt, history } = layout(); + const first = sh(LOAD, wt).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; + const second = sh(LOAD, main).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; + expect(first).toBe(path.join(history, '2026-11-01-1.json')); + expect(second).toBe(path.join(history, '2026-11-01-2.json')); + expect(fs.readFileSync(first!, 'utf-8').trim()).toBe('{}'); + }); + test('the gbrain prior-retros query reads the same state-root history', () => { expect(TMPL).toContain('glob: "{gstack_state_root}/projects/{repo_slug}/retros/*.json"'); expect(TMPL).not.toContain('glob: ".context/retros/*.json"'); From 99f7f7c8be2f7581650ab4f25d8f10e7205a93c5 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:37:16 +0000 Subject: [PATCH 07/35] docs(changelog): fragment for the #3749 retro history fix Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/fix-3749-retro-history-state-root.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog.d/fix-3749-retro-history-state-root.md diff --git a/changelog.d/fix-3749-retro-history-state-root.md b/changelog.d/fix-3749-retro-history-state-root.md new file mode 100644 index 000000000..026253cb2 --- /dev/null +++ b/changelog.d/fix-3749-retro-history-state-root.md @@ -0,0 +1,10 @@ +**/retro now remembers its history across git worktrees, so a run in a linked worktree compares against last week instead of announcing "First retro recorded".** Retro snapshots used to live in `.context/retros/` inside the working tree. A worktree (Conductor, Claude Code) saw none of the main checkout's history, and its own snapshot was deleted along with the worktree (#3749). + +### Fixed +- **`bin/gstack-retro-metrics --history [--next ]`**: keeps retro history in `~/.gstack/projects//retros/` (the state root), which every worktree of a repository shares. It lists earlier snapshots as `RETRO_PRIOR` lines, newest first, and reserves the next `-.json` as `RETRO_FILE`, so two retros running at once in different worktrees never write the same file. +- **Moving old history**: snapshots in `.context/retros/` of the current worktree and the main checkout move to the shared folder on the first run. A copy identical to a stored snapshot from the same date is dropped, and a different snapshot with the same name takes the next free number, so nothing is overwritten. A `.context/retros` that points outside its worktree (a symlink to the shared folder or anywhere else) is left alone, and moves never replace an existing file. +- **`retro/SKILL.md`**: Steps 12 and 13 use the helper, the gbrain prior-retros query reads the shared folder, and a history error is reported to the user instead of being mistaken for an outdated helper. `docs/skills.md` says where snapshots are saved. +- `gstack-retro-metrics` no longer loops forever when an option such as `--next` is given without a value. + +### Tests +- `test/regression-3749-retro-history-worktree.test.ts` runs the real Step 12 block in a main checkout with a linked worktree: the worktree sees the main checkout's snapshot, its new snapshot path is outside the worktree and survives `git worktree remove`, migration keeps every snapshot, a symlinked `.context/retros` cannot delete history, and two runs get different snapshot paths. It failed 4 of 4 on the code before the fix. From c8462cc4b6176cdd7b993a44bd41c117cf272109 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:46:32 +0000 Subject: [PATCH 08/35] fix(retro): trim Step 12/13 prose back under the parity skeleton cap Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- retro/SKILL.md | 6 +++--- retro/SKILL.md.tmpl | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/retro/SKILL.md b/retro/SKILL.md index 256ffb2d0..3c6136a1f 100644 --- a/retro/SKILL.md +++ b/retro/SKILL.md @@ -504,7 +504,7 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Check prior history, shared by every worktree of this repo (substitute the session-reminder date for ``): +Load history (shared across worktrees; `` = session-reminder date): ```bash _RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics" @@ -512,7 +512,7 @@ _RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics" "$_RM" --history --next "" ``` -`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; skip history, suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` @@ -533,7 +533,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema: +Read Step 12's reserved `RETRO_FILE`, then Write it with this schema: ```json { "date": "2026-03-08", diff --git a/retro/SKILL.md.tmpl b/retro/SKILL.md.tmpl index 1ec7941f3..9a2bd9167 100644 --- a/retro/SKILL.md.tmpl +++ b/retro/SKILL.md.tmpl @@ -349,7 +349,7 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Check prior history, shared by every worktree of this repo (substitute the session-reminder date for ``): +Load history (shared across worktrees; `` = session-reminder date): ```bash _RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics" @@ -357,7 +357,7 @@ _RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics" "$_RM" --history --next "" ``` -`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; skip history, suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` @@ -378,7 +378,7 @@ Deep sessions: 3 → 5 ↑2 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`. -Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema: +Read Step 12's reserved `RETRO_FILE`, then Write it with this schema: ```json { "date": "2026-03-08", From 4bda4807a8b998197bc812c5b207676ef2482437 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 16:55:51 +0000 Subject: [PATCH 09/35] fix(retro): keep today="" in Step 12 so the judge excerpt keeps both date inputs Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- retro/SKILL.md | 7 ++++--- retro/SKILL.md.tmpl | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/retro/SKILL.md b/retro/SKILL.md index 3c6136a1f..85b3ba8df 100644 --- a/retro/SKILL.md +++ b/retro/SKILL.md @@ -504,15 +504,16 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Load history (shared across worktrees; `` = session-reminder date): +Load history, shared across worktrees (``: session-reminder date): ```bash _RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics" [ -x "$_RM" ] || _RM=".claude/skills/gstack/bin/gstack-retro-metrics" -"$_RM" --history --next "" +today="" +"$_RM" --history --next "$today" ``` -`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; skip history, suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` diff --git a/retro/SKILL.md.tmpl b/retro/SKILL.md.tmpl index 9a2bd9167..00a7d8961 100644 --- a/retro/SKILL.md.tmpl +++ b/retro/SKILL.md.tmpl @@ -349,15 +349,16 @@ End the section with: `N markers, M with no trigger.` If none: `No shortcut debt ### Step 12: Load History & Compare -Load history (shared across worktrees; `` = session-reminder date): +Load history, shared across worktrees (``: session-reminder date): ```bash _RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics" [ -x "$_RM" ] || _RM=".claude/skills/gstack/bin/gstack-retro-metrics" -"$_RM" --history --next "" +today="" +"$_RM" --history --next "$today" ``` -`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; skip history, suggest `/gstack-upgrade`. +`RETRO_PRIOR:` lists snapshots, newest first (skip `{}` stubs). On `RETRO_HISTORY_ERROR:` show its fix, skip history and Step 13. No `RETRO_FILE:` otherwise: stale helper; suggest `/gstack-upgrade`. **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead): ``` From 5b6258151aaf8ffb5d69a9908e95e6456e8118a2 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 17:57:45 +0000 Subject: [PATCH 10/35] test(retro): compare #3749 history paths with forward slashes so the Windows shard passes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- ...ession-3749-retro-history-worktree.test.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/test/regression-3749-retro-history-worktree.test.ts b/test/regression-3749-retro-history-worktree.test.ts index 0e4a16a2c..af3daa419 100644 --- a/test/regression-3749-retro-history-worktree.test.ts +++ b/test/regression-3749-retro-history-worktree.test.ts @@ -38,12 +38,15 @@ const LOAD = bashBlockAfter('### Step 12: Load History & Compare', 'retro').repl let tmp: string; let home: string; +/** Git Bash on Windows prints the state root as given plus `/projects/...`; compare with forward slashes. */ +const fwd = (p: string) => p.replaceAll('\\', '/'); + function sh(script: string, cwd: string) { const env: Record = {}; for (const [k, v] of Object.entries(process.env)) if (v !== undefined) env[k] = v; for (const k of ['GSTACK_STATE_ROOT', 'GSTACK_STATE_DIR', 'GSTACK_PROJECT_SLUG', 'CLAUDE_PLUGIN_DATA', 'CLAUDE_PROJECT_DIR']) delete env[k]; const r = spawnSync('bash', ['-c', script], { cwd, env: { ...env, HOME: tmp, GSTACK_HOME: home }, encoding: 'utf-8', timeout: 30_000 }); - return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1 }; + return { stdout: fwd(r.stdout ?? ''), stderr: r.stderr ?? '', status: r.status ?? -1 }; } function git(cwd: string, ...args: string[]) { @@ -90,7 +93,7 @@ describe('#3749 retro history is shared across linked worktrees', () => { const r = sh(LOAD, wt); expect(r.status).toBe(0); - expect(r.stdout).toContain(path.join(history, '2026-10-25-1.json')); + expect(r.stdout).toContain(fwd(path.join(history, '2026-10-25-1.json'))); expect(fs.existsSync(path.join(history, '2026-10-25-1.json'))).toBe(true); expect(fs.existsSync(path.join(main, '.context', 'retros'))).toBe(false); }); @@ -100,14 +103,14 @@ describe('#3749 retro history is shared across linked worktrees', () => { const save = sh(LOAD, wt); expect(save.status).toBe(0); const file = save.stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; - expect(file).toBe(path.join(history, '2026-11-01-1.json')); - expect(file!.startsWith(wt)).toBe(false); + expect(file).toBe(fwd(path.join(history, '2026-11-01-1.json'))); + expect(file!.startsWith(fwd(wt))).toBe(false); fs.writeFileSync(file!, JSON.stringify({ date: '2026-11-01', window: '7d' })); git(main, 'worktree', 'remove', '--force', wt); const fromMain = sh(LOAD, main).stdout; expect(fromMain).toContain(file!); - expect(fromMain).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`); + expect(fromMain).toContain(`RETRO_FILE: ${fwd(path.join(history, '2026-11-01-2.json'))}`); }); test('migration keeps every snapshot: duplicates collapse, same-name differing snapshots get the next number', () => { @@ -133,15 +136,15 @@ describe('#3749 retro history is shared across linked worktrees', () => { const r = sh(LOAD, wt); expect(r.status).toBe(0); expect(fs.readFileSync(kept, 'utf-8')).toContain('2026-10-25'); - expect(r.stdout).toContain(`RETRO_PRIOR: ${kept}`); + expect(r.stdout).toContain(`RETRO_PRIOR: ${fwd(kept)}`); }); test('RETRO_FILE is reserved, so two runs in different worktrees never get the same path', () => { const { main, wt, history } = layout(); const first = sh(LOAD, wt).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; const second = sh(LOAD, main).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1]; - expect(first).toBe(path.join(history, '2026-11-01-1.json')); - expect(second).toBe(path.join(history, '2026-11-01-2.json')); + expect(first).toBe(fwd(path.join(history, '2026-11-01-1.json'))); + expect(second).toBe(fwd(path.join(history, '2026-11-01-2.json'))); expect(fs.readFileSync(first!, 'utf-8').trim()).toBe('{}'); }); From 7bc4f0ea06fa56a7b3e1e43b05af8ba562572ab1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:10:47 +0000 Subject: [PATCH 11/35] test(review): review-plan and gate-diff plan the same with a stale local main or a shadowing tag (GST-883, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-plan-origin-base.test.ts | 95 ++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 test/review-plan-origin-base.test.ts diff --git a/test/review-plan-origin-base.test.ts b/test/review-plan-origin-base.test.ts new file mode 100644 index 000000000..cb5f27bce --- /dev/null +++ b/test/review-plan-origin-base.test.ts @@ -0,0 +1,95 @@ +/** + * gstack-review-plan and gstack-gate-diff diff against origin/ (GST-883): with a local that lags + * origin/, review-plan handed the raw name to gstack-gate-diff, which merged against the stale local ref and + * scanned upstream commits the branch only inherited, so a code diff planned as gate class at full depth. + */ +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'; + +const PLAN = path.join(import.meta.dir, '..', 'bin', 'gstack-review-plan'); +const GATE_DIFF = path.join(import.meta.dir, '..', 'bin', 'gstack-gate-diff'); +const FIXTURES = path.join(import.meta.dir, 'fixtures', 'gate-diff'); +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(), 'rp-origin-'))); + 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') }; + fs.writeFileSync(path.join(root, 'gitconfig'), '[commit]\n\tgpgsign = false\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 -b main origin.git && git clone -q origin.git work 2>/dev/null'); + sh(work, `git apply '${FIXTURES}/rh1-skip.base.patch' && git add -A && git commit -qm base && git push -q origin main`); + sh(root, `git clone -q origin.git other && cd other && git apply '${FIXTURES}/rh1-skip.patch' && git commit -qam upstream && git push -q origin main`); + sh(work, "git fetch -q origin && git checkout -qb feat origin/main && echo 'export const v = 1;' > src/v.ts && git add -A && git commit -qm feat"); + return { work, env, sh }; +} + +const planOf = (work: string, env: NodeJS.ProcessEnv) => { + const r = spawnSync(PLAN, ['--base', 'main', '--json'], { cwd: work, env, encoding: 'utf8', timeout: 180_000 }); + expect(r.status, r.stdout + r.stderr).toBe(0); + const p = JSON.parse(r.stdout) as { class: string; depth: string; gate_scan: unknown }; + return { class: p.class, depth: p.depth, gate_scan: p.gate_scan }; +}; +const gateDiff = (work: string, env: NodeJS.ProcessEnv, base: string) => { + const r = spawnSync(GATE_DIFF, [base], { cwd: work, env, encoding: 'utf8', timeout: 120_000 }); + expect(r.status, r.stdout + r.stderr).toBe(0); + return { tagged: /tagged=\{([^}]*)\}/.exec(r.stdout)?.[1], candidate: /candidate=(\S+)/.exec(r.stdout)?.[1], stderr: r.stderr }; +}; + +describe('review-plan and gate-diff read the origin merge base (GST-883)', () => { + // Value: protects=a stale local never turns inherited upstream commits into gate hunks that force full depth; + // fails_when=gstack-gate-diff merges against the local while origin/ is ahead, or review-plan hands it a base the plan's own diff did not use; + // why_new=GEVA-117 paid ~15-20M tokens of full-depth readers for a gate hunk already on origin/main; seam=bin/gstack-review-plan, bin/gstack-gate-diff; + // no_claim=which gate rules fire + test('local main behind origin/main plans the same class, depth and gate-tagged count as a fresh main', () => { + const { work, env, sh } = fixture(); + expect(sh(work, 'git merge-base main HEAD')).not.toBe(sh(work, 'git merge-base origin/main HEAD')); + const stale = { plan: planOf(work, env), tags: gateDiff(work, env, 'main').tagged }; + sh(work, 'git branch -f main origin/main'); + const fresh = { plan: planOf(work, env), tags: gateDiff(work, env, 'main').tagged }; + expect(fresh).toEqual({ plan: { class: 'code', depth: 'light', gate_scan: expect.anything() }, tags: '' }); + expect(stale).toEqual(fresh); + }, 240_000); + + // Value: protects=a tag cannot stand in for the base branch, so it cannot move the merge base to HEAD and hide committed edits; + // fails_when=resolveBase or collectLive read `origin/main` or `main` by short name while a tag of that name exists; + // why_new=outside-voice finding on the whole diff (shadow tag reproduced); seam=bin/gstack-review-plan, bin/gstack-gate-diff + for (const [tag, branch] of [['origin/main', 'refs/remotes/origin/main'], ['main', 'refs/heads/main']] as const) { + test(`a tag named ${tag} at HEAD does not shadow ${branch}`, () => { + const { work, env, sh } = fixture(); + if (branch === 'refs/heads/main') sh(work, 'git remote remove origin'); + const before = { plan: planOf(work, env), candidate: gateDiff(work, env, 'main').candidate }; + sh(work, `git tag '${tag}' HEAD`); + const after = { plan: planOf(work, env), candidate: gateDiff(work, env, 'main').candidate }; + expect(after).toEqual(before); + expect(after.candidate?.startsWith(sh(work, `git merge-base ${branch} HEAD`).slice(0, 12))).toBe(true); + }, 240_000); + } + + // Value: protects=without a remote-tracking twin the local base still works and the fallback is named on stderr; + // fails_when=the origin preference turns a remote-less branch into no_base, or falls back silently; why_new=the fallback is new; seam=bin/gstack-gate-diff + test('no origin/: gate-diff falls back to the local ref and says so', () => { + const { work, env, sh } = fixture(); + sh(work, 'git branch trunk origin/main'); + const r = gateDiff(work, env, 'trunk'); + expect(r.tagged).toBe(''); + expect(r.stderr).toContain('gstack-gate-diff: no remote-tracking branch for trunk; base is the local trunk, which may lag its remote'); + }, 240_000); + + // Value: protects=the fallback note names why the local ref stands, and a remote-less repository stays quiet; + // fails_when=an unrelated origin/ is reported as missing, or a repository with no remote prints a lag note; why_new=review finding on the fallback; seam=bin/gstack-gate-diff + test('an unrelated origin/ is named in the note; no remote at all uses the local base quietly', () => { + const { work, env, sh } = fixture(); + sh(work, 'git branch -f main origin/main && git update-ref refs/remotes/origin/main "$(git commit-tree "HEAD^{tree}" -m orphan)"'); + expect(gateDiff(work, env, 'main').stderr).toContain('gstack-gate-diff: origin/main shares no history with HEAD; base is the local main, which may lag its remote'); + sh(work, 'git remote remove origin'); + const none = gateDiff(work, env, 'main'); + expect(none.tagged).toBe(''); + expect(none.stderr).not.toContain('base is'); + }, 240_000); +}); From dff41a922b872d945116810e11b790f5c348bcda Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:12:08 +0000 Subject: [PATCH 12/35] fix(review): gate-diff and review-plan resolve a branch base to origin/ by full ref name; review-plan scans the merge base its own diff used (GST-883) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-gate-diff | 6 ++-- bin/gstack-review-plan | 5 +-- docs/designs/HONEST_WORK_GATE_INTEGRITY.md | 9 +++-- lib/gate-diff/git.ts | 41 ++++++++++++++-------- lib/review-collect.ts | 2 +- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/bin/gstack-gate-diff b/bin/gstack-gate-diff index 0a1af715c..d22d46075 100755 --- a/bin/gstack-gate-diff +++ b/bin/gstack-gate-diff @@ -39,7 +39,8 @@ const BIN_DIR = path.dirname(Bun.fileURLToPath(import.meta.url)); const USAGE = `usage: gstack-gate-diff [base-ref] [--format table|jsonl] [--commit ] [--help] Lists every hunk in a test, CI, runner/lint-config, snapshot or golden path of the -candidate (merge-base of and HEAD -> working tree + untracked files), +candidate (merge-base of and HEAD -> working tree + untracked files; +a branch name resolves to origin/, the local ref only when no remote has it), tags the gate edits it recognises (RH-1 RH-3 RH-4 RH-12 RH-13 RH-14 RH-15 RH-16), and prints the read-level listing after one GATE_SUMMARY: line. --commit scans that commit's first-parent delta instead of the working tree. @@ -148,7 +149,8 @@ function main(argv: string[]): number { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); return 2; } - if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : `no local branch ${baseRef}`}; base is ${base.ref}`); + if (base.local === 'fallback') console.error(`gstack-gate-diff: ${base.unrelatedRemote ? `${base.unrelatedRemote} shares no history with HEAD` : `no remote-tracking branch for ${baseRef}`}; base is the local ${baseRef}, which may lag its remote`); + else if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : `no local branch ${baseRef}`}; base is ${base.ref}`); const mb = mergeBase(base.sha, head, root); if (!mb) { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 93f6aee5e..8cd1f8d2a 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -116,8 +116,9 @@ function main(): number { const m = /GATE_SUMMARY: .*?tagged=\{([^}]*)\}/.exec(g.stdout ?? ''); return g.status === 0 && m ? m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0) : null; }; - const scope = scopeOf([base]); - let gateTagged = flag('--gate-tagged') !== undefined ? Number(flag('--gate-tagged')) : gateTagsOf([base]); + const diffBase = collected.mergeBase ?? base; + const scope = scopeOf([diffBase]); + let gateTagged = flag('--gate-tagged') !== undefined ? Number(flag('--gate-tagged')) : gateTagsOf([diffBase]); if (gateTagged !== null && !Number.isFinite(gateTagged)) gateTagged = null; const force = flag('--force') === 'all' ? 'all' as const : list('--force'); const tiersSetting = runScriptSync(path.join(binDir, 'gstack-config'), ['get', 'review_effort_tiers'], { encoding: 'utf8', timeout: 15_000 }); diff --git a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md index adfad8d8c..28a8563d9 100644 --- a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md +++ b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md @@ -180,9 +180,12 @@ only history need is calibration, which `--commit ` covers by scanning one commit's first-parent delta without the working tree.) Positional base defaults the way `gstack-diff-scope` does (the remote's `HEAD` target, origin first, then the other remotes → `main` → `master`); an -already-qualified ref is used as given, and a branch name with no local -branch resolves through its remote-tracking twin (`origin/main` for `main`: -the merge-base is the same commit), noted once on stderr. stdout: one +already-qualified ref is used as given, and a branch name resolves through +its remote-tracking twin (`origin/main` for `main`), so a stale local `main` +never adds commits the branch only inherited (GST-883). Only when no remote +has the branch does the local ref stand, noted once on stderr (a repository +with no remote at all uses it quietly). Branches are read by full ref name, +so a tag named `main` or `origin/main` cannot stand in for them. stdout: one `GATE_SUMMARY: listed=N read=N tagged={RH-1:2,RH-15:1} unmatched=N coverage= languages_unlisted=<…> candidate=` line, then the read-level listing (bounded; the full listing goes to `--out`). diff --git a/lib/gate-diff/git.ts b/lib/gate-diff/git.ts index a9d4f07c0..3e3587dcd 100644 --- a/lib/gate-diff/git.ts +++ b/lib/gate-diff/git.ts @@ -85,24 +85,37 @@ export function isShallow(cwd: string): boolean { /** * The base as a ref that resolves here and, when `head` is given, shares - * history with it: the ref itself, else its remote-tracking branch - * (`origin/main` for `main`). A clone that checked out only the feature - * branch has no local `main`, and the merge-base against `origin/main` is the - * same commit; a stale local `main` that shares no history with HEAD (a - * fetched-over clone) yields to the twin too, so the fix line's fetch into the - * tracking ref is a fix and not a loop. + * history with it. A branch name resolves to its remote-tracking branch + * (`origin/main` for `main`, origin first) as review-collect's collectLive and + * review-dispatch do: a stale local `main` behind `origin/main` would scan + * upstream commits the branch only inherited (GST-883). The local branch is the + * fallback when no remote has the branch, or only one that shares no history + * with HEAD (`local: 'fallback'`, naming it as `unrelatedRemote`; a repository + * with no remote at all has nothing to lag behind, so it is plain). A clone + * that checked out only the feature branch has no local `main` (`missing`); a + * local `main` that shares no history with HEAD is `unrelated`, so the fix + * line's fetch into the tracking ref is a fix and not a loop. Branches are + * read by their full names (`refs/heads/`, `refs/remotes/`), so a tag named + * `main` or `origin/main` cannot stand in for them. */ -export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' } | null { +export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'fallback'; unrelatedRemote?: string } | null { const related = (sha: string) => !head || mergeBase(sha, head, cwd) !== null; - const direct = resolvesToCommit(ref, cwd); - if (direct && related(direct)) return { ref, sha: direct }; - if (!isBranchName(ref)) return direct ? { ref, sha: direct } : null; - for (const remote of remoteNames(cwd)) { + const direct = () => resolvesToCommit(ref, cwd); + if (!isBranchName(ref)) { const sha = direct(); return sha ? { ref, sha } : null; } + const qualified = resolvesToCommit(`refs/remotes/${ref}`, cwd); + if (qualified) return { ref, sha: qualified }; + const local = resolvesToCommit(`refs/heads/${ref}`, cwd); + const remotes = remoteNames(cwd); + let unrelatedRemote: string | undefined; + for (const remote of remotes) { const tracking = `${remote}/${ref}`; - const sha = resolvesToCommit(tracking, cwd); - if (sha && related(sha)) return { ref: tracking, sha, local: direct ? 'unrelated' : 'missing' }; + const sha = resolvesToCommit(`refs/remotes/${tracking}`, cwd); + if (sha && related(sha)) return { ref: tracking, sha, ...(!local ? { local: 'missing' as const } : !related(local) ? { local: 'unrelated' as const } : {}) }; + if (sha) unrelatedRemote ??= tracking; } - return direct ? { ref, sha: direct } : null; + if (local) return { ref, sha: local, ...(remotes.length ? { local: 'fallback' as const, ...(unrelatedRemote ? { unrelatedRemote } : {}) } : {}) }; + const sha = direct(); + return sha ? { ref, sha } : null; } /** A word for a printed shell line: as is when it is plain, single-quoted otherwise (ref names may hold `;`, `$`, `(`, `{1,2}`). */ diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7fbfdae12..1b15bf12a 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -171,7 +171,7 @@ export function collectCommit(root: string, baseSha: string, headSha: string): C /** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ export function collectLive(root: string, base: string): Collected { const errors: DepthError[] = []; - const ref = git(root, ['rev-parse', '--verify', '-q', `origin/${base}^{commit}`]).ok ? `origin/${base}` : base; + const ref = [`refs/remotes/origin/${base}`, `refs/heads/${base}`].find(r => git(root, ['rev-parse', '--verify', '-q', `${r}^{commit}`]).ok) ?? base; const registry = loadRegistryAt(root, ref); const mb = git(root, ['merge-base', ref, 'HEAD']); if (!mb.ok) { From 76ec4259aa8015ca1c4f9fc655592e025d4571a1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:12:08 +0000 Subject: [PATCH 13/35] docs(changelog): GST-883 fragment Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-883-review-plan-origin-base.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog.d/gst-883-review-plan-origin-base.md diff --git a/changelog.d/gst-883-review-plan-origin-base.md b/changelog.d/gst-883-review-plan-origin-base.md new file mode 100644 index 000000000..10c43e6ba --- /dev/null +++ b/changelog.d/gst-883-review-plan-origin-base.md @@ -0,0 +1,8 @@ +**A stale local `main` no longer turns a /ship review into a full-depth gate review (GST-883).** `gstack-review-plan` handed the raw base name to `gstack-gate-diff`, which diffed against the local branch. With a local `main` four commits behind `origin/main`, the gate scan read a test edit that was already on `origin/main`, so a plain code diff planned as gate class at full depth. On gstack-evals#123 that cost about 15-20M tokens of readers the change did not need (GEVA-117). + +### Fixed +- **`gstack-gate-diff `** resolves a branch name to its remote-tracking branch (`origin/main` for `main`, origin first), the way `gstack-review-plan`'s own diff and review dispatch already did (#3780). It uses the local branch only when no remote has the branch, and says so on stderr: `no remote-tracking branch for `, or `/ shares no history with HEAD`. A repository with no remote at all uses the local branch without a note. Branches are read by their full ref names, so a tag named `main` or `origin/main` can no longer stand in for the base and hide committed edits from the scan. +- **`gstack-review-plan`** passes the merge base its own diff used to `gstack-diff-scope` and `gstack-gate-diff`, so the scope flags, the gate scan and the line counts all read the same commits. Its diff reads `refs/remotes/origin/` and `refs/heads/` by full name too. + +### Tests +- `test/review-plan-origin-base.test.ts`: a fixture whose `origin/main` is one commit ahead of the local `main`, with a test edit on `origin/main` and a branch forked from it. `gstack-review-plan --base main` and `gstack-gate-diff main` plan the same class, depth and gate-tagged count as with a fresh `main` (before the fix: gate, full, `RH-1:1` against code, light, none). A tag named `origin/main` or `main` at HEAD leaves the plan and the scanned range unchanged. Also covers the stderr note for a branch no remote has, an unrelated `origin/main`, and the quiet path with no remote. From d603162add7703eaad33d96b9a89fa325ae12687 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 02:12:37 +0000 Subject: [PATCH 14/35] test(review-risk): hostile-input time bounds for every risk pattern; the R51 rm and rmSync rules fail (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-risk.ts | 4 +-- test/review-risk-redos.test.ts | 61 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 test/review-risk-redos.test.ts diff --git a/lib/review-risk.ts b/lib/review-risk.ts index c1ed328a1..50b152edb 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -33,7 +33,7 @@ export const SECRET_LINE = [ ]; const SECURITY_PATH_TOKENS = ['trust', 'redact', 'redaction', 'redactor', 'sanitize', 'sanitizer', 'neutralize', 'neutralizer', 'consent', 'escape']; -const SECURITY_LINE = [ +export const SECURITY_LINE = [ /\bshell\s*:\s*true\b/, /\bshell\s*=\s*True\b/, /(? unknown) => { const t = performance.now(); f(); return performance.now() - t; }; +const run = (anchor: string, unit: string, tail = '') => [JSON.stringify(anchor) + ' + ' + JSON.stringify(unit) + '*n' + (tail ? ' + ' + JSON.stringify(tail) : ''), anchor + unit.repeat(Math.ceil(L / unit.length)) + tail] as const; +const security = (line: string) => matchRisk({ codePaths: ['x.sh'], added: { 'x.sh': [line] }, removed: {}, scopeAuth: false }).has('security'); + +describe('linear time on hostile input', () => { + // Value: protects=gstack-review-plan, the targeted pass and the queue depth check never stall on one added line; + // fails_when=any risk pattern backtracks quadratically on its anchor followed by a long run of the class it repeats, + // or on a line of repeated anchors; seam=every pattern in SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE and SECRET_PATH + const hostile = [ + run('rm -', 'r'), run('rm -', 'f'), run('rm -r', 'r'), run('rm -f', 'a'), run('rm\t-', 'r'), run('rm ', ' '), run('rm -', 'r', '1'), run('rm -', 'rf', '_'), run('', 'rm -r '), + run('', 'rmSync('), run('rmSync(', ' '), run('rmSync(', 'recursive '), run('rmSync(', 'recursive: '), + run('shell', ' '), run('shell:', ' '), run('shell=', ' '), run('', '; '), run(';', ' '), run('; eval', ' '), run('eval', ' '), run('', ';eval '), + run('execSync', ' '), run('child_process.exec', ' '), run('os.system', ' '), run('new', ' '), run('new Function', ' '), + run('spawn(', ' '), run('Bun.spawn(', ' '), run('Bun.spawn([', ' '), run('innerHTML', ' '), + run('tar -', 'A'), run('tar x', 'A'), run('tar', ' '), run('redirect', ' '), run('redirect:', ' '), run('maxRedirects', ' '), + run('DELETE', ' '), run('drop', ' '), run('truncate', ' '), run('delete from', ' '), run('delete from ', 'a.'), run('', 'delete from a '), run('.deleteMany', ' '), + run('', 'a_'), run('', 'aU'), run('a', 'A'), run('spend', '_'), run('new Anthropic', ' '), run('.messages.create', ' '), + run('$', 'A_'), run('${', 'A_'), run('', '$A_'), run('process.env.', 'A_'), run('process.env[', ' '), run('os.environ[', ' '), run('os.getenv(', ' '), run('ENV[', ' '), + ]; + test.each(Object.entries({ SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE, SECRET_PATH: [SECRET_PATH] }))('every %s pattern finishes under 100 ms on each 100k-character hostile line', (_n, patterns) => { + const slow = patterns.flatMap(r => hostile.map(([label, s]) => [r.source.slice(0, 50), label, Math.round(ms(() => r.test(s)))] as const)).filter(([, , t]) => t >= 100); + expect(slow).toEqual([]); + }, 120_000); + test("matchRisk on 'rm -' and 100,000 r's (the GST-898 repro) returns in under 250 ms and raises no class", () => { + const line = 'rm -' + 'r'.repeat(100_000); + let hit = true; + expect(ms(() => { hit = security(line); })).toBeLessThan(250); + expect(hit).toBe(false); + }, 60_000); +}); + +describe('the rm rule matches what R51 matched', () => { + // Value: protects=the rewrite changes no verdict; fails_when=the linear rm pattern matches a line R51's pattern did not, or misses + // one it did; seam=SECURITY_LINE's rm pattern against R51's original source + const r51 = /\brm\s+-(?:[a-z]*r[a-z]*f|[a-z]*f[a-z]*r)\b/; + const rm = SECURITY_LINE.find(r => r.source.startsWith(String.raw`\brm\s+-`))!; + test('the same verdict on 60,000 seeded short lines and the hand-picked edges', () => { + let seed = 898; + const next = (n: number) => { seed = (seed * 1103515245 + 12345) % 2 ** 31; return seed % n; }; + const prefixes = ['rm -', 'rm -', 'xrm -', ' rm\t-', 'rm-', 'rm -- ', '"rm -', 'brm -', 'rm -r ', '']; + const alphabet = 'rfFRa1_- .x/m'; + const lines = Array.from({ length: 60_000 }, () => prefixes[next(prefixes.length)] + Array.from({ length: next(11) }, () => alphabet[next(alphabet.length)]).join('')); + const edges = ['rm -rf /', 'rm -fr x', 'rm -Rf x', 'rm -rF x', 'rm -rvf x', 'rm -frv', 'rm -r -f x', 'rm -rf1', 'rm -rf_x', 'rm -rfx', 'rm -rf-x', 'rm -f', 'rm -r', 'rm -rrf', 'rm -ffr', 'rm -rfr', 'farm -rf x', 'rm-rf x', 'rm\t\t-rf', 'sudo rm -rf "$DIR"']; + const differ = [...lines, ...edges].filter(s => rm.test(s) !== r51.test(s)); + expect(differ).toEqual([]); + expect(edges.filter(s => rm.test(s))).toEqual(['rm -rf /', 'rm -fr x', 'rm -rvf x', 'rm -rf-x', 'rm -rrf', 'rm -ffr', 'rm -rfr', 'rm\t\t-rf', 'sudo rm -rf "$DIR"']); + }); + test('rmSync with recursive: true still raises security, up to 500 characters into the call', () => { + expect(security('fs.rmSync(dir, { recursive: true, force: true });')).toBe(true); + expect(security('fs.rmSync(' + 'a'.repeat(480) + ', { recursive: true })')).toBe(true); + }); +}); From 36af6b721175a6bb1c0fe40efea1c79df1cea19d Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 02:15:07 +0000 Subject: [PATCH 15/35] fix(review-risk): the R51 rm and rmSync rules run in linear time (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-898-rm-rule-redos.md | 2 ++ lib/review-risk.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 changelog.d/gst-898-rm-rule-redos.md diff --git a/changelog.d/gst-898-rm-rule-redos.md b/changelog.d/gst-898-rm-rule-redos.md new file mode 100644 index 000000000..eb4b26132 --- /dev/null +++ b/changelog.d/gst-898-rm-rule-redos.md @@ -0,0 +1,2 @@ +- **One hostile line no longer stalls review classification** (GST-898). R51's `rm -rf` rule in `lib/review-risk.ts` took quadratic time on a long flag run: one added line of `rm -` and 100,000 `r` characters held `matchRisk` for about 3.7 s, and each doubling took four times as long. The `rmSync(… recursive: true)` rule did the same on a line of repeated `rmSync(` calls. Both now run in linear time (the same line takes about 1 ms). The `rm` rule is rewritten with lookaheads and matches exactly the lines it matched before, checked against 60,000 seeded lines. The `rmSync` rule now looks at most 500 characters into the call, the bound GST-850 uses. A slow match never lowered review depth: `gstack-review-dispatch pass` stops when the plan times out, and the queue recomputes depth itself. +- A new test holds every risk pattern under 100 ms on 100,000-character hostile lines. Over the last 100 main commits, no commit changes depth, class or risk class, and none of the 69,023 changed lines gets a different verdict from either rule. diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 50b152edb..48b720e46 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -37,7 +37,7 @@ export const SECURITY_LINE = [ /\bshell\s*:\s*true\b/, /\bshell\s*=\s*True\b/, /(? Date: Mon, 2 Nov 2026 02:16:05 +0000 Subject: [PATCH 16/35] test(review-packets-removed): repin lib/review-risk.ts on the GST-898 verdict-identity replay Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-packets-removed.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 747723390..afd641f87 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -57,6 +57,7 @@ const NO_LOSS_REPLAYS = { 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", '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", + 'GST-898': "GST-898 verdict-identity no-loss check: the linear rm and rmSync rules give the same verdict as R51's on every changed line of the last 100 main commits (69,023 lines, a563f47 back 100 first-parent commits) and on 60,000 seeded lines (test/review-risk-redos.test.ts), and no commit changes depth, class or risk class, so every diff dispatches main's reviewers unchanged; reviewer text is unchanged", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; const REVIEWER_BRIEFS: Record = { @@ -79,7 +80,7 @@ const REVIEWER_BRIEFS: Record = { '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-risk.ts': { sha: '8810578c95c44d53cb3bdd96589b714acb8eca7fc3f7a9a808f1baba065ce6fb', replay: 'GST-898' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { From 45c13ce06a4eca1379f0c11643569d6a43445a9b Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 02:30:29 +0000 Subject: [PATCH 17/35] fix(review-risk): padding cannot push recursive: true out of the rmSync window; repin on the GST-898 no-loss replay Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-898-rm-rule-redos.md | 4 ++-- lib/review-risk.ts | 2 +- test/review-packets-removed.test.ts | 4 ++-- test/review-risk-redos.test.ts | 20 ++++++++++++++++---- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/changelog.d/gst-898-rm-rule-redos.md b/changelog.d/gst-898-rm-rule-redos.md index eb4b26132..13cb6ef38 100644 --- a/changelog.d/gst-898-rm-rule-redos.md +++ b/changelog.d/gst-898-rm-rule-redos.md @@ -1,2 +1,2 @@ -- **One hostile line no longer stalls review classification** (GST-898). R51's `rm -rf` rule in `lib/review-risk.ts` took quadratic time on a long flag run: one added line of `rm -` and 100,000 `r` characters held `matchRisk` for about 3.7 s, and each doubling took four times as long. The `rmSync(… recursive: true)` rule did the same on a line of repeated `rmSync(` calls. Both now run in linear time (the same line takes about 1 ms). The `rm` rule is rewritten with lookaheads and matches exactly the lines it matched before, checked against 60,000 seeded lines. The `rmSync` rule now looks at most 500 characters into the call, the bound GST-850 uses. A slow match never lowered review depth: `gstack-review-dispatch pass` stops when the plan times out, and the queue recomputes depth itself. -- A new test holds every risk pattern under 100 ms on 100,000-character hostile lines. Over the last 100 main commits, no commit changes depth, class or risk class, and none of the 69,023 changed lines gets a different verdict from either rule. +- **One hostile line no longer stalls review classification** (GST-898). R51's `rm -rf` rule in `lib/review-risk.ts` took quadratic time on a long flag run: one added line of `rm -` and 100,000 `r` characters held `matchRisk` for about 3.7 s, and each doubling took four times as long. The `rmSync(… recursive: true)` rule did the same on a line of repeated `rmSync(` calls. Both now run in linear time (the same line takes about 1 ms). The `rm` rule is rewritten with lookaheads and matches exactly the lines it matched before. The `rmSync` rule looks for `recursive: true` in the first 500 characters of the call, and a call with more than 500 characters before its closing parenthesis now counts as security on its own, so padding can't push the flag out of view. A slow match never lowered review depth: `gstack-review-dispatch pass` stops when the plan times out, and the queue recomputes depth itself. +- A new test holds every risk pattern under 100 ms on 100,000-character hostile lines and checks both rules against R51's on 60,000 seeded lines each. Over the last 100 main commits, no commit changes depth, class or risk class, and none of the 69,023 changed lines gets a different verdict from either rule. diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 48b720e46..9d4d556e5 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -37,7 +37,7 @@ export const SECURITY_LINE = [ /\bshell\s*:\s*true\b/, /\bshell\s*=\s*True\b/, /(? = { @@ -80,7 +80,7 @@ const REVIEWER_BRIEFS: Record = { '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: '8810578c95c44d53cb3bdd96589b714acb8eca7fc3f7a9a808f1baba065ce6fb', replay: 'GST-898' }, + 'lib/review-risk.ts': { sha: '94229a202f0eaa4887a11ed384ba18cf43172d54278092d74510d3c66046d93b', replay: 'GST-898' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-risk-redos.test.ts b/test/review-risk-redos.test.ts index 605e30c94..3e2503834 100644 --- a/test/review-risk-redos.test.ts +++ b/test/review-risk-redos.test.ts @@ -43,19 +43,31 @@ describe('the rm rule matches what R51 matched', () => { // one it did; seam=SECURITY_LINE's rm pattern against R51's original source const r51 = /\brm\s+-(?:[a-z]*r[a-z]*f|[a-z]*f[a-z]*r)\b/; const rm = SECURITY_LINE.find(r => r.source.startsWith(String.raw`\brm\s+-`))!; + let x = 898; + const next = (n: number) => { x ^= x << 13; x ^= x >>> 17; x ^= x << 5; return (x >>> 0) % n; }; test('the same verdict on 60,000 seeded short lines and the hand-picked edges', () => { - let seed = 898; - const next = (n: number) => { seed = (seed * 1103515245 + 12345) % 2 ** 31; return seed % n; }; + x = 898; const prefixes = ['rm -', 'rm -', 'xrm -', ' rm\t-', 'rm-', 'rm -- ', '"rm -', 'brm -', 'rm -r ', '']; - const alphabet = 'rfFRa1_- .x/m'; + const alphabet = 'rfrfrfFRa1_- .x/m'; const lines = Array.from({ length: 60_000 }, () => prefixes[next(prefixes.length)] + Array.from({ length: next(11) }, () => alphabet[next(alphabet.length)]).join('')); const edges = ['rm -rf /', 'rm -fr x', 'rm -Rf x', 'rm -rF x', 'rm -rvf x', 'rm -frv', 'rm -r -f x', 'rm -rf1', 'rm -rf_x', 'rm -rfx', 'rm -rf-x', 'rm -f', 'rm -r', 'rm -rrf', 'rm -ffr', 'rm -rfr', 'farm -rf x', 'rm-rf x', 'rm\t\t-rf', 'sudo rm -rf "$DIR"']; const differ = [...lines, ...edges].filter(s => rm.test(s) !== r51.test(s)); expect(differ).toEqual([]); + expect(lines.filter(s => r51.test(s)).length).toBeGreaterThan(1000); expect(edges.filter(s => rm.test(s))).toEqual(['rm -rf /', 'rm -fr x', 'rm -rvf x', 'rm -rf-x', 'rm -rrf', 'rm -ffr', 'rm -rfr', 'rm\t\t-rf', 'sudo rm -rf "$DIR"']); }); - test('rmSync with recursive: true still raises security, up to 500 characters into the call', () => { + test('the rmSync rule matches what R51 matched on 60,000 seeded short lines, and padding past its window still raises security', () => { + x = 898; + const r51Sync = /\brmSync\s*\([^)]*recursive\s*:\s*true/; + const rmSync = SECURITY_LINE.find(r => r.source.startsWith(String.raw`\brmSync`))!; + const parts = ['rmSync(', 'rmSync (', 'xrmSync(', 'recursive', ':', ' ', 'true', ')', 'a', 'recursive: true', 'rmSync', '(', '\t']; + const lines = Array.from({ length: 60_000 }, () => Array.from({ length: next(9) + 1 }, () => parts[next(parts.length)]).join('')); + expect(lines.filter(s => rmSync.test(s) !== r51Sync.test(s))).toEqual([]); + expect(lines.filter(s => r51Sync.test(s)).length).toBeGreaterThan(1000); expect(security('fs.rmSync(dir, { recursive: true, force: true });')).toBe(true); expect(security('fs.rmSync(' + 'a'.repeat(480) + ', { recursive: true })')).toBe(true); + expect(security('fs.rmSync(dir, /*' + 'x'.repeat(501) + '*/ { recursive: true, force: true });')).toBe(true); + expect(security('fs.rmSync(dir, /*' + 'x'.repeat(5_000_000) + '*/ { recursive: true });')).toBe(true); + expect(security('fs.rmSync(dir, { force: true });')).toBe(false); }); }); From c803339b8e1faf4c763b0bccf5a7e185dbcd3ec4 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 02:41:00 +0000 Subject: [PATCH 18/35] docs(changelog): GST-898 fragment wording Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-898-rm-rule-redos.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/gst-898-rm-rule-redos.md b/changelog.d/gst-898-rm-rule-redos.md index 13cb6ef38..42653c651 100644 --- a/changelog.d/gst-898-rm-rule-redos.md +++ b/changelog.d/gst-898-rm-rule-redos.md @@ -1,2 +1,2 @@ -- **One hostile line no longer stalls review classification** (GST-898). R51's `rm -rf` rule in `lib/review-risk.ts` took quadratic time on a long flag run: one added line of `rm -` and 100,000 `r` characters held `matchRisk` for about 3.7 s, and each doubling took four times as long. The `rmSync(… recursive: true)` rule did the same on a line of repeated `rmSync(` calls. Both now run in linear time (the same line takes about 1 ms). The `rm` rule is rewritten with lookaheads and matches exactly the lines it matched before. The `rmSync` rule looks for `recursive: true` in the first 500 characters of the call, and a call with more than 500 characters before its closing parenthesis now counts as security on its own, so padding can't push the flag out of view. A slow match never lowered review depth: `gstack-review-dispatch pass` stops when the plan times out, and the queue recomputes depth itself. +- **One hostile line no longer stalls review classification** (GST-898). R51's `rm -rf` rule in `lib/review-risk.ts` took quadratic time on a long flag run. One added line of `rm -` and 100,000 `r` characters held `matchRisk` for about 3.7 s, and each doubling took four times as long. The `rmSync(… recursive: true)` rule did the same on a line of repeated `rmSync(` calls. Both now run in linear time (the same line takes about 1 ms). The `rm` rule is rewritten with lookaheads and matches exactly the lines it matched before. The `rmSync` rule looks for `recursive: true` in the first 500 characters of the call, and a call with more than 500 characters before its closing parenthesis now counts as security on its own, so padding can't push the flag out of view. A slow match never lowered review depth: `gstack-review-dispatch pass` stops when the plan times out, and the queue recomputes depth itself. - A new test holds every risk pattern under 100 ms on 100,000-character hostile lines and checks both rules against R51's on 60,000 seeded lines each. Over the last 100 main commits, no commit changes depth, class or risk class, and none of the 69,023 changed lines gets a different verdict from either rule. From 4250be9c45745ce6f7ca359401894bc626827f7b Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 06:04:26 +0000 Subject: [PATCH 19/35] docs(review-policy): the linear-time claim covers R51's rm and rmSync rules (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index 51182ed28..f93c447d5 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -219,8 +219,7 @@ or removed), and any changed line that names the `child_process` or rules are coarse on purpose and parse no import clause or call arguments; `test/review-risk-linear.test.ts` holds each of them under 100 ms on 200,000-character hostile lines, its anchor followed by a long run of the -characters it repeats included. That claim covers only the rules GST-850 adds, -not R51's older ones. **data** is user data: a path naming pii, gdpr, ccpa, privacy, +characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` call with more than 500 characters before its closing parenthesis counts as security on its own, so padding cannot hide a recursive delete. **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 From 99646574cd1e498ae29867f44f0abf405aa5e21c Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 06:09:48 +0000 Subject: [PATCH 20/35] docs(review-policy): state the rmSync padding rule exactly (GST-898) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index f93c447d5..a626bab94 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -219,7 +219,7 @@ or removed), and any changed line that names the `child_process` or rules are coarse on purpose and parse no import clause or call arguments; `test/review-risk-linear.test.ts` holds each of them under 100 ms on 200,000-character hostile lines, its anchor followed by a long run of the -characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` call with more than 500 characters before its closing parenthesis counts as security on its own, so padding cannot hide a recursive delete. **data** is user data: a path naming pii, gdpr, ccpa, privacy, +characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` call with more than 500 characters before the first `)` after it counts as security on its own. **data** is user data: a path naming pii, gdpr, ccpa, privacy, personal, account, customer, cookie, persistence, storage or database, or a line that drops a table or column, deletes rows or bulk-deletes records. Auth scope, migrations and deleted code paths were already gate class. `RISK_RULES` hashes From f254cebf1200c20cd48af4d2d44fd032c0192aad Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 03:13:34 +0000 Subject: [PATCH 21/35] 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 22/35] 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 23/35] 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 24/35] 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 25/35] 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 26/35] 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 27/35] 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 28/35] 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 29/35] 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' }, }; From b3d0eb8ad8572af0a5c8a3a6ed87609eed8101ca Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:01:34 +0000 Subject: [PATCH 30/35] fix(review-depth): linear url() refusal in svgDrawing (wave review-2, red-team CRITICAL) SVG_REFUSED's url() entry backtracked quadratically on url( followed by a long whitespace run (80k spaces: 820 ms; 1 MB: over 230 s), and svgDrawing runs in collectLive and in collectCommit, which ship-queue ready, train build and land call with no timeout. The rewritten entry refuses exactly what #3838's pattern refused. Repin lib/review-depth.ts under no-loss replay GST-911-redos (verdict identity: 60,000 seeded strings, the edges and every SVG blob in history, zero differences). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 2 +- test/review-packets-removed.test.ts | 3 ++- test/review-risk-redos.test.ts | 29 +++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 6697679e4..691733879 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -85,7 +85,7 @@ 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 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]; +export 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_OPEN = /<(?:[A-Za-z_][\w.-]*:)?style\b/gi; const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style\s*>/i; diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index ec2f95dde..4206a69bb 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -57,6 +57,7 @@ const NO_LOSS_REPLAYS = { 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", 'GST-850': "GST-850 dispatch-superset no-loss check: on the 26 GSTA-167 no-loss units the branch plan (wider security and money classes, write-up text in the batched diff) drops no reviewer role main dispatches, so no saved blocker can be lost, and over the last 100 main commits no commit with a gate or risk class drops depth (test/review-risk-routing.test.ts, test/review-risk-linear.test.ts); reviewer text is unchanged", + 'GST-911-redos': "GST-911 ReDoS fix verdict-identity no-loss check (wave review-e7b551ebd865): SVG_REFUSED's url() entry, rewritten to run in linear time, refuses exactly the strings #3838's pattern refused (test/review-risk-redos.test.ts: 60,000 seeded strings and the hand-picked edges, and every SVG blob in the repository's history, with zero differences), so GST-911's no-loss replay holds unchanged; reviewer text is unchanged", 'GST-911': "GST-911 no-loss replay (GSTA-249/13): the planner over the last 100 first-parent main commits to f5cb6ec1e, origin/main's code against this branch, gate scan fixed at 0, changes no commit's depth, class, risk classes, targeted classes or SCOPE_AUTH; the three misreads (a drawing-only SVG, a session-named data file, a bare base) are proven on fixtures (test/review-planner-misreads.test.ts), where every SVG that can run script stays code with the security class; reviewer text is unchanged", 'R51': "R51 risk-tier routing no-loss re-score (GSTA-167): the saved panel findings (349 records over 24 of the 26 no-loss units) re-scored against the branch's plan lose no blocker whose only finders are roles it drops, and every gate, security, user-data and money fixture keeps main's reviewers plus the full panel (test/review-targeted.test.ts, test/review-risk-routing.test.ts); reviewer text is unchanged", 'GST-898': "GST-898 verdict-identity no-loss check: the linear rm rule, and the rmSync rule (which also raises security on any call with more than 500 characters before its closing parenthesis), give the same verdict as R51's on every changed line of the last 100 main commits (69,023 lines, a563f47 back 100 first-parent commits) and on 60,000 seeded lines per rule (test/review-risk-redos.test.ts), and no commit changes depth, class or risk class, so every diff dispatches main's reviewers unchanged; reviewer text is unchanged", @@ -81,7 +82,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: '17a78ec841a62e86e772dcb13e84ef10102db7a24cf4cf8af618c41436d368d5', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: 'b16f28d6eecc162928d58fcfc6209f703b9692bfe5e901d381972939d2e16f7e', replay: 'GST-911-redos' }, 'lib/review-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-911' }, }; diff --git a/test/review-risk-redos.test.ts b/test/review-risk-redos.test.ts index 3e2503834..7d40d9df3 100644 --- a/test/review-risk-redos.test.ts +++ b/test/review-risk-redos.test.ts @@ -4,6 +4,7 @@ * class it repeats, and the rewritten rm rule matches exactly the lines R51's pattern matched. */ import { describe, expect, test } from 'bun:test'; +import { SVG_REFUSED, svgDrawing } from '../lib/review-depth'; import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_LINE, matchRisk } from '../lib/review-risk'; const L = 100_000; @@ -71,3 +72,31 @@ describe('the rm rule matches what R51 matched', () => { expect(security('fs.rmSync(dir, { force: true });')).toBe(false); }); }); + +describe('svgDrawing in linear time', () => { + // Value: protects=gstack-review-plan and the commit collector behind ship-queue ready, train build and land never stall on one + // changed SVG; fails_when=a whole-text SVG pattern backtracks quadratically on a long whitespace run, or the linear url() + // refusal reads any url( differently from #3838's pattern; seam=SVG_REFUSED's url() entry and svgDrawing + const old = /url\(\s*(?!['"]?\s*#)/i; + test('a 1 MB whitespace run in url(), an attribute, a style element and an href returns in under 250 ms each', () => { + const pad = ' '.repeat(1_000_000); + const cases = [ + ['', true], ['', false], + ['', true], ['', true], + ['', true], ['' + pad + '', true], + ] as const; + const wrong = cases.map(([s, want], i) => { let got = !want; const t = ms(() => { got = svgDrawing(s); }); return [i, got === want, Math.round(t)] as const; }).filter(([, ok, t]) => !ok || t >= 250); + expect(wrong).toEqual([]); + }, 60_000); + test("the url() refusal matches #3838's pattern on 60,000 seeded short strings and the hand-picked edges", () => { + const url = SVG_REFUSED.find(r => r.source.startsWith('url'))!; + let x = 911; + const next = (n: number) => { x ^= x << 13; x ^= x >>> 17; x ^= x << 5; return (x >>> 0) % n; }; + const parts = ['url(', 'URL(', ' ', '\t', '\n', '#', "'", '"', 'a', '#a', ')', 'http:', '(', 'u']; + const lines = Array.from({ length: 60_000 }, () => Array.from({ length: next(8) + 1 }, () => parts[next(parts.length)]).join('')); + const edges = ['url(#a)', 'url( #a)', "url('#a')", "url( '#a')", 'url( "#a")', 'url(" #a")', 'url(\t#a)', 'url(x)', 'url( x)', "url('http://x')", 'url(', 'url( ', "url('')"]; + expect([...lines, ...edges].filter(s => url.test(s) !== old.test(s))).toEqual([]); + expect(lines.filter(s => old.test(s)).length).toBeGreaterThan(1000); + expect(lines.filter(s => /url\(/i.test(s) && !old.test(s)).length).toBeGreaterThan(100); + }); +}); From 86d0414fc15027ad6db9f2ddfc5a18df8ee62530 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:05:51 +0000 Subject: [PATCH 31/35] fix: pre-landing review fixes (wave review-2, pass 1) - bin/gstack-diff-scope: _resolve_base takes a related local before another remote's twin, the order resolveBase uses, so the two tools pick the same base for a bare name when origin/ is unrelated. - lib/review-depth.ts: a drawing's DOCTYPE names no document but W3C's SVG DTD; SYSTEM and other PUBLIC identifiers keep the SVG code. Repin lib/review-depth.ts under no-loss replay GST-911-redos (the DOCTYPE change only keeps more SVGs code, so it drops no reviewer). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 14 ++++++++------ lib/review-depth.ts | 4 ++-- test/diff-scope.test.ts | 13 +++++++++++++ test/review-packets-removed.test.ts | 4 ++-- test/review-planner-misreads.test.ts | 4 +++- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index dcbb0311a..7609aefbf 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -53,8 +53,9 @@ 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 +# refs/remotes/origin/ that shares history with the head, then refs/heads/ +# that does, then another remote's twin that does, then refs/heads/ as it is, +# all by full ref name and in resolveBase's order, 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. @@ -64,14 +65,15 @@ _is_branch_name() { } _commit() { git rev-parse --verify -q "$1^{commit}" 2>/dev/null; } _resolve_base() { - local ref="$1" head="$2" sha remote + local ref="$1" head="$2" sha loc 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 + loc=$(_commit "refs/heads/$ref") + for remote in $(git remote 2>/dev/null | grep -x origin) ${loc:+:local} $(git remote 2>/dev/null | grep -vx origin); do + if [ "$remote" = :local ]; then sha=$loc; else sha=$(_commit "refs/remotes/$remote/$ref") || continue; fi git merge-base "$sha" "$head" >/dev/null 2>&1 && { echo "$sha"; return; } done - sha=$(_commit "refs/heads/$ref") && { echo "$sha"; return; } + [ -n "$loc" ] && { echo "$loc"; return; } _commit "$ref" } diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 691733879..59340ac8e 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -79,8 +79,8 @@ const SVG_ELEMENTS = new Set([ '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; +/** Markup a drawing may hold besides elements: comments, CDATA, the XML declaration, and a DOCTYPE that names no document but W3C's SVG DTD. */ +const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg(?:\s+PUBLIC\s+(["'])-\/\/W3C\/\/DTD SVG [\w .]+\/\/EN\1\s+(["'])https?:\/\/www\.w3\.org\/[\w.\/-]+\.dtd\2)?\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. diff --git a/test/diff-scope.test.ts b/test/diff-scope.test.ts index 05192b07e..dc2f75835 100644 --- a/test/diff-scope.test.ts +++ b/test/diff-scope.test.ts @@ -9,6 +9,7 @@ import { mkdtempSync, writeFileSync, mkdirSync, rmSync, readFileSync } from 'fs' import { join } from 'path'; import { tmpdir } from 'os'; import { spawnSync } from 'child_process'; +import { resolveBase } from '../lib/gate-diff/git'; const SCRIPT = join(import.meta.dir, '..', 'bin', 'gstack-diff-scope'); @@ -380,6 +381,18 @@ describe('a bare base name reads origin/ when it exists (#3650)', () => { expect(scopeAt(dir, 'main').SCOPE_BACKEND).toBe('true'); expect(upstreamCommit).not.toBe(''); }); + + test('with origin/main unrelated, a related local main comes before a related other remote, the base resolveBase picks', () => { + const { dir, git } = behindBase('main'); + git('remote', 'add', 'upstream', 'file:///nonexistent-upstream'); + git('update-ref', 'refs/remotes/upstream/main', git('rev-parse', 'origin/main')); + git('checkout', '-q', '--orphan', 'island'); + git('commit', '-q', '--allow-empty', '-m', 'unrelated'); + git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + git('checkout', '-q', 'feature/test'); + expect(resolveBase('main', dir, 'HEAD')?.sha).toBe(git('rev-parse', 'main')); + expect(scopeAt(dir, 'main').SCOPE_BACKEND).toBe('true'); + }); }); describe('uncommitted work is visible (#2299)', () => { diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 4206a69bb..1ae42964e 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -57,7 +57,7 @@ const NO_LOSS_REPLAYS = { 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", 'GST-850': "GST-850 dispatch-superset no-loss check: on the 26 GSTA-167 no-loss units the branch plan (wider security and money classes, write-up text in the batched diff) drops no reviewer role main dispatches, so no saved blocker can be lost, and over the last 100 main commits no commit with a gate or risk class drops depth (test/review-risk-routing.test.ts, test/review-risk-linear.test.ts); reviewer text is unchanged", - 'GST-911-redos': "GST-911 ReDoS fix verdict-identity no-loss check (wave review-e7b551ebd865): SVG_REFUSED's url() entry, rewritten to run in linear time, refuses exactly the strings #3838's pattern refused (test/review-risk-redos.test.ts: 60,000 seeded strings and the hand-picked edges, and every SVG blob in the repository's history, with zero differences), so GST-911's no-loss replay holds unchanged; reviewer text is unchanged", + 'GST-911-redos': "GST-911 ReDoS fix verdict-identity no-loss check (wave review-e7b551ebd865): SVG_REFUSED's url() entry, rewritten to run in linear time, refuses exactly the strings #3838's pattern refused (test/review-risk-redos.test.ts: 60,000 seeded strings and the hand-picked edges, and every SVG blob in the repository's history, with zero differences); SVG_MARKUP now refuses a DOCTYPE naming any document but W3C's SVG DTD (test/review-planner-misreads.test.ts), which only keeps more SVGs code and so drops no reviewer; GST-911's no-loss replay holds unchanged; reviewer text is unchanged", 'GST-911': "GST-911 no-loss replay (GSTA-249/13): the planner over the last 100 first-parent main commits to f5cb6ec1e, origin/main's code against this branch, gate scan fixed at 0, changes no commit's depth, class, risk classes, targeted classes or SCOPE_AUTH; the three misreads (a drawing-only SVG, a session-named data file, a bare base) are proven on fixtures (test/review-planner-misreads.test.ts), where every SVG that can run script stays code with the security class; reviewer text is unchanged", 'R51': "R51 risk-tier routing no-loss re-score (GSTA-167): the saved panel findings (349 records over 24 of the 26 no-loss units) re-scored against the branch's plan lose no blocker whose only finders are roles it drops, and every gate, security, user-data and money fixture keeps main's reviewers plus the full panel (test/review-targeted.test.ts, test/review-risk-routing.test.ts); reviewer text is unchanged", 'GST-898': "GST-898 verdict-identity no-loss check: the linear rm rule, and the rmSync rule (which also raises security on any call with more than 500 characters before its closing parenthesis), give the same verdict as R51's on every changed line of the last 100 main commits (69,023 lines, a563f47 back 100 first-parent commits) and on 60,000 seeded lines per rule (test/review-risk-redos.test.ts), and no commit changes depth, class or risk class, so every diff dispatches main's reviewers unchanged; reviewer text is unchanged", @@ -82,7 +82,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: 'b16f28d6eecc162928d58fcfc6209f703b9692bfe5e901d381972939d2e16f7e', replay: 'GST-911-redos' }, + 'lib/review-depth.ts': { sha: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' }, 'lib/review-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-911' }, }; diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index 80873fe3e..0c34036a0 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -123,7 +123,7 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se }, 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, + // fails_when=an external image or stylesheet, an entity declaration, an external DTD, 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), or any CSS beyond declarations and url(#id) (Step 11 finding: image-set()); // seam=collectCommit + depthOfCommitInput @@ -134,6 +134,8 @@ 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', ' ]>'], + ['an external DTD', ''], + ['an external DTD behind the SVG public identifier', ''], ['a CSS-escaped import', ' '], ['a CSS-escaped url()', ' '], ['a zero-padded character reference import', ' '], From 170b32a88da0a31741df13185c98df7e7291b0e0 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:14:42 +0000 Subject: [PATCH 32/35] fix: freeze the exported SVG_REFUSED list (wave review-2, Step 11) Exported only so test/review-risk-redos.test.ts can reach the url() entry; a frozen readonly copy keeps any importer from changing verdicts while DEPTH_RULES still reports the original rules. Repin lib/review-depth.ts under no-loss replay GST-911-redos (no pattern changed). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 2 +- test/review-packets-removed.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 59340ac8e..d531f1e4c 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -85,7 +85,7 @@ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg(?:\s+PUBLIC\s+([" * 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. */ -export const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\((?!\s*#|['"]\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; +export const SVG_REFUSED: readonly RegExp[] = Object.freeze([/&(?!(?: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_OPEN = /<(?:[A-Za-z_][\w.-]*:)?style\b/gi; const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style\s*>/i; diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 1ae42964e..3ce595af9 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -82,7 +82,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: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' }, + 'lib/review-depth.ts': { sha: 'e6c9f53a95b72f77d82541f84df692bee2a19d73d3e4c7c696a1e3f8ce47dc7e', replay: 'GST-911-redos' }, 'lib/review-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-911' }, }; From 0d5f01be74653867a54d981063b954628f5b190f Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:17:41 +0000 Subject: [PATCH 33/35] Revert "fix: freeze the exported SVG_REFUSED list (wave review-2, Step 11)" The fix-check (wave review-2) showed Object.freeze is shallow: each RegExp can still be changed in place, so the freeze did not close adversarial-native#1, and it moved the tree past the one the Step 11 voices reviewed. The finding ships as an open note (its reviewer rated it Low, not blocking; no production caller mutates the list). Restores the lib/review-depth.ts pin under no-loss replay GST-911-redos. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 2 +- test/review-packets-removed.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index d531f1e4c..59340ac8e 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -85,7 +85,7 @@ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg(?:\s+PUBLIC\s+([" * 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. */ -export const SVG_REFUSED: readonly RegExp[] = Object.freeze([/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\((?!\s*#|['"]\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]); +export 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_OPEN = /<(?:[A-Za-z_][\w.-]*:)?style\b/gi; const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style\s*>/i; diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 3ce595af9..1ae42964e 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -82,7 +82,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: 'e6c9f53a95b72f77d82541f84df692bee2a19d73d3e4c7c696a1e3f8ce47dc7e', replay: 'GST-911-redos' }, + 'lib/review-depth.ts': { sha: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' }, 'lib/review-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-911' }, }; From 5acdd24471d68d584f5cb394f4c96d3dc51382fc Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:21:43 +0000 Subject: [PATCH 34/35] docs(changelog): wave review-2 entry for the integrator fixes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/wave-review-e7b551ebd865.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog.d/wave-review-e7b551ebd865.md diff --git a/changelog.d/wave-review-e7b551ebd865.md b/changelog.d/wave-review-e7b551ebd865.md new file mode 100644 index 000000000..0e8e47c70 --- /dev/null +++ b/changelog.d/wave-review-e7b551ebd865.md @@ -0,0 +1,5 @@ +### Fixed + +- **The SVG drawing check runs in linear time.** One changed SVG with a long run of spaces inside `url(` could stall `gstack-review-plan` and the queue's depth check for hours (80,000 spaces took 0.8 s, a 1 MB run over 230 s). The check now reads it in about a millisecond and refuses exactly what it refused before. +- **An SVG whose DOCTYPE points at an outside document stays code.** Only a bare `` or W3C's own SVG DTD still counts as a drawing, so an SVG with a `SYSTEM` DTD keeps its full review. +- **`gstack-diff-scope` picks the same base as `gstack-gate-diff`.** When `origin/` is missing or unrelated, a related local `` now comes before another remote's branch in both tools. From b306a49bc507911d216fa5c37e0f4b1e0ddcd104 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:23:42 +0000 Subject: [PATCH 35/35] docs(review-policy): a drawing's DOCTYPE names no outside document (wave review-2 docs audit) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index 390ab75e6..62a9cbf09 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -187,7 +187,7 @@ 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 +entity declaration, and no DOCTYPE but a bare `` or W3C's own SVG DTD, 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`