Skip to content

refactor(runner): share execution context across shell launch paths - #1408

Merged
swjnxyf merged 1 commit into
mainfrom
codex/runner-shell-execution-context
Nov 9, 2026
Merged

swjnxyf merged 1 commit into
mainfrom
codex/runner-shell-execution-context

Conversation

@yyjeqhc

@yyjeqhc yyjeqhc commented Nov 9, 2026

Copy link
Copy Markdown
Owner

Runner launch paths repeatedly forwarded configuration generation, policy, shell configuration, profile registry/cache, cwd, timeout, and cancellation through long positional argument lists. Carry these required borrowed fields in ShellExecutionContext across synchronous dispatch, structured Jobs, Skill resources, interactive/detached preparation, builds, formatting, and internal search.

Process and script entry points now accept their start/preparation hooks directly, removing three production forwarding wrappers and one test-only profile wrapper. Private ProcessOutputOptions names the environment, hooks, output mode, and deadline choices at the internal process boundary. Command preparation and execution bodies retain their existing behavior, including policy checks, process ownership, cancellation, bounded I/O, preparation fences, and start notification order. No wire or persisted representation changes are involved.

The change spans 21 files, removes 17 argument-count lint suppressions, and reduces production code by 258 lines (153 lines net including expanded test coverage). Moving functions into impl blocks accounts for part of the larger diff; reviewing with whitespace ignored makes the substantive changes clearer.

Validation on macOS:

  • Before migration: the focused shell/Job/build/format suites passed 214 tests, with 5 existing ignored tests.
  • Final focused shell/Job/build/format/profile/dispatch suites: 247 passed, 5 ignored. runner_skills::tests: 6 passed.
  • Five targeted ignored tests passed with runner-real-process-tests: profile timeout reclaims the descendant process group; Rust build observes selected cached artifacts; Python and Rust formatter profile preparation obey the total deadline; rustfmt stdin leaves sources untouched.
  • Added a real-child regression proving that a rejected preparation fence produces no payload effect or started notification, while an admitted invocation preserves environment overrides and callback order.
  • Compared the unchanged execution bodies for native process, detached preparation, script, raw shell, Windows native search, and Windows internal POSIX/search paths after accounting for argument binding and formatting.
  • Production cargo check --locked -p webcodex-runner --bin webcodex-runner passed without warnings. Test compilation with --features runner-real-process-tests --no-run, package formatting, and git diff --check passed.

Native Windows/Linux execution was not run locally.

@swjnxyf swjnxyf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the 21-file Runner shell execution-context refactor at a840cde13ffac3379651d5c8b577e27864b90279 against the prior dispatch, structured/interactive/detached process, script, Skill, build, format and internal-search paths. Policy checks, config generation and profile cache, cwd restrictions, timeout/cancellation propagation, preparation/start hooks, and internal environment overrides remain wired consistently; no blocking behavioral differences found. The PR's complete Linux/Windows/macOS CI run is green. I also checked out the exact PR head in an isolated worktree on mini and ran cargo test --locked --offline -p webcodex-runner --bin webcodex-runner shell -- --test-threads=4 (149 passed, 0 failed). LGTM.

@swjnxyf
swjnxyf merged commit d523d53 into main Nov 9, 2026
18 checks passed
@swjnxyf
swjnxyf deleted the codex/runner-shell-execution-context branch November 9, 2026 15:54
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.

2 participants