refactor: remove legacy Python compatibility comments and dead code#331
Open
l50 wants to merge 17 commits into
Open
refactor: remove legacy Python compatibility comments and dead code#331l50 wants to merge 17 commits into
l50 wants to merge 17 commits into
Conversation
**Changed:** - Updated or removed outdated, redundant, or now-misleading comments across orchestrator automation modules, state publishing, monitoring, task queue, and related tests for clarity and accuracy - Clarified intent in test and code comments, especially around credential, domain, and SID handling, to reflect current logic and prevent confusion from obsolete implementation notes - Removed references to no-longer-relevant tool behaviors or code paths (e.g., legacy ephemeral consumer patterns, removed tools) - Streamlined documentation and refactored comments in output extraction, result processing, recovery, and utility modules to match current architecture and avoid misleading future contributors **Removed:** - Removed `certifried` automation module and test suite (CVE-2022-26923 machine account spoofing) as no exploit primitive is registered and the dispatch path is unused - Removed `auto_certifried` from orchestrator automation exports and spawner
…only **Changed:** - Updated result pattern matching logic to ignore top-level `error` fields and scalar `output`/`tool_output` payload fields, restricting matching to structured `tool_outputs` arrays for LLM and legacy workers - Refactored `result_matches_patterns` in `s4u.rs` and `has_lockout_in_result` in `discovery_polling.rs` to remove legacy branches that previously matched on scalar payloads and top-level error strings - Simplified associated test helpers and updated tests to expect pattern matching only via `tool_outputs`, ensuring that narrative fields do not drive retry or lockout logic - Removed tests for legacy/worker-specific error and scalar output detection, consolidating around the new, stricter logic - Clarified documentation comments to explain the rationale for the change and the separation of loop-control/status from retry/error detection
**Changed:** - Simplified `collect_payload_text_parts` to only process `tool_outputs` array, removing support for legacy `tool_output` and `output` scalar fields - Updated `payload_contains_golden_ticket_marker` to align with the new payload text extraction logic, ensuring only modern output conventions are considered - Removed `collect_payload_text_parts_with_policy` and `payload_contains_golden_ticket_marker_with_policy` helper functions to reduce code complexity and eliminate unused options **Removed:** - Dropped support for extracting text from legacy scalar fields (`tool_output` and `output`) in payload processing, focusing solely on current formats
**Changed:** - Removed all conditional logic for supporting legacy scalar output fields (`tool_output`, `output`) from result processing functions; all logic now relies solely on structured `tool_outputs` arrays - Simplified function signatures and internal calls by removing `include_legacy_scalar_outputs` flags and associated branching - Updated related tests to expect only `tool_outputs` sources, removing checks for scalar output fields and policies - Cleaned up documentation to reflect the exclusive use of structured outputs - Refactored payload text collection helpers to operate on the new expectations, streamlining input handling and reducing ambiguity **Removed:** - Eliminated `legacy_scalar_outputs_allowed` and all code paths enabling fallback to legacy scalar output fields in result analysis - Removed policy-based test cases and helper functions for toggling legacy scalar output support in tests and production code
… clarity
**Added:**
- Introduced new parameter structs for major entry points and helpers, including:
- `BlueSubmitParams`, `BlueFromOperationParams`, `OpsInjectVulnerabilityParams`,
`OpsInjectHashParams`, `OpsSubmitParams`, `RelayCoerceInputs`, `Esc1ChainInputs`,
`Esc4ChainArgs`, `DispatcherDeps`, `BlueTaskLoopDeps`, `HeartbeatConfig`,
`AddConnectionParams`, `TraceToolCallParams`, `TraceDiscoveryParams`,
`TraceDecisionParams`, `RunAgentLoopParams`, `FinishArgs`
- Added helper constructors in tests for easier struct instantiation
**Changed:**
- Refactored functions with many arguments to take parameter structs instead,
improving readability and maintainability across multiple modules:
- Blue and ops command/submit/inject functions in `ares-cli`
- ADCS exploitation and orchestrator dispatcher constructors
- Lateral movement graph and analyzer connection methods in `ares-core`
- Telemetry span helper functions for tool calls, discovery, and decision
- Agent loop runner and related interfaces in `ares-llm`
- Worker task loop and heartbeat functions
- Updated all call sites, tests, and integration tests to use the new struct-based
signatures, eliminating use of clippy's `too_many_arguments` suppression
- Improved future extensibility and parameter passing by grouping related arguments
into dedicated types (e.g., `BlueSubmitParams`, `OpsSubmitParams`, `AddConnectionParams`)
- Ensured all trait and interface changes are consistently applied throughout codebase
**Removed:**
- Removed legacy multi-argument function signatures in favor of parameter structs
- Eliminated redundant clippy `too_many_arguments` attributes throughout the codebase
**Changed:** - Reformatted the `basic_conn` function signature to span multiple lines for improved readability and consistency with Rust style guidelines in `tests.rs`
**Changed:** - Investigation outcome logging now directly references `steps` and `severity` fields without intermediate variables - `InvestigationOutcome` enum simplified by removing redundant field attributes - `OrchestratorConfig` no longer uses `#[allow(dead_code)]` on struct - Global deferred task cap enforced in `DeferredQueue::enqueue` to prevent single task types from exceeding total limit - Recovered state in recovery manager now drops loaded state after use and no longer stores it in the returned struct - Re-exports in recovery module clarified for test and worker usage, with deduplication helpers re-exported only where needed - Unused import of `SharedRedTeamState` removed from recovery types - Test-only code throughout orchestrator (task queue, throttling, monitoring) now uses `#[cfg(test)]` instead of `#[allow(dead_code)]` - Helper methods in task queue and throttling modules gated to tests with `#[cfg(test)]` - Conversation trimming in LLM agent loop now test-only with `#[cfg(test)]` - Legacy and unused code paths and comments cleaned up for clarity **Removed:** - Unused MSSQL enum unpersist helper from shared state dedup module - Unused Kerberos ticket lookup helper from shared state publishing/kerberos module - Redundant `state` field from `RecoveredState` struct in recovery types - Unused import of `optional_i64` from tools ACL parser - Unused constant for all extended rights GUID from NTSD parser - Redundant `#[allow(dead_code)]` and `#[allow(unused_imports)]` attributes throughout orchestrator and detection modules
…ipts **Added:** - Introduced atomic enqueue and remove operations for deferred tasks using Redis Lua scripts to enforce per-type and global queue limits - Added `total_key()` to generate a unique Redis key for tracking total deferred tasks per operation - Implemented a global counter for deferred tasks, maintained atomically via Lua scripts and exposed via `total_count()` for O(1) reads - Provided `reconcile_total()` method to repair or initialize the global counter by scanning all queue ZSETs - Integrated startup reconciliation of the global counter in orchestrator initialization, with warning logging on failure **Changed:** - Reworked `enqueue()` to use the atomic Lua script, ensuring correct enforcement of per-type and global queue caps, and handling idempotent reinserts - Updated `pop_best()` and eviction logic to use atomic removal and decrement the global counter consistently - Modified total count logic to use the new atomic counter rather than scanning all ZSETs, improving performance and correctness **Removed:** - Eliminated ad hoc counting of total deferred tasks by scanning all ZSETs, replacing with atomic counter approach
…pace **Added:** - Enabled workspace-level lint configuration in all member crates by adding `[lints] workspace = true` to each crate's `Cargo.toml` - Introduced workspace-level clippy lint for `too_many_arguments` set to "deny" to enforce parameter struct usage in function signatures **Changed:** - Centralized lint configuration by moving clippy lint rules to the root `Cargo.toml` under `[workspace.lints.clippy]` for consistent enforcement across all workspace members
…dules **Changed:** - Removed or reworded comments and docstrings referencing "mirrors Python" or "matches Python" in orchestrator modules and related code, clarifying that implementations are now standalone or simply describing the logic - Updated doc comments and inline comments to focus on current Rust behavior, improving maintainability and reducing cross-language coupling - Preserved meaningful explanations of logic, such as concurrency, credential extraction, and Redis key usage, while removing references to Python counterparts or implementation history
**Changed:** - Removed references to matching Python classes and protocols from doc comments across models, state, telemetry, and token usage modules to reduce cross-language coupling in documentation - Updated comments and docstrings to clarify data formats, encoding, deduplication, and Redis key structures, focusing on the Rust-side behavior and interoperability requirements rather than Python specifics - Improved clarity in doc comments for meta value encoding, deduplication keys, and token usage field naming, emphasizing JSON encoding and base64 field formatting - Revised test and code comments to generalize or rephrase notes about compatibility, replacing "Python stores..." with direct statements about storage formats and encoding conventions used in the Rust codebase
…ic in docs **Changed:** - Removed comments and docstrings referencing Python implementation details, emphasizing Rust-native logic and behaviors throughout the codebase - Clarified and streamlined documentation for modules, functions, and comments to focus on current Rust implementation and usage, avoiding references to ported or replaced Python code - Updated wording in help messages, doc comments, and inline comments to improve clarity and accuracy regarding current behavior - Ensured all changes preserve functionality and intent while improving maintainability and developer experience
…derive_partial_eq_without_eq sites
Cleanup pass enforcing four clippy lints across the workspace and locking them
into the workspace gate so future regressions fail CI.
- manual_let_else: converted ~50 `let x = match opt { Some(x) => x, None => ... };`
patterns to `let Some(x) = opt else { ... };` for less rightward drift.
- redundant_clone: dropped 19 `.clone()` calls at last-use sites where a move
suffices.
- needless_collect: removed 13 intermediate `Vec` allocations in favor of
`.iter().any()` / `.count()` / direct iteration. One site in
`mssql_link_pivot::tail_lines` kept the collect with
`#[expect(clippy::needless_collect, reason = "Lines: !ExactSizeIterator so
.take(n).rev() doesn't typecheck")]` because the suggested rewrite doesn't
compile (`Lines` is `DoubleEndedIterator` but not `ExactSizeIterator`).
- derive_partial_eq_without_eq: added `Eq` to 15 derive sites whose fields
permit it. `VulnerabilityInfo` keeps `PartialEq`-only with
`#[expect(... reason = "details: HashMap<String, serde_json::Value> —
serde_json::Value contains f64 Number, so Eq cannot be derived")]` because
`serde_json::Value`'s `Number` variant wraps `f64`.
All four lints are now `deny`d in `[workspace.lints.clippy]`. `cargo fmt`,
`cargo check --workspace --all-targets`, `cargo clippy --workspace --all-targets
-- -D warnings`, and `cargo test --workspace --lib --bins` (6073 passed / 0
failed) all pass.
l50
changed the title
**Changed:** - Refactored credential selection to use chained iterators instead of collecting into an intermediate Vec, improving efficiency and satisfying clippy recommendations in `adcs.rs` - Updated logic to prioritize same-domain credentials first, then same-forest cross-domain credentials, stopping at the first unprocessed dedup key
**Added:** - Unit tests for environment variable helpers in `ops/submit.rs`, covering collection and resolution logic for env vars and model selection - Unit tests for `process_outcome` and `resolve_report_dir` in `orchestrator/blue/investigation.rs`, validating outcome mapping and report directory resolution priority - Unit tests for `result_from_outcome` in `worker/blue_task_loop.rs`, ensuring all `LoopEndReason` variants are mapped correctly to `BlueTaskResult` - Unit tests for `parse_operation_id_envelope` in `agent_loop/runner.rs`, verifying correct parsing for plain strings, JSON envelopes, missing fields, and malformed input - Updated `codecov.yml` to ignore true CLI/service plumbing files that are impractical to unit test, such as orchestrator/worker entry points and glue layers for Redis/NATS/LLM, while documenting rationale for each exclusion **Changed:** - Refactored `execute_blue_task` in `worker/blue_task_loop.rs` to delegate outcome-to-result mapping and logging to separate functions, improving testability and separation of concerns
l50
changed the title
l50
changed the title
…sing logic **Added:** - Extended unit tests for DC discovery logic, covering all `DcTier` selection paths, fallback, forest, and service-based discovery in `ares-llm/src/routing/dc_discovery.rs` - Added tests for password policy parsing, authentication tools output, domain trusts, merging discoveries, and IP validation in `ares-tools/src/parsers/mod.rs` - Added tests for ACL enumeration, including handling of generic rights, filtering of self/system permissions, object type resolution, and multiple vuln types from a single ACE in `ares-tools/src/parsers/ntsd.rs`
**Changed:** - Reformatted chained method calls in test assertions to place each method on a new line for better readability in `mod.rs` and `ntsd.rs` - Reformatted inline arrays and argument lists to be more compact or readable as appropriate, reducing unnecessary line breaks - Unified style of multi-line assertions and function calls for consistency across test cases - Adjusted comments and spacing to align with code formatting best practices strictly for code clarity
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.
Key Changes:
Changed:
focus on their Rust implementation and remove historical references to Python
focusing on current Rust usage and key behaviors
referencing previous Python code or bugs
approach, clarifying behaviors and removing historical context
strategies
Removed:
certifried.rsfile and all related test cases and automation spawner references"matches Python", "mirrors Python", and "ported from Python" notes in docstrings
including test-only or private helpers no longer relevant to the Rust codebase
certifried.rsautomation module (no longer used or referenced)