diff --git a/CHANGELOG.md b/CHANGELOG.md index 9db52db7b..b50edb266 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ bus event types) are noted explicitly even in the `0.x` range. - **Tools vs skills vocabulary** — atoms are **tools** (`tool.json`, `ToolRegistry`; audit readers dual-match legacy `skill.*`); collections are **skills**. (#1485, #1489; ADR-031) - **Polymorphic pins** — a pin resolves a skill, a single tool, or an MCP-projected skill (per-tool `action_risk` preserved); `enable_task_management` retired for pinning `tasks` + `documents`; `tasks` gains `plan`/`checkpoint`, `memory` gains `decay-warnings-list`. (#1489, #1494; ADR-032) - **`approve-grant-recommendation`** — `action_risk` raised low→critical, matching direct permission grants. (#1499) +- **KG relationship tools** — `query-relationships`/`delete-relationship` moved from the `contacts` skill to `memory`. (#1502) ### Fixed diff --git a/agents/contacts.yaml b/agents/contacts.yaml index 862652421..901801757 100644 --- a/agents/contacts.yaml +++ b/agents/contacts.yaml @@ -1,5 +1,5 @@ name: contacts -version: "0.11.0" +version: "0.12.0" role: specialist description: > Contact domain specialist — briefings, CRUD, deduplication, relationship management, @@ -311,6 +311,11 @@ pinned_skills: # Memory — query/store only (not memory-confirm / decay-warnings-list) - memory-query - memory-store + # KG relationship tools — moved to the `memory` skill (#1502); contacts still + # manages interpersonal edges (spouse, reports_to), so pin them individually here + # rather than pulling the whole memory bundle. + - query-relationships + - delete-relationship # Calendar — read-only, used for meeting history enrichment in briefings only. # Calendar specialist owns the domain; contacts keeps this read-only pin for briefing enrichment. - calendar-list-events diff --git a/agents/coordinator.yaml b/agents/coordinator.yaml index a59110880..2b7a35f52 100644 --- a/agents/coordinator.yaml +++ b/agents/coordinator.yaml @@ -674,6 +674,8 @@ pinned_skills: - executive-profile - email - context-bridge + # memory bundle now also carries the KG relationship tools (query/delete edges, + # #1502); the coordinator intentionally gains them as the KG-central agent. - memory - scheduler - autonomy diff --git a/skills/contacts/SKILL.md b/skills/contacts/SKILL.md index 43e800cff..648d3ee2e 100644 --- a/skills/contacts/SKILL.md +++ b/skills/contacts/SKILL.md @@ -1,8 +1,8 @@ --- name: contacts description: > - Contact identity, lifecycle, grant recommendations, and relationship graph tools. Mixed action_risk (lookup=none … grant/revoke=critical) preserved per tool. -version: "0.1.0" + Contact identity, lifecycle, and grant recommendations. Mixed action_risk (lookup=none … grant/revoke=critical) preserved per tool. +version: "0.2.0" tools: - contact-create - contact-lookup @@ -23,6 +23,4 @@ tools: - scan-grant-recommendations - approve-grant-recommendation - decline-grant-recommendation - - query-relationships - - delete-relationship --- diff --git a/skills/memory/SKILL.md b/skills/memory/SKILL.md index 821182ed4..26405cc5d 100644 --- a/skills/memory/SKILL.md +++ b/skills/memory/SKILL.md @@ -1,11 +1,13 @@ --- name: memory description: > - Shared knowledge-graph memory — query/store/confirm plus decay-warnings-list. -version: "0.1.0" + Shared knowledge-graph memory — query/store/confirm facts, manage entity-to-entity relationships (query/delete edges), plus decay-warnings-list. +version: "0.2.0" tools: - memory-query - memory-store - memory-confirm - decay-warnings-list + - query-relationships + - delete-relationship --- diff --git a/skills/memory/tools/_shared/test-helpers.ts b/skills/memory/tools/_shared/test-helpers.ts new file mode 100644 index 000000000..c7c9debdd --- /dev/null +++ b/skills/memory/tools/_shared/test-helpers.ts @@ -0,0 +1,35 @@ +// Shared test scaffolding for the relationship-tool handler tests +// (query-relationships, delete-relationship). Both build an in-memory EntityMemory +// and a minimal ToolContext the same way; keep that in one place. +// +// NOTE: this directory has no tool.json, so skill discovery (discoverNestedToolNames) +// skips it — it is not a tool. +import pino from 'pino'; +import { KnowledgeGraphStore } from '../../../../src/memory/knowledge-graph.js'; +import { EmbeddingService } from '../../../../src/memory/embedding.js'; +import { EntityMemory } from '../../../../src/memory/entity-memory.js'; +import { MemoryValidator } from '../../../../src/memory/validation.js'; +import { createSilentLogger } from '../../../../src/logger.js'; +import type { ToolContext } from '../../../../src/skills/types.js'; + +// Returns both mem and store for tests that need direct store access — e.g. to +// simulate pre-migration duplicate data by bypassing upsert logic. +export function makeEntityMemoryWithStore() { + const embeddingService = EmbeddingService.createForTesting(); + const store = KnowledgeGraphStore.createInMemory(embeddingService); + const validator = new MemoryValidator(store, embeddingService); + return { mem: new EntityMemory(store, validator, embeddingService, createSilentLogger()), store }; +} + +export function makeEntityMemory(): EntityMemory { + return makeEntityMemoryWithStore().mem; +} + +export function makeCtx(entityMemory: EntityMemory, input: Record): ToolContext { + return { + input, + secret: () => 'test-key', + log: pino({ level: 'silent' }), + entityMemory, + } as unknown as ToolContext; +} diff --git a/skills/contacts/tools/delete-relationship/handler.test.ts b/skills/memory/tools/delete-relationship/handler.test.ts similarity index 81% rename from skills/contacts/tools/delete-relationship/handler.test.ts rename to skills/memory/tools/delete-relationship/handler.test.ts index f51a9dd3c..bf80d32dc 100644 --- a/skills/contacts/tools/delete-relationship/handler.test.ts +++ b/skills/memory/tools/delete-relationship/handler.test.ts @@ -1,34 +1,6 @@ import { describe, it, expect } from 'vitest'; -import pino from 'pino'; -import { KnowledgeGraphStore } from '../../../../src/memory/knowledge-graph.js'; -import { EmbeddingService } from '../../../../src/memory/embedding.js'; -import { EntityMemory } from '../../../../src/memory/entity-memory.js'; -import { MemoryValidator } from '../../../../src/memory/validation.js'; -import { createSilentLogger } from '../../../../src/logger.js'; import { DeleteRelationshipHandler } from './handler.js'; -import type { ToolContext } from '../../../../src/skills/types.js'; - -function makeEntityMemory() { - return makeEntityMemoryWithStore().mem; -} - -// Returns both mem and store for tests that need to bypass upsert -// (e.g. to simulate pre-migration duplicate data by inserting directly) -function makeEntityMemoryWithStore() { - const embeddingService = EmbeddingService.createForTesting(); - const store = KnowledgeGraphStore.createInMemory(embeddingService); - const validator = new MemoryValidator(store, embeddingService); - return { mem: new EntityMemory(store, validator, embeddingService, createSilentLogger()), store }; -} - -function makeCtx(entityMemory: EntityMemory, input: Record): ToolContext { - return { - input, - secret: () => 'test-key', - log: pino({ level: 'silent' }), - entityMemory, - } as unknown as ToolContext; -} +import { makeEntityMemory, makeEntityMemoryWithStore, makeCtx } from '../_shared/test-helpers.js'; describe('DeleteRelationshipHandler', () => { it('returns error for unknown predicate', async () => { diff --git a/skills/contacts/tools/delete-relationship/handler.ts b/skills/memory/tools/delete-relationship/handler.ts similarity index 100% rename from skills/contacts/tools/delete-relationship/handler.ts rename to skills/memory/tools/delete-relationship/handler.ts diff --git a/skills/contacts/tools/delete-relationship/tool.json b/skills/memory/tools/delete-relationship/tool.json similarity index 100% rename from skills/contacts/tools/delete-relationship/tool.json rename to skills/memory/tools/delete-relationship/tool.json diff --git a/skills/contacts/tools/query-relationships/handler.test.ts b/skills/memory/tools/query-relationships/handler.test.ts similarity index 84% rename from skills/contacts/tools/query-relationships/handler.test.ts rename to skills/memory/tools/query-relationships/handler.test.ts index 373892c78..0dac2381e 100644 --- a/skills/contacts/tools/query-relationships/handler.test.ts +++ b/skills/memory/tools/query-relationships/handler.test.ts @@ -1,34 +1,6 @@ import { describe, it, expect } from 'vitest'; -import pino from 'pino'; -import { KnowledgeGraphStore } from '../../../../src/memory/knowledge-graph.js'; -import { EmbeddingService } from '../../../../src/memory/embedding.js'; -import { EntityMemory } from '../../../../src/memory/entity-memory.js'; -import { MemoryValidator } from '../../../../src/memory/validation.js'; -import { createSilentLogger } from '../../../../src/logger.js'; import { QueryRelationshipsHandler } from './handler.js'; -import type { ToolContext } from '../../../../src/skills/types.js'; - -// makeEntityMemoryWithStore returns both for tests that need direct store access -// (e.g. to simulate pre-migration duplicates by bypassing upsert logic) -function makeEntityMemoryWithStore() { - const embeddingService = EmbeddingService.createForTesting(); - const store = KnowledgeGraphStore.createInMemory(embeddingService); - const validator = new MemoryValidator(store, embeddingService); - return { mem: new EntityMemory(store, validator, embeddingService, createSilentLogger()), store }; -} - -function makeEntityMemory() { - return makeEntityMemoryWithStore().mem; -} - -function makeCtx(entityMemory: EntityMemory, input: Record): ToolContext { - return { - input, - secret: () => 'test-key', - log: pino({ level: 'silent' }), - entityMemory, - } as unknown as ToolContext; -} +import { makeEntityMemory, makeEntityMemoryWithStore, makeCtx } from '../_shared/test-helpers.js'; describe('QueryRelationshipsHandler', () => { it('returns empty relationships when entity is not found', async () => { @@ -154,4 +126,21 @@ describe('QueryRelationshipsHandler', () => { expect(xiaopuData.relationships[0]!.subject).toBe('Jane Doe'); expect(xiaopuData.relationships[0]!.object).toBe('John Smith'); }); + + it('uses the resolved canonical label for the queried side, not the raw input casing', async () => { + const mem = makeEntityMemory(); + const { entity: jane } = await mem.createEntity({ type: 'person', label: 'Jane Doe', properties: {}, source: 'test' }); + const { entity: acme } = await mem.createEntity({ type: 'organization', label: 'Acme Corp', properties: {}, source: 'test' }); + await mem.upsertEdge(jane.id, acme.id, 'member_of', {}, 'test', 0.8); + + const handler = new QueryRelationshipsHandler(); + // Query with a differently-cased label; resolution is case-insensitive, so this + // resolves to the 'Jane Doe' node whose canonical label must be what we return. + const ctx = makeCtx(mem, { entity: 'jane doe' }); + const result = await handler.execute(ctx); + + const data = (result as { success: true; data: { relationships: Array<{ subject: string; object: string }> } }).data; + expect(data.relationships[0]!.subject).toBe('Jane Doe'); // canonical, not 'jane doe' + expect(data.relationships[0]!.object).toBe('Acme Corp'); + }); }); diff --git a/skills/contacts/tools/query-relationships/handler.ts b/skills/memory/tools/query-relationships/handler.ts similarity index 88% rename from skills/contacts/tools/query-relationships/handler.ts rename to skills/memory/tools/query-relationships/handler.ts index 7364bf048..f69f5e4de 100644 --- a/skills/contacts/tools/query-relationships/handler.ts +++ b/skills/memory/tools/query-relationships/handler.ts @@ -60,9 +60,12 @@ export class QueryRelationshipsHandler implements ToolHandler { const relationships = edges.map(({ edge, node, direction }) => ({ edge_id: edge.id, - subject: direction === 'outbound' ? entity : node.label, + // Use the resolved node's canonical label for the queried side (not the raw + // `entity` input), so both sides read consistently — e.g. querying "jane doe" + // returns "Jane Doe" to match the KG, not the caller's casing. + subject: direction === 'outbound' ? entityNode.label : node.label, predicate: edge.type, - object: direction === 'outbound' ? node.label : entity, + object: direction === 'outbound' ? node.label : entityNode.label, direction, confidence: edge.temporal.confidence, last_confirmed_at: edge.temporal.lastConfirmedAt.toISOString(), diff --git a/skills/contacts/tools/query-relationships/tool.json b/skills/memory/tools/query-relationships/tool.json similarity index 98% rename from skills/contacts/tools/query-relationships/tool.json rename to skills/memory/tools/query-relationships/tool.json index 29ee04ca6..e57a5924d 100644 --- a/skills/contacts/tools/query-relationships/tool.json +++ b/skills/memory/tools/query-relationships/tool.json @@ -1,7 +1,7 @@ { "name": "query-relationships", "description": "Query entity-to-entity relationships from the knowledge graph. Resolves the entity by name. Returns all stored relationships, optionally filtered by edge type. When multiple nodes share the same name, returns an ambiguous response with candidates so you can ask the user to clarify.", - "version": "1.0.0", + "version": "1.0.1", "sensitivity": "normal", "action_risk": "none", "inputs": { diff --git a/tests/unit/agents/resolved-pins-regression.test.ts b/tests/unit/agents/resolved-pins-regression.test.ts index f344f9e4f..ede7eab01 100644 --- a/tests/unit/agents/resolved-pins-regression.test.ts +++ b/tests/unit/agents/resolved-pins-regression.test.ts @@ -164,4 +164,38 @@ describe('resolved pin sets after #1494 bundling', () => { expect(config.pinned_skills).toContain('contact-update'); expect(config.pinned_skills).not.toContain('contact-lookup'); }); + + // #1502: the KG relationship tools moved from the contacts bundle to the memory + // bundle. They must NOT come along with the contacts bundle anymore, and the + // contacts agent keeps them only via its explicit individual pins (resolved set + // unchanged). The coordinator gains them because it pins the whole memory bundle. + it('query/delete-relationship live in the memory bundle, not contacts', () => { + const skills = new SkillRegistry(); + loadOnDiskSkills(skills); + const memory = skills.get('memory'); + const contacts = skills.get('contacts'); + expect(memory?.manifest.tools).toEqual( + expect.arrayContaining(['query-relationships', 'delete-relationship']), + ); + expect(contacts?.manifest.tools).not.toContain('query-relationships'); + expect(contacts?.manifest.tools).not.toContain('delete-relationship'); + }); + + it('contacts still resolves the relationship tools via explicit individual pins', () => { + const config = loadAgentConfig(resolve(agentsDir, 'contacts.yaml')); + // contacts pins the contacts bundle (which no longer carries them) AND the two + // tools individually — so removing them from the bundle is behavior-preserving. + expect(config.pinned_skills).toContain('query-relationships'); + expect(config.pinned_skills).toContain('delete-relationship'); + const tools = resolveAgent('contacts.yaml'); + expect(tools).toContain('query-relationships'); + expect(tools).toContain('delete-relationship'); + }); + + it('coordinator gains the relationship tools via the memory bundle (#1502)', () => { + const tools = resolveAgent('coordinator.yaml'); + expect(tools).toEqual( + expect.arrayContaining(['query-relationships', 'delete-relationship']), + ); + }); });