fix(toolboxes): branch skill/connection mutations from latest version#9013
fix(toolboxes): branch skill/connection mutations from latest version#9013glharper wants to merge 2 commits into
Conversation
`azd ai toolbox skill add/remove` and `connection add/remove` created the new immutable toolbox version by branching from the toolbox's DEFAULT version, not its latest. Sequential mutations therefore produced sibling versions that each dropped earlier changes (e.g. two `skill add` calls yielded a v3 containing only the second skill), with no warning — a silent, data-loss-shaped trap. Branch from the LATEST version by default so sequential mutations accumulate (v3 = v2 + change). Add a shared `--from-version <n>` flag to override the branch source (the prior default-snapshot behavior is still reachable via `--from-version <default>`), and print a note when the new version was branched from a non-default version. The toolbox's default version is still unchanged. Applied symmetrically to all four verbs via a shared resolveBranchVersion helper. When a toolbox reports no versions (edge case), it falls back to the default version, so existing behavior is preserved. Duplicate/not-found messages now name the specific version being extended, and the skill/connection group help documents the accumulate semantics and --from-version. Fixes #8674 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #8674 in the azure.ai.toolboxes extension, where azd ai toolbox skill add/remove and connection add/remove branched each new immutable toolbox version from the toolbox's default version instead of its latest. That caused sequential mutations to produce sibling versions that silently dropped earlier changes — a data-loss-shaped UX trap. The fix branches from the latest version by default so mutations accumulate, adds a shared --from-version override, and surfaces a clarifying note when branching from a non-default version.
Changes:
- New shared
toolbox_branch.gohelper (resolveBranchVersion,registerFromVersionFlag,printBranchNote) reused by all four verbs; branches from latest by default, validates--from-version, and falls back to the default when no versions are reported. - Wired the four verbs to branch from the resolved version, updated duplicate/not-found messages to name the specific version being extended, and refreshed group
--helptext to document accumulate semantics and--from-version. - Added
toolbox_version_not_founderror code, a newtoolbox_branch_test.gocovering latest-branching/override/not-found/empty-list cases, and a CHANGELOG entry (1.0.0-beta.2).
The implementation is consistent with existing patterns: fmt.Printf for user-facing stdout matches the rest of the extension, versionSortDescending is correctly reused for numeric-aware "latest" resolution, and the accumulation logic (slices.Clone(current.Skills/Tools)) carries forward the branched-from version's contents. Tests exercise the shared helper end-to-end for skill add/remove and connection add, plus the edge cases. I found no correctness defects. Note that this deliberately changes the default behavior of customer-facing (preview) CLI commands, which is the main reason for the assessment below.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/cmd/toolbox_branch.go |
New shared helper resolving the branch source, registering --from-version, and printing the non-default branch note. |
internal/cmd/toolbox_branch_test.go |
New tests for latest-branching, --from-version override/not-found, and empty-list fallback. |
internal/cmd/toolbox_skill_add.go |
Branches from resolved version; updates duplicate message; prints branch note. |
internal/cmd/toolbox_skill_remove.go |
Adds --from-version; branches from resolved version; updates not-attached message; prints branch note. |
internal/cmd/toolbox_connection_add.go |
Adds --from-version; branches from resolved version; prints branch note. |
internal/cmd/toolbox_connection_remove.go |
Adds --from-version; branches from resolved version; updates not-attached message; prints branch note. |
internal/cmd/toolbox_skill_group.go |
Updates group --help to document accumulate semantics and --from-version. |
internal/cmd/toolbox_connection.go |
Updates group --help to document accumulate semantics and --from-version. |
internal/exterrors/codes.go |
Adds CodeToolboxVersionNotFound error code. |
CHANGELOG.md |
Adds 1.0.0-beta.2 unreleased bug-fix entry. |
jongio
left a comment
There was a problem hiding this comment.
Verified that
esolveBranchVersion correctly sorts existing versions descending and branches from the latest, that --from-version validation rejects unknown versions with an actionable error, and that the fallback to DefaultVersion on an empty version list preserves prior behavior. The four verbs (skill add/remove, connection add/remove) all use the shared helper consistently, and the tests cover the key scenarios including the original regression case from #8674.
RickWinter
left a comment
There was a problem hiding this comment.
This changes skill/connection add and remove to branch the new immutable version from the toolbox's latest version instead of its default, fixing the silent drop of earlier mutations in #8674, and adds a shared --from-version override plus a text-mode note. The approach is the right shape: one resolveBranchVersion helper feeds all four verbs, it reuses the existing numeric-aware versionSortDescending, and the duplicate and not-found messages now name the specific version being extended rather than "current default". Tests cover latest-vs-default for add and remove, --from-version pinning, an unknown version rejected as toolbox_version_not_found, and the empty-list fallback to default.
One question inline on how "latest" is derived. The extra ListToolboxVersions call added to every mutation is a reasonable cost for computing latest and is correct to leave as is. The empty-list branch trusting an unchecked --from-version is already documented in the code and surfaces cleanly on the later GetToolboxVersion, so that is fine too. Disposition: COMMENT, nothing blocking.
| slices.SortFunc(sorted, func(a, b azure.ToolboxVersionObject) int { | ||
| return versionSortDescending(a.Version, b.Version) | ||
| }) | ||
| latest = sorted[0].Version |
There was a problem hiding this comment.
Here "latest" is computed as the highest version number rather than a tip pointer the service hands back. That is a fine proxy while versions only ever increment, but it assumes max-numeric always equals the most recently mutated version. If a version can be deleted such that the max number is no longer the true tip, or if the service exposes its own latest/head, this would branch from the wrong base and reintroduce a quieter form of the same drop this PR fixes. For a preview feature I think computing it here is acceptable. Can we confirm the toolbox API has no canonical latest we should prefer over deriving it?
RickWinter
left a comment
There was a problem hiding this comment.
This changes skill/connection add and remove to branch the new immutable version from the toolbox's latest version instead of its default, fixing the silent data loss (#8674) where sequential mutations forked from default and dropped earlier changes. The approach is right: a shared resolveBranchVersion helper reused by all four verbs, latest computed with the existing numeric-aware versionSortDescending, an explicit --from-version override that validates against the version list before use, and a text-only note when the branch source is not the default.
The edge cases are handled well. An empty version list falls back to the default version, preserving prior behavior, and the duplicate and not-found messages now name the specific version being extended rather than "current default version", which matches the new semantics. Each mutation now makes one extra ListToolboxVersions call, which is inherent to resolving latest and is fine.
Tests cover branch-from-latest for skill add/remove and connection add (carrying v2's contents forward), the --from-version pin not carrying v2's skills, the unknown --from-version rejection with no version created, and the empty-list fallback. No correctness or security concerns.
Disposition: no blockers.
Fixes #8674
What
azd ai toolbox skill add/removeandazd ai toolbox connection add/removecreated the new immutable toolbox version by branching from the toolbox's default version rather than its latest. Sequential mutations therefore produced sibling versions that each silently dropped earlier changes:No error, no warning — a silent, data-loss-shaped UX trap on a preview feature.
Fix
Branch from the latest version by default so sequential mutations accumulate (
v3 = v2 + change):--from-version <n>(new, shared across all four verbs) overrides the branch source. The prior default-snapshot behavior is still reachable via--from-version <default-version>. An unknown version is rejected with a clear error.publishto promote, as before.Implemented via a shared
resolveBranchVersionhelper (toolbox_branch.go) reused by all four verbs, reusing the existing numeric-awareversionSortDescending. When a toolbox reports no versions (edge case; a real toolbox always has its default), it falls back to the default version, preserving today's behavior. Duplicate/not-found messages now name the specific version being extended, and the skill/connection group--helpdocuments the accumulate semantics and--from-version.Tests
skill add/skill remove/connection addbranch from latest (v2), not default (v1), carrying forward v2's contents.--from-versionpins the branch source (v1) and does not carry v2's skills.--from-version→toolbox_version_not_found, no version created.Validation
go build ./..., fullgo test ./...,go vet,gofmt,golangci-lint run(0 issues), andcspell— all clean.Notes
--helpwording (stopgap UX items 1–2). Public-docs updates (item 3) live in a separate docs repo and are out of scope for this PR.