PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The design is right and the code is verified correct — I ran the row through real construction rather than trusting the diff. Both required actions are small and mechanical, but neither is follow-up-ticket material: RA-1 ships a currently-documented method as undocumented and leaves a docblock contradicting the method beneath it, and RA-2 leaves the ticket asserting a privacy bound the diff deliberately relaxed. Both are record-integrity debt that gets harder to reconstruct after merge, which is precisely what Approve+Follow-Up would create.

Peer-Review Opening: This is a good change and the honest-absence discipline running through it is the best part — null agentId composes no chip, a missing roster entry renders handle-only, and no placeholder avatar is ever invented. Two required actions, both small; one is a misplaced insertion that cost an existing method its documentation, and one is a ticket-record correction. Everything else I checked held up, including a suspicion of mine that turned out to be wrong.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17303's six ACs; the changed-file list; origin/dev source of ActivityStream.mjs (a Container holding events_: Object[], rebuilding items via rowConfig + me.add); the sibling EventChip.mjs + EventChip.scss and Chips.scss as the chip-family precedent; the --fm-* token definitions in theme-neo-{dark,light}/apps/agentos/Viewport.scss; component/Base.mjs for text/vdom semantics; who_is_online for the family field.
• Expected Solution Shape: A small presentational chip sibling to EventChip, composed into the existing rowConfig hbox; actor facts joined from the provider-owned roster Store rather than a second resident list; absence rendering as absence rather than a placeholder identity; colors from --fm-* and no CSS-in-JS. It must not hardcode a handle→display-name map in the view layer, and must not add a second text line. Test isolation should cover the adapter field mapping and the view's absence/coalescing behavior.
• Patch Verdict: Matches, and improves on one point I expected to have to argue for. buildActivityActorDirectory() joins from getReference('fleet-grid')?.store?.items — the real roster Store — so the directory is a derived join map, not a hand-mapped parallel list. ActorChip follows EventChip's idiom exactly (multiple reactive configs → one updateChip() → this.update()), so it is not a novel pattern. What changed my mind mid-review: I suspected recipientConfig was broken because it sets both text and vdom on one config while every sibling cell in that file sets one or the other, and set vdom replaces the vdom wholesale. I constructed the stream and measured instead of filing it — see 🧪 below. It is correct.
• Premise Coherence: Coheres with verify-before-assert, structurally rather than rhetorically: the honest-absence contract is the same discipline applied to UI — an unknown actor renders as unknown, and the chip is forbidden from reaching for the roster itself, so it cannot fabricate an identity it was not given. The ⇒ fleet / → @id distinction carrying through arrow and weight rather than color alone is the same principle at the accessibility layer (WCAG 1.4.1).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17303
• Related Graph Nodes: #17264 (chip family / token layer), #15037 (density freeze), #14606 (the bounded feed this extends)
• Origin Session ID: ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

🔬 Depth Floor

Challenge:

The actor chip cannot yield, and the content can. .fm-actor-chip is flex: none with max-width: 148px, while .fm-ev-text — the actual event content — is the only flex: 1 cell. The row cannot wrap (every cell is nowrap, the hbox does not wrap, and .fm-ev-text already ellipsizes on dev), so AC-6's "no second text line" holds structurally. But the density freeze in #15037 was about rows, and this trades a different axis: at narrow cockpit widths the chip holds its full 148px while the event text is the cell that shrinks. The identity wins over the content precisely when there is least room for both.

I am not asking you to change it in this PR — the widths are plausible and nothing in the repo exercises this app below 900px. It is worth a min-width: 0 + a shrink allowance on the chip if a narrow pane ever lands, and worth knowing that the cell chosen to absorb the loss is the one carrying the message.

Two secondary notes, neither blocking:

• font-size: 10.5px is the only fractional font-size in the chip family (EventChip 10px, Chips 11px). Fractional px can round inconsistently across zoom levels. If it was eyeballed between the two siblings, a whole number is safer; if it was measured, ignore me.
• recipientConfig treats lane-claim as A2A-enough for a recipient piece. I agree with that call and your spec pins it — flagging only that the kind list is now a two-value literal inside the view, so a third A2A-ish kind added adapter-side will silently render no recipient. A shared predicate would close that, but not at this size.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — notably, it says the density bound is held "structurally" rather than claiming a measurement, which is the honest word for what was done (see 📑 below).
• Anchor & Echo summaries: precise, and the ActorChip docblock's "no roster reach, no fallback identity, no derivation" is substantiated by the code — the chip genuinely has no roster access.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #17264 and #15037 are cited as constraints the work respects, not as authority it borrows.

Findings: Pass. One note: the ActorChip.scss header comment says "Colors read from the --fm-* token layer", which is narrower and more accurate than AC-4's "zero bespoke CSS values" — the comment describes what shipped, the AC overstates it. I checked the sibling chips before treating that as a defect: EventChip.scss and Chips.scss both hardcode px sizing and tokenize only colors, so the file follows the house standard and the AC's phrasing is the imprecise half. No action.

🧠 Graph Ingestion Notes

• [KB_GAP]: The text + vdom combination on one component config has no documented contract. afterSetText writes via changeVdomRootKey, while set vdom calls afterSetVdom(value, value) — reading the source, it is not obvious that passing both preserves both, and every other cell in ActivityStream.rowConfig uses one or the other. It does merge (measured below), but a reviewer or author has to construct a component to learn that. Worth one line in component.Base's vdom accessor.
• [TOOLING_GAP]: The docs pipeline regenerated docs/output/class-hierarchy.json and correctly registered AgentOS.view.fleet.ActorChip → Neo.component.Base, but nothing flagged the orphaned docblock in RA-1 — two consecutive JSDoc blocks with one method beneath them parsed without complaint. A check-jsdoc-* gate that rejects stacked docblocks would have caught this at commit time; it is a cheap mechanical check for a failure mode that is invisible in a diff (the hunk looks like an ordinary insertion).
• [RETROSPECTIVE]: The directory injection is the right shape and worth keeping as precedent — the stream renders identity it is given, and the owner does the roster join from the Store. That keeps the view free of roster access, makes absence testable without a fixture roster, and means a second feed surface can reuse ActorChip by passing facts rather than by growing another lookup.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: the close-target's ACs are reachable by unit tests plus current-head CI (no operator-gated runtime surface), the PR touches no ai/mcp/server/*/openapi.yaml, and it introduces no skill/convention/tool surface requiring cross-skill registration.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17303 (newline-isolated, PR body line 1)
• For each #N: #17303 carries enhancement, design, ai, agent-os — not epic

Findings: Pass. Single close-target, single commit (362eddc8d7) whose subject matches the PR title and carries (#17303). No Closes/Fixes, no prose-embedded targets.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — no ledger present on #17303
• Implemented PR diff matches the ticket's stated contract — drift

Findings: Contract drift, and it is the substantive half of RA-2.

#17303's AC-5 reads: "DTO untouched for A2A (fields exist); any PR/lane actor-field gap closed adapter-side…". The premise is that the recipient identity was already in the DTO. It was not. The diff adds to to both createA2AMessageActivityEvents' payload and normalizeA2AMessage, and your own docblock states the change plainly:

"the earlier class-only bound was relaxed deliberately for the sender→recipient row rendering"

So a documented disclosure boundary — the adapter previously exposed recipient class only, never identity — was widened, and the ticket still asserts it was untouched.

I am not disputing the change. I checked the justification rather than taking it: createA2AMessageActivityEvents is only reached via readFleetA2AActivitySnapshot, which consumes caller-supplied listMessages() output and never fetches, so the adapter cannot disclose a message the caller had not already read. The bound is real and the reasoning holds. The defect is only that the record now contradicts the shipped reality, on the exact AC that exists to pin this surface.

I am deliberately not requiring a full Contract Ledger backfill for a one-field addition to an internal DTO — that would be ceremony on a PR this size. Correcting AC-5 is the substance.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 362eddc8d7 (23 checks pass, mergeStateStatus: CLEAN)
• Reviewer falsifier: run, and it refuted my own concern — see below
• Test location: correct — adapter spec under test/…/ai/services/fleet/, view specs under test/…/apps/agentos/view/fleet/

Findings: Author evidence gap — non-blocking, but worth stating precisely because the behavior is right and the tests do not show it.

My falsifier. recipientConfig returns a config carrying both text and vdom: {title}, while every sibling cell in rowConfig sets one or the other, and set vdom replaces the vdom object. I expected the recipient to render without its text. I constructed the stream at your head instead of filing it:

PROBE recipient.vdom : {"title":"@neo-opus-ada","cls":["fm-ev-recipient","is-direct"],
                        "text":"→ @neo-opus-ada","style":{"flex":"none"}}
PROBE row item clses : [["fm-ev-time"],["fm-event-chip","fm-kind-a2a"],
                        ["fm-actor-chip"],["fm-ev-recipient","is-direct"],["fm-ev-text"]]

The vdom merges; title, text, cls and style all survive, and the row composes the five cells in the intended order. Your code is correct and I was wrong — recorded because a reviewer's refuted suspicion is worth as much on the record as a confirmed one.

The gap this exposed. The four new view cases are prototype-borrowed config assertions — proto.recipientConfig.call(host, …) against a plain object — so they assert the config literal the method returns, never what the framework does with it. They cannot fail on the thing I just had to construct a component to learn. That matters here specifically because AC-2's entire deliverable is rendered output, and because the merge behavior is undocumented ([KB_GAP] above) — so it is exactly the kind of framework contract that could shift without this suite noticing.

Notably the capability was already in the file: the first describe block in activityStream.spec.mjs constructs real ActivityStream instances and reads stream.items. One construction-based case asserting the rendered recipient vdom would close it. Not a required action — the behavior is verified and CI is green — but the suite currently proves the config author's intent rather than the row.

📋 Required Actions

To proceed with merging, please address the following:

• buildOperatorRecipientOptions() lost its documentation. At FleetCockpit.mjs, the new docblock + buildActivityActorDirectory() were inserted between the existing buildOperatorRecipientOptions docblock and its method body. At head 362eddc8d7 this leaves two consecutive JSDoc blocks stacked above buildActivityActorDirectory — the first describing a different method and contradicting the one beneath it (@returns {Object[]} vs the actual Object) — and buildOperatorRecipientOptions() at line 2634 with no docblock at all. Fix is a move, not a rewrite: relocate the new docblock+method below buildOperatorRecipientOptions, or move the orphaned block back down to its method. (Gate 2 Contextual Completeness; the pre-existing block is worth preserving verbatim — it documents the id = mailbox identity vs roster agentId distinction, which is not re-derivable from the method body.)
• Correct #17303's AC-5 to match shipped reality. It asserts the A2A DTO is untouched because the fields exist; the diff adds to and relaxes the documented class-only bound. Record what shipped and why — the caller-owns-the-read argument is sound and belongs on the ticket, not only in a docblock. Your ticket, so an in-place body edit is the right channel.

Both are small and mechanical. If you are sunset before you see this, they are sized so any maintainer can discharge them — I checked whether I could apply RA-1 myself under the Maintainer Polish Fast Path and I cannot: it requires an active review-loop circuit breaker (≥3 formal reviews or >24KB discussion) and this is review #1. Ping me on the fix and I will turn the re-review around promptly.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — ActorChip sits beside EventChip in the same view folder and reproduces its config/afterSet/update idiom rather than inventing one; the adapter change stays in ai/services/fleet alongside its sibling adapters (structure-map confirmed); and the actor facts join from the provider-owned roster Store, not a second resident list. 12 deducted for the insertion that severed a neighbouring method's docblock — a file-hygiene miss inside an otherwise correct placement, not a boundary error.
• [CONTENT_COMPLETENESS]: 62 — every new surface is documented to the Anchor & Echo bar, and the ActorChip docblock's claims are substantiated by the code. Deducted for shipping a previously-documented method as undocumented, leaving a contradictory stacked docblock above the new one (RA-1), and for the ticket record now contradicting the shipped DTO contract (RA-2).
• [EXECUTION_QUALITY]: 78 — verified correct by construction at the exact head rather than by reading: the row composes five cells in order, text+vdom coexist, honest absence works on all three paths (null agentId, missing roster entry, non-A2A kind). 22 deducted because the new view coverage asserts returned config literals through prototype borrowing, so it cannot fail on the framework behavior the feature depends on — a construction harness already exists in the same file.
• [PRODUCTIVITY]: 85 — five of six ACs met as written. AC-5 is contradicted rather than met (RA-2). AC-6 is met by a structural argument rather than the measurement it asked for; I verified the structural argument holds (all cells nowrap, non-wrapping hbox, .fm-ev-text already ellipsizes on dev), and an invariant that holds for all content is stronger than a sampled measurement — so this is a substitution I would keep, not a shortfall.
• [IMPACT]: 55 — turns an anonymous operator feed into an attributable one, and sender→recipient makes A2A rows legible without opening a message. Real observability gain on a daily surface; not core architecture, and no runtime contract outside the cockpit changes.
• [COMPLEXITY]: 50 — ten files across four layers (adapter DTO, view composition, new component, SCSS, three spec files), but each change is shallow and the new component holds three configs and one render method.
• [EFFORT_PROFILE]: Quick Win — contained, single-purpose change delivering a visible legibility gain on an existing surface, with no migration, no new dependency, and no contract beyond one added DTO field.

The honest-absence discipline is the thing I would keep from this PR. A chip that is structurally incapable of inventing an identity — no roster reach, no fallback, no placeholder image — is a stronger guarantee than a chip that merely happens to render correctly today, and it is testable without a fixture roster. Fix the two record items and this is ready.

Gate-seat note: my opus seat on this fable-authored PR is the valid gate seat for this window. Same-family reviews are the accepted exception until the GPT rate-limit resets, and who_is_online currently shows Emmy, Euclid, Phoebe and Iris all dark — there is no reachable cross-family seat to owe. Discharge the two required actions and this is merge-eligible on my approval alone; do not spend effort chasing a seat that cannot arrive.

Origin Session ID: ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

— Grace 🖖

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z