fix(metadata-fs): declare startWatcher()'s chokidar atomic option explicitly (#12757)

* fix(metadata-fs): declare startWatcher()'s chokidar atomic option explicitly

Not passing `atomic` left it inherited from chokidar's own defaults merge,
which assigns `atomic: true` before the caller's options are spread in --
so chokidar's own default-correction for usePolling (which would want
`atomic: false` here) is dead code and never fires. Runtime-verified the
resolved value is unchanged (true, both before and after); this only makes
the value declared instead of inherited from a branch that cannot execute.

Adds a pin that spies on the actual chokidar.watch() call startWatcher()
makes and asserts `atomic` is an OWN key of the options object -- a merged
`watcher.options.atomic` read can't distinguish declared from inherited,
since both resolve to `true` today.

Part of #12696

* test(metadata-fs): split the atomic-declared pin's positive control into its own case

The usePolling control assertions were unreachable when the atomic
assertion above them failed (single it() block, assertions run in order),
so the control never actually demonstrated anything under the ablation.
Splitting it into its own it() lets it run to completion, and show green,
independently of the atomic case's verdict.

Part of #12696

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/metadata-fs-watcher-atomic-declared.md

@@ -0,0 +1,33 @@
+---
+"@objectstack/metadata-fs": patch
+---
+
+fix(metadata-fs): declare `startWatcher()`'s chokidar `atomic` option explicitly (#12696)
+
+`FileSystemRepository.startWatcher()` constructed its chokidar watcher with
+`usePolling: true` but never passed `atomic`, leaving it to inherit chokidar's
+default. That default is unconditionally `true` in the installed version
+(chokidar 5.0.0): the defaults literal assigns `atomic: true` *before* the
+caller's options are spread in, so chokidar's own default-correction
+(`if (opts.atomic === undefined) opts.atomic = !opts.usePolling`) can never
+fire — it only runs when `atomic` is literally `undefined` after the merge,
+which it never is. The comment beside that correction ("Editor atomic write
+normalization enabled by default with fs.watch") reads as "off under
+polling"; the actual resolved behaviour was on regardless.
+
+This change passes `atomic: true` explicitly at the call site, with a comment
+explaining why. **Patch, not a behaviour change**: verified at runtime
+(constructing a watcher the way `startWatcher()` does and reading back
+`watcher.options.atomic`) that the resolved value is identical before and
+after — `true` either way, today. The only thing that changes is that the
+value is now DECLARED rather than inherited from an upstream branch that
+cannot execute, so a future chokidar release that fixes the ordering (making
+the correction real) cannot silently flip this repository's watcher to
+`atomic: false` under polling and change behaviour with no diff to review.
+
+Not addressed here (see #12696): whether `atomic: true` (the 100ms
+unlink-coalescing deferral and the `DOT_RE` editor-temp-file matcher it turns
+on) is actually the right value. No evidence surfaced that either has ever
+affected a run; flipping it to `false` is a deliberate behaviour change to a
+live delivery path that needs its own reverse verification, and is out of
+scope for this card.

packages/metadata-fs/src/repository.ts

@@ -595,6 +595,23 @@ export class FileSystemRepository implements MetadataRepository {
       usePolling: true,
       interval: 1000,
       binaryInterval: 2000,
+      // Declared explicitly, not inherited. chokidar's own default-correction
+      // (`if (opts.atomic === undefined) opts.atomic = !opts.usePolling`) can
+      // only fire when the caller omits `atomic`, but its defaults literal
+      // already assigns `atomic: true` *before* the caller's options are
+      // spread in — so leaving `atomic` unset here does not mean "off under
+      // polling" the way the correction's own comment claims, it silently
+      // resolves to `true` regardless of `usePolling`. That has been this
+      // repository's actual runtime behaviour all along (verified by reading
+      // back the resolved option from a real watcher instance, #12696): every
+      // `unlink` gets chokidar's 100ms editor-atomic-write deferral, and
+      // `DOT_RE` (vim swap files, `~`, sublime tmp) is folded into
+      // `_isIgnored` on top of this repository's own `isIgnoredWatchPath`
+      // (#7150). `atomic: true` here keeps that behaviour byte-for-byte —
+      // this is a declaration, not a change. Flipping it to `false` would
+      // remove both behaviours from a live delivery path and needs its own
+      // reverse verification; see #12696 for the analysis.
+      atomic: true,
     });
     w.on('add', (p) => void this.handleFsChange(p, 'add'));
     w.on('change', (p) => void this.handleFsChange(p, 'change'));

packages/metadata-fs/test/watcher-atomic-declared.test.ts

@@ -0,0 +1,113 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #12696 — `startWatcher()` must pass `atomic` to chokidar EXPLICITLY.
+ *
+ * chokidar 5's defaults literal assigns `atomic: true` BEFORE the caller's
+ * options are spread in (`node_modules/chokidar/index.js`):
+ *
+ *   const opts = {
+ *     // Defaults
+ *     ...
+ *     atomic: true, // NOTE: overwritten later (depends on usePolling)
+ *     ..._opts,
+ *     ...
+ *   };
+ *   ...
+ *   // Editor atomic write normalization enabled by default with fs.watch
+ *   if (opts.atomic === undefined)
+ *       opts.atomic = !opts.usePolling;
+ *
+ * so the "overwritten later" comment is aspirational: the correction can
+ * only run when the caller omits `atomic` AND that omission left
+ * `opts.atomic === undefined`, but it never does — the defaults literal
+ * already assigned `true`, and the caller's spread has no `atomic` key to
+ * override it with. The branch is dead.
+ *
+ * The consequence for THIS pin: the RESOLVED value chokidar hands back
+ * (`watcher.options.atomic`) is `true` whether `startWatcher()` declares it
+ * or not — a merged-value read cannot tell "declared here" apart from
+ * "inherited a dead branch that happens to agree today". A pin written
+ * against that merged read would stay green across the exact regression it
+ * exists to catch: a future chokidar release that fixes the ordering (making
+ * the correction real) would then silently flip this repository's watcher to
+ * `atomic: false` under `usePolling` — losing the 100ms unlink-coalescing
+ * deferral and the `DOT_RE` editor-temp matcher from a live delivery path —
+ * and nothing here would notice.
+ *
+ * So this pin does not read the merged option. It reads the ACTUAL call
+ * `startWatcher()` makes to `chokidar.watch()`, via a spy that lets the real
+ * call through unchanged (this pins DECLARATION, not delivery — every other
+ * watcher behaviour in this package must keep working identically). That is
+ * a runtime observation of what this repository hands off, not a grep of the
+ * call-site literal, and it is the one thing whose presence a future
+ * chokidar default cannot silently override.
+ *
+ * ⛔ No wall-clock wait anywhere in this file, deliberately (see
+ * `watch-dot-root.test.ts` for the standing prohibition and its history of
+ * merge-queue ejections). None is needed: the root is created by `mkdtemp()`
+ * before `start()` runs, so `start()` arms the watcher SYNCHRONOUSLY inside
+ * its own call (see its comment on #7000/#9339) — `chokidar.watch()` is
+ * invoked, and the spy has recorded the call, by the time `start()`'s
+ * promise resolves. This case never touches the 100ms unlink window itself
+ * (out of scope for #12696 — see the card's "carry forward" section).
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import fs from 'node:fs/promises';
+import path from 'node:path';
+import os from 'node:os';
+import chokidar from 'chokidar';
+import { FileSystemRepository } from '../src/index.js';
+
+describe('FileSystemRepository — startWatcher() declares `atomic` explicitly (#12696)', () => {
+  let root: string;
+  let repo: FileSystemRepository | undefined;
+  let watchSpy: ReturnType<typeof vi.spyOn>;
+
+  beforeEach(async () => {
+    root = await fs.mkdtemp(path.join(os.tmpdir(), 'objectstack-fsatomic-'));
+    // Let the real call through — nothing here is faked, so every other
+    // watcher behaviour this case exercises stays exactly as it runs in
+    // production.
+    watchSpy = vi.spyOn(chokidar, 'watch');
+  });
+
+  afterEach(async () => {
+    if (repo) await repo.close().catch(() => undefined);
+    repo = undefined;
+    watchSpy.mockRestore();
+    await fs.rm(root, { recursive: true, force: true });
+  });
+
+  it('passes `atomic: true` as an OWN key of the options object handed to chokidar.watch()', async () => {
+    repo = new FileSystemRepository({ root, org: 'system', disableWatch: false });
+    await repo.start();
+
+    expect(watchSpy).toHaveBeenCalledTimes(1);
+    const [, options] = watchSpy.mock.calls[0] as [string, Record<string, unknown>];
+
+    // The behaviour this pin exists to protect: `atomic` must be an OWN key
+    // of the call-site options object, not merely absent-and-coincidentally
+    // `true` after chokidar's internal merge (see file header).
+    expect(Object.prototype.hasOwnProperty.call(options, 'atomic')).toBe(true);
+    expect(options.atomic).toBe(true);
+  });
+
+  // Positive control (#12696 ablation) — a SEPARATE case so it runs to
+  // completion, and so its verdict, on its own assertions, is independent of
+  // whatever the case above does. `usePolling` is unconditionally declared
+  // today and untouched by this card's fix; it must stay green under the
+  // ablation that removes the explicit `atomic`. If it ever went red too,
+  // the spy/harness would be the suspect, not the `atomic` declaration.
+  it('[control] passes `usePolling: true` as an OWN key of the same options object', async () => {
+    repo = new FileSystemRepository({ root, org: 'system', disableWatch: false });
+    await repo.start();
+
+    expect(watchSpy).toHaveBeenCalledTimes(1);
+    const [, options] = watchSpy.mock.calls[0] as [string, Record<string, unknown>];
+
+    expect(Object.prototype.hasOwnProperty.call(options, 'usePolling')).toBe(true);
+    expect(options.usePolling).toBe(true);
+  });
+});