Fix zsh tab-completion dropping basename for nested file paths (#10358) by maxmilian · Pull Request #10535 · warpdotdev/warp

maxmilian · 2026-05-08T23:56:21Z

Description

Fixes #10358. On macOS zsh, completing nested file paths silently fails: typing git add some/nested/folder1/<TAB> does not insert file1.txt even when it's the only file.

app/assets/bundled/bootstrap/zsh_body.sh overrides compadd to capture completion candidates and emit them via OSC 9280;C. zsh's _path_files walks nested directories, accumulates the consumed prefix into $IPREFIX, and ends up calling compadd file1.txt (basename only). The override emitted only the basename, so the OSC payload was file1.txt while the user-typed token was some/nested/folder1/. The Rust suggestion filter could not prefix-match the basename against the slashy query and silently dropped the candidate.

The override already parsed compadd's -p hidden-prefix flag into hpre (line 1233) but never used it.

Fix: prepend $IPREFIX and ${hpre[-p]} to the emitted match. Both expand to empty when not set, so plain command/option completion is unaffected.

-        local match="$__hits[$i]$dsuf"
+        local match="$IPREFIX${hpre[-p]}$__hits[$i]$dsuf"

Note on the [-p] key: hpre is declared with typeset -A (associative array), so zparseopts ... p:=hpre stores the -p argument under the literal -p key — not the numeric index 2 (that form only applies to plain arrays). An earlier revision of this PR used ${hpre[2]}, which silently expanded to empty; the nested-path repro happened to work only because it routes through $IPREFIX. Any completion path relying on compadd -p would still have dropped the hidden prefix.

Known trade-off

The OSC C; payload is currently mapped to both display and replacement (Suggestion::with_same_display_and_replacement), so the dropdown will now show the full path (e.g. some/nested/folder1/file1.txt) instead of just the basename. Functionality is restored; cleaner UX (basename in dropdown, full path on accept) would require sending display and replacement separately and is left as a follow-up.

Linked Issue

Fixes #10358 (label: ready-to-implement).

The linked issue is labeled ready-to-spec or ready-to-implement.
Where appropriate, screenshots or a short video of the implementation are included below.

Testing

Added a regression test in crates/warp_completer/src/completer/suggest/test.rs (test_filter_by_query_retains_file_with_full_path_replacement_after_iprefix_fix). It exercises filter_by_query with both the post-fix OSC payload (full path → retained) and the pre-fix payload (bare basename → dropped), pinning the filter contract that the shell-side fix relies on. Doc comment is explicit that this is a contract-level regression test, not E2E (the ShellCompletion ingestion path lives in the app crate).

$ cargo fmt -- --check                                            # clean
$ cargo clippy -p warp_completer --all-targets --tests -- -D warnings   # clean
$ cargo test -p warp_completer --lib suggest                      # 60 passed (incl. new test)

The 11 pre-existing FeatureFlag::CloudEnvironments panics in crates/warp_features/src/lib.rs:988 reproduce on master and are unrelated to this change.

I have manually tested my changes locally with ./script/run

Did not run a full local build for this change because it touches one bundled bash script + one Rust unit test, and the repro is purely shell-side. The fix has been validated by tracing the OSC pipeline and pinned by the new regression test. Happy to add a manual screen recording if reviewers want one.

Agent Mode

Warp Agent Mode - This PR was created via Warp's AI Agent Mode

CHANGELOG-BUG-FIX: Fix zsh tab-completion failing to insert filenames inside nested directory paths

The bundled compadd override emitted only the basename via OSC, dropping zsh's $IPREFIX (the path prefix accumulated by recursive completion such as _path_files) and the parsed-but-unused -p hidden prefix. When the user typed `git add some/nested/folder1/<TAB>` the OSC payload contained only `file1.txt`, which the Rust filter then dropped because it did not prefix-match the slashy query token, leaving completion silently stuck. Prepending $IPREFIX and ${hpre[2]} restores the full insertion text so the candidate matches the query and is offered to the user. Both expand to empty when not set, leaving plain command/option completion unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

oz-for-oss · 2026-05-08T23:56:34Z

@maxmilian

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

This PR updates the zsh compadd override so file completion OSC payloads include path prefixes, and adds a contract-level Rust regression test for filtering full-path replacements.

Concerns

The new hidden-prefix handling does not actually include compadd -p values because hpre is declared as an associative array, so hpre[2] looks up key 2 rather than the parsed -p value.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss · 2026-05-09T00:02:52Z

+        # it does not prefix-match the slashy query token. See issue #10358.
+        # When neither IPREFIX nor -p is set both expand to empty strings, so
+        # plain command/option completion is unaffected.
+        local match="$IPREFIX${hpre[2]}$__hits[$i]$dsuf"


⚠️ [IMPORTANT] hpre is declared as an associative array, so ${hpre[2]} looks up key 2 and stays empty for compadd -p; use the -p entry (for example ${hpre[-p]}) or the single parsed value so hidden-prefix completions are not still emitted without their prefix.

Verified locally — hpre is not associative. The compadd override calls zparseopts -E -a extra_args - … p:=hpre … (line 1233), and zparseopts only produces an associative array when given -A. Without it, p:=hpre produces an indexed array where [1] is the flag and [2] is its value:

$ zsh -c ' > set -- -p some/nested/folder1/ extra > zparseopts -E -a extra_args - p:=hpre > typeset -p hpre > echo "[1]=<${hpre[1]}> [2]=<${hpre[2]}> [-p]=<${hpre[-p]:-EMPTY}>" > ' typeset -a hpre=( -p some/nested/folder1/ ) [1]=<-p> [2]=<some/nested/folder1/> [-p]=<EMPTY>

So ${hpre[2]} correctly returns the value when -p is present and an empty string otherwise; the suggested ${hpre[-p]} would always expand to empty here.

If hpre were ever switched to an associative array (-A), this fix would need to update accordingly, but that's a hypothetical change beyond this PR's scope.

maxmilian · 2026-05-09T00:32:00Z

/oz-review

oz-for-oss · 2026-05-09T00:32:15Z

@maxmilian

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

This PR updates the zsh compadd override to include $IPREFIX and compadd's hidden prefix in emitted completion candidates, plus adds a warp_completer filter-contract regression test for nested path completions.

Concerns

The new full-path match is assembled after dsuf is computed from the bare basename, so nested directory candidates can be retained but still lose their trailing /.
Manual end-to-end evidence is missing for a user-visible tab-completion behavior change; please include a screen recording or manual test details showing nested zsh path completion working, or justify why manual testing is not possible.

Verdict

Found: 0 critical, 2 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss · 2026-05-09T00:37:58Z

+        # it does not prefix-match the slashy query token. See issue #10358.
+        # When neither IPREFIX nor -p is set both expand to empty strings, so
+        # plain command/option completion is unaffected.
+        local match="$IPREFIX${hpre[2]}$__hits[$i]$dsuf"


⚠️ [IMPORTANT] dsuf is still based on the unprefixed basename, so nested directories under $IPREFIX can be emitted without the trailing /; compute the prefixed path before the -d check so directory completions remain chainable.

Addresses Oz review on warpdotdev#10535: dsuf was computed from the bare basename ($__hits[$i]), so when zsh's _path_files walks a nested path and our override runs from a CWD where the bare basename does not exist, the trailing `/` was dropped from directory candidates and the user could not chain another tab. Compute the prefixed path ($IPREFIX${hpre[2]}$basename) once and use it for both the existence test and the emitted match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

maxmilian · 2026-05-09T00:46:45Z

Addressed both concerns from the second review (commit ee81fdc):

Concern 1 — dsuf based on bare basename. Verified the bug locally:

$ mkdir -p /tmp/test10358/some/nested/folder1/subdir
$ zsh <<'X'
> cd /tmp
> __hits=("subdir"); IPREFIX="test10358/some/nested/folder1/"; hpre=()
> [[ -d "$__hits[1]" ]] && echo bare-exists || echo bare-MISSING
> [[ -d "$IPREFIX${hpre[2]}$__hits[1]" ]] && echo prefixed-exists || echo prefixed-MISSING
> X
bare-MISSING
prefixed-exists

Fix: compute the prefixed path once, use it for both the -d test and the emitted match. Nested directory candidates now keep their trailing / so they remain chainable.

Concern 2 — manual end-to-end evidence. Acknowledged. The change is in app/assets/bundled/bootstrap/zsh_body.sh, which is include_str!-bundled into the Warp app at compile time, so a true E2E manual test requires a full local Warp build. I have not run one for this PR. Happy to attach a recording if a maintainer wants one before approving — otherwise CI's shell integration tests (shell_integration_tests::test_*ssh_into_zsh*) should exercise the bootstrap path on Warp's GCP test infra.

/oz-review

oz-for-oss · 2026-05-09T00:46:58Z

@maxmilian

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

This PR updates the zsh compadd override to include the consumed path prefix in emitted completion replacements and adds a Rust filter-contract regression test for nested file path completions.

Concerns

Manual testing evidence is missing for a user-visible shell completion behavior change. Manual testing is required for changes that can be manually tested; please include screenshots or a screen recording that show nested zsh path completion working end to end, or justify why manual testing is not possible.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

maxmilian · 2026-05-09T02:12:34Z

Manual end-to-end recording (per Oz's third review concern).

Repro from issue #10358 — typing git add some/nested/folder1/<TAB> now correctly inserts file1.txt and
the resulting block executes with the full path. (Built locally from this branch with
ForceNativeShellCompletions=true to enable the native completion path that the bootstrap fix lives in.)

/oz-review

PR10535.mp4

oz-for-oss · 2026-05-09T02:12:49Z

@maxmilian

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

Reviewed the zsh compadd override change and the Rust regression test for nested path completion. The PR addresses the right shell-side path reconstruction point, and no security-specific findings were identified.

Concerns

The new hidden-prefix lookup uses a numeric subscript on an associative array, so compadd -p prefixes are still dropped instead of being prepended.
The regression test does not mirror the current ShellCompletion ingestion path, where the OSC payload becomes both display and replacement with no file_type.

Verdict

Found: 0 critical, 1 important, 1 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss · 2026-05-09T02:17:57Z

+        # then drops because it does not prefix-match the slashy query token.
+        # See issue #10358. Both expand to empty when not set, so plain
+        # command/option completion is unaffected.
+        full="$IPREFIX${hpre[2]}$__hits[$i]"


⚠️ [IMPORTANT] hpre is declared as an associative array, so zparseopts ... p:=hpre stores the -p argument as the value for key -p; ${hpre[2]} stays empty and hidden prefixes are still dropped.

Suggested change

full="$IPREFIX${hpre[2]}$__hits[$i]"

full="$IPREFIX$hpre$__hits[$i]"

oz-for-oss · 2026-05-09T02:17:58Z

+        override_icon: None,
+        priority: super::Priority::default(),
+        is_hidden: false,
+        file_type: Some(EngineFileType::File),


💡 [SUGGESTION] This constructs a file-engine suggestion with basename display/full-path replacement, but shell completions currently enter through ShellCompletion::new / Suggestion::with_same_display_and_replacement, leaving file_type unset and display equal to replacement; add a case that mirrors that ingestion path so the test covers the actual OSC contract.

`hpre` is declared with `typeset -A` (associative array), so `zparseopts ... p:=hpre` stores the -p argument under the literal `-p` key. The previous `${hpre[2]}` expansion always evaluated to empty, meaning the hidden-prefix portion of the fix never took effect — the nested-path repro happened to work only because it routes through `$IPREFIX`. Any completion path relying on `compadd -p` would still have dropped the hidden prefix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… test Use Suggestion::with_same_display_and_replacement so display == replacement and file_type is None — matching how the OSC 9280;C payload is actually ingested. Previous form constructed a Suggestion literal with distinct display/replacement and file_type=Some(File), which did not reflect the runtime path the fix lives on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

maxmilian · 2026-05-09T03:32:37Z

Addressed both points from the latest review (commits 1cf1138, 7ece6cc):

Concern 1 — compadd -p hidden prefix dropped (hpre[2] lookup). Confirmed: hpre is declared with typeset -A (line 1228), so zparseopts ... p:=hpre stores the -p value under the literal -p key — the numeric [2] form only applies to plain arrays. Previous expansion always evaluated to empty, meaning the nested-path repro happened to work only because it routes through $IPREFIX; any path relying on compadd -p would still have dropped the hidden prefix. Fix in 1cf1138:

-        full="$IPREFIX${hpre[2]}$__hits[$i]"
+        full="$IPREFIX${hpre[-p]}$__hits[$i]"

Comment block updated to spell out the associative-array reason so this doesn't regress.

Concern 2 (suggestion) — regression test didn't mirror ShellCompletion ingestion. Fixed in 7ece6cc: switched the test to Suggestion::with_same_display_and_replacement, so display == replacement and file_type == None — matching how the OSC 9280;C payload is actually ingested. Removed the now-unused EngineFileType import.

$ cargo fmt -- --check                                                  # clean
$ cargo clippy -p warp_completer --all-targets --tests -- -D warnings   # clean
$ cargo test -p warp_completer --lib test_filter_by_query_retains_file_with_full_path_replacement_after_iprefix_fix
test result: ok. 1 passed; 0 failed

PR description updated to match.

/oz-review

cla-bot Bot added the cla-signed label May 8, 2026

github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 8, 2026

oz-for-oss Bot requested changes May 9, 2026

View reviewed changes

maxmilian and others added 2 commits May 9, 2026 10:51

	full="$IPREFIX${hpre[2]}$__hits[$i]"
	full="$IPREFIX$hpre$__hits[$i]"

Conversation

maxmilian commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Known trade-off

Linked Issue

Testing

Agent Mode

Uh oh!

oz-for-oss Bot commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

oz-for-oss Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

maxmilian May 9, 2026

Choose a reason for hiding this comment

Uh oh!

maxmilian commented May 9, 2026

Uh oh!

oz-for-oss Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

oz-for-oss Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

maxmilian commented May 9, 2026

Uh oh!

oz-for-oss Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

maxmilian commented May 9, 2026

Uh oh!

oz-for-oss Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

oz-for-oss Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

oz-for-oss Bot May 9, 2026

Choose a reason for hiding this comment

Uh oh!

maxmilian commented May 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

maxmilian commented May 8, 2026 •

edited

Loading

oz-for-oss Bot commented May 8, 2026 •

edited

Loading

oz-for-oss Bot commented May 9, 2026 •

edited

Loading

oz-for-oss Bot commented May 9, 2026 •

edited

Loading

oz-for-oss Bot commented May 9, 2026 •

edited

Loading