Dashboard enrichment follow-ups: pending-count index + DNS-error visibility

[jiashuoz]

Follow-up from #114 review. Two medium-severity items that don't block but are worth cleaning up.

1. Missing supporting index for the PendingCount subquery in `ListAgentsByUser`

The enriched-agent path runs five correlated subqueries per agent row (identity/store.go:584-630). Four of them hit good indexes; the PendingCount subquery does not.

```sql
(SELECT count(*) FROM messages
WHERE agent_id = a.id AND status = 'pending_approval')
```

Existing indexes that the planner can use:

• `idx_messages_agent_created(agent_id, created_at DESC)` — leading column matches, but `status` is residual.
• `idx_messages_pending_approval(approval_expires_at) WHERE status='pending_approval'` — partial, but leading column is wrong for an agent lookup.

In practice the planner falls back to `idx_messages_agent_created` and filters residually. For users with normal pending volumes this is fine. For users with many agents × growing archival pending volume, this could degrade list-agents latency over time.

Author already acknowledged the cost concern in #114's commit body ("Switch to denormalized columns if read cost ever bites"). Two cleaner fixes:

Option A (cheapest): add a composite index.
```sql
CREATE INDEX IF NOT EXISTS idx_messages_agent_pending
ON messages (agent_id)
WHERE status = 'pending_approval';
```
Partial index on the pending subset, leading column `agent_id` — exactly what the subquery wants. Idempotent.

Option B (denormalize): cache the count on `agent_identities` with triggers or a periodic refresh job. More moving parts; defer until A proves insufficient.

Recommend A as a 5-line migration.

2. `handleVerifyDomain` silently swallows DNS lookup errors

`internal/agent/api.go` — `checkDomainRecords` was reshaped in #114 to return a per-record diagnostic. Previously, `net.LookupTXT` failures short-circuited with HTTP 400 ("DNS lookup failed for X: ..."). Now the error is swallowed:

```go
if txts, err := net.LookupTXT(domain); err == nil {
// ... process records ...
}
// no else branch — err is dropped
```

Same pattern for `net.LookupMX`. If TXT is genuinely missing, the response is 412 with `mx/spf` all "missing" — fine for the UI which renders chips. But true infrastructure failures (network down, resolver returning SERVFAIL, etc.) now look identical to "TXT record missing" → 412 with same shape. CLI and SDK callers lose the distinction.

Fix: preserve the structured diagnostic but surface the lookup error. Two options:

Option A: add a top-level field.
```json
{
"mx": "missing",
"spf": "missing",
"dkim": "deferred",
"verified": false,
"dns_error": "lookup_failed: dial tcp ..." // optional, only set on err
}
```

Option B: tri-state per record.
```go
type RecordStatus string
const (
RecordFound RecordStatus = "found"
RecordMissing RecordStatus = "missing"
RecordError RecordStatus = "error" // new
RecordDeferred RecordStatus = "deferred"
)
```
Per-record granularity (TXT errors don't affect MX lookup status, etc.) — but more surface area.

Option A is the lower-friction choice. Also: log the actual `err` server-side regardless (the current code drops it on the floor).

Effort

Both small — A1 is a 5-line migration + index, A2 is ~10 lines of Go + a new optional field in the response struct. Could land as one PR.

Test coverage gaps

• No test that exercises the real-DNS failure path on `handleVerifyDomain`. Hard without DNS mocking — acceptable as long as the new error field is at least visible to the operator via log.
• No explicit negative-window test for `?window=-5` on `/api/dashboard/stats` (Atoi succeeds, `<= 0` falls back to default). Behavior is correct but unpinned.

Existing mitigations

• The PR's commit body acknowledges the cost concern on (1) — this is documented, just unaddressed.
• DKIM "deferred" is correctly handled end-to-end (backend hardcodes; UI hides the row) so (2) doesn't propagate to that surface.

[jiashuoz]

🔍 Issue audit (automated triage, 2026-06-26)

Audited against main (HEAD 6ddd35c). Both items are still open.

---

Item 1 — Missing index for pending_count correlated subquery

Stale? No — the index proposed in the issue (idx_messages_agent_pending ON messages (agent_id) WHERE status='pending_approval') was never created. The status name changed from pending_approval → pending_review in migration 044_unify_holds.sql (line 44 drops the old idx_messages_pending_approval), but no replacement index with agent_id as the leading column was added.

Current correlated subquery (internal/identity/store.go:929-930):

(SELECT count(*) FROM messages m
   WHERE m.agent_id = a.id AND m.status = 'pending_review' AND m.direction = 'outbound') AS pending_count,

Existing indexes the planner can fall back to:

• idx_messages_agent_created (agent_id, created_at DESC) — agent_id leads, but status/direction are residual (001_init.sql:94)
• idx_messages_pending_review (approval_expires_at) WHERE status='pending_review' — partial but wrong leading column; useful for expiration sweeps, not per-agent counts (040_screening.sql:43-45)

Migration 044's comment ("the outbound pending-sweep index is now covered by idx_messages_pending_review", line 42) refers to the expiry sweep query at store.go:3131-3132, not the correlated subquery in ListAgentsByUser. The two queries have different access patterns — the sweep is global; the correlated subquery iterates per-agent.

• Severity: medium — no correctness bug, latency degrades silently as pending backlog grows across many agents; acceptable today, becomes a real problem under scale
• Correctness bug? No
• Root cause: store.go:929-930 — no partial index with (agent_id) WHERE status='pending_review' AND direction='outbound' exists in any migration
• Best fix: Add one migration (the issue's Option A, updated for the renamed status):

  CREATE INDEX IF NOT EXISTS idx_messages_agent_pending
      ON messages (agent_id)
      WHERE status = 'pending_review' AND direction = 'outbound';

  This is exactly what the correlated subquery needs and makes the ListAgentsByUser enrichment O(index-scan) per agent rather than residual-filter on the full agent_id index.
• Worth fixing? Fix later — 5-line migration, no schema risk, but not urgent until pending volumes are observed causing latency. File as a standalone perf migration.

---

Item 2 — checkDomainRecords silently drops DNS lookup errors

Stale? No — the handler moved from internal/agent/api.go to internal/httpapi/domains.go (handleVerifyDomain) via the /v1 cutover, but the underlying probe function checkDomainRecords (internal/agent/api.go:924) is unchanged. The error-swallowing pattern at lines 942 and 954 is still present:

if txts, err := net.LookupTXT(domain); err == nil {
    // ... process records ...
}
// err is silently dropped — no else branch, no log

Same pattern for net.LookupMX at line 954. Neither the DomainCheckResult struct (internal/httpapi/domains.go:154-159) nor the VerifyDomainView response struct (domains.go:162-169) carries a dns_error field. The apiserver wires CheckDomainRecords → VerifyProbe at internal/apiserver/apiserver.go:175-178 and the field mapping is lossless — but there is nothing to pass through since the inner function discards the error.

Result: a SERVFAIL, a network partition, or a resolver outage returns an identical 412 {mx: "missing", spf: "missing"} as "record not yet published". The operator sees the same diagnosis for two completely different root causes.

• Severity: medium — misclassifies infrastructure failures as missing-record conditions; breaks the trust of the diagnostic endpoint that the issue explicitly created to help operators debug
• Correctness bug? Yes — the endpoint's documented purpose is to return a per-record diagnostic; returning "missing" when the cause is SERVFAIL is an incorrect classification
• Root cause: internal/agent/api.go:942 and :954 — err from net.LookupTXT / net.LookupMX is discarded with no log and no API surface
• Best fix: Option A from the issue (lower friction): add an optional dns_error string field to DomainCheckResult and VerifyDomainView; populate it in checkDomainRecords when either lookup fails; log server-side regardless. The DNSRecordCheck → DomainCheckResult mapping in apiserver.go:177 would need to pass the new field through. No breaking API change since the field is omitempty.
• Worth fixing? Fix now — the diagnostic endpoint is the primary self-service tool for domain setup failures. Silently misclassifying infra failures as user-config errors is the opposite of what that endpoint is for.

---

Bonus — negative ?window test gap (issue's "test coverage gaps" item)

Stale? Effectively closed. TestHandleDashboardStats_WindowQueryParam (internal/auth/auth_test.go:419) pins default, ?window=30, a garbage string, and ?window=9999 (clamped to 90). The windowDays <= 0 guard at store.go:3090-3091 means ?window=-5 falls back to default correctly. The exact -5 case is unpinned in the test but the behavior is structurally guaranteed by the store guard. Low risk, cosmetic.

---

Summary

Item	Fixed?	Severity	Correctness bug?	Action
1. PendingCount index	No	Medium	No	Fix later — 5-line migration
2. DNS error swallowed	No	Medium	Yes	Fix now — add dns_error field + logging
3. Negative-window test	Effectively yes	Cosmetic	No	Wontfix