Skip to content

fix(cli): a renamed destructuring key off ctx is a key, not a free identifier - #22489

Merged
objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-22459-hook-lowering-destructuring-alias
Oct 9, 2026
Merged

objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/issue-22459-hook-lowering-destructuring-alias

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Fixes #22459

Clause-②: no
The rule's published text negates this refusal: its own remedy line reads "Inline the value(s) into the handler, or reach them through ctx." (packages/cli/src/lint/hook-body-lowering.ts:163), and const { previous: prev } = ctx reaches the value through ctx. Removing a misrejection that the rule's text already denies is no.

What changed

The free-identifier scan behind objectstack lint's hook-body/not-lowerable rule and objectstack build's hook lowering (packages/cli/src/utils/detect-free-identifiers.ts) counted a renamed destructuring key as a value reference. For a hook that destructures a renamed key off ctx, such as const { event, input, previous: prev } = ctx, it reported previous as free. The binding walk (collectBindings) already bound the alias prev. The reference walk (collectReferences) then met the element's propertyName as a plain identifier. So lint refused a lowerable hook with advice describing what it already did, and the build bundled it instead of lowering it.

collectReferences now handles a BindingElement the way it already handles a PropertyAssignment. A non-computed propertyName is a key, not a reference. The walk still visits a computed key's expression, the binding name (an alias is subtracted by bindings; a nested pattern recurses), and the default, because { [k]: v = d } reads k and d. It is one branch in the existing walker, with no second walker. extract-hook-body.ts, the rule's message text and packages/spec are untouched.

Changeset: .changeset/22459-hook-lowering-destructuring-alias.md, @objectstack/cli patch. The remedy is: none needed, because a hook written this way now lowers.

Reproduction, red then green

  • Red. The new pins, run against the unmodified collectReferences (base f66c440de): detect-free-identifiers.test.ts has 8 failed and 37 passed (45). The failures read expected [ 'previous' ] to deeply equal [] (the card's A shape, an object pattern inside an array pattern, a nested callback parameter, and the compiled .toString() shape), [ 'a', 'b' ] (nested), [ 'key' ] (defaulted alias), [ 'message' ] (catch clause), and [ 'FALLBACK', 'key' ] where the control expects [ 'FALLBACK' ].
  • Green at 38fa4d085: detect-free-identifiers.test.ts and hook-body-lowering.test.ts give 63 passed (63).
  • Ablation (one-shot, run from the committed fix). The new branch was neutralised on disk with scripts/ablation-replace.mjs: anchor hit 1 time and went from 1 to 0, and the blob went b768875ce125 to dcd006617c08. Both suites then gave 9 failed and 54 passed (63). The ninth failure is the lint/build pin: expected [ { severity: 'error', …(3) } ] to deeply equal []. On restore, the blob matched HEAD (b768875ce125) and git diff HEAD was empty.

Pins, as triage directed:

  • the card's A shape ({ event, input, previous: prev } = ctx) lowers;
  • nested ({ a: { b: c } } = ctx) and defaulted ({ key: alias = d }, d in scope) shapes lower, and so does a computed key with an in-scope default;
  • control: a defaulted alias whose default is free still reports FALLBACK (also the shorthand { previous = FALLBACK }), and a free computed key still reports KEY;
  • control: a genuinely free previous (no ctx source) is still reported, at the helper and at lint/build level.

H2 and H3 (measured)

  • Array destructuring (const [first, second] = ctx.items): no false positive before the fix. It was green on base, because an array element has no key. Parameter destructuring: the handler's own top-level parameters never had it, because the reference walk does not visit top-level parameter patterns (the existing ({ a: { b } }) => b + 1 pin was green on base). The same element shape inside the body DID have it: ctx.items.map(({ previous: prev }) => prev.n), const [{ previous: prev }] = ctx.items, and catch ({ message: msg }) were all red on base. The same rule covers them, and each is pinned.
  • End to end (hook-body-lowering.test.ts): for a hook that destructures a renamed key off ctx, checkHookBodyLowering returns []. lowerCallables records no extraction warning, bodyExtracted is 1, and the lowered body.source carries previous: prev. The control hook reads a module-scope previous. It is still an error under hook-body/not-lowerable at hooks[0].handler, and the build records ['free-identifiers', ['previous']] and lowers nothing. Under the ablation the positive pin went red; the control stayed green both ways.

Verification, at 38fa4d085

  • pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2: 272 files passed. 2 files failed on a prerequisite, not a verdict ("packages/cli is not built (./dist/index.js is absent)"): published-subpath-hook-body.pin and published-subpath-console.pin. After pnpm --filter @objectstack/cli build, those two gave 29 passed (29). Totals: 274 of 274 files, 4023 + 29 tests passed, 29 skipped. The integration layer is declared to CI, because the diff touches no integration-layer file and no spawn entry.
  • pnpm --filter @objectstack/cli typecheck: exit 0. tsc -p tsconfig.json --listFilesOnly includes both edited test files (count 2).
  • Lint, as a proven narrowing rather than the full pnpm lint. (1) Population, read from eslint.config.mjs through the ESLint API: none of the 3 changed TS files is ignored. The changeset .md is outside every files glob. (2) Count, from --format json: 3 files, 0 errors, 0 warnings. (3) Invariance: the config enables no type-aware linting (each file's resolved parserOptions is { ecmaVersion, sourceType } only, with no project), and the diff touches no ESLint config, plugin or baseline. So no untouched file's verdict can move.
  • Gates: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (no paths) derives 64 commands; all 64 exit 0. Four of them (check:dual-build-cjs-loads, check:i18n, check:i18n-coverage, check:i18n-walk-parity) first exited 3, PREREQUISITE NOT MET, and exited 0 after the build. --ran reconciles: 64 derived, 64 run, 0 NOT-MEASURED, 0 UNRUN.

Acceptance notes

  • Observation, not filed (reach not measured). Lowering drops the handler's own parameter list. The runner wraps a hook body as (async (ctx) => { … })(ctx), and the scan treats the handler's top-level parameters as bound. Measured at 38fa4d085, a pre-existing path this PR does not touch: extractHookBody(({ input }) => { input.x = 1; }) lowers to the source input.x=1, and (c) => { c.input.x = 1; } lowers to c.input.x=1. checkHookBodyLowering reports 0 issues for both. Relatedly, the reference walk never visits a top-level parameter pattern's default: detectFreeIdentifiers('({ x = FREE }) => x') gives [], while the in-body spelling gives ['FREE']. NOT MEASURED: whether such a body throws when the runtime evaluates it. Carrier: none.

Generated by Claude Code

claude added 2 commits October 9, 2026 12:20
…entifier

The hook-body free-identifier scan walked a BindingElement's propertyName
as a value reference, so `const { previous: prev } = ctx` reported
`previous` as free: `objectstack lint` refused a lowerable hook with advice
describing what it already did, and `objectstack build` bundled it instead
of lowering it. The reference walk now treats a non-computed destructuring
key the way it already treats a `{ key: value }` key, and still walks a
computed key's expression and the element's default.

Claude-Session: https://claude.ai/code/session_01BmsuLyUeuG5CNpZFMH1jzS
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 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; 97 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.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 28 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 8b713fad783e23d4313fa7d291b88de0bde6efd7 → packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 8b713fad783e23d4313fa7d291b88de0bde6efd7

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

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

1 participant