fix(completions): stop Fish completions falling back to filenames

[leo-ar]

Status

LGTM for re-review. The implementation has been hardened against real Fish 4.8.1 behavior, the branch is mergeable without conflicts, and all GitHub checks are green on the updated head.

What was wrong

Fish falls back to filename completion whenever no matching rule suppresses it. OpenSpec's commands, subcommands, boolean flags, enum values, and non-path positionals therefore mixed repository files into their own suggestions.

The first implementation also had three edge cases: path-option rules could enable files outside the option value, command-name-like values could activate the wrong completion scope, and parent/global options could hide the real command path.

How it was fixed

• Suppress filesystem fallback in every active command and subcommand context.
• Restore files only for registry-backed path options and path positionals.
• Scope path-option fallback to the option's current value, including long = forms, named workset members, and attached short values.
• Match command and subcommand words in their actual slots instead of searching every token.
• Skip global and parent options, including consumed option values, while resolving the command path.
• Generate the global --no-color completion and keep top-level commands available after it.
• Track indexed positional slots while ignoring value-taking flags.
• Preserve Fish-safe literal descriptions without displaying unnecessary escape backslashes.

The shared command registry remains the source of truth for command-specific metadata. Other shell generators are unchanged.

Replication / proof

• Fish generator suite: 35/35 passed.
• Command-registry suite: 6/6 passed.
• Affected workset suite: 41/41 passed.
• Completion generator suites across Bash, Zsh, PowerShell, and Fish: 126/126 passed.
• Build, TypeScript, lint, and git diff --check pass.
• The generated registry script parses under Fish 4.8.1.
• Live Fish checks cover path options in space and = forms, name=path, attached short values, command-name value collisions, parent/global options, indexed positionals, --, and fallback suppression after a path is consumed.

Notes / nits

This changes generated Fish completion behavior only. Commands and subcommands suppress arbitrary files by default; explicit path metadata is required to restore filesystem suggestions.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 077a6a07-2f41-489f-ba05-a33ce9f0691a

📥 Commits

Reviewing files that changed from the base of the PR and between bfa5211 and ad317c3.

📒 Files selected for processing (2)

• src/core/completions/generators/fish-generator.ts
• test/core/completions/generators/fish-generator.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

Fish completion generation now prevents filesystem fallback for non-path completions. It adds indexed positional completion support, path metadata for flags, Fish helper logic, and tests for forced and path-specific completion rules.

Changes

Fish Completion Fallback Prevention

Layer / File(s)	Summary
Completion metadata contracts src/core/completions/types.ts, src/core/completions/command-registry.ts	FlagDefinition supports optional completion types. Path-related registry options use path completion metadata.
Fish completion generation src/core/completions/templates/fish-templates.ts, src/core/completions/generators/fish-generator.ts	Fish generation emits -f for non-path values, emits -F for path positionals, and supports indexed positional rules.
Completion behavior validation test/core/completions/generators/fish-generator.test.ts, .changeset/fish-completion-no-file-fallback.md	Tests inspect exact generated rules for command, flag, dynamic, indexed positional, and path completions. A patch-release entry records the change.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to ad317

This change prevents Fish from suggesting repository filenames for non-path arguments while preserving completion for true paths; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: alfred-openspec, tabishb, clay-good

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing Fish completions from falling back to filenames.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

test/core/completions/generators/fish-generator.test.ts (1)

428-429: 💤 Low value

Consider breaking up the long assertion for maintainability.

The assertion spans the entire Fish completion condition string. While this ensures exact format, it's brittle—generator changes to whitespace, condition order, or quoting would break the test even if the semantic behavior is correct.

♻️ More maintainable alternative

Check the key components separately rather than the full string:

-      expect(secondLine).toContain("complete -c openspec -n '__fish_openspec_using_subcommand workspace; and __fish_openspec_using_subcommand relink; and __fish_openspec_positional_index 1 2 --workspace'");
-      expect(secondLine).not.toContain('-f');
+      expect(secondLine).toContain('__fish_openspec_using_subcommand workspace');
+      expect(secondLine).toContain('__fish_openspec_using_subcommand relink');
+      expect(secondLine).toContain('__fish_openspec_positional_index 1 2 --workspace');
+      expect(secondLine).not.toContain('-f');

This preserves semantic correctness checks while being more resilient to formatting changes.

🤖 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 `@test/core/completions/generators/fish-generator.test.ts` around lines 428 -
429, The long brittle assertion on secondLine should be split into multiple
targeted checks: replace the single expect(secondLine).toContain(...) with
separate expectations that secondLine contains the completion command prefix
("complete -c openspec -n"), contains "__fish_openspec_using_subcommand
workspace", contains "__fish_openspec_using_subcommand relink", and contains
"__fish_openspec_positional_index 1 2 --workspace", while keeping the existing
expect(secondLine).not.toContain('-f'); this uses the secondLine variable in
test/core/completions/generators/fish-generator.test.ts and makes the test
resilient to formatting or ordering changes.

🤖 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 `@test/core/completions/generators/fish-generator.test.ts`:
- Around line 428-429: The long brittle assertion on secondLine should be split
into multiple targeted checks: replace the single
expect(secondLine).toContain(...) with separate expectations that secondLine
contains the completion command prefix ("complete -c openspec -n"), contains
"__fish_openspec_using_subcommand workspace", contains
"__fish_openspec_using_subcommand relink", and contains
"__fish_openspec_positional_index 1 2 --workspace", while keeping the existing
expect(secondLine).not.toContain('-f'); this uses the secondLine variable in
test/core/completions/generators/fish-generator.test.ts and makes the test
resilient to formatting or ordering changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0d28a349-8fdf-42aa-b558-1cdba32f0860

📥 Commits

Reviewing files that changed from the base of the PR and between 1b06fdd and d033217.

📒 Files selected for processing (6)

• .changeset/fish-completion-no-file-fallback.md
• src/core/completions/command-registry.ts
• src/core/completions/generators/fish-generator.ts
• src/core/completions/templates/fish-templates.ts
• src/core/completions/types.ts
• test/core/completions/generators/fish-generator.test.ts

[alfred-openspec]

PR #1199 review: Fish completion filesystem fallback

Date: 2026-06-12
PR: #1199
Author: @leo-ar

Summary

The product direction is right: Fish completions should use -f for commands, subcommands, boolean flags, enum values, and non-path positional/value completions so Fish does not mix filesystem suggestions into OpenSpec command suggestions. The PR also correctly introduces completionType: 'path' for value-taking flags where file completion should remain enabled.

Codebase pattern check

Referenced Product/Codebase/core-completions.md:

• COMMAND_REGISTRY remains the single source of truth for completion data.
• Shell-specific logic stays inside the Fish generator/template pair.
• New positional behavior is covered in Fish-specific tests rather than changing other shell generators.
• No installer or marker-block behavior changed.

Blocking finding

The submitted Fish generator test suite fails on the PR branch.

Command run in a temporary clone of the PR branch:

pnpm exec vitest run test/core/completions/generators/fish-generator.test.ts

Result:

FAIL test/core/completions/generators/fish-generator.test.ts > FishGenerator > generate > should allow file completion for path-typed indexed positionals
AssertionError: the given combination of arguments (undefined and string) is invalid for this assertion.

The failing assertion means completionLine(...) did not find the expected line for the path-typed indexed positional case. Until that is fixed, this PR should not merge.

Likely area to inspect

The failure is probably in the exact emitted condition for indexed positionals with value-consuming flags:

• generateIndexedPositionalCompletions(...)
• collectValueFlags(...)
• the test expectation for __fish_openspec_positional_index 1 2 --workspace

The generated condition may include both long and short value flags, have different ordering, or not emit a path positional rule because generatePositionalCompletion('path', ...) intentionally emits no explicit completion rule. If Fish needs filesystem fallback for path positionals, no explicit complete line may be correct, but then the test should assert the absence of -f differently or the implementation should emit a path rule intentionally.

Review decision

Changes requested.

Once the failing Fish test is fixed and the completion test suite passes, I expect this to be mergeable. The underlying behavior is useful and scoped, and it complements discussion #1197 without claiming to solve Fish install/detection issues.

[leo-ar]

Thanks for the review — addressed in the latest push.

I split the brittle assertion into smaller checks, and I also fixed the path-typed indexed positional case by emitting an explicit Fish completion rule for path positionals so filesystem completion is preserved there.

I re-ran the Fish completion test suite and typecheck, and both pass on the updated branch.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[clay-good]

Hardened and ready for re-review at 25b804c. The original failing indexed-path case passes, and the follow-up adds real Fish 4.8.1 coverage for scoped path fallback, exact command paths, global/parent options, command-name value collisions, and attached short path values. Focused completion checks are green locally; CI is rerunning on the pushed head.