Keep fresh card revisions out of old job completions - #2
MagMueller wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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
| @@ -0,0 +1,182 @@ | |||
| import assert from "node:assert/strict"; | |||
There was a problem hiding this comment.
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>
c2afdd6 to
9ca9229
Compare
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
expectedIdeaVersionsupports 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
Validation
npm test: build succeeds, 108 tests pass.batchverifies the same-outcome race and explicit-version completion.npm run lint: no errors; one existing hook dependency warning.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.
ideaStatus: nullwhen no state changed.versionasexpectedIdeaVersion; the API validates it and rejects a mismatched value.Written for commit 9ca9229. Summary will update on new commits.