Skip to content

AISOS-2537: support explicit tool output when Vertex native formatting is disabled - #353

Open
JoshSalomon wants to merge 1 commit into
forge-sdlc:mainfrom
JoshSalomon:codex/fix-vertex-structured-output-policy
Open

JoshSalomon wants to merge 1 commit into
forge-sdlc:mainfrom
JoshSalomon:codex/fix-vertex-structured-output-policy

Conversation

@JoshSalomon

Copy link
Copy Markdown
Contributor

Problem

PR #351 correctly made native-output policy and authorization failures terminal, but left no explicit non-native option for deployments that disable structured output. This scoped follow-up provides an operator-selected compatibility path without restoring broad automatic fallback.

Tracking bug: AISOS-2537 (manual fix; not an automatic Forge work item).

Changes

  • Add VERTEX_STRUCTURED_OUTPUT_STRATEGY=native|tool; native remains the default and invalid values fail configuration loading.
  • Select ToolStrategy(handle_errors=False) before graph creation only for structured host Claude-on-Vertex calls, including named targets.
  • Preserve ordinary open-dictionary tool inputs and typed local Pydantic validation. Reject explicitly strict tools, including strictness inside provider definitions, before HTTP.
  • Omit native format/response_format/strict wire parameters in explicit tool mode. Missing, invalid, or multiple final outputs stop without repair loops.
  • Document configuration, recovery boundaries, test matrix, and the gap: tool mode does not provide provider-side constrained decoding.

Other backends, unstructured calls, container agents, recursion settings, and outer workflow retries are unchanged. Native mode still fails closed on policy, authorization, schema, SDK, refusal, and truncation errors; existing confirmed-unsupported fallback remains unchanged.

Verification

  • TDD: configuration/strategy and mocked policy-denial regressions failed before implementation. Review's nested strict-marker bypass was separately reproduced before its fix.
  • Final full suite: 3,687 passed, 42 skipped. Unchanged upstream baseline: 3,660 passed, the same 42 skipped.
  • Skips: 17 unavailable container-infrastructure cases, 2 unavailable Redis Stack cases, 3 Kind gate cases, and 20 superseded persistence-boundary scenarios.
  • All new executable source lines covered; changed-file coverage improved against the upstream baseline. Existing uncovered source paths are not claimed as fully covered.
  • Changed-file Ruff checks, formatting, and git diff --check passed. Independent review found no remaining actionable issues.
  • Scoped mypy is not a green verification: both the upstream baseline and final fix report the same 34 typing errors in 2 files. These existing type-checking limitations are not presented as passing checks.

All provider traffic in the new tests is mocked and uses synthetic identifiers. No raw runtime logs or cloud/account configuration are attached. No live probe, worker restart, cloud-policy change, Redis migration, or ticket retry was performed. Deployment and approved recovery remain operator actions after merge.

**kwargs: Any,
) -> Any:
del strict # LangChain's binding-wide strictness is not a per-tool opt-in.
if self.tool_output_only and any(_explicitly_strict(tool) for tool in tools):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Preserve strict-tool metadata through MCP loading

These checks run after _load_mcp_tools(), whose _wrap_tool_with_error_handling() reconstructs each tool as a new StructuredTool without preserving extras. Consequently, a tool declaring extras["strict"] = True (or strictness in extras["provider_tool_definition"]) reaches this binding as non-strict and is silently weakened instead of rejected in explicit tool-output mode.

The new rejection test mocks _load_mcp_tools() to return the original strict tool, so it bypasses the wrapper. I reproduced this by mocking only MultiServerMCPClient.get_tools() to return a tool with extras={"strict": True}, calling the real _load_mcp_tools(), and then binding the returned tool with tool_output_only=True: binding succeeds instead of raising the expected ValueError. All 89 targeted PR tests passed; this additional regression fails.

Please preserve provider metadata when wrapping tools, or reject strict tools before wrapping, and add coverage through the real loader.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants