PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Request Changes, not Drop+Supersede, because the registry direction is correct and CI/local targeted tests are green. The blockers are review-surface contract issues: the PR would auto-close #11658 while explicitly deferring one of its ACs, and the newly exposed config registration path is not directly tested/documented enough for a Phase 0/1B substrate primitive.

Peer-Review Opening: The implementation is a good foundation for the source/parser registry. I am blocking on close-target correctness and config-path coverage, because those are exactly the surfaces later cloud-ingestion work will trust.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11658
• Related Graph Nodes: #11624 Cloud-Native KB Ingestion, #11625 Phase 0/1, #11647 Phase 0/1A schemas + identity tuple, SourceRegistry, KB source discovery, config-template clone-sync discipline.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The PR body says Resolves #11658, but also says per-source path externalization is deferred and that #11658 remains open until the path-externalization slice lands. Those cannot both be true under GitHub magic-close semantics; merging this PR would close #11658 while the body says #11658 must remain open.

Rhetorical-Drift Audit (per guide §7.4):

Verify symmetry between stated framing and mechanical implementation:

• PR description: drift flagged on close-target semantics and on the claim that #11658 remains open despite Resolves #11658.
• Anchor & Echo summaries: mostly strong; drift flagged in _export.mjs where the declarative custom-source prose says [{className, sourceName, ...}] while the actual code and config template expect {SourceClass, sourceName?}.
• [RETROSPECTIVE] tag: N/A; no tag present.
• Linked anchors: #11658/#11625/#11624/#11647 are relevant.

Findings: Drift requires Required Actions below.

🧠 Graph Ingestion Notes

• [KB_GAP]: The current KB index does not yet know about SourceRegistry / useDefaultSources; this PR is creating that substrate. The review therefore relies on live diff + issue body + local tests.
• [TOOLING_GAP]: None branch-caused. CI is green and focused local unit tests pass.
• [RETROSPECTIVE]: Source discovery should become data-driven before Phase 2 ingestion-service work consumes tenant sources, but partial delivery must not use a magic close-target that hides deferred ACs.

🛂 Provenance Audit

• Internal Origin: Discussion #11623 → Epic #11624 → Phase 0/1 #11625 → #11658. This is native Neo Agent OS cloud-ingestion substrate, not an external framework import.

Findings: Pass.

🎯 Close-Target Audit

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

• Close-targets identified: Resolves #11658 in the PR body.
• For #11658: verified labels are enhancement, ai, architecture; not epic.
• Close-target completion check: fails. #11658 includes ACs for per-source path config externalization, useDefaultSources:false skip-default tests, custom source registration round-trip, byte-equivalence / sync-kb smoke evidence, and the PR body itself says one AC is deferred and #11658 remains open.

Findings: Close-target is syntactically valid and non-epic, but semantically wrong for a partial-resolution PR.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix: N/A formal ledger, but #11658 itself is the consumed contract for this phase.
• Implemented PR diff matches the Contract Ledger exactly (no drift): drift against #11658 ACs as listed in Required Actions.

Findings: Contract drift flagged against issue ACs: partial scope is valid, but the close-target and coverage claims must match the partial scope.

🪜 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 deferred / follow-up.
• Two-ceiling distinction: PR body frames the achieved evidence as L2 unit-test evidence.
• Evidence-class collapse check: fails for #11658 as close-target because not all #11658 ACs are covered or explicitly residualized in a way compatible with magic-close.

Findings: Evidence mismatch. The implementation may be a correct registry-foundation slice, but it is not a complete Resolves #11658 slice as currently described.

📜 Source-of-Authority Audit

N/A. The review cites public issue/PR/diff/test evidence; no private operator authority is used as a Required Action substitute.

📡 MCP-Tool-Description Budget Audit

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

🔌 Wire-Format Compatibility Audit

N/A. No JSON-RPC or A2A wire format changed.

🔗 Cross-Skill Integration Audit

• ai/mcp/server/knowledge-base/config.template.mjs changed, so I loaded pull-request/references/mcp-config-template-change-guide.md.
• Changed keys are listed in the PR body: useDefaultSources, useDefaultParsers, customSources, customParsers.
• Local config.mjs follow-up is not explicit. Existing clones can tolerate missing keys for defaults, but operators need explicit guidance for enabling cloud/tenant mode (useDefaultSources:false) and whether harness restart is required.
• The PR body says config.mjs + config.template.mjs changed, but only the template is committed. That is correct mechanically, but the body should state that live gitignored config.mjs files require optional local shape updates when operators want the new toggles.

Findings: Config-template sync guidance gap flagged.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request on PR #11659 at head f9b3ee227.
• Canonical Location: new spec is under test/playwright/unit/ai/services/knowledge-base/source/, matching the modified KB source surface.
• If a test file changed: ran the specific new test plus related source/regression specs.
• If code changed: verified related tests exist and ran them.

Findings: Local targeted tests pass:

npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/source/SourceRegistry.spec.mjs test/playwright/unit/ai/services/knowledge-base/source/AdrSource.spec.mjs test/playwright/unit/ai/services/knowledge-base/source/SkillSource.spec.mjs test/playwright/unit/ai/services/knowledge-base/DatabaseService.backup.spec.mjs

Result: 19 passed (993ms). Coverage gap remains for useDefaultSources:false and declarative customSources / customParsers config registration.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11659 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.

📋 Required Actions

To proceed with merging, please address the following:

• Fix close-target semantics. If this PR intentionally defers per-source path externalization and #11658 must remain open, change Resolves #11658 to a non-closing reference such as Related: #11658, or file/use a narrower sub-ticket for the registry-foundation slice and close that instead. Do not merge a PR whose body both magic-closes #11658 and says #11658 remains open.
• Add direct coverage for the new config-driven surfaces, or explicitly narrow the PR to exclude them. At minimum, test useDefaultSources:false skip-default behavior and the declarative customSources / customParsers registration path. Programmatic registerSource() tests are useful, but they are not equivalent to proving the config array path introduced in config.template.mjs and _export.mjs works.
• Tighten config-template and docs prose. The PR body should state live config.mjs follow-up / restart guidance per mcp-config-template-change-guide.md, and _export.mjs should not document customSources = [{className, sourceName, ...}] when the implemented/configured shape is {SourceClass, sourceName?}.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 84 - 16 points deducted because the registry direction is correct, but the partial-delivery close-target and unproven config-driven path weaken the phase contract.
• [CONTENT_COMPLETENESS]: 68 - 32 points deducted because the PR body overcloses #11658, clone-sync guidance for the config-template change is missing, and _export.mjs has a shape mismatch in its custom-source prose.
• [EXECUTION_QUALITY]: 78 - 22 points deducted because CI and 19 focused local tests pass, but the tests do not directly cover useDefaultSources:false or declarative custom registration.
• [PRODUCTIVITY]: 72 - 28 points deducted because the registry foundation lands useful progress, but the PR does not yet satisfy the close-target as declared.
• [IMPACT]: 78 - Major substrate value for cloud-native KB ingestion because Phase 2 can consume a registry instead of a hardcoded source list; not foundational enough to score higher while path externalization is deferred.
• [COMPLEXITY]: 66 - Moderate-high: new singleton, import-time registration side effects, config-template shape, DatabaseService source discovery, and test isolation around registry state.
• [EFFORT_PROFILE]: Heavy Lift - High strategic value with non-trivial integration risk across config, source discovery, and future tenant ingestion.

Measurement Payload

Cycle: 1 / cold-cache
Static wc -c: 78,102 (pr-review guide + full template + CI audit + measurement methodology)
Dynamic wc -c: 43,705 (PR JSON payload 9,027 + issue body 9,910 + diff 24,768)
Total wc -c: 121,807

Closing: the core registry idea is sound. The next cycle should be a focused metadata/coverage repair, not a redesign.