test(e2e): prove state-dir metadata in final images (#6059)#6717
Merged
cv merged 6 commits intoJul 13, 2026
Conversation
…VIDIA#6059) state-dir-guard.py's xattr/ACL preservation across shields up/down was only exercised by source-level fakes (PR NVIDIA#5874, advisor findings PRA-19/PRA-T5), never against a real sandbox filesystem's actual kernel ACL/mode-resync behavior. Add a shared seed/verify helper plus live E2E coverage for both OpenClaw and Hermes sandboxes, proving in-place fchown/fchmod on unlock, fresh-inode replace on lock, ACL MASK resync on chmod, and execute-bit preservation hold against real kernels. Closes NVIDIA#6059 Signed-off-by: DisturbedSage <maruteymani31@gmail.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:
📝 WalkthroughWalkthroughAdds live E2E coverage for state-directory guard metadata behavior in OpenClaw and Hermes images, including xattrs, POSIX ACLs, inode transitions, and filesystem budgets. Updates CI image handling and adds workflow-boundary and mock-parity validation. ChangesLive state-directory metadata validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Docker as Docker images
participant Test as Live metadata test
participant Guard as state-dir-guard.py
participant FS as Linux filesystem
CI->>Docker: Load OpenClaw and Hermes image artifacts
CI->>Test: Run state-dir-guard-metadata suite
Test->>FS: Seed files, xattr, ACL, and content
Test->>Docker: Probe exact bind mount
Docker->>Guard: Execute preflight, lock, and unlock
Guard->>FS: Replace files and apply metadata
Test->>FS: Read metadata and budget measurements
Test-->>CI: Write assertions and artifact evidence
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds live (real-sandbox) E2E coverage to prove scripts/state-dir-guard.py preserves xattrs and POSIX ACL semantics across shields up/down transitions for both OpenClaw and Hermes, closing the previously deferred real-kernel proof gap from #5874/#6059.
Changes:
- Added a shared Python helper that seeds files with a
user.*xattr marker plus asystem.posix_acl_accesspayload and can verify post-transition metadata/effective ACL permissions. - Added two live E2E tests (OpenClaw + Hermes) that install NemoClaw into a real Docker/OpenShell sandbox, run shields up/down, and assert inode replacement vs in-place unlock behavior plus effective-permission clamping.
- Registered both tests in
test/e2e/mock-parity.jsonwith explicitliveOnlyReasonentries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/e2e/mock-parity.json | Declares the new live-only E2E tests and documents why they cannot have mock equivalents. |
| test/e2e/live/state-dir-guard-xattr-acl.test.ts | Adds OpenClaw live-sandbox E2E coverage that seeds/validates xattr + POSIX ACL behavior across shields transitions. |
| test/e2e/live/hermes-state-dir-guard-xattr-acl.test.ts | Adds Hermes live-sandbox E2E coverage for the same xattr/ACL contract via nemohermes shields up/down. |
| test/e2e/lib/state-dir-guard-xattr-acl.py | Introduces a test-side helper to seed and verify kernel ACL/xattr wire-format behavior without requiring userspace acl packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+130
to
+136
| await sandbox | ||
| .openshell(["gateway", "destroy", "-g", "nemoclaw"], { | ||
| artifactName: "cleanup-openshell-gateway-destroy", | ||
| env: commandEnv(), | ||
| timeoutMs: 60_000, | ||
| }) | ||
| .catch(() => undefined); |
Comment on lines
+187
to
+198
| def cmd_verify(args: argparse.Namespace) -> int: | ||
| st = os.stat(args.path, follow_symlinks=False) | ||
| report = { | ||
| "path": args.path, | ||
| "inode": st.st_ino, | ||
| "uid": st.st_uid, | ||
| "gid": st.st_gid, | ||
| "mode": oct(st.st_mode & 0o7777), | ||
| "sha256": _sha256_file(args.path), | ||
| } | ||
| fd = os.open(args.path, os.O_RDONLY) | ||
| try: |
Comment on lines
+39
to
+42
| const TEST_TIMEOUT_MS = 30 * 60_000; | ||
| const INSTALL_TIMEOUT_MS = 25 * 60_000; | ||
| const COMMAND_TIMEOUT_MS = 60_000; | ||
|
|
Comment on lines
+39
to
+42
| const TEST_TIMEOUT_MS = 30 * 60_000; | ||
| const INSTALL_TIMEOUT_MS = 25 * 60_000; | ||
| const COMMAND_TIMEOUT_MS = 60_000; | ||
|
|
![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: 7
🤖 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 `@test/e2e/live/hermes-state-dir-guard-xattr-acl.test.ts`:
- Line 414: Update the inode evidence configuration and assertions around the
transition test to distinguish fresh-inode behavior during lock from
stable-inode behavior during unlock. Remove the blanket freshInodePerTransition
expectation and add separate assertions matching the intentionally preserved
locked inode described by the surrounding test cases.
- Around line 328-347: Extend the ownership assertions in the workspace and
credentials transition checks to validate both uid and gid for each root in both
locked and unlocked states. Capture the expected policy ownership once and
compare every reported uid/gid against it, including adding the missing
workspaceLocked.gid assertion and corresponding assertions in the unlock
verification blocks.
- Around line 230-289: Replace the inline prerequisite and capability
conditionals in the test setup around dockerInfo and capability with the
established prerequisite/capability fixture abstraction. Preserve the existing
behavior: CI must fail when Docker or xattr/ACL support is unavailable, while
local runs must skip with the current diagnostic messages; keep artifact
recording and subsequent setup unchanged.
In `@test/e2e/live/state-dir-guard-xattr-acl.test.ts`:
- Around line 242-249: Remove the prerequisite-handling if conditionals from the
test and route Docker availability through the established helper or fixture
mechanism used by the surrounding live E2E tests. Preserve the existing
behavior: provide Docker failure details in GitHub Actions and skip when
unavailable elsewhere, while ensuring the test file contains no conditional
statements in these prerequisite paths.
- Around line 405-423: The credentials unlock assertions in the phase-5
verification should also validate the raw named-user ACL entry. Reuse the
existing ACL-entry verification data for credentials and assert that the entry
remains 0o7, alongside the effective permission check, matching the workspace
assertion.
- Around line 336-344: Update the transition assertions in the live
state-directory guard test: add a gid expectation alongside workspaceLocked.uid
for the lock phase, and assert both uid and gid on every unlock report. Apply
these checks to each existing unlock transition, reusing the expected ownership
values already established by the fixture.
- Around line 33-35: Define a shared gateway-name constant near SANDBOX_NAME,
using the OPENSHELL_GATEWAY environment override with the existing default, and
update both commandEnv() and cleanupSandbox() to use it. Ensure setup and
cleanup consistently target the same gateway instead of hardcoding nemoclaw.
🪄 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: 503eda3d-25e0-4056-8093-a19c970435a2
📒 Files selected for processing (4)
test/e2e/lib/state-dir-guard-xattr-acl.pytest/e2e/live/hermes-state-dir-guard-xattr-acl.test.tstest/e2e/live/state-dir-guard-xattr-acl.test.tstest/e2e/mock-parity.json
codebase-growth-guardrails flagged the Docker-required and xattr/ACL capability-required gate checks added in 9eb183c as new if-statements in changed .test.ts files. Extract both gates into non-test helper functions in state-dir-guard-xattr-acl-helpers.ts, following the repo's established idiom (see test/e2e/fixtures/docker-probe.ts's DockerPrerequisite and the *-helpers.ts files throughout test/e2e/live/) of keeping conditional prerequisite logic out of scanned test bodies. The helpers take already-executed probe results rather than performing their own Docker invocation, so the tests keep using the existing HostCliClient command path unchanged. Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
…/ACL live E2E Address automated review findings (Copilot/CodeRabbit) on the NVIDIA#6059 xattr/ACL live E2E coverage: - state-dir-guard-xattr-acl.py's cmd_verify stat'd args.path with follow_symlinks=False, then reopened it by path for xattr/hash reads. A symlink swapped in between those two path-based calls would be silently followed. Replace both with a single O_NOFOLLOW open and derive fstat, xattr, and the sha256 hash from that one fd, so a swapped-in symlink now fails loudly instead of being followed. - Both live tests only asserted numeric uid/gid, never the actual root/sandbox identity the guard's policy is supposed to produce. Resolve uid/gid to names via pwd/grp and assert them at all four lock/unlock checkpoints, matching _expected_ids in state-dir-guard.py. - Add the missing raw ACL entry parity check for the credentials-unlock case, mirroring the workspace-unlock assertion already present. - Split the self-contradictory freshInodePerTransition assertion (lock produces a fresh inode; unlock preserves it) into lockReplacesWithFreshInode and unlockPreservesLockedInode. - Fix the OpenClaw variant hardcoding "nemoclaw" as the gateway name in cleanupSandbox instead of using the configurable GATEWAY_NAME already used elsewhere in the file, and bump COMMAND_TIMEOUT_MS to 120s in both variants to match the Hermes/OpenClaw CLI's real command latency. Verified locally: typecheck and Biome are clean, and the OpenClaw variant's live E2E run passes end-to-end with all new assertions. Signed-off-by: DisturbedSage <maruteymani31@gmail.com>
wscurran
added
area: e2e
Contributor
|
✨ Thanks for the E2E coverage work, @DisturbedSage5840C. Adding real-sandbox xattr/ACL validation closes the proof gap from PR #5874. Ready for maintainer review. Related open PRs: Related open issues: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
changed the title
prekshivyas
force-pushed
the
feat/state-dir-guard-xattr-acl-6059
branch
from
July 13, 2026 03:07
78f7be2 to
5e541b0
Compare
Contributor
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 `@test/e2e/live/state-dir-guard-metadata.test.ts`:
- Around line 437-443: Extend the ACL metadata assertions around the seeded
entries and corresponding cases to exercise real access as UID 65534, not just
parsed getfacl output. Add read/write probes through the public path boundary,
asserting locked plugins are readable but not writable, locked credentials are
neither readable nor writable, and unlocked targets support both operations;
preserve the existing metadata checks.
🪄 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: 4bc015df-e2e1-4626-ac0d-6a25b9608932
📒 Files selected for processing (5)
.github/workflows/sandbox-images-and-e2e.yamltest/e2e/live/state-dir-guard-metadata-helpers.tstest/e2e/live/state-dir-guard-metadata.test.tstest/e2e/mock-parity.jsontest/e2e/support/sandbox-images-workflow-boundary.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
force-pushed
the
feat/state-dir-guard-xattr-acl-6059
branch
from
July 13, 2026 03:19
5e541b0 to
ccf5867
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/e2e/sandbox-images-workflow-boundary.mts (1)
598-699: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHigh complexity in
validateStateDirGuardMetadataImageReuse.This ~100-line function packs job-dependency, timeout, env-contract, auth/rebuild-absence, producer-probe-absence, download-metadata, load/tools/probe checks, and 5-way step-ordering into one function body — well beyond Biome's default cognitive-complexity threshold of 15 for
noExcessiveCognitiveComplexity. Consider splitting into focused helpers (e.g., contract/env checks, artifact download checks, load/tools/probe checks, ordering checks) mirroring the natural blocks already present.As per path instructions,
**/*.{ts,tsx}guidance is to "Keep TypeScript function complexity low and use the repository Biome formatting and linting configuration." The line-range change details also flag this block ascode_block_complexity_high.♻️ Example decomposition sketch
function validateStateDirGuardMetadataJobContract(errors: string[], jobName: string, job: SandboxImagesWorkflowJob): void { // needs, timeout, env, Set up Node/Install deps, auth absence, rebuild absence, producer-probe absence } function validateStateDirGuardMetadataArtifacts(errors: string[], jobName: string, job: SandboxImagesWorkflowJob): { openclawDownload, hermesDownload, load, tools, probe, upload } { // download metadata, load fragments, tools fragments, probe timeout/target, upload uploader } function validateStateDirGuardMetadataOrdering(errors: string[], job: SandboxImagesWorkflowJob, steps: {...}): void { // 5-way ordering check } function validateStateDirGuardMetadataImageReuse(errors: string[], workflow: SandboxImagesWorkflow): void { const jobName = "state-dir-guard-metadata"; const job = workflow.jobs[jobName] ?? {}; validateStateDirGuardMetadataJobContract(errors, jobName, job); const steps = validateStateDirGuardMetadataArtifacts(errors, jobName, job); validateStateDirGuardMetadataOrdering(errors, job, steps); }🤖 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 `@tools/e2e/sandbox-images-workflow-boundary.mts` around lines 598 - 699, Reduce the cognitive complexity of validateStateDirGuardMetadataImageReuse by extracting focused helpers for the job contract checks, artifact/load/tools/probe validation, and step ordering validation. Keep all existing checks and error messages unchanged, and have the main function coordinate these helpers using the shared jobName, job, and validated step references.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.
Nitpick comments:
In `@tools/e2e/sandbox-images-workflow-boundary.mts`:
- Around line 598-699: Reduce the cognitive complexity of
validateStateDirGuardMetadataImageReuse by extracting focused helpers for the
job contract checks, artifact/load/tools/probe validation, and step ordering
validation. Keep all existing checks and error messages unchanged, and have the
main function coordinate these helpers using the shared jobName, job, and
validated step references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0cad59cf-c7fb-4082-93e6-53793ab246ea
📒 Files selected for processing (6)
.github/workflows/sandbox-images-and-e2e.yamltest/e2e/live/state-dir-guard-metadata-helpers.tstest/e2e/live/state-dir-guard-metadata.test.tstest/e2e/mock-parity.jsontest/e2e/support/sandbox-images-workflow-boundary.test.tstools/e2e/sandbox-images-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/live/state-dir-guard-metadata-helpers.ts
- test/e2e/mock-parity.json
- .github/workflows/sandbox-images-and-e2e.yaml
- test/e2e/support/sandbox-images-workflow-boundary.test.ts
Contributor
PR Review AdvisorTrusted manual dispatch against final head
|
prekshivyas
approved these changes
Jul 13, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Contributor
There was a problem hiding this comment.
Reviewed final head and trusted validation. Both PR advisors and the exact final-image metadata E2E passed; remaining Copilot threads are outdated against removed files and non-blocking. LGTM.
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
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 #6059 with one focused, parameterized final-image probe for both built-in agents. The probe runs the installed
/usr/local/lib/nemoclaw/state-dir-guard.pyas root through Docker against exact bind mounts at/sandbox/.openclawand/sandbox/.hermes.Changes
preflight,lock, andunlockfor the high-riskpluginsroot and confidentialitycredentialsroot.setfacl/getfaclandsetfattr/getfattr; GitHub-hosted Ubuntu fails clearly if the exact bind mount loses xattr or ACL support, while unsupported local filesystems skip explicitly.sandbox-images-and-e2e.yaml. The Hermes producer now publishes its image, and a failure-isolated consumer downloads both artifacts without rebuilding.Production guard behavior is unchanged. No
tcb-boundary.mdxupdate is needed because the probe did not establish a new compatibility limitation.Quality Gates
Verification
npx vitest run --project e2e-support test/e2e/support/sandbox-images-workflow-boundary.test.ts(9/9)Signed-off-by: DisturbedSage maruteymani31@gmail.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit