feat(task-board): idempotent task creation via external_ref#414
feat(task-board): idempotent task creation via external_ref#414dimakis wants to merge 4 commits into
Conversation
Tasks can now carry an optional externalRef field (e.g. "pr_shepherd:dimakis/centaur#42"). The server checks for an existing task with the same ref before creating, returning 200 + deduplicated flag instead of a duplicate 201. A partial unique index on external_ref (WHERE NOT NULL) provides the DB-level safety net for concurrent requests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
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
Taskinterface now includesexternalRef: string | null, but the frontend Task type (frontend/src/types/task.tsline 48) and the client package Task type (packages/client/src/slices/tasks.tsline 39) were not updated. The server API returnsexternalRefin all task responses, creating a type mismatch. Any future frontend code referencingtask.externalRefwill 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):
externalRefis validated asz.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 withexternalRef: "". Consider adding.min(1)(liketitleon 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_refcolumn. The existing migration test (line 512) verifies workflow columns survive reopen, but doesn't assertexternalRefis populated correctly after migration 2 runs. Following the same pattern, a test should create a DB, close it, reopen (triggering migration 2), and verifytask.externalRefisnullfor pre-existing rows.[fixable]
| gateConfig: z.record(z.string(), z.unknown()).optional(), | ||
| maxRetries: z.number().min(0).optional(), | ||
| templateId: z.string().optional(), | ||
| externalRef: z.string().optional(), |
There was a problem hiding this comment.
🔵 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]
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur review: empty strings are non-NULL in SQLite and would collide on the unique index. Add .min(1) validation to match the title field pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 4 issue(s) (2 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
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_refcolumn. The existingmigrates existing database to add workflow columnstest 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]
| expect(first.status).toBe(201); | ||
| expect(second.status).toBe(201); | ||
| expect(first.body.task.id).not.toBe(second.body.task.id); | ||
| }); |
There was a problem hiding this comment.
🔵 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]
| store.create({ title: 'B' }); | ||
| const roots = store.listRoots(); | ||
| expect(roots.length).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
🔵 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]
Centaur review: cover the .min(1) validation with a 400 test, and verify migration 2 adds external_ref to pre-existing DBs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 4 issue(s) (1 warning).
|
Centaur ReviewFound 3 issue(s) (1 warning).
|
Centaur ReviewFound 4 issue(s) (2 warning).
|
Centaur ReviewFound 4 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (1 warning).
server/app.ts
Clean, well-tested idempotency feature with good schema design (partial unique index, immutable ref). The main concern is a TOCTOU race in the check-then-insert pattern — under concurrent requests the UNIQUE constraint will fire as a raw 400 error instead of a graceful 200 dedup response. Low risk for single-user usage but worth hardening.
- 🟡 bugs (L639): TOCTOU race condition: the check-then-insert pattern (
getByExternalRef→create) is not atomic. Two concurrent requests with the sameexternalRefcan both pass the check, and the secondcreate()will throw a UNIQUE constraint violation, returning a generic 400 error instead of the expected 200 deduplicated response. Fix by catching the UNIQUE constraint error in thecatchblock and retrying the lookup, or by usingINSERT ... ON CONFLICTin the store layer.[fixable] - 🔵 unsafe_assumptions (L642): The deduplicated response returns a flat
Taskobject (withchildren: []sincerowToTaskalways setschildren: []), while the normal creation path also returns a flat task. This is consistent, but note that if the existing task has children (e.g., it was created and subtasks were added), the dedup response will showchildren: []becausegetByExternalRefusesrowToTaskwhich doesn't assemble the tree. This may surprise callers expecting the full subtree. Consider whetherget()(which also returns flat) orgetTree()filtered to the task is more appropriate.[fixable]
server/__tests__/task-routes.test.ts
Clean, well-tested idempotency feature with good schema design (partial unique index, immutable ref). The main concern is a TOCTOU race in the check-then-insert pattern — under concurrent requests the UNIQUE constraint will fire as a raw 400 error instead of a graceful 200 dedup response. Low risk for single-user usage but worth hardening.
- 🔵 missing_tests: No test for the UNIQUE constraint error path. If the race condition from the TOCTOU gap occurs (or if
taskStore.create()is called directly with a duplicate ref bypassing the route), the catch block returns a raw SQLite error message to the client. A test that callsstore.create()with a duplicate ref via the API (simulating the race) would validate error handling.[fixable] - 🔵 missing_tests: No test verifying that the deduplicated response still returns the original task's full shape (e.g.,
description,priority,annotations). The current test only checksidandtitle. IfrowToTaskever changes field mapping, this wouldn't catch regressions in the dedup path.[fixable]
server/task-store.ts
Clean, well-tested idempotency feature with good schema design (partial unique index, immutable ref). The main concern is a TOCTOU race in the check-then-insert pattern — under concurrent requests the UNIQUE constraint will fire as a raw 400 error instead of a graceful 200 dedup response. Low risk for single-user usage but worth hardening.
- 🔵 style (L306): The JSDoc example
pr_shepherd:dimakis/centaur#42contains a GitHub username (dimakis). While this is just a doc comment example, the CLAUDE.md states 'No hardcoded machine-specific paths or configuration.' Consider using a generic example likepr_shepherd:org/repo#42(which is what the test uses).[fixable]
| } | ||
| try { | ||
| // Idempotency guard: if externalRef provided and already exists, return existing task | ||
| if (body.data.externalRef) { |
There was a problem hiding this comment.
🟡 bugs: TOCTOU race condition: the check-then-insert pattern (getByExternalRef → create) is not atomic. Two concurrent requests with the same externalRef can both pass the check, and the second create() will throw a UNIQUE constraint violation, returning a generic 400 error instead of the expected 200 deduplicated response. Fix by catching the UNIQUE constraint error in the catch block and retrying the lookup, or by using INSERT ... ON CONFLICT in the store layer. [fixable]
| if (body.data.externalRef) { | ||
| const existing = taskStore.getByExternalRef(body.data.externalRef); | ||
| if (existing) { | ||
| res.status(200).json({ task: existing, deduplicated: true }); |
There was a problem hiding this comment.
🔵 unsafe_assumptions: The deduplicated response returns a flat Task object (with children: [] since rowToTask always sets children: []), while the normal creation path also returns a flat task. This is consistent, but note that if the existing task has children (e.g., it was created and subtasks were added), the dedup response will show children: [] because getByExternalRef uses rowToTask which doesn't assemble the tree. This may surprise callers expecting the full subtree. Consider whether get() (which also returns flat) or getTree() filtered to the task is more appropriate. [fixable]
| return row ? rowToTask(row) : null; | ||
| } | ||
|
|
||
| /** Look up a task by its external reference (e.g. "pr_shepherd:dimakis/centaur#42"). */ |
There was a problem hiding this comment.
🔵 style: The JSDoc example pr_shepherd:dimakis/centaur#42 contains a GitHub username (dimakis). While this is just a doc comment example, the CLAUDE.md states 'No hardcoded machine-specific paths or configuration.' Consider using a generic example like pr_shepherd:org/repo#42 (which is what the test uses). [fixable]
1 participant
Summary
external_refcolumn to task store with a partial unique index (WHERE NOT NULL)POST /api/taskschecks for existing task byexternalRefbefore creating — returns 200 +deduplicated: trueinstead of a duplicate 201Companion PR: dimakis/mgmt — wires
external_refinto PR Shepherd goal creation andmitzo_client.create_task()Test plan
🤖 Generated with Claude Code