Frontmatter
| title | feat(kb): add PullRequestSource for PR conversation embeddings (#10057) |
| author | tobiu |
| state | Merged |
| createdAt | Apr 18, 2026, 8:15 PM |
| updatedAt | Apr 18, 2026, 8:44 PM |
| closedAt | Apr 18, 2026, 8:44 PM |
| mergedAt | Apr 18, 2026, 8:44 PM |
| branches | dev ← agent/10057-pull-request-source |
| url | https://github.com/neomjs/neo/pull/10066 |
Merged

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.mjsverbatim rather than implement the per-section chunking the ticket prescribed, because downstream chunking happens inDatabaseServiceand 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 ofDiscussionSource. Final test conforms to the unit-test skill's Critical Rules (importcore/_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 —@summaryon 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 oldtest/playwright/mcp/knowledge-base/path, reports "3 wiring/structure tests" when there are now 5 singleton tests, and citesplaywright.config.mjsinstead ofplaywright.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-kbwasn'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. Futureask_knowledge_basecalls 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 in144326a4-c1e8-4eee-80e4-b984f3c79ddcduring PR #10053 review)
🧠 Graph Ingestion Notes
[KB_GAP]: The unit-test skill mandates importingcore/_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.mjsis gitignored and generated bynode buildScripts/ai/initServerConfigs.mjs(called vianpm 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 theChromaManager.spec.mjsfailure noted in #10065's review). Worth considering: have theplaywright.config.unit.mjsglobal setup auto-invokeinitServerConfigsso test runs are self-bootstrapping. (2) The legacytest/playwright/mcp/tree is an active trap — its presence alongside the canonicaltest/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) Usesrc/testsymmetry as the decisive path signal, not closest-path-match. The canonical test for code atai/mcp/server/<X>/is attest/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 callingsetup(), isn't settingunitTestMode, isn't importingcore/_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
devpost-merge, verifyquery_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 legacytest/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.
Summary
Adds
PullRequestSourceto 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
ai/mcp/server/knowledge-base/source/PullRequestSource.mjs(~40 LOC, direct mirror ofDiscussionSource.mjs)ai/mcp/server/knowledge-base/services/DatabaseService.mjs— import + register in thesourcesarray (2 lines)test/playwright/unit/ai/mcp/server/knowledge-base/PullRequestSource.spec.mjs— 5 tests exercising the real singleton against a mock filesystemDesign 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/TicketSourcepattern is one chunk per markdown file — downstream chunking happens inDatabaseService. Deviating would have fragmented the taxonomy and brokentype=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 forquery_documents(query='...', type='pull').Ticket location vs actual registration
Ticket said "register in
config.mjs" — actual wiring lives inDatabaseService.mjs:127where the other six sources are listed. Followed actual architecture.Test style: real singleton, not static regex
The spec boots the framework via
setup(), importscore/_export.mjsper unit-test skill Rule #1, dynamic-importsaiConfig+PullRequestSourceso the test can stubaiConfig.neoRootDirto a tmp mock filesystem, captureswriteStream.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
Tests cover: (1) singleton identity — correct
className,extract()method; (2)extract()emits one chunk per.mdfile with correcttype: 'pull'/kind: 'pull'metadata; (3) non-.mdfiles are ignored; (4) missing pulls directory returns0and writes nothing; (5)DatabaseService.mjsimports + registersPullRequestSource.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
update_issue_relationship)Avoided Traps
TicketSourceto scrapeResolves #Nbacklinks. That would duplicate content and blur thetypetaxonomy.TicketSource.mjs:14-17JSDoc (says.github/ISSUE_ARCHIVE, code readsresources/content/issues). Noted in intake comment for follow-up.Commit History
feat(kb): add PullRequestSource for PR conversation embeddingstest(kb): move spec to canonicaltest/playwright/unit/ai/mcp/server/path (was in legacytest/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 session144326a4-c1e8-4eee-80e4-b984f3c79ddcduring PR #10053 review.Resolves #10057