diff --git a/integration_tests/test_deployment_validation_logic.py b/integration_tests/test_deployment_validation_logic.py index 513e30ebb6..9c430b242b 100644 --- a/integration_tests/test_deployment_validation_logic.py +++ b/integration_tests/test_deployment_validation_logic.py @@ -373,12 +373,18 @@ def test_pipeline_id_regex_valid_at_boundaries_pass( special chars all must NOT reject at the label-validator stage. They may then short-circuit at the CNI gate (B2 today) or run the probe — but they must not 400. + + Uses ``timeout=90`` to leave headroom over the orchestrator's 75s + probe-pod wait (#2699): when the probe actually runs instead of + short-circuiting, the default 60s HTTP timeout is too tight and + sporadically reads-out before the orchestrator responds. """ resp = _post( orchestrator_url, "/api/v1/deployment/validate-network-isolation", secret=lifecycle_secret, body={"pipeline_id": pipeline_id, "role": "coder"}, + timeout=90, ) assert resp.status_code == 200, ( f"expected 200 for valid label {pipeline_id!r}, got {resp.status_code}: " diff --git a/orchestrator/mcp_tools.py b/orchestrator/mcp_tools.py index a44cc38d8f..26c4771864 100644 --- a/orchestrator/mcp_tools.py +++ b/orchestrator/mcp_tools.py @@ -1019,8 +1019,20 @@ def _is_timeout_error(exc: BaseException) -> bool: "description": ( "Populate a pipeline's SDLC contract from its plan draft. Reads the " "plan document, extracts task structure, and writes tasks and acceptance " - "criteria to the contract.\n\n" - "Error responses include a machine-readable `reason` code (#1939). " + "criteria to the contract. On the `POPULATED` outcome the route also " + "commits the contract and pushes the work branch to origin so fresh " + "agent spawns (restart_phase, restart_agent, post-cancel restart) pull " + "the populated state on respawn (#2629).\n\n" + "Success response data includes `pushed_to_origin` (bool): True only " + "when `push_worktree_branch` reported success (a no-op fast-forward " + "push counts; a no-op commit alone does not). False when the push " + "failed, the commit/push step raised, or the push was not attempted " + "(`pipeline.branch` unset, or worktree resolves to the orchestrator's " + "repo path). When False the operator must commit and push themselves " + "before respawning agents — otherwise agents will pull the empty " + "contract from origin and the implement-start guard will wedge the " + "pipeline.\n\n" + "Error responses include a machine-readable `reason` code (#1939, #2627). " "Note: reason codes are only visible to direct HTTP callers; the " "MCP handler layer does not yet surface them.\n" "- `invalid_pipeline_id` (400), `pipeline_not_found` (404)\n" diff --git a/orchestrator/routes/phases.py b/orchestrator/routes/phases.py index 271075fdd9..4e28601d18 100644 --- a/orchestrator/routes/phases.py +++ b/orchestrator/routes/phases.py @@ -1032,19 +1032,43 @@ def populate_contract(pipeline_id: str) -> tuple[Response, int]: Reads the plan document from the pipeline's worktree, extracts task structure, and writes tasks and acceptance criteria to the contract. + On the ``POPULATED`` outcome the route also commits the contract to + the orchestrator's local worktree and pushes the work branch to + origin so fresh agent spawns (``restart_phase``, ``restart_agent``, + post-cancel restart) pull the populated state on respawn (#2629). URL params: pipeline_id: Pipeline ID - Response: + Response (200 — ``POPULATED``): { "success": true, "message": "Contract populated from plan", "data": { "phase_count": 2, - "task_count": 6 + "task_count": 6, + "pushed_to_origin": true } } + + ``pushed_to_origin`` is the operator's signal for whether agents + will see the populated state on respawn. ``True`` iff + ``push_worktree_branch`` reported success (a no-op fast-forward push + counts; a no-op commit alone does not). ``False`` means the commit + or push failed (or the push was not attempted because + ``pipeline.branch`` is unset or the worktree resolves to + ``store.repo_path``) and the operator must commit and push + themselves before respawning. + + Error responses include a machine-readable ``reason`` code (#1939, + #2627): + + - 400 ``invalid_pipeline_id`` + - 404 ``pipeline_not_found`` / ``draft_missing`` / ``no_draft_path`` + - 422 ``parse_failed`` / ``empty_result`` / forest violations + (structured body) + - 500 ``contract_load_failed`` / ``egg_contracts_unavailable`` / + ``unexpected_exception`` / ``populate_contract_failed`` """ try: store, pipeline = get_state_store_for_pipeline(pipeline_id) @@ -1055,7 +1079,14 @@ def populate_contract(pipeline_id: str) -> tuple[Response, int]: worktree_path = resolve_worktree_path(pipeline_id, store.repo_path) # Import and call the populate function - from routes.pipelines import PopulateOutcome, _populate_contract_from_plan + from routes.pipelines import ( + PopulateOutcome, + _commit_statefiles_to_worktree, + _compute_gateway_mode, + _get_spawner, + _pipeline_identifier, + _populate_contract_from_plan, + ) _populate_endpoint_result = _populate_contract_from_plan( repo_path=worktree_path, @@ -1071,11 +1102,70 @@ def populate_contract(pipeline_id: str) -> tuple[Response, int]: # outer ``except`` (HTTP 422 with structured errors). _outcome = _populate_endpoint_result.outcome if _outcome == PopulateOutcome.POPULATED: + # Persist the populated contract back to origin so fresh + # agent spawns (restart_phase, restart_agent, post-cancel + # restart) pull the populated state on respawn rather than + # the empty contract on origin. Without this, the route + # mutates only the orchestrator's local worktree and is + # unusable as a recovery primitive — the implement-start + # guard would refuse to demote to monolithic and the + # pipeline would wedge. See #2629. + # + # Failures here are fail-soft: ``pushed_to_origin`` in the + # response data tells the caller whether the contract is + # visible to agents. ``False`` means the operator must + # commit and push themselves before respawning. + pushed_to_origin = False + if pipeline.branch and worktree_path != store.repo_path: + try: + identifier = _pipeline_identifier(pipeline.issue_number, pipeline_id) + _commit_statefiles_to_worktree( + worktree_path, + f"Populate contract for {identifier} (#2629)", + pipeline_identifier=identifier, + pipeline_id=pipeline_id, + ) + # Push unconditionally — a no-op commit does NOT + # imply origin matches local. The per-pipeline + # worktree is long-lived (see + # ``resolve_worktree_path``) and may carry commits + # ahead of origin from a prior failed push. + # Pushing unconditionally fast-forwards in the safe + # case (origin already matches → no-op push) and + # delivers the un-pushed commit in the dangerous + # one (the exact wedge #2629 was opened against). + # The ``populate_contract`` route is an + # operator-initiated recovery primitive, not a hot + # loop, so the gateway round-trip is cheap relative + # to the correctness benefit. + gateway_mode, _ = _compute_gateway_mode(pipeline) + push_result = _get_spawner().gateway.push_worktree_branch( + pipeline_id=pipeline_id, + repo_path=str(worktree_path), + branch=pipeline.branch, + mode=gateway_mode, + base_branch=pipeline.base_branch, + ) + pushed_to_origin = bool(push_result) + if not pushed_to_origin: + logger.warning( + "populate_contract: push failed (continuing)", + pipeline_id=pipeline_id, + detail=push_result.describe(), + ) + except Exception as persist_err: # noqa: BLE001 + logger.warning( + "populate_contract: persist to origin failed (continuing)", + pipeline_id=pipeline_id, + error=str(persist_err), + ) + return make_success_response( "Contract populated from plan", data={ "phase_count": _populate_endpoint_result.slice_count, "task_count": _populate_endpoint_result.task_count, + "pushed_to_origin": pushed_to_origin, }, ) if _outcome in {PopulateOutcome.DRAFT_MISSING, PopulateOutcome.NO_DRAFT_PATH}: diff --git a/orchestrator/tests/test_populate_contract_endpoint.py b/orchestrator/tests/test_populate_contract_endpoint.py index 731ae3e6a2..75d11e28eb 100644 --- a/orchestrator/tests/test_populate_contract_endpoint.py +++ b/orchestrator/tests/test_populate_contract_endpoint.py @@ -10,6 +10,7 @@ import pytest from flask import Flask +from gateway_client import PushResult from models import Pipeline, PipelinePhase from routes.phases import phases_bp from routes.pipelines import PopulateOutcome, PopulateResult @@ -82,16 +83,31 @@ def test_invalid_pipeline_id(self, mock_get_store, client): data = json.loads(resp.data) assert data["success"] is False + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") @patch("routes.pipelines._populate_contract_from_plan") @patch("routes.resolve_worktree_path") @patch("routes.phases.get_state_store_for_pipeline") def test_pipeline_mode_from_pipeline_not_config( - self, mock_get_store, mock_resolve_wt, mock_populate, client + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, ): """pipeline.mode (not pipeline.config.mode) is used for pipeline_mode. Regression test: originally pipeline.config.mode was used but PipelineConfig has no mode attribute — mode lives on Pipeline directly. + + Stacks the same persist-block mocks as the success-path tests + so the new commit/push block doesn't execute against the real + :func:`_commit_statefiles_to_worktree` and (depending on host + worktree state) fire real ``git add``/``commit`` calls. """ pipeline = _make_pipeline() # pipeline.mode defaults to 'issue' from PipelineMode.ISSUE @@ -100,6 +116,11 @@ def test_pipeline_mode_from_pipeline_not_config( mock_get_store.return_value = (mock_store, pipeline) mock_resolve_wt.return_value = Path("/tmp/wt") mock_populate.return_value = _populated() + mock_commit.return_value = False + mock_gw_mode.return_value = ("public", None) + push_result = MagicMock() + push_result.__bool__.return_value = True + mock_spawner.return_value.gateway.push_worktree_branch.return_value = push_result resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") @@ -128,11 +149,21 @@ def test_populate_function_exception( assert data["success"] is False assert "plan draft not found" in data["message"] + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") @patch("routes.pipelines._populate_contract_from_plan") @patch("routes.resolve_worktree_path") @patch("routes.phases.get_state_store_for_pipeline") def test_success_with_counts_and_issue_number( - self, mock_get_store, mock_resolve_wt, mock_populate, client + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, ): """Successful populate returns phase/task counts and forwards issue_number.""" pipeline = _make_pipeline() @@ -141,6 +172,11 @@ def test_success_with_counts_and_issue_number( mock_get_store.return_value = (mock_store, pipeline) mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") mock_populate.return_value = _populated(slice_count=1, task_count=2) + mock_commit.return_value = True + mock_gw_mode.return_value = ("public", None) + push_result = MagicMock() + push_result.__bool__.return_value = True + mock_spawner.return_value.gateway.push_worktree_branch.return_value = push_result resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") @@ -151,19 +187,327 @@ def test_success_with_counts_and_issue_number( # follow-up): no separate read-back to disagree with the populator. assert data["data"]["phase_count"] == 1 assert data["data"]["task_count"] == 2 + assert data["data"]["pushed_to_origin"] is True # Verify populate was called with pipeline.mode (defaults to 'issue') call_kwargs = mock_populate.call_args[1] assert str(call_kwargs["pipeline_mode"]) == "issue" assert call_kwargs["issue_number"] == 42 + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_success_commits_and_pushes_contract_to_origin( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """populate_contract commits and pushes the contract so fresh agent + spawns see the populated state on origin (#2629).""" + pipeline = _make_pipeline() + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + worktree_path = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_resolve_wt.return_value = worktree_path + mock_populate.return_value = _populated() + mock_commit.return_value = True + mock_gw_mode.return_value = ("public", None) + push_result = MagicMock() + push_result.__bool__.return_value = True + gateway = mock_spawner.return_value.gateway + gateway.push_worktree_branch.return_value = push_result + + client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + mock_commit.assert_called_once() + commit_kwargs = mock_commit.call_args.kwargs + assert commit_kwargs["pipeline_identifier"] == 42 + assert commit_kwargs["pipeline_id"] == "issue-42" + + gateway.push_worktree_branch.assert_called_once_with( + pipeline_id="issue-42", + repo_path=str(worktree_path), + branch="egg/issue-42", + mode="public", + base_branch=pipeline.base_branch, + ) + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_push_failure_reports_pushed_to_origin_false( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """When the push fails, populate still returns success but reports + ``pushed_to_origin=False`` so the operator knows the contract is + only on the orchestrator's local worktree (#2629).""" + pipeline = _make_pipeline() + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_populate.return_value = _populated(slice_count=1, task_count=1) + mock_commit.return_value = True + mock_gw_mode.return_value = ("public", None) + gateway = mock_spawner.return_value.gateway + gateway.push_worktree_branch.side_effect = RuntimeError("network down") + + resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["success"] is True + assert data["data"]["pushed_to_origin"] is False + # Counts still come back so the caller can confirm populate worked. + assert data["data"]["task_count"] == 1 + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_falsy_push_result_reports_pushed_to_origin_false( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """When ``push_worktree_branch`` *returns* a falsy ``PushResult`` + (the gateway client converts most push failures to this shape — + ``non_fast_forward``, ``auth_failed``, ``reconcile_fetch_failed``, + etc. — rather than raising), the route logs + ``push_result.describe()`` and reports + ``pushed_to_origin=False`` (#2629). + + This covers the falsy-return branch separately from the + exception branch exercised by + ``test_push_failure_reports_pushed_to_origin_false`` — the + falsy-return shape is the more common one in practice because + :func:`gateway_client._do_push` catches most exceptions and + converts them via :func:`_classify_push_stderr`. + """ + pipeline = _make_pipeline() + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_populate.return_value = _populated(slice_count=1, task_count=1) + mock_commit.return_value = True + mock_gw_mode.return_value = ("public", None) + gateway = mock_spawner.return_value.gateway + gateway.push_worktree_branch.return_value = PushResult( + ok=False, + category="non_fast_forward", + detail="(fetch first)", + ) + + resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["success"] is True + assert data["data"]["pushed_to_origin"] is False + # The push attempt happened — only the result reports failure. + gateway.push_worktree_branch.assert_called_once() + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_commit_noop_still_pushes( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """When the commit short-circuits (nothing staged), the push + still runs — a no-op commit does NOT imply origin matches local + (#2629). + + The per-pipeline worktree is long-lived and may carry commits + ahead of origin from a prior failed push. Pushing + unconditionally fast-forwards in the safe case (origin already + matches → no-op push) and delivers the un-pushed commit in the + dangerous one. Reporting ``pushed_to_origin=True`` here + requires the push to actually report success — we do not + infer it from the no-op commit alone. + """ + pipeline = _make_pipeline() + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_populate.return_value = _populated() + mock_commit.return_value = False + mock_gw_mode.return_value = ("public", None) + push_result = MagicMock() + push_result.__bool__.return_value = True + gateway = mock_spawner.return_value.gateway + gateway.push_worktree_branch.return_value = push_result + + resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["data"]["pushed_to_origin"] is True + gateway.push_worktree_branch.assert_called_once() + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_failed_push_retry_with_noop_commit_still_reports_failure( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """Regression for the failed-push-then-retry recovery scenario + (#2629). + + Models the post-failure retry state: a prior ``populate_contract`` + call committed locally but the push failed, leaving local HEAD + ahead of origin by one commit. The operator retries — the + file on disk now matches HEAD so ``_commit_statefiles_to_worktree`` + returns ``False`` (no-op commit) — but the un-pushed commit is + still on local only. + + The route must still attempt the push. If the push fails again + (gateway down, ``non_fast_forward``, etc.), ``pushed_to_origin`` + must be ``False`` so the operator does not interpret a no-op + commit as success when origin is in fact still empty. The + original wedge #2629 was opened against was a caller silently + treating ``pushed_to_origin=True`` as "contract is on origin" + when it was not. + """ + pipeline = _make_pipeline() + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_populate.return_value = _populated() + # Second-call shape: file already matches HEAD locally → no-op. + mock_commit.return_value = False + mock_gw_mode.return_value = ("public", None) + gateway = mock_spawner.return_value.gateway + # Push fails again — origin still does not have the contract. + gateway.push_worktree_branch.return_value = PushResult( + ok=False, + category="non_fast_forward", + detail="(fetch first)", + ) + + resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + assert resp.status_code == 200 + data = json.loads(resp.data) + # The push was attempted (no shortcut from no-op commit) and + # its failure was honored (no false success). + gateway.push_worktree_branch.assert_called_once() + assert data["data"]["pushed_to_origin"] is False + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") + @patch("routes.pipelines._populate_contract_from_plan") + @patch("routes.resolve_worktree_path") + @patch("routes.phases.get_state_store_for_pipeline") + def test_branch_unset_skips_persist_and_reports_false( + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, + ): + """When ``pipeline.branch`` is unset, the persist block is skipped + and ``pushed_to_origin=False`` is returned (#2629). + + The persist block is gated on ``pipeline.branch and worktree_path + != store.repo_path``; without a branch there is nothing to push + to. ``False`` is the correct signal here — the caller knows the + populated state is only on the orchestrator's local worktree. + """ + pipeline = _make_pipeline() + pipeline.branch = None # No work branch configured yet. + mock_store = MagicMock() + mock_store.repo_path = Path("/home/egg/repos/egg") + mock_get_store.return_value = (mock_store, pipeline) + mock_resolve_wt.return_value = Path("/home/egg/.egg-worktrees/issue-42/egg") + mock_populate.return_value = _populated() + mock_gw_mode.return_value = ("public", None) + gateway = mock_spawner.return_value.gateway + + resp = client.post("/api/v1/pipelines/issue-42/phase/populate-contract") + + assert resp.status_code == 200 + data = json.loads(resp.data) + assert data["success"] is True + assert data["data"]["pushed_to_origin"] is False + # Neither commit nor push are attempted without a branch. + mock_commit.assert_not_called() + gateway.push_worktree_branch.assert_not_called() + + @patch("routes.pipelines._get_spawner") + @patch("routes.pipelines._compute_gateway_mode") + @patch("routes.pipelines._commit_statefiles_to_worktree") @patch("routes.pipelines._populate_contract_from_plan") @patch("routes.resolve_worktree_path") @patch("routes.phases.get_state_store_for_pipeline") def test_worktree_path_passed_to_populate( - self, mock_get_store, mock_resolve_wt, mock_populate, client + self, + mock_get_store, + mock_resolve_wt, + mock_populate, + mock_commit, + mock_gw_mode, + mock_spawner, + client, ): - """Verify worktree path (not raw repo path) is passed to populate.""" + """Verify worktree path (not raw repo path) is passed to populate. + + Stacks the same persist-block mocks as the success-path tests + so the new commit/push block doesn't execute against the real + :func:`_commit_statefiles_to_worktree`. + """ pipeline = _make_pipeline() mock_store = MagicMock() mock_store.repo_path = Path("/home/egg/repos/egg") @@ -172,6 +516,11 @@ def test_worktree_path_passed_to_populate( worktree_path = Path("/home/egg/.egg-worktrees/issue-42/egg") mock_resolve_wt.return_value = worktree_path mock_populate.return_value = _populated() + mock_commit.return_value = False + mock_gw_mode.return_value = ("public", None) + push_result = MagicMock() + push_result.__bool__.return_value = True + mock_spawner.return_value.gateway.push_worktree_branch.return_value = push_result client.post("/api/v1/pipelines/issue-42/phase/populate-contract")