Skip to content

fix(workflows): reject a switch expression that is never evaluated - #4295

Open
ntdatt812 wants to merge 2 commits into
github:mainfrom
ntdatt812:fix/switch-expression-never-evaluated
Open

ntdatt812 wants to merge 2 commits into
github:mainfrom
ntdatt812:fix/switch-expression-never-evaluated

Conversation

@ntdatt812

@ntdatt812 ntdatt812 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

The defect

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:

expression: inputs.mode
  validate  -> []
  status    -> COMPLETED
  output    -> {'matched_case': '__default__', 'expression_value': 'inputs.mode'}

expression: {{ inputs.mode }}      (control)
  status    -> COMPLETED
  output    -> {'matched_case': 'review', 'expression_value': 'review'}

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.

Verification

Windows, Python 3.11.

tests/test_workflows.py -k Switch                       25 passed  (22 before)
tests/test_workflows.py + test_condition_expression_block.py
  + tests/test_extensions.py                            22 failed, 1731 passed
main, same three files                                  22 failed, 1728 passed

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.

@ntdatt812
ntdatt812 requested a review from mnriem as a code owner August 24, 2026 08:53
@mnriem
mnriem requested a balanced review from Copilot August 31, 2026 18:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The validator incorrectly rejects valid non-boolean string literals used as switch values.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds switch-expression validation to catch unevaluated or malformed expression blocks.

Changes:

  • Reuses existing condition-expression predicates for switch validation.
  • Adds regression and boundary tests.
File summaries
File Description
src/specify_cli/workflows/steps/switch/__init__.py Validates switch expressions.
tests/test_workflows.py Covers invalid and accepted expression forms.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Only these two checks 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.
elif condition_is_never_evaluated(config["expression"]):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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 mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

`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.
@ntdatt812
ntdatt812 force-pushed the fix/switch-expression-never-evaluated branch from 37ea908 to 2554a12 Compare September 11, 2026 01:32
@ntdatt812

Copy link
Copy Markdown
Contributor Author

@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.

@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 14, 2026
@mnriem

mnriem commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

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).

@mnriem mnriem added author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-awaiting Waiting on author response author-needs-tests Real change but missing a regression test — add one that fails before / passes after labels Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-needs-tests Real change but missing a regression test — add one that fails before / passes after triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants