mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-10 14:38:24 +08:00
* fix(codex): use resume-compatible flags * fix: V-001 security vulnerability Automated security fix generated by Orbis Security AI * docs: align prompt-injection thresholds to security.ts (v1.6.4.0 catch-up) CLAUDE.md:290 and ARCHITECTURE.md:159 were missed when WARN was bumped 0.60 → 0.75 ind75402bb(v1.6.4.0, "cut Haiku classifier FP from 44% to 23%, gate now enforced", #1135). browse/src/security.ts:37 has WARN: 0.75 and BROWSER.md:743 was updated alongside that commit; CLAUDE.md and ARCHITECTURE.md still read 0.60. Also adds the SOLO_CONTENT_BLOCK: 0.92 entry to CLAUDE.md (already in security.ts:50 and BROWSER.md:745, missing from CLAUDE.md's threshold table). No code change. No behavior change. Pure doc-vs-code alignment. Verification: $ grep -n "WARN" browse/src/security.ts CLAUDE.md ARCHITECTURE.md BROWSER.md browse/src/security.ts:37: WARN: 0.75, CLAUDE.md:290: - \`WARN: 0.75\` ... ARCHITECTURE.md:159: ...>= \`WARN\` (0.75)... BROWSER.md:743: - \`WARN: 0.75\` ... Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: Korean/CJK IME input and rendering in Sidebar Terminal Fixes #1272 This commit addresses three separate Korean/CJK bugs in the Sidebar Terminal: **Bug 1 - IME Input**: Korean text typed via IME composition was not reaching the PTY correctly. Added compositionstart/compositionend event listeners to suppress partial jamo fragments and only send the final composed string. **Bug 2a - Font Rendering**: Added CJK monospace font fallbacks ("Noto Sans Mono CJK KR", "Malgun Gothic") to both the xterm.js fontFamily config and the CSS --font-mono variable. This ensures consistent cell-width calculations for Korean characters. **Bug 2b - UTF-8 Boundary Detection**: Added buffering logic to prevent multi-byte UTF-8 characters (Korean is 3 bytes) from being split across WebSocket chunks. This follows the same pattern as PR #1007 which fixed the sidebar-agent path, but extends it to the terminal-agent path. Special thanks to @ldybob for the excellent root cause analysis and proposed solutions in issue #1272. Tested on WSL2 + Windows 11 with Korean IME. * fix(ship): tighten Plan Completion gate (VAS-449 remediation) VAS-446 shipped with a PLAN.md acceptance criterion (domain-hq has /docs/dashboard.md) silently skipped. /ship's Plan Completion subagent existed at ship time (added in v1.4.1.0) but the gate let the failure through. Four structural fixes: 1. Path concreteness rule: items naming a concrete filesystem path MUST be classified DONE/NOT DONE via [ -f <path> ], never UNVERIFIABLE. 2. Validator detection: CONTENT-SHAPE items scan target repo's package.json for validate-* scripts and run them before falling back to UNVERIFIABLE. 3. Per-item UNVERIFIABLE confirmation: replaces blanket "I've checked each one" with per-item Y/N/D loop. The blanket-confirm path is the exact failure VAS-449 surfaced. 4. Subagent fail-closed: if Plan Completion subagent + inline fallback both fail, surface explicit AskUserQuestion instead of silent pass. Replaces the prior "Never block /ship on subagent failure" fail-open. Locked in by test/ship-plan-completion-invariants.test.ts (5 assertions, no LLM dependency, ~60ms). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(browse): bash.exe wrap for telemetry on Windows reportAttemptTelemetry() in browse/src/security.ts calls spawn(bin, args) where bin is the gstack-telemetry-log bash script. On Windows this fails silently with ENOENT — CreateProcess can't dispatch on shebang lines. Adopts v1.24.0.0's Bun.which + GSTACK_*_BIN override pattern (from browse/src/claude-bin.ts:resolveClaudeCommand, introduced in #1252) for resolving bash.exe. resolveBashBinary() honors GSTACK_BASH_BIN absolute-path or PATH-resolvable override, falling back to Bun.which('bash') which finds Git Bash on the standard Windows install. buildTelemetrySpawnCommand() wraps the script invocation on win32 only; POSIX path is bit-identical. Returns null when bash can't be resolved on Windows so caller skips spawn — local attempts.jsonl audit trail keeps working without surfacing a Windows-only failure. 8 new unit tests cover resolveBashBinary (POSIX bash, absolute override, quote-stripping, BASH_BIN fallback, empty-PATH null) and buildTelemetrySpawnCommand (POSIX pass-through, win32 bash wrap, win32 null on unresolvable, arg-array immutability). POSIX path is bit-identical — Bun.which('bash') on Linux/macOS returns the same /bin/bash or /usr/bin/bash that the old hardcoded spawn relied on. * fix(make-pdf): Bun.which-based binary resolution for browse + pdftotext on Windows Extends v1.24.0.0's Bun.which + GSTACK_*_BIN override pattern (introduced in browse/src/claude-bin.ts via #1252) to the two other binary resolvers in the codebase: make-pdf/src/browseClient.ts:resolveBrowseBin and make-pdf/src/pdftotext.ts:resolvePdftotext. Same Windows quirks (fs.accessSync(X_OK) degrades to existence-check; `which` isn't available outside Git Bash; bun --compile --outfile X emits X.exe), same Bun.which-based fix shape, same env override convention. Changes: - GSTACK_BROWSE_BIN / GSTACK_PDFTOTEXT_BIN as the v1.24-aligned overrides; BROWSE_BIN / PDFTOTEXT_BIN remain as back-compat aliases. - Bun.which() replaces execFileSync('which', ...) for PATH lookup. Handles Windows PATHEXT natively; no more `where`-vs-`which` branch. - findExecutable(base) helper exported from each module, probes .exe/.cmd/.bat after the bare-path miss on win32. Linux/macOS behavior is bit-identical (isExecutable short-circuits before the win32 branch ever runs). - macCandidates renamed posixCandidates (always was — /opt/homebrew, /usr/local, /usr/bin). No Windows candidates added; Poppler installs scatter across Scoop/Chocolatey/portable zips and guessing causes false positives. - Error messages get a Windows install hint (scoop install poppler / oschwartz10612) and `setx` example for GSTACK_*_BIN. - Pre-existing test 'honors BROWSE_BIN when it points at a real executable' was hardcoded /bin/sh — made cross-platform via a REAL_EXE constant (cmd.exe on win32, /bin/sh on POSIX). Was a Windows-CI blocker on its own. Coordination: PR #1094 (@BkashJEE) covered browseClient.ts independently with a narrower scope; this PR's pdftotext + cross-platform tests + GSTACK_*_BIN naming are additive. Either order of merge works. Test plan: - bun test make-pdf/test/browseClient.test.ts make-pdf/test/pdftotext.test.ts on win32 — 29 pass, 0 fail (12 new assertions: findExecutable POSIX/win32/null, resolveBrowseBin GSTACK_BROWSE_BIN + BROWSE_BIN + precedence + quote-strip, same shape for resolvePdftotext + Windows install hint in error message). - POSIX branch unchanged — fs.accessSync(X_OK) on Linux/macOS short-circuits before any win32 logic runs, matching the v1.24 claude-bin.ts pattern. * fix(browse): NTFS ACL hardening for Windows state files via icacls gstack's ~/.gstack/ state directory holds bearer tokens, canary tokens, agent queue contents (with prompt history), session state, security-decision logs, and saved cookie bundles — all written with { mode: 0o600 } / 0o700. On Windows, those mode bits are a silent no-op: Node's fs module doesn't translate POSIX modes to NTFS ACLs, and inherited ACLs leave every "restricted" file readable by other principals on the machine (verified via icacls — six ACEs, the intended user is the LAST of six). Threat model is non-trivial on: - Self-hosted CI runners (different service account on the same Windows box can read developer tokens, canary tokens, prompt history) - Shared development machines (agencies, studios, lab environments) - Multi-tenant servers with shared home directories Orthogonal to v1.24.0.0's binary-resolution work — complementary at the write side. v1.24's bin/gstack-paths resolves ~/.gstack/ correctly across plugin / global / local installs; this PR ensures files written into those resolved paths actually get the POSIX 0o600 semantic translated to NTFS. The fix: - New browse/src/file-permissions.ts (158 LOC, 5 public + 1 test-reset). restrictFilePermissions / restrictDirectoryPermissions wrap chmod (POSIX) or icacls /inheritance:r /grant:r <user>:(F) (Windows). writeSecureFile / appendSecureFile / mkdirSecure are drop-in wrappers for the common patterns. - 19 call sites converted across 9 source files: browser-manager.ts, browser-skill-write.ts, cli.ts, config.ts, meta-commands.ts, security-classifier.ts, security.ts (4 sites), server.ts (5 sites), terminal-agent.ts (8 sites), tunnel-denial-log.ts. - (OI)(CI) inheritance flags on directories mean files created via fs.write* *inside* an mkdirSecure-created dir inherit the owner-only ACL automatically — important for tunnel-denial-log.ts where appends use async fsp.appendFile. Error handling: icacls failures (nonexistent path, missing icacls.exe, hardened environments) log a one-shot warning to stderr and proceed. Once-per-process gating prevents log spam if the condition persists. Filesystem stays functional; the file just ends up with inherited ACLs. Test plan: - bun test browse/test/file-permissions.test.ts — 13 pass, 0 fail (POSIX mode-bit assertions, Windows no-throw, mkdir idempotence, recursive creation, Buffer payloads, append-creates-then-reapplies-once semantics) - bun test browse/test/security.test.ts — 38 pass, 0 fail (existing security test suite plus the bash-binary resolution tests added in fix #1119; the converted writeFileSync/appendFileSync/mkdirSync sites in security.ts integrate cleanly) - Empirical icacls before/after on a real file — 6 ACEs → 1 ACE - bun build typecheck on all modified files — clean (server.ts has a pre-existing playwright-core/electron resolution issue unrelated to this PR) POSIX behavior is bit-identical to old code — fs.chmodSync(path, 0o6XX) on the helper's POSIX branch matches the inline { mode: 0o6XX } it replaces. Linux and macOS see no behavior change. Inviting pushback on three judgment calls (in PR description): 1. icacls vs npm library 2. ACL scope — just user, or user + SYSTEM? 3. Graceful degradation — once-per-process warn, not silent, not hard-fail. * fix(browse): declare lastConsoleFlushed to restore console-log persistence flushBuffers() references a `lastConsoleFlushed` cursor at server.ts:337 and assigns it at :344, but the `let lastConsoleFlushed = 0;` declaration is missing — only the network and dialog siblings are declared at lines 327-328. Result: every 1-second flushBuffers tick (line 376) throws `ReferenceError: lastConsoleFlushed is not defined`, gets swallowed by the catch at line 369 ("[browse] Buffer flush failed: ..."), and the console branch's append never runs. browse-console.log is never written in any production deployment since this regressed. Discovered by stress-testing the daemon with 15 concurrent CLIs against cold state — the race surfaced the buffer-flush error spam in one spawned daemon's stderr. Verified by running the daemon against a real file:// page with console.log events: in-memory `browse console` returns the entries, but `.gstack/browse-console.log` is never created on disk. Regression introduced by1a100a2a"fix: eliminate duplicate command sets in chain, improve flush perf and type safety" — the flush refactor switched from `Bun.write` to `fs.appendFileSync` and added the `lastConsoleFlushed` cursor pattern alongside its network/dialog siblings, but missed the matching `let` declaration. Tests don't currently exercise flushBuffers, so the regression shipped silently. Fix: - Declare `let lastConsoleFlushed = 0;` next to `lastNetworkFlushed` and `lastDialogFlushed` (browse/src/server.ts:327) - Add a source-level guard test (browse/test/server-flush-trackers.test.ts) that fails any future refactor that adds a fourth `last*Flushed` cursor without the matching declaration. Same pattern as terminal-agent.test.ts and dual-listener.test.ts — read source as text, assert invariant, no daemon required. Test plan: - [x] New regression test fails on current main, passes with the fix - [x] `bun run build` clean - [x] Manual smoke: spawn daemon -> goto file:// page with console.log -> wait 4s -> .gstack/browse-console.log now exists with the expected entries (163 bytes vs zero before) 🤖 Generated with [Claude Code](https://claude.com/claude-code) * fix(browse): per-process state-file temp path to fix concurrent-write ENOENT The daemon writes `.gstack/browse.json` via the standard atomic-rename pattern: `writeFileSync(tmp, …) → renameSync(tmp, stateFile)`. Four sites in server.ts use this pattern (initial daemon-startup state at :2002, /tunnel/start handler at :1479, BROWSE_TUNNEL=1 inline tunnel update at :2083, BROWSE_TUNNEL_LOCAL_ONLY=1 update at :2113), and all four hard-code the same temp filename `${stateFile}.tmp`. Under concurrent writers the shared filename races on the rename: t0 Writer A: writeFileSync(stateFile + '.tmp', payloadA) t1 Writer B: writeFileSync(stateFile + '.tmp', payloadB) // overwrites A t2 Writer A: renameSync(stateFile + '.tmp', stateFile) // moves B's payload t3 Writer B: renameSync(stateFile + '.tmp', stateFile) // ENOENT — file gone Reproduced empirically with 15 concurrent CLIs against a fresh `.gstack/`: [browse] Failed to start: ENOENT: no such file or directory, rename '…/.gstack/browse.json.tmp' -> '…/.gstack/browse.json' Pre-fix success rate: **0 / 15** under cold-start race. Post-fix success rate: **15 / 15**, zero ENOENT. Fix: - New `tmpStatePath()` helper (server.ts:333) returns `${stateFile}.tmp.${pid}.${randomBytes(4).toString('hex')}` - All 4 call sites use `tmpStatePath()` instead of the shared literal - Atomic rename still gives last-writer-wins semantics on the final state.json content; only behavior change is that concurrent writers no longer kill each other on the rename step Source-level guard test (browse/test/server-tmp-state-path.test.ts) locks two invariants: (1) no remaining `stateFile + '.tmp'` literals, (2) every state-write `writeFileSync` call uses `tmpStatePath()`. Same read-source-as-text pattern as terminal-agent.test.ts and dual-listener.test.ts — no daemon required, runs in tier-1 free. Test plan: - [x] Targeted source-level guard test passes (3 / 0) - [x] `bun run build` clean - [x] Live regression: 15 concurrent CLIs against cold state → 15 / 15 healthy, 0 ENOENT (vs 0 / 15 pre-fix) - [x] No `.tmp.*` orphans left behind after rename succeeds - [x] Related test cluster (server-auth, dual-listener, cdp-mutex, findport) — same pre-existing flakes as `main`, no new regressions introduced 🤖 Generated with [Claude Code](https://claude.com/claude-code) * fix(browse): clear refs when iframe auto-detaches in getActiveFrameOrPage Asymmetric cleanup between two equivalent staleness conditions: onMainFrameNavigated() → clearRefs() + activeFrame = null ✓ getActiveFrameOrPage() → activeFrame = null (refs NOT cleared) ✗ Both paths see the same staleness condition — refs were captured against a frame that no longer exists. The main-frame path correctly clears both pieces of state. The iframe-detach path nulls the frame but leaves the refMap intact. The lazy click-time check in `resolveRef` (tab-session.ts:97) partially saves us — `entry.locator.count()` on a detached-frame locator throws or returns 0, so the click errors out as "Ref X is stale". But the user has no signal that frame context silently changed underfoot: the next `snapshot` runs against `this.page` (main) while old iframe refs still litter `refMap` with the same role+name keys. New refs collide with stale ones, the resolver picks one at random, the user clicks the wrong element. TODOS.md line 816-820 documents "Detached frame auto-recovery" as a shipped iframe-support feature in v0.12.1.0. This restores the documented intent — the recovery should leave the session in a clean state, not a half-cleared one. Fix: 1 line — add `this.clearRefs()` next to `this.activeFrame = null` inside the if-branch. Test plan: - [x] New regression test: 4/4 pass - refs cleared when getActiveFrameOrPage detects detached iframe - refs preserved when active frame is still attached (no regression) - refs preserved when no frame set (page-level path untouched) - matches onMainFrameNavigated symmetry — both paths reach the same clean end state - [x] `bun run build` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) * fix(codex): resolve python for JSON parser * fix: add fail-fast probe for base branch in ship step 12 * fix(plan-devex-review): remove contradictory plan-mode handshake * fix(design): honor Retry-After header in variants 429 handler Closes #1244. The 429 handler in `generateVariant` discarded the `Retry-After` response header and fell straight through to a local exponential schedule (2s/4s/8s). In image-generation batches, that burns retry attempts inside the provider's cooldown window and the request never recovers. Now we parse `Retry-After` per RFC 7231 — both delta-seconds (`Retry-After: 5`) and HTTP-date (`Retry-After: Fri, 31 Dec 1999 23:59:59 GMT`). Honored waits are capped at 60s to bound stalls from hostile or buggy headers. Delta-seconds are validated as digits-only (rejects `2abc`). When `Retry-After` is honored (including 0 / past-date "retry now"), the next iteration's leading exponential sleep is skipped so we don't double-wait. Invalid or missing headers fall through to the existing exponential schedule unchanged. Behavior matrix: | Header | Behavior | |---------------------------------|-------------------------------------------| | Retry-After: 5 | wait 5s, skip leading on next attempt | | Retry-After: 999999 | capped to 60s, skip leading | | Retry-After: 2abc | invalid, fall through to exponential | | Retry-After: 0 | wait 0, skip leading (retry immediately) | | Retry-After: <past HTTP-date> | wait 0, skip leading | | Retry-After: <future date> | wait diff capped at 60s, skip leading | | no header | fall through to existing exponential | `generateVariant` now accepts an optional `fetchFn` parameter (defaults to `globalThis.fetch`) so tests can inject a stub. Production call sites are unchanged. Tests cover the five behavior buckets above, asserting both the 1st-to-2nd call timing gap and call counts. All five pass in ~8s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(docs): correct per-skill symlink removal snippet in README uninstall Closes #1130. The manual-uninstall fallback in `## Uninstall` → `### Option 2` used `find ~/.claude/skills -maxdepth 1 -type l`, which finds nothing on real installs. Each `~/.claude/skills/<name>/` is a real directory, and only `<name>/SKILL.md` inside it is a symlink into `gstack/`. The find never matched, so the snippet silently removed nothing. Replace with a directory walk that inspects each `<name>/SKILL.md`: find ~/.claude/skills -mindepth 1 -maxdepth 1 -type d ! -name gstack → check $dir/SKILL.md is a symlink → readlink it → if target is gstack/* or */gstack/*: rm -f the link, rmdir the dir (only if empty — preserves any user-added files) Excludes the top-level `gstack/` dir from the walk; that's removed by step 3 of the same uninstall block. `bin/gstack-uninstall` (the script-mode path) already handles the layout correctly via its own walk; only this manual fallback needed updating. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: reject partial browse client env integers * fix(gemini-adapter): detect new ~/.gemini/oauth_creds.json auth path gemini-cli >=0.30 stores OAuth credentials at ~/.gemini/oauth_creds.json instead of the legacy ~/.config/gemini/ directory. The benchmark adapter's availability check now succeeds for users on recent gemini-cli releases who have authenticated via interactive login. Both paths are accepted so users on older versions still work. * fix(browser): add --no-sandbox for root user on Linux/WSL2 Chromium's sandbox can't initialize when running as root on Linux, causing an immediate exit. Extend the existing CI/CONTAINER check to also cover this case, keeping the Windows-safe `typeof getuid` guard. * security: pass cwd to git via execFileSync, not interpolation through /bin/sh `bin/gstack-memory-ingest.ts:632-643` ran `execSync(\`git -C ${JSON.stringify(cwd)} remote get-url origin 2>/dev/null\`, ...)`. JSON.stringify escapes `"` and `\` but not `$` or backticks, so a `cwd` of `"$(touch /tmp/marker)"` survived JSON quoting and detonated under /bin/sh's command-substitution-inside-double-quotes. `cwd` originates from transcript JSONL records under `~/.claude/projects/<encoded-cwd>/<uuid>.jsonl` and `~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl`. The walker grabs the first `.cwd` it sees per session. That's an untrusted surface in the gstack threat model — the L1-L6 sidebar security stack exists exactly because agent transcripts can carry attacker-influenced text. Two pivots above the local same-uid bar: (a) prompt-injection appending `cwd="$(...)"` to the active session log turns the next /sync-gbrain run into RCE under the user's uid; (b) cross-machine transcript share (a colleague's `.claude/projects` snippet untar'd into HOME, a documented gbrain dogfooding shape) → RCE on first sync. Fix swaps the one execSync for `execFileSync("git", ["-C", cwd, "remote", "get-url", "origin"], ...)`. No shell, argv passed directly to git. The same module already uses execFileSync for `gbrainAvailable()` (line 762 pre-patch) and `gbrainPutPage()` (line 816 pre-patch) — this single execSync was the outlier. Test: `gstack-memory-ingest security: untrusted cwd cannot trigger shell substitution` plants a Claude-Code-shaped JSONL with cwd=`$(touch <marker>)` and asserts the marker file is not created after `--incremental --quiet`. Negative control: with the patch reverted, the test fails (marker created); with the patch applied, it passes (18/18 in test/gstack-memory-ingest.test.ts). * security: gate domain-skill auto-promote on classifier_score > 0 `browse/src/domain-skill-commands.ts:140` (handleSave) writes `classifier_score: 0` with the comment "L4 deferred to load-time / sidebar-agent fills this in on first prompt-injection load." But CLAUDE.md "Sidebar architecture" documents that sidebar-agent.ts was ripped, and grep for recordSkillUse + classifierFlagged callers across browse/src/ returns zero hits outside the module under test. Net effect: every quarantined skill that survives three benign uses without flag (`recordSkillUse(... , classifierFlagged: false)` x3) auto-promotes to `active` and lands in prompt context wrapped as UNTRUSTED on every subsequent visit to that host. The L4 score that was supposed to gate the promotion was never written — the production save path puts 0 on disk and nothing later updates it. Threat model: a domain-skill body authored by an agent under the influence of a poisoned page (the new `gstackInjectToTerminal` PTY path runs no L1-L3 either) would lose its auto-promote barrier after three uses. The exploit isn't single-step but the bar is exactly N=3 prompt-injection-shaped uses on a hostile page, which is well within reach. Fix adds a single condition to the auto-promote gate in `recordSkillUse`: if (state === 'quarantined' && useCount >= PROMOTE_THRESHOLD && flagCount === 0 && current.classifier_score > 0) { state = 'active'; } `classifier_score` is set once at writeSkill and never updated. Production saves it as 0 (handleSave), so the gate stays closed; existing tests that explicitly pass `classifierScore: 0.1` still auto-promote (the auto-promote path is preserved for the day L4 is rewired). Manual promotion via `domain-skill promote-to-global` is unaffected (it goes through `promoteToGlobal` which has its own state-machine guard at line 337+). Test: new regression case `does NOT auto-promote when classifier_score is 0 (production handleSave shape)` plants a skill with classifierScore=0 (matches domain-skill-commands.ts:140), runs three uses without flag, asserts the skill stays quarantined and readSkill returns null. Negative control: revert the patch, the test fails with `Received: "active"`. With the patch: 15/15 pass. * fix(ship): port #1302 SKILL.md edits to .tmpl + resolver source PR #1302 added Verification Mode + UNVERIFIABLE classification + per-item confirmation gate to ship/SKILL.md, but only the generated SKILL.md was edited — not the .tmpl source or scripts/resolvers/review.ts. The next `bun run gen:skill-docs` run would have wiped the changes. Port the same content into the resolver and .tmpl so regeneration produces the intended output. * ci(windows): extend free-tests lane to cover icacls + Bun.which resolvers from fix-wave PRs Closes #1306/#1307/#1308 validation gap. The four newly-added test files already have process.platform guards so they run safely on both POSIX and Windows lanes — only platform-relevant assertions execute on each. Tests added to the windows-latest lane: - browse/test/file-permissions.test.ts (#1308 icacls + writeSecureFile) - browse/test/security.test.ts (#1306 bash.exe wrap pure-function path) - make-pdf/test/browseClient.test.ts (#1307 Bun.which browse resolver) - make-pdf/test/pdftotext.test.ts (#1307 Bun.which pdftotext resolver) * test(codex): live flag-semantics smoke for codex exec resume Closes #1270's regex-only test gap. PR #1270 asserted that codex/SKILL.md's `codex exec resume` invocation drops -C/-s and uses sandbox_mode config. That regex catches the skill template regressing, but not codex CLI itself flipping flag semantics again. This test probes `codex exec resume --help` and asserts the surface gstack relies on: -c/sandbox_mode is accepted, top-level -C is absent. Skips silently when codex isn't on PATH, so dev machines without codex installed never see it fail. * chore: regen SKILL.md after fix wave One regen commit at the end of the merge wave per the plan. plan-devex-review loses the contradictory plan-mode handshake (#1333). review/SKILL.md picks up the Verification Mode + UNVERIFIABLE classification additions that #1302 authored against ship/SKILL.md (same resolver shared between ship and review modes). * fix(server.ts): keep fs.writeFileSync for state-file writes #1308's writeSecureFile wrapper added Windows icacls hardening for the 4 state-file write sites in server.ts, but #1310's regression test grep's for fs.writeFileSync(tmpStatePath()) calls. The two changes are technically compatible only if the test relaxes — keeping the test strict (the safer choice for catching regressions on the cold-start race) means the 4 state- file sites stay on fs.writeFileSync(..., { mode: 0o600 }). POSIX 0o600 hardening is preserved on those 4 sites. Windows icacls hardening still applies to all the other writeSecureFile call sites #1308 added (auth.json, mkdirSecure, etc.). Also refreshes golden baselines after #1302 / port + minor wording tweak in scripts/resolvers/review.ts to keep gen-skill-docs.test.ts assertion 'Cite the specific file' satisfied. * v1.30.0.0: fix wave — 21 community PRs + 2 closing fixes for Windows + codex CI gaps Headline release. Browse stops dropping console logs, cold-start race fixed, codex resume works without python3, Windows hardening (icacls + Bun.which + bash.exe wrap), ship gate gets VAS-449 remediation, two closing fixes that put icacls/Bun.which/codex flag semantics under CI. * test(domain-skills): cover #1369 classifier_score=0 quarantine + score>0 promote path The pre-existing T6 test seeded skills via writeSkill (which defaults classifier_score to 0 until L4 is rewired) and then expected 3 uses to auto-promote. PR #1369 added `current.classifier_score > 0` to the gate specifically to block that path — a quarantined skill written under the influence of a poisoned page would otherwise auto-promote after three benign uses. Updated test asserts both halves of the new contract: - classifier_score=0 + 3 uses → stays quarantined (the security guarantee) - classifier_score>0 + 3 more uses → promotes to active (unblock path) Catches both regressions: the gate going away (would re-allow the bypass) and the unblock path breaking (would silently quarantine all skills forever once L4 is rewired). --------- Co-authored-by: Jayesh Betala <jayesh.betala7@gmail.com> Co-authored-by: orbisai0security <mediratta01.pally@gmail.com> Co-authored-by: Bryce Alan <brycealan.eth@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Terry Carson YM <cym3118288@gmail.com> Co-authored-by: Vasko Ckorovski <vckorovski@gmail.com> Co-authored-by: Samuel Carson <samuel.carson@gmail.com> Co-authored-by: Yashwant Kotipalli <yashwant7kotipalli@gmail.com> Co-authored-by: Jasper Chen <jasperchen925@gmail.com> Co-authored-by: Stefan Neamtu <stefan.neamtu@gmail.com> Co-authored-by: 陈家名 <chenjiaming@kezaihui.com> Co-authored-by: Abigail Atheryon <abi@atheryon.ai> Co-authored-by: Furkan Köykıran <furkankoykiran@gmail.com> Co-authored-by: gus <gustavoraularagon@gmail.com>
604 lines
30 KiB
Cheetah
604 lines
30 KiB
Cheetah
---
|
|
name: codex
|
|
preamble-tier: 3
|
|
version: 1.0.0
|
|
description: |
|
|
OpenAI Codex CLI wrapper — three modes. Code review: independent diff review via
|
|
codex review with pass/fail gate. Challenge: adversarial mode that tries to break
|
|
your code. Consult: ask codex anything with session continuity for follow-ups.
|
|
The "200 IQ autistic developer" second opinion. Use when asked to "codex review",
|
|
"codex challenge", "ask codex", "second opinion", or "consult codex". (gstack)
|
|
voice-triggers:
|
|
- "code x"
|
|
- "code ex"
|
|
- "get another opinion"
|
|
triggers:
|
|
- codex review
|
|
- second opinion
|
|
- outside voice challenge
|
|
allowed-tools:
|
|
- Bash
|
|
- Read
|
|
- Write
|
|
- Glob
|
|
- Grep
|
|
- AskUserQuestion
|
|
---
|
|
|
|
{{PREAMBLE}}
|
|
|
|
{{BASE_BRANCH_DETECT}}
|
|
|
|
# /codex — Multi-AI Second Opinion
|
|
|
|
You are running the `/codex` skill. This wraps the OpenAI Codex CLI to get an independent,
|
|
brutally honest second opinion from a different AI system.
|
|
|
|
Codex is the "200 IQ autistic developer" — direct, terse, technically precise, challenges
|
|
assumptions, catches things you might miss. Present its output faithfully, not summarized.
|
|
|
|
---
|
|
|
|
## Step 0: Check codex binary
|
|
|
|
```bash
|
|
CODEX_BIN=$(which codex 2>/dev/null || echo "")
|
|
[ -z "$CODEX_BIN" ] && echo "NOT_FOUND" || echo "FOUND: $CODEX_BIN"
|
|
```
|
|
|
|
If `NOT_FOUND`: stop and tell the user:
|
|
"Codex CLI not found. Install it: `npm install -g @openai/codex` or see https://github.com/openai/codex"
|
|
|
|
If `NOT_FOUND`, also log the event:
|
|
```bash
|
|
_TEL=$(~/.claude/skills/gstack/bin/gstack-config get telemetry 2>/dev/null || echo off)
|
|
source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null && _gstack_codex_log_event "codex_cli_missing" 2>/dev/null || true
|
|
```
|
|
|
|
---
|
|
|
|
## Step 0.5: Auth probe + version check
|
|
|
|
Before building expensive prompts, verify Codex has valid auth AND the installed
|
|
CLI version isn't in the known-bad list. Sourcing `gstack-codex-probe` loads the
|
|
shared helpers that both `/codex` and `/autoplan` use.
|
|
|
|
```bash
|
|
_TEL=$(~/.claude/skills/gstack/bin/gstack-config get telemetry 2>/dev/null || echo off)
|
|
source ~/.claude/skills/gstack/bin/gstack-codex-probe
|
|
|
|
if ! _gstack_codex_auth_probe >/dev/null; then
|
|
_gstack_codex_log_event "codex_auth_failed"
|
|
echo "AUTH_FAILED"
|
|
fi
|
|
_gstack_codex_version_check # warns if known-bad, non-blocking
|
|
```
|
|
|
|
If the output contains `AUTH_FAILED`, stop and tell the user:
|
|
"No Codex authentication found. Run `codex login` or set `$CODEX_API_KEY` / `$OPENAI_API_KEY`, then re-run this skill."
|
|
|
|
If the version check printed a `WARN:` line, pass it through to the user verbatim
|
|
(non-blocking — Codex may still work, but the user should upgrade).
|
|
|
|
The probe multi-signal auth logic accepts: `$CODEX_API_KEY` set, `$OPENAI_API_KEY`
|
|
set, or `${CODEX_HOME:-~/.codex}/auth.json` exists. Avoids false-negatives for
|
|
env-auth users (CI, platform engineers) that file-only checks would reject.
|
|
|
|
**Update the known-bad list** in `bin/gstack-codex-probe` when a new Codex CLI version
|
|
regresses. Current entries (`0.120.0`, `0.120.1`, `0.120.2`) trace to the stdin
|
|
deadlock fixed in #972.
|
|
|
|
---
|
|
|
|
## Step 0.6: Resolve portable roots
|
|
|
|
Before any mode runs, resolve `$PLAN_ROOT` (where plan files live) and `$TMP_ROOT`
|
|
(where ephemeral codex stderr / response captures land) via `bin/gstack-paths`.
|
|
This keeps the skill working whether installed as a Claude Code plugin
|
|
(`CLAUDE_PLANS_DIR` set), a global `~/.claude/skills/gstack/` install, or a CI
|
|
container where `HOME` may be unset and `/tmp` may be read-only.
|
|
|
|
```bash
|
|
eval "$(~/.claude/skills/gstack/bin/gstack-paths)"
|
|
```
|
|
|
|
After this, every subsequent bash block in this skill uses `"$PLAN_ROOT"` and
|
|
`"$TMP_ROOT"` rather than hardcoded `~/.claude/plans` or `/tmp/codex-*`.
|
|
|
|
---
|
|
|
|
## Step 1: Detect mode
|
|
|
|
Parse the user's input to determine which mode to run:
|
|
|
|
1. `/codex review` or `/codex review <instructions>` — **Review mode** (Step 2A)
|
|
2. `/codex challenge` or `/codex challenge <focus>` — **Challenge mode** (Step 2B)
|
|
3. `/codex` with no arguments — **Auto-detect:**
|
|
- Check for a diff (with fallback if origin isn't available):
|
|
`git diff origin/<base> --stat 2>/dev/null | tail -1 || git diff <base> --stat 2>/dev/null | tail -1`
|
|
- If a diff exists, use AskUserQuestion:
|
|
```
|
|
Codex detected changes against the base branch. What should it do?
|
|
A) Review the diff (code review with pass/fail gate)
|
|
B) Challenge the diff (adversarial — try to break it)
|
|
C) Something else — I'll provide a prompt
|
|
```
|
|
- If no diff, check for plan files scoped to the current project:
|
|
`ls -t "$PLAN_ROOT"/*.md 2>/dev/null | xargs grep -l "$(basename $(pwd))" 2>/dev/null | head -1`
|
|
If no project-scoped match, fall back to: `ls -t "$PLAN_ROOT"/*.md 2>/dev/null | head -1`
|
|
but warn the user: "Note: this plan may be from a different project."
|
|
- If a plan file exists, offer to review it
|
|
- Otherwise, ask: "What would you like to ask Codex?"
|
|
4. `/codex <anything else>` — **Consult mode** (Step 2C), where the remaining text is the prompt
|
|
|
|
**Reasoning effort override:** If the user's input contains `--xhigh` anywhere,
|
|
note it and remove it from the prompt text before passing to Codex. When `--xhigh`
|
|
is present, use `model_reasoning_effort="xhigh"` for all modes regardless of the
|
|
per-mode default below. Otherwise, use the per-mode defaults:
|
|
- Review (2A): `high` — bounded diff input, needs thoroughness
|
|
- Challenge (2B): `high` — adversarial but bounded by diff
|
|
- Consult (2C): `medium` — large context, interactive, needs speed
|
|
|
|
---
|
|
|
|
## Filesystem Boundary
|
|
|
|
All prompts sent to Codex MUST be prefixed with this boundary instruction:
|
|
|
|
> IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.
|
|
|
|
This applies to Review mode (prompt argument), Challenge mode (prompt), and Consult
|
|
mode (persona prompt). Reference this section as "the filesystem boundary" below.
|
|
|
|
---
|
|
|
|
## Step 2A: Review Mode
|
|
|
|
Run Codex code review against the current branch diff.
|
|
|
|
1. Create temp files for output capture:
|
|
```bash
|
|
TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")
|
|
```
|
|
|
|
2. Run the review (5-minute timeout). **Always** pass the filesystem boundary instruction
|
|
as the prompt argument, even without custom instructions. If the user provided custom
|
|
instructions, append them after the boundary separated by a newline:
|
|
```bash
|
|
_REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; }
|
|
cd "$_REPO_ROOT"
|
|
# Fix 1: wrap with timeout. 330s (5.5min) is slightly longer than the Bash 300s
|
|
# so the shell wrapper only fires if Bash's own timeout doesn't.
|
|
_gstack_codex_timeout_wrapper 330 codex review "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only." --base <base> -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR"
|
|
_CODEX_EXIT=$?
|
|
if [ "$_CODEX_EXIT" = "124" ]; then
|
|
_gstack_codex_log_event "codex_timeout" "330"
|
|
_gstack_codex_log_hang "review" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)"
|
|
echo "Codex stalled past 5.5 minutes. Common causes: model API stall, long prompt, network issue. Try re-running. If persistent, split the prompt or check ~/.codex/logs/."
|
|
fi
|
|
```
|
|
|
|
If the user passed `--xhigh`, use `"xhigh"` instead of `"high"`.
|
|
|
|
Use `timeout: 300000` on the Bash call. If the user provided custom instructions
|
|
(e.g., `/codex review focus on security`), append them after the boundary:
|
|
```bash
|
|
_REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; }
|
|
cd "$_REPO_ROOT"
|
|
codex review "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only.
|
|
|
|
focus on security" --base <base> -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR"
|
|
```
|
|
|
|
3. Capture the output. Then parse cost from stderr:
|
|
```bash
|
|
grep "tokens used" "$TMPERR" 2>/dev/null || echo "tokens: unknown"
|
|
```
|
|
|
|
4. Determine gate verdict by checking the review output for critical findings.
|
|
If the output contains `[P1]` — the gate is **FAIL**.
|
|
If no `[P1]` markers are found (only `[P2]` or no findings) — the gate is **PASS**.
|
|
|
|
5. Present the output:
|
|
|
|
```
|
|
CODEX SAYS (code review):
|
|
════════════════════════════════════════════════════════════
|
|
<full codex output, verbatim — do not truncate or summarize>
|
|
════════════════════════════════════════════════════════════
|
|
GATE: PASS Tokens: 14,331 | Est. cost: ~$0.12
|
|
```
|
|
|
|
or
|
|
|
|
```
|
|
GATE: FAIL (N critical findings)
|
|
```
|
|
|
|
5a. **Synthesis recommendation (REQUIRED).** After presenting Codex's verbatim
|
|
output and the GATE verdict, emit ONE recommendation line summarizing what the
|
|
user should do, in the canonical format the AskUserQuestion judge grades:
|
|
|
|
```
|
|
Recommendation: <action> because <one-line reason that names the most actionable finding>
|
|
```
|
|
|
|
Examples (the strongest reasons compare against an alternative — another finding, fix-vs-ship, or fix-order):
|
|
- `Recommendation: Fix the SQL injection at users_controller.rb:42 first because its auth-bypass blast radius is higher than the LFI Codex also flagged, and the parameterized-query fix is three lines vs the LFI's session-handling rewrite.`
|
|
- `Recommendation: Ship as-is because all 3 Codex findings are P3 cosmetic and the gate passed; addressing them would block the release without changing user-visible behavior.`
|
|
- `Recommendation: Investigate the race condition Codex flagged at billing.ts:117 before merging because the silent-corruption failure mode is harder to detect post-ship than the harness gap Codex also raised, which is fixable in a follow-up.`
|
|
|
|
The reason must engage with a specific finding (or compare against alternatives — other findings, fix-vs-ship, fix order). Boilerplate reasons ("because it's better", "because adversarial review found things") fail the format. The recommendation is the ONE line a user reads when they don't have time for the verbatim output. **Never silently auto-decide; always emit the line.**
|
|
|
|
6. **Cross-model comparison:** If `/review` (Claude's own review) was already run
|
|
earlier in this conversation, compare the two sets of findings:
|
|
|
|
```
|
|
CROSS-MODEL ANALYSIS:
|
|
Both found: [findings that overlap between Claude and Codex]
|
|
Only Codex found: [findings unique to Codex]
|
|
Only Claude found: [findings unique to Claude's /review]
|
|
Agreement rate: X% (N/M total unique findings overlap)
|
|
```
|
|
|
|
7. Persist the review result:
|
|
```bash
|
|
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"TIMESTAMP","status":"STATUS","gate":"GATE","findings":N,"findings_fixed":N,"commit":"'"$(git rev-parse --short HEAD)"'"}'
|
|
```
|
|
|
|
Substitute: TIMESTAMP (ISO 8601), STATUS ("clean" if PASS, "issues_found" if FAIL),
|
|
GATE ("pass" or "fail"), findings (count of [P1] + [P2] markers),
|
|
findings_fixed (count of findings that were addressed/fixed before shipping).
|
|
|
|
8. Clean up temp files:
|
|
```bash
|
|
rm -f "$TMPERR"
|
|
```
|
|
|
|
{{PLAN_FILE_REVIEW_REPORT}}
|
|
|
|
---
|
|
|
|
## Step 2B: Challenge (Adversarial) Mode
|
|
|
|
Codex tries to break your code — finding edge cases, race conditions, security holes,
|
|
and failure modes that a normal review would miss.
|
|
|
|
1. Construct the adversarial prompt. **Always prepend the filesystem boundary instruction**
|
|
from the Filesystem Boundary section above. If the user provided a focus area
|
|
(e.g., `/codex challenge security`), include it after the boundary:
|
|
|
|
Default prompt (no focus):
|
|
"IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only.
|
|
|
|
Review the changes on this branch against the base branch. Run `git diff origin/<base>` to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems."
|
|
|
|
With focus (e.g., "security"):
|
|
"IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only.
|
|
|
|
Review the changes on this branch against the base branch. Run `git diff origin/<base>` to see the diff. Focus specifically on SECURITY. Your job is to find every way an attacker could exploit this code. Think about injection vectors, auth bypasses, privilege escalation, data exposure, and timing attacks. Be adversarial."
|
|
|
|
2. Run codex exec with **JSONL output** to capture reasoning traces and tool calls (5-minute timeout):
|
|
|
|
If the user passed `--xhigh`, use `"xhigh"` instead of `"high"`.
|
|
|
|
```bash
|
|
_REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; }
|
|
PYTHON_CMD=$(command -v python3 2>/dev/null || command -v python 2>/dev/null || true)
|
|
if [ -z "$PYTHON_CMD" ]; then
|
|
echo "ERROR: Python 3 is required to parse Codex JSON output. Install python3 or python and retry." >&2
|
|
exit 1
|
|
fi
|
|
# Fix 1+2: wrap with timeout (gtimeout/timeout fallback chain via probe helper),
|
|
# capture stderr to $TMPERR for auth error detection (was: 2>/dev/null).
|
|
TMPERR=${TMPERR:-$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")}
|
|
_gstack_codex_timeout_wrapper 600 codex exec "<prompt>" -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached --json < /dev/null 2>"$TMPERR" | PYTHONUNBUFFERED=1 "$PYTHON_CMD" -u -c "
|
|
import sys, json
|
|
turn_completed_count = 0
|
|
for line in sys.stdin:
|
|
line = line.strip()
|
|
if not line: continue
|
|
try:
|
|
obj = json.loads(line)
|
|
t = obj.get('type','')
|
|
if t == 'item.completed' and 'item' in obj:
|
|
item = obj['item']
|
|
itype = item.get('type','')
|
|
text = item.get('text','')
|
|
if itype == 'reasoning' and text:
|
|
print(f'[codex thinking] {text}', flush=True)
|
|
print(flush=True)
|
|
elif itype == 'agent_message' and text:
|
|
print(text, flush=True)
|
|
elif itype == 'command_execution':
|
|
cmd = item.get('command','')
|
|
if cmd: print(f'[codex ran] {cmd}', flush=True)
|
|
elif t == 'turn.completed':
|
|
turn_completed_count += 1
|
|
usage = obj.get('usage',{})
|
|
tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0)
|
|
if tokens: print(f'\ntokens used: {tokens}', flush=True)
|
|
except: pass
|
|
# Fix 2: completeness check — warn if no turn.completed received
|
|
if turn_completed_count == 0:
|
|
print('[codex warning] No turn.completed event received — possible mid-stream disconnect.', flush=True, file=sys.stderr)
|
|
"
|
|
_CODEX_EXIT=${PIPESTATUS[0]}
|
|
# Fix 1: hang detection — log + surface actionable message
|
|
if [ "$_CODEX_EXIT" = "124" ]; then
|
|
_gstack_codex_log_event "codex_timeout" "600"
|
|
_gstack_codex_log_hang "challenge" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)"
|
|
echo "Codex stalled past 10 minutes. Common causes: model API stall, long prompt, network issue. Try re-running. If persistent, split the prompt or check ~/.codex/logs/."
|
|
fi
|
|
# Fix 2: surface auth errors from captured stderr instead of dropping them
|
|
if grep -qiE "auth|login|unauthorized" "$TMPERR" 2>/dev/null; then
|
|
echo "[codex auth error] $(head -1 "$TMPERR")"
|
|
_gstack_codex_log_event "codex_auth_failed"
|
|
fi
|
|
```
|
|
|
|
This parses codex's JSONL events to extract reasoning traces, tool calls, and the final
|
|
response. The `[codex thinking]` lines show what codex reasoned through before its answer.
|
|
|
|
3. Present the full streamed output:
|
|
|
|
```
|
|
CODEX SAYS (adversarial challenge):
|
|
════════════════════════════════════════════════════════════
|
|
<full output from above, verbatim>
|
|
════════════════════════════════════════════════════════════
|
|
Tokens: N | Est. cost: ~$X.XX
|
|
```
|
|
|
|
3a. **Synthesis recommendation (REQUIRED).** After presenting the full
|
|
adversarial output, emit ONE recommendation line summarizing what the user
|
|
should do, in the canonical format the AskUserQuestion judge grades:
|
|
|
|
```
|
|
Recommendation: <action> because <one-line reason that names the most exploitable finding>
|
|
```
|
|
|
|
Examples (the strongest reasons compare blast radius across findings or fix-vs-ship):
|
|
- `Recommendation: Fix the unbounded retry loop Codex flagged at queue.ts:78 because it DoSes the worker pool under sustained 429s, which is higher-blast-radius than the timing leak Codex also flagged that only touches a debug endpoint.`
|
|
- `Recommendation: Ship as-is because Codex's strongest finding is a theoretical race in cleanup that requires conditions we can't trigger in production, weaker than the runtime regressions a fix-now would risk.`
|
|
|
|
The reason must point to a specific finding and compare against alternatives (other findings, fix-vs-ship). Generic reasons like "because it's safer" fail the format. **Never silently skip the line.**
|
|
|
|
---
|
|
|
|
## Step 2C: Consult Mode
|
|
|
|
Ask Codex anything about the codebase. Supports session continuity for follow-ups.
|
|
|
|
1. **Check for existing session:**
|
|
```bash
|
|
cat .context/codex-session-id 2>/dev/null || echo "NO_SESSION"
|
|
```
|
|
|
|
If a session file exists (not `NO_SESSION`), use AskUserQuestion:
|
|
```
|
|
You have an active Codex conversation from earlier. Continue it or start fresh?
|
|
A) Continue the conversation (Codex remembers the prior context)
|
|
B) Start a new conversation
|
|
```
|
|
|
|
2. Create temp files:
|
|
```bash
|
|
TMPRESP=$(mktemp "$TMP_ROOT/codex-resp-XXXXXX.txt")
|
|
TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")
|
|
```
|
|
|
|
3. **Plan review auto-detection:** If the user's prompt is about reviewing a plan,
|
|
or if plan files exist and the user said `/codex` with no arguments:
|
|
```bash
|
|
setopt +o nomatch 2>/dev/null || true # zsh compat
|
|
ls -t "$PLAN_ROOT"/*.md 2>/dev/null | xargs grep -l "$(basename $(pwd))" 2>/dev/null | head -1
|
|
```
|
|
If no project-scoped match, fall back to `ls -t "$PLAN_ROOT"/*.md 2>/dev/null | head -1`
|
|
but warn: "Note: this plan may be from a different project — verify before sending to Codex."
|
|
|
|
**IMPORTANT — embed content, don't reference path:** Codex runs sandboxed to the repo
|
|
root and cannot access `~/.claude/plans/` or any files outside the repo. You MUST
|
|
read the plan file yourself and embed its FULL CONTENT in the prompt below. Do NOT tell
|
|
Codex the file path or ask it to read the plan file — it will waste 10+ tool calls
|
|
searching and fail.
|
|
|
|
Also: scan the plan content for referenced source file paths (patterns like `src/foo.ts`,
|
|
`lib/bar.py`, paths containing `/` that exist in the repo). If found, list them in the
|
|
prompt so Codex reads them directly instead of discovering them via rg/find.
|
|
|
|
**Always prepend the filesystem boundary instruction** from the Filesystem Boundary
|
|
section above to every prompt sent to Codex, including plan reviews and free-form
|
|
consult questions.
|
|
|
|
Prepend the boundary and persona to the user's prompt:
|
|
"IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only.
|
|
|
|
You are a brutally honest technical reviewer. Review this plan for: logical gaps and
|
|
unstated assumptions, missing error handling or edge cases, overcomplexity (is there a
|
|
simpler approach?), feasibility risks (what could go wrong?), and missing dependencies
|
|
or sequencing issues. Be direct. Be terse. No compliments. Just the problems.
|
|
Also review these source files referenced in the plan: <list of referenced files, if any>.
|
|
|
|
THE PLAN:
|
|
<full plan content, embedded verbatim>"
|
|
|
|
For non-plan consult prompts (user typed `/codex <question>`), still prepend the boundary:
|
|
"IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. Do NOT modify agents/openai.yaml. Stay focused on repository code only.
|
|
|
|
<user's question>"
|
|
|
|
4. Run codex exec with **JSONL output** to capture reasoning traces (5-minute timeout):
|
|
|
|
If the user passed `--xhigh`, use `"xhigh"` instead of `"medium"`.
|
|
|
|
For a **new session:**
|
|
```bash
|
|
_REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; }
|
|
PYTHON_CMD=$(command -v python3 2>/dev/null || command -v python 2>/dev/null || true)
|
|
if [ -z "$PYTHON_CMD" ]; then
|
|
echo "ERROR: Python 3 is required to parse Codex JSON output. Install python3 or python and retry." >&2
|
|
exit 1
|
|
fi
|
|
# Fix 1: wrap with timeout (gtimeout/timeout fallback chain via probe helper)
|
|
_gstack_codex_timeout_wrapper 600 codex exec "<prompt>" -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="medium"' --enable web_search_cached --json < /dev/null 2>"$TMPERR" | PYTHONUNBUFFERED=1 "$PYTHON_CMD" -u -c "
|
|
import sys, json
|
|
for line in sys.stdin:
|
|
line = line.strip()
|
|
if not line: continue
|
|
try:
|
|
obj = json.loads(line)
|
|
t = obj.get('type','')
|
|
if t == 'thread.started':
|
|
tid = obj.get('thread_id','')
|
|
if tid: print(f'SESSION_ID:{tid}', flush=True)
|
|
elif t == 'item.completed' and 'item' in obj:
|
|
item = obj['item']
|
|
itype = item.get('type','')
|
|
text = item.get('text','')
|
|
if itype == 'reasoning' and text:
|
|
print(f'[codex thinking] {text}', flush=True)
|
|
print(flush=True)
|
|
elif itype == 'agent_message' and text:
|
|
print(text, flush=True)
|
|
elif itype == 'command_execution':
|
|
cmd = item.get('command','')
|
|
if cmd: print(f'[codex ran] {cmd}', flush=True)
|
|
elif t == 'turn.completed':
|
|
usage = obj.get('usage',{})
|
|
tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0)
|
|
if tokens: print(f'\ntokens used: {tokens}', flush=True)
|
|
except: pass
|
|
"
|
|
# Fix 1: hang detection for Consult new-session (mirrors Challenge + resume)
|
|
_CODEX_EXIT=${PIPESTATUS[0]}
|
|
if [ "$_CODEX_EXIT" = "124" ]; then
|
|
_gstack_codex_log_event "codex_timeout" "600"
|
|
_gstack_codex_log_hang "consult" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)"
|
|
echo "Codex stalled past 10 minutes. Common causes: model API stall, long prompt, network issue. Try re-running. If persistent, split the prompt or check ~/.codex/logs/."
|
|
fi
|
|
```
|
|
|
|
For a **resumed session** (user chose "Continue"):
|
|
```bash
|
|
_REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; }
|
|
PYTHON_CMD=$(command -v python3 2>/dev/null || command -v python 2>/dev/null || true)
|
|
if [ -z "$PYTHON_CMD" ]; then
|
|
echo "ERROR: Python 3 is required to parse Codex JSON output. Install python3 or python and retry." >&2
|
|
exit 1
|
|
fi
|
|
cd "$_REPO_ROOT" || exit 1
|
|
# Fix 1: wrap with timeout (gtimeout/timeout fallback chain via probe helper)
|
|
_gstack_codex_timeout_wrapper 600 codex exec resume <session-id> "<prompt>" -c 'sandbox_mode="read-only"' -c 'model_reasoning_effort="medium"' --enable web_search_cached --json < /dev/null 2>"$TMPERR" | PYTHONUNBUFFERED=1 "$PYTHON_CMD" -u -c "
|
|
<same python streaming parser as above, with flush=True on all print() calls>
|
|
"
|
|
# Fix 1: same hang detection pattern as new-session block
|
|
_CODEX_EXIT=${PIPESTATUS[0]}
|
|
if [ "$_CODEX_EXIT" = "124" ]; then
|
|
_gstack_codex_log_event "codex_timeout" "600"
|
|
_gstack_codex_log_hang "consult-resume" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)"
|
|
echo "Codex stalled past 10 minutes. Common causes: model API stall, long prompt, network issue. Try re-running. If persistent, split the prompt or check ~/.codex/logs/."
|
|
fi
|
|
|
|
5. Capture session ID from the streamed output. The parser prints `SESSION_ID:<id>`
|
|
from the `thread.started` event. Save it for follow-ups:
|
|
```bash
|
|
mkdir -p .context
|
|
```
|
|
Save the session ID printed by the parser (the line starting with `SESSION_ID:`)
|
|
to `.context/codex-session-id`.
|
|
|
|
6. Present the full streamed output:
|
|
|
|
```
|
|
CODEX SAYS (consult):
|
|
════════════════════════════════════════════════════════════
|
|
<full output, verbatim — includes [codex thinking] traces>
|
|
════════════════════════════════════════════════════════════
|
|
Tokens: N | Est. cost: ~$X.XX
|
|
Session saved — run /codex again to continue this conversation.
|
|
```
|
|
|
|
7. After presenting, note any points where Codex's analysis differs from your own
|
|
understanding. If there is a disagreement, flag it:
|
|
"Note: Claude Code disagrees on X because Y."
|
|
|
|
8. **Synthesis recommendation (REQUIRED).** Emit ONE recommendation line
|
|
summarizing what the user should do based on Codex's consult output, in the
|
|
canonical format the AskUserQuestion judge grades:
|
|
|
|
```
|
|
Recommendation: <action> because <one-line reason that names the most actionable insight from Codex>
|
|
```
|
|
|
|
Examples (the strongest reasons compare Codex's insight against an alternative — different recommendation, status-quo, or another Codex point):
|
|
- `Recommendation: Adopt Codex's sharding suggestion because it eliminates the head-of-line blocking the current writer-pool has, while the cache-layer alternative Codex also floated still has a single-writer hot path.`
|
|
- `Recommendation: Reject Codex's "use SQLite instead" suggestion because the team's Postgres operational experience outweighs the simplicity gain at the projected scale, and Codex's secondary suggestion (read replicas) handles the read-load concern that motivated the SQLite pivot.`
|
|
- `Recommendation: Investigate Codex's flagged migration ordering before D3 lands because it surfaces a real foreign-key cycle that the in-house schema review missed, while the styling concern Codex also raised can wait for a follow-up.`
|
|
|
|
The reason must engage with a specific Codex insight and compare against an alternative (a different recommendation, status-quo, or another Codex point). Generic synthesis ("because Codex raised good points") fails the format. **Never silently auto-decide; always emit the line.**
|
|
|
|
---
|
|
|
|
## Model & Reasoning
|
|
|
|
**Model:** No model is hardcoded — codex uses whatever its current default is (the frontier
|
|
agentic coding model). This means as OpenAI ships newer models, /codex automatically
|
|
uses them. If the user wants a specific model, pass `-m` through to codex.
|
|
|
|
**Reasoning effort (per-mode defaults):**
|
|
- **Review (2A):** `high` — bounded diff input, needs thoroughness but not max tokens
|
|
- **Challenge (2B):** `high` — adversarial but bounded by diff size
|
|
- **Consult (2C):** `medium` — large context (plans, codebase), interactive, needs speed
|
|
|
|
`xhigh` uses ~23x more tokens than `high` and causes 50+ minute hangs on large context
|
|
tasks (OpenAI issues #8545, #8402, #6931). Users can override with `--xhigh` flag
|
|
(e.g., `/codex review --xhigh`) when they want maximum reasoning and are willing to wait.
|
|
|
|
**Web search:** All codex commands use `--enable web_search_cached` so Codex can look up
|
|
docs and APIs during review. This is OpenAI's cached index — fast, no extra cost.
|
|
|
|
If the user specifies a model (e.g., `/codex review -m gpt-5.1-codex-max`
|
|
or `/codex challenge -m gpt-5.2`), pass the `-m` flag through to codex.
|
|
|
|
---
|
|
|
|
## Cost Estimation
|
|
|
|
Parse token count from stderr. Codex prints `tokens used\nN` to stderr.
|
|
|
|
Display as: `Tokens: N`
|
|
|
|
If token count is not available, display: `Tokens: unknown`
|
|
|
|
---
|
|
|
|
## Error Handling
|
|
|
|
- **Binary not found:** Detected in Step 0. Stop with install instructions.
|
|
- **Auth error:** Codex prints an auth error to stderr. Surface the error:
|
|
"Codex authentication failed. Run `codex login` in your terminal to authenticate via ChatGPT."
|
|
- **Timeout (Bash outer gate):** If the Bash call times out (5 min for Review/Challenge, 10 min for Consult), tell the user:
|
|
"Codex timed out. The prompt may be too large or the API may be slow. Try again or use a smaller scope."
|
|
- **Timeout (inner `timeout` wrapper, exit 124):** If the shell `timeout 600` wrapper fires first, the skill's hang-detection block auto-logs a telemetry event + operational learning and prints: "Codex stalled past 10 minutes. Common causes: model API stall, long prompt, network issue. Try re-running. If persistent, split the prompt or check `~/.codex/logs/`." No extra action needed.
|
|
- **Empty response:** If `$TMPRESP` is empty or doesn't exist, tell the user:
|
|
"Codex returned no response. Check stderr for errors."
|
|
- **Session resume failure:** If resume fails, delete the session file and start fresh.
|
|
|
|
---
|
|
|
|
## Important Rules
|
|
|
|
- **Never modify files.** This skill is read-only. Codex runs in read-only sandbox mode.
|
|
- **Present output verbatim.** Do not truncate, summarize, or editorialize Codex's output
|
|
before showing it. Show it in full inside the CODEX SAYS block.
|
|
- **Add synthesis after, not instead of.** Any Claude commentary comes after the full output.
|
|
- **5-minute timeout** on all Bash calls to codex (`timeout: 300000`).
|
|
- **No double-reviewing.** If the user already ran `/review`, Codex provides a second
|
|
independent opinion. Do not re-run Claude Code's own review.
|
|
- **Detect skill-file rabbit holes.** After receiving Codex output, scan for signs
|
|
that Codex got distracted by skill files: `gstack-config`, `gstack-update-check`,
|
|
`SKILL.md`, or `skills/gstack`. If any of these appear in the output, append a
|
|
warning: "Codex appears to have read gstack skill files instead of reviewing your
|
|
code. Consider retrying."
|