fix(hook): declare fish target_dir variable outside if/else block to fix scoping #65

yoshiori · 2025-12-17T06:36:30Z

What

Fix fish shell variable scoping bug in wtp cd hook that caused the command to fail silently.

Why

The wtp cd <worktree> command was not working in fish shell. The target_dir variable was always empty when checked outside the if/else block, causing the hook to fall through to the error handling path instead of changing directories.

This bug was introduced in commit 3afdc10 (#64) when adding support for no-argument wtp cd. The fish implementation incorrectly declared target_dir with set -l inside the if/else blocks, but fish's block scoping means local variables declared inside a block are not accessible outside it.

How

Declare the local variable before the if/else block, then assign to it (without -l) inside the blocks:

# Before (broken)
if test -z "$argv[2]"
    set -l target_dir (command wtp cd 2>/dev/null)
else
    set -l target_dir (command wtp cd $argv[2] 2>/dev/null)
end
# $target_dir is empty here!

# After (fixed)
set -l target_dir
if test -z "$argv[2]"
    set target_dir (command wtp cd 2>/dev/null)
else
    set target_dir (command wtp cd $argv[2] 2>/dev/null)
end
# $target_dir contains the path

Testing

Added TestFishHook_VariableScoping to verify correct variable declaration pattern
Updated existing fish hook tests to expect the fixed syntax
All unit tests pass
Manual verification:
source (wtp shell-init fish | psub)
wtp cd # now works correctly

Breaking Changes

None. This is a bug fix that makes the existing wtp cd command work as documented in fish shell.

Summary by CodeRabbit

Bug Fixes
- Improved Fish shell variable scoping for the cd command to ensure correct behavior.
Tests
- Added test coverage for variable scoping validation in shell hooks.

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

…fix scoping

coderabbitai · 2025-12-17T06:36:41Z

Walkthrough

The changes refactor how the Fish shell script handles variable scoping for target_dir in the "cd" command processing. The variable is now declared once outside conditional blocks with set -l and reassigned without the -l flag inside branches. Tests are added and updated to validate this new scoping pattern.

Changes

Cohort / File(s)	Summary
Fish hook variable scoping `cmd/wtp/hook.go`	Modified `printFishHook` to declare `target_dir` outside if/else blocks with `set -l`, then assign without `-l` inside conditional branches
Variable scoping tests `cmd/wtp/hook_test.go`	Added new test `TestFishHook_VariableScoping` to validate scoping behavior; updated existing fish hook no-arg cd tests to expect assignments without `-l` flag inside conditional blocks

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Verify Fish shell syntax correctness for the set -l declaration pattern used
Confirm test assertions accurately validate the new scoping behavior and edge cases
Check that all conditional branches consistently apply the assignment-without--l pattern

Possibly related PRs

Fix shell hook behavior for "wtp cd" with no args #64: Both PRs modify the Fish hook's handling of the target_dir variable and related tests

Poem

A rabbit hops through scopes so neat,
Where set -l makes the dance complete!
Outside the blocks, declare with care,
Assign within—no flag to spare. 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.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 accurately describes the main change: fixing fish shell variable scoping by declaring target_dir outside if/else blocks.

✨ 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: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3afdc10 and e856f76.

📒 Files selected for processing (2)

cmd/wtp/hook.go (1 hunks)
cmd/wtp/hook_test.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.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/hook_test.go
cmd/wtp/hook.go

**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Unit tests must be placed alongside packages as *_test.go files in the same directory
Target 70% code coverage with unit tests focusing on fast feedback and mocked git interactions using table-driven test cases

Files:

cmd/wtp/hook_test.go

🧠 Learnings (8)

📓 Common learnings

Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T13:33:48.693Z
Learning: Shell integration must separate completion and hooks for clarity; `wtp cd` outputs absolute worktree path with no side effects