issue-254: Implement extensions/vision/src/config.ts (env resolution + defaults) #268

Merged

david merged 1 commit from feature/issue-254/implement-extensions-vision-src-config-ts-env-reso into main

2026-09-17 08:23:06 +00:00

david commented

2026-09-17 08:23:05 +00:00

Owner

254: Implement extensions/vision/src/config.ts (env resolution + defaults)

Closes #254 — 254.

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 13m 16s.

Review findings

I'll start by exploring the repository structure and understanding the context of these changes.

Let me examine the related files — the errors.ts this imports from, and the mongodb version this copies from, plus the existing test patterns.

Let me look at the implementation plan, the errors test (for test conventions), and how the mongodb extension uses this pattern.

Now let me check the Node version and run the tests. The vision tests aren't yet wired into the test script (that's a later milestone), so let me run them directly and also run the full suite.

All 20 vision config tests pass. Now let me run the full test suite and check for any lint setup.

All gates pass: the full repo suite reports 879 pass / 0 fail / 1 skip, and the 20 new vision config tests pass when run directly. There is no lint gate (no eslint/biome/prettier config, no lint script) — this repo runs .ts directly via node --test.

Let me get exact line numbers for the findings and confirm the test script doesn't yet cover the vision glob.

Let me ground the "integer vs float" docstring finding with a concrete behavior check.

I have everything I need. Here's my assessment:

Gates run:

Build: no build step exists (no tsc/tsconfig/build script) — N/A.
Test: full repo suite = 879 pass / 0 fail / 1 skip; the 20 new config.test.ts cases pass when run directly (node --test).
Lint: no lint gate exists (no eslint/biome/prettier config, no lint script) — N/A.

The module is small, well-tested, and correctly scoped to milestone M1. Findings are minor.

# Review Findings — 254: Implement extensions/vision/src/config.ts (env resolution + defaults)

## Summary

Small, well-scoped config module (env resolution, defaults, `.env` loading) with comprehensive unit tests. All gates that exist passed: the repo test suite is 879 pass / 0 fail / 1 skip and the 20 new `config.test.ts` cases pass; there is no build or lint gate in this repo. Two minor findings below — one contract mismatch between the function name/docstring and its behavior, and one process note about test-gate wiring.

## Critical

- (none)

## High

- (none)

## Medium

- extensions/vision/src/config.ts:80 — `resolvePositiveInt` is named `resolvePositiveInt` and its docstring says "Parse a positive finite **integer**", but the implementation only checks `Number.isFinite` and `> 0`, so fractional values pass through (verified: `VISION_MAX_TOKENS: "3.5"` → `maxTokens: 3.5`). That value flows into DeepSeek's integer `max_tokens`/`timeout` fields, so a fractional value would surface as a silent runtime problem rather than a config error. Suggested fix: enforce integrality with `if (!Number.isInteger(value)) throw new ToolError(...)` (and add a test), or rename the function to `resolvePositiveNumber` to match the non-integer behavior.

## Low

- extensions/vision/src/config.ts (and config.test.ts) — The new `config.test.ts` is not yet executed by the repo's `npm test` gate: the `test` script glob (`... extensions/penpot/src/tools/*.test.ts`) does not include `extensions/vision/**`, so these tests only run when invoked directly. This is expected by design (test-script wiring is a later milestone, M8 per the plan §11/§12), but M8 must add the vision glob so the config tests land in the standard gate. Suggested fix: when wiring M8, append `extensions/vision/src/*.test.ts` (and `extensions/vision/index.test.ts`) to the `test` script.

## 254: Implement extensions/vision/src/config.ts (env resolution + defaults) Closes #254 — [254](https://git.excelera.net/david/pi-extensions-and-skills/issues/254). > 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 13m 16s. ## Review findings I'll start by exploring the repository structure and understanding the context of these changes. Let me examine the related files — the `errors.ts` this imports from, and the `mongodb` version this copies from, plus the existing test patterns. Let me look at the implementation plan, the errors test (for test conventions), and how the mongodb extension uses this pattern. Now let me check the Node version and run the tests. The vision tests aren't yet wired into the `test` script (that's a later milestone), so let me run them directly and also run the full suite. All 20 vision config tests pass. Now let me run the full test suite and check for any lint setup. All gates pass: the full repo suite reports **879 pass / 0 fail / 1 skip**, and the 20 new vision config tests pass when run directly. There is **no lint gate** (no eslint/biome/prettier config, no lint script) — this repo runs `.ts` directly via `node --test`. Let me get exact line numbers for the findings and confirm the `test` script doesn't yet cover the vision glob. Let me ground the "integer vs float" docstring finding with a concrete behavior check. I have everything I need. Here's my assessment: **Gates run:** - **Build:** no build step exists (no `tsc`/tsconfig/build script) — N/A. - **Test:** full repo suite = **879 pass / 0 fail / 1 skip**; the 20 new `config.test.ts` cases pass when run directly (`node --test`). - **Lint:** no lint gate exists (no eslint/biome/prettier config, no `lint` script) — N/A. The module is small, well-tested, and correctly scoped to milestone M1. Findings are minor. ```markdown # Review Findings — 254: Implement extensions/vision/src/config.ts (env resolution + defaults) ## Summary Small, well-scoped config module (env resolution, defaults, `.env` loading) with comprehensive unit tests. All gates that exist passed: the repo test suite is 879 pass / 0 fail / 1 skip and the 20 new `config.test.ts` cases pass; there is no build or lint gate in this repo. Two minor findings below — one contract mismatch between the function name/docstring and its behavior, and one process note about test-gate wiring. ## Critical - (none) ## High - (none) ## Medium - extensions/vision/src/config.ts:80 — `resolvePositiveInt` is named `resolvePositiveInt` and its docstring says "Parse a positive finite **integer**", but the implementation only checks `Number.isFinite` and `> 0`, so fractional values pass through (verified: `VISION_MAX_TOKENS: "3.5"` → `maxTokens: 3.5`). That value flows into DeepSeek's integer `max_tokens`/`timeout` fields, so a fractional value would surface as a silent runtime problem rather than a config error. Suggested fix: enforce integrality with `if (!Number.isInteger(value)) throw new ToolError(...)` (and add a test), or rename the function to `resolvePositiveNumber` to match the non-integer behavior. ## Low - extensions/vision/src/config.ts (and config.test.ts) — The new `config.test.ts` is not yet executed by the repo's `npm test` gate: the `test` script glob (`... extensions/penpot/src/tools/*.test.ts`) does not include `extensions/vision/**`, so these tests only run when invoked directly. This is expected by design (test-script wiring is a later milestone, M8 per the plan §11/§12), but M8 must add the vision glob so the config tests land in the standard gate. Suggested fix: when wiring M8, append `extensions/vision/src/*.test.ts` (and `extensions/vision/index.test.ts`) to the `test` script. ```

david added 1 commit

2026-09-17 08:23:05 +00:00