Repository navigation
Conversation
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…rsion on Fastly Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed cd704a7aacb4df866c471cfafe3cf6d130ad5fcb against a4e01eb55fe940bd02b2426dccd46050704c54c5. Requesting changes for the two P2 correctness issues documented inline. Both were reproduced in temporary fixtures without editing repository files.
Validation
The 10 resolver tests, 7 core header tests, and focused Fastly, Axum, Cloudflare, and Spin tests passed. Consecutive override builds emitted the requested versions without cleaning the target cache; removing the override restored the local branch value. All 20 reported CI checks pass.
The second pass exercised the shipped header helper with repository-locked dependencies and the unchanged build script in a packed-ref Cargo fixture. No duplicate review feedback was present. Full Viceroy HTTP smoke testing and live Cloudflare/Spin deployment were not independently repeated.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The compiled-in version and shared header helper provide consistent version reporting across adapters, and the Fastly service-version rename is documented. Two cases can still report a misleading deployed version: retaining an origin's header when the build version is unknown, and resolving Git metadata from an unrelated parent repository.
Verdict: REQUEST_CHANGES
Two inline comments contain one-click GitHub suggestion blocks. The third is a nonblocking improvement to a documented cache-invalidation limitation.
Blocking
- 🔧 Remove inherited version headers when the build version is unknown — see
crates/trusted-server-core/src/version_header.rs:37–39. - 🔧 Reject Git metadata from an unrelated parent repository — see
crates/trusted-server-core/build.rs:35.
Non-blocking
- 🤔 Improve invalidation when the current branch ref is packed — see
crates/trusted-server-core/build.rs:46–48.
Verification
All 10 resolver tests passed locally. Each one-click suggestion independently passed formatting, all eight target-matched clippy aliases, Fastly/Axum/Cloudflare compile checks, all four adapter test aliases, and the 14-test parity suite. Post-verification patches matched the approved suggestion bytes exactly. Targeted scratch probes verified inherited-header removal and correct repository-root handling for normal checkouts, linked worktrees, exported sources, and explicit overrides.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- Analyze (actions): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- Analyze (rust): PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- CLAUDE.md symlink guard: PASS
- vitest: PASS
- prepare integration artifacts: PASS
Remove an existing x-ts-version when the compiled-in version is unknown or invalid, so a proxied origin header is never reported as the deployed version. Ignore git metadata unless the repository top level is the workspace root, and watch the nearest existing ancestor of a packed current-branch ref so the next commit re-runs the build script. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed e6904260ab48917114d9b134bc7f5fbcfe826f6f against a4e01eb55fe940bd02b2426dccd46050704c54c5, from feat/git-version-header into main.
Inspected all 17 changed files and traced build inputs, header finalization, adapter conversions, configuration precedence, and consumers. The header rename is intentionally breaking and documented. No new actionable issues remain at this revision.
Safety proof
- Version resolution survives the tested cache and repository transitions. Executed 16 passing probes in temporary Cargo fixtures using the unchanged build script and resolver. These covered override changes/removal, tag creation, packed-branch advancement, detached HEAD, linked worktrees, exported sources, and unrelated parent repositories. Proven for these cases.
- Finalized responses report the compiled version rather than an inherited origin value. All 10 core header tests passed, including valid replacement and unknown/invalid removal. Focused tests passed for all four adapters, the Fastly header split, Fastly
/health, Axum's service path, and Spin's startup-fallback/health. Proven for these paths.
Findings
No meaningful findings introduced by this PR remain at the reviewed revision.
Validation and review context
Passed:
cargo test-fastly version_header
cargo test-fastly --test git_version_resolve
cargo test-fastly version_headers_split_git_and_fastly_versions
cargo test-fastly health_response_reports_git_version_only
cargo test-axum emits_git_version_header
cargo test-axum --test routes health_reports_git_version
cargo test-cloudflare emits_git_version_header
cargo test-spin emits_git_version_header
cargo test-spin startup_error_router_answers_health_with_200
cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity
cargo fmt --all -- --check
cargo clippy-fastly
cargo clippy-axum
cargo clippy-cloudflare
cargo clippy-cloudflare-wasm
cargo clippy-spin-native
cargo clippy-spin-wasm
Parity: 14 passed. The lint batch initially timed out during Cloudflare WASM compilation; the remaining aliases passed on rerun. Build-script fixtures executed cargo run --offline --quiet --manifest-path <temporary-workspace>/Cargo.toml; all 16 probes passed.
- CI:
gh pr checks 1213reports only one passing JavaScript/TypeScript CodeQL check at this head. The earlier revision's full green suite does not validate this revision. - Existing feedback: Reviewed bodies, inline comments, replies, issue comments, and thread resolution. All five prior threads are resolved; the three distinct defects are fixed and independently checked.
- Residual risk: Full CI and live Fastly, Cloudflare, and Spin HTTP deployment checks were not repeated. Missing Git metadata watch paths retain the documented invalidation limitation.
- No repository files were modified. The worktree remained clean, and head and base were rechecked before submission.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed e6904260ab48917114d9b134bc7f5fbcfe826f6f against merge base a4e01eb55fe940bd02b2426dccd46050704c54c5. No actionable findings remain. The compiled-in version is shared across adapters, the Fastly service-version rename is documented, and operator response-header precedence is preserved.
The latest commit addresses the previous inherited-header, unrelated-parent-repository, and packed-branch-ref findings. Header regression tests and temporary Cargo fixtures independently verified those behaviors.
Verdict: APPROVE
Validation
- 27 distinct focused Rust tests passed: 10 resolver tests, 10 shared header tests, and 7 adapter middleware/health tests.
- Cross-adapter parity suite: 14 passed.
- All eight target-matched clippy aliases passed, including CLI and codegen.
cargo fmt --all -- --checkpassed.- 15 temporary Cargo build/version probes passed, covering overrides, tags, packed refs, detached HEAD, linked worktrees, and exported sources inside unrelated repositories.
The initial sandboxed Viceroy run failed during macOS certificate loading; the retry with keychain access passed. The complete Rust test suites, standalone JS tests, docs formatting, and live deployment smoke tests were not repeated in this pass. Tracked repository files remain unchanged.
CI Status
- Analyze (javascript-typescript): PASS.
- Other CI gates: not run at this head according to the reported check set. Only the above check is reported; no required checks are reported. Earlier revisions' CI results do not validate this revision.
aram356
left a comment
There was a problem hiding this comment.
Summary
The resolver, the shared version_header helper, and the round-2 fixes hold up. All 14 CI-equivalent steps pass locally at e6904260a. The blocker is the merge state: the branch conflicts with main, so GitHub's test, format, and integration workflows have never run on this head.
3 of the 6 inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch to apply several at once). The other three describe the fix in prose, because it touches several files or lines outside the diff.
Blocking
🔧 wrench
- Rebase onto
mainso CI can run on the merged result: see Cross-cutting below
Non-blocking
🤔 thinking
- Fastly startup failures omit
x-ts-version: see inline atcrates/trusted-server-adapter-fastly/src/main.rs:72 - An ambient
TS_GIT_VERSIONcompiles in when the version is unknown: see inline atcrates/trusted-server-core/build.rs:112 - Document the build input and the header split for operators: see inline at
CHANGELOG.md:12
♻️ refactor
- Keep the testable helpers private: see inline at
crates/trusted-server-core/src/version_header.rs:18
⛏ nitpick
- The
OVERRIDE_ENVcomment overstates the CI checkout limitation: see inline atcrates/trusted-server-core/build.rs:11 - The spec header is stale: see inline at
docs/superpowers/specs/2026-09-25-git-version-header-design.md:3
Cross-cutting / body-level findings
🔧 Rebase onto main so CI can run on the merged result
GitHub reports this PR as conflicting with main. While that's true it can't build the pull_request merge ref, so Run Tests, Run Format, Integration Tests, and CodeQL Advanced have not run for e6904260a. The only check on this head is the dynamic CodeQL JavaScript analysis. Both approvals call out the same gap. Three files conflict:
crates/trusted-server-core/build.rs: #879 mademain()loadedgezero.tomland emitTRUSTED_SERVER_DEFAULT_CONFIG_STORE_ID, withedgezero-coreas a build-dependency. Keep both bodies. Puttingmain's manifest block first keeps itscargo::error=exit ahead of the version lookup.crates/trusted-server-adapter-fastly/src/main.rs: imports only. Keepcache_policy::{EdgeCacheHeader, cache_control_headers_have_directive}from #1179 next toconstants::HEADER_X_TS_VERSION.health_responseis unchanged onmain.CHANGELOG.md: keep both sides'### Changedentries.
The merged build.rs header and main() shape:
use std::env;
use std::path::{Path, PathBuf};
use std::process::Command;
use edgezero_core::manifest::ManifestLoader;
use git_version::{Candidates, is_usable, resolve_git_version};
// ... OVERRIDE_ENV, git(), rerun_if_exists(), resolve_from_local_git() unchanged ...
fn main() {
println!("cargo:rerun-if-changed=build.rs");
println!("cargo:rerun-if-changed=build_support/git_version.rs");
println!("cargo:rerun-if-env-changed={OVERRIDE_ENV}");
// main's edgezero.toml block, unchanged, ending with:
// println!("cargo:rustc-env=TRUSTED_SERVER_DEFAULT_CONFIG_STORE_ID={default_store_id}");
// this PR's override / local-git resolution, unchanged
}I resolved the conflicts this way in a scratch merge with main at 182fdf45c. It passes fmt, clippy-fastly, test-fastly, test-fastly-reuse, test-axum, check-cloudflare, test-cloudflare, test-spin, and parity (17/17), and the merged build script emits both TRUSTED_SERVER_DEFAULT_CONFIG_STORE_ID and TS_GIT_VERSION. Once merged, both halves derive the workspace root from CARGO_MANIFEST_DIR: one with ancestors().nth(2), the other with parent()?.parent()?. Computing it once and passing it to resolve_from_local_git would remove the duplicate.
CI Status
- Analyze (javascript-typescript): PASS
- Run Tests, Run Format, Integration Tests, CodeQL Advanced: not run at this head (no merge ref while the PR conflicts)
- Local results at
e6904260a:cargo fmt --check, all eight clippy aliases,test-fastly,test-axum,test-cloudflare,test-spin, and parity (14/14) PASS. The docs and root Markdown Prettier checks PASS. There are no JS changes, so vitest was not run.
Always re-emit the validated value under the same name so an unvalidated ambient value never reaches option_env!, and treat an empty value as unknown. Report x-ts-version on Fastly responses whose finalization is skipped because settings fail to load, drop an inherited x-ts-fastly-version when FASTLY_SERVICE_VERSION is unset or invalid, make the version_header test helpers private, and document the build input and header split for operators. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me> # Conflicts: # CHANGELOG.md # crates/trusted-server-adapter-fastly/src/main.rs # crates/trusted-server-core/build.rs
|
Merged Also in 50ad004: an inherited |
Summary
x-ts-versionnow reports the deployed git version: the tag, else the branch, else exactly the first 6 characters of the commit. It is compiled in from the build-timeTRUSTED_SERVER__GIT_VERSION(set by the deploy pipeline), falling back to local git, and is omitted when neither is available. The validated value is always re-emitted under the same name (empty when unknown), so an unvalidated ambient value never reaches the binary.x-ts-fastly-version; an inherited origin value is removed whenFASTLY_SERVICE_VERSIONis unset or invalid. Under Viceroy,maintoday answers withx-ts-version: 0, which is the Fastly version, not a Trusted Server one. Breaking for anything that readsx-ts-versionas the Fastly version number.x-ts-version, including the FastlyGET /healthfast path that deploy health checks probe and Fastly responses whose finalization is skipped because settings fail to load. Operatorresponse_headersstill apply last.Changes
crates/trusted-server-core/build.rsTRUSTED_SERVER__GIT_VERSION, read byconstants::TS_GIT_VERSION;rerun-if-env-changed=TRUSTED_SERVER__GIT_VERSION; watch the worktreeHEAD, the current branch's ref,refs/tagsandpacked-refs(existing paths only; other branches andrefs/remotesskipped)crates/trusted-server-core/build_support/git_version.rs#[path]; visible-ASCII rulecrates/trusted-server-core/tests/git_version_resolve.rscrates/trusted-server-core/src/constants.rsHEADER_X_TS_FASTLY_VERSION,TS_GIT_VERSIONcrates/trusted-server-core/src/version_header.rsgit_version_header_value,apply_git_version_header(+ private testable variants)crates/trusted-server-core/src/lib.rsversion_headercrates/trusted-server-adapter-fastly/src/middleware.rsx-ts-version,FASTLY_SERVICE_VERSIONtox-ts-fastly-version(removing an inherited value when unset or invalid); doc commentscrates/trusted-server-adapter-fastly/src/main.rs/healthfast path and the finalize-skipped path setx-ts-versioncrates/trusted-server-adapter-{axum,cloudflare,spin}/src/middleware.rsapply_git_version_headerbefore operator headerscrates/trusted-server-adapter-axum/tests/routes.rs,crates/trusted-server-adapter-spin/src/app.rs/healthroute tests assert the headerdocs/guide/getting-started.md,docs/guide/edgezero.mdTRUSTED_SERVER__GIT_VERSIONwhen deploying, and a post-deployx-ts-versioncheckdocs/guide/first-party-proxy.mdX-TS-VersionCHANGELOG.mddocs/superpowers/{specs,plans}/2026-09-25-git-version-header*.mdCloses
Closes #1212
Related: #325 (introduced
x-ts-versionas the Fastly version)Test plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spinclippy-fastly,-axum,-cloudflare,-cloudflare-wasm,-spin-native,-spin-wasm,-cli,-codegen)cargo fmt --all -- --checkcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycd crates/trusted-server-js/lib && npx vitest run(1130 passed; no JS changes. Run withNODE_OPTIONS=--no-webstoragelocally, because Node 26's built-inlocalStorageshadows jsdom's. The repo pins Node 24.12.0, where this doesn't apply.)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve: offline end-to-end under Viceroy (below)End-to-end under Viceroy. Release Wasm served with
fastly compute serve --file <wasm>and a local config pushed withts config push --adapter fastly --local, set up the same way asscripts/smoke-fastly.sh. Each case curlsGET /healthandGET /and asserts200.x-geo-info-availableon/proves the response went throughapply_finalize_headers. Viceroy reportsFASTLY_SERVICE_VERSION=0./healthx-ts-version/x-ts-version/x-ts-fastly-versionTRUSTED_SERVER__GIT_VERSION=v9.9.9-testv9.9.9-testv9.9.9-test0…=v9.9.9-test2, no clean; Cargo: "the env variable TRUSTED_SERVER__GIT_VERSION changed"v9.9.9-test2v9.9.9-test20feat/git-version-header0HEAD00.git, unset0It also checks the rebuild triggers. A new tag alone triggers a rebuild through
refs/tags. Moving the current branch off a tagged commit triggers one through the branch ref and switches the value from the tag to the branch. A simulated fetch (git update-ref refs/remotes/...) and a ref update on another branch both leave coreFresh. An override ofv1-étéprints acargo:warningand falls back to local git.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — this repo useslogper AGENTS.md; the onlyprintln!calls are thecargo:directives inbuild.rs