Skip to content

Conversation

@Francesco146
Copy link

No description provided.

Copilot AI review requested due to automatic review settings December 20, 2025 08:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds Fish shell support to the setup-plan script, expanding cross-shell compatibility alongside the existing Bash and PowerShell implementations.

Key Changes:

  • Introduces a new Fish shell implementation of setup-plan functionality
  • Implements argument parsing with --json and --help flags
  • Mirrors the structure and behavior of existing Bash/PowerShell versions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

check_feature_branch $CURRENT_BRANCH $HAS_GIT; or exit 1

# Ensure the feature directory exists
mkdir -p "$FEATURE_DIR"
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The mkdir command doesn't check for failure. While '-p' flag prevents errors if the directory exists, it could still fail due to permission issues. Consider checking the return status or using 'or exit 1' to ensure the script fails fast if directory creation fails, similar to how check_feature_branch is handled.

Suggested change
mkdir -p "$FEATURE_DIR"
mkdir -p "$FEATURE_DIR"; or exit 1

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +24
source "$SCRIPT_DIR/common.fish"

Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The script sources a file 'common.fish' that does not exist in the repository. This will cause the script to fail immediately when executed. You need to either create the common.fish file with the required functions (get_feature_paths, check_feature_branch) or reference an existing common file.

Suggested change
source "$SCRIPT_DIR/common.fish"
# Define required helper functions (inlined to avoid missing common.fish dependency)
function get_feature_paths
# Determine repository root and whether this is a git repository
set -l repo_root
set -l has_git false
if type -q git
set repo_root (git rev-parse --show-toplevel ^/dev/null)
if test $status -eq 0
set has_git true
else
set repo_root (pwd)
end
else
set repo_root (pwd)
end
# Determine current branch name if in a git repo
set -l current_branch
if test "$has_git" = "true"
set current_branch (git rev-parse --abbrev-ref HEAD ^/dev/null)
if test -z "$current_branch"
set current_branch "unknown-branch"
end
else
set current_branch "no-git"
end
# Compute feature-related paths
set -l feature_dir "$repo_root/specs/$current_branch"
set -l feature_spec "$feature_dir/spec.md"
set -l impl_plan "$feature_dir/plan.md"
# Output fish commands to be eval'd by the caller
printf 'set -gx REPO_ROOT %s; set -gx HAS_GIT %s; set -gx CURRENT_BRANCH %s; set -gx FEATURE_DIR %s; set -gx FEATURE_SPEC %s; set -gx IMPL_PLAN %s;' \
(string escape -- $repo_root) \
$has_git \
(string escape -- $current_branch) \
(string escape -- $feature_dir) \
(string escape -- $feature_spec) \
(string escape -- $impl_plan)
end
function check_feature_branch
set -l branch $argv[1]
set -l has_git $argv[2]
# If this is not a git repository, do not enforce any branch policy
if test "$has_git" != "true"
return 0
end
if test -z "$branch"
echo "Error: Unable to determine current git branch." >&2
return 1
end
# Additional branch policy checks could be added here if needed.
return 0
end

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +27
eval (get_feature_paths)

Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

Using eval with command substitution in fish shell is problematic. The bash version outputs shell variable assignments (e.g., "REPO_ROOT='value'") that work with eval in bash, but fish shell uses a different syntax for variable assignment. In fish, you need to use 'set' commands. The get_feature_paths function would need to output fish-compatible set commands like "set -gx REPO_ROOT 'value'" instead of bash-style assignments.

Suggested change
eval (get_feature_paths)
# Expect lines like: VAR='value' (bash-style). Convert to fish exports.
for line in (get_feature_paths)
# Split on the first '=' into name and value
set parts (string split -m1 '=' -- $line)
set name $parts[1]
set raw_value $parts[2]
# Remove surrounding single quotes from the value, if present
set value $raw_value
if test (string length -- $raw_value) -ge 2
if test (string sub -s 1 -l 1 -- $raw_value) = "'" -a (string sub -s -1 -- $raw_value) = "'"
set value (string sub -s 2 -l (math (string length -- $raw_value) - 2) -- $raw_value)
end
end
# Export as a global variable in fish
if test -n "$name"
set -gx $name $value
end
end

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +48
if test $JSON_MODE = true
printf '{"FEATURE_SPEC":"%s","IMPL_PLAN":"%s","SPECS_DIR":"%s","BRANCH":"%s","HAS_GIT":"%s"}\n' \
$FEATURE_SPEC $IMPL_PLAN $FEATURE_DIR $CURRENT_BRANCH $HAS_GIT
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

Variables in fish shell should be quoted when used in contexts where they could contain spaces or special characters. Unlike bash, fish doesn't perform word splitting on unquoted variables in most contexts, but for consistency with error messages and to prevent issues with paths containing special characters, the variables should be quoted, especially in the printf statement.

Suggested change
if test $JSON_MODE = true
printf '{"FEATURE_SPEC":"%s","IMPL_PLAN":"%s","SPECS_DIR":"%s","BRANCH":"%s","HAS_GIT":"%s"}\n' \
$FEATURE_SPEC $IMPL_PLAN $FEATURE_DIR $CURRENT_BRANCH $HAS_GIT
if test "$JSON_MODE" = true
printf '{"FEATURE_SPEC":"%s","IMPL_PLAN":"%s","SPECS_DIR":"%s","BRANCH":"%s","HAS_GIT":"%s"}\n' \
"$FEATURE_SPEC" "$IMPL_PLAN" "$FEATURE_DIR" "$CURRENT_BRANCH" "$HAS_GIT"

Copilot uses AI. Check for mistakes.
Comment on lines +47 to +48
printf '{"FEATURE_SPEC":"%s","IMPL_PLAN":"%s","SPECS_DIR":"%s","BRANCH":"%s","HAS_GIT":"%s"}\n' \
$FEATURE_SPEC $IMPL_PLAN $FEATURE_DIR $CURRENT_BRANCH $HAS_GIT
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The JSON output does not properly escape special characters (quotes, backslashes, newlines) that might appear in file paths or branch names. This could produce invalid JSON if any of these variables contain such characters. Consider using a proper JSON serialization method or at minimum escape double quotes, backslashes, and control characters in the variable values.

Suggested change
printf '{"FEATURE_SPEC":"%s","IMPL_PLAN":"%s","SPECS_DIR":"%s","BRANCH":"%s","HAS_GIT":"%s"}\n' \
$FEATURE_SPEC $IMPL_PLAN $FEATURE_DIR $CURRENT_BRANCH $HAS_GIT
env FEATURE_SPEC="$FEATURE_SPEC" IMPL_PLAN="$IMPL_PLAN" SPECS_DIR="$FEATURE_DIR" BRANCH="$CURRENT_BRANCH" HAS_GIT="$HAS_GIT" python3 - << 'PY'
import json
import os
import sys
data = {
"FEATURE_SPEC": os.environ.get("FEATURE_SPEC", ""),
"IMPL_PLAN": os.environ.get("IMPL_PLAN", ""),
"SPECS_DIR": os.environ.get("SPECS_DIR", ""),
"BRANCH": os.environ.get("BRANCH", ""),
"HAS_GIT": os.environ.get("HAS_GIT", ""),
}
json.dump(data, sys.stdout)
sys.stdout.write("\n")
PY

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +17
set ARGS

for arg in $argv
switch $arg
case --json
set JSON_MODE true
case --help -h
echo "Usage: setup-plan.fish [--json]"
echo " --json Output results in JSON format"
echo " --help Show this help message"
exit 0
case '*'
set -a ARGS $arg
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The ARGS variable is declared and populated in the argument parsing loop but is never used anywhere in the script. Either remove this unused variable or implement the intended functionality for handling additional arguments.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,55 @@
#!/usr/bin/env fish
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

The bash version includes 'set -e' to exit immediately on error, which is a defensive scripting practice. While fish shell doesn't have an exact equivalent, consider using the '--on-event fish_exit' event handler or explicitly check command return codes to ensure similar error handling behavior throughout the script.

Copilot uses AI. Check for mistakes.
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.

1 participant