mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-02 23:14:33 +01:00
fix(review): readers start their reply with the bare REVIEWED_TREE line; a printed: label is read (GST-776)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
f739756389
commit
726aaf138e
2 files changed
+41
-3
No files matched your search
+3
-3
@@ -142,7 +142,7 @@ function renderRole(role: BriefRole, slots: Slots, t: SectionTexts): string {
|
||||
*/
|
||||
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 ${q} && \`, and give every file read, search, edit or write an absolute path under ${dir}. Start your final reply with the bare line \`REVIEWED_TREE: <tree> <toplevel>\`, even where this brief limits your output to findings: nothing before it, not the command and not a label. Get the line by running \`cd ${q} && echo "REVIEWED_TREE: $(git rev-parse 'HEAD^{tree}') $(git rev-parse --show-toplevel)"\` and copy what it prints. A reply without that line, or from another checkout, does not count.`;
|
||||
}
|
||||
export const withWorkspace = (text: string, dir: string) => `${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. */
|
||||
@@ -152,11 +152,11 @@ export function checkoutRoot(): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* One REVIEWED_TREE line as "<tree> <toplevel>", or null. A list marker or heading mark may lead it, and a whole-line `**…**` or
|
||||
* One REVIEWED_TREE line as "<tree> <toplevel>", or null. A list marker, heading mark or `printed:` label (GST-776) may lead it, and a whole-line `**…**` or
|
||||
* backtick wrapper, a bold label and backticks around the sha or the whole path are removed; the path's own characters are kept.
|
||||
*/
|
||||
function parseTreeLine(line: string): string | null {
|
||||
let t = line.trim().replace(/^(?:[-*>]\s+|\d+[.)]\s+|#+\s+)/, '');
|
||||
let t = line.trim().replace(/^(?:[-*>]\s+|\d+[.)]\s+|#+\s+)/, '').replace(/^[Pp]rinted:\s+/, '');
|
||||
for (const w of ['**', '`']) if (t.length > 2 * w.length && t.startsWith(w) && t.endsWith(w)) t = t.slice(w.length, -w.length).trim();
|
||||
const m = /^(?:\*\*REVIEWED_TREE:\*\*|REVIEWED_TREE:)\s+`?([0-9a-f]{40,64})`?\s+(.+)$/.exec(t);
|
||||
if (!m) return null;
|
||||
|
||||
@@ -580,3 +580,41 @@ describe('reading the REVIEWED_TREE line (GST-482 review fixes)', () => {
|
||||
expect(r.stderr).toContain('BRIEF_RENDER_FAILED: cannot read HEAD^{tree} and the toplevel of');
|
||||
});
|
||||
});
|
||||
|
||||
describe('the REVIEWED_TREE line as GBRA-218 readers wrote it (GST-776)', () => {
|
||||
const top = fs.realpathSync(spawnSync('git', ['rev-parse', '--show-toplevel'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim());
|
||||
const tree = spawnSync('git', ['rev-parse', 'HEAD^{tree}'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim();
|
||||
const command = `cd '${top}' && echo "REVIEWED_TREE: $(git rev-parse 'HEAD^{tree}') $(git rev-parse --show-toplevel)"`;
|
||||
// Value: protects=a reader that reviewed the right checkout is not failed for how it wrote its fingerprint; fails_when=the brief asks for "the line <command> prints" (two Sonnet 5.5 low readers then wrote `printed: REVIEWED_TREE: …` and were failed as reported="none"), or a `printed:` prefix lets a wrong tree through;
|
||||
// why_new=GBRA-218 pass restarts on gstack 1.92.25.0; seam=lib/review-brief.ts workspaceParagraph and checkReviewedTree via gstack-review-dispatch collect; no_claim=that every model now writes the bare line
|
||||
test('the brief asks for the bare line first and gives the command only as the way to get it', () => {
|
||||
const p = workspaceParagraph(top);
|
||||
expect(p).not.toContain('the line `cd');
|
||||
const bare = p.indexOf('`REVIEWED_TREE: <tree> <toplevel>`');
|
||||
expect(bare).toBeGreaterThan(0);
|
||||
expect(p.indexOf(command)).toBeGreaterThan(bare);
|
||||
});
|
||||
|
||||
test('the echoed command then a printed: line is read; the same shape with a wrong tree or checkout is still refused', () => {
|
||||
const reply = (fingerprint: string) => `${command}\nprinted: REVIEWED_TREE: ${fingerprint}\nNO FINDINGS\n`;
|
||||
expect(checkReviewedTree(reply(`${tree} ${top}`), `${tree} ${top}`)).toMatchObject({ ok: true });
|
||||
expect(checkReviewedTree(reply(`${'b'.repeat(40)} ${top}`), `${tree} ${top}`)).toMatchObject({ ok: false, moved: true, reported: `${'b'.repeat(40)} ${top}` });
|
||||
expect(checkReviewedTree(reply(`${tree} /elsewhere`), `${tree} ${top}`)).toMatchObject({ ok: false, moved: false, reported: `${tree} /elsewhere` });
|
||||
expect(checkReviewedTree(`Note: REVIEWED_TREE: ${tree} ${top}\nNO FINDINGS`, `${tree} ${top}`).reported).toBe('none');
|
||||
|
||||
const out = tmp();
|
||||
const ledger = path.join(out, 'review-ledger.jsonl');
|
||||
const merged = path.join(out, 'merged.jsonl');
|
||||
const file = field(run(['render', '--role', 'specialist', '--name', 'testing', '--out', out, '--diff-base', SHA]).stdout, 'file');
|
||||
run(['sent', '--ledger', ledger, '--role', 'specialist', '--name', 'testing', '--sent', file]);
|
||||
const collectReply = (text: string) => {
|
||||
const f = path.join(out, 'reply.out');
|
||||
fs.writeFileSync(f, text);
|
||||
return spawnSync(BIN, ['collect', '--ledger', ledger, '--role', 'specialist', '--name', 'testing', '--output', f, '--merged', merged], { cwd: ROOT, encoding: 'utf8', timeout: 60_000 });
|
||||
};
|
||||
const wrong = collectReply(reply(`${'b'.repeat(40)} ${top}`));
|
||||
expect([wrong.status, wrong.stdout.startsWith('COLLECT_TREE_MOVED: reader=specialist/testing')]).toEqual([1, true]);
|
||||
const right = collectReply(reply(`${tree} ${top}`));
|
||||
expect(right.stdout).toStartWith('COLLECT: reader=specialist/testing findings=0 ');
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user