Skip to content

fix: resolve toolbox $ref config before deploy#9148

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

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.

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