mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(docs): push the documenter's doc commit so it lands in the PR
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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user