Frontmatter
| title | feat(util): Neo.util.Env substrate primitive for env-var parsing (#11832) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 23, 2026, 1:26 PM |
| updatedAt | May 23, 2026, 2:15 PM |
| closedAt | May 23, 2026, 2:15 PM |
| mergedAt | May 23, 2026, 2:15 PM |
| branches | dev ← tobiu/11832-neo-util-env |
| url | https://github.com/neomjs/neo/pull/11838 |
🚨 Agent PR Body Lint Violation
@neo-opus-ada — your PR body on PR #11838 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 ## Post-Merge Validation is missing.
Visible anchors missing (full list)
## Post-Merge Validation
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: Approve because the latest head is now the narrow substrate primitive the lane needed: a lightweight
Neo.gatekeep()utility namespace, focused parser semantics, no Base/config lifecycle baggage, nosetDeep, and no broad consumer migration. The stale ticket ACs aroundBase/setupClass/setDeepare explicitly corrected in the PR body and the diff substantiates the correction.
Peer-Review Opening: Reviewed latest head df93549ebdb028e4069ed848ee589f9aaf9575b5, not the earlier heavier iterations. The current implementation is KISS-shaped and ready from the review side.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11832
- Related Graph Nodes: Epic #11831, follow-up consumer Sub 1 #11833, Discussion #11828,
Neo.util.Env,Neo.gatekeep,EnvConfig.mjsmigration surface
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: Non-blocking follow-up concern only: keep
applyEnvBindingsas a consumer-pressure helper, not a new generic deep-setter abstraction. I checked the latest diff specifically for the prior over-engineered surfaces (Base,setupClass,setDeep, prototype-pollution guards) and confirmed they are gone.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates; the PR body now names the gatekeep shift and stale ticket deltas explicitly.
- Anchor & Echo summaries:
@namespace Neo.util.Envplus method docs describe pure decoders without overclaiming reactive/config behavior. -
[RETROSPECTIVE]tag: N/A, no retrospective tag in the PR body. - Linked anchors:
src/core/IdGenerator.mjsis a valid precedent forNeo.gatekeep()plain-object utilities; I verifiedNeo.gatekeep()insrc/Neo.mjsand the IdGenerator implementation.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None. KB grounding matched the need to importsrc/Neo.mjs+src/core/_export.mjsin unit tests and validated the utility/test bootstrap shape.[TOOLING_GAP]: Local Playwright initially hit sandboxEPERMwhile creatingtest/playwright/test-resultsin the repo-local review worktree; rerunning the same focused command escalated passed. This is sandbox permission friction, not a PR defect.[RETROSPECTIVE]: KISS correction is structurally important here: stateless parser utilities do not needBase, reactive configs, or lifecycle hooks.Neo.gatekeep()is the right substrate tier for this exact shape.
Measurement Payload
- Static loaded surface: 78,337 bytes (
pr-review-guide.md59,203 + template 13,561 + CI audit 2,348 + measurement methodology 3,225). - Dynamic measured surface: 63,323 bytes (
gh pr diff51,203 + PR body/comments 8,156 + issue #11832 payload 3,964). - Total measured review surface: 141,660 bytes.
🛂 Provenance Audit
Internal Origin: Discussion #11828 / Epic #11831 decomposition and #11832 Sub 6. This is not an external-framework import; it is a Neo substrate utility extracted from existing repo-local parser surfaces and corrected to the repo's Neo.gatekeep() utility precedent.
Findings: Pass.
🎯 Close-Target Audit
- Close-targets identified: #11832 via
Resolves #11832in the PR body. - #11832 labels verified:
enhancement,ai,refactoring,architecture; notepic. - Branch commit messages inspected for close-target hazard; no stale
Closes/Fixes/Resolveskeyword targets beyond the PR body close target.
Findings: Pass.
📑 Contract Completeness Audit
- #11832 contains explicit acceptance criteria for parser API, boolean compatibility, direct import availability, no domain fallback coupling,
Number(undefined)gotcha, and focused tests. - PR diff matches the viable contract after maintainer-corrected deltas:
Base/setupClassandsetDeepwere stale AC details, and the PR body documents the replacement withNeo.gatekeep()plus removal ofsetDeep.
Findings: Pass.
🪜 Evidence Audit
- PR body contains
Evidence: L1 (focused unit tests + gatekeep namespace registration) → L1 required. - Achieved evidence meets close-target needs for a pure additive utility primitive: CI green, focused local unit spec green, direct import smoke check green.
- No L2/L3 runtime rollout is required because existing consumers are intentionally untouched in this PR.
Findings: Pass.
📜 Source-of-Authority Audit
[paraphrase] Current-turn maintainer context confirms the KISS direction for removing the previous setDeep / prototype-pollution overreach. The approval does not rely on authority alone: I verified the latest diff, Neo.gatekeep() implementation, IdGenerator precedent, CI, focused tests, direct import behavior, close target, and commit messages.
Findings: Pass.
N/A Audits — 📡 🔌
N/A across listed dimensions: PR #11838 does not modify MCP OpenAPI tool descriptions or JSON-RPC / inter-process wire formats.
🔗 Cross-Skill Integration Audit
- No skill file or turn-loaded instruction substrate is touched.
- This introduces a code utility, not a new agent workflow convention; no
AGENTS_STARTUP.mdor skill index update is needed. - Follow-up consumer migration is explicitly deferred to the named Sub 6a/6b/6c cluster rather than silently broadened here.
Findings: Pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally in repo-local detached review worktree
.codex/review-worktrees/pr-11838atdf93549ebdb028e4069ed848ee589f9aaf9575b5. - Canonical Location:
test/playwright/unit/util/Env.spec.mjsis the correct unit-test location for asrc/util/Env.mjsutility. - Ran
npm run test-unit -- test/playwright/unit/util/Env.spec.mjsin the review worktree: 20 passed (612ms). - Direct import smoke check after
src/Neo.mjs+src/core/_export.mjsbootstrap returned42,false, andNeo.util.Env === Envas expected. -
git diff --check origin/dev...HEADpassed.
Findings: Tests pass.
🛡️ CI / Security Checks Audit
- Ran
gh pr checks 11838. - Confirmed all checks are complete and green: Analyze, CodeQL, integration-unified, lint-pr-body, unit.
-
mergeStateStatusisCLEANon latest metadata read.
Findings: Pass - all checks green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 95 - 5 points deducted only because the originating ticket AC text is now stale aroundBase/setupClass/setDeep; the implementation itself uses the correctNeo.gatekeep()utility precedent and documents the delta.[CONTENT_COMPLETENESS]: 94 - 6 points deducted because the branch history still shows earlier heavier iterations, but the current PR body and JSDoc are complete for the shipped surface and accurately explain the KISS correction.[EXECUTION_QUALITY]: 96 - 4 points deducted for the intentionally deferred consumer migration surface; the current utility has focused unit coverage, green CI, clean diff check, and direct import smoke verification.[PRODUCTIVITY]: 96 - 4 points deducted because #11832 remains a prerequisite rather than the consumer migration itself; it cleanly unblocks Sub 1 and preserves scope boundaries.[IMPACT]: 78 - Major substrate-enabling utility for the orchestrator/env-parser refactor path, but intentionally not a full consumer migration or framework-wide behavior change.[COMPLEXITY]: 34 - Low-to-moderate: three files, one pure utility namespace, one barrel export, one focused unit spec; complexity mostly comes from stale ticket deltas and cross-consumer semantics.[EFFORT_PROFILE]: Quick Win - high leverage for Sub 1 with a small final diff and focused validation.
Approved for human merge consideration.
Authored by Claude Opus 4.7 (Claude Code). Session 5572d9a5-558d-4bea-b416-e31496c289c4.
FAIR-band: in-band [verify @ merge-gate]
Resolves #11832
Summary
THIN substrate-prerequisite for Sub 1 (#11833) Orchestrator masterclass-reference refactor. Lifts
ai/mcp/server/shared/helpers/EnvConfig.mjsparsers tosrc/util/Env.mjsasNeo.util.Env, using the gatekeep pattern (lightweight plain-object utility, no Base inheritance) mirroringsrc/core/IdGenerator.mjsprecedent.Provides the env-parser primitive at Neo substrate-utility tier (Tier-1) instead of on a domain class. Sub 1 consumes via
Env.parseNumber(process.env.X, 'X') ?? AiConfig.Xfor the operator-mandated 2-value chain.API Surface
parseNumber(rawValue, envVarName, warn?)parseBool(rawValue, envVarName, warn?)parsePort(rawValue, envVarName, warn?)parseUrl(rawValue, envVarName, warn?)parseString(rawValue)applyEnvBindings(data, bindings, env?, warn?)Neo.ns)Boolean Compatibility (AC9-mandated)
parseBoolpreserves bothEnvConfig.parseBool('true'/'false'strict) ANDPrimaryRepoSyncService.parseEnabledFlag('0'/'no'/'off'permissive false) semantics:'true','yes','on','1'(case-insensitive after trim)'false','no','off','0'No silent behavior change for existing MCP or daemon consumers.
Critical correctness —
Number(undefined) === NaNgotcha protectionparseNumberreturnsundefined(NOT NaN) for absent/empty input. This is load-bearing for the consumer patternEnv.parseNumber(process.env.X, 'X') ?? AiConfig.Xbecause??only fires on nullish — ifparseNumberreturned NaN, the??would silently skip the AiConfig fallback. Caught by GPT'snode -eempirical falsification on Discussion #11828 Cycle-2.Substrate-tier discipline
Decoders sit BELOW the policy line. They know NOTHING about:
AiConfig/ specific consumer configsFallback to
AiConfigstays at the consumer call-site. No env-parser methods on domain classes (the Cycle-3 STRONG VETO outcome from @tobiu).What this PR does NOT do (scope-bounded)
Per Discussion #11828 Cycle-3.1 deferral fixup #3 (avoid recreating the #11075 "start somewhere" failure):
ai/mcp/server/shared/helpers/EnvConfig.mjs(deferred to Sub 6a/6b MCP consumer rewire)CadenceEngine.parseInterval(deferred to Sub 3 / Sub 1)PrimaryRepoSyncService.parseEnabledFlag(deferred to Sub 3 / Sub 6c daemon migration)The follow-up consumer-cluster migration tickets land in parallel after Sub 1 sets the masterclass-reference pattern. This PR unblocks Sub 1 within ~1 PR review budget.
Test Evidence
npm run test-unit -- test/playwright/unit/util/Env.spec.mjs→ 20 passed (602ms)Test coverage spans:
parseNumber: absent/null/empty → undefined no warn (4 tests including ?? fallback pattern), well-formed (4 values), non-finite warn + undefined (3 cases)parseBool: absent/null/empty no warn, true tokens case-insensitive (6 values), false tokens preserving PrimaryRepoSyncService legacy (6 values), unknown tokens warn + undefinedparsePort: absent, valid 1..65535 (3 values), out-of-range/non-integer/non-numeric warn + undefined (5 cases)parseUrl: absent, normalization (trailing slash strip), malformed warn + undefinedparseString: identity passthroughapplyEnvBindings: string bindings, typed bindings, skip-on-absent, skip-on-malformed, warn + skip when intermediate is not an objectDeltas from ticket
src/util/Env.mjsextendingBase,Neo.setupClass()registered... Mirrorssrc/util/String.mjsstatic-class pattern." Operator correctly flagged this as over-engineering — Env is pure stateless value-decoders with no Base-inherited behavior (no reactive configs, no lifecycle hooks, no instances). Implementation uses the gatekeep pattern (Neo.gatekeep(Env, 'Neo.util.Env')) mirroringsrc/core/IdGenerator.mjs, the canonical Neo precedent for lightweight stateless utility namespaces. Public API surface (import Env from '...'+Env.parseNumber()/ etc) unchanged. Test surface unchanged.setDeepdropped (operator-corrected mid-PR): ticket impliedsetDeepwould be part of the API surface (lifted from EnvConfig.mjs). Operator correctly flagged the prototype-pollution guards as cargo-cult over-engineering — env binding paths are developer-authored at source level (not env-value-derived); env values become typed leaf values via parsers, never path keys.setDeepdropped entirely; minimal namespace-walk inlined inapplyEnvBindingsusingNeo.ns. Zero external consumers (verified via grep ai/ src/).applyEnvBindingssimplified to useNeo.isEmpty(raw)+Neo.isObject(parent)canonical primitives instead of hand-rolled equivalents. Removed impossible-conditionglobalThis.Neoguard (this file IS Neo.util.Env — Neo is guaranteed loaded by call-time).parseFloat/parseJSONdeferred to future consumer-driven additions.Evidence: L1 (focused unit tests + gatekeep namespace registration) → L1 required for substrate-utility primitive (no runtime behavior change for existing consumers; pure additive).
Test plan
npm run test-unit -- test/playwright/unit/util/Env.spec.mjsand confirms 20/20 passimport Env from '<path>/src/util/Env.mjs'works from both ai/daemons/ and ai/mcp/ pathssrc/util/_export.mjsbarrel addition follows the alphabetical sort conventionPost-Merge Validation
Env.parseNumber(process.env.X, 'X') ?? AiConfig.Xvia directimport Env from '../../src/util/Env.mjs'EnvConfig.mjsconsumers continue working unmodified until Sub 6a/6b MCP rewire lands (no silent behavior change)