From 453724e48a523f72072f8991fc060843c4ae40fa Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 10:23:08 -0400 Subject: [PATCH 1/2] Patch audited dependencies and enforce platform feature and MSRV checks --- .githooks/pre-commit | 8 ++- .github/dependabot.yml | 14 +++++ .github/workflows/audit.yml | 26 +++++++++ .github/workflows/ci.yml | 70 +++++++++++++++++----- .github/workflows/release.yml | 31 +++++----- Cargo.lock | 12 ++-- Cargo.toml | 4 +- docs/code-review-2026-09-05.md | 103 +++++++++++++++++++++++++++++++++ 8 files changed, 228 insertions(+), 40 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/audit.yml create mode 100644 docs/code-review-2026-09-05.md diff --git a/.githooks/pre-commit b/.githooks/pre-commit index b2c0404..29ff76a 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,6 +1,8 @@ #!/usr/bin/env bash set -euo pipefail -cargo fmt -- --check -cargo clippy -- -D warnings -cargo test --verbose +cargo fmt --all -- --check +cargo clippy --locked --all-targets --all-features -- -D warnings +cargo clippy --locked --all-targets --no-default-features -- -D warnings +cargo test --locked --all-features +cargo test --locked --no-default-features diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..5e6f94c --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,14 @@ +version: 2 +updates: + - package-ecosystem: cargo + directory: / + schedule: + interval: weekly + open-pull-requests-limit: 5 + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + groups: + actions: + patterns: ['*'] diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml new file mode 100644 index 0000000..3353822 --- /dev/null +++ b/.github/workflows/audit.yml @@ -0,0 +1,26 @@ +name: Dependency audit + +on: + push: + branches: [main] + pull_request: + schedule: + - cron: '21 9 * * 1' + workflow_dispatch: + +permissions: + contents: read + +jobs: + audit: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + key: cargo-audit + - run: cargo install cargo-audit --version 0.22.2 --locked + - run: cargo audit --deny warnings diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f6cfd83..7cb32ca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,32 +2,70 @@ name: Rust CI on: push: - branches: [ "main" ] + branches: [main] pull_request: - branches: [ "main" ] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true env: CARGO_TERM_COLOR: always jobs: - test: + quality: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master with: + toolchain: stable components: rustfmt, clippy - - uses: Swatinem/rust-cache@v2 - + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + - run: cargo fmt --all -- --check + - run: cargo clippy --locked --all-targets --all-features -- -D warnings + - run: cargo clippy --locked --all-targets --no-default-features -- -D warnings - - name: Check formatting - run: cargo fmt --all -- --check - - - name: Lint code - run: cargo clippy --all-targets --all-features -- -D warnings + test: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + - run: cargo test --locked --all-features - - name: Run all-feature tests - run: cargo test --all-features --verbose + features: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + feature: ["", lang-c, lang-cpp, lang-java, lang-python, lang-go, lang-swift, lang-php, lang-objc, lang-rust, lang-javascript, lang-typescript] + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + key: feature-${{ matrix.feature }} + - run: cargo test --locked --no-default-features --features "${{ matrix.feature }}" - - name: Run minimal-feature tests - run: cargo test --no-default-features --verbose + msrv: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master + with: + toolchain: '1.88.0' + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + - run: cargo check --locked --all-targets --all-features diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f3e950f..c4c2c91 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,7 +6,7 @@ on: - "v*" permissions: - contents: write + contents: read jobs: build-binaries: @@ -32,13 +32,14 @@ jobs: ext: "" archive: tar.gz steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master with: + toolchain: stable targets: ${{ matrix.target }} - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: cargo build - run: cargo build --release --target ${{ matrix.target }} + run: cargo build --locked --release --target ${{ matrix.target }} - name: package shell: bash run: | @@ -53,31 +54,35 @@ jobs: tar -czf "${dist}/cipherscope-${{ github.ref_name }}-${{ matrix.target }}.tar.gz" -C "target/${{ matrix.target }}/release" "${bin}" fi - name: upload artifacts - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: cipherscope-${{ matrix.target }} path: dist/* if-no-files-found: error publish: + permissions: + contents: write needs: build-binaries runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@d1031067263f94b142dd6c0ce24c5eb9d02d52a0 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: cargo test - run: cargo test + run: cargo test --locked --all-features - name: cargo publish - run: cargo publish --allow-dirty + run: cargo publish --locked env: CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }} - name: download artifacts - uses: actions/download-artifact@v4 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: path: dist - name: GitHub Release - uses: softprops/action-gh-release@v2 + uses: softprops/action-gh-release@efb35369e0ad2afab669f228072c1b0d510eae64 # v3.0.3 with: files: | dist/**/*.tar.gz diff --git a/Cargo.lock b/Cargo.lock index 1923b57..4870e5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -92,9 +92,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.100" +version = "1.0.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a23eb6b1614318a8071c9b2521f36b424b2c83db5eb3a0fead4a6c0809af6e61" +checksum = "2a4385e2e34eb35d6b3efe798b9eb88096925d87726c0798709bf56d9ed84af3" [[package]] name = "assert_cmd" @@ -336,9 +336,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" dependencies = [ "crossbeam-utils", ] @@ -545,9 +545,9 @@ checksum = "f52b00d39961fc5b2736ea853c9cc86238e165017a493d1d5c8eac6bdc4cc273" [[package]] name = "memmap2" -version = "0.9.9" +version = "0.9.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "744133e4a0e0a658e1374cf3bf8e415c4052a15a111acd372764c55b4177d490" +checksum = "d1219ed1b7f229ee7104d281dd01d6802fe28bb6e95d292942c4daacdeb798c0" dependencies = [ "libc", ] diff --git a/Cargo.toml b/Cargo.toml index a4af229..e600c7f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,11 +23,11 @@ include = [ ] [dependencies] -anyhow = "1.0" +anyhow = "1.0.103" ahash = { version = "0.8", features = ["serde"] } clap = { version = "4.5", features = ["derive"] } ignore = "0.4" -memmap2 = "0.9" +memmap2 = "0.9.11" rayon = "1.10" regex = { version = "1.11", default-features = false, features = ["std", "unicode-perl"] } serde = { version = "1.0", features = ["derive"] } diff --git a/docs/code-review-2026-09-05.md b/docs/code-review-2026-09-05.md new file mode 100644 index 0000000..f7714f4 --- /dev/null +++ b/docs/code-review-2026-09-05.md @@ -0,0 +1,103 @@ +# Code review, 2026-09-05 + +Reviewed baseline: `a659f1c` (main, including the previous modernization PR). +Scope: scanner/library implementation, CLI discovery and output, pattern loading, +test coverage, manifests/lockfile, CI/release workflows, and benchmark structure. +This is a targeted engineering review, not an exhaustive cryptographic catalog +validation or a performance study. + +## Findings addressed by this series + +| Priority | Finding and evidence | Change | +| --- | --- | --- | +| P1 | `--roots input.rs --output input.rs` returned success after reducing the source to zero bytes. Output was opened before validating exclusion globs. | [#14](https://github.com/script3r/cipherscope/pull/14): protect inputs, validate before output creation, stage output until success. | +| P1 | A nonexistent root printed a walk error but exited with status 0. Worker read/parse errors were also only logged. | [#14](https://github.com/script3r/cipherscope/pull/14): propagate incomplete scans through the exit status; preserve previous file output. | +| P1 | File-backed mmap had no protection against concurrent truncation or writes, violating the assumptions needed for safe access. The size limit was checked only during discovery. | [#14](https://github.com/script3r/cipherscope/pull/14): bounded reads into owned memory, including a read-time size check. | +| P2 | An OpenSSL include after a copyright comment produced no library finding. A node-anchored regex was reused against the whole file. | [#15](https://github.com/script3r/cipherscope/pull/15): conservative hints with authoritative AST matching. | +| P2 | A commented-out `EVP_aes_256_gcm()` generated an AES-GCM finding; API-only library anchors and comments inside call nodes could also match. | [#15](https://github.com/script3r/cipherscope/pull/15): mask AST comments while preserving evidence offsets. | +| P2 | `languages = ["Pythno"]` silently disabled the rule; unknown fields and unsupported schema versions were ignored. | [#16](https://github.com/script3r/cipherscope/pull/16): strict schema validation and contextual regex diagnostics. | +| P2 | Directory scans dropped `source.PY` despite the resolver accepting it. TSX was routed to a grammar without JSX syntax. | [#17](https://github.com/script3r/cipherscope/pull/17): one extension resolver, dedicated TSX parsing, C-only fixture coverage. | +| P2 | The lockfile contained three dependencies covered by active RustSec advisories; CI did not audit them. | This PR: targeted patched versions and recurring dependency auditing. | +| P2 | CI tested only Linux, all features, and no features; the advertised MSRV and individual parsers were unchecked. Actions used mutable major tags and older runtimes. | This PR: platform/feature/MSRV coverage, current action versions pinned to commits, Dependabot, and matching pre-commit checks. | + +The mmap concern is supported by the library's +[documented safety contract](https://docs.rs/memmap2/latest/memmap2/struct.MmapOptions.html#safety). +Owned reads remove that memory-safety hazard but do not provide an atomic snapshot +of a repository while other processes edit it. + +## Dependency evidence + +`cargo-audit 0.22.2` checked 136 locked dependencies against RustSec database commit +`5a0ebedfe8bdd2e295b171f4162f8c977bcad9a5` (updated September 2, 2026). + +| Package | Baseline | Patched version | Advisory | +| --- | --- | --- | --- | +| crossbeam-epoch | 0.9.18 | 0.9.20 | [RUSTSEC-2026-0204](https://rustsec.org/advisories/RUSTSEC-2026-0204.html) | +| anyhow | 1.0.100 | 1.0.103 | [RUSTSEC-2026-0190](https://rustsec.org/advisories/RUSTSEC-2026-0190.html) | +| memmap2 | 0.9.9 | 0.9.11 | [RUSTSEC-2026-0186](https://rustsec.org/advisories/RUSTSEC-2026-0186.html) | + +The first is classified as a vulnerability; the other two are informational +unsoundness advisories. Reachability of the affected dependency functions from the +scanner has not been established. Source inspection found no direct calls to the +affected `anyhow` or `memmap2` APIs. The patched lockfile passes `cargo audit --deny +warnings`. PR #14 removes memmap2 entirely; this PR patches it so it can also be +reviewed and merged independently. + +## Remaining issues and follow-up acceptance criteria + +- **Constant resolution ignores scope (P2).** `collect_constants` builds one map + for the entire file, and textual substitution does not distinguish identifiers + from string contents. A Python function-local `KEY_SIZE = 256` can overwrite a + module-level `KEY_SIZE = 128` in the map, reporting 256 for the module-level + call. Replace this with scope-aware resolution, or decline ambiguous + substitutions; test shadowing, reassignment, string literals, and forward use. +- **Exclusions are relative to the process directory (P2).** + `OverrideBuilder::new(".")` does not anchor `vendor/**` to an external scan root. + Define per-root exclusion semantics and test absolute roots, multiple roots, + anchored globs, and nested dependency directories. +- **Repeated/overlapping roots repeat findings (P2).** The walker receives every + root and there is no cross-file identity deduplication. Choose and document + whether identity means canonical path or inode, accounting for explicitly + requested ignored paths and the memory cost of tracking every file. +- **Evidence and deduplication are coarse (P2).** API-only library findings always + use line 1, column 1. Algorithm hits are collapsed per line, generic algorithms + can be suppressed by separate calls on that line, and one parameterized hit can + suppress all parameterless hits for the algorithm in the file. Define callsite + identity and metadata precedence before changing this policy and its fixtures. +- **The catalog needs a separate semantic review (P2).** It contains duplicate + algorithm definitions and three parameter patterns without capture group 1: + RustCrypto/ChaCha20Poly1305 `keySize`, PyCA/Fernet `algorithm`, and + CommonCrypto (Objective-C)/AES `mode`. They cannot extract values under the + current implementation. `nistQuantumSecurityLevel` is parsed but never emitted. + Establish intended semantics against official library documentation before + changing these definitions. Do not automatically expose unvalidated security + classifications in output. +- **Coverage and performance limits (P3).** Ordinary strings/docstrings can still + trigger regex fallback. Imports/aliases and data flow are not fully resolved. + Kotlin and Erlang catalog entries have no parsers. Component benchmarks assume + certain parser features and are not reliable as a feature-isolation test suite. + Benchmark owned reads and comment masking on representative repositories before + making new throughput claims. + +No `patterns.toml` or fixture files were modified in this series. Existing fixture +agreement establishes compatibility; the added negative and boundary tests cover +defects that the original positive-heavy fixture suite did not exercise. + +## Validation and rollout + +Each code PR was checked with formatting and warnings-denied Clippy before commit. +All-feature and minimal-feature tests pass; the source-language PR also passes +C-only and TypeScript-only suites. Rust 1.88 all-target/all-feature compilation, +`actionlint`, and the patched dependency audit pass locally. Expanded CI exercises +the other platforms and parser combinations on GitHub. + +All five PRs target main. Review #14 first for its deliberate output behavior +change: it requires a writable destination directory, rejects source/symlink +destinations, and preserves previous file output when scanning fails. #17 adds a +public `Language::Tsx` variant. #16 rejects configuration previously ignored. +The workflow changes do not publish a release during this review; the live release +upload and crates.io publication path remains unexecuted. + +The I/O and dependency PRs both touch memmap2. When combining them, retain its +removal and retain the patched anyhow/crossbeam-epoch versions. Other changes are +intended to remain independently reviewable. From bd3bc24b596a72bbb2963a46f90ca5e9438c8063 Mon Sep 17 00:00:00 2001 From: Isaac Elbaz Date: Sat, 5 Sep 2026 10:45:41 -0400 Subject: [PATCH 2/2] Close temporary output handles before scanner subprocesses --- README.md | 2 ++ benches/file_size_bench.rs | 7 +++++-- benches/large_fixture_bench.rs | 7 +++++-- benches/memory_bench.rs | 21 +++++++++++++------ benches/scale_bench.rs | 7 +++++-- benches/scan_bench.rs | 7 +++++-- benches/scan_large_bench.rs | 7 +++++-- benches/thread_scaling_bench.rs | 7 +++++-- docs/code-review-2026-09-05.md | 5 +++++ tests/cli_io.rs | 21 +++++++++++++++++++ tests/integration.rs | 36 +++++++++++++++++++-------------- 11 files changed, 94 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index cbabc88..d4b2950 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,8 @@ diagnostics go to stderr. Stdout may contain partial findings on failure. With successful scan, preserving any previous inventory on failure. Output must be a regular file, and existing source files or the custom patterns file cannot be used as the destination. The destination directory must be writable. +On Windows, close open handles to an existing destination before scanning so it +can be replaced atomically. ## Installation diff --git a/benches/file_size_bench.rs b/benches/file_size_bench.rs index 6f38c54..cf08532 100644 --- a/benches/file_size_bench.rs +++ b/benches/file_size_bench.rs @@ -20,10 +20,13 @@ fn is_extended() -> bool { } fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/large_fixture_bench.rs b/benches/large_fixture_bench.rs index f91741f..66ae52b 100644 --- a/benches/large_fixture_bench.rs +++ b/benches/large_fixture_bench.rs @@ -21,10 +21,13 @@ fn is_extended() -> bool { } fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/memory_bench.rs b/benches/memory_bench.rs index 6dca665..81a7e16 100644 --- a/benches/memory_bench.rs +++ b/benches/memory_bench.rs @@ -26,14 +26,17 @@ fn run_scan_with_memory( threads: usize, patterns_path: &Path, ) -> (Duration, Option) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let start = Instant::now(); let result = Command::new("/usr/bin/time") .arg("-v") .arg(assert_cmd::cargo::cargo_bin!("cipherscope")) .arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") @@ -64,12 +67,15 @@ fn run_scan_with_memory( threads: usize, patterns_path: &Path, ) -> (Duration, Option) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let start = Instant::now(); let status = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")) .arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") @@ -85,10 +91,13 @@ fn run_scan_with_memory( } fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/scale_bench.rs b/benches/scale_bench.rs index 0b2e5ff..6d6d87c 100644 --- a/benches/scale_bench.rs +++ b/benches/scale_bench.rs @@ -20,10 +20,13 @@ fn is_extended() -> bool { } fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/scan_bench.rs b/benches/scan_bench.rs index 906a815..0c7a774 100644 --- a/benches/scan_bench.rs +++ b/benches/scan_bench.rs @@ -6,10 +6,13 @@ use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main}; use tempfile::NamedTempFile; fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/scan_large_bench.rs b/benches/scan_large_bench.rs index 7b10e01..db3b7cf 100644 --- a/benches/scan_large_bench.rs +++ b/benches/scan_large_bench.rs @@ -6,10 +6,13 @@ use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main}; use tempfile::NamedTempFile; fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/benches/thread_scaling_bench.rs b/benches/thread_scaling_bench.rs index 824da03..9023996 100644 --- a/benches/thread_scaling_bench.rs +++ b/benches/thread_scaling_bench.rs @@ -20,10 +20,13 @@ fn is_extended() -> bool { } fn run_scan(roots: &[PathBuf], threads: usize, patterns_path: &Path) { - let output = NamedTempFile::new().expect("create temp output file"); + // Close the destination handle so Windows can atomically replace the output. + let output = NamedTempFile::new() + .expect("create temp output file") + .into_temp_path(); let mut cmd = Command::new(assert_cmd::cargo::cargo_bin!("cipherscope")); cmd.arg("--output") - .arg(output.path()) + .arg(output.as_os_str()) .arg("--threads") .arg(threads.to_string()) .arg("--patterns") diff --git a/docs/code-review-2026-09-05.md b/docs/code-review-2026-09-05.md index c12cbba..1b9ac11 100644 --- a/docs/code-review-2026-09-05.md +++ b/docs/code-review-2026-09-05.md @@ -92,6 +92,11 @@ C-only and TypeScript-only suites. Rust 1.88 all-target/all-feature compilation, `actionlint`, and the patched dependency audit pass locally. Expanded CI exercises the other platforms and parser combinations on GitHub. +The combined Windows CI run exposed test and benchmark callers retaining open +temporary output handles during atomic replacement. They now retain `TempPath` +cleanup guards while closing the file handles before invoking the scanner. A +regression test also verifies successful replacement of an existing inventory. + PRs #14–#17 have merged into main. PR #14 changed output behavior: it requires a writable destination directory, rejects source/symlink destinations, and preserves previous file output when scanning fails. #17 adds a diff --git a/tests/cli_io.rs b/tests/cli_io.rs index 51ed931..da9e298 100644 --- a/tests/cli_io.rs +++ b/tests/cli_io.rs @@ -5,6 +5,27 @@ fn scanner() -> Command { Command::new(env!("CARGO_BIN_EXE_cipherscope")) } +#[test] +fn successful_scan_replaces_existing_output() { + let dir = TempDir::new().unwrap(); + let output_path = dir.path().join("inventory.jsonl"); + fs::write(&output_path, "previous inventory\n").unwrap(); + let output = scanner() + .arg("--roots") + .arg(dir.path()) + .arg("--output") + .arg(&output_path) + .output() + .unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(fs::read(output_path).unwrap().is_empty()); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); +} + #[test] fn missing_root_fails_and_preserves_existing_output() { let dir = TempDir::new().unwrap(); diff --git a/tests/integration.rs b/tests/integration.rs index 1aecaf4..31eb493 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -3,9 +3,15 @@ use std::{ path::{Path, PathBuf}, process::Command, }; -use tempfile::NamedTempFile; #[cfg(feature = "lang-rust")] use tempfile::TempDir; +use tempfile::{NamedTempFile, TempPath}; + +// Keep automatic cleanup without holding a destination handle open: Windows +// cannot atomically replace an output that the parent test still has open. +fn temporary_output() -> TempPath { + NamedTempFile::new().unwrap().into_temp_path() +} fn normalize_path_in_value(mut v: serde_json::Value) -> serde_json::Value { if let Some(obj) = v.as_object_mut() @@ -100,8 +106,8 @@ fn fixtures_match_ground_truth() { let src_dir = case_dir.join("src"); let expected_path = case_dir.join("expected.jsonl"); - let tmp_out = NamedTempFile::new().unwrap(); - let out_path = tmp_out.path().to_path_buf(); + let tmp_out = temporary_output(); + let out_path = tmp_out.to_path_buf(); let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) .current_dir(&repo_root) @@ -142,8 +148,8 @@ fn exclude_works() { let repo_root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); let fixtures_dir = repo_root.join("fixtures"); - let tmp_out = NamedTempFile::new().unwrap(); - let out_path = tmp_out.path().to_path_buf(); + let tmp_out = temporary_output(); + let out_path = tmp_out.to_path_buf(); // We'll scan the whole fixtures/go directory, but exclude the tink_aesgcm subdirectory let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) @@ -182,8 +188,8 @@ fn multiple_roots_work() { let repo_root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); let fixtures_dir = repo_root.join("fixtures"); - let tmp_out = NamedTempFile::new().unwrap(); - let out_path = tmp_out.path().to_path_buf(); + let tmp_out = temporary_output(); + let out_path = tmp_out.to_path_buf(); // We'll scan two directories and check for combined output let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) @@ -248,8 +254,8 @@ symbol_patterns = ["test_algo"] big_content.push_str(&"a".repeat(1024 * 1024)); fs::write(&big_path, big_content).unwrap(); - let tmp_out = NamedTempFile::new().unwrap(); - let out_path = tmp_out.path().to_path_buf(); + let tmp_out = temporary_output(); + let out_path = tmp_out.to_path_buf(); let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) .current_dir(&repo_root) @@ -395,7 +401,7 @@ symbol_patterns = ["test_algo"] ) .unwrap(); - let default_output = NamedTempFile::new().unwrap(); + let default_output = temporary_output(); let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) .args([ "--roots", @@ -403,14 +409,14 @@ symbol_patterns = ["test_algo"] "--patterns", patterns_path.to_str().unwrap(), "--output", - default_output.path().to_str().unwrap(), + default_output.to_str().unwrap(), ]) .status() .unwrap(); assert!(status.success()); - assert!(read_jsonl(default_output.path()).is_empty()); + assert!(read_jsonl(&default_output).is_empty()); - let unignored_output = NamedTempFile::new().unwrap(); + let unignored_output = temporary_output(); let status = Command::new(env!("CARGO_BIN_EXE_cipherscope")) .args([ "--roots", @@ -418,11 +424,11 @@ symbol_patterns = ["test_algo"] "--patterns", patterns_path.to_str().unwrap(), "--output", - unignored_output.path().to_str().unwrap(), + unignored_output.to_str().unwrap(), "--gitignore=false", ]) .status() .unwrap(); assert!(status.success()); - assert_eq!(read_jsonl(unignored_output.path()).len(), 2); + assert_eq!(read_jsonl(&unignored_output).len(), 2); }