NVIDIA / NVIDIA/OpenShell

chore: refactor remaining too_many_arguments functions to config structs

Open
#2,003 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

area:cli area:gateway area:supervisor spike state:stale
Dominant language
Rust
Stars
8.7k
Forks
1.3k
Avg merge
2d 11h
Merged PRs (30d)
253

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the Tier 1 entry point in crates/openshell-cli/src/run.rs, using PR #1997 and SandboxCreateConfig as the pattern; gateway_add is the first recommended target. Update its call sites, including crates/openshell-cli/tests/, then run cargo test -p openshell-cli and mise run pre-commit. Done means the selected suppression is removed while existing behavior and tests remain intact.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, cli, developer-experience
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.