fix: repair nightly Hermes and shields regressions - #2877
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughThis PR standardizes file permissions for the sandbox mutable configuration directory from a mixed scheme to a consistent setgid-enabled group-writable model (2770 on directories, 660 on files). It also adds Hermes venv binaries to the shell initialization PATH, and refactors permission-locking logic to conditionally strip setgid during config immutability. ChangesShell Environment & PATH Setup
Mutable Config Permission Standardization
Sequence Diagram(s)sequenceDiagram
participant Startup as Non-Root Startup
participant ConfigVerify as Config Verification
participant Normalize as normalize_mutable_config_perms()
participant Lock as lockAgentConfig()
participant FSState as Filesystem State
Startup->>ConfigVerify: verify_config_integrity_if_locked()
ConfigVerify->>FSState: Check /sandbox/.openclaw
ConfigVerify->>Startup: Integrity valid or repaired
Startup->>Normalize: normalize_mutable_config_perms()
Normalize->>FSState: chmod -R g+rwX,o-rwx
Normalize->>FSState: find -exec chmod g+s (dirs)
Normalize->>FSState: chmod 2770 /sandbox/.openclaw
Normalize->>FSState: chmod 660 on config files
Normalize->>Startup: Permissions normalized
Startup->>Lock: shields up (on demand)
Lock->>FSState: applyStateDirLockMode (locking=true)
Lock->>FSState: chmod g-s /sandbox/.openclaw (clear setgid)
Lock->>FSState: chmod 755 /sandbox/.openclaw
Lock->>Startup: Config locked (immutable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans multiple interconnected subsystems (shell initialization, permission normalization, lock/unlock semantics) with heterogeneous logic changes. The permission model refactoring requires careful validation across startup paths, container defaults, and lock state transitions. Test expansions are substantial and logic-dense, particularly for conditional setgid handling and workspace inheritance patterns. 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
…ields-regressions # Conflicts: # test/repro-2681-group-writable.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.ts (1)
270-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the same mutable-default mode that startup uses.
The unlock branch still applies
2775plus recursiveg+w,o-w, so a shields up/down cycle does not converge back to the660/2770 + o-rwxcontract fromscripts/nemoclaw-start.sh. Top-level state dirs become world-traversable, and descendants can retain stale read/execute bits or missg+rentirely depending on their pre-lock mode. That reopenscredentials/identity/workspacemore broadly than intended aftershields down.Suggested fix
- const writeStrip = isLocking ? "go-w" : "g+w,o-w"; - const dirMode = isLocking ? "755" : "2775"; + const recursiveMode = isLocking ? "go-w" : "g+rwX,o-rwx"; + const dirMode = isLocking ? "755" : "2770"; @@ - kubectlExec(sandboxName, ["chmod", "-R", writeStrip, dirPath]); + kubectlExec(sandboxName, ["chmod", "-R", recursiveMode, dirPath]); @@ -write_strip="$3" +recursive_mode="$3" dir_mode="$4" clear_setgid="$5" for dir in "$config_dir"/workspace-*; do [ -d "$dir" ] || continue chown -R "$owner" "$dir" 2>/dev/null || true chmod "$dir_mode" "$dir" 2>/dev/null || true [ "$clear_setgid" = "1" ] && chmod g-s "$dir" 2>/dev/null || true - chmod -R "$write_strip" "$dir" 2>/dev/null || true + chmod -R "$recursive_mode" "$dir" 2>/dev/null || true done @@ - writeStrip, + recursiveMode,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 270 - 318, The unlock branch uses writeStrip = "g+w,o-w" and dirMode = "2775", which doesn't restore the startup permissions; change the unlock values so they match the startup contract (top-level dirs 2770 and recursive owner/group read/write with others removed). Specifically, update the declarations to use writeStrip = isLocking ? "go-w" : "g+rw,o-rwx" and dirMode = isLocking ? "755" : "2770", and ensure the same values are passed to kubectlExec in both the HIGH_RISK_STATE_DIRS loop (kubectlExec calls) and the workspace loop (the heredoc args configDir, owner, writeStrip, dirMode, clearSetgid) so post-unlock permissions converge to 660/2770 + o-rwx as startup expects.
🧹 Nitpick comments (1)
agents/hermes/Dockerfile.base (1)
118-123: Please run the Hermes selective E2E workflows before merge.Given this is under
agents/hermes/**, runninghermes-e2eandrebuild-hermes-e2ewould reduce regression risk for onboarding/health/inference paths.As per coding guidelines:
agents/hermes/**→ E2E recommendation includeshermes-e2eandrebuild-hermes-e2e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/hermes/Dockerfile.base` around lines 118 - 123, Change requires running the Hermes selective end-to-end workflows before merging: execute the hermes-e2e and rebuild-hermes-e2e workflows to validate onboarding, health, and inference paths affected by the Dockerfile.base changes; if failures occur, fix the Dockerfile changes (the bashrc PATH/export block) and re-run these two workflows until they pass, then include passing workflow run IDs in the PR before merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/shields.ts`:
- Around line 270-318: The unlock branch uses writeStrip = "g+w,o-w" and dirMode
= "2775", which doesn't restore the startup permissions; change the unlock
values so they match the startup contract (top-level dirs 2770 and recursive
owner/group read/write with others removed). Specifically, update the
declarations to use writeStrip = isLocking ? "go-w" : "g+rw,o-rwx" and dirMode =
isLocking ? "755" : "2770", and ensure the same values are passed to kubectlExec
in both the HIGH_RISK_STATE_DIRS loop (kubectlExec calls) and the workspace loop
(the heredoc args configDir, owner, writeStrip, dirMode, clearSetgid) so
post-unlock permissions converge to 660/2770 + o-rwx as startup expects.
---
Nitpick comments:
In `@agents/hermes/Dockerfile.base`:
- Around line 118-123: Change requires running the Hermes selective end-to-end
workflows before merging: execute the hermes-e2e and rebuild-hermes-e2e
workflows to validate onboarding, health, and inference paths affected by the
Dockerfile.base changes; if failures occur, fix the Dockerfile changes (the
bashrc PATH/export block) and re-run these two workflows until they pass, then
include passing workflow run IDs in the PR before merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7fa90049-7c12-4dc8-a08b-574c51859bc1
📒 Files selected for processing (10)
agents/hermes/Dockerfile.basescripts/nemoclaw-start.shsrc/lib/agent-onboard.test.tssrc/lib/agent-onboard.tssrc/lib/shields.tstest/e2e/test-shields-config.shtest/nemoclaw-start.test.tstest/repro-2376.test.tstest/repro-2681-group-writable.test.tstest/shields.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 25242756784
|
Selective E2E Results — ✅ All requested jobs passedRun: 25243200348
|
…ields-regressions # Conflicts: # src/lib/agent-onboard.test.ts # src/lib/agent-onboard.ts # src/lib/shields.ts # test/e2e/test-shields-config.sh
3 participants
Summary
Validation
Notes
Summary by CodeRabbit
Bug Fixes
Improvements