Repository navigation
fix: cap pm item reads at 64 MiB instead of Node's 1 MiB spawnSync default - #33
Conversation
…fault the ts-starter-prefix search provider 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 985089 bytes — 6% under the cap (latent). 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-ts-starter-bftc (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. |
![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 (5)
✨ 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. |
Bot-feedback triage — round 1CodeRabbit — check SUCCESS, no findings. Extra context so a re-review can be sharper: this is a one-constant change with a measured failure behind it, not a defensive tweak. The exact argv this package uses returns over 1 MiB (or within 6% of it) against a real 443-item pm workspace, and past Node's default Sourcery — weekly 500k diff-character rate limit, no review performed; downvote is on the limit notice only, not on Sourcery's judgment. Gemini — sunset notice, no review; downvoted as non-actionable. cubic — no findings surfaced. @greptileai review @coderabbitai full review |
Rate Limit Exceeded
|
Greptile SummaryCaps
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain in the fixes associated with the previous review threads.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (7): Last reviewed commit: "fix: reject malformed read-buffer overri..." | Re-trigger Greptile |
Greptile SummaryThis PR raises the search provider's
Confidence Score: 3/5The incomplete handling of nonzero The larger buffer is propagated correctly to the runtime artifact, but the new diagnostic branch handles only spawn-level errors; normal nonzero CLI exits still discard stderr and return an indistinguishable empty result set. index.ts, dist/index.js
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:643-645
**Overrun guidance is not actionable**
The ENOBUFS message tells users to narrow the operation or raise `PM_JSON_MAX_BUFFER`, but this provider always executes the full `list-all` read and the limit is a module-private constant. A workspace over 64 MiB therefore receives recovery instructions that cannot be followed through the integration.
### Issue 2 of 2
index.ts:661-663
**Nonzero read failures remain silent**
When `pm list-all` runs but exits nonzero because the workspace is unreadable or invalid, `result.error` is unset and execution reaches the unchanged silent `status !== 0` return. The provider discards `stderr` and reports an empty result set, so the diagnostic added here still leaves ordinary CLI read failures indistinguishable from no matches.
Reviews (1): Last reviewed commit: "fix: cap pm item reads at 64 MiB instead..." | 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>
Greptile SummaryCaps
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness or security issues identified. The changed search path raises the output limit while preserving successful parsing and explicitly diagnosing spawn failures before returning the provider’s established empty-result fallback.
T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (3): Last reviewed commit: "fix: cap pm item reads at 64 MiB instead..." | Re-trigger Greptile |
Round 1 findings addressed → pushed 808cee4
Both Greptile fixes were applied across the entire 8-package sweep, not just here — the same diagnostic text and the same never-throw pattern exist in the sibling packages. @greptileai review @coderabbitai full review |
Rate Limit Exceeded
|
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>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…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>
Final round — all findings addressed, T-Rex verification acknowledgedGreptile's T-Rex independently executed the before/after here and captured exactly the failure this PR fixes — pre-change: Findings this round and their resolution:
Test count 8 → 10. All bot comments read, voted and answered in-thread. Merging. |
1 participant
Problem
the
ts-starter-prefixsearch provider 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: 985,089 bytes — 6% under the cap, latent.
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:This is a search provider, where
{results: []}is a perfectly ordinary answer — so an unreadable workspace looked exactly like no matches. The cap fix is paired with a stderr line naming the read failure.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
Caps the
pmJSON read buffer to 64 MiB (configurable) and resolve it per call so env overrides always apply. Prevents Node’s 1 MiBspawnSynclimit from killing large workspaces and makes read failures visible instead of looking like empty results.pm list-all --jsonviaPM_JSON_MAX_BUFFER(default 64 MiB), resolved per call; reject malformed or non‑integer overrides (e.g.64MiB) so they fall back to the default instead of silently imposing tiny caps.ENOBUFSoverruns with the active limit and non‑zeropmexits with stderr while keeping the empty‑result contract; restored the public “never throws” JSDoc and added a smoke test that enforces the overrun reporting.Written for commit 2fd437d. Summary will update on new commits.