fix(cache): repair discovery globs, error discrimination, cache-dir linking, manifest refcounts, and LRU adapter defects - #3329
Conversation
…inking, manifest refcounts, and LRU adapter defects
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change updates bundle manifest reference cleanup, memory cache expiration and state handling, cache filesystem operations, and file discovery ignore-pattern matching. Tests cover reference counts, snapshots, injected clocks, filesystem errors, eviction handling, and wildcard patterns. ChangesBundle manifest lifecycle
Memory cache behavior
Cache filesystem operations
File discovery matching
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple cache and filesystem correctness issues in src/utils, including discovery ignore globs, cache file existence error discrimination, per-cache-dir node_modules linking on Node, bundle-manifest index/refcount correctness, and several LRU memory-cache adapter behaviors (with deterministic expiry testing support).
Changes:
- Fix file discovery ignore handling to support
*/?glob patterns (without regex compilation) and add coverage for glob ignores. - Make cache existence checks distinguish true absence from operational/stat failures (rethrowing non-NotFound errors) and add tests for those cases.
- Repair cache-dir
node_moduleslinking memoization (per resolved cache base dir + await in-flight work), plus bundle manifest source-index replacement cleanup and shared-code deletion safety, plus LRU adapter fixes (expiry boundary,has()semantics,keys()filtering, robustclear(), tag snapshotting) with new tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/file-discovery.ts | Add non-regex glob matcher and apply it to ignorePatterns. |
| src/utils/file-discovery.test.ts | Add tests covering glob (*) and single-char (?) ignores. |
| src/utils/cache-file-ops.ts | Re-throw non-NotFound stat failures in verifyCacheFileExists (log + propagate). |
| src/utils/cache-file-ops.test.ts | Add tests asserting operational/stat failures are propagated. |
| src/utils/cache-dir.ts | Replace global done-flag with per-cacheBase promise memoization for node_modules linking. |
| src/utils/bundle-manifest.ts | Fix stale source index on replacement; avoid deleting shared code still referenced by other bundles. |
| src/utils/bundle-manifest.test.ts | Add tests for source-index replacement cleanup and shared-code retention. |
| src/utils/cache/stores/memory/types.ts | Add optional now clock injection to LRUCacheOptions for deterministic expiry tests. |
| src/utils/cache/stores/memory/entry-manager.ts | Thread optional now clock into expiry calculation. |
| src/utils/cache/stores/memory/lru-cache-adapter.ts | Fix has() for stored undefined, filter expired keys, robust clear() when onEvict throws, snapshot tag arrays, unify expiry boundary via now. |
| src/utils/cache/stores/memory/lru-cache-adapter.test.ts | Add tests for the adapter fixes, including deterministic expiry boundary checks. |
Verification (reviewer-run):
- Not run in this review environment.
- PR description reports targeted
deno test ... src/utilspassing, plus formatting and typecheck passing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/cache-dir.ts:77
- nodeModulesLinkOperations retains a resolved promise for every distinct cache base dir forever. Because getCacheBaseDir() is AsyncLocalStorage-scoped, test/integration contexts (and potentially long-lived servers) can generate many unique cache dirs, causing this map to grow without bound. Keeping the map only for in-flight link operations still prevents the original race, and avoids unbounded retention.
export async function ensureCacheNodeModules(): Promise<void> {
if (!isNode) return;
// Key the memoized link operation by the resolved cache base dir:
// getCacheBaseDir() is AsyncLocalStorage-scoped, so different requests can
// resolve different cache dirs. A single global done-flag would let the
// first cache dir claim the link forever and leave every other cache dir
// without a node_modules symlink (second React copy → "Invalid hook call").
// Storing the in-flight promise also makes concurrent callers wait for the
// link to actually exist instead of returning before the async work is done.
const cacheBase = getCacheBaseDir();
let operation = nodeModulesLinkOperations.get(cacheBase);
if (!operation) {
operation = linkCacheNodeModules(cacheBase);
nodeModulesLinkOperations.set(cacheBase, operation);
}
await operation;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/cache-dir.ts:75
nodeModulesLinkOperationsgrows a new entry for every distinctcacheBaseand never releases it. SincegetCacheBaseDir()can be AsyncLocalStorage-scoped andrunWithCacheDir()is used with varying temp dirs in tests (and potentially per-project dirs in long-lived processes), this can lead to unbounded memory growth over time.
Consider deleting the memoized promise once it settles. Subsequent calls will re-run linkCacheNodeModules, which returns quickly when the symlink already exists.
let operation = nodeModulesLinkOperations.get(cacheBase);
if (!operation) {
operation = linkCacheNodeModules(cacheBase);
nodeModulesLinkOperations.set(cacheBase, operation);
}
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Review: 79/100 — request changes (narrowly)
| Axis | Score |
|---|---|
| Correctness | 33/40 |
| Test adequacy | 19/25 |
| Security / prod-safety | 16/20 |
| Maintainability | 11/15 |
| Total | 79/100 |
Head 77bef6e43, merge base d63ea1b93. +572/-44 across 11 files (grew from the stated +487/-41). CI green.
All five defects are real and each was verified against the merge base. The refcount work — the highest-risk item — is correct: I traced increment/decrement pairing across every path including error and expiry paths and found no double-free and no new leak. Tests are strong and genuinely red-on-main. This sits one point under the line on three things.
Per-item
| # | Claim | True? | Fix correct? | Residual |
|---|---|---|---|---|
| 1 | shouldIgnore used includes(), so *.test.* never matched |
Yes — all 3 callers pass ["node_modules",".git","__tests__","*.test.*","*.spec.*"]; name.includes("*.test.*") matches nothing |
Yes — matchesEntryGlob is a correct backtracking matcher; traced */?, empty-name, trailing-star, Unicode |
Include path not fixed; silent breaking change; ** unsupported |
| 2 | verifyCacheFileExists swallowed EACCES/EIO as a miss |
Yes | Yes — isNotFoundError covers ENOENT, ENOTDIR, Deno.errors.NotFound, Veryfront file-not-found |
Converts return-false → throw at 3 sites; one loses invalidation |
| 3 | ensureCacheNodeModules global flag set before async work |
Yes — eager let nodeModulesLinked = false vs AsyncLocalStorage-scoped getCacheBaseDir() |
Yes — promise map keyed by resolved cache base, errors swallowed internally so no poisoned promise | Map never invalidated; lstatSync accepts any entry type |
| 4 | Stale source index + shared-code deletion | Yes — setBundleMetadata never removed the key from the old source's set; deleteBundle did code.delete(codeHash) unconditionally |
Yes — refcount trace below | Two pre-existing TTL desync paths |
| 5 | Four LRU adapter defects | Yes, all four | Yes, all four | Boundary change is cosmetic; two isExpired semantics coexist |
Refcount audit — the highest-risk item
Every path traced, not just the happy one:
| Path | Behavior | Verdict |
|---|---|---|
set, no previous |
increment(new) |
balanced |
set, previous same codeHash |
no-op | correct — the key holds exactly one reference |
set, previous different codeHash |
decrement(old) + increment(new) |
balanced |
deleteBundle → removeMetadata |
decrement |
balanced |
invalidateSource |
removeMetadata per key over a copied array; sourceIndex.delete idempotent |
correct |
getBundleMetadata on expiry |
removeMetadata → decrement; another live key's reference keeps count ≥ 1 |
correct |
clear() |
clears codeReferenceCounts too |
correct |
Double removeMetadata(key) |
second call returns early on missing metadata before decrementing | idempotent — no double-free |
setBundleMetadata reads previous via this.metadata.get(key)?.value, bypassing the expiry check — the right choice, since an expired-but-present entry still decrements its old hash.
Leaks only via two pre-existing paths: getBundleCode (bundle-manifest.ts:104) still uses getIfNotExpired, which deletes from this.code without touching codeReferenceCounts — so metadata with a 1 h TTL can point at code with a 60 s TTL and return valid metadata for a missing blob; and metadata that expires unread never decrements, pinning its code (expiry is lazy, no sweeper). Both predate this PR.
P2 — the glob repair is asymmetric; the include path was left broken
file-discovery.ts:54-57. matchesPatterns still does fileName.includes(pattern) and is untouched. A caller passing patterns: ["*.eval.ts"] still matches nothing — exactly the bug just fixed on the other half. The title says "repair discovery globs"; half the glob surface is unrepaired and untested.
P2 — fixing the ignore globs is a silent breaking change
Before, *.test.* and *.spec.* were inert, so eval/task/trigger discovery imported user test files. Discovery does dynamically import (trigger/discovery.ts:395, task/discovery.ts:170 both call importDiscoveryModule), so the body's claim that top-level test code executed outside a test runner is substantiated — a genuine correctness and safety fix.
The flip side: any project defining a task, eval, or trigger in a file matching *.test.*/*.spec.* will have it silently stop being discovered after upgrading. For triggers that means scheduled jobs and webhooks quietly stop registering, with no error. Needs a changelog entry and ideally a one-time warning naming the newly-ignored files.
P2 — the error-discrimination throw skips cache invalidation at one site
ssr-module-loader/loader.ts:254. Previously an EACCES returned false, which ran invalidateMdxEsmCacheEntry + invalidateFilePathCacheEntry and threw a classified error carrying CACHE_FILE_MISSING_PREFIX. Now the raw stat error propagates from verifyCacheFileExists, so neither invalidation runs and the classified prefix is gone — any upstream handler keying on it will not match. The other two sites already threw, so they only lose message quality.
The body's justification ("callers looped forever re-transforming the same module") — I could not find the retry loop that would close that argument. Labeling that claim unverified.
P2 — custom FileSystem adapters rejecting with a bare Error now throw
FileSystem is an injectable interface. isNotFoundError requires a native error brand plus code/Deno-prototype/Veryfront-slug evidence, so an adapter rejecting stat with new Error("not found") now propagates instead of returning false. The PR's own edited test proves the semantics changed — the mock had to be upgraded to an ENOENT-coded error. Disclosed in the body, but it is a real compatibility constraint on a public extension point with no test pinning the new requirement.
P3s
- Body contradicts the diff on metadata cloning. It claims the branch "took none of" the metadata-cloning rewrite; the diff adds
structuredClonein bothgetBundleMetadata:78andsetBundleMetadata:83. Deliberate and well-tested, so the code is fine — the body is wrong. Minor perf cost: a deep clone on every metadata get and set. - Body overcounts discovery callers — claims four (eval/task/trigger/workflow); only three exist.
- The expiry boundary change is a tightening, not a fix. Merge-base was consistent at
now > expiryin bothget()andcleanupExpired(). The PR changes the adapter tonow >= expiry— defensible (a 10 ms TTL should be valid for 10 ms, not 11) but cosmetic, andEvictionManager.isExpired(eviction-manager.ts:155) still usesnow > expiry, so the subsystem now holds two disagreeing definitions. I checked whether the injectednowclock created a split-brain withenforceMemoryLimits— it does not; that path evicts purely on count and size and never consults expiry.
Security surfaces — all clear
- Symlink escape: no.
linkCacheNodeModulescreatesjoin(cacheBase, "node_modules")inside the cache root, pointing at a path derived fromrequire.resolve("react")— trusted local resolution, no untrusted input. - Widened globs exposing secrets/dotfiles: no. The change affects the ignore path only, so it strictly narrows results.
.gitremains a substring pattern. - ReDoS: no.
matchesEntryGlobis hand-written specifically to avoid compiling caller input into a regex — real hardening if these ever become user-supplied. - Evicted-but-referenced entry used after free: no.
get()/has()delete on expiry before returning;keys()/entries()filter without deleting.
Test adequacy
Genuinely strong. The bundle-manifest suite covers refcount pairing properly — transfers code references when metadata is replaced, does not double-count an unchanged key and code hash, releases code and source references when metadata expires, clear resets code reference counts, retains shared code across partial source invalidation. That is the error-path coverage refcount changes need, and it is the best test work in this batch.
Gaps: nothing covers matchesPatterns (because it was not fixed); nothing pins the new bare-Error requirement; nothing covers the getBundleCode TTL/refcount desync; every injected-clock test seeds from a real Date.now(), so a small-epoch clock is untested.
Production risk & rollback
Low-to-moderate, concentrated in item 1. Items 2-5 are contained. Item 1 changes what gets discovered in user projects, and the failure mode is silent — a trigger stops firing — which is the hardest kind to notice. CI is green.
Rollback is clean per-file, but five unrelated fixes in one commit range means reverting one reverts all — poor granularity for a cache subsystem where you may need to back out exactly one behavior change under incident pressure.
To merge
- Fix
matchesPatternstoo, or retitle to say only ignore globs were repaired — and test it. - Changelog entry for the discovery behavior change; consider a one-time warning listing newly-ignored files.
- At
loader.ts:254, keep the invalidation on a non-ENOENT stat failure, or state why dropping it is correct. - Fix the two body inaccuracies.
- Ideally split item 1 out — it is the only one with user-visible behavior change.
|
Addressed the 79/100 review at |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/cache/stores/memory/lru-cache-adapter.ts:88
LRUCacheAdaptersupports an injectednow()clock (andEntryManageruses it), butLRUListManager.moveToFront()/addToFront()still updateentry.lastAccessedviaDate.now()(seesrc/utils/cache/stores/memory/lru-list-manager.ts:15-38). This makeslastAccessedtimestamps non-deterministic even whenoptions.nowis provided, and contradicts the stated goal that the injected clock owns access timestamps.
Consider threading the injected clock into LRUListManager (or having the adapter set entry.lastAccessed = this.now() before moving nodes) so all lastAccessed writes use the same clock.
private readonly now: () => number;
constructor(options: LRUCacheOptions = {}) {
this.maxEntries = options.maxEntries || 1000;
this.maxSizeBytes = options.maxSizeBytes || 50 * 1024 * 1024;
this.defaultTtlMs = options.ttlMs;
this.onEvict = options.onEvict;
this.now = options.now ?? Date.now;
const estimateSizeOf = options.estimateSizeOf ?? defaultSizeEstimator;
this.evictionManager = new EvictionManager({
onEvict: this.onEvict,
loggerContext: "MemoryCache",
});
this.entryManager = new EntryManager(estimateSizeOf, this.now);
}
/** Entries expire exactly at their expiry timestamp. */
private isExpired(entry: LRUEntry<unknown>, now: number): boolean {
return typeof entry.expiry === "number" && now >= entry.expiry;
}
Dismissing per merge-campaign protocol: all review points verified addressed at head fe2d726 by an independent verifier (92% confidence) — glob dispatch preserves OTLP spans and substring semantics; EINVAL fallback byte-identical to main; cache-dir promise map keeps catch semantics; both bundle-manifest tests red on main/green here; LRU fixes are against main's managers. CI fully green; local 936 steps 0 failures.
|
Removed from merge queue for merge-readiness policy compliance. Exact head |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/utils/file-discovery.test.ts:16
- withFixtureTree() will leak the temporary directory if the synchronous build() callback throws before run() is invoked, because cleanup only happens in the Promise.finally() of run(). Wrap build() in a try/catch (or ensure it is covered by the same Promise chain) so the fixture root is always removed.
function withFixtureTree<T>(build: (root: string) => void, run: (root: string) => Promise<T>) {
const root = mkdtempSync(join(tmpdir(), "veryfront-file-discovery-"));
build(root);
return run(root).finally(() => rmSync(root, { recursive: true, force: true }));
}
src/utils/cache-dir.test.ts:147
- These tests embed user-home absolute paths ("/Users/..." and "C:\Users\..."). The repo guidelines avoid including local home-directory paths in code/tests; use generic placeholder paths instead while keeping the redaction behavior under test.
it("redacts both quoted symlink operands when POSIX paths contain spaces", () => {
const cacheRoot = "/Users/Private Person/cache root";
const frameworkRoot = "/Users/Private Person/framework/node_modules";
const reason = `EEXIST: symlink '${frameworkRoot}' -> '${cacheRoot}/node_modules'`;
const redacted = __cacheDirInternals.redactCachePathDetails(reason, cacheRoot);
assertEquals(redacted, "EEXIST: symlink '[path]' -> '[path]'");
assertEquals(redacted.includes("Private Person"), false);
});
it("redacts both quoted symlink operands when Windows paths contain spaces", () => {
const cacheRoot = "C:\\Users\\Private Person\\cache root";
const frameworkRoot = "C:\\Users\\Private Person\\framework\\node_modules";
const reason = `EPERM: symlink '${frameworkRoot}' -> '${cacheRoot}\\node_modules'`;
const redacted = __cacheDirInternals.redactCachePathDetails(reason, cacheRoot);
assertEquals(redacted, "EPERM: symlink '[path]' -> '[path]'");
assertEquals(redacted.includes("Private Person"), false);
});
src/utils/cache-file-ops.ts:79
- The updated JSDoc says this returns false only when the path is absent, but the implementation also returns false when the path exists but is not a regular file (e.g. a directory). Adjust the comment so it matches the actual behavior.
* Verify a cache file exists before attempting dynamic import.
* Returns true if the file exists and is a regular file, false when the path
* is genuinely absent. Non-absence stat failures (EACCES, EIO, ...) are
* rethrown so callers do not misreport an unreadable cache as a cache miss
* and loop forever re-transforming the same module.
|
Merge readiness for 20292b5: Merge confidence: 94%. Reasoning:
I am scheduling this PR with |
Address the remaining suppressed review feedback without changing runtime behavior: fixture setup failures now clean up temp trees, redaction tests avoid user-home-shaped paths, and the cache-file existence contract describes directory results accurately. Constraint: PR review comments requested direct fixes on the current head. Confidence: high Scope-risk: narrow Tested: DENO_TESTING=1 npx --yes deno@2.7.7 test --no-check --allow-all src/utils/file-discovery.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/utils/file-discovery.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.ts Tested: npx --yes deno@2.7.7 lint src/utils/file-discovery.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.ts Tested: npx --yes deno@2.7.7 check src/utils/file-discovery.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.ts Tested: git diff --check
|
Addressed the latest suppressed review feedback in What changed:
Local validation on exact head
Pushed with |
|
Merge readiness at head Merge confidence: 93%. Reasoning: the PR is currently Scheduling for merge with |
Deno fmt now normalizes the workflow YAML comments and expanded needs lists. Committing the generated formatter output keeps PR-local format checks green without changing job behavior. Constraint: PR #3329 touched the workflow and format checks run over changed files. Rejected: Leave the workflow unformatted | deno fmt --check fails on the exact PR surface. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 fmt --check .github/workflows/cicd.yml docs/guides/configuration.md scripts/lint/test-typecheck-baseline.json src/modules/react-loader/ssr-module-loader/loader.test.ts src/modules/react-loader/ssr-module-loader/loader.ts src/transforms/mdx/esm-module-loader/module-writer.test.ts src/transforms/mdx/esm-module-loader/module-writer.ts src/utils/bundle-manifest.test.ts src/utils/bundle-manifest.ts src/utils/cache-dir.test.ts src/utils/cache-dir.ts src/utils/cache-file-ops.test.ts src/utils/cache-file-ops.ts src/utils/cache/eviction/eviction-manager.test.ts src/utils/cache/eviction/eviction-manager.ts src/utils/cache/stores/memory/entry-manager.ts src/utils/cache/stores/memory/lru-cache-adapter.test.ts src/utils/cache/stores/memory/lru-cache-adapter.ts src/utils/cache/stores/memory/lru-list-manager.test.ts src/utils/cache/stores/memory/lru-list-manager.ts src/utils/cache/stores/memory/types.ts src/utils/file-discovery.test.ts src/utils/file-discovery.ts src/utils/lru-wrapper.test.ts src/utils/lru-wrapper.ts Tested: npx --yes deno@2.7.7 lint <changed TS files> Tested: npx --yes deno@2.7.7 check <changed TS files> Tested: DENO_TESTING=1 npx --yes deno@2.7.7 test --no-check --allow-all src/transforms/mdx/esm-module-loader/module-writer.test.ts src/utils/bundle-manifest.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.test.ts src/utils/cache/eviction/eviction-manager.test.ts src/utils/cache/stores/memory/lru-cache-adapter.test.ts src/utils/cache/stores/memory/lru-list-manager.test.ts src/utils/file-discovery.test.ts src/utils/lru-wrapper.test.ts src/modules/react-loader/ssr-module-loader/loader.test.ts Not-tested: Full repository suite on this PR head.
The test used /tmp as the project read root, which canonicalizes to /private/tmp on macOS and can subsume the repository worktree when a PR is reviewed from a temp worktree. That made the extension read root intentionally dedupe away and turned the assertion into a checkout-location dependency rather than a permissions check. Constraint: PR review worktrees may live below the canonicalized temp root.\nRejected: Change worker permission deduplication | the implementation correctly removes child read roots when a broader root is already granted.\nConfidence: high\nScope-risk: narrow\nTested: npx --yes deno@2.7.7 fmt --check src/security/sandbox/worker-pool.test.ts\nTested: npx --yes deno@2.7.7 lint src/security/sandbox/worker-pool.test.ts\nTested: npx --yes deno@2.7.7 check src/security/sandbox/worker-pool.test.ts\nTested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text npx --yes deno@2.7.7 test --preload=src/schemas/_test-setup.ts --no-check --allow-all --unstable-worker-options --unstable-net src/security/sandbox/worker-pool.test.ts\nNot-tested: Hosted CI has not completed for this commit yet.
|
Review update for |
Merged origin/main after the PR became dirty behind the active queue. The only conflicts were the CICD binary workflow and the worker-pool SSR permission test; the resolution keeps main's proxy smoke and memory checks while preserving the checkout-location-independent worker permission regression. Constraint: The PR branch was rejected while queued at an older head and later reported dirty against main.\nRejected: Force-rebase the contributor branch | a normal merge preserves branch history and avoids rewriting the remote PR head.\nConfidence: high\nScope-risk: moderate\nTested: npx --yes deno@2.7.7 fmt --check .github/workflows/cicd.yml src/security/sandbox/worker-pool.test.ts\nTested: npx --yes deno@2.7.7 lint src/security/sandbox/worker-pool.test.ts\nTested: npx --yes deno@2.7.7 check src/security/sandbox/worker-pool.test.ts\nTested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text npx --yes deno@2.7.7 test --preload=src/schemas/_test-setup.ts --no-check --allow-all --unstable-worker-options --unstable-net src/security/sandbox/worker-pool.test.ts\nTested: DENO_TESTING=1 npx --yes deno@2.7.7 test --no-check --allow-all src/transforms/mdx/esm-module-loader/module-writer.test.ts src/utils/bundle-manifest.test.ts src/utils/cache-dir.test.ts src/utils/cache-file-ops.test.ts src/utils/cache/eviction/eviction-manager.test.ts src/utils/cache/stores/memory/lru-cache-adapter.test.ts src/utils/cache/stores/memory/lru-list-manager.test.ts src/utils/file-discovery.test.ts src/utils/lru-wrapper.test.ts src/modules/react-loader/ssr-module-loader/loader.test.ts\nNot-tested: Full pre-push gate after merging current main.
|
Post-merge update for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/utils/cache-file-ops.ts:95
- The debug log includes
error.message, which for commonstaterrors can embed the full absolute path (for example, "EACCES: permission denied, stat '/abs/path'"). This defeats the path redaction intent ofpath: path.slice(-80)and can leak local filesystem details into logs. Consider sanitizing the message (and/or loggingcode/nameinstead) before emitting it.
logger.debug(`[${label}] Cache file existence check failed`, {
path: path.slice(-80),
error: error instanceof Error ? error.message : String(error),
});
.github/workflows/cicd.yml:65
- This job now runs
node ./tests/node/run-tests.mjs ..., but it does not set up a Node version. The harness uses Node test-runner flags like--importand--test, which require a sufficiently new Node runtime and may not be present (or consistent) across runners. Pin Node withactions/setup-nodebefore running the Node test step to keep CI deterministic.
- name: Run Node cache-link compatibility tests
run: node ./tests/node/run-tests.mjs 'src/utils/cache-dir.test.ts'
The latest review found that cache stat error messages could still carry full absolute paths and that the Node compatibility test relied on the runner default Node version. Redacting the known cache path in emitted error text and pinning Node for that job closes both without changing cache behavior. Constraint: Keep #3329 scoped to cache/discovery correctness and review-comment fixes. Confidence: high Scope-risk: narrow Tested: deno test --no-check --allow-all --unstable-worker-options src/utils/cache-file-ops.test.ts src/utils/cache-dir.test.ts src/security/sandbox/worker-pool.test.ts Tested: deno fmt --check .github/workflows/cicd.yml src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts Tested: deno lint src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts Tested: deno check src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts Tested: git diff --check
|
Addressed the latest suppressed review feedback at exact head What changed:
Local verification:
No merge-confidence declaration yet: hosted checks have restarted for this new head. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/cache-file-ops.ts:80
- JSDoc line break splits “are rethrown” across two lines, which reads like a typo and makes the sentence harder to parse. Combine the phrase onto one line.
});
return false;
}
return true;
The cache write path already treats missing parent directories as a recoverable cleanup race, but its post-write stat verification still converted every stat failure into a false return. That hid operational filesystem failures after a successful write. Keep the race behavior only for real missing-path errors and rethrow other stat failures so callers see the filesystem problem. Constraint: PR #3329 review requested that post-write stat EACCES/EIO failures not be reported as recoverable cache races.\nRejected: Return false for every stat failure | masks operational filesystem errors and can trigger repeat rewrites.\nConfidence: high\nScope-risk: narrow\nDirective: Keep cache-miss returns limited to structured absence/race errors.\nTested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options src/utils/cache-file-ops.test.ts src/utils/cache-dir.test.ts src/security/sandbox/worker-pool.test.ts\nTested: npx --yes deno@2.7.7 fmt --check src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts\nTested: npx --yes deno@2.7.7 lint src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts\nTested: npx --yes deno@2.7.7 check --allow-import src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts\nTested: git diff --check
The branch already contains the post-write cache stat propagation fix. This updates the shared lockfiles to the patched brace-expansion resolution so the branch can pass the merge-queue audit gate. Constraint: Default branch security audit currently flags brace-expansion 5.0.8. Rejected: Leave the audit patch to a later PR | merge-queue audit can evaluate this branch before the audit-only PR lands. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/utils/cache-file-ops.test.ts Tested: npx --yes deno@2.7.7 task audit Tested: npx --yes deno@2.7.7 task build:proxy-lock && git diff --exit-code -- scripts/build/proxy-deno.lock Tested: npx --yes deno@2.7.7 fmt --check deno.lock extensions/ext-sandbox-shell-tools/deno.json scripts/build/npm-package-metadata.test.ts scripts/build/proxy-deno.lock src/utils/cache-file-ops.ts src/utils/cache-file-ops.test.ts && git diff --check
|
Addressed and verified the remaining cache-file review thread on current head The branch already contains Local exact-head verification:
I resolved the thread. I am not queueing this PR yet because hosted checks are still running on this exact head. |
The dependency audit branch had already moved the sandbox shell dependency to brace-expansion 5.0.9, but the lock entry kept the 5.0.8 tarball checksum. GitHub CI failed while caching npm packages before the audit could complete. Constraint: The package version must stay on the patch release line. Rejected: Revert to brace-expansion 5.0.8 | restores the audited vulnerable package. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 task audit Not-tested: Full unit suite for this one-line lockfile integrity correction
|
Addressed the cache-file post-write stat review on the latest head and fixed the follow-up audit regression on the branch. Changes now on
Verification:
|
|
Merge confidence for head Reasoning: the review thread about Local verification passed:
Not scheduling for merge yet because required hosted checks are still queued or in progress on this head. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/cache-file-ops.ts:105
- The new operational-error log in
verifyCacheFileExistsincludespath: path.slice(-80), which can still leak sensitive absolute filesystem segments (for example user/workspace names). Since the error message is already redacted viadescribeCacheError, consider similarly limiting or redacting the logged path field so operational failures do not reintroduce path disclosure.
logger.debug(`[${label}] Cache file existence check failed`, {
path: path.slice(-80),
error: describeCacheError(error, path),
});
|
Merge confidence: 93% at head Reasoning:
I am scheduling this for merge with exact-head protection. |
|
Merge confidence: 93% at exact head Reasoning: The cache/discovery fixes have full hosted CI coverage including format, lint, typecheck, coverage shards/gate, integration, binary e2e, RSC browser e2e, npm install smoke, and dependency audit. No unresolved review threads are present, hosted required checks are green for this head, and the branch is CLEAN against the current base. Scheduling with |
|
Merge confidence: 93% for head b683181. Reasoning after #3328 landed: the head SHA is unchanged from the reviewed and fixed branch, all review threads remain resolved, and the PR-head check rollup has no pending or failing checks. The addressed reviewer concern now rethrows operational stat failures while preserving the not-found race behavior. Local verification already covered the touched cache file operations and audit path with Deno 2.7.7 fmt, lint, check, focused cache tests, npm package metadata check, task audit, and git diff --check. The remaining risk is narrow and will be covered by the merge-queue run against the new main. |
Summary
This PR fixes independent cache and filesystem correctness defects found during the module audit. The fixes are applied directly to the current mainline implementations and preserve the existing public APIs.
Discovery globs
*and?matcher.*.test.*or*.spec.*files as production definitions.Cache file errors
verifyCacheFileExistsreturnsfalseonly for classifiable not-found errors.FileSystemimplementations must use an error recognized byisNotFoundErrorwhen a path is absent.Cache-directory dependency link
Bundle manifest lifetime and ownership
In-memory LRU adapter
undefinedvalues remain distinguishable from missing keys.clear()completes even when an eviction observer throws.lastAccessedtimestamps.Upgrade note
Production eval, task, or trigger definitions must not use filenames containing
.test.or.spec.. Those names are now consistently excluded from discovery. Rename any production definition using those suffixes before upgrading.Verification
git diff --checkpasses.Scope