feat: Go rewrite scaffold with core packages and security hardening#1
Merged
feat: Go rewrite scaffold with core packages and security hardening#1
Conversation
- api/: interfaces for SandboxManager, NetworkController, MountManager, RuntimeDetector, and VM Provider (dependency inversion) - config/: TOML + YAML config loading with validation, defaults, and format auto-detection (prefers TOML) - profile/: security profiles (strict, cautious, dev, trusted) as named presets with extensible registry - detect/: runtime auto-detection engine (node, go, rust, python, docker, compose, make, dotnet) with pluggable detectors - mount/: JSON-backed mount registry with persistence - network/: iptables-based network policy controller (Lima backend) - sysutil/: system resource detection and sandbox feasibility checks - cmd/airlock/cli/: Cobra CLI skeleton with sandbox, setup, shell commands - PRINCIPLES.md: core design rules for maintainability, scalability, usability, DRY, and SOLID
Requires full disclosure of AI tool usage in PRs, human ownership of all committed code, equal quality standards regardless of origin, and extra scrutiny for security-sensitive code.
…d injection - bin/airlock: block path traversal in airlock.toml mount paths (mounts resolving outside the project directory are rejected) - bin/airlock: require user confirmation before copying Claude credentials into sandbox VM (prevents automatic credential exposure in non-interactive environments) - bin/airlock: properly escape ANTHROPIC_API_KEY to prevent shell injection via single-quote breaking - internal/mount: add sanitizeName() to strip directory traversals and path components from mount names before constructing VM paths - internal/mount: add tests for sanitizeName and Register sanitization AI-assisted: security audit findings from Gemini CLI, fixes by Claude Code
Contributor
|
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 13 minutes and 19 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 (11)
WalkthroughAdds a new CLI-driven sandbox manager "airlock": Go module and main entrypoint, Cobra-based CLI, API surface types for VMs, sandboxes, mounts, network and detection, implementations and tests for config loading, runtime detection, mounts, networking, profiles, sysutil, and a project-level PRINCIPLES.md document. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (Cobra)
participant Config as Config Manager
participant Detector as Runtime Detector
participant Profile as Profile Registry
participant Sandbox as Sandbox Manager
participant VM as VM Provider
participant Mount as Mount Manager
participant Network as Network Controller
User->>CLI: airlock sandbox --profile <name> <path-or-url>
CLI->>Config: Load(path-or-url)
Config-->>CLI: Config (or defaults)
CLI->>Detector: Detect(path-or-url)
Detector-->>CLI: DetectedRuntime
CLI->>Profile: Get(profile)
Profile-->>CLI: Profile (policies)
CLI->>Sandbox: Create(SandboxSpec)
Sandbox->>VM: Create(VMSpec)
VM-->>Sandbox: Created
Sandbox->>Mount: Register(mounts)
Mount-->>Sandbox: Registered
Sandbox->>Network: ApplyPolicy(policy)
Network-->>Sandbox: Applied (or error)
Sandbox-->>CLI: SandboxInfo
CLI-->>User: Output result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (19)
cmd/airlock/cli/cli.go (1)
13-13: Consider using build-time version injection.Hardcoding the version as a constant requires code changes for each release. A common pattern is to use
-ldflagsduring build to inject the version.♻️ Build-time version injection pattern
-const version = "0.1.0" +// version is set via -ldflags at build time +var version = "dev"Then build with:
go build -ldflags "-X github.com/muneebs/airlock/cmd/airlock/cli.version=1.0.0" .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/cli.go` at line 13, Replace the hardcoded const version = "0.1.0" with a package-level string variable named version (e.g., var version = "dev") so it can be overridden at build time; update any references to the version variable (in this file's CLI output or flags) to use the new variable and document building with go build -ldflags "-X github.com/muneebs/airlock/cmd/airlock/cli.version=1.0.0" so release builds inject the correct version via ldflags.internal/sysutil/sysutil_test.go (1)
7-31: Good table-driven test coverage forparseMemoryString.The tests cover the main unit suffixes and edge cases. Consider adding a test case for
KiBsuffix to ensure completeness across all supported units in the implementation.📝 Optional: Add KiB test case
{"512M", 512 * 1024 * 1024}, + {"1024KiB", 1024 * 1024}, {"", 0},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sysutil/sysutil_test.go` around lines 7 - 31, Add a table-driven test case in TestParseMemoryString to cover the "KiB" suffix: update the tests slice in internal/sysutil/sysutil_test.go (inside TestParseMemoryString) to include an entry like {"1KiB", 1024} so parseMemoryString("1KiB") is validated; run the suite to ensure the new case passes.internal/api/detect.go (1)
40-42:RuntimeDetectorFunc.SupportedTypes()returns nil.Returning
nilinstead of an empty slice is technically valid in Go, but callers iterating over the result won't have issues (ranging over nil slice is safe). However, if any caller checkslen(types) == 0vstypes == nil, behavior could differ. Consider returning an empty slice for consistency:♻️ Suggested fix
func (f RuntimeDetectorFunc) SupportedTypes() []RuntimeType { - return nil + return []RuntimeType{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/detect.go` around lines 40 - 42, RuntimeDetectorFunc.SupportedTypes() currently returns nil; change it to return an empty slice instead to ensure callers that distinguish nil vs empty get a consistent empty slice (e.g., return []RuntimeType{} or make([]RuntimeType, 0)). Update the RuntimeDetectorFunc.SupportedTypes method to construct and return an empty []RuntimeType rather than nil.internal/detect/detect_test.go (2)
11-26: Consider checkingos.WriteFileerrors in tests.Multiple tests ignore errors from
os.WriteFile. While failures are rare in temp directories, checking errors improves test reliability and makes failures easier to diagnose.♻️ Example fix for one test
func TestNodeDetection(t *testing.T) { dir := t.TempDir() - os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0644) + if err := os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0644); err != nil { + t.Fatal(err) + } d := NewCompositeDetector()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/detect/detect_test.go` around lines 11 - 26, TestNodeDetection currently ignores the error returned by os.WriteFile; update the test to check that os.WriteFile succeeds and fail the test if it returns an error (e.g., call t.Fatalf on error) before calling NewCompositeDetector and d.Detect so failures creating the fixture are reported. Ensure you update the os.WriteFile call in TestNodeDetection to handle its error return and keep the rest of the assertions (result.Type, result.InstallCmd) unchanged.
228-254: Custom detector test doesn't verify file-based detection.The
staticDetectoralways returns success regardless of directory contents (line 264-265). The test createsmix.exsbut the detector never checks for it. This means if the composite detector's registration or ordering is broken, this test might still pass.Consider making
staticDetector.Detectactually check for the sentinel file:♻️ Suggested improvement for more meaningful test
func (s *staticDetector) Detect(dir string) (api.DetectedRuntime, error) { + // Check for the expected sentinel file + if _, err := os.Stat(filepath.Join(dir, "mix.exs")); err != nil { + return api.DetectedRuntime{}, fmt.Errorf("mix.exs not found") + } return s.result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/detect/detect_test.go` around lines 228 - 254, The test TestCustomDetector is ineffective because staticDetector.Detect always returns the configured result regardless of directory contents; modify staticDetector.Detect to check for a sentinel file (e.g., "mix.exs") before returning the api.DetectedRuntime so the detector only succeeds when that file exists, then leave TestCustomDetector as-is to create mix.exs and assert detection via NewCompositeDetector.Register and Detect; this ensures the test verifies file-based detection behavior rather than unconditional success.internal/mount/mount_test.go (1)
156-156: Minor: Register error ignored.The error from
s.Register()is not checked. While unlikely to fail in tests, checking errors improves reliability.♻️ Suggested fix
- s.Register(context.Background(), "test", mount) + if err := s.Register(context.Background(), "test", mount); err != nil { + t.Fatalf("Register() error: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mount/mount_test.go` at line 156, The call to s.Register(context.Background(), "test", mount) ignores its returned error; update the test to capture and assert the error (e.g., err := s.Register(...); require.NoError(t, err) or if err != nil { t.Fatalf(...) }) so failures are surfaced; reference the s.Register call and the mount variable in your change.internal/network/network.go (3)
60-65: DNS over TCP (port 53) is not allowed.Only UDP port 53 is permitted for DNS. DNS uses TCP for responses exceeding 512 bytes (common with DNSSEC) and for zone transfers. Consider allowing both protocols:
♻️ Suggested fix to allow TCP DNS
// DNS (UDP port 53) if policy.AllowDNS { if err := limactlRun(ctx, vmName, "sudo iptables -A OUTPUT -p udp --dport 53 -j ACCEPT"); err != nil { return fmt.Errorf("allow DNS: %w", err) } + if err := limactlRun(ctx, vmName, "sudo iptables -A OUTPUT -p tcp --dport 53 -j ACCEPT"); err != nil { + return fmt.Errorf("allow DNS TCP: %w", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network.go` around lines 60 - 65, The current AllowDNS branch only opens UDP port 53; update the policy.AllowDNS handling in internal/network/network.go to also allow TCP port 53 by invoking limactlRun with a rule for "-p tcp --dport 53 -j ACCEPT" (mirror the existing UDP iptables call pattern using vmName and limactlRun), and ensure both calls return errors wrapped similarly (e.g., "allow DNS tcp: %w") so TCP-based DNS responses and zone transfers are permitted alongside UDP.
50-84: Non-atomic rule application could briefly allow traffic.Flushing OUTPUT and then adding rules one-by-one creates a window where no rules exist. In a high-security context, consider using iptables-restore with a complete ruleset, or setting a default DROP policy before flushing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network.go` around lines 50 - 84, The current sequence in network.go flushes OUTPUT then adds rules via limactlRun which creates a transient allow-all window; instead build and apply the full ruleset atomically (e.g., render the complete iptables rules and apply with iptables-restore via limactlRun) or at minimum set the OUTPUT policy to DROP before flushing and then install the rules to avoid a brief permit gap; update the code paths that currently call limactlRun("sudo iptables -F OUTPUT") and the subsequent limactlRun calls controlled by policy.AllowDNS, policy.AllowEstablished, and policy.AllowOutbound so they use an atomic apply (iptables-restore) or the default-drop-then-add sequence.
89-96:IsLockedheuristic is fragile.Checking for
"DROP"substring in iptables output could produce false positives if "DROP" appears in comments, chain names, or other contexts. Consider a more precise check:♻️ Suggested improvement
func (lc *LimaController) IsLocked(ctx context.Context, sandboxName string) (bool, error) { output, err := limactlOutput(ctx, lc.vmName, "sudo iptables -L OUTPUT -n") if err != nil { return false, fmt.Errorf("check iptables: %w", err) } - return strings.Contains(output, "DROP"), nil + // Look for a DROP policy or rule at end of chain + // More robust: check if last rule is DROP or policy is DROP + lines := strings.Split(output, "\n") + for _, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), "DROP") { + return true, nil + } + } + return false, nil }Or use
iptables -L OUTPUT -n --line-numbersand check for a DROP rule explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network.go` around lines 89 - 96, The IsLocked heuristic in IsLocked is too fragile because it looks for the substring "DROP" in the full iptables output; instead call limactlOutput with a more precise iptables invocation (e.g., "sudo iptables -L OUTPUT -n --line-numbers" or "sudo iptables -S OUTPUT") and parse the returned lines to detect an explicit DROP rule by matching rule entries (for example lines starting with a rule number followed by "DROP" or rules whose target field equals "DROP"); update IsLocked to use this new command and parse logic (still using lc.vmName and limactlOutput) so false positives from comments or chain names are avoided.internal/network/network_test.go (2)
39-52: Test doesn't verify any actual behavior.This test constructs an
api.NetworkPolicystruct literal and then asserts the same values that were just assigned. It doesn't call any code under test. Consider either removing this test or converting it to test actualUnlock()behavior (similar to howTestLockPolicycallsApplyPolicy).♻️ Suggested improvement to test actual behavior
func TestUnlockPolicy(t *testing.T) { + lc := NewLimaController("test-vm") + policy := api.NetworkPolicy{ AllowDNS: true, AllowOutbound: true, AllowEstablished: true, } - if !policy.AllowOutbound { - t.Error("unlocked policy should allow outbound") - } - if !policy.AllowEstablished { - t.Error("unlocked policy should allow established") - } + // Verify the policy matches what Unlock() should produce + err := lc.ApplyPolicy(context.Background(), "test", policy) + if err == nil { + t.Error("expected error from stub limactlRun") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network_test.go` around lines 39 - 52, The current TestUnlockPolicy only asserts fields set on api.NetworkPolicy and doesn't exercise behavior; change it to construct a locked policy (e.g., with AllowOutbound/AllowEstablished false or by calling Lock()), call the code under test (either policy.Unlock() if that method exists or call ApplyPolicy(policy) to apply the unlocked policy), then assert the resulting system/policy state actually allows outbound and established traffic; update TestUnlockPolicy to invoke Unlock()/ApplyPolicy and verify the behavior rather than re-checking the literal fields.
61-82: Same issue: test only verifies struct literal values.This test creates a struct with specific values and asserts those same values. It provides no coverage of actual
LimaControllerbehavior. Consider testing through the controller's methods or documenting that this is an intentional "documentation test" for the cautious profile policy shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network_test.go` around lines 61 - 82, The test currently only asserts the literal values of api.NetworkPolicy; instead, change it to exercise real behavior by invoking the relevant LimaController method that produces or applies the cautious profile (e.g., LimaController.GetNetworkPolicy, LimaController.ConfigureNetwork, or whichever method returns the policy), then assert the returned/applied api.NetworkPolicy fields (AllowDNS, AllowOutbound, AllowEstablished, LockAfterSetup); if no such factory exists, add and test a constructor like NewCautiousNetworkPolicy that returns the policy and validate its fields rather than asserting a literal struct in TestCautiousProfilePolicy.internal/config/config_test.go (1)
226-251: Unusedpathvariable in test assertions.The
pathvariable fromConfigFile(dir)is assigned but never used in the assertions at lines 229 and 247. Either use it to verify the returned path is correct, or use blank identifier.♻️ Suggested fix
- path, format := ConfigFile(dir) - if path != "" || format != "" { + path, format := ConfigFile(dir) + if path != "" || format != "" { t.Errorf("expected no config, got path=%s format=%s", path, format) } // ... later ... - path, format = ConfigFile(dir) + _, format = ConfigFile(dir) if format != "yaml" { t.Errorf("expected yaml, got %s", format) } // ... later ... - path, format = ConfigFile(dir) + _, format = ConfigFile(dir) if format != "toml" { t.Errorf("expected toml (preferred), got %s", format) }Or better, add assertions to verify
pathcontains the expected filename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 226 - 251, The test TestConfigFileDetection assigns the return values from ConfigFile(dir) to path and format but never asserts path; either discard path with the blank identifier or add assertions to check the returned path is correct. Update the two calls to ConfigFile in TestConfigFileDetection: if you want to validate the filename, assert that path ends with or contains "airlock.yml" after the first write and "airlock.toml" after the second write (and still assert the expected formats "yaml" and "toml"), otherwise change the unused path variable to _ to silence the unused-variable issue.internal/profile/profile.go (3)
66-73: List() returns non-deterministic ordering.Map iteration order in Go is intentionally randomized. If CLI output or tests depend on consistent ordering, this could cause flaky behavior.
Proposed fix to sort the output
+import "sort" + // List returns all registered profile names. func (r *Registry) List() []string { names := make([]string, 0, len(r.profiles)) for name := range r.profiles { names = append(names, name) } + sort.Strings(names) return names }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/profile/profile.go` around lines 66 - 73, Registry.List currently returns profile names in random map iteration order; make the output deterministic by sorting the names before returning. In the Registry.List function (the loop that collects names from r.profiles), call sort.Strings(names) after populating the slice and ensure the sort package is imported; return the sorted names slice so CLI/tests get consistent ordering.
42-45: Registry is not thread-safe.If
RegisterandGet/Listare called concurrently, there's a data race on theprofilesmap. This may not be an issue if registration only happens at init time, but worth noting for future extensibility.Also applies to: 75-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/profile/profile.go` around lines 42 - 45, The Registry struct's profiles map is not protected against concurrent access; add a sync.RWMutex field to Registry and use it to guard all accesses: acquire write lock in Register (to mutate profiles) and read lock in Get and List (to read profiles); ensure List returns a copy or safe snapshot of entries while holding the read lock. Update references to Registry, Register, Get, and List to use the mutex to prevent data races.
148-168: "dev" and "trusted" profiles have identical policy settings.Both profiles enable full network, writable mounts, and Docker access. The only difference is the description text. Consider differentiating them (e.g.,
trustedcould allow additional host mounts or skip certain checks) or document why both exist for user communication purposes.Also applies to: 169-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/profile/profile.go` around lines 148 - 168, The "dev" and "trusted" profile entries in internal/profile/profile.go are identical except for Description; update them so each has a distinct purpose: either adjust one profile's policy fields (for example change MountPolicy.AllowHostMounts, NetworkPolicy.AllowOutbound/AllowEstablished, DockerPolicy.Allowed or DockerPolicy.ReadOnlySocket, or FilesystemPolicy.WritableProjectDir) to reflect a stricter or more permissive stance, or update the Description to explicitly document why both exist; locate the two profile literals named "dev" and "trusted" and modify the NetworkPolicy, MountPolicy, DockerPolicy, or FilesystemPolicy fields (or description text) to make their differences clear.internal/sysutil/sysutil.go (1)
152-165: Disk detection uses home directory, which may differ from VM storage location.
detectDiskchecks free space on the user's home directory, but VMs may be stored elsewhere (e.g., Lima uses~/.limaby default, but this could be configured differently). Consider parameterizing the path or documenting this assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sysutil/sysutil.go` around lines 152 - 165, The detectDisk function currently probes the user's home directory (os.UserHomeDir()) which may not reflect the VM storage location; change detectDisk to accept a path parameter (e.g., detectDisk(path string) (int64, string)) or add a companion detectDiskForPath that calls syscall.Statfs on the given path and falls back to the default 50GiB; update all callers of detectDisk to pass the configured VM/storage directory (or the home dir if unspecified), and keep formatBytes as-is for formatting the returned freeBytes; alternatively, document in the function comment that the path used is the user home and must be overridden by callers if VMs are stored elsewhere.internal/api/sandbox.go (1)
55-58: Inconsistent representation for optional resource fields.
CPUuses*intto distinguish "unset" from "0", butMemoryandDiskuse empty strings. While empty string works for these fields, consider using pointers consistently or documenting the convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/sandbox.go` around lines 55 - 58, The struct currently mixes pointer and value types for optional resources (CPU is *int while Memory and Disk are string), causing inconsistent semantics for "unset" vs empty; change Memory and Disk to pointer types (e.g., *string) and keep `json:"memory,omitempty" yaml:"memory,omitempty"`/`disk` tags so nil means unset, then update any code that reads or writes these fields to handle nil checks (references: CPU, Memory, Disk).internal/detect/detect.go (1)
276-289: Consider checking for .fsproj (F#) alongside .csproj.The .NET detector looks for
*.csprojand*.slnbut misses F# projects (*.fsproj) and VB.NET projects (*.vbproj). This is a minor gap if those runtimes are in scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/detect/detect.go` around lines 276 - 289, The current detection block using filepath.Glob for "*.csproj" should also check for F# and VB project files; update the logic in the function that sets matches (the filepath.Glob calls around matches, err and dir) to also test for "*.fsproj" and "*.vbproj" (in addition to "*.csproj" and "*.sln") before returning api.DetectedRuntime; ensure the fallback behavior (ErrNotDetected) remains the same and leave the returned api.DetectedRuntime (Type, InstallCmd, RunCmd, Confidence) unchanged unless you intentionally want to adjust confidence for broader detection.internal/config/config.go (1)
181-195: Import profile and runtime lists from their source packages to maintain a single source of truth.The
validProfilesmap duplicates names defined inprofile.builtins(), andvalidRuntimesduplicates the mapping indetect.ResolveRuntimeType(). Consider importing these frominternal/profileandinternal/detectinstead of hardcoding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 181 - 195, The validate function currently hardcodes validProfiles and validRuntimes; replace those maps by importing and using the canonical sources: call the profile package's Builtins (e.g., profile.Builtins()) to build the set used for checking cfg.Security.Profile, and call the detect package's ResolveRuntimeType or its canonical runtime list (e.g., detect.KnownRuntimes or a helper that ResolveRuntimeType uses) to build the set for validating cfg.Runtime.Type; remove the duplicated literal maps, construct lookup sets from those upstream values, and keep the same error behavior in validate when a value is invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/airlock`:
- Around line 678-686: The script currently only sets _skip_claude_creds when
the user explicitly answers no in the TTY prompt, so in non-TTY sessions
credentials get copied by default; change the logic so non-TTY defaults to
skipping copying by initializing or setting _skip_claude_creds=1 when stdin is
not a TTY (i.e., inside the else branch of the if [ -t 0 ] check or by
defaulting the variable before the prompt), and only clear or set it to 0 when
an affirmative interactive confirmation is received (reference the interactive
prompt block and the _skip_claude_creds variable).
In `@internal/api/vm.go`:
- Around line 35-40: VMMount is missing a destination (guest) path so consumers
of api.VMSpec.Mounts won't know where to mount host directories; add a GuestPath
string field to the VMMount struct (tagged `json:"guest_path"
yaml:"guest_path"`) alongside HostPath, Writable, and Inotify, then update any
code that constructs or validates VMMount (references to VMSpec.Mounts) to
require non-empty GuestPath and use it as the mount target inside the VM.
In `@internal/config/config.go`:
- Around line 203-210: The mount path validation in the loop over cfg.Mounts
currently only rejects absolute paths but allows path traversal like "../";
update the validation in the same loop (where you check m.Path == "" and
strings.HasPrefix(m.Path, "/")) to also reject any path that would traverse
upward: compute a cleaned/normalized form (use filepath.Clean) and ensure it
does not start with ".." nor contain any ".." path segment (e.g., split on
filepath.Separator or check that the first element != ".." and that no element
== ".."); return a clear error (e.g., "mounts[].path must be relative and must
not contain path traversal sequences") when this check fails. Ensure you
reference the same cfg.Mounts loop and m.Path variable so tests/code using that
logic pick up the stricter validation.
- Around line 171-176: The current loop in the config defaulting sets mounts
with nil Writable to true which is insecure; change the default to false so
unspecified mounts become read-only by default: locate the loop handling
cfg.Mounts and the Writable pointer assignment (the block referencing
cfg.Mounts[i].Writable) and set the default boolean to false (e.g., w := false)
instead of true; update any related comments/tests that assume
writable-by-default to reflect the new cautious default.
In `@internal/detect/detect.go`:
- Around line 199-201: The current assignment when hasPyproject sets
runtime.RunCmd to the invalid command "python -m ."; replace this by not
emitting an invalid module invocation — set runtime.RunCmd to an empty string or
a clear placeholder (e.g., "") so consumers know it requires user configuration,
or leave it unset; update the conditional that checks hasPyproject and the
runtime.RunCmd assignment (referencing hasPyproject and runtime.RunCmd) so no
invalid "python -m ." value is produced.
In `@internal/network/network.go`:
- Around line 46-87: ApplyPolicy currently ignores NetworkPolicy.LockAfterSetup;
add support by making ApplyPolicy check for an existing lock at the top (e.g.,
call lc.IsNetworkLocked(ctx, sandboxName) and return an error if locked) and,
after successfully applying iptables rules, if policy.LockAfterSetup is true
call a new method lc.LockNetwork(ctx, sandboxName) to persist a lock
(implementation detail up to you — e.g., create a per-sandbox lock file or
controller state) and implement corresponding IsNetworkLocked (and optional
UnlockNetwork) methods on LimaController so future ApplyPolicy invocations
respect the lock.
In `@internal/sysutil/sysutil.go`:
- Around line 101-111: The multipliers map in internal/sysutil/sysutil.go
currently uses binary (1024-based) values for both IEC (GiB/MiB/KiB) and SI
(GB/MB/KB) units; change the entries for "GB", "MB", and "KB" to decimal
multipliers (1000^3, 1000^2, 1000) while leaving "GiB", "MiB", and "KiB" as
1024-based, and ensure any function that uses this map (the byte-size parsing
routine that references multipliers) still behaves correctly; update or add
tests and comments to reflect the SI vs IEC distinction.
In `@PRINCIPLES.md`:
- Line 31: Replace the typo in the PRINCIPLES.md heading: change "Userability"
to "Usability" (update the header line that currently reads "## 3. Userability"
to "## 3. Usability") so the section title is correct.
---
Nitpick comments:
In `@cmd/airlock/cli/cli.go`:
- Line 13: Replace the hardcoded const version = "0.1.0" with a package-level
string variable named version (e.g., var version = "dev") so it can be
overridden at build time; update any references to the version variable (in this
file's CLI output or flags) to use the new variable and document building with
go build -ldflags "-X github.com/muneebs/airlock/cmd/airlock/cli.version=1.0.0"
so release builds inject the correct version via ldflags.
In `@internal/api/detect.go`:
- Around line 40-42: RuntimeDetectorFunc.SupportedTypes() currently returns nil;
change it to return an empty slice instead to ensure callers that distinguish
nil vs empty get a consistent empty slice (e.g., return []RuntimeType{} or
make([]RuntimeType, 0)). Update the RuntimeDetectorFunc.SupportedTypes method to
construct and return an empty []RuntimeType rather than nil.
In `@internal/api/sandbox.go`:
- Around line 55-58: The struct currently mixes pointer and value types for
optional resources (CPU is *int while Memory and Disk are string), causing
inconsistent semantics for "unset" vs empty; change Memory and Disk to pointer
types (e.g., *string) and keep `json:"memory,omitempty"
yaml:"memory,omitempty"`/`disk` tags so nil means unset, then update any code
that reads or writes these fields to handle nil checks (references: CPU, Memory,
Disk).
In `@internal/config/config_test.go`:
- Around line 226-251: The test TestConfigFileDetection assigns the return
values from ConfigFile(dir) to path and format but never asserts path; either
discard path with the blank identifier or add assertions to check the returned
path is correct. Update the two calls to ConfigFile in TestConfigFileDetection:
if you want to validate the filename, assert that path ends with or contains
"airlock.yml" after the first write and "airlock.toml" after the second write
(and still assert the expected formats "yaml" and "toml"), otherwise change the
unused path variable to _ to silence the unused-variable issue.
In `@internal/config/config.go`:
- Around line 181-195: The validate function currently hardcodes validProfiles
and validRuntimes; replace those maps by importing and using the canonical
sources: call the profile package's Builtins (e.g., profile.Builtins()) to build
the set used for checking cfg.Security.Profile, and call the detect package's
ResolveRuntimeType or its canonical runtime list (e.g., detect.KnownRuntimes or
a helper that ResolveRuntimeType uses) to build the set for validating
cfg.Runtime.Type; remove the duplicated literal maps, construct lookup sets from
those upstream values, and keep the same error behavior in validate when a value
is invalid.
In `@internal/detect/detect_test.go`:
- Around line 11-26: TestNodeDetection currently ignores the error returned by
os.WriteFile; update the test to check that os.WriteFile succeeds and fail the
test if it returns an error (e.g., call t.Fatalf on error) before calling
NewCompositeDetector and d.Detect so failures creating the fixture are reported.
Ensure you update the os.WriteFile call in TestNodeDetection to handle its error
return and keep the rest of the assertions (result.Type, result.InstallCmd)
unchanged.
- Around line 228-254: The test TestCustomDetector is ineffective because
staticDetector.Detect always returns the configured result regardless of
directory contents; modify staticDetector.Detect to check for a sentinel file
(e.g., "mix.exs") before returning the api.DetectedRuntime so the detector only
succeeds when that file exists, then leave TestCustomDetector as-is to create
mix.exs and assert detection via NewCompositeDetector.Register and Detect; this
ensures the test verifies file-based detection behavior rather than
unconditional success.
In `@internal/detect/detect.go`:
- Around line 276-289: The current detection block using filepath.Glob for
"*.csproj" should also check for F# and VB project files; update the logic in
the function that sets matches (the filepath.Glob calls around matches, err and
dir) to also test for "*.fsproj" and "*.vbproj" (in addition to "*.csproj" and
"*.sln") before returning api.DetectedRuntime; ensure the fallback behavior
(ErrNotDetected) remains the same and leave the returned api.DetectedRuntime
(Type, InstallCmd, RunCmd, Confidence) unchanged unless you intentionally want
to adjust confidence for broader detection.
In `@internal/mount/mount_test.go`:
- Line 156: The call to s.Register(context.Background(), "test", mount) ignores
its returned error; update the test to capture and assert the error (e.g., err
:= s.Register(...); require.NoError(t, err) or if err != nil { t.Fatalf(...) })
so failures are surfaced; reference the s.Register call and the mount variable
in your change.
In `@internal/network/network_test.go`:
- Around line 39-52: The current TestUnlockPolicy only asserts fields set on
api.NetworkPolicy and doesn't exercise behavior; change it to construct a locked
policy (e.g., with AllowOutbound/AllowEstablished false or by calling Lock()),
call the code under test (either policy.Unlock() if that method exists or call
ApplyPolicy(policy) to apply the unlocked policy), then assert the resulting
system/policy state actually allows outbound and established traffic; update
TestUnlockPolicy to invoke Unlock()/ApplyPolicy and verify the behavior rather
than re-checking the literal fields.
- Around line 61-82: The test currently only asserts the literal values of
api.NetworkPolicy; instead, change it to exercise real behavior by invoking the
relevant LimaController method that produces or applies the cautious profile
(e.g., LimaController.GetNetworkPolicy, LimaController.ConfigureNetwork, or
whichever method returns the policy), then assert the returned/applied
api.NetworkPolicy fields (AllowDNS, AllowOutbound, AllowEstablished,
LockAfterSetup); if no such factory exists, add and test a constructor like
NewCautiousNetworkPolicy that returns the policy and validate its fields rather
than asserting a literal struct in TestCautiousProfilePolicy.
In `@internal/network/network.go`:
- Around line 60-65: The current AllowDNS branch only opens UDP port 53; update
the policy.AllowDNS handling in internal/network/network.go to also allow TCP
port 53 by invoking limactlRun with a rule for "-p tcp --dport 53 -j ACCEPT"
(mirror the existing UDP iptables call pattern using vmName and limactlRun), and
ensure both calls return errors wrapped similarly (e.g., "allow DNS tcp: %w") so
TCP-based DNS responses and zone transfers are permitted alongside UDP.
- Around line 50-84: The current sequence in network.go flushes OUTPUT then adds
rules via limactlRun which creates a transient allow-all window; instead build
and apply the full ruleset atomically (e.g., render the complete iptables rules
and apply with iptables-restore via limactlRun) or at minimum set the OUTPUT
policy to DROP before flushing and then install the rules to avoid a brief
permit gap; update the code paths that currently call limactlRun("sudo iptables
-F OUTPUT") and the subsequent limactlRun calls controlled by policy.AllowDNS,
policy.AllowEstablished, and policy.AllowOutbound so they use an atomic apply
(iptables-restore) or the default-drop-then-add sequence.
- Around line 89-96: The IsLocked heuristic in IsLocked is too fragile because
it looks for the substring "DROP" in the full iptables output; instead call
limactlOutput with a more precise iptables invocation (e.g., "sudo iptables -L
OUTPUT -n --line-numbers" or "sudo iptables -S OUTPUT") and parse the returned
lines to detect an explicit DROP rule by matching rule entries (for example
lines starting with a rule number followed by "DROP" or rules whose target field
equals "DROP"); update IsLocked to use this new command and parse logic (still
using lc.vmName and limactlOutput) so false positives from comments or chain
names are avoided.
In `@internal/profile/profile.go`:
- Around line 66-73: Registry.List currently returns profile names in random map
iteration order; make the output deterministic by sorting the names before
returning. In the Registry.List function (the loop that collects names from
r.profiles), call sort.Strings(names) after populating the slice and ensure the
sort package is imported; return the sorted names slice so CLI/tests get
consistent ordering.
- Around line 42-45: The Registry struct's profiles map is not protected against
concurrent access; add a sync.RWMutex field to Registry and use it to guard all
accesses: acquire write lock in Register (to mutate profiles) and read lock in
Get and List (to read profiles); ensure List returns a copy or safe snapshot of
entries while holding the read lock. Update references to Registry, Register,
Get, and List to use the mutex to prevent data races.
- Around line 148-168: The "dev" and "trusted" profile entries in
internal/profile/profile.go are identical except for Description; update them so
each has a distinct purpose: either adjust one profile's policy fields (for
example change MountPolicy.AllowHostMounts,
NetworkPolicy.AllowOutbound/AllowEstablished, DockerPolicy.Allowed or
DockerPolicy.ReadOnlySocket, or FilesystemPolicy.WritableProjectDir) to reflect
a stricter or more permissive stance, or update the Description to explicitly
document why both exist; locate the two profile literals named "dev" and
"trusted" and modify the NetworkPolicy, MountPolicy, DockerPolicy, or
FilesystemPolicy fields (or description text) to make their differences clear.
In `@internal/sysutil/sysutil_test.go`:
- Around line 7-31: Add a table-driven test case in TestParseMemoryString to
cover the "KiB" suffix: update the tests slice in
internal/sysutil/sysutil_test.go (inside TestParseMemoryString) to include an
entry like {"1KiB", 1024} so parseMemoryString("1KiB") is validated; run the
suite to ensure the new case passes.
In `@internal/sysutil/sysutil.go`:
- Around line 152-165: The detectDisk function currently probes the user's home
directory (os.UserHomeDir()) which may not reflect the VM storage location;
change detectDisk to accept a path parameter (e.g., detectDisk(path string)
(int64, string)) or add a companion detectDiskForPath that calls syscall.Statfs
on the given path and falls back to the default 50GiB; update all callers of
detectDisk to pass the configured VM/storage directory (or the home dir if
unspecified), and keep formatBytes as-is for formatting the returned freeBytes;
alternatively, document in the function comment that the path used is the user
home and must be overridden by callers if VMs are stored elsewhere.
🪄 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: ceebfa95-90a1-49e2-8aad-96c30a70fa3d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
PRINCIPLES.mdbin/airlockcmd/airlock/cli/cli.gogo.modinternal/api/detect.gointernal/api/mount.gointernal/api/network.gointernal/api/sandbox.gointernal/api/vm.gointernal/config/config.gointernal/config/config_test.gointernal/detect/detect.gointernal/detect/detect_test.gointernal/mount/mount.gointernal/mount/mount_test.gointernal/network/network.gointernal/network/network_test.gointernal/profile/profile.gointernal/profile/profile_test.gointernal/sysutil/sysutil.gointernal/sysutil/sysutil_test.gomain.go
- bin/airlock: default to skipping credential copy in non-TTY sessions (_skip_claude_creds=1 by default, only cleared on explicit 'yes') - internal/api/vm.go: add GuestPath to VMMount so consumers know where host dirs are mounted inside the VM - internal/config: default mount writable to false (read-only), add path traversal validation (reject '..' segments in mount paths) - internal/detect: remove invalid 'python -m .' run command for pyproject.toml projects (leave empty — requires user config) - internal/network: track applied policy state in LimaController, add CurrentPolicy() method, IsLocked() uses tracked state - internal/sysutil: fix SI vs IEC unit multipliers (GB=1000^3, GiB=1024^3, etc.) - PRINCIPLES.md: fix typo 'Userability' -> 'Usability' AI-assisted: audit findings verified and fixed by Claude Code
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
PRINCIPLES.md (1)
15-15: Replacegolintwith a maintained Go linting baseline.
golintis deprecated (archived since 2021); encoding it into principles/CI policy can mislead contributors and lock the project to stale tooling. Go's current standard (2025-2026) isstaticcheckfor deep analysis andgolangci-lintas the community de facto standard for comprehensive linting. Consider keeping the policy tool-agnostic (e.g., "use a maintained Go linter") and let CI enforce the specific tool.Also applies to: line 90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PRINCIPLES.md` at line 15, Update the "Go standard style." guideline to remove the deprecated reference to `golint` and replace it with a recommendation to use a maintained Go linter (for example mention `staticcheck` and/or `golangci-lint` or phrase it tool-agnostic as "use a maintained Go linter such as staticcheck or golangci-lint"); apply the same replacement where `golint` is referenced again (around line 90) so the PRINCIPLES.md text no longer directs contributors to the archived `golint` tool.internal/config/config_test.go (2)
365-365: Use the table test case name for subtest naming.Line 365 currently uses
tt.path; usingtt.namemakes output cleaner and avoids slash-heavy test names.Suggested change
- t.Run(tt.path, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` at line 365, Replace the subtest name that currently uses tt.path with the table-driven test case's descriptive field tt.name in the t.Run call inside the test function so subtests are labeled by the test case name; locate the t.Run invocation where tt.path is passed (in the table-driven loop) and change it to use tt.name instead.
28-30: Prefer deriving expected profile fromDefaults()instead of hard-coding.Line 28 hard-codes
"cautious", which can drift frominternal/config/config.go:69-80and make this test brittle.Suggested change
- if cfg.Security.Profile != "cautious" { - t.Errorf("expected default profile cautious, got %s", cfg.Security.Profile) - } + defaults := Defaults() + if cfg.Security.Profile != defaults.Security.Profile { + t.Errorf("expected default profile %s, got %s", defaults.Security.Profile, cfg.Security.Profile) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 28 - 30, The test currently hard-codes "cautious" for the expected security profile which can drift from the canonical default; change the assertion to derive the expected value from the Defaults() helper instead—call Defaults() (or the exported function that returns the default config), read Defaults().Security.Profile into an expected variable, and compare cfg.Security.Profile against that expected value (update the assertion in config_test.go that references cfg.Security.Profile accordingly).internal/sysutil/sysutil.go (1)
170-183: Avoid rounding up displayed capacities informatBytes.Using
%.0fcan overstate values (e.g., 1.5GiB shown as2GiB), which is confusing in insufficiency messages.Small display-only tweak
- return fmt.Sprintf("%.0fGiB", float64(b)/float64(GiB)) + return fmt.Sprintf("%.1fGiB", float64(b)/float64(GiB))(Apply similarly for MiB/KiB, or floor instead of rounding.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sysutil/sysutil.go` around lines 170 - 183, The formatBytes function currently uses floating rounding "%.0f" which can round up (e.g., 1.5GiB -> "2GiB"); change format to avoid rounding up by using integer truncation instead (e.g., use b/GiB, b/MiB, b/KiB to produce whole-number GiB/MiB/KiB strings) and apply the same truncation approach for the GiB, MiB and KiB cases (and ensure the default case remains bytes); update the switch in formatBytes and the GiB/MiB/KiB constant uses accordingly so displayed capacities never overstate available size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 99-103: The test dereferences cfg.Mounts[i].Writable without
checking for nil which can panic; update the assertions in Test (using
cfg.Mounts) to first assert the pointer is non-nil (e.g., fail with t.Fatalf or
t.Fatalf-like message if cfg.Mounts[0].Writable == nil) and only then
dereference to check its boolean value for mount 0 and mount 1, referencing
cfg.Mounts[0].Writable and cfg.Mounts[1].Writable in your guards.
In `@internal/detect/detect.go`:
- Around line 199-207: The code currently sets runtime.RunCmd to "python app.py"
whenever main.py is missing, which can produce a failing default; update the
logic in the block that uses hasPyproject and mainPy so you check for the
existence of app.py before assigning the fallback: first test os.Stat(mainPy)
and set runtime.RunCmd = "python main.py" if present, otherwise test appPy :=
filepath.Join(dir, "app.py") and only set runtime.RunCmd = "python app.py" if
that file exists; if neither exists leave runtime.RunCmd empty (or nil) so the
detector doesn't emit a guaranteed-broken command.
- Around line 219-223: The detectors only check for docker-compose.yml|yaml and
miss modern compose filenames; add a shared helper (e.g., hasComposeFile(dir
string) bool) that calls fileExists for all four names
("docker-compose.yml","docker-compose.yaml","compose.yml","compose.yaml") and
replace the inline fileExists checks in both dockerfileDetector and
dockerComposeDetector with this helper so the logic that returns ErrNotDetected
(or continues) uses the unified check; update the checks at the spots
referencing fileExists (including the ones around ErrNotDetected) to call
hasComposeFile instead.
- Around line 61-75: The loop in CompositeDetector.Detect currently uses a
concrete type assertion to check ErrNotDetected and accumulates lastErr, which
fails to detect wrapped ErrNotDetected and silently discards higher-priority
detector errors; change the logic to use errors.As to detect wrapped
ErrNotDetected values and, if an error from a detector is not an ErrNotDetected,
return it immediately (fail fast) instead of continuing to lower-priority
detectors, otherwise continue scanning; update handling around the detectors
slice, result/err variables and remove/repurpose lastErr accordingly so only
ErrNotDetected falls through to returning ErrNotDetected{Dir: dir}.
- Around line 311-313: The fileExists helper should be changed to return (bool,
error) so callers can distinguish "not found" from real filesystem errors:
update fileExists to call os.Stat(filepath.Join(dir,name)) and return (true,
nil) when err==nil, return (false, nil) when os.IsNotExist(err), and return
(false, err) for any other error; then update callers (notably
CompositeDetector.Detect and any other uses of fileExists) to handle the error
return and propagate non-nil errors up instead of treating them as simple false
negatives.
In `@internal/network/network_test.go`:
- Around line 14-37: Tests like TestLockPolicy recreate an expected
api.NetworkPolicy instead of exercising the real code path; update the tests to
call the production entry points (e.g., use lc.Lock and lc.Unlock or the actual
policy-builder function) and assert on the policy produced, or refactor the
policy construction into a helper (e.g., BuildCautiousPolicy or whatever
function builds the api.NetworkPolicy) and call that from the test; also make
limactl executable behavior injectable on NewLimaController (or provide a fake
limactl runner used by ApplyPolicy/limactlRun) so tests can assert the
constructed policy without invoking real limactl.
In `@internal/network/network.go`:
- Around line 57-90: Current code mutates OUTPUT rules one-by-one via limactlRun
causing a window where a failure leaves the VM exposed; instead build the
complete iptables ruleset (respecting policy.AllowDNS, policy.AllowEstablished,
policy.AllowOutbound, always allow loopback) as a single string and apply it
atomically using iptables-restore via limactlRun (e.g. run "sudo
iptables-restore" and feed the assembled ruleset on stdin), or alternatively
stage rules in a temporary chain and swap it into OUTPUT only after all commands
succeed; update the code that currently calls limactlRun repeatedly to assemble
the rules and call limactlRun once to install them atomically (reference
limactlRun, vmName, and the policy fields to locate where to change).
In `@internal/sysutil/sysutil.go`:
- Around line 134-150: detectMemory currently assumes Linux and returns a
hardcoded 16GiB when /proc/meminfo is missing; replace this with OS-specific
detectors and a non-confident fallback. Move the Linux-specific parsing of
/proc/meminfo out of detectMemory into a file with a linux build tag (e.g.,
detect_memory_linux.go) that implements detectMemory by reading /proc/meminfo
and returning (memBytes, "linux:/proc/meminfo"). Add a Darwin implementation
(e.g., detect_memory_darwin.go) that uses sysctl hw.memsize and returns
(memBytes, "darwin:sysctl") and a default implementation (e.g.,
detect_memory_others.go) that returns (0, "unknown") or another clearly
conservative sentinel instead of 16GiB; update any callers of detectMemory to
handle the (0,"unknown") case appropriately. Ensure references to detectMemory,
memBytes, and the returned source string are used so callers can detect
"unknown" rather than assuming the value is accurate.
- Around line 95-131: parseMemoryString currently returns 0 on parse errors,
which makes invalid size strings indistinguishable from a zero value and lets
malformed inputs bypass checks in CheckResources; fix by adding explicit
validation: either change parseMemoryString to return (int64, error) and
propagate that error to callers (notably CheckResources) so malformed inputs
produce validation errors, or add a new validation step in
internal/config/config.go's validate() that calls parseMemoryString and rejects
any non-empty string that fails to parse; update all call sites (e.g.,
CheckResources) to handle the error instead of treating 0 as "no requirement".
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Line 365: Replace the subtest name that currently uses tt.path with the
table-driven test case's descriptive field tt.name in the t.Run call inside the
test function so subtests are labeled by the test case name; locate the t.Run
invocation where tt.path is passed (in the table-driven loop) and change it to
use tt.name instead.
- Around line 28-30: The test currently hard-codes "cautious" for the expected
security profile which can drift from the canonical default; change the
assertion to derive the expected value from the Defaults() helper instead—call
Defaults() (or the exported function that returns the default config), read
Defaults().Security.Profile into an expected variable, and compare
cfg.Security.Profile against that expected value (update the assertion in
config_test.go that references cfg.Security.Profile accordingly).
In `@internal/sysutil/sysutil.go`:
- Around line 170-183: The formatBytes function currently uses floating rounding
"%.0f" which can round up (e.g., 1.5GiB -> "2GiB"); change format to avoid
rounding up by using integer truncation instead (e.g., use b/GiB, b/MiB, b/KiB
to produce whole-number GiB/MiB/KiB strings) and apply the same truncation
approach for the GiB, MiB and KiB cases (and ensure the default case remains
bytes); update the switch in formatBytes and the GiB/MiB/KiB constant uses
accordingly so displayed capacities never overstate available size.
In `@PRINCIPLES.md`:
- Line 15: Update the "Go standard style." guideline to remove the deprecated
reference to `golint` and replace it with a recommendation to use a maintained
Go linter (for example mention `staticcheck` and/or `golangci-lint` or phrase it
tool-agnostic as "use a maintained Go linter such as staticcheck or
golangci-lint"); apply the same replacement where `golint` is referenced again
(around line 90) so the PRINCIPLES.md text no longer directs contributors to the
archived `golint` tool.
🪄 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: de3bbe36-70c9-4896-8064-3c823c80fbc6
📒 Files selected for processing (10)
PRINCIPLES.mdbin/airlockinternal/api/vm.gointernal/config/config.gointernal/config/config_test.gointernal/detect/detect.gointernal/network/network.gointernal/network/network_test.gointernal/sysutil/sysutil.gointernal/sysutil/sysutil_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/sysutil/sysutil_test.go
- internal/api/vm.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/airlock
- internal/config/config.go
- internal/config: add nil guards before dereferencing Writable pointers in mount tests (prevents potential panics) - internal/detect: fileExists now returns (bool, error) to distinguish 'not found' from real filesystem errors; all 16 call sites and hasComposeFile propagate errors; use errors.As for wrapped ErrNotDetected in CompositeDetector; fail fast on real errors instead of continuing to lower-priority detectors; check app.py existence before using it as run command fallback; support modern compose filenames (compose.yml/compose.yaml) - internal/network: injectable CommandRunner/OutputRunner for testability; tests now exercise real Lock/Unlock/ApplyPolicy code paths via fakes; atomic iptables-restore instead of one-by-one rule mutation (eliminates exposure window on failure) - internal/sysutil: parseMemoryString returns (int64, error) so malformed inputs don't silently bypass resource checks; CheckResources returns error on invalid memory/disk strings; OS-specific memory detection (darwin: sysctl, linux: /proc/meminfo, others: 0/'unknown') instead of hardcoded 16GiB fallback; Resources.MemorySource field lets callers detect unknown detection AI-assisted: findings verified and fixed by Claude Code
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
5 tasks
muneebs
added a commit
that referenced
this pull request
Apr 16, 2026
PR #3 — Sandbox Orchestrator: - Add internal/sandbox/ package implementing api.SandboxManager - sandbox.go: Manager struct with JSON-backed state persistence, Resetter interface for snapshot support, injectable ResourceChecker - create.go: Full create workflow (validate -> resources -> runtime detection -> profile resolution -> VM provision -> network policy -> mount registration) - run.go: Run workflow (start VM, exec command as user) + Start method - stop.go: Graceful stop workflow - delete.go: Destroy workflow (stop -> delete VM -> cleanup mounts -> state) - reset.go: Reset workflow (check snapshot -> stop -> restore -> restart) - list.go: List and Status with live state reconciliation from VM - sandbox_test.go: 36 tests with faked dependencies covering all workflows, error paths, persistence, and edge cases - Compile-time interface check: var _ api.SandboxManager = (*Manager)(nil) Security fixes (from audit): - Fix #1: Lima config file permissions 0644 -> 0600 (owner-only) - Fix #2: Block sensitive host mount paths (/etc, /root, /proc, /sys, /dev, /home, /var/run/docker.sock, etc.) via allowlist - Fix #3: Reject shell metacharacters (;;&|$`!) in provision commands - Fix #4: Individual argument escaping in ExecAsUser preserves argument boundaries (was joining then escaping, now escapes each arg separately) - Fix #5: Permission masking in snapshot/restore strips SUID, SGID, and world-write bits via safeFilePerm() mask to 0755 AI-assisted: this PR was generated with AI assistance. Every line has been reviewed, understood, and owned by the human who commits it.
muneebs
added a commit
that referenced
this pull request
Apr 16, 2026
* feat: implement sandbox orchestrator and harden Lima provider PR #3 — Sandbox Orchestrator: - Add internal/sandbox/ package implementing api.SandboxManager - sandbox.go: Manager struct with JSON-backed state persistence, Resetter interface for snapshot support, injectable ResourceChecker - create.go: Full create workflow (validate -> resources -> runtime detection -> profile resolution -> VM provision -> network policy -> mount registration) - run.go: Run workflow (start VM, exec command as user) + Start method - stop.go: Graceful stop workflow - delete.go: Destroy workflow (stop -> delete VM -> cleanup mounts -> state) - reset.go: Reset workflow (check snapshot -> stop -> restore -> restart) - list.go: List and Status with live state reconciliation from VM - sandbox_test.go: 36 tests with faked dependencies covering all workflows, error paths, persistence, and edge cases - Compile-time interface check: var _ api.SandboxManager = (*Manager)(nil) Security fixes (from audit): - Fix #1: Lima config file permissions 0644 -> 0600 (owner-only) - Fix #2: Block sensitive host mount paths (/etc, /root, /proc, /sys, /dev, /home, /var/run/docker.sock, etc.) via allowlist - Fix #3: Reject shell metacharacters (;;&|$`!) in provision commands - Fix #4: Individual argument escaping in ExecAsUser preserves argument boundaries (was joining then escaping, now escapes each arg separately) - Fix #5: Permission masking in snapshot/restore strips SUID, SGID, and world-write bits via safeFilePerm() mask to 0755 AI-assisted: this PR was generated with AI assistance. Every line has been reviewed, understood, and owned by the human who commits it. * fix(sandbox): harden orchestrator against races, stale state, and orphaned VMs - Create: make existence check + name reservation atomic (TOCTOU fix); add concurrent duplicate test proving only one sandbox is created - Create: delete VM on Start failure to avoid orphaned VMs - Create: collapse redundant Ephemeral if/else into unconditional assignment - Create: return empty SandboxInfo on final put error (was returning *info with stale state) - Delete: return error from IsRunning instead of silently proceeding - Delete: unregister all mounts by listing first, not just the sandbox-named one - Stop: re-read info under lock before each state mutation (stale pointer fix) - Stop: surface m.put errors instead of discarding them - Stop: surface m.put errors on success path; keep discard on error path where Stop error takes priority - resolveResources: nil-safe CPU dereference via derefInt helper - TestCheckResourcesForSpec: assert non-nil return instead of discarding result * fix(lima): whitelist provision command chars and wrap shell-escaped args in quotes - Replace blacklist regex (dangerousShellChars) with whitelist regex (safeProvisionCmd) that only permits alphanumerics, spaces, hyphens, underscores, dots, slashes, colons, equals, tildes, plus, comma, hash, and at-sign. Rejects newline, carriage return, quotes, parentheses, redirection, backslash, and all shell metacharacters. - Update shellEscape to wrap each argument in single quotes so spaces and special characters preserve argument boundaries when passed to bash -c. Previously "hello world" would be split into two args. - Replace TestShellEscape with TestShellEscapeWrapsInQuotes verifying quoting behavior; strengthen TestLimaProviderExecAsUserArgPreservation to assert single-quoted "hello world" token instead of substring match. - Rename TestGenerateConfigValidationDangerousProvisionCmd to TestGenerateConfigValidationProvisionCmdWhitelist with expanded cases covering 10 safe commands and 14 injection patterns. * fix(sandbox): eliminate stale pointer races and surface persistence errors - list.go (Status): move info.State mutation inside m.mu lock scope; re-read via m.get(name) to avoid stale pointer after concurrent writes - reset.go: re-read info via m.get(name) under lock before every state mutation; guard against nil info if sandbox deleted concurrently; surface m.put errors on success paths (Stopped, Running state transitions); keep discard on error paths where primary error dominates - run.go (Run + Start): re-read info via m.get(name) under lock before mutating State; only update if current state is not already Running; surface m.put errors instead of discarding
7 tasks
This was referenced Apr 16, 2026
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
1 participant
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.
AI Transparency Disclosure
This PR was AI-assisted. Architecture, interfaces, and package structure were designed by a human. Code generation was done by Claude Code (Anthropic), with human review and iteration on each package. Security audit findings were produced by Gemini CLI, fixes applied by Claude Code, reviewed by human.
Summary
SandboxManager,Provider,NetworkController,MountManager,RuntimeDetector) enabling SOLID dependency inversionSecurity Hardening
Three vulnerabilities identified and fixed:
airlock.tomlmounts — mount paths resolving outside the project directory are now blocked (bash script + GosanitizeName())claudecommand now requires explicit user confirmation before copying~/.claudeinto the sandbox VM, preventing credential theft by untrusted codeANTHROPIC_API_KEY— properly escape single quotes in the API key to prevent breaking out of the shell commandPackage Structure
internal/api/internal/config/internal/profile/internal/detect/internal/mount/internal/network/internal/sysutil/cmd/airlock/cli/All 37 tests pass. The CLI builds successfully. Lima exec stubs are in place for the next phase (Lima VM provider implementation).
What's Next
internal/vm/lima) — the main implementation blockerinternal/sandbox) — wires everything togetherChecklist
go test ./...)Summary by CodeRabbit
New Features
Improvements
Documentation