tech_debt_cleanup: resolve P1/P2/P3 review backlog (~145 todos) + 4 critical PR-review fixes

[unclesp1d3r]

Summary

tech_debt_cleanup resolves ~145 review-backlog items spanning P1/P2/P3, plus 4 follow-up critical/high findings from a focused PR review of the result. 50 commits, 141 files changed (+13,022 / -4,291), 745/745 tests pass, cargo deny check clean.

The branch closes the four major architecture gaps previously flagged in AGENTS.md:

• F005 (exit codes): typed GoldDiggerError enum with downcast-based classifier; substring matching demoted to legacy fallback.
• F007 (streaming output): RowSink trait + conn.query_iter pipeline; output streams through <path>.tmp and atomic-renames on success. Peak memory is now O(1 row), not O(N rows).
• F008 (structured logging): tracing + tracing-subscriber; every error string routed through utils::redact_sql_error before reaching the subscriber.
• F001-F003 (CLI-first config): ResolvedConfig::from_cli projects Cli into a fully-validated config exactly once at the binary boundary; downstream code never touches Cli.

Plus 4 compound docs in docs/solutions/best-practices/ capturing the durable patterns from this work.

Notable changes

Architecture

• main.rs shrank from ~1050 LOC to 55 LOC; logic extracted into run.rs, config.rs, connection.rs, completion.rs, output.rs, sink.rs, logging.rs.
• tls.rs (1911 LOC) split into src/tls/ directory: mod, error, classifier, config, ca, pool.
• Typed GoldDiggerError + ConfigError + TlsError enums replace ad-hoc anyhow! strings at error origin sites.
• ResolvedConfig (with ResolvedQuery, OutputTarget, EnvSnapshot) front-loads validation; mutually-exclusive flags become enum variants.
• Streaming RowSink trait with per-format implementations (DelimitedSink for CSV/TSV, JsonSink); atomic <output>.tmp → rename commit, Drop-based cleanup on error.
• CaFile newtype validates PEM at construction; TlsConfig collapsed into TlsValidationMode.

Security hardening

• Path safety: --query-file canonicalised + size-capped (10 MiB default, configurable via --max-query-file-size) + extension deny-list; --output uses O_NOFOLLOW + 0o600 + create_new(true); --force opt-in to overwrite.
• Atomic-rename TOCTOU fix: RENAME_NOREPLACE on Linux, hard_link+unlink on other Unix, OpenOptions::create_new on Windows; preserve .tmp on rename failure for user recovery.
• Credential redaction unified through utils::redact_sql_error / redact_url; \b word-boundary patterns; cross-redactor placeholder consistency.
• Typed mysql::Error classifier in tls/pool.rs::classify_mysql_pool_error replaces ~80-line substring cascade.
• redact_url fail-closed on set_password/set_username failure (parseable-but-unredactable URLs return REDACTED_URL_PLACEHOLDER).
• validate_time uses u64 arithmetic to prevent u32 overflow on hostile MySQL Time wire data.

UX

• Structured tracing-subscriber stderr; coloured [DANGER] / [WARNING] banners (respects NO_COLOR and TTY detection); indicatif progress spinner during connect / query / write phases.
• ProgressGuard RAII wrapper ensures finish_and_clear fires on every return path.

Testing

• Test count: ~280 → 745 (+465 across the branch).
• New end-to-end coverage for exit codes 1, 3, 4, 5 (each driven by real binary invocations against assert_cmd).
• tests/credential_leak_regression.rs, credential_leak_sweep.rs, redact_sql_error_truth_table.rs, dump_config_redaction.rs — adversarial credential-leak coverage with high-entropy sentinels.
• tests/atomic_output.rs, path_safety.rs, sink_atomicity.rs — atomic-rename + O_NOFOLLOW + state-guard coverage.
• tests/tls_classifier_rcgen.rs — rcgen-driven exhaustive classifier tests for rustls::Error variants.
• tests/init_crypto_provider_idempotency.rs, tls_danger_banner_snapshot.rs, additional_mysql_types.rs, stream_separation.rs — closed coverage gaps.
• proptest invariants in src/exit.rs (1024 cases each) for typed-error stability.
• Helper consolidation: tests/test_support/cli.rs::clean_cmd() is the canonical assert_cmd::Command builder; deprecated Command::cargo_bin() callers migrated to the cargo::cargo_bin_cmd! macro.

CI / devops

• New workflows: coverage.yml (80% threshold), cargo-audit-pr.yml (PR gate), benchmarks.yml (containerised MySQL), codeql.yml (Rust support).
• dependabot.yml: grouped github-actions updates, weekly cadence.
• cargo cyclonedx for SBOM (replaces Syft to avoid stale-lockfile false positives).
• Bumped aws-lc-rs 1.16.0 → 1.16.3 (cascades aws-lc-sys 0.37.1 → 0.40.0) to clear RUSTSEC-2026-0045/0046/0047/0048.
• Bumped mysql and rustls deps; added nix (Linux-only), itoa, ryu, tracing, tracing-subscriber, indicatif, owo-colors, proptest (dev), rcgen (dev).

Documentation

• docs/solutions/best-practices/ (new): 4 compound docs capturing the durable patterns:
  • drop-safe-atomic-file-output-rust-cli-2026-04-25.md — process::exit + Drop interaction, atomic-rename platform matrix
  • replace-substring-error-classifiers-with-typed-enums-2026-04-25.md — typed enum + downcast + anyhow::Error::from(typed).context(...) discipline
  • front-load-validation-with-resolved-config-type-2026-04-25.md — parse-don't-validate Cli → ResolvedConfig at the binary boundary
  • unified-credential-redaction-rust-binary-2026-04-25.md — single redactor module, fail-closed regex compile, sentinel + corpus testing
• AGENTS.md refreshed: closed-gaps section reflects current state; stale resolve_config_value block replaced with description of ResolvedConfig::from_cli; "Documented Solutions" pointer added so future agents discover docs/solutions/.
• SECURITY.md: new "Operational Risk Surface" section documenting --allow-invalid-certificate (MITM exposure), --query-file (file-read scope), --dump-config (best-effort redaction).
• README.md: "Known Limits" section (memory profile pre-streaming), --allow-invalid-certificate MITM warning, --query-file safety note, cross-links to tests/README.md and benches/README.md.
• CHANGELOG.md [Unreleased] rewritten with breaking/behavior section + features section.

Test plan

• just ci-check passes locally: 745 tests, 0 failures, 0 skipped, advisories/bans/licenses/sources all ok
• No unsafe introduced
• No new clippy warnings
• cargo fmt --check clean
• Snapshot tests up to date (insta)
• No eprintln! remaining in src/ (all routed through tracing)
• No process::exit from inside run::run / stream_query (so Drop runs on error paths)
• No raw mysql_error interpolation without redact_sql_error (verified by tests/credential_leak_sweep.rs)
• CI runs the new coverage.yml, cargo-audit-pr.yml, benchmarks.yml, codeql.yml workflows successfully on this PR
• CodeRabbit review pass (per project convention)
• Manual smoke test against a real MySQL instance pre-merge

Notes for review

This PR is intentionally large because the architecture changes are tightly coupled — the typed-error pipeline, the Result-returning run, the Drop-safe sink, and the ResolvedConfig boundary all reinforce each other. Splitting into smaller PRs would have produced an intermediate state where individual fixes were either unsafe (typed errors without preservation) or inert (atomic rename without Drop running). Commits are individually buildable and atomic; reviewing commit-by-commit may be easier than diff-by-diff.

Recommended commit-walk order for review:

1. 1ea0c3d (foundation: typed errors + Result-returning run) — the substrate everything else builds on
2. e3b9e7e (sink atomicity) — TOCTOU + state guard + .tmp preservation
3. 5d6458e (type-transformer + redaction polish) — defensive hardening across multiple modules
4. b11dc7b (tls.rs split) — large but mechanical; tests prove behaviour preserved
5. 74e2a5d (main.rs extraction) — also large, also mechanical
6. f052c46 (F007 streaming) — semantic change in the data pipeline
7. d42c7cb (ResolvedConfig) — the validation boundary
8. The compound docs (620c729-6e53abb) capture the design rationale for items 1-7

Open follow-ups intentionally NOT in this PR (tracked separately):

• #013 SHA-pin release.yml actions (touches user's in-progress workflow file)
• #164 SIGINT handler for clean shutdown (a SIGINT bypasses Drop, so .tmp files orphan on Ctrl-C; this is a separate signal-handling concern)
• A handful of P3 polish items deferred for v0.4.0+

[Copilot]

Pull request overview

Copilot reviewed 97 out of 144 changed files in this pull request and generated 5 comments.

[codecov]

Codecov Report

❌ Patch coverage is 91.42756% with 242 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/output.rs	49.29%	36 Missing ⚠️
src/tls/pool.rs	69.36%	34 Missing ⚠️
src/config.rs	93.81%	29 Missing ⚠️
src/tls/classifier.rs	84.65%	29 Missing ⚠️
src/logging.rs	70.83%	28 Missing ⚠️
src/tls/ca.rs	84.89%	21 Missing ⚠️
src/run.rs	85.60%	19 Missing ⚠️
src/tls/error.rs	95.25%	12 Missing ⚠️
src/connection.rs	76.74%	10 Missing ⚠️
src/sink.rs	98.21%	7 Missing ⚠️
... and 7 more

📢 Thoughts on this report? Let us know!

[coderabbitai]

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 23

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

tests/integration/common.rs (1)

1-2015: 🧹 Nitpick | 🔵 Trivial

File exceeds recommended length guideline.

At 2015 lines, this file significantly exceeds the ≤1000 line (preferably ≤500) guideline. Consider splitting into separate modules:

• cli.rs for GoldDiggerCli and command execution
• parsing.rs for OutputParser and format validation
• temp.rs for TempFileManager and TestIsolation
• predicates.rs for CliPredicates and validation predicates

This would improve maintainability and reduce cognitive load when navigating the test utilities.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/common.rs` around lines 1 - 2015, The file is too large
(2015 lines); split it into focused modules to improve maintainability by
extracting related types and functions: create cli.rs and move GoldDiggerCli
(and its impls: execute, execute_with_assertions, execute_with_timeout,
execute_raw, execute_with_output_validation, execute_with_snapshot,
redact_sensitive_output) and AssertCmdIntegration into a cli module; create
parsing.rs and move OutputParser, CsvParseResult/JsonParseResult/TsvParseResult,
and OutputValidator + related predicate structs (CsvContentPredicates,
JsonContentPredicates, TsvContentPredicates) into a parsing module; create
temp.rs and move TempFileManager, TestIsolation, TestEnvironment,
TempFileManager methods and TempFileManager Drop into a temp module; create
predicates.rs and move CliPredicates and related predicate helpers into it;
update the original tests/integration/common.rs to declare mod cli; mod parsing;
mod temp; mod predicates; re-export the moved symbols (pub use
crate::cli::GoldDiggerCli; etc.) and update use paths inside moved code to
reference the new module boundaries (e.g., references to OutputParser in cli ->
parsing::OutputParser or re-exported names), ensure ENV_VARS_TO_REMOVE and
TestCase/OutputFormat/GoldDiggerResult remain accessible (keep them in common.rs
or a small shared mod) and run cargo check to fix any import visibility issues.

tests/integration/tls_tests.rs (2)

40-47: ⚠️ Potential issue | 🟠 Major

std::process::exit(0) in test skip logic bypasses test framework.

Using std::process::exit(0) terminates the entire test process, not just the current test case. This can mask failures in other tests running in the same process and prevents proper test reporting.

Consider using a proper test-skip mechanism or restructuring with conditional compilation:

🛠️ Proposed fix using early return with explicit skip

 /// Skip test if Docker is not available
-fn skip_if_no_docker() {
+fn skip_if_no_docker() -> bool {
     if !is_docker_available() {
         println!("Skipping test: Docker not available");
-        // Use proper test skipping mechanism
-        std::process::exit(0); // Exit gracefully for skipped tests
+        return true;
     }
+    false
 }

Then update call sites to return early:

fn test_platform_certificate_store(#[case] db_flavor: &str) -> Result<()> {
    if skip_if_no_docker() {
        return Ok(());
    }
    // ... rest of test
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/tls_tests.rs` around lines 40 - 47, The helper
skip_if_no_docker currently calls std::process::exit(0), which aborts the whole
test run; change skip_if_no_docker to return a bool (e.g., fn
skip_if_no_docker() -> bool) or Result to indicate skipping instead of exiting,
remove the std::process::exit(0) call, and update callers (like
test_platform_certificate_store) to check skip_if_no_docker() and return early
(e.g., if skip_if_no_docker() { return Ok(()); }) so only the individual test is
skipped and the test framework continues reporting other tests.

---

639-754: ⚠️ Potential issue | 🔴 Critical

Add missing imports for CertificateValidator and CertificateLoader from the fixtures module.

The ephemeral_certificate_tests module uses CertificateValidator and CertificateLoader (defined in tests/fixtures/tls/mod.rs), but these types are not imported in tests/integration/tls_tests.rs. The use super::*; in the submodule only brings in what's available in the parent scope, so this will fail to compile.

Add to the imports in tests/integration/tls_tests.rs:

use crate::fixtures::tls::{CertificateValidator, CertificateLoader, EphemeralCertificate};

Or import them separately if preferred. Without this, all test functions in ephemeral_certificate_tests will fail with "cannot find type" errors.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/tls_tests.rs` around lines 639 - 754, The tests in the
ephemeral_certificate_tests module reference CertificateValidator,
CertificateLoader, and EphemeralCertificate but do not import them, causing
"cannot find type" compile errors; add imports for these symbols from the
fixtures tls module (e.g., import CertificateValidator, CertificateLoader,
EphemeralCertificate from crate::fixtures::tls or equivalent) at the top of
tests/integration/tls_tests.rs so the submodule can resolve those types
referenced in test_ephemeral_certificate_generation,
test_certificate_loading_utilities, and test_certificate_validation_utilities.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/dependabot.yml:
- Around line 15-16: YAML lint fails due to spaces inside inline array brackets;
update the inline arrays (the keys patterns and update-types) to remove inner
spacing so they read like patterns: ["*"] and update-types: ["minor","patch"]
(also apply the same fix to the other occurrences mentioned for the file).
- Around line 30-39: The "security-sensitive" group aggregates multiple crates
into one PR, which contradicts the comment that each crate should be reviewed
and held back individually; update the Dependabot config so matching crates are
not batched by this group—either remove the "security-sensitive" group and
create distinct entries for each crate pattern (the listed "mysql*", "rustls*",
"clap*", "tokio*", "serde*") or change the group settings to disable aggregation
so each pattern produces its own PR (update the "security-sensitive" grouping
behavior referenced by the group name and the "patterns" list).

In @.github/workflows/benchmarks.yml:
- Around line 8-13: The YAML linter flags extra spaces inside branch list
brackets for the workflow triggers; update the bracketed branch arrays under the
on block (the pull_request and push entries) from "branches: [ main ]" to
"branches: [main]" to make formatting consistent and satisfy the linter while
leaving behavior unchanged.

In @.github/workflows/cargo-audit-pr.yml:
- Around line 10-12: The YAML lint fails due to bracket-spacing in the
workflow's branch lists; update the two occurrences of "branches: [ main ]" to
remove the inner spaces so they read "branches: [main]" or, alternatively,
convert them to explicit YAML sequences like "branches:\n  - main"; update the
entries for both occurrences (the two "branches: [ main ]" lines) so the spacing
rule is satisfied.

In @.github/workflows/ci.yml:
- Around line 5-7: YAML lint fails due to spaces inside bracketed sequence
literals; update the branch and needs list syntaxes (the "branches:" entries and
any "needs:" list like the one around line 215) to remove inner spaces so
bracketed lists use the normalized form (e.g., change "[ main ]" and "[
test-tls, ... ]" to "[main]" and "[test-tls,...]" respectively) across the CI
workflow file to satisfy the bracket-spacing rule.

In @.github/workflows/codeql.yml:
- Around line 3-37: YAMLlint flags are caused by bracketed arrays with spaces
and the bare on key; update the workflow to use YAMLlint-safe styles by (1)
quoting the top-level key "on" (e.g., "on":) to avoid truthy-warning and (2)
normalize array syntax for keys like branches and language by removing spaces
inside brackets or converting to block lists (e.g., change branches: [ main ] to
branches: [main] or to branches: - main and matrix: language: - rust). Apply
these changes for all occurrences (branches, matrix.language, cron array) and
keep existing keys like concurrency, defaults, permissions, and jobs names
intact.

In @.github/workflows/coverage.yml:
- Around line 3-8: The bracket spacing in the workflow trigger arrays is
inconsistent; update the array literals under the on: push: branches and on:
pull_request: branches (and any similar arrays like workflow_dispatch if
present) to use compact bracket syntax (e.g., change "[ main ]" to "[main]") so
spacing matches the style used in benchmarks.yml.

In @.github/workflows/scorecard.yml:
- Line 15: The YAML lint error is caused by extra spaces inside the inline array
for the branches key; update the branches declaration (branches: [ "main" ]) to
remove the inner spaces so it becomes branches: ["main"] to satisfy YAMLlint's
inline-array spacing rule.

In @.github/workflows/security.yml:
- Around line 5-6: The YAML arrays "workflows: [ CI ]" and "types: [ completed
]" use inline spacing that fails YAMLlint; update them to the linter's
bracket-spacing style by normalizing spacing inside the brackets (e.g., remove
or adjust spaces around items so they match project lint rules) for the entries
currently shown as workflows and types in the security.yml workflow definition.

In `@AGENTS.md`:
- Around line 127-139: Replace the redundant phrase "comprehensive CLI
interface" in the Project Overview with a non-redundant alternative (e.g.,
"comprehensive CLI" or "comprehensive command-line interface"); locate the exact
text "comprehensive CLI interface" and update it so the description reads
cleanly (for example: "It features a comprehensive CLI" or "It features a
comprehensive command-line interface"), leaving surrounding lines like
"CLI-first with environment variable fallbacks using `clap`" unchanged.

In `@SECURITY.md`:
- Around line 101-104: The SECURITY.md claim that created output files are
always set to 0o600 conflicts with the repo policy to respect the system umask;
update the doc and/or implementation so they agree: either change the
SECURITY.md bullet ("0o600 permissions (Unix)") to state that files are created
with owner-only intent but the process respects the system umask (while still
enforcing O_CREAT | O_EXCL, O_NOFOLLOW and --force semantics), or change the
code that opens output files to apply the explicit 0o600 mode only after
clearing/applying the umask so the effective permissions respect the system
umask; ensure the wording references the same behaviors (O_CREAT | O_EXCL /
create_new, O_NOFOLLOW, --force, and the permission semantics) so documentation
and implementation match.

In `@src/completion.rs`:
- Around line 10-35: The public funcs generate_completion_to<W: Write> and
generate_completion are missing the required rustdoc sections; add standardized
/// docs for each including a short description plus # Arguments (documenting
shell and writer for generate_completion_to, shell for generate_completion), #
Returns (-> () ), and a # Example block (use no_run showing calling
generate_completion_to with a Vec<u8> writer and generate_completion with a
Shell variant), ensuring you reference the Shell type and Write bound in the
docs and keep the format consistent with other public APIs.

In `@src/connection.rs`:
- Around line 127-143: The tests test_create_database_connection and
test_create_database_connection_with_tls_options currently attempt real network
resolution against "nonexistent:3306", causing flaky DNS/timeouts; change them
to induce a deterministic parse/validation failure or move them to integration
tests. Specifically, update the test inputs used by create_database_connection
to a syntactically invalid DSN (e.g., a malformed URI) or otherwise trigger the
function's validation path so the call returns Err immediately instead of
attempting a network connect, or mark these as integration tests and remove them
from unit tests. Ensure the tests still assert result.is_err() and keep
references to TlsConfig and create_database_connection intact.

In `@src/delimited.rs`:
- Around line 54-65: The write and flush calls on the CSV writer lose context
because they use bare `?`; update the `wtr.write_record(row)?;` and
`wtr.flush()?;` calls to map errors into richer messages (e.g., include the
record index or contents and the operation) using `.map_err(...)` before the `?`
so failures from `wtr.write_record` and `wtr.flush` surface as contextualized
errors; target the `wtr` usage created via `WriterBuilder::new()` (variable
`wtr`) and ensure the returned error type matches the function's error
(wrap/convert as needed).

In `@src/logging.rs`:
- Around line 155-179: warn_banner and danger_banner double-allocate when colors
are supported because they call if_supports_color with a closure that returns a
String and then call .to_string() again; since you already check
stderr_supports_color(), remove the if_supports_color call and apply the style
directly to message (e.g., use message.yellow().to_string() for warn_banner and
message.red().bold().to_string() for danger_banner) and keep the else branch
returning message.to_owned(), referencing the warn_banner, danger_banner,
stderr_supports_color, and if_supports_color symbols to locate the code.

In `@src/tls/ca.rs`:
- Around line 123-161: The TOCTOU comes from checking ca_file_path.exists()
before File::open in load_ca_certificates; remove the exists() check and instead
attempt to open the file directly, mapping File::open errors into the
appropriate TlsError (e.g., map io::ErrorKind::NotFound to
TlsError::ca_file_not_found(ca_file_path.display().to_string()) and other I/O
errors to TlsError::invalid_ca_format with the error string) so behavior aligns
with CaFile::load and eliminates the race window.

In `@tests/additional_mysql_types.rs`:
- Around line 177-187: The test extended_types_null_paths_converge calls
TypeTransformer::value_to_string(&v).unwrap(); replace the .unwrap() with
.expect("value_to_string") to match the pattern used in other NULL tests (e.g.,
decimal_null_yields_empty_and_null), ensuring consistent failure messages;
update only the call in the test to use .expect with that string.
- Around line 116-154: Add a MySQL TIME maximum-boundary case to the cases array
in the time_boundaries_stable test: insert a tuple using Value::Time for the max
positive TIME (hours aggregated from days+h to 838:59:59) — e.g. (false, 34, 22,
59, 59, 0, "838:59:59.000000") — and optionally add the negative counterpart
(true, 34, 22, 59, 59, 0, "-838:59:59.000000"); then run the assertions that
call TypeTransformer::value_to_string and TypeTransformer::value_to_json to
ensure the transformer emits the canonical string for the extreme boundary.

In `@tests/credential_leak_sweep.rs`:
- Around line 46-53: LEAK_MARKERS currently lists common leak patterns but
misses "auth=" and "bearer="; update the LEAK_MARKERS constant to include
"auth=" and "bearer=" (e.g., add "auth=" and "bearer=" to the &[&str] array) so
tests detect those common credential patterns; ensure casing or trimming
behavior is consistent with existing entries in LEAK_MARKERS.

In `@tests/path_safety.rs`:
- Around line 150-202: The test function query_file_over_size_cap_rejected
declares a redundant variable size that is never passed into the assert!
message; either delete the unused size binding or actually use it in the
assertion by adding it to the format arguments and updating the format
placeholder (e.g., change "size_target={size}" to "{}" and append size as an
argument, or include size with a named format argument), ensuring the variable
name matches the placeholder or is removed entirely to avoid the unused-binding
warning.

In `@tests/stream_separation.rs`:
- Around line 122-137: The test connection_error_routes_to_stderr_only currently
only checks exit code and empty stdout but doesn't assert anything about stderr;
update the assertion chain on fresh_cmd() to also assert that stderr is
non-empty and contains the connection error diagnostic (e.g., use a predicate
like predicate::str::contains("connect") or "connection" or a suitable error
phrase) so the test fails if the diagnostic disappears; locate the assertion in
connection_error_routes_to_stderr_only and add the stderr(...) check after the
existing stdout(...) check.

In `@tests/test_support/cli.rs`:
- Around line 24-47: Add the required standardized doc sections to the two new
public helpers clean_cmd and clean_cmd_no_color: update each /// comment to
include an Arguments section (if none, state "None"), a Returns section
describing the returned assert_cmd::Command, and an Example section showing
minimal usage (example snippet invoking the function and running/asserting the
command); keep the existing descriptive text and ensure markdown formatting
(headings like "Arguments", "Returns", "Example") and triple-slash comments are
used for both functions.
- Around line 181-184: The test setup currently builds a Command inline and
manually removes ENV_VARS_TO_REMOVE; instead, refactor execute() to call the
existing clean_cmd() helper so command hygiene is centralized—locate the code
creating cmd in execute() and replace the manual env_remove loop with a call to
clean_cmd() (or have execute() delegate to clean_cmd() to obtain the sanitized
Command), ensuring you reference clean_cmd() and execute() so all env_scrub
logic lives in one place.

---

Outside diff comments:
In `@tests/integration/common.rs`:
- Around line 1-2015: The file is too large (2015 lines); split it into focused
modules to improve maintainability by extracting related types and functions:
create cli.rs and move GoldDiggerCli (and its impls: execute,
execute_with_assertions, execute_with_timeout, execute_raw,
execute_with_output_validation, execute_with_snapshot, redact_sensitive_output)
and AssertCmdIntegration into a cli module; create parsing.rs and move
OutputParser, CsvParseResult/JsonParseResult/TsvParseResult, and OutputValidator
+ related predicate structs (CsvContentPredicates, JsonContentPredicates,
TsvContentPredicates) into a parsing module; create temp.rs and move
TempFileManager, TestIsolation, TestEnvironment, TempFileManager methods and
TempFileManager Drop into a temp module; create predicates.rs and move
CliPredicates and related predicate helpers into it; update the original
tests/integration/common.rs to declare mod cli; mod parsing; mod temp; mod
predicates; re-export the moved symbols (pub use crate::cli::GoldDiggerCli;
etc.) and update use paths inside moved code to reference the new module
boundaries (e.g., references to OutputParser in cli -> parsing::OutputParser or
re-exported names), ensure ENV_VARS_TO_REMOVE and
TestCase/OutputFormat/GoldDiggerResult remain accessible (keep them in common.rs
or a small shared mod) and run cargo check to fix any import visibility issues.

In `@tests/integration/tls_tests.rs`:
- Around line 40-47: The helper skip_if_no_docker currently calls
std::process::exit(0), which aborts the whole test run; change skip_if_no_docker
to return a bool (e.g., fn skip_if_no_docker() -> bool) or Result to indicate
skipping instead of exiting, remove the std::process::exit(0) call, and update
callers (like test_platform_certificate_store) to check skip_if_no_docker() and
return early (e.g., if skip_if_no_docker() { return Ok(()); }) so only the
individual test is skipped and the test framework continues reporting other
tests.
- Around line 639-754: The tests in the ephemeral_certificate_tests module
reference CertificateValidator, CertificateLoader, and EphemeralCertificate but
do not import them, causing "cannot find type" compile errors; add imports for
these symbols from the fixtures tls module (e.g., import CertificateValidator,
CertificateLoader, EphemeralCertificate from crate::fixtures::tls or equivalent)
at the top of tests/integration/tls_tests.rs so the submodule can resolve those
types referenced in test_ephemeral_certificate_generation,
test_certificate_loading_utilities, and test_certificate_validation_utilities.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 89bb672f-fcbd-4d16-a2de-9da6b5746907

📥 Commits

Reviewing files that changed from the base of the PR and between ffd149a and 389996b.

⛔ Files ignored due to path filters (61)

• .cargo/audit.toml is excluded by none and included by none
• .gitattributes is excluded by none and included by none
• .github/workflows/release.yml is excluded by !.github/workflows/release.yml and included by .github/**
• Cargo.lock is excluded by !**/*.lock and included by none
• benches/README.md is excluded by none and included by none
• benches/memory_ceiling.rs is excluded by none and included by none
• benches/memory_usage.rs is excluded by none and included by none
• benches/output_formats.rs is excluded by none and included by none
• docs/solutions/best-practices/drop-safe-atomic-file-output-rust-cli-2026-04-25.md is excluded by none and included by none
• docs/solutions/best-practices/front-load-validation-with-resolved-config-type-2026-04-25.md is excluded by none and included by none
• docs/solutions/best-practices/replace-substring-error-classifiers-with-typed-enums-2026-04-25.md is excluded by none and included by none
• docs/solutions/best-practices/unified-credential-redaction-rust-binary-2026-04-25.md is excluded by none and included by none
• mise.lock is excluded by !**/*.lock and included by none
• tests/snapshots/cli_testing_example__help_output.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/cli_testing_example__verbose_quiet_mutex_error.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/csv_empty_result_set.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/csv_escaping_quotes_and_commas.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/csv_newlines.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/csv_null_values.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/csv_standard_data.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/json_empty_result_set.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/json_large_integers.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/json_null_handling.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/json_pretty_printed.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/json_standard_data.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tls_backward_compatibility__integration_compatibility_tests__cli_help_output.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tls_cli_integration__tls_cli_flag_tests__invalid_ca_file_content_error.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tls_cli_integration__tls_cli_flag_tests__nonexistent_ca_file_error.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tls_cli_integration__tls_cli_flag_tests__tls_help_options.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tls_danger_banner_snapshot__allow_invalid_certificate_danger_banner.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tsv_null_conversion.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tsv_special_characters.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/tsv_standard_data.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_bytes_valid_utf8.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_date.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_datetime_with_microseconds.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_double.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_float_infinity.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_float_nan.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_int.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_invalid_date_error.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_large_binary.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_null.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_time.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_json_uint_max.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_date.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_datetime.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_double.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_float.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_infinity.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_int.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_invalid_utf8.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_large_binary.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_nan.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_neg_infinity.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_null.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_time.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_time_with_days.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_uint.snap is excluded by !**/*.snap and included by tests/**
• tests/snapshots/value_to_string_utf8_bytes.snap is excluded by !**/*.snap and included by tests/**
• tests/tests/snapshots/json_null_handling.snap is excluded by !**/*.snap and included by tests/**

📒 Files selected for processing (83)

• .cursor/rules/rust-best-practices.mdc
• .github/ci-performance-config.yml
• .github/dependabot.yml
• .github/error-reporting-config.yml
• .github/workflows/benchmarks.yml
• .github/workflows/cargo-audit-pr.yml
• .github/workflows/ci.yml
• .github/workflows/codeql.yml
• .github/workflows/copilot-setup-steps.yml
• .github/workflows/coverage.yml
• .github/workflows/scorecard.yml
• .github/workflows/security.yml
• .github/zizmor.yml
• AGENTS.md
• CHANGELOG.md
• CONTRIBUTING.md
• Cargo.toml
• README.md
• RELEASING.md
• SECURITY.md
• deny.toml
• dist-workspace.toml
• docs/src/development/api-reference.md
• docs/src/development/architecture.md
• docs/src/troubleshooting/type-errors.md
• docs/src/usage/configuration.md
• justfile
• mise.toml
• src/cli.rs
• src/completion.rs
• src/config.rs
• src/connection.rs
• src/csv.rs
• src/delimited.rs
• src/exit.rs
• src/json.rs
• src/lib.rs
• src/logging.rs
• src/main.rs
• src/mysql_errors.rs
• src/output.rs
• src/run.rs
• src/sink.rs
• src/tab.rs
• src/tls.rs
• src/tls/ca.rs
• src/tls/classifier.rs
• src/tls/config.rs
• src/tls/error.rs
• src/tls/mod.rs
• src/tls/pool.rs
• src/type_transformer.rs
• src/utils.rs
• tessl.json
• tests/additional_mysql_types.rs
• tests/atomic_output.rs
• tests/cli_testing_example.rs
• tests/credential_leak_regression.rs
• tests/credential_leak_sweep.rs
• tests/dump_config_redaction.rs
• tests/exit_code_1_no_rows.rs
• tests/exit_code_3_auth.rs
• tests/exit_code_4_query.rs
• tests/exit_code_5_io.rs
• tests/exit_codes.rs
• tests/init_crypto_provider_idempotency.rs
• tests/integration/common.rs
• tests/integration/tls_tests.rs
• tests/lib.rs
• tests/output_format_snapshots.rs
• tests/path_safety.rs
• tests/redact_sql_error_truth_table.rs
• tests/stream_separation.rs
• tests/test_support/cli.rs
• tests/tls_backward_compatibility.rs
• tests/tls_classifier_rcgen.rs
• tests/tls_cli_integration.rs
• tests/tls_config_unit_tests.rs
• tests/tls_danger_banner_snapshot.rs
• tests/tls_integration_old.rs
• tests/tls_variants_test.rs
• tests/type_safety.rs
• tests/type_transformer_snapshots.rs

💤 Files with no reviewable changes (3)

• .github/error-reporting-config.yml
• .github/ci-performance-config.yml
• src/tls.rs

[Copilot]

Pull request overview

Copilot reviewed 96 out of 143 changed files in this pull request and generated 7 comments.

[coderabbitai]

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/zizmor.yml:
- Around line 9-24: Update the .github/zizmor.yml suppressions so only
release.yml remains under unpinned-uses.ignore and remove docs.yml, ci.yml, and
security.yml from that list (they are SHA-pinned); also remove ci.yml from the
excessive-permissions suppression because its permissions are contents: read (no
write) and therefore should be evaluated normally; finally, adjust the
surrounding reason comment to state that only release.yml is intentionally
pinned by tag (v6.0.2) while the others are SHA-pinned and thus not suppressed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5fab9b9e-4901-4697-9dd3-bc43f8c62415

📥 Commits

Reviewing files that changed from the base of the PR and between 389996b and 5add972.

📒 Files selected for processing (1)

• .github/zizmor.yml

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

workflows=(.github/workflows/release.yml .github/workflows/ci.yml .github/workflows/docs.yml .github/workflows/security.yml)

echo "== uses refs classification =="
for wf in "${workflows[@]}"; do
  echo "-- $wf --"
  rg -n '^\s*-\s*uses:\s*[^[:space:]]+@[^[:space:]]+' "$wf" | while IFS=: read -r ln txt; do
    ref="$(sed -E 's/.*@([^[:space:]#]+).*/\1/' <<<"$txt")"
    if [[ "$ref" =~ ^[0-9a-fA-F]{40}$ ]]; then kind="sha-pinned"; else kind="non-sha"; fi
    echo "Line $ln [$kind] $ref"
  done
done

echo
echo "== permissions blocks (root + jobs) =="
for wf in .github/workflows/ci.yml .github/workflows/docs.yml .github/workflows/release.yml; do
  echo "-- $wf --"
  rg -n -C2 '^\s*permissions:' "$wf"
done

Repository: EvilBit-Labs/gold_digger

Length of output: 1986

---

Remove SHA-pinned workflows from unpinned-uses suppressions and evaluate ci.yml in excessive-permissions.

ci.yml, docs.yml, and security.yml are all SHA-pinned, so they should not appear in unpinned-uses.ignore. Additionally, ci.yml has only contents: read permissions (no write scope), so its suppression under excessive-permissions should be removed.

Only release.yml should remain in unpinned-uses.ignore—it uses tag pins (v6.0.2) rather than SHAs. Update the reason comment to reflect this.

Proposed changes

 rules:
   excessive-permissions:
     ignore:
       # reason: release.yml needs `contents: write` for tags/artifacts;
-      # docs.yml needs `pages: write` to publish mdBook; ci.yml's broad
-      # token surface is intentional for the matrix jobs.
+      # docs.yml needs `pages: write` to publish mdBook.
       - release.yml
       - docs.yml
-      - ci.yml
   unpinned-uses:
     ignore:
-      # reason: third-party action versions are pinned by tag rather than
-      # SHA in these workflows; tracked separately in todo `#013` (P1) for
-      # the release pipeline. The list intentionally drops the historical
-      # cargo-dist.yml entry — that workflow no longer exists (todo `#113`).
+      # reason: release.yml is still tag-pinned; migrate to full SHAs in
+      # todo `#013` (P1). cargo-dist.yml no longer exists (todo `#113`).
       - release.yml
-      - ci.yml
-      - docs.yml
-      - security.yml

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# reason: release.yml needs `contents: write` for tags/artifacts;
	# docs.yml needs `pages: write` to publish mdBook; ci.yml's broad
	# token surface is intentional for the matrix jobs.
	- release.yml
	- docs.yml
	- ci.yml
	unpinned-uses:
	ignore:
	# reason: third-party action versions are pinned by tag rather than
	# SHA in these workflows; tracked separately in todo #013 (P1) for
	# the release pipeline. The list intentionally drops the historical
	# cargo-dist.yml entry — that workflow no longer exists (todo #113).
	- release.yml
	- cargo-dist.yml
	- ci.yml
	- codeql.yml
	- docs.yml
	- security.yml
	# reason: release.yml needs `contents: write` for tags/artifacts;
	# docs.yml needs `pages: write` to publish mdBook.
	- release.yml
	- docs.yml
	unpinned-uses:
	ignore:
	# reason: release.yml is still tag-pinned; migrate to full SHAs in
	# todo `#013` (P1). cargo-dist.yml no longer exists (todo `#113`).
	- release.yml

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/zizmor.yml around lines 9 - 24, Update the .github/zizmor.yml
suppressions so only release.yml remains under unpinned-uses.ignore and remove
docs.yml, ci.yml, and security.yml from that list (they are SHA-pinned); also
remove ci.yml from the excessive-permissions suppression because its permissions
are contents: read (no write) and therefore should be evaluated normally;
finally, adjust the surrounding reason comment to state that only release.yml is
intentionally pinned by tag (v6.0.2) while the others are SHA-pinned and thus
not suppressed.

[coderabbitai]

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 6

♻️ Duplicate comments (1)

.github/workflows/security.yml (1)

5-5: ⚠️ Potential issue | 🟡 Minor

Fix inline-array spacing on Line 5 to unblock YAMLlint.

workflows: [ CI ] still violates the bracket-spacing rule; use workflows: [CI].

Suggested fix

-        workflows: [ CI ]
+        workflows: [CI]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/security.yml at line 5, The inline array spacing in the
YAML key "workflows" is violating YAMLlint's bracket-spacing rule; update the
value used in the workflow declaration from "workflows: [ CI ]" to remove the
spaces inside the brackets (i.e., "workflows: [CI]") so the inline array
conforms to the linter rule.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/coverage.yml:
- Around line 10-12: The workflow currently only sets permissions: contents:
read but still relies on the CODECOV_TOKEN secret; update the permissions block
to include id-token: write (e.g., add "id-token: write" alongside contents:
read), change the Codecov action invocation to use OIDC by setting use_oidc:
true and remove the CODECOV_TOKEN secret input (or guard its removal by
switching to the OIDC flow in the codecov action call), and ensure any
references to CODECOV_TOKEN in the job steps are deleted so the action
authenticates via OIDC instead of the long‑lived secret.

In `@src/logging.rs`:
- Around line 56-84: Add the missing documentation sections for the new public
logging helpers: init_tracing, stderr_supports_color, warn_banner,
danger_banner, and make_progress. For each function add /// Arguments describing
parameters, /// Returns describing the return value (or unit), and an ///
Example showing minimal usage (copyable snippet) consistent with existing crate
style; keep wording concise and include any side-effects (e.g., global
subscriber installation in init_tracing or terminal output for banners). Ensure
the doc comments are placed immediately above each pub fn and follow the
project's three-section requirement (Arguments, Returns, Example).

In `@src/tls/ca.rs`:
- Around line 8-12: Imports in src/tls/ca.rs are misordered; reorder them to
follow the coding guideline: list std imports first (e.g., std::fs::File,
std::io::BufReader, std::path::{Path, PathBuf}), then external crate imports
(e.g., rustls_pki_types::{CertificateDer, pem::PemObject}), and finally local
module imports (e.g., use super::error::TlsError), with a blank line between
each group to match the project's import ordering convention.

In `@tests/credential_leak_sweep.rs`:
- Around line 163-166: Replace the hardcoded host-dependent missing CA path
"/nonexistent/path/to/ca.pem" used in the .args([...]) call in
tests/credential_leak_sweep.rs with a deterministic tempdir-relative path (e.g.,
create a TempDir via tempfile::tempdir() or use std::env::temp_dir() and join a
non-existent filename) and pass that joined path to the "--tls-ca-file" argument
so the test fails for a missing file consistently across platforms
(Windows/Linux/macOS).

In `@tests/exit_code_3_auth.rs`:
- Around line 28-51: The test constructs the Command by manually removing env
vars (cargo::cargo_bin_cmd!("gold_digger") with multiple .env_remove calls),
which duplicates scrub logic and can miss vars like RUST_LOG honored by
src/logging.rs::init_tracing; replace this manual setup with the shared helper
tests/test_support/cli.rs::clean_cmd() so the test calls
clean_cmd(cargo::cargo_bin_cmd!("gold_digger")) (or the equivalent pattern used
elsewhere) and remove the explicit .env_remove calls to ensure all scrubbed envs
(including RUST_LOG) are consistently applied and keep exit-code assertions
unchanged.

In `@tests/test_support/cli.rs`:
- Around line 16-23: The tests currently fail to scrub RUST_LOG from spawned
test processes; update the shared test helper so clean_cmd() also removes
"RUST_LOG" from the environment (i.e., add "RUST_LOG" to the ENV_VARS_TO_REMOVE
constant and ensure the code in clean_cmd() iterates that list and calls
.env_remove(...) for each entry) so parent-shell RUST_LOG won't leak into test
subprocesses or logging.rs behavior.

---

Duplicate comments:
In @.github/workflows/security.yml:
- Line 5: The inline array spacing in the YAML key "workflows" is violating
YAMLlint's bracket-spacing rule; update the value used in the workflow
declaration from "workflows: [ CI ]" to remove the spaces inside the brackets
(i.e., "workflows: [CI]") so the inline array conforms to the linter rule.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6ff5f849-d075-4ba8-9015-660922455ea3

📥 Commits

Reviewing files that changed from the base of the PR and between 5add972 and 72ba337.

📒 Files selected for processing (28)

• .github/dependabot.yml
• .github/workflows/benchmarks.yml
• .github/workflows/cargo-audit-pr.yml
• .github/workflows/ci.yml
• .github/workflows/coverage.yml
• .github/workflows/scorecard.yml
• .github/workflows/security.yml
• AGENTS.md
• Cargo.toml
• src/completion.rs
• src/connection.rs
• src/delimited.rs
• src/logging.rs
• src/tls/ca.rs
• tests/additional_mysql_types.rs
• tests/credential_leak_sweep.rs
• tests/database_seeding_test.rs
• tests/exit_code_3_auth.rs
• tests/exit_code_5_io.rs
• tests/integration/common.rs
• tests/path_safety.rs
• tests/stream_separation.rs
• tests/test_execution_utilities.rs
• tests/test_seed_data.rs
• tests/test_support/cli.rs
• tests/tls_classifier_rcgen.rs
• tests/tls_integration.rs
• tests/tls_variants_test.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For codecov/codecov-action v6, what are the exact workflow permission and input requirements for OIDC uploads (use_oidc: true), and are there any private-repo prerequisites?

💡 Result:

For codecov/codecov-action v6 with use_oidc: true: Workflow permissions required: permissions: id-token: write Input requirements: - uses: codecov/codecov-action@v6 with: use_oidc: true Any token provided is ignored; the action fetches the OIDC token using core.getIDToken('https://codecov.io') (audience defaults to Codecov's URL). Private repo prerequisites: None explicitly stated beyond the above. OIDC works equivalently to tokens for private repos (authenticates uploads). For PRs from forks, OIDC fails (no id-token access, similar to secrets), falling back to tokenless (may require Codecov settings for private repos, but OIDC itself has no extra private-repo setup like audience config in sources).

Citations:

• 1: https://github.com/codecov/codecov-action/tree/v6.0.0
• 2: https://github.com/codecov/codecov-action
• 3: https://github.com/marketplace/actions/codecov
• 4: how to enable openid connect with codecov and github actions? codecov/codecov-action#1489
• 5: codecov/codecov-action@d820d60

---

Migrate Codecov auth from long-lived token to OIDC.

The workflow currently grants only contents: read (lines 10-12) but depends on CODECOV_TOKEN secret (line 65). Adopt OIDC authentication instead: add id-token: write permission and use use_oidc: true in the codecov action. This eliminates the static CI secret, reducing credential exposure surface.

Proposed fix

 permissions:
     contents: read
+    id-token: write
@@
             - name: Upload to Codecov
               if: github.event_name != 'pull_request' ||
                   github.event.pull_request.head.repo.full_name == github.repository
               uses: codecov/codecov-action@3f20e214133d0983f9a10f3d63b0faf9241a3daa # v6.0.0
               with:
                   files: lcov.info
                   fail_ci_if_error: true
-                  token: ${{ secrets.CODECOV_TOKEN }}
+                  use_oidc: true
                   slug: EvilBit-Labs/gold_digger

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/coverage.yml around lines 10 - 12, The workflow currently
only sets permissions: contents: read but still relies on the CODECOV_TOKEN
secret; update the permissions block to include id-token: write (e.g., add
"id-token: write" alongside contents: read), change the Codecov action
invocation to use OIDC by setting use_oidc: true and remove the CODECOV_TOKEN
secret input (or guard its removal by switching to the OIDC flow in the codecov
action call), and ensure any references to CODECOV_TOKEN in the job steps are
deleted so the action authenticates via OIDC instead of the long‑lived secret.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Complete the required doc sections on the new public logging helpers.

init_tracing, stderr_supports_color, warn_banner, danger_banner, and make_progress still do not consistently provide the required Arguments, Returns, and Example sections. Please finish those while these APIs are still new.

As per coding guidelines, "src/**/*.rs: Doc comments (///) required for all public functions with Arguments, Returns, and Example sections`."

Also applies to: 129-206

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/logging.rs` around lines 56 - 84, Add the missing documentation sections
for the new public logging helpers: init_tracing, stderr_supports_color,
warn_banner, danger_banner, and make_progress. For each function add ///
Arguments describing parameters, /// Returns describing the return value (or
unit), and an /// Example showing minimal usage (copyable snippet) consistent
with existing crate style; keep wording concise and include any side-effects
(e.g., global subscriber installation in init_tracing or terminal output for
banners). Ensure the doc comments are placed immediately above each pub fn and
follow the project's three-section requirement (Arguments, Returns, Example).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Import ordering differs from coding guidelines.

The guidelines specify grouping imports by: std, external crates, then local modules (separated by newlines). Current order is local → external → std.

♻️ Suggested reordering

-use super::error::TlsError;
-use rustls_pki_types::{CertificateDer, pem::PemObject};
 use std::fs::File;
 use std::io::BufReader;
 use std::path::{Path, PathBuf};
+
+use rustls_pki_types::{CertificateDer, pem::PemObject};
+
+use super::error::TlsError;

As per coding guidelines: "Group imports by standard library, external crates, and local modules, separated by newlines."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	use super::error::TlsError;
	use rustls_pki_types::{CertificateDer, pem::PemObject};
	use std::fs::File;
	use std::io::BufReader;
	use std::path::{Path, PathBuf};
	use std::fs::File;
	use std::io::BufReader;
	use std::path::{Path, PathBuf};
	use rustls_pki_types::{CertificateDer, pem::PemObject};
	use super::error::TlsError;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tls/ca.rs` around lines 8 - 12, Imports in src/tls/ca.rs are misordered;
reorder them to follow the coding guideline: list std imports first (e.g.,
std::fs::File, std::io::BufReader, std::path::{Path, PathBuf}), then external
crate imports (e.g., rustls_pki_types::{CertificateDer, pem::PemObject}), and
finally local module imports (e.g., use super::error::TlsError), with a blank
line between each group to match the project's import ordering convention.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use a tempdir-relative missing CA path here.

/nonexistent/path/to/ca.pem is host-dependent and awkward on Windows. A missing path under temp keeps the failure mode identical while making the test deterministic on every runner.

Suggested patch

     let temp = tempdir().expect("tempdir");
     let out = temp.path().join("out.json");
+    let missing_ca = temp.path().join("missing-ca.pem");
 
     let url = sentinel_url();
     let output = clean_cmd()
         .args([
             "--tls-ca-file",
-            "/nonexistent/path/to/ca.pem",
+            missing_ca.to_str().expect("utf-8 path"),
             "--db-url",
             &url,

As per coding guidelines, "tests/**: Review tests for: Cross-platform compatibility`."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/credential_leak_sweep.rs` around lines 163 - 166, Replace the hardcoded
host-dependent missing CA path "/nonexistent/path/to/ca.pem" used in the
.args([...]) call in tests/credential_leak_sweep.rs with a deterministic
tempdir-relative path (e.g., create a TempDir via tempfile::tempdir() or use
std::env::temp_dir() and join a non-existent filename) and pass that joined path
to the "--tls-ca-file" argument so the test fails for a missing file
consistently across platforms (Windows/Linux/macOS).

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Route these auth tests through clean_cmd() instead of re-listing env removals.

These blocks bypass the shared helper and hand-maintain the scrub list. That already risks drift with the current binary: src/logging.rs::init_tracing also honors RUST_LOG, so parent-shell logging can still bleed into these runs. Building both commands via tests/test_support/cli.rs::clean_cmd() keeps exit-code coverage deterministic and future-proof.

As per coding guidelines, "tests/**/*.rs: Use .env_remove(\"DATABASE_URL\") etc. on Command to prevent user shell env leakage in integration tests`."

Also applies to: 64-78

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/exit_code_3_auth.rs` around lines 28 - 51, The test constructs the
Command by manually removing env vars (cargo::cargo_bin_cmd!("gold_digger") with
multiple .env_remove calls), which duplicates scrub logic and can miss vars like
RUST_LOG honored by src/logging.rs::init_tracing; replace this manual setup with
the shared helper tests/test_support/cli.rs::clean_cmd() so the test calls
clean_cmd(cargo::cargo_bin_cmd!("gold_digger")) (or the equivalent pattern used
elsewhere) and remove the explicit .env_remove calls to ensure all scrubbed envs
(including RUST_LOG) are consistently applied and keep exit-code assertions
unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Scrub RUST_LOG in the shared test command helper.

clean_cmd() is now the single source of truth for test-process hygiene, but src/logging.rs also honors parent RUST_LOG. A developer shell with RUST_LOG=trace can therefore inject extra stderr into helper-based tests and destabilize snapshots/assertions even when the Clap env vars are cleared.

Suggested patch

-pub const ENV_VARS_TO_REMOVE: &[&str] =
-    &["DATABASE_URL", "DATABASE_QUERY", "OUTPUT_FILE", "NO_COLOR"];
+pub const ENV_VARS_TO_REMOVE: &[&str] = &[
+    "DATABASE_URL",
+    "DATABASE_QUERY",
+    "OUTPUT_FILE",
+    "NO_COLOR",
+    "RUST_LOG",
+];

As per coding guidelines, "tests/**/*.rs: Use .env_remove(\"DATABASE_URL\") etc. on Command to prevent user shell env leakage in integration tests`."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_support/cli.rs` around lines 16 - 23, The tests currently fail to
scrub RUST_LOG from spawned test processes; update the shared test helper so
clean_cmd() also removes "RUST_LOG" from the environment (i.e., add "RUST_LOG"
to the ENV_VARS_TO_REMOVE constant and ensure the code in clean_cmd() iterates
that list and calls .env_remove(...) for each entry) so parent-shell RUST_LOG
won't leak into test subprocesses or logging.rs behavior.