Repository navigation
fix(lint): hook write-set findings on a lowered hook point at hooks[i].handler - #17438
Conversation
…[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
📓 Docs Drift CheckThis PR changes 2 package(s): 30 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 8 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 26 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # 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
|
Closes #16546
What changed
lowerCallablesnow records, per call, whichhooks[*].handlerref stringshad their
bodyminted here — extracted from the author's inlinehandlerfunction — as opposed to abodythe author wrote directly(
LoweringResult.loweredHookRefs). The CLI's four lowering doors (os buildunion run,
os buildper-package run,os lint,os validate, andos init/dev's scaffold validation) thread that set throughrunAuthoringRules'sctx.loweredHookRefs(AuthoringRuleContext,@objectstack/lint).validateReadonlyHookWritesandvalidateHookBodyWrites(
packages/lint/src/validate-readonly-hook-writes.ts/validate-hook-body-writes.ts) share one new helper,hookBodyFindingLocation(hook, hookIndex, ctx), which both call beforepushing a finding: when the hook's ref is named in
ctx.loweredHookRefsitreturns
path: hooks[i].handler(the key that replaced the function theauthor wrote) plus a message suffix —
" (judged on the metadata body lowered from the inline handler)"— otherwise it returns the unchangedhooks[i].body.source. One implementation, soos buildandos lintcannot disagree about it (#16095's rule, applied one layer down).
Why
path: hooks[i].handler, not an indexThe marker is a ref string (
hook.handler's value after lowering), nota hook index.
lowerCallables'staken/refByFnregistries are sharedacross the whole walk (top level and every ADR-0130 D4 package body), so a
ref is unique for one
lowerCallables()call and — becausehandleris anordinary string field — survives a Zod
safeParseunchanged. An index wouldnot:
authoringRuleUnionStackfolds package hooks into a new top-level arraywith 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
pathnames a key that exists in the author's sourceRepro fixture (the card's own example):
hooks[0].handleris a real key in this source (grep handlerfinds it);hooks[0].body.source— the oldpath— is not.2.
os buildandos lintagree, 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↔fixare identical between the twocommands (lint's
messageis${where}: ${message}andfixis build'shint— that envelope difference predates this change, per #16095). Bothruns 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, atwhat severity, why — are byte-for-byte unchanged except for
pathand theappended suffix. No branch of the judgement (the readonly strip channel
distinction, the
runAs: 'system'exemption, the field-existence walk) wastouched.
4. Non-regression control (verified on the built CLI too)
The identical hook, authored with an explicit
bodyinstead of ahandler,keeps
path: hooks[0].body.sourceon bothos lintandos build—unmoved. (
hooks[0].handlerreportedhook-api-update-readonly-fieldforthe handler fixture above; the body-authored control below reports the
unchanged path for the same rule.)
Clause-②Clause-②: no
path's documented purpose — as this very card states it — is to name thelocation 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 orremoved from
--json, andmessage/hintkeep their existing type too.What changes is the value
pathcarries for one previously-brokensubclass (a hook whose
bodynever 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
pathas "a locator into the author's source" gets amore accurate answer than before for this subclass, never a differently
shaped one.
Lane / spillover
The two validators and the
ctx.loweredHookRefsplumbing throughAuthoringRuleContext/runAuthoringRules/ the reference-integrity suiteare all in
packages/lint— the assigned lane.Declared cross-domain spillover: the marker's source —
LoweringResult.loweredHookRefs— lives inpackages/cli/src/utils/lower-callables.ts, outsidepackages/lint. This was necessary:lowerCallablesis the only place that knows whether a given hook's
bodywas minted from ahandler or authored directly, and duplicating that judgement inside
packages/lintwould re-derive a factlowerCallablesalready 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
packages/lint/src/validate-readonly-hook-writes.test.ts/validate-hook-body-writes.test.ts: new unit coverage forhookBodyFindingLocationand the two rules' redirect + non-regressioncontrol, direct (no CLI).
packages/cli/test/lower-callables.test.ts:loweredHookRefsis set onlywhen a body was actually minted from a handler (not on extraction failure,
not when an author-written
bodyalready existed).packages/cli/test/lint-hook-rules-reach-handler-hooks.test.ts: updatedthe lint: every
hook-body-*/hook-api-update-readonly-*rule is unreachable for a hook authored as ahandlerfunction — the whole family is gated onbody.language === 'js', and the reference CRM has 39 hooks and 0 bodies #16095 RED/CONTROL pins to assert the correctedpath(and added onefor the scaffold-validate door), verdict assertions untouched.
packages/cli/test/lint-hook-rules-reach-handler-hooks.e2e.test.ts: notrun locally — it is outside both the
unitandintegrationvitestprojects (a separate e2e suite spawning the real CLI); its assertions
check rule ids / exit codes only, not
path, so it is unaffected eitherway. Left to CI.
packages/cli'sintegrationvitest tier was not run: this diff touches nointegration-layer files (no
bin/, no spawn helper) perpackages/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/cliand every other downstream consumer of@objectstack/lint).Derived gate sweep
node scripts/pm/dispatch-gates.mjs --commandsderived 63 gate families fromthis diff. Ran all 63 (self-tests included): 60 passed (exit 0).
The remaining 3 read
PREREQUISITE NOT MET(exit 3, per each gate's ownwording — "NOT a pass and NOT a finding: nothing was measured"), all for
the identical reason: they require a full
pnpm buildacross the wholeworkspace (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-measureneeds@objectstack/spec'sbuilt
.d.tsresolved through the FULL workspace closure, not thisdiff'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/lintandpackages/cliboth publishdist(
check:published-filesgreen) and this changes an exported function'sbehaviour (
validateReadonlyHookWrites,validateHookBodyWrites) plus apublished CLI command's
--jsonoutput — a real shipped behaviour change,not internal-only.
.changeset/hook-write-set-finding-path-lowered-handler.mdbumps both
patch.skip-changesetNot applied — a changeset is owed and included (see above).
Generated by Claude Code