fix(iii-worker): validate worker config and fix CLI help text#1450
Merged
fix(iii-worker): validate worker config and fix CLI help text#1450
Conversation
Fix two DX bugs:
MOT-3002: Change clap display name from "iii-worker" to "iii worker"
so help text no longer shows the hyphenated binary name.
MOT-3021: Validate iii.worker.yaml config before sandbox startup.
Missing package_manager now shows a Tier 1 error with inline YAML fix
instead of a misleading "permission denied" from the sandbox. Also
split pnpm/yarn from npm inference so each uses correct commands
(pnpm install/pnpm exec tsx, yarn install/yarn exec tsx).
Additional hardening:
- Add ("typescript", _) catch-all for unrecognized package managers
- Add ProjectInfo::validate() checking language and run_cmd
- Validate in both start_local_worker() and handle_local_add()
- 6 new tests covering pnpm, yarn, missing PM, unknown PM, language
typos, and empty run_cmd
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated CLI metadata (command/bin name changed to "iii worker"); added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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: 1
🧹 Nitpick comments (1)
crates/iii-worker/src/cli/project.rs (1)
385-450: Add regression coverage for non-TypeScript runtime manifests withoutpackage_manager.The new tests cover TypeScript well; adding python/rust no-
package_managermanifest tests would protect against this behavior drifting again.
🤖 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/project.rs`:
- Around line 146-155: The current check mistakenly requires package_manager
whenever language is set (if !language.is_empty() &&
package_manager.is_empty()), which forces package_manager for non-JS runtimes;
change this to only require package_manager for JS-style runtimes. Replace the
condition with a membership test (e.g., known_js_langs.contains(&language) &&
package_manager.is_empty()) or an explicit equality check for the JS languages
you support (e.g., language == "node" || language == "javascript" || language ==
"typescript"), and keep the existing eprintln block (using manifest_path,
language, entry) only for that JS-specific branch; leave other languages to fall
through or produce a separate unknown-language message.
🪄 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: 6970a5f8-b152-4867-a6a1-fd1d39c3b455
📒 Files selected for processing (3)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/project.rs
Comment on lines
+146
to
+155
| if !language.is_empty() && package_manager.is_empty() { | ||
| eprintln!( | ||
| "{} missing `package_manager` in {}\n\n runtime:\n language: {}\n package_manager: npm <-- add this line\n entry: {}\n\nhint: valid options are: npm, yarn, pnpm, bun", | ||
| "error:".red(), | ||
| manifest_path.display(), | ||
| language, | ||
| entry | ||
| ); | ||
| return None; | ||
| } |
Contributor
There was a problem hiding this comment.
package_manager is being required too broadly here.
At Line 146, !language.is_empty() forces package_manager for all runtime languages. That breaks valid Python/Rust manifests without scripts, and can mask unknown-language errors as “missing package_manager”.
💡 Proposed fix
- if !language.is_empty() && package_manager.is_empty() {
+ if language == "typescript" && package_manager.is_empty() {
eprintln!(
"{} missing `package_manager` in {}\n\n runtime:\n language: {}\n package_manager: npm <-- add this line\n entry: {}\n\nhint: valid options are: npm, yarn, pnpm, bun",
"error:".red(),
manifest_path.display(),
language,
entry
);
return None;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/iii-worker/src/cli/project.rs` around lines 146 - 155, The current
check mistakenly requires package_manager whenever language is set (if
!language.is_empty() && package_manager.is_empty()), which forces
package_manager for non-JS runtimes; change this to only require package_manager
for JS-style runtimes. Replace the condition with a membership test (e.g.,
known_js_langs.contains(&language) && package_manager.is_empty()) or an explicit
equality check for the JS languages you support (e.g., language == "node" ||
language == "javascript" || language == "typescript"), and keep the existing
eprintln block (using manifest_path, language, entry) only for that JS-specific
branch; leave other languages to fall through or produce a separate
unknown-language message.
The clap `name` attribute controls the command identity, but the Usage line derives from argv[0] (the binary name). Adding `bin_name` ensures the usage line also shows "iii worker" instead of "iii-worker".
Contributor
Author
EvidenceMOT-3002 — Help text fixUsage line now shows MOT-3021 — Missing
|
sergiofilhowz
previously approved these changes
Apr 10, 2026
… in CLI definition
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
iii-workerdisplay name in CLI help text → now showsiii workeriii.worker.yamlconfig before sandbox startup — missingpackage_managernow shows a Tier 1 error with inline YAML fix instead of a misleading "permission denied"pnpm install/pnpm exec tsx,yarn install/yarn exec tsx) instead of npm commands("typescript", _)catch-all for unrecognized package managersProjectInfo::validate()with language and run_cmd checks in bothstart_local_worker()andhandle_local_add()Test plan
cargo build -p iii-worker— compiles cleancargo test -p iii-worker --lib -- project— 19 tests pass (6 new)cargo run -p iii-worker -- --help— verify "iii worker" (no hyphen)iii.worker.yamlwithlanguage: typescriptbut nopackage_manager— verify Tier 1 error with YAML fix showniii.worker.yamlwithpackage_manager: deno— verify "unrecognized" errorSummary by CodeRabbit
Bug Fixes
Improvements
Tests