ci: replace starter workflow with real gates (#75)

[leo-aa88]

Summary

.github/workflows/ci.yml was the unmodified GitHub Actions Python starter template — it enforced essentially nothing this project cares about: lint via flake8 (project uses ruff), everything non-fatal run with --exit-zero, deps installed from a hand-maintained requirements.txt, and bare pytest with no Postgres (so all of tests/integration/ skipped on the DB_URL guard). Now unblocked since #68 made make lint pass on main.

Changes

• Lint gate: make lint (ruff), blocking on failure. Added ruff to the dev extra so the toolchain is reproducible from pip install -e ".[dev]" (it was previously only present via a global install, so CI would have failed with ruff: not found).
• Deps: install via pip install -e ".[dev]"; delete requirements.txt (a drifting hand-mirror of pyproject.toml, per the "delete or generate-and-diff" acceptance option) and repoint the Makefile, README.md, and CONTRIBUTING.md. The Dockerfile already used the extra.
• Integration tests actually run: pgvector/pgvector:pg16 service container + DB_URL, via make test-int. The suites self-provision their schema (Base.metadata.create_all + CREATE EXTENSION vector), so no migration step is needed.
• Matrix over Python 3.10 and 3.12.
• docker build . smoke job.
• Coverage floor on src/core via --cov-fail-under=85 — current is ~87%, so this is a floor to ratchet upward, not an aspirational target.
• OpenAPI export + release-attach preserved (the release job now needs: openapi).

⚠️ Action needed on merge

Job names change (build → test / openapi / docker). If branch protection lists build as a required status check, update it to the new names (test (3.10), test (3.12), openapi, docker) or this and future PRs will wait on a check that no longer runs.

Testing

Locally (the parts runnable without the CI services):

• make lint → All checks passed
• pytest tests/unit/ --cov=src/core --cov-fail-under=85 → 87.21%, 911 passed
• .github/workflows/ci.yml validated as YAML

Integration/docker/matrix steps run in CI on this PR.

Closes #75

🤖 Generated with Claude Code

[leo-aa88]

COMMENT

(Would be REQUEST CHANGES — GitHub blocks a formal changes-request on your own PR, so treat the two inline BLOCKING findings as merge-blockers.)

The CI redesign itself is right and is exactly what #75 asked for — ruff lint gate, unit tests with a src/core coverage floor, real integration tests against a pgvector/pgvector:pg16 service across Python 3.10/3.12, a docker smoke build, and OpenAPI export preserved (release needs: openapi). I verified the wiring at head f4d6a1e:

• CONFIRMED — no dangling requirements.txt. Deletion is clean; Makefile (drops install-requirements), README.md, CONTRIBUTING.md repointed to pip install -e ".[dev]"; Dockerfile already used the extra; ruff added to the dev extra so make lint is reproducible in CI.
• CONFIRMED — the integration gate is real, not a fake skip. Tests guard on not DB_URL and not INTEGRATION_TESTS; the test job sets DB_URL at job level and make test-int sets INTEGRATION_TESTS=1, so the suite runs (20 collected+run per matrix leg, not skipped). scripts/export_openapi.py exists; the service has proper health-checks.

But the PR's own CI is red, and merging it would immediately make main's new gates fail. Three surfaced problems (all pre-existing, which is the gate doing its job):

1. test (both matrix legs): 2 failed / 20 passed — a test-isolation bug in test_ingest_dedup (global row count) and a likely real CloudWatch-timestamp-fallback bug. Inline on the integration step.
2. docker: metadata-generation-failed — Dockerfile copies pyproject.toml and installs before COPY . ., but hatchling needs README.md at metadata time. Inline on the docker step.

Minor (non-blocking):

• MINOR — silent-skip escape hatch. Each integration module also pytest.skip("Cannot connect to database") if check_connection() fails. With the health-checked service this shouldn't trigger, but for a gate whose thesis is "integration tests were being skipped," consider failing the job when 0 integration tests run (e.g. assert a non-zero collected count) so a future DB-wiring regression can't go green silently.
• Heads-up (already in your PR description): job rename build → test/openapi/docker means branch-protection required-check names must be updated, or PRs will block on a check that no longer runs.

VERDICT

The CI design is correct and worth landing — but not at its current SHA, because its own test and docker jobs fail and merging would red-line main. Fix the Dockerfile readme ordering and the two integration failures (dedup isolation + CloudWatch timestamp) in this PR (or stack them ahead of it) so the gate goes green, then it's an easy APPROVE. Everything else is CONFIRMED sound.

[leo-aa88]

BLOCKING — this step is red on the PR's own CI run (test (3.10) and test (3.12)): 2 failed, 20 passed. The new gate is working exactly as intended — it runs the integration suite against real Postgres for the first time — and it immediately surfaces two genuine failures. Both must be fixed before this can merge green, otherwise merging makes main's required checks red from day one.

1. test_ingest_dedup.py::test_reingest_identical_lines_keeps_single_row — CONFIRMED test-isolation bug. assert 397 == 1. The assertion counts LogEntry globally (select(func.count()).select_from(LogEntry)), not scoped to the job/file it just ingested. In a shared CI database, rows from test_full_flow (which ingests the whole sample_incident corpus) and others are still present, so the global count is 397. Scope the count to the ingested job (or fingerprint), or run the test in a rolled-back transaction / truncate between tests. (The dedup logic may well be fine — the test is what's wrong.)

2. test_cloudwatch_ingest.py::test_cloudwatch_ingest_then_explain — LIKELY a real bug. assert 1 == 5: the query is job-scoped (ingestion_job_id == job.id, timestamp IS NOT NULL), so only 1 of 5 CloudWatch rows persisted with a timestamp. The test's own comment says this guards the CloudWatch-event-timestamp → LogEntry.timestamp fallback (rows previously persisted null and were invisible to windowed queries). Please confirm whether the adapter fallback regressed or the fixture/expectation is off — but as it stands the contract the test documents is violated.

Note these never ran locally — the PR description shows only pytest tests/unit/ was run — so CI is the first place they execute. That's the whole point of the PR, and it earned its keep on the first run.

[leo-aa88]

BLOCKING — the docker job fails on this PR's CI: pip install -e ".[dev]" → metadata-generation-failed. CONFIRMED root cause (pre-existing Dockerfile bug this new job exposes):

OSError: Readme file does not exist: README.md

Dockerfile does COPY pyproject.toml . then RUN pip install --no-cache-dir -e ".[dev]" before COPY . .. But the build backend is hatchling with readme = "README.md" in pyproject.toml, and hatchling validates that the readme file exists while generating metadata — README.md hasn't been copied into the image yet, so the editable install aborts.

Fix: copy the readme alongside the manifest before installing, e.g. COPY pyproject.toml README.md ./ (or move the install after COPY . ., at the cost of layer-cache efficiency). Same blocking rationale: a red docker check on merge means main is red.

[leo-aa88]

COMMENT

(Would be REQUEST CHANGES — self-PR, so the one inline BLOCKING is the merge-blocker.)

Good iteration — two of my three findings are fixed and I re-verified against the fresh CI run on 539bea3:

• CONFIRMED fixed — docker now passes. Reordering to COPY . . before pip install -e ".[dev]" is the correct fix: hatchling can now read README.md (and src/) at metadata time. Green.
• CONFIRMED fixed — dedup isolation. Adding Base.metadata.drop_all(engine) before create_all in each integration db_session fixture gives each module a clean slate; test_ingest_dedup now passes (21 passed this run vs 20 before, with only the CloudWatch case still red). openapi green too.

One blocker remains, and I owe a correction: the CloudWatch failure I previously called a "LIKELY real bug" is, on tracing, a test-fixture bug — the 5 identical events dedup to a single row via ux_log_entries_dedup (scope, source_ref, original_line_hash, timestamp), so timestamped == 5 is unreachable while the fallback itself works. Details and a test-only fix inline. It's the last thing keeping test (3.10/3.12) red.

VERDICT

Almost there. The CI design is correct, docker + dedup isolation are fixed, and the sole remaining red is a one-line test-fixture fix (make the 5 CloudWatch messages distinct). Land that and the whole matrix goes green — then this is a clean APPROVE. No product code is at fault.

[leo-aa88]

BLOCKING (last one) — test (3.10) / test (3.12) are still red: test_cloudwatch_ingest_then_explain fails assert 1 == 5. And I need to correct my previous review: this is a CONFIRMED test-fixture bug, NOT the product bug I labelled "LIKELY."

I traced it on a clean DB (so it's not the isolation issue you just fixed). Root cause:

• The dedup upsert keys on ux_log_entries_dedup = (scope, source_ref, original_line_hash, timestamp) (service.py:178, ON CONFLICT DO NOTHING).
• The fixture emits 5 byte-identical events — same now_ms, same message JSON — in for _ in range(5). So all four key columns are identical across the 5, and the upsert collapses them to a single row (parsed_count == 5 passes because that counts pre-dedup; deduped_count absorbs the other 4).
• That surviving row does carry the fallback timestamp, so timestamped is 1, and == 5 can never hold. The CloudWatch event-timestamp → LogEntry.timestamp fallback is actually working — the assertion is simply incompatible with content-dedup.

Fix is test-only: make the 5 messages distinct so they don't dedup, e.g. embed the loop index:

'"message": "Stripe webhook signature verification failed #%d",' % i

(iterate for i in range(5)), then all 5 persist and timestamped == 5 holds and genuinely proves the fallback lands for every row. Alternatively assert timestamped == 1 and add distinct rows separately — but distinct messages keeps the test's stated intent.

With this, the test job goes green and the PR is mergeable.

[leo-aa88]

APPROVE

LGTM. The final commit applies the exact test-only fix from my last review — distinct per-event message (attempt {i}) and timestamp (now_ms + i * 1000) so content-dedup no longer collapses the five CloudWatch events — and the whole matrix is now green.

CONFIRMED green on a87ea4c: test (3.10) ✓, test (3.12) ✓, docker ✓, openapi ✓ (publish-openapi-release correctly skips off-tag). Every blocker I raised across the three reviews is resolved:

• Dockerfile now COPY . . before pip install -e → hatchling reads README.md at metadata time (docker green).
• Integration db_session fixtures drop_all before create_all → real-DB isolation (dedup test green).
• CloudWatch events made distinct → all five persist with the event-timestamp fallback, timestamped == 5 holds and now genuinely proves the fallback lands for every row (not a product bug, as corrected last round).

The change is sound: now_ms + 4*1000 stays inside the 1h+5m window, parsed_count == 5 and services_detected still hold, and the five distinct (original_line_hash, timestamp) keys defeat the dedup upsert as intended.

The CI redesign itself (verified earlier) does exactly what #75 asked — ruff gate, unit + src/core coverage floor, real Postgres+pgvector integration across Py 3.10/3.12, docker smoke, OpenAPI preserved, clean requirements.txt removal.

Reminder (from your PR description): on merge, update branch-protection required checks from build to the new names (test (3.10), test (3.12), openapi, docker), or PRs will wait on a check that no longer runs.

VERDICT

All gates green, all prior findings resolved, no product code implicated. No BLOCKING/MAJOR/MINOR findings remain. Merge it (after the required-check rename). (Comment rather than Approve — GitHub blocks self-approval on your own PR.)