Skip to content

Keep fresh card revisions out of old job completions - #2

Open
MagMueller wants to merge 1 commit into
mainfrom
fix/preserve-new-card-revisions
Open

MagMueller wants to merge 1 commit into
mainfrom
fix/preserve-new-card-revisions

Conversation

@MagMueller

@MagMueller MagMueller commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

What changes

A fresh card revision must not inherit Done from an older job. The maintenance sweep and terminal job endpoint now check the captured card version. An explicitly supplied expectedIdeaVersion supports a worker completing its own result-card replacement.

The card update also depends on winning the job-status comparison inside the same atomic D1 batch. A losing worker cannot overwrite the winning outcome. New-task snapshots now include the card version.

Before and after

  • Before: complete v1, ingest a fresh v2, run maintenance: v2 disappears into Done.
  • After: v2 stays New; the old verified result remains recorded.
  • Default completion for an older version records the job result but leaves the new card alone.
  • An exact current replacement version can still complete normally.
  • Skip, newer queued/running work, malformed legacy context and same-terminal retries retain their intended behavior.

Validation

  • The new source-derived SQL regression suite fails 20 of 34 checks on main and passes all 34 on this head. The baseline failures include one source-contract assertion for the new API.
  • Full npm test: build succeeds, 108 tests pass.
  • Actual isolated Miniflare D1 batch verifies the same-outcome race and explicit-version completion.
  • Real HTTP routes against a separate local D1 database verify new task snapshots, completed-then-replaced, late worker completion, explicit result completion, idempotent retries, Skip and invalid versions. Restarted the process and verified all expected states after fresh maintenance.
  • npm run lint: no errors; one existing hook dependency warning.
  • Standalone TypeScript is not clean: main and this head produce the same 20 diagnostics, including absent Cloudflare binding types. No new diagnostic was introduced. This is distinct from the passing application build and runtime checks.

Scope and limits

No schema migration, dependencies, UI, sorting, points formula or historical result edits. No real card data is included. This prevents future stale transitions; it does not restore cards that already became Done. Automatic restoration would be unsafe because a legitimate explicit result-card completion can have a newer version than the job snapshot.

No live app installation or data repair is included in this PR.


Summary by cubic

Stops old job completions from marking a fresh card revision as Done. Maintenance and the terminal job endpoint now require the job's captured card version to match the current card before applying an outcome.

  • Default completion for a stale version records the job result but leaves the new card alone; the response returns ideaStatus: null when no state changed.
  • A worker completing its own replacement passes the exact returned version as expectedIdeaVersion; the API validates it and rejects a mismatched value.
  • The card update and job-status check run in one atomic D1 batch; a losing race returns 409 and cannot overwrite the winner.
  • New-task snapshots include the card version; legacy unrevised cards still reconcile, and same-second replacement ordering stays ambiguous.
  • Adds a SQL regression suite that fails 20 of 34 checks on main and passes all 34 here.
  • No schema migration; cards already set to Done are not restored.

Written for commit 9ca9229. Summary will update on new commits.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/job-reconciliation.test.mjs">

<violation number="1" location="tests/job-reconciliation.test.mjs:14">
P2: The test extracts the reconcile and endpoint SQL with regexes matched against the first occurrence in three source files (`databaseSource.match(/await db\.prepare\(`\s*(UPDATE ideas...`)[1]`, `jobsSource.match(/updates\.push\(db\.prepare\(`([\s\S]*?)`\)/)[1]`). Adding another update statement before the current one, or reformatting these queries, silently binds the tests to a different or stale statement (the non-greedy `[\s\S]*?` anchored to the first `updates.push` picks whatever precedes it) and can leave the suite passing against the wrong SQL. This is the highest-risk point of the whole change since these tests are the only guard for the CAS/version logic. Prefer extracting the SQL into exported constants in `lib/` (alongside `JOB_MATCHES_IDEA_SQL`) that both the source and the test import, so producers and tests stay in lockstep without regex coupling to formatting.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread db/index.ts
@@ -0,0 +1,182 @@
import assert from "node:assert/strict";

@cubic-dev-ai cubic-dev-ai Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The test extracts the reconcile and endpoint SQL with regexes matched against the first occurrence in three source files (databaseSource.match(/await db\.prepare\(\s*(UPDATE ideas...)[1], jobsSource.match(/updates\.push\(db\.prepare\(([\s\S]*?)\)/)[1]). Adding another update statement before the current one, or reformatting these queries, silently binds the tests to a different or stale statement (the non-greedy [\s\S]*? anchored to the first updates.push picks whatever precedes it) and can leave the suite passing against the wrong SQL. This is the highest-risk point of the whole change since these tests are the only guard for the CAS/version logic. Prefer extracting the SQL into exported constants in lib/ (alongside JOB_MATCHES_IDEA_SQL) that both the source and the test import, so producers and tests stay in lockstep without regex coupling to formatting.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/job-reconciliation.test.mjs, line 14:

<comment>The test extracts the reconcile and endpoint SQL with regexes matched against the first occurrence in three source files (`databaseSource.match(/await db\.prepare\(`\s*(UPDATE ideas...`)[1]`, `jobsSource.match(/updates\.push\(db\.prepare\(`([\s\S]*?)`\)/)[1]`). Adding another update statement before the current one, or reformatting these queries, silently binds the tests to a different or stale statement (the non-greedy `[\s\S]*?` anchored to the first `updates.push` picks whatever precedes it) and can leave the suite passing against the wrong SQL. This is the highest-risk point of the whole change since these tests are the only guard for the CAS/version logic. Prefer extracting the SQL into exported constants in `lib/` (alongside `JOB_MATCHES_IDEA_SQL`) that both the source and the test import, so producers and tests stay in lockstep without regex coupling to formatting.</comment>

<file context>
@@ -0,0 +1,182 @@
+const jobsSource = readFileSync(source("AGENCY_JOBS_SOURCE", "../app/api/agent-jobs/route.ts"), "utf8");
+const tasksSource = readFileSync(source("AGENCY_TASKS_SOURCE", "../app/api/tasks/route.ts"), "utf8");
+const expand = (sql) => sql.replaceAll("${JOB_MATCHES_IDEA_SQL}", JOB_MATCHES_IDEA_SQL);
+const reconcileSql = expand(databaseSource.match(/await db\.prepare\(`\s*(UPDATE ideas\n[\s\S]*?)`\)\.run\(\);/)[1]);
+const finishJobSql = jobsSource.match(/db\.prepare\("(UPDATE agent_jobs SET status = [^"]+)"\)/)[1];
+const finishIdeaSql = expand(jobsSource.match(/updates\.push\(db\.prepare\(`([\s\S]*?)`\)/)[1]);
</file context>
Fix with cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant