feat: insta build — pre-push verification (plan + Dockerfile + static checks) - #104
Conversation
… checks) Agents (and humans) currently learn a deploy would fail only after pushing a remote Fly build. `insta build [dir]` answers it locally and offline, with no login: the detection plan (builder, install/build/start commands, port with rationale, .env.example keys), the Dockerfile that would be used (the user's, or nixpacks-generated via `nixpacks build --out` — static, no Docker daemon), and checks that each carry a next action. --json for agents, --explain to include the Dockerfile content, exit 1 on a failed verdict. nixpacks is auto-installed best-effort (mirrors ensureFlyctl); without it the command degrades to Dockerfile-only verification. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwfing
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (posted as a comment — GitHub blocks a formal request-changes review on your own PR). One Critical finding below should be resolved before merge.
Summary
insta build adds a well-tested, cleanly-factored local pre-push verification command (detection plan + generated Dockerfile + static checks + verdict); the one blocker is that the --json output — the flagship agent contract — gets corrupted on the fresh-machine path because human progress lines are written to stdout before the JSON.
Requirements context
No matching spec/plan found — the repo has no docs/superpowers/ and no docs/specs/ entry for this feature (only the developing-insta-cli SKILL.md architecture table, which this PR updates). Assessed against the PR description/title alone. The implementation faithfully matches the described scope (local, offline, static-only Phase 1; --explain/--port/--json; deployable/needs-attention/failed+exit 1), and scope discipline is good — no drive-by changes, dockerfileExposedPort is reused rather than duplicated, and nixpacks.ts mirrors the existing flyctl-build.ts injectable-runner pattern.
Findings
Critical
Functionality — --json output is corrupted on the no-Dockerfile + nixpacks-absent path (src/commands/build.ts:238-242, src/nixpacks.ts:84,88,93, src/util.ts:26-28)
build() calls await ensureNixpacks() (line 238) before the printJson(...) branch (line 242). ensureNixpacks() emits human-facing progress via info() (nixpacks.ts:84/88/93), and info() writes to stdout (util.ts:26-28). On the install path it also runs curl … | bash / brew install with stdio:'inherit', teeing installer output to stdout too. So when a directory has no Dockerfile and nixpacks isn't already present — exactly the fresh-machine / CI / agent bootstrap case the PR says it's built for ("Agents especially need a cheap, structured pre-push probe insta build --explain --json") — stdout becomes [info lines][installer output][JSON], and JSON.parse(stdout) fails. Note this fires even without a successful install: the final fallback info('nixpacks not found — install it …') (nixpacks.ts:93) still lands on stdout in --json mode. Suggested fix: in JSON mode suppress these, or route progress/install chatter to stderr so stdout carries only the JSON document. No test exercises build() in --json mode with nixpacksAvailable=false, which is why this slipped through.
Suggestion
-
Functionality —
--portisn't validated; a non-numeric flag yields a silently-null port (src/commands/build.ts:49).inferPortdoesNumber(flag)with no guard, so--port abc→NaN; the port check then passes (NaN !== undefined), the human line printsNaN (--port flag), andJSON.stringifyserializes the plan'sportasnull. This mirrorsdeploy.ts'sNumber(opts.port), so it's consistent — but sincebuildis explicitly a verification tool whose whole job is to catch config mistakes before deploy, validating the flag (reject/dieonNumber.isNaN) would be a natural fit here. -
Software engineering — test coverage gaps on the
build()orchestration layer (test/build.test.ts). The pure helpers andbuildReportare thoroughly covered (nice TDD), butbuild()itself has no tests: thedieon a missing directory, the--jsoncontent-stripping (dockerfile omitted without--explain), theprocess.exitCode = 1onfailed, and the--json+ensureNixpacksinteraction (the Critical above) are all untested. Thecontextcheck's "too big" branch (totalBytes > CONTEXT_WARN_BYTES) and theWALK_CAPfloor are also only covered on the node_modules path. -
Security/functionality — a "verify" command auto-runs
curl -fsSL … | bash(src/nixpacks.ts:88-91). Triggering a remote install script from a command the PR describes as "entirely local and offline" is surprising, especially in unattended CI/agent contexts. It does mirror the establishedensureFlyctl()pattern (flyctl-build.ts:88-102) and degrades gracefully, so it's not a regression — but consider gating auto-install behind an opt-in flag/env (or at least noting thatbuildmay reach the network to installnixpacks), since it slightly contradicts the "offline" framing. The install source (nixpacks.com) is reputable and no user input is interpolated into the shell string, so there's no injection concern.
Information
- Security — no injection surface:
nixpacksPlan/nixpacksGeneratedDockerfilespawnnixpackswith an args array (no shell),diris never string-interpolated into a shell, and the onlysh -cuses a static literal..env.examplehandling extracts keys only, never values, so no secret leakage into output. No auth/authorization paths touched. - Performance —
contextStatsdoes a synchronous recursivereaddirSync/statSyncwalk, but it's a one-shot local CLI invocation (not an event-loop server) and is bounded byWALK_CAP(50k entries), with the result reported as a floor beyond that. Acceptable. - Correctness (positive) — the
portRationale: 'not detected — deploy defaults to 8080'string accurately matchesdeploy.ts:67(opts.port ? Number(opts.port) : 8080), anddockerfileExposedPortis reused from deploy rather than reimplemented — good consistency.
Verdict
request_changes — one Critical (--json stdout corruption on the fresh-machine path) blocks merge. Everything else is non-blocking; the structure, reuse, and test discipline on the pure layer are solid.
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ll it The verifier no longer auto-installs nixpacks (no curl|bash / brew from a command advertised as local and offline, and no stdout chatter corrupting the --json agent contract on the fresh-machine path): nixpacksAvailable() is a silent --version probe and the report's nextAction carries the install hint. Also from review: --port validated as an integer in 1..65535, dockerignored node_modules no longer counted in the context size, the walk cap marks the stats truncated (conservative warn instead of a silent undercount), and a non-directory argument dies cleanly. contextCheck/jsonReport extracted pure and covered (10 new tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round 1 addressed in dc072ed — all seven cubic findings plus the Critical:
PR description updated to drop the auto-install claim; the skills mirror row (InsForge/insta-skills#42) is updated to match. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…s entries, not files Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
insta build [dir] [--explain] [--port <n>] [--json]— verify a source directory would build beforeinsta deploypushes it to a remote Fly build. Entirely local and offline: no login, no project link, nothing deployed, no Docker daemon.Output (human, or
--jsonfor agents):dockerfile|nixpacks), providers, install/build/start commands, port with the reason it was chosen (--port flag/Dockerfile EXPOSE n/ not detected), env keys from.env.examplenixpacks build --outinto a temp dir; the source tree stays clean).--explainincludes its contentnextAction: Dockerfile present, start command (CMD/ENTRYPOINT or nixpacks start), port detected (the CLI v0.1: auth, projects, branches, secrets, deploy, governance, observe #1 deploy mistake per deploy.ts), build context (node_modulesnot dockerignored / >100MB)deployable|needs-attention|failed(exit 1)Why
Today the first signal that a deploy can't work arrives after the remote build —
deploy.tshard-fails without a Dockerfile and a port mismatch surfaces as a dead app. Agents especially need a cheap, structured pre-push probe (insta build --explain --json).How
src/nixpacks.ts— nixpacks glue, same injectable-runner pattern asflyctl-build.ts; capture-only runner with a 30s wedge guard;ensureNixpacks()mirrorsensureFlyctl()(brew/curl best-effort, degrades to Dockerfile-only when unavailable). Handles real nixpacks ≥1.x plan output (providersempty, names inNIXPACKS_METADATA)src/commands/build.ts— pure, unit-tested report assembly (buildReport/computeVerdict/inferPort/renderReport); reusesdockerfileExposedPortfrom deploydeployinindex.ts; SKILL.md architecture table updatedTesting
npm run typecheckclean;npm test333/333 (22 new tests, TDD)Follow-ups (not in this PR)
--check— localdocker build+ run + port probe (skips cleanly without a daemon)skills/insta/cli-reference.mdgoes in a parallel insta-skills PR🤖 Generated with Claude Code
Summary by cubic
Adds
insta build [dir]to verify a source directory will build beforeinsta deploy. Previously failures surfaced after a remote Fly build; now you get a local, offline plan, optional generated Dockerfile, static checks, and a verdict that can block deploys.buildwith--explain,--port <n>, and--json. Exits 1 on a failed verdict. No login, no Docker, never pushes; rejects non-directory args. Validates--portto 1..65535 and rejects an explicit empty value.dockerfileornixpacks), providers, install/build/start commands, port with rationale, and.env.examplekeys.--explainincludes the Dockerfile content.nixpackswithout auto-install: quiet--versionprobe only; if unavailable, degrades to Dockerfile-only checks. Parses realnixpacks planJSON (includingNIXPACKS_METADATA) and generates a Dockerfile vianixpacks build --outinto a temp dir.nixpacksstart; missing is critical fornixpacks), port detection (flag wins, else EXPOSE), and build context size (ignores dockerignorednode_modules; warns on large or truncated scans; truncation message counts entries).--jsonoutput pure; Dockerfile content is omitted unless--explain. Adds unit tests for report assembly, JSON rendering,nixpacksglue, and context scanning.Written for commit ffcd98a. Summary will update on new commits.