Repository navigation
fix: cap pm item reads at 64 MiB instead of Node's 1 MiB spawnSync default - #44
Conversation
…fault readPmItems (export/import idempotency 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 1054050 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-beads-6a9l (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 selected for processing (4)
✨ 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 31 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 |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR diff =='
git diff --find-renames origin/main...HEAD || git diff --find-renames HEAD~1...HEAD
echo '== Repository source map =='
fd -e ts -e mts -e cts -e js -e mjs -e cjs . \
-E node_modules -E dist -E build \
| sort
echo '== Imports/requires and child-process call sites =='
rg -n -i -C 4 \
'(from\s+["'\'']node:child_process["'\'']|from\s+["'\'']child_process["'\'']|require\(["'\''](?:node:)?child_process["'\'']\)|\b(?:spawnSync|execSync|execFileSync|spawn|exec|execFile)\s*\()' \
-g '*.{ts,mts,cts,js,mjs,cjs}' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' .
echo '== pm command construction and invocation contexts =='
rg -n -i -C 5 \
'(["'\'']pm["'\'']|(?:command|executable|bin|args|argv)\s*[:=].*\bpm\b|\bpm\s+(?:list|export|import|show|item|items)\b)' \
-g '*.{ts,mts,cts,js,mjs,cjs}' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' .Length of output: 50532 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Relevant production symbols =='
ast-grep outline index.ts \
--items all \
--type function \
--match '^(readWorkspaceBeadIds|readPmItems|applyTimestamps|runExport|buildBeadsFromWorkspace|describePmReadFailure)$' \
--view expanded || true
echo '== Production implementations and callers =='
for range in '408,475' '750,825' '1355,1420' '1510,1625' '1800,1910'; do
echo "-- index.ts:${range} --"
sed -n "${range}p" index.ts
done
echo '== References to workspace reads, buffer cap, diagnostic, and exports =='
rg -n -C 4 \
'\b(readWorkspaceBeadIds|readPmItems|buildBeadsFromWorkspace|runExport|PM_JSON_MAX_BUFFER|describePmReadFailure)\b|export\s*\{' \
index.ts test/smoke.test.ts
echo '== Tests affected by this PR =='
git diff --unified=80 origin/main...HEAD -- test/smoke.test.ts index.ts
echo '== Every production pm process, with command arguments =='
rg -n -U -C 2 \
'spawnSync\(\s*\n?\s*"pm"[\s\S]{0,500}?\)' \
index.tsLength of output: 42430
One finding:
✏️ Learnings added
✅ 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 26 minutes. |
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>
1 participant
Problem
readPmItems()(the export/import idempotency index) 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,050 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: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
Cap
pmitem reads at 64 MiB (override per call) to bypass Node’s 1 MiBspawnSynclimit. Large exports now succeed; buffer overruns and non‑zeropmexits are clearly reported.maxBufferinreadPmItems(); default 64 MiB; override viaPM_JSON_MAX_BUFFER(bytes), resolved on each call. Invalid or non‑positive overrides (e.g. "64MiB") fall back to the default to avoid accidental tiny caps.ENOBUFSnames the actual limit used and how to fix it; non‑zeropmexits are surfaced.Written for commit e7854f9. Summary will update on new commits.