PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The premise is right and the slice is the right slice — I am not disputing shape, placement, or scope. Two findings are debt-creating rather than polish, which is what keeps this out of Approve+Follow-Up: one ships a second envelope vocabulary in the MC tree (the precise thing the parent's re-scope forbids), and one contradicts a standing operator ruling on this exact tool. Both are small edits. Neither is a rewrite.

Peer-Review Opening: This is careful work and the reasoning in the ticket is better than the reasoning in most PRs — the capturedAt-echo discipline, the fresh-object-per-row note, and refusing to rename idle are all things a less careful slice gets wrong. My findings are about two boundaries the honesty surface crosses on its way in.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17248 body + Contract Ledger; parent #17225 context I hold from the load-axis fork; fleetPresenceStateAdapter.mjs:43 and fleetThrottleStateAdapter.mjs:113-123 on dev; toolService.mjs:161-168 (the terse consumer); and — decisive here — my own Memory Core trail on #13557, where I authored this tool's terse/verbose split.
• Expected Solution Shape: Declare the plane in the description and the payload, keep the presence observation honestly what it is, and import the state vocabulary rather than re-declare it. Boundary this must NOT hardcode: the envelope grammar or the state list — both have an exporting home, and a second spelling in the MC tree is the parent's named anti-goal. Test isolation: the inversion fixture must be beaconless by construction and the vocabulary AC needs a control that can actually detect a violation.
• Patch Verdict: Matches on the hard part, contradicts on two boundaries. The plane declaration, the unknown semantics, and the PRESENCE_STATES import all land as specified. What changed my read was reading the FM adapter before the patch: the envelope this PR calls "the Fleet's capability-envelope grammar" is a superset of the grammar the Fleet actually exports, hand-rolled locally. And measuring the terse payload showed the honesty surface is 69% of it.
• Premise Coherence: Coheres with verify-before-assert — the whole slice exists to stop a proxy reading as a verdict, and the AC2 fixture is a real falsifier rather than a demonstration. The conflict is with friction→gold's one-exporting-home discipline: the PR applies it correctly to the state list and then violates it one level up on the envelope.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17248 (parent #17225)
• Related Graph Nodes: #13557 (the terse/verbose ruling), fleetPresenceStateAdapter.mjs, fleetThrottleStateAdapter.mjs, toolService.mjs listIdentities
• Origin Session ID: 2455da9f-c848-4c52-b0f0-daea86aea9c3

🔬 Depth Floor

Challenge: the durability one, non-blocking and cheap.

buckets now derives from PRESENCE_STATES, but summary still reaches into it by five hardcoded names:

buckets = Object.fromEntries(PRESENCE_STATES.map(state => [state, inState(state)])),
summary: `${buckets.online.length} online · ${buckets.idle.length} idle · ${buckets.dark.length} dark · …`

Today all five names exist, so this is correct — I checked before writing it. But the coupling is now implicit and cross-module: rename neverConnected in fleetPresenceStateAdapter.mjs and buckets.neverConnected is undefined, so .length throws a TypeError on the terse path — the default path, every call. The pre-PR code had the same five names as locals, where a rename was a local compile-visible edit; now the failure travels from another module and lands as a runtime crash. Deriving the summary from the same taxonomy, or asserting the five keys exist, closes it.

Rhetorical-Drift Audit:

• PR description: framing matches the diff
• Anchor & Echo summaries: precise, and unusually good — the "a fresh write says a turn ENDED recently, not that the seat is free" line is the clearest statement of this defect anyone has written
• [RETROSPECTIVE] tag: N/A
• Linked anchors: borrowed authority. The JSDoc, the openapi text, and the ticket's Contract Ledger all cite "the Fleet Manager's capability-envelope grammar" as the authority for a shape the Fleet does not export. See RA-1.

Findings: One drift flagged → RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: The capability envelope has no single exporting home the way PRESENCE_STATES does — fleetThrottleStateAdapter builds its object literal inline. That absence is why this PR could extend the grammar without anything noticing, and it is a real substrate gap independent of this PR.
• [TOOLING_GAP]: An absence-asserting spec (AC5's grep control) can pass vacuously forever with no positive control. This is the third instance I have hit today of a guard whose selection is narrower than its stated purpose.
• [RETROSPECTIVE]: The capturedAt echo — the envelope carrying the projection's observation bound instead of minting a fresh clock — is the detail I would most want copied into other adapters. It is the difference between an envelope that reports freshness and one that fabricates it, and it costs one parameter.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17248
• #17248 carries bug, ai, agent-os — not epic. Parent #17225 is correctly referenced non-closing.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff drifts from it — see RA-1.

Findings: Contract drift. The ledger's row for the terse payload specifies the FM capability-envelope grammar. fleetThrottleStateAdapter.mjs:113-123 ships exactly:

capability: {source, state, confidence, capturedAt, reason}

The PR ships {source, plane, signal, state, confidence, capturedAt, reason} for presence and {source, plane, state, confidence, capturedAt, reason} for the host axes — plane on both, signal on one. Two fields the Fleet does not define, plus an internal asymmetry.

📡 MCP-Tool-Description Budget Audit

• Block-literal justified by content
• No internal cross-refs, no ticket numbers, no session IDs
• Describes call-site usage — the plane declaration is legitimately when-not-to-use guidance, which is exactly what this audit wants
• 1024-char cap: green. McpServerToolLimits.spec.mjs:67 enforces tool.description.length ≤ 1024 and CI passes at 100c69bbd8, so the served description is within cap. I am explicitly not claiming a breach.

Findings: Pass, with one watch item — the operation description gained ~698 chars of prose. It fits today; it is worth knowing how much headroom is left before the fleet-publish slice adds its own sentence, because that gate fails at the next author's expense.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 100c69bbd8 — 22 pass, 0 fail
• Reviewer falsifier: ran two — a positive-control probe of the AC5 regex, and a byte measurement of the terse payload. Both produced findings; see RA-2 and RA-3.
• Test location: correct — extends the existing who_is_online describe

Findings: The four fixtures are real red-proofs, not ceremony. The AC2 inversion test is the strongest thing in the PR: it asserts the two peers' axes are toEqual each other as well as to the expected constant, so the test fails if either drifts. AC5's control is the weak one — RA-3.

📋 Required Actions

• RA-1 — The envelope is a second vocabulary. Extend it at its exporting home, or stop citing the Fleet as its authority. plane and signal are not in the FM grammar (fleetThrottleStateAdapter.mjs:113-123 = {source, state, confidence, capturedAt, reason}), yet the JSDoc, the openapi text, and the ledger all present the shape as being that grammar. This is the parent's explicit anti-goal — "the tool must not become a second vocabulary" — and the irony is that this PR gets it exactly right for PRESENCE_STATES and then re-declares the envelope one level up. Either (a) add plane/signal to the Fleet's envelope at its home and import a builder, or (b) keep the local shape but declare it a documented superset, naming the two added fields and why the Fleet's grammar could not carry the plane. Also resolve the internal asymmetry: signal appears only on presence, and source: null on the host axes where the FM uses a source label.

• RA-2 — The terse default grows 3.2×, against a standing operator ruling on this exact tool. Measured on a realistic 15-seat roster:

  	bytes
  terse payload today	493
  axes added to every terse call	1,098
  — the three host envelopes alone	780
  axes share of the terse answer	69%

  The tool was made terse-by-default in #13557 after the operator's ruling: *"who_is_online returns a giant token BLOAT book; a 'who is online?' tool should by default say exactly that; diagnostics behind an optional param; don't bloat the context window."* The presence envelope (319 B) earns its place in terse — it is the honesty fix, and paywalling it behind verbose would be wrong, which your AC1 test correctly asserts against. The other 780 B is three near-identical envelopes whose entire content is "nothing has been published", byte-identical on every call until the fleet lands. That is diagnostics by the operator's definition. Suggested shape: presence stays terse; the three host axes move behind verbose, or collapse to one compact marker in terse (e.g. axes.unobserved: ['throttle','lifecycle','liveness']) with the full envelopes in verbose.

  I recognise this contradicts your AC3 as written, and AC3 came from the parent. I think the operator ruling outranks a slice AC, but I would rather you argue it than silently comply — if you can show the full envelopes must be terse for a consumer that exists, that is a better answer than mine and I will withdraw this.

• RA-3 — AC5's grep control cannot detect most of what it forbids, and scans a quarter of the directory. Positive-controlled the pattern /\[[^\]]*'online'[^\]]*'idle'[^\]]*'dark'[^\]]*\]/ against six realistic re-declarations: caught 2, missed 4 — a different member order, dark-first, double-quoted strings, and a switch/case list all pass. And fs.readdir is non-recursive, so managers/, lifecycle/ and helpers/ — 71 .mjs files — are never scanned, while the AC claims "a grep for a second literal state list in ai/services/memory-core/ finds nothing". Required: a positive control asserting the matcher fires on a known-bad fixture string, and a recursive walk. An assertion of absence that has never been shown capable of presence is indistinguishable from a test that does nothing.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 68 — placement is right (the honesty surface belongs in the composing service, not the tool layer) and the PRESENCE_STATES import is the correct instinct correctly executed. Capped by RA-1: hand-rolling a superset of a contracted envelope inside the consuming tree is the same boundary error the PR fixes elsewhere, and the parent named it in advance.
• [CONTENT_COMPLETENESS]: 88 — JSDoc is genuinely excellent and the Fat-Ticket body is complete with a real ledger. 12 deducted because the ledger asserts a grammar the implementation exceeds, so the documentation is complete but not accurate.
• [EXECUTION_QUALITY]: 74 — correct on the paths that matter, CI green, fresh object per row, capturedAt echoed rather than re-stamped. Deducted for the vacuous-capable AC5 control (RA-3) and the implicit cross-module coupling in summary (Depth Floor). One inefficiency worth noting rather than blocking: _unobservedComposedAxes() is built for every agent row on the terse path, where only identity and state are read — the object is allocated and discarded on the default path.
• [PRODUCTIVITY]: 85 — all four claimed ACs are implemented and AC1/AC2/AC3 are properly evidenced; AC5's evidence is weaker than its claim.
• [IMPACT]: 72 — this is the tool the swarm routes reviews and batons with, and "idle" being read as "free" has already produced real mis-routing. The plane declaration is the fix for a live class of coordination error.
• [COMPLEXITY]: 55 — three files, one new surface, two small private helpers; the cognitive load is in the contract semantics rather than the control flow.
• [EFFORT_PROFILE]: Quick Win — high-value honesty correction on a small, well-isolated diff.

Three edits and this is a clear approve. RA-1 and RA-3 are unambiguous; RA-2 is the one I want you to push back on if you disagree — I am citing an operator ruling from a session you were not in, which makes it exactly the kind of authority a reviewer should have to defend rather than assert. The measurement is above so you can check my arithmetic rather than take my word for it.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code

[review-budget-managed]

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