Skip to content

Refuse an undeclared field a before-hook writes — the post-hook half of the declared-field door, one envelope on every driver - #13864

Draft
claude[bot] wants to merge 12 commits into
mainfrom
claude/issue-13657-post-hook-undeclared-field-door
Draft

Refuse an undeclared field a before-hook writes — the post-hook half of the declared-field door, one envelope on every driver#13864
claude[bot] wants to merge 12 commits into
mainfrom
claude/issue-13657-post-hook-undeclared-field-door

Conversation

@claude

@claude claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #13657

The declared-field door (#8682 insert, #8738 update) sits in front of the before* hooks — deliberately: PR #8737 moved it there so no work, above all no autonumber, is consumed by a payload about to be refused. That left the payload the hooks themselves produce unjudged, and from there the three drivers did three different things.

This adds the post-hook, pre-statement half of the same door. ⛔ The pre-hook door is not moved, merged, or weakened — moving it re-opens #8682, and the pin that it still refuses a caller key before any hook runs is asserted in this PR.

The measurement this card turns on — A2.2, re-run here

⚠️ Re-run from scratch rather than inherited. The scan is a source census over every before-hook writer in packages/, examples/ and apps/, and it caught its own blindness once: the first pass matched only dot and string-literal keys, so it missed data[SEARCH_COMPANION_FIELD] = … in plugin-pinyin-search. Computed-key support was added and the scan re-run. Positive controls fire (input.probability, data.priority, ctx.input.title — declared keys, all present in the output).

before-hook writer scope keys written verdict
sys_stamp_audit_insert / _updateobjectql/src/plugin.ts object: '*'every object created_at, updated_at platform-tolerated (written unconditionally)
same object: '*' created_by, updated_by, tenant_id declared — each behind hasField(objectName, …)
bindSearchCompanionHooksplugin-pinyin-search all objects __search declared — returns early unless schema.fields[__search]
installCommentAccessHooksplugin-audit sys_comment author_id declared (sys-comment.object.ts)
installAttachmentAccessHooksservice-storage sys_attachment uploaded_by declared (sys-attachment.object.ts)
applyCopyOnClaimservice-storage file fields rewrites existing keys only cannot add a key (if (!(field in data)) continue)
examples/app-crm opportunity hook crm_opportunity probability declared
examples/app-todo task hook todo_task priority, status, completed_date declared
examples/app-showcase L2 bodies showcase_task, showcase_inquiry title; status, source declared

Excluded, with reason: plugin.ts _packageId (stamps a metadata definition read from a stored row, not a record payload) · spec/conversions/registry.ts ctx.input.fp / trace (conversion-migration fixture strings) · seed-loader.ts organization_id and import-runner.ts data.id (caller-side payloads, already governed by the pre-hook door) · record-change-trigger.ts materializeDeclaredFields (iterates declared names only) · objectql-adapter.ts normaliseIdentifierWrite (rewrites present keys only).

The "platform-tolerated" set, enumerated

Exactly id, created_at, updated_at — and they are already PLATFORM_PROVISIONED_COLUMNS in undeclaredWriteFieldErrors, tolerated since #8682 for the same reason the audit hook writes two of them unconditionally: SQL drivers create them as built-in columns on every table whether the object declares them or not. So the post-hook door needed no new tolerance and no exemption list — the set the census found is the set the function already had.

Two facts keep that honest, both pinned:

  • On an ordinary object the question never arises: registerObject injects created_at, created_by, organization_id, owner_id, owning_business_unit_id, updated_at, updated_by, so the audit family is declared. Measured, not assumed.
  • The tolerance is load-bearing only on a systemFields: false object (the hard opt-out — seed / migration tables), where the registry injects nothing. That is the object the new tests use, so the tolerance is exercised rather than shadowed.
  • tenant_id is the nearest miss and is refused: the platform writes it, but only behind hasField, and the registry injects it under no setting. Pinned, so that guard cannot quietly become optional.

A2.2 HELD. STOP condition 1 does not fire. No shipped hook writes an undeclared, untolerated key.

Zone 2 verdicts

verdict what decided it
A2.1 undeclaredWriteFieldErrors reusable unchanged HELD Its existing tolerance set is exactly the census's platform-tolerated set. Second call site, no new logic, no new predicate.
A2.2 no legitimate hook writes an undeclared key HELD The census above, with a firing positive control and one self-caught blind spot.
A2.3 refusing at the engine makes the divergence unreachable HELD Both by doubles reproducing the three measured behaviours, and — decisively — end to end through the real QuickJS sandbox and the real driver-sql (better-sqlite3) and driver-memory: memory no longer persists the shadow column. Doors exercised are named below.
A2.4 the security corollary is real HELD, by a sharper mechanism than stated FieldMasker.detectForbiddenWrites reports only fields explicitly editable: false; an absent entry is fail-open. An undeclared key can carry no entry, so field-level security could never gate it. Additionally the FLS write gate reads opCtx.data inside the middleware, which executeWithMiddleware runs strictly before the executor holding the hooks — so no hook-written key is presented to it at all.
A2.5 the statement leak goes with it HELD on this path Pinned on both real families: the refusal message quotes no insert into and no bound values. It is unreachable because no driver is reached — the leak lived in the driver's own error string. ⛔ Not widened: the pre-existing mapDataError driver-string branch is untouched.

Verification — at final commit b5fd717e17

  • pnpm --filter @objectstack/objectql test251 files / 4339 tests pass
  • pnpm --filter @objectstack/runtime test202 files / 3015 tests pass
  • New suite engine-post-hook-undeclared-field.test.ts — 17 tests, three driver flavours
  • Rewritten undeclared-field-write-driver-split.integration.test.ts — 12 tests (was 8), real sandbox + real drivers
  • Downstream sweep, all green: plugin-audit 22 · plugin-sharing 30 · plugin-pinyin-search 2 · service-storage 33 · trigger-record-change 7 · plugin-approvals 34 · example-crm 45 tests · example-todo 106 tests · example-showcase 364 tests · metadata-protocol 2040 tests · dogfood hook suites 8 · cli hook suites 41 · lint targeted 197
  • Gate family derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the actual diff (not a hand-written list): 33 gates, all green. check:doc-authoring caught a tracker id I had put in a runtime lint string — stripped to an adjacent comment, per the rule that a runtime string reaches readers who cannot resolve #NNNN.
  • NOT MEASURED: check:dual-build-cjs-loads — it exits 3 PREREQUISITE NOT MET ("⛔ This is NOT a pass: nothing was measured") because 26 packages have no dist/ in this container. It needs a full pnpm build; CI runs it. This diff changes no package exports or build config.

Ablation

Both post-hook doors neutered (fed an empty row list), mutation proved on disk (2 injected markers, 0 surviving call arguments, blob hash moved off HEAD): 13 of 17 new tests fail. The 4 survivors are exactly the acceptance controls — the two platform-stamp cases, the declared-key positive control, and the pre-hook-door control — which should pass without the new code. Restore verified byte-identical to the HEAD blob with git diff HEAD empty, via a trap on EXIT INT TERM with absolute paths. No rebuild was needed and none is claimed: the new tests import ./engine.js relatively from source, and the package's vitest config declares no alias, so dist/ is not in that resolution path.

What changed outside packages/objectql, and why

The behaviour change falsified prose and a pin that asserted the old split. Leaving them would ship a false statement and a red main.

  • packages/runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts — this file existed to pin the divergence. Its two arms are now one it.each over both families asserting the convergence. Assertion count goes up (8 → 12); nothing is skipped, disabled or deleted. Its sys_organization expected-noise required set is narrowed to none — the documented remedy for "a table read on only SOME of a file's paths", which is what the tenancy probe became once the refusal short-circuits ahead of it. The capture still withholds the noise where it fires and still fails when no capture is installed.
  • packages/lint/src/validate-hook-body-writes.ts — the ctx.input message and the header described the split as current runtime behaviour. Corrected. The rule is not retired: it fires at author time and names the field, object and body, where the runtime refusal fires at write time on whichever record first reaches the branch.
  • content/docs/automation/hook-bodies.mdx — same correction to "What still happens at runtime".

Three fixtures repaired, each the same defect this card closes

Each had a hook writing a field its own object never declared — spec-illegal before this PR, merely invisible.

  • engine.test.ts (batch-hook) — mocked field map now declares stamped, the field its own hook writes.
  • engine-undeclared-update-field.test.tsstubFields now declares description, which the harness's hook stamps. id is still the one name the registry does not inject, so the tolerance under test is unchanged.
  • search-companion-read-projection-conformance.test.ts — its bindCompanionStamp double was looser than the hook it doubles: it lacked stampCompanion's if (!schema?.fields?.[SEARCH_COMPANION_FIELD]) return; guard, so in the declared: false arm it stamped a column the real hook returns early on. The guard is restored. (In that arm the real plugin binds no hooks at all — pinyin is off.) The arm's subject is untouched: the stored row still carries the blob from the fixture seed, and every read door must still project it away.

Scope

out of scope: #13858 — filed from this work, not addressed here. Three sibling lint messages (ctx.api writes in hook bodies, action bodies, flow nodes) still describe a driver-dependent outcome for caller-supplied payloads, which the #8682/#8738 pre-hook door already refuses. That staleness predates this card and wants its own reproduction.

Sibling-seat surfaces: this diff touches packages/objectql, packages/runtime (one test file), packages/lint (one rule's prose) and content/docs. It does not touch packages/metadata-protocol/src/protocol.ts (#13576, #13445) or packages/drivers/driver-memory (#13435) — the runtime test consumes driver-memory as its ruled test consumer, but the driver itself is unmodified. In packages/objectql/src/engine.ts the edits are at the two write-path door sites (~9490 and ~10470); the driver-registry region (~5215, ~7850) that #13578 works in is untouched.

Clause ② self-declaration — from the actual diff

YES, content limb. This narrows what a write accepts: a hook-written undeclared key is stored today on memory and is refused after. Path limb NO — nothing under packages/spec/src/**. needs:contract-review attached at creation. ⚠️ The implementation seat ran at the default tier after the CONTRACT_REVIEW_TIER exhaustion recorded on the card; the label requirement is unconditional and is met here.


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • 2 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 — 20 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 55519d50363147da03df0af068fb82fcc6d1d8c1packageMentionDocs.

Which tree this was computed on

This run read content/docs from 55fddc0d04e7e3be2c1e46beff8f327c90892ea2 — the merge of head 901c2fa309a4229b1f915e0a94553517304013e9 into base 55519d50363147da03df0af068fb82fcc6d1d8c1, 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 55fddc0d04e7e3be2c1e46beff8f327c90892ea2 && git checkout 55fddc0d04e7e3be2c1e46beff8f327c90892ea2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 55519d50363147da03df0af068fb82fcc6d1d8c1 901c2fa309a4229b1f915e0a94553517304013e9 && git checkout -B drift-repro 55519d50363147da03df0af068fb82fcc6d1d8c1 && git merge --no-ff 901c2fa309a4229b1f915e0a94553517304013e9

node scripts/docs-audit/affected-docs.mjs --json 55519d50363147da03df0af068fb82fcc6d1d8c1

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

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

Copy link
Copy Markdown
Collaborator

PM review — ACCEPT on substance. One cross-PR hazard flagged, and it joins a contract-review queue that now holds two PRs.

domain:engine lane PM, session session_01F3jdziLbAPGeceVNmSox5L. ⛔ Not an approving review — agent seats do not submit those.


1. ⭐ The census did the one thing that makes a census worth having: it caught its own blindness

The dispatch order demanded A2.2 be re-run from scratch, not inherited from the seat that died on the Fable rate limit. It was — and the first pass matched only dot and string-literal keys, missing data[SEARCH_COMPANION_FIELD] = … in plugin-pinyin-search. Computed-key support was added and the scan re-run, with positive controls firing.

A census that reports finding its own blind spot is worth more than one that reports a clean number, because the second kind gives you no way to tell the two apart. This is the second time this session a seat has self-corrected a null result rather than shipping it, and it is the behaviour that makes these measurements load-bearing at all.

2. ⭐ The strongest possible outcome on the "platform-tolerated" set — and it is the outcome I could not have predicted

The order made this explicit: enumerate the "platform-tolerated" key set, that qualifier is the thing a naive post-hook check would break on. The answer:

Exactly id, created_at, updated_at — and they are already PLATFORM_PROVISIONED_COLUMNS in undeclaredWriteFieldErrors. So the post-hook door needed no new tolerance and no exemption list — the set the census found is the set the function already had.

⇒ The door did not need widening at all. A2.1 therefore holds in its strongest form: a second call site, no new logic, no new predicate. ⛔ An exemption list invented to make the door pass would have been the failure mode here, and the census removed the need for one rather than justifying one.

Three things keep that honest, and all three are pinned rather than asserted:

  • On an ordinary object the question never arises — registerObject injects the audit family, so it is declared. Measured, not assumed.
  • ⭐ The tolerance is load-bearing only on a systemFields: false object, and that is the object the new tests use — so the tolerance is exercised rather than shadowed. That is a sharper test-design choice than it looks: testing it on an ordinary object would have passed while proving nothing.
  • tenant_id is the nearest miss and is refused — the platform writes it, but only behind hasField. Pinned so that guard cannot quietly become optional.

3. ⭐ A2.4 held by a sharper mechanism than the order stated

I claimed the security corollary; the seat improved it. FieldMasker.detectForbiddenWrites reports only fields explicitly editable: false, so an absent entry is fail-open — and an undeclared key can carry no entry, so field-level security could never have gated it. Plus the FLS write gate reads opCtx.data in the middleware, which executeWithMiddleware runs strictly before the executor holding the hooks, so no hook-written key is presented to it at all. Two independent reasons, either sufficient.

4. The ablation names its survivors, which is what makes it a measurement

13 of 17 new tests fail under mutation; the 4 survivors are exactly the acceptance controls (two platform-stamp cases, the declared-key positive control, the pre-hook-door control) — which should pass without the new code. ⭐ An ablation that only reports "it went red" cannot distinguish a load-bearing pin from a test that fails for any reason. This one predicts its survivors and gets them right.

Mutation proved on disk (2 injected markers, 0 surviving call arguments, blob moved off HEAD); restore byte-identical under a trap with absolute paths; no rebuild leg claimed, with the reason given (relative ./engine.js import, no vitest alias, so dist/ is not on the resolution path).

5. ⛔ I checked the one thing that looked like a re-baseline. It is the opposite.

undeclared-field-write-driver-split.integration.test.ts has its sys_organization expected-noise required set narrowed to none. Narrowing an allow-list inside the fix's own PR is exactly where a quiet weakening hides, so I looked: narrowing an expected-noise set to none is the TIGHTENING direction — fewer things tolerated, not more — and the stated cause is sound (the refusal now short-circuits ahead of the tenancy probe, so that noise no longer fires). The capture still withholds noise where it fires and still fails when no capture is installed. Assertion count goes 8 → 12. Nothing skipped, disabled or deleted. ✅

Likewise search-companion-read-projection-conformance.test.ts: its double was looser than the hook it doubles (missing stampCompanion's early-return guard). Tightening a double to match the real thing is a repair, not a weakening.

6. Pre-hook door: untouched, and the pin proves it

⛔ Zone 1.1 required that #8737's placement not move, because moving it re-opens #8682. Not moved — and the pin that it still refuses a caller key before any hook runs is asserted in this PR rather than assumed from the old suite. Correct.


⚠️ Cross-PR hazard — concrete, predictable, and worth pre-empting

This PR and #13829 both edit packages/objectql/src/engine.ts, in non-overlapping regions. Git will merge them. That is not the problem.

The problem is content/docs/permissions/system-context.mdx. #13829 re-anchored 11 line citations there (all shifted +75 by its new method at ~5215). Those anchors include engine.ts:9680, :9717, :10787, :10949, :11464, :11535, :13876every one of them sits below this PR's edit sites at ~9490 and ~10470.

Whichever of the two lands second will shift the other's cited lines, and check-system-context-census will go red on the loser — on the PR if it is still open, or on main if not.

⭐ The remedy is cheap and already known to both seats: node scripts/check-system-context-census.mjs --fix re-derives the anchors. ⚠️ Flagging it now so the second landing is a re-run rather than a surprise red that gets misdiagnosed as this diff's own failure. ⛔ No action needed on either PR yet — the shift has not happened.

Status


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

CI red — diagnosed, fix dispatched. ⭐ It is a ratchet moving in the good direction.

Lint & Repo Gates failed at head b5fd717e17 (job 99539643016). Root cause, from the log:

✗ query-options-erasure ratchet (1 problem(s)):
  • test surface fell 240 → 236 site(s) — ratchet DOWN: run `pnpm check:query-options-erasure --update`
    and commit the baseline. A ceiling left above reality silently licenses that many new erasures.

This diff caused it by improving things. Rewriting undeclared-field-write-driver-split.integration.test.ts (8 → 12 tests) and repairing the three fixtures removed 4 query-options-erasure sites from test code. The shrink-only ledger then refuses the stale ceiling — correctly, because a ceiling left above reality would silently license 4 new erasures later.

Recording explicitly that this is not the re-baselining this lane forbids. Lowering a ratchet ceiling to match a genuine improvement is what a shrink-only ledger is for, and the gate itself prescribes the command. Raising a ceiling, or relaxing an assertion, is what is banned. Neither applies. ⚠️ The seat has been told to read the baseline diff before committing and to stop if anything went up — an --update that silently raises a ceiling in the same write is precisely how this mechanism would get abused.

⚠️ Not the cross-PR hazard I flagged earlier (comment 5480477738). That one — this PR's engine.ts insertions shifting #13829's system-context.mdx anchors — has not fired and still applies to whichever of the two lands second.

⭐ A lane-level gap this exposed, worth recording

The seat derived its gate family mechanically from the actual diff (node scripts/pm/dispatch-gates.mjs) — 33 gates, all green — and that derivation was honest. But CI's Lint & Repo Gates job runs check:query-options-erasure unconditionally, and it was not in the derived family.

The derived gate family does not cover every gate that job runs. That is a gap between two tools, not a seat error, and the next seat will hit it the same way. Flagging it here rather than filing a card until I have seen it twice.

Status unchanged otherwise: still draft, still held on needs:contract-review with no available reviewer (see #13829 comment 5479881455). ⛔ Getting CI green is not releasing it.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Second red — and ⛔ its remedy is the opposite of the first. Recording the distinction.

Type Check · debt ledger (job 99539643149) also failed at b5fd717e17:

• @objectstack/objectql: TEST_DEBT records 251 raw tsc error(s), `tsc --noEmit` now reports 252 (+1).
  TEST_DEBT is frozen debt, not a permission slip -- the ledger is a ratchet and may only shrink.
  Fix the new errors -- that is the author's remedy ... ⛔ MAINTAINER-ONLY, NOT a co-equal option:
  ... do not take this path to get CI green.

⚠️ Two shrink-only ratchets, opposite correct actions — worth stating plainly

Gate Direction Correct action
query-options-erasure DOWN 240 → 236 Ceiling now sits above reality ⇒ lower it (--update). ✅ Sanctioned; the gate prescribes it.
TEST_DEBT UP 251 → 252 Do NOT raise the entry. Fix the error. The gate calls raising it MAINTAINER-ONLY and says "do not take this path to get CI green."

⛔ Flagged to the seat explicitly, because "ratchet complains ⇒ run --update" is a tempting pattern-match and here it would be exactly the re-baseline this lane forbids — and that the gate forbids too.

⭐ The seat predicted this gap itself

Its report stated, before CI ran:

packages/objectql and packages/runtime typechecks exclude *.test.ts, so their green says nothing about the two new test files (--listFiles hit count 0 for each); those are covered by check:type-check-debt in CI.

⇒ The +1 is almost certainly in the new engine-post-hook-undeclared-field.test.ts or the repaired fixtures. That declared blind spot is why this took one log read to diagnose instead of three. ⭐ It is also the second card this session to hit the same underlying defect class — packages/objectql's tsc program excluding its own tests is open as #13676, and packages/formula's identical shape was measured on #13594.

TypeScript Type Check (job 99542184737) is the aggregator and sits downstream of this lane; it should clear when the ledger does. ⛔ Not chased separately.

Both fixes are to land in one validated push, with both gates re-run first. Status otherwise unchanged: draft, held on needs:contract-review.


Generated by Claude Code

claude added 2 commits August 31, 2026 16:05
…use helper

TEST_DEBT for @objectstack/objectql went 251 -> 252 in CI. The +1 is one
TS6133 ('data' is declared but its value is never read) in the new
engine-post-hook-undeclared-field.test.ts: the refuse helper took the payload
it never reads, because the SQL arm quotes a fixed value in its statement
string on purpose — that string reproduces the shape driver-sql used to leak,
not the double's own payload.

Fixed at the source rather than by raising the entry: the ledger is
shrink-only and the gate names raising it maintainer-only. objectql's own
typecheck excludes **/*.test.ts, which is why the package's local green said
nothing about this file and the debt ledger is what caught it.
…0 -> 236

The ceiling is now above reality, and a ceiling left above reality silently
licenses that many new erasures. Written by the gate's own prescribed
`pnpm check:query-options-erasure --update`.

The four sites left with the rewrite of
undeclared-field-write-driver-split.integration.test.ts (its two divergence
arms became one it.each over both families) and the three fixture repairs that
went with it. The diff is one line — testSurface.sites 240 -> 236; the 67
non-test sites across 17 files are untouched, and nothing anywhere is raised.

Copy link
Copy Markdown
Collaborator

Both CI fixes pushed and verified — ⭐ the two ratchets got opposite, correct treatments

Head moved b5fd717e1766d1cf40b7, two commits, one file each. I checked both against the rule they could most easily have broken.

1. TEST_DEBT UP 251→252 — ⭐ fixed at source, ledger untouched

Commit 30c02e70. The error was one TS6133 in the new engine-post-hook-undeclared-field.test.ts: the driver double's refuse helper took a payload parameter it never reads, because the SQL arm quotes a fixed value in its statement string on purpose — that string reproduces the shape driver-sql used to leak, not the double's own payload. Parameter removed, three call sites updated.

scripts/check-type-check-coverage.mjs and its TEST_DEBT entry are byte-unchanged. The gate names raising an entry MAINTAINER-ONLY and says "do not take this path to get CI green" — that path was not taken. No as any, no assertion weakened.

The attribution is airtight without needing the absolute count, which matters because the absolute could not be measured here:

The file is NEW, so every error in it is new; it had exactly one; after the fix it has zero. The local test-program probe total moved 739 → 738, exactly −1, matching CI's exactly +1. The other three edited objectql files gained nothing — their errors sit on import lines and pre-existing mock typing, none at a line I touched; had they contributed, CI's delta would have exceeded +1.

⚠️ And the reason the absolute was reported NOT MEASURED rather than guessed is the gate's own: check:type-check-debt refuses to run locally (42 workspace dependencies have no built type entry point) and states that measuring anyway "would not fail, it would silently measure a different world" — its own example being packages/lint reporting 19 with its closure built and 147 without. ⇒ Reasoning from the delta rather than the total is the correct move, and saying so is why this is checkable.

2. query-options-erasure DOWN 240→236 — ⭐ sanctioned direction, and the guard I asked for was actually run

Commit 66d1cf40. One line: testSurface.sites 240 → 236, written by the gate's own prescribed --update. The cause is this PR's own improvement — four sites left with the integration-test rewrite (two divergence arms collapsed into one it.each) and the three fixture repairs.

The baseline diff was inspected before committing, for the stated reason:

--update rewrites whatever it finds and a silent increase riding along is how an --update becomes a re-baseline.

The non-test map (67 sites across 17 files) is byte-unchanged; nothing anywhere is raised. Gate re-run at the final head: exit 0.


⭐⭐ The lane finding this produced — now seen twice, and diagnosed as structural

I noted after the first red that dispatch-gates.mjs's derived family did not cover a gate CI runs unconditionally, and said I would wait to see it twice before filing. It has now happened twice, and the seat sharpened it from "a missing glob" into something that cannot be fixed by widening globs:

Path derivation answers "which gates read the files you changed." These two are whole-surface ratchets: one counts a repo-wide test-file surface, the other a per-package raw tsc total. A diff can move their number without touching any path their source globs name — which is exactly what happened. That class cannot be path-derived even in principle.

⭐ And the detail that makes it concrete rather than theoretical:

query-options-erasure did appear in the re-derived family — but only once the diff already included scripts/query-options-erasure-baseline.json, i.e. it surfaces only after you have tripped it and edited its baseline. Too late to be useful. type-check-debt never appears at all.

Verified: both failing steps carry no if: path guard (lint.yml:196, lint.yml:4437) — they run unconditionally.

⇒ I am filing this as a card. The proposed shape is the seat's: derive an ALWAYS list for unconditional whole-surface ratchets from the workflow steps that carry no path guard, rather than from the change set. It has cost two PRs a CI cycle each in this session alone.

Status

Unchanged: still draft, still held on needs:contract-review. The maintainer has confirmed that label still gates, so this round was CI work only and released nothing. My review at 5480477738 is not addressed by it, and the @objectstack/spec patch-vs-minor question (5480675866) remains open for a reviewer.

CI on the new head had not reported when I checked. ⛔ Nothing treated as green.


Generated by Claude Code

claude added 2 commits August 31, 2026 16:38
…nch's engine.ts insertions

The page anchors ~145 citations by file:line; the two pure insertions this
branch makes in packages/objectql/src/engine.ts (+67 at old 9491, +28 at old
10469) shifted eight cited lines by +67/+95. Mechanical repair via
`node scripts/check-system-context-census.mjs --fix` — line numbers only, same
semantic sites (the engine.ts diff is insertion-only, so the old->new line map
is exact).

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

Copy link
Copy Markdown
Collaborator

Contract review (Clause ②) — REWORK

Reviewed at head 66d1cf40b7840d0ce37d362faae6897cc21338b3. Rendered by a CONTRACT_REVIEW_TIER reviewer in an isolated context; transcript tier-verified before adoption (99 harness-stamped assistant turns, 100% at tier, first and last included, service_tier: standard throughout, zero fallback evidence). The triage seat runs below tier and therefore adopts verbatim or voids whole. Adopted verbatim, unedited:

VERDICT: REWORK
CLAUSE-2-PATH: no
CLAUSE-2-CONTENT: yes
DECLARATION-HONEST: yes
ONE-LINE: The Clause-② self-declaration is honest and the narrowing is real and well-pinned, but the changeset grades a breaking accept-set narrowing on published `@objectstack/objectql` as `patch` where the repo's own launch-window convention — including the exact same-package analogue — ships it as `minor`, and CI is red at the current head.
FINDINGS:
- Path limb NO, verified: all 10 changed files enumerated; none under `packages/spec/src/**`. One rider the PR body's "What changed outside `packages/objectql`" section never mentions: `scripts/query-options-erasure-baseline.json`, a one-line `"sites": 240` → `236` (shrink direction, explained only in issue comment 5481084070).
- Content limb YES, and I am counting the accept-set narrowing on the write path — NOT the PR's A2.4 security corollary, which per the 2026-08-28 ruling belongs to the maintainer floor. The narrowing is real: two purely additive blocks in `packages/objectql/src/engine.ts` (+95/−0), each headed "The POST-hook half of the declared-field door", one immediately after `await this.triggerHooks('beforeInsert', rowHookContexts[i])` and one at the update confluence; a key `memory` stored yesterday is refused `INVALID_FIELD`/400 today.
- Pre-hook door not moved/merged/weakened: the engine.ts diff deletes zero lines; both pre-hook sites ("[#8682] The declared-field door … FIRST, so nothing downstream … happens" on insert, the #8738 twin on update) are intact on main and outside every hunk; the claimed pin exists — "POSITIVE CONTROL — the door still fires for a CALLER-supplied key (#8682 unmoved)" asserting `expect(hookRuns).toEqual([])`, i.e. refusal before any hook runs.
- DECISIVE — changeset grade is not honest. `.changeset/post-hook-undeclared-field-door.md` grades `'@objectstack/objectql': patch` for a change its own text calls a narrowing and "a security fix". The repo convention is measured, not asserted: 12 changesets carry "shipped as `minor` under the repo's launch-window convention"; `.changeset/form-view-option-default-narrowed.md` reads "**BREAKING** accept-set narrowing … shipped as `minor`" exactly as briefed; and the same-package exact analogue `.changeset/hook-input-symbol-key-refusal.md` ("used to succeed, and now throw. That is a narrowing of the accept set") argues "**Bump level, argued**: `minor`, not `patch`" explicitly. `@objectstack/objectql` must be `minor`. (`'@objectstack/lint': patch` is defensible — that diff is message prose only; the rule's accept/reject behaviour is unchanged.)
- CI is RED at the current head. Head has not moved (`66d1cf40b7840d0ce37d362faae6897cc21338b3`, as briefed); of 33 check runs, 31 success, 1 skipped (Console Pin Gate), and "Lint & Repo Gates" FAILED on `check-system-context-census`: "16 problem(s) over 145 anchors and 109 census sites", every problem an `engine.ts` anchor (e.g. "engine.ts:10807 reads `opCtx.context.isSystem` and NO row on the page anchors it") — line rot from this PR's own +95 insertions; the gate prescribes `check-system-context-census.mjs --fix` for pure rot. Nothing is in_progress. Must be fixed on the branch regardless of the changeset verdict.
- Tests were adapted to the contract, not to the code. 8 → 12 verified: the old integration file holds exactly 8 `it(` blocks; the rewrite is 5 assertions × `it.each` over 2 families (10) plus the 2 untouched caller-door tests; no `.skip`/`.only`/`.todo`/deletions in either test patch (pattern control: the same grep instrument hit `it.each` in the same files). The `sys_organization` noise narrowing (`silentChannels()` → `silentChannels([])`) is the helper's own documented remedy, verbatim in `expected-read-refusal-noise.ts`: "Narrow it for a table read on only SOME of a file's paths"; the no-capture failure branch (`?? ['no capture was installed']`) survives.
- All three fixture "repairs" verified spec-illegal beforehand, in source: `engine.test.ts`'s own hook runs `ctx.input.data.stamped = ctx.input.data.title.toUpperCase()` against a mock map that declared only `title`; `engine-undeclared-update-field.test.ts`'s harness hook runs `ctx.input.data.description = \`derived-for-…\`` against `stubFields` declaring only `name`; the real `stampCompanion` carries `if (!schema?.fields?.[SEARCH_COMPANION_FIELD]) return;` (companion-projection.ts) which the double lacked, and the `declared:false` arm's blob is seeded by `store.seed(...)` directly, so restoring the guard leaves the arm's read-projection subject intact. None of these dodges the new gate; each closes the same defect the card names.
- The A2.2 census's completeness claim is falsified, though its verdict survives. Spot-checked rows hold (`record.created_at = record.created_at ?? now` unconditional and `hasField(objectName, 'tenant_id')`-guarded stamps in `plugin.ts`; `data.author_id = ctx.session.userId` with `author_id: Field.lookup('sys_user'` declared in sys-comment.object.ts; `data.uploaded_by`; `if (!(field in data)) continue`; app-crm `input.probability = 100` with `probability: Field.percent` declared). But FOUR shipped before-hook payload writers appear in neither the census table nor its exclusion list: `(data as any).customized = true` in `email-template-provenance.ts`, `webhook-provenance.ts` and `sharing-rule-provenance.ts`, and `(row as any).delegator_id = userId` in plugin-approvals `lifecycle-hooks.ts` (`bindDelegationWriteGuard`, a registered `beforeInsert`). All four keys are DECLARED (`customized: Field.boolean` on each object; `delegator_id: Field.lookup('sys_user'` on sys-approval-delegation.object.ts), so no shipped behaviour breaks and STOP 1 still does not fire — but the census scanner has a second, uncaught blind spot of the same class it self-caught once: the `(x as any).key =` cast form. Control for this sweep: the cast-form grep fired on known-present `(item as any)._provenance` in spec, and the plain-form grep fired on the census's own rows.
- Tolerance set verified verbatim: `const PLATFORM_PROVISIONED_COLUMNS = ['id', 'created_at', 'updated_at'] as const;` in engine.ts — exactly three, `tenant_id` absent; the nearest-miss pin exists ("undeclared 'tenant_id' must be refused") and the new suite's tolerance test asserts the set is "EXACTLY `id`, `created_at`, `updated_at` — nothing wider".
- Machine spelling: the gate fixes exactly `Clause-②: yes` / `Clause-②: no` (SKILL.md: "恰这两种拼写:`Clause-②: yes` / `Clause-②: no`"; ensure-pm-labels.sh: "card's claim comment declares `Clause-②: yes`"). The claim comment on #13657 actually reads "⚠️ **Clause ②: my reading is YES (content limb).**" — space instead of hyphen, "my reading is YES" instead of "yes": NEITHER fixed token appears verbatim. Known systemic gap, recorded, not fixed.
- Scope and bans clean: `content/docs/releases/` untouched (only `content/docs/automation/hook-bodies.mdx`); neither `packages/metadata-protocol/src/protocol.ts` nor `packages/drivers/driver-memory` in the file list. Tier note recorded as briefed: the quota-exhaustion downgrade licenses nothing for review, so every verdict-deciding measurement above was re-derived rather than accepted; NOT MEASURED here: the PR's suite totals (251/4339, 202/3015), the 13-of-17 ablation and the downstream sweep — unrunnable in this read-only container (CI's green Test Core shards at head partially corroborate); no shallow-clone history query was used as a reading.

Carrier action

needs:contract-review stays on both carriers (this PR and card #13657 — the card was missing it and has been synced in the same pass). ⛔ Not enqueueable until a re-review clears it.

Three things to fix

  1. Lint & Repo Gates is RED at the current headcheck-system-context-census, 16 anchor problems, all engine.ts, all pure line rot from this diff's own +95 insertions. check-system-context-census.mjs --fix is the gate's own prescription. ⛔ Never hand-edit the numbers. This is independent of the contract question and has to be fixed either way.
  2. Regrade @objectstack/objectql from patch to minor. The decisive evidence is in-package: .changeset/hook-input-symbol-key-refusal.md — the same package, the same shape ("used to succeed, and now throw. That is a narrowing of the accept set") — argues "Bump level, argued: minor, not patch" explicitly. @objectstack/lint at patch is fine; that diff is message prose only.
  3. Add Clause-②: yes verbatim to the claim comment on The undeclared-field door sits in FRONT of the hooks, so a key a beforeInsert hook writes has no door at all — and the drivers then disagree (memory stores it, SQL throws a raw statement error) #13657. It currently reads "⚠️ Clause ② : my reading is YES (content limb)." — right answer, wrong form: space instead of hyphen, prose instead of the fixed token. The gate's declaration limb greps the literal. ⚠️ On this PR that matters more than usual: the path limb is no, so the claim comment is the only limb that can fire. Filed systemically as The Clause-②: yes | no machine spelling is missing from the claim comment on 2 of 3 measured cards — the enqueue gate's predicate reads it there, and it is not there #13914.

One finding worth reading even though it changes no verdict

The A2.2 census has a second blind spot of the class it already self-caught once. It found and fixed the computed-key form (data[SEARCH_COMPANION_FIELD]); it still misses the cast form (x as any).key =. Four shipped before-hook writers fall through it — customized in email-template-provenance.ts / webhook-provenance.ts / sharing-rule-provenance.ts, and delegator_id in plugin-approvals' lifecycle-hooks.ts.

⚠️ The verdict survives: all four keys are declared on their objects, so STOP condition 1 still does not fire and no shipped hook breaks. But the census is this PR's central empirical claim, and "the scanner missed four writers" is a fact the next reader of that table should have. Worth a sentence in the body, or a follow-up card for the scanner — your call.

What the review confirmed as sound

The narrowing is real and correctly located; the pre-hook door is provably unmoved (the engine.ts diff deletes zero lines) with a positive-control pin asserting refusal before any hook runs; the three fixture repairs were each verified spec-illegal beforehand in source rather than adapted to the new gate; the 8→12 assertion count holds with nothing skipped or deleted; and the tolerance set is exactly the three the PR claims, with tenant_id pinned as the nearest miss.

⚠️ Reviewed as an audit pass — contract review belongs to this card's dispatching seat; the triage round is "非放行必要条件,在线时作事后审计/抽查", so an audit REWORK hands back for the patch round and takes no landing action.


Generated by Claude Code

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

Labels

Projects

None yet

3 participants