# Code Review Tracker

Structured review of all product JavaScript source files under `architect-language-tools/`.
Approach: **Review-then-fix** (Option A). Each session produces a findings document.
Fixes are planned and applied in a separate follow-up session.

---

## Review checklist (applied to every file)

- **Correctness** — logic errors, edge cases, off-by-one, silent wrong-path behaviour
- **Security** — path traversal, regex DoS (ReDoS), command injection, untrusted input
- **Architecture compliance** — no `require('vscode')` in core; no new runtime dependencies
- **Maintainability** — dead code, duplicated logic, unclear naming, overly complex functions
- **Error handling** — unhandled promise rejections, swallowed errors, missing failure paths
- **Test coverage** — behaviour covered by tests? notable gaps?

---

## Severity scale

| Label | Meaning |
|---|---|
| **Critical** | Correctness or security defect — must fix before next release |
| **Major** | Significant maintainability or reliability problem — fix soon |
| **Minor** | Small improvement — fix opportunistically |
| **Observation** | Noteworthy but not actionable — document and move on |

---

## Batch status

| # | Area | Files | Status | Findings doc |
|---|---|---|---|---|
| 1 | Core infrastructure | `parser.js`, `constants.js`, `logger.js`, `telemetry.js`, `plainTextDocument.js`, `vscodeTypes.js` | Complete | [BATCH_01_core_infrastructure.md](findings/BATCH_01_core_infrastructure.md) |
| 2 | Analysis layer | `analysis/executionContext.js`, `analysis/functionAnalysis.js`, `analysis/functionResolution.js`, `analysis/handleBindingAnalysis.js`, `analysis/macroAnalysis.js`, `analysis/documentFacts.js` | Complete | [BATCH_02_analysis_layer.md](findings/BATCH_02_analysis_layer.md) |
| 3 | Builtins & types | `builtins/constantsRegistry.js`, `builtins/fieldRegistry.js`, `builtins/functions.js`, `builtins/helpers.js`, `builtins/tableSynonymsRegistry.js`, `builtins/functionsRegistry.js`, `types/acurityTypes.js` | Complete | [BATCH_03_builtins_and_types.md](findings/BATCH_03_builtins_and_types.md) |
| 4 | Include resolution | `includeResolver.js`, `includeResolver/cache.js`, `includeResolver/graph.js`, `includeResolver/traversal.js` | Complete | [BATCH_04_include_resolution.md](findings/BATCH_04_include_resolution.md) |
| 5 | Diagnostics infrastructure | `diagnostics/diagnosticCatalog.js`, `diagnostics/quickFixCatalog.js`, `diagnostics/diagnosticFactory.js`, `collectDiagnostics.js`, `blockValidator.js`, `suppressionParser.js` | Complete | [BATCH_05_diagnostics_infrastructure.md](findings/BATCH_05_diagnostics_infrastructure.md) |
| 6 | Validators — semantic | `validators/functionValidator.js`, `validators/macroValidator.js`, `validators/variableValidator.js`, `validators/typeValidator.js`, `validators/requireValidator.js`, `validators/includeValidator.js` | Complete | [BATCH_06_validators_semantic.md](findings/BATCH_06_validators_semantic.md) |
| 7 | Validators — style/naming | `validators/keywordCapsValidator.js`, `validators/typeKeywordCapsValidator.js`, `validators/namingValidator.js`, `validators/argumentStringValidator.js`, `validators/interpolationValidator.js`, `validators/usagePatternValidator.js` | Complete | [BATCH_07_validators_style_naming.md](findings/BATCH_07_validators_style_naming.md) |
| 8 | Validators — misc | `validators/hashModeValidator.js`, `validators/robodocValidator.js`, `validators/todoValidator.js` | Complete | [BATCH_08_validators_misc.md](findings/BATCH_08_validators_misc.md) |
| 9 | VS Code Extension | All `vscode-extension/src/` (11 files) | Complete | [BATCH_09_vscode_extension.md](findings/BATCH_09_vscode_extension.md) |
| 10 | CLI | `cli/index.js` | Complete | [BATCH_10_cli.md](findings/BATCH_10_cli.md) |
| 11 | Test layer | All `test/` JS files (~24 files) — coverage gaps and test quality | Complete | [BATCH_11_test_layer.md](findings/BATCH_11_test_layer.md) |

---

## Sessions log

| Date | Batch # | Outcome |
|---|---|---|
| 2026-03-26 | 1 | Complete — 1 Critical, 3 Major, 6 Minor, 4 Observations |
| 2026-03-26 | 2 | Complete — 0 Critical, 1 Major, 8 Minor, 5 Observations |
| 2026-03-26 | 3 | Complete — 0 Critical, 1 Major, 3 Minor, 5 Observations |
| 2026-03-26 | 4 | Complete — 0 Critical, 2 Major, 3 Minor, 4 Observations |
| 2026-03-26 | 5 | Complete — 1 Critical, 1 Major, 6 Minor, 5 Observations |
| 2026-03-26 | 6 | Complete — 4 Critical, 0 Major, 6 Minor, 1 Design, 9 Observations |
| 2026-03-26 | 7 | Complete — 6 Critical, 1 Major, 7 Minor, 4 Observations |
| 2026-03-26 | 8 | Complete — 3 Critical, 0 Major, 2 Minor, 3 Observations |
| 2026-03-26 | 9 | Complete — 1 Critical, 1 Major, 9 Minor, 5 Observations |
| 2026-03-26 | 10 | Complete — 0 Critical, 0 Major, 5 Minor, 4 Observations |
| 2026-03-26 | 11 | Complete — 0 Critical, 3 Major, 10 Minor, 5 Observations |

---

## Actions backlog

Issues from completed reviews that are queued for fixing. Updated after each findings doc is produced.

| Batch | Severity | File | Issue | Status |
|---|---|---|---|---|
| 1 | Critical | `core/src/parser.js:247` | `require('vscode')` in core/ | Done |
| 1 | Major | `core/src/constants.js:18` | `ACURITY_TYPES` inconsistent with parser type list | Done |
| 1 | Major | `core/src/constants.js:5` | `MACRO`/`ENDMACRO` missing from `ACURITY_KEYWORDS` | Done |
| 1 | Major | `core/src/vscodeTypes.js:66` | `withProgress` shim swallows callback | Done — moot; `vscodeTypes.js` deleted (2.16) |
| 1 | Minor | `core/src/logger.js:32` | Unused `context` parameter | Done |
| 1 | Minor | `core/src/telemetry.js:33` | Null return when logging disabled | Done |
| 1 | Minor | `core/src/parser.js:70` | `parseAssignment` indexOf may hit wrong position | Done |
| 1 | Minor | `core/src/plainTextDocument.js:21` | `uri.toString()` returns path not URI | Done |
| 1 | Minor | `core/src/plainTextDocument.js` | Missing `getText()` method | Done |
| 1 | Minor | `core/src/vscodeTypes.js:51` | Config shim ignores workspace settings | Document in CLI DEVELOPMENT.md |
| 1 | Minor | `core/src/parser.js:337` | Bare include path allows `../` traversal | Pending fix in resolver |
| 2 | Major | `core/src/analysis/functionAnalysis.js:15` | Duplicate `parseFunctionDeclaration` diverges from `parser.js` | Done |
| 2 | Minor | `core/src/analysis/executionContext.js:288` | `while` condition tests `rawLine` instead of current line | Done |
| 2 | Minor | `core/src/analysis/executionContext.js:245` | Backslash escape detection may not apply to Architect | Done (removed) |
| 2 | Minor | `core/src/analysis/handleBindingAnalysis.js:16` | Module-level stateful `/g` regex | Done |
| 2 | Minor | `core/src/analysis/handleBindingAnalysis.js:61` | `INDEX_CLOSE` case-sensitive handle lookup | Done |
| 2 | Minor | `core/src/analysis/functionAnalysis.js:101` | `params` Map uses case-sensitive keys | Done (fixed isFunctionParameter lookup) |
| 2 | Minor | `core/src/analysis/macroAnalysis.js:105` | Arg count wrong for nested macro args | Done |
| 2 | Minor | `core/src/analysis/macroAnalysis.js` | Two separate document scans for catalog + duplicates | Merge if profiling warrants |
| 2 | Minor | `core/src/analysis/documentFacts.js` | No unit tests for cache hit/miss/eviction | Done |
| 3 | Major | `core/src/builtins/functionsRegistry.js:~1387` | `params: [{ type: 'UNKNOWN' }]` format inconsistent with rest of registry | Normalise to `[AcurityTypes.UNKNOWN]` . UPDATE: This has likely now been actioned.|
| 3 | Minor | `core/src/builtins/helpers.js:31` | `hasOwnProperty` called directly on object | Done |
| 3 | Minor | `core/src/builtins/functionsRegistry.js` | String literal `returns` values mixed with `AcurityTypes` constants | Normalise to `AcurityTypes.*` throughout . UPDATE: This has likely now been actioned.|
| 3 | Minor | `core/src/builtins/constantsRegistry.js:45` | `HEADER`/`BODY`/`FOOTER` as both keywords and constants | Done |
| 4 | Major | `core/src/includeResolver/graph.js:29` | Third independent `parseFunctionDeclarationName` — adds `#?` prefix handling absent from other two | Done |
| 4 | Major | `core/src/includeResolver/graph.js:138` | `resolveIncludePathAsync` does not do case-insensitive walk — diverges from sync path | Done |
| 4 | Minor | `core/src/includeResolver/graph.js:171` | `document` parameter unused in `isFunctionInScope` | Done |
| 4 | Minor | `core/src/includeResolver/graph.js:311` | `findCircularIncludeDirectives` — INCLUDEONCE not respected in child files — false positive circular detections | Done |
| 4 | Minor | `core/src/includeResolver/traversal.js:494` | Three single-purpose scope builders have no deprecation marker | Done |
| 5 | Critical | `core/src/blockValidator.js:6` | `require('vscode')` in core/ (second violation) | Done |
| 5 | Major | `core/src/diagnostics/diagnosticCatalog.js` | `legacy-compatible` profile missing 5 macro rule severity entries | Done |
| 5 | Minor | `core/src/diagnostics/diagnosticCatalog.js:152` | `ACU-MAC-003` paramKeys includes `expectedCount` not in template | Done |
| 5 | Minor | `core/src/collectDiagnostics.js:17` | `path` import unused | Done |
| 5 | Minor | `core/src/collectDiagnostics.js:80` | `getFunctionScopeAtLine` called twice per variable per line | Done |
| 5 | Minor | `core/src/collectDiagnostics.js:119` | First 6 validators not wrapped in try/catch — asymmetric error isolation | Done |
| 5 | Minor | `core/src/suppressionParser.js:32` | `@rule-ignore` regex matches inside string literals | Done |
| 5 | Minor | `core/src/diagnostics/diagnosticFactory.js:52` | Regex built from unescaped param key | Done |
| 6 | Critical | `core/src/validators/functionValidator.js:6` | `require('vscode')` — 3rd violation | Done |
| 6 | Critical | `core/src/validators/macroValidator.js:6` | `require('vscode')` — 4th violation | Done |
| 6 | Critical | `core/src/validators/typeValidator.js:6` | `require('vscode')` — 5th violation | Done |
| 6 | Critical | `core/src/validators/requireValidator.js:6` | `require('vscode')` — 6th violation | Done |
| 6 | Minor | `core/src/validators/functionValidator.js:394` | Dead `if (builtInSignature)` guard — always true when reached | Done |
| 6 | Minor | `core/src/validators/macroValidator.js:30` | Output-line guard inactive when `executionClassificationMap` not provided | Done |
| 6 | Minor | `core/src/validators/variableValidator.js:265` | `FIELD_HANDLE_LIKE` fallthrough — unknown handle-like fields produce ACU-VAR-001 instead of ACU-ARG-006 | Done |
| 6 | Minor | `core/src/validators/typeValidator.js:44` | Full-line range for ACU-TYPE-002 — should target assignment target token | Done |
| 6 | Minor | `core/src/validators/requireValidator.js:50` | Case-sensitive `indexOf("REQUIRE")` — returns -1 for lowercase keyword | Done — fixed during 4.1 merge |
| 6 | Minor | `core/src/validators/requireValidator.js:47` | `fs.existsSync` bypasses shared stat cache | Route through `cache.js` stat helper or accept as-is |
| 6 | Design | `core/src/validators/requireValidator.js` | Merge into `includeValidator.js` — fixes Critical + 2 Minors simultaneously | Done |
| 7 | Critical | `core/src/validators/keywordCapsValidator.js:6` | `require('vscode')` — 7th violation | Done |
| 7 | Critical | `core/src/validators/typeKeywordCapsValidator.js:7` | `require('vscode')` — 8th violation | Done |
| 7 | Critical | `core/src/validators/namingValidator.js:6` | `require('vscode')` — 9th violation | Done; `vscode.workspace.getConfiguration` replaced with hardcoded defaults (accepted limitation — consistent with CLI shim) |
| 7 | Critical | `core/src/validators/argumentStringValidator.js:7` | `require('vscode')` — 10th violation | Done |
| 7 | Critical | `core/src/validators/interpolationValidator.js:6` | `require('vscode')` — 11th violation | Done |
| 7 | Critical | `core/src/validators/usagePatternValidator.js:11` | `require('vscode')` — 12th violation | Done |
| 7 | Major | `core/src/validators/typeKeywordCapsValidator.js:147` | `currentPos` not advanced for correctly-cased params — wrong column for subsequent type errors in multi-param functions | Done |
| 7 | Minor | `core/src/validators/keywordCapsValidator.js:77` | Dead variable `currentIndex` — declared but never used | Done |
| 7 | Minor | `core/src/validators/keywordCapsValidator.js:80` | Regex rebuilt on every `findKeywordMatches` call | Done |
| 7 | Minor | `core/src/validators/namingValidator.js:134,168,183` | Full-line ranges for ACU-NMG-001/002/003 — variable name token not targeted | Done |
| 7 | Minor | `core/src/validators/namingValidator.js:101` | Dead conditional in `stripExistingVariablePrefix` — always true at that point | Done |
| 7 | Minor | `core/src/validators/argumentStringValidator.js:104` | `extractStringLiteralArg` only handles double-quoted strings | Done |
| 7 | Minor | `core/src/validators/interpolationValidator.js:39` | Silent no-op when `executionClassificationMap` absent — zero diagnostics with no warning | Done |
| 7 | Minor | `core/src/validators/usagePatternValidator.js:102` | `getFunctionScopeAtLine` called per regex match per line, not per line | Done |
| 8 | Critical | `core/src/validators/hashModeValidator.js:10` | `require('vscode')` — 13th violation | Done |
| 8 | Critical | `core/src/validators/robodocValidator.js:1` | `require('vscode')` — 14th violation | Done |
| 8 | Critical | `core/src/validators/todoValidator.js:11` | `require('vscode')` — 15th violation | Done |
| 8 | Minor | `core/src/validators/hashModeValidator.js:25,53,71` | Three separate full-document loops for three independent checks | Done |
| 8 | Minor | `core/src/validators/robodocValidator.js:67` | `buildLineRange` called with `""` token produces zero-width range for ACU-RBD-001 — invisible squiggle | Done |
| 9 | Critical | `vscode-extension/src/diagnostics.js:91` | `path.normalize` called but `path` module never imported — `ReferenceError` on any include file change event | Done |
| 9 | Major | `vscode-extension/src/codeActionProvider.js:567` | `createIndexSaveRestoreFixes` inserts handle variable name as `INDEX_SAVE_POSITION` argument — table synonym string expected | Done |
| 9 | Minor | `vscode-extension/src/commands/batchValidateTxtFolder.js:92` | Duplicate `require('vscode')` and debug notification in `runBatchValidation` | Done |
| 9 | Minor | `vscode-extension/src/commands/discoveryReport.js:36` | Duplicate `require('vscode')` and debug notification in `runDiscoveryReport` | Done |
| 9 | Minor | `vscode-extension/src/commands/discoveryReport.js:27` | `workspaceFolders[0]` accessed without null check in `chooseOutputFile` | Done |
| 9 | Minor | `vscode-extension/src/commands/discoveryReport.js:119` | `fs.writeFileSync` in async context — blocks event loop | Done |
| 9 | Minor | `vscode-extension/src/commands/discoveryReport.js:41` | `findFiles('**/*.txt')` misses `.TXT` files — inconsistent with sibling command | Done |
| 9 | Minor | `vscode-extension/src/hoverProvider.js:372` | JSDoc comment for `buildCharToTypeMap` describes hover content — copy-paste from `buildVariableHoverContent` | Done |
| 9 | Minor | `vscode-extension/src/hoverProvider.js:458` | `name.search(/[A-Z]/)` returns -1 for all-lowercase names; `substring(-1)` returns full string — wrong recommended name | Done |
| 9 | Minor | `vscode-extension/src/commands/setupWizard.js:265` | `runSetupWizard` launched from `setTimeout` without await or error handling — unhandled promise rejection | Done |
| 9 | Minor | `vscode-extension/src/hoverProvider.js:7,15` | Two separate `require` calls for `../../core/src/parser` | Done |
| 10 | Minor | `cli/DEVELOPMENT.md` | `vscode-shim.js` listed in folder structure but file does not exist — shim is inline in `index.js` | Done |
| 10 | Minor | `cli/DEVELOPMENT.md` | `--format` flag documented without *(planned)* marker but not implemented in `parseArgs` | Done |
| 10 | Minor | `cli/DEVELOPMENT.md` | `--surface` default documented as `cicd` but code default is `null` (all surfaces) | Done |
| 10 | Minor | `cli/DEVELOPMENT.md` | `--surface all` listed as valid but validation rejects it — only `ide`/`cicd` accepted | Done |
| 10 | Minor | `cli/index.js:77,189` | `readdirSync` and `readFileSync` unguarded — permission error / TOCTOU race crashes CLI with Node stack trace instead of exit 2 | Done |
| 10 | Minor | `cli/index.js:171` | `fs.existsSync` does not check `isDirectory()` — passing a file path causes `ENOTDIR` crash | Done |
| 11 | Major | `test/unit/validators/functionValidator.test.js:29` | `.not.toThrow` is a property access not a call — assertion is vacuous; test always passes | Done |
| 11 | Major | `test/unit/validators/functionValidator.test.js` | Only 3 trivial test cases for the most complex validator — no tests for ACU-FNC-001/002/004 or built-in function calls | Done |
| 11 | Major | `test/unit/validators/typeKeywordCapsValidator.test.js` | No regression guard for the `currentPos` column-offset bug (Batch 7 Major) — multi-param with mixed correctness is untested | Done |
| 11 | Minor | `test/helpers/mockDocument.js:56` | `new RegExp(regex, 'g')` double-wraps a RegExp arg — produces broken literal pattern matching slashes | Done |
| 11 | Minor | `test/unit/validators/namingValidator.test.js:97` | Dead `vars` variable declared but `vars2` is what's actually tested | Done |
| 11 | Minor | `test/unit/validators/macroValidator.test.js` | ACU-MAC-002 (macro case mismatch) has no test — `macroCaseMap` mechanism is untested | Done |
| 11 | Minor | `test/unit/validators/robodocValidator.test.js` | ACU-RBD-003 (wrong header target) and `requiredSections` enforcement are configured but never asserted | Done |
| 11 | Minor | `test/syntax-tests/syntaxSpecs.test.js:14` | Discovery guard `toBeGreaterThanOrEqual(10)` far below actual spec count — will not catch accidental spec deletions | Done |
| 11 | Minor | `test/unit/completionProvider.test.js` | Only 4 tests; no edge cases for position-in-string, position-in-comment, no macros, no prefix | Done |
| 11 | Minor | `test/unit/templateProvider.test.js:15,52` | Template asserted to contain both `<HTML>` and `<html>` — reveals mixed-case HTML tags in generated output | Done |
