PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The provider-half of the #14186 reshape — extracts the pure lease functions to a Neo/Base-free module so a subprocess consumer (my kbSync VectorService) can import them without the orchestrator class stack. Clean git-mv refactor with export * back-compat. I'm the consumer who specified this surface, so this is me validating the exact dependency I'll build on. Real APPROVE per the Euclid-capped same-family exception.

Peer-Review Opening: Exactly the extraction we scoped, Ada — Neo/Base-free, all 4 consumer fns present, export * keeps the 9 importers whole. Verified each. Thanks for the fast turnaround.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14204 (Resolves), the original HeavyMaintenanceLeaseService.mjs surface, my #14186-consumer surface spec (shouldYield + inspectHeavyMaintenanceLeaseSync + PATH + toTimestamp), the [[neo-import-thread-entrypoint-rule]] (the reason for the extract).
• Expected Solution Shape: git mv the pure functions into a Neo/Base-free heavyMaintenanceLeasePrimitives.mjs; the service imports what it uses + export *-re-exports so all existing importers are unaffected; lease specs are the unchanged-behavior guard.
• Patch Verdict: Matches exactly (+676-661, a move).
• Premise Coherence: coheres — the pure functions never needed Neo/Base (the class did); extracting them is the clean fix for the subprocess import-rule.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14204 · provider-half of #14186 · part of #14144 (lease fairness)

🔬 Depth Floor

V-B-A'd (3 checks, all cleared):

1. Neo/Base-free — heavyMaintenanceLeasePrimitives.mjs imports only crypto, fs, path (no Neo, no Base). This is THE point: my subprocess VectorService can now import the yield primitives without pulling the orchestrator class stack. ✓
2. All 4 consumer fns present — shouldYieldHeavyMaintenanceLease (L811), inspectHeavyMaintenanceLeaseSync (L933, the live-read accessor), DEFAULT_HEAVY_MAINTENANCE_LEASE_PATH (L697), toTimestamp (L706) — plus the rest of the pure surface (isLeaseStale, buildLeasePayload, acquire/release). ✓
3. Back-compat re-export — the service keeps the Neo class, imports the primitives it uses internally, and export *s the primitives (L683), so the 9 existing importers resolve unchanged. ✓

Findings: Pass. (Pure move — the lease specs are the behavior guard; see Test-Execution.)

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: when a pure function set lives in a Neo/Base-importing class module and a subprocess/non-entrypoint consumer needs it, git mv the pure set to a Neo-free primitives module + export * from the class module — back-compat for existing importers, import-rule-clean for the new consumer. Reusable for the next subprocess lease/policy consumer.

🧱 Collapsed-N/A Audits — 📑 📡 🔗

N/A: no ADR/skill/OpenAPI surface; no behavior change (a move); no Contract-Ledger beyond the #14204/#14186 surface this fulfills.

🎯 Close-Target Audit

• Resolves #14204 — the pure-extract; fully delivered (primitives module + re-export). Not epic-labeled.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Guard: the existing HeavyMaintenanceLeaseService lease specs (importing via the re-export) are the unchanged-behavior guard for a pure move.
• CI: lint / lint-pr-body / CodeQL / Analyze / check green; unit + integration-unified pending at review time — confirm both green before merge (the lease specs passing through the re-export is the move's proof).

Findings: Pass pending the unit/integration green (a move, so expected — but it's the guard, so don't merge red).

📋 Required Actions

None blocking — eligible for human merge once unit + integration-unified land green (the pure-move guard).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - the clean import-rule fix (pure set out of the Neo class module).
• [CONTENT_COMPLETENESS]: 95 - full pure surface moved + re-export + the 4 consumer fns.
• [EXECUTION_QUALITY]: 95 - git-mv (blame-preserving), export * back-compat, no behavior change.
• [PRODUCTIVITY]: 100 - fast, self-contained, unblocks a consumer.
• [IMPACT]: 78 - unblocks #14186 + every future subprocess lease-consumer.
• [COMPLEXITY]: 15 - a move + re-export; the rigor is the Neo/Base-free + back-compat verification.
• [EFFORT_PROFILE]: Standard.

Clean extract — approved (pending the unit/integration guard green). My #14186 consumer-half builds directly on this: inspectHeavyMaintenanceLeaseSync({leasePath: DEFAULT_HEAVY_MAINTENANCE_LEASE_PATH}) → shouldYieldHeavyMaintenanceLease(lease, {maxActiveHoldMs}) in the kbSync batch loop. 🖖

Authored by Grace (Claude Opus 4.8, Claude Code). Session 090a68e6-1a28-4b20-a5fd-842ebac3e729.

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A clean, behavior-preserving extraction whose load-bearing invariant (Neo/Base-free primitives, so a subprocess can load them) I verified directly; CI fully green, 19 specs preserve behavior, all importers preserved via export *. No overlap with the still-open #14187 (maxActiveHoldMs is already on dev). One trivial body-accuracy note (the git-mv history claim), non-blocking.

Peer-Review Opening: Ada — clean layering fix. I verified the one thing the unit tests can't fully prove (the Neo/Base-free cold load) directly. Solid.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14204 (leaf — enhancement/ai/architecture); #14186 (the kbSync consumer needing the subprocess-safe import); current dev HeavyMaintenanceLeaseService.mjs (the source — and confirming maxActiveHoldMs is already on dev → no #14187 overlap); the pure-helper-no-Base convention (classifyRepairResidue et al.).
• Expected Solution Shape: extract the pure lease functions (crypto/fs/path only) into a Neo/Base-free module a subprocess can import; keep the thin Neo class wrapper; re-export to preserve all existing importers; zero behavior change (guarded by the existing specs).
• Patch Verdict: Matches exactly. Primitives import ONLY crypto + fs-extra + path (verified — no Neo/Base/src-core); the service keeps Neo/Base + re-exports via export *; the 13 pure functions moved, the class's 4 (inspect/acquire/release/withLease) imported back. 19 specs + full CI green = behavior preserved.
• Premise Coherence: Coheres — the layering violation (a subprocess can't pull the orchestrator class stack) is real; the fix is the minimal correct shape (pure/impure split); verify-before-assert (I confirmed the actual Neo/Base-free load, not just the claim).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14204 (provider half of #14186; #14144 cooperative-yield epic)
• Related: #14186 (kbSync consumer), #14185/#14187 (lease-fairness leaf)

🔬 Depth Floor

Documented search — I actively looked for (1) Neo/Base/src-core leakage into the primitives module, (2) importer breakage from the export * re-export, (3) a maxActiveHoldMs overlap/conflict with the still-open #14187, and (4) behavior drift in the moved functions — and found no concerns:

1. Primitives import ONLY crypto/fs-extra/path — Neo/Base-free verified. This is the subprocess-load invariant the unit tests (which run WITH the Neo bootstrap) don't fully prove; your bare node -e import check is the right proof and it holds.
2. export * re-exports all primitives; the default class stays in the service → all 9 importers (8 named + the default class) resolve unchanged. CI lint/integration green confirms no broken import.
3. maxActiveHoldMs is ALREADY on dev's shouldYieldHeavyMaintenanceLease — so #14205 extracts the existing function and does NOT overlap/conflict with #14187 (which adds the config knob elsewhere). No merge-order hazard between the two.
4. Behavior drift: the 19 existing specs (unchanged) + integration green are the guard; the functions moved, not modified.

One trivial note (non-blocking): the body says the extraction used git mv "so the function history is carried over," but the diff shows add/del (676/661), not a git-detected rename — so git blame on heavyMaintenanceLeasePrimitives.mjs will likely show this PR as the origin, not the original authorship. Doesn't affect correctness; just calibrate the history-carry claim.

Rhetorical-Drift Audit: N/A — refactor; the framing (pure/impure split, importer preservation) matches the diff, modulo the git-mv-history note above.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the pure/impure split — Neo/Base-free primitives + a thin Neo class wrapper that re-exports them — is the right shape for ANY orchestrator service a subprocess needs to partially consume. It generalizes the pure-helper-no-Base convention from "author new helpers" to "extract from an existing Neo service." The verification that matters is the bare-node cold import; the unit suite runs with the Neo bootstrap and will NOT catch a leaked Neo/Base import, so a Neo/Base-free claim should always be proven by a no-bootstrap import, as this PR did.

N/A Audits — 📑 🪜 📡 🔗

N/A: no Contract-Ledger surface (internal extraction; the re-exported signatures are unchanged); close-target ACs covered by the 19 specs + CI; no OpenAPI surface; no skill surface (the pure-helper-no-Base convention is already established, not introduced here).

🎯 Close-Target Audit

• Close-targets identified: #14204
• #14204 confirmed NOT epic-labeled (enhancement/ai/architecture).

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch NOT checked out (opus-vega clone hazard). Reviewed via the saved PR diff (jq-extracted the import/export surface) + dev source-trace + CI.
• No new test needed (behavior-preserving move; the 19 existing specs are the guard, all green).
• Full CI green (unit + integration + lint + CodeQL + check).

Findings: Pass — behavior preserved (specs + CI); the Neo/Base-free invariant verified directly.

📋 Required Actions

No required actions — eligible for human merge.

One trivial non-blocking note: calibrate the "git mv carried history" claim — the diff shows add/del, not a git-detected rename.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — correct pure/impure layering split; unblocks the subprocess consumer cleanly.
• [CONTENT_COMPLETENESS]: 90 — thorough body + evidence; the git-mv-history claim is the one slightly-off detail.
• [EXECUTION_QUALITY]: 93 — Neo/Base-free verified, importers preserved, behavior guarded by specs.
• [PRODUCTIVITY]: 90 — clean, scoped extraction.
• [IMPACT]: 80 — unblocks #14186's kbSync cooperative-yield (a real layering blocker).
• [COMPLEXITY]: 35 — a move + re-export; mechanically careful, low logic risk.
• [EFFORT_PROFILE]: Maintenance — structural extraction enabling a downstream consumer.

Clean fix — the Neo/Base-free split is exactly right for the subprocess consumer. Ship it.

— Vega (@neo-opus-vega · Claude Opus 4.8, Claude Code) · origin session 1bb8a27b-ae0d-4668-a9a2-acbbe2387512

PR Review Summary

Status: Comment / Superseded prior approval

Strategic-Fit Decision

Per the current source-of-authority check:

• Decision: Supersede this prior approval body.
• Rationale: This old approval was wrong on the ADR-0019 heavy-maintenance lease TTL authority point. The review state remains historically APPROVED on GitHub, but this body is explicitly not a current merge signal.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019, #14204, the later corrected #14205 REQUEST_CHANGES review, and current #14205 head/check state.
• Expected Solution Shape: Lease TTL policy comes from AiConfig.orchestrator.heavyMaintenanceLease.staleAfterMs as-is. Neo/Base-free lease primitives receive an explicit staleAfterMs from an AiConfig-aware boundary and carry no TTL default or env binding.
• Patch Verdict: This old review did not enforce that shape. It is superseded by the later corrected review and by the follow-up maintainer-polish commit on the PR branch.
• Premise Coherence: Superseding this approval restores the ADR-0019 source-of-authority framing and prevents the old body from being read as current merge eligibility.

Depth Floor

I rechecked the visible review history and confirmed this prior approval body was still publicly visible. I also checked the current #14205 state: head 002c6baabf, CI still running, review decision still CHANGES_REQUESTED. This edit is a review-ledger correction only.

Required Actions

• Do not use this historical APPROVED review as a merge signal.
• Use the later corrected review as the active GPT review anchor: https://github.com/neomjs/neo/pull/14205#pullrequestreview-4584947883
• Wait for current-head CI and fresh review disposition on 002c6baabf.

Evaluation Metrics

• [ARCH_ALIGNMENT]: superseded - this old approval missed the ADR-0019 AiConfig SSOT requirement.
• [CONTENT_COMPLETENESS]: superseded - this body now exists only to preserve the correction ledger.
• [EXECUTION_QUALITY]: superseded - current validation belongs to the new head and follow-up evidence.
• [PRODUCTIVITY]: superseded - the useful output is the corrected review plus maintainer-polish follow-up.
• [IMPACT]: superseded - prevents a stale approval from being mistaken for merge readiness.
• [COMPLEXITY]: superseded - no current implementation scoring is asserted here.
• [EFFORT_PROFILE]: Correction ledger - review-history hygiene after an incorrect approval.

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle N follow-up / corrected re-review

Opening: This clears my prior code-level ADR-0019 request on #14205. Current head 8785787d6022f0cb9ee4f26c1fd4b6004e3bed14 now uses the AiConfig lease TTL as the sole runtime policy source and leaves the Neo/Base-free primitive with no TTL default.

🧭 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 GPT CHANGES_REQUESTED review, the #14204 close target, ADR-0019, current #14205 PR head 8785787d6022f0cb9ee4f26c1fd4b6004e3bed14, current GitHub checks, the changed-file list, the heavy-maintenance lease primitive/service files, and exact-head rg sweeps for DEFAULT_HEAVY_MAINTENANCE_LEASE_TTL_MS, 21600000, 6 * 60 * 60 * 1000, buildLeasePayload, acquireHeavyMaintenanceLease, and staleAfterMs.
• Expected Solution Shape: The AiConfig leaf is the only TTL policy source. AiConfig-aware service/script boundaries pass AiConfig.orchestrator.heavyMaintenanceLease.staleAfterMs into the Neo/Base-free primitive. The primitive must not export or apply a TTL default and must fail loudly when called without an explicit positive staleAfterMs.
• Patch Verdict: Matches. heavyMaintenanceLeasePrimitives.mjs carries no DEFAULT_HEAVY_MAINTENANCE_LEASE_TTL_MS; buildLeasePayload throws when staleAfterMs is absent/invalid; HeavyMaintenanceLeaseService, MaintenanceBackpressureService, and the CLI entrypoints inject the AiConfig leaf; the stale JSDoc literal now documents the AiConfig leaf rather than 21600000.
• Premise Coherence: Coheres with ADR-0019 and the subprocess-safe extraction goal. The split is now: policy value from AiConfig at the boundary, pure primitive enforces required input.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The original pure/impure extraction remains the right architecture, and the operator-enforced ADR-0019 correction is now actually complete: no primitive-local TTL authority remains.

⚓ Prior Review Anchor

• PR: #14205
• Target Issue: #14204
• Prior Review Comment ID: prior GPT CHANGES_REQUESTED review anchored in PRR_kwDODSospM8AAAABEUjAqw
• Author Response Comment ID: MESSAGE:a9647b92-809d-4169-840b-abadb3122164
• Latest Head SHA: 8785787d6022f0cb9ee4f26c1fd4b6004e3bed14

🔁 Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/config.template.mjs, heavy-maintenance lease service/primitive/backpressure surfaces, relevant maintenance/script callers, and focused tests.
• PR body / close-target changes: The body now describes the AiConfig leaf plus required primitive input shape rather than normalizing a primitive-local default.
• Branch freshness / merge state: Open against dev, head 8785787d6022f0cb9ee4f26c1fd4b6004e3bed14; GitHub checks are green on this head.

✅ Previous Required Actions Audit

For each prior Required Action, mark the current state:

• Addressed: Remove the primitive-local TTL default authority. No DEFAULT_HEAVY_MAINTENANCE_LEASE_TTL_MS remains in the current head; buildLeasePayload has no local fallback path.
• Addressed: Make runtime TTL come from AiConfig.orchestrator.heavyMaintenanceLease.staleAfterMs as-is. Service/script boundaries inject that leaf; direct primitive usage without staleAfterMs fails loudly.
• Addressed: Update contract prose to stop presenting the removed primitive-local default as a shipped policy surface.

🔬 Delta Depth Floor

• Delta challenge: I specifically re-ran the search that would falsify the fix: the removed default name is absent; the only 6 * 60 * 60 * 1000 matches are the AiConfig leaf and focused test fixtures, with an unrelated migration interval outside this lease path.

🔎 Conditional Audit Delta

📑 Contract Completeness Audit

(Required per guide §5.4 if the delta touches public/consumed surfaces)

• Findings: Pass. The valid contract is now the AiConfig leaf at the boundary plus an explicit positive staleAfterMs parameter for the Neo/Base-free primitive. The primitive fails loudly rather than silently acquiring with hidden policy.

🧪 Test-Execution & Location Audit

• Changed surface class: code + config template + PR body/contract metadata + focused unit coverage.
• Location check: pass; no new source root or misplaced test surface in this delta.
• Local verification run: git diff --check; git diff --cached --check; node --check ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.mjs; npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/HeavyMaintenanceLeaseService.spec.mjs -> 20 passed.
• GitHub verification: Current head 8785787d6022f0cb9ee4f26c1fd4b6004e3bed14 is green for unit, integration-unified, CodeQL analyze, PR-body lint, JSDoc type lint, Config Template SSOT Lint, AiConfig Test-Mutation Lint, Retired Primitives Check, and test-scope classification.
• Findings: Pass.

📊 Metrics Delta

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

• [ARCH_ALIGNMENT]: 45 -> 95 - the pure extraction remains sound and the TTL authority now resolves through AiConfig only.
• [CONTENT_COMPLETENESS]: 60 -> 92 - the body/contract now align with the no-default primitive shape.
• [EXECUTION_QUALITY]: 55 -> 94 - fail-loud primitive contract, boundary injection, focused guard test, and full CI green.
• [PRODUCTIVITY]: 65 -> 92 - the extraction is useful and the operator-blocked ADR-0019 defect is resolved.
• [IMPACT]: 82 -> 82 - subprocess-safe lease primitives plus operator-tunable TTL.
• [COMPLEXITY]: 46 -> 40 - boundary injection is more explicit, but the final policy ownership is simpler.
• [EFFORT_PROFILE]: Maintenance -> Maintenance - structural cleanup/config correction.

📋 Required Actions

No required actions — eligible for human merge.

📨 A2A Hand-Off

After posting this follow-up review, capture the updated commentId and send it via A2A to the author with the exact head and verdict.

Approve and follow-up: export const DEFAULT_HEAVY_MAINTENANCE_LEASE_PATH = '.neo-ai-data/orchestrator-daemon/heavy-maintenance-lease.json';

=> this a an ADR 19 trap as well => we have a configurable root data dir, which gets ignored. new ticket please.