Update for better maintainability - #406
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 5. **Update agent file incrementally** (O(1) operation): | ||
| - Run `{SCRIPT}` for your AI assistant | ||
| - Run `{SCRIPT}` | ||
| **IMPORTANT**: Execute it exactly as specified above. Do not add or remove any arguments. |
There was a problem hiding this comment.
The instruction 'Execute it exactly as specified above' refers to something that isn't actually specified in the visible context. This could be confusing since there's no specific execution command shown above this line.
| **IMPORTANT**: Execute it exactly as specified above. Do not add or remove any arguments. | |
| **IMPORTANT**: When the actual script command is provided, execute it exactly as shown, without adding or removing any arguments. |
| grep "^\*\*${field_pattern}\*\*: " "$plan_file" 2>/dev/null | \ | ||
| head -1 | \ | ||
| sed "s/^**${field_pattern}**: //" | \ | ||
| sed "s|^\*\*${field_pattern}\*\*: ||" | \ | ||
| sed 's/^[ \t]*//;s/[ \t]*$//' | \ |
There was a problem hiding this comment.
[nitpick] The regex pattern uses escaped asterisks \*\* which creates unnecessary complexity. Consider using a more readable approach or add a comment explaining why the asterisks need to be escaped in this context.
| if [[ -d "$dir" ]]; then | ||
| local dirname=$(basename "$dir") | ||
| if [[ "$dirname" =~ ^([0-9]{3})- ]]; then | ||
| local number=${BASH_REMATCH[1]} |
There was a problem hiding this comment.
[nitpick] The arithmetic expansion $((10#$number)) forces base-10 interpretation to handle potential leading zeros, but this could be clearer with a comment explaining why this is necessary for proper numeric comparison.
| local number=${BASH_REMATCH[1]} | |
| local number=${BASH_REMATCH[1]} | |
| # Force base-10 interpretation to handle leading zeros (e.g., "007" -> 7) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| sed "s|^\*\*${field_pattern}\*\*: ||" | \ | ||
| sed 's/^[ \t]*//;s/[ \t]*$//' | \ |
There was a problem hiding this comment.
Multiple sed calls can be combined into a single sed command for better performance. Consider using: sed 's|^**'"$field_pattern"'**: ||; s/^[ \t]//; s/[ \t]$//' for improved efficiency.
| sed "s|^\*\*${field_pattern}\*\*: ||" | \ | |
| sed 's/^[ \t]*//;s/[ \t]*$//' | \ | |
| sed "s|^\*\*${field_pattern}\*\*: ||;s/^[ \t]*//;s/[ \t]*$//" | \ |
| $regex = "^\*\*$([Regex]::Escape($FieldPattern))\*\*: (.+)$" | ||
| Get-Content -LiteralPath $PlanFile | ForEach-Object { | ||
| if ($_ -match $regex) { | ||
| $val = $Matches[1].Trim() |
There was a problem hiding this comment.
The regex uses $Matches[1] but the capture group is actually the second group (index 1). However, the intended capture should be $Matches[2] since the first group is the escaped field pattern and the second group (.+) contains the actual value.
| $val = $Matches[1].Trim() | |
| $val = $Matches[2].Trim() |
| if [ -f "$TEMPLATE" ]; then cp "$TEMPLATE" "$SPEC_FILE"; else touch "$SPEC_FILE"; fi | ||
|
|
||
| # Set the SPECIFY_FEATURE environment variable for the current session | ||
| export SPECIFY_FEATURE="$BRANCH_NAME" |
There was a problem hiding this comment.
The export statement only affects the current script's environment. Consider documenting that users need to manually export this variable in their shell session, or provide instructions for persistent environment variable setup.
| @@ -158,11 +161,11 @@ build_variant() { | |||
| mkdir -p "$base_dir/.windsurf/workflows" | |||
| generate_commands windsurf md "\$ARGUMENTS" "$base_dir/.windsurf/workflows" "$script" ;; | |||
| codex) | |||
There was a problem hiding this comment.
[nitpick] The path change from '.codex/commands' to '.codex/prompts' should be documented in the commit message or comments, as this represents a breaking change for existing Codex integrations.
| codex) | |
| codex) | |
| # BREAKING CHANGE: Codex integration path changed from '.codex/commands' to '.codex/prompts'. | |
| # Existing Codex integrations expecting '.codex/commands' will need to be updated. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
scripts/bash/check-prerequisites.sh:1
- Line 62 contains PowerShell syntax in a bash script. The variable assignment should use bash syntax:
has_git=$(test -d \"$REPO_ROOT/.git\" && echo \"true\" || echo \"false\")
#!/usr/bin/env bash
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| param( | ||
| [Parameter(Position=0)] | ||
| [ValidateSet('claude','gemini','copilot','cursor','qwen','opencode','codex','windsurf')] |
There was a problem hiding this comment.
[nitpick] The ValidateSet attribute should be consistent across all scripts. Consider defining this list as a constant or shared variable to ensure consistency and easier maintenance when adding new agents.
| param( | |
| [Parameter(Position=0)] | |
| [ValidateSet('claude','gemini','copilot','cursor','qwen','opencode','codex','windsurf')] | |
| # Define agent types as a constant array for consistency | |
| $AGENT_TYPES = @('claude','gemini','copilot','cursor','qwen','opencode','codex','windsurf') | |
| param( | |
| [Parameter(Position=0)] | |
| [ValidateSet($AGENT_TYPES)] |
| check_file "$RESEARCH" "research.md" | ||
| check_file "$DATA_MODEL" "data-model.md" | ||
| check_dir "$CONTRACTS_DIR" "contracts/" | ||
| check_file "$QUICKSTART" "quickstart.md" |
There was a problem hiding this comment.
The check_file and check_dir functions are being called but are not defined in this script. These functions need to be defined or sourced from common.sh.
| Test-FileExists -Path $paths.RESEARCH -Description 'research.md' | Out-Null | ||
| Test-FileExists -Path $paths.DATA_MODEL -Description 'data-model.md' | Out-Null | ||
| Test-DirHasFiles -Path $paths.CONTRACTS_DIR -Description 'contracts/' | Out-Null | ||
| Test-FileExists -Path $paths.QUICKSTART -Description 'quickstart.md' | Out-Null | ||
|
|
||
| if ($IncludeTasks) { | ||
| Test-FileExists -Path $paths.TASKS -Description 'tasks.md' | Out-Null |
There was a problem hiding this comment.
The functions Test-FileExists and Test-DirHasFiles are being called but are not defined in this script or imported from common.ps1. These functions need to be defined or the calls should be replaced with appropriate PowerShell equivalents.
| Test-FileExists -Path $paths.RESEARCH -Description 'research.md' | Out-Null | |
| Test-FileExists -Path $paths.DATA_MODEL -Description 'data-model.md' | Out-Null | |
| Test-DirHasFiles -Path $paths.CONTRACTS_DIR -Description 'contracts/' | Out-Null | |
| Test-FileExists -Path $paths.QUICKSTART -Description 'quickstart.md' | Out-Null | |
| if ($IncludeTasks) { | |
| Test-FileExists -Path $paths.TASKS -Description 'tasks.md' | Out-Null | |
| if (Test-Path $paths.RESEARCH) { | |
| Write-Output " [x] research.md" | |
| } else { | |
| Write-Output " [ ] research.md (missing)" | |
| } | |
| if (Test-Path $paths.DATA_MODEL) { | |
| Write-Output " [x] data-model.md" | |
| } else { | |
| Write-Output " [ ] data-model.md (missing)" | |
| } | |
| if ((Test-Path $paths.CONTRACTS_DIR) -and (Get-ChildItem -Path $paths.CONTRACTS_DIR -File -ErrorAction SilentlyContinue | Select-Object -First 1)) { | |
| Write-Output " [x] contracts/" | |
| } else { | |
| Write-Output " [ ] contracts/ (missing or empty)" | |
| } | |
| if (Test-Path $paths.QUICKSTART) { | |
| Write-Output " [x] quickstart.md" | |
| } else { | |
| Write-Output " [ ] quickstart.md (missing)" | |
| } | |
| if ($IncludeTasks) { | |
| if (Test-Path $paths.TASKS) { | |
| Write-Output " [x] tasks.md" | |
| } else { | |
| Write-Output " [ ] tasks.md (missing)" | |
| } |
| mkdir -p "$base_dir/.codex/prompts" | ||
| generate_commands codex md "\$ARGUMENTS" "$base_dir/.codex/prompts" "$script" ;; | ||
| esac |
There was a problem hiding this comment.
[nitpick] The directory structure for Codex has changed from .codex/commands to .codex/prompts but this change should be documented or commented to explain why the directory structure differs from other agents.
| mkdir -p "$base_dir/.codex/prompts" | |
| generate_commands codex md "\$ARGUMENTS" "$base_dir/.codex/prompts" "$script" ;; | |
| esac | |
| # Note: Codex uses `.codex/prompts` instead of `.codex/commands` to match its expected directory structure. | |
| # This differs from other agents, which use a `commands` subdirectory. See Codex integration docs for details. | |
| mkdir -p "$base_dir/.codex/prompts" | |
| generate_commands codex md "\$ARGUMENTS" "$base_dir/.codex/prompts" "$script" ;; |
Update for better maintainability
Update for better maintainability
Updates that make general QOL improvements for better project maintainability.