pal-e-docs

Review: Interaction logging to Postgres

review-684-2026-03-28 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone
  • [x] Repo — forgejo_admin/westside-ai-assistant
  • [x] User Story — present, 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 listed
  • [x] Acceptance Criteria — 5 items
  • [x] Test Expectations — 3 unit tests + run command
  • [x] Constraints — asyncpg, fire-and-forget, token counting, CNPG cluster, 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. No standalone arch-A2 note, but the component is covered in the existing architecture notes, which is acceptable for this project's convention.
  • [x] Forgejo issue — forgejo_admin/westside-ai-assistant#19, state: open

File Targets

  • [x] app/ai.py — verified: exists, contains process_message() function. Logging call should go after Anthropic API response processing. The response object has usage.input_tokens / usage.output_tokens available from the Anthropic SDK.
  • [x] app/config.py — verified: exists, contains Settings(BaseSettings) class. Adding database_url: str = "" is straightforward.
  • [x] requirements.txt — verified: exists, current deps include fastapi, uvicorn, httpx, anthropic, pydantic-settings, pytest.
  • [x] app/logging_db.py — new file to create. Parent directory app/ exists.
  • [ ] migrations/001_interaction_log.sql — ISSUE: migrations/ directory does not exist. Agent will need to create the directory. This is fine for a new file target, but the ticket should note the directory must be created.
  • [x] Exclusions: app/basketball.py, app/groupme.py, app/confirmation.py — all verified to exist. Good boundary definition.

Repo Placement

OK — issue filed on forgejo_admin/westside-ai-assistant, all app-level file targets are in that repo. However, the Constraints section mentions two pieces of infra work that belong in OTHER repos:
  • Create new DB in CNPG cluster (pal-e-postgres) — this is a pal-e-services or pal-e-platform concern (kubectl exec / terraform)
  • NetworkPolicy update for westside-ai-assistant → postgres namespace — this is a pal-e-platform concern
These infra prerequisites are mentioned in Constraints but have no corresponding Forgejo issues. The app code will fail to connect until these are done. This is the primary scoping gap.

Dependencies

  • Infra: Postgres database creation — new database must be created in CNPG pal-e-postgres cluster before the app can connect. No ticket exists for this.
  • Infra: NetworkPolicy — westside-ai-assistant namespace needs egress to postgres namespace. Issue #3 (closed) covered the original NetworkPolicy for basketball-api access, not postgres. No ticket exists for postgres access.
  • Infra: K8s secret — DATABASE_URL must be wired as a k8s secret and mounted in the deployment. No ticket exists — would need a pal-e-deployments kustomize overlay update.
  • Board item #683 (Repo restructure: Nemo agent + modular tool registry) — also in backlog, also touches app/ai.py. These two tickets will conflict if worked in parallel. Not a blocker but ordering matters.
  • Board item #685 (Read-only lockdown) — also in backlog, also touches app/ai.py. Same ordering concern.

Acceptance Criteria

  • [x] AC1 (table created via migration SQL) — verifiable: agent can check file exists with correct DDL
  • [x] AC2 (every message logged with all fields) — verifiable: unit test can mock asyncpg and assert fields
  • [x] AC3 (logging failure doesn't crash bot) — verifiable: unit test can raise in mock and assert no propagation
  • [x] AC4 (DATABASE_URL configurable) — verifiable: check Settings class has the field
  • [x] AC5 (query logs SQL) — verifiable only in integration, not unit test. The SQL in the AC is a demonstration query, not a test command. Acceptable as a "this should work" criterion.
All AC items are testable by an agent. No missing criteria detected.

Blast Radius

  • app/ai.py is the core engine (~350 lines). Changes must be minimal — a single await log_interaction(...) call wrapped in try/except after response processing. The fire-and-forget constraint is correct.
  • No sibling services share this pattern — this is the only GroupMe bot in the platform.
  • The Anthropic SDK's synchronous messages.create() is used (not async). The logging call is async (asyncpg). This means process_message() is already async, so await log_interaction() works naturally.
  • Token counting: The current code does not capture the response.usage object. The agent will need to extract it from the Anthropic response. This is straightforward but the ticket could be more explicit about where in the tool loop to capture tokens (multiple Anthropic calls happen in the tool_use loop).

Decomposition

5 file targets (2 new, 3 modified) in 1 repo. 5 AC. Estimated agent time: ~3-4 minutes. The app-level work fits in a single agent pass.
However, the infra work (DB creation + NetworkPolicy + k8s secret) is a separate concern that MUST be a separate ticket (or tickets). The app ticket should be scoped to app code only, with a documented dependency on the infra ticket(s).
No decomposition of the app ticket itself is needed.

Recommendations

  • [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 → postgres namespace, (3) k8s secret with DATABASE_URL wired in pal-e-deployments kustomize overlay. This ticket blocks #19.
  • [BODY] Add note in File Targets that migrations/ directory must be created (does not exist yet).
  • [BODY] Add note in Constraints or Context clarifying where in the tool_use loop to capture token counts — the current code makes multiple Anthropic API calls during tool loops, so total tokens should be summed across all calls in a single process_message() invocation.
  • [BODY] Add explicit dependency reference: "Blocked by: infra ticket for Postgres access (DB + NetworkPolicy + k8s secret)" in Constraints section.