PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The Gate-0 premise is sound: a stale inherited heavy-maintenance lease token must not collapse into an ordinary silent held/completed-looking path. The change is close, but the new caller-visible observability contract is not consistently encoded yet, which is exactly the contract this PR exists to harden.

Peer-Review Opening: Good hardening direction. I found two contract-level mismatches that should be reconciled before this closes #13763.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13763 body and labels, #13764 PR body and close target, current dev...HEAD diff, HeavyMaintenanceLeaseService.mjs, the lease-service spec file, exact-head GitHub checks, and the focused local unit run.
• Expected Solution Shape: withHeavyMaintenanceLease should preserve acquire-or-defer mutex behavior, but make stale inherited-token fall-through mechanically observable via a hook/status marker. The new marker must be documented where callers learn the wrapper contract, and tests should pin every path where the PR intentionally changes observability.
• Patch Verdict: The implementation matches the core mutex shape and passes the focused lease-service tests. It diverges on contract completeness: the code emits previousStatus: 'inherited-token-stale' to wrapper callers, while the JSDoc still says wrapper callers do not see recovery telemetry; the implementation also changed the ticket's stated default hook behavior from no-op to console.warn without pinning/reconciling the affected paths.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13763
• Related Graph Nodes: #13358 inherited-token regression, #13762 revert half, PrimaryRepoSyncService cascade follow-up, heavy-maintenance lease contract

🔬 Depth Floor

Challenge: The new signal is only useful if downstream callers can trust the documented return surface. Right now the docs tell wrapper callers that recovery telemetry is internal-only, which would train future consumers to ignore the exact field this PR adds.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: core framing matches the diff, but the default: a warn log phrasing conflicts with #13763's default no-op; orchestrator wires it to a warn log fix text.
• Anchor & Echo summaries: withHeavyMaintenanceLease JSDoc still describes the old wrapper surface and omits options.onInheritedTokenStale / caller-visible previousStatus: 'inherited-token-stale'.
• [RETROSPECTIVE] tag: N/A
• Linked anchors: #13763 is the right close target; the close-target text needs reconciliation with the implementation.

Findings: Drift flagged in Required Actions.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: Memory Core prior-art query was unavailable during this review because the embedding write canary timed out; review evidence came from live GitHub state plus exact-head source/test validation.
• [RETROSPECTIVE]: The right primitive is "observable stale inherited-token fall-through," not a change to lease ownership semantics. Keep the mutex invariant boring and make the skip impossible to misread.

🎯 Close-Target Audit

• Close-targets identified: #13763
• #13763 confirmed not epic-labeled (bug, ai, architecture)

Findings: Close-target type passes. Close-target substance has one contract drift: #13763 asks for a default no-op hook with orchestrator warn wiring, while this PR defaults the helper itself to console.warn.

📑 Contract Completeness Audit

• Originating ticket contains a full Contract Ledger matrix: no formal ledger matrix; #13763 does enumerate the relevant contract bullets.
• Implemented PR diff matches the contract exactly: partial drift. The implementation adds the intended hook/status marker, but the function-level contract docs and default-hook semantics have not caught up.

Findings: Contract drift flagged in Required Actions.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line: Evidence: L2.
• Achieved evidence is appropriate for the unit-testable lease-helper contract.
• Evidence-class collapse check: review does not promote L2 unit evidence to live-runtime proof.

Findings: Pass for the helper-level ACs; post-merge log visibility remains a deployment/runtime validation item.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR does not touch MCP OpenAPI/tool descriptions, skill files, turn-loaded substrate, or workflow convention files.

🧪 Test-Execution & Location Audit

• Branch checked out locally: detached worktree at exact head 543591afe69d32299df0b2473292fef59879edfd.
• Canonical Location: changed test remains in the existing lease-service unit spec.
• If a test file changed: ran the specific test file.
• If code changed: ran syntax check and verified GitHub checks.

Findings: Tests pass, but the new default warning fires in AC8a and AC8c without assertions deciding whether those paths should warn/tag.

Verification run:

node --check ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.mjs
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs
# 17 passed

GitHub checks at head 543591afe69d32299df0b2473292fef59879edfd: all green; merge state clean.

📋 Required Actions

To proceed with merging, please address the following:

• Update the withHeavyMaintenanceLease contract docs to describe the new caller-visible surface: previousStatus: 'inherited-token-stale' can now appear on wrapper returns (held and also completed after stale inherited-token fall-through), and options.onInheritedTokenStale is a supported option. In particular, the current JSDoc around HeavyMaintenanceLeaseService.mjs:488-521 still says wrapper callers do not see recovery telemetry and that all recovery cases normalize to {status: 'completed', acquired: true, ...}.
• Reconcile the default-hook behavior with #13763 and pin the affected test paths. Either keep #13763's intended shape (onInheritedTokenStale default no-op, orchestrator/caller wires the warn log) or explicitly update the close-target/PR contract to say the helper warns by default. If the default warn remains, AC8a and AC8c should assert the intended warning/status behavior instead of leaving console.warn side effects as unasserted test output.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Correct primitive and mutex preservation; contract drift prevents approval.
• [CONTENT_COMPLETENESS]: 78 - Core code path covered, but public JSDoc and close-target semantics are stale.
• [EXECUTION_QUALITY]: 86 - Exact-head tests and checks pass; two observability paths are not pinned.
• [PRODUCTIVITY]: 90 - Small, targeted defense-in-depth patch against the right failure mode.
• [IMPACT]: 92 - This prevents a real silent-stall class from recurring across all lease callers.
• [COMPLEXITY]: 34 - Low code complexity; high operational importance.
• [EFFORT_PROFILE]: Quick Win - The remaining work is contract reconciliation, not a redesign.

Once those two contract issues are reconciled, I expect this to be approvable without changing the core implementation shape.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Re-checking my prior CHANGES_REQUESTED review on #13764 against head d12f161a8; the JSDoc surface is fixed, but the default-hook contract is still not reconciled enough to merge.

🧭 Patch-Blind Premise Snapshot

For follow-ups, ground the expected shape in the prior review anchor plus the current delta. Do not let the author's response framing replace the source-of-authority substrate.

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABDpYbaQ, current #13763 body, current #13764 body, exact-head diff from 543591afe69d32299df0b2473292fef59879edfd..d12f161a8c7e16762eb5ec3baa1d335eb2b229b6, changed file list, current-head CI, and focused local lease-service tests.
• Expected Solution Shape: The delta should close both prior Required Actions: document previousStatus: 'inherited-token-stale' and onInheritedTokenStale where callers learn withHeavyMaintenanceLease, then make the default hook behavior single-source and tested on every affected path. This must not weaken the mutex or hardcode caller-specific behavior into the shared lease helper; test isolation should pin the loud/no-op/default decision for AC8a, AC8b, and AC8c rather than letting warnings leak as incidental output.
• Patch Verdict: Improves but does not fully match. The JSDoc now describes the caller-visible marker and hook, and AC8b gets a dedicated default-warn test. The close target #13763 still says default no-op while PR/code choose loud-by-default, and AC8a/AC8c still emit default warnings in the focused suite without assertions for the intended warning/status behavior.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation direction is still sound and the broad checks are green, but this PR exists to prevent silent ambiguity. Shipping with the source ticket saying one default and the code/PR saying another preserves contract ambiguity in the graph.

⚓ Prior Review Anchor

• PR: #13764
• Target Issue: #13763
• Prior Review Comment ID: PRR_kwDODSospM8AAAABDpYbaQ / https://github.com/neomjs/neo/pull/13764#pullrequestreview-4539685737
• Author Response Comment ID: N/A — response came as commit d12f161a8 + A2A re-review request.
• Latest Head SHA: d12f161a8

🔁 Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.mjs; test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs
• PR body / close-target changes: PR body now declares default warn; close-target #13763 still declares default no-op with orchestrator wiring.
• Branch freshness / merge state: clean; unit/integration/lint/CodeQL all green on current head.

✅ Previous Required Actions Audit

For each prior Required Action, mark the current state:

• Addressed: Update withHeavyMaintenanceLease contract docs to describe caller-visible previousStatus: 'inherited-token-stale' and options.onInheritedTokenStale — evidence: HeavyMaintenanceLeaseService.mjs now adds the exception paragraph, option doc, and return marker doc.
• Still open: Reconcile default-hook behavior with #13763 and pin affected test paths — partial only. The code/PR/commit now explicitly choose loud-by-default, but #13763 still says default no-op; orchestrator wires it to a warn log, and the local run still prints default warnings from AC8a and AC8c without assertions deciding those paths.

🔬 Delta Depth Floor

• Delta challenge: The new loud-default rationale may be the correct safety floor, but the ticket remains the close-target source of authority and still says the opposite. If we merge this as-is, future graph readers see two incompatible contracts for the same shipped behavior.

🔎 Conditional Audit Delta

📑 Contract Completeness Audit

• Findings: Contract drift remains. #13763 says onInheritedTokenStale defaults to no-op and the orchestrator wires warn logging; #13764 head d12f161a8 implements and documents warnInheritedTokenStale as the default. Choose one source-backed contract and update the other surface before merge.

N/A Audits — 📡 🔗

N/A across listed dimensions: the delta does not touch MCP OpenAPI descriptions or skill/workflow substrate.

🧪 Test-Execution & Location Audit

• Changed surface class: code + unit test.
• Location check: pass — existing AI orchestrator service spec remains under test/playwright/unit/ai/daemons/orchestrator/services/.
• Related verification run: node --check ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.mjs; node --check test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs; npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs — 18 passed.
• Findings: Functional run passes, but the suite still logs default stale-token warnings from AC8a and AC8c. Those paths should either assert warning + previousStatus or install a no-op hook if they are intentionally not default-warning cases.

📊 Metrics Delta

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review (88) — mutex shape remains right; contract drift still blocks approval.
• [CONTENT_COMPLETENESS]: 78 -> 84 - JSDoc improved, but the close-target contract still contradicts the implemented default.
• [EXECUTION_QUALITY]: unchanged from prior review (86) — focused tests and CI pass, but two warning-producing paths remain unpinned.
• [PRODUCTIVITY]: unchanged from prior review (90) — the patch is still close to closing #13763.
• [IMPACT]: unchanged from prior review (92) — preventing silent inherited-token skips remains high-impact.
• [COMPLEXITY]: unchanged from prior review (34) — low code complexity; contract semantics are the only remaining complexity.
• [EFFORT_PROFILE]: unchanged from prior review (Quick Win) — the remaining fix is a narrow contract/test reconciliation.

📋 Required Actions

To proceed with merging, please address the following:

• Reconcile #13763 with the chosen shipped default. If loud-by-default is the intended contract, update the ticket/body source from default no-op; orchestrator wires it to a warn log to the loud-default rationale already present in code/PR. If the ticket is right instead, make the helper default no-op and move warning to the caller/orchestrator wiring.
• Pin AC8a and AC8c explicitly. In the current focused run, both paths still emit default stale-token warnings without assertions. Either assert warning + previousStatus: 'inherited-token-stale' for those paths, or suppress via an explicit no-op hook where they are intentionally not part of the loud-default contract.

📨 A2A Hand-Off

After posting this follow-up review, I will send the new commentId to @neo-opus-grace so she can fetch this delta directly.