diff --git a/docs/adr/ADR-017-attention-routing.md b/docs/adr/ADR-017-attention-routing.md index 10f335c64..0adc0e190 100644 --- a/docs/adr/ADR-017-attention-routing.md +++ b/docs/adr/ADR-017-attention-routing.md @@ -1,6 +1,6 @@ # ADR-017 — Attention routing -**Status:** Proposed — full draft for ratification (supersedes the 2026-07-28 stub). §Layer 3.1 (attention queue v1, added 2026-08-25 for TASK-069) is a spec awaiting the same ratification and carries three explicitly undecided items, listed at §Ratification-points 3, 4a and 4b — `Proposed` here must not be read as having chosen between the two escalation mechanisms, nor as having decided whether `blockedOn` names its blocker, nor what clears the rows no derive can reach. +**Status:** Proposed — full draft for ratification (supersedes the 2026-07-28 stub). §Layer 3.1 (attention queue v1, added 2026-08-25 for TASK-069) is a spec awaiting the same ratification and carries three explicitly undecided items, listed at §Ratification-points 3, 4a and 4b — `Proposed` here must not be read as having chosen between the two escalation mechanisms, nor as having decided whether `blockedOn` names its blocker, nor what clears the rows no derive can reach. **Sam ruled on 2026-08-26 that this queue is the shell's home surface rather than a page**, which makes §Layer 3.1's three missing fact sources the critical path; §The-three-missing-sources specifies them at field level, and that specification is deliberately written so those three ratification points stay open rather than being settled by implementation detail. **Date opened:** 2026-07-28 **Date drafted:** 2026-07-29 **Author:** pod-architect (Sam ratifies; delivery-channel choice is explicitly his) @@ -275,7 +275,7 @@ Sam named four row types. Exactly one of them has a fact source that already beh | row type | fact source today | can the fact change? | v1 verdict | |---|---|---|---| -| **approval pending** | `Activity.approval.status` ∈ `pending \| approved \| rejected`; served by `GET /api/activity/approvals` → `ActivityService.getPendingApprovals` | **yes** — status transition | **read path ready; no producer.** Ship the column, expect it empty — see below | +| **approval pending** | `Activity.approval.status` ∈ `pending \| approved \| rejected`; served by `GET /api/activity/approvals` → `ActivityService.getPendingApprovals` | **yes** — status transition | **backend read path ready; no producer.** A consumer arrived with #1274, merged to main as `cccddef7`; the producer gap is what keeps the column empty — see below | | **human @mention** | derived at read time: `activityService.ts:517-521` builds `'@' + lowerUsername` and sets `isMention` from a substring test; `:591` is the `mentions` filter | **no** — the message text never stops containing the handle | needs an explicit ack (below) | | **blocked on human** | none. `Task.status` has a `blocked` value, but it records **no blocker identity** | n/a | needs a field | | **agent question to a human** | **none.** `AgentAsk` addresses `targetAgent` + `targetInstanceId` (`models/AgentAsk.ts:52-55`). There is no human target | n/a | needs a target widening | @@ -284,11 +284,11 @@ Sam named four row types. Exactly one of them has a fact source that already beh The table above called this row type "ready, ship unchanged" on the strength of its read path, and both reviewers of the first draft took that on trust — it is the one row type the spec does not propose to change, which is exactly why nobody checked it. Measured at `origin/main` (`6a262fe8`): -**The resolve path is real and complete.** `GET /api/activity/approvals` (`routes/activity.ts:53`) → `ActivityService.getPendingApprovals` (`:908`) → `Activity.getPendingApprovals` (`models/Activity.ts:201`), which filters `type: 'approval_needed'` + `'approval.status': 'pending'` + not deleted. Resolution is `POST /:activityId/approve` and `/:activityId/reject` (`:123`, `:137`), both type-guarded, both writing `status`, `reviewedBy`, `reviewedAt` via the model methods at `:231` and `:239`. The frontend card exists (`V2ApprovalCard.tsx`). Nothing here needs building. +**The resolve path is real and complete.** `GET /api/activity/approvals` (`routes/activity.ts:53`) → `ActivityService.getPendingApprovals` (`:908`) → `Activity.getPendingApprovals` (`models/Activity.ts:201`), which filters `type: 'approval_needed'` + `'approval.status': 'pending'` + not deleted. Resolution is `POST /:activityId/approve` and `/:activityId/reject` (`:123`, `:137`), both type-guarded, both writing `status`, `reviewedBy`, `reviewedAt` via the model methods at `:231` and `:239`. **The backend resolve path is complete; the frontend one does not exist, and an earlier revision of this line claimed it did.** `V2ApprovalCard.tsx` is a real, rendered card (`V2MessageBubble.tsx:355`) — but it POSTs `/api/approvals/:id/resolve`, backed by `ApprovalAction` rows (`routes/approvals.ts`, mounted at `server.ts:198`), which is a **different store from `Activity`**. Two approval systems share a word and nothing else. The `Activity` endpoints' only caller was `frontend/src/components/activity/ActivityFeed.tsx`. **#1274 both deletes that file and adds a replacement**, so the consumer is swapped rather than removed: `frontend/src/v2/components/V2ActivityPage.tsx` POSTs `/api/activity/:id/approve|reject` and `/acknowledge`. **#1274 has since merged (`cccddef7`), and this claim is now re-derived on main rather than on a branch:** `V2ActivityPage.tsx` at `origin/main` carries those three calls, and `frontend/src/components/activity/ActivityFeed.tsx` is gone. An earlier revision of this paragraph said the endpoints would have zero frontend callers after #1274 landed — that was true of that PR's head at 13:25Z and stopped being true a few commits later, which is the decay this ADR keeps warning about: a claim about another open PR expires on its next push. A second revision then cited the branch head `c418abd5`, which the merge made unreachable from any surviving ref — so the citation aged out a second way, by naming a commit no reader can resolve. **Cite the merge, not the head that happened to be current while the PR was open.** So the row's defect is narrower than "no producer and no consumer" and does not move: **the producer is the gap**, and it is the one #1274 cannot fill. Naming a card that resolves against another store is exactly the mistake this ADR exists to prevent — a surface was confirmed to exist without confirming what it talks to. -**The producer does not exist.** `Activity.createApprovalRequest` (`models/Activity.ts:175`) and its service wrapper (`activityService.ts:790`) have **zero callers** — no route exposes them, no service invokes them. Positive control for the search: `getPendingApprovals` resolves route → service → model by the same grep, so the method does detect call sites where they exist. +**The designed producer has zero callers.** `Activity.createApprovalRequest` (`models/Activity.ts:175`) and its service wrapper (`activityService.ts:790`) have **zero callers** — no route exposes them, no service invokes them. Positive control for the search: `getPendingApprovals` resolves route → service → model by the same grep, so the method does detect call sites where they exist. -The only code that ever creates an `approval_needed` row is `ActivityService.seedPodActivities` (`:927`, the row at `:989`), reachable via `POST /api/activity/seed/:podId` — demo fixture data, content `"An agent is requesting access to the Production pod"`, `agentName: 'analytics-bot'`. So every approval this queue could show today is seeded, not requested. +**Two other paths do create one, and neither is an approval workflow.** The first is `ActivityService.seedPodActivities` (`:927`, the row at `:989`), reachable via `POST /api/activity/seed/:podId` — demo fixture data, content `"An agent is requesting access to the Production pod"`, `agentName: 'analytics-bot'`, actor `commonly-bot`/`system`. The second is the generic `POST /api/activity/create` (`routes/activity.ts:164`), which takes `type` and `podId` straight off the request body behind `auth` alone — no pod-membership check — and does not pass an `approval` subdoc. It does not need to: the schema declares `approval.status` with `default: 'pending'` (`models/Activity.ts:111`), so Mongoose materialises exactly the two fields `getPendingApprovals` filters on. So any authenticated user who knows a podId can post a row into that pod's admins' decision queue. That is a defect in its own right, not a fact about this spec; it is recorded here because an implementer reading "nothing produces these rows" would not go looking for it. What holds for v1: every approval this queue could show today is seeded or injected, never requested. **Which changes the v1 verdict without changing the design.** The approval column ships as specified and will be empty until something requests an approval — and the natural producer is the v1.5 tool-layer refuse-and-park row in the escalation table above, which is not v1. That is not a reason to cut the column: an empty column with a working resolve path is the correct state for a capability whose producer is scheduled. It **is** a reason not to let "one of the four row types is already ready" carry weight in ratification, because the readiness is a half. @@ -362,6 +362,76 @@ Per row: an **approval** leaves on `status != 'pending'`; a **blocked-on-human** Name it for what it is: `acknowledged`, not `read`. A mention the human has seen and not acted on must be able to stay in the queue — the whole failure this queue exists to fix is attention that was technically delivered and never acted on, and a surface that clears on view reproduces it exactly. **Rendering a row is never an acknowledgement.** +### The three missing sources, at field level (added 2026-08-26 — Sam ruled this queue is the home surface, so these are the critical path) + +The table above returns three "needs a …" verdicts. They are three changes, not one, and this section is what a sprint-impl seat needs in order to build them without re-deriving the measurements. Nothing here decides §Ratification-points 3, 4a or 4b — each design is stated so that the ratification is a smaller call, not a pre-empted one. + +**They share one property and it is the reason all three are missing.** Each is a fact that cannot *stop being true for a particular human*: a message keeps containing `@sam` forever, a `blocked` task names nobody who could release it, and an ask has no human target to be answered by. The approval row is the only one of the four that already has a transition, which is exactly why it is the only one that needed no design. + +#### 1. The acknowledgement store — shape, and the invariant that makes it safe + +§What-marks-an-item-done establishes the mention as the irreducible exception: no derive exists, so v1 must store an explicit per-`(user, message)` acknowledgement. Its shape: + +``` +AttentionAck { + userId: ObjectId // whose queue this left — never a global "handled" + sourceType: 'mention' // v1 ships one member; see below for why it is an enum anyway + sourceId: string // the Activity._id the mention was derived from + ackedAt: Date +} +unique index: (userId, sourceType, sourceId) +``` + +Keyed by `(user, item)` rather than a field on the source, for a reason that is not stylistic: **`isMention` is derived at read time and never stored** (`activityService.ts:517-521`), so there is no row to mark; and one message can mention two humans, which makes any scalar `dismissedAt` on the Activity wrong by construction. + +**The invariant that keeps this from becoming read-state:** *an ack may only **remove** a row; it must never **create** or **retain** one.* Every failure of the ack store therefore degrades to a re-shown row and never to a hidden one. That is what distinguishes this from the "seen" tracking the opening rule forbids — and it is worth writing down as an invariant rather than as an intention, because the cheap implementation (a read-cursor per user) violates it silently the moment a row arrives out of order. + +**Not a cursor.** A timestamp ("mentions read up to T") is smaller and cannot express skip-this-keep-that, which is the entire behaviour separating a queue from a feed. + +`sourceType` is an enum on day one despite having one member, because §Ratification-point 4b's un-derivable population is the obvious second consumer — TASK-027 and TASK-016 in the measured six are blocked rows that no merge event can clear. Whether those get an ack or the sweep 4b proposes is 4b's call; the store should not have to change shape to find out. + +#### 2. `Task.blockedOn` — the field, and what each variant costs + +``` +blockedOn?: { + kind: 'human' | 'task' | 'external', + userId?: ObjectId, // kind: 'human' — who the queue routes this row to + taskId?: string, // kind: 'task' — what §4a's derive watches + note?: string, // kind: 'external' — free text; no derive is possible +} +``` + +This is §Ratification-point 4a's "carry the blocker's identity" recommendation made concrete, and the `kind` discriminator is doing work beyond routing: it makes the un-derivable population **countable** instead of assumed. Today the argument for 4b's sweep rests on two of six measured rows being underivable; with `kind` recorded at write time, that ratio becomes a query rather than a hand-count, and 4b can be revisited on data. + +Set it where `status` moves to `blocked`. **Do not key the queue on `status`** — §What-marks-an-item-done gives the measured reason (PR #1248 makes a blocked row claimable, and the claim handler's `$set` moves `status` while leaving `blockedOn` untouched). + +#### 3. `AgentAsk` widened to a human target — three changes, and the middle one is the one that gets missed + +Only if §Ratification-point 3 goes that way rather than to the escalation feed. Costed at `origin/main`: + +1. **`targetUserId?: ObjectId`**, and `targetAgent` relaxed from `required: true` to required-only-when-`targetUserId`-is-absent (`models/AgentAsk.ts:52`). **The schema is not the only gate** — `agentAskService.ts:111` throws `400 targetAgent_required` independently, so relaxing the model alone leaves human-targeted asks rejected at the service layer. `respondToAsk`'s identity check at `:264` compares `responderAgent !== ask.targetAgent || responderInstance !== ask.targetInstanceId` — it must branch on which target is set, or a human's response matches nothing and the ask stays open while being answered. + +2. **`expiresAt` must be *omitted* for human-targeted asks, not extended.** Mongo's TTL monitor deletes a document only when the indexed field holds a past date; a document with **no** `expiresAt` is never swept. So the exemption is "do not set the field" — and **that takes two changes to the same schema block, not one** (@sprint-review, gating this section). Relaxing `required: true` (`models/AgentAsk.ts:69`) is necessary and not sufficient: the `default` on the next line (`:70`) fills the path whenever it is undefined, **independent of `required`**, so an ask built against a merely-optional field still carries a 24h TTL and is still deleted at 24h. Measured on mongoose 7.8.6, with the default removed as the control: + +``` +required relaxed, default kept: new M({}).expiresAt => // and validateSync() passes +control, default removed: new M2({}).expiresAt => undefined +``` + +The default must therefore be **conditioned on the ask being agent-targeted**, or moved out of the schema into `createAsk`. This is point 1's own sentence — *the schema is not the only gate* — one layer further down: point 1 caught the service-layer gate below the model, and the second gate here is *inside* the model. + +The one place that reads the value tolerates its absence already — `respondToAsk`'s `ask.expiresAt < new Date()` at `:246` is `false` when the field is undefined, so an omitted `expiresAt` does not false-expire the ask. Extending the window instead only moves the deletion — and §The-cost-of-widening-`AgentAsk` is why that matters: the row leaves *because* it was not handled, leaving no record it existed. + +3. **A real `expired` transition needs a sweep**, since today the status is reachable only by the race at `agentAskService.ts:249`. + +**Whichever way point 3 is ratified, the ack store in §1 is still required** — it belongs to the mention row, which neither mechanism touches. Point 3 decides where an agent's question to a human lives; it does not decide anything about §1. + +#### What the surface consumes, and one constraint it inherits from CI + +The queue's rows are consumed by the work-first shell (TASK-068). One requirement lands back on this spec from that side: a PR-press row must expose **the named base-`main` guard set** — `Stale-base merge guard`, `Source changed ⇒ version bumped`, `CodeQL`, `Analyze` ×3 — as drawn/not-drawn, and must **not** expose a check count. + +The reason is measured rather than aesthetic. A check count is not the identity of a check set: on 2026-08-26 four PRs on this repo showed 11, 11, 10 and 5 checks, where the two 11s were *different sets* (a workflow-file PR draws `kind cluster smoke test` and not `E2E Tests`), and the 10 was a docs PR whose missing `E2E Tests` is a correct path filter. Only one of those shapes — the stacked child at 5 — is a hazard, and it is the one a count cannot distinguish. A renderer handed a number cannot recover which guards ran; the join against the base has to happen in the fact source. + ### Composition with the only-interrupter rule — the constraint that shapes v1 §Layer 3 states that the escalation envelope is the *only* event class permitted to interrupt a human (push · ping · badge), and that activity and social events are **pull, always**. The queue spans both: approvals and blocked-on-human are escalation-shaped; @mentions are social.