From e3a286ef6fe25b8e0099cd5e49d88d032f86ce9a Mon Sep 17 00:00:00 2001 From: Gros Frumos Date: Wed, 18 Mar 2026 14:06:23 +0200 Subject: [PATCH] kin: auto-commit after pipeline --- agents/runner.py | 16 +- core/followup.py | 13 +- core/models.py | 16 +- tests/test_kin_111_regression.py | 1314 +++++++++++++++++++++++++++++- tests/test_kin_124_regression.py | 245 ++++++ 5 files changed, 1598 insertions(+), 6 deletions(-) create mode 100644 tests/test_kin_124_regression.py diff --git a/agents/runner.py b/agents/runner.py index a57b9de..44ee780 100644 --- a/agents/runner.py +++ b/agents/runner.py @@ -826,7 +826,7 @@ _WORKTREE_ROLES = {"backend_dev", "frontend_dev", "debugger"} _DEV_GUARD_ROLES = {"backend_dev", "frontend_dev", "debugger"} -def _detect_test_command(project_path: str) -> str | None: +def _detect_test_command(project_path: str, role: str | None = None) -> str | None: """Auto-detect test command by inspecting project files. Candidates (in priority order): @@ -835,10 +835,22 @@ def _detect_test_command(project_path: str) -> str | None: 3. pytest — pyproject.toml or setup.py exists 4. npx tsc --noEmit — tsconfig.json exists + When role='backend_dev' and a Python project marker (pyproject.toml / setup.py) + is present, pytest is returned directly — bypassing make test. This prevents + false-positive failures in mixed projects whose Makefile test target also runs + frontend (e.g. vitest) commands that may be unrelated to backend changes. + Returns the first matching command, or None if no framework is detected. """ path = Path(project_path) + # For backend_dev: Python project marker takes precedence over Makefile. + # Rationale: make test in mixed projects often runs frontend tests too; + # backend changes should only be validated by the Python test runner. + if role == "backend_dev": + if (path / "pyproject.toml").is_file() or (path / "setup.py").is_file(): + return f"{sys.executable} -m pytest" + # 1. make test makefile = path / "Makefile" if makefile.is_file(): @@ -1882,7 +1894,7 @@ def run_pipeline( if p_test_cmd_override: p_test_cmd = p_test_cmd_override else: - p_test_cmd = _detect_test_command(p_path_str) + p_test_cmd = _detect_test_command(p_path_str, role=role) if p_test_cmd is None: # No test framework detected — skip without blocking pipeline diff --git a/core/followup.py b/core/followup.py index aa8f365..d7b9c80 100644 --- a/core/followup.py +++ b/core/followup.py @@ -126,16 +126,25 @@ def generate_followups( parsed = _try_parse_json(output) if not isinstance(parsed, list): if isinstance(parsed, dict): - parsed = parsed.get("tasks") or parsed.get("followups") or [] + if "tasks" in parsed: + parsed = parsed["tasks"] + elif "followups" in parsed: + parsed = parsed["followups"] + else: + parsed = [] else: return {"created": [], "pending_actions": []} + # Guard: extracted value might be null/non-list (e.g. {"tasks": null}) + if not isinstance(parsed, list): + parsed = [] + # Separate permission-blocked items from normal ones created = [] pending_actions = [] for item in parsed: - if not isinstance(item, dict) or "title" not in item: + if not isinstance(item, dict) or not item.get("title"): continue if _is_permission_blocked(item): diff --git a/core/models.py b/core/models.py index eb666f0..62587a6 100644 --- a/core/models.py +++ b/core/models.py @@ -31,13 +31,27 @@ def validate_completion_mode(value: str) -> str: return "review" +# Columns that are stored as JSON strings and must be decoded on read. +# Text fields (title, description, name, etc.) are NOT in this set. +_JSON_COLUMNS: frozenset[str] = frozenset({ + "tech_stack", + "brief", "spec", "review", "test_result", "security_result", "labels", + "tags", + "dependencies", + "steps", + "artifacts", "decisions_made", "blockers", + "extra_json", + "pending_actions", +}) + + def _row_to_dict(row: sqlite3.Row | None) -> dict | None: """Convert sqlite3.Row to dict with JSON fields decoded.""" if row is None: return None d = dict(row) for key, val in d.items(): - if isinstance(val, str) and val.startswith(("[", "{")): + if key in _JSON_COLUMNS and isinstance(val, str) and val.startswith(("[", "{")): try: d[key] = json.loads(val) except (json.JSONDecodeError, ValueError): diff --git a/tests/test_kin_111_regression.py b/tests/test_kin_111_regression.py index f086cb0..e0db27b 100644 --- a/tests/test_kin_111_regression.py +++ b/tests/test_kin_111_regression.py @@ -23,9 +23,18 @@ Batch B coverage: (11) GET /api/projects returns Content-Type: application/json (not text/html) (12) PATCH /api/projects/{id} with worktrees_enabled=True → 200, not 400 Bad Request (13) POST /api/projects/{id}/deploy without deploy config → 400 (button blocked correctly) -(14) vite.config.ts has server.proxy for /api → proxy to FastAPI (the actual fix)""" +(14) vite.config.ts has server.proxy for /api → proxy to FastAPI (the actual fix) + +KIN-P1-001 revision (empty array edge cases — deeper investigation): +(15) _row_to_dict: text field title='[]' must NOT be decoded to list (latent bug) +(16) _row_to_dict: JSON field brief='[]' must be decoded to [] list (correct behavior) +(17) Task round-trip with brief=[] — stored and retrieved as empty list +(18) Task round-trip with title='[]' — title must stay string (latent bug, needs whitelist fix) +(19) Task brief=None — stays None after round-trip +(20) Task brief=['developer'] (single-element) — round-trips correctly""" import json +import sqlite3 import pytest from click.testing import CliRunner @@ -424,6 +433,36 @@ class TestGenerateFollowupsNullAndDict: ) assert result["pending_actions"] == [] + @patch("agents.runner._run_claude") + def test_dict_tasks_empty_followups_nonempty_does_not_create_tasks(self, mock_claude, conn): + """generate_followups: {"tasks": [], "followups": [{"title": "X"}]} → no tasks created. + + This tests the deeper edge case from KIN-P1-001 revision: + when tasks=[] (empty list is falsy), the `or` logic in line 129 of followup.py + incorrectly falls through to followups and creates tasks. + + BUG: `parsed.get('tasks') or parsed.get('followups') or []` + — [] is falsy, so followups is used even though tasks was explicitly set. + + Fix: use explicit key-presence check instead of truthiness check. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [], "followups": [{"title": "Follow-up task", "type": "backend_dev"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected created=[] when tasks=[] (even with non-empty followups), got: {result['created']}. " + "Bug in followup.py line 129: empty list is falsy, so `or` falls through to followups key." + ) + assert tasks_after == tasks_before, ( + f"Task count changed from {tasks_before} to {tasks_after}. " + "No tasks must be created when tasks key is explicitly empty." + ) + # --------------------------------------------------------------------------- # Batch B — (11/12/13/14) Deploy button, Settings JSON, Worktrees toggle @@ -526,3 +565,1276 @@ class TestViteProxyConfig: assert "/api" in content or "'/api'" in content or '"/api"' in content, ( "vite.config.ts proxy must include '/api' route to FastAPI backend." ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 revision — _row_to_dict latent bug: text fields decoded as JSON +# --------------------------------------------------------------------------- + +def _make_sqlite_row(conn_in_memory, **kwargs): + """Insert a row into a temp table and return it as sqlite3.Row.""" + conn_in_memory.execute( + "CREATE TABLE IF NOT EXISTS _tmp_row_test (id TEXT, title TEXT, brief TEXT, description TEXT)" + ) + conn_in_memory.execute( + "INSERT INTO _tmp_row_test VALUES (:id, :title, :brief, :description)", + { + "id": kwargs.get("id", "x"), + "title": kwargs.get("title", None), + "brief": kwargs.get("brief", None), + "description": kwargs.get("description", None), + }, + ) + row = conn_in_memory.execute( + "SELECT * FROM _tmp_row_test WHERE id = :id", {"id": kwargs.get("id", "x")} + ).fetchone() + conn_in_memory.execute("DELETE FROM _tmp_row_test WHERE id = :id", {"id": kwargs.get("id", "x")}) + return row + + +@pytest.fixture +def raw_conn(): + """Bare sqlite3 in-memory connection with row_factory (no kin schema needed).""" + c = sqlite3.connect(":memory:") + c.row_factory = sqlite3.Row + yield c + c.close() + + +class TestRowToDictJsonWhitelist: + """(15/16) _row_to_dict JSON decoding whitelist behavior. + + Latent bug: _row_to_dict decodes ALL string fields starting with '[' or '{' + through JSON, including text fields (title, description, name). + Expected fix: only decode fields in a _JSON_COLUMNS whitelist. + """ + + def test_text_title_with_empty_array_stays_string(self, raw_conn): + """_row_to_dict: title='[]' must stay '[]' (string), not be decoded to []. + + This test FAILS without the _JSON_COLUMNS whitelist fix in core/models.py. + """ + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, title="[]", brief=None) + result = _row_to_dict(row) + assert isinstance(result["title"], str), ( + f"Expected title to be str '[]', got {type(result['title'])}: {result['title']!r}. " + "Latent bug: _row_to_dict decodes text fields as JSON. " + "Fix: introduce _JSON_COLUMNS whitelist in core/models.py." + ) + assert result["title"] == "[]" + + def test_brief_empty_array_decoded_to_list(self, raw_conn): + """_row_to_dict: brief='[]' must be decoded to [] list (it is a JSON column).""" + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, title="Normal title", brief="[]") + result = _row_to_dict(row) + assert isinstance(result["brief"], list), ( + f"Expected brief to be list [], got {type(result['brief'])}: {result['brief']!r}" + ) + assert result["brief"] == [] + + def test_description_with_bracket_text_stays_string(self, raw_conn): + """_row_to_dict: description='[deprecated]' must stay string (invalid JSON, not decoded).""" + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, title="T", description="[deprecated]") + result = _row_to_dict(row) + assert isinstance(result["description"], str) + assert result["description"] == "[deprecated]" + + +class TestTaskModelEmptyArrayEdgeCases: + """(17/18/19/20) End-to-end task creation/retrieval with empty array edge cases.""" + + def test_task_with_empty_brief_list_round_trips(self, conn): + """(17) create_task(brief=[]) → get_task returns brief as [] list.""" + models.create_task(conn, "PROJ-002", "proj", "Empty brief task", brief=[]) + task = models.get_task(conn, "PROJ-002") + assert isinstance(task["brief"], list), ( + f"Expected brief to be list, got: {type(task['brief'])}" + ) + assert task["brief"] == [], ( + f"Expected brief=[], got: {task['brief']!r}" + ) + + def test_task_with_json_looking_title_preserved(self, conn): + """(18) create_task(title='[]') → get_task returns title as '[]' string, not list. + + This test FAILS without the _JSON_COLUMNS whitelist fix in core/models.py. + Without the fix, _row_to_dict converts title='[]' (str) to [] (list). + """ + models.create_task(conn, "PROJ-003", "proj", "[]") + task = models.get_task(conn, "PROJ-003") + assert isinstance(task["title"], str), ( + f"Expected task['title'] to be str '[]', got {type(task['title'])}: {task['title']!r}. " + "Latent bug in _row_to_dict: text field decoded as JSON. " + "Fix: introduce _JSON_COLUMNS whitelist in core/models.py." + ) + assert task["title"] == "[]" + + def test_task_brief_none_stays_none(self, conn): + """(19) create_task without brief → get_task returns brief=None.""" + models.create_task(conn, "PROJ-004", "proj", "No brief task") + task = models.get_task(conn, "PROJ-004") + assert task["brief"] is None, ( + f"Expected brief=None, got: {task['brief']!r}" + ) + + def test_task_with_single_element_brief_round_trips(self, conn): + """(20) create_task(brief=['developer']) → get_task returns brief=['developer'].""" + models.create_task(conn, "PROJ-005", "proj", "Single step task", brief=["developer"]) + task = models.get_task(conn, "PROJ-005") + assert isinstance(task["brief"], list), ( + f"Expected brief to be list, got: {type(task['brief'])}" + ) + assert task["brief"] == ["developer"], ( + f"Expected brief=['developer'], got: {task['brief']!r}" + ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper investigation — additional empty array edge cases +# --------------------------------------------------------------------------- + + +class TestTryParseJsonEmptyArrayCases: + """Direct unit tests for _try_parse_json with empty array edge cases. + + _try_parse_json is used by followup.py and runner.py to parse all agent output. + Its correct handling of empty arrays is critical for the empty-array bug fix. + """ + + def test_empty_array_string_returns_list(self): + """_try_parse_json('[]') → [] (empty list).""" + from agents.runner import _try_parse_json + result = _try_parse_json("[]") + assert result == [], f"Expected [], got {result!r}" + assert isinstance(result, list) + + def test_whitespace_wrapped_empty_array_returns_list(self): + """_try_parse_json(' [] ') → [] (strips whitespace then parses).""" + from agents.runner import _try_parse_json + result = _try_parse_json(" [] ") + assert result == [], f"Expected [] for whitespace-wrapped '[]', got {result!r}" + + def test_empty_string_returns_none(self): + """_try_parse_json('') → None (empty input = no result).""" + from agents.runner import _try_parse_json + result = _try_parse_json("") + assert result is None, f"Expected None for empty string, got {result!r}" + + def test_null_string_returns_none(self): + """_try_parse_json('null') → None (JSON null decodes to Python None).""" + from agents.runner import _try_parse_json + result = _try_parse_json("null") + assert result is None, f"Expected None for 'null', got {result!r}" + + def test_empty_dict_string_returns_dict(self): + """_try_parse_json('{}') → {} (empty dict).""" + from agents.runner import _try_parse_json + result = _try_parse_json("{}") + assert result == {}, f"Expected {{}}, got {result!r}" + assert isinstance(result, dict) + + def test_empty_array_in_markdown_fence_returns_list(self): + """_try_parse_json with ```json\\n[]\\n``` code fence → [].""" + from agents.runner import _try_parse_json + result = _try_parse_json("```json\n[]\n```") + assert result == [], f"Expected [] from markdown fence, got {result!r}" + + +class TestRowToDictEmptyDictAndTextFields: + """Deeper _row_to_dict tests: empty dict JSON column, text field with dict-like content.""" + + def test_brief_empty_dict_decoded(self, raw_conn): + """_row_to_dict: brief='{}' → decoded to {} (empty dict, JSON column).""" + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, brief="{}") + result = _row_to_dict(row) + assert isinstance(result["brief"], dict), ( + f"Expected brief to be dict {{}}, got {type(result['brief'])}: {result['brief']!r}" + ) + assert result["brief"] == {} + + def test_title_with_dict_content_stays_string(self, raw_conn): + """_row_to_dict: title='{\"key\": \"val\"}' must stay string (not in _JSON_COLUMNS).""" + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, title='{"key": "val"}') + result = _row_to_dict(row) + assert isinstance(result["title"], str), ( + f"Expected title to remain str, got {type(result['title'])}: {result['title']!r}. " + "title is not in _JSON_COLUMNS and must never be decoded." + ) + assert result["title"] == '{"key": "val"}' + + def test_description_with_dict_content_stays_string(self, raw_conn): + """_row_to_dict: description='{\"a\": 1}' must stay string (not in _JSON_COLUMNS).""" + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, description='{"a": 1}') + result = _row_to_dict(row) + assert isinstance(result["description"], str), ( + f"Expected description to remain str, got {type(result['description'])}" + ) + + +class TestTaskBriefEmptyDictRoundTrip: + """create_task/get_task round-trip with empty dict brief.""" + + def test_task_with_empty_dict_brief_round_trips(self, conn): + """create_task(brief={}) → get_task returns brief as {} dict.""" + models.create_task(conn, "PROJ-006", "proj", "Empty dict brief", brief={}) + task = models.get_task(conn, "PROJ-006") + assert isinstance(task["brief"], dict), ( + f"Expected brief to be dict, got: {type(task['brief'])}" + ) + assert task["brief"] == {}, f"Expected brief={{}}, got: {task['brief']!r}" + + def test_project_tech_stack_empty_list_round_trips(self, conn): + """create_project(tech_stack=[]) → get_project returns tech_stack as [] list.""" + models.create_project(conn, "proj2", "Proj2", "/tmp/proj2", tech_stack=[]) + project = models.get_project(conn, "proj2") + assert isinstance(project["tech_stack"], list), ( + f"Expected tech_stack to be list, got: {type(project['tech_stack'])}" + ) + assert project["tech_stack"] == [], ( + f"Expected tech_stack=[], got: {project['tech_stack']!r}" + ) + + def test_project_name_with_brackets_stays_string(self, conn): + """create_project(name='[Proj]') → get_project returns name as string, not list.""" + models.create_project(conn, "proj3", "[Proj]", "/tmp/proj3") + project = models.get_project(conn, "proj3") + assert isinstance(project["name"], str), ( + f"Expected name to be str '[Proj]', got {type(project['name'])}: {project['name']!r}. " + "name is a text field and must never be JSON-decoded." + ) + assert project["name"] == "[Proj]" + + +class TestGenerateFollowupsEmptyFollowupsKey: + """generate_followups deeper edge cases: empty followups key, items without title.""" + + @patch("agents.runner._run_claude") + def test_dict_with_empty_followups_key_creates_no_tasks(self, mock_claude, conn): + """generate_followups: {"followups": []} → no tasks, empty result.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"followups": []}', "returncode": 0} + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected created=[], got: {result['created']}" + ) + assert tasks_after == tasks_before, ( + f"Task count changed from {tasks_before} to {tasks_after}" + ) + + @patch("agents.runner._run_claude") + def test_list_with_items_missing_title_creates_no_tasks(self, mock_claude, conn): + """generate_followups: [{"type": "backend_dev"}] (no title) → items skipped, no tasks.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"type": "backend_dev", "brief": "Do something"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected created=[] for items missing 'title', got: {result['created']}. " + "Items without 'title' key must be skipped." + ) + assert tasks_after == tasks_before + + @patch("agents.runner._run_claude") + def test_list_with_mixed_valid_and_titleless_items(self, mock_claude, conn): + """generate_followups: one valid item + one without title → only valid one creates task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": "Valid task", "type": "backend_dev"}, {"type": "frontend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert len(result["created"]) == 1, ( + f"Expected 1 task created (item with title), got: {len(result['created'])}" + ) + assert tasks_after == tasks_before + 1 + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper investigation — null-valued dict keys +# +# Bug: followup.py extracts `parsed["tasks"]` or `parsed["followups"]` from dict, +# but does NOT check if the extracted value is None/non-iterable. +# `for item in None:` → TypeError: 'NoneType' object is not iterable +# --------------------------------------------------------------------------- + + +class TestGenerateFollowupsNullValuedDictKeys: + """generate_followups: dict with null tasks/followups values — must not crash. + + Bug: when agent returns {"tasks": null} or {"followups": null}, the code does: + parsed = parsed["tasks"] # parsed = None + for item in parsed: # TypeError: 'NoneType' is not iterable + + Fix: guard against None after extraction, e.g. `parsed = parsed["tasks"] or []` + """ + + @patch("agents.runner._run_claude") + def test_dict_with_null_tasks_key_returns_empty_no_crash(self, mock_claude, conn): + """generate_followups: {"tasks": null} → {created: [], pending_actions: []}, no TypeError.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": null}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], ( + f"Expected created=[], got: {result['created']}. " + "Bug: parsed['tasks']=None causes TypeError in for-loop." + ) + assert result["pending_actions"] == [] + + @patch("agents.runner._run_claude") + def test_dict_with_null_followups_key_returns_empty_no_crash(self, mock_claude, conn): + """generate_followups: {"followups": null} → empty result, no TypeError.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"followups": null}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], ( + f"Expected created=[], got: {result['created']}. " + "Bug: parsed['followups']=None causes TypeError in for-loop." + ) + assert result["pending_actions"] == [] + + @patch("agents.runner._run_claude") + def test_dict_with_null_tasks_and_nonempty_followups_no_crash(self, mock_claude, conn): + """generate_followups: {"tasks": null, "followups": [...]} → takes tasks path, returns empty, no crash. + + tasks key is present (even as null), so followups fallback must NOT be reached. + But extracted None must be handled gracefully — no tasks created, no TypeError. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": null, "followups": [{"title": "Should be ignored"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + "tasks key is present (even as null) — tasks path taken, followups must be ignored. " + "No tasks must be created." + ) + assert tasks_after == tasks_before + + @patch("agents.runner._run_claude") + def test_dict_null_tasks_creates_no_tasks_in_db(self, mock_claude, conn): + """generate_followups: {"tasks": null} → no tasks created in DB.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": null}', "returncode": 0} + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert tasks_after == tasks_before, ( + f"Task count changed from {tasks_before} to {tasks_after}. " + "No tasks must be created when tasks=null." + ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — additional empty array edge cases +# --------------------------------------------------------------------------- + + +class TestGenerateFollowupsTasksKeyPriority: + """tasks key present and non-empty: followups key must be completely ignored.""" + + @patch("agents.runner._run_claude") + def test_nonempty_tasks_ignores_followups_key(self, mock_claude, conn): + """{"tasks": [{"title": "T1"}], "followups": [{"title": "F1"}]} → only T1 created. + + Verifies that when tasks key is present AND non-empty, the followups key + is not consulted at all. Result must contain exactly 1 task (T1), not 2. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [{"title": "T1", "type": "backend_dev"}], "followups": [{"title": "F1"}]}', + "returncode": 0, + } + result = generate_followups(conn, "PROJ-001") + assert len(result["created"]) == 1, ( + f"Expected 1 task (from tasks key), got {len(result['created'])}. " + "followups key must be ignored when tasks key is present and non-empty." + ) + assert result["created"][0]["title"] == "T1" + + @patch("agents.runner._run_claude") + def test_nonempty_tasks_does_not_create_followups_tasks(self, mock_claude, conn): + """Verify only the task from tasks[] is created in DB, not the one from followups[].""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [{"title": "Real task"}], "followups": [{"title": "Should not exist"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert tasks_after == tasks_before + 1, ( + f"Expected exactly 1 new task, got {tasks_after - tasks_before}. " + "followups items must not be created when tasks key present." + ) + + +class TestGenerateFollowupsFalsyNonListTasksValue: + """generate_followups: tasks key maps to falsy non-null, non-list values — must not crash.""" + + @patch("agents.runner._run_claude") + def test_tasks_is_integer_zero_returns_empty(self, mock_claude, conn): + """{"tasks": 0} → empty result, no crash. Guard `if not isinstance(parsed, list)` fires.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": 0}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], f"Expected created=[], got: {result['created']}" + assert result["pending_actions"] == [] + + @patch("agents.runner._run_claude") + def test_tasks_is_false_returns_empty(self, mock_claude, conn): + """{"tasks": false} → empty result, no crash.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": false}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], f"Expected created=[], got: {result['created']}" + assert result["pending_actions"] == [] + + @patch("agents.runner._run_claude") + def test_tasks_is_empty_string_returns_empty(self, mock_claude, conn): + """{"tasks": ""} → empty result, no crash.""" + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": ""}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], f"Expected created=[], got: {result['created']}" + assert result["pending_actions"] == [] + + +class TestGenerateFollowupsMixedListItems: + """generate_followups: list with non-dict and non-title elements mixed with valid items.""" + + @patch("agents.runner._run_claude") + def test_list_with_null_and_valid_item_creates_only_valid(self, mock_claude, conn): + """[null, {"title": "Valid task"}] → only the valid dict item creates a task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[null, {"title": "Valid task", "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert len(result["created"]) == 1, ( + f"Expected 1 task from valid dict item, got {len(result['created'])}" + ) + assert tasks_after == tasks_before + 1 + + @patch("agents.runner._run_claude") + def test_list_with_integers_strings_and_valid_item(self, mock_claude, conn): + """[42, "string", {"title": "T1"}] → only the valid dict item creates a task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[42, "string", {"title": "T1"}]', + "returncode": 0, + } + result = generate_followups(conn, "PROJ-001") + assert len(result["created"]) == 1, ( + f"Expected 1 task (only dict with title), got {len(result['created'])}" + ) + assert result["created"][0]["title"] == "T1" + + @patch("agents.runner._run_claude") + def test_list_with_all_non_dict_items_creates_no_tasks(self, mock_claude, conn): + """[null, 1, "string", true] → no tasks created (no dict items with title).""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[null, 1, "string", true]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected no tasks created, got: {result['created']}" + ) + assert tasks_after == tasks_before + + +class TestTaskSpecEmptyArrayRoundTrip: + """spec field (JSON column) round-trip with empty array.""" + + def test_task_spec_empty_list_round_trips(self, conn): + """create_task(spec=[]) → get_task returns spec as [] list.""" + models.create_task(conn, "PROJ-007", "proj", "Spec test", spec=[]) + task = models.get_task(conn, "PROJ-007") + assert isinstance(task["spec"], list), ( + f"Expected spec to be list, got: {type(task['spec'])}" + ) + assert task["spec"] == [], f"Expected spec=[], got: {task['spec']!r}" + + def test_task_spec_nonempty_list_round_trips(self, conn): + """create_task(spec=["step1"]) → get_task returns spec=['step1'] list.""" + models.create_task(conn, "PROJ-008", "proj", "Spec test 2", spec=["step1"]) + task = models.get_task(conn, "PROJ-008") + assert isinstance(task["spec"], list) + assert task["spec"] == ["step1"] + + +# --------------------------------------------------------------------------- +# KIN-P1-001 regression — followups key positive path (no tasks key) +# +# Bug: the old `or` chain `parsed.get('tasks') or parsed.get('followups') or []` +# made tasks=[] fall through to followups, but the followups path itself must +# still work when only the followups key is present. +# Fix: explicit key-presence check with `in` — elif branch handles this case. +# --------------------------------------------------------------------------- + + +class TestGenerateFollowupsFollowupsPathPositive: + """Followups fallback path works after the `or`→`in` fix. + + When agent returns {"followups": [...]} without a tasks key, items must be + processed via the elif branch. This confirms the fix did not break the + followups fallback. + """ + + @patch("agents.runner._run_claude") + def test_followups_key_only_with_valid_item_creates_task(self, mock_claude, conn): + """{"followups": [{"title": "X"}]} (no tasks key) → task created via followups path.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"followups": [{"title": "Followup task", "type": "backend_dev"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert len(result["created"]) == 1, ( + f"Expected 1 task from followups key, got {len(result['created'])}. " + "The elif 'followups' branch must be reachable when tasks key is absent." + ) + assert result["created"][0]["title"] == "Followup task" + assert tasks_after == tasks_before + 1 + + @patch("agents.runner._run_claude") + def test_dict_with_unknown_key_only_creates_no_tasks(self, mock_claude, conn): + """{"steps": [{"title": "X"}]} (neither tasks nor followups key) → no tasks, else branch.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"steps": [{"title": "Should be ignored"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected no tasks when dict has neither tasks nor followups key, got: {result['created']}" + ) + assert tasks_after == tasks_before + + @patch("agents.runner._run_claude") + def test_tasks_is_truthy_nonlist_string_no_crash(self, mock_claude, conn): + """{"tasks": "pending_review"} (truthy non-list string) → empty result, no crash. + + Regression for the `if not isinstance(parsed, list): parsed = []` guard. + A truthy string like "pending_review" passes `or` truthiness but is not iterable + as a task list. The isinstance guard must convert it to []. + """ + from core.followup import generate_followups + mock_claude.return_value = {"output": '{"tasks": "pending_review"}', "returncode": 0} + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], ( + f"Expected created=[] for tasks='pending_review', got: {result['created']}. " + "Non-list tasks value must be replaced with [] by isinstance guard." + ) + assert result["pending_actions"] == [] + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — update_task with empty list fields +# --------------------------------------------------------------------------- + + +class TestUpdateTaskEmptyListFields: + """update_task correctly handles empty list fields after the _JSON_COLUMNS whitelist fix. + + Previous tests only covered create_task round-trips. This class tests that + update_task also correctly encodes/decodes empty lists for JSON columns. + """ + + def test_update_task_brief_to_empty_list_round_trips(self, conn): + """update_task(brief=[]) on existing task → get_task returns brief as [] list.""" + models.update_task(conn, "PROJ-001", brief=[]) + task = models.get_task(conn, "PROJ-001") + assert isinstance(task["brief"], list), ( + f"Expected brief=[] after update, got {type(task['brief'])}: {task['brief']!r}" + ) + assert task["brief"] == [] + + def test_update_task_labels_to_empty_list_round_trips(self, conn): + """update_task(labels=[]) on existing task → get_task returns labels as [] list.""" + models.update_task(conn, "PROJ-001", labels=[]) + task = models.get_task(conn, "PROJ-001") + assert isinstance(task["labels"], list), ( + f"Expected labels=[] after update, got {type(task['labels'])}: {task['labels']!r}" + ) + assert task["labels"] == [] + + def test_update_task_brief_from_dict_to_empty_list(self, conn): + """update_task: overwriting dict brief with [] → retrieved as [].""" + task_before = models.get_task(conn, "PROJ-001") + assert isinstance(task_before["brief"], dict), "Precondition: brief must be a dict" + models.update_task(conn, "PROJ-001", brief=[]) + task_after = models.get_task(conn, "PROJ-001") + assert task_after["brief"] == [], ( + f"Expected brief=[] after overwrite from dict, got: {task_after['brief']!r}" + ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — _row_to_dict: None input and "null" string +# --------------------------------------------------------------------------- + + +class TestRowToDictNoneAndNullString: + """_row_to_dict(None) → None; brief='null' stays string (not decoded via JSON).""" + + def test_row_to_dict_none_returns_none(self): + """_row_to_dict(None) must return None without raising.""" + from core.models import _row_to_dict + result = _row_to_dict(None) + assert result is None, f"Expected None for None input, got: {result!r}" + + def test_brief_null_string_stays_string(self, raw_conn): + """_row_to_dict: brief='null' stays as string — 'null' doesn't start with '[' or '{'. + + Contrast with _try_parse_json('null') → None. + _row_to_dict relies on startswith('[', '{') guard, so 'null' is NOT decoded. + """ + from core.models import _row_to_dict + row = _make_sqlite_row(raw_conn, brief="null") + result = _row_to_dict(row) + assert isinstance(result["brief"], str), ( + f"Expected brief to remain str 'null', got {type(result['brief'])}: {result['brief']!r}. " + "'null' does not start with '[' or '{{', so _row_to_dict must leave it as a string." + ) + assert result["brief"] == "null" + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — decisions.tags empty list round-trip +# --------------------------------------------------------------------------- + + +class TestDecisionsTagsEmptyList: + """add_decision with tags=[] — tags stored as JSON and retrieved as empty list.""" + + def test_add_decision_tags_empty_list_round_trips(self, conn): + """add_decision(tags=[]) → retrieved decision has tags=[] (list, not None).""" + dec = models.add_decision( + conn, "proj", "gotcha", "Empty tags decision", "Some description", tags=[] + ) + assert isinstance(dec["tags"], list), ( + f"Expected tags=[] after add_decision, got {type(dec['tags'])}: {dec['tags']!r}" + ) + assert dec["tags"] == [] + + def test_add_decision_tags_none_stays_none(self, conn): + """add_decision(tags=None) → decision.tags is None (not encoded as JSON).""" + dec = models.add_decision( + conn, "proj", "decision", "No tags decision", "description", tags=None + ) + assert dec["tags"] is None, f"Expected tags=None for tags=None input, got: {dec['tags']!r}" + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — _try_parse_json: whitespace-only and embedded [] +# --------------------------------------------------------------------------- + + +class TestTryParseJsonWhitespaceAndEmbedded: + """_try_parse_json edge cases: whitespace-only input; [] embedded in prose text.""" + + def test_newline_only_returns_none(self): + """_try_parse_json('\\n') → None (strips to empty string).""" + from agents.runner import _try_parse_json + result = _try_parse_json("\n") + assert result is None, f"Expected None for newline-only input, got: {result!r}" + + def test_whitespace_and_newlines_returns_none(self): + """_try_parse_json('\\t \\n ') → None (strips to empty string).""" + from agents.runner import _try_parse_json + result = _try_parse_json("\t \n ") + assert result is None, f"Expected None for whitespace-only input, got: {result!r}" + + def test_empty_array_embedded_in_prose_extracted(self): + """_try_parse_json with '[]' embedded in prose → extracts and returns []. + + The bracket-scanning fallback in _try_parse_json finds the first '[...]' + block even when surrounded by text. This is the same code path that + extracts JSON from agent outputs that contain prose before the JSON. + """ + from agents.runner import _try_parse_json + result = _try_parse_json("The result is: [] as expected") + assert result == [], ( + f"Expected [] extracted from prose, got: {result!r}. " + "The bracket-scanning fallback must find the first '[...]' block." + ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — null-title item guard +# +# Bug: followup.py filters items with `"title" not in item`, but does NOT check +# if item["title"] is None (JSON null). When agent returns {"title": null}, the +# key IS present → filter passes → create_task(title=None) → IntegrityError +# (title is NOT NULL in DB schema). +# +# Deeper edge: {"title": ""} (empty string) — key present, string is falsy +# but valid. Currently creates a task with empty title in DB. +# --------------------------------------------------------------------------- + + +class TestGenerateFollowupsNullAndEmptyTitle: + """Items with title=null or title='' — should be skipped or handled gracefully. + + title TEXT NOT NULL in DB schema, so title=None would cause an IntegrityError. + Items with null title must be skipped (same as items with no title key). + """ + + @patch("agents.runner._run_claude") + def test_item_with_null_title_does_not_crash(self, mock_claude, conn): + """[{"title": null}] — must NOT raise IntegrityError or any exception. + + DB schema: title TEXT NOT NULL. Inserting None crashes unless guarded. + Filter `"title" not in item` does NOT catch this — key exists, value is null. + Fix required: also check `if not item.get("title"):` or `if not item["title"]:`. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": null, "type": "backend_dev"}]', + "returncode": 0, + } + # Must not raise — either skips item or handles gracefully + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], ( + f"Expected created=[] for item with null title, got: {result['created']}. " + "Items with null title must be skipped (title NOT NULL in DB schema)." + ) + + @patch("agents.runner._run_claude") + def test_item_with_null_title_creates_no_tasks_in_db(self, mock_claude, conn): + """[{"title": null}] — no tasks must be created in DB.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": null, "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert tasks_after == tasks_before, ( + f"Task count changed from {tasks_before} to {tasks_after}. " + "Items with null title must be skipped." + ) + + @patch("agents.runner._run_claude") + def test_mixed_null_title_and_valid_item(self, mock_claude, conn): + """[{"title": null}, {"title": "Valid"}] — null skipped, valid item creates task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": null}, {"title": "Valid task", "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + # Null-title item must be skipped; valid item must create 1 task + assert len(result["created"]) == 1, ( + f"Expected 1 task (valid item only), got {len(result['created'])}. " + "Null-title item must be skipped without affecting subsequent items." + ) + assert tasks_after == tasks_before + 1 + + @patch("agents.runner._run_claude") + def test_item_with_empty_string_title_does_not_crash(self, mock_claude, conn): + """[{"title": ""}] — empty string title must be skipped, not create a task. + + Fix: `not item.get("title")` guards against both title=null and title="". + Empty string is invalid as a task title (title NOT NULL in DB schema requires + a meaningful value; an empty string would silently create a broken task). + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": "", "type": "backend_dev"}]', + "returncode": 0, + } + result = generate_followups(conn, "PROJ-001") + assert result["created"] == [], ( + f"Expected created=[] for item with empty string title, got: {result['created']}. " + "Items with title='' must be skipped (not item.get('title') guard)." + ) + + @patch("agents.runner._run_claude") + def test_item_with_empty_string_title_creates_no_tasks_in_db(self, mock_claude, conn): + """[{"title": ""}] — no tasks must be created in DB.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": "", "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert tasks_after == tasks_before, ( + f"Task count changed from {tasks_before} to {tasks_after}. " + "Items with empty string title must be skipped." + ) + + @patch("agents.runner._run_claude") + def test_mixed_empty_string_title_and_valid_item(self, mock_claude, conn): + """[{"title": ""}, {"title": "Valid"}] — empty-string skipped, valid creates task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[{"title": ""}, {"title": "Valid task", "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert len(result["created"]) == 1, ( + f"Expected 1 task (valid item only), got {len(result['created'])}. " + "Empty-string-title item must be skipped without blocking subsequent items." + ) + assert tasks_after == tasks_before + 1 + + +class TestTryParseJsonInternalWhitespace: + """_try_parse_json with arrays containing internal whitespace.""" + + def test_array_with_internal_spaces_returns_empty_list(self): + """_try_parse_json('[ ]') — array with space inside → [] (valid JSON).""" + from agents.runner import _try_parse_json + result = _try_parse_json("[ ]") + assert result == [], f"Expected [] for '[ ]', got: {result!r}" + assert isinstance(result, list) + + def test_array_with_newline_inside_returns_empty_list(self): + """_try_parse_json('[\\n]') — array with newline inside → [].""" + from agents.runner import _try_parse_json + result = _try_parse_json("[\n]") + assert result == [], f"Expected [] for '[\\n]', got: {result!r}" + + def test_nested_empty_array_extracted_from_tasks(self): + """_try_parse_json('{"tasks": [[]]}') → dict with tasks=[[]]. + + tasks is a list containing one empty list. Not a valid task item (non-dict), + so generate_followups will skip it. Verifies _try_parse_json handles it. + """ + from agents.runner import _try_parse_json + result = _try_parse_json('{"tasks": [[]]}') + assert isinstance(result, dict), f"Expected dict, got {type(result)}: {result!r}" + assert result == {"tasks": [[]]}, f"Got: {result!r}" + + +class TestGenerateFollowupsNestedEmptyList: + """generate_followups: items list contains nested empty lists — must skip them.""" + + @patch("agents.runner._run_claude") + def test_nested_empty_list_skipped_valid_item_creates_task(self, mock_claude, conn): + """[[], {"title": "T1"}] — inner [] is non-dict, skipped; T1 creates task.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '[[], {"title": "T1", "type": "backend_dev"}]', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert len(result["created"]) == 1, ( + f"Expected 1 task (T1 only), inner [] must be skipped, got: {len(result['created'])}" + ) + assert tasks_after == tasks_before + 1 + + @patch("agents.runner._run_claude") + def test_tasks_nested_empty_list_via_tasks_key(self, mock_claude, conn): + """{"tasks": [[]]} — tasks contains a list, not a dict — all items skipped.""" + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [[]]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected no tasks from nested [[]], got: {result['created']}" + ) + assert tasks_after == tasks_before + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — _JSON_COLUMNS whitelist completeness +# --------------------------------------------------------------------------- + + +class TestJsonColumnsWhitelist: + """Regression guard: _JSON_COLUMNS whitelist must include/exclude correct columns. + + This test locks down the whitelist so that future accidental additions or + removals are caught immediately. Both the inclusion of JSON fields and the + exclusion of text fields are verified. + """ + + def test_json_columns_includes_required_fields(self): + """_JSON_COLUMNS must contain all columns that store JSON arrays/dicts.""" + from core.models import _JSON_COLUMNS + required = { + "tech_stack", "brief", "spec", "review", "test_result", + "security_result", "labels", "tags", "dependencies", + "steps", "artifacts", "decisions_made", "blockers", + "extra_json", "pending_actions", + } + missing = required - _JSON_COLUMNS + assert not missing, ( + f"_JSON_COLUMNS is missing expected JSON columns: {missing}. " + "These columns store JSON and must be decoded by _row_to_dict." + ) + + def test_json_columns_excludes_text_fields(self): + """_JSON_COLUMNS must NOT contain text fields that should never be JSON-decoded.""" + from core.models import _JSON_COLUMNS + text_fields = {"title", "description", "name", "path", "status", + "acceptance_criteria", "assigned_role", "id", "project_id"} + wrongly_included = text_fields & _JSON_COLUMNS + assert not wrongly_included, ( + f"Text fields found in _JSON_COLUMNS: {wrongly_included}. " + "These fields are plain text and must never be JSON-decoded." + ) + + +class TestAcceptanceCriteriaTextFieldNotDecoded: + """acceptance_criteria is a text field — must never be JSON-decoded by _row_to_dict.""" + + def test_acceptance_criteria_with_brackets_stays_string(self, conn): + """create_task(acceptance_criteria='[]') → field stays as '[]' string, not list.""" + models.create_task( + conn, "PROJ-009", "proj", "AC test", + acceptance_criteria="[]", + ) + task = models.get_task(conn, "PROJ-009") + assert isinstance(task["acceptance_criteria"], str), ( + f"Expected acceptance_criteria to be str '[]', got " + f"{type(task['acceptance_criteria'])}: {task['acceptance_criteria']!r}. " + "acceptance_criteria is not in _JSON_COLUMNS and must stay as string." + ) + assert task["acceptance_criteria"] == "[]" + + def test_acceptance_criteria_with_json_dict_stays_string(self, conn): + """create_task(acceptance_criteria='{\"k\":1}') → stays as JSON-encoded string.""" + models.create_task( + conn, "PROJ-010", "proj", "AC dict test", + acceptance_criteria='{"must": "pass"}', + ) + task = models.get_task(conn, "PROJ-010") + assert isinstance(task["acceptance_criteria"], str), ( + f"Expected acceptance_criteria to remain str, got " + f"{type(task['acceptance_criteria'])}: {task['acceptance_criteria']!r}" + ) + + +class TestGenerateFollowupsWithEmptyBriefTask: + """generate_followups called on a task that has brief=[] (empty list). + + Verifies that the context-building step in generate_followups handles + brief=[] without crashing — the empty list must serialize cleanly to the + prompt context and Claude must still be invoked. + """ + + @patch("agents.runner._run_claude") + def test_task_with_empty_brief_list_does_not_crash(self, mock_claude, conn): + """generate_followups on task with brief=[] must not raise any exception.""" + from core.followup import generate_followups + models.create_task(conn, "PROJ-011", "proj", "Empty brief followup test", brief=[]) + mock_claude.return_value = {"output": "[]", "returncode": 0} + result = generate_followups(conn, "PROJ-011") + assert result["created"] == [], ( + f"Expected created=[], got: {result['created']}" + ) + assert result["pending_actions"] == [] + mock_claude.assert_called_once() + + @patch("agents.runner._run_claude") + def test_task_with_empty_brief_list_claude_called(self, mock_claude, conn): + """generate_followups on task with brief=[] must still call Claude (no early bail).""" + from core.followup import generate_followups + models.create_task(conn, "PROJ-012", "proj", "Claude called test", brief=[]) + mock_claude.return_value = {"output": "[]", "returncode": 0} + generate_followups(conn, "PROJ-012") + mock_claude.assert_called_once() + + +# --------------------------------------------------------------------------- +# KIN-P1-001 fix 3 — _detect_test_command: role='backend_dev' bypasses Makefile +# +# Bug: in mixed (frontend + backend) projects, make test runs both frontend +# (vitest) and backend (pytest) suites. A backend_dev agent change should only +# validate Python tests, not fail on unrelated vitest failures. +# +# Fix: when role='backend_dev' and a Python marker (pyproject.toml / setup.py) +# is present, _detect_test_command returns pytest directly — bypassing Makefile. +# --------------------------------------------------------------------------- + + +class TestDetectTestCommandRoleBackendDev: + """_detect_test_command with role='backend_dev' — pytest bypasses Makefile in mixed projects.""" + + def test_backend_dev_with_pyproject_bypasses_makefile(self, tmp_path): + """role='backend_dev' + pyproject.toml + Makefile → pytest returned, not make test. + + Without the fix, Makefile has priority and make test is returned — which + runs frontend tests that are unrelated to backend changes. + """ + import sys + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[tool.pytest.ini_options]\n") + (tmp_path / "Makefile").write_text("test:\n\tpytest && vitest run\n") + result = _detect_test_command(str(tmp_path), role="backend_dev") + assert result == f"{sys.executable} -m pytest", ( + f"Expected pytest for backend_dev+pyproject.toml, got: {result!r}. " + "backend_dev role must bypass Makefile and return pytest directly." + ) + + def test_backend_dev_with_setup_py_bypasses_makefile(self, tmp_path): + """role='backend_dev' + setup.py + Makefile → pytest returned, not make test.""" + import sys + from agents.runner import _detect_test_command + (tmp_path / "setup.py").write_text("from setuptools import setup\nsetup(name='x')\n") + (tmp_path / "Makefile").write_text("test:\n\tpytest && npm test\n") + result = _detect_test_command(str(tmp_path), role="backend_dev") + assert result == f"{sys.executable} -m pytest", ( + f"Expected pytest for backend_dev+setup.py, got: {result!r}. " + "setup.py is a Python project marker — backend_dev must bypass Makefile." + ) + + def test_backend_dev_without_python_marker_still_uses_makefile(self, tmp_path): + """role='backend_dev' + only Makefile (no pyproject/setup.py) → make test returned. + + If no Python project marker is present, the backend_dev shortcut does NOT + apply — normal priority order is used (Makefile first). + """ + from agents.runner import _detect_test_command + (tmp_path / "Makefile").write_text("test:\n\tmake check\n") + result = _detect_test_command(str(tmp_path), role="backend_dev") + assert result == "make test", ( + f"Expected 'make test' when no Python marker present, got: {result!r}. " + "Without pyproject.toml or setup.py, backend_dev must use normal priority." + ) + + def test_role_none_pyproject_and_makefile_uses_makefile(self, tmp_path): + """role=None + pyproject.toml + Makefile → make test (normal priority order). + + The backend_dev bypass is role-specific. With role=None, Makefile takes + priority over pyproject.toml as before. + """ + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[tool.pytest.ini_options]\n") + (tmp_path / "Makefile").write_text("test:\n\tpytest\n") + result = _detect_test_command(str(tmp_path), role=None) + assert result == "make test", ( + f"Expected 'make test' for role=None, got: {result!r}. " + "Makefile must take priority over pyproject.toml when role is not backend_dev." + ) + + def test_role_frontend_dev_pyproject_and_makefile_uses_makefile(self, tmp_path): + """role='frontend_dev' + pyproject.toml + Makefile → make test (bypass not triggered).""" + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[tool.pytest.ini_options]\n") + (tmp_path / "Makefile").write_text("test:\n\tvitest run\n") + result = _detect_test_command(str(tmp_path), role="frontend_dev") + assert result == "make test", ( + f"Expected 'make test' for frontend_dev role, got: {result!r}. " + "Only backend_dev role triggers the Python-marker bypass." + ) + + def test_backend_dev_pyproject_only_no_makefile_returns_pytest(self, tmp_path): + """role='backend_dev' + only pyproject.toml (no Makefile) → pytest returned.""" + import sys + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[tool.pytest.ini_options]\n") + result = _detect_test_command(str(tmp_path), role="backend_dev") + assert result == f"{sys.executable} -m pytest", ( + f"Expected pytest for backend_dev+pyproject.toml (no Makefile), got: {result!r}" + ) + + def test_backend_dev_no_files_returns_none(self, tmp_path): + """role='backend_dev' + empty directory → None (no framework detected).""" + from agents.runner import _detect_test_command + result = _detect_test_command(str(tmp_path), role="backend_dev") + assert result is None, ( + f"Expected None for backend_dev with no project files, got: {result!r}" + ) + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — empty array edge cases: both keys empty/null +# --------------------------------------------------------------------------- + + +class TestGenerateFollowupsBothKeysEmptyOrNull: + """Empty array edge cases when BOTH tasks and followups keys are empty or null. + + These complement test_dict_tasks_empty_followups_nonempty_does_not_create_tasks + by covering scenarios where the followups side is also empty/null. + """ + + @patch("agents.runner._run_claude") + def test_tasks_empty_and_followups_empty_creates_no_tasks(self, mock_claude, conn): + """{"tasks": [], "followups": []} — both empty → no tasks created. + + tasks key is present (even as empty list) → followups key ignored entirely. + Both branches produce empty results, so no tasks should be created. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [], "followups": []}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected created=[] when tasks=[] and followups=[], got: {result['created']}" + ) + assert tasks_after == tasks_before + + @patch("agents.runner._run_claude") + def test_tasks_empty_and_followups_null_creates_no_tasks(self, mock_claude, conn): + """{"tasks": [], "followups": null} — tasks empty, followups null → no tasks, no crash. + + tasks key present and empty → followups key ignored. + The null followups value is never accessed so no crash should occur. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [], "followups": null}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected created=[] when tasks=[], followups=null, got: {result['created']}" + ) + assert result["pending_actions"] == [] + assert tasks_after == tasks_before + + @patch("agents.runner._run_claude") + def test_tasks_empty_followups_nonempty_list_ignores_all_followups(self, mock_claude, conn): + """{"tasks": [], "followups": [X, Y, Z]} — tasks path taken, all 3 followups ignored. + + Three items in followups key, but none must create tasks because tasks key + (even empty) takes priority and is iterated as an empty list. + """ + from core.followup import generate_followups + mock_claude.return_value = { + "output": '{"tasks": [], "followups": [{"title": "F1"}, {"title": "F2"}, {"title": "F3"}]}', + "returncode": 0, + } + tasks_before = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + result = generate_followups(conn, "PROJ-001") + tasks_after = conn.execute("SELECT COUNT(*) FROM tasks").fetchone()[0] + assert result["created"] == [], ( + f"Expected no tasks (3 followups all ignored due to empty tasks key), " + f"got: {result['created']}. " + "When tasks=[], the followups key must be completely ignored regardless of its contents." + ) + assert tasks_after == tasks_before + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — create_task with labels=[] (JSON column) +# --------------------------------------------------------------------------- + + +class TestCreateTaskLabelsEmptyList: + """create_task with labels=[] — labels is a JSON column, must round-trip as [].""" + + def test_create_task_labels_empty_list_round_trips(self, conn): + """create_task(labels=[]) → get_task returns labels as [] list (not None). + + Existing tests cover update_task(labels=[]). This tests the create path. + """ + models.create_task(conn, "PROJ-013", "proj", "Labels test", labels=[]) + task = models.get_task(conn, "PROJ-013") + assert isinstance(task["labels"], list), ( + f"Expected labels to be list [], got {type(task['labels'])}: {task['labels']!r}. " + "labels is in _JSON_COLUMNS and must be decoded from '[]' to []." + ) + assert task["labels"] == [], f"Expected labels=[], got: {task['labels']!r}" + + def test_create_task_labels_nonempty_list_round_trips(self, conn): + """create_task(labels=['bug', 'urgent']) → get_task returns correct list.""" + models.create_task(conn, "PROJ-014", "proj", "Labels nonempty test", labels=["bug", "urgent"]) + task = models.get_task(conn, "PROJ-014") + assert isinstance(task["labels"], list) + assert task["labels"] == ["bug", "urgent"] + + def test_create_task_labels_none_stays_none(self, conn): + """create_task without labels → get_task returns labels=None.""" + models.create_task(conn, "PROJ-015", "proj", "No labels task") + task = models.get_task(conn, "PROJ-015") + assert task["labels"] is None, f"Expected labels=None, got: {task['labels']!r}" + + +# --------------------------------------------------------------------------- +# KIN-P1-001 deeper revision — multiple JSON columns with empty arrays in one row +# --------------------------------------------------------------------------- + + +class TestMultipleJsonColumnsEmptyArraySingleRow: + """Stress test: task with multiple JSON columns all set to empty arrays. + + Verifies that _row_to_dict correctly decodes all JSON columns simultaneously + without any cross-contamination or decoding errors. + """ + + def test_task_with_multiple_empty_array_json_columns(self, conn): + """create_task with brief=[], spec=[], labels=[] → all three fields round-trip as []. + + Tests that _row_to_dict handles multiple JSON columns with empty arrays + in a single row without any field being decoded incorrectly. + """ + models.create_task( + conn, "PROJ-016", "proj", "Multi empty arrays", + brief=[], spec=[], labels=[], + ) + task = models.get_task(conn, "PROJ-016") + assert task["brief"] == [], f"Expected brief=[], got: {task['brief']!r}" + assert task["spec"] == [], f"Expected spec=[], got: {task['spec']!r}" + assert task["labels"] == [], f"Expected labels=[], got: {task['labels']!r}" + assert isinstance(task["brief"], list) + assert isinstance(task["spec"], list) + assert isinstance(task["labels"], list) + + def test_task_title_stays_string_when_other_columns_are_empty_arrays(self, conn): + """title stays string even when brief=[], spec=[], labels=[] in same row. + + Regression guard: ensuring that the whitelist fix doesn't affect title + decoding when many JSON columns in the same row have empty arrays. + """ + models.create_task( + conn, "PROJ-017", "proj", "Normal title", + brief=[], spec=[], labels=[], + ) + task = models.get_task(conn, "PROJ-017") + assert isinstance(task["title"], str), ( + f"Expected title to be str, got {type(task['title'])}: {task['title']!r}" + ) + assert task["title"] == "Normal title" diff --git a/tests/test_kin_124_regression.py b/tests/test_kin_124_regression.py new file mode 100644 index 0000000..67f7afb --- /dev/null +++ b/tests/test_kin_124_regression.py @@ -0,0 +1,245 @@ +"""Regression tests for KIN-124 — auto-test ложно определяет failure. + +Root cause: make test запускал vitest после pytest, vitest падал на всех 16 +тест-файлах с useI18n() без плагина i18n → make test возвращал exit code != 0 +→ auto-test считал весь прогон failed. + +Исправления: +1. web/frontend/vite.config.ts: добавлен setupFiles с vitest-setup.ts +2. web/frontend/src/__tests__/vitest-setup.ts: глобальный i18n plugin для mount() +3. _detect_test_command(role='backend_dev'): возвращает pytest напрямую (не make test) + — это предотвращает запуск vitest при backend_dev auto-test + +Coverage: +(1) _run_project_tests: exit code 0 + "1533 passed" → success=True (главный регрессион) +(2) _run_project_tests: exit code 1 + "2 failed" → success=False +(3) _run_project_tests: exit code 0 + output содержит "failed" в середине → success=True + (success определяется ТОЛЬКО по returncode, не по строке вывода) +(4) _detect_test_command: backend_dev + pyproject.toml → возвращает pytest, не make test +(5) _detect_test_command: backend_dev + pyproject.toml + Makefile → всё равно pytest +(6) _detect_test_command: frontend_dev + Makefile с test: → возвращает make test +(7) _detect_test_command: frontend_dev + pyproject.toml (без Makefile) → возвращает pytest +(8) _run_project_tests: timeout → success=False, returncode=124 +(9) _run_project_tests: команда не найдена → success=False, returncode=127 +(10) vite.config.ts содержит setupFiles с vitest-setup.ts +(11) vitest-setup.ts устанавливает i18n plugin глобально +""" + +import subprocess +import sys +import os +from pathlib import Path +from unittest.mock import patch, MagicMock + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_subprocess_result(returncode: int, stdout: str = "", stderr: str = "") -> MagicMock: + """Build a MagicMock simulating subprocess.CompletedProcess.""" + r = MagicMock() + r.returncode = returncode + r.stdout = stdout + r.stderr = stderr + return r + + +# --------------------------------------------------------------------------- +# (1-3) _run_project_tests: success determined solely by returncode +# --------------------------------------------------------------------------- + +class TestRunProjectTestsSuccessDetermination: + """_run_project_tests must use returncode, never parse stdout for pass/fail.""" + + @patch("subprocess.run") + def test_exit_code_0_with_1533_passed_returns_success_true(self, mock_run): + """Regression KIN-124: exit code 0 + '1533 passed' → success=True.""" + from agents.runner import _run_project_tests + mock_run.return_value = _make_subprocess_result( + returncode=0, + stdout="===================== 1533 passed in 42.7s =====================\n", + ) + result = _run_project_tests("/tmp/proj", "pytest") + assert result["success"] is True, ( + f"Expected success=True for returncode=0, got: {result}" + ) + assert result["returncode"] == 0 + + @patch("subprocess.run") + def test_exit_code_1_with_failed_output_returns_success_false(self, mock_run): + """exit code 1 + '2 failed' output → success=False.""" + from agents.runner import _run_project_tests + mock_run.return_value = _make_subprocess_result( + returncode=1, + stdout="FAILED tests/test_foo.py::test_bar\n2 failed, 10 passed in 3.1s\n", + ) + result = _run_project_tests("/tmp/proj", "pytest") + assert result["success"] is False, ( + f"Expected success=False for returncode=1, got: {result}" + ) + assert result["returncode"] == 1 + + @patch("subprocess.run") + def test_exit_code_0_with_failed_substring_in_output_returns_success_true(self, mock_run): + """exit code 0 but output has 'failed' in a log line → success=True. + + Success must be based on returncode only — not string matching. + Example: a test description containing 'failed' should not confuse auto-test. + """ + from agents.runner import _run_project_tests + mock_run.return_value = _make_subprocess_result( + returncode=0, + stdout=( + "tests/test_retry.py::test_handles_previously_failed_request PASSED\n" + "1 passed in 0.5s\n" + ), + ) + result = _run_project_tests("/tmp/proj", "pytest") + assert result["success"] is True, ( + "success must be True when returncode=0, even if 'failed' appears in output" + ) + + @patch("subprocess.run") + def test_output_is_concatenation_of_stdout_and_stderr(self, mock_run): + """output field = stdout + stderr (both captured).""" + from agents.runner import _run_project_tests + mock_run.return_value = _make_subprocess_result( + returncode=0, + stdout="1 passed\n", + stderr="PytestWarning: something\n", + ) + result = _run_project_tests("/tmp/proj", "pytest") + assert "1 passed" in result["output"] + assert "PytestWarning" in result["output"] + + +# --------------------------------------------------------------------------- +# (8-9) _run_project_tests: error handling +# --------------------------------------------------------------------------- + +class TestRunProjectTestsErrorHandling: + + @patch("subprocess.run", side_effect=subprocess.TimeoutExpired(cmd="pytest", timeout=60)) + def test_timeout_returns_success_false_and_returncode_124(self, mock_run): + """Timeout → success=False, returncode=124.""" + from agents.runner import _run_project_tests + result = _run_project_tests("/tmp/proj", "pytest", timeout=60) + assert result["success"] is False + assert result["returncode"] == 124 + assert "timed out" in result["output"].lower() + + @patch("subprocess.run", side_effect=FileNotFoundError("pytest: not found")) + def test_command_not_found_returns_success_false_and_returncode_127(self, mock_run): + """Command not found → success=False, returncode=127.""" + from agents.runner import _run_project_tests + result = _run_project_tests("/tmp/proj", "pytest") + assert result["success"] is False + assert result["returncode"] == 127 + + +# --------------------------------------------------------------------------- +# (4-7) _detect_test_command: role-based logic +# --------------------------------------------------------------------------- + +class TestDetectTestCommandRoleLogic: + """_detect_test_command must return pytest (not make test) for backend_dev + when pyproject.toml is present. This prevents vitest from running during + backend-only changes (the root cause of KIN-124).""" + + def test_backend_dev_with_pyproject_toml_returns_pytest_not_make_test(self, tmp_path): + """Regression KIN-124: backend_dev + pyproject.toml → pytest, not make test.""" + from agents.runner import _detect_test_command + # Create both pyproject.toml and Makefile with test target + (tmp_path / "pyproject.toml").write_text("[tool.pytest.ini_options]\n") + makefile = tmp_path / "Makefile" + makefile.write_text("test:\n\tmake test\n") + + cmd = _detect_test_command(str(tmp_path), role="backend_dev") + assert cmd is not None + assert "pytest" in cmd, ( + f"Expected pytest command for backend_dev, got: {cmd!r}. " + "backend_dev must not run make test (which triggers vitest)." + ) + assert "make" not in cmd, ( + f"backend_dev must not use make test, got: {cmd!r}" + ) + + def test_backend_dev_with_only_pyproject_toml_returns_pytest(self, tmp_path): + """backend_dev + only pyproject.toml (no Makefile) → pytest.""" + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[build-system]\n") + cmd = _detect_test_command(str(tmp_path), role="backend_dev") + assert cmd is not None + assert "pytest" in cmd + + def test_frontend_dev_with_makefile_returns_make_test(self, tmp_path): + """frontend_dev + Makefile with test: target → make test (correct for frontend).""" + from agents.runner import _detect_test_command + (tmp_path / "Makefile").write_text("test:\n\tnpm test\n") + cmd = _detect_test_command(str(tmp_path), role="frontend_dev") + assert cmd == "make test", ( + f"Expected 'make test' for frontend_dev with Makefile, got: {cmd!r}" + ) + + def test_frontend_dev_with_pyproject_toml_no_makefile_returns_pytest(self, tmp_path): + """frontend_dev + pyproject.toml (no Makefile) → pytest (fallback).""" + from agents.runner import _detect_test_command + (tmp_path / "pyproject.toml").write_text("[tool.pytest]\n") + cmd = _detect_test_command(str(tmp_path), role="frontend_dev") + assert cmd is not None + assert "pytest" in cmd + + def test_no_markers_returns_none(self, tmp_path): + """Empty directory → None (no test framework detected).""" + from agents.runner import _detect_test_command + cmd = _detect_test_command(str(tmp_path)) + assert cmd is None + + +# --------------------------------------------------------------------------- +# (10-11) Frontend vitest setup files +# --------------------------------------------------------------------------- + +class TestVitestSetupFiles: + """Verify the vitest setup changes that fix the KIN-124 root cause.""" + + def test_vite_config_has_setup_files(self): + """vite.config.ts must declare setupFiles pointing to vitest-setup.ts.""" + vite_config = Path(__file__).parent.parent / "web/frontend/vite.config.ts" + assert vite_config.exists(), "vite.config.ts not found" + content = vite_config.read_text() + assert "setupFiles" in content, ( + "vite.config.ts must have setupFiles to load global vitest setup" + ) + assert "vitest-setup" in content, ( + "setupFiles must reference vitest-setup.ts" + ) + + def test_vitest_setup_file_exists(self): + """web/frontend/src/__tests__/vitest-setup.ts must exist.""" + setup_file = ( + Path(__file__).parent.parent + / "web/frontend/src/__tests__/vitest-setup.ts" + ) + assert setup_file.exists(), ( + "vitest-setup.ts not found — global i18n setup is missing, " + "vitest will fail on all useI18n() components" + ) + + def test_vitest_setup_registers_i18n_plugin(self): + """vitest-setup.ts must register i18n as a global plugin.""" + setup_file = ( + Path(__file__).parent.parent + / "web/frontend/src/__tests__/vitest-setup.ts" + ) + assert setup_file.exists() + content = setup_file.read_text() + assert "i18n" in content, ( + "vitest-setup.ts must register the i18n plugin" + ) + assert "config.global.plugins" in content, ( + "vitest-setup.ts must set config.global.plugins to inject i18n into all mounts" + )