Conversation
Replace all stub CLI commands with real wiring through a Dependencies struct that holds only interface fields (Dependency Inversion). Add Run and Reset to SandboxManager interface, Provisioner and ShellProvider interfaces (ISP). Fix LimaController to use sandboxName parameter instead of hardcoded vmName, with per-sandbox policy tracking. Wire production limactl runners to exec.CommandContext. Add JSONStore.Apply, LimaProvider.Shell, compile-time interface checks, and fix TestCheckResourcesForSpec.
- Add api.ProfileRegistry interface, replace *profile.Registry in Dependencies and Manager with the interface (DIP) - Add RemovePolicy to api.NetworkController interface, call from Destroy workflow to prevent memory leak in policies map - Add TestRemovePolicy covering multi-sandbox cleanup isolation - Improve TestCheckResourcesForSpec: use excessive spec, assert len(issues) > 0
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
WalkthroughThis pull request implements real command functionality in the CLI through dependency injection, refactors the network controller for per-sandbox policy management, and introduces new interfaces for provisioning and shell access. Thirteen files across API, CLI, network, sandbox, and VM layers are modified to support these operational behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/network/network.go (1)
83-96:⚠️ Potential issue | 🟡 MinorShell-quoting is fragile and a latent injection surface.
printf '%%s' '%s' | sudo iptables-restoreembedsrulesetinside a single-quoted shell string. TodaybuildOutputRulesonly interpolates booleans into hardcoded rules, so this is safe — but the momentapi.NetworkPolicygains any string-valued field (allowlisted domains, per-port rules, custom chains), a single'in the input breaks out of the quoting and runs arbitrary commands as root inside the VM (note thesudo).Two safer alternatives:
- Write the ruleset to a temp file via
provider.CopyToVMand runsudo iptables-restore < /tmp/rules.- Pipe via stdin: pass
rulesetasc.Stdinto asudo iptables-restorecommand — no shell interpolation at all.Worth tightening now, or at least a
// SECURITY:comment documenting the invariant thatbuildOutputRulesmust never emit user-controlled strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network.go` around lines 83 - 96, ApplyPolicy currently shells the ruleset into a single-quoted printf which is a command-injection risk if NetworkPolicy ever contains strings; change ApplyPolicy (and its use of buildOutputRules/runCmd) to avoid shell interpolation by either: (A) writing the ruleset to a temp file in the VM via provider.CopyToVM and invoking "sudo iptables-restore < /tmp/RENAME" with runCmd, or (B) piping the ruleset to a non-shell command by invoking "sudo iptables-restore" and supplying ruleset as the command's stdin (modify runCmd to accept stdin if needed); update lc.policies and locking behavior as before and add a SECURITY comment near buildOutputRules/ApplyPolicy documenting the invariant if you choose not to change behavior now.
🧹 Nitpick comments (8)
internal/api/vm.go (1)
23-33: Minor signature consistency nits on the new interfaces.
HasCleanSnapshot(name string) boolat Line 26 drops bothctxanderror. For a filesystemos.Statcall that's defensible, but a mock/remote implementation can't signal transient errors. Consider(bool, error)and optionallyctx.Shell(name string) errorat Line 32 — see the detailed comment oninternal/vm/lima/provider.go; addingctx context.Contextaligns withProviderand resolves thenoctxlint there.Not blocking, but easier to change now than once more implementations exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/vm.go` around lines 23 - 33, Update the Provisioner and ShellProvider interfaces to be context- and error-consistent: change HasCleanSnapshot to accept a context and return (bool, error) (i.e., HasCleanSnapshot(ctx context.Context, name string) (bool, error)) so implementations can report transient failures, and change Shell to include context (Shell(ctx context.Context, name string) error); then update all implementations, tests and mocks of Provisioner.HasCleanSnapshot and ShellProvider.Shell (e.g., provider implementations in internal/vm/lima/provider.go and any mock types) to match the new signatures and propagate/return errors accordingly.internal/api/network.go (1)
18-18: Consider aligningRemovePolicysignature with the rest ofNetworkController.All other methods take
ctx context.Contextand returnerror.RemovePolicy(sandboxName string)breaks that pattern. If a future implementation needs to tear down iptables rules on the VM (not just drop an in-memory entry), the missingctx/errorreturn will force a breaking interface change. Consider:- RemovePolicy(sandboxName string) + RemovePolicy(ctx context.Context, sandboxName string) errorAnd handle the error in
Destroy(at least log it) so cleanup failures aren't silently swallowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/network.go` at line 18, Update the NetworkController.RemovePolicy signature to follow the existing pattern by adding a context parameter and returning an error (i.e., change RemovePolicy(sandboxName string) to RemovePolicy(ctx context.Context, sandboxName string) error), then update all implementations of NetworkController and any callers (notably Destroy) to pass a ctx and propagate or handle the returned error (at minimum log it) so cleanup failures aren’t silently ignored; search for the RemovePolicy, NetworkController, and Destroy symbols to adjust implementations and call sites accordingly.internal/api/profile.go (1)
1-11: No import cycle exists; consider whetherprofile.Profilebelongs ininternal/apilayer.
internal/api/profile.gosafely importsinternal/profilewithout creating a cycle. However, the current structure inverts typical Dependency Inversion Principle layering:internal/api(the abstraction/contract layer) depends oninternal/profile(implementation details), when ideally implementations should depend on abstractions, not vice versa. Consider whetherprofile.Profileshould be defined ininternal/apias a pure data type to maintain correct dependency direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/profile.go` around lines 1 - 11, The ProfileRegistry currently depends on the concrete type profile.Profile from internal/profile which inverts the dependency direction; to fix this, introduce a pure abstraction in the api package (e.g. define an api.Profile interface or data struct) and update ProfileRegistry to reference that api.Profile instead of profile.Profile, then change the internal/profile package to implement/translate to that api.Profile (implementing the interface or returning the api struct) so implementations depend on the api abstraction (symbols to change: ProfileRegistry, profile.Profile, and the internal/profile implementation).cmd/airlock/cli/cli.go (3)
666-672: Silently skipping profiles onGeterror hides registry bugs.If
deps.Profiles.Get(name)fails for a name returned byList(), the row is dropped without any indication. SinceListis the source of names, aGetfailure signals a bug in the registry — suppressing it turns a loud error into a subtly shorter table. At minimum, log tocmd.ErrOrStderr(); ideally, return the error.♻️ Suggested change
for _, name := range names { p, err := deps.Profiles.Get(name) if err != nil { - continue + fmt.Fprintf(cmd.ErrOrStderr(), "warning: profile %q: %v\n", name, err) + continue } fmt.Fprintf(w, "%s\t%s\n", p.Name, p.Description) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/cli.go` around lines 666 - 672, The loop over names silently skips errors from deps.Profiles.Get(name) which hides registry bugs; update the loop in cli.go to surface failures instead of dropping rows by either returning the error from the function or at minimum logging it to cmd.ErrOrStderr() with context (include the profile name and error). Change the block around the call to deps.Profiles.Get to check err and call cmd.ErrOrStderr().Write or processLogger (or return the error) so any Get(name) failure is reported rather than ignored.
698-703: Strip.gitsuffix when deriving sandbox name from a GitHub URL.For
https://github.com/user/repo.git,parts[1]is"repo.git"and the name is not passed throughsanitizeName, so the sandbox ends up namedrepo.git(with a.that is allowed bysanitizeName). Users typing the URL straight from GitHub's clone box get a surprising name. Consider trimming.gitand routing throughsanitizeNamefor consistency with the local-path branch.♻️ Suggested change
if strings.HasPrefix(source, "https://github.com/") { parts := strings.SplitN(strings.TrimPrefix(source, "https://github.com/"), "/", 3) if len(parts) >= 2 { - return parts[1] + return sanitizeName(strings.TrimSuffix(parts[1], ".git")) } }Apply the same
TrimSuffixto thegh:branch for symmetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/cli.go` around lines 698 - 703, The GitHub-URL branch currently returns parts[1] which can end with ".git" and bypasses sanitizeName; update the logic that handles strings.HasPrefix(source, "https://github.com/") to TrimSuffix(parts[1], ".git") and then pass the resulting repo name through sanitizeName before returning it; also mirror this TrimSuffix + sanitizeName behavior in the existing "gh:" branch so both GitHub URL and gh: inputs produce consistent, sanitized sandbox names.
111-117: Prefergolang.org/x/term.IsTerminalfor TTY detection.The
ModeCharDeviceheuristic works for the common case but is not idiomatic Go.golang.org/x/term.IsTerminalis the recommended approach by the Go team, handles edge cases like pseudo-terminals across platforms (including Windows), and is actively maintained. Not blocking.import "golang.org/x/term" func isTerminal(f *os.File) bool { return term.IsTerminal(int(f.Fd())) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/cli.go` around lines 111 - 117, Replace the custom ModeCharDevice heuristic in the isTerminal function with the upstream terminal detection: import "golang.org/x/term" and change isTerminal(f *os.File) to return term.IsTerminal(int(f.Fd())); update imports accordingly and remove the old os.File.Stat() based logic so isTerminal uses term.IsTerminal across platforms.internal/sandbox/sandbox_test.go (1)
161-162: Consider trackingRemovePolicycalls to strengthen Destroy-path coverage.The no-op satisfies the interface, but the PR introduces
m.network.RemovePolicy(name)inManager.Destroyspecifically to prevent policy-map leaks. Without recording removals,TestDeleteSandbox(and similar) cannot actually verify that Destroy cleans up network state. A one-lineremoved []stringfield plus an append here would let you addassert len(network.removed) == 1in the delete tests.♻️ Suggested change
type fakeNetworkController struct { policies []api.NetworkPolicy locked []string unlocked []string + removed []string } @@ -func (f *fakeNetworkController) RemovePolicy(sandboxName string) { +func (f *fakeNetworkController) RemovePolicy(sandboxName string) { + f.removed = append(f.removed, sandboxName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sandbox/sandbox_test.go` around lines 161 - 162, Add tracking for RemovePolicy calls on the fake network controller by adding a removed []string field to fakeNetworkController and appending the sandboxName inside RemovePolicy; update tests (e.g., TestDeleteSandbox) to assert on network.removed contents/length. Modify the fakeNetworkController struct to include removed []string and change the RemovePolicy(sandboxName string) method to append sandboxName to removed so Destroy's call to m.network.RemovePolicy(name) can be verified.internal/api/sandbox.go (1)
40-42: Consider streaming output forRunin a future iteration.
Runreturns the entire command output as astring, which buffers everything in memory. For long-running commands or large outputs (logs, builds), this will either delay output until completion or blow up memory. Anio.Writer-based variant (or addingStdout/Stderrto a request struct) would let the CLI stream output directly to the terminal. Not blocking for this PR since it matches the CLI's current synchronousfmt.Fprint(cmd.OutOrStdout(), output)pattern, but worth tracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/sandbox.go` around lines 40 - 42, The Run method on the sandbox interface buffers entire command output into a string which can OOM or delay output; add a non-breaking streaming variant to support large/long-running commands by extending the interface with a RunStream(ctx context.Context, name string, command []string, stdout io.Writer, stderr io.Writer) error (keep the existing Run for compatibility), update implementors to provide the new method (or stub it to call Run and copy to writers for now), and add a TODO comment on the Run signature to migrate callers to the streaming API later; reference the Run method and the new RunStream method when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/airlock/cli/cli.go`:
- Line 434: The call to MarkFlagRequired("sandbox") returns an error that is
currently ignored; update the MarkFlagRequired invocation in cli.go to either
check and handle the returned error (e.g., err :=
cmd.MarkFlagRequired("sandbox") and handle/log/return if err != nil) or
explicitly discard it with a clear comment to satisfy linters (e.g., _ =
cmd.MarkFlagRequired("sandbox") // explicitly ignore error for required flag
that was just defined). Use the same function name MarkFlagRequired and the
"sandbox" flag reference when making the change.
- Around line 291-293: The message about ephemeral sandboxes is misleading
because the flag stored in SandboxInfo is never acted upon; update the logic so
ephemeral sandboxes are actually cleaned up: in newRunCmd and newShellCmd (and
any other long-running command that creates a sandbox via newSandboxCmd), check
sandboxInfo.Ephemeral and register a cleanup that calls
Manager.Destroy(sandboxID) on exit—use a defer for non-interactive flows and a
signal handler (SIGINT/SIGTERM) for interactive shells to ensure Manager.Destroy
is invoked before process exit; alternatively, if you intentionally want no
automatic cleanup, remove the “will be destroyed on exit” message in
newSandboxCmd so the flag is treated as informational only.
In `@internal/network/network_test.go`:
- Around line 263-264: In TestRemovePolicy, the calls to lc.Lock(...) and
lc.Unlock(...) are missing error checks; update the test to capture and assert
those returns (e.g., err := lc.Lock(ctx, "sandbox-a"); require.NoError(t, err)
and similarly for lc.Unlock) so failures fail the test and subsequent assertions
(like CurrentPolicy calls) aren't misleading; reference the lc.Lock and
lc.Unlock calls inside TestRemovePolicy and use the existing test
helper/assertion style used elsewhere in network_test.go (e.g., require.NoError
or t.Fatalf) to handle errors.
In `@internal/network/network.go`:
- Around line 165-189: Both limactlRunExec and limactlOutputExec are discarding
stderr which hides useful error messages; change them to capture stderr (e.g.,
use a bytes.Buffer for c.Stderr or use c.CombinedOutput()) and include that
captured stderr text in the returned error (wrap with fmt.Errorf and include
vmName and stderr), updating limactlRunExec to run the command, check for
non-nil err and return an error containing the stderr buffer, and updating
limactlOutputExec to return combined output or include stderr in the fmt.Errorf
when c.Output()/c.Run() fails so callers of limactlRunExec and limactlOutputExec
see the real limactl/command error details.
In `@internal/vm/lima/provider.go`:
- Around line 232-243: The Shell method must accept a context to satisfy noctx
and propagate cancellation; change the Shell signature from Shell(name string)
error to Shell(ctx context.Context, name string) error on LimaProvider and the
ShellProvider interface, validate the name with validateName(ctx?) as before
(keep calling validateName(name)), and replace exec.Command(...) with
exec.CommandContext(ctx, p.limactlPath, "shell", name) while still wiring
cmd.Stdin/os.Stdout/os.Stderr and returning cmd.Run(); update all call sites and
interface declarations accordingly to keep the API consistent with other
Provider methods.
---
Outside diff comments:
In `@internal/network/network.go`:
- Around line 83-96: ApplyPolicy currently shells the ruleset into a
single-quoted printf which is a command-injection risk if NetworkPolicy ever
contains strings; change ApplyPolicy (and its use of buildOutputRules/runCmd) to
avoid shell interpolation by either: (A) writing the ruleset to a temp file in
the VM via provider.CopyToVM and invoking "sudo iptables-restore < /tmp/RENAME"
with runCmd, or (B) piping the ruleset to a non-shell command by invoking "sudo
iptables-restore" and supplying ruleset as the command's stdin (modify runCmd to
accept stdin if needed); update lc.policies and locking behavior as before and
add a SECURITY comment near buildOutputRules/ApplyPolicy documenting the
invariant if you choose not to change behavior now.
---
Nitpick comments:
In `@cmd/airlock/cli/cli.go`:
- Around line 666-672: The loop over names silently skips errors from
deps.Profiles.Get(name) which hides registry bugs; update the loop in cli.go to
surface failures instead of dropping rows by either returning the error from the
function or at minimum logging it to cmd.ErrOrStderr() with context (include the
profile name and error). Change the block around the call to deps.Profiles.Get
to check err and call cmd.ErrOrStderr().Write or processLogger (or return the
error) so any Get(name) failure is reported rather than ignored.
- Around line 698-703: The GitHub-URL branch currently returns parts[1] which
can end with ".git" and bypasses sanitizeName; update the logic that handles
strings.HasPrefix(source, "https://github.com/") to TrimSuffix(parts[1], ".git")
and then pass the resulting repo name through sanitizeName before returning it;
also mirror this TrimSuffix + sanitizeName behavior in the existing "gh:" branch
so both GitHub URL and gh: inputs produce consistent, sanitized sandbox names.
- Around line 111-117: Replace the custom ModeCharDevice heuristic in the
isTerminal function with the upstream terminal detection: import
"golang.org/x/term" and change isTerminal(f *os.File) to return
term.IsTerminal(int(f.Fd())); update imports accordingly and remove the old
os.File.Stat() based logic so isTerminal uses term.IsTerminal across platforms.
In `@internal/api/network.go`:
- Line 18: Update the NetworkController.RemovePolicy signature to follow the
existing pattern by adding a context parameter and returning an error (i.e.,
change RemovePolicy(sandboxName string) to RemovePolicy(ctx context.Context,
sandboxName string) error), then update all implementations of NetworkController
and any callers (notably Destroy) to pass a ctx and propagate or handle the
returned error (at minimum log it) so cleanup failures aren’t silently ignored;
search for the RemovePolicy, NetworkController, and Destroy symbols to adjust
implementations and call sites accordingly.
In `@internal/api/profile.go`:
- Around line 1-11: The ProfileRegistry currently depends on the concrete type
profile.Profile from internal/profile which inverts the dependency direction; to
fix this, introduce a pure abstraction in the api package (e.g. define an
api.Profile interface or data struct) and update ProfileRegistry to reference
that api.Profile instead of profile.Profile, then change the internal/profile
package to implement/translate to that api.Profile (implementing the interface
or returning the api struct) so implementations depend on the api abstraction
(symbols to change: ProfileRegistry, profile.Profile, and the internal/profile
implementation).
In `@internal/api/sandbox.go`:
- Around line 40-42: The Run method on the sandbox interface buffers entire
command output into a string which can OOM or delay output; add a non-breaking
streaming variant to support large/long-running commands by extending the
interface with a RunStream(ctx context.Context, name string, command []string,
stdout io.Writer, stderr io.Writer) error (keep the existing Run for
compatibility), update implementors to provide the new method (or stub it to
call Run and copy to writers for now), and add a TODO comment on the Run
signature to migrate callers to the streaming API later; reference the Run
method and the new RunStream method when making changes.
In `@internal/api/vm.go`:
- Around line 23-33: Update the Provisioner and ShellProvider interfaces to be
context- and error-consistent: change HasCleanSnapshot to accept a context and
return (bool, error) (i.e., HasCleanSnapshot(ctx context.Context, name string)
(bool, error)) so implementations can report transient failures, and change
Shell to include context (Shell(ctx context.Context, name string) error); then
update all implementations, tests and mocks of Provisioner.HasCleanSnapshot and
ShellProvider.Shell (e.g., provider implementations in
internal/vm/lima/provider.go and any mock types) to match the new signatures and
propagate/return errors accordingly.
In `@internal/sandbox/sandbox_test.go`:
- Around line 161-162: Add tracking for RemovePolicy calls on the fake network
controller by adding a removed []string field to fakeNetworkController and
appending the sandboxName inside RemovePolicy; update tests (e.g.,
TestDeleteSandbox) to assert on network.removed contents/length. Modify the
fakeNetworkController struct to include removed []string and change the
RemovePolicy(sandboxName string) method to append sandboxName to removed so
Destroy's call to m.network.RemovePolicy(name) can be verified.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dd2e724-2132-4de1-bd24-af36ef4c4476
📒 Files selected for processing (13)
cmd/airlock/cli/cli.gointernal/api/network.gointernal/api/profile.gointernal/api/sandbox.gointernal/api/vm.gointernal/mount/mount.gointernal/network/network.gointernal/network/network_test.gointernal/sandbox/delete.gointernal/sandbox/sandbox.gointernal/sandbox/sandbox_test.gointernal/vm/lima/provider.gointernal/vm/lima/snapshot.go
…tderr capture, Shell context, test error checks
…+error, heredoc injection fix, Profile DIP, deriveSandboxName sanitization, test coverage
1 participant
Summary
create,start,stop,delete,status,list,run,reset,shell,mount,unmount,config,network lock,network unlock,network policy) to their real implementations through theDependenciesstruct, replacing stub handlersRunandResettoapi.SandboxManagerinterface — eliminates LSP/DIP violations from type assertions on concrete*sandbox.Managerapi.Provisioner,api.ShellProvider, andapi.ProfileRegistryinterfaces (ISP/DIP) — CLI never imports concrete types in handlersLimaControllerto usesandboxNameparameter instead of hardcodedvmName; add per-sandboxpoliciesmap andRemovePolicyfor proper multi-sandbox supportApply()method tomount.JSONStoreto satisfyapi.MountManagerlimactlRunExec/limactlOutputExectoexec.CommandContextLimaControllerinstances — single shared instance for Manager and Dependencies.NetworkTASKS.mdandREVIEW.mdfrom git trackingKey design decisions
Dependenciesfields are interfaces — zero concrete type imports in CLI handlers, noif concreteTypechecksProvisionerandShellProviderare separate small interfaces rather than lumping all VM operations into oneProfileRegistryinterface decouplessandbox.Managerand CLI from*profile.RegistryRemovePolicy(sandboxName)called inDestroyworkflow prevents memory leaks in the network controller's policies mapAI Use Disclosure
AI-assisted. This PR was developed with AI assistance (opencode / GLM-5.1). AI was used for code generation, refactoring suggestions, and test writing. Every line was reviewed, understood, and owned by the human committer. Security-sensitive code (network policy enforcement, mount handling, credential copying, shell command execution) was reviewed line-by-line per P6.
Test plan
go test ./...— all existing tests passgo vet ./...— cleango build ./...— cleanTestRemovePolicycovering multi-sandbox cleanup isolationTestCheckResourcesForSpecwith excessive resource spec andlen(issues) > 0assertionProvisionerandShellProviderSummary by CodeRabbit
Release Notes
New Features
runcommand to execute commands inside sandboxesconfigcommand to display resolved configurationprofilecommand to list available security profilesImprovements