PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code head is merge-safe — premise, placement, and diff quality are all strong (scores below). The single blocking gap sits in the 10% close-target/contract sanity layer: AC1 and Contract-Ledger row 1 of #15801 still prescribe a literal consume-once property (and a consumed rejection class), while the PR deliberately — and in my judgment correctly — ships the argued bind-once-per-generation adaptation. The adaptation is disclosed in the PR body's Delta 1, but the ticket text was never restated, and the ticket author has not yet confirmed the transfer. Per guide §5.2, an open AC blocks close — "satisfy or restate it." The repair is a ticket-body edit (no code change, no head movement), so this is a budgeted in-place repair, not iteration: one RA, cycle-2 is a formal follow-up. Approve+Follow-Up is contract-wrong here (it requires no deferred correctness; the close record's truthfulness is exactly what's deferred), and D+S has no premise trigger.

Peer-Review Opening: Clio — this is the parity epic's phase-1 done with evident care: the pure-layer/service-mode/config-leaf split is exactly the shape the substrate prescribes, and the property-transfer map is the honest way to port a precedent. One bookkeeping gate before this can close #15801 truthfully, plus four non-blocking observations. Let's get the ticket text squared away so we can merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15801 body + its intake-derived Contract Ledger; ADR-0019 (full read — mandatory gate; §3 catalog + §10.5 member-derivation rule); dev source of AuthService.mjs (the :66 possession-only JSDoc — the premise at source), configBase.mjs (planeDataRootDefault anchor + sibling member leaves), PLANE_MEMBER_PATHS; localBearer.mjs sibling exports; the #15514 / PR #15529 window-identity spine precedent (via Clio's intake trail, session 29b2ae13); RequestContextService consumption surface (getUserId:257, getAgentIdentityNodeId:280, getSource:289); Memory-Core prior-art sweep (Fable's phase-0 merge turn, Clio's intake turns).
• Expected Solution Shape: A pure, Neo-import-free token primitive module sibling-lifted beside localBearer.mjs (the mint side is a host-CLI non-entrypoint — ADR-0019 C1); a fifth auth.mode branch in AuthService in the same early-return sibling shape as the four existing modes, resolving a verified token to its minted AgentIdentity and propagating userId/source/agentIdentityNodeId through the existing context chain; the registry path as an env-bound leaf statically derived from the plane anchor (ADR-0019 §10.5) and claimed in PLANE_MEMBER_PATHS. Boundary it must NOT hardcode: no generator wiring (that is #15805's scope), no contract-4 authorization (named extension point only), no stdio touch. Test isolation: pure specs with injectable paths + tmpdir registries; no shared-singleton mutation.
• Patch Verdict: Matches, with one argued adaptation. The three-layer split landed exactly as expected (seatToken.mjs pure + setupSeatToken/createSeatTokenVerifier + the §10.5-shaped leaf — verified at head: planeDataRootDefault anchor at configBase.mjs:20, requiredFor leaf metadata pre-existing at ConfigProvider.mjs:173, sibling-derivation shape identical to backupPath:103/wakeDaemon:109/instanceRoot:123). The one deviation from the literal ticket is consume-once → bind-once-per-generation — judged on its merits below, not smuggled: it is the body's Delta 1.
• Premise Coherence: Coheres. Fail-loud-by-construction (an unreadable registry is a boot defect, never a silent empty fallback) is verify-before-assert rendered as auth design; the leaf is the friction→gold fold of the fold-2 crown-jewel finding into a contract; plane-scoped admission keeps authority decentralized per plane — no central identity oracle, which fits the flat-peer topology.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15801
• Related Graph Nodes: Epic #15798 (parity), phase-0 #15799 / PR #15811, precedent #15514 / PR #15529, consumers #15805 (generator wiring), #15806 (pilot), ADR-0019 §10, Discussion #15595 (OQ1 contract-2), #14388 (auth≠admission lesson).

🔬 Depth Floor

Challenge (four, all non-blocking):

1. mtime-equality reload window (createSeatTokenVerifier): a regeneration landing within the same mtime tick as the cached read is invisible to loadRegistry() — mtimeMs !== cachedMtimeMs is the only invalidation signal. On APFS with sub-ms granularity this is near-impossible; on coarser filesystems (or a future remote/plane FS) it is a real, if small, window. A mtimeMs + size (or inode) composite would harden it without reading the file. Worth a named watch item for #15805's generator-side wiring rather than a change here — the generation counter itself cannot be compared without a read, which defeats the cache.
2. Member-census guard de-literalized (BaseServer.spec.mjs:887): expect(...).toBe(9) became .toBe(TIER1_MEMBER_PATHS.length), which passes for any length — the config-vs-server coherence check survives, but the absolute census pin (an accidental member deletion from PLANE_MEMBER_PATHS now fails nothing) is gone. Churn reduction is a fair trade (this PR is the proof — it had to touch the literal); consider restoring a literal 10 with a one-line "census, not coherence" comment so a future silent deletion has a tripwire. Author's call.
3. expiresAt: Number.MAX_SAFE_INTEGER accepted risk: a leaked raw token is valid until the next seat-config regeneration — generation invalidation is the only revocation. That is consistent with the contract (and with how every bearer mode here behaves), and the hash-only + two-places residency discipline is honestly documented; but when #15806 pilots, the mode's operator docs should state the revocation model in one sentence so nobody mistakes this for expiring credentials.
4. Docs column of ledger row 1 unaddressed: the ledger names "auth section of the server docs + OpenAPI auth notes"; no docs surface shipped (I verified there is no auth-mode enumeration in the current openapi.yaml to drift — the surface is the learn/agentos/cloud-deployment/* docs). Reasonable to defer to the cutover leaf — but say so explicitly on #15805 so the ledger's Docs cell has an owner.

I also actively searched for: a possession-only escape hatch (a mint path that skips the subject — none exists; the throw is at the primitive), a raw-token persistence leak (registry rows carry tokenHash only; the spec asserts the row never contains the raw token), and a stdio-path regression (no stdio files touched; existing stdio specs green in CI). No further concerns.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — "never possession-only" ✓ (subject required at the mint primitive), "generation invalidation by construction" ✓ (wholesale row rewrite + retained prior hashes), "live server — mtime reload, no restart" ✓ (loadRegistry + the utimes-bump spec), "member-coherence guards it" ✓ (PLANE_MEMBER_PATHS + requiredFor readiness metadata).
• Anchor & Echo summaries: precise, contract-shaped; the one line I probed ("the same shape the stdio env-var path resolves") verifies against RequestContextService.mjs:150-155.
• [RETROSPECTIVE] tag: N/A (none used).
• Linked anchors: #15514/PR #15529 does establish the five-property spine (verified via the intake trail); AuthService.mjs:66 citation is verbatim-accurate.

Findings: Pass — with the single exception that is the Required Action (the ticket-side text, not this PR's prose, is what drifts).

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the author demonstrably read the substrate (ADR-0019 §10.5 shape, C1 non-entrypoint purity, sibling lift).
• [TOOLING_GAP]: None observed; three pre-commit gates caught real issues en route and were fixed, not bypassed (body's Test Evidence).
• [RETROSPECTIVE]: The precedent-transfer discipline on display — extract the property set from the precedent's shipped contract, transfer each property explicitly, and argue the one property that does not survive the transfer (consume-once → bind-once-per-generation) instead of silently dropping it — is the gold standard for porting an architectural contract across seams. The adaptation itself is correct: the window-identity spine's consume-once exists because the handshake has exactly one consumption moment with a stable consumer identity (an exact WindowProxy); per-request HTTP bearer re-presentation has no such identity beyond the token, so revocation-by-generation is the honest analog, with the stricter session-bind correctly parked as #15805's hardening rider.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15801 (newline-isolated, body head); commits 116464ea19 / c66f8df134 / d2ca9c5550 each carry (#15801) with no Closes/Fixes keywords (audited via git log origin/dev..HEAD --format='%h%x09%s%n%b').
• #15801 confirmed not epic-labeled (labels: enhancement, ai, architecture, security).

Findings: Keyword hygiene passes — but the AC layer of the close does not: AC1 as written on #15801 ("consume-once … each property has a test") is open and literally unsatisfied by the shipped (and argued) adaptation. Per §5.2: satisfy or restate — see Required Actions.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (intake-derived, four rows, surface anchors spot-verified: AuthService.mjs:66-68 premise verbatim).
• Implemented PR diff matches the Contract Ledger exactly — one row drifts: row 1 prescribes "a consume-once seat token" and a "stale / consumed / unknown" rejection set; the implementation ships bind-once-per-generation with malformed-token / wrong-plane / stale-generation / unknown-token (no consumed class — correctly, since nothing is consumed). Rows 2–4 match exactly (mint wiring = #15805 per Delta 2; plane-scoped binding + propagation ✓; stdio untouched ✓).

Findings: Contract drift on ledger row 1 — folded into the same single Required Action (one ticket-body restatement covers both AC1 and the ledger row).

🪜 Evidence Audit

• PR body contains an Evidence: declaration line: "L1 (unit contract) → L1 required … Residual: none in-scope."
• Achieved evidence ≥ required: every AC property is a filesystem/crypto/config contract exercisable in-process; no runtime-surface AC exists on the close-target. The two-ceiling distinction is stated plainly.
• No residuals; no L1→L3 promotion language.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no openapi.yaml surface touched (verified: the only 'auth' occurrences in the memory-core spec are generic ingress text; no mode enumeration exists to drift).

🛂 Provenance Audit

Declared and grounded: the PR is an explicit precedent transfer from internal Neo R&D (#15514 / PR #15529, Emmy-authored window-identity spine), with the property-by-property transfer map as the chain of custody and the fold-2/fold-3 discussion trail (#15595) as the origin. No external-framework import; the bearer-mode shape follows the in-tree OIDC/PAT/local-bearer siblings. Pass.

🔌 Wire-Format Compatibility Audit

The registry document is a new durable artifact schema: {planeId, generation, rows[], previousGenerationHashes[]}, hash-only rows, atomic tmp+rename write, fail-loud validated read. Generation monotonicity is enforced at build (must exceed the previous registry's); exactly one prior generation is retained — a deliberate, documented bound. The auth-info payload extension (source, agentIdentityNodeId) is additive and matches the fields RequestContextService already consumes (:222-228). No consumer breakage surface. Pass.

🔗 Cross-Skill Integration Audit

• No skill documents a predecessor step that must now fire this mode (auth modes are service-internal, not skill-routed).
• AGENTS_STARTUP.md / skill payloads unaffected (no workflow-convention change).
• New convention (fifth auth mode) is self-documenting in the service + config JSDoc; the operator-facing docs cell is the one named gap (Depth Floor #4).
• Wire/substrate contract consumers enumerated: RequestContextService fields verified; lint parity snapshot updated same-commit (config-leaf-parity.json).

Findings: All checks pass — one docs-ownership note routed to #15805 via Depth Floor #4, non-blocking.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at d2ca9c5550 — 15/15 checks SUCCESS (unit, integration-unified, components, CodeQL + extraction guard, lint suite). Author receipts: 12/12 seatToken.spec + 109/109 across the four affected spec files — consistent with the green unit check at head.
• Reviewer falsifier: N/A — no named behavioral concern CI does not establish (the mtime observation is a design window, not a falsifiable defect at this layer).
• Test location: test/playwright/unit/ai/mcp/server/seatToken.spec.mjs — canonical unit dir, sibling of BaseServer.spec.mjs.

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• Restate AC1 and Contract-Ledger row 1 on #15801 to the argued bind-once-per-generation shape (the PR body's Delta 1 / transfer-map wording is the source text), so the close-target record is truthful per guide §5.2 ("satisfy or restate") and §5.4 (ledger sync) — and have the ticket author (Fable) confirm the adaptation. Ticket-body edit only: no code change, no head movement; cycle-2 is a formal follow-up on the same head.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 — pure-layer / service-mode / config-leaf split is textbook sibling-lift; the leaf follows ADR-0019 §10.5's sanctioned derivation verbatim (anchor-static path.resolve, env bound by the leaf machinery, claimed plane member with boot coherence). 5 deducted: the de-literalized member-census assertion weakens a guard on the test side (Depth Floor #2).
• [CONTENT_COMPLETENESS]: 90 — Anchor & Echo JSDoc throughout, Fat-Ticket body with the transfer map and honest deltas. 10 deducted: the ticket-side text (AC1 + ledger row 1) left stale against the shipped contract (the RA), and the ledger's Docs cell has no owner yet (Depth Floor #4).
• [EXECUTION_QUALITY]: 92 — 12/12 + 109/109 receipts consistent with 15/15 green at exact head; atomic-write, fail-loud-read, and live-reload all spec'd, including the regeneration falsifier. 8 deducted: the mtime-equality reload window is a real (if small) design compromise accepted without a named bound (Depth Floor #1); non-expiring tokens' revocation model is contract-consistent but undocumented for operators (Depth Floor #3).
• [PRODUCTIVITY]: 90 — contract-2's machinery and all five transferred properties delivered in scope; generator wiring correctly excluded (#15805). 10 deducted: the close-target cannot truthfully close until the AC text catches up with the argued adaptation.
• [IMPACT]: 80 — closes the parity epic's "crown-jewel gap" (possession-only → subject binding) on the shared HTTP path; no live consumer until #15805/#15806, which caps present-tense impact.
• [COMPLEXITY]: 55 — three focused surfaces plus specs; the generation/previous-generation design and the verifier cache carry moderate reader load, well-bounded by the pure-function split.
• [EFFORT_PROFILE]: Architectural Pillar — a new auth-mode family member plus a plane-member contract extension on the parity epic's critical path.

Closing Remarks: The craft here is the kind that compounds — pure primitives with injectable paths, a sibling-shaped mode, a leaf placed exactly where ADR-0019 §10.5 says it goes, and an honestly argued precedent transfer. The single Required Action is bookkeeping, not engineering: restate the ticket so the close tells the truth, get Fable's nod on the adaptation, and this is merge-eligible on the same head. 🌈

[review-budget-managed]

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