NVIDIA / NVIDIA/OpenShell

chore: refactor remaining too_many_arguments functions to config structs

Aperta
#2,003 3 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

area:cli area:gateway area:supervisor spike state:stale
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, no Default, consumed via fn(server, input, tls)
  • SandboxCreateConfig<'a> (PR #1997): 18 fields with Default impl, consumed via fn(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 SshSessionContext struct 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 ProxyContext struct 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: &TlsOptions pair appears in nearly every function in run.rs. A GatewayConnection struct could eliminate this repetition, but that's a broader refactoring suitable for a separate issue.
  • ComputeRuntime::from_driver has a parameter _allows_loopback_endpoints: bool prefixed 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 ProxyContext hold Arcs directly or references? The tokio::spawn in the accept loop likely requires Arcs — confirm ownership model before implementing.
  • server + tls extraction: Each individual struct PR should keep server/tls positional (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

  1. gateway_addGatewayAddConfig (9 params, 11 call sites)
  2. provider_create / provider_create_with_optionsProviderCreateConfig (8-9 params, ~30 call sites)
  3. sandbox_policy_update + build_policy_update_planPolicyUpdateConfig (13 params)
  4. sandbox_exec_grpc / sandbox_list / sandbox_logs → individual small structs (8 params each)
  5. ComputeRuntime::from_driverComputeRuntimeDeps (12 params, 4 call sites)
  6. SSH spawn cluster → SshSessionContext (5 functions, overlapping params)
  7. 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.rs has ~30 calls that will each shrink from ~12 lines to ~4 lines.
  • All config structs should have Default where sensible, with server/tls as mandatory function params.
  • Run cargo test -p <crate> and mise run pre-commit for 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

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. 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

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.