Repository navigation
Conversation
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>
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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-runreview step prints page text unescaped: see inline atdocs/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
--yesis passed, as the PR description notes under "Behavior change for scripts".ts audit ad-templates generatealready shipped inv0.1.0-rc.202609.1andv0.1.1-rc.202610.1, andCHANGELOG.mdrecords CLI breaking changes under[Unreleased]/### Changed(see thets prebid bundlerename). Proposed entry (apply manually:CHANGELOG.mdis 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
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>
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
Summary
ts audit ad-templates generate <url>replaced an existingtrusted-server.tomlas soon as the crawl produced a valid candidate. An operator running it just to inspect a site could change slot definitions by accident, and--replacecould drop configured slots and hand-written slot fields without warning.y/yesbefore 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 pushdon't run.--replacewarns before the question.--yesflag. Without a terminal and without--yes, the run previews the diff and fails without writing.--dry-runis unchanged and conflicts with--yes. A candidate whose managed fields are unchanged is no longer rewritten.Changes
crates/trusted-server-cli/src/commands/audit/generate/mod.rsrun_update_slotstakes an injected answer reader andyes/interactiverequest fields. It computes the managed-field diff for both paths, skips writing an unchanged candidate, and gates the write withconfirm_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--yes(conflicts with--dry-run) and passes aBufReaderover stdin (locked per read, not for the whole crawl) plusinteractive = stdin && stderr are terminalstorun_update_slots.crates/trusted-server-cli/tests/audit_config_diagnostics.rs--yeson non-dry runs, since a test has no terminal to confirm on.crates/trusted-server-cli/src/run.rs--yesdefaults off, parses, and is rejected together with--dry-run.docs/guide/cli.md--yes,--dry-runthen--yesworkflow), plus a--yesexample and a--replacenote.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
--yesis passed. The error names both--yesand--dry-run. An interactive decline also exits non-zero. A re-run whose managed fields are unchanged no longer rewrites the file or printsWrote ..., as #1229 asks.Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo 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, andTS_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:yes please, an empty line, and a BOM-prefixedyes){section}candidate does not print the deploy-order note--yeswith and without a terminal--replacewarning appears before the prompt and the pruned slot shows as removed--replacewarning on a merge--dry-runnever prompts and keeps its stdout diffEight 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
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)