diff --git a/orchestrator/routes/signals.py b/orchestrator/routes/signals.py index 8a89a520a4..b4aaae5532 100644 --- a/orchestrator/routes/signals.py +++ b/orchestrator/routes/signals.py @@ -950,47 +950,86 @@ def _validate_tester_check_coverage( ) -def _validate_planner_role_alignment( +def _validate_plan_proposal( pipeline_id: str, payload: dict[str, Any], repo_path: Path, *, pipeline_state: Any | None = None, worktree_path: Path | None = None, + branch_verified: bool | None = True, ) -> None: - """Validate task role↔files alignment for a planner proposal (#2527). - - Reads the plan draft at the proposed commit via ``git show`` against - the orchestrator's pipeline worktree and runs - ``validate_task_role_alignment``. Raises ``ValueError`` if any task - is assigned to a role that cannot push its files — the caller's - ``handle_consensus_propose_signal`` ``except`` block then returns 400 - to the planner before the proposal is recorded on the tracker, so no - reviewer cycle is wasted on a structurally-broken plan. - - The check mirrors the gateway's push-time blocked-pattern logic - (``gateway/phase_filter.py::FileRestriction.is_file_blocked``), so a - rejection here predicts a push-time ``403 restricted_path_modified`` - in the implement phase. Caught here it costs the planner one - re-propose; caught at push time it costs a full producer cycle. - - Graceful degradation: when the plan file doesn't exist at the - proposed commit, ``git show`` fails, parsing fails, or the validator - raises, this function returns silently. The push-time gateway check - remains the backstop, and the existing forest-validation pass at - plan-ingestion time still runs. - - ``pipeline_state`` and ``worktree_path`` should be passed in by - ``handle_consensus_propose_signal`` so the state-store + worktree - lookups it has already performed for ``_verify_commit_on_branch`` - aren't duplicated. The function falls back to loading them itself - when called directly (e.g. from unit tests), preserving backward - compatibility with patches on ``get_state_store`` / ``resolve_worktree_path``. + """Validate a ``task_planner`` proposal at propose-time (#3016 / #3026 / #2527). + + A single ``git show`` + a single ``parse_plan`` of the plan draft at the + proposed commit, feeding three checks that the populate / gate / push paths + would otherwise enforce only later and far more expensively: + + 1. **Presence** (#3016): the canonical plan draft exists and is non-empty at + the proposed commit. The operator gate, contract populator, and resume + path all read the plan from ``_get_draft_path("plan", …)``; a draft + committed off-path (or not at all) is invisible to them. + 2. **Parseability** (#3026): the draft parses via the *same* ``parse_plan`` + the contract populator runs. A draft that is complete in prose but omits + the machine-readable ``# yaml-tasks`` appendix parses to ``success=False`` + — it passes BRC consensus and phase completion on content grounds, then + fails the *whole pipeline* at ``populate_contract`` (``parse_failed`` / + ``empty_result``) ~40 min later, after the plan HITL gate, surfacing an + expensive recovery decision. Using the same parser means the propose-time + check and the populate check cannot diverge **on parse failures** + (``_populate_result_is_empty_contract``'s ``not success`` branch); other + populate-time outcomes — ``FOREST_VIOLATION`` for a cyclic ``depends_on`` + DAG, ``UNEXPECTED_EXCEPTION`` for downstream contract-build failures — + are still caught only at populate, by design (this guard is for the + fence-less / unparseable case #3026 names). Catching that here is one + cheap NACK→re-propose cycle. (``parse_plan`` itself guarantees + ``success=True`` ⇒ ≥1 phase ⇒ ≥1 slice via ``to_contract_slices`` — the + per-phase placeholder-task injection sees to it — so a separate + ``slice_count == 0`` check would be dead code here.) + 3. **Role↔files alignment** (#2527 / #2528): no task is assigned to a role + whose blocklist forbids its files (would 403 at push time per + ``gateway/phase_filter.py::FileRestriction.is_file_blocked`` / + ``shared/egg_restrictions/patterns.py``). + + All three run BEFORE ``handle_propose`` records the proposal, so a rejection + costs the still-alive planner one re-propose and never mutates the tracker; + the caller's ``except`` maps the raised ``ValueError`` to a 400. + + Consolidates the former ``_validate_producer_draft_present("plan", …)`` + + ``_validate_planner_role_alignment`` pair, which each ``git show``-ed the + same draft at the same commit and disagreed on how to treat a missing one. + A single read now feeds every check. (Refine still uses + ``_validate_producer_draft_present`` — analysis drafts have no parseable + appendix, so existence-only is the right check there.) + + ``pipeline_state`` / ``worktree_path`` are threaded in by + ``handle_consensus_propose_signal`` to reuse the lookups it already performed + for ``_verify_commit_on_branch``; the function loads them itself when called + directly (e.g. from unit tests). + + Graceful degradation: returns silently when the proposal carries no commit + SHA, branch verification was inconclusive (``branch_verified is None`` — an + orchestrator-side fetch glitch, not a producer fault; a non-zero ``git show`` + could then mean "commit not in local cache" rather than "path absent"), the + pipeline has no branch / no resolvable draft path, ``git show`` errors for an + infrastructure reason (timeout / git failure), the parser raises, or + ``to_contract_slices`` raises (e.g. a future Pydantic field tightening). It + raises only when it can positively confirm — at a resolved, branch-verified + commit — that the draft is absent/empty, unparseable, or role-misassigned. + ``branch_verified`` defaults to ``True`` so direct callers (unit tests) that + don't run ``_verify_commit_on_branch`` still get the check. """ commit_sha = (payload.get("commit_sha") or "").strip() if not commit_sha: return + # Orchestrator-side commit verification was inconclusive — a non-zero + # ``git show`` below could be "commit not in local object cache" rather than + # "path absent at commit", so skip rather than false-blame the producer (the + # same tri-state guard ``_validate_producer_draft_present`` uses, #3016). + if branch_verified is None: + return + if pipeline_state is None: try: pipeline_state = get_state_store(repo_path).load_pipeline(pipeline_id) @@ -1020,10 +1059,9 @@ def _validate_planner_role_alignment( if worktree_path is None: worktree_path = resolve_worktree_path(pipeline_id, repo_path) - # Read plan content as committed at the proposed SHA so a stale - # local checkout cannot mask a real misassignment. The preceding - # ``_verify_commit_on_branch`` call has already done a ``git fetch``, - # making the commit reachable in the worktree. + # Read plan content as committed at the proposed SHA, not from the working + # tree (which can lag HEAD — #2723). The preceding ``_verify_commit_on_branch`` + # call has already done a ``git fetch``, making the commit reachable. try: result = subprocess.run( ["git", "-C", str(worktree_path), "show", f"{commit_sha}:{plan_rel}"], @@ -1032,29 +1070,29 @@ def _validate_planner_role_alignment( timeout=15, check=False, ) - if result.returncode != 0: - # Plan absent at this commit — nothing to validate. Note: on the - # production handler path this branch is unreachable for - # task_planner because ``_validate_producer_draft_present`` (#3016) - # now runs first in ``handle_consensus_propose_signal`` and rejects - # an absent plan with 400 before this validator is invoked. The - # silent-skip here is retained as a safety net for direct callers - # (tests, future re-orderings) — but the two guards have opposite - # responses to the same ``returncode != 0`` signal, so any - # refactor that moves the alignment guard ahead of the presence - # guard would silently re-introduce the #3016 bug. Keep - # ``_validate_producer_draft_present`` upstream of this call. - return - plan_text = result.stdout except Exception as exc: logger.warning( - "plan role-alignment validation: git show failed (non-blocking)", + "plan proposal validation: git show failed (non-blocking)", pipeline_id=pipeline_id, commit_sha=commit_sha, error=str(exc), ) return + # (1) Presence — absent or empty at the proposed commit. (Previously a + # separate ``_validate_producer_draft_present("plan", …)`` call; folded in + # here so the same read serves all three checks.) + if result.returncode != 0 or not result.stdout.strip(): + raise ValueError( + f"Plan proposal rejected: no plan draft found at `{plan_rel}` in the " + f"proposed commit ({commit_sha[:8]}). The phase gate, contract " + f"population, and resume all read the plan from this exact path — a " + f"draft committed to a different path (or an empty one) is invisible " + f"to them. Write your plan to `{plan_rel}`, commit and push it, then " + f"re-propose." + ) + plan_text = result.stdout + try: from egg_contracts.plan_parser import ( parse_plan, @@ -1065,13 +1103,54 @@ def _validate_planner_role_alignment( try: parsed = parse_plan(plan_text) - if not parsed.success: - return + except Exception as exc: + logger.warning( + "plan proposal validation: parser raised (non-blocking)", + pipeline_id=pipeline_id, + commit_sha=commit_sha, + error=str(exc), + ) + return + + # (2) Parseability — the #3026 fix. Reuses the contract populator's parser + # so a draft that would later populate an empty contract on the + # ``not parsed.success`` branch of ``_populate_result_is_empty_contract`` is + # NACKed now instead. (``success=True`` already guarantees ``phases`` is + # non-empty and every phase has ≥1 task via the parser's + # placeholder-task injection, so ``to_contract_slices()`` cannot return an + # empty list — no separate slice-count check is needed.) + if not parsed.success: + detail = f" {parsed.error}" if parsed.error else "" + raise ValueError( + f"Plan proposal rejected: the plan draft at `{plan_rel}` " + f"({commit_sha[:8]}) does not parse into any tasks.{detail} The " + f"contract populator runs this exact parser at plan-completion; a " + f"draft that is complete in prose but omits the machine-readable " + f"``# yaml-tasks`` appendix passes consensus and then fails the whole " + f"pipeline at populate. Add a ``# yaml-tasks`` code fence enumerating " + f"your slices and tasks, commit and push it, then re-propose." + ) + + # Symmetric with the ``parse_plan`` / ``validate_task_role_alignment`` + # wraps above and below: a future Pydantic field tightening on ``Task`` / + # ``Slice`` could cause this to raise, and the old single-try posture + # silently skipped any such failure rather than surfacing a 500. + try: slices = parsed.to_contract_slices() - # #2528: pass the pipeline's repo so per-repo role_patterns from - # repositories.yaml are honoured. Plan-time validation must - # mirror push-time enforcement, which now also reads the per-repo - # overrides (gateway/agent_restrictions.py). + except Exception as exc: + logger.warning( + "plan proposal validation: to_contract_slices raised (non-blocking)", + pipeline_id=pipeline_id, + commit_sha=commit_sha, + error=str(exc), + ) + return + + # (3) Role↔files alignment (#2527). #2528: pass the pipeline's repo so + # per-repo ``role_patterns`` from repositories.yaml are honoured — plan-time + # validation must mirror push-time enforcement, which also reads the per-repo + # overrides (gateway/agent_restrictions.py). + try: errors = validate_task_role_alignment( slices, repo=getattr(pipeline_state, "repo", None) or None ) @@ -1110,17 +1189,24 @@ def _validate_producer_draft_present( """Reject a producer proposal whose canonical phase draft is absent at the proposed commit (#3016). - The refine/plan operator gate, the contract populator, and the resume path all - read the phase artifact from the canonical + Used for the **refine** phase. (Plan goes through ``_validate_plan_proposal``, + which folds this presence check together with parseability and role-alignment + into a single read — a plan draft that parses to ≥1 slice is necessarily + present and non-empty, so a separate existence-only pass would be redundant. + The ``phase`` parameter is kept generic so the function remains a phase-neutral + presence guard for any future producer whose artifact is *not* parsed.) + + The refine operator gate, the contract populator, and the resume path all read + the phase artifact from the canonical ``.egg-state/drafts/{prefix}-{analysis|plan}.md`` path (``_get_draft_path``). A producer that commits its draft to some other path — or not at all — still reaches BRC consensus and completes the phase; only later does the gate report - "No draft was found on the work branch", a silent + "No draft was found on the work branch", a silent deterministic-input failure. (Observed: a refiner that committed ``.egg-state/agent-outputs/refiner-refine.md`` — a path no code reads — instead of ``.egg-state/drafts/-analysis.md``.) - refine and plan are always concurrent, so the producer always issues + refine is always concurrent, so the producer always issues ``consensus_propose``. Validating here — before ``handle_propose`` records the proposal — turns a missing draft into a 400 the producer (still alive) can fix by re-proposing with the draft at the right path, instead of the failure surfacing a @@ -1129,8 +1215,8 @@ def _validate_producer_draft_present( Existence-only by design. Format/template conformance is intentionally *not* enforced here — that would risk false-rejecting a legitimate-but-differently- - structured draft (see #3016 / #3017); the gate and reviewers remain the content - backstop. + structured analysis draft (see #3016 / #3017); the gate and reviewers remain the + content backstop. Graceful degradation: when the proposal carries no commit SHA, the pipeline has no branch, the draft path can't be derived, ``git show`` errors for an @@ -1181,7 +1267,7 @@ def _validate_producer_draft_present( return # Lazy import to avoid pulling the ~24k-line ``routes.pipelines`` module into - # ``signals`` import time (matches ``_validate_planner_role_alignment``). + # ``signals`` import time (matches ``_validate_plan_proposal``). try: from routes.pipelines import _get_draft_path except ImportError: @@ -1390,19 +1476,19 @@ def handle_consensus_propose_signal( # completion handler — graceful degradation on network errors (None). # # ``pipeline_state`` and ``worktree_path`` are loaded once here and - # threaded into ``_validate_planner_role_alignment`` below so the - # validator's dependency on this block is explicit and the - # state-store + worktree lookups aren't duplicated. + # threaded into the producer validators below (``_validate_plan_proposal`` / + # ``_validate_producer_draft_present``) so their dependency on this block is + # explicit and the state-store + worktree lookups aren't duplicated. commit_sha = payload.get("commit_sha", "") pipeline_state = None worktree_path = None # Tri-state mirroring ``_verify_commit_on_branch``: True (commit on # branch), False (commit NOT on branch — 409 short-circuit below), or # None (verification inconclusive — fetch or branch-contains errored). - # Threaded into ``_validate_producer_draft_present`` so that a glitch - # in our fetch doesn't get blamed on the producer as a missing draft - # (a non-zero ``git show`` after a failed fetch could be "commit not - # in local object cache" rather than "path absent at commit"). + # Threaded into the producer validators so that a glitch in our fetch + # doesn't get blamed on the producer as a missing draft (a non-zero + # ``git show`` after a failed fetch could be "commit not in local object + # cache" rather than "path absent at commit"). branch_verified: bool | None = True if commit_sha: try: @@ -1460,13 +1546,13 @@ def handle_consensus_propose_signal( worktree_path=worktree_path, branch_verified=branch_verified, ) - # Validate task_planner proposals: (a) the plan draft is present at the - # canonical path (#3016 — same deterministic-input guard as refine), and - # (b) no task is misassigned to a role whose blocklist forbids its files - # (#2527). Both run BEFORE handle_propose so the tracker isn't mutated. + # Validate task_planner proposals in one pass: the plan draft is present + # at the canonical path (#3016), parses into ≥1 slice via the same parser + # the contract populator runs (#3026), and assigns no task to a role whose + # blocklist forbids its files (#2527). Runs BEFORE handle_propose so the + # tracker isn't mutated on a rejected proposal. elif agent_role == "task_planner": - _validate_producer_draft_present( - "plan", + _validate_plan_proposal( pipeline_id, payload, repo_path, @@ -1474,13 +1560,6 @@ def handle_consensus_propose_signal( worktree_path=worktree_path, branch_verified=branch_verified, ) - _validate_planner_role_alignment( - pipeline_id, - payload, - repo_path, - pipeline_state=pipeline_state, - worktree_path=worktree_path, - ) # Check if this is a re-proposal changed_artifacts = data.get("changed_artifacts") diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 2d95343e38..79c4b75c24 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -2208,26 +2208,28 @@ def test_rejected_proposal_does_not_mutate_tracker(self): mock_tracker.handle_propose.assert_not_called() -class TestPlannerRoleAlignmentValidation: - """Tests for ``_validate_planner_role_alignment`` in ``signals.py`` (#2527). - - Exercises the production code path the original PR-1 implementation - couldn't reach: in concurrent BRC mode, ``_run_concurrent_phase`` - builds every reviewer prompt up-front before the planner has produced - the plan, so a prompt-time validator can never fire on the first - cycle. This validator runs at ``CONSENSUS_PROPOSE`` instead — by - that point the planner has pushed the plan to origin, and the - orchestrator reads it via ``git show :``. +class TestPlanProposalValidation: + """Tests for ``_validate_plan_proposal`` in ``signals.py``. + + Consolidates the former ``_validate_planner_role_alignment`` (#2527) and the + plan branch of ``_validate_producer_draft_present`` (#3016) into one + propose-time validator that does a single ``git show`` + single ``parse_plan`` + and asserts three things, in order: the plan draft is present and non-empty at + the proposed commit (#3016), it parses into ≥1 slice via the same parser the + contract populator runs (#3026), and no task is assigned to a role whose + blocklist forbids its files (#2527). + + Runs at ``CONSENSUS_PROPOSE`` — by that point the planner has pushed the plan + to origin and the orchestrator reads it via ``git show :``. + A prompt-time validator could never fire on the first cycle (in concurrent BRC + mode ``_run_concurrent_phase`` builds every reviewer prompt up-front, before + the planner has produced the plan). """ # A coder→docs assignment is still a misassignment (docs are the # documenter's scope). Note: coder→test-files is NO LONGER a violation — # the coder authors its own tests now (intentional overlap with the # tester), so this fixture uses a docs file to exercise the reject path. - # (Cherry-picked from main #2936 in the slice-4 v3 cycle to match the - # current validator semantics; the original slice-3 conflict-resolution - # kept the old ``integration_tests/conftest.py`` fixture but that path - # no longer trips the validator post-#2936.) _PLAN_WITH_MISASSIGNED_TASK = ( "# Plan\n" "\n" @@ -2266,6 +2268,28 @@ class TestPlannerRoleAlignmentValidation: "```\n" ) + # The #3026 incident fixture: a draft that is complete and faithful in prose + # — narrative slice breakdown, task references, acceptance criteria — but + # omits the machine-readable ``# yaml-tasks`` code fence the populator parses. + # ``parse_plan`` finds no yaml fence, no ``## Phase N:`` headers, and no + # ``[TASK-n-m] … — Acceptance: …`` markdown lines, so it returns + # ``success=False`` ("No tasks or phases found"). Pre-#3026 this sailed + # through consensus and failed the whole pipeline at ``populate_contract``. + _PLAN_WITHOUT_YAML_TASKS = ( + "# Plan: issue-3026\n" + "\n" + "## Overview\n" + "\n" + "Consolidate the two propose-time plan validators into one so the\n" + "presence check and the parse check cannot diverge.\n" + "\n" + "## Slice breakdown\n" + "\n" + "Slice one rewrites the validator; slice two updates the tests. Each\n" + "task ships with acceptance criteria and a role assignment, described\n" + "in prose here rather than in a structured appendix.\n" + ) + @staticmethod def _patched_store(issue_number: int | None = 2527, branch: str = "egg/issue-2527"): mock_pipeline = MagicMock() @@ -2289,16 +2313,16 @@ def _patched_worktree(): def test_skips_when_commit_sha_missing(self): """No commit SHA on payload → nothing to validate against.""" - from routes.signals import _validate_planner_role_alignment + from routes.signals import _validate_plan_proposal # Should not raise even with no other patches in place — the # bail-out happens before any state-store / git access. - _validate_planner_role_alignment("issue-2527", {"payload": {}}, Path("/tmp")) - _validate_planner_role_alignment("issue-2527", {"commit_sha": ""}, Path("/tmp")) + _validate_plan_proposal("issue-2527", {"payload": {}}, Path("/tmp")) + _validate_plan_proposal("issue-2527", {"commit_sha": ""}, Path("/tmp")) def test_rejects_misassigned_plan_at_propose_time(self): - """Planner pushed a plan with coder→test-files: validator raises.""" - from routes.signals import _validate_planner_role_alignment + """Planner pushed a plan with coder→docs: validator raises (#2527).""" + from routes.signals import _validate_plan_proposal with ( self._patched_store(), @@ -2307,11 +2331,11 @@ def test_rejects_misassigned_plan_at_propose_time(self): ): payload = {"commit_sha": "abc1234"} with pytest.raises(ValueError, match="role↔files alignment violations"): - _validate_planner_role_alignment("issue-2527", payload, Path("/tmp/repo")) + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) def test_accepts_clean_plan(self): - """Planner pushed a plan with correctly-assigned roles: no raise.""" - from routes.signals import _validate_planner_role_alignment + """Planner pushed a present, parseable, correctly-assigned plan: no raise.""" + from routes.signals import _validate_plan_proposal with ( self._patched_store(), @@ -2320,11 +2344,31 @@ def test_accepts_clean_plan(self): ): payload = {"commit_sha": "abc1234"} # Should not raise. - _validate_planner_role_alignment("issue-2527", payload, Path("/tmp/repo")) + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) + + def test_rejects_when_plan_lacks_yaml_tasks_appendix(self): + """#3026 regression: a present, prose-complete plan that omits the + ``# yaml-tasks`` appendix parses to ``success=False`` and is NACKed at + propose-time — instead of passing consensus and failing the whole + pipeline at ``populate_contract`` (``parse_failed``) ~40 min later. + """ + from routes.signals import _validate_plan_proposal - def test_skips_when_git_show_fails(self): - """``git show`` non-zero exit (plan absent at commit) → graceful skip.""" - from routes.signals import _validate_planner_role_alignment + with ( + self._patched_store(), + self._patched_worktree(), + self._patched_subprocess(self._PLAN_WITHOUT_YAML_TASKS), + ): + payload = {"commit_sha": "abc1234"} + with pytest.raises(ValueError, match="does not parse into any tasks"): + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) + + def test_rejects_when_plan_draft_absent(self): + """``git show`` non-zero exit (plan absent at commit) → presence raise + (#3016). The consolidated validator no longer silently skips this — it + rejects so the gate can't later false-negative on an off-path draft. + """ + from routes.signals import _validate_plan_proposal with ( self._patched_store(), @@ -2332,12 +2376,63 @@ def test_skips_when_git_show_fails(self): self._patched_subprocess("", returncode=128), ): payload = {"commit_sha": "abc1234"} - # Should not raise. - _validate_planner_role_alignment("issue-2527", payload, Path("/tmp/repo")) + with pytest.raises(ValueError, match=r"no plan draft found.*-plan\.md"): + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) + + def test_rejects_when_plan_draft_empty(self): + """Draft exists but is empty/whitespace-only → presence raise (#3016).""" + from routes.signals import _validate_plan_proposal + + with ( + self._patched_store(), + self._patched_worktree(), + self._patched_subprocess(" \n \n"), + ): + payload = {"commit_sha": "abc1234"} + with pytest.raises(ValueError, match="no plan draft found"): + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) + + def test_skips_when_branch_verified_is_none(self): + """``branch_verified=None`` (orchestrator-side fetch/contains glitch) → + graceful skip without touching git, so a transient fetch failure isn't + mis-blamed on the producer as a missing/unparseable draft. + """ + from routes.signals import _validate_plan_proposal + + with ( + self._patched_store(), + self._patched_worktree(), + patch("routes.signals.subprocess.run") as mock_run, + ): + _validate_plan_proposal( + "issue-2527", + {"commit_sha": "abc1234"}, + Path("/tmp/repo"), + branch_verified=None, + ) + mock_run.assert_not_called() + + def test_skips_when_git_show_errors(self): + """An infra failure (timeout, not a clean non-zero exit) → graceful skip, + distinct from the definitive absent-at-commit signal the presence raise + covers. + """ + from routes.signals import _validate_plan_proposal + + with ( + self._patched_store(), + self._patched_worktree(), + patch( + "routes.signals.subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd="git", timeout=15), + ), + ): + # Should not raise — infra failures degrade gracefully. + _validate_plan_proposal("issue-2527", {"commit_sha": "abc1234"}, Path("/tmp/repo")) def test_skips_when_pipeline_lookup_fails(self): """State store load failure → graceful skip.""" - from routes.signals import _validate_planner_role_alignment + from routes.signals import _validate_plan_proposal from state_store import StateValidationError mock_store = MagicMock() @@ -2349,11 +2444,11 @@ def test_skips_when_pipeline_lookup_fails(self): ): payload = {"commit_sha": "abc1234"} # Should not raise — graceful degradation - _validate_planner_role_alignment("issue-2527", payload, Path("/tmp/repo")) + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) def test_skips_when_pipeline_has_no_branch(self): """A pipeline with ``branch=None`` → graceful skip (no git context).""" - from routes.signals import _validate_planner_role_alignment + from routes.signals import _validate_plan_proposal with ( self._patched_store(branch=None), @@ -2361,20 +2456,15 @@ def test_skips_when_pipeline_has_no_branch(self): ): payload = {"commit_sha": "abc1234"} # Should not raise — branch is required to resolve the worktree commit. - _validate_planner_role_alignment("issue-2527", payload, Path("/tmp/repo")) + _validate_plan_proposal("issue-2527", payload, Path("/tmp/repo")) def test_rejected_proposal_does_not_mutate_tracker(self): """Integration: a planner proposal carrying a misassigned plan is rejected at ``handle_consensus_propose_signal`` BEFORE the - tracker is mutated — same guarantee as - ``test_rejected_proposal_does_not_mutate_tracker`` for - testers (#1459). - - This is the production-sequence end-to-end test the PR-1 review - flagged as missing: it builds the propose signal exactly the - way the planner agent does in concurrent BRC mode, mocks - ``git show`` to return the misassigned plan (the file the - orchestrator's worktree would read at the proposed commit), + tracker is mutated (#1459 / #2527). + + Builds the propose signal exactly the way the planner agent does in + concurrent BRC mode, mocks ``git show`` to return the misassigned plan, and asserts the tracker is left untouched. """ from flask import Flask @@ -2383,15 +2473,13 @@ def test_rejected_proposal_does_not_mutate_tracker(self): mock_tracker = MagicMock() mock_tracker.handle_propose = MagicMock(return_value={"version": 1}) - # Four subprocess calls happen in this path (task_planner): + # Three subprocess calls happen in this path (task_planner) — the + # consolidated ``_validate_plan_proposal`` reads the plan once, not + # twice as the former presence+alignment pair did: # 1. _verify_commit_on_branch's git fetch # 2. _verify_commit_on_branch's git branch --contains - # 3. _validate_producer_draft_present's git show (#3016) — plan - # present at the commit, so the presence guard passes - # 4. _validate_planner_role_alignment's git show — returns the - # misassigned plan, so the alignment guard raises - # The first two return success; calls 3 and 4 both read the plan draft - # at the proposed commit, so both return the (misassigned) plan. + # 3. _validate_plan_proposal's single git show — returns the + # misassigned plan, so the alignment check raises side_effect = [ subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr=""), subprocess.CompletedProcess( @@ -2406,12 +2494,6 @@ def test_rejected_proposal_does_not_mutate_tracker(self): stdout=self._PLAN_WITH_MISASSIGNED_TASK, stderr="", ), - subprocess.CompletedProcess( - args=[], - returncode=0, - stdout=self._PLAN_WITH_MISASSIGNED_TASK, - stderr="", - ), ] app = Flask(__name__) @@ -2442,23 +2524,74 @@ def test_rejected_proposal_does_not_mutate_tracker(self): assert "role↔files alignment violations" in data_out.get("message", "") # Tracker.handle_propose must NOT have been called — the # validator runs BEFORE the tracker, so a rejected proposal - # never mutates tracker state. This is the regression - # guarantee the PR-1 review flagged as the missing - # production-sequence test. + # never mutates tracker state. + mock_tracker.handle_propose.assert_not_called() + + def test_fence_less_plan_rejected_does_not_mutate_tracker(self): + """#3026 end-to-end: a task_planner proposal whose plan draft is present + but omits the ``# yaml-tasks`` appendix is rejected (400) at + ``handle_consensus_propose_signal`` BEFORE the tracker is mutated — the + cheap NACK→re-propose cycle, not a post-consensus populate failure. + """ + from flask import Flask + from routes.signals import handle_consensus_propose_signal + + mock_tracker = MagicMock() + mock_tracker.handle_propose = MagicMock(return_value={"version": 1}) + + # 1. fetch, 2. branch --contains, 3. _validate_plan_proposal's git show + # (present but fence-less → parse fails → raise). + side_effect = [ + subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr=""), + subprocess.CompletedProcess( + args=[], returncode=0, stdout=" origin/egg/issue-2527\n", stderr="" + ), + subprocess.CompletedProcess( + args=[], returncode=0, stdout=self._PLAN_WITHOUT_YAML_TASKS, stderr="" + ), + ] + + app = Flask(__name__) + with ( + app.app_context(), + self._patched_store(), + self._patched_worktree(), + patch("routes.signals.subprocess.run", side_effect=side_effect), + patch("peer_consensus.get_peer_consensus_tracker", return_value=mock_tracker), + ): + data = { + "agent_role": "task_planner", + "payload": { + "summary": ( + "Plan v1: 2-slice decomposition described in prose, " + "consolidating the propose-time validators" + ), + "artifacts": [".egg-state/drafts/2527-plan.md"], + "commit_sha": "abc1234", + }, + } + response, status_code = handle_consensus_propose_signal( + "issue-2527", data, Path("/tmp/repo") + ) + assert status_code == 400 + data_out = response.get_json() + assert "does not parse into any tasks" in data_out.get("message", "") mock_tracker.handle_propose.assert_not_called() class TestProducerDraftPresentValidation: """Tests for ``_validate_producer_draft_present`` in ``signals.py`` (#3016). - A refine/plan producer that commits its draft to a non-canonical path (or not - at all) used to reach BRC consensus and complete the phase, after which the - operator gate — which reads ``.egg-state/drafts/{prefix}-{analysis|plan}.md`` - via ``_get_draft_path`` — reported "No draft was found". This - validator runs at ``CONSENSUS_PROPOSE`` (always issued in concurrent refine/ - plan), reads the draft at the proposed commit via ``git show``, and rejects - (400) when it is absent so the still-alive producer re-proposes with the draft - at the right path. + Now the **refine** presence guard. (Plan presence is checked inside + ``_validate_plan_proposal`` — see ``TestPlanProposalValidation`` — which folds + presence, parseability, and role-alignment into a single read.) A refiner that + commits its analysis draft to a non-canonical path (or not at all) used to + reach BRC consensus and complete the phase, after which the operator gate — + which reads ``.egg-state/drafts/{prefix}-analysis.md`` via ``_get_draft_path`` + — reported "No analysis draft was found". This validator runs at + ``CONSENSUS_PROPOSE`` (always issued in concurrent refine), reads the draft at + the proposed commit via ``git show``, and rejects (400) when it is absent so + the still-alive refiner re-proposes with the draft at the right path. """ @staticmethod @@ -2531,39 +2664,6 @@ def test_rejects_when_refine_draft_empty(self): "refine", "issue-3016", {"commit_sha": "abc1234"}, Path("/tmp/repo") ) - def test_rejects_when_plan_draft_absent_names_plan_path(self): - """Plan-phase rejection names the plan draft, not analysis.""" - from routes.signals import _validate_producer_draft_present - - with ( - self._patched_store(), - self._patched_worktree(), - self._patched_subprocess("", returncode=128), - ): - with pytest.raises(ValueError, match=r"no plan draft found.*-plan\.md"): - _validate_producer_draft_present( - "plan", "issue-3016", {"commit_sha": "abc1234"}, Path("/tmp/repo") - ) - - def test_accepts_when_plan_draft_present(self): - """Plan draft present and non-empty at the proposed commit → no raise. - - Mirrors ``test_accepts_when_refine_draft_present`` to lock the symmetry - between the two producer phases — the end-to-end planner test exercises - the present-plan path indirectly (call 3 returning non-empty stdout), - but this is the direct unit-level confirmation. - """ - from routes.signals import _validate_producer_draft_present - - with ( - self._patched_store(), - self._patched_worktree(), - self._patched_subprocess("# Plan: issue-3016\n\n## Task 1\nDo the thing.\n"), - ): - _validate_producer_draft_present( - "plan", "issue-3016", {"commit_sha": "abc1234"}, Path("/tmp/repo") - ) - def test_skips_when_pipeline_lookup_fails(self): """State store load failure → graceful skip. @@ -5846,11 +5946,11 @@ def _raise(*_args, **_kwargs): # opposite of the truth. # # Resolution (this PR-2): orchestrator-side validation runs at -# CONSENSUS_PROPOSE in routes/signals.py:_validate_planner_role_alignment, +# CONSENSUS_PROPOSE in routes/signals.py:_validate_plan_proposal, # rejecting the planner's proposal with HTTP 400 before the tracker # state is mutated. The reviewer prompt no longer carries a per-prompt # section; the validator-runs-here tests live in this same file under -# class TestPlannerRoleAlignmentValidation (above). +# class TestPlanProposalValidation (above). class TestPlanReviewCriteriaReflectsOrchestratorSideValidation: