Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 21, 2026, 5:54 AM |
| updatedAt | Jun 21, 2026, 6:07 AM |
| closedAt | Jun 21, 2026, 6:07 AM |
| mergedAt | Jun 21, 2026, 6:07 AM |
| branches | dev ← fix/lms-chat-parallel-13700 |
| url | https://github.com/neomjs/neo/pull/13706 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: Approve+Follow-Up because the code change is a narrow, high-impact launch-argument fix with green CI and exact-head local validation. The only remaining evidence is necessarily live-host/runtime evidence (
lms ps+ RSS after reload), and the PR already names that as post-merge validation rather than claiming it is proven in CI.
Peer-Review Opening: Reviewed at exact head b6a5d2a98814f4788581e7d1736068a85afdc30e. The important shape is right: this does not weaken requireParallelModels: 2; it adds the separate LM Studio per-model request-slot knob where the RAM multiplier actually lives.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #13700, changed-file list,
origin/dev...HEADdiff, currentproviderReadinessHelper/ orchestrator preload path, ADR 0019 AiConfig SSOT boundary, #13624 incident context, PR CI/check state, and exact-head tests. - Expected Solution Shape: Correct shape should add one canonical config leaf for chat-model
--parallel, thread it through the existing LM Studio preload option boundary, leaverequireParallelModelsuntouched, and verify the changed preload/tests without hardcoding a global model policy or embedding override. Test isolation should remain in the existing right-hemisphere unit specs with injectedloadModel/execFileFnseams. - Patch Verdict: Matches.
ai/config.template.mjsownslocalModels.chat.parallel;buildLmsPreloadConfig()emits a chat-onlyparallelsmap;ensureLmsModelsLoaded()force-includes resident models whenparallelmust be enforced;loadLmsModel()appends--parallel; andtaskDefinitionsreceives the map through the existing LMS preload boundary.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #13700
- Related Graph Nodes: #13624, #13539, ADR 0019, LM Studio preload / provider readiness
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The committed tests cover the
buildLmsPreloadConfig()chat-only map and the focused suite passes, but they do not add a direct committed assertion thatloadLmsModel()appends--parallel. I verified that path manually at exact head (args: ['load','chat-model','--context-length','131072','--parallel','1']). Non-blocking because the implementation is small, directly inspected, and the runtime post-merge validation is the real AC; worth adding a direct unit assertion if this surface is touched again.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; it states L1 launch-arg evidence and does not claim live RSS is already proven.
- Anchor & Echo summaries: config/JSDoc comments distinguish per-model
--parallelfrom resident-model count. -
[RETROSPECTIVE]tag: N/A. - Linked anchors: #13700/#13624 establish the incident context; #13539 is a non-closing related memory-pressure reference.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: Review-side semantic KB/Memory sweeps were unavailable (ask_knowledge_basetimeout; Memory Core embedding canary timeout). Exact-head source, GitHub, CI, and local tests were used as the review authority.[RETROSPECTIVE]: Good separation of two overloaded “parallel” concepts:requireParallelModelsis resident distinct-model capacity; LM Studio--parallelis per-model request-slot/KV-cache multiplication.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: #13700.
- #13700 confirmed not
epic-labeled (bug,ai,architecture).
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket / PR contract names the shipped surface:
localModels.chat.parallel, envNEO_LOCAL_MODELS_CHAT_PARALLEL, helperparallel, and LM Studio--parallel. - Implemented PR diff matches that contract exactly: chat-only map, embedding absent,
requireParallelModelsunchanged.
Findings: Pass for this narrow incident hotfix. No drift observed; promote to a full Contract Ledger if this knob expands beyond the single chat-model launch override.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is correctly framed as L1 launch-arg/config evidence; runtime
lms ps+ RSS remains post-merge validation. - Evidence-class collapse check passes: the PR does not promote static/unit evidence into live-host proof.
Findings: Pass with explicit post-merge follow-up: after orchestrator reload, record lms ps showing chat PARALLEL 1 plus the observed llmworker RSS reduction.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no ai/mcp/server/*/openapi.yaml tool descriptions changed.
📜 Source-of-Authority Audit
Findings: Pass. ADR 0019’s AiConfig SSOT stays intact: the new leaf lives in ai/config.template.mjs, Orchestrator reads the resolved config at the bootstrap boundary, and the helper receives a value-object map parallel to the existing contextLengths launch-option map rather than introducing a new shadow resolver.
🔗 Cross-Skill Integration Audit
Findings: N/A — no skill file, workflow convention, MCP tool surface, wire format, or startup instruction substrate changed.
🧪 Test-Execution & Location Audit
- Branch checked out locally: repo-local worktree
tmp/reviews/pr-13706atb6a5d2a98814f4788581e7d1736068a85afdc30e. - Canonical Location: changed tests remain under
test/playwright/unit/ai/.... - Syntax checks passed:
node --checkon the modified JS/spec files. - Whitespace check passed:
git diff --check origin/dev...HEAD. - Related tests passed:
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.invariants.spec.mjs test/playwright/unit/ai/scripts/runners/runSandman.spec.mjs→ 62 passed. - Direct review probes passed:
loadLmsModel()producedlms load chat-model --context-length 131072 --parallel 1;ensureLmsModelsLoaded()passed{contextLength: 131072, parallel: 1}for chat and context-only for embedding.
Findings: Tests pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 94 - Deducted 6 for the missing committed directloadLmsModel --parallelassertion, but the architecture matches the existing preload contract and ADR 0019 boundary.[CONTENT_COMPLETENESS]: 92 - PR body cleanly explains the two parallel knobs and evidence ceiling; deducted 8 because #13700 lacks a formal ledger matrix for the new env/config surface.[EXECUTION_QUALITY]: 94 - Exact-head focused suite, syntax checks, diff check, and direct probes pass; runtime RSS remains intentionally post-merge.[PRODUCTIVITY]: 96 - Delivers the core #13700 fix without changing resident-model policy or embedding behavior.[IMPACT]: 90 - High operational value for local/cloud memory pressure under #13624, with a small code footprint.[COMPLEXITY]: 62 - Moderate cross-file threading through config, orchestrator task definitions, provider helper, and tests; conceptually simple once the two parallel meanings are separated.[EFFORT_PROFILE]: Quick Win - Small, focused patch with large expected RAM payoff and contained post-merge validation.
Approve+Follow-Up: merge-eligible from this review; the follow-up is live-host validation after reload, not another pre-merge review cycle.
Resolves #13700.
Summary
localModels.chat.parallel(default 1) threads throughbuildLmsPreloadConfig→ensureLmsModelsLoaded→loadLmsModelas the chat model's lms--parallelslot count. Each--parallelslot holds an independent KV cache at the 128K context, so the lms-default 4 multiplied gemma's resident RAM ~4× (a ~29 GB worker for a 19.7 GB model). The chat roles (graph extraction / session summary / miniSummary) are lease-serialized — measured concurrent demand 1 — so slots beyond the first are idle KV bloat;--parallel 1reclaims it.Two distinct knobs (the V-B-A that drove this)
requireParallelModels: 2— how many DISTINCT models stay co-resident (1 chat + 1 embedding in parallel). Untouched — both models stay loaded; the operator's "hold both like ollama" intent is preserved.--parallel <N>— the per-model request-slot KV multiplier. This is the fix. Chat → 1; the embedding role keeps the lms default (the parallels map is chat-only, intentionally).Evidence: measured peak concurrent local-gemma call = 1, derived from the
MaintenanceBackpressureServicelease invariant (DEFAULT_HEAVY_MAINTENANCE_TASK_NAMES+ the single[kbSync, memory-summary-backfill]= embedding∥chat compatible pair), not assumed.ask_knowledge_baseruns on remote Gemini (off-box); the dream is internally sequential (one tri-vector call per session).Test Evidence
Evidence:
buildLmsPreloadConfigreturn-shape pins updated for the additiveparallelsfield + a positive chat-only-map test. 62 unit pass (Orchestrator.invariants+runSandman). This is an L1 launch-arg change — the runtime AC is observable vialms ps(thePARALLELcolumn) + host RSS, not a unit assertion.Deltas
parallelknob onlocalModels.chat(default 1; envNEO_LOCAL_MODELS_CHAT_PARALLEL); was unset → lms-default 4.loadLmsModel/ensureLmsModelsLoaded/buildLmsPreloadConfigthread a chat-onlyparallelsmap (mirrors the existing--context-lengththreading exactly).Post-Merge Validation
lms psshows the chat model atPARALLEL 1and thellmworkerRSS drops from ~29 GB toward ~22 GB (the reclaimed ~3×128K KV slots).requireParallelModels: 2still holds both models resident (no reload churn — the embedding worker is unaffected).