fix(agent+ui): whatsapp session audit — approvals, stuck indicator, stale execs

P1: add docker compose (logs|ps|top|config|images|port|cp) to read-only
allowlist. docker compose logs was classified as config_mutation, causing
individual approval cards for read-only inspection commands.

P2: remove approval entries from activityLog. They were always status=running
and never transitioned to done (the derived store builds from tool-call
text, not execution status), causing AgentIndicator to latch onto a stale
'Approval: ...' entry and never clear — even after the session completed.

P3: remove InlineApproval from Chat.svelte. The green 'Completed in 1s on
lxc:...' boxes were noise in the chat stream. Approval UX belongs in the
Operations page (already has it via Ops.svelte), not inline in the chat.

P4: stale execution cleanup. Startup sweep (mark >1hr non-terminal as
cancelled) + 5-min periodic sweep (mark >10min non-terminal as cancelled).
98 orphaned executions accumulated from eval testing (39 running from
apt_upgrade:audit timeouts, 19 pending_approval, 3 approved).

P5: refuse second config_mutation run when an approval is already pending
for the session. Without this, the agent queues N individual approvals
before the operator can respond — confirmed in session 20757eb9 (two
approval cards for what should have been one plan-level approval).

VERSION 0.7.0 → 0.7.1

plans/done/2026-07-15-whatsapp-session-audit.md

@@ -0,0 +1,235 @@
+# 2026-07-15 — WhatsApp session audit: approvals, stuck indicator, stale executions
+
+**Status:** Done — 2026-07-15. P1–P5 implemented; build + tests + vet pass.
+Session audit of the WhatsApp bridge
+investigation (`20757eb9`) following the plan-first + iteration deploy.
+Four issues reported by the operator, each traced to a distinct root cause.
+
+## Session under audit
+
+| Session | Goal | Messages | Outcome |
+|---|---|---|---|
+| `20757eb9` | Diagnose and fix the Matrix WhatsApp bridge (stopped delivering messages) | 3 (1 user, 2 assistant) | success — image was 3 months old, `docker compose pull` fixed it |
+
+The agent correctly diagnosed a 405 protocol-version rejection, pulled the
+latest image, and the bridge reconnected. The structural fixes all worked
+(plan-first gate refused the first `run` before `propose_plan`, plan was
+proposed, writeback happened, `complete_task` closed the loop). But the UX
+around the execution was wrong.
+
+---
+
+## Findings
+
+### F1 — Two approvals instead of one (BLOCKER, misclassification + prose)
+
+**What happened:** The agent proposed a 6-step plan and immediately started
+executing steps 1-2 (read-only inspection: `docker compose logs`,
+`docker compose ps`). Both `run` calls were classified as `config_mutation`
+and queued for individual approval. The operator saw two approval cards
+instead of one plan-level approval.
+
+**Root cause A — `docker compose` subcommands missing from read-only
+allowlist.** `internal/policy/command.go:69-78` has `docker\s+(ps|images|
+inspect|logs|version|info|stats)` but NOT `docker compose` subcommands.
+`docker compose logs` and `docker compose ps` are read-only inspection
+verbs that the classifier escalates to `config_mutation`. This is the same
+class of bug as the `find` omission from the Proton Drive audit (P4).
+
+**Root cause B — agent didn't stop after proposing.** The `propose_plan`
+return text says "If any step is config_mutation/destructive, STOP and wait
+for operator approval." The agent ignored this — it started executing in the
+same turn. This is prose enforcement, not structural. Combined with root
+cause A, the read-only steps generated approval cards.
+
+### F2 — Agent indicator stuck at "Approval: Check WhatsApp bridge..." (FRICTION)
+
+**What happened:** After the session completed (status=`done`), the agent
+indicator at the bottom of the chat stayed stuck showing "Approval: Check
+WhatsApp bridge container status on elementsynapse" with a spinner.
+
+**Root cause:** `web/src/lib/stores/activity.ts:131-150` creates an
+`approval` entry with `status: 'running'` whenever a tool result contains
+"requires approval." This entry is **never transitioned to `done`** — the
+derived store rebuilds from messages on every poll, but the approval entry
+is always set to `status: 'running'` (line 146). The `AgentIndicator`
+(`Chat.svelte:153`) shows the first `running` entry from `$activityLog`,
+so it latches onto the stale approval entry and never clears.
+
+There's no mechanism to check whether the execution has actually completed
+— the activity store derives purely from tool-call text, not execution
+status from the API.
+
+### F3 — Green "Completed in 1s on lxc:..." boxes in chat (COSMETIC)
+
+**What happened:** Green success cards (`InlineApproval.svelte:154-163`)
+rendered inline in the chat message stream for each completed execution.
+
+**Root cause:** `Chat.svelte:144-146` renders `<InlineApproval>` inside
+each message bubble when `msg.pendingApprovals.length > 0`. The
+`InlineApproval` component shows the full approval lifecycle (pending →
+running → completed/failed) inline in the chat. The operator considers
+this noise — execution results belong in the activity sidebar, not in the
+chat stream. The chat should show the agent's text + tool call summary, not
+approval UX.
+
+### F4 — 98 stale non-terminal executions (COSMETIC, ops debt)
+
+**What happened:** 98 executions in non-terminal states
+(39 `running`, 19 `pending_approval`, 3 `approved`, 37 more `running`
+orphaned) from eval testing.
+
+**Breakdown:**
+- 39 `running` executions from `apt_upgrade:audit` actions — these were
+  created by the MCP `run` handler, then the MCP call timed out (30s
+  context deadline), leaving the execution in `running` state forever.
+  Not linked to any session (orphaned).
+- 19 `pending_approval` — config_mutation `run` calls that were queued for
+  approval but never approved/denied (eval sessions that completed without
+  resolving them).
+- 3 `approved` — approved but never executed (the execution dispatch
+  failed or timed out).
+
+**Root cause:** No startup or periodic cleanup of stale executions. The
+`run` handler creates an execution entity BEFORE attempting SSH — if the
+SSH call times out or the MCP connection drops, the execution is
+orphaned in `running` state. `completeTask` cancels pending approvals for
+its own session, but nothing cleans up orphaned executions or old
+sessions' leftovers.
+
+---
+
+## Improvement plan
+
+### P1 — Add `docker compose` read-only subcommands to allowlist
+
+**Fix:** `internal/policy/command.go` — add to `readOnlyLeadPattern`:
+`docker\s+compose\s+(logs|ps|top|config|images|port|cp)\b`.
+Do NOT add `docker compose exec` or `docker compose run` — these execute
+arbitrary commands and must stay gated.
+
+Add unit test case: `"docker compose logs --tail=100"` → `read_only`.
+
+**Severity:** blocker (directly caused the two-approval issue).
+**Files:** `internal/policy/command.go:69-78`, `command_test.go`.
+
+### P2 — Fix stuck agent indicator
+
+**Fix:** `web/src/lib/stores/activity.ts:131-150` — the approval entries
+are always `status: 'running'` and never transition. Two options:
+
+**Option A (recommended): Remove approval entries from activityLog
+entirely.** They're already rendered as `InlineApproval` cards in the chat
+(or, after P3, in the Operations page). Duplicating them in the activity
+log causes the stuck indicator — the activity store derives from tool-call
+text, not execution status, so it can't know when the execution completed.
+Removing them means `AgentIndicator` won't latch onto stale approval
+entries.
+
+**Option B: Fetch execution status.** When building approval entries,
+call `getExecution(execId)` to check the real status. This is more
+correct but adds async API calls to a synchronous derived store — would
+require restructuring the store to be async or pre-fetching statuses.
+
+**Decision:** Option A — simpler, eliminates the bug class. If the
+operator wants approval status in the activity sidebar, that's a separate
+feature that should use the REST `/approvals` endpoint (already used by
+`context.ts` and `Ops.svelte`), not text parsing of tool results.
+
+**Severity:** friction.
+**Files:** `web/src/lib/stores/activity.ts:131-150`.
+
+### P3 — Move InlineApproval out of chat, into activity sidebar
+
+**Fix:** Remove `<InlineApproval>` from `Chat.svelte:144-146`. The chat
+stream shows only the agent's text + `ToolCallGroup` (the compact tool
+counter). Approval UX (Approve/Deny buttons, completed/failed cards)
+moves to the Operations page (already has it via `Ops.svelte`) and/or a
+dedicated approval panel in the sidebar.
+
+The `pendingApprovals` field on `ChatMessage` can stay (for counting
+badges in the session rail), but the inline rendering is removed.
+
+**Migration:** `InlineApproval.svelte` is not deleted — it's reused in
+the Operations page or a new sidebar approval panel. The component's
+props (`PendingApproval[]`) and API (`getExecution`, `decideApproval`)
+stay the same.
+
+**Severity:** cosmetic.
+**Files:** `web/src/pages/Chat.svelte:144-146`.
+
+### P4 — Stale execution cleanup
+
+**Fix:** Add a startup cleanup + periodic sweep in nomos:
+
+1. **Startup cleanup:** on `nomos serve` boot, mark all non-terminal
+   executions older than 1 hour as `cancelled` with result
+   `{"message": "cleaned up at startup — stale from prior session"}`.
+   This handles the 98 stale executions from eval testing.
+
+2. **Run handler fix:** in `internal/mcp/server.go` `classifyAndGate`,
+   the execution entity is created (line 1284-1293) BEFORE the SSH call.
+   If the SSH call fails or times out, the execution is already marked
+   `running` but never transitions. The error paths (lines 1315-1321,
+   1333-1340, etc.) already mark `failed` — but the MCP client timeout
+   (30s, in `agent.go`'s `client.callTool`) kills the connection before
+   the error path runs. Fix: move the execution entity creation to AFTER
+   the SSH call succeeds, or add a `running` → `failed` timeout sweep.
+
+3. **Periodic sweep:** add a 5-minute timer (like the continuation
+   worker) that marks executions in `running` state for more than 10
+   minutes as `failed` with result `{"message": "execution timed out"}`.
+   This catches orphaned executions that the run handler didn't clean up.
+
+**Severity:** cosmetic (ops debt, not a functional bug).
+**Files:** `cmd/nomos/main.go` (startup), `internal/mcp/server.go`
+(run handler), `cmd/nomos/continue.go` (periodic sweep).
+
+### P5 — Enforce "stop after proposing a plan with config_mutation steps"
+
+**Fix:** This is the structural enforcement gap behind the "agent didn't
+stop after proposing" behavior. The `propose_plan` return text says "STOP
+and wait" but nothing enforces it. Two options:
+
+**Option A (structural):** In the `run` handler (`classifyAndGate`), after
+the plan-first gate, check if the plan has any `config_mutation` steps
+AND no assent window is active. If so, refuse the `run` with "This plan
+has config_mutation steps — wait for operator approval before executing."
+This would force the agent to stop after proposing, but it would also
+block the legitimate case where the operator already said "go ahead" (the
+assent window would be active, so the check would pass).
+
+**Option B (prose):** Strengthen the `propose_plan` return text and
+SOUL.md to be more directive. This is what we've been doing — it works
+for strong models but not for weaker ones.
+
+**Decision:** Option A — structural enforcement. The check is simple
+(assent window active?) and catches the exact case where the agent
+proposes a plan with config_mutation steps and starts executing without
+approval. Read-only steps still auto-execute (they don't need the
+assent window).
+
+**Severity:** friction (prevents the two-approval UX, but doesn't block
+functionality).
+**Files:** `internal/mcp/server.go` `classifyAndGate`, `nomos/SOUL.md`.
+
+---
+
+## Sequencing
+
+- **P1** (docker compose allowlist) is independent — ship immediately.
+- **P2** (stuck indicator) + **P3** (inline approval removal) ship
+  together — both touch the chat rendering surface.
+- **P4** (stale cleanup) is independent — ship anytime.
+- **P5** (config_mutation enforcement) depends on P1 (the allowlist fix
+  reduces false config_mutation classifications) — ship after P1.
+
+## Verification
+
+- `go test ./internal/policy/...` — new `docker compose logs` read-only
+  test case.
+- Manual: replay the WhatsApp bridge prompt, confirm a single plan-level
+  approval (not two), no stuck indicator, no green boxes in chat.
+- `docker exec oikos-postgres-1 psql -U oikos oikos -c "SELECT COUNT(*)
+  FROM executions WHERE status NOT IN ('completed','failed','cancelled')"`
+  → 0 after the startup cleanup runs.