diff --git a/.agent/handoff/241.md b/.agent/handoff/241.md new file mode 100644 index 0000000..b89d629 --- /dev/null +++ b/.agent/handoff/241.md @@ -0,0 +1,25 @@ +# Handoff note: issue #241 + +- issue: 241 +- started_at: 2026-10-14T12:31:26.475895Z +- reason: token_soft_threshold +- last_work_commit: 11f6b9bf07d96aeea85e3e1c2207a4eaab2d8f29 + +## Summary + +Implemented lifecycle memory for telemetry publication progress and workspace-dirty state in `simple_coding_agent/lifecycle.py` (uncommitted). Added `_observed_publication` / `_observed_workspace_dirty` dicts, `_remember_publication_result` / `_remember_workspace_dirty` / `_observe_and_remember_workspace_dirty` helpers, and hooked `_publish_bounded`, `_publish_terminal`, recovery-retry publish, `DirtyWorkspaceError` prepare paths, `_hold_dirty_workspace`/`_emit_workspace_hold`, and successful cleanup paths. Still pending: status/telemetry recovery `workspace_dirty` fields and telemetry remembered-value serving. + +## Remaining work + +1. Finish `_recovery_snapshot_locked` (status): add live `workspace_dirty` field to all three return dicts (startup-hold, dirty-tree hold, checkpoint-present) without changing live GitHub lookups. +2. Finish `_recovery_snapshot_network_free` (telemetry): serve remembered `publication.comment_confirmed`/`pull_request` plus remembered `workspace_dirty` (None when nothing observed); add `workspace_dirty` field; make no tracker/publisher/workspace method calls (only `working_directory` attr reads + remembered dicts). +3. Patch remaining `DirtyWorkspaceError` prepare paths if missed (release) and retry/release cleanup success paths to observe dirty. +4. Add regression test in `tests/test_telemetry.py`: push + failed publication (comment_posted=False, PR known) leading to recovery hold; assert telemetry shows known PR/comment while exploding tracker fake fails on any telemetry-path call; assert `agentctl status` still does live lookups. +5. Update `docs/agents/telemetry-snapshot.md` null-means-unknown bullet to remembered semantics if needed. +6. Run gate: ruff check, pytest py3.11, pytest py3.13, then `code-review` skill, then commit. + +## Evidence + +- Modified file: `simple_coding_agent/lifecycle.py` (git diff --stat: 1 file, ~133 insertions). +- Relevant code: `_remember_publication_result`, `_observe_and_remember_workspace_dirty`, `_publish_bounded`, `_publish_terminal`, `_retry_retained_attempt`, `_hold_dirty_workspace`, `_emit_workspace_hold`. +- Tests to check: `tests/test_telemetry.py` (esp. `test_telemetry_reports_a_retained_hold_without_lookups`), `tests/test_recovery.py`, `tests/test_lifecycle_control.py`. diff --git a/docs/agents/telemetry-snapshot.md b/docs/agents/telemetry-snapshot.md index 978a22e..93e0249 100644 --- a/docs/agents/telemetry-snapshot.md +++ b/docs/agents/telemetry-snapshot.md @@ -93,7 +93,11 @@ fields (ids, timestamps, enums, booleans) are bare values, not figures. `last_finished: null` is no finished attempt known. Likewise a null `pull_request` inside `last_finished` means no PR, and null `comment_confirmed`/`pull_request` inside `control.recovery.publication` - mean "not known yet" (the snapshot performs no network I/O). + or null `workspace_dirty` inside `control.recovery` mean "not observed + yet in this process": while an attempt is publishing or held in recovery, + telemetry carries the publication progress and workspace-dirty state the + lifecycle itself last observed (no network or git I/O on the telemetry + path), and `agentctl status` keeps its live lookups for the same fields. ## Versioning rules diff --git a/simple_coding_agent/lifecycle.py b/simple_coding_agent/lifecycle.py index a1ae77a..be562da 100644 --- a/simple_coding_agent/lifecycle.py +++ b/simple_coding_agent/lifecycle.py @@ -1220,6 +1220,16 @@ def __init__( # ``control_observed_at``. Served again while the control lock is # busy; ``None`` until the first successful build. self._telemetry_control: dict | None = None + # Last publication progress and workspace-dirty state the lifecycle + # itself observed, keyed by stable attempt ID. Publication flows + # (live attempts, startup reconciliation, recovery retry) record the + # publisher's verified result here; dirty handlers and cleanup + # outcomes record the workspace state. Telemetry serves these + # remembered values without network or git I/O; they stay absent + # (``None``) until something is observed in this process. ``status`` + # keeps its live lookups and never reads these. + self._observed_publication: dict[str, dict] = {} + self._observed_workspace_dirty: dict[str, bool] = {} @property def active_issue_number(self) -> int | None: @@ -1687,6 +1697,82 @@ def _workspace_is_dirty(self) -> bool: except Exception: return True + def _remember_publication_result( + self, attempt_id: str, published: object, phase: str | None = None + ) -> None: + """Remember the publisher's verified result for telemetry. + + Records ``comment_confirmed`` from ``comment_posted`` and the pull + request number/url when the publisher returns one. Called in the + publication flows (live attempts, startup reconciliation, recovery + retry) where the lifecycle already holds the verified result, so + telemetry can later serve it without network I/O. Never raises. + """ + + if not isinstance(attempt_id, str) or not attempt_id: + return + try: + try: + comment_confirmed: bool | None = bool(published.comment_posted) + except AttributeError: + comment_confirmed = None + raw_pr = getattr(published, "pull_request", None) + pull_request: dict | None = None + if raw_pr is not None: + if isinstance(raw_pr, dict): + number = raw_pr.get("number") + url = raw_pr.get("url") + else: + number = getattr(raw_pr, "number", None) + url = getattr(raw_pr, "url", None) + # A publisher that returns a PR object without a number still + # counts as an observation (known, but unnumbered). + pull_request = {"number": number, "url": url} + resolved_phase = phase + if resolved_phase is None: + try: + checkpoint = self._attempt_state.read() + if ( + checkpoint is not None + and checkpoint.started_at == attempt_id + ): + resolved_phase = checkpoint.phase.value + except Exception: + resolved_phase = None + self._observed_publication[attempt_id] = { + "phase": resolved_phase, + "comment_confirmed": comment_confirmed, + "pull_request": copy.deepcopy(pull_request) + if pull_request is not None + else None, + } + except Exception: + return + + def _remember_workspace_dirty(self, attempt_id: str, dirty: bool) -> None: + """Remember the workspace-dirty state for telemetry (never raises).""" + + if not isinstance(attempt_id, str) or not attempt_id: + return + try: + self._observed_workspace_dirty[attempt_id] = bool(dirty) + except Exception: + return + + def _observe_and_remember_workspace_dirty(self, attempt_id: str) -> None: + """Best-effort dirty check after publication; telemetry remembers it. + + Runs only in publication flows (which already perform git I/O), never + on the telemetry path. An unreadable tree records nothing, leaving + the remembered value ``None`` (not known yet). + """ + + try: + dirty = not self._workspace.is_clean() + except Exception: + return + self._remember_workspace_dirty(attempt_id, dirty) + def _dirty_tree_hold_locked(self) -> bool: """Block the claim while the tree holds unexplained work (no exit). @@ -1895,6 +1981,7 @@ def _retry_retained_attempt( base_branch=profile.base_branch, issue_number=claim.issue.number ) except DirtyWorkspaceError as error: + self._remember_workspace_dirty(attempt_id, True) return False, ( f"recovery retry for attempt {attempt_id} not fulfilled:" f" {_exception_detail(error)} The branch, checkpoint, and" @@ -1951,6 +2038,8 @@ def _retry_retained_attempt( " the hold remains." f" {hold_suffix}" ) + self._remember_publication_result(attempt_id, published) + self._observe_and_remember_workspace_dirty(attempt_id) outcome = published.outcome comment_posted = getattr(published, "comment_posted", True) if not comment_posted: @@ -1987,6 +2076,7 @@ def _retry_retained_attempt( # above), so only genuinely unpublished work retains. retain_branch=bool(retain_branch), ) + self._observe_and_remember_workspace_dirty(attempt_id) except DirtyWorkspaceError as error: # Cleanup refused to leave the attempt branch while the tree is # dirty: the hold remains with everything preserved for @@ -2083,6 +2173,7 @@ def _release_retained_attempt( base_branch=profile.base_branch, issue_number=claim.issue.number ) except DirtyWorkspaceError as error: + self._remember_workspace_dirty(attempt_id, True) return False, ( f"recovery release for attempt {attempt_id} not fulfilled:" f" {_exception_detail(error)} The branch, checkpoint, and" @@ -2457,7 +2548,8 @@ def _recovery_snapshot_locked( # the startup hold (read-only; never logs from status) with # the dirty paths when the tree is dirty too. reason = self._startup_hold_detail - if self._workspace_is_dirty(): + workspace_dirty = self._workspace_is_dirty() + if workspace_dirty: suffix = self._dirty_paths_suffix() reason += " The working tree is also dirty" + ( suffix if suffix else "." @@ -2469,6 +2561,7 @@ def _recovery_snapshot_locked( "outcome": None, "branch": None, "workspace": str(self._workspace.working_directory), + "workspace_dirty": workspace_dirty, "checkpoint": False, "publication": { "phase": None, @@ -2492,6 +2585,7 @@ def _recovery_snapshot_locked( "outcome": None, "branch": None, "workspace": str(self._workspace.working_directory), + "workspace_dirty": True, "checkpoint": False, "publication": { "phase": None, @@ -2537,6 +2631,7 @@ def _recovery_snapshot_locked( "outcome": outcome, "branch": checkpoint.branch, "workspace": str(self._workspace.working_directory), + "workspace_dirty": self._workspace_is_dirty(), "checkpoint": True, "publication": publication, "hold_reason": describe_hold( @@ -2823,9 +2918,10 @@ def telemetry_control_snapshot(self) -> dict: The same payload :meth:`control_status` builds, but without workspace or GitHub I/O: no dirty-tree check runs, no tracker or publisher call - is made, and publication fields that would need a lookup stay ``None`` - ("not known yet"). The section carries ``control_observed_at`` saying - when it was built. + is made, and the publication progress and workspace-dirty state are + served from what the lifecycle itself last observed (``None``/"not + known yet" until something is observed in this process). The section + carries ``control_observed_at`` saying when it was built. Waits at most ``CONTROL_LOCK_TIMEOUT_SECONDS`` for the control lock so telemetry never holds up other control commands. While the lock is @@ -2956,9 +3052,11 @@ def _recovery_snapshot_network_free( Mirrors :meth:`_recovery_snapshot_locked`: the workspace dirty check is skipped (a dirty tree alone never produces a section here) and the - publication progress stays ``None`` ("not known yet") because the - GitHub lookups never run. Never touches the tracker, publisher, or - workspace methods. Caller holds the control lock. + publication progress and workspace-dirty state are served from what + the lifecycle itself last observed (``None``/"not known yet" until + something is observed in this process) because the GitHub lookups + never run. Never touches the tracker, publisher, or workspace + methods. Caller holds the control lock. """ from simple_coding_agent.recovery import ( @@ -2977,6 +3075,7 @@ def _recovery_snapshot_network_free( "outcome": None, "branch": None, "workspace": str(self._workspace.working_directory), + "workspace_dirty": None, "checkpoint": False, "publication": { "phase": None, @@ -3003,11 +3102,24 @@ def _recovery_snapshot_network_free( outcome = terminal.outcome.value except Exception: outcome = None - publication = { - "phase": checkpoint.phase.value, - "comment_confirmed": None, - "pull_request": None, - } + remembered = self._observed_publication.get(checkpoint.started_at) + if remembered is not None: + publication = { + "phase": remembered.get("phase") + if remembered.get("phase") is not None + else checkpoint.phase.value, + "comment_confirmed": remembered.get("comment_confirmed"), + "pull_request": copy.deepcopy(remembered.get("pull_request")) + if remembered.get("pull_request") is not None + else None, + } + else: + publication = { + "phase": checkpoint.phase.value, + "comment_confirmed": None, + "pull_request": None, + } + workspace_dirty = self._observed_workspace_dirty.get(checkpoint.started_at) if self._recovering: next_action = ( "Wait for startup reconciliation to finish; do not run recovery" @@ -3027,6 +3139,7 @@ def _recovery_snapshot_network_free( "outcome": outcome, "branch": checkpoint.branch, "workspace": str(self._workspace.working_directory), + "workspace_dirty": workspace_dirty, "checkpoint": True, "publication": publication, "hold_reason": describe_hold( @@ -3176,6 +3289,7 @@ def run_once(self) -> LifecycleResult: # the finally boundary below logs ``attempt_finalization_held`` # and retains the checkpoint, so the next poll holds on the # retained attempt instead of claiming new work. + self._remember_workspace_dirty(checkpoint.started_at, True) self._event_log( "workspace_hold", f"{_exception_detail(error)} The branch, checkpoint, and working tree " @@ -3277,6 +3391,9 @@ def run_once(self) -> LifecycleResult: retain_branch=retain_branch or not comment_posted, ) cleanup_ok = True + self._observe_and_remember_workspace_dirty( + checkpoint.started_at + ) except DirtyWorkspaceError as error: # Cleanup refused to leave the attempt branch while the # tree is dirty: hold intake with everything preserved @@ -3632,8 +3749,14 @@ def _publish_bounded(self, request: PublicationRequest): budget = self._handoff_publish_budget() if budget is None: - return self._publisher.publish(request) - return self._publisher.publish(request, timeout=budget) + published = self._publisher.publish(request) + else: + published = self._publisher.publish(request, timeout=budget) + # Remember the verified result for telemetry (no extra lookup), plus + # the workspace state observed right after publication. + self._remember_publication_result(request.started_at, published) + self._observe_and_remember_workspace_dirty(request.started_at) + return published def _handoff_completion_cause( self, @@ -4004,6 +4127,11 @@ def _hold_dirty_workspace(self, issue_number: int, error: DirtyWorkspaceError) - inspection. """ + try: + attempt_id = _attempt_id(self._attempt_state.read()) + self._remember_workspace_dirty(attempt_id, True) + except Exception: + pass self._emit_workspace_hold(issue_number, error) def _emit_workspace_hold(self, issue_number: int, error: DirtyWorkspaceError) -> None: @@ -4014,6 +4142,11 @@ def _emit_workspace_hold(self, issue_number: int, error: DirtyWorkspaceError) -> best-effort from the tree. """ + try: + attempt_id = _attempt_id(self._attempt_state.read()) + self._remember_workspace_dirty(attempt_id, True) + except Exception: + pass detail = ( f"{_exception_detail(error)} The branch, checkpoint, and working tree " "are preserved for inspection; issue intake is held until the " @@ -4318,6 +4451,7 @@ def _reconcile_startup(self) -> AttemptOutcome | None: # the finally boundary below logs ``attempt_finalization_held`` # and retains the checkpoint, so the next poll holds on the # retained attempt instead of claiming new work. + self._remember_workspace_dirty(checkpoint.started_at, True) self._event_log( "workspace_hold", f"{_exception_detail(error)} The branch, checkpoint, and working tree " @@ -4411,6 +4545,9 @@ def _reconcile_startup(self) -> AttemptOutcome | None: retain_branch=retain_branch or not comment_posted, ) cleanup_ok = True + self._observe_and_remember_workspace_dirty( + checkpoint.started_at + ) except DirtyWorkspaceError as error: # Cleanup refused to leave the attempt branch while the # tree is dirty: hold intake with everything preserved @@ -4497,8 +4634,12 @@ def _publish_terminal( requeue_source=requeue_source, ) if timeout is None: - return self._publisher.publish(request) - return self._publisher.publish(request, timeout=timeout) + published = self._publisher.publish(request) + else: + published = self._publisher.publish(request, timeout=timeout) + self._remember_publication_result(started_at, published) + self._observe_and_remember_workspace_dirty(started_at) + return published def _publish_rebase_conflict( self, diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index ebaefb3..dabae50 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -598,6 +598,127 @@ def black_hole(connection: socket.socket) -> None: assert elapsed < 8 +def test_telemetry_serves_remembered_publication_while_status_stays_live( + tmp_path: Path, +) -> None: + """Telemetry shows what publication itself observed; status stays live. + + After a push and a failed (unconfirmed) publication the attempt is held + for recovery with its checkpoint retained. Telemetry must report the + pull request and comment state the lifecycle itself observed — without + any tracker, publisher, or workspace call — while ``agentctl status`` + keeps its live GitHub lookups and live dirty-tree check. + """ + + from types import SimpleNamespace + + from simple_coding_agent.completion import AttemptOutcome + from simple_coding_agent.publication import PullRequest + + lifecycle, _, _ = make_lifecycle(tmp_path) + state = AttemptStateStore(tmp_path) + checkpoint = state.start(issue_number=24, branch="agent/issue-24") + assert checkpoint is not None + attempt_id = checkpoint.started_at + + workspace = lifecycle._workspace # noqa: SLF001 + workspace.make_dirty("work.txt", "uncommitted work") + + live_calls: list[str] = [] + + class LiveTransport: + """Live GitHub answers that deliberately differ from memory.""" + + def find_attempt_comment( + self, repository: str, issue_number: int, marker: str + ) -> bool: + live_calls.append("find_attempt_comment") + assert attempt_id in marker + return True + + def find_pull_request(self, repository: str, head: str): + live_calls.append("find_pull_request") + return PullRequest(number=9, url="https://example.test/9") + + remembered_pr = PullRequest(number=7, url="https://example.test/7") + + class PushThenUnconfirmedPublisher: + """The push succeeded (known PR) but the result comment is unconfirmed.""" + + _repository = "owner/repo" + + def __init__(self) -> None: + self._github = LiveTransport() + + def publish( + self, request: object, *, timeout: float | None = None + ): + return SimpleNamespace( + outcome=AttemptOutcome.COMPLETE, + branch_url="https://example.test/tree/agent/issue-24", + pull_request=remembered_pr, + comment_posted=False, + ) + + lifecycle._publisher = PushThenUnconfirmedPublisher() # noqa: SLF001 + claim = SimpleNamespace( + issue=SimpleNamespace(number=24, title="Implement recovery") + ) + # Drive the real publication seam so the remember hook is exercised, not + # the remember helper directly. + lifecycle._publish_terminal( # noqa: SLF001 + claim, attempt_id, None, None, AttemptOutcome.COMPLETE + ) + # The tree is clean again by the time anyone looks: the live status check + # must see clean while telemetry still reports what publication itself + # observed (dirty). + workspace.dirty.clear() + + # From here the telemetry path must make no calls: the tracker explodes + # on any method, the workspace refuses any inspection, and the live + # transport counts every GitHub lookup. + lifecycle._tracker = ExplodingTracker() # noqa: SLF001 + original_is_clean = workspace.is_clean + original_dirty_paths = workspace.dirty_paths + workspace.is_clean = lambda: (_ for _ in ()).throw( # noqa: SLF001 + AssertionError("is_clean must not run on the telemetry path") + ) + workspace.dirty_paths = lambda: (_ for _ in ()).throw( # noqa: SLF001 + AssertionError("dirty_paths must not run on the telemetry path") + ) + try: + snapshot = lifecycle.telemetry_snapshot() + finally: + workspace.is_clean = original_is_clean # noqa: SLF001 + workspace.dirty_paths = original_dirty_paths # noqa: SLF001 + + recovery = snapshot["control"]["recovery"] + assert recovery is not None + assert recovery["attempt_id"] == attempt_id + assert recovery["publication"] == { + "phase": checkpoint.phase.value, + "comment_confirmed": False, + "pull_request": {"number": 7, "url": "https://example.test/7"}, + } + assert recovery["workspace_dirty"] is True + assert live_calls == [] + + # ``agentctl status`` keeps its live lookups: it reports what GitHub and + # the tree say now, not what the lifecycle remembered. + status = lifecycle.control_status("owner/repo") + + live = status["recovery"] + assert live is not None + assert live["publication"]["comment_confirmed"] is True + assert live["publication"]["pull_request"] == { + "number": 9, + "url": "https://example.test/9", + } + assert live["workspace_dirty"] is False + assert "find_attempt_comment" in live_calls + assert "find_pull_request" in live_calls + + def test_telemetry_uses_a_short_timeout_while_other_commands_keep_theirs() -> None: import simple_coding_agent.telemetry as telemetry_module