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..34e4a8ec 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,23 @@ 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. + +`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/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..fd321ad1 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,63 @@ 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 = [] + # "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)) + + 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..8aac66e6 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__) @@ -93,61 +99,64 @@ 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} - # 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 "" - - 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 + # 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: 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, - ) + 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..8ab6619f 100644 --- a/services/langgraph/tests/unit/test_qa_consumer.py +++ b/services/langgraph/tests/unit/test_qa_consumer.py @@ -10,11 +10,12 @@ 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 -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 +26,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 +99,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 +123,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 +287,147 @@ 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" + + @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 + ): + """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 +466,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 +482,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 +496,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 +506,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..64607c7e 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,153 @@ 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_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): + 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 ───────────────────────────────────────