Skip to content

Conversation

@Amolith
Copy link
Contributor

@Amolith Amolith commented Dec 4, 2025

Using env broke Fish completions because command is a Fish builtin while env is an external program unaware of what Fish does and doesn't provide:

env: ‘command’: No such file or directory

Fish supports FIELD=val natively, so we can just remove env.

Assisted-by: Claude Sonnet 4.5 via Crush [email protected]

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved shell completion efficiency by optimizing how environment variables are passed to the completion command.

✏️ Tip: You can customize this high-level summary in your review settings.

Using `env` broke Fish completions because `command` is a Fish builtin
while `env` is an external program unaware of what Fish does and doesn't
provide:

    env: ‘command’: No such file or directory

Fish supports `FIELD=val` natively, so we can just remove `env`.

Assisted-by: Claude Sonnet 4.5 via Crush <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

Modified fish shell completion script to set environment variables directly instead of using the env builtin command. The change affects how WTP_SHELL_COMPLETION=1 is passed to the wtp command invocation, aligning source code and test data accordingly.

Changes

Cohort / File(s) Summary
Fish completion env variable assignment
cmd/wtp/completion_config.go, cmd/wtp/testdata/completion/fish_expected.fish
Replaced env WTP_SHELL_COMPLETION=1 command wtp syntax with direct shell variable assignment WTP_SHELL_COMPLETION=1 command wtp in fish completion logic and corresponding test expectations

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 In the sea of shells, a fish swims free,
No env wrapper needed, just WTP!
Direct assignment flows cleaner, you see,
A hop and a skip—simpler syntax, hurray! 🎣

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(fish-completions): don't execute with env' directly describes the main change: removing the use of the env builtin from fish completion scripts, which aligns with the file modifications shown in the raw summary.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f7387d and de05740.

📒 Files selected for processing (2)
  • cmd/wtp/completion_config.go (1 hunks)
  • cmd/wtp/testdata/completion/fish_expected.fish (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Follow standard Go style with tabs and gofmt; package names must be short and lowercase
Keep import groups tidy; use goimports for organization with local prefix following module path github.com/satococoa/wtp/v2
Adhere to linting rules in .golangci.yml (vet, staticcheck, gosec, mnd, lll=120)
Wrap errors with context; avoid ignoring errors in Go code
Use snake_case for Go filenames (e.g., remove.go, shell_integration.go)
Document exported items in Go when non-trivial; follow godoc conventions

Files:

  • cmd/wtp/completion_config.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Toggle shell integration paths with `WTP_SHELL_INTEGRATION=1` when testing cd behavior
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects

Applied to files:

  • cmd/wtp/testdata/completion/fish_expected.fish
  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Use `go run ./cmd/wtp <args>` for rapid feedback during development instead of building binaries

Applied to files:

  • cmd/wtp/testdata/completion/fish_expected.fish
  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: CLI entrypoint is located at `cmd/wtp`; new command implementations must integrate with the main CLI structure

Applied to files:

  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: The root module is `github.com/satococoa/wtp/v2` running on Go 1.24; maintain module path consistency

Applied to files:

  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Applies to **/*.go : Keep import groups tidy; use goimports for organization with local prefix following module path `github.com/satococoa/wtp/v2`

Applied to files:

  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Run commands from inside a worktree to mimic real usage (e.g., `go run ../cmd/wtp add feature/new-feature`)

Applied to files:

  • cmd/wtp/completion_config.go
📚 Learning: 2025-12-02T13:33:48.673Z
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.673Z
Learning: Toggle shell integration paths with `WTP_SHELL_INTEGRATION=1` when testing cd behavior

Applied to files:

  • cmd/wtp/completion_config.go
🔇 Additional comments (2)
cmd/wtp/completion_config.go (1)

120-120: LGTM! Correctly fixes Fish builtin compatibility.

Removing env resolves the issue where env attempted to execute Fish's command builtin as an external program. Fish natively supports the VAR=value command syntax for per-command environment variables.

cmd/wtp/testdata/completion/fish_expected.fish (1)

25-25: LGTM! Test data correctly reflects the source change.

The expected output now matches the corrected Fish completion script from the source code.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@satococoa satococoa self-requested a review December 5, 2025 14:07
Copy link
Owner

@satococoa satococoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contributions!! 👍

@satococoa satococoa merged commit d19361b into satococoa:main Dec 5, 2025
7 checks passed
@Amolith Amolith deleted the fix-fish-completions branch December 5, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants