From 905f2a45fbcbaeb68ecc185b3162b7722a3f05f2 Mon Sep 17 00:00:00 2001 From: AIPass Date: Mon, 20 Apr 2026 11:24:33 -0700 Subject: [PATCH] =?UTF-8?q?feat(drone):=20fix=20pr=5Fhandler:=20scope=20co?= =?UTF-8?q?mmit=20to=20branch=5Fdir=20pathspec=20=E2=80=94=20prevents=20co?= =?UTF-8?q?ncurrent-PR=20index=20contamination=20(FPLAN-0190=20Task=20A)?= =?UTF-8?q?=20(#351)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(devpulse): watchdog: breadcrumb on exit + default timeout 1800s to 600s (FPLAN-0189) Co-Authored-By: @devpulse * feat(drone): fix pr_handler: scope commit to branch_dir pathspec — prevents concurrent-PR index contamination (FPLAN-0190 Task A) Co-Authored-By: @drone --------- Co-authored-by: @devpulse Co-authored-by: @drone --- .claude/hooks/auto_fix_diagnostics.py | 20 +++- .claude/hooks/subagent_stop_gate.py | 100 ++++++++++++++++++ src/aipass/devpulse/.seedgo/bypass.json | 5 + .../devpulse/apps/handlers/watchdog/agent.py | 5 +- src/aipass/devpulse/apps/modules/watchdog.py | 5 +- .../devpulse/tests/test_watchdog_module.py | 47 ++++++++ .../drone/apps/handlers/git/pr_handler.py | 7 +- .../apps/plugins/devpulse_ops/pr_plugin.py | 5 +- src/aipass/drone/tests/test_git_module.py | 61 +++++++++++ 9 files changed, 248 insertions(+), 7 deletions(-) create mode 100644 .claude/hooks/subagent_stop_gate.py diff --git a/.claude/hooks/auto_fix_diagnostics.py b/.claude/hooks/auto_fix_diagnostics.py index 3afc6dde..306fabc5 100644 --- a/.claude/hooks/auto_fix_diagnostics.py +++ b/.claude/hooks/auto_fix_diagnostics.py @@ -13,9 +13,10 @@ Key behaviors: - Surfaces ALL errors in additionalContext so Claude sees them - Smart batching per-file -Version: 5.0.0 +Version: 5.1.0 CHANGELOG: + - v5.1.0 (2026-04-19): Added ruff format --check to surface format drift. - v5.0.0 (2026-03-17): Replaced mcp__ide__getDiagnostics with direct pyright. Added state file for PreToolUse gate integration. Single-file pyright (not whole project). @@ -99,7 +100,22 @@ def run_python_checks(file_path: str) -> list[str]: except Exception: pass - # 3. AIPass-specific pattern checks + # 3. Ruff format check — detect format drift + try: + result = subprocess.run( + ["ruff", "format", "--check", file_path], + capture_output=True, + text=True, + timeout=10 + ) + if result.returncode != 0: + errors.append(f"FORMAT: {Path(file_path).name} needs ruff format (run: ruff format {Path(file_path).name})") + except FileNotFoundError: + pass + except Exception: + pass + + # 4. AIPass-specific pattern checks try: content = Path(file_path).read_text(encoding="utf-8") lines = content.split("\n") diff --git a/.claude/hooks/subagent_stop_gate.py b/.claude/hooks/subagent_stop_gate.py new file mode 100644 index 00000000..2fd640df --- /dev/null +++ b/.claude/hooks/subagent_stop_gate.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +""" +SubagentStop Gate — Checks files modified by subagents before allowing them to finish. + +Runs seedgo checklist + basic validation on any .py files the subagent touched. +If violations found, blocks the stop and tells the subagent to fix them. + +Version: 1.0.0 +""" + +import json +import sys +import subprocess +from pathlib import Path + +AIPASS_ROOT = Path.home() / "Projects" / "AIPass" + + +def get_modified_py_files() -> list[str]: + """Get Python files modified in the working tree (unstaged + staged).""" + try: + result = subprocess.run( + ["git", "diff", "--name-only", "HEAD"], + capture_output=True, text=True, timeout=5, + cwd=str(AIPASS_ROOT) + ) + files = [] + for line in result.stdout.strip().split("\n"): + line = line.strip() + if line.endswith(".py") and not line.startswith(".claude/"): + full = AIPASS_ROOT / line + if full.exists(): + files.append(str(full)) + return files + except Exception: + return [] + + +def run_seedgo_checklist(file_path: str) -> list[str]: + """Run seedgo checklist on a single file.""" + if "/.claude/" in file_path: + return [] + try: + result = subprocess.run( + ["drone", "@seedgo", "checklist", file_path], + capture_output=True, text=True, timeout=15, + cwd=str(AIPASS_ROOT) + ) + if result.returncode != 0: + return [] + violations = [] + for line in result.stdout.split("\n"): + line = line.strip() + if line.startswith("\u2717"): + v = line[1:].strip() + if v: + violations.append(v) + return violations[:5] + except Exception: + return [] + + +def main(): + try: + input_data = json.load(sys.stdin) + + modified = get_modified_py_files() + if not modified: + return # Nothing to check + + all_violations = {} + for f in modified: + vs = run_seedgo_checklist(f) + if vs: + name = Path(f).name + all_violations[name] = vs + + if not all_violations: + return # All clear + + # Build the block reason + lines = ["Standards violations found in files you modified:\n"] + for fname, vs in all_violations.items(): + lines.append(f" {fname}:") + for v in vs: + lines.append(f" - {v}") + lines.append("\nFix these violations before finishing.") + + output = { + "decision": "block", + "reason": "\n".join(lines) + } + print(json.dumps(output)) + + except Exception: + pass # Silent fail — don't block on errors + + +if __name__ == "__main__": + main() diff --git a/src/aipass/devpulse/.seedgo/bypass.json b/src/aipass/devpulse/.seedgo/bypass.json index eb944dce..e1422afb 100644 --- a/src/aipass/devpulse/.seedgo/bypass.json +++ b/src/aipass/devpulse/.seedgo/bypass.json @@ -100,6 +100,11 @@ "standard": "json_structure", "file": "apps/handlers/watchdog/registry.py", "reason": "Devpulse is a manager branch with no json_handler. Registry logs through prax system_logger. Same situation as watchdog/agent.py, timer.py, schedule.py." + }, + { + "standard": "encapsulation", + "file": "tests/test_watchdog_module.py", + "reason": "Test file imports aipass.devpulse.apps.modules.watchdog for direct router testing (same-branch import). Encapsulation check pattern-matches on 'from aipass.*.apps.*' shape regardless of same- vs cross-branch. Inherits branch-level gap (manager branch, no apps/handlers/__init__.py inspect.stack guard)." } ], "notes": { diff --git a/src/aipass/devpulse/apps/handlers/watchdog/agent.py b/src/aipass/devpulse/apps/handlers/watchdog/agent.py index 352564b8..86dba7a2 100644 --- a/src/aipass/devpulse/apps/handlers/watchdog/agent.py +++ b/src/aipass/devpulse/apps/handlers/watchdog/agent.py @@ -178,14 +178,15 @@ def _classify_exit(branch_path: Path, lock_existed: bool) -> tuple[str, str, int def watch_agent( agent_id: str, - timeout_seconds: int = 1800, + timeout_seconds: int = 600, poll_interval: float = 2.0, ) -> dict: """Block until the dispatched agent at `agent_id` exits. Args: agent_id: Branch token like ``@drone`` (or bare ``drone``). - timeout_seconds: Maximum wait. Default 30 min. + timeout_seconds: Maximum wait. Default 10 min — catches crashes + silent-finishes + fast; long agent watches should pass an explicit ``--timeout``. poll_interval: Seconds between checks. Default 2.0. Returns: diff --git a/src/aipass/devpulse/apps/modules/watchdog.py b/src/aipass/devpulse/apps/modules/watchdog.py index 8b32ab91..56c1b8c1 100644 --- a/src/aipass/devpulse/apps/modules/watchdog.py +++ b/src/aipass/devpulse/apps/modules/watchdog.py @@ -32,7 +32,7 @@ from aipass.prax.apps.modules.logger import system_logger as logger from aipass.cli.apps.modules import console, error, warning _VALID_SUBCOMMANDS = ["agent", "timer", "schedule", "status", "cancel", "list"] -_DEFAULT_AGENT_TIMEOUT = 1800 +_DEFAULT_AGENT_TIMEOUT = 600 _NOT_IMPLEMENTED_MSG = "{sub} is not yet implemented in this phase — see FPLAN-0186 (Phase {phase})" # Phase 4 wired cancel + list for real. Left the map so future deferrals can reuse the shape. _PHASE_BY_SUB: dict[str, int] = {} @@ -353,6 +353,9 @@ def _handle_agent(sub_args: List[str]) -> bool: reason = result.get("reason", "") elapsed = result.get("elapsed", 0) console.print(f"[bold]watchdog agent[/bold] {agent_id} -> state={state} elapsed={elapsed}s reason={reason}") + console.print( + f'watchdog: {agent_id} stopped (state={state}). Next: drone @ai_mail dispatch {agent_id} "check in" "..."' + ) return True diff --git a/src/aipass/devpulse/tests/test_watchdog_module.py b/src/aipass/devpulse/tests/test_watchdog_module.py index 83bf7809..49e0a95f 100644 --- a/src/aipass/devpulse/tests/test_watchdog_module.py +++ b/src/aipass/devpulse/tests/test_watchdog_module.py @@ -279,3 +279,50 @@ def test_agent_subcommand_invalid_timeout(capsys): captured = capsys.readouterr() combined = captured.out + captured.err assert "invalid" in combined.lower() or "--timeout" in combined.lower() + + +def test_agent_subcommand_default_timeout_is_600(): + """Without an explicit --timeout, the module passes 600s (FPLAN-0189).""" + captured_args = {} + + def fake_watch_agent(agent_id, timeout_seconds=9999): + """Fake watcher — records the timeout the module passed in.""" + captured_args["timeout"] = timeout_seconds + return { + "woke": True, + "reason": "fake", + "elapsed": 1, + "agent_state": "completed", + "exit_code": 0, + "agent_id": agent_id, + } + + fake_module = type(sys)("fake_agent_mod") + fake_module.watch_agent = fake_watch_agent + + with patch("importlib.import_module", return_value=fake_module): + wd_mod.handle_command("watchdog", ["agent", "@flow"]) + + assert captured_args["timeout"] == 600 + + +def test_agent_subcommand_emits_next_action_breadcrumb(capsys): + """On exit, the CLI prints a 'Next: drone @ai_mail dispatch' breadcrumb (FPLAN-0189).""" + fake_result = { + "woke": True, + "reason": "fake clean exit", + "elapsed": 5, + "agent_state": "completed", + "exit_code": 0, + "agent_id": "@drone", + } + fake_module = type(sys)("fake_agent_mod") + fake_module.watch_agent = lambda agent_id, timeout_seconds=600: fake_result + + with patch("importlib.import_module", return_value=fake_module): + wd_mod.handle_command("watchdog", ["agent", "@drone"]) + + captured = capsys.readouterr() + combined = captured.out + captured.err + assert "Next: drone @ai_mail dispatch @drone" in combined + assert "state=completed" in combined diff --git a/src/aipass/drone/apps/handlers/git/pr_handler.py b/src/aipass/drone/apps/handlers/git/pr_handler.py index 1e3461e7..9d7ad61d 100644 --- a/src/aipass/drone/apps/handlers/git/pr_handler.py +++ b/src/aipass/drone/apps/handlers/git/pr_handler.py @@ -122,9 +122,14 @@ def create_pr(branch_name: str, description: str, branch_dir: Path) -> dict: return result # Step 5: Commit on main (changes stay local) + # Pathspec ('-- rel_dir/') scopes the commit to branch_dir only, + # preventing pre-staged files from other concurrent PRs from being + # swept in. The staging step already scoped git add, but another + # drone @git pr could stage its own files into the shared index + # between our add and our commit. commit_msg = f"feat({branch_name}): {description}\n\nCo-Authored-By: @{branch_name} <{branch_name}@aipass>" commit = subprocess.run( - ["git", "commit", "-m", commit_msg], + ["git", "commit", "-m", commit_msg, "--", str(rel_dir) + "/"], capture_output=True, text=True, cwd=str(repo_root), diff --git a/src/aipass/drone/apps/plugins/devpulse_ops/pr_plugin.py b/src/aipass/drone/apps/plugins/devpulse_ops/pr_plugin.py index ab6279db..e3e09446 100644 --- a/src/aipass/drone/apps/plugins/devpulse_ops/pr_plugin.py +++ b/src/aipass/drone/apps/plugins/devpulse_ops/pr_plugin.py @@ -139,7 +139,10 @@ def create_system_pr(description: str, caller: str) -> dict: cwd=str(repo_root), ) if diff_check.returncode != 0: - # There are staged changes — commit them + # There are staged changes — commit them. + # No pathspec needed here: git add -A already staged the whole + # repo intentionally (system-pr is global by design), and the + # lock prevents concurrent system-prs from racing into the index. commit_msg = f"feat(system): {description}\n\nCo-Authored-By: @{caller} <{caller}@aipass>" commit = subprocess.run( ["git", "commit", "-m", commit_msg], diff --git a/src/aipass/drone/tests/test_git_module.py b/src/aipass/drone/tests/test_git_module.py index 17151baf..8df06a6b 100644 --- a/src/aipass/drone/tests/test_git_module.py +++ b/src/aipass/drone/tests/test_git_module.py @@ -407,6 +407,7 @@ class TestPRHandler: call_count = 0 def mock_subprocess_run(cmd, **kwargs): + """Simulate git subprocess returning main branch and staged-nothing.""" nonlocal call_count call_count += 1 result = MagicMock() @@ -448,6 +449,7 @@ class TestPRHandler: monkeypatch.chdir(tmp_path) def mock_subprocess_run(cmd, **kwargs): + """Simulate git subprocess returning main branch, then early exit on no staged files.""" result = MagicMock() result.stderr = "" result.stdout = "" @@ -478,6 +480,62 @@ class TestPRHandler: # Lock must always be released, even on early exit release_mock.assert_called_once_with(force=True) + def test_commit_uses_pathspec_not_whole_index(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Commit is scoped to branch_dir — pre-staged files outside it are excluded. + + Regression test for FPLAN-0190: concurrent drone @git pr calls could + contaminate each other's commits because git commit with no pathspec + commits the entire index, not just files staged in this invocation. + """ + registry = tmp_path / "AIPASS_REGISTRY.json" + registry.write_text("{}", encoding="utf-8") + monkeypatch.chdir(tmp_path) + + commit_cmd_seen: list[list[str]] = [] + + def mock_subprocess_run(cmd, **kwargs): + """Simulate git/gh subprocess calls, recording commit invocations.""" + r = MagicMock() + r.stderr = "" + r.stdout = "" + if cmd[1:3] == ["rev-parse", "--abbrev-ref"]: + r.returncode = 0 + r.stdout = "main\n" + elif cmd[0] == "git" and cmd[1] == "add": + r.returncode = 0 + elif cmd[1:3] == ["diff", "--cached"]: + r.returncode = 1 # 1 means something is staged + elif cmd[0] == "git" and cmd[1] == "commit": + commit_cmd_seen.append(list(cmd)) + r.returncode = 0 + r.stdout = "[main abc1234] feat(api): test" + elif cmd[0] == "git" and cmd[1] == "branch": + r.returncode = 0 + elif cmd[0] == "git" and cmd[1] == "push": + r.returncode = 0 + elif cmd[0] == "gh": + r.returncode = 0 + r.stdout = "https://github.com/test/repo/pull/1" + else: + r.returncode = 0 + return r + + with patch("aipass.drone.apps.handlers.git.pr_handler.subprocess.run", side_effect=mock_subprocess_run): + with patch( + "aipass.drone.apps.handlers.git.pr_handler.acquire_lock", + return_value={"success": True, "message": "ok"}, + ): + with patch("aipass.drone.apps.handlers.git.pr_handler.release_lock"): + create_pr("api", "test desc", tmp_path / "src" / "aipass" / "api") + + # The commit command must include '--' separator + pathspec to scope to branch_dir + assert commit_cmd_seen, "commit was never called" + commit_cmd = commit_cmd_seen[0] + assert "--" in commit_cmd, "commit missing '--' pathspec separator" + pathspec_idx = commit_cmd.index("--") + pathspec = commit_cmd[pathspec_idx + 1] + assert "src/aipass/api" in pathspec, f"pathspec should target branch_dir, got: {pathspec}" + # =========================================================================== # 5. git_module — command routing, unknown commands, help/introspection @@ -698,6 +756,7 @@ class TestTriggerFireIntegration: call_count = 0 def mock_run(cmd, **kwargs): + """Simulate git subprocess calls and count invocations.""" nonlocal call_count call_count += 1 r = MagicMock() @@ -749,6 +808,7 @@ class TestTriggerFireIntegration: monkeypatch.chdir(tmp_path) def mock_run(cmd, **kwargs): + """Simulate git/gh subprocess calls for trigger-failure resilience test.""" r = MagicMock() r.stderr = "" if cmd[1:3] == ["rev-parse", "--abbrev-ref"]: @@ -790,6 +850,7 @@ class TestTriggerFireIntegration: call_idx = 0 def mock_run(cmd, **kwargs): + """Simulate gh pr merge and git pull subprocess calls.""" nonlocal call_idx call_idx += 1 r = MagicMock()