feat(orchestrator): validate producer draft at canonical path on propose (#3016)

[jwbron]

What

Make the refine/plan phase-gate input deterministic: validate at consensus_propose that the producer's canonical phase draft exists at the path every downstream reader uses, and reject (400) when it is missing — so a producer that commits its draft off-path can no longer silently complete the phase and leave the operator gate reporting "No analysis draft was found on the work branch."

Closes #3016.

Why

A refiner committed its analysis to .egg-state/agent-outputs/refiner-refine.md — a path no orchestrator/sandbox code reads. The canonical path everything reads (gate _read_phase_draft, pipeline.analysis repopulation, contract population) is .egg-state/drafts/{issue}-analysis.md. Result: BRC consensus CONFIRMed, the phase completed, and only then did the gate come up empty. This is not caught by the read-side hardening in #2992 / #2723 — a draft at a path nobody reads is invisible to git show HEAD:.egg-state/drafts/… too.

refine / plan are always concurrent, so every producer issues consensus_propose — a universal choke point. The orchestrator already validates proposals there (_validate_tester_check_coverage, _validate_planner_role_alignment) before handle_propose records them; _validate_planner_role_alignment even already git shows the plan draft at the proposed commit — but treated a missing draft as "nothing to validate" and passed. This PR closes that hole and adds the equivalent guard for refine (which had none).

How

• New _validate_producer_draft_present(phase, …) in orchestrator/routes/signals.py: runs git show {commit}:{_get_draft_path(phase, …)} against the orchestrator worktree at the proposed commit; raises ValueError (→ 400) when the draft is absent or empty, with an actionable message naming the canonical path. Existence-only — format/template conformance is deliberately left to reviewers and the gate, to avoid false-rejecting a legitimate-but-differently-structured draft (see Genericize the SDLC pipeline into a declarative, config-driven workflow (custom phases + agents + per-phase slicing) #3017).
• Wired into handle_consensus_propose_signal before handle_propose, for refiner (phase refine) and task_planner (phase plan, alongside the existing role-alignment check). task_planner is the canonical plan-draft owner — architect/risk_analyst write agent-outputs JSON; _synthesize_plan_draft's docstring confirms task_planner writes drafts/{id}-plan.md directly.
• Graceful degradation preserved: no commit SHA, no branch, an underivable draft path, or a git/infra error all skip silently. It rejects only on a positively-confirmed-absent (or empty) draft at a branch-verified commit — _verify_commit_on_branch (which fetches the commit) runs first, so a non-zero git show here reliably means "path absent at commit", not "commit unknown".

Scope notes

• Orchestrator-only by design. The optional sandbox-side pre-flight mirror in brc_propose (issue feat(orchestrator): validate producer's phase draft at the canonical path on propose (deterministic refine/plan gate input) #3016 item 3) is a latency optimization, not correctness — the agent already receives the orchestrator's actionable 400 (surfaced by brc_propose's GatewayError) and re-proposes. Deferred to keep this focused.
• Structurally subsumed by Genericize the SDLC pipeline into a declarative, config-driven workflow (custom phases + agents + per-phase slicing) #3017 (declarative phase abstraction): once a phase declares its output-artifact spec in one place, this guard reads from that declaration instead of a hardcoded role→phase mapping.

Test

• New TestProducerDraftPresentValidation (8 cases): missing-SHA skip, present / absent / empty draft, plan-vs-analysis message wording, infra-error graceful skip, no-branch skip, and an end-to-end handle_consensus_propose_signal refiner-reject asserting the tracker is not mutated.
• Updated the existing task_planner integration test for the extra presence-check git show call now in its path.
• make test (changeset-aware) + make lint green.

[egg-reviewer]

Reviewed the diff and traced the full propose → presence-check → tracker mutation path. The PR closes a real bug: a producer that committed the draft to a non-canonical path (the actual .egg-state/agent-outputs/refiner-refine.md example) used to CONFIRM consensus and complete the phase, leaving the operator gate to report an empty draft a phase later. Validating at CONSENSUS_PROPOSE against the path every downstream reader (_get_draft_path) uses is the right choke point, and placing the check before handle_propose correctly preserves the "rejected proposal never mutates the tracker" invariant established for _validate_tester_check_coverage (#1459) and _validate_planner_role_alignment (#2527).

No blocking issues. A few non-blocking concerns and minor parity notes below.

Non-blocking

1. The "non-zero git show reliably means path absent at commit" claim breaks when _verify_commit_on_branch returns None

orchestrator/routes/signals.py:1127-1129 (docstring) and the corresponding behavior at signals.py:1191-1202 assume the prior _verify_commit_on_branch call has fetched the commit. But _verify_commit_on_branch returns None (non-blocking) on either a failed git fetch origin -- {branch} (signals.py:378-385) or a failed git branch -r --contains (signals.py:404-411). In handle_consensus_propose_signal:1365-1395, that None is silently discarded — only False short-circuits with a 409.

If the fetch failed and the commit isn't already in the worktree's local object cache, git show {commit}:{draft_rel} exits with returncode 128 and stderr fatal: invalid object name / fatal: bad revision. The new validator treats this the same as a clean "path absent at commit" and raises a Refine proposal rejected: no analysis draft found at … 400. The producer's error message will incorrectly accuse them of a path problem when the orchestrator simply couldn't reach the remote.

Self-healing on re-propose is plausible (the next attempt's fetch may succeed), but the misleading 400 burns an agent turn and produces a noisy log on a transient orchestrator-side failure. Two ways to address:

• Thread the result through: have handle_consensus_propose_signal capture branch_verified and pass it into _validate_producer_draft_present. When branch_verified is None, skip the presence check (same posture as the existing "could not verify branch" warning — don't penalize the producer for an orchestrator-side glitch).
• Distinguish the two failure modes via stderr inspection: parse result.stderr for bad revision / invalid object name and silently degrade in that case, while still rejecting on path 'X' does not exist in 'COMMIT'.

The first option is the minimal change and matches the existing graceful-degradation philosophy elsewhere in the function.

2. _validate_planner_role_alignment's "plan not present → silent skip" is now unreachable via the production handler path

signals.py:1035-1036 (pre-existing) returns silently when git show of the plan draft is non-zero — that branch is the original bug surface this PR closes. With the new presence guard chained in front of it at signals.py:1420-1428, any task_planner proposal whose git show returns non-zero is rejected by _validate_producer_draft_present before reaching the alignment validator, so the silent-skip branch in alignment is dead code on the production path. It's still reachable when alignment is called directly (tests), but the two validators now have opposite responses to the same returncode != 0 signal:

• _validate_producer_draft_present: reject (this PR)
• _validate_planner_role_alignment: silent pass

A future refactor that re-orders, parallelizes, or extracts these guards could quietly re-introduce the #3016 bug by losing the presence-check precedence. Either tightening alignment to raise (consistent with the new posture) or adding a one-line comment at signals.py:1035 noting "presence is now enforced upstream by _validate_producer_draft_present" would protect the invariant.

3. Minor test-parity gaps

The new TestProducerDraftPresentValidation class is missing two cases the analogous TestPlannerRoleAlignmentValidation class has:

• No test_accepts_when_plan_draft_present (only refine — test_pipeline_prompts.py:2493). The end-to-end test_rejected_proposal_does_not_mutate_tracker exercises the plan-present path indirectly via call 3 returning non-empty stdout (test_pipeline_prompts.py:2403-2408), but a direct unit test would lock the symmetry.
• No test_skips_when_pipeline_lookup_fails (the planner suite has it at test_pipeline_prompts.py:2338). The except StateStoreError: return branch at signals.py:1142-1144 is uncovered.

4. Stale comment in adjacent code (pre-existing, easy drive-by)

signals.py:1004 says "16k-line routes.pipelines" — the file is now 24,348 lines (the new function's comment at signals.py:1149-1150 correctly says "21k-line"). Worth aligning the two while you're touching this region.

What I verified

• Scoped to refiner (sole refine producer) and task_planner (canonical plan-draft owner). architect / risk_analyst write JSON to .egg-state/agent-outputs/ and don't own the canonical plan path — confirmed against agent_roles.py:362-455 and the _synthesize_plan_draft docstring at pipelines.py:18603-18617. Out-of-scope by design, matches the PR description.
• _get_draft_path("refine") → .egg-state/drafts/{prefix}-analysis.md, _get_draft_path("plan") → .egg-state/drafts/{prefix}-plan.md, _get_draft_path("implement") → None (pipelines.py:5152-5179). The defensive if not draft_rel: return covers the implement case if the function is ever called with a non-refine/non-plan phase.
• The integration test's subprocess side_effect list is correctly extended from 3 → 4 entries with calls 3 and 4 both returning the misassigned plan, so the presence guard passes and the alignment guard raises (test_pipeline_prompts.py:2395-2415). Matches the production sequence.
• All CI checks green at the time of review.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. All four items addressed in b89fe0f.

Per-item disposition

1. _verify_commit_on_branch → None mis-blames the producer — fixed-in-PR (commit b89fe0f). Took option 1 (thread the result through) per your recommendation — it matches the existing graceful-degradation philosophy and avoids brittle stderr parsing. handle_consensus_propose_signal now captures the tri-state from _verify_commit_on_branch and threads it into _validate_producer_draft_present via a new branch_verified: bool | None = True kwarg. On None, the validator returns before touching git so a "bad revision" on a missing commit can't be mistaken for an absent draft. The handler's except block (verifier raised) also degrades the value to None, matching the existing "could not verify branch (non-blocking)" warning posture. Default is True so direct unit-test callers (which don't run _verify_commit_on_branch) still get the check.

2. Planner alignment's now-unreachable silent-skip — fixed-in-PR (commit b89fe0f). Went with your suggested one-line-comment approach (signals.py:1036-1046) — tightening alignment to raise would expand scope into the pre-existing #2527 guard. The comment names the upstream guard, explains why the two responses to returncode != 0 differ, and explicitly flags that any refactor moving the alignment guard ahead of the presence guard would silently re-introduce #3016. The invariant is now documented at the point of risk.

3. Test-parity gaps — fixed-in-PR (commit b89fe0f). Added both:

• test_accepts_when_plan_draft_present — direct unit confirmation of the plan-present path that previously was only exercised indirectly via the end-to-end planner test (test_pipeline_prompts.py:2403-2408).
• test_skips_when_pipeline_lookup_fails — covers the except StateStoreError: return branch.
• Bonus: test_skips_when_branch_verified_is_none as a regression guard for item 1, asserting subprocess.run is not called when verification is inconclusive.

All 11 cases in TestProducerDraftPresentValidation pass; existing TestPlannerRoleAlignmentValidation cases still pass.

4. Stale 16k-line comment — fixed-in-PR (commit b89fe0f). signals.py:1004 updated 16k-line → ~24k-line (actual: 24,348 per wc -l). The newly-added function's comment was ~21k-line — also bumped to ~24k-line so both descriptors are now accurate and aligned with each other.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta between 1ba8611 and b89fe0f. All four items from the previous review are addressed correctly and surgically. Walked the threading through every edge case in handle_consensus_propose_signal; the tri-state lands at _validate_producer_draft_present consistently.

Per-item verification

1. branch_verified tri-state threading — signals.py:1108, 1165-1170, 1404, 1411, 1439, 1459, 1473. The validator short-circuits on branch_verified is None before the state-store load (signals.py:1169-1170 → 1172-1176), so a _verify_commit_on_branch glitch doesn't even attempt a second load_pipeline. Default of True keeps direct unit-test callers strict. The outer except Exception block at signals.py:1429-1439 correctly degrades to None for raises from _verify_commit_on_branch, load_pipeline, or resolve_worktree_path — all three are "orchestrator-side glitch, not producer fault" surfaces and skipping the presence check matches the existing "Could not verify commit on branch (non-blocking)" warning posture. Traced six edge cases (missing SHA, empty branch, True/False/None from verify, load_pipeline raises) — all produce the right behavior.

2. Planner alignment comment — signals.py:1036-1046. The comment explicitly names _validate_producer_draft_present as the upstream guard, explains why the two validators respond differently to the same returncode != 0 signal, and flags the refactor-risk that would re-introduce #3016. This is exactly the protection the invariant needed.

3. Test parity — three new tests at test_pipeline_prompts.py:2548-2617. test_accepts_when_plan_draft_present locks the symmetry. test_skips_when_pipeline_lookup_fails uses StateValidationError which is a StateStoreError subclass (state_store.py:73) — valid; covers the previously-uncovered except StateStoreError: return branch. test_skips_when_branch_verified_is_none is the right shape: mock_run.assert_not_called() confirms the validator short-circuits before git access, which is what the threading is supposed to guarantee. All 11 cases in TestProducerDraftPresentValidation pass locally; the 7 TestPlannerRoleAlignmentValidation cases still pass after the comment update.

4. Stale 16k-line comments — both signals.py:1004 and signals.py:1181 now say ~24k-line, aligned with each other and with wc -l reality. Drive-by fixed.

What I verified

• The threading edge cases above (six traced).
• _validate_planner_role_alignment is intentionally not threaded with branch_verified — its pre-existing returncode != 0 → silent skip already absorbs the same orchestrator-glitch scenario without false-rejecting, so there's no regression here, just an asymmetry the new comment correctly documents.
• The integration test test_refiner_proposal_rejected_when_analysis_missing_does_not_mutate_tracker (test_pipeline_prompts.py:2653-2701) still passes with its three-call subprocess sequence (fetch=0, contains=origin/branch, show=128). The branch-verified path through the handler is still True here, so the strict check fires as intended.
• All CI checks green at HEAD.

Non-blocking

1. No end-to-end test for the branch_verified=None threading path

The new test_skips_when_branch_verified_is_none exercises the validator directly with branch_verified=None. There's no integration-level test that mocks _verify_commit_on_branch to return None (via a fetch-failure subprocess sequence: returncode=1 on call 1) and asserts the producer's proposal reaches tracker.handle_propose instead of being 400'd. The threading is small and obvious enough that I don't think this is required for merge, but it would lock the wiring in the same way test_refiner_proposal_rejected_when_analysis_missing_does_not_mutate_tracker locks the rejection wiring. Worth considering for a follow-up.

2. Docstring redundancy at signals.py:1135-1141

The pre-existing "Graceful degradation" paragraph lists four skip conditions (no SHA, no branch, no path, infrastructure error on git show). The new branch_verified paragraph at 1148-1159 adds the fifth condition. The first paragraph's phrase "raises only when it can positively confirm the draft is absent (or empty) at a resolved, branch-verified commit" technically already excludes the new None case, but a reader scanning top-down might miss the dependency on the later paragraph. A single sentence on the first paragraph ("and when branch verification was inconclusive — see below") would tie them together. Minor wording nit.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Both non-blocking items addressed in 8900d12.

Per-item disposition

1. No end-to-end test for the branch_verified=None threading path — fixed-in-PR (commit 8900d12). Added test_refiner_proposal_accepted_when_branch_verification_inconclusive (test_pipeline_prompts.py:2653-2720). Mocks the fetch-failure subprocess sequence (returncode=1 on call 1, stderr fatal: unable to access remote: connection reset), so _verify_commit_on_branch returns None. Asserts (a) status_code == 200, (b) tracker.handle_propose.assert_called_once(), and (c) mock_run.call_count == 1 — confirming the validator short-circuited before the presence git show could fire and false-reject. Mirrors test_refiner_proposal_rejected_when_analysis_missing_does_not_mutate_tracker for the accept-on-glitch path. The test needed two extra mocks beyond the patterns the existing end-to-end test uses: current_phase.value on the pipeline mock (the success path emits a CONSENSUS_PROPOSE message whose phase field is Pydantic-validated; the existing test reaches the error path before message emission) and message_store.get_message_store (so the message-bus write hits a mock, not the real store).

2. Docstring redundancy at signals.py:1135-1141 — fixed-in-PR (commit 8900d12). The "Graceful degradation" paragraph (now signals.py:1135-1143) gets a fifth skip condition inline — "or branch verification was inconclusive (branch_verified is None — see below)" — so a top-down reader sees all five conditions in one place rather than having to scan ahead. The later branch_verified paragraph still carries the full rationale (why None skips); the first paragraph now just signals its existence and points at it.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta between b89fe0f and 8900d12 (the single new commit). Both non-blocking items from my second review are addressed correctly and surgically. Approving.

Per-item verification

1. End-to-end test for branch_verified=None threading — test_pipeline_prompts.py:2653-2735. The test models the right failure mode (fetch returncode=1, stderr fatal: unable to access remote: connection reset) and asserts the three guarantees that matter:

• status_code == 200 — the proposal isn't 400'd by the presence validator.
• mock_tracker.handle_propose.assert_called_once() — the proposal made it past both validators into the tracker.
• mock_run.call_count == 1 — confirms the validator short-circuits before calling git show. This is the threading guarantee under test: if the branch_verified kwarg were dropped from the call site at signals.py:1461, the validator would default to True, run git show as call #2, and the side_effect list (length 1) would raise StopIteration → the outer except returns 500. So the test does fail closed if the wiring breaks.

The two extra mocks beyond _patched_store (the explicit mock_pipeline.current_phase.value = "refine" and the message_store.get_message_store patch) are both required and the inline comment correctly justifies them — the rejection-path end-to-end test reaches the 400 before message emission, so it doesn't need them. Mirror with test_refiner_proposal_rejected_when_analysis_missing_does_not_mutate_tracker (3-call subprocess sequence vs. 1-call, accept vs. reject) is exactly the symmetric pair the wiring needs.

2. Docstring tie-in — signals.py:1135-1143. The "Graceful degradation" paragraph now lists all five skip conditions inline (no commit SHA, no branch, no draft path, git show infrastructure error, branch_verified is None — see below), with the explicit forward-pointer to the dedicated branch_verified paragraph at signals.py:1150-1161. Top-down readers no longer have to scan ahead to discover the fifth condition. Minor wording nit resolved.

What I verified

• Traced the success path through handle_consensus_propose_signal for the new test: _verify_commit_on_branch returns None (fetch returncode=1 → signals.py:378-385); branch_verified lands at _validate_producer_draft_present as None; validator returns at line 1172 before any state-store load or git call; handle_propose is invoked; result = {"version": 1} is dict-safe for the empty .get("stale_reviewers", []) / .get("newly_ready", []) / .get("status") lookups downstream; _resolve_pipeline_phase reads mock_pipeline.current_phase.value == "refine" (Pydantic-string-valid); make_success_response returns 200.
• The single subprocess call in the test (fetch only) is the correct minimum — if the validator were called with default branch_verified=True, the unprovided second subprocess.run call would StopIteration and the assertion mock_run.call_count == 1 would fail.
• No symmetric test for the task_planner path through _validate_producer_draft_present with branch_verified=None, but the threading is identical (same call site, same kwarg, same shared validator code), and the previous review explicitly didn't ask for it. Not worth adding.
• All CI checks green at HEAD (8900d12).

Non-blocking

None. This is a clean follow-up to the second review's non-blocking notes.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.