fix(zsh): show all matches when subcommand names contain :#666
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughcomplete-word now emits three tab-separated columns (value, description, shell-quoted insert) for zsh; the zsh completion template parses those columns, builds aligned display labels, and calls ChangesZsh Completion Format Refactoring
Sequence Diagram(s): sequenceDiagram
participant CLI
participant ZshTpl
participant Compadd
CLI->>ZshTpl: emit tab-separated lines (value\tdescription\t'insert')
ZshTpl->>ZshTpl: parse into values, descs, inserts
ZshTpl->>ZshTpl: build _usage_display
ZshTpl->>Compadd: call compadd -d _usage_display -a inserts
Compadd-->>ZshTpl: present completion candidates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a zsh completion bug where subcommands containing
Confidence Score: 5/5Safe to merge — the change is a clean, well-scoped replacement of The three-column protocol is internally consistent: No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "fix(zsh): update generated completion as..." | Re-trigger Greptile |
|
Both flagged issues are valid. Switched the contract between This comment was generated by an AI coding assistant. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/src/complete/zsh.rs (1)
19-26: ⚡ Quick winUpdate the function rustdoc to match the new 3-column contract.
The header comment above
render_completion_loopstill describes the old 2-column_describeflow, which now conflicts with this implementation.🤖 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 `@lib/src/complete/zsh.rs` around lines 19 - 26, Update the rustdoc for render_completion_loop to describe the current 3-column contract (raw value, description, shell-quoted insert) instead of the old 2-column `_describe` flow: explain that `complete-word --shell zsh` emits three tab-separated columns, that the function builds the formatted display (using `template` and aligned `descs`) and calls `compadd` directly so every match is offered (not collapsed), and note the handling of escaped prefixes like `\:`; reference `render_completion_loop`, `template`, and `compadd` in the comment so readers can locate the related logic.
🤖 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.
Nitpick comments:
In `@lib/src/complete/zsh.rs`:
- Around line 19-26: Update the rustdoc for render_completion_loop to describe
the current 3-column contract (raw value, description, shell-quoted insert)
instead of the old 2-column `_describe` flow: explain that `complete-word
--shell zsh` emits three tab-separated columns, that the function builds the
formatted display (using `template` and aligned `descs`) and calls `compadd`
directly so every match is offered (not collapsed), and note the handling of
escaped prefixes like `\:`; reference `render_completion_loop`, `template`, and
`compadd` in the comment so readers can locate the related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0acab893-7a8d-4dfe-8291-864c4643a5b0
⛔ Files ignored due to path filters (4)
lib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-2.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-3.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh_init.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
cli/src/cli/complete_word.rscli/tests/complete_word.rscli/tests/shell_completions_integration.rslib/src/complete/zsh.rs
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/complete/zsh.rs (1)
7-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale completion contract comment.
Line 7-Line 13 still documents the old 2-column
value:descriptionformat, but this function now reads 3 tab-separated fields (value,desc,insert). Please align the comment to avoid future regressions.Suggested diff
-/// `complete-word --shell zsh` emits two tab-separated columns per match: -/// the display string (`value:description` for `_describe`) and the -/// shell-quoted insert string. `usage complete-word` already filters by the -/// typed prefix, so `-U` tells `compadd` not to re-filter (which would -/// discard our pre-quoted matches whose literal text starts with `'`). +/// `complete-word --shell zsh` emits three tab-separated columns per match: +/// raw value, description, and shell-quoted insert. We build the display +/// string from value+description and pass insert to `compadd`. `usage +/// complete-word` already filters by the typed prefix, so `-U` tells +/// `compadd` not to re-filter (which would discard pre-quoted matches whose +/// literal text starts with `'`).🤖 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 `@lib/src/complete/zsh.rs` around lines 7 - 13, Update the stale comment describing the zsh completion contract to reflect that `complete-word --shell zsh` now emits three tab-separated fields (`value`, `desc`, `insert`) instead of the old two-column `value:description` format; keep the existing notes about why `-U` is required (to avoid re-filtering pre-quoted inserts) and that `compstate[insert]=menu` is used to skip longest-common-prefix insertion when values share a leading quote, and mention that the function parses these three fields accordingly so future maintainers know the expected order.
🤖 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.
Outside diff comments:
In `@lib/src/complete/zsh.rs`:
- Around line 7-13: Update the stale comment describing the zsh completion
contract to reflect that `complete-word --shell zsh` now emits three
tab-separated fields (`value`, `desc`, `insert`) instead of the old two-column
`value:description` format; keep the existing notes about why `-U` is required
(to avoid re-filtering pre-quoted inserts) and that `compstate[insert]=menu` is
used to skip longest-common-prefix insertion when values share a leading quote,
and mention that the function parses these three fields accordingly so future
maintainers know the expected order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4e6fdfd-76b7-4c74-90b6-f1229e3f259d
📒 Files selected for processing (1)
lib/src/complete/zsh.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/tests/shell_completions_integration.rs (1)
728-747:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the zsh subprocess exits successfully.
Line 728 and Line 1192 only check
stdout. If either script prints the expected inserts and then exits non-zero, these regressions still pass and hide a broken completion path.Proposed fix
let stdout = String::from_utf8_lossy(&result.stdout); let stderr = String::from_utf8_lossy(&result.stderr); + assert!( + result.status.success(), + "zsh completion script exited non-zero. stdout:\n{stdout}\nstderr:\n{stderr}" + ); let expected = [ "release:create", "release:docs-sync",let stdout = String::from_utf8_lossy(&result.stdout); let stderr = String::from_utf8_lossy(&result.stderr); + assert!( + result.status.success(), + "zsh init completion script exited non-zero. stdout:\n{stdout}\nstderr:\n{stderr}" + ); let expected = [ "release:create", "release:docs-sync",Also applies to: 1192-1211
🤖 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 `@cli/tests/shell_completions_integration.rs` around lines 728 - 747, The test runs a zsh subprocess with Command::new(...).output() but only asserts on stdout contents; update the test to also assert the child exited successfully by checking result.status.success() (or !result.status.success() in the failure message) before/with the existing subcommand asserts, and include result.status, stdout, and stderr in the failure message to aid debugging; apply the same change to the duplicate block around the other test (the one that mirrors lines 1192-1211) so both places verify the zsh process exit status in addition to checking stdout for expected inserts.
🤖 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.
Outside diff comments:
In `@cli/tests/shell_completions_integration.rs`:
- Around line 728-747: The test runs a zsh subprocess with
Command::new(...).output() but only asserts on stdout contents; update the test
to also assert the child exited successfully by checking result.status.success()
(or !result.status.success() in the failure message) before/with the existing
subcommand asserts, and include result.status, stdout, and stderr in the failure
message to aid debugging; apply the same change to the duplicate block around
the other test (the one that mirrors lines 1192-1211) so both places verify the
zsh process exit status in addition to checking stdout for expected inserts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c29a180d-d5bf-4f12-bff6-7eb4a1526397
⛔ Files ignored due to path filters (4)
lib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-2.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-3.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh_init.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
cli/tests/shell_completions_integration.rslib/src/complete/zsh.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/complete/zsh.rs
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
@coderabbitai applied in 94d5488. Both colon-grouping tests now assert This comment was generated by Claude Code. |
|
Tip For best results, initiate chat on the files or code changes.
|
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
`_describe` groups matches that share a `\:`-escaped prefix and surfaces only one entry per group. For specs that have several subcommands sharing a colon prefix (e.g. `release:create`, `release:docs-sync`, `release:pr`, `release:update`), the menu only lists `release:create` and silently drops the rest. The same collapse happens when a plain `lint` exists alongside `lint:fix`. Build the display column ourselves from the (insert, description) pair and call `compadd` directly so every match is offered, with descriptions aligned to the longest value. Regression test: feeds a colon-heavy spec through real zsh + zpty and asserts every subcommand appears in the menu output. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…desc` Greptile flagged that the previous fix used `inserts[i]` (which can be shell-quoted, e.g. `'Alice Bob'`) as the prefix to strip from the `zsh_escape`-d display string. For any value containing a space the strip silently failed and the menu showed `'Alice Bob' -- Alice Bob`. Switch the contract between `complete-word --shell zsh` and the generated template to emit three tab-separated columns — raw value, raw description, shell-quoted insert. The zsh template reads the three fields, builds the formatted display directly from value + description (no `\:`/`\(`/`\[` escaping needed), and feeds the inserts to compadd. Drops the now-unused `zsh_escape` helper. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
… output
zsh's `read -r v d i` with `IFS=$'\t'` collapses consecutive empty
delimiters, so a row like `val-1\t\tval-1` (entry without a description)
parsed as two fields — the value duplicated into descs and the insert
was lost. Switch to `${(@ps:\t:)line}` which preserves empty fields
between tabs.
Also drop the `_describe` else-branch in the completion loop: it
re-introduced the colon-grouping bug this PR fixes and would mis-parse
the pre-formatted display strings. The previous reason for keeping it
was the init-path integration test; that test (and the colon-grouping
regression tests) is rewritten to drive the generated function with a
stubbed `compadd` instead of the zpty harness, which crashes zsh 5.9 on
both this host and CI with "free(): invalid pointer".
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
94d5488 to
4c8c960
Compare
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 `@cli/assets/completions/_usage`:
- Around line 30-33: The TSV parsing loop using "while IFS=$'\\t' read -r value
desc insert; do" can drop empty middle fields; replace it with zsh's split that
preserves empty columns (use the ${(`@ps`:\\t:)line} form) to produce an array
(e.g., parts) and then append parts[1], parts[2], parts[3] to values, descs, and
inserts respectively so empty descriptions are kept deterministically; update
the loop that populates values, descs, and inserts to use that split instead of
the read invocation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e6c1211-325e-4a49-af55-f671beab239f
⛔ Files ignored due to path filters (4)
lib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-2.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh-3.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh.snapis excluded by!**/*.snaplib/src/complete/snapshots/usage__complete__zsh__tests__complete_zsh_init.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
cli/assets/completions/_usagecli/src/cli/complete_word.rscli/tests/complete_word.rscli/tests/shell_completions_integration.rslib/src/complete/zsh.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/tests/complete_word.rs
- lib/src/complete/zsh.rs
- cli/src/cli/complete_word.rs
- cli/tests/shell_completions_integration.rs
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Summary
_describegroups matches that share a\:-escaped prefix and surfaces only one entry per group. With several subcommands sharing a colon prefix (e.g.release:create,release:docs-sync,release:pr,release:update) the menu only listsrelease:createand silently drops the rest. The same collapse happens when a plainlintexists alongsidelint:fix.Repro
Generate zsh completion, tab
testcli <TAB>— before this PR, onlyrelease:createandlintappear. After: all six show.Discovered via jdx/mise where users have tasks named
release:create,release:docs-sync,release:pr,release:update— most of them disappeared from completion.Fix
Build the display column ourselves from the
(insert, description)pair and callcompadddirectly so every match is offered, with descriptions padded to the longest value for column alignment.Test plan
cargo testsuite passes locally (cargo test --release)lib/src/complete/snapshots/...)test_zsh_completion_includes_all_colon_subcommandsfeeds a colon-heavy spec through real zsh + zpty and asserts every subcommand appears in the menuThis PR was authored with assistance from an AI coding agent.
Summary by CodeRabbit