Repository navigation
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 #39
Description
Activity
Claiming this issue.
- Session:
session_01SqkTcrxUFci7nqXdbBSe2p - Branch:
claude/issue-39-completed-at-restamp - Worktree:
../duly-issue-39
Re-read the comment thread before starting — no prior claim from another session.
Generated by Claude Code
- Session:
{ "issue": 39, "status": "done", "branch": "claude/issue-39-completed-at-restamp", "pr": "https://github.com/objectstack-ai/duly/pull/80", "premise_still_valid": true, "summary": "The premise held exactly as filed: verified against a booted engine that a `multi: true` write of `status: 'done'` over a batch containing an already-done row re-stamps that row's `completed_at`. Fixed at the write, not the view — `src/hooks/task.hook.ts` now REFUSES such a predicate write with `DULY_TASK_BULK_ALREADY_DONE` (409), which is the route ADR-0058 Addendum II D3 sanctions for a row-conditional decision on a batch-scoped payload ('per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row'). The path is detected via `ctx.dispatch.mode === 'per-row'`, a declared field on HookContextSchema, rather than the `input.options` sniff the issue suggested — measured, `input.options` is real but arrives non-enumerable, while `dispatch.mode` is typed and contract-first. The issue's first fix option (skip the stamp, let `completed_at_required_when_done` refuse) was rejected on measurement: it removes bulk complete entirely, since a homogeneous batch of open rows would then commit `done` with no timestamp and be refused. The guard is decided from the row alone so it holds in either dispatch order; it turns on `status` being in the payload so administrative bulk backfills and the seed's `mode: 'update'` pass are untouched; and only the stamping direction is guarded, since clearing `completed_at` on a batch leaving `done` is genuinely row-invariant. The `visible` predicate on `bulkActionDefs` is kept as the outer UX layer and its docblock — which claimed it was the sole defence — is corrected. NOTE: no changeset was added; the coordinator corrected the dispatch mid-task after verifying this repo has no changesets tooling, and the file I had written was deleted before the commit.", "tests": "All four gates run under the container's shared verify lock (`scripts/pm/os-verify-lock.sh`) on the FINAL commit dfd0a48, each read from the gate's own printed verdict line, not from a bare $?: `✓ Validation passed (357ms)` · `tsc --noEmit` clean · `Test Files 20 passed (20)` / `Tests 544 passed (544)` · `✓ Build complete (734ms)` · wrapper `os-verify-lock: VERDICT command-exit 0`. FAILS-FIRST CONFIRMED: the three new refusal tests were run against the unmodified hook and were red — `× 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`, all with `Error: expected the predicate write to be refused, but it resolved`, `Tests 3 failed | 23 passed (26)`. Post-fix the same file is `Tests 26 passed (26)`. Blast radius across the repo after the fix and before the test rewrite was exactly one red test — `Tests 1 failed | 542 passed (543)`, the `task-actions.test.ts` case that PINNED the hazard with `.not.toBe(original)`; it was rewritten to pin the refusal, with the predicate half kept as a separate case. `test/seed-history.test.ts` stayed green throughout (its `mode: 'update'` backdating pass carries no `status`, so the guard cannot fire on it) and so did `task-hook.test.ts`'s administrative-write assertions. New tests: 7 cases covering the refusal, the both-orders property, no-partial-write, the homogeneous-batch control, the no-status administrative control, the single-record (`mode: 'record'`) control, and the row-invariant clearing direction. Refusals assert the ADR-0112 envelope (`code` + `status`), never a bare toThrow(). BUILD-LOWERING VERIFIED rather than assumed: the handler is lowered into a sandboxed metadata `body` that silently degrades on any module-scope reference, so `dist/objectstack.json` was parsed and `hooks[0].body` confirmed to carry the guard intact. No ablation was needed for this card — the fails-first run against the real unmodified hook is the direct evidence, so nothing was mutated on disk and no rebuild/restore leg applies.", "open_questions": [], "out_of_scope_findings": [ "filed as #78: a bulk write stamps `last_update_at` on rows that changed nothing — same defect class (row-conditional rewrite on a batch-scoped payload) on the sibling column, measured (a row whose note already equalled the payload's moved …986Z → …996Z), left unfixed because the safe rule is a real product call rather than a mechanical change: refusing all-unchanged batches over-refuses, watching the payload accumulate is order-dependent, and skipping the stamp makes bulk completion look like stagnation" ] }
Generated by Claude Code
os-dev-report
Supersedes the previous comment: its leading
<!-- os-dev-report -->marker was eaten by the body sanitizer after it landed, so that copy is invisible to a marker scan. Same JSON, marker as literal text. No edit route was available — this session's token cannot PATCH comments (403 GitHub access is not enabled for this session) and the MCP surface has no comment-edit method, so a replacement comment was the only way to restore the marker.{ "issue": 39, "status": "done", "branch": "claude/issue-39-completed-at-restamp", "pr": "https://github.com/objectstack-ai/duly/pull/80", "premise_still_valid": true, "summary": "The premise held exactly as filed: verified against a booted engine that a `multi: true` write of `status: 'done'` over a batch containing an already-done row re-stamps that row's `completed_at`. Fixed at the write, not the view — `src/hooks/task.hook.ts` now REFUSES such a predicate write with `DULY_TASK_BULK_ALREADY_DONE` (409), the route ADR-0058 Addendum II D3 sanctions for a row-conditional decision on a batch-scoped payload ('per-row previous is supplied so a guard can REFUSE the write, not so a rewrite can be aimed at one row'). The path is detected via `ctx.dispatch.mode === 'per-row'`, a declared field on HookContextSchema, rather than the `input.options` sniff the issue suggested — measured, `input.options` is real but arrives non-enumerable, while `dispatch.mode` is typed and contract-first. The issue's first fix option (skip the stamp, let `completed_at_required_when_done` refuse) was rejected on measurement: it removes bulk complete entirely, since a homogeneous batch of open rows would then commit `done` with no timestamp and be refused. The guard is decided from the row alone so it holds in either dispatch order; it turns on `status` being in the payload so administrative bulk backfills and the seed's `mode: 'update'` pass are untouched; and only the stamping direction is guarded, since clearing `completed_at` on a batch leaving `done` is genuinely row-invariant. The `visible` predicate on `bulkActionDefs` is kept as the outer UX layer and its docblock — which claimed it was the sole defence — is corrected. NOTE: no changeset was added; the coordinator corrected the dispatch mid-task after verifying this repo has no changesets tooling, and the file I had written was deleted before the commit.", "tests": "All four gates run under the container's shared verify lock (`scripts/pm/os-verify-lock.sh`) on the FINAL commit dfd0a48, each read from the gate's own printed verdict line, not from a bare $?: `✓ Validation passed (357ms)` · `tsc --noEmit` clean · `Test Files 20 passed (20)` / `Tests 544 passed (544)` · `✓ Build complete (734ms)` · wrapper `os-verify-lock: VERDICT command-exit 0`. FAILS-FIRST CONFIRMED: the three new refusal tests were run against the unmodified hook and were red — `× 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`, all with `Error: expected the predicate write to be refused, but it resolved`, `Tests 3 failed | 23 passed (26)`. Post-fix the same file is `Tests 26 passed (26)`. Blast radius across the repo after the fix and before the test rewrite was exactly one red test — `Tests 1 failed | 542 passed (543)`, the `task-actions.test.ts` case that PINNED the hazard with `.not.toBe(original)`; it was rewritten to pin the refusal, with the predicate half kept as a separate case. `test/seed-history.test.ts` stayed green throughout (its `mode: 'update'` backdating pass carries no `status`, so the guard cannot fire on it) and so did `task-hook.test.ts`'s administrative-write assertions. New tests: 7 cases covering the refusal, the both-orders property, no-partial-write, the homogeneous-batch control, the no-status administrative control, the single-record (`mode: 'record'`) control, and the row-invariant clearing direction. Refusals assert the ADR-0112 envelope (`code` + `status`), never a bare toThrow(). BUILD-LOWERING VERIFIED rather than assumed: the handler is lowered into a sandboxed metadata `body` that silently degrades on any module-scope reference, so `dist/objectstack.json` was parsed and `hooks[0].body` confirmed to carry the guard intact. No ablation was needed for this card — the fails-first run against the real unmodified hook is the direct evidence, so nothing was mutated on disk and no rebuild/restore leg applies.", "open_questions": [], "out_of_scope_findings": [ "filed as #78: a bulk write stamps `last_update_at` on rows that changed nothing — same defect class (row-conditional rewrite on a batch-scoped payload) on the sibling column, measured (a row whose note already equalled the payload's moved …986Z → …996Z), left unfixed because the safe rule is a real product call rather than a mechanical change: refusing all-unchanged batches over-refuses, watching the payload accumulate is order-dependent, and skipping the stamp makes bulk completion look like stagnation" ] }
Generated by Claude Code
Found while building #4 (bulk complete / bulk skip). Measured against a booted engine, not reasoned about.
What happens
A predicate (
multi: true) update carries one payload for all N matched rows —driver.updateManytakes a single SET clause, and ADR-0058 Addendum II D3 says so explicitly: a rewrite made during any row'sbeforeUpdatedispatch "takes effect on the WHOLE batch, whichever row's dispatch made it".src/hooks/task.hook.tsstamps on the transition:For a batch containing one
openrow and one already-donerow, both being writtenstatus: 'done', the open row's dispatch setsinput.completed_at = now— and that lands on the done row too. Its original completion instant is overwritten.Reproduced in
test/task-actions.test.ts→bulk > the visible predicate is what keeps an already-done row out of the batch:The single-record path is correct and stays correct —
task-hook.test.tsalready pins "is not re-stamped when an already-done task is saved again". This is specific to the shared-payload multi write.Why it is not reachable today
The
bulkActionDefsentries #4 landed carryvisible: Precord.status == "open" || record.status == "in_progress"``, and a bulk predicate is evaluated once per selected record with only the passing rows included in the run. So the UI cannot assemble a batch containing a done row.test/task-actions.test.tspins both halves — the hazard, and the predicate that excludes it.Why it is still worth closing
The guard is client-side, and the hook's own docblock is explicit that a client-side hide is not the authority. Anything that writes
duly_taskin bulk without going through that view — a future import, a backfill job, the dispatcher, an MCP caller,updateManyover a filter — reassembles the batch and silently moves completion history. Nothing errors; the numbers just quietly change.Shape of a fix (for triage, not prescriptive)
The hook can tell it is on the shared-payload path: a predicate write's per-row context arrives with
input.idbound and the caller'smulti/wherestill visible ininput.optionsduring thebefore*phase. Options roughly in order of appetite:completed_atstamp when the batch is not row-invariant, and letcompleted_at_required_when_donerefuse the write loudly — consistent with how the hook already treats the unscoped-multi case ("stamping nothing is fail-safe in both directions").Both change
src/hooks/task.hook.ts, which #4 deliberately did not touch (adjudicated: the actions only flipstatus).Filed unassigned.