feat: add read-only Postgres role for westside-streamlit (#5) #435

Merged

forgejo_admin merged 3 commits from 5-westside-streamlit-ro-role into main 2026-04-11 20:01:34 +00:00

Conversation 4 Commits 3 Files changed 2 +275

forgejo_admin commented 2026-04-10 22:25:02 +00:00

Owner

Copy link

Summary

Creates the westside_streamlit_ro Postgres role with SELECT-only access to the 6 tables the westside-streamlit dashboard reads. Defense-in-depth for the read-only invariant — even if app.py is modified to contain a write query, the DB role guarantees it cannot mutate state.

Cross-repo ticket: the issue is filed on forgejo_admin/westside-streamlit#5 (project tracking) but the migration lives here because basketball-api owns the Alembic schema.

Changes

• alembic/versions/040_add_westside_streamlit_ro_role.py — new migration (filename 040_ because 030_-039_ are already taken; 039 is the current head).
  • Creates role westside_streamlit_ro with LOGIN and a password read from the WESTSIDE_STREAMLIT_RO_PASSWORD env var (raises RuntimeError if missing).
  • Idempotent DO $$ ... CREATE/ALTER ROLE block so re-running is safe.
  • Grants USAGE on schema public and SELECT on players, parents, teams, player_teams, orders, products — explicit list, not ALL TABLES, so future tables are denied by default.
  • REVOKE CREATE ON SCHEMA public FROM PUBLIC — required for PG <=14 and for test environments that reset schema with GRANT ALL TO PUBLIC. No-op on correctly configured PG 15+.
  • downgrade() revokes all grants on tables, schema, sequences, then DROP ROLE IF EXISTS — verified clean.
• tests/test_westside_streamlit_ro_role.py — 13 integration tests.
  • 6 SELECT-success tests (parametrized per table).
  • 6 denial tests: INSERT, UPDATE, DELETE, TRUNCATE, CREATE TABLE, DROP TABLE — each asserts ProgrammingError with permission denied.
  • 1 sanity test that the migration file exists at the expected path.
  • Fixture mirrors the migration's grant/revoke sequence so the test is self-contained and does not require alembic upgrade to run against the test DB.

Test Plan

Ran locally against postgresql://basketball:basketball@localhost:5432/basketball_test on PG 18.3:

$ pytest tests/test_westside_streamlit_ro_role.py -v
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[players] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[parents] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[teams] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[player_teams] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[orders] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[products] PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_insert PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_update PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_delete PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_truncate PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_create_table PASSED
tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_drop_table PASSED
tests/test_westside_streamlit_ro_role.py::test_migration_file_exists PASSED
============================== 13 passed in 2.10s ==============================

End-to-end migration chain (alembic stamp 039 && alembic upgrade head) verified grants:

$ psql -c "SELECT grantee, privilege_type, table_name FROM information_schema.role_table_grants WHERE grantee='westside_streamlit_ro';"
 westside_streamlit_ro | SELECT | orders
 westside_streamlit_ro | SELECT | parents
 westside_streamlit_ro | SELECT | player_teams
 westside_streamlit_ro | SELECT | players
 westside_streamlit_ro | SELECT | products
 westside_streamlit_ro | SELECT | teams

Downgrade verified clean:

$ alembic downgrade -1
INFO  Running downgrade 040 -> 039
$ psql -c "SELECT 1 FROM pg_roles WHERE rolname='westside_streamlit_ro';"
 (0 rows)

ruff format and ruff check both clean on the two new files.

Secrets

A fresh 32-character password has been generated and stored at ~/secrets/pal-e-services/westside-streamlit.env (mode 0600) as WESTSIDE_STREAMLIT_RO_PASSWORD=.... The password is NOT in git, NOT in the migration file, and NOT in this PR body. To run the migration against any environment, source that file first:

source ~/secrets/pal-e-services/westside-streamlit.env
export WESTSIDE_STREAMLIT_RO_PASSWORD
alembic upgrade head

The follow-up (westside-streamlit#4 Kustomize overlay) will pick this password up via a SOPS-managed Secret.

Review Checklist

• Migration filename uses sequential prefix (040_), not a timestamp
• Password is read from env var, never hardcoded or committed
• Password stored in ~/secrets/pal-e-services/westside-streamlit.env on host
• Grants are an explicit table list (6 tables), not ALL TABLES — future tables denied by default
• All 6 denial cases tested (INSERT, UPDATE, DELETE, TRUNCATE, CREATE, DROP)
• SELECT success tested against all 6 granted tables
• downgrade() drops role cleanly with no leftover grants — verified with alembic downgrade -1
• Migration is idempotent (DO block checks for role existence)
• ruff format and ruff check clean
• No application code modified — schema-only change
• No existing migrations modified
• Reviewer: confirm WESTSIDE_STREAMLIT_RO_PASSWORD is wired into the basketball-api migration pipeline (Woodpecker secret) before deploy
• Reviewer: confirm the password SOPS secret is added for westside-streamlit#4 consumption

Related Notes

• story-westside-streamlit-reachable — the user story this ticket traces to
• arch-dataflow-westside-streamlit — documents the read-only invariant this migration enforces at the DB layer
• incident-2026-04-10-pal-e-streamlit-public-funnel — defense-in-depth rationale
• feedback_never_alter_prod_directly — why this goes through the Alembic pipeline, not a direct psql
• review-936-2026-04-10 — scope review that drove the final shape of this ticket

Related

• Closes forgejo_admin/westside-streamlit#5 (cross-repo; Forgejo won't auto-close across repos but this documents the linkage)
• Blocked downstream: westside-streamlit#4 (Kustomize overlay — consumes these credentials via SOPS Secret)
• Parallel: westside-streamlit#2 (Dockerfile — independent)
• Orthogonal: westside-streamlit#7 (Keycloak OIDC — auth-layer defense; this is DB-layer)

## Summary Creates the `westside_streamlit_ro` Postgres role with SELECT-only access to the 6 tables the westside-streamlit dashboard reads. Defense-in-depth for the read-only invariant — even if `app.py` is modified to contain a write query, the DB role guarantees it cannot mutate state. Cross-repo ticket: the issue is filed on `forgejo_admin/westside-streamlit#5` (project tracking) but the migration lives here because basketball-api owns the Alembic schema. ## Changes - `alembic/versions/040_add_westside_streamlit_ro_role.py` — new migration (filename `040_` because `030_`-`039_` are already taken; `039` is the current head). - Creates role `westside_streamlit_ro` with `LOGIN` and a password read from the `WESTSIDE_STREAMLIT_RO_PASSWORD` env var (raises RuntimeError if missing). - Idempotent `DO $$ ... CREATE/ALTER ROLE` block so re-running is safe. - Grants `USAGE` on schema `public` and `SELECT` on `players`, `parents`, `teams`, `player_teams`, `orders`, `products` — explicit list, not `ALL TABLES`, so future tables are denied by default. - `REVOKE CREATE ON SCHEMA public FROM PUBLIC` — required for PG <=14 and for test environments that reset schema with `GRANT ALL TO PUBLIC`. No-op on correctly configured PG 15+. - `downgrade()` revokes all grants on tables, schema, sequences, then `DROP ROLE IF EXISTS` — verified clean. - `tests/test_westside_streamlit_ro_role.py` — 13 integration tests. - 6 SELECT-success tests (parametrized per table). - 6 denial tests: INSERT, UPDATE, DELETE, TRUNCATE, CREATE TABLE, DROP TABLE — each asserts `ProgrammingError` with `permission denied`. - 1 sanity test that the migration file exists at the expected path. - Fixture mirrors the migration's grant/revoke sequence so the test is self-contained and does not require `alembic upgrade` to run against the test DB. ## Test Plan Ran locally against `postgresql://basketball:basketball@localhost:5432/basketball_test` on PG 18.3: ``` $ pytest tests/test_westside_streamlit_ro_role.py -v tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[players] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[parents] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[teams] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[player_teams] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[orders] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_can_select_from_granted_table[products] PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_insert PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_update PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_delete PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_truncate PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_create_table PASSED tests/test_westside_streamlit_ro_role.py::test_ro_role_cannot_drop_table PASSED tests/test_westside_streamlit_ro_role.py::test_migration_file_exists PASSED ============================== 13 passed in 2.10s ============================== ``` End-to-end migration chain (`alembic stamp 039 && alembic upgrade head`) verified grants: ``` $ psql -c "SELECT grantee, privilege_type, table_name FROM information_schema.role_table_grants WHERE grantee='westside_streamlit_ro';" westside_streamlit_ro | SELECT | orders westside_streamlit_ro | SELECT | parents westside_streamlit_ro | SELECT | player_teams westside_streamlit_ro | SELECT | players westside_streamlit_ro | SELECT | products westside_streamlit_ro | SELECT | teams ``` Downgrade verified clean: ``` $ alembic downgrade -1 INFO Running downgrade 040 -> 039 $ psql -c "SELECT 1 FROM pg_roles WHERE rolname='westside_streamlit_ro';" (0 rows) ``` `ruff format` and `ruff check` both clean on the two new files. ## Secrets A fresh 32-character password has been generated and stored at `~/secrets/pal-e-services/westside-streamlit.env` (mode 0600) as `WESTSIDE_STREAMLIT_RO_PASSWORD=...`. The password is NOT in git, NOT in the migration file, and NOT in this PR body. To run the migration against any environment, source that file first: ``` source ~/secrets/pal-e-services/westside-streamlit.env export WESTSIDE_STREAMLIT_RO_PASSWORD alembic upgrade head ``` The follow-up (westside-streamlit#4 Kustomize overlay) will pick this password up via a SOPS-managed Secret. ## Review Checklist - [x] Migration filename uses sequential prefix (`040_`), not a timestamp - [x] Password is read from env var, never hardcoded or committed - [x] Password stored in `~/secrets/pal-e-services/westside-streamlit.env` on host - [x] Grants are an explicit table list (6 tables), not `ALL TABLES` — future tables denied by default - [x] All 6 denial cases tested (INSERT, UPDATE, DELETE, TRUNCATE, CREATE, DROP) - [x] SELECT success tested against all 6 granted tables - [x] `downgrade()` drops role cleanly with no leftover grants — verified with `alembic downgrade -1` - [x] Migration is idempotent (DO block checks for role existence) - [x] `ruff format` and `ruff check` clean - [x] No application code modified — schema-only change - [x] No existing migrations modified - [ ] Reviewer: confirm `WESTSIDE_STREAMLIT_RO_PASSWORD` is wired into the basketball-api migration pipeline (Woodpecker secret) before deploy - [ ] Reviewer: confirm the password SOPS secret is added for westside-streamlit#4 consumption ## Related Notes - `story-westside-streamlit-reachable` — the user story this ticket traces to - `arch-dataflow-westside-streamlit` — documents the read-only invariant this migration enforces at the DB layer - `incident-2026-04-10-pal-e-streamlit-public-funnel` — defense-in-depth rationale - `feedback_never_alter_prod_directly` — why this goes through the Alembic pipeline, not a direct `psql` - `review-936-2026-04-10` — scope review that drove the final shape of this ticket ## Related - Closes forgejo_admin/westside-streamlit#5 (cross-repo; Forgejo won't auto-close across repos but this documents the linkage) - Blocked downstream: westside-streamlit#4 (Kustomize overlay — consumes these credentials via SOPS Secret) - Parallel: westside-streamlit#2 (Dockerfile — independent) - Orthogonal: westside-streamlit#7 (Keycloak OIDC — auth-layer defense; this is DB-layer)

forgejo_admin added 1 commit 2026-04-10 22:25:02 +00:00

feat: add read-only Postgres role for westside-streamlit (#5) ... c5bcf82440

Creates the westside_streamlit_ro role with SELECT on the 6 tables the
dashboard reads (players, parents, teams, player_teams, orders, products)
plus USAGE on schema public. Revokes CREATE from PUBLIC so the role
cannot create objects even via inheritance.

Password is read from WESTSIDE_STREAMLIT_RO_PASSWORD and lives only in
~/secrets/pal-e-services/westside-streamlit.env on the host - never in
git and never in the migration file.

Integration test asserts SELECT works against all 6 tables and that
INSERT, UPDATE, DELETE, TRUNCATE, CREATE TABLE, and DROP TABLE all fail
with a permission error.

Defense-in-depth for the read-only invariant documented in
arch-dataflow-westside-streamlit - even if app.py is modified to contain
a write query, the DB role guarantees it cannot mutate state.

Refs forgejo_admin/westside-streamlit#5

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-04-10 22:27:46 +00:00

Author

Owner

Copy link

PR #435 Review

Fresh-context review. Read-only agent — no code changes, no merge.

DOMAIN REVIEW

Stack: Python / Alembic / SQLAlchemy / Postgres — DB-layer defense-in-depth migration.

Migration correctness (alembic/versions/040_add_westside_streamlit_ro_role.py):

• Filename uses sequential 040_ prefix matching the convention in alembic/versions/ (001–030 sequential, same zero-pad style). Good.
• upgrade() reads password from WESTSIDE_STREAMLIT_RO_PASSWORD, raises RuntimeError with a useful message if unset. Good.
• SQL-literal escape password.replace("'", "''") is correct for Postgres single-quoted literals. CREATE/ALTER ROLE does not accept bind params, so inline interpolation is required here.
• Idempotent DO $$ ... END $$ block with CREATE/ALTER branch is safe to re-run.
• Grants: SELECT on exactly the 6 tables listed in the issue (players, parents, teams, player_teams, orders, products). Explicit list (not ALL TABLES) — future tables denied by default. Good.
• USAGE on schema public — required for reference resolution.
• downgrade() revokes grants on the 6 named tables, then defensively does REVOKE ALL PRIVILEGES ON ALL TABLES/SEQUENCES, then DROP ROLE IF EXISTS. Clean.
• Verified against the local models: all 6 tablenames exist (src/basketball_api/models.py lines 175, 196, 342, 385, 403; player_teams junction created in migration 019).

Test correctness (tests/test_westside_streamlit_ro_role.py):

• 13 tests as advertised: 6 parametrized SELECT-success + 6 denial (INSERT/UPDATE/DELETE/TRUNCATE/CREATE TABLE/DROP TABLE) + 1 migration-file-exists. Complete denial coverage.
• Denial helper asserts ProgrammingError with permission denied or must be owner in the message — robust against PG phrasing differences.
• ro_engine fixture builds a connection URL via make_url(settings.database_url).set(username=..., password=...) — clean.
• Fixture creates and drops the role itself, so the test does not depend on alembic upgrade head having run. Self-contained. Good.
• CI wired: .woodpecker.yaml has a postgres:16-alpine service with BASKETBALL_DATABASE_URL pointing at the test DB, and the default superuser (basketball) can CREATE ROLE. Tests will execute in CI without additional secret plumbing.

BLOCKERS

None.

NITS

1. Test fixture leaves REVOKE CREATE ON SCHEMA public FROM PUBLIC on teardown. The fixture mirrors the migration by revoking CREATE from PUBLIC inside setup, but the teardown only revokes role-specific grants and drops the role — it does not restore CREATE to PUBLIC. With conftest.setup_db running Base.metadata.create_all as the basketball owner role (which still has CREATE as the schema owner), the pollution is inert for current tests. But it is a cross-test side effect that could surprise future test authors who add a non-owner role. Consider adding GRANT CREATE ON SCHEMA public TO PUBLIC to the teardown, or leave a comment explaining why the asymmetry is intentional.

2. REVOKE CREATE ON SCHEMA public FROM PUBLIC in the migration is a global side effect. In prod, this revoke affects every role on the basketball DB, not just the new one. For the current basketball-api DB this is fine (the basketball owner retains CREATE via ownership, and there are no other service roles on this DB that I can see). But the PR comment should call this out explicitly so a future operator adding another DB role on this cluster is not surprised. Not a blocker — PG 15+ does this by default, and the PR body does explain the rationale. Consider tightening the comment in the migration itself to note "this is global to the basketball database."

3. test_ro_role_cannot_insert INSERT row references parent_id=1, tenant_id=1 which will not exist in an empty test DB. Postgres does permission checks before execution, so permission denied fires before the FK check — verified correct — but a reader might wonder. A one-line comment ("FK values are irrelevant — permission check fires first") would help future maintainers.

SOP COMPLIANCE

• Branch named after issue (5-westside-streamlit-ro-role) — cross-repo issue, matches westside-streamlit#5
• PR body has Summary / Changes / Test Plan / Related sections
• Review Checklist present, most items checked
• Related section references story, arch, incident, and review notes
• No secrets committed — password read from env var, test password is obviously-fake sentinel with # noqa: S105
• No existing migrations modified (diff shows 2 files added, 0 deletions, 275 additions)
• No application code modified — schema-only change
• Test Plan includes full pytest output + downgrade verification + ruff clean
• Cross-repo link documented with "Forgejo won't auto-close" caveat

PROCESS OBSERVATIONS

• Defense-in-depth posture is textbook: even if westside-streamlit/app.py drifts to contain a write query, the DB role guarantees immutability. This is the right layer.
• Two unchecked items in the PR Review Checklist are correctly flagged for the reviewer/operator and belong to downstream tickets (Woodpecker secret for the migration pipeline; SOPS secret for westside-streamlit#4). Track these as follow-up on the downstream tickets, not as blockers here.
• Scope discipline is tight: 2 files, 275 additions, 0 deletions, no existing file touched. Model for other migration PRs.
• Unverified (reviewer limitation): I could not independently verify that down_revision = "039" is correct. My local clone of ~/basketball-api is stale (head is 030). The PR body asserts 030–039 are taken and 039 is current head; the agent reports verifying this. Reviewer should confirm by running alembic history or checking git log --oneline alembic/versions/ on the branch base before merge. If 039 is not actually the head, the migration will fail with "Can't locate revision identified by '039'".

Specific concerns

• REVOKE CREATE on public schema — safe? Yes for this DB (basketball owner retains CREATE via ownership). Potentially affects any future non-owner roles on the same DB. Rationale is documented in the migration comment. Not a blocker.
• down_revision — correct? Unverified from my context; agent asserts yes. Reviewer should spot-check.
• Test coverage — sufficient for the 6 denial types? Yes — INSERT, UPDATE, DELETE, TRUNCATE, CREATE TABLE, DROP TABLE all covered, plus 6 parametrized SELECT successes and 1 file-exists sanity test.

VERDICT: APPROVED

3 nits, all non-blocking. Recommend addressing nit #1 (fixture teardown symmetry) before merge for hygiene, but not a gate. Reviewer must spot-check down_revision = "039" against the actual head of main before merge.

## PR #435 Review Fresh-context review. Read-only agent — no code changes, no merge. ### DOMAIN REVIEW **Stack:** Python / Alembic / SQLAlchemy / Postgres — DB-layer defense-in-depth migration. **Migration correctness (`alembic/versions/040_add_westside_streamlit_ro_role.py`):** - Filename uses sequential `040_` prefix matching the convention in `alembic/versions/` (001–030 sequential, same zero-pad style). Good. - `upgrade()` reads password from `WESTSIDE_STREAMLIT_RO_PASSWORD`, raises `RuntimeError` with a useful message if unset. Good. - SQL-literal escape `password.replace("'", "''")` is correct for Postgres single-quoted literals. CREATE/ALTER ROLE does not accept bind params, so inline interpolation is required here. - Idempotent `DO $$ ... END $$` block with CREATE/ALTER branch is safe to re-run. - Grants: `SELECT` on exactly the 6 tables listed in the issue (`players`, `parents`, `teams`, `player_teams`, `orders`, `products`). Explicit list (not `ALL TABLES`) — future tables denied by default. Good. - `USAGE` on schema public — required for reference resolution. - `downgrade()` revokes grants on the 6 named tables, then defensively does `REVOKE ALL PRIVILEGES ON ALL TABLES/SEQUENCES`, then `DROP ROLE IF EXISTS`. Clean. - Verified against the local models: all 6 tablenames exist (`src/basketball_api/models.py` lines 175, 196, 342, 385, 403; `player_teams` junction created in migration 019). **Test correctness (`tests/test_westside_streamlit_ro_role.py`):** - 13 tests as advertised: 6 parametrized SELECT-success + 6 denial (INSERT/UPDATE/DELETE/TRUNCATE/CREATE TABLE/DROP TABLE) + 1 migration-file-exists. Complete denial coverage. - Denial helper asserts `ProgrammingError` with `permission denied` or `must be owner` in the message — robust against PG phrasing differences. - `ro_engine` fixture builds a connection URL via `make_url(settings.database_url).set(username=..., password=...)` — clean. - Fixture creates and drops the role itself, so the test does not depend on `alembic upgrade head` having run. Self-contained. Good. - CI wired: `.woodpecker.yaml` has a `postgres:16-alpine` service with `BASKETBALL_DATABASE_URL` pointing at the test DB, and the default superuser (`basketball`) can `CREATE ROLE`. Tests will execute in CI without additional secret plumbing. ### BLOCKERS None. ### NITS 1. **Test fixture leaves `REVOKE CREATE ON SCHEMA public FROM PUBLIC` on teardown.** The fixture mirrors the migration by revoking CREATE from PUBLIC inside setup, but the teardown only revokes role-specific grants and drops the role — it does not restore CREATE to PUBLIC. With `conftest.setup_db` running `Base.metadata.create_all` as the `basketball` owner role (which still has CREATE as the schema owner), the pollution is inert for current tests. But it is a cross-test side effect that could surprise future test authors who add a non-owner role. Consider adding `GRANT CREATE ON SCHEMA public TO PUBLIC` to the teardown, or leave a comment explaining why the asymmetry is intentional. 2. **`REVOKE CREATE ON SCHEMA public FROM PUBLIC` in the migration is a global side effect.** In prod, this revoke affects *every* role on the basketball DB, not just the new one. For the current basketball-api DB this is fine (the `basketball` owner retains CREATE via ownership, and there are no other service roles on this DB that I can see). But the PR comment should call this out explicitly so a future operator adding another DB role on this cluster is not surprised. Not a blocker — PG 15+ does this by default, and the PR body does explain the rationale. Consider tightening the comment in the migration itself to note "this is global to the `basketball` database." 3. **`test_ro_role_cannot_insert` INSERT row references `parent_id=1, tenant_id=1`** which will not exist in an empty test DB. Postgres does permission checks before execution, so `permission denied` fires before the FK check — verified correct — but a reader might wonder. A one-line comment ("FK values are irrelevant — permission check fires first") would help future maintainers. ### SOP COMPLIANCE - [x] Branch named after issue (`5-westside-streamlit-ro-role`) — cross-repo issue, matches westside-streamlit#5 - [x] PR body has Summary / Changes / Test Plan / Related sections - [x] Review Checklist present, most items checked - [x] Related section references story, arch, incident, and review notes - [x] No secrets committed — password read from env var, test password is obviously-fake sentinel with `# noqa: S105` - [x] No existing migrations modified (diff shows 2 files added, 0 deletions, 275 additions) - [x] No application code modified — schema-only change - [x] Test Plan includes full pytest output + downgrade verification + `ruff` clean - [x] Cross-repo link documented with "Forgejo won't auto-close" caveat ### PROCESS OBSERVATIONS - Defense-in-depth posture is textbook: even if `westside-streamlit/app.py` drifts to contain a write query, the DB role guarantees immutability. This is the right layer. - Two unchecked items in the PR Review Checklist are correctly flagged for the reviewer/operator and belong to downstream tickets (Woodpecker secret for the migration pipeline; SOPS secret for westside-streamlit#4). Track these as follow-up on the downstream tickets, not as blockers here. - Scope discipline is tight: 2 files, 275 additions, 0 deletions, no existing file touched. Model for other migration PRs. - **Unverified (reviewer limitation):** I could not independently verify that `down_revision = "039"` is correct. My local clone of `~/basketball-api` is stale (head is 030). The PR body asserts 030–039 are taken and 039 is current head; the agent reports verifying this. **Reviewer should confirm** by running `alembic history` or checking `git log --oneline alembic/versions/` on the branch base before merge. If 039 is not actually the head, the migration will fail with "Can't locate revision identified by '039'". ### Specific concerns - **REVOKE CREATE on public schema — safe?** Yes for this DB (basketball owner retains CREATE via ownership). Potentially affects any future non-owner roles on the same DB. Rationale is documented in the migration comment. Not a blocker. - **down_revision — correct?** Unverified from my context; agent asserts yes. Reviewer should spot-check. - **Test coverage — sufficient for the 6 denial types?** Yes — INSERT, UPDATE, DELETE, TRUNCATE, CREATE TABLE, DROP TABLE all covered, plus 6 parametrized SELECT successes and 1 file-exists sanity test. ### VERDICT: APPROVED 3 nits, all non-blocking. Recommend addressing nit #1 (fixture teardown symmetry) before merge for hygiene, but not a gate. Reviewer must spot-check `down_revision = "039"` against the actual head of `main` before merge.

forgejo_admin commented 2026-04-10 22:29:01 +00:00

Author

Owner

Copy link

⚠️ Migration number collision discovered post-review

Post-review audit caught a blocker that the prior review missed (stale clone). This PR's branch was cut from an older main:

On origin/main right now: 040_create_16u_local_queens_team.py
On this PR branch: 040_add_westside_streamlit_ro_role.py (new), and 040_create_16u_local_queens_team.py is NOT present on the branch

Both migrations claim 040_ and down_revision = "039". Merging this PR as-is would create two Alembic heads, breaking alembic upgrade head.

Required fix:

1. Rebase the PR branch onto latest origin/main
2. Rename alembic/versions/040_add_westside_streamlit_ro_role.py → 041_add_westside_streamlit_ro_role.py
3. In the file, change revision = "040" → revision = "041"
4. Change down_revision = "039" → down_revision = "040" (so it chains after 040_create_16u_local_queens_team)
5. Re-run alembic upgrade head + the test suite to verify the chain is clean
6. Force-push to 5-westside-streamlit-ro-role

Once the fix is pushed, a fresh review agent will re-verify per pr-review-loop.

Not a fault of the prior review — the reviewer's clone was at head 030, and the dev agent's clone predated 040_create_16u_local_queens_team.py landing on main. Lesson for future: re-fetch from remote immediately before any migration-number decisions.

## ⚠️ Migration number collision discovered post-review Post-review audit caught a blocker that the prior review missed (stale clone). This PR's branch was cut from an older `main`: **On `origin/main` right now:** `040_create_16u_local_queens_team.py` **On this PR branch:** `040_add_westside_streamlit_ro_role.py` (new), and `040_create_16u_local_queens_team.py` is NOT present on the branch Both migrations claim `040_` and `down_revision = "039"`. Merging this PR as-is would create two Alembic heads, breaking `alembic upgrade head`. **Required fix:** 1. Rebase the PR branch onto latest `origin/main` 2. Rename `alembic/versions/040_add_westside_streamlit_ro_role.py` → `041_add_westside_streamlit_ro_role.py` 3. In the file, change `revision = "040"` → `revision = "041"` 4. Change `down_revision = "039"` → `down_revision = "040"` (so it chains after `040_create_16u_local_queens_team`) 5. Re-run `alembic upgrade head` + the test suite to verify the chain is clean 6. Force-push to `5-westside-streamlit-ro-role` Once the fix is pushed, a fresh review agent will re-verify per `pr-review-loop`. Not a fault of the prior review — the reviewer's clone was at head `030`, and the dev agent's clone predated `040_create_16u_local_queens_team.py` landing on main. Lesson for future: re-fetch from remote immediately before any migration-number decisions.

forgejo_admin force-pushed 5-westside-streamlit-ro-role from c5bcf82440 to ed8795a970 2026-04-10 22:31:02 +00:00 Compare

forgejo_admin commented 2026-04-10 22:31:13 +00:00

Author

Owner

Copy link

Fix round 1

Rebased onto latest main. Renumbered 040_add_westside_streamlit_ro_role.py -> 041_add_westside_streamlit_ro_role.py to chain after 040_create_16u_local_queens_team. Updated revision identifiers:

• revision = "040" -> revision = "041"
• down_revision = "039" -> down_revision = "040"

alembic heads returns single head:

041 (head)

alembic history shows clean linear chain:

040 -> 041 (head), Add read-only Postgres role for westside-streamlit.
039 -> 040, Create 16U Local Queens team.
038 -> 039, Add recovery_email_sent column ...

Tests not run locally — basketball_api module not importable from /tmp clone (no editable install). CI will verify.

No changes to upgrade/downgrade logic, grants, or password env var handling. Force-pushed with --force-with-lease.

Ready for re-review.

## Fix round 1 Rebased onto latest main. Renumbered `040_add_westside_streamlit_ro_role.py` -> `041_add_westside_streamlit_ro_role.py` to chain after `040_create_16u_local_queens_team`. Updated revision identifiers: - `revision = "040"` -> `revision = "041"` - `down_revision = "039"` -> `down_revision = "040"` `alembic heads` returns single head: ``` 041 (head) ``` `alembic history` shows clean linear chain: ``` 040 -> 041 (head), Add read-only Postgres role for westside-streamlit. 039 -> 040, Create 16U Local Queens team. 038 -> 039, Add recovery_email_sent column ... ``` Tests not run locally — `basketball_api` module not importable from /tmp clone (no editable install). CI will verify. No changes to upgrade/downgrade logic, grants, or password env var handling. Force-pushed with `--force-with-lease`. Ready for re-review.

forgejo_admin added 1 commit 2026-04-11 19:49:18 +00:00

fix: sweep stale 040_* references missed by first renumber ... a507de2f06

Round-2 review caught a blocker (test_migration_file_exists hardcoded the
old filename) plus 3 docstring nits still referencing migration 040.
Grep sweep confirms zero stale references to 040_add_westside remain.

- tests/test_westside_streamlit_ro_role.py:175 literal filename fix (BLOCKER)
- alembic/versions/041_*.py module docstring Revision ID/Revises fix
- tests/test_westside_streamlit_ro_role.py:3 module docstring fix
- tests/test_westside_streamlit_ro_role.py:51 fixture docstring fix

Lesson: feedback_verify_repo_layout_before_ticketing applies to fix
rounds too — after any rename, grep the tree, not just the renamed file.

forgejo_admin commented 2026-04-11 19:49:30 +00:00

Author

Owner

Copy link

Round-2 fix pushed as a507de2. Swept the 4 stale 040_* references missed by the first renumber:

1. BLOCKER tests/test_westside_streamlit_ro_role.py:175 — test_migration_file_exists hardcoded filename "040_add_westside_streamlit_ro_role.py" -> "041_add_westside_streamlit_ro_role.py"
2. alembic/versions/041_add_westside_streamlit_ro_role.py:3-4 — module docstring Revision ID: 040 -> 041, Revises: 039 -> 040 (the revision/down_revision code lines were already correct from round 1)
3. tests/test_westside_streamlit_ro_role.py:3 — module docstring migration 040 -> migration 041
4. tests/test_westside_streamlit_ro_role.py:51 — ro_role fixture docstring migration 040 -> migration 041

Post-fix grep sweep is clean:

$ grep -rn "040_add_westside\|Revises: 039\|Revision ID: 040\|migration 040" .
# only hit: alembic/versions/040_create_16u_local_queens_team.py (its own identity, unrelated)
$ grep -n "041_add_westside" tests/test_westside_streamlit_ro_role.py
175:        here, "alembic", "versions", "041_add_westside_streamlit_ro_role.py"

Zero stale 040_add_westside references remain. No upgrade/downgrade logic, grants list, or env var handling touched — only the 4 strings called out in the review. Ready for round-3 re-review.

Round-2 fix pushed as `a507de2`. Swept the 4 stale `040_*` references missed by the first renumber: 1. **BLOCKER** `tests/test_westside_streamlit_ro_role.py:175` — `test_migration_file_exists` hardcoded filename `"040_add_westside_streamlit_ro_role.py"` -> `"041_add_westside_streamlit_ro_role.py"` 2. `alembic/versions/041_add_westside_streamlit_ro_role.py:3-4` — module docstring `Revision ID: 040` -> `041`, `Revises: 039` -> `040` (the `revision`/`down_revision` code lines were already correct from round 1) 3. `tests/test_westside_streamlit_ro_role.py:3` — module docstring `migration 040` -> `migration 041` 4. `tests/test_westside_streamlit_ro_role.py:51` — `ro_role` fixture docstring `migration 040` -> `migration 041` Post-fix grep sweep is clean: ``` $ grep -rn "040_add_westside\|Revises: 039\|Revision ID: 040\|migration 040" . # only hit: alembic/versions/040_create_16u_local_queens_team.py (its own identity, unrelated) $ grep -n "041_add_westside" tests/test_westside_streamlit_ro_role.py 175: here, "alembic", "versions", "041_add_westside_streamlit_ro_role.py" ``` Zero stale `040_add_westside` references remain. No upgrade/downgrade logic, grants list, or env var handling touched — only the 4 strings called out in the review. Ready for round-3 re-review.

forgejo_admin merged commit a20a8275a1 into main 2026-04-11 20:01:34 +00:00

forgejo_admin deleted branch 5-westside-streamlit-ro-role 2026-04-11 20:01:34 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-11 20:01:35 +00:00

feat: add read-only Postgres role for westside-streamlit (#5) (#435)

forgejo_admin referenced this pull request 2026-04-11 20:08:25 +00:00

[CRITICAL] Migration 041 dual-revision collision — root cause of 20h CrashLoopBackOff #443

forgejo_admin referenced this pull request 2026-04-11 20:21:09 +00:00

[CRITICAL] Migration 044 streamlit_ro_role requires WESTSIDE_STREAMLIT_RO_PASSWORD env var that's not in pod env — basketball-api still CrashLoopBackOff #449

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/basketball-api!435

No description provided.