Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Refuse a bulk status write that would re-stamp an already-done task - #80

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp
Sep 1, 2026
Merged

Refuse a bulk status write that would re-stamp an already-done task#80
os-warren merged 1 commit into
mainfrom
claude/issue-39-completed-at-restamp

Conversation

@os-warren

@os-warrenos-warren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes#39

Moves the defence from a client-side view predicate to the write itself.

The defect, reproduced

A predicate (multi: true) update carries one payload for all N matched rows — driver.updateMany takes a single SET clause — so the completed_at that src/hooks/task.hook.ts stamps for a row genuinely transitioning into done was written to every row in the batch. A task completed days ago, swept into the same status: 'done' write, silently had its completion instant moved to now. Nothing errored.

Confirmed on a booted engine before writing the fix. The three refusal tests fail against the unmodified hook with:

× refuses a done batch that contains an already-done row, and does not move its clock
× writes nothing at all — the refusal is not a partial batch
× refuses whichever dispatch order the batch arrives in
Error: expected the predicate write to be refused, but it resolved
Tests 3 failed | 23 passed (26)

Why refusing is the fix, not skipping the stamp

ADR-0058 Addendum II is the authority, and it settles the shape rather than leaving it to taste. D1: a before* event dispatches once per matched row. D3: every per-row context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and therefore "a rewrite CONDITIONED on the row (ctx.previous, ctx.input.id) is outside this contract". D3 then names the route in as many words:

Per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row. The three supported routes for row-specific work are: throw (which is what the guard case wants), write through ctx.api per row, or have the CALLER paginate the batch into by-id updates.

completed_at is exactly such a rewrite — it is read off this row's pre-image. So the hook throws.

The issue's other candidate — skip the stamp on the bulk path and let completed_at_required_when_done refuse — was measured against and rejected: it takes bulk complete away entirely. With no stamp, a homogeneous batch of 20 open rows writes status: 'done' with no timestamp and the validation rule refuses the whole thing. "A week's worth of ticks in one gesture" is the feature #4 shipped; the guard has to refuse the mixed batch without costing the homogeneous one.

How the dispatch path is detected

ctx.dispatch is the engine's own marker, a declared field on HookContextSchema (mode: 'record' | 'per-row'), not an inference. Measured on this repo's declarative hook rather than assumed — the issue suggested reading input.options, which does work, but arrives as a non-enumerable property (absent from Object.keys(input)), whereas dispatch.mode is typed and contract-first:

{"event":"beforeUpdate","dispatch":{"mode":"record","index":0},"inputKeys":["id","note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":0},"inputKeys":["note","updated_at"]}
{"event":"beforeUpdate","dispatch":{"mode":"per-row","index":1},"inputKeys":["last_update_at","note","updated_at"]}

That third line is D3 happening in the open: dispatch 1 receives a payload already carrying the last_update_at dispatch 0 wrote.

Three boundaries the guard deliberately holds

  • Decided from the row alone — its own pre-image plus the payload, never from what an earlier dispatch left in the shared payload. Dispatch order is not the caller's to control, and an accumulator would only catch the orders in which the done row happens to come second. Pinned in both orders.
  • Turns on status being in the payload. A predicate write that does not write status computes no stamp, so there is nothing to leak. An administrative bulk backfill over done rows, and the seed's mode: 'update' backdating pass, are untouched — the latter matters because test/seed-history.test.ts is load-bearing and stays green.
  • Only the stamping direction. A batch moving rows out of done writes completed_at = null, and null is correct for every row being moved out of done, including one never completed. That rewrite is genuinely row-invariant, so it is allowed.

A batch in which every row is already done is refused too, though nothing would leak. The hook cannot see the batch — dispatch.index is a position, not a total — and a rule stated on the row is one a caller can predict and a test can pin.

The view predicate is kept

visible on bulkActionDefs still excludes done rows, so the console cannot assemble a batch the server now refuses and a user gets an unavailable action rather than an error they did not cause. Its docblock claimed the predicate was the sole defence and cited the hazard as live; that is now false, so it is corrected rather than left to mislead the next reader.

test/task-actions.test.ts previously pinned the hazard (.not.toBe(original)). That assertion is replaced by the refusal, with the predicate half kept as its own case — the test pinned the branch this PR removes, so rewriting it was the honest triage, not a convenience.

Verification

All four gates, run under this container's shared verify lock on the final commit dfd0a48, each read from the gate's own verdict line:

✓ Validation passed (357ms)
tsc --noEmit — clean
Test Files 20 passed (20)
Tests 544 passed (544)
✓ Build complete (734ms)
os-verify-lock: VERDICT command-exit 0

Blast radius before the test rewrite was exactly one red test repo-wide (Tests 1 failed | 542 passed) — the one pinning the bug.

The handler is lowered into a sandboxed metadata body at build time, which silently degrades if it references module scope, so the built artifact was checked rather than trusted — dist/objectstack.jsonhooks[0].body carries the guard intact:

if(ctx.dispatch?.mode==="per-row"&&"status"ininput&&wasDone&&isDone){throwObject.assign(newError(`Task ${String(input.id??previous.id??"")}isalreadydone.

Refusals assert the ADR-0112 envelope (code + status), never a bare toThrow(). Measured incidentally and worth recording: a throw from a declarative hook under onError: 'abort' reaches the caller with code and status intact.

No changeset

This repo has no changesets tooling — no .changeset/ now or anywhere in git log --all, no @changesets/* dependency, no script, no mention in AGENTS.md. Creating the directory would mint a mechanism nothing reads and the next agent would maintain. The four gates are the whole contract here.

Out of scope, filed separately

Issue #78 records the same defect class on the sibling column, and remains open — nothing here addresses it. last_update_at is stamped only when status/note/skip_reason actually changed against that row's pre-image, which is equally row-conditional, so in a bulk write where some rows change and others do not, the unchanged rows get their clock advanced anyway. Measured: a row whose note already equalled the payload's moved …986Z…996Z. That quiets the "Not moving" signal, which is the harm the hook's own docblock exists to prevent. Left out deliberately: the safe rule is a genuine product call (refusing all-unchanged batches over-refuses; watching the payload accumulate is order-dependent; skipping the stamp makes bulk completion look like stagnation), not a mechanical change.

Generated by Claude Code

A predicate (`multi: true`) update carries ONE payload for all N matched
rows -- `driver.updateMany` takes a single SET clause -- so the
`completed_at` that `task.hook.ts` stamps for a row genuinely
transitioning into `done` was written to every row in the batch. A task
completed days ago, swept into the same `status: 'done'` write, silently
had its completion instant moved to now.
ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate`
context carries that one payload, so "a rewrite takes effect on the WHOLE
batch, whichever row's dispatch made it", and a rewrite conditioned on
the row is outside the contract. D3 names the sanctioned route -- per-row
`previous` is supplied "so a guard can REFUSE the write, not so a rewrite
can be aimed at one row". The hook now throws
`DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove.
The guard is decided from the ROW alone -- its own pre-image plus the
payload -- never from what an earlier dispatch left in the shared
payload, so it holds in either dispatch order. It turns on `status` being
in the payload, which is the only shape that computes a stamp, so an
administrative bulk backfill and the seed's `mode: 'update'` backdating
pass are untouched. Only the stamping direction is guarded: a batch
moving rows out of `done` writes `completed_at = null`, which is correct
for every row in such a batch and so is genuinely row-invariant.
The `visible` predicate on `bulkActionDefs` is kept as the outer layer --
it stops the console assembling a batch the server would refuse -- but it
is no longer the only thing between an import, a backfill, the dispatcher
or an MCP caller and moved completion history. Its docblock, which
claimed the predicate was the sole defence, is corrected.
`test/task-actions.test.ts` previously PINNED the hazard
(`.not.toBe(original)`); that assertion is replaced by the refusal, with
the predicate half kept as its own case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewed, and I measured the one thing this design turns on — merging

Refusing a write on the product's central interaction is a bigger commitment than the card asked for, so the question I had to settle was not whether the guard is correct (the ADR-0058 Addendum II D3 reading is right, and the rejection of the issue's own first option on measurement is the right call) but whether a manager can hit this 409 by doing an ordinary thing. If selecting a few rows that happen to include a finished one produced a hard refusal of the whole batch, this would trade a subtle data bug for a loud usability one.

It cannot. Driven in Chromium against the seeded data, selecting a mix of open and already-done rows and pressing Complete:

dialog: "Complete | Mark the selected tasks done. |
Affected records (1):
3 selected record(s) are not eligible for this action and will be skipped."
write: PATCH /api/v1/data/duly_task/zySd2SSwsWAYgM9Y {"status":"done"} → 200

Two things there, both load-bearing:

  1. The Console filters the ineligible rows out before the write and says so on the confirm dialog, by count. The visible predicate is not decoration.
  2. The write is a per-record PATCH, not a multi: true predicate write at all. So the shared-payload path this guard governs is not the path the UI uses.

Selecting only done rows is handled one step earlier still — the bulk action does not render at all, because no selected row passes visible.

So the guard is genuine defence-in-depth against a programmatic or API caller, unreachable from the Console, and the visible predicate keeps its job as the outer UX layer exactly as the PR describes. Correcting that docblock's claim to be the sole defence was right, and it is now right in both directions.

Gates, re-run by me on dfd0a48 in a clean review worktree:validate 0, typecheck 0, test 0 (Test Files 20 passed, Tests 544 passed), build 0. The ERROR lines in the test output are the suites deliberately exercising failure paths — the duly_task_dispatch_identity unique violation is insertOnce doing its job — not failures.

The details I want to record because they are the reusable part:

  • Refusing an all-done batch too, even though nothing would leak, on the grounds that dispatch.index is a position and not a total, so the hook cannot see the batch. A rule decided from the row alone is one a caller can predict and a test can pin; a rule that depends on what an earlier dispatch left behind is order-dependent and only catches half the orders. That reasoning is why the guard holds in either dispatch order, and the both-orders test is the right thing to have pinned.
  • Verifying the build lowering rather than assuming it. The handler ships as a sandboxed metadata body that silently degrades on any module-scope reference, so parsing dist/objectstack.json and confirming hooks[0].body carries the guard intact is not belt-and-braces here — a guard that validates, typechecks and tests green while being absent from the artifact is exactly this repo's recurring failure shape.
  • Choosing ctx.dispatch.mode over the input.options sniff the issue suggested, on the measurement that options arrives non-enumerable. The issue was wrong about the mechanism and you measured instead of following it.

#78 is the right call as a separate card: the same defect class on last_update_at, left unfixed because the safe rule is a genuine product decision — refusing all-unchanged batches over-refuses, and skipping the stamp would make bulk completion read as stagnation, which is the one signal this product cannot afford to get wrong.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 1, 2026 09:58
@os-warren
os-warren merged commit 424bbd1 into mainSep 1, 2026
1 check passed
Sign up for freeto 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.

A bulk status write re-stamps completed_at on an already-done row in the same batch — only a client-side predicate keeps that batch from happening

1 participant

@os-warren