issue-253: Implement extensions/vision/src/errors.ts (ToolError + categories) #267

Merged

david merged 2 commits from feature/issue-253/implement-extensions-vision-src-errors-ts-toolerro into main

2026-09-17 08:09:46 +00:00

david commented

2026-09-17 08:09:45 +00:00

Owner

253: Implement extensions/vision/src/errors.ts (ToolError + categories)

Closes #253 — 253.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 16m 33s.

Review findings

I'll start by exploring the repository structure and understanding the current state of the relevant files.

Now let me look at the package.json test script, the IMPLEMENTATION_PLAN.md, and check the sibling extensions' structure.

Let me check the IMPLEMENTATION_PLAN.md §11 and the vision extension structure.

I now have a clear picture. Let me analyze the three findings against the implementation plan:

Medium (closed-set test at errors.test.ts:54): directly about the code in this diff (M2). Fixable and in scope.
Low (tsconfig.json): the plan §14 M8 is "wiring"; this diff is intentionally scoped to M2. Defer.
Low (package.json test script): the plan §11 and §14 M8 explicitly cover the test script wiring. Defer.

Let me apply the Medium fix — making TOOL_ERROR_CATEGORIES the single source of truth and closing the test in both directions.

Now let me run the test to confirm it passes.

All 5 tests pass. Let me verify the fix actually closes the "extra literal" direction the reviewer flagged, using a throwaway copy so I don't disturb the working tree.

The extra-literal case now fails finding #5 (previously it would have passed). Let me confirm the "missing literal" direction fails to compile too, then clean up.

The verification confirms the fix works: adding an extra literal now makes finding #5 fail (previously it passed silently). The single-source-of-truth means the union can no longer silently grow. Let me verify the final files.

Both files are correct. Let me confirm I only modified the two intended files and that the working tree is clean otherwise.

Only the two intended files changed; the verification was done in /tmp (now cleaned up). My remediation is complete:

Medium (errors.test.ts:54): Resolved — made TOOL_ERROR_CATEGORIES the single source of truth (export const ... as const + export type ToolErrorCategory = typeof TOOL_ERROR_CATEGORIES[number]) and closed the test in both directions (categories.length === TOOL_ERROR_CATEGORIES.length + membership check). Verified that adding an extra literal now fails the test (previously passed silently).
Low (tsconfig.json) and Low (package.json test wiring): Left unresolved — both are M8 "wiring" per IMPLEMENTATION_PLAN.md §11/§14; this diff is intentionally scoped to M2.

Review Findings — 253

Summary

The change adds extensions/vision/src/errors.ts (a ToolError class + a 10-member ToolErrorCategory union) and its test file, mirroring the extensions/mongodb/src/errors.ts precedent. The vision test file runs green directly (node --test extensions/vision/src/errors.test.ts → 5/5 pass), but there is no build or lint gate in the repo, the vision extension has no tsconfig.json (unlike every sibling extension), and the new test is not wired into npm test — so the only gate that actually executed was the manual node --test run against the new file, which passed. The doc comment's claim about the runtime's "Signaling errors" contract is accurate (verified against the pinned pi-coding-agent runtime at docs/extensions.md).

Remediation (this stage): the Medium closed-set finding was resolved by promoting TOOL_ERROR_CATEGORIES to the single source of truth and closing the test in both directions (verified: an added literal now fails the test). The two Low findings (typecheck gate + npm test wiring) are M8 "wiring" per IMPLEMENTATION_PLAN.md §11/§14 and were left for that milestone since this diff is scoped to M2.

Critical

(none)

High

(none)

Medium

extensions/vision/src/errors.test.ts:54 — The "closed set" test does not actually verify closure. The comment (line 55–56) claims "a missing or extra literal would fail to compile," but that is only half true: removing a literal from the union breaks the test, while adding a literal to the union does not — the array still compiles and length is still 10, so the union can silently grow and this test will never catch it. Suggested fix: make the category list the single source of truth, e.g. export const TOOL_ERROR_CATEGORIES = ["config", "image", "http", "auth", "rate-limit", "network", "timeout", "aborted", "response", "unexpected"] as const; export type ToolErrorCategory = typeof TOOL_ERROR_CATEGORIES[number]; and have the test assert categories.length === TOOL_ERROR_CATEGORIES.length and that every literal is present in TOOL_ERROR_CATEGORIES, so the union and the test stay in lockstep in both directions.

Low

extensions/vision/src/errors.ts:19 — The vision extension has no tsconfig.json, whereas every sibling extension (mongodb, pr-comments, forgejo, victorialogs) ships one with a tsc --noEmit check. There is therefore no typecheck gate for vision. Suggested fix: add a tsconfig.json mirroring extensions/mongodb/tsconfig.json so tsc --noEmit covers vision (defer to the M8 wiring milestone if this diff is intentionally milestone-scoped per IMPLEMENTATION_PLAN.md §11).
package.json — The new extensions/vision/src/*.test.ts is not wired into the root test script, so this milestone's tests don't run in the standard gate. Suggested fix: append extensions/vision/src/*.test.ts to the test script glob (or defer to M8 wiring per IMPLEMENTATION_PLAN.md §11).

(none)

## 253: Implement extensions/vision/src/errors.ts (ToolError + categories) Closes #253 — [253](https://git.excelera.net/david/pi-extensions-and-skills/issues/253). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 16m 33s. ## Review findings I'll start by exploring the repository structure and understanding the current state of the relevant files. Now let me look at the package.json test script, the IMPLEMENTATION_PLAN.md, and check the sibling extensions' structure. Let me check the IMPLEMENTATION_PLAN.md §11 and the vision extension structure. I now have a clear picture. Let me analyze the three findings against the implementation plan: - **Medium** (closed-set test at `errors.test.ts:54`): directly about the code in this diff (M2). Fixable and in scope. - **Low** (`tsconfig.json`): the plan §14 M8 is "wiring"; this diff is intentionally scoped to M2. Defer. - **Low** (`package.json` test script): the plan §11 and §14 M8 explicitly cover the `test` script wiring. Defer. Let me apply the Medium fix — making `TOOL_ERROR_CATEGORIES` the single source of truth and closing the test in both directions. Now let me run the test to confirm it passes. All 5 tests pass. Let me verify the fix actually closes the "extra literal" direction the reviewer flagged, using a throwaway copy so I don't disturb the working tree. The extra-literal case now fails finding #5 (previously it would have passed). Let me confirm the "missing literal" direction fails to compile too, then clean up. The verification confirms the fix works: adding an extra literal now makes finding #5 fail (previously it passed silently). The single-source-of-truth means the union can no longer silently grow. Let me verify the final files. Both files are correct. Let me confirm I only modified the two intended files and that the working tree is clean otherwise. Only the two intended files changed; the verification was done in `/tmp` (now cleaned up). My remediation is complete: - **Medium** (`errors.test.ts:54`): **Resolved** — made `TOOL_ERROR_CATEGORIES` the single source of truth (`export const ... as const` + `export type ToolErrorCategory = typeof TOOL_ERROR_CATEGORIES[number]`) and closed the test in both directions (`categories.length === TOOL_ERROR_CATEGORIES.length` + membership check). Verified that adding an extra literal now fails the test (previously passed silently). - **Low** (`tsconfig.json`) and **Low** (`package.json` test wiring): **Left unresolved** — both are M8 "wiring" per `IMPLEMENTATION_PLAN.md` §11/§14; this diff is intentionally scoped to M2. # Review Findings — 253 ## Summary The change adds `extensions/vision/src/errors.ts` (a `ToolError` class + a 10-member `ToolErrorCategory` union) and its test file, mirroring the `extensions/mongodb/src/errors.ts` precedent. The vision test file runs green directly (`node --test extensions/vision/src/errors.test.ts` → 5/5 pass), but there is **no build or lint gate** in the repo, the vision extension has **no `tsconfig.json`** (unlike every sibling extension), and the new test is **not wired into `npm test`** — so the only gate that actually executed was the manual `node --test` run against the new file, which passed. The doc comment's claim about the runtime's "Signaling errors" contract is accurate (verified against the pinned `pi-coding-agent` runtime at `docs/extensions.md`). Remediation (this stage): the Medium closed-set finding was resolved by promoting `TOOL_ERROR_CATEGORIES` to the single source of truth and closing the test in both directions (verified: an added literal now fails the test). The two Low findings (typecheck gate + `npm test` wiring) are M8 "wiring" per `IMPLEMENTATION_PLAN.md` §11/§14 and were left for that milestone since this diff is scoped to M2. ## Critical - [ ] (none) ## High - [ ] (none) ## Medium - [x] `extensions/vision/src/errors.test.ts:54` — The "closed set" test does not actually verify closure. The comment (line 55–56) claims "a missing or extra literal would fail to compile," but that is only half true: **removing** a literal from the union breaks the test, while **adding** a literal to the union does not — the array still compiles and `length` is still 10, so the union can silently grow and this test will never catch it. Suggested fix: make the category list the single source of truth, e.g. `export const TOOL_ERROR_CATEGORIES = ["config", "image", "http", "auth", "rate-limit", "network", "timeout", "aborted", "response", "unexpected"] as const; export type ToolErrorCategory = typeof TOOL_ERROR_CATEGORIES[number];` and have the test assert `categories.length === TOOL_ERROR_CATEGORIES.length` and that every literal is present in `TOOL_ERROR_CATEGORIES`, so the union and the test stay in lockstep in both directions. ## Low - [ ] `extensions/vision/src/errors.ts:19` — The vision extension has no `tsconfig.json`, whereas every sibling extension (`mongodb`, `pr-comments`, `forgejo`, `victorialogs`) ships one with a `tsc --noEmit` check. There is therefore no typecheck gate for vision. Suggested fix: add a `tsconfig.json` mirroring `extensions/mongodb/tsconfig.json` so `tsc --noEmit` covers vision (defer to the M8 wiring milestone if this diff is intentionally milestone-scoped per `IMPLEMENTATION_PLAN.md` §11). - [ ] `package.json` — The new `extensions/vision/src/*.test.ts` is not wired into the root `test` script, so this milestone's tests don't run in the standard gate. Suggested fix: append `extensions/vision/src/*.test.ts` to the `test` script glob (or defer to M8 wiring per `IMPLEMENTATION_PLAN.md` §11). ## - [ ] (none)

david added 2 commits

2026-09-17 08:09:45 +00:00