feat: documentation, polish, CLI styling, and VM creation fixes#6
Merged
feat: documentation, polish, CLI styling, and VM creation fixes#6
Conversation
…r Go project - README: rewritten for Go rewrite with commands, security profiles, configuration reference, architecture, and development instructions - CONTRIBUTING.md: development setup, code style, PR process, AI disclosure - Makefile: build, test, vet, fmt, lint, install, clean targets - .gitignore: replaced Python-centric entries with Go-appropriate ones, added TASKS.md to prevent local task tracker from being committed AI use disclosure: AI-assisted (Claude). Generated initial drafts of all documentation files; human-reviewed and modified.
- Add .goreleaser.yml for macOS arm64/amd64 binary releases - Add .goreleaser-release.tpl for structured release notes - Rewrite install.sh to download pre-built binary from GitHub Releases - Update CI to use Makefile targets and add goreleaser release job on tag push - Update Makefile with ldflags version injection, dist/snapshot/release/changelog targets - Update README install section (one-liner first, then from source) - Upgrade Go to 1.24 (required by charmbracelet deps) - Remove old bin/airlock bash script (replaced by Go binary)
- Add cmd/airlock/cli/tui package with lipgloss styles, tables, phase output - Replace bubbletea spinner with simple sequential ✓/✗ output (no signal issues) - Style list, profile, version, status, lock/unlock commands with colors - Add --json flag to sandbox create and status commands - Force ANSI 256-color profile so colors render in all terminals - Print errors to stderr with CleanError (strips limactl timestamps) Bug fixes: - limactl create: add --tty=false to prevent interactive editor hang - limactl create: write config to temp file instead of instance directory (writing to ~/.lima/<name>/lima.yaml caused "instance already exists") - Remove invalid mountInotify field from Lima config (not a real field) - Allow overwriting sandboxes in errored state in Manager.Create - Stream limactl create stderr to terminal (user sees progress) - Keep limactl start stderr buffered (streaming prevents daemonization) - Clean up stale VM directories on create failure AI use disclosure: AI-assisted (Claude). Generated initial drafts; human-reviewed and modified.
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 39 minutes and 49 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 (3)
WalkthroughThis PR transitions the airlock project from a Bash-based npm/pnpm/bun security sandbox CLI to a Go-based sandbox lifecycle manager with a TUI. It updates CI/release infrastructure for Go binaries, introduces phased provisioning with live progress feedback, adds network policy enforcement options, and replaces direct command invocations with file-based execution and detached processes. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Manager
participant Provider
participant Provisioner
participant Network
User->>CLI: setup [flags] [name]
CLI->>Manager: CreateWithOptions(ctx, spec, options)
Manager->>Manager: Phase 1: Create VM
Manager->>Provider: Create(ctx, spec)
Provider->>Provider: Check existence
Provider->>Provider: Write config to temp
Provider->>Provider: runCmdDetached(create)
Manager->>CLI: Progress("Creating VM...")
Manager->>Manager: Phase 2: Provision
Manager->>Provisioner: ProvisionSteps(name, version)
Provisioner->>Manager: Return []ProvisionStep
loop Each provisioning step
Manager->>Manager: Report step progress
Manager->>Provisioner: step.Run(ctx)
Provisioner->>Provider: Exec(...)
end
Manager->>CLI: Progress("Provisioning complete")
Manager->>Manager: Phase 3: Apply Network Policy
Manager->>Network: ApplyNetworkProfile(ctx, name)
Network->>Provider: Exec(iptables rules)
Manager->>CLI: Progress("Network locked")
Manager->>Manager: Phase 4: Snapshot
Manager->>Provider: SnapshotClean(...)
Manager->>CLI: Progress("Snapshot saved")
Manager-->>CLI: Return SandboxInfo
CLI->>User: ✓ Setup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/sandbox/create.go (1)
37-49:⚠️ Potential issue | 🟠 MajorAdd provider.Delete() cleanup when Create() fails, matching Start() error handling.
The gap you identified is real. When
m.provider.Create()fails (line 53), no cleanup occurs — the method just returns the error. In contrast,m.provider.Start()failure (line 61) properly callsprovider.Delete()to clean up the VM. This asymmetry means a failed Create() leaves the Lima VM directory on disk. When retrying with an errored sandbox:
m.remove()deletes only the registry entry- Old Lima VM state persists (~/.lima//...)
- Next
provider.Create()fails immediately with "VM already exists"The commit message mentions "Clean up stale VM directories on create failure," but
provider.Create()has no error path that triggers this cleanup. Add the same Delete() call after Create failure that exists for Start failure to match the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sandbox/create.go` around lines 37 - 49, Create() currently returns immediately on m.provider.Create(ctx, spec) errors and leaves the VM directory behind; update the error path to mirror the Start() handling by calling m.provider.Delete(ctx, spec.Name) (and ignore or log its error) before returning the original Create error, and ensure any registry cleanup already performed (m.put / m.remove) and mutex unlocking behavior remains unchanged; reference m.provider.Create, m.provider.Delete, m.remove, and the Create error return path so you add the Delete() call after the failed m.provider.Create() but before returning the error.internal/vm/lima/provider.go (1)
47-88:⚠️ Potential issue | 🟡 MinorDoc comment claims cleanup that the implementation does not perform.
The new comment at line 49 states "On failure, any partially created directory is cleaned up", but
Createdoes not attempt to remove any Lima instance directory onlimactl createfailure — it only defers removal of the temp YAML file (line 72). The PR description explicitly mentions "Clean up stale VM directories on create failure" as a fix; either the cleanup belongs here (e.g.,os.RemoveAll(filepath.Join(p.limaDir, spec.Name))on error) and is missing, or the doc comment should be removed to match behavior.🔧 Proposed fix (if cleanup is intended here)
_, err = p.runCmdStreaming(ctx, "create", "--tty=false", "--name="+spec.Name, tmpFile.Name()) if err != nil { + // Best-effort cleanup of a partially created instance directory so + // a retry is not blocked by an "instance already exists" error. + _ = os.RemoveAll(filepath.Join(p.limaDir, spec.Name)) return fmt.Errorf("create VM %s: %w", spec.Name, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/provider.go` around lines 47 - 88, The docstring claims partially created directories are cleaned up but Create only removes the temp YAML; when limactl create (p.runCmdStreaming) fails you must remove the Lima instance directory to match the comment: after the failing call to p.runCmdStreaming in Create, call os.RemoveAll(filepath.Join(p.limaDir, spec.Name)) (handle and wrap any error appropriately) before returning the wrapped error from Create; update/Create the error return to still include the original error from runCmdStreaming and ensure the temp file defer remains..github/workflows/ci.yml (1)
1-67:⚠️ Potential issue | 🟠 MajorSet explicit
permissionsforGITHUB_TOKEN(least privilege).Neither the workflow nor any job restricts
GITHUB_TOKENpermissions, so it inherits the repository default (which may be permissive). Set a restrictive top-level default and grantcontents: writeonly to thereleasejob that needs it to publish GitHub Releases via GoReleaser.🛡️ Proposed fix
on: push: branches: [main] tags: ["v*"] pull_request: branches: [main] +permissions: + contents: read + jobs: test: @@ release: needs: [test, lint] if: startsWith(github.ref, 'refs/tags/v') runs-on: macos-latest timeout-minutes: 20 + permissions: + contents: write steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1 - 67, Add a top-level restrictive permissions block for GITHUB_TOKEN (e.g., default to contents: read) and then grant contents: write specifically on the release job that runs goreleaser; update the workflow by adding a top-level "permissions" mapping (restrictive), and add a "permissions: contents: write" section to the "release" job (the job containing the goreleaser/goreleaser-action@v6 step) so only that job can write releases with GITHUB_TOKEN.
🧹 Nitpick comments (17)
CONTRIBUTING.md (1)
73-73: Missing language on fenced code block (MD040).Add a language hint (e.g.,
```text) for the package-structure block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 73, The fenced code block labeled "package-structure" in CONTRIBUTING.md triggers MD040 because it lacks a language hint; edit that fenced block (the triple-backtick block surrounding the package-structure example) and add an appropriate language token (for example ```text or ```bash) immediately after the opening ``` so the block reads e.g. ```text to satisfy the linter and preserve the example formatting.internal/vm/lima/config_test.go (1)
225-225: Uncheckedos.WriteFileerror (pre-existing pattern).Not introduced by this PR, but worth noting: if
WriteFilesilently fails,Create()will error with a confusing "exec" message instead of the real cause. Low priority since the test would still fail, just less clearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/config_test.go` at line 225, The test currently calls os.WriteFile(limactlPath, []byte(script), 0755) without checking the returned error; change this to capture and assert the error (e.g., err := os.WriteFile(...); if err != nil { t.Fatalf("writing limactl shim: %v", err) } or use your test helper/assertion) so failures surface with the real cause instead of an opaque exec error from Create(); update the test in internal/vm/lima/config_test.go where limactlPath is written.README.md (2)
195-207: Add language hints to fenced code blocks (MD040).Lines 195 and 215 open fences without a language. Use
```textto silence markdownlint and improve rendering.Also applies to: 215-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 195 - 207, The README has fenced code blocks that open with plain ``` which triggers MD040; update those code fences that list directories (the two directory-listing blocks shown in the diff) to include a language hint by replacing the opening triple-backticks with ```text for each block so markdownlint is satisfied and rendering is improved; specifically change both occurrences of the directory-listing fences (the block starting at the first ``` and the second similar block later in the file) to use ```text.
105-105: Capitalization nit:Lock/Unlockread as code but aren't.Minor readability: within the prose "
Lockblocks all outbound…" feels like method names. If you meant theairlock lock/airlock unlockcommands, inline-code them as such for consistency with the rest of the doc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 105, The text currently uses `Lock`/`Unlock` which look like code symbols but refer to commands; update the prose to reference the actual CLI commands by replacing `Lock`/`Unlock` with `airlock lock` and `airlock unlock` (inline code style) so the sentence reads e.g. "`airlock lock` blocks all outbound connections except DNS" and "`airlock unlock` restores normal access", ensuring consistency with other command mentions in the README..goreleaser-release.tpl (1)
15-17: Install snippet in release notes pins tomain, not the released tag.Users reading the notes for, say,
v0.3.0will pipeinstall.shfrommain, which could have drifted from the release they're looking at. Sinceinstall.shrespectsINSTALL_VERSIONand also resolves "latest" by default, this is usually fine — but consider either pinning the template URL to{{ .Tag }}or noting thatinstall.shalways fetches the latest release regardless of the URL used.Proposed fix (pin to tag)
-curl -fsSL https://raw.githubusercontent.com/muneebs/airlock/main/install.sh | bash +curl -fsSL https://raw.githubusercontent.com/muneebs/airlock/{{ .Tag }}/install.sh | bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.goreleaser-release.tpl around lines 15 - 17, The release notes currently show a curl command that fetches install.sh from the main branch; update the snippet so the URL is pinned to the release tag by replacing the branch segment with the template tag variable (use {{ .Tag }}) so the command pulls the install.sh corresponding to the release being viewed; alternatively, if you prefer to keep behavior that always installs the latest, add a short note explaining that install.sh respects INSTALL_VERSION and resolves "latest" by default (reference the curl line and the install.sh/INSTALL_VERSION behavior when making the change).install.sh (3)
9-9: Remove unusedBINARYvariable.
BINARY="airlock"is declared but never referenced; the binary name is hardcoded in latertar/mv/chmodinvocations. Either use it consistently or drop it (flagged by shellcheck SC2034).Proposed fix
REPO="muneebs/airlock" INSTALL_DIR="${HOME}/.local/bin" -BINARY="airlock"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` at line 9, The BINARY="airlock" variable is unused (shellcheck SC2034); remove the BINARY declaration or replace hardcoded "airlock" occurrences with the variable to use it consistently—update the tar, mv, and chmod invocations that reference "airlock" to instead use $BINARY (or delete the BINARY line if you prefer hardcoded names) and ensure any other references match the chosen approach (function/script surrounding install.sh will locate BINARY).
61-68: Use a unique temp path and trap cleanup.
/tmp/airlock.tar.gzand/tmp/airlockare predictable paths shared across users/runs. On a multi-user system this is a minor TOCTOU/symlink concern, and a partial run can leave stale files that a subsequenttar -xzf ... airlockwould reuse. Considermktemp -dplus atrapfor cleanup on any exit.Proposed fix
+TMPDIR="$(mktemp -d)" +trap 'rm -rf "${TMPDIR}"' EXIT + info "Downloading airlock v${VERSION} for macOS ${GOARCH}..." -if ! curl -fsSL "${DOWNLOAD_URL}" -o /tmp/airlock.tar.gz; then +if ! curl -fsSL "${DOWNLOAD_URL}" -o "${TMPDIR}/airlock.tar.gz"; then error "Download failed from ${DOWNLOAD_URL}. Check the version and try again." fi -tar -xzf /tmp/airlock.tar.gz -C /tmp/ airlock -mv /tmp/airlock "${INSTALL_DIR}/airlock" +tar -xzf "${TMPDIR}/airlock.tar.gz" -C "${TMPDIR}/" airlock +mv "${TMPDIR}/airlock" "${INSTALL_DIR}/airlock" chmod +x "${INSTALL_DIR}/airlock" -rm -f /tmp/airlock.tar.gz🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 61 - 68, Replace the hardcoded /tmp paths with a unique temporary directory created via mktemp -d and ensure cleanup on exit by adding a trap; specifically, when downloading and extracting the archive referenced by DOWNLOAD_URL, create a tempdir (e.g. TMPDIR=$(mktemp -d)), download into "$TMPDIR/airlock.tar.gz", extract the "airlock" member into that tempdir, move "$TMPDIR/airlock" to the target INSTALL_DIR/airlock, and register trap 'rm -rf "$TMPDIR"' for EXIT so partial runs cannot leave stale files or enable TOCTOU/symlink attacks.
23-30: Order OS check before arch check for clearer error on non-macOS.On Linux (or other non-Darwin systems),
uname -mreturning an unexpected value would report "Unsupported architecture" before the clearer "macOS required" message. Swapping the order yields a more informative error for the common case of a user trying to install on Linux.Proposed fix
+[[ "$(uname)" == "Darwin" ]] || error "macOS required. airlock uses Apple Virtualization framework via Lima." + ARCH="$(uname -m)" case "${ARCH}" in x86_64) GOARCH="x86_64" ;; arm64) GOARCH="arm64" ;; *) error "Unsupported architecture: ${ARCH}" ;; esac - -[[ "$(uname)" == "Darwin" ]] || error "macOS required. airlock uses Apple Virtualization framework via Lima."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 23 - 30, The script checks architecture (ARCH="$(uname -m)" and the case that sets GOARCH) before verifying the OS, which can yield "Unsupported architecture" on non-macOS systems; move the macOS check ([[ "$(uname)" == "Darwin" ]] || error "macOS required...") to run before assigning ARCH and the case block so the script exits with the clearer "macOS required" message for non-Darwin systems; update any dependent logic that assumes ARCH/GOARCH are present only after the OS check.internal/sandbox/create.go (1)
43-43: Silently ignoringm.removeerror — at least log it.
_ = m.remove(spec.Name)swallows a persistence error. Since the subsequentm.put(info)will re-save the map anyway, functionally this is OK, but a silent failure to persist makes post-mortem debugging harder. A log at warn level (or propagating the error) would help when this path is hit in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sandbox/create.go` at line 43, The call that currently swallows errors — _ = m.remove(spec.Name) — should not ignore failures silently; update the create logic to capture the error returned by m.remove(spec.Name) and either log it at warn level (including spec.Name and the error) or return/propagate the error upstream; ensure the adjacent m.put(info) behavior is preserved and include context in the log so callers can diagnose persistence failures (reference m.remove, spec.Name, and m.put(info) to locate the change)..github/workflows/ci.yml (1)
60-65: Consider pinning GoReleaser version for reproducible releases.
version: latestmeans release artifacts can change whenever GoReleaser ships a new major/minor. Pinning a specific version (e.g.,version: "~> v2"or an exact tag) yields reproducible release builds and avoids surprise breakages on tag pushes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 60 - 65, The workflow step "Run goreleaser" currently pins the action to goreleaser/goreleaser-action@v6 but uses version: latest which is non-reproducible; change the step so the "version" input is pinned to a specific GoReleaser release tag (or a constrained range) instead of "latest" — edit the step that references "Run goreleaser" and replace version: latest with a concrete tag (for example a specific vX.Y.Z) or a constrained spec to ensure reproducible releases.cmd/airlock/cli/tui/styles.go (1)
26-48:RunPhases/RunSpinneruse\rregardless of TTY.The success path writes
"\r ✓ ..."to overwrite the preceding" label ... "line. In non-terminal destinations (pipes, CI logs, log aggregators),\risn't interpreted as an overwrite and produces noisy output (and can confuse downstream log parsers). Consider using the existingisTerminal()helper to fall back to a plain newline in non-TTY mode:🔧 Proposed fix (illustrative)
- fmt.Printf("\r %s %s\n", CheckMark.String(), p.DoneLabel) + if isTerminal() { + fmt.Printf("\r %s %s\n", CheckMark.String(), p.DoneLabel) + } else { + fmt.Printf(" %s %s\n", CheckMark.String(), p.DoneLabel) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/tui/styles.go` around lines 26 - 48, RunPhases and RunSpinner currently unconditionally print a carriage return ("\r") to overwrite the previous progress line, which breaks when stdout is not a TTY; update both functions to call the existing isTerminal() helper and, on terminals, keep the current overwrite behavior (use "\r" + the success line) but when not a terminal print a plain newline prefixed success line (e.g. "\n <check> <doneLabel>\n" or "\n <check> <doneLabel>\n") instead of using "\r". Modify the success print paths that use "\r %s %s\n" to branch on isTerminal() and use the appropriate newline or carriage-return form; leave the error paths (which already print a leading "\n") unchanged and keep references to RunPhases, RunSpinner, CheckMark and CrossMark so you update the right locations.internal/vm/lima/provider.go (2)
68-80: Good temp-file isolation; minor nits.Writing config to a temp file instead of the Lima instance dir is a solid fix for the "instance already exists" issue. Two optional tweaks:
defer os.Remove(tmpFile.Name())ignores the error (errcheck). Preferdefer func() { _ = os.Remove(tmpFile.Name()) }()to make intent explicit and silence the linter.- Consider
defertheClose()right afterCreateTemp(guarded so double-close is a no-op) so an earlyWriteStringpanic doesn't leak the fd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/provider.go` around lines 68 - 80, Replace the plain defer os.Remove(...) with an explicit error-ignoring wrapper and defer closing the file immediately after os.CreateTemp to avoid fd leaks: after creating tmpFile (os.CreateTemp) add defer func() { _ = tmpFile.Close() }() and change the removal to defer func() { _ = os.Remove(tmpFile.Name()) }(); then remove or stop relying on later explicit tmpFile.Close() calls (or make them no-ops) so tmpFile.WriteString and subsequent error paths won’t leak the descriptor; reference tmpFile, tmpFile.WriteString and tmpFile.Close when making these changes.
255-285:cleanLimactlErrorduplicatestui.CleanErrorparsing logic.The structured-log parsing here (handling
time=.../level=...lines and extractingmsg=) is essentially the same astui.CleanErrorincmd/airlock/cli/tui/styles.go. Sincemain.gofurther runstui.CleanErroron the wrapped error, parsing happens twice for the same stderr. Consider extracting a shared helper into a neutral package (e.g.,internal/vm/limastderror a plain util pkg) and calling it from both sites to avoid drift. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/provider.go` around lines 255 - 285, cleanLimactlError duplicates the structured-log parsing in tui.CleanError; extract the shared parsing logic into a neutral helper (e.g., package internal/vm/limastderr or internal/util) that exposes a single function (e.g., ParseLimaStderr or CleanLimaStderr) which implements the time=/level=/msg= extraction, then update cleanLimactlError to call that helper and update tui.CleanError to call the same helper so both sites reuse the identical parsing logic; ensure the helper returns the cleaned message slice or final string so callers (cleanLimactlError and tui.CleanError) can format the final error text consistently.Makefile (1)
22-23:fmttarget: simpler and safer withgofmt -l.The current recipe relies on the exit status of
grep -qcombined with&& ... || ..., which prints "Formatting OK." even in some pathological failure modes and always runs gofmt over.including vendored/generated directories if present later.gofmt -lis the idiomatic check:🔧 Proposed fix
fmt: - `@gofmt` -d . | grep -q . && echo "Formatting issues found. Run gofmt -w ." && exit 1 || echo "Formatting OK." + `@out`=$$(gofmt -l $$(go list -f '{{.Dir}}' ./...)); \ + if [ -n "$$out" ]; then echo "Formatting issues in:"; echo "$$out"; echo "Run: gofmt -w ."; exit 1; fi; \ + echo "Formatting OK."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 22 - 23, Update the Makefile fmt target to use the idiomatic "gofmt -l" check instead of piping gofmt -d through grep and the unreliable &&/|| chain: in the fmt target (named "fmt") call "gofmt -l" over the repository packages (e.g. via go list ./... filtered to exclude vendor or generated dirs) and use the grep -q exit status to fail with "Formatting issues found. Run gofmt -w ." or echo "Formatting OK." so the target reliably returns non‑zero when files need formatting and avoids scanning vendor/generated code.cmd/airlock/cli/tui/table.go (1)
15-19: Unusedwidthfield inTable.The
width intfield is never read or set anywhere — golangci-lint flags it as unused. Either wire it intoRender(e.g., as an overall max width / padding) or drop it.🔧 Proposed fix
type Table struct { columns []TableColumn rows [][]string - width int }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/tui/table.go` around lines 15 - 19, The Table struct contains an unused field width which triggers golangci-lint; remove the width int field from the Table type declaration and any associated initialization or tests that reference Table.width (search for "width" usage tied to Table), or if you prefer to keep it, wire it into the rendering logic by updating Render/RenderRow functions to enforce overall max width/padding using Table.width; the simplest fix: delete the width field from type Table and remove/adjust any code that sets or expects it so the linter no longer reports an unused field.internal/vm/lima/config.go (1)
70-74:LimaNinePis unused dead code and should be removed or wired intoLimaConfig.The
LimaNinePstruct is defined at lines 70-74 but is never referenced inLimaConfignor instantiated inGenerateConfig. The currentMountTypeis hardcoded to"virtiofs", providing no pathway to use this struct. Either remove it, or add a field toLimaConfig(e.g.,NineP *LimaNineP) to be populated whenMountType == "9p".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/config.go` around lines 70 - 74, LimaNineP is dead code; either remove it or wire it into LimaConfig and GenerateConfig so 9p mounts are supported: add a field like NineP *LimaNineP to the LimaConfig struct, update GenerateConfig to populate LimaConfig.NineP when MountType == "9p" (or when a 9p option is detected), and ensure any place that currently hardcodes MountType = "virtiofs" allows selecting "9p" so the new NineP settings are used; alternatively, if 9p support is not desired, delete the LimaNineP type and any unused references.cmd/airlock/cli/cli.go (1)
802-807: Delete unusednetworkStatehelper function.The
networkStatefunction (lines 802–807) has no callers in the codebase. After the status command was refactored to render lock state inline viatui.LockColor(...)at lines 500–505, this helper became obsolete.Cleanup
-func networkState(locked bool) string { - if locked { - return "locked (outbound blocked)" - } - return "unlocked (outbound allowed)" -}🤖 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 802 - 807, Remove the now-unused helper function networkState since the status command renders lock state inline via tui.LockColor; locate the function declaration named networkState and delete it along with its two return branches ("locked (outbound blocked)" / "unlocked (outbound allowed)"), and run a quick build/search to ensure there are no remaining references to networkState before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.goreleaser.yml:
- Around line 65-70: The release configuration uses the invalid key header_extra
under the release.github block; replace the header_extra key with header while
keeping its multi-line string value so the installation snippet is prepended to
GitHub release notes (update the key name in the release -> github section,
changing header_extra to header).
- Around line 20-21: The manifest uses the deprecated archives.format key;
replace the singular "format" under the archives entry with the plural "formats"
and provide a list value (e.g., tar.gz) so GoReleaser v2.6+ accepts it—update
the archives entry that currently uses "format" to instead use "formats" (an
array) wherever the archives.format field appears.
In `@cmd/airlock/cli/cli.go`:
- Around line 283-296: The spinner writes human text to stdout which corrupts
JSON; modify the flow in the command handler around tui.RunSpinner and
jsonOutput so when jsonOutput is true you do not call tui.RunSpinner but instead
call deps.Manager.Create(ctx, spec) directly (assigning to info and capturing
err) and then encode info to cmd.OutOrStdout(); alternatively route spinner
output to stderr by changing tui.RunSpinner to accept an io.Writer and use
cmd.ErrOrStderr for spinner text; update the call site (the block using
tui.RunSpinner, info, deps.Manager.Create, jsonOutput, and cmd.OutOrStdout()) so
only the JSON encoder writes to stdout when jsonOutput is true.
In `@cmd/airlock/cli/tui/styles.go`:
- Around line 142-144: The init() currently forces ANSI256 via
lipgloss.SetColorProfile(termenv.ANSI256) which emits escape codes even when
stdout is not a TTY; update init() to use termenv.EnvColorProfile() (or call
lipgloss.SetColorProfile(termenv.EnvColorProfile())) so color respects
NO_COLOR/CLICOLOR and TTY detection, or alternatively guard the call with the
existing isTerminal() helper so colors are only enabled for terminals; ensure
isTerminal() is used so it is no longer unused and styled output will not leak
into JSON/redirected output.
In `@CONTRIBUTING.md`:
- Line 7: Update the Go version string in CONTRIBUTING.md from "Go 1.23+" to
match go.mod by changing it to "Go 1.24" (or "Go 1.24.2" if you prefer exact
patch) so the documented minimum Go version aligns with the project's go.mod;
locate the literal "Go 1.23+" text and replace it with the corrected version.
In `@Makefile`:
- Around line 27-29: The install target currently expands an unset GOPATH to
"/bin" and always echoes $(HOME)/go/bin, so change the Makefile to compute a
safe GOPATH (e.g., GOPATH := $(shell go env GOPATH 2>/dev/null || echo
$(HOME)/go)) or compute a DESTDIR variable using $(shell go env GOPATH) with
fallback, then use that DESTDIR/bin/$(BINARY) as the copy target for the install
rule (replace the two cp attempts with a single cp to the computed destination)
and update the echo to print that actual destination; reference the install
target and variables BINARY and GOPATH when making these edits.
In `@README.md`:
- Around line 15-21: The README's "From source" instructions use "make install"
which installs into $GOPATH/bin per CONTRIBUTING while the curl one-liner places
the binary at ~/.local/bin/airlock, causing possible stale-binary confusion;
update the README to either (a) explicitly call out this difference and tell
users which path to prefer and how to add it to PATH (mention
"~/.local/bin/airlock" vs "$GOPATH/bin"), or (b) change the project installation
instructions to unify destinations (adjust the Makefile's install target or the
one-liner so both install to the same path); reference the "make install" target
and the curl one-liner in the README and include the exact install paths
"~/.local/bin/airlock" and "$GOPATH/bin" so users know where to look.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-67: Add a top-level restrictive permissions block for
GITHUB_TOKEN (e.g., default to contents: read) and then grant contents: write
specifically on the release job that runs goreleaser; update the workflow by
adding a top-level "permissions" mapping (restrictive), and add a "permissions:
contents: write" section to the "release" job (the job containing the
goreleaser/goreleaser-action@v6 step) so only that job can write releases with
GITHUB_TOKEN.
In `@internal/sandbox/create.go`:
- Around line 37-49: Create() currently returns immediately on
m.provider.Create(ctx, spec) errors and leaves the VM directory behind; update
the error path to mirror the Start() handling by calling m.provider.Delete(ctx,
spec.Name) (and ignore or log its error) before returning the original Create
error, and ensure any registry cleanup already performed (m.put / m.remove) and
mutex unlocking behavior remains unchanged; reference m.provider.Create,
m.provider.Delete, m.remove, and the Create error return path so you add the
Delete() call after the failed m.provider.Create() but before returning the
error.
In `@internal/vm/lima/provider.go`:
- Around line 47-88: The docstring claims partially created directories are
cleaned up but Create only removes the temp YAML; when limactl create
(p.runCmdStreaming) fails you must remove the Lima instance directory to match
the comment: after the failing call to p.runCmdStreaming in Create, call
os.RemoveAll(filepath.Join(p.limaDir, spec.Name)) (handle and wrap any error
appropriately) before returning the wrapped error from Create; update/Create the
error return to still include the original error from runCmdStreaming and ensure
the temp file defer remains.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 60-65: The workflow step "Run goreleaser" currently pins the
action to goreleaser/goreleaser-action@v6 but uses version: latest which is
non-reproducible; change the step so the "version" input is pinned to a specific
GoReleaser release tag (or a constrained range) instead of "latest" — edit the
step that references "Run goreleaser" and replace version: latest with a
concrete tag (for example a specific vX.Y.Z) or a constrained spec to ensure
reproducible releases.
In @.goreleaser-release.tpl:
- Around line 15-17: The release notes currently show a curl command that
fetches install.sh from the main branch; update the snippet so the URL is pinned
to the release tag by replacing the branch segment with the template tag
variable (use {{ .Tag }}) so the command pulls the install.sh corresponding to
the release being viewed; alternatively, if you prefer to keep behavior that
always installs the latest, add a short note explaining that install.sh respects
INSTALL_VERSION and resolves "latest" by default (reference the curl line and
the install.sh/INSTALL_VERSION behavior when making the change).
In `@cmd/airlock/cli/cli.go`:
- Around line 802-807: Remove the now-unused helper function networkState since
the status command renders lock state inline via tui.LockColor; locate the
function declaration named networkState and delete it along with its two return
branches ("locked (outbound blocked)" / "unlocked (outbound allowed)"), and run
a quick build/search to ensure there are no remaining references to networkState
before committing.
In `@cmd/airlock/cli/tui/styles.go`:
- Around line 26-48: RunPhases and RunSpinner currently unconditionally print a
carriage return ("\r") to overwrite the previous progress line, which breaks
when stdout is not a TTY; update both functions to call the existing
isTerminal() helper and, on terminals, keep the current overwrite behavior (use
"\r" + the success line) but when not a terminal print a plain newline prefixed
success line (e.g. "\n <check> <doneLabel>\n" or "\n <check> <doneLabel>\n")
instead of using "\r". Modify the success print paths that use "\r %s %s\n" to
branch on isTerminal() and use the appropriate newline or carriage-return form;
leave the error paths (which already print a leading "\n") unchanged and keep
references to RunPhases, RunSpinner, CheckMark and CrossMark so you update the
right locations.
In `@cmd/airlock/cli/tui/table.go`:
- Around line 15-19: The Table struct contains an unused field width which
triggers golangci-lint; remove the width int field from the Table type
declaration and any associated initialization or tests that reference
Table.width (search for "width" usage tied to Table), or if you prefer to keep
it, wire it into the rendering logic by updating Render/RenderRow functions to
enforce overall max width/padding using Table.width; the simplest fix: delete
the width field from type Table and remove/adjust any code that sets or expects
it so the linter no longer reports an unused field.
In `@CONTRIBUTING.md`:
- Line 73: The fenced code block labeled "package-structure" in CONTRIBUTING.md
triggers MD040 because it lacks a language hint; edit that fenced block (the
triple-backtick block surrounding the package-structure example) and add an
appropriate language token (for example ```text or ```bash) immediately after
the opening ``` so the block reads e.g. ```text to satisfy the linter and
preserve the example formatting.
In `@install.sh`:
- Line 9: The BINARY="airlock" variable is unused (shellcheck SC2034); remove
the BINARY declaration or replace hardcoded "airlock" occurrences with the
variable to use it consistently—update the tar, mv, and chmod invocations that
reference "airlock" to instead use $BINARY (or delete the BINARY line if you
prefer hardcoded names) and ensure any other references match the chosen
approach (function/script surrounding install.sh will locate BINARY).
- Around line 61-68: Replace the hardcoded /tmp paths with a unique temporary
directory created via mktemp -d and ensure cleanup on exit by adding a trap;
specifically, when downloading and extracting the archive referenced by
DOWNLOAD_URL, create a tempdir (e.g. TMPDIR=$(mktemp -d)), download into
"$TMPDIR/airlock.tar.gz", extract the "airlock" member into that tempdir, move
"$TMPDIR/airlock" to the target INSTALL_DIR/airlock, and register trap 'rm -rf
"$TMPDIR"' for EXIT so partial runs cannot leave stale files or enable
TOCTOU/symlink attacks.
- Around line 23-30: The script checks architecture (ARCH="$(uname -m)" and the
case that sets GOARCH) before verifying the OS, which can yield "Unsupported
architecture" on non-macOS systems; move the macOS check ([[ "$(uname)" ==
"Darwin" ]] || error "macOS required...") to run before assigning ARCH and the
case block so the script exits with the clearer "macOS required" message for
non-Darwin systems; update any dependent logic that assumes ARCH/GOARCH are
present only after the OS check.
In `@internal/sandbox/create.go`:
- Line 43: The call that currently swallows errors — _ = m.remove(spec.Name) —
should not ignore failures silently; update the create logic to capture the
error returned by m.remove(spec.Name) and either log it at warn level (including
spec.Name and the error) or return/propagate the error upstream; ensure the
adjacent m.put(info) behavior is preserved and include context in the log so
callers can diagnose persistence failures (reference m.remove, spec.Name, and
m.put(info) to locate the change).
In `@internal/vm/lima/config_test.go`:
- Line 225: The test currently calls os.WriteFile(limactlPath, []byte(script),
0755) without checking the returned error; change this to capture and assert the
error (e.g., err := os.WriteFile(...); if err != nil { t.Fatalf("writing limactl
shim: %v", err) } or use your test helper/assertion) so failures surface with
the real cause instead of an opaque exec error from Create(); update the test in
internal/vm/lima/config_test.go where limactlPath is written.
In `@internal/vm/lima/config.go`:
- Around line 70-74: LimaNineP is dead code; either remove it or wire it into
LimaConfig and GenerateConfig so 9p mounts are supported: add a field like NineP
*LimaNineP to the LimaConfig struct, update GenerateConfig to populate
LimaConfig.NineP when MountType == "9p" (or when a 9p option is detected), and
ensure any place that currently hardcodes MountType = "virtiofs" allows
selecting "9p" so the new NineP settings are used; alternatively, if 9p support
is not desired, delete the LimaNineP type and any unused references.
In `@internal/vm/lima/provider.go`:
- Around line 68-80: Replace the plain defer os.Remove(...) with an explicit
error-ignoring wrapper and defer closing the file immediately after
os.CreateTemp to avoid fd leaks: after creating tmpFile (os.CreateTemp) add
defer func() { _ = tmpFile.Close() }() and change the removal to defer func() {
_ = os.Remove(tmpFile.Name()) }(); then remove or stop relying on later explicit
tmpFile.Close() calls (or make them no-ops) so tmpFile.WriteString and
subsequent error paths won’t leak the descriptor; reference tmpFile,
tmpFile.WriteString and tmpFile.Close when making these changes.
- Around line 255-285: cleanLimactlError duplicates the structured-log parsing
in tui.CleanError; extract the shared parsing logic into a neutral helper (e.g.,
package internal/vm/limastderr or internal/util) that exposes a single function
(e.g., ParseLimaStderr or CleanLimaStderr) which implements the
time=/level=/msg= extraction, then update cleanLimactlError to call that helper
and update tui.CleanError to call the same helper so both sites reuse the
identical parsing logic; ensure the helper returns the cleaned message slice or
final string so callers (cleanLimactlError and tui.CleanError) can format the
final error text consistently.
In `@Makefile`:
- Around line 22-23: Update the Makefile fmt target to use the idiomatic "gofmt
-l" check instead of piping gofmt -d through grep and the unreliable &&/||
chain: in the fmt target (named "fmt") call "gofmt -l" over the repository
packages (e.g. via go list ./... filtered to exclude vendor or generated dirs)
and use the grep -q exit status to fail with "Formatting issues found. Run gofmt
-w ." or echo "Formatting OK." so the target reliably returns non‑zero when
files need formatting and avoids scanning vendor/generated code.
In `@README.md`:
- Around line 195-207: The README has fenced code blocks that open with plain
``` which triggers MD040; update those code fences that list directories (the
two directory-listing blocks shown in the diff) to include a language hint by
replacing the opening triple-backticks with ```text for each block so
markdownlint is satisfied and rendering is improved; specifically change both
occurrences of the directory-listing fences (the block starting at the first ```
and the second similar block later in the file) to use ```text.
- Line 105: The text currently uses `Lock`/`Unlock` which look like code symbols
but refer to commands; update the prose to reference the actual CLI commands by
replacing `Lock`/`Unlock` with `airlock lock` and `airlock unlock` (inline code
style) so the sentence reads e.g. "`airlock lock` blocks all outbound
connections except DNS" and "`airlock unlock` restores normal access", ensuring
consistency with other command mentions in the README.
🪄 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: 61c01e9e-9012-4c3e-9aa0-99f1195078e1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignore.goreleaser-release.tpl.goreleaser.ymlCONTRIBUTING.mdMakefileREADME.mdbin/airlockcmd/airlock/cli/cli.gocmd/airlock/cli/tui/styles.gocmd/airlock/cli/tui/table.gogo.modinstall.shinternal/sandbox/create.gointernal/vm/lima/config.gointernal/vm/lima/config_test.gointernal/vm/lima/provider.gomain.go
💤 Files with no reviewable changes (1)
- bin/airlock
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Add top-level permissions: contents: read for test/lint jobs. Add job-level permissions: contents: write for release job (goreleaser needs this to create releases).
- archives.format -> archives.formats (plural array) for goreleaser v2.6+ - release.header_extra -> release.header (correct goreleaser v2 key) - Only set lipgloss color profile when stdout is a TTY; use EnvColorProfile() instead of hardcoded ANSI256 - Skip RunSpinner when --json is set so spinner text never hits stdout
Review feedback: - CI release job: remove duplicate `permissions:` key - install.sh: use mktemp -d scratch dir with trap cleanup; verify SHA256 against checksums.txt before extract; install -m 0755 - Manager.Create: delete orphaned VM (best-effort) when replacing a recoverable sandbox; log rollback delete failures to stderr - Lima provider: drop unused LimaNineP struct - runCmdStreaming: tee stderr (terminal + buffer) so failure messages flow through cleanLimactlError - Makefile fmt: use if/exit 1 so gofmt crashes aren't masked by `&& ... || ...` chain - Remove unreferenced .goreleaser-release.tpl Setup hang: - limactl start on a fresh Ubuntu cloud image takes 5-7 minutes (cloud-init, containerd, ssh setup). Buffered stderr meant the user saw no output and assumed the tool was stuck. Stream stderr via runCmdStreaming so lima's progress is visible. Stuck-state recovery: - If setup is interrupted (ctrl-c) the sandbox state could get stuck at "creating", blocking all retries with ErrAlreadyExists. Extend the recovery path to accept StateCreating as well as StateErrored. - Wire signal.NotifyContext(SIGINT/SIGTERM) through cobra's ExecuteContext so interrupts cancel in-flight exec.CommandContext calls and trigger Manager.Create rollback to StateErrored. Tests: - Unit: TestCreateReplacesErroredSandbox, TestCreateReplacesStuckCreatingSandbox, TestCreateAlreadyExistsNotReplacedWhenRunning - Integration: TestProviderCreateRejectsExisting covers the provider-level pre-exist check AI use disclosure: AI-assisted (Claude). Generated and human-reviewed.
Setup hung indefinitely on `applying network policy`. Root causes: 1. `Lock()` applied a ruleset without ESTABLISHED, which dropped the VM sshd's reply packets on the OUTPUT chain — killing the lima ssh connection that was applying the rule. Every subsequent limactl shell call then hung. Fix: Lock() now preserves ESTABLISHED so in-flight ssh sessions keep working while new outbound is blocked. 2. Network policy was applied before provisioning installed iptables. Fix: add CreateOptions.SkipNetworkPolicy and Manager.ApplyNetworkProfile; setup now creates the VM, provisions it, then applies the policy. 3. limactl calls had no timeout, so any future ssh/xtables stall would freeze setup forever. Fix: 60s per-call timeout with a clear error. 4. cmd.Stderr/Stdout on non-*os.File writers caused Go to spawn pipe copier goroutines that ssh ControlMaster daemons kept alive past cmd.Wait. Fix: redirect to tempfiles in both lima and network packages. Also brands setup output with per-phase airlock-styled spinner phases (CreateWithProgress reports internal sub-stages) and streams large snapshot disk copies instead of os.ReadFile-ing multi-GB sparse files.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
internal/vm/lima/config.go (1)
64-68:VMMount.Inotifyis now silently ignored.With
mountInotifyremoved from the YAML schema, theInotifyfield onapi.VMMount(seeinternal/api/vm.goline 69) is accepted by the API but has no effect. Consider either removing the field from the spec or documenting that it is currently a no-op to avoid confusing callers who set it expecting inotify support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/config.go` around lines 64 - 68, The VMMount.Inotify flag is accepted by the API but ignored because LimaMount (type LimaMount) no longer supports mountInotify; either remove the Inotify field from the API spec or mark it explicitly as a no-op. To fix: update the api.VMMount definition (internal/api/vm.go) to remove the Inotify field if you want to stop accepting it, or retain it but add a clear comment and API doc note stating it is currently ignored and has no effect, and ensure any validation or conversion code that maps api.VMMount -> LimaMount (the code using LimaMount) does not silently drop it without notice.internal/api/sandbox.go (2)
35-51: DocumentProgressFnconcurrency contract.Callers implementing
ProgressFnwill likely touch shared UI state. Please clarify in the doc comment whether the callback may be invoked concurrently from multiple goroutines, and whether it must be non-blocking (since a slow callback could stall VM create). This avoids subtle UI races / deadlocks downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/sandbox.go` around lines 35 - 51, Update the doc comment for ProgressFn (and reference CreateOptions.Progress) to state the concurrency and blocking expectations: explicitly declare whether ProgressFn may be invoked concurrently from multiple goroutines or only from a single goroutine, and state that implementations must not block (or must be fast) and should offload long work to a goroutine/channel to avoid stalling VM creation; mention thread-safety requirements for callers touching shared UI state.
55-67: Interface surface is growing redundant.
Create,CreateWithProgress, andCreateWithOptionsform a strict superset chain —CreateWithOptionscan express everything the other two do. Carrying three methods on the interface forces every implementation (and every test fake) to implement all three, and callers have to know which to pick. Consider collapsing to a singleCreate(ctx, spec, opts)(withoptsbeing the zero value for the simple case), or at minimum keep onlyCreate+CreateWithOptionsand make the others thin wrappers at the concrete-type level rather than on the interface.🤖 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 - 67, The SandboxManager interface exposes three overlapping creation methods (Create, CreateWithProgress, CreateWithOptions); collapse the interface to a single CreateWithOptions(ctx context.Context, spec SandboxSpec, opts CreateOptions) (SandboxInfo, error) (or similarly named Create with opts) and remove Create and CreateWithProgress from the interface surface; keep thin wrapper helper methods (Create and CreateWithProgress) on the concrete implementation type that call the new CreateWithOptions with zero-value CreateOptions or with a ProgressFn wired into opts so callers keep convenience overloads while implementations and test fakes only need to implement the single CreateWithOptions entry point (update types that reference ProgressFn and CreateOptions accordingly and adjust all concrete types, fakes, and tests to implement the single method).internal/api/vm.go (2)
28-33:ProvisionVMandProvisionStepsoverlap.
ProvisionVMnow exists only as a convenience over iteratingProvisionSteps. Keeping both on the interface forces every implementer to keep them in sync. Consider droppingProvisionVMfrom the interface and providing a package-level helperRunAllSteps(ctx, p, name, nodeVersion)that iteratesProvisionSteps, so there's one source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/vm.go` around lines 28 - 33, Remove the duplicated ProvisionVM method from the Provisioner interface and add a package-level helper RunAllSteps(ctx, p Provisioner, name string, nodeVersion int) error that becomes the single implementation point: call p.ProvisionSteps(name, nodeVersion) to get []ProvisionStep, iterate the slice and execute each step (invoking whatever execution method the ProvisionStep type exposes), short-circuiting and returning the first non-nil error; update callers to use RunAllSteps instead of ProvisionVM so implementers only need to implement ProvisionSteps (and SnapshotClean/HasCleanSnapshot).
15-19: Returning""for not-found conflates states.Empty string also naturally arises from "status unknown / not yet reported" in many provider APIs. Prefer a sentinel (
StatusUnknown) or(string, bool, error)so callers can't accidentally treat "doesn't exist" the same as "provider returned empty". Lower priority since the doc comment is clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/vm.go` around lines 15 - 19, The Status method should not return an empty string for “not found” because that conflates with legitimate empty/unknown provider statuses; change the API to make absence explicit by either (preferred) introducing a sentinel constant StatusUnknown (string) and return that instead of "" or by changing the signature to Status(ctx context.Context, name string) (string, bool, error) where the bool indicates existence; update the interface declaration for Status, all implementations of the VM interface (methods named Status in providers), and all call sites to handle the new sentinel or the existence bool, and update the doc comment to document the new return contract..github/workflows/ci.yml (1)
65-72: Consider pinning goreleaser version.
version: latestmeans a new goreleaser major with breaking changes (v2→v3, etc.) will silently break tagged releases. Pinning to a specific minor (e.g.version: "~> v2") keeps updates controlled and makes release runs reproducible.- version: latest + version: "~> v2"What is the latest stable major version of goreleaser-action and goreleaser CLI in 2026?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 65 - 72, The workflow uses the goreleaser action with version: latest which risks breaking changes; update the action block that defines the goreleaser step (the step named "Run goreleaser" and the uses: goreleaser/goreleaser-action@v6) to pin a stable major/minor instead of latest (for example change the version input from "latest" to a pinned constraint like "~> v2" or an explicit minor tag) and ensure the args/env lines remain unchanged so release runs are reproducible.internal/vm/lima/snapshot.go (1)
30-37: Consider removing partially written destination on copy failure.If
io.Copyfails mid-write, the destination file is left on disk (truncated/partial). For snapshot use this is typically discarded by the caller viaos.RemoveAll(cleanDir)on a subsequent attempt, but a partialout.Close()error also leaves the file behind. Minor — consideros.Remove(dst)on error for cleanliness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/snapshot.go` around lines 30 - 37, If io.Copy or out.Close fails while writing the snapshot (see the io.Copy(..., in) error handling and the subsequent out.Close() call that currently returns the error), remove the partially written destination file to avoid leaving truncated files behind: on any error path from io.Copy or from out.Close(), call os.Remove(dst) before returning the error, then return the original error; leave the successful path to continue to os.Chmod(dst, perm.Perm()). This touches the error handling around io.Copy, out.Close and the final os.Chmod call for dst.internal/vm/lima/provider.go (1)
302-332: Edge case: stderr withouttime=/level=returns only"limactl <action>: <line>: <line>".
cleanLimactlErrorparses structured log lines but, for free-form stderr lines (notime=/level=prefix, and nomsg=), it appends the raw line(s) joined by": ". That's fine for single-line errors but can produce awkward messages likelimactl create: Error: foo: details: stack tracefor multi-line stderr. Not a bug, just worth being aware of for UX polish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/provider.go` around lines 302 - 332, cleanLimactlError currently concatenates multiple free-form stderr lines into "limactl <action>: <line>: <line>" which reads poorly; update cleanLimactlError so that when adding raw non-prefixed lines (the branch handling line != ""), you only capture the first non-empty line (or join into a single-line summary and append an ellipsis/truncated marker if there are additional lines) instead of appending every line to parts; modify the logic in cleanLimactlError to collect at most one representative free-form line (and indicate truncation) before building the final fmt.Sprintf output.internal/sandbox/create.go (1)
62-79: Narrow race window betweenUnlockand subsequentLockduring replacement.Between line 73 (
m.mu.Unlock()) and line 78 (m.mu.Lock()), the sandbox entry has already been removed from the in-memory map. A concurrentCreateWithOptionscall for the same name would not see any "already exists" entry and could race forward while this call is still deleting the prior VM viam.provider.Delete. For a single-user CLI this is effectively unreachable, but ifManagerever gets used from a long-running daemon/server it would be observable. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sandbox/create.go` around lines 62 - 79, The race occurs because we unlock m.mu after removing the sandbox from m.sandboxes and before calling m.provider.Delete, allowing a concurrent CreateWithOptions to proceed; fix by installing a tombstone placeholder under spec.Name (e.g., set m.sandboxes[spec.Name] = api.SandboxInfo{State: api.StateDeleting} or a dedicated "deleting" marker) while holding m.mu, then unlock and call m.provider.Delete, and finally acquire m.mu to replace or remove the tombstone and continue; update references around m.remove, existing.State, spec.Name and m.provider.Delete to use this tombstone approach so concurrent creators see the entry and will fail or wait instead of racing.internal/network/network.go (1)
222-251: Consider also flagging context cancellation distinctly from timeouts.The timeout-specific error at Line 248–249 only triggers when
callCtx.Err() == context.DeadlineExceeded. If the parentctxis canceled (e.g., user SIGINT),callCtx.Err()returnscontext.Canceled, and the caller gets a genericlimactl exec in ... : exit ...wrap without any hint that cancellation occurred. Minor UX polish — consider branching onerrors.Is(ctx.Err(), context.Canceled)too so the CLI can show "interrupted" rather than a stderr-noise error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/network.go` around lines 222 - 251, The runLimactlFiles function currently only special-cases timeouts; update the error handling after c.Run() to also detect context cancellation and return a distinct "interrupted" error: after reading stdout/stderr, if runErr != nil then first check if errors.Is(callCtx.Err(), context.DeadlineExceeded) to return the existing timeout error, else if errors.Is(callCtx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.Canceled) return an error that clearly indicates the call was interrupted/canceled (wrap runErr for context), and otherwise return runErr; ensure you import the errors package and reference runLimactlFiles, callCtx, ctx, runErr and limactlCallTimeout in your changes.cmd/airlock/cli/cli.go (1)
842-847: Remove unusednetworkStatefunction (lines 842–847).The function is dead code. The status command (lines 540–545) now renders lock state inline via
tui.LockColor(...).Render(...)with hardcoded strings, makingnetworkStateunreferenced.🤖 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 842 - 847, Remove the dead helper function networkState from the file: delete the function definition func networkState(locked bool) string since the status command now renders lock state inline via tui.LockColor(...).Render(...); ensure no other code references networkState remain (search for networkState) and remove the unused symbol to avoid compiler warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 73-85: The fenced code block containing the directory listing in
CONTRIBUTING.md is missing a language identifier; update the opening fence from
``` to ```text (or ```plaintext) so markdownlint MD040 is satisfied, i.e.,
locate the block that lists cmd/airlock/cli/, internal/api/, etc., and change
its opening fence to include the language specifier.
In `@Makefile`:
- Around line 34-36: The install target in the Makefile can fail when
$(GOPATH)/bin doesn't exist; update the install recipe for target "install" to
ensure the destination directory exists before copying by creating $(GOPATH)/bin
(e.g., run mkdir -p $(GOPATH)/bin) prior to the cp $(BINARY)
$(GOPATH)/bin/$(BINARY) and echo lines so cp won't error on fresh systems.
In `@README.md`:
- Line 15: README currently has conflicting Go version notes ("From source
(requires Go 1.24)" vs "Requires Go 1.23+"); update both README occurrences to
the single correct version that matches the repository toolchain and CI (use Go
1.24 per PR/CI). Also verify and, if necessary, update the go.mod "go" toolchain
directive and any CI workflow matrix entries (e.g., GitHub Actions job/strategy)
to the same Go 1.24 so all docs, go.mod toolchain and CI are consistent.
- Around line 189-193: The README wording misstates what the ESTABLISHED,RELATED
rule permits; update the text to clarify that the ESTABLISHED,RELATED rule is
applied on the OUTPUT chain (see BuildOutputRules) and therefore allows
reply/related traffic for already-established outbound connections rather than
allowing inbound connections; reword the bullet to something like "Reply/related
traffic for already-established outbound connections is allowed in `strict` and
`cautious` profiles (implemented via the ESTABLISHED,RELATED rule on the OUTPUT
chain / BuildOutputRules)".
- Line 107: The README contains a typo: the field is named LockAfterSetup in
api.NetworkPolicy / NetworkConfig but the text uses LoadAfterSetup; update the
README sentence to use LockAfterSetup instead of LoadAfterSetup so it matches
the actual identifier (referencing api.NetworkPolicy / NetworkConfig and field
LockAfterSetup).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 65-72: The workflow uses the goreleaser action with version:
latest which risks breaking changes; update the action block that defines the
goreleaser step (the step named "Run goreleaser" and the uses:
goreleaser/goreleaser-action@v6) to pin a stable major/minor instead of latest
(for example change the version input from "latest" to a pinned constraint like
"~> v2" or an explicit minor tag) and ensure the args/env lines remain unchanged
so release runs are reproducible.
In `@cmd/airlock/cli/cli.go`:
- Around line 842-847: Remove the dead helper function networkState from the
file: delete the function definition func networkState(locked bool) string since
the status command now renders lock state inline via
tui.LockColor(...).Render(...); ensure no other code references networkState
remain (search for networkState) and remove the unused symbol to avoid compiler
warnings.
In `@internal/api/sandbox.go`:
- Around line 35-51: Update the doc comment for ProgressFn (and reference
CreateOptions.Progress) to state the concurrency and blocking expectations:
explicitly declare whether ProgressFn may be invoked concurrently from multiple
goroutines or only from a single goroutine, and state that implementations must
not block (or must be fast) and should offload long work to a goroutine/channel
to avoid stalling VM creation; mention thread-safety requirements for callers
touching shared UI state.
- Around line 55-67: The SandboxManager interface exposes three overlapping
creation methods (Create, CreateWithProgress, CreateWithOptions); collapse the
interface to a single CreateWithOptions(ctx context.Context, spec SandboxSpec,
opts CreateOptions) (SandboxInfo, error) (or similarly named Create with opts)
and remove Create and CreateWithProgress from the interface surface; keep thin
wrapper helper methods (Create and CreateWithProgress) on the concrete
implementation type that call the new CreateWithOptions with zero-value
CreateOptions or with a ProgressFn wired into opts so callers keep convenience
overloads while implementations and test fakes only need to implement the single
CreateWithOptions entry point (update types that reference ProgressFn and
CreateOptions accordingly and adjust all concrete types, fakes, and tests to
implement the single method).
In `@internal/api/vm.go`:
- Around line 28-33: Remove the duplicated ProvisionVM method from the
Provisioner interface and add a package-level helper RunAllSteps(ctx, p
Provisioner, name string, nodeVersion int) error that becomes the single
implementation point: call p.ProvisionSteps(name, nodeVersion) to get
[]ProvisionStep, iterate the slice and execute each step (invoking whatever
execution method the ProvisionStep type exposes), short-circuiting and returning
the first non-nil error; update callers to use RunAllSteps instead of
ProvisionVM so implementers only need to implement ProvisionSteps (and
SnapshotClean/HasCleanSnapshot).
- Around line 15-19: The Status method should not return an empty string for
“not found” because that conflates with legitimate empty/unknown provider
statuses; change the API to make absence explicit by either (preferred)
introducing a sentinel constant StatusUnknown (string) and return that instead
of "" or by changing the signature to Status(ctx context.Context, name string)
(string, bool, error) where the bool indicates existence; update the interface
declaration for Status, all implementations of the VM interface (methods named
Status in providers), and all call sites to handle the new sentinel or the
existence bool, and update the doc comment to document the new return contract.
In `@internal/network/network.go`:
- Around line 222-251: The runLimactlFiles function currently only special-cases
timeouts; update the error handling after c.Run() to also detect context
cancellation and return a distinct "interrupted" error: after reading
stdout/stderr, if runErr != nil then first check if errors.Is(callCtx.Err(),
context.DeadlineExceeded) to return the existing timeout error, else if
errors.Is(callCtx.Err(), context.Canceled) || errors.Is(ctx.Err(),
context.Canceled) return an error that clearly indicates the call was
interrupted/canceled (wrap runErr for context), and otherwise return runErr;
ensure you import the errors package and reference runLimactlFiles, callCtx,
ctx, runErr and limactlCallTimeout in your changes.
In `@internal/sandbox/create.go`:
- Around line 62-79: The race occurs because we unlock m.mu after removing the
sandbox from m.sandboxes and before calling m.provider.Delete, allowing a
concurrent CreateWithOptions to proceed; fix by installing a tombstone
placeholder under spec.Name (e.g., set m.sandboxes[spec.Name] =
api.SandboxInfo{State: api.StateDeleting} or a dedicated "deleting" marker)
while holding m.mu, then unlock and call m.provider.Delete, and finally acquire
m.mu to replace or remove the tombstone and continue; update references around
m.remove, existing.State, spec.Name and m.provider.Delete to use this tombstone
approach so concurrent creators see the entry and will fail or wait instead of
racing.
In `@internal/vm/lima/config.go`:
- Around line 64-68: The VMMount.Inotify flag is accepted by the API but ignored
because LimaMount (type LimaMount) no longer supports mountInotify; either
remove the Inotify field from the API spec or mark it explicitly as a no-op. To
fix: update the api.VMMount definition (internal/api/vm.go) to remove the
Inotify field if you want to stop accepting it, or retain it but add a clear
comment and API doc note stating it is currently ignored and has no effect, and
ensure any validation or conversion code that maps api.VMMount -> LimaMount (the
code using LimaMount) does not silently drop it without notice.
In `@internal/vm/lima/provider.go`:
- Around line 302-332: cleanLimactlError currently concatenates multiple
free-form stderr lines into "limactl <action>: <line>: <line>" which reads
poorly; update cleanLimactlError so that when adding raw non-prefixed lines (the
branch handling line != ""), you only capture the first non-empty line (or join
into a single-line summary and append an ellipsis/truncated marker if there are
additional lines) instead of appending every line to parts; modify the logic in
cleanLimactlError to collect at most one representative free-form line (and
indicate truncation) before building the final fmt.Sprintf output.
In `@internal/vm/lima/snapshot.go`:
- Around line 30-37: If io.Copy or out.Close fails while writing the snapshot
(see the io.Copy(..., in) error handling and the subsequent out.Close() call
that currently returns the error), remove the partially written destination file
to avoid leaving truncated files behind: on any error path from io.Copy or from
out.Close(), call os.Remove(dst) before returning the error, then return the
original error; leave the successful path to continue to os.Chmod(dst,
perm.Perm()). This touches the error handling around io.Copy, out.Close and the
final os.Chmod call for dst.
🪄 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: faac77a6-fd12-45b7-a7f2-fcc6c56ff5c1
📒 Files selected for processing (19)
.github/workflows/ci.yml.goreleaser.ymlCONTRIBUTING.mdMakefileREADME.mdcmd/airlock/cli/cli.gocmd/airlock/cli/tui/styles.goinstall.shinternal/api/sandbox.gointernal/api/vm.gointernal/network/network.gointernal/network/network_test.gointernal/sandbox/create.gointernal/sandbox/sandbox_test.gointernal/vm/lima/config.gointernal/vm/lima/provider.gointernal/vm/lima/snapshot.gomain.gotest/integration/create_test.go
✅ Files skipped from review due to trivial changes (2)
- .goreleaser.yml
- install.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- main.go
- cmd/airlock/cli/tui/styles.go
- Makefile: mkdir -p $(GOPATH)/bin before install to handle fresh systems - CONTRIBUTING.md: add 'text' language to package-structure fence (MD040) - README.md: fix LoadAfterSetup → LockAfterSetup typo - README.md: clarify ESTABLISHED,RELATED rule applies to OUTPUT chain - README.md: align Go version requirement to 1.24+
This was referenced Apr 17, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--jsonflag tosandboxandstatuscommandsairlock setupto fail or hangBug fixes
limactl createhung waiting for interactive editor — added--tty=falselimactl createreported "instance already exists" — config was written to the instance directory before calling limactl; now uses a temp filemountInotifywas not a valid Lima config field — removed iterroredsandboxes blocked re-runningsetup— now allows overwriting errored entrieslimactl starthung when stderr was streamed to terminal — keep stderr buffered so Lima daemonizeslimactl createstderr so user sees image download progressAI use disclosure
AI-assisted (Claude). Generated initial drafts of documentation and CLI styling code; human-reviewed and modified. Security-sensitive code (network policy, mount handling, privilege escalation paths) received line-by-line human review.
Summary by CodeRabbit
Release Notes
New Features
setup,run,shell,list,status,lock,unlock,destroy, and more.--json) for programmatic integration.strict,cautious,dev,trusted) replacing prior modes.Bug Fixes
Documentation
Chores