# Code Review Fix Plan

Companion to `TRACKER.md`. All items from the actions backlog, organised into
execution phases ordered by severity and logical dependency.

**Cross-session tracking:** Set Status to `Done` when the fix lands, and update
the corresponding row in `TRACKER.md` to `Done`. Update the progress summary table.

**Ordering rationale:**
1. Runtime crashes (users broken now)
2. Architectural violations — `require('vscode')` sweep (unblocks CLI shim removal)
3. Logic/correctness bugs
4. Consolidation / design
5. Minor fixes (grouped by area)
6. Test layer

---

## Phase 1 — Critical: Runtime crashes

These produce exceptions in the running extension today.

| # | Status | File | Issue | Fix |
|---|---|---|---|---|
| 1.1 | Done | `vscode-extension/src/diagnostics.js:91` | `path.normalize` called but `path` never imported → `ReferenceError` on any include-file change event | Add `const path = require("path")` to imports |

---

## Phase 2 — Critical: `require('vscode')` sweep in core/

**15 violations.** All follow the same pattern: replace `new vscode.Range(l, s, l, e)` with a
plain object `{ start: { line: l, character: s }, end: { line: l, character: e } }`.
Reference correct implementations: `variableValidator.js`, `includeValidator.js`.

Start with `parser.js:buildTokenRange` — it is called by nearly every validator, so fixing it
first cascades benefit.

After all 15 are done: remove the `Module._load` shim in `cli/index.js` and delete
`core/src/vscodeTypes.js` (item 2.16).

| # | Status | File | Location | Notes |
|---|---|---|---|---|
| 2.1 | Done | `core/src/parser.js` | `:247` `buildTokenRange` | Root fix — highest leverage |
| 2.2 | Done | `core/src/blockValidator.js` | `:6` | Return plain column indices; extension layer builds Range |
| 2.3 | Done | `core/src/validators/functionValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.4 | Done | `core/src/validators/macroValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.5 | Done | `core/src/validators/typeValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.6 | Done | `core/src/validators/requireValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.7 | Done | `core/src/validators/keywordCapsValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.8 | Done | `core/src/validators/typeKeywordCapsValidator.js` | `:7` | Replace `new vscode.Range` with plain object |
| 2.9 | Done | `core/src/validators/namingValidator.js` | `:6` | Replace `new vscode.Range` with plain object; `vscode.workspace.getConfiguration` replaced with parameter injection (`namingConfig` arg); config exposed via `.acurity.json` |
| 2.10 | Done | `core/src/validators/argumentStringValidator.js` | `:7` | Replace `new vscode.Range` with plain object |
| 2.11 | Done | `core/src/validators/interpolationValidator.js` | `:6` | Replace `new vscode.Range` with plain object |
| 2.12 | Done | `core/src/validators/usagePatternValidator.js` | `:11` | Replace `new vscode.Range` in `createCallRange` |
| 2.13 | Done | `core/src/validators/hashModeValidator.js` | `:10` | Replace `new vscode.Range` with plain object |
| 2.14 | Done | `core/src/validators/robodocValidator.js` | `:1` | Replace `new vscode.Range` in `buildLineRange` |
| 2.15 | Done | `core/src/validators/todoValidator.js` | `:11` | Replace `new vscode.Range` with plain object |
| 2.16 | Done | `cli/index.js` + `core/src/vscodeTypes.js` | — | Shim removed; `vscodeTypes.js` deleted; `robodocUtils.js` fixed (missed in Phase 2); robodocConfig injection added to complete the sweep |

---

## Phase 3 — Major: Correctness and logic bugs

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 3.1 | Done | 7 | `core/src/validators/typeKeywordCapsValidator.js:147` | `currentPos` not advanced for correctly-cased params → wrong column for subsequent errors in multi-param functions | Advance `currentPos` unconditionally after each param, not only on mismatch |
| 3.2 | Done | 9 | `vscode-extension/src/codeActionProvider.js:567` | `createIndexSaveRestoreFixes` inserts handle variable name as `INDEX_SAVE_POSITION` arg; table synonym string expected | Extract table synonym from `INDEX_OPEN_*` arg 0; insert SAVE before OPEN. Added ACU-PAT-002 fix (insert RESTORE after CLOSE). Syntax tests added. |
| 3.3 | Done | 1 | `core/src/vscodeTypes.js:66` | `withProgress` shim swallows callback result | Moot — 2.16 landed first; `vscodeTypes.js` deleted |
| 3.4 | Done | 5 | `core/src/diagnostics/diagnosticCatalog.js` | `legacy-compatible` profile missing 5 macro rule severity entries | Add missing `ACU-MAC-*` entries |
| 3.5 | Done | 1 | `core/src/constants.js:18` | `ACURITY_TYPES` list inconsistent with type tokens emitted by parser | Added `BYTE`/`LOGICAL` to `ACURITY_TYPES`; added `BOOLEAN`/`TIME`/`INTEGER` to parser alternate-format regex |
| 3.6 | Done | 1 | `core/src/constants.js:5` | `MACRO`/`ENDMACRO` missing from `ACURITY_KEYWORDS` | Add both keywords |

---

## Phase 4 — Major: Consolidation and design

| # | Status | Batch | Files | Issue | Fix |
|---|---|---|---|---|---|
| 4.1 | Done | 6 | `core/src/validators/requireValidator.js` → `includeValidator.js` | Merge: both validate external file references; merge fixes Critical + 2 Minors at once | Two separate exports in one file; delete `requireValidator.js`; update `collectDiagnostics.js` |
| 4.2 | Done | 2 + 4 | `functionAnalysis.js:15`, `graph.js:29`, `parser.js` | Three independent `parseFunctionDeclaration*` implementations that have diverged | Centralise in `parser.js`; have the other two call it. Note: `graph.js:29` adds `#?` prefix handling absent from others — preserve it in the canonical version |
| 4.3 | Done | 4 | `core/src/includeResolver/graph.js:138` | `resolveIncludePathAsync` does no case-insensitive walk — diverges from sync path (Linux CI risk) | Mirror the case-insensitive directory walk from the sync implementation |

---

## Phase 5 — Major: Test fixes

These are test defects that let bugs hide.

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 5.1 | Done | 11 | `test/unit/validators/functionValidator.test.js:29` | `.not.toThrow` is a property access not a call — assertion is vacuous; test always passes | `expect(() => fn()).not.toThrow()` and restructure test |
| 5.2 | Done | 11 | `test/unit/validators/functionValidator.test.js` | Only 3 trivial tests for the most complex validator; ACU-FNC-001/002/004 untested | Add tests for undefined function, wrong arg count, unused return value |
| 5.3 | Done | 11 | `test/unit/validators/typeKeywordCapsValidator.test.js` | No regression guard for the `currentPos` column-offset bug (Phase 3.1) | Add test checking `range.start.character` of second diagnostic in multi-param FUNCTION |

---

## Phase 6 — Minor fixes

### 6a — Core: parser, analysis, builtins

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.1 | Done | 1 | `core/src/logger.js:32` | Unused `context` parameter | Remove |
| 6.2 | Done | 1 | `core/src/telemetry.js:33` | Null return when logging disabled | Return `undefined` or `void` instead |
| 6.3 | Done | 1 | `core/src/parser.js:70` | `parseAssignment` — `indexOf` may hit wrong position for repeated tokens | Tighten search using offset |
| 6.4 | Done | 1 | `core/src/plainTextDocument.js:21` | `uri.toString()` returns path not URI | Used `pathToFileURL` from Node's `url` module |
| 6.5 | Done | 1 | `core/src/plainTextDocument.js` | Missing `getText()` method | Stored `_content` in constructor; implemented `getText()` |
| 6.6 | Done | 2 | `core/src/analysis/executionContext.js:288` | `while` condition tests `rawLine` instead of current iteration variable | Introduced `continuationLine` tracker updated on each iteration |
| 6.7 | Done | 2 | `core/src/analysis/executionContext.js:245` | Backslash escape detection — verify against language spec | Removed (Architect language has no backslash escapes) |
| 6.8 | Done | 2 | `core/src/analysis/handleBindingAnalysis.js:16` | Module-level stateful `/g` regex | Moved inside the per-line loop body; removed `lastIndex` reset |
| 6.9 | Done | 2 | `core/src/analysis/handleBindingAnalysis.js:61` | `INDEX_CLOSE` case-sensitive handle lookup | Normalised to lowercase on both set and delete; updated hoverProvider lookups and tests |
| 6.10 | Done | 2 | `core/src/analysis/functionAnalysis.js:101` | `params` Map uses case-sensitive keys | Fixed `isFunctionParameter` to do case-insensitive scan; kept original-case keys to preserve "did you mean" diagnostics |
| 6.11 | Done | 2 | `core/src/analysis/macroAnalysis.js:105` | Arg count wrong for nested macro args | Replaced comma-split with `extractArgAt` loop |
| 6.12 | Done | 3 | `core/src/builtins/helpers.js:31` | `hasOwnProperty` called directly on object | Replaced with `Object.prototype.hasOwnProperty.call(...)` |
| 6.13 | Done | 3 | `core/src/builtins/constantsRegistry.js:45` | `HEADER`/`BODY`/`FOOTER` as both keywords and constants | Removed from `ACURITY_KEYWORDS`; canonical role is built-in boolean constant (constantsRegistry.js); variable validator already accepts them via `isBuiltInConstant` |

### 6b — Include resolver

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.14 | Done | 4 | `core/src/includeResolver/graph.js:171` | `document` parameter unused in `isFunctionInScope` | Remove parameter |
| 6.15 | Done | 4 | `core/src/includeResolver/graph.js:311` | `findCircularIncludeDirectives` — INCLUDEONCE not respected in child files → false positive circular detections | Added `isIncludeOnce` skip inside `hasCircularIncludeFromFile` loop |
| 6.16 | Done | 4 | `core/src/includeResolver/traversal.js:494` | Three single-purpose scope builders with no deprecation marker | Added `@deprecated` JSDoc to two active builders; deleted `buildIncludedVariablesInScope` (no callers) |


### 6c — Diagnostics infrastructure

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.17 | Done | 5 | `core/src/diagnostics/diagnosticCatalog.js:152` | `ACU-MAC-003` paramKeys includes `expectedCount` not in template | Remove `expectedCount` from paramKeys |
| 6.18 | Done | 5 | `core/src/collectDiagnostics.js:17` | `path` import unused | Remove |
| 6.19 | Done | 5 | `core/src/collectDiagnostics.js:80` | `getFunctionScopeAtLine` called twice per variable per line | Hoist to single call |
| 6.20 | Done | 5 | `core/src/collectDiagnostics.js:119` | First 6 validators not wrapped in try/catch — asymmetric error isolation | Wrap consistently |
| 6.21 | Done | 5 | `core/src/suppressionParser.js:32` | `@rule-ignore` regex matches inside string literals | Strip quoted strings before matching |
| 6.22 | Done | 5 | `core/src/diagnostics/diagnosticFactory.js:52` | Regex built from unescaped param key | Escape key or use split/join |

### 6d — Validators

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.23 | Done | 6 | `core/src/validators/functionValidator.js:394` | Dead `if (builtInSignature)` guard — always true when reached | Remove conditional |
| 6.24 | Done | 6 | `core/src/validators/macroValidator.js:30` | Output-line guard inactive when `executionClassificationMap` not provided | Compute locally as fallback or document |
| 6.25 | Done | 6 | `core/src/validators/variableValidator.js:265` | `FIELD_HANDLE_LIKE` fallthrough — produces ACU-VAR-001 instead of ACU-ARG-006 | Add ACU-ARG-006 diagnostic and `continue` |
| 6.26 | Done | 6 | `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)` |
| 6.27 | Done | 6 | `core/src/validators/requireValidator.js:50` | Case-sensitive `indexOf("REQUIRE")` — returns -1 for lowercase keyword | Fixed during 4.1 merge — replaced with `buildTokenRange(lineText, lineIndex, filename, 0)` |
| 6.28 | Done | 7 | `core/src/validators/keywordCapsValidator.js:77` | Dead `currentIndex` variable — declared, never used | Remove |
| 6.29 | Done | 7 | `core/src/validators/keywordCapsValidator.js:80` | Regex rebuilt on every `findKeywordMatches` call | Hoist to module scope or cache |
| 6.30 | Done | 7 | `core/src/validators/namingValidator.js:134,168,183` | Full-line ranges for ACU-NMG-001/002/003 | Use `buildTokenRange(lineText, lineIndex, variableName, 0)` |
| 6.31 | Done | 7 | `core/src/validators/namingValidator.js:101` | Dead conditional in `stripExistingVariablePrefix` — always true | Remove `if` wrapper |
| 6.32 | Done | 7 | `core/src/validators/argumentStringValidator.js:104` | `extractStringLiteralArg` only handles double-quoted strings | Extend to single-quoted or document |
| 6.33 | Done | 7 | `core/src/validators/interpolationValidator.js:39` | Silent no-op when `executionClassificationMap` absent | Add early-return guard or log |
| 6.34 | Done | 7 | `core/src/validators/usagePatternValidator.js:102` | `getFunctionScopeAtLine` called per regex match per line | Hoist scope lookup before inner `while` |
| 6.35 | Done | 8 | `core/src/validators/robodocValidator.js:67` | `buildLineRange` called with `""` token → zero-width range for ACU-RBD-001 (invisible squiggle) | Pass `null` so fallback branch is used |
| 6.36 | Done | 8 | `core/src/validators/hashModeValidator.js:25,53,71` | Three separate full-document loops for three independent checks | Merge into single loop |

### 6e — VS Code Extension

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.37 | Done | 9 | `vscode-extension/src/commands/batchValidateTxtFolder.js:92` | Duplicate `require('vscode')` + debug notification | Remove lines 92–93 |
| 6.38 | Done | 9 | `vscode-extension/src/commands/discoveryReport.js:36` | Duplicate `require('vscode')` + debug notification | Remove lines 36–37 |
| 6.39 | Done | 9 | `vscode-extension/src/commands/discoveryReport.js:27` | `workspaceFolders[0]` without null check | Guard with `?.[0]?.uri.fsPath ?? ""` |
| 6.40 | Done | 9 | `vscode-extension/src/commands/discoveryReport.js:119` | `fs.writeFileSync` in async context — blocks event loop | Replace with `vscode.workspace.fs.writeFile` |
| 6.41 | Done | 9 | `vscode-extension/src/commands/discoveryReport.js:41` | `findFiles('**/*.txt')` misses `.TXT` files | Change to `**/*.{txt,TXT}` |
| 6.42 | Done | 9 | `vscode-extension/src/hoverProvider.js:372` | JSDoc comment for wrong function (copy-paste) | Move docstring to correct function |
| 6.43 | Done | 9 | `vscode-extension/src/hoverProvider.js:458` | `name.search(/[A-Z]/)` returns -1 for all-lowercase names → `substring(-1)` returns full string | Fix negative-index edge case |
| 6.44 | Done | 9 | `vscode-extension/src/commands/setupWizard.js:265` | `runSetupWizard` in `setTimeout` without await/error handling | Wrap in `async () => { try { await ... } catch (_) {} }` |
| 6.45 | Done | 9 | `vscode-extension/src/hoverProvider.js:7,15` | Two separate `require` calls for `parser` | Merge into single destructuring import |
| 6.46 | Done | 1 | `core/src/vscodeTypes.js:51` | Config shim ignores workspace settings | Skipped — 2.16 landed first; shim and vscodeTypes.js deleted |

### 6f — CLI + docs

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 6.47 | Done | 10 | `cli/DEVELOPMENT.md` | `vscode-shim.js` listed but does not exist | Already absent from file — moot after 2.16 |
| 6.48 | Done | 10 | `cli/DEVELOPMENT.md` | `--format` documented without *(planned)* marker but not implemented | Marked *(planned)* |
| 6.49 | Done | 10 | `cli/DEVELOPMENT.md` | `--surface` default documented as `cicd` but code default is `null` | Code default changed to `"cicd"` |
| 6.50 | Done | 10 | `cli/DEVELOPMENT.md` | `--surface all` listed as valid but validation rejects it | Added `all` → null translation; validation accepts `ide\|cicd\|all` |
| 6.51 | Done | 10 | `cli/index.js:77,189` | `readdirSync`/`readFileSync` unguarded — permission error crashes with Node stack trace | Wrapped in try/catch; exit 2 with clean message |
| 6.52 | Done | 10 | `cli/index.js:171` | `fs.existsSync` doesn't check `isDirectory()` — file path causes `ENOTDIR` crash | Replaced with `fs.statSync` + `isDirectory()` |

---

## Phase 7 — Minor: Test improvements

| # | Status | Batch | File | Issue | Fix |
|---|---|---|---|---|---|
| 7.1 | Done | 11 | `test/helpers/mockDocument.js:56` | `new RegExp(regex, 'g')` double-wraps a RegExp arg | `regex instanceof RegExp ? new RegExp(regex.source, 'g') : new RegExp(regex, 'g')` |
| 7.2 | Done | 11 | `test/unit/validators/namingValidator.test.js:97` | Dead `vars` variable; `vars2` is actually tested | Remove `vars`; rename `vars2` to `vars` |
| 7.3 | Done | 11 | `test/unit/validators/macroValidator.test.js` | ACU-MAC-002 (macro case mismatch) not tested | Added ACU-MAC-002 test; also fixed pre-existing MAC-003/004/severity tests (needed HASH-OFF prefix — default mode is HASH-ON so bare lines were classified as output and skipped) |
| 7.4 | Done | 11 | `test/unit/validators/robodocValidator.test.js` | ACU-RBD-003 and `requiredSections` not tested | Added ACU-RBD-003 (wrong kind char) and ACU-RBD-005 (missing required section) tests; fixed mock to include `kind` property |
| 7.5 | Done | 11 | `test/syntax-tests/syntaxSpecs.test.js:14` | Discovery guard `toBeGreaterThanOrEqual(10)` far below actual spec count | Raise to match actual count |
| 7.6 | Done | 11 | `test/unit/completionProvider.test.js` | Only 4 tests; no edge cases for position-in-string, position-in-comment, no macros | Add edge cases |
| 7.7 | Done | 11 | `test/unit/templateProvider.test.js:15,52` | Template asserted to contain both `<HTML>` and `<html>` — mixed-case in output | Both are correct and intentional (`<HTML>` = Architect section marker; `<html>` = HTML output content); added clarifying comments |
| 7.8 | Done | 11 | `test/unit/validators/` | Zero coverage: `collectDiagnostics`, `suppressionParser`, `blockValidator`, `diagnosticCatalog`, `documentFacts` | Added unit tests for all 4 remaining modules (`documentFacts` covered by 7.9) |
| 7.9 | Done | 2 | `test/unit/` | `documentFacts.test.js` missing — cache hit/miss/eviction untested | Add `documentFacts.test.js` |

---

## Progress summary

| Phase | Items | Done |
|---|---|---|
| 1 — Critical: Runtime crashes | 1 | 1 |
| 2 — Critical: require('vscode') sweep | 16 | 16 |
| 3 — Major: Correctness bugs | 6 | 6 |
| 4 — Major: Consolidation | 3 | 3 |
| 5 — Major: Test fixes | 3 | 3 |
| 6 — Minor fixes | 52 | 52 |
| 7 — Minor: Test improvements | 9 | 9 |
| **Total** | **90** | **90** |
