diff --git a/core/models.py b/core/models.py index d2aba73..eb666f0 100644 --- a/core/models.py +++ b/core/models.py @@ -103,7 +103,26 @@ def get_project(conn: sqlite3.Connection, id: str) -> dict | None: def delete_project(conn: sqlite3.Connection, id: str) -> None: """Delete a project and all its related data (modules, decisions, tasks, phases).""" - # Delete tables that have FK references to tasks BEFORE deleting tasks + # Delete child tables that reference pipelines/tasks without ON DELETE CASCADE, + # before their parents are deleted (FK constraint enforcement, foreign_keys=ON). + conn.execute( + "DELETE FROM department_handoffs WHERE task_id IN (SELECT id FROM tasks WHERE project_id = ?)", + (id,), + ) + conn.execute( + "DELETE FROM department_handoffs WHERE pipeline_id IN (SELECT id FROM pipelines WHERE project_id = ?)", + (id,), + ) + conn.execute( + "DELETE FROM pipeline_log WHERE pipeline_id IN (SELECT id FROM pipelines WHERE project_id = ?)", + (id,), + ) + conn.execute("DELETE FROM audit_log WHERE project_id = ?", (id,)) + conn.execute("DELETE FROM support_tickets WHERE project_id = ?", (id,)) + conn.execute("DELETE FROM hook_logs WHERE project_id = ?", (id,)) + conn.execute("DELETE FROM hooks WHERE project_id = ?", (id,)) + conn.execute("DELETE FROM project_links WHERE from_project = ? OR to_project = ?", (id, id)) + # Delete tables with direct project_id FK (order: FK children before parents) # project_environments must come before tasks (FK on project_id) for table in ("modules", "agent_logs", "decisions", "pipelines", "project_phases", "project_environments", "chat_messages", "tasks"): conn.execute(f"DELETE FROM {table} WHERE project_id = ?", (id,)) diff --git a/tests/test_KIN-117_regression.py b/tests/test_KIN-117_regression.py new file mode 100644 index 0000000..409940c --- /dev/null +++ b/tests/test_KIN-117_regression.py @@ -0,0 +1,428 @@ +"""Regression tests for KIN-117 — dev-agent silent success detection. + +Root cause: dev-agents периодически заявляют об успешной реализации, но файлы +не записываются на диск. Три причины: + (1) --dangerously-skip-permissions не добавляется когда stdin=DEVNULL (noninteractive) + (2) worktree создаётся, но merge не происходит (uncommitted files) + (3) permission-retry вызывает run_agent без working_dir_override=worktree_path + +Fix D добавляет guard: после успешного шага dev-агента проверяет git diff — +если изменений нет, помечает шаг suspicious=True. + +Acceptance criteria: +(1) Fix A: noninteractive=True → --dangerously-skip-permissions в subprocess cmd +(2) Fix D: dev-агент success + пустой git diff → step.suspicious=True +(3) Fix D: dev-агент success + непустой git diff → step.suspicious НЕ выставлен (нет false positive) +(4) Fix C: permission retry в auto_complete режиме → subprocess cwd=worktree_path +(5) Fix B: merge_worktree запускает git add -A + git commit перед git merge --no-ff +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from core.db import init_db +from core import models + + +# ─── Helpers ──────────────────────────────────────────────────────────────── + +def _ok(stdout=""): + m = MagicMock() + m.returncode = 0 + m.stdout = stdout + m.stderr = "" + return m + + +def _fail(stdout="", stderr="error"): + m = MagicMock() + m.returncode = 1 + m.stdout = stdout + m.stderr = stderr + return m + + +def _claude_success(): + return _ok(json.dumps({"result": "all done, implementation complete"})) + + +def _permission_denied(): + """Subprocess result that triggers _is_permission_error.""" + m = MagicMock() + m.returncode = 1 + m.stdout = "" + m.stderr = "permission denied to write file" + return m + + +# ─── Fixtures ──────────────────────────────────────────────────────────────── + +@pytest.fixture +def conn_basic(tmp_path): + """Project with path=tmp_path (real dir), review mode, no worktrees.""" + c = init_db(":memory:") + models.create_project(c, "prj1", "Test Project", str(tmp_path), + tech_stack=["python"]) + models.create_task(c, "TSK-001", "prj1", "Fix something", + brief={"route_type": "debug"}) + yield c, str(tmp_path) + c.close() + + +@pytest.fixture +def conn_auto_wt(tmp_path): + """Project with auto_complete mode and worktrees enabled, real path.""" + c = init_db(":memory:") + models.create_project(c, "prj2", "Auto WT Project", str(tmp_path), + tech_stack=["python"]) + c.execute( + "UPDATE projects SET execution_mode='auto_complete', worktrees_enabled=1 WHERE id='prj2'" + ) + c.commit() + models.create_task(c, "TSK-002", "prj2", "Fix with worktree", + brief={"route_type": "debug"}) + yield c, str(tmp_path) + c.close() + + +# ─── Fix A: noninteractive mode добавляет --dangerously-skip-permissions ───── + +class TestFixA_NoninteractiveSkipPermissions: + """Fix A: is_noninteractive вычисляется ДО проверки allow_write.""" + + @patch("agents.runner.subprocess.run") + def test_noninteractive_adds_dangerously_skip_permissions(self, mock_run): + """noninteractive=True, allow_write=False → --dangerously-skip-permissions в cmd.""" + from agents.runner import _run_claude + mock_run.return_value = _claude_success() + + _run_claude("test prompt", noninteractive=True, allow_write=False) + + cmd = mock_run.call_args[0][0] + assert "--dangerously-skip-permissions" in cmd, ( + "--dangerously-skip-permissions must be added when noninteractive=True, " + f"got cmd: {cmd}" + ) + + @patch("agents.runner.subprocess.run") + def test_allow_write_adds_dangerously_skip_permissions(self, mock_run): + """allow_write=True → --dangerously-skip-permissions в cmd (существующее поведение).""" + from agents.runner import _run_claude + mock_run.return_value = _claude_success() + + _run_claude("test prompt", allow_write=True, noninteractive=False) + + cmd = mock_run.call_args[0][0] + assert "--dangerously-skip-permissions" in cmd + + @patch("agents.runner.subprocess.run") + def test_interactive_no_allow_write_no_skip_permissions(self, mock_run): + """allow_write=False + noninteractive=False → --dangerously-skip-permissions НЕ в cmd.""" + from agents.runner import _run_claude + mock_run.return_value = _claude_success() + + _run_claude("test prompt", allow_write=False, noninteractive=False) + + cmd = mock_run.call_args[0][0] + assert "--dangerously-skip-permissions" not in cmd, ( + "--dangerously-skip-permissions must NOT be added in interactive mode without allow_write" + ) + + +# ─── Fix D: suspicious guard (non-worktree path) ───────────────────────────── + +class TestFixD_SuspiciousGuardNoWorktree: + """Fix D: guard проверяет git diff после успешного шага dev-агента.""" + + @patch("agents.runner.check_claude_auth") + @patch("agents.runner.subprocess.run") + def test_success_empty_git_diff_marks_step_suspicious(self, mock_run, mock_auth, + conn_basic): + """dev-агент success + пустой git diff → result['suspicious']=True.""" + from agents.runner import run_pipeline + conn, proj_path = conn_basic + + def side_effect(cmd, **kwargs): + cmd_str = " ".join(str(c) for c in cmd) + if "diff" in cmd_str: + return _ok("") # empty diff → no file changes + return _claude_success() # claude call + + mock_run.side_effect = side_effect + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-001", [{"role": "debugger", "brief": "fix it"}]) + + assert result["success"] is True + step_result = result["results"][0] + assert step_result.get("suspicious") is True, ( + "Step must be marked suspicious=True when agent claims success " + f"but git diff is empty. Got result: {step_result}" + ) + + @patch("agents.runner.check_claude_auth") + @patch("agents.runner.subprocess.run") + def test_success_nonempty_git_diff_not_suspicious(self, mock_run, mock_auth, + conn_basic): + """dev-агент success + непустой git diff → suspicious НЕ выставляется.""" + from agents.runner import run_pipeline + conn, proj_path = conn_basic + + def side_effect(cmd, **kwargs): + cmd_str = " ".join(str(c) for c in cmd) + if "diff" in cmd_str: + return _ok("src/app.py\nsrc/utils.py\n") # files changed + return _claude_success() + + mock_run.side_effect = side_effect + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-001", [{"role": "debugger", "brief": "fix it"}]) + + assert result["success"] is True + step_result = result["results"][0] + assert not step_result.get("suspicious"), ( + "Step must NOT be suspicious when git diff shows changed files. " + f"Got result: {step_result}" + ) + + @patch("agents.runner.check_claude_auth") + @patch("agents.runner.subprocess.run") + def test_suspicious_step_does_not_block_pipeline(self, mock_run, mock_auth, + conn_basic): + """suspicious шаг не останавливает pipeline — только выставляет флаг.""" + from agents.runner import run_pipeline + conn, proj_path = conn_basic + + def side_effect(cmd, **kwargs): + cmd_str = " ".join(str(c) for c in cmd) + if "diff" in cmd_str: + return _ok("") # empty diff → suspicious, but pipeline continues + return _claude_success() + + mock_run.side_effect = side_effect + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-001", [{"role": "debugger", "brief": "fix"}]) + + # Pipeline must succeed even when step is suspicious + assert result["success"] is True + assert result["steps_completed"] == 1 + + +# ─── Fix D: suspicious guard (worktree path) ───────────────────────────────── + +class TestFixD_SuspiciousGuardWithWorktree: + """Fix D: guard проверяет merged_files после worktree merge.""" + + @patch("agents.runner.check_claude_auth") + @patch("core.worktree.cleanup_worktree") + @patch("core.worktree.merge_worktree", + return_value={"success": True, "conflicts": [], "merged_files": []}) + @patch("core.worktree.create_worktree", return_value="/tmp/WT-suspicious-debugger") + @patch("core.worktree.ensure_gitignore") + @patch("agents.runner.subprocess.run") + def test_worktree_empty_merged_files_marks_suspicious( + self, mock_run, mock_gitignore, mock_create, mock_merge, mock_cleanup, mock_auth, + conn_auto_wt + ): + """worktree merge возвращает merged_files=[] → шаг помечается suspicious=True.""" + from agents.runner import run_pipeline + conn, proj_path = conn_auto_wt + mock_run.return_value = _claude_success() + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-002", [{"role": "debugger", "brief": "fix it"}]) + + assert result["success"] is True + step_result = result["results"][0] + assert step_result.get("suspicious") is True, ( + "Step must be marked suspicious when worktree merge returns merged_files=[]. " + f"Got result: {step_result}" + ) + + @patch("agents.runner.check_claude_auth") + @patch("core.worktree.cleanup_worktree") + @patch("core.worktree.merge_worktree", + return_value={"success": True, "conflicts": [], "merged_files": ["src/app.py"]}) + @patch("core.worktree.create_worktree", return_value="/tmp/WT-ok-debugger") + @patch("core.worktree.ensure_gitignore") + @patch("agents.runner.subprocess.run") + def test_worktree_nonempty_merged_files_not_suspicious( + self, mock_run, mock_gitignore, mock_create, mock_merge, mock_cleanup, mock_auth, + conn_auto_wt + ): + """worktree merge возвращает merged_files=['src/app.py'] → шаг НЕ suspicious.""" + from agents.runner import run_pipeline + conn, proj_path = conn_auto_wt + mock_run.return_value = _claude_success() + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-002", [{"role": "debugger", "brief": "fix it"}]) + + assert result["success"] is True + step_result = result["results"][0] + assert not step_result.get("suspicious"), ( + "Step must NOT be suspicious when worktree has merged files. " + f"Got result: {step_result}" + ) + + +# ─── Fix C: permission retry использует worktree_path ───────────────────────── + +class TestFixC_PermissionRetryWorktreePath: + """Fix C: retry после permission error передаёт working_dir_override=worktree_path.""" + + FAKE_WT = "/tmp/fake-worktree-kin117" + + @patch("agents.runner.check_claude_auth") + @patch("core.worktree.cleanup_worktree") + @patch("core.worktree.merge_worktree", + return_value={"success": True, "conflicts": [], "merged_files": ["fix.py"]}) + @patch("core.worktree.create_worktree", return_value=FAKE_WT) + @patch("core.worktree.ensure_gitignore") + @patch("agents.runner.subprocess.run") + def test_permission_retry_uses_worktree_cwd( + self, mock_run, mock_gitignore, mock_create, mock_merge, mock_cleanup, mock_auth, + conn_auto_wt + ): + """При permission error в auto_complete режиме retry запускается в cwd=worktree_path.""" + from agents.runner import run_pipeline + conn, proj_path = conn_auto_wt + + claude_calls = [] + + def side_effect(cmd, **kwargs): + if "--output-format" in cmd: + cwd = kwargs.get("cwd") + claude_calls.append({"cmd": list(cmd), "cwd": cwd}) + # First claude call → permission error triggers retry + if len(claude_calls) == 1: + return _permission_denied() + return _claude_success() + # Non-claude subprocess calls (git diff, etc.) + return _ok("") + + mock_run.side_effect = side_effect + mock_auth.return_value = None + + result = run_pipeline(conn, "TSK-002", + [{"role": "debugger", "brief": "implement feature"}]) + + assert len(claude_calls) >= 2, ( + f"Expected at least 2 claude calls (first + retry), got {len(claude_calls)}" + ) + retry_call = claude_calls[1] + assert retry_call["cwd"] == self.FAKE_WT, ( + f"Retry must use worktree_path={self.FAKE_WT!r} as cwd, " + f"got {retry_call['cwd']!r}. " + "Fix: add working_dir_override=worktree_path to permission retry run_agent call." + ) + + +# ─── Fix B: merge_worktree auto-commits перед merge ────────────────────────── + +class TestFixB_MergeWorktreePreCommit: + """Fix B: merge_worktree выполняет git add -A + git commit перед git merge --no-ff.""" + + @patch("subprocess.run") + def test_merge_worktree_runs_git_add_before_merge(self, mock_run, tmp_path): + """git add -A вызывается ДО git merge --no-ff.""" + from core.worktree import merge_worktree + + worktree_path = tmp_path / ".kin_worktrees" / "TASK-001-debugger" + worktree_path.mkdir(parents=True) + + commands_seen = [] + + def run_side_effect(cmd, **kwargs): + commands_seen.append(list(cmd)) + return _ok() + + mock_run.side_effect = run_side_effect + + merge_worktree(str(worktree_path), str(tmp_path)) + + flat = [" ".join(c) for c in commands_seen] + add_idx = next( + (i for i, c in enumerate(flat) if "add" in c and "-A" in c), None + ) + merge_idx = next( + (i for i, c in enumerate(flat) if "merge" in c and "--no-ff" in c), None + ) + + assert add_idx is not None, ( + "git add -A must be called before merge. " + f"Commands seen: {flat}" + ) + assert merge_idx is not None, "git merge --no-ff must be called" + assert add_idx < merge_idx, ( + f"git add -A (idx={add_idx}) must be BEFORE git merge --no-ff (idx={merge_idx})" + ) + + @patch("subprocess.run") + def test_merge_worktree_runs_git_commit_before_merge(self, mock_run, tmp_path): + """git commit вызывается ДО git merge --no-ff.""" + from core.worktree import merge_worktree + + worktree_path = tmp_path / ".kin_worktrees" / "TASK-001-debugger" + worktree_path.mkdir(parents=True) + + commands_seen = [] + + def run_side_effect(cmd, **kwargs): + commands_seen.append(list(cmd)) + return _ok() + + mock_run.side_effect = run_side_effect + + merge_worktree(str(worktree_path), str(tmp_path)) + + flat = [" ".join(c) for c in commands_seen] + commit_idx = next( + (i for i, c in enumerate(flat) if "commit" in c and "-m" in c), None + ) + merge_idx = next( + (i for i, c in enumerate(flat) if "merge" in c and "--no-ff" in c), None + ) + + assert commit_idx is not None, ( + "git commit -m must be called before merge. " + f"Commands seen: {flat}" + ) + assert merge_idx is not None, "git merge --no-ff must be called" + assert commit_idx < merge_idx, ( + f"git commit (idx={commit_idx}) must be BEFORE git merge --no-ff (idx={merge_idx})" + ) + + @patch("subprocess.run") + def test_merge_worktree_commit_failure_does_not_block_merge(self, mock_run, tmp_path): + """Если git commit возвращает non-zero (нечего коммитить) — merge всё равно запускается.""" + from core.worktree import merge_worktree + + worktree_path = tmp_path / ".kin_worktrees" / "TASK-001-debugger" + worktree_path.mkdir(parents=True) + + commands_seen = [] + + def run_side_effect(cmd, **kwargs): + commands_seen.append(list(cmd)) + cmd_str = " ".join(cmd) + if "commit" in cmd_str: + return _fail(stderr="nothing to commit") # nothing to commit + return _ok() + + mock_run.side_effect = run_side_effect + + result = merge_worktree(str(worktree_path), str(tmp_path)) + + flat = [" ".join(c) for c in commands_seen] + merge_calls = [c for c in flat if "merge" in c and "--no-ff" in c] + assert len(merge_calls) > 0, ( + "git merge --no-ff must still be called even when pre-commit fails. " + f"Commands seen: {flat}" + ) + assert result["success"] is True diff --git a/tests/test_models.py b/tests/test_models.py index 92e5703..2da8ea2 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -361,6 +361,47 @@ def test_delete_project_nonexistent_does_not_raise(conn): models.delete_project(conn, "nonexistent") +def test_delete_project_with_pipeline_and_handoffs(conn): + """FK bug fix: delete_project не падает при наличии department_handoffs и pipeline_log.""" + models.create_project(conn, "p1", "P1", "/p1") + models.create_task(conn, "P1-001", "p1", "Task", status="in_progress") + pipeline = models.create_pipeline(conn, "P1-001", "p1", "hotfix", [{"role": "backend_dev"}]) + models.create_handoff(conn, pipeline["id"], "P1-001", "engineering") + models.write_log(conn, pipeline["id"], "test log message") + models.log_audit_event(conn, "dangerous_skip", task_id="P1-001", project_id="p1") + + # Must not raise OperationalError: FOREIGN KEY constraint failed + models.delete_project(conn, "p1") + + assert conn.execute("SELECT COUNT(*) FROM department_handoffs").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM pipeline_log").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM audit_log WHERE project_id='p1'").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM pipelines WHERE project_id='p1'").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM tasks WHERE project_id='p1'").fetchone()[0] == 0 + + +def test_delete_project_cleans_hooks_and_project_links(conn): + """FK fix: delete_project удаляет hooks, hook_logs и project_links.""" + models.create_project(conn, "p1", "P1", "/p1") + models.create_project(conn, "p2", "P2", "/p2") + # Create hook and hook_log for p1 + conn.execute( + "INSERT INTO hooks (project_id, name, event, command) VALUES ('p1', 'h', 'pipeline_completed', 'echo ok')" + ) + hook_id = conn.execute("SELECT last_insert_rowid()").fetchone()[0] + conn.execute( + "INSERT INTO hook_logs (hook_id, project_id, success) VALUES (?, 'p1', 1)", (hook_id,) + ) + conn.commit() + models.create_project_link(conn, "p1", "p2", "depends_on") + + models.delete_project(conn, "p1") + + assert conn.execute("SELECT COUNT(*) FROM hooks WHERE project_id='p1'").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM hook_logs WHERE project_id='p1'").fetchone()[0] == 0 + assert conn.execute("SELECT COUNT(*) FROM project_links WHERE from_project='p1' OR to_project='p1'").fetchone()[0] == 0 + + # -- Agent Logs -- def test_log_agent_run(conn):