# Batch 1 — Core Infrastructure — Findings

**Date:** 2026-03-26
**Files reviewed:** `parser.js`, `constants.js`, `logger.js`, `telemetry.js`, `plainTextDocument.js`, `vscodeTypes.js`
**Status:** Complete

---

## Summary

The core infrastructure files are generally well-structured and clearly commented. The code is readable
and follows consistent conventions. However, there is one **Critical** architecture violation in
`parser.js` that must be fixed: a direct `require('vscode')` call inside a core module. Two **Major**
issues were found in `constants.js`: the type keyword sets are inconsistent across the codebase, and
`MACRO`/`ENDMACRO` are absent from the keywords set. `vscodeTypes.js` has a shim-correctness problem
with `withProgress`. `telemetry.js` has an ergonomics risk with its null-return contract.
`logger.js` and `plainTextDocument.js` are clean with only minor and observation-level findings.

---

## Findings

### `parser.js`

#### [CRITICAL] `require('vscode')` inside core/ — line 247

**Issue:** `buildTokenRange` calls `require("vscode")` directly. `parser.js` lives in `core/src/`
and rule #1 of CLAUDE.md is non-negotiable: _core/ must never require('vscode')_. This breaks
the CI/CD decoupling — the CLI and any headless test runner that loads `parser.js` will fail at
this call point unless the module intercept in `cli/index.js` is active.

**Recommendation:** Remove `vscode` from `buildTokenRange`. The function's only job is to
produce a `Range` object; it should accept a pre-constructed range factory or return a plain
`{ start, end }` object that callers convert. Alternatively, move `buildTokenRange` to
`vscode-extension/src/` where `vscode` is allowed, and have core return the raw indices.

---

#### [MINOR] `parseAssignment` — `indexOf` may hit wrong position — lines 70–71

**Issue:** `lineText.indexOf(target)` finds the _first_ occurrence of `target` in the raw
(un-stripped) line. If the variable name appears inside a preceding keyword or in a comment
segment before the actual assignment target, `targetIndex` is wrong, producing an incorrect
`expressionStart` offset.

**Example:** `// SET foo = 1 SET foo = 2` — `target` is `foo` but `indexOf("foo")` returns
the comment occurrence.

**Recommendation:** Search from the position after `SET ` or from character 0 if no `SET`,
and strip the trailing comment before the index search.

---

#### [MINOR] `parseInclude` / `parseRequire` — bare filename allows path traversal — lines 337, 352

**Issue:** The bare-filename capture group `([a-zA-Z0-9_.\/-]+)` allows `../` sequences.
If the resolved path is used to open files without normalization and boundary-checking,
a crafted Architect source file could cause the language server to read files outside the
workspace.

**Recommendation:** In the _resolver_ (not the parser), verify that the resolved absolute
path is within the workspace root before opening. The regex itself is fine for parsing.

---

### `constants.js`

#### [MAJOR] `ACURITY_TYPES` and `parseVarDeclaration` alternate-format list are inconsistent

**Issue:** `parseVarDeclaration` (in `parser.js` line 36) recognises these alternate-format
type keywords: `STRING, SHORT, LONG, DATE, DOUBLE, BYTE, LOGICAL`.

`ACURITY_TYPES` in `constants.js` (line 18) defines: `STRING, SHORT, LONG, DOUBLE, BOOLEAN, DATE, TIME, INTEGER`.

Discrepancies:
- `BYTE` and `LOGICAL` appear in the parser but not in `ACURITY_TYPES`
- `BOOLEAN`, `TIME`, `INTEGER` appear in `ACURITY_TYPES` but not in the parser alternate list

This means `ACURITY_TYPES` cannot be used as a canonical type-name validator — it will
miss `BYTE`/`LOGICAL` and falsely admit `BOOLEAN`/`TIME`/`INTEGER` in contexts where only
the alternate-format types are valid.

**Recommendation:** Establish a single source of truth. Determine the full canonical type set
for the Architect language (from the language spec), update `ACURITY_TYPES` accordingly, and
drive `parseVarDeclaration` from that set rather than a hardcoded inline list.

---

#### [MAJOR] `MACRO` and `ENDMACRO` missing from `ACURITY_KEYWORDS` — lines 5–15

**Issue:** `MACRO` and `ENDMACRO` are language keywords (the parser has `parseMacroDeclaration`;
validators reference macros) but are absent from `ACURITY_KEYWORDS`. This means:
- `extractVariables` in `parser.js` will not skip `MACRO`/`ENDMACRO` when scanning expressions
- Any keyword-caps validator that references `ACURITY_KEYWORDS` will not enforce casing on them
- Completion or hover providers that use the keyword set will omit them

**Recommendation:** Add `"MACRO"` and `"ENDMACRO"` to `ACURITY_KEYWORDS`. Also audit whether
`BYTE`, `LOGICAL`, and the `TIME`/`INTEGER` types (above finding) should be present.

---

### `logger.js`

#### [MINOR] `initLogger` — `context` parameter is declared but never used — line 32

**Issue:** The function signature is `initLogger(context)` and the JSDoc types it as
`import('vscode').ExtensionContext`, but `context` is not referenced anywhere in the
function body. The log directory is determined from `os.homedir()` alone.

This misleads readers and callers into thinking the extension context is needed; it also
forces `vscode-extension/` callers to pass `context` for no effect.

**Recommendation:** Remove the `context` parameter and its JSDoc. If the intent was to use
`context.logUri` (VS Code's managed log path) in future, leave a TODO comment explaining that.

---

#### [OBSERVATION] `writeQueue` promise chain grows unbounded

**Issue:** Every `queueLogWrite` call appends `.then(...)` to `writeQueue`. If logging is
active for a long session with many log events, the chain grows without being trimmed. In
practice, file I/O is fast and the chain stays shallow, but there is no mechanism to drain or
reset it.

**Recommendation:** Accept as-is for now. If performance ever becomes a concern, replace the
chain with a queue array and a single draining loop.

---

### `telemetry.js`

#### [MINOR] `createTelemetryRun` returns `null` when logging is disabled — line 33

**Issue:** When `logger.isLoggingEnabled()` is false, the function returns `null`. Every
caller must guard: `if (run) { run.startPhase(...) }`. If a caller forgets the null check,
it throws at runtime. The null return is not enforced by the type system and is only mentioned
in the schema comment block, not in the JSDoc.

**Recommendation:** Use a null-object pattern — return a no-op run object with the same
interface (all methods do nothing, `emit` does nothing). This eliminates all null-guard
boilerplate in callers. Alternatively, add `@returns {object|null}` JSDoc and a prominent
`// Returns null if logging is disabled — callers must guard.` comment.

---

#### [OBSERVATION] Module-level `runCounter` is mutable shared state — line 8

**Issue:** `runCounter` is incremented on every `createTelemetryRun` call and shared across
all require() consumers (Node caches modules). In a single-process extension this is harmless
and intentional. Noted for awareness if the module is ever used in a worker thread or test
isolation context.

---

### `plainTextDocument.js`

#### [MINOR] `uri.toString()` returns a file path, not a URI — line 21

**Issue:** `this.uri = { fsPath: filePath, toString: () => filePath }` — `toString()` returns
the raw path (e.g. `C:\path\to\file.acurity`). The real VS Code `TextDocument.uri.toString()`
returns a `file:///...` URI string. Any core code that calls `document.uri.toString()` and
then parses or compares it as a URI will behave differently between the extension and CLI paths.

**Recommendation:** Audit callers of `document.uri.toString()` in core. If any use the result
as a URI (e.g. to construct an include path), the shim should return `"file://" + filePath`
or the callers should use `document.uri.fsPath` instead.

---

#### [MINOR] No `getText()` method implemented — line 12

**Issue:** VS Code's `TextDocument` API includes `getText(range?: Range): string`. If any
existing or future validator calls `document.getText()`, it will throw `TypeError: document.getText is not a function` in CLI mode. Currently no validator in the batch calls it, but the missing method is a latent risk.

**Recommendation:** Add a `getText()` method that joins `this._lines`. This closes the interface gap cheaply.

---

#### [OBSERVATION] Out-of-bounds `lineAt` silently returns empty string — line 28

**Issue:** `this._lines[index] ?? ""` — requesting a line index beyond `lineCount - 1` silently
returns `""`. This is defensive, but it means callers with an off-by-one bug in their line
index will receive empty-string results rather than a thrown error, making the bug harder to
detect.

**Recommendation:** Accept the current behaviour — crashing a language server or CLI on an
out-of-bounds access would be worse than silently returning an empty line. Observation only.

---

### `vscodeTypes.js`

#### [MAJOR] `withProgress` shim does not invoke the callback — line 66

**Issue:** The real `vscode.window.withProgress(options, task)` calls `task(progress, token)`
and returns the task's result. The shim is `withProgress: () => {}` — it swallows the call
and never invokes the callback. If any CLI-path code wraps execution logic inside
`withProgress`, that logic silently does not run under CLI/headless conditions.

This would cause silent, complete feature omission rather than a thrown error, making it
difficult to diagnose.

**Recommendation:** Change the shim to invoke the task callback immediately:
`withProgress: (_opts, task) => task({ report: () => {} }, { isCancellationRequested: false })`.

---

#### [MINOR] Config shim always returns defaults — line 51

**Issue:** `getConfiguration: () => ({ get: (_key, defaultValue) => defaultValue })` — all
configuration reads in CLI mode return the default value, regardless of any `.vscode/settings.json`
or workspace config on disk. If a user's configuration changes validator behaviour (e.g. disabling
a rule), the CLI will not respect that configuration and may produce different results from
the extension.

**Recommendation:** Document this limitation explicitly in `cli/DEVELOPMENT.md`. Optionally,
implement a lightweight config reader for the two or three settings that affect validator
behaviour (already deferred as a CLI feature).

---

#### [OBSERVATION] TODO to eliminate this module is self-documented — lines 13–15

**Issue:** The file contains a TODO explaining that it should be eliminated by refactoring
validators to accept plain range objects and injected config. This is sound technical debt
documentation. The refactor would also fix the `require('vscode')` violation in `parser.js`
(Critical above) and the `withProgress` issue.

**Recommendation:** Treat this as a medium-term architectural goal. Add it to the actions
backlog below.

---

## Positive observations

- `telemetry.js` is a clean, self-contained module. The schema comment block at the top is
  excellent documentation for future event authors.
- `plainTextDocument.js` is admirably minimal — it does exactly what it needs to do.
- `stripTrailingComment`, `findUnclosedString`, and `stripQuotedStrings` in `parser.js`
  all handle both single and double quotes consistently and correctly.
- `extractArgAt` in `parser.js` correctly handles nested parentheses and quoted strings.
- `logger.js`'s use of a promise chain (`writeQueue → logReady`) ensures writes are not
  attempted before the log directory is created — a subtle but correct design.

---

## Actions for backlog

Copy these rows into `TRACKER.md` → Actions backlog.

| Severity | File | Issue | Recommended action |
|---|---|---|---|
| Critical | `core/src/parser.js:247` | `require('vscode')` in core/ | Remove; move `buildTokenRange` to extension or refactor to return plain indices |
| Major | `core/src/constants.js:18` | `ACURITY_TYPES` inconsistent with parser type list | Canonicalise against language spec; drive parser from the set |
| Major | `core/src/constants.js:5` | `MACRO`/`ENDMACRO` missing from `ACURITY_KEYWORDS` | Add to set; audit related validators |
| Major | `core/src/vscodeTypes.js:66` | `withProgress` shim swallows callback | Change shim to invoke task immediately |
| Minor | `core/src/logger.js:32` | Unused `context` parameter | Remove parameter and JSDoc |
| Minor | `core/src/telemetry.js:33` | Null return when logging disabled | Null-object pattern or prominent JSDoc |
| Minor | `core/src/parser.js:70` | `parseAssignment` indexOf may hit wrong position | Search from correct offset |
| Minor | `core/src/plainTextDocument.js:21` | `uri.toString()` returns path not URI | Audit callers; fix shim or callers |
| Minor | `core/src/plainTextDocument.js` | Missing `getText()` method | Add trivial implementation |
| Minor | `core/src/vscodeTypes.js:51` | Config shim ignores workspace settings | Document limitation in cli/DEVELOPMENT.md |
| Minor | `core/src/parser.js:337` | Bare include path allows `../` traversal | Add boundary check in resolver |
| Observation | `core/src/vscodeTypes.js:13` | TODO: eliminate this module via refactor | Track as medium-term architectural goal |
