fix(agent): make read_document robust to combined source/path arguments
The model treated the combined 'source/path' string (as printed in search result lines, read-result headers and refusals) as the document's identity and passed it as 'source' — e.g. source='homelab/active/container_caddy/caddy.md' instead of source='homelab', path='active/container_caddy/caddy.md'. - Rewrite the read_document description with the split rule (source = before the FIRST '/', path = after it) and a worked example; share the source/path parameter descriptions between read_document and search_documents; map search result lines back onto the split. - New _resolve_document: on a lookup miss with a '/' in source, retry at the first slash (source names are directory basenames and can never contain '/'), plus a continuation candidate for a split at a later slash; a self-corrected combined form for an already-in-context document is still rejected as ALREADY_IN_CONTEXT. - A slash-carrying source that matches nothing gets an educational refusal naming the corrected arguments instead of the generic line that repeated the combined form. Verified live against aipi (lite) + the imported homelab KB: A/B on the exact failure scenario (5 runs each, right after a combined-source search result) — old descriptions 5/5 combined, new descriptions 5/5 clean; two live UI turns (Playwright) produced only clean split arguments, including a multi-hop read of install_caddy_deskwork.yaml that landed in done.sources. Full suite: 1376 passed, app coverage 99% (agent.py 100%), ruff + pyright clean, agent/search E2E green in isolation.
This commit is contained in:
+105
-39
@@ -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."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user