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(); + }); +});