fix(dev-setup): name the Node.js heap limit when a type check exhausts it - #8865
Conversation
…s it The doctor discarded command output and reported every type-check failure as 'failed' with a build remediation. A contributor on a small host saw that message when Node.js had run out of heap, which points at the sources rather than the host. The check now classifies that failure and gives the NODE_OPTIONS remedy. It matches the captured output without printing it, because a failing command can name paths the report should not carry. Refs NVIDIA#8688
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe setup doctor now suppresses command output, detects Node.js heap exhaustion, and reports targeted ChangesSetup doctor diagnostics
Staging QA source update
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized change improves heap-exhaustion reporting without changing heap limits, defaults, or public remediation behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/dev-setup.sh (1)
240-240: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace the early-closing
grep -qpipeline with a shell-native substring check.
scripts/dev-setup.shenablespipefail. Large output causesprintfto receiveSIGPIPE, so the heap-remediation branch is skipped. The existing test covers the message but not large output. Add a large-output regression case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/dev-setup.sh` at line 240, In the error-handling branch around the JavaScript heap out-of-memory check, replace the pipefail-unsafe printf/grep -q pipeline with a shell-native substring check against output, preserving the existing message match and remediation behavior. Extend the relevant regression coverage with a large-output case that confirms this branch still executes without being skipped by SIGPIPE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/dev-setup.sh`:
- Line 240: In the error-handling branch around the JavaScript heap
out-of-memory check, replace the pipefail-unsafe printf/grep -q pipeline with a
shell-native substring check against output, preserving the existing message
match and remediation behavior. Extend the relevant regression coverage with a
large-output case that confirms this branch still executes without being skipped
by SIGPIPE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40ab120b-d07b-4d34-97fd-998d4447fa55
📒 Files selected for processing (2)
scripts/dev-setup.shtest/dev-setup-doctor.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
scripts/dev-setup.sh:238-240: check_quiet_command stores all command output in a shell variable, then uses printf | grep -q under pipefail. When a heap marker is followed by enough output, grep exits early and printf exits 141, so the function selects the generic type-check remediation. The unbounded variable can also consume the doctor's memory. This can hide the heap remedy or terminate the doctor, which defeats the accepted diagnostic behavior. Use a bounded or streaming matcher that drains discarded output, preserves the checked command's exit status, and avoids an early-close pipeline. Add a regression at test/dev-setup-doctor.test.ts:343 with a heap marker followed by large output; assert the heap remediation and output redaction. Regression evidence: the exact conditional selected the heap branch with 64 KiB after the marker but selected the generic branch with 128 KiB and 1 MiB.
wscurran
added
area: cli
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the large-output review finding in the latest PR commit.
The regression emits more than 128 KiB after the heap marker and verifies that the heap remediation still appears while the discarded marker remains redacted. Validation completed:
The documentation writer found no documentation change necessary because the public command, remediation, and defaults are unchanged. The security assessment found no new input interpolation, output exposure, credential handling, or privilege boundary; the matcher program is static and a matcher failure is handled conservatively as a generic check failure. Fresh repository checks and human re-review are still required. |
The latest PR commit drains command output through a bounded matcher and adds a large-output regression. Dismissing this resolved review before a full current-commit review.
|
Thanks both. @cv's finding was correct and the bug was mine. My version piped a captured variable into I verified @apurvvkumaria's replacement rather than taking it on trust:
The streaming matcher is the right shape. It drains the whole stream so nothing closes the pipe early, keeps only a bounded classification word instead of the full output, preserves both I have no further changes to make here. Ready for re-review when convenient. |
|
Readiness update:
Remaining blockers are successful infrastructure checks and maintainer re-review. No further branch change is indicated by the current failures. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Approved based on code review. Required CI checks remain merge gates.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
5 participants
Summary
The contributor doctor reported every failed type check as
CLI type check: failedwithNext: Run: npm run typecheck:cli. When Node.js ran out of heap, that message pointed at the checked sources rather than the host.check_quiet_commandnow reports the memory cause with the existingNODE_OPTIONSremedy.The classifier drains the complete compiler stream and retains only a bounded
heaporotherresult plus process statuses. Compiler output never enters the human-readable or JSON report, and large output cannot trigger an early-close pipe failure or grow a shell variable without a bound.Related Issue
Refs #8688. It does not close the issue: maintainers still need to decide whether setup should set a default heap size or whether the TypeScript configuration should change. This PR does not make that product or configuration choice and does not change any heap limit.
Changes
scripts/dev-setup.sh: stream and classify discarded command output while preserving the command and matcher statuses; retain the existing heap and generic remediations.test/dev-setup-doctor.test.ts: cover heap, generic, and more-than-128-KiB discarded-output behavior, including report redaction.Verification
npx vitest run --project integration test/dev-setup-doctor.test.ts— 41 passedprekhooks — passed, including ShellCheck and secret scanningnpm run test:titles:check— passednpm run test-size:check— passednpm run source-shape:check— passedbash -n scripts/dev-setup.sh— passedgit diff --check— passedType of Change
Quality Gates
Documentation Writer Review
no-docs-neededSecurity Review
The change adds no input interpolation, output exposure, credential handling, dependency, or privilege boundary. The matcher program is static, drains all input, and handles a matcher failure conservatively as a generic check failure. The regression confirms that discarded compiler output remains redacted.
Verification Checklist
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, including ShellChecknpm run docsbuilds without warnings (doc changes only)The owning integration suite is the proportionate local gate for this bounded contributor-doctor change. Fresh repository checks and human re-review are still required.
Signed-off-by: Vishnu Rajeev 19866703+VishnuR23@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
NODE_OPTIONSguidance when Node.js runs out of memory.Maintenance