From b9b63ff97703a454898da4e611d80a3119d27f2b Mon Sep 17 00:00:00 2001 From: autodev-bot Date: Fri, 9 Oct 2026 04:26:45 +0800 Subject: [PATCH 1/2] fix(hermes): rebuild-if-stale guard must track the ESM entry (#2470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_rebuild_if_stale` in the Hermes adapter compared TypeScript source timestamps against `dist/bridge.cjs`, but both spawn paths prefer `dist/bridge.mjs` since the ESM migration (#1736 / #1998). A stale `bridge.mjs` next to a fresh `bridge.cjs` therefore slipped past the guard and the daemon kept booting the stale ESM artifact without any warning — exactly the failure mode #2399 / #2465 triggered. Mirror `_bridge_script()`'s compiled-entry precedence and apply the stricter variant: rebuild when any compiled entry on disk is older than the newest `*.ts` source, treat a missing `dist/` as stale, and name the stale artifacts in the log line. Covered by 9 new unittest cases in `tests/python/test_rebuild_if_stale.py`, including the headline "stale .mjs, fresh .cjs, newer source" scenario the issue describes. Full plugin python suite (141 tests) and ruff check / format pass. --- .../hermes/memos_provider/daemon_manager.py | 53 +++-- .../tests/python/test_rebuild_if_stale.py | 187 ++++++++++++++++++ 2 files changed, 226 insertions(+), 14 deletions(-) create mode 100644 apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py diff --git a/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py b/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py index d7e6b6e8d..c124f9324 100644 --- a/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py +++ b/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py @@ -185,25 +185,50 @@ def _bridge_command(*, daemon: bool, runtime_home: Path | None = None) -> list[s def _rebuild_if_stale() -> bool: - """Run `npm run build` if any TypeScript source is newer than dist/bridge.cjs. - - Returns True if the binary is current or the build succeeded. - Returns False if the build failed — caller should continue with - the existing binary rather than blocking gateway startup. + """Run ``npm run build`` if any compiled bridge entry is older than its sources. + + The compiled candidates mirror :func:`_bridge_script`'s precedence + (``dist/bridge.mjs`` and ``dist/bridge.cjs``). The guard rebuilds + when any compiled entry that currently exists on disk is older than + the newest ``*.ts`` source under ``_plugin_root()``, so a partial + build that only refreshed one artifact — or a stale ESM entry next + to a fresh CJS entry (issue #2470) — no longer slips through. A + missing ``dist/`` is also treated as stale. + + Returns True if every compiled entry is current or the build + succeeded. Returns False if the build failed — callers should + continue with the existing binary rather than blocking gateway + startup. """ plugin_root = _plugin_root() - compiled = plugin_root / "dist" / "bridge.cjs" + # Keep this tuple in sync with the compiled candidates in + # `_bridge_script()` / `bridge_client._bridge_script()`. The source + # entries (`bridge.mts`, `bridge.cts`) are intentionally excluded — + # they are development inputs, not artifacts the build produces. + compiled_candidates = ( + plugin_root / "dist" / "bridge.mjs", + plugin_root / "dist" / "bridge.cjs", + ) + existing = [p for p in compiled_candidates if p.exists()] + newest_source = max( + (p.stat().st_mtime for p in plugin_root.rglob("*.ts")), + default=0.0, + ) - if compiled.exists(): - compiled_mtime = compiled.stat().st_mtime - newest_source = max( - (p.stat().st_mtime for p in plugin_root.rglob("*.ts")), - default=0.0, - ) - if newest_source <= compiled_mtime: + if existing: + compiled_mtimes = {p: p.stat().st_mtime for p in existing} + # Rebuild when *any* existing compiled entry is older than the + # newest source — the stricter variant from issue #2470 covers + # partial builds that leave one artifact fresh and the other + # stale. + if newest_source <= min(compiled_mtimes.values()): return True + stale_names = sorted(p.name for p, m in compiled_mtimes.items() if m < newest_source) + trigger = ", ".join(f"dist/{n}" for n in stale_names) or "dist/bridge.*" + logger.info("MemOS: TypeScript source newer than %s — rebuilding...", trigger) + else: + logger.info("MemOS: no compiled bridge entry under dist/ — building...") - logger.info("MemOS: TypeScript source newer than dist/bridge.cjs — rebuilding...") npm = shutil.which("npm") if not npm: logger.warning("MemOS: npm not found on PATH; skipping bridge rebuild") diff --git a/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py b/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py new file mode 100644 index 000000000..76b68b4af --- /dev/null +++ b/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py @@ -0,0 +1,187 @@ +"""Regression tests for `_rebuild_if_stale` (Issue #2470). + +Pre-fix, `_rebuild_if_stale()` compared TypeScript source timestamps against +`dist/bridge.cjs` only. Since the ESM migration (#1736 / #1998) both spawn +paths prefer `dist/bridge.mjs`, so a stale `.mjs` next to a fresh `.cjs` +would silently skip the rebuild and the daemon would start the stale ESM +artifact. + +The fix compares against **every compiled entry that exists** (mirroring +`_bridge_script()`'s precedence) and rebuilds if any one of them is older +than the newest TypeScript source. Missing compiled entries are also +treated as stale. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import unittest + +from pathlib import Path +from tempfile import TemporaryDirectory +from unittest.mock import MagicMock, patch + + +_ADAPTER_ROOT = Path(__file__).resolve().parent.parent.parent / "adapters" / "hermes" +_PLUGIN_DIR = _ADAPTER_ROOT / "memos_provider" +for _p in (_ADAPTER_ROOT, _PLUGIN_DIR): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +import daemon_manager as daemon_manager_mod # noqa: E402 + + +def _write(path: Path, mtime: float, body: str = "// stub\n") -> None: + """Create ``path`` (and parents) and stamp it with a known mtime.""" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body) + os.utime(path, (mtime, mtime)) + + +class _StaleGuardTestBase(unittest.TestCase): + """Shared harness: patch `_plugin_root` to a tmpdir and track `subprocess.run`.""" + + def setUp(self) -> None: + self._tmp = TemporaryDirectory() + self.root = Path(self._tmp.name) + # Install plugin_root patch for every test — _rebuild_if_stale reads it. + patch_root = patch.object( + daemon_manager_mod, + "_plugin_root", + return_value=self.root, + ) + self.addCleanup(patch_root.stop) + patch_root.start() + # Pretend npm is on PATH; individual tests can override. + patch_which = patch.object( + daemon_manager_mod.shutil, + "which", + return_value="/usr/bin/npm", + ) + self.addCleanup(patch_which.stop) + patch_which.start() + # Capture subprocess.run so tests can assert whether npm was invoked + # without actually launching anything. + patch_run = patch.object( + daemon_manager_mod.subprocess, + "run", + return_value=MagicMock(stderr="", stdout=""), + ) + self.addCleanup(patch_run.stop) + self.run_mock = patch_run.start() + + def tearDown(self) -> None: + self._tmp.cleanup() + + +class RebuildIfStaleRegressionTests(_StaleGuardTestBase): + """Direct behaviour tests covering the Issue #2470 scenarios.""" + + # Sources use `.ts` extension — the current `rglob("*.ts")` scan does + # not match `.mts` / `.cts`, and the issue explicitly leaves that scan + # alone. See issue #2470 ("The `rglob("*.ts")` source scan can stay as + # it is."). + + def test_both_compiled_fresh_skips_rebuild(self) -> None: + """Baseline sanity: nothing to do when every compiled entry is current.""" + source_mtime = 1_000.0 + compiled_mtime = source_mtime + 10.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", compiled_mtime) + _write(self.root / "dist" / "bridge.cjs", compiled_mtime) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_not_called() + + def test_stale_mjs_fresh_cjs_still_rebuilds(self) -> None: + """The headline bug from #2470. + + `dist/bridge.cjs` is newer than the sources, but the ESM entry + `dist/bridge.mjs` is older. Pre-fix this returned True without + rebuilding and the daemon then ran the stale `.mjs`. + """ + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + # mjs is older than source → should trigger rebuild + _write(self.root / "dist" / "bridge.mjs", source_mtime - 50.0) + # cjs is newer than source → old logic was fooled by this + _write(self.root / "dist" / "bridge.cjs", source_mtime + 50.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + cmd = self.run_mock.call_args.args[0] + self.assertEqual(cmd[0], "/usr/bin/npm") + self.assertEqual(cmd[1:], ["run", "build"]) + + def test_fresh_mjs_stale_cjs_still_rebuilds(self) -> None: + """Symmetric case: `.mjs` fresh but `.cjs` stale also triggers a build. + + A partial build that only refreshed the ESM entry still leaves + `dist/` inconsistent; keeping both compiled entries in lock-step + matches the "stricter option" from the issue. + """ + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", source_mtime + 50.0) + _write(self.root / "dist" / "bridge.cjs", source_mtime - 50.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + + def test_only_mjs_present_and_stale_rebuilds(self) -> None: + """Legacy CJS artifact removed; stale ESM entry alone must still rebuild.""" + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", source_mtime - 10.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + + def test_only_mjs_present_and_fresh_skips_rebuild(self) -> None: + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", source_mtime + 10.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_not_called() + + def test_only_cjs_present_and_stale_rebuilds(self) -> None: + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.cjs", source_mtime - 10.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + + def test_no_compiled_entry_triggers_build(self) -> None: + """A missing `dist/` must not silently claim the binary is current.""" + _write(self.root / "core" / "types.ts", 1_000.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + + def test_missing_npm_returns_false_without_running(self) -> None: + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", source_mtime - 10.0) + + with patch.object(daemon_manager_mod.shutil, "which", return_value=None): + self.assertFalse(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_not_called() + + def test_build_failure_returns_false(self) -> None: + source_mtime = 1_000.0 + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", source_mtime - 10.0) + + self.run_mock.side_effect = subprocess.CalledProcessError( + returncode=1, cmd=["npm", "run", "build"], stderr="boom\n" + ) + self.assertFalse(daemon_manager_mod._rebuild_if_stale()) + self.run_mock.assert_called_once() + + +if __name__ == "__main__": + unittest.main() From e96524f5b8c50390eb962c98b0e81e13ccb6b3a7 Mon Sep 17 00:00:00 2001 From: Autodev Bot Date: Fri, 9 Oct 2026 04:34:26 +0800 Subject: [PATCH 2/2] fix(hermes): exclude dist/ from source scan + sync stale-names condition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #2470. Two issues flagged by the automated review: 1. `_rebuild_if_stale()` scans the plugin tree with `rglob("*.ts")`, which also matches `.d.ts` declaration files. The project tsconfig has `"declaration": true` and `"outDir": "dist"`, so every build writes `.d.ts` artifacts under `dist/`. One of those (e.g. `dist/bridge.d.ts`) can end up with an mtime newer than the compiled `bridge.mjs`/`.cjs`, which inflates `newest_source` past the compiled artifacts and triggers a spurious rebuild — forever, on every startup. Fix: exclude anything whose parents include the `dist/` subtree from the source scan. 2. The `stale_names` filter used `m < newest_source` while the guard above uses `newest_source <= min(compiled_mtimes.values())` as its "fresh" check. Reword the filter as `not (newest_source <= m)` so the two expressions share the same notion of "not fresh". The change is semantically equivalent to the previous form; only the logged `trigger` list is affected, and only when timestamps match exactly (a cosmetic/debugging concern noted in the review). Also adds a regression test (`test_dist_dts_artifacts_do_not_trigger_ spurious_rebuild`) that writes a `.d.ts` under `dist/` with an mtime newer than `bridge.mjs`/`.cjs` and asserts no rebuild is attempted. Verified by temporarily reverting the source scan filter — the test fails as expected without the fix. Verification: python3 -m pytest apps/memos-local-plugin/tests/python/ test_rebuild_if_stale.py -v -> 10 passed. Full plugin suite: python3 -m pytest apps/memos-local-plugin/tests/python/ -v -> 153 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../hermes/memos_provider/daemon_manager.py | 14 +++++++++-- .../tests/python/test_rebuild_if_stale.py | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py b/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py index c124f9324..f885357ca 100644 --- a/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py +++ b/apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py @@ -210,8 +210,15 @@ def _rebuild_if_stale() -> bool: plugin_root / "dist" / "bridge.cjs", ) existing = [p for p in compiled_candidates if p.exists()] + # Exclude the `dist/` subtree from the source scan: `tsconfig.json` + # has `"declaration": true` + `"outDir": "dist"`, so each successful + # build writes `.d.ts` declaration files under `dist/` whose mtimes + # would otherwise be picked up by `rglob("*.ts")` and inflate + # `newest_source` past the compiled artifacts, triggering a spurious + # rebuild on the very next startup (and so on in a loop). + dist_dir = plugin_root / "dist" newest_source = max( - (p.stat().st_mtime for p in plugin_root.rglob("*.ts")), + (p.stat().st_mtime for p in plugin_root.rglob("*.ts") if dist_dir not in p.parents), default=0.0, ) @@ -223,7 +230,10 @@ def _rebuild_if_stale() -> bool: # stale. if newest_source <= min(compiled_mtimes.values()): return True - stale_names = sorted(p.name for p, m in compiled_mtimes.items() if m < newest_source) + # Mirror the guard's "not fresh" expression (`newest_source <= m` + # ⇒ fresh) so the logged `trigger` list stays consistent with the + # rebuild decision above. + stale_names = sorted(p.name for p, m in compiled_mtimes.items() if not (newest_source <= m)) trigger = ", ".join(f"dist/{n}" for n in stale_names) or "dist/bridge.*" logger.info("MemOS: TypeScript source newer than %s — rebuilding...", trigger) else: diff --git a/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py b/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py index 76b68b4af..ae76d868b 100644 --- a/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py +++ b/apps/memos-local-plugin/tests/python/test_rebuild_if_stale.py @@ -182,6 +182,31 @@ def test_build_failure_returns_false(self) -> None: self.assertFalse(daemon_manager_mod._rebuild_if_stale()) self.run_mock.assert_called_once() + def test_dist_dts_artifacts_do_not_trigger_spurious_rebuild(self) -> None: + """OCR regression: `dist/*.d.ts` must not bump `newest_source`. + + `tsconfig.json` has `declaration: true` + `outDir: "dist"`, so a + successful build writes `.d.ts` files under `dist/`. If those + artifacts are picked up by the `rglob("*.ts")` source scan (they + end in `.ts`), a `.d.ts` with mtime > the compiled `.mjs`/`.cjs` + would make the guard see sources as newer and rebuild on every + startup forever. Fix: exclude the `dist/` subtree from the scan. + """ + source_mtime = 1_000.0 + compiled_mtime = source_mtime + 10.0 + # Fresh TS source and compiled entries... + _write(self.root / "core" / "types.ts", source_mtime) + _write(self.root / "dist" / "bridge.mjs", compiled_mtime) + _write(self.root / "dist" / "bridge.cjs", compiled_mtime) + # ...and a declaration file written by the previous build whose + # mtime happens to be newer than the compiled artifacts (e.g. + # the compiler flushed .d.ts after bridge.mjs). + _write(self.root / "dist" / "bridge.d.ts", compiled_mtime + 5.0) + + self.assertTrue(daemon_manager_mod._rebuild_if_stale()) + # No rebuild — the `.d.ts` under dist/ must not count as source. + self.run_mock.assert_not_called() + if __name__ == "__main__": unittest.main()