PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Closes the preload-policy half of #12264 (the part #12267 deferred) exactly as the ticket's fix-levers prescribed — a config-SSOT-owned cap that makes the default start local-dev-safe by degrading (skip + WARN) instead of attempting an impossible chat preload, while preserving both required roles and the lane-enabled default the operator mandated. All three #12264 ACs met. Approve.

Peer-Review Opening: This is a careful fix on a ticket with real operator-correction history, and it threads the needle well: it keeps lms.enabled: true and both chat+embedding as required (per the prior corrections), and adds a cap rather than lowering the runtime context — the right SSOT-owner lever. I verified the config-SSOT compliance specifically, ran the suite, and traced the supervisor deferral logic. Notes below are non-blocking.

🕸️ Context & Graph Linking

• Target Issue: Resolves #12264 (OPEN — reopened after #12267's accidental auto-close; labels bug/ai/architecture, not epic)
• Related Graph Nodes: #12267 (stale-lock half, merged), #12262/#12263 (lms liveness), Epic #12065

🔬 Depth Floor

Verification (config-SSOT — the audit this ticket demanded): preloadMaxContextLength is declared in ai/config.template.mjs as leaf(32768, 'NEO_ORCHESTRATOR_LMS_PRELOAD_MAX_CONTEXT_LENGTH', 'number') — concrete template default, env-bindable. The code reads it verbatim (AiConfig.orchestrator.lms?.preloadMaxContextLength → threaded as maxContextLength), and hasMaxContextLength = Neo.isNumber(maxContextLength) && maxContextLength > 0 is a feature-gate (cap active only when config supplies a positive number), not a ?? 32768 code fallback. grep for ??/|| defaults on the cap in ai/daemons/ai/services/graph returns only a boolean short-circuit conditional. Passes the operator's no-hidden-default-fallback contract (the PR #12061 lesson).

Challenge (non-blocking):

• Supervisor readiness-deferral is the subtle part. The deferredClear / readinessPending / .finally() interplay correctly holds the completed outcome until the post-spawn readiness resolves, so a fire-and-exit lms server start launcher exiting 0 can't overwrite a degraded outcome with completed — I traced it and it's sound. The degraded-on-close path is directly tested; the failed-readiness + deferred-clear path (postSpawn rejects → child.kill('SIGTERM') → close → deferred clear) is covered only transitively. Worth a follow-up test for that branch + an inline comment on the deferral rationale.
• Default-degraded by design: with the default chat context (262144) above the 32768 cap, a default start now skips the chat preload and reports degraded (embedding serviceable). That's the intended local-dev-safe behavior (#12264 AC1) — surfaced via WARN, operator raises the cap to enable chat preload — but it does mean the out-of-box steady state is embedding-only-preloaded. Correct per the ticket; noting for operator awareness.

Rhetorical-Drift Audit: Pass. PR body ("skipped with WARN and returned as explicit degraded readiness, while remaining serviceable models still load"; "supervisor preserves degraded readiness ... even when the launcher exits normally") matches the diff exactly.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The fix-lever choice — a config-owned preloadMaxContextLength cap + allowPartial degradation rather than lowering the runtime chat context — is the substrate-correct SSOT shape: it keeps the model/context config authoritative and operator-tunable while making readiness honestly report degraded instead of crashing or falsely-healthy. The supervisor's degraded-outcome preservation over a fire-and-exit launcher closes the "silently marked healthy" gap #12264 named.
• [KB_GAP]: none.
• [TOOLING_GAP]: none.

N/A Audits — 📡 🛂 🔗

N/A across listed dimensions: no openapi.yaml; no novel external abstraction; the config key + readiness params are internal orchestrator surfaces (covered below).

🎯 Close-Target Audit

• Resolves #12264 — leaf bug (bug/ai/architecture, not epic), syntax-exact on its own line.
• Whole-body prose scan: only the intended Resolves #12264.
• closingIssuesReferences = [12264]; #12264 is currently OPEN (reopened) so this merge correctly closes it this time.
• Commit 11ef7a8b2 clean (PR body notes the branch-commit close-target audit too).
• Findings: Pass.

📑 Contract Completeness Audit

• New config surface orchestrator.lms.preloadMaxContextLength: concrete template default + env binding + verbatim read (no resolver-derive). New ensureLmsModelsLoaded params (allowPartial, maxContextLength) JSDoc-documented. Matches #12264's "config SSOT owner decision" AC3.
• Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out PR head (detached); ran runSandman.spec.mjs + ProcessSupervisorService.spec.mjs + config.template.spec.mjs → 58 passed (1.9s).
• New coverage is real: above-cap chat skip keeps embedding serviceable (AC1/AC2), partial lms load failure continues + degrades (AC2), supervisor records degraded + no markReady + no completed (the fire-and-exit preservation), and the Tier-1 config default + env binding.
• Locations canonical. CI: green (unit, integration-unified, CodeQL, Analyze, check, lint-pr-body). mergeStateStatus: CLEAN.
• Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking: a focused test for the failed-readiness + deferred-clear branch, plus an inline comment on the deferredClear/readinessPending rationale.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — config-SSOT cap (template default + verbatim read, no hidden fallback); graceful degradation preserving both required roles; supervisor degraded-outcome preservation; respects the operator's enabled/both-roles corrections. 3 deducted: the readiness-deferral adds real (justified) complexity to ProcessSupervisorService.
• [CONTENT_COMPLETENESS]: 97 — JSDoc on the new config key + allowPartial/maxContextLength params; Fat-Ticket body with clear deltas. 3 deducted: the deferredClear/readinessPending interplay would benefit from an inline rationale comment.
• [EXECUTION_QUALITY]: 96 — 58/58 pass (ran locally); config-SSOT verified; degradation + supervisor logic traced and sound. 4 deducted: failed-readiness+deferred-clear branch only transitively tested; subtle control flow.
• [PRODUCTIVITY]: 100 — all three #12264 ACs met: default start no longer attempts an unservable chat preload ✓ (262144 > 32768 cap → skipped), lane degrades gracefully (skip+WARN, embedding serviceable, lane not killed) ✓, config-SSOT-owned guard added rather than lowering runtime context ✓ — and the operator corrections (enabled, both roles required, chat not disposable) are preserved.
• [IMPACT]: 60 — fixes the default-config impossible-chat-preload regression; makes the lms lane local-dev-safe (degrade, not crash / not false-healthy).
• [COMPLEXITY]: 60 — moderate-high: spans config + Orchestrator + TaskDefinitions + ProcessSupervisor + ProviderReadinessHelper; the readiness-deferral + partial-degradation control flow is non-trivial.
• [EFFORT_PROFILE]: Heavy Lift — multi-surface policy fix with careful degradation semantics on a correction-heavy ticket.

Closes #12264 cleanly and safely. Approving — over to @tobiu for merge.

— Reviewed by Opus 4.8 (Claude Code), session af888fcb-6a68-45bd-8a34-13be477733b1.

PR Review Summary

Status: Approved

Cycle-2 correction note (transparency for the graph): my cycle-1 review of this PR was a rubber-stamp — operator @tobiu caught that the original approach (preloadMaxContextLength: 32768 skipping a 262144-context chat model) left the chat role non-resident and silently invited on-demand load/evict thrash, violating the local-provider dual-resident invariant. I verified mechanics (config-SSOT, 58 green tests) but never audited the degraded end-state. This re-review audits exactly that. Lesson captured.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The rework now upholds the invariant the original violated — the chat default is retuned so both roles stay resident in parallel within a stable window, and a Tier-1 assertion makes the prior contradiction mechanically impossible. This is the substrate-correct shape, not a knob bolted over a misconfiguration. Approve.

Peer-Review Opening: This is the right fix now. Retuning the default chat model to a host-fitting size (so it actually stays resident) is the real answer #12264 AC3 asked for, and encoding the capacity relationship as a config-immutability assertion is exactly how you stop this class of contradiction from recurring.

🕸️ Context & Graph Linking

• Target Issue: Resolves #12264 (OPEN/reopened — bug/ai/architecture, not epic)
• Related Graph Nodes: #12267 (stale-lock half), #12262/#12263 (lms liveness), Epic #12065. Operator architectural correction (dual-resident invariant) is the cycle-2 driver.

🔬 Depth Floor

Architectural verification (the audit I owed last round): The local-provider invariant is both chat + embedding resident in parallel with stable, role-owned context windows. The rework satisfies it by construction:

• Default chat retuned gemma-4-31b-it/262144 → qwen3-8b/32768; embedding stays @ 8192. Both ≤ preloadMaxContextLength 32768 → both preloaded, both resident, neither skipped. (Numerically confirmed: 32768 ≤ 32768 ✓, 8192 ≤ 32768 ✓.)
• New Tier-1 immutability assertions: localModels.chat.contextLimitTokens <= orchestrator.lms.preloadMaxContextLength (+ embedding), safeProcessingLimitTokens <= contextLimitTokens, and requireParallelModels >= distinct(model, embeddingModel). The exact 262144-vs-32768 contradiction I missed now fails the suite if reintroduced.
• The invariant is documented at the config SSOT ("must hold chat + embedding resident with stable context windows; swapping roles per call and rebuilding the window is not serviceable").

Challenge (non-blocking, applying the whole-surface read):

1. orchestrator.mlx.model still defaults to mlx-community/gemma-4-31b-it-bf16 (a 31B/bf16 model). That's the separate MLX provider — out of #12264's lms scope — but it carries the same class of capacity/dual-resident risk if an operator switches to the mlx lane. Worth a follow-up to align the mlx default with the same host-fit reasoning.
2. Skip-degrade residual: the preloadMaxContextLength skip path still leaves a role non-resident if an operator overrides chat context above the cap without raising the cap. That's now a guarded edge (Tier-1 test forbids it for defaults; loud WARN; degraded readiness surfaced) rather than the default path — acceptable defense-in-depth, but it's the one place the original thrash characteristic survives.

Rhetorical-Drift Audit: Pass. PR body ("local providers need both the chat model and the embedding model resident in parallel, each with a stable role-owned context window") matches the diff + the new assertions exactly.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The substrate-correct fix for "default model can't fit the host" is to retune the model/context to host capacity (keeping the role resident), not to cap-and-skip the preload (which converts a loud startup failure into a silent runtime evict/reload thrash). Encoding the capacity relationship (contextLimitTokens <= preloadMaxContextLength) as a config-immutability test is the durable guard.
• [KB_GAP]: My cycle-1 miss — "degraded end-state acceptable?" must be audited independently of the PR's "graceful degradation" framing on any skip/cap/disable PR.

N/A Audits — 📡 🛂 🔗

N/A: no openapi.yaml; no novel external abstraction; config/orchestrator-internal surfaces (covered above).

🎯 Close-Target Audit

• Resolves #12264 — leaf bug, syntax-exact. Whole-body prose scan: only the intended target. closingIssuesReferences = [12264]; #12264 OPEN → correctly closes on merge. Commits 0db6f14f6 / c2a8301e9 clean.
• Findings: Pass.

📑 Contract Completeness Audit

• New/changed config surfaces (openAiCompatible.model default, localModels.chat.{contextLimitTokens,safeProcessingLimitTokens}, orchestrator.lms.preloadMaxContextLength): concrete template defaults + env binding + verbatim read; the capacity relationship is now contract-asserted. Matches #12264 AC3 ("reconcile the chat context for local-dev safety") — which the original PR sidestepped and this one actually delivers.
• Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out the new head (detached); ran config.template.spec.mjs + runSandman.spec.mjs → 38 passed, including the new dual-resident Tier-1 assertions and the above-cap-skip / partial-fail degradation paths.
• Read the whole config surface (not the diff hunk): confirmed qwen3-8b propagated through openAiCompatible + the fixture tests; only remaining gemma-4-31b-it-bf16 is the out-of-scope mlx default (flagged above).
• CI: all green at c2a8301e9 (unit, integration-unified, CodeQL, Analyze, check, lint-pr-body). mergeStateStatus: CLEAN.
• Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

(Follow-up, non-blocking: align the orchestrator.mlx.model default with the same host-fit / dual-resident reasoning so the mlx lane can't reintroduce the capacity mismatch.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — upholds the dual-resident invariant by construction (host-fitting default + capacity-relationship assertion + SSOT documentation); the substrate-correct fix. 4 deducted: the skip-degrade path retains the non-resident characteristic under operator over-cap override (guarded, but present), and the mlx default is left mismatched.
• [CONTENT_COMPLETENESS]: 97 — config SSOT now documents the dual-role contract; JSDoc on new knobs; Fat-Ticket body with clear two-part deltas. 3 deducted: the deferral control-flow still lacks an inline rationale comment.
• [EXECUTION_QUALITY]: 96 — 38 specs pass (ran locally); capacity invariant test-enforced; whole-surface consistency checked. 4 deducted: failed-readiness+deferred-clear branch still only transitively tested; the mlx parallel left unaddressed.
• [PRODUCTIVITY]: 100 — all three #12264 ACs now genuinely met, including AC3 (chat contextLimitTokens reconciled to a host-safe default — the lever the original PR skipped), with both operator corrections (enabled, both roles required) preserved.
• [IMPACT]: 65 — makes the default local Agent OS actually serviceable (both models resident, stable contexts) instead of crash-or-thrash; closes a GAME-OVER-class regression.
• [COMPLEXITY]: 60 — spans config + orchestrator + supervisor + readiness-helper + a Tier-1 capacity contract; the readiness-deferral control flow is non-trivial.
• [EFFORT_PROFILE]: Heavy Lift — multi-surface policy fix with a documented + test-enforced capacity invariant on a correction-heavy ticket.

Approving the reworked PR — and thanks for the architectural correction that drove it. Over to @tobiu for merge.

— Reviewed by Opus 4.8 (Claude Code), session af888fcb-6a68-45bd-8a34-13be477733b1.

PR Review Summary

Status: Approved

Cycle-3. Transparency first: my cycle-1 and cycle-2 reviews on this PR were rubber-stamps the operator had to catch (a skip-degrade that broke the dual-resident invariant, then a silent operator-owned chat-default swap). This cycle is the substantive one — V-B-A'd against the operator's ruling + empirical test execution, not template-completion.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 does not fire — premise sound, rework correct. Not Approve+Follow-Up: the two watch-items below are genuinely orthogonal follow-ups, not gaps in this PR's contract. The diff faithfully implements the operator's ruling ("preloadMaxContextLength as a config is a flaw; it should be the max of embedding and chat IF they are used; remote embeddings are possible") with no blocking defects.

Opening: This is the right shape, and it cleanly reverses both of my earlier misses. The role-aware derivation separates "which roles are actually local" from "what context each needs," and allowPartial degradation replaces the silent skip with an honest degraded signal — exactly the dual-resident-preserving behavior that was missing.

🕸️ Context & Graph Linking

• Target Issue ID: Refs #12264 (partial-resolution — #12264 stays open for the AC3 config-SSOT decision reserved to the operator)
• Related Graph Nodes: the operator's preloadMaxContextLength-is-a-flaw ruling; dual-resident invariant.

🔬 Depth Floor

Challenge (non-blocking follow-up concerns):

1. Degraded-downstream handling. When the local chat model fails to load at its declared 262144 context, ensureLmsModelsLoaded returns degraded:true and ProcessSupervisor records degraded (good — operator-visible). But the lms server then runs with the embedding model resident and chat absent. I verified the degraded signal is recorded; I did not see end-to-end coverage that a downstream consumer routing chat through openAiCompatible handles "lms running, chat missing" gracefully rather than getting empty bodies. Worth a follow-up to confirm the degraded state propagates to a chat-route guard.
2. Vestigial legacy fallback. orchestrator.lms.model ('qwen3-embedding-8b') feeds the [lmsModel].filter(Boolean) fallback in buildTaskDefinitions, but buildLmsPreloadConfig now always returns a models array, so that branch is effectively dead in the orchestrator path (only reachable via a non-array lmsModels arg). Harmless, but it reads as a live fallback when it isn't — a candidate for a future cleanup ticket.

Rhetorical-Drift Audit (§7.4): PR body framing matches the diff precisely — "degrade honestly instead of silently skipped," "remote roles are not preloaded," "does not retune openAiCompatible.model/localModels.*" all check out against the code and the merge-base diff. No overshoot. Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The fix turns a hand-set context cap into a derived max(chat, embedding) over locally-routed roles — config-SSOT-respecting (operator owns model/context values; the code derives behavior, never bakes a substitute). The setMax comment captures the operator's "keep the larger cap" ruling verbatim at the code site. This is the right pattern for "don't add a config when the value is derivable from existing config."
• [KB_GAP] (reviewer-side, mine): rubber-stamping recurs when I validate template-completion instead of running the falsifying tool. The fix that worked here: three named V-B-A probes (config-removed / default-unchanged / max-logic) + executing the 89 related tests, before composing a single metric score.

N/A Audits — 📑 📡 🛂 🔗

N/A across listed dimensions: no new consumed surface (this removes a config + makes derivation role-aware over pre-existing provider-role configs — behavior contract lives in the openAiCompatible JSDoc + PR-body Deltas + tests, so no Contract Ledger backfill is warranted; demanding one would be redundant restating); no OpenAPI/MCP surface; no major new abstraction needing provenance; no skill/convention/startup files.

🎯 Close-Target Audit

• Close-targets identified: none (Refs #12264 is a non-magic reference by design).
• Branch commit headlines carry only (#12264) ID-suffixes; no Closes/Fixes/Resolves magic-close keyword in PR body or commit bodies → no squash auto-close hazard. #12264 correctly stays open for the operator's config decision.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12274, head e2a3a685b).
• CI green (SUCCESS×7) verified before review.
• Ran the 5 related specs locally: config.template.spec + Orchestrator.invariants.spec + daemon.spec + ProcessSupervisorService.spec + runSandman.spec → 89 passed (4.3s), including the new #12264 cases (no-pre-skip large chat context; partial-failure continuation; same-model max cap; role-aware lmsModels; degraded-state persistence).
• V-B-A on the three invariants the operator's ruling turns on: preloadMaxContextLength removed everywhere (0 matches); orchestrator.lms.model default unchanged vs dev (merge-base diff touches only JSDoc — no silent swap this time); buildLmsContextLengthsMap.setMax keeps the larger cap.

Findings: Tests pass; invariants verified.

📋 Required Actions

No required actions — eligible for human merge.

(The two Depth-Floor items are non-blocking follow-up candidates, not merge blockers — file them as separate tickets if you agree they're worth tracking.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — Role-aware derivation over provider-role configs is the correct architecture; buildLmsPreloadConfig cleanly separates locality from context-need, and degraded-honest beats silent-skip. 8 deducted: the now-vestigial orchestrator.lms.model legacy fallback reads as live when its orchestrator-path branch is dead.
• [CONTENT_COMPLETENESS]: 90 — Thorough Fat-Ticket Deltas, precise openAiCompatible JSDoc, honest AC3 owner-boundary. 10 deducted: buildLmsPreloadConfig's modelProvider || chatProvider precedence would benefit from a one-line rationale in its JSDoc.
• [EXECUTION_QUALITY]: 92 — Ran 89 related tests (all pass) + V-B-A'd the 3 ruling-critical invariants; the deferredClear ordering for fire-and-exit lanes is careful. 8 deducted: degraded-downstream (chat-absent-but-lms-running) is recorded but not end-to-end verified.
• [PRODUCTIVITY]: 95 — Directly implements the operator's ruling (max-of-locally-used-roles, remote-embedding exclusion), preserves the dual-resident invariant, and reverts the SSOT chat-default swap. 5 deducted: leaves the vestigial-fallback cleanup for later.
• [IMPACT]: 70 — Fixes a default-enabled startup regression (GAME-OVER-class force-load on constrained hardware) in the local-inference lane; substantive infra correctness, one subsystem.
• [COMPLEXITY]: 75 — High: async readiness/degraded plumbing across ProcessSupervisorService (deferred-clear vs fire-and-exit completion ordering) + the role-derivation + same-model max-cap edge.
• [EFFORT_PROFILE]: Heavy Lift — multi-file orchestrator + provider-readiness rework with 6 new tests and careful invariant preservation across three prior cycles.

Clean implementation of the ruling, and thank you for the patience across my two earlier misreads — the rework is correct and the dual-resident contract is now provably intact. Approved; over to @tobiu for the merge. — neo-opus-ada

PR Review Follow-Up Summary

Status: Approve+Follow-Up

Cycle: Cycle 5 follow-up / re-review (post operator-rejection reshape)

Opening: Cycle-4 was my retraction-to-Request-Changes after @tobiu caught three shape flaws I'd missed (magic provider-name const, module-level fn bloating the Orchestrator, ||-fallback + null-reset). All three are now genuinely resolved — and this time I ran the shape battery and the functional battery, verifying by reading the reshaped code and running tests, not assuming.

Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: The three operator blockers are correctly resolved, the reshape is config-driven and tested (89 specs green, CI fully green), so it's mergeable. The shape-scan surfaced one piece of reshape debt (orphaned helper + duplicated max-loop) — real but non-blocking; better folded-in-now-if-quick or tracked, not a merge gate.

Prior Review Anchor

• PR: #12274 / Target: Refs #12264
• Prior Review: pullrequestreview-4397774578 (cycle-4 CHANGES_REQUESTED — the retraction)
• Latest Head SHA: 3debfde70

Delta Scope

• Files changed since cycle-4: ai/services/graph/ProviderReadinessHelper.mjs (+buildLmsPreloadConfig), ai/services/graph/providerDispatch.mjs (+isOpenAiCompatibleProvider), ai/daemons/orchestrator/Orchestrator.mjs (delegation), test updates. Commits b1f7488f6 (move policy) + 3debfde70 (remove fallback).
• Branch freshness / merge state: clean; CI fully green (gh pr checks: unit, integration-unified, check, CodeQL, Analyze, lint-pr-body all pass).

Previous Required Actions Audit

• Addressed — RA1 (magic provider-name const): the OPEN_AI_COMPATIBLE_PROVIDER = 'openAiCompatible' literal is gone from the Orchestrator. Locality is now decided via isOpenAiCompatibleProvider(role.provider) imported from providerDispatch.mjs — the canonical provider-identity module that already owns the frozen GRAPH_MODEL_PROVIDERS registry + the dispatch switch. The provider-name knowledge lives at its SSOT, not as a consumer-side magic value.
• Addressed — RA2 (orchestrator bloat / module-level fn): buildLmsPreloadConfig now lives in ProviderReadinessHelper.mjs (a pure helper module, no extends Base/setupClass — module-level functions are correct there), alongside buildLmsContextLengthsMap. The Orchestrator carries only thin delegators (get lmsPreloadConfig() / get lmsModels()).
• Addressed — RA3 (null-reset → config-driven selection): the config.modelProvider || config.chatProvider ||-fallback and the usesLocalChat ? … : null null-reset are gone. The new shape builds a roles array keyed on each role's own selector (modelProvider/graphProvider/embeddingProvider) and .filter(role => isOpenAiCompatibleProvider(role.provider) && role.model). Config picks; code reads the pick by filtering — it never nulls a value or infers locality from a non-null model leaf. The JSDoc documents exactly that intent ("must never infer activity from non-null model leaves").

Delta Depth Floor

Delta challenge (the shape-scan working): running the config-SSOT/code-quality scan on the reshape surfaced two items the functional pass wouldn't:

1. (Non-blocking, worth folding in) Orphaned helper + duplicated logic. buildLmsContextLengthsMap is now referenced only by its own tests in production (grep finds it only in ProviderReadinessHelper.mjs), while buildLmsPreloadConfig re-inlines the same max-per-model loop. The reshape left the old helper stranded and duplicated its core logic. Cleaner: have buildLmsPreloadConfig reuse buildLmsContextLengthsMap for the max-merge (or remove the now-dead helper). Given @tobiu's debt-aversion, fold it in this cycle if it's quick; otherwise it's a clean follow-up.
2. (Non-blocking nit) config.openAiCompatible || {} null-guard — benign (guards an optional config object's absence; not a tunable-default substitution or config-pick like the RA3 case), but config.openAiCompatible?.model would avoid even that ||.

I verified the canonical predicate (isOpenAiCompatibleProvider reads provider === 'openAiCompatible' at the dispatch SSOT, fed by the frozen GRAPH_MODEL_PROVIDERS registry) and ran the functional battery rather than trusting the grep.

Test-Execution & Location Audit

• Changed surface class: code + test updates.
• Related verification run: UNIT_TEST_MODE=true npx playwright test … config.template.spec + Orchestrator.invariants.spec + daemon.spec + ProcessSupervisorService.spec + runSandman.spec → 89 passed (2.3s). The role-aware derivation, remote-exclusion, same-model max-cap, and degraded-readiness behavior all hold under the reshape.
• Findings: pass.

Contract Completeness Audit

• Findings: N/A — unchanged from cycle-3: bug-fix over pre-existing provider-role configs; behavior contract captured in JSDoc + tests. No new consumed surface.

Metrics Delta

• [ARCH_ALIGNMENT]: 45 -> 85 — All three config-SSOT/shape violations resolved (canonical provider predicate, helper relocation, config-driven selection reading the pick). 15 residual: the orphaned buildLmsContextLengthsMap + duplicated max-loop + benign || {}.
• [CONTENT_COMPLETENESS]: 88 -> 92 — buildLmsPreloadConfig JSDoc is excellent and encodes the operator's "don't infer from non-null model leaves" rule at the code site. 8 residual: transient nits.
• [EXECUTION_QUALITY]: 55 -> 85 — Correct config-driven reshape verified by reading + 89 green specs. 15 residual: the reshape's orphan/duplication debt.
• [PRODUCTIVITY]: 80 -> 88 — Three RAs resolved cleanly across two fast cycles.
• [IMPACT]: 70 — unchanged from prior review.
• [COMPLEXITY]: 75 — unchanged from prior review.
• [EFFORT_PROFILE]: Heavy Lift — unchanged; reshape on top of the cycle-3 surface.

Required Actions

No blocking required actions — eligible for human merge.

Non-blocking follow-up (fold in this cycle if quick, else track): resolve the buildLmsContextLengthsMap orphan + the duplicated max-loop in buildLmsPreloadConfig (reuse the helper or remove it).

The reshape is exactly right — locality is now read from the provider-role selectors via the canonical predicate, with no magic value, no || fallback, and no null-reset. Thank you for the fast, clean iterations across a rough review history on my side. Approved for @tobiu's merge; the orphan/duplication is the only loose thread. — neo-opus-ada

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 6 — micro-delta confirmation (circuit-breaker shape; 5 prior cycles, semantic approval standing from cycle-5)

Opening: Re-affirming my cycle-5 approve against head f4e5a2641 — the fold-in resolves both cycle-5 non-blocking items, so the approve is strengthened, not just carried.

Strategic-Fit Decision

• Decision: Approve
• Rationale: The only loose thread from cycle-5 (orphaned helper + duplicated max-loop) is now folded in; nothing new introduced. Mergeable.

Prior Review Anchor

• PR: #12274 / Refs #12264 / Prior: pullrequestreview-4397789438 (cycle-5 Approve+Follow-Up) / Head: f4e5a2641

Previous Required Actions Audit

• Addressed (was non-blocking follow-up) — orphan + duplication: buildLmsPreloadConfig now reuses buildLmsContextLengthsMap for the max-merge (passing each role's model only when that contextRole survived the selector filter) instead of re-inlining the loop. The helper is no longer orphaned and the duplication is gone.
• Addressed (was nit) — || {} → ?? {}: the null-guard now only defaults on null/undefined.

Delta Depth Floor

Delta challenge / verification: I traced equivalence of the reuse against the prior inline logic across the three cases — same-model both-local (max cap preserved), chat-local-only (embedding remote → only chat context), and distinct models — all equivalent. Ran the two decisive specs (Orchestrator.invariants.spec + runSandman.spec, which cover buildLmsPreloadConfig + buildLmsContextLengthsMap + the same-model max-cap) → 53 passed. CI fully green at f4e5a2641.

Test-Execution & Location Audit

• Related verification run: the two decisive specs → 53 passed; CI all-green.
• Findings: pass.

Metrics Delta

• [ARCH_ALIGNMENT]: 85 -> 92 — orphaned helper now reused; duplication removed; ?? over ||. The remaining gap is only the pre-existing vestigial orchestrator.lms.model fallback, out of this PR's scope.
• [CONTENT_COMPLETENESS]: 92 — unchanged from cycle-5.
• [EXECUTION_QUALITY]: 85 -> 92 — duplication eliminated via verified helper reuse; specs green.
• [PRODUCTIVITY]: 88 — unchanged from cycle-5.
• [IMPACT]: 70 — unchanged.
• [COMPLEXITY]: 75 — unchanged.
• [EFFORT_PROFILE]: Heavy Lift — unchanged.

Required Actions

No required actions — eligible for human merge.

Clean close. All three operator blockers plus both my follow-up nits are resolved, verified. — neo-opus-ada