From 0653f6c508487a522dd26b9b22c330514df26acb Mon Sep 17 00:00:00 2001 From: asepharyana Date: Mon, 21 Sep 2026 17:31:16 +0700 Subject: [PATCH] fix(worker): pure --dry runs + Claude Code MCP-server hang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - --dry now touches nothing: no state file writes (run_upstream_sync uses a throwaway state dict; sync_fork_repo refuses to mutate in dry), no Discord, no push, no PR (protected-branch dry prints would-open instead). The earlier dry run polluted /tmp/pr-queue-sync-state.json and posted a false 'skip' notification — both are gone. - Claude Code conflict/quality runs now use --strict-mcp-config (with --mcp-config ''): --mcp-config '' alone still lets claude -p spawn MCP servers from settings.json/managed/plugins (observed ouroboros mcp serve hanging 15+ min with zero output until the 900s timeout). These calls only read/edit a throwaway clone and run git — no MCP server is ever needed. - tests: 51/51 (added dry-purity cases: clean-merge, conflict-failure). --- scripts/pr-queue-worker.py | 110 +++++++++++++++++++++++----------- scripts/test_pr_queue_sync.py | 42 ++++++++++++- 2 files changed, 115 insertions(+), 37 deletions(-) diff --git a/scripts/pr-queue-worker.py b/scripts/pr-queue-worker.py index 9f2bf73..30d0019 100644 --- a/scripts/pr-queue-worker.py +++ b/scripts/pr-queue-worker.py @@ -1213,30 +1213,45 @@ def _push_ref(workdir, fork, source, dest, app_token, force=False): return False, last, False -def _run_claude_sync(workdir, prompt, label, fork): +def _run_claude_sync(workdir, prompt, label, fork, dry=False): """Run Claude Code inside the sync workdir. Returns (ok, snippet). Mirrors run_ai_fix's invocation (same binary resolution + provider env) but with its own log names, a longer timeout (conflict resolution runs a full - test suite) and no push expectations — the harness pushes.""" + test suite) and no push expectations — the harness pushes. Never spawns MCP + servers (--mcp-config ''), which have been observed hanging this worker. + + In `dry` mode Claude is still run — the whole point of --dry is to exercise + the real conflict resolution without pushing — but every side effect + (Discord, state file, workdir cleanup) is skipped.""" claude_bin = shutil.which("claude") or CLAUDE_BIN if not os.path.isfile(claude_bin): return False, "[INFRA] Claude Code CLI not found at " + str(claude_bin) - try: - (workdir / (label + ".prompt.txt")).write_text(prompt) - except OSError: - pass + if not dry: + try: + (workdir / (label + ".prompt.txt")).write_text(prompt) + except OSError: + pass env = {k: v for k, v in _claude_env().items() if k in ( "PATH", "HOME", "HERMES_HOME", "ANTHROPIC_API_KEY", "ANTHROPIC_BASE_URL", "ANTHROPIC_URL", )} + # Claude Code can hang for many minutes inside this worker when it spawns an + # MCP server that stalls (observed 2026-09-21: `ouroboros mcp serve` under + # uvx hung with zero output for 15+ minutes → the 900s timeout fired). + # --mcp-config '' alone does NOT stop it — `claude -p` still starts MCP + # servers from settings.json / managed / plugins. --strict-mcp-config + # ignores ALL other MCP configuration and confines file tools to the + # workdir. These calls only read/edit files in a throwaway clone and run + # git — no MCP server is ever needed. + mcp_args = ["--mcp-config", "", "--strict-mcp-config"] out_path = workdir / (label + ".out.log") try: with open(out_path, "w") as lf: res = subprocess.run( [claude_bin, "-p", prompt, "--allowedTools", "Read,Edit,Bash,Write", - "--max-turns", str(AI_FIX_MAX_TURNS)], + "--max-turns", str(AI_FIX_MAX_TURNS)] + mcp_args, cwd=str(workdir), stdout=lf, stderr=subprocess.STDOUT, text=True, env=env, timeout=SYNC_CLAUDE_TIMEOUT, ) @@ -1440,10 +1455,16 @@ def sync_fork_repo(token, fork, parent, local_branch, upstream_branch, upstream_ merge_count, divergence, state, cfg, dry=False): """One sync attempt for one fork: clone → merge upstream → Claude Code when needed → push (or open a PR when the branch is protected). Returns - (status, detail) with status ∈ synced|pr-opened|conflict-failed|push-failed|error|dry.""" - entry = _sync_entry(state, fork) + (status, detail) with status ∈ synced|pr-opened|conflict-failed|push-failed|error|dry|pr-path. + + `dry` mode runs every step except the ones with remote side effects — no + push, no PR, no state mutation, no Discord — because the caller creates + `state` fresh and passes it in (a dry run must leave nothing behind).""" + entry = _sync_entry(state, fork) if not dry else {} workdir = SYNC_TMP_BASE / fork.replace("/", "_") try: + if dry: + entry = {} # keep the state passed in untouched during a dry run if workdir.exists(): subprocess.run(["rm", "-rf", str(workdir)], timeout=60) workdir.parent.mkdir(parents=True, exist_ok=True) @@ -1477,29 +1498,32 @@ def sync_fork_repo(token, fork, parent, local_branch, upstream_branch, upstream_ _sync_git(["merge", "--abort"], workdir, 60) note = (f"{len(conflicted)} conflicting file(s) and conflict resolution is " f"disabled: {', '.join(conflicted[:5])}") - entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, - "skip_reason": note, "notified": False}) - save_sync_state(state) + if not dry: + entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, + "skip_reason": note, "notified": False}) + save_sync_state(state) return "conflict-failed", note BUFFER.append(f" 🤖 resolving {len(conflicted)} conflict(s) with Claude Code " f"(up to {SYNC_CLAUDE_TIMEOUT}s)...") ok, snippet = _run_claude_sync( workdir, _conflict_prompt(fork, parent, upstream_branch, local_branch, conflicted), - "claude_sync_conflicts", fork) + "claude_sync_conflicts", fork, dry) if not ok: _sync_git(["merge", "--abort"], workdir, 60) note = f"conflict resolution failed — {snippet[:200]}" - entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, - "skip_reason": note, "notified": False}) - save_sync_state(state) + if not dry: + entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, + "skip_reason": note, "notified": False}) + save_sync_state(state) return "conflict-failed", note done, why = _sync_finish_merge(workdir) if not done: _sync_git(["merge", "--abort"], workdir, 60) note = f"conflict resolution incomplete — {why}" - entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, - "skip_reason": note, "notified": False}) - save_sync_state(state) + if not dry: + entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, + "skip_reason": note, "notified": False}) + save_sync_state(state) return "conflict-failed", note resolution = f"Claude Code resolved {len(conflicted)} conflict(s)" else: @@ -1511,7 +1535,7 @@ def sync_fork_repo(token, fork, parent, local_branch, upstream_branch, upstream_ BUFFER.append(f" 🤖 Claude Code quality pass on {len(merged_files)} merged file(s)...") ok, snippet = _run_claude_sync( workdir, _quality_prompt(fork, parent, upstream_branch, merged_files), - "claude_sync_quality", fork) + "claude_sync_quality", fork, dry) if ok: if _sync_commit_if_dirty(workdir, "fix: auto-fix code quality [skip ci]"): resolution += " + Claude Code quality pass committed" @@ -1523,9 +1547,10 @@ def sync_fork_repo(token, fork, parent, local_branch, upstream_branch, upstream_ still_unmerged = _sync_unmerged_files(workdir) if still_unmerged: note = f"unmerged paths remain: {', '.join(still_unmerged[:5])}" - entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, - "skip_reason": note, "notified": False}) - save_sync_state(state) + if not dry: + entry.update({"last_sync_ts": time.time(), "last_attempt_sha": upstream_sha, + "skip_reason": note, "notified": False}) + save_sync_state(state) return "conflict-failed", note merged_sha = (_sync_git(["rev-parse", "HEAD"], workdir, 30).stdout or "").strip() @@ -1552,6 +1577,9 @@ def sync_fork_repo(token, fork, parent, local_branch, upstream_branch, upstream_ f"({resolution}); head {merged_sha[:8]}; {detail}") if protected: + if dry: + return "pr-path", (f"protected branch detected ({detail[:120]}) — " + f"would open an upstream-sync PR") pr_num = open_sync_pr(token, workdir, fork, parent, local_branch, upstream_branch, upstream_sha, merge_count, divergence, resolution) if pr_num: @@ -1617,12 +1645,19 @@ def open_sync_pr(token, workdir, fork, parent, base_branch, upstream_branch, def run_upstream_sync(only=None, dry=False): """Sync every fork in the installation (bounded per tick), verify merges we - pushed earlier, and return the report lines. Never raises.""" + pushed earlier, and return the report lines. Never raises. + + In `dry` mode: no state file writes, no Discord posts, no pushes, no PRs — + the compare flow runs against a throwaway in-memory state (so the gating + `_sync_entry` calls stay off the real /tmp state file) and reports what + WOULD happen.""" lines = [] if not UPSTREAM_SYNC["enabled"] and not only: return lines try: state = load_sync_state() + if dry: + state = {} # never touch the real state during a dry run forks = list_fork_repos() if only: forks = [f for f in forks if f[1] == only] @@ -1673,26 +1708,31 @@ def run_upstream_sync(only=None, dry=False): if res == "synced": merged_sha = (entry.get("pending_verify") or {}).get("sha", "")[:8] lines.append(f" ✅ merged + pushed: {detail}") - post_sync_discord( - f"🔁 Fork synced: {fork}", - [f"⬆️ {merge_count} upstream commit(s) from `{parent}` merged into `{local_branch}`.", - f"merge head `{merged_sha}` (CI-verified on the next ticks; reverted automatically if red).", - f"https://github.com/{fork}"], - ) + if not dry: + post_sync_discord( + f"🔁 Fork synced: {fork}", + [f"⬆️ {merge_count} upstream commit(s) from `{parent}` merged into `{local_branch}`.", + f"merge head `{merged_sha}` (CI-verified on the next ticks; reverted automatically if red).", + f"https://github.com/{fork}"], + ) budget -= 1 elif res == "pr-opened": lines.append(f" 📬 {detail}") - post_sync_discord( - f"📬 Fork sync PR opened: {fork}", - [detail, f"https://github.com/{fork}/pulls"], - ) + if not dry: + post_sync_discord( + f"📬 Fork sync PR opened: {fork}", + [detail, f"https://github.com/{fork}/pulls"], + ) budget -= 1 elif res == "dry": lines.append(f" 🧪 dry run: {detail}") budget -= 1 + elif res == "pr-path": + lines.append(f" 🧪 dry run (protected): {detail}") + budget -= 1 elif res == "conflict-failed": lines.append(f" ⏭️ {detail}") - if not entry.get("notified"): + if not dry and not entry.get("notified"): entry["notified"] = True save_sync_state(state) post_sync_discord( diff --git a/scripts/test_pr_queue_sync.py b/scripts/test_pr_queue_sync.py index 712d002..a8ff9b4 100644 --- a/scripts/test_pr_queue_sync.py +++ b/scripts/test_pr_queue_sync.py @@ -181,7 +181,7 @@ def test_clean_merge_synced(tmpdir): state = {} gitstate = _install_git_fake(merge_code=0, unmerged=[]) claude_calls = [] - W._run_claude_sync = lambda workdir, prompt, label, fork: (claude_calls.append(label), (True, "ok"))[1] + W._run_claude_sync = lambda workdir, prompt, label, fork, dry=False: (claude_calls.append(label), (True, "ok"))[1] W._sync_commit_if_dirty = lambda workdir, msg: False pushes = [] @@ -201,6 +201,43 @@ def test_clean_merge_synced(tmpdir): check("clean merge gets a quality pass", claude_calls == ["claude_sync_quality"], str(claude_calls)) +def test_dry_run_is_pure(tmpdir): + print("3b. --dry never writes state, never posts Discord, never pushes") + # NOTE: the dry-path asserts `state == {}` — under the hood the worker builds + # a NEW empty state dict for dry runs (run_upstream_sync sets state={}); + # sync_fork_repo itself also refuses to mutate state in dry mode. The unit + # test calls sync_fork_repo directly, so it asserts against a pre-seeded + # dict to catch any write through that seam. + make_state_file(tmpdir) + state = {} + _install_git_fake(merge_code=0, unmerged=[]) + W._run_claude_sync = lambda workdir, prompt, label, fork, dry=False: (True, "ok") + W._sync_commit_if_dirty = lambda workdir, msg: False + pushes = [] + + def fake_push(workdir, fork, source, dest, app_token, force=False): + pushes.append(dest) + return True, "pat push ok", False + + W._push_ref = fake_push + before = dict(state) + res, detail = W.sync_fork_repo("tok", "asepharyana/shiro-neko", "zakirkun/shiro-neko", + "main", "main", "up1", 4, 26, state, W.sync_config("x"), dry=True) + check("dry returns 'dry'", res == "dry", f"{res} {detail}") + check("dry never pushes", not pushes, str(pushes)) + check("dry never writes state", state == before, f"{state} vs {before}") + + # conflict path in dry: failed resolution must NOT write state either + state = {} + _install_git_fake(merge_code=1, unmerged=["src/tools.ts"]) + W._run_claude_sync = lambda workdir, prompt, label, fork, dry=False: (False, "[INFRA] timed out") + before = dict(state) + res, detail = W.sync_fork_repo("tok", "f/x", "up/x", "main", "main", "up1", 4, 26, state, + W.sync_config("x"), dry=True) + check("dry conflict failure returns conflict-failed", res == "conflict-failed", f"{res} {detail}") + check("dry conflict failure writes no state", state == before, f"{state} vs {before}") + + # ── 4. conflicted merge resolved by Claude ──────────────────────────────── def test_conflict_resolved(tmpdir): print("4. conflicted merge handed to Claude Code, then pushed") @@ -209,7 +246,7 @@ def test_conflict_resolved(tmpdir): _install_git_fake(merge_code=1, unmerged=["src/tools.ts", "src/ui/App.tsx"]) labels = [] - def fake_claude(workdir, prompt, label, fork): + def fake_claude(workdir, prompt, label, fork, dry=False): labels.append(label) # after resolution the tree is clean → _sync_finish_merge must be called W._sync_unmerged_files = lambda w: [] @@ -432,6 +469,7 @@ def main(): test_upstream_status_parsing() test_gating(tmpdir) test_clean_merge_synced(tmpdir) + test_dry_run_is_pure(tmpdir) test_conflict_resolved(tmpdir) test_conflict_failed_skips_and_dedupes(tmpdir) test_protected_branch_opens_pr(tmpdir)