diff --git a/docs/changelog.rst b/docs/changelog.rst index 0e792e3..2c09418 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -8,6 +8,12 @@ Unreleased +++++++++++++++++++++++++++++++++++ * New rule set :doc:`rules/dag` to validate DAG templates and their tasks. +* Redesign :doc:`rules/workflow-variable` to improve validation of Argo workflow variable references: + + * Rule ``VAR001`` has been renamed to :rule:`var101`. + * Rule ``VAR002`` has been renamed to :rule:`var201`. + * New rule :rule:`var102` to flag invalid usage of expr-lang format in simple tags. + * New rule :rule:`var202` to flag non-Argo variable references. :octicon:`plug` Enhancements ++++++++++++++++++++++++++++ diff --git a/docs/rules/variable.rst b/docs/rules/variable.rst new file mode 100644 index 0000000..587a975 --- /dev/null +++ b/docs/rules/variable.rst @@ -0,0 +1,147 @@ +Workflow Variable Rules (``VAR``) +================================= + +The code ``VAR`` identifies potential issues with `workflow variables`_, including syntax errors, misused functions, and other related problems. + +.. _workflow variables: https://argo-workflows.readthedocs.io/en/latest/variables/ + +.. admonition:: Known Limitations + :class: caution + + Currently, Tugboat supports only `simple tags`_, and does not support `expression tags`_. + + This means: + + * ``{{ inputs.parameters.simple }}`` can be checked + + * ``{{= inputs.parameters.expression }}`` will be ignored + + We are planning to add support for expression tags in an upcoming release. + + .. _simple tags: https://argo-workflows.readthedocs.io/en/latest/variables/#simple + .. _expression tags: https://argo-workflows.readthedocs.io/en/latest/variables/#expression + + +.. VAR1xx syntax errors + +.. rule:: VAR101 Syntax error + + This error occurs when a workflow variable fails to parse due to a syntax error. + + Note that most parsers report syntax errors as soon as they encounter the first issue. + This means the error message might not always indicate the exact location of the problem, but it provides a useful starting point for debugging. + +.. rule:: VAR102 Incorrect template tag format + + This error occurs when a reference uses expression tag syntax inside a simple template tag. + + Argo Workflows supports two types of template tags: + + - **Simple tags**: ``{{ inputs.parameters.foo }}`` + - **Expression tags**: ``{{= inputs.parameters.foo }}`` + + The syntax for referencing values differs between these two formats. + Simple tags use dot notation only (e.g., ``inputs.parameters.foo``), while expression tags use `expr-lang`_ syntax, which supports both dot notation and bracket notation for member access (e.g., ``inputs.parameters['foo']`` or ``inputs["parameters"]["foo"]``). + + This error is reported when expression tag syntax is mistakenly used inside a simple tag. + + .. code-block:: yaml + :emphasize-lines: 14 + + apiVersion: argoproj.io/v1alpha1 + kind: Workflow + metadata: + generateName: test- + spec: + entrypoint: whalesay + templates: + - name: whalesay + inputs: + parameters: + - name: message + container: + image: docker/whalesay:latest + args: ["{{ inputs.parameters['message'] }}"] + + In the example above, ``inputs.parameters['message']`` uses bracket notation, which is valid in expression tags but not in simple tags. + The correct format for a simple tag is ``{{ inputs.parameters.message }}``. + + .. _expr-lang: https://expr-lang.org/ + + +.. VAR2xx invalid references + +.. rule:: VAR201 Unknown Argo workflow variable reference + + This error occurs when a reference is not recognized as a valid Argo workflow variable in the current context. + + Tugboat validates references against a list of known Argo workflow variables based on the template type and defined inputs/outputs. + If a reference cannot be matched to any known variable, this error is reported. + + .. code-block:: yaml + :emphasize-lines: 18 + + apiVersion: argoproj.io/v1alpha1 + kind: Workflow + metadata: + generateName: test- + spec: + entrypoint: main + templates: + - name: main + inputs: + parameters: + - name: message + steps: + - - name: step-1 + template: produce + arguments: + parameters: + - name: msg + value: "{{ inputs.parameters.msg }}" + - - name: step-2 + template: consume + arguments: + parameters: + - name: msg + value: "{{ steps.step-1.outputs.parameters.massage }}" + + In the example above, ``inputs.parameters.msg`` is reported because the defined parameter is named ``message``, not ``msg``. + + When possible, Tugboat will suggest the closest matching variable name as a fix. + + .. note:: + + Tugboat can only validate references within the same manifest. + References to step outputs (e.g., ``steps.step-1.outputs.parameters.message``) cannot be fully validated because the output is defined by the referenced template, which may be a WorkflowTemplate defined elsewhere. + +.. rule:: VAR202 Not an Argo workflow variable reference + + This warning occurs when a template tag contains a single identifier that does not match any known Argo workflow variable pattern. + + Argo workflow variables typically have a dotted format like ``inputs.parameters.name`` or ``workflow.name``. + When a tag contains only a simple variable name (e.g., ``{{ foo }}``), it is unlikely to be an Argo variable. + + .. code-block:: yaml + :emphasize-lines: 14 + + apiVersion: argoproj.io/v1alpha1 + kind: Workflow + metadata: + generateName: test- + spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: docker/whalesay:latest + args: ["{{ message }}"] + + In the example above, ``{{ message }}`` is flagged because ``message`` is not a valid Argo workflow variable. + + This warning is commonly triggered when: + + - The manifest uses another templating engine (e.g., Jinja2, Helm) that shares the ``{{ }}`` syntax + - There is a typo or incomplete variable reference + + If the tag is intended for another templating engine, you can suppress this warning by adding a ``# noqa: VAR202`` comment. See :doc:`../violations` for more information on suppressing warnings. diff --git a/poetry.lock b/poetry.lock index d6956de..98c9576 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1161,6 +1161,24 @@ enabler = ["pytest-enabler (>=3.4)"] test = ["pyfakefs", "pytest (>=6,!=8.1.*)"] type = ["pygobject-stubs", "pytest-mypy (>=1.0.1)", "shtab", "types-pywin32"] +[[package]] +name = "lark" +version = "1.3.1" +description = "a modern parsing library" +optional = false +python-versions = ">=3.8" +groups = ["main"] +files = [ + {file = "lark-1.3.1-py3-none-any.whl", hash = "sha256:c629b661023a014c37da873b4ff58a817398d12635d3bbb2c5a03be7fe5d1e12"}, + {file = "lark-1.3.1.tar.gz", hash = "sha256:b426a7a6d6d53189d318f2b6236ab5d6429eaf09259f1ca33eb716eed10d2905"}, +] + +[package.extras] +atomic-cache = ["atomicwrites"] +interegular = ["interegular (>=0.3.1,<0.4.0)"] +nearley = ["js2py"] +regex = ["regex"] + [[package]] name = "lxml" version = "6.0.2" @@ -2997,4 +3015,4 @@ mcp = ["mcp"] [metadata] lock-version = "2.1" python-versions = ">=3.12,<4.0" -content-hash = "2629217cc2e4382d6ad533c59af469d8cab4e989eac9ba4f02153ba5b77458c7" +content-hash = "17ce0fa4b51caeebc1de84b97ae0f055384de0feb4af20e101940eb6314d3c2b" diff --git a/pyproject.toml b/pyproject.toml index 7bf5233..1562880 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,6 +10,7 @@ dependencies = [ "cloup (>=3.0.5,<4.0.0)", "colorlog (>=6.9.0,<7.0.0)", "frozendict (>=2.4.6,<3.0.0)", + "lark (>=1.3.1,<2.0.0)", "pluggy (>=1.5.0,<2.0.0)", "pydantic (>=2.9.2,<3.0.0)", "pydantic-settings (>=2.7.1,<3.0.0)", diff --git a/tests/analyzers/test_template_tag.py b/tests/analyzers/test_template_tag.py new file mode 100644 index 0000000..913b424 --- /dev/null +++ b/tests/analyzers/test_template_tag.py @@ -0,0 +1,171 @@ +from unittest.mock import Mock + +import lark +import pytest +from dirty_equals import IsPartialDict, IsStr + +from tests.dirty_equals import HasSubstring +from tugboat.analyzers.template_tag import ( + check_simple_tag_reference, + check_template_tags, + parse_argo_template_tags, + split_expr_membership, +) +from tugboat.references import ReferenceCollection + + +class TestParseArgoTemplateTags: + + def test_pass(self): + tree = parse_argo_template_tags( + """ + Hello {{ inputs.parameters.name }} + This is {{ inputs.parameters['bot'] }} speaking. + + The result is {{= 1 + 2 }}. + """ + ) + + (input_name_tag, bot_name_tag) = tree.find_data("simple_tag") + + (input_name,) = input_name_tag.find_token("REF") + assert input_name == "inputs.parameters.name" + + (bot_name,) = bot_name_tag.find_token("REF") + assert bot_name == "inputs.parameters['bot']" + + def test_error(self): + with pytest.raises(lark.exceptions.UnexpectedCharacters): + parse_argo_template_tags("{{ inputs. parameters.name }}") + with pytest.raises(lark.exceptions.UnexpectedEOF): + parse_argo_template_tags("{{ inputs.parameters.name ") + + +class TestCheckTemplateTags: + + def test_pass(self): + references = ReferenceCollection() + references.add(("inputs", "parameters", "name")) + + diagnoses = list( + check_template_tags("{{ inputs.parameters.name }}", references) + ) + + assert diagnoses == [] + + def test_simple_tag_picked(self): + diagnoses = list(check_template_tags("{{ inputs }}", ReferenceCollection())) + assert diagnoses == [IsPartialDict(code="VAR202")] + + def test_unexpected_character(self): + diagnoses = list( + check_template_tags( + "{{ inputs. parameters.name }}", + Mock(ReferenceCollection), + ) + ) + assert diagnoses == [ + IsPartialDict( + code="VAR101", + summary="Syntax error", + msg=( + IsStr() + & HasSubstring("The field contains a syntax error") + & HasSubstring("{{ inputs. parameters.name }}") + & HasSubstring("This error is usually caused by invalid characters") + ), + ) + ] + + def test_unexpected_eof(self): + diagnoses = list( + check_template_tags( + "{{ inputs", + Mock(ReferenceCollection), + ) + ) + assert diagnoses == [ + IsPartialDict( + code="VAR101", + summary="Syntax error", + msg=( + IsStr() + & HasSubstring("The field contains a syntax error") + & HasSubstring("{{ inputs") + & HasSubstring( + "This error is usually caused by an incomplete template tag" + ) + ), + ) + ] + + +class TestCheckSimpleTagReference: + + def test_pass(self): + references = ReferenceCollection() + references.add(("inputs", "parameters", "name")) + + diagnosis = check_simple_tag_reference("inputs.parameters.name", references) + assert diagnosis is None + + def test_incorrect_format(self): + references = ReferenceCollection() + references.add(("inputs", "parameters", "name")) + + diagnosis = check_simple_tag_reference("inputs.parameters['name']", references) + + assert diagnosis == IsPartialDict( + code="VAR102", + summary="Incorrect template tag format", + fix="inputs.parameters.name", + ) + + def test_syntax_error(self): + references = ReferenceCollection() + diagnosis = check_simple_tag_reference("inputs.parameters['name", references) + assert diagnosis == IsPartialDict(code="VAR101") + + def test_not_a_argo_variable_1(self): + references = ReferenceCollection() + diagnosis = check_simple_tag_reference("inputs", references) + assert diagnosis == IsPartialDict(code="VAR202") + + def test_unknown_variable(self): + references = ReferenceCollection() + references.add(("demo",)) + + diagnosis = check_simple_tag_reference("inputs", references) + assert diagnosis == IsPartialDict( + code="VAR201", + fix="demo", + ctx={ + "reference": { + "found": ("inputs",), + "found:str": "inputs", + "closest": ("demo",), + "closest:str": "demo", + } + }, + ) + + +class TestSplitExprMembership: + + @pytest.mark.parametrize( + ("source", "expected"), + [ + # success + ("parameters", ("parameters",)), + ("parameters.item", ("parameters", "item")), + ("parameters['item-1'].subkey", ("parameters", "item-1", "subkey")), + ('parameters["item-2"].subkey', ("parameters", "item-2", "subkey")), + ("a.b.c['d'].e['f'].g", ("a", "b", "c", "d", "e", "f", "g")), + # failed + ("0", ()), + ("parameters[\"item']", ()), + ], + ) + def test(self, source: str, expected: tuple[str, ...]): + parts = split_expr_membership(source) + assert parts == expected diff --git a/tugboat/analyzers/template_tag.py b/tugboat/analyzers/template_tag.py new file mode 100644 index 0000000..ca15a75 --- /dev/null +++ b/tugboat/analyzers/template_tag.py @@ -0,0 +1,285 @@ +""" +Parser for Argo template tags. + +See Also +-------- +Workflow Variables + https://argo-workflows.readthedocs.io/en/latest/variables/ +""" + +from __future__ import annotations + +import functools +import io +import re +import textwrap +import typing + +import lark + +if typing.TYPE_CHECKING: + from collections.abc import Iterator + + from lark import Token + from lark.exceptions import UnexpectedInput + + from tugboat.references import ReferenceCollection + from tugboat.types import Diagnosis + + +@functools.cache +def _argo_template_tag_parser(): + return lark.Lark( + r""" + %import common.DIGIT + %import common.LETTER + %import common.WS + + ?start: template + + template: (_TEXT | expression_tag | simple_tag)+ + + # ignore other text + _TEXT: /[^{]+/ + | "{" /[^{]/ + + # simple template tag + simple_tag: "{{" WS? REF WS? "}}" + REF: (LETTER | DIGIT | "_" | "-" | "." | "\"" | "'" | "[" | "]")+ + + # expression template tag + expression_tag: "{{=" WS? _ANY WS? "}}" + _ANY: /[^}]+/ + """ + ) + + +@functools.lru_cache(32) +def parse_argo_template_tags(source: str) -> lark.Tree: + """ + Parse Argo template tags in the given source string. + + Parameters + ---------- + source : str + The source string containing Argo template tags. + + Returns + ------- + lark.Tree + The parse tree representing the structure of the template tags. + """ + parser = _argo_template_tag_parser() + return parser.parse(source) + + +def check_template_tags( + source: str, references: ReferenceCollection +) -> Iterator[Diagnosis]: + """ + Check the given source string for errors in Argo template tags. + + Parameters + ---------- + source : str + The source string containing Argo template tags. + references : ReferenceCollection + The current active references. + + Yields + ------ + Diagnosis + A diagnosis for each error found. + """ + try: + tree = parse_argo_template_tags(source) + except lark.UnexpectedInput as e: + yield transform_lark_error(source, e) + return + + # analyze simple tags + for tag in tree.find_data("simple_tag"): + (ref_repr,) = tag.find_token("REF") + ref_repr = typing.cast("Token", ref_repr) + if diagnosis := check_simple_tag_reference(ref_repr, references): + yield diagnosis + + +def transform_lark_error(source: str, e: UnexpectedInput) -> Diagnosis: + with io.StringIO() as buf: + buf.write("The field contains a syntax error for Argo template tags.") + buf.write("\n\n") + buf.write("The parser reported the errors near:\n\n") + buf.write(textwrap.indent(e.get_context(source), " ")) + + match type(e): + case lark.UnexpectedCharacters: + buf.write( + "\n" + "This error is usually caused by invalid characters in the template tag.\n" + "Please ensure that the template tags are correctly formatted." + ) + case lark.UnexpectedEOF: + buf.write( + "\n" + "This error is usually caused by an incomplete template tag.\n" + "Please ensure that all template tags are properly closed." + ) + + message = buf.getvalue() + + return { + "type": "error", + "code": "VAR101", + "loc": (), + "summary": "Syntax error", + "msg": message, + } + + +def check_simple_tag_reference( + ref_repr: str, references: ReferenceCollection +) -> Diagnosis | None: + # early exit if the reference is known + ref = tuple(ref_repr.split(".")) + if ref in references: + return + + # case: user mistakenly use the expression tag format + if parts := split_expr_membership(ref_repr): + if parts in references: + # case: the reference exists, but the format is wrong + # this is common when the reference is built by coding agents + fix = ".".join(parts) + return { + "code": "VAR102", + "loc": (), + "summary": "Incorrect template tag format", + "msg": ( + f""" + The reference '{ref_repr}' mistakes the expression tag format in a simple tag. + Use the simple tag format instead, or convert it to an expression tag ({{{{= ... }}}}) if needed. + """ + ), + "input": ref_repr, + "fix": fix, + } + + else: + # case: the reference does not exist + # leave it to the unknown variable checker + ref = parts + ref_repr = ".".join(parts) + + if any(sym in ref_repr for sym in "'\"[]"): + # well-formed bracket notations should have been handled by split_expr_membership above + # reaching here means the reference contains unexpected characters + return { + "code": "VAR101", + "loc": (), + "summary": "Syntax error", + "msg": ( + f""" + The input '{ref_repr}' contains invalid characters for simple template tag. + + Simple tags only support dot notation (e.g., `inputs.parameters.name`). + For complex expressions, use expression tags instead: `{{{{= ... }}}}`. + """ + ), + "input": ref_repr, + } + + # case: the reference doesn't look like an Argo variable + if is_variable(ref_repr) and not has_simple_variable(references): + return { + "type": "warning", + "code": "VAR202", + "loc": (), + "summary": "Not a Argo workflow variable reference", + "msg": ( + f""" + The used reference '{ref_repr}' is invalid for Argo workflow variables. + + If this `{{{{ }}}}` tag is intended for other templating engines (e.g., Jinja), + you can ignore this warning, or suppress it by adding a: + + # noqa: VAR202; non-Argo variable reference + + comment on the field or model using this variable. + """ + ), + "input": ref_repr, + } + + # case: unknown variable + metadata = { + "found": ref, + "found:str": ref_repr, + } + + if closest := references.find_closest(ref): + metadata["closest"] = closest + metadata["closest:str"] = ".".join(closest) + + return { + "code": "VAR201", + "loc": (), + "summary": "Unknown Argo workflow variable reference", + "msg": ( + f"The reference '{ref_repr}' is not recognized as a valid Argo workflow variable under the current context." + ), + "input": ref_repr, + "fix": metadata.get("closest:str"), + "ctx": {"reference": metadata}, + } + + +def split_expr_membership(source: str) -> tuple[str, ...]: + """ + Split an expr lang membership string into its components. + + Parameters + ---------- + source : str + The expression membership string. + + Returns + ------- + tuple[str, ...] + The components of the expression membership. + """ + # extract first part + match_ = re.match(r"[a-zA-Z_]\w*", source) + if not match_: + return () + + parts = [match_.group(0)] + + regex_parts = re.compile( + r""" + \.([a-zA-Z_]\w*) # dot notation + |\['([\w-]+)'\] # single-quoted bracket notation + |\[\"([\w-]+)\"\] # double-quoted bracket notation + """, + re.VERBOSE, + ) + + while match_.end() < len(source): + match_ = regex_parts.match(source, match_.end()) + if not match_: + return () + + parts += filter(None, match_.groups()) + + return tuple(parts) + + +def is_variable(s: str) -> bool: + return re.fullmatch(r"[_a-zA-Z][_a-zA-Z0-9]*", s) is not None + + +def has_simple_variable(references: ReferenceCollection) -> bool: + for ref in references: + if len(ref) == 1: + return True + return False diff --git a/tugboat/references/__init__.py b/tugboat/references/__init__.py index 3554f32..f35587d 100644 --- a/tugboat/references/__init__.py +++ b/tugboat/references/__init__.py @@ -10,6 +10,7 @@ __all__ = [ "Context", + "ReferenceCollection", "get_global_context", "get_step_context", "get_task_context", @@ -17,7 +18,7 @@ "get_workflow_context", ] -from tugboat.references.context import Context +from tugboat.references.context import Context, ReferenceCollection from tugboat.references.step import get_step_context, get_task_context from tugboat.references.template import get_template_context from tugboat.references.workflow import get_global_context, get_workflow_context