From 571008241ede19ed82a937b0aac9694551e6dfd5 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Mon, 20 Jul 2026 14:56:35 +0500 Subject: [PATCH 1/2] fix(workflows): reject a non-string 'integration'/'model' in command & prompt steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-string `integration` on a command or prompt step is passed to `get_integration()`, which uses it as a dict key: an unhashable list/dict raises a raw `TypeError` there — and because neither `validate()` nor `validate_workflow` checked the type, this crashes even a *validated* run, not just an unvalidated one. A non-string `model` likewise reaches `build_exec_args()` and is fed into the CLI argv. Guard both fields in `validate()` (reject a literal non-string, mirroring the existing 'command'/'prompt'/'input'/'options' checks) and in `execute()` (fail the step cleanly rather than take down the whole run, mirroring the 'input'/'options' guards). An explicit YAML-null (inherit the workflow default) and a "{{ ... }}" expression both stay valid. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/command/__init__.py | 44 +++++++ .../workflows/steps/prompt/__init__.py | 44 +++++++ tests/test_workflows.py | 110 ++++++++++++++++++ 3 files changed, 198 insertions(+) diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 7a6d893ed0..7e257c2119 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -61,6 +61,31 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if model and isinstance(model, str) and "{{" in model: model = evaluate_expression(model, context) + # A non-string integration/model — a literal list/dict/number that + # skipped validation, an unvalidated workflow-level default, or an + # expression that resolved to one — crashes downstream: get_integration() + # uses the value as a dict key (raw TypeError on an unhashable list/dict, + # even on a *validated* run) and build_exec_args() feeds model into the + # CLI argv. Fail the step with the contract error rather than taking down + # the whole run, mirroring the 'input'/'options' guards above. ``None`` + # stays valid — it means "unset" and falls back to dispatch-not-possible. + if integration is not None and not isinstance(integration, str): + return StepResult( + status=StepStatus.FAILED, + error=( + f"Command step {config.get('id', '?')!r}: 'integration' must " + f"be a string, got {type(integration).__name__}." + ), + ) + if model is not None and not isinstance(model, str): + return StepResult( + status=StepStatus.FAILED, + error=( + f"Command step {config.get('id', '?')!r}: 'model' must be a " + f"string, got {type(model).__name__}." + ), + ) + # Merge options (workflow defaults ← step overrides) options = dict(context.default_options) step_options = config.get("options", {}) @@ -191,4 +216,23 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"Command step {config.get('id', '?')!r}: 'options' must be a mapping." ) + # execute() passes 'integration' to get_integration(), which uses it as a + # dict key — a non-string (list/dict) raises a raw TypeError (unhashable), + # even on a validated run — and feeds 'model' into the CLI argv. Reject a + # literal non-string here, mirroring the sibling type checks. ``None`` + # (an explicit ``integration:``/``model:`` YAML null) means "inherit the + # workflow default" and stays valid; an expression like "{{ ... }}" is + # still a str, so it stays valid too. + integration = config.get("integration") + if integration is not None and not isinstance(integration, str): + errors.append( + f"Command step {config.get('id', '?')!r}: 'integration' must be a " + f"string, got {type(integration).__name__}." + ) + model = config.get("model") + if model is not None and not isinstance(model, str): + errors.append( + f"Command step {config.get('id', '?')!r}: 'model' must be a " + f"string, got {type(model).__name__}." + ) return errors diff --git a/src/specify_cli/workflows/steps/prompt/__init__.py b/src/specify_cli/workflows/steps/prompt/__init__.py index c8f8a32fd6..033937567d 100644 --- a/src/specify_cli/workflows/steps/prompt/__init__.py +++ b/src/specify_cli/workflows/steps/prompt/__init__.py @@ -52,6 +52,31 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if model and isinstance(model, str) and "{{" in model: model = evaluate_expression(model, context) + # A non-string integration/model — a literal list/dict/number that + # skipped validation, an unvalidated workflow-level default, or an + # expression that resolved to one — crashes downstream: get_integration() + # uses the value as a dict key (raw TypeError on an unhashable list/dict, + # even on a *validated* run) and build_exec_args() feeds model into the + # CLI argv. Fail the step with the contract error rather than taking down + # the whole run. ``None`` stays valid — it means "unset" and falls back + # to dispatch-not-possible. + if integration is not None and not isinstance(integration, str): + return StepResult( + status=StepStatus.FAILED, + error=( + f"Prompt step {config.get('id', '?')!r}: 'integration' must " + f"be a string, got {type(integration).__name__}." + ), + ) + if model is not None and not isinstance(model, str): + return StepResult( + status=StepStatus.FAILED, + error=( + f"Prompt step {config.get('id', '?')!r}: 'model' must be a " + f"string, got {type(model).__name__}." + ), + ) + # Attempt CLI dispatch dispatch_result = self._try_dispatch( prompt, integration, model, context @@ -172,4 +197,23 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Prompt step {config.get('id', '?')!r}: 'prompt' must be a " f"string, got {type(config['prompt']).__name__}." ) + # execute() passes 'integration' to get_integration(), which uses it as a + # dict key — a non-string (list/dict) raises a raw TypeError (unhashable), + # even on a validated run — and feeds 'model' into the CLI argv. Reject a + # literal non-string here, mirroring the 'prompt' check above. ``None`` + # (an explicit ``integration:``/``model:`` YAML null) means "inherit the + # workflow default" and stays valid; an expression like "{{ ... }}" is + # still a str, so it stays valid too. + integration = config.get("integration") + if integration is not None and not isinstance(integration, str): + errors.append( + f"Prompt step {config.get('id', '?')!r}: 'integration' must be a " + f"string, got {type(integration).__name__}." + ) + model = config.get("model") + if model is not None and not isinstance(model, str): + errors.append( + f"Prompt step {config.get('id', '?')!r}: 'model' must be a " + f"string, got {type(model).__name__}." + ) return errors diff --git a/tests/test_workflows.py b/tests/test_workflows.py index b1d9841535..167d2da0e6 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1026,6 +1026,63 @@ def test_validate_rejects_non_mapping_input_and_options(self): assert res_opt.status is StepStatus.FAILED assert "'options' must be a mapping" in (res_opt.error or "") + @pytest.mark.parametrize("bad", [["claude"], {"a": 1}, 5, True]) + def test_validate_rejects_non_string_integration_and_model(self, bad): + """A non-string 'integration'/'model' must be rejected at validation. + + execute() passes 'integration' to get_integration(), which uses it as a + dict key — an unhashable list/dict raises a raw TypeError there, even on + a validated run — and feeds 'model' into the CLI argv. Mirrors the + 'command'/'input'/'options' type checks. + """ + from specify_cli.workflows.steps.command import CommandStep + + step = CommandStep() + errs = step.validate({"id": "c", "command": "/x", "integration": bad}) + assert any("'integration' must be a string" in e for e in errs), bad + errs = step.validate({"id": "c", "command": "/x", "model": bad}) + assert any("'model' must be a string" in e for e in errs), bad + + def test_validate_accepts_none_and_expression_integration_model(self): + """An explicit YAML-null (inherit default) or a '{{ ... }}' expression + integration/model stays valid — only literal non-strings are rejected.""" + from specify_cli.workflows.steps.command import CommandStep + + step = CommandStep() + assert step.validate( + {"id": "c", "command": "/x", "integration": None, "model": None} + ) == [] + assert step.validate( + { + "id": "c", + "command": "/x", + "integration": "{{ inputs.agent }}", + "model": "{{ inputs.model }}", + } + ) == [] + + def test_execute_non_string_integration_fails_loudly(self): + """On an unvalidated run, an unhashable 'integration' would crash + get_integration() (dict.get on a list) with a raw TypeError. execute() + must fail the step with the contract error instead.""" + from specify_cli.workflows.steps.command import CommandStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = CommandStep() + res = step.execute( + {"id": "c", "command": "speckit.specify", "integration": ["claude"]}, + StepContext(), + ) + assert res.status is StepStatus.FAILED + assert "'integration' must be a string" in (res.error or "") + # non-string model likewise fails before build_exec_args + res = step.execute( + {"id": "c", "command": "speckit.specify", "integration": "claude", "model": ["m"]}, + StepContext(), + ) + assert res.status is StepStatus.FAILED + assert "'model' must be a string" in (res.error or "") + def test_step_override_integration(self): from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep @@ -1413,6 +1470,59 @@ def test_validate_accepts_expression_prompt(self): ) assert errors == [] + @pytest.mark.parametrize("bad", [["claude"], {"a": 1}, 5, True]) + def test_validate_rejects_non_string_integration_and_model(self, bad): + """A non-string 'integration'/'model' must be rejected at validation. + + execute() passes 'integration' to get_integration(), which uses it as a + dict key — an unhashable list/dict raises a raw TypeError there, even on + a validated run — and feeds 'model' into the CLI argv.""" + from specify_cli.workflows.steps.prompt import PromptStep + + step = PromptStep() + errs = step.validate({"id": "p", "prompt": "hi", "integration": bad}) + assert any("'integration' must be a string" in e for e in errs), bad + errs = step.validate({"id": "p", "prompt": "hi", "model": bad}) + assert any("'model' must be a string" in e for e in errs), bad + + def test_validate_accepts_none_and_expression_integration_model(self): + """An explicit YAML-null (inherit default) or a '{{ ... }}' expression + integration/model stays valid — only literal non-strings are rejected.""" + from specify_cli.workflows.steps.prompt import PromptStep + + step = PromptStep() + assert step.validate( + {"id": "p", "prompt": "hi", "integration": None, "model": None} + ) == [] + assert step.validate( + { + "id": "p", + "prompt": "hi", + "integration": "{{ inputs.agent }}", + "model": "{{ inputs.model }}", + } + ) == [] + + def test_execute_non_string_integration_fails_loudly(self): + """On an unvalidated run, an unhashable 'integration' would crash + get_integration() (dict.get on a dict) with a raw TypeError. execute() + must fail the step with the contract error instead.""" + from specify_cli.workflows.steps.prompt import PromptStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = PromptStep() + res = step.execute( + {"id": "p", "prompt": "hi", "integration": {"a": 1}}, StepContext() + ) + assert res.status is StepStatus.FAILED + assert "'integration' must be a string" in (res.error or "") + res = step.execute( + {"id": "p", "prompt": "hi", "integration": "claude", "model": ["m"]}, + StepContext(), + ) + assert res.status is StepStatus.FAILED + assert "'model' must be a string" in (res.error or "") + class TestShellStep: """Test the shell step type.""" From 563d0d55774a7bf9134758eade3b9cd337fc28af Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 22 Jul 2026 01:57:54 +0500 Subject: [PATCH 2/2] fix(workflows): route falsey non-string integration/model to the type guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review: `config.get("integration") or context.default_integration` (and the model equivalent) coerced a *falsey* non-string ([], {}, 0, False) into the workflow default before the type guard ran. On an unvalidated execute() such a step was silently accepted and — with a configured default — could dispatch using the wrong integration/model instead of failing with the contract error. Fall back to the workflow default only for genuinely-unset values (missing / YAML-null / empty string) so every non-string reaches the guard. Add parametrized falsey execute() cases ([], {}, 0, False) to both TestCommandStep and TestPromptStep; with the fix stashed all 8 fail (swallowed into the default). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/command/__init__.py | 19 +++++-- .../workflows/steps/prompt/__init__.py | 19 +++++-- tests/test_workflows.py | 50 +++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 7e257c2119..aa17a117b9 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -51,13 +51,24 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: for key, value in input_data.items(): resolved_input[key] = evaluate_expression(value, context) - # Resolve integration (step → workflow default → project default) - integration = config.get("integration") or context.default_integration + # Resolve integration (step → workflow default → project default). + # Fall back to the workflow default ONLY for a genuinely-unset value + # (missing / YAML-null / empty string). A ``config.get(...) or ...`` + # would also swallow a falsey *non-string* ([], {}, 0, False), coercing + # it to the default before the guard below runs — so on an unvalidated + # execute() such a step would silently dispatch with the configured + # default instead of failing. Fall through instead, so every non-string + # reaches the type guard. + integration = config.get("integration") + if integration is None or integration == "": + integration = context.default_integration if integration and isinstance(integration, str) and "{{" in integration: integration = evaluate_expression(integration, context) - # Resolve model - model = config.get("model") or context.default_model + # Resolve model (same fallback rationale as 'integration' above). + model = config.get("model") + if model is None or model == "": + model = context.default_model if model and isinstance(model, str) and "{{" in model: model = evaluate_expression(model, context) diff --git a/src/specify_cli/workflows/steps/prompt/__init__.py b/src/specify_cli/workflows/steps/prompt/__init__.py index 033937567d..e81ca175dc 100644 --- a/src/specify_cli/workflows/steps/prompt/__init__.py +++ b/src/specify_cli/workflows/steps/prompt/__init__.py @@ -42,13 +42,24 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if not isinstance(prompt, str): prompt = str(prompt) - # Resolve integration (step → workflow default) - integration = config.get("integration") or context.default_integration + # Resolve integration (step → workflow default). + # Fall back to the workflow default ONLY for a genuinely-unset value + # (missing / YAML-null / empty string). A ``config.get(...) or ...`` + # would also swallow a falsey *non-string* ([], {}, 0, False), coercing + # it to the default before the guard below runs — so on an unvalidated + # execute() such a step would silently dispatch with the configured + # default instead of failing. Fall through instead, so every non-string + # reaches the type guard. + integration = config.get("integration") + if integration is None or integration == "": + integration = context.default_integration if integration and isinstance(integration, str) and "{{" in integration: integration = evaluate_expression(integration, context) - # Resolve model - model = config.get("model") or context.default_model + # Resolve model (same fallback rationale as 'integration' above). + model = config.get("model") + if model is None or model == "": + model = context.default_model if model and isinstance(model, str) and "{{" in model: model = evaluate_expression(model, context) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 167d2da0e6..5e1394750c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1083,6 +1083,33 @@ def test_execute_non_string_integration_fails_loudly(self): assert res.status is StepStatus.FAILED assert "'model' must be a string" in (res.error or "") + @pytest.mark.parametrize("falsey", [[], {}, 0, False]) + def test_execute_falsey_non_string_integration_fails_loudly(self, falsey): + """A *falsey* non-string ([], {}, 0, False) must fail the step, not be + swallowed by an ``or``-fallback to the workflow default. + + A ``config.get('integration') or context.default_integration`` coerces a + falsey non-string to the default *before* the type guard runs, so with a + configured default the step would silently dispatch using the wrong + integration instead of surfacing the contract error. The default is set + here so a regression dispatches rather than fails-not-possible.""" + from specify_cli.workflows.steps.command import CommandStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = CommandStep() + ctx = StepContext(default_integration="claude", default_model="sonnet") + res = step.execute( + {"id": "c", "command": "speckit.specify", "integration": falsey}, ctx + ) + assert res.status is StepStatus.FAILED, falsey + assert "'integration' must be a string" in (res.error or ""), falsey + # a falsey non-string model likewise reaches the guard + res = step.execute( + {"id": "c", "command": "speckit.specify", "model": falsey}, ctx + ) + assert res.status is StepStatus.FAILED, falsey + assert "'model' must be a string" in (res.error or ""), falsey + def test_step_override_integration(self): from unittest.mock import patch from specify_cli.workflows.steps.command import CommandStep @@ -1523,6 +1550,29 @@ def test_execute_non_string_integration_fails_loudly(self): assert res.status is StepStatus.FAILED assert "'model' must be a string" in (res.error or "") + @pytest.mark.parametrize("falsey", [[], {}, 0, False]) + def test_execute_falsey_non_string_integration_fails_loudly(self, falsey): + """A *falsey* non-string ([], {}, 0, False) must fail the step, not be + swallowed by an ``or``-fallback to the workflow default. + + A ``config.get('integration') or context.default_integration`` coerces a + falsey non-string to the default *before* the type guard runs, so with a + configured default the step would silently dispatch using the wrong + integration instead of surfacing the contract error. The default is set + here so a regression dispatches rather than fails-not-possible.""" + from specify_cli.workflows.steps.prompt import PromptStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = PromptStep() + ctx = StepContext(default_integration="claude", default_model="sonnet") + res = step.execute({"id": "p", "prompt": "hi", "integration": falsey}, ctx) + assert res.status is StepStatus.FAILED, falsey + assert "'integration' must be a string" in (res.error or ""), falsey + # a falsey non-string model likewise reaches the guard + res = step.execute({"id": "p", "prompt": "hi", "model": falsey}, ctx) + assert res.status is StepStatus.FAILED, falsey + assert "'model' must be a string" in (res.error or ""), falsey + class TestShellStep: """Test the shell step type."""