Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 67 additions & 34 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This workflow will install Python dependencies, run tests and lint with a single version of Python
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

# CI gates for raglogs. Enforces what the project actually cares about:
# ruff lint, unit tests with a src/core coverage floor, and integration tests
# against a real Postgres+pgvector service — across the supported Python range.
name: CI

on:
Expand All @@ -11,54 +11,87 @@ on:
paths-ignore:
- "**/*.md"
- "CODEOWNERS"
- "Makefile"
pull_request:
branches: [ "main" ]
paths-ignore:
- "**/*.md"
- "CODEOWNERS"
- "Makefile"

permissions:
contents: read

jobs:
build:
test:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
python-version: ["3.10", "3.12"]
services:
postgres:
image: pgvector/pgvector:pg16
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: raglogs
ports:
- 5432:5432
options: >-
--health-cmd "pg_isready -U postgres"
--health-interval 10s
--health-timeout 5s
--health-retries 5
env:
DB_URL: postgresql+psycopg://postgres:postgres@localhost:5432/raglogs
steps:
- uses: actions/checkout@v7
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v7
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -e ".[dev]"
- name: Lint (ruff)
run: make lint
- name: Unit tests (with src/core coverage floor)
# Floor starts at the current level (~87%); ratchet upward over time,
# never set an aspirational number that turns main red.
run: pytest tests/unit/ --cov=src/core --cov-report=term-missing --cov-fail-under=85
- name: Integration tests (real Postgres + pgvector)
run: make test-int

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


openapi:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
- name: Set up Python
uses: actions/setup-python@v7
with:
python-version: "3.12"
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -e ".[dev]"
- name: Export OpenAPI
run: PYTHONPATH=. python scripts/export_openapi.py
- name: Upload OpenAPI artifact
uses: actions/upload-artifact@v7
with:
name: openapi.json
path: clients/openapi.json

docker:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
- name: Set up Python 3.10
uses: actions/setup-python@v7
with:
python-version: "3.10"
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install flake8 pytest
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
- name: Lint with flake8
run: |
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
- name: Test with pytest
run: |
pytest
- name: Export OpenAPI
run: |
PYTHONPATH=. python scripts/export_openapi.py
- name: Upload OpenAPI artifact
uses: actions/upload-artifact@v7
with:
name: openapi.json
path: clients/openapi.json
- uses: actions/checkout@v7
- name: Docker build smoke test
run: docker build -t raglogs:ci .

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


publish-openapi-release:
if: startsWith(github.ref, 'refs/tags/')
needs: build
needs: openapi
runs-on: ubuntu-latest
permissions:
contents: write
Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ This document covers how to get set up, what the codebase expects, and how to su
git clone https://github.com/leo-aa88/raglogs
cd raglogs
python -m venv .venv && source .venv/bin/activate
pip install -r requirements.txt && pip install -e .
pip install -e ".[dev]"
cp .env.example .env
# Edit .env — set DB_URL at minimum
docker compose up postgres -d
Expand Down
7 changes: 4 additions & 3 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
libpq-dev \
&& rm -rf /var/lib/apt/lists/*

COPY pyproject.toml .
RUN pip install --no-cache-dir -e ".[dev]"

# Copy the whole project before installing: the hatchling build backend reads
# README.md (readme=) and src/ (packages=["src"]) to generate metadata, so an
# editable install can't run against pyproject.toml alone.
COPY . .
RUN pip install --no-cache-dir -e ".[dev]"

EXPOSE 8000

Expand Down
3 changes: 0 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ help:

# ── Setup ─────────────────────────────────────────────────────────────────────

install-requirements:
$(PIP) install -r requirements.txt

install-dev:
$(PIP) install -e ".[dev]"

Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1305,8 +1305,8 @@ injects the header, or keep auth off on loopback). `/health` stays public.
## Development

```bash
# Install everything
pip install -r requirements.txt && pip install -e .
# Install everything (runtime + dev tooling)
pip install -e ".[dev]"

# Unit tests (no DB needed)
make test-unit
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ dev = [
"pytest-cov>=5.0.0",
"anyio>=4.0.0",
"moto[logs]>=5.0.0",
"ruff>=0.6.0",
]

[project.scripts]
Expand Down
27 changes: 0 additions & 27 deletions requirements.txt

This file was deleted.

12 changes: 9 additions & 3 deletions tests/integration/test_cloudwatch_ingest.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ def db_session():
with engine.connect() as conn:
conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector"))
conn.commit()
# Clean slate so tests stay isolated when they share one database

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# (e.g. a single Postgres service across the whole CI run).
Base.metadata.drop_all(engine)
Base.metadata.create_all(engine)

with get_db() as db:
Expand All @@ -50,19 +53,22 @@ def test_cloudwatch_ingest_then_explain(db_session):
client.create_log_stream(logGroupName="/aws/lambda/my-service", logStreamName="stream-1")

now_ms = int(datetime.now(tz=timezone.utc).timestamp() * 1000)
# Distinct messages per event: identical content would be collapsed by
# the ingest content-dedup (see test_ingest_dedup), leaving one row and
# defeating this test's point — that all five persist with a timestamp.
client.put_log_events(
logGroupName="/aws/lambda/my-service",
logStreamName="stream-1",
logEvents=[
{
"timestamp": now_ms,
"timestamp": now_ms + i * 1000,
"message": (
'{"level": "error", "message": '
'"Stripe webhook signature verification failed", '
f'"Stripe webhook signature verification failed (attempt {i})", '
'"service": "billing-worker"}'
),
}
for _ in range(5)
for i in range(5)
],
)

Expand Down
3 changes: 3 additions & 0 deletions tests/integration/test_full_flow.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ def db_session():
with engine.connect() as conn:
conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector"))
conn.commit()
# Clean slate so tests stay isolated when they share one database
# (e.g. a single Postgres service across the whole CI run).
Base.metadata.drop_all(engine)
Base.metadata.create_all(engine)

with get_db() as db:
Expand Down
3 changes: 3 additions & 0 deletions tests/integration/test_ingest_dedup.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ def db_session():
with engine.connect() as conn:
conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector"))
conn.commit()
# Clean slate so tests stay isolated when they share one database
# (e.g. a single Postgres service across the whole CI run).
Base.metadata.drop_all(engine)
Base.metadata.create_all(engine)

with get_db() as db:
Expand Down
3 changes: 3 additions & 0 deletions tests/integration/test_trigger_search.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ def db_session():
with engine.connect() as conn:
conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector"))
conn.commit()
# Clean slate so tests stay isolated when they share one database
# (e.g. a single Postgres service across the whole CI run).
Base.metadata.drop_all(engine)
Base.metadata.create_all(engine)

with get_db() as db:
Expand Down