Fix all Hawk visibility findings - #432
Open
pesap wants to merge 6 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens Rust visibility across Torc’s server/client code to satisfy Hawk’s public-API checks, and wires Hawk into CI (with a pinned Rust toolchain) so visibility regressions block merges.
Changes:
- Reduce
pubvisibility topub(crate)/private for internal APIs, helpers, and struct fields. - Add
hawk.tomland runcargo hawk checkas a blocking CI lint (Rust 1.97.1). - Update contributor docs and CI toolchain pinning to match the Hawk requirement.
Reviewed changes
Copilot reviewed 117 out of 117 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tui.rs | Restrict TUI entrypoint visibility |
| src/tui_runner.rs | Restrict clap arg field visibility |
| src/server/transport_types/context_types.rs | Narrow error/span ID visibility |
| src/server/transport_types/auth_types.rs | Narrow auth types/helpers visibility |
| src/server/service.rs | Make service helpers internal |
| src/server/response_types.rs | Make response re-export modules internal |
| src/server/live_state.rs | Restrict server state fields/constructors |
| src/server/http_transport/path_parsing.rs | Restrict test helpers visibility |
| src/server/http_server/lifecycle_support.rs | Restrict internal lifecycle helper |
| src/server/http_server/bootstrap.rs | Restrict bootstrap helper visibility |
| src/server/http_server.rs | Restrict server internals/constructors |
| src/server/htpasswd.rs | Restrict htpasswd helpers visibility |
| src/server/event_broadcast.rs | Restrict event fields and broadcaster API |
| src/server/dashboard.rs | Restrict dashboard handler visibility |
| src/server/credential_cache.rs | Restrict credential cache API visibility |
| src/server/context.rs | Restrict context wrapper constructors |
| src/server/authorization.rs | Restrict authorization service API visibility |
| src/server/auth.rs | Restrict auth structs/constructors visibility |
| src/server/api/workflows.rs | Restrict API impl context/constructors |
| src/server/api/workflow_actions.rs | Restrict API impl context/constructors |
| src/server/api/user_data.rs | Restrict API impl context/constructors |
| src/server/api/sql_query_builder.rs | Restrict SQL builder visibility |
| src/server/api/slurm_stats.rs | Restrict API impl constructor visibility |
| src/server/api/schedulers.rs | Restrict API impl context/constructors |
| src/server/api/ro_crate.rs | Restrict RO-crate API internals visibility |
| src/server/api/results.rs | Restrict API impl context/constructors |
| src/server/api/resource_requirements.rs | Restrict API impl context/constructors |
| src/server/api/remote_workers.rs | Restrict API impl context/constructors |
| src/server/api/jobs.rs | Restrict job API internals visibility |
| src/server/api/files.rs | Restrict API impl context/constructors |
| src/server/api/failure_handlers.rs | Restrict API impl context/constructors |
| src/server/api/events.rs | Restrict API impl context/constructors |
| src/server/api/compute_nodes.rs | Restrict API impl context/constructors |
| src/server/api/admin.rs | Restrict admin helpers and structs visibility |
| src/server/api/access_groups.rs | Restrict access-group API internals |
| src/server/api.rs | Restrict shared server API utilities |
| src/server/api_stats.rs | Restrict stats internals/constructors |
| src/server/api_event_stream.rs | Restrict event-stream types/helpers |
| src/server/api_contract.rs | Make internal contracts private |
| src/server/api_constants.rs | Restrict API constants visibility |
| src/server.rs | Restrict internal server modules |
| src/run_jobs_cmd.rs | Restrict worker log helpers visibility |
| src/ro_crate_json_ld.rs | Restrict RO-crate JSON-LD helper |
| src/plot_resources_cmd.rs | Restrict plot command arg visibility |
| src/openapi_spec.rs | Restrict OpenAPI types/helpers visibility |
| src/network_utils.rs | Restrict network helper constant visibility |
| src/memory_utils.rs | Restrict memory helper visibility |
| src/lib.rs | Add crate-level dead_code allow; adjust exports |
| src/config/loader.rs | Restrict config path helper visibility |
| src/config/client.rs | Restrict client config field visibility |
| src/client/workflow_manager.rs | Restrict workflow manager helpers visibility |
| src/client/workflow_graph.rs | Restrict graph internals visibility |
| src/client/version_check.rs | Restrict version-check helpers/constants |
| src/client/utils.rs | Restrict client utility helpers visibility |
| src/client/sse_client.rs | Restrict SSE types/helpers visibility |
| src/client/scheduler_plan.rs | Restrict scheduler plan types/helpers |
| src/client/ro_crate_utils.rs | Restrict RO-crate client helpers |
| src/client/resource_monitor.rs | Restrict resource monitor API visibility |
| src/client/resource_correction.rs | Restrict correction report fields/helpers |
| src/client/report_models.rs | Restrict report model fields visibility |
| src/client/remote/worker_file.rs | Restrict worker file parsing visibility |
| src/client/remote/types.rs | Restrict remote worker types/helpers |
| src/client/remote/ssh.rs | Restrict SSH helper visibility |
| src/client/remote/shell.rs | Restrict remote shell helpers visibility |
| src/client/remote.rs | Narrow remote module re-export surface |
| src/client/parameter_expansion.rs | Restrict parameter expansion API visibility |
| src/client/offline_journal.rs | Restrict offline journal API visibility |
| src/client/log_paths.rs | Restrict log path helpers visibility |
| src/client/job_runner.rs | Restrict runner helper visibility |
| src/client/hpc/slurm.rs | Restrict Slurm HPC helpers visibility |
| src/client/hpc/profiles.rs | Restrict HPC profile helpers visibility |
| src/client/hpc/hpc_manager.rs | Restrict HPC manager visibility |
| src/client/hpc/dane.rs | Restrict Dane profile visibility |
| src/client/hpc/common.rs | Restrict HPC common helpers visibility |
| src/client/hpc.rs | Restrict HPC module surface/re-exports |
| src/client/execution_plan.rs | Restrict execution plan internals |
| src/client/commands/workflows.rs | Restrict internal command handlers |
| src/client/commands/workflow_export.rs | Restrict export/import helper visibility |
| src/client/commands/watch.rs | Remove backward-compat re-export |
| src/client/commands/table_format.rs | Restrict table helpers visibility |
| src/client/commands/self_update.rs | Restrict self-update args visibility |
| src/client/commands/reports.rs | Restrict report builders/entrypoints |
| src/client/commands/recover.rs | Restrict recovery helpers/fields visibility |
| src/client/commands/pagination/workflows.rs | Restrict pagination builder surface |
| src/client/commands/pagination/user_data.rs | Restrict pagination builder surface |
| src/client/commands/pagination/slurm_schedulers.rs | Restrict pagination builder surface |
| src/client/commands/pagination/scheduled_compute_nodes.rs | Restrict pagination builder surface |
| src/client/commands/pagination/ro_crate_entities.rs | Restrict pagination builder surface |
| src/client/commands/pagination/results.rs | Restrict pagination builder surface |
| src/client/commands/pagination/resource_requirements.rs | Restrict pagination builder surface |
| src/client/commands/pagination/jobs.rs | Restrict pagination builder surface |
| src/client/commands/pagination/job_user_data_relationships.rs | Restrict pagination builder surface |
| src/client/commands/pagination/job_file_relationships.rs | Restrict pagination builder surface |
| src/client/commands/pagination/job_dependencies.rs | Restrict pagination builder surface |
| src/client/commands/pagination/files.rs | Restrict pagination builder surface |
| src/client/commands/pagination/events.rs | Restrict pagination builder surface |
| src/client/commands/pagination/compute_nodes.rs | Restrict pagination builder surface |
| src/client/commands/pagination/base.rs | Restrict pagination core API visibility |
| src/client/commands/output.rs | Restrict JSON output helpers visibility |
| src/client/commands/orphan_detection.rs | Restrict orphan detection types/helpers |
| src/client/commands/logs.rs | Restrict log analysis fields visibility |
| src/client/commands/jobs.rs | Restrict internal job command helpers |
| src/client/commands/hpc.rs | Restrict HPC command helper visibility |
| src/client/commands/diagnose.rs | Restrict diagnose entrypoint visibility |
| src/client/commands.rs | Restrict command helper visibility |
| src/client/async_cli_command.rs | Restrict async command helpers visibility |
| src/client.rs | Restrict client module surface/re-exports |
| hawk.toml | Define production binaries for Hawk |
| docs/src/contributing.md | Document Hawk install / pinned toolchain |
| .github/workflows/lint.yml | Pin Rust toolchain; add Hawk CI check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
daniel-thom
reviewed
Aug 11, 2026
| /// Helper function to collect all paginated results into a Vec. | ||
| /// | ||
| /// This is a convenience function for when you need all results at once. | ||
| fn paginate<T: Paginatable>( |
Collaborator
There was a problem hiding this comment.
If this is dead code, I'm wondering if other modules also implemented this functionality.
Collaborator
Author
There was a problem hiding this comment.
Agreed. The helper had no direct callers because each resource specific pagination function collects its iterator directly. That duplication does not make the shared abstraction obsolete. I restored it in 175fbf0 and will treat any pagination consolidation as separately scoped cleanup.
daniel-thom
reviewed
Aug 11, 2026
| } | ||
|
|
||
| /// Get all group IDs that a user belongs to | ||
| async fn get_user_group_ids(&self, user_name: &str) -> Result<Vec<i64>, ApiError> { |
Collaborator
There was a problem hiding this comment.
Rather than delete this, perhaps there should be a client-side command to call it.
Collaborator
Author
There was a problem hiding this comment.
Agreed. torc access-groups list-user-groups already exposes the detailed group records for a user, but this raw ID helper remains a valid lower level building block for client side authorization features. I restored it in 175fbf0 rather than infer that it is obsolete. Any CLI expansion or API consolidation can be considered separately.
Collaborator
|
@pesap I have an overall sense that this is deleting potentially useful code. The problem may be that no client-side CLI command is invoking the code. I'll review in more detail later this week. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 118 out of 119 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/client/hpc/slurm.rs:1
- QueueDepthInfo no longer carries the partition identifier, but parse_queue_depth still aggregates per-partition and then returns a Vec. Once collected, callers can no longer determine which partition each queue-depth entry refers to, which is a functional regression. Re-introduce a
partition: Stringfield (it can bepub(crate)if needed) and populate it when inserting into the map, or alternatively return a keyed structure (e.g., Vec<(String, QueueDepthInfo)> or a map) so the partition name is preserved.
src/client/hpc/slurm.rs:1 - QueueDepthInfo no longer carries the partition identifier, but parse_queue_depth still aggregates per-partition and then returns a Vec. Once collected, callers can no longer determine which partition each queue-depth entry refers to, which is a functional regression. Re-introduce a
partition: Stringfield (it can bepub(crate)if needed) and populate it when inserting into the map, or alternatively return a keyed structure (e.g., Vec<(String, QueueDepthInfo)> or a map) so the partition name is preserved.
src/client.rs:52 - This PR removes multiple previously publicly re-exported types/functions from the crate’s public API (e.g.,
HpcManager,HpcType,create_hpc_interface, several report model types, and version helpers). That is a semver-breaking change for downstream crates usingtorcas a library. If the intent is to keep these as supported APIs, consider restoring the exports and usinghawk.tomloverrides for them; otherwise, document the breaking change and ensure the next release aligns with your versioning policy (e.g., major bump or explicit deprecation window).
pub use hpc::{
HpcDetection, HpcInterface, HpcJobInfo, HpcJobStats, HpcJobStatus, HpcPartition, HpcProfile,
HpcProfileRegistry, SlurmInterface,
};
src/client.rs:65
- This PR removes multiple previously publicly re-exported types/functions from the crate’s public API (e.g.,
HpcManager,HpcType,create_hpc_interface, several report model types, and version helpers). That is a semver-breaking change for downstream crates usingtorcas a library. If the intent is to keep these as supported APIs, consider restoring the exports and usinghawk.tomloverrides for them; otherwise, document the breaking change and ensure the next release aligns with your versioning policy (e.g., major bump or explicit deprecation window).
pub use report_models::{ResourceUtilizationReport, ResourceViolation};
// Version checking utilities
pub use version_check::{ServerInfo, VersionCheckResult, VersionMismatchSeverity, check_version};
.github/workflows/lint.yml:66
- The CI workflow installs Hawk via
curl ... | shwithout integrity verification, which is a supply-chain risk in a privileged CI context. Prefer downloading a versioned release artifact plus checksum/signature verification, or using a dedicated GitHub Action/release asset with pinned digest; at minimum, validate a published SHA256 before executing.
- name: Install Hawk
run: |
curl --proto '=https' --tlsv1.2 -LsSf \
https://github.com/astral-sh/hawk/releases/download/0.1.10/cargo-hawk-installer.sh | sh
echo "$HOME/.cargo/bin" >> "$GITHUB_PATH"
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
Thanks, I agree. I overreached by treating the absence of direct callers as evidence that code should be removed. I reverted the broad cleanup in 175fbf0. The PR is again limited to visibility tightening, Hawk configuration, and CI stability. Existing and potentially planned capabilities are preserved. Any removal should be handled in a separate, narrowly scoped change with an inventory, rationale, replacement where needed, and explicit review of the client and library API impact. |
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
Validation
cargo +1.97.1 clippy --all --all-targets --all-features -- -D warningscargo +1.97.1 test --all-featurescargo +1.97.1 hawk check -D warningscargo fmt -- --checkdprint check --excludes queries.mdThe unrelated pre-existing untracked
queries.mdfile is intentionally not included.