issue-255: Implement extensions/vision/src/images.ts (read, validate, data URLs) #269

Merged

david merged 2 commits from feature/issue-255/implement-extensions-vision-src-images-ts-read-val into main

2026-09-17 09:25:13 +00:00

david commented

2026-09-17 09:25:12 +00:00

Owner

255: Implement extensions/vision/src/images.ts (read, validate, data URLs)

Closes #255 — 255.

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 1h 02m 04s.

Review findings

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

Let me check the exact line numbers referenced in the findings and look at the errors module for consistency.

All line numbers are confirmed. Let me verify the image-size package exports to confirm finding #4's claim.

Confirmed. Let me check what the root index.d.ts actually exports to pick the right fix.

All findings confirmed. Let me check the vision directory for all test files to wire them correctly, and confirm the errors precedent.

Now I'll check for any TypeScript configuration before making edits (to know whether a type-check gate exists).

No tsconfig and no tsc — the only gate is node --test with type-stripping. Now I'll apply all four fixes. They're independent, so I'll batch the edits.

Let me verify the images.ts edits rendered correctly — I need to double-check the catch block's template literal quoting.

Confirmed a syntax error — the closing backtick got replaced with a ". Let me fix it.

Let me fix the exact bytes with a precise Python replacement.

Let me verify the fix and check the whole file parses correctly with Node's type-stripping.

Both files parse. Let me verify the test file's skip edit rendered correctly.

The structure is correct but the indentation is inconsistent. Let me re-indent that block cleanly.

Now let me verify the package.json edit and confirm the test file syntax, then run the full test gate.

All edits applied and syntax is valid. Now let me run the vision tests directly, then the full npm test gate.

All 43 vision tests pass (the unreadable test ran since we're not root). Now the full npm test gate.

The full gate passes: 922 pass / 1 skip / 0 fail — up from 879 pass, with all 43 vision tests now wired in and passing. Let me review the complete working-tree diff to confirm everything is clean.

All four findings are resolved, the working-tree diff is minimal and consistent with the surrounding code, and the full gate passes (922 pass / 1 skip / 0 fail — up from 879, with all 43 vision tests now wired in). Here is the annotated findings document.

Review Findings — issue-255

Summary

All four findings were resolved in a single pass. (1) High: appended extensions/vision/src/*.test.ts to scripts.test, so npm test now runs the vision suite — full gate is 922 pass / 1 skip / 0 fail (was 879 pass), the 43 new vision tests included. (2) Medium: moved fs.promises.readFile(resolved) inside the access(R_OK) try/catch so any TOCTOU/IO failure rethrows a ToolError(..., "image") instead of a native ErrnoException. (3) Low: dropped the unexported import type { ISizeCalculationResult } and the explicit annotation, letting TypeScript infer imageSize()'s return type. (4) Low: the unreadable-file test now skips via { skip: process.getuid?.() === 0 }. Nothing left unresolved; no regressions.

Critical

(none)

High

package.json:32 — The vision test files are not wired into the test script, so npm test (and CI) never runs images.test.ts (nor config.test.ts / errors.test.ts from the prior milestones). This defeats the gate that is supposed to protect the new module, and contradicts IMPLEMENTATION_PLAN §11 / M8, which lists appending the vision globs to scripts.test as a wiring step. Suggested fix: append the vision globs, e.g. extend the test script with extensions/vision/index.test.ts extensions/vision/src/*.test.ts.

Medium

extensions/vision/src/images.ts:176 — fs.promises.readFile(resolved) is not wrapped in a try/catch. The access(R_OK) check at line 171 precedes it, but a TOCTOU permission change or a raw I/O error between the two would surface a native NodeJS.ErrnoException instead of a ToolError(category === "image"), violating the module's documented contract ("Every failure is a ToolError with category === "image""). Suggested fix: move readFile inside the access try/catch (or its own try/catch) that rethrows a ToolError(..., "image").

Low

extensions/vision/src/images.ts:29 — import type { ISizeCalculationResult } from "image-size" imports a type that is not exported from the package root (image-size v2 root only exports imageSize / disableTypes / types; ISizeCalculationResult lives in a subpath). It only works because Node's type-stripping drops type-only imports at runtime without resolving them; a real tsc build would hard-fail with "no exported member". Suggested fix: drop the explicit let detected: ISizeCalculationResult annotation and let TypeScript infer the return type of imageSize() (removing the broken import), or import from the correct subpath.
extensions/vision/src/images.test.ts:226 — The "unreadable file" test blocks reads with fs.chmodSync(file, 0), but a root user bypasses DAC read checks, so access(R_OK) and readFile would succeed and the assertion would fail if the suite runs as root (e.g. some CI runners). Suggested fix: skip that test when process.getuid?.() === 0, or use a non-root mechanism to make the file unreadable.

Low

Empty sections

(No findings at the Critical level; the section is intentionally present but empty.)

## 255: Implement extensions/vision/src/images.ts (read, validate, data URLs) Closes #255 — [255](https://git.excelera.net/david/pi-extensions-and-skills/issues/255). > 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 1h 02m 04s. ## Review findings I'll start by exploring the repository structure and understanding the current state of the relevant files. Let me check the exact line numbers referenced in the findings and look at the errors module for consistency. All line numbers are confirmed. Let me verify the `image-size` package exports to confirm finding #4's claim. Confirmed. Let me check what the root `index.d.ts` actually exports to pick the right fix. All findings confirmed. Let me check the vision directory for all test files to wire them correctly, and confirm the errors precedent. Now I'll check for any TypeScript configuration before making edits (to know whether a type-check gate exists). No tsconfig and no tsc — the only gate is `node --test` with type-stripping. Now I'll apply all four fixes. They're independent, so I'll batch the edits. Let me verify the images.ts edits rendered correctly — I need to double-check the catch block's template literal quoting. Confirmed a syntax error — the closing backtick got replaced with a `"`. Let me fix it. Let me fix the exact bytes with a precise Python replacement. Let me verify the fix and check the whole file parses correctly with Node's type-stripping. Both files parse. Let me verify the test file's skip edit rendered correctly. The structure is correct but the indentation is inconsistent. Let me re-indent that block cleanly. Now let me verify the package.json edit and confirm the test file syntax, then run the full test gate. All edits applied and syntax is valid. Now let me run the vision tests directly, then the full `npm test` gate. All 43 vision tests pass (the unreadable test ran since we're not root). Now the full `npm test` gate. The full gate passes: **922 pass / 1 skip / 0 fail** — up from 879 pass, with all 43 vision tests now wired in and passing. Let me review the complete working-tree diff to confirm everything is clean. All four findings are resolved, the working-tree diff is minimal and consistent with the surrounding code, and the full gate passes (922 pass / 1 skip / 0 fail — up from 879, with all 43 vision tests now wired in). Here is the annotated findings document. # Review Findings — issue-255 ## Summary All four findings were resolved in a single pass. (1) High: appended `extensions/vision/src/*.test.ts` to `scripts.test`, so `npm test` now runs the vision suite — full gate is 922 pass / 1 skip / 0 fail (was 879 pass), the 43 new vision tests included. (2) Medium: moved `fs.promises.readFile(resolved)` inside the `access(R_OK)` `try`/`catch` so any TOCTOU/IO failure rethrows a `ToolError(..., "image")` instead of a native `ErrnoException`. (3) Low: dropped the unexported `import type { ISizeCalculationResult }` and the explicit annotation, letting TypeScript infer `imageSize()`'s return type. (4) Low: the unreadable-file test now skips via `{ skip: process.getuid?.() === 0 }`. Nothing left unresolved; no regressions. ## Critical - (none) ## High - [x] `package.json`:32 — The vision test files are not wired into the `test` script, so `npm test` (and CI) never runs `images.test.ts` (nor `config.test.ts` / `errors.test.ts` from the prior milestones). This defeats the gate that is supposed to protect the new module, and contradicts IMPLEMENTATION_PLAN §11 / M8, which lists appending the vision globs to `scripts.test` as a wiring step. Suggested fix: append the vision globs, e.g. extend the `test` script with `extensions/vision/index.test.ts extensions/vision/src/*.test.ts`. ## Medium - [x] `extensions/vision/src/images.ts`:176 — `fs.promises.readFile(resolved)` is not wrapped in a `try/catch`. The `access(R_OK)` check at line 171 precedes it, but a TOCTOU permission change or a raw I/O error between the two would surface a native `NodeJS.ErrnoException` instead of a `ToolError(category === "image")`, violating the module's documented contract ("Every failure is a `ToolError` with `category === "image"`"). Suggested fix: move `readFile` inside the `access` `try`/`catch` (or its own `try`/`catch`) that rethrows a `ToolError(..., "image")`. ## Low - [x] `extensions/vision/src/images.ts`:29 — `import type { ISizeCalculationResult } from "image-size"` imports a type that is **not** exported from the package root (`image-size` v2 root only exports `imageSize` / `disableTypes` / `types`; `ISizeCalculationResult` lives in a subpath). It only works because Node's type-stripping drops type-only imports at runtime without resolving them; a real `tsc` build would hard-fail with "no exported member". Suggested fix: drop the explicit `let detected: ISizeCalculationResult` annotation and let TypeScript infer the return type of `imageSize()` (removing the broken import), or import from the correct subpath. - [x] `extensions/vision/src/images.test.ts`:226 — The "unreadable file" test blocks reads with `fs.chmodSync(file, 0)`, but a root user bypasses DAC read checks, so `access(R_OK)` and `readFile` would succeed and the assertion would fail if the suite runs as root (e.g. some CI runners). Suggested fix: skip that test when `process.getuid?.() === 0`, or use a non-root mechanism to make the file unreadable. ## Low ## Empty sections - (No findings at the Critical level; the section is intentionally present but empty.)

david added 2 commits

2026-09-17 09:25:12 +00:00