fix(azure.ai.skills): address follow-up items from skill commands PR#8791
Conversation
…8366) - Extend ParseSkillMd to extract license, compatibility, and allowed_tools fields from SKILL.md front matter and propagate them through create and update inline flows, fixing silent field loss on round-trip. - Remove dead gzip-tar extraction code path from archive.go. The download endpoint always returns application/zip and the client rejects other content types, making the tar.gz branch unreachable. Simplified to zip-only with isZipMagic validation. - Promote downloadByteCap from a mutable package-level global to a Client struct field with WithMaxDownloadBytes option. Tests now use the instance method instead of mutating shared state. - Add stderr warning when the follow-up GetSkill call fails after create/update, so transient permission or network issues are visible to users instead of being silently swallowed. - Reject whitespace-only --version flag in skill download to catch scripting bugs like --version \\\. - Add tests for inline update requiring both --description and --instructions, ParseSkillMd field extraction, and download version validation. Closes #8366 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. |
There was a problem hiding this comment.
Pull request overview
This PR addresses several correctness and UX follow-ups in the azure.ai.skills azd extension, focusing on preserving SKILL.md front matter fields, removing unreachable archive handling code, making download size caps test-safe, and adding targeted validation/tests.
Changes:
- Extend
ParseSkillMd+ create/update SKILL.md flows to propagatelicense,compatibility, andallowed_toolsintoSkillInlineContent(preventing silent field loss). - Remove dead
tar.gzextraction support and replace it with a ZIP magic check prior to extraction. - Replace the mutable package-level download cap with a
Clientinstance field and add tests for validation/error paths (including whitespace--version).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/skill_md.go | Parses additional front matter fields (license, compatibility, allowed_tools) and adds list parsing helper. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/skill_md_test.go | Adds tests validating extraction of the new front matter fields and erroring on invalid allowed_tools. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/client.go | Moves download cap from mutable global to Client.maxDownloadCap with a testing override method. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/client_test.go | Updates download-size-cap tests to use the new client override instead of global mutation. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/archive.go | Removes tar.gz extraction path and validates ZIP magic bytes before extraction. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/archive_test.go | Removes tar.gz tests and adds tests for non-zip rejection + ZIP magic checking. |
| cli/azd/extensions/azure.ai.skills/internal/pkg/skill_api/archive_create_test.go | Updates archive creation assertions to use isZipMagic. |
| cli/azd/extensions/azure.ai.skills/internal/cmd/skill_update.go | Propagates new SKILL.md fields into update inline content; prints stderr warning on follow-up GET failure. |
| cli/azd/extensions/azure.ai.skills/internal/cmd/skill_update_test.go | Adds tests documenting inline update requiring both description and instructions. |
| cli/azd/extensions/azure.ai.skills/internal/cmd/skill_download.go | Adds whitespace-only --version validation to avoid accidental default-version downloads. |
| cli/azd/extensions/azure.ai.skills/internal/cmd/skill_download_test.go | Adds test for whitespace-only --version rejection. |
| cli/azd/extensions/azure.ai.skills/internal/cmd/skill_create.go | Propagates new SKILL.md fields into create inline content; prints stderr warning on follow-up GET failure. |
The previous check (version != '' && TrimSpace == '') missed the most
common scripting bug: --version \\\ expands to an empty
string so the guard was skipped. Now tracks cmd.Flags().Changed('version')
into versionSet and rejects when the flag was explicitly provided but
resolves to empty/whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
There was a problem hiding this comment.
Clean, well-scoped PR that addresses real correctness issues from #8366. Six focused fixes, each with targeted tests. A few observations:
Field propagation (skill_md.go, skill_create.go, skill_update.go): The License, Compatibility, and AllowedTools fields were already declared in SkillInlineContent but silently dropped during SKILL.md round-trips. Now properly wired through both create and update flows. The new frontMatterStringSlice helper reuses frontMatterString for consistent error reporting per element.
Dead code removal (archive.go): Removing ~55 lines of unreachable tar.gz support is the right call since the download endpoint always returns application/zip and the client already hard-rejects other content types. The replacement isZipMagic check is simple and sufficient.
Global-to-instance migration (client.go): Moving downloadByteCap to a Client struct field with WithMaxDownloadBytes for test overrides eliminates shared mutable state. Tests now get proper isolation without t.Cleanup gymnastics on a package global.
Version validation (skill_download.go): The versionSet + TrimSpace guard catches both whitespace-only and empty-string cases when the flag is explicitly provided. Good use of Cobra's cmd.Flags().Changed() to distinguish "flag absent" from "flag present with empty value".
Note on the existing Copilot reviewer comment: The concern about --version "$UNSET_VAR" expanding to empty string is actually handled by this PR. cmd.Flags().Changed("version") returns true when the flag appears on the command line regardless of value, and TestDownloadAction_RejectsEmptyVersionWhenFlagSet explicitly validates this path.
No blocking issues. CI is green across all platforms.
Motivation
Issue #8366 tracked several follow-up items from the initial
azd ai skillextension PR -- silent data loss on SKILL.md round-trips, dead code paths, test-unsafe mutable globals, and missing validation. This PR addresses the code-level fixes that don't require service-side coordination.Approach
Six focused fixes across the
azure.ai.skillsextension:SKILL.md field propagation:
ParseSkillMdnow extractslicense,compatibility, andallowed_toolsfrom front matter and bothcreate --fileandupdate --fileflows propagate them intoSkillInlineContent. Previously these fields were silently dropped on upload despite being declared in the wire model.Dead tar.gz code removal: The download endpoint always returns
application/zipand the client hard-rejects other content types, making theArchiveTarGzextraction branch unreachable. Replaced with a simpleisZipMagiccheck and removed ~55 lines of dead code plus unused imports.downloadByteCappromoted to instance field: The package-level mutable global (only safe because tests never ran in parallel) is now aClientstruct field initialized toMaxDownloadBytes, with aWithMaxDownloadBytesmethod for test overrides.Stderr warnings for swallowed errors:
printCreateResultandprintUpdateResultnow emit a warning when the follow-upGetSkillcall fails, instead of silently falling back to the version envelope.Whitespace
--versionrejection:azd ai skill download my-skill --version " "now returns a clear validation error instead of silently downloading the default version.New tests: Covers inline update requiring both flags,
ParseSkillMdfield extraction, download version validation, and non-zip archive rejection.Items tracked separately
The following higher-impact items from #8366 need design decisions or service-team coordination and are not addressed here:
CreateVersionFromZipmemory buffering (io.Pipe streaming refactor)/skills/{name}/versionsidempotency (needs Idempotency-Key header support)--forcedelete-then-create rollback safety (UX design decision)--set-default-versionpre-flight check (error code coordination)Fixes: #8366