close
Skip to content

Commit d0ba806

Browse files
howbazaarclaude
andcommitted
fix: Validate boolean CLI options instead of coercing them
Greptile caught that `--auto-update=treu` became `autoUpdate: false` rather than an error, which either turns auto-update off on an update or fails the mutation outright on a dynamic collection. Constrain these options with click.Choice so a bad value fails at parse time. `--cascade` on `repository remove` had the same flaw -- and worse, silently skipped the cascade -- so it gets the same treatment rather than being left inconsistent with the option this branch adds. The parser and choice type are now shared in utils. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent bf3559f commit d0ba806

6 files changed

Lines changed: 77 additions & 22 deletions

File tree

‎NEWS.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@
3131

3232
### Fixes
3333

34+
- `repository remove --cascade` now rejects a value that isn't `true` or `false`.
35+
Previously anything unrecognised was taken as `false`, so a typo silently skipped
36+
the cascade instead of reporting the mistake.
37+
3438
---
3539

3640
## August 10, 2026

‎stacklet/client/platform/graphql/snippets/policy_collection.py‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Copyright Stacklet, Inc.
22
# SPDX-License-Identifier: Apache-2.0
33

4+
from ...utils import BOOL_CHOICE, to_bool
45
from ..snippet import GraphQLSnippet
56

67
# Shared selection for a policy collection node. `repositoryConfig`/`repositoryView`
@@ -82,19 +83,12 @@
8283
"policy_directory": "[String!]",
8384
}
8485

85-
86-
def to_bool(value: str | None) -> bool | None:
87-
"""
88-
Parse an optional boolean option.
89-
90-
None has to survive as None: variables are transformed before the query is built,
91-
and the builder drops an option's line only when its value is None. Coercing an
92-
unset flag to False would send it, and `autoUpdate: false` is an error on a
93-
dynamic collection rather than a no-op.
94-
"""
95-
if value is None:
96-
return None
97-
return value.lower() in ("true", "t", "yes", "y")
86+
AUTO_UPDATE_OPTION = {
87+
"auto_update": {
88+
"help": "Bump policies to their latest version as they are scanned",
89+
"type": BOOL_CHOICE,
90+
},
91+
}
9892

9993

10094
class ListPolicyCollections(GraphQLSnippet):
@@ -172,7 +166,7 @@ class AddPolicyCollection(GraphQLSnippet):
172166

173167
optional = {
174168
"description": "Policy Collection Description",
175-
"auto_update": "Bump policies to their latest version as they are scanned (true|false)",
169+
**AUTO_UPDATE_OPTION,
176170
"repository_uuid": (
177171
"Repository config UUID. Setting it makes this a dynamic collection, whose "
178172
"policies always match the latest scan of that repository"
@@ -216,7 +210,7 @@ class UpdatePolicyCollection(GraphQLSnippet):
216210
"name": "Policy Collection Name in Stacklet",
217211
"provider": "Cloud Provider",
218212
"description": "Policy Collection Description",
219-
"auto_update": "Bump policies to their latest version as they are scanned (true|false)",
213+
**AUTO_UPDATE_OPTION,
220214
**VIEW_OPTIONS,
221215
}
222216
parameter_types = dict(VIEW_TYPES)

‎stacklet/client/platform/graphql/snippets/repository.py‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Copyright Stacklet, Inc.
22
# SPDX-License-Identifier: Apache-2.0
33

4+
from ...utils import BOOL_CHOICE, to_bool
45
from ..snippet import GraphQLSnippet
56

67

@@ -141,15 +142,12 @@ class RemoveRepository(GraphQLSnippet):
141142
}
142143
optional = {
143144
"cascade": {
144-
"help": (
145-
"Also remove bindings and policy collections tied to this repository (true|false)"
146-
),
145+
"help": "Also remove bindings and policy collections tied to this repository",
147146
"default": "false",
147+
"type": BOOL_CHOICE,
148148
},
149149
}
150-
variable_transformers = {
151-
"cascade": lambda x: x is not None and x.lower() in ("true", "t", "yes", "y")
152-
}
150+
variable_transformers = {"cascade": to_bool}
153151
result_expr = "data.removeRepositoryConfig"
154152

155153

‎stacklet/client/platform/utils.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,26 @@
1010

1111
USER_AGENT = f"stacklet.client.platform/{__version__}"
1212

13+
# Type for an optional boolean option. Constraining the values matters: these are sent
14+
# on to the API, where a wrong one is a rejected mutation or a silently different
15+
# setting rather than something the user gets told about.
16+
BOOL_CHOICE = click.Choice(["true", "false"], case_sensitive=False)
17+
18+
19+
def to_bool(value: str | None) -> bool | None:
20+
"""
21+
Parse an optional boolean option, as constrained by BOOL_CHOICE.
22+
23+
None has to survive as None: variables are transformed before the query is built,
24+
and the builder drops an option's line only when its value is None. Coercing an
25+
unset option to False would send it, and `autoUpdate: false` is an error on a
26+
dynamic policy collection rather than a no-op.
27+
"""
28+
if value is None:
29+
return None
30+
return value.lower() == "true"
31+
32+
1333
PAGINATION_OPTIONS = {
1434
"first": {
1535
"help": "For use with pagination. Return the first n results.",

‎tests/test_policy_collection.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,32 @@ def test_add_auto_update_given(self, run_query):
111111
pc_uuid = str(uuid.uuid4())
112112
res, body = run_query(
113113
"policy-collection",
114-
["add", "--name=c", "--provider=AWS", "--auto-update=true"],
114+
["add", "--name=c", "--provider=AWS", "--auto-update=TRUE"],
115115
collection_response("addPolicyCollection", pc_uuid),
116116
)
117117
assert body["variables"]["auto_update"] is True
118118
assert_query_contains(body, "$auto_update: Boolean!")
119119

120+
def test_add_auto_update_false(self, run_query):
121+
pc_uuid = str(uuid.uuid4())
122+
res, body = run_query(
123+
"policy-collection",
124+
["add", "--name=c", "--provider=AWS", "--auto-update=false"],
125+
collection_response("addPolicyCollection", pc_uuid),
126+
)
127+
assert body["variables"]["auto_update"] is False
128+
129+
def test_add_auto_update_rejects_nonsense(self, run_queries):
130+
"A misspelling has to fail, not quietly turn into false."
131+
res, bodies = run_queries(
132+
"policy-collection",
133+
["add", "--name=c", "--provider=AWS", "--auto-update=treu"],
134+
[],
135+
)
136+
assert res.exit_code != 0
137+
assert "'treu' is not one of 'true', 'false'" in res.output
138+
assert bodies == []
139+
120140
def test_add_view_option_without_repository(self, run_queries):
121141
"View options are meaningless without a repository, so say so up front."
122142
res, bodies = run_queries(

‎tests/test_repository.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,25 @@ def test_add_repository_with_auth(self, run_query):
250250
"auth_token": "sometoken",
251251
}
252252

253+
def test_remove_repository_cascade(self, run_query):
254+
res, body = run_query(
255+
"repository",
256+
["remove", f"--uuid={REPO_UUID}", "--cascade=true"],
257+
response={"data": {"removeRepositoryConfig": {"removed": [], "problems": []}}},
258+
)
259+
assert body["variables"] == {"uuid": REPO_UUID, "cascade": True}
260+
261+
def test_remove_repository_cascade_rejects_nonsense(self, run_queries):
262+
"A misspelling has to fail, not quietly leave the cascade off."
263+
res, bodies = run_queries(
264+
"repository",
265+
["remove", f"--uuid={REPO_UUID}", "--cascade=treu"],
266+
[],
267+
)
268+
assert res.exit_code != 0
269+
assert "'treu' is not one of 'true', 'false'" in res.output
270+
assert bodies == []
271+
253272
def test_process_repository(self, run_query):
254273
res, body = run_query(
255274
"repository",

0 commit comments

Comments
 (0)