CalvinBackup/gstack
1
0
Fork 0
mirror of https://github.com/garrytan/gstack.git synced 2026-09-13 08:29:04 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(codex,review,ship): scope codex review with an explicit --base flag, never prompt text

Browse Source 
`codex review` takes its scope ONLY from --base/--commit/--uncommitted. The
positional [PROMPT] is mutually exclusive with all three, and a prompt-only
`codex review "<text>"` silently falls back to the uncommitted working-tree
scope (verified on 0.144.1: it runs `git status --short; git diff` and
reviews that) — so the previous prompt-based scoping produced a
confidently-worded review of the WRONG changes and read "no changes" on a
clean tree. Every diff pass now invokes `codex review --base <base>` with no
prompt argument: /codex Step 2A default path, the /review structured pass,
and the /ship adversarial-section pass (all via scripts/resolvers/review.ts).

Custom review instructions keep their own `codex exec` path (the CLI rejects
prompt + scope flag together), with the filesystem boundary preserved there.
Two new Error Handling entries teach the failure shapes: the argv-parse
error, and the "review says no changes on a branch full of changes" symptom.

Tests updated to pin the new invariant instead of banning the fix: the old
assertions required the diff range in prompt text and banned the
`--base <base> -c '...'` substring, which the correct scoped form contains.
Also deletes test/fixtures/golden-ship-claude.md — a 2,565-line orphaned
fixture referenced by zero tests (the live goldens are in
test/fixtures/golden/, compared by test/host-config.test.ts); the factory
golden is refreshed from the regenerated output. Generated SKILL.md files
regenerated via gen:skill-docs in this commit.

Contributed by @fangearhq-boop (PR #2513).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-08-14 20:20:55 -07:00
co-authored by Claude Fable 5
parent 7a8e39d2cb
commit 8c5bb4545b
9 changed files with 211 additions and 2565 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/skill-validation.test.ts
+35 -5
View File
@@ -1500,11 +1500,37 @@ describe('Codex skill', () => {
});
test('codex review invocations avoid the prompt plus --base argument shape', () => {
// The real invariant is "never pass a positional [PROMPT] together with a
// scope flag" — the CLI rejects that combination at argv parse time
// (#1428, #1479). Two different shapes satisfy it, and these files have
// diverged on which one they use:
//
// scoped — `codex review --base <base>` with NO prompt argument. The
// scope comes from the CLI, which is the only thing that actually sets
// it. This is what all three files now use.
// broken — prompt-only `codex review "<text>"` describing the diff
// range in prose. This parses, but the CLI falls back to *uncommitted
// working-tree* scope, so the review silently covers the wrong changes.
//
// The old assertion banned the substring `--base <base> -c '...'`, which
// the correct scoped form also contains — it could not tell the two apart,
// so it effectively banned the fix.
for (const rel of ['codex/SKILL.md', 'review/SKILL.md', 'ship/SKILL.md']) {
// ship's codex command moved into sections/adversarial.md (T9 carve).
const content = rel === 'ship/SKILL.md' ? readShipUnion() : fs.readFileSync(path.join(ROOT, rel), 'utf-8');
expect(content).not.toContain('--base <base> -c \'model_reasoning_effort="high"\'');
expect(content).toContain('Run git diff origin/<base>...HEAD 2>/dev/null || git diff <base>...HEAD');
expect(content).toMatch(/codex\s+review\s+--base\b/);
const offending: string[] = [];
for (const line of content.split('\n')) {
if (line.includes('`codex review`')) continue;
const match = line.match(/(?:^|[;&|]\s*|\s)codex\s+review\b(.*)$/);
if (!match) continue;
const rest = match[1];
if (!/--base\b|--commit\b|--uncommitted\b/.test(rest)) continue;
const beforeFlag = rest.split(/--base\b|--commit\b|--uncommitted\b/)[0].trim();
// A quoted string or variable expansion before the scope flag is the bug.
if (/^["'$]|^--\s*["']/.test(beforeFlag)) offending.push(`${rel}: ${line.trim()}`);
}
expect(offending).toEqual([]);
}
});
@@ -1512,9 +1538,13 @@ describe('Codex skill', () => {
// Pre-#1209, the bare `codex review --base` path stripped the filesystem
// boundary instruction, letting Codex spend tokens reading skill files.
// #1209's prompt rewrite restored the boundary by routing every default
// call through a prompt. Pin both halves so a future refactor can't
// regress: (a) the boundary line must appear, (b) the call must be
// through `codex review "<prompt>"` not bare `codex review --base`.
// call through a prompt — but routing through a prompt is what breaks the
// diff scope, so codex/ no longer does that. What this test pins is the
// boundary TEXT, which must still be present for the paths that do take a
// prompt (`codex exec` for challenge, consult, and custom review focus).
// Do NOT "restore" the boundary by putting a prompt argument back on a
// scoped `codex review` call: that combination fails to parse, and
// dropping the scope flag to make it parse silently reviews the wrong diff.
const boundaryLine =
'Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/';
for (const rel of ['codex/SKILL.md', 'review/SKILL.md', 'ship/SKILL.md']) {