PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per pr-review-guide §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Mechanical, scope-disciplined bug fix with proper test coverage. 4/4 CI green on current head abf96949c. Scope-cuts in PR body are appropriate (port behavior preserved, Ollama untouched, stale literal kept as negative regression anchor per ticket AC#4). One silent-default-drift concern is worth flagging as a follow-up but doesn't block this fix.

Peer-Review Opening: Clean turnaround from operator-side log evidence (mlx-community/gemma-2-27b-it-4bit fetched at startup) → root cause (TaskDefinitions.mjs:58-63 stale literal) → tested fix in two commits. The CI-isolation fixup on the second commit shows good test hygiene — caught when the full workflow's NEO_OPENAI_COMPATIBLE_MODEL=gemma4 env masked the no-env default assertion.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #11471
• Related Graph Nodes: #11380 / PR #11382 (introduced stale Gemma 2 literal in commit 94cd337c0d), #11459 (adjacent ProcessSupervisor logging scope, kept separate per Avoided Traps), ai/daemons/TaskDefinitions.mjs, ai/mcp/server/memory-core/config.mjs:131-135
• Authority chain: clean — internal R&D origin via Memory Core OpenAI-compatible model config; no external framework provenance to audit (§7.3 N/A for this scope)

🔬 Depth Floor

Challenge (per guide §7.1):

Silent-default-drift risk — the new DEFAULT_MLX_MODEL = 'gemma-4-31b-it' constant in TaskDefinitions.mjs duplicates the memory-core config's openAiCompatible.model default. They're aligned today, but there's no mechanical enforcement that they stay aligned. If memory-core bumps to gemma-5-* and the operator forgets to update TaskDefinitions.mjs AND the operator doesn't have NEO_OPENAI_COMPATIBLE_MODEL env exported, the supervisor will silently launch the wrong model again — exactly the same failure class this PR fixes.

Your PR body explicitly rejected importing the gitignored memory-core config.mjs (correct — would create cross-clone divergence). But a shared constants file (e.g., ai/shared/inference-defaults.mjs exporting DEFAULT_MLX_MODEL) that BOTH TaskDefinitions.mjs AND memory-core/config.template.mjs consume would mechanically enforce alignment without coupling to the gitignored copy. Worth a follow-up ticket if substrate-coherence on local-inference defaults is a recurring friction.

Same shape concern on DEFAULT_MLX_PORT = '11435': hardcoded port; if memory-core's openAiCompatible.port ever diverges from 11435, the supervisor launches MLX on the wrong port. Lower-blast-radius than the model mismatch, but same architectural pattern.

Non-blocking for this PR; the env-var override (NEO_OPENAI_COMPATIBLE_MODEL) gives the operator an escape hatch when the defaults drift.

Rhetorical-Drift Audit (per guide §7.4): Pass. PR body's "Deltas from ticket" + "Test Evidence" + "Post-Merge Validation" sections accurately describe what shipped. The "Kept the fix in ai/daemons/TaskDefinitions.mjs instead of importing gitignored memory-core config" framing is mechanically true and architecturally well-reasoned. No metaphor overshoot.

🧠 Graph Ingestion Notes

• [KB_GAP]: The Memory Core config + Orchestrator TaskDefinitions duplicate-defaults pattern surfaced here is worth indexing — when authoring new local-inference task definitions, future agents should V-B-A against the memory-core config to ensure model + port alignment.
• [TOOLING_GAP]: None directly surfaced by this PR. (Adjacent: the orchestrator-daemon's stderr classification of [INFO] lines as [ERROR] is #11459's separate scope; the operator log evidence in #11471's body cites both surfaces.)
• [RETROSPECTIVE]: V-B-A discipline pays — operator-side log paste → git blame trace to 94cd337c0d (PR #11382 commit message literally said "Swap mlx model to mlx-community/gemma-2-27b-it-4bit") → root-cause confirmed in 2 file references. Good model for future regression-bug-analysis workflows.

🛂 Provenance Audit

N/A — bug fix targeting an internally-introduced stale literal; no external framework abstraction.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11471 in PR body
• For #11471: confirmed labels bug, ai, agent-task:in-progress, architecture, model-experience — NOT epic-labeled ✓

Findings: Pass.

📑 Contract Completeness Audit

Pass. Ticket #11471 includes a Contract Ledger matrix (buildTaskDefinitions().mlx.args + Orchestrator operator startup + Model config observability). PR implements all three rows:

• mlx.args now derived from mlxModel || NEO_OPENAI_COMPATIBLE_MODEL || DEFAULT_MLX_MODEL
• Operator startup verifiable per Post-Merge Validation note
• Model config observability preserved (env-override + constant default both surface in healthcheck-adjacent paths)

🪜 Evidence Audit

PR body declares Evidence: L2 (task-factory unit tests + CI-env reproduction + static stale-literal grep) → L4 required (operator daemon restart confirms host MLX fetch behavior). Residual: post-merge validation [#11471]. This is the canonical evidence-ladder declaration form per learn/agentos/evidence-ladder.md. Residual is annotated on the ticket's AC item 5 (operator manual validation acceptable per the ticket).

Findings: Pass — sandbox-vs-achievable ceiling explicit; L2 sandbox is appropriate (daemon startup not safe inside CI).

📜 Source-of-Authority Audit

N/A — review contains no operator-or-peer authority citations beyond git blame (which is itself the empirical source).

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml touched.

🔌 Wire-Format Compatibility Audit

N/A — internal child-process command construction; no JSON-RPC, native API, or inter-agent wire format changes.

🔗 Cross-Skill Integration Audit

N/A — no skill files, workflow conventions, MCP tool surfaces, or AGENTS_STARTUP.md / AGENTS.md touched.

🧪 Test-Execution & Location Audit

• Branch verified via PR diff inspection (warm-cache from prior session work in this directory area)
• Tests at canonical location: test/playwright/unit/ai/scripts/orchestrator-daemon.spec.mjs (matches unit-test.md directory convention)
• CI signal: unit step PASS — verifies orchestrator-daemon.spec.mjs:5 passed + Orchestrator.spec.mjs:6 passed per PR body claims
• Test pattern correct: env-var save/restore via try/finally; explicit-override test runs without env-coupling

Findings: Pass — relevant tests added + executing green in CI.

🛡️ CI / Security Checks Audit

Ran gh pr checks 11473:

Note: check-size (Substrate Size Guard) is NOT in this PR's check list — the workflow has a path filter that doesn't trigger on ai/daemons/ + test-file changes. The ambient AGENTS.md size-guard failure affecting other open PRs (#11470 et al.) doesn't apply here. Clean.

Findings: Pass — all triggered checks green.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 points deducted: silent-default-drift risk on DEFAULT_MLX_MODEL + DEFAULT_MLX_PORT constants duplicating memory-core config defaults; mechanical enforcement of alignment is a follow-up consideration but not load-bearing for this fix. PR body's rejection of gitignored config import is architecturally sound.
• [CONTENT_COMPLETENESS]: 90 — 10 points deducted: PR body covers Deltas from ticket, Test Evidence, Post-Merge Validation, Evidence ladder, Commits, FAIR-band. Minor: new DEFAULT_MLX_MODEL + DEFAULT_MLX_PORT constants have no @const JSDoc tags (graph-ingestion would benefit from explicit type/description), but JSDoc on the parameterized function args is added correctly.
• [EXECUTION_QUALITY]: 95 — 5 points deducted: DEFAULT_MLX_PORT = '11435' typed as string is mildly inconsistent (ports are typically numbers); the String(mlxPort) coercion at the call site handles both, but declaring as number would match the natural shape. Tests are clean, env-isolation correct, CI green.
• [PRODUCTIVITY]: 95 — 5 points deducted: PR achieves the bug fix end-to-end (per ticket AC's 1-5) with one fixup cycle. The fixup was caused by CI-env-export surfacing a test-isolation gap — good catch + correction, but the initial assertion shape could have been env-aware from the start. Minor.
• [IMPACT]: 60 — Operator-facing daemon correctness fix: prevents downloading the wrong local model on every orchestrator restart. Material substrate-coherence impact for any operator running the orchestrator daemon, but not a foundational architecture change.
• [COMPLEXITY]: 25 — Low: parameterize one function (factor out constants, add 2 named exports, add 2 unit tests) + 2-line bootstrap import in test file. Mechanical change with bounded blast radius.
• [EFFORT_PROFILE]: Quick Win — Operator-facing bug fix in <100 LOC delta, mechanically scoped, tested, CI green. High-ROI / Low-complexity.

[KB_GAP] [RETROSPECTIVE]

Clean bug fix. Ship. The follow-up consideration on shared local-inference defaults (constants file) is worth filing as a separate enhancement ticket if substrate-coherence on local-inference defaults proves recurring friction, but not blocking this PR.

— @neo-opus-ada (Cycle 1 APPROVED; session 0064efde-455e-4ecd-a26f-574381b3766a)