PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The VectorService-level yield mechanism is directionally correct, but the current-head lease-holder wiring still disables the yield in the real syncKnowledgeBase path, and the yielded child outcome is still classified as completed. Those are same-PR correctness defects for #14186, not follow-up polish.

Thanks for driving the consumer half of the lease-fairness chain. The preserve-not-promote shadow behavior and the between-batch yield point are the right shape, but the release path has to work at the script/scheduler boundary, not only inside the direct embedChunks unit seam.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14186, parent #14144, ADR 0022, ADR 0019, current dev source for syncKnowledgeBase.mjs, VectorService.mjs, ProcessSupervisorService.mjs, and PrimaryRepoSyncService.mjs; prior Memory Core context for the settled inspect/acquisition lease -> shouldYield -> preserve shadow + yield shape.
• Expected Solution Shape: A correct consumer wires the existing time-driven lease-yield predicate from the actual lease payload to a safe shadow-swap batch boundary, reads maxActiveHoldMs from the AiConfig leaf at the use site, and reports a yielded partial sync as a non-completion/resume outcome. It must not hardcode the stale-lease subtree as the fairness policy boundary, and tests need one script/supervisor-level assertion so the direct VectorService seam cannot mask a broken real path.
• Patch Verdict: Contradicts the expected shape at the boundary. The lower-level VectorService yield path works, but syncKnowledgeBase.mjs:77 reads AiConfig.orchestrator.heavyMaintenanceLease.maxActiveHoldMs, while the configured leaf is AiConfig.orchestrator.heavyMaintenance.maxActiveHoldMs (ai/config.template.mjs:687-688). A focused probe on current head showed prPathYield:false and correctPathYield:true for a one-hour lease. Also, yielded child output still flows through the existing completed path in both ProcessSupervisorService and the primary-dev cascade.
• Premise Coherence: The premise coheres with ADR 0022's fairness goal, but the current implementation violates the V-B-A/ADR 0019 use-site requirement by reading the wrong config branch and currently records a partial yield as completed telemetry.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14186
• Related Graph Nodes: #14144, #14146, #14161, ADR 0022, ADR 0019

🔬 Depth Floor

Challenge: The current tests prove the local embed loop and shadow preservation, but they do not exercise the real lease-holder boundary that builds the predicate from AiConfig and publishes the child outcome to the orchestrator. That is exactly where both defects are.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift detected. It says syncKnowledgeBase.mjs builds the predicate against the reactive maxActiveHoldMs config leaf, but the code reads the wrong branch.
• Linked anchors: #14186/#14144 establish yield-and-resume, but the shipped child outcome still classifies yielded:true as completed.

Findings: Required Actions below.

🧠 Graph Ingestion Notes

• [KB_GAP]: Boundary coverage gap: the direct VectorService tests pass while the script-level AiConfig leaf and child-outcome classifier remain wrong.
• [TOOLING_GAP]: None for the PR. One local rerun initially hit a Chroma port conflict because I started two unit commands in parallel; rerunning serially passed.
• [RETROSPECTIVE]: ADR 0019 failures can be subtle even when the code appears to read AiConfig; the exact subtree matters, and a direct behavior probe caught what green VectorService tests did not.

🎯 Close-Target Audit

• Close-targets identified: #14186
• #14186 is not epic-labeled. Parent #14144 is only related, not the close target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly

Findings: Missing ledger. This PR modifies consumed surfaces: the optional shouldYield service option, the VectorService result envelope (yielded), and the structured syncKnowledgeBase stdout consumed by ProcessSupervisorService / PrimaryRepoSyncService. #14186 and parent #14144 currently lack a Contract Ledger, and that missing contract is visible in the yielded-outcome classification drift.

🪜 Evidence Audit

Local evidence confirms the lower-level mechanism but falsifies the delivered boundary:

• node --check ai/scripts/maintenance/syncKnowledgeBase.mjs passed.
• node --check ai/services/knowledge-base/DatabaseService.mjs passed.
• node --check ai/services/knowledge-base/VectorService.mjs passed.
• npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/VectorService.tenantStamping.spec.mjs test/playwright/unit/ai/services/knowledge-base/VectorService.WorkVolumeBranching.spec.mjs test/playwright/unit/ai/services/knowledge-base/VectorService.leaseYield.spec.mjs passed 29/29.
• npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/ProcessSupervisorService.spec.mjs passed 40/40.
• Probe on current head: AiConfig.orchestrator.heavyMaintenance.maxActiveHoldMs === 1800000; shouldYieldHeavyMaintenanceLease(oneHourLease, {maxActiveHoldMs: AiConfig.orchestrator.heavyMaintenanceLease.maxActiveHoldMs}) === false; same probe with AiConfig.orchestrator.heavyMaintenance.maxActiveHoldMs returns true.
• Probe on current head: ProcessSupervisorService.classifySuccessfulChildOutcome('kbSync', {deferred:false, yielded:true, embedded:50}) returns {status:'completed'}.

GitHub CI at review time had CodeQL/lint/check green, with unit and integration-unified still in progress. The Request Changes verdict is based on deterministic current-head code probes, not pending CI.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP OpenAPI surface or cross-skill/workflow convention is added by this PR.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact current head 7e5c40b96d0707933aa945c5c76910570dc864a6.
• Canonical Location: new/changed tests are under test/playwright/unit/ai/services/knowledge-base/, matching right-hemisphere unit placement.
• Ran changed VectorService-related unit files and the existing ProcessSupervisor classifier suite.
• Additional probes covered the untested syncKnowledgeBase AiConfig leaf and child-outcome classifier paths.

Findings: Tests pass, but coverage is missing the two boundary defects called out below.

📋 Required Actions

To proceed with merging, please address the following:

• Fix the fairness-bound leaf read in ai/scripts/maintenance/syncKnowledgeBase.mjs:77. The PR currently reads AiConfig.orchestrator.heavyMaintenanceLease.maxActiveHoldMs, but the leaf is AiConfig.orchestrator.heavyMaintenance.maxActiveHoldMs. With the current code, maxActiveHoldMs is undefined, and shouldYieldHeavyMaintenanceLease() fail-safes to false, so the real lease-holder path never yields.
• Add boundary coverage for the real predicate construction, not just embedChunks. A focused test or pure helper seam should prove that a long-held acquisition lease plus the configured heavyMaintenance.maxActiveHoldMs makes syncKnowledgeBase / the threaded shouldYield predicate return true. This is the regression that the current 29/29 VectorService suite does not catch.
• Define and implement yielded child-outcome semantics in the orchestrator consumers. Today {deferred:false, yielded:true} is classified as completed by ProcessSupervisorService.mjs:774-798, and the cascade path in PrimaryRepoSyncService.mjs:587-606 also falls into markCompleted('kbSync'). A preserved partial shadow is not a completed full sync; it needs a skipped/yielded/resume classification that does not refresh lastSuccessAt as if the KB is fully current. Add ProcessSupervisor and cascade tests for this envelope.
• Backfill the Contract Ledger on #14186 (or parent #14144 if that is the intended authority) for the consumed shouldYield option, yielded result envelope, and syncKnowledgeBase structured stdout semantics, then align the PR body to the exact shipped contract.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 55 - The intended ADR 0022 fairness shape is right, but the real boundary violates ADR 0019 by reading the wrong AiConfig subtree and the scheduler telemetry still treats partial yield as completion.
• [CONTENT_COMPLETENESS]: 60 - JSDoc and PR prose are detailed, but the Contract Ledger is missing and the PR body overclaims the boundary behavior.
• [EXECUTION_QUALITY]: 45 - Lower-level tests pass, but the delivered lease-holder path never yields and yielded child output is classified as completed.
• [PRODUCTIVITY]: 45 - The VectorService mechanism advances #14186, but the close-target behavior is not delivered until the script and outcome-boundary defects are fixed.
• [IMPACT]: 75 - This is a meaningful v13.1 fairness fix for long kbSync runs that can starve githubWorkflowSync.
• [COMPLEXITY]: 65 - The change crosses VectorService, DatabaseService, a CLI child process, lease primitives, and orchestrator outcome classification.
• [EFFORT_PROFILE]: Heavy Lift - Moderate code size, but high behavioral coupling across resumable embedding and maintenance scheduling.