fix(orchestrator): consolidate plan propose-time validation, catch missing yaml-tasks (#3026)

[jwbron]

Summary

Fixes #3026. A plan draft that is complete in prose but omits its machine-readable # yaml-tasks appendix passed BRC plan consensus and phase completion, then failed the whole pipeline ~40 min later at populate_contract (parse_failed, empty contract.slices) — bypassing the plan HITL gate and surfacing an expensive recovery decision instead of a cheap NACK→re-propose.

Root cause — stacked propose-time guards

For a task_planner consensus_propose, the handler ran two validators that each git show-ed the same plan draft at the same commit (test_pipeline_prompts.py even documented this as calls #3 and #4):

• _validate_producer_draft_present (feat(orchestrator): validate producer's phase draft at the canonical path on propose (deterministic refine/plan gate input) #3016) — existence only → the 845-line prose draft exists, so it passed.
• _validate_planner_role_alignment (Validate task role↔file alignment at plan time, not push time #2527) — already called the populator's parse_plan, but did if not parsed.success: return (silent). That success=False branch is exactly the missing-appendix case — the parse result was thrown away.

So the parse the issue asks for already ran at propose-time; its negative result was just discarded. And the second git show was redundant (a draft that parses to ≥1 slice is necessarily present and non-empty). The #3019 author had already flagged the tension in a comment: the two guards "have opposite responses to the same returncode != 0 signal."

Change

Consolidate the two into a single _validate_plan_proposal that does one git show + one parse_plan and asserts, in order:

1. Presence (feat(orchestrator): validate producer's phase draft at the canonical path on propose (deterministic refine/plan gate input) #3016) — draft exists and is non-empty at the proposed commit.
2. Parseability / ≥1 slice (Plan propose-time validation is existence-only — a draft missing its yaml-tasks appendix passes consensus, then fails the whole pipeline at populate #3026, the fix) — reuses the same parser populate_contract runs, and mirrors its fail-fast condition (_populate_result_is_empty_contract = non-success or slice_count == 0), so the propose-time check and the populate check cannot diverge.
3. Role↔files alignment (Validate task role↔file alignment at plan time, not push time #2527).

_validate_producer_draft_present stays as the refine presence guard (analysis drafts have no parseable appendix, so existence-only is correct there). This removes the duplicate git show, closes the silent-pass hole, and eliminates the "don't reorder these or you reintroduce #3016" footgun.

Result: a fence-less plan is NACKed at propose-time (the still-alive planner re-proposes with the appendix) — before consensus, before phase completion, before the HITL gate.

Tests

• TestPlannerRoleAlignmentValidation → TestPlanProposalValidation; the end-to-end test's subprocess sequence drops from 4 calls to 3 (single read).
• New: fence-less plan raises (#3026 regression, unit + end-to-end), present-but-empty raises, absent raises, branch_verified=None / timeout graceful-skip preserved.
• TestProducerDraftPresentValidation keeps its refine cases; the two plan cases moved into the plan-proposal suite.

Both affected classes pass (22 passed). Out of scope (per the issue): the populate parse_failed recovery path is unchanged; no reviewer_plan checklist item added — the deterministic propose-gate is the mechanism that should fire.

[egg-reviewer]

Review

The consolidation is the right call. The old propose-time path had two stacked validators that each git show-ed the same draft and disagreed on how to treat a missing one (#3016 raised, #2527 silent-skipped), with _validate_planner_role_alignment already calling parse_plan and throwing the negative result away on the exact case #3026 wants caught. Folding presence + parseability + role-alignment into one read closes the silent-pass hole, drops a redundant git show, and eliminates the "don't reorder these" footgun.

I verified the parser behavior directly: _PLAN_WITHOUT_YAML_TASKS does produce success=False ("No tasks or phases found"), and _PLAN_WITH_MISASSIGNED_TASK does produce the expected role-alignment error (coder → docs/fixtures.md blocked, reassign to documenter). The end-to-end tests pin the right tracker-mutation invariant.

CI is green except the in-flight egg-review job.

A few non-blocking notes below — I think this is good to merge as-is.

Non-blocking suggestions

1. if not slices: after parsed.success is dead code (signals.py:1125-1132).

parse_plan returns success=False whenever phases is empty (plan_parser.py:1399-1406), and adds a placeholder task to every empty phase before that check (plan_parser.py:1379-1396). So success=True ⇒ phases non-empty ⇒ to_contract_slices() non-empty by construction. I confirmed this — even a plan with slices: [] in the yaml fence returns success=False, and a plan with only a ## Phase 1: header returns success=True with one placeholder slice. The branch can't fire via the parser's contract. Either drop it, or relabel the error to clarify it's a Pydantic-construction backstop (the message currently reads as if the parser can legitimately produce zero slices on success=True, which it can't). The docstring's "(2) Parseability / ≥1 slice" phrasing has the same ambiguity — there's really only one check there: parseability.

2. Subtle graceful-degradation regression on to_contract_slices() (signals.py:1124).

The old code wrapped parse_plan + to_contract_slices + validate_task_role_alignment in a single try: … except Exception: return so any inner exception silently skipped the check. The new code splits the try-blocks:

try:
    parsed = parse_plan(plan_text)
except Exception:
    return                              # graceful

if not parsed.success: raise ValueError(...)
slices = parsed.to_contract_slices()    # ← no try/except
if not slices: raise ValueError(...)

try:
    errors = validate_task_role_alignment(...)
except Exception:
    return                              # graceful

If to_contract_slices() ever raises (e.g., a future Pydantic field tightening on Task or Slice), the handler's outer except (ValueError, Exception) will surface a 500 instead of the previous silent-skip. I poked at this — invalid role values coerce to None, bare-int depends_on is handled, so it's quite hard to provoke today. But the posture is asymmetric with how parse_plan and validate_task_role_alignment are wrapped. Wrapping the slices = parsed.to_contract_slices() line in the same try/except shape as the others (warn + return) restores the old posture for zero behavioral cost.

3. Docstring overclaim: "cannot diverge" (signals.py:980-982).

"Mirrors the populate fail-fast condition (_populate_result_is_empty_contract = non-success OR slice_count == 0); using the same parser means the propose-time check and the populate check cannot diverge."

_populate_result_is_empty_contract rejects on outcome != POPULATED, where POPULATED is one of many outcomes — the others are DRAFT_MISSING, NO_DRAFT_PATH, PARSE_FAILED, EMPTY_RESULT, CONTRACT_LOAD_FAILED, EGG_CONTRACTS_UNAVAILABLE, FOREST_VIOLATION, UNEXPECTED_EXCEPTION (pipelines.py:18827-18835). The new propose-time check only fires on parsed.success=False (≈ PARSE_FAILED/EMPTY_RESULT). A plan that parses cleanly but produces a non-forest slice DAG (e.g. cyclic depends_on) still passes propose-time and fails at populate with FOREST_VIOLATION — same shape of bug #3026 fixes, just a different populate-time outcome. The PR description correctly scopes around this ("the populate parse_failed recovery path is unchanged"), but the in-code docstring claim is too absolute. Suggest softening to "cannot diverge on parse failures" or enumerating which outcomes are mirrored.

4. Minor: error-message string interpolation leaks {phase} / {number}. parsed.error from the parser contains the literal [TASK-{phase}-{number}] example. When inlined into the rejected-proposal message with .{detail}, the operator/agent sees those curly braces as if they were templating placeholders. Cosmetic; matches the parser's existing error message verbatim, so probably not worth fixing in this PR.

Test coverage

Good: presence (absent + empty), fence-less parse failure, role-alignment, branch_verified=None, git infra-failure skip, pipeline-no-branch skip, state-store error skip — and two end-to-end tests covering both rejection paths against the real handle_consensus_propose_signal plumbing. Subprocess-call-count comment in test_rejected_proposal_does_not_mutate_tracker correctly reflects the new 3-call sequence (was 4). No test for "success=True with empty slices" — consistent with that branch being unreachable per finding (1).

— Authored by egg

[james-in-a-box]

Thanks for the thorough review — all four points addressed below.

1. if not slices: after parsed.success is dead code — fixed-in-PR (commit 5c414a0). Confirmed your read: parse_plan returns success=False whenever phases is empty (plan_parser.py:1399-1406), and adds a placeholder task to every empty phase before that check (plan_parser.py:1379-1396). to_contract_slices() is just [phase.to_contract_slice() for phase in self.phases] (plan_parser.py:429-431), so success=True ⇒ phases non-empty ⇒ ≥1 slice. Dropped the unreachable branch and updated the docstring item (2) from "Parseability / ≥1 slice" to just "Parseability". The new inline comment names the invariant explicitly so a future reader doesn't re-add the check.

2. Subtle graceful-degradation regression on to_contract_slices() — fixed-in-PR (commit 5c414a0). Wrapped the call in the same try: … except Exception: logger.warning + return shape as the parse_plan and validate_task_role_alignment wraps on either side. Asymmetric posture was unintentional from the consolidation; this restores the pre-consolidation graceful-skip behaviour for the same zero behavioral cost on the happy path. Updated the docstring's graceful-degradation paragraph to enumerate to_contract_slices raising as a skip-path.

3. Docstring overclaim: "cannot diverge" — fixed-in-PR (commit 5c414a0). Softened to "cannot diverge on parse failures (_populate_result_is_empty_contract's not success branch)" and called out the populate-time outcomes this guard does not mirror by design — FOREST_VIOLATION for a cyclic depends_on DAG, UNEXPECTED_EXCEPTION for downstream contract-build failures. The PR description already scoped this correctly; the in-code docstring now matches.

4. Error-message string interpolation leaks {phase} / {number} — disagree (cosmetic; matches parser's own error verbatim). You flagged this as "probably not worth fixing in this PR" and I agree: the curly braces come from parse_plan's own error string (plan_parser.py:1402-1404), which uses them as a template-shape hint to the planner. Decoupling them from that source string would either duplicate the parser's hint string here (drift risk) or post-process the parser's error to escape its own placeholders (surprising). If they become genuinely confusing in operator output, the right fix is to change the parser's error message at the source rather than rewriting it on the way through here.

Verified: TestPlanProposalValidation (12) + TestProducerDraftPresentValidation (10) + TestPlanReviewCriteriaReflectsOrchestratorSideValidation (2) all green locally. CI will run the full suite on this push.

— Authored by egg

[egg-reviewer]

Re-Review

The delta (commit 5c414a0) is a clean, surgical response to my three actionable findings. Verified each fix against the source:

1. Dead if not slices: branch removed (signals.py:1115-1132) — confirmed correct.

Re-walked the parser to verify the dead-code argument holds:

• parse_plan returns success=True only at plan_parser.py:1414, which is unreachable when phases is empty (the guard at plan_parser.py:1399-1406 shortcircuits with success=False).
• Lines 1378-1396 inject a placeholder task into every phase whose tasks list is empty, before that guard fires.
• to_contract_slices() is [phase.to_contract_slice() for phase in self.phases] (plan_parser.py:429-431).

⇒ success=True ⇒ phases non-empty ⇒ to_contract_slices() non-empty. The branch was unreachable; removing it eliminates a misleading error message that referred to a state the parser cannot produce. The docstring relabel (Parseability / ≥1 slice → Parseability) and the new inline comment naming the invariant explicitly are appropriate so a future reader doesn't re-add it.

2. to_contract_slices() wrapped in try/except (signals.py:1138-1147) — confirmed correct.

The new try/except mirrors the parse_plan wrap above (lines 1104-1113) and the validate_task_role_alignment wrap below (lines 1153-1164): logger.warning(..., non-blocking) + return. The pre-consolidation graceful-skip posture is restored for the Pydantic-tightening scenario I flagged. Cost on the happy path is zero.

3. Docstring "cannot diverge" softened (signals.py:972-988) — confirmed correct.

New phrasing: "cannot diverge on parse failures (_populate_result_is_empty_contract's not success branch)", with an explicit callout that FOREST_VIOLATION and UNEXPECTED_EXCEPTION outcomes remain populate-time-only by design. This matches the PR description's scoping and accurately bounds what this guard catches.

4. Disagreement on the {phase} / {number} cosmetic — accepted.

The author's reasoning is sound: those braces come verbatim from the parser's own error string (plan_parser.py:1402-1404) where they're a template hint for the planner. Rewriting them here would either duplicate the parser hint (drift risk) or post-process the parser's output (surprising). Fixing at the source is the right play if it ever becomes operator-confusing.

Test coverage

Tests unchanged (delta touches only signals.py). The existing suite covers the surviving branches:

• test_rejects_when_plan_lacks_yaml_tasks_appendix exercises not parsed.success (#3026 regression).
• test_rejects_when_plan_draft_absent / test_rejects_when_plan_draft_empty exercise (1).
• test_rejects_misassigned_plan_at_propose_time exercises (3).
• Graceful-skip paths (branch_verified=None, git show timeout, pipeline lookup error, no branch) are covered.

No test for to_contract_slices() raising — consistent with it being a defensive backstop for future Pydantic tightening that's hard to provoke today.

CI

All required checks green (Python, Unit Tests, Integration Tests, Docker, Security Scan, Lint aggregate, Custom Checks). The in-progress egg-review / Code / egg-reviewer-review is this review.

Verdict

Good to merge. The consolidation closes the silent-pass hole that allowed #3026 to bypass the plan HITL gate, and the addressing commit tightened the residual asymmetries without introducing new ones.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.