Codex gates at explicit high effort + resolved-config echo (#110)

[yihanzhu]

What

Both scripts/codex-review.sh and scripts/manager-review.sh now:

• Source config/models.conf resolved relative to the script's own location (this clone's control-plane root, following symlinks — never a hardcoded personal path).
• When the reviewed/debated target repo has a committed .fabrica/models.conf, source it AFTER the shipped defaults — read from the same fetched/anchored worktree the review runs against (the temp detached worktree checked out at the exact PR head / anchored default-branch commit), never the operator's possibly-stale or dirty cwd checkout.
• Always pass -c model_reasoning_effort="$FABRICA_REVIEW_EFFORT" (codex-review.sh) / "$FABRICA_DEBATE_EFFORT" (manager-review.sh). -m <model> is passed only when a model is actually resolved — the existing -m CLI flag keeps precedence over FABRICA_CODEX_MODEL.
• Echo the resolved model + effort into the posted comment's header: reviewer: <model> @ <effort> (e.g. reviewer: operator-default @ high when nothing was pinned), so every review/debate documents on the record what gated it.
• Fail loudly on a missing/unsourceable config/models.conf (pointing at scripts/doctor.sh check (k)) rather than silently reviewing/debating at an unknown effort. Same for an unsourceable per-target override.

reviewer/codex-review.md and reviewer/manager-review.md are updated with a new "Model policy (#110)" section documenting the precedence and header echo.

Why

scripts/codex-review.sh and scripts/manager-review.sh previously ran codex exec with no explicit reasoning-effort override, so they silently inherited whatever the operator's personal Codex CLI/config (~/.codex/config.toml) defaulted to — often low. That's an unintended weakening of the merge-gating review and the manager-debate gate, both of which are supposed to always run at maximum capability (spend-by-leverage — see config/models.conf, from #109/PR #113). This raises both gates to an explicit, resolved effort (high by default) and makes the resolution auditable in the PR/issue history.

A portability fix found along the way

While testing the new sourcing logic I found that if ! . "$file"; then … does not reliably catch a sourcing failure under set -e on older bash (e.g. macOS's shipped /bin/bash 3.2.57) — the shell aborts the whole script immediately with the sourced command's own exit code, bypassing the intended then branch entirely. Both scripts now toggle set +e around the . call and check $? explicitly, which behaves consistently across bash versions. Verified this concretely with isolated repros before landing the fix (see commit message).

Test plan

• shellcheck -x -S style clean against the CI-pinned 0.9.0 release (downloaded the static binary; local install is 0.11.0 and was not used for the check).
• bash scripts/test/north-star-resolver.test.sh — 64/64 pass.
• bash scripts/test/north-star-gate.test.sh — 190/190 pass (this suite exercises manager-review.sh end-to-end; make_cp_clone was updated to also stage config/models.conf into its throwaway control-plane clones, since the script now requires it relative to its own location).
• Manual hermetic smoke tests (fake gh/codex on PATH, real git, throwaway bare remote simulating a PR) covering both scripts: default run (reviewer: operator-default @ high), CLI -m precedence, per-target .fabrica/models.conf override applying both model and effort, CLI -m still beating an override's model, a dirty uncommitted local override being correctly ignored (worktree/committed state wins), and the missing/unsourceable-config failure paths.

Closes #110

Sync: goes live as soon as the live checkout pulls main (scripts are invoked by absolute path). This raises the review/debate gate from implicitly-inherited low effort to explicit high — a rail-strengthening change approved in the model-tiering design session.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 294e0b9
Reviewed-base: 1425781

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/115-37446/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

No actionable findings could be identified because the repository diff could not be inspected: the execution environment failed to start its command host.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 294e0b9
Reviewed-base: 1425781

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/115-38046/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

No actionable findings could be identified because the repository inspection tool failed to start, so the requested diff could not be examined.

[cloudflare-workers-and-pages]

Deploying fabrica with    Cloudflare Pages

Latest commit:	c35bf1d
Status:	✅  Deploy successful!
Preview URL:	https://28fc6ef7.fabrica-6yx.pages.dev
Branch Preview URL:	https://coder-110-codex-gates.fabrica-6yx.pages.dev

View logs

[yihanzhu]

Revision: fixed P1 from the adversarial review

The P1. codex-review.sh was sourceing the PR head's .fabrica/models.conf directly into the main (non-sandboxed) harness shell. Two live attacks followed: (1) a malicious PR could put shell in that file and get arbitrary code execution with the operator's gh/codex credentials; (2) even without injection, any PR could commit FABRICA_REVIEW_EFFORT=low to downgrade its own review gate — the only prior check was "non-empty," no allowlist/floor. manager-review.sh was already anchored correctly (reads from the fetched default-branch commit) but still sourced the file as shell.

The fix — two parts, applied to both scripts:

1. Trust anchor (codex-review.sh only). The .fabrica/models.conf override is now read from the gh-bound default branch, fetched fresh into a private per-run ref — the same class of anchor manager-review.sh already used — never the PR head. A PR can no longer read (let alone execute) its own override; the trust anchor is always already-merged, already-gated content.
2. Parse, not source (both scripts). A new shared helper, scripts/lib/models-conf.sh (mc_parse_target_override), reads the override as data: only lines matching FABRICA_<allowedkey>=<value> (tight charset, optionally quoted) are recognized; everything else — comments, blank lines, shell metacharacters, command substitutions — is silently ignored and never executed. No eval, no source, bash-3.2 compatible, shellcheck-0.9.0 clean.
3. Gate keys are not target-overridable. The parser recognizes FABRICA_REVIEW_EFFORT/FABRICA_DEBATE_EFFORT but never applies them from a target override — only the producer/model keys (FABRICA_CODER_MODEL, FABRICA_HANDS_MODEL, FABRICA_CODEX_MODEL) can be set. An attempt prints a warning that is folded into the posted PR/issue comment header (warning: target override attempted to set gate effort — ignored), never silent.

Tests added:

• scripts/test/models-conf-parser.test.sh (new, 44 assertions) — isolated unit tests for the parser: malicious-file content never executes while valid keys in the same file still parse; gate-effort keys are recognized-but-rejected with a warning; valid producer-key overrides apply; charset/quoting edge cases; plus static source-text assertions confirming both scripts actually wire in the fix (no more direct . "$target_models_conf", both call mc_parse_target_override, codex-review.sh resolves/fetches the default branch).
• A new end-to-end case in scripts/test/north-star-gate.test.sh (7 assertions) exercising the real manager-review.sh against a target-committed malicious override (shell injection + gate-downgrade attempt) with real git + faked gh/codex.
• All existing suites stay green: 64 + 197 + 44 = 305 assertions, 0 failures.
• Manually verified end-to-end against the real codex-review.sh with a hermetic bare-repo/gh/codex harness: a malicious PR-head payload (shell injection + FABRICA_REVIEW_EFFORT=low + a spoofed model) neither executes nor affects the posted review — the resolved model/effort come entirely from the trusted default-branch override instead.

Docs updated: reviewer/codex-review.md, reviewer/manager-review.md, README.md (also corrected stale "gates don't read this yet" wording), and templates/repo-setup.md.

Confirmation both P1 scenarios are now dead:

• Arbitrary code execution via shell injection — impossible: the override is never sourced/evald, only parsed as data through a strict allowlist/charset filter.
• A PR downgrading its own review gate — impossible for two independent reasons: the override is read from the default branch (not the PR head) via the trust-anchor fix, and, even if it were somehow read from attacker-controlled content, the parser refuses to ever apply FABRICA_REVIEW_EFFORT/FABRICA_DEBATE_EFFORT from a target override at all.

[yihanzhu]

Revision: P2 fix — symlink-safe read of the target .fabrica/models.conf override

A verifier confirmed the earlier P1 fix (parse-not-source) is solid, but found a new P2 in the same code path on this PR:

Finding: scripts/manager-review.sh (~line 599-602 before this commit) read the target's per-target override as mc_parse_target_override < "$worktree/.fabrica/models.conf" — a shell redirection from the checked-out worktree path, which follows symlinks. A target repo committing .fabrica/models.conf as a symlink to an arbitrary operator-local regular file would pass the [ -f ] check, and the parser would read the pointed-to file's content. A charset-valid FABRICA_CODEX_MODEL=<value> line in that file would then be echoed into the public issue comment header (reviewer: <value> @ high) — a narrow info-leak of an arbitrary local file. (The device/FIFO DoS variant was already blocked by [ -f ].)

Its sibling scripts/codex-review.sh was already symlink-safe: it reads the override via git show "<anchor_commit>:.fabrica/models.conf" (a symlink's blob content is just the link-path string, which fails the charset check — no dereference).

Fix (commit c35bf1d): manager-review.sh now reads the override the same way, via git show "${head_commit}:.fabrica/models.conf" piped through a here-string into mc_parse_target_override — using the exact $head_commit anchor already resolved for the north-star reads (no new fetch). Everything else about the override handling is unchanged (gate-key immunity for FABRICA_DEBATE_EFFORT, the warning echo, the charset parse).

Tests added:

• scripts/test/north-star-gate.test.sh (22): commits .fabrica/models.conf as a symlink to a sentinel file carrying a valid FABRICA_CODEX_MODEL=leaked-sentinel-model line, runs the real gate end-to-end (fake gh/codex, real git), and asserts the sentinel value never appears in the output and the resolved model falls back to operator-default.
• scripts/test/models-conf-parser.test.sh (h): static source assertions that manager-review.sh now reads via git show "${head_commit}:.fabrica/models.conf" and no longer reads from the checked-out worktree path.

All suites green: 64 + 201 + 46 = 311 (baseline 305 + 6 new assertions), verified under both the pinned shellcheck 0.9.0 and real bash 3.2 (/bin/bash on this host is 3.2.57).