[CI] The feedback site's tests never run in CI #675

New issue

Closed

opened 2026-09-16 17:01:11 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-09-16 17:01:11 +00:00

Collaborator

Found by shipping a bug CI could not have caught: #673 added tools/feedback-site/static/prototype/lane2.html and a link to it, but not the route in app.py that serves it. /prototype/lane2 was a 404. There is a test that would have failed — the parametrised route test in tools/feedback-site/tests/test_app.py — but nothing runs it.

What is missing

.forgejo/workflows/ci.yml has jobs for the frontend, the backend (migrations, tests, audit), the bot, the eval harnesses, backend lint, version sync and the Docker image build. There is no job for tools/feedback-site.

Its README documents a manual command instead:

cd tools/feedback-site
docker run --rm -v "$PWD:/app" -w /app python:3.12-slim \
  sh -c "pip install -q fastapi 'uvicorn[standard]' pydantic pytest httpx && python -m pytest -q"

That is 80 tests covering the share-key gating, the round-one/round-two write paths, path traversal on /prototype/shared/{name}, and every prototype route — i.e. exactly the behaviour that breaks quietly. A test nobody runs is documentation.

Why it matters more now than it did

The feedback site started as a throwaway for the v4.4.0 review. It is now where the entire redesign is reviewed (#659) — one page per lane, eight lanes planned, each needing a route, and the pages are how the owner makes decisions that the rebuild then follows. A 404 on a review page stops the review.

It also serves /prototype/shared/{name}, which is the one piece of this service with real security logic (a name regex plus a resolved-path containment check). That has tests. They do not run either.

Suggested fix

A small job in ci.yml, in the same shape as the others:

  feedback-site:
    name: Feedback site tests
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: "3.12"
      - run: pip install fastapi "uvicorn[standard]" pydantic pytest httpx
      - run: python -m pytest -q
        working-directory: tools/feedback-site

Two things to decide while doing it:

Pin the dependencies. tools/feedback-site/requirements.txt exists; the README's command ignores it and installs unpinned. The job should use the file, and the README should be changed to match, so local and CI runs are the same thing. (I ran the suite in a scratch venv on Python 3.14 with unpinned deps to verify #674, because Docker Desktop was down — it passed, but that is not the documented environment and the gap is the point.)
Should it gate on paths? The rest of the matrix runs on everything. Keeping this one unconditional is simpler and the suite takes about a second.

Also worth considering

A test that asserts every static/prototype/*.html has a route, rather than a hand-maintained list — the failure mode here was "file added, route forgotten", and the list in the test is the same list someone forgets to update.

Found by shipping a bug CI could not have caught: #673 added `tools/feedback-site/static/prototype/lane2.html` and a link to it, but not the route in `app.py` that serves it. `/prototype/lane2` was a 404. There **is** a test that would have failed — the parametrised route test in `tools/feedback-site/tests/test_app.py` — but nothing runs it. ## What is missing `.forgejo/workflows/ci.yml` has jobs for the frontend, the backend (migrations, tests, audit), the bot, the eval harnesses, backend lint, version sync and the Docker image build. There is no job for `tools/feedback-site`. Its README documents a manual command instead: ```sh cd tools/feedback-site docker run --rm -v "$PWD:/app" -w /app python:3.12-slim \ sh -c "pip install -q fastapi 'uvicorn[standard]' pydantic pytest httpx && python -m pytest -q" ``` That is 80 tests covering the share-key gating, the round-one/round-two write paths, path traversal on `/prototype/shared/{name}`, and every prototype route — i.e. exactly the behaviour that breaks quietly. A test nobody runs is documentation. ## Why it matters more now than it did The feedback site started as a throwaway for the v4.4.0 review. It is now where the entire redesign is reviewed (#659) — one page per lane, eight lanes planned, each needing a route, and the pages are how the owner makes decisions that the rebuild then follows. A 404 on a review page stops the review. It also serves `/prototype/shared/{name}`, which is the one piece of this service with real security logic (a name regex plus a resolved-path containment check). That has tests. They do not run either. ## Suggested fix A small job in `ci.yml`, in the same shape as the others: ```yaml feedback-site: name: Feedback site tests runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: python-version: "3.12" - run: pip install fastapi "uvicorn[standard]" pydantic pytest httpx - run: python -m pytest -q working-directory: tools/feedback-site ``` Two things to decide while doing it: - **Pin the dependencies.** `tools/feedback-site/requirements.txt` exists; the README's command ignores it and installs unpinned. The job should use the file, and the README should be changed to match, so local and CI runs are the same thing. (I ran the suite in a scratch venv on Python 3.14 with unpinned deps to verify #674, because Docker Desktop was down — it passed, but that is not the documented environment and the gap is the point.) - **Should it gate on paths?** The rest of the matrix runs on everything. Keeping this one unconditional is simpler and the suite takes about a second. ## Also worth considering A test that asserts every `static/prototype/*.html` has a route, rather than a hand-maintained list — the failure mode here was "file added, route forgotten", and the list in the test is the same list someone forgets to update.