Adopt the Whitaker Dylint suite in the lint gate and CI - #134
Merged
Conversation
added 2 commits
July 9, 2026 18:45
Prepare the codebase for adopting the Whitaker Dylint suite by fixing every finding it reports, without suppressions: - bumpy_road_function: extract build_agent_overrides and serialize_agent_override from build_overrides in the config loader, and append_file_entry/append_symlink_entry from append_non_directory_entry in the credential archive writer. - module_max_lines: split the protocol exec module into stdin_io and output_io sibling submodules; move the error module's tests to a sibling error_tests.rs; split the github tests into client_tests and property_tests submodules; split the ACP frame and protocol ACP test modules; move the configuration layer-precedence BDD steps into tests/bdd_config_steps/layer.rs. - module_must_have_inner_docs: add //! inner documentation to every unit-test module that lacked it. - no_expect_outside_tests: helper and fixture functions no longer panic; they return Result (io::Result, serde_json::Error, or PodbotError) and propagate failures, with the calling test bodies unwrapping as the test verdict. Mutex-guard recorders in test doubles now recover from poisoning via PoisonError::into_inner instead of panicking. - no_std_fs_operations: the Makefile audit-target test and the BDD config-loader helpers now perform fixture filesystem work through cap-std directory handles instead of ambient std::fs calls. Also resolve the Clippy too_many_arguments and needless_pass_by_value diagnostics introduced by the refactoring.
Wire the Whitaker Dylint suite into the standard quality gates as part of the estate-wide rollout (see leynos/netsuke#410): - Makefile: add a WHITAKER tool variable and run the suite after Clippy in the lint target, with warnings denied. - CI: pin whitaker-installer to the 0.2.5 crates.io release via a job-level WHITAKER_INSTALLER_VERSION environment variable, cache the installer binary and the cargo-binstall download cache keyed by runner OS, architecture, and installer version, and install with cargo binstall --locked, falling back to building from crates.io on runners without binstall.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR pins and integrates a Whitaker Dylint installer into CI and the Makefile lint target, refactors config-loader agent overrides into helper functions, and substantially reworks the protocol exec stdin/output forwarding by extracting it into new ChangesWhitaker CI/lint integration
Config loader agent overrides
Protocol exec extraction and ACP tests
Test runtime fallibility hardening
Archive, error, and GitHub test reorganisation
Test infrastructure: BDD config and audit target
Sequence Diagram(s)sequenceDiagram
participant HostStdin
participant ForwardHostStdinToChannel
participant WriteCmdSink
participant OutboundPolicyAdapter
participant HostStdout
HostStdin->>ForwardHostStdinToChannel: read buffered stdin
ForwardHostStdinToChannel->>WriteCmdSink: send masked initialize frame (WriteCmd::Forward)
ForwardHostStdinToChannel->>WriteCmdSink: pump remaining raw frames until EOF
WriteCmdSink->>OutboundPolicyAdapter: handle_chunk(bytes)
OutboundPolicyAdapter->>HostStdout: write_output_chunk(forwarded bytes)
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/github/tests.rs (1)
41-61: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPropagate the fallible fixture with
?instead of.expect()-ing it in every test.
temp_key_dirnow returnsio::Result<(TempDir, Utf8Dir)>, but every consumer (this test andload_missing_file_returns_error,load_empty_file_returns_error,load_invalid_pem_returns_error,load_ec_key_returns_clear_error,load_ed25519_key_returns_clear_error,error_includes_file_path,load_private_key_resolves_full_path,load_invalid_key_types_return_clear_error) unwraps it via.expect("should create temp key dir"). The coding guideline explicitly states falliblerstestfixtures should be consumed by making the test returnResultand applying?, not by.expect()-ing them inline.♻️ Proposed pattern (apply across all affected tests)
#[rstest] fn load_valid_rsa_key_succeeds( valid_rsa_pem: String, temp_key_dir: io::Result<(TempDir, Utf8Dir)>, -) { - let (_tmp, dir) = temp_key_dir.expect("should create temp key dir"); +) -> io::Result<()> { + let (_tmp, dir) = temp_key_dir?; dir.write("key.pem", &valid_rsa_pem) .expect("should write key"); let path = Utf8Path::new("/display/key.pem"); let result = load_private_key_from_dir(&dir, "key.pem", path); assert!(result.is_ok(), "expected Ok, got: {result:?}"); + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/tests.rs` around lines 41 - 61, The `temp_key_dir` fixture is fallible, so update each affected `rstest` test that uses it to return `io::Result<()>` (or the appropriate `Result`) and use `?` when binding the fixture instead of calling `.expect("should create temp key dir")`; apply this consistently in `load_valid_rsa_key_succeeds`, `load_missing_file_returns_error`, `load_empty_file_returns_error`, `load_invalid_pem_returns_error`, `load_ec_key_returns_clear_error`, `load_ed25519_key_returns_clear_error`, `error_includes_file_path`, `load_private_key_resolves_full_path`, and `load_invalid_key_types_return_clear_error`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 45-62: The current cache in the CI workflow only covers the
Whitaker installer binary and binstall downloads, but the unconditional
`whitaker-installer` invocation still rebuilds the staged Dylint suite every
job. Update the cache setup in the `Cache Whitaker installer` / `Install
Whitaker` steps to also include the directory that `whitaker-installer`
populates for the built suite (or the relevant `~/.cargo`/target cache it uses),
and key it by `WHITAKER_INSTALLER_VERSION` so repeated runs can reuse the
expensive build output instead of rebuilding it each time.
In `@src/config/loader.rs`:
- Around line 247-257: `serialize_agent_override` has a redundant `Ok(...?)`
wrapper around `serde_json::to_value(value).map_err(...)`, which is already the
correct `Result<serde_json::Value, ConfigError>` shape. Simplify the function by
returning the mapped result directly and keep the existing
`ConfigError::ParseError` message formatting unchanged so the helper stays
clippy-clean under `-D warnings`.
In `@src/engine/connection/exec/acp_runtime_bdd_tests.rs`:
- Around line 69-77: The serialize_frame function contains a redundant explicit
drop of value after serde_json::to_vec(&value), since the Value is only borrowed
and will be dropped automatically at scope end. Remove the unnecessary
drop(value) call in serialize_frame and keep the rest of the frame serialization
logic unchanged.
In `@src/engine/connection/exec/protocol_acp_routing_tests.rs`:
- Around line 27-36: The blocked-frame builder is duplicated between the ACP
routing and policy test modules, so consolidate it into the shared test harness.
Move the parameterized frame builder into protocol_acp_tests.rs alongside
initialize_frame, then update blocked_terminal_create_frame and the existing
blocked_request_frame helper to call that shared function via super::. Keep the
shared helper generic over id/method so both sibling tests reuse the same
implementation.
In `@src/engine/connection/exec/runtime_helpers.rs`:
- Around line 110-115: The `block_on_runtime_maps_outcomes_outside_tokio` test
is still unwrapping the fallible `current_thread_runtime` fixture with `expect`;
update this `rstest` case to return `io::Result<()>` (or the appropriate Result
type) and propagate the fixture with `?` instead of unwrapping. Keep the rest of
the test logic the same by using the `Runtime` from `current_thread_runtime`
after successful propagation, and reference the `current_thread_runtime` fixture
and `block_on_runtime_maps_outcomes_outside_tokio` test when making the change.
In `@src/engine/connection/upload_credentials/archive.rs`:
- Around line 146-161: The size metadata in append_file_entry is fetched from
parent_dir.metadata before the file is opened, creating a TOCTOU window that can
make the tar header size inconsistent with the actual stream. Open the file
first with parent_dir.open, then read metadata from that file handle and use it
to build the header in new_entry_header so Builder::append_data gets the real
size from the opened file. Keep the fix localized to append_file_entry and its
use of metadata_mode/new_entry_header.
In `@src/github/client_tests.rs`:
- Around line 14-70: These rstest cases are unwrapping the fallible temp_key_dir
fixture with .expect, instead of propagating the error from the test itself.
Update build_app_client_with_valid_key_succeeds,
build_app_client_with_zero_app_id_succeeds, and
build_app_client_without_runtime_returns_error (and the related
validate_app_credentials tests) to return Result and use ? when consuming
temp_key_dir. Keep the rest of the setup and the calls to
build_app_client/load_private_key_from_dir the same, but remove the inline
expect on the fixture.
In `@tests/bdd_config_steps/layer.rs`:
- Around line 121-149: The test step configuration_is_merged has a redundant
#[expect(clippy::expect_used)] on its use of expect, and this lint is already
allowed by the test clippy settings. Remove that attribute from
configuration_is_merged and keep the rest of the merge logic unchanged so the
step remains a normal test helper without an unfulfilled lint expectation.
In `@tests/make_audit_target.rs`:
- Around line 47-68: De-duplicate the hard-coded cargo stub path used by
write_fake_cargo and its callers by introducing a shared relative path constant
or having write_fake_cargo return the path it creates. Update the
write_fake_cargo helper and the call sites that currently use
workspace.join("bin/cargo") so they reference the same unique symbol instead of
repeating the string literal.
---
Outside diff comments:
In `@src/github/tests.rs`:
- Around line 41-61: The `temp_key_dir` fixture is fallible, so update each
affected `rstest` test that uses it to return `io::Result<()>` (or the
appropriate `Result`) and use `?` when binding the fixture instead of calling
`.expect("should create temp key dir")`; apply this consistently in
`load_valid_rsa_key_succeeds`, `load_missing_file_returns_error`,
`load_empty_file_returns_error`, `load_invalid_pem_returns_error`,
`load_ec_key_returns_clear_error`, `load_ed25519_key_returns_clear_error`,
`error_includes_file_path`, `load_private_key_resolves_full_path`, and
`load_invalid_key_types_return_clear_error`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1416b58c-8abf-48e5-9d75-572a4e3cf560
📒 Files selected for processing (41)
.github/workflows/ci.ymlMakefilesrc/api/configure_git_identity.rssrc/api/repository_clone.rssrc/config/loader.rssrc/engine/connection/error_classification.rssrc/engine/connection/exec/acp_frame_split_tests.rssrc/engine/connection/exec/acp_frame_tests.rssrc/engine/connection/exec/acp_policy_tests.rssrc/engine/connection/exec/acp_runtime_bdd_tests.rssrc/engine/connection/exec/acp_runtime_tests.rssrc/engine/connection/exec/helpers_tests.rssrc/engine/connection/exec/host_io.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_bdd_tests.rssrc/engine/connection/exec/protocol_acp_forwarding_tests.rssrc/engine/connection/exec/protocol_acp_masking_tests.rssrc/engine/connection/exec/protocol_acp_policy_integration_tests.rssrc/engine/connection/exec/protocol_acp_routing_tests.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/protocol_output.rssrc/engine/connection/exec/protocol_stdin.rssrc/engine/connection/exec/runtime_helpers.rssrc/engine/connection/exec/session.rssrc/engine/connection/exec/terminal.rssrc/engine/connection/git_identity/container_configurator.rssrc/engine/connection/repository_clone/mod.rssrc/engine/connection/tests.rssrc/engine/connection/upload_credentials/archive.rssrc/error.rssrc/error_tests.rssrc/github/classify.rssrc/github/client_tests.rssrc/github/property_tests.rssrc/github/tests.rstests/bdd_config_helpers.rstests/bdd_config_loader_helpers.rstests/bdd_config_steps/layer.rstests/bdd_hosting_config_loader_helpers.rstests/make_audit_target.rstests/test_utils.rs
Consolidate the ACP test doubles and frame builders into a shared acp_test_support module: one poison-tolerant RecordingWriter replaces the three near-identical recorders, and one jsonrpc_frame builder replaces the per-module frame constructors, resolving the CodeScene code-duplication findings on acp_frame_tests, protocol_acp_policy_integration_tests, and protocol_acp_tests. The two structurally similar non-enforcing policy tests are now a single parameterized rstest case, and the two initialize-frame builders share a blocked_capabilities helper. Also, per reviewer feedback: - read tar entry metadata from the open file handle instead of a prior directory call, so the header size cannot drift from the streamed bytes if the file changes between the two calls; - consume fallible rstest fixtures by returning Result from the tests and propagating with `?` rather than unwrapping inline; - pass the frame value to serialize_frame by reference, removing the explicit drop; - return the mapped ConfigError directly instead of wrapping with Ok(...?); - name the fake cargo script path once via a constant.
leynos
commented
Jul 9, 2026
leynos
commented
Jul 9, 2026
leynos
commented
Jul 9, 2026
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CodeScene flagged the permitted-frame and blocked-notification tests as structurally duplicated. They are now a single parameterized rstest case that drives one frame through the adapter and asserts the expected host-stdout routing per frame kind.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The fixture-consumption rework made several tests return Result so fixtures propagate with `?`, but the crate denies clippy::panic_in_result_fn, which rejects assert!/assert_eq!/panic! in any Result-returning function, including tests. Swap those assertions for eyre::ensure! and the panicking match arms for eyre::bail!, and return eyre::Result so both io::Error and PodbotError propagate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change adopts the Whitaker Dylint suite in podbot's lint gate and continuous integration (CI), as part of the estate-wide rollout that began with leynos/netsuke#410. The Makefile gains a
WHITAKERtool variable and thelinttarget now runswhitaker --all -- --all-targets --all-featuresafter Clippy with warnings denied. The CI workflow installs the suite via the hardened estate pattern:whitaker-installeris pinned to the 0.2.5 crates.io release through a job-levelWHITAKER_INSTALLER_VERSIONenvironment variable (read as a plain shell variable inside the run block), installed withcargo binstall --lockedand a build-from-source fallback for runners without binstall, with a cache covering only the installer binary and the binstall download cache, keyed by runner operating system, architecture, and installer version.A preparatory commit resolves every finding the suite reported, without any suppressions or
dylint.tomlexclusions: twobumpy_road_functionextractions (config override building and credential archive entry writing); fivemodule_max_linessplits (the protocol exec module intostdin_ioandoutput_iosubmodules, the error module's tests, the GitHub tests, the ACP frame and protocol ACP test modules, and the configuration layer-precedence BDD steps); twelvemodule_must_have_inner_docsadditions; roughly fiftyno_expect_outside_testsconversions, under which helper and fixture functions now returnResultand propagate arrangement failures while only test bodies unwrap, and mutex-guard recorders recover from poisoning viaPoisonError::into_inner; and elevenno_std_fs_operationssites now performing fixture filesystem work through cap-std directory handles instead of ambientstd::fscalls.Review walkthrough
WHITAKERvariable and the extendedlinttarget.Validation
All gates were run locally with
env -u WHITAKERso the Makefile's default tool resolution was exercised:make check-fmt— passmake typecheck— passmake lint— pass (cargo doc, Clippy, andwhitaker --allclean under-D warnings, suite v0.2.5)make test— pass (all suites, zero failures)make markdownlint— pass (64 files, 0 errors)cargo check --no-default-featuresandcargo test --no-default-features --test cli_feature_gating --test compile_contract— pass (CI no-CLI legs)🤖 Generated with Claude Code