From daf4e84459ff0a574550150ebaf622e557796176 Mon Sep 17 00:00:00 2001 From: kunkunblueberry Date: Sat, 7 Nov 2026 10:47:39 +0800 Subject: [PATCH 1/2] [fix] agent: name the legal query types in the description and in rejections Issue #132: the assistant sent query type canvas_items, then could not tell why canvas.status also failed. The union already carries a discriminator and oneOf, but the tool description said only 'Query objects, jobs, assets or versions', and a rejected query surfaced the raw pydantic tag list, so the model had to reinterpret an error instead of reading the contract. QUERY_TYPES is now derived from the union itself, the description lists them, and a rejected query answers with the accepted types plus the closest match (canvas_items -> canvas.status) or the missing field. Refs #198, #132 --- packages/core/src/opendirector/agent/tools.py | 41 ++++++++++++++++--- tests/test_query_guidance.py | 36 ++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 tests/test_query_guidance.py diff --git a/packages/core/src/opendirector/agent/tools.py b/packages/core/src/opendirector/agent/tools.py index 91b3383d..0d9e92df 100644 --- a/packages/core/src/opendirector/agent/tools.py +++ b/packages/core/src/opendirector/agent/tools.py @@ -1,11 +1,12 @@ """Business commands shared by every agent runtime.""" import asyncio +import difflib import shutil from pathlib import Path +from typing import get_args from uuid import UUID, uuid5 -from pydantic import JsonValue, TypeAdapter - +from pydantic import JsonValue, TypeAdapter, ValidationError from opendirector.application.creation_artifacts import publish_artifact from opendirector.application.creation_assets import CreationAssets from opendirector.application.creation_canvas import CreationCanvas @@ -22,6 +23,30 @@ ROOT = SOURCE_ROOT or BUNDLED EXCLUDED = {'project.create', 'project.trash', 'project.restore', 'asset.import', 'job.checkpoint', 'canvas.applyEdit', 'canvas.publishMedia', 'timeline.publishRender'} + +QUERY_MODELS = get_args(get_args(Query)[0]) +#: The legal `type` values, read from the union itself. A new query type cannot +#: leave the tool description or the error guidance behind. +QUERY_TYPES = tuple(sorted(get_args(model.model_fields['type'].annotation)[0] for model in QUERY_MODELS)) + + +def query_guidance(failure): + """Explain an unusable query with the values the union actually accepts. + + A model that guesses a plural name (`canvas_items`) should be told the close + match instead of only that its tag was rejected. + """ + listed = ', '.join(QUERY_TYPES) + for issue in failure.errors(include_input=False, include_url=False): + if issue['type'] == 'union_tag_invalid': + given = issue.get('ctx', {}).get('tag', '') + closest = difflib.get_close_matches(str(given), QUERY_TYPES, n=1, cutoff=0.4) + hint = f' Did you mean {closest[0]}?' if closest else '' + return f'Unsupported query type {given!r}.{hint} Valid types: {listed}.' + if issue['type'] == 'missing': + field = '.'.join(str(part) for part in issue['loc']) + return f'Query is missing the required field {field}. Valid types: {listed}.' + return f'Query does not match any supported shape. Valid types: {listed}.' COMMANDS = {name: model for name, model in COMMAND_MODELS.items() if name not in EXCLUDED} for command_name, command_model in COMMANDS.items(): @@ -208,7 +233,8 @@ def tools() -> list[dict]: for field in ('queryId', 'projectId'): definition['properties'].pop(field) definition['required'].remove(field) - result.append(tool_schema('query', 'Query objects, jobs, assets or versions. canvas.status can wait; asset.preview provides media for native viewing.', query_schema)) + result.append(tool_schema('query', f'Query objects, jobs, assets or versions. Valid types: {", ".join(QUERY_TYPES)}. ' + 'canvas.status waits and needs objectId; asset.preview returns media for native viewing.', query_schema)) command_type = {'type': 'string', 'enum': list(COMMANDS)} result.append(tool_schema('command_schema', 'Read command help before the first execute_command call of that type in this turn. ' 'Find commands with read_project. For the first lookup, pass only the exact type, e.g. {"type":"canvas.show"}. ' @@ -243,8 +269,13 @@ async def execute(self, project_id: UUID, turn_id: UUID, session: str, call_id: if name == 'query': if {'projectId', 'queryId'} & arguments.keys(): raise ProjectError('查询身份与工程范围由宿主提供。') - query = TypeAdapter(Query).validate_python({**arguments, 'projectId': project_id, - 'queryId': uuid5(turn_id, call_id)}) + try: + query = TypeAdapter(Query).validate_python({**arguments, 'projectId': project_id, + 'queryId': uuid5(turn_id, call_id)}) + except ValidationError as failure: + # A rejected query must name the accepted types rather than echo a + # tag list the model has to reinterpret. + raise ProjectError(query_guidance(failure)) from None result = await CreationCanvas(self.projects).query(query) if query.type == 'asset.preview' and 'preview' in result: with self.projects.local.transaction() as db: diff --git a/tests/test_query_guidance.py b/tests/test_query_guidance.py new file mode 100644 index 00000000..69d6f0ce --- /dev/null +++ b/tests/test_query_guidance.py @@ -0,0 +1,36 @@ +"""Issue #132: a rejected query must name the types and fields it actually accepts.""" +from uuid import uuid4 + +import pytest +from pydantic import TypeAdapter, ValidationError + +from opendirector.agent.tools import QUERY_TYPES, query_guidance, tools +from opendirector.domain.creation.commands import Query + + +def guidance(arguments): + try: + TypeAdapter(Query).validate_python({'projectId': uuid4(), 'queryId': uuid4(), **arguments}) + except ValidationError as failure: + return query_guidance(failure) + raise AssertionError('expected the query to be rejected') + + +def test_the_tool_description_lists_every_legal_type(): + tool = next(item['function'] for item in tools() if item['function']['name'] == 'query') + for name in QUERY_TYPES: + assert name in tool['description'], name + # The type the assistant guessed in #132 is not one of them. + assert 'canvas_items' not in QUERY_TYPES + + +def test_an_unknown_type_is_answered_with_the_closest_legal_one(): + message = guidance({'type': 'canvas_items'}) + assert 'canvas_items' in message + assert 'canvas.status' in message # the suggestion + assert all(name in message for name in QUERY_TYPES) + + +def test_a_missing_field_is_named(): + message = guidance({'type': 'canvas.status'}) + assert 'objectId' in message From ccf18c7e798e674b9a541e3da536d537c32ab816 Mon Sep 17 00:00:00 2001 From: kunkunblueberry Date: Sat, 7 Nov 2026 21:21:23 +0800 Subject: [PATCH 2/2] [fix] agent: keep field-level query errors, and only rewrite a guessed type The first version replaced every rejected query with a type list, which dropped the field detail that test_agent_artifacts asserts: an unexpected waitSeconds and a missing objectId must still be named, because that is what lets the caller fix the argument. query_guidance() now rewrites only the union_tag_invalid case (suggestion plus the legal types, which is what #132 needed) and returns pydantic's own message for everything else. A fourth test pins the unexpected-field case so this cannot regress again. --- packages/core/src/opendirector/agent/tools.py | 17 ++++++++--------- tests/test_query_guidance.py | 5 +++++ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/core/src/opendirector/agent/tools.py b/packages/core/src/opendirector/agent/tools.py index 0d9e92df..369f13dc 100644 --- a/packages/core/src/opendirector/agent/tools.py +++ b/packages/core/src/opendirector/agent/tools.py @@ -31,22 +31,21 @@ def query_guidance(failure): - """Explain an unusable query with the values the union actually accepts. + """Explain an unusable query without losing the detail the caller needs. - A model that guesses a plural name (`canvas_items`) should be told the close - match instead of only that its tag was rejected. + A guessed type (`canvas_items`) is answered with the accepted values and the + closest match. Every other problem keeps pydantic's field-level message: the + field name and constraint are what let the caller fix the argument, so a + generic summary here would be a regression (see test_agent_artifacts, which + asserts that an unexpected `waitSeconds` is named). """ - listed = ', '.join(QUERY_TYPES) for issue in failure.errors(include_input=False, include_url=False): if issue['type'] == 'union_tag_invalid': given = issue.get('ctx', {}).get('tag', '') closest = difflib.get_close_matches(str(given), QUERY_TYPES, n=1, cutoff=0.4) hint = f' Did you mean {closest[0]}?' if closest else '' - return f'Unsupported query type {given!r}.{hint} Valid types: {listed}.' - if issue['type'] == 'missing': - field = '.'.join(str(part) for part in issue['loc']) - return f'Query is missing the required field {field}. Valid types: {listed}.' - return f'Query does not match any supported shape. Valid types: {listed}.' + return f'Unsupported query type {given!r}.{hint} Valid types: {", ".join(QUERY_TYPES)}.' + return str(failure) COMMANDS = {name: model for name, model in COMMAND_MODELS.items() if name not in EXCLUDED} for command_name, command_model in COMMANDS.items(): diff --git a/tests/test_query_guidance.py b/tests/test_query_guidance.py index 69d6f0ce..08623e3f 100644 --- a/tests/test_query_guidance.py +++ b/tests/test_query_guidance.py @@ -34,3 +34,8 @@ def test_an_unknown_type_is_answered_with_the_closest_legal_one(): def test_a_missing_field_is_named(): message = guidance({'type': 'canvas.status'}) assert 'objectId' in message + +def test_an_unexpected_field_keeps_its_name(): + """Field-level detail must survive: test_agent_artifacts relies on it.""" + message = guidance({'type': 'object.get', 'objectId': str(uuid4()), 'waitSeconds': 1}) + assert 'waitSeconds' in message