# Batch 6 — Validators (Semantic) — Findings

**Date:** 2026-03-26
**Files reviewed:** `validators/functionValidator.js`, `validators/macroValidator.js`, `validators/variableValidator.js`, `validators/typeValidator.js`, `validators/requireValidator.js`, `validators/includeValidator.js`
**Status:** Complete

---

## Summary

This batch is dominated by a **systemic `require('vscode')` violation**: four of the five files in
this batch import and use `vscode` directly, raising the total `require('vscode')` count in `core/`
to **six** (two previously found in Batch 1 and Batch 5; four new here). Every new validator
introduced for the extension has carried the violation with it. The architecture boundary is not
holding.

Beyond the architecture finding, `variableValidator.js` is the standout file in this batch — it
is the only one that correctly imports nothing from `vscode`, builds all ranges via `buildTokenRange`
from `parser.js`, and has the clearest separation of concerns. `requireValidator.js` has a Minor
correctness bug in the diagnostic Range position. `functionValidator.js` has a dead-code branch.
`macroValidator.js` has a mild output-line guard gap. `typeValidator.js` produces imprecise
full-line ranges for type diagnostics. No security issues found.

---

## Findings

### `functionValidator.js`

#### [CRITICAL] `require('vscode')` in core/ — line 6

**Issue:** `const vscode = require("vscode")` at line 6; used to construct `vscode.Range` at
lines 122, 149, 188, 285, and 298. This is the **third** independent violation of the
non-negotiable architecture rule (`parser.js` — Batch 1; `blockValidator.js` — Batch 5).

The pattern is the same as in all prior violations: the Range constructor is used only to build
diagnostic source locations, and could be replaced by plain `{ start, end }` position objects
that `createDiagnostic` already accepts (see `variableValidator.js` in this same batch, which
uses `buildTokenRange` from `parser.js` correctly).

**Recommendation:** Remove `vscode` import. Replace `new vscode.Range(l, s, l, e)` with
`{ start: { line: l, character: s }, end: { line: l, character: e } }` at every call site.
`createDiagnostic` and `vscode.TextDocument.lineAt` do not require `vscode.Range` objects —
they accept any object with compatible shape.

---

#### [MINOR] Dead code — redundant `if (builtInSignature)` guard — lines 394–407

**Issue:** At line 358, `if (!builtInSignature)` handles the non-built-in case and always
ends with an unconditional `continue` at line 390. Execution can only reach line 394 when
`builtInSignature` is truthy. The guard `if (builtInSignature)` at line 394 is therefore
always true — it can never be false at that point:

```javascript
if (!builtInSignature) {
    // ...
    continue;   // always reached — execution never falls through
}

// For functions with known signatures, validate argument shape.
if (builtInSignature) {    // ← always true; dead conditional
    validateFunctionArgumentShape(...);
    continue;
}

// Built-in case handled above.   ← misleading comment: built-in case is below, not above
```

The trailing comment "Built-in case handled above" (line 409) is also backward — the built-in
case is handled at line 394, which is below the non-built-in block, not above it.

**Recommendation:** Remove the `if (builtInSignature)` wrapper and convert the block to
unconditional code (since `builtInSignature` is guaranteed truthy here). Update the comment
to read "Built-in case — validate argument shape."

---

#### [OBSERVATION] `extractStandaloneCallName` — `ENDDO`/`ENDIF`/`ELSE` missing from exclusion list

**Issue:** The exclusion keyword list at line 51 omits `ENDDO`, `ENDIF`, `ENDMACRO`,
`ENDFUNCTION`, and `ELSE`. These cannot form a callable `IDENTIFIER(...)` pattern in practice
(they take no arguments), so the downstream regex `callMatch` would never match them anyway.
No diagnostic false positives result. Noted for completeness.

**Recommendation:** Accept as-is. No behavioural impact.

---

### `macroValidator.js`

#### [CRITICAL] `require('vscode')` in core/ — line 6

**Issue:** `const vscode = require("vscode")` at line 6; used to construct `vscode.Range` at
lines 55, 89, 105, 117, and 125. This is the **fourth** independent violation. All ranges are
source-location construction only — no VS Code API features are used beyond `Range`.

**Recommendation:** Same as `functionValidator.js` — remove import and replace with plain
position objects.

---

#### [MINOR] Output-line guard skipped when `executionClassificationMap` is not provided — line 76

**Issue:**
```javascript
const executionClassificationMap = options.executionClassificationMap || null;
// ...
if (executionClassificationMap && executionClassificationMap[lineIndex].isOutput) {
    continue;
}
```

When `executionClassificationMap` is not passed (defaults to `null`), the guard is inactive and
every line — including output lines — is scanned for macro invocations. `extractMacroInvocations`
only returns results for names present in `macroCatalog`, so false positives require the output
line to contain a token that matches a macro name. In documents with short macro names or
output text that resembles code, false positive macro invocation diagnostics can fire on
output lines.

**Recommendation:** Compute `executionClassificationMap` locally if not provided (same pattern
as `outputDelimiters` in `functionValidator.js` — fall back to computing from the document
rather than defaulting to null). Alternatively, document the caller requirement that this
option is always provided from `collectDiagnostics.js`.

---

#### [OBSERVATION] Case-mismatch check is limited to locally-declared macros — line 86

**Issue:** `macroCaseMap` is built from `buildMacroCaseMap(document)` — local declarations only.
Macros included from other files are not in the map. Since `extractMacroInvocations` also
receives the local `macroCatalog`, invocations of included macros are not scanned at all — the
case-mismatch check is consistently local-only. Not a bug within the current design, but a
known coverage gap: case mismatches in calls to included macros are silently undetected.

**Recommendation:** Accept as-is. Coverage could be extended when included macro metadata is
surfaced through the include resolver in a future enhancement.

---

#### [OBSERVATION] ACU-MAC-003 call passes `expectedCount: 0` — redundant but harmless — line 106

**Issue:**
```javascript
{ messageId: "ACU-MAC-003", params: { macroName: invocation.name, expectedCount: 0, actualCount: invocation.argCount } }
```

The `ACU-MAC-003` message template (`"Macro '{macroName}' expects no parameters but was called
with {actualCount}."`) does not contain `{expectedCount}`. The param is passed but unused by
`renderMessage`. This is consistent with the Batch 5 finding that `paramKeys` for this entry
incorrectly lists `expectedCount` — the fix there (removing from `paramKeys`) is the right
location; no change needed in this validator.

**Recommendation:** No action here; the Batch 5 backlog item (`diagnosticCatalog.js:152`)
covers the corresponding fix.

---

### `variableValidator.js`

No architecture violations — this is the only validator in the batch that correctly avoids
`require('vscode')`. Ranges are constructed via `buildTokenRange` from `parser.js`, which
returns plain `{ start: { line, character }, end: { line, character } }` objects.

#### [MINOR] `FIELD_HANDLE_LIKE` fallthrough — handle-like variables not in registry produce ACU-VAR-001 — lines 265–276

**Issue:** When a variable matches the `FIELD_HANDLE_LIKE` pattern (`/^h[A-Za-z]{2}[A-Za-z]_/`)
but `getFieldEntry` returns `null` (the field is not in the registry), the code falls through
without a `continue`:

```javascript
if (FIELD_HANDLE_LIKE.test(referencedVariable)) {
    const fieldEntry = getFieldEntry(referencedVariable);
    if (fieldEntry) {
        // ACU-ARG-007: case mismatch
        ...
        continue;   // only taken when field IS in registry
    }
    // NO continue here — falls through to scope check
}

// Reaches here for handle-like variables not in registry:
if (!isVariableInScope(...)) {
    // Generates ACU-VAR-001: "undefined variable" — not the right diagnostic
    ...
}
```

A handle-like variable that is unknown to the registry (incomplete registry or genuinely bad
field reference) receives `ACU-VAR-001` ("Undefined variable") rather than `ACU-ARG-006`
("Unknown database field handle"), which is the more accurate and actionable diagnostic.

Note: the parallel `isDatabaseHandle` check at line 245 (for recognised database handle
variables) does correctly generate `ACU-ARG-006` via its own branch. Only the `FIELD_HANDLE_LIKE`
path is missing the fallthrough diagnostic.

**Recommendation:** After the `FIELD_HANDLE_LIKE.test` block, if no `fieldEntry` was found,
add an `ACU-ARG-006` diagnostic and `continue` rather than falling through.

---

#### [OBSERVATION] `initializedVariables` is linear — no branch analysis — line 180

**Issue:** `initializedVariables` is populated in document order. A variable assigned inside
an `IF` branch is considered initialised on all subsequent lines, even if the `IF` was not
taken at runtime. This is an inherent limitation of single-pass static analysis (correct
behaviour for an unset-variable diagnostic at this complexity level).

**Recommendation:** Accept as-is. A flow-sensitive analysis would require building a
control-flow graph, which is well beyond scope for this validator.

---

#### [OBSERVATION] `isPseudoConstant` — `variableName.length < 2` guard is redundant — line 114

**Issue:**
```javascript
if (!variableName || variableName.length < 2) return false;
if (!variableName.toUpperCase().includes("CONST")) return false;
```

`"CONST"` is 5 characters; any string that includes it must be at least 5 characters long.
The `length < 2` check is a micro-optimisation that is already subsumed by the `includes`
check. Not incorrect, but adds noise.

**Recommendation:** Accept as-is. No impact.

---

### `typeValidator.js`

#### [CRITICAL] `require('vscode')` in core/ — line 6

**Issue:** `const vscode = require("vscode")` at line 6; used to construct `vscode.Range` at
line 44 (`new vscode.Range(lineIndex, 0, lineIndex, lineText.length)`). This is the **fifth**
independent violation.

**Recommendation:** Same pattern — remove import; replace with plain position object.

---

#### [MINOR] Type mismatch diagnostic covers entire line rather than the assignment target — line 44

**Issue:**
```javascript
const diag = createDiagnostic(
    new vscode.Range(lineIndex, 0, lineIndex, lineText.length),
    ...
```

The `ACU-TYPE-002` diagnostic highlights the entire line. Every other value-level diagnostic
in the codebase targets the specific token at fault (the assignment target, the variable
reference, the function name). A full-line highlight forces the user to read the entire line
to identify which part has the type mismatch.

**Recommendation:** Use `buildTokenRange(lineText, lineIndex, assignment.target, 0)` to
highlight the assignment target (consistent with how `variableValidator.js` highlights
assignment targets).

---

#### [OBSERVATION] `inferExpressionType` returns null for all compound expressions — line 103

**Issue:** Any expression involving operators, concatenation, or multiple operands returns
`null` from `inferExpressionType`. The type validator therefore only fires on:
- direct literal assignments (`SET x = 42`)
- single-function-call assignments (`SET x = FUNC(y)`)
- single-constant assignments (`SET x = TRUE`)
- single-variable-to-variable assignments (`SET x = y`)

In practice, most assignments in Architect code involve at least one operator or function
embedded in a larger expression. Coverage is narrow.

**Recommendation:** Accept as-is. Widening type inference risks false positives from incorrect
type resolution of sub-expressions. Document the known limitation in JSDoc.

---

#### [OBSERVATION] `userDefinedFunctionCatalog` defaults to `{}` — user-defined functions invisible to type inference

**Issue:**
```javascript
const userDefinedFunctionCatalog = options.userDefinedFunctionCatalog || {};
```

If pre-computed facts are not passed, user-defined function return types are unknown to the
type inferrer. `getResolvedFunctionReturnType` returns null for any user-defined function,
and no type mismatch is reported. This is a silent coverage gap when `validateTypes` is
called without the options object.

**Recommendation:** Accept as-is — `collectDiagnostics.js` passes pre-computed facts. The
fallback to `{}` is the backwards-compatibility pattern used across all validators in this
batch.

---

### `requireValidator.js`

#### [CRITICAL] `require('vscode')` in core/ — line 6

**Issue:** `const vscode = require("vscode")` at line 6; used to construct `vscode.Range` at
lines 48–51. This is the **sixth** independent violation in `core/`.

**Recommendation:** Remove import; replace with plain position object.

---

#### [MINOR] `lineText.indexOf("REQUIRE")` is case-sensitive — line 50

**Issue:**
```javascript
const range = new vscode.Range(
    lineIndex,
    lineText.indexOf("REQUIRE"),    // ← case-sensitive
    lineIndex,
    lineText.length
);
```

`parseRequire` (from `parser.js`) performs case-insensitive matching — it accepts `require`,
`REQUIRE`, `Require`, etc. If the keyword is lowercase or mixed-case, `indexOf("REQUIRE")`
returns `-1`. The Range is then `(lineIndex, -1, lineIndex, lineText.length)`. VS Code clamps
negative character positions to 0, so the diagnostic appears but highlights from column 0
rather than from the keyword position — technically incorrect.

**Recommendation:** Replace with `lineText.toUpperCase().indexOf("REQUIRE")` or extract the
match position from `parseRequire`'s return value.

---

#### [MINOR] `fs.existsSync` bypasses the shared `statMetadataCache` — line 47

**Issue:** The include resolver uses a `statMetadataCache` with a 200ms TTL to coalesce
repeated filesystem stats during a validation pass. `requireValidator.js` calls
`fs.existsSync(resolvedPath)` directly, bypassing that cache. If the same path is checked by
both the include resolver and the require validator in the same validation pass, the stat is
performed twice.

**Recommendation:** Replace `fs.existsSync(resolvedPath)` with `getFileMetadataAsync` (or
the sync stat helper from `cache.js`) to route through the shared cache. Alternatively,
accept as-is: REQUIRE files are typically few per document and the overhead is negligible.

---

#### [OBSERVATION] Paths outside workspace root are silently discarded — lines 42–44

**Issue:**
```javascript
if (workspaceRoot && !isPathWithinRoot(resolvedPath, workspaceRoot)) {
    continue;
}
```

A `REQUIRE` path that resolves outside the workspace root is silently skipped with no
diagnostic. The user gets no feedback that the path is unsafe or unresolvable within the
project. This is consistent with the Batch 1 path-traversal observation for INCLUDE, but
worth documenting: the validator silently drops rather than flagging workspace-boundary exits.

**Recommendation:** Accept as-is — generating a diagnostic for workspace-root exits is a
separate design decision. Document the silent discard in a comment.

---

### `includeValidator.js`

No architecture violations. No `require('vscode')` — correct. This file uses `buildTokenRange`
from `parser.js` for all ranges (the reference pattern for this batch).

#### [OBSERVATION] Param key inconsistency — `ACU-INC-003` uses `filename` while ACU-INC-001/002 use `includeFilename` — line 54

**Issue:** All three diagnostic calls use the variable `includeFilename`, but the params object
for `ACU-INC-003` names the key `filename` while `ACU-INC-001` and `ACU-INC-002` name it
`includeFilename`:

```javascript
// ACU-INC-001
{ messageId: "ACU-INC-001", params: { includeFilename } }

// ACU-INC-002
{ messageId: "ACU-INC-002", params: { includeFilename, actualFilename } }

// ACU-INC-003
{ messageId: "ACU-INC-003", params: { filename: includeFilename } }   // ← different key
```

This is not a bug (the catalog template for `ACU-INC-003` presumably uses `{filename}` to
match), but it is a micro-inconsistency worth noting if the `paramKeys` lists are audited
for completeness — a developer maintaining the catalog would see `includeFilename` in two
entries and `filename` in the third.

**Recommendation:** Accept as-is (driven by the catalog definition). When `ACU-INC-003` is
next edited, consider whether aligning on `includeFilename` across all three is worth the
catalog change.

---

#### [OBSERVATION] No `options` parameter — cannot receive pre-computed facts

**Issue:** `validateIncludes` takes only `(document, getRuleSeverity)` — no `options` object.
The three resolver functions (`findCircularIncludeDirectives`, `findIncludeFilenameCaseMismatches`,
`findMissingIncludeDirectives`) each re-scan the document independently. Unlike most validators
in this batch, there is no pathway to receive pre-computed resolver results from
`collectDiagnostics.js` without a signature change.

**Recommendation:** Accept as-is. All three resolver functions are cached (include resolver
uses `documentFacts`-style caching) so repeated calls are cheap. If profiling ever shows
this to be a bottleneck, add an `options` parameter.

---

## Merge assessment — `requireValidator.js` → `includeValidator.js`

### Basis for considering a merge

Both files validate "references to external files in Architect source":
- `includeValidator.js` validates `INCLUDE`/`INCLUDEONCE` directives
- `requireValidator.js` validates `REQUIRE` directives

Both produce "file not found" diagnostics (ACU-INC-003 and ACU-REQ-001 respectively). Both
receive `(document, getRuleSeverity)` as their primary arguments.

### Differences

| Concern | `includeValidator.js` | `requireValidator.js` |
|---|---|---|
| Resolution mechanism | Full include-resolver stack (cache, case-insensitive walk, circular detection) | Direct `fs.existsSync` |
| Diagnostic rules | 3 (circular, case mismatch, not found) | 1 (not found) |
| `options` parameter | None | `workspaceRoot`, pre-computed facts |
| `require('vscode')` | None — clean | Present — Critical violation |
| Architecture | Correct | Violates core/ boundary |

### Recommendation: merge is feasible and beneficial

The two validators are thematically coherent (both address external file references) and
close enough in structure that a merge is clean. The primary benefit is not conceptual
alignment — it is that the merge creates a natural moment to fix the `requireValidator.js`
Critical without a standalone refactor.

**Proposed approach:**
1. Add `validateRequire` as a second exported function inside `includeValidator.js`, cleaned
   up (no `vscode`, `indexOf` casing fixed, stat cache routed through resolver).
2. Delete `requireValidator.js`.
3. Update `collectDiagnostics.js` to import `validateRequire` from `includeValidator` alongside
   `validateIncludes`.

The two functions should remain **separate exports** — not collapsed into a single function —
because their resolution mechanisms, option shapes, and diagnostic rules differ enough that
a forced merge into one call would add complexity without benefit.

**Neutral case:** If the `requireValidator.js` Critical is fixed independently in the fix
phase, the merge argument weakens to pure organisation preference. Either outcome is valid;
the merge is cleaner but not essential.

---

## Systemic note — six `require('vscode')` violations in `core/`

| Batch | File | Line | Usage |
|---|---|---|---|
| 1 | `parser.js` | 247 | `buildTokenRange` uses `new vscode.Range` |
| 5 | `blockValidator.js` | 6 | `parseBlockKeyword` uses `new vscode.Range` |
| 6 | `functionValidator.js` | 6 | Five `new vscode.Range` call sites |
| 6 | `macroValidator.js` | 6 | Five `new vscode.Range` call sites |
| 6 | `typeValidator.js` | 6 | One `new vscode.Range` call site |
| 6 | `requireValidator.js` | 6 | One `new vscode.Range` call site |

Every violation uses `vscode` only for `new vscode.Range(...)`. The fix pattern is uniform:
replace with a plain `{ start: { line, character }, end: { line, character } }` object.
`createDiagnostic` in `diagnosticFactory.js` accepts both shapes (it passes the range directly
to `new vscode.Diagnostic(range, ...)`, and VS Code's constructor is documented to accept
compatible plain objects in the extension host). The `vscodeTypes.js` shim already provides
a `Range` constructor stub that could be leveraged by `core/` if a shim is preferred over
plain objects.

`variableValidator.js` is the reference implementation — already correct.

---

## Positive observations

- `includeValidator.js` correctly imports nothing from `vscode` and uses `buildTokenRange` for
  all ranges — exemplary pattern, and the cleanest file in this batch.
- `variableValidator.js` correctly imports nothing from `vscode` — one of only two validators
  in this batch to respect the architecture boundary.
- `variableValidator.js:getParamModeVariables` correctly uppercases the function name before
  `BuiltInFunctions` lookup — case-insensitive built-in resolution consistent with the rest of
  the codebase.
- `variableValidator.js` exports internal helpers (`isVariableInScope`, `findCaseInsensitiveVariableInScope`,
  `isPseudoConstant`, etc.) for testing — good design for a file with non-trivial logic.
- `functionValidator.js:extractFunctionArgs` and `countArguments` both handle nested parentheses
  and string state correctly — parenthesis depth tracking is correct and the string toggle is
  consistent with other parsers in the codebase.
- `functionValidator.js:validateCodeSnippet` uses `stripQuotedStrings` before scanning for
  function calls — identifiers inside string literals are correctly masked, preventing
  false positives.
- `requireValidator.js` correctly guards against absolute paths before calling `path.resolve`
  (line 36) and checks workspace root containment (line 42) before the filesystem access.
- `macroValidator.js:validateMacros` skips the first occurrence of a duplicate macro
  (reports only the re-declaration, not the original) — correct behaviour for
  `ACU-MAC-001`.

---

## Actions for backlog

| Severity | File | Issue | Recommended action |
|---|---|---|---|
| Critical | `core/src/validators/functionValidator.js:6` | `require('vscode')` — 3rd violation | Remove import; replace `new vscode.Range(l,s,l,e)` with plain position object |
| Critical | `core/src/validators/macroValidator.js:6` | `require('vscode')` — 4th violation | Remove import; replace `new vscode.Range(l,s,l,e)` with plain position object |
| Critical | `core/src/validators/typeValidator.js:6` | `require('vscode')` — 5th violation | Remove import; replace `new vscode.Range(l,s,l,e)` with plain position object |
| Critical | `core/src/validators/requireValidator.js:6` | `require('vscode')` — 6th violation | Remove import; replace `new vscode.Range(l,s,l,e)` with plain position object |
| Minor | `core/src/validators/functionValidator.js:394` | Dead `if (builtInSignature)` guard — always true when reached | Remove conditional; make unconditional; fix misleading comment |
| Minor | `core/src/validators/macroValidator.js:30` | Output-line guard inactive when `executionClassificationMap` not provided | Compute locally as fallback, or document caller requirement |
| Minor | `core/src/validators/variableValidator.js:265` | `FIELD_HANDLE_LIKE` fallthrough — handle-like unknown fields produce ACU-VAR-001 instead of ACU-ARG-006 | Add ACU-ARG-006 diagnostic and `continue` when `getFieldEntry` returns null |
| Minor | `core/src/validators/typeValidator.js:44` | Full-line range for ACU-TYPE-002 — should target assignment target token | Use `buildTokenRange(lineText, lineIndex, assignment.target, 0)` |
| Minor | `core/src/validators/requireValidator.js:50` | Case-sensitive `indexOf("REQUIRE")` — returns -1 for lowercase keyword | Use `lineText.toUpperCase().indexOf("REQUIRE")` or use match offset from `parseRequire` |
| Minor | `core/src/validators/requireValidator.js:47` | `fs.existsSync` bypasses shared stat cache | Route through `cache.js` stat helper or accept as-is |
| Design | `core/src/validators/requireValidator.js` | Merge into `includeValidator.js` — thematically coherent; merge fixes the Critical and the two Minors simultaneously | Keep as two separate exports within one file; delete `requireValidator.js`; update `collectDiagnostics.js` import |
