# Batch 4 — Include Resolution — Findings

**Date:** 2026-03-26
**Files reviewed:** `includeResolver.js`, `includeResolver/cache.js`, `includeResolver/graph.js`, `includeResolver/traversal.js`
**Status:** Complete

---

## Summary

The include resolver is the most architecturally sophisticated module in the codebase, implementing
memoised symbol extraction, dependency-graph invalidation, case-insensitive path resolution, and
async cache warming with in-flight coalescing. The design is generally sound and the critical
correctness concerns (circular reference guarding, recursion depth, cache invalidation propagation)
are all handled correctly. The two notable findings are: a **third independent
`parseFunctionDeclaration` variant** that deepens the already-flagged parser fragmentation, and an
**inconsistency between the sync and async path-resolution functions** — the sync path does a full
case-insensitive directory walk while the async path does a simple stat, making them behave
differently on case-sensitive filesystems. No critical or architecture violations.

---

## Findings

### `includeResolver.js`

No findings. The file is a clean barrel facade — a single-responsibility re-export layer with no
logic of its own. It correctly groups the public API surface of both `graph.js` and `traversal.js`
into one import point.

---

### `graph.js`

#### [MAJOR] Third independent `parseFunctionDeclaration` variant — lines 29–41

**Issue:** `graph.js` defines `parseFunctionDeclarationName` (line 29), which parses FUNCTION
declaration lines to extract the function name only. This is the **third** independent
implementation for the same construct:

- `parser.js` (line 260): captures name + parameter list; no return type
- `functionAnalysis.js` (line 15): captures name + parameter list + return type (already flagged Major in Batch 2)
- `graph.js` (line 29): captures name only

The `graph.js` regex is `/^\s*#?\s*function\s+([a-zA-Z_][a-zA-Z0-9_]*)\s*\(/i`. The `#?` prefix
handling (to match `#FUNCTION` in HASH-ON sections) is present here but may be absent in the other
two variants — a functional difference not captured in the Batch 2 finding. Three parsers for the
same syntax guarantee divergence as the language evolves.

**Recommendation:** Consolidate all three into one canonical implementation as planned in Batch 2.
The `graph.js` version's `#?` prefix support should be incorporated into the canonical form.

---

#### [MAJOR] Sync/async path resolution inconsistency — `resolveIncludePath` vs `resolveIncludePathAsync` — lines 119–148

**Issue:** The two path resolvers behave differently:

- **`resolveIncludePath`** (sync, line 119): delegates to `resolvePathCaseInsensitive`, which
  performs a full directory-by-directory walk (`fs.readdirSync`) to find the actual file regardless
  of how the include filename is cased. Produces the correctly-cased actual path.

- **`resolveIncludePathAsync`** (async, line 138): calls `path.resolve` with the filename exactly
  as written, then calls `getFileMetadataAsync(resolved)`. If the filesystem is case-sensitive
  (Linux), a file named `MyLib.acurity` included as `mylib.acurity` will return `null` from the
  async path but be found correctly by the sync path.

The async variant is used in `warmIncludeCacheForDocument` and `resolveSymbolsFromIncludeFileAsync`.
The sync variant is used in all `buildIncluded*InScope` functions and `collectIncludeDependenciesForDocument`.
This means warm-cache behaviour and async resolution silently fail on case mismatches that the
main resolution path handles correctly.

In practice the tool targets Windows (case-insensitive filesystem) so this does not bite
users today. But any CI/CD pipeline running on Linux would see the divergence.

**Recommendation:** Replace the body of `resolveIncludePathAsync` to use an async variant of the
directory-walk logic (or call `resolvePathCaseInsensitive` synchronously since the resolved path
cache makes repeat calls cheap), so both paths behave identically.

---

#### [MINOR] `isFunctionInScope` — unused `document` parameter — line 171

**Issue:**
```javascript
function isFunctionInScope(document, functionName, lineIndex, userDefinedFunctions) {
```
The `document` parameter is declared but never referenced in the function body. The logic only
uses `functionName`, `lineIndex`, and `userDefinedFunctions`. Callers must pass a document
object for no reason, and the function signature implies the document is consulted when it is not.

**Recommendation:** Remove the `document` parameter. Update all call sites to drop the argument.

---

#### [MINOR] `findCircularIncludeDirectives` — INCLUDEONCE not propagated into child-file check — line 311

**Issue:** `findCircularIncludeDirectives` skips INCLUDEONCE lines in the *root document*
(line 311: `if (isIncludeOnce(lineText)) continue`). However, the recursive helper
`hasCircularIncludeFromFile` reads child files and checks all their include lines regardless of
whether they are INCLUDEONCE or regular INCLUDE. This produces false-positive circular detections
for patterns like:

```
// root.acurity
INCLUDE b.acurity

// b.acurity
INCLUDEONCE root.acurity   ← prevents runtime re-inclusion
```

`hasCircularIncludeFromFile` will report the `INCLUDE b.acurity` in root as circular because it
traces `root → b → root` without respecting that `b`'s include of `root` is INCLUDEONCE (which
prevents actual re-expansion at runtime).

**Recommendation:** Pass an `isIncludeOnce(lineText)` flag into the recursive helper and treat
INCLUDEONCE includes as non-circular path members. Alternatively, document the known limitation
in a comment near `hasCircularIncludeFromFile`.

---

### `cache.js`

#### [OBSERVATION] `includeSymbolCache` has no size cap

**Issue:** `documentFacts.js` limits its cache to 50 entries with LRU eviction. The
`includeSymbolCache` in `cache.js` has no corresponding size limit — it grows for every unique
include file encountered. In a large project with many include files this could accumulate
significant memory. Unlike the document cache, the include cache is not per-document-version —
entries remain until explicitly invalidated.

**Recommendation:** Accept for now. Include files are typically stable and finite (on the order
of tens to low hundreds in typical projects). If memory becomes a concern, apply a size-bounded
LRU policy similar to `documentFacts.js`.

---

#### [OBSERVATION] `statMetadataCache` is unbounded in entries

**Issue:** The TTL-based stat cache (200ms expiry, lazy eviction) has no maximum size. Entries
for paths that are never re-accessed after their TTL expires stay in the map as dead entries until
GC collects the map itself. In a tight burst-validation window touching many unique paths, the
map could contain hundreds of expired entries that were never cleaned up.

**Recommendation:** Accept as-is. The 200ms TTL means entries are already expired by the time
the next GC cycle runs. At 200ms the cache is intended to be ephemeral; the dead-entry
accumulation is bounded by a single burst window, not the session lifetime.

---

### `traversal.js`

#### [MINOR] Three single-purpose scope builders preserved without enforcement to prefer the unified builder

**Issue:** `buildIncludedSymbolsInScope` (line 439) is documented as the preferred
single-pass builder: "each include line is resolved once rather than three times." The three
individual builders — `buildIncludedFunctionsInScope`, `buildIncludedVariablesInScope`,
`buildIncludedFunctionCasesInScope` — each independently scan all document lines and call
`resolveIncludePath` + `resolveSymbolsFromIncludeFile` per include directive. A caller that
uses all three (or two of three) triples the filesystem and cache access compared to the
unified builder. The preference is documented in a comment but not enforced.

**Recommendation:** Add a `@deprecated` JSDoc to the three individual builders pointing callers
to `buildIncludedSymbolsInScope`. If any callsites use more than one of the individual builders,
migrate them to the unified version.

---

#### [OBSERVATION] `resolveSymbolsFromIncludeFile` and `resolveSymbolsFromIncludeFileAsync` are parallel implementations

**Issue:** The two functions share near-identical logic: recursion depth guard, circular
reference guard, file read, line scan (include, function, variable), cache write, telemetry.
The async version additionally has the in-flight coalescing block (lines 214–222). Any change
to the parsing or caching logic must be applied to both functions. The code duplication is
approximately 120 lines.

**Recommendation:** Accept as-is. The sync and async versions have subtly different control
flow that makes extraction into a shared helper complex (the async IIFE wrapping the
`activeStack` add/delete sequence is the main complication). Document the parallel nature with
a comment on each function.

---

#### [OBSERVATION] `invalidateIncludeCacheForPath` iterates the cache during invalidation — potential miss

**Issue:** The invalidation BFS (lines 380–401) pops a path, deletes it from the cache, then
iterates `includeSymbolCache.entries()` to find other entries whose `dependencies` set contains
the deleted path. Since the deleted entry is removed before the inner loop, it won't appear in
the subsequent `entries()` iteration — this is correct. However, if the cache is mutated by a
concurrent async operation while the BFS is running (unlikely in extension's event loop, but
possible in the test layer), the entries iteration could miss a newly inserted entry that
depends on the deleted path.

**Recommendation:** Accept as-is. The extension runs on Node.js's single-threaded event loop;
there is no true concurrency during a synchronous BFS. The scenario only arises in worker
threads or test environments, neither of which is current usage.

---

## Positive observations

- `includeResolver.js` is an exemplary facade — zero logic, one import point for the entire
  public API surface.
- `resolvePathCaseInsensitive` correctly handles Windows path casing by doing a directory-by-directory
  walk and matching exact-case first, then case-insensitive. The cache key (`path.normalize().toLowerCase()`)
  ensures the resolved result is reused for any casing of the same path.
- `isPathWithinRoot` correctly handles both the case where `resolvedPath === allowedRoot` and
  where it is a descendant, using a trailing separator to prevent false matches on path prefixes
  (e.g. `/foo/bar` would not match root `/foo/b`).
- `resolveSymbolsFromIncludeFile` guards both circular references and maximum recursion depth
  independently — belt-and-suspenders approach to a potentially unbounded recursive walk.
- `invalidateIncludeCacheForPath` propagates invalidation upward through the dependency graph —
  not just the directly-changed file but all files that transitively included it. This prevents
  stale symbols from appearing in documents that include the changed file.
- The async in-flight coalescing in `resolveSymbolsFromIncludeFileAsync` (lines 214–222) is a
  good optimisation: concurrent warm requests for the same file reuse a single shared promise
  rather than duplicating file I/O.
- `cloneSymbols` and `clonePathSet` correctly shallow-clone the mutable collections before
  returning from the cache, preventing callers from mutating cached state.
- First-include-wins deduplication in `resolveSymbolsFromIncludeFile` (lines 111–118) is
  consistent with the same pattern used in `buildIncludedSymbolsInScope`.

---

## Actions for backlog

| Severity | File | Issue | Recommended action |
|---|---|---|---|
| Major | `core/src/includeResolver/graph.js:29` | Third independent `parseFunctionDeclarationName` — adds `#?` prefix handling absent from other two | Incorporate into canonical implementation from Batch 2 finding |
| Major | `core/src/includeResolver/graph.js:138` | `resolveIncludePathAsync` does not do case-insensitive directory walk — diverges from sync path | Replace body to use same case-insensitive walk or call sync resolver |
| Minor | `core/src/includeResolver/graph.js:171` | `document` parameter unused in `isFunctionInScope` | Remove parameter and update call sites |
| Minor | `core/src/includeResolver/graph.js:311` | `findCircularIncludeDirectives` — INCLUDEONCE not respected in child files — false positive circular detections | Pass INCLUDEONCE flag into helper or document limitation |
| Minor | `core/src/includeResolver/traversal.js:494` | Three single-purpose scope builders have no deprecation marker; callers may use them individually when unified builder is preferred | Add `@deprecated` JSDoc to three individual builders |
