Skip to content

refactor(trace): dedupe downloads_dir secret resolution into one helper - #494

Open
HappyDevs1 wants to merge 1 commit into
mainfrom
fix/491-dedupe-downloads-dir
Open

refactor(trace): dedupe downloads_dir secret resolution into one helper#494
HappyDevs1 wants to merge 1 commit into
mainfrom
fix/491-dedupe-downloads-dir

Conversation

@HappyDevs1

Copy link
Copy Markdown
Collaborator

Summary

  • The ${VAR}-resolution ladder for a browser's downloads_dir was copy-pasted verbatim in three places — flowproof-agent's web_browser_from_setup, flowproof-cli's stage_surface_browser, and flowproof-replay's run_trace_with_exports — each feeding WebBrowserConfig::from_setup_parts. Secret-resolution code with three copies is exactly the shape where a future fix or rule change lands in two of three places and the third silently regresses.
  • Added resolve_downloads_dir() to crates/flowproof-trace/src/secret.rs, next to resolve_refs/resolve_refs_in_json which it composes. All three call-site crates already depend on flowproof-trace directly, so no new dependency edges.
  • Each call site collapses to a one-line call, keeping its own existing error-conversion idiom unchanged (?/#[from] MissingSecret vs. the CLI's manual map_err).
  • Minimal scope, as agreed at plan time: no bundling of viewport/user_agent/args/clock/random/downloads_dir into a struct, no change to from_setup_parts's #[allow(clippy::too_many_arguments)] — that's a larger, separate refactor.
  • Added 4 direct unit tests on the new helper, closing a real gap: none of the three call sites' existing tests exercised the ${VAR}-resolution path (all constructed downloads_dir: None).

Test plan

  • cargo test -p flowproof-trace -p flowproof-agent -p flowproof-cli -p flowproof-replay — all suites green, 0 failures
  • cargo fmt --check
  • cargo clippy -p flowproof-trace -p flowproof-agent -p flowproof-cli -p flowproof-replay --all-targets -- -D warnings

Pure refactor — no behavior change.

Fixes #491

🤖 Generated with Claude Code

The ${VAR}-resolution ladder for a browser's downloads_dir was
copy-pasted verbatim in three places (recorder.rs, cli/lib.rs,
replay/lib.rs), each feeding WebBrowserConfig::from_setup_parts. Three
copies of secret-resolution code is exactly the shape where a future
change lands in two of three places and the third silently regresses.

resolve_downloads_dir() now lives once in flowproof_trace::secret,
next to resolve_refs which it composes. Each call site keeps its own
existing error-conversion idiom. Pure refactor, no behavior change.

Fixes #491
Copilot AI lite review requested due to automatic review settings August 18, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

downloads_dir secret-resolution is triplicated across recorder/cli/replay

2 participants