Repository navigation
Conversation
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
aram356
left a comment
There was a problem hiding this comment.
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: trueon an unhealthy probe: the after-probe check only runs when the probe is healthy (crates/edgezero-adapter-fastly/src/cli.rs:5641) - 🔧
buildswallows a trailing--format json: exits 0 with text on stdout (crates/edgezero-cli/src/args.rs:251) - 🔧
config gc --yeswith nothing to reclaim reportsdeleted: 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 validatereportsapp_config: nullalthough 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_idreflects 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::Statusvariant (args.rs:237),OutputFormatcompared with==(output.rs:75), a leftover bare block (edgezero-adapter-fastly/src/cli.rs:3958), and an unreachable--require-activepromise 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 gcolder_than_secs/deleted/store.id, andconfig 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.
- 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:
- 🤔 Test gaps
- No test produces
version_verified: true(the existing healthcheck test removes the token). - The
config gcmapping fromfailure_diagnosticto a partial result (config.rs:467-469), and howfailed/stranded/uncertain/deletedare filled, have no JSON-level test. deploy,rollbackandactive-versionhave no tests of their result mapping, or of theversion=/rolled-back-to=/ "no active version yet" lines that moved from the adapter into the CLI.config validatehas 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.
- No test produces
- 🌱 Lint gaps:
disallowed-methodsfires on everystatus/spawnform I tried (checked in a scratch crate using thisclippy.toml). It does not catch.stdout(Stdio::inherit()).output()(which really does leak at runtime),CommandExt::exec, orwriteln!(io::stdout(), …)(print_stdoutonly covers the macros). None of these is used today. Addingstd::io::stdoutto 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'stemplates/root/clippy.toml.hbsdoesn't carry the lint, so a downstream CLI's own subcommands are unguarded. - 🌱 A closed stderr loses the envelope:
CliLoggerwrites routedinfowitheprintln!, 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 whoamiexit code when logged out (unverified;wranglerwasn'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 jsonreportsstate: 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 thewhoamioutput 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": PASScargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASScargo test -p edgezero-adapter-fastly --features cli: PASS (288 passed)- Text mode vs
main: 130 paired hermetic runs across the 9 commands (fakecurl/fastly/wrangler/spin/cargo) gave byte-identical stdout, stderr and exit codes. The only difference is the usage-error text for--format yaml.
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
prk-Jr
left a comment
There was a problem hiding this comment.
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 thebuild -- --formatcase 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, thedisallowed-methodslint enforces it, and the exceptions carry#[expect]with reasons. "stdout stays clean" can't quietly regress. - Routing can't get stuck:
OutputScopesaves and restores the routing flags inDrop, so a?, an early return or a panic always restores routing. - Each fact stored once:
AuthState::Unauthenticated { reason },HealthcheckOutcome::healthy()andGcFailurerule out the contradictory states. - Tests that check behaviour: the fake-
curlAPI call log proves both version checks ran, rather than just trusting the flag.sole_json_documentandassert_envelope_invariantsmake 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
--versionflag that doesn't exist (cli-reference.md:662) - 🔧 Empty-stdout list misses the late-
--formatrejection, which contradicts the exit-1 guarantee (cli-reference.md:657)
Non-blocking
- 🤔
buildrejects-- --format json,deployforwards it. This also changes text mode forbuild(lib.rs:176). - 🤔
auth statusdecidesauthenticatedfrom 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).
- 🤔
OutputScopedoesn't support overlapping runs on separate threads. It needs a doc note (output.rs:68). - 🤔 Test gaps:
config gcis 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 printingtext_lineson failure) is untested. Thefake_fastly_gchelper in the Fastly adapter tests could be reused.auth statushas end-to-end tests only for the unauthenticated case. There's no test of the authenticated path or of a missing native CLI (result: nullper the docs).- Staging
deploy/rollbackaren't tested. That includes theversion=line that moved from the adapter to the CLI, and the resolvedservice_id(flag, thenFASTLY_SERVICE_ID). - The generated project's new
clippy.toml.hbslines aren't asserted. The generator test only checksallow-expect-in-tests, so addingassert!(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 againstmainfrom spec §9.2 is still not committed. - The test harness only clears
FASTLY_API_TOKEN/FASTLY_SERVICE_ID. An exportedEDGEZERO__*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 productionDeployreturnsDeploy{None,None}while other adapters returnEmpty(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:
healthcheckreportsstatus_code: 0when curl prints000. The docs saynullwhen no probe got a response.- A production Fastly deploy with only
FASTLY_SERVICE_IDset reportsservice_id: null, althoughfastlyused that id. Worth one sentence in thedeploytable. - The
#[expect]reason atconfig.rs:1374says "config pushhas no--format", butconfig diffcalls the same function. GcResult::newclones the wholeGcFailure, diagnostic string included, just to read three lists. Destructure it by reference instead.auth status --helpdescribes--formatwith shorter wording than the other eight commands (args.rs:236).- The
processtests restore the global setting by hand. Ifexpect("spawn")panics, the setting staystrueandPOLICY_LOCKis poisoned. Use a drop guard. native_auth_status_maps_exit_status_to_staterunstrue/falsebut isn't#[cfg(unix)], unlike the other fake-binary tests.
- 🌱 Lint gaps:
disallowed-methodsdoesn't catchstd::os::unix::process::CommandExt::execor.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:
provisionfailing partway: the result isnulland 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 12line 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 "noversion=<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 7is 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": PASScargo 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 thebuild -- --formatcase above.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
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
- 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.
- 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 --lockedcargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare spin" --lockedcargo 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.
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.
|
Addressed in e6a0b12, beyond the inline threads:
|
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
left a comment
There was a problem hiding this comment.
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
deploydocs and spec describe only the legacy path (docs/guide/cli-reference.md:822, spec L757)
Non-blocking
- 🤔 The JSON
deployresult has nopackage_sha256, which thedeploy-fastlyaction reads from text (crates/edgezero-cli/src/output.rs:210) - 🤔 The healthcheck
000change 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, astore.logicalthat is never null, andupdated/would_updatewith 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-
--formatrejection (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 stagedservice_id.deploy_managed_with_contextandexecute_managed_deploy_plan_with_emithave no tests in the repo, so neither a managed success nor a managed failure is tested in either format. - Also untested:
- a
DeployFailure::Finalizeon the fallback path (built-infastly compute deploy+finalize_deploy(None)); - "a finalizer's
Deployoutcome takes precedence" (adapter.rs:264), sinceRecordingDeployAdapterreturnsEmptyeverywhere; - the
GcFailure→ partial result mapping inconfig.rsend to end (gc_failure_fills_the_partial_resultre-implements the mapping rather than calling it); - the broken-stdout / broken-stderr behaviour from the last round;
- a mixed
503/000healthcheck.
- a
fake_fastly_stagingandFASTLY_STAGING_MANIFEST(format_json.rs L55, L271) are now dead; the test that used them fails before any provider call.
- The merge replaced the only successful staged-deploy end-to-end test (
- ⛏ Spec §7.2 and the revision notes are stale after the merge
- L293-294: the
Emptycomment lists "production deploy", but a managed production deploy returnsDeploy. - L362: "Data lines move from the Fastly adapter to the CLI" isn't true for deploy.
version=andpackage-sha256=are still logged by the adapter (fastlycli.rs:927,:1048,:1100,:2117). - The trait listing doesn't show
deploy/finalize_deploy/preflight_deploy, orDeployFailure. - L281 counts "two test adapters"; there are four.
- Rev 3 (L715): nothing passes
--require-activeany more. Rev 4 (L735): "the Fastly production deploy returnsEmpty" now holds only for the legacy path. - §9.2 (L593) lists the
buildrejection as the only intended text change, which leaves out the healthcheck000change.
- L293-294: the
- ⛏ 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 withversion_verified: false. - On the legacy path, when a version is parsed from the deploy output but can't be verified (no token), the
Finalizepartial 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. finishhard-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.
- A healthy probe whose after-probe version check fails returns
- 🌱 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 withsignal: 13under JSON. A managed deploy can fail this way after a provider mutation.run_shell_teedrains 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-methodsguard isn't exercised in CI. CI onlycargo checks a generated project, and the generator test string-matchesclippy.toml. Generated projects already fail strict clippy for reasons this PR didn't cause: item ordering intemplates/cli/src/main.rs.hbs(GcafterValidate, 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_json28/28) cargo check --workspace --all-targets --features "fastly cloudflare spin": PASScargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASScargo test -p edgezero-adapter-fastly --features cli: PASS (325 passed)- Text mode vs
main(f717674): 137 paired runs against fakefastly/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 healthcheck000differences (above). - JSON mode: 158 runs across the 9 commands (including managed staging and production deploys). Each wrote exactly one envelope, and
okmatched the exit code.
| .into()); | ||
| }; | ||
| return managed_adapter.deploy(context, adapter_args); | ||
| return Ok(managed_adapter.deploy(context, adapter_args)?); |
There was a problem hiding this comment.
🔧 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::deploya failure that carries aDeployOutcome, e.g. aDeployFailure::Partial { message, outcome }the CLI maps withFailure::with_result; - or follow the
HealthcheckOutcomepattern and returnOk(ActionOutcome::Deploy(..))with afailurefield.
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`. | |
There was a problem hiding this comment.
🔧 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_IDset returns the resolvedservice_idandversion: 42; - the same with
service_idinfastly.tomlreturns the same; - a managed
--stagingdeploy with the id only infastly.tomlalso 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 onFASTLY_SERVICE_IDreportsservice_id: null") is wrong; - the
versionrow ("the activated version for a Fastly production deploy with--service-id, andnullotherwise") 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'sfastly.tomlservice_id, elseFASTLY_SERVICE_ID. Otherwise--service-idas passed, elsenull."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-idis passed;nullotherwise."
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`: |
There was a problem hiding this comment.
🔧 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 { |
There was a problem hiding this comment.
🤔 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()), |
There was a problem hiding this comment.
🤔 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`. | |
There was a problem hiding this comment.
🤔 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: nullany more (ProvisionEntry::for_storealways sets it); - nothing produces
updatedorwould_update(only the runtime-env path did); - L881 ("Fastly's real run also omits the
edgezero_runtime_enventry…"), this row's example, spec §8 (L535-537) and theProvisionStoreRef.logicaldoc (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 |
There was a problem hiding this comment.
⛏ Doc comments made stale by the refactor and the merge
- Here: "an
Okoutcome carrying afailuremessage".AuthStatusOutcomenow usesAuthState::Unauthenticated { reason }and has nofailure. The comment also names onlyAdapter::execute, butdeployandfinalize_deployreturnActionOutcometoo. - L59
Empty: "login, logout, serve" leaves out the legacy production deploy and manifest command overrides. - L102
DeployOutcome: "Result ofDeploy/DeployStaging". Noexecutereturns it any more. It comes fromAdapter::deploy(managed) andfinalize_deploy, and Fastly'sexecute(DeployStaging)now errors. - L27-36
EmitVersion/Healthcheck: say the adapter emitsversion=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'sfinalize_deploy". It's the adapter's hook, andexecute(Deploy)isn't on the CLI's deploy path. - fastly
cli.rs:6584-6597: "emit it as aversion=<N>line" and "--require-active… is passed by the production-deployversion fallback".finalize_deploynow callsemit_active_version_for(.., true)directly, so nothing passes the flag andarg_flag(args, "--require-active")at L6601 is always false. - format_json.rs:674: "then the CLI prints the version line". The adapter's
finalize_deployprints 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 \ |
There was a problem hiding this comment.
⛏ 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
--formatbefore the first passthrough argument") also doesn't fitbuild -- --format json, which is now rejected too. Something like: "{flag}among the passthrough arguments is rejected:buildcannot forward a--format. Put EdgeZero's--formatbefore 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 exits2for every error (main.rs.hbs). The exit-code paragraph near L760 already scopes this correctly. - The
buildsection still says "Any arguments after--are forwarded to the adapter command" (cli-reference.md L108), with no mention of the--formatexception.
Summary
key=valuelog lines that were never a stable contract. Nine commands now accept--format json:active-version,auth status,build,config gc,config validate,deploy,healthcheck,provisionandrollback. Each one writes a single versioned envelope,{ schema_version, command, ok, result, error }, to stdout, on success and on failure.cargo,fastly,wrangler,spin, manifest commands) go to stderr. A new clippydisallowed-methodslint means future code can't bypass this.--format textis the default, and its output is byte-identical tomain.docs/superpowers/specs/2026-09-25-cli-format-json-design.md.config diff --format jsonis unchanged.Changes
edgezero-adapter(registry.rs)Adapter::executereturnsActionOutcome,provisionreturnsProvisionReport,gc_config_entriesreturnsGcReport, instead of()or prose lines. A negative result that was still measured (unhealthy probe, unauthenticated session, partly failed gc) is anOkoutcome carrying afailuremessage.edgezero-adapter(process.rs, new)process::status, the one sanctioned inheriting spawn.edgezero-adapter(cli_support.rs)native_auth_status.run_native_cligoes throughprocess::status.edgezero-adapter-{fastly,cloudflare,spin,axum}version=/healthy=/status-code=/rolled-back-to=data lines; the CLI prints the same bytes. All inheriting spawns go throughprocess::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)OutputFormat { Text, Json }and a--formatflag on the nine commands.DiffFormatis unchanged.edgezero-cli(lib.rs,auth.rs,provision.rs,config.rs,adapter.rs)run_*keeps its public signature and emits the envelope itself, so CLIs already generated from the template get JSON without regeneratingmain.rs. The logger'sinfooutput moves to stderr under JSON.clippy.tomldisallowed-methodsforCommand::status/Command::spawn. Piped spawns carry a documented#[expect].edgezero-cli/tests/format_json.rs(new)docs/guide/cli-reference.md--formaton 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.mdCloses
Closes #383
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"wasm32-wasip1(Fastly) /wasm32-wasip2(Spin) /wasm32-unknown-unknown(Cloudflare), via the fullformat.ymlwasm clippy matrix andcargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spinexamples/app-demoworkspace:cd examples/app-demo && cargo test --workspace --all-targets(--locked), plus its fmt and clippycd docs && npm run lint && npm run format && npm run buildedgezero serve --adapter axum(not applicable:serveis out of scope)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 thecheck_no_nested_app_configsteps.Checklist
{id}syntax (not:id)edgezero_core(nothttpcrate)KvRegistry/ConfigRegistry/SecretRegistry(not the legacy single-handle setters) — see spec §6.6