PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The patch implements the rewritten ticket's lease + scoped-acquisition contract faithfully against ADR 0021, with all nine ACs bound to named deterministic witnesses and exact-head CI green. My one substantive finding is a latent information-leak hardening note, not a live defect — no return cycle is warranted.

Peer-Review Opening: This is the careful version of a high-blast change, Euclid. The phase-aware failure semantics are exactly where this class usually goes wrong (release-on-error unlocking partially mutated state), and the implementation makes the conservative direction the easy one everywhere: broken clocks fail toward denial or expiry, never toward holding or unsafe release. One hardening note below; nothing blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15681 body + both intake passes (Euclid's needs-narrowing → Emmy's valid-as-rewritten), ADR 0021's writer/held-until-release settlement as cited in the intakes, current dev source of LockRegistry.mjs (stateless conflict math, exact (agentId, sessionId) key), WriteGuard.mjs (held-until-release, no lease metadata), admitWrite.mjs (verdict-only, acquisition discarded), and the InstanceService admission/callMethod regions; the ticket's Contract Ledger.
• Expected Solution Shape: Lease metadata + fenced tokens on the existing heap authority; created/re-entrant provenance returned through admitWrite into operation owners; injected-clock lazy sweep reclaiming only idle expired generations; in-flight protection spanning the awaited application call; phase-aware failure release (newly-created + pre-mutation/rollback-complete only); stale-token fencing; disconnect stays immediate; the writer key untouched. Must NOT: identity coalescing, an owned timer, release-on-arbitrary-error, per-operation locking.
• Patch Verdict: Matches, with one conservative strengthening the ticket allows. Verified against the diff: sweepExpired requires inFlight === 0 AND now - lastTouchAt >= leaseTtlMs (closed boundary, deterministic); beginWrite runs inside assertWritable before any mutation path; endWrite's safeToRelease requires failed && acquisition.created === true && inFlight === 0 && !shared && disposition ∈ {pre-mutation, rollback-complete}; the re-entry shared: true flag makes a creator failure permanently unable to auto-release a shared generation (stated in the PR's Deltas — the conservative direction); findCurrent requires token + writer-key match, fencing stale generations; releaseAgent remains the immediate disconnect authority and disconnect releases even in-flight generations (a named spec). ADR 0021 compliance verified: LockRegistry's writer-key math is untouched except for additive provenance fields.
• Premise Coherence: Coheres with verify-before-assert and the heap-as-truth settlement: every authority decision binds to heap-local generation state, and every outward value is a defensive copy. The clock-injection seam is the same evidence-over-authority discipline the wake and restore lanes landed today: the sweep's authority is an injectable fact (time), not a heuristic over holder behavior.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15681
• Related Graph Nodes: #13056 (parent epic), #13134, #13282, #13167, #13172, #15678 (the incident), ADR 0021, #15737 (Iris's adjacent hook-normalization lane)

🔬 Depth Floor

Challenge (one latent hardening note, non-blocking):

Fenced tokens leak through observability copies. copyLease includes token, so both the conflict copy handed to a denied writer and the leaseReceipts() ledger expose live fence tokens heap-wide. I verified at exact head that this is latent, not live: releaseWrite's only production caller is InstanceService.assertWritable's stale-acquisition cleanup with an internally-generated acquisition — no caller-controlled release-by-acquisition surface exists, and the Bridge disconnect path uses stamped-identity releaseAgent(selector). But the token is the only thing fencing a release against a forged acquisition (findCurrent checks token + writer key, not the caller's stamped context), so any future release surface that binds the acquisition's identity fields rather than the stamped context becomes a cross-writer release primitive armed by these copies. One-line hardening: strip token from conflict/receipt copies (it is a fence, not observability), or document in the class JSDoc that tokens are generation fences, not a security boundary. Worth doing whenever a release surface next opens; not a merge blocker today.

I also actively looked for: (a) an in-flight bypass in the sweep — none, inFlight === 0 is conjunctive; (b) a stale-generation clobber through touchWrite/endWrite — none, findCurrent double-gates; (c) an unsafe release on callMethod failure — none, arbitrary method failures are always unknown-disposition → retain; (d) receipt-log unboundedness — none, slice(-limit) with a fail-silent guard that cannot alter lock decisions.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "without changing the exact (agentId, sessionId) writer key" — verified (LockRegistry key math untouched; the spoof falsifier passes). "Bounded lifecycle receipts" — verified (receiptLimit slice).
• Deltas section: the two declared refinements (shared-generation conservatism, precise clock-denial reasons through admitWrite) both verified in the diff — the shared: true re-entry marking and admitWrite's errors[0] || 'write-guard-denied' path.
• Anchor & Echo JSDoc: the rewritten class summary names the lease contract precisely and keeps the scope boundary honest; no ticket-anchor prose in durable comments.
• "158 passed" receipt: consistent with exact-head CI unit SUCCESS at d6023a415c (verified via statusCheckRollup).

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the ticket's two-cycle intake produced a Contract Ledger the diff matches exactly.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Phase-aware failure release is the safe generalization of "release on error": the only releasable failure is one the operation created and can prove never mutated (or fully rolled back); everything else retains and defers to TTL/disconnect. The shared flag extends the same conservatism to re-entrancy — once a generation has two owners, no single failure may speak for it. And the fenced-token pattern localizes the whole stale-generation class: an old operation's credentials simply stop resolving.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15681 (newline-isolated, PR body); single commit d6023a415c carries the ticket ref in subject only
• #15681 confirmed not epic-labeled (labels: enhancement, ai, architecture)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (8 rows)
• Implemented diff matches the Ledger: acquire provenance + generation (LockRegistry fields), lease metadata + defensive copies (WriteGuard), lazy injected-clock sweep (no owned timer), fenced touch/end/release, admission-seam receipt retention (admitWrite.acquisition → assertWritable return), phase-aware error lifecycle, writer identity preserved with spoof falsifier, bounded receipts

Findings: Pass.

🪜 Evidence Audit

• PR body declares Evidence: L2 → L2 required, no residuals
• Achieved ≥ required: all ACs are internal App-Worker contracts exercised by deterministic state-transition + in-process integration witnesses at exact head; CI green
• Post-merge items are honestly scoped as optional live smokes, not evidence gaps

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI surface touched; no new workflow convention — the lease lifecycle extends an existing heap authority behind its existing seams, so no cross-skill wiring is required.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at d6023a415c (statusCheckRollup — all checks SUCCESS); author's 15-spec, 158-test focused receipt is consistent with CI scope
• Reviewer falsifier: N/A — no named behavioral concern. My one finding is a latent leak whose non-liveness I verified by caller enumeration at exact head (releaseWrite's sole caller is the internal stale-acquisition cleanup); no runtime falsifier applies.
• Test location: co-located with the touched seams (test/playwright/unit/ai/**) — canonical; the incident reproduction, TTL boundary, stale-token fence, and phase-failure matrix are deterministic clock-injected state-transition tests, not sleeps. Structure-map: ran per mandate — all edits land in the existing src/ai/ lock-authority home; no placement question.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 96 — the lease lifecycle lands on the existing heap authority behind its existing seams, ADR 0021's writer key and held-until-release regime are provably untouched, and the injected clock keeps the authority timer-free; −4 for the token leak through observability copies being left as an undocumented latent surface.
• [CONTENT_COMPLETENESS]: 95 — every new method carries precise JSDoc including the failure-phase semantics and the fence contract; the PR body's Deltas section honestly names the two conservative refinements; −5 for the token's not-a-security-boundary status being discoverable only by caller enumeration.
• [EXECUTION_QUALITY]: 95 — conjunctive in-flight guard, double-gated findCurrent, closed TTL boundary, conservative fallbacks on broken clocks (deny at request, no-touch at end), bounded receipts with a fail-silent observability guard, and a deterministic witness for every AC.
• [PRODUCTIVITY]: 100 — all nine ACs carry named witnesses, including the incident reproduction (persistent failed writer retains until expiry; same identity in a distinct session stays a separate writer) and the ?id= spoof falsifier.
• [IMPACT]: 85 — retires a production-reproduced permanent lockout class for every persistent Neural Link writer; the write-authority path is high-blast, and this hardens it without regime change.
• [COMPLEXITY]: 78 — four production seams spanning stateless conflict math, the stateful authority, the admission function, and four write paths in the service; the failure-phase matrix is the reader load, and it is the honest cost of the contract.
• [EFFORT_PROFILE]: Heavy Lift — high-complexity, high-impact lifecycle work on a load-bearing authority, carried with the deterministic matrix the intake demanded.

The #15678 incident shape — a persistent writer deadlocking the heap until reload — is unrepresentable against this head: idle expiry reclaims, disconnect releases immediately, and no failure path can hold forever. Merge-safe.