Repository navigation
Conversation
📝 WalkthroughWalkthroughChangesRegistry binary discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
51aed12 to
42c2c32
Compare
jdx
changed the title
Instruction counts
No instruction-count regression above 1%. Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
Greptile SummaryAdds a sandboxed maintainer utility for discovering executable names supplied by registry tools, together with contributor documentation and focused unit tests.
Confidence Score: 4/5The PR should not merge until the discovery image either uses a Linux binary matching the selected platform or rejects incompatible host builds. The documented default copies a host-native debug executable unchanged into a Linux/amd64 image, so maintainers on macOS, Windows, or ARM64 Linux cannot run the new discovery workflow successfully. Files Needing Attention: xtasks/discover_registry_bins.py and docs/contributing.md Important Files Changed
Reviews (1): Last reviewed commit: "chore(registry): add sandboxed bin disco..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| parser.add_argument("--mise-bin", type=Path, default=Path("target/debug/mise")) | ||
| parser.add_argument("--platform", default="linux/amd64") |
There was a problem hiding this comment.
When a maintainer runs the documented command on macOS, Windows, or ARM64 Linux, the script copies the host-native target/debug/mise into a Linux/amd64 image, causing registry listing or discovery containers to fail with an executable-format error.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
xtasks/discover_registry_bins.py (1)
213-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winName the registry-listing container so a timeout can clean it up.
discover_toolpasses--nameand force-removes the container aftersubprocess.TimeoutExpired.list_registry_toolsdoes not. A timeout here kills only the engine client, so the container keeps running and--rmnever triggers.♻️ Proposed cleanup for the listing container
def list_registry_tools(options: SandboxOptions, timeout: int) -> list[str]: - command = container_base_args(options) + [ - "/usr/local/bin/mise", - "registry", - "--json", - "--hide-aliased", - ] - result = subprocess.run(command, capture_output=True, text=True, timeout=timeout, check=False) + container_name = f"mise-bin-discovery-registry-{os.getpid()}" + command = container_base_args(options) + command[-1:-1] = ["--name", container_name] + command.extend(["/usr/local/bin/mise", "registry", "--json", "--hide-aliased"]) + try: + result = subprocess.run( + command, capture_output=True, text=True, timeout=timeout, check=False + ) + except subprocess.TimeoutExpired: + subprocess.run( + [options.engine, "rm", "--force", container_name], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + raise if result.returncode != 0: raise RuntimeError(f"failed to list registry tools: {result.stderr.strip()}")🤖 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 `@xtasks/discover_registry_bins.py` around lines 213 - 224, Update list_registry_tools to assign a unique container name and pass it through the command arguments, matching discover_tool’s --name behavior. Ensure the timeout cleanup path can use that name to force-remove the listing container after subprocess.TimeoutExpired, while preserving the existing registry parsing and error handling.xtasks/test_discover_registry_bins.py (1)
45-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for
classify_failureand the per-tool command assembly.
discover_toolinserts--nameandDISCOVERY_TOOLwithcommand[-1:-1]. That slice must keep the image as the last argument before/bin/sh -c. A regression there is silent.classify_failuremaps marker presence to an error category, and it is untested.🧪 Proposed additional tests
def test_classify_failure_categories(self): tool = discovery.MARKERS["tool"] versions = discovery.MARKERS["versions"] self.assertEqual(discovery.classify_failure(1, "", True), "timeout") self.assertEqual(discovery.classify_failure(None, "", False), "container") self.assertEqual(discovery.classify_failure(1, "noise", False), "install") self.assertEqual(discovery.classify_failure(1, tool, False), "tool_metadata") self.assertEqual( discovery.classify_failure(1, f"{tool}\n{versions}", False), "version_metadata" ) def test_discover_tool_keeps_image_before_entrypoint(self): options = discovery.SandboxOptions( engine="docker", image="test-image", platform="linux/amd64", memory="1g", cpus="1", pids=64, state_size="2g", ) command = discovery.container_base_args(options) command[-1:-1] = ["--name", "n", "--env", "DISCOVERY_TOOL=tool"] self.assertEqual(command[-1], "test-image") self.assertIn("DISCOVERY_TOOL=tool", command)🤖 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 `@xtasks/test_discover_registry_bins.py` around lines 45 - 60, Add focused tests for classify_failure covering timeout, container, install, tool_metadata, and version_metadata outcomes based on exit status and marker presence. Also test discover_tool’s command assembly to verify --name and DISCOVERY_TOOL insertion preserves the image as the final argument before the entrypoint.
🤖 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/contributing.md`:
- Around line 815-833: Update the documentation before the first
discover_registry_bins.py example to include the required cargo build command
that creates target/debug/mise. Revise the shard guidance to state that each
shard writes its own output and that maintainers must combine those outputs
manually, without implying the script provides a merge command.
In `@xtasks/discover_registry_bins.py`:
- Around line 257-264: Update installed_version() to select the record marked as
active when multiple installed records are returned, while preserving the
existing validation for missing or invalid versions. If no active record exists,
fall back to the sole installed record; continue raising the existing error when
neither selection is unambiguous.
---
Nitpick comments:
In `@xtasks/discover_registry_bins.py`:
- Around line 213-224: Update list_registry_tools to assign a unique container
name and pass it through the command arguments, matching discover_tool’s --name
behavior. Ensure the timeout cleanup path can use that name to force-remove the
listing container after subprocess.TimeoutExpired, while preserving the existing
registry parsing and error handling.
In `@xtasks/test_discover_registry_bins.py`:
- Around line 45-60: Add focused tests for classify_failure covering timeout,
container, install, tool_metadata, and version_metadata outcomes based on exit
status and marker presence. Also test discover_tool’s command assembly to verify
--name and DISCOVERY_TOOL insertion preserves the image as the final argument
before the entrypoint.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bf7b44a-88a7-40c0-a696-7b0f05d1c633
📒 Files selected for processing (3)
docs/contributing.mdxtasks/discover_registry_bins.pyxtasks/test_discover_registry_bins.py
| ```sh | ||
| python3 xtasks/discover_registry_bins.py actionlint age | ||
| ``` | ||
|
|
||
| The script builds `target/debug/mise` into a local Docker or Podman image and | ||
| installs each tool in its own disposable, resource-limited container. It does | ||
| not mount host directories or pass host credentials into those containers. | ||
| Successful results include the resolved backend, exact version, and executable | ||
| names reported by `mise bin-paths`; failures retain a per-tool log and are | ||
| retried when the same command is resumed. | ||
|
|
||
| To process the complete registry, use `--all`. Large runs can be split into | ||
| stable shards and merged afterward by running each shard with its own output: | ||
|
|
||
| ```sh | ||
| python3 xtasks/discover_registry_bins.py --all --jobs 8 \ | ||
| --shard-count 4 --shard-index 0 \ | ||
| --output registry-bin-discovery-0.json | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the required build step, and clarify that shard merging is manual.
The script defaults --mise-bin to target/debug/mise and raises "mise binary not found" when that file is absent. Add the build command before the first example. Also state that shard outputs are combined by the maintainer, because the script has no merge command.
📝 Proposed documentation update
Maintainers can collect the executable names installed by registry tools with
the sandboxed discovery script:
```sh
+cargo build
python3 xtasks/discover_registry_bins.py actionlint age@@
-To process the complete registry, use --all. Large runs can be split into
-stable shards and merged afterward by running each shard with its own output:
+To process the complete registry, use --all. Large runs can be split into
+stable shards by running each shard with its own output file. Combine the
+shard outputs manually after all shards finish:
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 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/contributing.md` around lines 815 - 833, Update the documentation before
the first discover_registry_bins.py example to include the required cargo build
command that creates target/debug/mise. Revise the shard guidance to state that
each shard writes its own output and that maintainers must combine those outputs
manually, without implying the script provides a merge command.
| def installed_version(tool: str, versions: Any) -> str: | ||
| records = versions.get(tool) if isinstance(versions, dict) else versions | ||
| if not isinstance(records, list) or len(records) != 1: | ||
| raise ValueError(f"expected one installed version for {tool}") | ||
| version = records[0].get("version") | ||
| if not isinstance(version, str) or not version: | ||
| raise ValueError(f"missing concrete installed version for {tool}") | ||
| return version |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the JSON schemas consumed by the discovery script.
set -eu
# `mise bin-paths --json` entries: does each entry expose `name`?
rg -nP -C5 'bin[_-]paths' --glob '!**/*.lock' src | head -100
# `mise tool --json`: does the payload expose `backend`?
ast-grep run --lang rust --pattern 'struct $NAME { $$$ }' src/cli/tool.rs || true
rg -nP -C3 '\bbackend\b' src/cli/tool.rs | head -40
# `mise ls --json --installed`: record fields (version, active, installed) and dict-vs-array shape.
rg -nP -C4 'active|installed|requested_version' src/cli/ls.rs | head -60
# `mise registry --json --hide-aliased`: does each entry expose `short`?
rg -nP -C4 'short|hide_aliased' src/cli/registry.rs | head -40Repository: jdx/mise
Length of output: 12195
🏁 Script executed:
#!/bin/bash
set -eu
echo "== locate files =="
git ls-files | rg 'xtasks/discover_registry_bins\.py|src/cli/ls|src/cli/tool|src/cli/bin_paths|src/cli/registry' || true
echo "== discover_registry_bins relevant lines =="
sed -n '1,420p' xtasks/discover_registry_bins.py
echo "== ls json structs and renderers =="
rg -n -C6 'struct .*Json|impl .*Json|serde_json|active|installed|version|current_source|requested|ToolVersion' src/cli/ls.rs src/cli/ls_json.rs src/cli/tool.rs src/cli/bin_paths.rs src/cli/registry.rs 2>/dev/null || true
echo "== json model definitions across source =="
rg -n -C5 'struct .*Json|struct .*Output|Serialize|Deserialize|json' src | rg -n -C5 'active|requested|installed|ToolVersion|BinPath|RegistryToolOutput|ToolInfo|output_json' | head -200Repository: jdx/mise
Length of output: 50364
Choose the active installed version instead of rejecting multiple installed records.
mise use --pin "${DISCOVERY_TOOL}@latest" can leave both the version created by test-tool and the pinned version installed. installed_version() currently raises when len(records) != 1, and discover_tool() wraps that in error_category: "output". Select the single active record when present, and fall back to the only installed record.
🤖 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 `@xtasks/discover_registry_bins.py` around lines 257 - 264, Update
installed_version() to select the record marked as active when multiple
installed records are returned, while preserving the existing validation for
missing or invalid versions. If no active record exists, fall back to the sole
installed record; continue raising the existing error when neither selection is
unambiguous.
Summary
mise bin-pathsSandbox model
Each tool runs with a read-only root filesystem, tmpfs-backed mise state, dropped capabilities,
no-new-privileges, and explicit CPU, memory, PID, and timeout limits. No host directory or host credential environment is passed into the tool container. Network access remains enabled because installation requires it.This follows #11666, which added the registry bin metadata consumed by the eventual bulk update.
Validation
python3 -m unittest xtasks/test_discover_registry_bins.pymise run lint-fixactionlint(single bin) andage(four bins)AI-assisted — Tool: Codex; model: OpenAI/GPT-5; version: unavailable.
Summary by CodeRabbit
New Features
Documentation
Tests