fix(workflows): reject a non-string 'integration'/'model' in command & prompt steps#3597
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
Conversation
…& prompt steps
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds validation and runtime guards for non-string command/prompt integration and model values.
Changes:
- Rejects invalid literal values during validation.
- Returns failed step results for invalid resolved values.
- Adds validation and execution tests.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/steps/command/__init__.py |
Validates command dispatch fields. |
src/specify_cli/workflows/steps/prompt/__init__.py |
Validates prompt dispatch fields. |
tests/test_workflows.py |
Tests new validation and failure behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
+64
to
+65
| # A non-string integration/model — a literal list/dict/number that | ||
| # skipped validation, an unvalidated workflow-level default, or an |
Comment on lines
+55
to
+56
| # A non-string integration/model — a literal list/dict/number that | ||
| # skipped validation, an unvalidated workflow-level default, or an |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A command or prompt step whose
integration:(ormodel:) is a non-string crashes the run with a rawTypeErrorinstead of a clean validation/step error.In
execute(), the resolvedintegrationvalue is passed toget_integration(), which doesINTEGRATION_REGISTRY.get(key)— using the value as a dict key. An unhashable value (list/dict) raisesTypeError: unhashable typeright there:Crucially this is not limited to unvalidated runs: neither the step's
validate()norvalidate_workflowchecked the type ofintegration/model, sovalidate()returns[]forintegration: [claude]and the crash survives validation. A non-stringmodelsimilarly reachesbuild_exec_args()and would be fed into the CLI argv.This is the same recurring shape as the existing
command/prompt/input/optionsguards — a string-typed dispatch field thatexecute()passes downstream without a type check.Fix
Both
commandandpromptsteps:validate()— reject a literal non-stringintegration/model, mirroring the existing sibling type checks. An explicit YAML-null (integration:→ inherit the workflow default) and a"{{ ... }}"expression both stay valid.execute()— guard the resolved value (after theor-fallback to the workflow default and expression evaluation) and return a cleanFAILEDStepResultrather than letting the run crash. This also catches an expression that resolves to a non-string and an unvalidated workflow-level default.Tests
Added parametrized
validate()rejection tests, "accepts None / expression" tests, andexecute()-fails-loudly tests to bothTestCommandStepandTestPromptStep. Test-the-test: with the fix stashed, the new tests fail — including the rawTypeError: unhashable typetraceback atget_integration. Fulltests/test_workflows.pyshows no new failures (the 20 pre-existing failures are all Windows symlink-guard tests, identical on a clean tree).🤖 Generated with Claude Code