Repository navigation
fix(import): raise the pm read buffer past 1 MiB and preview updates in non-atomic --dry-run - #13
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@greptileai review /gemini review @coderabbitai full review @sourcery-ai review |
|
Warning Review limit reached
Next review available in: 9 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 (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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
✅ 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 43 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
printf '%s\n' '== Changed files =='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD || git diff --name-status HEAD~1 HEAD
printf '%s\n' '== Candidate package/source files =='
fd -t f -E node_modules -E dist -E build | sed -n '1,240p'
printf '%s\n' '== Child-process imports and invocations =='
rg -n -i -C 4 \
'(from\s+["'\''](?:node:)?child_process["'\'']|require\(["'\''](?:node:)?child_process["'\'']\)|\b(?:spawnSync|execSync|execFileSync|spawn|exec|execFile)\s*\()' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' .
printf '%s\n' '== pm command construction / output reads =='
rg -n -i -C 4 \
'(\bpm\s+(?:list-all|show|[a-z-]+)|["'\'']pm["'\'']|\.stdout\b|JSON\.parse\s*\()' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' .Length of output: 179718 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
printf '%s\n' '== PR diff: index.ts and dry-run tests =='
git diff --unified=80 "$base" HEAD -- index.ts test/dryrun-preview.test.ts
printf '%s\n' '== Import-flow symbols =='
ast-grep outline index.ts --items all --type function --match 'runImport|readPmItems|runAtomicImport|pmRun'
printf '%s\n' '== Import execution and result paths =='
rg -n -C 24 \
'(shouldMatchExisting|opts\.dryRun|wouldImport|wouldUpdate|wouldSkip|indexByProvenance|readPmItems\(|runAtomicImport\()' \
index.ts test/dryrun-preview.test.ts
printf '%s\n' '== All production spawnSync call sites with surrounding source =='
rg -n -C 10 '\bspawnSync\s*\(' index.ts scripts/prepare-merge-driver.mjsLength of output: 87916
Focused audit results:
I did not execute the test suite because repository code execution is unavailable in this review sandbox. ✅ 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. |
…review updates Two defects found by running the importer against a real 443-item workspace rather than fixtures. Both make the integration fail or mislead exactly when a tracker is mature enough to matter. 1. `readPmItems()` spawned `pm list-all --full --include-body` without a `maxBuffer`, so Node's 1 MiB default applied. That workspace's dump is 1,052,859 bytes — 4 KiB over the cap — so the child was killed with `status: null`, `error.code: ENOBUFS` and EMPTY stderr, and the code reported a bare `pm list-all failed` with nothing to diagnose. Every import, atomic plan and sync against such a workspace was dead with no actionable message. Now capped at 64 MiB (matching pm-changelog / pm-context / pm-brief, which already learned this), applied to `readPmItems()`, the central `pmRun()` spawner and the `afterCommand` hook's `pm show`. A buffer overrun is now named explicitly — the message says the output exceeded the read buffer and suggests narrowing the import (`--labels`, `--since`) — instead of being reported as an unexplained failure. 2. The non-atomic `--dry-run` path deliberately skipped building the provenance index (`shouldMatchExisting = !dryRun || atomic`), so every issue was previewed as a create: "Would import 29, skip 0" where the real run performs updates for already-linked issues. A preview that overstates creates reads as "this will duplicate my whole tracker" — the single thing `--dry-run` exists to rule out. `--atomic --dry-run` reported the split correctly, so the two paths disagreed about the same plan. The index is now built for dry runs too, and the non-atomic preview labels each issue `import`/`update` and reports "Would import X, update Y, skip Z", returning `wouldUpdate` alongside `wouldImport`/`wouldSkip` like the atomic path. Note this also means dry runs now read the tracker — which is why (1) had to be fixed in the same change, or previews would newly hit ENOBUFS. Verified against the same real workspace: non-atomic reports "Would import 24, update 5, skip 0"; `--atomic --dry-run`, which previously died with ENOBUFS, now completes and agrees exactly (24/5/0). 168/168 tests pass, including two new tests covering the preview split and the two paths' agreement. Items: pm-github-<pending> (see PR) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fects Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
CHANGELOG.md is generated, so the rebase conflict against the merged changelog-hygiene PR is resolved by regeneration rather than by hand-picking hunks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9af7cd8 to
9d2070b
Compare
|
Force-pushed: rebased onto Also carried in from the Greptile round on the sibling packages in this sweep (same code shape, same defects):
@greptileai review @coderabbitai full review |
Rate Limit Exceeded
|
Two defects, both found by running the importer against a real 443-item workspace instead of fixtures
Both fail — or mislead — precisely when a tracker is mature enough for the integration to matter.
1. Import dies on any tracker over 1 MiB, with nothing to diagnose
readPmItems()spawnedpm list-all --full --include-bodywithout amaxBuffer, so Node's 1 MiBspawnSyncdefault applied. The real workspace used for testing dumps 1,052,859 bytes — 4 KiB over the cap — so the child was killed and the result came back as:readPmItems()only checksresult.status !== 0and reportsresult.stderr || "pm list-all failed", so every import, atomic plan and sync against that workspace failed with a barepm list-all failedand no cause. Nothing in the message hints at a buffer limit, so the natural next step is to go hunting for a broken tracker.Fixed by capping at 64 MiB — the same cap
pm-changelog,pm-contextandpm-briefalready settled on — applied toreadPmItems(), the centralpmRun()spawner (a largepm show --jsonhas the same exposure) and theafterCommandhook'spm show. A buffer overrun is now named explicitly and suggests narrowing the import (--labels,--since) instead of surfacing as an unexplained failure.2. Non-atomic
--dry-runpreviewed every issue as a createWith the provenance index never built,
matchwas always undefined, so the preview counted every issue as an import:A preview that overstates creates reads as "this will duplicate my whole tracker", which is the single thing
--dry-runexists to rule out.--atomic --dry-runbuilt the index and reported the split correctly, so the two paths disagreed about the same plan.The index is now built for dry runs too; the non-atomic preview labels each issue
import/updateand returnswouldUpdatealongsidewouldImport/wouldSkip, matching the atomic shape. Note the coupling: fixing (2) makes dry runs read the tracker, so (1) had to be fixed in the same change or previews would newly hit ENOBUFS.Verification (same real workspace that exposed the bugs)
pm github import unbraind/pm-cli --state open --dry-runWould import 29, skip 0Would import 24, update 5, skip 0… --dry-run --atomicError: pm list-all failed(ENOBUFS)wouldImport 24, wouldUpdate 5, wouldSkip 0Both paths now agree exactly. The 5 updates are real: 5 upstream issues in that workspace already carry
gh:unbraind/pm-cli#Nprovenance tags, and a real run would update rather than duplicate them — which the old preview could not tell you.168/168 tests pass, including two new tests: one asserting the non-atomic preview labels an already-linked issue as an update, one asserting the atomic and non-atomic previews agree on the split.
pm items
pm list-all failedon any tracker over 1 MiB, and non-atomic--dry-runpreviews every issue as a createRelated fleet context: the same unguarded-
maxBufferpattern exists in 7 sibling packages (pm-beads,pm-jira,pm-todos,pm-starter,pm-gantt-chart,pm-ts-starter, and one call inpm-csv); those are tracked separately.🤖 Generated with Claude Code
Summary by cubic
Raise the
pmread buffer to 64 MiB (overridable viaPM_JSON_MAX_BUFFER) and fix non-atomic--dry-runso previews correctly show imports vs updates. The cap is resolved per call and rejects malformed overrides, removing unexplainedpm list-all failederrors and making dry-run output match real plans.readPmItems, centralpmRun, andafterCommandpm show; resolve it per call. SupportPM_JSON_MAX_BUFFER, ignoring invalid values (e.g. "64MiB"); ENOBUFS now reports the effective limit with tips to narrow via--labels/--since.--dry-runbuilds the provenance index, labels each issueimport/update, logs "Would import X, update Y, skip Z", and returnswouldUpdate; tests verify the split and parity with--atomic --dry-run.Written for commit 9d2070b. Summary will update on new commits.