You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
SwitchStep.validate checks only that expression is present (steps/switch/__init__.py:105). The field goes through the same evaluate_expression as a condition, so one written without braces comes back as its own source text.
Measured on main (27f50f7), inputs.mode = "review", cases review / build, plus a default:
It matches no case key, falls through to default on every run — or, with no default:, dispatches nothing at all — and still reports COMPLETED. That is exactly the "silent empty result + COMPLETED" wiring bug this file's own cases: guard was written to prevent, quoting its comment, on the field one line above it.
if, while and do-while all run condition_is_never_evaluated and condition_has_malformed_expression_block on their condition. switch ran neither on its expression.
The fix
Reuse those two predicates rather than write a third scan, with switch-appropriate wording.
Only those two apply, deliberately. A switch matches on strings, so a composite key is legitimate here even though the same shape is a fault in a boolean condition:
expression
flagged?
why
inputs.mode
yes
never evaluated
{{ inputs.x
yes
unclosable block
{{ inputs.missing | default('oops }}
yes
raw-close fallback truncates it
{{ inputs.mode }}
no
the ordinary form
{{ inputs.a }}-{{ inputs.b }}
no
composite case key
true, ""
no
ordinary case keys
The last two rows are the condition-specific exemptions condition_is_never_evaluated already carries. They are harmless on a condition and actively correct here, which is what makes these two predicates safe to reuse on a non-boolean field. There is a test pinning that boundary so a later narrowing cannot quietly reject composite keys.
This is independent of #4292, deliberately: that PR adds a predicate for a condition holding more than one block, which is precisely the shape a switch is allowed to have. It is not applied here.
Identical 22 failures on both sides, diff clean — the pre-existing symlink and bash-parity classes on unelevated Windows. 1728 → 1731 is exactly the three cases added.
Mutation-checked, after confirming the edit applied: disabling the never-evaluated branch fails 2 of the 3 new cases, and the composite-key case stays green under it — which is the point, since it does not depend on the branch and still guards the other side.
Tests
tests/test_workflows.py::TestSwitchStep — three cases: the braceless form (asserting the current runtime behaviour first, then the validator), both unclosable shapes, and the composite-key/literal boundary that must stay accepted.
AI disclosure
Per CONTRIBUTING: this pull request (code, tests and description) was developed with Claude Code as a coding agent.
The reason will be displayed to describe this comment to others. Learn more.
Right: reusing the condition predicate was wrong for a switch. A condition is coerced by bool(), so any braceless text is always true there. A switch matches its value against case keys, and a case key is a literal, so expression: review is a valid (if constant) switch and whitespace strips to the "" key.
Fixed in 2554a12. switch_expression_is_never_evaluated flags braceless text only when it opens by walking into a root _build_namespace supplies (inputs.mode, item[0], context.run_id, ...), and keeps the verbatim-unclosable {{ case.
test_a_literal_expression_stays_accepted runs review, whitespace, approve me and a bare inputs through execute() first, asserts the case each one actually dispatches, then asserts validate() accepts it. inputsX.mode is accepted too: a name that merely starts like a root is not a reference into it.
test_every_namespace_root_written_without_a_block_is_rejected covers every root, plus a filter, a comparison and surrounding whitespace.
@mnriem Copilot's point is addressed in 2554a12 (details in the inline reply), and the branch is rebased onto current main. Ready for re-review.
Evidence:
TestSwitchStep: 24 passed.
Mutations, each one reverted afterwards: restoring the condition predicate fails the literal test; making the braceless branch always false fails 2 tests; dropping the ./[ requirement after the root fails 1.
uvx ruff@0.15.0 check src tests: clean.
Full tests/test_workflows.py + tests/unit on Windows: 22 failures, all symlink-privilege errors (WinError 1314). The same test fails identically on a clean checkout of main.
AI disclosure, per CONTRIBUTING: this change, its tests and this comment were written with Claude Code as a coding agent; the runs above were executed locally.
Thanks @ntdatt812 — the switch-specific check addresses the original concern about ordinary literals such as review, and the added boundary coverage is useful.
One valid literal case is still rejected: expression: inputs.mode with a declared inputs.mode case. execute() matches that case, but the new validator rejects it because the text resembles a namespace reference. Please preserve that valid literal behavior and add a regression checking that validation accepts what execution matches.
Please also refresh the PR description to reflect the new switch-specific helper rather than reuse of both condition predicates. Your Claude Code disclosure is present; please complete it with the model(s) and settings/mode used.
This is triage-nice-to-have. After those corrections, it needs CI and re-review on the updated head.
Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra).
`SwitchStep.validate` checked only that `expression` is present. It goes through
the same `evaluate_expression` as a condition, so one written without braces
comes back as its own source text:
expression: inputs.mode -> expression_value: "inputs.mode"
matched_case: "__default__"
status: COMPLETED
It matches no case key, falls through to `default` on every run — or dispatches
nothing at all when there is no default — and still reports COMPLETED. That is
the "silent empty result + COMPLETED" wiring bug this file's own `cases:` guard
was written to prevent, on the field one line above it.
`if`, `while` and `do-while` already run these two predicates on their
`condition`. This reuses them rather than writing a third scan.
Only those two apply. A switch matches on strings, so a composite key such as
`{{ inputs.a }}-{{ inputs.b }}` is legitimate here even though the same shape
would be a fault in a boolean condition — there is a test pinning that, and a
literal `true` and the empty string stay accepted as ordinary case keys for the
same reason.
…switch
The validator reused condition_is_never_evaluated, which flags every
braceless string because a condition is coerced by bool(). A switch is
not: it matches its resolved value against case keys, and a case key is
a literal. `expression: review` dispatches the `review:` case and
whitespace strips to the "" key, so both were being rejected while the
step runs them correctly.
switch_expression_is_never_evaluated flags braceless text only when it
opens by walking into a root _build_namespace supplies (inputs.mode,
item[0], context.run_id), and keeps the verbatim-unclosable {{ case.
Tests pin both directions, with the literal cases checked against what
execute() actually dispatches.
Rebased onto main (25d43a94). Was CONFLICTING, now clean.
The rebase applied without a textual conflict, but it left the branch broken in a way that only running the tests showed: the five tests here imported SwitchStep from specify_cli.workflows.steps.switch, and the package is workflows.step (singular). The import path was stale from before that rename, so every test in this pull request errored on collection while the diff looked fine. Fixed in place.
uv run --with pytest python -m pytest tests/test_workflows.py: 707 passed. The 5 failures are the symlink tests, which need a privilege Windows does not grant here and fail identically on main.
AI assistance disclosed: this change and this comment were written with an AI coding agent, reviewed and verified by me before posting.
The guard read the expression text alone, so `expression: inputs.mode` was
rejected even when an `inputs.mode:` case was declared -- a switch that
dispatches that case on every run, because `execute` matches the resolved
value, which for a braceless expression is that very text, stripped, against
`str(case_key)`. Constant, not unevaluated.
`switch_expression_is_never_evaluated` now takes the declared keys and flags a
namespace reference only when none of them declares it, which is the shape that
can do nothing but fall through to `default`. The `{{`-verbatim branch is
unchanged: there the emitted text is not the source text, so a key cannot vouch
for it.
Tests assert `execute` really dispatches the declared case before asserting
`validate` accepts it, then withdraw that one key and require the error back.
Reverting either the predicate or the call site turns the new test red.
@mnriem Addressed in 9b79e81 — answered in thread on expressions.py.
Copilot's point was correct and narrowing: the guard judged the expression text alone, so expression: inputs.mode was rejected even with an inputs.mode: case declared. That switch really does dispatch that case on every run, because execute matches the resolved value — for a braceless expression, that very text, stripped — against str(case_key). The predicate now takes the declared keys and flags a namespace reference only when none of them declares it, which is the only shape that can do nothing but fall through to default.
The new test runs execute() first and asserts the dispatched case, then withdraws that one key and requires the error back, so the exemption cannot disarm the guard. I mutated the predicate and the call site separately to confirm the test goes red both ways.
One note on the red check: pytest (windows-latest, 3.14) failed on tests/extensions/test_extension_agent_context.py::TestBundledUpdaterPathValidation::test_powershell_script_discovers_nested_plan with subprocess.TimeoutExpired after 30 s spawning powershell.exe. windows-latest, 3.13 passed on the same commit and this diff touches only switch validation, so it is a runner timeout rather than this change. This push re-runs it.
AI assistance disclosed: written with Claude Code; I reviewed and ran the tests and lint myself.
This branch also catches an unclosed block after a valid block, such as {{ inputs.a }}-{{ inputs.b: the first block is interpolated, so the result is not necessarily the literal source text and may match a case. Even a wholly unclosed block can match a declared literal case, and a switch may have no default. The message's “no usable block,” “none of which declares it,” and “falls through ... on every run” claims are therefore misleading. Describe the unevaluated reference without predicting the dispatched branch, and print the opening {{ delimiter literally (the current f-string renders {).
Correct raw-close fallback message for evaluation failures
The raw-close fallback evaluates the truncated block before switch case matching; for {{ inputs.missing | default('oops }} it can raise ValueError instead of matching any case. This message incorrectly promises a match, and the f-string prints { rather than the opening {{. Say that evaluation is attempted and recommend balancing the delimiters and quotes, as the condition validators do.
Test distinct errors for unclosed and raw-close forms
tests/test_workflows.py:3818
This only checks that both malformed forms produce some 'expression' error, so sending the raw-close form down the “never evaluated” path would still pass. These inputs need different guidance: the unclosed tail is emitted verbatim, while the raw-close form tries to evaluate a truncated block. Assert the relevant message for each form so a regression in branch selection is caught.
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
author-awaitingWaiting on author responseauthor-needs-disclosureAI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTINGauthor-needs-testsReal change but missing a regression test — add one that fails before / passes aftertriage-nice-to-haveVerdict: evidence-backed fix or greenlit feature — land after review
3 participants
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.
mnriem
left a comment
The defect
SwitchStep.validatechecks only thatexpressionis present (steps/switch/__init__.py:105). The field goes through the sameevaluate_expressionas a condition, so one written without braces comes back as its own source text.Measured on
main(27f50f7),inputs.mode = "review", casesreview/build, plus adefault:It matches no case key, falls through to
defaulton every run — or, with nodefault:, dispatches nothing at all — and still reports COMPLETED. That is exactly the "silent empty result + COMPLETED" wiring bug this file's owncases:guard was written to prevent, quoting its comment, on the field one line above it.if,whileanddo-whileall runcondition_is_never_evaluatedandcondition_has_malformed_expression_blockon theircondition.switchran neither on itsexpression.The fix
Reuse those two predicates rather than write a third scan, with switch-appropriate wording.
Only those two apply, deliberately. A switch matches on strings, so a composite key is legitimate here even though the same shape is a fault in a boolean condition:
inputs.mode{{ inputs.x{{ inputs.missing | default('oops }}{{ inputs.mode }}{{ inputs.a }}-{{ inputs.b }}true,""The last two rows are the condition-specific exemptions
condition_is_never_evaluatedalready carries. They are harmless on a condition and actively correct here, which is what makes these two predicates safe to reuse on a non-boolean field. There is a test pinning that boundary so a later narrowing cannot quietly reject composite keys.This is independent of #4292, deliberately: that PR adds a predicate for a condition holding more than one block, which is precisely the shape a switch is allowed to have. It is not applied here.
Verification
Windows, Python 3.11.
Identical 22 failures on both sides,
diffclean — the pre-existing symlink and bash-parity classes on unelevated Windows. 1728 → 1731 is exactly the three cases added.Mutation-checked, after confirming the edit applied: disabling the never-evaluated branch fails 2 of the 3 new cases, and the composite-key case stays green under it — which is the point, since it does not depend on the branch and still guards the other side.
Tests
tests/test_workflows.py::TestSwitchStep— three cases: the braceless form (asserting the current runtime behaviour first, then the validator), both unclosable shapes, and the composite-key/literal boundary that must stay accepted.AI disclosure
Per CONTRIBUTING: this pull request (code, tests and description) was developed with Claude Code as a coding agent.