feat(iii-worker): add --force, reinstall, and clear commands#1442
Merged
andersonleal merged 5 commits intomainfrom Apr 9, 2026
Merged
feat(iii-worker): add --force, reinstall, and clear commands#1442andersonleal merged 5 commits intomainfrom
andersonleal merged 5 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughIntroduces a shared Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (user)
participant Main as main.rs
participant Managed as managed.rs
participant FS as File System
CLI->>Main: invoke `iii-worker add|reinstall|clear ...`
Main->>Managed: call handler (handle_managed_add / handle_managed_clear) with args
alt add / reinstall
Managed->>Managed: parse/validate worker name (OCI vs plain)
Managed->>Managed: check running state (is_worker_running)
alt force
Managed->>FS: remove existing artifacts (delete_worker_artifacts)
end
Managed->>FS: pull/cache artifacts (image_cache_dir / OCI pull)
FS-->>Managed: artifact result
else clear
Managed->>Managed: if single -> validate -> confirm_clear (unless --yes)
Managed->>Managed: collect running OCI hashes to protect
Managed->>FS: delete selected artifacts or all
FS-->>Managed: freed size
end
Managed-->>Main: return status code
Main-->>CLI: exit with status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sergiofilhowz
previously approved these changes
Apr 8, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/iii-worker/src/cli/app.rs (1)
31-33: Consider enforcing--reset-configrequires--forceat the clap level.The help text states
--reset-configrequires--forceonadd, but this constraint is enforced at runtime inhandle_managed_addrather than via clap'srequiresattribute. The current approach works correctly, but clap-level validation would provide clearer error messages directly from the argument parser.♻️ Optional: Add clap constraint
/// Reset config: also remove config.yaml entry before re-adding (requires --force on add) - #[arg(long)] + #[arg(long, requires = "force")] pub reset_config: bool,Note: This would require
forceto be part ofAddArgsor use a different approach sinceforceis defined outside the flattened struct. The current runtime enforcement is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/app.rs` around lines 31 - 33, The CLI currently enforces the `--reset-config` + `--force` constraint at runtime in `handle_managed_add`; instead add a clap-level requirement so parsing fails with a clear message: update the `reset_config` arg declaration (the field `reset_config` in the struct) to include a clap `requires = "force"` (or `requires_if` variant) or alternatively move the `force` flag into the same flattened `AddArgs` struct so `requires` can reference it; keep `handle_managed_add` validation as a fallback but rely on clap's `requires` between `reset_config` and `force` for immediate parser-level validation.crates/iii-worker/src/cli/managed.rs (1)
458-461: Inconsistent error handling: deletion failures silently ignored.Lines 459 and 488 use
let _ = std::fs::remove_dir_all(...)which silently discards errors. This is inconsistent withdelete_worker_artifacts(lines 557-575) which emits warnings on failure. Users may be confused if the reported "freed" size doesn't match actual space reclaimed due to permission issues.♻️ Proposed fix: warn on deletion failures
- let _ = std::fs::remove_dir_all(entry.path()); + if let Err(e) = std::fs::remove_dir_all(entry.path()) { + eprintln!( + " {} Failed to remove {}: {}", + "warning:".yellow(), + entry.path().display(), + e + ); + } worker_count += 1;Apply similar change at line 488 for image deletion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/cli/managed.rs` around lines 458 - 461, The deletion calls currently ignore errors (e.g., `let _ = std::fs::remove_dir_all(entry.path())`) causing mismatched freed-size reporting; update these to handle the Result and emit a warning on failure (same style as `delete_worker_artifacts`), e.g., check the Result from `std::fs::remove_dir_all(...)` and call the project's logger (or `warn!`) with context including the path and the error; apply this change at the directory deletion site (where `dir_size(&entry.path())` and `worker_count` are used) and also to the image deletion call mentioned for line ~488 so both deletion paths consistently warn on errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 409-415: The prompt in confirm_clear() may not appear because
eprint! doesn't flush stderr; update confirm_clear to explicitly flush stderr
after the eprint! call (use std::io::stderr().flush() or equivalent) before
calling std::io::stdin().read_line so the user sees "Continue? [y/N]" prior to
input being read; handle any flush error as appropriate (propagate/log or
ignore) to avoid blocking behavior.
- Around line 502-509: The warning message prints the OCI image identifier from
the skipped list as the suggested stop target, which is invalid; update the loop
that iterates over skipped to detect when an entry is an OCI image (e.g.,
matches image/hash pattern) and lookup the corresponding worker name (use or add
a helper like find_running_by_image(image_id) or
get_worker_name_from_image(image_id)) and then print that worker name in the
stop suggestion; keep the existing message for entries that already are worker
names. Ensure you update the loop over skipped to call the lookup helper and
fall back to the original entry if no worker name is found so the suggestion is
always a valid worker name.
---
Nitpick comments:
In `@crates/iii-worker/src/cli/app.rs`:
- Around line 31-33: The CLI currently enforces the `--reset-config` + `--force`
constraint at runtime in `handle_managed_add`; instead add a clap-level
requirement so parsing fails with a clear message: update the `reset_config` arg
declaration (the field `reset_config` in the struct) to include a clap `requires
= "force"` (or `requires_if` variant) or alternatively move the `force` flag
into the same flattened `AddArgs` struct so `requires` can reference it; keep
`handle_managed_add` validation as a fallback but rely on clap's `requires`
between `reset_config` and `force` for immediate parser-level validation.
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 458-461: The deletion calls currently ignore errors (e.g., `let _
= std::fs::remove_dir_all(entry.path())`) causing mismatched freed-size
reporting; update these to handle the Result and emit a warning on failure (same
style as `delete_worker_artifacts`), e.g., check the Result from
`std::fs::remove_dir_all(...)` and call the project's logger (or `warn!`) with
context including the path and the error; apply this change at the directory
deletion site (where `dir_size(&entry.path())` and `worker_count` are used) and
also to the image deletion call mentioned for line ~488 so both deletion paths
consistently warn on errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dba303ae-0470-4942-ad37-ca4ab83619b1
📒 Files selected for processing (4)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/managed.rscrates/iii-worker/src/lib.rscrates/iii-worker/src/main.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/iii-worker/tests/worker_integration.rs (1)
59-64: Add default assertion forreset_configin this Add parsing test.At Line 59-64, this test validates defaults but skips
args.reset_config. Adding it will better lock down the new flattenedAddArgsbehavior.Proposed test tweak
Commands::Add { args, force } => { assert_eq!(args.worker_name, "ghcr.io/iii-hq/node:latest"); assert_eq!(args.runtime, "libkrun"); assert_eq!(args.address, "localhost"); assert_eq!(args.port, DEFAULT_PORT); + assert!(!args.reset_config); assert!(!force); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/tests/worker_integration.rs` around lines 59 - 64, The test for Commands::Add needs an assertion for the new flattened AddArgs.reset_config default; update the Add branch in the worker_integration.rs test (where it checks args.worker_name, args.runtime, args.address, args.port, and force) to also assert the expected default value of args.reset_config (likely false) so the parsing behavior of AddArgs is locked down; modify the assertion block that handles Commands::Add to include assert_eq!(args.reset_config, <expected_default>) referencing the args.reset_config field on the AddArgs instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/iii-worker/tests/worker_integration.rs`:
- Around line 59-64: The test for Commands::Add needs an assertion for the new
flattened AddArgs.reset_config default; update the Add branch in the
worker_integration.rs test (where it checks args.worker_name, args.runtime,
args.address, args.port, and force) to also assert the expected default value of
args.reset_config (likely false) so the parsing behavior of AddArgs is locked
down; modify the assertion block that handles Commands::Add to include
assert_eq!(args.reset_config, <expected_default>) referencing the
args.reset_config field on the AddArgs instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe777b34-8443-4ef3-b147-b84828e81f1b
📒 Files selected for processing (2)
crates/iii-worker/tests/config_file_integration.rscrates/iii-worker/tests/worker_integration.rs
Add CLI commands for re-downloading and cleaning up worker artifacts: - `iii worker add --force` / `-f`: delete local artifacts before re-downloading - `iii worker add --force --reset-config`: also reset config.yaml to defaults - `iii worker reinstall <name>`: alias for `add --force` - `iii worker clear [name] [--yes]`: wipe downloaded binaries and OCI images Implementation details: - Shared `AddArgs` struct via `#[command(flatten)]` for Add/Reinstall - `is_worker_running()` helper guards against clearing active workers - `delete_worker_artifacts()` handles both binary dirs and OCI image dirs - `image_cache_dir()` centralizes SHA-256 hash computation for OCI paths - `clear_all_workers` validates dir names and canonicalizes paths for safety - OCI references bypass worker name validation in --force path - Running OCI workers protected during clear-all via hash set lookup - Confirmation prompt for `clear` without `--yes`; extracted for testability
…signature Update tests to match AddArgs flatten refactor and new force/reset_config parameters on handle_managed_add.
… clear Unit tests (managed.rs): - delete_worker_artifacts: file removal, nested dir removal - is_worker_running: invalid PID content, empty PID file - image_cache_dir: deterministic hash, path structure Integration tests (worker_integration.rs): - CLI parsing for --force, -f, --reset-config, reinstall, clear, --yes/-y Integration tests (config_file_integration.rs): - handle_managed_add with force=true on builtins - handle_managed_add with force+reset_config clears user overrides - handle_managed_add with force preserves config when no reset - handle_managed_clear: single worker, invalid name, clear-all
andersonleal
force-pushed
the
feat/worker-clean-install
branch
from
f90d6ba to
459414e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/iii-worker/src/main.rs (1)
22-42: Deduplicate the forced add path and keep registry reuse in one place.
add --forceandreinstallnow own the same per-worker loop. That makes the two entrypoints easy to drift apart, and both currently bypass the dedicated multi-worker path by callinghandle_managed_add(..., None, ...)for every worker. I'd pull this into one helper so behavior stays aligned, and so a sharedcached_registrycan be threaded through if that parameter is meant to amortize repeated lookups.Also applies to: 47-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/iii-worker/src/main.rs` around lines 22 - 42, The forced per-worker loop in the Commands::Add branch duplicates logic used by the reinstall path and bypasses handle_managed_add_many; refactor by extracting a single helper (e.g., managed_add_many_with_options) that takes &Vec<String>, force flag, reset_config, and an optional cached_registry and implements the per-worker loop, calling iii_worker::cli::managed::handle_managed_add for each name; update both the Add --force branch and the reinstall entrypoint to call this helper so behavior stays aligned and the shared cached_registry is created once and threaded through to amortize lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/iii-worker/src/main.rs`:
- Around line 22-42: The forced per-worker loop in the Commands::Add branch
duplicates logic used by the reinstall path and bypasses
handle_managed_add_many; refactor by extracting a single helper (e.g.,
managed_add_many_with_options) that takes &Vec<String>, force flag,
reset_config, and an optional cached_registry and implements the per-worker
loop, calling iii_worker::cli::managed::handle_managed_add for each name; update
both the Add --force branch and the reinstall entrypoint to call this helper so
behavior stays aligned and the shared cached_registry is created once and
threaded through to amortize lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48648c91-057b-4a39-9fc4-ef7b0aa24d81
📒 Files selected for processing (6)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/managed.rscrates/iii-worker/src/lib.rscrates/iii-worker/src/main.rscrates/iii-worker/tests/config_file_integration.rscrates/iii-worker/tests/worker_integration.rs
✅ Files skipped from review due to trivial changes (1)
- crates/iii-worker/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/iii-worker/tests/config_file_integration.rs
- crates/iii-worker/tests/worker_integration.rs
- crates/iii-worker/src/cli/managed.rs
- crates/iii-worker/src/cli/app.rs
This was referenced Apr 9, 2026
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
2 participants
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
--force/-fflag toiii worker addto re-download worker artifacts (binary or OCI)--reset-configflag to also reset config.yaml entry to registry defaults (requires--force)iii worker reinstallas convenience alias foradd --forceiii worker clear [name] [--yes]to wipe downloaded artifacts from~/.iii/New CLI surface
Safety guards
clearwithout a name requires confirmation prompt (skip with--yes)~/.iii/Design
AddArgsstruct via#[command(flatten)]keeps Add/Reinstall in syncimage_cache_dir()centralizes OCI SHA-256 hash path computation (was duplicated 3x)handle_managed_clearis synchronous (no fake async)confirm_clear()extracted for testabilityTest plan
cargo test -p iii-worker --lib)iii worker add --helpshows--forceand--reset-configiii worker reinstall --helpshows shared args via flatteniii worker clear --helpshows[WORKER]and--yesiii worker clear --yeswith no artifacts prints "Nothing to clear."--reset-configrequires--force(clap validation)iii worker add pdfkit --forcere-downloads binaryiii worker reinstall pdfkitequivalent behavioriii worker clear pdfkitremoves artifacts, keeps configSummary by CodeRabbit
New Features
addgains--force/-fto force re-downloads and--reset-configto restore defaults.reinstallcommand that mirrorsadd --forceand supports--reset-config.clearcommand to remove local artifacts (all or single worker) with--yes/-yto skip confirmation.Tests