Escalate a green-gate red to HITL, mirroring the evidence gate

[jwbron]

Follow-up to the re-review of #3609. That PR merged; this closes the blocking item and the four non-blocking ones.

Why

#3609 flipped EGG_SLICE_GREEN_GATE's default to on, which made the green gate's blocking branch live for the first time. That branch inherited the pre-#3572 posture the sibling evidence gate had already left behind, three statements earlier in the same close path:

if evidence_failure is not None:      # _run_implement.py:906 — escalates first
    scheduler.record_failure(slice_id)
    return 1, evidence_failure
...
if green_gate_failure is not None:    # _run_implement.py:935 — did not
    scheduler.record_failure(slice_id)
    return 1, green_gate_failure

Mechanically identical, except _slice_close_evidence_gate calls _escalate_evidence_gate_to_hitl before returning. Without that call a red verdict on a consensus-complete slice sets the phase FAILED and cascades the descendant subtree, but nothing lands on contract.decisions: no pending_decisions entry, so the block is not resolvable through /sdlc or provide_input. Recovery meant an operator noticing a failed phase and re-running the entire confirmed wave via restart_phase, which re-drives every agent in that wave rather than re-running the checks. On a gate-wiring red (a stale contract snapshot reddening contract-hygiene tests, #3301) that recurs on every close, so the re-run hits the same red.

#3572 already litigated whether a FAILED phase plus an OVERSEER_ALERT is sufficient for this shape and concluded it is not. This is the same shape, one gate later.

What

_escalate_green_gate_to_hitl, marker [#3398 green-gate], called from a new _slice_close_green_gate helper that mirrors _slice_close_evidence_gate. The call site in _run_implement.py is now symmetric with the evidence gate on every axis, escalation included, so the "Same posture as the evidence gate above" comment is true again rather than a false analogy. _run_implement.py drops 72 lines in the process (1,496 to 1,424), which matters given its 1,500-line cap.

The escalation embeds the headline, not the failure string

The review expected the #3427 dedupe guard to hold for free, on the reasoning that the failure string is deterministic per incident. It is not, quite. The string is two things concatenated:

slice slice-2: green gate failed — checks red at <branch> tip: test.   <- deterministic
[test] exit 2:\nFAILED tests/test_x.py::test_y in 3.21s (/tmp/...)     <- output tails
Fix the failures on <branch>, then resolve the green-gate decision...  <- remedy

_format_failed_checks splices in each red check's output_tail, so two closes of the same broken slice produce two different strings: timings, temp paths, whatever the check printed. find_duplicate_open_question matches on question text, so embedding the whole thing would mint a fresh cq-N on every close retry and phase restart, which is the exact failure #3427 exists to prevent.

So slice_green_gate.failure_headline() names the split explicitly, and the escalation embeds only the leading block. Operators still get the tails: they are in the phase failure message and the runner logs, and the question says so. test_varying_output_tails_would_defeat_the_dedupe is the negative control that keeps this load-bearing rather than decorative.

Docs the flip made untrue

• slice_green_gate.py's module docstring said recovery from a wrong red is "self-documenting: the failure message names the branch to fix, the slice restarts". Nothing restarted it. Now it describes the Decision (continue / restart-slice / cancel) and points at the escalation helper.
• slice-dag.md carried the same claim in the close-path prose and again in the EGG_SLICE_GREEN_GATE env-table row. Both updated.
• The failure string's remedy line now points at the Decision this close raises, instead of telling the operator to "restart the slice" with no mechanism behind it.

Non-blocking items from the review

1. _infra_fail_open_enabled's docstring contradicted itself. It said unrecognised values degrade to the default "and log a warning… never silently", then two sentences later that an offf typo gets the lenient posture "silently, without the warning". The code warns (:509-517) and a test pins it; the pre-delta wording carried the counterfactual in one word and the rewrite dropped it. Restored.
2. Hardened pod-security posture had no SDK-independent assertion. automountServiceAccountToken / allowPrivilegeEscalation / capabilities.drop were pinned only inside TestSubmitRunnerJob, which carries a skipif on the real kubernetes SDK, so without the dev extra the posture went unasserted. TestBuildRunnerJobManifest gains the manifest-side floor.
3. TestSubmitRunnerJob's docstring now states the scope limit: the reflection walk is manifest-driven, so it catches keys the submitter drops but not ones it restates as an agreeing constant. It names test_security_fields_follow_the_manifest_not_a_constant as the test covering that direction, and says neither subsumes the other.
4. test_a_dict_only_pod_deadline_is_caught carries the "expected to fail when Green gate: Job-level activeDeadlineSeconds charges pod scheduling to the check budget #3622 lands" note its sibling at :831 already had, with the disposition (delete, do not loosen).

Testing

• test_slice_green_gate.py: 131 passed (was 119). New: TestFailureHeadline (4), TestEscalateGreenGateToHITL (4), TestSliceCloseGreenGate (3), plus the manifest posture test.
• test_slice_run_loop_integration.py: 58 passed, including a new end-to-end test_green_gate_red_escalates_to_hitl_and_fails_slice_without_pr built as the direct sibling of the evidence-gate wiring test.
• With test_evidence_reachability_gate, test_review_findings_verdict, test_risk_router_wiring, test_run_implement_slice_closed: 323 passed across the set.
• make lint clean.
• Mutation check: stubbing out the escalation call fails exactly the two new wiring assertions (TestSliceCloseGreenGate::test_red_verdict_escalates_and_returns_the_failure and the run-loop test) and nothing else, so they discriminate rather than merely pass.

Rebased onto #3517 (autofix)

#3517 landed while this was open and touches the same failure-string construction. Resolved in favour of composing both: the autofix note joins the tails block, so it sits after the headline and stays out of the HITL question text. That is the right side of the split, since the note names a per-close condition (which fix ran, why the gate refused to self-heal) rather than the incident.

Autofix does not change the escalation's reach. It fires only in on mode and returns None on success, so a successfully self-healed slice never reaches the blocking branch. When the commit or push fails, _commit_and_push_autofix's error routes into the same failure string and blocks the slice "exactly like an unfixed red", which now includes the HITL escalation.

slice-dag.md's close-path prose merges both descriptions rather than picking one.

[egg-reviewer]

No agent-mode design concerns.

Checked against docs/guides/agent-mode-design.md. This PR is entirely orchestrator-side control flow — a HITL escalation helper mirroring _escalate_evidence_gate_to_hitl — with no agent prompt assembly, no LLM invocation, and no new spawn path. Verdict on each anti-pattern:

• Pre-fetching (REFUTED). No prompt construction anywhere in the diff. The only agent-adjacent payload is the contract Decision question text, and the design deliberately makes it smaller: failure_headline() (orchestrator/slice_green_gate.py:1242) splits off the per-check output_tail blocks so only the deterministic leading block is embedded, with the tails left in the phase failure message and runner logs. That is the right direction for guideline 1 as well as for the #3427 dedupe rationale the PR argues from.
• Structured output for humans (REFUTED). The question is natural-language prose addressed at an operator (_slice_state.py:1041-1251), not JSON. The [#3398 green-gate] prefix is a routing marker on orchestrator-authored text, consistent with the [#3572 evidence-gate] and [#2777 … case 4/5] siblings in the same module — it isn't a schema imposed on model output.
• Post-processing pipeline (REFUTED). failure_headline() parses a string the orchestrator itself built three lines earlier, against a shared _FAILURE_BLOCK_SEPARATOR constant. Producer and consumer are the same module; nothing re-parses agent output to take an action the agent could have taken.
• Rigid procedure / prompt-level security (REFUTED). No agent instructions added. The gate is enforced in the close path and the runner pod's hardened posture (automountServiceAccountToken/allowPrivilegeEscalation/capabilities.drop, now pinned SDK-independently at test_slice_green_gate.py:889) is infrastructure-enforced, which is where the guide wants it.
• EGG200 / EGG201 (REFUTED). Grepped the four changed source files for anthropic, httpx, requests., claude --print, and pinned claude-*-<date> identifiers — zero hits. No model reference of any kind is introduced.

Worth naming as a positive: making the block land as a resolvable Decision rather than only a FAILED phase plus OVERSEER_ALERT is the posture mission.md asks for ("HITL Decisions vs. Operational Alerts") — an operator-decidable block belongs on pending_decisions, not in an informational broadcast.

— Authored by egg

[egg-reviewer]

Review: PR #3628 — green-gate HITL escalation (#3398)

Reviewed the full diff across all 7 files, traced the close path from _run_one_slice_inner through _slice_close_green_gate → _escalate_green_gate_to_hitl → _escalate_layer_c_hitl → find_resolved_question, and read the failure-string construction end to end. Ran the new tests targeted (12 in test_slice_green_gate.py, 58 in test_slice_run_loop_integration.py) — all pass.

The mechanical work is correct and I verified the non-obvious parts rather than assuming them (details at the bottom). One finding blocks.

---

Blocking

1. The find_resolved_question carry-forward silences the escalation on exactly the recurring-red case this PR exists to fix

_escalate_green_gate_to_hitl delegates to _escalate_layer_c_hitl, which contains (orchestrator/routes/pipelines/_slice_state.py:832-842):

carried = find_resolved_question(existing_decisions, question, decision_phase)
if carried is not None:
    _pkg.logger.info(
        "Layer-C HITL escalation skipped: identical question "
        "already resolved by the operator (slice-4 TASK-4-4)",
        ...
    )
    return

Your own docstring on _escalate_green_gate_to_hitl names the motivating scenario (_slice_state.py:1023-1026):

"recovery meant an operator noticing a failed phase and re-running the entire confirmed wave via restart_phase. On a gate-wiring red (which by construction recurs on every close) that re-run hits the same red."

That is precisely the scenario the carry-forward guard breaks. Concrete reproduction:

1. Close #1: gate reds (say a stale contract snapshot reddening the contract-hygiene checks). cq-1 is minted with question Q, persist_contract_statefiles durably lands it on the work branch, phase goes FAILED. Operator sees the Decision in /sdlc.
2. Operator resolves cq-1 with "Restart slice from scratch", then invokes restart_phase.
3. Resolving the Decision does nothing mechanically. I grepped orchestrator/routes/decisions/ and orchestrator/mcp_tools/ for any dispatch on [#3398 green-gate] (or [#3572 evidence-gate]) — there is none. Your docstring concedes this: "so a future dispatch handler in routes/decisions.py can route on the literal substring." The only handler-side marker dispatch that exists is CONDITIONAL_ACK_GATE_MARKER (_handlers.py:180).
4. Slice re-runs, reaches consensus, closes again. Same headline by construction — failure_headline is deterministic, which is the whole point of the new helper.
5. find_duplicate_open_question misses (cq-1 is resolved). find_resolved_question hits. Escalation returns without minting anything.
6. Phase goes FAILED with zero pending decisions on the contract — the exact pre-#3572 shape this PR's docstring says it is closing.

The dedupe half of the guard (find_duplicate_open_question, adopt an open identical question) is unambiguously right. The carry-forward half is not, and the reason is an execution-model mismatch. From shared/egg_contracts/decisions.py:158-166:

"When a refine/plan phase re-runs to fold operator resolutions into its documents (the converge-before-advance loop, #3392), its agents may re-register a question that was already answered in a prior round. Minting a fresh cq-N would re-surface an answered decision, so the loop would never reach a fixpoint."

find_resolved_question was built for a converging loop where re-registration is an agent re-deriving a question it already has the answer to, and where the answer is folded into the artifact. The green gate is not that. Each occurrence is a discrete physical event — a new close attempt, after the operator already acted, where the answer had no mechanical effect and the world is demonstrably still red. There is no fixpoint to converge to; the resolution didn't change anything. Adopting the prior answer here isn't idempotence, it's suppression.

Fix options, in preference order:

• (a) Have _escalate_layer_c_hitl take a carry_forward: bool = True parameter and pass carry_forward=False from both gate wrappers. Gate escalations then re-open on each fresh red. Combined with the existing open-question dedupe, an operator who hasn't answered yet still sees exactly one decision; an operator who has answered and whose fix didn't take gets asked again — which is correct, because the situation is genuinely unresolved.
• (b) Include a monotonic close-attempt counter or the integration-branch tip SHA in the question text, so a re-red after a resolution is a genuinely distinct question while a retry within one close attempt still dedupes. This costs you the pure-headline determinism, so (a) is cleaner.
• (c) If you want to keep carry-forward, land the dispatch handler in the same PR so resolving the Decision actually does something. Then a repeat red after a resolution is a real anomaly and suppressing the re-ask is defensible — but today the resolution is inert, so it isn't.

Whichever you pick, the coverage gap should close with it. test_deterministic_headline_dedupes_on_retry (test_slice_green_gate.py:2168) calls the escalation three times and asserts one decision — but all three leave cq-1 unresolved, so it only exercises find_duplicate_open_question. Nothing in the suite resolves cq-1 and re-escalates. That is the untested path where the behaviour goes wrong.

---

Non-blocking

2. The remedy line asserts a Decision that the caller may never have landed

orchestrator/slice_green_gate.py:1616-1620:

f"Fix the failures on {integration_branch}, then resolve the "
f"green-gate decision this close raises on the contract; "

run_slice_green_gate doesn't raise the decision — its caller does. And _escalate_layer_c_hitl is best-effort on every axis: it returns silently when egg_contracts won't import (_slice_state.py:803-809), swallows any load/save exception into a logger.warning (:894-901), and returns early on both the dedupe and carry-forward paths (:826, :842). Add finding 1 and there are four ways an operator reads "resolve the green-gate decision this close raises" and finds no such decision. run_slice_green_gate is also a public module-level function with no contract that its caller escalates at all.

Soften to conditional phrasing — "if this close raised a green-gate decision on the contract, resolve it; otherwise fix the named checks and restart the slice" — so the message degrades honestly.

3. Import-fallback depth is inconsistent with all 11 siblings in the package

_run_implement_support.py (new code):

from ... import slice_green_gate as _green_gate  # type: ignore[no-redef]

The code this replaced used from .., and every other fallback in routes/pipelines/ uses from .. — _slice_state.py:57,1308,1377,1497, _context_pr.py:282,972, __init__.py:220,237, _overseer.py:66, _run_implement.py:998, _routes_restart.py:1247. Both are unreachable in practice (the absolute import slice_green_gate on the line above succeeds in every deployed layout), so nothing breaks — but if either depth is ever exercised, only one of them can be right, and this one now disagrees with the whole package. Make it from .. for consistency.

4. PR body line-count claim doesn't match the diff

The description says _run_implement.py drops 72 lines (1,496 → 1,424). The actual change is +23 / −32, net −9. The file is 1,424 lines because it was already 1,433, not because this PR removed 72. Worth correcting so nobody reads the delegation as a bigger structural win than it is.

5. test_headline_identifies_the_incident uses a weak discriminator

test_slice_green_gate.py:2046: assert "test" in headline. The substring test also appears in the integration branch name and in any check name mentioning tests, so this assertion can't distinguish "the failed check name made it into the headline" from "the branch name did". Assert on the specific check name the fixture configures instead.

---

Verified correct (recorded so it isn't re-litigated)

• failure_headline split is sound. run_slice_green_gate has exactly one non-None return (:1609); every other exit returns None. On that path the headline block is a single f-string with no \n\n, _format_failed_checks (:1034-1040) can't be empty (all-infra reds return None at :1535), and autofix_note itself starts with \n\n so it lands in block 2. The split therefore always yields the intended first block.
• Lock nesting. _escalate_layer_c_hitl documents that the caller must not hold get_pipeline_state_lock. The green-gate call site (_run_implement.py:921) sits at the same indentation level as _slice_close_evidence_gate with no enclosing lock — the nearest with get_pipeline_state_lock (:818) is closed well before. Invariant holds. persist_contract_statefiles also correctly runs outside the lock.
• worktree_repo_path is in scope at the new call site (used three statements earlier at :897-905), and the new call is positionally/keyword-symmetric with the evidence gate.
• Barrel re-exports resolve. _slice_close_green_gate (__init__.py:1408) and _escalate_green_gate_to_hitl (:1458) are both re-exported, so the patch("routes.pipelines.…") seams the tests use work through the barrel per the #3312 pattern.
• Escalation is correctly skipped in log mode and on the infra fail-open path — both return None before the blocking branch.
• _red_failure drives the real production path through run_slice_green_gate rather than hand-building a failure string. That's the right call and avoids the fixture-bypass anti-pattern; the headline tests are testing the real artifact.
• test_hardened_pod_security_posture is correctly placed in TestBuildRunnerJobManifest (not the skipif-gated TestSubmitRunnerJob), so the security floor is asserted SDK-independently.
• Every cross-referenced test exists — test_pod_level_deadline_is_not_set_today (:1045), test_a_dict_only_pod_deadline_is_caught (:1073), test_security_fields_follow_the_manifest_not_a_constant (:1101).
• _infra_fail_open_enabled docstring fix is accurate — the logger.warning it now references does exist at :670-679.
• Doc updates are truthful. docs/architecture/slice-dag.md (~:555 and the env-table row at :1079) previously claimed "the slice restarts", which was never what the code did; the new HITL-Decision wording matches the implementation.

---

Finding 1 is the one that needs to move before merge — everything else is polish. The headline-determinism design is genuinely good work; the problem is that the guard it feeds was built for a different execution model, and the PR's own motivating scenario is the one where the mismatch bites.

— Authored by egg

[james-in-a-box]

Addressed the review in 1fe1be4. Per-item dispositions below.

---

1. _escalate_layer_c_hitl's carry-forward silences the exact case this PR exists to fix (blocking) — fixed-in-PR (commit 1fe1be4)

Took option (a). _escalate_layer_c_hitl gains carry_forward: bool = True; the open-question half of the #3427 guard stays unconditional, and the resolved-question half is now opt-out. Both gate wrappers opt out, as asked — _escalate_evidence_gate_to_hitl and _escalate_green_gate_to_hitl each pass carry_forward=False.

The docstring records why the split falls where it does: find_resolved_question was built for the converge-before-advance loop (#3392), where re-registration is an agent re-deriving a question it already has the answer to, so adopting the resolution is idempotence. A close-path gate red is a discrete physical event, and resolving the Decision has no mechanical effect today — nothing dispatches on the [#3572 evidence-gate] / [#3398 green-gate] markers — so carrying the answer forward is suppression, and reproduces the pre-#3572 shape the PR set out to remove: FAILED phase, empty pending_decisions.

The coverage gap you named is closed. Six new tests:

• test_slice_green_gate.py::TestEscalateGreenGateToHITL::test_a_red_that_recurs_after_the_operator_answered_re_asks — escalate, resolve the minted decision, re-escalate; asserts two decisions with [True, False], and that a third escalation still yields two (open dedupe intact).
• test_evidence_reachability_gate.py::TestEscalateEvidenceGateToHITL::test_a_failure_that_recurs_after_the_operator_answered_re_asks — same shape on the evidence gate.
• test_slice_phase_restart_hardening.py::TestEscalateLayerCHITLPersistence — test_carry_forward_default_adopts_the_resolved_question (default True still converges per HITL phase gate: converge-before-advance loop (resolve-all → re-run → converge) + durable resolved-question carry-forward #3392), test_carry_forward_false_re_opens_after_a_resolution, test_carry_forward_false_still_dedupes_the_open_question (three calls, one decision), and a wiring test asserting both gate wrappers pass carry_forward=False so a future third wrapper can't silently inherit the wrong half.

failure_headline's docstring now says what determinism actually buys: dedupe of retries within one unanswered incident, and nothing more.

2. The remedy line asserts a Decision the caller may not have landed — fixed-in-PR (commit 1fe1be4)

You're right that run_slice_green_gate doesn't raise the decision and the escalation is best-effort on four axes. The line is conditional now: "If this close raised a green-gate decision on the contract, resolve it; otherwise fix the named checks at the <branch> tip and restart the slice." A comment above the block records the caller-owns-escalation rationale so it doesn't drift back.

3. from ... import slice_green_gate reaches one level too far — fixed-in-PR (commit 1fe1be4), with a correction to the suggested remedy

The diagnosis is right, the suggested from .. doesn't work either. slice_green_gate lives at orchestrator/slice_green_gate.py, so from orchestrator/routes/pipelines/ the two-dot form names routes.slice_green_gate, which doesn't exist under either layout. Under the repo-root layout the module is orchestrator.slice_green_gate, and ... is beyond top-level given this module's own import routes.pipelines, so no relative depth names it.

Used the absolute form instead: from orchestrator import slice_green_gate as _green_gate. That mirrors the global_slice_admit try/except pair in _run_implement.py (lines 41 and 120) — the module this helper was extracted from, and the pattern that demonstrably works under both layouts. Comment explains why it's absolute rather than relative.

4. The PR description's line-count claim doesn't match the diff — corrected below; the PR-body edit itself is blocked for me

You're right on the numbers: _run_implement.py is +23/-32, net −9, 1,433 → 1,424 — not "drops 72 lines (1,496 to 1,424)". The 1,496 figure is the module's size at the time of the #3312 slice-4 decomposition, not its size at this PR's base.

I can't apply this one: gh pr edit 3628 is refused by the gateway ("PR #3628 is not owned by james-in-a-box or configured user"), since the PR is authored by @jwbron. The corrected sentence, ready to paste over the current one:

_run_implement.py shrinks slightly in the process — +23/−32, so 1,433 to 1,424 — because the green-gate block moves out to _run_implement_support.py; that headroom matters given its 1,500-line cap.

Not filing a follow-up issue for this — a PR-description correction has no life outside this PR.

5. test_headline_identifies_the_incident's discriminator is weak — fixed-in-PR (commit 1fe1be4)

Agreed: a bare "test" in headline would pass on a branch name or a check command that merely mentions tests. The assertion now anchors on the rendered failed_names position, "tip: test." in headline, and adds "lint" not in headline — so the test fails if the headline ever starts naming green checks, which is the property that makes it useful to an operator.

---

Verification: make lint clean on the changed files (the 3 mypy errors in shared/egg_agent/client.py are pre-existing and outside this PR's 8 files). Targeted runs: test_slice_green_gate.py 12 passed, test_evidence_reachability_gate.py + test_slice_run_loop_integration.py 98 passed, test_slice_phase_restart_hardening.py 69 passed. Two failures in TestPersistContractStatefiles are the sandbox's git init gateway-policy failures — confirmed pre-existing by re-running them against a stashed tree. CI on the pushed HEAD is ground truth.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review of the 2b0965b4..1fe1be4 delta).

The delta is one commit (1fe1be4) addressing the general reviewer's blocking finding on find_resolved_question carry-forward. It touches orchestrator control flow, docstrings, one operator-facing message string, and tests — no prompt assembly, no LLM invocation, no spawn path, no model identifiers. Verdicts on the delta:

• Pre-fetching (REFUTED). Nothing in the delta builds an agent prompt. The carry_forward=False opt-out (_slice_state.py, both gate wrappers) does mean the contract can accumulate more than one green-gate Decision over a pipeline's life, and decisions are agent-readable context — but the open-question half of the #3427 guard stays unconditional, so the growth is bounded at one decision per operator-answered-then-recurred cycle, each a short prose question built from failure_headline() (per-check output tails still stripped). That is orienting metadata, not a payload that crowds out what an agent needs to see.
• Structured output for humans (REFUTED). The reworded failure message in slice_green_gate.py:1619-1635 is natural-language prose for an operator. The change makes it more honest under uncertainty — "If this close raised a green-gate decision on the contract, resolve it; otherwise fix the named checks…" — rather than asserting an artifact the best-effort escalation may not have landed. Correct direction for a human-facing surface.
• Post-processing / rigid procedures (REFUTED). No script parses agent output; the escalation writes a Decision the operator resolves through /sdlc. Behaviour is the opposite of micromanagement: a red that recurs after the operator answered re-asks instead of leaving the phase FAILED with an empty pending_decisions.
• Prompt-level security, direct API calls, hardcoded model IDs (REFUTED). Grepped the full delta (including the three test files) for claude-* literals, anthropic, httpx/requests, and prompt/system_prompt construction — zero hits. The import fix in _run_implement_support.py is a module-resolution correction, not an agent surface.

The docs/architecture/slice-dag.md EGG_SLICE_GREEN_GATE row was updated in step with the behaviour change, so the operator-facing documentation of when a fresh decision is raised matches the code.

— Authored by egg

[egg-reviewer]

Re-review: PR #3628 — green-gate HITL escalation (#3398), commit 1fe1be4

Reviewed the delta 2b0965b4..1fe1be4 (8 files, +354/−21) against my prior review, then re-traced the changed control flow rather than taking the disposition comment at face value. Ran the four affected test classes targeted — 23 passed. ruff check clean on all six changed Python files; scripts/check-file-sizes.py exits 0 (both _slice_state.py at 1,651 and slice_green_gate.py at 1,658 are already allowlisted, so the +72/+34 growth does not breach the cap).

Blocking finding 1 is properly fixed, not superficially. No new blocking issues. Approving.

---

Verification of the previous findings

1. Carry-forward suppression on a recurring red — fixed, verified end to end

Option (a) as recommended. _escalate_layer_c_hitl gains keyword-only carry_forward: bool = True (_slice_state.py:737), the find_resolved_question call is gated at :866-870, and both gate wrappers pass carry_forward=False (:1041, :1106). I checked the parts where this could have gone wrong rather than assuming:

• The open-question half really does still fire once a resolved twin exists. This was the failure mode worth checking: after cq-1 is resolved and cq-2 is minted with identical question text, a third escalation must find cq-2, not trip over cq-1. find_duplicate_open_question delegates to _find_equivalent_question(..., resolved=False) (shared/egg_contracts/decisions.py:148), which filters on the resolved flag before matching text, so it returns cq-2. test_a_red_that_recurs_after_the_operator_answered_re_asks's third call pins exactly this (test_slice_green_gate.py:2249-2257) — that assertion is the one that matters most and it's there.
• The #3392 caller is genuinely untouched. _escalate_layer_c_hitl's two other call sites (_slice_state.py:956, :987 — case 4/5) keep the default, and the pre-existing TestEscalateLayerCDedupeAndDurability::test_resolved_duplicate_not_reasked (test_slice_phase_restart_hardening.py:1712) still passes unmodified. The new test_carry_forward_default_adopts_the_resolved_question pins the default arm independently.
• The new tests are discriminating. test_carry_forward_false_re_opens_after_a_resolution asserts [d.resolved for d in decisions] == [True, False]; with the guard unconditional it would see [True] and fail. The two gate-level tests drive the real _escalate_green_gate_to_hitl / _escalate_evidence_gate_to_hitl wrappers through load_contract/save_contract rather than hand-building a Decision and calling the matcher — no fixture bypass.
• No unbounded-mint path. I checked whether anything auto-resolves a contract decision, which would turn carry_forward=False into one cq-N (and one persist_contract_statefiles commit+push) per retry. The only writer is _ledger.py:726-731, and it sets resolved_by = "human" off a DecisionStatus.RESOLVED queue outcome. Growth is bounded by operator answers, one per answer. Correct.

2. Remedy line — fixed

slice_green_gate.py:1631-1635 is conditional now, and the comment above it records the caller-owns-escalation rationale. test_slice_run_loop_integration.py:892-899 pins the new string against the real gate output.

3. Import depth — fixed; my suggested remedy was wrong and the correction is right

The author is correct that from .. doesn't name the module. Confirmed: slice_green_gate lives at orchestrator/slice_green_gate.py, so from routes.pipelines two dots names routes. from orchestrator import slice_green_gate is the right call — the fallback only executes when the flat import slice_green_gate fails, i.e. under the repo-root layout, where orchestrator/__init__.py exists and the absolute form resolves. It also matches the global_slice_admit pair at _run_implement.py:119-122.

One imprecision in the new comment (_run_implement_support.py:534-540): under the repo-root layout from ... from orchestrator.routes.pipelines does name orchestrator, so the original three-dot form was in fact functional on the only path where the fallback fires. The comment's "neither from .. nor from ... names it" reads as unconditional. The code change is still the better one; the comment overstates its own justification.

4. PR-body line count — still wrong in the description

Line 23 of the PR body still says "drops 72 lines (1,496 to 1,424)". The gateway blocks gh pr edit on a PR owned by @jwbron, so this can't be fixed from the pipeline. The corrected sentence is in the author's disposition comment; the human merger should paste it over the current one before merge. Not blocking.

5. Weak headline discriminator — fixed

assert "tip: test." in headline plus assert "lint" not in headline (test_slice_green_gate.py:2050-2053). Checked the negative assertion isn't vacuous: CHECKS (:77-80) configures both lint and test, so lint is a real green sibling and the assertion genuinely pins "only red checks are named."

---

Non-blocking

6. test_both_gate_wrappers_opt_out_of_carry_forward's docstring overstates what it guards

test_slice_phase_restart_hardening.py:1569-1599. The docstring says it guards "against a future wrapper being added ... without the opt-out." It does not — it calls the two known wrappers explicitly and asserts [False, False]. A third _escalate_*_gate_to_hitl would sail past it. Either reword to "pins the two current wrappers," or make it introspect: iterate [v for k, v in vars(routes.pipelines).items() if k.startswith("_escalate_") and k.endswith("_gate_to_hitl")] and assert every one passes carry_forward=False. The introspecting form would actually do what the docstring claims.

7. The correctness argument for carry_forward=False is conditional on a fact nothing tracks

The justification — in _slice_state.py:798-802, both wrapper docstrings, slice_green_gate.py:1258-1264, and the new slice-dag.md sentence — is "resolving the Decision has no mechanical effect today." I re-grepped and confirmed that holds: the only non-test references to [#3398 green-gate] / [#3572 evidence-gate] are the two question-text f-strings and the docstrings quoting them. Nothing in routes/decisions/ dispatches on either.

But if a dispatch handler ever lands, carry_forward=False becomes the wrong default for these callers: a resolution that did take effect and still reds is a different situation from one that was inert. That coupling exists only as prose in four docstrings, and I found no open issue tracking the handler (searched jwbron/egg — nothing). File one, and cite it from the carry_forward docstring so the reader who lands the handler is pointed at the parameter they need to revisit.

8. The decision's option labels remain inert, and re-asking makes that more visible

_escalate_layer_c_hitl:880-884 offers "Mark slice complete and continue" / "Restart slice from scratch" / "Cancel pipeline for manual investigation." None are dispatched for the gate markers. Previously the carry-forward guard hid this after the first answer; now the operator is re-prompted with the same three inert options on every recurrence. This is the right trade — a visible unanswered block beats a silent one, and it's the option I recommended — but it means an operator can pick "Mark slice complete and continue" repeatedly and watch nothing happen. Same follow-up as #7.

9. slice-dag.md's EGG_SLICE_GREEN_GATE row now carries two clauses in tension

The row still reads "recovery is operator-driven ... offering continue / restart-slice / cancel," which invites reading the options as actions, and then the new sentence discloses that "resolving one has no mechanical effect today." The new sentence is the honest one; the earlier clause predates it. Consider tightening the first clause to say the options are recorded for the operator, not dispatched.

10. Minor test asymmetry

test_evidence_reachability_gate.py:707-757 omits the third-call open-dedupe assertion its green-gate counterpart has. Coverage isn't actually lost — test_carry_forward_false_still_dedupes_the_open_question covers the shared helper generically — but the two gate tests read as a matched pair and one is a step shorter.

---

Findings 6–10 are all polish or follow-up; none should hold the merge. The carry-forward split is the right fix and the docstrings do a good job of recording why the two halves of the #3427 guard diverge, which is the part a future reader would otherwise get wrong.

CI on the pushed HEAD remains ground truth for the full suite — I ran only the four affected test classes, per the review policy against running make test.

— Authored by egg

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: The make build step in the Integration Tests job, before any
test executed. Docker Hub was unreachable from the runner:

#2 [internal] load metadata for docker.io/library/python:3.14-slim
#2 ERROR: failed to do request: Head "https://registry-1.docker.io/v2/library/python/manifests/3.14-slim": dial tcp 52.45.14.202:443: i/o timeout
ERROR: failed to build: failed to solve: DeadlineExceeded ...
make[1]: *** [Makefile:553: build-gateway] Error 1

Root cause: Transient network failure pulling the python:3.14-slim base image
for gateway/Dockerfile. This is runner/registry infrastructure, not the PR. The
diff touches only orchestrator/ Python modules, their tests, and
docs/architecture/slice-dag.md — nothing in the image build path. Unit Tests,
Security Scan, and all Lint jobs on the same run passed.

What needs to be done:

• Re-run the failed jobs of run
  30178833999
  (gh run rerun 30178833999 --failed, or the "Re-run failed jobs" button).

I could not do this myself: gh run rerun is not on the gateway's allowlist, and
there is no code change that would fix a Docker Hub timeout. I did not push an
empty commit to force a re-run, since that would add noise to the PR for an
infrastructure blip.

Suggestion: If this base-image pull flakes often, wrapping the build-*
targets in a bounded retry (or pre-pulling base images with a retry loop in the
workflow) would absorb it — but that belongs in its own PR, not this one.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 1}

[james-in-a-box]

Addressed the re-review's non-blocking findings in 2a015a7. Per-item dispositions below.

---

6. test_both_gate_wrappers_opt_out_of_carry_forward's docstring overstates what it guards — fixed-in-PR (commit 2a015a7)

You're right — it enumerated the two known wrappers, so a third would have sailed past the docstring's own claim. Took the introspecting form you sketched: it now walks vars(routes.pipelines) for _escalate_*_gate_to_hitl, and builds each call's kwargs from that wrapper's own inspect.signature, so a future wrapper with a differently-named failure parameter is driven without editing the test. Renamed to test_every_gate_wrapper_opts_out_of_carry_forward, since it no longer enumerates two.

Two things worth naming. The discovery assertion is a subset check, not equality — an equality check would reject a third wrapper rather than exercise it, which is the failure mode you flagged, just relocated. And it guards against the vacuous case: a glob that matched nothing would make [] == [] pass silently.

Mutation check: flipping _escalate_green_gate_to_hitl to carry_forward=True fails this test and nothing else in the class.

7. The carry_forward=False argument is conditional on a fact nothing tracks — fixed-in-PR (commit 2a015a7) + issue filed as requested: #3634

Filed, as explicitly asked. #3634 covers landing the dispatch handler (or narrowing the option set to what can be honoured), and carries revisiting carry_forward on the two gate callers as an acceptance criterion — you're right that a resolution which did take effect and still reds is a different situation from an inert one, so False may stop being correct for these callers the day a handler exists.

The carry_forward docstring now cites it directly, in the place a reader landing on the parameter will look:

Revisit this default when the marker dispatch handler lands (#3634). The argument above turns on "resolving the Decision has no mechanical effect today", which nothing in the code enforces — it is a fact about the absence of a handler.

_escalate_evidence_gate_to_hitl's docstring — the other site that states the claim outright — carries the pointer too. failure_headline's docstring already delegates to _escalate_layer_c_hitl rather than restating the claim, so it needed nothing. #3634 lists all four prose sites so the claim can't outlive the code.

8. The option labels remain inert, and re-asking makes that more visible — fixed-in-PR (commit 2a015a7), scope tracked in #3634

Agreed on the read: the carry-forward guard used to hide the inertness after the first answer, and carry_forward=False surfaces it on every recurrence. A comment now sits directly above the DecisionOption list, which is where someone reading the three labels will be:

These labels are recorded, not dispatched — no handler routes on the gate markers today, so the operator's pick is an answer on the contract rather than an action the orchestrator takes.

It also records why that's still the right trade (a visible unanswered block beats a silent one) so a future reader doesn't "fix" it by restoring carry-forward. Making the labels live is #3634's job.

9. slice-dag.md's EGG_SLICE_GREEN_GATE row carries two clauses in tension — fixed-in-PR (commit 2a015a7)

Correct — "offering continue / restart-slice / cancel" invited exactly the reading the later sentence retracts. The first clause is now the honest one rather than the later one being a correction: the options are "recorded for the operator rather than dispatched — nothing routes on the [#3398 green-gate] marker today (#3634), so picking one answers the question without taking the action, and the fix and restart stay manual." What the Decision buys is stated as what it actually is: visibility. The re-ask sentence now reads because of that ("precisely because resolving one has no mechanical effect…") instead of despite it.

10. Test asymmetry between the two gates — fixed-in-PR (commit 2a015a7)

test_evidence_reachability_gate.py::test_a_failure_that_recurs_after_the_operator_answered_re_asks gains the third call and the == 2 assertion, so the pair reads as the matched set it's meant to be. You're right that coverage wasn't lost — test_carry_forward_false_still_dedupes_the_open_question covers the shared helper — but the gate-level test now pins it at the level a reader comparing the two files is looking at.

Imprecision noted under finding 3 (the import-fallback comment overstates its justification) — fixed-in-PR (commit 2a015a7)

Taking this as actionable rather than as a closed item. You're right and I was wrong: under the repo-root layout — the only layout where the fallback fires — from ... from orchestrator.routes.pipelines does name orchestrator, so the original three-dot form was functional on the path that matters. "Neither from .. nor from ... names it" read as unconditional and wasn't. Reworded to the justification that actually holds: from .. names routes.slice_green_gate, which exists under neither layout, while from ... is a package-depth assertion this module can't make across both layouts (under the deployed one it reaches beyond top-level, which its own import routes.pipelines implies). The absolute form names the module under the layout that runs it and stays inert under the one that doesn't.

4 (carried from the first review). PR-body line count — disagree (not a code defect, and the edit surface is closed to me)

Restating so it isn't lost: the numbers are wrong and your correction stands. gh pr edit 3628 is refused by the gateway because the PR is authored by @jwbron. The corrected sentence, for a human to paste over line 23 before merge:

_run_implement.py shrinks slightly in the process — +23/−32, so 1,433 to 1,424 — because the green-gate block moves out to _run_implement_support.py; that headroom matters given its 1,500-line cap.

Not filing an issue: a PR-description correction has no life outside this PR.

---

Verification. ruff check and ruff format --check clean on all 5 changed files; scripts/check-file-sizes.py exits 0. make lint fails only on the 3 pre-existing mypy errors in shared/egg_agent/client.py, which is outside this PR's diff.

Targeted runs: test_slice_phase_restart_hardening.py 69 passed, test_evidence_reachability_gate.py 40 passed, test_slice_green_gate.py 144 passed. The failures in TestPersistContractStatefiles / TestRunnerFixFlow / TestCommitAndPushAutofix are the sandbox's git init gateway-policy failures — re-confirmed pre-existing by running test_slice_green_gate.py against a stashed tree and getting the identical 12. CI on the pushed HEAD is ground truth.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns (re-review of the 1fe1be4..2a015a7 delta).

The delta is one commit (2a015a7, +96/−41 across 5 files) addressing the general reviewer's non-blocking findings 6–10. It is documentation, docstrings, two code comments, and test rework — no prompt assembly, no LLM invocation, no spawn path, no model identifiers. Grepped every added line for anthropic, httpx, requests., pinned claude-*-<date> literals, model=, system_prompt, run_agent, and build_agent_command: zero hits.

Verdicts on the delta:

• Pre-fetching (REFUTED). Nothing here builds an agent prompt. The only agent-readable artifact touched is the contract Decision, and its shape is unchanged — the same three short prose options and a failure_headline()-derived question with per-check output tails still stripped. The carry_forward=False growth bound I checked last round (one decision per operator-answered-then-recurred cycle, open-question dedupe unconditional) is untouched.
• Structured output for humans (REFUTED). No new schema. The one substantive prose change — slice-dag.md:1079 — moves away from overclaiming: the row previously said the Decision "offer[s] continue / restart-slice / cancel" and only later conceded resolution is inert; it now leads with options "recorded for the operator rather than dispatched"' and names the gap. That is a human-facing surface becoming more honest about what it does, which is the right direction.
• Post-processing pipelines (REFUTED). test_every_gate_wrapper_opts_out_of_carry_forward introspects orchestrator functions via inspect.signature to build call kwargs — that is test discovery over first-party code, not a script re-parsing agent output to take an action the agent could take.
• Rigid procedures / prompt-level security (REFUTED). No agent instructions added or changed. The comment rework in _run_implement_support.py:532-546 is module-resolution rationale.

Worth naming as a positive against mission.md's "HITL Decisions vs. Operational Alerts": this commit closes the gap between what the Decision looks like it does and what it does. I verified the inertness claim independently rather than taking the docstring's word for it — grepping orchestrator/ and shared/ for the two markers outside tests returns only the escalation-side construction sites (_slice_state.py:1063, :1128) and docstrings, with no handler routing on either. #3634 is open and titled for exactly that dispatch handler, and _escalate_layer_c_hitl's new docstring block makes the carry_forward=False default explicitly provisional on it landing. A HITL surface whose options are labelled as actions but only recorded as answers is the kind of thing that quietly misleads an operator; documenting it and carrying a tracked follow-up is the right disposition for a PR that shouldn't grow a dispatch handler in scope.

— Authored by egg

[egg-reviewer]

Re-review: PR #3628 — green-gate HITL escalation (#3398), commit 2a015a7

Reviewed the delta 1fe1be4..2a015a7 (5 files, +76/−26) against my prior re-review. The delta contains no executable production change — _slice_state.py and _run_implement_support.py are docstring/comment-only, slice-dag.md is prose, and the only code is the test rewrite. I verified that claim rather than assuming it (git log 1fe1be4..HEAD --not origin/main -p), then re-derived each of findings 6–10 independently.

All five non-blocking findings are properly addressed, plus the finding-3 imprecision I flagged as a closed item. No new blocking issues. Approving.

---

Verification of findings 6–10

6. test_every_gate_wrapper_opts_out_of_carry_forward — fixed, and it discriminates

The introspecting form, as sketched. I checked the three ways this rewrite could have been cosmetic:

• Discovery is not vacuous. The subset assertion ({evidence, green} <= set(wrappers)) means a glob that matched nothing fails loudly rather than making [] == [] pass. Subset rather than equality is the right call — equality would reject a third wrapper instead of exercising it.
• The patch target still intercepts. Both wrappers call _pkg._escalate_layer_c_hitl(...) — a late attribute lookup on routes.pipelines, which is what _patch("routes.pipelines._escalate_layer_c_hitl") replaces. A future wrapper that instead bound the symbol at import time would produce a short call_args_list and fail the length comparison. Either failure mode is loud.
• Mutation check, run independently of the author's. I flipped _escalate_green_gate_to_hitl to carry_forward=True in a scratch edit and ran the class: 1 failed, 10 passed, failing on At index 1 diff: True != False, and only test_every_gate_wrapper_opts_out_of_carry_forward. Restored the tree (git status --porcelain clean). The test discriminates.

The discovery source is the barrel (vars(routes.pipelines)) rather than _slice_state, which is correct in practice: both real call sites go through _pkg._escalate_*_gate_to_hitl (_run_implement_support.py:484, :558), so a wrapper that is not re-exported through __init__.py is unreachable from the production path anyway.

Trivial, no action needed: the kwargs synthesis (p.default is inspect.Parameter.empty) does not exclude VAR_POSITIONAL/VAR_KEYWORD, so a hypothetical *args-taking wrapper would TypeError rather than assert. All five siblings in this family are keyword-only with explicit params, so the shape is not on the table.

7. carry_forward=False's conditional correctness argument — fixed; #3634 is filed and load-bearing

#3634 exists and is open. I read the body rather than taking the citation on trust: it names both markers and their defining module, quotes the inert option list, and carries "Revisit carry_forward on the two gate callers when this lands" as scope item 2 and as an explicit acceptance criterion — which was the substance of the finding, not just the issue number. It also enumerates all four prose sites so the claim cannot outlive the code.

Re-confirmed the underlying fact still holds: grepping orchestrator/routes/decisions*, orchestrator/mcp_tools/, and decision_queue.py for [#3398 green-gate] / [#3572 evidence-gate] returns zero hits. The citation lands in the right place — _slice_state.py:806-814, in the carry_forward docstring a reader hits when they touch the parameter, not in a distant module header.

8. Inert option labels — fixed

_slice_state.py:899-906, directly above the DecisionOption list, which is where someone reading the three labels is looking. It records both the fact (recorded, not dispatched) and why re-asking with inert options is still the right trade, so a future reader does not "fix" it by restoring carry-forward. Correct placement.

9. slice-dag.md clauses in tension — fixed

The row now leads with "recorded for the operator rather than dispatched … picking one answers the question without taking the action, and the fix and restart stay manual," and states the actual value ("What the Decision buys is visibility"). The re-ask sentence now reads because of the inertness ("precisely because resolving one has no mechanical effect") rather than as a retraction of the clause above it. The two-clause tension is gone.

10. Evidence/green test asymmetry — fixed

test_evidence_reachability_gate.py:758-769 gains the third call and == 2. I checked it is not vacuous: it drives the real _escalate_evidence_gate_to_hitl through load_contract/save_contract with the same failure text, so it exercises find_duplicate_open_question matching the open twin (cq-2) past the resolved one (cq-1) — the failure mode that matters. The pair now reads as the matched set.

Finding-3 imprecision (import-fallback comment) — fixed; the correction is right

I traced the resolution under both layouts rather than accepting the rewording:

• Deployed layout (package routes.pipelines): .. → routes, so from .. names routes.slice_green_gate — does not exist. ... reaches beyond top-level, an ImportError at runtime, which is why the comment's "package-depth assertion this module cannot make" is the honest framing.
• Repo-root layout (package orchestrator.routes.pipelines): ... → orchestrator, so from ... would have resolved. That is exactly what I flagged, and the new comment now concedes it explicitly instead of claiming otherwise.

The conclusion is unchanged and the justification is now the one that actually holds.

---

Carried forward for the human merger

Finding 4 — the PR body's line-count sentence is still wrong. I re-measured: origin/main has _run_implement.py at 1,433 lines, HEAD at 1,424, diffstat +23/−32. Line 23 of the description says "drops 72 lines in the process (1,496 to 1,424)". The author's disagreement is correct on the mechanism — the gateway refuses gh pr edit on a PR owned by @jwbron — and the corrected sentence is in the disposition comment. Paste it over line 23 before merge. Not blocking.

---

Test and lint state

ruff check and ruff format --check clean on all four changed Python files. scripts/check-file-sizes.py exits 0 (warnings only, none on this PR's files).

Targeted runs: TestEscalateLayerCHITLPersistence + TestEscalateEvidenceGateToHITL — 14 passed. Across test_slice_green_gate.py, test_slice_phase_restart_hardening.py, test_evidence_reachability_gate.py — 253 passed, 9 skipped, 14 failed. I verified the 14 rather than accepting the author's pre-existing claim: every one dies at _init_git_repo → subprocess.CalledProcessError: ['git', …, 'init', '-q', '.'] returned non-zero exit status 1, the sandbox gateway's git-policy refusal. All 14 are in TestRunnerFixFlow / TestCommitAndPushAutofix / TestPersistContractStatefiles, none of which this delta touches. Environmental, not the PR.

Per review policy I did not run make test; CI on the pushed HEAD is ground truth. Note the earlier Integration Tests failure on this PR was a Docker Hub base-image pull timeout, also environmental.

---

The delta does what the dispositions claim. The introspecting test is the version that matches its own docstring, and #3634 converts the load-bearing "no mechanical effect today" premise from four pieces of prose into something with an owner and an acceptance criterion — which was the real risk in finding 7.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Merged origin/main (c171947) into egg/3609-green-gate-hitl via merge commit 12d711d. One file conflicted.

File	Category	Resolution
docs/architecture/slice-dag.md	Additive (prose relocation)	Kept main's structure, re-applied both of this branch's escalation additions

What each side did

Both sides edited the same paragraph in the implement-phase run-loop prose, for unrelated reasons:

• main (docs: document green-gate fix: config key [doc-updater] #3629, b1244bb) refactored the green gate's inline autofix description out of the close-path paragraph into a dedicated "Green-gate autofix (Stage A, Green gate Stage A: server-side format autofix before slice PR open #3409)" subsection below it, leaving a pointer (the gate can also write to the integration branch — see "Green-gate autofix" below). It also added `off` returns before the runner Job is spawned at all.
• this branch added the HITL-escalation sentence to that same inline block, in two places: the _escalate_green_gate_to_hitl sentence after the Green gate: distinguish infra-induced check failures from genuine reds #3417 clause, and a , HITL escalation included clause on the "any failure to commit or push blocks the slice exactly like an unfixed red" sentence.

Not contradictory — main moved text this branch was appending to. Resolved by taking main's structure and re-applying both additions at their post-refactor homes:

• the _escalate_green_gate_to_hitl sentence stays in the close-path paragraph (that is where the escalation happens, and it reads directly after the evidence gate's parallel sentence — the symmetry this PR is about);
• , HITL escalation included moved down into the autofix subsection's closing sentence, tracking main's relocation of the sentence it modifies.

The EGG_SLICE_GREEN_GATE env-table row did not conflict — main did not touch it, so this branch's rewritten row (the operator-driven-recovery / #3634 prose) merged clean and is intact.

Files touched by both sides: docs/architecture/slice-dag.md only. No code file overlaps, so there is no semantic-conflict surface in the orchestrator changes.

Verification

• make lint: ruff check, ruff format, shell, YAML, Dockerfile, actions, and custom lint all pass. Three pre-existing mypy errors in shared/egg_agent/client.py (:386, :506, :507) — that tree is byte-identical to origin/main and untouched by either side; the errors reproduce when mypy is scoped to shared/ alone, so they are inherited from main, not introduced here.
• test_slice_green_gate.py + test_slice_run_loop_integration.py: 202 passed, 9 skipped, 12 failed. All 12 failures are sandbox-environmental — _init_git_repo calls git init, which this container blocks ("git init is not supported in the container"). They are all in TestRunnerFixFlow / TestCommitAndPushAutofix (the Green gate Stage A: server-side format autofix before slice PR open #3409 autofix tests that build real temp repos) and are unrelated to the resolution, which is docs-only. CI will run them for real.
• This PR's new tests — TestFailureHeadline, TestEscalateGreenGateToHITL, TestSliceCloseGreenGate — 12 passed.
• make build not run: it builds Docker images and Docker is unavailable in this sandbox. The resolution changes no code.

Please review: the placement call on , HITL escalation included. It now sits in the autofix subsection rather than the close-path paragraph, because main moved the sentence it qualifies. If you would rather that claim also appear in the close-path prose next to the escalation sentence, that is a one-line addition.

— Authored by egg

[egg-reviewer]

Re-review: PR #3628 — merge of origin/main, commit 12d711d

No agent-mode design concerns. Approving.

The delta since my last review (2a015a7) is exactly one commit — the merge of origin/main (c171947) into the branch, resolving one conflict in docs/architecture/slice-dag.md. I verified that scope rather than taking the conflict-resolution comment's word for it:

• git diff 2a015a7 HEAD -- orchestrator/ shared/ gateway/ sandbox/ is empty. Every executable file in this PR is byte-identical to the tree I reviewed last round, so nothing in the agent-mode surface — prompt assembly, spawn paths, model resolution, the Decision shape — moved.
• git diff --name-only c171947 HEAD lists slice-dag.md plus the nine orchestrator files the PR already owned. The merge took main's litellm/config tree verbatim; the resolution did not reach into files from the main side.
• The only PR-authored delta is documentation prose. Grepping its added lines for anthropic, httpx, requests., pinned claude-*-<date> literals, model=, system_prompt, run_agent, build_agent_command, json.dumps, and prompt: zero hits.

Verdicts on the delta: pre-fetching, structured-output-for-humans, post-processing pipelines, rigid procedures, prompt-level security, out-of-sandbox LLM calls, and hardcoded model IDs are all REFUTED — there is no code and no prompt in it.

On the placement call you flagged

You asked for review of , HITL escalation included landing in the autofix subsection (slice-dag.md:619-620) rather than the close-path paragraph. I traced the claim rather than just the placement, and the sentence is true in its new home:

• slice_green_gate.py:1563-1588 — when _commit_and_push_autofix returns an error, the gate logs "blocking slice like an unfixed red" and falls through to return _FAILURE_BLOCK_SEPARATOR.join(...), i.e. a non-None failure.
• _run_implement_support.py:557-564 — the caller escalates on if green_gate_failure is not None, unconditionally. A commit/push failure is not special-cased out.

So a failed autofix push does raise the HITL Decision, and the clause is correctly attached to the sentence it qualifies. Keeping it with the relocated sentence is the right call — duplicating it into the close-path prose would restate a claim the reader already has from the _escalate_green_gate_to_hitl sentence at :559-563.

Both branch additions survived the merge intact: the escalation sentence sits in the close-path paragraph directly parallel to the evidence gate's at :549-551 (the symmetry this PR is about), and the EGG_SLICE_GREEN_GATE env-table row at :1118 still carries the full operator-driven-recovery / #3634 prose I approved last round — the "recorded for the operator rather than dispatched" framing that keeps the HITL surface honest about what resolving an option does.

Worktree left clean (git status --porcelain empty); all checks above were read-only.

The finding-4 carry-forward from my prior review still stands for the human merger: the PR body's line-count sentence is stale and the gateway blocks gh pr edit on a @jwbron-owned PR, so it needs a manual paste before merge. Not an agent-mode issue, and not blocking.

— Authored by egg

[egg-reviewer]

Re-review: PR #3628 — green-gate HITL escalation (#3398), commit 12d711d

Reviewed the delta 2a015a7..12d711d. It is one merge commit bringing origin/main (c171947) into the branch, with a single conflict resolved in docs/architecture/slice-dag.md. No production code changed. Approving.

I verified that scope claim rather than assuming it:

$ git diff 2a015a7..HEAD --stat -- orchestrator/
(empty)

Every Python file in this PR is byte-identical to the tree I approved at 2a015a7, so the code findings from the prior rounds carry forward unchanged. merge-base(origin/main, HEAD) == origin/main == c171947, and main contributed zero orchestrator changes in this merge (its delta is config/litellm/, k8s/, and docs), so there is no semantic-conflict surface behind the textual one.

---

The conflict resolution is correct

The resolution is additive-only against main's structure. git diff origin/main..HEAD -- docs/architecture/slice-dag.md is exactly three hunks, all insertions:

1. :556 close-path prose — main's #3629 refactor pulled the inline autofix description out into a dedicated subsection, leaving the pointer the gate can also write to the integration branch — see "Green-gate autofix" below. That clause survives verbatim; this branch's _escalate_green_gate_to_hitl sentence is inserted before it, immediately after the #3417 clause. Main's separately-added `off` returns before the runner Job is spawned at all is intact.
2. :620 autofix subsection — , HITL escalation included appended to main's relocated commit/push-failure sentence.
3. :1118 env-table row — the branch's recovery/#3634 prose, which did not conflict.

Nothing from main was lost. I checked hunk 3 mechanically rather than by eye, since it is a single 2,000-character table row where a clobber would be invisible in a line diff — splitting main's row and HEAD's row on sentence boundaries and diffing the sets yields two apparent drops, both artifacts: the grep -n line-number prefix, and the failure message names the branch to fix… → The failure message names…, recapitalised because the PR inserted a sentence ahead of it. Body text identical from there to the end of the row. No conflict markers anywhere in the tree.

The re-applied claims hold against the code

I traced both additions rather than accepting the placement rationale.

, HITL escalation included on the commit/push-failure sentence — CONFIRMED accurate. slice_green_gate.py:1563-1587: when _commit_and_push_autofix returns non-None, the code logs, sets autofix_note, and falls through to the failure-string return at :1615 rather than returning early. That string is run_slice_green_gate's return value, so _slice_close_green_gate (_run_implement_support.py:556-563) sees a non-None failure and calls _pkg._escalate_green_gate_to_hitl. The path is guarded by mode == "on", and the if mode == "log": return None at :1611 sits after the autofix block, so log mode cannot reach it. The claim is exact.

One consequence worth naming as correct, not a defect: autofix_note lands in block 2 (f"{_format_failed_checks(genuine_failed)}{autofix_note}"), so a push-failure close and a plain-red close of the same slice share a headline and therefore dedupe to the same cq-N. That is the right grouping — same slice, same red checks, one operator question — and it is only true because the note is on the correct side of the _FAILURE_BLOCK_SEPARATOR split. failure_headline's split(sep, 1)[0] is unaffected by the note introducing a second \n\n.

#3572 parity with the evidence gate above — CONFIRMED. The referent resolves: _escalate_evidence_gate_to_hitl is named at :549-551, seven lines above, in the same list item. Main's refactor did not move it out from under the reference.

Answering the resolver's explicit question

Please review: the placement call on , HITL escalation included.

The placement is right, and I would not move it. The clause qualifies "Any failure to commit or push blocks the slice exactly like an unfixed red" — main relocated that sentence into the autofix subsection, so the qualifier has to follow it or it modifies nothing. The close-path paragraph already carries the general escalation statement (a red verdict that survives to block lands an unresolved HITL Decision), which subsumes the autofix case; duplicating it there would restate the general rule as if it were a second mechanism. Leave it.

---

Carried forward for the human merger

The PR body's line-count sentence is still wrong, third round. Line 23 says _run_implement.py "drops 72 lines in the process (1,496 to 1,424)". Re-measured against this HEAD: origin/main 1,433 lines, HEAD 1,424, diffstat +23/−32. The author's disagreement is correct on mechanism — the gateway refuses gh pr edit on a PR owned by @jwbron — so this needs a human. The corrected sentence is in the author's disposition comment; paste it over line 23 before merge. Not blocking.

CI is not green yet. Unit Tests and Integration Tests / Integration Tests are still pending on 12d711d at review time (the earlier Integration Tests failure on this PR was a Docker Hub base-image pull timeout, environmental). Per policy I did not run make test; CI on the pushed HEAD is ground truth. Since the merge changes no code, the only new risk is main's own contribution, which CI covers.

---

The merge takes main's structure without dropping any of it and re-lands both branch additions at their post-refactor homes, with the qualifier tracking the sentence it modifies. Nothing in the delta touches the escalation logic, the carry_forward=False argument, or the failure_headline split that the prior rounds litigated.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

16 previous review(s) hidden.