Repository navigation
Fix #2470: [Bug] Hermes adapter: _rebuild_if_stale() checks dist/bridge.cjs, but the runtim - #2472
Open
Memtensor-AI wants to merge 2 commits into
Open
Memtensor-AI wants to merge 2 commits into
Memtensor-AI wants to merge 2 commits into
Conversation
…or#2470) `_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 (MemTensor#1736 / MemTensor#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 MemTensor#2399 / MemTensor#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.
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2472 ✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 2 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
Review follow-up on MemTensor#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) <noreply@anthropic.com>
Collaborator
Author
❌ Automated Test Results: FAILED
Error detailsBranch: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix Issue #2470:
_rebuild_if_stale()in the Hermes adapter (apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py) was comparing TypeScript source timestamps againstdist/bridge.cjs, but both spawn paths (daemon_manager._bridge_script()andbridge_client._bridge_script()) preferdist/bridge.mjssince the ESM migration (#1736 / #1998). This let a stalebridge.mjsnext to a freshbridge.cjsslip past the rebuild guard — the daemon then kept booting the stale ESM artifact without any warning, which is exactly the failure mode #2399 / #2465 triggered.The fix mirrors
_bridge_script()'s compiled-entry precedence (dist/bridge.mjs,dist/bridge.cjs) and adopts the stricter variant from the issue: rebuild when any compiled entry on disk is older than the newest*.tssource, treat a missingdist/as stale, and name the stale artifacts in the log line for easier diagnosis of future partial-build regressions.Behaviour change is narrow — only two matrix rows flip: (a) the headline bug "stale .mjs + fresh .cjs + newer source" now rebuilds, and (b) "only .mjs present + fresh" now correctly skips the rebuild (old code treated missing
.cjsas stale). All other branches (missingnpm, build failure, build timeout, both compiled fresh, etc.) keep identical behaviour.Tests: new file
apps/memos-local-plugin/tests/python/test_rebuild_if_stale.pywith 9 unittest cases covers every branch, including the exact failure scenario from the issue (TDD: 2 cases went red before the fix, all 9 green after). Full plugin python suite (141 tests) passes with no regressions; siblingtest_bridge_script_resolution(11 tests) also passes.ruff check+ruff format --checkboth clean. No API / schema / deps change. Reviewers: @whipser030, @hijzy.Related Issue (Required): Fixes #2470
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist