Skip to content

lint: warn when an action body writes a readonlyWhen field through ctx.api - #13844

Merged
os-project-manager merged 4 commits into
mainfrom
claude/issue-13770-action-body-readonly-write
Aug 31, 2026
Merged

lint: warn when an action body writes a readonlyWhen field through ctx.api#13844
os-project-manager merged 4 commits into
mainfrom
claude/issue-13770-action-body-readonly-write

Conversation

@claude

@claude claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Part of #13770 — deliberately not Fixes. The card asked for two rule ids on the action surface; only one of them is true on this tree, and the other needs a decision this PR does not take. Details under "The premise, re-measured" below. #13770 remains open.

Adds validateReadonlyActionWrites, the action-surface member of the readonly write family, wired through REFERENCE_INTEGRITY_RULES so it runs on os validate, os lint and os compile at once.

The premise, re-measured — and half of it does not hold

The card's mechanism is: "An action body reaches the engine through the same ScopedContext, so ctx.api.object('x').update({ someReadonlyField }) in an action is dropped by the same non-system stripReadonlyFields pass". That is true of a hook body. It is not true of an action body, and the difference is the run identity, not a reading.

An action body's ctx.api is buildActionApi = ql.createContext(buildActionExecutionContext(ec)), and buildActionExecutionContext is three lines:

export function buildActionExecutionContext(ec: any): Record<string, unknown> {
    const base = ec && typeof ec === 'object' ? { ...(ec as Record<string, unknown>) } : {};
    return { ...base, isSystem: true };
}

Both production dispatch paths build it that way — REST /actions in packages/runtime/src/domains/actions.ts (whose own comment reads "TRUSTED — system-elevated, RLS/FLS-bypassing by design") and MCP run_action in packages/runtime/src/action-execution.ts. The engine's static strip runs under if (!opCtx.context?.isSystem); the conditional one runs before that guard and takes no isSystem exemption at all.

Measured, not read — a real ObjectQL engine over a memory driver, driven with exactly the context an action body gets ({ userId, tenantId, isSystem: true }), against a field declared readonly: true and a field declared readonlyWhen: record.status == 'paid' on a record in the locked state:

[action ctx.api] static readonly  -> completed_at = "2026-08-31"   (LANDED)
[action ctx.api] readonlyWhen     -> frozen_note  = "original"     (STRIPPED)
[hook ctx.api, non-system] static readonly -> completed_at = null  (STRIPPED)
[hook ctx.api, non-system] readonlyWhen    -> frozen_note = "original"
[ctx.api.sudo()]           readonlyWhen    -> frozen_note = "original"  (STRIPPED)
[action ctx.api] INSERT -> completed_at = "2026-01-01", frozen_note = "seeded"  (INSERT exempt from both)

The repo already pins both halves independently, and both suites pass on this branch:

  • packages/objectql/src/engine-readonly-strict-writes.test.ts — "strict invents no rejection where the strip does not run — isSystem still writes readonly columns"
  • packages/objectql/src/engine-readonly-when-derived-writes.test.ts — "LOCK 2 — isSystem does NOT exempt a caller-supplied value"

So action-api-update-readonly-field at error would gate a build over code that works, on a message ("the write never lands") that is false. That is the claim #8141 removed from the engine's own log, and it is not re-manufactured here. Only the conditional half ships:

  • action-api-update-readonly-when-field — warning. A literal ctx.api.object('…').update() / .updateById() in an action body writing a field the named object declares readonlyWhen. Elevation is not the remedy and the hint does not offer one: an action body is already system-elevated and the lock still applies. What it names instead is what measures true — confirm the call only targets records whose predicate is FALSE, or derive the field in a beforeUpdate hook on the target object (a hook-written value is not caller-supplied and does land, even on a locked record).

The residual is recorded in the rule header rather than guessed at: the third executeAction caller, ObjectQL's ObjectRepository.execute(), supplies neither api nor executionContext, so the sandbox falls back to a context-less repo facade and the static strip does run on that path. A gate whose truth depends on which of three dispatchers invoked the action is not a statically decidable fact; the honest fix there is to give that path the identity the other two have. That is the decision left on #13770.

The ctx.record confirmation the card and triage required

Still true on this tree, and it is what fixes the rule's match surface. buildActionSandboxContext (packages/runtime/src/sandbox/body-runner.ts) binds record: unwrapProxyToPlain(actionCtx?.record) — a materialised plain copy, commented in place as "A snapshot by construction, and read-only by contract (#4345): nothing downstream writes it back" — and boundActionHandler returns result.value with no write-back step. applyMutationsToInput is called on the hook path only; the action path's counterpart is warnDiscardedRecordWrites, which merely reports.

So a ctx.record write never reaches the engine, no readonly strip is ever consulted on it, and a readonly verdict there would be a false positive on every occurrence. record-property-assign is therefore an entry in READONLY_ACTION_WRITE_EXCLUSIONS with that reason attached, and action-record-write-discarded keeps ownership of the shape.

One refinement on the dispatch's expectation: the rule does not consult ctxRecordEscapes. That flag answers "could this record write be live?", which gates the discarded-write finding; the readonly question is about a live write being silently stripped. The exclusion here is unconditional, so the escape verdict never enters it. The escaping shape (ctx.record.f = x; ctx.api.object('o').update(ctx.record)) stays silent for a different reason — the payload is an identifier, not a literal, so no field name is statically knowable — and that is pinned as its own test.

The examples/ population, re-measured with its positive control

Re-ran the card's method on this branch (the real extractHookBodyWriteSet over every source: string literal under examples/, string-concatenation chains folded):

BODY SOURCES PARSED: 187
WRITES BY PATTERN: { 'api-crud-literal': 5, 'input-property-assign': 4 }
POSITIVE CONTROL (input-property-assign writes): 4

ctx.api update/updateById writes (5):
  examples/app-showcase/src/ui/actions/index.ts :: showcase_task.id via update
  examples/app-showcase/src/ui/actions/index.ts :: showcase_task.done via update
  examples/app-showcase/src/ui/actions/index.ts :: showcase_task.progress via update
  examples/app-showcase/src/ui/actions/index.ts :: showcase_invoice.id via update
  examples/app-showcase/src/ui/actions/index.ts :: showcase_invoice.status via update

ctx.record property writes (0):

Identical to the card's measurement at 787d75740. None of the three non-address targets carries either flag today: showcase_task.done and showcase_task.progress are plain fields, and showcase_invoice.status is a Field.select with no readonly and no readonlyWhen (the object's readonlyWhen declarations sit on tax_rate and the line-item columns, which no action body writes). So the rule reports zero findings across examples/ — this is prevention, not a live outage, exactly as the card graded it.

The fixture that proves the rule fires

A green run over today's tree proves nothing on its own, so the suite carries a fixture that would fire, modelled on the shipped showcase invoice's state lock and reproduced synthetically rather than imported from examples/** (maintainer ruling, 2026-08-13):

objects: [{ name: 'showcase_invoice', fields: {
  status:         { type: 'text' },
  tax_rate:       { type: 'number', readonlyWhen: "record.status == 'paid'" },
  invoice_number: { type: 'text', readonly: true },
}}],
actions: [{ name: 'settle_invoice', objectName: 'showcase_invoice',
  body: { language: 'js',
    source: "await ctx.api.object('showcase_invoice').update({ id: ctx.recordId, tax_rate: 8 });" } }],

which yields exactly one action-api-update-readonly-when-field warning at actions[0].body.source, and the same stack with invoice_number in the payload yields none — that pair is the falsification above, pinned as a test rather than left as prose. The suite also carries the walk's positive control (a body declared under objects[].actions is reached, and a merged action is reported once, not twice), the whole ctx.record family, ctx.input, insert/create, sudo, dynamic and unknown targets, the id address key in both directions, and a ledger-partition test that fails if a fifth shared write pattern ever lands unclassified.

Reuse rather than new machinery

Per the card's scope note, nothing new was built. buildReadonlyIndex comes from the flow rule; collectActionBodies and ActionBodySite are now exported from validate-action-body-writes.ts and shared, so both registration sites, the by-value de-duplication of a merged action, the type: 'script' default and the authored-location path stay a single implementation. The hook rule's behaviour is untouched.

Verification

All on 337814c25, this PR's final commit.

what result
pnpm --filter '@objectstack/lint^...' build exit 0
pnpm --filter @objectstack/lint typecheck exit 0
pnpm --filter @objectstack/lint test 88 files, 2434 tests, all passed
pnpm --filter @objectstack/lint build exit 0, check-dts-emitted: 4/4 declaration file(s) present
npx eslint . --no-inline-config (repo-wide sweep) exit 0, 5815 files, 0 errors, 0 warnings
packages/objectql readonly-strip suites 2 files, 26 tests, all passed
derived gate family (scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack) 44 runnable local gates, all exit 0

The sweep gate was run in full rather than narrowed. The family was re-derived three times — after merging origin/main (the first derivation warned it was reading stale workflow files), after the code commit, and again after the docs commit below, which pulled in 20 further gates the code-only diff never touched (check:doc-anchors, check:docs-single-h1, check:corpus-claim-drift, the @objectstack/spec docs family, and the rest). All were run on the final commit.

Two readings that are NOT MEASURED and are reported as such rather than as green:

  • node scripts/check-test-completeness.mjs exits 3 with PREREQUISITE NOT MET locally — it grades a saved turbo run test log that only CI produces. Its own text says to record it as NOT MEASURED.
  • packages/lint/tsconfig.json excludes **/*.test.ts, so the package's typecheck says nothing about the new test file. Checked separately with a temporary project including tests: zero diagnostics reference validate-readonly-action-writes.test.ts (the errors that run reports are pre-existing debt in six other test files, untouched here, which is why the exclusion exists).

Docs drift, addressed

The Docs Drift Check on this PR raised two things. Both are answered here rather than deferred.

1. content/docs/api/error-handling-server.mdx — named, and not falsified. The anchor is the string literal updateById, which this PR introduces in the new rule's STRIP_SUBJECT_METHODS; the existing rule file's diff is purely additive (two symbols exported, JSDoc), with no behaviour change. Read in full, the page makes no claim this PR can falsify: it contains no readonly, readonlyWhen, stripReadonly, isSystem, sudo or elevation statement anywhere (its only readonly hits are TypeScript private readonly class fields in a circuit-breaker sample). Its two updateById mentions are a hook example writing a plain, non-locked field, and the ScopedContext signature note — "the record id travels inside data for update" — which is exactly the fact the new rule's PAYLOAD_ADDRESSED_METHODS encodes. Page and code agree; no edit.

2. packages/lint/src/index.ts yielded no anchor, so the drift run did not cover it — checked by hand, and it did find something. Those eleven lines export a new rule id, and one page enumerates this family across surfaces: content/docs/automation/hook-bodies.mdx, whose "Writing a readonly field" section closes by naming the surfaces that carry the gate. Landing action-api-update-readonly-when-field would have left that enumeration one short — the same defect class as the four pages fixed in #13720 — and, worse on a page titled "Hook & Action Bodies", would have left its hook-scoped table readable as covering actions, which is exactly the reading the measurement above refutes.

So this PR adds two sentences there, stating the measured difference: an action body runs elevated, so the static strip does not apply and a readonly write lands, while the conditional lock is not waived by elevation and does carry across. Nothing else on that page is touched.

Two neighbouring pages were checked and deliberately left alone. content/docs/automation/hooks.mdx says writability is "now gated on both", meaning hook and flow — still true, since the action rule warns rather than gates, and that passage is a hooks-versus-flows comparison rather than a surface enumeration. content/docs/ui/actions.mdx documents only action-record-write-discarded, scoped to ctx.record, and its closing example (mutate the snapshot, then hand it to an api update) is precisely the escaping shape the new rule leaves silent — consistent, not falsified. No page enumerates REFERENCE_INTEGRITY_RULES members to consumers; the only mention is in a dated audit record under docs/audits/, which describes the mechanism, not a member list.

One thing the drift check surfaced belongs to #13832 rather than here: the same section's hook-api-update-readonly-when-field bullet tells authors that readonlyWhen strips a beforeUpdate-derived value and that sudo() is the workaround. Both are measured false, both are the prose twin of the hint defect filed there, and neither is created by this PR — so they are recorded on that card for a single fix, not patched here.

Out of scope, filed separately

#13832 records a defect the measurement above turned up in the two shipped siblings: the readonlyWhen hints on hook-api-update-readonly-when-field and flow-update-readonly-when-field both recommend elevation (ctx.api.sudo(), runAs: 'system'), which the conditional strip does not honour, and the hook one additionally asserts a beforeUpdate-derived value is stripped — the behaviour #9107 removed. Message text only; not folded in here, because #13770's scope fence forbids touching the hook rule.

Authored by Claude Code, session session_01Pk26oZ12t5N1hwGW1m1MgC — recorded in the prose because a body edit rewrites the footer link.

Generated by Claude Code


Generated by Claude Code

claude added 3 commits August 31, 2026 13:47
…ugh ctx.api

Adds validateReadonlyActionWrites, the action-surface member of the readonly
write family, wired through REFERENCE_INTEGRITY_RULES.

An action body's ctx.api is createContext({ ...callerEnvelope, isSystem: true }),
so the engine's static readonly strip - which runs only under
!opCtx.context?.isSystem - is skipped and a readonly:true write LANDS there. The
conditional strip takes no isSystem exemption, so a readonlyWhen field written
through ctx.api is still dropped on records whose predicate is TRUE. Only that
second shape is reported, as a warning.

ctx.record is excluded from the match set: an action's ctx.record is a dead
snapshot the runtime never writes back, so no strip is ever consulted on it and
a readonly verdict there would be false on every occurrence.

Reuses buildReadonlyIndex from the flow rule and collectActionBodies from the
action rule rather than growing a second walk.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pk26oZ12t5N1hwGW1m1MgC
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/lint, touching 20 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/lint/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/error-handling-server.mdx (via updateById (literal, a string literal in STRIP_SUBJECT_METHODS))
What this run could not see
  • 1 changed file(s) yielded no anchor (packages/lint/src/index.ts) — pages documenting those are invisible to this run
  • 5 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f532630d0246e93b36f869579eb5fa4184d51141packageMentionDocs.

Which tree this was computed on

This run read content/docs from c297243b1f4008648e8f51346278786746729e9d — the merge of head 337814c259646d285d2b4d24fe1b2a67fa4c3f10 into base f532630d0246e93b36f869579eb5fa4184d51141, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c297243b1f4008648e8f51346278786746729e9d && git checkout c297243b1f4008648e8f51346278786746729e9d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f532630d0246e93b36f869579eb5fa4184d51141 337814c259646d285d2b4d24fe1b2a67fa4c3f10 && git checkout -B drift-repro f532630d0246e93b36f869579eb5fa4184d51141 && git merge --no-ff 337814c259646d285d2b4d24fe1b2a67fa4c3f10

node scripts/docs-audit/affected-docs.mjs --json f532630d0246e93b36f869579eb5fa4184d51141

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f532630d0246e93b36f869579eb5fa4184d51141 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
…enumeration

The 'Writing a readonly field' section's table is hook-scoped and its closing
sentence enumerated the surfaces carrying the gate (hook, flow). Landing
action-api-update-readonly-when-field would have left that enumeration one short,
and left the hook-scoped table readable as covering actions on a page titled
'Hook & Action Bodies'.

States the measured difference: an action body runs elevated, so the static strip
does not apply and a readonly write LANDS there, while the conditional lock is
not waived by elevation and does carry across.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pk26oZ12t5N1hwGW1m1MgC
@os-project-manager
os-project-manager marked this pull request as ready for review August 31, 2026 15:43
@os-project-manager
os-project-manager added this pull request to the merge queue Aug 31, 2026
Merged via the queue into main with commit 46b53a2 Aug 31, 2026
38 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-13770-action-body-readonly-write branch August 31, 2026 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants