refactor(cli): replace arg-count suppressions with request structs - #591
Merged
Conversation
PR #565 papered over two `too_many_arguments` hits in the CLI run commands with `#[allow]`. Group the arguments into `CliRunRequest` / `CliMessageRequest` instead, matching the `request: T` command convention already used by `benchmark_*`, `human_session_*`, and pagination. `turn_intent_id` + `client_message_id` move into a `TurnIdentity` pair so the "adopt the client's ids or mint fresh ones" rule lives in one place rather than being duplicated across the two entry points.
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
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
Follow-up to #565, which introduced two
#[allow(clippy::too_many_arguments)]suppressions inagent_sessions/cli/commands/run.rs. This removes both by grouping the arguments into request structs instead of silencing the lint.Problem
#565 grew
cli_agent_run_internalto 8 arguments and thecli_agent_messagecommand to 9, and suppressed the resulting clippy warnings. Beyond the suppressions themselves, the flat signatures made every Rust caller unreadable — the agent-core bridge calledcli_agent_messagewith five consecutive positionalNones, and the debug runtime probes had six such call sites. The "adopt the client's turn ids or mint fresh ones" rule was also duplicated across both entry points.Solution
CliRunRequest/CliMessageRequestreplace the flat argument lists. Both deriveDefault, so callers name only the fields they mean (..Default::default()) instead of padding with positionalNones. This matches therequest: Tcommand convention already used bybenchmark_*,human_session_*,pagination_*, andxlsx_*.TurnIdentityholds theturn_intent_id+client_message_idpair, withgenerate()(mint both) andfrom_client()(adopt whichever halves the client supplied). The id-defaulting rule now lives in one place.cli_agent_run_internalbecomesrun_turn(request, turn)— 2 args, no suppression.cli_agent_messagenow takes a singlerequestobject, so the frontend sends{ request: {...} }.CliMessageRequestSchemais the inner shape;CliMessageInputSchemawraps it.Net: both suppressions gone, 159 lines deleted for 193 added (mostly doc comments on the new structs), and 8 call sites became self-documenting.
Potential risks
cli_agent_messagepayload shape changed.rpc.cli.messageis the only caller (cliTransport.ts), updated in this PR along with its unit test. The zod input schema still enforcessessionId/content/turnIntentId/clientMessageId, so a stale caller fails validation before IPC rather than silently sending a malformed payload.cli_agent_run's argument shape also changed, but it has no TS caller — only the agent-core bridge and the debug-only runtime probes invoke it, both updated here.Defaulton the request structs meanssession_idcan silently default to""at a Rust call site that forgets it. Not reachable over the wire: serde still rejects missing non-Optionfields, and all four Rust call sites setsession_idexplicitly.turn_intent_idon everystatus_changedbroadcast, receipt-based resolution) are untouched.Test plan
cargo clippy --all-targets— clean; no warnings in the three touched Rust files. The 11 pre-existingorg2lib warnings are all in files this PR does not touch.cargo fmt --check— no diffs in touched files (27 pre-existing diffs elsewhere in the repo).npx tsc --noEmit— passes.npx eslinton the three touched TS files — clean.npx vitest run src/engines/SessionCore/sync/adapters/cli/__tests__/cliTransport.test.ts— 2/2 passing (expectation updated for the{ request }payload).Not run: end-to-end CLI turn against a live agent. The change is signature-level with no behavioral edits, but a manual follow-up turn (send message → verify the optimistic row reconciles against the receipt) would confirm the
{ request }payload lands correctly through real IPC.