feat(code-review-beta): code review delegation to Codex#784
Open
davidalee wants to merge 15 commits intoEveryInc:mainfrom
Open
feat(code-review-beta): code review delegation to Codex#784davidalee wants to merge 15 commits intoEveryInc:mainfrom
davidalee wants to merge 15 commits intoEveryInc:mainfrom
Conversation
…ewers Introduces a beta skill that mirrors ce-code-review but routes the mid-tier persona reviewers (testing, maintainability, project-standards, plus cross-cutting and stack-specific conditionals) to `codex exec` instead of the platform's subagent primitive, conserving session tokens on a workflow that fans out 6-10+ reviewers per run. High-stakes reviewers (correctness, security, adversarial) stay on the session model so capability is not lost where it matters most. Unstructured-output agents (agent-native, learnings-researcher, schema-drift-detector, deployment-verification-agent) also stay local -- they return prose / checklists, not the findings JSON shape that `--output-schema` enforces. Hardcoded `-s read-only` sandbox: persona reviewers don't write project files, run tests, or build, so workspace-write was YAGNI. Read-only empirically permits read-oriented git/gh evidence-gathering commands. Design hardening from a two-lane (Claude + Codex MCP) adversarial pre-ship review: - Stage 3c persona-file resolution (CLAUDE_PLUGIN_ROOT primary, repo-relative fallback) so delegated `<persona>` blocks can never ship empty - Preflight one delegated reviewer first; if it fails, disable delegation and run all reviewers locally -- avoids the parallel-launch failure cascade that would defeat a post-hoc circuit breaker - Explicit Stage 4 -> Stage 5 barrier on per-reviewer status map; merge cannot start while any reviewer is still pending - mode:report-only suppresses delegation silently (incompatible with the required scratch/artifact writes); mode:headless without recorded consent fails fast with a structured error envelope rather than silently degrading - validate-then-write-full-then-strip ordering for the JSON contract; reversing it would silently empty headless detail-enrichment Beta is `disable-model-invocation: true` -- manual invocation only. Stable ce-code-review and upstream callers (ce-work, lfg, ce-polish-beta) are unchanged.
Harden Codex delegation against unsafe local config, mutable self-review prompt files, shell interpolation, prompt injection, and late background results. Keep GitHub-auth-dependent reviewers local and copy delegated persona sidecars into the beta skill so installed and converted runs are self-contained.
Apply concrete fixes from PR #8 review: - F3: resolve `scripts/resolve-base.sh` via `${CLAUDE_SKILL_DIR}` with bare-path fallback so the call works when the Bash tool runs from the user's project CWD rather than the skill directory. Applied to both stable and beta. - F4: rewrite `{output_contract}` substitution rule to override the compact-only return paragraph in subagent-template.md and align with the `<constraints>` block, so delegated reviewers return FULL JSON (with why_it_matters and evidence) — preventing silently empty Why:/Evidence: lines in headless output. - F14: standardize the gate name as "Self-Review Prompt Integrity Gate" across SKILL.md and the workflow reference; reduce the workflow's section 0b to a one-line back-reference to SKILL.md Stage 4 (the authoritative spec). - F15: add `additionalProperties: false` at root and items in findings-schema for both stable and beta; declare `_meta` as a root property so the existing documentation block remains valid. - F16: replace ".context/ artifact path" with the OS-temp path declared in the same output contract, matching the run-artifact contract. Both copies updated. - F22: append "seconds" to the delegate_timeout_seconds default doc. - F9: scratch-cleanup scope guard. Add explicit assertion `[ -n "$SCRATCH_DIR" ] && [ "$CODEX_HOME" = "$SCRATCH_DIR/codex-home" ]` before any rm of codex-home or auth.json; document SCRATCH_DIR as immutable. - F23: add `Skill: ce-code-review-beta` (and `Skill: ce-code-review` for stable) discriminator line to the headless envelope so callers can gate skill-specific reason-string parsing. - F2 (sub-fix only): add `scripts/resolve-base.sh` to the protected-paths list guarded by the Self-Review Prompt Integrity Gate. - F7: write codex result and exit-code sentinel via tmp-file + atomic mv, with `sync` between, so polling readers never see a partial write or a sentinel that implies a result not yet durable. - F8: hard wall-clock guard inside the polling loop body; document that the polling Bash call inherits the harness's foreground timeout. - F5/F6 (highest-value tests): add compact-split-ordering and circuit-breaker contract tests to tests/review-skill-contract.test.ts. - F10: add stable/beta sidecar parity test to flag silent drift in shared reference files. Updated three existing review-skill-contract tests to match the renamed gate and atomic-rename launch template. Routed to failed bucket (need design decisions or new feature work): F1, F2 (broader deterministic-gate piece), F5/F6 (broader test gap), F11, F12, F13, F18, F19, F20, F21, F24. bun test: 1290 pass, 0 fail. bun run release:validate: in sync.
- chmod 700 on the per-run /tmp artifact directory in stable + beta so per-reviewer findings JSON is owner-only, not world-readable. mkdir -p inherits umask 022 (755), which leaves evidence and delegated-reviewer output exposed to other local users/processes. - Tighten Stage 5 merge-queue rule: populated only from the in-memory status map (status=succeeded), never by re-scanning the scratch directory. Closes the bypass where an `ignored` reviewer's result file could leak into merge despite cancellation. - Tag post-circuit-breaker local-fallback failures distinctly in Coverage so users can distinguish original delegation failures from local re-dispatch failures. - Strengthen Codex-binary smoke-check to use the same `env -i` shape as the actual delegated launch (no TERM, no SHELL, no terminfo). Catches TTY-blocking CLI builds with one short probe instead of waiting out the full review_delegate_timeout_seconds (default 900s). - Align prose terminology with directory name: "delegated personas" / "delegated persona files" instead of "persona sidecars" / "delegated sidecar files". Renames the parity-test describe block to match. Directory `references/delegated-personas/` is unchanged. Stable/beta sync decision: chmod 700 propagated to stable; the Stage 5 barrier, smoke-check, and Coverage tagging changes are beta-only because they apply to the delegation workflow that exists only in beta.
Address findings from the multi-persona PR review (security, swe, architect, qa, devops). Highest-impact fixes are process lifecycle and credential containment in the delegated lane: - Real PID capture + setsid + SIGTERM/SIGKILL cancellation. The prior Bash-tool handle could not kill orphan codex processes. - trap-based auth.json cleanup on EXIT/INT/TERM so credentials never linger in /tmp on Ctrl-C, OOM, or orchestrator crash. - Wave-based fan-out cap via review_delegate_max_parallel (default 4) to bound workstation memory/CPU under N-reviewer fan-out. - Captured stderr per delegated reviewer so failure messages are durable instead of "exit code 1". - chmod 600 on copied auth.json, chmod 700 on every parent of the per-skill scratch path; auth source ownership/mode verified. - NO_PROXY/HTTP_PROXY hard-disabled in the binary smoke probe so a future telemetry call would fail loudly instead of silently leaking. Security checks extracted to scripts so the contract is testable, not just prose: scripts/trust-check-codex.sh and scripts/integrity-check-config.sh. The prose contract is kept alongside the script invocation so future edits can cross-check; tests/review-skill-contract.test.ts now exercises the integrity check behaviorally with fixture filesystems. resolve-base.sh: switched awk to `-v` interpolation, captured fetch stderr into the error envelope so callers can distinguish auth/network/ no-such-branch failures, and added --pr-base-repo / --pr-base-branch flags so PR-mode and standalone mode share one tested code path. The former inline 20-line PR-mode bash in SKILL.md is gone. Findings schema (stable + beta, parity preserved): added $id and an optional top-level schema_version field plus _meta.schema_version "1.0.0" with a written version policy. evidence.minItems dropped from 1 to 0 so reviewers with no specific snippet don't fabricate one. persona-catalog (stable + beta, parity preserved): added a Lane column declaring each reviewer's local-vs-delegation-eligible status, plus a "Lane assignment policy" section so future reviewers don't drift into the wrong lane by default. SKILL.md: added BETA-STATUS pointer, confidentiality warning, documented model allowlist (gpt-5-codex, gpt-5, o4-mini, gpt-5-mini), single-source-of-truth Self-Review Prompt Integrity Gate with explicit trigger globs, max-parallel cap doc, Stage 5 step renumbering (6/6b/6c -> 6/7/8/9), and converted the heterogeneous-fixer-queue prose into a routing table. Trusted config path is re-derived at runtime; the pre-resolved sentinel path is documented as informational only. BETA-STATUS.md: new doc capturing graduation criteria, sunset criteria, telemetry plan, and removal procedure (including the STALE_SKILL_DIRS / EXTRA_LEGACY_ARTIFACTS_BY_PLUGIN registry entries that must be added when the skill is deleted). Tests: 8 new contract tests covering the gate's trigger globs, env scrub, codex-home failed-check name, max-parallel cap presence, BETA-STATUS required sections, script existence and executable bits, behavioral integrity-check (symlink/tracked/gitignore rejection), and schema versioning. Catalog parser regex updated for the Lane column. Skipped with explicit rationale: disable-model-invocation kept (beta convention); local-redispatch on unconfirmed cancellation kept fail-closed (documented as future work in BETA-STATUS); persona-sha256 hashing, dry-run mode, lockfiles, and combined CODEX_DELEGATION.md left out of scope. bun test: 1298 pass, 0 fail. bun run release:validate: clean.
…rkflow Reduce skill body multiplicative cost (per docs/solutions/best-practices/codex-delegation-best-practices-2026-04-01.md): 1. Extract delegation-conditional content from SKILL.md (1076 -> 960 lines) to references/codex-delegation-workflow.md: settings resolution, mode interaction, persona file mapping, model override, delegated dispatch, and JSON return contract. Leave compact stubs at each extraction point. Self-Review Prompt Integrity Gate and lane-split argument parsing stay inline since they fire on every invocation. 2. Restructure codex-delegation-workflow.md: dedup mode handling, lane split, JSON return contract, sandbox rationale, and mixed-model attribution. Convert circuit-breaker action and cancellation-failure handling to step-by-step lists. Dedup the gate-must-pass-before-persona- read rule from 5 restatements to 2. Front-load the Model Override Opus cost warning. Replace prose integrity-check duplication with calls to scripts/integrity-check-config.sh. Update tests/review-skill-contract.test.ts to follow content moves and restructured assertions. No behavior change. Stable ce-code-review/ and parity-enforced shared references untouched.
resolve-base.sh: guard $1 reference under set -u so the parser doesn't abort with "unbound variable" when all positional args were consumed. trust-check-codex.sh: add python3 and perl canonicalization fallbacks before the dirname-only last resort, and explicitly reject a symlinked final component so a symlinked launcher cannot pass the world-writable and repo/scratch checks against the symlink's parent.
…it codes The script previously printed ERROR:<reason> but exited 0 for every branch, so callers using `set -e` or simple `script || handle_error` would silently treat a failed integrity check as success. Align exit codes with the prefix: OK and ABSENT exit 0; every ERROR branch exits 1. Update the test runner to capture stdout regardless of exit status so the ERROR-prefix assertions still pass under the new failing-exit behavior.
Three independent post-review hardenings to the delegation workflow doc,
each with matching contract-test coverage:
- Resolve helper scripts via ${CLAUDE_SKILL_DIR} instead of bare relative
paths. The Bash tool's CWD is the reviewed repo, so a bare invocation
could either fail to find the script or execute a malicious script of
the same name planted by the reviewed PR before any trust check runs.
- Document setsid as not-shipped on macOS base userland and prescribe a
PG_PREFIX probe: when present, prefix `setsid` and cancel via
`kill -SIGNAL -PID` (process group); when absent, omit the prefix and
cancel via `kill -SIGNAL PID` plus `pkill -P` sweep. Replace the EXIT
trap on auth.json with an INT/TERM-only trap so a finishing reviewer
doesn't yank credentials from siblings still running in the same wave.
- Cap review_delegate_max_parallel at a hard maximum of 16 and fall back
to the default for non-integer, non-positive, or oversized values. The
cap is a safety control on local resources and Codex spend, not just a
tuning knob.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1016ed5cb6
ℹ️ 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".
| NO_PROXY="*" \ | ||
| HTTP_PROXY="http://127.0.0.1:1" \ | ||
| HTTPS_PROXY="http://127.0.0.1:1" \ | ||
| timeout 10 "$CANONICAL" --version 2>&1) || PROBE_STATUS=$? |
There was a problem hiding this comment.
timeout probe in trust check
The smoke probe hard-codes timeout 10 ... in the env -i launch, but timeout is not present in default macOS userland, so this command exits 127 and the script rejects otherwise valid Codex binaries. In the environments this skill explicitly targets (including macOS), that makes delegation preflight fail closed every time instead of verifying the binary. Please add a portable fallback (e.g., gtimeout, perl/python timeout wrapper, or a shell-based watchdog) before treating probe failure as binary untrusted.
Useful? React with 👍 / 👎.
The merge from main converted the ce-code-review-beta skill mention into a markdown link pointing at docs/skills/ce-code-review-beta.md, but no such doc exists. Keep the skill mention; drop the link.
PR EveryInc#779 added pipe-escaping guidance to the stable copy of references/review-output-template.md but the beta copy didn't pick it up through the main->branch merge. The shared-reference parity test was failing because of the drift; copying stable's content over beta's restores byte parity and unblocks CI.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51217e5076
ℹ️ 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".
davidalee
changed the title
davidalee
changed the title
davidalee
changed the title
…ote matching - Remove CWD fallback from RESOLVE_SCRIPT in all three scope modes (PR, branch, standalone). After gh pr checkout the CWD is the reviewed repository; a malicious PR could plant scripts/resolve-base.sh there and intercept execution. Fail closed with an explicit error when CLAUDE_SKILL_DIR is unset instead. - Add boundary check to awk remote-matching in resolve-base.sh so "org/repo" does not spuriously match "org/repo-extra". Uses a tail_ok() helper that asserts the URL ends with the repo path followed by .git or end-of-string.
💡 Codex Review Parse base repo host-agnostically in resolve-base flow
The PR-base detection is hard-coded to ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
ce-code-review-beta, a beta skill that mirrors stablece-code-reviewand optionally delegates mid-tier persona reviewers tocodex exec. Goal: keep the existing review quality model on large fan-outs while reducing token pressure on the orchestrating session.Stable
ce-code-reviewis unchanged, as are upstream callers (ce-work,lfg,ce-polish-beta).Review Lane Split
references/delegated-personas/so the skill is self-contained after plugin conversion.What's in the Diff
plugins/compound-engineering/skills/ce-code-review-beta/— new skill:SKILL.md,BETA-STATUS.md, references for delegation workflow, persona catalog, schema, output template, walkthrough, bulk-preview, diff-scope, tracker-defer, validator template, subagent template, and 13 vendored persona files.scripts/— three executable helpers:resolve-base.sh(fork-safe base-branch resolution),integrity-check-config.sh(config trust check),trust-check-codex.sh(Codex binary trust check).plugins/compound-engineering/README.md— skill-count bump only.tests/review-skill-contract.test.ts— +562 lines covering lane split, delegated-persona byte-parity, prompt escaping, integrity gate ordering, mode behavior, binary trust, isolated Codex home, fixed launch env, exit sentinels, cancellation, ignored late results, max-parallel cap, and schema parity.No release-owned versions, manifests, or changelogs touched.
Behavior
Mode gating.
mode:headlessfails fast with a machine-readable error envelope when consent or prechecks are missing.mode:autofixfalls back to local.mode:report-onlystays local. Only Interactive mode shows the consent prompt.Config trust boundary. Pre-resolution emits only a sentinel; runtime config is read only after
integrity-check-config.shpasses (symlink rejection on.compound-engineering/andconfig.local.yaml, regular-file requirement, gitignore coverage, not-tracked-by-git, resolved-path-stays-inside-root). Exit code matches the prefix soset -ecallers fail closed.Codex binary trust.
trust-check-codex.shcanonicalizes the path with a portable fallback chain and rejects any remaining symlink, shell metacharacters, repo/scratch paths, world-writable parents, and bins that fail a 10-second--versionsmoke probe under the sameenv -ishape used at launch (with proxy vars hard-disabled). Catches nvm/asdf shim and TTY-blocking failures up front.Trust-script invocation. Both trust scripts are invoked via
bash "${CLAUDE_SKILL_DIR}/scripts/...", never bare relative paths — the Bash tool's CWD is the reviewed repo, so a bare path could resolve to a script planted by the reviewed PR before the trust check ran.Prompt-injection mitigation. Persona text, PR metadata, intent summaries, file lists, and diffs are XML-escaped before insertion. Raw placeholders (
{diff},{persona_content},{pr_metadata}) are explicitly rejected. Delegated prompts are forbidden from reading home,~/.codex, auth, history, sessions, rules, or plugin state.Self-Review Integrity Gate. Stage 4 checks for diff modifications to delegated personas, the workflow file, schema, scripts, scope rules, or subagent templates before reading any of those files; if matched, delegation is disabled for the run. Vendored persona files are tested byte-for-byte against the canonical agent source.
Runtime isolation. Per-run
CODEX_HOMEunder scratch (chmod 700).auth.jsonis the only file copied (chmod 600) and only after source ownership/mode are verified. Launches useenv -iwith fixedPATH/HOME/CODEX_HOME,--ignore-user-config,--ignore-rules,--cd "$REPO_ROOT", and-s read-only.Process lifecycle. PIDs captured per reviewer;
setsidforms a process group when available with a portablepkill -Pfallback for systems without it (e.g., macOS). Cancellation sendsSIGTERMthenSIGKILL. Result files written viamv -ffrom a tmp path; exit-code sentinels written aftersync. Stage 5 merge consumes only the in-memory status map —ignoredreviewers' on-disk files do not enter merge.Credential cleanup. Trap on
INT/TERM(notEXIT) wipes copiedauth.jsonon signal exit.EXITis excluded becauseCODEX_HOMEis shared across reviewers and the first reviewer's normal exit would otherwise wipe credentials out from under siblings still running. End-of-run cleanup handles the happy path.Fan-out cap.
review_delegate_max_parallel(default4, hard maximum16) bounds the wave-based scheduler. Out-of-range values fall back to the default — the cap is a safety control, not a tuning knob.Verification
bun test: 1292 pass, 0 fail.bun run release:validate: in sync; compound-engineering reports 49 agents, 39 skills, 0 MCP servers.Suggested Reviewer Focus
previous-commentslocal.CODEX_HOME, credential cleanup, fixed launch env.