From b462b83a7f0ed1c355116b1173ee455b111ae277 Mon Sep 17 00:00:00 2001 From: istos Date: Fri, 31 Jul 2026 16:57:09 +0200 Subject: [PATCH] Archiving a card reaches git, like every other task-file write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An archive renamed the file on disk and stopped there, leaving an uncommitted deletion of a tracked file — exactly what sync._clean() refuses to run over, so one drag onto the tray stalled every later board commit, kept the archive off the other boards, and left the card in a single working tree. Appended agent reports had the same gap. taskfiles now has one door out of a stage directory, _relocate(): it registers the expectation, writes, moves, and commits under the same COMMIT_MOVES gate a move uses — so archive, unarchive and move all commit by construction rather than by each caller remembering. The messages read like the others (`board: -> archived ()`, and the stage slug on the way back) and carry the `board: ` prefix sync's piggyback guard requires; routing through _commit means state.task_committed() fires, so the event-driven push publishes it. The second door is append_to_task(), which writes into a card where it stands and commits via commit_edit(); agents._file_report() uses it instead of touching config.TASKS itself, so a run's closing report is recorded rather than left modified. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 10 ++ manager/core/agents.py | 17 +-- manager/core/taskfiles.py | 96 ++++++++++--- tests/test_archive_commits.py | 246 ++++++++++++++++++++++++++++++++++ tests/test_boards_sync.py | 16 +++ 5 files changed, 356 insertions(+), 29 deletions(-) create mode 100644 tests/test_archive_commits.py diff --git a/AGENTS.md b/AGENTS.md index 6e9cd0f..b00f7ae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,6 +106,16 @@ out of every column, never deleted, Status set to `Archived`. The toast says without an undo in the same breath. Cards in the working stages (in-progress, review) cannot be archived; finish or walk them back first. +Archiving is a move, so it commits like one: under `BOARD_COMMIT_MOVES` +the rename into `tasks/archive/` lands in a single `board: → archived +()` commit naming both paths, and ⌘Z commits its own way back. That +is not bookkeeping for its own sake — an uncommitted deletion of a tracked +file is exactly what `BOARD_SYNC` refuses to run over, so an archive that +stopped at the disk would silently hold up every later move on that board, +and the archived card would exist in one working tree only. The same law +covers every write the board makes to a task file: the `**PR:**` line, a +claim, and an agent's closing report all reach git the same way. + ## Local commands Projects grow chores that belong to a specific checkout — applying a diff --git a/manager/core/agents.py b/manager/core/agents.py index 2d432b1..09d7f21 100644 --- a/manager/core/agents.py +++ b/manager/core/agents.py @@ -22,7 +22,8 @@ import config import events import reports import state -from taskfiles import actor_name, find_stage_of, move_task, read_task, set_assignee +from taskfiles import (actor_name, append_to_task, find_stage_of, move_task, + read_task, set_assignee) def _report_of(record: dict, text: str | None = None) -> str: @@ -39,17 +40,17 @@ def _report_of(record: dict, text: str | None = None) -> str: def _file_report(record: dict, heading: str, report: str) -> None: - """The report travels with the task, like every review does.""" + """The report travels with the task, like every review does — and, like + every other board-made write to a task file, it reaches git rather than + sitting modified in one working tree (task 44).""" stage = find_stage_of(record["task"]) if not stage or not report: return - path = config.TASKS / stage / record["task"] stamp = time.strftime("%Y-%m-%d %H:%M") - try: - with path.open("a", encoding="utf-8") as fh: - fh.write(f"\n\n---\n\n## {heading} — {stamp} ({record.get('name') or 'agent'})\n\n{report}\n") - except OSError: - pass + name = record.get("name") or "agent" + append_to_task(record["task"], stage, + f"\n\n---\n\n## {heading} — {stamp} ({name})\n\n{report}\n", + f"{heading} filed") def _session_report(record: dict, report: str) -> None: diff --git a/manager/core/taskfiles.py b/manager/core/taskfiles.py index 6c9e6bf..515e997 100644 --- a/manager/core/taskfiles.py +++ b/manager/core/taskfiles.py @@ -1,8 +1,14 @@ -"""Reading and moving task files — the only module that touches tasks/. +"""Reading, moving and writing task files — the only module that touches tasks/. The directory a task file sits in *is* its status (see ../AGENTS.md). Nothing here knows about agents or HTTP; it is the same folder kanban you could drive by hand with mv. + +Every write goes out through one of two doors — `_relocate` for anything +that changes which directory a card sits in, `append_to_task`/`commit_edit` +for anything written into it where it stands — and both commit under the +`COMMIT_MOVES` gate. That is deliberate: committing is a property of +writing to a task file, not something each caller has to remember. """ from __future__ import annotations @@ -105,9 +111,15 @@ def find_stage_of(filename: str) -> str | None: ARCHIVE_FROM = {"backlog", "to-do", "done"} -def archive_task(filename: str, source: str) -> dict: +def archive_task(filename: str, source: str, actor: str = "you") -> dict: """Archive: out of the flow but never deleted. tasks/archive/ is not a - stage — archived cards simply leave the board.""" + stage — archived cards simply leave the board. + + It is still a board-made write to a task file, so it goes out through + `_relocate` like every other one: attributed, and committed under the + same gate a move is (an uncommitted deletion of a tracked file is + precisely what stops sync publishing anything else). + """ if source not in ARCHIVE_FROM: raise ValueError("archive takes cards from backlog, to-do or done only") if Path(filename).name != filename or not filename.endswith(".md"): @@ -121,14 +133,13 @@ def archive_task(filename: str, source: str) -> dict: text = src.read_text(encoding="utf-8") if STATUS_RE.search(text): text = STATUS_RE.sub("**Status:** Archived", text, count=1) - dst.parent.mkdir(parents=True, exist_ok=True) - src.write_text(text, encoding="utf-8") - shutil.move(str(src), str(dst)) + _relocate(filename, src, dst, text, "archived", actor) return {"file": filename, "from": source} -def unarchive_task(filename: str, target: str) -> dict: - """⌘Z: bring the last archived card back where it came from.""" +def unarchive_task(filename: str, target: str, actor: str = "you") -> dict: + """⌘Z: bring the last archived card back where it came from — and + record the way back in git, exactly as the way out was.""" if target not in ARCHIVE_FROM: raise ValueError("unknown stage to restore into") src = config.TASKS / "archive" / filename @@ -140,8 +151,7 @@ def unarchive_task(filename: str, target: str) -> dict: text = src.read_text(encoding="utf-8") if STATUS_RE.search(text): text = STATUS_RE.sub(f"**Status:** {config.STAGE_LABELS[target]}", text, count=1) - src.write_text(text, encoding="utf-8") - shutil.move(str(src), str(dst)) + _relocate(filename, src, dst, text, target, actor) return {"file": filename, "to": target} @@ -225,9 +235,20 @@ def _commit(filename: str, message: str, spec: list[str], failure: str) -> bool: return False +def _number(filename: str) -> str | None: + match = NUMBER_RE.match(filename) + 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.""" + """The move and the claim in one commit. + + 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. + """ spec = [str(dst)] try: tracked = _git("ls-files", "--", str(src)) @@ -239,6 +260,27 @@ def _commit_move(filename: str, target: str, src: Path, dst: Path, who: str, spec, 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: + """The one door out of a directory under tasks/: register who is doing + it, write the file, move it, commit it. + + Every mover in this module goes through here, so committing is a + property of *writing to a task file* rather than something each caller + remembers — the way archiving forgot it. `target` is what the ticker + 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. + """ + state.expect_move(filename, target, actor) + 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: + _commit_move(filename, target, src, dst, + actor_name() if who is None else who, _number(filename)) + + def commit_edit(filename: str, stage: str, what: str) -> bool: """Commit a board-made edit to a card in place — the `**PR:**` line and anything else the board writes into a file it does not move. @@ -251,14 +293,32 @@ def commit_edit(filename: str, stage: str, what: str) -> bool: if not config.COMMIT_MOVES: return False path = config.TASKS / stage / filename - number = NUMBER_RE.match(filename) return _commit(filename, - f"board: {number.group(1) if number else filename[:-3]} " + f"board: {_number(filename) or filename[:-3]} " f"{what} ({actor_name() or 'board'})", [str(path)], f"{filename}: {what} recorded, but committing it failed") +def append_to_task(filename: str, stage: str, text: str, what: str) -> bool: + """Append to a card where it stands, and commit the write. + + The other half of the same law: an agent's closing report is the + permanent record the project keeps on purpose, so it reaches git like + the `**PR:**` line does instead of sitting modified in one working + tree. Returns whether the text was written (the commit is the gate's + business, and is narrated if it fails). + """ + path = config.TASKS / stage / filename + try: + with path.open("a", encoding="utf-8") as fh: + fh.write(text) + except OSError: + return False + commit_edit(filename, stage, what) + return True + + def move_task(filename: str, source: str, target: str, actor: str = "you") -> dict: """Move a task file between stage directories and fix its Status line. @@ -293,11 +353,5 @@ def move_task(filename: str, source: str, target: str, actor: str = "you") -> di elif name and claims(source, target): text = _set_assignee(text, name) - state.expect_move(filename, target, actor) - dst.parent.mkdir(parents=True, exist_ok=True) - src.write_text(text, encoding="utf-8") - shutil.move(str(src), str(dst)) - task = read_task(dst, target) - if config.COMMIT_MOVES: - _commit_move(filename, target, src, dst, name, task["number"]) - return task + _relocate(filename, src, dst, text, target, actor, who=name) + return read_task(dst, target) diff --git a/tests/test_archive_commits.py b/tests/test_archive_commits.py new file mode 100644 index 0000000..919c6b1 --- /dev/null +++ b/tests/test_archive_commits.py @@ -0,0 +1,246 @@ +"""Archiving a card reaches git (task 44), and so does every other write the +board makes to a task file. + +A move commits itself; an archive used to rename the file on disk and stop +there, leaving an uncommitted deletion of a tracked file — precisely what +sync refuses to run over. Appended agent reports had the same gap. These +cases run against a throwaway git repo, so the commits are real ones. + + python3 -m unittest discover -s tests -v +""" + +from __future__ import annotations + +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO / "manager" / "core")) + +import agents # noqa: E402 +import config # noqa: E402 +import state # noqa: E402 +import taskfiles # noqa: E402 +import watch # noqa: E402 + +CARD = ("# 44 — A card that gets archived\n\n" + "**Status:** Backlog\n" + "**Priority:** High\n\n" + "Body text nobody should touch.\n") +FILENAME = "44-a-card.md" +MOVER = "Mover One" + + +def git(cwd: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(cwd), *args], + capture_output=True, text=True) + + +class ArchiveReachesGit(unittest.TestCase): + def setUp(self): + # resolve(): macOS tempdirs sit behind the /var → /private/var + # symlink and git reports the resolved path, so absolute pathspecs + # only match if we resolve too. + tmp = Path(tempfile.mkdtemp(prefix="bench-archive-")).resolve() + self.addCleanup(shutil.rmtree, tmp, True) + self.repo = tmp / "repo" + for slug in config.STAGE_DIRS: + (self.repo / "tasks" / slug).mkdir(parents=True) + subprocess.run(["git", "init", "-q", "-b", "main", str(self.repo)], + check=True, capture_output=True) + git(self.repo, "config", "user.name", MOVER) + git(self.repo, "config", "user.email", "mover@example.com") + + self.patch(TASKS=self.repo / "tasks", REPO=self.repo, + SESSIONS_DIR=tmp / "sessions", COMMIT_MOVES=True) + state.BOARD_EVENTS.clear() + state.EXPECTED_MOVES.clear() + + self.write("backlog", CARD) + git(self.repo, "add", "-A") + git(self.repo, "commit", "-q", "-m", "root") + self.baseline = self.commit_count() + + def patch(self, **values) -> None: + for attr, value in values.items(): + self.addCleanup(setattr, config, attr, getattr(config, attr)) + setattr(config, attr, value) + + # — the repo under test — + + def path(self, stage: str) -> Path: + return self.repo / "tasks" / stage / FILENAME + + def write(self, stage: str, text: str) -> None: + self.path(stage).write_text(text, encoding="utf-8") + + def read(self, stage: str) -> str: + return self.path(stage).read_text(encoding="utf-8") + + def commit_count(self) -> int: + return int(git(self.repo, "rev-list", "--count", "HEAD").stdout.strip()) + + def head_message(self) -> str: + return git(self.repo, "log", "-1", "--pretty=%s").stdout.strip() + + def head_files(self) -> list[str]: + # --no-renames: the point is which paths the commit touched, not how + # git chooses to describe the pair. + out = git(self.repo, "show", "--name-only", "--no-renames", + "--pretty=format:", "HEAD").stdout + return sorted(line for line in out.splitlines() if line.strip()) + + def porcelain(self) -> str: + return git(self.repo, "status", "--porcelain").stdout + + def dirty(self) -> str: + """What sync._clean() looks at: tracked files only.""" + return git(self.repo, "status", "--porcelain", + "--untracked-files=no").stdout + + # — archiving — + + def test_archiving_commits_the_move_out_of_the_stage(self): + taskfiles.archive_task(FILENAME, "backlog") + + self.assertEqual(self.commit_count(), self.baseline + 1) + self.assertEqual(self.head_message(), f"board: 44 → archived ({MOVER})") + self.assertEqual(self.head_files(), + ["tasks/archive/" + FILENAME, "tasks/backlog/" + FILENAME], + "both paths, so git records a rename not a delete and an add") + self.assertEqual(self.porcelain(), "", + "nothing is left behind for a human to find later") + self.assertIn("**Status:** Archived", self.read("archive")) + + def test_the_archive_no_longer_stalls_sync(self): + """The bug, stated the way it bit: `sync._clean()` reads + `git status --porcelain --untracked-files=no`, and an uncommitted + deletion of a tracked file makes it false for every later move.""" + taskfiles.archive_task(FILENAME, "backlog") + + self.assertEqual(self.dirty(), "") + + def test_the_archive_commit_is_pushed_like_a_move(self): + """Event-driven push hangs off state.task_committed(), so routing + through the same helper publishes the archive without sync having + to know it happened.""" + published: list[str] = [] + state.COMMIT_HOOKS.append(published.append) + self.addCleanup(state.COMMIT_HOOKS.remove, published.append) + + taskfiles.archive_task(FILENAME, "backlog") + + self.assertEqual(published, [FILENAME]) + + def test_a_card_git_has_never_seen_is_archived_as_an_addition(self): + """A brand-new backlog file has no path in HEAD, so naming the + source in the pathspec would fail the commit outright.""" + fresh = "45-brand-new.md" + (self.repo / "tasks" / "backlog" / fresh).write_text( + CARD.replace("44", "45"), encoding="utf-8") + + taskfiles.archive_task(fresh, "backlog") + + self.assertEqual(self.commit_count(), self.baseline + 1) + self.assertEqual(self.head_message(), f"board: 45 → archived ({MOVER})") + self.assertEqual(self.head_files(), ["tasks/archive/" + fresh]) + self.assertEqual(self.porcelain(), "") + + def test_the_archive_names_who_did_it_rather_than_the_disk(self): + taskfiles.archive_task(FILENAME, "backlog") + + self.assertEqual(state.claim_expected(FILENAME, "archived"), "you", + "the mover registers itself, as every other move does") + + # — the way back — + + def test_the_undo_commits_its_own_restore(self): + taskfiles.archive_task(FILENAME, "backlog") + + taskfiles.unarchive_task(FILENAME, "backlog") + + self.assertEqual(self.commit_count(), self.baseline + 2, + "one commit out, one commit back") + self.assertEqual(self.head_message(), f"board: 44 → backlog ({MOVER})") + self.assertEqual(self.head_files(), + ["tasks/archive/" + FILENAME, "tasks/backlog/" + FILENAME]) + self.assertEqual(self.porcelain(), "") + self.assertEqual(self.read("backlog"), CARD, + "the card comes back exactly as it went") + + def test_the_undo_restores_the_status_of_the_stage_it_returns_to(self): + shutil.move(str(self.path("backlog")), str(self.path("to-do"))) + self.write("to-do", CARD.replace("Backlog", "To Do")) + git(self.repo, "add", "-A") + git(self.repo, "commit", "-q", "-m", "hand move to to-do") + self.baseline = self.commit_count() + taskfiles.archive_task(FILENAME, "to-do") + + taskfiles.unarchive_task(FILENAME, "to-do") + + self.assertIn("**Status:** To Do", self.read("to-do")) + self.assertEqual(self.head_message(), f"board: 44 → to-do ({MOVER})") + + def test_the_restored_card_is_attributed_to_the_person_on_the_ticker(self): + """The watcher sees the card reappear in a stage directory; without + an expectation registered it would call that appearance `disk`.""" + taskfiles.archive_task(FILENAME, "backlog") + before = {slug: set() for slug in config.STAGE_DIRS} + + taskfiles.unarchive_task(FILENAME, "backlog") + state.BOARD_EVENTS.clear() + watch.narrate(before, {slug: ({FILENAME} if slug == "backlog" else set()) + for slug in config.STAGE_DIRS}) + + events = [e for e in state.BOARD_EVENTS if e.get("file") == FILENAME] + self.assertEqual([e["actor"] for e in events], ["you"]) + + # — appended reports — + + def test_an_appended_report_commits_too(self): + record = {"task": FILENAME, "name": "Wren", "log": "/dev/null"} + + agents._file_report(record, "Work report", "It works.") + + self.assertIn("## Work report", self.read("backlog")) + self.assertIn("It works.", self.read("backlog")) + self.assertEqual(self.commit_count(), self.baseline + 1) + self.assertEqual(self.head_message(), f"board: 44 Work report filed ({MOVER})") + self.assertEqual(self.head_files(), ["tasks/backlog/" + FILENAME]) + self.assertEqual(self.porcelain(), "", + "no run leaves a modified task file behind") + + def test_an_empty_report_writes_nothing_and_commits_nothing(self): + agents._file_report({"task": FILENAME, "name": "Wren"}, "Work report", "") + + self.assertEqual(self.read("backlog"), CARD) + self.assertEqual(self.commit_count(), self.baseline) + + # — the gate — + + def test_gate_off_archives_exactly_as_before(self): + self.patch(COMMIT_MOVES=False) + + taskfiles.archive_task(FILENAME, "backlog") + + self.assertEqual(self.commit_count(), self.baseline) + self.assertIn("**Status:** Archived", self.read("archive")) + self.assertEqual(self.porcelain(), + " D tasks/backlog/44-a-card.md\n?? tasks/archive/\n", + "the archive stays for a human to commit, index untouched") + + def test_gate_off_files_a_report_without_committing_it(self): + self.patch(COMMIT_MOVES=False) + + agents._file_report({"task": FILENAME, "name": "Wren"}, "Work report", "Done.") + + self.assertIn("Done.", self.read("backlog")) + self.assertEqual(self.commit_count(), self.baseline) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_boards_sync.py b/tests/test_boards_sync.py index 1265b2f..2621f3f 100644 --- a/tests/test_boards_sync.py +++ b/tests/test_boards_sync.py @@ -171,6 +171,22 @@ class TwoBoards(unittest.TestCase): "a move that arrived over origin is not 'disk'") self.assertEqual((moves[0]["from"], moves[0]["to"]), ("backlog", "to-do")) + def test_an_archive_publishes_itself_and_empties_the_other_board(self): + """Task 44: an archive is a board-made move like any other, so it + commits, pushes and reaches the teammate — where the card is simply + gone from every column rather than lingering as one it never saw + leave.""" + self.use(self.ada) + taskfiles.archive_task(FILENAME, "backlog") + self.assertEqual(sync.push_now(), "ok") + + self.use(self.elena) + self.assertEqual(sync.pull_now(), "pulled") + + self.assertIsNone(self.stage_of(self.elena), "out of every column") + self.assertTrue((self.elena / "tasks" / "archive" / FILENAME).is_file(), + "never deleted: a fresh clone still has the card") + def test_attribution_is_consumed_once_and_expires(self): self.move(self.ada, "backlog", "to-do") sync.push_now()