mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(workflows): guard a non-string overlay edit 'operation' (#3881)
`_parse_edit` reads `operation` straight from hand-edited YAML and then
does `if operation not in VALID_OPERATIONS`. `VALID_OPERATIONS` is a
frozenset, so that membership test hashes the value — and an unhashable
one raises:
operation={'insert_after': 'a'} -> TypeError: unhashable type: 'dict'
operation=['insert_after'] -> TypeError: unhashable type: 'list'
`validate_overlay_yaml`'s docstring promises "validation never raises",
and nothing upstream catches TypeError (layer_sources wraps only
YAMLError/OSError/UnicodeDecodeError; _commands catches only ValueError),
so the CLI dies with a raw traceback instead of reporting the error.
The trigger is an ordinary authoring mistake: nesting the recommended
shorthand form under the explicit key.
Every other field in the same function is isinstance-guarded first
(`anchor`, `step`, `step["id"]`); `operation` was the outlier. Check the
type first and return the message the function already uses for
`operation: None` / `operation: 7`.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fd8cc6da22
commit
3451a21277
@@ -87,7 +87,12 @@ def _parse_edit(edit_raw: dict[str, Any], idx: int) -> tuple[OverlayEdit | None,
|
||||
else:
|
||||
return None, f"Edit at index {idx} has no operation; expected one of {sorted(VALID_OPERATIONS)}."
|
||||
|
||||
if operation not in VALID_OPERATIONS:
|
||||
# ``operation`` comes straight from hand-edited YAML, so it may be an
|
||||
# unhashable mapping/sequence (``operation: {insert_after: a}`` when the
|
||||
# shorthand form is nested by mistake). Membership-testing an unhashable
|
||||
# value against the frozenset raises TypeError, which would escape this
|
||||
# never-raising validator; check the type first, like 'anchor' below.
|
||||
if not isinstance(operation, str) or operation not in VALID_OPERATIONS:
|
||||
return None, f"Edit at index {idx} has invalid operation {operation!r}."
|
||||
|
||||
if not isinstance(anchor, str) or not anchor:
|
||||
|
||||
@@ -136,6 +136,44 @@ class TestShorthandEdits:
|
||||
assert overlay is None
|
||||
assert any("operation" in e.lower() for e in errors), errors
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"operation",
|
||||
[
|
||||
{"insert_after": "a"},
|
||||
["insert_after"],
|
||||
],
|
||||
)
|
||||
def test_non_string_operation_rejected_without_raising(self, operation):
|
||||
"""An unhashable 'operation' must be reported, not raised.
|
||||
|
||||
`VALID_OPERATIONS` is a frozenset, so `operation not in ...` hashes the
|
||||
value. Nesting the shorthand form under the explicit key by mistake
|
||||
(`operation: {insert_after: a}`) therefore raised
|
||||
`TypeError: unhashable type: 'dict'` out of a validator whose docstring
|
||||
promises "validation never raises" — and nothing upstream catches
|
||||
TypeError, so the CLI died with a raw traceback.
|
||||
"""
|
||||
overlay, errors = validate_overlay_yaml(
|
||||
{
|
||||
"id": "ov",
|
||||
"extends": "wf",
|
||||
"priority": 10,
|
||||
"edits": [
|
||||
{
|
||||
"operation": operation,
|
||||
"anchor": "a",
|
||||
"step": {
|
||||
"id": "b",
|
||||
"type": "command",
|
||||
"command": "echo",
|
||||
},
|
||||
}
|
||||
],
|
||||
}
|
||||
)
|
||||
assert overlay is None
|
||||
assert any("invalid operation" in err for err in errors), errors
|
||||
|
||||
def test_shorthand_and_explicit_mixed_list(self):
|
||||
overlay, errors = validate_overlay_yaml(
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user