Skip to content

feat: insta build — pre-push verification (plan + Dockerfile + static checks) - #104

Merged
jwfing merged 3 commits into
mainfrom
feat/build-explain
Aug 19, 2026
Merged

feat: insta build — pre-push verification (plan + Dockerfile + static checks)#104
jwfing merged 3 commits into
mainfrom
feat/build-explain

Conversation

@jwfing

@jwfing jwfing commented Aug 19, 2026

Copy link
Copy Markdown
Member

What

insta build [dir] [--explain] [--port <n>] [--json] — verify a source directory would build before insta deploy pushes it to a remote Fly build. Entirely local and offline: no login, no project link, nothing deployed, no Docker daemon.

Output (human, or --json for agents):

  • detection plan — builder (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.example
  • the Dockerfile that would be used — the user's, or nixpacks-generated (nixpacks build --out into a temp dir; the source tree stays clean). --explain includes its content
  • static checks, each with a nextAction: 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_modules not dockerignored / >100MB)
  • verdict: deployable | needs-attention | failed (exit 1)

Why

Today the first signal that a deploy can't work arrives after the remote build — deploy.ts hard-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 as flyctl-build.ts; capture-only runner with a 30s wedge guard; ensureNixpacks() mirrors ensureFlyctl() (brew/curl best-effort, degrades to Dockerfile-only when unavailable). Handles real nixpacks ≥1.x plan output (providers empty, names in NIXPACKS_METADATA)
  • src/commands/build.ts — pure, unit-tested report assembly (buildReport/computeVerdict/inferPort/renderReport); reuses dockerfileExposedPort from deploy
  • Registered beside deploy in index.ts; SKILL.md architecture table updated

Testing

  • npm run typecheck clean; npm test 333/333 (22 new tests, TDD)
  • Live smoke with nixpacks 1.41.0: node app w/o Dockerfile → nixpacks plan + generated Dockerfile, verdict deployable; Dockerfile w/o CMD → needs-attention; undetectable dir → failed, exit 1

Follow-ups (not in this PR)

  • Phase 2: --check — local docker build + run + port probe (skips cleanly without a daemon)
  • Mirror row in skills/insta/cli-reference.md goes in a parallel insta-skills PR

🤖 Generated with Claude Code


Summary by cubic

Adds insta build [dir] to verify a source directory will build before insta 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.

  • Registers build with --explain, --port <n>, and --json. Exits 1 on a failed verdict. No login, no Docker, never pushes; rejects non-directory args. Validates --port to 1..65535 and rejects an explicit empty value.
  • Assembles a plan: builder (dockerfile or nixpacks), providers, install/build/start commands, port with rationale, and .env.example keys. --explain includes the Dockerfile content.
  • Integrates nixpacks without auto-install: quiet --version probe only; if unavailable, degrades to Dockerfile-only checks. Parses real nixpacks plan JSON (including NIXPACKS_METADATA) and generates a Dockerfile via nixpacks build --out into a temp dir.
  • Static checks with next actions: Dockerfile presence (critical), start command (CMD/ENTRYPOINT vs nixpacks start; missing is critical for nixpacks), port detection (flag wins, else EXPOSE), and build context size (ignores dockerignored node_modules; warns on large or truncated scans; truncation message counts entries).
  • Keeps --json output pure; Dockerfile content is omitted unless --explain. Adds unit tests for report assembly, JSON rendering, nixpacks glue, and context scanning.

Written for commit ffcd98a. Summary will update on new commits.

Review in cubic

… 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 jwfing left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — --port isn't validated; a non-numeric flag yields a silently-null port (src/commands/build.ts:49). inferPort does Number(flag) with no guard, so --port abcNaN; the port check then passes (NaN !== undefined), the human line prints NaN (--port flag), and JSON.stringify serializes the plan's port as null. This mirrors deploy.ts's Number(opts.port), so it's consistent — but since build is explicitly a verification tool whose whole job is to catch config mistakes before deploy, validating the flag (reject/die on Number.isNaN) would be a natural fit here.

  • Software engineering — test coverage gaps on the build() orchestration layer (test/build.test.ts). The pure helpers and buildReport are thoroughly covered (nice TDD), but build() itself has no tests: the die on a missing directory, the --json content-stripping (dockerfile omitted without --explain), the process.exitCode = 1 on failed, and the --json+ensureNixpacks interaction (the Critical above) are all untested. The context check's "too big" branch (totalBytes > CONTEXT_WARN_BYTES) and the WALK_CAP floor 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 established ensureFlyctl() 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 that build may reach the network to install nixpacks), 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/nixpacksGeneratedDockerfile spawn nixpacks with an args array (no shell), dir is never string-interpolated into a shell, and the only sh -c uses a static literal. .env.example handling extracts keys only, never values, so no secret leakage into output. No auth/authorization paths touched.
  • PerformancecontextStats does a synchronous recursive readdirSync/statSync walk, but it's a one-shot local CLI invocation (not an event-loop server) and is bounded by WALK_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 matches deploy.ts:67 (opts.port ? Number(opts.port) : 8080), and dockerfileExposedPort is 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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 6 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/nixpacks.ts Outdated
Comment thread src/commands/build.ts Outdated
Comment thread src/commands/build.ts
Comment thread src/nixpacks.ts Outdated
Comment thread src/commands/build.ts Outdated
Comment thread src/commands/build.ts Outdated
Comment thread src/commands/build.ts Outdated
…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>
@jwfing

jwfing commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Review round 1 addressed in dc072ed — all seven cubic findings plus the Critical:

  • Critical / --json purity + auto-install (also cubic P1, P2): ensureNixpacks() is gone. The command now runs a silent nixpacks --version probe (nixpacksAvailable(), 30s-guarded quiet runner — which also retires the timeout-less ok helper) and never installs anything: no curl | bash, no brew, no stdout chatter. When nixpacks is missing the report degrades to Dockerfile-only checks and the dockerfile check's nextAction carries the install hint. Verified on the fresh-machine path: with nixpacks stripped from PATH, insta build --json stdout parses as JSON (verdict failed, exit 1, zero bytes on stderr).
  • --port validation (cubic P1 + suggestion): inferPort rejects anything but an integer 1–65535 (--port abc/0/70000/80.5 → clean error via the command guard).
  • context size vs .dockerignore (cubic P2): a dockerignored node_modules is skipped during the walk, so totalBytes now sizes what actually ships. (Only the node_modules pattern is honored — full dockerignore glob semantics stay out of scope, noted in a comment.)
  • WALK_CAP (cubic P2): stats carry truncated; a capped scan now fails the context check conservatively ("size is a floor") instead of silently undercounting.
  • non-directory input (cubic P3): isDirectory() guard → no such directory.
  • coverage gaps: contextCheck and jsonReport extracted pure and tested — JSON content-stripping with/without --explain, JSON purity on the nixpacks-missing path, tooBig and truncated branches. 343/343, typecheck clean.

PR description updated to drop the auto-install claim; the skills mirror row (InsForge/insta-skills#42) is updated to match.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/commands/build.ts Outdated
Comment thread src/commands/build.ts Outdated
…s entries, not files

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jwfing
jwfing enabled auto-merge August 19, 2026 04:35

@Fermionic-Lyu Fermionic-Lyu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Approved.

@jwfing
jwfing merged commit 9c96f0c into main Aug 19, 2026
2 checks passed
@jwfing jwfing mentioned this pull request Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants