Skip to content

ActivityFeed 200-cap can drop a still-Proposed row → on-Activity "NEEDS YOU" band + badge undercount waiting approvals #216

Description

@hellno

How this surfaced

A codex (GPT-5) cross-model review of #200 / PR #213 (the "surface pending approvals from any surface" work) caught this. #213 fixed the off-Activity cues by re-sourcing them from the uncapped PendingList, but the review exposed a pre-existing daemon-level hazard underneath it that #213 deliberately left in scope for a follow-up. This issue is that follow-up. The brief below comes from a fresh read-only exploration of the daemon + consumers + wire + tests — but the root cause is worth re-examining with fresh eyes, not just patched at the first plausible seam (see Open questions).

Root cause (verify before fixing)

activity_feed() collects all of self.requests, sorts newest-first purely by insertion seq, then .take(ACTIVITY_FEED_CAP) with zero regard for lifecycle:

  • crates/deckard-signerd/src/daemon.rs:1547-1551 — rows.sort_by_key(|(_, req)| Reverse(req.seq)); rows.into_iter().take(ACTIVITY_FEED_CAP)
  • cap constant = 200 at daemon.rs:151

A still-Proposed request is by nature old (proposed earlier, not yet acted on → lower seq), so it is exactly the row shed once 200 higher-seq records exist. The cap is a response-size bound, not a correctness bound (daemon.rs:148-150): expire_stale() only flips status in place (daemon.rs:1679-1692), terminal records are never pruned (no remove/retain; only a wholesale clear() on Unlock), so self.requests grows unbounded per session and >200 is genuinely reachable in a long, high-volume agent run.

The authoritative uncapped set already exists — pending_list() maps the full request map with no .take() (daemon.rs:1499-1526). The deeper asymmetry: a value-bearing "waiting on you" count is derived from a lossy, size-bounded LEDGER (ActivityFeed) instead of the uncapped INBOX (PendingList).

Blast radius

On the Activity surface only, two consumers undercount together (both read the capped self.activity):

  1. the sidebar Activity badge — Shell::waiting_count (shell.rs:3056, on-Activity branch) → activity_pending over the capped feed;
  2. the NEEDS YOU triage band (crates/deckard-app/src/activity_view.rs:343, filter at :259-261).

They hide up to (N − 200) waiting approvals on the very surface built to triage them — the request can lapse unseen at the approval TTL (daemon.rs:44).

Already safe (post-#200): the home waiting strip + meta-rail read the uncapped PendingList background poll (see the documented NOTE at shell.rs:3050-3055). Not affected: the MCP sidecar — activity_lifecycle_label (crates/deckard-mcp/src/sidecar.rs:404-412) maps a single-request StatusView from a per-request Status poll, not a feed-derived count. No agent-facing tool counts feed rows today.

Severity: ~P2. A real wrong-direction trust cue, but the trigger is narrow (>200 records in one session before a Pending expires) and #200 already closed the everywhere-cue.

Solution space (a menu, not a decision — trade-offs to weigh)

A. Lifecycle-aware feed selection (fix at the daemon source). Always include all Proposed rows; apply the 200 cap only to the settled tail. Pros: fixes every feed consumer at the source; keeps band == feed-count coupling; keeps the rich display fields (reason, timestamp_ms). Cons: feed can exceed 200 when many proposals are open (naturally bounded — a human has few open cards); doesn't bound the settled LOG. Wire: additive (daemon-internal).

B. App sources band + badge from the uncapped PendingList on Activity too. Use ActivityFeed only for the settled LOG. Pros: fully closes the undercount from the existing authoritative source; one source of truth for "waiting" matching #200. Cons: two wire calls per Activity refresh; a PendingRecord lacks the feed's display fields (reason: BreachedLimit, timestamp_ms — rpc.rs:246-255 vs 363/369), so the band loses the over-cap reason chip + relative time unless joined by request_id; must rewrite the test that pins the current coupling (badge_matches_feed_on_activity_surface, shell.rs:3656-3661); doesn't protect a future feed-count consumer. Wire: additive (app-only, both requests already exist).

C. Add a feed-level truncation signal (truncated: bool / total_pending: u32). Pros: makes the loss explicit + auditable for any consumer. Cons: the only option that touches the wire, and it's breaking if it wraps the existing reply — Activity(Vec<..>) is a CBOR array; turning it into Activity(struct{rows,total}) is a shape change that breaks old decoders and bumps spec_version (evolution rules #2/#3, docs/build/40-wire-evolution.md). Additive-safe only as a new variant + new capability name. Likely unnecessary if B/A already yield an uncapped count.

D. Prune terminal records and/or raise/remove the cap. Pros: pruning settled rows attacks the root unbounded-growth the cap only masks; raising is a one-line stopgap. Cons: raising only lowers probability (still a cliff); removing serializes the unbounded map on every ~2s poll → frame/memory/render growth over a long session; pruning changes the session-ledger semantics the feed exists to preserve and needs a what/when-to-prune policy.

(A + a settled-only cap, or A + B, may compose better than any single option — worth discussing.)

Test strategy (write the guard at the level the bug lives)

  • Primary — daemon-level regression test in crates/deckard-signerd/src/daemon.rs (closest template: the existing in-process daemon unit tests there). Insert one Proposed/Pending record at a low seq, then 200+ newer settled records, and assert the old row is still discoverable — on activity_feed() (fails today) and on pending_list() (passes today, pins the uncapped guarantee). This is the tightest guard: the defect is daemon-level and there is currently no test exercising the cap boundary. Needs only in-process record insertion — no GPUI, no socket.
  • App-level caveat: shell.rs mod pending_poll_tests currently pins the buggy coupling — badge_matches_feed_on_activity_surface asserts the on-Activity badge equals the capped feed count. Option B must rewrite it; option A keeps it valid.
  • Wire (only if C): the E1–E4 round-trip / unknown-variant / golden-byte freeze tests (docs/build/40-wire-evolution.md) already gate any new field/variant.

Open questions (please reconsider, don't just pick A)

  • Should the daemon prune terminal records by age/count so the settled LOG is naturally bounded while Proposed rows are always kept (A + settled-only cap)? What prune policy?
  • Is the 200 cap worth keeping at all given self.requests already grows unbounded per session? Is a much higher cap + lifecycle priority the simpler honest fix, or does removing it reintroduce unbounded ~2s-poll render growth?
  • Confirm no future agent-facing tool will derive a pending/proposed count from ActivityFeed — if one is planned, the daemon-source fix (A) protects it; the app-only fix (B) does not.

Found via cross-model review of #200 (PR #213). Root-caused by a read-only multi-agent exploration; claims cite file:line but a fresh look at the root cause is explicitly invited before committing to a fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions