diff --git a/.claude/skills/test/SKILL.md b/.claude/skills/test/SKILL.md new file mode 100644 index 0000000..b4b372a --- /dev/null +++ b/.claude/skills/test/SKILL.md @@ -0,0 +1,62 @@ +--- +name: test +description: Write tests for a change following red-green-refactor TDD, testing behavior through this service's public HTTP interface rather than internals. Use during implement, before considering a ticket done. +--- + +Write the test first. Watch it fail for the right reason, a missing +behavior or a real bug, not a typo or an import error, then write the +minimum code to make it pass, then refactor with the test as a safety +net. Skipping straight to the implementation and testing it afterward is +not TDD, even if a test file ends up next to the code. + +Test through `/records`, `/records/{ave_id}`, `/records/{ave_id}/mitigation`, +and `/search` with FastAPI's `TestClient`, the same way a real caller +would, rather than importing and calling `_refresh_cache()` or +`_ensure_fresh()` directly. A test that reaches into an internal function +locks in that function's shape and breaks on refactors that don't change +any actual behavior; a test that hits the HTTP interface survives them. + +Never let a test touch the real network. `main.py` makes exactly one +outbound call, to `aveproject/ave`'s raw content; monkeypatch +`main._refresh_cache` to populate `main._cache` with a small, fake +fixture of one or two records instead of hitting GitHub. A test suite +that depends on GitHub being up, or on `aveproject/ave`'s current record +content, is not a test suite, it is a flaky integration check wearing a +test's clothes. + +Reset shared state before every test: `main._cache` and `main.limiter`'s +storage are both module-level singletons that persist across the whole +test session unless something clears them. An autouse fixture that +resets both before each test is not optional scaffolding, it is what +makes the rest of the suite trustworthy; without it, test order starts +to matter and failures stop being reproducible. + +One behavior per test, named as a sentence that describes it, +`test_get_mitigation_404s_when_record_has_no_mitigation`, not +`test_mitigation` or `test_2`. If a test's name can't describe what it +checks in one sentence, it's checking more than one thing and should be +split. + +Cover the behaviors that are actually load-bearing for this repo, not +just the happy path: +- Every route's 404 case, worded well enough that whatever it says is + actually useful to whoever hits it. +- `/mitigation` returns the neutral `mitigation` object, and nothing + else, and 404s cleanly when a record has none. +- `/search` matches across all four documented fields (`title`, + `description`, `attack_class`, `behavioral_fingerprint`), + case-insensitively, and returns an empty list rather than an error + for no matches. +- The resilience pattern from `CLAUDE.md`: a refresh failure with a + populated cache serves the stale data (200, not 500); a refresh + failure with an empty cache is a real failure (500, it has to + surface); startup logs and continues on a failed initial fetch rather + than crash-looping. +- Every route carries its `@limiter.limit(...)` decorator; a route + discovered without one while writing its tests is a bug in that + route, not a gap in the test. + +Run `pytest` before considering a ticket done, same bar as +`python3 -m py_compile main.py` in `implement`'s own checklist: a red +suite caught in code review that a local run would have caught is +wasted review time. diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 2b33ff5..8a64b59 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -5,7 +5,7 @@ on: schedule: - cron: "30 1 * * 6" push: - branches: ["main", "develop"] + branches: ["main"] permissions: read-all diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c8e1b84..fc4501c 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -52,11 +52,22 @@ when and why this was added. ## ADR: hosting platform Considered Vercel (rejected: serverless-only Python, no persistent -in-memory state, incompatible with this service's cache design). Chose -Render as primary (zero card on file, real cost is a slower cold start -after idle) over Cloud Run (technically better fit for the persistent- -container model, but requires a card on file even though usage should -stay within the free tier). See `README.md` deployment section for the -current live choice; check there before assuming this ADR reflects where -it's actually running today, ADRs record reasoning at decision time, they -don't self-update. +in-memory state, incompatible with this service's cache design). +Originally chose Render as primary (zero card on file, real cost was a +slower cold start after idle) over Cloud Run (technically better fit +for the persistent-container model, but requires a card on file even +though usage should stay within the free tier). + +Revisited for the first production deployment and switched to Cloud +Run: the card-on-file requirement was accepted deliberately, with a $1 +budget alert as the concrete mitigation (a service expected to cost $0 +means any alert firing at all is the signal, not a soft limit), in +exchange for the persistent-container model actually matching this +service's in-memory cache design instead of working around Render's +sleep-and-wake cycle. `--min-instances 0` keeps it free at idle the +same way Render's sleep did, just without the cold-start latency on +wake. + +See `README.md`'s deployment section for the current live choice; check +there before assuming this ADR reflects where it's actually running +today, ADRs record reasoning at decision time, they don't self-update. diff --git a/CHANGELOG.md b/CHANGELOG.md index fc2088f..e7ad9c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,31 @@ All notable changes to this project are documented here. Format loosely follows [Keep a Changelog](https://keepachangelog.com/). +## [Unreleased] + +### Changed + +- Hosting switched from the planned Render deployment to Google Cloud + Run for the first production deployment (`--min-instances 0`, + `--max-instances 3`, `us-central1`), accepting the card-on-file + tradeoff in exchange for a persistent-container model that matches + this service's in-memory cache design. A $1 budget alert is the + concrete mitigation. See `ARCHITECTURE.md`'s ADR section for the full + reasoning. + +### Fixed + +- `Dockerfile` only copied `main.py`, never `constants.py`, so the + built image crashed on startup with `ModuleNotFoundError` before + `uvicorn` ever bound to the port. Caught by the first real deploy + attempt; `python3 -m py_compile main.py` never catches this since it + runs against the full checked-out repo, not the trimmed set of files + the Dockerfile actually copies into the image. +- The fix for the above (`COPY main.py constants.py .`) built fine + locally under BuildKit but failed on Cloud Build's classic builder, + which enforces the Dockerfile spec strictly: a multi-source `COPY`'s + destination must end with `/`. + ## [1.0.0] - initial release ### Added diff --git a/Dockerfile b/Dockerfile index 551737f..b31c4ae 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,7 +2,7 @@ FROM python:3.14-slim WORKDIR /app COPY requirements.txt . RUN pip install --no-cache-dir -r requirements.txt -COPY main.py . +COPY main.py constants.py ./ ENV PORT=8080 EXPOSE 8080 CMD exec uvicorn main:app --host 0.0.0.0 --port ${PORT} diff --git a/README.md b/README.md index ea8ac70..b2b289e 100644 --- a/README.md +++ b/README.md @@ -45,9 +45,9 @@ minutes thereafter. This repo holds no copy of the record data itself; ## Deployment -Deployed on Render. See `ARCHITECTURE.md`'s ADR section for why Render was -chosen over Cloud Run and Vercel, and what would have to change to revisit -that. +Deployed on Google Cloud Run (`--min-instances 0`, scales to zero at +idle). See `ARCHITECTURE.md`'s ADR section for the full reasoning, +including why Render was the original choice and what changed. ## Contributing diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..60e163f --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,2 @@ +-r requirements.txt +pytest==8.3.3 diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..e5d752d --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,49 @@ +import time + +import pytest +from fastapi.testclient import TestClient + +import main + +FAKE_RECORDS = [ + { + "ave_id": "AVE-0001", + "title": "Prompt injection via tool output", + "description": "An agent trusts unsanitized tool output as instructions.", + "attack_class": "instruction-injection", + "behavioral_fingerprint": "unexpected-tool-call-sequence", + "mitigation": { + "summary": "Treat tool output as untrusted data, not instructions." + }, + }, + { + "ave_id": "AVE-0002", + "title": "Excessive agency", + "description": "An agent takes an irreversible action without confirmation.", + "attack_class": "privilege-escalation", + "behavioral_fingerprint": "unconfirmed destructive action", + "mitigation": None, + }, +] + + +@pytest.fixture(autouse=True) +def isolated_service_state(monkeypatch): + """Every test starts from known cache and rate-limit state, and no test + ever reaches the real network.""" + + async def fake_refresh_cache() -> None: + main._cache["records"] = FAKE_RECORDS + main._cache["fetched_at"] = time.time() + + monkeypatch.setattr(main, "_refresh_cache", fake_refresh_cache) + main._cache["records"] = [] + main._cache["fetched_at"] = 0.0 + main.limiter.reset() + yield + + +@pytest.fixture +def client(): + with TestClient(main.app) as test_client: + yield test_client diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..1c92ca9 --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,134 @@ +import time + +from fastapi.testclient import TestClient + +import main +from conftest import FAKE_RECORDS + + +def test_index_reports_service_metadata(client): + response = client.get("/") + + assert response.status_code == 200 + body = response.json() + assert body["name"] == "AVE Reference API" + assert body["record_count"] == len(FAKE_RECORDS) + assert "cache_age_seconds" in body + + +def test_list_records_returns_all_cached_records(client): + response = client.get("/records") + + assert response.status_code == 200 + assert len(response.json()) == len(FAKE_RECORDS) + + +def test_get_record_returns_matching_record(client): + response = client.get("/records/AVE-0001") + + assert response.status_code == 200 + assert response.json()["ave_id"] == "AVE-0001" + + +def test_get_record_404s_for_unknown_id(client): + response = client.get("/records/does-not-exist") + + assert response.status_code == 404 + assert "does-not-exist" in response.json()["detail"] + + +def test_get_mitigation_returns_only_the_mitigation_object(client): + response = client.get("/records/AVE-0001/mitigation") + + assert response.status_code == 200 + assert response.json() == { + "summary": "Treat tool output as untrusted data, not instructions." + } + + +def test_get_mitigation_404s_when_record_has_no_mitigation(client): + response = client.get("/records/AVE-0002/mitigation") + + assert response.status_code == 404 + + +def test_get_mitigation_404s_for_unknown_id(client): + response = client.get("/records/does-not-exist/mitigation") + + assert response.status_code == 404 + + +def test_search_matches_title_case_insensitively(client): + response = client.get("/search", params={"q": "PROMPT"}) + + assert response.status_code == 200 + assert [r["ave_id"] for r in response.json()] == ["AVE-0001"] + + +def test_search_matches_across_attack_class_and_behavioral_fingerprint(client): + response = client.get("/search", params={"q": "unconfirmed"}) + + assert [r["ave_id"] for r in response.json()] == ["AVE-0002"] + + +def test_search_returns_empty_list_for_no_match(client): + response = client.get("/search", params={"q": "nothing-matches-this"}) + + assert response.status_code == 200 + assert response.json() == [] + + +def test_every_route_is_rate_limited(client): + for _ in range(60): + response = client.get("/records") + assert response.status_code == 200 + + response = client.get("/records") + + assert response.status_code == 429 + + +def test_startup_does_not_crash_when_initial_fetch_fails(monkeypatch): + async def failing_refresh() -> None: + raise RuntimeError("simulated network failure") + + monkeypatch.setattr(main, "_refresh_cache", failing_refresh) + + with TestClient(main.app) as test_client: + response = test_client.get("/") + + assert response.status_code == 200 + assert response.json()["record_count"] == 0 + + +def test_refresh_failure_serves_stale_cache_instead_of_failing(client, monkeypatch): + # `client` already has a populated, fresh cache via the autouse fixture; + # push it past REFRESH_INTERVAL_SECONDS so the next request tries a + # refresh, then make that refresh fail. + main._cache["fetched_at"] = time.time() - main.REFRESH_INTERVAL_SECONDS - 1 + + async def failing_refresh() -> None: + raise RuntimeError("simulated transient network failure") + + monkeypatch.setattr(main, "_refresh_cache", failing_refresh) + + response = client.get("/records") + + assert response.status_code == 200 + assert len(response.json()) == len(FAKE_RECORDS) + + +def test_refresh_failure_is_a_real_failure_when_cache_was_never_populated( + monkeypatch, +): + async def failing_refresh() -> None: + raise RuntimeError("simulated network failure, never succeeded") + + monkeypatch.setattr(main, "_refresh_cache", failing_refresh) + main._cache["records"] = [] + main._cache["fetched_at"] = 0.0 + + with TestClient(main.app, raise_server_exceptions=False) as test_client: + response = test_client.get("/records") + + assert response.status_code == 500