NVIDIA / NVIDIA/OpenShell

chore: refactor remaining too_many_arguments functions to config structs

Offen
#2,003 3 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Dieses Issue hat noch niemand übernommen.

area:cli area:gateway area:supervisor spike state:stale
Vorherrschende Sprache
Rust
Sterne
8.7k
Forks
1.3k
Ø Merge
2 T. 11 Std.
Gemergte PRs (30 T.)
253

Beschreibung

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.

Beitragsleitfaden

Beitragsleitfaden öffnen

Erste Schritte

  1. Lies das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreib ins Issue, dass du es übernimmst — das erspart doppelte Arbeit.
  3. Forke das Repository und arbeite in einem Branch.
  4. Öffne einen Pull Request, der die Issue-Nummer nennt.

Rechercherichtung

Beginne mit dem Tier-1-Einstiegspunkt in crates/openshell-cli/src/run.rs und verwende PR #1997 und SandboxCreateConfig als Muster; gateway_add ist das zuerst empfohlene Ziel. Aktualisiere seine Aufrufstellen, einschließlich crates/openshell-cli/tests/, und führe anschließend cargo test -p openshell-cli und mise run pre-commit aus. Erledigt ist die Aufgabe, wenn die ausgewählte Unterdrückung entfernt wurde und das bestehende Verhalten sowie die Tests intakt bleiben.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
rust
Bereich
backend, cli, developer-experience
Issue-Typ
Refactoring
Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Aktivitätsstatus
Aktiv
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
45/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.