From 0b8013cb068370667e2e38a729b13bb358536378 Mon Sep 17 00:00:00 2001 From: Dev Agent Date: Wed, 3 Jun 2026 23:30:08 +0300 Subject: [PATCH] fix(stage): approved verdict advances analysis->architecture instead of re-running gate --- src/stage_engine.py | 66 ++++++++++++++++++++++---------------- tests/test_stage_engine.py | 57 ++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 27 deletions(-) diff --git a/src/stage_engine.py b/src/stage_engine.py index 43d762f..e6f4341 100644 --- a/src/stage_engine.py +++ b/src/stage_engine.py @@ -189,36 +189,48 @@ def advance_stage( # --- Quality gate ---------------------------------------------------- if qg_name and qg_name in QG_CHECKS: - # Human-approval gate: special analyst approved-flow (launcher only). + # Human-approval gate: split by path. if qg_name == "check_analysis_approved": - _handle_analysis_approved_flow( - task_id, current_stage, repo, work_item_id, branch, agent, result - ) - return result + # Launcher path (analyst just finished): set In Review + ask for + # the Approved status. This gate never advances on its own -- a + # human Approved verdict does that. + if agent == "analyst": + _handle_analysis_approved_flow( + task_id, current_stage, repo, work_item_id, branch, agent, result + ) + return result + # Webhook Approved-verdict path (agent is None): the human flipped + # the Plane status to Approved, which IS the approval. The gate is + # satisfied -- do NOT re-run check_analysis_approved (it looks for + # an :approved: *comment* and would block on a status-only + # approval). Mark it passed and fall through to the Advance block. + result.qg_name = qg_name + result.qg_passed = True + result.qg_reason = "approved-via-status" + else: + passed, reason = _run_qg(qg_name, repo, work_item_id, branch) + result.qg_passed = passed + result.qg_reason = reason - passed, reason = _run_qg(qg_name, repo, work_item_id, branch) - result.qg_passed = passed - result.qg_reason = reason + if not passed: + logger.info( + f"Task {task_id}: QG '{qg_name}' not passed after {agent}: {reason}" + ) + # Behaviour parity: + # - webhook path (finished_agent is None): emit the generic + # QG-failure notification, exactly like the old plane handler. + # - launcher path (finished_agent set): NO generic notification; + # the rollback branches below own their own messaging, exactly + # like the old launcher handler. + if agent is None: + notify_qg_failure(task_id, current_stage, qg_name, reason) + plane_notify_qg(work_item_id, current_stage, qg_name, reason) - if not passed: - logger.info( - f"Task {task_id}: QG '{qg_name}' not passed after {agent}: {reason}" - ) - # Behaviour parity: - # - webhook path (finished_agent is None): emit the generic - # QG-failure notification, exactly like the old plane handler. - # - launcher path (finished_agent set): NO generic notification; - # the rollback branches below own their own messaging, exactly - # like the old launcher handler. - if agent is None: - notify_qg_failure(task_id, current_stage, qg_name, reason) - plane_notify_qg(work_item_id, current_stage, qg_name, reason) - - _handle_qg_failure_rollbacks( - task_id, current_stage, repo, work_item_id, branch, - agent, qg_name, reason, result, - ) - return result + _handle_qg_failure_rollbacks( + task_id, current_stage, repo, work_item_id, branch, + agent, qg_name, reason, result, + ) + return result elif qg_name: # QG name set but not registered — do not advance (launcher behavior). diff --git a/tests/test_stage_engine.py b/tests/test_stage_engine.py index 47f7965..cb8eb0f 100644 --- a/tests/test_stage_engine.py +++ b/tests/test_stage_engine.py @@ -358,6 +358,63 @@ class TestAnalysisApprovedFlow: assert stage_engine.notify_approve_requested.called assert _jobs() == [] + def test_approved_verdict_advances_analysis_to_architecture(self, monkeypatch): + """BUG 4: a human Approved STATUS (webhook path, finished_agent=None) + must satisfy the analysis gate and advance analysis -> architecture, + enqueuing the architect. The status-only approval must NOT re-run + check_analysis_approved (which looks for an :approved: COMMENT and would + otherwise wrongly block the advance). + """ + # Make check_analysis_approved FAIL if it is ever called: the webhook + # path must bypass it entirely (status == approval). If the engine were + # to re-run the gate, this would block the advance and fail the test. + monkeypatch.setattr( + stage_engine, "QG_CHECKS", + { + **stage_engine.QG_CHECKS, + "check_analysis_approved": _fail("no :approved: comment"), + }, + ) + # Guard: the approval-flow (launcher-only) must NOT be invoked here. + flow = MagicMock() + monkeypatch.setattr(stage_engine, "_handle_analysis_approved_flow", flow) + + task_id = _make_task("analysis") + res = advance_stage( + task_id, "analysis", "enduro-trails", "ET-001", + "feature/ET-001-x", finished_agent=None, + ) + + assert res.advanced is True + assert res.to_stage == "architecture" + assert _stage(task_id) == "architecture" + assert res.enqueued_agent == "architect" + # Sanity: agent for analysis is architect, never analyst (no re-run loop). + assert get_agent_for_stage("analysis") == "architect" + jobs = _jobs() + assert len(jobs) == 1 + assert jobs[0]["agent"] == "architect" + # The launcher-only approval-flow was NOT called on the webhook path. + flow.assert_not_called() + + def test_launcher_path_does_not_advance_and_calls_flow(self, monkeypatch): + """Regression: the launcher path (finished_agent='analyst') still routes + into _handle_analysis_approved_flow and does NOT advance. + """ + flow = MagicMock() + monkeypatch.setattr(stage_engine, "_handle_analysis_approved_flow", flow) + + task_id = _make_task("analysis") + res = advance_stage( + task_id, "analysis", "enduro-trails", "ET-001", + "feature/ET-001-x", finished_agent="analyst", + ) + + assert res.advanced is not True + assert _stage(task_id) == "analysis" + assert _jobs() == [] + flow.assert_called_once() + # --------------------------------------------------------------------------- # launcher + plane both delegate to the engine -- 2.49.1