Skip to content

fix(spec,core): HotReloadManager.startWatching refuses instead of reporting success; watchPatterns retired (#12428) - #12571

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-12428-hotreload-watch-placeholder
Aug 26, 2026
Merged

fix(spec,core): HotReloadManager.startWatching refuses instead of reporting success; watchPatterns retired (#12428)#12571
os-warren merged 1 commit into
mainfrom
claude/issue-12428-hotreload-watch-placeholder

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#12428

Clause-②: YES, the label stays — this narrows an accept set. It does not widen public surface. Both halves measured, git diff --stat origin/main...HEAD:

 .changeset/hot-reload-watch-placeholder-retired.md | 86 +++++++++++++
.../kernel/plugin-lifecycle-advanced.mdx | 2 +-
packages/core/examples/phase2-integration.ts | 1 -
packages/core/src/hot-reload.test.ts | 133 ++++++++++++++++++++
packages/core/src/hot-reload.ts | 135 +++++++++++++--------
packages/spec/authorable-surface/kernel.json | 2 +-
.../src/kernel/plugin-lifecycle-advanced.test.ts | 30 ++++-
.../src/kernel/plugin-lifecycle-advanced.zod.ts | 94 +++++++++++++-
.../18.kernel__HotReloadConfig__watchPatterns.ts | 40 ++++++
.../18.hot-reload-watch-placeholder-retired.ts | 81 +++++++++++++
packages/spec/src/migrations/registry.ts | 115 ++++++++++++++++++
11 files changed, 662 insertions(+), 57 deletions(-)

Configurations that boot today start failing: any host passing watchPatterns, or calling startWatching, is now refused. Every changed line is a removal, a tombstone, a registration, a test or a comment — no new exported name, and the two type changes both narrow (watchPatterns input string[] | undefinednever | undefined; startWatching return voidnever). In-tree blast radius is zero; the cost is borne by out-of-tree hosts of a published library. Unlike #12394, where the anticipated widening did not materialise and the label came off, here the narrowing is real.

What was wrong

HotReloadManager.startWatching contained no watcher. Its whole body was a guard plus logger.info('File watching started', { patterns }), above an in-source note saying real watching "would require chokidar or similar / This is a placeholder for the integration point".

watchHandles was only ever read, deleted, iterated and cleared and never set, so stopWatching's cleanup branch and the teardown loop over its keys were structurally unreachable, not merely untaken. watchPatterns therefore had no reader that acted on it — its only two uses were log lines.

Where #12340's inert fallback at least announced itself at DEBUG, this claimed success at INFO. That is the part that needed no ruling: an operator who set enabled: true with watchPatterns and read that line had been told the opposite of the truth.

Reproduced before repairing (worktree at base 52a9823887):

[SPEC] watchPatterns accepted -> ["plugins/**/*.ts"]
[RUNTIME] startWatching('p') log -> ["info:File watching started"]
[RUNTIME] stopWatching('p') log -> [] <- "File watching stopped" absent => watchHandles never populated
[RUNTIME] watchHandles size -> 0
[RUNTIME] shutdown teardown loop ran 0 times over watchHandles.keys()

Unreachability pinned with a firing positive control

A zero-hit is not a reading until a control fires in the same scan.

  • Control: reloadTimers.set1 hit, packages/core/src/hot-reload.ts:474. The sibling map in the same class, found by the same <map>\.set scan. The scan sees writers.
  • Target: watchHandles.set0 hits, anywhere in the tree. The other references (declaration, .get, .delete, .keys(), .clear()) are all reads.
  • Callers: core's startWatching had zero. The only reference was its own declaration; core's stopWatching had exactly one caller — the teardown loop over the map nothing writes.
  • Composition: nothing composes HotReloadManager at runtime. Only its own unit test and packages/core/examples/phase2-integration.ts construct it.
  • ⚠️ Deliberately not resolved: the otherstartWatching, in packages/metadata/src/node-metadata-manager.ts, which really does poll. Same method name, different subsystem.

Route, and why the presumption was not overturned

ADR-0049 leaves three states. The other two were measured and both unavailable:

So the refuse/retire presumption stands. The only recorded statement in the neighbourhood — docs/protocol-upgrade-guide.md, "kept as the starting point for a future enforce decision" — reserves the enforce question; it is not a roadmap entry, and #12425 already established that per-key readership decides inside this kept container.

The one thing to look at — the route was chosen by the build, not by me

I started on #12340's route 3 (plain deletion, no tombstone: nothing parses HotReloadConfigSchema). gen:schemagate (a) refused it, and the refusal is the evidence:

❌ 1 authorable key(s) disappeared from the contract:
- kernel/HotReloadConfig:watchPatterns
These schemas are NOT .strict(), so Zod silently STRIPS an unknown key...
To retire a key, tombstone it instead of deleting it

That is correct and I switched. #12340 could take route 3 because what left there was a whole def, which has its own registry and its own ratchet movement. A key leaving a surviving def has no such exit — deleting it would have been a silent strip, i.e. the very defect being retired, one layer down. So:

  • watchPatterns is retiredKey()-tombstoned; its surface line now carries [RETIRED].
  • kernel/HotReloadConfig:watchPatterns registered by exact key in RETIRED_KEYS_BY_MAJOR[18].
  • No D2 conversion, deliberately: the chain walks a normalized stack and HotReloadConfig is not an authorable surface — no metadata-type binding, stack collection or manifest embed ever carried it — so a conversion would be a transform with no seam that ever runs. For the same reason the prescription carries no os migrate meta sentence, exactly as its stateStrategy sibling in this module does not; the migrate-sentence pin judges only prescriptions that do name the command, so this absence is in scope by construction rather than by exemption.

Ratchet reading, and it is the one a key tombstone must produce — not the def-removal shape #12425 saw:

RatchetPredictedObserved
authorable-surfaceone line bare → [RETIRED], count unchangedkernel/HotReloadConfig:watchPatterns… [RETIRED]; kernel keys 859 → 859
api-surface / json-schema.manifestunmoved — the def still emitsunchanged

Because the tombstone alone reaches nobody (nothing parses this schema outside its own unit test), the doors carry the prescription: startWatching throws an ADR-0112 envelope (code: VALIDATION_ERROR, status: 400) naming scheduleReload, and registerPlugin refuses a leftover watchPatterns the same way — before the enabled check, so a disabled config cannot smuggle the false declaration through.

startWatching is kept as a throwing door rather than deleted, on purpose: deleting the method leaves a JavaScript host a bare TypeError: not a function with no prescription, and this is the one place a caller of the old placeholder is guaranteed to arrive. This is the call worth a maintainer's eye.

Runtime reload behaviour is unchanged for every config that worked: nothing was ever watched, so nothing that used to happen stops happening. stopWatching keeps the half that always did something (it cancels a pending debounced reload), and shutdown lost only a loop that iterated watchHandles and therefore ran zero times — both pinned by new tests.

Two small in-scope refactors, named rather than slipped in: assertNoRetiredDistributedConfig is now table-driven (RETIRED_HOT_RELOAD_KEYS) so this retirement is one row rather than a second near-copy — #12340's message is carried across byte-for-byte, and its pins assert content, so they do not move; and the private stateStrategyRefusal is renamed hotReloadRefusal, since #12340 already used it for a non-strategy key.

Declared test edit

The spec fixture should validate custom hot reload configuration listed watchPatterns and asserted toEqual(config). That assertion passed precisely because the key parsed and did nothing. The key is removed from the fixture and its departure pinned separately as a refusal — declared here, in the report, and on the line in the test file. Never a quiet edit that turns a red pin green.

Verification (all at 1d4795c17c, tree clean)

Gate union derived, not recallednode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack over the real changeset: 43 matched families plus the convention-triggered ones. Every exit code captured before any pipe (cmd > log 2>&1; e=$?).

  • 29 green: check:generated, check:authorable-surface, check:migration-registry, check:spec-changes, check:upgrade-guide, check:docs, check:api-surface, check:liveness, check:empty-state, check:variant-docs, check:strictness-ledger, check:skill-refs, check:skill-docs, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check:nul-bytes, check:engine-double-contract, check:where-matcher, check:test-source-alias, check:type-source-resolution, check-comment-mask-adoption, check-plugin-teardown-shape, check-doc-frontmatter, check:objectui-changeset, check:type-check-coverage, check:query-options-erasure, check-cross-package-test-inputs, check:type-check-debt.
  • check:type-check-debt first refused outright ("--re-measure cannot run: 54 workspace dependencies have no built type entry point") — reported as not measured, never as green. The full closure was then built (turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70, 6m05s) and it re-ran green: "32 ledger entries re-measured, 1843 raw tsc errors total, none above its recorded number. surplus: none."
  • check:api-surface / check:generated transiently refused after the ablation, with the stale-dist message ("dist/**/*.d.ts is OLDER than src" — the restore bumped source mtimes). Rebuilt and re-run green at the final head. Recorded because that refusal is a not-measured, and the gate says so itself.
  • Not measured locally: repo-wide pnpm lint, left to CI. A bare tsc --noEmit -p packages/core/tsconfig.json was also run and is not a valid reading — @objectstack/core deliberately has no typecheck script and carries a declared TEST_DEBT entry of 98 pre-existing errors; that run reported 100 with the closure unbuilt, which the ratchet's own docblock names as "a DIFFERENT WORLD". Zero of the errors were in files this PR touches. The ratchet above is the real measurement.
  • Tests: core 18/18, spec kernel + migrations 143/143.

Ablations — direction and exact failure count predicted in writing first, mutation proved on disk by anchored grep -cF counts before any result was read, restore under trap … EXIT INT TERM and verified with an empty git diff:

AblationPredictedObserved
remove the startWatching refusalRED, exactly 2 of 18 core tests — the two refuses startWatching… cases2 failed | 16 passed (18) — precisely those two
untombstone watchPatterns in the specRED, exactly 1refuses watchPatterns with the retirement prescription (#12428)1 failed | 16 passed (17) — precisely that one

Neither rebuilt, and neither needed to: both subjects resolve through relative source imports (./hot-reload.js, ./plugin-lifecycle-advanced.zod), not through a package exports field to dist/. The only @objectstack/spec/kernel import in the core test is import type, erased at runtime.

Changeset

minor for both packages, graded deliberately: v17.0.0 is already cut, so this accept-set narrowing rides the 17.x line under the lockstep launch-window convention, with the prescription registered under protocol major 18 where os migrate meta users look — the same grading #12340 used one day earlier in this module. check:changeset-no-major is green, which is the mechanical confirmation.

Generated by Claude Code


Generated by Claude Code

…atchPatterns retired
`HotReloadManager.startWatching` contained no watcher. Its whole body was a
guard plus `logger.info('File watching started', { patterns })`, above an
in-source note saying real watching "would require chokidar or similar". Where
the sibling defect in this file announced its inert fallback at DEBUG, this
claimed success at INFO: an operator who set `enabled: true` with
`watchPatterns` and read that line had been told the opposite of the truth.
`watchHandles` was only ever read, deleted, iterated and cleared and never set,
so `stopWatching`'s cleanup branch and the teardown loop over its keys were
structurally unreachable rather than merely untaken. The zero was pinned with a
firing positive control before anything was removed: `reloadTimers.set`
resolves a real writer in the same file and the same scan, while
`watchHandles.set` resolves nothing anywhere in the tree.
`watchPatterns` therefore had no reader that acted on it — its only two uses
were log lines — so ADR-0049 enforce-or-remove applies. Neither of the other
two states was available: no runtime composes `HotReloadManager`, so enforcing
would build for a caller that does not exist, and a scan of every planning doc
returned zero mentions of hot-reload file watching against 145 control hits in
the same files, so there is no roadmap for `experimental` to point at.
The key is tombstoned rather than deleted, and the build chose that: the plain
deletion was tried first and `gen:schema` gate (a) refused it, because
`HotReloadConfigSchema` is not `.strict()` and a bare deletion would be a
silent strip (ADR-0104) — the very defect being retired, one layer down.
`kernel/HotReloadConfig:watchPatterns` is registered in
`RETIRED_KEYS_BY_MAJOR[18]` with the D3 semantic entry
`hot-reload-watch-placeholder-retired`. No D2 conversion: `HotReloadConfig` is
not an authorable surface, so a conversion would be a transform with no seam
that ever runs.
Because nothing in the tree parses `HotReloadConfigSchema` outside its own unit
test, the tombstone alone reaches nobody, so the runtime doors carry the
prescription: `startWatching` throws an ADR-0112 envelope pointing at
`scheduleReload`, and `registerPlugin` refuses a leftover `watchPatterns` the
same way, before the `enabled` check. `startWatching` is kept as a throwing
door rather than deleted so that caller meets a prescription instead of a bare
TypeError.
Runtime reload behaviour is unchanged: nothing was ever watched, so nothing
that used to happen stops happening. `stopWatching` keeps the half that always
did something, and `shutdown` lost only a loop that iterated `watchHandles` and
therefore ran zero times.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/core, @objectstack/spec, touching 17 documentable anchor(s). ⚠️2 changed file(s) yielded no anchor (packages/spec/authorable-surface/kernel.json, packages/spec/src/migrations/entries/retired-keys/18.kernel__HotReloadConfig__watchPatterns.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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

  • content/docs/protocol/kernel/lifecycle.mdx(via registerPlugin (symbol))
What this run could not see
  • 2 changed file(s) yielded no anchor (packages/spec/authorable-surface/kernel.json, packages/spec/src/migrations/entries/retired-keys/18.kernel__HotReloadConfig__watchPatterns.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 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 — 132 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 aa45919714828002caba204b627debcbe96c3e1fpackageMentionDocs.

Which tree this was computed on

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

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

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — accepted, flipped ready, auto-merge armed

Measured independently: 11 files, +662/−57, all in-scope. Clause ② confirmed yes and the label stays — the accept set genuinely narrows (watchPatterns refused where it parsed, startWatching throws where it lied), and the surface does not widen: a grep for added export declarations across both touched source files returns nothing. Both type changes move inward (string[] | undefinednever | undefined; voidnever). Unlike #12394 today, the anticipated narrowing did materialise, so the label is not removable — that is the label working, not the label sticking.

The route was chosen by the build, and that is the most interesting thing in this report

The dev started on #12340's route 3 — plain deletion — because that is what its sibling had just done in the same file four hours earlier. gen:schema gate (a) refused it, and the reason it gave is exactly right: these schemas are not .strict(), so a bare deletion is a silent strip — the very defect class being retired, one layer down.

But the distinction that actually separates the two cards is one neither the gate nor the dev's first instinct supplied: #12340 removed a whole DEF, so nothing survives for an author to keep writing. #12428 removes a KEY from a def that survives and keeps emitting — there is no route-3 exit from that, because the author's document is still valid and the stripped key still parses to nothing. Landing on the tombstone here is correct, and the reasoning in the PR body arrives at it.

That the right answer came out of a gate message that does not draw that distinction is the part worth recording. Filed as #12574 (domain:spec) on the dev's behalf — it could not file it itself, REST being 403 on that seat. I verified its central claim on the committed baseline rather than on PR prose:

kernel/HotReloadConfig:distributedConfig @ b6c96bcea5d4 → 1 · @ 4635f3e079 → 0
control :stateStrategy @ b6c96bcea5d4 → 1 · @ 4635f3e079 → 1

Route 3 is real, it landed today, and gate (a)'s text calls an aged-out tombstone "the ONE legitimate reason to delete". The card states both horns — either the text is incomplete or the playbook is wrong — and deliberately picks neither; that is the spec lane's call.

Evidence quality

Both ablations name the failure count and the specific cases in advance and hit them exactly (2 of 18; 1 of 17), prove the mutation on disk with a two-way anchor — removed text 1→0 and injected sentinel 0→1 — before reading any result, and verify the restore with an empty git diff rather than trusting the trap. The no-rebuild decision is justified per leg by the import form ('./hot-reload.js' is a relative source import, and the only @objectstack/spec/kernel import in that test is import type, erased at runtime) rather than asserted.

ENFORCE and EXPERIMENTAL were measured unavailable, not dismissed: no runtime composes HotReloadManager, chokidar is a dependency of metadata / metadata-fs / cli and never of core, and a scan of every planning doc returned 0 hits for hot-reload file watching against 145 control hits in the same files. That control is what makes the zero a reading, and it is the difference between "we checked" and "we looked".

Honest non-measurements — all three correctly refused rather than counted

  • tsc --noEmit -p packages/core exited 2 with 100 errors and is reported as not a valid reading: core has no typecheck script by design and carries a declared 98-error debt entry, and all 100 errors sit in 13 test files this PR never opened. Reporting that as red would have been as wrong as reporting it green.
  • check:type-check-debtrefused outright on the first attempt ("54 workspace dependencies have no built type entry point") and is listed green only on the strength of the second run after a 70/70 build. The gate refusing to measure a different world is the gate working.
  • check:api-surface / check:generated transiently refused with a stale-dist message after the ablation, because the restore bumped source mtimes. Rebuilt and re-run green at the final head, and recorded rather than quietly re-run.

The declared test edit

The fixture should validate custom hot reload configuration listed watchPatterns and asserted toEqual(config) — an assertion that passed precisely because the key parsed and did nothing. It is removed and its departure re-pinned as a refusal, declared in the report, in the PR body, and on the line in the test file. Same shape as #12560 today, handled the same correct way.

Two dispositions for the maintainer — surfaced, not decided

  1. startWatching is kept as a permanently-throwing door rather than deleted. Implemented as A, and the argument for it is sound: deleting the method is the only variant that reaches a JavaScript host with no prescription at all, and this method is the one place a caller of the old placeholder is guaranteed to arrive. It also keeps the refusal assertable by code/status rather than merely catchable. B (delete it) is defensible if the preference is for the public surface to shrink, and is a two-line change. This is the call worth a maintainer's eye.
  2. stopWatching is now a misnomer — it never stopped a watcher, because none ever started; what survives is cancellation of a pending debounced reload. Kept under its published name with JSDoc saying precisely that. Renaming a published method is its own accept-set decision with its own blast radius and correctly does not ride this card. Recording it here rather than filing a card, per the dev's reasoning that it is a consequence of this disposition rather than an independent finding — but it is a naming lie on a published surface, which is the same family this card just closed, so it should not evaporate.

⚠️ Report marker eaten by the body sanitizer again — fourth instance today; cross-posted to #12133. The authoritative report is the second comment on #12428, which leads with literal os-dev-report text.

CI is the remaining gate.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-warren@claude