diff --git a/AGENTS.md b/AGENTS.md index 72c9e1c..1fcfa2e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -635,6 +635,17 @@ no push, no thread, no behaviour change at all. - **Offline is quiet.** An unreachable origin says so once, then works locally; commits queue on `main` and go out on the next reachable fetch. +- **Which remote is one answer, not two.** One remote and one branch, by + design — but the remote is the one `BOARD_GIT_REMOTE` names, else the + checkout's first, resolved in the same place PR opening asks, so the two + halves of team mode can never publish to different places. It is used as + named: a `BOARD_GIT_REMOTE` this checkout has no remote for stalls + saying so rather than reaching past it for another one. A checkout with + no remote at all stalls the same way, from startup — team mode syncing + nothing is exactly the state a board must not render as healthy, and it + is the likeliest first state of a fresh installation. Add a remote (or + set the setting) and the chip clears with a line saying sync is + converging again. ### State syncs; reactions don't diff --git a/manager/core/.env.example b/manager/core/.env.example index 197d40b..1a2385e 100644 --- a/manager/core/.env.example +++ b/manager/core/.env.example @@ -58,8 +58,10 @@ BOARD_AGENT_COMMANDS=python3 -m unittest BOARD_WORKTREES=.worktrees # GitHub plumbing. The gh CLI used for PRs (stub-able like claude); the git -# remote PRs are pushed to (empty = the repo's first remote); and how often -# to poll open PRs of cards sitting in review/ for reviews and checks. +# remote this board's work goes to (empty = the repo's first remote) — PRs +# and BOARD_SYNC both ride it, and a name set here is used as named, never +# swapped for another remote; and how often to poll open PRs of cards +# sitting in review/ for reviews and checks. BOARD_GH_BIN=gh BOARD_GIT_REMOTE= BOARD_PR_POLL_INTERVAL=60 @@ -114,7 +116,11 @@ BOARD_COMMIT_MOVES= # ever fast-forwards. Whoever clicks needs merge rights on the repo. # # One board fetches twice a minute at the default interval; raise it on a -# rate-limited or metered remote. Sync rides `origin` and `main` only. +# rate-limited or metered remote. Sync rides one remote and `main` only — +# the remote BOARD_GIT_REMOTE names, or the checkout's first one, exactly +# as PRs do. Nothing to ride (no remote, or a BOARD_GIT_REMOTE naming one +# this checkout does not have) is a `sync stalled` chip and a ticker line, +# never a board that looks healthy while syncing nothing. BOARD_SYNC= BOARD_SYNC_INTERVAL=30 diff --git a/manager/core/sync.py b/manager/core/sync.py index 587937f..1143cab 100644 --- a/manager/core/sync.py +++ b/manager/core/sync.py @@ -1,5 +1,12 @@ -"""Boards converge through origin/main: push what this board commits, pull -what the other boards published. +"""Boards converge through the remote's main: push what this board commits, +pull what the other boards published. + +One remote and one branch, by design — but *which* remote is `config`'s +answer (`BOARD_GIT_REMOTE`, else the checkout's first remote), the same +one PR opening already asks for, so the two halves of team mode can never +publish to different places. A checkout with no remote to sync with, or a +`BOARD_GIT_REMOTE` naming one it does not have, is not a quiet no-op: it +is narrated and held on the header like every other condition here. Gated on `BOARD_SYNC` (which implies `BOARD_COMMIT_MOVES` — a move that never commits has nothing to publish). Off, nothing here runs: no fetch, @@ -38,9 +45,8 @@ import config import state from taskfiles import NUMBER_RE -REMOTE = "origin" # one remote, one branch — by design -BRANCH = "main" -UPSTREAM = f"{REMOTE}/{BRANCH}" +BRANCH = "main" # one remote, one branch — by design; + # the remote is resolved, not assumed BOARD_COMMIT = "board: " # the prefix taskfiles messages its own commits with PUSH_TIMEOUT = 120 REBASE_TIMEOUT = 120 @@ -98,8 +104,38 @@ def _clear(key: str, recovery: str = "") -> None: # ── the checkout ─────────────────────────────────────────────────────── -def _origin_present() -> bool: - return REMOTE in _git("remote").stdout.split() +def _upstream(remote: str) -> str: + return f"{remote}/{BRANCH}" + + +def _remote() -> str | None: + """The remote this board syncs through, or None with the reason said. + + Resolved in `config`, verified here: a `BOARD_GIT_REMOTE` naming a + remote this checkout does not have is a typo, and reaching past it for + another one would be exactly the silence this narration exists to end. + Team mode with nothing to sync with is a stalled board, not a no-op — + it is the likeliest first state of a fresh installation, and the header + has to say so. + """ + names = config.git_remotes() + name = config.git_remote() + if name and name in names: + _clear("no-remote", + f"sync is converging again: this board rides {_upstream(name)}") + return name + if name: + have = (f"this checkout has {', '.join(names)}" if names + else "this checkout has no remotes at all") + _note("no-remote", f"sync stalled: BOARD_GIT_REMOTE names '{name}' but {have} " + f"— add that remote, or set BOARD_GIT_REMOTE to one that " + f"exists (sync will not pick another for you)") + else: + _note("no-remote", "sync stalled: BOARD_SYNC is on and this checkout has no " + "remote to sync through — add one (git remote add origin " + "), or set BOARD_GIT_REMOTE to the remote this board " + "should ride") + return None def _head() -> str: @@ -128,29 +164,29 @@ def _tasks_prefix() -> str: return "tasks/" -def _fetch() -> bool: - result = _git("fetch", REMOTE, BRANCH, timeout=config.FETCH_TIMEOUT) +def _fetch(remote: str) -> bool: + result = _git("fetch", remote, BRANCH, timeout=config.FETCH_TIMEOUT) if result.returncode != 0: if "couldn't find remote ref" in (result.stderr or "").lower(): - _note("no-branch", f"sync stalled: {REMOTE} has no {BRANCH} branch — " - f"sync rides {UPSTREAM} and nothing else") + _note("no-branch", f"sync stalled: {remote} has no {BRANCH} branch — " + f"sync rides {_upstream(remote)} and nothing else") return False _note("offline", - f"sync is behind: {REMOTE} is unreachable — this board keeps " + f"sync is behind: {remote} is unreachable — this board keeps " f"working locally and catches up when it returns", "offline") return False _clear("no-branch") - _clear("offline", f"sync caught up: {REMOTE} is reachable again") + _clear("offline", f"sync caught up: {remote} is reachable again") return True # ── publishing ───────────────────────────────────────────────────────── -def _ahead() -> list[str]: - """` ` for every commit local main has and - origin/main does not — newest first.""" - out = _git("log", "--format=%h %s", f"{UPSTREAM}..{BRANCH}").stdout +def _ahead(remote: str) -> list[str]: + """` ` for every commit local main has and the + remote's main does not — newest first.""" + out = _git("log", "--format=%h %s", f"{_upstream(remote)}..{BRANCH}").stdout return [line for line in out.splitlines() if line.strip()] @@ -165,20 +201,21 @@ def _stray(commits: list[str]) -> str: return "" -def _publish() -> str: +def _publish(remote: str) -> str: """Push local main if — and only if — everything on it is the board's. ok | nothing | stray | not-on-main | retry | offline | stalled """ + upstream = _upstream(remote) if not _on_main(): branch = _git("rev-parse", "--abbrev-ref", "HEAD").stdout.strip() or "a detached HEAD" _note("branch", f"sync paused: this checkout is on '{branch}', not {BRANCH} — " f"board commits are not landing where sync publishes from") return "not-on-main" _clear("branch") - if _git("rev-parse", "--verify", "--quiet", UPSTREAM).returncode != 0: + if _git("rev-parse", "--verify", "--quiet", upstream).returncode != 0: return "retry" # never fetched: converge first, then publish - commits = _ahead() + commits = _ahead(remote) stray = _stray(commits) if stray: _note("stray", f"not pushing: {stray} is not a board commit — sync will not " @@ -189,25 +226,25 @@ def _publish() -> str: if not commits: return "nothing" - result = _git("push", REMOTE, f"{BRANCH}:{BRANCH}", timeout=PUSH_TIMEOUT) + result = _git("push", remote, f"{BRANCH}:{BRANCH}", timeout=PUSH_TIMEOUT) if result.returncode == 0: _clear("push") - _clear("offline", f"sync caught up: {REMOTE} is reachable again") + _clear("offline", f"sync caught up: {remote} is reachable again") state.record_board_event({ "kind": "sync", "actor": "sync", "summary": f"pushed {len(commits)} board commit" - f"{'s' if len(commits) > 1 else ''} to {UPSTREAM}"}) + f"{'s' if len(commits) > 1 else ''} to {upstream}"}) return "ok" stderr = (result.stderr or result.stdout).strip() if _rejected(stderr): return "retry" if _unreachable(stderr): _note("offline", - f"sync is behind: {REMOTE} is unreachable — this board keeps " + f"sync is behind: {remote} is unreachable — this board keeps " f"working locally and catches up when it returns", "offline") return "offline" detail = stderr.splitlines()[-1][:140] if stderr else "git said nothing" - _note("push", f"sync could not push to {UPSTREAM}: {detail}") + _note("push", f"sync could not push to {upstream}: {detail}") return "stalled" @@ -248,10 +285,10 @@ def _number(filename: str) -> str: return match.group(1) if match else filename[:-3] if filename.endswith(".md") else filename -def _lost(filename: str) -> None: +def _lost(filename: str, remote: str) -> None: """The local move lost the race. Say who took the card — the file itself - reverts to origin's version when the rebase drops our commit.""" - who = _author_of(filename, UPSTREAM) or "someone else" + reverts to the remote's version when the rebase drops our commit.""" + who = _author_of(filename, _upstream(remote)) or "someone else" message = f"{_number(filename)} claimed by {who} — your move was undone" state.record_board_event({"kind": "sync", "actor": "sync", "file": filename, "summary": message}) @@ -259,21 +296,23 @@ def _lost(filename: str) -> None: state.broadcast({"type": "board"}) -def _replay() -> str: - """Rebase this board's commits onto origin/main. Conflicts on a task - file are resolved by dropping our commit: origin is the linearizer, and - a card someone else moved first is theirs. Anything conflicting outside - tasks/ is a real collision — abort and wait for a human. +def _replay(remote: str) -> str: + """Rebase this board's commits onto the remote's main. Conflicts on a + task file are resolved by dropping our commit: the remote is the + linearizer, and a card someone else moved first is theirs. Anything + conflicting outside tasks/ is a real collision — abort and wait for a + human. ok | dirty | stalled """ + upstream = _upstream(remote) if not _clean(): _note("dirty", "sync paused: main has uncommitted changes — commit or stash " "them and sync resumes (code work belongs in a worktree)") return "dirty" _clear("dirty") - result = _git("rebase", UPSTREAM, timeout=REBASE_TIMEOUT) + result = _git("rebase", upstream, timeout=REBASE_TIMEOUT) for _ in range(50): # bounded: one round per replayed commit if result.returncode == 0: _clear("replay") @@ -282,46 +321,47 @@ def _replay() -> str: if not conflicted or not all(_is_task_file(p) for p in conflicted): _git("rebase", "--abort") detail = ", ".join(conflicted[:3]) or (result.stderr or result.stdout).strip()[-140:] - _note("replay", f"sync stalled: replaying this board's commits onto {UPSTREAM} " + _note("replay", f"sync stalled: replaying this board's commits onto {upstream} " f"collides outside tasks/ ({detail}) — a human has to settle it") return "stalled" for name in dict.fromkeys(Path(p).name for p in conflicted): - _lost(name) + _lost(name, remote) result = _git("rebase", "--skip", timeout=REBASE_TIMEOUT) _git("rebase", "--abort") - _note("replay", f"sync stalled: replaying onto {UPSTREAM} did not settle — " + _note("replay", f"sync stalled: replaying onto {upstream} did not settle — " f"a human has to settle it") return "stalled" -def _integrate() -> str: - """Bring local main to origin/main without ever merging past a +def _integrate(remote: str) -> str: + """Bring local main to the remote's main without ever merging past a divergence. up-to-date | pulled | not-on-main | dirty | diverged | stalled """ - if _count(f"{BRANCH}..{UPSTREAM}") == 0: + upstream = _upstream(remote) + if _count(f"{BRANCH}..{upstream}") == 0: return "up-to-date" if not _on_main(): branch = _git("rev-parse", "--abbrev-ref", "HEAD").stdout.strip() or "a detached HEAD" _note("branch", f"sync paused: this checkout is on '{branch}', not {BRANCH} — " - f"switch back and the board catches up with {UPSTREAM}") + f"switch back and the board catches up with {upstream}") return "not-on-main" _clear("branch") # Diverged. The board's own bookkeeping can be replayed on top of what # arrived — that is how a lost race resolves. A human's commit cannot, # and the guard that refuses to push it refuses to rebase it too. - commits = _ahead() + commits = _ahead(remote) stray = _stray(commits) if stray: - _note("diverged", f"sync stalled: main and {UPSTREAM} have diverged and " + _note("diverged", f"sync stalled: main and {upstream} have diverged and " f"{stray} is not a board commit — pull or rebase it by " f"hand, and this board starts converging again") return "diverged" _clear("diverged") if commits: - outcome = _replay() + outcome = _replay(remote) return "pulled" if outcome == "ok" else outcome if not _clean(): @@ -329,17 +369,17 @@ def _integrate() -> str: "them and sync resumes (code work belongs in a worktree)") return "dirty" _clear("dirty") - result = _git("merge", "--ff-only", UPSTREAM, timeout=REBASE_TIMEOUT) + result = _git("merge", "--ff-only", upstream, timeout=REBASE_TIMEOUT) if result.returncode != 0: detail = (result.stderr or result.stdout).strip().splitlines() - _note("merge", f"sync stalled: fast-forwarding to {UPSTREAM} failed " + _note("merge", f"sync stalled: fast-forwarding to {upstream} failed " f"({detail[-1][:140] if detail else 'no detail'})") return "stalled" _clear("merge") return "pulled" -def _record_arrivals(before: str) -> None: +def _record_arrivals(before: str, remote: str) -> None: """Attribute what the pull brought: each task file it touched is filed under the name of whoever committed it, for the watcher to use instead of "disk" when the move surfaces on the next poll.""" @@ -364,7 +404,7 @@ def _record_arrivals(before: str) -> None: count = _count(rng) state.record_board_event({ "kind": "sync", "actor": "sync", - "summary": f"pulled {count} commit{'s' if count != 1 else ''} from {UPSTREAM}" + "summary": f"pulled {count} commit{'s' if count != 1 else ''} from {_upstream(remote)}" + (f" ({', '.join(sorted(authors))})" if authors else "")}) state.broadcast({"type": "board"}) @@ -385,15 +425,16 @@ def arrived_actor(filename: str) -> str: def _converge() -> str: """One full beat: fetch, integrate what arrived, publish what is ours.""" - if not _origin_present(): - return "no-origin" - if not _fetch(): + remote = _remote() + if remote is None: + return "no-remote" + if not _fetch(remote): return "offline" before = _head() - outcome = _integrate() - _record_arrivals(before) + outcome = _integrate(remote) + _record_arrivals(before, remote) if outcome in ("up-to-date", "pulled"): - published = _publish() + published = _publish(remote) if published in ("stray", "offline", "stalled", "not-on-main"): return published return outcome @@ -406,9 +447,10 @@ def push_now() -> str: if not config.SYNC: return "off" with _LOCK: - if not _origin_present(): - return "no-origin" - outcome = _publish() + remote = _remote() + if remote is None: + return "no-remote" + outcome = _publish(remote) if outcome != "retry": return outcome return _converge() @@ -432,9 +474,17 @@ def on_commit(filename: str) -> None: def install() -> None: - """Wire the push hook. Called once at startup, only with the gate on.""" + """Wire the push hook, and answer the remote question straight away. + + Called once at startup, only with the gate on. The resolution runs here + rather than waiting for the first beat because "there is nothing to + sync with" is true before any converge, and the header has to carry it + from first paint — a board that never started syncing must not look + like one that is.""" if on_commit not in state.COMMIT_HOOKS: state.COMMIT_HOOKS.append(on_commit) + if config.SYNC: + _remote() def beat(interval: float | None = None) -> None: diff --git a/tests/test_boards_sync.py b/tests/test_boards_sync.py index 2621f3f..f26d43c 100644 --- a/tests/test_boards_sync.py +++ b/tests/test_boards_sync.py @@ -24,6 +24,7 @@ REPO = Path(__file__).resolve().parents[1] sys.path.insert(0, str(REPO / "manager" / "core")) import config # noqa: E402 +import github # noqa: E402 import state # noqa: E402 import sync # noqa: E402 import taskfiles # noqa: E402 @@ -388,14 +389,113 @@ class TwoBoards(unittest.TestCase): sync.pull_now() self.assertEqual(self.stage_of(self.elena), "to-do") - def test_no_origin_at_all_is_simply_nothing_to_do(self): + # — which remote this board rides (task 37) — + + def test_a_remote_named_otherwise_syncs_exactly_the_same(self): + """The bug this card came from: sync hardcoded `origin`, so a + checkout whose remote is called anything else synced nothing at + all — silently, with a healthy header.""" + self.use(self.ada) + git(self.ada, "remote", "rename", "origin", "upstream") + + self.move(self.ada, "backlog", "to-do") + self.assertEqual(sync.push_now(), "ok") + self.assertEqual(self.origin_head(), self.head(self.ada)) + self.assertEqual(sync.status()["state"], "ok") + self.assertTrue(any("upstream/main" in s for s in self.summaries()), + "the ticker names the remote it actually rode") + + self.use(self.elena) + self.assertEqual(sync.pull_now(), "pulled") + self.assertEqual(self.stage_of(self.elena), "to-do") + + def test_the_configured_remote_wins_over_the_first_one_listed(self): + """BOARD_GIT_REMOTE is what PRs already honoured; sync honours the + same answer, so the two halves of team mode cannot disagree about + where this board's work goes.""" + self.use(self.ada) + git(self.ada, "remote", "rename", "origin", "fork") + git(self.ada, "remote", "add", "backup", str(self.tmp / "elsewhere.git")) + self.patch(GIT_REMOTE="fork") + + self.assertEqual(config.git_remotes()[0], "backup", + "auto-detection alone would pick the wrong one here") + self.assertEqual(github.remote(), "fork") + + self.move(self.ada, "backlog", "to-do") + self.assertEqual(sync.push_now(), "ok") + self.assertEqual(self.origin_head(), self.head(self.ada)) + + def test_no_remote_at_all_stalls_the_board_and_names_both_fixes(self): self.use(self.ada) git(self.ada, "remote", "remove", "origin") self.move(self.ada, "backlog", "to-do") - self.assertEqual(sync.push_now(), "no-origin") - self.assertEqual(sync.pull_now(), "no-origin") + self.assertEqual(sync.push_now(), "no-remote") + self.assertEqual(sync.pull_now(), "no-remote") + + self.assertEqual(sync.status()["state"], "stalled") + stalled = [s for s in self.summaries() if "no remote to sync through" in s] + self.assertEqual(len(stalled), 1, "narrated once, not once per beat") + self.assertIn("git remote add", stalled[0]) + self.assertIn("BOARD_GIT_REMOTE", stalled[0]) + self.assertIn(stalled[0], sync.status()["detail"]) + + def test_the_stall_clears_when_a_remote_appears(self): + self.use(self.ada) + git(self.ada, "remote", "remove", "origin") + self.move(self.ada, "backlog", "to-do") + self.assertEqual(sync.push_now(), "no-remote") + + git(self.ada, "remote", "add", "origin", str(self.origin)) + + # never fetched from it, so this is the full converge: fetch, then push + self.assertEqual(sync.push_now(), "up-to-date") + self.assertEqual(sync.status()["state"], "ok") + self.assertTrue(any("converging again" in s for s in self.summaries()), + "the ticker closes the loop, as the offline path does") + self.assertEqual(self.origin_head(), self.head(self.ada)) + + def test_a_named_remote_that_does_not_exist_stalls_naming_it(self): + """A typo in BOARD_GIT_REMOTE is likelier than no remote at all, and + quietly using origin instead would hide it.""" + self.use(self.ada) + self.patch(GIT_REMOTE="typo") + before = self.origin_head() + self.move(self.ada, "backlog", "to-do") + + self.assertEqual(sync.push_now(), "no-remote") + self.assertEqual(self.origin_head(), before, "origin was not used behind our back") + + self.assertEqual(sync.status()["state"], "stalled") + stalled = [s for s in self.summaries() if "BOARD_GIT_REMOTE names 'typo'" in s] + self.assertEqual(len(stalled), 1) + self.assertIn("origin", stalled[0], "it says which remotes this checkout has") + + def test_the_missing_remote_is_on_the_header_from_startup(self): + """Not on the second beat: the condition is true before the first + converge, and a board that never started syncing must not render + like one that is.""" + self.use(self.ada) + git(self.ada, "remote", "remove", "origin") + + sync.install() + + self.assertEqual(sync.status()["state"], "stalled") + self.assertEqual(len([s for s in self.summaries() + if "no remote to sync through" in s]), 1) + + def test_the_gate_off_says_nothing_about_a_missing_remote(self): + self.patch(SYNC=False) + self.use(self.ada) + git(self.ada, "remote", "remove", "origin") + + sync.install() + self.assertEqual(sync.push_now(), "off") + self.assertEqual(sync.pull_now(), "off") + self.assertEqual(self.summaries(), []) + self.assertEqual(sync.status(), {"enabled": False, "state": "off", "detail": ""}) # — never pulling into a checkout that is not ready — @@ -546,6 +646,41 @@ class TheStrayCommitTest(unittest.TestCase): "abc fix the board: really") +class TheRemoteResolver(unittest.TestCase): + """One answer to "which remote is this board's", in config — the module + that already owns the setting. Resolved on demand: config is imported + everywhere and must not shell out at import.""" + + def setUp(self): + self.tmp = Path(tempfile.mkdtemp(prefix="bench-remote-")).resolve() + self.addCleanup(shutil.rmtree, self.tmp, True) + subprocess.run(["git", "init", "-q", "-b", "main", str(self.tmp)], + check=True, capture_output=True) + for attr in ("REPO", "GIT_REMOTE"): + self.addCleanup(setattr, config, attr, getattr(config, attr)) + config.REPO, config.GIT_REMOTE = self.tmp, "" + + def test_a_checkout_with_no_remotes_resolves_to_nothing(self): + self.assertEqual(config.git_remotes(), []) + self.assertIsNone(config.git_remote()) + + def test_the_first_remote_when_the_setting_is_empty(self): + git(self.tmp, "remote", "add", "upstream", "https://example.invalid/x.git") + self.assertEqual(config.git_remotes(), ["upstream"]) + self.assertEqual(config.git_remote(), "upstream") + + def test_the_setting_wins_and_is_taken_exactly_as_named(self): + git(self.tmp, "remote", "add", "origin", "https://example.invalid/x.git") + config.GIT_REMOTE = "fork" + self.assertEqual(config.git_remote(), "fork", + "a configured name is never swapped for another") + + def test_somewhere_that_is_not_a_repo_answers_without_raising(self): + config.REPO = self.tmp / "not-a-checkout" + self.assertEqual(config.git_remotes(), []) + self.assertIsNone(config.git_remote()) + + class TheSyncChip(unittest.TestCase): """board.html is a single file with no frontend runner — these are the source-level invariants of the surface this card adds."""