fix(init-qm): only write a Dockerfile where one can actually run - #67
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
a2b13b0 to
9715b51
Compare
|
All three are right — fixed in 9715b51. Also rebased onto the current #64, so this now includes 1. Rather than re-key one string, the skip now returns a reason instead of a boolean, so the message is derived from the same value that made the decision and can't drift from it again: Verified both end to end. 2. Re-run doesn't clean up. Right, and this is the case that actually bites — everyone who already ran 2.3.1 has that file. Re-running now prints an 3. Tests only covered helpers. Added the shapes that ship — On trailing commas I went further than documenting it. Fail-open there writes the exact file we're trying to avoid, so a stray comma would have silently defeated the change on a Sprites deployment. Framing. Fair point — the README now says this reflects current QM and that qm#272 being fixed reverses it, and the code comment says the same. 95 tests pass, build and lint clean. On merge order — agreed, and to be explicit: #64 first, then this, then rebase #66 so 2.3.2 pins this code. Publishing #66 first would put 2.3.2 on npm with 2.3.1's |
The merge-base changed after approval.
Scaffolding wrote sandbox/Dockerfile unconditionally. On Fly Sprites and AWS MicroVM sandboxes that file never executes — Sprites boot the stock image and ignore a published one (yc-software/qm#272), and AWS MicroVM has no install mechanism at all (#350). A Dockerfile that cannot run is worse than none. It reads as the supported install path, so when the binary turns out to be missing the operator debugs the file rather than learning the image was never used. Two upstream PRs (qm#427, qm#379) would turn this from useless into a hard validation failure, so a scaffold emitting the rejected file would fail its own `qm check`. init-qm now reads target and sandbox.backend from the operator's existing qm.config.jsonc and writes the Dockerfile only where a custom image boots. An absent or unparseable config scaffolds exactly as before: guessing wrong in that direction removes a file someone needs. tool.json is still written everywhere. Beyond `install`, it carries `egress`, `auth`, and `approvals` — none of which need anything materialized, all of which are enforced today, and which have no skill-side equivalent. Dropping those preemptively would trade working security controls for tidiness against an upstream change that has not merged and that we have asked to narrow. Comes with a JSONC comment stripper rather than a regex, because the naive version eats the second slash of "https://" and turns a valid config into a parse error in a file the operator did not know we read. Docs updated to match: the bundle README no longer lists the Dockerfile as unconditional, the operator guide explains why Sprites skips it, and the air-gapped section now covers the first-run install, which is the path that actually reaches npm on Sprites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015PnyXXBMwhMj5CtcZRJoJ2
… file Three corrections from review. An AWS control plane running Sprites sandboxes was described as Lambda MicroVM, which told the operator the CLI could never be installed there. That is false — Sprites install it on first use. `backend` decides where sandboxes run, not `target`: the CLI requires `"backend": "aws"` to have `target: "aws"` but not the reverse (config.js:1106-1112), so the two are different deployments. The skip now carries a reason rather than a boolean, so the message cannot describe a deployment as something it is not. Skipping the file does nothing for anyone who already ran the previous version: they still have a Dockerfile that upcoming QM validation rejects. Re-running now says so and what to do. It is not deleted for them — it is their file and may carry their own build steps. Trailing commas made the config unparseable, which fell open and wrote the Dockerfile we were trying not to write. JSONC allows them and hand-edited files collect them, so they are now handled rather than documented as a hole. Fail-open remains for genuinely unreadable configs. Tests now cover the shapes that ship rather than only the helpers: target docker with the sprites backend (what our own guide documents), an AWS target with Sprites sandboxes, and real MicroVM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015PnyXXBMwhMj5CtcZRJoJ2
The merge-base changed after approval.
ad10639 to
d65c2df
Compare
The problem
pipeshub init-qmwrote asandbox/Dockerfileinto every deployment. On Fly Sprites and AWS MicroVM sandboxes that file never runs — Sprites boot the stock image and ignore a published one (qm#272), and AWS MicroVM has no way to install a binary at all (qm#350).A Dockerfile that cannot run is worse than no Dockerfile. It reads as the install path, so when someone's agent reports
pipeshub: command not found, they open the Dockerfile and start debugging a file that was never used.There's a deadline on this too: two upstream PRs (qm#427, qm#379) turn that file from useless into a hard validation failure. Today our scaffold produces a deployment that limps; after those land it produces one that won't pass
qm check— and the error names our files, not the reason.What changed
init-qmnow readstargetandsandbox.backendfrom the operator's existingqm.config.jsoncand writes the Dockerfile only where a custom image actually boots.target: flyThat last row is deliberate. Guessing wrong in that direction deletes a file someone needs, so anything we can't classify keeps the old behaviour.
What the operator now sees on Sprites:
What did NOT change, on purpose
tool.jsonis still written everywhere, including AWS.Both upstream PRs reject custom tools, not just Dockerfiles — so a literal reading says we should stop writing it. I didn't, because a tool descriptor carries more than
install:egress— the only hostname the agent may reachauth.check/auth.reauth— how QM tests whether someone's credential still worksapprovals—denyon the two subcommands that could take a credential on a command line, andrequire_approvalbefore pulling a full document into the transcriptNone of those need anything materialized, all of them are enforced today, and none has a skill-side equivalent — I checked the contract types. Dropping them now would trade working security controls for tidiness against a change that hasn't merged and that we've asked upstream to narrow (comment).
If upstream keeps the wholesale rejection, that's the point to revisit — and the answer then is to find those fields a new home, not to ship without them.
A small thing worth mentioning
Reading the config needed JSONC comment-stripping. The naive version eats the second slash of
"https://..."and turns a valid config into a parse error — in a file the operator doesn't know we read. There's a test for exactly that.Docs
Checked
target: docker+backend: spritesdeployment, a plain docker one, and an empty directory — and confirmed the right files appear in each