diff --git a/simple_coding_agent/attempt_workspace.py b/simple_coding_agent/attempt_workspace.py index 3b59355..c509a9e 100644 --- a/simple_coding_agent/attempt_workspace.py +++ b/simple_coding_agent/attempt_workspace.py @@ -96,8 +96,22 @@ def handoff_note_commits( position-independent, and limited to this attempt (earlier rounds' note commits are excluded). The newest entry is what :meth:`handoff_note_commit` returns. Used by handoff-note validation - to skip the unbroken run of this attempt's note commits beneath the - newest one when deriving the expected ``last_work_commit``. + together with :meth:`handoff_note_commit_carries_work` to skip the + unbroken run of this attempt's note commits beneath the newest one + when deriving the expected ``last_work_commit``. + """ + ... + + def handoff_note_commit_carries_work(self, revision: str, issue_number: int) -> bool: + """Whether a note-touching commit is really a work commit. + + True only when ``revision`` deletes the issue's handoff note while + also changing other files -- a work commit that dropped a stale + note as a "process artifact" alongside a real change. Such a + commit ends the note run, so validation expects it as + ``last_work_commit``. A commit that leaves the note in place + (even with other files swept in) or that only touches the note + (including deleting just the note) is still part of the run. """ ... diff --git a/simple_coding_agent/git_workspace.py b/simple_coding_agent/git_workspace.py index 52b79fb..4f72fd1 100644 --- a/simple_coding_agent/git_workspace.py +++ b/simple_coding_agent/git_workspace.py @@ -446,6 +446,33 @@ def handoff_note_commit( ) return commits[0] if commits else None + def handoff_note_commit_carries_work(self, revision: str, issue_number: int) -> bool: + """Whether a note-touching commit is really a work commit. + + True only when ``revision`` deletes the issue's handoff note while + also changing other files. ``git log -- `` (which backs + :meth:`handoff_note_commits`) lists such a commit because it + touched the path, but it did so by removing a stale note, not by + writing one. + """ + + relpath = handoff_note_relpath(issue_number) + files = [ + line.strip() + for line in self._git( + "diff-tree", "--no-commit-id", "--name-only", "-r", "--root", revision, "--" + ).splitlines() + if line.strip() + ] + touches_other_files = any(path != relpath for path in files) + if not touches_other_files: + return False + try: + self._git("cat-file", "-e", f"{revision}:{relpath}") + except GitWorkspaceError: + return True + return False + def cleanup( self, *, base_branch: str, prepared: PreparedAttempt, retain_branch: bool = True ) -> None: diff --git a/simple_coding_agent/lifecycle.py b/simple_coding_agent/lifecycle.py index 47a1669..3b68015 100644 --- a/simple_coding_agent/lifecycle.py +++ b/simple_coding_agent/lifecycle.py @@ -4894,11 +4894,15 @@ def _this_attempt_note_commit_revisions( issue_number: int, attempt_id: str, ) -> frozenset[str]: - """Revisions of this attempt's note-touching commits for note validation. - - An unreadable note history fails closed to just the newest note commit, - which is exactly the pre-fix strictness: validation then expects the - commit directly beneath the note. + """Revisions of this attempt's note-run commits for note validation. + + The run is every note-touching commit of this attempt except work + commits that only touched the note path by deleting a stale note + alongside a real change: those end the run, so validation expects + them as ``last_work_commit`` instead of skipping past them. A + commit that cannot be inspected fails closed to work (it is not + skipped), which is exactly the pre-fix strictness: validation then + expects the commit directly beneath the note. """ try: @@ -4907,7 +4911,17 @@ def _this_attempt_note_commit_revisions( ) except Exception: return frozenset() - return frozenset(commit.revision for commit in commits) + run: list[str] = [] + for commit in commits: + try: + carries_work = workspace.handoff_note_commit_carries_work( + commit.revision, issue_number + ) + except Exception: + continue + if not carries_work: + run.append(commit.revision) + return frozenset(run) def _validate_handoff_note( @@ -4988,12 +5002,14 @@ def _validate_handoff_note( # when the note is the oldest attempt commit. When the model committed # the note more than once, the commits beneath the newest note commit # are earlier note commits of this same attempt, not work: skip the - # unbroken run of this attempt's note-touching commits below the note - # (whatever other files they also carry) and expect the first commit - # beneath that run. Note commits from earlier rounds of a continued - # branch are never in ``note_commit_revisions``, so a new round that - # names the previous round's note commit (HEAD at injection) still - # validates. + # unbroken run of this attempt's note-run commits below the note and + # expect the first commit beneath that run. A note-touching commit + # that deleted the note while carrying other files is work that + # dropped a stale note, not a note commit, so it is already excluded + # from ``note_commit_revisions`` and ends the run. Note commits from + # earlier rounds of a continued branch are never in + # ``note_commit_revisions``, so a new round that names the previous + # round's note commit (HEAD at injection) still validates. if note_commit_revisions is None: note_revisions = {note_commit.revision} else: diff --git a/tests/fakes.py b/tests/fakes.py index 869023f..5193a6a 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -97,6 +97,7 @@ class _RecordedCommit: body: str files: frozenset[str] prior_round: bool = False + existing: frozenset[str] = frozenset() class InMemoryWorkspace: @@ -285,6 +286,14 @@ def handoff_note_commit( ) return commits[0] if commits else None + def handoff_note_commit_carries_work(self, revision: str, issue_number: int) -> bool: + relpath = handoff_note_relpath(issue_number) + record = next((entry for entry in self._commits if entry.revision == revision), None) + if record is None: + return True + touches_other_files = any(name != relpath for name in record.files) + return touches_other_files and relpath not in record.existing + def cleanup( self, *, base_branch: str, prepared: PreparedAttempt, retain_branch: bool ) -> None: @@ -315,6 +324,9 @@ def _record_commit( body=body, files=files, prior_round=prior_round, + existing=frozenset( + name for name in files if (self.working_directory / name).exists() + ), ) self._commits.append(record) if clear_dirty: diff --git a/tests/test_attempt_workspace_conformance.py b/tests/test_attempt_workspace_conformance.py index efb807e..23317b4 100644 --- a/tests/test_attempt_workspace_conformance.py +++ b/tests/test_attempt_workspace_conformance.py @@ -433,6 +433,84 @@ def test_handoff_note_commits_is_empty_without_a_note( ) +def test_handoff_note_commit_carries_work_is_false_for_a_note_only_commit( + attempt: SimpleNamespace, +) -> None: + note = attempt.workspace.commit_handoff_note(24, "# Handoff note: issue #24\n") + + assert attempt.workspace.handoff_note_commit_carries_work(note.revision, 24) is False + + +def test_handoff_note_commit_carries_work_is_false_for_a_note_commit_with_other_files( + attempt: SimpleNamespace, +) -> None: + """A note commit with other files swept in is still a note commit (#34).""" + + if attempt.kind == "git": + note_path = attempt.root / ".agent" / "handoff" / "24.md" + note_path.parent.mkdir(parents=True, exist_ok=True) + note_path.write_text("# Handoff note: issue #24\n") + (attempt.root / "work.txt").write_text("work") + git(attempt.root, "add", ".agent/handoff/24.md", "work.txt") + git(attempt.root, "commit", "-m", "Handoff note: issue #24") + note = attempt.workspace.handoff_note_commit( + attempt.prepared, 24, attempt_id=attempt.attempt_id + ) + assert note is not None + revision = note.revision + else: + attempt.workspace.make_dirty(".agent/handoff/24.md", "# Handoff note: issue #24\n") + attempt.workspace.make_dirty("work.txt", "work") + revision = attempt.workspace.model_commit( + "Handoff note: issue #24", + files=(".agent/handoff/24.md", "work.txt"), + ).revision + + assert attempt.workspace.handoff_note_commit_carries_work(revision, 24) is False + + +def test_handoff_note_commit_carries_work_is_false_for_a_note_only_deletion( + attempt: SimpleNamespace, +) -> None: + """Deleting just the note carries no work, so it stays in the run.""" + + model_commit(attempt, ".agent/handoff/24.md", "# Handoff note: issue #24\n", "Stale note") + if attempt.kind == "git": + git(attempt.root, "rm", ".agent/handoff/24.md") + git(attempt.root, "commit", "-m", "Remove agent handoff note") + deletion = attempt.workspace.commits_added(attempt.prepared)[0].revision + else: + (attempt.root / ".agent" / "handoff" / "24.md").unlink() + deletion = attempt.workspace.model_commit( + "Remove agent handoff note", files=(".agent/handoff/24.md",) + ).revision + + assert attempt.workspace.handoff_note_commit_carries_work(deletion, 24) is False + + +def test_handoff_note_commit_carries_work_is_true_for_work_that_deletes_the_note( + attempt: SimpleNamespace, +) -> None: + """Work that drops a stale note alongside a real change ends the run (#216).""" + + model_commit(attempt, ".agent/handoff/24.md", "# Handoff note: issue #24\n", "Stale note") + if attempt.kind == "git": + (attempt.root / "work.txt").write_text("work") + git(attempt.root, "rm", ".agent/handoff/24.md") + git(attempt.root, "add", "work.txt") + git(attempt.root, "commit", "-m", "Do work, drop the stale handoff note") + work = attempt.workspace.commits_added(attempt.prepared)[0].revision + else: + attempt.workspace.make_dirty("work.txt", "work") + (attempt.root / ".agent" / "handoff" / "24.md").unlink() + work = attempt.workspace.model_commit( + "Do work, drop the stale handoff note", + files=(".agent/handoff/24.md", "work.txt"), + ).revision + + assert attempt.workspace.handoff_note_commit_carries_work(work, 24) is True + + def test_cleanup_on_a_dirty_tree_stays_on_the_attempt_branch( attempt: SimpleNamespace, ) -> None: diff --git a/tests/test_handoff_end_to_end.py b/tests/test_handoff_end_to_end.py index 7965d8a..3471537 100644 --- a/tests/test_handoff_end_to_end.py +++ b/tests/test_handoff_end_to_end.py @@ -711,6 +711,46 @@ def do_handoff(working_directory: Path) -> None: assert "Halfway done, with more detail." in comment.body +def test_handoff_note_names_work_commit_that_deleted_a_stale_note(tmp_path: Path) -> None: + """The #216 shape: a work commit deletes a stale note alongside a code change. + + The work commit touches the handoff note path (by dropping an earlier + round's note as a "process artifact"), so it shows up in this attempt's + note-touching commits -- but it deletes the note instead of writing it, + so it ends the note run: the note names it as ``last_work_commit`` and + the attempt still publishes as a ``handoff``. + """ + + remote, _ = repository_with_main(tmp_path) + github = FakeGitHub(issue(24, body="Rewrite the parser.", labels=frozenset({"ready-for-agent"}))) + clone_dir = tmp_path / "clone" + + def do_handoff(working_directory: Path) -> None: + # A stale note from an earlier round, still on the branch. + write_handoff_note(working_directory, 24, "Stale note from an earlier round.\n") + # This round's work: a code change that also drops the stale note. + (working_directory / "parser.py").write_text("half done") + git(working_directory, "rm", ".agent/handoff/24.md") + git(working_directory, "add", "parser.py") + git(working_directory, "commit", "-m", "Half-finish the parser, drop the stale handoff note") + work = git(working_directory, "rev-parse", "HEAD") + write_handoff_note(working_directory, 24, "Halfway done.\n", last_work_commit=work) + write_handoff_note( + working_directory, 24, "Halfway done, with more detail.\n", last_work_commit=work + ) + + executor = FakeModelExecutor(actions=[do_handoff], handoff=True) + lifecycle = build_lifecycle(remote, clone_dir, tmp_path / "data", github, executor) + + result = lifecycle.run_once() + + assert result.outcome is AttemptOutcome.HANDOFF + assert github.pull_requests == {} + assert github.added_labels == [("issue-24", "round-finished")] + [comment] = [c for c in github.comments if "Agent Attempt Result: handoff" in c.body] + assert "Halfway done, with more detail." in comment.body + + def test_handoff_note_with_work_swept_into_first_note_names_the_base(tmp_path: Path) -> None: """The #34 shape: the first note commit also carries another staged file.