diff --git a/.github/workflows/ci-unit.yml b/.github/workflows/ci-unit.yml index 7e7b7efc..fe8be39c 100644 --- a/.github/workflows/ci-unit.yml +++ b/.github/workflows/ci-unit.yml @@ -197,11 +197,40 @@ jobs: - name: Run unit tests run: uv run pytest vektra-app/tests/ -v --tb=short -m "not integration" + # The structural guards, and the only job here that no path filter can skip. That is + # the whole point of it. Every other job is gated on dorny/paths-filter, so a PR that + # only edits the Makefile, only edits a workflow, or adds a brand-new vektra-foo/ + # package matches no filter and skips them all — and those are exactly the three + # changes a guard over the runner lists exists to catch. A guard a path filter can + # skip is not a guard (DEBT-031), which also applies to the DEBT-029 guard that until + # now ran only when vektra-shared itself changed. + # + # It parses the Makefile and the workflows; it runs no suite and needs no services. + test-structure: + name: Structural guards (suite execution, env isolation) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + # Runs PR-authored test code; don't leave the token in .git/config + persist-credentials: false + - uses: astral-sh/setup-uv@v7 + with: + enable-cache: true + cache-dependency-glob: "uv.lock" + - run: uv sync --dev --frozen --package vektra-shared + - name: Check every suite is executed, and every package is env-isolated + run: > + uv run pytest + vektra-shared/tests/test_suite_execution_coverage.py + vektra-shared/tests/test_env_isolation_coverage.py + -v --tb=short + # Aggregator job used as the single required status check in branch protection. # Passes if no upstream job failed (skipped jobs are treated as passing). ci-gate: name: CI gate - needs: [detect-changes, test-shared, test-core, test-ingest, test-index, test-admin, test-analytics, test-learn, test-app] + needs: [detect-changes, test-shared, test-core, test-ingest, test-index, test-admin, test-analytics, test-learn, test-app, test-structure] runs-on: ubuntu-latest if: always() steps: diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 9d306f25..bebe5fb8 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -72,10 +72,14 @@ jobs: echo "startup_ms=$ELAPSED_MS" >> "$GITHUB_OUTPUT" echo "::notice::Container startup: ${ELAPSED_MS}ms (NFR-004 limit: 60000ms)" - - name: Run integration tests (REQ-032) + # test_startup.py (ARCH-057) asserts the 11 startup steps against the logs of the + # stack this job has just brought up. Its docstring said it ran here; it did not, + # because this step named tests/integration/ and nothing named tests/ (DEBT-031). + # It is the same startup surface BUG-024 broke, and it was watching nothing. + - name: Run integration tests (REQ-032) and startup validation (ARCH-057) id: integration run: | - uv run pytest tests/integration/ \ + uv run pytest tests/integration/ tests/test_startup.py \ -v --tb=short \ --junitxml=test-results/integration-${{ matrix.vector_store }}.xml timeout-minutes: 10 @@ -118,16 +122,21 @@ jobs: path: test-results/ retention-days: 14 - # The ARCH-057 startup sequence and the REQ-011/NFR-009 error contract. These - # tests existed but no workflow ran them (DEBT-030), which is the surface a - # provider-wiring defect broke in BUG-024 — and, unrun, they had themselves rotted: - # they masked the container password and failed at setup for anyone who tried. - # They bring up their own postgres via testcontainers, so they need Docker but not - # the compose stack. - app-integration: - name: App integration tests (startup + error contract) + # Every `integration`-marked test in every package, vektra-app included (its own + # suite is the ARCH-057 startup sequence and the REQ-011/NFR-009 error contract). + # DEBT-030 wired up vektra-app by hand and left the identical hole open next door: + # the integration suites of vektra-admin, vektra-index and vektra-ingest are excluded + # from every unit run by `-m "not integration"` and were named by no workflow, so they + # had never been executed once (DEBT-031). + # + # The target is a glob deliberately. A hand-written package list is the thing that + # keeps failing here — twice now — so a package added tomorrow is picked up with no + # edit to this file. These suites bring up their own postgres via testcontainers, so + # they need Docker but not the compose stack. + package-integration: + name: Package integration tests (all packages) runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 30 steps: - uses: actions/checkout@v7 @@ -146,16 +155,16 @@ jobs: - name: Prepare test results directory run: mkdir -p test-results - - name: Run vektra-app integration tests + - name: Run every package's integration tests run: | - uv run pytest vektra-app/tests/ -m integration \ - -v --tb=short --junitxml=test-results/app-integration.xml + uv run pytest vektra-*/tests -m integration \ + -v --tb=short --junitxml=test-results/package-integration.xml - name: Upload test results if: always() uses: actions/upload-artifact@v7 with: - name: app-integration-test-results + name: package-integration-test-results path: test-results/ retention-days: 14 @@ -163,24 +172,25 @@ jobs: # matrix reports one check per provider ("Integration tests (pgvector)", "… # (qdrant)"), so without a job carrying the required name the check would never # report and every PR would sit BLOCKED. Same pattern as ci-gate in ci-unit.yml: - # adding a provider to the matrix, or a job like app-integration, then needs no - # change to branch protection. + # adding a provider to the matrix, or a job like package-integration, then needs no + # change to branch protection. The name below is load-bearing: change it and every PR + # sits BLOCKED with all checks green. integration-gate: name: Integration tests + NFR gates - needs: [integration, app-integration] + needs: [integration, package-integration] runs-on: ubuntu-latest if: always() steps: - name: Check every integration job passed run: | MATRIX="${{ needs.integration.result }}" - APP="${{ needs.app-integration.result }}" + PACKAGES="${{ needs.package-integration.result }}" if [[ "$MATRIX" != "success" ]]; then echo "Integration matrix did not pass (result: $MATRIX)." exit 1 fi - if [[ "$APP" != "success" ]]; then - echo "App integration tests did not pass (result: $APP)." + if [[ "$PACKAGES" != "success" ]]; then + echo "Package integration tests did not pass (result: $PACKAGES)." exit 1 fi - echo "Integration tests passed on every vector store provider, and the app suite passed." + echo "Integration tests passed on every vector store provider, and every package's integration suite passed." diff --git a/.s2s/BACKLOG.md b/.s2s/BACKLOG.md index 1a490c21..669a54b8 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -163,9 +163,40 @@ They need Docker, and they now carry the `integration` marker, so the unit runs --- -### DEBT-031: nothing guarantees a test suite is actually executed +### BUG-025: a startup failure prints a raw Python traceback next to the structured error **Status**: planned | **Priority**: medium | **Created**: 2026-07-14 +**Origin**: DEBT-031 (2026-07-14). Surfaced by running `tests/test_startup.py` for the first time. + +**Context**: ARCH-057 exists so that a misconfiguration is reported clearly (REQ-011, NFR-009): a structured, remediable error rather than a stack dump. Step 1 does emit exactly that — + +``` +{"event": "startup_failed", "error": "[STARTUP ERROR] Step: config_validation\n Detail: ...\n Remediation: Set all required environment variables. At minimum: VEKTRA_LLM_PROVIDER ..."} +``` + +— and then `raise SystemExit(1)` propagates out of the ASGI lifespan into uvicorn, which logs the exception with `Traceback (most recent call last)` before "Application startup failed. Exiting." So the operator who typo'd an env var gets the good message *and* a Python traceback. `tests/test_startup.py` has asserted the absence of that traceback since the day it was written; nothing ever ran it (DEBT-031), so nobody found out. + +This is not cosmetic: the traceback is the failure mode NFR-009 names, and it applies to **every** one of the 11 steps, not only config validation. + +**Proposed approach** (recommended): validate before serving rather than inside the lifespan. The container runs `exec uvicorn vektra_app.main:app` (`docker/entrypoint.sh`), so the 11-step sequence necessarily runs as ASGI startup and any abort must travel through uvicorn. Replacing the uvicorn CLI with a `main()` that runs the validation and only then calls `uvicorn.run()` lets a failed step print its structured error and `sys.exit(1)` with no ASGI involvement and no traceback. + +Alternatives considered: suppressing uvicorn's exception logging (fragile, and hides real errors); a preflight that validates only step 1 in the entrypoint (leaves steps 2-11 still leaking a traceback, so it fixes the test rather than the contract). + +**Caution**: this touches the boot path, which is the surface BUG-024 broke. `vektra-app/tests/test_app.py::test_missing_llm_provider_aborts_startup` drives the lifespan in-process and expects `SystemExit`; a change to a hard exit there would kill the test runner. + +**Acceptance criteria**: +- [ ] A misconfigured env var produces the structured `[STARTUP ERROR]` block and **no** `Traceback (most recent call last)` in container output +- [ ] Holds for a step other than config validation (e.g. database connectivity), not just step 1 +- [ ] The container still exits non-zero +- [ ] The `xfail(strict=True)` on `tests/test_startup.py::test_no_raw_traceback_on_startup_failure` is removed; strict mode makes the suite go red if the fix lands and the marker is left behind + +**Traceability**: REQ-011, NFR-009, ARCH-057, DEBT-031 (found by), BUG-024 (same surface) + +--- + +### DEBT-031: nothing guarantees a test suite is actually executed + +**Status**: done (2026-07-14) | **Priority**: medium | **Created**: 2026-07-14 **Origin**: DEBT-029/030 (2026-07-14). The lesson the fix left behind, rather than a defect the fix left behind. **Context**: BUG-024 (a startup blocker) shipped because `vektra-app/tests/` was executed by nothing — neither `make test` nor CI. DEBT-030 wired that one package in, and the two files in it turned out to have **never worked at all** (`str(make_url(...))` masks the password as `***`, so alembic authenticated with `***` and every test died in setup). A test nobody runs rots. @@ -177,10 +208,20 @@ That is the exact shape of the hole BUG-024 fell through, still open one level u **Proposed approach**: extend the structural test (or add a sibling) so that every `vektra-*/tests` directory, plus `tests/integration` and `tests/nfr`, is referenced by the `make test` target **and** by a CI job. Parsing the Makefile and the workflow YAML is enough; it does not need to run them. Consider also asserting that a package's `integration`-marked tests are named in some workflow, which is the specific gap DEBT-030 closed by hand. **Acceptance criteria**: -- [ ] A test fails when a `vektra-*/tests` directory exists that no CI job runs -- [ ] A test fails when such a directory is missing from the `make test` target, so the local gate and CI cannot drift apart (they are two independent hand-maintained lists today) -- [ ] It fails for the unit path and the integration path independently (an `integration`-marked suite excluded from unit runs and named in no workflow is the DEBT-030 case, and must be caught) -- [ ] Verified by deleting a package from the workflow, and separately from the Makefile, and watching the test go red each time +- [x] A test fails when a `vektra-*/tests` directory exists that no CI job runs +- [x] A test fails when such a directory is missing from the `make test` target, so the local gate and CI cannot drift apart (they are two independent hand-maintained lists today) +- [x] It fails for the unit path and the integration path independently (an `integration`-marked suite excluded from unit runs and named in no workflow is the DEBT-030 case, and must be caught) +- [x] Verified by deleting a package from the workflow, and separately from the Makefile, and watching the test go red each time + +**Resolution**: `vektra-shared/tests/test_suite_execution_coverage.py` parses the `test:` recipe and every workflow's `pytest` invocations, and asserts each test file would actually be *collected* — honouring the `-m` filter, not just the paths. That distinction is the whole thing: a directory-level check would have passed, because `vektra-admin/tests/` **is** named in both lists; what nobody ran was the `integration`-marked file inside it. YAML is parsed, not grepped, so the commented-out `pytest` line in `integration.yml` grants no coverage. The guard runs in a `test-structure` job with **no path filter** — every other unit job is gated on `paths-filter`, so a Makefile-only edit, a workflow-only edit, or a new package skips them all, which are exactly the three cases the guard is for. The DEBT-029 guard now runs there too. + +Proven by breaking it five ways, each going red on the right assertion and green on restore: package dropped from the workflow; package dropped from the Makefile; the integration glob narrowed back to `vektra-app` (the DEBT-030 regression); a `vektra-foo/tests/` nobody wired up; and the allowlist emptied, which correctly re-flags the commented-out line as no coverage. + +**Found by the guard, previously unknown** (see CHANGELOG): five suites executed by nothing. `tests/test_startup.py` (ARCH-057 startup validation — its docstring assumed integration.yml ran it; integration.yml names `tests/integration/`, never `tests/`), and the `integration`-marked suites of vektra-admin, vektra-index (×2) and vektra-ingest, which DEBT-030 left behind when it wired up vektra-app by hand. All now run. The `app-integration` job becomes `package-integration` over a `vektra-*/tests` glob so the list stops being hand-maintained. + +**And, on cue, one of them had rotted — and it was hiding two product bugs.** The four package suites passed, but `tests/test_startup.py` went red in CI the first time it ever ran: `test_graceful_failure_on_missing_config` hung until timeout. It sets `-e VEKTRA_LLM_PROVIDER=`, believing that *unsets* the variable; it sets it to the **empty string**. `llm_provider` was a bare required `str`, and `""` is a valid `str`, so pydantic never raised, step 1 never fired, and **the stack booted and served with an empty LLM provider** — deferring the misconfiguration to the first query, which is precisely what ARCH-057 exists to prevent. Fixed at the source (`min_length=1`), not by weakening the test. With that fixed, the same test surfaced **BUG-025**: the structured error is emitted and a raw uvicorn traceback follows it, where NFR-009 asks for the one instead of the other. That one needs the boot path restructured (validate before `uvicorn.run()`), which is the surface BUG-024 broke, so it is filed rather than rushed in here; the assertion is `xfail(strict=True)` so it cannot rot in turn. + +Third time in this repo that a suite turned out to be broken the moment it was executed. The difference is that this one was also concealing live defects — which is the argument for the guard, made better than the guard's own docstring makes it. **Traceability**: BUG-024 (root cause), DEBT-029, DEBT-030 diff --git a/CHANGELOG.md b/CHANGELOG.md index f2fa76f8..06433437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Convention (Keep a Changelog 1.1.0): ### Added +- **ci**: a structural guard that fails when a test suite is executed by nothing (DEBT-031). Nothing checked this. `make test` and `ci-unit.yml` enumerate the packages in two independent hand-maintained lists, and the DEBT-029 guard verifies only that a package is *isolated*, never that anyone *runs* it — so a `vektra-foo/tests/` added tomorrow was silently unexecuted and no test went red. That is the exact shape of the hole BUG-024 fell through, still open one level up. `vektra-shared/tests/test_suite_execution_coverage.py` reads the runners instead of trusting them: it parses the `test:` recipe and every `pytest` invocation in every workflow, and asserts that each test file would actually be *collected* by one — which means honouring the `-m` filter and not just the paths, because a file can sit in a directory both runners name and still be run by neither, every unit run passing `-m "not integration"`. The unit path and the integration path are therefore checked independently. Workflow YAML is parsed, never grepped: `integration.yml` carries a commented-out `pytest` line, and a guard that counted it as coverage would certify a suite nobody runs. It executes in a `test-structure` job carrying **no path filter**, deliberately — every other unit job is gated on `dorny/paths-filter`, so a PR that only edits the Makefile, only edits a workflow, or adds a new package matches no filter and skips them all, and those are precisely the three changes this guard exists to catch. A guard a path filter can skip is not a guard, so the DEBT-029 guard (until now gated on `vektra-shared/**`) runs there too. - **ci**: publish versioned container images to GHCR on tag push (INFRA-007). `v*` tags trigger `.github/workflows/publish.yml`, which builds and pushes `ghcr.io/vektralabs/vektra:{version}` and `:{version}-ocr` (GHA build cache, OCI version/revision labels, built-in `GITHUB_TOKEN`, no new secrets). Deployment docs and a new `deploy/docker-compose.image.yml.example` overlay cover the resulting `docker compose pull && docker compose up -d` flow as an alternative to building from source. No `latest` tag. The manual tagging flow is unchanged. - **index**: `GET /api/v1/documents/{document_id}/chunks` lists a document's stored chunks (text, position, parent link, metadata) at the active index version, read from the active vector store. Any valid scope. - **index**: `reindex_jobs.chunks_reindexed` (migration `0007`) records how many chunks a reindex actually re-embedded and wrote, exposed on `GET /api/v1/reindex/{job_id}/status`. A job that walked every document and stored nothing used to be indistinguishable from a real one. @@ -26,6 +27,8 @@ Convention (Keep a Changelog 1.1.0): ### Fixed +- **config**: `VEKTRA_LLM_PROVIDER=""` was accepted and the stack **booted and served with an empty LLM provider**, deferring the misconfiguration to the first query — which is the one thing the ARCH-057 startup sequence exists to prevent (REQ-011, NFR-009). The field is required, but typed as a bare `str`, and `""` is a perfectly valid `str`, so pydantic never raised and step 1 never fired. It is now `min_length=1`, so an empty value fails at startup with the same structured, remediable error an absent one already produced. Found by running `tests/test_startup.py` for the first time (DEBT-031): the test had asserted this exact behaviour since it was written, believing `docker run -e VAR=` *unset* the variable when it in fact sets it to the empty string — so it had been asserting against a value the product happily accepted. Nobody found out, because nothing ever ran it. This is the third time a suite in this repo turned out to be broken the moment someone executed it. The same test also caught **BUG-025** (filed, not fixed here): the structured error is emitted and then `raise SystemExit(1)` travels out of the ASGI lifespan into uvicorn, which logs a `Traceback` after it — so a typo'd env var yields the good message *and* a stack dump, where NFR-009 asks for the former instead of the latter. Fixing that means validating before `uvicorn.run()` rather than inside the lifespan, i.e. touching the boot path BUG-024 broke, so it is tracked on its own; the assertion is `xfail(strict=True)`, which turns the suite red the moment the fix lands and the marker is left behind. +- **tests**: five suites that no runner executed now run, all five found by the guard above the moment it was pointed at `develop` (DEBT-031). Four are `integration`-marked — `vektra-admin/tests/test_integration.py`, `vektra-index/tests/test_integration.py`, `vektra-index/tests/test_benchmark.py`, `vektra-ingest/tests/test_integration.py` — so every unit run excluded them by `-m "not integration"`, while the only job that ran `-m integration` named `vektra-app/tests/` alone: DEBT-030 wired up vektra-app by hand and left the identical hole open in three neighbouring packages. The fifth is `tests/test_startup.py`, the ARCH-057 startup-validation suite, whose own docstring states it requires the stack "started by integration.yml" — it did not run there, because that workflow names `tests/integration/` and nothing named `tests/`. It was watching the precise surface BUG-024 broke, and it was watching nothing. The `app-integration` job becomes `package-integration` and targets `vektra-*/tests -m integration`, a glob on purpose: the hand-written package list is the thing that has now failed twice, and a package added tomorrow is picked up with no edit. `tests/test_startup.py` joins the job that already brings the stack up. Unlike vektra-app's suites in DEBT-030 these four had **not** rotted — 52 integration tests pass — but that was luck, not a property of the arrangement. The five suites under `tests/` were also missing the `integration` marker they require, which is what made them indistinguishable from unit tests `make test` had merely forgotten; `tests/nfr/test_performance.py` remains deliberately unrun (it needs a live LLM that GitHub Actions does not have) and is the single justified entry in the guard's `EXPECTED_UNRUN`, where a reviewer can see it. The required check names (`CI gate`, `Integration tests + NFR gates`) are unchanged. - **tests**: the local `.env` reached **every** test package, including the three that carried the DEBT-025 scrub fixture (DEBT-029). litellm calls `dotenv.load_dotenv()` at import and finds the repo `.env` by walking up from its own module inside `.venv/`, so the developer's configuration landed in `os.environ` — and the scrub could not stop it, because the scrub runs *before* the test body while the import that re-injects the file happens *inside* it. Measured: after `import litellm` in a test, a config left at its default resolved to the value in the developer's `.env` (`qdrant`) rather than the code's default (`pgvector`) in all four packages checked, `vektra-core` and `vektra-shared` included. Tests therefore ran a different code path locally than in CI, which is the one thing a test must not do. `vektra_shared.testing` now sets `LITELLM_MODE` before litellm can be imported, so litellm skips the dotenv load entirely; all eight test packages import the single shared fixture, and a structural test fails if a package is added without it. Two empty `tests/__init__.py` files (analytics, learn) that made pytest derive the same module name for two conftests were removed. - **tests**: `vektra-app`'s two Docker-backed test files (the ARCH-057 startup sequence and the REQ-011/NFR-009 error contract) now run in CI, in an `app-integration` job gated by the integration aggregator (DEBT-030). They were run by nothing — and, unrun, they had rotted: both built the test container's URL with `str(make_url(...))`, which masks the password as `***`, so Alembic authenticated with a literal `***` and every test errored at setup. They had never worked. Fixed; 8 tests pass. `vektra-app` also never registered the `integration` marker it relies on. - **index**: in Qdrant mode, every code path that read chunk text from Postgres worked on an empty table and reported success (BUG-023, [ADR-0026](.s2s/decisions/ADR-0026-document-chunks-pgvector-internal.md)). `document_chunks` is written only by the pgvector provider, so with `VEKTRA_VECTOR_STORE_PROVIDER=qdrant` — the configuration every real deployment runs — it holds nothing, and the paths that queried it did not fail, they lied: `reindex` re-embedded no chunks and reported `completed` (the root cause of the no-op previously attributed to BUG-021), `GET /stats` reported `chunk_count: 0` for populated namespaces, `POST /documents/{id}/chunks` wrote to the *inactive* store, `GET /health` reported the index healthy while the store backing it was unreachable, and — the serious one — `DELETE /documents/{id}` returned `200 {"chunks_removed": 0}`, soft-deleted the Postgres row, left the Qdrant points in place, and **the deleted document went on answering queries**. Retention (REQ-057) had the same defect and was worse: it hard-deleted the document row and relied on the `document_chunks` CASCADE, leaving the content in Qdrant, still searchable and no longer traceable to any document. All of these now go through the `VectorStoreProvider` Protocol, which grows `list_chunks()` and `count_chunks()` and an optional `index_version` on `store()`; `document_chunks` is now formally private to the pgvector provider. A reindex over a non-empty namespace that stores nothing fails loudly instead of reporting `completed`. Verified against the live Qdrant stack: reindex rewrites the collection (measured 0 → 12 points on the target version, source version preserved), stats match the real point counts, and a deleted document is no longer retrievable. diff --git a/tests/integration/test_chunk_lifecycle.py b/tests/integration/test_chunk_lifecycle.py index cfa11aa8..5ca04dc2 100644 --- a/tests/integration/test_chunk_lifecycle.py +++ b/tests/integration/test_chunk_lifecycle.py @@ -22,6 +22,9 @@ import httpx import pytest +# Runs against the live stack (integration.yml), so every unit run must exclude it. +pytestmark = pytest.mark.integration + CHUNK_TEXT = ( "# Zarnak protocol\n\n" "The Zarnak protocol reaches consensus with a quorum of seven nodes.\n" diff --git a/tests/integration/test_e2e_flow.py b/tests/integration/test_e2e_flow.py index 9e50544a..e1ae127b 100644 --- a/tests/integration/test_e2e_flow.py +++ b/tests/integration/test_e2e_flow.py @@ -17,6 +17,9 @@ import httpx import pytest +# Runs against the live stack (integration.yml), so every unit run must exclude it. +pytestmark = pytest.mark.integration + FIXTURES_DIR = Path(__file__).resolve().parents[1] / "fixtures" SAMPLE_PDF = FIXTURES_DIR / "sample.pdf" diff --git a/tests/nfr/test_nfr_hard.py b/tests/nfr/test_nfr_hard.py index f86f7c5d..27184e59 100644 --- a/tests/nfr/test_nfr_hard.py +++ b/tests/nfr/test_nfr_hard.py @@ -24,6 +24,9 @@ import httpx import pytest +# Runs against the live stack (integration.yml), so every unit run must exclude it. +pytestmark = pytest.mark.integration + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- diff --git a/tests/nfr/test_performance.py b/tests/nfr/test_performance.py index a3ffbf01..93d699f0 100644 --- a/tests/nfr/test_performance.py +++ b/tests/nfr/test_performance.py @@ -17,6 +17,10 @@ import httpx import pytest +# Needs the live stack *and* a real LLM, so CI runs it nowhere; it is the one entry in +# EXPECTED_UNRUN in the DEBT-031 guard. Marked all the same, so no unit run picks it up. +pytestmark = pytest.mark.integration + # NFR-001 target: query latency (not a hard gate in Phase 1 per EX-004) _QUERY_LATENCY_TARGET_MS = 2000 _NOTE_THRESHOLD = 1.2 # 20% above target -> note diff --git a/tests/test_startup.py b/tests/test_startup.py index 41e938dd..084f56b3 100644 --- a/tests/test_startup.py +++ b/tests/test_startup.py @@ -13,6 +13,14 @@ import subprocess +import pytest + +# Needs the running stack, so it is an integration test and now says so. Unmarked, it +# was indistinguishable from a unit test that `make test` had merely forgotten — and +# in fact no runner ran it at all, despite the docstring above assuming otherwise +# (DEBT-031). +pytestmark = pytest.mark.integration + # All 11 ARCH-057 startup step names in execution order _STARTUP_STEPS = [ "config_validation", @@ -59,10 +67,15 @@ def test_startup_complete_logged() -> None: ) -def test_graceful_failure_on_missing_config() -> None: - """Missing required env var produces a structured error, not a traceback. +def _run_with_empty_llm_provider() -> str: + """Boot a throwaway container whose VEKTRA_LLM_PROVIDER is the empty string. - Runs a separate short-lived container with VEKTRA_LLM_PROVIDER unset. + `-e VEKTRA_LLM_PROVIDER=` sets the variable to `""`; it does not unset it, as this + module used to claim. That mattered: `llm_provider` was a bare required `str`, `""` + is a valid `str`, so nothing raised, the container booted and served, and the + misconfiguration surfaced at the first query instead of at startup. The test never + once ran (DEBT-031), so nobody found out. The field is now `min_length=1`, and an + empty value fails at step 1 exactly as an absent one does. """ result = subprocess.run( [ @@ -81,15 +94,35 @@ def test_graceful_failure_on_missing_config() -> None: text=True, timeout=30, ) - - output = result.stdout + result.stderr assert result.returncode != 0, ( - "Container should have exited non-zero with missing VEKTRA_LLM_PROVIDER" + "Container should have exited non-zero with an empty VEKTRA_LLM_PROVIDER" ) + return result.stdout + result.stderr + + +def test_graceful_failure_on_missing_config() -> None: + """A required env var with no usable value fails startup, not the first query.""" + output = _run_with_empty_llm_provider() assert "startup_failed" in output, ( "Expected 'startup_failed' in output, got: " + output[:500] ) - # No raw Python tracebacks should leak to the user + + +@pytest.mark.xfail( + strict=True, + reason=( + "BUG-025: the structured [STARTUP ERROR] block is emitted, but `raise " + "SystemExit(1)` then travels out of the ASGI lifespan into uvicorn, which logs " + "the exception — so the operator gets the good message and a Python traceback. " + "NFR-009 asks for the former instead of the latter, not both. Fixing it means " + "validating before uvicorn.run() rather than inside the lifespan, which is the " + "boot path BUG-024 broke, so it is tracked separately. strict=True: when " + "BUG-025 lands this test XPASSes and the suite goes red until the marker goes." + ), +) +def test_no_raw_traceback_on_startup_failure() -> None: + """A misconfiguration is reported, not dumped as a stack trace (REQ-011, NFR-009).""" + output = _run_with_empty_llm_provider() assert "Traceback (most recent call last)" not in output, ( "Raw traceback leaked in startup failure output" ) diff --git a/uv.lock b/uv.lock index c7dff6be..29dbc9f2 100644 --- a/uv.lock +++ b/uv.lock @@ -5454,6 +5454,7 @@ dev = [ { name = "import-linter" }, { name = "pytest" }, { name = "pytest-asyncio" }, + { name = "pyyaml" }, ] [package.metadata] @@ -5473,6 +5474,7 @@ dev = [ { name = "import-linter", specifier = ">=2.0" }, { name = "pytest", specifier = ">=8.0" }, { name = "pytest-asyncio", specifier = ">=0.24" }, + { name = "pyyaml", specifier = ">=6.0.3" }, ] [[package]] diff --git a/vektra-shared/pyproject.toml b/vektra-shared/pyproject.toml index 1e125d54..b39c197a 100644 --- a/vektra-shared/pyproject.toml +++ b/vektra-shared/pyproject.toml @@ -31,4 +31,8 @@ dev = [ "pytest-asyncio>=0.24", "httpx>=0.27", "import-linter>=2.0", + # test_suite_execution_coverage.py parses the workflow YAML (DEBT-031). + # Declared here so a `uv sync --package vektra-shared` has it, rather than + # relying on the root dev group leaking in. + "pyyaml>=6.0.3", ] diff --git a/vektra-shared/src/vektra_shared/config.py b/vektra-shared/src/vektra_shared/config.py index 894a0030..22f014c2 100644 --- a/vektra-shared/src/vektra_shared/config.py +++ b/vektra-shared/src/vektra_shared/config.py @@ -498,6 +498,7 @@ class VektraSettings(BaseSettings): # LLM llm_provider: str = Field( ..., + min_length=1, alias="VEKTRA_LLM_PROVIDER", description="LLM model identifier in litellm format.", ) diff --git a/vektra-shared/tests/test_suite_execution_coverage.py b/vektra-shared/tests/test_suite_execution_coverage.py new file mode 100644 index 00000000..32dc4a8b --- /dev/null +++ b/vektra-shared/tests/test_suite_execution_coverage.py @@ -0,0 +1,327 @@ +"""Every test suite must actually be executed by something (DEBT-031). + +`test_env_isolation_coverage.py` proves each test package is *isolated*. It does not +prove anyone *runs* it. Nothing did: `make test` and `ci-unit.yml` each enumerate the +packages by hand, in two independent lists, so a suite absent from both is silently +never executed — and no test goes red. That is the hole BUG-024 fell through +(`vektra-app/tests/` was run by neither), and when DEBT-030 finally ran those files it +turned out they had never worked at all. A test nobody runs does not stay neutral, it +rots. + +So this guard reads the runners instead of trusting them: the `test:` recipe in the +Makefile, and every `pytest` invocation in every workflow. For each test file it asks +whether some runner would actually *collect* it, which means honouring the `-m` filter +as well as the paths — a file can sit in a directory both runners name and still be +executed by neither, because every unit run passes `-m "not integration"`. That is not +hypothetical: it is exactly how the integration suites of vektra-admin, vektra-index +and vektra-ingest were never once executed. + +The unit path and the integration path are therefore checked independently. + +Parsing is enough; nothing here executes a suite. Workflow YAML is parsed, never +grepped: a commented-out `pytest` line is a comment, and a guard that counted it as +coverage would certify a suite that no one runs. +""" + +from __future__ import annotations + +import ast +import shlex +from dataclasses import dataclass +from pathlib import Path + +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +MAKEFILE = REPO_ROOT / "Makefile" +WORKFLOW_DIR = REPO_ROOT / ".github" / "workflows" + +# The `-m` expressions this guard knows how to reason about. An unrecognised one is a +# hard failure rather than a silent "covers nothing" / "covers everything" guess: the +# whole point is not to certify coverage we cannot actually demonstrate. +_SELECTS_UNIT = {None: True, "not integration": True, "integration": False} +_SELECTS_INTEGRATION = {None: True, "not integration": False, "integration": True} + +# Suites deliberately run by nothing. Every entry needs a reason, and the entry has to +# stay true: a stale one fails below. Adding to this list is how you *knowingly* stop +# running a suite, which is a thing a reviewer should see you do. +EXPECTED_UNRUN: dict[str, str] = { + "tests/nfr/test_performance.py": ( + "measures query latency against a live LLM, which GitHub Actions does not " + "have; run manually on Kalypso (see the note in integration.yml)" + ), +} + + +# --------------------------------------------------------------------------- +# What tests exist, and what kind they are +# --------------------------------------------------------------------------- + + +def _mark_name(node: ast.expr) -> str | None: + """The `x` of a `pytest.mark.x` node, with or without a call.""" + if isinstance(node, ast.Call): + node = node.func + if isinstance(node, ast.Attribute) and isinstance(node.value, ast.Attribute): + if node.value.attr == "mark": + return node.attr + return None + + +def _pytestmark_marks(body: list[ast.stmt]) -> set[str]: + """Marks a `pytestmark` assignment applies to everything in this scope. + + Valid at module level and inside a class, and pytest honours both. + """ + marks: set[str] = set() + for node in body: + if not isinstance(node, ast.Assign): + continue + if not any( + isinstance(t, ast.Name) and t.id == "pytestmark" for t in node.targets + ): + continue + values = ( + node.value.elts + if isinstance(node.value, ast.List | ast.Tuple) + else [node.value] + ) + marks |= {name for v in values if (name := _mark_name(v))} + return marks + + +@dataclass(frozen=True) +class Suite: + """A test file, and the kinds of test it holds.""" + + path: Path + has_unit: bool + has_integration: bool + + @property + def rel(self) -> str: + return str(self.path.relative_to(REPO_ROOT)) + + +def _classify(path: Path) -> Suite: + """Which kinds of test a file holds, honouring how a mark is inherited. + + A test is `integration` if it says so itself, or if the class or the module around + it does — pytest applies all three, so a guard that read only the innermost one + would call a class of integration tests "unit", never ask for an integration run, + and let them go unexecuted. Which is the bug this whole file exists to prevent. + """ + tree = ast.parse(path.read_text(encoding="utf-8")) + found = {"unit": False, "integration": False} + + def visit(body: list[ast.stmt], inherited: bool) -> None: + scoped = inherited or "integration" in _pytestmark_marks(body) + for node in body: + marked = scoped or any( + _mark_name(d) == "integration" + for d in getattr(node, "decorator_list", []) + ) + if isinstance(node, ast.ClassDef): + visit(node.body, marked) + elif isinstance( + node, ast.FunctionDef | ast.AsyncFunctionDef + ) and node.name.startswith("test_"): + found["integration" if marked else "unit"] = True + + visit(tree.body, inherited=False) + return Suite(path, has_unit=found["unit"], has_integration=found["integration"]) + + +def _suites() -> list[Suite]: + roots = [*REPO_ROOT.glob("vektra-*/tests"), REPO_ROOT / "tests"] + files = { + f + for root in roots + if root.is_dir() + for f in root.rglob("test_*.py") + if "__pycache__" not in f.parts + } + return sorted((_classify(f) for f in files), key=lambda s: s.rel) + + +# --------------------------------------------------------------------------- +# What the runners actually run +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class Invocation: + """One `pytest ...` command found in a runner.""" + + source: str + paths: tuple[Path, ...] + marker: str | None + + def collects(self, suite: Suite) -> bool: + """Would this command collect that file, on paths alone?""" + return any( + target == suite.path or (target.is_dir() and target in suite.path.parents) + for target in self.paths + ) + + +def _resolve(token: str) -> list[Path]: + """A pytest target as the paths it names. Globs expand; non-paths vanish. + + `vektra-*/tests` is a glob the shell expands, and a runner is free to use one — + it is the cure for the hand-maintained list, so the guard has to understand it. + A token that matches nothing in the repo is not a path (a stray flag value, say), + and is ignored rather than mistaken for coverage. + """ + return sorted(REPO_ROOT.glob(token.rstrip("/"))) + + +def _invocations(script: str, source: str) -> list[Invocation]: + found = [] + for line in script.replace("\\\n", " ").splitlines(): + line = line.strip() + # A `#` line is a comment even inside a `run:` block, and a comment runs nothing. + if line.startswith("#") or "pytest" not in line: + continue + try: + tokens = shlex.split(line) + except ValueError: + continue + if "pytest" not in tokens: + continue + + paths: list[Path] = [] + marker: str | None = None + rest = tokens[tokens.index("pytest") + 1 :] + i = 0 + while i < len(rest): + token = rest[i] + if token == "-m": + marker = rest[i + 1] if i + 1 < len(rest) else None + i += 2 + elif token.startswith("-"): + i += 1 + else: + paths.extend(_resolve(token)) + i += 1 + + if paths: + found.append(Invocation(source, tuple(paths), marker)) + return found + + +def _makefile_invocations() -> list[Invocation]: + recipe: list[str] = [] + inside = False + for line in MAKEFILE.read_text(encoding="utf-8").splitlines(): + if line.startswith("test:"): + inside = True + elif inside: + if line.startswith("\t"): + recipe.append(line) + elif line.strip(): + break # the next target begins + return _invocations("\n".join(recipe), "the `test` target in the Makefile") + + +def _workflow_invocations() -> list[Invocation]: + found = [] + for workflow in sorted(WORKFLOW_DIR.glob("*.yml")): + data = yaml.safe_load(workflow.read_text(encoding="utf-8")) or {} + for job_id, job in (data.get("jobs") or {}).items(): + for step in job.get("steps") or []: + if run := step.get("run"): + found.extend( + _invocations(run, f"job `{job_id}` in {workflow.name}") + ) + return found + + +# --------------------------------------------------------------------------- +# Guard the guard +# --------------------------------------------------------------------------- + + +def test_the_runners_are_discovered() -> None: + """A parser that silently found nothing would make every check below vacuous.""" + assert len(_suites()) >= 40, "test files are not being discovered" + assert _makefile_invocations(), "no pytest command found in the `make test` recipe" + assert len(_workflow_invocations()) >= 8, "workflow pytest commands not discovered" + + +def test_every_marker_expression_is_understood() -> None: + """A `-m` expression the guard cannot reason about must not be waved through.""" + unknown = { + f"{inv.source}: -m {inv.marker!r}" + for inv in _makefile_invocations() + _workflow_invocations() + if inv.marker not in _SELECTS_UNIT + } + assert unknown == set(), ( + "this guard decides coverage from the -m filter, and it does not know what " + "these expressions select. Teach it, or it will certify suites nobody runs:\n " + + "\n ".join(sorted(unknown)) + ) + + +def test_the_deliberately_unrun_suites_still_exist() -> None: + """A stale exemption is a suite quietly re-covered, or a file long gone.""" + stale = [rel for rel in EXPECTED_UNRUN if not (REPO_ROOT / rel).is_file()] + assert stale == [], ( + "EXPECTED_UNRUN exempts files that no longer exist; drop them:\n " + + "\n ".join(stale) + ) + + +# --------------------------------------------------------------------------- +# The guard +# --------------------------------------------------------------------------- + + +def test_every_unit_suite_runs_in_make_test() -> None: + """The local gate and CI are two hand-written lists; neither may lose a suite.""" + runs = _makefile_invocations() + missing = [ + s.rel + for s in _suites() + if s.has_unit + and s.rel not in EXPECTED_UNRUN + and not any(i.collects(s) and _SELECTS_UNIT[i.marker] for i in runs) + ] + assert missing == [], ( + "`make test` does not run these unit tests, so the local gate is blind to " + "them and has drifted from CI:\n " + "\n ".join(missing) + ) + + +def test_every_unit_suite_runs_in_ci() -> None: + runs = _workflow_invocations() + missing = [ + s.rel + for s in _suites() + if s.has_unit + and s.rel not in EXPECTED_UNRUN + and not any(i.collects(s) and _SELECTS_UNIT[i.marker] for i in runs) + ] + assert missing == [], ( + "no CI job runs these unit tests. Nothing will go red when they break:\n " + + "\n ".join(missing) + ) + + +def test_every_integration_suite_runs_in_ci() -> None: + """The DEBT-030 case: marked `integration`, excluded from every unit run by + `-m "not integration"`, and named by no workflow. Belonging to a directory that + both runners list is not enough, and is precisely what made this invisible.""" + runs = _workflow_invocations() + missing = [ + s.rel + for s in _suites() + if s.has_integration + and s.rel not in EXPECTED_UNRUN + and not any(i.collects(s) and _SELECTS_INTEGRATION[i.marker] for i in runs) + ] + assert missing == [], ( + "these tests are marked `integration`, which every unit run excludes, and no " + "workflow runs them with `-m integration`. They are executed by nothing:\n " + + "\n ".join(missing) + )