From 240d6e90af206ade7131994e905fe1ee176e90e5 Mon Sep 17 00:00:00 2001 From: Withphildev <220684403+Withphildev@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:54:05 -0700 Subject: [PATCH] Require revisions for local mutations --- .github/workflows/ci.yml | 4 +- README.md | 24 +++++- docs/nova-conversation-contract.md | 6 ++ scripts/agent-smoke.mjs | 10 ++- server/src/index.test.ts | 133 ++++++++++++++++++++++++++++- server/src/index.ts | 67 +++++++++------ 6 files changed, 209 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e68618f..2143de4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,8 +13,8 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v7 + - uses: actions/setup-node@v6 with: node-version: 22 cache: npm diff --git a/README.md b/README.md index cf9e0db..644d1d4 100644 --- a/README.md +++ b/README.md @@ -51,10 +51,32 @@ Environment variables: with explicit backflow, blocking, and reopening routes exposed by `GET /api/lanes`. - Create, update, and delete requests accept an `Idempotency-Key` header or `eventId` body field. Replays return the original task identity without repeating side effects. -- `expectedRevision` on updates provides optimistic concurrency protection. +- Local card, snooze, promotion, decomposition, reminder acknowledgement, and checklist updates + require `expectedRevision`. Missing revisions return `400`; stale revisions return `409` with + the current revision so an agent can reload before deciding whether to retry. - `GET /api/cards/:id/events` exposes the append-only event ledger for a task. - Numbered, transactional SQLite migrations preserve and backfill legacy databases. +### Date and time semantics + +- `dueAt` is planning information. It accepts either a calendar date (`YYYY-MM-DD`) or an ISO 8601 + instant ending in `Z` or a numeric offset. Offset-less date-times are rejected because their + meaning changes with the machine timezone. +- `remindAt`, `recurrenceEndAt`, and snooze times are delivery instants. They always require an ISO + 8601 value with `Z` or a numeric offset plus an IANA timezone where the endpoint requests one. +- Natural-language dates are interpreted conversationally by Nova and confirmed before the API + receives them; KanbanX never guesses phrases such as “later” or “next Friday.” + +### Audit model + +- `task_events` is the durable, append-only source of truth for task mutations and idempotency. + Events remain queryable by the former card ID after a card is deleted, including the final + `card.deleted` event. +- `activity_log` is a compact recent-activity feed for the UI and operations. It is prunable, and + card-specific activity rows are removed with a deleted card; a detached deletion marker remains. +- Checklist rows are task-owned materialized state and are deleted by foreign-key cascade. Their + already-recorded task events remain in the durable ledger. + The board keeps its existing visual design while offering only valid lifecycle moves, surfacing API errors, and sending idempotency/revision guards. diff --git a/docs/nova-conversation-contract.md b/docs/nova-conversation-contract.md index 2718c26..e3ab6ea 100644 --- a/docs/nova-conversation-contract.md +++ b/docs/nova-conversation-contract.md @@ -20,6 +20,10 @@ Content-Type: application/json The server creates a `TASK` in `TRIAGE`, preserves the original text, and marks its source as `nova`. Reuse the same `eventId` if a response is lost and the exact request must be retried. +`dueAt` is planning information: send either a calendar date (`YYYY-MM-DD`) or an exact ISO instant +with `Z` or a numeric offset. Never send an offset-less date-time. Reminder and snooze fields are +delivery instants and always require an offset-bearing ISO value and their requested IANA timezone. + ## Reminder interpretation KanbanX never guesses what a natural-language date means. Nova performs the conversational step: @@ -132,4 +136,6 @@ execution poll during ordinary conversation. - Never create both a cron job and a KanbanX reminder for the same request unless Phil explicitly asks for two delivery mechanisms. - Never reuse an idempotency event id for a different action. +- Always read and send the current `expectedRevision` for local mutations. Reload on `409`; do not + blindly retry a stale change. - Keep LoopX-managed cards read-only and separate from local notebook capture. diff --git a/scripts/agent-smoke.mjs b/scripts/agent-smoke.mjs index 7ff7cd1..5755621 100644 --- a/scripts/agent-smoke.mjs +++ b/scripts/agent-smoke.mjs @@ -32,18 +32,24 @@ const run = async () => { if (!Number.isInteger(cardId)) { throw new Error('create failed: missing card id') } + let revision = created.card.revision for (const lane of ['TODO', 'READY', 'RUNNING', 'DONE']) { - await requestJson( + const moved = await requestJson( `/api/cards/${cardId}`, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ lane, eventId: `smoke-${cardId}-${lane.toLowerCase()}` }), + body: JSON.stringify({ + lane, + expectedRevision: revision, + eventId: `smoke-${cardId}-${lane.toLowerCase()}`, + }), }, 200, `move to ${lane}`, ) + revision = moved.card.revision } await requestJson(`/api/cards/${cardId}`, { method: 'DELETE' }, 204, 'delete') diff --git a/server/src/index.test.ts b/server/src/index.test.ts index 5770259..1c76c8c 100644 --- a/server/src/index.test.ts +++ b/server/src/index.test.ts @@ -95,6 +95,12 @@ describe('workflow hooks', () => { const createRes = await request(mod.app).post('/api/cards').send({ title: 'Delete me' }) expect(createRes.status).toBe(201) const id = createRes.body.card.id as number + const checklist = await request(mod.app).post(`/api/cards/${id}/checklist`).send({ + text: 'Temporary child step', + expectedRevision: 1, + eventId: 'evt-delete-checklist', + }) + expect(checklist.status).toBe(201) const deleteRes = await request(mod.app).delete(`/api/cards/${id}`) expect(deleteRes.status).toBe(204) @@ -102,6 +108,26 @@ describe('workflow hooks', () => { const cardsRes = await request(mod.app).get('/api/cards') expect(cardsRes.status).toBe(200) expect((cardsRes.body.cards as Array<{ id: number }>).some((c) => c.id === id)).toBe(false) + + const deletedChecklist = await request(mod.app) + .patch(`/api/checklist-items/${checklist.body.item.id}`) + .send({ isDone: true, expectedRevision: 1 }) + expect(deletedChecklist.status).toBe(404) + + const events = await request(mod.app).get(`/api/cards/${id}/events`) + expect(events.body.events.map((event: { eventType: string }) => event.eventType)).toEqual([ + 'card.created', + 'checklist.created', + 'card.deleted', + ]) + + const activity = await request(mod.app).get('/api/activity') + expect(activity.body.activity).toContainEqual( + expect.objectContaining({ card_id: null, action: 'card.deleted', detail: `card:${id}` }), + ) + expect( + activity.body.activity.some((entry: { card_id: number | null }) => entry.card_id === id), + ).toBe(false) }) it('persists LoopX task fields with stable defaults', async () => { @@ -136,19 +162,58 @@ describe('workflow hooks', () => { ) }) + it('separates flexible due dates from exact delivery instants', async () => { + process.env.OPENCLAW_WORKFLOW_HOOK_URL = '' + const mod = await import('./index.js') + + const calendarDue = await request(mod.app).post('/api/cards').send({ + title: 'Calendar planning date', + dueAt: '2026-09-15', + eventId: 'evt-date-semantics-calendar', + }) + const exactDue = await request(mod.app).post('/api/cards').send({ + title: 'Exact planning instant', + dueAt: '2026-09-15T09:00:00-07:00', + eventId: 'evt-date-semantics-exact', + }) + const ambiguousDue = await request(mod.app).post('/api/cards').send({ + title: 'Ambiguous planning time', + dueAt: '2026-09-15T09:00:00', + eventId: 'evt-date-semantics-ambiguous', + }) + const impossibleDue = await request(mod.app).post('/api/cards').send({ + title: 'Impossible calendar date', + dueAt: '2026-02-30', + eventId: 'evt-date-semantics-impossible', + }) + + expect(calendarDue.status).toBe(201) + expect(calendarDue.body.card.dueAt).toBe('2026-09-15') + expect(exactDue.status).toBe(201) + expect(ambiguousDue.status).toBe(400) + expect(ambiguousDue.body.error).toContain('YYYY-MM-DD or an ISO instant with an offset') + expect(impossibleDue.status).toBe(400) + }) + it('rejects invalid lifecycle jumps and records valid transitions', async () => { process.env.OPENCLAW_WORKFLOW_HOOK_URL = '' const mod = await import('./index.js') const createRes = await request(mod.app).post('/api/cards').send({ title: 'Lifecycle task' }) const id = createRes.body.card.id as number - const invalid = await request(mod.app).patch(`/api/cards/${id}`).send({ lane: 'RUNNING' }) + const invalid = await request(mod.app) + .patch(`/api/cards/${id}`) + .send({ lane: 'RUNNING', expectedRevision: 1 }) expect(invalid.status).toBe(409) expect(invalid.body.allowedTransitions).toEqual(['TODO']) + let revision = 1 for (const lane of ['TODO', 'READY', 'RUNNING', 'DONE']) { - const move = await request(mod.app).patch(`/api/cards/${id}`).send({ lane }) + const move = await request(mod.app) + .patch(`/api/cards/${id}`) + .send({ lane, expectedRevision: revision }) expect(move.status).toBe(200) + revision = move.body.card.revision } const cardsRes = await request(mod.app).get('/api/cards') @@ -206,6 +271,61 @@ describe('workflow hooks', () => { ]) }) + it('requires expectedRevision on every local update path', async () => { + process.env.OPENCLAW_WORKFLOW_HOOK_URL = '' + const mod = await import('./index.js') + const created = await request(mod.app).post('/api/cards').send({ + title: 'Revision required everywhere', + eventId: 'evt-required-revision-create', + }) + const id = created.body.card.id as number + const until = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() + + const missingPatch = await request(mod.app) + .patch(`/api/cards/${id}`) + .send({ title: 'Must not update', eventId: 'evt-required-revision-patch' }) + const missingSnooze = await request(mod.app) + .post(`/api/cards/${id}/snooze`) + .send({ + until, + timezone: 'America/Los_Angeles', + eventId: 'evt-required-revision-snooze', + }) + const missingChecklistCreate = await request(mod.app) + .post(`/api/cards/${id}/checklist`) + .send({ text: 'Must not create', eventId: 'evt-required-revision-checklist-create' }) + + for (const response of [missingPatch, missingSnooze, missingChecklistCreate]) { + expect(response.status).toBe(400) + expect(response.body.error).toContain('expectedRevision is required') + } + + const checklist = await request(mod.app).post(`/api/cards/${id}/checklist`).send({ + text: 'Created with a revision', + expectedRevision: 1, + eventId: 'evt-required-revision-checklist-valid', + }) + expect(checklist.status).toBe(201) + + const missingChecklistUpdate = await request(mod.app) + .patch(`/api/checklist-items/${checklist.body.item.id}`) + .send({ isDone: true, eventId: 'evt-required-revision-checklist-update' }) + expect(missingChecklistUpdate.status).toBe(400) + expect(missingChecklistUpdate.body.error).toContain('expectedRevision is required') + + const stalePatch = await request(mod.app) + .patch(`/api/cards/${id}`) + .send({ title: 'Stale', expectedRevision: 1, eventId: 'evt-required-revision-stale' }) + expect(stalePatch.status).toBe(409) + expect(stalePatch.body).toMatchObject({ error: 'revision conflict', currentRevision: 2 }) + + const events = await request(mod.app).get(`/api/cards/${id}/events`) + expect(events.body.events.map((event: { eventType: string }) => event.eventType)).toEqual([ + 'card.created', + 'checklist.created', + ]) + }) + it('captures a confirmed reminder once and supports acknowledgement', async () => { process.env.OPENCLAW_WORKFLOW_HOOK_URL = '' const mod = await import('./index.js') @@ -865,6 +985,7 @@ describe('workflow hooks', () => { const secondItem = await request(mod.app).post(`/api/cards/${task.body.card.id}/checklist`).send({ text: 'Do not confuse this item with the first', + expectedRevision: 3, eventId: 'evt-checklist-second', }) const crossItemReplay = await request(mod.app) @@ -885,11 +1006,17 @@ describe('workflow hooks', () => { blockers: [], }) + let taskRevision = secondItem.body.card.revision as number for (const lane of ['TODO', 'READY', 'RUNNING', 'DONE']) { const move = await request(mod.app) .patch(`/api/cards/${task.body.card.id}`) - .send({ lane, eventId: `evt-progress-${lane.toLowerCase()}` }) + .send({ + lane, + expectedRevision: taskRevision, + eventId: `evt-progress-${lane.toLowerCase()}`, + }) expect(move.status).toBe(200) + taskRevision = move.body.card.revision } const completedRestart = await request(mod.app).get( `/api/cards/${project.body.card.id}/restart-packet`, diff --git a/server/src/index.ts b/server/src/index.ts index 0b11764..e2daec7 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -444,16 +444,23 @@ const reconcileLoopx = async (execute: boolean) => { } } -const validDate = (value: unknown) => - value === null || - (typeof value === 'string' && value.trim() !== '' && !Number.isNaN(Date.parse(value))) - const validInstant = (value: unknown) => value === null || (typeof value === 'string' && /(?:Z|[+-]\d{2}:\d{2})$/i.test(value.trim()) && !Number.isNaN(Date.parse(value))) +const validDate = (value: unknown) => { + if (value === null) return true + if (typeof value !== 'string') return false + const normalized = value.trim() + if (/^\d{4}-\d{2}-\d{2}$/.test(normalized)) { + const parsed = new Date(`${normalized}T00:00:00.000Z`) + return !Number.isNaN(parsed.getTime()) && parsed.toISOString().startsWith(normalized) + } + return validInstant(normalized) +} + const validTimezone = (value: unknown) => { if (typeof value !== 'string' || !value.trim()) return false try { @@ -464,6 +471,22 @@ const validTimezone = (value: unknown) => { } } +const requireExpectedRevision = ( + res: express.Response, + value: unknown, + currentRevision: number, +) => { + if (!Number.isInteger(value)) { + res.status(400).json({ error: 'expectedRevision is required and must be an integer' }) + return false + } + if (value !== currentRevision) { + res.status(409).json({ error: 'revision conflict', currentRevision }) + return false + } + return true +} + const localDateKey = (instant: string | Date, timezone: string) => { const date = instant instanceof Date ? instant : new Date(instant) const parts = new Intl.DateTimeFormat('en-CA', { @@ -1418,9 +1441,7 @@ app.post('/api/cards/:id/promote', (req, res) => { } if (existing.item_type !== 'TASK') return res.status(409).json({ error: 'only a task can become a project' }) if (existing.lane === 'DONE') return res.status(409).json({ error: 'reopen a completed task before promoting it' }) - if (!Number.isInteger(body.expectedRevision) || body.expectedRevision !== existing.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: existing.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, existing.revision)) return const now = new Date().toISOString() const updated = db.transaction(() => { @@ -1498,9 +1519,7 @@ app.post('/api/cards/:id/decompose', (req, res) => { plan: normalized.milestones, } if (body.execute !== true) return res.json(responseBase) - if (!Number.isInteger(body.expectedRevision) || body.expectedRevision !== project.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: project.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, project.revision)) return const now = new Date().toISOString() const createdCardIds: number[] = [] @@ -1656,7 +1675,9 @@ const createCard = async (req: express.Request, res: express.Response) => { return res.status(400).json({ error: 'invalid recurrenceEndAt; use an ISO instant with an offset' }) } if (body.dueAt !== undefined && !validDate(body.dueAt)) { - return res.status(400).json({ error: 'invalid dueAt' }) + return res.status(400).json({ + error: 'invalid dueAt; use YYYY-MM-DD or an ISO instant with an offset', + }) } if (body.itemType !== undefined && !itemTypes.includes(body.itemType)) { return res.status(400).json({ error: 'invalid itemType' }) @@ -1855,9 +1876,7 @@ app.post('/api/cards/:id/snooze', (req, res) => { res.set('Idempotent-Replay', 'true') return res.json({ card: cardRowToJson(card), eventId, idempotentReplay: true }) } - if (body.expectedRevision !== undefined && body.expectedRevision !== existing.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: existing.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, existing.revision)) return const now = new Date().toISOString() const updated = db.transaction(() => { @@ -1914,9 +1933,7 @@ app.post('/api/cards/:id/reminders/acknowledge', (req, res) => { if (existing.reminder_status !== 'PENDING' && existing.reminder_status !== 'DELIVERED') { return res.status(409).json({ error: 'reminder is not awaiting acknowledgement' }) } - if (!Number.isInteger(body.expectedRevision) || body.expectedRevision !== existing.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: existing.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, existing.revision)) return const now = new Date().toISOString() let nextRemindAt: string | null = null @@ -2056,7 +2073,9 @@ app.patch('/api/cards/:id', async (req, res) => { return res.status(400).json({ error: 'invalid recurrenceEndAt; use an ISO instant with an offset' }) } if (body.dueAt !== undefined && !validDate(body.dueAt)) { - return res.status(400).json({ error: 'invalid dueAt' }) + return res.status(400).json({ + error: 'invalid dueAt; use YYYY-MM-DD or an ISO instant with an offset', + }) } if (body.estimateMinutes !== undefined && !validNonNegativeInteger(body.estimateMinutes)) { return res.status(400).json({ error: 'invalid estimateMinutes' }) @@ -2087,9 +2106,7 @@ app.patch('/api/cards/:id', async (req, res) => { return res.json({ card: cardRowToJson(card), eventId, idempotentReplay: true }) } - if (body.expectedRevision !== undefined && body.expectedRevision !== existing.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: existing.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, existing.revision)) return const nextLane = body.lane ?? existing.lane const invalidTransition = transitionError(existing.lane, nextLane) @@ -2279,9 +2296,7 @@ app.post('/api/cards/:id/checklist', (req, res) => { res.set('Idempotent-Replay', 'true') return res.json({ structure: structureForCard(card), eventId, idempotentReplay: true }) } - if (body.expectedRevision !== undefined && body.expectedRevision !== card.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: card.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, card.revision)) return const now = new Date().toISOString() const defaultPosition = ( @@ -2357,9 +2372,7 @@ app.patch('/api/checklist-items/:id', (req, res) => { res.set('Idempotent-Replay', 'true') return res.json({ item: checklistRowToJson(current), eventId, idempotentReplay: true }) } - if (body.expectedRevision !== undefined && body.expectedRevision !== item.revision) { - return res.status(409).json({ error: 'revision conflict', currentRevision: item.revision }) - } + if (!requireExpectedRevision(res, body.expectedRevision, item.revision)) return const now = new Date().toISOString() db.transaction(() => {