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
Adds upper, lower, split, length, and to_json to the workflow expression evaluator. Per the agreed Option A scope, first, last, sort, unique, and trim stay deferred until real usage justifies their semantics.
Semantics
Every filter validates its own input types and raises ValueError naming the problem instead of coercing, matching the strict argument handling already used by join/map/from_json. Coercion is rejected on purpose: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. "3" for an int) would hide that — it would also leak a cryptic TypeError/AttributeError past the evaluator, which the engine does not wrap in a try/except, crashing the whole run.
Filter
Input → Output
Empty/edge
Invalid input
upper
str → uppercased str
"" → ""
non-str → ValueError
lower
str → lowercased str
"" → ""
non-str → ValueError
split
str → list[str], split(sep) only
"".split(",") → [""]
non-str value or sep → ValueError
length
list/str → int (Jinja2 behavior)
[]/"" → 0
mappings and other types → ValueError
to_json
any serializable value → str
None → "null"
non-serializable or non-finite float → ValueError
length rejects mappings so the same expression cannot mean key count for one shape and value count for another.
to_json pins sort_keys=True and ensure_ascii=False, so output is byte-stable regardless of dict insertion order or platform — that is what makes the to_json ↔ from_json round trip reproducible, and keeps non-ASCII readable instead of \uXXXX-escaped. Reproducible is not the same as shell-safe: interpolation adds no quoting or escaping, so the value is still read by the shell if it lands in a run field. See Interpolation and shell safety in docs/reference/workflows.md.
Implementation
The strict no-argument branch is generalized into _ZERO_ARG_FILTERS, shared by from_json, upper, lower, length, and to_json, so mis-wired forms (| upper('x'), | length extra, | to_json()) raise naming the filter rather than falling through to the unknown-filter path. split joins the single-argument dispatch in _apply_filter.
Tests
Existing tests that used length/upper as examples of unknown filters were retargeted at a genuinely unknown name (truncate), not deleted — that coverage still asserts unknown names fail loudly.
test_registered_filters_unaffected now asserts all ten registered filters, and test_registered_filters_covers_every_implemented_filter keeps _REGISTERED_FILTERS in sync with what is implemented.
New coverage: happy paths, empty/edge inputs, every ValueError path, filter chaining, the from_json ↔ to_json round trip, length driving conditions, and strict zero-arg forms across all five.
tests/test_workflows.py + tests/unit/test_condition_expression_block.py: 1069 passed, 5 skipped. Full suite: no new failures against an origin/main baseline (the 30 pre-existing failures were reproduced by stashing this change). ruff@0.15.0 check src tests is clean for the changed files.
Documentation
Behavior documented per constitution Principles I/II/V in workflows/README.md, workflows/ARCHITECTURE.md, and docs/reference/workflows.md. Two source comments that hardcoded the old five-filter list were rewritten so they cannot go stale again.
Correction to the issue's design note
The issue says "length enables conditionals: Unlocks {% if items | length > 0 %} patterns". That does not hold:
There is no {% if %} syntax in the evaluator at all — conditions are condition: "{{ ... }}" expressions.
The pipe binds tighter than comparison operators, so a trailing comparison after any filter raises. This is pre-existing design, not new: {{ count | default(0) > 5 }} already raises today.
Docs therefore show the form that does work — condition: "{{ items | length }}", where 0 is falsy and any non-zero count is truthy — rather than documenting a pattern that would fail at runtime.
AI disclosure
This change was developed under human direction by @Quratulain-bilal and generated by the opencode CLI agent.
Agent/tool
opencode (CLI coding agent)
Model
mimo-v2.6-flash-free
Mode
Autonomous execution under direct human direction
Authoring extent
All implementation, tests, comments, and documentation text in this PR
Human extent
Scope selection, requirement direction, review-round decisions, verification review, GitHub credentials, and every release decision
How the human was involved:
Chose and confirmed the scope. Adopted the maintainer-approved Option A subset (length, split, upper, lower, to_json) and kept first/last/sort/unique/trim deferred rather than taking the larger batch.
Directed every round of work. Read each review round and decided what to act on — the latest round instructed the agent to address the outstanding Copilot finding first, covering the PR description and the test comment, not just the code.
Reviewed results, not diffs. Reviewed the verification output (spec conformance, test totals, and a stashed origin/main baseline showing no new failures) before authorizing commits.
Controlled credentials and publication. Held the GitHub credentials behind every API call, and approved each commit, push, PR body edit, and comment before it went out.
Audited this disclosure. Required that it state the human's actual contribution instead of crediting the whole change to the agent.
The agent authored the code autonomously; the human did not line-by-line review the diff before commit, which is why the commit trailer records autonomous rather than supervised. The human is expected to be able to answer for this change during review.
Option A of github#4614: the smallest high-value batch of expression filters
(length, split, upper, lower, to_json), leaving first, last, sort,
unique, and trim deferred until real usage justifies their semantics.
Each filter validates its own input types and raises ValueError naming
the problem rather than coercing, matching the strict argument handling
already used by join/map/from_json. Coercion is rejected because a type
mismatch almost always means the workflow is wired to the wrong
variable, and a coerced result (e.g. "3" for an int) would hide that;
it would also leak a cryptic TypeError/AttributeError past the
evaluator, which the engine does not wrap in a try/except, crashing the
whole run.
- upper/lower: str -> str, empty string valid, non-str -> ValueError
- split: str -> list[str], single split(sep) form only, "" -> [""],
both value and separator must be strings
- length: list and str -> int (Jinja2 behavior), mappings rejected so
the same expression cannot mean key count for one shape and value
count for another, empty -> 0, other types -> ValueError
- to_json: any serializable value via json.dumps(..., sort_keys=True,
ensure_ascii=False), None -> "null", non-serializable -> ValueError
to_json pins sort_keys and ensure_ascii so output is byte-stable
regardless of dict insertion order or platform, which is what makes a
to_json -> shell step -> from_json round trip safe.
The strict no-argument branch is generalized into _ZERO_ARG_FILTERS so
from_json, upper, lower, length, and to_json share it; mis-wired forms
(upper('x'), length extra, to_json()) raise naming the filter instead
of falling through to the unknown-filter path.
Existing tests that used length/upper as examples of unknown filters
are retargeted at a genuinely unknown name rather than deleted, and
test_registered_filters_unaffected now asserts all ten registered
filters. Behavior is documented in workflows/README.md,
workflows/ARCHITECTURE.md, and docs/reference/workflows.md.
Note: the issue's claim that length unlocks `{% if items | length > 0 %}`
does not hold. There is no `{% if %}` syntax in the evaluator, and the
pipe binds tighter than comparisons, so a trailing comparison after any
filter raises (pre-existing design, same as `count | default(0) > 5`).
Docs instead show condition: "{{ items | length }}" (0 is falsy).
Closesgithub#4614
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
Seven findings from the Copilot review of github#4766, addressed in code,
tests, and docs.
Define and test empty-separator errors: `split('')` reached str.split and
raised the bare `ValueError: empty separator`, naming neither the filter
nor the expression, and no test covered it. An empty separator has no
meaning in Python, so the filter now rejects it with a filter-specific
message alongside the existing value and separator type checks.
Scope strictness claims to the new filters: the docs said "every filter"
validates its inputs, which the older filters do not — `join` stringifies
unsupported shapes and elements, and `map`/`contains` return fallbacks
rather than raising. README and ARCHITECTURE now state the contract for
the five new filters and explicitly note that the older ones are
unchanged, so authors are not promised behavior the evaluator does not
provide.
Replace shell-safety claims with reproducibility: determinism from
`sort_keys`/`ensure_ascii` was described as making a `to_json` -> shell ->
from_json round trip "safe", which contradicts the repository's shell
safety model — interpolation adds no quoting or escaping, so JSON quotes
and metacharacters are still interpreted by the shell. All three
locations now claim reproducibility only and point at the
interpolation-and-shell-safety guidance.
Remove unsupported comparison guidance: "compare it before filtering"
was not a usable alternative, because the count does not exist until
`length` runs. The docs now say plainly that `{{ items | length > 0 }}`
is unreachable and that truthiness in a `condition:` is the supported
form.
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
The empty-separator gap was the only code change. split('') used to fall through to str.split and surface Python's bare ValueError: empty separator, which names neither the filter nor the expression and was untested. The filter now rejects an empty separator with its own message, next to the existing value and separator type checks, with a regression test pinning it.
The rest was documentation accuracy, not behavior:
Strictness claims are now scoped to the five new filters. The text previously said "every filter" validates its inputs, which the older filters do not — join stringifies unsupported shapes and elements, and map/contains return fallbacks rather than raising. workflows/README.md and workflows/ARCHITECTURE.md now state the new filters' contract and say explicitly that the older ones are unchanged.
The to_json → shell → from_json round trip was described as "safe", which contradicts this repository's shell-safety model. Determinism from sort_keys/ensure_ascii gives reproducibility and nothing else: interpolation adds no quoting or escaping, so JSON quotes and metacharacters are still read by the shell. All three places it appeared — the _filter_to_json docstring, ARCHITECTURE.md, and docs/reference/workflows.md — now claim reproducibility only and point at the interpolation-and-shell-safety section.
The "compare it before filtering" alternative is gone. The count does not exist until length runs, so that guidance was unreachable; the docs now say plainly that {{ items | length > 0 }} cannot be written and that truthiness in a condition: is the supported form.
Verification
tests/test_workflows.py + tests/unit/test_condition_expression_block.py: 1069 passed, 5 skipped. ruff@0.15.0 check src tests reports only two pre-existing errors, both outside this change (integrations/forge/__init__.py F811 and the untracked tests/test_exception_narrowing.py F401).
Mixed mapping key types cause misleading serialization errors
src/specify_cli/workflows/expressions.py:234
sort_keys=True rejects some values that are otherwise JSON-serializable. For example, json.dumps({1: "a", "2": "b"}) succeeds, but this call raises TypeError because int and str keys cannot be ordered; the wrapper then inaccurately reports that the value is not serializable. Since workflow step outputs can contain arbitrary values, either explicitly validate/document string-only mapping keys or normalize supported JSON keys deterministically, and cover mixed key types with a regression test.
The test comment promised a structured value could pass through a shell
step and be recovered, which the evaluator does not guarantee:
interpolation adds no quoting, so ShellStep would hand the JSON to
shell=True unescaped. The test only exercises to_json/from_json inside
the evaluator, so say that and point at the interpolation-and-shell-safety
section instead of promising a shell round trip.
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
json.dumps emits NaN, Infinity, and -Infinity by default even though those tokens are not valid JSON. This lets to_json return output that standards-compliant downstream parsers reject instead of taking the documented ValueError path. Please set allow_nan=False and add regression coverage for all three non-finite values.
Clarify fallback applies only to None or empty strings
docs/reference/workflows.md:597
This newly added table overstates default: _filter_default only falls back for None and ""; an empty list or mapping is returned unchanged. Please describe this as “Fallback for None or an empty string” so workflow authors are not led to expect all empty values to trigger it.
Correct unsafe shell round-trip claim for JSON interpolation
workflows/ARCHITECTURE.md:138
The PR description says deterministic serialization makes a to_json → shell → from_json round trip safe, but this documentation correctly states that interpolation adds no shell quoting or escaping. Please remove or correct that safety claim in the PR description; byte stability does not prevent JSON quotes or metacharacters from being interpreted by the shell.
The remaining shell round-trip claim sat outside the docs: this PR description and the comment on test_filter_to_json_round_trips_from_json. That test only evaluates to_json/from_json inside the evaluator and never invokes a shell, so the comment now says exactly that and points at the interpolation-and-shell-safety section. The to_json paragraph in the description now claims reproducibility only, with the same caveat: interpolation adds no quoting or escaping, so a run field carrying the value is still read by the shell.
The description's test total was also stale at 1068, from before the empty-separator regression test was added. It now reads 1069, matching tests/test_workflows.py + tests/unit/test_condition_expression_block.py (1069 passed, 5 skipped).
json.dumps defaults to allow_nan=True, so to_json emitted bare NaN,
Infinity, and -Infinity. None of those is valid JSON: the filter promised
a JSON string and handed standards-compliant parsers something they must
reject. Pass allow_nan=False so non-finite floats take the same ValueError
path as any other unserializable value, and pin all three plus a nested
case with a regression test.
Documented alongside the existing to_json notes in README, ARCHITECTURE,
and the reference.
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
sort_keys=True rejects mappings that are otherwise accepted by json.dumps, such as {1: "a", "2": "b"}, because heterogeneous keys cannot be ordered. This path is caught and reported as “not JSON-serializable,” contradicting the filter's “any serializable value” contract. Either validate and document string-only mapping keys, or normalize supported JSON keys deterministically before sorting, and add regression coverage.
Enforce single-argument contract before separator evaluation
src/specify_cli/workflows/expressions.py:625
The single-argument contract is not enforced before evaluating the argument: split(',', 1) evaluates the whole comma expression to None, then reports expected a string separator, got NoneType instead of identifying unsupported arity. Since a max-split argument is a likely authoring mistake, detect multiple top-level arguments before dispatch and cover this malformed form with a regression test.
to_json was passing allow_nan=True (the json.dumps default), so NaN, Infinity, and -Infinity serialized to bare tokens, none of which is valid JSON — the filter promised a JSON string and handed standards-compliant parsers something they must reject. It now passes allow_nan=False, so all three take the existing ValueError path used for unserializable values, with the underlying message still chained so the cause stays visible.
A regression test covers each non-finite value at the top level and nested inside a container, since a container is how they normally reach the filter. The note is documented next to the other to_json behavior in workflows/README.md, workflows/ARCHITECTURE.md, and docs/reference/workflows.md, and the semantics table in the PR description now says non-finite floats are rejected.
Verification: tests/test_workflows.py + tests/unit/test_condition_expression_block.py — 1070 passed, 5 skipped (one new test). uvx ruff@0.15.0 check src tests reports only the two pre-existing errors outside this change (integrations/forge/__init__.py F811, the untracked tests/test_exception_narrowing.py F401).
Thanks for fixing the non-finite JSON case and adding regression coverage. The earlier inline review threads are resolved, but I don’t think the PR is ready for another review yet. The latest review identifies two previously missed cases still present at the current head: to_json rejects mixed-type mapping keys despite the “any serializable value” contract, and split(',', 1) reports a separator-type error rather than an unsupported extra argument. Please resolve each contract/diagnostic issue and add focused regression coverage.
There’s also an earlier documentation concern still present: the default table says “empty values,” but the implementation falls back only for None and an empty string. Please correct that wording as part of the follow-up.
Disclosure: Drafted for @mnriem with GitHub Copilot (GPT-6 Sol, interactive mode); AI assistance was limited to assessing the PR feedback and drafting this reply.
Both findings from the latest review round hid the real authoring mistake
behind a less useful error.
to_json relied on sort_keys=True for determinism, so a mapping with
mixed key types raised an ordering TypeError that the generic handler
reported as "not JSON-serializable", while a mapping with a lone integer
key was silently coerced to "1" and could collide with an existing "1"
key. JSON objects have string keys, so keys are validated before
json.dumps is reached and the message names the key type instead. The
walk is iterative with a seen set, leaving circular structures for
json.dumps to report.
split(',', 1) evaluated the argument expression first. The evaluator has
no comma operator, so the fragment evaluated to None and the reported
error was "expected a string separator, got NoneType" -- the extra
argument never surfaced. Arity for the single-argument filters is now
counted on the top-level comma split, before evaluation, so the message
names the filter and the argument count.
Documented in workflows/README.md, workflows/ARCHITECTURE.md, and
docs/reference/workflows.md, with a regression test for each behavior.
Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
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
triage-can-waitVerdict: valid and in-scope but deprioritized; held behind the evidence gate
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
added
the
triage-can-wait
Fixes #4614 (Option A)
Adds
upper,lower,split,length, andto_jsonto the workflow expression evaluator. Per the agreed Option A scope,first,last,sort,unique, andtrimstay deferred until real usage justifies their semantics.Semantics
Every filter validates its own input types and raises
ValueErrornaming the problem instead of coercing, matching the strict argument handling already used byjoin/map/from_json. Coercion is rejected on purpose: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g."3"for an int) would hide that — it would also leak a crypticTypeError/AttributeErrorpast the evaluator, which the engine does not wrap in atry/except, crashing the whole run.upperstr→ uppercasedstr""→""ValueErrorlowerstr→ lowercasedstr""→""ValueErrorsplitstr→list[str],split(sep)only"".split(",")→[""]ValueErrorlengthlist/str→int(Jinja2 behavior)[]/""→0ValueErrorto_jsonstrNone→"null"ValueErrorlengthrejects mappings so the same expression cannot mean key count for one shape and value count for another.to_jsonpinssort_keys=Trueandensure_ascii=False, so output is byte-stable regardless of dict insertion order or platform — that is what makes theto_json↔from_jsonround trip reproducible, and keeps non-ASCII readable instead of\uXXXX-escaped. Reproducible is not the same as shell-safe: interpolation adds no quoting or escaping, so the value is still read by the shell if it lands in arunfield. See Interpolation and shell safety indocs/reference/workflows.md.Implementation
The strict no-argument branch is generalized into
_ZERO_ARG_FILTERS, shared byfrom_json,upper,lower,length, andto_json, so mis-wired forms (| upper('x'),| length extra,| to_json()) raise naming the filter rather than falling through to the unknown-filter path.splitjoins the single-argument dispatch in_apply_filter.Tests
length/upperas examples of unknown filters were retargeted at a genuinely unknown name (truncate), not deleted — that coverage still asserts unknown names fail loudly.test_registered_filters_unaffectednow asserts all ten registered filters, andtest_registered_filters_covers_every_implemented_filterkeeps_REGISTERED_FILTERSin sync with what is implemented.ValueErrorpath, filter chaining, thefrom_json↔to_jsonround trip,lengthdriving conditions, and strict zero-arg forms across all five.tests/test_workflows.py+tests/unit/test_condition_expression_block.py: 1069 passed, 5 skipped. Full suite: no new failures against anorigin/mainbaseline (the 30 pre-existing failures were reproduced by stashing this change).ruff@0.15.0 check src testsis clean for the changed files.Documentation
Behavior documented per constitution Principles I/II/V in
workflows/README.md,workflows/ARCHITECTURE.md, anddocs/reference/workflows.md. Two source comments that hardcoded the old five-filter list were rewritten so they cannot go stale again.Correction to the issue's design note
The issue says "length enables conditionals: Unlocks
{% if items | length > 0 %}patterns". That does not hold:{% if %}syntax in the evaluator at all — conditions arecondition: "{{ ... }}"expressions.{{ count | default(0) > 5 }}already raises today.Docs therefore show the form that does work —
condition: "{{ items | length }}", where0is falsy and any non-zero count is truthy — rather than documenting a pattern that would fail at runtime.AI disclosure
This change was developed under human direction by @Quratulain-bilal and generated by the opencode CLI agent.
How the human was involved:
length,split,upper,lower,to_json) and keptfirst/last/sort/unique/trimdeferred rather than taking the larger batch.origin/mainbaseline showing no new failures) before authorizing commits.The agent authored the code autonomously; the human did not line-by-line review the diff before commit, which is why the commit trailer records
autonomousrather thansupervised. The human is expected to be able to answer for this change during review.Disclosure per CONTRIBUTING.md — AI contributions in Spec Kit.