Skip to content

fix(toolboxes): branch skill/connection mutations from latest version#9013

Open
glharper wants to merge 2 commits into
mainfrom
glharper/8674-toolbox-branch-from-latest
Open

fix(toolboxes): branch skill/connection mutations from latest version#9013
glharper wants to merge 2 commits into
mainfrom
glharper/8674-toolbox-branch-from-latest

Conversation

@glharper

@glharper glharper commented Jul 7, 2026

Copy link
Copy Markdown
Member

Fixes #8674

What

azd ai toolbox skill add/remove and azd ai toolbox connection add/remove created 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:

azd ai toolbox skill add web-search-toolbox greeting        # v2 = v1 + greeting
azd ai toolbox skill add web-search-toolbox code-review     # v3 = v1 + code-review  ❌ greeting dropped

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):

azd ai toolbox skill add web-search-toolbox greeting        # v2 = v1 + greeting
azd ai toolbox skill add web-search-toolbox code-review     # v3 = v2 + code-review  ✅ both present
  • --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.
  • When the new version is branched from a non-default version, a note is printed clarifying the branch source and that the default is unchanged (text output only).
  • The toolbox's default version is still unchanged — publish to promote, as before.

Implemented via a shared resolveBranchVersion helper (toolbox_branch.go) reused by all four verbs, reusing the existing numeric-aware versionSortDescending. 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 --help documents the accumulate semantics and --from-version.

Tests

  • skill add / skill remove / connection add branch from latest (v2), not default (v1), carrying forward v2's contents.
  • --from-version pins the branch source (v1) and does not carry v2's skills.
  • unknown --from-versiontoolbox_version_not_found, no version created.
  • empty version list → falls back to default (preserves prior behavior).
  • All existing toolbox tests still pass unchanged.

Validation

go build ./..., full go test ./..., go vet, gofmt, golangci-lint run (0 issues), and cspell — all clean.

Notes

  • Scope: this fixes the behavior (item 1 of the issue's "Proposed fix", preferred) plus the CLI-output note and --help wording (stopgap UX items 1–2). Public-docs updates (item 3) live in a separate docs repo and are out of scope for this PR.

`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>
Copilot AI review requested due to automatic review settings July 7, 2026 18:44
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

📋 Prioritization Note

Thanks for the contribution! The linked issue isn't in the current milestone yet.
Thank you for logging this issue; our team is reviewing it. If you need urgent prioritization, tag @RickWinter and @kristenwomack to let us know.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

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.go helper (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 --help text to document accumulate semantics and --from-version.
  • Added toolbox_version_not_found error code, a new toolbox_branch_test.go covering 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 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.

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 RickWinter 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.

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

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.

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 RickWinter 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.

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.

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

Labels

area/extensions Extensions (general)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[azure.ai.toolboxes] azd ai toolbox skill add silently drops earlier skills (branches from default version, not latest)

4 participants