fix(scripts): name setup-plan's feature directory key FEATURE_DIR - #4397
fix(scripts): name setup-plan's feature directory key FEATURE_DIR#4397Yash-Chindam wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The corrected output contract lacks an explicit regression test that would prevent all ports from reverting consistently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns setup-plan outputs with the repository-wide FEATURE_DIR naming convention.
Changes:
- Renames
SPECS_DIRtoFEATURE_DIRacross all script variants. - Updates the plan command’s JSON parsing instructions.
File summaries
| File | Description |
|---|---|
templates/commands/plan.md |
Uses the renamed output key. |
scripts/python/setup_plan.py |
Renames Python JSON and text outputs. |
scripts/powershell/setup-plan.ps1 |
Renames PowerShell outputs. |
scripts/bash/setup-plan.sh |
Renames Bash outputs, including both JSON paths. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| --arg feature_dir "$FEATURE_DIR" \ | ||
| --arg branch "$CURRENT_BRANCH" \ | ||
| '{FEATURE_SPEC:$feature_spec,IMPL_PLAN:$impl_plan,SPECS_DIR:$specs_dir,BRANCH:$branch}' | ||
| '{FEATURE_SPEC:$feature_spec,IMPL_PLAN:$impl_plan,FEATURE_DIR:$feature_dir,BRANCH:$branch}' |
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
setup-plan emitted a key called SPECS_DIR holding $FEATURE_DIR -- the per-feature subdirectory, not the specs root. The name is already taken elsewhere with the other meaning: create-new-feature.sh sets SPECS_DIR="$REPO_ROOT/specs" and derives FEATURE_DIR="$SPECS_DIR/$BRANCH_NAME". setup-plan was also the only script in the suite using it. setup-tasks and both check-prerequisites payloads already emit FEATURE_DIR for exactly this value, so this brings setup-plan in line rather than inventing a convention. Renamed in all three ports so the payloads stay identical, and in templates/commands/plan.md, which is the only consumer -- it parses the key by name, so it has to move in the same commit. Verified the bash, PowerShell, and Python variants all emit ['BRANCH','FEATURE_DIR','FEATURE_SPEC','IMPL_PLAN']. Fixes github#4017
65d59d4 to
72211e3
Compare
Addresses review feedback. The existing setup-plan tests compare the ports against each other, so all three could regress to SPECS_DIR together and still pass. This asserts the contract absolutely, in JSON and text mode and across bash/Python/PowerShell: the key is FEATURE_DIR, it carries the feature directory rather than the specs root, and SPECS_DIR is absent. The value is matched by suffix rather than full path because the ports legitimately differ in path flavour -- under MSYS bash reports /tmp/... where the Python and PowerShell ports report C:\... . The suffix still separates specs/001-my-feature from a bare specs, which is the regression being guarded; verified it rejects both /tmp/proj/specs and C:\proj\specs.
72211e3 to
5919579
Compare
|
Addressed in the latest push — good catch, and the criticism was accurate: every existing
One wrinkle worth flagging: the value is matched by suffix rather than by full path. The ports legitimately disagree on path flavour — under MSYS, bash reports Verified all six port × mode combinations produce the expected key and value. Also rebased onto current Disclosure: AI assistance (Claude) was used for this change and comment, as with the original PR. |
Fixes #4017
Problem
setup-planemitted a JSON key namedSPECS_DIRholding$FEATURE_DIR— the per-feature subdirectory, not the specs root.The name is already in use elsewhere with the other meaning. In
scripts/bash/create-new-feature.sh:So
SPECS_DIRmeantspecs/in one script andspecs/001-my-feature/in another.Why
FEATURE_DIRis the right namesetup-planwas the only script in the suite usingSPECS_DIR. Every sibling already emitsFEATURE_DIRfor exactly this value:This brings
setup-planin line rather than inventing a convention.Scope
The issue lists four bash locations. The same key is emitted by all three ports, and there is a consumer that parses it by name — fixing only bash would leave the ports disagreeing, which is worse than the current state. Full surface:
plan.mdships alongside the scripts, so it stays in sync automatically.On the compatibility question
I raised rename-vs-deprecated-alias on the issue. Going with the straight rename here, since
plan.mdis the only in-repo consumer and the value was never the specs root — anything relying onSPECS_DIRto mean the specs root was already reading the wrong path. Happy to switch to emittingFEATURE_DIRwhile keepingSPECS_DIRas a deprecated alias for a release or two if you would rather not break external parsers.Verification
Ran all three variants against a scratch project and compared the emitted keys:
Also confirmed no other reference to the old key remains outside
create-new-feature.sh, whereSPECS_DIRcorrectly means the specs root and is untouched.pytest tests/ -k "plan or command_template or preset"— 852 passed. The 8 failures are allWinError 1314: A required privilege is not held by the clientfrom symlink creation; the identical set fails on unmodifiedupstream/main, so they are environmental and unrelated.Disclosure: this change was developed with AI assistance (Claude). The AI helped map the affected call sites across the three ports, apply the rename, and draft this description. The three-port key comparison above was produced by running the scripts; I reviewed the change before submitting.