# Batch 5 — Diagnostics Infrastructure — Findings

**Date:** 2026-03-26
**Files reviewed:** `diagnostics/diagnosticCatalog.js`, `diagnostics/quickFixCatalog.js`, `diagnostics/diagnosticFactory.js`, `collectDiagnostics.js`, `blockValidator.js`, `suppressionParser.js`
**Status:** Complete

---

## Summary

The diagnostics infrastructure is well-architected: the catalog-driven approach (stable IDs,
parameterised messages, surface filtering, suppressibility) is exactly the right design for a
language server that must support CI/CD and IDE surfaces with different severity profiles. Most
files in this batch are clean. The two significant findings are a **second `require('vscode')` in
core/** (in `blockValidator.js` — same architecture violation as Batch 1's `parser.js` critical)
and a **silent gap in the `legacy-compatible` severity profile** that leaves five macro-related
rules with undefined severity. `collectDiagnostics.js` has an asymmetric try/catch pattern and a
dead `path` import. `suppressionParser.js` has a minor blind spot for `@rule-ignore` tokens
embedded in string literals. `quickFixCatalog.js` and `diagnosticFactory.js` are clean.

---

## Findings

### `blockValidator.js`

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

**Issue:** `blockValidator.js` calls `require("vscode")` at line 6 and uses `new vscode.Range(...)`
at line 129 inside `parseBlockKeyword`. The file lives in `core/src/` — the same architecture
boundary that was flagged as a Critical violation in Batch 1 (`parser.js:247`). This is a second
independent violation of the non-negotiable rule.

`validateBlockStructure` is called directly from `collectDiagnostics.js` (line 120), which is
consumed by both the extension and the CLI. Any headless or test run that loads `collectDiagnostics`
loads `blockValidator`, which immediately tries to resolve the `vscode` module. If the CLI's
module intercept is active this is silently papered over; if it is not (e.g. in a unit test that
only loads `core/`), the require fails.

**Recommendation:** Remove `vscode` from `blockValidator.js`. `parseBlockKeyword` should return
plain `{ start, end }` column indices rather than a `vscode.Range`. The caller
(`validateBlockStructure`) already receives a `document` — let callers or the extension layer
construct `Range` objects from the returned indices. This is the same fix pattern recommended
for `parser.js:buildTokenRange` in Batch 1.

---

#### [MINOR] Comment stripping does not handle `//` inside string literals — line 108

**Issue:**
```javascript
const commentIndex = lineText.indexOf("//");
const effectiveText = commentIndex >= 0 ? lineText.substring(0, commentIndex) : lineText;
```
A line such as `SET url = "http://example.com"` would be truncated at the `//` inside the
string, making the effective text `SET url = "http:`. Block keywords in the real part of such a
line would then be scanned on a truncated string. In practice, Architect source lines are unlikely
to contain URL strings, but the parser inconsistency is worth noting.

**Recommendation:** Use `stripTrailingComment` from `parser.js` (which handles quoted strings)
rather than a bare `indexOf("//")`.

---

### `diagnosticCatalog.js`

#### [MAJOR] `legacy-compatible` severity profile is missing all five macro rule entries

**Issue:** The `default` and `strict-modern` profiles define severity for all macro rules:
`macroRedeclared`, `macroCaseMismatch`, `macroUnexpectedParameters`, `macroMissingParameters`,
`macroParameterCountMismatch`. The `legacy-compatible` profile (lines 573–627) omits all five.

When `getProfileRuleSeverities("legacy-compatible")` is used to build `getRuleSeverity`, any
lookup for a macro rule ID returns `undefined`. The behavior depends on how the caller handles
`undefined`: if it treats it as `null` (rule disabled), macro diagnostics are silently suppressed
for legacy-compatible users; if it falls through to a default severity, the severity may differ
unexpectedly from intent. Either way, the profile is silently incomplete and its behavior is
unspecified for these rules.

**Recommendation:** Add the five missing macro rules to `legacy-compatible`. Reasonable defaults
(matching the legacy philosophy of `"warning"` for most rules):
```javascript
macroRedeclared: "error",
macroCaseMismatch: "warning",
macroUnexpectedParameters: "error",
macroMissingParameters: "error",
macroParameterCountMismatch: "error",
```

---

#### [MINOR] `ACU-MAC-003` — `paramKeys` includes `expectedCount` which is absent from the message template — line 152

**Issue:**
```javascript
"ACU-MAC-003": {
    message: "Macro '{macroName}' expects no parameters but was called with {actualCount}.",
    paramKeys: ["macroName", "expectedCount", "actualCount"],   // ← expectedCount not in template
```
The message for `macroUnexpectedParameters` says "expects no parameters" — the expected count is
always zero, so there is no `{expectedCount}` placeholder in the template. The `paramKeys` array
documents what params the template uses; having `expectedCount` there is misleading. Any tooling
that validates required params against `paramKeys` would expect callers to provide `expectedCount`
unnecessarily.

**Recommendation:** Remove `"expectedCount"` from the `paramKeys` array for `ACU-MAC-003`.

---

#### [OBSERVATION] `ACU-BLK` section is missing its description comment line — line 185

**Issue:** Every other domain section has a two-line header:
```
// ============================================================================
// ACU-INT: Interpolation validation (^variable^ syntax in output lines)
```
The `ACU-BLK` section has only the separator line with no description, making the block section
less discoverable in the file.

**Recommendation:** Add `// ACU-BLK: Block structure validation (IF/ENDIF, DO/ENDDO, etc.)` after
the separator on its own cosmetic-only edit.

---

#### [OBSERVATION] `isSuppressable` feature is designed but no catalog entry uses it

**Issue:** `isSuppressable(messageId)` returns `entry.suppressable !== false` — i.e., all
diagnostics are suppressable unless an entry explicitly sets `suppressable: false`. The design is
documented in the JSDoc (line 738–744) but no current catalog entry sets `suppressable: false`.
The mechanism exists in anticipation of marking block-structure errors as unsuppressable.

**Recommendation:** Accept as-is. When a future decision is made to protect block-structure
diagnostics from suppression, the infrastructure is ready.

---

#### [OBSERVATION] 2-space indentation inconsistent with 4-space codebase convention

**Issue:** `diagnosticCatalog.js` uses 2-space indentation throughout, while all other reviewed
files use 4-space indentation.

**Recommendation:** Cosmetic only. Normalise on the next substantive edit.

---

### `collectDiagnostics.js`

#### [MINOR] `path` module imported but never used — line 17

**Issue:** `const path = require("path")` at line 17 is never referenced anywhere in the file.

**Recommendation:** Remove the unused import.

---

#### [MINOR] `getFunctionScopeAtLine` called twice per variable declaration per line — lines 80, 91

**Issue:** For each variable declared on a line, the loop calls `getFunctionScopeAtLine(functionScopes, lineIndex)` twice: once to determine `isFunctionScoped` (line 80) and once to decide whether to add it to `globalVariables` (line 91). If `getFunctionScopeAtLine` is a linear scan through `functionScopes`, this doubles the work per variable.

**Recommendation:** Call once before the `declaration.variables.forEach` block, store the result in a local variable, and reuse it for both checks.

---

#### [MINOR] Asymmetric try/catch — first six validators unprotected — lines 119–143 vs 145–241

**Issue:** `validateBlockStructure`, `validateKeywordCapitalization`, `validateTypeKeywordCapitalization`, `validateTypes`, `validateNamingConventions`, `validateStringInterpolation`, and `validateVariableUsage` are called without try/catch. Any uncaught exception from these validators propagates up and causes `collectDiagnostics` to return nothing (the caller gets an exception rather than a partial result). By contrast, the function, macro, require, usage-pattern, robodoc, argument-string, and todo validators are all individually wrapped in try/catch with error logging.

The first group were presumably judged stable enough not to need guarding. However, if a regression in `validateBlockStructure` ever throws (e.g. from the `require('vscode')` violation above), the entire diagnostics pass silently fails with no user-facing message.

**Recommendation:** Wrap the unguarded validators in try/catch with the same error-logging pattern as the later ones, or document the intentional asymmetry in a comment.

---

#### [OBSERVATION] Suppression filtering runs after all validators complete — line 248

**Issue:** `buildSuppressionMap` and the suppression filter execute after every validator has run.
Suppressed diagnostics are produced in full before being discarded. For documents with many
`@rule-ignore` annotations this is wasteful — a validator could skip suppressed lines if it knew
the suppression map upfront.

**Recommendation:** Accept as-is. Suppression is rare and building the map per-document is cheap.
The current architecture is simpler than threading the suppression map into every validator.

---

### `suppressionParser.js`

#### [MINOR] `RULE_IGNORE_RE` matches inside string literals — line 32

**Issue:** `RULE_IGNORE_RE.exec(lineText)` is run against the raw line text. A line containing
a string literal that happens to include `@rule-ignore ACU-VAR-001` would be treated as a
suppression directive:

```
SET msg = "@rule-ignore ACU-VAR-001 this is a runtime string"
```

This would suppress `ACU-VAR-001` on the following line even though the intent is a string value.
The scenario is contrived in Architect source, but the parser is technically incorrect.

**Recommendation:** Strip string literals before applying `RULE_IGNORE_RE`, using
`stripQuotedStrings` from `parser.js`. Alternatively, require the `@rule-ignore` to appear
within a `//` comment: check that `//` precedes the `@rule-ignore` token before extracting IDs.

---

### `diagnosticFactory.js`

#### [MINOR] `renderMessage` constructs regex from unescaped param key — line 52

**Issue:**
```javascript
result = result.replace(new RegExp(`\\{${key}\\}`, "g"), String(value));
```
The `key` is embedded directly into the regex pattern without escaping regex metacharacters. If
a key ever contained a metacharacter (e.g. a dot), the regex would match unintended patterns.
In practice, all param keys are camelCase identifiers (`variableName`, `lineNumber`, etc.)
and pose no real risk. But the latent incorrectness is worth noting if params are ever derived
from external input.

**Recommendation:** Replace with `key.replace(/[.*+?^${}()|[\]\\]/g, '\\\\$&')` before
interpolating into the RegExp, or use a simpler string-replace approach:
`template.split(\`{${key}}\`).join(String(value))` (no regex needed for exact-string substitution).

---

### `quickFixCatalog.js`

No findings. `Object.freeze` on the catalog is a correct defensive practice. Accessor functions
are all thin wrappers with safe null-coalescing. The catalog entries correctly reference existing
diagnostic IDs.

---

## Positive observations

- The `ACU-XXX-NNN` stable ID scheme with `paramKeys`, `surfaces`, and `category` fields is
  excellent design — enables CI/CD surface filtering, future localization, and per-diagnostic
  suppression without touching validator code.
- `DEFAULT_RULE_SEVERITY_PROFILES` co-locating severity defaults with message definitions is
  a clean architectural choice — one file to review when tuning rule behavior.
- `suppressionParser.js` correctly normalises IDs to uppercase at both store (line 41) and lookup
  (line 71), making suppression case-insensitive end-to-end.
- `suppressionParser.js` guards against a suppression comment on the last line of the file
  (line 48–50: `if (targetLine >= document.lineCount) continue`) — prevents an off-by-one
  that would produce an out-of-bounds Map entry.
- `collectDiagnostics.js` uses `"use strict"` — the only file in the codebase reviewed so far
  that declares strict mode explicitly. Positive.
- `diagnosticFactory.js:createDiagnostic` validates config shape at entry (line 10) and throws
  a descriptive error for unknown `messageId` — fails fast rather than silently producing
  a malformed diagnostic.
- `quickFixCatalog.js` is `Object.freeze`'d — prevents callers from accidentally mutating catalog
  entries.

---

## Actions for backlog

| Severity | File | Issue | Recommended action |
|---|---|---|---|
| Critical | `core/src/blockValidator.js:6` | `require('vscode')` in core/ (second violation) | Remove; return plain column indices from `parseBlockKeyword`; let extension layer build Range |
| Major | `core/src/diagnostics/diagnosticCatalog.js` | `legacy-compatible` profile missing 5 macro rule severity entries | Add `macroRedeclared`, `macroCaseMismatch`, `macroUnexpectedParameters`, `macroMissingParameters`, `macroParameterCountMismatch` |
| Minor | `core/src/diagnostics/diagnosticCatalog.js:152` | `ACU-MAC-003` paramKeys includes `expectedCount` not in template | Remove `expectedCount` from paramKeys |
| Minor | `core/src/collectDiagnostics.js:17` | `path` import unused | Remove import |
| Minor | `core/src/collectDiagnostics.js:80` | `getFunctionScopeAtLine` called twice per variable per line | Hoist to single call before `forEach` |
| Minor | `core/src/collectDiagnostics.js:119` | First 6 validators not wrapped in try/catch — asymmetric error isolation | Wrap or document intentional asymmetry |
| Minor | `core/src/suppressionParser.js:32` | `@rule-ignore` regex matches inside string literals | Check that `//` precedes the token, or strip quoted strings first |
| Minor | `core/src/diagnostics/diagnosticFactory.js:52` | Regex built from unescaped param key | Escape key or use split/join approach |
