fix(platform): close remaining Vitest main failures - #6829
Merged
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds deadline-bounded process inspection and identity-pinned shields takeover handling, switches gateway shutdown authorization from usernames to numeric UIDs, improves Hermes fixture permission cleanup, and adds ChangesShields takeover control flow
Gateway UID authorization
Hermes fixture permission cleanup
macOS dependency verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShieldsFlow
participant waitForShieldsDownForwardCommit
participant timer-control
participant stopTimedOutShieldsDownTree
participant ProcessIdentity
ShieldsFlow->>waitForShieldsDownForwardCommit: await forward commit
waitForShieldsDownForwardCommit->>timer-control: read exact process status
timer-control->>ProcessIdentity: inspect identity and state
waitForShieldsDownForwardCommit->>stopTimedOutShieldsDownTree: stop pinned owner
stopTimedOutShieldsDownTree->>ProcessIdentity: enumerate and signal descendants
ProcessIdentity-->>stopTimedOutShieldsDownTree: verified stop or kill status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 3 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
changed the title
cv
approved these changes
Jul 14, 2026
wscurran
added
area: ci
Contributor
|
✨ Related open issues: Related open PRs: Related open issues: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/shields/timer-control.ts (1)
173-181: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
ps -o lstart=is not a safe exact-identity fallback on macOS.readProcessStartIdentity()falls back tops -o lstart=, which only yields second-level timestamps there. A PID reused within the same second can produce the same identity and slip past the exact-process checks that guard the shields takeover/restore paths. Treat this fallback as unverifiable on macOS or replace it with a collision-resistant process-start identity.🤖 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 `@src/lib/shields/timer-control.ts` around lines 173 - 181, The readProcessStartIdentity fallback using ps -o lstart= is not sufficiently precise on macOS and must not be used as an exact identity there. Replace it with a collision-resistant process-start identity or return it as unverifiable on macOS, while preserving exact-process checks on supported platforms; update the related coverage in src/lib/shields/flow.test.ts lines 678-717 to assert the revised macOS behavior.Source: Path instructions
🤖 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 `@src/lib/shields/index.ts`:
- Around line 331-338: Update excludeRecoveryProcessTree and its caller to match
excluded processes using both PID and startIdentity rather than PID alone;
ensure recoveryTree entries retain and compare that identity. Preserve
fail-closed behavior when identity data is missing or mismatched, preventing PID
reuse from bypassing recovery-child exclusion.
In `@src/lib/shields/timer-control.ts`:
- Around line 13-19: Replace wall-clock timing with performance.now() for
processInspectionDeadline and remainingProcessInspectionTimeout in
src/lib/shields/timer-control.ts, and use the same monotonic source for handoff
polling in src/lib/shields/index.ts lines 230-234 and all freeze/kill deadlines
and comparisons in lines 269-375. Preserve bounded, fail-closed timeout behavior
consistently across every affected site.
In `@test/helpers/hermes-restart-config-seal-fixture.ts`:
- Around line 98-103: Make allowRestartFixturePeerTraversal failure-atomic by
restoring the original parent directory mode if either permission update fails.
In test/hermes-restart-config-seal-recovery.test.ts lines 29-30 and 329-330,
invoke this helper within try/finally, conditionally execute the returned
teardown callback, and ensure cleanup remains registered even when setup throws.
---
Outside diff comments:
In `@src/lib/shields/timer-control.ts`:
- Around line 173-181: The readProcessStartIdentity fallback using ps -o lstart=
is not sufficiently precise on macOS and must not be used as an exact identity
there. Replace it with a collision-resistant process-start identity or return it
as unverifiable on macOS, while preserving exact-process checks on supported
platforms; update the related coverage in src/lib/shields/flow.test.ts lines
678-717 to assert the revised macOS behavior.
🪄 Autofix (Beta)
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: 6cd8d0a3-1be4-4a9c-8201-aaff98598b8c
📒 Files selected for processing (10)
.github/workflows/platform-vitest-main.yamlsrc/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/timer-control.tssrc/lib/tunnel/gateway-stop-script.test.tssrc/lib/tunnel/gateway-stop-script.tssrc/lib/tunnel/sandbox-gateway-stop.test.tstest/helpers/hermes-restart-config-seal-fixture.tstest/hermes-restart-config-seal-recovery.test.tstest/platform-vitest-main-workflow.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/shields/timer-control.test.ts`:
- Line 13: Update the test title in the “remain bounded when the wall clock
moves backward” case to append the applicable owning issue or PR reference as
the final suffix, following the repository’s parenthesized format such as
“(`#6829`)”.
In `@test/hermes-restart-config-seal-recovery.test.ts`:
- Around line 33-36: Update the chmodSync spy setup in the test to remove the
chmodCalls counter and conditional branch. Use chained mockImplementationOnce
calls so the first invocation delegates to realChmodSync and only the second
invocation throws the fixture chmod failure, preserving the existing test
behavior without adding an if statement.
🪄 Autofix (Beta)
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: a2c2f251-8e93-4896-8576-6e916ae57cb3
📒 Files selected for processing (6)
src/lib/shields/flow.test.tssrc/lib/shields/index.tssrc/lib/shields/timer-control.test.tssrc/lib/shields/timer-control.tstest/helpers/hermes-restart-config-seal-fixture.tstest/hermes-restart-config-seal-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/helpers/hermes-restart-config-seal-fixture.ts
- src/lib/shields/timer-control.ts
- src/lib/shields/index.ts
- src/lib/shields/flow.test.ts
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("remain bounded when the wall clock moves backward", () => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the issue reference to the test title.
Append the owning issue or PR reference as a final suffix, for example (#6829) if applicable, to satisfy the repository’s test-title guideline.
🤖 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 `@src/lib/shields/timer-control.test.ts` at line 13, Update the test title in
the “remain bounded when the wall clock moves backward” case to append the
applicable owning issue or PR reference as the final suffix, following the
repository’s parenthesized format such as “(`#6829`)”.
Source: Coding guidelines
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
9 tasks
22 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the remaining failures exposed by the Platform Vitest main watch across WSL and macOS. The patch fixes the original WSL gateway-owner mismatch, supplies the missing macOS fd dependency, makes Shields timer takeover fail closed around PID reuse and late descendants, and makes the WSL root-only Hermes fixtures traversable by the intended peer without weakening their contents.
Related Issue
Follow-up to #6825; preserves the bare-gateway stop behavior from #4951.
Changes
Type of Change
Quality Gates
Verification
Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
fdto macOS CI dependency installation.