issue-257: Implement extensions/vision/src/client.ts (request build, fetch, parse, error mapping) #271

Merged

david merged 2 commits from feature/issue-257/implement-extensions-vision-src-client-ts-request into main

2026-09-17 10:38:15 +00:00

david commented

2026-09-17 10:38:14 +00:00

Owner

257: Implement extensions/vision/src/client.ts (request build, fetch, parse, error mapping)

Closes #257 — 257.

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 42m 29s.

Review findings

I'll start by exploring the relevant files to understand the context, then apply fixes for the review findings.

Let me look at the sibling extension precedent and the project structure.

Let me check the test infrastructure and available type packages.

Now I understand the situation. Let me analyze the four findings:

Medium #1 (tsconfig/check): Vision has no package.json, no node_modules, no @types/node, and no root typescript. The check script would fail (no tsc available) — adding it without installable deps creates a broken gate. The reviewer themselves framed this as "expected until M8 wiring." I'll carry this forward as unresolved.
Medium #2 (timeout-any branch test): Feasible — add a live-signal + tiny-deadline test.
Low #3 (network-any branch test): Feasible — add a live-signal network test.
Low #4 (HINTS hoisting): Feasible — inline the single string.

Let me apply the code fixes first.

Now let me add the two new tests. First, the network-error test with a live signal (Low #3), inserted after the existing network test:

Now the timeout-via-AbortSignal.any test (Medium #2), inserted after the existing timeout test:

Now let me run the vision client tests to verify the new tests pass.

All 21 client tests pass (was 19, now +2). Let me run the full suite to confirm no regressions.

Full suite is green: 957 tests, 956 pass, 1 skipped (penpot gate), 0 fail (was 955, now +2 new tests). Let me verify the edited regions of client.ts are clean.

Let me do a final sanity typecheck of the edited client.ts using a throwaway tsconfig, to confirm the inline change is still type-clean.

That's just the .ts-extension import flag (a runtime-stripping detail, not a type error). Let me add that flag to confirm no real type errors.

All fixes are verified. client.ts is type-clean, and the full suite is green (957 tests, 956 pass, 1 skipped, 0 fail). Here is my final report.

Review Findings — issue-257: Implement extensions/vision/src/client.ts (request build, fetch, parse, error mapping)

Summary

Applied fixes for 3 of 4 findings in a single pass. Gates run: npm test (full suite) → 957 tests, 956 pass, 1 skipped (unrelated penpot gate requiring PENPOT_SOURCE_DIR), 0 fail (up from 955: +2 new tests); the 21 vision client.test.ts cases all pass. A throwaway tsc --noEmit on client.ts confirms it is type-clean after the edit.

Resolved:

Medium client.ts:216 — Added a test that passes a live AbortController signal with a tiny timeoutMs (20) against a never-resolving fetch, so AbortSignal.any fires the deadline (winning over pi's never-aborting signal) and classifyFetchError reads the TimeoutError off the combined signal's reason, yielding category timeout through the previously-untested AbortSignal.any + deadline path.
Low client.test.ts:367 — Added a network-error test that passes a live controller signal with a generous deadline, covering the AbortSignal.any + network branch (combined not aborted → network).
Low client.ts:229 — Inlined " — DeepSeek server error; retry the call" directly into toHttpError and removed the forward-referenced single-member HINTS object.

Carried forward (unresolved):

Medium extensions/vision/ (+ package.json) — Could not add a tsconfig.json + check (tsc --noEmit) script without creating a broken gate: vision has no package.json, no node_modules, no @types/node, and there is no root typescript binary. The check command would fail (no tsc), violating the keep-the-suite-green rule. This is a final-wiring (M8) concern, so it is left for that milestone.

Critical

(none)

High

(none)

Medium

extensions/vision/ (+ package.json) — The vision extension has no tsconfig.json and no check (tsc --noEmit) script, while all four sibling extensions that carry one — mongodb, pr-comments, forgejo, victorialogs — ship a tsconfig plus a "check": "tsc --noEmit" script. npm test runs the .ts through Node's native type-stripping, which only strips types, so a type error in client.ts would not be caught by any gate. Suggested fix: add a tsconfig.json + a check script for the vision extension (mirroring extensions/mongodb/tsconfig.json / extensions/mongodb/package.json) as part of final wiring, and wire it into npm test or CI.
extensions/vision/src/client.ts:216 — The AbortSignal.any deadline branch is never exercised by a passing test. The timeout test (client.test.ts:387) passes signal: undefined, which takes the combineSignals direct-return path (AbortSignal.timeout(timeoutMs), line 215); the abort test (client.test.ts:405) exercises AbortSignal.any but with pi's abort winning (60 s deadline never fires). The branch where AbortSignal.any is used and the deadline wins — i.e. timeout classified via the combined signal's reason in classifyFetchError (line 298) — is untested, and it is the core timeout-vs-abort classification logic. Suggested fix: add a test that passes a live AbortController signal with a tiny timeoutMs and a never-resolving fetch, asserting the deadline wins and yields category timeout through the AbortSignal.any path.

Medium

extensions/vision/src/client.test.ts:367 — The network-error test passes signal: undefined, so it does not cover the AbortSignal.any wrapper that combineSignals adds when a real signal is present. A network error with a live signal still classifies as network (combined.aborted is false in classifyFetchError, line 298), but that path is untested. Suggested fix: add a network-error test that passes a live controller signal to cover the AbortSignal.any + network branch.

Low

extensions/vision/src/client.ts:229 — HINTS (line 325) is a single-member object referenced before its declaration site (in toHttpError, line 229). It works because toHttpError is only called at runtime, well after module init (no temporal-dead-zone issue), but the indirection adds noise for one string. Suggested fix: inline " — DeepSeek server error; retry the call" directly, or move the HINTS declaration above toHttpError for readability.

## 257: Implement extensions/vision/src/client.ts (request build, fetch, parse, error mapping) Closes #257 — [257](https://git.excelera.net/david/pi-extensions-and-skills/issues/257). > 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 42m 29s. ## Review findings I'll start by exploring the relevant files to understand the context, then apply fixes for the review findings. Let me look at the sibling extension precedent and the project structure. Let me check the test infrastructure and available type packages. Now I understand the situation. Let me analyze the four findings: - **Medium #1 (tsconfig/check):** Vision has no `package.json`, no node_modules, no `@types/node`, and no root `typescript`. The `check` script would fail (no `tsc` available) — adding it without installable deps creates a broken gate. The reviewer themselves framed this as "expected until M8 wiring." I'll carry this forward as unresolved. - **Medium #2 (timeout-any branch test):** Feasible — add a live-signal + tiny-deadline test. - **Low #3 (network-any branch test):** Feasible — add a live-signal network test. - **Low #4 (HINTS hoisting):** Feasible — inline the single string. Let me apply the code fixes first. Now let me add the two new tests. First, the network-error test with a live signal (Low #3), inserted after the existing network test: Now the timeout-via-`AbortSignal.any` test (Medium #2), inserted after the existing timeout test: Now let me run the vision client tests to verify the new tests pass. All 21 client tests pass (was 19, now +2). Let me run the full suite to confirm no regressions. Full suite is green: **957 tests, 956 pass, 1 skipped (penpot gate), 0 fail** (was 955, now +2 new tests). Let me verify the edited regions of `client.ts` are clean. Let me do a final sanity typecheck of the edited `client.ts` using a throwaway tsconfig, to confirm the inline change is still type-clean. That's just the `.ts`-extension import flag (a runtime-stripping detail, not a type error). Let me add that flag to confirm no real type errors. All fixes are verified. `client.ts` is type-clean, and the full suite is green (957 tests, 956 pass, 1 skipped, 0 fail). Here is my final report. # Review Findings — issue-257: Implement extensions/vision/src/client.ts (request build, fetch, parse, error mapping) ## Summary Applied fixes for 3 of 4 findings in a single pass. **Gates run:** `npm test` (full suite) → 957 tests, **956 pass, 1 skipped (unrelated penpot gate requiring `PENPOT_SOURCE_DIR`), 0 fail** (up from 955: +2 new tests); the 21 vision `client.test.ts` cases all pass. A throwaway `tsc --noEmit` on `client.ts` confirms it is type-clean after the edit. **Resolved:** - **Medium `client.ts:216`** — Added a test that passes a live `AbortController` signal with a tiny `timeoutMs` (20) against a never-resolving fetch, so `AbortSignal.any` fires the deadline (winning over pi's never-aborting signal) and `classifyFetchError` reads the `TimeoutError` off the combined signal's reason, yielding category `timeout` through the previously-untested `AbortSignal.any` + deadline path. - **Low `client.test.ts:367`** — Added a network-error test that passes a live controller signal with a generous deadline, covering the `AbortSignal.any` + network branch (combined not aborted → `network`). - **Low `client.ts:229`** — Inlined `" — DeepSeek server error; retry the call"` directly into `toHttpError` and removed the forward-referenced single-member `HINTS` object. **Carried forward (unresolved):** - **Medium `extensions/vision/` (+ `package.json`)** — Could not add a `tsconfig.json` + `check` (`tsc --noEmit`) script without creating a broken gate: vision has no `package.json`, no `node_modules`, no `@types/node`, and there is no root `typescript` binary. The `check` command would fail (no `tsc`), violating the keep-the-suite-green rule. This is a final-wiring (M8) concern, so it is left for that milestone. ## Critical (none) ## High (none) ## Medium - [ ] `extensions/vision/` (+ `package.json`) — The vision extension has **no `tsconfig.json` and no `check` (`tsc --noEmit`) script**, while all four sibling extensions that carry one — `mongodb`, `pr-comments`, `forgejo`, `victorialogs` — ship a tsconfig plus a `"check": "tsc --noEmit"` script. `npm test` runs the `.ts` through Node's native type-stripping, which only strips types, so a type error in `client.ts` would **not** be caught by any gate. Suggested fix: add a `tsconfig.json` + a `check` script for the vision extension (mirroring `extensions/mongodb/tsconfig.json` / `extensions/mongodb/package.json`) as part of final wiring, and wire it into `npm test` or CI. - [x] `extensions/vision/src/client.ts:216` — The `AbortSignal.any` deadline branch is **never exercised by a passing test**. The timeout test (`client.test.ts:387`) passes `signal: undefined`, which takes the `combineSignals` direct-return path (`AbortSignal.timeout(timeoutMs)`, line 215); the abort test (`client.test.ts:405`) exercises `AbortSignal.any` but with pi's abort *winning* (60 s deadline never fires). The branch where `AbortSignal.any` is used **and the deadline wins** — i.e. timeout classified via the combined signal's `reason` in `classifyFetchError` (line 298) — is untested, and it is the core timeout-vs-abort classification logic. Suggested fix: add a test that passes a live `AbortController` signal with a tiny `timeoutMs` and a never-resolving fetch, asserting the deadline wins and yields category `timeout` through the `AbortSignal.any` path. ## Medium - [x] `extensions/vision/src/client.test.ts:367` — The network-error test passes `signal: undefined`, so it does not cover the `AbortSignal.any` wrapper that `combineSignals` adds when a real signal is present. A network error with a live signal still classifies as `network` (`combined.aborted` is false in `classifyFetchError`, line 298), but that path is untested. Suggested fix: add a network-error test that passes a live controller signal to cover the `AbortSignal.any` + network branch. ## Low - [x] `extensions/vision/src/client.ts:229` — `HINTS` (line 325) is a single-member object referenced before its declaration site (in `toHttpError`, line 229). It works because `toHttpError` is only called at runtime, well after module init (no temporal-dead-zone issue), but the indirection adds noise for one string. Suggested fix: inline `" — DeepSeek server error; retry the call"` directly, or move the `HINTS` declaration above `toHttpError` for readability.

david added 2 commits

2026-09-17 10:38:14 +00:00