Skip to content

feat(py): add agents conformance test harness and align wire initialization errors - #6016

Closed
huangjeff5 wants to merge 4 commits into
mainfrom
py-conformance-harness
Closed

feat(py): add agents conformance test harness and align wire initialization errors#6016
huangjeff5 wants to merge 4 commits into
mainfrom
py-conformance-harness

Conversation

@huangjeff5

Copy link
Copy Markdown
Contributor

Summary

This pull request adds the Python test harness for the Agent Conformance Specification (tests/specs/agent.yaml) to ensure Python SDK behavior stays consistent with the cross-language wire standards (JS/Go).

Public API & Behavior Changes

  • Agent Conformance Test Suite: Added agent_conformance_test.py executing the shared YAML spec suite against Python agent implementations.
  • Wire Error Alignment: Aligned wire-facing agent initialization error formats with the conformance spec to return structured standard error responses when agent session setup fails.

Verification

  • Ran pytest packages/genkit/tests/genkit/ai/agent_conformance_test.py (33 passed, 6 expected xfailed prior to behavior fixes in stacked PR B).

Adds the Python harness for the shared agents conformance spec, mirroring
js/ai/tests/agents_spec_test.ts and go/ai/exp/agents_conformance_test.go:
YAML spec loader, {{capture}} template resolution, contains/subsequence
matchers, the twelve harness agents over a programmable model, and
executors for the send / getSnapshotData / abort / waitUntilCompleted
steps.

8 spec tests expose known divergences in the Python implementation; they
are marked xfail(strict=True) in KNOWN_DIVERGENCES and will be flipped
green by the follow-up fixes. 31 passed, 8 xfailed.

Note: the spec's 'detach with background failure' tests pass without any
runtime change once the harness's customAgentFailing propagates turn
errors out of the agent fn (re-raising last_turn_error after run()),
matching how the JS/Go harness agents behave. This is the Python
equivalent of Go's 'return nil, err' after sess.Run.
The runtime's AgentInit validation errors used snake_case field names
('session_id') and listed seeded state sub-fields; JS/Go and
tests/specs/agent.yaml pin camelCase wire names and the 'state' wording.
Wire errors describe the wire.

The app-facing chat() path in _client.py keeps its richer field-detailed
message (it blames 'messages' when a chat caller passed messages=);
seeded_init_fields remains in use there.

Flips 2 conformance xfails green: 'server-managed agent rejects init
state', 'client-managed agent rejects sessionId'.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates wire-facing error messages in the agent runtime to use camelCase field names and introduces a comprehensive agent conformance test runner that executes test cases from a shared YAML specification. The review feedback suggests several robust improvements: removing the now-unused seeded_init_fields helper function to eliminate dead code, safely handling empty YAML files to prevent potential crashes, wrapping the bidi stream interactions in a try-except-finally block to ensure proper connection cleanup and error capturing, and catching general exceptions in the test runner loop to provide better debugging context for all step failures.

Comment on lines 330 to 331
if init.state is not None and store is not None:
fields = seeded_init_fields(init.state)
raise AgentInitError(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The helper function seeded_init_fields (defined at line 290) is no longer used anywhere in this file after removing its call here. It should be removed to keep the codebase clean and free of dead code.

Comment on lines +66 to +68
with SPEC_PATH.open() as f:
suite = yaml.safe_load(f)
tests = suite.get('tests')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the YAML file is empty, yaml.safe_load(f) will return None, which will cause an AttributeError when calling .get('tests'). Defaulting to an empty dictionary prevents this potential crash.

Suggested change
with SPEC_PATH.open() as f:
suite = yaml.safe_load(f)
tests = suite.get('tests')
with SPEC_PATH.open() as f:
suite = yaml.safe_load(f) or {}
tests = suite.get('tests')

Comment on lines +508 to +534
conn = await agent.stream_bidi(AgentInit.model_validate(resolved.get('init') or {}))
for inp in resolved.get('inputs') or []:
await conn.send(AgentInput.model_validate(inp))
await conn.close()

# expectError: the turn throws (API misuse) rather than resolving with a
# graceful finishReason='failed' output.
if resolved.get('expectError'):
expect_err = resolved['expectError']
thrown: BaseException | None = None
try:
async for _chunk in conn.receive():
pass
await conn.output()
except (GenkitError, Exception) as e: # noqa: BLE001 - spec asserts on the raised error
thrown = e
assert thrown is not None, 'Expected the turn to throw an error, but it resolved successfully.'
if 'status' in expect_err:
status = getattr(thrown, 'status', None)
assert status == expect_err['status'], (
f'Expected thrown error.status {expect_err["status"]!r}, got {status!r} (message: {thrown})'
)
if 'message' in expect_err:
assert expect_err['message'] in str(thrown), (
f'Expected thrown error.message to contain {expect_err["message"]!r}, got: {thrown}'
)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If an exception is thrown during stream_bidi or send (for example, due to an initialization error), it will not be caught by the expectError block because the try...except block is only entered during the receive phase. Additionally, if an exception occurs, the connection may not be closed properly. Wrapping the entire interaction in a try...except...finally block ensures that expected errors are caught correctly and connections are always closed.

    expect_err = resolved.get('expectError')
    thrown: BaseException | None = None
    conn = None
    try:
        conn = await agent.stream_bidi(AgentInit.model_validate(resolved.get('init') or {}))
        for inp in resolved.get('inputs') or []:
            await conn.send(AgentInput.model_validate(inp))
        await conn.close()
        if expect_err:
            async for _chunk in conn.receive():
                pass
            await conn.output()
    except (GenkitError, Exception) as e:
        if expect_err:
            thrown = e
        else:
            raise
    finally:
        if conn is not None:
            try:
                await conn.close()
            except Exception:
                pass

    if expect_err:
        assert thrown is not None, 'Expected the turn to throw an error, but it resolved successfully.'
        if 'status' in expect_err:
            status = getattr(thrown, 'status', None)
            assert status == expect_err['status'], (
                f'Expected thrown error.status {expect_err["status"]!r}, got {status!r} (message: {thrown})'
            )
        if 'message' in expect_err:
            assert expect_err['message'] in str(thrown), (
                f'Expected thrown error.message to contain {expect_err["message"]!r}, got: {thrown}'
            )
        return

Comment on lines +650 to +651
except AssertionError as e:
raise AssertionError(f'{label} in test {spec_test["name"]!r} failed: {e}') from e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching only AssertionError means that any other unexpected runtime exceptions (such as KeyError, AttributeError, or GenkitError) raised during step execution will propagate directly without being wrapped with the step label. Catching Exception instead ensures that all failures are clearly attributed to the specific step that failed, which greatly simplifies debugging.

Suggested change
except AssertionError as e:
raise AssertionError(f'{label} in test {spec_test["name"]!r} failed: {e}') from e
except Exception as e:
raise AssertionError(f'{label} in test {spec_test["name"]!r} failed: {e}') from e

Shape gates, JSON compact captures, turnEnd.finishReason via
model_fields_set, lookup miss as named AssertionError, shared
_thrown_message, keyword-only helpers, snapshot errorContains as
assert_contains. Applies on live py-conformance-harness (#6016).

Co-authored-by: jeffdh5 <jeffdh5@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants