Skip to content

Runtime safety: mandatory policy and real cancellation - #181

Merged
oratis merged 2 commits into
mainfrom
codex/runtime-safety
Aug 2, 2026
Merged

Runtime safety: mandatory policy and real cancellation#181
oratis merged 2 commits into
mainfrom
codex/runtime-safety

Conversation

@oratis

@oratis oratis commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • make the central tool dispatcher impossible to omit and fail safe for legacy callers
  • give LSP, VS Code, and desktop explicit safe policies
  • propagate aborts through providers, pending approvals, Node process groups, and Tauri/Rust process groups
  • preserve worktree branches and reject removal when user changes are present

Validation

  • pnpm docs:check
  • pnpm lint (3 pre-existing warnings, 0 errors)
  • pnpm format:check
  • pnpm typecheck
  • pnpm test
  • pnpm build
  • cargo check --manifest-path apps/desktop/src-tauri/Cargo.toml
  • cargo test --manifest-path apps/desktop/src-tauri/Cargo.toml (29 passed)

Stack

Depends on #180. This is the safety/cancellation slice of PR 1 in docs/CODEX_ALIGNMENT_PLAN.md; RuntimeHost assembly and background ownership can follow without weakening these invariants.

@oratis
oratis deleted the branch main August 2, 2026 06:34
@oratis oratis closed this Aug 2, 2026
@oratis oratis reopened this Aug 2, 2026
@oratis
oratis changed the base branch from codex/codex-alignment-plan to main August 2, 2026 06:35
@oratis
oratis marked this pull request as ready for review August 2, 2026 06:37
@oratis

oratis commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Review: approved ✅ — this is the most important PR in the stack

The bug being fixed is real and was severe. On main, agent.ts gated the entire dispatcher behind if (opts.mode). Since mode was optional, any caller that omitted it skipped mode checks, permission rules, and the PreToolUse hook chain entirely — every tool call was allowed. Both VS Code entry points and the LSP handler were exactly such callers. Making mode required and routing through resolveRuntimePolicy() closes that.

Verified in the diff

  • runtime/policy.ts: the fallback is genuinely fail-safe — untyped/legacy JS callers land on mode: 'default' + a read-only allow-list rather than the previous "no gate at all".
  • LSP: activeTurns Set<string>Map<string, AbortController>, and handleAbort now actually calls .abort(). Previously abort deleted a bookkeeping entry and returned true while the turn kept running — a false success.
  • bash.ts: killProcessTree() sends SIGKILL to -pid (the group) with a direct-child fallback, and terminate() is idempotent via the terminationReason guard. The settled flag on finish() correctly prevents the error/close double-resolve.
  • waitForApproval(): races the approval callback against the signal, removes its listener on both paths, and rejects (rather than swallowing) callback errors. The post-await signal.aborted re-check in the loop is the right belt-and-braces.
  • worktree: managedSymlinks means removal only unlinks symlinks DeepCode created, and removal is refused when user changes are present. Correct conservative default.

Two behavior changes worth recording (neither blocking):

  1. spawn(..., { detached: true }) moves Bash children into their own process group. That is what makes group-kill possible, but it also means a terminal SIGINT (Ctrl+C) no longer reaches the child directly — cancellation now depends entirely on the AbortSignal path. That path is wired here for tools and in Runtime: bind host safety services behind RuntimeHost #184 for headless SIGINT/SIGTERM, so it holds; it's just now a single point of failure where there used to be two. Worth a regression test if Ctrl+C-during-long-Bash ever misbehaves.

  2. When a caller passes mode but no permissions, the fallback injects allow: SAFE_READONLY_TOOLS. That's additive (an allow entry can't restrict), but it does mean WebFetch/WebSearch are auto-allowed rather than falling through to the mode default for those hosts. Defensible for a host with no approval UI — it's the whole point of the safe-default set — but it is a slightly wider network-egress default than before for that narrow case.

Validation: CI green both runners. Locally at the stack tip: typecheck, lint --max-warnings=0, format, docs-check, 1033 tests pass.

@oratis
oratis merged commit 5f498ea into main Aug 2, 2026
8 checks passed
oratis added a commit that referenced this pull request Aug 2, 2026
…ajors (#215)

Covers the 31-PR Codex alignment stack (#180-#210) plus the dependency
work (#211-#214, #155, #177-#179).

Leads with the security section because that is what this release is:
the central tool gate could be skipped entirely when `mode` was omitted
(#181), and the desktop renderer held provider credentials (#192).

Upgrade notes call out the two things a user could otherwise be
surprised by — the ~6.7 MB → ~115 MB desktop size increase from the Node
sidecar, and that the plugin capability RPC is not an OS boundary.

Co-authored-by: t <t@t>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant