From 8ad371f753d20c37bbda41a94adb2ef66c5d4727 Mon Sep 17 00:00:00 2001 From: tegwick Date: Thu, 26 Mar 2026 14:38:30 +0100 Subject: [PATCH] feat(consistency): fix-consistency-remote works without REPO for all repos Adds --remote CLI flag and fix_all_remote() function. When run without a REPO argument, the target checks all registered repos and: - Skips repos whose local path does not exist on this machine - Skips repos that are already clean (no fixable issues, no FAILs, not behind remote, only C-08 background noise allowed) - For repos that need work: git pull --ff-only then fix_repo() Prints a summary of CLEAN (skipped) and NOT ON THIS HOST (skipped) repos before the detailed fix reports. Simplifies the Makefile target from shell-level curl+git to a single uv run call using --remote. Same flag handles both single-repo and all-repos. Also adds _git_pull() helper and 13 new tests (71 total in consistency suite). Co-Authored-By: Claude Sonnet 4.6 --- state-hub/Makefile | 21 ++- state-hub/scripts/consistency_check.py | 171 +++++++++++++++++++--- state-hub/tests/test_consistency_check.py | 119 ++++++++++++++- 3 files changed, 276 insertions(+), 35 deletions(-) diff --git a/state-hub/Makefile b/state-hub/Makefile index d082740..58d3af2 100644 --- a/state-hub/Makefile +++ b/state-hub/Makefile @@ -252,19 +252,16 @@ fix-consistency: $(if $(REPO_PATH),--repo-path "$(REPO_PATH)",); \ e=$$?; [ $$e -eq 2 ] && exit 0 || exit $$e -## Pull repo then fix consistency (safe for multi-machine workflows): make fix-consistency-remote REPO=net-kingdom +## Pull then fix: single repo or all repos if REPO omitted +## make fix-consistency-remote — smart pull+fix all repos that need it +## make fix-consistency-remote REPO=slug — pull+fix one repo fix-consistency-remote: - @test -n "$(REPO)" || (echo "ERROR: REPO is required. Usage: make fix-consistency-remote REPO="; exit 1) - $(eval _REMOTE_REPO_PATH := $(shell \ - curl -s $${API_BASE:-http://127.0.0.1:8000}/repos/?slug=$(REPO) | \ - python3 -c "import json,sys; \ - repos=json.load(sys.stdin); \ - print(next((r.get('local_path','') for r in repos if r['slug']=='$(REPO)'), ''))" \ - )) - @test -n "$(_REMOTE_REPO_PATH)" || (echo "ERROR: repo '$(REPO)' not found or has no local_path in state-hub"; exit 1) - git -C "$(_REMOTE_REPO_PATH)" pull --ff-only || \ - (echo "WARN: pull failed (conflicts or no remote) — running fix-consistency anyway"; true) - $(MAKE) fix-consistency REPO=$(REPO) REPO_PATH=$(_REMOTE_REPO_PATH) + uv run python scripts/consistency_check.py \ + $(if $(REPO),--repo "$(REPO)",--all) \ + --remote \ + $(if $(API_BASE),--api-base "$(API_BASE)",) \ + $(if $(NO_WRITEBACK),--no-writeback,); \ + e=$$?; [ $$e -eq 2 ] && exit 0 || exit $$e ## Check all registered repos for ADR-001 consistency check-consistency-all: diff --git a/state-hub/scripts/consistency_check.py b/state-hub/scripts/consistency_check.py index 167b8de..c4b57fa 100644 --- a/state-hub/scripts/consistency_check.py +++ b/state-hub/scripts/consistency_check.py @@ -747,6 +747,25 @@ def _check_ghost_duplicates( # Git helpers (T02 pull gate, T03 writeback) # --------------------------------------------------------------------------- +def _git_pull(repo_path: str) -> tuple[bool, str]: + """Run ``git pull --ff-only`` on *repo_path*. + + Returns ``(success, message)`` where *message* describes the outcome. + Never raises — errors are returned as ``(False, "")``. + """ + try: + result = subprocess.run( + ["git", "-C", repo_path, "pull", "--ff-only"], + capture_output=True, text=True, timeout=30, + ) + if result.returncode == 0: + out = result.stdout.strip() + return True, out or "already up to date" + return False, result.stderr.strip() or "pull failed" + except Exception as exc: + return False, str(exc) + + def _detect_behind_remote(repo_path: str) -> bool: """Return True if the remote tracking branch has commits the local repo lacks. @@ -1078,6 +1097,92 @@ def fix_repo( return report +# Check IDs that are known-background noise in multi-machine setups: +# C-08 = completed/archived DB workstream with no file (pre-ADR-001 legacy) +# These alone do not warrant a pull+fix cycle. +_BACKGROUND_CHECKS: frozenset[str] = frozenset({"C-08"}) + + +def _report_needs_action(report: ConsistencyReport, behind_remote: bool) -> bool: + """Return True if the repo warrants a pull+fix cycle. + + A repo is considered clean (no action needed) when: + - It is not behind its remote tracking branch, AND + - It has no FAIL issues, AND + - Every WARN/INFO issue is in the background-noise set (C-08). + """ + if behind_remote: + return True + if report.failures: + return True + actionable_warns = [ + i for i in report.warnings + report.infos + if i.check_id not in _BACKGROUND_CHECKS + ] + return bool(actionable_warns) + + +def fix_all_remote( + api_base: str, + no_writeback: bool = False, +) -> list[ConsistencyReport]: + """Pull-then-fix all registered repos that need attention. + + For each repo with a resolvable local path on this machine: + 1. Skip if the path does not exist (repo lives on another machine). + 2. Run check_repo() and detect_behind_remote() — read-only. + 3. If the repo is clean (no actionable issues, not behind remote): skip. + 4. Otherwise: git pull --ff-only, then fix_repo(). + + Returns one ConsistencyReport per repo that was *not* skipped as clean. + Repos skipped as clean are printed to stdout but not included in the return value. + """ + repos = _api_get(api_base, "/repos") + if not isinstance(repos, list): + print("ERROR: Could not fetch repos from state-hub API", file=sys.stderr) + return [] + + reports: list[ConsistencyReport] = [] + skipped_clean: list[str] = [] + skipped_missing: list[str] = [] + + for repo in repos: + slug = repo["slug"] + # Resolve path using the same priority as check_repo + path = resolve_repo_path(repo) + if not path or not Path(path).is_dir(): + skipped_missing.append(slug) + continue + + # Read-only pass: detect issues and remote staleness + pre_report = check_repo(api_base, slug) + behind = _detect_behind_remote(path) + + if not _report_needs_action(pre_report, behind): + skipped_clean.append(slug) + continue + + # Pull before fixing + pull_ok, pull_msg = _git_pull(path) + pull_tag = f"pull: {pull_msg}" + if not pull_ok: + pull_tag = f"pull WARN: {pull_msg} — proceeding anyway" + + report = fix_repo(api_base, slug, no_writeback=no_writeback) + report.fixes_applied.insert(0, pull_tag) + reports.append(report) + + # Print clean/missing summary lines before the detailed reports + if skipped_clean: + print(f" CLEAN (skipped): {', '.join(skipped_clean)}") + if skipped_missing: + print(f" NOT ON THIS HOST (skipped): {', '.join(skipped_missing)}") + if skipped_clean or skipped_missing: + print() + + return reports + + # --------------------------------------------------------------------------- # Output / rendering # --------------------------------------------------------------------------- @@ -1172,6 +1277,10 @@ def main() -> None: help="Run checks against all registered repos with local_path") parser.add_argument("--fix", action="store_true", help="Apply auto-fixable issues (status drift, repo mismatch, etc.)") + parser.add_argument("--remote", action="store_true", + help="Pull each repo before fixing; when used with --all, skips repos " + "that are already clean (no actionable issues, not behind remote). " + "Implies --fix.") parser.add_argument("--no-writeback", action="store_true", dest="no_writeback", help="Disable DB→file status writeback (C-15) while keeping other fixes") parser.add_argument("--repo-path", metavar="PATH", default=None, @@ -1183,30 +1292,50 @@ def main() -> None: help="Output JSON instead of human-readable text") args = parser.parse_args() - # Resolve repo list - repo_slugs: list[str] = [] - if args.all: - repos = _api_get(args.api_base, "/repos") - if not isinstance(repos, list): - print("ERROR: Could not fetch repos from state-hub API", file=sys.stderr) - sys.exit(1) - repo_slugs = [r["slug"] for r in repos if r.get("local_path")] - if not repo_slugs: - print("No repos with local_path registered.", file=sys.stderr) + no_wb = getattr(args, "no_writeback", False) + do_fix = args.fix or args.remote + + # --remote --all: smart pull+fix across all repos + if args.remote and args.all: + reports = fix_all_remote(args.api_base, no_writeback=no_wb) + if not reports: sys.exit(0) else: - repo_slugs = [args.repo] + # Resolve repo list + repo_slugs: list[str] = [] + if args.all: + repos = _api_get(args.api_base, "/repos") + if not isinstance(repos, list): + print("ERROR: Could not fetch repos from state-hub API", file=sys.stderr) + sys.exit(1) + repo_slugs = [r["slug"] for r in repos if r.get("local_path")] + if not repo_slugs: + print("No repos with local_path registered.", file=sys.stderr) + sys.exit(0) + else: + repo_slugs = [args.repo] - # --repo-path only applies to single-repo runs; silently ignored with --all - path_override = args.repo_path if not args.all else None - if args.fix: - no_wb = getattr(args, "no_writeback", False) - reports = [ - fix_repo(args.api_base, slug, path_override, no_writeback=no_wb) - for slug in repo_slugs - ] - else: - reports = [check_repo(args.api_base, slug, path_override) for slug in repo_slugs] + # --repo-path only applies to single-repo runs; silently ignored with --all + path_override = args.repo_path if not args.all else None + + if args.remote: + # Single-repo remote: pull first, then fix + slug = repo_slugs[0] + repo = _api_get(args.api_base, f"/repos/{slug}") + path = resolve_repo_path(repo or {}, path_override) if repo else (path_override or "") + if path: + pull_ok, pull_msg = _git_pull(path) + prefix = "pull" if pull_ok else "pull WARN" + print(f" {prefix}: {pull_msg}") + report = fix_repo(args.api_base, slug, path_override, no_writeback=no_wb) + reports = [report] + elif do_fix: + reports = [ + fix_repo(args.api_base, slug, path_override, no_writeback=no_wb) + for slug in repo_slugs + ] + else: + reports = [check_repo(args.api_base, slug, path_override) for slug in repo_slugs] if args.as_json: output = ( diff --git a/state-hub/tests/test_consistency_check.py b/state-hub/tests/test_consistency_check.py index b368cda..b456db9 100644 --- a/state-hub/tests/test_consistency_check.py +++ b/state-hub/tests/test_consistency_check.py @@ -25,8 +25,11 @@ from consistency_check import ( Issue, FILE_TO_DB_WORKSTREAM_STATUS, STATUS_ORDER, + _BACKGROUND_CHECKS, _detect_behind_remote, + _git_pull, _patch_task_status_in_file, + _report_needs_action, get_tasks_from_workplan, normalise_workstream_status, parse_frontmatter, @@ -462,7 +465,7 @@ class TestDetectBehindRemote: ["git", "-C", str(clone), "commit", "--allow-empty", "-m", "init"], capture_output=True, ) - subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD:main"], capture_output=True) + subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD"], capture_output=True) # Add a local-only commit (not pushed) subprocess.run( ["git", "-C", str(clone), "commit", "--allow-empty", "-m", "local only"], @@ -483,7 +486,7 @@ class TestDetectBehindRemote: ["git", "-C", str(clone), "commit", "--allow-empty", "-m", "init"], capture_output=True, ) - subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD:main"], capture_output=True) + subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD"], capture_output=True) # Add a commit directly in the bare repo (simulates remote-only progress) work = tmp_path / "work" @@ -554,9 +557,121 @@ class TestPatchTaskStatusInFile: assert "id: T01\nstatus: todo" in patched assert "id: T02\nstatus: done" in patched + def test_does_not_touch_non_status_fields(self, tmp_path): + content = ( + "---\nid: WP\n---\n" + "```task\nid: T01\nstatus: todo\npriority: high\nstate_hub_task_id: \"abc\"\n```\n" + ) + f = self._make_workplan(tmp_path, content) + _patch_task_status_in_file(f, "T01", "done") + patched = f.read_text() + assert "priority: high" in patched + assert 'state_hub_task_id: "abc"' in patched + def test_idempotent_when_already_correct(self, tmp_path): content = "---\nid: WP\n---\n```task\nid: T01\nstatus: done\n```\n" f = self._make_workplan(tmp_path, content) result = _patch_task_status_in_file(f, "T01", "done") # status matches, re.sub produces same text → no write → False assert result is False + + +# --------------------------------------------------------------------------- +# _git_pull (T02 remote fix helper) +# --------------------------------------------------------------------------- + +class TestGitPull: + def test_returns_false_for_nonexistent_path(self): + ok, msg = _git_pull("/nonexistent/path") + assert ok is False + assert msg + + def test_returns_true_for_up_to_date_repo(self, tmp_path): + import subprocess + bare = tmp_path / "bare.git" + clone = tmp_path / "clone" + bare.mkdir() + subprocess.run(["git", "init", "--bare", str(bare)], capture_output=True) + subprocess.run(["git", "clone", str(bare), str(clone)], capture_output=True) + subprocess.run( + ["git", "-C", str(clone), "commit", "--allow-empty", "-m", "init"], + capture_output=True, + ) + subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD"], capture_output=True) + ok, msg = _git_pull(str(clone)) + assert ok is True + + def test_pulls_new_remote_commit(self, tmp_path): + import subprocess + bare = tmp_path / "bare.git" + clone = tmp_path / "clone" + work = tmp_path / "work" + bare.mkdir() + subprocess.run(["git", "init", "--bare", str(bare)], capture_output=True) + subprocess.run(["git", "clone", str(bare), str(clone)], capture_output=True) + subprocess.run( + ["git", "-C", str(clone), "commit", "--allow-empty", "-m", "init"], + capture_output=True, + ) + subprocess.run(["git", "-C", str(clone), "push", "origin", "HEAD"], capture_output=True) + # Push a new commit from a second clone + subprocess.run(["git", "clone", str(bare), str(work)], capture_output=True) + subprocess.run( + ["git", "-C", str(work), "commit", "--allow-empty", "-m", "remote work"], + capture_output=True, + ) + subprocess.run(["git", "-C", str(work), "push"], capture_output=True) + # Pull should succeed and bring in the new commit + ok, msg = _git_pull(str(clone)) + assert ok is True + head = subprocess.run( + ["git", "-C", str(clone), "log", "--oneline", "-1"], + capture_output=True, text=True, + ).stdout + assert "remote work" in head + + +# --------------------------------------------------------------------------- +# _report_needs_action (smart skip logic) +# --------------------------------------------------------------------------- + +class TestReportNeedsAction: + def _make_report(self, issues: list[tuple[str, str, bool]]) -> ConsistencyReport: + """Build a report from (severity, check_id, fixable) tuples.""" + r = ConsistencyReport(repo_slug="test", repo_path="/tmp/test") + for severity, check_id, fixable in issues: + r.add(severity=severity, check_id=check_id, message="test", fixable=fixable) + return r + + def test_clean_report_not_behind_is_skipped(self): + r = self._make_report([]) + assert _report_needs_action(r, behind_remote=False) is False + + def test_behind_remote_always_needs_action(self): + r = self._make_report([]) + assert _report_needs_action(r, behind_remote=True) is True + + def test_fail_always_needs_action(self): + r = self._make_report([("FAIL", "C-07", False)]) + assert _report_needs_action(r, behind_remote=False) is True + + def test_c08_only_is_background_noise(self): + """C-08 (legacy archived workstream) alone should not trigger a fix cycle.""" + r = self._make_report([("INFO", "C-08", False)]) + assert _report_needs_action(r, behind_remote=False) is False + + def test_c08_plus_actionable_warn_needs_action(self): + r = self._make_report([("INFO", "C-08", False), ("WARN", "C-10", True)]) + assert _report_needs_action(r, behind_remote=False) is True + + def test_fixable_warn_needs_action(self): + r = self._make_report([("WARN", "C-15", True)]) + assert _report_needs_action(r, behind_remote=False) is True + + def test_non_fixable_warn_needs_action(self): + """Non-fixable WARNs (e.g. C-07 ghost) still warrant attention.""" + r = self._make_report([("WARN", "C-14", False)]) + assert _report_needs_action(r, behind_remote=False) is True + + def test_background_checks_constant_contains_c08(self): + assert "C-08" in _BACKGROUND_CHECKS