mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(docs): i_documented persists DocRef dicts, not bare strings (#169)
Smoke-15: be-doc i_documented(files=["README.md"]) → choreographer
doc.py stamped `existing.documents = files` (a list[str]) onto
Task.documents. Task.documents is list[DocRef] persisted as dicts —
list_docs does DocRef(**d), _get_existing_doc_ref does d.get("path"),
the RAG indexer does d.get("path"). A bare string 500'd GET /docs
("TypeError: DocRef() argument after ** must be a mapping, not str")
during be-pm's PR review and would AttributeError the indexer.
Fix at source: new _doc_refs_for builds proper DocRef dicts
(path, title=filename, doc_type, created_by/at, updated_by/at) at the
i_documented stamp. Defensive read: new _coerce_doc_ref tolerates
dict / DocRef / bare-string / rejects unknown — applied at
list_docs and _get_existing_doc_ref so legacy/corrupted rows can't
500. _add_doc_to_task (docs-route write path, not the gateway flow
that broke) left as-is per scope.
This commit is contained in:
+20
-3
@@ -100,6 +100,22 @@ _SIMILARITY_THRESHOLD = 0.75
|
|||||||
_CONTENT_SUMMARY_LENGTH = 500
|
_CONTENT_SUMMARY_LENGTH = 500
|
||||||
|
|
||||||
|
|
||||||
|
def _coerce_doc_ref(d: object) -> DocRef:
|
||||||
|
"""Build a DocRef from a stored ``Task.documents`` element.
|
||||||
|
|
||||||
|
Canonical rows are dicts (``DocRef.model_dump()``). Defensive
|
||||||
|
against legacy/corrupted rows (#169): a bare path string is wrapped
|
||||||
|
instead of exploding ``DocRef(**str)`` and 500-ing the endpoint.
|
||||||
|
"""
|
||||||
|
if isinstance(d, DocRef):
|
||||||
|
return d
|
||||||
|
if isinstance(d, str):
|
||||||
|
return DocRef(path=d, title=Path(d).name, doc_type="doc")
|
||||||
|
if isinstance(d, dict):
|
||||||
|
return DocRef.model_validate(d)
|
||||||
|
raise TypeError(f"unsupported Task.documents element: {type(d).__name__}")
|
||||||
|
|
||||||
|
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
# SERVICE
|
# SERVICE
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
@@ -417,7 +433,7 @@ class DocsService(BaseService):
|
|||||||
task = result.scalar_one_or_none()
|
task = result.scalar_one_or_none()
|
||||||
if not task:
|
if not task:
|
||||||
raise NotFoundError("Task", str(task_id))
|
raise NotFoundError("Task", str(task_id))
|
||||||
return [DocRef(**d) for d in (task.documents or [])]
|
return [_coerce_doc_ref(d) for d in (task.documents or [])]
|
||||||
else:
|
else:
|
||||||
# Get agent's team and list files from filesystem
|
# Get agent's team and list files from filesystem
|
||||||
team = get_agent_team(agent_id)
|
team = get_agent_team(agent_id)
|
||||||
@@ -526,8 +542,9 @@ class DocsService(BaseService):
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
for doc in task.documents:
|
for doc in task.documents:
|
||||||
if doc.get("path") == path:
|
ref = _coerce_doc_ref(doc)
|
||||||
return DocRef(**doc)
|
if ref.path == path:
|
||||||
|
return ref
|
||||||
return None
|
return None
|
||||||
|
|
||||||
async def _add_doc_to_task(self, task_id: UUID, doc_ref: DocRef) -> None:
|
async def _add_doc_to_task(self, task_id: UUID, doc_ref: DocRef) -> None:
|
||||||
|
|||||||
@@ -32,12 +32,15 @@ requires.
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import contextlib
|
import contextlib
|
||||||
|
from datetime import UTC, datetime
|
||||||
|
from pathlib import Path
|
||||||
from types import SimpleNamespace
|
from types import SimpleNamespace
|
||||||
from typing import TYPE_CHECKING, Any
|
from typing import TYPE_CHECKING, Any
|
||||||
|
|
||||||
from roboco.config import settings
|
from roboco.config import settings
|
||||||
from roboco.foundation.policy import lifecycle as spec_module
|
from roboco.foundation.policy import lifecycle as spec_module
|
||||||
from roboco.foundation.policy import tracing as _tr
|
from roboco.foundation.policy import tracing as _tr
|
||||||
|
from roboco.models.task import DocRef
|
||||||
from roboco.services.gateway.envelope import Envelope
|
from roboco.services.gateway.envelope import Envelope
|
||||||
from roboco.services.gateway.evidence_builder import build_evidence_for_task
|
from roboco.services.gateway.evidence_builder import build_evidence_for_task
|
||||||
|
|
||||||
@@ -51,6 +54,30 @@ else:
|
|||||||
_Base = object
|
_Base = object
|
||||||
|
|
||||||
|
|
||||||
|
def _doc_refs_for(files: list[str], agent_id: UUID) -> list[dict[str, Any]]:
|
||||||
|
"""DocRef-shaped dicts for ``Task.documents`` (a JSON column).
|
||||||
|
|
||||||
|
``i_documented`` receives a flat list of file paths, but
|
||||||
|
``Task.documents`` is ``list[DocRef]`` persisted as dicts — readers do
|
||||||
|
``DocRef(**d)`` / ``d["path"]`` and the doc indexer does ``d.get``.
|
||||||
|
Stamping bare strings 500s ``list_docs`` and breaks indexing (#169).
|
||||||
|
"""
|
||||||
|
now = datetime.now(UTC).isoformat()
|
||||||
|
slug = str(agent_id)
|
||||||
|
return [
|
||||||
|
DocRef(
|
||||||
|
path=f,
|
||||||
|
title=Path(f).name,
|
||||||
|
doc_type="doc",
|
||||||
|
created_by=slug,
|
||||||
|
created_at=now,
|
||||||
|
updated_by=slug,
|
||||||
|
updated_at=now,
|
||||||
|
).model_dump()
|
||||||
|
for f in files
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def _extract_original_developer(task: Any) -> str | None:
|
def _extract_original_developer(task: Any) -> str | None:
|
||||||
"""Pull the original_developer slug out of a task's quick_context, if any.
|
"""Pull the original_developer slug out of a task's quick_context, if any.
|
||||||
|
|
||||||
@@ -385,7 +412,7 @@ class DocMixin(_Base):
|
|||||||
# sees it.
|
# sees it.
|
||||||
existing = await self.task.get(task_id)
|
existing = await self.task.get(task_id)
|
||||||
if existing is not None:
|
if existing is not None:
|
||||||
existing.documents = files
|
existing.documents = _doc_refs_for(files, doc_agent_id)
|
||||||
await self.task.session.flush()
|
await self.task.session.flush()
|
||||||
|
|
||||||
runner = self._verb_runner()
|
runner = self._verb_runner()
|
||||||
|
|||||||
@@ -0,0 +1,84 @@
|
|||||||
|
"""#169: i_documented must persist DocRef-shaped dicts, not bare strings.
|
||||||
|
|
||||||
|
Smoke-15: be-doc i_documented(files=["README.md"]) → choreographer
|
||||||
|
doc.py stamped `existing.documents = files` (list[str]). Task.documents
|
||||||
|
is list[DocRef] persisted as dicts; readers do DocRef(**d) / d["path"]
|
||||||
|
and the indexer does d.get("path"). A bare string then 500'd
|
||||||
|
GET /docs (`TypeError: DocRef() argument after ** must be a mapping,
|
||||||
|
not str`) and would AttributeError the indexer. Fix: build proper
|
||||||
|
DocRef dicts at the source (_doc_refs_for) + defensively coerce on read
|
||||||
|
(_coerce_doc_ref) so legacy/corrupted rows don't explode.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.models.task import DocRef
|
||||||
|
from roboco.services.docs import _coerce_doc_ref
|
||||||
|
from roboco.services.gateway.choreographer.doc import _doc_refs_for
|
||||||
|
|
||||||
|
|
||||||
|
def test_doc_refs_for_builds_valid_docref_dicts() -> None:
|
||||||
|
"""The stamped elements are dicts that survive DocRef(**d) and the
|
||||||
|
indexer's d.get('path') — the two paths that broke in smoke-15."""
|
||||||
|
agent = uuid4()
|
||||||
|
inputs = ["README.md", "docs/api/endpoints.md"]
|
||||||
|
out = _doc_refs_for(inputs, agent)
|
||||||
|
|
||||||
|
assert len(out) == len(inputs)
|
||||||
|
for d, expected_path, expected_title in (
|
||||||
|
(out[0], "README.md", "README.md"),
|
||||||
|
(out[1], "docs/api/endpoints.md", "endpoints.md"),
|
||||||
|
):
|
||||||
|
assert isinstance(d, dict), d
|
||||||
|
# Indexer path (roboco/services/task.py:_index_docs_background).
|
||||||
|
assert d.get("path") == expected_path
|
||||||
|
# list_docs / _get_existing_doc_ref path.
|
||||||
|
ref = DocRef(**d)
|
||||||
|
assert ref.path == expected_path
|
||||||
|
assert ref.title == expected_title
|
||||||
|
assert ref.doc_type == "doc"
|
||||||
|
assert ref.created_by == str(agent)
|
||||||
|
|
||||||
|
|
||||||
|
def test_doc_refs_for_empty_list() -> None:
|
||||||
|
assert _doc_refs_for([], uuid4()) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_coerce_doc_ref_passthrough_docref() -> None:
|
||||||
|
ref = DocRef(path="a.md", title="a.md", doc_type="doc")
|
||||||
|
assert _coerce_doc_ref(ref) is ref
|
||||||
|
|
||||||
|
|
||||||
|
def test_coerce_doc_ref_from_dict() -> None:
|
||||||
|
ref = DocRef(path="a.md", title="a.md", doc_type="doc")
|
||||||
|
out = _coerce_doc_ref(ref.model_dump())
|
||||||
|
assert isinstance(out, DocRef)
|
||||||
|
assert out.path == "a.md"
|
||||||
|
|
||||||
|
|
||||||
|
def test_coerce_doc_ref_from_bare_string_is_the_169_fix() -> None:
|
||||||
|
"""The exact smoke-15 corruption: a bare path string must NOT raise
|
||||||
|
(previously `DocRef(**"README.md")` → TypeError → 500)."""
|
||||||
|
out = _coerce_doc_ref("README.md")
|
||||||
|
assert isinstance(out, DocRef)
|
||||||
|
assert out.path == "README.md"
|
||||||
|
assert out.title == "README.md"
|
||||||
|
assert out.doc_type == "doc"
|
||||||
|
|
||||||
|
|
||||||
|
def test_coerce_doc_ref_rejects_unsupported_type() -> None:
|
||||||
|
with pytest.raises(TypeError, match=r"unsupported Task\.documents element"):
|
||||||
|
_coerce_doc_ref(123)
|
||||||
|
|
||||||
|
|
||||||
|
def test_source_output_round_trips_through_read_coercion() -> None:
|
||||||
|
"""End-to-end invariant: what _doc_refs_for writes is exactly what
|
||||||
|
_coerce_doc_ref reads back without loss."""
|
||||||
|
agent = uuid4()
|
||||||
|
stamped = _doc_refs_for(["README.md"], agent)
|
||||||
|
refs = [_coerce_doc_ref(d) for d in stamped]
|
||||||
|
assert [r.path for r in refs] == ["README.md"]
|
||||||
|
assert refs[0].created_by == str(agent)
|
||||||
Reference in New Issue
Block a user