# Round 2 Code Review — Findings

**Date:** 2026-03-28
**Scope:** All new and significantly modified files since `main` (21 commits on `devel5`)
**Reviewer:** AI-assisted structured review

---

## Severity summary

| Severity | Count |
|---|---|
| Critical | 0 |
| Major | 0 |
| Minor | 9 |
| Observation | 4 |

---

## Minor findings

---

### R2-M1 — `parser.js:312` — `"OtherwiseStatement"` not a `NodeType` constant

**File:** `core/src/ast/parser.js:312`

```js
push(stack, { type: "OtherwiseStatement", line: lineIndex }); // boundary marker
```

Every other node type uses `NodeType.*` from `nodes.js`. This inline string literal is not
registered in `NodeType`, making it invisible to any walker that dispatches on `NodeType.*`.
The existing walkers (`collectAssignmentNodes`, `collectIteratorNodes`) don't check for it and
happen to skip it silently, but a future walker might miss it or produce a misleading error.

**Fix:** Add `OtherwiseStatement: "OtherwiseStatement"` to `NodeType` in `nodes.js` and
reference it from `parser.js`.

---

### R2-M2 — `expressionNodes.js` — missing `"use strict"`

**File:** `core/src/ast/expressionNodes.js`

The file has no `"use strict"` directive. Every other file under `core/src/ast/` has it.
Missing strict mode allows silent global variable leaks on undeclared assignments.

**Fix:** Add `"use strict";` as the first line.

---

### R2-M3 — `discouragedTypeValidator.js` — missing `"use strict"`

**File:** `core/src/validators/discouragedTypeValidator.js`

Same as R2-M2 — no `"use strict"` directive. Every other validator file has it.

**Fix:** Add `"use strict";` as the first line.

---

### R2-M4 — `doIteratorValidator.js:58-60` — ACU-SYN-004 squiggle hardcoded at char 0–2

**File:** `core/src/validators/doIteratorValidator.js:58`

```js
const range = {
    start: { line: node.line, character: 0 },
    end:   { line: node.line, character: 2 }
};
```

In HASH-OFF mode `DO` starts at column 0 so this is correct. In HASH-ON mode the line is
`    #DO i = 1 TO 10` — the `D` of `DO` is typically at column 5 or later, not column 0.
The squiggle lands on leading whitespace/`#` rather than on the `DO` keyword.

The token is available from the AST (`iterator.variable` is known, but the DO token position
isn't stored on the node). The simplest fix: store the DO keyword's char offset when building
the iterator node in `parser.js` (add `keywordChar: meaningful[0].char` to the iterator
struct), then use it here.

Alternatively, walk the original token stream to find the keyword position. Minor cosmetic
impact — diagnostic fires on the right line regardless.

---

### R2-M5 — `optionValidator.js` — OICC/CICC order assumed fixed

**File:** `core/src/validators/optionValidator.js:57`

```js
const optionPattern = /^\s*#?\s*OPTION\s+OICC\s*=\s*["'](.?)["']\s*,\s*CICC\s*=\s*["'](.?)["']/i;
```

If the developer writes `OPTION CICC="@", OICC="^"` (CICC before OICC), the regex does not
match and both characters are silently unvalidated. The language spec should clarify ordering,
but defensive handling with two separate patterns or a more flexible parser would be safer.

**Fix (minimal):** Add a second pattern for the reversed order (`CICC ... OICC ...`) and check
either on a match miss. Or add a comment documenting the OICC-first requirement explicitly.

---

### R2-M6 — `semanticTokenClassifier.js:56,67` — `in` operator on plain objects

**File:** `core/src/analysis/semanticTokenClassifier.js:56,67`

```js
if (upper in BuiltInConstants) { ... }
if (upper in BuiltInFunctions) { ... }
```

Uses `in` instead of `Object.prototype.hasOwnProperty.call()`. This is the same pattern fixed
across `helpers.js` in Round 1 finding R1-3.2. In practice, no current constant or function
name shadows a prototype property, but the pattern should be consistent.

**Fix:** Replace with `Object.prototype.hasOwnProperty.call(BuiltInConstants, upper)` (and
similarly for `BuiltInFunctions`).

---

### R2-M7 — `diffFilter.js:24` — CRLF line endings corrupt file paths on Windows

**File:** `cli/diffFilter.js:24`

```js
const relPath = line.slice(6); // strip "+++ b/"
```

`git diff` output on Windows may use `\r\n` line endings when `core.autocrlf=true`.
Splitting on `"\n"` leaves a trailing `\r` on `relPath`, producing paths like
`src/foo.txt\r` that will never match real file paths. The resulting `changedLinesMap`
silently returns no matches for every file, making `--diff-only` a no-op on affected machines.

**Fix:** Trim each line before processing: `line.trimEnd()` or explicit `line.replace(/\r$/, "")`.

---

### R2-M8 — `indentationValidator.js` — ELSEIF not checked for indentation alignment

**File:** `core/src/validators/indentationValidator.js:94`

`ELSE` is validated against the enclosing `IF`'s indent column. `ELSEIF` is not — it goes
through the `default` case of the if/else chain, which does nothing. In practice:

```text
#IF x = 1
        #ELSEIF x = 2   ← misaligned ELSEIF fires no diagnostic
#ENDIF
```

**Fix:** Add an `ELSEIF` branch alongside `ELSE` in the mid-block keyword check. `ELSEIF`
should be treated identically to `ELSE` for indentation purposes (expected at the same column
as its parent `IF`).

---

### R2-M9 — `collectDiagnostics.js` — `validateOptionDirectives` and `validateVariableUsage` unwrapped

**File:** `core/src/collectDiagnostics.js:73`, `313`

```js
diagnostics.push(...validateOptionDirectives(document, getRuleSeverity));  // line 73 — unwrapped
...
diagnostics.push(...validateVariableUsage(...));                            // line 313 — unwrapped
```

All validators between lines 133–310 are wrapped in `try/catch` with structured logging (per
Round 1 finding 6.20). The `validateOptionDirectives` call (line 73) and `validateVariableUsage`
call (line 313) are not. An unhandled exception from either crashes the entire diagnostics run
and produces no diagnostics for the document.

**Fix:** Wrap both calls in the same try/catch pattern as the surrounding validators.

---

## Observations

---

### R2-O1 — `documentAst.js` — cache eviction is oldest-inserted, not true LRU

**File:** `core/src/ast/documentAst.js:50-52`

```js
astCache.delete(astCache.keys().next().value); // evict oldest-inserted entry
```

`Map` iteration is insertion-ordered; this evicts the entry inserted earliest, not the least-
recently-accessed one. Under heavy multi-file editing (many files open simultaneously), a
recently re-opened file could be evicted before a long-untouched one.

Acceptable given the 50-entry limit and the low cost of re-parsing. The same pattern exists in
`documentFacts.js` (same decision made in Round 1). No action required.

---

### R2-O2 — `expressionParser.js:187` — unrecognised token falls back to IDENTIFIER

**File:** `core/src/ast/expressionParser.js:187`

```js
return makeIdentifier(tok); // fallback for unrecognised tokens
```

Permissive design: an unrecognised token in prefix position (e.g. a stray keyword) becomes an
IDENTIFIER node rather than null. This prevents null-propagation errors through the tree but
means the type inferrer gets an IDENTIFIER with an unexpected name (returns null — unknown type).
No false-positive risk. Intentional and acceptable.

---

### R2-O3 — `expressionTypeInference.js:214` — unary minus passes through non-numeric type

**File:** `core/src/analysis/expressionTypeInference.js:214`

```js
if (node.op === "-") return inferExpressionType(node.operand, context);
```

For `- "hello"` this returns `STRING`, which is semantically incorrect. However, the only
realistic code paths that reach this are: (a) `-numericVar` (correct), (b) valid identifier
that happens to have a non-numeric type inferred (returns that type, which would not cause a
false-positive type mismatch since the assignment target would need to be STRING too for it
to match). Low practical impact. Could be tightened to `if (!isNumericType(result)) return null`.

---

### R2-O4 — `doIteratorValidator.js` — start expression not validated

**File:** `core/src/validators/doIteratorValidator.js`

`DO x = (missing start)` would produce `iterator.start = null`. The validator only checks
`iterator.to` and `iterator.by` — a missing start expression is not flagged. This is acceptable
since the parser would produce an empty-span `start: null` which other structural validators
would catch. No ACU-SYN-004 false negative here.

---
