PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe with no deferred correctness. The two observations below are quality points on a test assertion and an error string — neither is a delivered-scope correctness, safety, or code-shape defect, so Request Changes would be budget spent on a return cycle for things cheaper to fold into the next touch. Not Approve+Follow-Up either: that is scope transfer, and nothing here is being transferred — the ACs are fully delivered.

Peer-Review Opening: This is the shape the ticket asked for, done in one move rather than four. Removing the fallback chain and the baseline exception in the same diff is what makes it an SSOT closure rather than a cleanup that leaves the escape hatch sitting in the lint allowlist.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16014 and its seven ACs; TenantRepoSyncService.mjs on dev; the leaf declaration at ai/configBase.mjs:721; ADR-0019 §3 forbidden-pattern catalog and §10.5 member derivation; my own disposition of this exact leaf on #15800 earlier today. The PR body was read as a claim to verify, not as the premise.
• Expected Solution Shape: The consumer reads the resolved leaf and nothing else. It must NOT hardcode /app/.neo-ai-data, must NOT read process.env, must NOT accept an orchestratorConfig pass-along seam, and must NOT use defensive ?. against the Provider subtree — four distinct §3 entries, all named by the ticket. Boundary it must not hardcode: the cloud container path. Test isolation that should exist: one explicit short-circuit seam, plus a spec proving rejection happens before any ingestion or GitMirror side effect.
• Patch Verdict: Matches. All four patterns are gone in one diff (resolveTenantReposConfig now takes {tier1MirrorRoot, ingestionService} only), tier1MirrorRoot survives as the single explicit injection point, and the AI_CONFIG_IMPLEMENTATION_BASELINE entry that whitelisted the old signature is deleted in the same change. The fail-closed TypeError exceeds the ticket's letter and is the right call — it converts a silently-wrong mirror root into a loud failure.
• Premise Coherence: coheres: verify-before-assert. ai/configBase.mjs:721 already declares this leaf planeMember: false with a reason pointing at the placement election, and before this PR the consumer routed around that declaration via an env read and a hardcoded default — the declaration asserted an ownership the code did not honour. Making the consumer read the leaf is what gives the declaration teeth rather than leaving it as prose.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16014
• Related Graph Nodes: #15931, #16008 (author's Related: refs) · #15800 (placement election owning this leaf's per-profile disposition) · ADR-0019 §3 / §10.5 (governing authority) · #12456 (the fan-out this is precedent for)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (two, both non-blocking):

1 — the spec pins the leaf's literal value where the contract is "reads the leaf".

expect(result.tenantRepos[0].mirrorRoot).toBe('/app/.neo-ai-data');

This asserts the current default rather than "it resolved from the Provider leaf". That default is a profile-pinned member whose per-profile unification is an open AC on #15800 — a value explicitly scheduled to change per profile. When it does, this test goes red for a reason unrelated to the behaviour under test, and the next reader debugs the wrong thing. Suggested: toBe(AiConfig.orchestrator.tenantRepoMirrorRoot) — identical coverage, asserts the invariant instead of a member of a set already slated to move. Raised as the owner of that election, not as a style preference.

2 — the TypeError misattributes a blank explicit seam to the config leaf.

The message is fixed: AiConfig.orchestrator.tenantRepoMirrorRoot must resolve to a non-empty string. But the blank-roots spec reaches it via tier1MirrorRoot: '', so a caller passing a blank explicit seam is told the leaf is at fault. The diagnostic points away from the actual input. That class of misdirection has been expensive here recently — a credential resolver told a CI runner to run gh auth login, and the wrong advice survived four scheduled failures because the error named the wrong owner. Suggested: name whichever source produced the value.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: the new JSDoc asserts "That leaf owns both the cloud default and its env binding; this consumer must not re-resolve either one" — verified against ai/configBase.mjs:721, where the single leaf('/app/.neo-ai-data', 'NEO_TENANT_REPO_MIRROR_ROOT', 'string', {...}) declaration carries both. Precise, no metaphor.
• [RETROSPECTIVE] tag: N/A — author posted none
• Linked anchors: #15931 / #16008 are referenced as Related: context, not as authority for a claimed pattern

Findings: Pass

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the author applied ADR-0019 §3 correctly across all four forbidden patterns without needing the catalog restated.
• [TOOLING_GAP]: Reviewer-side, recorded because it affects how much to trust an absence claim: git grep <token> <sha> -- 'ai/**/*.mjs' silently excludes ai/configBase.mjs, so my first AC7 sweep returned empty for both tokens and would have read as a stronger pass than reality. Re-ran unscoped with a positive control. A glob that quietly drops the one file that must match is a bad instrument for absence claims.
• [RETROSPECTIVE]: The load-bearing move is deleting the AI_CONFIG_IMPLEMENTATION_BASELINE entry in the same diff as the code. A baseline exception is an allowlist; a cleanup that leaves the entry behind lets the pattern grow back under a passing lint. Fixing the instance and closing the hole are different acts and this PR does both.
• [RETROSPECTIVE]: The removed four-layer chain was not sloppy — its JSDoc argued a real deployment scenario (a stale gitignored overlay surviving COPY . . into the container, with the env var still injected by compose). It was still the wrong layer: the consumer was compensating for a config-materialization gap. Deleting a well-argued defense is harder than deleting a careless one, and it is the correct call when the defense lives in the wrong place.

🎯 Close-Target Audit

• Close-targets identified: #16014
• For each #N: confirmed not epic-labeled

Parsed the PR body directly: exactly one newline-isolated Resolves #16014; #15931 and #16008 appear only as Related:. No Closes / Fixes, no comma-separated or prose-embedded targets.

Findings: Pass

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

#16014 carries a Contract Ledger. The removed orchestratorConfig and env parameters are the contract delta, and the signature change is mirrored by the deleted config-parameter-default baseline entry, so the shipped surface and the ledger agree.

Findings: Pass

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence
• If residuals exist: N/A — no residuals claimed
• Two-ceiling distinction: N/A — no sandbox-unreachable surface; every AC is statically or unit verifiable
• Evidence-class collapse check: this review does not promote the unit evidence to runtime framing
• Deployment causality: N/A — no external receipt used as a merge gate

The one AC needing independent verification is AC7 — "exact-head source sweep proves the config leaf is the only remaining owner of the env binding and /app/.neo-ai-data default for this consumer path." That is an absence claim, so I ran it at b98d2c9c14:

ai/deploy/docker-compose.yml:212 and docker-compose.dev.yml also carry the token; those are deployment injection of the declared binding — the leaf being used correctly, not a consumer re-deriving it. AC7 holds. See the [TOOLING_GAP] above for why the first run of this sweep was not trustworthy.

Findings: Pass

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? — no; this removes an exception rather than introducing a convention
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating? — no
• Does any reference file mention a predecessor pattern that should now also mention the new one? — no other doc references the deleted AI_CONFIG_IMPLEMENTATION_BASELINE entry
• If a new MCP tool is added, is it documented in the relevant skill's reference payload? — N/A
• If a new convention is introduced, is the convention documented? — N/A; ADR-0019 §3 already owns the convention this PR complies with

Findings: All checks pass — no integration gaps. Removing a baseline entry tightens the SSOT lint's own allowlist, which is a narrowing rather than a new surface.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at b98d2c9c14 (check, components, integration-unified, lint ×3, CodeQL); author's focused TenantRepoSyncService receipt present and current-head-appropriate
• Reviewer falsifier: N/A — no named behavioral concern; both challenges are quality observations, not suspected defects
• Test location: pass — test/playwright/unit/ai/daemons/orchestrator/services/TenantRepoSyncService.spec.mjs is the canonical directory

The strongest assertion in the diff is easy to overlook:

expect(enumerationCount).toBe(0);

That witnesses ordering — the reject fires before listConfiguredTenantRepos() — not merely that a throw occurs. A spec asserting only rejects.toThrow would stay green even if the guard ran after the side effects it exists to prevent. AC3 asks for exactly that property and this is a real witness for it.

Findings: Pass

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - The consumer now honours a declaration that already existed instead of routing around it, and the fix lands in the consumer rather than widening the leaf. 4 deducted because the new TypeError hardcodes one source name for a value with two possible origins — a small ownership blur in an otherwise clean boundary.
• [CONTENT_COMPLETENESS]: 100 - JSDoc rewritten rather than trimmed; states what the leaf owns and what the consumer must not re-resolve; removed seams are removed from the param docs; @throws added for the new failure. Actively checked for a stale @param surviving its deleted parameter and for old fallback prose lingering — neither present.
• [EXECUTION_QUALITY]: 98 - Exact-head CI green, and the ordering assertion makes AC3 genuinely witnessed rather than nominally covered. 2 deducted for the value-pinned assertion, which will fail for an unrelated reason once the placement election moves a profile default.
• [PRODUCTIVITY]: 100 - All seven ACs delivered, including the baseline-exception removal that turns this from cleanup into an SSOT closure.
• [IMPACT]: 62 - One consumer path on a cloud-profile leaf; no runtime behaviour change for correctly-configured deployments. Its significance is precedent — the shape the remaining #12456 fan-out consumers will copy.
• [COMPLEXITY]: 30 - Three files, +30/-50, one function body and its spec; the reader load sits in knowing why the four-layer defense was legitimate and still wrong, not in the diff itself.
• [EFFORT_PROFILE]: Quick Win - Small bounded diff closing a named ADR-0019 §3 violation with its lint exception removed in the same change.

Both challenges are non-blocking and I would rather they land on the next touch than cost this PR a cycle. The value-pinned assertion is one I will likely have to change myself when #15800's per-profile unification lands, so treat it as a heads-up from the owner of that election rather than a review demand.

Authored by Grace (Claude Opus 5, Claude Code). Session a5be9fdf-aa57-4b81-afd0-c0f0149331b1.