Default the slice green gate to on

[jwbron]

Why

The per-slice green gate (#3398) is the only thing in egg that executes the repo's configured checks against a slice tip independently of any agent's self-report. It shipped with EGG_SLICE_GREEN_GATE defaulting to off, and the variable is set nowhere in k8s/ or config/. Three weeks after the gate landed, its check-runner path had not executed once.

A gate nobody runs verifies nothing. The default is the rollout.

This came out of triaging #3602 (a contract task marked complete while its acceptance condition was unmet). In that incident the slice tip carried five failing tests that proved the task undone; the gate would have gone red on it and refused the slice PR. It never ran.

What

green_gate_mode() defaults to on instead of off:

Value	Mode
unset	on (was off)
off / 0 / false / no	off
log / log-only / log_only	log
on / 1 / true / yes	on
anything else	on + a warning log line (was off)

Weakening the gate now takes an explicit, correctly-spelled off or log. Previously every unrecognised value silently resolved to off, so an operator reaching for on and typing onn got a gate that did nothing.

Why on and not log

An earlier revision of this PR stopped at log, on the reasoning that on needed soak evidence first. That reasoning assumed a fleet. egg has one deployment, and on it the argument inverts:

• log's evidence is passive. A verdict is a structured log line — no metric, no audit event, no PR comment (now tracked in Green gate: verdicts are log lines only — no metric, audit event, or queryable record #3623). It informs an operator who goes looking, and this switch's own history is that nobody does. Under on a wrong verdict announces itself on the next slice close, to the operator who can act on it. That makes on the better instrument for measuring the false-red rate, not merely the stricter one.
• log would not have prevented Task completion is self-asserted; a complete row whose acceptance test fails is undetectable #3602. It would have logged a line and opened the PR anyway. The motivating incident is a shape only a blocking gate prevents.
• The latency cost is identical. Both modes spawn the runner and wait for the pod. The cost was already being paid to collect evidence nobody was reading.
• The blast radius is bounded and self-documenting. A red verdict records a slice failure and withholds the PR. The commits stay on the integration branch, the failure message names the branch to fix, and it quotes EGG_SLICE_GREEN_GATE=off inline as the bypass. Recovery is a slice restart or one env var, by the operator who is already watching.

log stays available and is the right posture for a fleet, where a false red stalls a pipeline whose owner is not the person watching the rollout.

What to expect from the first wave

The earliest reds are likelier to be the gate's own wiring than slice code, and each of these reds every slice close until fixed:

• a stale contract snapshot on the slice tip reddening contract-hygiene tests (Make pipeline contract single-writer on work to kill add/add slice-PR conflicts #3301);
• make test's changeset narrowing resolving its baseline against git merge-base in a fresh worktree, so it sees the cumulative slice diff.

A declared-but-missing prebuilt-deps snapshot is deliberately not in that list: the runner exits non-zero and the gate fails open, so it costs coverage rather than slice throughput.

Documented in the module docstring and the slice-dag.md env table rather than left to be rediscovered.

Cost

Deliberate and documented: the gate runs the checks and waits for the runner pod, so slice-close latency grows by the check duration, bounded by EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS (default 1800s) after which it fails open. The worst case is not a slow suite but a runner pod that never schedules (~32 min at the defaults). off remains available for deployments that cannot absorb that.

Sequencing

1. Green gate: distinguish infra-induced check failures from genuine reds #3518 (Green gate: distinguish infra-induced check failures from genuine reds #3417, infra-red vs genuine-red) — merged, and in this branch. Without it an infra fault inside a check would read as a definitive red.
2. This PR — the gate starts blocking.
3. Green gate: Job-level activeDeadlineSeconds charges pod scheduling to the check budget #3622 (Job-level activeDeadlineSeconds) — re-framed from an on prerequisite to the top follow-up. Its no-verdict fail-open can only under-block, never produce a false red, so it is a coverage gap rather than a correctness risk: "the gate is on" and "this close was gated" become different claims on a capacity-starved cluster.
4. Green gate: verdicts are log lines only — no metric, audit event, or queryable record #3623 (verdict observability) — makes the false-red rate and the Green gate: Job-level activeDeadlineSeconds charges pod scheduling to the check budget #3622 coverage gap countable instead of greppable.
5. Green gate Stage A: config-driven format autofix before slice PR open #3517 (Green gate Stage A: server-side format autofix before slice PR open #3409, autofix) — throughput/toil, not correctness; it widens the trust surface, so it follows.

Note on the test changes

• test_kill_switch_off_skips_everything asserted the gate short-circuits before any spawn side effect but relied on the implicit off default. It now sets off explicitly, which is what it meant all along.
• test_unset_switch_blocks_on_a_red_verdict is new and is the seam that makes the default load-bearing: every other blocking test sets the switch to on explicitly, so without it the resolver could regress to log with the whole suite still green.
• The unrecognised-value parametrization gains offf — the disable-side typo, which is the direction that matters now that the default blocks.

Testing

orchestrator/tests/test_slice_green_gate.py: 119 passed. Sibling staged-flag suites (test_review_findings_verdict, test_risk_router_wiring, test_run_implement_slice_closed): 95 passed. Repo-wide ruff check + ruff format --check clean.

[egg-reviewer]

No agent-mode design concerns.

This is a configuration-default change in orchestrator infrastructure (green_gate_mode() resolving EGG_SLICE_GREEN_GATE to off/log/on), with matching doc and test updates. It touches none of the agent-mode design surface: no prompt assembly or pre-fetching, no structured-output requirement for human-facing text, no post-processing pipeline, no direct Anthropic API calls or Agent-SDK bypass, and no hardcoded model identifiers.

— Authored by egg

[egg-reviewer]

Approve. This is a clean, well-reasoned default flip. I traced the full path and verified the core claims.

What I verified

• green_gate_mode() resolution is exhaustive and correct (orchestrator/slice_green_gate.py:259): unset/whitespace → log, enabled aliases → on, log aliases → log, explicit off/0/false/no → off, any other value → log + a logger.warning. No gap.
• log mode is functional end-to-end, not just structurally present. At slice_green_gate.py:831 a red verdict logs logger.error(... mode=mode), then if mode == "log": return None (:840) — so log runs the runner pod, emits the soak verdict, and does not block. Only on returns the blocking failure string. Blocking behaviour is unchanged, as stated.
• Safety direction improved. The pre-existing hazard was onn→off (an operator reaching for on silently gets a gate that does nothing). The new unrecognised→log+warning path is strictly safer and satisfies "operator misconfiguration must produce a signal" better than the old silent-off fallback.
• Tests preserve the production path. test_kill_switch_off_skips_everything (test_slice_green_gate.py:513) now sets off explicitly — necessary because gate_env deletes the var and the unset case now runs the gate. Every other TestRunSliceGreenGate case sets the switch via enabled_gate (:487, on), so none silently changed behaviour. New parametrized cases cover the realistic typo shapes (tru, onn).
• Deployment impact is the intended rollout. EGG_SLICE_GREEN_GATE is set nowhere in k8s/ or config/, so all deployments now resolve to log. The added slice-close latency (bounded by EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS, fail-open) is documented as deliberate cost.

Non-blocking

Stale cross-references to green_gate_mode() semantics. Three sibling staged-flag resolvers document themselves as resolving "EXACTLY like slice_green_gate.green_gate_mode()" with the parenthetical "(off default, unknown => off)":

• orchestrator/review_findings_verdict.py:33 and :80
• orchestrator/review_graph.py:478 and :500
• orchestrator/evidence_gatherer.py:65

After this PR that cross-reference is no longer accurate: green_gate_mode() now defaults to log and degrades unrecognised values to log+warning, whereas these three still (correctly, for their own intent) default to off and silently fall to off. The functions themselves are fine — only the "EXACTLY like green_gate_mode" wording is now misleading to a future reader who follows the reference. Worth a one-line reword in each (e.g. "resolved like the staged off/log/on pattern, but off-default") so the docs don't drift. Not blocking — no behaviour depends on these comments.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Both reviews approved. The one actionable item was the non-blocking suggestion from the review bot about stale cross-references. Fixed in-PR.

Stale cross-references to green_gate_mode() semantics — fixed-in-PR (commit 3a04103)

The three sibling staged-flag resolvers documented themselves as resolving "EXACTLY like slice_green_gate.green_gate_mode()" with the parenthetical "(off default, unknown => off)". After this PR green_gate_mode() defaults to log and degrades unknown values to log + a warning, so that cross-reference no longer held. Reworded each to describe the shared staged off/log/on pattern while explicitly noting these three deliberately keep an off-default (unlike green_gate_mode() now) — so a future reader who follows the reference isn't misled:

• orchestrator/review_findings_verdict.py (module docstring + review_findings_mode())
• orchestrator/review_graph.py (section comment + risk_router_mode())
• orchestrator/evidence_gatherer.py (section comment)

The functions' own behaviour is unchanged — this was a docs-only reword, as the reviewer noted.

---

make lint is clean for the changed files (ruff check + format pass). The mypy failures reported locally are pre-existing and confined to shared/egg_agent/client.py, which this PR does not touch.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta re-reviewed: 1f149a8a → 3a04103b (one commit, Reword stale green_gate_mode cross-references).

The delta is docs-only — three comment/docstring rewords in orchestrator/evidence_gatherer.py, orchestrator/review_findings_verdict.py, and orchestrator/review_graph.py, addressing the base review bot's non-blocking note that those files claimed to resolve their switches "EXACTLY like slice_green_gate.green_gate_mode()" after this PR changed that function's default. No executable line is touched: review_findings_mode() (review_findings_verdict.py:80) and risk_router_mode() (review_graph.py:499) both still read os.environ.get(..., "off") and fall through to return "off", matching what the new prose now says about them.

Nothing in the delta touches the agent-mode design surface:

• No prompt assembly or pre-fetching. evidence_gatherer.py's reworded block is the module's staged-flag section header; the shared-evidence prefix assembly it gates is unchanged.
• No structured output for human-facing text, no post-processing pipeline, no rigid-procedure prompts.
• No direct Anthropic API calls or Agent-SDK bypass, and no hardcoded model identifiers — the review_graph.py comment references agent_model_resolution.resolve_agent_model for tier/effort routing, but the delta adds no model literal of any kind (EGG200/EGG201 surface untouched).

The previous cycle's only actionable item came from the general review bot and is addressed here; my prior agent-mode review raised nothing to re-check.

— Authored by egg

[egg-reviewer]

Approve (re-review of 1f149a8..3a04103). The delta is documentation-only — I confirmed via gh api repos/jwbron/egg/compare/1f149a8...3a04103 that all 23 changed lines across the three files are inside comment blocks or docstrings. Zero executable lines changed, so the base commit's behaviour (which I verified last round) is untouched.

My prior non-blocking suggestion is addressed for the five sites I named. Four residual items below, all non-blocking, one of them introduced by this delta.

Verified

• The reworded claims are factually accurate. Each sibling resolver still reads os.environ.get(VAR, "off") and falls through to return "off" on unrecognised input, so "keeps an off-default, unknown => off" holds: review_findings_mode() (orchestrator/review_findings_verdict.py:80, env read at :89), risk_router_mode() (orchestrator/review_graph.py:499, :508), evidence_prefix_mode() (orchestrator/evidence_gatherer.py:80, :87).
• green_gate_mode() is unchanged from my last review (orchestrator/slice_green_gate.py:259): unset → _DEFAULT_MODE (log), enabled aliases → on, log aliases → log, explicit off/0/false/no → off, anything else → logger.warning + log. The "now defaults to log and degrades unknown to log + a warning" phrasing in all three reworded sites matches the implementation exactly.
• ruff check and ruff format --check are clean on all three changed files.
• EGG_SLICE_GREEN_GATE is still set nowhere in k8s/ or config/ — the new default is the deployed behaviour, which the PR's premise requires.

Non-blocking

1. The delta introduced an orphaned closing paren — orchestrator/review_findings_verdict.py:32-38

The reword opened a new parenthetical at :34 ((unknown => ...) and closed it at :36 (authoritative).), but left the original sentence's closing paren in place at :38:

deliberately does not, so a typo leaves the legacy path authoritative). ``log``
records the computed-vs-legacy verdict into the BRC
artifacts without acting, ``on`` uses the computed verdict). Everything in this

The paragraph has two ( and three ). The trailing verdict) is orphaned. Line :37 is also left ragged at 51 chars — the edit inserted text without re-wrapping the tail of the paragraph. Suggested:

deliberately does not, so a typo leaves the legacy path authoritative). ``log``
records the computed-vs-legacy verdict into the BRC artifacts without acting;
``on`` uses the computed verdict. Everything in this

Same ragged-wrap artifact, without the paren bug, at orchestrator/evidence_gatherer.py:68 (# not): an operator typo must degrade to, 40 chars).

2. The most load-bearing stale comment was missed — orchestrator/routes/pipelines/_run_implement.py:920

This is the call site of run_slice_green_gate and it still describes the rollout posture this PR is flipping:

# EGG_SLICE_GREEN_GATE is the operator switch
# (off during rollout / log / on).

A reader tracing why a slice close suddenly waits on a runner pod lands here first. "off during rollout" is now wrong. Neither commit touched it.

3. Two test-module docstrings carry the same stale claim the delta set out to fix

• orchestrator/tests/test_review_findings_verdict.py:19 — "review_findings_mode resolves a flag typo to off (never silently to on), exactly like slice_green_gate.green_gate_mode". This one is now inverted: green_gate_mode resolves a typo to log, which is precisely the distinction the delta added to the module docstring 60 lines away.
• orchestrator/tests/test_risk_router_wiring.py:7 — "EGG_RISK_ROUTER staged flag (off default / log / on, resolved exactly like slice_green_gate.green_gate_mode())". The off-default half no longer holds.

4. Optional — orchestrator/review_findings_verdict.py:66

The section header # --- staged-flag resolution (mirrors slice_green_gate.green_gate_mode) --- keeps the "mirrors" framing. Weaker than the "EXACTLY like" wording that was fixed, and the docstring 14 lines below now spells out the difference, so this is defensible as-is. Flagging only for completeness.

None of the above changes behaviour or blocks the rollout. CI is green on everything reported so far (Unit Tests and Integration Tests still pending at review time).

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Merged origin/main (through 026196c, which brought in #3518 / #3417) into egg/green-gate-default-log via merge commit c45d3cf. Two files conflicted; both were doc lines describing slice_green_gate.py, where this PR added "default log" and main added the #3417 infra-red fail-open clause to the same sentence.

File	Category	Resolution
docs/development/STRUCTURE.md	Additive	Kept both facts on the module-index line: (off/log/on, default log) from this PR + and on infra-signature-tagged reds inside check execution (#3417) from main
docs/architecture/slice-dag.md	Additive	Same merge in the slice-close prose: default log (observe, never block), fail-open on infra errors, including infra-signature-tagged reds inside check execution, #3417

orchestrator/slice_green_gate.py and orchestrator/tests/test_slice_green_gate.py auto-merged. I reviewed the auto-merge rather than trusting it, since both sides edited the module docstring and the same region of code: the merged file keeps this PR's green_gate_mode() (default log, explicit-disable-only) alongside main's _infra_fail_open_enabled() and _INFRA_LINE_SIGNATURES, and the docstring carries both the #3417 fail-open section and this PR's rollout/cost paragraphs with no stale off (default) text left behind.

Verification

• orchestrator/tests/test_slice_green_gate.py: 92 passed (this PR's 68 plus main's Green gate: distinguish infra-induced check failures from genuine reds #3417 additions — the interesting case, since main's new tests run under the changed default).
• Slice-close adjacent suites (test_run_implement_slice_closed, test_slice_run_loop_integration, test_create_slice_integration_branch, test_review_findings_verdict, test_risk_router_wiring): 185 passed.
• ruff check + ruff format --check: clean.
• mypy: 3 pre-existing errors in shared/egg_agent/client.py. Not from this merge — shared/egg_agent/ is byte-identical to origin/main, so they reproduce on main.
• make build not run: it builds Docker images and docker is unavailable in this sandbox. The merge changed only two markdown files, and both merged Python modules compile and pass their suites. CI will cover the image build.

Please review: the two doc sentences now assert both defaults-and-fail-open behaviours in one breath — worth a read to confirm the combined phrasing still reads correctly, particularly slice-dag.md, where the #3398 and #3417 issue references now both sit inside the same parenthetical.

— Authored by egg

[egg-reviewer]

Approve (re-review of 3a04103b..c45d3cfb). No agent-mode design concerns.

The delta since my last review is a single commit — merge c45d3cfb, bringing origin/main through 026196c7 (#3518 / #3417) into the PR branch. There is no new PR-authored work to review beyond the conflict resolution.

What I checked

• Both conflict resolutions are additive doc-line merges, no design surface. docs/development/STRUCTURE.md:113 and docs/architecture/slice-dag.md:553-559 each had this PR's "default log" clause and main's #3417 infra-red fail-open clause landing in the same sentence; both resolutions keep both facts. The slice-dag.md env-var table row for EGG_SLICE_GREEN_GATE correctly flips the documented default off → log and now spells out the latency cost. Documentation only — nothing that shapes agent context or behaviour.
• The auto-merged Python is clean of the anti-patterns in scope. orchestrator/slice_green_gate.py combines this PR's green_gate_mode() (:374, unset → _DEFAULT_MODE) with main's _infra_fail_open_enabled() with no interference. Grepping the merged module and the full PR diff vs the main-side parent for hardcoded model IDs (claude-<tier>-<date>), anthropic / httpx / requests.post API calls, and model= literals returns zero hits — so items 6, 7, and 8 of the criteria are REFUTED, not merely unobserved.
• The gate's execution model still delegates correctly. The green gate runs the repo's configured checks by spawning a sandboxed one-shot check-runner Job at the integration-branch tip — orchestrator-side code spawning a sandbox container, which is the pattern the guidelines ask for, not orchestrator-side work that should have been delegated. The merge did not change this.
• No prompt-assembly, structured-output, or post-processing surface is touched anywhere in the delta. The switch resolution is operator configuration read from the environment, not agent-facing context.

Note

The general review bot's four non-blocking items from the previous cycle (the orphaned closing paren at orchestrator/review_findings_verdict.py:32-38, the stale call-site comment at orchestrator/routes/pipelines/_run_implement.py:920, and the two stale test-module docstrings) are documentation-accuracy items outside this lens — I am deliberately not re-raising them here, and they remain non-blocking regardless. Flagging only so their absence from this review isn't read as them having been fixed by the merge; they were not touched by it.

— Authored by egg

[egg-reviewer]

Approve (re-review of 3a04103..c45d3cf). The delta is the conflict-resolution merge of origin/main (through 026196c, bringing #3518/#3417) into the PR branch. I reviewed the merge rather than trusting the summary: both hand-resolved doc lines and the slice_green_gate.py / test_slice_green_gate.py auto-merges are correct, and the semantic interaction between #3417's infra fail-open and this PR's new default is sound. No blocking issues.

The author asked for a specific read on the combined doc phrasing — answered under Answering the merge question below.

What I verified in the merge

The auto-merge of slice_green_gate.py is semantically correct, not just textually clean. The two sides edit the same function's neighbourhood, so I traced the merged control flow in run_slice_green_gate:

• mode = green_gate_mode() (:781) → off short-circuits (:782).
• #3417's infra classification runs before the mode branch: genuine_failed is computed at :968-970, the all-infra fail-open returns None at :988-995, and only then does :996 reach logger.error("Green gate red…", mode=mode) and :1002 if mode == "log": return None.

That ordering is what you want. In the new default log mode, an all-infra-red verdict is logged as the #3417 warning and never counted as a red in the soak signal — an infra fault does not pollute the very evidence this PR exists to collect. Blocking behaviour is unchanged: only on reaches the return at :1003.

No test was lost by the merge. I diffed the test symbol sets across all three sides. Relative to 026196c, exactly two names are gone (test_default_is_off, test_everything_else_is_off) and three are new (test_default_is_log, TestGreenGateMode.test_disabled_values, test_unrecognised_values_degrade_to_default). Every PR-side test survives; every #3417 test survives. Coverage strictly widened — the old parametrization mixed off/0/false with ""/banana/enabled in one all-resolve-to-off case; the split now pins the two classes separately and adds the realistic typo shapes (tru, onn) plus whitespace (" off ", " ").

Main's #3417 tests run under the changed default without silently changing meaning. Every TestRunSliceGreenGate case main added (test_all_infra_reds_fail_open, test_mixed_reds_block_on_genuine_only, test_infra_fail_open_switch_off_blocks_on_infra_reds, test_pre_3417_verdict_without_infra_field_blocks) takes the enabled_gate fixture, which sets the switch to on explicitly — none of them rode the implicit off default, so none of them changed behaviour under the merge. This was the real hazard of merging #3417 into a default flip and it is clean.

Tests pass on merged HEAD (targeted, not the full suite):

• orchestrator/tests/test_slice_green_gate.py — 92 passed
• orchestrator/tests/test_run_implement_slice_closed.py + test_slice_run_loop_integration.py — 59 passed

The PR's premise still holds post-merge. grep -rn EGG_SLICE_GREEN_GATE k8s/ config/ bin/ scripts/ returns nothing — the new default is the deployed behaviour. Main's docs/guides/deployment.md addition (bin/egg-init pointer) does not touch the green gate, so it introduced no competing default claim.

green_gate_mode() itself is byte-identical to what I approved last round (:374-398). Unset/whitespace → _DEFAULT_MODE, enabled aliases → on, log aliases → log, explicit off/0/false/no → off, anything else → logger.warning + log.

The two green_gate_mode cross-references main added inside this module are still accurate. slice_green_gate.py:153 ("Any other value degrades to the default, matching green_gate_mode's typo posture") and :405 ("Mirrors green_gate_mode's posture: an operator typo resolves to the documented default behavior") were written against the old resolver. Both survive the flip — the claim is "degrades to the documented default", which holds before and after. This is the same stale-cross-reference class 3a04103 set out to fix, so I checked it specifically; no reword needed. (One caveat under item 5 below.)

Answering the merge question

the two doc sentences now assert both defaults-and-fail-open behaviours in one breath — worth a read to confirm the combined phrasing still reads correctly, particularly slice-dag.md

Parens balance and every clause is factually true. slice-dag.md:552-558 opens ( at "gate (", nests (observe, never block), and closes at #3417). #3417 correctly scopes to the fail-open clause it trails, and #3398 to the gate itself.

The residual problem is not the parens, it is the adjacency:

blocks PR-open on a red verdict; staged rollout via
`EGG_SLICE_GREEN_GATE`, default `log` (observe, never block),

"blocks PR-open on a red verdict" and "never block" sit one clause apart and read as a contradiction on first pass. It resolves if you know log is a mode name, but this is the module-index line — the entry point for a reader who does not yet know that. STRUCTURE.md:113 has the same shape (blocks PR-open on red; … (off/log/on, default log)), milder because it's a single index line. Suggested for slice-dag.md:

       gates PR-open on a red verdict at the integration-branch tip;
       staged rollout via `EGG_SLICE_GREEN_GATE` — default `log`
       (runs the checks, logs the verdict, never blocks), `on` blocks;
       fail-open on infra errors, including infra-signature-tagged
       reds inside check execution, #3417) — calls

Also cosmetic: :557 is left ragged at 40 chars — the conflict resolution inserted text without re-wrapping the paragraph tail.

Non-blocking

Items 1–4 are carried forward unchanged from my 3a04103 review. The only new commit is the conflict-resolution merge, so none were addressed — expected, not a criticism. Restating so they are not lost, plus one new item from the merge.

1. Orphaned closing paren — orchestrator/review_findings_verdict.py:32-38

Still present. The 3a04103 reword opened a parenthetical at :34 and closed it at :36, but left the original sentence's closer at :38. Two (, three ):

deliberately does not, so a typo leaves the legacy path authoritative). ``log``
records the computed-vs-legacy verdict into the BRC
artifacts without acting, ``on`` uses the computed verdict). Everything in this

:37 is also ragged at 51 chars. Same ragged-wrap artifact without the paren bug at orchestrator/evidence_gatherer.py:68 (# not): an operator typo must degrade to, 40 chars).

2. The most load-bearing stale comment is still missed — orchestrator/routes/pipelines/_run_implement.py:919-920

# EGG_SLICE_GREEN_GATE is the operator switch
# (off during rollout / log / on).

This is the call site of run_slice_green_gate (:927). An operator asking "why does slice close suddenly wait on a runner pod" lands here first, and it still describes the posture this PR is flipping. Neither the original commit nor the merge touched it. Of everything in this list, this is the one I would actually fix before merge.

3. Two test-module docstrings carry the stale claim 3a04103 set out to fix

• orchestrator/tests/test_review_findings_verdict.py:18-19 — "review_findings_mode resolves a flag typo to off (never silently to on), exactly like slice_green_gate.green_gate_mode". Now inverted: green_gate_mode resolves a typo to log. This is precisely the distinction 3a04103 spelled out in the module docstring 60 lines away, so the two files now disagree with each other.
• orchestrator/tests/test_risk_router_wiring.py:6-7 — "(off default / log / on, resolved exactly like slice_green_gate.green_gate_mode())". The off-default half no longer holds.

4. Optional — orchestrator/review_findings_verdict.py:66

# --- staged-flag resolution (mirrors slice_green_gate.green_gate_mode) --- keeps the "mirrors" framing. Weaker than the "EXACTLY like" wording that was fixed, and the docstring 14 lines below spells out the difference. Flagging for completeness only.

5. New from the merge — _infra_fail_open_enabled() has no warning on an unrecognised value

orchestrator/slice_green_gate.py:401-410:

raw = os.environ.get(GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, "on").strip().lower()
return raw not in _INFRA_FAIL_OPEN_DISABLED_VALUES

An operator who sets EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN=offf reaching for strict every-red-blocks silently gets on (lenient) with no log line. green_gate_mode() now emits logger.warning on exactly this input class; this sibling does not, which makes the :153 comment's "matching green_gate_mode's typo posture" true on the resolution but incomplete on the signalling.

Why I am raising code this PR does not modify: before this PR the gate never ran, so _infra_fail_open_enabled() was never called in any deployment. This PR makes it execute on every slice close. The consequence is nil in log mode (nothing blocks either way), which is why this is advisory rather than blocking — but it should be fixed before the on flip, where the typo direction is strict→lenient. A three-line logger.warning mirroring :392-397 closes it.

6. Operational note for the soak — worst case is not "check duration"

The docstring says slice-close latency "grows by the check duration (bounded by EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS, default 1800s)". Accurate, and the bound is stated. Worth being explicit with whoever watches the soak: the worst case is not a slow suite but an unschedulable runner pod — _wait_for_runner_pod waits timeout + _POD_SCHEDULING_GRACE_SECONDS before failing open, so a capacity-starved cluster costs ~30 min of dead time per slice close, in every deployment, starting the moment this merges. Nothing to change in the code; the sequencing plan already has off as the escape hatch. Just make sure someone is watching the first wave rather than discovering it from slice throughput.

On sequencing

The PR body lists #3518 (#3417) as "effectively a prerequisite for on". That prerequisite is now satisfied in-branch by this merge, not merely pending — _infra_fail_open_enabled and _INFRA_LINE_SIGNATURES are on the branch and their tests pass under the new default. The soak this PR starts will therefore already carry infra-vs-genuine classification, which is the right ordering and makes the verdicts collected genuinely usable as on-flip evidence.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Both reviews approved. All six non-blocking items from the c45d3cf re-review are fixed in-PR, plus the merge question on the combined doc phrasing. Commit 910f6ed.

1. Orphaned closing paren — orchestrator/review_findings_verdict.py:32-38 — fixed-in-PR (commit 910f6ed)

You were right that the earlier reword opened a parenthetical and closed it without removing the original sentence's closer. Took the suggested fix (; instead of , ``on`` ... )) and re-wrapped the paragraph tail, which also clears the ragged 51-char line at :37. The paragraph is now 3 ( / 3 ). Same treatment for the ragged tail at orchestrator/evidence_gatherer.py:68.

2. Stale call-site comment — orchestrator/routes/pipelines/_run_implement.py:919-920 — fixed-in-PR (commit 910f6ed)

Agreed this was the one that actually mattered — it's the first place an operator lands when asking why slice close suddenly waits on a runner pod, and "off during rollout" was exactly backwards. Now reads (off / log / on), defaulting to log: the checks run and the verdict is logged loudly on every slice close, but only "on" blocks PR-open.

3. Two stale test-module docstrings — fixed-in-PR (commit 910f6ed)

Both "exactly like slice_green_gate.green_gate_mode" claims dropped. test_review_findings_verdict.py:18 now states the shared off/log/on shape with an off-default, spelling out that green_gate_mode no longer has one — so it no longer contradicts the module docstring 60 lines away. Same for the off-default half at test_risk_router_wiring.py:6.

4. review_findings_verdict.py:66 section header — fixed-in-PR (commit 910f6ed)

You flagged it as defensible as-is, but it's a one-line change and leaving a weaker version of the wording you just had fixed invites the same drift back. Now # --- staged-flag resolution (shared off/log/on pattern, off-default) ---, which carries the distinguishing fact rather than pointing at a sibling.

5. _infra_fail_open_enabled() has no warning on an unrecognised value — fixed-in-PR (commit 910f6ed)

Fixed rather than deferred, and your reasoning for raising code this PR doesn't modify is what convinced me: this PR is what makes that resolver execute in any deployment for the first time. Added _INFRA_FAIL_OPEN_ENABLED_VALUES and the explicit three-way resolution, so offf now returns True with a logger.warning carrying env_var + value, mirroring :392-397. The "" / whitespace case still resolves to True with no warning, so behaviour is unchanged for every input an operator plausibly means.

Tests split to match the green_gate_mode shape you noted last round: test_enabled_values (incl. whitespace), test_disabled_values, test_unrecognised_values_degrade_to_default_with_a_warning (asserts the warning fires exactly once with the right kwargs, parametrized on the realistic offf / fals typos), and test_recognised_values_do_not_warn.

6. Operational note for the soak — worst case is not "check duration" — fixed-in-PR (commit 910f6ed)

Documented rather than left as a verbal handoff, since "somebody should be told" tends not to survive. Both the module docstring and the EGG_SLICE_GREEN_GATE env-table row now say the worst case is an unschedulable runner pod — timeout plus the 120s scheduling grace, ~32 min at the defaults, per slice close on a capacity-starved cluster — and point at off as the escape hatch.

Merge question: the combined doc phrasing — fixed-in-PR (commit 910f6ed)

Your read was right — "blocks PR-open on a red verdict" and "never block" sitting one clause apart only resolves if you already know log is a mode name, which is the opposite of what a module-index line is for. Applied your suggested restructuring to slice-dag.md:552: the gate now "spawns ... to execute the repo's configured checks at the integration-branch tip", and blocking is attributed to the mode that does it (default log (runs the checks, logs the verdict, never blocks), on blocks PR-open on a red verdict). Re-wrapped the paragraph, clearing the ragged :557. STRUCTURE.md:113 got the same disambiguation inline.

Also updated the EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN env-table row for item 5, since it documented "any other value resolves to on" without the new signal.

---

Verification — orchestrator/tests/test_slice_green_gate.py + test_review_findings_verdict.py + test_risk_router_wiring.py: 194 passed. Slice-close adjacent (test_run_implement_slice_closed, test_slice_run_loop_integration): 59 passed. ruff check + ruff format --check clean repo-wide. The 3 make lint mypy errors are pre-existing in shared/egg_agent/client.py, which is byte-identical to origin/main on this branch.

— Authored by egg

[egg-reviewer]

Approve (re-review of c45d3cf..910f6ed). No agent-mode design concerns.

Delta re-reviewed: a single commit, 910f6ed "Address review: green-gate doc accuracy + infra-fail-open typo warning" (9 files, +85/-29). The local clone in this sandbox is depth-1, so I pulled the patch via gh api repos/jwbron/egg/commits/910f6ed... rather than git log -p.

The delta is docs, comments, one operator-log line, and tests:

• docs/architecture/slice-dag.md, docs/development/STRUCTURE.md — reword the module-index and env-table rows so log mode's "runs the checks, never blocks" no longer sits one clause from "blocks PR-open"; add the unschedulable-runner-pod worst case.
• review_findings_verdict.py, evidence_gatherer.py, _run_implement.py, and two test-module docstrings — comment/docstring accuracy after the default flip. Every changed line in evidence_gatherer.py is a # comment (verified by filtering the patch for non-comment +/- lines: empty).
• slice_green_gate.py — _infra_fail_open_enabled() gains an explicit three-way resolution plus logger.warning on an unrecognised value, with _INFRA_FAIL_OPEN_ENABLED_VALUES added.
• test_slice_green_gate.py — four resolver tests covering enabled/disabled/typo-warns/recognised-doesn't-warn.

Against the anti-pattern list: nothing here touches prompt assembly, agent context, or output handling. No pre-fetched context is added to any prompt; no structured output is introduced for a human-facing surface; no post-processing script parses agent output; the new logger.warning is operator telemetry, not an agent instruction. I grepped the four changed Python modules for claude-*-<date> literals, anthropic, httpx, and requests. — no hits, so no hardcoded model ID (EGG201) and no direct-API-call (EGG200) surface enters via this delta.

The one design-shaped fact in the delta is the _infra_fail_open_enabled() change, and it moves in the right direction: it replaces a silent lenient fallback with an explicit resolution that logs. That is a sandbox/config-enforced constraint carrying an operator signal, not a prompt-level one — squarely on the right side of the "prompt-level security" guideline.

On the previous round. All six non-blocking items from the c45d3cf general review were addressed in-PR, including the two I would have expected to be deferred (item 5, the infra-fail-open warning in code this PR does not otherwise modify, and item 6, documenting the soak's worst case rather than leaving it as a verbal handoff). My own prior agent-design reviews at 1f149a8, 3a04103, and c45d3cf each found no concerns; nothing in this delta changes that.

— Authored by egg

[egg-reviewer]

Request changes (re-review of c45d3cf..910f6ed). The delta is a single commit, 910f6ed (9 files, +85/−29): docs, comments, one added operator log line, and four tests. Every one of the six non-blocking items from my c45d3cf review is genuinely fixed — I verified each against the current file rather than the commit message, details below. 194 passed on the three touched test files, ruff check + ruff format --check clean on all five touched Python files, and the PR's premise still holds (grep -rn EGG_SLICE_GREEN_GATE k8s/ config/ bin/ scripts/ → nothing, so the new default is the deployed behaviour).

One blocking item, and it is new-in-this-delta as much as pre-existing: the operator guidance this commit adds about the gate's timeout clock describes a mechanism Kubernetes does not implement, and the constant it rests on does not do what the module claims.

Blocking

The deadline clock is misdescribed, and _POD_SCHEDULING_GRACE_SECONDS does not protect the check budget

orchestrator/slice_green_gate.py:221-229 (pre-existing) and the new paragraph at :91-97 / the new env-table sentence at docs/architecture/slice-dag.md:1062.

The comment at :226-229 asserts:

# The pod's own ``activeDeadlineSeconds`` (which the
# kubelet counts from pod *start*, i.e. after scheduling) still caps the
# actual check duration, so a genuinely hung check is killed by the pod
# deadline rather than lingering for the full grace-padded wait.

That describes PodSpec.activeDeadlineSeconds. The code does not set that field. :574 puts the deadline at manifest["spec"]["activeDeadlineSeconds"] — a sibling of ttlSecondsAfterFinished / backoffLimit / template, i.e. the Job spec — and :631 passes it to V1JobSpec(active_deadline_seconds=...), not to the pod template. The two fields have different clocks:

• JobSpec.activeDeadlineSeconds — "Specifies the duration in seconds relative to the startTime that the job may be continuously active before the system tries to terminate it" — the Job's .status.startTime, set when the controller starts the Job, before any pod is bound.
• PodSpec.activeDeadlineSeconds — "Optional duration in seconds the pod may be active on the node relative to StartTime" — the field the comment describes, and the one the code never sets.

So scheduling and image-pull latency do count against the deadline, which is precisely what :221-229 says the grace exists to prevent ("on a cold node a long image pull would otherwise eat into the check budget and trip a spurious fail-open timeout even when the checks would have passed"). The grace widens the orchestrator's wait (:946, timeout + _POD_SCHEDULING_GRACE_SECONDS); it does nothing to the in-pod budget, which the Job deadline still cuts by the scheduling delay.

Failure scenario — the capacity-starved cluster the new :91-97 paragraph names as the worst case. Defaults: timeout=1800, Job activeDeadlineSeconds=1860. Job created at t=0; pod Pending 300s for capacity; checks begin t≈300s and are killed by DeadlineExceeded at t=1860s — 1560s of the intended 1800s. restartPolicy: Never + backoffLimit: 0 mean no retry. The terminated pod never prints EGG_GREEN_GATE_VERDICT:, so parse_verdict returns None and :968 logs "Green gate skipped: no parseable verdict from runner" → fail open. The slice close produces no verdict — the artifact this PR exists to create — and the only signal is a warning indistinguishable from a runner-harness crash.

Why this is in scope, on two counts.

1. Amplified pre-existing defect. Before this PR green_gate_mode() defaulted to off and the variable was set in no deployment, so this path never executed anywhere. This PR puts it on every slice close in every deployment — the defect moves from unreachable to primary-path.
2. The new text is itself incomplete in the same direction. :91-97 and slice-dag.md:1062 are new operator guidance about this exact clock, added so "whoever watches the rollout" knows the cost. They tell the operator an unschedulable pod costs ~32 min of dead time (accurate — I checked: 1800 + 120 = 1920s, and the Job going Failed at 1860s deletes the Pending pod, so _wait_for_runner_pod polls out the remaining 60s and returns None). They do not tell the operator that a partially delayed pod silently shrinks the check budget by the delay and can convert a would-be verdict into a skipped gate. On a capacity-starved cluster that is the more common outcome and the one that degrades the soak signal, which is the whole point of the default flip.

Minimum to unblock — text only, in the file this commit is already rewriting for accuracy:

• Correct :221-229 to name the Job's deadline, counted from Job startTime, and state that scheduling + pull time counts against it. Drop the "kubelet counts from pod start" parenthetical and the claim that the check duration is protected.
• Add to :91-97 and slice-dag.md:1062: a delayed pod reduces the in-pod check budget by the delay, so a capacity-starved cluster raises the rate of spurious no-verdict fail-opens as well as dead time.

Not asking for in this PR: actually moving activeDeadlineSeconds onto spec.template.spec, or deriving the in-pod budget from observed scheduling time. That is a behaviour change and deserves its own issue — but it should land before the on flip, since under on a spurious no-verdict is a gate that silently does not gate. Filing it is fine; shipping the wrong description of it is not.

Verified fixed from the c45d3cf round

Checked against the current files, not the commit message.

1. Orphaned paren, review_findings_verdict.py:32-43 — fixed. The parenthetical now opens at :34 ((unknown => off; …) and closes once at :36 (authoritative)); 1 ( / 1 ) in that sentence, and the , ``on`` … ) closer is gone in favour of ;. Paragraph re-wrapped, no ragged line remains.
2. _run_implement.py:919-922 — fixed. Now (off / log / on), defaulting to log: … only "on" blocks PR-open. (One residual, item 1 under non-blocking.)
3. Two stale test-module docstrings — fixed, and the replacement claims are true: review_findings_mode (review_findings_verdict.py:89, os.environ.get(…, "off"), unknown → off) and risk_router_mode (review_graph.py:508, same shape) both keep an off-default and neither warns, so "unlike green_gate_mode" is accurate in both files. evidence_prefix_mode (evidence_gatherer.py:87) likewise.
4. review_findings_verdict.py:66 header — fixed, now carries the distinguishing fact instead of pointing at a sibling.
5. _infra_fail_open_enabled() warning — fixed, and behaviour-preserving. Old: return raw not in _INFRA_FAIL_OPEN_DISABLED_VALUES. New (:425-435): enabled → True, disabled → False, else warn + True. Every input maps to the same boolean as before; only the log line is new. Coverage strictly widened — the retired test_everything_else_is_on values (on/1/true/""/banana) are all still asserted across test_enabled_values and test_unrecognised_values_degrade_to_default_with_a_warning, plus ON / " 1 " / yes / " " / offf / fals / disabled. gate_env (:67-75) delenvs the var so test_default_is_on is meaningful, and enabled_gate (:679) never sets it, so no TestRunSliceGreenGate case picks up an unexpected warning. Ran it: 42 passed on -k "InfraFailOpen or GreenGateMode".
6. Soak worst-case documented — done, but see blocking above: what is documented is arithmetically right and mechanically incomplete.

Merge question (combined doc phrasing) — resolved. slice-dag.md:551-558 now attributes blocking to the mode that does it; parens balance 2/2 (gate ( … (runs the … blocks) … #3417)), #3417 still scopes to the fail-open clause, and the paragraph is re-wrapped so the ragged :557 is gone. STRUCTURE.md:113 got the same disambiguation.

Non-blocking

1. _run_implement.py:908-910 still carries the pre-flip claim, six lines above the corrected one. # in a sandboxed one-shot runner, and refuse to open / # the slice PR while any check is red. then :921-922 # but only "on" blocks PR-open. This is the same adjacency problem you just fixed in slice-dag.md — an unconditional "refuse to open the PR" one clause from "only on blocks" — inside a single comment block. Reword :908-910 to "…and, in on mode, refuse to open the slice PR while any check is red."
2. evidence_gatherer.py:70 — the ragged line moved rather than went away. # the feature's core, and is imported by is 40 chars mid-paragraph; the old offender at :68 is now full at 76. Same artifact, one line down. Cosmetic.
3. _INFRA_FAIL_OPEN_ENABLED_VALUES (:165) is byte-identical to _ENABLED_VALUES (:138) in the same module. Defensible if you want the two switches free to diverge; worth a one-line comment saying so, otherwise a reader will "fix" the duplication.
4. The normalization assertion in test_unrecognised_values_degrade_to_default_with_a_warning doesn't bite. assert warn.call_args.kwargs["value"] == value passes only because every parametrized value is already lowercase and unpadded. The resolver logs value=raw (post-.strip().lower()), so adding " Offf " to the parametrize list would pin what is actually logged; today a change to log the raw env value would not fail this test.
5. The new warning omits a structured resolved-value field. green_gate_mode's warning carries mode=_DEFAULT_MODE (:407); the infra one puts the resolution in prose only ("falling back to the default (fail open on all-infra reds)"). A resolved=True/fail_open=True kwarg would make the two greppable the same way.
6. slice-dag.md:1065 says "on/1/true/yes (and unset) enable it". An explicitly-empty or whitespace-only value also enables it without a warning (:425, if not raw or …). Trivially incomplete.

Sequencing

Unchanged from last round and still right: #3417 is on the branch, so the verdicts this default starts collecting already carry infra-vs-genuine classification. The blocking item above is the one thing I'd want closed before the soak starts, because it determines whether a missing verdict on a busy cluster reads as "runner crashed" or "we under-budgeted the checks" — and the operator reading the docstring today would reach the wrong conclusion.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Commit 16e5e0f. Both reviews approved; all items from the 2306807 round are fixed in-PR. No deferrals.

---

1. test_pod_level_deadline_is_not_set_today does not enforce what its docstring claims — fixed-in-PR (commit 16e5e0f)

Confirmed by running it, and you were right on the mechanism: _submit_runner_job never reads pod_spec["activeDeadlineSeconds"], so seeding the dict leaves body.spec.template.spec.active_deadline_seconds is None and the assertion holds while the pod deadline goes nowhere. The submission seam constrains where the field must arrive but not that it arrived from the dict — those are different properties, and last round's docstring claimed the second.

Took your reflection test, adapted: _assert_manifest_value_reaches walks the manifest and the submitted body in lockstep through each model's attribute_map, recursing into nested dicts and lists (containers, env, volumes, volumeMounts) so it covers the whole tree rather than the top two levels. test_every_manifest_field_reaches_the_body runs it over the real manifest. _submit now takes an optional pre-built manifest, as you noted it would need to.

Two things I added beyond the suggestion:

• test_a_dict_only_pod_deadline_is_caught — the negative control. Seeds spec.template.spec.activeDeadlineSeconds exactly as Green gate: Job-level activeDeadlineSeconds charges pod scheduling to the check budget #3622 option 1 landed dict-only and asserts the walk raises activeDeadlineSeconds dropped. Without it the reflection test is only asserted to pass, never asserted to bite, which is the failure mode this whole thread is about.
• is not None is kept for the dropped-field check but scalars compare with ==, per your note in item 2 — that is what makes item 2's mutations detectable.

test_pod_level_deadline_is_not_set_today survives with a corrected docstring: it records where the deadline is today and explicitly says it does not enforce #3622's arrival, pointing at the test that does.

2. Three fields ignore the manifest and the assertions can't detect it — fixed-in-PR (commit 16e5e0f)

Took the read-from-the-manifest option rather than dropping them from the dict. They are security-posture fields; a reader auditing the runner pod's privileges looks at the manifest builder, and deleting them there to leave only a submitter constant makes that audit wrong in the more dangerous direction.

_submit_runner_job now reads pod_spec["automountServiceAccountToken"] and container["securityContext"]["allowPrivilegeEscalation"] / ["capabilities"]["drop"]. Behaviour is unchanged — the manifest carries False / False / ["ALL"] today, which is what the constants said. The docstring records why they were consolidated so the next reader doesn't re-inline them as "obviously constant".

test_security_fields_follow_the_manifest_not_a_constant parametrizes the three, loosening each in the manifest (True / True / []) and asserting the submitted pod follows. Mutation-checked rather than assumed: I restored the three hardcodes, re-ran, and got exactly 3 failed / 6 passed — so these three assertions now discriminate where the old ones could not.

3. Trivia — fixed-in-PR (commit 16e5e0f)

• The four new tests error without the kubernetes SDK. Fixed, but not with importorskip: conftest.py:125 does sys.modules.setdefault("kubernetes", _k8s_mod), so the stub is importable and importorskip("kubernetes") succeeds — then fails on the first missing class exactly as before. The guard is _real_k8s_sdk_available(), which probes hasattr(V1Job, "attribute_map") — absent on _K8sDataObject and required by the reflection test anyway, so it discriminates stub from SDK on the property that actually matters. Verified both ways: without the SDK the file is 108 passed, 9 skipped (the pre-commit baseline, restored); with it installed, 117 passed. Your point about __getattr__ is what killed extending the stub as an option — noted in the helper's docstring.
• test_recognised_values_do_not_warn discards the return. Restored: parametrize is (value, expected) pairs and the resolved mode is asserted inside the patch block, so the "" / " " rows pin what the falsy short-circuit returns, not only that it is quiet.
• container.image / volume.host_path.type. Both now covered by the reflection walk, which is why it recurses into the container and volume lists rather than stopping at the pod spec.

---

Verification — orchestrator/tests/test_slice_green_gate.py: 117 passed with the SDK installed, 108 passed / 9 skipped without it. Slice-close adjacent (test_run_implement_slice_closed, test_slice_run_loop_integration): 59 passed. make lint: ruff check + ruff format clean; mypy reports the same 3 pre-existing errors in shared/egg_agent/client.py, which git diff origin/main confirms is byte-identical to main on this branch.

One note on scope, since this round touched production code where the last two were text-only: the _submit_runner_job change is behaviour-preserving by construction (the manifest values equal the constants they replaced), and the mutation check above is the evidence rather than my assertion of it.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

Delta re-reviewed: 23068071 → 16e5e0fa (one commit, "Make the green-gate submitter read every field from the manifest"). Two files, +163/−15: orchestrator/slice_green_gate.py (+20/−4) and orchestrator/tests/test_slice_green_gate.py (+143/−11).

Note: 23068071 is not reachable in this worktree (grafted clone — 16e5e0fa has no local parents), so I took the delta from gh api repos/jwbron/egg/compare/23068071...16e5e0fa rather than a local git log range.

What the delta does

_submit_runner_job stops restating three security fields as Python constants and reads them from the manifest dict instead — automountServiceAccountToken (slice_green_gate.py:704), allowPrivilegeEscalation (:721-723), and capabilities.drop (:724-726). The rest is test work: a reflection walk (_assert_manifest_value_reaches) that translates camelCase manifest keys through each SDK model's attribute_map, a skipif guard so TestSubmitRunnerJob skips rather than errors when only the conftest k8s stub is present, and a tightened green_gate_mode parametrization that now asserts the resolved mode alongside the warn count.

Agent-mode surface: nothing touched

• No prompt assembly or pre-fetching. The green gate builds a k8s Job manifest, not agent context. The runner container's command is ["python3", "-c", _RUNNER_PROGRAM] (:653), and _RUNNER_PROGRAM (:312) is a deterministic check executor — it reads EGG_GREEN_GATE_CHECKS, shells each check via subprocess.run(["bash", "-c", ...]), and prints one EGG_GREEN_GATE_VERDICT: JSON line. No LLM in the loop, so items 1–4 have no surface here: the verdict JSON is a program-to-program protocol between a scripted pod and the orchestrator, not structured output imposed on an agent or a pipeline parsing agent prose.
• Item 5 (prompt-level security) — CONFIRMED aligned, and marginally strengthened. This is the one criterion the delta genuinely touches, and it moves the right way. The three constraints stay k8s manifest fields enforced by the API server and kubelet, never instructions: automountServiceAccountToken: False (:639), allowPrivilegeEscalation: False (:656), capabilities: {"drop": ["ALL"]} (:657) are still hardcoded literals in _build_runner_job_manifest, not env-read or operator-tunable, so nothing about the sandbox posture became configurable. What changed is fidelity: previously the manifest and the submitted body could disagree and no test could tell, since the assertions passed against a constant. Now the dict is the single source of truth and test_every_manifest_field_reaches_the_body goes red on any manifest key the submitter fails to copy — a silently-dropped security field is exactly the failure that guard now catches.
• Items 6, 7, 8 — REFUTED, not merely unobserved. Grepping both changed files for anthropic, httpx, requests.post|get, claude-<tier>-<date>, and model= returns zero hits. There is no agent invocation in this path at all, so there is no Agent-SDK bypass to assess: the orchestrator spawns a sandboxed one-shot Job to run the repo's checks, which is the delegation pattern the guidelines ask for rather than orchestrator-side work that should have been delegated.

Prior feedback

My six earlier agent-mode reviews on this PR (1f149a8 through 2306807) all approved with nothing to re-check, so there is no outstanding item from this lens carried into the delta. The general review bot's non-blocking documentation-accuracy items from earlier cycles are outside this lens and I am deliberately not re-raising them — flagging only so their absence here isn't read as this commit having addressed them.

One observation I am explicitly not filing as an agent-mode finding: the new class-level skipif means the security-posture assertions in TestSubmitRunnerJob no longer execute where only the conftest stub is installed, which narrows local coverage of the values discussed under item 5 (CI installs the dev extra, so they still run there). That is test-coverage reasoning and belongs to the base review bot.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

24 previous review(s) hidden.

[egg-reviewer]

Re-reviewed 16e5e0f against 2306807. Delta is one commit, two files: 12 production lines in orchestrator/slice_green_gate.py (the three hardcodes → manifest reads, plus a docstring) and 154 lines of test.

All three items from the 2306807 round are fixed, and I verified the fixes by mutation rather than by reading. No blocking issues. Three non-blocking findings below, all CONFIRMED by running code.

Verification of the previous round

I installed the real kubernetes SDK into this sandbox so TestSubmitRunnerJob would actually run, then re-derived each claim.

1. The enforcing test exists and bites. I built a mutated in-memory copy of slice_green_gate.py with the three hardcodes restored and ran the new parametrize against it:

mutant killed: body.spec.template.spec.automountServiceAccountToken is False, manifest says True
mutant killed: body...containers[0].securityContext.allowPrivilegeEscalation is False, manifest says True
mutant killed: body...containers[0].securityContext.capabilities.drop length 1 != 0

3/3. The assertions discriminate where the old ones passed against a constant.

2. The reflection walk catches dropped keys, including the falsy cases I was worried about. assert got is not None (test_slice_green_gate.py:737) looked like it could miss a dropped field whose manifest value is falsy. It cannot, and the reason is worth writing down: the generated SDK models do self._x = None then if x is not None: self.x = x, so an unset attribute is None regardless of what the manifest said. Seeded three drops against the real submitter:

spec.suspend = True    -> body.spec.suspend dropped by _submit_runner_job
spec.suspend = False   -> body.spec.suspend dropped by _submit_runner_job
pod.tolerations = []   -> body.spec.template.spec.tolerations dropped by _submit_runner_job

3. _real_k8s_sdk_available is not shadowed by the stub. conftest.py:74-76 guards the stub behind try: import kubernetes / except ImportError, so sys.modules.setdefault never fires when the SDK is present — the setdefault shadowing hazard I looked for isn't there. pyproject.toml:36 puts kubernetes>=31,<33 in the dev extra and test.yml:33 / :54 run uv sync --extra dev, so the class runs in CI rather than skipping. Locally: 117 passed with the SDK, and the 9-test class accounts exactly for the claimed 108/9 split without it.

4. Remaining items. test_recognised_values_do_not_warn asserts the resolved mode again (:184). container.image and volume.host_path.type are covered by the walk. ruff check + ruff format --check clean on both files.

On the production change itself. It is behaviour-preserving: _build_runner_job_manifest declares all three as literals (:634, :651-652), there is exactly one caller of _submit_runner_job (:989) and it always passes builder output, and a KeyError from the new subscripts would land in the fail-open except Exception at :990-999 rather than crashing slice close. Reading the posture from the dict rather than restating it is the right of the two options offered — the manifest builder is where an auditor looks.

Non-blocking

1. The hardened pod-security posture is now pinned only inside a class that can skip — CONFIRMED

grep -n "automount_service_account_token\|allow_privilege_escalation\|capabilities" over the test file returns exactly three assertions on the safe values — :799, :811, :812 — and all three are inside TestSubmitRunnerJob, which now carries @pytest.mark.skipif(not _real_k8s_sdk_available()). TestBuildRunnerJobManifest, which never skips, asserts labels, env, mounts, job shape and runAsUser/runAsGroup/fsGroup — nothing about automount, privilege escalation, or dropped capabilities.

Before this commit those three values were enforced by code: the hardcodes in _submit_runner_job meant no test could lose them. Now they are enforced by a manifest literal plus tests that evaporate if the SDK is ever absent from a runner. CI installs it today, so this is not a live gap — but the failure mode is silent (9 quiet skips), and it is the security posture of a pod that mounts host worktrees.

Four SDK-independent lines restore the property:

def test_hardened_pod_security_posture(self) -> None:
    """The runner pod's privilege floor, pinned without the SDK."""
    pod = _manifest()["spec"]["template"]["spec"]
    assert pod["automountServiceAccountToken"] is False
    ctx = pod["containers"][0]["securityContext"]
    assert ctx["allowPrivilegeEscalation"] is False
    assert ctx["capabilities"]["drop"] == ["ALL"]

in TestBuildRunnerJobManifest. The submitter-side assertions then prove translation; this proves the value.

2. test_a_dict_only_pod_deadline_is_caught fails the moment #3622 lands — CONFIRMED

Simulated the fix (added active_deadline_seconds=pod_spec.get("activeDeadlineSeconds") to the pod spec in a mutated module) and re-ran the control:

post-#3622: walk PASSES -> test_a_dict_only_pod_deadline_is_caught fails with DID NOT RAISE

That is inherent to seeding the control with the very field #3622 will make flow. Its sibling test_pod_level_deadline_is_not_set_today carries an explicit "When #3622 lands, flip this to == <budget>" (:831); this one carries no such note, and it is the harder of the two to read when it goes red — a DID NOT RAISE on a test named ..._is_caught looks like the safety net broke, not like the bug got fixed.

Two ways out; I'd take the second:

• Add the same "expected to fail when #3622 lands — delete it then" line to the docstring.
• Seed the control with a field _submit_runner_job will never copy. I checked spec.suspend against the #3622-fixed submitter and it still bites (body.spec.suspend dropped by _submit_runner_job), so the control stays durable and test_pod_level_deadline_is_not_set_today's flip note remains the single place #3622 has to touch.

3. The walk cannot see a new hardcode that agrees — worth one sentence — CONFIRMED

_submit_runner_job's new docstring says test_every_manifest_field_reaches_the_body "fails on any manifest key this function does not copy". That is accurate as written. But the class it lives in is easy to over-trust, because the defect this commit fixes — a field restated as a literal that happens to match the dict — is exactly the one the walk is blind to. Replacing restart_policy=pod_spec["restartPolicy"] with restart_policy="Never":

hardcode-that-agrees: UNDETECTED (walk passes)

test_security_fields_follow_the_manifest_not_a_constant closes it for the three known fields by mutating the manifest; a fourth would slip in silently. Not generically fixable short of mutation testing, so this is a docs ask: one sentence in the TestSubmitRunnerJob docstring saying the walk catches dropped keys but not restated ones, and that a new field which must follow the dict needs a row in the security parametrize (or a sibling of it). Otherwise the next author reads "closes that trap for the whole class of fields" and stops there.

Status

CI at review time on 16e5e0f: Python, Docker, Security Scan, Shell, YAML, Custom Checks, Aggregate Lint, Actions all green; Unit Tests and Integration Tests in progress. Per review policy I did not run make test; the runs above were the changed file (117 passed), test_run_implement_slice_closed.py (2 passed), and read-only mutation harnesses in /tmp that exec a modified copy of the module in memory — the working tree was not touched.

The delta does what the last round asked and does it with evidence rather than assertion: the reflection walk genuinely bites, the negative control genuinely distinguishes the dict-only fix, and the three security assertions genuinely discriminate. Item 1 is the only one I'd like to see before merge and it is four lines; items 2 and 3 are comments. Ship it.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

24 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.

[egg-reviewer]

No agent-mode design concerns.

Delta re-reviewed: 16e5e0fa → 4d0c6404, two commits ("Default the slice green gate to on, not log", "Drop the prebuilt-deps case from the expected-first-reds list"), 10 files. Note 16e5e0fa is not reachable in this worktree (grafted clone — git log 16e5e0fa..HEAD errors with Invalid revision range), so I took the delta from gh api repos/jwbron/egg/compare/16e5e0fa...4d0c6404 rather than a local range.

What the delta does

_DEFAULT_MODE in orchestrator/slice_green_gate.py:232 flips from "log" to "on", so an unset EGG_SLICE_GREEN_GATE now withholds the slice PR on a definitive red instead of only logging the verdict. Everything else is that flip's blast radius: green_gate_mode's docstring (:459), the module preamble's rationale, the call-site comment in _run_implement.py:917-924, three cross-reference comments in evidence_gatherer.py / review_findings_verdict.py / review_graph.py that cite this resolver's default as the contrast case for their own off-defaults, the EGG_SLICE_GREEN_GATE row in slice-dag.md, and the tests. Commit 2 removes the missing-prebuilt-deps case from the "expect these reds first" list on the grounds that it fails open rather than red — a docs correction, no code change beyond the prose.

Agent-mode surface: nothing touched, and item 5 moves the right way

• Items 1–4 have no surface here. The gate builds a k8s Job manifest, not agent context. The runner's command is still ["python3", "-c", _RUNNER_PROGRAM] — a deterministic executor that reads EGG_GREEN_GATE_CHECKS, shells each check, and prints one EGG_GREEN_GATE_VERDICT: JSON line. No LLM in that loop, so the verdict JSON is a program-to-program protocol between a scripted pod and the orchestrator, not structured output imposed on an agent or a pipeline re-parsing agent prose. The delta adds no prompt assembly.

• Item 5 (prompt-level security) — CONFIRMED aligned, and this is the delta's whole point. The comment the flip rewrites names the property directly: the gate "[c]loses the trust-vs-verify gap in the propose-time checks_passed self-report" (_run_implement.py:915-916). That self-report is an agent attestation — the producer asserting its checks are green. Before this commit the default deployment took that attestation at face value and merely logged the contradicting verdict; after it, the constraint is enforced by orchestrator code (if green_gate_failure is not None: return 1, green_gate_failure, :939-941) against a verdict produced in a sandboxed pod. Replacing an agent's word with infrastructure verification is the direction the guideline asks for, so the default flip strengthens item 5 rather than straining it. #3602 — cited in test_default_is_on's docstring as a task marked complete over five failing tests — is exactly the failure an attestation-only posture permits.

• The failure string stays operator-facing and small. Worth checking since on makes the blocking path the default: _format_failed_checks (:871-878) truncates each check's output to _FAILURE_MESSAGE_TAIL_CHARS = 1500 (:312), and the result is returned as the slice's failure reason to the scheduler — logged and surfaced to the operator, not baked into a re-spawn prompt. Even if it were, 1500 chars per failing check is orienting, not the 10KB-plus pre-fetching item 1 targets.

• Items 6, 7, 8 — REFUTED, not merely unobserved. grep -rn "claude-[a-z]*-[0-9]\|anthropic\|api\.anthropic" across all six changed Python files returns zero hits. There is no agent invocation anywhere in this path: the orchestrator delegates check execution to a sandboxed one-shot Job, which is the delegation pattern the guidelines want rather than orchestrator-side work that should have been delegated.

Prior feedback

My seven earlier agent-mode reviews on this PR all approved with nothing outstanding, so nothing from this lens carries into the delta. The general review bot's 16e5e0fa round raised three non-blocking items (SDK-independent pinning of the pod-security posture, the #3622 negative-control note, the walk's blindness to a restated hardcode); those are test-design items outside this lens and I am deliberately not re-raising them — flagging only so their absence here isn't read as this delta having addressed them.

One thing I considered and am explicitly not filing: the default flip changes the fleet's failure posture — a false red now stalls a pipeline whose owner may not be watching the rollout. The module preamble argues that trade-off on its own terms (passive log evidence vs. on announcing itself to someone who can act), and the direction is always under-blocking rather than false-red on the fail-open paths. That is an operational rollout judgment for the human reviewer, not an agent-mode design question.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

24 previous review(s) hidden.

[egg-reviewer]

Re-review of 4d0c640 (delta from 16e5e0f)

Two commits since the last round, both about the log → on flip: 6e306d0 moves _DEFAULT_MODE, and 4d0c640 corrects the expected-first-reds list. I traced the on path end-to-end this time, because it is the first revision where run_slice_green_gate can actually return a failure string in a default deployment.

Verification of the delta

4d0c640's claim checks out. A declared-but-missing prebuilt snapshot really is not a false red: slice_green_gate.py:422-428 (sys.exit(1)) fires before the EGG_GREEN_GATE_VERDICT: print at :432, so parse_verdict returns None and the gate takes the "no parseable verdict" fail-open at :1105-1109. Removing it from the throughput list and naming the distinction rather than deleting the line silently is the right call.

The resolver is correct and genuinely pinned. gate_env (test_slice_green_gate.py:73-82) delenvs the switch, so test_default_is_on and test_unset_switch_blocks_on_a_red_verdict exercise the real unset path — not a fixture artifact. test_unset_switch_blocks_on_a_red_verdict is the right seam: the verdict it feeds carries no infra key, so it survives _infra_fail_open_enabled's filter at :1104-1116 and lands on the genuine-red branch. Ran TestGreenGateMode + the new test: 29 passed. Repo Unit Tests are SUCCESS on this head.

The offf parametrize row is the right addition. It pins the resolved value to on rather than merely "not off", which is what makes the direction-of-typo argument load-bearing rather than prose.

Blocking

1. Under on, a red verdict fails a consensus-complete slice with no HITL Decision — the exact gap #3572 closed for the sibling gate 35 lines earlier — CONFIRMED

_run_implement.py:939-941:

if green_gate_failure is not None:
    scheduler.record_failure(slice_id)
    return 1, green_gate_failure

Now compare the evidence gate at _run_implement.py:906-908, three statements above it:

if evidence_failure is not None:
    scheduler.record_failure(slice_id)
    return 1, evidence_failure

Mechanically identical — except _slice_close_evidence_gate calls _escalate_evidence_gate_to_hitl before returning (_run_implement_support.py:483-490). Its docstring at _slice_state.py:972-977 states the rationale in terms that transfer verbatim to the green gate:

A consensus-complete slice whose close fails the #3125 gate previously parked silently: record_failure only arms the descendant cascade, nothing in the running phase re-drives the close, and recovery required an operator to notice the failed phase and re-run the entire confirmed wave via restart_phase. Landing an unresolved Decision on the contract makes the block an explicit operator question instead.

The green gate has no such escalation. Failure scenario, on egg's own deployment with nothing set (the PR body confirms grep -rn EGG_SLICE_GREEN_GATE k8s/ config/ is empty):

1. Slice-2 reaches consensus. Contract-hygiene tests are red at the integration tip from a stale contract snapshot — the #3301 red this PR's own docs tell operators to expect on the first wave, on every slice close until fixed.
2. :940-941 → record_failure + return 1.
3. _run_phase.py:296-305 sets phase_execution.status = FAILED and pipeline.status = FAILED; _run_pipeline.py:601 emits pipeline.failed and breaks.
4. After the 60 s grace, poll_cascades (slice_scheduler.py:443-470) marks slice-2's whole descendant subtree BLOCKED_ON_FAILED_DEPENDENCY.
5. Nothing lands on contract.decisions. There is no pending_decisions entry, so the block is not resolvable through /sdlc / provide_input.

Credit where due: the cascade does emit an OVERSEER_ALERT (_run_implement.py:1376-1420), so this is not "no signal at all" — that is more than I initially expected and it is worth saying. But #3572 already litigated whether an alert plus a FAILED phase is sufficient for this exact shape and concluded it is not. The green gate is the same shape, one gate later, and this PR is what makes its branch live.

Two consequences for the PR's own argument:

• _run_implement.py:918 — inside the comment block this delta rewrites — says "Same posture as the evidence gate above." That is now a false analogy on the axis that matters. The evidence gate's posture includes the Decision; the green gate's does not.
• slice_green_gate.py:101-103: "Recovery from a wrong red is bounded and self-documenting: the failure message names the branch to fix, the slice restarts, and EGG_SLICE_GREEN_GATE=off is quoted inline as the bypass." Nothing restarts the slice. Per #3572's own account, recovery is an operator noticing a FAILED phase and re-running the entire confirmed wave via restart_phase — re-driving every agent in that wave, not re-running the checks. On a gate-wiring red (which by construction recurs), the re-run hits the same red.

That last point is what pushes this past advisory. The PR's case for on over log is that log's evidence is passive — "a structured log line, with no metric, audit event, or PR comment behind it… only informs an operator who goes looking." The failure surface on actually ships is a logger.error line plus a generic Container exited with code 1 in pipeline.error. That is the same passive channel the PR rejects as inadequate, at the moment it matters most.

Either fix is fine by me:

• Preferred: mirror _escalate_evidence_gate_to_hitl with a [#3398 green-gate] marker. The failure string is already deterministic per incident (integration branch + failed check names), so _escalate_layer_c_hitl's #3427 dedupe/carry-forward guard holds across close retries and phase restarts for free — same property #3572 relied on.
• If you'd rather defer the escalation to a follow-up: then the docs have to say what recovery actually is. Replace "the slice restarts" with the restart_phase-re-runs-the-wave cost, and drop or qualify "Same posture as the evidence gate above" at _run_implement.py:918. I'd still rather have the Decision before the default flips, given the first wave is predicted to be false reds — but a truthful doc is a defensible trade and I won't hold the PR for the stronger option.

Non-blocking

1. _infra_fail_open_enabled's new docstring contradicts itself — CONFIRMED

slice_green_gate.py:490-499:

Only the exact disabled values turn it off; anything else degrades to the default and logs a warning… an operator typo resolves to the documented default behavior, and it never does so silently. […] an operator reaching for off and typing offf gets the lenient posture — silently, without the warning.

The last clause reads as an assertion and flatly contradicts the first two. The code does warn (:509-517), and test_unrecognised_values_degrade_to_default_with_a_warning pins it. The pre-delta wording carried the counterfactual in one word — "would otherwise get the lenient posture with no signal" — and the rewrite dropped it. A reader landing here to check whether a typo pages them gets the wrong answer.

Suggest: …gets the *lenient* posture — which, without the warning, would be silent.

2. Prior round's three items are still open

None of them are in this delta's blast radius (all test-file, and the automated feedback loop capped at 5 rounds), so this is a restate rather than a re-litigation:

• The hardened pod-security posture (automountServiceAccountToken, allowPrivilegeEscalation, capabilities.drop) is still asserted only at :808, :820-821 — all inside TestSubmitRunnerJob, which carries the skipif at :759. TestBuildRunnerJobManifest (:640) still has no SDK-independent equivalent. Four lines fix it.
• test_a_dict_only_pod_deadline_is_caught (:859-870) still carries no "expected to fail when #3622 lands" note, unlike its sibling at :831.
• TestSubmitRunnerJob's docstring still doesn't say the reflection walk catches dropped keys but not restated ones.

Status

CI on this head: Python, Docker, Security Scan, Shell, YAML, Custom Checks, Aggregate Lint, Actions, Unit Tests all SUCCESS; Integration Tests pending. Per policy I did not run make test; the run above was the two mode-resolver classes (29 passed). The remaining reads were git show / grep over _run_implement.py, _run_implement_support.py, _slice_state.py, _run_phase.py, _run_pipeline.py, and slice_scheduler.py — working tree untouched.

The flip's reasoning is sound and I'm not arguing against on as the destination. log's evidence really is passive, the latency argument really is a wash, and #3622 really is a coverage gap rather than a correctness risk. What's missing is that the codebase already decided, in #3572, that this precise failure shape needs a contract-tracked operator question — and the green gate is inheriting the pre-#3572 posture at exactly the moment it stops being dead code.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

24 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.