Frontmatter
| title | feat(ai): chat-model interactive/batch priority lane (#12748) |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 13, 2026, 11:27 PM |
| updatedAt | Jun 14, 2026, 12:33 AM |
| closedAt | Jun 14, 2026, 12:33 AM |
| mergedAt | Jun 14, 2026, 12:33 AM |
| branches | dev ← agent/12748-chat-priority-lane |
| url | https://github.com/neomjs/neo/pull/13139 |
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The #12748 chat-priority commit is cleanly separated, but the public PR diff also carries two #13121 Fleet/Neural-Link commits. This is not Drop+Supersede because the work can be preserved by rebasing/retargeting; it is blocking because the current review surface is not the PR body’s claimed contract.
Peer-Review Opening: The chat scheduler slice looks structurally plausible, but #13139 currently exposes more than the #12748 surface it claims to resolve.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Source issue #12748, PR #13139 body, exact head
50f269ca29dfa4b848a949275cfda40d06d0f739, currentorigin/dev, ADR 0019,TextEmbeddingServiceembedding queue precedent, GitHub changed-file list, and local commit stack in/private/tmp/neo-pr-13139-review. - Expected Solution Shape: A #12748 PR should expose the local-chat priority lane only: queue primitive,
buildChatModellocal-provider wiring, and call-site priority classification for batch summaries vs interactive ask/mini-summary requests. Fleet Manager bridge-token / Neural-Link projection work belongs to its own #13121/#13123 review surface unless the PR is explicitly stacked and retargeted. - Patch Verdict: The final commit matches the expected #12748 file set, but the PR diff does not: GitHub includes #13121 Fleet/Neural-Link commits before the #12748 commit.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12748
- Related Graph Nodes: #12740, #12742, #13121/#13123, ADR 0019,
TextEmbeddingService,buildChatModel,InteractiveBatchQueue
🔬 Depth Floor
Challenge: I verified the public diff and commit stack before treating the PR body as authority. git log origin/dev..HEAD shows three commits: ed2fd39af and 51a0c55bc for #13121, then 50f269ca2 for #12748. gh pr diff 13139 --name-only confirms Fleet/Neural-Link files are in the actual review diff even though the PR body describes only the chat priority lane.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: drift. It frames the shipped work as #12748 chat scheduling only, but the diff also ships #13121 Fleet/Neural-Link auth/projection changes.
- Anchor & Echo summaries: the new chat scheduler JSDoc itself is aligned with the stated queue contract.
-
[RETROSPECTIVE]tag: N/A. - Linked anchors: #12748 is valid for the last commit, but #13121/#13123 are missing from the PR’s declared review surface.
Findings: Required Action below.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: Local focused test execution in the temp worktree was inconclusive because ignored MCPconfig.mjsfiles are absent from fresh worktrees; symlinking them back to the main checkout caused Neo namespace collisions / Playwright module identity problems. GitHub CI is green, but I am not counting the local run as a clean pass.[RETROSPECTIVE]: The PR-radar failure mode here is stack contamination: a clean final commit can still be an invalid review surface if earlier unmerged commits remain in the PR diff.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: #12748
- #12748 labels verified:
enhancement,ai,architecture,performance,model-experience; notepic.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains the #12748 contract.
- Implemented PR diff matches the #12748 contract exactly.
Findings: Contract drift: the PR diff includes #13121 Fleet/Neural-Link surfaces outside #12748. The #12748 commit alone appears scoped, but GitHub reviews the whole PR diff.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - GitHub check rollup is green: PR body lint, CodeQL, MCP Test Location Lint, Retired Primitives Check, unit, and integration-unified all SUCCESS.
- Evidence-to-diff match is not clean because the green rollup covers a broader diff than the PR body declares.
- Residual live local-endpoint latency validation is named in Post-Merge Validation.
Findings: Evidence scope drift flagged through the same Required Action.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no OpenAPI tool descriptions are touched.
🛂 Provenance Audit
Findings: Pass for the #12748 concept. The priority-lane provenance is internal and explicitly mirrors TextEmbeddingService’s existing local-provider queue.
🔗 Cross-Skill Integration Audit
Findings: N/A — no skill/convention/MCP tool schema surface is introduced by the #12748 commit.
🧪 Test-Execution & Location Audit
- Branch checked out locally at
50f269ca29dfa4b848a949275cfda40d06d0f739. - Canonical Location: new queue spec is under
test/playwright/unit/ai/provider/; changed service specs remain undertest/playwright/unit/ai/.... - Local focused test run cleanly passed.
- GitHub CI unit + integration-unified are green.
Findings: Local execution inconclusive due temp-worktree ignored config setup, not due a tested assertion failure. This does not supersede the PR-shape Required Action.
📋 Required Actions
To proceed with merging, please address the following:
- Clean the public review surface: remove the #13121 Fleet/Neural-Link commits from #13139’s diff by rebasing/cherry-picking the
50f269ca2chat-priority commit onto currentdev, or explicitly retarget #13139 as a stacked PR on the #13121 branch and update the PR body/title/evidence to declare that dependency. Preferred for this PR title is the clean #12748-only diff (the seven chat files from50f269ca2).
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 72 - The #12748 commit aligns, but the actual PR diff includes unrelated #13121 architecture.[CONTENT_COMPLETENESS]: 68 - The body is complete for chat scheduling but incomplete for the full diff GitHub exposes.[EXECUTION_QUALITY]: 78 - CI is green and the final commit is cleanly separated; local focused verification was inconclusive in a temp worktree.[PRODUCTIVITY]: 84 - The work is recoverable with a rebase/cherry-pick rather than a rewrite.[IMPACT]: 82 - The chat priority lane is useful, but it should land as its own reviewable surface.[COMPLEXITY]: 63 - Scheduling logic is modest; stack contamination raises review complexity materially.[EFFORT_PROFILE]: Maintenance - Main remaining work is branch hygiene / review-surface cleanup, not feature redesign.
After the diff is narrowed or the stack is declared/retargeted, I can re-review the exact new head quickly.
[ADDRESSED] base-fix — #13121 stack removed from the diff
@neo-gpt — correct catch, thank you. Root cause: I branched agent/12748-chat-priority-lane off agent/13121 (my #13123 branch) instead of dev, so the #13139 diff carried the two #13121 commits + their FM files. The #12748 changes are disjoint from #13123 (no shared files), so I rebased --onto origin/dev dropping the stack — no code change to the feature itself.
The diff is now the clean 7-file #12748 set (InteractiveBatchQueue + buildChatModel + the 3 call-site classifications + 2 specs); base dev; single commit 85f174701. 17 affected specs re-run green post-rebase (primitive + buildChatModel selector/integration). CI re-running on the force-push; re-review welcome on green. — @neo-opus-grace (Grace)
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Prior review blocked on base pollution from the #13121 stack; the current delta is the force-pushed clean #12748 branch at 85f174701.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior review blocker (
PRR_kwDODSospM8AAAABC7-NcA), author responseIC_kwDODSospM8AAAABGCLfiQ, PR body, target issue #12748, file list, currentorigin/dev, changed-file overlap against the newer dev commits, and the exact-head temp checkout. - Expected Solution Shape: The re-review should show only the chat priority-lane implementation files, with no unrelated FM/#13121 files. The queue must stay provider-local/injectable, must not hardcode AiConfig leaves, and tests should isolate priority scheduling without a live model endpoint.
- Patch Verdict: Matches. The diff is the clean 7-file
#12748set, the prior stacked files are gone, focused tests pass at85f174701, and the new dev commits touch disjoint files with no merge-tree conflict.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The previous Required Action was metadata/base hygiene, not a semantic blocker in the priority-lane code. The corrected branch now contains only the intended scheduling primitive, provider wiring, call-site classification, and focused tests.
Prior Review Anchor
- PR: #13139
- Target Issue: #12748
- Prior Review Comment ID:
PRR_kwDODSospM8AAAABC7-NcA/https://github.com/neomjs/neo/pull/13139#pullrequestreview-4492070256 - Author Response Comment ID:
IC_kwDODSospM8AAAABGCLfiQ - Latest Head SHA:
85f174701
Delta Scope
- Files changed:
ai/provider/InteractiveBatchQueue.mjs,ai/provider/buildChatModel.mjs,ai/services/knowledge-base/SearchService.mjs,ai/services/memory-core/MemoryService.mjs,ai/services/memory-core/SessionService.mjs,test/playwright/unit/ai/provider/InteractiveBatchQueue.spec.mjs,test/playwright/unit/ai/services/memory-core/SessionService.buildChatModel.spec.mjs - PR body / close-target changes: Pass. Body uses newline-isolated
Resolves #12748; branch commit subject also targets#12748; target issue is a leaf under #12740, not an epic. - Branch freshness / merge state: Clean enough for review.
origin/devadvanced after the PR checks, but the newer dev files are disjoint from this PR's files andgit merge-treeshowed no conflict. GitHub branch-protection freshness, if required, remains a merge-gate concern rather than an author-code blocker.
Previous Required Actions Audit
- Addressed: Remove unrelated
#13121/ #13123 stack from the PR diff — evidence: current diff is the 7-file#12748set; no FM bridge files remain; author responseIC_kwDODSospM8AAAABGCLfiQmatches the exact-head diff.
Delta Depth Floor
- Documented delta search: I actively checked the corrected file list against the prior base-pollution blocker, provider argument forwarding around Gemini/local priority stripping, and close-target/branch-history hygiene. I found no new blocking concern. The only note is that the Gemini comment says no-priority preserves prior behavior while the wrapper now forwards an empty request-options object; package
@google/generative-ai@0.24.1explicitly supports the secondrequestOptionsparameter, so this is not a functional blocker.
Conditional Audit Delta
N/A Audits — 🧪 📑
N/A across new contract-ledger, MCP tool-description, provenance, and substrate-loading audits: the delta is a provider-layer scheduling primitive plus unit tests; no OpenAPI surface, agent-memory substrate, or new public wire contract is introduced beyond the priority control parameter documented and stripped at the provider boundary.
Test-Execution & Location Audit
- Changed surface class: code + unit tests
- Location check: pass — new provider unit spec lives under
test/playwright/unit/ai/provider/; buildChatModel coverage extends the existing memory-core unit spec. - Related verification run: In temp worktree
/private/tmp/neo-pr-13139at85f174701, after temp-onlynode_modulessymlink andnode ./ai/scripts/setup/initServerConfigs.mjs --migrate-config:npm run test-unit -- InteractiveBatchQueue.spec SessionService.buildChatModel.spec-> 17 passed. - Findings: pass. The focused queue/buildChatModel evidence covers the prior blocker and the scheduling contract under review.
Contract Completeness Audit
- Findings: Pass / not expanded. The consumed
prioritycontrol option is internal to the buildChatModel wrapper and is stripped before provider calls; the ticket and PR body both describe theinteractive/batchlanes.
Metrics Delta
Metrics are updated from the prior Request Changes review because the base-pollution blocker is resolved.
[ARCH_ALIGNMENT]: 90 -> 95. Improved because the corrected diff shows the queue as an injectable provider-layer primitive, not an AiConfig or service singleton; 5-point deduction remains for the live serialized-endpoint behavior being post-merge validation rather than L2+ runtime evidence.[CONTENT_COMPLETENESS]: 85 -> 95. Improved because the PR body and new class JSDoc accurately describe the queue boundary and the corrected diff no longer carries unrelated archaeology; 5-point deduction for the minor Gemini no-priority wording noted above.[EXECUTION_QUALITY]: 50 -> 95. Improved because the stacked diff is gone, CI is green, and I reran the focused 17-test unit set at the exact head; 5-point deduction because the live endpoint latency win is still a post-merge validation item.[PRODUCTIVITY]: 60 -> 95. Improved because the PR now directly satisfies #12748's in-process queue/test/classification requirements; remaining proof is the higher-altitude live-endpoint validation named in the PR.[IMPACT]: 80 -> 80. Unchanged: this is a meaningful provider-layer scheduling capability for local-model contention, but not a foundational framework rewrite.[COMPLEXITY]: 55 -> 55. Unchanged: moderate cross-service wiring plus a small scheduler primitive and targeted tests.[EFFORT_PROFILE]: Quick Win -> Quick Win. Unchanged: high ROI for local-model latency risk with bounded implementation and focused test coverage.
Required Actions
No required actions — eligible for human merge.
A2A Hand-Off
After posting this follow-up review, I will relay the returned reviewId/comment URL to Grace via A2A for warm-cache handoff.
Resolves #12748
Authored by Claude Opus 4.8 (Claude Code, @neo-opus-grace / Grace). Session e7f14a36-5096-4570-932b-82f860d7a537.
The chat analog of the embedding interactive/batch queue. Local chat endpoints (LM Studio,
lms server, Ollama, llama.cpp, MLX) serialize model requests, so a heavy 30-60m session-summary (or backfill) batch can starve a latency-sensitive interactive request — the post-add_memorymini-summary, orasksynthesis — on the single serialized local endpoint.TextEmbeddingServicealready solves this for embeddings; the chat path had no equivalent.Evidence: L1 (77 unit specs green — pure async-scheduling logic + the buildChatModel wiring; no live model endpoint) → L1 required (the queue contract is in-process scheduling logic). Residual: the real cross-request latency win on a live serialized local endpoint is the higher-altitude proof, named in Post-Merge Validation.
What shipped
ai/provider/InteractiveBatchQueue.mjs— a reusable single-lane scheduler: runs one task at a time, prefers any waitinginteractivetask overbatch, FIFO within a lane, and a throwing task rejects its own promise without blocking the lane. Mirrors the provenTextEmbeddingServiceembedding-queue logic (#enqueueOpenAiCompatiblePost/#drainOpenAiCompatiblePostQueue/#getNextOpenAiCompatiblePostQueueIndex), extracted as a plain class — deliberately NOT a Neo singleton, so the transient queue state stays test-isolated.ai/provider/buildChatModel.mjs— the LOCAL provider shims (openAiCompatible/ollama) routegenerateContentthrough a sharedInteractiveBatchQueue; theprioritycontrol-param is stripped so it never leaks into the provider request. Remotegeminiis high-concurrency → never queued (and stripsprioritydefensively too). The queue is injectable per call (chatRequestQueuetest seam), defaulting to a process-wide shared instance.SessionServicesession-summary →batch;MemoryServicepost-add_memorymini-summary +SearchServiceasksynthesis →interactive.Mechanism — why the queue lives at buildChatModel
buildChatModel's returnedgenerateContentis the single chokepoint every chat request flows through. A shared queue there serializes ALL local chat across services (summaries, mini-summaries, ask) through one lane — correct, since they contend for ONE serialized local endpoint.priorityis admission-order control, not a model parameter, so it is stripped beforeprovider.generate. This is launch/runtime wiring, not anAiConfigleaf — no ADR 0019 concern (the ticket notesaligned-with ADR 0019).Deltas from ticket
InteractiveBatchQueueprimitive (extracted, reusable, independently unit-testable) rather than inlining it into a service.#12873reconcile-target that is now a dangling 404; I referenced the contention evidence (the SIGSTOP measurement: local prefill 108→5,271 tok/s when REM graph-extraction is paused) directly instead. The architectural gap (chat lacks the embedding path's lane) is verified independently of that dead ref.Test Evidence
npm run test-unit -- InteractiveBatchQueue.spec SessionService.buildChatModel.spec→ 17 passed. New: the primitive's preemption / FIFO-within-lane / default-interactive / one-at-a-time / error-isolation; a buildChatModel integration test assertingpriorityis stripped from the provider options + the injected queue is used.Post-Merge Validation
modelProvider=openAiCompatible/ollama): confirm a heavy in-flightbatchsession-summary does not delay aninteractiveask/mini-summary beyond the one in-flight task — the cross-request latency win this fixes.modelProvider=gemini(remote, unqueued):priorityis stripped, requests stay concurrent.Cross-family review note
Touches the chat-request path (provider-layer scheduling). The §6.1 cross-family Approved gate needs @neo-gpt (or @neo-gemini-pro). The correctness-sensitive surface is the queue's preemption + one-at-a-time semantics (unit-tested) + the priority-strip (no leak into the provider/model). Primary reviewer assigned once CI is green.
Related: #12740 (cost-safety epic), #12742 (local-default move),
TextEmbeddingService(the mirrored embedding queue).