aboutsummaryrefslogtreecommitdiff
path: root/ISSUES.md
diff options
context:
space:
mode:
authorDavid Czihak <git@dcz.at>2026-05-10 19:24:37 +0200
committerDavid Czihak <git@dcz.at>2026-05-10 19:24:37 +0200
commit82901bb223b3592b847099d245495decfa0474c3 (patch)
treeccfd2a64e3db1f160ee5344f6bcc0d12fdb8696a /ISSUES.md
parentb80b9c1f82585677a7c042557576c41b1670d259 (diff)
Fix: Code review fixes and 0.2.0 release prep
- localizeText: return fallback when localization key is missing instead of a developer-facing "Localization missing for X" message - resolveCleanPaths: remove dead `normalized === ""` branch (unreachable after the preceding absolute-path guard) - resolveBuildStepAction: validate step before the async resolveZigExecutable call to avoid a pointless `which zig` subprocess on invalid input - buildShellCommand: join cd + command with `&&` instead of `;` so a failed cd aborts instead of running the command in the wrong directory - resolveWatchAction: cap debounceMs at 60 000; add min/max to schema - USER_OPTION_REGEX: tighten =.* to =.+ to reject empty option values - Bump version to 0.2.0 - Finalize CHANGELOG for 0.2.0 — 2026-05-10 - NOTICES/README: update zig-debug → zig-mark asset references - ISSUES.md: mark A1–A6 fixed, update all file paths to Zig.novaextension/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Diffstat (limited to 'ISSUES.md')
-rw-r--r--ISSUES.md108
1 files changed, 64 insertions, 44 deletions
diff --git a/ISSUES.md b/ISSUES.md
index 5ecd28c..7866ee1 100644
--- a/ISSUES.md
+++ b/ISSUES.md
@@ -18,19 +18,19 @@ A living catalog of identified issues in this extension. New findings should be
### FIXED — `launchInTerminal` could hang on synchronous start failure
-- **Location**: [Scripts/main.js](Scripts/main.js), `launchInTerminal`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `launchInTerminal`
- **Description**: `process.start()` was called outside any try/catch. A synchronous throw left the returned `Promise` unresolved, so the `runInTerminal` command would hang forever.
- **Resolution**: Wrapped in try/catch; on throw, resolves with `{status: -1, stderr: String(error)}` so the existing caller's `result.status !== 0` branch surfaces the failure.
### FIXED — Fire-and-forget `pushConfiguration` swallowed rejections
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` and `ZigLanguageServer.observeConfig` (three call sites)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` and `ZigLanguageServer.observeConfig` (three call sites)
- **Description**: All three sites used `void this.pushConfiguration()`. Any rejection became an unhandled promise rejection with no log trace.
- **Resolution**: Replaced with `.catch((error) => console.error(...))`.
### FIXED — `runProcess` had no rejection path for stuck child processes
-- **Location**: [Scripts/main.js](Scripts/main.js), `runProcess`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `runProcess`
- **Description**: The promise resolved only on `onDidExit`. A child process that started but never exited (e.g. a misbehaving `build.zig`) would stall any caller indefinitely. `provideTasks` was the most exposed.
- **Resolution**: Added optional `timeoutMs` option. On timeout, sends `SIGTERM` (guarded for missing `process.signal`) and resolves with `{status: -1, stderr: "...[timeout after Nms]"}`. Applied 60s timeout to the `zig build --list-steps` call in `stepCache.fetch`.
@@ -42,13 +42,13 @@ A living catalog of identified issues in this extension. New findings should be
#### FIXED — Dispose loops have no per-item try/catch
-- **Location**: [Scripts/main.js](Scripts/main.js), `deactivate` and `ZigLanguageServer.dispose`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `deactivate` and `ZigLanguageServer.dispose`
- **Description**: Both iterate disposables and call `.dispose()` without protection. One throwing disposable aborted the rest of the loop, leaking later registrations and listeners.
- **Resolution**: Wrapped each `disposable.dispose()` call in a try/catch that logs the error and continues iterating.
#### FIXED — ZLS crashed immediately on startup when zig was discovered automatically
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L980)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L980)
- **Description**: `syncWorkspaceZlsConfiguration` was called immediately after
`client.start()`. Writing `zls.zig_exe_path` to `nova.workspace.config` while the
LanguageClient is newly registered triggers Nova to fire workspace config change events.
@@ -64,7 +64,7 @@ A living catalog of identified issues in this extension. New findings should be
#### FIXED — `findOnPath` always returned null when no explicit path was configured
-- **Location**: [Scripts/main.js](Scripts/main.js), `findOnPath` (~L597)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `findOnPath` (~L597)
- **Description**: Nova's `Process` constructor does not inherit the parent's environment
automatically. The bare `new Process("/usr/bin/env", { args: ["which", name] })` call
received a stripped environment with no `PATH`, so `which` could not locate any binary
@@ -74,19 +74,19 @@ A living catalog of identified issues in this extension. New findings should be
#### OPEN — `pushConfiguration` re-resolves Zig path on every observation
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.pushConfiguration` (~L1040)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.pushConfiguration` (~L1040)
- **Description**: On rapid config-change storms, each observation calls `resolveSettings → resolveExecutable → findOnPath`, fanning out N parallel `which zig` subprocesses. The generation guard discards stale results but the work still runs.
- **Suggested fix**: Debounce the call (small `setTimeout` consolidator) or cache the resolved zig path for the lifetime of a generation.
#### OPEN — `start()` cannot distinguish handshake failure from runtime crash
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L1000)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L1000)
- **Description**: The try/catch around `client.start()` only fires when `start()` itself throws synchronously. If the language client starts successfully but the LSP fails handshake, the user only sees the generic "stopped unexpectedly" warning (via `onDidStop`), never the more informative "Unable to start" message.
- **Suggested fix**: Listen for an early failure on `onDidStop` within a brief window after start and emit the more informative warning.
#### OPEN — `stop()` removes from subscriptions before `client.stop()` settles
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.stop` (~L1058)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.stop` (~L1058)
- **Description**: If `client.stop()` is async in Nova's API, removal happens before the stop completes. Not observed to cause issues in practice; flagging for verification.
- **Action**: Confirm whether Nova's `LanguageClient.stop()` is sync or returns a Promise; document the assumption either way.
@@ -94,31 +94,31 @@ A living catalog of identified issues in this extension. New findings should be
#### FIXED — `parseProjectName` matched commented-out `.name` lines
-- **Location**: [Scripts/main.js](Scripts/main.js), `parseProjectName`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `parseProjectName`
- **Description**: The regex did not strip line comments. A `build.zig.zon` with `// .name = "evil"` above the real name field would match the comment first.
- **Resolution**: Strip `//`-to-EOL comments via `content.replace(/\/\/[^\n]*/g, "")` before matching.
#### FIXED — Step discovery failures were invisible
-- **Location**: [Scripts/main.js](Scripts/main.js), `stepCache.fetch`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `stepCache.fetch`
- **Description**: When `--list-steps` failed (non-zero exit, timeout), the user saw a sparse task list with no signal that discovery was broken.
- **Resolution**: Added a per-cwd `lastWarnedAt` map that emits a `console.warn` with the exit status and a 500-char stderr excerpt at most once per 5 minutes. `invalidate()` clears the throttle so manual cache reset re-enables warnings.
#### OPEN — `resolveCleanAction` skips uninstall on cold cache
-- **Location**: [Scripts/main.js](Scripts/main.js), `resolveCleanAction` (~L1202)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanAction` (~L1202)
- **Description**: The `zig build uninstall` step only runs when `stepCache.entries` already contains the cwd and includes "uninstall". A first-time clean (cold cache) silently skips uninstall and only removes the cache directories.
-- **Suggested fix**: Either accept the asymmetry and document it in [MANUAL.md](MANUAL.md), or warm the cache before deciding.
+- **Suggested fix**: Either accept the asymmetry and document it in [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md), or warm the cache before deciding.
#### DOCUMENTED — `getConfigValue` treats empty string as missing
-- **Location**: [Scripts/main.js](Scripts/main.js), `getConfigValue` (~L62)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `getConfigValue` (~L62)
- **Description**: A user cannot explicitly *unset* a global value by setting an empty workspace override; they must use Nova's "remove from workspace" UI. Acceptable but worth a note for power users.
-- **Action**: Document in [MANUAL.md](MANUAL.md) settings reference.
+- **Action**: Document in [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) settings reference.
#### OPEN — Step name regex rejects names starting with a digit
-- **Location**: [Scripts/main.js](Scripts/main.js), `stepCache.fetch` filter (~L457) and `resolveBuildStepAction` (~L1290)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `stepCache.fetch` filter (~L457) and `resolveBuildStepAction` (~L1290)
- **Description**: `/^[A-Za-z_][\w-]*$/` is stricter than Zig itself, which accepts step names beginning with a digit.
- **Action**: Either relax to allow leading digits, or document as an extension-imposed constraint.
@@ -126,7 +126,7 @@ A living catalog of identified issues in this extension. New findings should be
#### FIXED — `resolveCleanAction` built its shell string by hand
-- **Location**: [Scripts/main.js](Scripts/main.js), `resolveCleanAction`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanAction`
- **Description**: The uninstall+rm command was assembled with manual `quoteShellArgument` calls. Safe today but fragile: a future contributor adding a new arg could forget to quote.
- **Resolution**: Compose the command from two `buildShellCommand` calls so all quoting routes through one helper. The intentional `;` (rather than `&&`) between the two is preserved and now commented — rm should run even if uninstall fails.
@@ -134,27 +134,27 @@ A living catalog of identified issues in this extension. New findings should be
#### OPEN — Class methods lack JSDoc
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer` (~L860) and `ZigTaskAssistant` (~L1080)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer` (~L860) and `ZigTaskAssistant` (~L1080)
- **Description**: ~50% of utility helpers have JSDoc; 0% of class methods do. The prior review plan tracks this work; see [~/.claude/plans/i-want-to-perform-idiopotent-crystal.md](file:///Users/david/.claude/plans/i-want-to-perform-idiopotent-crystal.md) for the backlog.
- **Action**: Resume Groups 1, 5, 5a, 6, 7 from the JSDoc plan.
#### DOCUMENTED — Auto-discovered tasks cannot open the report automatically
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigTaskAssistant.provideTasks`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigTaskAssistant.provideTasks`
- **Description**: Tasks returned from `provideTasks()` do not support the `openLogOnRun` setting. The property exists on task JSON files for user-created tasks but is not honoured on `Task` objects constructed in JS — setting `task.openLogOnRun` has no effect. Nova's extension API simply does not expose this for programmatic tasks.
-- **Action**: Documented in [README.md](README.md) Known Issues.
+- **Action**: Documented in [Zig.novaextension/README.md](Zig.novaextension/README.md) Known Issues.
#### OPEN — Issue matcher pattern not documented
-- **Location**: [extension.json](extension.json) issue matcher block, [README.md](README.md), [MANUAL.md](MANUAL.md)
+- **Location**: [Zig.novaextension/extension.json](Zig.novaextension/extension.json) issue matcher block, [Zig.novaextension/README.md](Zig.novaextension/README.md), [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md)
- **Description**: Users authoring custom tasks have no reference for what fields the `zig.compiler` matcher captures.
-- **Suggested fix**: Add a short "Issue matcher" section to [MANUAL.md](MANUAL.md) documenting the regex, captures, and how to use it from custom tasks.
+- **Suggested fix**: Add a short "Issue matcher" section to [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) documenting the regex, captures, and how to use it from custom tasks.
### Structure
#### OPEN — `Scripts/main.js` is a 1,398-line monolith
-- **Location**: [Scripts/main.js](Scripts/main.js)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js)
- **Description**: Section dividers help, but two ~300-line classes plus utilities in one file impede navigation and review.
- **Suggested split**:
- `main.js` — activate/deactivate/registerCommands
@@ -174,39 +174,59 @@ A living catalog of identified issues in this extension. New findings should be
#### OPEN — Dead `warnedMissing.delete("zig")`
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigLanguageServer.start` (~L1010)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigLanguageServer.start` (~L1010)
- **Description**: `warnMissingTool` is only ever called with `"zls"`; the matching `delete("zig")` line never has anything to remove.
- **Action**: Either delete the line, or wire up a missing-zig warning path through `warnMissingTool`.
-#### OPEN — `buildShellCommand` uses `;` between cd and command
+#### FIXED — `buildShellCommand` uses `;` between cd and command
-- **Location**: [Scripts/main.js](Scripts/main.js), `buildShellCommand` (~L735)
+
+
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `buildShellCommand`
- **Description**: If `cd` fails (e.g. cwd was deleted between resolution and execution), the command runs in the wrong directory silently.
-- **Suggested fix**: Use `&&` so a failed `cd` aborts.
+- **Resolution**: Changed `segments.join("; ")` to `segments.join(" && ")` so a failed `cd` aborts the command.
### Logic
-#### OPEN — `USER_OPTION_REGEX` permits trailing `=` with empty value
+#### FIXED — `localizeText` ignored its `fallback` parameter
+
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `localizeText` (~L542)
+- **Description**: When a localization key was missing, the function returned `"Localization missing for ${key}"` instead of the caller-supplied English fallback. All callers pass real English strings as fallbacks.
+- **Resolution**: Changed the missing-key branch to `return fallback !== undefined ? String(fallback) : key`.
+
+#### FIXED — Dead `normalized === ""` branch in `resolveCleanPaths`
+
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveCleanPaths` (~L307)
+- **Description**: `nova.path.normalize(cwd)` can never return `""` when `cwd` starts with `"/"` (already guaranteed by the preceding guard), making the empty-string check dead code.
+- **Resolution**: Removed `|| normalized === ""` from the condition.
+
+#### FIXED — Step validated after `await` in `resolveBuildStepAction`
+
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveBuildStepAction` (~L1420)
+- **Description**: `resolveZigExecutable()` (async, runs `which zig`) was called before validating the `step` argument, wasting a subprocess on invalid input.
+- **Resolution**: Moved the step null/regex guard to the top of the function, before the `await`.
+
+#### FIXED — `USER_OPTION_REGEX` permitted trailing `=` with empty value
-- **Location**: [Scripts/main.js](Scripts/main.js), `USER_OPTION_REGEX` (~L9)
-- **Description**: `foo=` is accepted and produces `-Dfoo=`. Zig's behavior on empty values is option-dependent.
-- **Action**: Tighten to require at least one character after `=`, or document the current behavior.
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `USER_OPTION_REGEX` (~L9)
+- **Description**: `foo=` was accepted and produced `-Dfoo=`. Zig errors on empty option values.
+- **Resolution**: Changed `=.*` to `=.+` so at least one character is required after `=`.
#### OPEN — Build step name has no length cap
-- **Location**: [Scripts/main.js](Scripts/main.js), `resolveBuildStepAction` (~L1290)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveBuildStepAction` (~L1290)
- **Description**: A 1MB step name would pass the regex check.
- **Suggested fix**: Add a length anchor like `{1,128}`.
-#### OPEN — `debounceMs` has no upper bound
+#### FIXED — `debounceMs` had no upper bound
-- **Location**: [Scripts/main.js](Scripts/main.js), `resolveWatchAction` (~L1325) and [extension.json](extension.json) `debounceMs` schema
-- **Description**: `Number.isFinite` and `>= 0` are checked, but `1e18` would be accepted. The manifest schema also lacks `min`/`max`.
-- **Suggested fix**: Cap at 60_000 in code; add `"min": 0, "max": 60000` in `extension.json`.
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `resolveWatchAction`; [Zig.novaextension/extension.json](Zig.novaextension/extension.json) `debounceMs` schema
+- **Description**: `Number.isFinite` and `>= 0` were checked, but `1e18` would be accepted.
+- **Resolution**: Added `&& n <= 60000` guard in code; added `"min": 0, "max": 60000` to the manifest schema.
#### OPEN — `runProcess` joins stdout/stderr lines without separator
-- **Location**: [Scripts/main.js](Scripts/main.js), `runProcess` (~L511)
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `runProcess` (~L511)
- **Description**: `stdout.push(line)` then `stdout.join("")`. If Nova's `onStdout` strips trailing newlines, two lines `"foo"` and `"bar"` concatenate to `"foobar"`.
- **Action**: Verify Nova's behavior; if newlines are stripped, join with `"\n"`.
@@ -214,13 +234,13 @@ A living catalog of identified issues in this extension. New findings should be
#### OPEN — `build-parser.sh` lacks explicit `xcrun` failure check
-- **Location**: [Scripts/build-parser.sh](Scripts/build-parser.sh) `SDKROOT` line
+- **Location**: [Zig.novaextension/Scripts/build-parser.sh](Zig.novaextension/Scripts/build-parser.sh) `SDKROOT` line
- **Description**: An empty `SDKROOT` from a failed `xcrun` produces cryptic downstream errors.
- **Suggested fix**: Check `xcrun --show-sdk-path` exit status explicitly and abort with a clear message.
#### OPEN — Hardening tip for untrusted workspaces missing from MANUAL
-- **Location**: [MANUAL.md](MANUAL.md)
+- **Location**: [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md)
- **Description**: Login-shell PATH discovery means a workspace (or a shell profile that reads
workspace-local `.envrc` / `direnv` / `.env` files) that injects `.` early in PATH could
run a malicious local `zig`. The same surface area exists for `createAction` (tasks). Already
@@ -231,26 +251,26 @@ A living catalog of identified issues in this extension. New findings should be
#### OPEN — README troubleshooting omits common tool-not-found cases
-- **Location**: [README.md](README.md) Troubleshooting
+- **Location**: [Zig.novaextension/README.md](Zig.novaextension/README.md) Troubleshooting
- **Description**: Mentions parser issues but not "ZLS not found" or "lldb-dap not found" recovery paths, which are the most common user-reported pain points.
- **Suggested fix**: Add brief recovery steps for both.
#### OPEN — CHANGELOG entries don't link to MANUAL sections
-- **Location**: [CHANGELOG.md](CHANGELOG.md)
-- **Description**: Polish: feature entries could deep-link to relevant [MANUAL.md](MANUAL.md) sections so users can discover usage.
+- **Location**: [Zig.novaextension/CHANGELOG.md](Zig.novaextension/CHANGELOG.md)
+- **Description**: Polish: feature entries could deep-link to relevant [Zig.novaextension/MANUAL.md](Zig.novaextension/MANUAL.md) sections so users can discover usage.
- **Action**: Optional.
### Structure
#### OPEN — Dual escape paths for shell strings
-- **Location**: [Scripts/main.js](Scripts/main.js), `buildShellCommand` and `buildIssueNormalizedCommand`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `buildShellCommand` and `buildIssueNormalizedCommand`
- **Description**: Both functions quote arguments but live separately. Future changes risk drift.
- **Suggested fix**: Compose `buildIssueNormalizedCommand` on top of `buildShellCommand`.
#### OPEN — `createAction` defined mid-class
-- **Location**: [Scripts/main.js](Scripts/main.js), `ZigTaskAssistant.createAction`
+- **Location**: [Zig.novaextension/Scripts/main.js](Zig.novaextension/Scripts/main.js), `ZigTaskAssistant.createAction`
- **Description**: Central helper used by every `resolve*Action`; defined between two of its callers. Discoverability nit.
- **Action**: Move to top of class above `resolveTaskAction`.