Skip to content

fix(plan): use .specify/feature.json to allow /speckit.plan on custom git branches (#2305)#2349

Open
Adr1an04 wants to merge 6 commits intogithub:mainfrom
Adr1an04:fix/2305-plan-feature-metadata
Open

fix(plan): use .specify/feature.json to allow /speckit.plan on custom git branches (#2305)#2349
Adr1an04 wants to merge 6 commits intogithub:mainfrom
Adr1an04:fix/2305-plan-feature-metadata

Conversation

@Adr1an04
Copy link
Copy Markdown

@Adr1an04 Adr1an04 commented Apr 24, 2026

This PR Closes #2305.

Summary

This PR is based on the discussion in #2305, with clarification from @mnriem that Git branch creation and spec directory creation are intentionally separate in Spec Kit, this change preserves that behavior.

The issue addressed here is narrower than the original directory naming expectation from #2305. After the discussion with @mnriem, I focused this PR on the later /speckit.plan behavior: /speckit.plan could fail on a custom Git branch like feature/my-feature-branch before using the existing feature context from .specify/feature.json.

That meant a valid setup like this could still be rejected:

{"feature_directory":"specs/001-notes-app"}

This PR updates setup-plan so it can continue when .specify/feature.json points to a valid existing feature directory. If feature.json is missing, stale, points to a non-existent directory, or does not match the resolved feature directory, the existing branch validation behavior still runs.

What changed

  • Added Bash and PowerShell helpers to check whether .specify/feature.json matches the resolved feature directory.
  • Updated setup-plan.sh and setup-plan.ps1 to use that metadata check before failing on the branch-name pattern.
  • Added focused regression tests for the custom-branch plus valid-feature.json case and fallback behavior

What did not change

Based on @mnriem's clarification in #2305, this PR doesn't change /speckit.specify, branch naming, spec directory naming, or make Git branch names the source of truth.

/speckit.specify still uses the existing feature branch and directory behavior. This change is only about letting /speckit.plan continue when existing feature metadata already resolves the correct feature directory.

Test selection reasoning

Changed file Affects Test Why
scripts/bash/setup-plan.sh /speckit.plan T1, T2, T3 setup-plan.sh is invoked by the plan workflow
scripts/powershell/setup-plan.ps1 /speckit.plan T1, T2, T3 PowerShell setup path for the plan workflow
scripts/bash/common.sh /speckit.plan, shared script consumers T1, T2, T3, T4 Added helper is called by setup-plan.sh; file is shared, so existing consistency/full-suite tests were also run
scripts/powershell/common.ps1 /speckit.plan, shared script consumers T1, T2, T3, T4 Added helper is called by setup-plan.ps1; file is shared, so existing consistency/full-suite tests were also run
tests/test_setup_plan_feature_json.py Regression coverage T1 Covers custom branch + valid/missing feature.json and normal numbered branch behavior

Required tests

  • T1: focused regression tests for setup-plan feature metadata behavior
  • T2: /speckit.specify manual prerequisite workflow
  • T3: /speckit.plan manual workflow on a custom branch with valid .specify/feature.json
  • T4: /speckit.tasks downstream smoke test
  • T5: tests/test_agent_config_consistency.py
  • T6: full pytest suite

Testing

I did focused regression tests from native PowerShell:

.\.venv\Scripts\python.exe -m pytest tests/test_setup_plan_feature_json.py -v
# 1 passed, 4 skipped
# The skipped tests are bash-gated and do not run under native PowerShell.

also focused regression tests through Git Bash:

& "C:\Program Files\Git\bin\bash.exe" -lc "cd /c/Projects/spec-kit && .venv/Scripts/python.exe -m pytest tests/test_setup_plan_feature_json.py -v"
# 5 passed

Used the existing consistency test:

.\.venv\Scripts\python.exe -m pytest tests/test_agent_config_consistency.py -q
# 24 passed

As well as ran a full test suite:

python -m pytest -q
# 1556 passed, 0 failed, 115 skipped, 18 warnings in 58.49s

Normal diff check just in case:

git diff --check
# no output

Script level validation

I also tested the setup scripts directly in temporary repos for both Bash and PowerShell:

  • Custom branch with valid .specify/feature.json: setup-plan exited 0, reused the existing feature directory, created plan.md, did not create a duplicate specs folder, and did not switch branches.
  • Custom branch without .specify/feature.json: setup-plan kept the existing behavior and failed with ERROR: Not on a feature branch, with no plan.md created.
  • Normal numbered branch: existing behavior still worked and plan.md was created normally.

Manual testing

I also ran the flow through GitHub Copilot in VS Code on Windows.

I initialized a fresh project with:

python -m uv run specify init C:\Projects\speckit-2305-manual-test --integration copilot --offline

Then I ran /speckit.specify, which created branch 001-notes-app and specs/001-notes-app/spec.md.

To reproduce the custom-branch scenario from #2305, I committed that initial output, switched to feature/my-feature-branch, and added:

{"feature_directory":"specs/001-notes-app"}

Then I ran /speckit.plan in a fresh Copilot chat. It stayed on feature/my-feature-branch, reused specs/001-notes-app, and created plan.md without the previous ERROR: Not on a feature branch failure.

I also ran /speckit.tasks as a smoke test. It created tasks.md in the same feature directory and did not create a duplicate specs folder.

Key evidence after /speckit.plan:
image

Follow-up review cleanup

After the Copilot review, I pushed a follow up commit to harden the metadata validation path:

  • Guarded Bash jq / python3 parsing so parse failures return non-zero from the helper instead of interacting poorly with set -e.
  • Added the grep/sed fallback to match the existing feature-path parsing behavior.
  • Added PowerShell regression coverage for the custom-branch plus missing feature.json fallback path.
  • Updated this PRs wording.

AI disclosure

I used ChatGPT and GitHub Copilot to help investigate the issue, review test coverage and also helping me polish this description (edit: also to fix the last problem which i reviewed myself and commit it with Cursor). I personally wrote the script changes, ran the repro, tests, and manual validation, and reviewed the final diff before submitting.

Copilot AI review requested due to automatic review settings April 24, 2026 03:57
@Adr1an04 Adr1an04 requested a review from mnriem as a code owner April 24, 2026 03:57
Copy link
Copy Markdown
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 PR updates the /speckit.plan setup scripts so that when .specify/feature.json already pins an existing feature directory, the plan workflow can proceed even if the current Git branch name doesn’t match the usual feature-branch pattern (e.g., custom branches like feature/my-feature-branch).

Changes:

  • Added helpers (bash + PowerShell) to verify .specify/feature.json matches the resolved active feature directory.
  • Updated setup-plan scripts to skip branch-pattern validation when the pinned directory is valid and matches.
  • Added regression tests covering the bash and PowerShell custom-branch + valid feature.json scenario.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
scripts/bash/common.sh Adds feature_json_matches_feature_dir helper used to decide whether to bypass branch checks.
scripts/bash/setup-plan.sh Uses the new helper to conditionally skip check_feature_branch.
scripts/powershell/common.ps1 Adds Test-FeatureJsonMatchesFeatureDir helper to validate pinned feature directory.
scripts/powershell/setup-plan.ps1 Uses the new helper to conditionally skip Test-FeatureBranch.
tests/test_setup_plan_feature_json.py Adds regression tests for custom branches with valid/missing feature.json (bash) and valid (PowerShell).

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

Comment thread scripts/bash/common.sh Outdated
Comment thread scripts/powershell/setup-plan.ps1
Comment thread tests/test_setup_plan_feature_json.py
Comment thread scripts/bash/common.sh Outdated
@Adr1an04
Copy link
Copy Markdown
Author

I addressed the Copilot feedback in by hardening the Bash parsing path, adding the grep/sed fallback, and adding the missing PowerShell negative test.

I reran the focused tests after the update: Git Bash 5 passed, native PowerShell 2 passed, 3 skipped, agent config consistency 24 passed, and git diff --check is clean. I also updated the original PR description with the latest info. Thank you!

Copy link
Copy Markdown
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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.


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

Comment thread scripts/powershell/common.ps1 Outdated
Comment thread scripts/bash/common.sh Outdated
@Adr1an04
Copy link
Copy Markdown
Author

I’ll leave this decision up to you @mnriem. Copilot is suggesting a shared helper refactor, but I’m happy to make that change if you’d prefer.

Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

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

Please address the first comment by Copilot. For the second comment I will resolve that one

Copy link
Copy Markdown
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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.


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

Comment thread scripts/powershell/common.ps1 Outdated
Comment thread scripts/bash/common.sh Outdated
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Apr 24, 2026

@Adr1an04 Please address Copilot feedback

@Adr1an04
Copy link
Copy Markdown
Author

Fixed the copilot comments for the PowerShell path comparison and Bash parsing helper @mnriem .

I left the broader Get-FeaturePathsEnv behavior for malformed feature.json unchanged since that seems like a larger shared path resolution change. I can include that here too if you’d prefer, but I kept it out for now to avoid widening the PR.

Thanks for running copilot for me

Copy link
Copy Markdown
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.

Copilot's findings

Tip

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

  • Files reviewed: 5/5 changed files
  • Comments generated: 1

Comment on lines +68 to +87
subprocess.run(
["git", "checkout", "-q", "-b", "feature/my-feature-branch"],
cwd=plan_repo,
check=True,
)
feat = plan_repo / "specs" / "001-tiny-notes-app"
feat.mkdir(parents=True)
(feat / "spec.md").write_text("# spec\n", encoding="utf-8")
(plan_repo / ".specify" / "feature.json").write_text(
json.dumps({"feature_directory": "specs/001-tiny-notes-app"}),
encoding="utf-8",
)
script = plan_repo / ".specify" / "scripts" / "bash" / "setup-plan.sh"
result = subprocess.run(
["bash", str(script)],
cwd=plan_repo,
capture_output=True,
text=True,
check=False,
)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

These tests invoke the setup scripts without controlling the child process environment. If a developer/CI runner has SPECIFY_FEATURE, SPECIFY_FEATURE_DIRECTORY, or related env vars set, get_feature_paths can resolve a different feature directory/branch and make the tests flaky. Consider passing an explicit env to subprocess.run that starts from os.environ but removes/overrides those SPECIFY_* variables (and any other variables the scripts consult).

Copilot uses AI. Check for mistakes.
@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Apr 24, 2026

No problem. Note Copilot can be a bit annoying at times, but we require everyone to go through the same cycle of reviews until Copilot finally stops giving more feedback (and yes that includes maintainers too ;))

So time to address some more Copilot feedback!

@Adr1an04
Copy link
Copy Markdown
Author

Good to know this is my first PR here so! Thank you, I'll fix the comments copilot gave out.

Copy link
Copy Markdown
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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.


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

Comment thread scripts/bash/common.sh Outdated
Comment on lines +203 to +204
norm_json="$(cd -- "$_fd" 2>/dev/null && pwd)" || return 1
norm_active="$(cd -- "$active_feature_dir" 2>/dev/null && pwd)" || return 1
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

feature_json_matches_feature_dir claims to compare “resolved” paths, but it uses pwd (logical path), which does not resolve symlinks. If $FEATURE_DIR and the feature_directory from feature.json refer to the same directory via different symlink representations, this equality check will be false and /speckit.plan will incorrectly require a feature-branch name. Consider using a physical path (pwd -P) or another canonicalization approach to avoid false negatives.

Suggested change
norm_json="$(cd -- "$_fd" 2>/dev/null && pwd)" || return 1
norm_active="$(cd -- "$active_feature_dir" 2>/dev/null && pwd)" || return 1
norm_json="$(cd -- "$_fd" 2>/dev/null && pwd -P)" || return 1
norm_active="$(cd -- "$active_feature_dir" 2>/dev/null && pwd -P)" || return 1

Copilot uses AI. Check for mistakes.
@Adr1an04 Adr1an04 requested a review from Copilot April 24, 2026 16:45
Copy link
Copy Markdown
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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.


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

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.

[Bug]: custom branch from spec creation is not respected on directory creation and future steps

3 participants