docs: document macOS test command-line dependencies - #8700
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe contribution guide adds macOS test setup instructions. It covers GNU utility installation, PATH configuration, and the ChangesmacOS test setup
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 274-275: Update the Homebrew PATH setup in the installation
instructions to prepend $(brew --prefix bash)/bin, matching the macos-vitest
workflow while preserving the existing coreutils and gawk paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d8bda046-a33a-4ed4-b70d-c19a54bf2d60
📒 Files selected for processing (1)
CONTRIBUTING.md
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology 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 This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Applied the CodeRabbit finding in d01a844. The |
|
Maintainer readiness update This branch revision is technically prepared: the requested Homebrew Bash path is present, the documentation writer review is recorded, the docs build reports 0 errors with only the 2 warnings already present on The deterministic repository gate remains fail-closed because the secondary PR Review Advisor lane failed again during analysis after one safe rerun. The trusted publisher reports no blocking findings from the primary advisor, but the failed lane and provenance check prevent approval and merge. No bypass or merge was attempted. This PR remains open pending normal advisor automation recovery. |
|
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: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review for the latest PR commit: PASS The effective change adds 21 lines to
Validation passed: Markdown lint, the five macOS workflow contract tests, the documentation build with 0 errors and 2 existing warnings, and all applicable pre-commit checks. GitHub reports every PR commit as Verified, and the DCO declarations are preserved. The earlier Nemotron review-advisor failure was advisor infrastructure, not a documentation failure. Its artifact showed that the model emitted text before the required terminology-tool result. The artifact had already verified the documentation claims and reported no security findings. The updated branch is running a new advisor review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 274-278: Update the installation instruction in the CONTRIBUTING
guide to use precise terminology: either rename “GNU utilities” to “GNU
utilities and other command-line tools” or separate the Homebrew packages into
their respective categories, while preserving the installation and PATH-ordering
guidance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43e7b1e3-23ab-41a1-b027-be4cc46ebc68
📒 Files selected for processing (1)
CONTRIBUTING.md
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Final review update for the latest PR commit
The earlier Nemotron advisor failure was review infrastructure. The model emitted text before the required terminology-tool result completed. The preserved analysis reported all nine security categories as passing and recorded no findings. A new advisor run is evaluating the corrected branch. |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed latest PR commit 64b234b09; no substantive documentation finding.
The Homebrew package list and PATH order match the macOS workflow, including Homebrew Bash before coreutils/gawk gnubin, and the text accurately limits the workflow to pushes to main and manual dispatch. No runtime or security surface changes.
No approval because base 9257fe0f6 is behind current main, required checks are absent, and the Nemotron advisor check remains red.
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the current head and found no documentation correctness or style issue. I am withholding approval because this branch does not contain current main; refresh the branch and rerun the required checks so approval can bind to the integrated exact head.
Summary
npx vitest run --project clifails on a macOS host that does not provide the GNUtimeoututility.src/lib/shields/state-dir-lock.test.tsspawnstimeout, the spawn fails, the result carries nostdout, andparseGuardOutputreportsTypeError: Cannot read properties of undefined (reading 'split'). Before this change,CONTRIBUTING.mdlisted no command-line tool prerequisite, and the error named neither the missing utility nor macOS. After this change, the testing section names the required tools, gives thebrewandPATHcommands, and points to the workflow that owns the list.Related Issue
None.
Changes
macOS Test Dependenciessubsection to the testing section ofCONTRIBUTING.md.brew installandPATHcommands that themacos-vitestjob already runs.macos-vitestjob runs on a push tomainand on manual dispatch, so a pull request does not report a macOS-only failure.This PR adds no abstraction, configuration, fallback, migration, or compatibility path. It changes no source file and no workflow.
Reproduction and Verification
On macOS 26 arm64 with no GNU
timeoutonPATH:I confirmed the cause by putting a
timeoutstand-in first onPATHand running the same command again:.github/workflows/platform-vitest-main.yamlinstallsbash coreutils fd gawk ripgrep, adds the Bashbindirectory and thecoreutilsandgawkgnubindirectories toPATH, and printstimeout --versionin its environment step. The macOS lane therefore passes while a contributor host without those utilities fails.This PR documents the command-line tools rather than changing the test, because the
macos-vitestjob already resolves the same gap by installing them. Tell me if you would prefer the test to skip or to report the missing utility instead, and I will implement that.Type of Change
Quality Gates
CONTRIBUTING.md. It changes no source file, script, workflow, command, or default.Documentation Writer Review
docs-updatedCONTRIBUTING.md; the Homebrew Bash, coreutils, and gawk path order matches the macOS workflow. Markdown lint passed, the focused state-directory lock file passed 15 tests, and the documentation build completed with 0 errors and 2 existing Fern warnings.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablemarkdownlint-cli2,gitleaks, andcommitlintpassed inpre-commitandcommit-msg. Thepre-pushTypeScript checks passed. The focused state-directory lock file passed all 15 tests after one confirmed transient host-timing failure was safely rerun.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Notes on the unchecked verification items:
CONTRIBUTING.mdstates that a doc-only change does not need the full test suite, so I did not runnpm testornpm run check.npm run docs: I ran it. It reportsFound 0 errors and 2 warnings. I left the item unchecked because the command does not build without warnings. This PR changes no file underdocs/, so both warnings exist onmainindependently of this change.CONTRIBUTING.mdat the repository root. It adds no page underdocs/, so the Fern page style and new-page frontmatter items do not apply.Signed-off-by: Vishnu Rajeev 19866703+VishnuR23@users.noreply.github.com
Summary by CodeRabbit
PATHfor local testing.