Context
PR #10877 (Resolves #10862) institutionalized the canonical NEO_ prefix on 13 substrate env vars including TRANSPORT → NEO_TRANSPORT, AUTO_SYNC → NEO_AUTO_SYNC, AUTO_SUMMARIZE → NEO_AUTO_SUMMARIZE. The PR's AC4 grep verified zero residual bare-name occurrences in ai/, src/, buildScripts/ — but did NOT scope the audit to test/. As a result, one Playwright spec that spawns the MCP server with the now-renamed env vars was missed and ships its fixtures with the old (no-longer-read) names.
Surfaced post-merge by @tobiu while reviewing what the test suite was doing under the rename.
The Problem
test/playwright/unit/ai/mcp/Authorization.spec.mjs spawns node ai/mcp/server/.../mcp-server.mjs with an env block that uses pre-#10877 names:
mcpServerProcess = spawn('node', [SERVER_PATH, '--debug'], {
env: {
...process.env,
TRANSPORT : 'sse',
SSE_PORT : MCP_SSE_PORT.toString(),
AUTO_SYNC : 'false',
AUTO_SUMMARIZE : 'false',
NEO_AUTH_HOST : 'localhost',
NEO_AUTH_PORT : MOCK_OIDC_PORT.toString(),
NEO_AUTH_REALM : 'test',
NEO_OAUTH_CLIENT_ID: TEST_CLIENT_ID,
HOST : 'localhost'
}
});
Effect: the server boots with default transport: 'stdio' instead of fixture-injected 'sse', and default autoSync: false / autoSummarize: false (which happens to match what the fixture also passes — so the AUTO_* mismatches don't cause behavioral divergence here, BUT the TRANSPORT mismatch DOES — server doesn't open SSE, test setup fails or stalls).
The fixture has 6 affected occurrences across 2 spawn blocks (lines 76-79 and 188-191).
The Architectural Reality
test/playwright/unit/ai/mcp/Authorization.spec.mjs:73-86 — first spawn block (test.beforeAll), uses fixture env injection.
test/playwright/unit/ai/mcp/Authorization.spec.mjs:185-198 — second spawn block (similar shape).
- Server reads from
process.env.NEO_TRANSPORT / process.env.NEO_AUTO_SYNC / process.env.NEO_AUTO_SUMMARIZE post-#10877 — the env-binding tables in ai/mcp/server/{memory-core,knowledge-base}/config.template.mjs were updated, but no test file was touched in #10877's diff.
- Empirical:
grep -rln "TRANSPORT\|AUTO_SUMMARIZE\|AUTO_SYNC" test/ returns ONLY this single file (other test files don't spawn server processes with env injection).
SSE_PORT (line 77) is the legacy alias retained during the #10808 deprecation window — still resolves correctly to MCP_HTTP_PORT for now. Modernizing to MCP_HTTP_PORT is a separate concern under #10808's deprecation cleanup.
The Fix
Single-file rename in test/playwright/unit/ai/mcp/Authorization.spec.mjs:
| Line(s) |
Before |
After |
| 76, 188 |
TRANSPORT : 'sse', |
NEO_TRANSPORT : 'sse', |
| 78, 190 |
AUTO_SYNC : 'false', |
NEO_AUTO_SYNC : 'false', |
| 79, 191 |
AUTO_SUMMARIZE : 'false', |
NEO_AUTO_SUMMARIZE: 'false', |
Block-formatting alignment may need adjustment after the rename (longer property names shift the colon column). Apply repo coding-guideline §8.32 column-alignment after the substitution.
SSE_PORT left as-is (legacy alias under #10808 deprecation window, separate concern). Other env vars in the block (NEO_AUTH_*, ONEO_AUTH_*, HOST) are third-party canonical or host-config and are correctly NOT prefixed per #10862's out-of-scope policy.
Acceptance Criteria
Out of Scope
SSE_PORT modernization to MCP_HTTP_PORT — that's #10808's deprecation-cleanup lane.
TRANSPORT value semantics — fixture passes 'sse'; server post-#10877 reads NEO_TRANSPORT='sse'. No behavior change beyond the env-var-name surface.
- Adding test-execution to
npm run test-unit -- test/playwright/unit/ai/mcp/ discovery patterns — already covered.
- Repo-wide test audit for OTHER env-var-using fixtures — confirmed empirically only this one file affected via
grep -rln.
Avoided Traps
- Bundling SSE_PORT modernization — that's a separate substrate concern under #10808's deprecation chain. Keep this fix scoped tight.
- Re-deriving the env-binding table at test-fixture level — the test should pass env vars by their canonical names, not re-implement the binding logic.
- Implicit-pass via default-value coincidence — even though
AUTO_SUMMARIZE='false' and the default is also false, the rename should still happen so a future test changing the value to 'true' actually takes effect.
Related
- #10862 — parent NEO_ prefix institutionalization
- PR #10877 — the merged rename PR (this is its follow-up)
- #10808 —
MCP_HTTP_PORT / SSE_PORT deprecation window (separate concern)
- #10822 — config substrate cleanup epic (parent of #10862)
Lessons Learned (Reviewer-Side)
This issue surfaced because my Cycle 1 / Cycle 2 reviews of #10877 cited "substrate-contention concern in this active worktree" as rationale for skipping the test-execution step. Per pr-review-guide.md §2.2 Empirical Checkout Mandate: "A static diff read is insufficient to score [EXECUTION_QUALITY] for code changes. You MUST use the active harness tool (e.g., checkout_pull_request) to load the branch locally and execute RELATED tests." Renaming env-var consumers in production code IS structural code change; tests that spawn server processes ARE related tests. Skipping was wrong-shape regardless of worktree contention. Memory anchor feedback_run_related_tests_in_pr_review.md filed alongside this ticket.
Origin Session ID: 78a3272e-847b-4799-ad6c-ce334464844c
Retrieval Hint: query_raw_memories(query="Authorization.spec.mjs NEO_ env-var fixture rename followup")
Context
PR #10877 (Resolves #10862) institutionalized the canonical
NEO_prefix on 13 substrate env vars includingTRANSPORT → NEO_TRANSPORT,AUTO_SYNC → NEO_AUTO_SYNC,AUTO_SUMMARIZE → NEO_AUTO_SUMMARIZE. The PR's AC4 grep verified zero residual bare-name occurrences inai/,src/,buildScripts/— but did NOT scope the audit totest/. As a result, one Playwright spec that spawns the MCP server with the now-renamed env vars was missed and ships its fixtures with the old (no-longer-read) names.Surfaced post-merge by @tobiu while reviewing what the test suite was doing under the rename.
The Problem
test/playwright/unit/ai/mcp/Authorization.spec.mjsspawnsnode ai/mcp/server/.../mcp-server.mjswith an env block that uses pre-#10877 names:mcpServerProcess = spawn('node', [SERVER_PATH, '--debug'], { env: { ...process.env, TRANSPORT : 'sse', // server reads NEO_TRANSPORT, falls back to default 'stdio' SSE_PORT : MCP_SSE_PORT.toString(), // legacy alias still works per #10808 deprecation window AUTO_SYNC : 'false', // server reads NEO_AUTO_SYNC, fixture value ignored AUTO_SUMMARIZE : 'false', // server reads NEO_AUTO_SUMMARIZE, fixture value ignored NEO_AUTH_HOST : 'localhost', // third-party canonical (OIDC), no rename — OK NEO_AUTH_PORT : MOCK_OIDC_PORT.toString(), // OK NEO_AUTH_REALM : 'test', // OK NEO_OAUTH_CLIENT_ID: TEST_CLIENT_ID, // OK HOST : 'localhost' // OK (host config, not renamed) } });Effect: the server boots with default
transport: 'stdio'instead of fixture-injected'sse', and defaultautoSync: false/autoSummarize: false(which happens to match what the fixture also passes — so the AUTO_* mismatches don't cause behavioral divergence here, BUT the TRANSPORT mismatch DOES — server doesn't open SSE, test setup fails or stalls).The fixture has 6 affected occurrences across 2 spawn blocks (lines 76-79 and 188-191).
The Architectural Reality
test/playwright/unit/ai/mcp/Authorization.spec.mjs:73-86— first spawn block (test.beforeAll), uses fixture env injection.test/playwright/unit/ai/mcp/Authorization.spec.mjs:185-198— second spawn block (similar shape).process.env.NEO_TRANSPORT/process.env.NEO_AUTO_SYNC/process.env.NEO_AUTO_SUMMARIZEpost-#10877 — the env-binding tables inai/mcp/server/{memory-core,knowledge-base}/config.template.mjswere updated, but no test file was touched in #10877's diff.grep -rln "TRANSPORT\|AUTO_SUMMARIZE\|AUTO_SYNC" test/returns ONLY this single file (other test files don't spawn server processes with env injection).SSE_PORT(line 77) is the legacy alias retained during the #10808 deprecation window — still resolves correctly toMCP_HTTP_PORTfor now. Modernizing toMCP_HTTP_PORTis a separate concern under #10808's deprecation cleanup.The Fix
Single-file rename in
test/playwright/unit/ai/mcp/Authorization.spec.mjs:TRANSPORT : 'sse',NEO_TRANSPORT : 'sse',AUTO_SYNC : 'false',NEO_AUTO_SYNC : 'false',AUTO_SUMMARIZE : 'false',NEO_AUTO_SUMMARIZE: 'false',Block-formatting alignment may need adjustment after the rename (longer property names shift the colon column). Apply repo coding-guideline §8.32 column-alignment after the substitution.
SSE_PORTleft as-is (legacy alias under #10808 deprecation window, separate concern). Other env vars in the block (NEO_AUTH_*, ONEO_AUTH_*, HOST) are third-party canonical or host-config and are correctly NOT prefixed per #10862's out-of-scope policy.Acceptance Criteria
test/playwright/unit/ai/mcp/Authorization.spec.mjsuseNEO_prefix where applicable (TRANSPORT, AUTO_SYNC, AUTO_SUMMARIZE).npm run test-unit -- test/playwright/unit/ai/mcp/Authorization.spec.mjspasses (or its pre-existing pass/fail state is preserved post-rename — if the spec was already broken for unrelated reasons, this PR doesn't fix it).grep -rE "(?<![A-Z_])(AUTO_SUMMARIZE|AUTO_DREAM|AUTO_GOLDEN_PATH|REAL_TIME_MEMORY_PARSING|AUTO_INGEST_FS|AUTO_SYNC|MEMORY_COLLECTION_NAME|SESSION_COLLECTION_NAME|GRAPH_COLLECTION_NAME|GRAPH_DECAY_FACTOR)\\b" .returns zero results post-PR (full repo audit, not justai/src/buildScriptsfrom #10862's AC4).Out of Scope
SSE_PORTmodernization toMCP_HTTP_PORT— that's #10808's deprecation-cleanup lane.TRANSPORTvalue semantics — fixture passes'sse'; server post-#10877 readsNEO_TRANSPORT='sse'. No behavior change beyond the env-var-name surface.npm run test-unit -- test/playwright/unit/ai/mcp/discovery patterns — already covered.grep -rln.Avoided Traps
AUTO_SUMMARIZE='false'and the default is alsofalse, the rename should still happen so a future test changing the value to'true'actually takes effect.Related
MCP_HTTP_PORT/SSE_PORTdeprecation window (separate concern)Lessons Learned (Reviewer-Side)
This issue surfaced because my Cycle 1 / Cycle 2 reviews of #10877 cited "substrate-contention concern in this active worktree" as rationale for skipping the test-execution step. Per
pr-review-guide.md §2.2Empirical Checkout Mandate: "A static diff read is insufficient to score[EXECUTION_QUALITY]for code changes. You MUST use the active harness tool (e.g.,checkout_pull_request) to load the branch locally and execute RELATED tests." Renaming env-var consumers in production code IS structural code change; tests that spawn server processes ARE related tests. Skipping was wrong-shape regardless of worktree contention. Memory anchorfeedback_run_related_tests_in_pr_review.mdfiled alongside this ticket.Origin Session ID: 78a3272e-847b-4799-ad6c-ce334464844c
Retrieval Hint:
query_raw_memories(query="Authorization.spec.mjs NEO_ env-var fixture rename followup")