Conversation
392c994 to
087e1a7
Compare
d514dc6 to
f27cc1a
Compare
f11ad3c to
0ddbb88
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Requesting changes for the six blocking findings below. Also included are ten non-blocking observations.
Blocking (6)
🔧 W1 — Pinned Node is silently discarded, and docs-parity enforces the broken command
.github/workflows/docs-links.yml:27 + tools/docs-parity/src/workflow.rs:451
run: is a YAML plain scalar, so " reaches the shell literally. Proven from this PR's own CI log:
awk: cmd. line:1: ^ backslash not last character on line
node: v22.23.2
node-version is empty → setup-node falls back to the runner default. The job ran Node 22.23.2, not the pinned 24.12.0. The three " uses in test.yml are inside YAML double-quoted scalars and are correct; this is the only broken one.
Not suggestion-eligible: workflow.rs:451 requires this exact byte string, so fixing only the workflow turns check --all red. Both files must change together.
🔧 W2 — CLI-help gate hard-pins PR #1049; the first post-merge CLI change red-locks CI
tools/docs-parity/src/cli_help.rs:2103
|| run.pull_request != 1049
check_repository (wired into check --all at lib.rs:395) fails with "CLI source/blob set changed after capture" as soon as any crates/trusted-server-cli blob differs. The only golden-refresh path is import-hosted → validate_run, which rejects every run whose pull_request != 1049. cli-overrides.toml is per-command platform annotations, not an escape hatch. The runbook even says "only after PR #1049 has reached main" and directs receipts to comments on this PR.
🔧 W3 — A documented required gate runs in no workflow (and CI runs a gate the manifest omits)
tools/docs-parity/manifests/gates.toml:51 → generated into CLAUDE.md:396, TESTING.md:52, AGENTS.md, docs/guide/testing.md:
cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test documentation_snippets
Zero hits across .github/. That is the gate that compiles the documentation-snippet:runtime-services fixture in integration-guide.md — so the "compile-verified" fixture is verified by nothing. Conversely test.yml:303 runs clippy on the integration-tests crate, absent from the manifest and reachable by no alias. Root cause: gates.rs:298 compares the manifest only against four markdown files, never against .github/workflows/*.
🔧 W4 — GTM container_id contract documented wrong (newly added line)
docs/guide/configuration.md:1612 says "GTM- followed by alphanumeric characters; 5–50 characters". google_tag_manager.rs:54 is ^GTM-[A-Z0-9]{4,20}$ → 8–24 chars, uppercase only (length(min = 1, max = 50) is non-binding). A 5-char or lowercase value is rejected at startup.
The suggested replacement is attached inline.
🔧 W5 — Fabricated TSJS script tag survives; the PR removed 2 of 3 instances
docs/guide/creative-processing.md:792-798 documents src="/static/tsjs-core.min.js", async, data-tsjs-integration="core". Real emitter tsjs.rs:12,48-51 produces /static/tsjs=tsjs-unified.min.js?v={hash} with id="trustedserver-js" and no async. data-tsjs-integration appears nowhere in the codebase. Count went 3 → 1 in this PR; line 823 of the same file states the correct URL.
🔧 W6 — Sensitive-data scanner skips every non-PSL host
tools/docs-parity/src/scanner.rs:368, predicate at :563. A URL authority that isn't a registrable public-suffix domain is continued, so it never becomes a finding and never needs an allowlist entry. Verified against the live tree:
value source sha256 allowlist entries
169.254.169.254 sourcepoint.rs:1548 34146ce1… 0
192.168.1.1 settings.rs:6046 c5eb5a4c… 0
A committed https://jenkins-prod/job/deploy or https://10.0.3.14/keys is invisible to the gate. The .internal special-case one line above shows non-PSL hosts were meant to be covered. Both live hits are fictional test values, so this is a coverage hole, not a leak — reasonable to defer if you'd rather land the PR.
Non-blocking (10)
♻️ N1 — Documentation parity is the name of two different jobs (format.yml:136, docs-links.yml:13), both on pull_request. Branch protection matches required checks by name. The docs-links job's steps are a strict subset of the format.yml job's.
♻️ N2 — Five hardcoded toolchain: "1.95.0" in docs-links.yml (lines 30, 57, 89, 123, 153) while this same PR adds scripts/read-tool-versions.sh, which format.yml uses.
♻️ N3 — srcExclude still lists guide/onboarding.md, the file this PR deletes. Every other entry resolves. The suggested removal is attached inline.
♻️ N4 — docs/README.md:44,120 still instructs operators to edit docs/public/CNAME, which this PR deletes. Contradicts the PR's own decision record ("never restore the placeholder CNAME").
♻️ N5 — docs-parity check with no flags exits 0 having validated nothing (lib.rs:607; --all isn't required = true unlike five sibling subcommands), and Outcome::Drift exits nonzero with no output (main.rs:6-8) — a check --all failure gives an empty CI log.
🤔 N6 — Two commands in docs/guide/testing.md don't work (pre-existing, page edited here): cargo test-axum test_generate_ec_id runs zero tests (alias is -p trusted-server-adapter-axum; the test is in trusted-server-core), and cargo clippy-axum --fix --allow-dirty fails — I reproduced it with an equivalent alias: error: Unrecognized option: 'fix' (args land after the alias's --).
🤔 N7 — Spin is the one adapter excluded from the adapter first success matrix, and scripts/smoke-spin.sh is referenced by no workflow, yet test.yml drops the Spin boot env overrides saying boot "is covered by a separately provisioned smoke test."
🤔 N8 — crates/trusted-server-adapter-spin/src/logging.rs claims the process-global logger slot with one that emits only STARTUP_DIAGNOSTIC_TARGET. No regression today, but it permanently blocks any later Spin logger.
📌 N9 — docs/guide/integrations/lockr.md:147 documents POST /integrations/lockr/sync; lockr.rs:353 registers only /sdk, /api/*. File untouched by this PR; the PR's own generated api-reference.md lists the three real routes.
👍 N10 — CHANGELOG v1.2.0 correction is right (I confirmed only v1.1.0 exists on origin); fastly.toml drops a real author email and the retained service_id carries a properly typed/owned/expiring exception per CLAUDE.md.
- Fix the docs-links Node pin: the run step is a YAML plain scalar, so the escaped quotes reached awk and emptied the setup-node version; update the enforced byte string in workflow.rs to match - Rename the docs-links pull-request job so "Documentation parity" names exactly one required check - Unpin authenticated CLI capture imports from PR #1049 so any successful pull-request capture run can refresh the goldens after merge - Run the documentation_snippets gate in test.yml, add the integration-tests fmt and clippy gates to the manifest, and pin both couplings plus the toolchain versions with repository contract fragments - Correct the GTM container_id contract, the injected TSJS script tag, the Lockr routed endpoints, the stale onboarding srcExclude entry, and the CNAME setup instructions that contradicted the decision record - Require a mode flag on docs-parity check and name each drifting offline check on stderr instead of exiting silently - Refresh manifest selectors and fingerprints shifted by these edits Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All six blocking findings and eight of the ten observations are addressed in 51ad97a. Per-finding disposition: W1 Fixed. The run step is now a plain scalar with unescaped quotes; verified it emits W2 Fixed. W3 Fixed both directions. W4 Applied your suggestion verbatim. W5 Replaced with the real emitter output: W6 Deferred, per your offer, with data from an actual attempt: I implemented the predicate (flag DNS-shaped non-PSL authorities, exempt non-routable and documentation-range IPv4) and bootstrapped the tree. It produced 69 new findings, and apart from N1 The docs-links job is renamed "Documentation automation guard", so "Documentation parity" now names exactly one pull-request check (the format.yml superset job). N2 The five hardcoded pins stay, but N3 Applied your suggestion. N4 Rewrote the custom-domain instructions to configure the domain in GitHub Pages settings, stating the repository intentionally tracks no CNAME file; the troubleshooting checklist and project-structure tree no longer mention it. N5 N6 Both commands fixed: the axum example uses a test that exists in the adapter ( N7 Reworded the comment to state plainly that boot is exercised by the manually run N8 No change. Agreed the logger claim blocks any later Spin logger, but changing adapter runtime behavior is out of scope for this PR; I can file an issue. N9 Corrected to the real routes: GET The manifest selector and fingerprint churn in the diff is mechanical fallout from the byte shifts, reconciled against a fresh bootstrap. Full |
- Fix the docs-links Node pin: the run step is a YAML plain scalar, so the escaped quotes reached awk and emptied the setup-node version; update the enforced byte string in workflow.rs to match - Rename the docs-links pull-request job so "Documentation parity" names exactly one required check - Unpin authenticated CLI capture imports from PR #1049 so any successful pull-request capture run can refresh the goldens after merge - Run the documentation_snippets gate in test.yml, add the integration-tests fmt and clippy gates to the manifest, and pin both couplings plus the toolchain versions with repository contract fragments - Correct the GTM container_id contract, the injected TSJS script tag, the Lockr routed endpoints, the stale onboarding srcExclude entry, and the CNAME setup instructions that contradicted the decision record - Require a mode flag on docs-parity check and name each drifting offline check on stderr instead of exiting silently - Refresh manifest selectors and fingerprints shifted by these edits
0552d03 to
51ad97a
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The latest changes address the prior W1–W5 blockers; the scanner follow-up remains explicitly deferred. One new issue prevents the dependency-submission workflow from producing an API-valid payload.
Blocking
- 🔧 [P1] Include the API-required scan timestamp in dependency snapshots — see inline at
tools/docs-parity/src/dependency_snapshot.rs:85.
Validation and scope
The standalone docs-parity build and five existing snapshot tests passed. A scratch assertion on the generated payload reproduced the missing required field. No live dependency submission was attempted. This exceptionally large diff was reviewed with priority on prior fixes and substantive runtime/automation code; this was not an exhaustive audit of every extractor, generated record, and test.
CI Status
- adapter first success (axum): PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- adapter first success (cloudflare): PASS
- adapter first success (fastly): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo fmt: PASS
- cargo test (ts CLI, native): PASS
- Capture CLI help (macos): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (rust): PASS
- Documentation automation guard: PASS
- format-typescript: PASS
- Rust API documentation: PASS
- vitest: PASS
- cargo test: PASS
- Analyze (javascript-typescript): PASS
- Documentation parity: PASS
- Analyze (actions): PASS
- Capture CLI help (linux): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- cargo test (axum native): PASS
- format-docs: PASS
- Reconcile external-link issue: SKIPPED
- Check external documentation links: SKIPPED
- Submit dependency snapshot: SKIPPED
- Generate dependency snapshot: SKIPPED
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Request changes. I reviewed the non-tools/** diff with five parallel read-only reviewers against origin/rc/202608. The targeted core, CLI, integration, adapter, browser-fixture, and documentation-snippet checks passed, but the issues below remain.
Scope blocker
The entire tools/** subtree should not merge to main. It contributes roughly 125k lines of manifests, inventories, goldens, and tooling to this PR. The non-tool changes are currently coupled to it through include_str! calls in core tests, generated gate blocks, CI commands, and documentation instructions. Remove tools/** and those dependent changes before merging, or split the documentation-tool proposal into a separate, explicitly approved PR.
The generated gate matrix is also duplicated across multiple agent and testing documents, which increases drift and obscures the actual CI contract.
Validation
git diff --check passed. Targeted core, CLI, browser-fixture, integration-snippet, Spin, and Cloudflare checks passed. Real Spin/Cloudflare runtime log sinks were not exercised locally.
|
Review remediation is pushed in 9dfb956. The substantive documentation refresh and the separate internal onboarding path remain. I removed the standalone docs-parity Cargo workspace, generated inventories and goldens, scheduled link and dependency writers, and all core-crate coupling to that proposal. The retained automation is one source-preserving shell aggregate plus a read-only workflow_dispatch-only Documentation checks workflow, so it cannot block normal core or adapter CI. The Spin diagnostic no longer owns the global logger, and the Fastly smoke mutates only per-run manifest copies. Action references use exact release tags, and workflow logic longer than one command remains in repository scripts. Local verification covered the manual documentation aggregate, the target-matched Rust and JavaScript gates, all four real adapter smokes, workflow linting, shell linting, formatting, and an independent final review with no remaining findings. Hosted checks are now running against the exact pushed head. |
prk-Jr
left a comment
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.
…tics - Revert ProjectGovernance.md to the release-branch charter; chartered commitments are a Task Force decision and will be proposed separately - Revert the CLAUDE.md sensitive-data policy rewrite and the fastly.toml service-ID exception comment; the typed-exception system needs sign-off independent of the exception owner and moves to its own pull request - Format Spin and Cloudflare startup diagnostics with the full error-stack report instead of only the leaf context - Move the rustdoc commands out of "CI Gates" into a "Manual documentation gates" section that names the workflow_dispatch-only enforcement, and align the documented CLI clippy command with the one CI runs - Read the error-stack requirement from the workspace manifest in the documentation-snippet fixture instead of hardcoding 0.6 - Restore the [trusted_client_ip] template block, record the Spin boot-coverage gap in the decision record, drop the no-op [ec] overlay from .env.dev, and restore the codegen progress output with a scoped allow for the host-only tool
ChristianPavilonis
left a comment
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.
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; independent review found no Critical or Important issues. |
3 participants
Summary
mainat2e85a1cdc(VitePress site, root/crate markdown, in-code docs, config templates), every finding cited atfile:line, with ground-truth inventories (routes with per-adapter availability, all 15Settingssections, 14-integration capability matrix,tsCLI tree) as appendices.Changes
docs/superpowers/specs/2026-08-19-documentation-refresh-design.mdsrcExclude, CNAME,fastly.tomlsensitive values, empty/guide/page, gate alignmentRequestWrapper,.with_asset, GAM/Kargo pages, auction README rot,FAQ_POC.md, CHANGELOG/.env.examplerepairsSettingssections and 14 integration configsadserver_mock, script guards; nav repair;TESTING.mdrewriteplatform/docs, crate headers, tsjs JSDoccargo doc -D warnings, doctests in CI, dependabot gapsCloses
Closes #1038
Closes #277
Closes #341
Test plan
cd docs && npm run format(passes on the spec)cd docs && npm run lint && npm run build(after WP commits land)cargo fmt --all -- --checkand target-matched clippy/tests for crates touched by WP7cargo doc --no-depswarning-free for core and adapters (WP7/WP8)mainat2e85a1cdcby four parallel read-only audits, then realigned torc/202608(new[cache]section, admin EC diagnostics routes, restructured CLI)Checklist
Settings parity (WP3)
Generated from the checked settings record at
18f4d6b2eb2abdaef43b0b5f4b8c15d52be3eba0.Settings roots (17/17)
[auction][cache][consent][creative_opportunities][debug][ec][[handlers]][image_optimizer][integrations.*][proxy][publisher][request_signing][response_headers][rewrite][tester_cookie][tinybird][trusted_client_ip]Deploy-validated integration IDs (14/14)
adserver_mockapsdatadomedidomigoogle_tag_managergptgpt_diagnosticslockrnextjsosanopermutiveprebidsourcepointtestlightProvider profile schemas (3/3; 14 fields)
apsprebid-serverstandardDirectional field dispositions (18/18)
The checked axes are lifecycle, key identity, serialization, runtime use, and secret handling.
AssetOriginAuth.s3_sig_v4AssetOriginAuth.s3_sigv4DataDomeConfig.server_side_key_secret_nameDataDomeConfig.server_side_key_secret_storeDataDomeProtectionTestBypassConfig.credential_secret_nameDataDomeProtectionTestBypassConfig.credential_secret_storeEc.passphraseEcPartner.api_tokenEcPartner.ts_pull_tokenHandler.passwordPublisher.proxy_secretS3SigV4AuthConfig.access_key_idS3SigV4AuthConfig.secret_access_keyS3SigV4AuthConfig.secret_storeS3SigV4AuthConfig.session_tokenTinybirdSettings.access_token_secretTinybirdSettings.auction_token_secretTinybirdSettings.secret_storeTrustedClientIpConfig.shared_secretSecret classifications
trusted_client_ip.shared_secretis deliberately inline and may appear in diff/dry-run/confirmation output.tinybird.access_token_secretis accepted, discarded, and omitted from serialized config.secret_storeselectors are accepted and normalized away; none is recommended by the example template.Exact WP3 checks
settings --check,generate --check,snippets --check,classify --check,scan --check,links --local --check, andcheck --allcargo test-fastly config: 12 Fastly adapter tests and 199 core tests selected; no failuresorigin/rc/202608remains07dfc1c6dddf69345ded17bd2d40a3d01bb39bcfAdapter first-success smokes (WP5 deployment)
Hosted receipts use implementation SHA
2f1f11c2cf94cf0b5606e107672d6386d283959a. The Spin receipt was repeated at552dbeebe63aa6c742f2def95565bfcc80076513; the smoke script is byte-identical at both commits.Axum
Sequence: resolve/build
tsand the Axum binary; start an isolated loopback sentinel origin; initialize and strictly validate the app config; runts config push --adapter axum --local; read the generated envelope; launch one isolated Axum process for missing config, each missing secret, and the positive case.Cleanup: the
EXIT INT TERMtrap stops the active Axum process and sentinel origin, then removes the generated temporary workspace and.edgezerostate.Oracle: HTTP 200; body contains
SMOKE_ORIGIN_SENTINEL; rewritten URL targets the Axum listener; original origin URL is absent. Independent failures: missingTRUSTED_SERVER_CONFIG; missinghandlers[0].password; missingpublisher.proxy_secret; missingec.passphrase. Each failure requires HTTP 500, its exact startup diagnostic, and live adapter/origin processes.Receipt: Integration Tests / adapter smoke (Axum).
Fastly
Sequence: resolve/build
tsand release Wasm; start an isolated loopback sentinel origin; initialize and strictly validate the app config; prove/healthis 200 while an unconfigured publisher request fails; runts config push --adapter fastly --local; seed the threets_secretsentries; remove each secret independently; restore all entries and run the positive publisher case throughfastly compute serve.Cleanup: the
EXIT INT TERMtrap stops the active Fastly process and sentinel origin, restoresfastly.tomlbyte-for-byte, restores or removes.fastly.toml.edgezero-lockaccording to its initial state, and removes the temporary workspace.Oracle: HTTP 200; body contains
SMOKE_ORIGIN_SENTINEL; rewritten URL targets the Fastly listener; original origin URL is absent. Independent failures: missing config-store key while health remains 200; missinghandler_password; missingpublisher_proxy_secret; missingec_passphrase. Each publisher failure requires HTTP 500, its exact startup diagnostic, and live Fastly/origin processes.Receipt: Integration Tests / adapter smoke (Fastly).
Cloudflare
Sequence: require the exact Wrangler pin; resolve/build
tsand the Worker bundle; start an isolated loopback sentinel origin; initialize and strictly validate the app config; map the logical store toTRUSTED_SERVER_KV; runts config push --adapter cloudflare --local; readtrusted_server_configback with the explicit binding and local flags; encode the envelope asTRUSTED_SERVER_CONFIG.app_config; generate one isolated Wrangler manifest per missing binding and the positive case; run each withwrangler dev.Cleanup: the
EXIT INT TERMtrap stops the active Wrangler process and sentinel origin and removes the temporary workspace, including generated manifests and local Wrangler KV state.Oracle: HTTP 200; body contains
SMOKE_ORIGIN_SENTINEL; rewritten URL targets the Wrangler listener; original origin URL is absent. Independent failures: missingenv.TRUSTED_SERVER_CONFIG; missingenv.handler_password; missingenv.publisher_proxy_secret; missingenv.ec_passphrase. Each failure requires HTTP 500, a binding inventory that omits only the selected binding while retaining its control binding, the normalized exact diagnostic, and live Wrangler/origin processes.Receipt: Integration Tests / adapter smoke (Cloudflare).
Spin
Sequence: resolve/build
tsand release Wasm; start an isolated loopback sentinel origin; initialize and strictly validate the app config; prove a pre-push publisher request fails; map the logical store todefault; runts config push --adapter spin --local; launch onespin upprocess with each encoded secret variable omitted independently; launch the positive case with all three variables.Cleanup: the
EXIT INT TERMtrap stops the active Spin process and sentinel origin and removes the temporary workspace, including the generated manifest, component logs, and.spin/sqlite_key_value.db.Oracle: HTTP 200; body contains
SMOKE_ORIGIN_SENTINEL; rewritten URL targets the Spin listener; original origin URL is absent. Independent failures: missingdefault/trusted_server_config; missing encoded handler-password variable; missing encoded publisher-proxy-secret variable; missing encoded EC-passphrase variable. Each failure requires HTTP 503, the controlled config-push/one-variable delta, the normalized exact diagnostic, and live Spin/origin processes; a generic degraded-router 503 is insufficient.Time-bounded receipt: local run passed at
552dbeebe63aa6c742f2def95565bfcc80076513with Spin4.1.0(c0b3726, 2026-08-25), Rust1.95.0onaarch64-apple-darwin, and ownerdocumentation-maintainers; expires2026-10-07T00:00:00Z.Final documentation-refresh acceptance
f682c05df07f89bdbda61c2372d3f3f34e5c8de207dfc1c6dddf69345ded17bd2d40a3d01bb39bcf(rc/202608)bash -nfor the seven workflow scripts; and standalone docs-parity formatting, warning-denied Clippy, full tests, andcheck --all. The final docs-parity run passed 38 library, 35 classification, 27 CLI, 13 CLI-help, five dependency, six gate, 33 integration, two JSDoc, 62 link, 11 Markdown, two README, 70 route, 74 scanner, 24 settings, five snippet, and seven workflow tests. The reviewed sensitive manifest is SHA-256007c971f2d2b5bf1db923cf3ea45501917069a9a2a2b3534ff10e507abcc9661and governs 5,476 exact occurrences without changing the exception count.docs/internal/onboarding.mdunder the approved containment package; the public documentation was rewritten and expanded across configuration, routes, integrations, all four adapters, testing, telemetry, TSJS, architecture, and deployment. This change is a documentation refresh plus deterministic parity enforcement, not tooling alone.Cargo.lockSHA-2569bb34225c5b8d1da39c75c3a8143d905f4b7d228a8986dc93d7e58a4196b4bba; docs-parity lock SHA-256234a21b4831ec92fca081bc389dad6bdff1bc18d3a715f41831e2067a95e2ffb.0dcf054063dd6f746b1bb44ca58418d23efae887; Linux artifact ID10090397245, digestsha256:ff23327b542aca834c118a53313f50722164a1f92a52a563543979907310e7f4; macOS artifact ID10090451966, digestsha256:1844e6f1536b375fec8a7a92a18a6403831f289f5710d40684ae8fee4619563a. Two authenticated imports retained Linux help SHA-256d1cae561a509fac817befdca1ba733a2582dba87d322cf4c2efccce9729a6786, macOS help SHA-256cecd3a524ff6c50459b446dfe95980be0cdb037e1934ea0ab2a389aa6472f450, and capture-manifest SHA-256fc51a476a716e7b18ccd4a0dd9b912698448d78a26ecc65b7d1c139a4b3ddf2c. The exact-final-head capture jobs also passed without changing the reviewed goldens.main; the first real scheduled external-link run; dependency submission plus graph visibility; and optionalmainbranch-protection activation after the new contexts report from the expected apps. The committed release runbook owns these operations.