Skip to content

Commit 174e909

Browse files
committed
fix: address second round of PR review comments
- Add type validation for provides.commands (must be list) and hooks (must be dict) in manifest _validate() - Tighten malformed timestamp detection in git-common.sh to catch 7-digit dates without trailing slug (e.g. 2026031-143022) - Pass REPO_ROOT to has_git/Test-HasGit in create-new-feature scripts - Fix initialize command docs: surface errors on git failures, only skip when git is not installed - Fix commit command docs: 'skips with a warning' not 'silently' - Add tests for commands:null and hooks:list rejection
1 parent 9166afe commit 174e909

7 files changed

Lines changed: 52 additions & 10 deletions

File tree

extensions/git/commands/speckit.git.commit.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,6 @@ auto_commit:
4343
4444
## Graceful Degradation
4545
46-
- If Git is not available or not a repository: skips silently
46+
- If Git is not available or the current directory is not a repository: skips with a warning
4747
- If no config file exists: skips (disabled by default)
4848
- If no changes to commit: skips with a message

extensions/git/commands/speckit.git.initialize.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@ On success:
4242

4343
## Graceful Degradation
4444

45-
If Git is not installed or `git init` fails:
45+
If Git is not installed:
4646
- Warn the user
47-
- Do NOT block the rest of the workflow
47+
- Skip repository initialization
4848
- The project continues to function without Git (specs can still be created under `specs/`)
49+
50+
If Git is installed but `git init`, `git add .`, or `git commit` fails:
51+
- Surface the error to the user
52+
- Stop this command rather than continuing with a partially initialized repository

extensions/git/scripts/bash/create-new-feature.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ fi
240240

241241
# Check if git is available at this repo root
242242
if type has_git >/dev/null 2>&1; then
243-
if has_git; then
243+
if has_git "$REPO_ROOT"; then
244244
HAS_GIT=true
245245
else
246246
HAS_GIT=false

extensions/git/scripts/bash/git-common.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ check_feature_branch() {
2323
return 0
2424
fi
2525

26-
# Reject malformed timestamps (7-digit date or no trailing slug)
27-
if [[ "$branch" =~ ^[0-9]{7}-[0-9]{6}- ]] || [[ "$branch" =~ ^[0-9]{8}-[0-9]{6}$ ]]; then
26+
# Reject malformed timestamps (7-digit date, 8-digit date without trailing slug, or 7-digit with slug)
27+
if [[ "$branch" =~ ^[0-9]{7}-[0-9]{6} ]] || [[ "$branch" =~ ^[0-9]{8}-[0-9]{6}$ ]]; then
2828
echo "ERROR: Not on a feature branch. Current branch: $branch" >&2
2929
echo "Feature branches should be named like: 001-feature-name or 20260319-143022-feature-name" >&2
3030
return 1

extensions/git/scripts/powershell/create-new-feature.ps1

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ if (Get-Command Get-RepoRoot -ErrorAction SilentlyContinue) {
203203

204204
# Check if git is available
205205
if (Get-Command Test-HasGit -ErrorAction SilentlyContinue) {
206-
$hasGit = Test-HasGit
206+
$hasGit = Test-HasGit -RepoRoot $repoRoot
207207
} else {
208208
try {
209209
git -C $repoRoot rev-parse --is-inside-work-tree 2>$null | Out-Null

src/specify_cli/extensions.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,16 +183,28 @@ def _validate(self):
183183

184184
# Validate provides section
185185
provides = self.data["provides"]
186-
has_commands = "commands" in provides and provides["commands"]
187-
has_hooks = bool(self.data.get("hooks"))
186+
commands = provides.get("commands", [])
187+
hooks = self.data.get("hooks")
188+
189+
if "commands" in provides and not isinstance(commands, list):
190+
raise ValidationError(
191+
"Invalid provides.commands: expected a list"
192+
)
193+
if "hooks" in self.data and not isinstance(hooks, dict):
194+
raise ValidationError(
195+
"Invalid hooks: expected a mapping"
196+
)
197+
198+
has_commands = bool(commands)
199+
has_hooks = bool(hooks)
188200

189201
if not has_commands and not has_hooks:
190202
raise ValidationError(
191203
"Extension must provide at least one command or hook"
192204
)
193205

194206
# Validate commands (if present)
195-
for cmd in provides.get("commands", []):
207+
for cmd in commands:
196208
if "name" not in cmd or "file" not in cmd:
197209
raise ValidationError("Command missing 'name' or 'file'")
198210

tests/test_extensions.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,32 @@ def test_hooks_only_extension(self, temp_dir, valid_manifest_data):
291291
assert len(manifest.commands) == 0
292292
assert len(manifest.hooks) == 1
293293

294+
def test_commands_null_rejected(self, temp_dir, valid_manifest_data):
295+
"""Test manifest with commands: null is rejected."""
296+
import yaml
297+
298+
valid_manifest_data["provides"]["commands"] = None
299+
300+
manifest_path = temp_dir / "extension.yml"
301+
with open(manifest_path, 'w') as f:
302+
yaml.dump(valid_manifest_data, f)
303+
304+
with pytest.raises(ValidationError, match="Invalid provides.commands"):
305+
ExtensionManifest(manifest_path)
306+
307+
def test_hooks_not_dict_rejected(self, temp_dir, valid_manifest_data):
308+
"""Test manifest with hooks as a list is rejected."""
309+
import yaml
310+
311+
valid_manifest_data["hooks"] = ["not", "a", "dict"]
312+
313+
manifest_path = temp_dir / "extension.yml"
314+
with open(manifest_path, 'w') as f:
315+
yaml.dump(valid_manifest_data, f)
316+
317+
with pytest.raises(ValidationError, match="Invalid hooks"):
318+
ExtensionManifest(manifest_path)
319+
294320
def test_manifest_hash(self, extension_dir):
295321
"""Test manifest hash calculation."""
296322
manifest_path = extension_dir / "extension.yml"

0 commit comments

Comments
 (0)