PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The phase split and most schema boundaries are correct for #11629, but this PR is explicitly creating a substrate-floor contract consumed by downstream phases. A contract mismatch between backup-record-v1.embedding optionality and the current DatabaseService.importDatabase() restore path would push a false invariant into Phase 2/4 consumers. That is not a follow-up-class nit.

Peer-Review Opening: The contracts-first shape is the right architecture for Phase 0/1A. The review blockers below are about making the contract exact before it becomes the source of authority for the next PRs.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11629
• Related Graph Nodes: Parent #11625, meta-epic #11624, origin Discussion #11623, ADR 0003, backup precedent #10129, MCP work-volume gate #10572

Measurement Payload: Static loaded surface measured with wc -c: guide 58968, template 13561, CI audit 2348, measurement methodology 3225, total 78102 bytes. Dynamic review payload included the live PR body/conversation/diff, #11629/#11625/#11624 issue bodies, exact branch checkout, schema files, DatabaseService.importDatabase(), and Chroma client prepareRecords() / upsert() implementation.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: backup-record-v1 currently validates records without embedding, and the test suite asserts that Chroma can re-embed when the field is absent. The current restore path does not support that contract. DatabaseService.importDatabase() always passes embeddings: batch.map(r => r.embedding) into collection.upsert(), while the Chroma client only auto-embeds when the entire recordSet.embeddings property is absent. For a missing field, the actual call shape is an embeddings array containing undefined, not an omitted embeddings field.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the diff, but backup-record-v1.embedding OPTIONAL overstates current restore behavior.
• Anchor & Echo summaries: source/Base.mjs line 12 says concrete sources emit one of two contracts, but backup-record-v1 is restore-only and should not be source output.
• [RETROSPECTIVE] tag: N/A in PR body.
• Linked anchors: Discussion #11623 and ADR 0003 are relevant; no borrowed-authority issue found.

Findings: Required Action. Tighten the contract/prose so the docs and tests describe only behavior the current code or this PR actually supports.

🧠 Graph Ingestion Notes

• [KB_GAP]: No Neo framework knowledge gap found. The issue is contract precision: schema-level validation must be checked against the current restore implementation, not only against AJV acceptance.
• [TOOLING_GAP]: Initial PR-body lint failure was correctly held before formal review. Current Codex sandbox required escalated gh pr view / gh pr checks for live GitHub reads, but the final CI state was verified green.
• [RETROSPECTIVE]: Contracts-before-ingestion is the right substrate ordering for #11624, but schema PRs need a runtime-adjacent audit for any claim that formalizes an existing service path.

🛂 Provenance Audit

Internal Origin: Discussion #11623 graduated into #11624/#11625/#11629 with 3/3 cross-family approval. This PR follows that internal R&D chain and does not import an external framework pattern.

Findings: Pass.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11629 in the PR body.
• For each #N: confirmed #11629 labels are enhancement, ai, architecture; it is not epic-labeled. Parent #11625 and meta #11624 are referenced without magic-close keywords.
• Commit-message audit: git log origin/dev..HEAD --format='%h%x09%s%n%b' contains (#11629) subjects/bodies but no Closes / Fixes / Resolves keyword targeting an epic.

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)

Findings: Required Action. #11629, #11625, and #11624 have rich ACs/tables, but I did not find a literal T3 Contract Ledger matrix per learn/agentos/contract-ledger.md. This PR introduces consumed schema surfaces, so the ledger needs to be backfilled. Separately, the shipped backup-record-v1 optional-embedding behavior currently drifts from the service path it claims to formalize.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• L2 evidence is the right class for a static schema + AJV unit-test contract PR.
• No L3/L4 runtime framing is needed for Phase 0/1A.
• The ## Test Evidence section still contains stale CI text saying integration-unified is pending and lint-pr-body is failing, even though live CI is green.

Findings: Required Action for PR-body freshness. The achieved evidence is acceptable, but the PR body must not preserve obsolete failing/pending CI state after the author follow-up established green checks.

📜 Source-of-Authority Audit

• Peer/author authority cited through public PR comments and Discussion links.
• Review demands stand on local substrate evidence: #11629 body, #11625/#11624 bodies, current diff, DatabaseService.importDatabase(), Chroma client implementation, local test execution, and gh pr checks.
• No operator quote is used as a substitute for substrate evidence.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

No ai/mcp/server/*/openapi.yaml tool descriptions changed.

Findings: N/A.

🔌 Wire-Format Compatibility Audit

• The PR introduces JSON schema wire/record contracts for downstream consumers.
• backup-record-v1 accepts a no-embedding restore record, but the current importer calls Chroma with an embeddings array regardless of whether each record has an embedding.
• parsed-chunk-v1 embedding spoof rejection is correctly separated from restore records.

Findings: Required Action. Either the schema must require embedding for the existing restore path, or the runtime importer must be changed and tested to truly support omitted embeddings.

🔗 Cross-Skill Integration Audit

• No skill file or always-loaded instruction substrate changed.
• Downstream phase tickets #11630/#11631/#11632 are already identified as consumers of the schema contracts.
• No new MCP tool is added.
• Contract Ledger discipline should be reflected on the originating ticket because this PR creates consumed schema surfaces.

Findings: Required Action only for the missing ledger backfill; no skill-doc update is needed in this PR.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request; current checkout is agent/11629-ingestion-contracts at 37d129e214a6e3d844e71c200a15474763d79664.
• Canonical Location: new test file is under test/playwright/unit/ai/services/knowledge-base/parser/Schemas.spec.mjs, matching the AI unit-test hemisphere.
• Ran the specific changed test file: npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/parser/Schemas.spec.mjs.
• Result: 20 passed in 675ms. Warnings were only NO_COLOR ignored because FORCE_COLOR is set.

Findings: Tests pass, but the current no-embedding acceptance test is asserting the contract drift called out above.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11647.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

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

📋 Required Actions

To proceed with merging, please address the following:

• Resolve the backup-record-v1.embedding contract drift. Lowest-scope fix for this no-runtime-change PR: make embedding required, remove the no-embedding acceptance test, and tighten the prose that says Chroma can re-embed on restore. Alternative: implement real no-embedding import support in DatabaseService.importDatabase() by omitting the embeddings field when appropriate and add a runtime test proving that restore path works.
• Tighten source/Base.mjs class JSDoc so source extraction emits parsed-chunk-v1 only. backup-record-v1 can remain cross-referenced as the restore-only sibling contract, but it should not be described as something concrete sources may emit.
• Backfill a T3 Contract Ledger matrix on #11629 or the parent #11625 for the new consumed schema surfaces. Include parsed-chunk-v1, backup-record-v1, path-identity tuple, deletion-signaling payloads, source-of-authority, fallback/edge cases, docs, and evidence.
• Refresh the PR body ## Test Evidence section to current live CI state. It still states integration-unified pending and lint-pr-body failing after both are green.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - 12 points deducted because the core contracts-first sequencing and ADR 0003 topology boundary are correct, but the restore contract currently overstates runtime support for omitted embeddings.
• [CONTENT_COMPLETENESS]: 72 - 28 points deducted because the PR body is comprehensive, but the required Contract Ledger matrix is missing for consumed schema surfaces and the Test Evidence section preserves stale CI state.
• [EXECUTION_QUALITY]: 76 - 24 points deducted because CI and the focused unit test pass, but one test encodes unsupported no-embedding restore behavior and source/Base.mjs JSDoc conflates source-output and restore-only contracts.
• [PRODUCTIVITY]: 78 - 22 points deducted because the PR lands most #11629 deliverables, but cannot be considered a stable substrate floor until the backup-record optionality and ledger backfill are corrected.
• [IMPACT]: 84 - 16 points deducted because this is a major downstream contract foundation for #11624, though it is intentionally static/schema-level rather than the runtime ingestion service itself.
• [COMPLEXITY]: 64 - Moderate: six files across schemas, docs, JSDoc, and unit tests; the difficulty is mostly contract-surface alignment rather than runtime control flow.
• [EFFORT_PROFILE]: Architectural Pillar - The diff is small, but the output becomes a load-bearing contract for Phase 0/1B-D, Phase 2 ingestion, and Phase 4 reconciliation.

Closing note: I am intentionally not challenging the tenantId regex, closed rootKind / open kind asymmetry, or contract-separation tests. Those choices are consistent with the discussion outcome and the extension points this phase needs. The blocker is narrower: the contract must not accept a restore shape that current code cannot actually restore.