fix: dx issues#1970
Open
sergiofilhowz wants to merge 3 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds starter config creation, configurable worker config paths, bare worker entries, remote ChangesConfig and worker lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WorkerCLI
participant remote_ops
participant iii_worker_ops
participant EngineConfig
WorkerCLI->>remote_ops: Normalize --host and build AddOptions
remote_ops->>remote_ops: Probe engine authority
remote_ops->>iii_worker_ops: Trigger worker::add
iii_worker_ops->>EngineConfig: Update engine project config
EngineConfig-->>remote_ops: Return AddOutcome
remote_ops-->>WorkerCLI: Report result and exit code
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Contributor
skill-check — docs5 verified, 689 skipped. 1 warning across the verified workers.
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 4
🧹 Nitpick comments (1)
crates/iii-worker/src/cli/managed.rs (1)
188-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent hardcoded
config.yamlin CLI output.This PR introduces support for dynamic config paths via
III_CONFIG_PATHand correctly updates thehandle_managed_addbuilt-in flow to outputsuper::config_file::config_display_name(). However, other add handlers and error messages still hardcode"config.yaml". To prevent user confusion when a custom config path is used, consider replacing these hardcoded strings with the dynamic display name.
crates/iii-worker/src/cli/managed.rs#L188-L194: updatehandle_binary_addto usesuper::config_file::config_display_name().dimmed().crates/iii-worker/src/cli/managed.rs#L1990-L1996: updatehandle_oci_pull_and_addto usesuper::config_file::config_display_name().dimmed().crates/iii-worker/src/cli/local_worker.rs#L724-L729: update the error message inhandle_local_addto dynamically reference the config display name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/iii-worker/src/cli/managed.rs` around lines 188 - 194, The CLI still hardcodes config.yaml in multiple add flows. Update handle_binary_add in crates/iii-worker/src/cli/managed.rs at lines 188-194 and handle_oci_pull_and_add in crates/iii-worker/src/cli/managed.rs at lines 1990-1996 to use super::config_file::config_display_name().dimmed(), and update the handle_local_add error message in crates/iii-worker/src/cli/local_worker.rs at lines 724-729 to reference the same dynamic display name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/iii-worker/src/cli/app.rs`:
- Around line 22-25: Update the shared --reset-config help text in
crates/iii-worker/src/cli/app.rs (lines 22-25) to use neutral wording that
states add requires --force while reinstall enables force automatically.
Regenerate the reinstall-specific documentation in
docs/next/cli-reference/index.mdx (line 268), removing the stale “registry
defaults” wording from its introduction.
In `@crates/iii-worker/src/cli/remote_ops.rs`:
- Around line 21-32: Update host_to_ws_url in
crates/iii-worker/src/cli/remote_ops.rs to add the default engine port 49134 to
full ws:// and wss:// URLs that lack an explicit port, while preserving URLs
with specified ports and updating the pass-through test. In
docs/next/cli-reference/index.mdx at lines 167 and 269, retain or align the
default-port and reinstall documentation with this normalized behavior.
In `@crates/iii-worker/src/main.rs`:
- Around line 687-698: Update remote_source_for_cli so --host does not convert
client-local WorkerSource::Local paths into absolute paths and send them to the
engine. Reject local sources for remote adds, or explicitly treat them as
engine-host paths; preserve supported non-local source handling and ensure
remote installation cannot dereference a client-machine path.
In `@docs/next/using-iii/engine.mdx`:
- Around line 95-96: Update the documentation sentence near the configuration
worker guidance so `config:` blocks are described only as first-boot or
reset-time seeds, not runtime customization. Direct users to the configuration
worker and persisted per-worker configuration for changes after boot, while
preserving the existing runtime customization examples for ports, adapters, and
credentials.
---
Nitpick comments:
In `@crates/iii-worker/src/cli/managed.rs`:
- Around line 188-194: The CLI still hardcodes config.yaml in multiple add
flows. Update handle_binary_add in crates/iii-worker/src/cli/managed.rs at lines
188-194 and handle_oci_pull_and_add in crates/iii-worker/src/cli/managed.rs at
lines 1990-1996 to use super::config_file::config_display_name().dimmed(), and
update the handle_local_add error message in
crates/iii-worker/src/cli/local_worker.rs at lines 724-729 to reference the same
dynamic display name.
🪄 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: 7894ad69-929a-4255-b87c-cfa794006783
📒 Files selected for processing (30)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/config_file.rscrates/iii-worker/src/cli/local_worker.rscrates/iii-worker/src/cli/managed.rscrates/iii-worker/src/cli/mod.rscrates/iii-worker/src/cli/remote_ops.rscrates/iii-worker/src/core/project.rscrates/iii-worker/src/main.rscrates/iii-worker/tests/config_force_integration.rscrates/iii-worker/tests/config_managed_integration.rsdocs/next/cli-reference/index.mdxdocs/next/cli-reference/index.mdx.skill.mddocs/next/tutorials/incremental-adoption/overview.mdxdocs/next/tutorials/incremental-adoption/overview.mdx.skill.mddocs/next/using-iii/cli.mdxdocs/next/using-iii/cli.mdx.skill.mddocs/next/using-iii/engine.mdxdocs/next/using-iii/engine.mdx.skill.mddocs/next/using-iii/workers.mdxdocs/next/using-iii/workers.mdx.skill.mdengine/src/cli/exec.rsengine/src/cli/mod.rsengine/src/engine/mod.rsengine/src/main.rsengine/src/workers/config.rsengine/src/workers/external.rsengine/src/workers/registry_worker.rsengine/src/workers/reload.rsengine/tests/cli_args.rsengine/tests/fixtures/argv_probe.sh
- cli_integration: check for a 'create' SUBCOMMAND line instead of a raw substring — the --config help legitimately says iii offers to create the missing file (mirrors the existing 'start' check) - --reset-config help: neutral wording valid for both add (requires --force) and reinstall (forces automatically) - --host help: document that ws:// and wss:// URLs are used as-is and that local worker paths resolve on the engine host - remote adds: absolutize local paths against the CLI cwd only for loopback hosts; non-loopback targets get the path verbatim per the WorkerSource::Local engine-host contract, plus a stderr note - engine.mdx: describe config: blocks as first-boot seeds, not runtime customization (matches the configuration worker lifecycle) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/next/cli-reference/index.mdx.skill.md`:
- Line 165: Normalize the duplicated engine-directory wording in the host-option
descriptions: update the iii worker add entry at
docs/next/cli-reference/index.mdx.skill.md lines 165-165 and the iii worker
reinstall entry at lines 267-267 by replacing “ITS project directory” with “its
project directory” or “the engine's project directory.”
🪄 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: 671f4244-ac1d-49fe-a02d-df3259896a84
📒 Files selected for processing (8)
crates/iii-worker/src/cli/app.rscrates/iii-worker/src/cli/remote_ops.rscrates/iii-worker/src/main.rsdocs/next/cli-reference/index.mdxdocs/next/cli-reference/index.mdx.skill.mddocs/next/using-iii/engine.mdxdocs/next/using-iii/engine.mdx.skill.mdengine/tests/cli_integration.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/iii-worker/src/cli/app.rs
- docs/next/using-iii/engine.mdx.skill.md
- docs/next/using-iii/engine.mdx
- crates/iii-worker/src/cli/remote_ops.rs
- crates/iii-worker/src/main.rs
| | ------ | ----------- | | ||
| | `--reset-config` | Discard the worker's config.yaml entry and recreate it from registry defaults. Plain `add --force` would otherwise keep the entry. Only takes effect together with `--force` on add | | ||
| | `--reset-config` | Discard the worker's config.yaml entry and recreate it fresh (dropping any hand-written config block), instead of keeping the existing entry. With `add`, takes effect only together with `--force`; `reinstall` applies force automatically | | ||
| | `--host <HOST[:PORT]>` | Install through a RUNNING iii engine instead of editing the config file in the current directory: connects to HOST[:PORT] (ex. `localhost:49134`; port defaults to 49134; ws:// and wss:// URLs are also accepted and used as-is) and invokes its worker::add. The engine applies the add in ITS project directory, so this works from any folder and with engines on non-default ports. Local worker PATHs resolve on the engine host | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Normalize the duplicated engine-directory wording. Replace ITS project directory with its project directory or the engine's project directory in both host-option descriptions.
docs/next/cli-reference/index.mdx.skill.md#L165-L165: update theiii worker adddescription.docs/next/cli-reference/index.mdx.skill.md#L267-L267: apply the same correction toiii worker reinstall.
🧰 Tools
🪛 LanguageTool
[grammar] ~165-~165: Use a hyphen to join words.
Context: ... worker::add. The engine applies the add in ITS project directory, so this works ...
(QB_NEW_EN_HYPHEN)
📍 Affects 1 file
docs/next/cli-reference/index.mdx.skill.md#L165-L165(this comment)docs/next/cli-reference/index.mdx.skill.md#L267-L267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/next/cli-reference/index.mdx.skill.md` at line 165, Normalize the
duplicated engine-directory wording in the host-option descriptions: update the
iii worker add entry at docs/next/cli-reference/index.mdx.skill.md lines 165-165
and the iii worker reinstall entry at lines 267-267 by replacing “ITS project
directory” with “its project directory” or “the engine's project directory.”
Source: Linters/SAST tools
The skill-check workflow's vale layer rejects em dashes and 'just' as a hedge; rewrite the sentences this PR added with commas, parentheses, semicolons, and colons instead. Skill artifacts re-rendered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
guibeira
approved these changes
Jul 16, 2026
| } | ||
| }; | ||
|
|
||
| let spinner = super::spinner::Spinner::start(format!("Installing {label} via {url}...")); |
Contributor
There was a problem hiding this comment.
Nice touch including this spinner
| if info.accepts_engine_flag { | ||
| info.extra_args.push("--engine".to_string()); | ||
| info.extra_args | ||
| .push(format!("ws://127.0.0.1:{}", engine.worker_manager_port())); |
Contributor
There was a problem hiding this comment.
Could this hard code be a problem? If we have this config:
config:
host: 10.0.0.5
port: 5000
It's going to convert into ws://127.0.0.1:5000 instead ws://10.0.0.5:5000
| /// | ||
| /// Missing file: on an interactive terminal, ask before writing (running | ||
| /// `iii` in the wrong directory shouldn't silently litter a config.yaml | ||
| /// there); headless sessions (containers, CI, service managers) create it |
Contributor
There was a problem hiding this comment.
For a headless session, I don't think is good to auto create the file, since those environments are more used for deployment, create new file will potencially fail silently.
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.
Problem
Several DX issues around
iiistartup andiii worker addmade the worker workflow fragile unless everything ran from one directory, with the default file name, on the default port:iii worker addeditsconfig.yamlin the CLI's cwd. Run from any other folder it wrote to a file no engine was watching, then hung for 120s waiting for a worker that would never boot.config.yaml— engines started with--config config.yml(or any custom path) silently split into two files.addcopied each worker's defaultconfig:block intoconfig.yaml. Workers are configured live through the configuration worker, so those blocks only took effect after a full engine restart (legacy first-boot seeding).iii-worker-opsdaemon hardcodedws://127.0.0.1:49134; engines on any other port got a daemon stuck in a reconnect loop spamming connectivity warnings.iiirefused to start without aconfig.yaml, and the escape hatch--use-default-configran file-less — which broke the config setup (worker add, reload watcher) entirely.Solution
iiiruns without a pre-existing config file. On a TTY it asks before creating (No config.yaml found in <dir>. Create it and start the engine? [Y/n]); non-interactive sessions (containers, CI) create it without asking. Works for--config <custom-name>too. The created file is deliberately minimal — no pre-listed workers, you opt in withiii worker add:--use-default-configis removed (breaking). The created starter file replaces it, keeping the reload watcher and worker ops functional.iii worker add --host <host[:port]>(andreinstall) installs through a running engine by invoking itsworker::addtrigger — the engine applies the change in its project directory, so the command works from any folder, machine, or port (iii worker add pdfkit --host localhost:49134). Unreachable hosts fail fast instead of hanging; the local-path timeout message now points at--host.III_CONFIG_PATHhandshake: the engine exports its absolutized config path to every process it spawns (worker-ops daemon, sandbox daemon,iii worker startchildren, andiii --config x.yml worker …dispatch). All worker-op reads/writes resolve through it, so custom config names work end to end.Correct engine URL for daemons: the engine appends
--engine ws://127.0.0.1:<actual-port>when spawningiii-worker-ops/iii-sandbox, ending the 49134 reconnect spam on non-default ports.addwrites bare- name:entries — noconfig:blocks (registry/binary/OCI/local/builtin alike). Hand-written blocks are preserved on re-add. Exception:iii-sandboxkeeps its block, since its daemon is fail-closed on it and the image allowlist is an operator security surface.Verification
Live e2e: engine started with
--config config.ymlon port 49177;iii worker add iii-queue --host localhost:49177from an unrelated directory appended a bare entry to the engine'sconfig.yml, the engine hot-reloaded and registered the worker, and nothing in the CLI's cwd was touched. Fresh-dir boot auto-created the empty starter file and a follow-upiii worker add iii-statepopulated it live. Docs + generated CLI reference updated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--hostforiii worker add/reinstallto install via a running engine (supportsws:///wss://and custom ports).--configandIII_CONFIG_PATH.workers: []list.Changes
- name:entries (using built-in defaults unless a seeded per-workerconfig:exists).--reset-confignow recreates the config entry fresh (dropping hand-written blocks) and applies with--force, with updated messaging/timeouts.Documentation