diff --git a/app/rag/agent.py b/app/rag/agent.py index e702a63..0642ce7 100644 --- a/app/rag/agent.py +++ b/app/rag/agent.py @@ -45,7 +45,13 @@ task 04): half-specified ``source``/``path`` target; a document already in context (seed or previously read) → ``"Already in your context."``; an unknown ``source/path`` (read or scoped search) → - ``"No document at …"``. A search that ran but found nothing is NOT a + ``"No document at …"``. A ``source`` argument containing a ``'/'`` + (the model passed the combined ``source/path`` form) is first + self-corrected by splitting at the first slash (see + :func:`_resolve_document` — source names are directory basenames and + can never contain ``'/'``); if the split still matches nothing, the + refusal teaches the split instead of repeating the combined form. + A search that ran but found nothing is NOT a rejection — its ``"No matches for …"`` line is a (counted) result. A rejected call still consumes a *round* in the loop, so a pathological stream that keeps emitting rejected calls is bounded by @@ -104,6 +110,33 @@ from app.rag.llm import ( logger = logging.getLogger("app.agent") +#: Parameter descriptions shared by ``read_document`` and +#: ``search_documents``. The model repeatedly conflated the two fields — +#: passing the combined ``source/path`` string (as printed in search +#: result lines, read-result headers and refusals) as ``source`` — so +#: the descriptions define the split explicitly: ``source`` is the part +#: BEFORE the first ``'/'``, ``path`` the part after it, with a worked +#: example in the ``read_document`` description itself. +_SOURCE_PARAM: dict[str, Any] = { + "type": "string", + "description": ( + "Top-level source name only (e.g. 'homelab') — the part BEFORE " + "the first '/' of a combined 'source/path' string, exactly as " + "shown after 'source: ' in the list_documents output. Must not " + "contain '/' itself — do not pass the full source/path here." + ), +} +_PATH_PARAM: dict[str, Any] = { + "type": "string", + "description": ( + "File path relative to the source directory (e.g. " + "'active/container_caddy/caddy.md') — the part AFTER the first " + "'/' of a combined 'source/path' string, exactly as shown after " + "'path: ' in the list_documents output. Must not start with the " + "source name." + ), +} + #: The three agent tools (phase 37; ``search_documents`` added in phase #: 68): OpenAI function definitions passed as ``tools=AGENT_TOOLS`` to #: ``chat_stream`` for the whole grounded turn — phase 45 removed the @@ -127,26 +160,21 @@ AGENT_TOOLS: list[dict[str, Any]] = [ "name": "read_document", "description": ( "Add the full content of one more indexed document " - "to your context" + "to your context. A document is identified by the " + "(source, path) pair exactly as shown in the " + "list_documents output: 'source' is the top-level " + "source name only (e.g. 'homelab'), 'path' is the file " + "path inside that source (e.g. " + "'active/container_caddy/caddy.md'). If you only have a " + "combined 'source/path' string (as in search_documents " + "results), split it at the FIRST '/': the part before " + "is the source, the part after is the path. Example: " + "read_document(source='homelab', " + "path='active/container_caddy/caddy.md')." ), "parameters": { "type": "object", - "properties": { - "source": { - "type": "string", - "description": ( - "The document's source, as shown after 'source: ' in the " - "list_documents output." - ), - }, - "path": { - "type": "string", - "description": ( - "The document's path, as shown after 'path: ' in the " - "list_documents output." - ), - }, - }, + "properties": {"source": _SOURCE_PARAM, "path": _PATH_PARAM}, "required": ["source", "path"], }, }, @@ -159,9 +187,11 @@ AGENT_TOOLS: list[dict[str, Any]] = [ "Search every indexed document for an exact string " "(case-insensitive) and return up to 20 matching lines as " "'source/path:line: text' — use this to locate content, " - "then read_document the winner. Optionally pass 'source' " - "and 'path' (as shown in list_documents) to search one " - "document only." + "then read_document the winner (each result line's " + "'source/path' splits at the first '/': the part before " + "is the source, the part after is the path). Optionally " + "pass 'source' and 'path' (as shown in list_documents) " + "to search one document only." ), "parameters": { "type": "object", @@ -173,20 +203,8 @@ AGENT_TOOLS: list[dict[str, Any]] = [ "substring, not a regex)" ), }, - "source": { - "type": "string", - "description": ( - "The document's source, as shown after 'source: ' in the " - "list_documents output." - ), - }, - "path": { - "type": "string", - "description": ( - "The document's path, as shown after 'path: ' in the " - "list_documents output." - ), - }, + "source": _SOURCE_PARAM, + "path": _PATH_PARAM, }, "required": ["pattern"], }, @@ -239,6 +257,36 @@ def find_document(db: Session, source: str, path: str) -> Document | None: ) +def _resolve_document( + db: Session, source: str, path: str +) -> tuple[Document | None, str, str]: + """``(source, path)`` → document, with combined-form self-correction. + + The exact pair is tried first. If it misses and *source* contains a + ``'/'``, the model passed the combined ``source/path`` form — search + result lines, read-result headers and the generic refusal all print + that form, so the model treats it as the document's identity. Source + names are directory basenames (``app.rag.importer``: ``source = + root.name``) and can never contain a ``'/'``, so the pair is retried + at the FIRST slash: the part before is the source name, the part + after is the path. A second candidate covers a split at a LATER + slash (``source`` carried source + leading directories, ``path`` the + remainder). + + Returns ``(doc, src, p)`` where ``(src, p)`` is the first-slash + split when one was attempted (so a refusal can teach it), else the + original pair. + """ + doc = find_document(db, source, path) + if doc is not None or "/" not in source: + return doc, source, path + split_source, _, split_path = source.partition("/") + doc = find_document(db, split_source, split_path) + if doc is None and path and path != split_path: + doc = find_document(db, split_source, f"{split_path}/{path}") + return doc, split_source, split_path + + def all_documents(db: Session) -> list[Document]: """Every indexed document (full rows), ordered by ``(source, path)`` — catalog order. @@ -300,7 +348,8 @@ def _execute_tool( is a locator, locked A5); rejected calls return their refusal line and count in nothing. A search that ran but found nothing is still a successful (counted) call — its no-match line is a result, not a - refusal. + refusal. A combined-form ``source`` (containing a ``'/'``) is + self-corrected through :func:`_resolve_document` before any refusal. """ if call.name == "list_documents": rows = list_catalog(db) @@ -320,14 +369,26 @@ def _execute_tool( known = {(doc.source, doc.path) for doc in (*seed_docs, *holder.read_docs)} if (source, path) in known: return ALREADY_IN_CONTEXT - doc = find_document(db, source, path) + doc, split_source, split_path = _resolve_document(db, source, path) if doc is None: + if "/" in source: + # Educational refusal: the combined form is the model's + # mistake — teach the split instead of repeating it. + return ( + f"source must not contain '/': for '{source}' call " + f"read_document(source='{split_source}', " + f"path='{split_path}')." + ) return ( f"No document at {source}/{path} — check the list_documents output." ) + if (doc.source, doc.path) in known: + # A self-corrected combined form for a document already in + # context (the raw pair above cannot have matched it). + return ALREADY_IN_CONTEXT holder.read_docs.append(doc) holder.tool_calls += 1 - return f"Document {source}/{path}:\n{doc.content}" + return f"Document {doc.source}/{doc.path}:\n{doc.content}" if call.name == "search_documents": raw_pattern = call.arguments.get("pattern") pattern = raw_pattern.strip() if isinstance(raw_pattern, str) else "" @@ -343,8 +404,13 @@ def _execute_tool( # whole-KB search (house style). return MISSING_SEARCH_ARGS if source: - target = find_document(db, source, path) + target, split_source, split_path = _resolve_document(db, source, path) if target is None: + if "/" in source: + return ( + f"source must not contain '/': for '{source}' use " + f"source='{split_source}', path='{split_path}'." + ) return ( f"No document at {source}/{path} — check the list_documents output." ) diff --git a/tests/integration/test_agent_tools.py b/tests/integration/test_agent_tools.py index c1ee48e..ec8d1b2 100644 --- a/tests/integration/test_agent_tools.py +++ b/tests/integration/test_agent_tools.py @@ -7,7 +7,11 @@ document row (content included, for the never-truncated read) and return ``search_documents`` tool is pinned here too — its locked parameter shape in ``AGENT_TOOLS``, and a scripted ``ToolCallPiece`` executed through ``run_agent`` against the real DB (``all_documents`` for a -whole-KB search, ``find_document`` for a scoped one). +whole-KB search, ``find_document`` for a scoped one). The +combined-form self-correction (a ``source`` argument carrying +``source/path``) is pinned here as well, through ``run_agent``: +the split read executes against the real table, and a still-unknown +split gets the educational refusal. Requires: podman compose up -d db """ @@ -164,6 +168,18 @@ def _run_search( return holder, llm +def _run_read( + db: Session, arguments: dict[str, Any] +) -> tuple[AgentHolder, ScriptedToolLLM]: + """Drive one scripted ``read_document`` call through ``run_agent``.""" + holder = AgentHolder() + llm = ScriptedToolLLM( + ToolCallPiece(id="call_1", name="read_document", arguments=arguments) + ) + asyncio.run(_consume(llm, db, holder)) + return holder, llm + + async def _consume( llm: ScriptedToolLLM, db: Session, holder: AgentHolder ) -> list[StreamPiece | ToolCallPiece | RetryPiece]: @@ -229,6 +245,67 @@ def test_search_scoped_missing_doc_refused_through_run_agent(kb, db) -> None: assert holder.tool_calls == 0 and holder.read_docs == [] +# ---------- combined 'source/path' self-correction (read_document) ---------- + + +def test_read_combined_source_self_corrects_through_run_agent(kb, db) -> None: + """The model's combined 'source' ('Alpha/deep/nested/doc.md') resolves + through the first-slash split against the REAL table: the read + executes, the holder records the row, the result header carries the + true source/path.""" + created = _doc(db, "Alpha", "deep/nested/doc.md", "The Doc", "FULL-TEXT") + db.commit() + + holder, llm = _run_read( + db, + { + "source": "Alpha/deep/nested/doc.md", + "path": "deep/nested/doc.md", + }, + ) + + assert llm.requests[1][0][3]["content"] == ( + "Document Alpha/deep/nested/doc.md:\nFULL-TEXT" + ) + assert holder.tool_calls == 1 + assert holder.read_docs == [created] + + +def test_read_combined_source_later_slash_split_through_run_agent(kb, db) -> None: + """source='Alpha/deep' + path='nested/doc.md' (a split at a LATER + slash) resolves via the continuation candidate against the real + table.""" + created = _doc(db, "Alpha", "deep/nested/doc.md", "The Doc", "FULL-TEXT") + db.commit() + + holder, llm = _run_read( + db, {"source": "Alpha/deep", "path": "nested/doc.md"} + ) + + assert llm.requests[1][0][3]["content"] == ( + "Document Alpha/deep/nested/doc.md:\nFULL-TEXT" + ) + assert holder.tool_calls == 1 + assert holder.read_docs == [created] + + +def test_read_combined_source_refusal_teaches_split(kb, db) -> None: + """A combined source that matches nothing (even split) gets the + educational refusal naming the corrected arguments.""" + _doc(db, "Alpha", "x.md", "X", "X-CONTENT") + db.commit() + + holder, llm = _run_read( + db, {"source": "Alpha/nope/deep.md", "path": "nope/deep.md"} + ) + + assert llm.requests[1][0][3]["content"] == ( + "source must not contain '/': for 'Alpha/nope/deep.md' call " + "read_document(source='Alpha', path='nope/deep.md')." + ) + assert holder.tool_calls == 0 and holder.read_docs == [] + + def test_search_no_matches_through_run_agent(kb, db) -> None: _doc(db, "Alpha", "a/one.md", "One", "nothing matching") db.commit() diff --git a/tests/unit/test_agent.py b/tests/unit/test_agent.py index b3b4464..4127903 100644 --- a/tests/unit/test_agent.py +++ b/tests/unit/test_agent.py @@ -112,32 +112,49 @@ def test_agent_tools_names_and_parameters() -> None: read_params = by_name["read_document"]["function"]["parameters"] assert read_params["required"] == ["source", "path"] assert set(read_params["properties"]) == {"source", "path"} - # Phase 45: the per-tool budgets are gone — "exactly one more" - # dropped out of the read_document description. + # The model repeatedly conflated the two fields — passing the + # combined 'source/path' string as 'source' — so the read_document + # description pins the split rule with a worked example. assert by_name["read_document"]["function"]["description"] == ( - "Add the full content of one more indexed document to your context" + "Add the full content of one more indexed document to your " + "context. A document is identified by the (source, path) pair " + "exactly as shown in the list_documents output: 'source' is " + "the top-level source name only (e.g. 'homelab'), 'path' is " + "the file path inside that source (e.g. " + "'active/container_caddy/caddy.md'). If you only have a " + "combined 'source/path' string (as in search_documents " + "results), split it at the FIRST '/': the part before is the " + "source, the part after is the path. Example: " + "read_document(source='homelab', " + "path='active/container_caddy/caddy.md')." ) - # Phase 63 (A2): the parameter descriptions point the LLM at the - # labeled `source:` / `path:` fields of the list_documents output - # (the example was dropped by the phase-68 description fix — the - # wording stays pinned, the model saw invented paths in calls). + # The parameter descriptions define the split: source = before the + # first '/', path = after it. assert read_params["properties"]["source"]["description"] == ( - "The document's source, as shown after 'source: ' in the " - "list_documents output." + "Top-level source name only (e.g. 'homelab') — the part BEFORE " + "the first '/' of a combined 'source/path' string, exactly as " + "shown after 'source: ' in the list_documents output. Must not " + "contain '/' itself — do not pass the full source/path here." ) assert read_params["properties"]["path"]["description"] == ( - "The document's path, as shown after 'path: ' in the " - "list_documents output." + "File path relative to the source directory (e.g. " + "'active/container_caddy/caddy.md') — the part AFTER the first " + "'/' of a combined 'source/path' string, exactly as shown " + "after 'path: ' in the list_documents output. Must not start " + "with the source name." ) - # Phase 68: search_documents — the third tool, a locator (locked A5). + # Phase 68: search_documents — the third tool, a locator (locked + # A5); its description maps result lines back onto the split. search = by_name["search_documents"]["function"] assert search["description"] == ( "Search every indexed document for an exact string " "(case-insensitive) and return up to 20 matching lines as " "'source/path:line: text' — use this to locate content, " - "then read_document the winner. Optionally pass 'source' " - "and 'path' (as shown in list_documents) to search one " - "document only." + "then read_document the winner (each result line's " + "'source/path' splits at the first '/': the part before " + "is the source, the part after is the path). Optionally " + "pass 'source' and 'path' (as shown in list_documents) " + "to search one document only." ) search_params = search["parameters"] assert search_params["type"] == "object" @@ -146,15 +163,10 @@ def test_agent_tools_names_and_parameters() -> None: assert search_params["properties"]["pattern"]["description"] == ( "The exact text to search for (a plain substring, not a regex)" ) - # Phase 63 labeled-field wording, same as read_document's parameters. - assert search_params["properties"]["source"]["description"] == ( - "The document's source, as shown after 'source: ' in the " - "list_documents output." - ) - assert search_params["properties"]["path"]["description"] == ( - "The document's path, as shown after 'path: ' in the " - "list_documents output." - ) + # Shared constants: search's source/path params ARE read_document's + # (one definition, no drift between the two tools). + assert search_params["properties"]["source"] is read_params["properties"]["source"] + assert search_params["properties"]["path"] is read_params["properties"]["path"] # ---------- happy path: list → read → answer ---------- @@ -574,6 +586,189 @@ def test_read_document_missing_arguments_refused( assert llm.requests[1][1] == AGENT_TOOLS +# ---------- combined 'source/path' self-correction ---------- +# The model treats the combined 'source/path' string (search result +# lines, read-result headers, refusals) as the document's identity and +# sometimes passes it as 'source'. _resolve_document splits it at the +# first slash (source names are directory basenames — they can never +# contain '/'); a refusal for a still-unknown split teaches the split. + + +def test_read_combined_source_is_split_and_read( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """source='S/a/b.md' (the combined form) + path='a/b.md': the exact + lookup misses, the first-slash split hits — the read executes, the + holder records the document, and the result header carries the TRUE + source/path (not the model's raw arguments).""" + doc = _doc("S", "a/b.md", "B", "B-CONTENT") + calls: list[tuple[str, str]] = [] + + def _find(db: Any, source: str, path: str) -> Document | None: + calls.append((source, path)) + return doc if (source, path) == ("S", "a/b.md") else None + + monkeypatch.setattr(agent, "find_document", _find) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="read_document", + arguments={"source": "S/a/b.md", "path": "a/b.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings())) + # Exact pair first, then the first-slash split (no third attempt). + assert calls == [("S/a/b.md", "a/b.md"), ("S", "a/b.md")] + assert holder.read_docs == [doc] + assert holder.tool_calls == 1 + assert llm.requests[1][0][3]["content"] == "Document S/a/b.md:\nB-CONTENT" + + +def test_read_combined_source_split_at_later_slash( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """source='S/a' + path='b.md' — a split at a LATER slash (source + carried source + leading directory, path the remainder): the exact + lookup and the first-slash split miss, the continuation candidate + (source, split/path) hits.""" + doc = _doc("S", "a/b.md", "B", "B-CONTENT") + calls: list[tuple[str, str]] = [] + + def _find(db: Any, source: str, path: str) -> Document | None: + calls.append((source, path)) + return doc if (source, path) == ("S", "a/b.md") else None + + monkeypatch.setattr(agent, "find_document", _find) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="read_document", + arguments={"source": "S/a", "path": "b.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings())) + assert calls == [("S/a", "b.md"), ("S", "a"), ("S", "a/b.md")] + assert holder.read_docs == [doc] + assert holder.tool_calls == 1 + assert llm.requests[1][0][3]["content"] == "Document S/a/b.md:\nB-CONTENT" + + +def test_read_combined_source_unknown_teaches_the_split( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A combined source that matches nothing — even split — gets the + EDUCATIONAL refusal: it names the corrected arguments instead of + repeating the combined form (the old generic line reinforced the + mistake).""" + monkeypatch.setattr(agent, "find_document", lambda db, source, path: None) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="read_document", + arguments={"source": "S/a/b.md", "path": "a/b.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings())) + assert holder.read_docs == [] and holder.tool_calls == 0 + assert llm.requests[1][0][3]["content"] == ( + "source must not contain '/': for 'S/a/b.md' call " + "read_document(source='S', path='a/b.md')." + ) + assert llm.requests[1][1] == AGENT_TOOLS # rejected → tools stay offered + + +def test_read_combined_source_for_seed_doc_is_already_in_context( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The combined form of a document ALREADY in context: the raw pair + cannot match the dedupe set, so the split resolves it — and it is + still rejected as already-in-context (no duplicate read_docs entry, + no re-read into the context).""" + seed = [_doc("S", "a.md", "A", "A-CONTENT")] + + def _find(db: Any, source: str, path: str) -> Document | None: + return seed[0] if (source, path) == ("S", "a.md") else None + + monkeypatch.setattr(agent, "find_document", _find) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="read_document", + arguments={"source": "S/a.md", "path": "a.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings(), seed_docs=seed)) + assert holder.read_docs == [] and holder.tool_calls == 0 + assert llm.requests[1][0][3]["content"] == agent.ALREADY_IN_CONTEXT + + +def test_search_scoped_combined_source_is_split( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A scoped search whose 'source' carries the combined form resolves + through the split — the search runs on the right document.""" + doc = _doc("S", "a.md", "A", "needle here") + + def _find(db: Any, source: str, path: str) -> Document | None: + return doc if (source, path) == ("S", "a.md") else None + + monkeypatch.setattr(agent, "find_document", _find) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="search_documents", + arguments={"pattern": "needle", "source": "S/a.md", "path": "a.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings())) + assert llm.requests[1][0][3]["content"] == "S/a.md:1: needle here" + assert holder.tool_calls == 1 + assert holder.read_docs == [] # searched doc did not enter the context + + +def test_search_scoped_combined_source_unknown_teaches_the_split( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(agent, "find_document", lambda db, source, path: None) + holder = AgentHolder() + llm = ScriptedLLM( + [ + ToolCallPiece( + id="call_1", + name="search_documents", + arguments={"pattern": "x", "source": "S/ghost.md", "path": "ghost.md"}, + ) + ], + [StreamPiece("content", "ans")], + ) + asyncio.run(_run(llm, holder, _settings())) + assert llm.requests[1][0][3]["content"] == ( + "source must not contain '/': for 'S/ghost.md' use " + "source='S', path='ghost.md'." + ) + assert holder.tool_calls == 0 and holder.read_docs == [] # a refusal + + # ---------- search_documents (phase 68, locked A5/A6) ----------