diff --git a/backend/__tests__/service/migrate-task-source-ref-identity.test.js b/backend/__tests__/service/migrate-task-source-ref-identity.test.js new file mode 100644 index 000000000..0b9c81347 --- /dev/null +++ b/backend/__tests__/service/migrate-task-source-ref-identity.test.js @@ -0,0 +1,113 @@ +process.env.PG_HOST = ''; + +const Task = require('../../models/Task'); +const { + migrateTaskSourceRefIdentity, +} = require('../../scripts/migrate-task-source-ref-identity'); +const { + setupMongoDb, + closeMongoDb, + clearMongoDb, +} = require('../utils/testUtils'); + +const LEGACY_INDEX = 'podId_1_sourceRef_1_partial'; +const PAIR_INDEX = 'podId_1_sourceRef_1_title_1_partial'; + +// TASK-063's migration is load-bearing, not housekeeping: autoIndex CREATES the +// declared pair index but never DROPS the legacy ref-only index, and the legacy +// index is STRICTER — while it exists, a second ask under one sourceRef cannot +// be inserted at all (the route answers that with a named 503). So this +// exercises the drop/create against a real mongod rather than trusting the +// script's shape. +describe('migrate-task-source-ref-identity', () => { + beforeAll(async () => { + await setupMongoDb(); + }); + + afterAll(async () => { + await clearMongoDb(); + await closeMongoDb(); + }); + + const indexNames = async () => { + try { + return (await Task.collection.indexes()).map((i) => String(i.name)); + } catch (error) { + // A database that has never held a task has no `tasks` collection to list + // indexes from, and mongod answers NamespaceNotFound (code 26). + if (error.code === 26) return []; + throw error; + } + }; + + const seedLegacyIndex = async () => { + // Reproduce a pre-TASK-063 database: the declared pair index absent, the + // ref-only index present. + const names = await indexNames(); + if (names.includes(PAIR_INDEX)) await Task.collection.dropIndex(PAIR_INDEX); + if (!names.includes(LEGACY_INDEX)) { + await Task.collection.createIndex( + { podId: 1, sourceRef: 1 }, + { unique: true, name: LEGACY_INDEX, partialFilterExpression: { sourceRef: { $type: 'string' } } }, + ); + } + }; + + beforeEach(async () => { + await clearMongoDb(); + await seedLegacyIndex(); + }); + + it('reports the legacy index and changes nothing on a dry run', async () => { + const result = await migrateTaskSourceRefIdentity({ dryRun: true }); + + expect(result).toMatchObject({ + dryRun: true, + legacyIndexPresent: true, + pairIndexPresentBefore: false, + pairIndexPresentAfter: false, + droppedLegacy: false, + }); + const names = await indexNames(); + expect(names).toContain(LEGACY_INDEX); + expect(names).not.toContain(PAIR_INDEX); + }); + + it('drops the legacy index and creates the pair index', async () => { + const result = await migrateTaskSourceRefIdentity(); + + expect(result).toMatchObject({ + dryRun: false, + legacyIndexPresent: true, + pairIndexPresentBefore: false, + pairIndexPresentAfter: true, + droppedLegacy: true, + }); + const names = await indexNames(); + expect(names).not.toContain(LEGACY_INDEX); + expect(names).toContain(PAIR_INDEX); + }); + + it('is idempotent, and the pair index is what a second ask needs', async () => { + await migrateTaskSourceRefIdentity(); + const second = await migrateTaskSourceRefIdentity(); + expect(second).toMatchObject({ + legacyIndexPresent: false, + pairIndexPresentBefore: true, + pairIndexPresentAfter: true, + droppedLegacy: false, + }); + + const podId = new (require('mongoose').Types.ObjectId)(); + const base = { + podId, + source: 'import', + sourceRef: 'external:ticket:697', + updates: [], + }; + await Task.create({ ...base, taskNum: 1, taskId: 'TASK-001', title: 'First ask' }); + await Task.create({ ...base, taskNum: 2, taskId: 'TASK-002', title: 'Second ask' }); + + expect(await Task.countDocuments({ podId, sourceRef: 'external:ticket:697' })).toBe(2); + }); +}); diff --git a/backend/__tests__/service/tasks.source-ref-idempotency.test.js b/backend/__tests__/service/tasks.source-ref-idempotency.test.js index 7e80eebc2..ceaa72381 100644 --- a/backend/__tests__/service/tasks.source-ref-idempotency.test.js +++ b/backend/__tests__/service/tasks.source-ref-idempotency.test.js @@ -14,6 +14,11 @@ const { generateTestToken, } = require('../utils/testUtils'); +// TASK-063. A create's identity is the (sourceRef, title) PAIR. Everything in +// this file follows from that one sentence: the same pair is an idempotent +// retry (200, nothing mutated), a different title under the same ref is a +// different ask (201, its own row), and a completed row can only be reopened by +// the ask that completed it. describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { let app; let owner; @@ -75,51 +80,139 @@ describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { const existing = await seedTask(); const response = await postTask({ - title: 'Retry of existing task', + title: 'Existing task', sourceRef: 'external:ticket:697', }).expect(200); expect(response.body.alreadyExists).toBe(true); + expect(response.body.reopened).toBeUndefined(); expect(response.body.task._id).toBe(String(existing._id)); expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(1); }); + it('creates a second row when one sourceRef carries a different ask', async () => { + // The 58290 incident: one message raised two asks with two owners, the + // filer used the message id as the ref for both, and the second ask came + // back holding the first ask's row with its own title discarded. + const first = await seedTask(); + + const response = await postTask({ + title: 'Second ask from the same source', + sourceRef: 'external:ticket:697', + assignee: 'pod-architect', + }).expect(201); + + expect(response.body.task._id).not.toBe(String(first._id)); + expect(response.body.task.title).toBe('Second ask from the same source'); + expect(response.body.task.assignee).toBe('pod-architect'); + expect(response.body.alreadyExists).toBeUndefined(); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(2); + + const reloadedFirst = await Task.findById(first._id); + expect(reloadedFirst.title).toBe('Existing task'); + expect(reloadedFirst.status).toBe('pending'); + }); + it('keeps the pre-check reopen behavior for a completed task', async () => { const existing = await seedTask({ status: 'done', completedAt: new Date() }); const response = await postTask({ - title: 'Still-active source record', + title: 'Existing task', sourceRef: 'external:ticket:697', assignee: 'codex', }).expect(200); + // `alreadyExists: true` because it does: this request did not create a row, + // it reopened one. It used to answer `false`, and a caller branches on that + // field to decide whether its create landed, so the caller concluded it had + // created a task while holding a different task's row. expect(response.body).toMatchObject({ - alreadyExists: false, + alreadyExists: true, reopened: true, task: { _id: String(existing._id), status: 'pending', assignee: 'codex', + title: 'Existing task', }, }); expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(1); }); - it('preserves the completed run\'s notes in history and reports a differing submitted title', async () => { + it('clears the assignee when a same-pair reopen is submitted without one', async () => { + // The gap sprint-review measured (73815, 2026-09-25): every other reopen + // case passes an assignee, so the half the tool description warns about was + // documented in three places and pinned nowhere. `existing.assignee = + // assignee || undefined` (tasksApi.ts:290) means an OMITTED assignee clears + // the row's — a later change that made reopen preserve it would leave the + // tool description, task-board.mdx and guides.json all wrong with the rest + // of this file green. const existing = await seedTask({ status: 'done', completedAt: new Date(), - notes: 'Shipped as PR #1, squash-merged. Patch-id verified against the gated head.', + assignee: 'ux-lead', + }); + // Positive control: the field we assert cleared is genuinely set before the + // request, so a pass cannot come from the seed never carrying it. + expect((await Task.findById(existing._id)).assignee).toBe('ux-lead'); + + const response = await postTask({ + title: 'Existing task', + sourceRef: 'external:ticket:697', + }).expect(200); + + expect(response.body).toMatchObject({ alreadyExists: true, reopened: true }); + // The route assigns `undefined`, Mongoose `$unset`s the path, and hydration + // hands the String path back as its null default — so the two readers + // disagree on the sentinel (undefined in the response's in-memory doc, null + // on reload) while agreeing on the fact. Assert the fact, both ways round: + // the old assignee is gone, on the response AND in the store. + const reloaded = await Task.findById(existing._id); + expect([null, undefined]).toContain(response.body.task.assignee); + expect([null, undefined]).toContain(reloaded.assignee); + }); + + it('does not reopen a completed row when the submitted title differs', async () => { + // TASK-163, reproduced live 2026-09-25T08:09Z: a create with a done row's + // ref put that row back to pending under a title its caller never chose, + // and the caller's own ask was never filed. + const completed = await seedTask({ + status: 'done', + completedAt: new Date(), + notes: 'Shipped as PR #1169, squash-merged. Patch-id verified against the gated head.', }); + const updatesBefore = (await Task.findById(completed._id)).updates.length; const response = await postTask({ title: 'A follow-up filed against the same source', sourceRef: 'external:ticket:697', + }).expect(201); + + expect(response.body.task.title).toBe('A follow-up filed against the same source'); + expect(response.body.task._id).not.toBe(String(completed._id)); + + const reloaded = await Task.findById(completed._id); + expect(reloaded.status).toBe('done'); + expect(reloaded.notes).toBe('Shipped as PR #1169, squash-merged. Patch-id verified against the gated head.'); + expect(reloaded.updates.length).toBe(updatesBefore); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(2); + }); + + it('preserves the completed run\'s notes in history when a reopen does fire', async () => { + const existing = await seedTask({ + status: 'done', + completedAt: new Date(), + notes: 'Shipped as PR #1, squash-merged. Patch-id verified against the gated head.', + }); + + const response = await postTask({ + title: 'Existing task', + sourceRef: 'external:ticket:697', }).expect(200); expect(response.body.reopened).toBe(true); expect(response.body.task.title).toBe('Existing task'); - expect(response.body.task.notes).toContain('Submitted title ("A follow-up filed against the same source") was NOT applied'); + expect(response.body.task.notes).toBe('Reopened — the same source is active again.'); const reloaded = await Task.findById(existing._id); const history = reloaded.updates.map((u) => u.text).join('\n---\n'); @@ -127,7 +220,6 @@ describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { // used to destroy them by overwriting `notes` with one sentence. expect(history).toContain('Previous notes preserved from the completed run'); expect(history).toContain('Patch-id verified against the gated head.'); - expect(history).toContain('Submitted title differed and was not applied'); }); it('adds no conservation noise when the reopen changes nothing but the status', async () => { @@ -143,7 +235,6 @@ describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { const reloaded = await Task.findById(existing._id); const history = reloaded.updates.map((u) => u.text).join('\n---\n'); expect(history).not.toContain('Previous notes preserved'); - expect(history).not.toContain('Submitted title differed'); }); it('reconciles a sourceRef E11000 race as an idempotent 200', async () => { @@ -151,8 +242,11 @@ describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { const findOneSpy = jest.spyOn(Task, 'findOne'); findOneSpy.mockImplementationOnce(() => Promise.resolve(null)); + // Same ref AND same title: the insert really does collide on the pair + // index, so this exercises the E11000 classification against a live index + // rather than a mock of one. const response = await postTask({ - title: 'Concurrent retry', + title: 'Existing task', sourceRef: 'external:ticket:697', }).expect(200); @@ -161,6 +255,29 @@ describe('POST /api/v1/tasks/:podId sourceRef idempotency', () => { expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(1); }); + it('answers a named 503 when the database still carries the ref-only index', async () => { + // Deploy-order guard: with the pre-TASK-063 index still in place, a second + // title under one ref collides there, so the create the caller asked for is + // not one this database can perform. Fail with the migration's name rather + // than a generic 500. + await seedTask({ status: 'done', completedAt: new Date() }); + jest.spyOn(Task, 'findOne').mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(Task, 'create').mockRejectedValueOnce(Object.assign( + new Error('E11000 duplicate key error collection: commonly.tasks index: podId_1_sourceRef_1_partial dup key'), + { code: 11000, keyPattern: { podId: 1, sourceRef: 1 } }, + )); + jest.spyOn(console, 'error').mockImplementation(() => {}); + + const response = await postTask({ + title: 'A second ask, on a legacy index', + sourceRef: 'external:ticket:697', + }).expect(503); + + expect(response.body.code).toBe('task_source_ref_index_migration_pending'); + expect(response.body.error).toContain('migrate:task-source-ref-identity'); + expect(await Task.countDocuments({ podId: pod._id, sourceRef: 'external:ticket:697' })).toBe(1); + }); + it('does not misclassify a taskId E11000 as sourceRef idempotency', async () => { await seedTask({ sourceRef: 'external:ticket:different-source' }); const findOneSpy = jest.spyOn(Task, 'findOne'); diff --git a/backend/__tests__/unit/models/Task.sourceRefIndex.test.js b/backend/__tests__/unit/models/Task.sourceRefIndex.test.js new file mode 100644 index 000000000..49f20e2c8 --- /dev/null +++ b/backend/__tests__/unit/models/Task.sourceRefIndex.test.js @@ -0,0 +1,40 @@ +const Task = require('../../../models/Task'); + +// TASK-063. `sourceRef` is PROVENANCE, not identity: one source — a message, a +// PR — can raise several asks with several owners. The unique key is therefore +// the (sourceRef, title) PAIR. The ref-only index that preceded it made the +// second ask adopt the first ask's row: it discarded the caller's title and +// reopened whatever row that ref already belonged to, twice in production +// (TASK-052, TASK-163), the second time on a row a merged PR had completed. +// +// This asserts the DECLARATION, because the declaration is the whole guarantee. +// No runtime branch can notice if someone relaxes the key back to sourceRef +// alone — the route's pre-check and the index would silently agree with each +// other again, which is exactly the state that produced the incident. The +// partial filter is pinned because `sparse` on a compound index indexes every +// doc that has podId (i.e. all of them) and would E11000 the second +// sourceRef-less task in a pod; the name is pinned because +// scripts/migrate-task-source-ref-identity.ts drops the legacy index by name. +describe('Task model — sourceRef identity index', () => { + test('uniquely keys (podId, sourceRef, title) in that order', () => { + const pairIndex = Task.schema.indexes().find(([fields]) => ( + fields.podId === 1 && fields.sourceRef === 1 && fields.title === 1 + )); + + expect(pairIndex).toBeDefined(); + expect(Object.keys(pairIndex[0])).toEqual(['podId', 'sourceRef', 'title']); + expect(pairIndex[1]).toMatchObject({ + unique: true, + name: 'podId_1_sourceRef_1_title_1_partial', + partialFilterExpression: { sourceRef: { $type: 'string' } }, + }); + }); + + test('declares no ref-only unique index', () => { + const refOnly = Task.schema.indexes().find(([fields]) => ( + fields.sourceRef === 1 && fields.title === undefined + )); + + expect(refOnly).toBeUndefined(); + }); +}); diff --git a/backend/controllers/authController.ts b/backend/controllers/authController.ts index deaccaf19..a185678ab 100644 --- a/backend/controllers/authController.ts +++ b/backend/controllers/authController.ts @@ -203,9 +203,10 @@ const finishWorkspaceOnboarding = async (pod: any, userId: any) => { notes: 'Workspaces are better shared — humans and agents in the same room, one project memory. Use the pod invite link from the inspector panel.', }, ]; - // Distinct sourceRefs give each seed a stable identity under the - // unique (podId, sourceRef) partial index — re-running the seeding - // for a pod can never silently duplicate the checklist. + // Distinct sourceRefs — each with a fixed title — give every seed a + // stable identity under the unique (podId, sourceRef, title) partial + // index, so re-running the seeding for a pod cannot silently duplicate + // the checklist. await Task.create(starter.map((t, i) => ({ podId: pod._id, taskNum: i + 1, diff --git a/backend/models/Task.ts b/backend/models/Task.ts index ddc0373c6..2b02e4bd6 100644 --- a/backend/models/Task.ts +++ b/backend/models/Task.ts @@ -90,11 +90,25 @@ TaskSchema.index({ podId: 1, taskId: 1 }, { unique: true }); // index (name included) — this declaration matches it so boot-time // autoIndex neither conflicts nor recreates the broken sparse variant on // fresh installs. +// +// `sourceRef` is PROVENANCE, not identity (TASK-063). One source — a message, +// a PR — can raise several asks with several owners, and keying on the ref +// alone made the second ask adopt the first ask's row: it discarded the +// caller's title and reopened that row instead, twice in production (TASK-052, +// TASK-163), the second time on a row a merged PR had legitimately completed. +// The identity of a create is therefore the (sourceRef, title) PAIR — a retry +// of the same ask stays idempotent, a different title becomes its own row. +// +// The legacy index cannot enforce that, and autoIndex CREATES the declared +// index but never DROPS an undeclared one, so an existing database keeps the +// stricter ref-only index until scripts/migrate-task-source-ref-identity.ts +// runs. Until then the route answers a second title under one ref with a named +// 503 rather than a silent wrong row. TaskSchema.index( - { podId: 1, sourceRef: 1 }, + { podId: 1, sourceRef: 1, title: 1 }, { unique: true, - name: 'podId_1_sourceRef_1_partial', + name: 'podId_1_sourceRef_1_title_1_partial', partialFilterExpression: { sourceRef: { $type: 'string' } }, }, ); diff --git a/backend/package.json b/backend/package.json index 924feac9d..e1490b925 100644 --- a/backend/package.json +++ b/backend/package.json @@ -22,6 +22,7 @@ "discord:list": "node scripts/register-discord-commands.js list", "bootstrap:clawd-bot": "node scripts/bootstrap-clawd-bot.js", "backfill:attention-items": "ts-node scripts/backfill-attention-items.ts", + "migrate:task-source-ref-identity": "ts-node scripts/migrate-task-source-ref-identity.ts", "sweep:resolved-mention-attention": "ts-node scripts/sweep-resolved-mention-attention.ts", "sweep:done-task-handoffs": "ts-node scripts/sweep-done-task-handoffs.ts", "sweep:leftover-runtime-tokens": "ts-node scripts/sweep-leftover-runtime-tokens.ts", diff --git a/backend/routes/tasksApi.ts b/backend/routes/tasksApi.ts index 2e828710c..4132cacce 100644 --- a/backend/routes/tasksApi.ts +++ b/backend/routes/tasksApi.ts @@ -257,25 +257,28 @@ router.post('/:podId', rateLimit({ const access = await requirePodMember(podId || '', userId, { write: true }); if (access.error) return res.status(access.status || 500).json({ error: access.error }); if (sourceRef) { - const existing = await Task.findOne({ podId: mongoose.Types.ObjectId.createFromHexString(podId || ''), sourceRef }) as { title?: string; status?: string; assignee?: string; claimedAt?: Date | null; claimExpiresAt?: Date | null; notes?: string; updates: Array<{ text: string; author: string; authorId: string | null; createdAt: Date }>; save: () => Promise; toObject: () => unknown } | null; + // A create's identity is the (sourceRef, title) PAIR: the ref says where + // the ask came from, the title says which ask it is. Keying on the ref + // alone made a second ask from one source adopt the first ask's row and + // discard the caller's title (TASK-063). Same pair = idempotent retry. + const existing = await Task.findOne({ podId: mongoose.Types.ObjectId.createFromHexString(podId || ''), sourceRef, title }) as { title?: string; status?: string; assignee?: string; claimedAt?: Date | null; claimExpiresAt?: Date | null; notes?: string; updates: Array<{ text: string; author: string; authorId: string | null; createdAt: Date }>; save: () => Promise; toObject: () => unknown } | null; if (existing) { if (existing.status === 'done') { - // `sourceRef` is an idempotency key (a unique partial index backs it), - // so a settled row with the same ref is reopened in place rather than - // duplicated. Two things this branch deliberately does NOT do: + // `sourceRef` + `title` are an idempotency key (a unique partial + // index backs the pair), so a settled row with the same ref AND the + // same title is reopened in place rather than duplicated — the same + // source is active again. // - // 1. It does not apply the submitted title. Callers use `sourceRef` for - // provenance, so a differing title here is a follow-up being filed - // against the same source, not a rename of this row — but that - // difference IS reported, in `notes` and in the history below, - // because a caller who sent a title and got back a row with another - // one has no other way to notice (AX entry 59). - // 2. It does not discard the previous `notes`. They are moved into the - // append-only `updates` history before the reopen note replaces them; - // a reopen used to overwrite a completed row's writeup with one - // sentence, losing the only durable record of that work. + // It deliberately does NOT discard the previous `notes`. They are + // moved into the append-only `updates` history before the reopen note + // replaces them; a reopen used to overwrite a completed row's writeup + // with one sentence, losing the only durable record of that work. + // + // A differing title cannot reach this branch: it is a different ask, + // and the pre-check above does not match it. It used to reopen here + // and report the mismatch, which put a completed row back to pending + // under a title its caller never chose (TASK-063). const previousNotes = (existing.notes || '').trim(); - const titleDiffers = !!title && title !== existing.title; if (previousNotes) { existing.updates.push({ text: `Previous notes preserved from the completed run:\n${previousNotes}`, @@ -288,13 +291,9 @@ router.post('/:podId', rateLimit({ existing.assignee = assignee || undefined; existing.claimedAt = null; existing.claimExpiresAt = null; - existing.notes = titleDiffers - ? `Reopened — the same source is active again. Submitted title ("${title}") was NOT applied; this row keeps its original title.` - : 'Reopened — the same source is active again.'; + existing.notes = 'Reopened — the same source is active again.'; existing.updates.push({ - text: titleDiffers - ? `Reopened: task was done but its source is active again — picking up again. Submitted title differed and was not applied: "${title}"` - : 'Reopened: task was done but its source is active again — picking up again.', + text: 'Reopened: task was done but its source is active again — picking up again.', author: 'system', authorId: null, createdAt: new Date(), @@ -303,7 +302,13 @@ router.post('/:podId', rateLimit({ const reopenedObj = existing.toObject(); emitTaskUpdated(podId, reopenedObj, 'updated'); notifyAgents(req, podId, reopenedObj, 'updated'); - return res.json({ task: reopenedObj, alreadyExists: false, reopened: true }); + // `alreadyExists: true`, because it does: this request did not create + // a row, it reopened one. It used to answer `false`, and + // `alreadyExists` is the field a caller branches on to decide whether + // its create landed — so a caller concluded it had created a task + // while holding a different task's row (TASK-063). `reopened` carries + // what happened instead. + return res.json({ task: reopenedObj, alreadyExists: true, reopened: true }); } return res.json({ task: existing.toObject(), alreadyExists: true }); } @@ -329,16 +334,35 @@ router.post('/:podId', rateLimit({ keyPattern?: Record; message?: string; }; + // A database still carrying the pre-TASK-063 ref-only index cannot honour + // the (sourceRef, title) key: a second title under one ref collides there, + // so the create the caller asked for is not the create this database can + // perform. Name the migration instead of returning a generic 500 — the + // operator response is a command, not a code hunt. + const legacySourceRefIndex = duplicate.code === 11000 + && !!sourceRef + && duplicate.keyPattern?.podId === 1 + && duplicate.keyPattern?.sourceRef === 1 + && duplicate.keyPattern?.title === undefined + && duplicate.keyPattern?.taskId === undefined; const sourceRefIndexCollision = duplicate.code === 11000 && !!sourceRef && ( ( duplicate.keyPattern?.podId === 1 && duplicate.keyPattern?.sourceRef === 1 + && duplicate.keyPattern?.title === 1 && duplicate.keyPattern?.taskId === undefined ) - || duplicate.message?.includes('podId_1_sourceRef_1_partial') + || duplicate.message?.includes('podId_1_sourceRef_1_title_1_partial') ); + if (legacySourceRefIndex) { + console.error('POST /tasks: tasks index is still podId_1_sourceRef_1_partial — run `npm run migrate:task-source-ref-identity`'); + return res.status(503).json({ + error: 'task sourceRef index migration pending: run `npm run migrate:task-source-ref-identity`', + code: 'task_source_ref_index_migration_pending', + }); + } if (!sourceRefIndexCollision) throw createErr; // The pre-check can lose a race to another request. Re-read the winner @@ -348,6 +372,7 @@ router.post('/:podId', rateLimit({ const existing = await Task.findOne({ podId: mongoose.Types.ObjectId.createFromHexString(podId || ''), sourceRef, + title, }) as { toObject: () => unknown } | null; if (!existing) throw createErr; return res.json({ task: existing.toObject(), alreadyExists: true }); diff --git a/backend/scripts/migrate-task-source-ref-identity.ts b/backend/scripts/migrate-task-source-ref-identity.ts new file mode 100644 index 000000000..85e204377 --- /dev/null +++ b/backend/scripts/migrate-task-source-ref-identity.ts @@ -0,0 +1,108 @@ +#!/usr/bin/env node +/* + * TASK-063: replace the sourceRef-only unique index with the + * (podId, sourceRef, title) pair index that models/Task.ts declares. + * + * Why this has to be RUN rather than left to the boot-time autoIndex: mongoose + * CREATES the indexes a schema declares but never DROPS one it does not know + * about, and the ref-only index is STRICTER than the pair. While it exists, a + * second ask that shares a source with an existing task cannot be inserted at + * all — so the route answers that case with a named 503 + * (`task_source_ref_index_migration_pending`) instead of a silent wrong row, + * and the gap is loud rather than quiet. + * + * Ordering is safe either way: before the deploy it creates an index the + * deployed code never consults; after the deploy it only shortens the window in + * which a mismatched-title create is refused. + * + * Usage (from backend/): + * npm run migrate:task-source-ref-identity -- --dry + * npm run migrate:task-source-ref-identity + */ + +import mongoose from 'mongoose'; +import Task from '../models/Task'; + +const LEGACY_INDEX = 'podId_1_sourceRef_1_partial'; +const PAIR_INDEX = 'podId_1_sourceRef_1_title_1_partial'; + +export interface TaskSourceRefIdentityResult { + dryRun: boolean; + legacyIndexPresent: boolean; + pairIndexPresentBefore: boolean; + pairIndexPresentAfter: boolean; + droppedLegacy: boolean; +} + +async function indexNames(): Promise { + try { + const indexes = await Task.collection.indexes(); + return indexes.map((index) => String(index.name)); + } catch (error) { + // A database with no `tasks` collection yet has no indexes to report, and + // mongod answers NamespaceNotFound (code 26) rather than an empty list. + // Found by the migration's own test, whose first run is on an empty DB. + if ((error as { code?: number }).code === 26) return []; + throw error; + } +} + +export async function migrateTaskSourceRefIdentity( + options: { dryRun?: boolean } = {}, +): Promise { + const dryRun = options.dryRun === true; + const names = await indexNames(); + const result: TaskSourceRefIdentityResult = { + dryRun, + legacyIndexPresent: names.includes(LEGACY_INDEX), + pairIndexPresentBefore: names.includes(PAIR_INDEX), + pairIndexPresentAfter: names.includes(PAIR_INDEX), + droppedLegacy: false, + }; + if (dryRun) return result; + + if (result.legacyIndexPresent) { + await Task.collection.dropIndex(LEGACY_INDEX); + result.droppedLegacy = true; + } + // createIndexes, not syncIndexes: make sure the declared pair index exists + // without dropping any index this schema does not know about. The pair is a + // strict relaxation of the ref-only index, so no existing document can + // violate it. + await Task.createIndexes(); + result.pairIndexPresentAfter = (await indexNames()).includes(PAIR_INDEX); + return result; +} + +async function main(): Promise { + const dryRun = process.argv.includes('--dry'); + const mongoUri = process.env.MONGO_URI; + if (!mongoUri) { + console.error('MONGO_URI is required'); + process.exit(1); + } + await mongoose.connect(mongoUri); + try { + const result = await migrateTaskSourceRefIdentity({ dryRun }); + console.log( + `[migrate-task-source-ref-identity] ${dryRun ? 'DRY-RUN (no changes written) ' : ''}` + + `legacyIndexPresent=${result.legacyIndexPresent} ` + + `pairIndexPresentBefore=${result.pairIndexPresentBefore} ` + + `pairIndexPresentAfter=${result.pairIndexPresentAfter} ` + + `droppedLegacy=${result.droppedLegacy}`, + ); + if (!dryRun && !result.pairIndexPresentAfter) { + console.error(`[migrate-task-source-ref-identity] ${PAIR_INDEX} is still missing after createIndexes()`); + process.exitCode = 1; + } + } finally { + await mongoose.disconnect(); + } +} + +if (require.main === module) { + main().catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/commonly-mcp/__tests__/tools.test.mjs b/commonly-mcp/__tests__/tools.test.mjs index 9377db4a1..e749d6ed7 100644 --- a/commonly-mcp/__tests__/tools.test.mjs +++ b/commonly-mcp/__tests__/tools.test.mjs @@ -231,6 +231,17 @@ describe('commonly_get_tasks / create / claim / complete / update', () => { expect(url).toContain('status=pending'); }); + it('create_task documents the idempotency key, not just the fields it dedups on', () => { + // AX entry 70: `sourceRef` was an idempotency key the description never + // named, so callers read it as ordinary metadata and learned about the + // dedup from a self-contradictory response instead (TASK-063). + const desc = byName.commonly_create_task.description; + expect(desc).toMatch(/sourceRef.*title.*idempotency key/i); + expect(desc).toMatch(/different `title` is a different ask/i); + expect(desc).toMatch(/reopened to pending with `reopened: true`/); + expect(desc).toMatch(/omitting it leaves the reopened task unassigned/); + }); + it('create_task POSTs with the body fields verbatim', async () => { const fetchSpy = installFetch(async () => okResponse({ taskId: 'TASK-001' })); await byName.commonly_create_task.call({ diff --git a/commonly-mcp/package.json b/commonly-mcp/package.json index e9a46f035..424031558 100644 --- a/commonly-mcp/package.json +++ b/commonly-mcp/package.json @@ -1,6 +1,6 @@ { "name": "@commonlyai/mcp", - "version": "0.3.12", + "version": "0.3.13", "license": "Apache-2.0", "repository": { "type": "git", diff --git a/commonly-mcp/src/tools.js b/commonly-mcp/src/tools.js index f18163b2f..a36443200 100644 --- a/commonly-mcp/src/tools.js +++ b/commonly-mcp/src/tools.js @@ -337,7 +337,7 @@ export const buildTools = (config) => { }, { name: 'commonly_create_task', - description: 'Create a task in the pod task board. `dep` is a blocking dependency taskId; `parentTask` is hierarchical.', + description: 'Create a task in the pod task board. `dep` is a blocking dependency taskId; `parentTask` is hierarchical. `sourceRef` + `title` are an idempotency key: re-sending the same pair returns the existing task instead of creating a second one, and if that task is done it is reopened to pending with `reopened: true` — its assignee becomes whatever `assignee` this call passed, so omitting it leaves the reopened task unassigned. The same `sourceRef` with a different `title` is a different ask and gets its own task.', inputSchema: reqWith({ podId: STRING, title: STRING, diff --git a/docs-site/concepts/task-board.mdx b/docs-site/concepts/task-board.mdx index d9143869a..8c3885418 100644 --- a/docs-site/concepts/task-board.mdx +++ b/docs-site/concepts/task-board.mdx @@ -42,7 +42,7 @@ commonly_complete_task(podId, taskId, { prUrl: "https://github.com/..." }) | `description` | string | Full description with context | | `status` | enum | `pending` \| `claimed` \| `blocked` \| `done` | | `assignee` | string | Agent ID or username | -| `sourceRef` | string | External deduplication key — one task per source record | +| `sourceRef` | string | External deduplication key — one task per (source record, title) | | `prUrl` | string | PR URL set on completion | | `updates` | array | Activity timeline with notes | | `parentTask` | string | Parent task ID (for sub-tasks) | @@ -63,7 +63,7 @@ curl -X POST https://api.commonly.me/api/v1/tasks/:podId \ }' ``` -If a task with the same `sourceRef` already exists, the API returns `{ task, alreadyExists: true }` — safe to call repeatedly. +If a task with the same `sourceRef` and `title` already exists, the API returns `{ task, alreadyExists: true }` — safe to call repeatedly. If that task is `done`, it is reopened to `pending` and the response also carries `reopened: true`; its assignee is set from the request, so a call that omits `assignee` leaves the reopened task unassigned. The same `sourceRef` with a different `title` is a different ask and creates its own task, so one source record can raise several tasks. ## Sub-tasks diff --git a/docs/development/agent-experience-audit.md b/docs/development/agent-experience-audit.md index 622564db1..4a38e52ae 100644 --- a/docs/development/agent-experience-audit.md +++ b/docs/development/agent-experience-audit.md @@ -3988,3 +3988,13 @@ The name helped the error along: `isMessageId` reads like *the* id predicate whe Two functions named `isPodMember` lived in the backend with opposite rules. The util (`utils/isPodMember.ts`) returned true on `pod.createdBy` before looking at `members`; a local copy in `server.ts` (`:503`) read `pod.members` only. Having just spent a row on the util's creator bypass, I read `isPodMember(pod, socket.userId)` at `server.ts:536` as the util and filed "a creator who left can still post over the socket" as a live write-path gap, and repeated it to the operator. The call bound the file's own strict copy three dozen lines up; the socket path had refused a departed creator all along. The name was doing the reasoning, and it pointed at the definition I already had in my head. **Repair:** before asserting what a call does, resolve the binding in that file (a local `const`, an import, a re-export) and read the body it reaches, not the body its name reminds you of. And when two definitions share a name with different semantics, delete one or rename it, then write the entry: a name that means two things in one codebase will keep producing confident wrong claims until one of the meanings is gone. + +## 70. An idempotency key that the tool description never names is read as ordinary metadata (2026-09-25, sprint-impl) + +*Origin observation: TASK-063, filed by @pod-architect on 2026-08-25 and reproduced live by @ux-lead at 2026-09-25T08:09Z on TASK-163; repaired by keying the pair.* + +`commonly_create_task` listed `sourceRef` as one more optional string. Nothing on the tool surface said it was an idempotency key, so the natural reading — a source is where the ask came from, and each ask gets its own row — was the wrong one, and the failure arrived as a **success**: the route deduped on the ref alone and answered `alreadyExists: false` next to a *different* task's row, discarding the caller's title, and on a settled row it reopened that row to `pending` instead. Two seats concluded they had created tasks they had not; one lost a completed row for 28 seconds and had to re-complete it. The response was self-contradictory, but a caller that reads `alreadyExists` and the returned `_id` — the two fields the shape invites it to branch on — never sees the contradiction. + +The second half is that the only place the key was stated anywhere a caller could find it was prose **outside the tool**: the guides promised "the same source reference already has a task → returns the existing task". A guarantee that lives in the guide and not in the tool description is not a contract the caller has, because agents read the tool list. + +**Repair:** when a parameter changes what a call *means* rather than only what it carries, name that in the tool description where the tool is listed — and never let a response contradict itself on the field callers branch on (`alreadyExists: true` on a reopen, because it does exist; `reopened` carries what happened). A composite key states both halves in the description. The guides and `docs-site` were corrected in the same change, since a stale promise is how the next caller arrives confidently wrong. diff --git a/frontend/src/content/guides.json b/frontend/src/content/guides.json index 01cc954ba..a12634748 100644 --- a/frontend/src/content/guides.json +++ b/frontend/src/content/guides.json @@ -3280,7 +3280,7 @@ "Use parent/child work when a larger outcome can be divided into separate deliverables with a clear merge point. Commonly tasks support a parentTask relationship and a dep field for a blocking dependency, so the team can show which pieces belong together and what must finish first.", "For example, a parent task might be “Prepare an integration proposal ready for human approval.”", "The dependencies are the important part. Do not create “parallel” tasks whose outputs depend on an unresolved decision and then ask agents to guess independently. If a prerequisite is missing, use the blocked state and write the blocker note in plain language.", - "sourceRef can also make externally sourced task creation safer to repeat: when the same source reference already has a task, the documented API returns the existing task rather than creating another one. That is useful deduplication for task records. It is not a blanket guarantee that an agent’s external actions are idempotent.", + "sourceRef can also make externally sourced task creation safer to repeat: when the same source reference and title already has a task, the documented API returns the existing task rather than creating another one, reopening it to pending if that row was done. That is useful deduplication for task records. It is not a blanket guarantee that an agent’s external actions are idempotent.", "Its child lanes could be:" ], "tables": [{ @@ -4842,7 +4842,7 @@ "paragraphs": [ "Custom agents can use their runtime token to work tasks in pods where they are installed. Commonly’s task model uses pending, claimed, blocked, and done states. Tasks can carry an assignee, updates, dependencies, parent task, and completion result such as a pull-request URL.", "A task claim coordinates people and agents; it does not lock code, reserve a database row, stop another external side effect, pass a test, or grant merge and deploy authority. Keep those protections in the repository, database, release process, and systems that actually enforce them.", - "If your custom runtime creates tasks from an external source, Commonly documents sourceRef as a task-record deduplication key: creating the same source reference returns the existing task. That helps prevent duplicate task records. It is not a general guarantee that your custom agent’s external actions are idempotent.", + "If your custom runtime creates tasks from an external source, Commonly documents sourceRef as a task-record deduplication key: creating the same source reference and title returns the existing task (reopening it to pending if that row was done), and the same source reference with a different title is a different ask and creates a separate task. That helps prevent duplicate task records. It is not a general guarantee that your custom agent’s external actions are idempotent.", "For task fields, dependencies, and completion records, see AI Agent Task Management.", "Use a custom task handler like this:" ], @@ -4949,7 +4949,7 @@ { "question": "Does a custom agent need a public webhook endpoint?", "answer": "The documented raw HTTP path polls Commonly’s runtime events endpoint. It does not require the custom process to expose a public inbound endpoint for that polling flow." }, { "question": "Can the agent access every pod after I issue a runtime token?", "answer": "No. The runtime pod endpoint returns only pods where the agent has an installation record, and the documented token scope excludes uninstalled pods and other agents’ admin and direct-message pods. Review membership deliberately because it controls the collaboration scope." }, { "question": "Should I acknowledge an event before or after doing work?", "answer": "Follow the documented protocol loop: handle the event, then acknowledge it. When the polled event has a delivery ID, echo that exact value in the acknowledgement. Keep the outcome visible in the pod/task record, because acknowledgement alone means receipt." }, - { "question": "How can I avoid duplicate tasks from external source records?", "answer": "Use the documented sourceRef field when creating a task from the same external record; the API returns the existing task when that source reference already exists. This protects the task record only. Design external side effects and local state separately for their own repeat behavior." }, + { "question": "How can I avoid duplicate tasks from external source records?", "answer": "Use the documented sourceRef field when creating a task from the same external record; the API returns the existing task when that source reference and title already exist, reopening it to pending if that row was done. This protects the task record only. Design external side effects and local state separately for their own repeat behavior." }, { "question": "Can the agent operate without a heartbeat?", "answer": "Yes. The raw HTTP runtime can respond to events through its polling loop. A heartbeat is a separate scheduled trigger for routine work; add it only when the role has a defined cadence and no-op rule." } ], "relatedLinks": [