Address Copilot review: no-identity beat, unwritable log, remote phase base
Three points from the PR #41 Copilot review, each a robustness gap on a path the happy case never takes: - phases._mine() gated the beat on the assignee even when this checkout has no git name — but agents.claim_for_launch() cannot gate a launch there and lets it through, so a phase could start (branch cut, run recorded) and then advance nowhere. _mine() now treats "no local identity" as the lone actor, matching the launch it mirrors. - phases._record() ignored whether the log line landed. The log is the durable memory a restart reads to tell "already started" from "not reached yet"; a launch or merge with no line behind it is what a restart repeats. _record() now raises _Halt when the write fails — before the action it was meant to precede — split from a best-effort _write_log() the halt path and _start() use so recording a halt can never itself raise. - github._pr_base() switched a member PR's base to the phase branch only when it existed locally. A board that did not run the phase knows it only through the remote (sync fetches origin/main and nothing else), so _pr_base() now also honours a phase branch the remote carries, and _open_pr() only pushes the base when it is a local branch. Four new tests in tests/test_phase_runs.py cover each. Full suite green (740). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+22
-3
@@ -74,17 +74,33 @@ def _woven(filename: str, stage: str) -> dict:
|
||||
return read_task(config.TASKS / stage / filename, stage)
|
||||
|
||||
|
||||
def _remote_has_branch(rname: str, branch: str) -> bool:
|
||||
"""Whether the remote already carries this branch. Sync only ever
|
||||
fetches origin/main, so a phase branch the running board pushed is not a
|
||||
local ref on any other board even when it is published — asking the
|
||||
remote directly is the only way another board sees it."""
|
||||
return bool(_run(["git", "ls-remote", "--heads", rname, branch],
|
||||
timeout=30).stdout.strip())
|
||||
|
||||
|
||||
def _pr_base(task: dict) -> str:
|
||||
"""What a PR is opened against. A phase member's branch was cut from
|
||||
its phase's branch, so that is the only base whose diff is the member's
|
||||
own work — a PR into main would carry the whole phase, and invite a
|
||||
merge into main that this board exists not to make. Everything else,
|
||||
the phase card included, goes into main."""
|
||||
the phase card included, goes into main.
|
||||
|
||||
The phase branch is the board's own and is published as the phase runs,
|
||||
so a board that did not run the phase may know it only through the
|
||||
remote — that still makes it the base, not main."""
|
||||
phase = task.get("phase")
|
||||
if phase:
|
||||
branch = f"phase/{phase['file'][:-3]}"
|
||||
if _branch_exists(branch):
|
||||
return branch
|
||||
rname = remote()
|
||||
if rname and _remote_has_branch(rname, branch):
|
||||
return branch
|
||||
return "main"
|
||||
|
||||
|
||||
@@ -195,9 +211,12 @@ def _open_pr(filename: str) -> str:
|
||||
if ahead.isdigit() and int(ahead) > 0:
|
||||
raise ValueError(f"won't open a PR for {filename}: main is {ahead} commits "
|
||||
f"ahead of {rname} — push main first, then move the card again")
|
||||
else:
|
||||
elif _branch_exists(base):
|
||||
# A phase branch is the board's own: publish it so the member's PR
|
||||
# has a base on the remote to be opened against.
|
||||
# has a base on the remote to be opened against. When only the
|
||||
# remote carries it — a member PR opened from a board that did not
|
||||
# run the phase — it is already there, and there is nothing local
|
||||
# to push.
|
||||
_run(["git", "push", "-u", rname, base], timeout=180)
|
||||
|
||||
push = _run(["git", "push", "-u", rname, branch], timeout=180)
|
||||
|
||||
+34
-8
@@ -114,15 +114,30 @@ def log_entries(text: str) -> list[str]:
|
||||
return entries
|
||||
|
||||
|
||||
def _record(phase: dict, entry: str) -> None:
|
||||
def _write_log(phase: dict, entry: str) -> bool:
|
||||
"""One line into the log, where it reaches git like every other write
|
||||
the board makes to a task file. The stage is asked of the disk: a pass
|
||||
that finishes a phase moves the card, and the record is written to
|
||||
wherever the card actually is."""
|
||||
wherever the card actually is. Returns whether the line landed."""
|
||||
stage = taskfiles.find_stage_of(phase["file"]) or phase["stage"]
|
||||
stamp = time.strftime("%Y-%m-%d %H:%M")
|
||||
taskfiles.append_to_section(phase["file"], stage, LOG_HEADING,
|
||||
f"- {stamp} · {entry}", "phase log")
|
||||
return taskfiles.append_to_section(phase["file"], stage, LOG_HEADING,
|
||||
f"- {stamp} · {entry}", "phase log")
|
||||
|
||||
|
||||
def _record(phase: dict, entry: str) -> None:
|
||||
"""Record a decision, and refuse to act if it cannot be written down.
|
||||
|
||||
The log is the durable memory a restart reads to tell "this member has
|
||||
already started" from "the phase has not reached it yet". An action that
|
||||
outran its own record — a launch or a merge with no line behind it —
|
||||
is exactly what a restarted board would repeat. So a log that will not
|
||||
take the line is itself a halt, raised here before the action it was
|
||||
meant to precede ever happens. The halt path writes best-effort
|
||||
(`_write_log`) so that recording the halt can never raise in turn."""
|
||||
if not _write_log(phase, entry):
|
||||
raise _Halt("could not write the phase log — refusing to act without "
|
||||
"the record a restart reads")
|
||||
|
||||
|
||||
def _this_run(entries: list[str]) -> list[str]:
|
||||
@@ -354,11 +369,20 @@ def _freshen(phase: dict) -> None:
|
||||
def _mine(phase: dict) -> bool:
|
||||
"""State syncs; reactions don't. Only the board whose user holds the
|
||||
phase card advances it — every replica renders the same phase and
|
||||
launches nothing. Outside team mode there is one board, and it acts."""
|
||||
launches nothing. Outside team mode there is one board, and it acts.
|
||||
|
||||
No local git identity is the one case that does not gate: it is exactly
|
||||
where `agents.claim_for_launch` cannot write an assignee and so cannot
|
||||
refuse a launch either. Gating the beat on it while the launch went
|
||||
through would strand a phase — branch cut, run recorded — that then
|
||||
never advances. So a board with no name is the lone actor here, the same
|
||||
as it is for starting work."""
|
||||
if not config.COMMIT_MOVES:
|
||||
return True
|
||||
me = taskfiles.actor_name()
|
||||
return bool(me) and phase.get("assignee") == me
|
||||
if not me:
|
||||
return True
|
||||
return phase.get("assignee") == me
|
||||
|
||||
|
||||
def _unfinished_dependencies(member: dict, snapshot: dict,
|
||||
@@ -428,7 +452,7 @@ def _halt(phase: dict, member: dict | None, reason: str) -> None:
|
||||
"""Stop, and say so once. The log holds the halt from here on, so the
|
||||
next pass reads it rather than saying the same thing again."""
|
||||
at = f" at {member['number']}" if member and member["number"] else ""
|
||||
_record(phase, f"halted{at} — {reason}")
|
||||
_write_log(phase, f"halted{at} — {reason}") # best effort: never re-raise
|
||||
_say(phase["file"], f"{phase['file']} halted{at} — {reason}")
|
||||
|
||||
|
||||
@@ -570,7 +594,9 @@ def _start(phase: dict, filename: str) -> dict:
|
||||
if result.returncode != 0:
|
||||
raise ValueError(f"could not cut {branch}: {result.stderr.strip()[:200]}")
|
||||
_push_phase(phase)
|
||||
_record(phase, f"run started on {branch}")
|
||||
if not _write_log(phase, f"run started on {branch}"):
|
||||
raise ValueError(f"could not record the run on {filename} — its phase "
|
||||
f"log must be writable to run the phase safely")
|
||||
_say(filename, f"phase {filename} is running on {branch}"
|
||||
+ (f" — {note}" if note else ""))
|
||||
state.broadcast({"type": "board"})
|
||||
|
||||
Reference in New Issue
Block a user