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

.changeset/hot-reload-watch-placeholder-retired.md

@@ -0,0 +1,86 @@
+---
+"@objectstack/spec": minor
+"@objectstack/core": minor
+---
+
+fix(spec,core): `HotReloadManager.startWatching` refuses instead of reporting success; `HotReloadConfig.watchPatterns` retired (#12428, ADR-0049)
+
+<!-- adr-0087: registered hot-reload-watch-placeholder-retired -->
+
+**BREAKING** accept-set narrowing, landing after the v17.0.0 cut (the lockstep
+launch-window convention ships it as `minor`; the prescription is registered
+under protocol major 18 — `RETIRED_KEYS_BY_MAJOR[18]` + the D3 semantic entry
+`hot-reload-watch-placeholder-retired` — where `os migrate meta` users will
+look). Graded `minor` rather than `major` for the same reason #12340 was one
+day earlier, in this same module.
+
+ADR-0049 applied one symbol over from #12340, in the same file and on the same
+per-key test. The #11825 keep still stands: `HotReloadConfigSchema` and
+`PluginStateSnapshotSchema` still export, and `HotReloadManager` /
+`PluginHealthMonitor` are untouched apart from the two doors below.
+
+`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
+#12340's inert fallback at least announced itself 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. `watchPatterns` therefore
+had no reader that acted on it — its only two uses were log lines.
+
+FROM → TO:
+
+- `watchPatterns: ['src/**/*.ts']` → *(removed)* — delete the key. Declare your
+ globs wherever your own watcher reads them.
+- `manager.startWatching(name)` → `manager.scheduleReload(name, reloadFn)`,
+ called from your own watcher's change handler. That is the debounced
+ integration point this class does implement, and it is unchanged.
+
+One-line fix: delete `watchPatterns`, and call `scheduleReload` from your own
+file watcher instead of `startWatching` — nothing was ever watched, so nothing
+that used to happen stops happening. File watching is the host's job in this
+host-driven library; `chokidar` is already a dependency of
+`@objectstack/metadata`, `@objectstack/metadata-fs` and `@objectstack/cli` —
+never of `@objectstack/core` — so a host has a working model to copy.
+
+The retirement kit:
+
+- **key tombstone**, and the build is what chose it: 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 (#3733, ADR-0104) — the very defect being retired, one layer
+ down. #12340 could take route 3 because what left there was a whole *def*; a
+ key leaving a *surviving* def has no such exit. So `watchPatterns` is
+ `retiredKey()`-tombstoned, its surface line carries `[RETIRED]`, and
+ `kernel/HotReloadConfig:watchPatterns` is registered by exact key in
+ `RETIRED_KEYS_BY_MAJOR[18]`. A key tombstone on a surviving def moves
+ `authorable-surface` only — the def still emits, so `api-surface` and
+ `json-schema.manifest` do not.
+- **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.
+- **runtime doors** in `@objectstack/core`, because nothing in the tree parses
+ `HotReloadConfigSchema` outside its own unit test, so the tombstone alone
+ reaches nobody: `startWatching` now throws an ADR-0112 envelope
+ (`code: VALIDATION_ERROR`, `status: 400`) carrying the prescription, 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 so
+ that caller meets a prescription instead of a bare `TypeError`.
+- **dead code removed with a firing positive control**: `watchHandles` and both
+ of its unreachable readers are gone. The zero was pinned first —
+ `reloadTimers.set` resolves a real writer in the same file and the same scan,
+ while `watchHandles.set` resolves nothing anywhere. `stopWatching` keeps the
+ half that always did something (it cancels a pending debounced reload), and
+ `shutdown` is unchanged in effect: the loop it lost iterated `watchHandles`
+ and therefore ran zero times.
+- **ENFORCE and EXPERIMENTAL were both unavailable**, which is why this is a
+ removal: 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.

content/docs/references/kernel/plugin-lifecycle-advanced.mdx

@@ -41,7 +41,7 @@ const result = HotReloadConfigSchema.parse(data);
 | Property | Type | Required | Description |
 | :--- | :--- | :--- | :--- |
 | **enabled** | `boolean` | optional (default: `false`) | |
-| **watchPatterns** | `string[]` | optional | Glob patterns to watch for changes |
+| **watchPatterns** | `never` | optional | [REMOVED] `HotReloadConfig.watchPatterns` was removed in @objectstack/spec 18 (#12428, ADR-0049 enforce-or-remove) — nothing ever read it. Its only two uses were log lines in `HotReloadManager`, and one of them announced 'File watching started' at INFO level while no watcher was ever constructed: `startWatching` held a placeholder, and `watchHandles` was read, deleted, iterated and cleared but never set. So an author could declare a glob and no file change could ever trigger a reload. Delete the key. File watching is the HOST's job in this host-driven library: run your own watcher, declare your globs wherever that watcher reads them, and call `HotReloadManager.scheduleReload(pluginName, reloadFn)` when one matches — the debounced integration point this class does implement, and which is unchanged. |
 | **debounceDelay** | `integer` | optional (default: `1000`) | Wait time after change detection before reload |
 | **preserveState** | `boolean` | optional (default: `true`) | Keep plugin state across reloads |
 | **stateStrategy** | `Enum<'memory' \| 'none'>` | optional (default: `"memory"`) | How to preserve state during reload |

packages/core/examples/phase2-integration.ts

@@ -278,7 +278,6 @@ async function example() {
  // Hot reload
  hotReload: {
  enabled: true,
- watchPatterns: ['plugins/my-plugin/**/*.ts'],
  debounceDelay: 1000,
  preserveState: true,
  stateStrategy: 'memory',

packages/core/src/hot-reload.test.ts

@@ -328,3 +328,136 @@ describe('[#12340] stateStrategy refusal', () => {
  });
  }
 });
+
+
+// ── [#12428] The watching placeholder refuses instead of reporting success ───
+//
+// Before this card `startWatching` contained NO watcher: a guard plus
+// `logger.info('File watching started', { patterns })` above an in-source note
+// saying real watching "would require chokidar or similar". An operator who set
+// `enabled: true` with `watchPatterns` and read that INFO 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, not merely untaken.
+//
+// The last two tests are the behaviour-PRESERVATION pins for those removals:
+// what left was unreachable, and what stayed still works.
+describe('[#12428] startWatching refusal and the watch-handle removal', () => {
+ const liveConfig = (overrides: Record<string, unknown> = {}): HotReloadConfigParsed =>
+ ({
+ enabled: true,
+ debounceDelay: 1000,
+ preserveState: false,
+ stateStrategy: 'memory',
+ shutdownTimeout: 1000,
+ ...overrides,
+ }) as unknown as HotReloadConfigParsed;
+
+ let mgr: HotReloadManager;
+ beforeEach(() => {
+ mgr = new HotReloadManager(createRecordingLogger([]));
+ });
+
+ it('refuses startWatching with an ADR-0112 envelope and the prescription', () => {
+ mgr.registerPlugin('p', liveConfig());
+
+ let caught: (Error & { code?: string; status?: number }) | undefined;
+ try {
+ mgr.startWatching('p');
+ } catch (e) {
+ caught = e as Error & { code?: string; status?: number };
+ }
+
+ // The envelope, not merely "it threw": a bare toThrow() would stay green
+ // against any unrelated failure on this path.
+ expect(caught, 'startWatching must be refused').toBeDefined();
+ expect(caught?.code).toBe('VALIDATION_ERROR');
+ expect(caught?.status).toBe(400);
+
+ // The prescription's load-bearing facts, by CONTENT — this message is the
+ // whole migration document for whoever hits it.
+ const m = caught?.message ?? '';
+ expect(m).toContain('#12428');
+ expect(m).toContain('ADR-0049');
+ expect(m).toContain('never watched');
+ expect(m).toContain('scheduleReload');
+ expect(m).toContain('p'); // locates the offending plugin
+ });
+
+ it('refuses startWatching for an UNREGISTERED plugin too', () => {
+ // The old body early-returned when the plugin was unknown or disabled, so
+ // the lie was conditional. The refusal must not be: the method never
+ // worked for anyone, in any state.
+ expect(() => mgr.startWatching('never-registered')).toThrow(/#12428/);
+ });
+
+ it('refuses a leftover watchPatterns at registration', () => {
+ // The schema is not .strict(), so zod would STRIP this key on any parse
+ // path — a clean parse and a setting that never takes effect. Route 3 left
+ // no parse-time prescription (nothing parses the schema), so THIS is the
+ // door that keeps the removal honest for the audience that exists.
+ let caught: (Error & { code?: string; status?: number }) | undefined;
+ try {
+ mgr.registerPlugin('p', liveConfig({ watchPatterns: ['src/**/*.ts'] }));
+ } catch (e) {
+ caught = e as Error & { code?: string; status?: number };
+ }
+ expect(caught).toBeDefined();
+ expect(caught?.code).toBe('VALIDATION_ERROR');
+ expect(caught?.status).toBe(400);
+ expect(caught?.message).toContain('watchPatterns');
+ expect(caught?.message).toContain('#12428');
+ expect(caught?.message).toContain('nothing ever read it');
+ });
+
+ it('refuses watchPatterns even when hot reload is disabled', () => {
+ // The door must not depend on `enabled` — a false declaration is false
+ // whether or not the feature is switched on.
+ expect(() =>
+ mgr.registerPlugin('p', liveConfig({ enabled: false, watchPatterns: ['a/**'] }))
+ ).toThrow(/#12428/);
+ });
+
+ it('still registers a config that does not carry the retired key', () => {
+ // Anti-vacuity for the door: the refusals above must be about the key, not
+ // about registration having broken.
+ expect(() => mgr.registerPlugin('p', liveConfig())).not.toThrow();
+ });
+
+ it('stopWatching still cancels a pending debounced reload', () => {
+ // Behaviour preservation for the `watchHandles` removal. What left was the
+ // unreachable cleanup branch; the half that always did something — clearing
+ // the debounce timer armed by `scheduleReload` — is untouched.
+ vi.useFakeTimers();
+ try {
+ mgr.registerPlugin('p', liveConfig());
+ let ran = 0;
+ mgr.scheduleReload('p', async () => { ran++; });
+
+ mgr.stopWatching('p');
+ vi.advanceTimersByTime(5000);
+ expect(ran, 'the scheduled reload must have been cancelled').toBe(0);
+ } finally {
+ vi.useRealTimers();
+ }
+ });
+
+ it('shutdown still clears pending timers without the dead teardown loop', () => {
+ // The removed loop iterated `watchHandles.keys()` and therefore ran zero
+ // times; every timer it could have reached is cleared by the
+ // `reloadTimers` loop that follows it. This pins that equivalence.
+ vi.useFakeTimers();
+ try {
+ mgr.registerPlugin('p', liveConfig());
+ let ran = 0;
+ mgr.scheduleReload('p', async () => { ran++; });
+
+ mgr.shutdown();
+ vi.advanceTimersByTime(5000);
+ expect(ran, 'shutdown must leave no pending reload behind').toBe(0);
+ expect(vi.getTimerCount()).toBe(0);
+ } finally {
+ vi.useRealTimers();
+ }
+ });
+});