fix(runtime): harden overrides and gateway recovery - #2874
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughTightens model-override numeric validation to require positive integers; constructs a shell-safe gateway process-match pattern; regenerates recovery scripts to TERM/KILL stale gateway processes with bounded retries and hard-fail if persistent; strengthens guard verification to abort when required NODE_OPTIONS preloads are missing. ChangesGateway Safety Hardening
Model Override Validation
Agent Onboarding Guarding
Shields / Permissions
Sequence DiagramsequenceDiagram
participant User
participant ValidatorScript as Validator Script
participant RecoveryGen as Recovery Script<br/>Generator
participant ProcessMgr as Process Manager<br/>(pkill/pgrep)
participant EnvFile as Env File<br/>(/tmp/nemoclaw-proxy-env.sh)
participant Guards as Guard Verifier
participant GatewayBin as Gateway Binary
User->>ValidatorScript: apply_model_override(CONTEXT_WINDOW, MAX_TOKENS)
ValidatorScript->>ValidatorScript: Validate positive integers
alt Invalid
ValidatorScript->>User: SECURITY error / return 0
else Valid
ValidatorScript->>RecoveryGen: Regenerate recovery script
RecoveryGen->>RecoveryGen: Compute safe _GATEWAY_PROC_PATTERN from gateway_command
RecoveryGen->>User: Write recovery script (includes pattern)
RecoveryGen->>ProcessMgr: Recovery script runs -> pkill -TERM matching pattern
ProcessMgr->>ProcessMgr: Retry TERM, then pkill -KILL if needed
alt Stale processes persist
ProcessMgr->>User: Exit with GATEWAY_STALE_PROCESSES
else Stale processes gone
ProcessMgr->>EnvFile: Source /tmp/nemoclaw-proxy-env.sh
EnvFile->>Guards: Provide NODE_OPTIONS
Guards->>Guards: Verify preload guard patterns present
alt Guards missing
Guards->>User: ERROR and exit 1 (refuse relaunch)
else Guards present
Guards->>GatewayBin: Allow launch (nohup ...)
end
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25240057471
|
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/agent-runtime.ts`:
- Around line 79-82: The stale-process regex is built from just the executable
name (gatewayExecutableName) which assumes the literal token "gateway" follows
the binary; for custom gateway_command strings with arguments this produces a
non-matching pattern. Update the logic that computes staleGatewayPattern (and
calls selfSafeGatewayProcessPattern) to derive the pattern from the full custom
command string (customGatewayExecutable) rather than only the basename: parse
the custom command into executable + remaining args and pass the full command
shape (or at least the executable plus a pattern that allows arbitrary
intervening args/flags) into selfSafeGatewayProcessPattern so it matches the
actual launched process (e.g., allow any args between the executable and
eventual tokens or end-of-line). Modify selfSafeGatewayProcessPattern or its
caller accordingly (referencing gatewayExecutableName, customGatewayExecutable,
and selfSafeGatewayProcessPattern) so pkill/pgrep can correctly find stale
custom-launch processes.
🪄 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: 64f8c0e8-add5-49fc-86be-bf4eae6127fb
📒 Files selected for processing (5)
scripts/nemoclaw-start.shsrc/lib/agent-runtime.test.tssrc/lib/agent-runtime.tssrc/nemoclaw.tstest/nemoclaw-start.test.ts
ericksoa
changed the title
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25241511224
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25241683621
|
ericksoa
added
dependencies
Contributor
Author
Automated PR review summaryReviewed PR #2874: fix(runtime): harden overrides and gateway recovery Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Custom gateway stale-process pattern matches real full command and TERM alone does not clear a resistant stale process
Passing test 2: Recovery refuses relaunch when proxy env exists but required NODE_OPTIONS guards are missing
Passing test 3: Invalid supplemental overrides remain atomic no-ops without config or hash mutation
Bottom line
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/repro-2681-group-writable.test.ts`:
- Around line 204-210: The test currently checks commands contains
["chmod","755","/sandbox/.openclaw"] which may match an earlier chmod; change it
to locate the index of that exact chmod (e.g. findIndex over commands comparing
command.join("\0") to ["chmod","755","/sandbox/.openclaw"].join("\0")) and
assert that this chmod755Index is greater than stripSetgidIndex (and still
greater than stateDirLockIndex if needed) so the post-strip ordering is
enforced; use the existing commands array and the
stripSetgidIndex/stateDirLockIndex symbols to implement this assertion.
🪄 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: b60d4344-6aff-4fab-8ef6-2fb6f9dec0c0
📒 Files selected for processing (3)
src/lib/shields.tstest/e2e/test-shields-config.shtest/repro-2681-group-writable.test.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25241876578
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25242649869
|
…ssions # Conflicts: # src/lib/agent-onboard.test.ts # src/lib/agent-onboard.ts # src/lib/agent-runtime.test.ts # src/lib/agent-runtime.ts # src/nemoclaw.ts # test/e2e/test-shields-config.sh
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25243698352
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25244507003
|
wscurran
added
chore
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
2 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
Follow-up
Validation
Notes
Summary by CodeRabbit
Bug Fixes
New Features
Tests