Align labby CLI surfaces with implemented behavior#70
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 32 minutes and 45 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR completes the Lab CLI surface by removing legacy install/uninstall/init stubs, introducing shell-completions support, implementing canonical environment-merge preview and write flows for the extract command, and updating documentation and dispatch implementations to reflect working behavior only. ChangesLab CLI Surface Completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507976cc0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+96
to
+97
| name: "force", | ||
| ty: "bool", |
There was a problem hiding this comment.
force
Set ParamSpec.ty to "boolean" instead of "bool" for the new force parameters. The OpenAPI mapper only recognizes "boolean" and falls back unknown labels to string, so extract.apply/extract.diff are currently documented as accepting string force values while runtime parsing (parse_bool_param) rejects non-boolean JSON. This schema/runtime mismatch will break generated clients and UI form builders for these new actions.
Useful? React with 👍 / 👎.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lab/src/config/env_merge.rs`:
- Around line 346-381: The preview() function duplicates file-reading/key-value
parsing and request-entry deduplication logic that also appears in merge();
extract the shared logic into helpers (e.g., parse_existing_env(path: &Path) ->
Result<HashMap<String,String>, MergeError> which encapsulates fs::read_to_string
handling, NotFound => empty map, parsing lines, strip_quotes, and returning the
map, and dedupe_request_entries(entries: &[EnvEntry]) -> Vec<EnvEntry> which
merges duplicate keys keeping the last value) and then call these new helpers
from both preview() and merge(); ensure parse_existing_env returns the same
MergeError::WriteFailed semantics (using WriteFailReason::from_io) so behavior
remains identical.
In `@crates/lab/src/dispatch/extract.rs`:
- Around line 384-406: The function merge_request_from_creds is duplicated;
extract it into a shared module (e.g., create a new public function
merge_request_from_creds in crate::config) and replace both local
implementations with calls to that shared function. Make the new function
signature and behavior identical (accepting &[ServiceCreds] and force: bool and
returning env_merge::MergeRequest), mark it pub so both
dispatch::extract::merge_request_from_creds call sites can import it, update
imports/usages to call crate::config::merge_request_from_creds (or re-export if
preferred), and remove the duplicate definitions to prevent divergence.
In `@docs/superpowers/plans/2026-05-23-lab-cli-surface-completion.md`:
- Line 658: The verification gate wording contains a typo: the ambiguous token
"work-it" in the sentence referencing the compile break for
`tool_search_schema_visible` should be replaced with the intended term (likely
`worktree`); update the sentence to read "...fix that compile break in the same
worktree before continuing because worktree requires a green worktree" (or
better: "...fix that compile break in the same worktree before continuing
because worktree requires a green worktree")—locate the phrase "work-it" and
substitute "worktree" (or "workflow" if that fits the intended meaning) to make
the instruction clear and unambiguous.
In `@plugins/lab/skills/using-lab-cli/SKILL.md`:
- Around line 47-50: The example JSON uses an incorrect action name
"tool.search"; update the SKILL.md examples to use the actual synthetic
tool/service action name (e.g., replace "tool.search" with the real tool
identifier such as "scout.search" or its legacy alias) or a valid service action
string in the "action" field so it matches the runtime surface; adjust the
example lines that currently contain { "action": "tool.search", "params": {
"query": "radarr queue" } } to use the correct action name (e.g., { "action":
"scout.search", "params": { "query": "radarr queue" } }) and ensure consistency
with other examples.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4701a87-4604-4a5c-8b41-2fccecbd01f6
⛔ Files ignored due to path filters (6)
docs/generated/action-catalog.jsonis excluded by!**/generated/**and included by**/*docs/generated/action-catalog.mdis excluded by!**/generated/**and included by**/*docs/generated/cli-help.mdis excluded by!**/generated/**and included by**/*docs/generated/mcp-help.jsonis excluded by!**/generated/**and included by**/*docs/generated/mcp-help.mdis excluded by!**/generated/**and included by**/*docs/generated/openapi.jsonis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (15)
crates/lab/Cargo.tomlcrates/lab/src/cli.rscrates/lab/src/cli/extract.rscrates/lab/src/cli/gateway.rscrates/lab/src/cli/install.rscrates/lab/src/config/env_merge.rscrates/lab/src/dispatch/extract.rscrates/lab/src/dispatch/gateway/dispatch.rscrates/lab/src/docs/render.rscrates/lab/src/mcp/server.rsdocs/README.mddocs/superpowers/plans/2026-05-23-lab-cli-surface-completion.mddocs/surfaces/CLI.mdplugins/lab/skills/using-lab-cli/SKILL.mdplugins/lab/skills/using-lab-cli/references/config-reference.md
💤 Files with no reviewable changes (1)
- crates/lab/src/cli/install.rs
Comment on lines
+346
to
+381
| /// Classify a merge without writing, backing up, or pruning files. | ||
| pub fn preview(path: &Path, req: &MergeRequest) -> Result<MergePreview, MergeError> { | ||
| let existing_raw = match fs::read_to_string(path) { | ||
| Ok(s) => s, | ||
| Err(e) if e.kind() == ErrorKind::NotFound => String::new(), | ||
| Err(e) => { | ||
| return Err(MergeError::WriteFailed { | ||
| path: path.to_path_buf(), | ||
| reason: WriteFailReason::from_io(&e), | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| let mut existing_map: HashMap<String, String> = HashMap::new(); | ||
| for line in existing_raw.lines() { | ||
| let trimmed = line.trim(); | ||
| if trimmed.is_empty() || trimmed.starts_with('#') { | ||
| continue; | ||
| } | ||
| if let Some((k, v)) = trimmed.split_once('=') { | ||
| existing_map.insert(k.trim().to_owned(), strip_quotes(v.trim()).to_owned()); | ||
| } | ||
| } | ||
|
|
||
| let mut request_entries: Vec<EnvEntry> = Vec::new(); | ||
| for entry in &req.entries { | ||
| if let Some(slot) = request_entries | ||
| .iter_mut() | ||
| .find(|existing| existing.key == entry.key) | ||
| { | ||
| slot.value = entry.value.clone(); | ||
| } else { | ||
| request_entries.push(entry.clone()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting shared parsing logic.
The file-reading, key-value parsing, and request-entry deduplication logic (lines 348-380) is duplicated from merge() (lines 216-261). Extracting a shared helper (e.g., parse_existing_env and dedupe_request_entries) would reduce maintenance burden and ensure both paths stay in sync.
This is optional since the duplication is contained and both functions are cohesive.
🤖 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/lab/src/config/env_merge.rs` around lines 346 - 381, The preview()
function duplicates file-reading/key-value parsing and request-entry
deduplication logic that also appears in merge(); extract the shared logic into
helpers (e.g., parse_existing_env(path: &Path) -> Result<HashMap<String,String>,
MergeError> which encapsulates fs::read_to_string handling, NotFound => empty
map, parsing lines, strip_quotes, and returning the map, and
dedupe_request_entries(entries: &[EnvEntry]) -> Vec<EnvEntry> which merges
duplicate keys keeping the last value) and then call these new helpers from both
preview() and merge(); ensure parse_existing_env returns the same
MergeError::WriteFailed semantics (using WriteFailReason::from_io) so behavior
remains identical.
Comment on lines
+384
to
+406
| fn merge_request_from_creds(creds: &[ServiceCreds], force: bool) -> env_merge::MergeRequest { | ||
| let mut entries = Vec::new(); | ||
| for cred in creds { | ||
| let svc_upper = cred.service.to_uppercase(); | ||
| if let Some(url) = &cred.url { | ||
| entries.push(env_merge::EnvEntry::new( | ||
| format!("{svc_upper}_URL"), | ||
| url.clone(), | ||
| )); | ||
| } | ||
| if let Some(secret) = &cred.secret { | ||
| entries.push(env_merge::EnvEntry::new( | ||
| cred.env_field.clone(), | ||
| secret.clone(), | ||
| )); | ||
| } | ||
| } | ||
| env_merge::MergeRequest { | ||
| entries, | ||
| force, | ||
| expected_mtime: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Duplicated merge_request_from_creds logic.
This function is identical to the one in cli/extract.rs (lines 199-224). Consider extracting to a shared location (e.g., crate::config) to avoid divergence.
🤖 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/lab/src/dispatch/extract.rs` around lines 384 - 406, The function
merge_request_from_creds is duplicated; extract it into a shared module (e.g.,
create a new public function merge_request_from_creds in crate::config) and
replace both local implementations with calls to that shared function. Make the
new function signature and behavior identical (accepting &[ServiceCreds] and
force: bool and returning env_merge::MergeRequest), mark it pub so both
dispatch::extract::merge_request_from_creds call sites can import it, update
imports/usages to call crate::config::merge_request_from_creds (or re-export if
preferred), and remove the duplicate definitions to prevent divergence.
| cargo check --workspace --all-features | ||
| ``` | ||
|
|
||
| Expected: PASS. This is an early compile gate; `just check` below may repeat it as part of the repo-standard final gate. If this fails on the pre-existing `tool_search_schema_visible` compile issue, fix that compile break in the same worktree before continuing because work-it requires a green worktree. |
There was a problem hiding this comment.
Fix typo in verification gate wording.
work-it looks accidental and reads ambiguously in a critical verification instruction. Consider replacing it with the intended term (likely worktree or workflow).
🤖 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/superpowers/plans/2026-05-23-lab-cli-surface-completion.md` at line 658,
The verification gate wording contains a typo: the ambiguous token "work-it" in
the sentence referencing the compile break for `tool_search_schema_visible`
should be replaced with the intended term (likely `worktree`); update the
sentence to read "...fix that compile break in the same worktree before
continuing because worktree requires a green worktree" (or better: "...fix that
compile break in the same worktree before continuing because worktree requires a
green worktree")—locate the phrase "work-it" and substitute "worktree" (or
"workflow" if that fits the intended meaning) to make the instruction clear and
unambiguous.
Comment on lines
+47
to
+50
| ```json | ||
| { "action": "help" } | ||
| { "action": "schema", "params": { "action": "gateway.reload" } } | ||
| { "action": "tool.search", "params": { "query": "radarr queue" } } |
There was a problem hiding this comment.
Correct MCP example action naming for tool search.
"tool.search" appears inconsistent with the runtime surface and may mislead users. Use the actual synthetic tool name (e.g., scout/legacy alias) or a valid service action string in the action field.
🤖 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 `@plugins/lab/skills/using-lab-cli/SKILL.md` around lines 47 - 50, The example
JSON uses an incorrect action name "tool.search"; update the SKILL.md examples
to use the actual synthetic tool/service action name (e.g., replace
"tool.search" with the real tool identifier such as "scout.search" or its legacy
alias) or a valid service action string in the "action" field so it matches the
runtime surface; adjust the example lines that currently contain { "action":
"tool.search", "params": { "query": "radarr queue" } } to use the correct action
name (e.g., { "action": "scout.search", "params": { "query": "radarr queue" } })
and ensure consistency with other examples.
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
1 participant
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
Verification
Known blocker
Summary by CodeRabbit
New Features
labby completionscommand for shell completion support.labby extract --diffandlabby extract --applysubcommands with full functionality.--forceflag to override conflicting environment variables during extract operations.Removed
labby install,labby uninstall, andlabby initstubs.Documentation