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
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
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
| .result | ||
| .expect_err("should refuse an update confirmed against a stale file"); | ||
| assert!( | ||
| format!("{error:?}").contains("changed"), |
There was a problem hiding this comment.
🔧 wrench: This assertion passes even if a yes answer is declined.
"changed" is a substring of the decline error not applied; {path} left unchanged, and a decline also leaves the edited file in place, so both checks in this test hold even when the answer path is broken. I mutated confirm_config_write to decline every answer and this test still passed. Matching the refusal's own wording makes it fail under that mutation.
| format!("{error:?}").contains("changed"), | |
| format!("{error:?}").contains("refusing to overwrite"), |
| let stderr = String::from_utf8(err).expect("should decode stderr as UTF-8"); | ||
| result.expect_err("should fail a declined update"); | ||
| assert!( | ||
| stderr.contains("{section}"), |
There was a problem hiding this comment.
🔧 wrench: This assertion holds even when no template was inferred.
The literal-path fallback note in unit_template.rs (…crawl more sections to enable a {section} template) also contains {section}. If inference fell back to literal paths, policy would be None, this check would still pass, and the template-aware binary absence check below would pass trivially. Swapping this test's collector for FakeCollector::new(collected_page_with_header_slot()) leaves it green today. Checking for the section_root key the template writes into the preview diff ties the test to a templated candidate; with this change the swapped collector fails it.
| stderr.contains("{section}"), | |
| stderr.contains("+section_root = "), |
| replacement, | ||
| "the edit made during the prompt must not be overwritten" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔧 wrench: The pre-prompt staleness check has no test.
The first ensure_config_unchanged call (line 1167) exists so the operator is not asked to approve a write that will be refused. Deleting it leaves all 69 generate tests passing: the existing collection-time stale test runs with yes: true, so the post-prompt check catches it either way. This test drives an interactive run whose config changes during the crawl and asserts the run fails before prompting or reading an answer. It passes on this head and fails with line 1167 removed.
| } | |
| } | |
| #[test] | |
| fn update_slots_does_not_ask_to_apply_a_config_changed_during_the_crawl() { | |
| let temp = TempDir::new().expect("should create temp dir"); | |
| let (config_path, original) = write_changing_config(&temp); | |
| let replacement = format!("{original}\n# edited while the browser was running\n"); | |
| let collector = MutatingCollector { | |
| collected: collected_page_with_header_slot(), | |
| config_path: config_path.clone(), | |
| replacement: replacement.clone(), | |
| }; | |
| let mut input = io::Cursor::new(b"yes\n".to_vec()); | |
| let mut err = Vec::new(); | |
| let result = run_update_slots( | |
| &UpdateSlotsRequest { | |
| url: "https://publisher.example/", | |
| config_path: &config_path, | |
| existing_creative: None, | |
| page_patterns: &[], | |
| replace: false, | |
| cookies: &[], | |
| dry_run: false, | |
| yes: false, | |
| interactive: true, | |
| scroll: false, | |
| budget: CrawlBudget::default(), | |
| }, | |
| &[("desktop", &collector)], | |
| &mut io::sink(), | |
| &mut err, | |
| &mut input, | |
| ); | |
| let stderr = String::from_utf8(err).expect("should decode stderr as UTF-8"); | |
| let error = result.expect_err("should refuse an update built from a stale file"); | |
| assert!( | |
| format!("{error:?}").contains("refusing to overwrite"), | |
| "the refusal should say the file changed, got {error:?}" | |
| ); | |
| assert!( | |
| !stderr.contains(APPLY_PROMPT), | |
| "a write already certain to be refused must not be offered, got {stderr:?}" | |
| ); | |
| assert_eq!( | |
| input.position(), | |
| 0, | |
| "a stale file should be refused before an answer is read" | |
| ); | |
| assert_eq!( | |
| read_config(&config_path), | |
| replacement, | |
| "the edit made during the crawl must not be overwritten" | |
| ); | |
| } |
| // Checked before asking too, so the operator is not asked to approve a | ||
| // write that is already certain to be refused. | ||
| ensure_config_unchanged(request.config_path, &existing)?; | ||
| confirm_config_write(request, &diff, err, input)?; |
There was a problem hiding this comment.
🤔 thinking: The deploy-order note now appears only after the operator approves.
For a {section} candidate, stderr reads: the preview, Apply these changes to …? [y/N], and only after yes, note: this config now uses a {section} ad-unit template. Deploy a template-aware binary BEFORE pushing it…. In a probe with templated_site_collector() the prompt sits at byte 889 of stderr and the note at byte 963. That note is the most consequential fact about a templated change (an older binary rejects the whole config and serves an error on every route), and it now arrives after the decision it should inform; with && ts config push chained, nothing pauses between the note and the push.
Holding the "now uses" wording back for a declined write makes sense, so a conditionally worded line in the preview would cover both:
// In confirm_config_write, ahead of the --replace warning, when the candidate is templated.
writeln!(
err,
"note: applying this introduces a {{section}} ad-unit template; deploy a \
template-aware binary BEFORE pushing the config."
)
.map_err(write_error)?;Apply manually: it needs uses_template passed into confirm_config_write and update_slots_declined_template_omits_the_deploy_order_note updated to expect the conditional wording, which spans more than one hunk. The sentence at docs/guide/cli.md:465-466 would change with it.
| | The file changed during the crawl or the prompt | Refuses to overwrite it; re-run against the current file. | | ||
|
|
||
| `--yes` is the opt-in for automation and cannot be combined with `--dry-run`. | ||
| A useful pattern is to review a `--dry-run` first, then apply it: |
There was a problem hiding this comment.
🤔 thinking: The review step recommended here prints page-supplied text unescaped.
The prompt preview escapes control and bidi characters (lines 463-465), but --dry-run writes the managed diff to stdout verbatim, and TOML serialization escapes neither bidi controls nor C1 controls. A probe with a GPT div id of div-gpt-ad-\u{202e}redaeh put a raw U+202E on dry-run stdout, and div-gpt-ad-\u{9b}2Jhead put a raw U+009B (8-bit CSI) there. So the "review a --dry-run first" path doesn't get the protection the paragraph above describes.
Two options:
- Add a caveat here: the
--dry-rundiff is printed verbatim, so review it through a viewer that shows control characters (for examplecat -v), or rely on the prompt's escaped preview. - Escape each diff line when stdout is a terminal. Prompt before audit ad-template generation updates an existing config #1229 asks to keep the dry-run stdout contract, so that seems better as a follow-up issue.
| collectors: &[(&str, &dyn AuditCollector)], | ||
| out: &mut dyn Write, | ||
| err: &mut dyn Write, | ||
| input: &mut dyn BufRead, |
There was a problem hiding this comment.
⛏ nitpick: Doc comments were not updated for the confirmation flow.
run_update_slots(lines 848-857): the summary still ends "rewrite the config's slot array in place", and# Errorsdoesn't list the new failures: the operator declines (including an empty line, EOF, or a non-UTF-8 answer), a non-interactive run without--yes, or the config changing during the crawl or while the prompt is open.managed_creative_projection(lines 1315-1319): it now runs on every non-dry-run path, but its doc comment still says "from dry-run output" and its error readsfailed to parse config for dry-run diff. Something likefailed to parse config for the managed-field difffits both paths.
Apply manually: those lines are outside the diff hunks, so this can't be a suggestion.
|
|
||
| let run = run_write_gate(&config_path, &interactive_gate(), &mut input); | ||
|
|
||
| assert_declined(&run, "a declined update"); |
There was a problem hiding this comment.
⛏ nitpick: In the rejected-answer loop every failure reports a declined update, so a regression doesn't say which answer slipped through. Naming the answer makes the failure self-explanatory.
| assert_declined(&run, "a declined update"); | |
| assert_declined(&run, &format!("answer {answer:?}")); |
| `Wrote ...` line. Page-supplied text in the preview is shown with control and | ||
| bidirectional-override characters escaped, so they cannot reorder or hide lines | ||
| of the diff you are approving. The `{section}` deploy-order note is printed | ||
| only once such a config is written (or previewed with `--dry-run`). |
There was a problem hiding this comment.
⛏ nitpick: The note also prints when the candidate already matches the config: the unchanged path returns through emit_template_note (generate/mod.rs:1156) without writing anything.
| only once such a config is written (or previewed with `--dry-run`). | |
| only once such a config is written, previewed with `--dry-run`, or already in | |
| place. |
| // decline instead of a read failure. | ||
| let mut answer = Vec::new(); | ||
| input | ||
| .read_until(b'\n', &mut answer) |
There was a problem hiding this comment.
🌱 seedling: Input typed during the crawl is read as the answer.
In canonical mode the terminal queues whatever the operator types while the crawl runs (a stray Enter to space out progress lines, say), and this read_until consumes the first queued line. An Enter becomes an empty answer, so the run declines and exits non-zero after a multi-minute crawl; a pre-typed y would approve a diff that hadn't been shown yet. It fails safe in the common case. Discarding pending input before prompting (tcflush(STDIN_FILENO, TCIFLUSH)) would close it, but that needs a libc or rustix dependency the CLI doesn't have yet, so a follow-up seems the right size.
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!)