Uh oh!
There was an error while loading. Please reload this page.
fix(types,cli): resolve host-declared packages through the import condition, and read the cluster registry instead of assuming it - #14042
Conversation
📓 Docs Drift CheckThis PR changes 3 package(s): 24 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 26 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # 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
|
os-steve
commented
Sep 1, 2026
REQUEST CHANGES At-tier contract review under What must change
Limb 2 — public-surface widening: fires. Measured, not reasonedTwo builds at head
Limb 1 — accept/reject behaviour of a published mechanism: fires, in both directions
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 codeI read 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 trueRead in source at head ( 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 Changeset grade
On the PM's declaration
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
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 Follow-up at-tier contract review under The delta, verified
First-round items
On clearing |
os-steve
commented
Sep 1, 2026
REQUEST CHANGES — hold confirmed: At-tier contract review continuation under Owning the second error firstMy previous comment amended the list to four files and kept The census, re-derived at |
os-steve
commented
Sep 1, 2026
Match confirmed — Member-by-member against my re-derived census: the three call sites match exactly, line numbers included ( 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 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; Constraints kept: no merge, no auto-merge, no draft flip, no code edits, no label writes. Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
⛔ merge queue 构建失败 — 先分诊,再决定要不要重排队列构建 33472149653 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集), 失败的 job(日志抽取,best effort):
跨 PR 相同签名(24h,按失败测试文件聚合):
历史信号:
分诊清单:
Generated by Claude Code · merge-queue-triage workflow (#4859) |
Fixes#13330
createHostImporter's declared leg resolved withhostRequire.resolve(pkg)— a CommonJS resolution, which answers therequirecondition. Everytsupdual 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/cliis"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 serveloaded@objectstack/service-cluster-redisthrough this leg; the driver's load-timeregisterClusterDriver('redis', ...)ran against the CommonJS copy of@objectstack/service-cluster, and the ESMRuntimeread 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:@objectstack/service-clustergainslistClusterDrivers()on a genuinely published surface:npm view→17.2.0=dist-tags.latest, noprivateflag,filespublishesdist, andsrc/index.tsis the tsup entry behindexports["."]. Built twice at head3a3f89ced9— with the change and with the basecluster.ts/index.ts—dist/index.d.tsgainsdeclare function listClusterDrivers(): string[];plus its export-line entry, withdist/index.jsanddist/index.cjsdiffering likewise as the positive control that the rebuild picked the change up. The changeset'sminorgrade for this package is this limb, conceded.importFromHostis published surface —@objectstack/typespublishes./nodeas 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 returns1on a known caller and no-match onpackages/cli/src/utils/capability-preflight.ts, whose onlyimportFromHosthit is a docblock line — so the zero is a reading, not an artefact): three call sites —packages/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 testfilter 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=redisexited 1 atdefineCluster()on base and boots at head), and a declared dual-published package whoseimporttarget 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.
hostRequire.resolve(driver)dist/index.cjsdist/index.cjs(unchanged — the host anchor is not re-decided)importFromHostloadedcjsesmfalsetruetruefalseBoth 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 = resolvedshould 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…to6de1426f…) and occurrence counts both ways (fix 1 to 0, injected marker 0 to 1); restore proven by hash equality with theHEADblob, occurrence counts back to 1/0, emptygit diff HEADand cleangit 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 tosrc/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
importcondition 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 ownexportsmap.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:
exportsmap — untouched; CJS resolution already returnedmain, the only entry such a package publishes;exportsmap naming no import-condition target (CJS-only) — untouched;That narrowness does not extend to evaluation: all three fallbacks key on the
importtarget being absent, unreadable or escaping, so none of them catches a target that is present and broken. A dual-published package whoseimportbuild throws while itsrequirebuild works used to mask that break by silently loading the CJS build; it now surfaces it (measured: baseLOADED build=cjs, headTHREW: 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
serveno longer assumes the driver registered.@objectstack/service-clusterexportslistClusterDrivers()— the registrydefineCluster()itself consults — andservequeries it after the load.The silent
catchgave two reasons, and a single EE boot measured both wrong at once. Both are now readings:clusterConfignames the driver either way, sodefineCluster()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 registeredone line later: registered (silent); loaded-but-invisible (names the split and the one-line config remedy); not resolvable (prints theundeclared/declared-unresolvableclassification that was being swallowed); resolved-then-crashed (prints the driver's own error). An app on an older@objectstack/service-clusterhas 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.tsisdomain:servicesand was touched for one pure read accessor,listClusterDrivers()— not for theglobalThis-keyed registry (path ②), which proved unnecessary. The reading the card requires is impossible without a side-effect-free query: the only alternative is callingdefineCluster(), which constructs a real cluster. The accessor's agreement withdefineCluster()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 at3a3f89ced9(the final commit; working tree clean, no merge fromorigin/main, so every repo-level reading below is bound to this tree):37 measured green. 1 NOT MEASURED, by the gate's own verdict text:
node scripts/check-test-completeness.mjsexits 3 withPREREQUISITE NOT MET, because it grades a savedturbo run testlog 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:
pnpm lintis never named by the gate script; it was run repo-wide anyway, exit 0 — no narrowing claimed or needed. Also green:@objectstack/typestypecheck (--listFilesconfirms it compiles bothnode.tsandnode.test.ts, so "typecheck clean" really covers the new tests) and 482 tests;@objectstack/service-cluster70 tests;@objectstack/service-cluster-redistypecheck and 28 tests;@objectstack/clitypecheck, theserve-cluster-host-resolutionscan (32 tests) andserve-app-anchored-optional-import.e2e(6 tests).Stated gap. There is no test that boots
serveand 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