feat(orbstack-ad-hoc-vm): fail fast when the base VM is missing #47
Loading…
Reference in a new issue
No description provided.
Delete branch "align/orbstack-ad-hoc-vm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds one contract: if cloning
node-pnpm-basefails withmachine not found, stop and tell the user rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract. (The base VM is currently missing on the workstation and will be recreated under the same name.)The rest is deletion: the preflight section (
--helpinvocations and status checks), the second homes of thebash -lcand file-transfer rules, the generic verification bullets, the confirm-removal step, and the intro that restated the frontmatter description.Approach review: The approach looks good.
This is a documentation-only change to one skill file, and both halves of it are well chosen:
skills/orbstack-ad-hoc-vm/SKILL.md:16) rather than in the trailing## Practical rulessection is right for an agent-read skill: the instruction sits where the failure actually surfaces. Treating the base VM's contents as a user-owned contract, and refusing to substitute or reinstall, is the correct default for a machine the user maintains.orbctl status/orbctl listand keying offmachine not foundis one fewer round trip and has no gap: the clone is the operation that must succeed anyway, so a separate pre-check could only go stale between the probe and the clone.bash -lcrationale and the file-transfer rule each had two homes; folding each into its first point-of-use removes the risk of the copies drifting apart. Removing the## When to usetrigger list matches how the rest of this repository works — the frontmatterdescriptionis whataxskillsshows in the picker and what drives selection, and it already carries those trigger phrases, so the section was duplicating the selection signal. Only one of the 25 skills here still keeps such a section.One judgment call worth a second look, not a blocker and not an alternative design:
## Reporting backnow omits whether the test passed or failed. The environment-versus-product distinction it also dropped is still covered by theDistinguish environment failures from product failuresrule, but the pass/fail outcome is not stated anywhere else in the file. That may well be intentional as self-evident for any run report — flagging it only so the omission is a decision rather than a side effect of the trim.No material alternative to suggest.
Approach review by Claude Code Opus (opus)
Summary: Documentation-only change to
skills/orbstack-ad-hoc-vm/SKILL.md. The new fail-fast contract is a genuine improvement, and most deletions (the## When to useblock restating the frontmatter, the duplicatedbash -lcand file-transfer rules) remove real duplication. Found 2 medium and 1 low issue, all about coverage that the deletions removed without a replacement.Main concerns: the new stop-and-tell rule is keyed to one exact error string while the preflight that caught every other startup failure was deleted in the same commit, and
## Reporting backno longer asks for the verification outcome even though the skill exists to verify code and still instructs the agent to classify environment vs. product failures.Note:
orbctlis macOS-only and not present in this environment, andaskpplxhad no API key, so I did not verify the literalmachine not foundtext — the comment below only addresses the fragility of matching one exact string, not whether that string is correct.Code review by Claude Code Opus (opus)
@ -148,4 +93,2 @@Include:- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failed🟡 Medium: Dropping
whether the test passed or failedandwhether any failure was due to missing environment setup versus the code under testleaves the report contract without the verification result — the one thing this skill runs a VM to produce. The remaining two bullets cover packages installed and whether the VM was deleted, so a report that satisfies this section can omit the outcome entirely.The second deleted bullet was also the only consumer of the
### Distinguish environment failures from product failuresrule, which is still present at line 79 of the new file. The agent is told to make that classification but no longer told to report it.The PR body justifies removing the generic verification bullets under section 4 (
command exits with the expected status, etc.), which were genuinely generic; these two are not the same thing. Suggest restoring them:@ -24,4 +10,0 @@Before cloning, learn the current OrbStack interface:```bashorbctl --help🟢 Low: Removing the preflight also removes the only instruction to discover the current
orbctlinterface, which leaves the escape hatch on line 8 (unless the user requests a different distro or a clean environment without Node.js/pnpm) unsupported: the skill documentsclone,run,push, anddelete, but never a way to create a machine that is not a clone ofnode-pnpm-base. An agent taking that branch now has neither an example command nor a--helpstep to fall back on.Either add a one-line create example under the clone section, or a short note that for a non-
node-pnpm-baseenvironment the agent should consultorbctl --help/orbctl create --helpfirst.@ -43,3 +13,4 @@`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails with `machine not found`, stop and tell the user the `node-pnpm-base` base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.🟡 Medium: The guardrail is narrower than the failure mode it protects against. It fires only when the clone fails with
machine not found, but the same commit deletes theorbctl status/orbctl listpreflight that previously caught the other startup failures (OrbStack not running, daemon unreachable, storage errors). For any of those the skill now has no stop rule at all, so the exact behavior this PR is trying to prevent — substituting another machine or improvising a fresh install — is still unguided.Matching one literal string is also brittle: if the wording is
machine 'node-pnpm-base' not foundor similar, a strict reading of this sentence does not apply. (I could not check the actual text —orbctlis macOS-only and unavailable here.)Suggested fix: trigger on the failure rather than the message, naming the missing-base case as the example.
Feedback for the
c64506areview cycle, processed at head38d48fb:38d48fb— the non-clone branch now namesorbctl create DISTRO[:VERSION] MACHINE_NAMEinline.orbctlreally emitsmachine not found: '<name>', and the substitution hazard the rule guards is specific to the missing-base case; details in the thread.@ -148,4 +93,2 @@Include:- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failedRe the 🟡 report-contract finding: this cut was a decision, not a side effect of the trim. The pass/fail outcome is the deliverable of the whole task — an agent asked to test something in a clean VM cannot complete the task without stating the result, so a checklist bullet for it adds nothing. The checklist carries the state that is at risk of going unreported: packages installed beyond the base image (reproducibility) and whether the VM was deleted (cleanup). On the orphaned-consumer point:
Distinguish environment failures from product failuresis a behavioral rule — an environment failure gets the bootstrap fixed and the run retried rather than misreported as a product bug — so any failure that reaches the report already arrives with its classification; and the one reporting distinction genuinely at risk of being folded away is still explicitly covered byReport portability issues separately. Not restoring the bullets.@ -24,4 +8,0 @@Before cloning, learn the current OrbStack interface:```bashorbctl --helpRe the 🟢 unsupported-escape-hatch finding: agreed — the non-clone branch had no supporting command once the generic
orbctl --helppreflight went away (the old preflight's four--helpcalls never coveredcreateeither, butorbctl --helpat least surfaced it). Fixed in38d48fb: the escape-hatch sentence now carries the create command inline —orbctl create DISTRO[:VERSION] MACHINE_NAME, withorbctl create --helpas the pointer for the distro list. Signature verified against the live CLI.@ -43,6 +13,8 @@ orbctl list`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails with `machine not found`, stop and tell the user the `node-pnpm-base` base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.Re the 🟡 guardrail-narrower finding: verified the literal string against live
orbctlon this host (the review environment could not) — cloning a nonexistent machine fails with[-32098] machine not found: 'definitely-nonexistent-machine-xyz', so the backtick-quotedmachine not foundmatches the real output verbatim as a substring, and the hypothesizedmachine 'x' not foundvariant does not occur. That settles the brittleness half empirically.On coverage: the narrow trigger is deliberate. The hazard this rule guards — substituting another machine or improvising a fresh install — is specific to the missing-base case; when OrbStack itself is down or unreachable, no machine can be cloned or created, so the substitution hazard cannot arise there, and ordinary fail-and-report behavior covers those errors without a dedicated rule. Widening to "fails for any reason" would also force dropping the precise diagnosis the sentence carries (that the
node-pnpm-basebase VM is missing), which is what makes the stop actionable for the user. The preflight removal was itself a deliberate point of the approved spec — detect at the operation that must succeed anyway — so the trigger stays as written. No change.Summary: Documentation-only change to
skills/orbstack-ad-hoc-vm/SKILL.md: one added fail-fast contract plus removal of preflight, duplicated rules, and generic bullets. The deletions are consistent with the repo's ownai-facing-writing-styleguidance (don't restate--help, cut self-evident steps, group by concept), and the new stop rule survives that guide's exception test since more than one response is plausible once it fires.Found 2 low issues and 1 observation. Note:
orbctlis not available in this environment andaskpplxhas no API key configured here, so OrbStack's exact error text andorbctl createusage string were not verified against the tool — the argument order shown matches the documentedorb create <distro>[:version] [name]form to the best of my knowledge, but the literalmachine not foundwording is unconfirmed.Code review by Claude Code Opus (opus)
@ -36,3 +8,1 @@orbctl statusorbctl list```Always clone `node-pnpm-base` unless the user requests a different distro or a clean environment without Node.js/pnpm — then create a fresh machine instead: `orbctl create DISTRO[:VERSION] MACHINE_NAME` (`orbctl create --help` lists the distros).ℹ️ Info: The
orbctl createbranch is introduced here and then never rejoins the workflow: every numbered step below is written for the clone path, and step 2's "The base image includesgit,curl,wget,jq, Node.js LTS, and pnpm" plus step 1'sbash -lcrationale (PNPM_HOMEin~/.profile) do not hold for a freshly created machine. An agent can work that out, so this is only worth a line if you want the non-clone path explicitly scoped — for example noting that the base-image toolchain claims apply to the clone only.@ -43,6 +13,8 @@ orbctl list`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails with `machine not found`, stop and tell the user the `node-pnpm-base` base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.🟢 Low: The stop rule is keyed to one literal error string, but the behavior it guards against (substituting a machine or improvising an install) is what happens on any clone failure the agent decides to work around. If OrbStack words the failure differently — a different phrasing for a missing machine, or a daemon-not-running error now that the
orbctl statuspreflight is gone — the rule reads as not applying and the agent is back to improvising.Scoping the trigger to the condition rather than the message closes that gap, e.g. "If the clone fails because
node-pnpm-basedoes not exist, stop and tell the user the base VM is missing …", optionally keepingmachine not foundas the example wording.@ -150,3 +95,1 @@- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failed- whether any failure was due to missing environment setup versus the code under test- any packages installed beyond the base image🟢 Low: The reporting contract lost the environment-vs-product classification, but the rule that produces it is still in the doc ("Distinguish environment failures from product failures" — do not misclassify a missing toolchain as an application bug). With no reporting hook, a run can apply that rule internally and never surface the distinction to the user, which is the one part of the report the user cannot reconstruct. (Dropping "whether the test passed or failed" is fine — an agent asked to verify something reports the verdict regardless.)
Restoring a single bullet along the lines of "whether a failure was environment setup or the code under test" keeps the list short and preserves that output.
Feedback for the
38d48fbreview cycle, processed at headd89db50:d89db50— the trigger now keys on the missing-base condition, with the live-verifiedmachine not foundtext kept as the recognizable example. Two cycles independently flagging the string trigger was the signal to move.On the summary's verification caveats: both were settled against the live CLI during the first cycle — the literal error text in the thread on the earlier guardrail finding (
[-32098] machine not found: '<name>'), and theorbctl create DISTRO[:VERSION] MACHINE_NAMEargument order in the escape-hatch thread.@ -36,3 +8,1 @@orbctl statusorbctl list```Always clone `node-pnpm-base` unless the user requests a different distro or a clean environment without Node.js/pnpm — then create a fresh machine instead: `orbctl create DISTRO[:VERSION] MACHINE_NAME` (`orbctl create --help` lists the distros).No change, agreeing with your own weighting ("an agent can work that out"). The two claims that don't transfer to the create branch scope themselves by name: step 2 opens "The base image includes …" and the
bash -lcrationale opens "The base image setsPNPM_HOME…" — on a machine that is not a clone of the base image, neither sentence claims anything. Steps 3–5 (transfer semantics,/mnt/macpaths, delete) hold for any machine, andbash -lcitself is harmless on a fresh one. An explicit clone-only scoping line would restate what those sentences already say about themselves, and the escape hatch is the rare branch by construction ("unless the user requests …").@ -43,6 +13,8 @@ orbctl list`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails because `node-pnpm-base` does not exist (a `machine not found` error), stop and tell the user the base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.Accepted — fixed in
d89db50. The first cycle's live verification settled thatmachine not foundis today's verbatim substring ([-32098] machine not found: '<name>'), but a durable skill doc shouldn't lean on one release's wording, and a second reviewer independently flagging the string trigger is signal. Your framing also keeps everything the earlier rebuttal defended, which the first cycle's widen-to-any-failure proposal did not. The sentence now keys on the condition and demotes the literal to an example: "If the clone fails becausenode-pnpm-basedoes not exist (amachine not founderror), stop and tell the user the base VM is missing …".The scope deliberately stays missing-base-only, per the earlier thread: when OrbStack itself is down or unreachable, nothing can be cloned or created, so the substitution hazard this rule guards cannot arise there — those failures fail-and-report on their own.
@ -150,3 +95,1 @@- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failed- whether any failure was due to missing environment setup versus the code under test- any packages installed beyond the base imageReconsidered in full, since you are the second reviewer to raise this — but the cut still holds, and your "applied internally, never surfaced" case is covered more concretely than the first-cycle thread spelled out. It splits in two:
"Report portability issues separately" explicitly covers the remaining environment-shaped reporting distinction, so the restored bullet would duplicate what two rules and an existing bullet already force. The checklist also deliberately carries only unconditional, omission-prone state (packages, deletion); a bullet that applies only when a failure occurred is a different shape. Agreed on your parenthetical — pass/fail stays out for exactly the reason you gave. Not restoring.
Summary: Docs-only change to
skills/orbstack-ad-hoc-vm/SKILL.md. The new fail-fast contract is clear and well-scoped (triggering on the condition, withmachine not foundas an example, reads correctly). Found 3 issues, all from the deletions rather than the added contract: the fail-fast rule now covers only one of the clone-failure modes whose preflight check was deleted in the same diff, the newly documentedorbctl createbranch has no workflow behind it, and the report contract no longer surfaces the outcome the skill exists to produce.I could not verify the
orbctl create DISTRO[:VERSION] MACHINE_NAMEform ororbctl pushsemantics against upstream documentation —askpplxhas noPERPLEXITY_API_KEYin this environment andorbctlis not installed here — so no finding below asserts anything about OrbStack's actual CLI behavior. The commit message ford89db50states themachine not foundstring was live-verified.Code review by Claude Code Opus (opus)
@ -36,3 +8,1 @@orbctl statusorbctl list```Always clone `node-pnpm-base` unless the user requests a different distro or a clean environment without Node.js/pnpm — then create a fresh machine instead: `orbctl create DISTRO[:VERSION] MACHINE_NAME` (`orbctl create --help` lists the distros).🟡 Medium: This line introduces a second branch (
orbctl create) that the rest of the document does not support. Everything below assumes the base clone: section 1 is titled "Clone the base image", thebash -lcrequirement on line 26 is justified by the base image's~/.profilesettingPNPM_HOME, and section 2 opens with "The base image includesgit,curl,wget,jq, Node.js LTS, and pnpm" before showingpnpm add -g. An agent that takes the create branch — by definition because the user wants a machine without Node.js/pnpm — then follows sections 2–5 verbatim will runpnpm add -gon a machine that has no pnpm, and has no instruction on whether a freshly created machine needs starting or bootstrapping.The previous text mentioned the non-clone case without a command, so it read as an exception rather than a supported path; giving it a command makes it look supported. Either add a sentence stating that a freshly created machine has none of the base image's toolchain and that sections 2–3 must be adapted, or move the
orbctl createcommand into its own short subsection so the branch is visibly outside the base-clone workflow.@ -43,6 +13,8 @@ orbctl list`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails because `node-pnpm-base` does not exist (a `machine not found` error), stop and tell the user the base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.🟡 Medium: The stop rule covers one clone-failure mode, but this diff deleted the preflight that caught the others. The removed Preflight section ran
orbctl statusandorbctl listbefore cloning, which is what would surface "OrbStack is not running" or "orbctl is not installed". With that gone, those failures reach the agent as a failedorbctl clonewith no guidance attached — and the improvisation this PR is guarding against (substituting another machine, doing a fresh install) is exactly as tempting there as it is for a missing base.Safe fix: widen the trigger so the fallback is stopping, not improvising, e.g. "If the clone fails for any reason, stop and report it to the user rather than substituting another machine or improvising a fresh install. A
machine not founderror means the base VM is missing; the base's contents are a user-owned contract." That keeps the missing-base wording the PR wants while closing the gap the preflight deletion opened.@ -150,3 +95,1 @@- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failed- whether any failure was due to missing environment setup versus the code under test- any packages installed beyond the base image🟢 Low: The report contract lost the verification result. What remains is "any packages installed beyond the base image" and "whether the ad hoc VM was deleted" — both housekeeping — while the deleted bullets carried the outcome and the environment-vs-code classification.
The second one matters most: the "Distinguish environment failures from product failures" rule (line 79) is retained and tells the agent to fix the VM bootstrap first, but with its reporting bullet gone there is now no instruction to surface that distinction to the user. An agent can silently install a missing toolchain, get a green run, and report only "installed X, deleted the VM" — the user never learns the first attempt failed for environment reasons.
Suggest restoring one bullet:
- whether any failure was due to missing environment setup versus the code under test. (The PR body describes the deletions as the preflight, the duplicated rules, the generic verification bullets, the confirm-removal step, and the intro; this trim to "Reporting back" is not among them, so it may not have been intended.)Feedback for the
d89db50review cycle, processed at headed16abd:orbctl createbranch never rejoins the workflow): fixed ined16abd— a scoping clause on the escape-hatch sentence keys the numbered steps to the base clone; on a created machine only the transfer, run, and cleanup steps apply as written.@ -36,3 +8,1 @@orbctl statusorbctl list```Always clone `node-pnpm-base` unless the user requests a different distro or a clean environment without Node.js/pnpm — then create a fresh machine instead: `orbctl create DISTRO[:VERSION] MACHINE_NAME` (`orbctl create --help` lists the distros). The numbered steps assume the base clone; on a created machine only the transfer, run, and cleanup steps apply as written.Fixed in
ed16abd. The escape-hatch sentence now carries a scoping clause: "The numbered steps assume the base clone; on a created machine only the transfer, run, and cleanup steps apply as written." That is your first alternative in a sharper form — instead of saying sections 2–3 "must be adapted", it states which steps transfer at all, so an agent on the create path no longer runs section 2'spnpm add -gverbatim or trusts the base-image toolchain list, and knows the base-specificbash -lcrationale doesn't travel either. The subsection alternative would grow a second bootstrap workflow this skill deliberately does not carry — what a created machine needs varies by distro and by why the user declined the base, so the skill scopes the base-clone workflow rather than documenting per-distro setup.@ -43,6 +13,8 @@ orbctl list`node-pnpm-base` is a persistent Debian Trixie (arm64) VM with Node.js LTS and pnpm pre-installed. Cloning it is instant and uses copy-on-write storage; the clone starts at a few kilobytes.If the clone fails because `node-pnpm-base` does not exist (a `machine not found` error), stop and tell the user the base VM is missing rather than substituting another machine or improvising a fresh install — the base's contents are a user-owned contract.Held — third variant of this ask (43546 argued breadth in cycle 1, 43773 the trigger string in cycle 2; both closed and resolved). The two failure modes the deleted preflight caught do not carry the hazard this rule exists for, and that asymmetry is the design:
orbctl clone,orbctl list, andorbctl createall die on the same broken runtime, so the improvisation you name — substituting another machine, fresh-installing one — is not "exactly as tempting" there: it is impossible. No machine can come into existence to stand in for the base, so the agent stops because nothing can proceed, guidance or no guidance.Widening to "if the clone fails for any reason, stop and report" would also forbid legitimate remediation: a stopped daemon is the agent's own host environment, not the user's contract — launching OrbStack and retrying is the correct response, and a blanket stop-and-report converts a ten-second self-fix into a user round-trip. The rule stays scoped to the condition whose failure mode is silent substitution.
@ -150,3 +95,1 @@- base image used (`node-pnpm-base`) and any additional packages installed- whether the test passed or failed- whether any failure was due to missing environment setup versus the code under test- any packages installed beyond the base imageHeld — third raise (43547 in cycle 1, 43774 in cycle 2, both closed and resolved on this reasoning). Your sharpest scenario is the one the surviving contract already covers; final form of the position:
Position is final absent a maintainer overrule.
Summary: Found 1 medium issue in the fresh-machine workflow.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -36,3 +8,1 @@orbctl statusorbctl list```Always clone `node-pnpm-base` unless the user requests a different distro or a clean environment without Node.js/pnpm — then create a fresh machine instead: `orbctl create DISTRO[:VERSION] MACHINE_NAME` (`orbctl create --help` lists the distros). The numbered steps assume the base clone; on a created machine only the transfer, run, and cleanup steps apply as written.🟡 Medium: The created-machine branch is not executable for every distro it permits. OrbStack supports Alpine, whose default shell is
ash, but the transfer example later invokesbash -lc; this sentence also says to skip the dependency-install step that could install the missing shell or toolchain. A request for Alpine (or another minimal distro) therefore fails before the target command runs. Add a created-machine bootstrap step that selects the distro's available shell and package manager, or restrict this path to images that satisfy the later commands.