[ChristianPavilonis]

Summary

• Extract lockr's public app ID from rendered page metadata so publishers do not need to copy it into the audit-generated draft.
• Record the normalized ID and metadata source in the audit artifact. Populate a disabled lockr config section and retain manual review for SDK/API behavior on the serving domain.
• Leave missing or empty IDs on the manual-review path without inventing a value.

Changes

File	Change
crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs	Detect lockr metadata and extract the first nonempty, trimmed content value.
crates/trusted-server-cli/src/commands/audit/generate/mod.rs	Serialize the discovered ID, render an escaped app ID with enabled = false, and add offline file-output regressions.
docs/guide/cli.md	Document metadata extraction, audit evidence, disabled config, and manual review.
docs/guide/integrations/lockr.md	Link to automatic app ID discovery from the configuration guide.

Closes

Closes #1495

Test plan

• New offline regression fails against the original behavior because the audit artifact lacks the discovered ID.

• cargo +1.98.1 test -p trusted-server-cli --target x86_64-unknown-linux-gnu: 951 passed, 30 ignored, no failures on the combined state after merging current main.

• Offline regressions exercise metadata-only detection, concurrent script detection, whitespace normalization, empty-first metadata, HTML entity decoding, TOML escaping, missing/empty content, manual review, and deploy validation.

• cargo +1.98.1 fmt --all -- --check

• git diff --check

• cargo +1.98.1 clippy-cli --target x86_64-unknown-linux-gnu

• cargo +1.98.1 run --package trusted-server-cli --target x86_64-unknown-linux-gnu --bin ts -- dev lint domains --changed-vs origin/main

• Docs formatting: cd docs && npx prettier --check guide/cli.md guide/integrations/lockr.md

• Docs site build and link validation: cd docs && npm run build

Live lockr SDK/API behavior and the existing ignored browser/platform fixtures were not exercised. Adapter and standalone JS suites were not run. The documentation-only follow-up leaves the previously tested Rust sources unchanged.

Review follow-up

• Initialized the optional ID in the Didomi fixture added on current main, preventing a merged-test compilation failure.
• Shared offline lockr page setup between the two regression tests.
• Delegated correctness, architecture, code-smell, and style reviewers rechecked implementation commit cc11942bb. No remaining code findings. The later guide update passed docs formatting and site build checks.

Checklist

• Changes follow repository conventions.
• New behavior has offline regression coverage.
• No production unwrap() or new logging dependencies.
• Test data uses fictional public identifiers and publisher domains.
• Finding an app ID does not enable the integration.

[prk-Jr]

Summary

Approving. Extracting the public lockr app ID from page metadata is well-scoped: the ID lands in the audit artifact, the draft config gets an escaped app_id with enabled = false, and missing/empty IDs stay on the manual-review path.

Non-blocking

👍 praise

• The generated [integrations.lockr] section cannot collide with the starter template (its lockr section is fully commented), and subsequent slot tables keep the draft valid TOML.
• app_id goes through the shared toml_string escaping, and the offline tests cover quotes, backslashes, entity-decoded newlines, empty-first metadata, and deploy validation of the result.
• The tests run the full run_generate path both with and without a concurrent lockr script, so they pin the metadata-evidence precedence and the "finding an ID does not enable lockr" contract.

CI Status

• All 29 checks pass on a0a8d1367.
• Audit-generate CLI tests also pass locally at the PR head (280 passed, 6 ignored).

[dhruv8sh]

Summary

Adds lockr app ID extraction from lockr-signin-app_id page metadata to ts audit generate, records it in the audit artifact, and drafts a disabled [integrations.lockr] section. Well-contained, keeps the integration disabled and on the manual-review path, and thoroughly covered by offline regressions. Only non-blocking observations below.

Non-blocking

• 🤔 Live lockr section appended separately from the template's commented block — inline at generate/mod.rs:459
• 🤔 IDs containing control characters are accepted and asserted valid — inline at analyzer.rs:76
• 📝 Metadata without content still marks lockr detected — inline at analyzer.rs:73
• ⛏ Selector could be parsed once via LazyLock — inline at analyzer.rs:70
• 👍 Thorough offline tests and safe enabled = false default — inline at generate/mod.rs:2380

CI Status

All 29 checks: PASS.

[aram356]

Summary

Extracting lockr's public app ID from page metadata is well scoped, and the extraction itself is thoroughly tested. The blocking problem is the generated draft: it appends a second [integrations.lockr] table while the template's commented example stays in place, and uncommenting that example, which is the template's documented workflow, makes the draft fail to load (repro inline). The docs also ask operators to complete checks before enabling lockr that only work after it is enabled.

5 of the 6 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. The mod.rs:459 comment describes its fix in prose because it adds a helper outside the diff. Every suggestion was applied on its own and together on a scratch worktree at f1b5d4a6f and checked with cargo fmt --all -- --check, cargo clippy-cli, and cargo test -p trusted-server-cli --lib (767 passed); the docs ones with prettier and vitepress build.

Blocking

🔧 wrench

• Draft carries two lockr blocks; uncommenting the template one is a duplicate-table error — see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:459
• CLI guide asks for verification checks that need lockr enabled — see inline at docs/guide/cli.md:355
• Lockr guide has the same problem and no pointer to origin_override — see inline at docs/guide/integrations/lockr.md:20
• Conflicting page IDs are picked without a warning — see inline at crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs:76
• No test that the generated section can be enabled — see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:2444
• Three loop asserts have no message — see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:2420

Cross-cutting / body-level findings

• 🔧 Re-run the two failed CI jobs — prepare integration artifacts and worker-deadlines (spin) failed on f1b5d4a6f with HTTP 504 while downloading GitHub release assets (wasm-bindgen-0.2.126-x86_64-unknown-linux-musl.tar.gz and spin-v4.1.0-linux-amd64.tar.gz). That is infrastructure, unrelated to this diff, and neither check is required by branch protection; a re-run should clear both.
• 🔧 Answer and resolve the five open review threads — main requires review-thread resolution before merge, and the five threads from the earlier review (mod.rs:459, analyzer.rs:70, analyzer.rs:73, analyzer.rs:76, mod.rs:2380) have no reply, so the PR cannot merge as it stands regardless of approvals.
• 🔧 File a follow-up for the unused app_id — LockrConfig.app_id must be nonempty when lockr is enabled, but nothing reads it after validation: crates/trusted-server-core/src/integrations/lockr.rs never uses self.config.app_id. That is why the selection rules in this PR (trim, first nonempty, conflicting values) have no runtime effect today. Please open an issue to either consume the ID or drop the requirement, and link it here; the code change itself is out of scope for this PR.

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• HTML EOF runtime (Fastly): PASS
• worker-deadlines (cloudflare): PASS
• worker-deadlines (spin): FAIL (HTTP 504 fetching the Spin release; see above)
• prepare integration artifacts: FAIL (HTTP 504 fetching the wasm-bindgen release; see above)
• vitest: PASS
• ts dev lint domains (changed lines): PASS
• docs-proposal scripts: PASS
• CI Node pin guard: PASS
• CLAUDE.md symlink guard: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (python): PASS
• Analyze (actions): PASS
• CodeQL: PASS
• integration tests: SKIPPED
• integration tests (Fastly EC lifecycle): SKIPPED
• browser integration tests: SKIPPED
• trace runtime and browser acceptance: SKIPPED

[ChristianPavilonis]

Review follow-up

Pushed 41dcf1f.

• Filled the existing lockr template block in place, preserving optional-setting comments, escaped IDs, enabled = false, and the missing-ID manual-review path.
• Added one warning for distinct normalized IDs without changing first-nonempty selection or echoing page-controlled values.
• Added enabled-config validation, single-table and optional-setting regressions, and descriptive assertion messages.
• Corrected both guides and the generated comment to enable and verify on staging before production. The guides explain origin_override with an authorized, registered publisher origin.

All 11 inline review points have individual responses, including the no-change decisions, and their threads are resolved.

Review summaries

@prk-Jr, the guarantees noted in your approval remain covered. Although the original disabled draft parsed correctly, the later review identified a duplicate-table failure when operators followed the template's uncommenting instructions. In-place generation now removes that trap.

@dhruv8sh, every point in your review has a thread response. Control-character filtering was declined because it would impose an unsupported ID-format restriction on the agreed nonempty extraction contract. The escaping regression remains, and does not claim upstream SDK acceptance. Selector caching was deferred because there is no demonstrated performance problem. Metadata-only detection and disabled generation are unchanged.

@aram356, all six inline requests in your review are addressed in this commit. For the body-level requests:

• Replied to and resolved all five earlier threads, as well as the six newer ones.
• Filed Resolve unused lockr app_id configuration requirement #1519 to decide whether the runtime should consume app_id or remove its unused nonempty requirement. This PR does not change that runtime contract.
• Re-requested the failed jobs in Integration Tests and Worker auction deadline runtime regressions. Both failures were HTTP 504 release downloads. Those retries test the previously reviewed f1b5d4a6f revision, not the new head, so they do not replace current-head CI.
• Re-requested your review.

Verification

The two new behavioral regressions failed before the production fixes and passed afterward.

• cargo test -p trusted-server-cli --target x86_64-unknown-linux-gnu: 952 passed, 30 ignored, no failures.
• cargo clippy-cli --target x86_64-unknown-linux-gnu: passed.
• cargo fmt --all -- --check: passed.
• ts dev lint domains --changed-vs origin/main: passed.
• Docs Prettier checks and npm run build: passed.
• git diff --check: passed.

Live lockr SDK/API behavior, the ignored browser/platform fixtures, and separate adapter/JS suites were not exercised locally. Current-head CI is not yet complete.

[aram356]

Summary

Round 2 at 41dcf1ffd. Every round-1 finding is addressed. The draft now fills the template's lockr block in place: I re-probed uncommenting every optional lockr key and enabling the section, and the result parses and passes check_candidate. A template edit that breaks the replacement also fails run_generate_extracts_lockr_app_id_from_page_metadata, so it can't drift silently. Both guides now route verification through staging and point at origin_override. Conflicting IDs warn, the enabled-config and single-table regressions are in, #1519 tracks the unused app_id, all 11 threads are resolved, and CI is green on this head.

One new blocker, which my own round-1 suggestion introduced: the conflicting-ID warning leaks into ts audit ad-templates generate with wording that only fits ts audit generate.

The inline comment describes its fix in prose because it spans analyzer.rs and mod.rs. The fix and its regression test were scratch-verified at 41dcf1ffd (cargo fmt --all -- --check, cargo clippy-cli, cargo test -p trusted-server-cli --lib: 769 passed), and the test fails without the fix.

Blocking

🔧 wrench

• Lockr conflict warning leaks into ad-template notes with wrong wording — see inline at crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs:84

Cross-cutting / body-level findings

• 📝 Merge in main before merging — main requires branches to be up to date and this one is behind by #917 (6fbe64aa0, adds [publisher].public_origin). I merged origin/main into this head locally: no conflicts, and cargo test -p trusted-server-cli passes on the merged tree (768 lib tests plus the integration targets).

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• HTML EOF runtime (Fastly): PASS
• worker-deadlines (cloudflare): PASS
• worker-deadlines (spin): PASS
• prepare integration artifacts: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• trace runtime and browser acceptance: PASS
• vitest: PASS
• ts dev lint domains (changed lines): PASS
• docs-proposal scripts: PASS
• CI Node pin guard: PASS
• CLAUDE.md symlink guard: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (python): PASS
• Analyze (actions): PASS
• CodeQL: PASS

[aram356]

🔧 wrench — This warning now leaks into ts audit ad-templates generate, which writes no lockr config. It came from my round-1 suggestion: I missed that fold_collected turns every analyzer warning into a per-page note.

Repro at 41dcf1ffd: run_update_slots over a page with two distinct lockr-signin-app_id values, with desktop and mobile profiles, prints on stderr:

note: `/` on desktop: page carries more than one distinct lockr app ID; the draft uses the first
note: `/` on mobile: page carries more than one distinct lockr app ID; the draft uses the first

"the draft uses the first" is wrong for that command, which only rewrites [creative_opportunities] slots. And because the note is scoped per page and profile, a shared <head> repeats it for every crawled page (the default budget is 17 pages per profile). fold_collected already avoids that kind of repetition for CONSENT_STUB_WARNING.

Proposed fix (apply manually; it touches analyzer.rs and mod.rs): name the warning, and skip it in fold_collected the same way the consent stub is special-cased.

analyzer.rs, next to LOCKR_APP_ID_SELECTOR:

/// Recorded when page metadata carries more than one distinct lockr app ID.
pub(super) const LOCKR_APP_ID_CONFLICT_WARNING: &str =
    "page carries more than one distinct lockr app ID; the draft uses the first";

analyzer.rs, here:

        warnings.push(LOCKR_APP_ID_CONFLICT_WARNING.to_string());

mod.rs, at the top of the warning loop in fold_collected:

    for warning in &artifact.warnings {
        // Lockr app IDs only feed the `ts audit generate` draft. Slot generation
        // writes no lockr settings, so the conflict is not actionable here.
        if warning == analyzer::LOCKR_APP_ID_CONFLICT_WARNING {
            continue;
        }

Regression test for the mod.rs tests module:

    #[test]
    fn update_slots_omits_lockr_app_id_conflicts_from_notes() {
        let temp = TempDir::new().expect("should create temp dir");
        let config_path = temp.path().join("trusted-server.toml");
        fs::write(
            &config_path,
            "[creative_opportunities]\ngam_network_id = \"123456789\"\n",
        )
        .expect("should write config");
        let mut page = collected_page_with_header_slot();
        page.html = r#"<html><head><meta name="lockr-signin-app_id" content="first-app-id"><meta name="lockr-signin-app_id" content="second-app-id"></head></html>"#.to_string();
        let collector = FakeCollector::new(page);
        let mut err = Vec::new();

        run_update_slots(
            &UpdateSlotsRequest {
                url: "https://publisher.example/",
                config_path: &config_path,
                existing_creative: None,
                page_patterns: &[],
                replace: false,
                cookies: &[],
                dry_run: true,
                yes: false,
                interactive: false,
                scroll: false,
                budget: CrawlBudget::default(),
            },
            &[("desktop", &collector)],
            &mut Vec::new(),
            &mut err,
            &mut io::empty(),
        )
        .expect("should preview generated slots");

        let notes = String::from_utf8(err).expect("should write UTF-8 notes");
        assert!(
            !notes.contains("lockr app ID"),
            "should not report a generate-only lockr conflict during slot generation: {notes}"
        );
    }

Scratch-verified at 41dcf1ffd: cargo fmt --all -- --check clean, cargo clippy-cli clean, cargo test -p trusted-server-cli --lib 769 passed / 0 failed. Removing the fold_collected filter makes the new test fail. Your analyze_collected_page_warns_only_on_distinct_lockr_app_ids test passes unchanged, and it can compare against the constant if you prefer.