PR Review Summary

Status: Request Changes

Cycle-1 reviewers: run §9.0 Premise Pre-Flight BEFORE composing Required Actions. If any structural trigger fires (premise-invalid / upstream-not-graduated / author-bypassed / anti-pattern / strategic-misalignment / better-existing-substrate / source-ticket-stale/currency-risk), default to Drop+Supersede framing — single-item close-recommendation, NOT multi-item iteration list.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Request Changes, not Drop+Supersede, because the substrate-level collection-name guard is the right architecture and the branch is close. The blockers are contract/coverage mismatches against #11652: the PR narrows one explicit AC without declaring the delta, and one outside-manager bare client delete fallback remains in the diff.

Peer-Review Opening: The direction is correct: moving the protection from a test-helper-only check to a Chroma collection-name boundary is the right response to the wipe incident. I cannot approve the current head yet because the review evidence shows two close-target contract gaps that would leave the merged artifact weaker than #11652 promises.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11652
• Related Graph Nodes: #10867 prior insufficient helper-layer guard, #10845 DestructiveOperationGuard, #11656 PR, Memory Core wipe recovery, Knowledge Base Chroma collection guard

Measurement Payload: Static loaded surface: guide/template/audit payloads 76,150 bytes (wc -c .agents/skills/pr-review/SKILL.md .agents/skills/pr-review/references/pr-review-guide.md .agents/skills/pr-review/assets/pr-review-template.md .agents/skills/pr-review/audits/ci-security-audit.md). Dynamic payloads measured: PR body/comments 5,937 bytes; PR diff 29,569 bytes.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: #11652 explicitly asks the new canonical guard spec to prove a test-prefixed delete without UNIT_TEST_MODE refuses. The current implementation and test assert the opposite: assertCanonicalCollectionDeleteAllowed() returns for non-canonical names before checking UNIT_TEST_MODE, and the spec names that as allowed behavior. That may be a defensible narrowing, but it is not declared as a ticket delta or residual, while the PR body says all #11652 ACs are mechanically covered.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: partial drift. The PR states zero remaining bare client.deleteCollection callers and all four callers rerouted, but rg -n "client\.deleteCollection|deleteCollection\(" ai test/... still finds an outside-manager fallback in CollectionProxy.mjs.
• Anchor & Echo summaries: mostly precise and useful; the Memory Core / KB wrapper JSDoc correctly describes the canonical-name boundary.
• [RETROSPECTIVE] tag: N/A; no review tag in the PR body.
• Linked anchors: #11652, #10867, and #10845 are relevant and establish the stated lineage.

Findings: Required Actions below cover the drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The implementation correctly identifies that the prior helper-layer guard was insufficient and moves protection closer to the Chroma client boundary.
• [TOOLING_GAP]: None in this review cycle. gh pr checks 11656 is green; local targeted unit execution passed.
• [RETROSPECTIVE]: Collection-name guarding is the correct substrate layer for this incident class, but close-target AC drift still matters: if a PR claims all destructive-call paths are guarded while one bare fallback remains, future agents will inherit an overconfident guard model.

🛂 Provenance Audit

• Internal Origin: Incident-derived internal Neo substrate hardening from #11652 / #10867 / #10845 and the 2026-05-17 Memory Core wipe recovery.

Findings: Pass. No external framework provenance issue; this is native incident response.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: Resolves #11652 in PR body. Commit subject also references (#11652) without a magic close keyword.
• For #11652: confirmed labels are bug, ai, testing, architecture; no epic label.

Findings: Pass for close-target validity and syntax. Fails on close-target AC completion, tracked under Required Actions.

📑 Contract Completeness Audit

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

Findings: N/A for formal Contract Ledger. #11652 is an internal bug/hardening ticket, not a public API contract ticket. I instead audited against the issue Acceptance Criteria and found drift in Required Actions.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section.
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed].
• Two-ceiling distinction: PR body frames evidence as unit-test-verifiable L2.
• Evidence-class collapse check: review language does not promote L2 tests to live daemon / production restore proof.

Findings: Evidence mismatch. The unit suite is green, but #11652 includes AC text requiring the test-prefixed / missing-UNIT_TEST_MODE refusal scenario and a direct-client-unreachable or lint gate. The current PR neither implements those exact requirements nor marks them as residuals.

📜 Source-of-Authority Audit

N/A. This review cites public GitHub issue/PR state, PR diff, CI, and local repo evidence; no private operator directive is used as a Required Action authority.

📡 MCP-Tool-Description Budget Audit

N/A. The PR does not touch ai/mcp/server/*/openapi.yaml.

🔌 Wire-Format Compatibility Audit

N/A. The PR does not alter JSON-RPC, A2A, or native wire payload schemas.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? N/A; this is code hardening, not a workflow 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 skill/doc substrate change introduced.
• 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 somewhere (when it applies, how it fires)? N/A.

Findings: Pass — no cross-skill integration gap.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request on PR #11656.
• Canonical Location: New spec is under test/playwright/unit/ai/services/memory-core/managers/, matching the modified manager surface.
• If a test file changed: Ran the specific test file plus related regression specs.
• If code changed: Verified related tests exist and ran them.

Findings: Tests pass locally: npm run test-unit -- test/playwright/unit/ai/services/memory-core/managers/ChromaManager.canonicalGuard.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs test/playwright/unit/ai/services/memory-core/managers/ChromaManager.spec.mjs test/playwright/unit/ai/services/knowledge-base/DatabaseService.backup.spec.mjs test/playwright/unit/ai/services/knowledge-base/VectorService.WorkVolumeBranching.spec.mjs → 32 passed. Coverage still needs adjustment because one spec encodes the opposite of #11652’s stated missing-env expectation, and KB wrapper integration is not directly covered.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11656 to empirically verify CI status.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

Findings: Pass - all checks green: Analyze, CodeQL, check, integration-unified, lint-pr-body, and unit all pass on current head ea2d136dc.

📋 Required Actions

To proceed with merging, please address the following:

• Reconcile the missing-UNIT_TEST_MODE behavior with #11652. The ticket asks for the new ChromaManager.canonicalGuard.spec.mjs to cover “test-prefixed name without UNIT_TEST_MODE → expects throw,” but the current code returns before checking env for all non-canonical names (DestructiveOperationGuard.mjs:87-90) and the spec asserts test-memory-12345 without env does not throw (ChromaManager.canonicalGuard.spec.mjs:35-42). Either implement the stricter invariant or explicitly update #11652/PR body to declare this as an intentional scope delta with residual risk. As written, the PR claims all #11652 ACs are covered when one is inverted.
• Remove or guard the remaining outside-manager bare client delete fallback in CollectionProxy.mjs:123-126. #11652 AC3 requires direct ChromaManager.client.deleteCollection access to be made unreachable or blocked by a grep/lint alternative, and the PR body claims zero remaining bare callers. A direct rg -n "client\.deleteCollection" ai/ test/ still finds this outside-manager fallback. Preferred fix: drop the fallback and require manager.deleteCollection({name, confirmation}); if a future vector manager cannot implement the guarded wrapper, it should fail closed rather than falling back to raw client deletion.
• Add direct KB-side wrapper coverage or equivalent call-site coverage for the ai/services/knowledge-base/ChromaManager.mjs wrapper. The PR says the invariant is mirrored across MC and KB, but the current wrapper integration tests only exercise Memory Core ChromaManager.deleteCollection; KB is represented by set membership and call-site rewiring. For this destructive boundary, add a small test that proves neo-knowledge-base refuses before client.deleteCollection without env/confirmation and forwards without leaking confirmation when bypass is valid.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the substrate-layer guard placement is correct, but the remaining raw-client fallback contradicts the “all destructive collection deletes go through the wrapper” architectural boundary.
• [CONTENT_COMPLETENESS]: 74 - 26 points deducted because the PR body now satisfies the mechanical template, but it overstates AC completion: it claims no residuals while the missing-env non-canonical behavior and raw-client fallback remain unresolved against #11652.
• [EXECUTION_QUALITY]: 72 - 28 points deducted because CI and targeted local tests pass, but one new test encodes behavior opposite to a stated AC, and KB wrapper integration is not directly covered.
• [PRODUCTIVITY]: 78 - 22 points deducted because the main protection for canonical production names is delivered, but the PR does not yet satisfy all close-target ACs needed for merge.
• [IMPACT]: 88 - 12 points deducted because this is high-impact incident hardening protecting live Memory Core / KB substrate, but it is narrower than a full destructive-operation unification.
• [COMPLEXITY]: 72 - Medium-high: 8 files, two subsystems, shared guard code, production recovery bypass semantics, and both CI/test-helper and manager-level destructive paths.
• [EFFORT_PROFILE]: Heavy Lift - High value and non-trivial risk surface; the code is not huge, but the blast radius of getting destructive-substrate guards wrong is large.

This is close, but the remaining gaps are exactly where this PR needs to be strict: destructive operations must fail closed, and the PR body must not describe a stronger invariant than the diff actually enforces.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: Cycle 1 had three code/coverage blockers; the latest commit fixes those, and this review checks the delta plus the remaining PR-body substrate.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code delta is now approval-shaped, but the PR body still describes the pre-fix behavior. Because this PR is a destructive-substrate guard, stale evidence text would mislead future recovery work and graph ingestion.

Prior Review Anchor

• PR: #11656
• Target Issue: #11652
• Prior Review Comment ID: PRR_kwDODSospM8AAAABAYwPjg / https://github.com/neomjs/neo/pull/11656#pullrequestreview-4320923534
• Author Response Comment ID: IC_kwDODSospM8AAAABC54Jow / https://github.com/neomjs/neo/pull/11656#issuecomment-4489873827
• Latest Head SHA: 257cb9469

Delta Scope

• Files changed: ai/mcp/server/shared/services/DestructiveOperationGuard.mjs, ai/services/memory-core/managers/CollectionProxy.mjs, test/playwright/unit/ai/services/memory-core/managers/ChromaManager.canonicalGuard.spec.mjs.
• PR body / close-target changes: Still open. The PR body remains the Cycle 1 body and does not reflect commit 257cb9469.
• Branch freshness / merge state: PR open; latest head 257cb9469; all current GitHub checks green.

Previous Required Actions Audit

• Addressed: Uniform gate for missing UNIT_TEST_MODE — assertCanonicalCollectionDeleteAllowed() now requires UNIT_TEST_MODE=true or the production confirmation token for every delete attempt, regardless of canonical-name membership.
• Addressed: Remove outside-manager bare client fallback — CollectionProxy.drop() now fails closed when a manager lacks deleteCollection({name, confirmation}).
• Addressed: KB-side wrapper coverage — the canonical-guard spec now includes direct KB ChromaManager.deleteCollection refusal / UNIT_TEST_MODE forward / confirmation-token forward coverage.
• Still open: PR body evidence and commit metadata were not updated after the fix commit, so the public PR body still documents the old behavior.

Delta Depth Floor

• Delta challenge: The code is now stricter than the PR body says. The body still lists “Non-canonical (test-prefixed) name pass-through regardless of env” as test item 2, but the corrected code/test now intentionally refuses non-canonical names without env/token.

Test-Execution & Location Audit

• Changed surface class: code + tests.
• Location check: Pass. The updated tests stay in test/playwright/unit/ai/services/memory-core/managers/ChromaManager.canonicalGuard.spec.mjs, appropriate for the shared guard and manager-wrapper integration surface.
• Related verification run:

npm run test-unit -- test/playwright/unit/ai/services/memory-core/managers/ChromaManager.canonicalGuard.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/DestructiveOperationGuard.spec.mjs

Result: 28 passed (820ms).

• Findings: Pass.

Contract Completeness Audit

• Findings: Pass for the code delta. The implemented behavior now matches #11652's stricter missing-env expectation and the prior review's fail-closed fallback requirement.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11656 to empirically verify CI status.
• Confirmed no checks are pending/in-progress.
• Confirmed no current checks are failing.

Findings: Pass - all current checks green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, and unit.

Metrics Delta

• [ARCH_ALIGNMENT]: 82 -> 96 - The prior architectural deduction is removed because the fail-closed wrapper invariant is now consistent across non-canonical names, MC, KB, and CollectionProxy.
• [CONTENT_COMPLETENESS]: 74 -> 78 - Code/test comments are now strong, but the PR body still carries stale test evidence and commit metadata, so the content score cannot recover yet.
• [EXECUTION_QUALITY]: 72 -> 94 - The targeted local test run passes, CI is green, the previous inverted test behavior is corrected, and KB wrapper coverage is present.
• [PRODUCTIVITY]: 78 -> 90 - All substantive code blockers are addressed; only PR-body repair remains before merge eligibility.
• [IMPACT]: unchanged from prior review (88) - Same high-impact destructive-substrate hardening.
• [COMPLEXITY]: unchanged from prior review (72) - The delta keeps the same subsystem and guard complexity profile.
• [EFFORT_PROFILE]: unchanged from prior review (Heavy Lift) - High-value incident hardening with non-trivial blast radius.

Required Actions

To proceed with merging, please address the following:

• Refresh the PR body to match commit 257cb9469: update the Evidence/Test Evidence counts from the old 10-test description, remove the stale “non-canonical pass-through regardless of env” item, describe the uniform env-or-token gate, mention KB-side wrapper coverage, and add the second commit to the Commits list. The code is now stricter than the PR body; the PR body needs to stop teaching the old invariant.

A2A Hand-Off

ReviewId will be relayed to the author via direct A2A DM after this formal review posts.

Closing: I do not see remaining code blockers in the current head. This should be a PR-body refresh cycle, not another implementation cycle.