From 60db1be531d867e3298e722c73425a91081bdf11 Mon Sep 17 00:00:00 2001 From: Michal Date: Sun, 9 Aug 2026 23:36:53 +0100 Subject: [PATCH] feat(epictask): make the criterion-id convention an invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 175. Epic.doneCriteria and EpicTask.doneCriteria are both JSON blobs joined only by a free-form id, and nothing checked that a node's ids existed on its parent Epic. A typo produced a criterion belonging to nobody. The reconciliation added in 170 surfaces those as `unowned`, which is honest but late: the mistake was made at write time and is far cheaper to refuse there. Now create and update both reject unknown ids, naming every one of them and listing what the Epic actually has — or saying plainly that the Epic has no criteria yet, which is the likelier cause when a whole roadmap comes back empty. Enforced in the service, not the schema: production provisions with `prisma db push`, so a constraint would never exist where it matters. A node with no criteria of its own is still fine — not every roadmap node claims one — and an update that doesn't touch criteria doesn't pay for the check. This deliberately stops short of the EpicCriterion table with a real FK. That remains the stronger answer if criteria ever need to be addressed independently, but it costs a schema change plus migrating everything currently in the JSON blobs, and the invariant closes the actual failure mode today. Also files 177: agent chat --project reaches project prompts and MCP tools while checking only run:agents:. Raised by the agent that built membership, which correctly refused to invent the policy mid-task. Pre-existing behaviour for a legacy pin, but membership widens the blast radius considerably — it should be a deliberate decision with a test. Local: build clean, workspace 2557 passed (+7), lint unchanged at 794 errors. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01N4wNHWf7xSwnZCWpJcyv9p --- .taskmaster/tasks/tasks.json | 34 ++++++--- src/mcpd/src/services/epic-task.service.ts | 41 ++++++++++- .../tests/services/epic-task-service.test.ts | 73 +++++++++++++++++++ 3 files changed, 137 insertions(+), 11 deletions(-) diff --git a/.taskmaster/tasks/tasks.json b/.taskmaster/tasks/tasks.json index 239e6de..ff78915 100644 --- a/.taskmaster/tasks/tasks.json +++ b/.taskmaster/tasks/tasks.json @@ -3089,9 +3089,9 @@ "dependencies": [ "120" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-08T08:55:58.627Z" + "updatedAt": "2026-08-09T22:34:49.672Z" }, { "id": "153", @@ -3301,9 +3301,9 @@ "120", "152" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-09T19:36:34.052Z" + "updatedAt": "2026-08-09T22:34:49.682Z" }, { "id": "167", @@ -3426,9 +3426,9 @@ "dependencies": [ "170" ], - "status": "pending", + "status": "done", "subtasks": [], - "updatedAt": "2026-08-09T21:09:27.819Z" + "updatedAt": "2026-08-09T22:36:53.472Z" }, { "id": "176", @@ -3443,16 +3443,30 @@ "status": "pending", "subtasks": [], "updatedAt": "2026-08-09T21:09:27.819Z" + }, + { + "id": "177", + "title": "Agent chat --project reaches project tools with only run:agents", + "description": "`mcpctl chat --project

` surfaces that project prompts and MCP tools while checking only run:agents:. A caller with no run:projects:

reaches project tools through the agent.", + "details": "Raised by the agent that built AgentProject membership, which deliberately did NOT add a second check because this is already todays behaviour for an agent with a legacy projectId — adding one would have been new policy invented mid-task. Correct call; the policy decision is ours.\n\nWhat changed is the blast radius: membership makes this reachable for many more (agent, project) pairs than a single scalar pin ever could.\n\nOptions:\n- require run:projects:

in addition to run:agents: when --project is used (tightest; may break existing single-pin agents whose users were never granted the project);\n- require it only for membership-derived context, grandfathering the legacy pin;\n- leave as-is and document that agent membership delegates project tool access.\n\nWhichever way, it should be a deliberate decision with a test, not an accident of which check happened to be written first.", + "testStrategy": "See details.", + "priority": "high", + "dependencies": [ + "166" + ], + "status": "pending", + "subtasks": [], + "updatedAt": "2026-08-09T22:34:45.085Z" } ], "metadata": { "version": "1.0.0", - "lastModified": "2026-08-09T21:09:27.647Z", - "taskCount": 174, - "completedCount": 130, + "lastModified": "2026-08-09T22:36:53.472Z", + "taskCount": 177, + "completedCount": 133, "tags": [ "master" ] } } -} +} \ No newline at end of file diff --git a/src/mcpd/src/services/epic-task.service.ts b/src/mcpd/src/services/epic-task.service.ts index 667968b..0ee328f 100644 --- a/src/mcpd/src/services/epic-task.service.ts +++ b/src/mcpd/src/services/epic-task.service.ts @@ -72,6 +72,8 @@ export class EpicTaskService { ); } + this.assertCriteriaBelongToEpic(readCriteria(epic), data.doneCriteria, epic.name); + return this.repo.create({ name: data.name, epicId: data.epicId, @@ -85,7 +87,13 @@ export class EpicTaskService { async update(id: string, input: unknown): Promise { const data = UpdateEpicTaskSchema.parse(input); - await this.get(id); // 404 if missing + const node = await this.get(id); // 404 if missing + + if (data.doneCriteria !== undefined) { + const epic = await this.epicRepo.findById(node.epicId); + if (epic === null) throw new NotFoundError(`Epic not found: ${node.epicId}`); + this.assertCriteriaBelongToEpic(readCriteria(epic), data.doneCriteria, epic.name); + } return this.repo.update(id, { ...(data.description !== undefined ? { description: data.description } : {}), @@ -101,6 +109,37 @@ export class EpicTaskService { await this.repo.delete(id); } + /** + * A roadmap node may only claim criteria its parent Epic actually has. + * + * Epic.doneCriteria and EpicTask.doneCriteria are both JSON blobs joined by a + * free-form id, and until now nothing checked that the ids matched. A typo + * produced a criterion that belonged to nobody: the reconciliation reported + * it as `unowned`, which is honest but late — the mistake was made at write + * time and is far cheaper to refuse there. + * + * Enforced in the service, not the schema: production provisions with + * `prisma db push`, so a constraint here would never exist in production. + */ + private assertCriteriaBelongToEpic( + epicCriteria: DoneCriterion[], + nodeCriteria: DoneCriterion[] | undefined, + epicName: string, + ): void { + if (nodeCriteria === undefined || nodeCriteria.length === 0) return; + const known = new Set(epicCriteria.map((c) => c.id)); + const unknown = nodeCriteria.map((c) => c.id).filter((id) => !known.has(id)); + if (unknown.length === 0) return; + + const available = epicCriteria.map((c) => c.id); + throw badRequest( + `Epic '${epicName}' has no done-criterion ${unknown.map((u) => `'${u}'`).join(', ')}. ` + + (available.length > 0 + ? `Its criteria are: ${available.join(', ')}.` + : 'It has no criteria yet — add them to the Epic first.'), + ); + } + // ── Backup / restore helpers ── async upsertByName(data: Record, ownerId: string): Promise { diff --git a/src/mcpd/tests/services/epic-task-service.test.ts b/src/mcpd/tests/services/epic-task-service.test.ts index 585e926..0a7b520 100644 --- a/src/mcpd/tests/services/epic-task-service.test.ts +++ b/src/mcpd/tests/services/epic-task-service.test.ts @@ -248,3 +248,76 @@ describe('EpicTaskService CRUD', () => { } }); }); + +describe('EpicTaskService — criterion ids must belong to the parent Epic', () => { + beforeEach(() => { vi.clearAllMocks(); }); + + /** An epicRepo whose Epic carries the given criterion ids. */ + function withEpicCriteria(ids: string[]) { + const { service, epicRepo, repo } = makeGraph([{ id: 'a', name: 'chassis' }]); + (epicRepo.findById as ReturnType).mockResolvedValue({ + id: 'epic-1', + name: 'the-car', + doneCriteria: ids.map((id) => ({ id, text: id, kind: 'outcome', met: false })), + } as never); + return { service, repo }; + } + + it('accepts a node claiming a criterion the Epic has', async () => { + const { service } = withEpicCriteria(['wheels', 'engine']); + await expect( + service.create({ name: 'n1', epicId: 'epic-1', doneCriteria: [ + { id: 'wheels', text: 'has wheels', kind: 'outcome', met: false }, + ] }, 'user-1'), + ).resolves.toBeDefined(); + }); + + it('refuses a criterion the Epic never had, and lists what it does have', async () => { + // Previously this succeeded and the criterion silently belonged to nobody — + // surfacing only later as `unowned` in the reconciliation. + const { service } = withEpicCriteria(['wheels', 'engine']); + await expect( + service.create({ name: 'n1', epicId: 'epic-1', doneCriteria: [ + { id: 'whels', text: 'typo', kind: 'outcome', met: false }, + ] }, 'user-1'), + ).rejects.toThrow(/no done-criterion 'whels'.*wheels, engine/s); + }); + + it('names every unknown id, not just the first', async () => { + const { service } = withEpicCriteria(['wheels']); + await expect( + service.create({ name: 'n1', epicId: 'epic-1', doneCriteria: [ + { id: 'aaa', text: 'a', kind: 'outcome', met: false }, + { id: 'bbb', text: 'b', kind: 'outcome', met: false }, + ] }, 'user-1'), + ).rejects.toThrow(/'aaa', 'bbb'/); + }); + + it('says so plainly when the Epic has no criteria at all', async () => { + const { service } = withEpicCriteria([]); + await expect( + service.create({ name: 'n1', epicId: 'epic-1', doneCriteria: [ + { id: 'x', text: 'x', kind: 'outcome', met: false }, + ] }, 'user-1'), + ).rejects.toThrow(/no criteria yet — add them to the Epic first/); + }); + + it('allows a node with no criteria of its own', async () => { + // Not every roadmap node claims a criterion; that is not an error. + const { service } = withEpicCriteria(['wheels']); + await expect(service.create({ name: 'n1', epicId: 'epic-1' }, 'user-1')).resolves.toBeDefined(); + }); + + it('enforces the same rule on update', async () => { + const { service } = withEpicCriteria(['wheels']); + await expect( + service.update('a', { doneCriteria: [{ id: 'nope', text: 'n', kind: 'outcome', met: false }] }), + ).rejects.toThrow(/no done-criterion 'nope'/); + }); + + it('does not re-check when an update leaves criteria alone', async () => { + const { service, repo } = withEpicCriteria(['wheels']); + await expect(service.update('a', { status: 'done' })).resolves.toBeDefined(); + expect(repo.update).toHaveBeenCalled(); + }); +});