Skip to content

fix(workflows): resolve negative list indices in expressions - #4416

Open
NgoQuocViet2001 wants to merge 2 commits into
github:mainfrom
NgoQuocViet2001:fix-negative-list-index
Open

fix(workflows): resolve negative list indices in expressions#4416
NgoQuocViet2001 wants to merge 2 commits into
github:mainfrom
NgoQuocViet2001:fix-negative-list-index

Conversation

@NgoQuocViet2001

Copy link
Copy Markdown
Contributor

Problem

_resolve_dot_path matches the index bracket with ^([\w-]+)\[(\d+)\]$, which accepts digits only. A negative index therefore never enters the indexing branch — it falls through to the dict lookup and asks for the literal key "task_list[-1]", which is absent, so the whole path resolves to None:

ctx = StepContext(steps={"tasks": {"output": {"task_list": [{"file": "a.md"}, {"file": "b.md"}]}}})

evaluate_expression("{{ steps.tasks.output.task_list[0].file }}", ctx)   # 'a.md'
evaluate_expression("{{ steps.tasks.output.task_list[-1].file }}", ctx)  # None

list[-1] is valid in both Python and the Jinja2 subset the module documents itself as providing, and "the last item a step produced" is a natural thing for a workflow template to want. There is no error: the template renders empty, and evaluate_condition on the same path reads false, so a step can be skipped for a reason that never surfaces.

Fix

Accept the negative form in the pattern and bound the index from both ends. Out-of-range in either direction keeps returning None rather than raising, matching the existing behaviour for [9] on a short list.

Scope

Two lines in src/specify_cli/workflows/expressions.py plus a docstring note, and one test next to the existing test_list_indexing. Positive indices and non-index path segments are untouched.

Test plan

  • Ran: pytest tests/test_workflows.py -k "indexing or literal" → 11 passed.
  • Ran: pytest tests/test_workflows.py → 942 passed. The 20 failures are the TestWorkflowCliAlignment symlink cases, which fail identically on an unmodified checkout here (Windows, no symlink privilege).
  • Checked: reverting only expressions.py fails the new test with assert None == 'b.md'.

_resolve_dot_path matched only digits in the index bracket, so
`task_list[-1]` never entered the indexing branch. It fell through to
the dict lookup and asked for the literal key "task_list[-1]", which
returns None — a template reaching for the last element of a step output
rendered empty with no error, and a condition on it silently read false.

Accept the negative form Python and Jinja2 both use, and bound the index
from both ends so out-of-range still yields None rather than raising.

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

Condition-remediation parsing still rejects negative indices supported by the resolver.

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

Pull request overview

Adds Python-style negative list indexing to workflow expressions.

Changes:

  • Supports bounded negative indices.
  • Adds resolution and bounds tests.
File summaries
File Description
expressions.py Extends list-index parsing and bounds checks.
test_workflows.py Tests negative and out-of-range indices.
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.

# Handle list indexing: name[0]
idx_match = re.match(r"^([\w-]+)\[(\d+)\]$", part)
# Handle list indexing: name[0], name[-1]
idx_match = re.match(r"^([\w-]+)\[(-?\d+)\]$", part)

@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

Addressing review feedback. The resolver now takes `item[-1]`, but the
two matchers the condition-remediation path uses were left on `\d+`:
_PATH_SEGMENT and the indexed-root fullmatch. So a condition the
evaluator resolves fine was classified unresolvable, and
format_condition_remediation withheld the 'wrap the expression'
correction from it:

  item[0] == 'x'   -> Wrap the expression: "{{ item[0] == 'x' }}".
  item[-1] == 'x'  -> No correction is offered because 'item[-1]' is not
                      a name the evaluator can resolve

Allow -?\d+ in both, and extend the existing parametrize with the two
negative cases.
@NgoQuocViet2001

Copy link
Copy Markdown
Contributor Author

Addressed — the Copilot finding was correct, thanks.

_PATH_SEGMENT and the indexed-root fullmatch were both still on \d+, so a condition the resolver handles was classified unresolvable and the correction withheld:

item[0] == 'x'   -> Wrap the expression: "{{ item[0] == 'x' }}".
item[-1] == 'x'  -> No correction is offered because 'item[-1]' is not a name
                    the evaluator can resolve

Both now allow -?\d+, and the two negative cases are in the existing parametrize beside test_an_indexed_item_root_keeps_the_correction. The refusal side is unchanged — inputs[0] and steps[-1] are still rejected as non-roots.

pytest tests/unit/test_condition_expression_block.py → 337 passed. Reverting only expressions.py fails the two new cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants