Requesting changes based on a few implementation gaps that affect the delivered behavior for workstreams C and E.
Approving on re-review. The SPA deep-link cache-header bug is fixed, the missing regression coverage is now present, and the verification pass on preview and the focused test suite came back clean.
Re-review complete. The prior blocker is fixed: preview now returns Cache-Control: no-cache, must-revalidate for SPA deep links like /people, and the PR now adds a deploy smoke assertion for that path plus hashed assets.
This matcher only covers requests whose original path is / or /index.html. For SPA deep links, try_files rewrites /people or /people/:id to index.html, but the @htmlShell matcher never fires, so those hard-refresh responses go out without Cache-Control: no-cache, must-revalidate. I reproduced that on preview with curl -I https://archive-preview.lintel.live/people and locally on http://127.0.0.1:18632/people.
This only checks that the Caddyfile text contains the intended directives. It doesn't verify the actual behavior for rewritten SPA routes, which is exactly why the missing Cache-Control on /people and /people/:id goes unnoticed. We need a runtime assertion for deep-link HTML responses here or in the deploy/browser checks before this issue can really be considered covered.
Requesting changes for the two blockers noted in the draft review: the HTML cache policy does not apply to SPA deep links, and the new regression coverage does not exercise that path.
This matcher only covers requests whose original path is / or /index.html. For SPA deep links, try_files rewrites /people or /people/:id to index.html, but the @htmlShell matcher never fires, so those hard-refresh responses go out without Cache-Control: no-cache, must-revalidate. I reproduced that on preview with curl -I https://archive-preview.lintel.live/people and locally on http://127.0.0.1:18632/people.
This only checks that the Caddyfile text contains the intended directives. It doesn't verify the actual behavior for rewritten SPA routes, which is exactly why the missing Cache-Control on /people and /people/:id goes unnoticed. We need a runtime assertion for deep-link HTML responses here or in the deploy/browser checks before this issue can really be considered covered.
Approved on re-review. The prior blocking issue is fixed, and the updated PR passed ruff, mypy, and the full backend unit/integration test suite locally.
Re-review complete. The previously flagged tag-ordering inconsistency between bootstrap and scripts/seed.py has been fixed: the seed path now preserves the shared sort_order from backend/app/data/reference_data.py instead of recomputing it. I also reran the stated verification locally (ruff, mypy, and pytest tests/unit tests/integration), and everything passed on this head commit.
reference_data.TAGS now carries an explicit sort_order, but this projection drops it and later reassigns order via enumerate(all_tags). That makes the seed path interpret the shared data differently from backend/app/services/bootstrap.py, which uses the provided sort_order directly. If someone updates tag ordering in reference_data.py, bootstraped users and seeded users can end up with different defaults. Please keep sort_order in the seed representation and write that value through to the tag rows instead of recomputing it here.
Requesting changes for one blocking consistency issue: the seed script recomputes tag sort order instead of honoring the explicit sort_order now defined in the shared reference-data module, so the two consumers can drift from the intended source of truth.
reference_data.TAGS now carries an explicit sort_order, but this projection drops it and later reassigns order via enumerate(all_tags). That makes the seed path interpret the shared data differently from backend/app/services/bootstrap.py, which uses the provided sort_order directly. If someone updates tag ordering in reference_data.py, bootstraped users and seeded users can end up with different defaults. Please keep sort_order in the seed representation and write that value through to the tag rows instead of recomputing it here.
The refactor is headed in the right direction and I verified the stated test matrix locally, but there’s one consistency issue I don’t think we should merge with: scripts/seed.py now centralizes tag metadata via backend/app/data/reference_data.py, yet it drops the explicit sort_order from that shared source and recomputes ordering by enumeration. That means bootstraped users and seeded users can diverge as soon as someone edits tag ordering in the new source-of-truth module. Because the whole point of this PR is to make these defaults safely editable, I think the seed path needs to honor the shared sort_order values too.
