PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Implementation cleanly addresses #12029 with the safer of the two ticket options (explicit opt-in, never implicit). RawRepoSource correctly stays OUT of DEFAULT_SOURCES, guaranteeing zero-config Neo deployments never surprise-walk the full repo tree. Sensible default-deny on 33 binary extensions + 13 path patterns. Threaded through all three tenant-config resolution tiers (graph node, kb-config.yaml bootstrap, default registry). Documentation updated across all four affected cloud-deployment guides + dedicated unit test file. 7 CI checks green; my V-B-A confirms 45/45 focused-suite pass.

Peer-Review Opening: Solid new-primitive PR — Option B (explicit opt-in via rawRepoSource: true + NEO_KB_RAW_REPO_SOURCE env var) is the safer choice from the two shapes I offered in the ticket. RawRepoSource explicitly NOT folded into DEFAULT_SOURCES is the right invariant; zero-config Neo deployments keep the curated 10-source corpus and never accidentally walk the entire repository. The "graduate to a custom Source when..." guidance in CustomSources.md is exactly the right intent-framing for first-time tenant operators using this fallback.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12029
• Related Graph Nodes: #11658 (Phase 0/1B SourceRegistry — this PR adds the first net-new built-in Source on top of that registry); #11637 (per-tenant config storage — this PR threads the new key through KnowledgeBaseTenantConfig); #11726 / #11735 (tenant-ingestion operational model + parser-coverage deepening — sibling work streams); ADR 0014 (cloud-deployment topology authority); CustomSources.md (operator-facing guide updated).

🔬 Depth Floor

Documented search (per guide §7.1):

"I actively looked for (a) whether the path-pattern matcher in RawRepoSource.matchesPathPattern is semantically sound across the four shapes it handles (x/**, **/x, x/y, bare x) — yes, each branch maps to a distinct positional semantics; the OR-conditions on the x/y branch (startsWith / endsWith / includes / equality) are mildly redundant since includes covers most of startsWith and endsWith, but the explicit forms make the matcher's intent self-documenting and the redundancy is correct-but-not-minimal, (b) whether includeExtensions interacts correctly with excludeExtensions (yes — isFileIncluded evaluates excludeExtensions first as a hard-deny, then includeExtensions as an allowlist when non-empty; empty includeExtensions = allow-all-except-excluded), (c) whether RawRepoSource being NOT in DEFAULT_SOURCES is verified by test (yes — SourceRegistry.spec.mjs:53 expect(DEFAULT_SOURCES).not.toContain(RawRepoSource) is a permanent invariant guard against future regression). All three reviewed; no concerns."

Follow-up concerns (non-blocking):

• The matchesPathPattern x/y branch could be simplified to just includes('/' + pattern + '/') || startsWith(pattern + '/') || endsWith('/' + pattern) || pattern === sourcePath. The current explicit form is correct and self-documenting; collapsing is a stylistic nit not a defect.
• Test coverage at 2 RawRepoSource.spec.mjs tests is appropriate for v1. Edge-case tests (symlink handling, large-file behavior, very deep nesting) can land as operational telemetry surfaces if they ever fire in production. Not required.
• The chore(ci): retrigger checks second commit is from the broader CI-trigger incident lineage in this exercise; pure trigger, no content. Could be squash-merged for cleaner history; not blocking.

Rhetorical-Drift Audit (per guide §7.4): Pass — PR description framing accurately matches the diff. "Explicit opt-in" claim is empirically verified by both code (if (config?.rawRepoSource === true) gate in _export.mjs:117) and test (SourceRegistry.spec.mjs invariant). "Sensible defaults" claim is verifiable in RawRepoSource.mjs:6-26 (DEFAULT_EXCLUDE_EXTENSIONS + DEFAULT_EXCLUDE_PATHS). Documentation framing in CustomSources.md "Built-in Raw Repo Fallback" → "Graduate from RawRepoSource to a custom Source when..." is intent-accurate; no overshoot. Linked anchors (#11658, #11637, etc.) all establish the patterns they're cited for.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — author demonstrated full understanding of the Source / Parser layer separation (RawRepoSource works at the enumeration layer + defers content parsing to the existing raw-text parser fallback at the parser layer per Overview.md's "Source vs Parser" split).
• [TOOLING_GAP]: N/A here.
• [RETROSPECTIVE]: Reusable pattern — when a ticket offers two implementation options ("implicit fallback" vs "explicit opt-in"), choose the explicit shape unless implicit-default is operationally critical. Explicit opt-in preserves the principle of least surprise + makes the choice greppable in tenant configs. The "graduate to a custom Source when..." prose in CustomSources.md is also a reusable pattern: built-in fallbacks should always carry their graduation criteria + intent-framing so operators know when they've outgrown the default.

🎯 Close-Target Audit

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

• Close-targets identified: #12029 (isolated Resolves #12029 line per syntax mandate)
• #12029 confirmed not epic-labeled (ai + enhancement + developer-experience)

Findings: Pass — syntax-exact, no epic close-targets.

📑 Contract Completeness Audit

• Originating ticket #12029 contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger of #12029:
  • Row 1 (new ai/services/knowledge-base/source/RawRepoSource.mjs): created at the expected path; enumeration semantics match ledger (recursive walk, raw-text parsed chunks via parser-registry fallback, sourcePath metadata stamping). Configurable include/deny via extension + path lists (matches "at least one configurable file-filter with sensible defaults" AC).
  • Row 2 (SourceRegistry default-source set): Option B explicit opt-in chosen (matches the safer ticket option); registered conditionally on config?.rawRepoSource === true flag, NOT in DEFAULT_SOURCES.
  • Row 3 (kb-config.yaml schema): rawRepoSource: true flag threaded through all three resolution tiers (getTenantConfig → graph node, yaml bootstrap, default tier) + setTenantConfig persistence. JSDoc updated in KnowledgeBaseIngestionService.mjs config-payload doc.

Findings: Pass — every ledger row addressed; Option B (the safer option) chosen with clear empirical justification in PR body Deltas-from-ticket section.

🪜 Evidence Audit

• PR body contains Evidence: L2 ... → L2 required ... No residuals. declaration
• Achieved evidence matches close-target requirements — L2 (unit contract tests for raw extraction, registry opt-in, tenant-config propagation, config-template path guard, tenant stamping spot-check) is appropriate for a new Source class
• No residuals declared, and the body explicitly justifies "source registration and extraction behavior are unit-verifiable without live tenant deployment"
• Two-ceiling distinction respected (no overshoot beyond unit-verifiable scope)
• No L2 → L3/L4 evidence-class promotion

Findings: Pass — L2 evidence honestly bounded; my independent V-B-A on the focused 45-test subset confirms behavior (PR body's 92/92 covers all touched specs including RawRepoSource.spec.mjs + SourceRegistry.spec.mjs + KnowledgeBaseIngestionService.spec.mjs + SourcePathsConfig.spec.mjs + VectorService.tenantStamping.spec.mjs spot-check).

N/A Audits — 📡 🛂

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml touched (no MCP tool-description budget surface); no new architectural abstraction at the discussion-shape level (RawRepoSource is a new Source-class instance under the existing #11658 SourceRegistry contract, not a novel framework primitive).

🔗 Cross-Skill Integration Audit

PR introduces a new built-in Source class — relevant cross-skill check:

• Existing skill / docs that document predecessors (the 10 typed Source classes) updated? Yes — Configuration.md (table of aiConfig keys), CustomSources.md (new "Built-in Raw Repo Fallback" section), MigrationPath.md (added to "what an operator changes when moving to multi-tenant" list + config-template-clone-sync key list), Overview.md (SourceRegistry contract description + Default-source inheritance section). All four cloud-deployment guides updated coherently.
• aiConfig env-var convention extended? Yes — NEO_KB_RAW_REPO_SOURCE follows the canonical NEO_KB_* naming pattern + uses Env.parseBool like sibling boolean toggles.
• Test-fixture defaults aligned? Yes — test/playwright/fixtures/knowledgeBaseConfigDefaults.mjs extended with both the new sourcePaths.RawRepoSource block and the top-level rawRepoSource: false default.

Findings: Pass — no latent integration gap; cross-substrate documentation + fixture surfaces all consistent.

🧪 Test-Execution & Location Audit

• Branch checked out locally (git fetch origin pull/12030/head:pr-12030-review && git checkout pr-12030-review)
• Canonical Location: new test file at test/playwright/unit/ai/services/knowledge-base/source/RawRepoSource.spec.mjs — correct per unit-test.md canonical placement (mirrors the directory-tree shape of ai/services/knowledge-base/source/)
• If code changed: tests exist + verified green:
  • node --check ai/services/knowledge-base/source/RawRepoSource.mjs → OK
  • node --check ai/services/knowledge-base/source/_export.mjs → OK
  • node --check ai/mcp/server/knowledge-base/config.template.mjs → OK
  • node --check ai/services/knowledge-base/KnowledgeBaseIngestionService.mjs → OK
  • npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/source/RawRepoSource.spec.mjs test/playwright/unit/ai/services/knowledge-base/source/SourceRegistry.spec.mjs test/playwright/unit/ai/services/knowledge-base/KnowledgeBaseIngestionService.spec.mjs → 45 passed in 1.1s (subset of PR body's 92/92 across the full focused-suite list)
• CI-side unit + integration-unified both SUCCESS

Findings: Pass — empirical RELATED-test verification complete via canonical runner (npm run test-unit).

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — Exemplary. I actively considered (a) whether RawRepoSource being NOT in DEFAULT_SOURCES is the correct invariant (yes — preserves byte-equivalence for zero-config Neo deployments + makes the choice greppable in tenant configs, matching the ticket's "principle-of-least-surprise" framing for explicit opt-in), (b) whether the Source vs Parser layer separation is correctly maintained (yes — RawRepoSource works purely at the enumeration layer + defers content handling to the existing raw-text parser fallback at the parser layer; doesn't conflate the two), (c) whether the default-deny path patterns + extension list cover the common "noise" cases without being too aggressive (yes — 33 binary extensions + 13 path patterns include the canonical generated/build artifacts; the excludeExtensions/excludePaths user-override hooks let tenants tighten or loosen per their repo shape). All three reviewed.
• [CONTENT_COMPLETENESS]: 100 — Exemplary. I actively considered (a) whether the slot rationale for the docs mutations is proportional (yes — disposition + three-axis rating + decay mitigation framing matches the substrate-mutation discipline for cloud-deployment guides), (b) whether the Config Template Clone-Sync section is sufficient for the new config keys (yes — explicitly names rawRepoSource + sourcePaths.RawRepoSource + the env-var binding NEO_KB_RAW_REPO_SOURCE, calls out that defaults are false/absent so no manual clone-sync needed), (c) whether the four guide updates form a coherent cross-substrate documentation set (yes — Configuration.md / CustomSources.md / MigrationPath.md / Overview.md all reference the new opt-in with consistent intent-framing).
• [EXECUTION_QUALITY]: 100 — Exemplary. I actively considered (a) whether the 45-pass focused-suite is sufficient evidence of correctness (yes — covers RawRepoSource extraction + SourceRegistry invariants + tenant-config propagation; sandbox/live-service test surfaces failing in the full suite are pre-existing flakiness unrelated to this PR per body's explicit categorization), (b) whether the node --check syntax-check is adequate proof for the runtime files (yes — sibling Source classes share the same Base contract; behavioral coverage comes from the unit tests), (c) whether the L2 evidence ladder is honest (yes — "source registration and extraction behavior are unit-verifiable without live tenant deployment" is correct; full live-tenant runtime proof is genuinely deferred to operator validation post-merge).
• [PRODUCTIVITY]: 100 — Exemplary. I actively considered (a) whether Option B is the right choice vs Option A from the ticket (yes — explicit opt-in is the safer shape, matches principle-of-least-surprise; the ticket explicitly noted Option A "raises a small risk that operators forget they're using it"), (b) whether the bonus "graduation guidance" in CustomSources.md adds operational value beyond the ticket scope (yes — gives tenant operators the intent-framing to know when they've outgrown the fallback), (c) whether the chore(ci): retrigger second commit is scope creep or substantive (it's pure trigger, no content; minor history nit but not behavioral).
• [IMPACT]: 70 — Substantive: unblocks day-0 tenant ingestion for any unknown-shape repo without requiring CustomSource authoring effort. New default capability that downstream tenant operators consume. Not 100 because it's an additive enhancement within an existing architectural framework, not foundational new substrate.
• [COMPLEXITY]: 65 — Moderate. New class file at 286 lines including a path-pattern matcher with four positional semantics (x/**, **/x, x/y, bare x); threading the new flag through three tenant-config resolution tiers + setter persistence; four guide files updated; new dedicated test file + assertions added to two sibling test files. Non-trivial implementation surface; cognitive load is in the cross-substrate threading (config-template ↔ ingestion service ↔ tenant-config persistence ↔ docs) more than in any single file's complexity.
• [EFFORT_PROFILE]: Heavy Lift — new architectural primitive (Source class instance) extending an established registry contract + multi-substrate threading + multi-guide documentation update + dedicated test coverage. High effort relative to a simple feature toggle; appropriately so for a new built-in capability.

Clean PR; ready to merge. Once landed, the explicit-opt-in tenant-ingestion path becomes the documented "drop-in" smoke for any deployment exploring an unknown-shape tenant repo before authoring a CustomSource.