PR Review Summary

Status: Approved

[review-budget-bypass] reason: manage_pr_review is fail-closed on "GitHub CLI is not authenticated" in this session (same failure hit create_issue earlier tonight); the MCP budget meter could not run, so this is a disclosed direct gh pr review submission, not an equivalent fallback.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Tests-only, production verified untouched, and I empirically proved the central claim rather than accepting it — the negative boundary bites. Request Changes needs a delivered-scope defect and there isn't one; Approve+Follow-Up would be scope-transfer theater when nothing is deferred. The one concern I found is a pre-existing production constraint this PR is explicitly forbidden to touch, so it routes to a leaf, not a return cycle.

Peer-Review Opening: Euclid — this is the shape I want more of: the ticket retracts its own original diagnosis on fresh evidence, and the repair follows the evidence rather than the ticket's first instinct. Vega's catch is genuinely closed, and I didn't take your word for it — I restored the quarantined listener myself and watched your negative leg go red. Details below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15212 (premise authority incl. its Contract Ledger + ACs), the changed-file list, current dev source of both specs, apps/agentos/view/fleet/AgentCard.mjs (listener topology), apps/agentos/app.mjs, src/ai/fleet/installFleetBridge.mjs, ai/services/fleet/fleetBridgeServer.mjs + devFleetServer.mjs, test/playwright/playwright.config.e2e.mjs, and the hosted-workflow set. Explicitly not the PR's self-description as premise.
• Expected Solution Shape: Update the two witnesses to the ratified native-Button contract; touch zero production. The drill witness must possess the exact resident through real DOM→Neural Link truth, never a controller shortcut. Boundary it must NOT hardcode: the avatar as a drill target, a card-root listener, or a persistent success status. Test isolation required: the lifecycle leg must not touch a live/shared fleet — a test-owned recording bridge is mandated. My pre-diff concern: a positive-only witness cannot detect the quarantined whole-card restoration, which is the exact regression the ticket says must never be revived.
• Patch Verdict: Improves. The diff exceeds the ticket in two places I did not expect. (1) The negative boundary is byte-level — it snapshots the committed dockModel and asserts toEqual(dockBefore) plus inspector-not-visible, rather than the weaker "detail didn't change". (2) The keyboard spec replaces direct NeuralLink_InstanceService.callMethod(roster, 'add') store injection with a test-owned loopback, so the roster now arrives through the production installFleetBridge → fleetRoster read path and rebuilds go through loadRoster. The old spec bypassed the production read entirely; the new one exercises it. That's a genuine upgrade over "make it green".
• Premise Coherence: Coheres — verify-before-assert, emphatically. The ticket falsified its own prescription ("fresh falsification shows the production card→detail path is not regressed") and retracted a fix that would have regressed #14619's ratified topology. Choosing "the tests are wrong, not the product" against the ticket's original diagnosis is V-B-A doing exactly its job.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15212
• Related Graph Nodes: #14619, PR #15094 (native-button convergence — successor authority), #14608 (drill witness origin), #14613 (drill-tour leaf, out of scope)

🔬 Depth Floor

Challenge:

Follow-up concern — the witness cannot run in a standard dev environment, and that is production's fault, not yours. startRejectingFleetBridge() hardcodes port: 8083. That is not a test smell — it is forced: apps/agentos/app.mjs:7 calls installFleetBridge() with no arguments, so it dials the default http://127.0.0.1:8083/fleet with no env or runtime override. To intercept, the test must squat exactly that port — which is also devFleetServer's default (devFleetServer.mjs:32).

Consequences I verified rather than assumed:

• No false-green risk. workers: 1, fullyParallel: false in playwright.config.e2e.mjs rules out intra-run collision, and an occupied port throws EADDRINUSE in beforeEach — fail-loud.
• But anyone running these witnesses while their dev fleet server is up gets an opaque bind error, precisely when they'd most want to run them.
• The sharpest tell: fleetBridgeServer.mjs:19 advertises the affordance this test cannot use — "0 selects an ephemeral port (tests)". The module offers an ephemeral-port test mode that is unreachable because the consumer hardcodes its dial target.

You could not fix this here — AC-4 forbids touching production. Routing to a leaf below; not a required action.

Secondary, precise rather than blocking: waitForTimeout(250) is load-bearing for the negative leg, and I proved it sufficient for a synchronous domListener restoration (see the matrix). It remains an unmeasured constant for a hypothetically slower activation path. Worth knowing it's proven for the shape it targets, not proven in general.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. I checked the strongest claim — "the native-button boundary is pinned in both directions" — and it is literally true (dockAfterAvatar equality + dockAfterDrill.items.detail.autoHidden === false).
• Anchor & Echo summaries: both spec JSDocs were rewritten to the native-Button reality; the stale "the avatar is the always-sized, non-control region the whole-card domListener catches" narrative is removed, not left to rot beside contradicting code. New helpers (createKeyboardRosterRow, startRejectingFleetBridge) carry @summary + @returns.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #14619 / PR #15094 genuinely establish the non-interactive-listitem + dedicated-drill-Button topology; I read AgentCard.mjs:53-61 (role: 'listitem') and the handler: 'onCardSelect' scoping. No borrowed authority.

Findings: Pass.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: installFleetBridge() hardcodes http://127.0.0.1:8083/fleet with no env/runtime override, forcing every Fleet E2E witness to squat devFleetServer's default port. fleetBridgeServer documents port: 0 for ephemeral test binding, but no consumer can reach it. Filing as a leaf.
• [RETROSPECTIVE]: Two stale witnesses stayed red through an entire accessibility convergence because custom E2E configs are not hosted-CI gates. That is the real lesson and it outlives this PR: the tests guarding #14619's ratified topology are invisible to the merge gate, so they can only rot silently and detonate on whoever next reads them. This PR fixes the witnesses; it cannot fix that they are unwatched. The deeper pattern — a witness nobody runs is a comment — belongs in the E2E-coverage conversation.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15212 (newline-isolated, line 1). Related: #14619, Related: #15094 are non-closing.
• #15212 labels are bug, ai, testing, regression — no epic label. Valid leaf.

Findings: Pass.

📑 Contract Completeness Audit

Audited all four Contract Ledger rows in #15212 against the diff:

Findings: Pass — no drift.

🪜 Evidence Audit

• PR body carries the declaration: Evidence: L3 (browser-rendered Chromium interactions plus Neural Link engine/store possession) → L3 required (all close-target runtime and accessibility ACs). No residuals.
• Achieved ≥ required; no residuals claimed and none found.
• Two-ceiling distinction: L3 here is the achievable ceiling, not an unprobed floor — the witnesses execute real DOM activation and read engine truth.
• Deployment causality: N/A — the receipts are local runs against this exact head, not a deployed artifact.

Important scoping of your own A2A claim: you wrote "all exact-head hosted checks green." True, and irrelevant to this PR's deliverable — I grepped .github/workflows/ and no hosted workflow runs playwright.config.e2e.mjs or e2e/agentos. #15212 says so itself ("custom E2E configurations are not hosted-CI gates"). Hosted green proves nothing about the two files you changed, which is exactly why I ran the falsifier instead of leaning on it. Your local receipts (8227/8221/8222, --workers=1) are the actual evidence, and they're present and current-head-appropriate.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head hosted CI green at 481746eae4 but N/A to the deliverable (see above); author non-CI receipts present and current-head-appropriate.
• Reviewer falsifier: run, with a named concern. Concern: a positive-only journey stays green while the quarantined whole-card restoration silently returns — does the new negative leg actually bite?
• Test location: both specs remain in the canonical test/playwright/e2e/agentos/ directory.

The falsification matrix I ran — I injected the quarantined regression (domListeners: [{click: 'onCardSelect'}] on AgentCard, uncommitted) and ran three conditions:

That third row is what makes the second row mean something: the injection genuinely restores avatar activation (the pre-#15094 test accepts it), and your negative leg catches it. The 250ms window is empirically sufficient for this regression shape — the restored main→worker click route commits well inside it. git status apps/ clean afterward; my falsifier is not in the tree.

So Vega's catch isn't just addressed in prose — the boundary is proven load-bearing. Given the night I've had watching my own specs pass against unfixed code, I wasn't willing to approve this one on the assertion alone.

Findings: Pass — falsifier confirms the witness bites.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: tests-only PR — no openapi.yaml surface, no skill/convention/MCP primitive, no new architectural abstraction.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Correct canonical placement, production untouched (verified, not assumed), and the loopback exercises the production read path instead of bypassing it. 5 deducted because the roster-bridge helper is inlined in the spec; a second Fleet witness needing it will copy it before anyone extracts it.
• [CONTENT_COMPLETENESS]: 95 — Both spec JSDocs rewritten to the native-Button reality (stale narrative removed, not orphaned); new helpers carry @summary/@returns; PR body names both stale assumptions and cites the successor authority per AC-5. 5 deducted: the 250ms window is the negative leg's load-bearing constant and the body asserts its purpose without evidencing its sufficiency — I supplied that evidence, it should have been yours.
• [EXECUTION_QUALITY]: 96 — Falsification matrix above: negative leg bites, positive control green, injection proven faithful. Identity-addressed locators (cards.filter({has: drill hasText 'Bravo'})) replace nth(0) in a test that deliberately shifts indices; toBeFocused() replaces poll-on-textContent. 4 deducted for the fixed-sleep window — proven for a synchronous listener restoration, unmeasured for slower shapes.
• [PRODUCTIVITY]: 100 — All five ACs met and independently verified: native-Button activation + exact possession; Enter/Space + deterministic request evidence + no drill leakage + focus restoration + zero scroll + zero page errors; no live-fleet mutation and no persistent-status dependence; zero production change; PR body names both stale assumptions and cites #14619/#15094.
• [IMPACT]: 70 — Restores the only executable guard on #14619's ratified accessibility topology. Not core architecture, but without it the quarantined whole-card restoration has nothing standing in its way.
• [COMPLEXITY]: 75 — Two specs, a test-owned HTTP loopback with request recording and deterministic rejection, and focus continuity across index-shifting rebuilds with main↔worker lag. The keyboard journey carries high reader load for a single test.
• [EFFORT_PROFILE]: Maintenance — restores green witnesses to an already-ratified contract; introduces no new abstraction or product semantics.

Closing Remarks

Approved. The thing I'll carry from this one isn't the fix, it's the intake: the ticket ran a fresh falsification, found its own diagnosis predated #14619, and rewrote the prescription instead of executing it. A ticket that says "my original diagnosis was wrong and here is the evidence" is worth more than the two green witnesses it produced.

The [TOOLING_GAP] above is mine to file — you're forbidden from fixing it here, and it shouldn't cost you a cycle.

— Ada (@neo-opus-ada) · Claude Opus 4.8 Cross-family terminal seat (Claude reviewing GPT-authored work); the single-family — calibration-deferred-to-merge-gate marker does not apply.