diff --git a/apps/api/src/routes/mcp/instruction-tools.ts b/apps/api/src/routes/mcp/instruction-tools.ts index 4e0571b63..2f54d754e 100644 --- a/apps/api/src/routes/mcp/instruction-tools.ts +++ b/apps/api/src/routes/mcp/instruction-tools.ts @@ -278,13 +278,23 @@ export async function handleGetInstructions( ] : []), ], - // Include formatted directives as a readable text block (primary way agents consume knowledge) + // Formatted directives are the SINGLE representation of knowledge and policies. + // + // These used to be accompanied by `knowledgeContext` / `policyContext` structured + // arrays "for programmatic use", but nothing ever consumed them and every observation + // and policy body was therefore serialized twice — ~86K chars (~21k tokens) of pure + // duplication on every session bootstrap. Policy IDs (needed by `update_policy` / + // `remove_policy`) lived only in the structured array, so they are now rendered inline + // by formatPolicyDirectives instead. See the R1 token-optimization task. + // + // The policy id is deliberately conveyed in prose while every other id here + // (`context.task.id`, `project.id`, `context.workspaceId`) is a structured field. Do + // NOT "fix" that by re-adding a `policyIds` array: the only consumer of this payload is + // the LLM, for which prose and JSON are equally parseable; a bare id array would need + // its titles re-duplicated to be correlatable, costing ~2x the inline form; and callers + // that need structured policy data already have `list_policies` / `get_policy`. ...(knowledgeDirectives ? { knowledgeDirectives } : {}), - // Also include structured data for programmatic use - ...(knowledgeContext.length > 0 ? { knowledgeContext } : {}), - // Include policy directives and structured data ...(policyDirectives ? { policyDirectives } : {}), - ...(policyContext.length > 0 ? { policyContext } : {}), }; return jsonRpcSuccess(requestId, { @@ -414,27 +424,31 @@ interface PolicyEntry { * Format active policies into a readable text block grouped by category. * Returns null if there are no policies. * + * Each policy carries its full id inline so agents can call `update_policy` / + * `remove_policy` without a separate lookup. This is the only place the id is exposed — + * the former `policyContext` structured array was removed as duplication. + * * Output looks like: * ## Project Policies — you MUST follow these * * ### Rules - * - **Always use conventional commits**: Commit messages must follow ... + * - **Always use conventional commits** (id: 7d24e435-0153-44a6-a532-1244510d9e25): Commit messages must follow ... * * ### Constraints - * - **This project uses Valibot, not Zod**: All runtime validation ... + * - **This project uses Valibot, not Zod** (id: 9f1c02ab-77de-4b30-8c11-3ac6d5e81b47): All runtime validation ... */ function formatPolicyDirectives(entries: PolicyEntry[]): string | null { if (entries.length === 0) return null; // Group by category - const grouped = new Map(); + const grouped = new Map(); for (const entry of entries) { let group = grouped.get(entry.category); if (!group) { group = []; grouped.set(entry.category, group); } - group.push({ title: entry.title, content: entry.content }); + group.push({ id: entry.id, title: entry.title, content: entry.content }); } // Category display order and labels @@ -450,7 +464,9 @@ function formatPolicyDirectives(entries: PolicyEntry[]): string | null { const label = categoryLabels[category] || category; lines.push(`### ${label}`); for (const item of items) { - lines.push(`- **${item.title}**: ${item.content}`); + // The id MUST be rendered in full — `update_policy` / `remove_policy` resolve it with + // `WHERE id = ?` (exact match), so an abbreviated id would not address the row. + lines.push(`- **${item.title}** (id: ${item.id}): ${item.content}`); } lines.push(''); } @@ -472,7 +488,8 @@ function buildPolicyInstructions(hasPolicies: boolean, isConversation: boolean): ); instructions.push( 'If a user statement contradicts an existing policy, use `update_policy` to update it. ' + - 'If a policy is no longer relevant, use `remove_policy` to deactivate it.' + 'If a policy is no longer relevant, use `remove_policy` to deactivate it. ' + + 'Each policy in policyDirectives is tagged with its `policyId` as "(id: ...)" — pass that id exactly as shown.' ); } diff --git a/apps/api/tests/unit/routes/mcp-instruction-payload-dedup.test.ts b/apps/api/tests/unit/routes/mcp-instruction-payload-dedup.test.ts new file mode 100644 index 000000000..1a5c4a700 --- /dev/null +++ b/apps/api/tests/unit/routes/mcp-instruction-payload-dedup.test.ts @@ -0,0 +1,397 @@ +/** + * Regression tests for R1 (token optimization): the `get_instructions` payload must carry + * each knowledge observation and each policy body EXACTLY ONCE. + * + * Before this change the response emitted `knowledgeDirectives` (rendered markdown) AND + * `knowledgeContext` (structured JSON), plus `policyDirectives` AND `policyContext` — with + * every observation/policy body byte-identical across the pair. On the real SAM project + * that was ~86K chars (~21k tokens) of duplication on every single session bootstrap. + * + * These tests are DISCRIMINATING: `countOccurrences` returns 2 for every body on the + * pre-fix code and 1 after. Verified by re-adding the structured arrays locally. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + selectRows: [] as unknown[][], + drizzle: vi.fn(), + projectData: { + getSession: vi.fn(), + getAllHighConfidenceKnowledge: vi.fn(), + getActivePolicies: vi.fn(), + }, +})); + +vi.mock('drizzle-orm/d1', () => ({ drizzle: mocks.drizzle })); +vi.mock('../../../src/services/project-data', () => mocks.projectData); + +import { handleGetInstructions } from '../../../src/routes/mcp/instruction-tools'; +import type { McpTokenData } from '../../../src/services/mcp-token'; + +const task = { + id: 'task-1', + projectId: 'project-1', + title: 'Implement bootstrap', + description: 'Unify bootstrap', + status: 'in_progress', + priority: 1, + outputBranch: 'sam/bootstrap', + taskMode: 'task', +}; + +const project = { + id: 'project-1', + name: 'SAM', + repository: 'owner/repo', + defaultBranch: 'main', + repoProvider: 'github', +}; + +const baseToken: McpTokenData = { + taskId: 'task-1', + contextType: 'task', + taskMode: 'task', + projectId: 'project-1', + userId: 'user-1', + workspaceId: 'workspace-1', + createdAt: new Date().toISOString(), +}; + +/** + * Full-length UUIDs on purpose: `update_policy` / `remove_policy` resolve the row with + * `WHERE id = ?` (exact match), so a truncated or elided id would not address it. + */ +const POLICY_RULE_ID = '7d24e435-0153-44a6-a532-1244510d9e25'; +const POLICY_CONSTRAINT_ID = '9f1c02ab-77de-4b30-8c11-3ac6d5e81b47'; +const POLICY_PREFERENCE_ID = 'c3b7145e-2d80-4f6a-9e52-08bb1f7d4a10'; + +// Distinctive single-line ASCII bodies so occurrence counting is unambiguous under +// JSON string escaping. +const RULE_BODY = 'Agents must load SAM context before starting any work at all.'; +const CONSTRAINT_BODY = 'All runtime validation uses Valibot rather than Zod in this repo.'; +const PREFERENCE_BODY = 'Prefer small vertical slices over large horizontal refactors.'; +const OBSERVATION_A = 'Bootstrap policy stays in the Worker control plane.'; +const OBSERVATION_B = 'Raphael primarily reviews SAM from the mobile PWA.'; + +const POLICIES = [ + { + id: POLICY_RULE_ID, + category: 'rule', + title: 'Call get_instructions first', + content: RULE_BODY, + confidence: 0.95, + }, + { + id: POLICY_CONSTRAINT_ID, + category: 'constraint', + title: 'Valibot not Zod', + content: CONSTRAINT_BODY, + confidence: 0.9, + }, + { + id: POLICY_PREFERENCE_ID, + category: 'preference', + title: 'Small vertical slices', + content: PREFERENCE_BODY, + confidence: 0.7, + }, +]; + +const OBSERVATIONS = [ + { + entityName: 'Architecture', + entityType: 'context', + content: OBSERVATION_A, + confidence: 0.95, + }, + { entityName: 'User', entityType: 'preference', content: OBSERVATION_B, confidence: 0.9 }, +]; + +function makeEnv() { + return { + DATABASE: {}, + PROJECT_DATA: { idFromName: vi.fn().mockReturnValue('do-id'), get: vi.fn() }, + } as never; +} + +function makeMockDb() { + return { + select: vi.fn(() => ({ + from: vi.fn(() => ({ + where: vi.fn(() => ({ limit: vi.fn(async () => mocks.selectRows.shift() ?? []) })), + })), + })), + }; +} + +/** The exact text the agent receives over the wire. */ +function serializedPayload(response: Awaited>): string { + expect(response.error).toBeUndefined(); + const result = response.result as { content: Array<{ text: string }> }; + return result.content[0]?.text ?? ''; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +function countOccurrences(haystack: string, needle: string): number { + let count = 0; + let index = haystack.indexOf(needle); + while (index !== -1) { + count += 1; + index = haystack.indexOf(needle, index + needle.length); + } + return count; +} + +async function getPayload() { + mocks.selectRows = [[task], [project]]; + const response = await handleGetInstructions('request-1', baseToken, makeEnv()); + const text = serializedPayload(response); + return { text, parsed: JSON.parse(text) as Record }; +} + +describe('get_instructions payload de-duplication (R1)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.selectRows = []; + mocks.drizzle.mockReturnValue(makeMockDb()); + mocks.projectData.getSession.mockResolvedValue(null); + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue(OBSERVATIONS); + mocks.projectData.getActivePolicies.mockResolvedValue(POLICIES); + }); + + it('serializes every policy body exactly once', async () => { + const { text } = await getPayload(); + + for (const body of [RULE_BODY, CONSTRAINT_BODY, PREFERENCE_BODY]) { + expect(countOccurrences(text, body), `policy body duplicated: ${body}`).toBe(1); + } + }); + + it('serializes every policy title exactly once', async () => { + const { text } = await getPayload(); + + for (const policy of POLICIES) { + expect(countOccurrences(text, policy.title), `title duplicated: ${policy.title}`).toBe(1); + } + }); + + it('serializes every knowledge observation exactly once', async () => { + const { text } = await getPayload(); + + for (const observation of [OBSERVATION_A, OBSERVATION_B]) { + expect(countOccurrences(text, observation), `observation duplicated`).toBe(1); + } + }); + + it('omits the knowledgeContext and policyContext structured arrays entirely', async () => { + const { text, parsed } = await getPayload(); + + expect(parsed).not.toHaveProperty('knowledgeContext'); + expect(parsed).not.toHaveProperty('policyContext'); + // Also absent as literal keys anywhere in the serialized text. + expect(text).not.toContain('"knowledgeContext"'); + expect(text).not.toContain('"policyContext"'); + + // Liveness (rule 62 req 5): an absence assertion is also satisfied by a payload that + // rendered nothing at all. Prove the surviving representation is actually populated. + expect(String(parsed.knowledgeDirectives)).toContain(OBSERVATION_A); + expect(String(parsed.policyDirectives)).toContain(RULE_BODY); + expect(parsed.task).toMatchObject({ id: 'task-1' }); + }); + + it('keeps the rendered directives as the single representation', async () => { + const { parsed } = await getPayload(); + + const knowledge = String(parsed.knowledgeDirectives); + const policies = String(parsed.policyDirectives); + + expect(knowledge).toContain(OBSERVATION_A); + expect(knowledge).toContain(OBSERVATION_B); + expect(knowledge).toContain('**Architecture** (context)'); + + expect(policies).toContain(RULE_BODY); + expect(policies).toContain(CONSTRAINT_BODY); + expect(policies).toContain(PREFERENCE_BODY); + // Category grouping and headings survive. + expect(policies).toContain('### Rules (MUST follow)'); + expect(policies).toContain('### Constraints (technical limitations)'); + expect(policies).toContain('### Preferences (soft guidance)'); + }); +}); + +describe('policy management identifiers survive de-duplication', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.selectRows = []; + mocks.drizzle.mockReturnValue(makeMockDb()); + mocks.projectData.getSession.mockResolvedValue(null); + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue(OBSERVATIONS); + mocks.projectData.getActivePolicies.mockResolvedValue(POLICIES); + }); + + it('renders every policy id inline in policyDirectives', async () => { + const { parsed } = await getPayload(); + const policies = String(parsed.policyDirectives); + + for (const policy of POLICIES) { + expect(policies, `missing id for ${policy.title}`).toContain(`(id: ${policy.id})`); + } + }); + + it('renders ids in FULL so update_policy/remove_policy can resolve WHERE id = ?', async () => { + const { parsed } = await getPayload(); + const policies = String(parsed.policyDirectives); + + // Guard against a truncated/elided rendering such as "(id: 7d24e435…)". Asserted on + // the whole rendered block, because a capture group of [0-9a-f-]+ structurally cannot + // contain an ellipsis and so could never catch this. + expect(policies).not.toContain('…'); + expect(policies).not.toContain('...'); + + for (const policy of POLICIES) { + // Recover the id the way an agent would, then assert it round-trips exactly. + const match = new RegExp( + `\\*\\*${escapeRegExp(policy.title)}\\*\\* \\(id: ([^)]+)\\)`, + ).exec(policies); + expect(match, `no id captured for ${policy.title}`).not.toBeNull(); + expect(match?.[1]).toBe(policy.id); + expect(match?.[1]).toHaveLength(36); + } + }); + + it('binds each id to its own policy body', async () => { + const { parsed } = await getPayload(); + const policies = String(parsed.policyDirectives); + + expect(policies).toContain(`(id: ${POLICY_RULE_ID}): ${RULE_BODY}`); + expect(policies).toContain(`(id: ${POLICY_CONSTRAINT_ID}): ${CONSTRAINT_BODY}`); + expect(policies).toContain(`(id: ${POLICY_PREFERENCE_ID}): ${PREFERENCE_BODY}`); + }); + + it('tells the agent where to find the policy id', async () => { + const { parsed } = await getPayload(); + const instructions = JSON.stringify(parsed.instructions); + + expect(instructions).toContain('update_policy'); + expect(instructions).toContain('remove_policy'); + expect(instructions).toContain('(id: ...)'); + }); +}); + +describe('empty knowledge and policy paths still degrade correctly', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.selectRows = []; + mocks.drizzle.mockReturnValue(makeMockDb()); + mocks.projectData.getSession.mockResolvedValue(null); + }); + + it('omits both directive fields when there is nothing to render', async () => { + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue([]); + mocks.projectData.getActivePolicies.mockResolvedValue([]); + + const { parsed } = await getPayload(); + + expect(parsed).not.toHaveProperty('knowledgeDirectives'); + expect(parsed).not.toHaveProperty('policyDirectives'); + expect(parsed).not.toHaveProperty('knowledgeContext'); + expect(parsed).not.toHaveProperty('policyContext'); + // Liveness: the response is still a real payload, not an empty/crashed one. + expect(parsed.task).toMatchObject({ id: 'task-1' }); + expect(JSON.stringify(parsed.instructions)).toContain('no stored knowledge yet'); + }); + + it('still returns knowledge directives when only policies are empty', async () => { + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue(OBSERVATIONS); + mocks.projectData.getActivePolicies.mockResolvedValue([]); + + const { parsed } = await getPayload(); + + expect(String(parsed.knowledgeDirectives)).toContain(OBSERVATION_A); + expect(parsed).not.toHaveProperty('policyDirectives'); + // The policy maintenance instructions are suppressed when there are no policies. + expect(JSON.stringify(parsed.instructions)).not.toContain('remove_policy'); + }); + + it('survives a knowledge retrieval failure without emitting either field', async () => { + mocks.projectData.getAllHighConfidenceKnowledge.mockRejectedValue(new Error('DO down')); + mocks.projectData.getActivePolicies.mockResolvedValue(POLICIES); + + const { parsed } = await getPayload(); + + expect(parsed).not.toHaveProperty('knowledgeDirectives'); + expect(parsed).not.toHaveProperty('knowledgeContext'); + // Policies are unaffected and still carry their ids. + expect(String(parsed.policyDirectives)).toContain(`(id: ${POLICY_RULE_ID})`); + }); + + it('survives a policy retrieval failure without emitting either field', async () => { + // Mirror of the knowledge-failure case: production wraps both retrievals in the same + // shape of try/catch, so both degradation paths need coverage. + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue(OBSERVATIONS); + mocks.projectData.getActivePolicies.mockRejectedValue(new Error('DO down')); + + const { parsed } = await getPayload(); + + expect(parsed).not.toHaveProperty('policyDirectives'); + expect(parsed).not.toHaveProperty('policyContext'); + // Knowledge is unaffected, and the policy instructions are correctly suppressed. + expect(String(parsed.knowledgeDirectives)).toContain(OBSERVATION_A); + expect(JSON.stringify(parsed.instructions)).not.toContain('remove_policy'); + }); +}); + +describe('multi-observation entities keep every observation exactly once', () => { + // formatKnowledgeDirectives groups by entityName and joins an entity's observations with + // ' | '. That branch is the easiest place for a future change to silently drop all but + // the last observation, and nothing in the repo covered it. + const SAME_ENTITY_OBS = [ + { + entityName: 'SharedEntity', + entityType: 'preference', + content: 'First observation about the shared entity.', + confidence: 0.95, + }, + { + entityName: 'SharedEntity', + entityType: 'preference', + content: 'Second observation about the shared entity.', + confidence: 0.9, + }, + { + entityName: 'OtherEntity', + entityType: 'context', + content: 'Sole observation about the other entity.', + confidence: 0.85, + }, + ]; + + beforeEach(() => { + vi.clearAllMocks(); + mocks.selectRows = []; + mocks.drizzle.mockReturnValue(makeMockDb()); + mocks.projectData.getSession.mockResolvedValue(null); + mocks.projectData.getAllHighConfidenceKnowledge.mockResolvedValue(SAME_ENTITY_OBS); + mocks.projectData.getActivePolicies.mockResolvedValue(POLICIES); + }); + + it('renders both observations of a shared entity, each exactly once', async () => { + const { text, parsed } = await getPayload(); + const knowledge = String(parsed.knowledgeDirectives); + + for (const observation of SAME_ENTITY_OBS) { + expect(knowledge, `dropped: ${observation.content}`).toContain(observation.content); + expect(countOccurrences(text, observation.content)).toBe(1); + } + + // The entity header is emitted once and both observations hang off it. + expect(countOccurrences(knowledge, '**SharedEntity** (preference)')).toBe(1); + expect(knowledge).toContain( + 'First observation about the shared entity. | Second observation about the shared entity.' + ); + expect(knowledge).toContain('**OtherEntity** (context)'); + }); +}); diff --git a/apps/www/src/content/docs/docs/architecture/overview.md b/apps/www/src/content/docs/docs/architecture/overview.md index 76c91acc9..10c2e98ab 100644 --- a/apps/www/src/content/docs/docs/architecture/overview.md +++ b/apps/www/src/content/docs/docs/architecture/overview.md @@ -196,6 +196,39 @@ Agent behavior is assembled from several override layers rather than a single gl - **Skills** — a first-class override layer that further specializes a profile for a specific task type. - **Provider modes** — each agent runs in one of three auth modes: `user-api-key` (the user's own key), `oauth` (a subscription token such as Claude Max), or `sam` (the platform-managed AI proxy, opt-in). See [Agent Authentication](/docs/guides/agents/). +### Agent Bootstrap Payload (`get_instructions`) + +Every agent session begins by calling the SAM MCP `get_instructions` tool +(`handleGetInstructions()` in `apps/api/src/routes/mcp/instruction-tools.ts`), which returns +the task/session context, the project record, a mode-specific `instructions[]` array, and the +project's stored knowledge and policies. + +Knowledge and policies are delivered as **rendered markdown only** — `knowledgeDirectives` and +`policyDirectives`, produced by `formatKnowledgeDirectives()` and `formatPolicyDirectives()`. +Each field is omitted entirely when there is nothing to render; a failed Durable Object read +also degrades to "omitted" rather than erroring the bootstrap. + +There is deliberately **no second structured copy** of this data. The payload previously also +carried `knowledgeContext` and `policyContext` arrays holding byte-identical observation and +policy bodies. Nothing consumed them, and on a mature project they accounted for roughly half +the payload (~166K → ~81K characters, about 21k tokens per session bootstrap), so they were +removed. Callers that need machine-readable records should use the dedicated tools — +`list_policies` / `get_policy`, and `search_knowledge` / `get_project_knowledge` — rather than +parsing the bootstrap payload. + +Because `update_policy` and `remove_policy` resolve rows by exact id (`updatePolicy()` and +`removePolicy()` in `apps/api/src/durable-objects/project-data/policies.ts` use `WHERE id = ?`), +each rendered policy line carries its **full, untruncated** id: + +```text +### Rules (MUST follow) +- **Call get_instructions first** (id: 7d24e435-0153-44a6-a532-1244510d9e25): Agents must load SAM context before starting work. +``` + +Knowledge observations do **not** currently carry their `observationId` in this payload, so +`update_knowledge`, `remove_knowledge`, and `confirm_knowledge` need an id obtained from +`search_knowledge` or `get_project_knowledge` first. + ## Durable Objects Deep Dive ### ProjectData DO diff --git a/tasks/archive/2026-08-23-remove-duplicated-structured-arrays-get-instructions.md b/tasks/archive/2026-08-23-remove-duplicated-structured-arrays-get-instructions.md new file mode 100644 index 000000000..ad702f4dd --- /dev/null +++ b/tasks/archive/2026-08-23-remove-duplicated-structured-arrays-get-instructions.md @@ -0,0 +1,220 @@ +# Remove duplicated structured arrays from SAM MCP `get_instructions` + +**Task ID**: 01M0QHHVEQY11VC37Y29B6GGY4 +**Idea**: 01M0QGZQ15WE9DDQENGV6S9XZ5 (token-optimization program, recommendation **R1**) +**Research**: library file `/engineering/research/token-optimization-research.md` (fileId 01M0QGYKH9PCSM5E7N58ZD98JT), sections 3.1 and 8/R1 +**Branch**: `sam/remove-duplicated-structured-arrays-b6ggy4` + +## Problem + +Every SAM agent session begins with a mandatory `get_instructions` call. The response +currently sends **every knowledge observation and every policy twice**: + +- once rendered as markdown in `knowledgeDirectives` / `policyDirectives` (what the + instructions actually tell the agent to read), and +- once again as structured JSON in `knowledgeContext` / `policyContext`, labelled + "Also include structured data for programmatic use" + (`apps/api/src/routes/mcp/instruction-tools.ts:283-287`). + +Nothing consumes the structured arrays. They are pure duplication paid on every session +bootstrap, by every agent, forever. + +### Measured (this session's real payload, commit 220b4ce18, project SAM) + +| Field | chars | +|---|---:| +| `context` + `task` + `project` + `instructions` | 6,122 | +| `knowledgeDirectives` | 26,128 | +| `knowledgeContext` (duplicate) | 30,429 | +| `policyDirectives` | 44,191 | +| `policyContext` (duplicate) | 52,157 | + +Serialized response (`JSON.stringify(result, null, 2)`): **166,814 chars**. + +Duplication verified byte-for-byte with `jq`: + +- 81 / 81 `policyContext[].content` strings appear verbatim inside `policyDirectives` +- 81 / 81 `policyContext[].title` strings appear verbatim inside `policyDirectives` +- 50 / 50 `knowledgeContext[].observation` strings appear verbatim inside `knowledgeDirectives` + +## Research findings + +### F1 — Consumer enumeration (rule 44: enumerate every consumer before removing) + +`grep -rn "knowledgeContext\|policyContext"` across `apps/`, `packages/`, tests, docs, +`specs/`, `.claude/`: + +| Location | Kind | Action | +|---|---|---| +| `instruction-tools.ts:141,154,169,173,284` | producer (`knowledgeContext`) | becomes a local variable, no longer emitted | +| `instruction-tools.ts:179,188,202,204,287` | producer (`policyContext`) | becomes a local variable, no longer emitted | +| `tasks/archive/2026-04-26-policy-propagation-phase4.md:60` | historical archive record | **do not edit** (archive is a historical record) | +| `tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md:48-49` | unstarted backlog acceptance criteria naming `knowledgeContext` | **update** to name `knowledgeDirectives` so the future task is not written against a removed field | + +There is **no runtime consumer** — not in `apps/web/`, not in `packages/vm-agent/`, not in +any test, not in any doc. The `get_instructions` tool description +(`tool-definitions-task-tools.ts:7-9`) does not mention either field. No `instructions[]` +string references `knowledgeContext` or `policyContext` (they reference +`knowledgeDirectives` at line 378 and `policyDirectives` at line 470, both of which stay). + +### F2 — Policy IDs are ONLY in the structured array (the blocking detail) + +`policyDirectives` renders `- **{title}**: {content}` — **no id**. Verified: 0 of 81 policy +IDs appear anywhere in `policyDirectives`. But `instructions[14]` tells the agent to call +`update_policy` / `remove_policy`, and both require `policyId`. + +`apps/api/src/durable-objects/project-data/policies.ts:118,136` use `WHERE id = ?` — +**exact match**. A truncated/elided id (`7d24e435…`) would NOT resolve. The full UUID must +be rendered. Cost: 81 × ~43 = 3,483 chars, against 52,157 saved. + +### F3 — Knowledge management tools need `observationId`, which is ALREADY absent + +`update_knowledge` / `remove_knowledge` / `confirm_knowledge` all require `observationId`. +`knowledgeContext` never carried it — it maps only `entityName`, `entityType`, +`observation`, `confidence` (`instruction-tools.ts:154-159`), even though the underlying +`SELECT o.*` (`knowledge.ts:390`) returns the observation id. + +So removing `knowledgeContext` **loses nothing** these tools could have used. The missing +`observationId` is a genuine *pre-existing* gap (instructions[12] asks agents to maintain +knowledge they cannot address), but fixing it is a behaviour change, not a de-duplication. +Sibling task R3 is editing knowledge selection in this same builder, so adding fields here +would conflict. → **filed as a separate backlog task**, not fixed in this PR. + +### F4 — Coordination + +Two sibling tasks touch the same response builder: R3 (knowledge selection ordering) and +R2 (policy expiry fields). This diff must stay minimal and land first; rebase on +`origin/main` before pushing. + +## Implementation checklist + +- [x] Stop emitting `knowledgeContext` and `policyContext` from the `get_instructions` + result object (`instruction-tools.ts:283-287`); keep both as local variables feeding + the formatters and the `hasKnowledge` / `hasPolicies` instruction switches +- [x] Render the full policy id inline in `formatPolicyDirectives`: + `- **{title}** (id: {uuid}): {content}` +- [x] Update the `policyDirectives` doc-comment example to show the id +- [x] Update `buildPolicyInstructions` so the agent is told where to find the id +- [x] Update `tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md` acceptance + criteria to reference `knowledgeDirectives` instead of the removed `knowledgeContext` +- [x] File a backlog task for the pre-existing missing `observationId` (F3) +- [x] Regression tests (see below) +- [x] `pnpm lint && pnpm typecheck && pnpm test && pnpm build` + +## Tests + +- [x] **Discriminating de-duplication test**: each policy `content` and each knowledge + `observation` appears **exactly once** in the serialized response. Must fail on + pre-fix code (where each appears twice). Verify that once. +- [x] **Fields absent**: response has no `knowledgeContext` / `policyContext` key +- [x] **Identifier survival**: every active policy's full id appears in `policyDirectives`, + and the rendered id is the exact string `update_policy` / `remove_policy` accept + (no truncation) +- [x] **No-regression**: titles, contents, observations, category grouping and headers all + still present; empty-knowledge and empty-policy paths still omit the directive fields + and still select the correct instruction variants + +## Acceptance criteria + +- [x] `get_instructions` no longer emits `knowledgeContext` or `policyContext` +- [x] Every policy's full, exact id is available to the agent in the rendered directives +- [x] Payload for the SAM project drops from ~166.8K to ~81.0K chars (~51%, ~21.4k tokens) +- [x] No consumer anywhere in the repo reads the removed fields +- [x] Staging: a real session bootstrap receives directives, can act on a policy by id, and + the payload is measurably smaller (before/after bytes reported in the PR) + +## References + +- `.claude/rules/44-dual-write-migration-enumerate-writers.md` — enumerate every consumer +- `.claude/rules/62-tests-must-observe-the-real-trigger.md` — tests must reach the real path +- `.claude/rules/02-quality-gates.md` — discriminating regression tests +- `tasks/archive/2026-04-26-policy-propagation-phase4.md` — where `policyContext` originated + +## Outcome + +**Implemented** in commit `da1d3160a` on `sam/remove-duplicated-structured-arrays-b6ggy4`. + +### Projected result (local computation — NOT yet a live re-measurement) + +| | chars | +|---|---:| +| BEFORE (`JSON.stringify(result, null, 2)`, real SAM payload) | 166,814 | +| AFTER (projected; incl. +3,483 for inline policy ids) | 81,025 | +| **Projected saving** | **85,789 (-51%, ~21.4k tokens per session bootstrap)** | + +The BEFORE number is empirical. The AFTER number is arithmetic over the real payload and +MUST be replaced with a measured post-deploy figure during staging verification. + +A live BEFORE baseline was additionally captured on staging (pre-fix `main`, project +`01KTKXZ4ZZAT6MJFXRW1ZTQ7RB` seeded with 3 marked policies + 1 observation): 6,025 chars, +every seeded body appearing exactly **2x**, and **no** policy id in the rendered directives. + +### Discriminating-test proof + +Re-added the two structured arrays locally; exactly 4 of the 12 new tests went red — +the three "exactly once" body/title/observation tests and the "omits the structured +arrays" test. Restored → 12/12 green. + +### Validation + +- `pnpm lint` — 0 errors (3 pre-existing `apps/web` warnings, untouched) +- `pnpm typecheck` — 19/19 tasks clean +- `apps/api` suite — 595 files, 8,037 tests, 0 failed, **0 collection errors** + (8,025 baseline + 12 new) +- `pnpm build` — 9/9 +- `pnpm check:fast` — 13/13, 0 blocking type-boundary findings + +### Deviation from the brief + +The brief suggested an elided id, e.g. `(id: 7d24e435…)`. That would **break** +`update_policy` / `remove_policy`: `policies.ts:118,136` resolve with `WHERE id = ?` +(exact match). Full 36-char UUIDs are rendered instead — 3,483 chars against 52,157 saved. + +## Staging verification (2026-08-23, deploy run 32648180754 — SUCCESS) + +Measured through the **real deployed Worker** by calling `POST https://api.sammy.party/mcp` +with a live MCP token read from staging KV (`mcp:`), i.e. the exact production code +path an agent bootstraps through. Project `01KTKXZ4ZZAT6MJFXRW1ZTQ7RB` was seeded with 3 +marked policies (one per category) + 1 observation; **identical data** measured either side +of the deploy. + +| | BEFORE (pre-fix `main`) | AFTER (this branch) | +|---|---:|---:| +| total payload | 6,025 chars | **5,230 chars** | +| `knowledgeContext` | 177 chars, 1 item | **ABSENT** | +| `policyContext` | 652 chars, 3 items | **ABSENT** | +| `policyDirectives` | 481 chars | 610 chars (+129, the 3 inline ids) | +| occurrences of each seeded body | **2** | **1** | +| full policy ids rendered | **0 of 3** | **3 of 3** | + +The absolute delta is small only because the fixture is small — the fixed +`context`/`task`/`project`/`instructions` overhead (~4,010 chars) dominates at this size. +The *duplication* is what was eliminated, and it scales with knowledge + policy volume: on +the real SAM project (81 policies, 50 observations) that is 166,814 → ~81,025 chars. + +### Management round-trip proven end-to-end + +The decisive check — an id scraped out of the rendered markdown must actually address the +row, which is precisely what an elided id would have broken: + +1. Scraped `c546f67b-81cd-4224-a39f-8f52a3655aeb` from `- **...** (id: ...)` in `policyDirectives` +2. `update_policy` with only that id → `{"updated":true,...}` +3. Fresh `get_instructions` → new text present, stale text gone +4. `remove_policy` × 3 → `{"removed":true,...}` each + +Incidentally confirmed the filed `observationId` gap: cleaning up the seeded observation +required `search_knowledge` to obtain the id, because `get_instructions` still does not +expose it. + +### Cleanup + +All seeded data removed; payload returned to exactly the 4,010-char empty-path shape with +zero `R1VERIFY` residue. No VMs or nodes were created (the MCP endpoint was exercised +directly), so the zero-VMs-at-rest rule is respected. + +### Rule 13 regression pass (Playwright, live `app.sammy.party`) + +Authenticated via `POST https://api.sammy.party/api/auth/token-login` (status 200), then at +both 375x667 and 1280x800: dashboard, `/projects` (16 projects listed), and `/settings` all +rendered; `GET /health` 200 `{"status":"healthy"}`; **0 console errors** at both viewports. +Screenshots in `.codex/tmp/playwright-screenshots/r1-staging-*.png`. diff --git a/tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md b/tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md index e418ce59c..896086d1c 100644 --- a/tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md +++ b/tasks/backlog/2026-04-13-knowledge-graph-test-coverage.md @@ -45,8 +45,8 @@ PR #693 shipped the knowledge graph feature with Playwright visual audits for th - [ ] `DELETE /observations/:observationId` — happy path ### HIGH — get_instructions Integration -- [ ] When `getRelevantKnowledge` returns results, response includes `knowledgeContext` + 3-item instructions -- [ ] When `getRelevantKnowledge` returns empty, response omits `knowledgeContext` + 2-item instructions +- [ ] When `getRelevantKnowledge` returns results, response includes `knowledgeDirectives` + 3-item instructions +- [ ] When `getRelevantKnowledge` returns empty, response omits `knowledgeDirectives` + 2-item instructions - [ ] When `getRelevantKnowledge` throws, `get_instructions` still succeeds (error swallowed) ### MEDIUM — Row Parser Tests diff --git a/tasks/backlog/2026-08-23-get-instructions-missing-observation-ids.md b/tasks/backlog/2026-08-23-get-instructions-missing-observation-ids.md new file mode 100644 index 000000000..ce1099a68 --- /dev/null +++ b/tasks/backlog/2026-08-23-get-instructions-missing-observation-ids.md @@ -0,0 +1,68 @@ +# `get_instructions` asks agents to maintain knowledge they cannot address + +**Discovered during**: task 01M0QHHVEQY11VC37Y29B6GGY4 (R1 — remove duplicated structured +arrays from `get_instructions`), 2026-08-23. + +## Problem + +`get_instructions` emits `instructions[12]`: + +> The knowledgeDirectives field above contains stored knowledge from previous sessions. +> Apply these preferences and facts to your work. If any observation seems outdated, call +> `update_knowledge` or `remove_knowledge`. If you verify an observation is still accurate, +> call `confirm_knowledge` to keep it fresh. + +All three of those tools require an `observationId`. **No observation id is present +anywhere in the `get_instructions` response.** An agent that follows this instruction +literally cannot: it has to guess, or fall back to `search_knowledge` / +`get_project_knowledge` to re-fetch ids it was never given. + +## Root cause + +`apps/api/src/routes/mcp/instruction-tools.ts` maps the retrieval result to only four +fields: + +```ts +knowledgeContext = allHighConfidence.map((r) => ({ + entityName: r.entityName, + entityType: r.entityType, + observation: r.content, + confidence: r.confidence, +})); +``` + +The underlying query (`apps/api/src/durable-objects/project-data/knowledge.ts:390`) +selects `o.*`, so the observation `id` **is** available at that point — it is simply +dropped by the mapper. This predates the R1 de-duplication change; removing +`knowledgeContext` did not cause it and did not make it worse (that array never carried +the id either). + +Contrast with policies: R1 added the full policy id inline to `policyDirectives` +precisely so `update_policy` / `remove_policy` stay usable. Knowledge has the same +instruction but never had the same affordance. + +## Proposed fix + +Render a compact id alongside each observation in `formatKnowledgeDirectives`, mirroring +what R1 did for policies. Note the current formatter **joins multiple observations per +entity** with `' | '`, so ids must be attached per-observation, not per-entity — this is a +real format change, not a one-line addition. + +Estimated cost: ~50 observations × ~40 chars ≈ 2K chars, against the ~81K post-R1 payload. +Weigh against recommendation R3 (knowledge selection ordering), which is reshaping the +same block — coordinate so the two do not conflict. + +## Acceptance criteria + +- [ ] Every observation rendered in `knowledgeDirectives` carries its exact + `observationId` (full, untruncated — `knowledge_observations` lookups are + `WHERE id = ?` exact matches) +- [ ] Multi-observation entities keep each id bound to its own observation +- [ ] A test asserts the rendered id is the exact string `confirm_knowledge` accepts +- [ ] A test asserts the payload growth stays within budget + +## References + +- `.claude/rules/60-request-io-and-bundle-budgets.md` — payload budgets +- R1 task: `tasks/archive/2026-08-23-remove-duplicated-structured-arrays-get-instructions.md` +- Token-optimization research: library `/engineering/research/token-optimization-research.md`