fix(resource-loader): track mod.ts-based extension dirs in managed-resources manifest#3481
fix(resource-loader): track mod.ts-based extension dirs in managed-resources manifest#3481NilsR0711 wants to merge 1 commit intogsd-build:mainfrom
Conversation
…sources manifest remote-questions uses mod.ts as its entry point instead of index.ts. The installedExtensionDirs filter in writeManagedResourceManifest only checked for index.js/index.ts, so remote-questions was silently excluded from the manifest. This caused two problems on upgrades: - Partial copies of remote-questions were not detected or repaired - The directory was never pruned when removed from the bundle The symptom was a crash at startup: Cannot find module '.../remote-questions/format.js' Fix: extend the entry-point check to also match mod.js/mod.ts. Adds a regression test that reads managed-resources.json after initResources() and asserts remote-questions is present in installedExtensionDirs. Closes gsd-build#2367 Verified with AI.
🟠 PR Risk Report — HIGH
Affected Systems
File Breakdown
|
trek-e
left a comment
There was a problem hiding this comment.
Verdict: APPROVE | Severity: MINOR
The fix is correct and the regression test validates the stated behavior. A few observations:
What's right:
- One-line change in
writeManagedResourceManifestis minimal and correct.mod.js/mod.tsare the only other entry-point conventions present in the bundled extensions dir per the PR description, so the expansion is bounded. - The test drives
initResources()against a real temp dir and reads the manifest — not a mock, not a source-scan. This is the right level of validation for a file-system–touching path. t.aftercleanup is correct and won't leak on failure.
MINOR — test doesn't actually exercise the fix:
The new test (initResources tracks mod.ts-based extension dirs) calls initResources(fakeAgentDir) against an empty temp dir that has no bundled extensions at all. It then asserts dirs.includes("remote-questions") — but remote-questions is a bundled extension that lives inside the installed package, not the test's temp dir. Unless initResources copies the real bundled extensions into the temp dir during the call (i.e., it runs copyDirRecursive from the real bundledExtensionsDir), this test will pass vacuously if installedExtensionDirs simply doesn't exist yet, or it will fail because remote-questions was never written there. Either outcome means the test is not actually asserting what the comment claims.
The existing passing-CI state may mask this if initResources writes real extension dirs in place during the test run. Confirm the test actually goes red without the one-line fix — the PR description says "verified red before fix", which is the key claim here.
MINOR — existsSync probing has a TOCTOU window:
Not actionable in this PR, but the filter does four existsSync calls per directory. Under concurrent initResources calls (unlikely but possible on first install) a directory could be deleted between the probe and the subsequent copy. Not a blocker for this fix.
No security or logic issues. The fix itself is exactly right.
TL;DR
What:
remote-questionswas silently excluded frominstalledExtensionDirsinmanaged-resources.json.Why: The entry-point filter only checked for
index.js/index.ts;remote-questionsusesmod.ts.How: Extend the filter to also match
mod.js/mod.ts.What
src/resource-loader.ts— one-line fix inwriteManagedResourceManifestsrc/tests/resource-loader.test.ts— regression test (test-first, verified red before fix)Why
writeManagedResourceManifestbuildsinstalledExtensionDirsby scanningbundledExtensionsDirand keeping only subdirectories that containindex.jsorindex.ts.remote-questionsusesmod.tsas its entry point and has noindex.*file — so it was never recorded in the manifest.This caused two problems:
initResourcesskips the sync when version + content hash match. If a previous sync was interrupted and only 8 of 14 files landed, the missing files (format.js,notify.js,telegram-adapter.js, …) are never restored.remote-questionsis removed from the bundle in a future release, the stale copy in~/.gsd/agent/extensions/remote-questions/would remain forever.Observed crash symptom:
How
Extended the filter from:
to also match
mod.js/mod.ts— the two entry-point conventions present in the bundled extensions directory.The regression test runs
initResources()against a temp dir and reads the resultingmanaged-resources.json, asserting thatremote-questionsappears ininstalledExtensionDirs.Closes #2367
Verified with AI.