Skip to content

Follow up items from azd ai skill commands PR #8366

@trangevi

Description

@trangevi

Higher-impact (worth a follow-up issue at minimum)

  • CreateVersionFromZip buffers the full archive in memory — client.go (around the new multipart build). The pre-refactor CreatePackage took an io.ReadSeeker and streamed via httpReq.SetBody(streaming(archive), …). The new path does io.Copy(part, archive) into a bytes.Buffer and then bytes.NewReader(buf.Bytes()). Given the documented 512 MB archive ceiling, a near-cap upload allocates ≥512 MB heap (often 2× during buf.Bytes()), plus pipeline retry buffering. Easy OOM on constrained runners. Consider io.Pipe — a goroutine writes the files[] part + default field to the pipe writer, pass the pipe reader to httpReq.SetBody, set ContentLength = -1. Restoring the io.ReadSeeker parameter would also let the pipeline rewind on retry.

  • POST /skills/{name}/versions has no idempotency guard — the Azure SDK pipeline retries automatically. A client-side timeout on a request the server actually processed will create a second immutable version, and because both CreateVersionInline and CreateVersionFromZip set default: true, the retry silently re-flips default_version to the duplicate. The user is now running on a version they didn't intend. Options: send an Idempotency-Key if the service supports it, or set policy.RetryOptions{MaxRetries: 0} for these two requests so retries are explicit. Worth checking with the service team which they prefer.

Medium

  • --force is delete-then-create with no rollback, and the blast radius grew under versioning — DeleteSkill now wipes the skill and every version. If the subsequent CreateVersion* fails (network, 5xx, validation), all prior versions are gone. verifyFileNameMatches protects against the wrong-target case but not the upload-failure case. Two ideas: (a) on --force without --no-prompt, prompt with the version count and default_version name being deleted; (b) longer-term, implement --force as "upload new non-default version → repoint default → delete old versions" so an upload failure leaves the original intact.

  • SKILL.md license, compatibility, allowed_tools are silently dropped on upload — SkillInlineContent (models.go:51-54) exposes these fields on the wire, but ParseSkillMd (skill_md.go:31-87) only extracts name / description / metadata, and neither runFileMd nor buildInlineContent copies them through. Round-trip a SKILL.md and these fields disappear, even though README/AGENTS claim agentskills.io conformance. Either extend ParseSkillMd + propagate (recommended), or drop the fields from SkillInlineContent until they're plumbed end-to-end. Worth a round-trip test either way.

  • --set-default-version is not pre-flighted — UpdateSkillDefaultVersion blindly POSTs {default_version: X}. With no client-side GetSkillVersion first, "skill not found" / "version not found" / "version exists but half-deleted" can all surface as a generic server 400. A short pre-flight call + a CodeSkillVersionNotFound error code would let users distinguish these cases. (Also no test for the error path.)

  • update inline validation is split across two stages with a misleading error — validateFlags accepts at least one of --description / --instructions, but buildInlineContent requires both non-empty. Running azd ai skill update foo --description "new" passes the first gate, then fails at build time with "update requires non-empty instructions" — without ever explaining that update needs both. Help text reads (--description / --instructions) where the / looks like "or". Easiest fix: tighten validateFlags to require both when inline mode is used; update help to (--description AND --instructions).

  • The gzip-tar branch in archive.go is dead under the new content-type check — downloadContent (client.go:425-432) hard-rejects anything that isn't application/zip, but SafeExtract / DetectArchiveFormat still carry an ArchiveTarGz branch with a comment claiming "resilience". That branch can never trigger. Either restore Accept: application/zip, application/gzip on the download (real resilience), or delete ArchiveTarGz + the magic-byte sniff and call zip.NewReader directly. Pick one, drop the other.

Low / cleanup

  • client.go package-level downloadByteCap is a mutable global mutated by tests. Safe today only because nothing calls t.Parallel(). Promote to a Client field with a WithMaxDownloadBytes option.

  • skill_md.go SkillMdFileName constant is dead after c040787 removed the materialize path. Delete.

  • printCreateResult (skill_create.go) and the analogous post-update GET in skill_update.go silently swallow the follow-up GetSkill error. A fmt.Fprintf(os.Stderr, "Warning: ... follow-up GET failed: %v\n", err) would surface transient permission/network issues without failing the command.

  • skill_download.go accepts --version "" / --version " " and silently downloads the default version. strings.TrimSpace + reject empty would catch unset-$VER scripting bugs.

  • Worth adding a test that asserts update --description "x" (no --instructions) is rejected — documents the contract once you decide how to fix Lowercase template repository URL for telemetry #6.

  • --version flag help on download could note "must exist on the server" so users don't expect arbitrary strings.

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineering itemInternal engineering work itemext-agentsazure.ai.{agents,connections,inspector,projects,routines,skills,toolboxes} extensions

    Type

    No fields configured for Task.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions