feat(epictask): make the criterion-id convention an invariant

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:<name>. 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4wNHWf7xSwnZCWpJcyv9p
This commit is contained in:
Michal
2026-08-09 23:36:53 +01:00
parent 4515eb4517
commit 60db1be531
3 changed files with 137 additions and 11 deletions

View File

@@ -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 <agent> --project <p>` surfaces that project prompts and MCP tools while checking only run:agents:<name>. A caller with no run:projects:<p> 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:<p> in addition to run:agents:<name> 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"
]
}
}
}
}

View File

@@ -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<EpicTask> {
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<string, unknown>, ownerId: string): Promise<EpicTask> {

View File

@@ -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<typeof vi.fn>).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();
});
});