PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve because the PR removes the latent whole-.neo-ai-data release artifact path, routes both release and install through the existing KB JSONL backup/import substrate, and adds focused guard coverage for the leak classes named by #12157. The remaining live-daemon export/import proof is correctly declared as post-merge validation rather than hidden inside this unit-testable rewrite.

Peer-Review Opening: Reviewed the release upload/download rewrite, the new artifact guard, the real KB backup/import SDK, the close-target issue, and the targeted test evidence. The implementation is merge-eligible.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12157
• Related Graph Nodes: Epic #12153; ADR 0017 / PR #12159; #12152; KB_DatabaseService.manageDatabaseBackup; backup.mjs; restore.mjs; package.json prepare; release publish pipeline

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: I found one direct-call seam worth watching: downloadKnowledgeBase() calls lifecycleService.ready() before entering its soft-fail try block, so a test seam that rejects there makes the exported function throw instead of returning {status:'error'}. I am not making this a required action because the actual CLI/prepare entrypoint catches that path and exits non-fatally, and a local CLI probe confirmed the user-facing script soft-fails on fetch failure. If this exported helper becomes a supported SDK surface, fold the pre-download lifecycle/setup work into the same soft-fail block and add a regression test.

Rhetorical-Drift Audit (per guide §7.4):

Verify symmetry between stated framing and mechanical implementation:

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology; no durable JSDoc/source comments add transient lane/AC framing
• [RETROSPECTIVE] tag: N/A — no retrospective tag added
• Linked anchors: #12157, #12153, and ADR 0017 establish the collection-scoped release requirement

Findings: Pass. The PR body's L3 post-merge note is honest about what the unit tests do not prove.

🧠 Graph Ingestion Notes

• [KB_GAP]: The current KB retrieval still describes Chroma/KB collection concepts broadly, but not the newly corrected release artifact shape; this PR is the concrete substrate update.
• [TOOLING_GAP]: Non-escalated GitHub calls intermittently failed with error connecting to api.github.com; escalated rechecks succeeded. A local downloadKnowledgeBase.mjs CLI probe hit sandbox network fetch failed, but verified the script exits non-fatally.
• [RETROSPECTIVE]: The important architectural move is making privacy a collection/export contract instead of a folder contract. The guard rejects Memory Core collection names, SQLite payloads, unexpected JSONL, and empty KB exports on both producer and consumer paths.

🎯 Close-Target Audit

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

• Close-targets identified: #12157
• For each #N: confirmed not epic-labeled (bug, ai)

Findings: Pass. The PR body uses Resolves #12157; branch commit history contains only #12157 as the close target.

📑 Contract Completeness Audit

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

Findings: Pass. #12157 asks for collection-scoped upload/import, no .neo-ai-data directory restore, no Memory Core leak, and asset-name reconciliation. The diff covers those surfaces through uploadKnowledgeBase.mjs, downloadKnowledgeBase.mjs, and knowledgeBaseArtifact.mjs; publish.mjs continues to invoke the upload script, so the asset-name constant flows through the existing pipeline entrypoint.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Post-Merge Validation section
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed] — N/A, the ticket itself marks artifact inspection as post-merge
• Two-ceiling distinction: PR body distinguishes L2 unit/script proof from L3 live-daemon release-export proof
• Evidence-class collapse check: review language does NOT promote L2 to L3

Findings: Pass. I independently ran the new artifact tests, backup/restore regression tests, and KB backup SDK tests; the live release export/import remains correctly listed as post-merge validation.

📡 MCP-Tool-Description Budget Audit

For every modified or added OpenAPI tool description:

• Single-line preferred — block-literal (|) descriptions justified by content, not authorial habit — N/A
• No internal cross-refs (no ticket numbers, Phase sequencing, session IDs, or memory anchor names in the description payload) — N/A
• No architectural narrative — descriptions describe call-site usage — N/A
• External standard URLs OK — N/A
• 1024-char hard cap respected — N/A

Findings: N/A — no OpenAPI/tool-description surfaces touched.

🛂 Provenance Audit (§7.3)

Sources checked:

• PR head a0353d0bdc84cd9b1cde4776cdf66687f9a44a88
• #12157 close-target body and #12153 parent epic body
• ADR 0017 / PR #12159 decision surface for collection-scoped shipping
• ai/services/knowledge-base/DatabaseService.mjs export/import behavior: KB-only export prefix and directory import support
• ai/scripts/maintenance/backup.mjs / restore.mjs existing SDK callers
• package.json ai:download-kb and prepare entrypoints
• buildScripts/release/publish.mjs release upload invocation

Findings: Pass. The PR reuses the existing KB database service rather than inventing a parallel export/import codec.

🔌 Wire-Format Compatibility Audit

• Producer writes a flat artifact containing knowledge-base-backup-*.jsonl plus kb-artifact-meta.json, not an on-disk data directory
• Consumer validates the artifact before import and imports the extracted directory through the existing KB JSONL restore path
• The artifact keeps the historical published filename chroma-neo-knowledge-base.zip, avoiding a release-asset URL break
• Embedding provider/dimension provenance is sidecar metadata and does not alter the existing KB backup-record-v1 JSONL rows

Findings: Pass. The payload envelope changes from unsafe directory zip to flat collection export, while the row format remains the existing KB backup/import record shape.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern?
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating?
• Does any reference file mention a predecessor pattern that should now also mention the new one?
• 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)?

Findings: All checks pass for this PR scope. The operator-facing guide updates are already tracked under #12158, so this PR does not need to mutate broader docs.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request
• Canonical Location: new test file is under test/playwright/unit/ai/scripts/maintenance/, matching right-hemisphere maintenance-script placement
• If a test file changed: ran the specific test file
• If code changed: ran related script/unit regression coverage

Findings: Tests pass. Local validation:

• git diff --check origin/dev...HEAD
• node --check on uploadKnowledgeBase.mjs, downloadKnowledgeBase.mjs, knowledgeBaseArtifact.mjs, and knowledgeBaseArtifact.spec.mjs
• node ./buildScripts/util/check-whitespace.mjs
• npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/knowledgeBaseArtifact.spec.mjs → 13 passed
• npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/backup.spec.mjs test/playwright/unit/ai/scripts/maintenance/restore.spec.mjs → 18 passed
• npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/DatabaseService.backup.spec.mjs → 3 passed
• Current PR checks are green: Analyze, CodeQL, check, integration-unified, lint-pr-body, unit

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - 4 points deducted only for the minor direct-call soft-fail seam noted above; the main architecture correctly uses the existing KB backup/import service and collection boundary.
• [CONTENT_COMPLETENESS]: 94 - 6 points deducted because downloadKnowledgeBase() JSDoc says every failure path returns, while pre-try lifecycle failures can still reject direct callers; otherwise the Anchor & Echo context is thorough.
• [EXECUTION_QUALITY]: 93 - 7 points deducted for the same untested direct seam and lack of L3 live-daemon export proof in this PR; all reachable static/unit/regression checks passed.
• [PRODUCTIVITY]: 97 - Achieves the #12157 privacy fix directly, including producer guard, consumer guard, merge-only ingest, and asset-name reconciliation.
• [IMPACT]: 96 - Prevents release artifacts from carrying private Memory Core/graph/mailbox data; high-impact privacy and data-safety fix.
• [COMPLEXITY]: 82 - High: rewrites both release producer/consumer flows, introduces a shared artifact contract, and crosses release, install, Chroma, and backup/restore boundaries.
• [EFFORT_PROFILE]: Heavy Lift - A compact file count, but the behavior spans public release artifacts, npm install, Chroma import/export, and privacy guarantees.

Approved. Merge remains human-only.