Skip to content

fix(cli): report only what the hook-body build gates establish - #10912

Merged
os-elon merged 1 commit into
mainfrom
claude/issue-10678-hook-body-gate-honesty
Aug 21, 2026
Merged

fix(cli): report only what the hook-body build gates establish#10912
os-elon merged 1 commit into
mainfrom
claude/issue-10678-hook-body-gate-honesty

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Part of #10678

Three defects on the hook-body build-gate surface, each ruled separately by triage. All three are one shape: a gate reporting something it never established. The enforcement net itself held throughout and is unchanged — no forbidden body ever shipped as body.source, and every forbidden or free-identifier hook is still refused under --strict-body, at the same exit codes as before. This is reporting and reachability.

Part of rather than Fixes: defects 1 and 3 each leave a fenced maintainer decision open (below), so the card should not close on merge.

All three reproduced at head before any edit

Anchors re-derived at head (7c02a4529c), measured by spawning the real CLI against mkdtemp projects:

DefectMeasured at head
1 · @capabilities dead through os buildhandler asking for api.read api.write shipped "capabilities": []
2 · require() reason deadrefusal fired, but as handler references identifier(s) not in scope at runtime: __require
3 · default build silentfetch() in a hook body: exit 0, no output at all

Defect 3 — surface the recorded warnings (the highest-value one)

bodyExtractionWarnings was already recorded by lowerCallables and printed nowhere. compile.ts now prints it on the default path, naming the hook and the pattern, and pointing at --strict-body; --json carries it under a new bodyExtractionWarnings key.

Separate key from warnings on purpose: warnings carries author-time rule advisories in the shape os validate --json also reports ({where, message, rule, path, hint}), and these are a different record ({origin, reason}). Folding them together would have broken that shared shape for every consumer reading one shape from either command. It is an empty array on a clean build, so CI can read it unconditionally — pinned by a control test.

Exit code unchanged (still 0). Flipping the default build to hard-fail would change what os build accepts — fenced, raised in the report instead.

Defect 2 — the require() reason now fires on the real path

esbuild's ESM interop shim rewrites require('node:os') to __require("node:os") before String(fn) runs, so /\brequire\s*\(/ could never match on a TS config. Now /\b(?:__)?require\s*\(/, and the reason explains the rewrite so __require in the dumped source is not a mystery identifier.

Accept behaviour unchanged, and tested as such: the body was already refused (via the #1876 free-identifier gate), already bundled, at the same exit codes — 1 under --strict-body, 0 by default. Only the wording moved. The widening cannot reach an author's own myrequire( (no word boundary inside it); a test pins that.

The sibling import() pattern was checked for the same class and is fine — esbuild preserves the import( spelling in its rewrite, so that reason still fires. Nothing to file.

Defect 1 — docs-truth route, and the premise came back stronger

Triage said "document that the directive only survives pre-bundled JS." Measured, it survives nothing that an app author would write.bundleRequire runs esbuild on every config regardless of extension, and esbuild strips // line comments before the handler is a runtime value:

Authoring shapedirective reaches the extractor?
objectstack.config.tsNo
objectstack.config.jsNo
objectstack.config.mjsNo
handler imported from a local ./handlers.jsNo

So the docs do not point readers at a .js file that also does not work — that would have been a fresh false claim of exactly the kind this card is about. They state the reach as measured and point at body.capabilities (data, not a comment) as the escape hatch that does survive — itself measured, not assumed.

The os build-level test, and why it had to be one

test/hook-body-build-reach.e2e.test.ts spawns the real CLI (bin/run-dev.js + tsx, the validate-top-level-strict.e2e.test.ts pattern) and reads the artifact and the shell's exit code.

The existing extractor unit tests could not have caught any of this: they feed raw JS function literals, which keep their comments and their require( spelling because nothing transformed them. That is why honours explicit @capabilities override has passed for as long as it has existed while the override never once worked through os build. A unit test over the extractor alone would have restated exactly that false confidence.

The defect-1 assertions pin reach, not endorsement — the file says so, and says that if the fenced decision lands they must be rewritten alongside the docs page, not deleted.

Reverse-verification

Both fix legs were reverted on top of the commit and re-run; the restore leg was re-run to green. Predicted direction was red, and red is what happened, each for the right reason:

× rejects __require() ... expected to throw /`require\(\)` is not allowed/ but got 'handler references identifier(s) not in scope...'
× --strict-body names require() ... output lacked the require()-specific reason
× prints the recorded warning ... output lacked "hook 'hb_forbidden'" (and showed "Build complete")
× carries the warnings in --json ... bodyExtractionWarnings undefined
× and says nothing about it (control) ... expected undefined to deeply equal []
Test Files 2 failed | Tests 5 failed | 21 passed (26)

Restore leg: Test Files 2 passed (2) · Tests 26 passed (26).

No rebuild was needed for either leg and none is claimed: bin/run-dev.js runs the CLI from src/ through tsx — its own header says it exists "so they do not depend on packages/cli/dist having been built" — and the unit test imports from src. Nothing in this ablation resolves through dist.

The defect-1 reach test stayed green under ablation, correctly: it pins pre-existing measured behaviour and has no code mutation to be sensitive to. Its regression sensitivity is to a future change in reach.

Verification

Head is 4edd41f194. Being exact about which tree each run saw, since two of them predate the final amend:

  • 29 gates green at 4edd41f194, re-run on the final head. Derived with node scripts/pm/dispatch-gates.mjs against the real change set — no hand-built path list — which is 27 families, plus check:nul-bytes and the two convention-triggered families the residue names for new test files (check:engine-double-contract, check:where-matcher). os-verify-lock: VERDICT command-exit 0.
  • check:type-check-debt — the ratchet half — run after turbo run build over the workspace closure (70 successful, 70 total), as lint.yml does. Its own verdict line: check-type-check-coverage: OK — 64/77 workspace packages type-checked (plus the root), 13 in the DEBT ledger. TCDEBT_EXIT=0.
  • packages/cli full suite: 147 files / 1622 tests passed, 0 failed, plus pnpm --filter @objectstack/cli typecheck clean. (The -- file filter was not honoured, so the whole package ran — more coverage than intended, not less.)

The suite and ratchet runs both saw commit 5b09f67a9e; the amend to 4edd41f194 added one prose line to hook-bodies.mdx and changed nothing else. That line is in no tsc program and in no test, so neither reading moves — and the docs families that can see it (check:doc-anchors, check:doc-authoring, check:doc-frontmatter, check:role-word, check:docs-audit-scope) were re-run on the amended tree before the gate batch, and again inside it. check:doc-anchors is what confirms the new #capability-inference fragment link resolves.

Regression set from the card (capability inference, the crypto.hash non-inference of #4391, the #1876 bundle fallback, all-body-only skip plus .mjs cleanup) is green inside that full-suite run. No threshold moved, no baseline raised, nothing skipped or quarantined.

Two fenced decisions, not taken here

Both are maintainer calls about the published surface; both are in the report's open_questions with the evidence.

  1. Should @capabilities get a real authorable surface, or be retired? Today it is a documented feature reachable from no ordinary authoring path. Documenting it as unreachable is stable but leaves a directive in the docs that does nothing. Adding a config property expands the public surface.
  2. Should a forbidden pattern fail the default build? The header claimed it did; the code warns and bundles. This PR makes the header and the output match the code. Making the code match the old header instead would change what os build accepts.

Nothing under content/docs/releases/** was touched; the changeset is the release-notes input.


Generated by Claude Code

Three defects on the hook-body build-gate surface, all reporting and
reachability — the enforcement net itself held throughout.
- Default os build recorded bodyExtractionWarnings and printed them
nowhere, so a forbidden pattern in a hook body was a silent exit-0
success. Surface them on stdout and in --json under a new
bodyExtractionWarnings key. Exit code unchanged.
- The require() refusal reason could never match on a TS config, because
esbuild rewrites require( to __require( before String(fn) runs. Match
both spellings under the one reason. Accept behaviour unchanged.
- The // @capabilities directive reaches the extractor from no ordinary
authoring shape through os build. Document the measured reach and pin
it with an os build-level test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 4 documentable anchor(s).

16 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 58563be069a70d09c4d9cc5f522bb27c5f8a78b2.

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

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 23 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 58563be069a70d09c4d9cc5f522bb27c5f8a78b2packageMentionDocs.

Which tree this was computed on

This run read content/docs from 36e878f452fcb03000db4636b297bb5e825837ba — the merge of head 4edd41f194e87cf1c440466d41594f754246338f into base 58563be069a70d09c4d9cc5f522bb27c5f8a78b2, 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 36e878f452fcb03000db4636b297bb5e825837ba && git checkout 36e878f452fcb03000db4636b297bb5e825837ba
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 58563be069a70d09c4d9cc5f522bb27c5f8a78b2 4edd41f194e87cf1c440466d41594f754246338f && git checkout -B drift-repro 58563be069a70d09c4d9cc5f522bb27c5f8a78b2 && git merge --no-ff 4edd41f194e87cf1c440466d41594f754246338f
node scripts/docs-audit/affected-docs.mjs --json 58563be069a70d09c4d9cc5f522bb27c5f8a78b2

⚠️ 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 58563be069a70d09c4d9cc5f522bb27c5f8a78b2 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32508204346 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 5.85s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 39 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@os-elon