feat(policy): add sandbox policy get command for clean YAML output#6122
Open
kagura-agent wants to merge 2 commits into
Open
feat(policy): add sandbox policy get command for clean YAML output#6122kagura-agent wants to merge 2 commits into
sandbox policy get command for clean YAML output#6122kagura-agent wants to merge 2 commits into
Conversation
…VIDIA#6052) The openshell policy get --full output includes a metadata header that cannot be piped directly to policy set. This adds a CLI command that strips the header via parseCurrentPolicy and outputs only usable YAML. A --raw flag preserves the original openshell output when needed. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
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 (4)
📝 WalkthroughWalkthroughAdds a new ChangesSandbox Policy Get Command
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SandboxPolicyGetCommand
participant getSandboxPolicy
participant runCapture
participant parseCurrentPolicy
User->>SandboxPolicyGetCommand: sandbox:policy:get <name> [--raw]
SandboxPolicyGetCommand->>getSandboxPolicy: retrieve policy data
getSandboxPolicy->>runCapture: openshell policy get --full <name>
runCapture-->>getSandboxPolicy: raw output
getSandboxPolicy->>parseCurrentPolicy: parse when raw output exists
parseCurrentPolicy-->>getSandboxPolicy: yaml
getSandboxPolicy-->>SandboxPolicyGetCommand: { raw, yaml }
alt --raw set
SandboxPolicyGetCommand->>User: log raw output
else
SandboxPolicyGetCommand->>User: log yaml
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
src/commands/sandbox/policy/get.test.ts (1)
29-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests assert mock calls instead of observable output.
Both the default-parsing test and the
--rawtest verify only that internal helpers were invoked with certain args, never that the command actually logs the expected YAML/raw content. This locks tests to implementation details rather than the command's public behavior (what a user sees on stdout), and would mask a behavioral regression while still passing.As per path instructions, tests should "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
✅ Suggested addition: assert on logged output
+ it("invokes openshell and parses policy by default", async () => { const rawOutput = "Version: 1\nHash: abc\nStatus: active\n---\nversion: 1\nnetwork_policies: []"; mocks.runCapture.mockReturnValue(rawOutput); mocks.parseCurrentPolicy.mockReturnValue("version: 1\nnetwork_policies: []"); + const logSpy = vi.spyOn(SandboxPolicyGetCommand.prototype, "log"); await SandboxPolicyGetCommand.run(["alpha"], rootDir); expect(mocks.assertOpenshellResolvable).toHaveBeenCalled(); expect(mocks.buildPolicyGetCommand).toHaveBeenCalledWith("alpha"); expect(mocks.runCapture).toHaveBeenCalledWith([...]); expect(mocks.parseCurrentPolicy).toHaveBeenCalledWith(rawOutput); + expect(logSpy).toHaveBeenCalledWith("version: 1\nnetwork_policies: []"); });🤖 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/commands/sandbox/policy/get.test.ts` around lines 29 - 58, The SandboxPolicyGetCommand tests are only checking internal helper calls instead of the command’s observable output. Update the tests around SandboxPolicyGetCommand.run to assert the stdout/logged result for both the default parsed YAML path and the --raw path, using the mocked runCapture output as the public boundary. Keep the helper setup assertions only if needed for setup, but make the primary expectation the actual emitted content so the tests validate user-visible behavior rather than implementation details.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/commands/sandbox/policy/get.ts`:
- Around line 36-58: Move the orchestration out of SandboxPolicyGetCommand.run()
and into a sandbox action under src/lib/actions/sandbox, since run() is
currently doing precondition checks, executing OpenShell, and handling parse
branches. Extract the policy retrieval and parsing flow
(assertOpenshellResolvable, buildPolicyGetCommand, runCapture,
parseCurrentPolicy) into a typed action such as getSandboxPolicy, then keep the
command as thin argv/flag parsing plus translating the action result into
this.log/this.error. Preserve the raw/raw-flag and YAML parsing behavior in the
action’s returned result so the command no longer owns product logic.
---
Nitpick comments:
In `@src/commands/sandbox/policy/get.test.ts`:
- Around line 29-58: The SandboxPolicyGetCommand tests are only checking
internal helper calls instead of the command’s observable output. Update the
tests around SandboxPolicyGetCommand.run to assert the stdout/logged result for
both the default parsed YAML path and the --raw path, using the mocked
runCapture output as the public boundary. Keep the helper setup assertions only
if needed for setup, but make the primary expectation the actual emitted content
so the tests validate user-visible behavior rather than implementation details.
🪄 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: ece117d6-bea0-4744-adfe-63a0598ad246
📒 Files selected for processing (2)
src/commands/sandbox/policy/get.test.tssrc/commands/sandbox/policy/get.ts
Address CodeRabbit review: - Extract orchestration into src/lib/actions/sandbox/policy-get.ts per single-path oclif architecture (command = argv glue, action = logic) - Tests now assert on logged output (observable behavior) - Add dedicated unit tests for the action layer
wscurran
added
area: cli
Contributor
|
✨ Thanks for the fix. Adding a new Related open issues: |
Contributor
Author
|
Thanks @wscurran! Happy to hear the approach works well. Let me know if you'd like any changes — happy to iterate. |
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
Adds a new
nemoclaw sandbox policy getCLI command that retrieves the active sandbox policy and outputs clean YAML suitable for piping topolicy set.Closes #6052
Problem
openshell policy get --fullincludes metadata header lines (Version, Hash, Status, Active, Created, Loaded) before the actual YAML. When this output is piped to a file and fed back toopenshell policy set --policy, it fails with a YAML parse error becausepolicy setcannot recognize the metadata fields. The get/set round-trip is broken.Solution
Added
nemoclaw sandbox policy get <sandbox>which wrapsopenshell policy get --fulland uses the existingparseCurrentPolicy()function (fromsrc/lib/policy/index.ts) to strip the metadata header, outputting only usable YAML.A
--rawflag is available to get the unmodified openshell output with metadata when needed.Usage
Changes
src/commands/sandbox/policy/get.ts— new oclif command (sandbox:policy:get)src/commands/sandbox/policy/get.test.ts— 4 unit tests (default parse, --raw bypass, error paths)Testing
npx vitest run src/commands/sandbox/policy/get.test.ts)Summary by CodeRabbit
sandbox:policy:getcommand to display the active policy for a specified sandbox.--rawmode to show the unprocessed policy output.--rawoutput, and error scenarios for both retrieval and parsing.