Skip to content

Add --format json to CLI commands - #388

Open
dhruv8sh wants to merge 9 commits into
mainfrom
spec/383-cli-format-json
Open

dhruv8sh wants to merge 9 commits into
mainfrom
spec/383-cli-format-json

Conversation

@dhruv8sh

Copy link
Copy Markdown

Summary

  • CI scripts that drive EdgeZero have had to scrape key=value log lines that were never a stable contract. Nine commands now accept --format json: active-version, auth status, build, config gc, config validate, deploy, healthcheck, provision and rollback. Each one writes a single versioned envelope, { schema_version, command, ok, result, error }, to stdout, on success and on failure.
  • stdout stays clean because, under JSON, logs and every child process's stdout (cargo, fastly, wrangler, spin, manifest commands) go to stderr. A new clippy disallowed-methods lint means future code can't bypass this. --format text is the default, and its output is byte-identical to main.
  • Follows the spec that Add --json output across the CLI #383 requires, included in this PR as docs/superpowers/specs/2026-09-25-cli-format-json-design.md. config diff --format json is unchanged.

Changes

Crate / File Change
edgezero-adapter (registry.rs) Adapter::execute returns ActionOutcome, provision returns ProvisionReport, gc_config_entries returns GcReport, instead of () or prose lines. A negative result that was still measured (unhealthy probe, unauthenticated session, partly failed gc) is an Ok outcome carrying a failure message.
edgezero-adapter (process.rs, new) Process-wide child-stdout policy plus process::status, the one sanctioned inheriting spawn.
edgezero-adapter (cli_support.rs) native_auth_status. run_native_cli goes through process::status.
edgezero-adapter-{fastly,cloudflare,spin,axum} Return typed outcomes and reports. Fastly no longer prints the version= / healthy= / status-code= / rolled-back-to= data lines; the CLI prints the same bytes. All inheriting spawns go through process::status.
edgezero-cli (output.rs, new) OutputScope (routes stdout while alive), Failure / Outcome, the envelope, and the serde wire schema, kept separate from the adapter types so internal refactors can't change the JSON.
edgezero-cli (args.rs) New OutputFormat { Text, Json } and a --format flag on the nine commands. DiffFormat is unchanged.
edgezero-cli (lib.rs, auth.rs, provision.rs, config.rs, adapter.rs) Each run_* keeps its public signature and emits the envelope itself, so CLIs already generated from the template get JSON without regenerating main.rs. The logger's info output moves to stderr under JSON.
clippy.toml disallowed-methods for Command::status / Command::spawn. Piped spawns carry a documented #[expect].
edgezero-cli/tests/format_json.rs (new) End-to-end tests against the real binary.
docs/guide/cli-reference.md --format on each command, plus a new "Machine-readable output" section covering the envelope, streams, exit codes, compatibility policy, per-command schemas and changelog.
docs/superpowers/specs/…-cli-format-json-design.md The spec, revision 2 (§13 notes what changed during implementation).

Closes

Closes #383

Test plan

  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • WASM builds: wasm32-wasip1 (Fastly) / wasm32-wasip2 (Spin) / wasm32-unknown-unknown (Cloudflare), via the full format.yml wasm clippy matrix and cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
  • examples/app-demo workspace: cd examples/app-demo && cargo test --workspace --all-targets (--locked), plus its fmt and clippy
  • Docs build: cd docs && npm run lint && npm run format && npm run build
  • Manual testing via edgezero serve --adapter axum (not applicable: serve is out of scope)
  • Other:
    • Text mode compared byte-for-byte against main: both binaries ran 21 hermetic scenarios (success and failure paths for every in-scope command, a usage error, the bundled stub), with stdout, stderr and exit codes identical in all of them.
    • cargo test -p edgezero-cli --test generated_project_builds -- --ignored, cargo test -p edgezero-adapter-fastly --features cli, and the check_no_nested_app_config steps.
    • All checks were run on the pinned toolchain 1.95.0.

Checklist

  • Changes follow CLAUDE.md conventions
  • No Tokio deps added to core or adapter crates
  • Route params use {id} syntax (not :id)
  • Types imported from edgezero_core (not http crate)
  • Store wiring goes through KvRegistry / ConfigRegistry / SecretRegistry (not the legacy single-handle setters) — see spec §6.6
  • New code has tests
  • No secrets or credentials committed

Spec for #383: settles the open questions (shared OutputFormat enum, `text` default, stdout/stderr discipline, error envelopes, schema versioning, bundled stubs) and defines the per-command JSON schema. Also corrects the docs/.prettierignore comment: design docs are tracked in git.
active-version, auth status, build, config gc, config validate, deploy, healthcheck, provision and rollback accept --format text|json. JSON mode writes one versioned envelope to stdout and routes logs and child-process stdout to stderr; text mode is byte-identical to before.

Adapters now return typed outcomes (ActionOutcome, ProvisionReport, GcReport) instead of printing their results, and every inheriting child spawn goes through edgezero_adapter::process::status, enforced by a clippy disallowed-methods lint.

Closes #383
@dhruv8sh dhruv8sh self-assigned this Sep 25, 2026

@aram356 aram356 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR Review

Summary

Adds --format json to nine commands: one versioned envelope on stdout, with logs and child stdout moved to stderr, and typed adapter outcomes instead of printed lines. The design holds up, and the text path is unchanged (see CI Status). Three JSON results are wrong and should be fixed before schema v1 ships: version_verified on an unhealthy probe, build silently dropping a trailing --format json, and config gc --yes reporting deleted: null when there was nothing to reclaim.

Findings

Blocking

  • 🔧 version_verified: true on an unhealthy probe: the after-probe check only runs when the probe is healthy (crates/edgezero-adapter-fastly/src/cli.rs:5641)
  • 🔧 build swallows a trailing --format json: exits 0 with text on stdout (crates/edgezero-cli/src/args.rs:251)
  • 🔧 config gc --yes with nothing to reclaim reports deleted: null: the docs say null means a dry run (crates/edgezero-adapter-fastly/src/cli.rs:2497)
  • ❓ Spec approval: the spec header says "Status: Approved", and #383 asks for the spec to be approved before implementation starts. I couldn't find the approval on #383 or on this PR (the spec and the implementation arrived in the same push). Was it approved somewhere else? If so, a link in the spec header would help.

Non-blocking

  • 🤔 Raw config validate reports app_config: null although it reads and requires that file (crates/edgezero-cli/src/config.rs:235)
  • 🤔 Provision dry-run actions don't pair with real-run actions (crates/edgezero-adapter-spin/src/cli.rs:249, crates/edgezero-adapter-fastly/src/cli.rs:522)
  • 🤔 A deploy that went live but couldn't resolve its version reports result: null (crates/edgezero-cli/src/lib.rs:382)
  • 🤔 deploy.service_id reflects only the flag (crates/edgezero-cli/src/lib.rs:216)
  • ♻️ Outcome types can hold contradictory values (crates/edgezero-adapter/src/registry.rs:116)
  • ⛏ Small items: the AuthSub::Status variant (args.rs:237), OutputFormat compared with == (output.rs:75), a leftover bare block (edgezero-adapter-fastly/src/cli.rs:3958), and an unreachable --require-active promise in the spec (spec L152)
  • 🤔 Docs: nullability and edge cases
    • The compatibility policy says making a non-null key nullable is breaking, but the Results section lists key names only, so readers can't tell which keys may be null. Spec §10 called for one schema table per command. Nullable today: active-version.version, build.artifact, deploy.service_id, deploy.version, healthcheck.staging_ip, healthcheck.status_code, rollback.rolled_back_to, provision.entries[].store, provision.entries[].store.logical, config gc older_than_secs / deleted / store.id, and config validate.app_config.
    • Exit code 2 means three things in a generated CLI: a command failure, a clap usage error, and an unsupported config diff. The "empty stdout means no envelope" rule in Streams is what tells them apart; worth repeating next to Exit codes.
    • --format json --help (and --version) print clap's text to stdout and exit 0, which contradicts "stdout holds exactly one JSON document".
    • JSON routing depends on init_cli_logger() being the installed logger. A downstream CLI that installs its own logger (e.g. simple_logger, which writes every level to stdout) would put log lines on stdout. One sentence in the docs would cover it.
  • 🤔 Test gaps
    • No test produces version_verified: true (the existing healthcheck test removes the token).
    • The config gc mapping from failure_diagnostic to a partial result (config.rs:467-469), and how failed / stranded / uncertain / deleted are filled, have no JSON-level test.
    • deploy, rollback and active-version have no tests of their result mapping, or of the version= / rolled-back-to= / "no active version yet" lines that moved from the adapter into the CLI.
    • config validate has no failure (null result) or typed-mode test.
    • run_shell_tee (the deploy capture path) isn't exercised under JSON.
    • The end-to-end tests cover 5 of the 9 commands, and the 21-scenario byte comparison from the PR description isn't committed. Even a reduced version committed as a test would keep the text contract from drifting.
  • 🌱 Lint gaps: disallowed-methods fires on every status / spawn form I tried (checked in a scratch crate using this clippy.toml). It does not catch .stdout(Stdio::inherit()).output() (which really does leak at runtime), CommandExt::exec, or writeln!(io::stdout(), …) (print_stdout only covers the macros). None of these is used today. Adding std::io::stdout to the list, with #[expect] at the three existing call sites (output.rs:537, adapter.rs:431, config.rs:1370), would close the last gap. The generated project's templates/root/clippy.toml.hbs doesn't carry the lint, so a downstream CLI's own subcommands are unguarded.
  • 🌱 A closed stderr loses the envelope: CliLogger writes routed info with eprintln!, which panics on a broken pipe. Under JSON all info goes to stderr, so if the stderr reader exits first, the command panics at its first log line (exit 101) and writes no envelope. Text mode has the same failure on stdout, so this isn't new, but JSON makes stderr the busy stream. let _ = writeln!(io::stderr(), "{}", record.args()); avoids it.

📌 Out of Scope

  • wrangler whoami exit code when logged out (unverified; wrangler wasn't available to test): some wrangler versions print "You are not authenticated" and still exit 0. If current ones do, auth status --adapter cloudflare --format json reports state: authenticated. Text mode already exited 0 in that case, so this predates the PR, but the JSON value now states it outright. Worth checking, and if so parsing the whoami output rather than trusting the exit code.

CI Status

  • fmt: PASS
  • clippy: PASS
  • tests: PASS (cargo test --workspace --all-targets: 1459 passed, 0 failed)
  • cargo check --workspace --all-targets --features "fastly cloudflare spin": PASS
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASS
  • cargo test -p edgezero-adapter-fastly --features cli: PASS (288 passed)
  • Text mode vs main: 130 paired hermetic runs across the 9 commands (fake curl / fastly / wrangler / spin / cargo) gave byte-identical stdout, stderr and exit codes. The only difference is the usage-error text for --format yaml.

Comment thread crates/edgezero-adapter-fastly/src/cli.rs Outdated
Comment thread crates/edgezero-cli/src/args.rs
Comment thread crates/edgezero-adapter-fastly/src/cli.rs Outdated
Comment thread crates/edgezero-cli/src/config.rs Outdated
Comment thread crates/edgezero-adapter-spin/src/cli.rs Outdated
Comment thread crates/edgezero-adapter/src/registry.rs
Comment thread crates/edgezero-cli/src/args.rs
Comment thread crates/edgezero-cli/src/output.rs Outdated
Comment thread crates/edgezero-adapter-fastly/src/cli.rs Outdated
Comment thread docs/superpowers/specs/2026-09-25-cli-format-json-design.md Outdated
Revision 3 records the review's contract fixes: version_verified only when both active checks ran, config gc deleted: 0 for an empty real run, build rejecting a late --format, config validate always reporting app_config, deploy keeping its result and resolved service id, and Spin reporting created. Drops the unreachable active-version --require-active promise and documents --help/--version and the exit-code-2 overlap.
Maps each review comment to the change that addresses it, with the tests and verification that back it.
- healthcheck: version_verified is true only when the after-probe check ran
- config gc: a real run with nothing to reclaim reports deleted: 0, not null
- build: reject a --format that follows passthrough args instead of forwarding it
- config validate: always report app_config; the mode is passed explicitly
- deploy: report the adapter-resolved service id, and keep the result when the deploy went live but its version could not be resolved
- spin provision: an added label reports created, pairing with would_create
- Outcome types store each fact once: HealthcheckOutcome::healthy(), AuthState::Unauthenticated { reason }, GcFailure
- Lint direct std::io::stdout writers (also in the generated project's clippy.toml) and never panic the logger on a closed stderr
- Exhaustive OutputFormat matches, #[non_exhaustive] AuthSub::Status, flatten a leftover block
- Docs: per-command result tables with nullability, --help/--version, exit code 2, logger dependency, provision action pairing
- Tests: token healthcheck, gc report fields, deploy tee and partial result, active-version, rollback, validate failure, late --format
@dhruv8sh
dhruv8sh marked this pull request as ready for review October 5, 2026 09:15

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR Review (re-review at cc6f1ac)

Summary

The fix commit addresses aram356's blocking items, and the new tests fail against the old code: version_verified only when both checks ran, deleted: 0 on an empty real gc run, and the late---format guard. Almost every non-blocking item is closed too.

I also built this branch and main and ran them with fake fastly / curl / wrangler / spin / cargo:

  • Under --format json, stdout held exactly one valid envelope in every scenario tried. That covered all 9 commands on success and failure paths, more than 1 MB of output from a child process, a killed child, RUST_LOG=trace, a closed stderr, and Unicode/escaping in paths and messages.
  • In text mode, 49 of 54 scenarios were byte-identical to main. All 5 differences come from the new flag, and one is the build -- --format case below.

What remains is the unanswered spec-approval question, two doc errors in the output contract, and some test gaps.

😃 Praise

  • One gate for child stdout: every child process that inherits our stdout goes through edgezero_adapter::process::status, the disallowed-methods lint enforces it, and the exceptions carry #[expect] with reasons. "stdout stays clean" can't quietly regress.
  • Routing can't get stuck: OutputScope saves and restores the routing flags in Drop, so a ?, an early return or a panic always restores routing.
  • Each fact stored once: AuthState::Unauthenticated { reason }, HealthcheckOutcome::healthy() and GcFailure rule out the contradictory states.
  • Tests that check behaviour: the fake-curl API call log proves both version checks ran, rather than just trusting the flag. sole_json_document and assert_envelope_invariants make the one-envelope rule hard to break.
  • Docs match the code: the per-command result tables match the serde wire structs field by field, including nullability and enum spellings.

Findings

Blocking

  • ❓ Spec approval still unanswered (spec:4)
  • 🔧 Docs promise a --version flag that doesn't exist (cli-reference.md:662)
  • 🔧 Empty-stdout list misses the late---format rejection, which contradicts the exit-1 guarantee (cli-reference.md:657)

Non-blocking

  • 🤔 build rejects -- --format json, deploy forwards it. This also changes text mode for build (lib.rs:176).
  • 🤔 auth status decides authenticated from the exit code alone. Real wrangler exits 0 when not logged in (cli_support.rs:112).
  • 🤔 A broken stdout pipe hides the command's real error (output.rs:545).
  • 🤔 The plan doc's status is wrong, and it includes a line specific to one machine (plan:5).
  • 🤔 Spec §7.2 types and §9.3 test list are stale (spec:331).
  • 🤔 OutputScope doesn't support overlapping runs on separate threads. It needs a doc note (output.rs:68).
  • 🤔 Test gaps:
    • config gc is the only one of the 9 commands with no end-to-end test. The CLI-side mapping (dry_run = args.dry_run || !args.yes, older_than_secs, and not printing text_lines on failure) is untested. The fake_fastly_gc helper in the Fastly adapter tests could be reused.
    • auth status has end-to-end tests only for the unauthenticated case. There's no test of the authenticated path or of a missing native CLI (result: null per the docs).
    • Staging deploy / rollback aren't tested. That includes the version= line that moved from the adapter to the CLI, and the resolved service_id (flag, then FASTLY_SERVICE_ID).
    • The generated project's new clippy.toml.hbs lines aren't asserted. The generator test only checks allow-expect-in-tests, so adding assert!(clippy.contains("std::io::stdout")) would cover it.
    • The text-mode checks for build, rollback and deploy use starts_with / ends_with, so lines added in the middle would pass. auth, provision, validate and gc have no text-mode check. The byte comparison against main from spec §9.2 is still not committed.
    • The test harness only clears FASTLY_API_TOKEN / FASTLY_SERVICE_ID. An exported EDGEZERO__* or app-overlay variable on the machine running the tests can change provision and validate results.
  • ♻️ Fastly staged deploy works out the service id a second time with .ok(). The production Deploy returns Deploy{None,None} while other adapters return Empty (fastly cli.rs:434).
  • ♻️ Result<Result<u64, String>, String> would read better as a small enum. deploy() is about 140 lines (lib.rs:368).
  • ⛏ Small items:
    • healthcheck reports status_code: 0 when curl prints 000. The docs say null when no probe got a response.
    • A production Fastly deploy with only FASTLY_SERVICE_ID set reports service_id: null, although fastly used that id. Worth one sentence in the deploy table.
    • The #[expect] reason at config.rs:1374 says "config push has no --format", but config diff calls the same function.
    • GcResult::new clones the whole GcFailure, diagnostic string included, just to read three lists. Destructure it by reference instead.
    • auth status --help describes --format with shorter wording than the other eight commands (args.rs:236).
    • The process tests restore the global setting by hand. If expect("spawn") panics, the setting stays true and POLICY_LOCK is poisoned. Use a drop guard.
    • native_auth_status_maps_exit_status_to_state runs true / false but isn't #[cfg(unix)], unlike the other fake-binary tests.
  • 🌱 Lint gaps:
    • disallowed-methods doesn't catch std::os::unix::process::CommandExt::exec or .stdout(Stdio::inherit()).output(). Neither is used today.
    • The public outcome structs (HealthcheckOutcome, GcReport, RollbackOutcome, ProvisionEntry, …) aren't #[non_exhaustive], so adding a field later breaks adapters built outside this repo.

📌 Out of Scope

These all predate this PR. Worth tracking as follow-ups:

  • provision failing partway: the result is null and neither stream says what was already created.
  • Deploy output parsing: in the tee, a non-UTF-8 line stops the version parsing, so a later SUCCESS … version 12 line is never seen. With more than 64 KB after the bad byte, the child gets SIGPIPE.
  • Built-in Fastly production deploy: this path doesn't capture fastly compute deploy's output. Without a token it fails with "no version=<N> line in the deploy output" even when Fastly printed the version.
  • Empty token: FASTLY_API_TOKEN= (empty) is treated as set.
  • Rollback to the same version: rollback --version 7 --rollback-to 7 is accepted.

CI Status

  • fmt: PASS
  • clippy (--workspace --all-targets --all-features -D warnings): PASS
  • tests (cargo test --workspace --all-targets): PASS, 0 failures
  • cargo check --workspace --all-targets --features "fastly cloudflare spin": PASS
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASS
  • Black-box JSON contract: every scenario tried produced exactly one valid envelope. Text mode vs main: 49/54 byte-identical; the 5 differences are expected, and one is the build -- --format case above.

Comment thread docs/superpowers/specs/2026-09-25-cli-format-json-design.md Outdated
Comment thread docs/guide/cli-reference.md Outdated
Comment thread docs/guide/cli-reference.md
Comment thread crates/edgezero-cli/src/lib.rs
Comment thread crates/edgezero-adapter/src/cli_support.rs
Comment thread docs/superpowers/plans/2026-10-05-pr-388-review-fixes.md Outdated
Comment thread docs/superpowers/specs/2026-09-25-cli-format-json-design.md Outdated
Comment thread crates/edgezero-cli/src/output.rs
Comment thread crates/edgezero-adapter-fastly/src/cli.rs Outdated
Comment thread crates/edgezero-cli/src/lib.rs Outdated

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary

Reviewed all 24 changed files and traced adapter outcomes, CLI consumers, subprocess routing, and Fastly action parsers.

Head: cc6f1ac99aa09de8025bc26847e424dd7b02fba9
Base: 98930917d96cb665c7255f36ca8bbd61fc7539e1

Safety proof

  1. JSON remains isolated from logs and child stdout on tested paths. Executed and proven for covered cases: all 18 binary-level format tests passed, including inherited and captured child output, failure envelopes, and token-backed health verification.
  2. GC preserves partial-failure information without continuing a damaged generation. Proven through the binary with a fake provider: an injected second-delete failure produced exit 1, deleted: 1, and exact failed/stranded keys. The third delete was not attempted. A rerun retained the incomplete generation; an unknown root blocked deletion.

Findings

No meaningful new findings beyond existing PR feedback. This approval reflects the new-issues-only scope of this review; existing review concerns remain unresolved.

Validation and review context

Passed locally at the locked head:

  • cargo test --workspace --all-targets --locked
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
  • cargo check --workspace --all-targets --features "fastly cloudflare spin" --locked
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --locked
  • Focused JSON, Fastly CLI, and adapter tests.
  • Binary-level GC interruption and rerun checks.

All currently reported GitHub checks passed. Existing reviews, inline threads, and issue comments were inspected to avoid duplicate findings.

No live-provider operations were attempted. The generated-project compilation test remained ignored locally. Head/base were unchanged at completion, and the worktree remained clean. No repository files changed; no delegation.

prk-Jr and others added 2 commits October 7, 2026 19:00
Mark the spec as proposed and awaiting maintainer approval, and update its
type listings, testing section and revision notes to match the code. Remove
the retroactive plan document.

Keep the command's own error when writing the JSON envelope fails. Report
the staged Fastly deploy's service id from the deploy itself, return Empty
for the production deploy, and replace the nested Result with a
ProductionDeploy enum. Treat curl's 000 as no response, not status 0.

Document that build never forwards --format, that auth status reports the
probe's exit status, and that only one command may run per process.

Cover config gc, auth status, staged deploy and staged rollback end to end,
pin the text output of all nine commands, and clear EDGEZERO__ overlays in
the test harness. Disallow CommandExt::exec, and give the lifecycle fixture
its own clippy.toml so the CLI stdout guard does not apply to it.
@dhruv8sh

dhruv8sh commented Oct 7, 2026

Copy link
Copy Markdown
Author

Addressed in e6a0b12, beyond the inline threads:

  • Test gaps: format_json.rs went from 18 to 28 tests.
    • config gc end to end: a default dry run (dry_run: true, deleted: null, older_than_secs: null) and --yes --older-than 1h with nothing to reclaim (deleted: 0, older_than_secs: 3600).
    • auth status: the authenticated path, and a missing native CLI (result: null).
    • A staged deploy resolving its service from FASTLY_SERVICE_ID, and a staged rollback.
    • Exact text bytes for all nine commands, replacing the starts_with/ends_with checks. This is the committed stand-in for the byte comparison against main.
    • The harness also clears EDGEZERO__* / DEMO_APP__*.
    • The generator test asserts each disallowed-methods path in the generated clippy.toml.
    • Still not covered end to end: the GcFailure to partial-result path. Building a failing chunked generation needs the Fastly crate's private chunker. That mapping is covered by gc_failure_fills_the_partial_result in output.rs and the adapter's gc tests.
  • Small items:
    • healthcheck treats curl's 000 as no response rather than status 0: status_code keeps the last real HTTP status, or null if no probe got one.
    • The deploy table notes that a production deploy relying on FASTLY_SERVICE_ID reports service_id: null.
    • The #[expect] reason in config.rs now mentions config diff.
    • GcResult::new destructures GcFailure by reference instead of cloning it.
    • auth status --help uses the same --format wording as the other commands.
    • The process tests use a drop guard and recover a poisoned lock.
    • native_auth_status_maps_exit_status_to_state is #[cfg(unix)].
  • Lint gaps:
    • std::os::unix::process::CommandExt::exec is now disallowed (with allow-invalid, for non-unix targets), in the workspace and in the generated project.
    • I didn't ban Stdio::inherit: .stderr(Stdio::inherit()) is legitimate, and the lifecycle fixture harness uses it.
    • I didn't add #[non_exhaustive] to the outcome structs. Adapter crates build them with struct literals, and #[non_exhaustive] forbids that outside edgezero-adapter, so every adapter would need constructors or builders. Today all adapters are in this workspace (publish = false).
  • CI: the cargo fmt job was red on the merge head (082168c). The lifecycle fixture from Add opt-in reusable application lifecycles #379 has no clippy.toml of its own, so it picked up the root disallowed-methods guard. It now has its own clippy.toml with just the test exemptions.
  • Out of scope: the five pre-existing items (partial provision, the non-UTF-8 tee, the uncaptured built-in production deploy, an empty token, rolling back to the same version) are left for follow-up issues.

@dhruv8sh
dhruv8sh requested a review from prk-Jr October 7, 2026 19:29
Bring in #381 (Fastly stores bound per deployment environment), which
moved deploy into the Adapter::preflight_deploy / deploy / finalize_deploy
hooks and made Fastly staging an adapter-managed release deploy.

Resolution keeps the --format json deploy result on the new hooks:
- Adapter::deploy and Adapter::finalize_deploy return ActionOutcome, like
  execute. Fastly reports DeployOutcome from the managed deploy plan and
  from finalize_deploy's resolved production version.
- The CLI's adapter::deploy returns DeployFailure, separating a failed
  deploy from a live deploy whose finalization failed; the latter keeps its
  JSON result with version: null.
- Fastly provision takes main's logical/physical alias handling with the
  typed ProvisionReport; the removed runtime-env provisioning and the old
  CLI-side staging and version-resolution code are dropped.
- emit_active_version_for returns ActiveVersionOutcome; finalize_deploy
  logs version=<N>, so text output is unchanged.
- Tests follow main's version-list schema and the --application-release
  requirement for staged deploys. Spec revision 5 records the merge.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR Review (re-review at 392546a)

Summary

Every finding from the first two review rounds is fixed on the legacy deploy path, and the new tests prove it: 6 of the 28 end-to-end tests fail against ee94a6a because of the bugs they cover. Text output still matches main across 124 of 137 compared scenarios: 11 differences are intended (help text, --format parsing, the build rejection) and 2 are the healthcheck 000 change below. All 158 JSON-mode runs, including main's new managed Fastly deploy, wrote exactly one envelope.

What's left comes from the merge with main (#381). Fastly deploys with --staging, --application-release or declared stores now take an adapter-managed path. The PR's JSON handling and docs weren't brought along, so a managed deploy that fails after creating (or even activating) a version reports result: null, and the deploy docs describe only the legacy path.

Findings

Blocking

  • 🔧 A managed deploy that fails after creating a version reports result: null, even when the version went live (crates/edgezero-cli/src/adapter.rs:219)
  • 🔧 The deploy docs and spec describe only the legacy path (docs/guide/cli-reference.md:822, spec L757)

Non-blocking

  • 🤔 The JSON deploy result has no package_sha256, which the deploy-fastly action reads from text (crates/edgezero-cli/src/output.rs:210)
  • 🤔 The healthcheck 000 change alters text output and isn't declared (crates/edgezero-adapter-fastly/src/cli.rs:6833)
  • 🤔 The provision schema is stale after the merge: edgezero_runtime_env, a store.logical that is never null, and updated / would_update with no producer (docs/guide/cli-reference.md:873)
  • ⛏ Doc comments made stale by the refactor and the merge (crates/edgezero-adapter/src/registry.rs:48)
  • ⛏ Loose ends on the late---format rejection (crates/edgezero-cli/src/lib.rs:189)
  • 🤔 Test gaps
    • The merge replaced the only successful staged-deploy end-to-end test (staged_deploy_text_and_json_resolve_the_service_from_the_env) with a failure-only one, which reopens the earlier gap for the resolved staged service_id. deploy_managed_with_context and execute_managed_deploy_plan_with_emit have no tests in the repo, so neither a managed success nor a managed failure is tested in either format.
    • Also untested:
      • a DeployFailure::Finalize on the fallback path (built-in fastly compute deploy + finalize_deploy(None));
      • "a finalizer's Deploy outcome takes precedence" (adapter.rs:264), since RecordingDeployAdapter returns Empty everywhere;
      • the GcFailure → partial result mapping in config.rs end to end (gc_failure_fills_the_partial_result re-implements the mapping rather than calling it);
      • the broken-stdout / broken-stderr behaviour from the last round;
      • a mixed 503 / 000 healthcheck.
    • fake_fastly_staging and FASTLY_STAGING_MANIFEST (format_json.rs L55, L271) are now dead; the test that used them fails before any provider call.
  • ⛏ Spec §7.2 and the revision notes are stale after the merge
    • L293-294: the Empty comment lists "production deploy", but a managed production deploy returns Deploy.
    • L362: "Data lines move from the Fastly adapter to the CLI" isn't true for deploy. version= and package-sha256= are still logged by the adapter (fastly cli.rs:927, :1048, :1100, :2117).
    • The trait listing doesn't show deploy / finalize_deploy / preflight_deploy, or DeployFailure.
    • L281 counts "two test adapters"; there are four.
    • Rev 3 (L715): nothing passes --require-active any more. Rev 4 (L735): "the Fastly production deploy returns Empty" now holds only for the legacy path.
    • §9.2 (L593) lists the build rejection as the only intended text change, which leaves out the healthcheck 000 change.
  • ⛏ Small items
    • A healthy probe whose after-probe version check fails returns result: null. Spec §8 (L504) only covers failures before the probe. Document it, or return the result with version_verified: false.
    • On the legacy path, when a version is parsed from the deploy output but can't be verified (no token), the Finalize partial result drops the parsed version, and the error is just "FASTLY_API_TOKEN must be set in the environment", with no "the deploy went live" context.
    • finish hard-codes the [edgezero] prefix in its write-failure log (output.rs:561), even in generated CLIs that use [<name>]. Rev 4 says "the command's outcome is returned"; for a command that succeeded, the write error replaces it.
  • 🌱 Under JSON, a broken stderr pipe kills child processes. process::status (process.rs:42-44) points the child's stdout at our stderr, so if the stderr reader goes away the child dies of SIGPIPE. Reproduced: build = "echo built-ok" succeeds in text mode and fails with signal: 13 under JSON. A managed deploy can fail this way after a provider mutation. run_shell_tee drains the output and ignores write errors, so the two paths behave differently. The envelope itself is still written in every case.

📌 Out of Scope

  • The generated project's new disallowed-methods guard isn't exercised in CI. CI only cargo checks a generated project, and the generator test string-matches clippy.toml. Generated projects already fail strict clippy for reasons this PR didn't cause: item ordering in templates/cli/src/main.rs.hbs (Gc after Validate, L80/L86) and lints from the #[fastly::main] expansion on wasm32-wasip1.

CI Status

  • fmt: PASS
  • clippy (--workspace --all-targets --all-features -D warnings): PASS
  • tests (cargo test --workspace --all-targets): PASS (1569 passed, 0 failed; format_json 28/28)
  • cargo check --workspace --all-targets --features "fastly cloudflare spin": PASS
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASS
  • cargo test -p edgezero-adapter-fastly --features cli: PASS (325 passed)
  • Text mode vs main (f717674): 137 paired runs against fake fastly / curl / wrangler / spin / cargo, including managed deploys built from real verified releases. 124 byte-identical in stdout, stderr, exit code and call log; 11 intended differences; 2 healthcheck 000 differences (above).
  • JSON mode: 158 runs across the 9 commands (including managed staging and production deploys). Each wrote exactly one envelope, and ok matched the exit code.

.into());
};
return managed_adapter.deploy(context, adapter_args);
return Ok(managed_adapter.deploy(context, adapter_args)?);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔧 A managed deploy that fails after creating a version reports result: null, even when the version went live

Main's merge (#381) sends every Fastly deploy that has --staging, --application-release or declared stores through the managed path (owns_managed_deploy, fastly cli.rs:2019-2021), so this is the path most real apps take. Its text contract is explicit: the adapter logs version=<N> "as soon as a recoverable target draft exists, so a caller can recover that version when later preparation fails" (cli-reference.md L238-243; cli.rs:1048, :1100), plus package-sha256= (:927).

Under --format json that recovery data never reaches the envelope. Every error from managed_adapter.deploy(..) goes through this ?, becomes DeployFailure::Deploy, and deploy() maps it to a Failure with no result (lib.rs:308). Reproduced with the repo's own stateful Fastly fakes (from .github/actions/deploy-fastly/tests/make-fake-fastly-env.sh) and a real verified release, injecting failures at six steps: compute update, link delete, link create, stage, activate, and activate on the initial-draft path. In every case stderr has version=42 and the envelope is {"ok": false, "result": null, …}. In the worst case, the activate PUT returns 500 after Fastly committed it: the fake provider shows version 42 active, and the envelope still says result: null.

That contradicts the deploy docs (cli-reference.md L829-831: "Check result, not only ok, before assuming nothing was deployed") and the DeployFailure::Deploy doc at L75 ("nothing is known to be live"). A JSON consumer can only recover by scraping stderr, which is exactly what --format json is meant to replace.

Suggestion: let the managed path report a partial outcome. Two ways:

  • give Adapter::deploy a failure that carries a DeployOutcome, e.g. a DeployFailure::Partial { message, outcome } the CLI maps with Failure::with_result;
  • or follow the HealthcheckOutcome pattern and return Ok(ActionOutcome::Deploy(..)) with a failure field.

Either way, track the draft version inside execute_managed_deploy_plan_with_emit once it exists. Then document what version means on a failed deploy (a non-live draft, or possibly live after an activate with an unknown outcome), reword the DeployFailure::Deploy doc, and add an end-to-end test for a failure after the draft exists.


| Key | Type | Nullable | Meaning |
| ------------ | ------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| `service_id` | string | yes | The service the adapter resolved (a Fastly `--staging` deploy falls back to `FASTLY_SERVICE_ID`), else `--service-id` as passed, else `null`. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔧 The deploy table describes only the legacy deploy path

Since the merge, a Fastly deploy with --staging, --application-release or declared stores is adapter-managed, and that path resolves the service id from --service-id, then the verified release's fastly.toml, then FASTLY_SERVICE_ID (resolve_managed_plan_service_id, fastly cli.rs:1684-1699). It reports both the id and the staged or activated version (deploy_managed_with_context, cli.rs:901-911). Reproduced:

  • a managed production deploy with only FASTLY_SERVICE_ID set returns the resolved service_id and version: 42;
  • the same with service_id in fastly.toml returns the same;
  • a managed --staging deploy with the id only in fastly.toml also resolves it.

So, for the path the docs' own deploy examples use:

  • L826-827 ("A production deploy reads only --service-id, so a Fastly production deploy that relies on FASTLY_SERVICE_ID reports service_id: null") is wrong;
  • the version row ("the activated version for a Fastly production deploy with --service-id, and null otherwise") is wrong;
  • this row's staging note leaves out fastly.toml, which takes precedence over the env var.

These only hold on the store-free, release-free Fastly production path (manifest command or built-in fastly compute deploy + finalize_deploy).

Suggested wording:

  • service_id: "Fastly adapter-managed deploys (any --staging, --application-release, or an app that declares stores) report the id they used: --service-id, else the release's fastly.toml service_id, else FASTLY_SERVICE_ID. Otherwise --service-id as passed, else null."
  • version: "The staged or activated version for a managed Fastly deploy; on the store-free, release-free Fastly production path, the activated version when --service-id is passed; null otherwise."

Scope the FASTLY_SERVICE_ID → null note to that legacy path, and mirror the change in spec §8 and the rev 5 note.

- **A Fastly staged deploy** is adapter-managed and requires
`--application-release`; it reports the service and the staged version
from its deploy plan. Without a release it fails before any provider call.
- **A Fastly production deploy's version** is resolved by `finalize_deploy`:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔧 True only for the legacy path

A managed Fastly production deploy (one with --application-release or declared stores) never calls finalize_deploy. adapter::deploy returns straight from Adapter::deploy (adapter.rs:219), which reports the version from the deploy plan (fastly cli.rs:901-911). See the comment on cli-reference.md L822. §8's deploy section (L476-484) needs the same correction, and rev 5 should mention that a managed failure currently loses its result (see the comment on adapter.rs:219).


/// `deploy` result.
#[derive(Debug, Serialize)]
pub(crate) struct DeployResult {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 The JSON deploy result has no package_sha256

The managed Fastly deploy logs package-sha256=<SHA256> (fastly cli.rs:927), the CLI reference documents it (L240-243), and the deploy-fastly action reads it from text output (deploy.sh:89, parse_contract_value package-sha256). DeployOutcome and DeployResult only carry service_id and version, so under JSON the digest exists only as a stderr log line, and deploy can't move off text scraping. That's the problem this PR sets out to solve.

Suggestion: add package_sha256: Option<String> to DeployOutcome and DeployResult (null when not managed), and document it in the deploy table. Adding a key needs no schema version bump, and it's cheapest to do before v1 ships. Otherwise, at least say in the Results table that it's only on stderr. --application-release isn't echoed either, which is fine, but worth one sentence.

match stdout.trim().parse::<u16>() {
// curl writes `000` when no HTTP response arrived; that is a transport
// failure, not a status code.
Ok(0) => Err("curl got no HTTP response (status 000)".to_owned()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 This changes text output too, and the PR doesn't declare it

Because the 000 mapping lives in the adapter, text mode changes as well as JSON. Against main, with a curl that prints 000 and exits 0:

-status-code=0
 healthy=false
-[edgezero] healthcheck for app.example.com failed after 2 attempt(s): unhealthy HTTP status 0
+[edgezero] healthcheck for app.example.com failed after 2 attempt(s): curl got no HTTP response (status 000)

With 503 then 000, text now prints status-code=503 where main printed status-code=0. The healthcheck-fastly action publishes the last status-code= line as its status-code output (healthcheck.sh:124-126), so that output changes from 0 to empty or 503. Spec rev 4 / §9.2 list the build rejection as the only intended text change. On the same mixed run, JSON status_code reports 503 while error.message describes the 000, and no test covers a mixed sequence.

Real curl usually exits 7 on a connect failure, which takes the transport-error branch and is unchanged, so this is rare. Either declare it as an intended text change (PR description, spec §9.2, a test for the mixed case), or keep the adapter's text behaviour and do the 0 → null mapping only in HealthcheckResult::new.

| `action` | string | no | `created`, `already_present`, `would_create`, `updated`, `would_update`, `not_applicable`, or `note`. |
| `store` | object | yes | `{ kind, logical, platform }`; `null` for an adapter-level note. |
| `store.kind` | string | no | `config`, `kv`, or `secrets`. |
| `store.logical` | string | yes | The manifest id; `null` for a store EdgeZero owns itself, such as Fastly's `edgezero_runtime_env`. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 The provision schema is stale after the merge

#381 removed Fastly's edgezero_runtime_env store from provision (the Fastly provision now emits only per-store entries plus the "no declared stores" note). As a result:

  • nothing produces store.logical: null any more (ProvisionEntry::for_store always sets it);
  • nothing produces updated or would_update (only the runtime-env path did);
  • L881 ("Fastly's real run also omits the edgezero_runtime_env entry…"), this row's example, spec §8 (L535-537) and the ProvisionStoreRef.logical doc (registry.rs:340-341) describe something that no longer exists.

Suggestion: drop the edgezero_runtime_env references. Before v1 ships, either make store.logical non-null and drop Updated / WouldUpdate (making a nullable key always non-null is allowed by the compatibility policy, and a v1 enum doesn't need values nothing emits), or keep them and say they're reserved and not currently emitted.

/// pre-existing text output or, under `--format json`, as a JSON result, so
/// adapters report data here instead of printing it.
///
/// A negative result the action still measured (an unhealthy probe, an

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⛏ Doc comments made stale by the refactor and the merge

  • Here: "an Ok outcome carrying a failure message". AuthStatusOutcome now uses AuthState::Unauthenticated { reason } and has no failure. The comment also names only Adapter::execute, but deploy and finalize_deploy return ActionOutcome too.
  • L59 Empty: "login, logout, serve" leaves out the legacy production deploy and manifest command overrides.
  • L102 DeployOutcome: "Result of Deploy / DeployStaging". No execute returns it any more. It comes from Adapter::deploy (managed) and finalize_deploy, and Fastly's execute(DeployStaging) now errors.
  • L27-36 EmitVersion / Healthcheck: say the adapter emits version= or exits non-zero. The adapter now returns an outcome, and the CLI emits the line and sets the exit code.
  • L340-341 ProvisionStoreRef.logical: "Fastly's runtime-override store" no longer exists.
  • fastly cli.rs:2087: "the CLI's finalize_deploy". It's the adapter's hook, and execute(Deploy) isn't on the CLI's deploy path.
  • fastly cli.rs:6584-6597: "emit it as a version=<N> line" and "--require-active … is passed by the production-deploy version fallback". finalize_deploy now calls emit_active_version_for(.., true) directly, so nothing passes the flag and arg_flag(args, "--require-active") at L6601 is always false.
  • format_json.rs:674: "then the CLI prints the version line". The adapter's finalize_deploy prints it.

.find(|arg| *arg == "--format" || arg.starts_with("--format="))
{
return Err(format!(
"`{flag}` after a passthrough argument is forwarded to the build command, not read \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⛏ The late---format rejection has three loose ends

  • This message says the flag "is forwarded to the build command", but it's rejected. Its remedy ("Put --format before the first passthrough argument") also doesn't fit build -- --format json, which is now rejected too. Something like: "{flag} among the passthrough arguments is rejected: build cannot forward a --format. Put EdgeZero's --format before the first passthrough argument."
  • The docs (cli-reference.md L720, L813) and spec (§5.2, §6.4, §9.3) say the rejection exits 1. That's only true for the bundled binary; a CLI generated from the template exits 2 for every error (main.rs.hbs). The exit-code paragraph near L760 already scopes this correctly.
  • The build section still says "Any arguments after -- are forwarded to the adapter command" (cli-reference.md L108), with no mention of the --format exception.

This branch has not been deployed

No deployments
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.

Add --json output across the CLI

4 participants