Uh oh!
There was an error while loading. Please reload this page.
Add Fastly deploy action with config push - #303
Conversation
094a3b5 to
9debe90Compare
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
The Fastly deploy composite action is well-built and its credential-handling discipline is genuinely strong (every non-deploy step clears 15 FASTLY_* env aliases, the token is scoped to the Deploy step only, argv is NUL-delimited, the Fastly CLI is pinned by version + SHA-256, and paths are confined to github.workspace). CI passes locally (fmt, clippy, edgezero-core tests = 419, the action's own tests/run.sh, and shellcheck). The changes requested below are about scope, robustness, and hygiene — none are correctness bugs, but they affect reviewability and the durability of the action's security controls.
Findings
Requested changes
🤔 Undocumented scope creep bundled in —
crates/edgezero-core/src/key_value_store.rs(~+200 lines) adds a KV operation-timing / observability feature that is unrelated to a "Fastly deploy action," is not mentioned anywhere in the PR body, and rides inside the single commit titled "Add Fastly deploy GitHub action." Please split it into its own PR/commit (or at least describe it in the body). The code itself is fine —web-time/logare already unconditional core deps, and the debug-timing is zero-cost when debug logging is off — but bundling an unannounced core-crate feature into an infra PR hurts reviewability and bisectability.🤔
deploy-argsdenylist is fragile —validate-inputs.shrejects a fixed set of long flags but not short flags (-s,-t,-v) or future Fastly flags. Since it's presented as a security control, an allowlist (fails closed) is sturdier than a denylist (fails open). See inline. (non-blocking; trusted caller input today)⛏
python3used beforerequire_cmd—validate-inputs.shline 47. See inline. Also flags an opportunity to drop Python from the SHA-256 and path-confinement helpers in favor of coreutils (sha256sum,realpath).⛏ Personal absolute path committed —
docs/specs/trusted-server-deployer-migration-plan.md:6. See inline.
Informational
📝 Stale PR description — the body advertises
edgezero-cli config push --adapter fastlyand "Fastly Config Store upsert support," but both already exist onmain; this diff touches neither the CLI nor the fastly adapter. Please trim the body so reviewers aren't hunting for code that isn't in the diff.📝 Incidental
Cargo.lockchurn — bumpsbrotli,chrono,handlebars,http, and pullsindexmapin underserde_json(preserve_orderbecame transitively enabled). Unrelated to the action; fine to keep, just noting it's bundled.
CI Status
- fmt: PASS
- clippy (edgezero-core, --all-features): PASS
- tests (edgezero-core --all-targets): PASS (419)
- deploy action contract tests (
tests/run.sh): PASS - shellcheck: PASS (SC1091 info only)
Note: full-workspace clippy/test not re-run locally; only the crate touched by this diff (edgezero-core) plus the action's own gates were exercised.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
A composite Fastly deploy action (shell) plus docs. The security design is strong, but the action is currently non-functional: it invokes a CLI binary name that the crate never produces, and the contract test hides the defect. Three CI checks fail. Requesting changes.
😃 Praise
- Excellent credential threat model: every non-deploy step zeroes 15
FASTLY_*env aliases,clear_fastly_aliasesruns before the CLI is invoked, and the Fastly CLI download is checksum-verified before extraction. - Defense in depth on supply chain and inputs: full-SHA pinning enforced by a custom check + zizmor + actionlint, dirty-tree rejection, path-traversal containment (
is_under), and NUL-delimited arg passing that preserves argument boundaries.
Findings
Blocking
- 🔧 CRITICAL — binary name mismatch, action cannot run: crate bin is
edgezero(crates/edgezero-cli/Cargo.toml:11-13); scripts calledgezero-cli.composite-smokefails atinstall-edgezero.sh:23. Also affectsinstall-edgezero.sh:26andrun-edgezero.sh:13,26,43. See inline comments. - 🔧 Contract test masks the bug:
tests/run.sh:208shims a fakeedgezero-cli, so the wrong binary name passes CI. See inline. - 🔧 New actionlint gate fails on pre-existing lint:
deploy-action.yml:37lints all workflows and flags SC2086 informat.yml:37,98,128,test.yml:38,122,deploy-docs.yml:33(files untouched by this PR) →contract-testsfails. See inline. - 🔧 Docs fail
prettier --check→format-docsfails:docs/guide/deploy-github-actions.mdanddocs/specs/edgezero-deploy-github-action.md. Runprettier --writeon both and commit.
Non-blocking
- 🤔 Cache key includes
source-revision(resolve-project.sh:142) → every new commit is a guaranteed cache miss;target/cache only reused on identical-commit re-runs (and moot by default since fastlyauto→never). See inline. - 🤔 deploy-args guard is a denylist (
validate-inputs.sh:98) → incomplete against short/alternate flag spellings. See inline. - 🤔 actionlint pinned by tag
@v1.7.7, not SHA (deploy-action.yml:37) — the PR's own full-SHA policy covers onlyuses:. - ⛏
provider-cli-versionoutputs the pinned version, not the installed one (install-fastly.sh:48-51).
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test (+ wasm): PASS
- Deploy action / contract-tests: FAIL (actionlint on pre-existing workflows)
- Deploy action / composite-smoke: FAIL (binary name mismatch)
- Run Format / format-docs: FAIL (prettier)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid, security-conscious Fastly-only v0 deploy action with thorough spec, docs, and contract tests. Two blocking items: the shellcheck CI failure and a stale PR title; everything else is minor polish.
😃 Praise
- Credential hygiene is genuinely strong: the token-presence boolean computed in the expression (
action.yml:72) keeps the secret out of the validation step env entirely; the ruby metadata test enforces alias-clearing on every non-Deploy step so future steps can't regress; NUL-delimited arg files preserve argument boundaries with zeroeval; annotations are escaped and outputs reject newlines.
Findings
Blocking
- 🔧 Shellcheck CI failure: see inline comment on
.github/workflows/deploy-action.yml:72— fix verified locally withshellcheck -x -P SCRIPTDIR. - 🔧 PR title stale: the title says "with config push" but this PR contains no config push — it is an explicit non-goal (
docs/guide/deploy-github-actions.md:198, spec §4 item 6: "push runtime config or secrets to Fastly"). Rename to e.g. "Add Fastly deploy GitHub action".
Non-blocking
- 🌱 Test gaps vs spec §17.2:
tests/run.shlacks tests the spec lists as required: symlink-escape rejection, NUL/non-string argument rejection, invalid boolean, cache-enabled-missing-Cargo.lockfailure, andbuild-mode: alwaysresolution. Good follow-up candidate. - 📝 Cache only hits re-runs of the same commit: the key includes the source revision, so
cache: truenever reuses across commits. This is documented and spec'd, but callers commonly expect cross-commit reuse — worth stating explicitly in the caching section of the guide.
CI Status
- fmt: PASS (local)
- clippy: PASS (PR CI)
- tests: PASS (local, workspace all-targets)
- contract-tests: FAIL (shellcheck SC1091 — fix above)
- composite-smoke: PASS
| sudo apt-get update | ||
| sudo apt-get install -y shellcheck | ||
| - name: Shellcheck deploy action scripts | ||
| run: shellcheck .github/actions/deploy/scripts/*.sh .github/actions/deploy/tests/run.sh |
There was a problem hiding this comment.
🔧 Shellcheck CI failure — missing -x -P SCRIPTDIR: SC1091 is info-level but shellcheck exits 1 on any finding. The # shellcheck source=common.sh directives resolve relative to the CWD, not the script dir, so plain -x also fails (common.sh: openBinaryFile: does not exist). Verified locally (shellcheck 0.11.0, exit 0):
run: shellcheck -x -P SCRIPTDIR .github/actions/deploy/scripts/*.sh .github/actions/deploy/tests/run.shBonus: common.sh then actually gets followed and linted in each caller's context instead of being skipped.
| require_cmd rustup | ||
| rustup toolchain install "$RUST_TOOLCHAIN" --profile minimal | ||
| rustup target add "$TARGET" --toolchain "$RUST_TOOLCHAIN" | ||
| append_env RUSTUP_TOOLCHAIN "$RUST_TOOLCHAIN" |
There was a problem hiding this comment.
🤔 RUSTUP_TOOLCHAIN leaks past the action: append_env writes to GITHUB_ENV, so the application toolchain pins all subsequent caller job steps after this action finishes, and cleanup does not undo it. A caller's later cargo steps would silently compile with the app's toolchain. Consider documenting this side effect in the guide, or passing the toolchain to the build/deploy steps via step env: (from the resolve output) instead of GITHUB_ENV.
| export PATH="$TOOL_ROOT/bin:$PATH" | ||
| PROVIDER_CLI_VERSION=$(fastly version 2>/dev/null || fastly --version 2>/dev/null || true) | ||
| PROVIDER_CLI_VERSION=${PROVIDER_CLI_VERSION%%$'\n'*} | ||
| [[ -n "$PROVIDER_CLI_VERSION" ]] || fail "installed Fastly CLI did not report a version" |
There was a problem hiding this comment.
♻️ Installed version never checked against pin: this only asserts the version string is non-empty. A wrong archive at the pinned URL (matching checksum but wrong contents is unlikely — but a stale mirror or bad versions.json edit is not) would pass silently. Suggest:
[[ "$PROVIDER_CLI_VERSION"==*"$VERSION"* ]] || fail "installed Fastly CLI reports '$PROVIDER_CLI_VERSION', expected version $VERSION"⛏ Related: action.yml:53 describes the provider-cli-version output as a "semantic version", but the value is the full first line of fastly version (e.g. fastly v15.1.0 (...)). Adjust the description or trim the value.
| fi | ||
| ACTUAL=$(sha256_file "$ARCHIVE") | ||
| [[ "$ACTUAL" == "$SHA256" ]] || fail "Fastly CLI checksum mismatch for version $VERSION" |
There was a problem hiding this comment.
⛏ Corrupt cached archive survives checksum mismatch: on mismatch the archive stays in downloads/, so a retry in the same temp dir reuses the corrupt file and fails again without redownloading. Delete before failing:
[[ "$ACTUAL"=="$SHA256" ]] || { rm -f "$ARCHIVE"; fail "Fastly CLI checksum mismatch for version $VERSION"; }| - [Configuration](https://stackpop.github.io/edgezero/guide/configuration) - `edgezero.toml` reference | ||
| - [CLI Reference](https://stackpop.github.io/edgezero/guide/cli-reference) - All CLI commands | ||
| - [Blob App-Config Migration](https://stackpop.github.io/edgezero/guide/blob-app-config-migration) - Typed `AppConfig<C>` extractor + `config push` / `config diff` workflow | ||
| - [Deploy with GitHub Actions](https://stackpop.github.io/edgezero/guide/deploy-github-actions) - Full-SHA-pinned Fastly deploy action |
There was a problem hiding this comment.
⛏ Stale wording after ref relaxation: "Full-SHA-pinned Fastly deploy action" contradicts 153ca54, which relaxed the action to accept any trusted ref with SHA pinning only recommended. Suggest "Fastly deploy action (full-SHA pinning recommended)".
| } | ||
| # action metadata parses, exposes the expected v0 API, and clears provider env in non-deploy steps. | ||
| ruby - "$ACTION_DIR/action.yml" <<'RUBY' |
There was a problem hiding this comment.
⛏ Missing dependency guard: the suite hard-requires ruby (here) and jq (via the scripts under test); if either is absent the failure is confusing. A small guard up front keeps it actionable:
forcmdin ruby jq git;docommand -v "$cmd">/dev/null 2>&1|| fail "required command '$cmd' not found"done- ShellCheck: exclude SC1091 (can't follow the dynamic $SCRIPT_DIR/common.sh source from repo root — an info finding, not a defect). zizmor now passes via the inline unpinned-uses ignore. - Smoke fixture: the real Fastly CLI (installed by install-fastly) shadowed the fake and errored on a missing package. Replace it with an edgezero.toml Fastly deploy-command override (the proven #303 approach) that records the passthrough argv; assert the typed --service-id (dummy-service) threaded through.
aram356
commented
Aug 14, 2026
Superseded by #316 |
Summary
.github/actions/deploy.Validation
.github/actions/deploy/tests/run.shbash -n .github/actions/deploy/scripts/*.sh .github/actions/deploy/tests/run.shgo run github.com/rhysd/actionlint/cmd/actionlint@03d0035246f3e81f36aed592ffb4bebf33a03106zizmor --offline .github/actions/deploy/action.yml .github/workflows/deploy-action.ymlcd docs && npm run format && npm run lint && npm run buildcargo fmt --all -- --checkcargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spingit diff --checkNotes
Use the commit SHA from this PR for pre-release action pinning while testing. If this PR changes before merge, update downstream pins to the latest desired commit SHA; for production, prefer the final merged commit SHA.