diff --git a/backend/tests/integration/test_migrations.py b/backend/tests/integration/test_migrations.py index c9c3539f..4725468a 100644 --- a/backend/tests/integration/test_migrations.py +++ b/backend/tests/integration/test_migrations.py @@ -92,6 +92,30 @@ def _sync_database_url() -> str: return get_settings().database_url.replace("postgresql+asyncpg://", "postgresql://") +def _current_head() -> str: + """Resolve the current Alembic head revision id at test-body execution time. + + Invokes ``uv run alembic heads`` from the repo root and parses the first + whitespace-separated token of the single output line as the head id. + Asserts the chain has exactly one head — if a future branch lands and + Alembic emits multiple head lines, silently picking the first would mask + a real schema-design issue. Raise ``AssertionError`` instead, naming + ``alembic merge`` as the canonical fix. + + MUST be called inside test bodies, NOT at module import. The module-level + ``pytestmark = pytest.mark.skipif(not _postgres_reachable(), ...)`` only + runs at collection time; an import-time invocation would bypass it and + fail on hosts where the integration tests are meant to skip. + """ + result = _alembic("heads").stdout + lines = [line for line in result.splitlines() if line.strip()] + assert len(lines) == 1, ( + f"Expected exactly one Alembic head, got {len(lines)}: {lines!r}. " + "Run `alembic merge` to consolidate." + ) + return lines[0].split()[0] + + @pytest.fixture def fresh_db() -> Iterator[None]: """Roll back to base, then leave whatever state the test ends in. @@ -120,16 +144,8 @@ def test_upgrade_head_creates_alembic_version(self, fresh_db: None) -> None: result = conn.execute(text("SELECT version_num FROM alembic_version")) row = result.fetchone() assert row is not None, "alembic_version table empty after upgrade head" - # Baseline is "0001" per migrations/versions/0001_baseline.py. - # Head extended by feat_data_table_primitive migrations - # 0008–0013 (search_vector columns + GIN indexes on 6 tables); - # 0014 adds clusters.target_filter (feat_cluster_target_filter); - # 0015 adds trials.per_query_metrics (feat_pr_metric_confidence); - # 0016 adds config_repos.last_merged_proposal_id - # (feat_config_repo_baseline_tracking); - # 0017 adds proposals.last_polled_at - # (chore_reconciler_terminal_closed_no_poll). - assert row[0] == "0017" + # Head is resolved dynamically via _current_head() — see helper docstring. + assert row[0] == _current_head() finally: engine.dispose() @@ -152,9 +168,8 @@ def test_round_trip(self, fresh_db: None) -> None: with engine.connect() as conn: row = conn.execute(text("SELECT version_num FROM alembic_version")).fetchone() assert row is not None - # Head: 0017 (chore_reconciler_terminal_closed_no_poll adds - # proposals.last_polled_at). - assert row[0] == "0017" + # Head is resolved dynamically via _current_head() — see helper docstring. + assert row[0] == _current_head() finally: engine.dispose() diff --git a/docs/00_overview/DASHBOARD.md b/docs/00_overview/DASHBOARD.md index 03364f6e..5b85374a 100644 --- a/docs/00_overview/DASHBOARD.md +++ b/docs/00_overview/DASHBOARD.md @@ -6,7 +6,7 @@ _Top-level index across MVP1 → GA v1+ as of **2026-05-23**. Click a release na | Release | Theme | Progress | Status | |---|---|---|---| -| [MVP1 / v0.1](MVP1_DASHBOARD.md) | The Loop | 69 / 69 scoped done · 5 remaining | **In progress** | +| [MVP1 / v0.1](MVP1_DASHBOARD.md) | The Loop | 69 / 70 scoped done · 6 remaining | **In progress** | | [MVP1.5 / v0.1.5](MVP1_5_DASHBOARD.md) | Real Signals | 1 item(s) queued | **Held / queued** | | [MVP2 / v0.2](MVP2_DASHBOARD.md) | Observable | 1 / 1 scoped done · 1 remaining | **In progress** | | MVP3 / v0.3 | Production Stacks | — | **Not yet scoped** | diff --git a/docs/00_overview/MVP1_DASHBOARD.md b/docs/00_overview/MVP1_DASHBOARD.md index 5678d3d9..07e3181e 100644 --- a/docs/00_overview/MVP1_DASHBOARD.md +++ b/docs/00_overview/MVP1_DASHBOARD.md @@ -6,22 +6,28 @@ _Reflects feature-folder state as of **2026-05-23** (latest mtime of any planned ## Next up -All scoped MVP1 features shipped 🎉 +**[chore_migration_test_head_brittleness](../02_product/planned_features/chore_migration_test_head_brittleness/feature_spec.md)** — Chore, currently in **Plan** -Pull from the Idea backlog or capture a new feature spec. +> Replace the two hardcoded `"0017"` literals with a dynamic `_current_head()` helper that shells out to `uv run alembic heads` and returns the current head revision id. + +Plan approved; run /impl-execute to ship + +```bash +/impl-execute docs/02_product/planned_features/chore_migration_test_head_brittleness/implementation_plan.md --all +``` ## MVP1 Progress | Metric | Value | |---|---| -| Scoped items done | **69 / 69** (100%) — feat_/infra_/chore_/epic_ past idea stage | -| Pending work | **12** items (every not-done feat/infra/chore/bug across all priorities) | +| Scoped items done | **69 / 70** (99%) — feat_/infra_/chore_/epic_ past idea stage | +| Pending work | **13** items (every not-done feat/infra/chore/bug across all priorities) | | → P0 — do next | **0** unblocking / paying daily cost | | → P1 | **0** high-value, ready when P0 clears | -| → P2 (default) | 11 important to file, not blocking | +| → P2 (default) | 12 important to file, not blocking | | → Backlog | 1 captured for record, not planned | | Open bugs | 0 | -| Legacy "Path to MVP1" | 5 items — scoped-not-done + bugs + chore-ideas only (excludes feat/infra ideas) | +| Legacy "Path to MVP1" | 6 items — scoped-not-done + bugs + chore-ideas only (excludes feat/infra ideas) | | Backlog ideas | 7 idea-only feat/infra (not yet scoped into MVP1) | | In flight | 0 feature(s) actively shipping | @@ -122,9 +128,11 @@ Pull from the Idea backlog or capture a new feature spec. _None._ -### Plan (0) +### Plan (1) -_None._ +| # | Priority | Feature | Type | One-liner | Depends on | Status | +|---|---|---|---|---|---|---| +| 1 | P2 | [chore_migration_test_head_brittleness](../02_product/planned_features/chore_migration_test_head_brittleness/feature_spec.md) | Chore | Replace the two hardcoded `"0017"` literals with a dynamic `_current_head()` helper that shells out to `uv run alembic heads` and returns the current head revision id. | — | — | ### Spec (0) @@ -142,7 +150,7 @@ _None._ | 6 | P2 | [infra_agent_sibling_worktree_isolation](../02_product/planned_features/infra_agent_sibling_worktree_isolation/idea.md) | Infra | Running an autonomous agent in a sibling git worktree while the operator's main checkout has the Docker Compose stack up exposes two surfaces that aren't designed for parallel work: | — | Idea — tangential observations from the autonomous `chore_reconciler_terminal_closed_no_poll` agent run (PR #216, merged 2026-05-23) | | 7 | P2 | [infra_study_preflight_real_engine_integration](../02_product/planned_features/infra_study_preflight_real_engine_integration/idea.md) | Infra | `feat_study_preflight_overlap_probe`'s integration tests (AC-1 through AC-4b in [`backend/tests/integration/test_studies_api.py`](../../backend/tests/integration/test_studies_api.py)) use… | — | Idea — surfaced during `feat_study_preflight_overlap_probe` (PR ___) phase-gate review | | 8 | P2 | [chore_dashboard_pr_extraction_from_idea](../02_product/planned_features/chore_dashboard_pr_extraction_from_idea/idea.md) | Chore | Several early MVP1 features shipped before the `/pipeline` ceremony solidified, leaving them with only an `idea.md` in `implemented_features/_/`. Examples (as of 2026-05-23): | — | Idea — surfaced during the tangential-observations sweep of `bug_dashboard_depends_on_column_bloat` (PR pending) | -| 9 | P2 | [chore_migration_test_head_brittleness](../02_product/planned_features/chore_migration_test_head_brittleness/idea.md) | Chore | [`backend/tests/integration/test_migrations.py`](../../backend/tests/integration/test_migrations.py) has two assertions: | — | Idea — surfaced during `chore_reconciler_terminal_closed_no_poll` implementation | +| 9 | P2 | [chore_e2e_seed_acme_idea_obsolete](../02_product/planned_features/chore_e2e_seed_acme_idea_obsolete/idea.md) | Chore | [`chore_e2e_seed_acme_helper_dead/idea.md`](../02_product/planned_features/chore_e2e_seed_acme_helper_dead/idea.md) (dated 2026-05-21) proposed two paths: | — | Idea — surfaced during `chore_migration_test_head_brittleness` `/idea-preflight` pick (2026-05-23) | | 10 | P2 | [chore_studies_post_arq_spy_fixture](../02_product/planned_features/chore_studies_post_arq_spy_fixture/idea.md) | Chore | The studies POST handler at [`backend/app/api/v1/studies.py:307`](../../backend/app/api/v1/studies.py#L307) calls `await _enqueue_start_study(request, study_id)` after a successful create. The helper | — | Idea — surfaced during `feat_study_preflight_overlap_probe` (PR ___) phase-gate review | | 11 | P2 | [chore_template_library_expansion](../02_product/planned_features/chore_template_library_expansion/idea.md) | Chore | Three connected gaps: | — | Idea — surfaced during a UX review of parameter-tuning ergonomics on 2026-05-19. | | 12 | Backlog | [chore_e2e_seed_acme_helper_dead](../02_product/planned_features/chore_e2e_seed_acme_helper_dead/idea.md) | Chore | `seedAcmeProductsChain` is a 140-line helper that constructs a cluster + query_set + template + judgment_list + study + optional proposal/digest chain "Acme Products" demo scenario. The function is co | — | Idea — surfaced during `chore_e2e_test_rows_isolation` Story 1.2 coverage audit | @@ -158,6 +166,8 @@ graph LR classDef plan fill:#fef9c3,stroke:#854d0e,color:#854d0e; classDef spec fill:#dbeafe,stroke:#1e40af,color:#1e40af; classDef idea fill:#f1f5f9,stroke:#334155,color:#334155; + chore_migration_test_head_brittleness["migration test head brittleness"] + class chore_migration_test_head_brittleness plan; infra_foundation["foundation"] class infra_foundation done; feat_study_lifecycle["study lifecycle"] diff --git a/docs/00_overview/dashboard.html b/docs/00_overview/dashboard.html index caa58f11..b9ca4ff4 100644 --- a/docs/00_overview/dashboard.html +++ b/docs/00_overview/dashboard.html @@ -384,7 +384,7 @@

Releases

MVP1 / v0.1
The Loop
-
69 / 69 scoped done · 5 remaining
+
69 / 70 scoped done · 6 remaining
In progress
diff --git a/docs/00_overview/mvp1_dashboard.html b/docs/00_overview/mvp1_dashboard.html index b47d88e6..a3e4f080 100644 --- a/docs/00_overview/mvp1_dashboard.html +++ b/docs/00_overview/mvp1_dashboard.html @@ -382,12 +382,12 @@

RelyLoop MVP1 Dashboard

-
-
Next up
-
All scoped MVP1 features shipped 🎉
-
- Pull from the Idea backlog or capture a new feature spec. -
+
+
Next up — Chore, currently in Plan
+
Migration Test Head Brittleness
+
Replace the two hardcoded `"0017"` literals with a dynamic `_current_head()` helper that shells out to `uv run alembic heads` and returns the current head revision id.
+
Plan approved; run /impl-execute to ship
+ /impl-execute docs/02_product/planned_features/chore_migration_test_head_brittleness/implementation_plan.md --all
@@ -395,15 +395,15 @@

RelyLoop MVP1 Dashboard

MVP1 Progress

-
+
Scoped items done
-
69 / 69
-
100% of feat_/infra_/chore_/epic_ items past idea stage
-
+
69 / 70
+
99% of feat_/infra_/chore_/epic_ items past idea stage
+
Pending work
-
12
+
13
every not-done feat/infra/chore/bug across all priorities
@@ -425,7 +425,7 @@

MVP1 Progress

P2 (default)
-
11
+
12
important to file, not blocking
@@ -435,7 +435,7 @@

MVP1 Progress

Legacy "Path to MVP1"
-
5
+
6
scoped not-done + bugs + chore-ideas only (excludes feat/infra ideas)
@@ -570,13 +570,13 @@

Idea 12

-
Migration Test Head Brittleness
+
E2E Seed Acme Idea Obsolete
Chore P2
-
[`backend/tests/integration/test_migrations.py`](../../backend/tests/integration/test_migrations.py) has two assertions:
+
[`chore_e2e_seed_acme_helper_dead/idea.md`](../02_product/planned_features/chore_e2e_seed_acme_helper_dead/idea.md) (dated 2026-05-21) proposed two paths:
@@ -628,7 +628,19 @@

Spec 0

-

Plan 0

+

Plan 1

+ +
+
Migration Test Head Brittleness
+
+ Chore + P2 + +
+
Replace the two hardcoded `"0017"` literals with a dynamic `_current_head()` helper that shells out to `uv run alembic heads` and returns the current head revision id.
+ + +
@@ -1770,6 +1782,8 @@

Dependency graph (feat_ + infra_)

classDef plan fill:#fef9c3,stroke:#854d0e,color:#854d0e; classDef spec fill:#dbeafe,stroke:#1e40af,color:#1e40af; classDef idea fill:#f1f5f9,stroke:#334155,color:#334155; + chore_migration_test_head_brittleness["migration test head brittleness"] + class chore_migration_test_head_brittleness plan; infra_foundation["foundation"] class infra_foundation done; feat_study_lifecycle["study lifecycle"] @@ -1961,6 +1975,8 @@

Dependency graph (feat_ + infra_)

classDef plan fill:#fef9c3,stroke:#854d0e,color:#854d0e; classDef spec fill:#dbeafe,stroke:#1e40af,color:#1e40af; classDef idea fill:#f1f5f9,stroke:#334155,color:#334155; + chore_migration_test_head_brittleness["migration test head brittleness"] + class chore_migration_test_head_brittleness plan; infra_foundation["foundation"] class infra_foundation done; feat_study_lifecycle["study lifecycle"] diff --git a/docs/02_product/planned_features/chore_e2e_seed_acme_idea_obsolete/idea.md b/docs/02_product/planned_features/chore_e2e_seed_acme_idea_obsolete/idea.md new file mode 100644 index 00000000..205826e7 --- /dev/null +++ b/docs/02_product/planned_features/chore_e2e_seed_acme_idea_obsolete/idea.md @@ -0,0 +1,69 @@ +# chore_e2e_seed_acme_helper_dead idea is obsolete — close or update it + +**Date:** 2026-05-23 +**Status:** Idea — surfaced during `chore_migration_test_head_brittleness` `/idea-preflight` pick (2026-05-23) +**Priority:** P3 — doc-only cleanup; no behavioral impact. 5–10 LOC + a coverage-audit refresh. +**Origin:** While running `/idea-preflight` against [`chore_e2e_seed_acme_helper_dead/idea.md`](../chore_e2e_seed_acme_helper_dead/idea.md) to pick a non-overlapping feature for `feat_auto_followup_studies`, I discovered the idea's central premise is contradicted by current code: `seedAcmeProductsChain` now has a real Playwright caller at [`ui/tests/e2e/guides/06_create_and_monitor_study.spec.ts:28`](../../../../ui/tests/e2e/guides/06_create_and_monitor_study.spec.ts#L28) (`import { seedAcmeProductsChain } from '../helpers/seed';` and `await seedAcmeProductsChain();` at line 34). The spec uses the chain's `studyId` and `studyName` to render guide screenshots against a realistic "Acme Products" seeded study. +**Depends on:** None. + +## Problem + +[`chore_e2e_seed_acme_helper_dead/idea.md`](../chore_e2e_seed_acme_helper_dead/idea.md) (dated 2026-05-21) proposed two paths: + +- **Path A — Delete the helper** (recommended in the idea, "probably correct"). +- **Path B — Wire a spec that uses it.** + +Path B effectively shipped between 2026-05-21 and 2026-05-23 (commit `2cbcb93b chore(guides): regen guide 06 with realistic seed data + new target ...`). The guide-06 walkthrough spec imports the helper, calls it once per test run, and asserts the resulting `/studies/[id]` page renders. The helper is no longer dead code. + +Two stale artifacts result: + +1. **`docs/02_product/planned_features/chore_e2e_seed_acme_helper_dead/idea.md`** — still describes the helper as "0 Playwright spec callers" (line 6 + 14–16) and recommends Path A (delete) without acknowledging Path B has shipped. +2. **[`ui/tests/e2e/helpers/coverage-audit.md`](../../../../ui/tests/e2e/helpers/coverage-audit.md)** — the §"Coverage matrix" table still reports `seedAcmeProductsChain` as `0 specs — currently uncalled` and the §"Gaps" section + §"Verdict" both claim the helper is dead code. + +Neither is load-bearing, but both surface in `/pipeline status` / `MVP1_DASHBOARD.md` and confuse the next infra-sweep agent. + +## Proposed capabilities + +Two options: + +### Option A — Close the idea as "won't do / superseded by Path B" + +Add a one-block update to the top of [`chore_e2e_seed_acme_helper_dead/idea.md`](../chore_e2e_seed_acme_helper_dead/idea.md): + +``` +**Status (updated 2026-05-23):** Closed — Path B effectively shipped via guide-06 spec +(`ui/tests/e2e/guides/06_create_and_monitor_study.spec.ts`, commit `2cbcb93b`, +2026-05-22). The helper now has a real caller. No further action needed beyond +refreshing `coverage-audit.md` to reflect the new coverage state. +``` + +Refresh `ui/tests/e2e/helpers/coverage-audit.md`: +- Update the table row for `seedAcmeProductsChain` from `0 specs` to `guides/06_create_and_monitor_study.spec.ts`. +- Remove the §"Gaps" subsection (or replace it with `## Gaps\n\nNone as of 2026-05-23.`). +- Refresh the §"Verdict" sentence count from "8 of 9" to "9 of 9". + +### Option B — Move the folder to `implemented_features/` with a `pipeline_status.md` shim + +Per [`docs/02_product/planned_features/feature_templates/README.md`](../feature_templates/README.md) folder lifecycle, ideas that genuinely ship via another feature can be moved to `docs/00_overview/implemented_features/2026_05_22_chore_e2e_seed_acme_helper_dead/` with a tiny `pipeline_status.md` saying "shipped via `feat_pr_metric_confidence`-aware guide-06 spec, PR #X." This is more ceremony than Option A for very little extra discoverability win. + +**Recommendation:** Option A. The original idea was correctly captured as a small chore; it just got OBE'd by adjacent work. A one-paragraph status update + a coverage-audit refresh is the right ceremony for that shape. + +## Scope signals + +- **Backend:** none. +- **Frontend:** none (no spec edits — the spec is already correct). +- **Migration:** none. +- **Config:** none. +- **Audit events:** N/A. +- **Tests:** none. +- **Docs:** ~10 LOC across two files (`chore_e2e_seed_acme_helper_dead/idea.md` + `ui/tests/e2e/helpers/coverage-audit.md`). + +## Why deferred + +Surfaced during the `chore_migration_test_head_brittleness` `/idea-preflight` run, while the agent was looking for a feature that wouldn't overlap with `feat_auto_followup_studies`. Editing the original idea file directly on the migration-test chore's branch would mix scope across two unrelated folders (CLAUDE.md "Tangential discoveries" rubric: "Cross-subsystem mixing in one PR breaks reviewability"). Captured here so the next infra-sweep agent has a clean target. + +## Relationship to other work + +- **Originating idea:** [`chore_e2e_seed_acme_helper_dead`](../chore_e2e_seed_acme_helper_dead/idea.md) — the one this captures was OBE'd. +- **Source of OBE:** [`chore_guide_06_screenshot_refresh_target_picker`](../../../00_overview/implemented_features/2026_05_21_chore_guide_06_screenshot_refresh_target_picker/idea.md) — recommended the wrapper helper approach that became `seedAcmeProductsChain`. Now consumed by the guide-06 spec. +- **No mechanical dependency on this chore.** Any future infra-sweep can run it in any order. diff --git a/docs/02_product/planned_features/chore_migration_test_head_brittleness/feature_spec.md b/docs/02_product/planned_features/chore_migration_test_head_brittleness/feature_spec.md new file mode 100644 index 00000000..ac592c68 --- /dev/null +++ b/docs/02_product/planned_features/chore_migration_test_head_brittleness/feature_spec.md @@ -0,0 +1,278 @@ +# Feature Specification — `test_migrations.py` dynamic head lookup + +**Date:** 2026-05-23 +**Status:** Draft +**Owners:** Eric Starr (engineering) +**Related docs:** +- [`idea.md`](idea.md) +- [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) +- [`docs/05_quality/testing.md`](../../../05_quality/testing.md) — integration test layer convention +- [`CLAUDE.md`](../../../../CLAUDE.md) — Absolute Rule #5 (every migration includes `downgrade()` and round-trips cleanly) + +--- + +## 1) Purpose + +- **Problem:** Two assertions in [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) (lines 132 and 157) pin the Alembic head string to a hardcoded literal (currently `"0017"`). Every new migration bumps the head and breaks both assertions. The failure only manifests in `make test-integration` (which requires a running Postgres and is therefore skipped on the host that runs `make test-unit`-only verification per [`local-dev.md` §"Local-vs-CI test layers"](../../../03_runbooks/local-dev.md)). +- **Outcome:** Replace the two hardcoded `"0017"` literals with a dynamic `_current_head()` helper that shells out to `uv run alembic heads` and returns the current head revision id. Adding a new migration no longer requires a sympathy edit in this file. The verification path remains the same (CI's integration job runs the round-trip test against the canonical migration chain). +- **Non-goal:** Per-migration shape tests like [`backend/tests/integration/test_migration_0016.py`](../../../../backend/tests/integration/test_migration_0016.py). Those intentionally pin to a specific revision via `_alembic("downgrade", "0016") + _alembic("upgrade", "0016")` to assert the post-migration schema shape — that's correct behavior, not the failure mode this chore addresses. + +## 2) Current state audit + +### Existing implementations + +- [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) — Alembic round-trip integration tests. Marked `@pytest.mark.integration`; auto-skips on hosts where Postgres isn't TCP-reachable (the common case for `make test-integration` from the operator shell — Compose's Postgres is internal-only per CLAUDE.md "Ports" + `local-dev.md` §"Local-vs-CI test layers"). Has two assertion sites: + - **L132** inside `TestBaselineMigration::test_upgrade_head_creates_alembic_version`: asserts `row[0] == "0017"` after `alembic upgrade head`. + - **L157** inside `TestBaselineMigration::test_round_trip`: asserts `row[0] == "0017"` after `downgrade -1 + upgrade head`. + - Both assertions are preceded by a multi-line comment chain documenting each migration that bumped the head (`# 0001 baseline`, `# 0008–0013 search_vector`, `# 0014 clusters.target_filter`, ..., `# 0017 proposals.last_polled_at`). The comment chain grows by ~2 lines per migration; the chore preserves it for human readers but rewrites the assertion line. +- [`backend/tests/integration/test_migration_0016.py:174-184`](../../../../backend/tests/integration/test_migration_0016.py#L174) — the precedent for tests that legitimately pin to a specific revision: pins via `_alembic("downgrade", "0016")` + `_alembic("upgrade", "0016")` so the assertions about the column shape are tied to that specific migration's effects, regardless of how many later migrations exist. **Not in scope for this chore** — those tests are correctly pinned. +- [`migrations/versions/0017_proposals_last_polled_at.py`](../../../../migrations/versions/0017_proposals_last_polled_at.py) — current head migration. The string `"0017"` appears here as `revision: str = "0017"` (the Alembic-generated identifier); not touched by this chore. + +`grep -rn '"0017"' backend/tests/ migrations/` returns exactly three matches: the two assertions on lines 132 + 157 of `test_migrations.py` (in scope) and the `revision: str` declaration in `0017_proposals_last_polled_at.py` (out of scope — Alembic source-of-truth). + +### Navigation and link impact + +N/A — no UI, no URL changes, no docs link rewrites. + +### Existing test impact + +| Test file | Pattern | Count | Required change | +|---|---|---|---| +| `backend/tests/integration/test_migrations.py` | `assert row[0] == "0017"` | 2 | Replace with `assert row[0] == _current_head()` | + +No other test file references the hardcoded literal. + +### Existing behaviors affected by scope change + +- **Behavior:** Both assertions hardcode the latest revision id. **Current:** Every new migration requires editing both lines. **New:** Both assertions resolve the head dynamically; new migrations require no edit. **Decision needed:** No — the dynamic lookup is strictly less brittle. The comment chain documenting "what each migration adds" stays in the file as human-readable changelog (it's still valuable; just no longer load-bearing for the assertion). + +--- + +## 3) Scope + +### In scope + +- New private helper `_current_head() -> str` in [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py), defined once and reused by both test methods. +- Replace the two hardcoded `"0017"` assertions with `_current_head()` calls. +- Replace the growing per-migration changelog comment chain above each assertion with a single anchor comment pointing to `_current_head()`'s docstring. The detailed migration history continues to live in `migrations/versions/*.py` (each migration carries its own docstring) and `docs/00_overview/implemented_features/`; duplicating it inside `test_migrations.py` was the source of the 2-lines-per-migration tax that this chore eliminates. + +### Out of scope + +- Per-migration shape tests (`test_migration_0016.py`, `test_clusters_target_filter_migration.py`, `test_trials_per_query_metrics_migration.py`, etc.) that legitimately pin to a specific revision. Those use the right pattern already. +- The `revision: str = "0017"` declaration inside `migrations/versions/0017_proposals_last_polled_at.py`. That's Alembic's own source-of-truth; not a duplicate to remove. +- Adding new test coverage. The existing two integration tests already cover the assertion path. The helper is exercised whenever either test runs. +- Refactoring `test_migrations.py`'s `_alembic()` helper or the `pytestmark` skip logic. Out of scope. +- Linting/style sweeps unrelated to the two assertion sites. + +### API convention check + +N/A — this chore touches only an integration test file. No API endpoints, no router files, no error envelopes. + +### Phase boundaries + +Single phase, single PR. No phase boundaries. + +## 4) Product principles and constraints + +- **CLAUDE.md Absolute Rule #5 (every migration must include `downgrade()` and round-trip cleanly).** Preserved: the integration test continues to verify `alembic upgrade head && alembic downgrade -1 && alembic upgrade head` against the canonical chain. +- **Test layer convention** ([`docs/05_quality/testing.md`](../../../05_quality/testing.md)): integration tests are DB-backed and `@pytest.mark.integration`-marked. Preserved. +- **Skip-on-unreachable behavior** ([`test_migrations.py:46-66`](../../../../backend/tests/integration/test_migrations.py#L46)): the module-level `pytestmark` skip logic is unchanged. The dynamic helper does NOT run when the module skips — `_current_head()` is called inside test bodies, downstream of the skip gate. + +### Anti-patterns + +- **Do not** parse migration filenames from the `migrations/versions/` directory to derive the head — Option B in the idea, explicitly rejected. Alembic owns the revision-graph traversal (downgrades may not be alphabetically ordered if a future branch lands), and `alembic heads` is its source-of-truth output. +- **Do not** add `_current_head()` as a public module-level constant evaluated at import time (`_HEAD = _current_head()`). The helper must be called inside the test body so the `pytestmark` skip gate runs first; otherwise a non-Compose host that doesn't have `uv` installed will fail at import. The skip is module-level (`pytestmark`), so deferring the subprocess call to test-body execution time is correct. +- **Do not** replicate the helper into other migration tests. The per-migration tests (e.g., `test_migration_0016.py`) intentionally pin to a specific revision and should keep their hardcoded string. Only `test_migrations.py`'s "always-at-head" assertions need the dynamic lookup. +- **Do not** keep the per-migration changelog comments above each assertion. They grew by 2 lines per migration — exactly the maintenance tax this chore eliminates. The detailed changelog lives in `migrations/versions/*.py` (each migration's docstring) and `docs/00_overview/implemented_features/`; a single anchor comment pointing to `_current_head()`'s docstring is sufficient at the call site. + +## 5) Assumptions and dependencies + +- **Assumption:** `uv run alembic heads` is callable from the integration-test environment. Verified — the existing `_alembic()` helper at [`test_migrations.py:79-87`](../../../../backend/tests/integration/test_migrations.py#L79) already shells out as `["uv", "run", "alembic", *args]` from the repo root. The new helper follows the same invocation convention. +- **Dependency:** Alembic's `heads` subcommand prints the current head id followed by `(head)` (e.g., `0017 (head)\n`). Verified — this is documented Alembic behavior and matches every Alembic version the project has used. +- **Dependency:** CI (`.github/workflows/pr.yml`) runs `make test-integration` against a service-container Postgres. No CI changes required. + +## 6) Actors and roles + +- Primary actor: developer adding a new migration (the future contributor whose merge would have broken the head assertion). +- Role model: N/A — single-tenant install, no auth surface. + +### Authorization + +N/A — single-tenant install, no auth surface. + +### Audit events + +N/A — `audit_log` lands at MVP2; this chore touches no business state. + +--- + +## 7) Functional requirements + +### FR-1: Dynamic head lookup helper + +- **Requirement:** + - The system **MUST** define a private helper `_current_head() -> str` in `backend/tests/integration/test_migrations.py` that returns the current Alembic head revision id by invoking `uv run alembic heads` from the repo root. + - The helper **MUST** assert that `alembic heads` returns exactly one non-empty line and raise `AssertionError` if the project enters a multi-head state. Rationale: the spec assumes a single linear migration chain (no merge migrations), and silently picking the first head if a future branch lands would mask a real schema-design issue. The error message **MUST** name `alembic merge` as the canonical fix. + - The helper **MUST** parse the first whitespace-separated token of the (single) line as the revision id (matching Alembic's documented output format ` (head)`). + - The helper **MUST** raise `subprocess.CalledProcessError` if `alembic heads` exits non-zero (no defensive try/except — failure means the toolchain is broken and should fail loudly). + - The helper **MUST NOT** be invoked at module import time. It is called inside test bodies, downstream of the `pytestmark` skip gate. + +### FR-2: Replace hardcoded assertions + +- **Requirement:** + - The system **MUST** replace `assert row[0] == "0017"` at `test_migrations.py:132` with `assert row[0] == _current_head()`. + - The system **MUST** replace `assert row[0] == "0017"` at `test_migrations.py:157` with `assert row[0] == _current_head()`. + - The per-migration changelog comments preceding the assertions **MUST** be replaced with a single anchor comment of the form `# Head is resolved dynamically via _current_head() — see helper docstring.` to prevent the comment chain from growing on every future migration. The detailed changelog of which migration shipped what continues to live in `migrations/versions/*.py` and `docs/00_overview/implemented_features/`. + +### FR-3: No regression in skip behavior + +- **Requirement:** + - The module-level `pytestmark = pytest.mark.skipif(not _postgres_reachable(), ...)` **MUST** continue to gate every test in the module unchanged. + - The helper **MUST NOT** be referenced from any code that runs at module collection time (top-level assignments, default arguments, decorator arguments). This ensures that on a host where Postgres is unreachable, the module skips before `_current_head()` is ever invoked. +- **Skip-gate scope (informational, not a new requirement):** + - Hosts where Postgres is **unreachable** skip the module entirely — no `uv` invocation occurs. ✅ + - Hosts where Postgres is **reachable but `uv` is missing** are not protected by the skip gate. The pre-existing `_alembic()` helper at [`test_migrations.py:79-87`](../../../../backend/tests/integration/test_migrations.py#L79) already shells out as `["uv", "run", "alembic", *args]` and would fail in this case — so `uv` is an existing prerequisite of the integration-test toolchain, not a new one this chore introduces. The chore does not extend the skip gate to cover this case; the existing failure mode is intentional and out of scope. + +--- + +## 8) API and data contract baseline + +N/A across all subsections — this chore touches no API surface, no error codes, no enumerated value contracts. + +## 9) Data model and state transitions + +N/A — no schema changes, no new tables, no migrations, no state machines touched. + +## 10) Security, privacy, and compliance + +- Threats: none. The helper shells out to a developer-tool binary (`uv run alembic`) with no user-supplied input. No new attack surface. +- Controls: subprocess is invoked with a fixed argv list (no shell, no string interpolation). Matches the existing `_alembic()` helper's pattern. +- Secrets/key handling: N/A. +- Auditability: N/A — integration test, no business state mutation. +- Data retention/deletion/export impact: N/A. + +## 11) UX flows and edge cases + +N/A — no UI changes. + +## 12) Given/When/Then acceptance criteria + +### AC-1: Helper resolves current head from `alembic heads` + +- **Given** the integration test module is running (Postgres reachable, `uv` installed) +- **When** `_current_head()` is called against a single-head migration chain +- **Then** it returns the first whitespace-separated token of `alembic heads`'s single output line — i.e., the current head revision id (e.g., `"0017"` against the chain as of this spec) +- **Example values (current chain):** + - `subprocess.check_output(["uv", "run", "alembic", "heads"], cwd=REPO, text=True)` returns `"0017 (head)\n"` + - `_current_head()` returns `"0017"` +- **Example values (future chain):** + - After migration `0018_.py` ships, the same call returns `"0018 (head)\n"` and `_current_head()` returns `"0018"` — without any edit to the helper or its callers. + +### AC-1b: Helper rejects multi-head state + +- **Given** a hypothetical broken state where `alembic heads` returns two non-empty lines (multiple heads) +- **When** `_current_head()` is called +- **Then** it raises `AssertionError` with a message naming `alembic merge` as the canonical fix +- **Verification:** unit-level inspection of the helper, not a CI-exercised path (the project does not currently have multi-head migrations; this AC documents the safety net) + +### AC-2: `test_upgrade_head_creates_alembic_version` passes against the dynamic head + +- **Given** a fresh Postgres database, all migrations applied via `alembic upgrade head` +- **When** the test queries `SELECT version_num FROM alembic_version` +- **Then** the assertion `assert row[0] == _current_head()` passes — both sides resolve to the same head revision id + +### AC-3: `test_round_trip` passes against the dynamic head after downgrade + upgrade + +- **Given** a database at head, then `alembic downgrade -1` followed by `alembic upgrade head` +- **When** the test queries `SELECT version_num FROM alembic_version` +- **Then** the assertion `assert row[0] == _current_head()` passes + +### AC-4: Future migration adds no sympathy edit to `test_migrations.py` + +- **Given** a contributor adds a new migration `migrations/versions/0018_.py` with `down_revision = "0017"` and `revision = "0018"` +- **When** the contributor runs `make test-integration` against a Postgres that has applied the new migration +- **Then** both `test_upgrade_head_creates_alembic_version` and `test_round_trip` pass without any edit to `test_migrations.py` +- **Verification:** Manually exercised during implementation: create a stub migration that adds a no-op `op.execute("SELECT 1")`, run the tests, confirm green, delete the stub before commit. Documented in the implementation plan. + +### AC-5: Helper failure fails the test loudly + +- **Given** `uv run alembic heads` exits non-zero (e.g., broken `alembic.ini`, corrupt migration chain) +- **When** `_current_head()` is invoked +- **Then** a `subprocess.CalledProcessError` propagates out of the test (no defensive swallow). The test failure clearly identifies a toolchain problem rather than a stale-pin problem. + +### AC-6: Module skips when Postgres unreachable (no regression) + +- **Given** a host without Postgres reachable on the configured `DATABASE_URL` +- **When** pytest collects the module +- **Then** the entire module is skipped via the existing `pytestmark` — `_current_head()` is never called, no `uv` invocation occurs +- **Anti-regression note:** This is the reason FR-1 forbids module-import-time invocation of the helper. + +## 13) Non-functional requirements + +- **Performance:** negligible — adds one `subprocess.check_output` call per test method (≤2 calls per integration run). The subprocess invocation cost is dominated by the existing `_alembic("upgrade", "head")` calls; the helper adds no measurable overhead. +- **Reliability:** unchanged — the test continues to gate on the same migration chain. +- **Operability:** no new metrics, logs, or alerts. The integration test outcome (pass/fail) remains the only operational signal. +- **Accessibility/usability:** N/A. + +## 14) Test strategy requirements (spec-level) + +- **Unit tests:** None added. `_current_head()` is exercised by the two existing integration tests; a dedicated unit test would have to mock `subprocess.check_output` to verify the parsing logic, which adds little value over the integration-test coverage and creates a mock-divergence failure mode. Trade-off accepted. +- **Integration tests:** Existing — `test_upgrade_head_creates_alembic_version` and `test_round_trip` in `backend/tests/integration/test_migrations.py`. Both must continue to pass. +- **Contract tests:** N/A. +- **E2E tests:** N/A. +- **Verification gates the implementer must hit:** + - `make lint` (ruff) — green + - `make typecheck` (mypy --strict) — green; `_current_head()` is typed `() -> str` + - `make test-unit` — green (no new unit tests, but the suite must not regress) + - `make test-integration` — green against the local Compose Postgres (or CI Postgres service container). Both `test_upgrade_head_creates_alembic_version` and `test_round_trip` MUST pass with `_current_head()` resolving to `"0017"`. + +## 15) Documentation update requirements + +- `docs/01_architecture`: none. +- `docs/02_product`: none (the idea + spec + plan stay in the planned-features folder until finalization moves them to `implemented_features/`). +- `docs/03_runbooks`: none. The new helper is self-documenting via its docstring; no operator-facing behavior changes. +- `docs/04_security`: none. +- `docs/05_quality`: none — `testing.md`'s description of the integration test layer is unchanged. + +## 16) Rollout and migration readiness + +- **Feature flags / staged rollout:** N/A — test-only change. +- **Migration/backfill expectations:** N/A — no schema changes. +- **Operational readiness gates:** N/A. +- **Release gate:** PR-level — green CI (lint + typecheck + unit + integration + contract + Docker build + frontend) is the only gate. No staging deploy, no maintainer sign-off beyond Gemini review. + +## 17) Traceability matrix + +| FR ID | Acceptance Criteria IDs | Planned stories/tasks | Test files/suites | Docs to update | +|---|---|---|---|---| +| FR-1 (helper) | AC-1, AC-5, AC-6 | Story 1.1 (define `_current_head()`) | `backend/tests/integration/test_migrations.py` | none | +| FR-2 (replace assertions) | AC-2, AC-3, AC-4 | Story 1.2 (replace both assertions + comment update) | `backend/tests/integration/test_migrations.py` | none | +| FR-3 (skip regression) | AC-6 | Story 1.1 (helper placement) | `backend/tests/integration/test_migrations.py` | none | + +## 18) Definition of feature done + +This feature is complete when: + +- [ ] **CI-exercised acceptance criteria pass in CI**: AC-1 (helper resolves head — exercised by AC-2 + AC-3 invocations), AC-2 (upgrade-head test passes), AC-3 (round-trip test passes). +- [ ] **Inspection-level acceptance criteria documented and verified at code-review time**: AC-1b (multi-head assertion — verified by reading the helper), AC-4 (no-sympathy-edit — verified by the manual stub-migration check described in AC-4), AC-5 (loud failure on non-zero exit — verified by reading the helper), AC-6 (skip-on-unreachable — exercised every time CI runs but the skip path itself is the existing pytestmark, not a new code path introduced by this chore). +- [ ] `make lint`, `make typecheck`, `make test-unit`, `make test-integration`, `make test-contract` are all green locally on the feature branch before push. +- [ ] CI workflow (`.github/workflows/pr.yml`) is green on the PR. +- [ ] Manual AC-4 verification recorded in the implementation plan (stub migration added → tests pass → stub removed → tests still pass). +- [ ] Gemini Code Assist review comments adjudicated. +- [ ] Final GPT-5.5 review (per CLAUDE.md cross-model review policy) is clean or has documented Accept/Reject adjudications. +- [ ] No open questions remain in §19. +- [ ] Finalization PR moves `planned_features/chore_migration_test_head_brittleness/` to `implemented_features/_chore_migration_test_head_brittleness/` and updates `state.md`. + +## 19) Open questions and decision log + +### Open questions + +None. + +### Decision log + +- **2026-05-23 — Option A (`alembic heads` subprocess) selected** — Locked in idea.md preflight 2026-05-23. Option B (parse migration filenames) rejected: carries lexicographic-sortability risk if a future branch lands and offers no upside since `alembic heads` is Alembic's source-of-truth output. +- **2026-05-23 — No new unit test for `_current_head()`** — Mocking `subprocess.check_output` to test the parsing logic adds a mock-divergence failure mode without meaningfully exceeding the coverage the two existing integration tests already provide. Trade-off accepted; documented in §14. +- **2026-05-23 — Helper invoked inside test bodies, not at module import** — Required so the `pytestmark = pytest.mark.skipif(...)` gate runs first. A host without `uv` installed (or with no Postgres) will see the module skip rather than fail at collection time. +- **2026-05-23 — Per-migration changelog comments collapsed to a single anchor comment** — The detailed changelog of which migration shipped what is preserved in `migrations/versions/*.py` (each migration carries its own docstring) and `docs/00_overview/implemented_features/`. Keeping the comment chain in `test_migrations.py` duplicates that source and grows with every migration; collapsing it removes the maintenance tax without losing information. (Locked by FR-2 + §3 In scope + §4 Anti-pattern — no remaining ambiguity.) +- **2026-05-23 — Helper asserts single-head invariant** — `_current_head()` raises `AssertionError` if `alembic heads` returns more than one head, with a message naming `alembic merge` as the fix. Reason: silently picking the first head if a future branch lands would mask a real schema-design issue. Surfaced by GPT-5.5 cross-model review (cycle 1, Pass B finding #2, accepted). diff --git a/docs/02_product/planned_features/chore_migration_test_head_brittleness/idea.md b/docs/02_product/planned_features/chore_migration_test_head_brittleness/idea.md index 5d99c52a..08dcfe78 100644 --- a/docs/02_product/planned_features/chore_migration_test_head_brittleness/idea.md +++ b/docs/02_product/planned_features/chore_migration_test_head_brittleness/idea.md @@ -3,7 +3,7 @@ **Date:** 2026-05-23 **Status:** Idea — surfaced during `chore_reconciler_terminal_closed_no_poll` implementation **Priority:** P3 — small-but-recurring tax; safe to defer until the next ~3 migrations land. -**Origin:** Implementation of `chore_reconciler_terminal_closed_no_poll` (PR pending). After adding migration `0017`, two assertions in `backend/tests/integration/test_migrations.py` (lines 132 and 157) failed because they pinned `row[0] == "0016"`. Updating them to `"0017"` is mechanical but easy to miss in `make test-unit`-only verification flows. +**Origin:** Implementation of [`chore_reconciler_terminal_closed_no_poll`](../../../00_overview/implemented_features/2026_05_23_chore_reconciler_terminal_closed_no_poll/) (PR #216, shipped 2026-05-23). After adding migration `0017`, two assertions in `backend/tests/integration/test_migrations.py` (lines 132 and 157) failed because they pinned `row[0] == "0016"`. Updating them to `"0017"` is mechanical but easy to miss in `make test-unit`-only verification flows. ## Problem @@ -50,7 +50,7 @@ Then assertions become `assert row[0] == _current_head()`. Drift-proof; one func Walk `migrations/versions/`, find the file with the highest sortable name, parse `revision: str = "XXXX"`. More work; less robust if revision IDs ever stop being lexicographically sortable. -**Recommendation:** Option A. The `alembic heads` command is the source of truth Alembic itself uses. +**Decision (locked, 2026-05-23):** Option A. The `alembic heads` command is the source of truth Alembic itself uses. Option B (parse the latest migration file's `revision: str = "..."`) is rejected — it carries lexicographic-sortability risk and offers no upside. ## Scope signals diff --git a/docs/02_product/planned_features/chore_migration_test_head_brittleness/implementation_plan.md b/docs/02_product/planned_features/chore_migration_test_head_brittleness/implementation_plan.md new file mode 100644 index 00000000..f300ea9e --- /dev/null +++ b/docs/02_product/planned_features/chore_migration_test_head_brittleness/implementation_plan.md @@ -0,0 +1,433 @@ +# Implementation Plan — `test_migrations.py` dynamic head lookup + +**Date:** 2026-05-23 +**Status:** Draft +**Primary spec:** [`feature_spec.md`](feature_spec.md) +**Policy source(s):** [`CLAUDE.md`](../../../../CLAUDE.md) Absolute Rule #5 (migrations include `downgrade()` and round-trip cleanly); [`docs/05_quality/testing.md`](../../../05_quality/testing.md) (integration test layer). + +--- + +## 0) Planning principles + +- **Spec traceability first** — every story maps to one or more FR IDs from `feature_spec.md`. +- **Single phase, single PR** — the chore is ~15 LOC; further sub-phasing would be ceremony noise. +- **No new test infrastructure** — the helper is exercised by the two existing integration tests; the spec explicitly rejects adding a mocked unit test (see spec §14 + §19 decision log 2026-05-23). +- **Fail-loud** — the helper raises on the multi-head edge case rather than silently picking the first head (per spec FR-1 + AC-1b). +- **Preserve the skip gate** — the existing `pytestmark = pytest.mark.skipif(not _postgres_reachable(), ...)` continues to gate the module without modification (spec FR-3). + +## 1) Scope traceability (FR → epics/phases) + +| FR ID | Epic/Phase | Notes | +|---|---|---| +| FR-1 (dynamic head helper, with single-head assertion) | Epic 1 / Story 1.1 | New private `_current_head()` helper in `backend/tests/integration/test_migrations.py`. | +| FR-2 (replace hardcoded assertions + collapse comment chain) | Epic 1 / Story 1.2 | Two assertion call-sites + the per-migration changelog comments above them. | +| FR-3 (no regression in skip behavior) | Epic 1 / Story 1.1 | Helper is called inside test bodies only, downstream of the module-level `pytestmark`. No changes to skip logic. | + +No deferred phases — the spec defines a single phase (spec §3 "Phase boundaries: single phase, single PR"). + +## 2) Delivery structure + +**Epic → Story → Tasks → DoD.** One epic, two stories. + +### Story-level detail requirements + +Stories are test-only (no API surface, no migrations, no UI). Per the template: stories that are purely test-only "may omit Endpoints/Schemas sections but must still include New/Modified files and Tasks/DoD." Following that rule. + +### Conventions (applies to every story) + +- All Python additions follow the project's typing conventions (`from __future__ import annotations` already at the top of the target file; explicit return types; `mypy --strict` clean). +- No subprocess-by-string — every external command is invoked via a fixed `list[str]` argv (matches the existing `_alembic()` helper at [`test_migrations.py:79-87`](../../../../backend/tests/integration/test_migrations.py#L79)). +- No defensive try/except around `subprocess.check_output` — failure means the toolchain is broken and should propagate loudly (spec FR-1). +- The helper is private (`_current_head`, leading underscore) — no need to extend any module-level `__all__` (the file has none). + +### AI Agent Execution Protocol (applies to every story) + +Per template Step 0–9, plus the chore's narrower scope (no backend services, no frontend, no migrations): + +0. Read [`architecture.md`](../../../../architecture.md) (skim only — the chore touches no service the architecture doc describes) and [`state.md`](../../../../state.md) (confirm Alembic head, recent migrations, active branch). +1. Read story scope, FRs, ACs from the spec. +2. **Backend-only:** modify `backend/tests/integration/test_migrations.py`. No models, no migrations, no routers, no schemas. +3. Run `make test-unit` (must remain green — chore must not regress unrelated tests). +4. Skip frontend (no UI scope). +5. Skip E2E (no UI scope). +6. Update no docs in MVP1 release notes — the spec §15 says docs/01-05 require no updates. State.md gets a single-line entry at finalization (per template §4.0). +7. Skip migration round-trip (no schema change). The chore's WHOLE POINT is to make this verification not require a sympathy edit in test_migrations.py. +8. PR description must attach: `make lint`, `make typecheck`, `make test-unit`, `make test-integration` evidence, plus a one-line note for AC-4 manual verification (stub migration shipped + ran tests + removed stub + ran tests again). +9. At finalization (separate PR after merge): update `state.md` recent-changes section + move folder to `implemented_features/2026_05_23_chore_migration_test_head_brittleness/`. + +--- + +## Epic 1 — Dynamic head resolution in `test_migrations.py` + +### Story 1.1 — Add `_current_head()` helper with single-head invariant + +**Outcome:** A private helper in `backend/tests/integration/test_migrations.py` returns the current Alembic head revision id by invoking `uv run alembic heads`, and raises `AssertionError` if the project enters a multi-head state. The helper is callable only at test-body execution time (not at module import). + +**FR coverage:** FR-1, FR-3. AC coverage (verified by Story 1.2 + manual inspection): AC-1, AC-1b, AC-5, AC-6. + +**New files** + +None. + +**Modified files** + +| File | Change | +|---|---| +| [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) | Add `_current_head() -> str` private helper between the existing `_sync_database_url()` helper (line ~93) and the `fresh_db` fixture (line ~95). Helper invokes `uv run alembic heads` via the existing subprocess pattern, asserts exactly one non-empty output line, and returns the first whitespace-separated token of that line. | + +**Endpoints** + +N/A — test file only. + +**Key interfaces** + +```python +# backend/tests/integration/test_migrations.py + +def _current_head() -> str: + """Resolve the current Alembic head revision id at test-body execution time. + + Invokes ``uv run alembic heads`` from the repo root and parses the first + whitespace-separated token of the single output line as the head id. + Asserts the chain has exactly one head — if a future branch lands and + Alembic emits multiple head lines, silently picking the first would mask + a real schema-design issue. Raise ``AssertionError`` instead. + + Must be called inside test bodies, NOT at module import. The module-level + ``pytestmark = pytest.mark.skipif(not _postgres_reachable(), ...)`` only + runs at collection time; an import-time invocation would bypass it. + """ + result = _alembic("heads").stdout + lines = [line for line in result.splitlines() if line.strip()] + assert len(lines) == 1, ( + f"Expected exactly one Alembic head, got {len(lines)}: {lines!r}. " + "Run `alembic merge` to consolidate." + ) + return lines[0].split()[0] +``` + +**Pydantic schemas** + +N/A. + +**Tasks** + +1. Read the current state of `backend/tests/integration/test_migrations.py` to locate the helper insertion point (after `_sync_database_url()` at ~L93, before the `fresh_db` fixture at ~L95). Confirm `subprocess` is already imported (it is, line 33). +2. Add the `_current_head()` helper exactly as shown in **Key interfaces**. Match the existing module's docstring style (one-line summary + blank line + multi-paragraph body). +3. Do NOT modify the `pytestmark` skip gate (spec FR-3). +4. Run `make lint` (ruff) — green. +5. Run `make typecheck` (mypy --strict) — green; the helper is typed `() -> str`. +6. Run `make test-unit` — green. The helper isn't unit-tested directly (spec §14 decision); the suite must not regress unrelated tests. + +**Definition of Done (DoD)** + +- [ ] `_current_head()` exists in `backend/tests/integration/test_migrations.py` between `_sync_database_url()` and the `fresh_db` fixture. +- [ ] Helper signature is `_current_head() -> str` (typed, no kwargs). +- [ ] Helper raises `AssertionError` with an `alembic merge`-naming message if `subprocess.check_output` returns more than one non-empty line. **Verified by inspection at code review** (AC-1b is an inspection-level AC per spec §18 — the project does not currently have multi-head migrations to exercise it in CI). +- [ ] Helper does NOT appear in any top-level assignment, default-argument expression, decorator argument, or other code path that runs at module-collection time. **Verified by `grep` for the helper name outside test-body function definitions** (AC-6, anti-regression). +- [ ] No changes to `pytestmark`, `_postgres_reachable()`, or `_alembic()` (FR-3 scope hygiene). +- [ ] `make lint` / `make typecheck` / `make test-unit` green locally. + +### Story 1.2 — Replace hardcoded assertions and collapse the changelog comment chain + +**Outcome:** Both `assert row[0] == "0017"` sites in `backend/tests/integration/test_migrations.py` resolve the head dynamically via `_current_head()`. The growing per-migration changelog comment chain above each assertion is replaced with a single anchor comment pointing at the helper's docstring. Adding a new migration requires zero edits in this file. + +**FR coverage:** FR-2. AC coverage: AC-2, AC-3, AC-4. + +**New files** + +None. + +**Modified files** + +| File | Change | +|---|---| +| [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) | At ~L132 (inside `test_upgrade_head_creates_alembic_version`): replace `assert row[0] == "0017"` with `assert row[0] == _current_head()`. Replace the preceding ~10-line per-migration changelog comment (currently spanning lines ~123–131) with a single anchor comment of the form `# Head is resolved dynamically via _current_head() — see helper docstring.`. At ~L157 (inside `test_round_trip`): same assertion replacement; same comment collapse for the ~3-line preceding comment block (lines ~155–156). | + +**Endpoints** + +N/A. + +**Key interfaces** + +None — Story 1.2 only consumes the helper introduced in Story 1.1. + +**Pydantic schemas** + +N/A. + +**Tasks** + +1. Re-read [`backend/tests/integration/test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) to confirm the exact line numbers and surrounding comment text after Story 1.1's helper insertion (line numbers may shift by a few lines depending on the helper's exact placement and docstring length). +2. In `test_upgrade_head_creates_alembic_version`: replace the multi-line `# Baseline is "0001" per migrations/versions/0001_baseline.py. ...` comment block with the single anchor line `# Head is resolved dynamically via _current_head() — see helper docstring.`. Then change `assert row[0] == "0017"` to `assert row[0] == _current_head()`. +3. In `test_round_trip`: replace the multi-line `# Head: 0017 (chore_reconciler_terminal_closed_no_poll adds proposals.last_polled_at).` comment block with the same single anchor line. Then change `assert row[0] == "0017"` to `assert row[0] == _current_head()`. +4. Run `make lint` — green. +5. Run `make typecheck` — green. +6. Run `make test-unit` — green. +7. Run `make test-integration` against the local Compose Postgres (or the CI service container). Both `test_upgrade_head_creates_alembic_version` and `test_round_trip` MUST pass with `_current_head()` resolving to `"0017"`. +8. **AC-4 manual verification (mandatory, document in PR description):** + - Create a stub migration at `migrations/versions/0018_chore_sympathy_edit_check.py` with a minimal shape: `revision = "0018"`, `down_revision = "0017"`, `def upgrade(): op.execute("SELECT 1")`, `def downgrade(): pass`. + - Run `make test-integration` — both `test_upgrade_head_creates_alembic_version` and `test_round_trip` MUST pass without ANY edit to `test_migrations.py`. + - Delete `migrations/versions/0018_chore_sympathy_edit_check.py`. + - Run `make test-integration` again — both tests MUST pass against the original `"0017"` head. + - Paste the four-step shell transcript into the PR description. +9. `grep -rn '"0017"' backend/tests/` — confirm no remaining matches in `backend/tests/integration/test_migrations.py` (the `migrations/versions/0017_proposals_last_polled_at.py` match is expected and intentionally not touched, per spec §3 Out of scope). + +**Definition of Done (DoD)** + +- [ ] Both `assert row[0] == "0017"` instances at `test_migrations.py:132` and `:157` are replaced with `assert row[0] == _current_head()`. **Verified by `grep -c 'assert row\[0\] == _current_head()' backend/tests/integration/test_migrations.py` returning exactly `2` (the two call sites), and `grep -c '^def _current_head() -> str:' backend/tests/integration/test_migrations.py` returning exactly `1` (the helper definition).** Total `_current_head()` token count via raw `grep -c "_current_head()"` will be higher (~5) because the two anchor comments also contain the token — that's expected, not a regression. +- [ ] No remaining hardcoded `"0017"` literal in `backend/tests/integration/test_migrations.py`. **Verified by `grep -n '"0017"' backend/tests/integration/test_migrations.py` returning no output.** The match in `migrations/versions/0017_proposals_last_polled_at.py` is out of scope (Alembic's own `revision: str = ...` declaration). +- [ ] Per-migration changelog comment chains collapsed to a single anchor comment at each call site (the detailed changelog continues to live in `migrations/versions/*.py` docstrings + `docs/00_overview/implemented_features/`). +- [ ] `make test-integration` passes against the current chain (head `0017`). **AC-2 + AC-3 exercised.** +- [ ] AC-4 manual stub-migration verification documented in the PR description with the four-step shell transcript (create stub → tests pass → delete stub → tests pass). **AC-4 exercised.** +- [ ] No changes to `pytestmark`, `_postgres_reachable()`, `_alembic()`, the `fresh_db` fixture, or any test in `TestOptunaSchema`. + +--- + +## UI Guidance (required for frontend-facing work) + +**N/A — this chore has no frontend scope.** No user-facing components are added, moved, or deleted. The Legacy Behavior Parity table is omitted accordingly: no user-facing component >100 LOC is being deleted or migrated in this plan. + +--- + +## 3) Testing workstream (required) + +### 3.1 Unit tests + +- Location: `backend/tests/unit/` +- Scope: N/A for this chore. The spec §14 + §19 decision log (2026-05-23) explicitly rejects adding a mocked unit test for `_current_head()` — mocking `subprocess.check_output` to test the parsing logic creates a mock-divergence failure mode without exceeding the coverage the two existing integration tests already provide. Trade-off accepted. +- Tasks: none. +- DoD: existing unit suite remains green (`make test-unit`). + +### 3.2 Integration tests + +- Location: `backend/tests/integration/` +- Scope: the two **existing** tests in [`test_migrations.py`](../../../../backend/tests/integration/test_migrations.py) — `TestBaselineMigration::test_upgrade_head_creates_alembic_version` and `TestBaselineMigration::test_round_trip` — must continue to pass. No new tests added. +- Tasks: + - [ ] Run `make test-integration` after Story 1.2 against the current chain (head `0017`). Both tests pass with `_current_head()` resolving to `"0017"`. + - [ ] AC-4 stub-migration verification (described in Story 1.2 task 8). +- DoD: + - [ ] Both pre-existing integration tests pass. + - [ ] AC-4 manual verification documented in the PR description. + +### 3.3 Contract tests + +- Location: `backend/tests/contract/` +- Scope: N/A — no API surface. +- Tasks: none. +- DoD: existing contract suite remains green (`make test-contract`). + +### 3.4 E2E tests + +- Location: `ui/tests/e2e/` +- Scope: N/A — no UI surface. +- Tasks: none. +- DoD: existing E2E suite remains green (not run as part of this PR; CI gates it). + +### 3.5 Existing test impact audit (required for refactors and UI changes) + +| Test file | Pattern | Count | Action | +|---|---|---|---| +| `backend/tests/integration/test_migrations.py` | `assert row[0] == "0017"` | 2 | Replace with `assert row[0] == _current_head()` (Story 1.2). | +| `backend/tests/integration/test_migration_0016.py` | `assert ... row[0] == "0016"` | 2 (L180 + similar) | **No change.** This file legitimately pins to revision `0016` by using `_alembic("downgrade", "0016")` + `_alembic("upgrade", "0016")` to assert post-migration column shape — the right pattern for per-migration shape tests (spec §3 Out of scope). | +| `migrations/versions/0017_proposals_last_polled_at.py` | `revision: str = "0017"` | 1 | **No change.** Alembic's own source-of-truth declaration; out of scope. | +| All other files under `backend/tests/` | `"0017"` | 0 | None. (`grep -rn '"0017"' backend/tests/` returns only the two matches in `test_migrations.py`.) | + +### 3.5 Migration verification (if schema changes) + +N/A — this chore makes no schema changes. (The whole point of the chore is to remove the sympathy-edit tax on future migration verifications.) + +### 3.6 CI gates + +- [ ] `make lint` (ruff) — green +- [ ] `make typecheck` (mypy --strict) — green +- [ ] `make test-unit` — green +- [ ] `make test-integration` — green against the local Compose Postgres or the CI service container (`.github/workflows/pr.yml` Postgres service) +- [ ] `make test-contract` — green (no new contract tests; verifies unrelated regressions) +- [ ] `cd ui && pnpm lint && pnpm typecheck && pnpm test && pnpm build` — green (no UI changes; verifies unrelated regressions) + +--- + +## 4) Documentation update workstream (required) + +### 4.0 Core context files (required for every implementation plan) + +**`state.md`** — update at finalization: +- [ ] Add a one-line entry under "Recent changes" pointing at the chore PR. +- [ ] No active-branch change (the finalization PR cleans up the branch). +- [ ] Alembic head does NOT move — this chore makes no schema changes. + +**`architecture.md`** — no updates required. The chore is a test-file refactor; no new services, layers, data flows, or design decisions. Skipping this checklist item. + +**`CLAUDE.md`** — no updates required. No new conventions, rules, env vars, or build commands. Skipping this checklist item. + +### 4.1 Architecture docs (`docs/01_architecture`) + +- [ ] No updates required. (Spec §15.) + +### 4.2 Product docs (`docs/02_product`) + +- [ ] No updates required for the planned-features folder beyond finalization. At finalization, the folder moves to `docs/00_overview/implemented_features/2026_05_23_chore_migration_test_head_brittleness/`. + +### 4.3 Runbooks (`docs/03_runbooks`) + +- [ ] No updates required. The new helper is self-documenting via its docstring; no operator-facing behavior changes. + +### 4.4 Security docs (`docs/04_security`) + +- [ ] No updates required. + +### 4.5 Quality docs (`docs/05_quality`) + +- [ ] No updates required. `testing.md`'s description of the integration test layer is unchanged. + +**Documentation DoD** + +- [ ] `state.md` updated at finalization with a one-line recent-changes entry. +- [ ] `architecture.md`, `CLAUDE.md` unchanged (intentional — confirmed in §15 of the spec). +- [ ] docs/01–05 unchanged (intentional — confirmed in §15 of the spec). + +--- + +## 5) Lean refactor workstream (required) + +### 5.1 Refactor goals + +- **Eliminate the 2-lines-per-migration sympathy-edit tax** on `backend/tests/integration/test_migrations.py`. (Single explicit goal.) + +### 5.2 Planned refactor tasks + +- [ ] Backend refactor: replace two hardcoded head literals with a dynamic helper (Story 1.1 + Story 1.2). +- [ ] Frontend refactor: none. +- [ ] Remove dead/legacy branches: collapse the per-migration changelog comment chain (Story 1.2) — the changelog itself isn't dead, but the comment chain in `test_migrations.py` duplicates content that lives in `migrations/versions/*.py` and `docs/00_overview/implemented_features/`. + +### 5.3 Refactor guardrails + +- [ ] Behavioral parity proven by the two existing integration tests (`test_upgrade_head_creates_alembic_version` and `test_round_trip`) continuing to pass. +- [ ] AC-4 manual verification (stub migration → tests pass → stub removed → tests pass) proves the no-sympathy-edit property. +- [ ] `make lint` / `make typecheck` remain green. +- [ ] No expansion of product scope. The chore is explicitly bounded: ~15 LOC, one file, no per-migration shape tests touched. + +--- + +## 6) Dependencies, risks, and mitigations + +### Dependencies + +| Dependency | Needed by | Status | Risk if missing | +|---|---|---|---| +| `uv` installed and on PATH in the integration-test runner | Story 1.1 helper invocation | Implemented (CI's `pr.yml` uses `uv` already; the existing `_alembic()` helper at `test_migrations.py:79-87` already requires it) | Helper raises `FileNotFoundError` on hosts where `uv` is missing — same failure mode the pre-existing helpers already have. Out of scope to harden further. | +| Alembic's `heads` subcommand returns ` (head)` on stdout | Story 1.1 parsing logic | Implemented (documented Alembic behavior, stable across versions in use) | If Alembic ever changes the output format, the parser breaks. Recovery: update the parser. Low likelihood given Alembic's API stability. | +| Single-head migration chain | Story 1.1 single-head assertion | Implemented (the project has only had linear migrations to date; no `alembic merge` has been needed) | If a future branch introduces multi-head state, the helper raises a clear `AssertionError` naming the fix (`alembic merge`). This is preferred over silently picking the first head. | + +### Risks + +| Risk | Likelihood | Impact | Mitigation | +|---|---|---|---| +| The helper gets invoked at module-import time by a future contributor (e.g., as a default argument or top-level constant) | Low | Module fails at collection time on hosts that skip the integration tests — defeats the purpose of `pytestmark` | FR-3 explicitly forbids module-import-time invocation; the helper has a docstring stating this; the DoD verifies via `grep` that the helper name only appears inside test-body function definitions. | +| Hard-pinning the Story 1.1 line number for the helper insertion proves brittle (e.g., the file has been edited between when this plan was written and when execution runs) | Medium | Story 1.1 task 1 picks the wrong insertion line and breaks the file structure | Story 1.1 task 1 re-reads the file before inserting; the spec describes the insertion point relationally ("between `_sync_database_url()` and `fresh_db` fixture") rather than by absolute line number. | +| `make test-integration` runs in CI but the local operator runs it from a host where Postgres is internal-only and unreachable | Low | Local verification skips; the CI run is the only verification path | This is the existing reality per [`local-dev.md` §"Local-vs-CI test layers"](../../../03_runbooks/local-dev.md); not a regression introduced by this chore. The implementer should rely on CI for AC-2/AC-3 verification if their local Postgres is unreachable, and run the AC-4 stub-migration step against the local Compose stack via `make up` first. | + +### Failure mode catalog + +| Failure mode | Trigger | Expected system behavior | Recovery | +|---|---|---|---| +| `uv run alembic heads` exits non-zero | Broken `alembic.ini`, corrupt migration chain, missing `uv` | `subprocess.CalledProcessError` propagates out of the test; test fails loudly | Manual — fix the toolchain or migration chain | +| `alembic heads` returns multiple non-empty lines (multi-head state) | A future merge migration or unmerged branch introduces a second head | `_current_head()` raises `AssertionError` with a message naming `alembic merge` | Manual — run `alembic merge` to consolidate, then re-run tests | +| `alembic heads` returns empty output | Migration chain is broken / no revisions exist | `_current_head()` raises `AssertionError` (the `len(lines) == 1` check covers `len(lines) == 0` too) | Manual — investigate the migration chain | + +## 7) Sequencing and parallelization + +### Suggested sequence + +1. **Story 1.1** — add the helper. No dependency on existing tests beyond the file's structure. +2. **Story 1.2** — replace the two assertions + collapse the comment chains. Depends on Story 1.1 (the call sites need the helper to exist). + +Story 1.1 and Story 1.2 can be bundled into a single commit if preferred — they are tightly coupled and the diff is small. Splitting them into two commits is preferred for reviewability (the helper definition + the call-site swap are conceptually distinct). + +### Parallelization opportunities + +None — the two stories are sequential by data dependency. + +## 8) Rollout and cutover plan + +- **Rollout stages:** N/A — test-only change, no production behavior, no staged rollout. +- **Feature flag strategy:** N/A. +- **Migration/cutover steps:** N/A — no schema changes. +- **Reconciliation/repair strategy:** N/A — no external systems. + +## 9) Execution tracker (copy/paste section) + +### Current sprint + +- [ ] Story 1.1 — add `_current_head()` helper with single-head invariant +- [ ] Story 1.2 — replace the two hardcoded assertions and collapse the comment chains +- [ ] AC-4 manual verification (stub migration → tests pass → stub removed → tests pass) +- [ ] CI green (lint + typecheck + unit + integration + contract + Docker build + frontend) +- [ ] Gemini Code Assist review adjudicated +- [ ] Final GPT-5.5 cross-model review on the merged diff + +### Blocked items + +None. + +### Done this sprint + +- (populated during execution) + +## 10) Story-by-Story Verification Gate (Agent Checklist) + +Before marking any story complete, the executing engineer or agent must attach evidence for: + +- [ ] Files modified match story scope (`Modified files` tables — single file: `backend/tests/integration/test_migrations.py`). +- [ ] No endpoint contract changes (chore is test-only). +- [ ] Key interface (Story 1.1's `_current_head()`) implemented with the exact signature documented in **Key interfaces**. +- [ ] Required tests pass at every layer where applicable: + - [ ] `make test-unit` — green (no new unit tests; suite must not regress) + - [ ] `make test-integration` — green for both `test_upgrade_head_creates_alembic_version` and `test_round_trip` (or skipped with a documented reason if Postgres is unreachable locally — CI is then the authoritative gate) + - [ ] `make test-contract` — green (no new contract tests; suite must not regress) + - [ ] `cd ui && pnpm test` — green (no UI changes; suite must not regress) +- [ ] No migration round-trip evidence required (no schema change). +- [ ] AC-4 stub-migration verification documented in the PR description with a four-step shell transcript. +- [ ] No docs/01–05 updates required (per spec §15). + +## 11) Plan consistency review (required before execution) + +Performed inline during plan generation. Findings: + +1. **Spec ↔ plan FR coverage:** All three FRs (FR-1, FR-2, FR-3) covered by Story 1.1 + Story 1.2. ✅ +2. **Spec ↔ plan AC coverage:** AC-1, AC-1b, AC-2, AC-3, AC-4, AC-5, AC-6 all mapped to either a test pass, an inspection check, or the manual stub-migration verification (per spec §18). ✅ +3. **Spec ↔ plan endpoint count:** Both are zero. Trivially consistent. ✅ +4. **Spec ↔ plan error code coverage:** Both are zero. Trivially consistent. ✅ +5. **Test file count:** zero new test files; only the existing two integration tests are exercised. Matches §3 testing workstream inventory. ✅ +6. **Open questions resolved:** Spec §19 reports zero open questions. ✅ +7. **Plan ↔ codebase verification:** + - File path `backend/tests/integration/test_migrations.py` exists ✅ + - The two `assert row[0] == "0017"` sites exist at L132 + L157 ✅ (verified by `grep -n '"0017"' backend/tests/integration/test_migrations.py` returning exactly those two matches) + - `subprocess` is imported at L33 ✅ + - Existing `_alembic()` helper at L79-87 uses the same `["uv", "run", "alembic", *args]` invocation convention the new helper follows ✅ + - `pytestmark` skip gate at L69-76 is unmodified by the plan ✅ + - Alembic head is `0017` (verified by `ls migrations/versions/ | tail -1` showing `0017_proposals_last_polled_at.py`) ✅ +8. **Infrastructure path verification:** + - Migration directory: `migrations/versions/` (verified by `ls migrations/versions/`). NOT `backend/alembic/versions/` or `backend/app/db/migrations/versions/`. ✅ + - Test directory: `backend/tests/integration/` (verified by `ls backend/tests/integration/test_migrations.py`). ✅ +9. **Enumerated value contract audit:** N/A — no filters, no dropdowns, no sort controls, no badges, no API enums. ✅ +10. **Admin control / ceiling enforcement audit:** N/A — MVP1 has no admin/tenant model. ✅ +11. **Audit-event coverage audit:** N/A — MVP1 has no `audit_log` table; chore touches no business state regardless. ✅ +12. **Persistence scope:** N/A — no client-side storage. +13. **Legacy behavior parity:** N/A — no user-facing component >100 LOC is being deleted or migrated. + +No unresolved findings. + +--- + +## 12) Definition of plan done + +- [x] Every FR is mapped to stories/tasks/tests/docs updates. (FR-1 → Story 1.1; FR-2 → Story 1.2; FR-3 → Story 1.1 + DoD asserts.) +- [x] Every story includes New files (zero, explicit), Modified files, Tasks, and DoD. (Endpoints / Schemas omitted per template rule — story is test-only.) +- [x] Test layers explicitly scoped (Unit: N/A by spec decision; Integration: existing tests; Contract/E2E: N/A). +- [x] Documentation updates across docs/01-05 are planned (zero updates, explicit per spec §15; `state.md` gets one line at finalization). +- [x] Lean refactor scope and guardrails explicit. +- [x] Phase/epic gates measurable (Story 1.2 DoD includes the AC-4 verification). +- [x] Story-by-Story Verification Gate included (§10). +- [x] Plan consistency review (§11) performed with no unresolved findings. diff --git a/docs/02_product/planned_features/chore_migration_test_head_brittleness/pipeline_status.md b/docs/02_product/planned_features/chore_migration_test_head_brittleness/pipeline_status.md new file mode 100644 index 00000000..c12afbe6 --- /dev/null +++ b/docs/02_product/planned_features/chore_migration_test_head_brittleness/pipeline_status.md @@ -0,0 +1,24 @@ +# Pipeline Status — chore_migration_test_head_brittleness + +## Idea +- Status: Complete +- File: idea.md +- Preflight: passed 2026-05-23 (2 patches — Origin link refresh, decision lock for Option A) + +## Spec +- Status: Approved +- Date: 2026-05-23 +- File: feature_spec.md +- Cross-model review: GPT-5.5 passed (1 cycle, 5 findings: 0 High / 1 Medium / 4 Low — all accepted as minor clarity/precision improvements, no major contract or data changes) +- Phases: 1 (single phase, single PR) + +## Plan +- Status: Approved +- Date: 2026-05-23 +- File: implementation_plan.md +- Cross-model review: GPT-5.5 passed (1 cycle, 1 finding: 0 High / 1 Medium / 0 Low — Medium accepted and applied — DoD grep arithmetic fixed) +- Stories: 2 across 1 epic +- Phases covered: 1 (single phase, single PR) + +## Implementation +- Status: Not started