Repository navigation
Conversation
Port the remaining slice 2.3 resource-discovery change from #1262 at 122b786. Preserve the build facts already carried in #1435. Verify the three pure filesystem tests in development and release-build configurations with exact-name selection guards. CLI clippy, Rust formatting, domain lint and independent review pass.
Record Gate 1 for user approval: real A/B comparison and rebuild-check publication preserve Phase 1 identities while qualification remains closed. The narrower ordinary-release assurance claim requires explicit approval before implementation.
Record the shared runner Cargo home as the cargo-home-unvaried gap and describe it accurately, and document that the gate runs on every push to main and every v* tag. Let the signer accept a companion sealed in an earlier attempt, retry transient read-only transfer failures, and report rebuild mismatches and identity failures as such. Pin the signing validator, comparison, measurement, npm-lock and post-reap checks the review found unpinned, forbid continue-on-error on the gate jobs, and tie the closed cohort to the published binaries. Rename release environment names to TRUSTED_SERVER__RELEASE_*, reject `..` in both helper TMPDIR guards, move the index schema into the release crate, and drop fixture producers no job runs. Refresh stale status across the guide, CHANGELOG, spec and plans.
|
Replies to the review's body-level findings, all in "Producer CI" doesn't exist. The ignore reasons now say "frozen #1262 producer test, no workflow runs it". The qualification README's staging note now says its text is the frozen Stale status. The spec status now keeps only the design contract and points to #1449 comments and #1235 for rehearsal and approval state. The three plans have a dated current-state note: four binaries and 18 files, no post-publish byte comparison, the gate on every Closed-path defects. Not fixed here, because Smaller items.
This round's verification (macOS arm64, Rust 1.98.1, Node 24.20.0):
Fork rehearsal |
Dependency sources under the shared runner Cargo home stayed unmapped in the final wasm, so A/B equality depended on that shared path. Each role now builds with a fresh Cargo home beside its target root, both mapped to /cargo-home. Observations record the Cargo home, the recorder requires every resolved package to come from the checkout or that home, and the binary scan covers all six physical roots. The cargo-home-unvaried gap is removed.
Pin the job interval, compare-time target, variant, locale, timezone and acquisition label, and the signing job's asset allowlist, identity formats and size bounds; each new case asserts its own guard. Remove release-crate items no caller or test uses: the DenialReceipt accessors, index_identity, read_final_bytes, the startup diagnostic_for and the composition projection fixtures.
Pin the current lock's unreviewed install scripts so the next hook-carrying bump fails visibly, check the pinned Node before the signing job validates its inputs, and remove release-crate items that no caller or test references.
|
The optional and "consider" items from the review body are in
Two items stay as they are. Isolating the euid check needs a fixture owned by another user, which needs root Fork rehearsal |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Requesting changes for the two reachable issues below. The scanner-budget comment is future-facing and is not, by itself, an ordinary-release blocker.
Reviewed commit: 0b1e350e8cbcb0df8b3ab2addfa8c3aa6ac2d47d.
Coverage: an incremental review of the updates and live release paths, not an exhaustive reading of the full 306-file PR (approximately 94,000 added lines).
Validation at the reviewed commit:
- All 30 remote CI checks passed.
- The locked/offline native release-producer build passed.
- 127 focused Rust tests and 43 release-script Node tests passed.
- The pinned-range whitespace check passed.
- The package allocation failure and the live ordinary-measurement signal-handling issue were reproduced.
All three comments are prose-only, with no GitHub suggestions. Proposed fixes were not applied or verified.
Check the manual package module against its independently approved SHA-256 before the Wasm parser runs, so an unapproved module cannot drive parser allocations. Restore the default SIGCHLD action at startup and refuse every spawn while an ignored SIGCHLD or SA_NOCLDWAIT would let the kernel reap the leader. On Linux an inherited ignored SIGCHLD made ordinary-measure's tool queries fail with ECHILD and signal a process group the leader no longer pinned.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 3 at 37dfc6d28. Thanks for working through the whole round-2 list. We re-checked every claimed fix against the code, re-ran the round-2 mutants, and checked the private Cargo homes in the published .33 and .34 wasm: no /home/runner path is left, and every dependency path reads /cargo-home (3,417 registry and 119 git lines). ChristianPavilonis's two fixes hold too: on Linux the inherited-SIG_IGN command test fails with the fix removed, and a mismatched package is rejected before parsing. What's left is small: a few new checks that tests still don't pin (most importantly the recorder's package-location check), a tagging checklist that the new "every main push" sentence relies on but that doesn't name the comparison runs, a description that calls an unrehearsed live-path change rehearsed, and doc leftovers.
10 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. Each was applied alone to37dfc6d28and checked with the matching gate (Rust:cargo fmt --check, release-crate clippy with-D warningsand the full release-crate suite; Node and workflows: prettier, the full release Node suite andactionlint; docs: prettier), and all ten were checked together, with no other file changes. The test suggestions were also run against the mutants they target and fail on each. The remaining comments describe the fix in prose because it spans files or lines outside the diff.
Blocking
🔧 wrench
- The new "every package from the checkout or the private Cargo home" check can be deleted with the suite green — see inline at
crates/trusted-server-release/src/ordinary/inputs/record/tests.rs:458 - Two signer checks are still unpinned, one of them added by the bounded-attempt change — see inline at
.github/workflows/release-build.yml:534 - This sentence relies on a tagging checklist that doesn't name the comparison check runs — see inline at
docs/guide/release-verification.md:128 - The description says the current head is
0b1e350e8and exact-head rehearsed — below
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note
- 🤔 The Cargo-home proof reads a separate
cargo metadatarun, not the build — see inline atcrates/trusted-server-release/src/ordinary/inputs/record.rs:244 - ♻️ The archive retry policy and the real backoff/deadline path aren't pinned — see inline at
scripts/release/phase1.test.mjs:242 - ♻️ The fail-closed contract still allows
continue-on-erroronchecksand on the CLI build step — see inline atscripts/release/phase1.test.mjs:2482 - ⛏ This step goes green with zero tests if the test is renamed — see inline at
.github/workflows/test.yml:398 - ⛏ The Fastly jobs still pass the runner's Cargo home, which the script then discards — see inline at
.github/workflows/release-build.yml:110 - ⛏ The Node floor is 22.18, not 24.2; my round-2 comment overstated it — see inline at
CHANGELOG.md:17 - ⛏
SA_NOCLDWAITdoesn't surviveexec, and its branch is unpinned — see inline atcrates/trusted-server-release/src/process/supervise.rs:132 - ⛏
CARGO_HOMEis matched by path components, not by its exact spelling — see inline atcrates/trusted-server-release/src/ordinary/normalization.rs:140 - ⛏ The recipe policy still describes four maps — see inline at
crates/trusted-server-release/src/ordinary/normalization.rs:307 - ⛏ No test pins the new error variants — see inline at
crates/trusted-server-release/tests/commands.rs:1196 - 📝 The three owner checks aren't pinned individually — see inline at
crates/trusted-server-release/src/ordinary/inputs/record.rs:217 - ⛏ Doc leftovers after the round-2 cleanup — below
- 📝 Phase 3 still uses helpers this PR deleted — below
- 📝 Smaller items — below
Cross-cutting / body-level findings
-
🔧 The description says the current head is
0b1e350e8and exact-head rehearsed. The head is37dfc6d28, which changes live release code:mainnow resets SIGCHLD before any child exists,spawn_grouprefuses unwaitable children for everyBoundedCommand(so forordinary-measure,inspect-fastly-packageandpreflight), andpackage.rschecks the digest before parsing. Nothing has been rehearsed at that head, and the checklist still ticks "Current0b1e350e8… exact-head rehearsal.34". Please rehearse37dfc6d28(or say plainly that it isn't rehearsed) before merge. Also in the description: every rehearsal is said to pass "all 13 jobs", but those runs have 12 (checks, four builds, reproduction, comparison, four signing legs, release; for example run 42789411060 reportstotal_count: 12, and the Gate 1 plan line 128 says the same for.27), and the local count is now 987 + 4 + 20. -
⛏ Doc leftovers after the round-2 cleanup.
- "Keep #1449 draft, without reviewers." is still at
binary-attestations-phase2-fastly-integration.md:220, and "Keep #1449 draft and request no reviewers." atbinary-attestations-phase2-fastly-integration-implementation.md:177. binary-attestations-phases-2-4.md: line 39 still says the Fastly build script and the build-info module are "already landed in #1435" and that the CLIlib.rs/run.rs"carry the build-info re-export", while ledger rows 245 and 252 say both were removed; line 65 counts "22 carried documentation/evidence paths" (the ledger has 2 carried, 1 moved, 19 not carried); rows 136-156 keep "1 — carried" in the Landing column although their State says "Not carried"; line 32 saysfixture_for_projectionremains, but092de447bdeleted it.- "The frozen lifecycle observer remains byte-identical" (
binary-attestations-phases-2-4.md:582) and "The observer is ported unchanged" (binary-attestations-phase2-fastly-integration.md:162) are no longer true:f243408cerewrotescripts/release/observe-npm.mjs's deletion regex at our request. Reword both, or restore the frozen bytes. binary-attestations-slice-map.json:615still ends "Phase 1 publishes only 4 per executable plus SHA256SUMS: 29." two lines belowphase1_release_files: 17.binary-attestations-phase2-fastly-integration-implementation.md:50says a partial rerun carrying old successful jobs fails closed and to rerun all jobs;ordinary/run.rs:50-57and the guide (lines 207-211) now say carried-over binding is intended.docs/guide/release-verification.md:419(outside the diff) says "The eight pre-creationgh attestation verifyruns";publish.mjsnow runs nine (two per binary plus the rebuild check).- Four
#[ignore]reasons don't use the new wording:recipe/startup.rs:626still says "requires explicit ordinary CI CLI fixture", which implies CI provides it, andmaterials/npm_bundle.rs:261,materials/git/tests.rs:748andrecipe/apple/sdk.rs:545don't say no workflow runs them.
- "Keep #1449 draft, without reviewers." is still at
-
📝 Phase 3 still uses helpers this PR deleted. At the #1463 head (
origin/feature/binary-attestations-phase3,d885a965a),distribution.rs:244callsindex_identity(), andcomposition/native/tests.rs:656-792plus the frozenspdx/native/tests.rsusefixture_for_projectionandfixture_native_registry, all removed in092de447bat our round-2 suggestion. The next merge of Phase 2 into Phase 3 won't compile unless they're restored there; worth a line in the phases plan so it isn't a surprise. -
📝 Smaller items.
- The shell-side Cargo-home wiring (
phase1-build.sh:33-37, and the capture'srealpathSync) has no PR-CI coverage: deleting the export, the refusal or the per-role name passes the Node suite. Each fails closed on the nextmainpush throughnormalization.rs:136-142, so this is optional; oneassert.matchon the export, ordered beforeordinary-rustflags, would catch it in PRs. tool()maps every process failure, including the newUnwaitableChildren, to "ordinary measurement inputs or tool pins are invalid" (record.rs:303), which is what the pre-fix Linux run printed.ProcessFailurekinds carry no child output, so attaching the kind would be safe and would make that failure self-explaining.- Module-level
cfg_attr(not(test), expect(dead_code))flags a module that becomes fully live, butprocess,documents,sourceandinspectare partly live, so their remaining dead items stay hidden. Item-level attributes would be needed to see them; not asking for that here.
- The shell-side Cargo-home wiring (
Round-2 follow-up
Verified fixed at this head: the private remapped Cargo home (no runner path in .33/.34), the every-main-push wording, the signer's receipt, attempt, allowlist, format, size and Node checks (except the two in the comment on release-build.yml:534), the canonical-side, start-time, Node-pin, name-uniqueness, compare-scan, rustc-pin, tool, euid, identity and npm-lock tests, the post-reap no-SIGKILL test (it fails on the restored SIGKILL under Linux), the module doc, the error variants and Usage text, the carried-over binding doc, the TRUSTED_SERVER__RELEASE_* renames (no setter/reader mismatch, no gate silently skipped), the dead-code attributes and deletions (clean on macOS and Linux), the schema move, the retries, the continue-on-error jobs we listed, the cohort tie, the .. guard, the CLI release-cfg step, the npm notifier, the signing-job Node threat note, the CHANGELOG, the offline block, the spec anchor, the producer-CI wording and links (all 13 pinned links resolve at 122b78664), and the esbuild/fsevents install-hook pin. Closed-path items are tracked in #1517.
CI Status
At 37dfc6d28: 30 of 30 checks pass.
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- trace runtime and browser acceptance: PASS
- CodeQL: PASS
- prepare integration artifacts: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo fmt: PASS (required)
- cargo test (cross-adapter parity): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- vitest: PASS
- HTML EOF runtime (Fastly): PASS
- ts dev lint domains (changed lines): PASS
- CLAUDE.md symlink guard: PASS
- cargo test (axum native): PASS
- worker-deadlines (cloudflare): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- docs-proposal scripts: PASS
- worker-deadlines (spin): PASS
- release scripts: PASS
- format-docs: PASS (required)
- CI Node pin guard: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
Tie the Cargo-home proof to the build's own compiler-artifact messages, match CARGO_HOME by its exact spelling, name the Cargo-home prefix in the v2 recipe policy and stop passing the runner's Cargo home to the Fastly jobs. Pin the recorder's dependency-source check, the fractional signer attempt, the SPDX size bound, a file-wide continue-on-error ban, the archive retry policy and deadline, the SA_NOCLDWAIT refusal, the reproducibility error variant, the private Cargo-home wiring and the release-cfg test count. Correct the Node floor to 22.18/24.2, the pre-tag checklist, the verification count and the stale plan, ledger and ignore-reason text.
|
Round-3 body-level items, addressed in Description / rehearsal. Agreed. Doc leftovers: all fixed.
Phase 3 helpers: Smaller items
Local checks at
Linux-only paths are left to CI. |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 4 at e9e46e584, now against main. Every round-3 fix checks out:
- Fixes: I re-ran each mutant at this head (dropping the recorder's package-location call or its build-message clause, the signer's integer and SPDX bounds,
continue-on-erroronchecks, the exactCARGO_HOMEspelling, theSA_NOCLDWAITarm, the mismatch variant, and the shell's per-role Cargo home), and the suite fails for each. - The
mainmerge kept every change from both sides: the PR's patch againstmainequals its pre-merge patch, apart from context lines. - Rehearsal:
.39was rehearsed at this exact head.
What's left is mostly a scope question: about two-thirds of the added code is producer code no release runs, and it brings the dependency that moves the shipped wasm's crates. There's also a design suggestion for a much smaller rebuild check, a CI rerun, and one stale number.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. It was applied alone toe9e46e584and checked withprettier --config docs/.prettierrc, with no other file changes. The remaining comments describe the change in prose because it spans many files.
Blocking
🔧 wrench
- Two Integration checks failed — below
❓ question
- Can the unrun #1262 producer code, and the
jsonschemadependency it brings, land with the phase that turns it on? — see inline atcrates/trusted-server-release/src/main.rs:12
Non-blocking
🤔 thinking / ⛏ nitpick
- 🤔 The rebuild check could be a GitHub-native attestation over in-run artifacts — see inline at
.github/workflows/release-build.yml:339 - ⛏ Run
.27had 12 jobs, not 13 — see inline atdocs/superpowers/plans/binary-attestations-phase2-fastly-integration.md:128
Cross-cutting / body-level findings
- 🔧 Two Integration checks failed:
browser integration testsandtrace runtime and browser acceptance. Neither is a required check, and both logs show an infrastructure failure rather than a test failure: apt couldn't resolveazure.archive.ubuntu.com("Temporary failure resolving") while installing packages, on both attempts of run 43038396012. The PR's only changes in that area are doc comments (browser/helpers/infra.ts,tests/parity.rs,adapter-spin/tests/routes.rs), so I don't expect a real failure; please re-run them once the mirror is back, so the head is green before merge.
Round-3 follow-up
Verified fixed at this head:
- Recorder: the package-location test, the build-message check, and the exact
CARGO_HOMEmatch. - Signer: the
oversized-spdxandfractional-attemptmodes. - Test and CI guards:
- the file-wide
continue-on-errorcontract; - the retry tests (the archive-status mutants and the backoff mutants now fail);
- the CLI release-cfg step's
1 passedguard; - the shell-side Cargo-home wiring test.
- the file-wide
- Docs and doc comments:
- the tagging checklist, which now names Reproduce and Compare;
- the
SA_NOCLDWAITdoc and its test; - the
ordinary-rust-path-remap-v2policy; - the
RebuildMismatchassertion; - the Node 22.18/24.2 wording;
- the dropped Fastly
CARGO_HOMEpassthrough; - the description, the doc leftovers, the four
#[ignore]reasons and the Phase 3 restore note.
Still open by your choice, which is fine: the shared-deadline seam in acquireOrdinaryTransfers, the RebuildIdentity context drops, and the three owner checks that need a root fixture.
CI Status
At e9e46e584: 28 of 30 checks pass.
- browser integration tests: FAIL
- trace runtime and browser acceptance: FAIL
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- HTML EOF runtime (Fastly): PASS
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- worker-deadlines (spin): PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- cargo test: PASS (required)
- ts dev lint domains (changed lines): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- Analyze (javascript-typescript): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- prepare integration artifacts: PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- docs-proposal scripts: PASS
- cargo test (cross-adapter parity): PASS
- release scripts: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- CI Node pin guard: PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Follow-up to the round-4 review: one more blocking item, inline on .github/workflows/release-build.yml:149. The release should publish and attest the Fastly package that fastly compute deploy uploads, as a downloadable .tar.gz built reproducibly, alongside the wasm.
Defer the unused controlled producer graph, fixtures and schemas to its own enabling phase. Preserve main's production dependency resolution and retain the live ordinary comparison and process safety helpers. Build deterministic Fastly packages from a fixed service-neutral manifest, bind both packages to their observations, compare every archive byte, and include the package in provenance, SPDX signing and exact release sets. Preserve wasm verification and document deployment of the unchanged package.
|
For the round-4 CI blocker: reran the failed Integration jobs at The new package contract was rehearsed at that exact head in fork |
The release-verification heading became "Deploy the verified Fastly package", so links to #deploy-the-verified-fastly-wasm from the getting started and EdgeZero guides no longer resolved.
Keep both Unreleased breaking-change entries in CHANGELOG.md: main's integration configuration validation (#1455) and Phase 2's tsjs build requirements.
Per the user, binary attestation and package publishing (#1449, #1463, #1543) are not part of this work and carry lower priority (DEC-19). AX-5 keeps only the production image built from source, A6 shrinks to building it and no longer waits on #1449, and the image and Terraform units restack on #1531, where they apply cleanly, instead of #1543.
Summary
Build Fastly twice on every push to
mainand everyv*tag, require equality of the complete wasm and deployable package, and publish the genuine signed rebuild check. A mismatch prevents signing and publication. This remains ordinary equality (release_qualification:false), not controlled-recipe qualification.trusted-server-fastly.tar.gzalongside the unchanged wasm name. A fixed service-neutral manifest and normalized tar/gzip metadata make packaging independent of host modes, ownership, paths and timestamps. Operators verify and deploy the downloaded archive unchanged.jsonschema; main's existing dependency versions and external dependency edges are preserved. The complete PR now adds about 25.6k lines rather than about 94k.Spec / Plan
Phase 2 spec and reviewed package amendment · Gate 1 record · implementation record · phase/dependency assignment.
Part of #1235. Phase 3 #1463 must restore the deferred foundations it consumes and reconcile package evidence/indexing. Packages PR #1543 must reconcile the additional release asset and subjects. This PR does not edit either dependent branch or open later qualification/index/consumer gates. Closed-path findings in #1517 belong to the deferred implementation.
Verification
Current head:
27361ec58, signed and GitHub Verified.package-sizesigning case. Restored source passes. Missing/changed/unsigned packages and package attestation failures stop publication before mutation..40package on macOS exactly matches its Linux-published bytes. The published wasm serves three publisher pages plus injected JS under pinned Viceroy with full source headers, actual request/response logs and no build-fact lines; listeners stopped and bytes remained unchanged. The downloaded macOS arm64 CLI starts with the exact source/target/version, and both macOS binaries retain the 13.0 metadata floor. This is local execution, not a Fastly deployment or macOS 13 runtime qualification.e9e46e584also passed on attempt 3, including both previously failed browser jobs.v0.0.0-rehearsal.40at this exact head: all 12 jobs pass, with 19 immutable prerelease assets, not Latest. Independent download checks passed: 18 checksums, all release memberships, 44 bundled attestation checks (online/offline with workflow and exact-tag certificate identities), 11 GitHub attestation API lookups, all four transfer/job bindings and source-owned recipe/lock hashes. Offline checks ran with network denied; a same-name one-byte-altered package was rejected after its unchanged control passed. Reproduction took 384s and comparison 69s; both finished before the remaining native builds, so neither delayed signing in this run.Historical
.39ate9e46e584passed all 12 jobs and published the earlier 18-file contract. It is not evidence for this new package contract. Earlier source-specific transcripts remain linked in the phase records and PR conversations.Human gates and limits
Merging Phase 2 activates the second Fastly build plus comparison on every
mainpush andv*tag; there is no separate equality switch. Build/comparison ceilings remain 90/10 minutes. Maintainers still own activation, immutable release/tag rules and publication settings. Native release builds remain single builds. Cloudflare/Spin are outside the published cohort. No IAB release, tag, setting change or deployment is performed by this PR response.The signing job still trusts the pinned hosted Node installation while holding its signing permissions; it never checks out or executes build inputs. No global host allowlist additions remain after deferring the schemas/policies; existing reviewed per-line API suppressions remain.
Checklist
Closes #1235