Skip to content

Commit ca16dc2

Browse files
claude[bot]claude
andauthored
docs(example-todo): completeTask states the real completed_date hazard — the handler write is not stripped, it lands (#17817)
Fixes #17629 Clause-②: no ## What this changes One docblock, on `completeTask` in `examples/app-todo/src/actions/task.handlers.ts`. Nothing else in the tree moves. The old text described a read-only strip that **does not run on this path**, and it did so in the direction that leaves a copier less safe. PR #17608 removed the identical prose from `content/docs/ui/actions.mdx`; this is the other half — the example the docs page had copied it from, and the demonstrated origin of the falsehood that cost #17608 three review rounds. ## The mechanism, re-measured in this worktree Re-derived here rather than inherited from the card, at `objectstack-ai/objectstack` `c9eb773ba` (the branch base): | site | reading | |:--|:--| | `packages/runtime/src/action-execution.ts:1420` | `buildActionExecutionContext(ec)` returns `{ ...base, isSystem: true }` — unconditional, no branch | | `packages/runtime/src/action-execution.ts:1458-1459` | `buildActionEngineFacade` calls it once and threads the result as `context` into every `ql.insert` / `update` / `delete` / `find` the handler's `ctx.engine` makes | | `packages/objectql/src/engine.ts:12227` | the static read-only strip (`stripReadonlyFields` + its `reportDroppedFields`) is inside `if (!opCtx.context?.isSystem)` — single-row update path | | `packages/objectql/src/engine.ts:12410` | the same gate on the multi-row update path | | `packages/objectql/src/engine.ts:11480` | the pre-hook withholding of caller read-only keys is gated identically | | `examples/app-todo/src/objects/task.hook.ts:78` | the stamp is conditional: `data.status === 'completed' && previous?.status !== 'completed'` — the **transition** only | ⇒ On a handler's write the strip never runs, so the write **lands**; and a completion write that is not a transition is never stamped, so a handler-supplied `completed_date` silently replaces the true one. A server-owned field is **more** dangerous in a handler than in a form PUT, not less. This agrees with the mechanism in the dispatch and with the corrected `content/docs/ui/actions.mdx` (read directly, lines 157-171). **Line-number drift, reported as asked:** the card cites `engine.ts:12213` and `:11466`; at `c9eb773ba` those gates sit at `:12227` and `:11480` (+14). `action-execution.ts:1420` is unmoved. Everything above was located from the symbol, not the line number. ## Acceptance 1 + 2 — whole-file grep, with a lit control Not only the three lines the card quotes. File read by state at `c9eb773ba` (pre) and at this PR's head (post). **Pass A — the retracted claim's own phrases.** | pattern (case-insensitive) | pre | post | |:--|--:|--:| | `stripped from the payload` | 1 | 0 | | `before the record is validated` | 1 | 0 | | `refuse itself` | 1 | 0 | | `the strip lets through` | 1 | 0 | | `lets? through` | 1 | 0 | | `completed_date_required` | 1 | 0 | | **union, distinct lines** | **5** (lines 45, 46, 47, 48, 50) | **0** | **Pass B — bare mechanism tokens** (`strip|read-?only|server-owned|refuse|elevated|isSystem`), a deliberate superset, because the *true* sentence necessarily reuses the same vocabulary: **7** matching lines post-change, all of them lines 48-63 — inside the new docblock, and each one a true statement. Reported rather than driven to zero: a zero here would only mean the replacement had stopped naming the mechanism. **Lit controls, same pass, same file:** | control | pre | post | why it is lit | |:--|--:|--:|:--| | `completed_date` | 3 (L45, L48, L70) | 3 (L48, L57, L87) | fires, and reaches **outside** the docblock (`cloneTask`'s destructure) — so the pass really covers all 127/144 lines | | `#7036` | 2 (L45, L78) | 2 (L45, L95) | fires, and reaches `massCompleteTasks` | | `zzz_absent` (negative) | 0 | 0 | the matcher is not matching everything | ⇒ The card's quoted span **was** the whole survival in this file — but that is now a measurement, not the assumption the card warned against. **Also checked and deliberately left alone** (both accurate as written, neither states the retracted mechanism): - `examples/app-todo/src/objects/task.object.ts:74-80` — scopes its claim explicitly to *"a non-system caller's write"*, where the strip really does run. True. - `examples/app-todo/src/objects/task.hook.ts:24-48` — same, and its "a value a hook wrote is a platform value and survives" is the `hookWrittenKeys` / `suppliedValues` behaviour on the non-system path. True. - `task.handlers.ts:95` (`massCompleteTasks`) — cross-references `completeTask`'s `status`-only rule. The **prescription** survives the correction unchanged; only its reason changes, and the reason now lives one `{@link}` away. Left as-is, and the new docblock names bulk re-completion as the concrete non-transition case so the cross-reference lands somewhere useful. ## Acceptance 3 + 4 — the replacement Written for someone who will copy it: the hazard first, the concrete wrong line (`completed_date: new Date().toISOString()`) second, and the silence of the failure stated outright — no refusal, no dropped-field report, the write simply lands. It names symbols (`buildActionExecutionContext`, `isSystem`) rather than file line numbers, so it cannot rot the way the citations in this PR body can. ## Acceptance 5 — changeset: skipped, with a reading Not an assumption: - **`files[]` reading.** `examples/app-todo/package.json` declares `"private": true` and **no `files[]` at all**. Across the workspace: 70 publishable manifests, all 70 declaring `files[]`, and **zero** of their `files[]` entries mention `example`; no publishable package lives under `examples/`. - **Built-output grep with a positive control** — see the check block below; symbol count zero, control lit. - **Reachability grep.** Nothing copies this file into a shipped package. `git grep` for its unique symbols outside `examples/app-todo`: `exportTasksToCSV` 0 files, `massCompleteTasks` 0 files, `deleteCompletedTasks` 2 files — both of which only *name* it in prose (`packages/spec/src/ui/action-params.test.ts:501`, `packages/runtime/src/action-execution-destructive.test.ts:241`), neither a copy of the content. ⇒ Nothing published moves. A false statement about a mechanism becomes a true one, inside a private package. ## Acceptance 5, measured half — built-output grep with a positive control Run against the fully built tree at `9d7d2497e` (`pnpm turbo run build --filter='!@objectstack/docs'` — 73/73 tasks, exit 0). The scan surface is every path the 70 publishable manifests declare in `files[]`: **186 roots, 2967 shipped files, 247.8 MiB**. | needle | shipped hits | reading | |:--|--:|:--| | `exportTasksToCSV` (symbol unique to the changed file) | **0** | the file's content is in no shipped artifact | | `massCompleteTasks` (ditto) | **0** | ditto | | `what a handler names, lands` (the NEW sentence) | **0** | this PR ships no text | | `stripped from the payload` (the RETRACTED sentence) | 4 | **not this file** — `packages/spec/dist/analytics.zod-*.d.{ts,mts}` (the `onFieldsDropped` docblock from `packages/spec/src/contracts/data-engine.ts`, which correctly scopes its claim to *caller-supplied* fields) and `packages/plugins/plugin-auth/CHANGELOG.md` (a whitelist strip, a different mechanism) | | `buildActionExecutionContext` — **positive control** | **13** | matcher lit across the shipped set | | `ActionHandlerContext` — **positive control** | **5** | matcher lit | | `zzz_absent_token` — negative control | 0 | the matcher is not matching everything | ⇒ Symbols zero, controls lit. Nothing this diff touches reaches a consumer. `skip-changeset` applied additively (`POST /issues/17817/labels`, HTTP 200) and read back on two legs — `GET /pulls?head=…` and `GET /issues/17817/labels` both return `['size/s', 'skip-changeset']`, so nothing was stripped by a concurrent write. ## Verification Exit codes captured **before** any pipe. Gate families derived mechanically, not guessed: `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` → **33 commands**, all run. | what | result | |:--|:--| | 33/33 derived gate commands | **exit 0** — includes `check:nul-bytes` (8463 text files, no raw control bytes), `check:system-context-census` (110 elevation read sites / 91 symbols — the census this PR's prose talks about), `check:comment-mask-adoption` + `-corpus`, `check:keyed-text-bounds`, `check:reference-carrier-shape` (6645 files), `check:examples-live-imports`, `check:published-files`, `check:dts-closure`, `check:lean-entry-closure`, `check:sourcemap-no-sources-content`, `check:dual-build-cjs-loads` (104 require entry points / 67 packages / 620 CJS files) | | `pnpm --filter @objectstack/example-todo typecheck` | **exit 0** — and the changed file is genuinely inside the program: `tsc --noEmit --listFiles` lists `examples/app-todo/src/actions/task.handlers.ts` (28 files from `app-todo/src`), so the green is a measurement, not an exclusion | | `pnpm --filter @objectstack/example-todo exec vitest run --maxWorkers=2` | **exit 0** — 4 files, **106 tests passed** | | `pnpm turbo run build --filter='@objectstack/example-todo^...'` | **exit 0** — 59/59 tasks (the affected package's dependency closure) | | `npx eslint . --no-inline-config --format json` | **exit 0** — the whole repo, **6645 files, 0 errors, 0 warnings**. Run in full rather than narrowed, so no narrowing claim is needed | | control-byte self-scan on the changed file | no match (`grep -naP` over the C0/DEL classes) | **First-pass NOT MEASURED, then resolved:** four gates (`check:dts-closure`, `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:sourcemap-no-sources-content`) exited **3** on the unbuilt tree — their own words, "PREREQUISITE NOT MET … NOT a pass and NOT a finding". They were re-run after the build and are among the 33 greens above. `check:dual-build-cjs-loads` needed the *whole* tree, not just this card's closure, and got it. **Genuinely NOT MEASURED here, by design, and left to CI:** the five path-scheduled CI jobs `dispatch-gates` names as having no local invocation (`Test Core` 6 shards, `Temporal Conformance`, `Dogfood Regression Gate`, `Dogfood Verify CLI`, `Build Core`), the 11 families whose declared population is too wide to place, and the 51 artifact-roster families whose silence is a fact about a list rather than about these paths. None of them is reported as green. Authored by Claude Code, session `session_01TSf4DV7ziu4V5j73e46b7c` (https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c). This body was edited through a bare REST `PATCH /pulls`, the one channel measured to append its own attribution block, so the footer below is the platform's and this line is the durable record. --- _Generated by [Claude Code](https://claude.ai/code)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent f830fa2 commit ca16dc2

1 file changed

Lines changed: 23 additions & 6 deletions

File tree

‎examples/app-todo/src/actions/task.handlers.ts‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,29 @@ import type { ActionHandlerContext } from '@objectstack/spec/ui';
4242
/**
4343
* Mark a single task as complete.
4444
*
45-
* [#7036] `status` only. `completed_date` is `readonly` — server-owned — so a
46-
* caller's write to it is stripped from the payload before the record is
47-
* validated, and sending it here made this action refuse itself against
48-
* `todo_task`'s `completed_date_required` rule. The stamp belongs to the
49-
* `beforeUpdate` leg of `src/objects/task.hook.ts`, which runs on the
50-
* transition and whose write the strip lets through.
45+
* [#7036] `status` only — and the reason is the opposite of the intuitive one,
46+
* so read it before copying this handler.
47+
*
48+
* `completed_date` is `readonly` on `todo_task` — server-owned — but a
49+
* handler's `ctx.engine` runs **elevated**: `buildActionExecutionContext`
50+
* forces `isSystem: true` on every call it makes (#3914), and the engine's
51+
* read-only strip is gated on *not* being a system write. So on a handler's
52+
* write the strip never runs at all: **what a handler names, lands**. A
53+
* server-owned field is therefore MORE dangerous here than in a form PUT, not
54+
* less — the protection an ordinary caller gets is the one thing a handler
55+
* does not have.
56+
*
57+
* Adding `completed_date: new Date().toISOString()` to the write below looks
58+
* harmless, and on the transition into `completed` it is: the `beforeUpdate`
59+
* leg of `src/objects/task.hook.ts` stamps the column and overwrites it. But a
60+
* completion write that is NOT a transition — re-completing a task that is
61+
* already `completed`, which is exactly what {@link massCompleteTasks} does to
62+
* an already-finished row in a bulk selection — is not stamped, and nothing
63+
* strips the handler's value either. It does not fail: there is no refusal and
64+
* no dropped-field report, the write simply lands, and the real completion
65+
* timestamp is silently replaced with "now".
66+
*
67+
* Hence `status` alone: let the hook own the column it owns.
5168
*/
5269
export async function completeTask(ctx: ActionHandlerContext): Promise<void> {
5370
const { record, engine } = ctx;

0 commit comments

Comments
 (0)