LearnNewsExamplesServices
Frontmatter
titlefeat(kb): add PullRequestSource for PR conversation embeddings (#10057)
authortobiu
stateMerged
createdAtApr 18, 2026, 8:15 PM
updatedAtApr 18, 2026, 8:44 PM
closedAtApr 18, 2026, 8:44 PM
mergedAtApr 18, 2026, 8:44 PM
branchesdevagent/10057-pull-request-source
urlhttps://github.com/neomjs/neo/pull/10066
Merged
tobiu
tobiu commented on Apr 18, 2026, 8:15 PM

Summary

Adds PullRequestSource to the Knowledge Base, closing the RAG asymmetry where tickets capture the problem but PRs capture the solution (merge rationale, reviewer corrections, scope-creep warnings, follow-up links). After this lands, ask_knowledge_base(query='how was X resolved?') can surface both the intent (ticket) and the reasoning recorded during execution (PR).

Scope

  • New: ai/mcp/server/knowledge-base/source/PullRequestSource.mjs (~40 LOC, direct mirror of DiscussionSource.mjs)
  • Modified: ai/mcp/server/knowledge-base/services/DatabaseService.mjs — import + register in the sources array (2 lines)
  • New: test/playwright/unit/ai/mcp/server/knowledge-base/PullRequestSource.spec.mjs — 5 tests exercising the real singleton against a mock filesystem

Design Decisions

Followed DiscussionSource exactly; did NOT implement per-section chunking

The ticket prescribed "chunk the PR body + conversation into embeddable segments", but the actual DiscussionSource / TicketSource pattern is one chunk per markdown file — downstream chunking happens in DatabaseService. Deviating would have fragmented the taxonomy and broken type= filtering consistency. Documented this hypothesis-vs-root-cause correction in #10057's intake comment before starting.

Chunk shape

{ type: 'pull', kind: 'pull', name: 'pr-10065', content, source: '<abs path>', hash }

type: 'pull' matches the ticket's Acceptance Criteria for query_documents(query='...', type='pull').

Ticket location vs actual registration

Ticket said "register in config.mjs" — actual wiring lives in DatabaseService.mjs:127 where the other six sources are listed. Followed actual architecture.

Test style: real singleton, not static regex

The spec boots the framework via setup(), imports core/_export.mjs per unit-test skill Rule #1, dynamic-imports aiConfig + PullRequestSource so the test can stub aiConfig.neoRootDir to a tmp mock filesystem, captures writeStream.write() output, and asserts the exact chunk shape — type, kind, name, content, source, hash. A static-regex version was rejected during review as a structure check, not a unit test.

Verification

$ node buildScripts/ai/initServerConfigs.mjs   # bootstraps gitignored config.mjs files
$ npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/server/knowledge-base/
  5 passed (560ms)

Tests cover: (1) singleton identity — correct className, extract() method; (2) extract() emits one chunk per .md file with correct type: 'pull' / kind: 'pull' metadata; (3) non-.md files are ignored; (4) missing pulls directory returns 0 and writes nothing; (5) DatabaseService.mjs imports + registers PullRequestSource.

End-to-end embedding validation (ai:sync-kb) was not run in this worktree — requires ChromaDB + API keys. Post-merge, the next KB sync will pick up PRs automatically.

Graph Wiring

  • Parent: #9999 (set via update_issue_relationship)
  • Related (not parent): #10030 — Concept Ontology is a different architectural layer (deterministic JSONL graph). Complementary: concepts tell you what; PR embeddings surface how things got done.

Avoided Traps

  • Not resurrecting the abandoned Macro KB approach (#10014, closed for semantic dilution). This is conventional chunked source — fundamentally different.
  • Not extending TicketSource to scrape Resolves #N backlinks. That would duplicate content and blur the type taxonomy.
  • Not bundling a drive-by fix for the stale TicketSource.mjs:14-17 JSDoc (says .github/ISSUE_ARCHIVE, code reads resources/content/issues). Noted in intake comment for follow-up.

Commit History

  • feat(kb): add PullRequestSource for PR conversation embeddings
  • test(kb): move spec to canonical test/playwright/unit/ai/mcp/server/ path (was in legacy test/playwright/mcp/)
  • test(kb): exercise PullRequestSource singleton against mock fs (replaced static-regex checks)

Origin Session ID

97343e6e-9d1c-4170-b3a5-6e58d41511b6 (this session). The ticket itself originated in session 144326a4-c1e8-4eee-80e4-b984f3c79ddc during PR #10053 review.

Resolves #10057

tobiu
tobiu commented on Apr 18, 2026, 8:42 PM

Input from Claude Opus 4.7 (Claude Code):

❋ # PR Review Summary

Status: Comment (Self-Review)

Self-Review Opening: Self-review of #10057. I chose to mirror DiscussionSource.mjs verbatim rather than implement the per-section chunking the ticket prescribed, because downstream chunking happens in DatabaseService and deviating would have fragmented the type taxonomy. The more interesting thread is how the test got there: two course-corrections (location + style) landed during this session, documented honestly below.


📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 90 — Source provider is a byte-for-byte pattern clone of DiscussionSource. Final test conforms to the unit-test skill's Critical Rules (import core/_export.mjs, unitTestMode: true, mock-fs isolation, afterAll cleanup). Wouldn't have hit 90 without tobiu's two corrections; I landed it lower and iterated up.
  • [CONTENT_COMPLETENESS]: 78 — @summary on the new class explains why (signal asymmetry) not just what. Intake comment on #10057 preserves the hypothesis-vs-root-cause reasoning. Deduction: the PR body is stale — it references the old test/playwright/mcp/knowledge-base/ path, reports "3 wiring/structure tests" when there are now 5 singleton tests, and cites playwright.config.mjs instead of playwright.config.unit.mjs. I'm fixing this as part of this review.
  • [EXECUTION_QUALITY]: 82 — 5/5 green against the real singleton. Final spec is elegant; first two versions were not. No regression risk — additive source provider, no changes to query surface. Runtime verification still missing end-to-end (ai:sync-kb wasn't executed — needs ChromaDB + API keys).
  • [PRODUCTIVITY]: 95 — Ticket goal achieved: #10057 closed.
  • [IMPACT]: 65 — Improves RAG coverage for ~283 PR files, but doesn't unblock a host like #10064/#10065 did. Future ask_knowledge_base calls querying how something was built will benefit.
  • [COMPLEXITY]: 30 — ~210 LOC total across three files.
  • [EFFORT_PROFILE]: Quick Win — high ROI, low code complexity, simple additive wiring.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #10057
  • Related Graph Nodes: Parent #9999 (set via update_issue_relationship). Conceptually related to #10030 (Concept Ontology — complementary layer, not parent). Avoided the abandoned #10014 Macro KB approach.
  • Origin Session ID: 97343e6e-9d1c-4170-b3a5-6e58d41511b6 (ticket originated in 144326a4-c1e8-4eee-80e4-b984f3c79ddc during PR #10053 review)

🧠 Graph Ingestion Notes

  • [KB_GAP]: The unit-test skill mandates importing core/_export.mjs (Critical Rule #1), but nothing enforces it — silently-skipped imports still pass Playwright. A static analyzer or ESLint rule would catch tests that miss the augmentation and warn agents early that they're probably writing a structure-check, not a unit test.
  • [TOOLING_GAP]: (1) ai/mcp/server/*/config.mjs is gitignored and generated by node buildScripts/ai/initServerConfigs.mjs (called via npm run prepare). Fresh worktrees fail any test that imports these configs until this is run. Second PR this session where this friction cost time (first was the ChromaManager.spec.mjs failure noted in #10065's review). Worth considering: have the playwright.config.unit.mjs global setup auto-invoke initServerConfigs so test runs are self-bootstrapping. (2) The legacy test/playwright/mcp/ tree is an active trap — its presence alongside the canonical test/playwright/unit/ai/mcp/server/ tree causes agents (me, in this PR) to follow the wrong precedent. Either migrate the remaining files into the unit tree or add a deprecation README.
  • [RETROSPECTIVE]: Two honest lessons from my own process. (a) Use src/test symmetry as the decisive path signal, not closest-path-match. The canonical test for code at ai/mcp/server/<X>/ is at test/playwright/unit/ai/mcp/server/<X>/ — mirror of the source tree. Whenever two test-tree precedents exist, pick the one co-located with the source hierarchy. (b) The unit-test skill's Critical Rules are a canary. If my test isn't calling setup(), isn't setting unitTestMode, isn't importing core/_export.mjs, and isn't exercising real instances — I'm probably not writing a unit test, I'm writing a linter rule. Treat the rules as a filter: if they don't apply, reconsider the test.

📋 Required Actions

  • Update the PR body to reflect the final state (test path, count, config file). I'll do this immediately after posting this review.
  • Human QA — once the KB sync runs against dev post-merge, verify query_documents(query='grid refactoring', type='pull') returns relevant PR chunks (the acceptance criterion from #10057).
  • (Optional follow-up) File a ticket for the two [TOOLING_GAP] items — config.mjs bootstrap automation, and the legacy test/playwright/mcp/ tree deprecation. Both are real repo friction, not specific to this PR.

No block-level changes to the code itself — the final state is clean.

Handoff: awaiting human merge per pull-request-workflow.md §5 (Human-in-the-Loop). I will not self-merge.