# V2 Code Review — Overview

**Date:** 2026-04-06  
**Scope:** Lexer, Parser, AST, Validators — architectural coherence  
**Reviewer:** Claude (claude-sonnet-4-6)  
**Basis:** Full read of all in-scope source files; comparison against v1 review findings  
**Prior review:** `planning/reviews/v1_review/`

---

## Purpose

The v1 review (2026-03-26) identified correctness and architecture issues across the whole codebase.
Almost all v1 findings have been resolved — the TRACKER shows 100% closure except two deferred items.

This v2 review has a different focus: **plugin architecture coherence**, specifically:

1. Does validator behaviour compensate for incomplete AST functionality?
2. Do regex patterns in validators indicate parsing work that should belong earlier in the pipeline?
3. What is the relationship between the new Lexer/AST layer and the old `parser.js` string-level layer?

---

## What changed since v1

The biggest structural change since v1 is the introduction of a **proper Lexer + AST**:

| New file | Role |
|---|---|
| `core/src/lexer/lexer.js` | Full tokeniser — handles hash mode, string/date/time literals, operators |
| `core/src/lexer/tokenTypes.js` | 30+ distinct token types |
| `core/src/ast/parser.js` | Statement-level parser: Token[] → Program AST |
| `core/src/ast/expressionParser.js` | Pratt expression parser: TokenSpan → ExprNode tree |
| `core/src/ast/nodes.js` | 30+ AST node types covering all Architect constructs |
| `core/src/ast/documentAst.js` | Memoised AST builder (cache by uri+version) |

`documentFacts.js` has been partially migrated to use the AST for:
- Function scope building (`buildFunctionScopesFromAst`)
- Function catalog (`buildFunctionCatalogFromAst`)
- Duplicate declaration detection (`findDuplicateFunctionDeclarationsFromAst`)
- Macro catalog and duplicates (`buildMacroCatalogFromAst`, `findDuplicateMacroDeclarationsFromAst`)

`hashModeValidator.js` uses the **token stream** for ACU-SYN-003 — an example of the right approach.

All 15 `require('vscode')` violations from v1 are fixed. The `vscodeTypes.js` shim has been deleted.

---

## The central architectural tension

The codebase now has **two parallel parsing layers**:

```
Layer A (old, string-level):
  raw line text
    → stripTrailingComment() / stripQuotedStrings()
      → regex patterns in parser.js
        → extracted values (target, expression, name, ...)

Layer B (new, AST-level):
  raw document text
    → tokenize() [lexer]
      → parse() [ast/parser]
        → Program { statements: [...] }
          → VarStatement / SetStatement / AssignStatement / ReturnStatement / DoStatement / ...
            → expressionParser → ExprNode tree
```

`documentFacts.js` uses **Layer B** for structural analysis (scopes, catalogs).
Most validators still use **Layer A** to work line-by-line on raw text.

This split creates three classes of problem:

1. **Compensating behaviour** — validators use regex to detect what the AST already knows
   (example: checking if a line starts with `FUNCTION` to skip it, rather than checking the AST node type)

2. **Duplicated type inference** — the expression parser infers literal types; the return-type validator
   reimplements the same logic via regex on raw expression text

3. **Position fragility** — validators that use `indexOf` / `search` on raw strings to find token
   positions can get incorrect columns when the same string appears earlier on the line

---

## Findings summary

| Finding | File(s) | Severity | Category |
|---|---|---|---|
| F-01 | `variableValidator.js` | Major | Compensating for AST |
| F-02 | `returnTypeValidator.js` | Major | Compensating for AST + duplicated inference |
| F-03 | `collectDiagnostics.js` | Major | Compensating for AST |
| F-04 | `functionValidator.js` | Major | Compensating for AST (skip guards) |
| F-05 | `functionValidator.js` | Moderate | Position fragility via indexOf |
| F-06 | `executionContext.js` | Design | Line classifier parallel to AST |
| F-07 | `hashModeValidator.js` | Minor | Mixed: uses token stream for ACU-SYN-003 but falls back to regex for ACU-SYN-001/002 |
| F-08 | `expressionParser.js` | Observation | Correct and capable, but validators don't call it |

Detailed findings are in the `findings/` subdirectory.

---

## Remediation philosophy

The AST is a **good investment** — it is correct, well-tested, and complete. The migration path is not
"big bang refactor all validators" but rather incremental wins ordered by risk vs. benefit:

**Tier 1 — Clear wins, low risk** (individual functions; no validator signature change required):
- Replace `DO_ITERATOR_RE` regex in variableValidator with AST `DoStatement.iterator`
- Replace `RETURN_RE` regex in returnTypeValidator with AST `ReturnStatement` nodes
- Move `parseVarDeclaration` scanning in collectDiagnostics to documentFacts using `VarStatement` nodes
- Replace `inferReturnExpressionType` regex type-inference with `expressionParser.parseExpression`

**Tier 2 — Medium wins, moderate complexity** (validator receives AST reference):
- Eliminate `shouldSkipFunctionValidationLine` skip guards in functionValidator by driving from AST nodes
- Replace `parseAssignment` calls in variableValidator with `SetStatement`/`AssignStatement` nodes

**Tier 3 — Architectural decision required** (affects ECM consumers; large surface):
- Rebuild `executionClassificationMap` from an AST walk instead of regex line scan
- Port `validateCodeSnippet` in functionValidator to use expression parser Call nodes

Tier 3 changes touch the most code and should be preceded by the Tier 1/2 wins to build confidence.

See individual findings files for per-change analysis.
