Skip to content

fix: avoid paths-only feature context writes#3053

Open
luohui1 wants to merge 1 commit into
github:mainfrom
luohui1:fix/3025-feature-context-paths
Open

fix: avoid paths-only feature context writes#3053
luohui1 wants to merge 1 commit into
github:mainfrom
luohui1:fix/3025-feature-context-paths

Conversation

@luohui1

@luohui1 luohui1 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #3025 by preventing check-prerequisites --paths-only / -PathsOnly from mutating .specify/feature.json while it is only resolving paths.

SPECIFY_FEATURE_DIRECTORY remains an explicit feature-directory override. Normal command execution still persists that override through .specify/feature.json, so future sessions without the environment variable continue to work. The no-write behavior is scoped specifically to paths-only mode because that mode is documented as read-only path resolution.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

Additional validation after addressing review feedback:

  • uv run --extra test python -m pytest tests/test_check_prerequisites_paths_only.py tests/test_setup_plan_feature_json.py tests/test_timestamp_branches.py::TestFeatureDirectoryResolution -q - 8 passed, 12 skipped
  • uvx ruff check tests/test_check_prerequisites_paths_only.py - passed
  • uvx ruff format --check tests/test_check_prerequisites_paths_only.py - passed
  • git diff --check - passed

Manual test results

Agent: Codex (GPT-5) | OS/Shell: Windows 11 / PowerShell 7.5.4

Command tested Notes
.specify/scripts/bash/check-prerequisites.sh --json --paths-only Covered by regression test for read-only .specify/feature.json behavior.
.specify/scripts/powershell/check-prerequisites.ps1 -Json -PathsOnly Covered by regression test for read-only .specify/feature.json behavior.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

This PR was implemented by Codex (GPT-5) acting as an AI coding agent on behalf of @luohui1. The agent read the repository contribution/security/code-of-conduct docs and issue templates, inspected the relevant code paths, implemented the changes, ran the validation listed above, and prepared this PR description.

@luohui1 luohui1 requested a review from mnriem as a code owner June 18, 2026 08:26
@mnriem

mnriem commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Please separate out in 3 PRs. Thanks!

@luohui1 luohui1 force-pushed the fix/3025-feature-context-paths branch from 99577f8 to 1c89df3 Compare June 18, 2026 14:06
@luohui1 luohui1 changed the title fix: harden feature context path resolution fix: avoid paths-only feature context writes Jun 18, 2026
@mnriem

mnriem commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this. The fix direction is right — --paths-only is documented as read-only path resolution, so it shouldn't write tracked files. A couple of requests before this lands:

1. Reframe the rationale. The PR/issue describes SPECIFY_FEATURE_DIRECTORY as "only a temporary read-only override," but that's not how it's defined in the code. In get_feature_paths() it's an explicit override that is deliberately persisted ("Persist to feature.json so future sessions without the env var still work"). Can you re-anchor the justification on the defensible claim — "--paths-only must not mutate tracked files" — rather than on the env var being temporary? Also worth a line confirming persistence is still covered by the non-paths-only commands (specify, setup-plan, setup-tasks, etc.), so the "future sessions still work" guarantee holds.

2. Drop the new SPECIFY_NO_PERSIST_FEATURE_JSON env var. It's a global back-channel for one bit of state, and the PowerShell path sets $env:SPECIFY_NO_PERSIST_FEATURE_JSON = '1' without ever unsetting it — so suppression leaks for the rest of the process and would silently disable persistence for any later in-process Save-FeatureJson call. Please pass this as a local parameter/switch instead:

  • Bash: a flag/arg on get_feature_paths threaded down to _persist_feature_json.
  • PowerShell: a [switch]$NoPersist on Get-FeaturePathsEnv passed to Save-FeatureJson.

That keeps it self-contained and removes the leak. (If there's a strong reason to keep the env var, the PS side at least needs to scope/unset it in a finally to match the inline scoping Bash already uses.)

@luohui1 luohui1 force-pushed the fix/3025-feature-context-paths branch from 1c89df3 to 072c69d Compare June 22, 2026 04:55
@luohui1

luohui1 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in the latest update:

  • Reframed the PR rationale around the --paths-only contract: path resolution should not mutate .specify/feature.json.
  • Removed the global SPECIFY_NO_PERSIST_FEATURE_JSON back-channel. Bash now passes --no-persist to get_feature_paths; PowerShell now passes -NoPersist to Get-FeaturePathsEnv.
  • Kept the normal non-paths-only flow unchanged, so SPECIFY_FEATURE_DIRECTORY is still persisted by regular commands such as setup/plan/task flows.

Validation:

  • uv run --extra test python -m pytest tests/test_check_prerequisites_paths_only.py tests/test_setup_plan_feature_json.py tests/test_timestamp_branches.py::TestFeatureDirectoryResolution -q - 8 passed, 12 skipped
  • uvx ruff check tests/test_check_prerequisites_paths_only.py - passed
  • uvx ruff format --check tests/test_check_prerequisites_paths_only.py - passed
  • git diff --check - passed

AI disclosure: this response was prepared by Codex (GPT-5) acting as an AI coding agent on behalf of @luohui1.

@luohui1 luohui1 force-pushed the fix/3025-feature-context-paths branch from 072c69d to 401cf91 Compare June 22, 2026 08:41
@mnriem mnriem requested a review from Copilot June 22, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

Prevents check-prerequisites in --paths-only / -PathsOnly mode from persisting feature-directory overrides into .specify/feature.json, keeping paths-only behavior read-only as documented (closes #3025).

Changes:

  • Add “no-persist” capability to Bash get_feature_paths and PowerShell Get-FeaturePathsEnv.
  • Update Bash/PowerShell check-prerequisites entrypoints to use no-persist path resolution in paths-only mode.
  • Add regression tests ensuring paths-only does not rewrite .specify/feature.json.
Show a summary per file
File Description
tests/test_check_prerequisites_paths_only.py Adds regression coverage ensuring paths-only does not mutate .specify/feature.json (Bash + PowerShell).
scripts/powershell/common.ps1 Adds -NoPersist switch to avoid saving feature.json in paths-only mode.
scripts/powershell/check-prerequisites.ps1 Routes paths-only mode through Get-FeaturePathsEnv -NoPersist.
scripts/bash/common.sh Adds --no-persist flag to skip _persist_feature_json during path resolution.
scripts/bash/check-prerequisites.sh Uses get_feature_paths --no-persist when --paths-only is set.

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: 4

Comment on lines +150 to +158
"""--paths-only must not rewrite .specify/feature.json when env differs."""
common = (prereq_repo / ".specify" / "scripts" / "bash" / "common.sh").read_text(
encoding="utf-8"
)
script_text = (
prereq_repo / ".specify" / "scripts" / "bash" / "check-prerequisites.sh"
).read_text(encoding="utf-8")
assert "SPECIFY_NO_PERSIST_FEATURE_JSON" not in common
assert "get_feature_paths --no-persist" in script_text
Comment on lines +82 to +86
if $PATHS_ONLY; then
_paths_output=$(get_feature_paths --no-persist) || { echo "ERROR: Failed to resolve feature paths" >&2; exit 1; }
else
_paths_output=$(get_feature_paths) || { echo "ERROR: Failed to resolve feature paths" >&2; exit 1; }
fi
Comment thread scripts/bash/common.sh
Comment on lines +155 to +158
local no_persist=false
if [[ "${1:-}" == "--no-persist" ]]; then
no_persist=true
fi
Comment thread scripts/bash/common.sh
Comment on lines +177 to +179
if [[ "$no_persist" == "false" ]]; then
_persist_feature_json "$repo_root" "$SPECIFY_FEATURE_DIRECTORY"
fi

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please address Copilot feedback

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]: get_feature_paths persists .specify/feature.json during read-only path resolution (check-prerequisites.sh --paths-only)

3 participants