Skip to content

fix(sandbox): resolve codex-prefixed pull request roles - #164

Open
igoraboim wants to merge 1 commit into
theam:mainfrom
igoraboim:fix/codex-prefix-pull-request-mode
Open

fix(sandbox): resolve codex-prefixed pull request roles#164
igoraboim wants to merge 1 commit into
theam:mainfrom
igoraboim:fix/codex-prefix-pull-request-mode

Conversation

@igoraboim

@igoraboim igoraboim commented Aug 16, 2026

Copy link
Copy Markdown

Closes #163.

What changes

githubPullRequestMode now strips the codex- prefix from runs.mode, exactly as readOnlyRepositoryMode and repairPullRequestMode already do. After this, a Codex-engine reviewer or repair agent resolves to the same governed role as its Claude counterpart and receives the pull request's branch in its run bundle.

Why

runs.mode is the agent's name (routes/v1/runs.ts:246) and Codex agents carry a codex- prefix by convention — the seed ships codex-architect and codex-builder (packages/db/src/seed.ts:52-53). githubPullRequestMode was the only role predicate that did not normalise that prefix, and it decides whether the bundle gets the PR branch (orchestrator.ts:1733).

A project that follows the naming convention and adds a Codex reviewer or repair agent gets checkoutBranch = null and clones the default branch:

  • codex-review reviews the default branch while reporting on the pull request — silent.
  • codex-address-review / codex-ci-doctor also pin an admitted head through repairExpectedHeadSha, which does strip the prefix. The bundle then asks the runner to clone the default branch and verify the PR's head, so the run fails at repository_head_sha_mismatch (runner/src/index.ts:460-471) before provisioning.

The two predicates disagreed about the same run. The second test added here pins that agreement rather than just the prefix handling, so the invariant survives a future role being added to one list and not the other.

The predicate is exported to make it testable, following the precedent of repairExpectedHeadSha (exported and covered in test/sandbox.test.ts).

Verification

  • services/api full suite: 234 passed, 16 skipped (npx vitest run --fileParallelism=false). The skips are the Docker/Postgres-backed tiers — see the caveat below.
  • Targeted: test/sandbox.test.ts, test/orchestrator-checks.test.ts, test/ci-doctor-policy.test.ts, test/address-review-policy.test.ts, test/sandbox-capabilities.test.ts — 69 passed in the four policy suites.
  • node guards/run.mjs — 2 guards ran, 0 failed.
  • npx tsc --noEmit in services/api — clean. npx biome check on both changed files — clean.
  • The new tests were confirmed to fail without the fix. I reverted githubPullRequestMode to its previous body and re-ran: both new cases fail (expected false to be true, at the codex- assertions and at the branch/head agreement check), then pass again with the fix restored. They are a regression pin, not a restatement.

Caveat, stated plainly: I could not run pnpm verify end to end — Docker is unavailable on this machine, so the isolated-database integration tier and the sandbox E2E could not execute locally. Everything above is what I could actually run. I also have not exercised this against a live Codex agent; the impact description is traced from the code path, and I would value a maintainer sanity-check on how you expect projects to name Codex agents.

  • pnpm verify passes locally — not run, no Docker available; see the caveat above
  • Behaviour verified beyond the test suite — verified by reverting the fix and confirming the new tests fail, and by tracing the bundle through buildRunBundle to the runner's repository_head_sha_mismatch check
  • Documentation updated, or no user-facing change — no user-facing change

`githubPullRequestMode` was the only role predicate that did not strip the
`codex-` prefix from `runs.mode`, so a Codex-engine reviewer or repair agent
resolved to a different role than its Claude counterpart.

Because `runs.mode` is the agent's name and Codex agents carry a `codex-`
prefix by convention (the seeded `codex-architect` and `codex-builder`), a
project that adds `codex-review`, `codex-address-review` or `codex-ci-doctor`
gets `checkoutBranch = null` in `buildRunBundle` and clones the default branch
instead of the pull request's branch:

- `codex-review` silently reviews the default branch while reporting on the
  pull request.
- `codex-address-review` and `codex-ci-doctor` also pin an admitted head via
  `repairExpectedHeadSha`, which does strip the prefix, so the bundle asks the
  runner to clone the default branch and verify the pull request's head. The
  run fails at `repository_head_sha_mismatch` before provisioning.

Strip the prefix exactly as `readOnlyRepositoryMode` and `repairPullRequestMode`
already do, and pin the invariant that the checked-out branch and the pinned
head agree for every repair role.

Co-Authored-By: Claude <noreply@anthropic.com>
@UsamaIslam

Copy link
Copy Markdown

Hit this exact bug independently while auditing the runner/gate boundary, before finding this PR — same fix (mode.replace(/^codex-/, "").replace(/-/g, "_"), matching repairPullRequestMode). I verified it two ways: a full truth table across all mode variants (bare and codex--prefixed review/address-review/ci-doctor/builder/architect/security-sweep — only the three PR-bound codex- modes change), and reverting just the regex while keeping the export, which reproduces exactly one failure: codex-review expected true, got false. Consistent with what you found — independent +1.

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the solid contribution!

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.

fix(sandbox): codex-prefixed review and repair agents clone the default branch

3 participants