From 54e1eb6a79d289cd9d183648119c81462d59e7c3 Mon Sep 17 00:00:00 2001 From: ProtocolWarden Date: Sat, 13 Jun 2026 22:41:22 -0400 Subject: [PATCH] fix(reviewer): GC orphaned PR-review state files (state/pr_reviews) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit state/pr_reviews/-.json is unlinked by _merge_and_done and _close_and_requeue — but only when THIS watcher is the one that terminates the PR. PRs merged or closed by any other means (a manual `gh pr merge`, another host, or while the watcher was down/stale) leave their state file behind, and they accumulate indefinitely (observed: 73 files, ~41 for already-merged/closed OperationsCenter PRs going back to May). Add _prune_orphan_state_files, called from _poll_once right after a SUCCESSFUL list_open_prs: any state file for that repo whose PR number is not in the open set is for a terminated PR and is deleted. The fetch having succeeded is the guard — on a list_open_prs exception the loop already `continue`s before the sweep. A false prune (PR open but missing from a partial fetch) is self-healing: the next poll re-discovers the PR and re-creates its state. Other repos' files and non-numeric filenames are left untouched. +2 tests: mixed open/terminal/other-repo/non-numeric files → only this repo's terminal numeric files pruned; empty open set → all of that repo's files pruned. Co-Authored-By: Claude Opus 4.8 --- .console/log.md | 9 +++++ .../entrypoints/pr_review_watcher/main.py | 35 +++++++++++++++++++ tests/test_pr_review_watcher.py | 34 ++++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/.console/log.md b/.console/log.md index 84a2b9755..c4452ddf7 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,12 @@ +## 2026-06-13 — fix(reviewer): GC orphaned PR-review state files + +state/pr_reviews/-.json is unlinked by _merge_and_done / _close_and_requeue, but only when +THIS watcher terminates the PR. PRs merged/closed by other means (manual gh merge, another host, or +while the watcher was down/stale) leave their state files behind forever (observed: 73 files, ~41 for +already-terminal OC PRs from May). Added _prune_orphan_state_files, called each _poll_once after a +SUCCESSFUL list_open_prs: any state file for that repo whose PR isn't in the open set is pruned. A +false prune is self-healing (next poll re-discovers the open PR and re-creates state). +2 tests. + ## 2026-06-13 — fix(spec-hygiene): active.json projects only active campaigns (campaign GC) _rebuild_active_projection wrote every campaign — incl. complete/cancelled — to state/campaigns/ diff --git a/src/operations_center/entrypoints/pr_review_watcher/main.py b/src/operations_center/entrypoints/pr_review_watcher/main.py index 6b036b301..7f0ad1c49 100644 --- a/src/operations_center/entrypoints/pr_review_watcher/main.py +++ b/src/operations_center/entrypoints/pr_review_watcher/main.py @@ -78,6 +78,35 @@ def _state_path(oc_root: Path, repo_key: str, pr_number: int) -> Path: return oc_root / _STATE_SUBDIR / f"{_state_key(repo_key, pr_number)}.json" +def _prune_orphan_state_files(oc_root: Path, repo_key: str, open_numbers: set[int]) -> None: + """Delete review-state files for PRs that are no longer open. + + The per-merge/close unlinks in _merge_and_done / _close_and_requeue only fire + when THIS watcher terminates a PR. PRs merged or closed by any other means + (a manual ``gh pr merge``, another host, or while this watcher was down/stale) + leave their state/pr_reviews/-.json behind, and they accumulate + forever. Callers MUST pass a set built from a SUCCESSFUL list_open_prs — any + state file for this repo whose PR is not in that set is for a terminated PR. + A false prune (PR open but missing from a partial fetch) is self-healing: the + next poll re-discovers the open PR and re-creates its state. + """ + state_dir = oc_root / _STATE_SUBDIR + if not state_dir.is_dir(): + return + prefix = f"{repo_key}-" + for f in state_dir.glob(f"{repo_key}-*.json"): + if not f.stem.startswith(prefix): + continue + num_part = f.stem[len(prefix):] + if not num_part.isdigit() or int(num_part) in open_numbers: + continue + try: + f.unlink(missing_ok=True) + logger.info("pr_review_watcher: pruned orphan review-state %s (PR not open)", f.name) + except Exception as exc: + logger.debug("pr_review_watcher: prune failed for %s — %s", f.name, exc) + + def _load_state(path: Path) -> dict: try: return json.loads(path.read_text(encoding="utf-8")) @@ -2300,6 +2329,12 @@ def _poll_once(oc_root: Path, config_path: Path, settings) -> None: logger.warning("pr_review_watcher: failed to list PRs %s/%s — %s", owner, repo, exc) continue + # GC leftover review-state for PRs that terminated outside this watcher's + # merge/close path (manual merge, another host, or while it was down). + _prune_orphan_state_files( + oc_root, repo_key, {int(p["number"]) for p in open_prs if p.get("number") is not None} + ) + # Build the worklist (discover + load state) before processing, so the # sweep can be ordered. A single slow PR (a multi-pass fix battle) must # not push a merge-ready PR to the back of the sweep — or off it entirely diff --git a/tests/test_pr_review_watcher.py b/tests/test_pr_review_watcher.py index 016faef9a..2ded5d99f 100644 --- a/tests/test_pr_review_watcher.py +++ b/tests/test_pr_review_watcher.py @@ -2190,3 +2190,37 @@ def test_phase1_untruncated_diff_skips_file_list(tmp_path: Path) -> None: ) gh.list_pr_files.assert_not_called() + + +def test_prune_orphan_state_files(tmp_path: Path) -> None: + """Only state files for PRs not in the open set (and matching this repo, with a + numeric suffix) are pruned; other repos and non-numeric files are untouched.""" + sub = tmp_path / "state" / "pr_reviews" + sub.mkdir(parents=True) + for name in ( + "OperationsCenter-100.json", # open → keep + "OperationsCenter-101.json", # terminal → prune + "OperationsCenter-102.json", # terminal → prune + "OtherRepo-100.json", # different repo → untouched + "OperationsCenter-readme.json", # non-numeric → untouched + ): + (sub / name).write_text("{}", encoding="utf-8") + + watcher._prune_orphan_state_files(tmp_path, "OperationsCenter", {100}) + + remaining = {p.name for p in sub.iterdir()} + assert remaining == { + "OperationsCenter-100.json", + "OtherRepo-100.json", + "OperationsCenter-readme.json", + } + + +def test_prune_orphan_state_files_empty_open_set_prunes_all_for_repo(tmp_path: Path) -> None: + """No open PRs for the repo (all merged) → all its state files pruned.""" + sub = tmp_path / "state" / "pr_reviews" + sub.mkdir(parents=True) + (sub / "OperationsCenter-1.json").write_text("{}", encoding="utf-8") + (sub / "OperationsCenter-2.json").write_text("{}", encoding="utf-8") + watcher._prune_orphan_state_files(tmp_path, "OperationsCenter", set()) + assert list(sub.iterdir()) == []