Repository navigation
fix: cap pm item reads at 64 MiB instead of Node's 1 MiB spawnSync default - #33
Conversation
…fault readPmItems (the documented safe-read demo pattern) spawned the pm CLI without a maxBuffer, so Node's 1 MiB default applied. Measured against a real 443-item pm workspace, this package's exact argv returns 1054002 bytes (ENOBUFS today). Past the cap the child is killed with status null, signal SIGTERM, error.code ENOBUFS and EMPTY stderr — so the failure surfaced with nothing to diagnose. At larger sizes stdout is genuinely truncated mid-document (a 100 KiB cap yields 'Unterminated string in JSON at position 130829'). Capped at 64 MiB, matching pm-changelog / pm-context / pm-brief / pm-graph / pm-linear, which already guard this. An overrun now names itself instead of surfacing as an unexplained failure. Item: pm-starter-y8hb (closed) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@greptileai review /gemini review @coderabbitai full review @sourcery-ai review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
1 similar comment
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 30 minutes. |
Hound posted 8 inline comments on this PR, all on the generated `dist/index.js` and all artifacts of jshint's default ES5 profile: "'const' is available in ES6 (use 'esversion: 6')", "'template literal syntax' is only available in ES6", and style opinions about line breaks in emitted code. The package targets ES2022 on Node 22 and `dist/` is tsc output committed so the extension can be installed straight from the repo, so none of it is actionable — it just buries real findings under boilerplate. pm-web already carries the same config for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bot-feedback triage — round 1Hound — 8 inline comments, all on generated Sourcery — weekly 500k diff-character rate limit, no review performed. CodeRabbit — check SUCCESS, no findings. Gemini — sunset, no review. Re-requesting review on the new head: @greptileai review @coderabbitai full review @sourcery-ai review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 27 minutes. |
Greptile SummaryThis PR raises the
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (5): Last reviewed commit: "fix: reject malformed read-buffer overri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe PR raises
Confidence Score: 4/5The PR appears safe to merge, with two non-blocking documentation and diagnostic-actionability issues worth correcting. The 64 MiB buffer addresses the reported read failure, but its overrun message advertises unavailable controls and the source placement removes useful documentation from the generated public declaration. index.ts, dist/index.d.ts
T-Rex LogsWhat T-Rex did
Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
index.ts:154-157
**Diagnostic advertises unavailable controls**
The ENOBUFS message tells users to narrow the operation or raise `PM_JSON_MAX_BUFFER`, but `readPmItems` hardcodes the full operation and the buffer is a private constant. Users therefore cannot apply either advertised remedy without modifying the package source.
### Issue 2 of 2
index.ts:140-145
**Buffer comments detach API documentation**
Inserting the buffer declarations between the JSDoc and `readPmItems` detaches the documentation from the exported function, so the generated declaration no longer describes its never-throw and empty-array behavior.
Reviews (2): Last reviewed commit: "chore(ci): disable jshint for generated ..." | Re-trigger Greptile |
Greptile SummaryThe PR raises
Confidence Score: 4/5The change is safe to merge, with non-blocking API documentation and test-coverage issues to address. The runtime buffer fix is aligned across source and generated output, but the inserted declarations remove index.ts, dist/index.d.ts
T-Rex LogsWhat T-Rex did
Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
index.ts:141-159
**Preserve the exported API docs**
The new constant and helper separate `readPmItems` from its JSDoc, so TypeScript omits the function's never-throw and empty-array-on-failure contract from the shipped declaration and IDE documentation. Move the JSDoc back alongside the export.
### Issue 2 of 2
index.ts:166-172
**Cover the new buffer branch**
The new `maxBuffer` and `result.error` behavior has no test coverage, including the `ENOBUFS` diagnostic that motivates this change. Add a mocked `spawnSync` case that verifies the 64 MiB option, diagnostic, and empty-array return so this failure contract cannot silently regress.
Reviews (2): Last reviewed commit: "chore(ci): disable jshint for generated ..." | Re-trigger Greptile |
Greptile review findings, all confirmed: - **P2 — the ENOBUFS diagnostic prescribed an impossible remedy.** It told the reader to raise the cap, but the cap was a module-private compile-time constant with no override, so a published-package consumer could only follow that advice by patching and rebuilding. The cap now reads an env var (`PM_JSON_MAX_BUFFER` / `PM_LIST_MAX_BUFFER`, in bytes), falling back to the default on any invalid or non-positive value so the guard cannot be silently disabled. - **P2 (pm-starter) — inserting the constant between the existing JSDoc and `readPmItems` orphaned the doc comment**, dropping the "never throws / safe read pattern" contract from the consumer-facing `dist/index.d.ts`. Verified in the generated output, then moved the block above the JSDoc; the contract is back in `dist/index.d.ts`. - **P1 (starters) — a genuine nonzero `pm` exit was still silent.** Only the `result.error` branch was hardened, so an unreadable or invalid workspace still returned an empty result with stderr discarded — leaving exactly the failure mode this change set out to remove: silence that looks like "no items" / "no matches". Both never-throw paths now log the exit code and stderr while keeping their empty-result contract. Typecheck and full test suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the Greptile round-2 findings. - The cap was resolved once at module load, so the `PM_JSON_MAX_BUFFER` override only applied if the env var was set before the module was imported — an import-order dependency that also made the branch untestable. Now resolved per call (one integer parse against a process spawn: free), and the ENOBUFS message reports the limit actually in effect rather than a constant's name. - Added coverage for the failure contract in the starter packages: a real pm workspace read with the cap forced to 64 bytes must return the documented empty result AND report the overrun on stderr. Without a test, "a read failure must not look like an empty workspace" was an assertion in a commit message rather than something the suite enforces. Typecheck and the full test suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eric prefix
Greptile P1: `Number.parseInt("64MiB", 10)` returns 64, so that value silently
imposed a **64-BYTE** cap — breaking every ordinary read while appearing to honor
the documented invalid-value fallback. The plausible-looking typo was the worst
case: it turned a safety guard into a guaranteed failure.
Switched to `Number()`, which rejects the whole string, plus
`Number.isSafeInteger(raw) && raw > 0`. Covered in the starter suite across
"64MiB", "64 MB", "abc", "-1", "0", "6.5" and "", each of which must fall back to
the default, while a valid explicit value is still honored.
Also narrowed the starters' overrun message: those reads are always the full
workspace, so "narrow the operation" was a dead instruction — the message now
names only the lever the reader has.
Typecheck and full test suites green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 findings addressed → pushed 7fdb919
Every Greptile finding this round was correct, including two where the fix needed fixing. Test count here went 7 → 9, both new tests covering the failure contract rather than the happy path. @greptileai review @coderabbitai full review |
Rate Limit Exceeded
|
Final round — both notes in the Greptile summary are already addressed on this head (7fdb919)The summary flagged "non-blocking API documentation and test-coverage issues"; both were fixed in 01cb689 / 7fdb919 after that review ran. Proof from the current head: Documentation restored in the shipped types ( /**
* Safely read all items from the workspace by shelling out to `pm`. Returns an
* empty array on any failure so demos never throw at activation/read time.
* This is the SAFE read pattern every demo reuses.
*/
export declare function readPmItems(pmRoot: string): Array<Record<string, any>>;Coverage added — 7 → 9 tests, both new ones on the failure contract rather than the happy path: an overrun forced through the env override against a real On T-RexWorth calling out: T-Rex executed the before/after and independently reproduced the defect — "observed that it produced 0 items before the buffer change", then confirmed the post-change build parses the complete payload. That is a materially stronger signal than a static read, and it matched the measurement that motivated this sweep (1,054,002 bytes against a real 443-item workspace). Same on All bot comments on this PR are read, voted and answered in-thread. Merging. |
1 participant
Problem
readPmItems()— the documented safe read pattern every demo reuses spawns thepmCLI to read items but passes nomaxBuffer, so Node's 1 MiBspawnSyncdefault applies.Measured with this package's exact argv against a real 443-item pm workspace: 1,054,002 bytes — over the cap, failing today.
Past the cap the child is killed and the result comes back as:
Because
stderris empty, the failure surfaced with nothing to diagnose. And at larger sizes stdout is genuinely truncated mid-document — verified by forcing a 100 KiB cap on the same workspace:Because this path is contractually never-throw (
return []on any failure), a read failure was indistinguishable from an empty workspace — every demo would show no items and look correct. The cap fix is paired with a stderr line so the cause is visible without breaking the contract.This was a straggler:
pm-changelog,pm-context,pm-brief,pm-graphandpm-linearalready cap their pm reads explicitly. A fleet-wide audit this session found 8 packages that did not.Change
ENOBUFSoverrun explicitly — naming the limit and the remedy — instead of an unexplained failureVerification
Same argv, same real 443-item workspace:
status: null,ENOBUFS, empty stderrstatus: 0, complete payloadTypecheck and the full test suite pass;
changelog:checkgreen.pm items
Companion-tracked fleet sweep; the same fix landed in
pm-github(unbraind/pm-github#13), where it additionally unblocked--atomic --dry-run.🤖 Generated with Claude Code
Summary by cubic
Capped
pmitem reads at 64 MiB with a per‑callPM_JSON_MAX_BUFFERoverride and clearer errors so large or failing workspaces don’t look empty. Malformed overrides now fall back to the default instead of imposing tiny caps.PM_JSON_MAX_BUFFERon everyreadPmItemscall; reject invalid/non‑positive values withNumber()+isSafeInteger; overrun message names the actual byte limit and points to the env var. Matches sibling packages and resolvespm-starter-y8hb.[]contract but log ENOBUFS and non‑zero exits (exit code + stderr); restored the safe‑read JSDoc in the emittedd.ts.dist/in Hound to avoid ES5 lint noise.Written for commit 7fdb919. Summary will update on new commits.