mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-16 09:12:13 +08:00
v1.34.2.0 fix wave: /codex review on CLI 0.130+, /investigate learnings, /sync-gbrain on Supabase (3 community-reported bugs) (#1478)
* fix(learnings): accept type:"investigation" in gstack-learnings-log The /investigate skill instructed agents to log learnings with type:"investigation", but bin/gstack-learnings-log:22 rejected anything not in [pattern, pitfall, preference, architecture, tool, operational]. Every investigation run exited 1 to stderr and the learning was dropped, silently to the user. Fix: add 'investigation' to ALLOWED_TYPES. Regression test: round-trips a learning with type:"investigation" and asserts exit 0 + file write; second test reads investigate/SKILL.md.tmpl and asserts it emits the literal type:"investigation" string, guarding the template/validator contract at both ends. Fixes #1423. Reported by diogolealassis. * fix(gbrain): engine detection survives gbrain ≥0.25 schema + non-zero doctor exit freshDetectEngineTier() in lib/gstack-memory-helpers.ts returned engine: "unknown" for every Supabase user on gbrain ≥0.25. Two stacking bugs: 1. execSync("gbrain doctor --json --fast 2>/dev/null") threw on non-zero exit. gbrain doctor exits 1 whenever health_score < 100, which is essentially every fresh install due to resolver_health warnings. The JSON output never reached the parser. 2. gbrain ≥0.25 shipped schema_version:2 doctor output that dropped the top-level 'engine' field entirely. Result: every /sync-gbrain on Supabase logged 'engine=unknown' and skipped all sync stages silently. Fix: - Replace execSync with execFileSync (no shell, no bash-specific 2>/dev/null redirect; portable to Windows). - Recover stdout from the thrown error object so non-zero exits still parse. - Fall back to reading gbrain's config.json (respecting GBRAIN_HOME env var, defaulting to ~/.gbrain/config.json) when doctor output doesn't surface an engine field. - Add logGbrainError() helper that appends one-line JSONL to ~/.gstack/.gbrain-errors.jsonl on parse failure, so future regressions leave a forensic trail. The "supabase" tier here means "remote postgres" in practice — gbrain config uses engine:"postgres" for both real Supabase and any other remote postgres (e.g. local-postgres-for-testing). Downstream sync code treats them identically, so the label compression is intentional and documented inline. Regression test: existing detectEngineTier suite now isolates HOME + GBRAIN_HOME + PATH to temp dirs (closes a flake source where the prior tests would read whatever was on the reviewer's machine). New test forces gbrain off PATH, writes a synthetic config.json with engine:"postgres", asserts detectEngineTier() returns engine:"supabase". Fixes #1415. Patch shape contributed by Shiv @shivasymbl (tested on gstack v1.31.0.0 + gbrain v0.31.3 + Supabase). * 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. * test: raise timeouts for slow integration tests Two test files were timing out at the default 5s on developer machines, both pre-existing on origin/main but unrelated to this branch's bug fixes: - test/gstack-artifacts-init.test.ts: 13 tests spawning real subprocesses via fake gh/glab/git shims in PATH. bun's fork+exec overhead pushed these past 5s consistently. Added a local test-wrapper that aliases test() with a 30s timeout (matches the brain-sync.test.ts pattern already in the repo). - test/gstack-next-version.test.ts: one integration smoke test that spawns 'bun run ./bin/gstack-next-version' and parses the resulting JSON. The subprocess does a 'gh pr list' against the live GitHub API to enumerate claimed version slots. Network latency makes 5s tight; raised this single test to 30s. No production code changed. The tests already passed deterministically once given enough wall-clock time. * chore: bump version and changelog (v1.34.2.0) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
@@ -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);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
@@ -11,12 +11,18 @@
|
||||
* auto-executes (no MCP probe). Per Finding #10: stored URL is HTTPS.
|
||||
*/
|
||||
|
||||
import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
|
||||
import { describe, test as _test, expect, beforeEach, afterEach } from 'bun:test';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { spawnSync } from 'child_process';
|
||||
|
||||
// Integration tests spawn real git/gh/glab subprocesses. The default 5s
|
||||
// per-test timeout is tight on developer machines; raise to 30s to match
|
||||
// the brain-sync.test.ts pattern. The tests stay deterministic (fake bins,
|
||||
// no network), but subprocess fork+exec under bun adds non-trivial overhead.
|
||||
const test = (name: string, fn: any) => _test(name, fn, 30000);
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..');
|
||||
const INIT_BIN = path.join(ROOT, 'bin', 'gstack-artifacts-init');
|
||||
|
||||
|
||||
@@ -272,17 +272,36 @@ describe("withErrorContext", () => {
|
||||
|
||||
describe("detectEngineTier", () => {
|
||||
let savedHome: string | undefined;
|
||||
let savedGbrainHome: string | undefined;
|
||||
let savedRealHome: string | undefined;
|
||||
let savedPath: string | undefined;
|
||||
let testHome: string;
|
||||
let testGbrainHome: string;
|
||||
|
||||
beforeEach(() => {
|
||||
savedHome = process.env.GSTACK_HOME;
|
||||
savedGbrainHome = process.env.GBRAIN_HOME;
|
||||
savedRealHome = process.env.HOME;
|
||||
savedPath = process.env.PATH;
|
||||
testHome = mkdtempSync(join(tmpdir(), "gstack-test-engine-"));
|
||||
testGbrainHome = mkdtempSync(join(tmpdir(), "gstack-test-gbrain-"));
|
||||
process.env.GSTACK_HOME = testHome;
|
||||
process.env.GBRAIN_HOME = testGbrainHome;
|
||||
// Isolate HOME too — even though gbrainConfigPath() prefers GBRAIN_HOME
|
||||
// when set, defense-in-depth against future code reading ~/.gbrain
|
||||
// directly. See #1415 codex review finding #6.
|
||||
process.env.HOME = testHome;
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
if (savedHome === undefined) delete process.env.GSTACK_HOME;
|
||||
else process.env.GSTACK_HOME = savedHome;
|
||||
if (savedGbrainHome === undefined) delete process.env.GBRAIN_HOME;
|
||||
else process.env.GBRAIN_HOME = savedGbrainHome;
|
||||
if (savedRealHome === undefined) delete process.env.HOME;
|
||||
else process.env.HOME = savedRealHome;
|
||||
if (savedPath === undefined) delete process.env.PATH;
|
||||
else process.env.PATH = savedPath;
|
||||
});
|
||||
|
||||
it("returns a valid EngineDetect shape (engine, detected_at, schema_version)", () => {
|
||||
@@ -307,4 +326,19 @@ describe("detectEngineTier", () => {
|
||||
const second = detectEngineTier();
|
||||
expect(second.detected_at).toBe(first.detected_at);
|
||||
});
|
||||
|
||||
it("falls back to GBRAIN_HOME/config.json when gbrain doctor omits engine (schema_version:2 case)", () => {
|
||||
// Regression test for #1415: gbrain >=0.25 doctor output dropped the
|
||||
// top-level `engine` field. The detect path must fall back to config.json.
|
||||
// We force the doctor call to fail (PATH stripped of gbrain) and write a
|
||||
// synthetic config to GBRAIN_HOME so the fallback path is deterministic.
|
||||
process.env.PATH = "/nonexistent-no-gbrain-here";
|
||||
writeFileSync(
|
||||
join(testGbrainHome, "config.json"),
|
||||
JSON.stringify({ engine: "postgres", database_url: "postgresql://test/example" }),
|
||||
"utf-8"
|
||||
);
|
||||
const result = detectEngineTier();
|
||||
expect(result.engine).toBe("supabase");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -153,6 +153,9 @@ describe("markActiveSiblings", () => {
|
||||
// Integration smoke — only runs if gh is available and authenticated. Confirms
|
||||
// the CLI executes end-to-end against real APIs without crashing.
|
||||
describe("integration (smoke)", () => {
|
||||
// Bumps timeout to 30s — the test spawns a real `bun run` subprocess that
|
||||
// does a `gh pr list` against the live GitHub API to inspect claimed slots.
|
||||
// Network latency makes 5s tight on developer machines.
|
||||
test("CLI runs against real repo and emits parseable JSON", async () => {
|
||||
const proc = Bun.spawnSync([
|
||||
"bun",
|
||||
@@ -178,5 +181,5 @@ describe("integration (smoke)", () => {
|
||||
expect(Array.isArray(parsed.claimed)).toBe(true);
|
||||
expect(parsed).toHaveProperty("siblings");
|
||||
expect(parsed.siblings).toEqual([]); // --workspace-root null disabled scanning
|
||||
});
|
||||
}, 30000);
|
||||
});
|
||||
|
||||
@@ -102,6 +102,27 @@ describe('gstack-learnings-log', () => {
|
||||
const lines = fs.readFileSync(f!, 'utf-8').trim().split('\n');
|
||||
expect(lines.length).toBe(2);
|
||||
});
|
||||
|
||||
// Regression test for #1423: investigate skill emits type:"investigation"
|
||||
// but ALLOWED_TYPES previously rejected it. Now accepted.
|
||||
test('accepts type:"investigation" (regression: #1423)', () => {
|
||||
const input = '{"skill":"investigate","type":"investigation","key":"root-cause","insight":"verified","confidence":9,"source":"observed"}';
|
||||
const result = runLog(input);
|
||||
expect(result.exitCode).toBe(0);
|
||||
const f = findLearningsFile();
|
||||
expect(f).not.toBeNull();
|
||||
const parsed = JSON.parse(fs.readFileSync(f!, 'utf-8').trim());
|
||||
expect(parsed.type).toBe('investigation');
|
||||
});
|
||||
|
||||
// Caller contract: investigate/SKILL.md.tmpl must emit type:"investigation"
|
||||
// verbatim. Guards against the template drifting to an invalid type and
|
||||
// silently breaking the log path. See codex review finding for #1423.
|
||||
test('investigate template emits type:"investigation" verbatim (caller contract)', () => {
|
||||
const tmpl = fs.readFileSync(path.join(ROOT, 'investigate/SKILL.md.tmpl'), 'utf-8');
|
||||
// The invocation line must include "type":"investigation" exactly.
|
||||
expect(tmpl).toContain('"type":"investigation"');
|
||||
});
|
||||
});
|
||||
|
||||
describe('gstack-learnings-search', () => {
|
||||
|
||||
Reference in New Issue
Block a user