refactor(config): rewrite update_config through the locked handle by sachiniyer · Pull Request #257 · usedetail/cli

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer · 2026-04-21T23:07:40Z

Summary

update_config opened a second File::create handle to truncate and write the config, which made file.unlock() act on a handle that was never locked. The real lock release happened via Drop at end of scope — functionally correct but confusing, and it matched the shape of the race described in bug_e79362bd / [Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229.
Rewind, truncate in place (set_len(0)), and write through the already-locked handle. file.unlock() now releases the lock that was actually acquired.

Why this is a refactor, not a bug fix

The linked bug claimed the shadowed handle drops early, releasing the flock mid-write. It does not — Rust shadowing keeps the old binding alive until end of scope. I verified with a subprocess flock -n test that the lock is still held after the shadow. The bug report's failing test most likely picked up the tiny artifact between function return (Drop releases the flock) and the main thread flipping its "in-update" flag to false, not a real mid-write unlock.

No behavior change; existing concurrent_update_config_does_not_corrupt still passes.

Test plan

cargo test --lib config::storage — 15 pass
cargo clippy -- -D warnings clean
cargo fmt --check clean

🤖 Generated with Claude Code

`update_config` opened a second `File::create` handle solely to truncate and write, which made `file.unlock()` act on a handle that was never locked. The real lock release happened implicitly via `Drop` at end of scope — functionally correct but confusing, and it made the code look like it contained the race described in bug_e79362bd (github #229). Rewind, truncate in place (`set_len(0)`), and write through the handle that already holds the flock. `file.unlock()` now releases the lock that was actually acquired. No behavior change; existing `concurrent_update_config_does_not_corrupt` still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cubic-dev-ai

No issues found across 1 file

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-04-21T23:09:15Z

🚩 save_config has the same pattern the PR fixes in update_config
The save_config function at src/config/storage.rs:65-73 uses File::create followed by lock_exclusive, which means data is written to the file after locking, but there's a window between File::create (which truncates the file) and lock_exclusive where another reader could see an empty/partial file. This is a pre-existing issue with a similar pattern to what the PR fixes in update_config. It's less critical here because save_config doesn't read-then-write (no TOCTOU), but a concurrent load_config (which doesn't acquire a lock) could read a truncated file during the window between File::create and write_all.
(Refers to lines 68-71)
Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-21T23:09:16Z

+ (&file).seek(SeekFrom::Start(0))?;
+ file.set_len(0)?;


📝 Info: Old code silently released the exclusive lock before writing
The old code let file = File::create(&path)?; at line 98 shadowed the file variable, causing the original locked file descriptor to be dropped. This released the exclusive lock before the write, creating a TOCTOU race window where another process could interleave. The new code correctly reuses the same handle via seek + set_len, maintaining the lock throughout the entire read-modify-write cycle. This is the core correctness improvement of the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.

## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>  Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sachiniyer

## Summary Expands `REVIEW.md` from a single rule (no `pub(super)`) into a comprehensive set of review guidelines extracted from the full commit history (259 commits). Each section documents a convention that has been repeatedly enforced through past PRs and/or the lint configuration in `Cargo.toml` / `lib.rs`. New sections cover: visibility, error handling, type safety & casts, imports & paths, generated code & OpenAPI, output format discipline, config file handling, concurrency, testing, CLI argument design, code organization, and commit messages. Documentation-only change — no code modified. ## Review & Testing Checklist for Human - [ ] **Accuracy of each convention**: These were inferred from commit history by an AI, not dictated by a human. Read through each bullet and verify it matches your actual intent — some rules may be stated too broadly or too narrowly (e.g., is `console::Term` truly the only acceptable output mechanism? Is `const fn` always preferred, or only in specific contexts?). - [ ] **Completeness**: Are there important conventions missing that should be documented? For example, dependency management policies, PR size expectations, or branch naming. - [ ] **Commit message section**: The final section prescribes conventional commits (`type(scope): description`). Confirm this is a convention you want enforced — the commit history shows mixed adherence (earlier commits don't follow it). ### Notes - The original `pub(super)` rule is preserved and expanded with a visibility-narrowing guideline derived from PRs #180 and #190. - Guidelines about config handling (atomic writes, comment preservation, locking) reflect the significant effort invested in PRs #131, #220, #257, and #266. - The lint-related sections mirror what's already enforced in `Cargo.toml` `[lints.clippy]` and `[lints.rust]` — making them explicit here helps reviewers who don't check the lint config. Link to Devin session: https://app.devin.ai/sessions/67611649d13b486dbe1ebf06c87acc47 Requested by: @sachiniyer  --- <a href="https://app.devin.ai/review/usedetail/cli/pull/297" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a>   --- ## Summary by cubic Expanded and slimmed `REVIEW.md` into a guide for human-judged conventions only; lint-enforced rules are omitted. Covers visibility choices, error context with `anyhow::Result`, strict `--format json`, optional `repo` arg with git-remote fallback, generated code via `progenitor` and `cargo xtask` updates to `openapi.json`/help, config writes via `update_config`, and conventional commits. - **Migration** - No migration needed; confirm the conventions match current expectations. Written for commit 74c602d. Summary will update on new commits. <a href="https://cubic.dev/pr/usedetail/cli/pull/297?utm_source=github">Review in cubic</a>  --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>

sachiniyer temporarily deployed to integration-tests April 21, 2026 23:07 — with GitHub Actions Inactive

cubic-dev-aiBot reviewed Apr 21, 2026
View reviewed changes

devin-ai-integrationBot reviewed Apr 21, 2026
View reviewed changes

sachiniyer merged commit 7f61a89 into mainApr 21, 2026
13 checks passed

sachiniyer deleted the siyer/clarify-update-config branch April 21, 2026 23:10

This was referenced Apr 21, 2026
[Detail Bug] CLI config updates can unlock mid-write, allowing concurrent reads of truncated/corrupted config #229
Closed
chore(release): bump version to 0.2.2 #265
Merged

devin-ai-integrationBot mentioned this pull request May 16, 2026
docs: expand REVIEW.md with conventions from commit history #297
Merged
3 tasks