Skip to content

fix(lint): hook write-set findings on a lowered hook point at hooks[i].handler - #17438

Merged
baozhoutao merged 1 commit into
mainfrom
claude/issue-16546-lowered-hook-finding-path
Sep 10, 2026
Merged

baozhoutao merged 1 commit into
mainfrom
claude/issue-16546-lowered-hook-finding-path

Conversation

@baozhoutao

Copy link
Copy Markdown
Contributor

Closes #16546

What changed

lowerCallables now records, per call, which hooks[*].handler ref strings
had their body minted here — extracted from the author's inline
handler function — as opposed to a body the author wrote directly
(LoweringResult.loweredHookRefs). The CLI's four lowering doors (os build
union run, os build per-package run, os lint, os validate, and os init/dev's scaffold validation) thread that set through
runAuthoringRules's ctx.loweredHookRefs (AuthoringRuleContext,
@objectstack/lint).

validateReadonlyHookWrites and validateHookBodyWrites
(packages/lint/src/validate-readonly-hook-writes.ts /
validate-hook-body-writes.ts) share one new helper,
hookBodyFindingLocation(hook, hookIndex, ctx), which both call before
pushing a finding: when the hook's ref is named in ctx.loweredHookRefs it
returns path: hooks[i].handler (the key that replaced the function the
author wrote) plus a message suffix — " (judged on the metadata body lowered from the inline handler)" — otherwise it returns the unchanged
hooks[i].body.source. One implementation, so os build and os lint
cannot disagree about it (#16095's rule, applied one layer down).

Why path: hooks[i].handler, not an index

The marker is a ref string (hook.handler's value after lowering), not
a hook index. lowerCallables's taken/refByFn registries are shared
across the whole walk (top level and every ADR-0130 D4 package body), so a
ref is unique for one lowerCallables() call and — because handler is an
ordinary string field — survives a Zod safeParse unchanged. An index would
not: authoringRuleUnionStack folds package hooks into a new top-level array
with its own numbering, and a per-call hook index recorded against the
pre-fold array would silently address a different hook post-fold.

Acceptance evidence

1. The reported path names a key that exists in the author's source

Repro fixture (the card's own example):

hooks: [{
  name: 'escalate',
  object: 'crm_case',
  events: ['afterUpdate'],
  handler: async (ctx) => {
    await ctx.api.object('crm_case').update({ id: ctx.input.id, is_escalated: true });
  },
}],

hooks[0].handler is a real key in this source (grep handler finds it);
hooks[0].body.source — the old path — is not.

2. os build and os lint agree, verified on the built CLI (not just unit tests)

os lint --json:

{
  "severity": "error",
  "rule": "hook-api-update-readonly-field",
  "message": "hook \"escalate\" > body: body writes field 'is_escalated' through ctx.api.object('crm_case').update(...), and object 'crm_case' declares it readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so on every non-system trigger the engine strips readonly keys from that UPDATE payload - the write never lands, while the call still returns success. (judged on the metadata body lowered from the inline handler)",
  "path": "hooks[0].handler",
  "fix": "If automation is meant to maintain 'is_escalated', stamp it on the record's OWN hook - ctx.input.is_escalated = ... in beforeInsert/beforeUpdate survives the strip, and is the recommended shape. To keep writing it CROSS-OBJECT from here, declare runAs: 'system' on this hook: the strip skips a system context, so the write lands, and the triggering user is still stamped on the record. Note that ctx.api.sudo() is NOT an option from a body: sudo() lives on the in-process ScopedContext and is not marshalled into the sandbox, so calling it here is a TypeError at run time. Otherwise drop readonly:true from 'is_escalated'."
}

os build --json:

{
  "severity": "error",
  "rule": "hook-api-update-readonly-field",
  "where": "hook \"escalate\" > body",
  "path": "hooks[0].handler",
  "message": "body writes field 'is_escalated' through ctx.api.object('crm_case').update(...), and object 'crm_case' declares it readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so on every non-system trigger the engine strips readonly keys from that UPDATE payload - the write never lands, while the call still returns success. (judged on the metadata body lowered from the inline handler)",
  "hint": "If automation is meant to maintain 'is_escalated', stamp it on the record's OWN hook - ctx.input.is_escalated = ... in beforeInsert/beforeUpdate survives the strip, and is the recommended shape. To keep writing it CROSS-OBJECT from here, declare runAs: 'system' on this hook: the strip skips a system context, so the write lands, and the triggering user is still stamped on the record. Note that ctx.api.sudo() is NOT an option from a body: sudo() lives on the in-process ScopedContext and is not marshalled into the sandbox, so calling it here is a TypeError at run time. Otherwise drop readonly:true from 'is_escalated'."
}

rule / path / message / hint↔fix are identical between the two
commands (lint's message is ${where}: ${message} and fix is build's
hint — that envelope difference predates this change, per #16095). Both
runs made against the built CLI (node bin/run.js …), exit code 1 on both.

3. Verdict unchanged

HOOK_API_UPDATE_READONLY_FIELD / HOOK_API_UPDATE_READONLY_WHEN_FIELD
(#13653) and HOOK_BODY_WRITE_UNKNOWN_FIELD /
HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR (#4271) — which hooks are flagged, at
what severity, why — are byte-for-byte unchanged except for path and the
appended suffix. No branch of the judgement (the readonly strip channel
distinction, the runAs: 'system' exemption, the field-existence walk) was
touched.

4. Non-regression control (verified on the built CLI too)

The identical hook, authored with an explicit body instead of a handler,
keeps path: hooks[0].body.source on both os lint and os build —
unmoved. (hooks[0].handler reported hook-api-update-readonly-field for
the handler fixture above; the body-authored control below reports the
unchanged path for the same rule.)

$ node bin/run.js lint repro-control/objectstack.config.ts --json | … path
hooks[0].body.source
$ node bin/run.js build repro-control/objectstack.config.ts --json | … path
hooks[0].body.source

Clause-②

Clause-②: no

path's documented purpose — as this very card states it — is to name the
location responsible for a finding, so an author (or an editor/CI
integration reading it) can act on it. That purpose, and the field's shape
(a string on every AuthoringFinding), is unchanged: no key was added or
removed from --json, and message/hint keep their existing type too.
What changes is the value path carries for one previously-broken
subclass (a hook whose body never existed in the author's source at all)
— from a key that could never be resolved against the author's own file to
one that can. This is a correction toward the field's own already-declared
contract, not a widening of the accepted input set or the published
surface: no new field, no new accepted shape, no relaxed validation. A
consumer parsing path as "a locator into the author's source" gets a
more accurate answer than before for this subclass, never a differently
shaped one.

Lane / spillover

The two validators and the ctx.loweredHookRefs plumbing through
AuthoringRuleContext / runAuthoringRules / the reference-integrity suite
are all in packages/lint — the assigned lane.

Declared cross-domain spillover: the marker's source —
LoweringResult.loweredHookRefs — lives in packages/cli/src/utils/lower-callables.ts, outside packages/lint. This was necessary: lowerCallables
is the only place that knows whether a given hook's body was minted from a
handler or authored directly, and duplicating that judgement inside
packages/lint would re-derive a fact lowerCallables already computed
(and could drift from it). Touched cli-side files, all wiring only (no new
lowering logic): packages/cli/src/utils/lower-callables.ts,
src/commands/compile.ts, src/commands/lint.ts, src/commands/validate.ts,
src/utils/scaffold-validate.ts, plus their tests.

Tests

pnpm --filter @objectstack/lint exec vitest run \
  src/validate-hook-body-writes.test.ts src/validate-readonly-hook-writes.test.ts \
  src/reference-integrity-suite.test.ts src/authoring-rule-wiring.test.ts \
  src/authoring-rule-input-tier.test.ts
  → 5 test files, 179 tests, all passed

pnpm --filter @objectstack/cli exec vitest run --project unit \
  test/lower-callables.test.ts test/lint-hook-rules-reach-handler-hooks.test.ts
  → 2 test files, 21 tests, all passed

pnpm --filter @objectstack/lint --filter @objectstack/cli run typecheck
  → both packages: tsc --noEmit clean, check:test-typecheck clean (debt ledger unchanged)

packages/cli's integration vitest tier was not run: this diff touches no
integration-layer files (no bin/, no spawn helper) per
packages/cli/vitest-tiers.ts's own partition, so it is declared to CI.

Build

pnpm --filter '@objectstack/lint^...' build — dependency-closure build,
exit 0 (covers @objectstack/cli and every other downstream consumer of
@objectstack/lint).

Derived gate sweep

node scripts/pm/dispatch-gates.mjs --commands derived 63 gate families from
this diff. Ran all 63 (self-tests included): 60 passed (exit 0).

The remaining 3 read PREREQUISITE NOT MET (exit 3, per each gate's own
wording — "NOT a pass and NOT a finding: nothing was measured"), all for
the identical reason: they require a full pnpm build across the whole
workspace (studio, client-react, the connector packages, etc.), not just this
diff's dependency closure — squarely CI's job, not local scope:

  • check:dual-build-cjs-loads — no packages/apps/studio/dist,
    no packages/client-react/dist, and 10 more unrelated packages.
  • check:i18n-coverage — a partial lint round cannot judge the ratchet
    ("an unmeasured config is indistinguishable from a deleted one").
  • check:type-check-debt — --re-measure needs @objectstack/spec's
    built .d.ts resolved through the FULL workspace closure, not this
    diff's targeted one, or it "measures a different world" by its own
    wording.

None of the 3 name a file this diff touches. NOT MEASURED, left to CI.

Changeset

packages/lint and packages/cli both publish dist
(check:published-files green) and this changes an exported function's
behaviour (validateReadonlyHookWrites, validateHookBodyWrites) plus a
published CLI command's --json output — a real shipped behaviour change,
not internal-only. .changeset/hook-write-set-finding-path-lowered-handler.md
bumps both patch.

skip-changeset

Not applied — a changeset is owed and included (see above).


Generated by Claude Code

…[i].handler, not the un-authored body.source (#16546)

lowerCallables now records which hooks[*].handler refs had their body
minted from the inline handler it replaced; validateReadonlyHookWrites
and validateHookBodyWrites redirect path (+ a message suffix) there via
one shared helper, so os build and os lint keep agreeing word for word.
Author-written bodies are unaffected; no verdict moved.

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/cli, @objectstack/lint, touching 18 documentable anchor(s).

30 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 5d527f7e177473ee7dafdf00e277b1cded3daf42.

⛔ 8 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: os validate (command, 49 pages)
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 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 — 26 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 5d527f7e177473ee7dafdf00e277b1cded3daf42 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 001d5513fe813b4499f414c7beade2f569eb6f4f — the merge of head d78a056a5e8dc42a351687d93044bb69e54c5fe7 into base 5d527f7e177473ee7dafdf00e277b1cded3daf42, 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 001d5513fe813b4499f414c7beade2f569eb6f4f && git checkout 001d5513fe813b4499f414c7beade2f569eb6f4f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5d527f7e177473ee7dafdf00e277b1cded3daf42 d78a056a5e8dc42a351687d93044bb69e54c5fe7 && git checkout -B drift-repro 5d527f7e177473ee7dafdf00e277b1cded3daf42 && git merge --no-ff d78a056a5e8dc42a351687d93044bb69e54c5fe7

node scripts/docs-audit/affected-docs.mjs --json 5d527f7e177473ee7dafdf00e277b1cded3daf42

⚠️ 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 5d527f7e177473ee7dafdf00e277b1cded3daf42 → 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 Sep 10, 2026
@baozhoutao
baozhoutao marked this pull request as ready for review September 10, 2026 14:17
@baozhoutao
baozhoutao added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit e958468 Sep 10, 2026
35 checks passed
@baozhoutao
baozhoutao deleted the claude/issue-16546-lowered-hook-finding-path branch September 10, 2026 15:10
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/m tests tooling

Projects

None yet

2 participants