feat(task-board): idempotent task creation via external_ref

[dimakis]

Summary

• Adds external_ref column to task store with a partial unique index (WHERE NOT NULL)
• Middleware on POST /api/tasks checks for existing task by externalRef before creating — returns 200 + deduplicated: true instead of a duplicate 201
• DB constraint is the safety net for concurrent requests; middleware provides the clean UX
• Migration 2 added for existing databases

Companion PR: dimakis/mgmt — wires external_ref into PR Shepherd goal creation and mitzo_client.create_task()

Test plan

• TaskStore: create with externalRef, getByExternalRef, rejects duplicates, allows null refs
• Routes: POST deduplicates by externalRef, no dedup without ref
• All 70 existing tests pass

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

frontend/src/types/task.ts

Clean, well-tested feature. The main gap is that frontend Task types in frontend/src/types/task.ts and packages/client/src/slices/tasks.ts were not updated to include externalRef, creating a server/client type mismatch.

• 🟡 regressions: The server-side Task interface now includes externalRef: string | null, but the frontend Task type (frontend/src/types/task.ts line 48) and the client package Task type (packages/client/src/slices/tasks.ts line 39) were not updated. The server API returns externalRef in all task responses, creating a type mismatch. Any future frontend code referencing task.externalRef will get a TypeScript error despite the field being present at runtime. [fixable]

server/api-schemas.ts

Clean, well-tested feature. The main gap is that frontend Task types in frontend/src/types/task.ts and packages/client/src/slices/tasks.ts were not updated to include externalRef, creating a server/client type mismatch.

• 🔵 unsafe_assumptions (L126): externalRef is validated as z.string().optional() — this accepts empty strings. An empty string is non-NULL in SQLite, so it would hit the UNIQUE constraint on a second task with externalRef: "". Consider adding .min(1) (like title on line 116) to prevent ambiguous empty-string refs. [fixable]

server/__tests__/task-store.test.ts

Clean, well-tested feature. The main gap is that frontend Task types in frontend/src/types/task.ts and packages/client/src/slices/tasks.ts were not updated to include externalRef, creating a server/client type mismatch.

• 🔵 missing_tests: No migration test for the new external_ref column. The existing migration test (line 512) verifies workflow columns survive reopen, but doesn't assert externalRef is populated correctly after migration 2 runs. Following the same pattern, a test should create a DB, close it, reopen (triggering migration 2), and verify task.externalRef is null for pre-existing rows. [fixable]

[dimakis]

🔵 unsafe_assumptions: externalRef is validated as z.string().optional() — this accepts empty strings. An empty string is non-NULL in SQLite, so it would hit the UNIQUE constraint on a second task with externalRef: "". Consider adding .min(1) (like title on line 116) to prevent ambiguous empty-string refs. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (2 warning).

frontend/src/types/task.ts

Solid implementation with good test coverage and clean migration. The main gap is that both frontend Task type definitions (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) are missing the new externalRef field, creating a type contract mismatch with the server.

• 🟡 regressions (L49): Frontend Task interface is missing the new externalRef: string | null field. The server now returns this field in every task JSON response, but TypeScript on the frontend doesn't declare it — any future code that accesses task.externalRef will fail type-checking. [fixable]

packages/client/src/slices/tasks.ts

Solid implementation with good test coverage and clean migration. The main gap is that both frontend Task type definitions (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) are missing the new externalRef field, creating a type contract mismatch with the server.

• 🟡 regressions (L39): Same issue in @mitzo/client: the Task interface here also lacks externalRef: string | null. Both frontend type definitions need to stay in sync with the server's Task type. [fixable]

server/__tests__/task-store.test.ts

Solid implementation with good test coverage and clean migration. The main gap is that both frontend Task type definitions (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) are missing the new externalRef field, creating a type contract mismatch with the server.

• 🔵 missing_tests (L547): The existing migration test ('migrates existing database to add workflow columns') doesn't assert that externalRef defaults to null after migration 2 runs. Adding expect(task!.externalRef).toBeNull() would verify the new migration works on pre-existing databases. [fixable]

server/__tests__/task-routes.test.ts

Solid implementation with good test coverage and clean migration. The main gap is that both frontend Task type definitions (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) are missing the new externalRef field, creating a type contract mismatch with the server.

• 🔵 missing_tests (L163): No test for the case where a deduplicated task is returned but the caller sent different fields (e.g. different parentId, priority, or description). The current idempotency guard silently ignores mismatched fields and returns the original — this is fine if intentional, but a test documenting this 'first-write-wins' behavior would prevent future confusion. [fixable]

[dimakis]

Centaur Review

Found 2 issue(s).

server/__tests__/task-routes.test.ts

Clean, well-structured PR. Schema, migration, idempotency guard, and test coverage are all solid. Two minor test gaps — an empty-string rejection regression test and a migration-specific test — would harden the feature further.

• 🔵 missing_tests (L194): No test that externalRef: "" (empty string) is rejected with 400. Commit 0ece048 specifically added .min(1) to fix this — a regression test would prevent that validator from being accidentally weakened. [fixable]

server/__tests__/task-store.test.ts

Clean, well-structured PR. Schema, migration, idempotency guard, and test coverage are all solid. Two minor test gaps — an empty-string rejection regression test and a migration-specific test — would harden the feature further.

• 🔵 missing_tests (L545): No migration test for the new external_ref column. The existing migrates existing database to add workflow columns test covers migration 1 by creating a DB without the column and reopening it. A parallel test for migration 2 would ensure the ALTER TABLE + CREATE UNIQUE INDEX migration runs correctly on pre-existing databases. [fixable]

[dimakis]

🔵 missing_tests: No test that externalRef: "" (empty string) is rejected with 400. Commit 0ece048 specifically added .min(1) to fix this — a regression test would prevent that validator from being accidentally weakened. [fixable]

[dimakis]

🔵 missing_tests: No migration test for the new external_ref column. The existing migrates existing database to add workflow columns test covers migration 1 by creating a DB without the column and reopening it. A parallel test for migration 2 would ensure the ALTER TABLE + CREATE UNIQUE INDEX migration runs correctly on pre-existing databases. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

frontend/src/types/task.ts

Solid, well-tested feature. The main gap is that the frontend Task types (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) were not updated to include externalRef, which will cause type mismatches if the field is accessed on the client side.

• 🟡 regressions (L48): The server Task interface now includes externalRef: string | null, but the frontend type at line 48 (and packages/client/src/slices/tasks.ts line 38) still end at templateId. Both need externalRef: string | null added to stay in sync — without it, any frontend code that tries to read task.externalRef (e.g. for display on the task board) won't type-check. [fixable]

server/app.ts

Solid, well-tested feature. The main gap is that the frontend Task types (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) were not updated to include externalRef, which will cause type mismatches if the field is accessed on the client side.

• 🔵 unsafe_assumptions (L639): TOCTOU gap: between the getByExternalRef check (line 640) and the create call (line 646), a concurrent request with the same externalRef could insert first. The UNIQUE index protects data integrity (the second insert throws), but the caller gets an opaque SQLite constraint error (400) instead of the clean { deduplicated: true, task } response. For a single-user app this is low-risk, but wrapping the check+insert in a transaction or catching the constraint violation with a retry/lookup would make the idempotency guarantee airtight. [fixable]

server/__tests__/task-routes.test.ts

Solid, well-tested feature. The main gap is that the frontend Task types (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) were not updated to include externalRef, which will cause type mismatches if the field is accessed on the client side.

• 🔵 missing_tests (L163): The dedup route test only verifies id and title of the returned task. Consider also asserting that the second request's different body fields (e.g. title: 'Duplicate create') do NOT overwrite the original — i.e. the existing task is returned unmodified. A test for deduplicated on the first (non-dedup) response being absent (expect(first.body.deduplicated).toBeUndefined()) would also strengthen the contract. [fixable]

server/__tests__/task-store.test.ts

Solid, well-tested feature. The main gap is that the frontend Task types (frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) were not updated to include externalRef, which will cause type mismatches if the field is accessed on the client side.

• 🔵 missing_tests (L564): The migration test creates a fresh DB (which already gets the full schema including external_ref), so the migration check finds the column exists and skips the ALTER TABLE path entirely. It tests idempotent reopen, not actual migration. To test the real migration path, the test would need to create a DB with a schema that lacks external_ref (e.g. drop the column manually before reopening). This matches the pattern of the existing migration-1 test, so it's a pre-existing limitation, not introduced by this PR. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

frontend/src/types/task.ts

Clean, well-tested idempotency feature. The main issue is that the frontend Task type (in both frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) was not updated to include the new externalRef field, creating a type-level inconsistency with the server API response.

• 🟡 regressions (L48): The server Task interface now includes externalRef: string | null, but the frontend Task type is missing it. The API response will include the field; the frontend type should stay in sync. Same issue in packages/client/src/slices/tasks.ts (line 39). [fixable]

server/__tests__/task-store.test.ts

Clean, well-tested idempotency feature. The main issue is that the frontend Task type (in both frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) was not updated to include the new externalRef field, creating a type-level inconsistency with the server API response.

• 🔵 missing_tests: No test verifies that getByExternalRef leverages the unique index efficiently (e.g. with many tasks). More importantly, there's no test that externalRef uniqueness is scoped globally — i.e. two tasks under different parents with the same externalRef still conflict. The rejects duplicate externalRef test only covers the same-parent case implicitly. A cross-parent uniqueness test would document the design intent. [fixable]

server/app.ts

Clean, well-tested idempotency feature. The main issue is that the frontend Task type (in both frontend/src/types/task.ts and packages/client/src/slices/tasks.ts) was not updated to include the new externalRef field, creating a type-level inconsistency with the server API response.

• 🔵 style (L646): body.data as TaskCreateInput — this cast is used because TaskCreateBody (Zod) and TaskCreateInput (interface) are nearly identical but technically separate types. Now that both include externalRef, the cast still works, but consider sharing the type (e.g. using z.infer<typeof TaskCreateBody> as TaskCreateInput) to avoid future drift. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (2 warning).

server/app.ts

Clean feature addition with good test coverage. Main concern is a TOCTOU race in the idempotency guard that can surface a raw SQLite error on concurrent duplicate requests, and the frontend Task type is missing the new externalRef field.

• 🟡 unsafe_assumptions (L638): TOCTOU race: getByExternalRef + create are not atomic. Two concurrent requests with the same externalRef can both pass the check, then the second create hits the UNIQUE constraint and returns a raw 400 error instead of a graceful 200 dedup response. Since this is single-process Node.js with synchronous SQLite, the window is narrow but real (e.g. two requests arrive before the event loop yields). Consider catching the UNIQUE constraint error and falling back to getByExternalRef, or wrapping both in a single better-sqlite3 transaction. [fixable]

frontend/src/types/task.ts

Clean feature addition with good test coverage. Main concern is a TOCTOU race in the idempotency guard that can surface a raw SQLite error on concurrent duplicate requests, and the frontend Task type is missing the new externalRef field.

• 🟡 regressions (L48): The server Task interface now includes externalRef: string | null, but the frontend Task type in types/task.ts is missing this field. The server serializes externalRef in JSON responses, so the frontend type is out of sync. This won't cause a runtime error (TypeScript types are erased), but it means frontend code can't safely access task.externalRef without casting. Add externalRef: string | null to the frontend Task interface for type-level consistency. [fixable]

server/__tests__/task-routes.test.ts

Clean feature addition with good test coverage. Main concern is a TOCTOU race in the idempotency guard that can surface a raw SQLite error on concurrent duplicate requests, and the frontend Task type is missing the new externalRef field.

• 🔵 missing_tests (L163): No test for the TOCTOU/UNIQUE-constraint race scenario: what happens when two requests with the same externalRef race past the getByExternalRef check and the second hits the SQLite UNIQUE constraint? Currently this would return a 400 with a raw SQLite error message. A test would document and enforce the desired behavior (either graceful dedup or a clear error).

server/__tests__/task-store.test.ts

Clean feature addition with good test coverage. Main concern is a TOCTOU race in the idempotency guard that can surface a raw SQLite error on concurrent duplicate requests, and the frontend Task type is missing the new externalRef field.

• 🔵 missing_tests (L514): No test that getByExternalRef returns the task with children populated (since rowToTask always sets children: []). If a caller expects getByExternalRef to return a full tree like getTree does, this could be surprising. Consider a test documenting that getByExternalRef returns a flat task without assembled children.

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

frontend/src/types/task.ts

Clean, well-tested feature. The main gap is the frontend Task type not being updated to include externalRef, which creates a type divergence between server and client.

• 🟡 regressions (L48): The server-side Task interface now includes externalRef: string | null, but the frontend Task type in frontend/src/types/task.ts was not updated. While no frontend code currently reads this field, the type divergence means any future frontend code (e.g., displaying externalRef in TaskNode or TaskSidebar) would lack type safety. The field should be added to keep the two interfaces in sync. [fixable]

server/app.ts

Clean, well-tested feature. The main gap is the frontend Task type not being updated to include externalRef, which creates a type divergence between server and client.

• 🔵 bugs (L639): The check-then-insert for externalRef deduplication is a TOCTOU pattern. However, since this project uses better-sqlite3 (synchronous) in a single-threaded Node.js process, concurrent interleaving is not possible — the race condition cannot actually occur at runtime. The UNIQUE index provides a safety net regardless. No change needed, but worth a comment if the server ever moves to async DB calls.
• 🔵 style (L642): The deduplicated response returns the existing task without broadcasting via onTaskBroadcast. This is correct (no new task was created), but the response also omits the deduplicated field from any schema — it's an ad-hoc addition to the response shape. Consider documenting or typing this response variant for frontend consumers. [fixable]

server/__tests__/task-routes.test.ts

Clean, well-tested feature. The main gap is the frontend Task type not being updated to include externalRef, which creates a type divergence between server and client.

• 🔵 missing_tests: No test verifies that the UNIQUE constraint error from taskStore.create() (when two tasks share an externalRef) is caught and returns a 400 response. The route test only covers the application-level dedup check (200 path). A test that bypasses the app-level check (e.g., by directly calling store.create twice) exists in task-store.test.ts, but the HTTP error path for a constraint violation is untested. [fixable]