pal-e-docs

Re-Review: Interaction logging to Postgres

review-684-2026-03-28-v2 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review of board item #684. Previous review (review-684-2026-03-28) returned NEEDS_REFINEMENT with 4 recommendations. None have been addressed -- the Forgejo issue body is unchanged and no comments exist.

Previous Recommendations -- Status

  • [SCOPE] Create infra prerequisite ticket (DB creation + NetworkPolicy + k8s secret) -- NOT ADDRESSED. No new issue exists in westside-ai-assistant, pal-e-services, or pal-e-platform for this work. This remains the primary blocker.
  • [BODY] Note that migrations/ directory must be created -- NOT ADDRESSED. Issue still says "create migrations/001_interaction_log.sql" without noting the directory is new.
  • [BODY] Clarify token counting across tool_use loop -- NOT ADDRESSED. Issue still says "Token counts come from the Anthropic API response" without specifying summing across multiple calls in the tool loop.
  • [BODY] Add explicit blocked-by dependency reference -- NOT ADDRESSED. No blocker reference in Constraints.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Standalone
  • [x] Repo -- forgejo_admin/westside-ai-assistant
  • [x] User Story -- well-formed (As Lucas / I want logging / So that I can analyze)
  • [x] Context -- thorough, includes CNPG cluster decision and NetworkPolicy mention
  • [x] File Targets -- 2 new files, 3 modified files, 3 exclusions
  • [x] Acceptance Criteria -- 5 items
  • [x] Test Expectations -- 3 unit tests + run command
  • [x] Constraints -- asyncpg, fire-and-forget, CNPG, NetworkPolicy
  • [x] Checklist -- standard 3-item
  • [x] Related -- project-westside-ai-assistant referenced

Traceability

  • [x] story:read-ops label -- "Admin Read Operations via GroupMe" (key: read-ops in project-westside-ai-assistant user-stories table)
  • [x] story note verified -- found in project-westside-ai-assistant user-stories section
  • [x] arch:A2 label -- AI Engine component
  • [x] arch note verified -- A2 (AI Engine) documented in arch-domain-westside-ai-assistant and arch-dataflow-westside-ai-assistant notes
  • [x] Forgejo issue -- forgejo_admin/westside-ai-assistant#19, state: open

File Targets

  • [x] app/ai.py -- verified: exists, contains process_message(). Anthropic SDK response has usage.input_tokens / usage.output_tokens.
  • [x] app/config.py -- verified: exists, Settings(BaseSettings) class. Adding database_url is straightforward.
  • [x] requirements.txt -- verified: exists with fastapi, uvicorn, httpx, anthropic, pydantic-settings, pytest. No asyncpg present.
  • [x] app/logging_db.py -- new file. Parent app/ exists.
  • [ ] migrations/001_interaction_log.sql -- ISSUE: migrations/ directory does not exist. Ticket should note directory creation needed.
  • [x] Exclusions: app/basketball.py, app/groupme.py, app/confirmation.py -- all verified to exist.

Repo Placement

App code is correctly scoped to forgejo_admin/westside-ai-assistant. However, the Constraints section references infra work belonging to other repos (CNPG DB creation, NetworkPolicy, k8s secret) with no corresponding issues filed.

Dependencies

  • [ ] Infra: Postgres database creation -- new DB needed in CNPG pal-e-postgres cluster. No ticket exists. UNRESOLVED.
  • [ ] Infra: NetworkPolicy -- westside-ai-assistant namespace needs egress to postgres namespace. Issue #3 (closed) covered basketball-api access only. No ticket for postgres access. UNRESOLVED.
  • [ ] Infra: K8s secret -- DATABASE_URL must be wired as k8s secret in pal-e-deployments kustomize overlay. No ticket exists. UNRESOLVED.
  • [x] Board item #683 (Repo restructure) -- also in backlog, also touches app/ai.py. Ordering conflict if worked in parallel. Documented awareness sufficient.
  • [x] Board item #685 (Read-only lockdown) -- also in backlog, also touches app/ai.py. Same ordering concern. Documented awareness sufficient.

Acceptance Criteria

All 5 AC items are verifiable by an agent via unit tests and file inspection. No missing criteria. AC5 (query logs SQL) is a demonstration query rather than a test command, which is acceptable.

Blast Radius

  • app/ai.py is the core engine. Changes are minimal (single await log_interaction() wrapped in try/except). Fire-and-forget constraint is correct.
  • No sibling services share this pattern -- only GroupMe bot in the platform.
  • Token counting across multiple Anthropic calls in the tool_use loop needs clarification in the ticket (current code makes multiple messages.create() calls during tool loops; total tokens should be summed).

Decomposition Assessment

5 file targets (2 new, 3 modified) in 1 repo. 5 AC. Estimated agent time: ~3-4 minutes. Passes the three-thing limit (3 discrete changes: new logging module, migration SQL, wire into ai.py). Passes the five-minute rule. No independent subtasks that need parallelization.
The infra work (DB creation + NetworkPolicy + k8s secret) is a separate concern that MUST be a separate ticket. The app ticket itself does not need decomposition.

Recommendation

All 4 recommendations from the prior review remain open:
  • [SCOPE] Create a separate Forgejo issue for infra prerequisites: (1) create interaction_log database in CNPG pal-e-postgres cluster, (2) NetworkPolicy update for westside-ai-assistant to reach postgres namespace, (3) k8s secret with DATABASE_URL wired in pal-e-deployments kustomize overlay. This ticket blocks #19. File on appropriate repo (pal-e-services or pal-e-platform).
  • [BODY] Add note in File Targets that migrations/ directory must be created (does not exist yet).
  • [BODY] Clarify in Constraints that token counts must be summed across all Anthropic API calls within a single process_message() invocation (tool_use loop makes multiple calls).
  • [BODY] Add explicit dependency: "Blocked by: infra ticket for Postgres access (DB + NetworkPolicy + k8s secret)" in Constraints section.