Skip to content

sandboxing: use OsString for SandboxCommand.program#15897

Merged
bolinfest merged 1 commit intomainfrom
pr15897
Mar 26, 2026
Merged

sandboxing: use OsString for SandboxCommand.program#15897
bolinfest merged 1 commit intomainfrom
pr15897

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Mar 26, 2026

Why

SandboxCommand.program represents an executable path, but keeping it as String forced path-backed callers to run to_string_lossy() before the sandbox layer ever touched the command. That loses fidelity earlier than necessary and adds avoidable conversions in runtimes that already have a PathBuf.

What changed

  • Changed SandboxCommand.program to OsString.
  • Updated SandboxManager::transform to keep the program and argv in OsString form until the SandboxExecRequest conversion boundary.
  • Switched the path-backed apply_patch and js_repl runtimes to pass into_os_string() instead of to_string_lossy().
  • Updated the remaining string-backed builders and tests to match the new type while preserving the existing Linux helper arg0 behavior.

Verification

  • cargo test -p codex-sandboxing
  • just argument-comment-lint -p codex-core -p codex-sandboxing
  • cargo test -p codex-core currently fails in unrelated existing config tests: config::tests::approvals_reviewer_* and config::tests::smart_approvals_alias_*

@bolinfest bolinfest changed the title fix: use OsString for SandboxCommand.program sandboxing: use OsString for SandboxCommand.program Mar 26, 2026
@bolinfest bolinfest requested a review from viyatb-oai March 26, 2026 18:48
@bolinfest bolinfest enabled auto-merge (squash) March 26, 2026 18:52
@bolinfest bolinfest merged commit dfb3657 into main Mar 26, 2026
85 of 92 checks passed
@bolinfest bolinfest deleted the pr15897 branch March 26, 2026 20:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants