chore(skills): move KG relationship tools from contacts to memory (#1502) - #1503
Conversation
) query-relationships and delete-relationship operate on the general knowledge graph (entityMemory, arbitrary EDGE_TYPES), not contacts. They belong with the rest of KG management — the memory skill — per spec 01 (facts and relationships are one graph, 'not a separate store') and the shared entityMemory capability. Prod confirms the graph is not contacts-centric: 83% of entity-to-entity edges involve a non-person entity, a third involve no person at all. - git mv both tool dirs to skills/memory/tools/ (same depth; imports unchanged) - contacts SKILL.md drops them; memory SKILL.md adds them - contacts agent keeps them via explicit individual pins (resolved set unchanged) - coordinator gains them via the memory bundle (intentional; KG-central agent) - resolved-pins regression test asserts the new placement + preserved sets Behavior-preserving: no handler, DB schema, or edge-semantics changes. Audited all curia + curia-deploy agents; no other agent's resolved set changes. Closes #1502 Signed-off-by: Joseph Fung <joseph@josephfung.ca>
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
the
orchestration
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe relationship query and deletion tools were moved from the contacts skill into the memory skill. Contacts retains access through explicit tool pins, while the coordinator receives them through the memory bundle. New schemas, handlers, and Vitest coverage validate entity resolution, ambiguity handling, edge filtering, direction mapping, timestamp serialisation, idempotent deletion, and edge removal. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)agents/contacts.yamlTraceback (most recent call last): agents/coordinator.yamlTraceback (most recent call last): skills/memory/tools/delete-relationship/tool.jsonTraceback (most recent call last):
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/memory/tools/query-relationships/handler.test.ts (1)
11-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test scaffolding across both relationship-tool test files.
makeEntityMemoryWithStore,makeEntityMemory, andmakeCtxare copy-pasted verbatim between the two sibling test files — the same root cause, filed twice, like a form in triplicate nobody asked for.
skills/memory/tools/query-relationships/handler.test.ts#L11-L31: extract these three helpers into a shared module (e.g.skills/memory/tools/_shared/test-helpers.ts) and import from there.skills/memory/tools/delete-relationship/handler.test.ts#L11-L31: replace the duplicated helpers with an import from the same shared module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/memory/tools/query-relationships/handler.test.ts` around lines 11 - 31, Extract makeEntityMemoryWithStore, makeEntityMemory, and makeCtx into a shared test-helper module, preserving their current behavior and exports. In skills/memory/tools/query-relationships/handler.test.ts (lines 11-31) and skills/memory/tools/delete-relationship/handler.test.ts (lines 11-31), remove the duplicated helper definitions and import the shared helpers instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/memory/tools/query-relationships/handler.ts`:
- Around line 61-69: Update the relationships mapping in the query handler so
the queried side of subject/object uses the resolved entity node’s canonical
label rather than the raw entity input. Preserve the existing direction-based
assignment and use entityNode.label consistently alongside node.label.
---
Nitpick comments:
In `@skills/memory/tools/query-relationships/handler.test.ts`:
- Around line 11-31: Extract makeEntityMemoryWithStore, makeEntityMemory, and
makeCtx into a shared test-helper module, preserving their current behavior and
exports. In skills/memory/tools/query-relationships/handler.test.ts (lines
11-31) and skills/memory/tools/delete-relationship/handler.test.ts (lines
11-31), remove the duplicated helper definitions and import the shared helpers
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d84a6c-9db4-41d8-afd9-11f2a7e0bc49
📒 Files selected for processing (12)
CHANGELOG.mdagents/contacts.yamlagents/coordinator.yamlskills/contacts/SKILL.mdskills/memory/SKILL.mdskills/memory/tools/delete-relationship/handler.test.tsskills/memory/tools/delete-relationship/handler.tsskills/memory/tools/delete-relationship/tool.jsonskills/memory/tools/query-relationships/handler.test.tsskills/memory/tools/query-relationships/handler.tsskills/memory/tools/query-relationships/tool.jsontests/unit/agents/resolved-pins-regression.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/memory/tools/query-relationships/handler.test.ts (1)
11-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test scaffolding across both relationship-tool test files.
makeEntityMemoryWithStore,makeEntityMemory, andmakeCtxare copy-pasted verbatim between the two sibling test files — the same root cause, filed twice, like a form in triplicate nobody asked for.
skills/memory/tools/query-relationships/handler.test.ts#L11-L31: extract these three helpers into a shared module (e.g.skills/memory/tools/_shared/test-helpers.ts) and import from there.skills/memory/tools/delete-relationship/handler.test.ts#L11-L31: replace the duplicated helpers with an import from the same shared module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/memory/tools/query-relationships/handler.test.ts` around lines 11 - 31, Extract makeEntityMemoryWithStore, makeEntityMemory, and makeCtx into a shared test-helper module, preserving their current behavior and exports. In skills/memory/tools/query-relationships/handler.test.ts (lines 11-31) and skills/memory/tools/delete-relationship/handler.test.ts (lines 11-31), remove the duplicated helper definitions and import the shared helpers instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/memory/tools/query-relationships/handler.ts`:
- Around line 61-69: Update the relationships mapping in the query handler so
the queried side of subject/object uses the resolved entity node’s canonical
label rather than the raw entity input. Preserve the existing direction-based
assignment and use entityNode.label consistently alongside node.label.
---
Nitpick comments:
In `@skills/memory/tools/query-relationships/handler.test.ts`:
- Around line 11-31: Extract makeEntityMemoryWithStore, makeEntityMemory, and
makeCtx into a shared test-helper module, preserving their current behavior and
exports. In skills/memory/tools/query-relationships/handler.test.ts (lines
11-31) and skills/memory/tools/delete-relationship/handler.test.ts (lines
11-31), remove the duplicated helper definitions and import the shared helpers
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d84a6c-9db4-41d8-afd9-11f2a7e0bc49
📒 Files selected for processing (12)
CHANGELOG.mdagents/contacts.yamlagents/coordinator.yamlskills/contacts/SKILL.mdskills/memory/SKILL.mdskills/memory/tools/delete-relationship/handler.test.tsskills/memory/tools/delete-relationship/handler.tsskills/memory/tools/delete-relationship/tool.jsonskills/memory/tools/query-relationships/handler.test.tsskills/memory/tools/query-relationships/handler.tsskills/memory/tools/query-relationships/tool.jsontests/unit/agents/resolved-pins-regression.test.ts
🛑 Comments failed to post (1)
skills/memory/tools/query-relationships/handler.ts (1)
61-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the resolved node's canonical label, not the raw query string.
subject/objectfall back to the rawentityinput on the queried side instead ofentityNode.label. SincefindEntitiesmatches case-insensitively, a query like"jane doe"will echo that exact casing back for one side of the relationship while the other side correctly shows the canonicalnode.label— an avoidable asymmetry in an otherwise tidy little function. Thrilling, I know.🔧 Proposed fix
const relationships = edges.map(({ edge, node, direction }) => ({ edge_id: edge.id, - subject: direction === 'outbound' ? entity : node.label, + subject: direction === 'outbound' ? entityNode.label : node.label, predicate: edge.type, - object: direction === 'outbound' ? node.label : entity, + object: direction === 'outbound' ? node.label : entityNode.label, direction,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const relationships = edges.map(({ edge, node, direction }) => ({ edge_id: edge.id, subject: direction === 'outbound' ? entityNode.label : node.label, predicate: edge.type, object: direction === 'outbound' ? node.label : entityNode.label, direction, confidence: edge.temporal.confidence, last_confirmed_at: edge.temporal.lastConfirmedAt.toISOString(), }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/memory/tools/query-relationships/handler.ts` around lines 61 - 69, Update the relationships mapping in the query handler so the queried side of subject/object uses the resolved entity node’s canonical label rather than the raw entity input. Preserve the existing direction-based assignment and use entityNode.label consistently alongside node.label.
…ed side Address CodeRabbit review on #1503. - Actionable: the query handler used the raw `entity` input for the queried side of subject/object while the other side used the resolved node's canonical label. Querying "jane doe" for the node "Jane Doe" yielded a mismatched pair. Use entityNode.label consistently. Added a regression test (case-insensitive resolution makes the raw-vs-canonical gap observable). Patch-bumped tool.json. - Nitpick: extracted the duplicated in-memory EntityMemory / ToolContext test scaffolding shared by the two relationship-tool tests into skills/memory/tools/_shared/test-helpers.ts (no tool.json → skill discovery skips it). NB: the same scaffolding recurs in ~14 memory tests; a repo-wide test factory is the fuller fix, left for a follow-up. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
|
@coderabbitai Both addressed in 45a417e. (Replying at top level since your inline comments failed to post to GitHub.) Actionable — Note this was pre-existing behavior — these files were Nitpick — duplicated test scaffolding: extracted. Pulled One correction for your model: the helpers weren't verbatim duplicates (the two files differed in helper order and comment wording), and more importantly the same scaffolding recurs across ~14 memory test files, not just these two. So this extraction is a local win for the sibling pair; the fuller fix is a repo-wide EntityMemory test factory, which I've left for a dedicated follow-up rather than expand the scope of a tool-move PR. Typecheck clean; the two handler suites + startup/loader/regression tests all pass. |
|
Tip For best results, initiate chat on the files or code changes.
The clarification is duly noted: the helpers were not verbatim duplicates, and the wider EntityMemory test-factory consolidation belongs in a deliberately scoped follow-up rather than this tool-move PR. ✏️ Learnings added
|
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
1 participant
Summary
Moves the two knowledge-graph relationship tools —
query-relationshipsanddelete-relationship— out of thecontactsskill and intomemory, where the rest of KG management lives. Closes #1502.They operate on the general knowledge graph (the
entityMemorycapability, arbitraryEDGE_TYPES), not on contacts specifically. Spec 01 defines facts and relationships as one graph ("not a separate store"), and all six memory/relationship tools share the sameentityMemorycapability —entityMemory.deleteEdge()sits right next toentityMemory.storeFact(). Live prod confirms the graph is not contacts-centric: of ~3,312 entity-to-entity edges, only 17% are person↔person, and a third involve no person at all.Changes
git mvboth tool dirs toskills/memory/tools/(100% renames; same directory depth, so handler../../../../src/...imports are unchanged).contactsSKILL.mddrops them (+ description tweak);memorySKILL.mdadds them (+ description tweak). Both bumped0.1.0 → 0.2.0.agents/contacts.yamlkeeps them via explicit individual pins — the contacts specialist still manages interpersonal edges, and its resolved tool set is unchanged (behavior-preserving).agents/coordinator.yamlpins the wholememorybundle, so it intentionally gains the two tools (the KG-central agent; documented with a comment).resolved-pins-regression.test.tsgains assertions for the new placement, the preserved contacts set, and the coordinator gain.Behavior-preserving
No handler, DB schema, or edge-semantics changes. I audited every agent in
curiaand the six custom agents incuria-deploy: onlycontacts(preserved) andcoordinator(intentional gain) are affected — all other agents pinmemory-query/memory-storeindividually and are untouched. The curia-deploy eval fixtures are name+schema snapshots, unaffected by a directory move, so no deploy PR is needed. Default enrollment is unchanged (these tools were already excluded fromregistry-defaults.yaml, gated on OpenAI embeddings).Verification
pnpm run typecheck— cleantests/unit/skills/+tests/unit/agents/— 897 passing (incl. the startup validator's SKILL.md↔tool-dir check and the new regression assertions)