Skip to content

Remove any identifiers from planning docs - #1178

Open
jevansnyc wants to merge 1 commit into
mainfrom
redact-publisher-ids
Open

jevansnyc wants to merge 1 commit into
mainfrom
redact-publisher-ids

Conversation

@jevansnyc

@jevansnyc jevansnyc commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Two stale planning docs on main still 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 grep for each identifier returns clean across the tree
  • Prettier check passes on the modified file
  • No remaining references to the deleted doc

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.
@jevansnyc jevansnyc changed the title Remove publisher-specific ad identifiers from planning docs Remove any identifiers from planning docs Sep 16, 2026
@jevansnyc
jevansnyc requested a review from prk-Jr September 16, 2026 19:55

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the gamNetworkCode key 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 via 26e0137c6 ("replace references with prospect a (#738)"), an earlier sanitization pass over this same fixture that rewrote the publisher domain to test-publisher.com but missed this key. No test asserts on the value — I checked, there are no matches in any .rs file — so replacing it with a placeholder is safe and won't touch assertions.

    Can't be offered as a suggestion because 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_id value (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), and docs/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 main still 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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants