mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-17 15:20:11 +02:00
fix(codex): /codex review works on Codex CLI ≥0.130.0
Codex CLI 0.130.0 made [PROMPT] and --base <BRANCH> mutually exclusive at argv level. Step 2A of codex/SKILL.md.tmpl had always passed both (the filesystem boundary prefix as the prompt argument + the base branch), so every /codex review call died with: error: the argument '[PROMPT]' cannot be used with '--base <BRANCH>' Fix: split Step 2A into two paths. Default (no custom user instructions): bare 'codex review --base <base>'. Codex's review prompt is internally diff-scoped, so the model focuses on the changes against base. The filesystem boundary prefix is dropped here because Codex 0.130 has no documented system-prompt config key (probed -c 'system_prompt="..."' against 0.130 — the flag is silently accepted but the value isn't applied). Skill files under .claude/ and agents/ are public, so this is a token-efficiency concern, not a safety one. Custom instructions (/codex review <focus>): route through codex exec with the diff written to a tempfile, inlined into the prompt between explicit DIFF_START / DIFF_END markers. The boundary is preserved here because codex exec isn't auto-scoped to the diff. The DIFF_START/END delimiters tell the model where data ends and instructions resume, which materially reduces prompt-injection hijack rates when the diff contains adversarial content. Note on bash semantics: codex's earlier review flagged the exec route as "command injection via $_DIFF interpolation." That framing is wrong — bash parameter expansion does not re-evaluate $(...) or backticks inside the expanded value, so a diff containing $(rm -rf /) is plain string data to codex exec. The real risk is prompt injection (model-side, not shell-side), which the DIFF_START/END pattern mitigates. Regression tests in test/codex-hardening.test.ts assert across BOTH codex/SKILL.md.tmpl AND the generated codex/SKILL.md: 1. No 'codex review' invocation line combines a quoted-string OR variable positional argument with --base. 2. Step 2A still contains either bare 'codex review --base' OR 'codex exec' (guards against accidental deletion of both fix paths). Fixes #1428. Reported by Stashub.
This commit is contained in:
@@ -364,3 +364,66 @@ describe('gstack-codex-probe: telemetry event emission', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ── Step 2A argv guard ─────────────────────────────────────────────────────
|
||||
// Regression test for #1428: Codex CLI >=0.130.0 rejects passing a quoted
|
||||
// prompt argument together with `--base <branch>`. Step 2A must never combine
|
||||
// the two on the same line. Asserts across both the .tmpl source and the
|
||||
// generated SKILL.md so template drift can't silently re-introduce the bug.
|
||||
|
||||
describe('codex SKILL.md.tmpl Step 2A: PROMPT + --base mutual exclusion guard', () => {
|
||||
function extractStep2A(filePath: string): string {
|
||||
const content = fs.readFileSync(filePath, 'utf-8');
|
||||
const startIdx = content.indexOf('## Step 2A: Review Mode');
|
||||
expect(startIdx).toBeGreaterThan(-1);
|
||||
// End at next `## ` heading (skill section boundary).
|
||||
const tail = content.slice(startIdx);
|
||||
const nextHeading = tail.slice(2).search(/\n## /);
|
||||
return nextHeading === -1 ? tail : tail.slice(0, nextHeading + 2);
|
||||
}
|
||||
|
||||
for (const relPath of ['codex/SKILL.md.tmpl', 'codex/SKILL.md']) {
|
||||
test(`${relPath}: no \`codex review\` line combines a quoted prompt argument with --base`, () => {
|
||||
const section = extractStep2A(path.join(ROOT, relPath));
|
||||
// Find all lines invoking `codex review` (any prefix wrapper allowed).
|
||||
const lines = section.split('\n');
|
||||
const offendingLines: string[] = [];
|
||||
for (const line of lines) {
|
||||
// Skip prose lines that just discuss codex review. Only inspect lines
|
||||
// that look like an actual shell invocation (codex review followed by
|
||||
// a non-prose token).
|
||||
const match = line.match(/\bcodex\s+review\b(.*)$/);
|
||||
if (!match) continue;
|
||||
const rest = match[1];
|
||||
// Two regression patterns:
|
||||
// codex review "..." --base <foo>
|
||||
// codex review $VAR --base <foo>
|
||||
// codex review -- "..." --base <foo>
|
||||
// Acceptable: codex review --base <foo> (bare, no prompt arg)
|
||||
const hasBase = /--base\b/.test(rest);
|
||||
if (!hasBase) continue;
|
||||
// Strip --base <token> and any trailing -c/--enable flags so they
|
||||
// don't look like positional args. Anything that remains BEFORE
|
||||
// --base and looks like a positional is the regression.
|
||||
const beforeBase = rest.split(/--base\b/)[0].trim();
|
||||
// Empty (or just whitespace) before --base => bare review, safe.
|
||||
if (beforeBase === '') continue;
|
||||
// Allow `--` separator that introduces nothing else (rare). Anything
|
||||
// that looks like a quoted string OR variable expansion is the bug.
|
||||
if (/^["'$]|^--\s*["']/.test(beforeBase)) {
|
||||
offendingLines.push(line);
|
||||
}
|
||||
}
|
||||
expect(offendingLines).toEqual([]);
|
||||
});
|
||||
|
||||
test(`${relPath}: Step 2A still contains at least one fix-path invocation`, () => {
|
||||
const section = extractStep2A(path.join(ROOT, relPath));
|
||||
// At least one of: bare `codex review --base` OR `codex exec ...` must
|
||||
// remain. Guards against accidental deletion of both fix paths.
|
||||
const bareReview = /codex\s+review\s+--base\b/.test(section);
|
||||
const execRoute = /codex\s+exec\b/.test(section);
|
||||
expect(bareReview || execRoute).toBe(true);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user