pal-e-docs

Review: Create read-only Postgres role for westside-streamlit

review-936-2026-04-10 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item #936 on board-westside-streamlit. Forgejo issue: forgejo_admin/westside-streamlit#5 (state: open). Issue body declares target repo as forgejo_admin/basketball-api — cross-repo pattern: tracked on the consumer board, code lands in the schema owner.

Template Completeness

Type: Feature. All required sections present per template-issue-feature:
  • [x] Type
  • [x] Lineage
  • [x] Repo (explicit: forgejo_admin/basketball-api)
  • [x] User Story
  • [x] Context (cites arch-dataflow-westside-streamlit and feedback_never_alter_prod_directly)
  • [x] File Targets (creations + explicit "do not touch")
  • [x] Acceptance Criteria (6 items, positive + negative)
  • [x] Test Expectations (unit + manual, run command)
  • [x] Constraints (role name, grants, denials, password handling, reversibility)
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:reachable label — verified: story-westside-streamlit-reachable exists and is listed in project-westside-streamlit user-stories section (Platform + Head Coach role).
  • [x] arch:postgres label — present on board item.
  • [ ] arch note MISSING — no arch-postgres note found in pal-e-docs via search_notes. The issue also references arch-dataflow-westside-streamlit and arch-deployment-westside-streamlit; neither exist yet (project page links them as planned architecture artifacts). [SCOPE] Create architecture note arch-postgres (or scope down to arch-deployment-westside-streamlit which would cover the DB connection).
  • [x] Forgejo issue — westside-streamlit#5, open, correctly filed.

File Targets

  • [x] alembic/versions/<timestamp>_add_westside_streamlit_ro_role.py — to be created. Verified ~/basketball-api/alembic/versions/ directory exists with 001–029 migrations. Naming convention in the repo uses numeric prefixes (e.g. 029_add_schedule_tables.py), not timestamps. [BODY] Recommend clarifying filename convention: next sequential number (likely 030_add_westside_streamlit_ro_role.py) rather than timestamp, to match repo convention.
  • [x] Tests directory ~/basketball-api/tests/ exists. New file test_westside_streamlit_ro_role.py is a clean add.
  • [x] "Files NOT to touch" section correctly excludes retroactive migration edits and app code.

Repo Placement

Board lives on board-westside-streamlit (consumer), code change lands in basketball-api (schema owner). Issue body is explicit about this via the ### Repo header. Acceptable cross-repo pattern — the dashboard is the consumer, but the role and grants belong to the database the schema owner manages. Single PR on basketball-api closes this.

Dependencies

Independent. Foundation work — no blocking tickets. The only blast-dependent ticket on the board (#939, arch:streamlit-app) will eventually consume WESTSIDE_STREAMLIT_RO_PASSWORD, but that consumption is not required for this migration to land. No items in in_progress on board-westside-streamlit.

Tables verified against app.py queries

Grepped ~/westside-streamlit/app.py. Real tables referenced: players, parents, teams, player_teams, orders, products. The string jersey_orders appears at line 123 but is a WITH jersey_orders AS (...) CTE, not a real table — it reads from orders + products internally. The 6-table list in Constraints is exact and complete.
Table existence confirmed in migrations: parents/players (001), teams (008), player_teams (019), orders/products (013).

Acceptance Criteria

6 criteria, all machine-verifiable:
  • Role creation with LOGIN — assertable via pg_roles.
  • All app.py queries succeed under new role — testable by capturing queries and replaying.
  • INSERT / UPDATE / CREATE TABLE all fail — negative assertions with pytest.raises.
  • Password stored in ~/secrets/pal-e-services/westside-streamlit.env — verified absent on host, clean add.
One implicit AC gap: no criterion for DELETE / TRUNCATE denial, even though Constraints list them. [BODY] Recommend adding one AC line: "When I connect as westside_streamlit_ro and run DELETE FROM players, then the query fails." Symmetry with the INSERT/UPDATE cases.

Blast Radius

Sibling pattern exists: westside-ops project uses the same "read-only Postgres role" architectural invariant (see story-westside-ops-spreadsheet-access, arch-deployment-westside-ops). This migration establishes the pattern; westside-ops should follow with its own westside_ops_ro role migration later. Not blocking — the pattern is additive per-consumer.
No downstream runtime consumers yet — the role is provisioned before any deployed service uses it. Password rotation story is out of scope (appropriate).

Decomposition

5-minute rule check:
  • File targets: 2 (1 migration + 1 test) — within limit.
  • Repos touched: 1 (basketball-api) — within limit.
  • AC count: 6 (plus 1 recommended DELETE addition = 7) — at limit but all same domain (Postgres grants).
  • Estimated agent work: <5 min — schema-only change, template-driven migration.
No decomposition needed.

Recommendation

  • [SCOPE] Create architecture note for Postgres or westside-streamlit deployment — either arch-postgres or arch-deployment-westside-streamlit. Current issue cites an arch doc that doesn't exist in pal-e-docs. Per prior precedent (review-836-r2) this is flagged but does not block dev dispatch if Ava accepts the gap.
  • [BODY] Clarify migration filename convention in File Targets: next sequential numeric prefix (e.g. 030_add_westside_streamlit_ro_role.py), not a timestamp — to match the existing 001_…029_ numbering in ~/basketball-api/alembic/versions/.
  • [BODY] Add a DELETE denial acceptance criterion for symmetry with INSERT/UPDATE/CREATE TABLE.
No [LABEL] or [DECOMPOSE] fixes needed. Scope is solid in substance; the gaps are cosmetic + a missing backing architecture note.