Skip to content

fix(agent): render tool parameters deterministically - #721

Merged
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Anon2Tokyo:fix/deterministic-tool-params
Aug 6, 2026
Merged

fix(agent): render tool parameters deterministically#721
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Anon2Tokyo:fix/deterministic-tool-params

Conversation

@Anon2Tokyo

@Anon2Tokyo Anon2Tokyo commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

Render tool parameters in a deterministic order when building tool descriptions for the plan-stage system prompt.

formatToolDefs previously ranged over the JSON schema properties map directly, so the rendered parameter order could vary across runs. Sorting parameter names before rendering keeps the generated prompt byte-stable for the same tool definitions, improving prefix/prompt cache reproducibility without changing prompt semantics.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)
  • go test ./internal/agent
  • go test -race ./internal/agent
  • go vet ./...
  • make build
  • gofmt -l internal/agent/agent.go internal/agent/agent_test.go produced no output

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Focused agent tests pass locally, including race detection. No documentation update is needed because this does not change user-facing behavior. I will follow the CLA bot instructions if required.

Related Issues

Closes #719

@CLAassistant

CLAassistant commented Aug 4, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Thanks for this thorough contribution — the analysis in the PR description is well-written and the test coverage is solid.

I'd like to suggest two refinements before merging:

1. Preserve original parameter order from tools.json instead of alphabetical sorting

Tool authors intentionally order parameters by importance (e.g. query first, optional flags later). While sort.Strings does achieve determinism, it discards that semantic ordering. The root issue is that FunctionDef.Parameters is typed as map[string]any, which loses insertion order during json.Unmarshal.

A possible approach: keep the raw JSON bytes available at render time and walk the properties object with json.Decoder to extract keys in their original document order. Alternatively, we could introduce an explicit parameter_order field in tools.json. Either way, the goal is: deterministic output that respects the author's intended order.

2. Consider using slices.Sorted (or slices.Sort) instead of sort.Strings

Our go.mod specifies Go 1.25, so the newer slices package is available. If you do keep alphabetical sorting as a fallback (e.g. for dynamically-registered MCP tools where original order isn't available), prefer slices.Sorted(maps.Keys(params)) — it's more idiomatic for modern Go.

Let me know if you'd like to discuss the design direction for preserving JSON key order — happy to help scope that out.

@lizhengfeng101 lizhengfeng101 left a comment

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.

@Anon2Tokyo I'd like to suggest two refinements before merging

@Anon2Tokyo

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Thanks for the suggestions. I updated the implementation to preserve the original properties order from tools.json by carrying the raw tool definition through BuildToolDefs and using json.Decoder at prompt-render time to walk the properties object in document order.
For tool definitions that do not have raw JSON available, such as dynamically generated MCP tools, the renderer falls back to deterministic alphabetical ordering using slices.Sorted(maps.Keys(params)).

@lizhengfeng101 lizhengfeng101 left a comment

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.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 842253e into alibaba:main Aug 6, 2026
11 checks passed
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.

Plan-phase system prompt renders tool parameters in nondeterministic order, so byte-identical requests defeat provider prefix caching

3 participants