Skip to content

fix: cap pm item reads at 64 MiB instead of Node's 1 MiB spawnSync default - #28

Merged
unbraind merged 4 commits into
mainfrom
fix/pm-read-buffer-1mib-ceiling
Jul 24, 2026
Merged

unbraind merged 4 commits into
mainfrom
fix/pm-read-buffer-1mib-ceiling

Conversation

@unbraind

@unbraind unbraind commented Jul 24, 2026 •

Copy link
Copy Markdown
Owner

Problem

readPmItemsForUpsert() (the --upsert index) spawns the pm CLI to read items but passes no maxBuffer, so Node's 1 MiB spawnSync default applies.

Measured with this package's exact argv against a real 443-item pm workspace: 985,137 bytes — 6% under the cap, latent.

Past the cap the child is killed and the result comes back as:

status: null   signal: SIGTERM   error.code: ENOBUFS   stderr: ""

Because stderr is 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:

JSON.parse -> Unterminated string in JSON at position 130829

This was a straggler: pm-changelog, pm-context, pm-brief, pm-graph and pm-linear already cap their pm reads explicitly. A fleet-wide audit this session found 8 packages that did not.

Change

  • cap the pm read at 64 MiB, matching the sibling packages
  • report an ENOBUFS overrun explicitly — naming the limit and the remedy — instead of an unexplained failure

Verification

Same argv, same real 443-item workspace:

read buffer result
Node default (1 MiB) status: null, ENOBUFS, empty stderr
64 MiB (this PR) status: 0, complete payload

Typecheck and the full test suite pass; changelog:check green.

pm items

item status
pm-todos-9b5z — pm item reads are capped at Node's 1 MiB spawnSync default, so a mature tracker fails with no diagnosis closed

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

Cap pm JSON reads at 64 MiB by default and make the buffer configurable per call to prevent silent truncation on large workspaces and report clear overruns. Also reject malformed overrides to avoid accidental tiny caps.

  • Bug Fixes
    • Pass a 64 MiB maxBuffer to the pm list-all --json spawnSync call in readPmItemsForUpsert.
    • Allow per-call overrides via PM_JSON_MAX_BUFFER (bytes); invalid, non‑positive, or malformed values (e.g. "64MiB") fall back to 64 MiB.
    • On ENOBUFS, throw a clear error that names the actual limit used and suggests narrowing the operation or raising PM_JSON_MAX_BUFFER.

Written for commit 88291e1. Summary will update on new commits.

Review in cubic

…fault

readPmItemsForUpsert (--upsert index) 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 985137 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-todos-9b5z (closed)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@unbraind

Copy link
Copy Markdown
Owner Author

@greptileai review

/gemini review

@coderabbitai full review

@sourcery-ai review

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

1 similar comment
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @unbraind, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

@unbraind, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c74db3cb-db17-4830-a3a5-2a9d1c8fc9ec

📥 Commits

Reviewing files that changed from the base of the PR and between d5785f9 and 88291e1.

📒 Files selected for processing (4)
  • .agents/pm/history/pm-todos-9b5z.jsonl
  • .agents/pm/issues/pm-todos-9b5z.toon
  • CHANGELOG.md
  • index.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pm-read-buffer-1mib-ceiling

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

@unbraind: I’ll perform a full review of #28.

✅ Action performed

Full 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.

@unbraind

Copy link
Copy Markdown
Owner Author

Bot-feedback triage — round 1

CodeRabbit — 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 spawnSync cap the child is killed with status: null, signal: SIGTERM, error.code: ENOBUFS and empty stderr — so the pre-existing if (result.status !== 0) throw result.stderr || "...failed" reported an unexplained failure. At larger sizes stdout truncates mid-JSON (forcing a 100 KiB cap on the same workspace yields Unterminated string in JSON at position 130829). The highest-value thing to look for here is any other unguarded child-process read of pm output in this package that the audit's grep missed.

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

@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Source-file map =='
fd -t f -e ts -e mts -e cts -e js -e mjs -e cjs . | sort

echo
echo '== Child-process imports and invocations =='
rg -n -i -C 5 \
  '(from\s+["'\''](?:node:)?child_process["'\'']|require\(["'\''](?:node:)?child_process["'\'']\)|\b(?:spawnSync|execSync|execFileSync|spawn|exec|execFile)\s*\()' \
  -g '*.{ts,mts,cts,js,mjs,cjs}' .

echo
echo '== pm command construction and maxBuffer options =='
rg -n -i -C 5 \
  '(["'\'']pm["'\'']|\bpm\s+(?:list|show|items|search)|\bmaxBuffer\b)' \
  -g '*.{ts,mts,cts,js,mjs,cjs}' .

Length of output: 25791


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Relevant production code =='
sed -n '1860,1930p' index.ts
sed -n '2190,2235p' index.ts

echo
echo '== PR diff =='
git diff --unified=80 "$(git merge-base HEAD origin/main)" HEAD -- index.ts 2>/dev/null \
  || git diff --unified=80 HEAD^ HEAD -- index.ts

echo
echo '== Call sites of fetchPmItems =='
rg -n -C 6 '\bfetchPmItems\s*\(' index.ts

Length of output: 15557


@unbraind: The focused audit found one missed production read:

  • index.ts, fetchPmItems() invokes pm list-all --json with no maxBuffer. It powers both export (buildTodoMarkdown()) and context generation, so a sufficiently large workspace can still hit Node’s 1 MiB default and surface the same opaque ENOBUFS failure.
  • The only other production pm child-process calls are create and update writes; I found no further unguarded workspace-list reads.

This should share PM_JSON_MAX_BUFFER and handle result.error through describePmReadFailure() before checking status, matching readPmItemsForUpsert().

I’ve also initiated the requested complete re-review.

✅ Action performed

Full 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 26 minutes.

unbraind and others added 3 commits July 24, 2026 22:34
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>
@unbraind
unbraind merged commit f38818b into main Jul 24, 2026
5 checks passed
@unbraind
unbraind deleted the fix/pm-read-buffer-1mib-ceiling branch July 24, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant