Conversation
Delete the archived publisher IDs audit. It catalogued hardcoded values in gam.rs and prebid.rs; gam.rs no longer exists and the segment string is gone from crates/, so the cleanup it tracked is complete and the only remaining content is the identifiers themselves. Replace the ad server network ID in the PR #680 plan's example config with a fictional value, matching the instruction in that same step to use only example values.
aram356
left a comment
There was a problem hiding this comment.
Summary
Docs-only redaction: deletes the archived publisher IDs audit and replaces one ad server network ID in the PR #680 plan with a placeholder. The change itself is correct and well-scoped — the deleted doc has no remaining references anywhere in the tree, the deletion is inert to the build (nothing include_str!s or path-references it), and all 20 CI checks pass. I confirmed the GAM publisher ID, the Permutive segment string, and the Equativ page/format IDs catalogued in the deleted doc are genuinely gone tree-wide.
The blocker is the stated verification, not the diff: "git grep for each identifier returns clean across the tree" does not hold. One of the two redacted values is still present in a tracked test fixture.
Identifiers are referred to below by file and config key rather than by value, so this review doesn't republish what the PR is removing.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it. The remaining findings are body-level because they concern files outside this PR's diff and can't be auto-applied.
Blocking
🔧 wrench
- Redacted network ID survives in a tracked test fixture — see Cross-cutting below
Non-blocking
🤔 thinking
- A same-class network ID remains in 25 places, including two Rust test files — see Cross-cutting below
⛏ nitpick
- Replacement doesn't match the repo's canonical placeholder — see inline at
docs/superpowers/plans/2026-05-29-pr680-reviewer-findings.md:224
📝 note
- PR body wording overstates the scope of the cleanup — see Cross-cutting below
Cross-cutting / body-level findings
-
🔧 Redacted network ID survives in a tracked test fixture — The value this PR replaces in the plan doc is still present in
crates/trusted-server-core/src/html_processor.test.html(line 59), as the value of thegamNetworkCodekey in an embedded JSON config blob. It appears exactly once in that file, so it's easy to locate by key name. This is the same identifier in the same role, which means the PR's verification claim doesn't hold, and the stated goal — removing these from the working tree, code search, and secret scanning — isn't met while the fixture still carries it.Worth noting this is not a regression from this PR: the value is already on
main, and it entered via26e0137c6("replace references with prospect a (#738)"), an earlier sanitization pass over this same fixture that rewrote the publisher domain totest-publisher.combut missed this key. No test asserts on the value — I checked, there are no matches in any.rsfile — so replacing it with a placeholder is safe and won't touch assertions.Can't be offered as a
suggestionbecause the file isn't part of this PR's diff. Two acceptable resolutions: fold the fixture fix into this PR, or split it into a follow-up — but in the follow-up case, please also correct the "returns clean across the tree" line in the PR body, so the merged record doesn't assert something that isn't true of the repo. -
🤔 A same-class network ID remains in 25 places, including two Rust test files — A different
gam_network_idvalue (11 digits) appears 25 times across five files:crates/trusted-server-core/src/settings.rs(7),crates/trusted-server-core/src/publisher.rs(2),docs/superpowers/plans/2026-04-30-server-side-ad-templates.md(12), anddocs/superpowers/specs/2026-04-15-server-side-ad-templates-design.md(4). Its length matches real GAM network-code format, and it stands out against the obviously fictional placeholders used elsewhere in this repo (99999,12345,123456789).If the value redacted here warranted removal, this one plausibly does too — and it's the substantially larger exposure of the two. Flagging rather than blocking because I can't confirm its provenance from the tree alone, and because it reaches into Rust test assertions, making it a mechanical but non-trivial sweep that belongs in its own PR. Worth a quick check on whether it's real before it accumulates more call sites.
-
📝 PR body wording overstates the scope of the cleanup — The body opens with "Two stale planning docs on
mainstill carried publisher-specific ad identifiers." One doc was deleted and the other edited, and per the wrench above, the identifiers were not confined to those two docs. A small wording fix keeps the merged history accurate about what was and wasn't cleaned up.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
All 20 checks pass, including all four required gates. I also re-ran the docs Prettier check locally against the edited file and it passes.
| ```toml | ||
| [creative_opportunities] | ||
| gam_network_id = "88059007" | ||
| gam_network_id = "12345678" |
There was a problem hiding this comment.
⛏ nitpick — The line just above says "Use only example/fictional values per project convention," but this placeholder doesn't match the one the repo already uses everywhere else. trusted-server.example.toml:332 and docs/guide/configuration.md both use a 9-digit placeholder; this introduces an 8-digit one, so the plan doc's example now disagrees with the config template it's telling the reader to write.
Aligning them avoids establishing a second placeholder convention:
| gam_network_id = "12345678" | |
| gam_network_id = "123456789" |
Scratch-verified in an isolated worktree: the docs Prettier gate passes with this applied, and a byte-exact before/after patch comparison confirms nothing else in the tree changed.
Two stale planning docs on
mainstill carried publisher-specific ad identifiers. Both describe work that has since shipped.Changes
certain parameters that are available in the browser, but could be considered sensitive have been removed.
Scope
Docs only, no code or config touched. These values remain in git history (they trace back to the initial commit), so this is not a purge. It removes them from the working tree, code search, and secret scanning.
Verification
git grepfor each identifier returns clean across the tree