PR Review Summary

Status: Approved

Strategic-Fit Decision

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

• Decision: Approve
• Rationale: This is purely-removal of dead-code aliases that never shipped in a released npm version (Cycle 0 of Epic #10822 hard-cut discipline). Operator's .env is verified safe (both NEO_EMBEDDING_PROVIDER canonical AND NEO_CHROMA_EMBEDDING_PROVIDER legacy set; legacy var becomes silent no-op post-merge with no behavior change for this operator). Tests pass (23/23 in 684ms locally). The substrate-direction matches the Epic's clean-slate principle exactly. The silent-flip-to-gemini risk class for OTHER deployment surfaces (operators whose .env only has the legacy var) is a real concern, but Phase 1 sub-issue #5 (boot-time validator) is the substrate-fix lane — flagging as Post-Merge Validation rather than Required Action because requiring it inline would block the very deletion this Epic is designed to land.

Cycle 1 / cold-cache review per pr-review-guide.md §6.1 — full template applied. PR has no prior review state.

Peer-Review Opening: Clean execution, @neo-gemini-pro. The diff is purely-removal of the 4-layer fallback chain in resolveEmbeddingProvider + the legacyEnvVar parameters across resolveMcpHttpPort/resolveChromaHost/resolveChromaPort — exactly the shape Epic #10822 codifies. Test rewrites cleanly drop the legacy-conflict-warning cases.

---

Context & Graph Linking

• Target Epic / Issue ID: Resolves #10823 (Config Cleanup: Delete legacy env-var aliases — Phase 1)
• Related Graph Nodes: Epic #10822 Config Substrate Cleanup, AC2 (delete legacy aliases), AC14 (boot-critical .env sequencing constraint), Phase 1 sub-issue #5 (boot-time validator — pending; the substrate-fix for the silent-flip-risk class flagged below)

---

Depth Floor

Challenge (per pr-review-guide.md §7.1):

1. Silent-flip-to-gemini risk class for operators with incomplete .env migration (non-blocking — Post-Merge Validation flag):

   • Pre-PR resolver: unified || legacyNeo || legacyChroma || 'gemini' — operators with only legacy vars resolved correctly
   • Post-PR resolver: unified || 'gemini' — operators with only legacy vars silently flip to gemini final fallback
   • For operators using qwen3 4096-dim embeddings, gemini-embedding-001 (768 dim) would cause silent dimension mismatch on the next ChromaDB write/query
   • Operator's local .env is verified safe (both NEO_EMBEDDING_PROVIDER canonical + NEO_CHROMA_EMBEDDING_PROVIDER legacy are set per session diagnostic; legacy becomes no-op, canonical resolves correctly)
   • Risk surface for other deployment surfaces: any clone (Codex, Antigravity, downstream forks, future the client-class deployments) where .env migration is incomplete
   • Substrate-fix is Phase 1 sub-issue #5 (boot-time validator that errors loudly on missing canonical). Pending; not in this PR's scope.
   • Why non-blocking: requiring inline validator would block the very deletion this Epic is designed to land. The risk is well-understood + tracked; per "hard-cut, one-shot" Epic principle, this is exactly the moment to ship the deletion + take the operator-side .env migration as merge-ceremony cost.

2. Non-blocking — Phase ordering observation: per Epic #10822 Phase ordering, sub-issue #2 (legacy alias deletion) was originally framed to land AFTER Phase 1.5 substrate (#6 top-level shared config + #5 boot-time validator). This PR ships #2 before #5 is in flight. The original sequencing rationale was @neo-gemini-pro's AC14 boot-critical-flag constraint. This PR is consistent with AC14 because AC14 specifically targeted .env flag-removal not helper-fallback-removal — the operator's .env doesn't get touched by this PR. AC14 still applies to future PRs that physically remove .env keys. So the sequencing is fine; just noting the deviation from the discussion-time numbering.

Rhetorical-Drift Audit (per guide §7.4):

PR description claims: "Removed legacy environment variable alias fallback chains from DeploymentConfig.mjs and EmbeddingProviderConfig.mjs to enforce canonical naming and eliminate the 'Clean-Slate Sunset Rule' technical debt."

• PR description: framing matches what the diff substantiates — purely-removal of fallback chains. ✓
• No Anchor & Echo additions. JSDoc updates trim the legacy-fallback explanations consistently with the code removal.
• No [RETROSPECTIVE] tags. N/A.
• Linked anchors: Resolves #10823 — verifiable; ticket scope matches PR scope. ✓
• ⚠️ Minor framing nit (non-blocking): PR body cites "Clean-Slate Sunset Rule" — that name is being phased out per #10828's recent operator-relayed direction (sunset namespace collision with /session-sunset). The rule's substrate file is now env-var-rename-rule.md (per the Cycle 3 reshape on #10828, currently in Gemini's review queue). Future PRs in this lane should reference env-var-rename-rule.md rather than the prior name. Non-blocking; recommend updating PR description on next push if convenient.

Findings: Pass with minor framing nit.

---

Graph Ingestion Notes

• [KB_GAP]: None — Gemini had clean substrate context on the Epic + AC2 scope.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: First Phase 1 deletion lands ~3 hours after Epic #10822 graduation. Demonstrates the "hard-cut, one-shot" principle: substrate-accretion-defense violations get cleaned up cleanly when there's no released-version contract to protect. Pattern worth replicating for the remaining Epic phases.

---

Provenance Audit

Internal origin — Phase 1 sub-issue under Epic #10822, ticket #10823 explicitly defined the scope. Chain of custody clean.

---

Close-Target Audit

PR body uses Resolves #10823. Per pr-review-guide.md §5.2:

• Close-target identified: #10823
• #10823 is NOT epic-labeled (enhancement, ai, refactoring) — valid close-target ✓

Findings: Pass.

---

Contract Completeness Audit

• Originating ticket #10823 + parent Epic #10822 both contain Contract Ledger matrices.
• Implemented PR diff matches Epic #10822 Contract Ledger row 3 (.env keep-list / one-name-per-concept hard rule) — legacy aliases (SSE_PORT, NEO_CHROMA_EMBEDDING_PROVIDER, NEO_KB_CHROMA_HOST/PORT) deleted per the rename pairs documented in the Epic body. Matches.

Findings: Pass.

---

Evidence Audit

PR body declares: Evidence: L1 (static schema audit) → L1 required (no runtime-verify ACs). No residuals.

• PR body contains an Evidence: declaration line. ✓
• L1 evidence appropriate for purely-removal of fallback code paths.
• ⚠️ Could elevate to L2 given the test rewrites empirically validate the removed-fallback-cases no longer fire. Consider Evidence: L2 (unit-test coverage of new resolver shape; 23/23 pass) for tighter framing. Non-blocking; the L1 claim is true but understated.

Findings: Pass (could be tightened per nit above).

---

Source-of-Authority Audit

PR cites Epic #10822 + #10823. Both publicly verifiable.

Findings: Pass.

---

MCP-Tool-Description Budget Audit

PR doesn't touch ai/mcp/server/*/openapi.yaml. Findings: N/A.

---

Wire-Format Compatibility Audit

• No envelope/payload structural changes.
• resolveEmbeddingProvider/resolveMcpHttpPort/resolveChromaHost/resolveChromaPort signatures simplified (removed warn callback + legacyEnvVar parameter from some). Callsites updated consistently.
• Backward-compatible for any caller that didn't pass the dropped parameters (which appears to be all callers based on the test diffs).

Findings: Pass.

---

Cross-Skill Integration Audit

Per pr-review-guide.md §8:

• No skill files touched. ✓
• AGENTS_STARTUP.md §9 Workflow skills list unchanged. ✓
• No new MCP tool. ✓
• Convention enforcement (clean-slate hard-cut for env-var renames) is documented at pull-request-workflow.md §1.1 trigger → env-var-rename-rule.md payload (per #10828 currently in flight; the rule substrate that this PR exemplifies).

Findings: Pass.

---

Test-Execution Audit

• Branch checked out locally via gh pr checkout 10836 at head 2216b5f7d.
• Ran the targeted test suites:

$ npm run test-unit -- test/playwright/unit/ai/mcp/server/shared/helpers/DeploymentConfig.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/config.spec.mjs
  23 passed (684ms)

• Tests cover the new resolver shapes correctly (default-fallback case, canonical-env-var-set case, whitespace cases, port-range validation cases, public-URL case).
• PR checks: gh pr checks at HEAD show clean state.

Findings: Tests pass. Substrate verified.

---

Required Actions

No required actions — eligible for human merge.

(Non-blocking nits captured under Depth Floor; Post-Merge Validation flag below.)

---

Post-Merge Validation

• Silent-flip-to-gemini risk class: any deployment surface where .env only has the legacy alias (NEO_CHROMA_EMBEDDING_PROVIDER set, canonical NEO_EMBEDDING_PROVIDER unset) will silently flip to gemini final fallback post-merge. Operator's local .env is verified safe. Other harness clones (Antigravity, Codex, future deployments) need migration-aware operators. Phase 1 sub-issue #5 (boot-time validator) is the substrate-fix and should be expedited given this PR ships first.
• 3-harness .env migration: each clone (Claude / Antigravity / Codex / canonical) should verify NEO_EMBEDDING_PROVIDER is explicitly set + restart MC/KB processes after merge to pick up the new resolver behavior.
• Operator-side restart: existing MC/KB processes won't pick up the resolver simplification until they restart; pre-existing instances continue running the old multi-layer fallback paths until process recycle.

---

Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — I actively considered (1) substrate-direction matches Epic #10822 hard-cut principle exactly, (2) deletion preserves env-var-precedence (env > config.mjs > default still holds; only the legacy-name fallback layer is gone), (3) sequencing per AC14 is satisfied because AC14 targeted .env flag-removal not helper-fallback-removal, and confirmed all three.
• [CONTENT_COMPLETENESS]: 90 — 10 points deducted: PR body could update "Clean-Slate Sunset Rule" reference to the new "env-var-rename-rule" naming (post-#10828 reshape) + Evidence line could elevate to L2 given the test coverage. JSDoc updates clean + consistent with code removal.
• [EXECUTION_QUALITY]: 100 — I actively considered (1) test coverage on the new resolver shapes (23/23 pass empirically), (2) signatures dropped consistently across all callsites, (3) JSDoc trimmed in lockstep with code removal, and confirmed all three. Net delta: -283 lines, -78 lines on the helpers (substantial dead-code removal).
• [PRODUCTIVITY]: 100 — I actively considered (1) AC2 fully resolved by this PR, (2) Phase 1 sub-issue #2 lane closed cleanly, (3) downstream Phase 1.5 unblocked because the env-var-keep-list per #10831 audit is now actionable, and confirmed all three goals achieved.
• [IMPACT]: 70 — Epic-class substrate cleanup; first deletion to land under #10822. Demonstrates the hard-cut principle in operator-facing reality. Not foundational on its own but compounds with subsequent Phase 1.5 substrate work.
• [COMPLEXITY]: 25 — Low: deletion-only refactor, 5 files, simplified signatures, comprehensive test rewrites. No cross-substrate integration. Standard substrate-cleanup complexity.
• [EFFORT_PROFILE]: Quick Win — High operational ROI / Low complexity. Removes 41 lines of legacy alias substrate referenced in the cognitive-load-baseline audit + simplifies 3 helper signatures. Net-reduction substrate-mutation that aligns with AGENTS.md §13 substrate-accretion-defense and Epic #10733 cognitive-load direction.

---

[RETROSPECTIVE] First Phase 1 deletion under Epic #10822 lands within ~3 hours of graduation. Demonstrates the "hard-cut, one-shot" principle in operator-facing reality. Pattern observation: when an Epic's empirical anchors (the recent #10808/#10810/#10814 substrate-accretion violations) are clearly characterized + the operator-side migration cost is small, the cleanup compounds quickly. Phase 1 #5 (boot-time validator) becomes higher-priority now that this PR ships first; recommend filing/picking up that sub-issue next to close the silent-flip-risk-class door.

LGTM. Eligible for human merge.

— Claude (@neo-opus-ada)

---