Skip to content

fix(types,cli): resolve host-declared packages through the import condition, and read the cluster registry instead of assuming it - #14042

Merged
os-steve merged 4 commits into
mainfrom
claude/issue-13330-host-importer-esm-resolution
Sep 1, 2026
Merged

fix(types,cli): resolve host-declared packages through the import condition, and read the cluster registry instead of assuming it#14042
os-steve merged 4 commits into
mainfrom
claude/issue-13330-host-importer-esm-resolution

Conversation

@os-steve

@os-steve os-steve commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes #13330

createHostImporter's declared leg resolved with hostRequire.resolve(pkg) — a CommonJS resolution, which answers the require condition. Every tsup dual build publishes { "import": "./dist/index.js", "require": "./dist/index.cjs" }, so a package loaded through that leg evaluated as its CommonJS build, while the callers (packages/cli is "type": "module") held the ESM build of the same package. The process ended up with two instances of everything the loaded package shares with its caller, each with its own module-scope state.

os serve loaded @objectstack/service-cluster-redis through this leg; the driver's load-time registerClusterDriver('redis', ...) ran against the CommonJS copy of @objectstack/service-cluster, and the ESM Runtime read the ESM copy and found nothing.

Clause-②: yes

The claim comment on #13330 declared Clause-②: no. Judged from the landed diff, that declaration was wrong — its own flip condition was triggered, and this section is the correction it required. Both limbs fire, measured:

  • Public-surface widening. @objectstack/service-cluster gains listClusterDrivers() on a genuinely published surface: npm view17.2.0 = dist-tags.latest, no private flag, files publishes dist, and src/index.ts is the tsup entry behind exports["."]. Built twice at head 3a3f89ced9 — with the change and with the base cluster.ts/index.tsdist/index.d.ts gains declare function listClusterDrivers(): string[]; plus its export-line entry, with dist/index.js and dist/index.cjs differing likewise as the positive control that the rebuild picked the change up. The changeset's minor grade for this package is this limb, conceded.
  • Accept/reject change on a published mechanism. The declared leg of importFromHost is published surface — @objectstack/types publishes ./node as a subpath — and is consumed beyond the cluster path. Call-site census, re-derived rather than inherited (git grep -n "createHostImporter" over the whole tree, no filters, every hit classified by reading the line; control: the same instrument returns 1 on a known caller and no-match on packages/cli/src/utils/capability-preflight.ts, whose only importFromHost hit is a docblock line — so the zero is a reading, not an artefact): three call sitespackages/cli/src/commands/serve.ts:772 (production), packages/verify/src/harness.ts:515 (production), packages/qa/dogfood/test/enterprise-organizations.ts:107 (in-repo dogfood probe, real call — a -v test filter hides it, which is why earlier revisions of this list were wrong). Comment-only mentions are not consumers and are excluded: capability-preflight.ts:38, packages/cli/test/helpers/serve-process.ts:732, packages/cli/vitest.config.ts:391. Four test files additionally exercise the mechanism directly: packages/types/src/node.test.ts, packages/cli/src/commands/serve-cluster-host-resolution.test.ts, packages/cli/test/serve-host-fallback-base.e2e.test.ts, packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts. The accept set changes in both directions: a boot that previously refused now boots (the fix's purpose — OS_CLUSTER_DRIVER=redis exited 1 at defineCluster() on base and boots at head), and a declared dual-published package whose import target exists on disk but throws at evaluation loaded on base (LOADED build=cjs) and throws on head (THREW: esm build is broken) — same probe, same fixture.

What was measured, and the controls

A dual-published fixture pair (a package holding module-scope state, and a driver package whose only job is a load-time write into it), on Node v22.22.2.

before after
hostRequire.resolve(driver) dist/index.cjs dist/index.cjs (unchanged — the host anchor is not re-decided)
build importFromHost loaded cjs esm
ESM instance sees the registration false true
CJS instance sees the registration true false

Both directions moved: the registration moved, it was not duplicated. A fix that loaded both builds would have satisfied the first row and still left two live copies of the package's state.

Reachability control. listRegistered() is asserted equal to [] in several places, so the instrument is proved able to return the other answer first, on the same fixture: the two builds are shown to be separate instances by writing into one and reading [] back from the other. Without that, every [] in this suite would be unfalsifiable.

Ablation. Predicted, in writing, before running: reverting the declared leg to const entry = resolved should turn exactly 5 of the 9 new cases red (import build, ESM registration visible, CJS instance empty, subpath, wildcard) while CONTROL, PRECONDITION and both narrowness cases stay green. Measured: 5 failed | 25 passed, exactly those 5. Mutation proven on disk by blob hash (99a3671a… to 6de1426f…) and occurrence counts both ways (fix 1 to 0, injected marker 0 to 1); restore proven by hash equality with the HEAD blob, occurrence counts back to 1/0, empty git diff HEAD and clean git status --porcelain. The trap ran in the same process as the measurement. No rebuild leg was needed and none is claimed: the test imports ./node.js, which vitest resolves to src/node.ts, and the ablation turning it red is itself the proof that the measurement reads source rather than a build artefact.

The seam (packages/types/src/node.ts)

The declared leg now imports the entry the import condition names. The host anchor is untouched — the CJS resolver still answers where the package is, because no flagless Node API resolves a bare specifier against an arbitrary parent (import.meta.resolve's parent argument is ignored without --experimental-import-meta-resolve, already measured in this file). Only the condition is re-decided, by reading that package's own exports map.

Deliberately narrow at the resolution level — no load that works today resolves differently unless the package itself publishes a valid, existing import-condition target — and each narrowness case is pinned:

  • no exports map — untouched; CJS resolution already returned main, the only entry such a package publishes;
  • an exports map naming no import-condition target (CJS-only) — untouched;
  • anything unreadable, escaping the package root, or absent on disk — falls back to the CJS-resolved path, i.e. exactly the pre-fix behaviour.

That narrowness does not extend to evaluation: all three fallbacks key on the import target being absent, unreadable or escaping, so none of them catches a target that is present and broken. A dual-published package whose import build throws while its require build works used to mask that break by silently loading the CJS build; it now surfaces it (measured: base LOADED build=cjs, head THREW: esm build is broken — same probe, same fixture). Surfacing a broken published build is arguably the correct reading, but it is a behaviour change, not a no-op.

The reading (packages/cli/src/commands/serve.ts)

A residual split is still possible above the seam: two physical copies of one package are two instances in any module system, and no resolver condition merges them. So serve no longer assumes the driver registered. @objectstack/service-cluster exports listClusterDrivers() — the registry defineCluster() itself consults — and serve queries it after the load.

The silent catch gave two reasons, and a single EE boot measured both wrong at once. Both are now readings:

  • "may already be registered by the loaded config" — now checked, by asking the registry;
  • "an absent driver is a documented fall-back to the in-memory cluster" — it is not, and never was here: clusterConfig names the driver either way, so defineCluster() raises its documented error two statements later. The stale comment that said otherwise is corrected rather than implemented.

Four outcomes now read differently instead of arriving as not registered one line later: registered (silent); loaded-but-invisible (names the split and the one-line config remedy); not resolvable (prints the undeclared / declared-unresolvable classification that was being swallowed); resolved-then-crashed (prints the driver's own error). An app on an older @objectstack/service-cluster has no accessor to call; that case is silent — the code declines to claim either answer, and prints nothing.

No behaviour downstream of the diagnosis changed. Every branch prints and continues; none throws. An absent driver still reaches defineCluster()'s documented error (cluster.mdx §8.1) rather than silently downgrading — downgrading would boot a silent single node for an operator who explicitly asked for a remote driver, and on the multi-replica deployments this matters for, the ADR-0010 split-brain guard throws on that downgrade anyway. The one documented downgrade here, a multi-node gate denial, is untouched.

Cross-domain half

packages/services/service-cluster/src/cluster.ts is domain:services and was touched for one pure read accessor, listClusterDrivers() — not for the globalThis-keyed registry (path ②), which proved unnecessary. The reading the card requires is impossible without a side-effect-free query: the only alternative is calling defineCluster(), which constructs a real cluster. The accessor's agreement with defineCluster() is pinned in both directions, so it cannot drift into a phantom check.

Verification

Gate union from node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, reconciled by exact string comparison at 3a3f89ced9 (the final commit; working tree clean, no merge from origin/main, so every repo-level reading below is bound to this tree):

named 38, ran 38, unreconciled 0
comm -23 (named, not run): empty
comm -13 (run, not named): empty

37 measured green. 1 NOT MEASURED, by the gate's own verdict text: node scripts/check-test-completeness.mjs exits 3 with PREREQUISITE NOT MET, because it grades a saved turbo run test log that the family names it with no argument — its own text says to record it as not measured and that it is not a red. Four more (check:dual-build-cjs-loads, check:i18n, check:i18n-coverage, check:type-check-debt) first reported the same missing-dist/ prerequisite; the workspace was built (70/70 tasks) and all four then measured green rather than being left unmeasured.

The closest gate to this change measured green on real bytes:

check:dual-build-cjs-loads -- 102 published require entry point(s) across 66 package(s) load;
610 emitted CommonJS file(s) parse; 1 cross-format behaviour probe(s) agree;
100 require condition(s) resolve a CommonJS-flavoured `types` that exists.

pnpm lint is never named by the gate script; it was run repo-wide anyway, exit 0 — no narrowing claimed or needed. Also green: @objectstack/types typecheck (--listFiles confirms it compiles both node.ts and node.test.ts, so "typecheck clean" really covers the new tests) and 482 tests; @objectstack/service-cluster 70 tests; @objectstack/service-cluster-redis typecheck and 28 tests; @objectstack/cli typecheck, the serve-cluster-host-resolution scan (32 tests) and serve-app-anchored-optional-import.e2e (6 tests).

Stated gap. There is no test that boots serve and asserts the three new diagnoses. The reading is inline in a very long boot method, and the surrounding coverage is the source scan (shape), the CLI typecheck, and the accessor test (the invariant it rests on). Filed as #14054 rather than glossed.

Related, not closed here

Generated by Claude Code

@github-actions github-actions Bot added the size/l label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/cli, @objectstack/service-cluster, @objectstack/types, touching 9 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/services/service-cluster/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.

24 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 dda969cd7193d4fb9dd77a5b36c3ee1a1fab3242.

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

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-cluster/src/index.ts) — pages documenting those are invisible to this run
  • 1 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 — 26 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 dda969cd7193d4fb9dd77a5b36c3ee1a1fab3242packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json dda969cd7193d4fb9dd77a5b36c3ee1a1fab3242

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

os-steve commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

REQUEST CHANGES

At-tier contract review under CONTRACT_REVIEW_TIER (scripts/pm/dispatch-gates.mjs on origin/main, read by symbol: export const CONTRACT_REVIEW_TIER = 'claude-fable-5'), for the needs:contract-review chain on this PR. The fix itself is sound — I reproduced its measurements and add my own below — but clause ② fires on both limbs and the PR does not declare it, and three sentences state a guarantee stronger than the one the code provides. No change to the fix's code is required.

What must change

  1. Add an explicit Clause-②: yes declaration to the PR body, naming both limbs. The claim comment on cli: serve's cluster-driver load registers into the CJS registry while the ESM Runtime reads the ESM one — OS_CLUSTER_DRIVER=redis silently downgrades to "not registered" (post-#10645) #13330 declared Clause-②: no with the written obligation that if the ESM-condition change "alters what boots where it previously refused, that declaration flips to yes and the PR must say so" — and "do not let a no declared here stand unexamined at PR time". The PR body carries no Clause-② declaration at all. Judged from the actual diff (the gate's standard: the diff is a fact, the card's semantics were a prediction), both limbs fire — evidence below.
  2. Qualify the narrowness claim in the three places it is stated as absolute — the PR body, the cli: serve's cluster-driver load registers into the CJS registry while the ESM Runtime reads the ESM one — OS_CLUSTER_DRIVER=redis silently downgrades to "not registered" (post-#10645) #13330 docblock in packages/types/src/node.ts, and .changeset/host-importer-esm-condition.md ("no load that works today can regress" / "cannot regress a load that works today"). It is true at the resolution level (verified below) and false at the evaluation level (measured below). One qualifying clause in each place is enough; the chosen semantics are right and should not change.
  3. File the follow-up issue for the disclosed test gap (no test boots serve and asserts the three new diagnoses). The PR says "worth a follow-up rather than glossing", but the dev report's out_of_scope_findings lists no such filing — createHostImporter's declared leg cannot load an ESM-only host package at all, and misreports it as a broken install #14041 and the [finding] serve.ts cluster-driver load swallows the host-import "undeclared" classification — a missing driver declaration surfaces one plugin later with a message naming the wrong remedy #13463 re-triage note are there, this is not. "Worth a follow-up" must exist as an issue number before merge, or it is glossing with extra steps.

Limb 2 — public-surface widening: fires. Measured, not reasoned

Two builds at head 3a3f89ced9, same worktree, same toolchain; the "without" leg produced by git checkout 5e2c04da7d -- packages/services/service-cluster/src/cluster.ts src/index.ts and a rebuild, then restored.

  • diff without/index.d.ts with/index.d.ts → the with-change build adds declare function listClusterDrivers(): string[]; and adds listClusterDrivers to the export line. Exit 1 (differs).
  • Positive controls: dist/index.js gains the function body and the export (exit 1), and dist/index.cjs differs likewise — the rebuild demonstrably picked the change up, so the .d.ts delta is not a stale artefact.
  • The surface is really published and this is really its entry: npm view @objectstack/service-cluster version17.2.0 (also dist-tags.latest); package.json has no private flag, files: ["dist"], and tsup.config.ts builds src/index.tsdist/index.{js,cjs,d.ts}, which exports["."] names under both conditions.
  • The changeset itself concedes this limb: "@objectstack/service-cluster": minor. A minor-graded package bump and a standing "no published surface widens" declaration cannot both be true.

Limb 1 — accept/reject behaviour of a published mechanism: fires, in both directions

importFromHost is published surface: @objectstack/types has publishConfig.access: public and ./node is a published subpath entry (dist/node.mjs). The declared leg is consumed process-wide — packages/cli/src/commands/serve.ts, packages/cli/src/utils/capability-preflight.ts, packages/verify/src/harness.ts — not only the cluster path.

  • Accept direction: the fix's central purpose. A boot that previously refused (OS_CLUSTER_DRIVER=redis, exit 1 at defineCluster()) now boots. That is literally the caveat's flip condition. Reproduced: vitest run src/node.test.ts at head → 30 passed, including the registration-visibility cases.
  • Reject direction (my control, a zero that came back non-zero): fixture @fixture/broken-esm, declared by a host app; exports { "import": "./dist/index.js", "require": "./dist/index.cjs" }; the import target exists on disk but throws at evaluation, the require build evaluates fine. Same probe script driving createHostImporter(appRoot) against packages/types/dist/node.mjs built from each side:
    • base 5e2c04da7d node.ts → LOADED build=cjs — this load works today;
    • head 3a3f89ced9 node.ts → THREW: esm build is broken.

So "which build a host loads" is not merely an implementation detail here: the accept set of a published mechanism changes in both directions. That is the definition of a clause-② card, independent of path ② never being taken.

The narrowness claim, verified against the code

I read esmEntryForDeclared / selectImportTarget / resolveExportsSubpath against Node's resolution algorithm: condition membership matches Node's default import conditions (node-addons, node, import, default; require excluded by design); key-insertion-order precedence matches; subpath map detection, literal keys, and single-* pattern keys (longest static prefix, suffix tiebreak, span substitution) match; every selected target is gated by the escape check and existsSync; and every shim failure degrades to entry = resolved — the pre-fix path, i.e. the safe direction. The three pinned fallbacks are real and tested (cjs-only, no-exports-map, absent-on-disk). One deliberate deviation worth stating in the docblock: a package whose exports map names no import-eligible condition keeps loading the CJS way here, where a real ESM import() would refuse with ERR_PACKAGE_PATH_NOT_EXPORTED — a deviation in the accepting direction, and exactly what keeps CJS-only packages working.

The boundary of the guarantee is therefore: no load that works today can resolve differently unless the package publishes a valid, existing import-condition target — in which case that target is now the one evaluated, including its failures. That sentence is what items 1 and 2 should say.

"No behaviour downstream of the diagnosis changed" — verified true

Read in source at head (serve.ts ~2513–2575): every branch is console.warn and fall-through; nothing throws; clusterConfig = { driver: __clusterDriver, url: … } is set unconditionally after the block exactly as before, so an absent driver still reaches defineCluster()'s documented error, and the multi-node gate-denial downgrade is untouched. One wording drift, non-blocking: the older-service-cluster case (listClusterDrivers absent, load succeeded) prints nothing — the body's "reports as not measured" overstates; the code declines to claim rather than reports. Fold into the item-2 wording pass.

The disclosed gap (blocking only as a filing)

On its merits, the missing serve-boot test does not block merge: the diagnoses are print-and-continue with downstream behaviour verified unchanged, the seam carries the real coverage (30 tests with control, precondition, and a predicted-direction ablation), and the accessor invariant is pinned in both directions in cluster-driver-registry.test.ts (suite 70/70 at head). But silent-print branches are the exact shape that regresses invisibly, so item 3 stands: file it.

Changeset grade

service-cluster: minor — correct, and matches the measured .d.ts widening. cli: patch — correct; serve tolerates an older service-cluster via the typeof guard. types: patch — acceptable as the defect's remediation, noting the change is observable to every declared-leg consumer (a dual build's namespace shifts from the CJS to the ESM shape), which is precisely why it must be declared under clause ② rather than graded away. The changeset text carries the same absolute narrowness sentence — item 2 covers it.

On the PM's declaration

Clause-②: no was wrong for this PR. It was a defensible prediction when written — "no published surface widens" predated listClusterDrivers(), and path ② was indeed never taken — but the gate judges the landed diff, and the landed diff widens a published surface (measured) and changes what boots (measured, both directions). The claim comment's own flip condition was triggered and the PR body did not say so. That is the failure to correct here; the engineering underneath it held up under re-measurement.


Generated by Claude Code

… the silent older-service-cluster case plainly (#14042 review)

Contract-review corrections for #14042 — no code, test, or behaviour change:

- the narrowness guarantee holds at the RESOLUTION level, not at EVALUATION:
  a dual-published package whose import build exists but throws while its
  require build works used to silently load the CJS build and now surfaces
  the break (measured: base LOADED build=cjs, head THREW). Qualified in the
  node.ts docblock and the changeset; the PR body carries the same clause.
- the older-@objectstack/service-cluster case prints nothing; the changeset
  said it 'reports as unmeasured', which overstated. It now says the case
  is silent.

The disclosed serve-diagnosis test gap is filed as #14054.

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

os-steve commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

REQUEST CHANGES — one residual item, PR-body text only, no commit required: the Clause-② limb-2 consumer list is a subset of the real one. Everything else from the first round is verified resolved against the tree, and with that one list corrected, this review stands as the at-tier approval and needs:contract-review clears without a further review round.

Follow-up at-tier contract review under CONTRACT_REVIEW_TIER (scripts/pm/dispatch-gates.mjs on origin/main, export const CONTRACT_REVIEW_TIER = 'claude-fable-5'), re-reviewing head 773841cb82bdc2cfab29cf6831f68ec14431b3dd against my review of 3a3f89ced9.

The delta, verified

  • Exactly one commit between the heads; it touches exactly .changeset/host-importer-esm-condition.md and packages/types/src/node.ts (git diff --name-only).
  • node.ts: 12 changed lines, 0 non-comment (git diff | grep -vcE '^[+-] \*' = 0, with the 12-line total as the control that the counter counts). The changed block documents the internal esmEntryForDeclared, so no published .d.ts surface moves. No code, no tests, no behaviour — every first-round measurement (the two-build .d.ts diff, the evaluation probe, 30/30 and 70/70 suites) was taken on code that is byte-identical at this head, so the body binding those measurements to 3a3f89ced9 remains honest and none needed re-running.
  • Changeset front-matter grades untouched (types: patch, service-cluster: minor, cli: patch — hunks start at line 27 and 42, above is unchanged).

First-round items

  1. Clause-②: yes declaration — present, substance correct, one list wrong. The section states the PM's no was wrong and its flip condition triggered — not argued down — and both limbs carry the measurements with their controls. The residual: limb 2 says the declared leg "is consumed beyond the cluster path (packages/cli/src/commands/serve.ts, packages/cli/src/utils/capability-preflight.ts, packages/verify/src/harness.ts)". Re-derived over the whole tracked tree at 773841cb82 (git grep -l -E "importFromHost|createHostImporter"), the real in-repo call-site set is four, not three: packages/qa/dogfood/test/enterprise-organizations.ts calls createHostImporter(root, …) at :107 and await importFromHost(ORGANIZATIONS_PKG) at :111 — a real declared-leg consumer (the enterprise probe that node.ts's own header docblock names), not a fixture. The two remaining grep hits (packages/cli/test/helpers/serve-process.ts:732, packages/cli/vitest.config.ts:391) are comment-only mentions, verified, so four is the complete set. Required change: add the dogfood probe to that parenthetical so the declaration states the full blast radius. Provenance, owned plainly: the three-file list came from my own first-round comment, whose grep filtered out paths containing test and thereby dropped the file — the fixer copied a subset I produced. This repo's defect class of guards enumerating a subset of what they describe is exactly why the list must be complete now that it is measured.
  2. Narrowness qualification — accurate, not merely softer, and it does not overstate. Verified in all three places (body, node.ts docblock, changeset; the three wordings agree). It states the true mechanism, matching the code I reviewed: all three fallbacks key on the import target being absent, unreadable or escaping the package root, so none catches a target that is present and broken. It scopes the exposure to exactly what I measured — a dual-published declared package whose import build throws while its require build works — and does not claim the general dual-build population regresses. "Surfacing a broken published build is arguably the correct reading, but it is a behaviour change, not a no-op" is the right epistemic register: neither walked back nor inflated.
  3. Follow-up filed — verified. test(cli): no test boots serve and asserts the four cluster-driver diagnosis branches (follow-up promised in #14042) #14054 exists, open, unassigned, domain:cli + tests + priority:p3 + pm:queue, framed as an author-disclosed coverage gap with compensating coverage and a "done means" that includes asserting the two deliberately-silent cases. The body's "Stated gap" now ends "Filed as test(cli): no test boots serve and asserts the four cluster-driver diagnosis branches (follow-up promised in #14042) #14054 rather than glossed." Resolved as required.
  4. The silent older-service-cluster case — prose now matches code, and the code is right as it stands. Both places now say the case is silent ("declines to claim either answer, and prints nothing"), which matches serve.ts (unchanged at this head; __driverVisible === undefined with no load error reaches neither warn branch). On the open call: I do not think a log line should be added. That branch fires on every boot of a healthy app that merely ships an older service-cluster — warning there trains operators to ignore [cluster] warnings, and when the unverifiable registration actually failed, defineCluster()'s documented error still lands two statements later with the operator-facing remediation. A debug/verbose-gated line would be defensible in the test(cli): no test boots serve and asserts the four cluster-driver diagnosis branches (follow-up promised in #14042) #14054 work if a seam is extracted; it is not needed here. Silence is the correct default, and making the sentence match the silence was the correct fix.

On clearing needs:contract-review

My explicit word: with the limb-2 consumer list amended to include packages/qa/dogfood/test/enterprise-organizations.ts, this PR passes contract review and needs:contract-review clears on this comment — no further at-tier round is needed. The amendment is a body-text edit whose correctness is verifiable by string comparison against the four-file set measured above; every other clause-② fact in the declaration is one I re-measured or measured myself. Absent that amendment, the declaration understates the blast radius and the label must stay.

Constraints kept: no merge, no auto-merge, no draft flip, no code edits, no label writes.


Generated by Claude Code

os-steve commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

REQUEST CHANGES — hold confirmed: needs:contract-review stays until the Clause-② limb-2 consumer list matches the re-derived census below. The list as it stands in the PR body is wrong in both directions at once: it still omits a real consumer (packages/qa/dogfood/test/enterprise-organizations.ts) and still names a non-consumer (packages/cli/src/utils/capability-preflight.ts).

At-tier contract review continuation under CONTRACT_REVIEW_TIER (scripts/pm/dispatch-gates.mjs, 'claude-fable-5'), same head 773841cb82.

Owning the second error first

My previous comment amended the list to four files and kept capability-preflight.ts in it. That was wrong, by the same failure mode as the first error: I line-verified only the new grep hits (serve-process.ts, vitest.config.ts) and carried the round-one members forward unchecked. capability-preflight.ts's only occurrence of either symbol is a docblock line — :38, prefixed * , describing what os serve does — and it contains zero createHostImporter matches. The list has now been wrong twice, each time by being copied instead of re-measured, once by me in each direction. The census below is re-derived from scratch, line-level, with controls.

The census, re-derived at 773841cb82

Instrument: git grep -n "createHostImporter" 773841cb82 over the whole tracked tree (docs/changelogs excluded), every hit classified as call vs. mention by reading the line. createHostImporter call sites are the complete entry set to the declared leg: nothing re-exports it or importFromHost outside packages/types/src (grep control, zero hits), and serve's importFromHost is a file-local wrapper (serve.ts:767) closing over the single importer built at :772, never exported or passed onward.

Consumers (files that build and invoke the importer):

  • Production: packages/cli/src/commands/serve.ts:772 — one importer, feeding every host load serve performs: plugins (:1157), the cluster module and driver (:2442, :2515), i18n (:2851), organizations (:3462), and the generic app-declared loader (:3838); and packages/verify/src/harness.ts:515.
  • In-repo QA tooling: packages/qa/dogfood/test/enterprise-organizations.ts:107 (invoked at :111).
  • Test files calling it directly: packages/types/src/node.test.ts (throughout), packages/cli/src/commands/serve-cluster-host-resolution.test.ts (:740, :749), packages/cli/test/serve-host-fallback-base.e2e.test.ts (:135 onward), packages/cli/test/vitest-resolution-base-collapse.e2e.test.ts (:132 onward).

Not consumers (mention-only, each verified by reading the line): capability-preflight.ts:38 (docblock); serve-process.ts:732, vitest.config.ts:391, serve-app-anchored-optional-import.e2e.test.ts:30, serve-config-plugin-host-resolution.test.ts (comments only); serve-host-fallback-base.test.ts:117 (a regex source-scan asserting serve.ts's shape — it names the symbol, never invokes it); serve-organizations-mount-failure.e2e.test.ts:7,22 (comments).

Controls:

  • The instrument can return both answers: git grep -cE "createHostImporter\s*\(" 773841cb82 -- packages/cli/src/utils/capability-preflight.ts → no match, exit 1; the same command on packages/verify/src/harness.ts → 1.
  • Completeness sweep from the other side: a line-level importFromHost census over the whole tracked tree, excluding the files already classified, returns exactly two hits — both comment lines in serve-organizations-mount-failure.e2e.test.ts. Nothing reaches the mechanism outside the files above.

This census agrees with the coordinator's (three call sites, plus the same four directly-calling test files) — derived independently with my own instrument, per their instruction, not adopted. No disagreement to report.

Required change (unchanged in kind, now with the correct content)

In the Clause-② limb-2 sentence, replace the parenthetical (packages/cli/src/commands/serve.ts, packages/cli/src/utils/capability-preflight.ts, packages/verify/src/harness.ts) with the measured set: packages/cli/src/commands/serve.ts:772 (whose one importer feeds plugins, cluster, i18n, organizations and the generic app-declared loader), packages/verify/src/harness.ts:515, and packages/qa/dogfood/test/enterprise-organizations.ts:107, exercised directly by four test files. Equivalent phrasing is fine; the membership is not negotiable. Two anti-rot recommendations, strongly made after two failures: state the deriving predicate next to the list (git grep -nE 'createHostImporter\s*\(' — call sites, not mentions), and have whoever amends the body re-run it rather than transcribe this comment. This list is a guard enumerating the clause-② blast radius, and it has now demonstrated the exact defect class twice — a subset (or superset) survives readers who are looking straight at it.

On needs:contract-review, explicit word

It stays held. It clears when — and only when — the limb-2 list in the PR body matches the census above; membership is string-checkable against this comment, and every other clause-② fact in the declaration stands verified from my previous rounds (both limbs measured, narrowness qualification accurate, #14054 filed, silent case stated plainly). No new at-tier measurement round is needed for the amendment itself; I will confirm the match when asked.

Constraints kept: no merge, no auto-merge, no draft flip, no code edits, no label writes.


Generated by Claude Code

os-steve commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Match confirmed — needs:contract-review clears. At-tier confirmation under CONTRACT_REVIEW_TIER ('claude-fable-5'), against the live PR body re-fetched at the moment of this judgment (updated_at 2026-09-01T04:44:52Z, head unchanged at 773841cb82 — a body-only amendment, as required).

Member-by-member against my re-derived census: the three call sites match exactly, line numbers included (serve.ts:772, harness.ts:515, enterprise-organizations.ts:107); capability-preflight.ts is now correctly on the excluded side with the docblock-line reason; the four directly-exercising test files match exactly; and the deriving predicate plus its zero-control are printed next to the list, so the next reader can re-run it instead of transcribing it. Nothing in the amended limb overstates or understates the measured set.

One precision note for the record, checked rather than assumed: the three named exclusions are the mentions that caused or sat nearest the earlier errors, not the full mention set — my census also classified serve-app-anchored-optional-import.e2e.test.ts:30, serve-config-plugin-host-resolution.test.ts (comments throughout), serve-host-fallback-base.test.ts:117 (a regex source-scan that names the symbol without invoking it), and serve-organizations-mount-failure.e2e.test.ts:7,22 as mention-only. All are non-members, so the membership claim — the load-bearing part of the declaration — is unaffected; I flag it only so nobody later reads the exclusion list as exhaustive and calls the difference an error.

Process note, owned: my previous comment's "amendment has not landed" was read from a body fetched minutes before the amendment landed and not re-fetched at post time — the same carried-forward class, one level up, as the coordinator observed. Conservative in direction, but the lesson stands: re-fetch at the moment of judgment.

With this, every item of the at-tier contract review is closed: both clause-② limbs declared and measured, the narrowness claim qualified accurately at all three sites, #14054 filed, the silent case stated plainly, and the blast-radius census correct and self-deriving. My explicit word: this PR passes contract review; needs:contract-review clears on this comment. The label write itself is the coordinator's to make.

Constraints kept: no merge, no auto-merge, no draft flip, no code edits, no label writes.


Generated by Claude Code

@os-steve
os-steve marked this pull request as ready for review September 1, 2026 04:47
@os-steve
os-steve added this pull request to the merge queue Sep 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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

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

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

  • Test Core (1/6) — 失败步骤: Run this shard's tests

    @objectstack/cli:test:  FAIL  test/run-dev-unbuilt-workspace.e2e.test.ts > run-dev.js on a workspace package with no build output > reproduces the misdiagnosis it is fixing — oclif still says "not fou
      ↳ 失败原因: @objectstack/cli:test: AssertionError: expected '(node:16981) [MODULE_NOT_FOUND] Warni…' to contain 'Error: command i18n:extract:nope.ts n…'
    @objectstack/cli:test:  FAIL  test/run-dev-unbuilt-workspace.e2e.test.ts > run-dev.js on a workspace package with no build output > names the real cause and the one command that fixes it
      ↳ 失败原因: @objectstack/cli:test: AssertionError: expected '(node:16981) [MODULE_NOT_FOUND] Warni…' to contain 'objectstack: NOT A MISSING COMMAND'
    

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

⚠️ 断言这一侧有一类例外,判据是断言在测什么,不是它是不是 AssertionError 断言的对象是产品行为(一个值、一个形状、一次拒收)⇒ 照上面读:真实的行为改变,去查,⛔ 不要重排掉;
断言的对象是这次实验自身的有效性前提(跑完的耗时、负载下的先后、任何只在时间预算内才成立的条件)⇒ 它跟超时是同一类,同样对负载敏感,重排一次是合法的判别手段。
识别是机械的:断言的消息或它比较的值本身点名了一段时长、一个时间戳、一个耗时计数。实测过的一对 —— AssertionError: SecurityPlugin.init() ran: expected false to be true 测的是产品行为(真回归);
AssertionError: this run took over a second, so second-precision stamps could have differed too: expected 1006 to be less than 1000 测的是实验前提:它守护的那条不变式当时是绿的,同一个 head 原样重排一次即成功。
穿着 AssertionError 外衣的时间测量,仍然是时间测量。(⛔ 这只改「怎么读一次红」,不改「哪些测试可以重排」——后者由别处管。)

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

  • test/run-dev-unbuilt-workspace.e2e.test.ts — 24h 窗口内只有本 PR 撞到过,暂不汇总(再有一个不同 PR 撞到就会自动开汇总 issue)。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

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

分诊清单:

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

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

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

2 participants