pal-e-docs

Review: Provision infrastructure in pal-e-services (re-review)

review-593-2026-03-28-v2 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review of board item #593 after refinement. Previous review: review-593-2026-03-28 (5 findings, all addressed). This re-review found 2 new issues.

Previous Findings Resolution

  • [x] Finding 1: Add file target terraform/variables.tf — ADDRESSED. Now listed in File Targets with correct scope (extend keycloak_clients type).
  • [x] Finding 2: Add file target terraform/keycloak.tf — ADDRESSED. Now listed with correct scope (service_accounts_enabled + new role resource).
  • [x] Finding 3: Fix AC1 missing forgejo_repo and image_repo — ADDRESSED. Both fields now in AC1.
  • [x] Finding 4: Add dependency note to Constraints — ADDRESSED. Lineage section now documents #2 merge ordering.
  • [x] Finding 5: Clarify "should NOT touch" re: variables.tf/keycloak.tf — ADDRESSED. Those files removed from should-not-touch, correctly listed in should-modify.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone, with merge-order dependency on #2 documented
  • [x] Repo — forgejo_admin/pal-e-services
  • [x] User Story — Marcus as admin, infra provisioning
  • [x] Context — includes explanation that keycloak variable type + resource need extension
  • [x] File Targets — 3 should-modify, 3 should-not-touch, all correct
  • [x] Acceptance Criteria — 6 criteria listed
  • [x] Test Expectations — tofu validate + plan command with -lock=false
  • [x] Constraints — includes backward-compatibility requirement and no-apply guard
  • [x] Checklist — present
  • [x] Related — present

Traceability

  • [x] story:read-ops — Admin Read Operations via GroupMe
  • [x] story:write-ops — Admin Write Operations via GroupMe
  • [x] arch:A4 — Deployment architecture component
  • [x] Forgejo issue — forgejo_admin/westside-ai-assistant#1, open

File Targets

  • [x] terraform/k3s.tfvars — verified: file exists, contains services map (line 104) and keycloak_clients map (line 40). basketball-api reference pattern confirmed. All existing FastAPI services (port 8000) use /api suffix in image_repo.
  • [x] terraform/variables.tf — verified: file exists. keycloak_clients type at line 102-125 has no service_accounts_enabled or service_account_realm_roles. Extension is needed as described. Note: valid_redirect_uris = list(string) at line 112 is REQUIRED (no default).
  • [x] terraform/keycloak.tf — verified: file exists. keycloak_openid_client resource at line 91-118 does not set service_accounts_enabled. No keycloak_openid_client_service_account_realm_role resource exists. Keycloak provider is mrparkers/keycloak, which does support this resource.

Repo Placement

Correct. Issue filed on westside-ai-assistant, work targets forgejo_admin/pal-e-services. Single-repo change.

Dependencies

  • [x] Issue #2 (kustomize overlay) — board item #594, backlog. Merge-order dependency documented in Lineage: "#2 must also merge before ArgoCD can sync, but the two PRs are to different repos and can be developed in parallel."
  • [x] Issue #3 (NetworkPolicy) — board item #595, backlog. Post-deploy dependency, not blocking.

Acceptance Criteria

  • [ ] AC1: services entry — image_repo naming mismatch. Issue says image_repo: "westside-ai-assistant/app" but the established pattern for FastAPI services (port 8000) is /api: basketball-api/api, pal-e-docs/api, mcd-tracker/api, pal-e-mail/api. SvelteKit frontends (port 80) use /app. This is a FastAPI service, so it should be "westside-ai-assistant/api".
  • [ ] AC2: keycloak_clients entry — missing valid_redirect_uris. The keycloak_clients variable type requires valid_redirect_uris = list(string) with no default. A service-account-only client doesn't use browser redirects, but the agent must provide a value. The AC should specify valid_redirect_uris: [] (or make the variable optional). Without this, the agent will either guess or hit a validation error.
  • [x] AC3: variables.tf type extended — clear, specifies exact field names and defaults.
  • [x] AC4: keycloak.tf resource includes service_accounts_enabled — clear.
  • [x] AC5: new service_account_realm_role resource — clear, conditionally applied.
  • [x] AC6: tofu plan shows expected resources — testable.

Blast Radius

Low for the services entry — existing for_each handles new entries. Medium for keycloak changes — extending the variable type and resource affects all existing clients. The issue's Constraints section correctly requires backward-compatibility via optional() with safe defaults. Verified: no existing client uses service_accounts_enabled, so new optional fields with false/[] defaults are safe. Rollback: revert the tfvars entry.

Decomposition Assessment

3 files in 1 repo, 6 AC. The 6 AC technically exceeds the >5 threshold, but these are tightly coupled changes to a single variable type + resource + tfvars entry. Splitting would create artificial dependencies (can't extend variable type without extending resource and providing values). Estimated agent time: <5 minutes. No decomposition needed.

Recommendation

  • [BODY] Fix AC1: change image_repo: "westside-ai-assistant/app" to image_repo: "westside-ai-assistant/api" — follows established convention that FastAPI services (port 8000) use /api suffix.
  • [BODY] Fix AC2: add valid_redirect_uris: [] to the westside-ai-bot keycloak_clients entry, OR add a note that valid_redirect_uris should be made optional(list(string), []) in the variable type extension. The agent needs an explicit directive since this is a required field.