fix(workflows): resolve negative list indices in expressions - #4416
fix(workflows): resolve negative list indices in expressions#4416NgoQuocViet2001 wants to merge 2 commits into
Conversation
_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.
There was a problem hiding this comment.
🟡 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.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
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.
|
Addressed — the Copilot finding was correct, thanks.
Both now allow
|
mnriem
added
author-needs-disclosure
|
Thanks for the quick turnaround — the fix to |
Two regression tests for the property this refactor is for, both of which a second copy of the grammar in the gate would break while every existing test stayed green: - widening _INDEXED_SEGMENT alone reaches the gate (the negative-index shape from github#4416) - when the evaluator stops treating something as a leaf, the gate stops checking it, with no gate edit (the grouped-operand shape from github#4417) Both were checked by reintroducing the drift: giving the gate its own segment regex again fails the first with the real message rather than an import error.
|
The re-run review is green now — the negative-index handling and its regression coverage look solid, thanks for turning the earlier findings around. One thing before I can merge: please add the AI-disclosure per [CONTRIBUTING](https://github.com/github/spec-kit/blob/main/CONTRIBUTING.md#ai-contributions-in-spec-kit) — either describe the AI assistance and its extent, or check the "no AI assistance" box. That's the only item still outstanding on your side. Heads-up on sequencing: this and #4417 both touch |
…4460) * refactor(workflows): let the evaluator report its own leaves (#4274) _unresolvable_term answered one question -- does every operand in this condition resolve to something? -- by walking the expression itself: filters, then or/and/not, then comparisons, then list literals, down to the leaves. That walk was a second implementation of the parsing in _evaluate_simple_expression, kept in step with it by hand. Two helpers existed only to restate rules the evaluator already had. _looks_numeric mirrored the float()-only-when-a-dot-is-present rule because a bare float() accepts 1e3 and the evaluator does not. _is_literal mirrored the matching-close-is-the-final-character string test because startswith/endswith accepts 'a' 'b' and the evaluator does not. Both docstrings said "mirror the evaluator exactly", which is the tell: when the two drift nothing breaks loudly, the gate just answers wrongly, and the wrong answer is a paste-ready correction that inverts a condition. Seven of the nine findings in #4230 were the same defect wearing different clothes -- the gate disagreeing with the evaluator about where the operands are. Each round fixed one shape. Nothing stopped a tenth. _evaluate_simple_expression has exactly one place where a substring stops being grammar and becomes a name to resolve: its final line, _resolve_dot_path. Literals return before it; operands, filter arguments and list elements all arrive there by construction. Record the leaf there, behind a ContextVar that is None outside a probe, and the gate applies namespace rules to that list instead of re-deriving it. It now contains no grammar at all. Two properties this rests on, both asserted rather than assumed: * or/and are not short-circuited -- both sides are evaluated and only then combined -- so a leaf is recorded whatever the other side is worth. If that ever changes the gate would go quietly blind, so there is a test for it. * A probe run can raise on its own placeholder values. The leaves seen before that point are real, so they are kept rather than discarded; discarding them would lose `bogus` in `inputs.tags | join(bogus)`, which an earlier round of #4230 had to add by hand. expressions.py is 109 lines lighter and 84 heavier. All 336 existing tests pass unchanged, including the 20 cases of test_operands_must_be_literals_or_known_paths that took eight rounds to get right. test_literal_test_mirrors_the_evaluator tested the mirror, so it becomes test_literal_handling_comes_from_the_evaluator and asserts the same knowledge about 1e3 and 'a' 'b' through the gate instead. Four mutations, each killed by the tests that should kill it -- removing the leaf report alone turns 38 red. ruff 0.15.0 clean. * refactor(workflows): let _resolve_dot_path define the indexed segment The gate no longer restates the operator grammar, but it still restated the shape of a path segment: _PATH_SEGMENT and an inline fullmatch both described the index form that _resolve_dot_path matches with its own regex. Three copies of one rule, kept in step by hand -- the same drift this refactor set out to remove, one layer down. Name the form once as _INDEXED_SEGMENT beside _resolve_dot_path and have the gate ask it. Behaviour is unchanged: the regex is copied verbatim. What changes is that widening indexing now reaches the gate for free. * test(workflows): pin that the gate reads the evaluator's definitions Two regression tests for the property this refactor is for, both of which a second copy of the grammar in the gate would break while every existing test stayed green: - widening _INDEXED_SEGMENT alone reaches the gate (the negative-index shape from #4416) - when the evaluator stops treating something as a leaf, the gate stops checking it, with no gate edit (the grouped-operand shape from #4417) Both were checked by reintroducing the drift: giving the gate its own segment regex again fails the first with the real message rather than an import error. * fix(workflows): keep collecting leaves after a probe error The refactor stopped the leaf walk at the first exception a probe value raised, so every leaf further along the chain was lost. That is the one thing the collection exists to report, and it was a step backwards from the hand-written walk this PR replaces: inputs.blob | from_json | contains(bogus) origin/main reports 'bogus' this PR before the fix MISSED this PR after the fix reports 'bogus' from_json receives the probe placeholder mapping and raises; the walk ended there and contains(bogus) was never reached. Carry on past a failing filter while the sink is armed. _apply_filter evaluates a filter argument before it can raise on the value, so the failing segment's own leaves are already recorded; a fresh placeholder goes into the next filter, matching what the probe namespace hands out. Scoped to the probe: the sink is armed only by _collect_leaves, and _evaluator_rejects runs its own probe without it, so a mis-wired filter is still rejected and a real evaluation still raises rather than quietly returning the unfiltered value.
Problem
_resolve_dot_pathmatches 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 toNone: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, andevaluate_conditionon 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
Nonerather than raising, matching the existing behaviour for[9]on a short list.Scope
Two lines in
src/specify_cli/workflows/expressions.pyplus a docstring note, and one test next to the existingtest_list_indexing. Positive indices and non-index path segments are untouched.Test plan
pytest tests/test_workflows.py -k "indexing or literal"→ 11 passed.pytest tests/test_workflows.py→ 942 passed. The 20 failures are theTestWorkflowCliAlignmentsymlink cases, which fail identically on an unmodified checkout here (Windows, no symlink privilege).expressions.pyfails the new test withassert None == 'b.md'.