From 8dcd6f77f3e7a26e6e711273ba2c631dad69b51f Mon Sep 17 00:00:00 2001 From: aka Date: Sat, 21 Mar 2026 07:21:39 +1030 Subject: [PATCH] refactor: drop gherlint, use pure Python AST lint checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gherlint requires pydantic v1 which conflicts with our stack, and was called via subprocess through the sandbox — never actually available in practice. Replace with pure Python lint checks against the gherkin-official parsed AST. No subprocess, no sandbox dependency, no external tools. Checks: E001 (no Feature), W001-W006 (naming, empty scenarios, empty background), C001-C004 (missing Given/When/Then, bad background). 19 tests, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/components/validate_gherkin.py | 245 ++++++++++--------- platform/requirements.txt | 4 - platform/tests/test_validate_gherkin.py | 309 ++++++++++++------------ 3 files changed, 282 insertions(+), 276 deletions(-) diff --git a/platform/components/validate_gherkin.py b/platform/components/validate_gherkin.py index ac137f5..3731a91 100644 --- a/platform/components/validate_gherkin.py +++ b/platform/components/validate_gherkin.py @@ -1,12 +1,14 @@ -"""Validate Gherkin tool component — syntax parsing and lint checks.""" +"""Validate Gherkin tool component — syntax parsing and structural lint checks. + +Tier 1: Syntax validation via gherkin-official parser. +Tier 2: Structural lint checks via pure Python against the parsed AST. +No external lint tools or subprocess calls required. +""" from __future__ import annotations -import base64 import json import logging -import shlex -import uuid from langchain_core.tools import tool @@ -19,27 +21,6 @@ def validate_gherkin_factory(node): """Return a LangChain tool that validates Gherkin .feature specs.""" - # Resolve parent workspace and build sandbox backend (same pattern as run_command) - from components.run_command import _resolve_parent_workspace - from components._agent_shared import _build_backend - - parent_extra = _resolve_parent_workspace(node) - backend = None - if parent_extra.get("workspace_id"): - try: - backend = _build_backend(parent_extra) - logger.info( - "validate_gherkin %s: using sandbox backend (workspace_id=%s)", - node.node_id, - parent_extra["workspace_id"], - ) - except Exception: - logger.warning( - "validate_gherkin %s: failed to build sandbox backend, lint checks will be skipped", - node.node_id, - exc_info=True, - ) - @tool def validate_gherkin(gherkin_spec: str) -> str: """Validate a Gherkin feature spec for syntax errors and lint warnings. @@ -71,15 +52,13 @@ def validate_gherkin(gherkin_spec: str) -> str: from gherkin.parser import Parser parser = Parser() - parser.parse(gherkin_spec) + doc = parser.parse(gherkin_spec) except Exception as e: result["valid"] = False error_info = {"message": str(e), "line": 0} - # Try to extract line number from the error message err_str = str(e) if "(" in err_str and ":" in err_str: try: - # gherkin-official errors often contain "(line:col)" patterns parts = err_str.split("(") for part in parts: if ":" in part and ")" in part: @@ -92,106 +71,134 @@ def validate_gherkin(gherkin_spec: str) -> str: result["parse_errors"].append(error_info) return json.dumps(result) - # ── Tier 2: Lint via gherlint CLI (sandboxed) ──────────────────── - if backend is not None: - try: - # Encode content as base64 so arbitrary Gherkin can be safely - # embedded in a shell command without quoting issues. - encoded = base64.b64encode(gherkin_spec.encode()).decode() - temp_filename = f"/tmp/_validate_gherkin_{uuid.uuid4().hex}.feature" - cmd = ( - f"echo {shlex.quote(encoded)} | base64 -d > {shlex.quote(temp_filename)}" - f" && gherlint lint {shlex.quote(temp_filename)}" - f"; STATUS=$?; rm -f {shlex.quote(temp_filename)}; exit $STATUS" - ) - resp = backend.execute(cmd, timeout=30) - _parse_gherlint_output(resp.output or "", "", resp.exit_code or 0, result) - except Exception: - logger.warning("gherlint lint failed", exc_info=True) - else: - logger.debug("validate_gherkin: no sandbox backend, skipping lint checks") + # ── Tier 2: Structural lint checks against parsed AST ──────────── + _lint_ast(doc, result) return json.dumps(result) return validate_gherkin -def _parse_gherlint_output( - stdout: str, stderr: str, returncode: int, result: dict -) -> None: - """Parse gherlint CLI output and populate result dict.""" - output = (stdout + "\n" + stderr).strip() - if not output: +def _lint_ast(doc: dict, result: dict) -> None: + """Run structural lint checks against a parsed Gherkin AST.""" + feature = doc.get("feature") + + if not feature: + result["lint_errors"].append({ + "code": "E001", + "message": "No Feature block found", + "line": 0, + }) + result["valid"] = False return - for line in output.splitlines(): - line = line.strip() - if not line: - continue + # Check feature has a name + if not feature.get("name", "").strip(): + result["lint_warnings"].append({ + "code": "W001", + "message": "Feature has no name", + "line": feature.get("location", {}).get("line", 0), + }) + + children = feature.get("children", []) + scenarios = [c for c in children if "scenario" in c] + backgrounds = [c for c in children if "background" in c] + + # Check feature has scenarios + if not scenarios: + result["lint_warnings"].append({ + "code": "W002", + "message": "Feature has no scenarios", + "line": feature.get("location", {}).get("line", 0), + }) + return + + # Check for duplicate scenario names + seen_names: dict[str, int] = {} + for child in scenarios: + sc = child["scenario"] + name = sc.get("name", "").strip() + line = sc.get("location", {}).get("line", 0) + if name: + if name in seen_names: + result["lint_warnings"].append({ + "code": "W003", + "message": f"Duplicate scenario name: '{name}' (first at line {seen_names[name]})", + "line": line, + }) + else: + seen_names[name] = line + + # Check each scenario + for child in scenarios: + sc = child["scenario"] + name = sc.get("name", "").strip() + line = sc.get("location", {}).get("line", 0) + steps = sc.get("steps", []) + + # Unnamed scenario + if not name: + result["lint_warnings"].append({ + "code": "W004", + "message": "Scenario has no name", + "line": line, + }) - # gherlint output format is typically: - # filename:line:col: CODE message - # or just warning/error messages - entry = _parse_lint_line(line) - if entry is None: + # Empty scenario + if not steps: + result["lint_warnings"].append({ + "code": "W005", + "message": f"Scenario '{name or '(unnamed)'}' has no steps", + "line": line, + }) continue - code = entry.get("code", "") - # Convention: Cxxx = convention, Wxxx = warning, Exxx = error - if code.startswith("E"): - result["lint_errors"].append(entry) - result["valid"] = False - else: - # W, C, and other codes are treated as warnings - result["lint_warnings"].append(entry) - - -def _parse_lint_line(line: str) -> dict | None: - """Parse a single gherlint output line into a structured dict. - - Expected formats: - filename.feature:10:1: C0101 Step should start with a capital letter - filename.feature:5: W0301 Scenario has no Given step - C0101: Step should start with a capital letter (line 10) - """ - # Format: path:line:col: CODE message - parts = line.split(":", maxsplit=3) - if len(parts) >= 4: - try: - line_no = int(parts[1].strip()) - remainder = parts[3].strip() - code, _, message = remainder.partition(" ") - if code and code[0].isalpha() and any(c.isdigit() for c in code): - return {"code": code, "message": message.strip(), "line": line_no} - except (ValueError, IndexError): - pass - - # Format: path:line: CODE message (no column) - if len(parts) >= 3: - try: - line_no = int(parts[1].strip()) - remainder = parts[2].strip() - code, _, message = remainder.partition(" ") - if code and code[0].isalpha() and any(c.isdigit() for c in code): - return {"code": code, "message": message.strip(), "line": line_no} - except (ValueError, IndexError): - pass - - # Format: CODE: message (line N) or CODE message - if line and line[0].isalpha(): - code_part = line.split()[0].rstrip(":") - if any(c.isdigit() for c in code_part): - message = line[len(code_part):].strip().lstrip(": ") - line_no = 0 - # Try to extract (line N) from message - if "(line" in message: - try: - idx = message.index("(line") - num = message[idx + 5:].split(")")[0].strip() - line_no = int(num) - message = message[:idx].strip() - except (ValueError, IndexError): - pass - return {"code": code_part, "message": message, "line": line_no} + # Extract keyword types (Given, When, Then, And, But, *) + keywords = [s.get("keyword", "").strip() for s in steps] + + # Check for missing Given + if "Given" not in keywords: + result["lint_warnings"].append({ + "code": "C001", + "message": f"Scenario '{name or '(unnamed)'}' has no Given step", + "line": line, + }) + + # Check for missing When + if "When" not in keywords: + result["lint_warnings"].append({ + "code": "C002", + "message": f"Scenario '{name or '(unnamed)'}' has no When step", + "line": line, + }) + + # Check for missing Then + if "Then" not in keywords: + result["lint_warnings"].append({ + "code": "C003", + "message": f"Scenario '{name or '(unnamed)'}' has no Then step", + "line": line, + }) + + # Check backgrounds + for child in backgrounds: + bg = child["background"] + bg_steps = bg.get("steps", []) + bg_line = bg.get("location", {}).get("line", 0) + + if not bg_steps: + result["lint_warnings"].append({ + "code": "W006", + "message": "Background has no steps", + "line": bg_line, + }) - return None + # Background should only contain Given steps + for step in bg_steps: + kw = step.get("keyword", "").strip() + if kw not in ("Given", "And", "But", "*"): + result["lint_warnings"].append({ + "code": "C004", + "message": f"Background contains non-Given step: '{kw}'", + "line": step.get("location", {}).get("line", 0), + }) diff --git a/platform/requirements.txt b/platform/requirements.txt index bbc76be..5ef7d24 100644 --- a/platform/requirements.txt +++ b/platform/requirements.txt @@ -24,10 +24,6 @@ requests>=2.31 jinja2>=3.1 pyotp>=2.9 gherkin-official>=29.0.0 -# gherlint is used via subprocess (CLI), NOT as a Python import. -# It requires pydantic v1 which conflicts with our stack. -# Install separately: pipx install gherlint -# The validate_gherkin tool gracefully handles gherlint being absent. # Testing pytest>=8.0 diff --git a/platform/tests/test_validate_gherkin.py b/platform/tests/test_validate_gherkin.py index 9499653..51359ad 100644 --- a/platform/tests/test_validate_gherkin.py +++ b/platform/tests/test_validate_gherkin.py @@ -3,17 +3,10 @@ from __future__ import annotations import json -import sys -from pathlib import Path from types import SimpleNamespace -from unittest.mock import patch, MagicMock import pytest -_platform_dir = str(Path(__file__).resolve().parent.parent) -if _platform_dir not in sys.path: - sys.path.insert(0, _platform_dir) - # ── helpers ──────────────────────────────────────────────────────────────────── @@ -32,35 +25,11 @@ def _make_node(): ) -def _exec_resp(output="", exit_code=0): - """Build a fake backend.execute() response.""" - resp = MagicMock() - resp.output = output - resp.exit_code = exit_code - return resp - - -def _get_tool(lint_resp=None): - """Create a validate_gherkin tool instance. - - When *lint_resp* is None, no sandbox backend is injected (lint skipped). - When *lint_resp* is provided, a mock backend returning that response is used. - """ +def _get_tool(): + """Create a validate_gherkin tool instance.""" from components.validate_gherkin import validate_gherkin_factory node = _make_node() - - if lint_resp is None: - # No workspace → no backend → lint tier skipped - with patch("components.run_command._resolve_parent_workspace", return_value={}): - return validate_gherkin_factory(node) - - mock_backend = MagicMock() - mock_backend.execute.return_value = lint_resp - with ( - patch("components.run_command._resolve_parent_workspace", return_value={"workspace_id": "test-ws"}), - patch("components._agent_shared._build_backend", return_value=mock_backend), - ): - return validate_gherkin_factory(node) + return validate_gherkin_factory(node) VALID_FEATURE = """\ @@ -71,12 +40,6 @@ def _get_tool(lint_resp=None): Then the user is redirected to the dashboard """ -MALFORMED_FEATURE = """\ -This is not valid Gherkin at all - No Feature keyword here - Just random indented text -""" - NO_GIVEN_FEATURE = """\ Feature: Missing Given Scenario: No given step @@ -84,6 +47,68 @@ def _get_tool(lint_resp=None): Then something happens """ +NO_WHEN_FEATURE = """\ +Feature: Missing When + Scenario: No when step + Given some precondition + Then something happens +""" + +NO_THEN_FEATURE = """\ +Feature: Missing Then + Scenario: No then step + Given some precondition + When something happens +""" + +EMPTY_SCENARIO = """\ +Feature: Empty + Scenario: Nothing here +""" + +UNNAMED_SCENARIO = """\ +Feature: Test + Scenario: + Given something + When action + Then result +""" + +DUPLICATE_NAMES = """\ +Feature: Dups + Scenario: Do something + Given x + When y + Then z + Scenario: Do something + Given a + When b + Then c +""" + +UNNAMED_FEATURE = """\ +Feature: + Scenario: Test + Given x + When y + Then z +""" + +NO_SCENARIOS = """\ +Feature: Empty feature +""" + +BACKGROUND_WITH_WHEN = """\ +Feature: Bad background + Background: + Given some setup + When bad step in background + Scenario: Test + Given x + When y + Then z +""" + # ── Tests ───────────────────────────────────────────────────────────────────── @@ -100,88 +125,36 @@ def test_factory_returns_tool(self): class TestValidateGherkinValidSpec: def test_valid_gherkin_returns_valid(self): - """Valid Gherkin spec should return valid: true with no errors.""" - tool = _get_tool(lint_resp=_exec_resp(output="", exit_code=0)) + tool = _get_tool() raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) result = json.loads(raw) assert result["valid"] is True assert result["parse_errors"] == [] assert result["lint_errors"] == [] - - def test_valid_gherkin_no_lint_errors(self): - """When gherlint reports no issues, lint_errors should be empty.""" - tool = _get_tool(lint_resp=_exec_resp(output="", exit_code=0)) - raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) - result = json.loads(raw) - - assert result["lint_errors"] == [] + assert result["lint_warnings"] == [] class TestValidateGherkinSyntaxError: - def test_malformed_gherkin_returns_parse_error(self): - """Malformed Gherkin (bad keyword) should produce a parse error.""" + def test_non_gherkin_text_returns_parse_error(self): tool = _get_tool() - raw = tool.invoke({"gherkin_spec": MALFORMED_FEATURE}) + raw = tool.invoke({"gherkin_spec": "This is not Gherkin at all."}) result = json.loads(raw) assert result["valid"] is False assert len(result["parse_errors"]) > 0 - assert result["parse_errors"][0]["message"] - -class TestValidateGherkinLintWarnings: - def test_lint_warnings_populated(self): - """Lint warnings should be captured from gherlint output.""" - resp = _exec_resp( - output="test.feature:3:1: C0101 Step should start with a capital letter\n", - exit_code=0, - ) - tool = _get_tool(lint_resp=resp) - raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) - result = json.loads(raw) - - assert result["valid"] is True - assert len(result["lint_warnings"]) == 1 - assert result["lint_warnings"][0]["code"] == "C0101" - assert result["lint_warnings"][0]["line"] == 3 - - def test_lint_errors_set_valid_false(self): - """Lint errors (E-codes) should set valid to false.""" - resp = _exec_resp( - output="test.feature:5:1: E0001 Critical error found\n", - exit_code=1, - ) - tool = _get_tool(lint_resp=resp) - raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) - result = json.loads(raw) - - assert result["valid"] is False - assert len(result["lint_errors"]) == 1 - assert result["lint_errors"][0]["code"] == "E0001" - - def test_multiple_warnings_and_errors(self): - """Multiple lint issues should all be captured.""" - resp = _exec_resp( - output=( - "test.feature:3:1: C0101 Step lowercase\n" - "test.feature:5:1: W0301 Missing Given\n" - "test.feature:8:1: E0001 Critical problem\n" - ), - exit_code=1, - ) - tool = _get_tool(lint_resp=resp) - raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) + def test_python_code_returns_parse_error(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": "def hello():\n print('hello')\n"}) result = json.loads(raw) assert result["valid"] is False - assert len(result["lint_warnings"]) == 2 # C0101 + W0301 - assert len(result["lint_errors"]) == 1 # E0001 + assert len(result["parse_errors"]) > 0 class TestValidateGherkinEmptyInput: def test_empty_string_returns_error(self): - """Empty string should return valid: false with an error.""" tool = _get_tool() raw = tool.invoke({"gherkin_spec": ""}) result = json.loads(raw) @@ -191,7 +164,6 @@ def test_empty_string_returns_error(self): assert "empty" in result["parse_errors"][0]["message"].lower() def test_whitespace_only_returns_error(self): - """Whitespace-only input should be treated as empty.""" tool = _get_tool() raw = tool.invoke({"gherkin_spec": " \n\t\n "}) result = json.loads(raw) @@ -200,72 +172,103 @@ def test_whitespace_only_returns_error(self): assert len(result["parse_errors"]) > 0 -class TestValidateGherkinNonGherkin: - def test_random_text_returns_parse_error(self): - """Non-Gherkin text should produce a parse error.""" +class TestValidateGherkinLintChecks: + def test_missing_given(self): tool = _get_tool() - raw = tool.invoke({"gherkin_spec": "This is just random text, not Gherkin at all."}) + raw = tool.invoke({"gherkin_spec": NO_GIVEN_FEATURE}) result = json.loads(raw) - assert result["valid"] is False - assert len(result["parse_errors"]) > 0 + assert result["valid"] is True # lint warnings don't fail validation + codes = [w["code"] for w in result["lint_warnings"]] + assert "C001" in codes - def test_python_code_returns_parse_error(self): - """Python code input should produce a parse error.""" + def test_missing_when(self): tool = _get_tool() - raw = tool.invoke({"gherkin_spec": "def hello():\n print('hello')\n"}) + raw = tool.invoke({"gherkin_spec": NO_WHEN_FEATURE}) result = json.loads(raw) - assert result["valid"] is False - assert len(result["parse_errors"]) > 0 + codes = [w["code"] for w in result["lint_warnings"]] + assert "C002" in codes + + def test_missing_then(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": NO_THEN_FEATURE}) + result = json.loads(raw) + codes = [w["code"] for w in result["lint_warnings"]] + assert "C003" in codes + + def test_empty_scenario(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": EMPTY_SCENARIO}) + result = json.loads(raw) + + codes = [w["code"] for w in result["lint_warnings"]] + assert "W005" in codes + + def test_unnamed_scenario(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": UNNAMED_SCENARIO}) + result = json.loads(raw) + + codes = [w["code"] for w in result["lint_warnings"]] + assert "W004" in codes + + def test_duplicate_scenario_names(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": DUPLICATE_NAMES}) + result = json.loads(raw) -class TestValidateGherkinNoBackend: - def test_no_backend_still_validates_syntax(self): - """When no sandbox backend is available, syntax validation still works.""" - tool = _get_tool() # no lint_resp → no backend + codes = [w["code"] for w in result["lint_warnings"]] + assert "W003" in codes + + def test_unnamed_feature(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": UNNAMED_FEATURE}) + result = json.loads(raw) + + codes = [w["code"] for w in result["lint_warnings"]] + assert "W001" in codes + + def test_no_scenarios(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": NO_SCENARIOS}) + result = json.loads(raw) + + codes = [w["code"] for w in result["lint_warnings"]] + assert "W002" in codes + + def test_background_with_non_given_step(self): + tool = _get_tool() + raw = tool.invoke({"gherkin_spec": BACKGROUND_WITH_WHEN}) + result = json.loads(raw) + + codes = [w["code"] for w in result["lint_warnings"]] + assert "C004" in codes + + def test_valid_spec_no_warnings(self): + """A well-formed spec should produce zero warnings.""" + tool = _get_tool() raw = tool.invoke({"gherkin_spec": VALID_FEATURE}) result = json.loads(raw) - assert result["valid"] is True - assert result["parse_errors"] == [] - # Lint results empty since no backend available assert result["lint_warnings"] == [] assert result["lint_errors"] == [] - def test_no_backend_syntax_error_still_caught(self): - """Syntax errors are caught even without a backend.""" - tool = _get_tool() # no lint_resp → no backend - raw = tool.invoke({"gherkin_spec": MALFORMED_FEATURE}) - result = json.loads(raw) - assert result["valid"] is False - assert len(result["parse_errors"]) > 0 +class TestLintAstDirect: + """Test _lint_ast directly for edge cases.""" - -class TestParseLintLine: - def test_standard_format(self): - from components.validate_gherkin import _parse_lint_line - entry = _parse_lint_line("test.feature:10:1: C0101 Step should start with a capital letter") - assert entry is not None - assert entry["code"] == "C0101" - assert entry["line"] == 10 - assert "capital" in entry["message"].lower() - - def test_no_column_format(self): - from components.validate_gherkin import _parse_lint_line - entry = _parse_lint_line("test.feature:5: W0301 Scenario has no Given step") - assert entry is not None - assert entry["code"] == "W0301" - assert entry["line"] == 5 - - def test_bare_code_format(self): - from components.validate_gherkin import _parse_lint_line - entry = _parse_lint_line("C0101: Step should start with a capital letter") - assert entry is not None - assert entry["code"] == "C0101" - - def test_non_lint_line_returns_none(self): - from components.validate_gherkin import _parse_lint_line - assert _parse_lint_line("") is None - assert _parse_lint_line("some random output") is None + def test_no_feature_key(self): + from components.validate_gherkin import _lint_ast + result = {"valid": True, "parse_errors": [], "lint_warnings": [], "lint_errors": []} + _lint_ast({}, result) + assert result["valid"] is False + assert result["lint_errors"][0]["code"] == "E001" + + def test_feature_with_no_children(self): + from components.validate_gherkin import _lint_ast + result = {"valid": True, "parse_errors": [], "lint_warnings": [], "lint_errors": []} + _lint_ast({"feature": {"name": "Test", "children": []}}, result) + codes = [w["code"] for w in result["lint_warnings"]] + assert "W002" in codes