feat(drone): fix pr_handler: scope commit to branch_dir pathspec — prevents concurrent-PR index contamination (FPLAN-0190 Task A) (#351)
* feat(devpulse): watchdog: breadcrumb on exit + default timeout 1800s to 600s (FPLAN-0189) Co-Authored-By: @devpulse <devpulse@aipass> * feat(drone): fix pr_handler: scope commit to branch_dir pathspec — prevents concurrent-PR index contamination (FPLAN-0190 Task A) Co-Authored-By: @drone <drone@aipass> --------- Co-authored-by: @devpulse <devpulse@aipass> Co-authored-by: @drone <drone@aipass>
This commit is contained in:
co-authored by
@devpulse
@drone
parent
c941c3d6ac
commit
905f2a45fb
@@ -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")
|
||||
|
||||
@@ -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()
|
||||
@@ -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": {
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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],
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user