fix(cli): target portable Podman during cleanup - #8585
Conversation
|
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:
📝 WalkthroughWalkthroughPortable launch now resolves the receipt-owned rootless Podman container for privileged cleanup. It validates socket authority, container identity, labels, name, and running state. The workflow tests chat, ChangesPortable Podman execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LaunchE2E
participant privilegedSandboxExecArgv
participant resolvePortableDemoPrivilegedExecTarget
participant Podman
LaunchE2E->>privilegedSandboxExecArgv: run portable privileged cleanup
privilegedSandboxExecArgv->>resolvePortableDemoPrivilegedExecTarget: resolve sandbox target
resolvePortableDemoPrivilegedExecTarget->>Podman: inspect receipt-owned container
Podman-->>resolvePortableDemoPrivilegedExecTarget: verified container and socket
resolvePortableDemoPrivilegedExecTarget-->>privilegedSandboxExecArgv: container ID and Docker host
privilegedSandboxExecArgv-->>LaunchE2E: execute root cleanup command
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
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 @.github/workflows/portable-profile-e2e.yaml:
- Around line 152-167: Update the `--user try-restart podman.service` branch to
launch a replacement `podman system service` after stopping the existing process
and removing its socket. Reuse the startup, PID-file, socket-waiting, and
failure-reporting behavior from the `--user enable --now podman.socket` branch
so the socket is available before returning.
🪄 Autofix
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: c3fc6670-9efe-422a-b2da-543061654389
📒 Files selected for processing (11)
.github/workflows/portable-profile-e2e.yamlsrc/lib/onboard/experimental/portable-demo-lifecycle.test.tssrc/lib/onboard/experimental/portable-demo-lifecycle.tssrc/lib/sandbox/privileged-exec.test.tssrc/lib/sandbox/privileged-exec.tstest/e2e/fixtures/availability-env.tstest/e2e/live/full-e2e.test.tstest/e2e/live/launch-agent-turn.tstest/e2e/mock-parity.jsontest/e2e/support/e2e-workflow.test.tstest/e2e/support/launch-agent-turn.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
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 `@src/lib/onboard/experimental/portable-demo-lifecycle.ts`:
- Around line 434-440: Update the provider construction in the sandbox lifecycle
to always pass a sanitized capture created with podman and podmanEnv, rather
than conditionally spreading it only when deps.podman exists. Ensure every
provider.capture invocation, including ps and inspect, uses podmanEnv and the
validated local Podman runtime.
🪄 Autofix
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: ad8e28b6-41ab-4e65-b227-7a0381c434b5
📒 Files selected for processing (5)
src/lib/adapters/podman/index.tssrc/lib/onboard/experimental/portable-demo-lifecycle.test.tssrc/lib/onboard/experimental/portable-demo-lifecycle.tssrc/lib/sandbox/privileged-exec.test.tssrc/lib/sandbox/privileged-exec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/sandbox/privileged-exec.test.ts
- src/lib/sandbox/privileged-exec.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/experimental/portable-demo-lifecycle.test.ts (1)
340-346: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the hardening order, not only the call.
Line [346] verifies that
hardenSocketDirectoryreceivedSOCKET_PATH, but it does not verify that hardening completes before socket-authority capture. If that order regresses, this test still passes while the authority guarantee is untested. Record events in the injected hardener andsocketAuthorityDeps, then assert that hardening occurs first, matchingsrc/lib/onboard/experimental/portable-demo-lifecycle.tsLines [419-462].🤖 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/onboard/experimental/portable-demo-lifecycle.test.ts` around lines 340 - 346, Update the resolveTarget test around hardenSocketDirectory to record ordered events from the injected hardener and socketAuthorityDeps callbacks. Assert that the hardening event occurs before socket-authority capture, while retaining the existing SOCKET_PATH argument assertion and result expectations.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 `@src/lib/onboard/experimental/portable-demo-lifecycle.test.ts`:
- Around line 340-346: Update the resolveTarget test around
hardenSocketDirectory to record ordered events from the injected hardener and
socketAuthorityDeps callbacks. Assert that the hardening event occurs before
socket-authority capture, while retaining the existing SOCKET_PATH argument
assertion and result expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 046893cf-0bb3-40c7-8273-093969ee374e
📒 Files selected for processing (9)
.github/workflows/portable-profile-e2e.yamlsrc/lib/adapters/podman/index.tssrc/lib/adapters/podman/socket-authority.test.tssrc/lib/adapters/podman/socket-authority.tssrc/lib/onboard/experimental/portable-demo-lifecycle.test.tssrc/lib/onboard/experimental/portable-demo-lifecycle.tssrc/lib/onboard/experimental/portable-host-preparation.test.tssrc/lib/onboard/experimental/portable-host-preparation.tstest/e2e/live/portable-profile-rootless-linux.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/adapters/podman/index.ts
- src/lib/onboard/experimental/portable-host-preparation.ts
- test/e2e/live/portable-profile-rootless-linux.test.ts
- src/lib/adapters/podman/socket-authority.ts
- src/lib/adapters/podman/socket-authority.test.ts
- src/lib/onboard/experimental/portable-host-preparation.test.ts
- .github/workflows/portable-profile-e2e.yaml
- src/lib/onboard/experimental/portable-demo-lifecycle.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Exact-head independent live acceptance passed for the production change in this PR.
Independent code review found no correctness, security, scope, or coverage blockers. Current head |
prekshivyas
left a comment
There was a problem hiding this comment.
Requesting changes for three blocking issues:
- A stale portable receipt can take precedence over the registry's current runtime owner and route privileged execution to an old Podman container.
- Portable host preparation honors ambient Podman remote selectors before applying a local chmod to the returned socket path.
- The new portable-launch E2E emits mismatched target IDs between declaration and completion.
I ran the focused suite (122 tests) and typecheck successfully; these are missing lifecycle and evidence contracts rather than existing test failures.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
prekshivyas
left a comment
There was a problem hiding this comment.
changes on 197af2f
[P1] Linux CI deterministically fails. The socket test creates its fixture beneath /tmp, while the production code correctly rejects world-writable path components. This already failed CLI shard 2; the test is unchanged in the latest head. Create the fixture beneath a secure directory such as the real user home.
[P2] Portable E2E path filtering misses src/lib/adapters/podman/**. Future isolated changes to this socket-security boundary would not trigger the portable-profile workflow. Add that path and cover it in portableProofInputs.
The original three requested changes are properly addressed: registry-generation binding, sanitized/prequalified Podman sockets, and consistent E2E target IDs. I found no remaining production security blocker across authorization, injection, secrets, dependencies, or error handling. Local focused tests passed: 145 CLI + 47 E2E-support.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@prekshivyas The retained-install compatibility blocker is addressed at exact head |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@prekshivyas The exact head is now |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@prekshivyas The exact head is now |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
@prekshivyas The interrupted schema-2 migration retry gap is addressed in current PR commit Recovery now re-enters qualified local Podman authority and receipt-owned container validation when a schema-2 receipt already matches the registry generation, then finishes the schema-3 receipt write without repeating the registry compare-and-set. Schema-1 startup-refresh behavior remains unchanged. The new regression pre-commits the registry generation, verifies no second CAS, verifies qualified socket discovery, and verifies the receipt advances to schema 3 with the same generation. Validation: focused lifecycle/migration/privileged-exec suites Independent retained-install acceptance also passed on Please re-review the current commit. |
Summary
Portable OpenClaw launch cleanup now targets the receipt-owned rootless Podman container instead of querying the unrelated default Docker daemon. The Docker path is unchanged, and portable cleanup still fails closed when runtime, lifecycle generation, socket, or container identity cannot be proved.
Related Issue
Fixes #8584
Changes
0700without following symlinks, allowing the standard rootless systemd0660socket only inside that private directory./exit, exit status0, and the OpenClaw2770/660mutable config permission contract.Type of Change
Quality Gates
/exit, exit-code preservation, fail-closed OpenClaw permission restoration, the2770/660contract, and the portable Docker-compatible Podman path. This fix restores and hardens that existing contract without adding a supported surface.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailableb0b33facepasses the focused lifecycle/migration/privileged-exec suites (72/72), Biome,npm run typecheck:cli,npm run checks:repository,npm run test-size:check,npm run test-conditionals:scan,npm run build:cli,git diff --check, and all commit hooks. Its parentdded31205cleanly merged updated upstreammainwithout changing the PR patch and passed the six focused socket/lifecycle/migration/create-flow/privileged-exec/registry suites (148/148). Earliernpm run test:changedreached 4,213 passing tests; six unrelated 5-second host timeouts occurred in Hermes watcher/rebuild recovery tests, with the rebuild test passing on an isolated rerun.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Independent retained-install acceptance passed on
nc-gpu-aug5without a permission workaround. A preserved schema-2 receipt and Docker registry row with no lifecycle generation were migrated to schema 3 with both receipt and registry bound to container4019ad7d...f493; a fresh session returnedPR8585_B4_RETAINED_OK,/exitreturned status0, Podman parent/socket modes were0700/0660, and/sandbox/.openclawplusopenclaw.jsonwere2770/660. The documented Brev169.254.1.2/32loopback prerequisite lost during reboot was restored; nochmodor receipt/registry edits were used. Current commitb0b33faceadditionally makes an interrupted registry-claim/receipt-write migration retry revalidate the same authority and identity before finishing the receipt upgrade without another registry compare-and-set.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests