From 3cdc830862ba8f1c61cc93aecd8a40df1c9b08f2 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 24 Jul 2026 14:35:43 +0500 Subject: [PATCH 1/3] fix(integrations): reject empty --commands-dir in generic raw_options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../integrations/generic/__init__.py | 13 ++++++++++-- .../integrations/test_integration_generic.py | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/integrations/generic/__init__.py b/src/specify_cli/integrations/generic/__init__.py index a2fd430f75..fae4fcb5d4 100644 --- a/src/specify_cli/integrations/generic/__init__.py +++ b/src/specify_cli/integrations/generic/__init__.py @@ -63,10 +63,19 @@ def _resolve_commands_dir( import shlex tokens = shlex.split(raw) for i, token in enumerate(tokens): + # Only accept a NON-EMPTY value, matching the parsed-options + # branch above (`if commands_dir:`). An empty value + # (``--commands-dir=`` or ``--commands-dir ""``) must fall + # through to the "required" error below, not be returned + # verbatim — otherwise it resolves to the project root and + # command files get written there instead of a dedicated dir. if token == "--commands-dir" and i + 1 < len(tokens): - return tokens[i + 1] + if tokens[i + 1]: + return tokens[i + 1] if token.startswith("--commands-dir="): - return token.split("=", 1)[1] + candidate = token.split("=", 1)[1] + if candidate: + return candidate raise ValueError( "--commands-dir is required for the generic integration" diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 3f079c438d..a00b0d907f 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -55,6 +55,27 @@ def test_setup_requires_nonempty_commands_dir(self, tmp_path): with pytest.raises(ValueError, match="--commands-dir is required"): i.setup(tmp_path, m, parsed_options={"commands_dir": ""}) + @pytest.mark.parametrize("raw", ["--commands-dir=", "--commands-dir ''", '--commands-dir ""']) + def test_resolve_commands_dir_rejects_empty_raw_value(self, raw): + """An empty --commands-dir in raw_options must raise the same "required" + error as the parsed-options path — not return "" (which resolves to the + project root and writes command files there). Mirrors the parsed branch.""" + from specify_cli.integrations.generic import GenericIntegration + + with pytest.raises(ValueError, match="--commands-dir is required"): + GenericIntegration._resolve_commands_dir({}, {"raw_options": raw}) + + def test_resolve_commands_dir_accepts_nonempty_raw_value(self): + """A non-empty raw --commands-dir still resolves unchanged.""" + from specify_cli.integrations.generic import GenericIntegration + + assert GenericIntegration._resolve_commands_dir( + {}, {"raw_options": "--commands-dir .myagent/commands"} + ) == ".myagent/commands" + assert GenericIntegration._resolve_commands_dir( + {}, {"raw_options": "--commands-dir=.myagent/commands"} + ) == ".myagent/commands" + def test_setup_writes_to_correct_directory(self, tmp_path): i = get_integration("generic") m = IntegrationManifest("generic", tmp_path) From b69d1a1dcce51bd7478cc5dad3240412aaff48fa Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 10:27:13 +0500 Subject: [PATCH 2/3] fix(integrations): reject a BLANK --commands-dir, not just an empty one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../integrations/generic/__init__.py | 21 ++++++++------ .../integrations/test_integration_generic.py | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/integrations/generic/__init__.py b/src/specify_cli/integrations/generic/__init__.py index fae4fcb5d4..0b745f8397 100644 --- a/src/specify_cli/integrations/generic/__init__.py +++ b/src/specify_cli/integrations/generic/__init__.py @@ -53,7 +53,15 @@ def _resolve_commands_dir( """ parsed_options = parsed_options or {} + # Accept a value only when it is non-BLANK, and normalize the padding. + # An empty value resolves to the project root (``project_root / ""``), + # and a whitespace-only one to a directory literally named " ", so + # either would silently scatter command files instead of failing with + # the documented "required" error. Both branches below apply the same + # rule so they cannot drift apart. commands_dir = parsed_options.get("commands_dir") + if isinstance(commands_dir, str): + commands_dir = commands_dir.strip() if commands_dir: return commands_dir @@ -63,17 +71,12 @@ def _resolve_commands_dir( import shlex tokens = shlex.split(raw) for i, token in enumerate(tokens): - # Only accept a NON-EMPTY value, matching the parsed-options - # branch above (`if commands_dir:`). An empty value - # (``--commands-dir=`` or ``--commands-dir ""``) must fall - # through to the "required" error below, not be returned - # verbatim — otherwise it resolves to the project root and - # command files get written there instead of a dedicated dir. if token == "--commands-dir" and i + 1 < len(tokens): - if tokens[i + 1]: - return tokens[i + 1] + candidate = tokens[i + 1].strip() + if candidate: + return candidate if token.startswith("--commands-dir="): - candidate = token.split("=", 1)[1] + candidate = token.split("=", 1)[1].strip() if candidate: return candidate diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index a00b0d907f..11dfe0ee7e 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -55,6 +55,34 @@ def test_setup_requires_nonempty_commands_dir(self, tmp_path): with pytest.raises(ValueError, match="--commands-dir is required"): i.setup(tmp_path, m, parsed_options={"commands_dir": ""}) + @pytest.mark.parametrize("blank", [" ", "\t"]) + def test_resolve_commands_dir_rejects_blank_parsed_value(self, blank): + """A whitespace-only value must raise too: it resolves to a directory + literally named " ", scattering command files just like the empty case.""" + from specify_cli.integrations.generic import GenericIntegration + + with pytest.raises(ValueError, match="--commands-dir is required"): + GenericIntegration._resolve_commands_dir({"commands_dir": blank}, {}) + + @pytest.mark.parametrize( + "raw", ["--commands-dir ' '", "--commands-dir=' '", "--commands-dir '\t'"] + ) + def test_resolve_commands_dir_rejects_blank_raw_value(self, raw): + """Same rule on the raw_options branch, so the two cannot drift apart.""" + from specify_cli.integrations.generic import GenericIntegration + + with pytest.raises(ValueError, match="--commands-dir is required"): + GenericIntegration._resolve_commands_dir({}, {"raw_options": raw}) + + @pytest.mark.parametrize("padded", [" .myagent/cmds ", "\t.myagent/cmds"]) + def test_resolve_commands_dir_strips_padding(self, padded): + """A padded but real value is normalized rather than rejected.""" + from specify_cli.integrations.generic import GenericIntegration + + assert GenericIntegration._resolve_commands_dir( + {"commands_dir": padded}, {} + ) == ".myagent/cmds" + @pytest.mark.parametrize("raw", ["--commands-dir=", "--commands-dir ''", '--commands-dir ""']) def test_resolve_commands_dir_rejects_empty_raw_value(self, raw): """An empty --commands-dir in raw_options must raise the same "required" From 818a19c2e8fabc84be14af66c3301c91ad4f15c0 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 28 Jul 2026 10:31:31 +0500 Subject: [PATCH 3/3] fix(integrations): use strip() only to test blankness, return the value verbatim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: normalizing with strip() changed EXISTING valid values, contrary to this PR's "no behaviour change for valid usage" claim -- a quoted `--commands-dir ' commands '` previously targeted the literal ` commands ` directory and would have started writing to `commands` instead. The blankness test still uses strip(), but the accepted value is now returned unchanged, so the fix stays limited to empty/blank input. Test updated accordingly: a padded non-blank value must round-trip verbatim (quoted in raw_options, since shlex.split() consumes unquoted padding before this code sees it). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- .../integrations/generic/__init__.py | 26 +++++++++---------- .../integrations/test_integration_generic.py | 13 +++++++--- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/integrations/generic/__init__.py b/src/specify_cli/integrations/generic/__init__.py index 0b745f8397..f8ea47ccb2 100644 --- a/src/specify_cli/integrations/generic/__init__.py +++ b/src/specify_cli/integrations/generic/__init__.py @@ -53,16 +53,16 @@ def _resolve_commands_dir( """ parsed_options = parsed_options or {} - # Accept a value only when it is non-BLANK, and normalize the padding. - # An empty value resolves to the project root (``project_root / ""``), - # and a whitespace-only one to a directory literally named " ", so - # either would silently scatter command files instead of failing with - # the documented "required" error. Both branches below apply the same - # rule so they cannot drift apart. + # Accept a value only when it is non-BLANK. An empty value resolves to + # the project root (``project_root / ""``) and a whitespace-only one to + # a directory literally named " ", so either would silently scatter + # command files instead of failing with the documented "required" + # error. ``strip()`` is used ONLY to decide blankness -- the value + # itself is returned verbatim, so a deliberate (if unusual) padded + # directory name still targets exactly what the user asked for. Both + # branches below apply the same rule so they cannot drift apart. commands_dir = parsed_options.get("commands_dir") - if isinstance(commands_dir, str): - commands_dir = commands_dir.strip() - if commands_dir: + if commands_dir and (not isinstance(commands_dir, str) or commands_dir.strip()): return commands_dir # Fall back to raw_options (--integration-options="--commands-dir ...") @@ -72,12 +72,12 @@ def _resolve_commands_dir( tokens = shlex.split(raw) for i, token in enumerate(tokens): if token == "--commands-dir" and i + 1 < len(tokens): - candidate = tokens[i + 1].strip() - if candidate: + candidate = tokens[i + 1] + if candidate.strip(): return candidate if token.startswith("--commands-dir="): - candidate = token.split("=", 1)[1].strip() - if candidate: + candidate = token.split("=", 1)[1] + if candidate.strip(): return candidate raise ValueError( diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 11dfe0ee7e..202f7ab3dd 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -75,13 +75,20 @@ def test_resolve_commands_dir_rejects_blank_raw_value(self, raw): GenericIntegration._resolve_commands_dir({}, {"raw_options": raw}) @pytest.mark.parametrize("padded", [" .myagent/cmds ", "\t.myagent/cmds"]) - def test_resolve_commands_dir_strips_padding(self, padded): - """A padded but real value is normalized rather than rejected.""" + def test_resolve_commands_dir_returns_padded_value_verbatim(self, padded): + """A padded but non-blank value is accepted and returned UNCHANGED: the + blankness test uses strip(), but rewriting the value would silently + retarget a directory the user asked for by name.""" from specify_cli.integrations.generic import GenericIntegration assert GenericIntegration._resolve_commands_dir( {"commands_dir": padded}, {} - ) == ".myagent/cmds" + ) == padded + # Quoted in raw_options, since shlex.split() would otherwise consume the + # surrounding whitespace before this code ever sees it. + assert GenericIntegration._resolve_commands_dir( + {}, {"raw_options": f"--commands-dir='{padded}'"} + ) == padded @pytest.mark.parametrize("raw", ["--commands-dir=", "--commands-dir ''", '--commands-dir ""']) def test_resolve_commands_dir_rejects_empty_raw_value(self, raw):