Skip to content

Upgrade EdgeZero to the deploy-actions branch - #940

Open
aram356 wants to merge 18 commits into
mainfrom
worktree-edgezero-316-upgrade
Open

Upgrade EdgeZero to the deploy-actions branch#940
aram356 wants to merge 18 commits into
mainfrom
worktree-edgezero-316-upgrade

Conversation

@aram356

@aram356 aram356 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Upgrades the six edgezero workspace dependencies from tag = "v0.0.4" to the feature/edgezero-deploy-actions tree and adapts Trusted Server to its API changes.

  • Repoint edgezero-adapter-{axum,cloudflare,fastly,spin}, edgezero-cli, and edgezero-core at an immutable rev = "5f3d648c" pin on that branch; the lock is rebuilt from main with a scoped update, so the only non-edgezero lock changes are syn 3.0.3 and toml_edit 0.25.12 (both required by the new edgezero tree).
  • Wire the new ts CLI subcommands surfaced by edgezero-cliactive-version, healthcheck, rollback, config gc — plus deploy --staging, config push/diff --staging, and a top-level --version flag, with argument-parsing coverage.
  • Document the new lifecycle and config gc surface in docs/guide/cli.md, including the staged-deploy config leg (config push --staging before healthcheck --staging), the destructive config gc --yes semantics, and the deploy -- passthrough boundary.
  • Migrate TrustedServerAppConfig to the AppConfigMeta::secret_fields() method that replaces the removed SECRET_FIELDS associated constant.

Verification

All CI gates run locally against the branch and pass:

  • cargo fmt --all -- --check — clean
  • cargo clippy — fastly, axum, cloudflare native + wasm, spin native + wasm — clean
  • Tests — core 1644, fastly 99, axum 32, cloudflare 32, spin 72, cli 29 — all pass
  • integration parity — 13 pass
  • JS build/test/format — 411 pass, prettier clean
  • Review-round follow-ups verified with cargo metadata --locked, cargo check-fastly --locked, host CLI suite (168 tests + clippy -D warnings), and docs prettier

⚠️ Blocked on upstream

stackpop/edgezero#316 is not yet merged. The six deps are pinned to the immutable rev = "5f3d648c" (the commit the lock resolves), so the reference cannot silently move; before this lands, re-pin rev → tag once #316 merges and a tag containing it exists (the v0.0.5 tag cut since then has diverged from the branch and does not contain it). Per the status comment below, move the deployer's deploy-fastly@<ref> in lockstep.

Two residuals the rev pin does not cover (from review):

  • Reachability is branch-scoped. 5f3d648c is ahead of edgezero main, so the only ref keeping it fetchable is feature/edgezero-deploy-actions. If that branch is deleted after merge (the GitHub default) or force-pushed, cargo fetch fails for every cold checkout and CI runner. The rev pin is immutable, not unconditionally fetchable — another reason the rev → tag repin must happen before this merges.
  • A squash merge of Remove immutable cache headers from integrations #316 breaks the mechanical rev → tag swap. A squash commit is not an ancestor of 5f3d648c, so re-pinning to a tag containing it means different bytes and a full gate re-run, not a lock refresh. A merge commit or fast-forward keeps 5f3d648c an ancestor and the swap mechanical. Confirm the merge method on Remove immutable cache headers from integrations #316 before planning the repin.

Out of scope

  • Adopting the new deploy GitHub Actions (deploy-fastly / healthcheck-fastly / rollback-fastly) in this repo's workflows is separate, unstarted work.
  • Embedding the resolved edgezero rev in ts --version output (reviewer suggestion) is a follow-up.

Closes #939

Point the edgezero-* dependencies at the feature/edgezero-deploy-actions
branch (PR #316) and adapt Trusted Server to its API changes:

- Wire the new ts CLI subcommands surfaced by edgezero-cli: active-version,
  healthcheck, and rollback, plus deploy --stage and a --version flag, with
  argument-parsing coverage.
- Migrate TrustedServerAppConfig to the AppConfigMeta::secret_fields() method
  that replaces the removed SECRET_FIELDS associated constant.
@aram356 aram356 self-assigned this Jul 21, 2026
@aram356

aram356 commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Status update — P0/P1 tracking

Head: `96dd1f72` (main merged in twice since the original commit; edgezero dep unchanged, no conflicts).

P0.2 — CI + output-line contract

  • Local verification at head is green: fmt, clippy (fastly), core+fastly tests (1648+99), vitest (411). The earlier red checks were a systemic/environmental run on `96dd1f72` (every job failed, incl. JS/fmt that pass locally) — fresh reruns of all four workflows are in progress.
  • Machine-readable output lines the deployer parses are verified end-to-end. All three are emitted by edgezero-cli at `log::info!` (Info→stdout), and `ts`'s `main.rs` calls `edgezero_cli::init_cli_logger()`, so they reach stdout rather than being swallowed:
    Line Source (edgezero @ 145f1699)
    `version=` `edgezero-cli/src/lib.rs:253`
    `pushed-key=` `edgezero-cli/src/config.rs:386`
    `rolled-back-to=` `edgezero-adapter-fastly/src/cli.rs:2807`
  • Dispatcher/arg-parse tests for `active-version`, `healthcheck`, `rollback` are in `crates/trusted-server-cli/src/run.rs`.

P1.3 — edgezero library ↔ action lockstep (recorded pair)

The `ts` CLI is built from this repo's `Cargo.toml` (edgezero library rev), and the deployer separately pins `stackpop/edgezero/.github/actions/deploy-fastly@` (edgezero action ref). These MUST be the same edgezero commit so library and action can't diverge.

  • Current pair: both = `stackpop/edgezero@145f1699` (branch `feature/edgezero-deploy-actions`).
  • On merge: when edgezero Remove immutable cache headers from integrations #316 lands and is tagged, move BOTH the six `Cargo.toml` deps and the deployer's `deploy-fastly@` to that same tag/SHA in lockstep.

P0.1 — re-pin off the moving branch (blocker, still open)

edgezero #316 is not yet merged and no new tag exists. Cannot re-pin until it lands; PR stays draft until then.

aram356 added 2 commits July 26, 2026 11:26
Re-resolve the six edgezero-* deps from 145f1699 to bb441162 (current tip
of feature/edgezero-deploy-actions, PR #316). The deploy staging flag was
renamed there from --stage to --staging, standardizing on the same verb
healthcheck/rollback/config-push already use; update the deploy CLI parse
test to match. No production dispatch change is needed — ts passes the
edgezero-cli arg structs through, so the renamed flag is picked up
automatically.
Re-resolve the six edgezero-* deps from bb441162 to 908e229a (current tip
of feature/edgezero-deploy-actions, PR #316), and adapt the ts CLI to its
surface changes:

- Wire the new `ts config gc` subcommand (reclaims orphaned config-store
  chunk entries) to edgezero_cli::run_config_gc, with parse coverage for the
  preview default, destructive --yes/--older-than sweep, and the
  --dry-run/--yes conflict.
- Lock the hardened deploy staging behavior: --stage was renamed to
  --staging and deploy passthrough is now last=true, so a stray --stage
  fails closed at parse time instead of routing a staging-intended deploy to
  production. Add tests for the rejection and for post---- passthrough
  capture.
@aram356
aram356 marked this pull request as ready for review August 15, 2026 21:07
@aram356
aram356 requested review from ChristianPavilonis and prk-Jr and removed request for ChristianPavilonis August 15, 2026 21:07
aram356 added a commit that referenced this pull request Aug 16, 2026
aram356 added a commit that referenced this pull request Aug 16, 2026
#1010)

Resolved publisher.rs to keep #1008's inactive-SSAT cache policy;
datadome protection.rs resolved to main's final #992 squash.
aram356 added a commit that referenced this pull request Aug 16, 2026
Re-resolve the six edgezero-* deps from 908e229a to 5f3d648c (current tip
of feature/edgezero-deploy-actions, PR #316). The upstream change is an
internal review-addressing pass (redact config-store errors, fix version
parse, log cleanup, docs) confined to the Fastly adapter CLI; no ts CLI
surface change, so no run.rs adaptation is needed.

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Tight, well-tested upgrade: six edgezero deps repointed at the deploy-actions branch, SECRET_FIELDS migrated to secret_fields(), and the new active-version / healthcheck / rollback / config gc subcommands wired through with argument-parsing coverage for each. Dispatch is a clean passthrough and every new command has a doc comment. Two things block: the dependency reference is mutable while upstream #316 is still open, and the operator docs weren't updated for the new (partly destructive) surface.

Blocking

🔧 wrench

  • Mutable branch = dep on an unmerged upstream PR (Cargo.toml:57-62): upstream stackpop/edgezero#316 is confirmed open / merged: false, but this PR is isDraft: false and MERGEABLE. See the inline comment for the rev pin suggestion.

  • docs/guide/cli.md not updated for the new CLI surface: four subcommands ship undocumented — active-version, healthcheck, rollback, and config gc. config gc is the one that matters most: with --yes and --older-than it destructively sweeps config-store entries, and the only place its semantics are written down right now is a one-line clap doc string and three parse tests. The "Lifecycle commands" block (docs/guide/cli.md:79-90) and "Configuration commands" block (:43-77) both enumerate the available commands, so they read as complete and are now silently wrong.

    The same file also carries a version claim that this PR invalidates (docs/guide/cli.md:67-70):

    EdgeZero v0.0.4 only overrides leaves already present in the TOML; add newly introduced fields to existing configs before relying on their overrides.

    The dependency is no longer v0.0.4. Either confirm the behavior still holds on the new tree and drop the version qualifier, or update it to whatever gets pinned at merge time.

Non-blocking

🤔 thinking

  • Unrelated dependency downgrades in the lock refresh (Cargo.lock): windows-sys 0.61.2 → 0.48.0 and itertools 0.13.0 → 0.10.5. Details and a scoped cargo update inline.

🌱 seedling

  • ts --version can't identify the embedded edgezero (crates/trusted-server-cli/src/run.rs:16): follow-up, not this PR.

📝 note

  • secret_fields() port (crates/trusted-server-core/src/config.rs:115): mechanical and correct; the empty-set caveat above it is unchanged.

👍 praise

  • deploy_rejects_renamed_stage_flag_before_separator (crates/trusted-server-cli/src/run.rs:306): catches a silent staging→production routing hazard created by the upstream flag rename plus last = true passthrough.

CI Status

All 19 checks green on fe5767e — reported from GitHub, not re-run locally:

  • fmt: PASS
  • clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
  • rust tests (core, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
  • integration + browser integration + Fastly EC lifecycle: PASS
  • js tests (vitest) / format-typescript / format-docs: PASS
  • CodeQL (actions, javascript-typescript, rust): PASS

Comment thread Cargo.toml Outdated
Comment thread Cargo.lock Outdated
Comment thread crates/trusted-server-core/src/config.rs
Comment thread crates/trusted-server-cli/src/run.rs
Comment thread crates/trusted-server-cli/src/run.rs
Replace the mutable branch = "feature/edgezero-deploy-actions" reference
with rev = "5f3d648c", the exact commit the lockfile already resolved, so
cargo update or an unlocked resolve cannot silently retarget the branch tip.

Rebuild the lock from main with a scoped update of the six edgezero
packages. The re-resolve of the edgezero subtree unifies wide version
ranges onto co-present lower majors (prost-build/prost-derive onto
itertools 0.10.5, colored/winapi-util onto windows-sys 0.48.0); restore
those five dependency edges to main's picks so the only non-edgezero lock
changes are syn 3.0.3 and toml_edit 0.25.12, both required by the new
edgezero tree. Verified with cargo metadata/check --locked and the host
CLI test suite.
Cover active-version, healthcheck, rollback, deploy --staging with the --
passthrough boundary, and the destructive config gc surface (preview
default, --yes with a required --older-than window, and gc's different
--no-env meaning). Drop the stale EdgeZero v0.0.4 qualifier from the
env-overlay note; the leaves-only behavior is unchanged at the pinned rev.
The dependency is no longer pinned at v0.0.4. The leaves-only overlay
behavior is unchanged at the pinned rev (apply_env_overlay still only
overrides keys already present in the parsed tree), so keep the behavior
claims and remove the version qualifier.
@aram356
aram356 requested a review from prk-Jr August 21, 2026 05:29

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Re-review of the three commits added since the last pass (f90f6384, bf47680d, 40dd9b37). Both prior findings are resolved: the six deps moved from a mutable branch = ref to rev = 5f3d648c…, and the unrelated lock downgrades are gone from the diff. Verified independently — 5f3d648c is an ancestor of the branch tip b3e02111 (behind_by: 0), and the v0.0.5 tag is genuinely diverged from the pin (117 behind / 134 ahead), so the "no tag contains it" reasoning holds.

What remains blocking is the upstream merge gate plus one gap in the new staging docs: the documented deploy --staginghealthcheck --staging sequence never mentions the config leg, so following it literally stages a version pointed at a config key nothing wrote.

4 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change is "wait for upstream", needs a new test function, or touches files outside this diff.

Blocking

🔧 wrench

  • Upstream #316 still open; branch-only reachability and squash-merge consequences — see inline at Cargo.toml:57
  • Staged-deploy docs omit the config leg — see inline at docs/guide/cli.md:120-123

Non-blocking

♻️ refactor

  • --retry 3 is 3 total attempts, not 3 retries — see inline at docs/guide/cli.md:140-141 and crates/trusted-server-cli/src/run.rs:182
  • config_gc_previews_by_default never asserts yes — see inline at crates/trusted-server-cli/src/run.rs:489

🤔 thinking

  • secret_fields() comment names a blocker this upgrade removes — see inline at crates/trusted-server-core/src/config.rs:115

⛏ nitpick

  • No test for the new top-level --version — see inline at crates/trusted-server-cli/src/run.rs:16

Cross-cutting / body-level findings

  • ♻️ 40dd9b37 missed two live v0.0.4 qualifiers. The commit dropped the stale version qualifier across docs/guide/*.md, but two non-archival sites still restate exactly the same claim:

    • trusted-server.example.toml:124 — the operator-facing template: "Keep this leaf present when using the EdgeZero v0.0.4 environment override."
    • crates/trusted-server-cli/tests/config_env_overlay.rs:47"EdgeZero v0.0.4 environment overlays cannot create missing TOML leaves, …"

    Neither file is in this diff, so neither can carry a suggestion — apply manually, dropping v0.0.4 the same way the docs did. (docs/superpowers/plans/2026-07-15-gam-ts-cohort-attribution.md:835 also mentions it, but that is a dated archived plan describing a past experiment; leaving it is defensible.)

    Separately, the genericization itself checks out: at the pinned rev apply_env_overlay still only overrides keys already present in the parsed tree, and env_overlay_only_overrides_existing_keys asserts an env var for an absent key is silently ignored. So this is a by-design property, not a v0.0.4 bug, and dropping the version qualifier is the right call.

  • 📝 Two syn majors and two toml_edit majors now compile. The lock carries syn 2.0.118 alongside the new syn 3.0.3 (pulled by edgezero-macros), and toml_edit 0.23.10+spec-1.0.0 alongside 0.25.12+spec-1.1.0. Both duplicates are host-build-only — proc-macro expansion and CLI-side TOML editing — with no wasm32-wasip1 runtime exposure, so this is compile-time cost rather than shipped weight. Nothing to change in this PR; it collapses when the rest of the tree catches up.

  • 👍 Two things worth keeping. First, the rev pin was done as a scoped lock rebuild — restore main's lock, run the narrow update, then restore the five edges the resolver had needlessly walked back — and verified with cargo metadata --locked rather than asserted. That is exactly the right way to answer "is this lock diff minimal?". Second, the config gc documentation catches a real footgun rather than paraphrasing --help: it spells out that --older-than is a safety assertion about the whole physical store, and that gc's --no-env means something different from every other config subcommand's --no-env (it reselects the target store instead of skipping the app-config overlay). On a destructive command, that distinction is the one an operator would otherwise learn the hard way.

CI Status

All 19 reported checks PASS at 40dd9b37:

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS

Verification performed for this review

Every suggestion below was applied in an isolated worktree at 40dd9b37, verified alone and then batched, with a byte-exact pre/post patch snapshot each time:

cargo fmt --all -- --check prettier 3.8.1 (docs/node_modules) clippy -D warnings tests
each suggestion alone PASS PASS PASS 165 pass
all four batched PASS PASS PASS 165 pass

No drift between the approved patch and the post-verification tree on any run. Scope note: clippy and tests were run as --package trusted-server-cli --target aarch64-apple-darwin rather than the full six-alias chain, because both Rust suggestions are assertion lines inside that crate's own #[cfg(test)] module and cannot reach an adapter target.

Comment thread Cargo.toml
Comment thread docs/guide/cli.md
Comment thread docs/guide/cli.md Outdated
Comment thread crates/trusted-server-cli/src/run.rs Outdated
Comment thread crates/trusted-server-cli/src/run.rs
Comment thread crates/trusted-server-core/src/config.rs
Comment thread crates/trusted-server-cli/src/run.rs
ChristianPavilonis added a commit that referenced this pull request Aug 21, 2026
# Conflicts:
#	docs/guide/cli.md
…ording

A staged deploy only redirects the staged version's config selector at the
<logical-store-id>_staging key; it does not copy the production blob. Add
the config push/diff --staging step to the staging sequence so following
the guide no longer stages a version whose config key nothing wrote, and
note the --key conflict. Reword the healthcheck defaults: --retry 3 is 3
total attempts (attempts = retry.max(1) upstream), not 3 retries after a
first try.
Add parse coverage for config push/diff --staging including the --key
conflict, assert healthcheck's path default and reword the retry assertion
to total attempts, assert config gc's yes default since --yes is the only
destructive gate, and pin the top-level --version flag via
ErrorKind::DisplayVersion.
The pinned edgezero rev ships nested/array secret extraction, so the
secret_fields comment no longer names it as the blocker; what remains is
spelling out the paths in this hand-written impl (it does not inherit the
derive's) plus operator migration. Drop the stale v0.0.4 qualifiers the
earlier docs pass missed in the example config and the env-overlay test.
@aram356
aram356 requested a review from prk-Jr August 22, 2026 05:41

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Repoints the six edgezero workspace deps from tag = "v0.0.4" to an immutable rev = "5f3d648c" pin on the unmerged feature/edgezero-deploy-actions branch, wires the new ts lifecycle subcommands, and documents them. The Rust side is in good shape: the lockfile claims in the PR body check out, the SECRET_FIELDS -> secret_fields() migration is behavior-preserving, and no production wire format moved across the 39k-line upstream jump. The remaining blockers are the upstream merge gate and one operator-docs gap that leaves a rollback unreachable.

3 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. All three are docs/guide/cli.md prose and were verified against the pinned prettier in docs/node_modules (3.8.1), individually and batched, with no drift.

Blocking

🔧 wrench

  • Upstream stackpop/edgezero#316 is still open; the pin is immutable but branch-scoped — see inline at Cargo.toml:57
  • The documented rollback lifecycle gives an operator no way to obtain --rollback-to — see inline at docs/guide/cli.md:140-153

Non-blocking

♻️ refactor

  • config gc's destructive-target warning enumerates --no-env but omits --store — see inline at docs/guide/cli.md:94-100
  • deploy passthrough is a breaking change, documented as if it were always the rule — see inline at docs/guide/cli.md:114-118

👍 praise

  • The reworded secret_fields() comment is now accurate — see inline at crates/trusted-server-core/src/config.rs:113-116

Cross-cutting / body-level findings

  • 📝 Lockfile claims audited and confirmed; the "second lockfile" CI risk does not exist on this branch. All six Cargo.toml deps carry the same rev = "5f3d648c3c6c38fc6e6b22b5c65c66177363aad8". Cargo.lock has 8 matching source lines (the six plus transitive edgezero-adapter and edgezero-macros), all identical, and zero remaining tag= / v0.0.4 references. cargo metadata --locked --offline exits 0, so the lock is consistent with the manifest and needs no re-resolve. The body's "only syn 3.0.3 and toml_edit 0.25.12 are non-edgezero changes" is true — the rest of the lock diff is source-line repointing plus mechanical toml_edit -> toml_edit 0.23.10+spec-1.0.0 / 0.25.12+spec-1.1.0 disambiguation, and every new transitive (winnow 1.0.3, toml_datetime 1.1.1, toml_writer, toml_parser 1.1.2, indexmap 2.14.0) already existed in the lock. The earlier-round windows-sys 0.61.2->0.48.0 and itertools 0.13.0->0.10.5 regressions are gone. On the separate-lockfile concern raised in an earlier round: crates/trusted-server-integration-tests/Cargo.lock does not exist on this branch — the crate is a workspace member using { workspace = true } deps, so it shares the root lock. The only other tracked lockfile is crates/trusted-server-openrtb-codegen/Cargo.lock, which has no edgezero edges, is untouched here, and has no --locked CI gate. No lockfile-mismatch gate is at risk.

  • 📝 Production wire formats and runtime behavior are unchanged across the upstream jump. Diffed 9e661ae..5f3d648 directly, since neither the PR body nor CI answers this. Config blob compatibility holds: BlobEnvelope keeps ENVELOPE_VERSION_V1 and the same four fields (only error Display/Debug gained hash redaction), and POINTER_KIND = "fastly_config_chunks", CHUNK_KEY_INFIX = ".__edgezero_chunks.", FASTLY_CONFIG_ENTRY_LIMIT = 8_000, CHUNK_PAYLOAD_TARGET = 7_000 are all unchanged — which matters because crates/trusted-server-core/src/settings_data.rs duplicates those constants in this repo's own resolver. edgezero-core/src/manifest.rs changed only in style and env_config.rs is not in the diff at all, so EnvConfig::store_key("config", ...) still resolves the staging selector a staged deploy redirects. EdgeError::status() maps identically in both revs, so no status-code drift. New surface is additive with no-op defaults (Router::with_state plus a per-request extensions.extend(state_extensions.clone()), empty here; the Fastly dispatch_with_handles extend closure is |_req, _extensions| {} on the default path). One thing deliberately not raised as a finding: edgezero's ConfigOutOfDate error now emits a redacted field_path and a literal <app-cli> placeholder, which would be an operator-visible 503-body regression — except this repo never uses edgezero's AppConfig<C> extractor, loading config through its own config_payload.rs -> BlobEnvelope -> Settings::from_json_value path with TrustedServerError::Configuration. Not reachable.

  • 📝 What was and was not verified locally. Run in an isolated worktree at this exact head: cargo fmt --all -- --check clean; pinned prettier 3.8.1 from docs/node_modules (not npx, which resolves a different version and reports false failures) --check . clean at baseline, for each suggestion alone, and for all three batched; cargo metadata --locked --offline exit 0; ./scripts/test-cli.sh exit 0; cargo test -p trusted-server-cli --target aarch64-apple-darwin --lib 168 passed, 0 failed, with all 14 new run::tests::* present and green. Byte-exact pre/post patch snapshots showed no drift on every verification run. Relied on green CI at this head for the six-alias clippy chain, cargo test-fastly/-axum/-cloudflare/-spin, the parity suite, and vitest — no Rust suggestion is proposed, so none was required for scratch verification. Not verifiable from here at all: the runtime behavior of every lifecycle command (deploy --staging, healthcheck, rollback, active-version, config gc), which needs a live Fastly service and API token. All coverage in this PR and in this review is clap parse-level only.

CI Status

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS

Comment thread Cargo.toml
edgezero-adapter-spin = { git = "https://github.com/stackpop/edgezero", tag = "v0.0.4", default-features = false }
edgezero-cli = { git = "https://github.com/stackpop/edgezero", tag = "v0.0.4" }
edgezero-core = { git = "https://github.com/stackpop/edgezero", tag = "v0.0.4", default-features = false }
edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero", rev = "5f3d648c3c6c38fc6e6b22b5c65c66177363aad8", default-features = false }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 wrench — Upstream stackpop/edgezero#316 is still open, so this pin is immutable but branch-scoped.

Re-verified against the upstream API at review time, not carried over from the last round:

  • repos/stackpop/edgezero/pulls/316 -> state: open, merged: false, merged_at: null, head b3e02111, mergeable_state: clean.
  • compare/5f3d648c...main -> status: behind, ahead_by: 0, behind_by: 116 — the pinned commit is 116 commits ahead of edgezero main, so feature/edgezero-deploy-actions is the only ref keeping it fetchable.
  • Tag list unchanged: v0.0.5 = a2d0c538, v0.0.4 = 9e661ae5. No tag contains 5f3d648c.

Why this still blocks even though the body records it: this PR is non-draft and MERGEABLE, so nothing mechanical stops it landing before the repin. Two consequences follow.

  1. Deleting the feature branch after Remove immutable cache headers from integrations #316 merges — the GitHub default — makes cargo fetch fail on every cold checkout and CI runner. The rev pin is immutable, not unconditionally fetchable.
  2. A squash merge of Remove immutable cache headers from integrations #316 produces a commit that is not an ancestor of 5f3d648c, so "rev -> tag" becomes a repin to different bytes plus a full gate re-run, not a reference swap. A merge commit or fast-forward keeps 5f3d648c an ancestor and keeps the swap mechanical.

No code change requested here — this comment is the merge gate, and it is the only reason the verdict is REQUEST_CHANGES rather than COMMENT. Worth confirming the merge method on #316 before planning the repin, and moving the deployer's deploy-fastly@<ref> in lockstep.

Comment thread docs/guide/cli.md
Comment on lines +140 to +153
Inspect and verify deployments with the deploy lifecycle commands:

```bash
# Print the currently active deployment version
ts active-version --adapter fastly --service-id <service-id>

# Probe a deployed version until it reports healthy
ts healthcheck --adapter fastly --service-id <service-id> \
--version <version> --domain edge.example

# Re-activate a previously active version
ts rollback --adapter fastly --service-id <service-id> \
--version <bad-version> --rollback-to <previous-version>
```

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 wrench — The newly documented rollback lifecycle gives an operator no way to obtain --rollback-to.

Lines 140-153 present active-version -> healthcheck -> rollback as one "inspect and verify" group, and the active-version comment reads "Print the currently active deployment version". An operator who deploys, sees breakage, and then reaches for rollback runs ts active-version — and gets the bad version back. Traced at the pinned rev:

  • edgezero_cli::run_active_version resolves the currently active version (Action::EmitVersion), and its own doc says the value must be "captured BEFORE a deploy ... Fastly's version list has no field to infer a previously-live version afterward."
  • run_rollback hard-errors without --rollback-to: "a production rollback requires --rollback-to ... it must be captured before the deploy that superseded it -- use deploy's previous-version output." That upstream hint is itself stale: grep -rn 'previous.version' across the pinned tree finds no such emission. Production deploy emits only the activated version, via log::info!("version={version}").
  • Upstream's own GitHub action encodes the ordering the docs omit: "A production deploy runs active-version to capture the rollback target and fails fast (before touching the provider) if it is missing."

Two supporting corrections are folded into the same contiguous range, since they sit inside it: the <version> placeholder for healthcheck has no stated source (it comes from the deploy's version=<N> line, which run_deploy only emits when args.service_id.is_some() && adapter == "fastly"), and all three commands are Fastly-only — edgezero-adapter-{axum,cloudflare,spin}/src/cli.rs each return Err for EmitVersion | Healthcheck | Rollback ("The Fastly staging lifecycle is Fastly-only") — yet only deploy --staging is labelled "(Fastly only)".

Same class as the config-leg gap fixed last round: following the guide literally leaves you stuck, here at the worst possible moment.

Suggested change
Inspect and verify deployments with the deploy lifecycle commands:
```bash
# Print the currently active deployment version
ts active-version --adapter fastly --service-id <service-id>
# Probe a deployed version until it reports healthy
ts healthcheck --adapter fastly --service-id <service-id> \
--version <version> --domain edge.example
# Re-activate a previously active version
ts rollback --adapter fastly --service-id <service-id> \
--version <bad-version> --rollback-to <previous-version>
```
Inspect and verify deployments with the deploy lifecycle commands. All three are
Fastly-only — the axum, cloudflare, and spin adapters reject them:
```bash
# Capture the production rollback target BEFORE deploying: after a deploy this
# prints the NEW version, and Fastly keeps no record of which version was live
# before it, so the target is then unrecoverable.
ts active-version --adapter fastly --service-id <service-id>
# Probe a deployed version until it reports healthy. `<version>` is the version
# the deploy activated; pass `--service-id` to `ts deploy` and it emits that as
# a machine-readable `version=<N>` line.
ts healthcheck --adapter fastly --service-id <service-id> \
--version <version> --domain edge.example
# Re-activate the version captured before the deploy
ts rollback --adapter fastly --service-id <service-id> \
--version <bad-version> --rollback-to <previous-version>
```

Comment thread docs/guide/cli.md
Comment on lines +94 to +100
`config gc` sweeps every root in the selected physical store, so `--older-than`
is a safety assertion about the whole store: nothing in it changed within the
window and no writer is targeting it. Unlike the other `config` subcommands,
`gc` never loads the typed app config; its `--no-env` flag instead ignores
`EDGEZERO__STORES__CONFIG__<ID>__NAME` when resolving which physical store to
sweep. On a destructive run, check the store id `gc` reports before passing
`--yes`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

♻️ refactorconfig gc's destructive-target warning enumerates --no-env but omits --store.

This paragraph deliberately spells out that gc's --no-env retargets the physical store rather than skipping an overlay — that is the real footgun and it is good that it is called out. But ConfigGcArgs also carries #[arg(long)] pub store: Option<String> ("Override the config-store id (defaults to the manifest's)"), which retargets more directly and is both undocumented and untested. An operator who reads this paragraph as the complete list of ways --yes can hit the wrong store is wrong.

Suggested change
`config gc` sweeps every root in the selected physical store, so `--older-than`
is a safety assertion about the whole store: nothing in it changed within the
window and no writer is targeting it. Unlike the other `config` subcommands,
`gc` never loads the typed app config; its `--no-env` flag instead ignores
`EDGEZERO__STORES__CONFIG__<ID>__NAME` when resolving which physical store to
sweep. On a destructive run, check the store id `gc` reports before passing
`--yes`.
`config gc` sweeps every root in the selected physical store, so `--older-than`
is a safety assertion about the whole store: nothing in it changed within the
window and no writer is targeting it. Unlike the other `config` subcommands,
`gc` never loads the typed app config; its `--no-env` flag instead ignores
`EDGEZERO__STORES__CONFIG__<ID>__NAME` when resolving which physical store to
sweep, and `--store <id>` overrides the manifest's config-store id outright.
Both change which store gets swept, so on a destructive run check the store id
`gc` reports before passing `--yes`.

Comment thread docs/guide/cli.md
Comment on lines +114 to +118
`ts deploy` accepts `--staging` (Fastly only) to build and upload a staged
draft version cloned from the active one instead of activating a production
deploy. Adapter passthrough arguments must follow a `--` separator; unknown
flags before `--` (including the renamed-away `--stage`) are rejected at parse
time rather than forwarded:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

♻️ refactor — The deploy passthrough boundary is a breaking change, documented as if it were always the rule.

Upstream changed DeployArgs.adapter_args from #[arg(trailing_var_arg = true, allow_hyphen_values = true)] to #[arg(last = true)] — confirmed by diffing edgezero-cli/src/args.rs between 9e661ae and 5f3d648. So ts deploy --adapter fastly --comment "release" used to work and now fails at parse time.

It fails loudly, which is the right design, and the docs state the new rule correctly. The gap is that they read as a new safety property rather than as a migration. Nothing in this repo invokes ts deploy with passthrough args (only docs/guide/cli.md mentions it; no workflow or script does), so the blast radius is operator runbooks living outside the repo — exactly the audience for this page.

Suggested change
`ts deploy` accepts `--staging` (Fastly only) to build and upload a staged
draft version cloned from the active one instead of activating a production
deploy. Adapter passthrough arguments must follow a `--` separator; unknown
flags before `--` (including the renamed-away `--stage`) are rejected at parse
time rather than forwarded:
`ts deploy` accepts `--staging` (Fastly only) to build and upload a staged
draft version cloned from the active one instead of activating a production
deploy. Adapter passthrough arguments must now follow a `--` separator; unknown
flags before `--` (including the renamed-away `--stage`) are rejected at parse
time rather than forwarded. This is a change: passthrough args previously
worked without the separator, so existing runbooks and CI jobs that pass
adapter flags directly need the `--` added:

Comment on lines +113 to +116
// references needs the secret-field paths spelled out here (this
// hand-written impl does not inherit the derive's nested/array paths)
// plus operator migration work tracked separately.
fn secret_fields() -> Vec<edgezero_core::app_config::SecretField> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 praise — The reworded secret_fields() comment is now accurate.

Last round flagged that the retained comment named a blocker the upgrade had removed. 4aee0527 fixed it correctly rather than cosmetically: the derive does emit Field/ArrayEach paths at the pinned rev, and TrustedServerAppConfig hand-implements AppConfigMeta so it genuinely does not inherit them — which is exactly what the new wording says.

Also verified the migration itself is behavior-preserving: the trait has exactly one item in both revs (const SECRET_FIELDS: &'static [SecretField] -> fn secret_fields() -> Vec<SecretField>), Vec::new() does not allocate, and the only consumer is deploy-time validate_excluding_secrets (edgezero-core/src/app_config.rs:304), so an empty set remains a no-op. Secrets still ship plaintext in the pushed blob — unchanged by this PR and out of scope.

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.

Upgrade edgezero dependencies to the deploy-actions branch (PR #316)

2 participants