Skip to content

fix: resolve toolbox $ref config before deploy - #9148

Draft
huimiu wants to merge 2 commits into
mainfrom
hui/toolbox-ref-config
Draft

fix: resolve toolbox $ref config before deploy#9148
huimiu wants to merge 2 commits into
mainfrom
hui/toolbox-ref-config

Conversation

@huimiu

@huimiu huimiu commented Jul 15, 2026

Copy link
Copy Markdown
Member

Why this PR is needed

Toolbox service configuration can be written inline in azure.yaml or loaded from a local file with $ref. The deploy path previously parsed the $ref object as the toolbox configuration instead of loading the referenced file. As a result, deployments using file-based toolbox configuration could omit the description and tools defined in that file, or fail to deploy with the expected configuration.

This fix resolves local $ref files relative to the project root before parsing the toolbox configuration. Inline configuration and the legacy config: shape continue to use the existing behavior. This keeps file-based and inline configuration consistent at deploy time.

Changes

  • Resolve toolbox $ref file includes with foundry.ResolveFileRefs when a reference is present anywhere in the service configuration.
  • Preserve the existing inline and legacy config: parsing behavior when no $ref is present.
  • Add a regression test covering a toolbox configuration loaded from a local file.

Testing

  • go build ./... in cli/azd/extensions/azure.ai.toolboxes
  • go test ./internal/cmd/... in cli/azd/extensions/azure.ai.toolboxes

Copilot AI review requested due to automatic review settings July 15, 2026 03:13
@github-actions

Copy link
Copy Markdown

🔗 Linked Issue Required

Thanks for the contribution! Please link a GitHub issue to this PR by adding Fixes #123 to the description or using the sidebar.
No issue yet? Feel free to create one!

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
21 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI 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.

Pull request overview

Resolves local $ref configurations for Azure AI toolbox services before deployment.

Changes:

  • Adds project-root-aware $ref resolution.
  • Preserves inline and legacy config parsing.
  • Adds referenced-file test coverage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
README.md Removes trailing whitespace.
service_target.go Resolves toolbox configuration references.
service_target_test.go Tests referenced toolbox configuration parsing.

b, err := json.Marshal(props.AsMap())
values := props.AsMap()
if projectRoot != "" {
resolved, err := foundry.ResolveFileRefs(values, projectRoot)
Comment on lines +360 to +364
return nil, fmt.Errorf(
"resolving toolbox service %q config: %w",
svc.GetName(),
err,
)
Copilot AI review requested due to automatic review settings July 15, 2026 03:28

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two things in the new $ref resolution are worth sorting before this merges.

  1. parseToolboxServiceConfigAtRoot wraps the ResolveFileRefs error with fmt.Errorf("...: %w") (service_target.go:360). ResolveFileRefs already returns a structured *azdext.LocalError (foundry/errors.go:15-21), and this extension's own AGENTS.md:93-98 says not to wrap an already-structured error: during gRPC serialization azd keeps the inner error's message/code/category and drops the outer text, so the resolving toolbox service %q config context is lost anyway. Return err directly, or fold the toolbox name into a fresh structured error.

  2. containsToolboxFileRef plus ResolveFileRefs resolve every $ref anywhere in the config, not just the service-entry include (service_target.go:388, foundry/includes.go:84-113). The schema allows tools with type: openapi and additionalProperties: true (schemas/azure.ai.toolbox.json:17-27), and real OpenAPI documents carry JSON-pointer refs like #/components/schemas/Foo. refTargetPath doesn't treat those as remote, so they get joined onto projectRoot and loadRefFile fails with cannot read $ref file. Before this change those inline specs were passed through verbatim, so this is a deploy regression for openapi tools. Can resolution be scoped to the include points you actually support (service entry, deployment array items) instead of walking into tool payloads?

Both reproduce against the current diff. Confirmed the extension builds and the parse tests pass locally, so this is behavior and convention, not a compile break.

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@huimiu huimiu added the area/extensions Extensions (general) label Jul 15, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added no-recent-activity identity issues with no activity and removed no-recent-activity identity issues with no activity labels Jul 23, 2026

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed the current HEAD (ba1d6be). The code hasn't changed since my last pass, so the two issues from that review are still open.

  1. parseToolboxServiceConfigAtRoot wraps the ResolveFileRefs error with fmt.Errorf("resolving toolbox service %q config: %w", ...). ResolveFileRefs already returns a structured *azdext.LocalError (pkg/foundry/errors.go), and this extension's AGENTS.md is explicit: don't add fmt.Errorf context to an already-structured error, because gRPC serialization keeps the inner message/code/category and drops the outer wrapper. The toolbox-name context is lost anyway. Return err directly, or fold the name into a fresh structured error.

  2. containsToolboxFileRef plus ResolveFileRefs resolve every $ref anywhere in the config, not just the service-entry include. containsToolboxFileRef returns true for any $ref key at any depth, and ResolveFileRefs then walks the whole tree resolving each one as a local file. Tool payloads are passed to the data plane verbatim and openapi is a listed tool type; OpenAPI documents routinely carry JSON-pointer refs like #/components/schemas/Foo. refTargetPath doesn't treat those as remote, so they get joined onto the project root and loadRefFile fails with "cannot read $ref file". Before this change those inline specs passed through untouched, so this is a deploy regression, and it fires even for configs that use no file includes at all (an inline $ref in a tool payload is enough to trigger it). Scope resolution to the include points you actually support, and add a regression test for a tool payload that carries a non-file $ref. The new test only covers the file-include happy path.

Both still reproduce against this diff.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-checked HEAD ba1d6be. The code hasn't changed since my last pass, and I traced both issues through the foundry package this time to confirm they still reproduce.

  1. containsToolboxFileRef matches any $ref key at any depth, and ResolveFileRefs then resolves every one as a local file, tool payloads included. openapi is a supported tool type (schema line 27, tool items are additionalProperties: true) and inline OpenAPI docs routinely carry pointer refs like #/components/schemas/Foo. In refTargetPath that value doesn't match remoteRefPattern and isn't absolute, so it gets joined onto the project root and loadRefFile fails with "cannot read $ref file". Those payloads used to marshal through verbatim, so this is a deploy regression, and it fires whenever a tool carries any $ref, even for configs with no file include at all. Worth scoping resolution to the include points you actually support (service entry, deployment items) and adding a regression test for a tool payload that carries a non-file $ref. The new test only covers the file-include happy path.

  2. parseToolboxServiceConfigAtRoot wraps the ResolveFileRefs error with fmt.Errorf("resolving toolbox service %q config: %w", ...), but that error is already a structured *azdext.LocalError. AGENTS.md lines 93-98 say not to wrap an already-structured error: gRPC serialization keeps the inner message/code/category and drops the outer text, so the toolbox-name context never surfaces. Return err directly, or fold the name into a fresh structured error.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed at HEAD ba1d6be (unchanged since my last pass). The two issues from earlier are still open, so I'm pinning them inline this time with a suggested fix for the first one.

Comment on lines +359 to +365
if err != nil {
return nil, fmt.Errorf(
"resolving toolbox service %q config: %w",
svc.GetName(),
err,
)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ResolveFileRefs already returns a structured *azdext.LocalError (pkg/foundry/errors.go). This extension's AGENTS.md error rule says not to wrap an already-structured error with fmt.Errorf("...: %w"): during gRPC serialization azd keeps the inner message/code/category and drops the outer text, so the resolving toolbox service %q config context never reaches the user. Return the structured error unchanged, or fold the toolbox name into a fresh structured error.

Suggested change
if err != nil {
return nil, fmt.Errorf(
"resolving toolbox service %q config: %w",
svc.GetName(),
err,
)
}
if err != nil {
return nil, err
}

func containsToolboxFileRef(value any) bool {
switch typed := value.(type) {
case map[string]any:
if _, exists := typed["$ref"]; exists {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

containsToolboxFileRef matches a $ref key at any depth, and ResolveFileRefs then resolves every one as a local file, tool payloads included. openapi is a supported tool type (schemas/azure.ai.toolbox.json line 27; tool items are additionalProperties: true), and OpenAPI documents carry JSON-pointer refs such as #/components/schemas/Foo. In refTargetPath that value isn't a URL and isn't absolute, so it gets joined onto the project root and loadRefFile fails with "cannot read $ref file". Those payloads marshalled through verbatim before this change, so a tool that carries any non-file $ref would now fail to deploy, even when the config has no file include. Consider scoping resolution to the include points you actually support (the service entry and deployment items) and add a regression test for a tool payload that carries a non-file $ref; the new test only covers the file-include happy path.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-checked at HEAD ba1d6be. The code hasn't changed since my last pass, so the two issues I pinned inline are still open.

  1. parseToolboxServiceConfigAtRoot wraps the ResolveFileRefs error with fmt.Errorf("resolving toolbox service %q config: %w", ...) (service_target.go:358-365). ResolveFileRefs already returns a structured *azdext.LocalError (pkg/foundry/errors.go:15-31). This extension's AGENTS.md error rule (the "Most important rule" section) says not to wrap an already-structured error: during gRPC serialization azd keeps the inner message, code, and category and drops the outer text, so "resolving toolbox service X config" never reaches the user. Return the error unchanged, or fold the extra context into the structured message where it's created.

  2. containsToolboxFileRef matches a $ref key at any depth, and ResolveFileRefs then resolves each one as a local file (service_target.go:321, 358, 388). openapi is a supported tool type (schemas/azure.ai.toolbox.json:27, and tool items are additionalProperties: true), and OpenAPI documents carry JSON-pointer refs like {"$ref": "#/components/schemas/Foo"}. refTargetPath only rejects URL-scheme refs, not # fragments, so an inline openapi tool would push that pointer through filepath.Join(baseDir, "#/components/schemas/Foo") and fail at os.ReadFile, breaking deploy. Consider skipping fragment-only refs (values starting with #), or scoping $ref resolution to the service-entry level rather than the full tool payload.

@huimiu
huimiu marked this pull request as draft July 27, 2026 05:42

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ran the regression from my earlier comments as an actual test this time, against the dependency the extension really builds against (github.com/azure/azure-dev/cli/azd v1.27.1, no replace directive in the extension's go.mod). It reproduces, and there's a wrinkle I had wrong before.

A toolbox with an inline openapi tool and no file include anywhere:

tools:
  - type: openapi
    spec:
      paths:
        /pets:
          get:
            responses:
              "200":
                schema:
                  $ref: "#/components/schemas/Pet"

containsToolboxFileRef returns true, so Deploy resolves the project root and hands the whole map to ResolveFileRefs, which joins the JSON pointer onto the project root:

resolving toolbox service "toolbox" config: cannot read $ref file
"...\001\#\components\schemas\Pet": The system cannot find the path specified.

Before this change that payload marshalled through untouched, so it's a deploy regression, and it fires even when the config uses no file include at all.

The wrinkle: filtering # values inside containsToolboxFileRef isn't sufficient on its own. It fixes the inline-only case, but as soon as the config has a genuine service-entry include, resolution turns on and ResolveFileRefs walks the whole tree again with tool payloads included. I checked both halves separately. What passed for me was also keeping tools out of the walk and overlaying it back:

inlineTools, hasInlineTools := values["tools"]
entry := make(map[string]any, len(values))
for key, value := range values {
	if key == "tools" {
		continue
	}
	entry[key] = value
}
resolved, err := foundry.ResolveFileRefs(entry, projectRoot)
if err != nil {
	return nil, err
}
if hasInlineTools {
	resolved["tools"] = inlineTools
}
values = resolved

With that plus the # filter, your three existing tests still pass, including TestParseToolboxServiceConfig_FileRef where the referenced file is the thing supplying tools, and both regression shapes pass. go test ./... on the extension is green.

Note this can't be fixed in pkg/foundry from this PR. The extension resolves the azd module from the proxy at v1.27.1, so a change to refTargetPath wouldn't reach this code until a release. It has to be scoped on the extension side.

Worth adding a test for a tool payload carrying a non-file $ref. The current test only covers the file-include happy path, which is why this got through.

On the error wrapping I flagged earlier, I'll soften that one. The rule in AGENTS.md is real and the mechanism checks out (WrapError replaces the message with the inner LocalError.Message, so "resolving toolbox service X config" never reaches the user), but azure.ai.agents/internal/synthesis/synthesizer.go:348 wraps the same call the same way. It's consistent with what's already in the tree, so treat it as optional.

Separately, CI is red. PR Governance wants a linked issue. The ValidateCrossCompile WindowsARM64 failure looks unrelated to the source, since the extension cross-compiles clean for both windows/arm64 and linux/arm64 locally at this commit.

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

Labels

area/extensions Extensions (general) ext-toolboxes azure.ai.toolboxes extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants