fix: stop check-prerequisites --paths-only from writing feature.json (#3025)#3190
Open
Noor-ul-ain001 wants to merge 3 commits into
Open
fix: stop check-prerequisites --paths-only from writing feature.json (#3025)#3190Noor-ul-ain001 wants to merge 3 commits into
Noor-ul-ain001 wants to merge 3 commits into
Conversation
…ithub#3025) check-prerequisites --paths-only / -PathsOnly is documented as pure, read-only path resolution, but when SPECIFY_FEATURE_DIRECTORY was set it called the persist routine and rewrote .specify/feature.json. That dirtied the working tree and overwrote a pinned feature directory during what should be a no-op. Add an explicit opt-out at the resolver boundary instead of a global env back-channel: - bash: get_feature_paths accepts a leading --no-persist flag that skips _persist_feature_json; check-prerequisites.sh passes it in --paths-only mode. - PowerShell: Get-FeaturePathsEnv gains a -NoPersist switch that skips Save-FeatureJson; check-prerequisites.ps1 passes it in -PathsOnly mode. Normal (non-paths-only) invocations are unchanged and still persist the override, so future sessions without the env var keep working. Add regression tests asserting --paths-only/-PathsOnly leaves a pinned feature.json untouched even when the env override differs, plus a guard that normal mode still persists.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes unintended side effects during “read-only” path resolution: check-prerequisites --paths-only / -PathsOnly now resolves feature paths without persisting SPECIFY_FEATURE_DIRECTORY into .specify/feature.json, preventing working tree dirtiness and accidental overwrites of pinned feature directories (issue #3025).
Changes:
- Bash: add
get_feature_paths --no-persistopt-out and use it fromcheck-prerequisites.sh --paths-only. - PowerShell: add
Get-FeaturePathsEnv -NoPersistopt-out and use it fromcheck-prerequisites.ps1 -PathsOnly. - Add regression tests ensuring paths-only honors the override in output but does not rewrite
.specify/feature.json(bash + PowerShell), and that bash normal mode still persists.
Show a summary per file
| File | Description |
|---|---|
| tests/test_check_prerequisites_paths_only.py | Adds regression coverage for “paths-only does not write feature.json”, plus a bash “normal mode still persists” guard. |
| scripts/powershell/common.ps1 | Adds -NoPersist switch to skip feature.json persistence for read-only callers. |
| scripts/powershell/check-prerequisites.ps1 | Passes -NoPersist to Get-FeaturePathsEnv when -PathsOnly is used. |
| scripts/bash/common.sh | Adds --no-persist flag handling to get_feature_paths to skip _persist_feature_json. |
| scripts/bash/check-prerequisites.sh | Uses get_feature_paths --no-persist in --paths-only mode. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
Comment on lines
+167
to
+171
| # Persist to feature.json so future sessions without the env var still | ||
| # work — unless the caller opted out for read-only resolution (#3025). | ||
| if (-not $NoPersist) { | ||
| Save-FeatureJson -RepoRoot $repoRoot -FeatureDirectory $env:SPECIFY_FEATURE_DIRECTORY | ||
| } |
The em-dash in the persist comment introduced non-ASCII bytes, failing test_ps1_file_is_ascii_only which enforces ASCII-only PowerShell sources for Windows PowerShell 5.1 compatibility. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+348
to
+352
| @pytest.mark.skipif(not (HAS_PWSH or _WINDOWS_POWERSHELL), reason="no PowerShell available") | ||
| def test_ps_paths_only_does_not_persist_feature_json(prereq_repo: Path) -> None: | ||
| """-PathsOnly must not rewrite feature.json even when the env override | ||
| differs from the pinned value (#3025).""" | ||
| pinned = "specs/001-my-feature" |
mnriem
requested changes
Jun 29, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
mnriem
requested changes
Jun 29, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback and add the requested test
Addresses Copilot review feedback on github#3190: the bash side had a `test_normal_mode_still_persists_feature_json` guard, but there was no symmetric PowerShell test asserting that running check-prerequisites.ps1 *without* -PathsOnly still persists the SPECIFY_FEATURE_DIRECTORY override into .specify/feature.json. Add test_ps_normal_mode_still_persists_feature_json, which guards against accidentally passing -NoPersist unconditionally (or flipping the default) in a future refactor. Verified it fails when -NoPersist is passed in the non -PathsOnly branch and passes with the current conditional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
check-prerequisites --paths-only(and PowerShell-PathsOnly) is documented as pure, read-only path resolution. But whenSPECIFY_FEATURE_DIRECTORYis set,get_feature_paths/Get-FeaturePathsEnvcalled the persist routine and rewrote.specify/feature.json— dirtying the working tree and overwriting a pinned feature directory during what should be a no-op.Fixes #3025.
How
Opt out of the write at the resolver boundary with an explicit flag, rather than a global env back-channel (per maintainer feedback on #3053):
get_feature_pathsaccepts a leading--no-persistflag that skips_persist_feature_json;check-prerequisites.shpasses it in--paths-onlymode.Get-FeaturePathsEnvgains a-NoPersistswitch that skipsSave-FeatureJson;check-prerequisites.ps1passes it in-PathsOnlymode.Normal (non-paths-only) invocations are unchanged and still persist the override, so future sessions without the env var keep working.
Tests
Added regression tests (bash + PowerShell):
--paths-only/-PathsOnlyleaves a pinnedfeature.jsonuntouched even whenSPECIFY_FEATURE_DIRECTORYdiffers from the pinned value, while still honoring the override in the output.Notes
This is an alternative to #3053, which originally used a
SPECIFY_NO_PERSIST_FEATURE_JSONenv var; that approach was flagged in review as a leaky global back-channel. This PR uses explicit local flags/parameters at the call site instead.