ci(hooks): separate pre-commit and pre-push stages by cv · Pull Request #5901 · NVIDIA/NemoClaw

cv · 2026-06-27T07:43:03Z

Summary

Make prek's lifecycle stages explicit so pre-commit owns formatting, linting, and tests while pre-push owns typechecking and version synchronization. This keeps local hooks and static CI aligned without running the same expensive work twice.

Changes

Set pre-commit as the default stage and pin upstream hooks that opt into multiple stages to pre-commit.
Run the static-checks action against the pre-commit stage; keep TypeScript and version-sync hooks exclusively on pre-push.
Add a workflow contract test that ratchets the intended stage ownership.
Raise only the copied rlimit test fixture's process cap to 4096 so 16-worker Vitest runs cannot starve their own shell; the production cap remains 512.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)
Doc only (includes code sample changes)

Quality Gates

Tests added or updated for changed behavior
Existing tests cover changed behavior — justification:
Tests not applicable — justification:
Docs updated for user-facing behavior changes
Docs not applicable — justification: internal hook and CI stage scheduling only; the stage boundary is documented by an executable contract test.
Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
Targeted tests pass for changed behavior
Full npm test passes (broad runtime changes only)
Quality Gates section completed with required justifications or waivers
No secrets, API keys, or credentials committed
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

Targeted verification:

VITEST_MAX_WORKERS=16 npx vitest run --project cli test/pr-workflow-contract.test.ts test/sandbox-rlimit-hooks.test.ts
npx prek validate-config .pre-commit-config.yaml
npx prek run --all-files --stage pre-push --dry-run --no-progress
Signed commit and push hooks with VITEST_MAX_WORKERS=16

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Bug Fixes
- Updated CI static hook checks to run during the pre-commit lifecycle (instead of pre-push) and aligned related hook skipping behavior.
- Improved sandbox test stability by standardizing the overridden NPROC limit during test setup.
Tests
- Enhanced pre-commit contract tests to validate default_stages and confirm which hooks are explicitly pinned to pre-commit vs pre-push.
- Added coverage ensuring the production sandbox-rlimits.sh fixture retains its default NPROC limit.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

coderabbitai · 2026-06-27T07:43:17Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c7004958-fae8-407d-8dd6-62f40e021e1f

📥 Commits

Reviewing files that changed from the base of the PR and between 69cc96f and 57e2d23.

📒 Files selected for processing (2)

.github/actions/ci-static-checks/action.yaml
test/sandbox-rlimit-hooks.test.ts

📝 Walkthrough

Walkthrough

The PR switches static hook checks and related contract tests from pre-push to pre-commit, adds explicit pre-commit stage settings in the pre-commit config, and makes the sandbox NPROC hook fixture use a fixed limit of 4096.

Changes

Pre-commit stage alignment

Layer / File(s)	Summary
Config contract and hook stages `.pre-commit-config.yaml`, `test/pr-workflow-contract.test.ts`	Adds `default_stages: [pre-commit]`, sets selected hooks to `stages: [pre-commit]`, and adds a `PrekConfig` contract test that checks the stage mapping.
Static hook command stage `.github/actions/ci-static-checks/action.yaml`, `test/pr-workflow-contract.test.ts`	Changes the static hook check command to `--stage pre-commit` and updates the workflow contract assertions for that stage.

Sandbox NPROC fixture

Layer / File(s)	Summary
Fixture NPROC limit `test/sandbox-rlimit-hooks.test.ts`	`copyRlimitFixture` now always uses an NPROC limit of `4096` and verifies the production rlimit file still uses `512`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

NVIDIA/NemoClaw#5840: Also changes test/sandbox-rlimit-hooks.test.ts and RLIMIT/NPROC-related test behavior.

Suggested labels

area: ci, chore

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: making hook stages explicit by separating pre-commit and pre-push responsibilities.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch codex/separate-prek-hook-stages

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

github-code-quality · 2026-06-27T07:43:37Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/separate-prek-... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`codex/separate-prek-...` 57e2d23	+/-
`nemoclaw/src/se...cret-scanner.ts`	—	100%	—
`nemoclaw/src/commands/slash.ts`	—	100%	—
`nemoclaw/src/li...bprocess-env.ts`	—	100%	—
`nemoclaw/src/bl...eprint/state.ts`	—	98%	—
`nemoclaw/src/onboard/config.ts`	—	98%	—
`nemoclaw/src/bl...int/snapshot.ts`	—	97%	—
`nemoclaw/src/bl...print/runner.ts`	—	95%	—
`nemoclaw/src/co...ration-state.ts`	—	94%	—
`nemoclaw/src/bl...ate-networks.ts`	—	94%	—
`nemoclaw/src/index.ts`	—	94%	—

TypeScript / code-coverage/cli

The overall coverage in the codex/separate-prek-... branch is 47%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`codex/separate-prek-...` 57e2d23	+/-
`src/lib/state/o...oard-session.ts`	—	91%	—
`src/lib/actions...dbox/rebuild.ts`	—	73%	—
`src/lib/sandbox/config.ts`	—	72%	—
`src/lib/onboard/preflight.ts`	—	62%	—
`src/lib/shields/index.ts`	—	62%	—
`src/lib/actions...licy-channel.ts`	—	60%	—
`src/lib/state/sandbox.ts`	—	56%	—
`src/lib/policy/index.ts`	—	48%	—
`src/lib/onboard...er-gpu-patch.ts`	—	47%	—
`src/lib/onboard.ts`	—	18%	—

_{Updated June 27, 2026 07:52 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

github-actions · 2026-06-27T07:43:51Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None. No E2E is recommended. The PR changes CI/static-check hook staging and test assertions only. It does not modify runtime code or assets that affect installer/onboarding, sandbox lifecycle behavior, credentials, security boundaries, network policy, inference routing, deployment, or real assistant user flows.

Optional E2E

None.

New E2E recommendations

None.

github-actions · 2026-06-27T07:43:52Z

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: None
Optional Vitest E2E scenarios: None

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

None. No Vitest E2E scenario dispatch is required: the PR changes static CI/pre-commit hook configuration and non-e2e-scenario Vitest contract/unit tests, without touching .github/workflows/e2e-vitest-scenarios.yaml, test/e2e-scenario fixtures/runtime/scenario registry/live tests, or scenario-relevant application paths.

Optional Vitest E2E scenarios

None.

Relevant changed files

None.

github-actions · 2026-06-27T07:45:02Z

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 3 test follow-ups
Since last review: 0 prior items resolved · 3 still apply · 0 new items found

Action checklist

PRA-T1 Add or justify test follow-up: Runtime validation
PRA-T2 Add or justify test follow-up: Runtime validation
PRA-T3 Add or justify test follow-up: Runtime validation

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Runtime validation — Validate that `npx prek run --all-files --stage pre-commit --dry-run --no-progress` selects the expected static/security hooks, including detect-private-key, gitleaks, hadolint, validate-config-schemas, shellcheck, and no-force-added-ignored, while excluding pre-push-only tsc and version-sync hooks.. Changed behavior is CI/hook runner stage scheduling and a sandbox-adjacent test fixture. Static contract tests cover the intended YAML shape, but read-only review cannot prove prek's runtime stage resolver selects exactly the intended hooks.
PRA-T2 Runtime validation — Validate that `npx prek run --all-files --stage pre-push --dry-run --no-progress` keeps tsc-plugin, tsc-js, tsc-cli, and version-tag-sync in the pre-push stage after adding default_stages: [pre-commit].. Changed behavior is CI/hook runner stage scheduling and a sandbox-adjacent test fixture. Static contract tests cover the intended YAML shape, but read-only review cannot prove prek's runtime stage resolver selects exactly the intended hooks.
PRA-T3 Runtime validation — If avoiding runner invocation in automated tests, extend test/pr-workflow-contract.test.ts to assert critical security/static hooks are either stage-less under default_stages: [pre-commit] or explicitly stages: [pre-commit], and that the static CI skip list does not skip those hooks.. Changed behavior is CI/hook runner stage scheduling and a sandbox-adjacent test fixture. Static contract tests cover the intended YAML shape, but read-only review cannot prove prek's runtime stage resolver selects exactly the intended hooks.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

github-actions · 2026-06-27T07:45:09Z

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: No test validates production 512 nproc enforcement; then add or justify PRA-T1.
Open items: 3 required · 3 warnings · 2 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 4 still apply · 3 new items found

Action checklist

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-2`	Required	security	`test/sandbox-rlimit-hooks.test.ts:38`	Add a test case that sources the unmodified scripts/lib/sandbox-rlimits.sh (or a copy without the 4096 replacement) and asserts nproc limit enforcement at ≤ 512 via expectSystemRlimitHookEnforcesLimits.
`PRA-3`	Required	architecture	`test/sandbox-rlimit-hooks.test.ts:62`	Add a source-of-truth review comment block documenting all five fields: invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add the missing regression test (see PRA-1).
`PRA-4`	Required	correctness	`test/pr-workflow-contract.test.ts:183`	Add negative assertions: for each pre-push hook (tsc-plugin, tsc-js, tsc-cli, version-tag-sync), assert stages do not contain 'pre-commit'. For each pre-commit hook (trailing-whitespace, end-of-file-fixer, shfmt, check-added-large-files, check-executables-have-shebangs, check-shebang-scripts-are-executable), assert stages do not contain 'pre-push'.
`PRA-5`	Resolve/justify	correctness	`.pre-commit-config.yaml:36`	Add explicit `stages: [pre-commit]` to test-cli, test-plugin, source-shape-test-budget, test-file-size-budget, test-skills-yaml for clarity and to document intent. Add a contract test assertion that pre-push stage only contains the four expected hooks, validated by running `npx prek run --stage pre-push --dry-run --no-progress` in the test.
`PRA-6`	Resolve/justify	security	`.github/actions/ci-static-checks/action.yaml:25`	Add a contract test assertion that the union of hooks run by static-checks (pre-commit) and build-typecheck (pre-push) covers all security-relevant hooks, and that no hook runs in both stages unnecessarily.
`PRA-7`	Improvement	correctness	`test/sandbox-rlimit-hooks.test.ts:62`	Rename copyRlimitFixture to copyRlimitFixtureForTest or add @internal JSDoc to signal test-only usage.
`PRA-8`	Improvement	tests	`test/pr-workflow-contract.test.ts:177`	Add a spot-check in the test (or a separate integration test) that runs `npx prek run --stage pre-push --dry-run` and asserts only tsc-plugin, tsc-js, tsc-cli, version-tag-sync appear. Similarly for pre-commit stage.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

`PRA-2` Required — No test validates production 512 nproc enforcement

Location: test/sandbox-rlimit-hooks.test.ts:38
Category: security
Problem: All functional sandbox rlimit tests use copyRlimitFixture which overrides NEMOCLAW_SANDBOX_NPROC_LIMIT from 512 to 4096. The new test 'keeps the production nproc default at 512' only validates the default value in the source file, not that the limit is actually enforced at 512. This is a sandbox escape boundary — fork bomb mitigation depends on nproc ≤ 512.
Impact: A regression that breaks enforcement at 512 (e.g., conditional logic that only triggers at higher limits) would not be caught. Production sandbox could allow more processes than intended.
Required action: Add a test case that sources the unmodified scripts/lib/sandbox-rlimits.sh (or a copy without the 4096 replacement) and asserts nproc limit enforcement at ≤ 512 via expectSystemRlimitHookEnforcesLimits.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run the test suite and verify a test exists that does not call copyRlimitFixture (or calls it with limit=512) and passes expectSystemRlimitHookEnforcesLimits with nproc ≤ 512.
Missing regression test: Test case validating production 512 nproc enforcement without the test-only override
Done when: The required change is committed and verification passes: Run the test suite and verify a test exists that does not call copyRlimitFixture (or calls it with limit=512) and passes expectSystemRlimitHookEnforcesLimits with nproc ≤ 512.
Evidence: test/sandbox-rlimit-hooks.test.ts:38-41 overrides limit to 4096; scripts/lib/sandbox-rlimits.sh:8 has NEMOCLAW_SANDBOX_NPROC_LIMIT=512; all functional tests use copyRlimitFixture

`PRA-3` Required — Source-of-truth review incomplete for copyRlimitFixture workaround

Location: test/sandbox-rlimit-hooks.test.ts:62
Category: architecture
Problem: The copyRlimitFixture comment explains the invalid state (test flakiness under 16 Vitest workers sharing RLIMIT_NPROC) and source boundary (scripts/lib/sandbox-rlimits.sh:8), but does not document: (1) source-fix constraint — why production limit cannot be changed or test cannot run with fewer workers; (2) regression test that validates production 512 enforcement when not overridden; (3) removal condition — when this workaround can be removed.
Impact: Without a documented removal condition and regression test, this workaround can become permanent, silently reducing test fidelity to production sandbox behavior.
Required action: Add a source-of-truth review comment block documenting all five fields: invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add the missing regression test (see PRA-1).
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Check test/sandbox-rlimit-hooks.test.ts for a comment block with all five source-of-truth fields and a test validating 512 enforcement.
Missing regression test: Test case that sources unmodified scripts/lib/sandbox-rlimits.sh and asserts nproc limit enforcement at 512
Done when: The required change is committed and verification passes: Check test/sandbox-rlimit-hooks.test.ts for a comment block with all five source-of-truth fields and a test validating 512 enforcement.
Evidence: test/sandbox-rlimit-hooks.test.ts:62-68 has partial comment; missing source-fix constraint, regression test reference, removal condition

`PRA-4` Required — Contract test missing negative stage assertions

Location: test/pr-workflow-contract.test.ts:183
Category: correctness
Problem: New test 'keeps ordinary hooks in pre-commit and heavyweight push hooks explicit' only validates positive stage assignments (hooks ARE in expected stages). Missing negative assertions: pre-push hooks should NOT be in pre-commit stage, and pre-commit hooks should NOT be in pre-push stage. A future change adding pre-commit stage to a pre-push hook would not be caught.
Impact: Silent regression where a pre-push hook (e.g., tsc-plugin) accidentally runs at pre-commit, slowing down commits, or a pre-commit hook runs at pre-push, missing fast feedback.
Required action: Add negative assertions: for each pre-push hook (tsc-plugin, tsc-js, tsc-cli, version-tag-sync), assert stages do not contain 'pre-commit'. For each pre-commit hook (trailing-whitespace, end-of-file-fixer, shfmt, check-added-large-files, check-executables-have-shebangs, check-shebang-scripts-are-executable), assert stages do not contain 'pre-push'.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run npx vitest run test/pr-workflow-contract.test.ts — current test passes. Add negative assertions and verify they pass.
Missing regression test: Negative stage membership assertions for all hooks in the contract test
Done when: The required change is committed and verification passes: Run npx vitest run test/pr-workflow-contract.test.ts — current test passes. Add negative assertions and verify they pass.
Evidence: test/pr-workflow-contract.test.ts:177-210 only has positive stage assertions

Review findings by urgency: 3 required fixes, 3 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

`PRA-1` Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: Missing. Need a test that sources unmodified sandbox-rlimits.sh and asserts nproc ≤ 512 enforcement.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: test/sandbox-rlimit-hooks.test.ts:62-68 has partial comment; missing source-fix constraint, regression test reference, removal condition

`PRA-5` Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

Location: .pre-commit-config.yaml:36
Category: correctness
Problem: Adding default_stages: [pre-commit] changes behavior: hooks without explicit stages now only run at pre-commit (previously ran at all stages). Priority 20 hooks (test-cli, test-plugin, source-shape-test-budget, test-file-size-budget, test-skills-yaml) have no explicit stages and inherit pre-commit. The test validates they have undefined stages but does not verify they don't run at pre-push. No explicit verification that only the four intended pre-push hooks execute at push time.
Impact: If any hook relied on running at pre-push without explicit stages, it would silently stop running at push time. Conversely, a future change could add pre-push to a test hook without detection.
Recommended action: Add explicit `stages: [pre-commit]` to test-cli, test-plugin, source-shape-test-budget, test-file-size-budget, test-skills-yaml for clarity and to document intent. Add a contract test assertion that pre-push stage only contains the four expected hooks, validated by running `npx prek run --stage pre-push --dry-run --no-progress` in the test.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` and confirm output shows only tsc-plugin, tsc-js, tsc-cli, version-tag-sync.
Missing regression test: Contract test assertion that pre-push dry-run output matches exactly the four expected hooks
Done when: The risk is fixed or explicitly justified in the PR. Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` and confirm output shows only tsc-plugin, tsc-js, tsc-cli, version-tag-sync.
Evidence: .pre-commit-config.yaml:36 adds default_stages; hooks at lines 238-263 have explicit stages: [pre-push]; priority 20 hooks have no stages

`PRA-6` Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

Location: .github/actions/ci-static-checks/action.yaml:25
Category: security
Problem: CI static checks now run only pre-commit stage hooks (skipping test hooks). The build-typecheck job runs pre-push hooks (tsc-plugin, tsc-js, tsc-cli, version-tag-sync). This separation is correct but the security posture depends on both jobs running. No test validates that the combined PR workflow still executes all security-relevant hooks across both jobs.
Impact: If build-typecheck job is skipped or fails silently, TypeScript type-checking (a security-relevant gate for sandbox/SSRF boundaries) is missed. No test asserts union of hooks covers all security-relevant hooks.
Recommended action: Add a contract test assertion that the union of hooks run by static-checks (pre-commit) and build-typecheck (pre-push) covers all security-relevant hooks, and that no hook runs in both stages unnecessarily.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Review .github/workflows/pr.yaml jobs static-checks and build-typecheck to confirm both run and cover disjoint hook sets. Add test assertion.
Missing regression test: Contract test validating complete hook coverage across PR workflow jobs
Done when: The risk is fixed or explicitly justified in the PR. Verification: Review .github/workflows/pr.yaml jobs static-checks and build-typecheck to confirm both run and cover disjoint hook sets. Add test assertion.
Evidence: .github/workflows/pr.yaml lines 30-70 (static-checks) and 72-100 (build-typecheck); .github/actions/ci-build-typecheck/action.yaml runs TypeScript checks

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

`PRA-7` Improvement — copyRlimitFixture function name does not indicate test-only usage

Location: test/sandbox-rlimit-hooks.test.ts:62
Category: correctness
Problem: The copyRlimitFixture function has a comment explaining the TEST-ONLY override rationale, but the function name does not indicate it's test-only. This could encourage accidental production use.
Impact: Low — function is in test file and not exported, but naming clarity improves maintainability.
Suggested action: Rename copyRlimitFixture to copyRlimitFixtureForTest or add @internal JSDoc to signal test-only usage.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Check function name and JSDoc in test/sandbox-rlimit-hooks.test.ts.
Missing regression test: None - code clarity improvement
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: test/sandbox-rlimit-hooks.test.ts:62 function copyRlimitFixture(rlimitLib: string): void

`PRA-8` Improvement — Contract test does not validate actual prek dry-run behavior

Location: test/pr-workflow-contract.test.ts:177
Category: tests
Problem: The new contract test validates stage assignments in YAML but does not verify that hooks with explicit stages match the actual prek config behavior under dry-run. A drift between config and test expectations could go undetected.
Impact: Config/test drift would not be caught until a developer runs hooks locally and observes unexpected behavior.
Suggested action: Add a spot-check in the test (or a separate integration test) that runs `npx prek run --stage pre-push --dry-run` and asserts only tsc-plugin, tsc-js, tsc-cli, version-tag-sync appear. Similarly for pre-commit stage.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` manually and compare with test expectations.
Missing regression test: Integration test validating actual prek dry-run output matches contract test expectations
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: test/pr-workflow-contract.test.ts:177-210 validates YAML only; no runtime validation

Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

PRA-7 shrink (test/sandbox-rlimit-hooks.test.ts:62): function copyRlimitFixture(rlimitLib: string): void
- Replacement: function copyRlimitFixtureForTest(rlimitLib: string): void
- Net: 0 lines
- Safety boundary: Test file only; no production code affected

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Runtime validation — Run npx prek run --stage pre-push --dry-run --no-progress and assert only tsc-plugin, tsc-js, tsc-cli, version-tag-sync appear. Changed files are CI action and hook config; behavior must be validated by running actual hooks. Static tests cover YAML structure but not runtime hook execution.
PRA-T2 Runtime validation — Run npx prek run --stage pre-commit --dry-run --no-progress and assert pre-push hooks are excluded. Changed files are CI action and hook config; behavior must be validated by running actual hooks. Static tests cover YAML structure but not runtime hook execution.
PRA-T3 Runtime validation — Add test sourcing unmodified scripts/lib/sandbox-rlimits.sh and asserting nproc ≤ 512 enforcement. Changed files are CI action and hook config; behavior must be validated by running actual hooks. Static tests cover YAML structure but not runtime hook execution.
PRA-T4 Runtime validation — Add negative stage membership assertions to contract test for all hooks. Changed files are CI action and hook config; behavior must be validated by running actual hooks. Static tests cover YAML structure but not runtime hook execution.
PRA-T5 Contract test does not validate actual prek dry-run behavior — Add a spot-check in the test (or a separate integration test) that runs `npx prek run --stage pre-push --dry-run` and asserts only tsc-plugin, tsc-js, tsc-cli, version-tag-sync appear. Similarly for pre-commit stage.
PRA-T6 Acceptance clause — Validate stage assignments in contract test — add test evidence or identify existing coverage. New test validates positive assignments but missing negative assertions (pre-push hooks ∉ pre-commit, pre-commit hooks ∉ pre-push)
PRA-T7 Acceptance clause — Validate production 512 enforcement — add test evidence or identify existing coverage. No test validates enforcement at 512; all functional tests use 4096 override via copyRlimitFixture
PRA-T8 Acceptance clause — Source-of-truth review for localized workaround — add test evidence or identify existing coverage. Missing source-fix constraint, regression test for 512 enforcement, and removal condition in copyRlimitFixture comment

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: Missing. Need a test that sources unmodified sandbox-rlimits.sh and asserts nproc ≤ 512 enforcement.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: test/sandbox-rlimit-hooks.test.ts:62-68 has partial comment; missing source-fix constraint, regression test reference, removal condition

`PRA-2` Required — No test validates production 512 nproc enforcement

Location: test/sandbox-rlimit-hooks.test.ts:38
Category: security
Problem: All functional sandbox rlimit tests use copyRlimitFixture which overrides NEMOCLAW_SANDBOX_NPROC_LIMIT from 512 to 4096. The new test 'keeps the production nproc default at 512' only validates the default value in the source file, not that the limit is actually enforced at 512. This is a sandbox escape boundary — fork bomb mitigation depends on nproc ≤ 512.
Impact: A regression that breaks enforcement at 512 (e.g., conditional logic that only triggers at higher limits) would not be caught. Production sandbox could allow more processes than intended.
Required action: Add a test case that sources the unmodified scripts/lib/sandbox-rlimits.sh (or a copy without the 4096 replacement) and asserts nproc limit enforcement at ≤ 512 via expectSystemRlimitHookEnforcesLimits.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run the test suite and verify a test exists that does not call copyRlimitFixture (or calls it with limit=512) and passes expectSystemRlimitHookEnforcesLimits with nproc ≤ 512.
Missing regression test: Test case validating production 512 nproc enforcement without the test-only override
Done when: The required change is committed and verification passes: Run the test suite and verify a test exists that does not call copyRlimitFixture (or calls it with limit=512) and passes expectSystemRlimitHookEnforcesLimits with nproc ≤ 512.
Evidence: test/sandbox-rlimit-hooks.test.ts:38-41 overrides limit to 4096; scripts/lib/sandbox-rlimits.sh:8 has NEMOCLAW_SANDBOX_NPROC_LIMIT=512; all functional tests use copyRlimitFixture

`PRA-3` Required — Source-of-truth review incomplete for copyRlimitFixture workaround

Location: test/sandbox-rlimit-hooks.test.ts:62
Category: architecture
Problem: The copyRlimitFixture comment explains the invalid state (test flakiness under 16 Vitest workers sharing RLIMIT_NPROC) and source boundary (scripts/lib/sandbox-rlimits.sh:8), but does not document: (1) source-fix constraint — why production limit cannot be changed or test cannot run with fewer workers; (2) regression test that validates production 512 enforcement when not overridden; (3) removal condition — when this workaround can be removed.
Impact: Without a documented removal condition and regression test, this workaround can become permanent, silently reducing test fidelity to production sandbox behavior.
Required action: Add a source-of-truth review comment block documenting all five fields: invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add the missing regression test (see PRA-1).
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Check test/sandbox-rlimit-hooks.test.ts for a comment block with all five source-of-truth fields and a test validating 512 enforcement.
Missing regression test: Test case that sources unmodified scripts/lib/sandbox-rlimits.sh and asserts nproc limit enforcement at 512
Done when: The required change is committed and verification passes: Check test/sandbox-rlimit-hooks.test.ts for a comment block with all five source-of-truth fields and a test validating 512 enforcement.
Evidence: test/sandbox-rlimit-hooks.test.ts:62-68 has partial comment; missing source-fix constraint, regression test reference, removal condition

`PRA-4` Required — Contract test missing negative stage assertions

Location: test/pr-workflow-contract.test.ts:183
Category: correctness
Problem: New test 'keeps ordinary hooks in pre-commit and heavyweight push hooks explicit' only validates positive stage assignments (hooks ARE in expected stages). Missing negative assertions: pre-push hooks should NOT be in pre-commit stage, and pre-commit hooks should NOT be in pre-push stage. A future change adding pre-commit stage to a pre-push hook would not be caught.
Impact: Silent regression where a pre-push hook (e.g., tsc-plugin) accidentally runs at pre-commit, slowing down commits, or a pre-commit hook runs at pre-push, missing fast feedback.
Required action: Add negative assertions: for each pre-push hook (tsc-plugin, tsc-js, tsc-cli, version-tag-sync), assert stages do not contain 'pre-commit'. For each pre-commit hook (trailing-whitespace, end-of-file-fixer, shfmt, check-added-large-files, check-executables-have-shebangs, check-shebang-scripts-are-executable), assert stages do not contain 'pre-push'.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run npx vitest run test/pr-workflow-contract.test.ts — current test passes. Add negative assertions and verify they pass.
Missing regression test: Negative stage membership assertions for all hooks in the contract test
Done when: The required change is committed and verification passes: Run npx vitest run test/pr-workflow-contract.test.ts — current test passes. Add negative assertions and verify they pass.
Evidence: test/pr-workflow-contract.test.ts:177-210 only has positive stage assertions

`PRA-5` Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

Location: .pre-commit-config.yaml:36
Category: correctness
Problem: Adding default_stages: [pre-commit] changes behavior: hooks without explicit stages now only run at pre-commit (previously ran at all stages). Priority 20 hooks (test-cli, test-plugin, source-shape-test-budget, test-file-size-budget, test-skills-yaml) have no explicit stages and inherit pre-commit. The test validates they have undefined stages but does not verify they don't run at pre-push. No explicit verification that only the four intended pre-push hooks execute at push time.
Impact: If any hook relied on running at pre-push without explicit stages, it would silently stop running at push time. Conversely, a future change could add pre-push to a test hook without detection.
Recommended action: Add explicit `stages: [pre-commit]` to test-cli, test-plugin, source-shape-test-budget, test-file-size-budget, test-skills-yaml for clarity and to document intent. Add a contract test assertion that pre-push stage only contains the four expected hooks, validated by running `npx prek run --stage pre-push --dry-run --no-progress` in the test.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` and confirm output shows only tsc-plugin, tsc-js, tsc-cli, version-tag-sync.
Missing regression test: Contract test assertion that pre-push dry-run output matches exactly the four expected hooks
Done when: The risk is fixed or explicitly justified in the PR. Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` and confirm output shows only tsc-plugin, tsc-js, tsc-cli, version-tag-sync.
Evidence: .pre-commit-config.yaml:36 adds default_stages; hooks at lines 238-263 have explicit stages: [pre-push]; priority 20 hooks have no stages

`PRA-6` Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

Location: .github/actions/ci-static-checks/action.yaml:25
Category: security
Problem: CI static checks now run only pre-commit stage hooks (skipping test hooks). The build-typecheck job runs pre-push hooks (tsc-plugin, tsc-js, tsc-cli, version-tag-sync). This separation is correct but the security posture depends on both jobs running. No test validates that the combined PR workflow still executes all security-relevant hooks across both jobs.
Impact: If build-typecheck job is skipped or fails silently, TypeScript type-checking (a security-relevant gate for sandbox/SSRF boundaries) is missed. No test asserts union of hooks covers all security-relevant hooks.
Recommended action: Add a contract test assertion that the union of hooks run by static-checks (pre-commit) and build-typecheck (pre-push) covers all security-relevant hooks, and that no hook runs in both stages unnecessarily.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Review .github/workflows/pr.yaml jobs static-checks and build-typecheck to confirm both run and cover disjoint hook sets. Add test assertion.
Missing regression test: Contract test validating complete hook coverage across PR workflow jobs
Done when: The risk is fixed or explicitly justified in the PR. Verification: Review .github/workflows/pr.yaml jobs static-checks and build-typecheck to confirm both run and cover disjoint hook sets. Add test assertion.
Evidence: .github/workflows/pr.yaml lines 30-70 (static-checks) and 72-100 (build-typecheck); .github/actions/ci-build-typecheck/action.yaml runs TypeScript checks

`PRA-7` Improvement — copyRlimitFixture function name does not indicate test-only usage

Location: test/sandbox-rlimit-hooks.test.ts:62
Category: correctness
Problem: The copyRlimitFixture function has a comment explaining the TEST-ONLY override rationale, but the function name does not indicate it's test-only. This could encourage accidental production use.
Impact: Low — function is in test file and not exported, but naming clarity improves maintainability.
Suggested action: Rename copyRlimitFixture to copyRlimitFixtureForTest or add @internal JSDoc to signal test-only usage.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Check function name and JSDoc in test/sandbox-rlimit-hooks.test.ts.
Missing regression test: None - code clarity improvement
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: test/sandbox-rlimit-hooks.test.ts:62 function copyRlimitFixture(rlimitLib: string): void

`PRA-8` Improvement — Contract test does not validate actual prek dry-run behavior

Location: test/pr-workflow-contract.test.ts:177
Category: tests
Problem: The new contract test validates stage assignments in YAML but does not verify that hooks with explicit stages match the actual prek config behavior under dry-run. A drift between config and test expectations could go undetected.
Impact: Config/test drift would not be caught until a developer runs hooks locally and observes unexpected behavior.
Suggested action: Add a spot-check in the test (or a separate integration test) that runs `npx prek run --stage pre-push --dry-run` and asserts only tsc-plugin, tsc-js, tsc-cli, version-tag-sync appear. Similarly for pre-commit stage.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Run `npx prek run --stage pre-push --dry-run --no-progress` manually and compare with test expectations.
Missing regression test: Integration test validating actual prek dry-run output matches contract test expectations
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: test/pr-workflow-contract.test.ts:177-210 validates YAML only; no runtime validation

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

cv · 2026-06-27T07:47:56Z

Automated review follow-up:

PRA-1, PRA-2, PRA-T4: added an explicit test-only override comment and a regression assertion that the production source default remains exactly 512. The behavioral tests execute the same production shell logic at 4096. They intentionally do not source it at 512: RLIMIT_NPROC is per real UID, so doing that inside a 16-worker suite recreates the shared-host failure this change fixes. The localized fixture override can be removed if these tests run under an isolated UID or cgroup.
PRA-T1: npx prek run --all-files --stage pre-commit passed, exercising the selected static/security hooks rather than merely dry-running them.
PRA-T2: npx prek run --all-files --stage pre-push --dry-run --no-progress selected exactly tsc-plugin, tsc-js, tsc-cli, and version-tag-sync.
PRA-T3: both signed commit and push hooks passed with VITEST_MAX_WORKERS=16.
PRA-3: no code change. Both stage groups are asserted with exact array equality (["pre-commit"] or ["pre-push"]), which already fails if the opposite stage is added; extra negative assertions would be redundant.
PRA-4: added the CI ownership comment.
PRA-5: no explicit stages added to the local test hooks. Their inheritance is intentional and directly asserted; duplicating the default on every hook would create two sources of truth. The pre-push runtime validation above confirms that only the four intended hooks remain there.

Follow-up commit 57e2d2393 is signed, GitHub-verified, and passed the full 16-worker commit gate plus the isolated pre-push gate.

cv · 2026-06-27T07:51:47Z

Maintainer rationale for the latest Nemotron findings (57e2d2393): I am not taking the three requested code changes.

PRA-2 / PRA-3: enforcing RLIMIT_NPROC=512 in this shared-host test is precisely what made the 16-worker gate intermittently unable to fork. User namespaces are unavailable on this runner, so there is no portable isolated UID in which to execute that assertion. The durable split is intentional: one test ratchets the production source default to exactly 512, while the behavioral tests execute the same unmodified enforcement logic with a concurrency-safe bounded value. Raising the production limit or reducing the suite to four workers would fix the test rather than the architecture.
PRA-4: expect(stages).toEqual(["pre-push"]) necessarily fails for ["pre-commit", "pre-push"]; the same is true in the other direction. Adding not.toContain(...) after exact equality cannot detect any additional state and would only add boilerplate.
PRA-5 / PRA-8: leaving local test hooks stage-less deliberately tests the single default_stages source of truth. Actual prek resolution was validated both by dry-run and by the signed commit/push lifecycle. Running prek recursively inside the Vitest contract suite would couple a fast YAML contract test to an external runner and duplicate the lifecycle acceptance test.
PRA-6: existing contract coverage already requires both static-checks and build-typecheck in the aggregate PR and main gates, asserts both shared actions are wired into the workflows, and checks their commands. There is no uncovered security path to model as a synthetic union.

The GPT advisor reports no blocking findings, CodeRabbit has no inline findings, all completed required checks are green, and the remaining checks are runtime E2E/coverage aggregation.

## Summary Make prek's lifecycle stages explicit so pre-commit owns formatting, linting, and tests while pre-push owns typechecking and version synchronization. This keeps local hooks and static CI aligned without running the same expensive work twice. ## Changes - Set `pre-commit` as the default stage and pin upstream hooks that opt into multiple stages to pre-commit. - Run the static-checks action against the pre-commit stage; keep TypeScript and version-sync hooks exclusively on pre-push. - Add a workflow contract test that ratchets the intended stage ownership. - Raise only the copied rlimit test fixture's process cap to 4096 so 16-worker Vitest runs cannot starve their own shell; the production cap remains 512. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal hook and CI stage scheduling only; the stage boundary is documented by an executable contract test. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Targeted verification: - `VITEST_MAX_WORKERS=16 npx vitest run --project cli test/pr-workflow-contract.test.ts test/sandbox-rlimit-hooks.test.ts` - `npx prek validate-config .pre-commit-config.yaml` - `npx prek run --all-files --stage pre-push --dry-run --no-progress` - Signed commit and push hooks with `VITEST_MAX_WORKERS=16` --- Signed-off-by: Carlos Villela <cvillela@nvidia.com>  ## Summary by CodeRabbit * **Bug Fixes** * Updated CI static hook checks to run during the `pre-commit` lifecycle (instead of `pre-push`) and aligned related hook skipping behavior. * Improved sandbox test stability by standardizing the overridden `NPROC` limit during test setup. * **Tests** * Enhanced pre-commit contract tests to validate `default_stages` and confirm which hooks are explicitly pinned to `pre-commit` vs `pre-push`. * Added coverage ensuring the production `sandbox-rlimits.sh` fixture retains its default `NPROC` limit.  --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>

ci(hooks): separate pre-commit and pre-push stages

69cc96f

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

test(ci): clarify hook stage safeguards

57e2d23

Signed-off-by: Carlos Villela <cvillela@nvidia.com>

cv merged commit d2de3fc into main Jun 27, 2026
45 checks passed

cv deleted the codex/separate-prek-hook-stages branch June 27, 2026 07:57

cv added the v0.0.69 label Jun 27, 2026

wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions chore Build, CI, dependency, or tooling maintenance labels Aug 1, 2026

Uh oh!

Conversation

cv commented Jun 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Type of Change

Quality Gates

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

❌ Failed checks (1 warning)

Uh oh!

github-code-quality Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Vitest E2E Scenario Recommendation

Vitest E2E Scenario Advisor

Required Vitest E2E scenarios

Optional Vitest E2E scenarios

Relevant changed files

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor — No blocking findings

Action checklist

Uh oh!

github-actions Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor (Nemotron Ultra) — Changes requested

Action checklist

Findings index

🚨 Required before merge

PRA-2 Required — No test validates production 512 nproc enforcement

PRA-3 Required — Source-of-truth review incomplete for copyRlimitFixture workaround

PRA-4 Required — Contract test missing negative stage assertions

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

PRA-5 Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

PRA-6 Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

💡 In-scope improvements

PRA-7 Improvement — copyRlimitFixture function name does not indicate test-only usage

PRA-8 Improvement — Contract test does not validate actual prek dry-run behavior

PRA-1 Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

PRA-2 Required — No test validates production 512 nproc enforcement

PRA-3 Required — Source-of-truth review incomplete for copyRlimitFixture workaround

PRA-4 Required — Contract test missing negative stage assertions

PRA-5 Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

PRA-6 Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

PRA-7 Improvement — copyRlimitFixture function name does not indicate test-only usage

PRA-8 Improvement — Contract test does not validate actual prek dry-run behavior

Uh oh!

cv commented Jun 27, 2026

Uh oh!

cv commented Jun 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

cv commented Jun 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 27, 2026 •

edited

Loading

github-code-quality Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

github-actions Bot commented Jun 27, 2026 •

edited

Loading

`PRA-2` Required — No test validates production 512 nproc enforcement

`PRA-3` Required — Source-of-truth review incomplete for copyRlimitFixture workaround

`PRA-4` Required — Contract test missing negative stage assertions

`PRA-1` Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

`PRA-5` Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

`PRA-6` Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

`PRA-7` Improvement — copyRlimitFixture function name does not indicate test-only usage

`PRA-8` Improvement — Contract test does not validate actual prek dry-run behavior

`PRA-1` Resolve/justify — Source-of-truth review needed: test/sandbox-rlimit-hooks.test.ts copyRlimitFixture nproc override

`PRA-2` Required — No test validates production 512 nproc enforcement

`PRA-3` Required — Source-of-truth review incomplete for copyRlimitFixture workaround

`PRA-4` Required — Contract test missing negative stage assertions

`PRA-5` Resolve/justify — default_stages changes behavior for hooks without explicit stages; no dry-run verification

`PRA-6` Resolve/justify — No contract test validates complete hook coverage across PR workflow jobs

`PRA-7` Improvement — copyRlimitFixture function name does not indicate test-only usage

`PRA-8` Improvement — Contract test does not validate actual prek dry-run behavior