Skip to content

Ask before ad-template generation writes a changed config - #1420

Open
dhruv8sh wants to merge 9 commits into
mainfrom
fix/audit-ad-templates-confirm-write
Open

dhruv8sh wants to merge 9 commits into
mainfrom
fix/audit-ad-templates-confirm-write

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • ts audit ad-templates generate <url> replaced an existing trusted-server.toml as soon as the crawl produced a valid candidate. An operator running it just to inspect a site could change slot definitions by accident, and --replace could drop configured slots and hand-written slot fields without warning.
  • The command now shows the target path and the managed-field diff on stderr, then asks for an explicit y/yes before writing. The default is no. Declining, EOF, or a non-UTF-8 answer leaves the file untouched and exits non-zero, so chained steps such as && ts config push don't run. --replace warns before the question.
  • Automation opts in with a new --yes flag. Without a terminal and without --yes, the run previews the diff and fails without writing. --dry-run is unchanged and conflicts with --yes. A candidate whose managed fields are unchanged is no longer rewritten.

Changes

File Change
crates/trusted-server-cli/src/commands/audit/generate/mod.rs run_update_slots takes an injected answer reader and yes/interactive request fields. It computes the managed-field diff for both paths, skips writing an unchanged candidate, and gates the write with confirm_config_write. The prompt escapes page-supplied control and bidi-override characters in the preview, line by line, and reads the answer as bytes. The {section} deploy-order note is now printed only after a write (or in a dry run or unchanged run), not before a prompt that may be declined. The stale-file check (ensure_config_unchanged) now runs before the prompt and again after confirmation. Adds tests and updates existing call sites.
crates/trusted-server-cli/src/commands/audit/mod.rs Adds --yes (conflicts with --dry-run) and passes a BufReader over stdin (locked per read, not for the whole crawl) plus interactive = stdin && stderr are terminals to run_update_slots.
crates/trusted-server-cli/tests/audit_config_diagnostics.rs The browser-gated generation fixture passes --yes on non-dry runs, since a test has no terminal to confirm on.
crates/trusted-server-cli/src/run.rs Clap tests: --yes defaults off, parses, and is rejected together with --dry-run.
docs/guide/cli.md New "Confirming the write" section (prompt, non-interactive behavior, --yes, --dry-run then --yes workflow), plus a --yes example and a --replace note.

Closes

Closes #1229

Spec

Spec: Not a feature. This fixes #1229 (issue type Bug), which defines the expected behavior and the acceptance criteria.

Behavior change for scripts

Non-interactive runs that previously wrote the config silently now exit with an error unless --yes is passed. The error names both --yes and --dry-run. An interactive decline also exits non-zero. A re-run whose managed fields are unchanged no longer rewrites the file or prints Wrote ..., as #1229 asks.

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: the full AGENTS.md CI gate list run locally (all 11 gates, including the remaining clippy and test aliases, parity, markdown format, build-digest, wasm-clock lint and docs-proposal scripts), plus cargo test --package trusted-server-cli --target x86_64-unknown-linux-gnu (744 unit tests passed; the 9 ignored are the existing browser-gated tests) the TSJS eslint step, and TS_AUDIT_BROWSER_TESTS=1 scripts/test-cli.sh (all browser-gated audit suites against local Chromium)

The new tests feed scripted answers through the injected reader. They never check for a real TTY, launch a browser, or read an environment gate, and none is #[ignore]d, so they always run in CI. They cover:

  • accept, and a table of accepted and rejected answers (including yes please, an empty line, and a BOM-prefixed yes)
  • decline, which exits non-zero and leaves no temp file
  • EOF, and a non-UTF-8 answer (both decline)
  • a page-supplied bidi override is escaped in the preview
  • a declined {section} candidate does not print the deploy-order note
  • no terminal: refuses, does not read piped input, and still previews the diff
  • --yes with and without a terminal
  • the --replace warning appears before the prompt and the pruned slot shows as removed
  • no --replace warning on a merge
  • an unchanged candidate: no prompt, and the file's contents and mtime are untouched
  • --dry-run never prompts and keeps its stdout diff
  • a config edited while the prompt is open is not overwritten

Eight of these failed before the fix, with the file written without confirmation. The other three guard behavior that already held. The clap conflict test and the deploy-order note test were each checked by reverting the guarded behavior and watching the test fail.

Checklist

  • Feature PRs: spec linked above, ending in a review log of two or more agent review rounds (not a feature)
  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

ts audit ad-templates generate now previews the managed-field diff on stderr and asks for an explicit yes before replacing an existing config. Non-interactive runs fail without writing unless --yes is passed, --replace warns before the prompt, and an unchanged candidate is no longer rewritten.

Closes #1229

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh dhruv8sh self-assigned this Oct 19, 2026
A declined or unanswerable confirmation now exits non-zero so chained steps do not run against a config that was not regenerated. The preview escapes page-supplied control and bidi-override characters, a non-UTF-8 answer counts as a decline, the section-template deploy note is printed only once such a config is written or previewed, and stdin is locked per read instead of for the whole crawl.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
The browser-gated generation tests run ts without a terminal, so a non-dry-run write now needs the same explicit --yes that automation uses.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>

@ChristianPavilonis ChristianPavilonis 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.

Approved. Requiring explicit confirmation before writing changed ad-template configuration, with --yes for automation and no rewrite for unchanged managed fields, is a sensible safeguard against accidental configuration changes.

@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

Adds a confirmation gate to ts audit ad-templates generate: a changed candidate is previewed on stderr and written only after an explicit y/yes (or --yes), non-interactive runs refuse without --yes, and an unchanged candidate is no longer rewritten. The runtime behavior matches #1229 and held up under probing. The blocking items are a missing CHANGELOG entry for the script-breaking change and three tests that keep passing when the behavior they guard is broken.

5 of the 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 remaining comments describe the fix in prose because the change spans several hunks or touches lines outside the diff.

Each suggestion was applied on this head alone and together with the others: cargo fmt --check, cargo clippy-cli, and the full trusted-server-cli suite pass (743 tests), each test suggestion fails under the mutation it targets, and the docs suggestion passes the docs prettier check. Separately, this head merged with current main (Rust 1.98.1) passes fmt, clippy-cli, and the CLI suite.

Blocking

🔧 wrench

  • Stale-prompt test passes when every answer is declined: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:5101
  • Declined-template test passes without a template: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:5207
  • Pre-prompt staleness check has no test: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:5109
  • Missing CHANGELOG entry for the breaking change: see body-level finding below

Non-blocking

🤔 thinking

  • Deploy-order note appears only after the operator approves: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:1168
  • Recommended --dry-run review step prints page text unescaped: see inline at docs/guide/cli.md:478

⛏ nitpick

  • Doc comments not updated for the confirmation flow: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:863
  • Rejected-answer loop doesn't name the failing answer: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:4775
  • Docs omit the unchanged path for the deploy-order note: see inline at docs/guide/cli.md:466

🌱 seedling

  • Input typed during the crawl is read as the answer: see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:1306

Cross-cutting / body-level findings

  • 🔧 Missing CHANGELOG entry for the breaking change. Non-interactive runs that used to write the config now exit with an error unless --yes is passed, as the PR description notes under "Behavior change for scripts". ts audit ad-templates generate already shipped in v0.1.0-rc.202609.1 and v0.1.1-rc.202610.1, and CHANGELOG.md records CLI breaking changes under [Unreleased] / ### Changed (see the ts prebid bundle rename). Proposed entry (apply manually: CHANGELOG.md is not in this diff):

    - **Breaking:** `ts audit ad-templates generate` now previews the managed-field diff on stderr and asks before writing a changed config. Runs whose stdin or stderr is not a terminal fail without writing unless `--yes` is passed, so add `--yes` to scripts and CI jobs that apply generated slots. A declined prompt exits non-zero, `--yes` cannot be combined with `--dry-run`, and a candidate whose managed fields are unchanged is no longer rewritten.

CI Status

  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • integration tests: PASS
  • CodeQL: PASS
  • cargo test (ts CLI, native) (macos-latest): PASS
  • vitest: PASS
  • Analyze (python): PASS
  • cargo test (axum native): PASS
  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • Analyze (javascript-typescript): PASS
  • Analyze (javascript-typescript): PASS
  • format-typescript: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • Analyze (rust): PASS
  • format-docs: PASS (required)
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • docs-proposal scripts: PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native) (ubuntu-latest): PASS
  • CLAUDE.md symlink guard: PASS
  • prepare integration artifacts: PASS
  • Analyze (actions): PASS

Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated
Comment thread docs/guide/cli.md
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated
Comment thread docs/guide/cli.md Outdated
Comment thread crates/trusted-server-cli/src/commands/audit/generate/mod.rs
Make the stale-prompt and declined-template tests fail when the behavior they guard breaks, and add a test for the pre-prompt staleness check. Show the deploy-order warning for a {section} candidate in the preview, before the question, instead of only after the write.

Add the breaking-change CHANGELOG entry, note that the --dry-run diff is printed unescaped, and update the doc comments for the confirmation flow.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh
dhruv8sh requested a review from aram356 October 26, 2026 15:10
aram356 and others added 3 commits October 26, 2026 08:21
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…s-confirm-write

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>

# Conflicts:
#	crates/trusted-server-cli/src/commands/audit/generate/mod.rs

This branch has not been deployed

No deployments
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.

Prompt before audit ad-template generation updates an existing config

4 participants