chore: refactor remaining too_many_arguments functions to config structs
Nessuno ha ancora preso questa issue.
- Lingua principale
- Rust
- Stelle
- 8.7k
- Fork
- 1.3k
- Merge medio
- 2g 7h
- PR unite (30g)
- 243
Descrizione
Problem Statement
PR #1997 (issue #1408) refactored run::sandbox_create from a 21-parameter positional function into a 4-parameter function using a SandboxCreateConfig struct with Default. This eliminated the clippy::too_many_arguments suppression and cut test call sites from ~23 lines to ~6 lines.
The codebase still has 28 remaining #[allow(clippy::too_many_arguments)] suppressions across 11 files. Several of these functions have the same symptoms as sandbox_create: walls of None, false, &[], &HashMap::new() at call sites, test readability problems, and fragility when parameters are added or reordered.
This issue tracks the follow-up cleanup, prioritized by call-site count and readability impact.
Technical Context
Two config-struct precedents now exist in the CLI crate:
ProviderRefreshConfigInput<'a>(PR #1349): 6 all-required fields, noDefault, consumed viafn(server, input, tls)SandboxCreateConfig<'a>(PR #1997): 18 fields withDefaultimpl, consumed viafn(server, gateway_name, config, tls)
Both keep infrastructure params (server, tls) positional and bundle domain-specific options into a struct. The refactoring pattern is mechanical: define struct → add Default if most fields have sensible defaults → destructure at function entry → update call sites.
Affected Components
| Component | Key Files | Role |
|---|---|---|
| CLI run module | crates/openshell-cli/src/run.rs |
6 remaining suppressed functions |
| CLI policy update | crates/openshell-cli/src/policy_update.rs |
build_policy_update_plan (7 params) |
| CLI dispatch | crates/openshell-cli/src/main.rs |
Call sites for all CLI functions |
| CLI integration tests | crates/openshell-cli/tests/ |
30+ provider test calls, 11+ gateway test calls |
| Server compute | crates/openshell-server/src/compute/mod.rs |
ComputeRuntime::from_driver (12 params) |
| Supervisor network | crates/openshell-supervisor-network/src/proxy.rs, run.rs |
4 functions sharing proxy context params |
| Supervisor process | crates/openshell-supervisor-process/src/ssh.rs |
5 functions sharing SSH session params |
Technical Investigation
Complete Inventory — too_many_arguments Suppressions
Tier 1 — High Priority (many call sites, clear struct boundary)
| Function | File | Params | Call Sites | Natural Struct Grouping |
|---|---|---|---|---|
gateway_add |
run.rs:851 |
9 | 11 (1 main + 3 mTLS tests + 7 inline tests) | OIDC fields (issuer, client_id, audience, scopes) + connection params |
provider_create / provider_create_with_options |
run.rs:4515/4540 |
8-9 | ~30 combined (1 main + ~29 in provider_commands_integration.rs) |
Credential source params (from_existing, credentials, from_gcloud_adc, runtime_credentials) |
sandbox_policy_update |
run.rs:6691 |
13 | 2 (1 main) | Policy modification params + execution options (dry_run, wait, timeout_secs) |
Tier 2 — Medium Priority (fewer call sites, straightforward)
| Function | File | Params | Call Sites | Notes |
|---|---|---|---|---|
sandbox_exec_grpc |
run.rs:2654 |
8 | 2 | Also has implicit_hasher — both suppressions vanish with one struct |
sandbox_list |
run.rs:3265 |
8 | 2 | Filter/output options struct |
sandbox_logs |
run.rs:7301 |
8 | 2 | Log query options struct |
build_policy_update_plan |
policy_update.rs:21 |
7 | 2 + 22 indirect test calls | Couples with sandbox_policy_update |
Tier 3 — Medium Priority (clusters sharing overlapping params)
Supervisor SSH cluster (5 functions in ssh.rs):
handle_connection(9 params),SshHandler::new(7),apply_child_env(8),spawn_pty_shell(11),spawn_pipe_exec(10)- Share:
policy,workdir,netns_fd,proxy_url,ca_file_paths,provider_env,user_environment - One
SshSessionContextstruct replaces 5 suppressions
Supervisor network cluster (4 functions across proxy.rs and run.rs):
run_networking(12 params),start_with_bind_addr(11),handle_tcp_connection(12),handle_forward_proxy(14)- Share:
opa_engine,entrypoint_pid,tls_state,inference_ctx,denial_tx,activity_tx - One
ProxyContextstruct serves all four
Server compute:
ComputeRuntime::from_driver(12 params, 4 call sites) — gateway shared state is the same across all 4 callers
Tier 4 — Low Priority (skip or defer)
| Function | Reason |
|---|---|
BaseEvent::new (OCSF, 8 params) |
Internal to builders; callers use builder API |
draw_text_field (TUI, 9 params) |
UI helper, 5 calls in same module |
build_vertex_route (inference, 8 params) |
Pure data construction, 3 calls |
stream_exec_over_relay / stream_interactive_exec_over_relay (gRPC, 8-9 params) |
1 call site each |
| VM driver / K8s driver helpers | Internal methods, 1-2 call sites each |
EntrypointProcess::spawn / spawn_impl |
Dispatcher pattern |
ServerState::new / build_compute_runtime |
Startup constructors, 1-3 calls |
Cross-Cutting Observations
- The
server: &str+tls: &TlsOptionspair appears in nearly every function inrun.rs. AGatewayConnectionstruct could eliminate this repetition, but that's a broader refactoring suitable for a separate issue. ComputeRuntime::from_driverhas a parameter_allows_loopback_endpoints: boolprefixed with_— appears unused (dead code). Confirm and remove before or during the struct refactoring.
Proposed Approach
Tackle Tier 1 items first as individual PRs (one per function or function cluster), following the same mechanical pattern established by PR #1997. Each PR defines a config struct with Default, updates the function signature, destructures at entry, and migrates call sites. Tier 2 items follow. Tier 3 clusters (proxy context, SSH session) require a brief design decision on struct scope before implementation. Tier 4 items are deferred unless someone is already touching those areas.
Scope Assessment
- Complexity: Low for Tier 1-2 (mechanical, identical to #1997). Medium for Tier 3 (struct design needed for shared contexts).
- Confidence: High — clear path, excellent test coverage, two established precedents.
- Estimated files to change: ~3 per Tier 1 item; ~2 per Tier 2 item; ~2 per Tier 3 cluster.
- Issue type:
chore
Risks & Open Questions
- Proxy context struct design: Should
ProxyContextholdArcs directly or references? Thetokio::spawnin the accept loop likely requires Arcs — confirm ownership model before implementing. server+tlsextraction: Each individual struct PR should keepserver/tlspositional (matching existing precedent) so a broader extraction can happen independently later.- Sequencing: Tier 1 items are independent of each other and can be tackled in parallel by different contributors.
_allows_loopback_endpoints: Confirm this is dead code before removing.
Recommended Execution Order
gateway_add→GatewayAddConfig(9 params, 11 call sites)provider_create/provider_create_with_options→ProviderCreateConfig(8-9 params, ~30 call sites)sandbox_policy_update+build_policy_update_plan→PolicyUpdateConfig(13 params)sandbox_exec_grpc/sandbox_list/sandbox_logs→ individual small structs (8 params each)ComputeRuntime::from_driver→ComputeRuntimeDeps(12 params, 4 call sites)- SSH spawn cluster →
SshSessionContext(5 functions, overlapping params) - Proxy context cluster →
ProxyContext(4 functions, overlapping params)
Test Considerations
- No new tests needed for any item. Existing tests validate behavior preservation.
- Tier 1 items produce the biggest test readability wins:
provider_commands_integration.rshas ~30 calls that will each shrink from ~12 lines to ~4 lines. - All config structs should have
Defaultwhere sensible, withserver/tlsas mandatory function params. - Run
cargo test -p <crate>andmise run pre-commitfor each PR.
Created by spike investigation. Predecessor: #1408 / PR #1997. Use build-from-issue to plan and implement individual items.
Guida per i contributori
Apri la guida per i contributori
Come iniziare
- Leggi tutta la issue e poi la guida ai contributi del progetto.
- Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
- Fai un fork del repository e lavora su un branch.
- Apri una pull request che faccia riferimento al numero della issue.
Direzione di ricerca
Inizia dal punto di ingresso Tier 1 in crates/openshell-cli/src/run.rs, usando PR #1997 e SandboxCreateConfig come modello; gateway_add è il primo obiettivo consigliato. Aggiorna i relativi punti di chiamata, inclusi crates/openshell-cli/tests/, quindi esegui cargo test -p openshell-cli e mise run pre-commit. Il lavoro è completato quando la soppressione selezionata è stata rimossa, mentre il comportamento esistente e i test rimangono intatti.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- rust
- Ambito
- backend, cli, developer-experience
- Tipo di issue
- Refactoring
- Difficoltà
- 4/5
- Tempo stimato
- 3-5 giorni
- Stato di attività
- Attiva
- Chiarezza
- Abbastanza chiara
- Idoneità per principianti
- 45/100