[PR #388] chore: enforce workspace clippy lints #397

New issue

Open

opened 2026-10-29 14:22:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented 2026-10-29 14:22:58 +00:00

Owner

Copy link

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/388
Author: @yingdi-shan
Created: 10/16/2026
Status: 🔄 Open

Base: main ← Head: chore/clippy-workspace-lints

---

📝 Commits (2)

• 784d88c chore: enforce workspace clippy lints
• 1afc6fe chore: forbid expect in production code and trim noisy lints

📊 Changes

150 files changed (+1564 additions, -1202 deletions)

View changed files

📝 Cargo.toml (+47 -0)
📝 adev/Cargo.toml (+3 -0)
📝 adev/src/codegen.rs (+12 -17)
📝 adev/src/coverage.rs (+6 -8)
📝 adev/src/ensure_tool.rs (+5 -6)
📝 adev/src/util.rs (+1 -2)
📝 build.rs (+4 -5)
➕ clippy.toml (+6 -0)
📝 crates/aenv/Cargo.toml (+3 -0)
📝 crates/aenv/src/client/buildkit.rs (+2 -2)
📝 crates/aenv/src/commands/build.rs (+1 -1)
📝 crates/aenv/src/commands/codex/guest.rs (+6 -3)
📝 crates/aenv/src/commands/codex/project.rs (+1 -1)
📝 crates/aenv/src/commands/codex/runtime.rs (+28 -13)
📝 crates/aenv/src/commands/codex/socket.rs (+16 -9)
📝 crates/aenv/src/commands/completion.rs (+1 -1)
📝 crates/aenv/src/commands/connect.rs (+8 -4)
📝 crates/aenv/src/commands/pull.rs (+3 -3)
📝 crates/aenv/src/commands/upload.rs (+1 -1)
📝 crates/benchmarks/Cargo.toml (+3 -0)

...and 80 more files

📄 Description

What

Adds a [workspace.lints.clippy] table with 42 clippy lints, opts every handwritten crate into it, and fixes or explicitly suppresses every hit. Generated crates (agentenv_http_server, custom_extension_client, firecracker_client) stay opted out because codegen would overwrite their manifests. Because make clippy runs with -D warnings, the lints are enforced in CI.

How the lints were chosen:

• Restriction and nursery lints: about 50 candidates were measured against production code (libs and bins only, no tests or generated code). Those with 0–5 hits are enabled, except lints with false positives here or a known false-positive record (see below).
• Pedantic lints: about 20 cheap ones with 5 or fewer hits each, plus large_futures, unused_async, assigning_clones, ref_option, implicit_clone and cloned_instead_of_copied.
• too_many_lines: threshold 150, set in clippy.toml. The 16 existing long functions have #[allow], so only new long functions are rejected.
• unwrap_used and expect_used: production code may neither unwrap nor expect. allow-unwrap-in-tests and allow-expect-in-tests are set. Bench, example and integration-test helper files outside cfg(test) have a file-level allow.

Excluded after measurement because they produced false positives here or are nursery lints with known false positives: branches_sharing_code, collection_is_never_read, create_dir, duration_suboptimal_units, mutex_atomic, needless_pass_by_ref_mut, set_contains_or_insert, trait_duplication_in_bounds, unnecessary_wraps and unused_self.

Why

These lints catch real problems the default set misses:

• panics in production code (unwrap, expect, todo, unchecked time subtraction)
• large futures on the async stack
• async functions that never await
• needless clones
• lossy Arc<Vec<_>> patterns
• functions growing past a reviewable size

Replacing unwrap() with expect() only renames the panic, so expect_used is enforced too.

Related issue

None. Tooling and cleanup change.

Scope and non-goals

• In scope: Cargo.toml lint tables, clippy.toml, and the code changes needed to satisfy the lints, including test code, which the lints also cover.
• Not included: high-volume pedantic lints such as needless_pass_by_value (37 hits), cast_lossless (47), the cast_* family (200+), missing_errors_doc and must_use_candidate. panic and indexing_slicing are also not enabled.

Design and behavior changes

• Lock poisoning: std lock()/read()/write()/Condvar::wait/get_mut results recover the guard with unwrap_or_else(|poisoned| poisoned.into_inner()), matching existing code in resolver.rs and startup_pack.rs.
• Other production expects:
  • Most return errors through ? (fixed-size header parsing, the zstd contexts, the ublk and ublk-daemon binaries, the CLI commands).
  • The rest are restructured so the invariant needs no check (let … else, iterating pairs instead of re-looking up keys, deriving VM/TAP IPs in NetworkAddressPlan::from_config, keeping a pre-keyed HMAC, storing CommittedSnapshot directly in RunnableSnapshot).
• Panics that could really happen and now return errors:
  • IndexLBPT::new on an empty mapping.
  • pack_planner::stage_block with more than 65,536 objects.
  • Slot::new failures in the network manager, which also free the slot bit now.
• Remaining expects: 12 sites carry #[expect(clippy::expect_used, reason = …)]:
  • global singletons that require startup initialization (ConfigManager, UblkDeviceManager, NetworkManager)
  • the embedded deps_manifest.toml ×3, now each covered by a parse test
  • the envd bootstrap HTTP client
  • the host network-namespace fd
  • four helpers that serialize plain derived structs, whose bytes are hashed into P2P keys and index paths and so must keep their encoding
• Egress deny list: if the platform deny list cannot be derived, all egress is denied (fail closed) and an error is logged.
• Shutdown signals: if the server cannot install its Ctrl+C or SIGTERM handler, it logs an error and keeps running instead of panicking.
• unused_async: functions with no .await are no longer async, and callers are updated (see Compatibility).
• large_futures: futures over 16 KiB are boxed with Box::pin at their await sites.
• Cleanups kept although their lints were later dropped: functions that always returned Ok or Some now return the value directly (for example ArtifactCache::evict_lru, the setup/packages.rs helpers and two ublk-daemon handlers). Methods that never used self became associated functions. A few &mut parameters became &.
• Smaller fixes:
  • The 128 KiB digest buffers move from the stack to the heap.
  • The Firecracker stderr tail read is bounded with take().
  • The egress proxy reaps finished connections with HashMap::extract_if.
  • Arc<Vec<PathBuf>> becomes Arc<[PathBuf]>.
  • Two extension checks in deps/mod.rs and sandbox/envd.rs now use Path::extension() and compare case-insensitively.
• Removed: RegistryFsV2::new() and impl Default for RegistryFsV2, which had no callers and panicked on failure.

Compatibility and operations

• Public API or generated protocol: these Rust APIs inside the workspace changed. All workspace callers are updated. No HTTP, gRPC or OpenAPI changes, and no generated code was edited.
  • async fn → fn:
    • overlaybd::ImageService::{new, from_config_path, from_config_path_with_p2p_publish_url}
    • VolumeManager::{open_with_repository, open_with_repository_and_limits}
    • the ublk io_buffer constructors
  • Now return Result:
    • IndexLBPT::new
    • UserBuffer::uring_buf_idx / IOBuffer::uring_buf_idx (io::Result<u16>)
    • Orchestrator::with_in_memory_store
  • Removed: RegistryFsV2::new / Default.
• Configuration or defaults: none.
• Snapshot manifest, artifact layout, or storage format: none. P2P keys and commit-index paths keep their serde encoding.
• Upgrade and rollback: none needed; code-only change.
• Host requirements, permissions, ports, or dependencies: none. Needs Rust 1.88+ for HashMap::extract_if; the toolchain is stable.

Validation

• make fmt
• make clippy
• make test-unit
• Relevant Rust integration tests
• make -C services test (required when services/ changes)
• Generated clients/server regenerated with the documented make target
• Documentation updated
• Benchmarks or performance comparison completed

Commands and results:

make fmt                                                                ok
make clippy                                                             ok
make test-unit                                                          ok (989 + 114 + 49 + 18 + 4 + 3 passed, 0 failed)
cargo test -p overlaybd -p storage-util -p adev -p warm-pool \
  -p shell-util -p object-store-operator --lib                          ok (387 + 13 + 12 + 7 + 5 passed)
cargo test -p uvm-ublk-daemon --test ublk_daemon_test                   ok (37 passed)

Skipped checks and reasons:

• KVM integration suites (tests/integration/*, make test-agent-integration): not run. The changes there are signature-only or test-helper changes, and they compile under clippy.
• services/: unchanged.
• Generated code: untouched.
• Benchmarks: not run; they compile under clippy.

Risks and reviewer notes

• Poison recovery: a panic while holding a std lock no longer cascades into every later locker; the guard is recovered instead. This matches the existing resolver and startup-pack code.
• Egress deny list: NetworkAddressPlan derivation failing now denies all egress instead of panicking. Startup already validates the same config, so this should not happen in practice.
• Server signal handlers: a failed handler install now logs an error and continues, so that signal no longer triggers graceful shutdown.
• Async to sync: the former async fns did no awaiting, so whatever blocking they did already happened on the executor thread; making them sync changes nothing at runtime.
• Box::pin: adds one heap allocation per affected call, all on control-plane or test paths.
• The remaining files: mostly mechanical. clippy --fix output was reviewed, and the poor rewrites were changed back.

Checklist

• The PR contains one coherent change and no unrelated formatting or refactoring.
• New behavior is covered by tests, or I explained why testing is impractical.
• Logs and examples contain no credentials, tokens, or private registry information.
• I did not manually edit generated code without updating its source and regenerating it.

🤖 Generated with Claude Code

---

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/388 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/16/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `chore/clippy-workspace-lints` --- ### 📝 Commits (2) - [`784d88c`](https://github.com/kvcache-ai/AgentENV/commit/784d88ce6a10045bb0e1b42714b61684fd7b996d) chore: enforce workspace clippy lints - [`1afc6fe`](https://github.com/kvcache-ai/AgentENV/commit/1afc6fedcb7a2d7aafdf9a7c8c0af8589cbbbf79) chore: forbid expect in production code and trim noisy lints ### 📊 Changes **150 files changed** (+1564 additions, -1202 deletions) <details> <summary>View changed files</summary> 📝 `Cargo.toml` (+47 -0) 📝 `adev/Cargo.toml` (+3 -0) 📝 `adev/src/codegen.rs` (+12 -17) 📝 `adev/src/coverage.rs` (+6 -8) 📝 `adev/src/ensure_tool.rs` (+5 -6) 📝 `adev/src/util.rs` (+1 -2) 📝 `build.rs` (+4 -5) ➕ `clippy.toml` (+6 -0) 📝 `crates/aenv/Cargo.toml` (+3 -0) 📝 `crates/aenv/src/client/buildkit.rs` (+2 -2) 📝 `crates/aenv/src/commands/build.rs` (+1 -1) 📝 `crates/aenv/src/commands/codex/guest.rs` (+6 -3) 📝 `crates/aenv/src/commands/codex/project.rs` (+1 -1) 📝 `crates/aenv/src/commands/codex/runtime.rs` (+28 -13) 📝 `crates/aenv/src/commands/codex/socket.rs` (+16 -9) 📝 `crates/aenv/src/commands/completion.rs` (+1 -1) 📝 `crates/aenv/src/commands/connect.rs` (+8 -4) 📝 `crates/aenv/src/commands/pull.rs` (+3 -3) 📝 `crates/aenv/src/commands/upload.rs` (+1 -1) 📝 `crates/benchmarks/Cargo.toml` (+3 -0) _...and 80 more files_ </details> ### 📄 Description ## What Adds a `[workspace.lints.clippy]` table with 42 clippy lints, opts every handwritten crate into it, and fixes or explicitly suppresses every hit. Generated crates (`agentenv_http_server`, `custom_extension_client`, `firecracker_client`) stay opted out because codegen would overwrite their manifests. Because `make clippy` runs with `-D warnings`, the lints are enforced in CI. How the lints were chosen: - **Restriction and nursery lints:** about 50 candidates were measured against production code (libs and bins only, no tests or generated code). Those with 0–5 hits are enabled, except lints with false positives here or a known false-positive record (see below). - **Pedantic lints:** about 20 cheap ones with 5 or fewer hits each, plus `large_futures`, `unused_async`, `assigning_clones`, `ref_option`, `implicit_clone` and `cloned_instead_of_copied`. - **`too_many_lines`:** threshold 150, set in `clippy.toml`. The 16 existing long functions have `#[allow]`, so only new long functions are rejected. - **`unwrap_used` and `expect_used`:** production code may neither unwrap nor expect. `allow-unwrap-in-tests` and `allow-expect-in-tests` are set. Bench, example and integration-test helper files outside `cfg(test)` have a file-level allow. Excluded after measurement because they produced false positives here or are nursery lints with known false positives: `branches_sharing_code`, `collection_is_never_read`, `create_dir`, `duration_suboptimal_units`, `mutex_atomic`, `needless_pass_by_ref_mut`, `set_contains_or_insert`, `trait_duplication_in_bounds`, `unnecessary_wraps` and `unused_self`. ## Why These lints catch real problems the default set misses: - panics in production code (`unwrap`, `expect`, `todo`, unchecked time subtraction) - large futures on the async stack - `async` functions that never await - needless clones - lossy `Arc<Vec<_>>` patterns - functions growing past a reviewable size Replacing `unwrap()` with `expect()` only renames the panic, so `expect_used` is enforced too. ## Related issue None. Tooling and cleanup change. ## Scope and non-goals - **In scope:** `Cargo.toml` lint tables, `clippy.toml`, and the code changes needed to satisfy the lints, including test code, which the lints also cover. - **Not included:** high-volume pedantic lints such as `needless_pass_by_value` (37 hits), `cast_lossless` (47), the `cast_*` family (200+), `missing_errors_doc` and `must_use_candidate`. `panic` and `indexing_slicing` are also not enabled. ## Design and behavior changes - **Lock poisoning:** std `lock()`/`read()`/`write()`/`Condvar::wait`/`get_mut` results recover the guard with `unwrap_or_else(|poisoned| poisoned.into_inner())`, matching existing code in `resolver.rs` and `startup_pack.rs`. - **Other production `expect`s:** - Most return errors through `?` (fixed-size header parsing, the zstd contexts, the ublk and ublk-daemon binaries, the CLI commands). - The rest are restructured so the invariant needs no check (`let … else`, iterating pairs instead of re-looking up keys, deriving VM/TAP IPs in `NetworkAddressPlan::from_config`, keeping a pre-keyed HMAC, storing `CommittedSnapshot` directly in `RunnableSnapshot`). - **Panics that could really happen and now return errors:** - `IndexLBPT::new` on an empty mapping. - `pack_planner::stage_block` with more than 65,536 objects. - `Slot::new` failures in the network manager, which also free the slot bit now. - **Remaining `expect`s:** 12 sites carry `#[expect(clippy::expect_used, reason = …)]`: - global singletons that require startup initialization (`ConfigManager`, `UblkDeviceManager`, `NetworkManager`) - the embedded `deps_manifest.toml` ×3, now each covered by a parse test - the envd bootstrap HTTP client - the host network-namespace fd - four helpers that serialize plain derived structs, whose bytes are hashed into P2P keys and index paths and so must keep their encoding - **Egress deny list:** if the platform deny list cannot be derived, all egress is denied (fail closed) and an error is logged. - **Shutdown signals:** if the server cannot install its Ctrl+C or SIGTERM handler, it logs an error and keeps running instead of panicking. - **`unused_async`:** functions with no `.await` are no longer `async`, and callers are updated (see Compatibility). - **`large_futures`:** futures over 16 KiB are boxed with `Box::pin` at their await sites. - **Cleanups kept although their lints were later dropped:** functions that always returned `Ok` or `Some` now return the value directly (for example `ArtifactCache::evict_lru`, the `setup/packages.rs` helpers and two ublk-daemon handlers). Methods that never used `self` became associated functions. A few `&mut` parameters became `&`. - **Smaller fixes:** - The 128 KiB digest buffers move from the stack to the heap. - The Firecracker stderr tail read is bounded with `take()`. - The egress proxy reaps finished connections with `HashMap::extract_if`. - `Arc<Vec<PathBuf>>` becomes `Arc<[PathBuf]>`. - Two extension checks in `deps/mod.rs` and `sandbox/envd.rs` now use `Path::extension()` and compare case-insensitively. - **Removed:** `RegistryFsV2::new()` and `impl Default for RegistryFsV2`, which had no callers and panicked on failure. ## Compatibility and operations - **Public API or generated protocol:** these Rust APIs inside the workspace changed. All workspace callers are updated. No HTTP, gRPC or OpenAPI changes, and no generated code was edited. - `async fn` → `fn`: - `overlaybd::ImageService::{new, from_config_path, from_config_path_with_p2p_publish_url}` - `VolumeManager::{open_with_repository, open_with_repository_and_limits}` - the ublk `io_buffer` constructors - Now return `Result`: - `IndexLBPT::new` - `UserBuffer::uring_buf_idx` / `IOBuffer::uring_buf_idx` (`io::Result<u16>`) - `Orchestrator::with_in_memory_store` - Removed: `RegistryFsV2::new` / `Default`. - **Configuration or defaults:** none. - **Snapshot manifest, artifact layout, or storage format:** none. P2P keys and commit-index paths keep their serde encoding. - **Upgrade and rollback:** none needed; code-only change. - **Host requirements, permissions, ports, or dependencies:** none. Needs Rust 1.88+ for `HashMap::extract_if`; the toolchain is stable. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` - [x] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [ ] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text make fmt ok make clippy ok make test-unit ok (989 + 114 + 49 + 18 + 4 + 3 passed, 0 failed) cargo test -p overlaybd -p storage-util -p adev -p warm-pool \ -p shell-util -p object-store-operator --lib ok (387 + 13 + 12 + 7 + 5 passed) cargo test -p uvm-ublk-daemon --test ublk_daemon_test ok (37 passed) ``` Skipped checks and reasons: - **KVM integration suites** (`tests/integration/*`, `make test-agent-integration`): not run. The changes there are signature-only or test-helper changes, and they compile under clippy. - **`services/`:** unchanged. - **Generated code:** untouched. - **Benchmarks:** not run; they compile under clippy. ## Risks and reviewer notes - **Poison recovery:** a panic while holding a std lock no longer cascades into every later locker; the guard is recovered instead. This matches the existing resolver and startup-pack code. - **Egress deny list:** `NetworkAddressPlan` derivation failing now denies all egress instead of panicking. Startup already validates the same config, so this should not happen in practice. - **Server signal handlers:** a failed handler install now logs an error and continues, so that signal no longer triggers graceful shutdown. - **Async to sync:** the former `async fn`s did no awaiting, so whatever blocking they did already happened on the executor thread; making them sync changes nothing at runtime. - **`Box::pin`:** adds one heap allocation per affected call, all on control-plane or test paths. - **The remaining files:** mostly mechanical. `clippy --fix` output was reviewed, and the poor rewrites were changed back. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

bsakaguchi added the

pull-request

label 2026-10-29 14:22:58 +00:00

bsakaguchi referenced this issue 2026-10-29 14:23:02 +00:00

[PR #397] [MERGED] feat: persist source sandbox user metadata in published snapshot records #404

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

coverage-data

gh-pages

v0.3.0-rc.1

v0.2.4

v0.2.3

v0.2.2

v0.2.1

v0.2.0

v0.1.3

v0.1.2

v0.1.1

v0.1.0

Labels

Clear labels   

bug

  

documentation

  

enhancement

  

enhancement

  

pull-request

Mirrored from GitHub Pull Request

No labels

bug

documentation

enhancement

enhancement

pull-request

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

kvcache-ai/AgentENV#397

No description provided.