issue-138: Implement src/serialize.ts with unit tests (EJSON + truncation) #155

Merged

david merged 2 commits from feature/issue-138/implement-src-serialize-ts-with-unit-tests-ejson-t into main

2026-09-01 00:02:21 +00:00

david commented

2026-09-01 00:02:20 +00:00

Owner

138: Implement src/serialize.ts with unit tests (EJSON + truncation)

Implements 138.

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 14m 38s.

Review findings

Now let me look at the DESIGN.md section that describes the EJSON mode, and check the IMPLEMENTATION_PLAN's relaxed-mode statement, to decide the fix direction for the High finding.Let me read the DESIGN.md goals section (around line 20) before editing.Now I have the full picture. The project's design docs explicitly commit to relaxed EJSON ("Output matches mongosh/Compass"), and the plan's round-trip scope deliberately lists ObjectId/Date/Binary/Decimal128 (not Long). So for the High finding I'll take the review's second offered path: keep relaxed mode, correct the false "Long round-trips losslessly" claims in code/docs, and add a test that covers Long by pinning the documented limitation. Let me apply all fixes.Now the test file:Now the doc updates (find.ts docstring, README.md, DESIGN.md):Let me review the final state of the changed files:Now let me run the project gates. First verify no lint config exists, then run tests and the TypeScript check:All 70 tests pass (67 original + 3 new). Now the TypeScript check:Gates green. Let me verify the final working-tree diff scope:All fixes are in place and gates are green. Here is the annotated findings document.

Review Findings — 138

Summary

All five findings were remediated in a single pass. High: kept the project's deliberately relaxed EJSON mode (DESIGN.md and IMPLEMENTATION_PLAN both commit to relaxed output to "match mongosh/Compass"), and instead corrected the false "Long round-trips losslessly" claims in src/serialize.ts, src/tools/find.ts, README.md, and docs/DESIGN.md (goal at line 20 + "Why EJSON" paragraph), and added a test that covers Long by pinning the documented >2^53 precision limitation. Medium: truncated now also flags a lone kept document that still exceeds the cap (with a included.length > 0 guard so empty input is never flagged), with a new single-doc-over-cap test asserting truncated: true plus marker presence; added a cap-47 boundary test that pins the byte-based truncation decision (a char-based counter would wrongly include the second doc). Low: moved the over-cap cost computation inside the included.length > 0 branch so the unused first-iteration value is no longer computed; replaced the brittle split("\n…")[0] sites with a jsonBeforeMarker() helper that uses lastIndexOf with an untruncated fallback. Gates: bun test in extensions/mongodb — PASS (70 pass / 0 fail, 116 expect calls across 8 files, 3 tests added); bunx tsc --noEmit — PASS (exit 0); lint — N/A (no eslint/biome/prettier config exists anywhere in the repo). Nothing remains unresolved.

Critical

(none)

High

extensions/mongodb/src/serialize.ts:19-20 — The documented contract "BSON types (ObjectId, Date, Binary, Decimal128, Long) round-trip losslessly via EJSON" is false for Long. EJSON.stringify defaults to relaxed mode (relaxed: true, see bson stringify), and bson's Long.toExtendedJSON returns this.toNumber() in that mode, silently losing precision for int64 values beyond Number.MAX_SAFE_INTEGER. Probe: ejsonSerializeWithTruncation([{ big: Long.fromString("9007199254740993") }], 10_000) emits "big": 9007199254740992 (not {"$numberLong":"9007199254740993"}). The same false claim appears in extensions/mongodb/README.md and docs/DESIGN.md:20, and no test covers Long (this diff even narrowed the round-trip test name from "BSON types" to "ObjectId/Date/Binary/Decimal128"). Suggested fix: add a round-trip test with a >2^53 Long (e.g. Long.fromString("9007199254740993")) — it will fail today — then either emit canonical EJSON (EJSON.stringify(doc, null, 2, { relaxed: false })) so Longs serialize to $numberLong, or correct README/DESIGN/docstring to scope the lossless claim to ObjectId/Date/Binary/Decimal128 and document the Long limitation.

Medium

extensions/mongodb/src/serialize.ts:50 — truncated is computed as included.length < docs.length, so a lone document that exceeds byteCap returns truncated: false even though the output is over budget (probe: ejsonSerializeWithTruncation([{ payload: "x".repeat(500) }], 10) → { truncated: false, docCount: 1 }). This contradicts the interface's own wording at line 8 ("True when documents were dropped to stay under the byte cap") and is surfaced through mongo_find's details.truncated, so the LLM can treat over-cap output as complete. Suggested fix: also flag over-cap output, e.g. const truncated = included.length < docs.length || bytes + 2 > byteCap;, and add a single-doc-over-cap test asserting truncated: true plus marker presence.
extensions/mongodb/src/serialize.test.ts:64-78 — The UTF-8 test only distinguishes byte-counting through the marker byte total (48/48 vs a char-based 46/48); the truncation decision is identical under both counting schemes at cap 48 (both exclude doc 3). Suggested fix: add a boundary case at a cap between the char and byte lengths of the second doc — e.g. ejsonSerializeWithTruncation(docs, 47) expecting docCount: 1 and 25/47 bytes (a char-based implementation would include the second doc at 46 ≤ 47) — to pin the byte-based decision itself.

Low

extensions/mongodb/src/serialize.ts:42-43 — candidateBytes is computed for the first document but never used (the included.length > 0 guard always skips the check for the first iteration). Suggested fix: move the candidateBytes computation inside the included.length > 0 branch so the unused first-iteration value is not computed.
extensions/mongodb/src/serialize.test.ts:57,75,99 — The repeated result.text.split("\n…")[0] helper is brittle: if a document's content ever contained the two-character sequence \n…, the split would cut the JSON mid-document and the parse assertions would fail (or silently assert on truncated text). Test-only, but suggested fix: extract a jsonBeforeMarker() helper that splits on the last occurrence or asserts the marker is present before splitting.

## 138: Implement src/serialize.ts with unit tests (EJSON + truncation) Implements [138](https://git.excelera.net/david/pi-extensions-and-skills/issues/138). > 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 14m 38s. ## Review findings Now let me look at the DESIGN.md section that describes the EJSON mode, and check the IMPLEMENTATION_PLAN's relaxed-mode statement, to decide the fix direction for the High finding.Let me read the DESIGN.md goals section (around line 20) before editing.Now I have the full picture. The project's design docs explicitly commit to **relaxed** EJSON ("Output matches mongosh/Compass"), and the plan's round-trip scope deliberately lists ObjectId/Date/Binary/Decimal128 (not Long). So for the High finding I'll take the review's second offered path: keep relaxed mode, correct the false "Long round-trips losslessly" claims in code/docs, and add a test that covers `Long` by pinning the documented limitation. Let me apply all fixes.Now the test file:Now the doc updates (find.ts docstring, README.md, DESIGN.md):Let me review the final state of the changed files:Now let me run the project gates. First verify no lint config exists, then run tests and the TypeScript check:All 70 tests pass (67 original + 3 new). Now the TypeScript check:Gates green. Let me verify the final working-tree diff scope:All fixes are in place and gates are green. Here is the annotated findings document. # Review Findings — 138 ## Summary All five findings were remediated in a single pass. **High:** kept the project's deliberately relaxed EJSON mode (DESIGN.md and IMPLEMENTATION_PLAN both commit to relaxed output to "match mongosh/Compass"), and instead corrected the false "Long round-trips losslessly" claims in `src/serialize.ts`, `src/tools/find.ts`, `README.md`, and `docs/DESIGN.md` (goal at line 20 + "Why EJSON" paragraph), and added a test that covers `Long` by pinning the documented >2^53 precision limitation. **Medium:** `truncated` now also flags a lone kept document that still exceeds the cap (with a `included.length > 0` guard so empty input is never flagged), with a new single-doc-over-cap test asserting `truncated: true` plus marker presence; added a cap-47 boundary test that pins the byte-based truncation *decision* (a char-based counter would wrongly include the second doc). **Low:** moved the over-cap cost computation inside the `included.length > 0` branch so the unused first-iteration value is no longer computed; replaced the brittle `split("\n…")[0]` sites with a `jsonBeforeMarker()` helper that uses `lastIndexOf` with an untruncated fallback. Gates: `bun test` in `extensions/mongodb` — **PASS** (70 pass / 0 fail, 116 expect calls across 8 files, 3 tests added); `bunx tsc --noEmit` — **PASS** (exit 0); lint — **N/A** (no eslint/biome/prettier config exists anywhere in the repo). Nothing remains unresolved. ## Critical (none) ## High - [x] extensions/mongodb/src/serialize.ts:19-20 — The documented contract "BSON types (ObjectId, Date, Binary, Decimal128, Long) round-trip losslessly via EJSON" is false for `Long`. `EJSON.stringify` defaults to relaxed mode (`relaxed: true`, see bson `stringify`), and bson's `Long.toExtendedJSON` returns `this.toNumber()` in that mode, silently losing precision for int64 values beyond `Number.MAX_SAFE_INTEGER`. Probe: `ejsonSerializeWithTruncation([{ big: Long.fromString("9007199254740993") }], 10_000)` emits `"big": 9007199254740992` (not `{"$numberLong":"9007199254740993"}`). The same false claim appears in `extensions/mongodb/README.md` and `docs/DESIGN.md:20`, and no test covers `Long` (this diff even narrowed the round-trip test name from "BSON types" to "ObjectId/Date/Binary/Decimal128"). Suggested fix: add a round-trip test with a >2^53 Long (e.g. `Long.fromString("9007199254740993")`) — it will fail today — then either emit canonical EJSON (`EJSON.stringify(doc, null, 2, { relaxed: false })`) so Longs serialize to `$numberLong`, or correct README/DESIGN/docstring to scope the lossless claim to ObjectId/Date/Binary/Decimal128 and document the Long limitation. ## Medium - [x] extensions/mongodb/src/serialize.ts:50 — `truncated` is computed as `included.length < docs.length`, so a lone document that exceeds `byteCap` returns `truncated: false` even though the output is over budget (probe: `ejsonSerializeWithTruncation([{ payload: "x".repeat(500) }], 10)` → `{ truncated: false, docCount: 1 }`). This contradicts the interface's own wording at line 8 ("True when documents were dropped to stay under the byte cap") and is surfaced through `mongo_find`'s `details.truncated`, so the LLM can treat over-cap output as complete. Suggested fix: also flag over-cap output, e.g. `const truncated = included.length < docs.length || bytes + 2 > byteCap;`, and add a single-doc-over-cap test asserting `truncated: true` plus marker presence. - [x] extensions/mongodb/src/serialize.test.ts:64-78 — The UTF-8 test only distinguishes byte-counting through the marker byte total (48/48 vs a char-based 46/48); the truncation *decision* is identical under both counting schemes at cap 48 (both exclude doc 3). Suggested fix: add a boundary case at a cap between the char and byte lengths of the second doc — e.g. `ejsonSerializeWithTruncation(docs, 47)` expecting `docCount: 1` and `25/47 bytes` (a char-based implementation would include the second doc at 46 ≤ 47) — to pin the byte-based decision itself. ## Low - [x] extensions/mongodb/src/serialize.ts:42-43 — `candidateBytes` is computed for the first document but never used (the `included.length > 0` guard always skips the check for the first iteration). Suggested fix: move the `candidateBytes` computation inside the `included.length > 0` branch so the unused first-iteration value is not computed. - [x] extensions/mongodb/src/serialize.test.ts:57,75,99 — The repeated `result.text.split("\n…")[0]` helper is brittle: if a document's content ever contained the two-character sequence `\n…`, the split would cut the JSON mid-document and the parse assertions would fail (or silently assert on truncated text). Test-only, but suggested fix: extract a `jsonBeforeMarker()` helper that splits on the last occurrence or asserts the marker is present before splitting.

david added 2 commits

2026-09-01 00:02:20 +00:00