Skip to content

Commit 84fa06a

Browse files
jawwad-aliclaude
andcommitted
fix(integrations): use strip() only to test blankness, return the value verbatim
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) <noreply@anthropic.com>
1 parent 49de444 commit 84fa06a

2 files changed

Lines changed: 23 additions & 16 deletions

File tree

‎src/specify_cli/integrations/generic/__init__.py‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,16 @@ def _resolve_commands_dir(
5353
"""
5454
parsed_options = parsed_options or {}
5555

56-
# Accept a value only when it is non-BLANK, and normalize the padding.
57-
# An empty value resolves to the project root (``project_root / ""``),
58-
# and a whitespace-only one to a directory literally named " ", so
59-
# either would silently scatter command files instead of failing with
60-
# the documented "required" error. Both branches below apply the same
61-
# rule so they cannot drift apart.
56+
# Accept a value only when it is non-BLANK. An empty value resolves to
57+
# the project root (``project_root / ""``) and a whitespace-only one to
58+
# a directory literally named " ", so either would silently scatter
59+
# command files instead of failing with the documented "required"
60+
# error. ``strip()`` is used ONLY to decide blankness -- the value
61+
# itself is returned verbatim, so a deliberate (if unusual) padded
62+
# directory name still targets exactly what the user asked for. Both
63+
# branches below apply the same rule so they cannot drift apart.
6264
commands_dir = parsed_options.get("commands_dir")
63-
if isinstance(commands_dir, str):
64-
commands_dir = commands_dir.strip()
65-
if commands_dir:
65+
if commands_dir and (not isinstance(commands_dir, str) or commands_dir.strip()):
6666
return commands_dir
6767

6868
# Fall back to raw_options (--integration-options="--commands-dir ...")
@@ -72,12 +72,12 @@ def _resolve_commands_dir(
7272
tokens = shlex.split(raw)
7373
for i, token in enumerate(tokens):
7474
if token == "--commands-dir" and i + 1 < len(tokens):
75-
candidate = tokens[i + 1].strip()
76-
if candidate:
75+
candidate = tokens[i + 1]
76+
if candidate.strip():
7777
return candidate
7878
if token.startswith("--commands-dir="):
79-
candidate = token.split("=", 1)[1].strip()
80-
if candidate:
79+
candidate = token.split("=", 1)[1]
80+
if candidate.strip():
8181
return candidate
8282

8383
raise ValueError(

‎tests/integrations/test_integration_generic.py‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,20 @@ def test_resolve_commands_dir_rejects_blank_raw_value(self, raw):
7575
GenericIntegration._resolve_commands_dir({}, {"raw_options": raw})
7676

7777
@pytest.mark.parametrize("padded", [" .myagent/cmds ", "\t.myagent/cmds"])
78-
def test_resolve_commands_dir_strips_padding(self, padded):
79-
"""A padded but real value is normalized rather than rejected."""
78+
def test_resolve_commands_dir_returns_padded_value_verbatim(self, padded):
79+
"""A padded but non-blank value is accepted and returned UNCHANGED: the
80+
blankness test uses strip(), but rewriting the value would silently
81+
retarget a directory the user asked for by name."""
8082
from specify_cli.integrations.generic import GenericIntegration
8183

8284
assert GenericIntegration._resolve_commands_dir(
8385
{"commands_dir": padded}, {}
84-
) == ".myagent/cmds"
86+
) == padded
87+
# Quoted in raw_options, since shlex.split() would otherwise consume the
88+
# surrounding whitespace before this code ever sees it.
89+
assert GenericIntegration._resolve_commands_dir(
90+
{}, {"raw_options": f"--commands-dir='{padded}'"}
91+
) == padded
8592

8693
@pytest.mark.parametrize("raw", ["--commands-dir=", "--commands-dir ''", '--commands-dir ""'])
8794
def test_resolve_commands_dir_rejects_empty_raw_value(self, raw):

0 commit comments

Comments
 (0)