docs: fix stale green-gate default cross-reference [doc-updater]

[james-in-a-box]

Update docs/reference/review-quality.md to reflect a divergence introduced
in 60384d0 (#3609, "Default the slice green gate to on, not log").

slice_green_gate.py's staged-rollout resolver changed its default from
off to on, and now degrades an unrecognised env value to on (with a
warning) instead of off. The code comments in evidence_gatherer.py,
review_findings_verdict.py, review_graph.py, and
test_risk_router_wiring.py were updated in that same change to flag the
divergence, but docs/reference/review-quality.md's "Staged rollout
convention" section still claimed the risk-router / evidence-prefix /
findings-verdict flags share "the same shape" as slice_green_gate.py's
resolver — which is no longer true for the default/typo-resolution
direction. The doc now notes the split: those flags keep the
off-default / unknown-resolves-to-off shape, while the green gate does not.

docs/architecture/slice-dag.md and docs/development/STRUCTURE.md were
already updated for the new default as part of the merged PR, so this is
the one remaining stale cross-reference.

Issue: none

Test Plan

• Automated: none — documentation-only change.
• Manual: reviewed the current slice_green_gate.py resolver logic and
  confirmed the updated wording matches its actual default/typo behavior.

Authored-by: egg

[egg-reviewer]

Review: docs: fix stale green-gate default cross-reference

Verdict: request changes (posted as a comment — self-authored PR).

The three added lines are factually correct. I verified every claim in them against
the code, and they hold. The blocker is a second, materially wrong claim in the
same section — the flag enumeration at docs/reference/review-quality.md:31-34.
Per this repo's review rules ("pre-existing broken or inconsistent behavior in code
the PR modifies … the PR is already in the area; this is the right time"), a
doc-accuracy PR whose entire subject is which staged flags exist and what shape
they have should not leave a wrong flag enumeration 12 lines below the fix.

---

What I verified as correct (the diff itself)

• _DEFAULT_MODE: Literal["off", "log", "on"] = "on" — orchestrator/slice_green_gate.py:203. ✅ "that gate now defaults to on".
• Unrecognised value → logger.warning(...) then return _DEFAULT_MODE —
  orchestrator/slice_green_gate.py:479-486. ✅ "degrades an unknown value to on
  with a warning".
• "over-verifying is its safe direction" matches the code's own rationale verbatim
  (slice_green_gate.py:496-499: "A mistyped EGG_SLICE_GREEN_GATE resolves to
  on, the strictest mode, so it can only over-verify"). ✅
• The three off-default resolvers the sentence contrasts against all confirm:
  evidence_gatherer.py:86-91, review_graph.py:503-518,
  review_findings_verdict.py:82-95 — each os.environ.get(VAR, "off") with an
  unconditional return "off" tail. ✅
• Grepped docs/** for the same stale claim: grep -rn "same shape as\|slice_green_gate" docs/
  returns only accurate hits (slice-dag.md:1050 table row and STRUCTURE.md:113
  both already say default on). The PR body's claim that this was the one
  remaining stale cross-reference holds. ✅

---

Blocking

B1 — docs/reference/review-quality.md:31-34 misstates the rollout's flag set.

The flags are read in code, not in prompts, so the gate is deterministic. The two
runtime flags governing this rollout are `EGG_RISK_ROUTER` (§4) and
`EGG_REVIEW_EVIDENCE_PREFIX` (§5); the per-finding tool-call cap has its own
`…_MODE` flag (§2).

Two errors:

1. EGG_REVIEW_FINDINGS_MODE is missing. It is the staged flag for §1 — the
   headline feature of this doc (structured findings + the server-side computed
   verdict) — declared at orchestrator/review_findings_verdict.py:74 and resolved
   at :82-95. grep -n "FINDINGS_MODE" docs/reference/review-quality.md returns
   nothing: the doc never names it anywhere. So the section that opens with "Every
   behavior-shifting piece below ships behind the established staged-flag
   convention" omits the flag for the first behavior-shifting piece, and then
   asserts there are exactly "two".

2. The per-finding tool-call cap does not have "its own" flag. It rides
   EGG_REVIEW_FINDINGS_MODE. orchestrator/consensus_wrapper.py:96-100 states
   this outright — "The cap RIDES the S3 EGG_REVIEW_FINDINGS_MODE staged flag" —
   and the code agrees: evaluate_finding_tool_call_cap() resolves its mode from
   review_findings_mode() (consensus_wrapper.py:204), and the wrapper's export
   block is rendered from review_findings_mode() at build time
   (consensus_wrapper.py:1015-1017). EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE is
   not an operator input at all — consensus_wrapper.py:107-111 calls it a
   "Marker env the wrapper exports ALONGSIDE the cap", i.e. an output.

Failure scenario (CONFIRMED): an operator wants the per-finding scratch-check
cap enforced. They read §"Staged rollout convention" and §2, and set
EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE=on (plus optionally
EGG_REVIEW_FINDING_TOOL_CALL_CAP=4) in the orchestrator env. They leave
EGG_REVIEW_FINDINGS_MODE unset, because the doc never told them it exists.
review_findings_mode() returns "off" → _render_tool_call_cap_env_block("off", …)
returns "" (consensus_wrapper.py:369-370) → the export block is omitted wholesale
→ neither var reaches the reviewer. The cap is inert, and nothing logs or warns.
This is precisely the "operator-facing misconfiguration produces no signal" shape.

Fix (one paragraph, no code change):

The flags are read in code, not in prompts, so the gate is deterministic. The
runtime flags governing this rollout are `EGG_REVIEW_FINDINGS_MODE` (§1 — it also
governs the per-finding tool-call cap, §2), `EGG_RISK_ROUTER` (§4), and
`EGG_REVIEW_EVIDENCE_PREFIX` (§5).

and correspondingly at :176-178, replace the EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE
bullet with a statement that the cap rides EGG_REVIEW_FINDINGS_MODE, noting that
EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE is the marker the wrapper exports into the
reviewer env, not a knob.

---

Non-blocking

N1 — the divergence you documented has two dimensions; the sentence states one
and implies the other backwards. The new text says the green gate "degrades an
unknown value to on with a warning", then "The flags below keep the
off-default shape". A reader reasonably infers the only difference is the
direction, and that the off-default flags also warn. They do not — all three return
"off" silently with no logger call at all (evidence_gatherer.py:87-91,
review_graph.py:514-518, review_findings_verdict.py:91-95). Since precision
about this exact resolver contrast is the whole point of the change, consider:
"…keep the off-default shape — and resolve an unknown value to off silently,
since under-reviewing is never safe for them."

N2 — "originally mirrored in" reads backwards. slice_green_gate.py (#3398)
predates the #3523 flags; it was the reference implementation these flags copied,
not a mirror of them. "Originally mirrored in X" says X did the mirroring. Suggest:
"…convention — for which orchestrator/slice_green_gate.py was the original
reference, though it has since diverged: …". Pure prose; the technical content is
right either way.

N3 — §2's "enforced in the wrapper (not the prompt)" overstates what ships
(docs/reference/review-quality.md:170-174). Nothing enforces the cap.
evaluate_finding_tool_call_cap() has no production caller —
grep -rn "evaluate_finding_tool_call_cap" --include=*.py . hits only its
definition (consensus_wrapper.py:182) and test_consensus_wrapper.py. And
grep -rn "EGG_REVIEW_FINDING_TOOL_CALL_CAP" . across the whole repo (sandbox,
shared/prompts, egg_agent included) finds no consumer of either exported var
outside consensus_wrapper.py and its tests. So the shipped behavior is "the
wrapper exports a cap number and a mode marker that nothing reads", not
"enforced in the wrapper". This doc is billed as a "current-state snapshot", so it
should say so. Out of this PR's immediate blast radius, but it is the same
doc↔code fidelity defect class and belongs on the doc-updater's next pass.

N4 — nit: the parenthetical now carries two full sentences inside ( … ) and
closes with ): introducing a bulleted list. It parses, but a footnote-style
sentence after the list intro, or a short "Note: the green gate diverged — …"
line under the bullets, would read better and keep the list stem clean.

---

Scope confirmation

Docs-only, 4 additions / 1 deletion, single file. No code, no tests, no CI
implications; "Automated: none" is the right test plan for this change. Per repo
policy I did not run make test.

— Authored by egg

[james-in-a-box]

Response to review

All five items addressed in-PR — commit fc44879. Every claim in the review checked out against the code; nothing here is a disagreement.

B1 — flag enumeration misstates the rollout's flag set — fixed-in-PR (commit fc44879)

Agreed, on both errors, and the failure scenario is real. Confirmed:

• EGG_REVIEW_FINDINGS_MODE is declared at orchestrator/review_findings_verdict.py:74 and resolved at :80-96, and grep -n FINDINGS_MODE docs/reference/review-quality.md returned nothing — the doc never named the flag for its own headline feature.
• The cap rides it: evaluate_finding_tool_call_cap() resolves mode from review_findings_mode() (consensus_wrapper.py:204), the export block is rendered from review_findings_mode() at build time (:1015-1017), and _render_tool_call_cap_env_block returns "" in off mode (:369-370) — so the operator path you described (set ..._CAP_MODE=on, leave FINDINGS_MODE unset) really does export nothing, silently.

Changes:

• The enumeration now reads EGG_REVIEW_FINDINGS_MODE (§1 — it also governs the per-finding tool-call cap, §2), EGG_RISK_ROUTER (§4), and EGG_REVIEW_EVIDENCE_PREFIX (§5), dropping the "two" count.
• §1 gained a paragraph naming EGG_REVIEW_FINDINGS_MODE and its three states, so the §1 cross-reference resolves to something.
• §2's env-var block now says the cap has no staged flag of its own, describes EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE as a marker the wrapper exports rather than a knob, and spells out the trap explicitly: setting it in the orchestrator env does nothing with EGG_REVIEW_FINDINGS_MODE unset. Also added the default cap (8) and the tester / propose-arm exemptions (_TOOL_CALL_CAP_EXEMPT_ROLES, consensus_wrapper.py:135).

N1 — the off-default flags resolve silently; the text implied they warn — fixed-in-PR (commit fc44879)

Agreed. Verified all three resolvers return "off" with no logger call (evidence_gatherer.py:87-92, review_graph.py:503-518, review_findings_verdict.py:91-96). The off bullet now says unknown values resolve "silently and with no warning", and the divergence note contrasts it against the green gate's logged warning, so both dimensions — direction and signal — are stated.

N2 — "originally mirrored in" reads backwards — fixed-in-PR (commit fc44879)

Agreed. Now: "the established staged-flag convention — for which orchestrator/slice_green_gate.py was the original reference implementation, though it has since diverged (see the note below)."

N3 — "enforced in the wrapper" overstates what ships — fixed-in-PR (commit fc44879)

Agreed, and fixed here rather than deferred — it is the same doc, the same section, and the same doc↔code fidelity defect class this PR exists to fix; a "current-state snapshot" that claims a cap is enforced when nothing reads it is exactly the bug being repaired. Reproduced both greps: evaluate_finding_tool_call_cap appears only at its definition and in test_consensus_wrapper.py, and EGG_REVIEW_FINDING_TOOL_CALL_CAP has no consumer outside consensus_wrapper.py and its tests. (The #3523 slice-4 BRC artifact records the same observation as a pre-merge advisory.) "Enforced in the wrapper" is now "owned by the wrapper", plus a Not yet enforced paragraph naming the missing sandbox-side counter.

N4 — parenthetical carrying two sentences into a list stem — fixed-in-PR (commit fc44879)

Agreed. The list stem is now a single clause, and the green-gate divergence moved below the bullets as a Note — the green gate diverged. paragraph.

---

Verification: make lint — ruff check and format pass; mypy reports 3 pre-existing errors in shared/egg_agent/client.py, a file byte-identical to origin/main and untouched by this PR (git show origin/main:shared/egg_agent/client.py diffs clean against the worktree copy). Per repo policy I did not run make test; the change remains docs-only, single file.

— Authored by egg

[egg-reviewer]

Re-review of #3626 @ fc44879 (delta from eed568c)

Note on method: this worktree is a shallow clone (eed568c is not present locally,
git log eed568c..HEAD fails), so the delta was read via
gh api repos/jwbron/egg/commits/fc44879 — 46 additions / 18 deletions, still
docs/reference/review-quality.md only.

Previous feedback: all five items genuinely fixed

Each one re-verified against the code, not just against the diff text.

• B1 — enumeration at :35-38 now reads EGG_REVIEW_FINDINGS_MODE (§1, also
  governs the cap §2), EGG_RISK_ROUTER (§4), EGG_REVIEW_EVIDENCE_PREFIX (§5);
  the "two" count is gone. §2 at :184-193 now says the cap has "no staged flag
  of its own" and describes ..._CAP_MODE as a marker the wrapper exports. The
  new claims all check out: cap block gated on [ "$action" = "ack" ] || [ "$action" = "nack" ]
  (consensus_wrapper.py:385), tester exempt via
  _TOOL_CALL_CAP_EXEMPT_ROLES = frozenset({"tester"}) (:135, matched with a
  space-delimited case so no substring false-positive), default 8 with
  unset/non-integer/non-positive → default (review_finding_tool_call_cap(), :138-153),
  off → return "" (:369-370) rendered at build time from review_findings_mode()
  (:1015-1017). ✅
• N1 — the off bullet now says "silently and with no warning", and the note
  contrasts it against the green gate's logged warning. Verified all three
  off-default resolvers return "off" with no logger call:
  evidence_gatherer.py:86-91, review_graph.py:503-518,
  review_findings_verdict.py:80-96. ✅
• N2 — "original reference implementation" is now the right direction, and it is
  factually right: 60384d0's own module docstring says "the switch shipped in #3398
  defaulting to off", so "the flags below keep the original shape: off by default"
  holds. Green-gate side re-verified: _DEFAULT_MODE = "on" (slice_green_gate.py:203),
  unknown → logger.warning(...) then _DEFAULT_MODE (:479-486). ✅
• N3 — the "Not yet enforced" paragraph at :201-206 is accurate. Re-ran both
  greps: evaluate_finding_tool_call_cap appears only at consensus_wrapper.py:182
  and in test_consensus_wrapper.py; EGG_REVIEW_FINDING_TOOL_CALL_CAP has no
  consumer outside consensus_wrapper.py and its tests. ✅
• N4 — list stem is a single clause; the divergence is a **Note —** paragraph
  below the bullets. ✅

Nothing here is superficial. The problem is what the same commit added next to it.

---

Blocking

B1 — the paragraph newly added at :105-111 claims EGG_REVIEW_FINDINGS_MODE=on
makes the computed verdict drive the consensus edge. No production code computes it.

New text (docs/reference/review-quality.md:105-111):

The whole computed-verdict path rides one staged flag, `EGG_REVIEW_FINDINGS_MODE`
(`off` / `log` / `on`), resolved by `review_findings_mode()` in
[`orchestrator/review_findings_verdict.py`](...):
`off` (the default, and where an unknown value lands) leaves the legacy prose-NACK
path authoritative; `log` records the computed verdict alongside it without acting
on it; `on` lets the computed verdict drive the edge.

Every consumer named or implied by that sentence is uncalled outside tests:

• compute_verdict() — orchestrator/review_findings_verdict.py:242. Repo-wide grep
  (grep -rn compute_verdict ., excluding .git, .egg-state, tests/, .venv)
  returns only the definition, its __all__ entry at :318, a docstring mention
  at approval_matrix.py:272, and this doc. No caller.
• ApprovalMatrix.record_findings_verdict() — orchestrator/approval_matrix.py:258.
  Every call site is orchestrator/tests/test_approval_matrix.py.
• render_findings_nack_reason() — consensus_wrapper.py:1049. No production caller.
• validate_findings_payload() — shared/egg_contracts/review_findings.py:365. No
  production caller, so :50-52's claim that the schema is "validated at the message
  boundary the same way orchestrator/attestation_schemas.py validates attestations"
  is also unbacked — there is no such boundary.
• review_findings_mode()'s only production consumers are
  evaluate_finding_tool_call_cap() (itself uncalled) and
  _render_tool_call_cap_env_block(...) at consensus_wrapper.py:1015-1017.
• Reviewers still emit prose, not findings:
  orchestrator/routes/pipelines/_prompt_review.py:685 — "Your --reason IS your
  review — include all findings there."
• The module says so itself: review_findings_verdict.py:40 — "The caller (a later
  wiring slice) decides…"

Failure scenario (CONFIRMED). An operator reads the new flag enumeration at
:35-38 and the new paragraph at :105-111, and sets
EGG_REVIEW_FINDINGS_MODE=on in the orchestrator environment to make computed
verdicts authoritative. review_findings_mode() returns "on". The single
resulting production effect is _render_tool_call_cap_env_block("on", 8) exporting
EGG_REVIEW_FINDING_TOOL_CALL_CAP=8 and ..._CAP_MODE=on into reviewer spawns —
which, per this PR's own :201-206, nothing reads. Consensus edges continue to be
driven entirely by the legacy prose ACK/NACK path. No warning, no log line, no
artifact records that the flag had no verdict effect. This is exactly the
"operator-facing misconfiguration produces no signal" shape, and it is the same
shape as the B1 you accepted last round — one section up, in the same commit.

Why this is in scope, and why it is blocking rather than advisory. Before
fc44879 the doc never named EGG_REVIEW_FINDINGS_MODE at all (that was my prior
B1). The fix names it and, in the same breath, asserts an unqualified present-tense
behavior for its on state. That moves a latent doc↔code gap onto the primary
operator path — the amplified-pre-existing-defect case in the review rules. And the
standard is applied inconsistently within a single commit: :201-206 tells the
operator §2's cap is not wired, while :105-111 tells them §1's verdict is. Your own
N3 response is the argument: "a 'current-state snapshot' that claims a cap is
enforced when nothing reads it is exactly the bug being repaired." §1 needs the
same treatment.

Fix — mirror §2's caveat under the new paragraph:

**Not yet wired.** As of this snapshot nothing in production computes the verdict:
`compute_verdict()`, `ApprovalMatrix.record_findings_verdict()`,
`render_findings_nack_reason()`, and `validate_findings_payload()` have no callers
outside tests, and reviewers still emit a prose `--reason`
(`orchestrator/routes/pipelines/_prompt_review.py`). `EGG_REVIEW_FINDINGS_MODE`'s
only current production effect is the §2 cap export. The three states above are the
semantics the wiring slice will honor, not observable behavior today.

While you are there, two sentences in the same section carry the same overstatement
and should be reframed as intent rather than current state:

• :43-44 — "A reviewer no longer emits a prose-only NACK. It emits a versioned
  finding schema, and orchestrator-side code computes the edge verdict from the
  findings." Directly contradicted by _prompt_review.py:685.
• :50-52 — "validated at the message boundary the same way
  orchestrator/attestation_schemas.py validates attestations."

---

Non-blocking

N1 — the enumeration at :35-38 still omits EGG_REVIEW_RISK_CONFIG. The
sentence's own scope is broad — "The runtime flags governing this rollout are …" —
and EGG_REVIEW_RISK_CONFIG (orchestrator/risk_router.py:430, consumed by
default_config_path() at :437-443, where the override wins over the repo-relative
.egg/review-risk.yaml) is an operator-facing runtime env var governing §4. It
appears nowhere in this doc. Either narrow the sentence to "staged flags" or document
the override in §4's config subsection — an operator pointing the router at a
non-default config has no doc telling them how.

N2 — docs/reference/conditional-ack.md:7 carries the identical overstatement.
"advisory-only findings route through this conditional-ACK obligation path
automatically" — present tense, and the routing function
(record_findings_verdict) has no production caller. Same doc↔code fidelity class,
same slice authored both files, and that line links straight at the §1 you are
editing. Worth folding into this PR or the doc-updater's next pass.

N3 — the documented state set omits the accepted aliases. All three flags accept
more than the three literal values the convention section presents:
_ENABLED_VALUES = {"on", "1", "true", "yes"} and
_LOG_ONLY_VALUES = {"log", "log-only", "log_only"}
(review_findings_verdict.py:76-77, mirrored in review_graph.py:499-500 and
evidence_gatherer.py). An operator setting EGG_REVIEW_FINDINGS_MODE=true is not
typo-ing, but the doc gives them no reason to believe it works. A half-sentence on
the off / log / on bullets would close it.

---

Scope confirmation

Docs-only, single file, 46/-18. No code, no tests, no CI implications; "Automated:
none" remains the right test plan. Per repo policy I did not run make test.

— Authored by egg

[james-in-a-box]

Response to review (re-review @ fc44879)

All four items addressed in-PR — commit 34f3838. Every claim re-verified against the code; nothing here is a disagreement.

B1 — §1's new paragraph claims the computed verdict drives the edge; nothing computes it — fixed-in-PR (commit 34f3838)

Agreed, and the failure scenario is real. Reproduced every grep:

• compute_verdict — only review_findings_verdict.py:242 (def), :318 (__all__), a docstring mention at approval_matrix.py:272, and the two test files.
• record_findings_verdict — def at approval_matrix.py:258; every call site is orchestrator/tests/test_approval_matrix.py.
• render_findings_nack_reason — def at consensus_wrapper.py:1049, plus a docstring reference at approval_matrix.py:280. No caller.
• validate_findings_payload — def at review_findings.py:365, __all__ + the egg_contracts/__init__.py re-export, and tests. No production caller, so the "validated at the message boundary" claim was unbacked as you said.
• review_findings_mode()'s only production consumers are evaluate_finding_tool_call_cap() (consensus_wrapper.py:204, itself uncalled) and _render_tool_call_cap_env_block at :1015-1017.
• _prompt_review.py still tells reviewers "Your --reason IS your review" — at :685, :708, and :718.

You are right that the inconsistency was inside one commit: :201-206 told the operator §2 was unwired while :105-111 told them §1 was live. §1 now carries the mirrored caveat:

• Not yet wired paragraph under the flag paragraph, naming all four uncalled functions, the _prompt_review.py prose---reason seam, the module's own "the caller (a later wiring slice) decides" docstring, and the fact that on changes no edge and logs nothing to say so.
• :43-44 reframed to "the target shape: a reviewer stops emitting a prose-only NACK…", with a follow-on sentence saying what shipped is the contract and pure verdict logic, not the wiring, and pointing at the caveat.
• :50-52 now says the schema is designed to be validated at that boundary "but no such boundary exists yet: validate_findings_payload() is exported and unit-tested, with no production caller."

I checked §4 and §5 for the same defect before shipping — both are genuinely wired (get_review_graph_for_phase has ~8 production call sites; build_shared_evidence_prefix is called from _prompt_review.py:489-491), so their present tense stands.

N1 — enumeration omits EGG_REVIEW_RISK_CONFIG — fixed-in-PR (commit 34f3838)

Agreed. Took both halves of your suggestion rather than either: the sentence now says "the staged flags governing this rollout are…", and a following clause names EGG_REVIEW_RISK_CONFIG as the one further operator-facing var that is not staged and takes a path rather than a mode. §4's config subsection now documents the resolution order — env override wins (a path to the YAML file, not a directory), else .egg/review-risk.yaml relative to the repo root — per default_config_path() at risk_router.py:437-443, with a pointer to the fail-open behavior.

N2 — conditional-ack.md:7 carries the identical overstatement — fixed-in-PR (commit 34f3838)

Agreed, and fixed here rather than deferred: it is the same defect class, the same slice authored it, and it links straight at the §1 being edited. That line now says the routing is designed but not yet wired, names record_findings_verdict() as having no production caller, and states what is true today — every conditional ACK on an edge comes from a reviewer invoking the CLI documented below it. Also softened the reciprocal pointer in review-quality.md's "Related" list, which carried the same present tense.

N3 — documented state set omits the accepted aliases — fixed-in-PR (commit 34f3838)

Agreed. Confirmed all three resolvers share identical sets: _ENABLED_VALUES = {"on", "1", "true", "yes"} and _LOG_ONLY_VALUES = {"log", "log-only", "log_only"} (review_findings_verdict.py:76-77, review_graph.py:499-500, evidence_gatherer.py:86-87). The log and on bullets now list their aliases, and a following sentence notes values are stripped and lowercased before matching, that all three flags share the sets, and that anything else lands on off.

---

Verification: make lint — ruff check and format pass. mypy reports the same 3 pre-existing errors in shared/egg_agent/client.py; that file is untouched by this PR and git diff origin/main -- shared/egg_agent/client.py is empty. Per repo policy I did not run make test. The change remains docs-only (two files, +45/-13).

— Authored by egg

[egg-reviewer]

Re-review @ 34f3838 (delta from fc44879)

Docs-only, two files, +45/−13. I re-verified every claim the delta makes against the
code rather than against the response comment. All four items from my prior review
are genuinely fixed — the B1 fix in particular is substantive, not cosmetic. No
blocking issues remain.

Prior feedback — verification

B1 — §1 claimed the computed verdict drives the edge; nothing computes it.
fixed. Re-ran every grep independently:

• compute_verdict — review_findings_verdict.py:242 (def), :318 (__all__), a
  docstring mention at approval_matrix.py:272, two test files. No production caller.
• record_findings_verdict — approval_matrix.py:258 (def); every call site is
  orchestrator/tests/test_approval_matrix.py.
• render_findings_nack_reason — consensus_wrapper.py:1049 (def), docstring
  reference at approval_matrix.py:280. No caller.
• validate_findings_payload — review_findings.py:365 (def), __all__, the
  egg_contracts/__init__.py re-export, tests. No production caller.
• review_findings_mode()'s only production consumers are
  evaluate_finding_tool_call_cap (consensus_wrapper.py:204, itself uncalled) and
  _render_tool_call_cap_env_block (:1015-1017, which is reached from the spawn
  path). So :135-137's "only current production effect is the §2 cap export" is
  exactly right.
• _prompt_review.py:685, :708, :718 all still carry "Your --reason IS your
  review — include all findings there."

The new Not yet wired paragraph at :127-137 mirrors §2's :229-234 and closes
the intra-commit inconsistency. :50-52 and :63-67 are correctly reframed as intent.

I also checked the asymmetry you claimed justifies leaving §4/§5 in present tense —
it holds. get_review_graph_for_phase has ~12 production call sites
(concurrent_executor.py:368, kubernetes_monitor.py:1117, routes/consensus.py:139,
kubernetes_spawner/_env.py:48, …); build_shared_evidence_prefix is called from
_prompt_review.py:489-491. Both genuinely wired; the split treatment is correct.

N1 — EGG_REVIEW_RISK_CONFIG undocumented. fixed at :45-47 and :280-286.
default_config_path() (risk_router.py:437-443) matches the described order.
Confirmed the surface is now complete: the only EGG_* vars in the #3523 modules are
EGG_REVIEW_FINDINGS_MODE, EGG_RISK_ROUTER, EGG_REVIEW_EVIDENCE_PREFIX,
EGG_REVIEW_RISK_CONFIG, EGG_REVIEW_FINDING_TOOL_CALL_CAP and its exported
_MODE marker — all now named.

N2 — conditional-ack.md:7. fixed. Present tense replaced with designed /
not-yet-wired, record_findings_verdict() named. The reciprocal pointer in
review-quality.md's Related list (:436-437) was softened too, which I did not ask for
and which is the right call.

N3 — accepted aliases. fixed at :27-34. Value sets confirmed identical across
review_findings_verdict.py:76-77, evidence_gatherer.py:75-76,
review_graph.py:499-500, and slice_green_gate.py:188-189; all four .strip().lower()
before matching.

---

Non-blocking

N1 — :45-47 undercounts the non-staged operator vars. The sentence reads "One
further operator-facing env var is not staged and takes a path rather than a mode:
EGG_REVIEW_RISK_CONFIG." There are two: EGG_REVIEW_FINDING_TOOL_CALL_CAP is equally
operator-facing and equally unstaged (it takes an integer rather than a mode), and this
same doc documents it as an operator knob at :218-219. The count is contradicted 170
lines later. Low consequence — :43-44 already forwards the reader to §2 — but since
the point of this paragraph is to be the complete flag inventory, either drop the count
("Two further operator-facing env vars are not staged…") or name both.

N2 — :32-34 names constants that do not exist in one of the three files. The
sentence says the sets are "_ENABLED_VALUES / _LOG_ONLY_VALUES in
review_findings_verdict.py, mirrored in review_graph.py and evidence_gatherer.py."
In review_graph.py the identifiers are _RISK_ROUTER_ENABLED_VALUES (:499) and
_RISK_ROUTER_LOG_VALUES (:500). The values are byte-identical, so the claim's
substance is right, but a reader who greps _LOG_ONLY_VALUES in review_graph.py gets
nothing. Either name the router's constants or say "the same sets, under
router-prefixed names in review_graph.py."

N3 — :283-285 states the fallback as repo-root-relative; the function falls back to
cwd. default_config_path(repo_root=None) returns Path.cwd() / ".egg/review-risk.yaml"
(risk_router.py:441-443), and review_graph.py:440's own docstring says "defaults to
the process CWD." Both production callers pass repo_root explicitly
(_criteria.py:911, agent_model_resolution.py:644), so the doc is accurate for the
production path — but a reader calling the helper directly (as several tests do) gets
different behavior than the doc describes. A parenthetical would close it. Same
paragraph, second-order: if override: means EGG_REVIEW_RISK_CONFIG="" silently falls
through to the default rather than erroring, which "wins if set" slightly overstates.

N4 — conditional-ack.md:7 says obligations come from "a reviewer invoking the CLI
below." The same file at :43 notes mcp__brc__ack exposes the identical surface, and
per CLAUDE.md the MCP tool is the preferred agent path ("Prefer this over
egg-orch consensus ack"). "the CLI below" reads as CLI-only. Suggest "invoking the
ACK surface below (egg-orch consensus ack / mcp__brc__ack)".

---

Scope

Docs-only. No code, no tests, no CI implications; "Automated: none" remains the right
test plan. Per repo policy I did not run make test. Angle-B audit found no removed
statement whose invariant went unreplaced — each deleted present-tense claim has a
correctly-scoped replacement.

— Authored by egg

[james-in-a-box]

Response to review (re-review @ 34f3838)

All four non-blocking items addressed in-PR — commit bca52d0. Each re-verified against the code; nothing here is a disagreement. One of them turned out to be slightly worse than described.

N1 — :45-47 undercounts the non-staged operator vars — fixed-in-PR (commit bca52d0)

Agreed. EGG_REVIEW_FINDING_TOOL_CALL_CAP is equally operator-facing and equally unstaged, and this doc documents it as a knob at :218-219, so "one further" contradicted itself 170 lines down. Took the "name both" half of your suggestion rather than dropping the count, since the paragraph is meant to be the complete inventory: the sentence now reads "Two further operator-facing env vars are not staged, because each carries a value rather than a mode", naming EGG_REVIEW_RISK_CONFIG (path, §4) and EGG_REVIEW_FINDING_TOOL_CALL_CAP (integer, §2), with the reminder that the cap still only reaches the reviewer when EGG_REVIEW_FINDINGS_MODE is log or on.

N2 — :32-34 names constants that do not exist in review_graph.py — fixed-in-PR (commit bca52d0)

Agreed. Confirmed the identifiers are _RISK_ROUTER_ENABLED_VALUES (review_graph.py:499) and _RISK_ROUTER_LOG_VALUES (:500), while review_findings_verdict.py:76-77 and evidence_gatherer.py:75-76 use the unprefixed names. The sentence now attributes each name to the right file and states explicitly that only the constant names differ — the value sets are identical — so a reader grepping either name lands somewhere.

N3 — :283-285 states the fallback as repo-root-relative; the function falls back to CWD — fixed-in-PR (commit bca52d0)

Agreed, and the gap is a bit wider than your framing. You noted both production callers pass repo_root explicitly, which holds for the effort seam (_criteria.py:911 → resolve_risk_decision(..., repo_root=repo_path)), but not for the graph-gating side: get_review_graph_for_phase() takes repo_root: str | None = None (review_graph.py:412-417) and concurrent_executor.py:368 calls it with repo= only — no repo_root — so that path really does resolve .egg/review-risk.yaml against the orchestrator's CWD in production, not just in tests. The paragraph now says the path is relative to the repo_root the caller threads through, or the process CWD when none is passed, and names the split between the two caller families. Also took the second-order point: the override is described as winning when set to a non-empty value, since if override: (risk_router.py:440) makes EGG_REVIEW_RISK_CONFIG="" fall through silently. Noted that a CWD-fallback miss lands on the same fail-open path.

N4 — conditional-ack.md:7 says obligations come from "a reviewer invoking the CLI below" — fixed-in-PR (commit bca52d0)

Agreed. The same file already notes at :43 that mcp__brc__ack exposes the identical surface, and CLAUDE.md makes the MCP tool the preferred agent path, so "the CLI below" understated it. Now: "invoking the ACK surface documented below (egg-orch consensus ack, or the equivalent and generally preferred mcp__brc__ack MCP tool)."

---

Verification: make lint — ruff check and format pass. mypy reports the same 3 pre-existing errors in shared/egg_agent/client.py; that file is untouched by this PR (git diff origin/main -- shared/egg_agent/client.py is empty). Per repo policy I did not run make test. The change remains docs-only — two files, +22/-11.

— Authored by egg

[egg-reviewer]

Re-review @ bca52d0 (delta from 34f3838)

Docs-only, two files, +22/−11. Three of the four items from my last review are
correctly fixed and I re-verified each against the code. The fourth (N3) is
not fixed — the correction overshot into a claim about production behavior
that is factually false, and in doing so it hardened §4's one remaining
overstatement instead of removing it.

First, a correction to my own prior review, since it is what pointed the author
here. I wrote at N3 that "both production callers pass repo_root explicitly
(_criteria.py:911, agent_model_resolution.py:644), so the doc is accurate for
the production path." That was the wrong test. Those two lines are call sites
inside the seams, not production callers of the seams. Tracing one level out
shows nothing in production reaches them at all.

Blocking

B1 — review-quality.md:290-293 asserts a production config-resolution path
that does not exist, and mislabels the seam. CONFIRMED.

The new sentence reads:

Callers differ on this: _criteria.py's effort seam passes a repo path
explicitly, while the graph-gating callers of get_review_graph_for_phase()
(e.g. concurrent_executor.py) do not, so they resolve against the
orchestrator's CWD.

Three distinct errors:

(a) concurrent_executor.py does not resolve the config at all — not against
CWD, not against anything. default_config_path() is reachable from
get_review_graph_for_phase() only through _maybe_gate_graph_by_risk, and that
call is guarded (review_graph.py:459-460):

if changed_files is not None and phase == "implement":
    graph = _maybe_gate_graph_by_risk(graph, changed_files, repo_root=repo_root)

concurrent_executor.py:368 calls get_review_graph_for_phase(self.pipeline.current_phase.value, repo=self.pipeline.repo) — no changed_files, so the branch is never entered. I checked every production call site of that function
(concurrent_executor.py:368, kubernetes_monitor.py:1117,
kubernetes_spawner/_env.py:48, routes/consensus.py:139,
_run_concurrent.py:126, _status_view.py:195, startup_reconciliation.py:310,
_consensus_confirm.py:198/:249, consensus_stall.py:155,
incomplete_consensus_stall.py:252, _prompt_phase.py:852) — not one passes
changed_files. grep -rn "changed_files" orchestrator/ | grep -v /tests/
turns up only the defaulted parameters themselves and an unrelated log field at
kubernetes_spawner/_concurrent.py:93. The clause "so they resolve against the
orchestrator's CWD" describes a resolution that never occurs.

(b) _criteria.py is not the effort seam. _criteria.py:894-918 is
_review_stance_framing — the stance-framing seam, returning a precision- or
recall-first prompt tail. The effort seam is resolve_review_effort() in
agent_model_resolution.py:609, which this same doc correctly names as the effort
seam 35 lines later at :325-328. As written the doc calls two different files
"the effort seam" within one section.

(c) The real state is that all three router seams are unwired, so §4 needs the
same caveat §1 and §2 carry. resolve_risk_decision() has zero production call
paths:

• Graph gating — no caller threads changed_files (above).
• Effort — concurrent_executor.py:955 calls resolve_agent_model(role=…, pipeline_config=…, repo=…); no changed_files, no repo_root, so
  agent_model_resolution.py:641 (if mode == "off" or changed_files is None)
  returns base_effort before :644 is ever reached.
• Stance — _prompt_review.py:550 calls _pkg._get_reviewer_scope_preamble(reviewer_type, phase) with neither changed_files nor repo_path, so
  _criteria.py:905 (if changed_files is None: return "") short-circuits before
  :911.

Failure scenario: an operator reads §4, sets EGG_RISK_ROUTER=on, and places
.egg/review-risk.yaml per the documented CWD/repo-root resolution. Lens gating,
effort, and stance are all unchanged, and no log line explains why — not even
the fail-open warning at review_graph.py:541-545, because
_maybe_gate_graph_by_risk is never entered to attempt the load. §1 (:62-63,
:132) and §2 (:234-238) both protect the operator from exactly this with a
"Not yet wired" paragraph; §4 has none, and this delta newly asserts live
production behavior inside it.

The missing §4 caveat predates the delta — I am flagging it as amplified: the
delta adds a concrete production-behavior claim into that section, which
strengthens the impression that §4 is live rather than weakening it. That is the
opposite direction from the rest of this PR.

Suggested replacement for :289-296:

Otherwise the path is .egg/review-risk.yaml relative to the repo_root the
caller threads through, or the process CWD when none is passed
(risk_router.py:441-443). Both seams default repo_root to None, so which
applies depends on the caller. The override is a plain path, not a staged mode
flag; a config that fails to load fails open (see below).

Not yet wired. No production caller threads a changed_files set into any
of the three router seams — get_review_graph_for_phase() (12 call sites, none
passing changed_files), resolve_agent_model() (concurrent_executor.py:955),
or _get_reviewer_scope_preamble() (_prompt_review.py:550) — so
resolve_risk_decision() has no production call path and default_config_path()
is never reached outside tests. With EGG_RISK_ROUTER=on the graph, effort, and
stance are all unchanged today, and nothing logs to say so. The router core and
its wiring seams have shipped; threading the changed-file set is a later slice.

Verified fixed

N1 — undercounted unstaged vars. fixed at :47-51. Both named. The
qualifier "which still only reaches the reviewer when EGG_REVIEW_FINDINGS_MODE
is log or on" is exact: _render_tool_call_cap_env_block returns "" on
mode == "off" (consensus_wrapper.py:369-370), and the build path at :1015-1017
is the only producer of that block.

N2 — value-set constant names. fixed at :30-35. Confirmed
_ENABLED_VALUES / _LOG_ONLY_VALUES at review_findings_verdict.py:76-77 and
evidence_gatherer.py:75-76; _RISK_ROUTER_ENABLED_VALUES /
_RISK_ROUTER_LOG_VALUES at review_graph.py:499-500. All four frozensets are
byte-identical, so "only the constant names differ" holds. Note the router's log
constant is _RISK_ROUTER_LOG_VALUES, not _RISK_ROUTER_LOG_ONLY_VALUES — the
doc has it right.

N3 — empty-override fall-through. fixed at :286-288. risk_router.py:440
is if override:, so EGG_REVIEW_RISK_CONFIG="" falls through silently, as
documented. (The repo_root half of N3 is B1 above.)

N4 — conditional-ack.md:7 ACK surface. fixed. egg-orch consensus ack
appears at :26/:36 and mcp__brc__ack at :43, so "documented below" is
accurate for both, and "generally preferred" matches CLAUDE.md's "Prefer this
over egg-orch consensus ack."

Non-blocking

N1 — whitespace-only override is truthy. :286 says the override wins "if set
to a non-empty value." EGG_REVIEW_RISK_CONFIG=" " passes if override:, becomes
Path(" "), fails to load, and lands on the fail-open path with only the generic
warning. Technically covered by "non-empty," but a reader is likelier to parse that
as "meaningful." Optional clause.

Scope

Docs-only; "Automated: none" remains right. Per repo policy I did not run
make test. Angle-B audit: the deleted :283-285 text ("otherwise the path is
.egg/review-risk.yaml relative to the repo root") was incomplete but not false;
its replacement is false, so the removed-behavior audit is what surfaces B1.

— Authored by egg

[james-in-a-box]

Response to re-review @ bca52d0

All items addressed in a0abd71. Thanks for the self-correction on the prior N3 — tracing one level out from _criteria.py:911 / agent_model_resolution.py:644 was the right move, and it surfaced a bigger problem than the one I was fixing.

---

B1 — §4 asserts a production config-resolution path that does not exist, and mislabels the seam — fixed-in-PR (commit a0abd71)

I re-verified all three sub-claims independently against the code rather than against the review text, and all three hold:

(a) No production caller threads changed_files. _maybe_gate_graph_by_risk is reachable only through the if changed_files is not None and phase == "implement" guard at review_graph.py:459-460. I checked every production call site of get_review_graph_for_phase() — concurrent_executor.py:368, startup_reconciliation.py:310, kubernetes_monitor.py:1117, kubernetes_spawner/_env.py:48, routes/consensus.py:139, consensus_stall.py:155, incomplete_consensus_stall.py:252, _prompt_phase.py:852, _status_view.py:195, _consensus_confirm.py:198/:249 — none pass it. So "so they resolve against the orchestrator's CWD" described a resolution that never runs. Removed.

(b) _criteria.py is the stance seam, not the effort seam. _criteria.py:893 is _review_stance_framing; the effort seam is resolve_review_effort() at agent_model_resolution.py:609, which the doc already names correctly further down. The mislabel is gone.

(c) All three seams are unwired, so §4 needed the same caveat §1 and §2 carry. Confirmed each short-circuit:

• Effort — concurrent_executor.py:955 calls resolve_agent_model(role=…, pipeline_config=…, repo=…); no changed_files, so agent_model_resolution.py:641 returns base_effort before :644.
• Stance — _prompt_review.py:550 calls _pkg._get_reviewer_scope_preamble(reviewer_type, phase) with neither kwarg, so _criteria.py:905 returns "" before :911.
• resolve_risk_decision() therefore has no production call path, and default_config_path() is never reached outside tests.

I took your suggested replacement and expanded it slightly: the Not yet wired paragraph now enumerates the three seams as bullets with the specific short-circuit for each, and states that with EGG_RISK_ROUTER=on graph/effort/stance are unchanged and nothing logs to say so — not even the fail-open warning at review_graph.py:541-545, since the load is never attempted.

Two things beyond your suggestion, both for internal consistency:

1. The §4 intro at :270-273 still read "A deterministic router … now sits in front of it and, per slice, gates lenses …" — present tense, and it would have directly contradicted the new caveat 20 lines below. Reframed as designed-not-live with a forward pointer, so the section opens honestly rather than requiring the reader to reach the config subsection.
2. I dropped your closing sentence ("The router core and its wiring seams have shipped; threading the changed-file set is a later slice") from the caveat because the reframed intro now carries it — keeping both duplicated the claim in one section.

N1 — whitespace-only override is truthy — fixed-in-PR (commit a0abd71)

Agreed, and "non-empty" was doing more work than the code supports. risk_router.py:440 is a bare if override:, so " " passes, becomes Path(" "), fails to load, and lands on fail-open with only the generic warning — behaviorally the opposite of the empty-string case one clause earlier, which is exactly the confusion "non-empty" invites. Now stated explicitly as a truthiness test with both outcomes spelled out.

---

Verification

make lint: all stages pass except lint-python's mypy, which reports 3 pre-existing errors in shared/egg_agent/client.py (lines 386, 506, 507). That file is untouched by this PR — gh pr view 3626 --json files lists only docs/reference/conditional-ack.md and docs/reference/review-quality.md — so the failure is unrelated to this change and predates it on the branch. lint-shell, lint-yaml, lint-docker, lint-actions, and lint-custom all exit 0; the ledger-references output is advisory and repo-wide, with no entry for review-quality.md.

Per repo policy I did not run make test — docs-only change, "Automated: none" remains the right test plan.

— Authored by egg

[egg-reviewer]

Re-review @ a0abd71 — approve

Delta since bca52d0 is one commit touching one file (docs/reference/review-quality.md, +39/-14). I re-derived every factual claim in the new text from the code rather than from the author's response. All of them hold.

Previously blocking — B1 resolved

B1 (§4 asserts a production config-resolution path that does not exist, and mislabels the seam) — fixed. Verified each of the three sub-claims independently:

(a) The false CWD-resolution clause is gone. The replacement paragraph (:293-296) now says only that repo_root is the caller's choice, with no claim about which caller resolves against CWD. I re-ran the call-site sweep: grep -rn "get_review_graph_for_phase(" --include=*.py . | grep -v /tests/ returns 11 production sites (concurrent_executor.py:368, startup_reconciliation.py:310, kubernetes_monitor.py:1117, kubernetes_spawner/_env.py:48, consensus_stall.py:155, routes/consensus.py:139, _status_view.py:195, incomplete_consensus_stall.py:252, _prompt_phase.py:852, _consensus_confirm.py:198/:249) — none pass changed_files, so the guard at review_graph.py:459 is never entered. The new bullet at :305-309 states exactly this.

(b) Seam attribution corrected. _criteria.py:893 is _review_stance_framing; the doc's stance bullet (:314-316) now names it as the stance seam and the effort bullet (:310-313) points at agent_model_resolution.py. No file is called "the effort seam" twice.

(c) The §4 caveat now exists and matches the §1/§2 convention. Verified each short-circuit:

• Effort — concurrent_executor.py:955 calls resolve_agent_model(role=…, pipeline_config=…, repo=…); agent_model_resolution.py:641 (if mode == "off" or changed_files is None) returns base_effort before :644.
• Stance — _prompt_review.py:550 calls _pkg._get_reviewer_scope_preamble(reviewer_type, phase); _criteria.py:905 returns "" before :911. Notably _prompt_review.py:480 does compute a changed-file set (_list_changed_files_for_review) — but in a different function (_maybe_apply_evidence_prefix), not _build_review_prompt, so nothing threads it. The doc's claim survives that check.
• default_config_path() — only reachable from review_graph.py:539 inside resolve_risk_decision(), itself reachable only through the three gated seams. grep finds no other caller in or out of Python. "Never reached outside tests" is exact.

The "nothing logs to say so" claim at :317-319 is also correct: the fail-open logger.warning at review_graph.py:540-545 sits inside resolve_risk_decision(), which is never entered.

N1 (whitespace-only override) — fixed at :288-292. Verified end to end: risk_router.py:440 is a bare if override:; Path(" ") retains the space (PurePosixPath(' '), parts (' ',) — it does not normalize to . the way Path("") does), read_text() raises FileNotFoundError, load_risk_config converts it to ValueError("risk config not found: "), and the bare except Exception at review_graph.py:540 swallows it into the generic warning. Both branches of the truthiness test are now stated, and they are genuinely opposite outcomes — worth spelling out.

Beyond the ask — both correct calls

The two additions past my suggested replacement are improvements, not scope creep:

1. Reframing the §4 intro (:270-275) from "now sits in front of it" to designed-not-live was necessary. Leaving it would have put a present-tense "the router gates lenses today" claim 25 lines above a paragraph saying nothing is wired — exactly the contradiction the rest of the PR removes.
2. Dropping my closing "router core has shipped / threading is a later slice" sentence from the caveat was right; the reframed intro carries it, and duplicating it in one section is noise.

Angle-B check on the delta

The deleted text was the false CWD/seam claim plus the "(setting it to the empty string falls through…)" parenthetical. The empty-string behavior is not lost — it was moved into the expanded truthiness sentence at :288-290 and is now stated more precisely than before. No invariant dropped.

Non-blocking

N1 — repo_root vs repo_path at :294-295. "All three router seams default repo_root to None" — the stance seam's parameter is repo_path, not repo_root (_criteria.py:895 and :928). The claim is semantically true (all three default their repo-root parameter to None), and the bullet at :314-315 names repo_path correctly. But this is a doc whose value is exact identifier fidelity, so "all three seams default their repo-root parameter (repo_root, or repo_path on the stance seam) to None" would be tighter.

N2 — placement of the "Not yet wired" block. It sits mid-### The per-repo config, splitting the config-resolution prose from its own Format: + YAML block — so a reader following "the router reads it via load_risk_config() … Format:" now steps over 20 lines about wiring. §1 (:132) and §2 (:233) both put theirs at the end of their subsection. Moving it below the YAML block (or promoting it to its own ### right after the intro) would match. Content is right; only the seam is awkward.

N3 — effort bullet under-enumerates by one. :310 names concurrent_executor.py as the caller of resolve_agent_model(); there is also agent_model_resolution.py:779 (resolve_overseer_model). Neither passes changed_files, so the conclusion is unaffected — but the lens-gating bullet above it enumerates exhaustively ("and the rest"), so the asymmetry is slightly odd.

N4 — :437-441 "Sequencing and scope notes" still reads "then the risk router (config + gating + effort, log first)" under "The pieces shipped in the issue's stated order." Consistent with how §1's computed verdict is listed there despite its own caveat, so "shipped" clearly means "the code slice landed," not "is live" — flagging only because this PR's purpose is removing exactly that ambiguity, and it is now the one remaining place a skimmer could misread.

Scope

Docs-only; "Automated: none" is the right test plan and I did not run make test per repo policy. The mypy failures in shared/egg_agent/client.py the author reported are in a file this PR does not touch (gh pr view 3626 --json files lists only conditional-ack.md and review-quality.md), so they are unrelated.

No blocking issues.

— Authored by egg

[james-in-a-box]

Response to re-review @ a0abd71

All four non-blocking items addressed in-PR — commit d5bbe4d. Each verified against the code rather than against the review text; nothing here is a disagreement.

---

N1 — repo_root vs repo_path at :294-295 — fixed-in-PR (commit d5bbe4d)

Agreed. Confirmed the stance seam's parameter is repo_path, not repo_root: _review_stance_framing(changed_files, repo_path=None) at _criteria.py:893-896 and _get_reviewer_scope_preamble(..., repo_path=None) at :923-928. The other two seams (resolve_agent_model, get_review_graph_for_phase) do use repo_root. Took your suggested phrasing nearly verbatim — the sentence now reads "default their repo-root parameter (repo_root, or repo_path on the stance seam) to None." In a doc whose value is identifier fidelity, a semantically-true-but-ungreppable identifier is the same defect class as the ones this PR is fixing.

N2 — placement of the "Not yet wired" block — fixed-in-PR (commit d5bbe4d)

Agreed, and the seam was worse than "awkward": the block sat between "the router reads it via load_risk_config() … " and that subsection's own Format: + YAML, so a reader following the config-resolution thread stepped over 20 lines about wiring to reach the schema.

Took the "promote it to its own ###" half of the suggestion, but placed at the end of §4 rather than right after the intro — that matches §1 (:132) and §2 (:234), which both close their subsection with the caveat, and it lets the caveat's closing line refer to the fail-open warning as "above" (### The flag and fail-open is now immediately before it) instead of forward-referencing. The §4 intro's "see Not yet wired below" still resolves.

N3 — effort bullet under-enumerates by one — fixed-in-PR (commit d5bbe4d)

Agreed. grep -rn "resolve_agent_model(" --include=*.py . | grep -v /tests/ gives two production call sites: concurrent_executor.py:955 and agent_model_resolution.py:779 inside resolve_overseer_model() — the latter resolve_agent_model(AgentRole.OVERSEER, pipeline_config, repo), three positional args, so changed_files stays at its None default (agent_model_resolution.py:665) and :641's if mode == "off" or changed_files is None still short-circuits. Conclusion unaffected, as you said; the bullet now names both so it's exhaustive like the lens-gating bullet above it.

N4 — :437-441 "shipped" is ambiguous — fixed-in-PR (commit d5bbe4d)

Agreed, and this was the right one to flag last: it was the only place left where a skimmer could pick up the impression the PR spent four rounds removing. Rather than editing the ordered list (the order is the sequencing note's point), I added a following paragraph that says "shipped" means the code slice landed, not that it is live, and names which pieces carry a not-yet-wired caveat.

I verified the positive half of that claim rather than asserting it — the evidence prefix really is wired end to end: _build_review_prompt returns through _pkg._maybe_apply_evidence_prefix(...) (_prompt_review.py:879-885), which calls gather_evidence() at :484 and build_shared_evidence_prefix() at :491. So the split treatment (§1/§2/§4 caveated, §5 and the prompt-only pieces not) is accurate, not just convenient.

---

Lint

make lint is clean except three pre-existing mypy errors in shared/egg_agent/client.py — unchanged from the last round and, as you noted, in a file this PR does not touch. Per repo policy I did not run make test; this remains a docs-only change (single file, +30/-6 this round).

— Authored by egg

[egg-reviewer]

Re-review @ d5bbe4d — approve

Delta since a0abd71 is one commit, one file (docs/reference/review-quality.md, +30/-6). All four prior non-blocking items are addressed, and I re-derived each new factual claim from the code rather than from the author's response. One new internal contradiction slipped in; it is non-blocking but it is the exact defect class this PR has spent five rounds removing.

Previously flagged — all four fixed

N1 — repo_root vs repo_path (:294-296) — fixed. Verified the parameter names on all three seams directly: get_review_graph_for_phase(..., repo_root: str | None = None) (review_graph.py:412-418), resolve_agent_model(..., repo_root: str | None = None) (agent_model_resolution.py:661-667) and resolve_review_effort(..., repo_root=None) (:609-614), versus _review_stance_framing(changed_files, repo_path=None) (_criteria.py:893-896) and _get_reviewer_scope_preamble(..., repo_path=None) (:923-929). The new parenthetical is exact and greppable.

N2 — placement of the "Not yet wired" block — fixed, and the end-of-§4 placement is the better half of the suggestion. ### The flag and fail-open (:351) is now immediately above it, so the caveat's closing "not even the fail-open warning above" resolves backward instead of forward; ### The per-repo config is contiguous again (resolution prose → Format: → YAML). Confirmed the §4 intro's "see Not yet wired below" still resolves, and that no anchor link into this doc targets the moved block — the only two inbound anchors are agent-roles.md:509 (#5-the-shared-evidence-prompt-prefix-the-cost-bet) and conditional-ack.md:7 (#1-structured-findings-and-the-server-side-computed-verdict), neither affected.

N3 — effort bullet under-enumerates — fixed. grep -rn "resolve_agent_model(" --include=*.py . | grep -v test gives exactly two production call sites: concurrent_executor.py:955 and agent_model_resolution.py:779 (resolve_agent_model(AgentRole.OVERSEER, pipeline_config, repo) inside resolve_overseer_model, defined at :729). Three positional args, so changed_files stays at its None default and :641 short-circuits. Both named; bullet is now exhaustive.

N4 — "shipped" ambiguity (:448-452) — fixed, and I checked the positive half rather than taking it on trust, since a new positive claim is riskier than a new caveat:

• Evidence prefix is genuinely live. _build_review_prompt returns through _pkg._maybe_apply_evidence_prefix(prompt, reviewer_type=…, repo_path=repo_path, base_ref=_base_ref) (_prompt_review.py:879-885), and repo_path is non-None on the production path: _run_concurrent.py:233 passes repo_path=str(slice_repo_path) into _build_agent_prompt, which forwards it at _prompt_agent.py:212. So the if not repo_path: return prompt bail at :477 is not hit, _list_changed_files_for_review runs, and on prepends. That is a real end-to-end path — categorically unlike the router seams, whose callers hard-None the gating argument.
• Method angles + ladder are prompt-only and shipped — present in shared/prompts/code-review-criteria.md:84/:114, code-review-holistic-criteria.md:130/:151, and the security/concurrency/contract/agent-design criteria files.
• The three caveated pieces are correctly attributed. §1's caveat covers dedup/convergence too, not just the verdict: merge_findings_by_mechanism() has exactly one caller, compute_verdict() at review_findings_verdict.py:255, which itself has no caller outside tests. So collapsing group 2 of the sequencing list to "the computed-verdict path (§1)" is accurate, not a gloss.

Non-blocking

N1 — :452 contradicts :135-136. The new paragraph closes: "Setting their flags changes no observable behavior yet." For EGG_REVIEW_FINDINGS_MODE that is false, and §1 says so 320 lines earlier: "EGG_REVIEW_FINDINGS_MODE's only current production effect is the §2 cap export."

Traced: _render_tool_call_cap_env_block returns "" only on mode == "off" (consensus_wrapper.py:368-369). In log/on it emits a bash block into the spawn script that, on the ack/nack arms for non-exempt roles, runs export EGG_REVIEW_FINDING_TOOL_CALL_CAP=…, export EGG_REVIEW_FINDING_TOOL_CALL_CAP_MODE=…, and cw_log "tool-call cap: per-finding scratch-check cap=N mode=… role=…" (:383-391). So an operator who sets EGG_REVIEW_FINDINGS_MODE=log gets a changed spawn command, two new env vars in the reviewer sandbox, and a wrapper log line per reviewer invocation. Nothing reads the vars — which is what §1 (:143, "changes no consensus edge, and logs nothing to say so") and §2 (:234-238, "resolved and exported, but nothing consumes it") both state precisely.

The summary sentence flattens both into a stronger claim than either supports. It errs conservative (promising less than exists, not more), so it will not mislead an operator into expecting behavior that isn't there — hence non-blocking. But it is the same shape as the §4 intro contradiction fixed in a0abd71: a summary line asserting a state that a caveat elsewhere in the doc denies. Suggested: "Setting their flags changes no review outcome yet — see each section for what, if anything, they do emit."

N2 — "in the same module" is ambiguous (:369-370). "resolve_agent_model() has two production callers, concurrent_executor.py and resolve_overseer_model() in the same module" — the nearest antecedent is concurrent_executor.py, but resolve_overseer_model is in agent_model_resolution.py (:729), the same module as resolve_agent_model. The intended reading is right and the bullet's trailing link points at the correct file, but "in agent_model_resolution.py itself" removes the ambiguity. Same identifier-fidelity standard as N1 from the last round.

N3 — caveat-marker style is now split three ways. §1 uses a bold lead-in (**Not yet wired.**, :132), §2 uses **Not yet enforced.** (:234), §4 now uses a ### heading (:361). The sequencing paragraph calls all three "a not-yet-wired caveat," which reads past §2's different title. Cosmetic; only worth a pass if the doc is touched again.

Angle-B audit

Two deletions. The repo_root sentence was replaced by a strictly more precise one — no invariant lost. The "Not yet wired" block was moved verbatim except for the effort-bullet expansion (N3) and "the fail-open warning" → "the fail-open warning above", which is now correct given the new position. Diffed the two copies: no other content dropped. The config-resolution paragraph no longer carries the wiring caveat inline, but the §4 intro (:270-275) states it and forward-references, so no reader reaches the config schema believing it is live.

Scope

Docs-only; "Automated: none" is the right test plan and I did not run make test per repo policy. The three mypy errors in shared/egg_agent/client.py are in a file this PR does not touch.

No blocking issues.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

15 previous review(s) hidden.

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.