Conversation
392c994 to
087e1a7
Compare
f11ad3c to
0ddbb88
Compare
05896d4 to
ce63f6e
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
A large, unusually careful documentation refresh that also carries runtime Rust, CI workflow, shell script, config template, and governance changes. The documentation itself is high quality — I mechanically cross-checked every documented route and every TOML key in the changed docs against the source and found zero fabricated routes or config keys, which is a rare result for a 6,000-line docs change.
The blocking findings are not about the docs. They are three things riding along inside a "docs refresh": a runtime observability regression on the Spin adapter, a rewrite of the repository's sensitive-data policy that legalizes an existing violation, and a rewrite of the project's governance charter.
2 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 this branch. Both were scratch-verified in an isolated worktree at this head, individually and together. The remaining comments describe the fix in prose because the change is a scope/ownership decision or spans multiple sites and can't be auto-applied.
Note on the stated diff size
The PR description cites 107 commits / 140,494 insertions, which is measured against an older base. Against current rc/202608 (07dfc1c6dddf69345ded17bd2d40a3d01bb39bcf) the real diff is 109 commits, 203 files, +6,003 / −3,631. This review is against that diff, not the description.
Blocking
🔧 wrench
- Spin startup diagnostic drops the error-stack report — see inline at
crates/trusted-server-adapter-spin/src/app.rs:509(suggestion) - Sensitive-data policy rewritten to sanction a value the same PR keeps — see inline at
fastly.toml:10 ProjectGovernance.mdrewrites the chartered governance of an IAB Tech Lab project — see inline atProjectGovernance.md:13
❓ question
- Seven commands documented under "CI Gates" are run by no PR-triggered workflow — see inline at
.github/workflows/documentation-checks.yml:3
Non-blocking
🤔 thinking / ♻️ refactor / 📌 out of scope / ⛏ nitpick / 📝 note
- 🤔 Spin release build lost its boot-proving env overrides with no CI replacement — see inline at
.github/workflows/test.yml:201 - 🤔
documentation_snippets.rshardcodes dep versions and leans on ambient cargo cache — see inline atcrates/trusted-server-integration-tests/tests/documentation_snippets.rs:40 - 🤔
[trusted_client_ip]removed from the operator template with no decision-record entry — see inline attrusted-server.example.toml:145(LEFT side) - ♻️ Same
current_context()truncation on Cloudflare — see inline atcrates/trusted-server-adapter-cloudflare/src/app.rs:362(suggestion) - 📌
spinis not pinned in.tool-versions— see inline at.tool-versions:4 - ⛏
uncomment_blockheuristic still mis-handles prose — see inline atcrates/trusted-server-core/src/config.rs:540 - ⛏ The new
.env.dev[ec]entry is a no-op — see inline at.env.dev:8 - 📝 OpenRTB codegen binary is now completely silent — see inline at
crates/trusted-server-openrtb-codegen/src/main.rs:25(LEFT side)
Cross-cutting / body-level findings
🤔 Actions are pinned to mutable release tags, not commit SHAs
I verified every pin against the GitHub releases API. All thirteen exist and are the current latest release, so there are no broken or hallucinated pins:
| action | pinned | latest |
|---|---|---|
actions/checkout |
v7.0.1 | v7.0.1 |
actions/setup-node |
v7.0.0 | v7.0.0 |
actions/cache |
v6.1.0 | v6.1.0 |
actions/upload-artifact |
v7.0.1 | v7.0.1 |
actions/download-artifact |
v8.0.1 | v8.0.1 |
actions/configure-pages |
v6.0.0 | v6.0.0 |
actions/upload-pages-artifact |
v5.0.0 | v5.0.0 |
actions/deploy-pages |
v5.0.1 | v5.0.1 |
actions-rust-lang/rustfmt |
v1.1.2 | v1.1.2 |
github/codeql-action |
v4.37.9 | current |
browser-actions/setup-chrome |
v2.2.0 | v2.2.0 |
fastly/compute-actions |
v14 | v14 |
The upload-artifact v7 / download-artifact v8 major mismatch is genuine upstream, not an authoring error. actions-rust-lang/setup-rust-toolchain stays at v1.17.0 while v2.0.0 exists — a safe, deliberate non-bump.
The remaining concern is only that git tags are mutable and can be repointed by a compromised maintainer; SHA pinning is the hardening standard, and this PR bills itself partly as a hardening pass. docs/internal/audits/documentation-refresh-decisions.md:93 explicitly chose "exact release tags", so this is a recorded disagreement rather than a defect — flagging so the tradeoff is visible.
Verified clean on the rest of the supply-chain surface: zero ${{ github.event.* }} interpolations anywhere in .github/workflows/ or .github/actions/ (no script-injection sinks), and every workflow declares least-privilege permissions: (contents: read, with pages: write + id-token: write scoped to deploy-docs.yml alone).
📝 gam.md / kargo.md are published but unlinked — intentional, recording so it isn't "fixed" later
Both were dropped from the sidebar in docs/.vitepress/config.mts yet still build (.vitepress/dist/guide/integrations/{gam,kargo}.html) with zero inbound links from any published page. I read both: they are tombstones ("Trusted Server does not ship a direct Google Ad Manager integration…"), whose purpose is to catch stale external links, so absence from navigation is correct. No action requested — noting it so a future reader doesn't re-link them. (Anchored here rather than inline because the removal is a deletion spanning several config.mts hunks.)
👍 Praise
- Route-contract tests across all four adapters (
crates/trusted-server-adapter-fastly/src/app.rs:1592-1631, andtests/routes.rsin the cloudflare / spin / axum adapters) pin the exact(method, path)set, including the legacy/admin/keys/*aliases. This is precisely the regression net this area needed. crates/trusted-server-adapter-cloudflare/src/platform.rs:585-598corrects a doc claim that was wrong. The old text said config and KV "are sourced from the edgezero handles thatrun_appinjects". I verifiedgrep -n "fn stores"across all four adapters returns onlycrates/trusted-server-adapter-fastly/src/app.rs:1336, so the new text ("the Cloudflare application does not implementHooks::stores()") is the accurate one. Correcting a doc toward the less flattering truth is the right instinct.- Zero fabricated routes or config keys. I extracted all 27 backticked route paths from the changed guides and every
key =from every TOML fence in every added/modified doc, then grepped them against the full Rust source. The only hit wasthumbnail, which is a user-chosen profile-map key rather than a struct field. - The
secret_storetemplate removals are correct —crates/trusted-server-core/src/settings.rs:291-305, 913-915, 1922-1924deprecate-and-ignoresecret_store,server_side_key_secret_store, andcredential_secret_storewith a warning, so dropping them from the template is right. - The deploy-docs provenance assertion actually works. I ran
GITHUB_SHA=aaaa…aaaa npm run buildin the worktree and confirmedgrep -R --fixed-strings --quiet "$GITHUB_SHA" .vitepress/distsucceeds. docs/public/CNAMEdeletion is safe — it contained the literal placeholderyour-custom-domain.com, not a live domain.scripts/smoke-fastly.shcorrectly avoids the tracked-manifest secret hazard — it copiesfastly.tomlinto a per-run temp project (scripts/smoke-fastly.sh:33-34) beforets config push --localwrites secrets into it, so the tracked file is never written.- The
.envsynthetic → EC migration is correct —grep "counter_store\|opid_store\|SYNTHETIC" crates/trusted-server-core/src/settings*.rsreturns nothing, so the removed keys really are dead. cache-dependency-pathfixed frompackage.json→package-lock.jsonin bothformat.ymlandtest.yml;$GITHUB_OUTPUTquoted throughout; multi-linerun:blocks moved into repository scripts.
Local verification
Run from an isolated worktree detached at 9dfb956cda2674b2ad04796321a8f6edcd26337b.
| Gate | Result |
|---|---|
cargo fmt --all -- --check |
PASS |
cargo fmt --manifest-path crates/trusted-server-integration-tests/Cargo.toml -- --check |
PASS |
cargo clippy-spin-native |
PASS (no warnings) |
cargo clippy-cloudflare |
PASS |
cargo clippy-cloudflare-wasm |
PASS |
cargo test-spin |
PASS — 38 passed, 0 failed |
cargo test-cloudflare |
PASS — 24 + 23 passed, 0 failed |
cd docs && npm ci && npm run format |
PASS — "All matched files use Prettier code style!" |
cd docs && npm run lint |
PASS |
cd docs && npm run build |
PASS — build complete in 6.02s, no dead links |
Both suggestion blocks were applied and verified individually against a clean tree, then together as a batch (fmt + target-matched clippy + cargo test-spin + cargo test-cloudflare all green in every configuration). The worktree was restored to the PR head afterwards.
Not run locally (cost, and green on CI): cargo test-fastly, cargo test-axum, the parity suite, the full clippy alias chain, vitest.
Verified vs. unverified
Scratch-verified: the Spin and Cloudflare diagnostic findings (fix applied, fmt/clippy/tests green); the policy and governance findings (both sides of the diff plus the decision record read in full); the CI-gate-enforcement question (grep across all workflows plus live gh pr checks); the Spin release-build finding (workflow diff and smoke matrix contents); the action pins (all 13 checked against the GitHub releases API); the [trusted_client_ip] finding (settings.rs:2739-2810, all four adapter call sites, doc coverage at both base and head, and absence from every decision record); and the config.rs, .env.dev, .tool-versions, openrtb-codegen, and gam/kargo findings. All praise items verified as stated.
Unverified / judgement: the documentation_snippets.rs finding — I reasoned about --offline resolution and the shared CARGO_TARGET_DIR from reading the test; I did not construct a cold-cache run to prove it fails. The spin pinning finding relies on this PR's own evidence document for the Spin 4.1.0 claim; I could not run the Spin CLI here.
Concerns I raised and then killed by checking — listing these so they don't get re-raised in a later pass: bogus or nonexistent action version pins (all real and current); the docs/public/CNAME deletion breaking a custom domain (it was a placeholder); scripts/install-wrangler.sh's strict test "$(wrangler --version)" = "$version" (the adapter first success (cloudflare) check is green, so it works); the [ec] and secret_store template removals being wrong (both correct against the code); and a "docs-parity standalone crate with its own lockfile" (git ls-files | grep -i docs.parity returns nothing — it was withdrawn by commit 9dfb956c, "Remove intrusive documentation tooling").
Recommendation
Three of the four blockers are scope problems rather than code problems. The cleanest path forward:
- Split
ProjectGovernance.mdinto its own PR for Task Force / IAB Tech Lab sign-off. Deleting chartered commitments (biweekly cadence, published minutes, continuous release) is not a change a code review can approve. - Split the
CLAUDE.mdsensitive-data policy rewrite into its own PR so the new typed-exception system gets explicit maintainer sign-off from someone other than the exception's own owner — and either remove theservice_idor have the exception re-approved independently. - Apply the one-line Spin diagnostic suggestion (and optionally the matching Cloudflare one).
- Answer the "CI Gates" labelling question.
What remains after that split is a strong, accurate documentation refresh I would be glad to see merged.
CI Status
All 22 reported checks pass. Branch protection reports no required checks on spec-docs-refresh.
.github/dependabot.yml: PASSAnalyze (actions): PASSAnalyze (javascript-typescript): PASSAnalyze (rust): PASSCodeQL: PASSadapter first success (axum): PASSadapter first success (cloudflare): PASSadapter first success (fastly): PASSbrowser integration tests: PASScargo check (cloudflare native + wasm32-unknown-unknown): PASScargo check/build/test (spin native + wasm32-wasip1): PASScargo fmt: PASScargo test: PASScargo test (axum native): PASScargo test (cross-adapter parity): PASScargo test (ts CLI, native): PASSformat-docs: PASSformat-typescript: PASSintegration tests: PASSintegration tests (Fastly EC lifecycle): PASSprepare integration artifacts: PASSvitest: PASSDocumentation checks: not run — the workflow this PR adds isworkflow_dispatch-only, which is the subject of the ❓ finding above.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed at e7687a1b075847bf50c7bdc64bfc99b737a53c97 against 07dfc1c6dddf69345ded17bd2d40a3d01bb39bcf. I have not repeated findings already covered by existing review threads.
One follow-up worth tracking: trusted_client_ip.shared_secret is the secret value itself and is serialized into the app-config blob. Redacted only masks debug and display output, and this field is not registered for secret-store resolution. Changing that storage contract is outside this documentation refresh, but it should get a separate issue with a compatibility and migration plan.
|
Note Attribution, added 2026-09-20 in response to this comment: the review below was generated by an automated Claude Code review agent and posted from my account. Its "Verdict" is the tool's output, not an independent maintainer sign-off, and its "Correction to something I said earlier" refers to the tool's own prior automated pass, not to anything a person said on this PR. Review feedbackReviewed at Blocking: onboarding is the only content removed without a destinationEverything else that shrank in this PR was deduplicated into something better. Onboarding is different. It goes 160→42 lines, moves to
Net effect: after merge a new engineer has no discoverable onboarding entry point. The containment rationale is right — contacts and meeting times should not be public. The execution over-applied it. The material that did not survive is the part that is not sensitive and has no other home: what the system does, how a request flows through it, and what the ad-tech vocabulary means. Nothing in the refreshed reference set absorbed it; I have put up #1165 as a draft stacked on this branch showing one way to close it: a published ScopeThe title says documentation refresh; the diff is 202 files, of which 144 are non-doc (+2641/-1146) including adapter source, workflows, I checked whether that hides behaviour changes and it largely does not — the TypeScript edits in That is solid work. It is also why this needed four review rounds: reviewers had to audit a runtime surface to approve a docs change, and two real bugs (the Spin global logger suppressing production logging, the Not asking you to re-cut it now — flagging it for the next one of this size. Smaller points
Correction to something I said earlierI previously flagged VerdictMerge it once the onboarding placement is settled. That is a one-line |
|
Latest feedback is addressed in 5c043a3.
Verification on the final tree: full scripts/check-documentation.sh passed; all 949 Vitest tests passed; Prettier passed for the root README and JS/docs trees. |
|
The PR description no longer matches the branch. It still describes the docs-parity tooling that 9dfb956 removed: the "Settings parity (WP3)" block, the The description becomes the merge record, so please rewrite it against the current head. A short summary of what changed plus the test plan is enough. The generated ledger sections (SHA-256 manifests, "immutable inputs", artifact digests, the Spin receipt with an expiry date) can go; the decision record under |
|
On the "Review feedback" comment from 2026-09-14 (#1049 (comment)): it is posted from the PR author's account but written as a third-party review, down to "Verdict: Merge it". It also has a "Correction to something I said earlier" about Two related items:
|
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Full-surface documentation refresh plus deterministic parity enforcement: 202 files, ~6.1k insertions / ~3.6k deletions against rc/202608. Reviewed the whole diff — the Rust and TypeScript changes are overwhelmingly doc comments, new #[cfg(test)] route-contract tests, and test-fixture hardening; the only runtime deltas are additive startup diagnostics on Cloudflare and Spin. Public APIs, routes, status codes, and successful-request behaviour are unchanged. All 21 GitHub checks pass on the reviewed head.
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.
Non-blocking
♻️ refactor
- Canonical CI gate list omits the two template-cache harness steps — see inline at
CLAUDE.md:382
Cross-cutting / body-level findings
- 📝 Spin startup diagnostic now writes directly to stderr —
crates/trusted-server-adapter-spin/src/logging.rsreplaceslog::error!with a directwriteln!(io::stderr(), ...). This is a deliberate, well-documented deviation from the repo's "uselogmacros, notprintln!/eprintln!" convention: the EdgeZero Spin logger initializer is a no-op, so the boot failure would otherwise be invisible, and writing directly avoids claiming the process-global logger. Flagging it only so the exception is visible to future reviewers — no action requested. - 📝 Cloudflare startup errors now log the full
Reportdebug —crates/trusted-server-adapter-cloudflare/src/app.rsaddsworker::console_error!with{error:?}under#[cfg(target_arch = "wasm32")]. The response body still exposes onlyuser_message(), and this matches the pre-existing Spin behaviour, so it is not a new disclosure class.workeris an unconditionalcfg(target_arch = "wasm32")dependency, so thecfggate compiles on both the native and--features cloudflarewasm legs. - 👍 Smoke scripts never mutate the tracked working tree —
scripts/smoke-fastly.shcopiesfastly.toml/edgezero.tomlinto an isolatedmktemp -dproject and symlinkscrates/, sots config push --localand the seededts_secretsblocks land only in the throwaway workspace.smoke_remove_workspaceadditionally refuses torm -rfany path outside the generated prefix. That is a meaningfully safer shape than "mutate and restore on trap", and the three sibling scripts follow the same pattern. Secret values are obviously synthetic (smoke-admin-password-32-bytes-ok, etc.). - 👍 Route-registration contract tests across all four adapters —
complete_route_registration_set_matches_the_prechange_contractin the Fastly/Axum/Cloudflare/Spin route suites pins the exact(method, path)set, and the matchingstartup_error_route_set_...tests pin the fallback router. Combined with the existing parity suite this makes a silent route/method regression during a docs-driven refactor very hard to land. - 👍
docs/public/CNAMEremoval anddeploy-docs.ymlprovenance assertion — dropping theyour-custom-domain.complaceholder is correct (it would have hijacked the Pages custom domain), and the newgrep -R --fixed-strings "$GITHUB_SHA" .vitepress/diststep is backed by a real emitter:docs/.vitepress/config.mtsinjectsprovenanceBanner()into every page's parsed markdown and validates the SHA shape, so the assertion cannot silently pass on an unstamped build. - 📝 Docs build is enforced on PRs — worth recording since the new
Documentation checksworkflow isworkflow_dispatch-only: the dead-link-failing VitePress build still runs on every PR viaformat.yml'sformat-docsjob ("Build with VitePress (fails on dead links)").TESTING.mdand the new## Manual documentation gatessection both state the manual-only scope explicitly, so the split is intentional and documented.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cloudflare) / cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- adapter first success (axum): PASS
- adapter first success (fastly): PASS
- adapter first success (cloudflare): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CodeQL: PASS
- Documentation checks: not run (workflow_dispatch only, by design)
No required checks are configured on this branch.
jevansnyc
left a comment
There was a problem hiding this comment.
checks out sans 2 minor comments which are fine to move forward with outside of housekeeping
|
Housekeeping from the two 2026-09-17 comments is done:
|
…aces Transplant the reader-facing content of the rc/202608 documentation refresh (PR #1049) onto main so the docs ship ahead of the release merge: - Rebuilt guides (adapters, configuration, testing, creative processing, integrations inventory, telemetry, tsjs, CLI, API reference), the restored public onboarding page, ten crate READMEs, and the internal decision and evidence records - The four adapter smoke scripts plus scripts/README.md, and the documentation-snippet compile test - docs/public/CNAME placeholder removed; VitePress config updated Re-pointed at main where the release branch differs: the rc-only `ts audit ad-templates` and `ts config ad-templates` command families are removed from the CLI guide (every remaining documented command probed against a main-built binary); CI-gate links target AGENTS.md; the documentation-checks aggregate script is left out because main's rustdoc does not yet pass with warnings denied, and testing.md lists the two commands that do run here; scripts/README.md lists only scripts present on main. Merge conflicts with main's newer CHANGELOG, README, TESTING, and auction README resolved to main's side. Verified: VitePress lint/format/build (dead links fail the build), documentation_snippets compiles against main's core, smoke-axum.sh boots main's adapter, and a claims scan checked every referenced script, repo path, and route against this tree.
1c6b23a to
853eb45
Compare
|
Retargeted per maintainer decision: base is now |
- Remove the documentation-refresh decision and evidence records: they declare rc/202608 as the delivery target and describe code decisions (Spin logger, workflow scoping, policy split) that this docs-only diff does not carry; both stay accurate on spec-docs-refresh-rc - Keep the full ts audit and ad-templates command index: PR #823 landed on main and the merge from main restored those subcommands, so every documented chain is re-verified against a freshly built binary - Add the missing js_asset_proxy integration to the configuration inventory (with a field-level section), the integrations overview, and both API-reference tables; the deploy-validated set has 15 entries - Correct CONTRIBUTING.md: TESTING.md is an auction-orchestration runbook that repeats the adapter aliases, not a link index, and the gate link points at AGENTS.md - Give each smoke script a distinct default origin port outside every claimed adapter port range, and make smoke_stop_process capture child PIDs via pgrep, wait for them to exit after signaling, and escalate to KILL on timeout; pgrep is now a declared requirement in all four
Summary
Important
Retargeted on 2026-09-21: the base changed from
rc/202608tomainand the head was force-pushed from the rc-scoped refresh (1c6b23a54, preserved at branchspec-docs-refresh-rc) to a docs-only transplant verified againstmain. The three earlier approvals were given for the rc-scoped diff and predate this change — please re-review.Brings the documentation content of the refresh to
mainnow, rather than waiting for therc/202608release merge. Scope is documentation surfaces only: the docs site, top-level and crate READMEs, the four adapter smoke scripts the guides teach, and the documentation-snippet compile test. The refresh's Rust doc-comments, route-contract tests, startup diagnostics, and CI workflow changes are not included; they land with the release merge fromspec-docs-refresh-rc.What differs from the rc-scoped version reviewers approved
Everything was re-verified against
main, and the guides were re-pointed where the branches diverge:ts audit ad-templatesandts config ad-templatescommand families are removed from the CLI guide. Every remaining documented command was probed against atsbinary built from this branch.check-documentation.shis not included: main's rustdoc does not yet pass with warnings denied (29 intra-doc-link errors that the rc-scoped refresh fixes).testing.mddocuments the two documentation checks that do run on main.scripts/README.mdlists only scripts that exist here; CI-gate links point atAGENTS.md(CLAUDE.mdis a symlink on main).CHANGELOG.md,README.md,TESTING.md, and auction README resolved to main's side;configuration.mdkeeps both main's initial-deployment steps and the refresh's secret-field migration guidance.Verification
cd docs && npm run lint && npm run format && npm run build— clean; the build fails on dead links and passed.cargo test --test documentation_snippets— the documented integration fixture compiles against main's core../scripts/smoke-axum.sh— boots main's Axum adapter end to end.docs/public/CNAMEintentionally does not exist, and the gitignoreddist/directory).Relation to the release branch
The full rc-scoped refresh (with the code changes and the review history above) is preserved at
spec-docs-refresh-rcand merges intorc/202608with the release. When rc later merges to main, its versions supersede these files and restore the rc-only material. Merging this PR publishes the docs site from main viadeploy-docs.yml.Follow-ups
spinand add Spin to the adapter-first-success smoke matrix.trusted_client_ip.shared_secretto secret-store resolution.