From d44f4c359d8cd8532763abfe837e7bcaf9adb954 Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 21:48:51 +0000 Subject: [PATCH 1/5] chore(ci): fail when a test suite is executed by nothing (DEBT-031) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `make test` and `ci-unit.yml` enumerate the packages in two independent hand-maintained lists, and the DEBT-029 guard checks only that a package is isolated, never that anyone runs it. A suite absent from both lists was silently never executed and nothing went red. That is the hole BUG-024 fell through, still open one level up. Add `vektra-shared/tests/test_suite_execution_coverage.py`: it 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 point: a directory-level check passes, because `vektra-admin/tests/` is named in both lists; what nobody ran was the `integration`-marked file inside it, excluded from every unit run by `-m "not integration"`. Unit and integration paths are checked independently. YAML is parsed, never grepped, so the commented-out pytest line in integration.yml grants no coverage. It runs in a `test-structure` job with no path filter, deliberately: 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 — exactly the three changes the guard exists to catch. The DEBT-029 guard runs there too. Pointed at develop, it found five suites executed by nothing: `tests/test_startup.py` (ARCH-057 startup validation; its docstring assumed integration.yml ran it, which names tests/integration/ and never tests/), and the integration suites of vektra-admin, vektra-index (x2) and vektra-ingest, left behind when DEBT-030 wired up vektra-app by hand. All now run. The four had not rotted (52 integration tests pass), which was luck, not design. `app-integration` becomes `package-integration` over a `vektra-*/tests` glob, so the package list stops being hand-maintained. The five suites under tests/ also gain the `integration` marker they require; `tests/nfr/test_performance.py` stays deliberately unrun (needs a live LLM) as the one justified entry in the guard's EXPECTED_UNRUN. Required check names are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci-unit.yml | 28 +- .github/workflows/integration.yml | 54 +-- .s2s/BACKLOG.md | 16 +- CHANGELOG.md | 2 + tests/integration/test_chunk_lifecycle.py | 3 + tests/integration/test_e2e_flow.py | 3 + tests/nfr/test_nfr_hard.py | 3 + tests/nfr/test_performance.py | 4 + tests/test_startup.py | 8 + .../tests/test_suite_execution_coverage.py | 317 ++++++++++++++++++ 10 files changed, 410 insertions(+), 28 deletions(-) create mode 100644 vektra-shared/tests/test_suite_execution_coverage.py diff --git a/.github/workflows/ci-unit.yml b/.github/workflows/ci-unit.yml index 7e7b7efc..8dbfec18 100644 --- a/.github/workflows/ci-unit.yml +++ b/.github/workflows/ci-unit.yml @@ -197,11 +197,37 @@ 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 + - 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..fa7499cd 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -165,7 +165,7 @@ They need Docker, and they now carry the `integration` marker, so the unit runs ### DEBT-031: nothing guarantees a test suite is actually executed -**Status**: planned | **Priority**: medium | **Created**: 2026-07-14 +**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 +177,16 @@ 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 four had not rotted (52 integration tests pass), which was luck. The `app-integration` job becomes `package-integration` over a `vektra-*/tests` glob so the list stops being hand-maintained. **Traceability**: BUG-024 (root cause), DEBT-029, DEBT-030 diff --git a/CHANGELOG.md b/CHANGELOG.md index f2fa76f8..606ee900 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,7 @@ Convention (Keep a Changelog 1.1.0): ### Fixed +- **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..030bebdc 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", 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..7627c099 --- /dev/null +++ b/vektra-shared/tests/test_suite_execution_coverage.py @@ -0,0 +1,317 @@ +"""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 _module_marks(tree: ast.Module) -> set[str]: + """Marks applied to the whole module via a top-level `pytestmark`.""" + marks: set[str] = set() + for node in tree.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: + tree = ast.parse(path.read_text(encoding="utf-8")) + + if "integration" in _module_marks(tree): + # A module-level mark applies to every test in the file, so there is nothing + # left that a unit run could pick up. + return Suite(path, has_unit=False, has_integration=True) + + has_unit = False + has_integration = False + for node in ast.walk(tree): + if not isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): + continue + if not node.name.startswith("test_"): + continue + if any(_mark_name(d) == "integration" for d in node.decorator_list): + has_integration = True + else: + has_unit = True + return Suite(path, has_unit=has_unit, has_integration=has_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) + ) From 1b2ceefa5956dc1c776326b80e7377e4e368a1be Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 22:02:29 +0000 Subject: [PATCH 2/5] fix(tests): honour class-level integration marks in the DEBT-031 guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The classifier read only module-level `pytestmark` and function decorators. A test class carrying `@pytest.mark.integration` (or a class-body `pytestmark`) would therefore be classified as unit: the guard would never demand an integration run for it, and every unit run would deselect it by `-m "not integration"`. Silently unexecuted — the exact failure this file exists to prevent, reproduced inside the guard itself. Class-based tests are already used here (vektra-admin), so the hole was live. Marks are now inherited module -> class -> function, as pytest applies them. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/test_suite_execution_coverage.py | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/vektra-shared/tests/test_suite_execution_coverage.py b/vektra-shared/tests/test_suite_execution_coverage.py index 7627c099..32dc4a8b 100644 --- a/vektra-shared/tests/test_suite_execution_coverage.py +++ b/vektra-shared/tests/test_suite_execution_coverage.py @@ -68,10 +68,13 @@ def _mark_name(node: ast.expr) -> str | None: return None -def _module_marks(tree: ast.Module) -> set[str]: - """Marks applied to the whole module via a top-level `pytestmark`.""" +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 tree.body: + for node in body: if not isinstance(node, ast.Assign): continue if not any( @@ -101,25 +104,32 @@ def rel(self) -> str: def _classify(path: Path) -> Suite: - tree = ast.parse(path.read_text(encoding="utf-8")) - - if "integration" in _module_marks(tree): - # A module-level mark applies to every test in the file, so there is nothing - # left that a unit run could pick up. - return Suite(path, has_unit=False, has_integration=True) + """Which kinds of test a file holds, honouring how a mark is inherited. - has_unit = False - has_integration = False - for node in ast.walk(tree): - if not isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): - continue - if not node.name.startswith("test_"): - continue - if any(_mark_name(d) == "integration" for d in node.decorator_list): - has_integration = True - else: - has_unit = True - return Suite(path, has_unit=has_unit, has_integration=has_integration) + 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]: From cd6c83daa9cc00a0e30eb4ee0fedad6eb7c8a935 Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 22:27:50 +0000 Subject: [PATCH 3/5] fix(config): reject an empty VEKTRA_LLM_PROVIDER at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `llm_provider` is required but typed as a bare `str`, and `""` is a valid `str`, so `VEKTRA_LLM_PROVIDER=` was accepted: pydantic never raised, ARCH-057 step 1 never fired, and the stack booted and served with an empty LLM provider, deferring the misconfiguration to the first query. That is the exact failure mode the startup sequence exists to prevent (REQ-011, NFR-009). `min_length=1` makes an empty value fail at step 1 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 has asserted this behaviour since it was written, believing `docker run -e VAR=` *unsets* the variable when it sets it to the empty string — so it was asserting against a value the product happily accepted, and hung waiting for a container that had booted fine. Nothing ever ran it, so nobody found out. Fixed at the source rather than by weakening the test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .s2s/BACKLOG.md | 4 +++- CHANGELOG.md | 1 + tests/test_startup.py | 11 ++++++++--- vektra-shared/src/vektra_shared/config.py | 1 + 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.s2s/BACKLOG.md b/.s2s/BACKLOG.md index fa7499cd..b60caf1b 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -186,7 +186,9 @@ That is the exact shape of the hole BUG-024 fell through, still open one level u 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 four had not rotted (52 integration tests pass), which was luck. The `app-integration` job becomes `package-integration` over a `vektra-*/tests` glob so the list stops being hand-maintained. +**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 a product bug.** 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. 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 a real defect. **Traceability**: BUG-024 (root cause), DEBT-029, DEBT-030 diff --git a/CHANGELOG.md b/CHANGELOG.md index 606ee900..8175ac34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ 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. - **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. diff --git a/tests/test_startup.py b/tests/test_startup.py index 030bebdc..28f6baf8 100644 --- a/tests/test_startup.py +++ b/tests/test_startup.py @@ -68,9 +68,14 @@ 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. - - Runs a separate short-lived container with VEKTRA_LLM_PROVIDER unset. + """A required env var with no usable value fails startup, not the first query. + + Runs a short-lived container with `-e VEKTRA_LLM_PROVIDER=`, which sets the + variable to the empty string — it does not unset it, as this docstring used to + claim. That mattered: `str` accepted `""`, so 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. `llm_provider` is now `min_length=1` + and both cases fail at step 1 with a structured error. """ result = subprocess.run( [ 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.", ) From eebb3d94c0c7ce68b6c1c249afc5c3268b3fd846 Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 22:35:19 +0000 Subject: [PATCH 4/5] test(startup): file BUG-025 for the traceback that leaks past the structured error With the empty-provider fix in place, tests/test_startup.py exposes the next layer: step 1 emits the structured [STARTUP ERROR] block, and then `raise SystemExit(1)` travels out of the ASGI lifespan into uvicorn, which logs the exception. The operator who typo'd an env var gets the good message *and* a Python traceback. NFR-009 asks for the former instead of the latter, not both, and it applies to all 11 steps, not just config validation. The fix is to validate before uvicorn.run() rather than inside the lifespan -- the boot path BUG-024 broke, and vektra-app's own tests drive that lifespan in-process and expect SystemExit. Too much to bolt onto a PR about test execution, so it is filed as BUG-025 with the analysis and the trap written down. The assertion is split into its own test with xfail(strict=True): the defect stays visible and CI stays honest, and when BUG-025 lands the test XPASSes and the suite goes red until the marker is removed. A known defect that cannot rot. Co-Authored-By: Claude Opus 4.8 (1M context) --- .s2s/BACKLOG.md | 35 +++++++++++++++++++++++++++++++- CHANGELOG.md | 2 +- tests/test_startup.py | 46 +++++++++++++++++++++++++++++++------------ 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/.s2s/BACKLOG.md b/.s2s/BACKLOG.md index b60caf1b..669a54b8 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -163,6 +163,37 @@ They need Docker, and they now carry the `integration` marker, so the unit runs --- +### 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 @@ -188,7 +219,9 @@ Proven by breaking it five ways, each going red on the right assertion and green **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 a product bug.** 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. 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 a real defect. +**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 8175ac34..06433437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ 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. +- **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. diff --git a/tests/test_startup.py b/tests/test_startup.py index 28f6baf8..084f56b3 100644 --- a/tests/test_startup.py +++ b/tests/test_startup.py @@ -67,15 +67,15 @@ def test_startup_complete_logged() -> None: ) -def test_graceful_failure_on_missing_config() -> None: - """A required env var with no usable value fails startup, not the first query. - - Runs a short-lived container with `-e VEKTRA_LLM_PROVIDER=`, which sets the - variable to the empty string — it does not unset it, as this docstring used to - claim. That mattered: `str` accepted `""`, so 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. `llm_provider` is now `min_length=1` - and both cases fail at step 1 with a structured error. +def _run_with_empty_llm_provider() -> str: + """Boot a throwaway container whose VEKTRA_LLM_PROVIDER is the empty string. + + `-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( [ @@ -94,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" ) From 6b998558c464d17c192bc81fc4679bdf719c4a41 Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 23:00:16 +0000 Subject: [PATCH 5/5] =?UTF-8?q?chore(ci):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20isolate=20token=20in=20test-structure,=20declare=20pyyaml?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two verified review findings from the PR bots: - test-structure ran PR-authored test code without persist-credentials: false, unlike test-app and app-integration which strip the token from .git/config for exactly that reason. Same threat model; same fix. - test_suite_execution_coverage.py imports yaml, but vektra-shared did not declare pyyaml — it relied on the root dev group leaking into a `uv sync --package vektra-shared`. It works (both test-shared and test-structure were green), but an implicit transitive dependency for a guard is the precise fragility this PR is about, so declare it where it is used. A third bot comment (class-level integration marks) was already fixed in 1b2ceef before the comment landed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci-unit.yml | 3 +++ uv.lock | 2 ++ vektra-shared/pyproject.toml | 4 ++++ 3 files changed, 9 insertions(+) diff --git a/.github/workflows/ci-unit.yml b/.github/workflows/ci-unit.yml index 8dbfec18..fe8be39c 100644 --- a/.github/workflows/ci-unit.yml +++ b/.github/workflows/ci-unit.yml @@ -211,6 +211,9 @@ jobs: 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 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", ]