Skip to content

refactor: harden native CLI and workflows - #453

Closed
keys-i wants to merge 4 commits into
sdkman:masterfrom
keys-i:rust-modules
Closed

keys-i wants to merge 4 commits into
sdkman:masterfrom
keys-i:rust-modules

Conversation

@keys-i

@keys-i keys-i commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #454, #455

Summary

  • Flatten Rust binaries from src/bin/<command>/main.rs to src/bin/<command>.rs.
  • Consolidate GitHub Actions and add standalone formatting, linting, and testing checks with a ✅/❌ summary table.
  • Harden path validation, uninstall safety, default-version recovery, current-version detection, help aliases, and version error handling.
  • Parameterize tests and reduce runtime dependencies from 13 to 6.

Verification

  • cargo fmt --all -- --check
  • cargo clippy --locked --all-targets --all-features -- -D warnings
  • cargo test --locked — 50 passed
  • actionlint .github/workflows/*.yml
  • git diff --check

Notes

  • Binary flattening preserves Cargo’s existing command behavior.
  • Destructive path, symlink escape, rollback, and stale-backup cases are covered.

keys-i added 4 commits August 24, 2026 08:55
Signed-off-by: Keys <70819367+keys-i@users.noreply.github.com>
Signed-off-by: Keys <70819367+keys-i@users.noreply.github.com>
also added in a checks to fmt + clippy + test it

Signed-off-by: Keys <70819367+keys-i@users.noreply.github.com>
Signed-off-by: Keys <70819367+keys-i@users.noreply.github.com>
@keys-i keys-i changed the title Rust modules refactor: harden native CLI and workflows Aug 30, 2026
@chloe41427

chloe41427 Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Review: LOW risk, approve after one thing is fixed.

  • The pull_request_target security fix is correct — untrusted fork code no longer runs with secret access, and it now actually checks out the PR's own code (fixing the CI: Consolidate GitHub Actions workflows #454 bug where it validated master instead).
  • Rust hardening (path traversal/symlink validation, active-version guard on uninstall, default rollback-on-failure) is solid with strong adversarial test coverage.
  • ⚠️ Blocker before merge: the new CI (Checks, Pull Requests) is stuck on action_required — it's never actually run against this PR's code. The only green check is the old workflow being replaced, which validated the wrong branch. Needs "Approve and run" + confirm green before merging.
  • Minor: failure-alert scope narrowed from all workflows to 4 named ones — worth a one-line confirmation from the author that's intentional.

One meta note for future PRs: could we lean toward smaller, single-purpose PRs going forward? This one bundles a security fix, Rust hardening, and a CI refactor, which makes it harder to review each change on its merits and to bisect later if something regresses. Not a blocker here — just a preference for splitting things up next time. Thanks!

@marc0der

Copy link
Copy Markdown
Member

Actually, I'll veto this one. It's a massive PR that touches way too much. I want small, fine grained PRs with well expressed intent. It makes reviewing easier and lowers the risk unwanted things creeping in.

@marc0der marc0der closed this Aug 30, 2026
@marc0der

Copy link
Copy Markdown
Member

@keys-i I'd love to incorporate your workflow changes and file layout stuff. We never discussed broad changes to the code. Let's take these improvements one step at a time please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: Consolidate GitHub Actions workflows

2 participants