Skip to content

feat: formalize the manager-reviewer (rounds + issue-as-bus harness) - #48

Merged
yihanzhu merged 7 commits into
mainfrom
issue-44-manager-reviewer
Jun 26, 2026
Merged

feat: formalize the manager-reviewer (rounds + issue-as-bus harness)#48
yihanzhu merged 7 commits into
mainfrom
issue-44-manager-reviewer

Conversation

@yihanzhu

Copy link
Copy Markdown
Owner

Closes #44

What

Adds a cross-vendor manager-reviewer — the mirror of codex-review.sh one layer up. Where the code reviewer debates a diff with the PR as the message bus, this debates a proposed issue with the ISSUE as the message bus, asking Codex whether a Faber-drafted proactive proposal is worth raising toward the north star. Proceed only on consensus; the reviewer is veto-only.

  • scripts/manager-review.sh <issue#> — reads the north star from NORTH_STAR.md, runs codex exec -c sandbox_mode="read-only" with the manager-reviewer prompt + north star + the issue title/body + "read the repo to ground your judgment", asks for a PROCEED / REFINE / DROP verdict (+ reasoning + any gap Faber missed), and posts Codex's response VERBATIM as an issue comment (also echoes to stdout).
  • NORTH_STAR.md (new, tracked) — current north star + done-signal + log; seeded with A — "Frictionless first-run" (active). The script reads from it.
  • debating label — added to setup-target-repo.sh's labels array (covered by --check automatically) + the label lists in templates/repo-setup.md and RESTORE.md.
  • Faber intake flow (manager/CLAUDE.md + templates/faber-command.md) — proactive issues: draft (not ready, labeled debating) → manager-review.shconsensus proceed (remove debating, then the human front gate) / REFINE re-run (a round, cap ~2) / DROP or no-consensus-by-cap close. Veto-only, default-drop, but log in NORTH_STAR.md when Faber thought a vetoed item was north-star-relevant. User-directed issues skip the gate.
  • reviewer/manager-review.md (new) — documents the mechanism: issue-as-bus rationale, rounds model, consensus/veto rule, the prompt, how to invoke.
  • Manifest + README Layoutscripts/manager-review.sh, reviewer/manager-review.md, NORTH_STAR.md added.

Why

The manager debate has been done ad-hoc (codex exec by hand). This formalizes it into a reproducible, rounds-based harness + doc, with the same cross-vendor split that decorrelates blind spots (manager = Claude/Faber, manager-reviewer = Codex), and the same comments-only / read-only invariants as the code reviewer — one level up.

Invariants (mirror codex-review.sh)

Read-only sandbox forced (-c sandbox_mode="read-only", never --dangerously-bypass-*); comments-only (its only write is the single issue comment — never edits the issue, labels, pushes, or merges); unset GH_REPO + explicit --repo on every gh call; -m passthrough (no hardcoded model); preflight (gh, codex); numeric <issue#> validation; set -euo pipefail; mktemp + trap cleanup; shellcheck-clean; executable.

How tested

Self-modification call-out (REQUIRED)

This changes prompt docs + adds a new label, so it does not change live behavior until synced:

  • Prompt changemanager/CLAUDE.md takes effect on Faber's next read; the /faber command needs scripts/install.sh re-run to pick up the updated templates/faber-command.md.
  • New debating label — must be created on each target repo via the updated scripts/setup-target-repo.sh (a normal run force-edits/creates it; --check will report it missing until then).
  • Safety rails preserved: reviewer stays read-only / comments-only / veto-only; no auto-merge touched; rounds cap + needs-human intact.

🤖 Generated with Claude Code

Add a cross-vendor MANAGER reviewer that debates whether a proactive,
Faber-drafted issue is worth raising toward the north star — the mirror of
codex-review.sh one layer up, with the ISSUE as the message bus. Proceed only
on consensus; the reviewer is veto-only.

- scripts/manager-review.sh <issue#>: reads the north star from NORTH_STAR.md,
  runs `codex exec -c sandbox_mode="read-only"` with the manager-reviewer prompt
  + north star + issue, asks for a PROCEED/REFINE/DROP verdict, and posts Codex's
  response VERBATIM as an issue comment (also echoed to stdout). Mirrors
  codex-review.sh's invariants: read-only forced, comments-only (single issue
  comment), `unset GH_REPO` + explicit --repo, -m passthrough, preflight, numeric
  arg validation, set -euo pipefail, shellcheck-clean, +x, mktemp+trap cleanup.
- NORTH_STAR.md (new, tracked): current north star + done-signal + log; seeded
  with A — "Frictionless first-run" (active). manager-review.sh reads from it.
- `debating` label added to setup-target-repo.sh's labels array (+ usage line),
  and to the label lists in templates/repo-setup.md and RESTORE.md.
- Faber intake flow (manager/CLAUDE.md + templates/faber-command.md): proactive
  issues are drafted (not ready, labeled `debating`) -> manager-review.sh ->
  consensus proceeds (remove debating, then the human front gate) / REFINE
  re-runs (round, cap ~2) / DROP closes. Veto-only, default-drop, log overrides.
  User-directed issues skip the gate.
- reviewer/manager-review.md (new): documents the mechanism, rounds model,
  consensus/veto rule, the prompt, and how to invoke it.
- Manifest + README Layout updated with the 3 new files.

Verified end to end: ran scripts/manager-review.sh 44 — Codex's verdict posted
verbatim as a comment on issue #44. shellcheck -S style clean on all scripts;
structure check passes incl. the 3 new files.

Closes #44

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu yihanzhu added the round-0 Review-loop counter: initial PR label Jun 26, 2026
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 7b561af
Reviewed-base: e7cf905

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

The new manager-review workflow can bypass the explicit user approval gate, and the harness is unusable for normal external target repos because it looks for the north star in the wrong repository. These are functional/safety issues in the introduced workflow.

Full review comments:

  • [P1] Keep proactive issues behind the user gate — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.otJ9y2QJED/reviewer/manager-review.md:35-35
    When a proactive issue reaches Codex/Faber consensus, this step tells Faber to apply ready and enter the coder loop. For proactive issues there has not been explicit user approval yet, and ready is the launch cue, so following this doc bypasses the front gate/self-approval invariant despite the manager prompt saying to bring it to the user first. Change this path to remove debating and ask the user for approval before applying ready.

  • [P2] Resolve NORTH_STAR from the control plane — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.otJ9y2QJED/scripts/manager-review.sh:106-106
    In the documented workflow this script is run by absolute path from a target repo, but this resolves NORTH_STAR.md relative to that target's git root. setup-target-repo/Quickstart only create labels and do not install a NORTH_STAR.md in target repos; the new file added by this PR lives in the control-plane repo. For any external target repo, manager-review exits before running Codex unless the user has manually copied a file the setup never mentions.

…solve NORTH_STAR.md from control plane

Round-1 review feedback on #48.

[P1] The proactive-issue flow said consensus → apply `ready` (enter the
coder loop), which bypasses the front gate: `ready` is the coder launch
cue and only the user's explicit approval may apply it. Change the flow
in reviewer/manager-review.md, manager/CLAUDE.md, and
templates/faber-command.md so consensus only *vets* — it removes
`debating` and brings the issue to the USER for front-gate approval
(`ready` still requires the user's sign-off; Faber never self-applies it).
Add a note that fully-autonomous proactive mode (consensus AS the gate) is
a deliberate front-gate change deferred to #49, pending the user's sign-off.

[P2] manager-review.sh resolved NORTH_STAR.md from the cwd/target-repo git
root, but the file lives in the control-plane (fabrica) repo. Resolve it
from the script's own location (follow symlinks, then dirname/..), the same
derivation install.sh/doctor.sh use, so it reads the control plane's north
star regardless of which target repo it runs from. Keep the actionable
missing-file error.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu yihanzhu added round-1 Review-loop counter: revision 1 and removed round-0 Review-loop counter: initial PR labels Jun 26, 2026
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-1: both Codex findings addressed in 25d69d4.

[P1] Keep proactive issues behind the user gate. The proactive-issue flow no longer self-applies ready on consensus. In reviewer/manager-review.md, manager/CLAUDE.md, and templates/faber-command.md, CONSENSUS to proceed now means: remove debating, then bring the vetted issue to the user for the front gateready is applied only on the user's explicit approval. The manager-debate is a vetting / recommendation step, not an approval; "never self-approve / the user's approval is the front gate" is preserved. Added a note (all three docs) that fully-autonomous proactive mode — manager-debate consensus AS the gate, no per-issue user approval — is a deliberate front-gate change deferred to #49, pending the user's explicit sign-off (today = vet then user gates; #49 = consensus gates after sign-off).

[P2] Resolve NORTH_STAR.md from the control plane. scripts/manager-review.sh now resolves NORTH_STAR.md from the script's own location (follow symlinks, then dirname/..) — the same derivation install.sh/doctor.sh use — so it reads the control-plane repo's north star regardless of which target repo's cwd it runs from. The actionable missing-file error is kept (and clarified to point at the control-plane repo). The doc prose in manager-review.md step 1 was updated to match.

Verification: shellcheck scripts/*.sh clean; structure check passes; manager-review.sh stays +x. Ran the script from /tmp (a non-fabrica cwd) — it gets past the NORTH_STAR.md resolution (no "not found" error) and only fails later at the gh-repo guard, confirming the north star is now resolved from the control plane independent of cwd. The manager-review.sh mechanism, debating label, issue-as-bus/rounds/veto-only content, and manifest entries are intact.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 25d69d4
Reviewed-base: e7cf905

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

The new manager-review flow can be invoked from generated /faber sessions without loading the active control-plane north star, which undermines the proactive issue gate. The rest of the shell changes appear structurally sound.

Review comment:

  • [P2] Add NORTH_STAR.md to Faber startup sources — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.iDraZBRcWD/templates/faber-command.md:16-16
    When a fresh /faber session starts in a target repo, this new proactive-flow instruction refers to NORTH_STAR.md by a relative path, but that file lives in the Fabrica control-plane and target repos do not have it. For proactive issues, Faber needs the active north star before drafting and when logging vetoed-but-relevant drops, so add {{FABRICA_ROOT}}/NORTH_STAR.md to the startup files or qualify the path here.

Round-2 [P2]: a fresh /faber session in a target repo couldn't find
NORTH_STAR.md — it was referenced by a relative/bare path, but the file
lives in the Fabrica control plane, not in target repos.

- templates/faber-command.md: add {{FABRICA_ROOT}}/NORTH_STAR.md to the
  startup source-of-truth read-list, and qualify both body references
  (north-star steer + vetoed-but-relevant LOG) to {{FABRICA_ROOT}}/NORTH_STAR.md
  so install.sh substitutes the control-plane path.
- manager/CLAUDE.md: clarify NORTH_STAR.md is this control-plane repo's
  file (not target repos'), resolved by manager-review.sh regardless of cwd.

Docs-only; structure check + shellcheck unaffected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu yihanzhu added round-2 Review-loop counter: revision 2 and removed round-1 Review-loop counter: revision 1 labels Jun 26, 2026
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-2: Codex [P2] addressed in 2448619.

[P2] Add NORTH_STAR.md to Faber startup sources / qualify the path. A fresh /faber session in a target repo couldn't resolve NORTH_STAR.md — it was referenced by a relative/bare path, but the file lives in the Fabrica control plane, not in target repos.

  • templates/faber-command.md: added {{FABRICA_ROOT}}/NORTH_STAR.md to the startup source-of-truth read-list (alongside {{FABRICA_ROOT}}/manager/CLAUDE.md, README.md, etc.), so Faber loads the active north star up front. Both body references — the north-star steer in the manager-debate gate and the vetoed-but-relevant LOG — are now qualified to {{FABRICA_ROOT}}/NORTH_STAR.md, which install.sh substitutes to the control-plane path.
  • manager/CLAUDE.md: clarified that NORTH_STAR.md is this control-plane repo's file (not target repos'), resolved by manager-review.sh regardless of which target repo's cwd it runs in. The other references now read unambiguously against that anchor.

Verification: docs-only — structure check passes and shellcheck -S style scripts/*.sh is clean (no scripts touched; manager-review.sh stays +x). The vet-then-user-gate flow, #49 reference, manager-review.sh, NORTH_STAR.md, debating label, and docs are all intact.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 2448619
Reviewed-base: e7cf905

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

The new manager-review harness can produce incomplete reviews when run from a repo subdirectory, despite documenting that it may be run from within the clone. This should be fixed so Codex consistently reviews the full target repository.

Review comment:

  • [P2] Run manager review from the repo root — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.oDi3N2uooh/scripts/manager-review.sh:219-223
    When this is invoked from a subdirectory of the target repo, which the usage text allows and gh repo view accepts, codex exec is launched with that subdirectory as its working root while still posting a verdict to the repo-level issue. That can ground the manager-review in only part of the repository; resolve the git top-level and pass it via -C, or reject non-root invocations.

manager-review.sh launched `codex exec` in the current working directory.
Usage allows running from a subdirectory of the target repo (and
`gh repo view` resolves <owner>/<repo> from any subdir), so from a subdir
Codex would ground its read-only review in only that subtree while still
posting a repo-level verdict to the issue.

Resolve the git top-level (`git rev-parse --show-toplevel`) after the
existing gh-repo guard and pass it to Codex via `codex exec -C "$repo_root"`
(the `-C` flag precedes the prompt, matching codex-review.sh), so the
manager-review always reads the whole repo regardless of cwd. Header
comment updated to match. Read-only sandbox, verbatim posting, GH_REPO
pinning, and NORTH_STAR control-plane resolution all unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu yihanzhu added round-3 Review-loop counter: revision 3 (cap) and removed round-2 Review-loop counter: revision 2 labels Jun 26, 2026
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-3: Codex [P2] addressed in 0634588.

[P2] Run manager review from the repo root. manager-review.sh launched codex exec in the current working directory. Since usage allows invoking from a subdirectory of the target repo (and gh repo view resolves <owner>/<repo> from any subdir), running from a subdir would ground Codex's read-only review in only that subtree while still posting a repo-level verdict to the issue.

Fix: after the existing gh-repo guard, resolve the git top-level with repo_root="$(git rev-parse --show-toplevel)" (with an actionable error if it can't be resolved) and pass it to Codex via codex exec -C "$repo_root" … — the -C flag precedes the prompt, the same pattern codex-review.sh uses with -C <worktree>. So the manager-review now reads the whole repo regardless of cwd. The script's header comment was updated to match.

Verification: shellcheck scripts/manager-review.sh clean (also -S style, the CI floor); structure check passes; manager-review.sh stays +x. Confirmed codex exec supports -C/--cd <DIR> ("use the specified directory as its working root") and that git rev-parse --show-toplevel returns the repo root from a subdir; dry-ran the assembled command — codex exec -C <repo_root> -c sandbox_mode=read-only -o <tmp> [-m <model>] <prompt> (all options before the positional prompt). Read-only sandbox (-c sandbox_mode="read-only"), verbatim issue-comment posting, unset GH_REPO + explicit --repo, NORTH_STAR.md control-plane resolution, preflight, and the vet-then-user-gate flow are all unchanged.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 0634588
Reviewed-base: e7cf905

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

The new manager-review workflow has safety and resumability gaps: it runs Codex against the live checkout, does not pass issue-comment history into later debate rounds, and introduces a debating state that status scans do not surface.

Full review comments:

  • [P1] Isolate Codex from the live checkout — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.YCUz6B6RLG/scripts/manager-review.sh:232-232
    When manager-review.sh is run in a target repo that has untracked/ignored local files such as .env or local WIP, this command gives Codex the operator's live worktree. The read-only sandbox only prevents writes; it does not stop the review from reading those files or judging against uncommitted state, unlike codex-review.sh's temp worktree. Use a clean detached worktree at the commit being debated, or otherwise restrict Codex to tracked files before invoking it.

  • [P2] Feed issue comments into review rounds — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.YCUz6B6RLG/scripts/manager-review.sh:153-158
    In a REFINE round, the documented flow has Faber reply in an issue comment and rerun the script, but the script only fetches title/body here. Any prior Codex verdicts and Faber replies on the issue bus are invisible to the next Codex run, so it can repeat the same objection or miss the refinement rationale. Include the relevant issue comment thread in the prompt for reruns.

  • [P2] Surface debating issues in status scans — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.YCUz6B6RLG/manager/CLAUDE.md:26-28
    When a proactive issue is created with debating and the Faber session ends before consensus, the current status/brief routines still scan only merge-ready, needs-human, ready, and idle issues. That leaves this new in-progress state out of later status checks, so issue-as-bus rounds are not reliably resumable from GitHub state. Add debating to the manager/brief label queries.

…d, surface debating

Addresses three Codex findings on PR #48 (round-3):

- [P1] Isolate Codex from the live checkout. manager-review.sh now runs the
  review in a clean DETACHED temp worktree at HEAD (git worktree add --detach
  <tmpdir> HEAD, under mktemp -d), mirroring codex-review.sh's worktree+trap
  pattern, and points codex via -C <worktree>. The read-only sandbox blocked
  writes but not READS, so Codex could read untracked/ignored files (.env,
  secrets, WIP) or judge against uncommitted state; it now sees only tracked
  content at HEAD. Trap cleans up the worktree + temp files on every exit.
  Added git to the preflight tool check.

- [P2] Feed the issue thread into REFINE rounds. The script now also fetches
  the issue's comments (gh issue view --json comments) and includes the
  rendered comment thread in the prompt, so a rerun sees the prior Codex
  verdicts and Faber's refinement replies (the issue-as-bus debate).

- [P2] Surface debating issues in status scans. Added `debating` to the label
  queries in manager/CLAUDE.md Tracking and routines/brief.md, so an
  in-progress manager-debate whose session ended is resumable from GitHub state.

Kept intact: read-only codex exec, verbatim issue-comment posting, unset
GH_REPO + explicit --repo, NORTH_STAR.md control-plane resolution, preflight,
numeric-arg guard, vet-then-user-gate flow, #49 reference. shellcheck clean
(default + -S style), structure check passes, manager-review.sh stays +x.

Note: prompt change is a proposal — faber-command.md/manager prompt take
effect only when re-pasted into the live routine.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-3: all three Codex findings addressed in 3d8a36e.

  • [P1] Isolate Codex from the live checkout. manager-review.sh now runs the review in a clean detached temp worktree at HEAD (git worktree add --detach <tmpdir> HEAD under mktemp -d) and points Codex at it via codex exec -C "$worktree", mirroring codex-review.sh's worktree+trap pattern. The read-only sandbox blocked writes but not reads, so Codex could previously read untracked/ignored files (.env, secrets, WIP) or judge against uncommitted state; it now sees only the tracked content at HEAD. The trap removes the worktree (git worktree remove --force) + temp files on every exit (success or failure). Added git to the preflight tool check.

  • [P2] Feed issue comments into review rounds. The script now also fetches the issue's comments (gh issue view <issue#> --json comments) and includes the rendered comment thread (@author (createdAt): body, oldest first) in the prompt, so a REFINE rerun sees the prior Codex verdicts and Faber's refinement replies (the issue-as-bus debate) instead of repeating an already-addressed objection.

  • [P2] Surface debating issues in status scans. Added debating to the label queries in manager/CLAUDE.md Tracking and routines/brief.md, flagged resumable — an in-progress manager-debate whose session ended before consensus is now picked up from GitHub state (re-run manager-review.sh to continue, or drop).

Kept intact: read-only codex exec (-c sandbox_mode="read-only"), verbatim issue-comment posting, unset GH_REPO + explicit --repo, NORTH_STAR.md control-plane resolution, preflight, numeric-arg guard, vet-then-user-gate flow, #49 reference, set -euo pipefail.

Verification: shellcheck scripts/*.sh clean (default + -S style, the CI floor); structure check passes; manager-review.sh stays +x. Dry-ran the worktree mechanics — git worktree add --detach <tmp> HEAD materializes the full tracked tree and the trap cleans it up (gone from git worktree list after exit); confirmed the comments jq filter (.author.login / .createdAt / .body, with a (no comments yet) empty branch) renders against gh's bundled jq. Did not post stray issue comments. Label left at round-3.

Prompt-change note: the manager/CLAUDE.md / routines/brief.md edits are a proposal — they take effect in the live routines only when re-pasted into the Claude UI.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3d8a36e
Reviewed-base: e7cf905

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

The implementation largely appears functional, but the new manager-review documentation contradicts the script's actual temp-worktree isolation behavior, which is a safety-relevant maintenance issue.

Review comment:

  • [P3] Correct the manager-review execution description — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.GGDcf00bbZ/reviewer/manager-review.md:104-105
    When someone relies on this runbook to audit or modify manager-review.sh, this paragraph says Codex reads the operator's checkout and that there is no temp worktree. The new script actually creates a detached worktree at HEAD and runs codex exec -C "$worktree", so the doc reverses the safety property around dirty/untracked files and can lead maintainers/operators to reason about the wrong inputs.

…HEAD)

Codex [P3]: reviewer/manager-review.md's "how it runs" step described
Codex as reading the operator's checkout with no temp worktree — the
reverse of the script's actual safety property. manager-review.sh now
creates a clean detached temp worktree at HEAD (git worktree add
--detach <worktree> HEAD under mktemp -d) and runs codex exec -C
<worktree>, cleaned up via trap, mirroring codex-review.sh's isolation.

Update the paragraph (and the step-2 command snippet) to describe the
real behavior: the review runs in a clean detached temp worktree at
HEAD, isolated from the live checkout, so Codex sees only tracked
content at HEAD — never untracked/ignored/uncommitted files. Note that
the read-only sandbox blocks writes but not reads, so the worktree (not
the sandbox) is what keeps the operator's dirty/local state out.

Closes #44

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-3: Codex [P3] addressed in dcc7198.

[P3] Correct the manager-review execution description. reviewer/manager-review.md's "how it runs" paragraph (step 2's codex exec snippet + the ~104-105 prose) said Codex reads the operator's checkout and that there is no temp worktree — the reverse of the script's actual safety property. manager-review.sh creates a clean detached temp worktree at HEAD (git worktree add --detach <worktree> HEAD under mktemp -d) and runs codex exec -C <worktree>, cleaned up via trap.

Updated to describe the real behavior: the review runs in a clean detached temp worktree at HEAD, isolated from the operator's live checkout, so Codex sees only tracked content at HEAD — never untracked/ignored/uncommitted files (.env, secrets, local WIP). Added that the read-only sandbox blocks writes but not reads, so the worktree (not the sandbox) is what keeps the operator's dirty/local state out, mirroring codex-review.sh's isolation. The step-2 command snippet now shows codex exec -C <worktree> … too. Rest of the doc unchanged.

Verification: docs-only (one file, +10/-3) — structure check passes; shellcheck unaffected (no scripts touched). Re-read the paragraph against manager-review.sh: it now matches the temp-worktree behavior. Label left at round-3; no merge/approve.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Faber — paused: Codex reviewer rate-limited

Final confirming re-review is blocked — the Codex CLI hit its usage limit (resets ~11:18 AM). State: CI green; the last completed Codex review (round-3/4) had no code findings — only a [P3] doc-description mismatch, fixed in dcc7198 (pure doc paragraph). Awaiting the confirming Codex pass on the current head before merge — not bypassing the cross-vendor gate. Will resume the moment Codex is back.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: dcc7198
Reviewed-base: e7cf905

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

The new manager-debate flow has a conflicting generated instruction around pre-approval labels, and the harness passes potentially large/sensitive issue content via argv. These are actionable issues that can break or undermine the new workflow.

Full review comments:

  • [P2] Exempt debating from the pre-approval label ban — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.hwhTAVPRtd/templates/faber-command.md:17-17
    For proactive issues, this new instruction says to draft the issue and apply debating before asking for user approval, but the immediately preceding generated /faber rule still says “Never label an issue the user hasn't approved.” In that scenario the live prompt is self-contradictory, so Faber may skip the debating label or wait for approval before running the manager review, breaking the new debate/brief state flow. Please narrow the no-label rule to ready or explicitly exempt debating.

  • [P2] Send the Codex prompt over stdin — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.hwhTAVPRtd/scripts/manager-review.sh:273-273
    When an issue has a long body or a few debate comments, $prompt can contain GitHub's 65k issue body plus multiple comment bodies; passing that as a command-line argument can fail with E2BIG once the thread grows, and on shared machines it also exposes the issue/north-star text in process listings while Codex runs. codex exec supports -/stdin, so feed the prompt through stdin or a temp file rather than argv.

Address two Codex [P2]s on PR #48:

- Narrow the no-label ban to `ready` (exempt `debating`). The proactive
  manager-debate flow applies `debating` before user approval, but the
  front-gate rule said "never label an issue the user hasn't approved" —
  self-contradictory. Narrow it to "never apply `ready`" and note tracking
  labels like `debating` are fine pre-approval. (templates/faber-command.md,
  manager/CLAUDE.md)

- Feed the Codex prompt via stdin, not argv. The assembled prompt (north
  star + issue body + comment thread) was passed as a `codex exec` argv
  argument — risks E2BIG for long issues and exposes content in process
  listings. Now piped over stdin via the trailing `-` positional, with all
  flags before it. (scripts/manager-review.sh; doc snippet in
  reviewer/manager-review.md kept in sync)

All of #48's behavior intact (temp-worktree isolation, read-only sandbox,
verbatim posting, NORTH_STAR control-plane resolution, #49 reference,
`debating` label). shellcheck clean (+ -S style), structure check passes,
script stays +x.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@yihanzhu

Copy link
Copy Markdown
Owner Author

Round-3: both Codex [P2]s addressed in 3e571fe.

[P2] Exempt debating from the pre-approval label ban. The new proactive flow applies debating before user approval, but the front-gate rule said "never label an issue the user hasn't approved" — a self-contradiction that could make Faber skip debating or wait for approval before the manager-debate. Narrowed the rule to ready specifically in both generated/persona docs:

  • templates/faber-command.md: "Never apply ready to an issue the user hasn't approved, and never self-approve" + a note that tracking labels like debating are fine before approval (they record in-progress state, not a go; only ready is gated on the user's sign-off).
  • manager/CLAUDE.md (Front gate): same narrowing to ready, with the same debating-is-fine-pre-approval note. (The ## Merge & never "never self-approve" line was already ready-specific — left as is.)

[P2] Send the Codex prompt over stdin. scripts/manager-review.sh passed the assembled $prompt (north star + issue body + full comment thread) as a codex exec argv argument — risks E2BIG once the thread grows and exposes the issue/north-star text in process listings. Now fed over stdin via the trailing - positional: printf '%s' "$prompt" | codex exec -C "$worktree" -c sandbox_mode="read-only" -o "$tmp" [-m "$model"] -. All flags stay before the -. Kept -C "$worktree", -c sandbox_mode="read-only", -o, and the -m passthrough intact. The step-2 snippet in reviewer/manager-review.md was updated to match the stdin form.

Kept intact: manager-review mechanism, detached temp-worktree isolation at HEAD, verbatim issue-comment posting, unset GH_REPO + explicit --repo, NORTH_STAR.md control-plane resolution, debating label, vet-then-user-gate flow, #49 reference, set -euo pipefail.

Verification: shellcheck scripts/manager-review.sh clean (default + -S style, the CI floor); structure check passes; manager-review.sh stays +x. Confirmed via codex exec --help that the positional [PROMPT] is "read from stdin" when omitted (the - form), and dry-ran the assembled command to confirm flag-then-- ordering with the prompt only on stdin (never argv). Did not post stray issue comments. Label left at round-3.

Prompt-change note: the templates/faber-command.md / manager/CLAUDE.md edits are a proposal — they take effect in the live /faber routine only when re-pasted into the Claude UI.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3e571fe
Reviewed-base: e7cf905

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

No discrete, actionable bugs were found in the changed scripts or documentation. The new manager-review harness is wired into the required-files manifest and passes shell syntax and shellcheck validation.

@yihanzhu
yihanzhu merged commit 2571ffa into main Jun 26, 2026
1 check passed
@yihanzhu
yihanzhu deleted the issue-44-manager-reviewer branch June 26, 2026 15:38
yihanzhu added a commit that referenced this pull request Jun 26, 2026
…ve issues (#51)

* feat: move front gate to north-star altitude; consensus gates proactive issues

Closes #49

Implements the user-authorized front-gate rail change: the user's gate moves
up an altitude to the north star / direction; within an approved north star,
Faber⇄Codex manager-debate consensus gates proactive issues (no per-issue
user approval). User-directed issues keep the direct gate (the user's ask).

- manager/CLAUDE.md: front gate reframed to the north-star altitude with two
  paths to `ready` (user-directed = user's ask; proactive = consensus). The
  manager-debate CONSENSUS branch now applies `ready` and runs the loop with
  no per-issue ask. "Never self-approve" reframed to Faber-alone-can't /
  Faber+Codex-consensus-can. User is involved only at north-star achieved /
  goal drift / needs-human.
- templates/faber-command.md: same reconciliation (front gate, manager-debate
  consensus branch, launch flow, "never self-approve").
- README.md: loop diagram + "Judgment lives at the direction" design decision
  now show the two gate paths and the north-star altitude.
- reviewer/manager-review.md: rounds diagram + consensus rule + the #49 note
  rewritten from "deferred / vet-then-gate" to "consensus IS the gate."
- CLAUDE.md: self-mod rail line clarified to Faber-alone-can't / consensus-can.

Rails preserved: reviewer read-only/comments-only/veto-only, CI hard gate,
rounds cap, needs-human, auto-merge safety (in-session merge-pr.sh, high-risk
→human), north-star log, #48's debating-pre-approval clarification.

Self-modification call-out: this is the user-authorized front-gate rail change.
It does NOT change live behavior until synced — the `/faber` command needs
`scripts/install.sh` re-run to pick up the updated templates/faber-command.md;
manager/CLAUDE.md takes effect on Faber's next read. Docs/prompt-only; CI
structure check + shellcheck stay green.

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

* fix: gate proactive consensus on user-approved north star; clarify coder contract

Address Codex round-0 [P2]s on #51:

1. Require a USER-APPROVED north star before the proactive consensus path
   can apply `ready`. The active north star is the user's authorization, so
   proactive autonomy is only legitimate when the north star is explicitly
   approved by the user. Added a fresh-setup guard: an unset / not-approved /
   shipped-default north star means Faber does NOT auto-pursue — it asks the
   user to set + approve first.
   - manager/CLAUDE.md + templates/faber-command.md: proactive path + the
     manager-debate gate now have an explicit gate-0 precondition.
   - NORTH_STAR.md: added an approval status line (approved-by-user) so the
     state is legible; adopters must set + approve their own.
   - QUICKSTART.md + templates/repo-setup.md: one-liner that setting +
     approving the north star unlocks proactive autonomous mode.

2. Fix the coder authorization contract for consensus-cleared issues.
   `ready` now means "cleared to run — via the user's direct approval OR
   Faber⇄Codex consensus toward an approved north star." Reworded
   routines/coder.md (header, intro, step 0) and the `ready` label
   description (setup-target-repo.sh + templates/repo-setup.md). The
   "if not `ready`, stop" guard is kept. coder-revision.md had no such
   wording — left unchanged.

Docs/string-only; structure check passes, shellcheck clean.

Note: prompt changes here are PROPOSALS — they do not change live Faber/coder
behavior until the human re-pastes the updated instructions into the routine.

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

* fix: align user-directed front gate to spec approval (round-2)

Reconcile the [P2] consistency gap between the manager prompt and
QUICKSTART. The prompt read as if a user-directed issue was cleared by
the user's initial one-liner (implying Faber could `ready` on draft),
contradicting QUICKSTART's approve-the-drafted-issue step.

Resolve by keeping the user spec-approval front gate and aligning the
prompt to it: user-directed = Faber drafts the spec → user approves the
drafted spec → Faber applies `ready` (the one-liner is the request;
approval is of the resulting spec; drafting alone never earns `ready`).
Proactive path (consensus-as-gate under a user-approved north star) and
all rails left intact.

- manager/CLAUDE.md, templates/faber-command.md: reword the user-directed
  path (front-gate bullet, skip-manager-debate note, never-self-approve)
  to "draft → user approves the spec → ready".
- QUICKSTART.md step 9: tighten to "Approve the drafted spec"; loop order
  unchanged (one-liner → draft → approve → ready). User-approval step kept.

Structure check passes; docs-only (shellcheck unaffected).

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

* docs: fix user-directed front-gate wording in README + 3 others (#49)

Round-3 [P2]: README.md (and NORTH_STAR.md, CLAUDE.md, reviewer/manager-review.md)
still said the user's "explicit ask" was the gate for a user-directed issue,
implying Faber could `ready` an issue on the one-liner before the user reviews
the drafted spec. Since /faber reads README as source-of-truth, the contradiction
with the round-2-corrected manager/CLAUDE.md + QUICKSTART.md persisted.

Corrected all four to match: for a user-directed issue the one-liner is the
*request* -> Faber drafts the spec -> the user approves that drafted spec ->
Faber applies `ready` (drafting alone does not earn `ready`). The proactive
path stays consensus-gated under a user-approved north star. All other rails
left intact.

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

* docs: propagate user-directed spec-approval gate + surface north-star setup step

Addresses the two round-3 Codex findings on #51.

[P2] Make every depiction of the user-directed gate show "approve the drafted
spec -> ready", not "ask -> ready":
- README.md: rewrote the loop diagram's user-directed line (one-liner = request
  -> Faber drafts spec -> YOU approve that drafted spec -> ready; drafting alone
  never earns ready).
- reviewer/manager-review.md: "the user's explicit request is the judgment" ->
  "the user's approval of the spec Faber drafts from their one-liner is the
  judgment (the one-liner is the request, not the go)".
- RESTORE.md: three spots tightened to spec-approval (the front-gate paragraph,
  the smoke-test approve step, and the safety-rail front-gate line).

[P3] Surface the required north-star approval in setup output:
- scripts/install.sh: added a "set + approve the north star" step to "Next steps".
- scripts/setup-target-repo.sh: added the same as follow-up #5.

Self-modification note: these are prompt/docs proposals; live Faber/coder behavior
does not change until the human re-pastes the updated instructions into the routine.

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

* docs: anchor proactive gate on operator approval, not inheritable token; align RESTORE rail (round-3)

Addresses the two Codex [P2]s on PR #51:

[P2] Shipped north star must not auto-unlock proactive mode. Reframe the
proactive gate so it keys on the operator's explicit approval of the active
north star (Faber's own knowledge), NOT an in-file `approved-by-user` token a
fresh clone would inherit as "approved for me":
- NORTH_STAR.md: approval lines are now descriptive owner-history + an
  adopter-must-approve guard; the status/log lines say the history is not an
  inheritable token.
- manager/CLAUDE.md + templates/faber-command.md: proactive precondition and
  gate-0 check now state the precondition as "the operator has explicitly
  approved the active north star," and explicitly say any in-file text (incl.
  the shipped default) is NOT that approval.
- Propagated to the onboarding surfaces that told adopters to "mark the status
  line approved-by-user": QUICKSTART.md, templates/repo-setup.md,
  scripts/install.sh, scripts/setup-target-repo.sh now say set + explicitly
  approve your own north star (Faber gates on the approval, not the file line).

[P2] Align the RESTORE safety-rail with the consensus gate. RESTORE.md (~226)
rail now states both paths: user-directed = approve the drafted spec; proactive
= user-approved north star + Faber<->Codex consensus; `ready` = cleared via
either path; Faber never self-applies it acting alone.

Rails intact: consensus-as-gate for proactive, user-directed = approve-the-spec,
north-star altitude, never-self-approve-alone. shellcheck clean; structure check
passes; scripts remain +x.

Self-modification note: these are prompt/docs changes — proposals that do NOT
change live Faber/coder behavior until the human re-pastes the updated
instructions into the routine.

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

* docs: order north-star approval after /faber is summoned

QUICKSTART's fresh-clone flow told adopters to "tell Faber you approve
the north star" before the step that opens Claude Code and runs /faber —
but no Faber session exists yet to receive that approval. install.sh and
setup-target-repo.sh mirrored the same ordering.

Split editing from approving:
- QUICKSTART step 6 now only *sets* NORTH_STAR.md (a pre-flight, file-only
  step); step 7 runs /faber and then approves the north star in-session.
  Renumbered/cross-refs fixed (step 9 + doctor.sh comment point at step 7).
- install.sh / setup-target-repo.sh printed output: set your north star in
  the file; approve it with Faber once you run /faber (unlocks proactive
  mode), in-session not by editing the file.

Round-5 anchor reframe kept intact: approval is the operator's explicit
approval (not an inheritable in-file token); consensus-as-gate, user-
directed = approve-the-drafted-spec, all rails unchanged.

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

* docs: reword stale ready="approved" to "cleared to run" (#49)

Addresses Codex [P2]: several spots still described a `ready` issue as
"approved" / "approved but no PR picked up yet". For a consensus-cleared
proactive issue that is wrong and could make Faber misreport it as
user-approved. `ready` now means "cleared to run" via the user's
spec-approval OR Faber⇄Codex consensus toward a user-approved north star.

Fixed `ready`-status descriptions:
- routines/brief.md: brief queue entry
- manager/CLAUDE.md: loop step ("strictly means") + Tracking queue entry
- README.md: "one coder launch per approved issue" -> "per cleared issue"
- RESTORE.md: §2 self-guard note + §5 troubleshooting "Coder won't start"

Genuinely-correct "approve/approved" uses left intact (user approves the
drafted spec, the user-approved north star, manager-reviewer veto,
never-self-approve). Gate logic unchanged.

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

* docs: scope no-per-issue-approval to proactive; mirror north-star step-0 guard into reviewer flow (#49)

Addresses both Codex [P2]s in the final consistency pass:

- README (~71-72): scope "stop approving each issue" / "no per-issue
  approval" to PROACTIVE work only. User-directed issues still require
  the user's approval of the drafted spec ("not exempt from per-spec
  approval"); only the proactive consensus path has no per-issue ask, and
  it is conditional on an explicitly operator-approved active north star.
- reviewer/manager-review.md: add the same step-0 north-star-approval
  guard that manager/CLAUDE.md + templates/faber-command.md carry. Before
  drafting, running the manager-debate, or applying `ready` on consensus,
  Faber must confirm the operator has explicitly approved the ACTIVE
  north star (a fresh adopter clone / shipped default is NOT approved).
  Added both to the rounds-model diagram and the consensus rule prose.

Gate logic unchanged — wording aligned + the missing guard added. Both
invariants now hold across every surface (re-grep confirmed).

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

* docs: scope autonomy/altitude framing to proactive; add authoritative two-gates rule (round-3)

Codex [P2]: manager/CLAUDE.md stated unscoped that "everything inside an
approved north star is Faber's to drive" / "I am involved only at the
north-star altitude", which conflicted with the user-directed spec-approval
gate and could let Faber skip per-spec approval for user-directed issues.

- Scope the autonomy/altitude statement in manager/CLAUDE.md to *proactive*
  work; user-directed issues still require approval of the drafted spec.
- Add ONE authoritative, up-front "two gates" rule near the top of the
  front-gate section in both manager/CLAUDE.md and templates/faber-command.md,
  stated once and unambiguously, that governs every later autonomy statement.
- Scope the remaining "Faber's to drive" / "pulled back in only at" / "user's
  gate at the north-star altitude" framing in reviewer/manager-review.md,
  README.md, and CLAUDE.md to proactive, each restating the user-directed
  spec exception.

Gate logic unchanged: consensus-as-gate (proactive) + gate-0 north-star guard,
user-directed = approve-the-spec, never-self-approve-alone, all rails intact.
Structure check passes; shellcheck clean (no scripts touched).

Self-modification note: prompt/docs changes are proposals — they do not change
live Faber behavior until the human re-pastes the updated instructions.

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

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

round-3 Review-loop counter: revision 3 (cap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formalize the manager-reviewer: rounds + issue-as-bus harness (scripts/manager-review.sh)

1 participant