PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The helper appears mechanically coherent, but the solution shape contradicts the current source of authority for AiConfig: ADR 0019 says tests must isolate by construction and must not mutate the shared AiConfig singleton. This PR turns the stale mutation pattern into a shared fixture, so the correct next step is not merge-polish; it is a shape correction.

Peer-Review Opening: Thanks for moving quickly on the test-bleed hazard. The focused helper spec passes locally, but the current ADR changes the acceptable fix shape enough that I cannot approve this as a closing PR for #12435.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12435 issue body and comments; changed-file list for PR #12660; ADR 0019 learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md; unit-test workflow; live CI state at head d4691d63b3ed96555d0b16bdfaec8eab1019b8a8; focused local execution of test/playwright/unit/test/fixtures/aiConfigIsolation.spec.mjs.
• Expected Solution Shape: A correct B4 cleanup should reduce or eliminate runtime writes to the shared AiConfig Provider singleton, or explicitly route tests through a by-construction/injected test config boundary. It must not hardcode a local graph path into cloud/runtime code, and it must not create a reusable pattern that invites future tests to mutate the shared SSOT and rely on cleanup. Test isolation should be structurally safe even under failure ordering, not only restored on the happy path.
• Patch Verdict: Contradicts the expected shape. The new fixture is named and documented as a general-purpose captureAiConfigKeys(aiConfig, keys) path, and the example explicitly teaches “capture first, then mutate” against the shared singleton. That is the stale ticket prescription, not the current ADR 0019 sanctioned pattern.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12435
• Related Graph Nodes: #12456 AiConfig SSOT cleanup stream; ADR 0019; B4 runtime writes to AiConfig; #12335 live/test DB bleed mechanism.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The PR assumes “snapshot/restore around shared singleton mutation” is still the correct current shape because #12435 says so. I checked the newer ADR authority and it says the opposite: test isolation should be by construction, and B4 runtime writes to AiConfig are the safety-critical class to remove. A helper that standardizes those writes is an attractive partial mitigation, but it also cements the anti-pattern before the lint/fan-out work can eliminate it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description reviewed against mechanical implementation and ADR authority.
• Anchor & Echo summaries: flagged below. The helper JSDoc accurately describes what the code does, but it presents mutation+restore as a sanctioned reusable workflow, which overshoots current ADR 0019.
• [RETROSPECTIVE] tag: none in the PR body.
• Linked anchors: #12435 does establish the older snapshot/restore prescription; ADR 0019 supersedes that prescription for current work.

Findings: Required Action. The documentation and helper name should not teach future tests to mutate the shared Provider singleton as the normal solution.

🧠 Graph Ingestion Notes

• [KB_GAP]: ADR 0019 is now the controlling source for this surface; #12435’s original body is stale where it prescribes a generic snapshot/restore helper as the fix.
• [TOOLING_GAP]: GitHub CI unit is red on head d4691d63b3ed96555d0b16bdfaec8eab1019b8a8; the job-log fetch timed out in my sandbox, so I am not asserting the cause.
• [RETROSPECTIVE]: This is a good example of “tickets can stale”: the implementation follows the ticket text, but the current ADR moved the acceptable solution boundary.

🎯 Close-Target Audit

• Close-targets identified: #12435
• #12435 labels verified: enhancement, ai, testing; not epic.
• PR body uses newline-isolated Resolves #12435.
• Branch commit messages use ticket-id commit subjects/bodies; I did not find Closes / Fixes close-target drift in the commit list.

Findings: Syntax/label pass, but close-target validity is blocked by the Evidence Audit below.

📑 Contract Completeness Audit

Findings: N/A — this introduces an internal test fixture and spec wiring, not a runtime public API, MCP tool contract, or operator-facing consumed surface with a Contract Ledger.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is not yet enough to close #12435 as written. The ticket AC explicitly requires a representative full-suite --workers=1 negative isolation run, while the PR body moves that to Post-Merge Validation and CI unit is currently red.
• Residual handling is not aligned with a closing PR. If the full-suite run remains deferred, the PR should not use Resolves #12435 unless #12435 is updated to record the residual/deferment explicitly.
• Evidence-class collapse checked: I am not promoting the passing helper spec to a full-suite isolation proof.

Findings: Required Action. Either satisfy/downgrade the AC before closing #12435, or change the PR to a non-closing partial mitigation.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml descriptions changed.

🔗 Cross-Skill Integration Audit

• Checked ADR 0019 because this is ai/ config work.
• Existing source-of-authority conflict found: the PR introduces a pattern that the ADR and future lint stream are supposed to retire.

Findings: Required Action. The fix shape must converge with ADR 0019, not with the stale pre-ADR ticket wording.

🧪 Test-Execution & Location Audit

• Branch checked out locally at d4691d63b3ed96555d0b16bdfaec8eab1019b8a8.
• Canonical Location: the helper lives under test/playwright/fixtures/; the contract spec lives under test/playwright/unit/test/fixtures/, consistent with a test fixture contract.
• Ran the focused helper contract spec: npm run test-unit -- test/playwright/unit/test/fixtures/aiConfigIsolation.spec.mjs → 4/4 passed locally.
• GitHub unit check is failing on the same head; integration is green.

Findings: Local helper contract passes, but the PR remains unmergeable because CI unit is red and the solution shape is ADR-incompatible.

📋 Required Actions

To proceed with merging, please address the following:

• Rework the #12435 closeout shape so it does not introduce a general shared fixture that normalizes runtime writes to the shared AiConfig singleton. Align with ADR 0019’s by-construction isolation rule, or explicitly downgrade this PR to a non-closing, timeboxed partial mitigation with the remaining ADR-compliant cleanup tracked.
• Resolve the red GitHub unit check on head d4691d63b3ed96555d0b16bdfaec8eab1019b8a8.
• Fix the close-target/evidence mismatch: either provide the required representative full-suite --workers=1 isolation evidence before keeping Resolves #12435, or change the PR body/ticket state so #12435 is not auto-closed with that AC still deferred.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 35 - 65 points deducted because the diff standardizes the B4 AiConfig mutation pattern ADR 0019 says to remove, despite keeping the mutation inside tests.
• [CONTENT_COMPLETENESS]: 65 - 35 points deducted because the helper and PR body are thorough for the stale ticket shape, but they do not reconcile that shape with ADR 0019 or the unresolved full-suite AC.
• [EXECUTION_QUALITY]: 45 - 55 points deducted because the focused helper spec passes locally, but GitHub unit is red and the helper does not protect failure-path/test-abort ordering as strongly as by-construction isolation.
• [PRODUCTIVITY]: 45 - 55 points deducted because this reduces some immediate cross-test bleed risk but does not honestly complete #12435 under the current ADR/evidence bar.
• [IMPACT]: 75 - 25 points deducted because the surface is test-only, but it sits on a safety-critical multi-tenant data isolation failure mode.
• [COMPLEXITY]: 70 - Medium-high: one helper plus 11 migrated specs across Memory Core/ingestion/orchestrator tests, with top-level mutation and cleanup-order edge cases.
• [EFFORT_PROFILE]: Heavy Lift - Broad test migration with real safety value, but the architecture premise needs correction before merge.

Please treat this as a shape blocker, not a request to patch the helper edges. The stale ticket intent was “stop shared config bleed”; current reality says the durable fix is to stop relying on shared-singleton mutation as the normal isolation mechanism.