From 693b8c3403276d405fbd8886129fd93863840aab Mon Sep 17 00:00:00 2001 From: vladmesh <16962535+vladmesh@users.noreply.github.com> Date: Fri, 17 Jul 2026 00:50:41 +0300 Subject: [PATCH 1/2] fix: resolve acceptance criteria before handing a deploy to QA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-07-16 mega deployed successfully and its QA run, qa-88a6cc0c, died with qa_no_acceptance_criteria. Repository.acceptance_criteria was only ever written by the architect's update_acceptance_criteria tool, so a story that never went through the architect reached QA with nothing to test — and QA only discovered that after the story had already moved to TESTING and the run existed, leaving partially applied state behind a run that could only error. Make the repository the one source of truth and seed it. POST /api/repositories/ now writes BASELINE_ACCEPTANCE_CRITERIA, so every repository has the one check that holds for any deployed service from the moment it exists; the architect still extends the list as stories add functionality. Story and task criteria describe work to be done and are not what QA runs. Resolve the criteria in the producer, before any state changes. The deploy → QA handoff reads them and carries them on QAMessage, and the consumer no longer looks them up itself — that producer/consumer split is what lost them. Missing criteria now route to the same visible failure as a missing deployed_url or application_id: story failed, admins notified, no TESTING transition and no QA run. Admin run-e2e answers 422 instead of creating a run it cannot finish. The contract rejects blank criteria, which would otherwise parse to "no checks" and reach the agent with nothing to test. Criteria whose every line is a plain "GET returns " are decided over HTTP by the QA consumer, retrying while the service comes up. A health-only story therefore completes with outcome passed and no LLM, and no coding agent is paid for a check that is one request. The mega now gates on the QA run the pipeline produced rather than a health request it issues itself, which proved the service answers but not that QA had concluded anything. run_non_llm_qa moves to pipeline_helpers, where the run observation rules it needs already live: it reads /api/runs/ as an internal service with no user header, and looks the run up by story, since a project can carry QA runs of other stories. --- docs/CHANGELOG.md | 9 ++ docs/CONTRACTS.md | 11 +- services/api/src/routers/applications.py | 10 ++ services/api/src/routers/repositories.py | 2 + .../api/tests/service/test_admin_actions.py | 25 +++- services/langgraph/src/clients/api.py | 5 - .../langgraph/src/consumers/_qa_runner.py | 67 +++++++++- services/langgraph/src/consumers/qa.py | 55 ++++---- .../langgraph/tests/unit/test_qa_consumer.py | 118 ++++++++++++++---- .../langgraph/tests/unit/test_qa_runner.py | 117 ++++++++++++++++- services/scheduler/src/tasks/supervisor.py | 38 +++++- .../tests/unit/_run_routing_factories.py | 3 + .../tests/unit/test_supervisor_run_routing.py | 79 +++++++++++- shared/contracts/acceptance.py | 57 +++++++++ shared/contracts/queues/qa.py | 6 + shared/tests/test_acceptance.py | 54 ++++++++ shared/tests/unit/test_qa_contract.py | 44 +++++++ tests/live/live_harness.py | 27 ---- tests/live/pipeline_helpers.py | 60 +++++++++ tests/live/test_full_pipeline.py | 10 +- tests/live/test_harness_contract.py | 83 +++++++++--- 21 files changed, 770 insertions(+), 110 deletions(-) create mode 100644 shared/contracts/acceptance.py create mode 100644 shared/tests/test_acceptance.py diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e9ab7823..0f5ddd17 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,15 @@ ## 2026-07-16 +- Give post-deploy QA one source of truth for acceptance criteria. `Repository.acceptance_criteria` + is seeded with a baseline health check at repository creation, so an automatically created story + reaches QA without anyone filling the repository in by hand. The deploy → QA handoff resolves the + criteria and carries them on `QAMessage`; missing criteria now fail the story visibly before it + reaches TESTING (and answer 422 on admin `run-e2e`) instead of leaving a QA run that can only + error with `qa_no_acceptance_criteria`. Criteria stating only `GET returns ` are + checked over HTTP by the QA consumer, so a health-only story completes with outcome `passed` and + no LLM. The mega now gates on the QA run the pipeline itself produced rather than its own health + request. - Fix live server cleanup for service-template compose names that normalize project slugs with underscores. Teardown now discovers actual `com.docker.compose.project` labels from live containers, runs compose down for both manifest and discovered names, removes by label and diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 19657104..056098d4 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -72,7 +72,7 @@ cannot poison-loop the reclaim. |-------|-------|-----|-----------|----------|---------| | `engineering:queue` | `capability-workers` | EngineeringMessage | Task Dispatcher (scheduler) | langgraph | Start development task | | `deploy:queue` | `capability-workers` | DeployMessage | Task Dispatcher (scheduler) / PO | langgraph | Start deploy task | -| `qa:queue` | `qa-consumers` | QAMessage | Task Dispatcher (scheduler) / Admin API | langgraph (qa-worker) | Post-deploy QA testing via Claude Code on prod server | +| `qa:queue` | `qa-consumers` | QAMessage | Task Dispatcher (scheduler) / Admin API | langgraph (qa-worker) | Post-deploy QA: HTTP checks for GET-only criteria, else Claude Code on prod server | --- @@ -1035,12 +1035,19 @@ class QAMessage(BaseMessage): user_id: str deployed_url: str application_id: int + acceptance_criteria: str # resolved by the producer, never by the consumer run_id: str = "" bot_username: str | None = None qa_attempt: int = 0 ``` -**Flow:** Deploy succeeds → supervisor creates QA run → publishes QAMessage → QA consumer SSHes to prod server → runs Claude Code with QA prompt → writes `QAOutcome` to `run.result`. Supervisor polls run outcome and routes: PASSED → complete story, FAILED → create fix task + redispatch to engineering, EXHAUSTED/ERROR → fail story. +**Acceptance criteria:** `Repository.acceptance_criteria` is the single source of truth for what QA tests. `POST /api/repositories/` seeds every repository with `BASELINE_ACCEPTANCE_CRITERIA` (`shared/contracts/acceptance.py`), so a story that never reached the architect still has criteria; the architect's `update_acceptance_criteria` tool extends the list as stories add functionality. Story and task criteria describe work to be done and are not what QA runs. + +Producers (supervisor, admin `run-e2e`) resolve the criteria and put them on the message. Both refuse to create a QA run without them — the supervisor fails the story visibly before it reaches TESTING, and `run-e2e` answers 422 — so QA never starts a run it can only error out of. + +**Health-only criteria:** criteria whose every line is a plain `- GET returns ` are decided by the QA consumer over HTTP (`parse_health_only_criteria` → `run_health_checks`), with no SSH and no LLM. One prose line sends the whole block to Claude Code on the server instead. + +**Flow:** Deploy succeeds → supervisor resolves criteria → transitions story to TESTING → creates QA run → publishes QAMessage → QA consumer runs the criteria (HTTP checks, or Claude Code on the prod server) → writes `QAOutcome` to `run.result`. Supervisor polls run outcome and routes: PASSED → complete story, FAILED → create fix task + redispatch to engineering, EXHAUSTED/ERROR → fail story. **Lifecycle operations:** `stop` and `undeploy` actions are handled by the `deploy_lifecycle` module, which SSHes to the server and runs `docker compose stop/down` directly — skipping the full DevOps subgraph. diff --git a/services/api/src/routers/applications.py b/services/api/src/routers/applications.py index d6121540..12527bec 100644 --- a/services/api/src/routers/applications.py +++ b/services/api/src/routers/applications.py @@ -384,6 +384,15 @@ async def run_e2e( port = app.port_allocations[0].port if app.port_allocations else 0 deployed_url = f"http://{server.public_ip}:{port}" if port else f"http://{server.public_ip}" + # QA validates against the repository's criteria — reject before creating the + # Run, so a project without them doesn't leave a QA run that can only error. + acceptance_criteria = (repo.acceptance_criteria or "").strip() + if not acceptance_criteria: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail=f"Repository {repo.id} has no acceptance_criteria. Cannot run QA.", + ) + # Create Run run_id = f"qa-{uuid.uuid4().hex[:12]}" run = Run( @@ -403,6 +412,7 @@ async def run_e2e( user_id="", deployed_url=deployed_url, application_id=application_id, + acceptance_criteria=acceptance_criteria, run_id=run_id, bot_username=repo.bot_username, ) diff --git a/services/api/src/routers/repositories.py b/services/api/src/routers/repositories.py index b3cc1786..f0eb29c9 100644 --- a/services/api/src/routers/repositories.py +++ b/services/api/src/routers/repositories.py @@ -9,6 +9,7 @@ from sqlalchemy.ext.asyncio import AsyncSession import structlog +from shared.contracts.acceptance import BASELINE_ACCEPTANCE_CRITERIA from shared.models.project import Project from shared.models.repository import Repository @@ -54,6 +55,7 @@ async def create_repository( role=body.role.value, visibility=body.visibility.value, is_managed=body.is_managed, + acceptance_criteria=BASELINE_ACCEPTANCE_CRITERIA, created_at=now, updated_at=now, ) diff --git a/services/api/tests/service/test_admin_actions.py b/services/api/tests/service/test_admin_actions.py index 6d0c9ace..6a589291 100644 --- a/services/api/tests/service/test_admin_actions.py +++ b/services/api/tests/service/test_admin_actions.py @@ -12,6 +12,8 @@ import pytest from redis.asyncio import Redis +from shared.contracts.acceptance import BASELINE_ACCEPTANCE_CRITERIA + TASK_TEST_TELEGRAM_ID = 999000999 TASK_TEST_PROJECT_ID = "00000000-0000-0000-0000-000000000001" @@ -226,9 +228,9 @@ async def _create_repo(client) -> str: return resp.json()["id"] -async def _create_running_app(client, server_handle, app_status="running"): +async def _create_running_app(client, server_handle, app_status="running", repo_id=None): """Helper: create an application with a unique repo and a port.""" - rid = await _create_repo(client) + rid = repo_id or await _create_repo(client) svc_name = f"svc-{uuid.uuid4().hex[:6]}" resp = await client.post( "/api/applications/", @@ -328,6 +330,9 @@ async def test_run_e2e_on_running_app(self, client, redis, server_handle): msg = await _read_last_message(redis, "qa:queue") assert msg["application_id"] == app_id assert "10.0.0.1" in msg["deployed_url"] + # A repository is seeded with criteria at creation, so QA gets something + # to test without anyone filling the repository in by hand. + assert msg["acceptance_criteria"] == BASELINE_ACCEPTANCE_CRITERIA @pytest.mark.asyncio async def test_run_e2e_not_running_fails(self, client, server_handle): @@ -336,6 +341,22 @@ async def test_run_e2e_not_running_fails(self, client, server_handle): resp = await client.post(f"/api/applications/{app_id}/run-e2e") assert resp.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + @pytest.mark.asyncio + async def test_run_e2e_without_criteria_creates_no_run(self, client, server_handle): + """Criteria cleared → rejected before a Run exists, not a run that can only error.""" + rid = await _create_repo(client) + app_id = await _create_running_app(client, server_handle, repo_id=rid) + + resp = await client.patch(f"/api/repositories/{rid}", json={"acceptance_criteria": ""}) + assert resp.status_code == HTTPStatus.OK + + resp = await client.post(f"/api/applications/{app_id}/run-e2e", json={"actor": "test"}) + assert resp.status_code == HTTPStatus.UNPROCESSABLE_ENTITY + assert "acceptance_criteria" in resp.text + + runs = await client.get(f"/api/applications/{app_id}/runs") + assert runs.json() == [] + # --------------------------------------------------------------------------- # from-repo diff --git a/services/langgraph/src/clients/api.py b/services/langgraph/src/clients/api.py index ec53d6d1..14585ffb 100644 --- a/services/langgraph/src/clients/api.py +++ b/services/langgraph/src/clients/api.py @@ -260,11 +260,6 @@ async def get_primary_repository(self, project_id: str) -> RepositoryDTO | None: return repo return repos[0] if repos else None - async def get_repository(self, repo_id: str) -> RepositoryDTO: - """Get a single repository by ID.""" - resp = await self._request("GET", f"repositories/{repo_id}") - return RepositoryDTO.model_validate(resp.json()) - async def update_repository(self, repo_id: str, payload: dict) -> RepositoryDTO: """PATCH a repository.""" resp = await self._request("PATCH", f"repositories/{repo_id}", json=payload) diff --git a/services/langgraph/src/consumers/_qa_runner.py b/services/langgraph/src/consumers/_qa_runner.py index ab87ccbe..26252c02 100644 --- a/services/langgraph/src/consumers/_qa_runner.py +++ b/services/langgraph/src/consumers/_qa_runner.py @@ -1,7 +1,9 @@ -"""QA runner — SSH to prod server, run Claude Code, parse result. +"""QA runners — HTTP checks for criteria we can decide, Claude Code for the rest. -Delegates actual testing to Claude Code CLI running on the target server. -The prompt is built from the acceptance criteria and deployment URL. +Criteria that only state GET expectations are run directly against the deployed +URL by `run_health_checks`. Anything else goes to `run_qa_on_server`, which +delegates testing to the Claude Code CLI on the target server, prompted with the +acceptance criteria and deployment URL. """ from __future__ import annotations @@ -16,11 +18,16 @@ import httpx import structlog +from shared.contracts.acceptance import HealthCriterion + from ..prompts.qa import build_qa_prompt logger = structlog.get_logger(__name__) QA_TIMEOUT = 1200 # 20 minutes +HEALTH_CHECK_TIMEOUT = 30 +HEALTH_CHECK_ATTEMPTS = 5 +HEALTH_CHECK_RETRY_DELAY = 5 SERVICE_BASE_DIR = "/opt/services" CREDENTIALS_PATH = "$HOME/.claude/.credentials.json" LOCAL_CREDENTIALS_PATH = "/secrets/claude-credentials.json" # mounted from host @@ -90,6 +97,60 @@ def parse_qa_result(raw: str) -> QAResult: ) +async def run_health_checks( + *, + deployed_url: str, + checks: list[HealthCriterion], +) -> QAResult: + """Run GET criteria against the deployed URL. No SSH, no LLM. + + Each check is retried while the service is still coming up; a check that + never answers with its expected status fails the run. + """ + results = [] + async with httpx.AsyncClient(timeout=HEALTH_CHECK_TIMEOUT, follow_redirects=True) as client: + for check in checks: + results.append(await _run_health_check(client, deployed_url, check)) + + failed = [c for c in results if not c["pass"]] + passed = not failed + summary = ( + f"{len(results)} GET check(s) passed against {deployed_url}" + if passed + else f"{len(failed)}/{len(results)} GET check(s) failed against {deployed_url}" + ) + logger.info("qa_health_checks_done", deployed_url=deployed_url, passed=passed) + return QAResult( + passed=passed, + checks=results, + summary=summary, + report="\n".join(f"- {c['name']}: {c['detail']}" for c in results), + ) + + +async def _run_health_check( + client: httpx.AsyncClient, + deployed_url: str, + check: HealthCriterion, +) -> dict: + """GET one path, retrying until it answers as expected or attempts run out.""" + name = f"GET {check.path} returns {check.expected_status}" + detail = "no response" + for attempt in range(HEALTH_CHECK_ATTEMPTS): + if attempt: + await asyncio.sleep(HEALTH_CHECK_RETRY_DELAY) + try: + response = await client.get(f"{deployed_url.rstrip('/')}{check.path}") + except httpx.HTTPError as e: + detail = f"request failed: {e}" + continue + if response.status_code == check.expected_status: + return {"name": name, "pass": True, "detail": f"got {response.status_code}"} + detail = f"got {response.status_code}, expected {check.expected_status}" + logger.warning("qa_health_check_failed", path=check.path, detail=detail) + return {"name": name, "pass": False, "detail": detail} + + async def _ensure_claude_credentials(conn: asyncssh.SSHClientConnection) -> None: """Check Claude Code OAuth credentials on server, refresh if expired. diff --git a/services/langgraph/src/consumers/qa.py b/services/langgraph/src/consumers/qa.py index 99baf0fc..7ca07cde 100644 --- a/services/langgraph/src/consumers/qa.py +++ b/services/langgraph/src/consumers/qa.py @@ -11,6 +11,7 @@ import structlog +from shared.contracts.acceptance import parse_health_only_criteria from shared.contracts.dto.run import RunStatus from shared.contracts.dto.run_result import QAFailedCheck, QARunResult from shared.contracts.queues.qa import QAMessage, QAOutcome, QAServerInfo @@ -19,7 +20,12 @@ from ..clients.api import api_client from ._base import run_queue_worker, validate_queued_message -from ._qa_runner import QAResult, credential_refresh_loop, run_qa_on_server +from ._qa_runner import ( + QAResult, + credential_refresh_loop, + run_health_checks, + run_qa_on_server, +) logger = structlog.get_logger(__name__) @@ -117,37 +123,36 @@ async def process_qa_job(job_data: dict, redis: RedisStreamClient) -> dict: await _update_run(run_id, RunStatus.FAILED, QAOutcome.ERROR, error=error) return {"status": "error", "error": error} - # Resolve acceptance criteria from repository - app = await api_client.get_application(msg.application_id) - repo = await api_client.get_repository(app.repo_id) - acceptance_criteria = repo.acceptance_criteria or "" + # The criteria travel on the message — the producer resolves them from the + # repository before it creates this run. + acceptance_criteria = msg.acceptance_criteria - if not acceptance_criteria: - error = ( - f"Repository {repo.id} has no acceptance_criteria. " - "Cannot run QA without regression test criteria." - ) - logger.error("qa_no_acceptance_criteria", repo_id=repo.id) - await _update_run(run_id, RunStatus.FAILED, QAOutcome.ERROR, error=error) - return {"status": "error", "error": error} - - # Mark run as running before starting Claude Code + # Mark run as running before starting the checks if run_id: await api_client.patch( f"runs/{run_id}", json={"status": RunStatus.RUNNING.value}, ) - # Run QA on server - qa_result = await run_qa_on_server( - server_ip=server_info.server_ip, - ssh_user=server_info.ssh_user, - ssh_key=server_info.ssh_key, - project_name=server_info.project_name, - acceptance_criteria=acceptance_criteria, - deployed_url=msg.deployed_url, - bot_username=msg.bot_username, - ) + # Criteria that only state GET expectations are decidable over HTTP, so + # run them here rather than paying for a coding agent on the server. + health_checks = parse_health_only_criteria(acceptance_criteria) + if health_checks is not None: + logger.info("qa_health_only_criteria", story_id=story_id, checks=len(health_checks)) + qa_result = await run_health_checks( + deployed_url=msg.deployed_url, + checks=health_checks, + ) + else: + qa_result = await run_qa_on_server( + server_ip=server_info.server_ip, + ssh_user=server_info.ssh_user, + ssh_key=server_info.ssh_key, + project_name=server_info.project_name, + acceptance_criteria=acceptance_criteria, + deployed_url=msg.deployed_url, + bot_username=msg.bot_username, + ) logger.info( "qa_result", diff --git a/services/langgraph/tests/unit/test_qa_consumer.py b/services/langgraph/tests/unit/test_qa_consumer.py index d4682771..c68b92dd 100644 --- a/services/langgraph/tests/unit/test_qa_consumer.py +++ b/services/langgraph/tests/unit/test_qa_consumer.py @@ -14,7 +14,6 @@ from shared.contracts.dto.application import ApplicationDTO from shared.contracts.dto.project import ProjectDTO, ProjectStatus -from shared.contracts.dto.repository import RepositoryDTO from shared.contracts.dto.run import RunStatus from shared.contracts.dto.server import ServerDTO from shared.contracts.dto.story import StoryDTO @@ -25,6 +24,10 @@ process_qa_job, ) +# Criteria with a prose line — not decidable over HTTP, so QA hands these to the +# agent on the server. Tests that want the HTTP path override this. +AGENT_CRITERIA = "- GET /health returns 200\n- GET /api/weather returns forecast" + def _application(**overrides) -> ApplicationDTO: base = { @@ -94,22 +97,6 @@ def mock_api_client(): mock.get_server_ssh_key = AsyncMock( return_value="-----BEGIN RSA KEY-----\nfake\n-----END RSA KEY-----" ) - mock.get_repository = AsyncMock( - return_value=RepositoryDTO( - id="repo-1", - project_id="116c9678-5872-4ce5-8332-9a267ab27604", - name="weather_bot", - git_url="https://github.com/test/weather_bot.git", - role="primary", - visibility="private", - is_managed=True, - acceptance_criteria=( - "- GET /health returns 200\n- GET /api/weather returns forecast" - ), - created_at=datetime.now(UTC), - updated_at=datetime.now(UTC), - ) - ) mock.patch = AsyncMock(return_value={}) mock.create_task = AsyncMock(return_value={"id": "task-fix-1"}) yield mock @@ -134,6 +121,7 @@ def qa_message_data(): "user_id": "12345", "deployed_url": "https://weather.example.com", "application_id": 1, + "acceptance_criteria": AGENT_CRITERIA, "run_id": "qa-run-1", "bot_username": None, "qa_attempt": 0, @@ -297,6 +285,89 @@ async def test_max_qa_loops_stores_exhausted_outcome( assert run_data["result"]["qa_outcome"] == QAOutcome.EXHAUSTED.value +class TestHealthOnlyCriteriaRouting: + """Criteria that only state GET expectations are decided over HTTP, no agent.""" + + @pytest.mark.asyncio + async def test_health_only_criteria_pass_without_the_agent( + self, mock_api_client, mock_redis, qa_message_data + ): + """A health-only story (the mega case) completes with outcome passed.""" + from src.consumers._qa_runner import QAResult + + qa_message_data["acceptance_criteria"] = "- GET /health returns 200" + + with ( + patch("src.consumers.qa.run_health_checks", new_callable=AsyncMock) as mock_health, + patch("src.consumers.qa.run_qa_on_server", new_callable=AsyncMock) as mock_agent, + ): + mock_health.return_value = QAResult( + passed=True, + checks=[{"name": "GET /health returns 200", "pass": True, "detail": "got 200"}], + summary="1 GET check(s) passed", + ) + result = await process_qa_job(qa_message_data, mock_redis) + + assert result["status"] == "passed" + mock_agent.assert_not_called() + + checks = mock_health.call_args[1]["checks"] + assert [(c.path, c.expected_status) for c in checks] == [("/health", 200)] + assert mock_health.call_args[1]["deployed_url"] == "https://weather.example.com" + + completed_call = mock_api_client.patch.call_args_list[-1] + run_data = completed_call[1]["json"] + assert run_data["status"] == RunStatus.COMPLETED.value + assert run_data["result"]["qa_outcome"] == QAOutcome.PASSED.value + + @pytest.mark.asyncio + async def test_failing_health_check_stores_failed_outcome( + self, mock_api_client, mock_redis, qa_message_data + ): + from src.consumers._qa_runner import QAResult + + qa_message_data["acceptance_criteria"] = "- GET /health returns 200" + + with patch("src.consumers.qa.run_health_checks", new_callable=AsyncMock) as mock_health: + mock_health.return_value = QAResult( + passed=False, + checks=[ + { + "name": "GET /health returns 200", + "pass": False, + "detail": "got 502, expected 200", + } + ], + summary="1/1 GET check(s) failed", + ) + result = await process_qa_job(qa_message_data, mock_redis) + + assert result["status"] == "qa_failed" + run_data = mock_api_client.patch.call_args[1]["json"] + assert run_data["result"]["qa_outcome"] == QAOutcome.FAILED.value + assert run_data["result"]["failed_checks"][0]["detail"] == "got 502, expected 200" + + @pytest.mark.asyncio + async def test_prose_criteria_still_go_to_the_agent( + self, mock_api_client, mock_redis, qa_message_data + ): + """Only fully machine-checkable criteria skip the agent.""" + from src.consumers._qa_runner import QAResult + + qa_message_data["acceptance_criteria"] = AGENT_CRITERIA + + with ( + patch("src.consumers.qa.run_health_checks", new_callable=AsyncMock) as mock_health, + patch("src.consumers.qa.run_qa_on_server", new_callable=AsyncMock) as mock_agent, + ): + mock_agent.return_value = QAResult(passed=True, checks=[], summary="OK", raw="") + result = await process_qa_job(qa_message_data, mock_redis) + + assert result["status"] == "passed" + mock_health.assert_not_called() + mock_agent.assert_called_once() + + class TestProcessQAJobEdgeCases: @pytest.mark.asyncio async def test_application_not_found(self, mock_api_client, mock_redis, qa_message_data): @@ -335,6 +406,7 @@ async def test_inflight_dedup_uses_application_id_when_no_story( "user_id": "12345", "deployed_url": "https://weather.example.com", "application_id": 42, + "acceptance_criteria": AGENT_CRITERIA, "run_id": "qa-run-1", "qa_attempt": 0, } @@ -350,8 +422,12 @@ async def test_inflight_dedup_uses_application_id_when_no_story( assert inflight_key != "qa:inflight:" # not empty @pytest.mark.asyncio - async def test_standalone_qa_uses_acceptance_criteria(self, mock_api_client, mock_redis): - """Standalone QA (no story_id) uses repo acceptance_criteria, not story description.""" + async def test_qa_runs_the_criteria_from_the_message(self, mock_api_client, mock_redis): + """QA tests against the criteria the producer resolved, not its own lookup. + + The producer resolves them from the repository before creating the run, so + the consumer must not re-read them — that split is what lost them before. + """ from src.consumers._qa_runner import QAResult data = { @@ -360,6 +436,7 @@ async def test_standalone_qa_uses_acceptance_criteria(self, mock_api_client, moc "user_id": "12345", "deployed_url": "https://weather.example.com", "application_id": 1, + "acceptance_criteria": AGENT_CRITERIA, "run_id": "qa-run-1", "qa_attempt": 0, } @@ -369,8 +446,7 @@ async def test_standalone_qa_uses_acceptance_criteria(self, mock_api_client, moc result = await process_qa_job(data, mock_redis) assert result["status"] == "passed" - # Acceptance criteria fetched from repo, not story - mock_api_client.get_repository.assert_called_once() + assert mock_run.call_args[1]["acceptance_criteria"] == AGENT_CRITERIA mock_api_client.get_story.assert_not_called() @pytest.mark.asyncio diff --git a/services/langgraph/tests/unit/test_qa_runner.py b/services/langgraph/tests/unit/test_qa_runner.py index 230f7cc3..bef75040 100644 --- a/services/langgraph/tests/unit/test_qa_runner.py +++ b/services/langgraph/tests/unit/test_qa_runner.py @@ -1,12 +1,15 @@ -"""Unit tests for QA runner — SSH to server, run Claude Code, parse result.""" +"""Unit tests for QA runner — HTTP health checks, SSH to server, parse result.""" from __future__ import annotations from unittest.mock import AsyncMock, MagicMock, patch +import httpx import pytest +import respx -from src.consumers._qa_runner import parse_qa_result, run_qa_on_server +from shared.contracts.acceptance import HealthCriterion +from src.consumers._qa_runner import parse_qa_result, run_health_checks, run_qa_on_server from src.prompts.qa import build_qa_prompt @@ -111,6 +114,116 @@ def test_output_format_json_wrapper_non_json_result(self): assert "parse" in result.summary.lower() or "failed" in result.summary.lower() +class TestRunHealthChecks: + """GET criteria are decided against the deployed URL — no SSH, no LLM.""" + + @pytest.fixture(autouse=True) + def _no_retry_delay(self): + """Keep the retry loop's timing out of the test's wall clock.""" + with patch("src.consumers._qa_runner.HEALTH_CHECK_RETRY_DELAY", 0): + yield + + @respx.mock + @pytest.mark.asyncio + async def test_http_200_passes(self): + """The mega health-only case: service answers 200 → QA passes.""" + route = respx.get("http://svc.example.com/health").mock(return_value=httpx.Response(200)) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is True + assert route.called + assert result.checks == [ + {"name": "GET /health returns 200", "pass": True, "detail": "got 200"} + ] + assert "http://svc.example.com" in result.summary + + @respx.mock + @pytest.mark.asyncio + async def test_trailing_slash_does_not_double_up(self): + respx.get("http://svc.example.com/health").mock(return_value=httpx.Response(200)) + + result = await run_health_checks( + deployed_url="http://svc.example.com/", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is True + + @respx.mock + @pytest.mark.asyncio + async def test_wrong_status_fails_with_detail(self): + respx.get("http://svc.example.com/health").mock(return_value=httpx.Response(502)) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is False + assert result.checks[0]["pass"] is False + assert result.checks[0]["detail"] == "got 502, expected 200" + + @respx.mock + @pytest.mark.asyncio + async def test_retries_while_the_service_comes_up(self): + """A service still starting must not fail the run on the first 503.""" + route = respx.get("http://svc.example.com/health").mock( + side_effect=[ + httpx.Response(503), + httpx.ConnectError("connection refused"), + httpx.Response(200), + ] + ) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is True + assert route.call_count == 3 + + @respx.mock + @pytest.mark.asyncio + async def test_unreachable_service_fails_after_attempts(self): + from src.consumers._qa_runner import HEALTH_CHECK_ATTEMPTS + + route = respx.get("http://svc.example.com/health").mock( + side_effect=httpx.ConnectError("connection refused") + ) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is False + assert route.call_count == HEALTH_CHECK_ATTEMPTS + assert "request failed" in result.checks[0]["detail"] + + @respx.mock + @pytest.mark.asyncio + async def test_one_failing_check_fails_the_run(self): + respx.get("http://svc.example.com/health").mock(return_value=httpx.Response(200)) + respx.get("http://svc.example.com/ready").mock(return_value=httpx.Response(404)) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[ + HealthCriterion(path="/health", expected_status=200), + HealthCriterion(path="/ready", expected_status=200), + ], + ) + + assert result.passed is False + assert [c["pass"] for c in result.checks] == [True, False] + assert "1/2" in result.summary + + class TestRunQAOnServer: @pytest.fixture(autouse=True) def _skip_credential_refresh(self): diff --git a/services/scheduler/src/tasks/supervisor.py b/services/scheduler/src/tasks/supervisor.py index 55d0bf2c..5035ac40 100644 --- a/services/scheduler/src/tasks/supervisor.py +++ b/services/scheduler/src/tasks/supervisor.py @@ -354,8 +354,8 @@ async def _handle_deploy_success_story( ) -> bool: """Deploy succeeded — transition story to TESTING, create QA run, publish QA message. - Returns True if the story was handed off to QA, False if the success result - lacked the fields QA needs (handled as a visible failure). + Returns True if the story was handed off to QA, False if QA's preconditions + were not met (handled as a visible failure). """ deployed_url = result.deployed_url application_id = result.application_id @@ -379,6 +379,21 @@ async def _handle_deploy_success_story( ) return False + # QA validates the story against the repository's criteria, so resolve them + # here and carry them on the message. Same reason as the fields above: a + # story whose criteria are missing must not reach TESTING with a QA run that + # can only error out. + acceptance_criteria = await _resolve_acceptance_criteria(api_client, project_id, log) + if acceptance_criteria is None: + await api_client.fail_story(story_id) + await _notify_admin_failure( + story_id, + project_id, + "deploy succeeded but the project's repository has no acceptance criteria — " + "cannot run QA", + ) + return False + await api_client.transition_story(story_id, "test") # Create QA run so the consumer can store its outcome @@ -401,6 +416,7 @@ async def _handle_deploy_success_story( user_id="", deployed_url=deployed_url, application_id=application_id, + acceptance_criteria=acceptance_criteria, bot_username=bot_username, run_id=qa_run_id, ), @@ -409,6 +425,24 @@ async def _handle_deploy_success_story( return True +async def _resolve_acceptance_criteria( + api_client: SchedulerAPIClient, + project_id: str, + log: structlog.stdlib.BoundLogger, +) -> str | None: + """Read the project's QA criteria, or None if there are none to run.""" + repo = await api_client.get_primary_repository(project_id) + if repo is None: + log.error("deploy_success_no_primary_repository") + return None + + criteria = (repo.acceptance_criteria or "").strip() + if not criteria: + log.error("deploy_success_no_acceptance_criteria", repo_id=repo.id) + return None + return criteria + + async def _handle_deploy_code_fix( api_client: SchedulerAPIClient, redis_client: RedisStreamClient, diff --git a/services/scheduler/tests/unit/_run_routing_factories.py b/services/scheduler/tests/unit/_run_routing_factories.py index 3c185ee4..d780143e 100644 --- a/services/scheduler/tests/unit/_run_routing_factories.py +++ b/services/scheduler/tests/unit/_run_routing_factories.py @@ -11,6 +11,7 @@ from pydantic import ValidationError +from shared.contracts.acceptance import BASELINE_ACCEPTANCE_CRITERIA from shared.contracts.dto.repository import RepositoryDTO from shared.contracts.dto.run import RunDTO, RunStatus, RunType from shared.contracts.dto.story import StoryDTO @@ -77,6 +78,7 @@ def _make_repo( role: str = "primary", visibility: str = "private", is_managed: bool = True, + acceptance_criteria: str | None = BASELINE_ACCEPTANCE_CRITERIA, created_at: datetime | None = None, updated_at: datetime | None = None, ) -> RepositoryDTO: @@ -88,6 +90,7 @@ def _make_repo( role=role, visibility=visibility, is_managed=is_managed, + acceptance_criteria=acceptance_criteria, created_at=created_at or _NOW, updated_at=updated_at, ) diff --git a/services/scheduler/tests/unit/test_supervisor_run_routing.py b/services/scheduler/tests/unit/test_supervisor_run_routing.py index 37c0efa0..e82b765d 100644 --- a/services/scheduler/tests/unit/test_supervisor_run_routing.py +++ b/services/scheduler/tests/unit/test_supervisor_run_routing.py @@ -11,12 +11,14 @@ # Sibling test-helper module (not a test module); on sys.path via pytest prepend import mode. from _run_routing_factories import ( _invalid_result_error, + _make_repo, _make_run, _make_story, _terminal_no_result_error, ) import pytest +from shared.contracts.acceptance import BASELINE_ACCEPTANCE_CRITERIA from shared.contracts.dto.run import RunStatus, RunType from shared.contracts.queues.deploy import DeployOutcome from shared.contracts.queues.qa import QAOutcome @@ -24,7 +26,10 @@ @pytest.fixture def api_client(): - return AsyncMock() + client = AsyncMock() + # QA runs the repository's criteria, so the deploy→QA handoff resolves them. + client.get_primary_repository.return_value = _make_repo() + return client @pytest.fixture @@ -85,6 +90,78 @@ async def test_success_transitions_to_testing(self, api_client, redis_client): assert qa_msg.deployed_url == "https://example.com" assert qa_msg.application_id == 42 assert qa_msg.run_id # run_id must be set + # The criteria travel on the message — QA does not resolve them itself. + assert qa_msg.acceptance_criteria == BASELINE_ACCEPTANCE_CRITERIA + + @pytest.mark.asyncio + async def test_criteria_are_resolved_before_the_story_moves(self, api_client, redis_client): + """The handoff carries the repository's criteria, whatever they say.""" + from src.tasks.supervisor import supervise_deploying_stories + + criteria = "- GET /health returns 200\n- Telegram: /start responds with welcome" + api_client.get_stories_by_status.return_value = [ + _make_story(id="story-1", status="deploying") + ] + api_client.get_latest_run_by_story.return_value = _make_run( + result={ + "deploy_outcome": DeployOutcome.SUCCESS.value, + "deployed_url": "https://example.com", + "application_id": 42, + }, + ) + api_client.get_primary_repository.return_value = _make_repo(acceptance_criteria=criteria) + api_client.transition_story.return_value = {} + api_client.create_run.return_value = {"id": "qa-run-1"} + + await supervise_deploying_stories(api_client, redis_client) + + from shared.queues import QA_QUEUE + + qa_calls = [c for c in redis_client.publish_message.call_args_list if c[0][0] == QA_QUEUE] + assert qa_calls[0][0][1].acceptance_criteria == criteria + + @pytest.mark.parametrize( + ("repo", "case"), + [ + (None, "no primary repository"), + (_make_repo(acceptance_criteria=None), "criteria never set"), + (_make_repo(acceptance_criteria=" \n"), "criteria blank"), + ], + ) + @pytest.mark.asyncio + async def test_success_without_criteria_fails_story(self, api_client, redis_client, repo, case): + """No criteria → visible failure before TESTING, not a QA run that can only error. + + This is the `qa_no_acceptance_criteria` case: QA used to discover it after + the story had already moved and a run had been created. + """ + from src.tasks.supervisor import supervise_deploying_stories + + api_client.get_stories_by_status.return_value = [ + _make_story(id="story-1", status="deploying") + ] + api_client.get_latest_run_by_story.return_value = _make_run( + result={ + "deploy_outcome": DeployOutcome.SUCCESS.value, + "deployed_url": "https://example.com", + "application_id": 42, + }, + ) + api_client.get_primary_repository.return_value = repo + api_client.fail_story.return_value = {} + + with patch( + "src.tasks.supervisor.notify_admins_best_effort", new_callable=AsyncMock + ) as mock_notify: + result = await supervise_deploying_stories(api_client, redis_client) + + assert result["failed"] == 1, case + api_client.fail_story.assert_called_once_with("story-1") + mock_notify.assert_called_once() + # No partial state: no story transition, no QA run created, no QA message. + api_client.transition_story.assert_not_called() + api_client.create_run.assert_not_called() + redis_client.publish_message.assert_not_called() @pytest.mark.asyncio async def test_success_without_application_id_fails_story(self, api_client, redis_client): diff --git a/shared/contracts/acceptance.py b/shared/contracts/acceptance.py new file mode 100644 index 00000000..e42a105a --- /dev/null +++ b/shared/contracts/acceptance.py @@ -0,0 +1,57 @@ +"""Acceptance criteria — the contract QA validates a deployed story against. + +`Repository.acceptance_criteria` is the single source of truth: it holds the +accumulated regression checklist for the project, seeded when the repository is +created and extended by the architect as stories add functionality. Story and +task criteria describe work to be done; they are not what QA runs. + +Format is one check per line, starting with "- ". Checks that state a plain GET +expectation are machine-checkable, so QA runs them itself over HTTP instead of +handing the criteria to a coding agent. +""" + +from __future__ import annotations + +import re + +from pydantic import BaseModel + +#: Every repository starts with the one check that holds for any deployed +#: service. Seeded at repository creation so QA has criteria for a story that +#: never went through the architect. +BASELINE_ACCEPTANCE_CRITERIA = "- GET /health returns 200" + +_HEALTH_CHECK_RE = re.compile( + r"^-\s*GET\s+(?P/\S*)\s+returns\s+(?P\d{3})$", + re.IGNORECASE, +) + + +class HealthCriterion(BaseModel): + """A GET check QA can verify without an LLM.""" + + path: str + expected_status: int + + +def parse_health_only_criteria(criteria: str) -> list[HealthCriterion] | None: + """Parse criteria into GET checks, or None if any line needs an LLM. + + Returns None unless *every* check is a plain GET expectation — a criteria + block with one prose line is not something HTTP calls can decide. + """ + checks = [] + for line in criteria.splitlines(): + stripped = line.strip() + if not stripped: + continue + match = _HEALTH_CHECK_RE.match(stripped) + if not match: + return None + checks.append( + HealthCriterion( + path=match.group("path"), + expected_status=int(match.group("expected_status")), + ) + ) + return checks or None diff --git a/shared/contracts/queues/qa.py b/shared/contracts/queues/qa.py index 717049c5..9368a688 100644 --- a/shared/contracts/queues/qa.py +++ b/shared/contracts/queues/qa.py @@ -1,6 +1,8 @@ from dataclasses import dataclass from enum import StrEnum +from pydantic import Field + from shared.contracts.base import BaseMessage from shared.contracts.dto.server import SSHUser @@ -22,6 +24,10 @@ class QAMessage(BaseMessage): user_id: str deployed_url: str application_id: int + # What QA tests the deployment against, resolved from the repository by the + # producer. Blank would parse to "no checks" and quietly reach the agent with + # nothing to test, so the contract rejects it rather than QA discovering it. + acceptance_criteria: str = Field(min_length=1) run_id: str = "" bot_username: str | None = None qa_attempt: int = 0 diff --git a/shared/tests/test_acceptance.py b/shared/tests/test_acceptance.py new file mode 100644 index 00000000..65b90577 --- /dev/null +++ b/shared/tests/test_acceptance.py @@ -0,0 +1,54 @@ +"""Tests for acceptance criteria parsing — which criteria QA can decide over HTTP.""" + +from __future__ import annotations + +import pytest + +from shared.contracts.acceptance import ( + BASELINE_ACCEPTANCE_CRITERIA, + parse_health_only_criteria, +) + + +class TestParseHealthOnlyCriteria: + def test_baseline_criteria_are_health_only(self): + """The criteria every repository is seeded with must not need an LLM.""" + checks = parse_health_only_criteria(BASELINE_ACCEPTANCE_CRITERIA) + + assert checks is not None + assert len(checks) == 1 + assert checks[0].path == "/health" + assert checks[0].expected_status == 200 + + def test_parses_several_get_checks(self): + checks = parse_health_only_criteria( + "- GET /health returns 200\n- GET /api/cities returns 404\n" + ) + + assert [(c.path, c.expected_status) for c in checks] == [ + ("/health", 200), + ("/api/cities", 404), + ] + + def test_blank_lines_are_ignored(self): + checks = parse_health_only_criteria("\n- GET /health returns 200\n\n") + + assert len(checks) == 1 + + @pytest.mark.parametrize( + "criteria", + [ + # One prose line makes the whole block undecidable over HTTP. + "- GET /health returns 200\n- Telegram: /start responds with welcome message", + "- POST /api/cities with {'name': 'Moscow'} returns 201", + "- GET /api/weather returns forecast", + "- The service starts without errors", + ], + ) + def test_criteria_needing_an_agent_return_none(self, criteria): + assert parse_health_only_criteria(criteria) is None + + def test_empty_criteria_return_none(self): + """No criteria is not 'zero checks that trivially pass'.""" + assert parse_health_only_criteria("") is None + assert parse_health_only_criteria(" \n\n") is None diff --git a/shared/tests/unit/test_qa_contract.py b/shared/tests/unit/test_qa_contract.py index 13d6cc98..904e4def 100644 --- a/shared/tests/unit/test_qa_contract.py +++ b/shared/tests/unit/test_qa_contract.py @@ -1,9 +1,14 @@ """Unit tests for QA queue message contract and queue topology.""" +from pydantic import ValidationError +import pytest + from shared.contracts.dto.run import RunType from shared.contracts.queues.qa import QAMessage, QAOutcome from shared.queues import QA_GROUP, QA_QUEUE, QUEUE_TOPOLOGY +CRITERIA = "- GET /health returns 200" + class TestQAMessage: def test_minimal_construction(self): @@ -13,6 +18,7 @@ def test_minimal_construction(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, ) assert msg.story_id == "story-abc" assert msg.project_id == "proj-123" @@ -27,6 +33,7 @@ def test_defaults(self): user_id="user-1", deployed_url="https://example.com", application_id=1, + acceptance_criteria=CRITERIA, ) assert msg.qa_attempt == 0 assert msg.bot_username is None @@ -42,6 +49,7 @@ def test_with_bot_username(self): user_id="user-1", deployed_url="https://example.com", application_id=1, + acceptance_criteria=CRITERIA, bot_username="my_test_bot", ) assert msg.bot_username == "my_test_bot" @@ -53,6 +61,7 @@ def test_qa_attempt_increment(self): user_id="user-1", deployed_url="https://example.com", application_id=1, + acceptance_criteria=CRITERIA, qa_attempt=2, ) assert msg.qa_attempt == 2 @@ -64,6 +73,7 @@ def test_serialization_roundtrip(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, bot_username="bot", qa_attempt=1, ) @@ -74,6 +84,34 @@ def test_serialization_roundtrip(self): assert restored.qa_attempt == msg.qa_attempt assert restored.bot_username == msg.bot_username assert restored.application_id == 17 + assert restored.acceptance_criteria == CRITERIA + + def test_acceptance_criteria_is_required(self): + """QA can't run without criteria, so a message may not omit them. + + The producer resolves them from the repository; a message that reaches the + queue without them is a bug that must surface here, not in the consumer. + """ + with pytest.raises(ValidationError): + QAMessage( + story_id="story-abc", + project_id="proj-123", + user_id="user-1", + deployed_url="https://example.com", + application_id=17, + ) + + def test_acceptance_criteria_may_not_be_blank(self): + """Blank criteria parse to "no checks" — reject them at the contract.""" + with pytest.raises(ValidationError): + QAMessage( + story_id="story-abc", + project_id="proj-123", + user_id="user-1", + deployed_url="https://example.com", + application_id=17, + acceptance_criteria="", + ) class TestRunTypeQA: @@ -108,6 +146,7 @@ def test_run_id_field(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, run_id="qa-run-001", ) assert msg.run_id == "qa-run-001" @@ -119,6 +158,7 @@ def test_run_id_roundtrip(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, run_id="qa-run-002", ) data = msg.model_dump() @@ -134,6 +174,7 @@ def test_story_id_defaults_to_empty(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, ) assert msg.story_id == "" @@ -144,6 +185,7 @@ def test_story_id_explicit(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, ) assert msg.story_id == "story-abc" @@ -154,6 +196,7 @@ def test_standalone_roundtrip(self): user_id="user-1", deployed_url="https://example.com", application_id=17, + acceptance_criteria=CRITERIA, ) data = msg.model_dump() restored = QAMessage.model_validate(data) @@ -166,6 +209,7 @@ def test_backward_compat_no_story_id_in_dict(self): "user_id": "user-1", "deployed_url": "https://example.com", "application_id": 17, + "acceptance_criteria": CRITERIA, } msg = QAMessage.model_validate(data) assert msg.story_id == "" diff --git a/tests/live/live_harness.py b/tests/live/live_harness.py index 911f7899..39ab51c9 100644 --- a/tests/live/live_harness.py +++ b/tests/live/live_harness.py @@ -1,15 +1,12 @@ """Safety contracts shared by Stage 7 live tests.""" -import asyncio from collections.abc import Awaitable, Callable from contextlib import asynccontextmanager from dataclasses import dataclass, field import json import os from pathlib import Path -import time -import httpx import structlog logger = structlog.get_logger() @@ -163,27 +160,3 @@ def teardown( ) if errors: raise CleanupError("owned-resource cleanup failed: " + "; ".join(errors)) - - -async def run_non_llm_qa( - client: httpx.AsyncClient, - deployed_url: str, - *, - timeout: float, - poll_interval: float = 3, -) -> dict[str, str]: - """Run an observable health-only QA gate and require terminal ``passed``.""" - run_id = f"health-{int(time.time())}" - deadline = time.monotonic() + timeout - while time.monotonic() < deadline: - for path in ("/health", "/v1/health"): - try: - response = await client.get(f"{deployed_url}{path}") - except httpx.HTTPError: - continue - if response.status_code == 200: - return {"run_id": run_id, "status": "completed", "qa_outcome": "passed"} - await asyncio.sleep(poll_interval) - raise AssertionError( - f"non-LLM QA run {run_id} ended with status=failed outcome=failed after {timeout}s" - ) diff --git a/tests/live/pipeline_helpers.py b/tests/live/pipeline_helpers.py index 34bb08df..4e4cd110 100644 --- a/tests/live/pipeline_helpers.py +++ b/tests/live/pipeline_helpers.py @@ -26,6 +26,7 @@ from shared.contracts.dto.run_result import DeployRunResult from shared.contracts.dto.story import StoryStatus from shared.contracts.dto.task import TaskStatus +from shared.contracts.queues.qa import QAOutcome from shared.queues import SCAFFOLD_QUEUE # ── Constants ──────────────────────────────────────────────────────────── @@ -55,6 +56,10 @@ # status, so this only covers that last write. DEPLOY_OUTCOME_TIMEOUT = 120 DEPLOY_OUTCOME_POLL_INTERVAL = 3 +# Deploy hands off to QA on the scheduler's next poll, then QA retries the health +# check while the service finishes coming up. +QA_RUN_TIMEOUT = 300 +QA_RUN_POLL_INTERVAL = 5 WORKER_REMOVAL_TIMEOUT = 15 WORKER_REMOVAL_POLL_INTERVAL = 0.25 RUN_CANCELLATION_TIMEOUT = 30 @@ -691,6 +696,61 @@ async def wait_deploy_outcome( return result +# ── QA run outcome ─────────────────────────────────────────────────────── + + +async def run_non_llm_qa( + api_internal: httpx.AsyncClient, + story_id: str, + *, + timeout: float, + poll_interval: float = QA_RUN_POLL_INTERVAL, +) -> dict[str, str]: + """Wait for this story's QA run and require a terminal ``passed``. + + The scheduler hands a successful deploy off to QA, and the QA consumer runs + the repository's criteria — for a scaffolded project those are the seeded + health check, which QA decides over HTTP with no LLM involved. The gate reads + the run the pipeline produced: a health request issued by this test would + prove the service answers, not that QA concluded anything about it. + + Reads /api/runs/ as an internal service with no user header — see + ``require_unscoped_run_observer``. + """ + require_unscoped_run_observer(api_internal) + deadline = time.monotonic() + timeout + run = None + while time.monotonic() < deadline: + resp = await api_internal.get( + "/api/runs/", + params={"story_id": story_id, "run_type": RunType.QA.value}, + headers=internal_headers(), + ) + resp.raise_for_status() + # The API orders runs newest first. A project can carry QA runs of other + # stories, so the run is checked against this mega's story too. + for candidate in resp.json(): + if candidate["story_id"] == story_id and candidate["status"] in _TERMINAL_RUN_STATUSES: + run = candidate + break + if run is not None: + break + await asyncio.sleep(poll_interval) + else: + raise AssertionError( + f"no QA run reached a terminal state for story {story_id} in {timeout}s" + ) + + result = run["result"] or {} + outcome = result.get("qa_outcome") + if run["status"] != "completed" or outcome != QAOutcome.PASSED.value: + raise AssertionError( + f"QA run {run['id']} ended with status={run['status']} outcome={outcome}: " + f"{result.get('summary') or result.get('error')}" + ) + return {"run_id": run["id"], "status": run["status"], "qa_outcome": outcome} + + # ── Cleanup helpers ────────────────────────────────────────────────────── diff --git a/tests/live/test_full_pipeline.py b/tests/live/test_full_pipeline.py index e598d84e..904506d5 100644 --- a/tests/live/test_full_pipeline.py +++ b/tests/live/test_full_pipeline.py @@ -17,7 +17,7 @@ import asyncio import httpx -from live_harness import cleanup_guard, run_non_llm_qa +from live_harness import cleanup_guard from pipeline_helpers import ( API_URL, AUTH_HEADERS, @@ -26,6 +26,7 @@ DEPLOY_TIMEOUT, ENGINEERING_TIMEOUT, EXPECTED_ENV_CONTRACT_FRAGMENTS, + QA_RUN_TIMEOUT, SCAFFOLD_TIMEOUT, cleanup_all, create_noop_project, @@ -34,6 +35,7 @@ ensure_test_user, internal_headers, record_env_contract, + run_non_llm_qa, trigger_scaffold, wait_deploy, wait_deploy_outcome, @@ -119,9 +121,9 @@ async def pipeline(): and ctx.get("deploy_outcome") == DeployOutcome.SUCCESS.value ): ctx["qa_result"] = await run_non_llm_qa( - api_no_auth, - ctx["deployed_url"], - timeout=DEPLOY_TIMEOUT, + api_internal, + ctx["story_id"], + timeout=QA_RUN_TIMEOUT, ) yield ctx diff --git a/tests/live/test_harness_contract.py b/tests/live/test_harness_contract.py index 73f9969d..e33038bd 100644 --- a/tests/live/test_harness_contract.py +++ b/tests/live/test_harness_contract.py @@ -15,13 +15,13 @@ OwnershipManifest, cleanup_guard, resolve_repo_root, - run_non_llm_qa, ) import pipeline_helpers from pipeline_helpers import ( _build_server_remote_cleanup_command, build_github_cleanup_script, build_registry_cleanup_script, + run_non_llm_qa, ) import pytest import structlog @@ -973,28 +973,79 @@ async def wait_for_runs(*args, **kwargs): ] -@pytest.mark.asyncio -async def test_qa_gate_requires_separate_passed_terminal_run(): - async def handler(request: httpx.Request) -> httpx.Response: - return httpx.Response(200) +def _qa_run(**overrides) -> dict: + run = { + "id": "qa-1", + "story_id": "story-1", + "status": "completed", + "result": {"qa_outcome": "passed", "summary": "1 GET check(s) passed"}, + } + run.update(overrides) + return run - transport = httpx.MockTransport(handler) - async with httpx.AsyncClient(transport=transport, base_url="http://test") as api: - result = await run_non_llm_qa(api, "http://deployed", timeout=1, poll_interval=0) - assert result["status"] == "completed" - assert result["qa_outcome"] == "passed" +def _runs_transport(runs: list[dict]) -> httpx.MockTransport: + async def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json=runs) + + return httpx.MockTransport(handler) @pytest.mark.asyncio -async def test_qa_gate_rejects_non_passing_terminal_outcome(): - async def handler(request: httpx.Request) -> httpx.Response: - return httpx.Response(503) +async def test_qa_gate_requires_separate_passed_terminal_run(monkeypatch): + """The gate reports the pipeline's own QA run, not a health probe of its own.""" + monkeypatch.setenv("INTERNAL_API_KEY", "test-internal-key") + async with httpx.AsyncClient( + transport=_runs_transport([_qa_run()]), base_url="http://test" + ) as api: + result = await run_non_llm_qa(api, "story-1", timeout=1, poll_interval=0) - transport = httpx.MockTransport(handler) - async with httpx.AsyncClient(transport=transport, base_url="http://test") as api: + assert result == {"run_id": "qa-1", "status": "completed", "qa_outcome": "passed"} + + +@pytest.mark.asyncio +async def test_qa_gate_rejects_non_passing_terminal_outcome(monkeypatch): + monkeypatch.setenv("INTERNAL_API_KEY", "test-internal-key") + runs = [ + _qa_run( + status="failed", + result={"qa_outcome": "failed", "summary": "1/1 GET check(s) failed"}, + ) + ] + async with httpx.AsyncClient(transport=_runs_transport(runs), base_url="http://test") as api: with pytest.raises(AssertionError, match="status=failed outcome=failed"): - await run_non_llm_qa(api, "http://deployed", timeout=0.001, poll_interval=0) + await run_non_llm_qa(api, "story-1", timeout=1, poll_interval=0) + + +@pytest.mark.asyncio +async def test_qa_gate_ignores_runs_of_other_stories(monkeypatch): + """A project carries QA runs of other stories; only this mega's run counts.""" + monkeypatch.setenv("INTERNAL_API_KEY", "test-internal-key") + runs = [_qa_run(id="qa-other", story_id="story-other")] + async with httpx.AsyncClient(transport=_runs_transport(runs), base_url="http://test") as api: + with pytest.raises(AssertionError, match="no QA run reached a terminal state"): + await run_non_llm_qa(api, "story-1", timeout=0.01, poll_interval=0) + + +@pytest.mark.asyncio +async def test_qa_gate_waits_out_a_non_terminal_run(monkeypatch): + monkeypatch.setenv("INTERNAL_API_KEY", "test-internal-key") + runs = [_qa_run(status="running", result=None)] + async with httpx.AsyncClient(transport=_runs_transport(runs), base_url="http://test") as api: + with pytest.raises(AssertionError, match="no QA run reached a terminal state"): + await run_non_llm_qa(api, "story-1", timeout=0.01, poll_interval=0) + + +@pytest.mark.asyncio +async def test_qa_gate_rejects_a_user_scoped_observer(): + """list_runs hides unowned runs from a user-scoped caller — crash, don't blind-poll.""" + async with httpx.AsyncClient( + transport=_runs_transport([_qa_run()]), + base_url="http://test", + headers={"X-Telegram-ID": "12345"}, + ) as api: + with pytest.raises(RuntimeError, match="X-Telegram-ID"): + await run_non_llm_qa(api, "story-1", timeout=1, poll_interval=0) # ── Environment contract preflight ─────────────────────────────────────── From e33ae5ad95d62dd7317da4a9ad4f07aa50031345 Mon Sep 17 00:00:00 2001 From: vladmesh <16962535+vladmesh@users.noreply.github.com> Date: Fri, 17 Jul 2026 01:12:28 +0300 Subject: [PATCH 2/2] fix: keep agent scaffolding out of the HTTP-only QA path Review found two defects in the non-LLM QA path, both real. The HTTP branch was chosen after _resolve_server_info had already read the application, its server, and the server's SSH private key. A criteria block of plain GET expectations is answered by the deployed URL alone, so a temporarily missing SSH key turned a service answering 200 into a terminal qa_outcome=error without QA ever issuing the request. The same held for bot_username: a tg_bot project's first story carries the seeded health check, so it failed before the architect had written any Telegram criteria. Parse the criteria first and resolve the server, its key, and bot_username only in the agent branch, which is what actually needs them. The health client also followed redirects while the grammar accepts any three-digit status, so the DSL promised an exact status check and broke it in both directions: "GET /old returns 301" could never pass because the client reported the destination's 200, and "returns 200" passed on a path that only redirected to a 200. Check the status the path itself answers. --- docs/CONTRACTS.md | 4 ++ .../langgraph/src/consumers/_qa_runner.py | 5 +- services/langgraph/src/consumers/qa.py | 56 +++++++++-------- .../langgraph/tests/unit/test_qa_consumer.py | 60 +++++++++++++++++++ .../langgraph/tests/unit/test_qa_runner.py | 37 ++++++++++++ 5 files changed, 135 insertions(+), 27 deletions(-) diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 056098d4..34e4a8ec 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -1047,6 +1047,10 @@ Producers (supervisor, admin `run-e2e`) resolve the criteria and put them on the **Health-only criteria:** criteria whose every line is a plain `- GET returns ` are decided by the QA consumer over HTTP (`parse_health_only_criteria` → `run_health_checks`), with no SSH and no LLM. One prose line sends the whole block to Claude Code on the server instead. +`returns ` means the path itself answers that status, so the checks do not follow redirects: a criterion naming a redirect is checked against the redirect, and a criterion naming 200 is not satisfied by a path that redirects to a 200. Checks are retried while the service is still coming up. + +The consumer parses the criteria *before* resolving anything else, and only the agent branch reads the server, its SSH key, and `bot_username`. A criteria block the deployed URL alone can answer must not fail over agent scaffolding it never uses. + **Flow:** Deploy succeeds → supervisor resolves criteria → transitions story to TESTING → creates QA run → publishes QAMessage → QA consumer runs the criteria (HTTP checks, or Claude Code on the prod server) → writes `QAOutcome` to `run.result`. Supervisor polls run outcome and routes: PASSED → complete story, FAILED → create fix task + redispatch to engineering, EXHAUSTED/ERROR → fail story. **Lifecycle operations:** `stop` and `undeploy` actions are handled by the `deploy_lifecycle` module, which SSHes to the server and runs `docker compose stop/down` directly — skipping the full DevOps subgraph. diff --git a/services/langgraph/src/consumers/_qa_runner.py b/services/langgraph/src/consumers/_qa_runner.py index 26252c02..fd321ad1 100644 --- a/services/langgraph/src/consumers/_qa_runner.py +++ b/services/langgraph/src/consumers/_qa_runner.py @@ -108,7 +108,10 @@ async def run_health_checks( never answers with its expected status fails the run. """ results = [] - async with httpx.AsyncClient(timeout=HEALTH_CHECK_TIMEOUT, follow_redirects=True) as client: + # "returns 200" means the path itself answers 200. Following redirects would + # report the destination's status instead, so a criterion naming a redirect + # could never pass and one naming 200 would pass on a redirected path. + async with httpx.AsyncClient(timeout=HEALTH_CHECK_TIMEOUT, follow_redirects=False) as client: for check in checks: results.append(await _run_health_check(client, deployed_url, check)) diff --git a/services/langgraph/src/consumers/qa.py b/services/langgraph/src/consumers/qa.py index 7ca07cde..8aac66e6 100644 --- a/services/langgraph/src/consumers/qa.py +++ b/services/langgraph/src/consumers/qa.py @@ -99,33 +99,40 @@ async def process_qa_job(job_data: dict, redis: RedisStreamClient) -> dict: return {"status": "skipped", "reason": "already_inflight"} try: - # Resolve server info - server_info = await _resolve_server_info(msg.application_id) - if not server_info: - error = f"Cannot resolve server for application {msg.application_id}" - logger.error( - "qa_server_resolve_failed", - application_id=msg.application_id, - ) - await _update_run(run_id, RunStatus.FAILED, QAOutcome.ERROR, error=error) - return {"status": "error", "error": error} - - # Fail-fast: if project has tg_bot module, bot_username is required - if not msg.bot_username: - project = await api_client.get_project(msg.project_id) - modules = (project.config or {}).get("modules", []) - if "tg_bot" in modules: - error = ( - "Project has tg_bot module but bot_username is missing in QAMessage. " - "Deploy smoke test should have resolved it via getMe." + # The criteria travel on the message — the producer resolves them from the + # repository before it creates this run. They decide how QA runs, so parse + # them first: criteria that only state GET expectations need nothing but + # the deployed URL. + acceptance_criteria = msg.acceptance_criteria + health_checks = parse_health_only_criteria(acceptance_criteria) + + # A server to SSH into and a bot to talk to are what the agent needs, not + # what the criteria ask for. Resolve them inside the agent branch only — + # an HTTP check must not fail over an SSH key it never reads. + server_info = None + if health_checks is None: + server_info = await _resolve_server_info(msg.application_id) + if not server_info: + error = f"Cannot resolve server for application {msg.application_id}" + logger.error( + "qa_server_resolve_failed", + application_id=msg.application_id, ) - logger.error("qa_bot_username_missing", story_id=story_id, modules=modules) await _update_run(run_id, RunStatus.FAILED, QAOutcome.ERROR, error=error) return {"status": "error", "error": error} - # The criteria travel on the message — the producer resolves them from the - # repository before it creates this run. - acceptance_criteria = msg.acceptance_criteria + # Fail-fast: if project has tg_bot module, bot_username is required + if not msg.bot_username: + project = await api_client.get_project(msg.project_id) + modules = (project.config or {}).get("modules", []) + if "tg_bot" in modules: + error = ( + "Project has tg_bot module but bot_username is missing in QAMessage. " + "Deploy smoke test should have resolved it via getMe." + ) + logger.error("qa_bot_username_missing", story_id=story_id, modules=modules) + await _update_run(run_id, RunStatus.FAILED, QAOutcome.ERROR, error=error) + return {"status": "error", "error": error} # Mark run as running before starting the checks if run_id: @@ -134,9 +141,6 @@ async def process_qa_job(job_data: dict, redis: RedisStreamClient) -> dict: json={"status": RunStatus.RUNNING.value}, ) - # Criteria that only state GET expectations are decidable over HTTP, so - # run them here rather than paying for a coding agent on the server. - health_checks = parse_health_only_criteria(acceptance_criteria) if health_checks is not None: logger.info("qa_health_only_criteria", story_id=story_id, checks=len(health_checks)) qa_result = await run_health_checks( diff --git a/services/langgraph/tests/unit/test_qa_consumer.py b/services/langgraph/tests/unit/test_qa_consumer.py index c68b92dd..8ab6619f 100644 --- a/services/langgraph/tests/unit/test_qa_consumer.py +++ b/services/langgraph/tests/unit/test_qa_consumer.py @@ -10,7 +10,9 @@ from datetime import UTC, datetime from unittest.mock import AsyncMock, patch +import httpx import pytest +import respx from shared.contracts.dto.application import ApplicationDTO from shared.contracts.dto.project import ProjectDTO, ProjectStatus @@ -347,6 +349,64 @@ async def test_failing_health_check_stores_failed_outcome( assert run_data["result"]["qa_outcome"] == QAOutcome.FAILED.value assert run_data["result"]["failed_checks"][0]["detail"] == "got 502, expected 200" + @respx.mock + @pytest.mark.asyncio + async def test_http_200_passes_when_the_server_cannot_be_resolved( + self, mock_api_client, mock_redis, qa_message_data + ): + """An HTTP-decidable check must not fail over agent scaffolding it never uses. + + The server's SSH key is what the coding agent needs to log in. A criteria + block of plain GET expectations is answered by the deployed URL alone, so a + missing key must not turn a service that answers 200 into a terminal error. + """ + route = respx.get("https://weather.example.com/health").mock( + return_value=httpx.Response(200) + ) + # Server resolution would fail outright: no SSH key for this application. + mock_api_client.get_server_ssh_key.return_value = None + qa_message_data["acceptance_criteria"] = "- GET /health returns 200" + + result = await process_qa_job(qa_message_data, mock_redis) + + assert result["status"] == "passed" + assert route.called + # Nothing about the server — or its private key — is read on this path. + mock_api_client.get_application.assert_not_called() + mock_api_client.get_server.assert_not_called() + mock_api_client.get_server_ssh_key.assert_not_called() + + run_data = mock_api_client.patch.call_args_list[-1][1]["json"] + assert run_data["status"] == RunStatus.COMPLETED.value + assert run_data["result"]["qa_outcome"] == QAOutcome.PASSED.value + + @respx.mock + @pytest.mark.asyncio + async def test_http_200_passes_for_a_tg_bot_project_without_a_bot_username( + self, mock_api_client, mock_redis, qa_message_data + ): + """bot_username is what the agent talks to Telegram with, not a GET check. + + A tg_bot project's first story carries the seeded health check, so it must + not error out before the architect has written any Telegram criteria. + """ + respx.get("https://weather.example.com/health").mock(return_value=httpx.Response(200)) + mock_api_client.get_project.return_value = ProjectDTO( + id="116c9678-5872-4ce5-8332-9a267ab27604", + name="tg_bot_project", + status=ProjectStatus.ACTIVE, + config={"modules": ["tg_bot"]}, + owner_id=1, + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + ) + qa_message_data["bot_username"] = None + qa_message_data["acceptance_criteria"] = "- GET /health returns 200" + + result = await process_qa_job(qa_message_data, mock_redis) + + assert result["status"] == "passed" + @pytest.mark.asyncio async def test_prose_criteria_still_go_to_the_agent( self, mock_api_client, mock_redis, qa_message_data diff --git a/services/langgraph/tests/unit/test_qa_runner.py b/services/langgraph/tests/unit/test_qa_runner.py index bef75040..64607c7e 100644 --- a/services/langgraph/tests/unit/test_qa_runner.py +++ b/services/langgraph/tests/unit/test_qa_runner.py @@ -153,6 +153,43 @@ async def test_trailing_slash_does_not_double_up(self): assert result.passed is True + @respx.mock + @pytest.mark.asyncio + async def test_a_redirect_criterion_checks_the_redirect_itself(self): + """ "returns 301" means the path answers 301, not that it leads somewhere 200.""" + redirect = respx.get("http://svc.example.com/old").mock( + return_value=httpx.Response(301, headers={"Location": "http://svc.example.com/new"}) + ) + destination = respx.get("http://svc.example.com/new").mock(return_value=httpx.Response(200)) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/old", expected_status=301)], + ) + + assert result.passed is True + assert redirect.called + # Following the redirect would report the destination's 200 and fail a + # criterion the service actually satisfies. + assert not destination.called + + @respx.mock + @pytest.mark.asyncio + async def test_a_redirected_path_does_not_pass_a_200_criterion(self): + """The inverse: a 301 must not be laundered into the 200 the criterion wants.""" + respx.get("http://svc.example.com/health").mock( + return_value=httpx.Response(301, headers={"Location": "http://svc.example.com/ok"}) + ) + respx.get("http://svc.example.com/ok").mock(return_value=httpx.Response(200)) + + result = await run_health_checks( + deployed_url="http://svc.example.com", + checks=[HealthCriterion(path="/health", expected_status=200)], + ) + + assert result.passed is False + assert result.checks[0]["detail"] == "got 301, expected 200" + @respx.mock @pytest.mark.asyncio async def test_wrong_status_fails_with_detail(self):