Repository navigation
Conversation
Co-authored-by: Codex <[email protected]>
Added instructions for reloading plugins and setup command.
…#13) On Windows, Node.js `spawnSync` without `shell: true` uses `CreateProcess`, which only resolves `.exe` files. npm installs global tools (like `codex`) as `.cmd` shims, so `spawnSync("codex", ...)` returns ENOENT even when codex is correctly installed and on PATH. Adding `shell: process.platform === "win32"` routes through `cmd.exe` on Windows, which properly resolves `.cmd`, `.bat`, and PATHEXT entries. No behavior change on macOS/Linux since the condition evaluates to false.
* chore: bump plugin version to 1.0.1 * chore: bump plugin metadata version to 1.0.1
Co-authored-by: VOIDXAI <[email protected]>
* fix: add shell and windowsHide options for Windows spawn in app-server
On Windows, spawn("codex", ["app-server"]) fails with ENOENT because
Node.js cannot resolve .cmd shims without shell: true. This adds
platform-gated shell and windowsHide options to the app-server spawn
call, and uses terminateProcessTree for proper process tree cleanup
since shell: true wraps the child in cmd.exe.
Without terminateProcessTree, plain SIGTERM only kills cmd.exe and
leaves the actual codex node process orphaned — verified with 274+
zombie node.exe processes accumulating on Windows.
Fixes #32
Fixes #46
* fix: guard terminateProcessTree against PID reuse after process exit
ChildProcess.killed only reflects whether .kill() was called by this
process — it stays false when the child exits on its own. On Windows,
where PIDs are recycled quickly, the 50 ms timer could fire after
cmd.exe has exited and its PID has been reassigned, causing taskkill
to terminate an unrelated process.
Add an exitCode === null check so the tree-kill path is skipped once
the child has already exited.
---------
* fix: make test suite portable across platforms Replace hardcoded macOS path with fileURLToPath for cross-platform compatibility. Add .cmd wrapper creation and platform-aware PATH separator in test fixtures so the fake codex binary is discoverable on Windows. Add shell and windowsHide options to the test helper run() function to match production behavior. Test results on Windows improve from 12/64 pass to 59/64 pass. * fix: skip shell for absolute-path commands to avoid Windows space-in-path breakage When `process.execPath` resolves to a path with spaces (e.g., `C:\Program Files\nodejs\node.exe`), `shell: true` causes cmd.exe to split the path at the space. Guard with `path.isAbsolute()` so only bare command names (which need `.cmd` shim resolution) use the shell. --------- Co-authored-by: Dominik Kundel <[email protected]>
Co-authored-by: VOIDXAI <[email protected]>
* fix: skip untracked directories in review context * fix: skip broken untracked symlinks in reviews
* Use app-server auth status for Codex readiness * fix: reuse existing app server for auth checks
…provided (#159) `SpawnedCodexAppServerClient.initialize()` passes `this.options.env` to `spawn()`, but no caller ever sets `env` in options. In Node.js, passing `undefined` for `env` gives the child process **no** environment variables, breaking any model provider that relies on env vars (e.g. DATABRICKS_TOKEN). Fall back to `process.env` when `this.options.env` is not set, matching the existing pattern in `broker-lifecycle.mjs` and `codex-companion.mjs`. Co-authored-by: Isaac Co-authored-by: Bhuvanesh Sridharan <[email protected]>
…#126) * fix: gracefully handle unsupported thread/name/set on older Codex CLI Codex CLI v0.118.0 does not recognize the thread/name/set JSON-RPC method, causing startThread() to throw. Thread naming is cosmetic (for job log labels) and should not block thread creation. Wraps the call in try/catch so it fails silently on older CLI versions. Fixes #119 * refactor: only suppress unsupported-method errors for thread/name/set Address Codex review feedback: the bare catch swallowed all errors including auth, network, and server failures. Now only suppresses errors containing 'unknown variant' or 'unknown method' (the specific error older CLI versions return) and rethrows everything else.
…sion (#83) Co-authored-by: VOIDXAI <[email protected]>
) Co-authored-by: VOIDXAI <[email protected]>
* fix: avoid embedding large adversarial review diffs * fix: preserve untracked content in lightweight review * address comments * fix: handle ENOBUFS type check in git diff sizing
* codex: honor --cwd when reporting session runtime * codex: keep session runtime lookup scoped to callers --------- Co-authored-by: VOIDXAI <[email protected]>
* fix: declare model in codex-rescue agent frontmatter The codex-rescue agent had no model field, leaving Claude Code to assign whatever default it chooses. As a thin forwarding wrapper that issues a single Bash call, this agent is well-suited to the haiku tier; declaring it explicitly ensures a predictable cost profile and tier guarantee. * Update codex-rescue.md --------- Co-authored-by: Dominik Kundel <[email protected]>
Closes #77 Updated README to replace unsupported 'xhigh' value with 'high' to prevent configuration errors.
Unquoted \$ARGUMENTS in the ! shell commands allowed shell metacharacters in user-supplied job IDs to be expanded before Node received them (e.g., `task-123; malicious-cmd` would execute the trailing command). This is inconsistent with review.md and adversarial-review.md, which both wrap "$ARGUMENTS" in double quotes.
…on (#234) (#235) * fix: route /codex:rescue through the Agent tool to stop Skill recursion (#234) `/codex:rescue` previously combined two things that together caused a hang: - `context: fork` in the frontmatter, which spawns a `general-purpose` subagent for the command body. - Body prose "Route this request to the `codex:codex-rescue` subagent." without naming the transport. When the main agent called `Skill(codex:rescue)` programmatically, the fork resolved the ambiguous prose by trying `Skill(codex:codex-rescue)` (unknown skill) and then falling back to `Skill(codex:rescue)`, which re-entered this command and hung the session until the user cancelled. No Codex job was ever created. Naming the transport as `Agent(codex:codex-rescue)` alone is not enough: forked general-purpose subagents do not expose the `Agent` tool, so the forked runner cannot reach the subagent that way either. The minimal fix is therefore two coordinated changes: - Drop `context: fork` so the command body runs inline in the calling agent's context, where `Agent` is in scope. - Say explicitly "use the `Agent` tool with `subagent_type: "codex:codex-rescue"`", and call out that `Skill(codex:codex-rescue)` and `Skill(codex:rescue)` are not valid routing paths. Add `Agent` to `allowed-tools` so the call does not prompt for permission. Everything else in rescue.md (resume-candidate check, flag handling, background/foreground semantics, operating rules) is unchanged. The `codex:codex-rescue` subagent itself is unchanged. Tests pin the new allow-list, the explicit `subagent_type`, the ban on `Skill(codex:codex-rescue)`, and the absence of `context: fork`. The existing "run the `codex:codex-rescue` subagent in the background" assertion continues to hold since that sentence still reads correctly with the Agent-tool transport. Fixes #234 * test: match quoted result and cancel command arguments --------- Co-authored-by: Dominik Kundel <[email protected]>
* Remove shell expansion for git commands * Version bump
…d-only (port openai#508) (#33) * feat(sandbox): resolve task sandbox from config.toml + add task --read-only (port openai#508) Port of @cubicj's openai#508. The plugin hardcoded the sandbox ("read-only"/"workspace-write"), ignoring sandbox_mode in ~/.codex/config.toml. Now the task path sends sandbox: null when the caller does not pin one, letting the app-server resolve it from config — the same mechanism already used for model. Explicit --write still forces workspace-write; review paths keep their pinned read-only. - buildThreadParams/buildResumeParams: sandbox ?? null (config-driven default) - task --read-only flag: per-run read-only pin, mutually exclusive with --write - stop-review-gate hook's internal task passes --read-only explicitly so the gate stays read-only regardless of config - rescue command/agent/skill docs updated for --read-only Merged onto the fork (conflicts in codex.mjs, codex-companion.mjs, rescue.md, fake-codex-fixture, runtime.test): kept our reasoning-validation + turn-timeout-ms + model aliases + --cwd, layered cubicj's sandbox-null + --read-only on top. Our fake-codex fixture already recorded sandbox in lastThreadStart/lastThreadResume, so openai#508's assertions pass as-is. Verified: npm run build clean; npm test 174/171 pass / 3 pre-existing (was 4 — the background test that previously asserted a foreign model-luna value now passes). All 6 new sandbox tests green. Co-Authored-By: Claude <[email protected]> * fix(sandbox): fail closed when a read-only pin is not honored on resume (cycle-review critical) Codex adversarial review of #33 flagged — and a real-codex smoke confirmed — that Codex 0.144.6 keeps a loaded thread's sandbox and ignores thread/resume overrides: `task --read-only --resume-last` on a write-capable thread ran with workspace-write access, violating the --read-only contract. Verified empirically (codex-cli 0.144.6): write-thread -> resume with --read-only returned resolved.sandbox = workspaceWrite. The code never compared the resolved sandbox to the explicit pin. Fix: assertExplicitSandboxHonored() in runAppServerTurn compares an explicit read-only pin to the app-server's resolved sandbox and throws on mismatch — fail closed instead of running write-capable under a read-only flag. Only the read-only pin is enforced (a write request on a read-only resolved sandbox is a downgrade, not a safety leak). TDD: a resume-ignores-sandbox fake-codex behavior reproduces the leak (RED), the assertion turns it green. Also confirms the port's cubicj claim: a plain task with sandbox:null resolves to read-only from config (smoke: resolved readOnly), so stock behavior is unchanged. Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude <[email protected]>
|
@cubicj — just wanted to say thanks for this PR (and #506). I maintain a fork (axisrow/codex-plugin-cc) where the plugin has drifted enough from upstream that merging here has been slow, and your config-driven sandbox work is genuinely useful — I ported #508 into my fork this week. One thing came up while porting that's worth flagging back to you, since it's about the My fork's fix (in case it's useful): after the thread/start-or-resume response, compare the requested pin to the app-server's resolved sandbox and fail closed on mismatch — Fork PR (port + the guard), with attribution to this PR: axisrow#33. Pure sharing — take it, adapt it, or set it aside. Either way, thanks for the PRs; they're the kind of substantive contribution this repo needs more of. |
Ships accumulated fixes since fork.4: - #29 broker-busy hang (PR #30): bound broker handshake - #31 spawned-app-server handshake (PR #32): bound request() primitive - openai#508 config-driven sandbox + --read-only (PR #33) + resume-sandbox fail-closed - #34 model/effort in /codex:rescue launch line (PR #35) - #28 turn-timeout gaps (already in fork.4 base) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6207a5144
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@axisrow I've pushed a guard in the same fail-closed shape you described: after The silent ignore itself looks like an app-server issue — |
|
Thanks for reproducing it independently on 0.145.0-alpha.29 — that confirms the loaded-thread boundary and makes the guard feel a lot less like a one-off workaround. Appreciate you carrying it upstream. |
e5ce272 to
03e8e96
Compare
Summary
The plugin always overrides the user's configured
sandbox_modewith hardcoded sandbox values (#482).buildThreadParams/buildResumeParamsfall back to"read-only"whenever the caller passes nothing, and the task path hardcodes the--writeternary to"workspace-write"/"read-only", so asandbox_modein~/.codex/config.tomlnever reaches any job. Users who want config-driven sandboxes (e.g.danger-full-accessfor trusted delegation setups) end up patching the installed plugin by hand.Change
Task paths now send
sandbox: nullwhen the caller does not ask for a specific sandbox, letting the app-server resolve it fromconfig.toml— the same mechanism the plugin already relies on formodel(model: options.model ?? nullfalls back to the configured model today). An explicit--writestill forcesworkspace-write, and both review paths (native and adversarial) deliberately keep their pinnedread-onlysandbox.lib/codex.mjs:buildThreadParamsandbuildResumeParamschangeoptions.sandbox ?? "read-only"tooptions.sandbox ?? null.ThreadStartParams.sandboxis declaredSandboxMode | null, so this is type-clean.codex-companion.mjs: the task path changesrequest.write ? "workspace-write" : "read-only"torequest.write ? "workspace-write" : request.readOnly ? "read-only" : null.Because omitting
--writenow defers to config instead of guaranteeing read-only, the same change adds atask --read-onlyflag as the per-run way to pin the read-only sandbox: it is mutually exclusive with--write(a combined call fails with a clear error), survives the background request round trip into the detached worker, and applies to resumed threads. The rescue command/agent/skill texts are updated one line each so that read-only intent means passing--read-onlyrather than merely omitting--write. The stop-review-gate hook's internal task now passes--read-onlyexplicitly, so every review-shaped run — native review, adversarial review, and the stop-time gate task — keeps a pinned read-only sandbox regardless of config.Behavior only changes for users who set
sandbox_modeinconfig.toml: when it is absent, the app-server's own default for an unset sandbox remains read-only, so stock installations keep today's behavior. Inheriting a permissive mode from config is a deliberate user choice made in their own Codex configuration, and #506 (resolved model/effort/sandbox capture, from #481) makes the inherited value visible on every job record. An explicit per-run escalation flag (#145, PR #147) is complementary to this config-driven default and intentionally out of scope here.Verification
thread/startwithsandbox: nullandconfig.tomlsandbox_mode = "danger-full-access"returned the config-resolved{"type":"dangerFullAccess"}sandbox in the response echo; explicit values still override.npm test: 94/94 passing. The fake-codex fixture now recordsthread/start/thread/resumeparams, with assertions covering every path: a plain task sendssandbox: null,--writesendsworkspace-write,--read-onlypinsread-onlyon fresh, resumed, and background-worker threads,--write --read-onlyis rejected, and native review, adversarial review, and the stop-review-gate hook's task all still sendread-only.npm run build(typecheck viatsconfig.app-server.json): clean.git diff --checkagainst the base commit: clean.Related Issues
Fixes #482