From ebc3b1ea1841651ea956bfb3ba88cf6e4ed74214 Mon Sep 17 00:00:00 2001 From: Renn F Date: Tue, 23 Jun 2026 00:39:08 +0200 Subject: [PATCH] fix(docs): push the documenter's doc commit so it lands in the PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documenter writes and commits docs onto the task branch in its own workspace clone, but i_documented had no push step — so the commit stayed local and the PM merged the already-open PR without the docs, which then vanished on merge. i_documented now pushes the task branch before handoff, mirroring the developer's _ensure_branch_pushed; a push failure holds the task in awaiting_documentation for a retry instead of silently dropping the docs. Extract _finalize_documented to keep the verb under the return-count ceiling. --- CHANGELOG.md | 1 + roboco/services/gateway/choreographer/doc.py | 62 ++++++++++++++++ tests/unit/gateway/test_choreographer_doc.py | 74 ++++++++++++++++++++ 3 files changed, 137 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa9c138..53c05bfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Fixed +- **Documentation now actually lands in the project repo.** A documenter's output reached a host-mounted, RAG-indexed knowledge store and (more recently) was committed onto the task branch — but in the documenter's own workspace clone, and nothing ever pushed that commit, so the PM merged the already-open PR without the docs and the deliverable vanished on merge. The documenter's `i_documented` now pushes the task branch before handing off (mirroring the developer's pre-QA push), so the doc commit rides the open PR into the repository; a push failure holds the task in `awaiting_documentation` for a retry instead of silently dropping the docs. - **The conventions standard now resolves for projects created before it existed.** It previously read the committed `.roboco/conventions.yml` and the repo scan from `project.workspace_path` — a field only a manual API call ever set — so an older project (or one whose workspace was cleared) showed an empty "missing" map no matter what was pushed. The service now ensures a dedicated, default-branch read clone on demand and reads from it, persisting the resolved path + HEAD (the backfill). The panel tab, the spawn-time ambient block, and the per-task constraints all resolve the committed standard with no manual setup. - **The conventions ambient prompt block no longer truncates mid-line.** It now lists only modules that actually constrain a kind, and when the list would exceed its budget it trims at a line boundary with a `+N more` pointer instead of cutting a module in half. - **The conventions read clone now stays current on a private repo.** Its refresh reused the orchestrator's token-less best-effort fetch, but the clone's remote URL is credential-stripped — so on a private repo the refresh fetch failed silently and the clone stayed frozen at clone-time, never seeing commits merged afterwards (the panel showed "auto-derived defaults" even after the standard was merged to the default branch). The refresh now performs a token-authenticated fetch + hard-reset, mirroring the clone. diff --git a/roboco/services/gateway/choreographer/doc.py b/roboco/services/gateway/choreographer/doc.py index a6db5188..c0ddc95a 100644 --- a/roboco/services/gateway/choreographer/doc.py +++ b/roboco/services/gateway/choreographer/doc.py @@ -454,6 +454,41 @@ class DocMixin(_Base): if gate_rejection is not None: return gate_rejection + # Push the doc commit to origin BEFORE the transition, so it reaches the + # already-open PR (the PM merges the pushed branch). Without this the + # doc commit lives only in the documenter's clone and the merge drops it. + if push_rejection := await self._ensure_doc_branch_pushed( + doc_agent_id, task_id, briefing + ): + return push_rejection + + return await self._finalize_documented( + doc_agent_id, + task_id, + files, + owned_task, + agent, + role_str, + spec_ctx, + briefing, + ) + + async def _finalize_documented( + self, + doc_agent_id: UUID, + task_id: UUID, + files: list[str], + owned_task: Any, + agent: Any, + role_str: str, + spec_ctx: Any, + briefing: Any, + ) -> Envelope: + """Stamp docs, dispatch the docs_complete transition, hand off to the PM. + + Split out of ``i_documented`` so its body stays under the + return-statement / cyclomatic-complexity ceilings. + """ # TaskService.docs_complete signature is (task_id, doc_notes); it # reads task.documents for indexing. Stamp the file list onto the # task before the runner dispatches docs_complete so the indexer @@ -486,6 +521,33 @@ class DocMixin(_Base): context_briefing=briefing, ).with_introspection(task=t, role=role_str) + async def _ensure_doc_branch_pushed( + self, doc_agent_id: UUID, task_id: UUID, briefing: Any + ) -> Envelope | None: + """Push the documenter's doc commit to origin before awaiting_pm_review. + + The documenter writes docs onto the task branch in its own workspace + clone (via write_doc or the gateway commit tool), but the PR is already + open and nothing pushes the new commit — so without this the PM merges a + PR that excludes the docs and the deliverable never lands in the repo. + Mirrors the developer's ``_ensure_branch_pushed``. Idempotent: a no-op + when nothing is unpushed. A push failure holds the task (no transition) + so the documenter retries rather than silently losing the docs. + """ + try: + await self.git.push_task_branch(doc_agent_id, task_id) + except Exception as exc: + return Envelope.invalid_state( + message=f"could not push your documentation to origin: {exc}", + remediate=( + "your doc commits are local-only and the PM merges the " + "pushed PR branch. resolve the push error (often a transient " + "network / fetch timeout) and call i_documented again." + ), + context_briefing=briefing, + ) + return None + async def _handoff_to_cell_pm( self, doc_agent_id: UUID, task_id: UUID, task: Any ) -> None: diff --git a/tests/unit/gateway/test_choreographer_doc.py b/tests/unit/gateway/test_choreographer_doc.py index 6e17e4eb..512cc650 100644 --- a/tests/unit/gateway/test_choreographer_doc.py +++ b/tests/unit/gateway/test_choreographer_doc.py @@ -236,6 +236,80 @@ async def test_i_documented_succeeds_and_transitions() -> None: a2a_svc.send.assert_awaited_once() +def _doc_success_task_svc(task_id: Any, doc_id: Any) -> AsyncMock: + """A task service stubbed for a passing i_documented (awaiting_pm_review).""" + t = _doc_owned_task(task_id, doc_id) + after = MagicMock( + id=task_id, + status="awaiting_pm_review", + assigned_to=doc_id, + team="backend", + ) + task_svc = AsyncMock() + task_svc.get.return_value = t + task_svc.agent_for.return_value = _doc_agent_mock(doc_id) + task_svc.docs_complete.return_value = after + task_svc.cell_pm_for_team.return_value = MagicMock(id=uuid4()) + task_svc.session = MagicMock() + task_svc.session.flush = AsyncMock() + task_svc.session.begin_nested = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=None), + __aexit__=AsyncMock(return_value=False), + ) + ) + return task_svc + + +@pytest.mark.asyncio +async def test_i_documented_pushes_doc_commit() -> None: + """The documenter's doc commit must be PUSHED so it reaches the open PR. + + The PR is already open by the time docs are written; without a push the + doc commit lives only in the documenter's clone and the PM merges a PR + that excludes the docs. Mirrors the developer's _ensure_branch_pushed. + """ + doc_id = uuid4() + task_id = uuid4() + task_svc = _doc_success_task_svc(task_id, doc_id) + git_svc = AsyncMock() + git_svc.push_task_branch.return_value = 1 + journal_svc = AsyncMock() + journal_svc.has_reflect_for_task.return_value = True + deps = _make_deps(task=task_svc, git=git_svc, journal=journal_svc) + c = Choreographer(deps) + + notes = "Wrote backend/guides/feature-x.md with usage examples and config." + env = await c.i_documented( + doc_id, task_id, notes=notes, files=["backend/guides/feature-x.md"] + ) + assert env.error is None + assert env.status == "awaiting_pm_review" + git_svc.push_task_branch.assert_awaited_once_with(doc_id, task_id) + + +@pytest.mark.asyncio +async def test_i_documented_push_failure_holds_task() -> None: + """A push failure must NOT transition — the docs are local-only, so the + documenter stays in awaiting_documentation and retries (no silent drop).""" + doc_id = uuid4() + task_id = uuid4() + task_svc = _doc_success_task_svc(task_id, doc_id) + git_svc = AsyncMock() + git_svc.push_task_branch.side_effect = RuntimeError("push rejected") + journal_svc = AsyncMock() + journal_svc.has_reflect_for_task.return_value = True + deps = _make_deps(task=task_svc, git=git_svc, journal=journal_svc) + c = Choreographer(deps) + + notes = "Wrote backend/guides/feature-x.md with usage examples and config." + env = await c.i_documented( + doc_id, task_id, notes=notes, files=["backend/guides/feature-x.md"] + ) + assert env.as_dict()["error"] == "invalid_state" + task_svc.docs_complete.assert_not_awaited() + + @pytest.mark.asyncio async def test_i_documented_not_assigned_returns_not_authorized() -> None: doc_id = uuid4()