fix(integrations): reject empty --commands-dir in generic raw_options#3714
Open
jawwad-ali wants to merge 2 commits into
Open
fix(integrations): reject empty --commands-dir in generic raw_options#3714jawwad-ali wants to merge 2 commits into
jawwad-ali wants to merge 2 commits into
Conversation
GenericIntegration._resolve_commands_dir has a parity gap: the parsed-options branch guards emptiness (`if commands_dir:`), but the raw_options fallback returned the value verbatim with no check. So `--integration-options= "--commands-dir="` (or `--commands-dir ""`) resolves to `""`, which makes setup() compute `dest = project_root / "" == project_root` and write every speckit command file (specify.md, plan.md, ...) directly into the PROJECT ROOT — silently bypassing the documented "--commands-dir is required" contract and polluting the repo root. Apply the same non-empty guard to the raw_options branch so an empty value falls through to the existing "required" ValueError on every input form. Non-empty values resolve exactly as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes generic integration handling so empty raw --commands-dir values cannot write command files into the project root.
Changes:
- Rejects empty raw option values.
- Adds regression tests for empty and valid forms.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/integrations/generic/__init__.py |
Validates raw command-directory values. |
tests/integrations/test_integration_generic.py |
Covers empty and non-empty raw values. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
Self-review follow-up: bare truthiness only closes the empty-string subset. A
whitespace-only value passed both branches (verified: raw "--commands-dir ' '"
returned ' ', parsed {"commands_dir": " "} returned ' '), so command files
still landed in a directory literally named " " instead of failing with the
documented "required" error.
Require a non-BLANK value and normalize the padding, in the parsed branch as
well as raw_options so the two cannot drift apart -- a padded but real value
(" .myagent/cmds ") now resolves to ".myagent/cmds" rather than being rejected,
matching how other padded config references are normalized.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
GenericIntegration._resolve_commands_dirhas a parity gap between its two resolution branches:So
specify init --integration generic --integration-options="--commands-dir="(and--commands-dir "") resolves to"".init's own generic guard (if selected_ai == "generic" and not integration_options) is satisfied becauseintegration_optionsis the non-empty string"--commands-dir=", so it doesn't catch it.setup()then computesdest = (project_root / "").resolve() == project_root, passes the containment check, and writes every speckit command file (specify.md,plan.md,tasks.md, …) directly into the project root — silently bypassing the documented "--commands-dir is required" contract and polluting the repo root.Verified on
main(4d3a428):_resolve_commands_dir({}, {"raw_options": "--commands-dir="})returns""(the parsed-options path with the same empty value correctly raises).Fix
Apply the same non-empty guard the parsed branch already uses to the raw_options branch, so an empty value falls through to the existing
raise ValueError("--commands-dir is required …")on every input form (--commands-dir=,--commands-dir ""). Non-empty values resolve exactly as before — no behaviour change for valid usage.Verification
test_resolve_commands_dir_rejects_empty_raw_value(--commands-dir=,--commands-dir '',--commands-dir ""): fails before (DID NOT RAISE), passes after.test_resolve_commands_dir_accepts_nonempty_raw_value: non-empty raw values still resolve unchanged.TestGenericIntegration: 35 passed.ruffclean.AI-assisted: authored with Claude Code. I reproduced the project-root resolution on
main(empty raw value →"") and confirmed the parsed-options branch already guards it.