fix: resolve toolbox $ref config before deploy#9148
Conversation
🔗 Linked Issue RequiredThanks for the contribution! Please link a GitHub issue to this PR by adding |
|
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. |
There was a problem hiding this comment.
Pull request overview
Resolves local $ref configurations for Azure AI toolbox services before deployment.
Changes:
- Adds project-root-aware
$refresolution. - 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) |
| return nil, fmt.Errorf( | ||
| "resolving toolbox service %q config: %w", | ||
| svc.GetName(), | ||
| err, | ||
| ) |
jongio
left a comment
There was a problem hiding this comment.
Two things in the new $ref resolution are worth sorting before this merges.
-
parseToolboxServiceConfigAtRootwraps theResolveFileRefserror withfmt.Errorf("...: %w")(service_target.go:360).ResolveFileRefsalready 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 theresolving toolbox service %q configcontext is lost anyway. Returnerrdirectly, or fold the toolbox name into a fresh structured error. -
containsToolboxFileRefplusResolveFileRefsresolve every$refanywhere in the config, not just the service-entry include (service_target.go:388, foundry/includes.go:84-113). The schema allowstoolswithtype: openapiandadditionalProperties: true(schemas/azure.ai.toolbox.json:17-27), and real OpenAPI documents carry JSON-pointer refs like#/components/schemas/Foo.refTargetPathdoesn't treat those as remote, so they get joined onto projectRoot andloadRefFilefails withcannot 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.
jongio
left a comment
There was a problem hiding this comment.
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.
-
parseToolboxServiceConfigAtRootwraps theResolveFileRefserror withfmt.Errorf("resolving toolbox service %q config: %w", ...).ResolveFileRefsalready returns a structured*azdext.LocalError(pkg/foundry/errors.go), and this extension's AGENTS.md is explicit: don't addfmt.Errorfcontext 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. Returnerrdirectly, or fold the name into a fresh structured error. -
containsToolboxFileRefplusResolveFileRefsresolve every$refanywhere in the config, not just the service-entry include.containsToolboxFileRefreturns true for any$refkey at any depth, andResolveFileRefsthen walks the whole tree resolving each one as a local file. Tool payloads are passed to the data plane verbatim andopenapiis a listed tool type; OpenAPI documents routinely carry JSON-pointer refs like#/components/schemas/Foo.refTargetPathdoesn't treat those as remote, so they get joined onto the project root andloadRefFilefails 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$refin 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
left a comment
There was a problem hiding this comment.
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.
-
containsToolboxFileRefmatches any$refkey at any depth, andResolveFileRefsthen resolves every one as a local file, tool payloads included.openapiis a supported tool type (schema line 27, tool items areadditionalProperties: true) and inline OpenAPI docs routinely carry pointer refs like#/components/schemas/Foo. InrefTargetPaththat value doesn't matchremoteRefPatternand isn't absolute, so it gets joined onto the project root andloadRefFilefails 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. -
parseToolboxServiceConfigAtRootwraps theResolveFileRefserror withfmt.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. Returnerrdirectly, or fold the name into a fresh structured error.
| if err != nil { | ||
| return nil, fmt.Errorf( | ||
| "resolving toolbox service %q config: %w", | ||
| svc.GetName(), | ||
| err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
| 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 { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Re-checked at HEAD ba1d6be. The code hasn't changed since my last pass, so the two issues I pinned inline are still open.
-
parseToolboxServiceConfigAtRootwraps theResolveFileRefserror withfmt.Errorf("resolving toolbox service %q config: %w", ...)(service_target.go:358-365).ResolveFileRefsalready 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. -
containsToolboxFileRefmatches a$refkey at any depth, andResolveFileRefsthen resolves each one as a local file (service_target.go:321, 358, 388).openapiis a supported tool type (schemas/azure.ai.toolbox.json:27, and tool items areadditionalProperties: true), and OpenAPI documents carry JSON-pointer refs like{"$ref": "#/components/schemas/Foo"}.refTargetPathonly rejects URL-scheme refs, not#fragments, so an inline openapi tool would push that pointer throughfilepath.Join(baseDir, "#/components/schemas/Foo")and fail atos.ReadFile, breaking deploy. Consider skipping fragment-only refs (values starting with#), or scoping$refresolution to the service-entry level rather than the full tool payload.
Why this PR is needed
Toolbox service configuration can be written inline in
azure.yamlor loaded from a local file with$ref. The deploy path previously parsed the$refobject 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
$reffiles relative to the project root before parsing the toolbox configuration. Inline configuration and the legacyconfig:shape continue to use the existing behavior. This keeps file-based and inline configuration consistent at deploy time.Changes
$reffile includes withfoundry.ResolveFileRefswhen a reference is present anywhere in the service configuration.config:parsing behavior when no$refis present.Testing
go build ./...incli/azd/extensions/azure.ai.toolboxesgo test ./internal/cmd/...incli/azd/extensions/azure.ai.toolboxes