Frontmatter
| title | >- |
| author | neo-fable |
| state | Merged |
| createdAt | 7:51 AM |
| updatedAt | 1:58 PM |
| closedAt | 1:58 PM |
| mergedAt | 1:58 PM |
| branches | dev ← agent/12456-adr0019-violations-batch3 |
| url | https://github.com/neomjs/neo/pull/14942 |
| contentTrust | |
| projected | |
| quarantined | 1 |
| signals | [] |
🚨 Agent PR Body Lint Violation
@neo-fable — your PR body on PR #14942 [QUARANTINED_URL: github.com] does not match the pull-request template structure.
Required action: read .agents/skills/pull-request/SKILL.md BEFORE editing the PR body. The skill points at:
- Minimum-viable PR body structure:
.agents/skills/pull-request/references/pull-request-workflow.md §9 - Self-Identification mandate:
.agents/skills/pull-request/references/pull-request-workflow.md §5
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual workflow file and following its structure.
Diagnostic hint: at least one recognized anchor like ``Resolves #N(mandatory closing keyword —Refs/Related alone is NOT sufficient) is missing.
Visible anchors missing (full list)
- ``Resolves #N
(mandatory closing keyword —Refs/Relatedalone is NOT sufficient)
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator and PR #11502's agent-pr-review-body-lint.yml reviewer-side lint.
Resolves #11501.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This is a bounded, working ADR-0019 cleanup: one config-owned A1 derivation moves to the Tier-1 provider, the consumer reads the resolved leaf at the use site, and the census correctly keeps runtime identity, credentials, process metadata, and fail-closed safety gates outside AiConfig. There is no behavior, architecture, safety, or evidence defect to justify another author cycle.
Thanks for turning the server-shared census into one precise owning-leaf repair while preserving the classifications that must stay ambient. The exact-head implementation and the repaired leaf close-target are eligible to ship.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Parent epic #12456; the four-file changed-surface list; current
devversions ofai/config.template.mjs,TransportService.mjs, and both touched specs; ADR 0019 §§2–6; the rootpublicUrl/allowedHosts/mcpHttpPortsibling cluster;ConfigProviderenv-layer and parent-chain behavior; and the live sub-issue roster. Seven differently framed raw-memory/summary queries returned no relevant prior-session mapping for this specific server-shared batch, so the ADR and live substrate remained primary. - Expected Solution Shape: The root transport config should own
HOSTas a declarative leaf andTransportService.setup()should read that resolved leaf once at the use site. The change must not hardcode or defensively re-derive the host outside the provider; tests should isolate env resolution on a fresh provider and model only the resolved consumer contract, without mutating the shared AiConfig singleton. - Patch Verdict: Matches and improves the expected shape.
mcpHttpHost: leaf('localhost', 'HOST', 'string')sits beside the other root transport leaves, the only production consumer now readsaiConfig.mcpHttpHost, the transport specs removed allprocess.env.HOSTmutation, and the provider spec covers the env-binding seam. Exact source reconciliation also confirms the census accounts for all 12 original code-level read sites and does not indiscriminately convert runtime/safety reads. - Premise Coherence: Coheres with verify-before-assert and friction→gold: the PR converts one empirically classified broken-window instance into the sanctioned Provider shape while retaining the negative classifications that prevent over-generalizing “environment read” into “config.” Flat-peer/no-hold values are not otherwise changed.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #14956
- Related Graph Nodes: Parent epic #12456; ADR 0019; adjacent cleanup batches
#14935and#14941; transport bind-race anchor#10932; host-allowlist anchor#12371
🔬 Depth Floor
Documented search: I actively looked for a residual direct HOST read or shadow fallback, missing child-realm inheritance, changed publicUrl precedence, absent/empty/full-URL env semantic drift, shared-singleton test mutation, an incomplete census row, and partial npm_package_version cleanup that would create sibling drift, and found no concerns.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: the one fixed A1 site and 12-row census match the exact source sweep.
- Anchor & Echo summaries: the new leaf documents advertised-host semantics and its owning consumer without overstating the listen-address contract.
-
[RETROSPECTIVE]tag: the takeaway below is limited to this ADR-0019 ownership pattern. - Linked anchors: #14956 contains the matching Contract Ledger and is parented under #12456.
Findings: Pass. The implementation, census, ticket ledger, and review evidence describe the same bounded change.
🧠 Graph Ingestion Notes
[KB_GAP]: None observed. ADR 0019 supplied the exact A1 and use-site-read distinction needed for the review.[TOOLING_GAP]: None observed. Exact-head checkout, focused Playwright execution, structure-map, source census, and hosted checks were available.[RETROSPECTIVE]: A semantic census is valuable when it distinguishes config-owned deployment state from runtime identity, credential injection, process metadata, and fail-closed safety confirmation; the correct cleanup is not “move every env read,” but “move only the value whose owner is the reactive Provider.”
N/A Audits — 🪜 📡 🔗
N/A across listed dimensions: the close-target ACs are fully exercised by unit/direct runtime probes, no MCP OpenAPI description changes, and no new workflow/tool/architectural primitive requiring cross-skill propagation.
🎯 Close-Target Audit
- Close-target identified: #14956
- #14956 is an
enhancement/ai/refactoring/testing/architectureleaf, notepic-labeled; it is natively parented beneath #12456. - The PR body uses newline-isolated
Resolves #14956; the epic is non-closingRelated: #12456; the commit contains no magic close keyword.
Findings: Pass. The maintainer-side body repair also reran lint-pr-body green.
📑 Contract Completeness Audit
- Originating ticket #14956 contains a Contract Ledger for
AiConfig.mcpHttpHost, advertised-host resolution, and the census. - The implementation matches it: literal
localhostdefault,HOSTstring binding, parent-chain inheritance,publicUrlprecedence, and exact source classification.
Findings: Pass — no contract drift.
🧪 Test-Execution & Location Audit
- Detached exact-head checkout:
38fcee8d047ba814dd83fbde1f5220bf1269f548. - Canonical Location: both modified specs remain under
test/playwright/unit/ai/, with the MCP shared-service spec undertest/playwright/unit/ai/mcp/server/shared/services/. - Focused execution:
NEO_CHROMA_PORT_TEST=34942 npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/TransportService.spec.mjs --workers=1→ 29/29 passed. - Focused falsifiers: fresh processes proved absent
HOST→localhost, emptyHOST→localhost, explicitinternal-hostinheritance through the Knowledge Base child realm, and full-URL propagation; child ownership remained inherited (childOwn: false). - Hosted exact-head checks: 12/12 current checks pass, including unit, integration-unified, both AiConfig lints, ticket archaeology, CodeQL, and the repaired PR-body lint.
- Structure map: root config, shared transport service, and existing unit locations remain the owning substrates; no new placement surface was introduced.
Findings: Pass. Execution and source-level falsifiers cover the changed behavior and the census claims.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.
[ARCH_ALIGNMENT]: 100 - Actively checked A1/B1/B3/B4/B5/C1 boundaries: the leaf is at the Tier-1 owner, the consumer reads it inline, child realms inherit it, and no parallel fallback, export, threading layer, defensive access, or singleton mutation remains.[CONTENT_COMPLETENESS]: 100 - The leaf and consumer carry durable intent, #14956 now supplies a precise Contract Ledger and ACs, and the PR body supplies the complete source census plus bounded defer/skip rationales.[EXECUTION_QUALITY]: 100 - 29/29 related tests, four fresh-process env/realm probes, exact census reconciliation, clean diff check, and 12/12 hosted checks cleared the relevant failure modes.[PRODUCTIVITY]: 100 - Every #14956 AC is delivered: owning leaf, inherited resolution, use-site consumption, precedence/fallback preservation, isolated tests, lints, and a truthful leaf close-target.[IMPACT]: 70 - The code delta is small, but it removes a shared transport-wide configuration split and leaves an auditable classification for the remaining server-shared env reads.[COMPLEXITY]: 35 - Four existing files, one leaf/consumer path, and two focused spec surfaces; most cognitive load lies in the 12-site semantic census rather than runtime control flow.[EFFORT_PROFILE]: Quick Win - High architectural ROI from a low-complexity owner correction, with unusually strong census and regression evidence.
Approved at exact head 38fcee8d047ba814dd83fbde1f5220bf1269f548. Human merge gate remains authoritative.
Resolves #14956
Related: #12456 (batch 3 — MCP server-shared layer env-read census + the clear config-owned fix).
Context
Batch 3 of the ADR-0019 cleanup sweeps the MCP server-shared layer for raw
process.envreads:BaseServer.mjs,shared/services/TransportService.mjs,shared/services/DestructiveOperationGuard.mjs,gitlab-workflow/Server.mjs,knowledge-base/Server.mjs+toolService.mjs(remaining reads), andneural-link/Bridge.mjs. Every surveyed read is classified in the census table below; the single CLEAR config-owned violation is fixed on the owning config.The fix (A1 — module/inline env re-derivation with a hidden default):
TransportService.setup()derived the advertised SSE/HTTP transport host asprocess.env.HOST || 'localhost'at the use site — a raw env read with a shadow default, sitting right next to config-ownedaiConfig.publicUrl/aiConfig.mcpHttpPortreads. The deployment host is config, so it now lives as amcpHttpHost: leaf('localhost', 'HOST', 'string')on the rootai/config.template.mjstransport cluster (betweenallowedHostsandmcpHttpPort), and TransportService consumesaiConfig.mcpHttpHostat the use site. The existingHOSTenv binding stays bound through the leaf — no operator-visible env-var rename. Placement on the root (not duplicated per-server) is the sanctioned realm-chain shape: per-server configs resolvemcpHttpHostoverride-else-inherit viagetParent().Exact-semantics preservation:
Neo.util.Env.parseStringreturnsundefinedfor absent and empty-string env values (Neo.isEmptyguard), so the leaf reproduces the||fallback exactly, including theHOST=''edge. Env re-resolution is bounded (construct /setEnvOverride/load/refreshEnv) per the documented ConfigProvider contract — for a server that reads the value once at boot, behavior is identical.Spec-shape shift: the three
mcpServerUrlresolution tests inTransportService.spec.mjspreviously mutatedprocess.env.HOST; they now model the resolved config contract (mcpHttpHostin the mockaiConfig), and the env binding itself is covered by a new rootconfig.template.spec.mjstest (setEnvOverride('HOST', ...)resolving through the leaf on a fresh provider instance). Net: zeroprocess.envmutation left in the transport spec.Gitignored overlays (
ai/config.mjs) were re-synced to the updated template in both the worktree and the operator checkout, so live servers pick up the leaf on next restart.Evidence: runtime-verified — realm-chain resolution probed on live configs (root/kb/memory-core all resolve
mcpHttpHost, andHOST=internal-hostresolves through the root leaf from a child config read), plus 402-spec unit run (see Test Evidence).Deltas from ticket
knowledge-base/toolService.mjs:23(NEO_AI_MCP_KB_OPENAPI_PATH) among census targets; it is already converted by open batch-1 PR #14935, so it is census-rowed here but deliberately untouched to avoid a cross-batch conflict.npm_package_versionduplicates ingitlab-workflow/Server.mjs/knowledge-base/Server.mjsare census-deferred rather than fixed: the identical shape exists in three sibling servers outside this batch's file list (github-workflow,file-system,memory-core), and removing only the in-scope two would create sibling drift. See census rows 3–4 for the recommended one-shot fold-out.DestructiveOperationGuard'sUNIT_TEST_MODEread is NOT mapped onto theuseTestDatabaseleaf pattern — deliberate skip with rationale (census row 10): the guard is fail-closed defense-in-depth against config-resolution mistakes; routing its bypass through config would reintroduce the exact trust-the-config dependency it exists to defend against.Census — every surveyed env read in the batch-3 file set
ai/mcp/server/shared/services/TransportService.mjs:160HOST'localhost'defaultmcpHttpHostleaf on rootai/config.template.mjs; consumed at use siteai/mcp/server/BaseServer.mjs:301npm_package_versionmetadata.version || env || '1.0.0')ai/mcp/server/gitlab-workflow/Server.mjs:37npm_package_versiongithub-workflow/Server.mjs:38,file-system/Server.mjs:46,memory-core/Server.mjs:84); partial removal here would create sibling driftai/mcp/server/knowledge-base/Server.mjs:40npm_package_versionai/mcp/server/knowledge-base/Server.mjs:106NEO_AGENT_ID,USERai/mcp/server/knowledge-base/Server.mjs:110NEO_SESSION_IDai/mcp/server/knowledge-base/toolService.mjs:23NEO_AI_MCP_KB_OPENAPI_PATHopenApiPathleaf on the kb server config); untouched here to avoid cross-batch conflictai/mcp/server/knowledge-base/toolService.mjs:94NEO_AGENT_ID,USERai/mcp/server/knowledge-base/toolService.mjs:95NEO_SESSION_IDai/mcp/server/shared/services/DestructiveOperationGuard.mjs:113UNIT_TEST_MODEuseTestDatabasestorage-selection caseai/mcp/server/shared/services/DestructiveOperationGuard.mjs:236env = process.envDI default (gatesNEO_ALLOW_PRODUCTION_DESTRUCTIVE_AI_SUBSTRATEin#isProductionConfirmed)ai/mcp/server/neural-link/Bridge.mjs:141NEO_FLEET_BRIDGE_PUBLIC_KEYTotals: 12 code-level env-read sites surveyed → 1 fixed here · 1 fixed in batch-1 · 5 runtime-identity/credential · 1 runtime process metadata (canonical) · 2 deferred (duplicate fold-out) · 2 deliberate safety-guard contracts.
Test Evidence
npm run test-unit -- test/playwright/unit/ai/mcp/ test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/scripts/lint/lintConfigTemplateSsot.spec.mjs --workers=1→ 400 passed / 402 on the mergedorigin/devbase (4d4b20dca), reproducible across two runs.origin/dev(no batch-3 diff): 399 passed / 401 with the exact same 2 failures — both are pre-existing in this environment, independent of this diff (the +1 test on the branch is the newmcpHttpHostleaf-binding test, green).BootEnvelopeResolver.spec.mjs:51("empty-string env vars are treated as absent (default instance)") — pre-existing, environment-specific (also fails on a clean stashed tree in isolation).memory-core/Server.spec.mjs:383(#14388 separate-graph-process identity) — order-dependent in the full run; passes 17/17 when the spec file runs in isolation; outside this diff's surface.lint-config-template-ssot) green: the new leaf default is a literal (no inline env), and no baseline row referencedTransportService/HOST, so no baseline churn.mcpHttpHost === 'localhost'; kb + memory-core configs resolve'localhost'via the realm chain;HOST=internal-hostresolves'internal-host'from a child-config read.node --checkgreen on all four touched files; whitespace / shorthand / jsdoc-types / ticket-archaeology / block-alignment / aiconfig-test-mutation gates green (lint-staged at commit).Post-Merge Validation
NEO_TRANSPORT=sse) and confirm the advertisedmcpServerUrlis unchanged for the deployment (HOSTenv still wins over thelocalhostdefault;publicUrlstill wins over both).ai/config.mjsoverlay: re-sync the overlay from the template (or add themcpHttpHostleaf) — the shipped worktree/checkout overlays were already re-synced; a missing leaf on a stale overlay resolves toundefinedand fails loud at SSE boot, per the no-defensive-fallback contract.Commits
refactor(mcp): ADR-0019 batch-3 — server-shared env reads onto owning config leaves + census (#12456)Authored by Mnemosyne (Claude Fable 5, Claude Code). Session b956ba53-01ed-4ea6-a1e5-62969f887bc3.