diff --git a/AGENTS.md b/AGENTS.md index 62e1689..2a02516 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -530,8 +530,10 @@ changes), **just move the card** (branch, PR and worktree stay), or branch into main, push (which marks the PR merged) and delete the remote branch, remove the worktree and local branch, then move the card. Every step narrates in the ticker; a merge conflict aborts cleanly and the card -stays put. Cards without work move silently, and hand-moves on disk are -never intercepted — the board only asks when you act through it. +stays put. On a phase card the same action also finishes the cards the +phase merged — see "Finishing a phase finishes its cards". Cards without +work move silently, and hand-moves on disk are never intercepted — the +board only asks when you act through it. That work takes as long as it takes, so **the card wears it** rather than sitting there looking idle while its branch is disassembled: from the @@ -975,6 +977,51 @@ stage — and, while a phase is in flight, the runner's own reading of each (merged in, working, checking, stopped here) — so the card answers "where is this up to" without a hunt across five columns. +### Finishing a phase finishes its cards + +A member stops at `review/` on purpose: `done/` has always meant *merged +into `main`*, and merged into a phase branch is not that. **Merge & clean +up** on the phase card is the moment it becomes that. The phase's PR goes +into `main`, so every card the phase carried is in `main` too — and since +a phase reaching `done/` releases its members back onto the Board, the +alternative is handing back three or five cards you have already judged, +in the column whose note is "your move". + +So the merge sweeps. After it has actually succeeded — never before, and +never if it conflicts — every member the phase merged moves to `done/`, +and each one's workspace is cleared exactly as completing an ordinary card +clears its own: the worktree removed, the local branch deleted, the branch +on the remote deleted. That half is not cosmetic. A stale worktree is a +trap laid for whoever reopens the card, since a work launch refuses a card +whose worktree already exists, and the accumulation is per member per +phase. + +Four rules keep it honest: + +- **Only what the phase merged.** A member is swept when its card settled + into `review/` or `done/` *and* its branch is contained in the phase + branch — the same pair the runner reads a member as `merged` by, and a + member with no branch at all was finished before the phase reached it. + One that halted, was held or was walked back is neither moved nor + cleared: there is work in its worktree and its branch, and it is the + reason a person will look at this phase afterwards. +- **Nothing uncommitted is thrown away.** A member's worktree comes out + without `--force`, so one with uncommitted changes in it is reported in + the ticker and kept, with its branch. Removing a worktree with work in + it is the one unrecoverable step in the ending. +- **One ending, told once.** The cards move together in a single commit + naming all of them (`board: 47, 52 → done with phase 53`), and the + ticker gets one line — the phase, how many cards went with it, and how + many it never merged and left where they are — instead of five moves + scrolling past. In team mode that commit publishes like any other. +- **Only merging sweeps.** "Just move the card" leaves the work alone, + including the members'. Archiving the phase card, or dropping a number + from its `## Cards`, releases the members to the Board in whatever stage + they are genuinely in (that is card 56's rule, and it is unchanged) — + neither one says anything is done, because neither one puts anything in + `main`. The members' own PRs are closed by their own merges into the + phase branch and are not touched here. + ### A phase's members leave the Board view The Board is the work you are personally holding, in five columns that @@ -990,9 +1037,12 @@ Membership is the only thing that hides a card, so *removing* membership is the un-hiding, with no sweep and no second rule: archive the phase card, drop a number from its `## Cards` list, or let the phase reach `done/` — it holds nothing once it is over — and its former members are -back in the columns they are genuinely in. A membership that did not -resolve hides nothing either: a card wearing `phase drift` stays on the -board, because an authoring mistake must not make work vanish. +back in the columns they are genuinely in. Which, when the phase got +there by being merged, is `done/`: the merge moved them (see "Finishing a +phase finishes its cards"). The other two endings move nothing, so the +cards they release are wherever they actually were. A membership that did +not resolve hides nothing either: a card wearing `phase drift` stays on +the board, because an authoring mistake must not make work vanish. That leaves the phase card standing for all of it, so it carries the summary it owes: a `⟶ 1 of 2 merged` chip in the footer row that opens diff --git a/manager/core/board.html b/manager/core/board.html index 7f36b6d..2bc83bd 100644 --- a/manager/core/board.html +++ b/manager/core/board.html @@ -2002,6 +2002,11 @@ function closeSheet() { $('#sheetwrap').classList.remove('open'); $('#sheetwrap' function completeSheet(task, from) { const d = S.state.drive; const driving = d && d.task === task.file && ['starting', 'up'].includes(d.status); + // A phase card stands for cards the Board does not draw, so the sheet + // has to say what merging it does to them: the ones it merged go to + // done/ with it and their workspaces go too. Counted the way the lane + // counts them, from the runner's reading and the phase's own log. + const swept = task.isPhase ? mergedIn(task).size : 0; const wrap = $('#sheetwrap'); wrap.innerHTML = `
` + @@ -2010,13 +2015,15 @@ function completeSheet(task, from) { `

This task has ${task.pr ? 'a PR and ' : ''}a branch with work on it. What should happen?

` + `
` + `` + - `` + + `` + // team mode merges on origin: the board never makes a merge commit of // its own, so every replica's main keeps fast-forwarding `` + `
`; wrap.classList.add('open'); @@ -2032,7 +2039,12 @@ function completeSheet(task, from) { }); const data = await res.json(); if (!res.ok) { toast(data.error || 'completion failed — the card stays', true); } - else toast(data.merged ? `${task.file} merged and cleaned up` : `${task.file} moved to done`); + else { + const went = (data.swept || []).length; + toast(data.merged ? `${task.file} merged and cleaned up` + + (went ? ` — ${went} card${went === 1 ? '' : 's'} went to done/ with it` : '') + : `${task.file} moved to done`); + } await loadState(); }); } diff --git a/manager/core/github.py b/manager/core/github.py index b17d105..5dacbed 100644 --- a/manager/core/github.py +++ b/manager/core/github.py @@ -28,7 +28,7 @@ import drive as drive_mod import reports import state from taskfiles import (STATUS_RE, collect, commit_edit, find_stage_of, - move_task, read_task) + member_entry, move_task, move_together, read_task) PR_STATE: dict[str, dict] = {} # filename -> {verdict, detail, url, ts} _OPENING: set[str] = set() # filenames with a PR-open in flight @@ -53,6 +53,13 @@ def _branch_exists(branch: str) -> bool: return _run(["git", "rev-parse", "--verify", "--quiet", branch]).returncode == 0 +def _contains(branch: str, tip: str) -> bool: + """Is `branch` already in `tip`? The same question phases.py asks of a + member — a merge leaves no record but this one, and it is the only + thing that can say a phase's ending speaks for a particular card.""" + return _run(["git", "merge-base", "--is-ancestor", branch, tip]).returncode == 0 + + def branch_of(filename: str) -> str: """The branch a card's work lives on. Ordinary cards get `task/`; a phase runs on `phase/`, its own integration branch (phases.py), @@ -493,6 +500,11 @@ def _complete(filename: str, stage: str) -> dict: """The steps themselves, run under the claim complete_task holds.""" stem = filename[:-3] branch = branch_of(filename) # a phase's own branch, or task/ + task = _woven(filename, stage) + # Read before anything is destroyed and acted on only after the merge + # lands: the phase branch is deleted in the middle of this, and it is + # the only thing that can say which members it carried. + brought = _brought(task, branch) if task.get("isPhase") else [] # 1. the app must not keep running code that is about to be merged away d = drive_mod.DRIVE @@ -504,6 +516,7 @@ def _complete(filename: str, stage: str) -> dict: time.sleep(0.5) merged = False + swept: list[dict] = [] if _branch_exists(branch): if config.SYNC: _merge_on_origin(filename, stage, branch) @@ -522,9 +535,129 @@ def _complete(filename: str, stage: str) -> dict: "kind": "agent", "actor": "board", "file": filename, "summary": f"cleaned up: worktree and local branch for {stem} removed"}) + # the merge succeeded, so the members' work is in main too + swept = _sweep(filename, task, brought) + move_task(filename, stage, "done", actor="you") state.broadcast({"type": "board"}) - return {"merged": merged} + return {"merged": merged, "swept": [card["file"] for card in swept]} + + +def _brought(task: dict, phase_branch: str) -> list[dict]: + """The members a phase actually merged — the only cards its ending + speaks for. + + Two things have to be true, and they are the same pair phases.py reads + a member as `merged` by: the card settled into `review/` or `done/`, + and its branch is contained in the phase branch — or there was never a + branch to bring, which is a member finished before the phase reached + it. Containment alone is not enough, for the reason it is not enough + there either: a run that exits without committing leaves an empty + branch that is trivially contained. + + A member that halted, was held or was walked back satisfies neither. + Its card stays where it is and so do its worktree and its branch: + there is work in them, and it is the reason a person will look at this + phase afterwards. Removing a worktree with work in it is the one + unrecoverable thing here. + """ + brought = [] + for member in task.get("members") or []: + if member.get("stage") not in ("review", "done"): + continue + branch = f"task/{member['file'][:-3]}" + if _branch_exists(branch) and not _contains(branch, phase_branch): + continue + brought.append(member) + return brought + + +def _clear_member(member: dict) -> str: + """One merged member's workspace, cleared exactly as completing an + ordinary card clears its own: the worktree, the local branch, the + branch on the remote. Returns what to say when something was kept. + + The worktree comes out without `--force`, which is the whole + difference from the card's own cleanup above. A member's work is in + `main` by the time this runs, so nothing in there is *needed* — but + that is a judgement about committed files, and an uncommitted change + is exactly what it does not cover. A dirty worktree is reported and + kept, with its branch, rather than forced: this is the one + unrecoverable step in the ending, and it is asked before it is taken + rather than left to git to refuse. + """ + stem = member["file"][:-3] + branch = f"task/{stem}" + worktree = config.WORKTREES / stem + try: + if worktree.exists(): + dirty = _run(["git", "-C", str(worktree), "status", + "--porcelain"]).stdout.split("\n") + dirty = [line for line in dirty if line.strip()] + if dirty: + return (f"{stem}: worktree kept — {len(dirty)} uncommitted " + f"change{'' if len(dirty) == 1 else 's'} in it") + removed = _run(["git", "worktree", "remove", str(worktree)]) + if removed.returncode != 0: + detail = (removed.stderr.strip() or removed.stdout.strip() + or "git would not remove it") + return f"{stem}: worktree kept — {detail.splitlines()[-1][:120]}" + if _branch_exists(branch): + # -D under sync for the same reason the card's own branch takes + # it: main here has not caught up with the merge origin made. + _run(["git", "branch", "-D" if config.SYNC else "-d", branch]) + rname = remote() + if rname: + _run(["git", "push", rname, "--delete", branch], timeout=60) + except (subprocess.SubprocessError, OSError) as exc: + return f"{stem}: workspace kept — {str(exc)[:120]}" + PR_STATE.pop(member["file"], None) + return "" + + +def _sweep(filename: str, task: dict, brought: list[dict]) -> list[dict]: + """Finish the cards the phase finished. + + A member stops at `review/` on purpose — `done/` has always meant + merged into `main`, and merged into a phase branch is not that. This + is the moment it becomes that: the phase's PR is in `main`, so every + card it carried is too, and leaving them in the column whose note is + "your move" would hand back a pile of work already judged. + + One ending, so it is told as one: the cards move together in a single + commit (`taskfiles.move_together`) and the ticker gets one line naming + the phase and how many went with it, rather than five moves scrolling + past. What was *not* swept is named too — a phase that left cards + behind is a phase somebody still has to look at. + """ + members = task.get("members") or [] + if not members: # a phase card listing nobody + return [] + kept = [note for note in (_clear_member(member) for member in brought) if note] + moved = move_together([(member["file"], member["stage"]) for member in brought + if member["stage"] != "done"], + "done", f"with phase {task['number'] or filename[:-3]}") + left = len(members) - len(brought) + name = (member_entry(task["number"], task["title"]) if task["number"] + else task["title"]) + summary = f"phase {name} finished — " + summary += (f"{_cards(len(brought))} merged into it went to done/, worktrees " + f"and branches cleaned up" if brought + else "it had merged nothing, so no card went with it") + if left: + summary += (f" · {_cards(left)} it never merged " + + ("stays where it is" if left == 1 else "stay where they are")) + state.record_board_event({"kind": "phase", "actor": "board", + "file": filename, "summary": summary}) + for note in kept: + state.record_board_event({ + "kind": "agent", "actor": "board", "file": filename, + "summary": f"{note} — nothing uncommitted is thrown away"}) + return moved + + +def _cards(count: int) -> str: + return f"{count} card{'' if count == 1 else 's'}" def _merge_locally(filename: str, branch: str) -> None: diff --git a/manager/core/phases.py b/manager/core/phases.py index 9e43b8d..1dfba4b 100644 --- a/manager/core/phases.py +++ b/manager/core/phases.py @@ -709,7 +709,7 @@ def _member_name(member: dict) -> str: way the card that holds it does.""" if not member.get("number"): return member["file"] - return taskfiles._member_entry(member["number"], member["title"]) + return taskfiles.member_entry(member["number"], member["title"]) def _joined(parts: list[str]) -> str: diff --git a/manager/core/state.py b/manager/core/state.py index 2b9c86e..cdaab13 100644 --- a/manager/core/state.py +++ b/manager/core/state.py @@ -21,7 +21,7 @@ SESSIONS: dict[str, dict] = {} # session_id -> meta EVENTS: dict[str, list[dict]] = {} # session_id -> slim events BOARD_EVENTS: list[dict] = [] # moves + agent lifecycle AGENTS: dict[str, dict] = {} # agent_id -> launch record -EXPECTED_MOVES: dict[tuple[str, str], tuple[str, float]] = {} # (file, to) -> (actor, ts) +EXPECTED_MOVES: dict[tuple[str, str], tuple[str, float, bool]] = {} # (file, to) -> (actor, ts, quiet) COMMIT_HOOKS: list = [] # run after a board-made task commit COMPLETING: dict[str, dict] = {} # filename -> {started, step}: merge & clean up in flight @@ -173,17 +173,30 @@ def task_committed(filename: str) -> None: pass -def expect_move(filename: str, target: str, actor: str) -> None: - """Tell the watcher who is about to move a file so it can attribute it.""" +def expect_move(filename: str, target: str, actor: str, quiet: bool = False) -> None: + """Tell the watcher who is about to move a file so it can attribute it. + + `quiet` adds the other half of that: the mover has already narrated + this one, so the watcher renders it and says nothing. A phase ending + moves every card it merged at once and reports it as the one thing it + is; five identical move lines scrolling behind that would be the same + fact told badly. It suppresses the ticker line only — the move is + still a move, and everything the board hangs off one still happens. + """ with LOCK: - EXPECTED_MOVES[(filename, target)] = (actor, time.time()) + EXPECTED_MOVES[(filename, target)] = (actor, time.time(), quiet) + + +def claim_move(filename: str, target: str) -> tuple[str, bool]: + """Who moved this, and whether they already said so.""" + with LOCK: + expected = EXPECTED_MOVES.pop((filename, target), None) + # forget stale expectations while we're here + cutoff = time.time() - 30 + for key in [k for k, (_, ts, _) in EXPECTED_MOVES.items() if ts < cutoff]: + EXPECTED_MOVES.pop(key, None) + return (expected[0], expected[2]) if expected else ("disk", False) def claim_expected(filename: str, target: str) -> str: - with LOCK: - actor_ts = EXPECTED_MOVES.pop((filename, target), None) - # forget stale expectations while we're here - cutoff = time.time() - 30 - for key in [k for k, (_, ts) in EXPECTED_MOVES.items() if ts < cutoff]: - EXPECTED_MOVES.pop(key, None) - return actor_ts[0] if actor_ts else "disk" + return claim_move(filename, target)[0] diff --git a/manager/core/taskfiles.py b/manager/core/taskfiles.py index 35cea24..b29f4c4 100644 --- a/manager/core/taskfiles.py +++ b/manager/core/taskfiles.py @@ -375,14 +375,13 @@ def _number(filename: str) -> str | None: return match.group(1) if match else None -def _commit_move(filename: str, target: str, src: Path, dst: Path, who: str, - number: str | None) -> None: - """The move and the claim in one commit. +def _move_spec(src: Path, dst: Path) -> list[str]: + """The paths one move's commit names. - Both paths are named, so git records a rename rather than a delete and - an add — and a card git has never seen (a brand-new backlog file) names - only its destination, since a pathspec matching nothing in HEAD would - fail the commit outright. + Both ends, so git records a rename rather than a delete and an add — + and a card git has never seen (a brand-new backlog file) names only its + destination, since a pathspec matching nothing in HEAD would fail the + commit outright. """ spec = [str(dst)] try: @@ -391,12 +390,19 @@ def _commit_move(filename: str, target: str, src: Path, dst: Path, who: str, spec.insert(0, str(src)) # git knew the old path: record its removal except (subprocess.SubprocessError, OSError): pass + return spec + + +def _commit_move(filename: str, target: str, src: Path, dst: Path, who: str, + number: str | None) -> None: + """The move and the claim in one commit.""" _commit(filename, f"board: {number or filename[:-3]} → {target} ({who or 'board'})", - spec, f"{filename} moved, but committing it failed") + _move_spec(src, dst), f"{filename} moved, but committing it failed") def _relocate(filename: str, src: Path, dst: Path, text: str, target: str, - actor: str, who: str | None = None) -> None: + actor: str, who: str | None = None, commit: bool = True, + quiet: bool = False) -> None: """The one door out of a directory under tasks/: register who is doing it, write the file, move it, commit it. @@ -406,12 +412,16 @@ def _relocate(filename: str, src: Path, dst: Path, text: str, target: str, and the commit message call the destination (a stage slug, or `archived`), and `who` is this checkout's git name when the caller has already paid for it. + + `commit` and `quiet` are what a batch of moves turns off: several cards + that moved as one thing are committed as one thing and narrated as one + thing (`move_together`), and both would be wrong done per file. """ - state.expect_move(filename, target, actor) + state.expect_move(filename, target, actor, quiet=quiet) dst.parent.mkdir(parents=True, exist_ok=True) src.write_text(text, encoding="utf-8") shutil.move(str(src), str(dst)) - if config.COMMIT_MOVES: + if config.COMMIT_MOVES and commit: _commit_move(filename, target, src, dst, actor_name() if who is None else who, _number(filename)) @@ -492,7 +502,7 @@ PHASE_JOIN_FROM = {"backlog", "to-do"} # a card joins a phase before it starts PHASE_HOST = "to-do" # and only a phase still waiting accepts it -def _member_entry(number: str, title: str) -> str: +def member_entry(number: str, title: str) -> str: """`33 — The landing page` — the way a person writes it. The section is authored by hand and read by hand, so a line the board @@ -569,7 +579,7 @@ def add_to_phase(filename: str, stage: str, phase_file: str) -> dict: else f"phase {_phase_name(holder)} already") raise ValueError(f"{where} lists {card['number']}") - entry = _member_entry(card["number"], card["title"]) + entry = member_entry(card["number"], card["title"]) if not append_to_section(phase_file, PHASE_HOST, "Cards", f"- {entry}", f"gained {number}"): raise ValueError(f"{phase_file} could not be written") @@ -578,12 +588,16 @@ def add_to_phase(filename: str, stage: str, phase_file: str) -> dict: "entry": entry, "line": f"- {entry}"} -def move_task(filename: str, source: str, target: str, actor: str = "you") -> dict: +def move_task(filename: str, source: str, target: str, actor: str = "you", + commit: bool = True, quiet: bool = False) -> dict: """Move a task file between stage directories and fix its Status line. With `BOARD_COMMIT_MOVES` on, the move also claims the card (an **Assignee:** line, this checkout's git name) or releases it when the card is walked back to backlog, and commits the whole change. + + `commit` and `quiet` belong to `move_together` below — a lone move + commits itself and narrates itself, as it always has. """ if source not in config.STAGE_DIRS or target not in config.STAGE_DIRS: raise ValueError("unknown stage") @@ -612,5 +626,46 @@ def move_task(filename: str, source: str, target: str, actor: str = "you") -> di elif name and claims(source, target): text = _set_assignee(text, name) - _relocate(filename, src, dst, text, target, actor, who=name) + _relocate(filename, src, dst, text, target, actor, who=name, + commit=commit, quiet=quiet) return read_task(dst, target) + + +def move_together(moves: list[tuple[str, str]], target: str, note: str, + actor: str = "you") -> list[dict]: + """Move several cards into one stage as one thing. + + A phase's ending is one event that happens to move five files, so the + record it leaves should be one too: one commit naming every card that + moved, rather than five `board: NN → done` lines in a row that a + reader of `git log` has to reassemble into the thing that happened. + The message still says what moved — the numbers are in it — and it + carries the `board: ` prefix, so in team mode it reaches the other + boards on the same push every other move rides. + + The moves are quiet for the same reason (see `state.expect_move`): the + caller narrates the ending, and the ticker does not also scroll the + parts. Each takes `moves` as `(filename, source stage)`. + + A card that cannot be moved — gone, or already in `target` — is + skipped rather than failing the rest. This runs after work that has + already landed in `main`, and finishing four cards beats abandoning + the sweep over the fifth. + """ + moved, spec = [], [] + for filename, source in moves: + src = config.TASKS / source / filename + dst = config.TASKS / target / filename + try: + moved.append(move_task(filename, source, target, actor=actor, + commit=False, quiet=True)) + except (ValueError, OSError): + continue + spec += _move_spec(src, dst) + if moved and config.COMMIT_MOVES: + numbers = ", ".join(task["number"] or task["file"][:-3] for task in moved) + _commit(moved[0]["file"], + f"board: {numbers} → {target}" + (f" {note}" if note else "") + + f" ({actor_name() or 'board'})", + spec, f"{len(moved)} cards moved, but committing them failed") + return moved diff --git a/manager/core/watch.py b/manager/core/watch.py index fc25b21..710e6e1 100644 --- a/manager/core/watch.py +++ b/manager/core/watch.py @@ -30,20 +30,25 @@ def _board_sig() -> dict[str, set[str]]: return sig -def _actor(filename: str, stage: str) -> tuple[str, bool]: - """Who did this, and whether it happened somewhere else. +def _actor(filename: str, stage: str) -> tuple[str, bool, bool]: + """Who did this, whether it happened somewhere else, and whether it has + already been narrated. A move this board made is claimed from the expectations; one a pull brought carries its commit author's name and is *remote* — this board is only rendering it; a plain mv on this disk is nobody in particular, but it is still this board's own disk, so it acts. + + *Quiet* is the mover saying it has told this story already: a phase + ending sweeps its whole list into done/ and reports one ending. The + line is skipped; nothing else about the move is. """ - actor = state.claim_expected(filename, stage) + actor, quiet = state.claim_move(filename, stage) if actor == "disk": who = sync.arrived_actor(filename) if who: - return who, True - return actor, False + return who, True, False + return actor, False, quiet def narrate(prev: dict[str, set[str]], cur: dict[str, set[str]]) -> None: @@ -52,12 +57,13 @@ def narrate(prev: dict[str, set[str]], cur: dict[str, set[str]]) -> None: cur_loc = {f: s for s, files in cur.items() for f in files} for f, stage in sorted(cur_loc.items()): if f in prev_loc and prev_loc[f] != stage: - actor, remote = _actor(f, stage) - state.record_board_event({ - "kind": "move", "file": f, "from": prev_loc[f], "to": stage, - "actor": actor, "remote": remote, - "summary": f"{f} moved {prev_loc[f]} → {stage} ({actor})", - }) + actor, remote, quiet = _actor(f, stage) + if not quiet: + state.record_board_event({ + "kind": "move", "file": f, "from": prev_loc[f], "to": stage, + "actor": actor, "remote": remote, + "summary": f"{f} moved {prev_loc[f]} → {stage} ({actor})", + }) # a failed run is worn by the card in the stage it died in — # wherever the card goes next, it arrives without the alarm agents.forget_failure(f) @@ -67,7 +73,9 @@ def narrate(prev: dict[str, set[str]], cur: dict[str, set[str]]) -> None: # per replica github.open_pr_async(f) elif f not in prev_loc: - actor, remote = _actor(f, stage) + actor, remote, quiet = _actor(f, stage) + if quiet: + continue state.record_board_event({ "kind": "new", "file": f, "to": stage, "actor": actor, "remote": remote, diff --git a/tests/test_phase_finishes.py b/tests/test_phase_finishes.py new file mode 100644 index 0000000..f321604 --- /dev/null +++ b/tests/test_phase_finishes.py @@ -0,0 +1,506 @@ +"""Finishing a phase finishes its cards, and clears up after them (task 59). + +A member stops at `review/` on purpose, and **merge & clean up** on the +phase card is the moment that stops being right: the phase's branch is in +`main`, so every card it carried is too. These run the real thing — a real +phase run over a real git repo, then a real merge — because what the card +is about is which branches ended up inside which, and mocking git would be +mocking the subject. + + python3 -m unittest discover -s tests -v +""" + +from __future__ import annotations + +import shutil +import subprocess +import sys +import unittest +from pathlib import Path + +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 taskfiles # noqa: E402 +import watch # noqa: E402 + +from tests.test_phase_runs import (DIES, ONE, PHASE, PHASE_BRANCH, TWO, # noqa: E402 + PhaseCase, card, git) + +THREE = "33-the-landing-page.md" +THREE_LISTED = ("- 31 — Stand up site/\n- 32 — Serve it\n" + "- 33 — The landing page\n") + + +class PhaseEnding(PhaseCase): + """The whole run, then the ending: two members merged into the phase + branch, the phase card in review/, and a person dragging it to done/.""" + + def run_phase(self) -> None: + self.start() # cut the branch, run 31 + self.advance() # merge 31, run 32 + self.advance() # merge 32, open the phase PR, card → review/ + self.assertEqual(self.stage_of(PHASE), "review", "the run never finished") + + def complete(self, stage: str = "review") -> dict: + return github.complete_task(PHASE, stage) + + def worktrees(self) -> list[str]: + return [line.split(" ", 1)[1] for line in + git(self.repo, "worktree", "list", "--porcelain").stdout.splitlines() + if line.startswith("worktree ")] + + def branches(self) -> list[str]: + return git(self.repo, "for-each-ref", "--format=%(refname:short)", + "refs/heads/").stdout.split() + + def ending(self) -> str: + """The one line the ticker gets for the ending.""" + lines = [s for s in self.summaries() if s.startswith("phase ") + and "finished" in s] + self.assertEqual(len(lines), 1, f"one ending, not {len(lines)}: {lines}") + return lines[0] + + +class AFinishedPhase(PhaseEnding): + """Merge & clean up on the phase card, single-player and offline.""" + + # — the cards — + + def test_the_phase_card_and_every_merged_member_are_done(self): + self.run_phase() + + self.complete() + + self.assertEqual(self.stage_of(PHASE), "done") + self.assertEqual(self.stage_of(ONE), "done") + self.assertEqual(self.stage_of(TWO), "done") + + def test_the_swept_cards_say_done_in_their_own_headers(self): + """A move is the file *and* the Status line — a swept card that + still says Review would wear `status drift` the moment it landed.""" + self.run_phase() + + self.complete() + + self.assertIn("**Status:** Done", self.text(ONE)) + self.assertIn("**Status:** Done", self.text(TWO)) + + def test_the_completion_reports_what_went_with_it(self): + self.run_phase() + + result = self.complete() + + self.assertTrue(result["merged"]) + self.assertEqual(sorted(result["swept"]), sorted([ONE, TWO]), + "the browser is told which cards went, for its toast") + + # — the workspaces — + + def test_no_merged_member_leaves_a_worktree_or_a_branch(self): + self.run_phase() + for stem in (PHASE[:-3], ONE[:-3], TWO[:-3]): + self.assertIn(str(config.WORKTREES / stem), self.worktrees(), + "the run should have left one to clean up") + + self.complete() + + self.assertEqual(self.worktrees(), [str(self.repo)], + "git worktree list names only what was there before") + self.assertEqual(self.branches(), ["main"], + "git branch names only what was there before") + self.assertFalse((config.WORKTREES / ONE[:-3]).exists()) + self.assertFalse((config.WORKTREES / TWO[:-3]).exists()) + + def test_a_members_pr_state_is_dropped_with_its_branch(self): + self.run_phase() + github.PR_STATE[ONE] = {"verdict": "green", "ci": "pass", "url": "u"} + + self.complete() + + self.assertNotIn(ONE, github.PR_STATE) + + # — one ending, said once — + + def test_the_ticker_reports_one_ending_naming_the_phase_and_the_count(self): + self.run_phase() + state.BOARD_EVENTS.clear() + + self.complete() + + line = self.ending() + self.assertIn("40 — Ship the site", line) + self.assertIn("2 cards", line) + self.assertIn("done/", line) + + def test_the_member_moves_do_not_also_scroll_past(self): + """The watcher narrates every move it finds on disk. A sweep is one + thing that happens to move several files, so the moves it makes are + claimed as already told — and the phase card's own move is not.""" + self.run_phase() + before = watch._board_sig() + state.BOARD_EVENTS.clear() + + self.complete() + watch.narrate(before, watch._board_sig()) + + moves = [e for e in state.BOARD_EVENTS if e.get("kind") == "move"] + self.assertEqual([e["file"] for e in moves], [PHASE], + "only the card the person dragged narrates its move") + self.ending() + + def test_a_card_moved_normally_still_narrates(self): + """The suppression is scoped to the sweep: an ordinary move made + through the board is as loud as it ever was.""" + before = watch._board_sig() + taskfiles.move_task(ONE, "backlog", "to-do") + watch.narrate(before, watch._board_sig()) + + self.assertTrue(any(e.get("kind") == "move" and e["file"] == ONE + for e in state.BOARD_EVENTS)) + + # — only what the phase merged — + + def test_a_member_the_phase_never_merged_is_left_exactly_as_it_is(self): + """Walked back out of review/ before the phase could bring it: its + card, its worktree and its branch are none of the ending's + business, and there is work in all three.""" + self.write_cards(listed=THREE_LISTED) + self.write(THREE, card("33 — The landing page")) + self.start() + self.advance() # merge 31, run 32 + self.advance() # merge 32, run 33 + self.assertEqual(self.stage_of(THREE), "review") + taskfiles.move_task(THREE, "review", "in-progress") + state.BOARD_EVENTS.clear() + + self.complete("in-progress") + + self.assertEqual(self.stage_of(THREE), "in-progress", "the card stays") + self.assertTrue(self.branch_exists(f"task/{THREE[:-3]}")) + self.assertIn(str(config.WORKTREES / THREE[:-3]), self.worktrees()) + self.assertEqual(self.stage_of(ONE), "done") + self.assertEqual(self.stage_of(TWO), "done") + self.assertIn("1 card it never merged stays where it is", self.ending()) + + def test_a_member_whose_run_died_is_not_swept(self): + self.write_cards(listed=THREE_LISTED) + self.write(THREE, card("33 — The landing page")) + self.start() + self.advance() + self.adapter_is(DIES) + self.advance() # 33 dies; the phase halts on it + self.advance() + self.assertEqual(self.stage_of(THREE), "in-progress") + + self.complete("in-progress") + + self.assertEqual(self.stage_of(THREE), "in-progress") + self.assertEqual(self.stage_of(ONE), "done") + + def test_only_what_reached_the_branch_goes_however_far_the_run_got(self): + """Stopped mid-run: 31 is merged, 32 has reached review/ but the + phase has not brought it, and 33 has not been started. Exactly one + of those three is the ending's business.""" + self.write_cards(listed=THREE_LISTED) + self.write(THREE, card("33 — The landing page")) + self.start() + self.advance() # merge 31, run 32 — and stop there + + self.complete("in-progress") + + self.assertEqual(self.stage_of(ONE), "done", "merged, so finished") + self.assertEqual(self.stage_of(TWO), "review", + "in review/ but never merged into the phase branch") + self.assertTrue(self.branch_exists(f"task/{TWO[:-3]}")) + self.assertEqual(self.stage_of(THREE), "backlog", "never started") + + # — abort together — + + def test_a_merge_conflict_moves_nothing_at_all(self): + self.run_phase() + for where, text in ((self.repo, "main's own line\n"), + (config.WORKTREES / PHASE[:-3], "the phase's line\n")): + (where / "contested.txt").write_text(text, encoding="utf-8") + git(where, "add", "-A") + git(where, "commit", "-q", "-m", "contested") + + with self.assertRaises(ValueError) as caught: + self.complete() + + self.assertIn("merge conflict", str(caught.exception)) + self.assertEqual(self.stage_of(PHASE), "review", "the phase card stays") + self.assertEqual(self.stage_of(ONE), "review", "and so does every member") + self.assertEqual(self.stage_of(TWO), "review") + self.assertTrue(self.branch_exists(f"task/{ONE[:-3]}")) + self.assertIn(str(config.WORKTREES / ONE[:-3]), self.worktrees()) + + def test_the_sweep_waits_for_the_merge_rather_than_racing_it(self): + """Ordering, not luck: the cards move after the merge has landed, + so a merge that fails half way finds nothing already swept.""" + self.run_phase() + seen = {} + real = github._merge_locally + self.addCleanup(setattr, github, "_merge_locally", real) + + def watched(filename, branch): + seen["before"] = (self.stage_of(ONE), self.stage_of(TWO)) + real(filename, branch) + github._merge_locally = watched + + self.complete() + + self.assertEqual(seen["before"], ("review", "review")) + + # — the other endings do not sweep — + + def test_just_moving_the_card_moves_no_member(self): + self.run_phase() + + taskfiles.move_task(PHASE, "review", "done") + + self.assertEqual(self.stage_of(PHASE), "done") + self.assertEqual(self.stage_of(ONE), "review") + self.assertEqual(self.stage_of(TWO), "review") + self.assertTrue(self.branch_exists(f"task/{ONE[:-3]}")) + + def test_archiving_the_phase_card_marks_nothing_done(self): + """Archiving releases the members to the board in whatever stage + they are genuinely in — which is not done/, because nothing was + merged into main.""" + self.run_phase() + taskfiles.move_task(PHASE, "review", "to-do") + + taskfiles.archive_task(PHASE, "to-do") + + self.assertTrue((self.tasks / "archive" / PHASE).is_file()) + self.assertEqual(self.stage_of(ONE), "review") + self.assertEqual(self.stage_of(TWO), "review") + self.assertTrue(self.branch_exists(f"task/{TWO[:-3]}")) + self.assertIn(str(config.WORKTREES / TWO[:-3]), self.worktrees()) + + # — the edges — + + def test_a_member_already_moved_to_done_by_hand_is_no_trouble(self): + self.run_phase() + taskfiles.move_task(ONE, "review", "done") + + result = self.complete() + + self.assertEqual(self.stage_of(ONE), "done") + self.assertEqual(self.stage_of(TWO), "done") + self.assertEqual(result["swept"], [TWO], + "a card already there is not moved twice") + self.assertFalse(self.branch_exists(f"task/{ONE[:-3]}"), + "it was still cleaned up: its work is in main too") + + def test_a_dirty_worktree_is_reported_and_kept_not_forced(self): + self.run_phase() + (config.WORKTREES / TWO[:-3] / "notes.txt").write_text( + "something a person was in the middle of\n", encoding="utf-8") + state.BOARD_EVENTS.clear() + + self.complete() + + self.assertTrue((config.WORKTREES / TWO[:-3] / "notes.txt").is_file(), + "uncommitted work is never thrown away") + self.assertTrue(self.branch_exists(f"task/{TWO[:-3]}"), + "and its branch stays with it — it is still checked out") + self.assertIn(f"{TWO[:-3]}: worktree kept — 1 uncommitted change in it " + f"— nothing uncommitted is thrown away", self.summaries(), + "the ticker says which one and why") + self.assertEqual(self.stage_of(TWO), "done", + "the card is still finished: its work is in main") + self.assertFalse((config.WORKTREES / ONE[:-3]).exists(), + "the clean one still went") + + def test_a_phase_that_merged_nothing_says_so_and_moves_nobody(self): + """The card is still the person's to drag to done/ — what it must + not do is claim an ending for work that never landed.""" + self.write_cards(listed=THREE_LISTED) + self.write(THREE, card("33 — The landing page")) + self.start() # 31 runs, but nothing is merged yet + state.BOARD_EVENTS.clear() + + result = self.complete("in-progress") + + self.assertEqual(result["swept"], []) + self.assertEqual(self.stage_of(ONE), "review") + self.assertIn("it had merged nothing, so no card went with it", + self.ending()) + self.assertIn("3 cards it never merged stay where they are", self.ending()) + + def test_a_card_with_no_members_completes_as_it_always_did(self): + """An ordinary card never reaches any of this.""" + self.write(ONE, card("31 — Stand up site/", status="Review"), "review") + (self.tasks / "backlog" / ONE).unlink() + git(self.repo, "worktree", "add", "-q", "-b", f"task/{ONE[:-3]}", + str(config.WORKTREES / ONE[:-3])) + (config.WORKTREES / ONE[:-3] / "work.txt").write_text("x\n", encoding="utf-8") + git(config.WORKTREES / ONE[:-3], "add", "-A") + git(config.WORKTREES / ONE[:-3], "commit", "-q", "-m", "work") + + result = github.complete_task(ONE, "review") + + self.assertEqual(result, {"merged": True, "swept": []}) + self.assertEqual(self.stage_of(ONE), "done") + + +class TheSweepReachesGit(PhaseEnding): + """With `BOARD_COMMIT_MOVES` on: a sweep that never left one working + tree is not a sweep.""" + + def setUp(self): + super().setUp() + self.patch(COMMIT_MOVES=True) + + def commits(self) -> list[str]: + return git(self.repo, "log", "--format=%s", "main").stdout.splitlines() + + def test_the_sweep_is_one_commit_that_names_what_moved(self): + self.run_phase() + + self.complete() + + sweep = [s for s in self.commits() if s.startswith("board: ") and "→ done" in s] + self.assertIn("board: 31, 32 → done with phase 40 (tester)", sweep) + self.assertNotIn("board: 31 → done (tester)", sweep, + "five cards are not five lines in the log") + self.assertIn("board: 40 → done (tester)", sweep, + "the card the person dragged still moves as itself") + + def test_the_swept_files_are_committed_where_they_landed(self): + self.run_phase() + + self.complete() + + self.assertEqual(git(self.repo, "status", "--porcelain").stdout.strip(), "", + "nothing is left modified for a human to notice later") + listed = git(self.repo, "ls-files", "tasks/done").stdout.split() + for name in (PHASE, ONE, TWO): + self.assertIn(f"tasks/done/{name}", listed) + + def test_the_commit_carries_the_prefix_sync_publishes_on(self): + """`board: ` is what the piggyback guard looks for: a commit + without it stalls every later push.""" + self.run_phase() + published: list[str] = [] + self.addCleanup(state.COMMIT_HOOKS.clear) + state.COMMIT_HOOKS.append(published.append) + + self.complete() + + self.assertIn(ONE, published, "the sweep fires the same commit hook") + for line in self.commits(): + if "→ done" in line: + self.assertTrue(line.startswith("board: "), line) + + +class TheRemoteIsClearedToo(PhaseEnding): + """The third workspace a member leaves behind is on the remote.""" + + REMOTE = True + + def remote_branches(self) -> list[str]: + out = git(self.repo, "ls-remote", "--heads", "origin").stdout + return [line.split("refs/heads/")[-1] for line in out.splitlines() if line] + + def test_no_merged_member_leaves_a_branch_on_the_remote(self): + self.run_phase() + git(self.repo, "push", "-q", "origin", + f"task/{ONE[:-3]}", f"task/{TWO[:-3]}") + self.assertIn(f"task/{ONE[:-3]}", self.remote_branches()) + + self.complete() + + self.assertEqual(self.remote_branches(), ["main"]) + + def test_a_member_the_phase_never_merged_keeps_its_remote_branch(self): + self.write_cards(listed=THREE_LISTED) + self.write(THREE, card("33 — The landing page")) + self.start() + self.advance() + self.advance() + taskfiles.move_task(THREE, "review", "in-progress") + git(self.repo, "push", "-q", "origin", f"task/{THREE[:-3]}") + + self.complete("in-progress") + + self.assertIn(f"task/{THREE[:-3]}", self.remote_branches()) + + +class TeamModeEndsItOnOrigin(PhaseEnding): + """With `BOARD_SYNC` on the merge is made by GitHub, so local main has + not seen it when the sweep runs. Nothing in the sweep may depend on + that: what the phase carried is answered by the phase branch, which is + still here, and the members' branches are deleted with `-D` for + exactly the reason the phase card's own is.""" + + REMOTE = True + + def test_the_cards_and_their_workspaces_still_go(self): + self.run_phase() + git(self.repo, "push", "-q", "origin", + f"task/{ONE[:-3]}", f"task/{TWO[:-3]}") + self.patch(SYNC=True, COMMIT_MOVES=True) + + result = self.complete() + + self.assertTrue(any(call[:2] == ["pr", "merge"] for call in self.gh_calls()), + "the merge is origin's to make in team mode") + self.assertEqual(sorted(result["swept"]), sorted([ONE, TWO])) + self.assertEqual(self.stage_of(ONE), "done") + self.assertEqual(self.stage_of(TWO), "done") + self.assertFalse(self.branch_exists(f"task/{ONE[:-3]}"), + "a branch main has not caught up with is still gone") + remote = git(self.repo, "ls-remote", "--heads", "origin").stdout + self.assertNotIn(f"task/{ONE[:-3]}", remote) + self.assertIn("board: 31, 32 → done with phase 40 (tester)", + git(self.repo, "log", "--format=%s", "main").stdout) + + +class TheDocumentedEnding(unittest.TestCase): + """AGENTS.md is the design record — the ending has to be in it.""" + + @classmethod + def setUpClass(cls): + cls.doc = (REPO / "AGENTS.md").read_text(encoding="utf-8") + + def test_the_sweep_is_described(self): + self.assertIn("### Finishing a phase finishes its cards", self.doc) + + def test_it_says_what_is_not_swept(self): + section = self.doc.split("### Finishing a phase finishes its cards")[1] + section = section.split("\n### ")[0] + for promise in ("halted", "walked back", "uncommitted", "single commit", + "Just move the card", "Archiving"): + self.assertIn(promise, section, f"the record must say: {promise}") + + +class TheSheetSaysSo(unittest.TestCase): + """board.html has no frontend runner — these are the source-level + invariants of what the person is told before they click.""" + + @classmethod + def setUpClass(cls): + cls.html = (REPO / "manager" / "core" / "board.html").read_text( + encoding="utf-8") + + def test_the_sheet_counts_the_cards_that_would_go_with_it(self): + self.assertIn("const swept = task.isPhase ? mergedIn(task).size : 0;", + self.html) + self.assertIn("card${swept === 1 ? '' : 's'} this phase merged go to done/", + self.html) + + def test_just_moving_the_card_says_the_members_stay(self): + self.assertIn('and so does every card in the phase', self.html) + + def test_the_toast_names_how_many_went(self): + self.assertIn("const went = (data.swept || []).length;", self.html) + + +if __name__ == "__main__": + unittest.main()