From 0fd4df234b4edda93c87412cc373f178355433e0 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 01:18:59 +0000 Subject: [PATCH] fix(review): open every outside-voice brief with the REVIEWED_TREE step (GST-897) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 14 +++++---- lib/review-brief.ts | 21 +++++++++---- .../gpt-6.1-sol-gst850-p1.out | 11 +++++++ .../gpt-6.1-sol-gst850-p2.out | 7 +++++ test/review-dispatch.test.ts | 30 +++++++++++++++++-- test/review-pass.test.ts | 5 ++-- 6 files changed, 72 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p1.out create mode 100644 test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p2.out diff --git a/docs/review-policy.md b/docs/review-policy.md index 1d62b14de..c061c6b38 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -559,12 +559,14 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: fix-check plan renders its reader from `--fix-list`, `--hunks`, `--tests` and `--callers`, `--audit-only` runs just the `--extras` readers (Step 14.5's docs audit), and the fork point is found against `origin/` first, as preflight does. -7. Every brief, audit readers' included, ends with a workspace paragraph naming the - checkout the bins run in (GST-482): a reader on a shared machine starts in the - machine's working directory, so it is told to `cd` there on every command, to give - every file tool an absolute path under it, and to start its reply with - `REVIEWED_TREE: ` measured there. The paragraph follows the - section's render, which stays byte for byte. `collect` and `mark --state collected` +7. Every brief, audit readers' included, opens with a fingerprint step and ends with a + workspace paragraph naming the checkout the bins run in (GST-482): a reader on a shared + machine starts in the machine's working directory, so it is told to `cd` there on every + command, to give every file tool an absolute path under it, and to start its reply with + the `REVIEWED_TREE: ` line the step's command outputs there. The + step comes first because a reader following a findings-only format skipped the request + when it only closed the brief (GST-897). The section's render sits between the two, + byte for byte. `collect` and `mark --state collected` compare that line with the fingerprint recorded at render: a missing or foreign line is `COLLECT_WRONG_TREE`, a later commit in the same checkout `COLLECT_TREE_MOVED`; nothing is merged, the reader becomes `failed`, and `verify` reports `NOT_COLLECTED`. An diff --git a/lib/review-brief.ts b/lib/review-brief.ts index fe1a3a5e7..b233aa40b 100644 --- a/lib/review-brief.ts +++ b/lib/review-brief.ts @@ -144,16 +144,27 @@ function renderRole(role: BriefRole, slots: Slots, t: SectionTexts): string { } } +const quoted = (dir: string) => `'${dir.replace(/'/g, `'\\''`)}'`; +const treeCommand = (dir: string) => `cd ${quoted(dir)} && echo "REVIEWED_TREE: $(git rev-parse 'HEAD^{tree}') $(git rev-parse --show-toplevel)"`; + +/** + * The opening step every dispatched brief carries (GST-897): the fingerprint command and what to copy from it. A reader that + * follows a findings-only output format can skip a request that only closes a long brief (gpt-6.1-sol did on GST-850), and + * "the line prints" was read as the command itself, so the step comes first and names the output line. + */ +export function workspaceStep(dir: string): string { + return `First step, before anything else: run \`${treeCommand(dir)}\`. It outputs one line, \`REVIEWED_TREE: \`. Copy that output line, not the command, as the first line of your final reply, above everything else, even where this brief limits your output to findings or says how to end it; a reply without it, or from another checkout, does not count.`; +} + /** * The closing paragraph every dispatched brief carries (GST-482): a reader on a shared machine starts in that machine's working - * directory, and every command runs in a fresh shell, so the brief names the checkout under review and asks for a fingerprint - * measured there. It is appended after the section's render, so the brief before it stays byte for byte what renderBrief returns. + * directory, and every command runs in a fresh shell, so the brief names the checkout under review and asks again for the + * fingerprint workspaceStep opens with. The section's render sits between the two, byte for byte what renderBrief returns. */ export function workspaceParagraph(dir: string): string { - const q = `'${dir.replace(/'/g, `'\\''`)}'`; - return `Workspace: the checkout under review is ${dir}. Your shell and file tools may start in another checkout, and every command runs in a fresh shell: begin each command with \`cd ${q} && \`, and give every file read, search, edit or write an absolute path under ${dir}. Make the first line of your final reply the line \`cd ${q} && echo "REVIEWED_TREE: $(git rev-parse 'HEAD^{tree}') $(git rev-parse --show-toplevel)"\` prints, even where this brief limits your output to findings; a reply without it, or from another checkout, does not count.`; + return `Workspace: the checkout under review is ${dir}. Your shell and file tools may start in another checkout, and every command runs in a fresh shell: begin each command with \`cd ${quoted(dir)} && \`, and give every file read, search, edit or write an absolute path under ${dir}. The first line of your final reply is the \`REVIEWED_TREE: \` line that \`${treeCommand(dir)}\` outputs (the output, not the command), even where this brief limits your output to findings; a reply without it, or from another checkout, does not count.`; } -export const withWorkspace = (text: string, dir: string) => `${text}\n\n${workspaceParagraph(dir)}`; +export const withWorkspace = (text: string, dir: string) => `${workspaceStep(dir)}\n\n${text}\n\n${workspaceParagraph(dir)}`; /** The checkout a brief names: the toplevel of the directory the bins run in, where its diff and merge base come from. */ export function checkoutRoot(): string { const r = spawnSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8', timeout: 30_000 }); diff --git a/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p1.out b/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p1.out new file mode 100644 index 000000000..fc647463e --- /dev/null +++ b/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p1.out @@ -0,0 +1,11 @@ +1. **Escaped executable URLs bypass the locale checks** — `lib/review-allowlist.ts:143–145,188–192`. Changing a consumed locale JSON value from `"Continue"` to `"java\nscript:alert(1)"` returns `docs` with zero reviewers. Checks inspect the escaped source, but JSON decoding followed by URL parsing produces `javascript:`. A translated `href` can therefore gain script execution without security review. Validate decoded values, including URL normalization. Severity: High | Blocks: yes + +2. **SVG attribute interpolation is still executable template content** — `lib/review-allowlist.ts:35–44`. `Label` returns `docs` with zero reviewers. AngularJS `templateUrl`/`$compile` consumers evaluate that attribute. Numeric references also bypass the text check: `{{run()}}` is accepted. Reject interpolation in attributes and inspect decoded text, or reject numeric references. Severity: High | Blocks: yes + +3. **YAML type changes qualify as string-only edits** — `lib/review-allowlist.ts:163`. `label: Save` → `label: null` or `label: false` returns `docs` with zero reviewers. Native YAML parsing produces null/boolean, not strings; the null reproduction crashes a string consumer. Require string-valued scalars on both sides. Severity: Medium | Blocks: yes + +4. **Named gettext placeholders aren't preserved** — `lib/review-allowlist.ts:115,188–192`. `msgstr "Hello %(name)s"` → `msgstr "Hello %(password)s"` is accepted as docs because neither placeholder matches `PLACEHOLDER`. Formatting with the original arguments raises `KeyError('password')`, reproduced with Python. Recognize named percent placeholders or reject unfamiliar percent syntax. Severity: Medium | Blocks: yes + +5. **The new import scanner is quadratic** — `lib/review-risk.ts:72,107`. A valid comment containing repeated `import {` forces rescanning from every occurrence. At this HEAD, 128 KB took 1.04 seconds and 524 KB took 13.35 seconds for one `matchRisk` call; the planner calls it twice. Larger attacker-controlled comments can stall review and queue validation. Use a forward-only scanner. Severity: Medium | Blocks: yes + +Recommendation: request changes because escaped locale URLs can introduce executable javascript: links while scheduling zero reviewers. \ No newline at end of file diff --git a/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p2.out b/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p2.out new file mode 100644 index 000000000..a0f21e15d --- /dev/null +++ b/test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-p2.out @@ -0,0 +1,7 @@ +1. **Quadratic regex lets a diff stall review planning** — `lib/review-risk.ts:60`. The trailing `\s*,?\s*$` repeatedly repartitions whitespace when an `exec` binding ends with a comment. On HEAD `c1eea4c`, passing `" exec" + " ".repeat(200000) + "// comment"` to `matchRisk()` took **15.47 seconds**, versus **0.69 ms** on the supplied base. At 50,000 and 100,000 spaces, HEAD took 0.95 and 3.94 seconds, confirming quadratic growth. This is valid inside a multiline import; larger diffs can monopolize the synchronous review classifier. Remove the overlapping whitespace repetitions and add this adversarial regression. Severity: High. Blocks: yes + +2. **A literal first argument incorrectly exempts input-controlled destinations** — `lib/review-risk.ts:50–52`. `requests.request("GET", user_url)` and `httpx.request("POST", user_url)` pass a method first, but `NOT_LITERAL` treats it as a safe URL. Node’s `http.get("http://public.example/", { hostname: req.query.target }, cb)` also lets options override the literal destination. All three reproduce `risk=[]`, `depth=light`, omitting the separate Red Team and outside voice. A local Node server confirmed that the hostname override changes the actual destination. Handle destination arguments and overriding options according to each API’s signature. Severity: Medium. Blocks: yes + +3. **One type-only specifier hides executable child-process imports** — `lib/review-risk.ts:79`. A complete multiline import containing `type ExecOptions, exec as run,` is rejected wholesale by `!/^type\b/.test(names)`. With `run(cmd)` afterward, the classifier returns no security risk and light depth. Bun’s TypeScript transpiler confirms this retains the executable `exec as run` import. Check individual value specifiers instead of excluding the entire clause when its first specifier is type-only. Severity: Medium. Blocks: yes + +Recommendation: request changes because a whitespace-padded exec binding causes quadratic CPU consumption in the synchronous review classifier. \ No newline at end of file diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index e1aa85d8d..f6874e886 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -3,7 +3,7 @@ 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 { BriefError, checkReviewedTree, promptHash, renderBrief, sectionTexts, workspaceParagraph } from '../lib/review-brief'; +import { BriefError, checkReviewedTree, promptHash, renderBrief, sectionTexts, workspaceParagraph, workspaceStep } from '../lib/review-brief'; import { collect, judge, parseFindings, readJsonl, type MergedLine } from '../lib/review-ledger'; const ROOT = path.resolve(import.meta.dir, '..'); @@ -705,7 +705,7 @@ describe('the workspace paragraph (GST-482)', () => { const r = run(['render', '--role', role, '--out', out, ...(role === 'fix-check' ? [] : ['--diff-base', SHA]), ...args]); expect(r.status, role + ': ' + r.stderr).toBe(0); const main = renderBrief(role as Parameters[0], { diff_base: role === 'fix-check' ? undefined : SHA, ...slots }, t); - expect(fs.readFileSync(field(r.stdout, 'file'), 'utf8'), role).toBe(main + '\n\n' + workspaceParagraph(top)); + expect(fs.readFileSync(field(r.stdout, 'file'), 'utf8'), role).toBe(workspaceStep(top) + '\n\n' + main + '\n\n' + workspaceParagraph(top)); } expect(workspaceParagraph(top)).toContain(`begin each command with \`cd '${top}' && \``); expect(workspaceParagraph(top)).toContain(`absolute path under ${top}`); @@ -761,6 +761,32 @@ describe('reading the REVIEWED_TREE line (GST-482 review fixes)', () => { expect([u.status, u.stdout.startsWith('COLLECT_UNAVAILABLE: reader=outside-voice')]).toEqual([0, true]); }); + // Value: protects=the fingerprint request opens every brief and asks for the command's output line, so a reader that + // follows a findings-only format still binds its tree; the captured gpt-6.1-sol replies from GST-850, which never ran + // the command, stay refused; fails_when=the request only closes the brief (GST-850 p1/p2 COLLECT_WRONG_TREE) or reads + // as "copy the command", or collect relaxes for a reply with findings and no line; seam=lib/review-brief.ts + // withWorkspace, bin/gstack-review-dispatch collect; no_claim=a model's compliance (the GST-897 replay measures that) + test('the fingerprint step opens the brief and its command prints a line collect accepts; GST-850 gpt-6.1-sol replies stay refused (GST-897)', () => { + const out = tmp(); + const ledger = path.join(out, 'review-ledger.jsonl'); + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim(); + const file = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head]).stdout, 'file'); + const brief = fs.readFileSync(file, 'utf8'); + const first = brief.split('\n')[0]!; + expect(first).toStartWith(`First step, before anything else: run \`cd '${top}' && echo "REVIEWED_TREE: `); + expect(first).toContain('Copy that output line, not the command, as the first line of your final reply'); + expect(brief.slice(brief.lastIndexOf('\n\n') + 2)).toContain('(the output, not the command)'); + const command = /run `([^`]+)`/.exec(first)![1]!; + const printed = spawnSync('bash', ['-c', command], { cwd: os.tmpdir(), encoding: 'utf8', timeout: 30_000 }).stdout.trim(); + expect(checkReviewedTree(printed + '\nNO FINDINGS', expected).ok).toBe(true); + run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', file]); + for (const p of ['p1', 'p2']) { + const reply = path.join(ROOT, `test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-${p}.out`); + const refused = spawnSync(BIN, ['collect', '--ledger', ledger, '--role', 'outside-voice', '--output', reply, '--merged', path.join(out, `${p}.jsonl`)], { cwd: ROOT, encoding: 'utf8', timeout: 60_000 }); + expect([p, refused.status, refused.stdout.startsWith('COLLECT_WRONG_TREE: reader=outside-voice reported="none"')]).toEqual([p, 1, true]); + } + }); + test('render outside a readable checkout refuses instead of recording a fingerprint no reply can match', () => { const dir = tmp(); const r = run(['render', '--role', 'adversarial-native', '--out', path.join(dir, 'o'), '--diff-base', SHA], dir); diff --git a/test/review-pass.test.ts b/test/review-pass.test.ts index 46f3df0f7..7dd3b07dc 100644 --- a/test/review-pass.test.ts +++ b/test/review-pass.test.ts @@ -7,7 +7,7 @@ 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 { promptHash, sectionTexts, withWorkspace } from '../lib/review-brief'; +import { promptHash, sectionTexts, withWorkspace, workspaceParagraph, workspaceStep } from '../lib/review-brief'; import { CHUNK_CHARS, buildManifest, chunkCount, chunkOf, markReader, renderStage, specialistFindings, type Manifest } from '../lib/review-pass'; import type { Dispatch } from '../lib/review-depth'; @@ -798,7 +798,7 @@ describe('readers on a shared machine review the checkout the pass ships (GST-48 expect(res).not.toContain('STOP:'); expect(res).toContain('NEXT: judge (Phase 4)'); expect(h.creates.length).toBeGreaterThan(2); - for (const c of h.creates) expect(c.prompt.endsWith('\n\n' + withWorkspaceTail(wt)), c.title).toBe(true); + for (const c of h.creates) expect([c.prompt.startsWith(workspaceStep(wt) + '\n\n'), c.prompt.endsWith('\n\n' + workspaceParagraph(wt))], c.title).toEqual([true, true]); expect(bin(wt, env, ['verify', '--ledger', path.join(out, 'review-ledger.jsonl')]).stdout).not.toContain('wrong_tree'); }, 240_000); @@ -865,5 +865,4 @@ describe('readers on a shared machine review the checkout the pass ships (GST-48 expect(v).toMatch(/^BRIEF outside-voice UNAVAILABLE /m); }, 240_000); }); -const withWorkspaceTail = (dir: string) => withWorkspace('', dir).slice(2); const briefDirOf = (h: ReturnType, title: string) => briefDir(h.creates.find(c => c.title === title)?.prompt ?? '');