PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The delivered scope is correct and the ACs are closed by evidence that CI actually runs. I carry one finding — the e2e's headline assertion is inert — but it is a durability observation about a test, not a delivered-scope defect: the unit layer independently closes the same chain with a real red control, and that layer is in the CI matrix. Request Changes would be spending a round on a one-line test nit while the operator-facing defect sits fixed; Approve+Follow-Up would mint a ticket for something with no day-after-merge counterfactual. Neither fits.

Peer-Review Opening: This is a good fix, and the part I want to name first is the locus. The alarm rendered in the view, and you fixed the generator — because the seed is derived and a hand-edit would have been silently reverted on the next derivation. That is the correct level, and it is one level up from where the symptom was.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17210 body and its ACs; the changed-file list; current dev source of apps/agentos/view/fleet/sourceHealth.mjs (read in full before the diff), apps/agentos/view/fleet/AgentCard.mjs, src/component/Base.mjs; the .github/workflows/test.yml suite matrix; your lane-claim intake comment; and a query_raw_memories sweep of the declared-absence / observation-vs-default decision space.
• Expected Solution Shape: The seed must stop emitting a present-but-unreadable fact. Two honest routes: omit sources.roster entirely (the contract's absent-key branch already returns the calm not-wired), or emit the contract's declared-absence shape. The boundary it must NOT hardcode is the producer literal — that belongs to the vocabulary twin. Test isolation should prove the shipped seed reads calm through the real contract functions, not through a shape guess.
• Patch Verdict: Improves on my expected shape, and I want to be explicit that I came in leaning the other way. My §0 premise was that omitting the key was the more honest route, because a build-time artifact asserting state: 'not-wired' looked like storing a runtime fact — the failure Grace's taxonomy names as "a state describing the observer's silence while claiming to describe the world." Reading sourceHealth.mjs changed my mind on specific evidence: normalizeFleetSourceFact passes FLEET_SOURCE_BY_KEY[sourceKey] as expectedSource, so your explicit fact is validated against the canonical producer literal. An omitted key is calm forever and can never detect vocabulary drift; your explicit fact goes invalid and loudly restores the alarms if the literal ever moves. Combined with importing FLEET_COCKPIT_SOURCES in the generator rather than hardcoding 'fleet:listAgents', the route you took is strictly more falsifiable than the one I expected. That is the reverse of the concern I arrived with.
• Premise Coherence: Coheres — verify-before-assert. The contract's whole design is that rejected evidence must never normalize into a green surface, and the fix respects that by making the seed satisfy the contract rather than by widening the contract to accept the seed. Pinning that a bare string still classifies invalid is the load-bearing half: you fixed the data and pinned the validator you did not touch.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17210
• Related Graph Nodes: #17209 (sibling from the same escalation) · #13015 (FM umbrella) · #16744 · #15621 / #15623 (generator lineage) · PR #16034 (prior is-sample selector-scoping defect in this same view family)
• Origin Session ID: 7d0477c6-e112-4903-a79a-42f06095863e

🔬 Depth Floor

Challenge:

The e2e's headline assertion cannot fail. This is the finding, and it is non-blocking for the reason given under Required Actions.

await expect(page.locator('.fm-card-strip:visible')).toHaveCount(0)

src/component/Base.mjs:146 defaults hideMode_ to 'removeDom', and the strip declares no override (AgentCard.mjs:261). AgentCard.mjs:467 then sets hidden: summary.level === 'ok'. So in the calm state the strip's vdom carries removeDom and the node is not in the DOM at all — .fm-card-strip matches zero elements before :visible is even applied.

toHaveCount(0) is therefore satisfied identically by "the fix works", "the class was renamed", and "no card rendered a strip for any reason". A non-event assertion cannot be validated by its own history — never failing and never testing look the same from the outside. The :visible filter also encodes a belief that the nodes persist and are merely hidden, which removeDom makes false.

Worth noting this view family has been bitten here before: PR #16034's retro records label selectors scoped to .is-sample that matched nothing once the cockpit promoted to live, reporting null — "the instrument was wired exclusively to the state we do not want to ship." Same shape, one layer over.

Two things that are genuinely sound and that I checked rather than assumed:

1. Your topology gate is real. expect(page.locator('.fm-fleet-head')).toHaveClass(/is-sample/) and the fm-fleet-stale → 'static roster' text are positive assertions that fail loudly if the run lands in the wrong topology. That is the discipline #16034 was written about, and it is present here.
2. The unit layer carries the red control the e2e lacks. Your PR body reports 10/10 rows classifying bad pre-fix through the real summarizeAnsweredAbnormal and 0/10 after. That is a sensitivity proof, on the real consumer, and it is why the finding above is not blocking: the chain closing AC-2 runs unit-side and does not depend on the inert assertion.

Follow-up concern (context, not a miss): the e2e suite is not in CI. .github/workflows/test.yml runs npm run test-${{ matrix.suite }} over exactly integration-unified, integration-parity, unit, components (lines 281/299/301/303) — no e2e anywhere in .github/workflows/. I ran that grep with a positive control first, because my initial pattern matched nothing for unit either and would have produced a false absence claim. Your Evidence line already says "local Chrome e2e on this checkout", so this is not an overclaim on your part — I am naming the consequence: this test will never gate a regression and can rot without anyone noticing, which is also why the inert assertion inside it matters less than it would in a CI-gated file.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "Zero view changes were needed" is verified — the diff touches no view file, and AgentCard.mjs:467 already hides at level: 'ok'.
• Anchor & Echo: the generator JSDoc addition explains why the bare string is rejected and what declared absence buys, in contract terminology, with no snapshot anchors.
• [RETROSPECTIVE]: N/A — none claimed.
• Linked anchors: lint-fleet-vocabulary-parity genuinely binds the Body-side twin; FLEET_COCKPIT_SOURCES.roster is the literal expectedSource validates against.

Findings: Pass. One phrase I'd tighten rather than flag: "zero .fm-card-strip:visible across all cards" in the Test Evidence reads as an observation of hidden-but-present strips; the mechanical truth is that the nodes are absent.

🧠 Graph Ingestion Notes

• [KB_GAP]: The hidden → hideMode: 'removeDom' → node absent from DOM chain is the kind of framework fact that silently converts an absence assertion into a tautology. Anyone writing "assert the thing is not shown" against a Neo component needs it, and it is not obvious from the test side.
• [TOOLING_GAP]: The e2e suite exists (test-e2e, playwright.config.e2e.mjs) with no CI job. Tests land there, pass locally once, and are never run again. That is a substrate observation, not this PR's debt.
• [RETROSPECTIVE]: The generator-not-artifact instinct is the transferable win. The alarm rendered in the view, the bad data lived in JSON, and the actual defect was in the script that writes the JSON — fixing either of the first two would have been reverted or cosmetic. Second: emitting an explicit declared-absence fact rather than omitting the key buys drift detection, because expectedSource validates the literal. The lazier route would have been calm forever and blind forever.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17210 — single, standalone, newline-isolated. No Closes / Fixes, no comma-separated targets.
• #17210 labels are bug, design, ai — not epic-labeled. Delivered leaf.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at e3118dfa31593fe69c8b979d5d7a1cb2267ccee3 — unit, components, integration-unified, integration-parity, lint ×8, lint-pr-body, CodeQL all pass. Author per-surface receipts present and current-head-appropriate, including the pre/post red control and deriveFleetRoster.mjs --check for the anti-hand-paint guard.
• Reviewer falsifier: ran one named concern — "can the e2e's zero-strip assertion fail?" Traced hideMode_ default (src/component/Base.mjs:146) → strip construction (AgentCard.mjs:261, no override) → hidden: summary.level === 'ok' (AgentCard.mjs:467). Result: it cannot; the node is absent, not hidden. Reported above rather than blocking, since the unit layer closes the same AC.
• Test location: added unit pin sits beside the generator's existing spec; the e2e pin sits in the existing cockpit e2e file. Both correct.

Findings: Pass, with the inert-assertion note carried as a non-blocking item.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed contract surface is introduced or modified (the seed satisfies an existing contract rather than changing it), close-target ACs are covered by unit tests plus a declared local render, no OpenAPI surface is touched, and no skill / convention / architectural primitive is added.

📋 Required Actions

No required actions — eligible for human merge.

One optional tightening, entirely your call and safe to land as-is without it: drop :visible and assert .fm-card-strip count 0 with a one-line comment naming removeDom as the reason the node is absent — that at least stops the selector implying a persistence it does not have. If you want the assertion to actually bite, the positive control would be asserting .fm-agent-card has the full roster count in the same breath, so a collapsed card tree can no longer read as "no alarms".

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — fixed at the generator rather than the generated artifact, and imported FLEET_COCKPIT_SOURCES instead of hardcoding the producer literal, so the emitted fact stays bound to the vocabulary twin expectedSource checks it against. 8 deducted for the reason string being duplicated across all ten rows while never rendering on the calm path — carried data with no consumer, defensible as provenance but it is weight.
• [CONTENT_COMPLETENESS]: 92 — the generator JSDoc addition explains the mechanism and why declared absence is the one calm present shape, which is the reasoning a future reader needs. PR body carries every anchor with an honest, correctly-scoped Evidence: line. 8 deducted for the Test Evidence phrasing that describes absent nodes as not-visible ones.
• [EXECUTION_QUALITY]: 84 — the unit pin asserts through the real contract functions rather than a shape guess, and the pre/post red control is a genuine sensitivity proof. 16 deducted for the e2e headline assertion being structurally incapable of failing.
• [PRODUCTIVITY]: 95 — all four ACs met; AC-1 took the first offered route with the rejected alternative reasoned rather than skipped, AC-3's "the validator stays correct" is pinned explicitly, and AC-4's offline topology is gated rather than assumed.
• [IMPACT]: 62 — removes ten false alarms from the first-run view, which is the first thing a new operator sees, but the blast radius is one view's presentation and no runtime behavior changes.
• [COMPLEXITY]: 40 — four files and a mechanically small diff, but it required reading a three-way honesty contract correctly and locating the fix a level above the symptom.
• [EFFORT_PROFILE]: Quick Win — high operator-facing return on a contained, fully-verified change with no runtime surface moved.

The thing I'd keep from this one: you fixed the class rather than the ten instances, and you pinned the validator you deliberately did not change. The finding I raised is about a test that cannot fail, in a suite that does not run — which is exactly the corner where an inert assertion survives longest.

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