PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Surgical preflight at the embedding/query boundary, well-bounded scope, full Contract Ledger alignment (with one explicit deferral the ledger itself enumerates as an alternative). 47/47 tests pass empirically, CI clean. The substrate-evolution surface is contained (one daemon + 3 fallback defaults + 3 docs); risk of further iteration outweighs the marginal-value of expanding scope to e.g. add actualDimensions live-smoke at healthcheck-time (better tracked as follow-up). Approve+Follow-Up wasn't needed — the ledger explicitly accepts the "or documents why live smoke is required" alternative.

Peer-Review Opening: Solid bug-fix for the operator-reported 2026-05-18 Sandman failure. Preflight pattern is the substrate-correct shape: refuse the Chroma query before raw shape-error, name the configured-vs-actual dimensions in the warning, name the remediation env-vars. Clean separation of concerns via the 3 exported pure helpers (testability + boundary clarity). The default-tuple realignment (gemini/3072 → openAiCompatible/Qwen3/4096) closes the internally-inconsistent-default subclass of the same bug. Approved on substrate; non-blocking follow-up concerns surfaced below.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11596
• Related Graph Nodes: #10003 (prior unified embedding boundary), #10723 (NEO_VECTOR_DIMENSION contract), #10804 (NEO_EMBEDDING_PROVIDER deprecation), GoldenPathSynthesizer, HealthService, EmbeddingProviderConfig

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Follow-up concern 1 (non-blocking, metadata): PR body's FAIR-band declaration reads [11/30 — current author count over last 30 merged], but canonical gh search prs --merged --repo neomjs/neo --limit 30 --sort updated --json author query returns neo-gpt: 10/30 for the current 30-PR window. Possible drift if --author neo-gpt filter was used instead of the canonical population query. Still in-band per FAIR ±3 from 10; not a blocker, but worth aligning the FAIR-band declaration discipline in the next cycle. (Reference: fair-band-pre-flight-gate.md canonical query.)
• Follow-up concern 2 (architectural, non-blocking): The PR explicitly defers live actualDimensions exposure at healthcheck-time, documented in MemoryCore.md + SharedDeployment.md per the ledger's "or documents why" alternative. The deferred path remains a future-improvement candidate — e.g. Golden Path could cache the last observed actualEmbeddingDimension on a Memory Core singleton that the healthcheck reads (avoids fresh provider calls at health-poll time). Better tracked as a separate ticket than scope-creeping this PR. Surfaces only if/when operator UX warrants drift detection without manual Sandman re-run.
• Edge case (acknowledged, not a concern): frontierEmbedding === null (provider call failure path before the preflight). getEmbeddingVectorLength(null) → null; !Number.isInteger(null) short-circuits the guard to log + return. Null-safe by construction.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot). "Preflight validates ... names provider, model, configured dimension, actual dimension, and remediation" — verified verbatim in buildEmbeddingDimensionMismatchMessage.
• Anchor & Echo summaries: precise codebase terminology in all 3 new helper @summaries; no metaphor overshoot.
• [RETROSPECTIVE] tag: none used in this PR body; N/A.
• Linked anchors: #10003 and #10723 citations accurately establish the prior boundary work + dimension contract.

Findings: Pass — substrate-grounded framing throughout.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework concepts misunderstood; the preflight pattern leverages existing aiConfig.vectorDimension + provider-output coherence semantics.
• [TOOLING_GAP]: N/A — no tooling failures during PR generation.
• [RETROSPECTIVE]: Default-tuple realignment (gemini → openAiCompatible as the unified-4096 source default) is the under-stated half of this fix. Internally-inconsistent defaults are a class of bug that emerges silently in self-bootstrapped clones; the Golden Path runtime guard catches them at query-time, but the template default change prevents the class entirely for fresh clones. Both halves are load-bearing.

🛂 Provenance Audit

N/A — bug fix on existing substrate boundary, not a major architectural abstraction or new core subsystem (§7.3 threshold not met). The preflight pattern is internal substrate-evolution from operator-V-B-A, not an external-framework concept.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: Resolves #11596 (single, isolated on own line per Syntax-Exact Keyword Mandate)
• For each #N: confirmed not epic-labeled — #11596 labels are ["bug","ai","regression","architecture"]; no epic

Commit body audit (per §5.2 squash-merge commit-body hazard #11185): single commit c026a8ab4 with subject fix(memory-core): guard Golden Path embedding dimensions (#11596); no stale Closes/Resolves/Fixes survivors in the body.

Findings: Pass — syntax-compliant, validity-compliant, no commit-body stale survivors.

📑 Contract Completeness Audit

Originating ticket #11596 contains a Contract Ledger matrix with 3 target surfaces:

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift) — the partial-deferral on surface 2 is explicitly enumerated as an acceptable alternative in the ledger surface itself ("or documents why live smoke is required")

Findings: Pass — full ledger alignment.

🪜 Evidence Audit

The PR body contains the explicit Evidence declaration:

Evidence: L2 (focused Playwright unit specs plus syntax/diff hygiene checks) → L3 required (post-merge Sandman smoke against the live provider + Chroma collection). Residual: post-merge Sandman run [#11596].

• PR body contains an Evidence: declaration line
• Achieved evidence (L2) is consistent with what's reachable from the agent sandbox; the L3-required gap (live Sandman + Chroma) is explicitly named as residual and folded into the ## Post-Merge Validation checklist
• Residual is annotated against close-target #11596 (the post-merge Sandman validation step)
• Two-ceiling distinction: PR body distinguishes "shipped at L2 because sandbox ceiling" (unit specs + static checks) from the L3-required live-provider/live-Chroma smoke ("post-merge Sandman run")
• Evidence-class collapse check: review prose does NOT promote L2 to L3 framing; the live Sandman smoke remains explicitly deferred

Findings: Pass — Evidence declaration and residual tracking are substrate-correct.

📜 Source-of-Authority Audit

N/A — no operator or peer authority citations in this review. All claims grounded in: empirical test run (47/47 PASS), git diff inspection, Contract Ledger comparison, canonical FAIR-band query.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml files modified in this PR.

🔌 Wire-Format Compatibility Audit

N/A — preflight guard is internal to GoldenPathSynthesizer; no JSON-RPC notification schemas, payload envelopes, or inter-process wire-formats altered. The buildEmbeddingProviderBlock healthcheck output shape is unchanged (only the fallback default key flipped).

🔗 Cross-Skill Integration Audit

N/A — no skill files modified, no new MCP tool surfaces, no AGENTS_STARTUP.md/AGENTS.md changes, no new architectural primitive. The 3 modified learn/agentos/*.md files are operator/agent reference docs (not skill substrate); they're updated to reflect the new default + Golden Path log-on-mismatch behavior — fact-aligning existing docs is not cross-skill integration in the §8 sense.

🧪 Test-Execution & Location Audit

• Branch checked out locally via temporary worktree overlay at PR head c026a8ab4
• Canonical Location: test/playwright/unit/ai/daemons/services/GoldenPathSynthesizer.spec.mjs + test/playwright/unit/ai/services/memory-core/HealthService.spec.mjs — both under test/playwright/unit/ai/* per unit-test.md canonical path
• Empirical execution at PR HEAD: npm run test-unit -- GoldenPathSynthesizer.spec.mjs → 2/2 PASS (859ms) (including the new synthesizeGoldenPath skips Chroma query when embedding dimension mismatches vectorDimension regression test); npm run test-unit -- HealthService.spec.mjs → 45/45 PASS (712ms) (including PR's updated unset embeddingProvider defaults provider to openAiCompatible test + new EmbeddingProviderConfig #11596 — resolveEmbeddingProvider defaults to openAiCompatible test)

Symmetric beforeEach/afterEach setup hardening (save+restore aiConfig.embeddingModel/embeddingProvider/vectorDimension/logger.warn) noted — matches feedback_symmetric_spec_cleanup discipline; prevents fullyParallel cross-test pollution on the singleton aiConfig.

Findings: All related tests pass on PR HEAD empirically. Location canonical.

🛡️ CI / Security Checks Audit

• Ran gh pr view 11613 --json statusCheckRollup to empirically verify CI status
• Confirmed no checks pending/in-progress
• Confirmed no checks failing — all green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit

Findings: Pass — all CI/security checks green on head c026a8ab4.

📋 Required Actions

No required actions — eligible for human merge.

The two follow-up concerns surfaced in §7.1 Depth Floor (FAIR-band declaration drift, healthcheck actualDimensions exposure) are non-blocking observations. Per feedback_blocker_reserved_for_merge_breaking, neither blocks merge — both are calibration/architectural-extension candidates better tracked as separate work.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 points deducted because the 3 exported pure helpers (getEmbeddingVectorLength, getEmbeddingModelName, buildEmbeddingDimensionMismatchMessage) live alongside the daemon class rather than in a shared helper module; acceptable as-is for testability + locality, but a candidate for extraction long-term if other call-sites need the same primitives. Otherwise substrate-correct: pure helpers, surgical preflight at boundary, no anti-patterns.
• [CONTENT_COMPLETENESS]: 95 — 5 points deducted because the existing synthesizeGoldenPath method JSDoc doesn't reference the new dimension-guard semantics (the 3 new helpers carry @summary, but the consuming method's JSDoc is unchanged). New helpers, new test, and updated learn/agentos docs all carry Anchor & Echo terminology consistent with the codebase. Fat Ticket present.
• [EXECUTION_QUALITY]: 100 — I actively considered (1) null-safety of frontierEmbedding === null paths, (2) singleton aiConfig mutation pollution across fullyParallel workers, (3) interaction with prior #10003 unified boundary work, and confirmed none apply or are correctly handled. 47/47 tests PASS empirically on PR HEAD; CI clean; symmetric beforeEach/afterEach setup harden the previously-unprotected aiConfig mutation.
• [PRODUCTIVITY]: 95 — 5 points deducted because AC 3 of #11596 ("Unit or healthcheck test surfaces actual active embedding length for at least one provider path") was satisfied via the alternative path ("or documents why live smoke is required") rather than the primary path. Ledger explicitly enumerates the alternative, so this is substrate-correct rather than a miss — but tracking the 5-point gap acknowledges the primary AC remains aspirational.
• [IMPACT]: 70 — Fixes a real production-observable bug (operator-reported 2026-05-18 Sandman failure) on the Golden Path retrieval boundary. Bounded scope (single subsystem, single failure mode) but load-bearing on the Sandman / Memory Core read path. Realigns the default-tuple coherence invariant across config.template.mjs + 2 fallback resolvers, which is a substantive cross-substrate-default fix.
• [COMPLEXITY]: 45 — Low-medium: one preflight gate + 3 pure helpers + a default-tuple realignment + 3 docs syncs. No new cross-substrate integration, no novel reasoning patterns. Well-contained inside GoldenPathSynthesizer.synthesizeGoldenPath() boundary + 2 fallback resolvers; cognitive load to internalize the change is bounded.
• [EFFORT_PROFILE]: Quick Win — High ROI (closes a production-observable failure mode on a load-bearing daemon) at low complexity (bounded preflight + default alignment). Tight test coverage and Contract Ledger alignment make this a model "Quick Win" execution shape.

Approved. Substrate-grounded fix with empirical evidence at sandbox ceiling + explicit deferral of post-merge L3 smoke. The follow-up concerns are non-blocking; merge-eligible at human-operator discretion per §0 Inv 1.