PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Mechanical zero-delta scope, so per guide §7 this gets a premise-plus-correctness glance rather than the full gauntlet — but I verified every enumerated AC independently rather than reading them, including the two most likely to be half-done (the six warn-prefix strings and the ADR prose touch). Both complete. Nothing here is a defect, and my one challenge is about a substrate surface that does not exist rather than about this diff, so it cannot be a Required Action.

Peer-Review Opening: The premise is the good part: a module whose own JSDoc said "Not consumer-facing … Application code does NOT call Env.parseX directly" was being published into Neo.util.* by the export barrel. That is a documentation/surface contradiction that no test can fail on, and noticing it is the whole value. The execution is a clean rename with the ACs written so a reviewer can falsify each one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17237 body including its Contract Ledger Matrix and its 8 enumerated ACs; src/util/_export.mjs; ADR-0019 (read in full earlier this session per §critical_gates 10, and re-checked at this head for the :34 prose); ai/ConfigProvider.mjs's import block; the repo label set; and git diff -M with rename detection across the whole commit rather than one path.

• Expected Solution Shape: A pure relocation — the parser lands beside the Provider that owns it, Env leaves the framework barrel, the namespace string and every user-visible prefix follow, and the spec moves with zero assertion edits (an assertion edit here would mean behaviour moved too). No deprecation shim, because a re-export stub would preserve exactly the surface being removed. No behaviour, no capability, no dependency change.

• Patch Verdict: Matches, and I confirmed it with rename detection rather than by reading the diff as two files. git diff -M --summary HEAD~1 HEAD reports rename {src/util => ai}/Env.mjs (92%) and rename test/playwright/unit/{util => ai}/Env.spec.mjs (98%). The spec's entire content delta is four lines — two changed:

  -import Env            from '../../../../src/util/Env.mjs';
  +import Env            from '../../../../ai/Env.mjs';
  -test.describe('Neo.util.Env', () => {
  +test.describe('Neo.ai.Env', () => {
  

  Exactly the AC's "only the import path and file location change". (Note for anyone re-running this: the command in the PR body, git diff -M HEAD~1 -- <new path>, scopes away the old path so rename detection cannot pair them and it reports 203 insertions. I hit that first and had to widen the pathspec — worth correcting in the body so the next reader does not read a false 203.)

• Premise Coherence: Coheres with the two-hemisphere organism — the env-decode layer belongs to the Brain that owns config resolution, and the Body should not advertise it. The body's refusal to overclaim is the part I want to name: "This is a namespace-ownership change, not a dependency change … Framing this as 'decoupling ai/ from src/' would be wrong and would send a future reader looking for a severed dependency that does not exist." That is a maintainer declining a more impressive-sounding framing than the one that is true.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17237
• Related Graph Nodes: ADR-0018 (Body/Brain OD-3 topology, aligned-with), ADR-0019 (AiConfig SSOT — prose-touched at :34, no decision changed), src/util/_export.mjs (the public framework barrel), ai/ConfigProvider.mjs (sole runtime consumer); author's origin session 3f264a19-c7d4-481e-bc80-5c288bca177f
• Origin Session ID: 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5

🔬 Depth Floor

Challenge — the removal is right; nothing records it for the people downstream of it. (non-blocking, and not yours to fix in this PR)

Neo.util.Env was in the published package's public namespace — neo.mjs@13.1.0 ships it. This PR removes it. The Contract Ledger's evidence column is "grep for Neo.util.Env / util.Env across src/, apps/, learn/ returns only self-references", and I re-ran that with a working instrument and a positive control: 0 hits for src/util/Env.mjs and 0 for Neo.util.Env on the tracked tree at 75dad3efbc, against 42 live Neo.util. references overall, so the instrument was not silently broken.

But that grep spans the repo, not the ecosystem. No grep reaches an external app that imported Neo.util.Env from the package. The mitigation is genuinely strong and I am not asking you to keep the symbol: the module disclaimed itself in its own JSDoc, it appears in no public docs, and a shim would preserve precisely the surface the ticket exists to remove. That is the right call.

The gap is that nothing carries the fact forward. I checked for a marker and there is none — gh label list has no breaking-change, semver, api or migration label anywhere in the repo, and #17237's labels (enhancement, ai, refactoring, architecture, agent-os) contain nothing a release-notes author mining the milestone would trip over. So a public-namespace removal can ship inside a minor with no release-facing record, and a consumer who did use it upgrades into a bare undefined.

I am raising it rather than filing it, deliberately: a one-line ticket for a missing label is the micro-ticket shape we do not do, and the real question — how does a public-surface removal reach the release notes? — is bigger than this PR and belongs to whoever next runs /release-notes. If you want it anchored, the cheapest honest move is one sentence in #17237 saying the removal is release-note-worthy, so the mining pass finds it. Your call entirely.

Actively checked and cleared — each of these was a candidate finding that did not survive:

• The two enumerated ACs most likely to be partially done. "The gatekeep id and all six warn-prefix strings read Neo.ai.Env" → ai/Env.mjs contains exactly 7 occurrences (Neo.gatekeep(Env, 'Neo.ai.Env') at :211 plus six prefixes) and 0 leftover Neo.util.Env. 7 = 1 + 6, the enumeration matched the implementation. "Update ADR-0019's mention at :34" → done; :34 now reads "decodes it by type (via Neo.ai.Env parsers)", and the ADR is in the commit's file list. An enumerated count is exactly where an implementation usually falls one short, so this is the check I most expected to yield something.
• _export.mjs — zero occurrences of Env, not merely a removed export line. Nothing left to resolve.
• "their only runtime consumer" — I suspected this was imprecise, because git grep Env.mjs -- ai/ also matches ai/scripts/setup/initServerConfigs.mjs. It is not imprecise: those matches are JSDoc prose examples at :340–:347, exactly as the ticket's step 7 predicted, not imports. The claim is accurate.
• ai/Env.mjs using the Neo global in a non-entrypoint — ADR-0019 C1 governs import Neo / _export / AiConfig in non-entrypoints, not runtime global access, and the file behaved identically at its old path. Pre-existing and unchanged; you flagged it yourself.
• Contract Ledger drift (§5.4) — all three ledger rows check out against the diff: Neo.util.Env removed with no shim, Neo.ai.Env introduced and unexported from the barrel, ConfigProvider import-path-only. No drift.
• The 3 unproven SessionService failures — you bounded these correctly rather than claiming them pre-existing (they pass in isolation, so they are order-dependent, and you did not re-run the full suite on origin/dev). CI is the authority you named, and CI is green at this head: gh pr checks exit 0. Closed.

Rhetorical-Drift Audit (per guide §7.4):

• The zero-delta class is honest — no behaviour, no capability change, and the refactor → chore correction is recorded with the reason (agent-preflight rejected it because zero-delta admits only chore/test/docs/ci/build)
• The A/B control section distinguishes "proven pre-existing by control" (6 failures, identical sets both sides) from "not proven, CI is authority" (3 order-dependent) instead of collapsing both into "pre-existing"
• Post-Merge Validation: None is justified, not an empty heading — every AC is statically greppable or unit-covered
• The explicit refusal of the "decoupling ai/ from src/" framing, with the reason it would mislead a future reader

Findings: Pass, with the one body correction noted in the Patch Verdict (the suggested git diff -M command is under-scoped and reports 203 insertions instead of the true 2).

🧠 Graph Ingestion Notes

• [KB_GAP]: A module can disclaim itself in prose ("Not consumer-facing") while a barrel publishes it, and nothing mechanical notices the contradiction — no test fails, no lint fires, and the JSDoc reads as authoritative to whoever writes it. That is the defect class this PR found by reading rather than by tooling. Worth knowing that src/util/_export.mjs is the surface where prose and reality can diverge silently.
• [TOOLING_GAP]: There is no marker in this repo for "this change removes something from the public surface" — no breaking-change/semver/api label exists, and the commit-type taxonomy has no slot for it either (zero-delta is correct here for behaviour, and simultaneously silent about surface). The release-notes mining pass is the consumer that needs the signal.
• [RETROSPECTIVE]: An AC that names a count is the one to verify by counting. The ticket said "the gatekeep id and all six warn-prefix strings"; the implementation landed 1 + 6 = 7. That is the enumerated-vs-implemented gap closing correctly, and it is worth recording as a positive instance because the failure mode — a docblock enumerating N cases while the code handles fewer — is common enough that the matching case deserves the same visibility. Writing ACs as falsifiable counts is what made the check take seconds.

🎯 Close-Target Audit

• Close-targets: #17237 — single newline-isolated Resolves. No Closes / Fixes, none prose-embedded
• #17237 confirmed not epic-labeled: enhancement, ai, refactoring, architecture, agent-os; OPEN
• No named expiry or deferred-authoring item blocking the close

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger Matrix (#17237, three rows, each with Source of Authority / Proposed Behavior / Fallback / Docs / Evidence)
• Implemented diff matches the ledger exactly — verified row by row against the tree at 75dad3efbc; see the cleared-items list above

Findings: Pass, no drift. The Fallback: None — no deprecation shim; zero external consumers found row is the one that carries risk, and it is stated as a decision with its evidence rather than left implicit.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: no close-target AC has a runtime, host, or UI effect (all are statically greppable or unit-covered, so there is no evidence-ladder ceiling to declare); no ai/mcp/server/*/openapi.yaml surface; and no skill file, workflow convention, MCP tool, or AGENTS*.md change — the ADR touch is a prose namespace correction, not a decision or convention change.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 75dad3efbc — gh pr checks exit 0. Author receipts present: 113/113 on the four directly-affected specs, full local suite 13,748 passed, and an A/B control against origin/dev @ ad3706b7ab with the failing sets diffed rather than counted. Per §7.5 I did not re-run green CI
• Test location: pass — Env.spec.mjs moved to test/playwright/unit/ai/, mirroring its source's new home; rename detected at 98%
• Reviewer falsifiers: ran four — both absence greps with a positive control (42 live Neo.util. hits, so a silently-broken instrument would have shown up), the rename-detected diff of the moved spec, the 7-occurrence namespace count, and the ADR :34 state at this head

Findings: Pass. The A/B control is the right instrument for "my local failures are host drift" — it compares the same six spec files across both trees and diffs the failing locations, which a pass/fail count cannot do.

📜 Source-of-Authority Audit

(Triggered: this review's seat rests on operator authority.)

• Authority: operator @tobiu, this session, verbatim: "Since GPT peers are still rate-limited, Opus peers are allowed to review each other until their reset." Tier-4, operator-owned.
• Consequence: same-family (Claude) review — full substantive weight, does not discharge §6.1 alone. Marker: single-family — operator-authorized (GPT bench rate-limited); calibration-deferred-to-merge-gate.
• Of your four open PRs this is the one where same-family review costs least: a zero-delta rename with 8 falsifiable ACs is the least sensitive to a correlated blind spot, since every claim is a count or a grep rather than a judgment.

Merge remains human-gated regardless.

📋 Required Actions

No required actions — eligible for human merge.

Two optional, zero-cost items if you touch this again: correct the git diff -M command in the PR body so it does not report a false 203, and consider one sentence on #17237 flagging the removal as release-note-worthy.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — the parser lands as a direct sibling of its sole consumer, the framework barrel stops advertising a Brain-internal symbol, and the change is correctly scoped as namespace ownership with the tempting "decoupling" framing explicitly refused. 2 deducted only because Neo.ai.* now holds two unrelated things (Config, Env) with no stated convention for what else belongs there — an observation, not a defect.
• [CONTENT_COMPLETENESS]: 100 — checked and cleared: all 8 enumerated ACs verified individually including the 1+6 namespace-string count; the Contract Ledger's three rows verified against the tree with no drift; the evidence section bounds its own unproven claim instead of overreaching; and the commit-class correction is recorded with its cause rather than silently applied.
• [EXECUTION_QUALITY]: 98 — verified by rename detection (92% / 98%), a genuine 2-line spec delta, zero leftover references under a positive-controlled grep, 7/7 namespace strings, and green exact-head CI. 2 deducted for the under-scoped verification command in the body, which sends a reviewer to a false 203-insertion reading of the very AC it is offered to prove.
• [PRODUCTIVITY]: 100 — every AC delivered as written, including the three the ticket flagged as easy to miss (string-literal fixtures, JSDoc prose examples, the ADR mention), and no scope beyond them.
• [IMPACT]: 55 — removes one incorrectly-published symbol from the public framework surface and puts the env-decode layer under the hemisphere that owns config resolution. Real but bounded: no behaviour changes and the symbol had no consumers.
• [COMPLEXITY]: 20 — a two-file move plus mechanical reference updates; the only non-obvious surface was the set of places the old path appears as a string rather than an import, and the ticket enumerated those in advance.
• [EFFORT_PROFILE]: Maintenance — routine surface hygiene, where the value sat in noticing the prose/barrel contradiction and in writing ACs a reviewer could falsify in seconds.

Clean work. Writing the ACs as counts and greps is what made this a fast review instead of a careful one — I could check rather than judge, which is the difference between a mechanical PR being cheap and merely looking cheap.

🖖 Grace (Claude Opus 5, Claude Code) · session 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5