Skip to content

fix(engine): aggregate boot-path advisory hits into one summary line, and count them by row - #15306

Merged
os-warren merged 8 commits into
mainfrom
claude/issue-13889-advisory-boot-path-aggregation
Sep 4, 2026
Merged

fix(engine): aggregate boot-path advisory hits into one summary line, and count them by row#15306
os-warren merged 8 commits into
mainfrom
claude/issue-13889-advisory-boot-path-aggregation

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Fixes #13889

Implements maintainer ruling B (comment 5494461940, 2026-09-01, verbatim 「同意」): aggregate advisory hits on the seed/boot load path into one summary line, and in the same stroke fix the by-write counting defect. Option A (an audience key on ValidationRuleSchema) stays parked — packages/spec is not touched. Option C is not taken.

The two landing sites had to be located by behaviour, not by the ruling's names

The ruling names demo_bootstrap and a claim-scan owner_id write-back; neither resolves in this repo (0 hits, with a firing control). Both platform halves were therefore located by symbol first:

half the ruling's name what it actually is here
(a) where advisory hits are emitted seed loader "advisory" emit packages/objectql/src/validation/rule-validator.ts — the logger.warn in evaluateValidationRules, one line per row per violated rule. Not in the seed loader at all.
(b) the system write-back demo_bootstrap claim scan packages/plugins/plugin-security/src/claim-seed-ownership.ts:342ql.update(name, { owner_id: adminUserId }, { where: predicate, multi: true, context: { isSystem: true } })

packages/objectql/src/engine.ts is not touched: it is the reserved serial resource for #14666. Every evaluateValidationRules call site lives in it, which is why neither half was implemented by threading a parameter from there.

What changed

1. Aggregated reporting (@objectstack/core, new runWithAdvisoryAggregation / recordAdvisoryHit). SeedLoaderService.load() runs inside an advisory aggregation scope and reports one summary line per rule — rule, object, row count, the rule's own message, example rows — instead of one line per row. The scope lives in core because it is the layer both ends already depend on, and the dependency only runs one way (objectql depends on metadata-protocol, never the reverse). It uses AsyncLocalStorage rather than a module flag on purpose: a per-org seed replay runs on a live server, so a global would capture concurrent interactive writes' advisories into the replay's summary. Hits are folded into their (object, rule) group on arrival, so 100k rows tripping one rule hold one group, not 100k records.

2. Advisory rules are counted by row, not by write (rule-validator.ts). An update whose payload touches only platform-injected system columns changes no business field, so it no longer re-evaluates the object's advisory rules. The rule is dropped from the run, not merely silenced — the ruling asks for no re-evaluation, and suppressing output would leave the wasted predicate work in place. Membership is resolved per object by the declared authority resolveInjectedSystemColumns, never a literal list, so ownership: 'org' (no owner_id) and systemFields: false are judged on their own columns.

error-severity rules are untouched by both halves. Rule semantics do not move: off the machine path the identical per-write line is emitted, byte for byte.

Verification

Behavioural pins, driven through a real ObjectQL engine, a real SeedLoaderService, and the real claim-scan write shape — packages/objectql/src/validation/advisory-boot-path.test.ts:

  • PIN 1 — one row rings once across a clean first boot (seed insert, then the claim scan). The acceptance anchor.
  • PIN 2 — a seed load reports one summary line for N rows, asserting the count, the rule, the message and the example-row pointer, and zero per-row lines.
  • PIN 3a/3b/3c — controls: an ordinary interactive write still emits the historical sentence verbatim; an error rule still rejects both a system-column-only write and an ordinary one.

Both pins were proved to red on the pre-change path (each ablation committed first, mutation confirmed on disk by anchored counts, rebuilt, marker confirmed in dist by ablation-dist-preflight, then restored and the marker re-proved absent):

  • ablation A (remove the aggregation, @objectstack/core rebuilt, marker live in dist/index.js + dist/index.cjs): PIN 1 and PIN 2 red.
  • ablation B (remove the by-row gate): PIN 1 red with the defect's exact shape — expected [ …(2) ] to have a length of 1 but got 2, the row ringing twice; PIN 3b red too.

Suites, all green: @objectstack/core 48 files / 1159 tests · @objectstack/metadata-protocol 161 files / 2370 tests · @objectstack/objectql 270 files / 4631 tests. Typecheck green for all three (their check:test-typecheck covers the new test file, so it is genuinely type-checked).

Two gates went red and were repaired, both real:

  • check:objectql-double-limit — the new test's find double ignored the caller's bound. Fixed by applying the bound after the filter, by presence; this matters because claimSeedOwnership's paged fallback reads with a limit.
  • check:system-context-census — pure line rot: the 61-line insertion in seed-loader.ts moved three anchors cited by content/docs/permissions/system-context.mdx. Repaired with the gate's own --fix; only line numbers moved. That page's claim that isSystem does not skip validation rules remains true — the new gate is keyed on the payload's columns, not on the context.

All 47 gate families derived by node scripts/pm/dispatch-gates.mjs (no path arguments) are green; the ratchet families and the pins were re-run on the final commit 28ea75b, after the last merge of origin/main.

A changeset is included and deliberately not skip-changeset: this changes runtime behaviour on a shipped boot path and adds a public export.


Generated by Claude Code

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/core, @objectstack/metadata-protocol, @objectstack/objectql, touching 17 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/core/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/validation.mdx (via SeedLoaderService (symbol, a top-level class))
  • content/docs/protocol/objectql/state-machine.mdx (via SeedLoaderService (symbol, a top-level class))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via evaluateValidationRules (symbol, a top-level function))
  • content/docs/releases/v17.mdx (via SeedLoaderService (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/core/src/index.ts) — pages documenting those are invisible to this run
  • 6 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 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 — 37 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 3a4373f4c3ff0b4d2e04a029f632392a0dc762b1packageMentionDocs.

Which tree this was computed on

This run read content/docs from 673a194f90e15771b7ae1f3b5e071bf069388731 — the merge of head 834991e11648c9f8c044b7a44fdde559ae68751e into base 3a4373f4c3ff0b4d2e04a029f632392a0dc762b1, 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 673a194f90e15771b7ae1f3b5e071bf069388731 && git checkout 673a194f90e15771b7ae1f3b5e071bf069388731
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3a4373f4c3ff0b4d2e04a029f632392a0dc762b1 834991e11648c9f8c044b7a44fdde559ae68751e && git checkout -B drift-repro 3a4373f4c3ff0b4d2e04a029f632392a0dc762b1 && git merge --no-ff 834991e11648c9f8c044b7a44fdde559ae68751e

node scripts/docs-audit/affected-docs.mjs --json 3a4373f4c3ff0b4d2e04a029f632392a0dc762b1

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

Copy link
Copy Markdown
Collaborator Author

Clause-② re-derived on the ACTUAL diff: yes. needs:contract-review hung on this PR and on #13889, in one stroke. ⚠️ My claim-time no was wrong.

domain:engine execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ, 09:39Z. ⛔ Not a finding against this delivery — see §3.

1. What the diff actually does to the published surface

Measured on 28ea75b4a against merge base 5b09356b7, not read from the PR body:

packages/core/src/index.ts   +export * from './utils/advisory-aggregation.js';

which publishes six new symbols from @objectstack/core:

symbol kind
ADVISORY_SAMPLE_ROWS const
AdvisoryHit interface
AdvisoryGroup interface
recordAdvisoryHit function
isAggregatingAdvisories function
runWithAdvisoryAggregation function

@objectstack/core's exports map declares exactly two entry points — . and ./logger — so an addition to src/index.ts is the published surface. And core carries no @internal convention (the tag is used in packages/client, packages/metadata-protocol and plugin-auth, but git grep -c '@internal' -- packages/core/src is empty), so there is no existing seam that would have kept these out of the public API.

Widening the public surface is the second limb of clause ②, independent of accept/reject behaviour. It fires here.

2. ⚠️ My claim-time judgment was falsified, and the way it failed is reusable

My claim comment (#13889, 5537948162) recorded Clause-②: no, reasoning from the ruling's own words: 「⛔ 不改规则语义,只改日志形状」 and 「不触 spec 声明面」.

Both of those statements are true of this diff — rule semantics do not move, and packages/spec is untouched. The error is in what I concluded from the second one:

"Does not touch packages/spec" is not the same as "does not widen the public surface." @objectstack/core is a published package too. I checked the accept/reject limb properly and then let "spec is untouched" stand in for the public-surface limb, which it does not cover.

This is the failure mode the tier tooling is explicitly written against — scripts/pm/dispatch-gates.mjs says a path derivation cannot answer clause ② and that 「The enforcement lives one step later, in the PM skill's enqueue gate over the PR's ACTUAL diff」. That later gate is what caught it. It working as designed does not make the earlier miss costless: had this PR been a smaller delivery I might have enqueued it.

Standing check for this seat from here on: when judging clause ②, enumerate every published package the surface touches and diff its entry point — never infer the public-surface limb from the absence of packages/spec.

3. ⛔ This is not a mark against the delivery

The new export is disclosed in the PR body by the dev itself — 「adds a public export」 — which is precisely what made it findable. The seam's placement is argued rather than assumed (objectql → metadata-protocol runs one way, so neither end can import the other; AsyncLocalStorage over a module flag because a per-org seed replay runs on a live server), and the engine.ts reservation for #14666 was honoured with a stated reason for not threading a parameter from there. ⛔ No rework is being asked for on this ground.

4. Consequences — what happens and what does not

  • needs:contract-review is now on both carriers ([Decision] severity: 'warning' 的校验规则没有受众概念 —— UI 级劝导在 seed/bootstrap 等机器写入路径上照样求值并打爆启动日志,且按写入次数而非按行计数 #13889 and this PR), hung together once a reviewable diff existed — ⛔ not pre-hung (maintainer 2026-08-28: 「⛔ 不预挂:可复审增量存在前永不挂标」). Read back on both.
  • ⛔ This PR stays draft. ⛔ Not flipped ready, ⛔ not enqueued, ⛔ no auto-merge.
  • The gate is unmet by the absence of a review, not by a failed one — those are different states and the label cannot distinguish them, so it is stated here. CONTRACT_REVIEW_TIER = claude-fable-5-1 (dispatch-gates.mjs:8831, re-read on origin/main this fire) is quota-exhausted in this session, measured three times today, most recently at 09:20Z with a minimal probe (req_011Cei9p9biyCTdZsq7tFbvj). ⚠️ That is a fact about this session, not about the tier — the director seat ran an at-tier review at 09:05Z. ⛔ 免复核不放行.
  • CI on 28ea75b4a is still running; nothing here pre-judges it.

5. One item for the card record, so it is not lost when #13889 closes

The ruling parks option A behind a tripwire, verbatim: 「若出现「某 warning 规则就该在机器路径响」的实例 ⇒ 受众键成真需求,census 随卡回决策箱」. Fixes #13889 is correct — the ruling's acceptance anchor (one row rings once on a clean first boot; one summary line) is what this PR delivers, and the ruled scope is complete. But the tripwire then survives only in the ruling comment on a closed card, where no queue walk will surface it. Naming it here so the reopen path is on the record: a future instance of a warning rule that genuinely should fire on a machine path reopens #13889 rather than starting cold.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Docs-accuracy round — three sentences this change falsified, repaired

Supersedes the "47 gate families … final commit 28ea75b" line in the body: the two content/docs edits widen the derived set to 75 families, all green, re-run on 834991e after merging origin/main (merge-base advanced to ed217e697).

Verified on the tree actually edited, not on the tree the drift bot computed (18156d74a) or the one they were read on (5b09356b7) — line numbers as found here:

page:line was why it was false
state-machine.mdx:111 "warning/info are logged" logged per write only off the machine path; folded into one summary line per rule on a seed/boot load; not evaluated at all on a system-column-only update
validation.mdx:86 "Shows a warning but allows save" never shown to the caller — advisory hits are logged server-side
validation.mdx:114-115 "an unevaluable warning/info rule is logged and does not throw" advisory rules are dropped from the run before evaluability is reached on that write shape

Net +6/-3 across the two pages, confined to rule semantics — no restatement of the aggregation design in either page.

Two sentences checked and deliberately NOT edited, because they survive: state-machine.mdx:112 and validation.mdx:209 ("every other validation still runs" of seed writes). Advisory rules still evaluate on the seed insert — only the reporting shape moved — and the by-row gate is mode === 'update' only, so it cannot reach a seed insert. What those sentences promise a seed "must still satisfy" is rejection, which is error-severity and untouched.

content/docs/releases/v16.mdx and v17.mdx were not touched: release-owned, and a code PR editing them is what that guardrail exists to stop.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Landing — contract review PASS at CONTRACT_REVIEW_TIER (card comment 5539903166). Ready, squash auto-merge armed.

Director seat, session session_01LsEjuNMPitCHwEfYftZ1um (os-warren), 11:39Z.


Generated by Claude Code

Merged via the queue into main with commit 2ed6be6 Sep 4, 2026
39 checks passed
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/l tests tooling

Projects

None yet

3 participants