feat: init wizard + principles audit fixes (DIP, OCP)#7
Merged
Conversation
Adds `airlock init` interactive wizard (charmbracelet/huh) that asks about trust level, resources, network needs, then saves airlock.toml and optionally creates the sandbox. Also: - config.Save for wizard-generated configs - StartAtLogin option on SandboxSpec + Lima YAML - Fix .gitignore to stop matching cmd/airlock directory
The cli package previously imported internal/vm/lima and instantiated LimaProvider + LimaController directly — violating PRINCIPLES.md §5 (Dependency Inversion: "the CLI never imports Lima directly"). Extract the dependency assembly into a new internal/bootstrap package. The cli package now only imports internal/api (interfaces) plus the bootstrap façade, and accepts a fully-wired *Dependencies via a new ExecuteWithDeps entry point. This unblocks substituting fakes in tests without touching production wiring. Also add api.RuntimeDetector to Dependencies so newInitCmd no longer constructs its own concrete detect.CompositeDetector.
Previously config.validate() hardcoded the list of valid security profiles
and runtime types inline. Adding a new profile or runtime required editing
this package — violating PRINCIPLES.md §5 OCP ("Open/Closed: add behavior
by implementing interfaces, not by editing existing functions") and §4 DRY
(the authoritative list already lives in profile.Registry and
detect.CompositeDetector).
Split validation into two layers:
- validate(): structural rules owned by config (mount path relativity,
traversal, services.compose shape). Runs inside Load.
- ValidateDynamic(): checks profile/runtime membership against
caller-supplied registry names. Called by the CLI via a new
loadAndValidateConfig helper that pulls the allowed names from
deps.Profiles.List() and deps.Detector.SupportedTypes().
Adding a new profile or runtime now requires zero edits to config.
Adds two guard tests: - TestAssembleWiresAllInterfaces: every Dependencies field is non-nil. - TestAssembleInterfaceTypes: return types satisfy the api interfaces, preventing a silent narrowing refactor (LSP).
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 9 minutes and 52 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 (7)
WalkthroughIntroduces an interactive TUI wizard and CLI "init" flow, refactors CLI to use a centralized bootstrap dependency assembler, adds config persistence/formatting for Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "CLI"
participant Wizard as "Wizard (huh)"
participant Detector as "Runtime Detector"
participant Manager as "Sandbox Manager"
participant Provisioner as "Provisioner"
participant Network as "Network Controller"
participant Config as "Config Save"
User->>CLI: run `airlock init [source]`
CLI->>Wizard: Run(source)
Wizard->>User: prompt for source/name, trust, resources, network, persistence
User->>Wizard: provide selections
Wizard->>Detector: (if CreateNow) detect runtime
Detector-->>Wizard: runtime type
Wizard->>Manager: Create sandbox with spec
Manager->>Provisioner: provision VM/filesystem
Manager->>Network: configure network per spec
Wizard->>Config: (if SaveConfig) Save(dir, cfg)
Config-->>Wizard: write `airlock.toml`
Wizard-->>CLI: return WizardResult
CLI-->>User: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
internal/bootstrap/bootstrap.go (2)
43-46:0755on~/.airlock— confirm intent.The config dir will hold
sandboxes.jsonandmounts.json, which may contain host-path mount mappings and sandbox metadata. World-readable (0755) is defensible for a dotdir but consider0700since this is per-user state and nothing external needs to traverse it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/bootstrap.go` around lines 43 - 46, The configDir creation uses os.MkdirAll with mode 0755 which makes ~/.airlock world-readable; change the permission to 0700 to restrict access to the user only. Update the os.MkdirAll call that creates configDir (the variable named configDir in bootstrap.go) to use FileMode 0700 so sandboxes.json and mounts.json remain private.
38-88:Assembleis untestable without Lima on the host, and fails early forinitwizard users.Two related concerns:
Testability (root cause of the CI failures in
bootstrap_test.go).Assembleconstructslima.NewLimaProvider()unconditionally. Any caller — test or otherwise — needslimactlon PATH. Consider introducing a small factory/options seam so tests (and future non-Lima providers) can swap it:type Options struct { ProviderFactory func() (api.Provider, error) // default: lima.NewLimaProvider // ... } func Assemble(opts ...Option) (*Dependencies, error) { ... }UX on
airlock init. The PR wires the interactive wizard throughbootstrap.Assemble(). If a first-time user runsairlock initbefore installing Lima, they'll hitinit lima provider: limactl not foundbefore ever seeing the wizard — despite the wizard not actually needing a live provider to writeairlock.toml. Consider deferring provider construction until a command that truly needs it (lazy init), or lettinginitrun against a partialDependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/bootstrap.go` around lines 38 - 88, Assemble currently constructs lima.NewLimaProvider() unconditionally which makes Assemble untestable and causes init to fail when Lima/limactl is absent; change Assemble to accept an Options/functional option (e.g., type Options struct { ProviderFactory func() (api.Provider, error) } with a default ProviderFactory = lima.NewLimaProvider) or an Option func to override the provider creation, and defer provider construction for commands that don't need a live provider (allow Assemble to return a partial Dependencies without calling ProviderFactory when a flag/option like LazyProvider or skipProvider is set); update callers/tests to pass a test factory or set the lazy flag so tests and airlock init can run without limactl.internal/bootstrap/bootstrap_test.go (1)
48-57: Nit: drop the explicit types on the interface assertion block.
staticcheck(QF1011) flags these; the RHS already carries the interface type fromDependencies. Either drop the type annotations or, better, keep them since they are precisely the LSP guard the comment advertises — in that case, add a//nolint:staticcheck // intentional interface pinto silence the linter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/bootstrap_test.go` around lines 48 - 57, The interface assertion block using explicit types (e.g., _ api.SandboxManager = deps.Manager, _ api.Provider = deps.Provider, etc.) is being flagged by staticcheck QF1011; to keep the explicit LSP-style interface pins, add a linter suppression comment above the block: //nolint:staticcheck // intentional interface pin, so the intent is documented and the linter is silenced; alternatively, remove the explicit interface types and use unnamed assertions (_ = deps.Manager, _ = deps.Provider, ...) if you prefer to avoid the nolint.internal/config/config.go (1)
219-233:nilvs empty-slice semantics are footguns.
ValidateDynamictreatsnilas "skip" but an empty non-nil slice as "nothing is valid → reject everything non-empty". Given the CLI buildsprofileNames/runtimeTypesviaappendon a nil start (seecmd/airlock/cli/cli.go), a registry with zero registrations yieldsnil(skip) rather than "empty list" (reject) — arguably the wrong direction: an empty registry should probably reject unknown values, not silently accept them.Consider either documenting this explicitly (done partially in the comment) or switching to a sentinel like
profileNames == nil && !checkProfilesso the contract isn't coupled to Go's nil-vs-empty-slice distinction. Low priority while the registries are always populated, but worth a note before third parties call this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 219 - 233, ValidateDynamic's current nil-vs-empty-slice semantics are ambiguous; change its signature to explicitly indicate whether to perform checks (e.g., func ValidateDynamic(cfg Config, profileNames []string, runtimeTypes []string, checkProfiles bool, checkRuntimes bool) error) and then: if checkProfiles is true validate cfg.Security.Profile against profileNames (treating an empty non-nil profileNames as "no valid profiles" and reject unknown values), otherwise skip profile validation; similarly use checkRuntimes and runtimeTypes for runtime validation. Update callers (e.g., where profileNames/runtimeTypes are built in cmd/airlock/cli/cli.go) to pass the appropriate checkProfiles/checkRuntimes booleans so the contract no longer relies on nil vs empty-slice.internal/config/save.go (1)
38-71: Section injection viastrings.Split(..., "\n\n")is brittle.This assumes
WriteTOMLemits exactly one blank line between every top-level table and never internally. Any of the following will break comment placement or omit comments entirely:
- A section produced without a preceding blank line (merges two sections into one chunk → only the first gets a comment).
- Inline-table or array-of-tables formatting that embeds a blank line inside a "section".
- A trailing newline producing an empty final chunk (currently skipped, but reorders the last separator).
- Any future change to the TOML serializer’s formatting would silently alter the output.
Consider a more structural approach: walk the TOML line-by-line, detect
[section]headers, and prepend the comment for that section. Something like:♻️ Proposed line-based injection
- // Process TOML content section by section, adding comments - sections := strings.Split(tomlStr, "\n\n") - for i, section := range sections { - if strings.TrimSpace(section) == "" { - continue - } - - // Add section comment based on section header - sectionName := extractSectionName(section) - comment := sectionComment(sectionName, cfg) - if comment != "" { - b.WriteString(comment) - b.WriteString("\n") - } - - b.WriteString(section) - - // Add separator between sections (but not after last) - if i < len(sections)-1 { - b.WriteString("\n\n") - } - } + // Walk line-by-line, injecting a comment block before each [section] header. + for _, line := range strings.Split(tomlStr, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]") && + !strings.HasPrefix(trimmed, "[[") { + name := strings.Trim(trimmed, "[]") + if comment := sectionComment(name, cfg); comment != "" { + b.WriteString("\n") + b.WriteString(comment) + b.WriteString("\n") + } + } + b.WriteString(line) + b.WriteString("\n") + }Also worth adding a test that feeds a config whose serialized output does not contain double newlines between sections, to guard against silent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/save.go` around lines 38 - 71, Replace the brittle chunk-based injection that splits tomlStr by "\n\n" with a line-oriented scan that detects TOML table headers and injects comments immediately before each header; update the logic around tomlStr/tomlBytes in save.go so instead of using strings.Split(..., "\n\n") you iterate over strings.Split(tomlStr, "\n") (or bufio.Scanner) and when you detect a section header (use the same parsing helper extractSectionName to extract the name) call sectionComment(sectionName, cfg) and write the comment then the header line into the strings.Builder; ensure you still preserve blank lines and other non-header lines verbatim and avoid dropping a trailing newline, and add a unit test that serializes a config with varying internal blank lines/inline-tables to validate comment placement.cmd/airlock/cli/wizard/questions.go (2)
32-36:SourceInfois dead code.The struct is declared but never constructed, returned, or referenced anywhere in the wizard package. Either wire it into
DeriveSandboxName(e.g., return aSourceInfoso callers can know whether the source was GitHub) or remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/wizard/questions.go` around lines 32 - 36, The SourceInfo struct is unused dead code; either remove the SourceInfo declaration or wire it into the wizard flow by having DeriveSandboxName (or a related function in the wizard package) return a SourceInfo (or include it in an existing return struct) so callers can detect IsGitHub and Path; update all call-sites of DeriveSandboxName to accept and use the new SourceInfo return (or delete SourceInfo and remove any vestigial references), and run tests/build to ensure no unused symbol remains.
363-370: Duplicated TTY detection.
wizard.IsTTY()andcli.isTerminal()(incmd/airlock/cli/cli.go:104-110) are identical in behavior. Additionally,IsTTY()is exported but not called from anywhere in this PR that I can see. Consider either exporting a single helper from one place (e.g., a small sharedcli/ttyutilpackage) and having both call sites use it, or droppingIsTTYif the wizard never needs to branch on it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/wizard/questions.go` around lines 363 - 370, The IsTTY function duplicates cli.isTerminal; either remove the unused exported IsTTY or consolidate both call sites to one shared helper: create a small package (e.g., ttyutil) exporting a single function (e.g., IsTTY or IsTerminal), move the existing logic there, update callers in wizard and cli to import and call that helper, and delete the duplicate IsTTY/cli.isTerminal implementations; ensure you update imports and keep the exported name consistent with existing usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/airlock/cli/cli.go`:
- Around line 855-863: The provisioner is receiving a NodeVersion of 0 because
WizardResult.ToConfig does not populate NodeVersion (so wizardCfg.VM.NodeVersion
is always 0); update the mapping so NodeVersion is set from config.Defaults()
(or otherwise apply config.Defaults() to the wizard-produced config) before it’s
used by deps.Provisioner.ProvisionSteps(spec.Name, wizardCfg.VM.NodeVersion).
Modify WizardResult.ToConfig to call config.Defaults() and copy the default
NodeVersion into the returned config.VMConfig (mirroring the behavior in
newSetupCmd where nodeVersion is defaulted to 22), or alternatively apply the
default at the call site before invoking ProvisionSteps.
- Around line 812-826: The code sets configDir = result.Source which will fail
if that local path doesn't exist; before calling config.Save(configDir,
wizardCfg) ensure the directory exists by either using the already-resolved
deps.ConfigDir instead of result.Source, or create the path with
os.MkdirAll(configDir, 0755) when result.Source is a local path, and then call
config.Save; update the logic around result.SaveConfig/result.Source/configDir
to choose deps.ConfigDir for resolved projects or create the directory for
user-provided local paths so config.Save and the final fmt.Fprintf succeed.
In `@cmd/airlock/cli/wizard/mappings_test.go`:
- Around line 154-176: Replace the non-fatal length assertion in TestTrustLevels
to stop the test immediately on mismatch: change the len(levels) check that
currently calls t.Errorf to t.Fatalf so the subsequent indexed access of
levels[i] cannot panic; do the same for the analogous length checks in
TestResourceLevels and TestNetworkLevels (use t.Fatalf for those size
assertions) so the tests fail fast and surface the original assertion rather
than an out-of-range runtime crash when TrustLevels(), ResourceLevels(), or
NetworkLevels() return too few entries.
In `@cmd/airlock/cli/wizard/mappings.go`:
- Around line 217-239: The ToConfig method on WizardResult currently zeroes
NodeVersion and hardcodes Disk and Ports; change it to start from
config.Defaults() and only override fields collected by the wizard (use cfg :=
config.Defaults(); set cfg.VM.CPU, cfg.VM.Memory from
MapResourceLevel(r.ResourceLevel), set cfg.Security.Profile from
MapTrustLevelToProfile(r.TrustLevel), set cfg.Runtime.Type=runtime, set
cfg.StartAtLogin=r.StartAtLogin) so NodeVersion and canonical Disk/Ports are
preserved; also mirror this approach in ToSandboxSpec so newInitCmd (which reads
wizardCfg.VM.NodeVersion) receives the proper nodeVersion and defaults remain
consistent.
In `@cmd/airlock/cli/wizard/questions.go`:
- Around line 20-30: Remove the unused lipgloss styles descriptionStyle and
successStyle (they are declared but never referenced); delete the variables
descriptionStyle and successStyle from the top-level style declarations in
questions.go, or alternatively replace the plain fmt.Printf summary output and
the success usage in cli.go with these styles if styling was
intended—specifically update/remove the symbols descriptionStyle and
successStyle and ensure no other code references them (the summary block
currently uses fmt.Printf and cli.go uses tui.SuccessLine).
In `@go.mod`:
- Around line 13-44: The go.mod shows charmbracelet/huh (and related
bubbletea/bubbles) only as indirect — run `go mod tidy` from the module root to
let the tool promote any actually imported packages (e.g., imports in
cmd/airlock/cli/wizard) into direct require entries, verify that
github.com/charmbracelet/huh (and bubbletea/bubbles if used) now appear in the
direct require block, and commit the updated go.mod/go.sum so CI tidy won't
produce a diff; also double-check cmd/airlock/cli/wizard imports reference the
correct module paths.
In `@internal/bootstrap/bootstrap_test.go`:
- Around line 12-58: Tests fail when Assemble() constructs a concrete Lima
provider (via lima.NewLimaProvider()) and limactl is not on PATH; update the
code so tests no longer require the Lima binary by either (A) making Assemble
tolerant of a missing limactl and returning a sentinel error the tests can skip
on (so TestAssembleWiresAllInterfaces and TestAssembleInterfaceTypes check for
that sentinel and call t.Skip), or (B) refactoring Assemble into a testable
constructor that accepts a provider factory (e.g., add
assembleWith(providerFactory func() (api.Provider, error), ...) and change
Assemble to call assembleWith(lima.NewLimaProvider) so tests can inject a fake
provider—choose one approach and implement the change, ensuring references to
Assemble and lima.NewLimaProvider are updated accordingly and tests are adjusted
to either skip on the sentinel error or pass a fake provider factory.
In `@internal/config/save_test.go`:
- Around line 97-117: In TestFormatWithComments_StartAtLogin, fix two issues:
replace the dead t.Log assertion with a real failure (use t.Error or t.Fatalf)
so missing "Auto-start" actually fails the test, and update the misleading
comment that says "omitempty=false" to accurately state that the struct tag is
`start_at_login,omitempty` and the field appears because StartAtLogin is true
(non-zero), not because omitempty is absent; reference
TestFormatWithComments_StartAtLogin, StartAtLogin, and FormatWithComments when
making the changes.
In `@internal/config/save.go`:
- Around line 38-71: Remove the trailing whitespace on the blank lines in the
TOML-building block so gofmt passes: edit the strings.Builder writes in the save
logic (the header/comment assembly around the Builder and the section-loop that
writes separators) to ensure blank-line strings contain no extra tabs/spaces,
then run `gofmt -w` (or strip trailing whitespace in your editor) to reformat;
you can locate the code in the function that builds tomlStr using
strings.Builder and references like sectionComment(...) and
extractSectionName(...) to find the exact places to clean up.
---
Nitpick comments:
In `@cmd/airlock/cli/wizard/questions.go`:
- Around line 32-36: The SourceInfo struct is unused dead code; either remove
the SourceInfo declaration or wire it into the wizard flow by having
DeriveSandboxName (or a related function in the wizard package) return a
SourceInfo (or include it in an existing return struct) so callers can detect
IsGitHub and Path; update all call-sites of DeriveSandboxName to accept and use
the new SourceInfo return (or delete SourceInfo and remove any vestigial
references), and run tests/build to ensure no unused symbol remains.
- Around line 363-370: The IsTTY function duplicates cli.isTerminal; either
remove the unused exported IsTTY or consolidate both call sites to one shared
helper: create a small package (e.g., ttyutil) exporting a single function
(e.g., IsTTY or IsTerminal), move the existing logic there, update callers in
wizard and cli to import and call that helper, and delete the duplicate
IsTTY/cli.isTerminal implementations; ensure you update imports and keep the
exported name consistent with existing usage.
In `@internal/bootstrap/bootstrap_test.go`:
- Around line 48-57: The interface assertion block using explicit types (e.g., _
api.SandboxManager = deps.Manager, _ api.Provider = deps.Provider, etc.) is
being flagged by staticcheck QF1011; to keep the explicit LSP-style interface
pins, add a linter suppression comment above the block: //nolint:staticcheck //
intentional interface pin, so the intent is documented and the linter is
silenced; alternatively, remove the explicit interface types and use unnamed
assertions (_ = deps.Manager, _ = deps.Provider, ...) if you prefer to avoid the
nolint.
In `@internal/bootstrap/bootstrap.go`:
- Around line 43-46: The configDir creation uses os.MkdirAll with mode 0755
which makes ~/.airlock world-readable; change the permission to 0700 to restrict
access to the user only. Update the os.MkdirAll call that creates configDir (the
variable named configDir in bootstrap.go) to use FileMode 0700 so sandboxes.json
and mounts.json remain private.
- Around line 38-88: Assemble currently constructs lima.NewLimaProvider()
unconditionally which makes Assemble untestable and causes init to fail when
Lima/limactl is absent; change Assemble to accept an Options/functional option
(e.g., type Options struct { ProviderFactory func() (api.Provider, error) } with
a default ProviderFactory = lima.NewLimaProvider) or an Option func to override
the provider creation, and defer provider construction for commands that don't
need a live provider (allow Assemble to return a partial Dependencies without
calling ProviderFactory when a flag/option like LazyProvider or skipProvider is
set); update callers/tests to pass a test factory or set the lazy flag so tests
and airlock init can run without limactl.
In `@internal/config/config.go`:
- Around line 219-233: ValidateDynamic's current nil-vs-empty-slice semantics
are ambiguous; change its signature to explicitly indicate whether to perform
checks (e.g., func ValidateDynamic(cfg Config, profileNames []string,
runtimeTypes []string, checkProfiles bool, checkRuntimes bool) error) and then:
if checkProfiles is true validate cfg.Security.Profile against profileNames
(treating an empty non-nil profileNames as "no valid profiles" and reject
unknown values), otherwise skip profile validation; similarly use checkRuntimes
and runtimeTypes for runtime validation. Update callers (e.g., where
profileNames/runtimeTypes are built in cmd/airlock/cli/cli.go) to pass the
appropriate checkProfiles/checkRuntimes booleans so the contract no longer
relies on nil vs empty-slice.
In `@internal/config/save.go`:
- Around line 38-71: Replace the brittle chunk-based injection that splits
tomlStr by "\n\n" with a line-oriented scan that detects TOML table headers and
injects comments immediately before each header; update the logic around
tomlStr/tomlBytes in save.go so instead of using strings.Split(..., "\n\n") you
iterate over strings.Split(tomlStr, "\n") (or bufio.Scanner) and when you detect
a section header (use the same parsing helper extractSectionName to extract the
name) call sectionComment(sectionName, cfg) and write the comment then the
header line into the strings.Builder; ensure you still preserve blank lines and
other non-header lines verbatim and avoid dropping a trailing newline, and add a
unit test that serializes a config with varying internal blank
lines/inline-tables to validate comment placement.
🪄 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: 426af0e5-3349-44d6-ab3a-f10aa3afd5ca
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.gitignorecmd/airlock/cli/cli.gocmd/airlock/cli/wizard/mappings.gocmd/airlock/cli/wizard/mappings_test.gocmd/airlock/cli/wizard/questions.gocmd/airlock/cli/wizard/questions_test.gogo.modinternal/api/vm.gointernal/bootstrap/bootstrap.gointernal/bootstrap/bootstrap_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/save.gointernal/config/save_test.gointernal/vm/lima/config.go
💤 Files with no reviewable changes (1)
- .gitignore
The init wizard hands the controlling TTY to huh during the form, then calls Manager.Create → provider.Start. limactl start without --tty=false tries to render progress via an interactive terminal writer against a stdout that is no longer a tty (or has been left in a mangled state by huh), which stalls or corrupts output. Create already passed --tty=false; add it to Start, Stop and Delete too so every non-interactive invocation is consistent. Update the fake limactl in the integration harness to tolerate flag args in any position before the VM name.
limactl rejects relative mount paths with: field `mounts[0].location` must be an absolute path, got "." The wizard (and any CLI invocation with Source=".") passed the literal source string straight through to api.VMMount.HostPath. Resolve local filesystem sources with filepath.Abs before handing them to the VM provider. Remote sources (gh:, https://, http://, git@) skip the mount entirely since they are cloned inside the VM, not mounted from the host.
The "Preparing airlock home" step ran `chown -R airlock:airlock /home/airlock`, which descended into /home/airlock/projects/<name> — a virtiofs mount backed by the host where chown returns EPERM: chown: changing ownership of '/home/airlock/projects/sandbox': Operation not permitted Use `find -xdev -exec chown ... +` so the traversal stops at filesystem boundaries and never touches the host-backed mount.
find -xdev still visits the mount point entry itself; chown on /home/airlock/projects/sandbox crosses into virtiofs and returns EPERM. Prune /home/airlock/projects from the walk and chown the dir non-recursively.
limactl shell defaults to host cwd which does not exist inside the VM, causing 'cd: No such file or directory' on session start. Explicitly cd into /home/airlock/projects/<name>.
limactl shell defaulted to the host user which cannot access /home/airlock/projects/<name>. Switch to the airlock user via sudo -iu and cd into the project mount before exec'ing the interactive bash.
- cli.go: nil-check Detector, log Detect errors, MkdirAll local configDir - wizard/mappings.go: ToConfig/ToSandboxSpec build on config.Defaults() so NodeVersion, Disk, Ports inherit canonical defaults instead of zero - wizard/questions.go: drop unused descriptionStyle/successStyle; sanitizeName preserves first rune after underscore prefix (e.g. "1project" -> "_1project") - wizard/mappings_test.go: length checks use t.Fatalf to avoid index panic - config/save.go: split TOML on section headers (^\[.*\]$) instead of blank lines; filter empty sections; trim trailing newlines - config/save_test.go: remove dead t.Log assertion; correct omitempty comment - bootstrap/bootstrap_test.go: skip when limactl not in PATH - sandbox/sandbox.go: Abs failure returns "" so caller skips mount - go mod tidy promotes huh to direct require
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
internal/bootstrap/bootstrap_test.go (1)
55-64:⚠️ Potential issue | 🟡 MinorAvoid the QF1011 lint failure while keeping the interface contract check.
The explicit typed blank identifier declarations are flagged by static analysis. A typed no-op function preserves the same assignment checks without tripping the lint rule.
🔧 Proposed fix
- var ( - _ api.SandboxManager = deps.Manager - _ api.Provider = deps.Provider - _ api.Provisioner = deps.Provisioner - _ api.ShellProvider = deps.Sheller - _ api.MountManager = deps.Mounts - _ api.NetworkController = deps.Network - _ api.ProfileRegistry = deps.Profiles - _ api.RuntimeDetector = deps.Detector - ) + requireInterfaces := func( + api.SandboxManager, + api.Provider, + api.Provisioner, + api.ShellProvider, + api.MountManager, + api.NetworkController, + api.ProfileRegistry, + api.RuntimeDetector, + ) { + } + requireInterfaces( + deps.Manager, + deps.Provider, + deps.Provisioner, + deps.Sheller, + deps.Mounts, + deps.Network, + deps.Profiles, + deps.Detector, + )Verify the lint pattern is gone:
#!/bin/bash rg -n -C2 '^\s*_\s+api\.[A-Za-z]+\s*=\s*deps\.' internal/bootstrap/bootstrap_test.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/bootstrap/bootstrap_test.go` around lines 55 - 64, The typed blank identifier interface checks (e.g., _ api.SandboxManager = deps.Manager, _ api.Provider = deps.Provider, etc.) trigger QF1011; replace them with a typed no-op helper that accepts the concrete values to enforce compile-time assignment without using typed blank identifiers — add a small function like assertImplementsSandboxManager(provider api.SandboxManager) and call it with deps.Manager (and similarly for api.Provider with deps.Provider, api.Provisioner with deps.Provisioner, api.ShellProvider with deps.Sheller, api.MountManager with deps.Mounts, api.NetworkController with deps.Network, api.ProfileRegistry with deps.Profiles, api.RuntimeDetector with deps.Detector) so the compiler still checks the interface contracts but the linter no longer flags QF1011.cmd/airlock/cli/cli.go (1)
860-867:⚠️ Potential issue | 🟠 MajorDefault the wizard Node.js version before provisioning.
Line 861 still passes
wizardCfg.VM.NodeVersiondirectly. If the wizard config mapping has not merged defaults, the provisioner receives0and can install/provision with an invalid Node version.Proposed defensive fix
// Add provisioning steps - for _, step := range deps.Provisioner.ProvisionSteps(spec.Name, wizardCfg.VM.NodeVersion) { + nodeVersion := wizardCfg.VM.NodeVersion + if nodeVersion <= 0 { + nodeVersion = config.Defaults().VM.NodeVersion + } + for _, step := range deps.Provisioner.ProvisionSteps(spec.Name, nodeVersion) {Run this read-only check to confirm whether
WizardResult.ToConfigalready sets a non-zero default:#!/bin/bash # Verify whether wizard config mapping populates VM.NodeVersion/defaults. sed -n '180,260p' cmd/airlock/cli/wizard/mappings.go rg -n -C4 'NodeVersion|func .*ToConfig|Defaults' cmd/airlock/cli/wizard/mappings.go internal/config🤖 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 860 - 867, The loop passes wizardCfg.VM.NodeVersion directly to deps.Provisioner.ProvisionSteps which can be zero if defaults weren't merged; ensure a non-zero Node.js version is used by normalizing wizardCfg.VM.NodeVersion before calling ProvisionSteps (e.g., if wizardCfg.VM.NodeVersion == 0 then set it to the agreed default constant or derive it from WizardResult.ToConfig/defaults) so ProvisionSteps always receives a valid Node version; update the code that builds phases (the block using deps.Provisioner.ProvisionSteps and wizardCfg.VM.NodeVersion) to compute and pass the normalizedVersion instead of the raw value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/airlock/cli/cli.go`:
- Around line 794-798: Before calling wizard.Run(source) add a fast-fail check
against the injected TTY state (e.g. the CLI's tty/IsInteractive() or injected
tty variable) and return an explicit error if not interactive; in other words,
before invoking wizard.Run(source) verify the injected TTY (tty.IsInteractive()
or equivalent) and if it reports non-interactive return a clear error such as
"init requires an interactive terminal" instead of launching the wizard.
- Around line 41-53: The CLI currently marks the session interactive in
FromBootstrap by only checking stdout (IsTTY = isTerminal(os.Stdout)), allowing
commands like `airlock shell < /dev/null` to be treated as interactive; change
the IsTTY assignment to require both stdin and stdout are terminals (e.g., IsTTY
= isTerminal(os.Stdout) && isTerminal(os.Stdin)) so Dependencies.IsTTY reflects
that both streams are TTYs; update the FromBootstrap function accordingly
(ensure isTerminal is called for os.Stdin as well).
In `@cmd/airlock/cli/wizard/mappings.go`:
- Around line 200-236: The NetworkLevel and StartAtLogin choices from
WizardResult are not propagated into the created spec/config: update
ToSandboxSpec and ToConfig to explicitly set network and startup intent rather
than relying solely on the profile defaults—add a NetworkLock/NetworkLevel (or
LockAfterSetup boolean) and a StartAtLogin field to api.SandboxSpec (so
resolveResources/VMSpec path receives it) and set those fields from
r.NetworkLevel and r.StartAtLogin in ToSandboxSpec; also in ToConfig ensure
cfg.Security.LockAfterSetup (or equivalent) and cfg.StartAtLogin are assigned
from r.NetworkLevel and r.StartAtLogin so persisted configs honor the wizard
choices (refer to WizardResult.ToSandboxSpec, WizardResult.ToConfig,
NeedsNetworkLock, api.SandboxSpec, config.Config, resolveResources, VMSpec).
In `@cmd/airlock/cli/wizard/questions.go`:
- Around line 261-274: The summary currently hardcodes "20GB disk"; change it to
read the actual default disk size used when generating the spec by fetching
config.Defaults().VM.Disk (or the corresponding disk value on the result if
that's where it's stored) and use that formatted value in the printf instead of
the literal "20GB disk". Locate the confirmation block (symbols:
MapResourceLevel, MapTrustLevelToProfile, getNetworkDescription, boolToYesNo)
and replace the hardcoded disk string with a formatted value derived from
config.Defaults().VM.Disk (e.g., format as "%dGB" or convert to a human-readable
string) so the displayed summary matches the real default.
---
Duplicate comments:
In `@cmd/airlock/cli/cli.go`:
- Around line 860-867: The loop passes wizardCfg.VM.NodeVersion directly to
deps.Provisioner.ProvisionSteps which can be zero if defaults weren't merged;
ensure a non-zero Node.js version is used by normalizing
wizardCfg.VM.NodeVersion before calling ProvisionSteps (e.g., if
wizardCfg.VM.NodeVersion == 0 then set it to the agreed default constant or
derive it from WizardResult.ToConfig/defaults) so ProvisionSteps always receives
a valid Node version; update the code that builds phases (the block using
deps.Provisioner.ProvisionSteps and wizardCfg.VM.NodeVersion) to compute and
pass the normalizedVersion instead of the raw value.
In `@internal/bootstrap/bootstrap_test.go`:
- Around line 55-64: The typed blank identifier interface checks (e.g., _
api.SandboxManager = deps.Manager, _ api.Provider = deps.Provider, etc.) trigger
QF1011; replace them with a typed no-op helper that accepts the concrete values
to enforce compile-time assignment without using typed blank identifiers — add a
small function like assertImplementsSandboxManager(provider api.SandboxManager)
and call it with deps.Manager (and similarly for api.Provider with
deps.Provider, api.Provisioner with deps.Provisioner, api.ShellProvider with
deps.Sheller, api.MountManager with deps.Mounts, api.NetworkController with
deps.Network, api.ProfileRegistry with deps.Profiles, api.RuntimeDetector with
deps.Detector) so the compiler still checks the interface contracts but the
linter no longer flags QF1011.
🪄 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: 047b4393-723a-40ba-bea5-0a558871e40f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.gitignorecmd/airlock/cli/cli.gocmd/airlock/cli/wizard/mappings.gocmd/airlock/cli/wizard/mappings_test.gocmd/airlock/cli/wizard/questions.gocmd/airlock/cli/wizard/questions_test.gogo.modinternal/bootstrap/bootstrap_test.gointernal/config/save.gointernal/config/save_test.gointernal/sandbox/sandbox.gointernal/vm/lima/provider.gointernal/vm/lima/snapshot.gotest/integration/harness_test.go
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- go.mod
- cmd/airlock/cli/wizard/mappings_test.go
- cmd/airlock/cli/wizard/questions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/save_test.go
- cli.go: IsTTY requires stdin AND stdout; init fails fast when non-TTY - api.SandboxSpec: add StartAtLogin so it propagates through create-now path - sandbox.resolveResources: pass spec.StartAtLogin into VMSpec - wizard.ToSandboxSpec: set StartAtLogin from wizard result - wizard summary: render actual default disk size instead of hardcoded 20GB NetworkLevel intent still only influences behavior via the trust profile; propagating it to applyNetworkPolicy requires a broader refactor left for a follow-up PR.
Address remaining CodeRabbit finding on the wizard mapping: NetworkLevel was collected but never reached the create path, so "Ongoing access" and "None" were silently overridden by the trust profile's default LockAfterSetup. - Add api.SandboxSpec.LockNetworkAfterSetup (*bool) override. - Manager.applyNetworkPolicy now honors the spec override; existing ApplyNetworkProfile path (no spec) passes nil to preserve behavior. - ToSandboxSpec sets LockNetworkAfterSetup from NetworkLevel. - init flow now Locks (None/Downloads) or Unlocks (Ongoing) after ApplyNetworkProfile so wizard intent is authoritative over the profile default.
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.
Summary
Bundles the in-progress
airlock initwizard with three PRINCIPLES.md audit fixes stacked on top.Commits
main. Addsairlock init(charmbracelet/huh),config.Save,StartAtLoginspec field + Lima YAML plumbing, and un-breaks.gitignorewhich was swallowing the entirecmd/airlock/directory.cmd/airlock/cli/cli.goimportedinternal/vm/limaand directly instantiatedLimaProvider+LimaController. Assembly now lives in a newinternal/bootstrappackage; the cli only depends oninternal/apiinterfaces and the bootstrap façade. AddsExecuteWithDepsso fakes can be injected in tests.config.validatepreviously hardcoded the profile and runtime lists; adding a new profile required editing the config package. Split into structuralvalidate()(still in Load) andValidateDynamic()which the CLI drives withdeps.Profiles.List()+deps.Detector.SupportedTypes().Deferred (out of scope for this PR)
internal/sandbox(audit finding feat: implement sandbox orchestrator and harden Lima provider #3). The Manager orchestrates VM lifecycle + snapshot restore + resource check + profile resolve + network policy + mounts. Splitting it intosandbox/provisioner/resetter/resourcecheckis a ~10-file behavior-preserving refactor that deserves its own PR per PRINCIPLES.md §1 ("reviewable in under 10 minutes"). Tracked for follow-up.Test plan
go build ./...go vet ./...go test ./...— all packages pass including new bootstrap tests and rewrittenValidateDynamictestsairlock initwizard end-to-endairlock setupstill wires through bootstrap correctly🤖 Generated with Claude Code
Summary by CodeRabbit