issue-139: Implement src/errors.ts with unit tests (ToolError + toToolError) #156

Merged

david merged 3 commits from feature/issue-139/implement-src-errors-ts-with-unit-tests-toolerror into main

2026-09-01 00:13:33 +00:00

david commented

2026-09-01 00:13:32 +00:00

Owner

139: Implement src/errors.ts with unit tests (ToolError + toToolError)

Implements 139.

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 11m 09s.

Review findings

Now I'll apply the fixes. Starting with the code changes:Now the test additions. First the host-only branch test in errors.test.ts:Let me verify how bun's toMatchObject behaves with Error instances before writing the assertions:toMatchObject works on rejected ToolErrors. Now the tool-level tests:Now let me verify the edits and run the gates:All references renamed. Now run the gates — first verify the lockfile sync, then tests and typecheck:Lockfile is in sync. Now the test suite and typecheck:Extension gates pass (78/78 tests, tsc clean). Now the root gate:All gates pass. Let me review the final diff to confirm coherence:All gates pass and the diff is coherent. Here is the annotated findings report.

Review Findings — 139

Summary

Resolved all Medium findings and all but one Low finding in a single pass. Medium 1 (undeclared bson dependency): declared "bson": "^7.2.0" in extensions/mongodb/package.json (matching mongodb's range), synced bun.lock (verified no-op via bun install), and hardened the check with an isBsonError() helper that falls back to the driver's duck-typed BSONError.isBSONError guard so duplicate bson copies can't mis-categorize. Medium 2: dropped the dead db field from ToolErrorContext and updated its doc comment. Medium 3: every tool-level error test now asserts category (and name/message) at the tool boundary — find network+auth, count server+network+auth, listCollections network+auth. Low 1: added the SRV host-only (no-port) toToolError message test. Low 2: renamed describeEndpoint → describeContext in client.ts, index.ts, and index.test.ts. Low 3 remains unresolved: the commit-message convention finding cannot be remediated here — this stage is forbidden from committing, and flipping the repo-wide AGENTS.md convention to bless the 139: shorthand is a policy change beyond this code issue's scope.

Gates re-run: bun test (extensions/mongodb) passed 78/78, bun run check (tsc --noEmit) passed, root npm test passed 58/58. No lint gate exists in the repo.

Critical

(none)

High

(none)

Medium

extensions/mongodb/src/errors.ts:1 — BSONError is imported from "bson", but extensions/mongodb/package.json does not declare bson as a dependency (it resolves only via mongodb's transitive bson ^7.2.0). The new err instanceof BSONError check is a hard dependency on class identity: if the extension's direct "bson" import ever resolves to a different copy than the one producing the error (version skew, duplicate install), bad EJSON input would silently mis-categorize as unexpected instead of invalid-query. Suggested fix: declare "bson": "^7.2.0" (matching mongodb's range) in extensions/mongodb/package.json dependencies so the direct import and driver share one pinned copy, and/or make the check robust with a fallback like err instanceof BSONError || (err as {name?: string}).name === "BSONError".
extensions/mongodb/src/errors.ts:49 — ToolErrorContext.db is declared and documented ("host/port/db are parsed from the URI by the caller … used to shape error messages"), but it is never consumed by formatEndpoint or any message builder, and no caller ever populates it (describeEndpoint only sets host/port). This is dead API surface whose docstring promises behavior that does not exist. Suggested fix: either drop db from the interface, or use it (e.g. include the database in the connection-failure message) and populate it in describeEndpoint/the factory — and update the doc comment to match reality.
extensions/mongodb/src/tools/find.test.ts:101 (also count.test.ts:59, listCollections.test.ts:69) — the tool-level error tests assert only the thrown ToolError type or message substring and never assert the new category field at the tool boundary. Since the category is the headline new behavior of this issue and the tools are the only production path that forwards ToolErrorContext, a regression that threads the wrong context or constructs a ToolError with the wrong category would not be caught outside errors.test.ts. Suggested fix: extend the tool-level error assertions to check category (e.g. rejects.toMatchObject({ category: "connection" }) / { category: "auth" }) for at least the network and code-18 cases in each tool's tests.

Low

extensions/mongodb/src/errors.test.ts:186 — the formatEndpoint host-only branch (SRV path, { host } with no port) has no direct test at the toToolError message level; only the empty-context placeholder case is covered. A regression that renders host:undefined or drops the host would go unnoticed. Suggested fix: add a case asserting capture(new MongoNetworkError("x"), { host: "cluster0.example.net" }) produces Could not reach MongoDB at cluster0.example.net — … (no :undefined, no port).
extensions/mongodb/src/client.ts:50 — describeEndpoint now returns a ToolErrorContext object rather than an endpoint string, so its name and the "host/port description of the client's configured endpoint" doc are stale and slightly misleading (the function no longer describes an endpoint). Suggested fix: rename to describeContext (and update index.ts:20/index.test.ts references) so the API name matches what it returns.
AGENTS.md:26 — the commit message for this change (139: Implement src/errors.ts …) omits the issue- prefix required by the standard's stated format issue-<N>: <summary>. The repo's own prior commits (135–138) use the same shorthand, so this is a precedent-aligned deviation, but the standard is explicit. Suggested fix: use issue-139: <summary> for future commits in this series (or update AGENTS.md if the shorthand is the intended convention).

Documentation

Updated files:

README.md

## 139: Implement src/errors.ts with unit tests (ToolError + toToolError) Implements [139](https://git.excelera.net/david/pi-extensions-and-skills/issues/139). > 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 11m 09s. ## Review findings Now I'll apply the fixes. Starting with the code changes:Now the test additions. First the host-only branch test in errors.test.ts:Let me verify how bun's `toMatchObject` behaves with Error instances before writing the assertions:`toMatchObject` works on rejected `ToolError`s. Now the tool-level tests:Now let me verify the edits and run the gates:All references renamed. Now run the gates — first verify the lockfile sync, then tests and typecheck:Lockfile is in sync. Now the test suite and typecheck:Extension gates pass (78/78 tests, tsc clean). Now the root gate:All gates pass. Let me review the final diff to confirm coherence:All gates pass and the diff is coherent. Here is the annotated findings report. # Review Findings — 139 ## Summary Resolved all Medium findings and all but one Low finding in a single pass. **Medium 1** (undeclared `bson` dependency): declared `"bson": "^7.2.0"` in `extensions/mongodb/package.json` (matching mongodb's range), synced `bun.lock` (verified no-op via `bun install`), and hardened the check with an `isBsonError()` helper that falls back to the driver's duck-typed `BSONError.isBSONError` guard so duplicate `bson` copies can't mis-categorize. **Medium 2**: dropped the dead `db` field from `ToolErrorContext` and updated its doc comment. **Medium 3**: every tool-level error test now asserts `category` (and `name`/message) at the tool boundary — `find` network+auth, `count` server+network+auth, `listCollections` network+auth. **Low 1**: added the SRV host-only (no-port) `toToolError` message test. **Low 2**: renamed `describeEndpoint` → `describeContext` in `client.ts`, `index.ts`, and `index.test.ts`. **Low 3 remains unresolved**: the commit-message convention finding cannot be remediated here — this stage is forbidden from committing, and flipping the repo-wide `AGENTS.md` convention to bless the `139:` shorthand is a policy change beyond this code issue's scope. Gates re-run: `bun test` (extensions/mongodb) **passed** 78/78, `bun run check` (`tsc --noEmit`) **passed**, root `npm test` **passed** 58/58. No lint gate exists in the repo. ## Critical - [ ] (none) ## High - [ ] (none) ## Medium - [x] `extensions/mongodb/src/errors.ts:1` — `BSONError` is imported from `"bson"`, but `extensions/mongodb/package.json` does not declare `bson` as a dependency (it resolves only via mongodb's transitive `bson ^7.2.0`). The new `err instanceof BSONError` check is a hard dependency on class identity: if the extension's direct `"bson"` import ever resolves to a different copy than the one producing the error (version skew, duplicate install), bad EJSON input would silently mis-categorize as `unexpected` instead of `invalid-query`. Suggested fix: declare `"bson": "^7.2.0"` (matching mongodb's range) in `extensions/mongodb/package.json` dependencies so the direct import and driver share one pinned copy, and/or make the check robust with a fallback like `err instanceof BSONError || (err as {name?: string}).name === "BSONError"`. - [x] `extensions/mongodb/src/errors.ts:49` — `ToolErrorContext.db` is declared and documented ("`host`/`port`/`db` are parsed from the URI by the caller … used to shape error messages"), but it is never consumed by `formatEndpoint` or any message builder, and no caller ever populates it (`describeEndpoint` only sets host/port). This is dead API surface whose docstring promises behavior that does not exist. Suggested fix: either drop `db` from the interface, or use it (e.g. include the database in the connection-failure message) and populate it in `describeEndpoint`/the factory — and update the doc comment to match reality. - [x] `extensions/mongodb/src/tools/find.test.ts:101` (also `count.test.ts:59`, `listCollections.test.ts:69`) — the tool-level error tests assert only the thrown `ToolError` type or message substring and never assert the new `category` field at the tool boundary. Since the category is the headline new behavior of this issue and the tools are the only production path that forwards `ToolErrorContext`, a regression that threads the wrong context or constructs a `ToolError` with the wrong category would not be caught outside `errors.test.ts`. Suggested fix: extend the tool-level error assertions to check `category` (e.g. `rejects.toMatchObject({ category: "connection" })` / `{ category: "auth" }`) for at least the network and code-18 cases in each tool's tests. ## Low - [x] `extensions/mongodb/src/errors.test.ts:186` — the `formatEndpoint` host-only branch (SRV path, `{ host }` with no port) has no direct test at the `toToolError` message level; only the empty-context placeholder case is covered. A regression that renders `host:undefined` or drops the host would go unnoticed. Suggested fix: add a case asserting `capture(new MongoNetworkError("x"), { host: "cluster0.example.net" })` produces `Could not reach MongoDB at cluster0.example.net — …` (no `:undefined`, no port). - [x] `extensions/mongodb/src/client.ts:50` — `describeEndpoint` now returns a `ToolErrorContext` object rather than an endpoint string, so its name and the "host/port description of the client's configured endpoint" doc are stale and slightly misleading (the function no longer describes an endpoint). Suggested fix: rename to `describeContext` (and update `index.ts:20`/`index.test.ts` references) so the API name matches what it returns. - [ ] `AGENTS.md:26` — the commit message for this change (`139: Implement src/errors.ts …`) omits the `issue-` prefix required by the standard's stated format `issue-<N>: <summary>`. The repo's own prior commits (135–138) use the same shorthand, so this is a precedent-aligned deviation, but the standard is explicit. Suggested fix: use `issue-139: <summary>` for future commits in this series (or update AGENTS.md if the shorthand is the intended convention). ## Documentation Updated files: - README.md

david added 3 commits

2026-09-01 00:13:32 +00:00