Add azd ai dataset extension for Foundry datasets - #9499
Add azd ai dataset extension for Foundry datasets#9499Mohamed Hessien (m7md7sien) wants to merge 45 commits into
Conversation
Datasets are a general Foundry asset that fine-tuning and other scenarios need as much as evaluation does, so the CRUD group ships as its own extension rather than inside azure.ai.evaluations. Generation stays with the evaluation extension, because it writes that extension's config file. Standalone: no dependency on azure.ai.evaluations, its own go module, lint workflow and release pipeline.
Adds internal/messages, holding the extension's errors, warnings and status lines behind one named function each, grouped under Datasets, Config, Output and Talking to the service. Wording is unchanged; the call sites now name the situation instead of spelling out the sentence. Cobra help, wire constants and table headers stay where they are.
Three things carried over from the extension this was carved out of. The User-Agent announced azure-ai-evaluations and the debug log was written to azd-ai-eval-<date>.log, so a dataset request could not be told from an eval one in either a service log or a local one. The FORMAT column was worse than wrong: format is a field the API accepts on upload and never sends back, so the column was blank for every dataset ever listed. It shows type, which the service does return.
Carved from the same code, so it had the same two: a nil Environment or GetValue response panicking inside the endpoint cascade, and an azd client abandoned when endpoint or credential setup failed.
Same credential failure and same 401/403 path as the eval extension, which this was carved from.
Same intermittent credential failure as the eval extension.
Same three as the eval extension, which this was carved from.
|
Azure Pipelines: 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
A BOM at the head of a .jsonl is uploaded as part of the first row's first key, so every consumer of the dataset sees one malformed record -- and nothing fails until something tries to read that row. Notepad, VS Code on Windows and PowerShell all write one. A file holding nothing but a BOM is still empty, and is still refused as such.
Found by bug-bashing the off-path cases. A missing dataset reported "has no versions", which is not what happened -- a dataset cannot exist with no versions, and the service answers an unknown name with an empty list rather than a 404. Listing its versions was worse: it said "No datasets found", as though the project had none at all. Both now say the name is not a dataset and point at the listing, matching how the evaluations extension already answers the same question. A name with a space took a file upload and then a 400 carrying four levels of nested JSON, with the sentence that matters at the bottom. The rule is simple and documented, so it is checked before the round trip.
A mistyped path surfaced as 'GetFileAttributesEx <path>: The system cannot find the file specified.' The syscall name is noise to the person who mistyped it.
--from-file <file> resolved to the file's DIRECTORY, and the upload helper then took whichever .jsonl sorted first. Pointing at one dataset in a folder holding several registered a different one under that name. A test pinned the old behaviour: 'a file resolves to the directory the upload scans'.
The unit tests cover the client layer and the helpers; nothing exercised the command surface a user touches - flag parsing, exit codes, the rendered table, and whether -o json emits something a script can consume. 18 tests behind the live tag, gated on AZURE_AI_DATASET_E2E_LIVE, type-checked by ci-test.ps1 like the eval extension's suites.
|
Azure Pipelines: 7 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
Adds a standalone Foundry dataset extension with CRUD/versioning commands, API clients, tests, documentation, and release automation.
Changes:
- Adds dataset creation, updates, listing, inspection, deletion, and version listing.
- Adds endpoint resolution, validation, structured errors, and JSON/table output.
- Adds unit/live tests plus build, lint, and release infrastructure.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
eng/pipelines/release-ext-azure-ai-dataset.yml |
Adds extension release pipeline. |
cli/azd/extensions/azure.ai.dataset/version.txt |
Sets initial beta version. |
cli/azd/extensions/azure.ai.dataset/tests/cli/harness_test.go |
Adds live CLI harness. |
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go |
Tests dataset CLI workflows. |
cli/azd/extensions/azure.ai.dataset/README.md |
Documents installation and commands. |
cli/azd/extensions/azure.ai.dataset/main.go |
Adds extension entry point. |
cli/azd/extensions/azure.ai.dataset/internal/version/version.go |
Defines build metadata. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/version_test.go |
Tests version handling. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/uri_test.go |
Tests URI resolution. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/upload_version_test.go |
Tests conflict-aware uploads. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go |
Implements dataset and storage operations. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations_wire_test.go |
Tests API request contracts. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go |
Defines API models and file handling. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/list.go |
Implements listing and version ordering. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/download_wire_test.go |
Tests download HTTP behavior. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/download_test.go |
Tests blob selection logic. |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/bom_test.go |
Tests BOM and file handling. |
cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go |
Centralizes user-facing messages. |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/validator.go |
Validates Foundry endpoints. |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/types.go |
Defines endpoint-resolution types. |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/store.go |
Reads shared project context. |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go |
Implements endpoint precedence. |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver_test.go |
Tests endpoint resolution. |
cli/azd/extensions/azure.ai.dataset/internal/exterrors/errors.go |
Adds structured error helpers. |
cli/azd/extensions/azure.ai.dataset/internal/exterrors/codes.go |
Defines structured error codes. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/table_test.go |
Tests table rendering. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/surface_test.go |
Tests command surface and flags. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/root.go |
Builds the command tree. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go |
Implements JSON and text output. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/output_test.go |
Tests output formatting. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/names.go |
Validates dataset names. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/names_test.go |
Tests naming constraints. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/helpers_test.go |
Tests command helpers. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/debug.go |
Configures debug logging. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go |
Implements dataset commands. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/context.go |
Creates command service context. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/artifacts.go |
Defines persistence helpers. |
cli/azd/extensions/azure.ai.dataset/internal/cmd/apiversions.go |
Defines service API versions. |
cli/azd/extensions/azure.ai.dataset/go.mod |
Defines the Go module. |
cli/azd/extensions/azure.ai.dataset/extension.yaml |
Declares extension metadata. |
cli/azd/extensions/azure.ai.dataset/cspell.yaml |
Configures spell checking. |
cli/azd/extensions/azure.ai.dataset/ci-test.ps1 |
Adds CI test execution. |
cli/azd/extensions/azure.ai.dataset/ci-build.ps1 |
Adds CI binary builds. |
cli/azd/extensions/azure.ai.dataset/CHANGELOG.md |
Records the initial release. |
cli/azd/extensions/azure.ai.dataset/build.sh |
Adds Unix cross-platform builds. |
cli/azd/extensions/azure.ai.dataset/build.ps1 |
Adds Windows cross-platform builds. |
cli/azd/extensions/azure.ai.dataset/.golangci.yaml |
Configures Go linting. |
cli/azd/extensions/azure.ai.dataset/.gitignore |
Ignores generated artifacts. |
.github/workflows/lint-ext-azure-ai-dataset.yml |
Adds GitHub lint workflow. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two lines of ci-test.ps1 assumed PowerShell 6 or later. $IsWindows does not exist on Windows PowerShell 5.1, so the .exe suffix was never appended; and Join-Path there takes a single child path, so the three-argument call failed outright. The script then found no gotestsum, printed the fallback notice, and ran bare `go test` with no JUnit report -- while still exiting 0, which is why it went unnoticed.
The lint/go-fix gate rewrites strings.Split to the Go 1.24 iterator and fails when that rewrite is not already applied.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated no new comments.
Suppressed comments (9)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:374
- This reverses the documented and tested idempotence guarantee: when the service returns 404 for an already-absent version,
deleteexits nonzero. Treat not-found as an already-completed delete and continue to the normal human/JSON success response. [azd-code-reviewer]
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
cli/azd/extensions/azure.ai.dataset/README.md:41
- The documented cascade omits both the persisted
azd ai project setvalue and theAZURE_AI_PROJECT_ENDPOINTfallback, so users may troubleshoot or override the wrong source. Document all four implemented levels in their actual precedence order. [azd-code-reviewer]
Every command resolves the Foundry project endpoint in this order:
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
- The advertised empty-and-success behavior does not handle the API's 404 form for an unknown dataset.
TestUploadNextVersionTreatsAnUnknownDatasetAsVersionlessexplicitly models/versionsreturning 404, but this command returns that error instead of[]. Normalize only not-found here and keep propagating other failures. [azd-code-reviewer]
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/README.md:22
- This usage is incomplete: the implementation requires
--version, so copying the documented command always fails before deletion. Include the required flag in the command table. [azd-code-reviewer]
| `azd ai dataset delete <name>` | Delete a dataset version |
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:139
- This extension newly reads
AZURE_AI_PROJECT_ENDPOINT, but that variable is absent fromcli/azd/docs/environment-variables.md, and the existingFOUNDRY_PROJECT_ENDPOINTentry still says it is agents-only. Update the environment-variable reference so this fallback is discoverable and its precedence is accurate. [azd-code-reviewer]
cli/azd/extensions/azure.ai.dataset/tests/cli/harness_test.go:130 - This test harness cannot actually detect the hang asserted by
TestCLIRequiredValuesFailInsteadOfHanging:exec.Commandhas no deadline, so a regression blocks until the entirego testtimeout. Run subprocesses withexec.CommandContextand a short per-command timeout, and fail with the captured output when it expires. [azd-code-reviewer]
cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:17 - The release note claims a user-visible read-back feature, but no command calls
DownloadDatasetContent; only tests do, and the package is internal to this module. Either expose the claimed capability through the CLI or remove it from the release notes. [azd-code-reviewer]
- Reads dataset content back, whether the service hands out a blob URI or the
container holding it.
cli/azd/extensions/azure.ai.dataset/.golangci.yaml:12
- The repository's Go guidance sets
lllto 125 characters (cli/azd/AGENTS.md:97-108), but this module allows 220, so its dedicated CI accepts lines the repository standard rejects. Restore the 125-character limit and wrap the existing overlong source line. [azd-code-reviewer]
line-length: 220
cli/azd/extensions/azure.ai.dataset/internal/cmd/surface_test.go:245
- Remove the accidental
//embedded in this sentence and wrap it to the repository's 125-character limit. [azd-code-reviewer]
// A message pointing at `azd ai eval dataset ...` is almost always the copy// these commands came from rather than a deliberate cross-extension pointer.
The Blob service answers a container listing one page at a time and returns a NextMarker, which this dropped -- it parsed the first page and stopped. DownloadDatasetContent falls back to listing the container and taking the first .jsonl by name, so a container holding more objects than fit in a page could report no file at all, or a different one, depending on where the page happened to end. The marker is the service's own opaque value replayed into a URL this code builds, so the walk carries none of the risk that following a body-supplied link would. Ported from the evaluations extension, which already had this and the tests for it; this copy had neither. Also makes blobHTTPClient a single shared client, as that copy does. Eighth divergence found between the two copies of dataset_api. Reported by a review agent diffing them.
The URI is the SAS the dataset service issued, not caller input, which is why the evaluations copy already suppresses G704 on the same call. Porting the paginated listing without it failed golangci-lint.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated no new comments.
Suppressed comments (8)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:375
- [azd-code-reviewer] This explicitly turns a not-found delete into failure, while
TestCLIDeleteIsIdempotentdefines deleting an unregistered version as successful. Treat 404 as an idempotent success so cleanup scripts do not fail when the version is already absent.
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
}
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:99
- [azd-code-reviewer] A non-404 listing failure leaves
exists=false, the probes ignore their errors, andcreateis then allowed to continue. If the transient failure clears whenUploadNextVersionlists again,createcan publish a new version of an existing dataset instead of failing as already-exists. Propagate an inconclusive list error forcreate.
if err := checkAssetExistence(
verb, "dataset", name, exists, dataset_api.IsNotFound(listErr),
); err != nil {
cli/azd/extensions/azure.ai.dataset/README.md:41
- [azd-code-reviewer] This omits two implemented resolution levels:
AZURE_AI_PROJECT_ENDPOINTis a fallback at both environment levels, and persistedazd ai project setcontext is checked before host variables. Users following this order can diagnose or update the wrong source. Document the actual four-level cascade.
Every command resolves the Foundry project endpoint in this order:
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/internal/cmd/root.go:16
- [azd-code-reviewer] The inherited
--environment/AZD_ENVIRONMENTvalue is discarded here. Both endpoint resolution and version persistence callEnvironment().GetCurrent(), which resolves the project's default environment, so-e stagingcan target the default Foundry project and writeEVAL_DATASET_VERSIONinto the wrong environment. ThreadextCtx.Environmentthrough the context/resolver and use the named environment when it is set.
rootCmd, _ := azdext.NewExtensionRootCommand(azdext.ExtensionCommandOptions{
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
- [azd-code-reviewer] A 404 still returns an error here, contradicting the new command contract and the live tests that require an unknown name to produce an empty successful list. Normalize
IsNotFound(err)to an emptyDatasetList; otherwiseversions list <unknown>still exits 1 on services that report absence with 404.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:34
- [azd-code-reviewer] Unsupported output values are silently treated as human/table output because only
jsonis recognized. For example,-o yamlexits successfully with non-YAML text, which can break automation. Register per-command output options with the extension SDK (for examplejsonandtable) so unsupported values fail validation.
return strings.ToLower(v)
}
// isJSON reports whether the command should emit machine-readable output.
func isJSON(cmd *cobra.Command) bool {
return outputFormat(cmd) == outputJSON
cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:357
- [azd-code-reviewer] This message always claims
--no-promptis active, butrequireFlagis also called during ordinary interactive invocations such asdataset delete <name>. The resulting error is factually wrong; either prompt when allowed or use a mode-neutral required-flag message.
cli/azd/extensions/azure.ai.dataset/README.md:22 - [azd-code-reviewer] The documented delete invocation always fails because the implementation requires
--version. Include the required flag so the command table is directly runnable.
| `azd ai dataset delete <name>` | Delete a dataset version |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated no new comments.
Suppressed comments (7)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:376
- A 404 currently becomes
DatasetVersionNotFound, so deleting an already-absent version exits non-zero. This directly contradicts the added idempotence test. Treat not-found as successful deletion and only return other service failures. (azd-code-reviewer)
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
}
return messages.DeletingDatasetVersion(name, version, err)
cli/azd/extensions/azure.ai.dataset/README.md:42
- The documented cascade omits two implemented sources: persisted
azd ai project setcontext and theAZURE_AI_PROJECT_ENDPOINTfallback at both environment levels. Users can therefore get an endpoint they did not expect without the README explaining its precedence. Document all four levels in resolver order. (azd-code-reviewer)
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:249
- An unknown dataset is returned as HTTP 404 by this API, so this branch currently exits with an error even though the new CLI tests require
versions listto succeed with an empty array. Normalize a not-found response to an emptyDatasetList; preserve other failures. (azd-code-reviewer)
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:309
- The API can return 404 for this version listing, but this path wraps it as a raw “resolving latest version” failure. That contradicts
TestCLIUnknownDatasetIsBrief, which expects the same conciseDatasetNotFoundresult used for an empty listing. Translate not-found before wrapping other errors. (azd-code-reviewer)
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ResolvingLatestDatasetVersion(name, err)
cli/azd/extensions/azure.ai.dataset/README.md:22
- This documented invocation always fails because
deleterequires--version. Include the required flag so the command table is executable as written. (azd-code-reviewer)
| `azd ai dataset delete <name>` | Delete a dataset version |
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:44
- These transport aliases are serialized directly by every
-o jsonpath. A response usingdata_uritherefore emitsdata_uri, while a response usingdataUriemitsdataUri, so scripts receive different schemas for the same field. MapDatasetto a stable CLI output type that always exposes one resolveddataUrifield. (azd-code-reviewer)
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:51 - Cleanup silently returns when listing or JSON decoding fails and ignores every delete exit code, so the live suite can pass while leaving datasets behind. Make cleanup failures fail the test instead of swallowing them. (azd-code-reviewer)
… copy does The two copies of dataset_api had drifted in exactly the two places bugs kept appearing: SAS redaction lived inline here and in an internal/urlsafe package there, and paging was split across list.go and operations.go here but had its own file there. Nine divergences have been found between these copies so far, four of them in these two areas. This adopts the eval extension's shape so the remaining difference is a handful of function names rather than a different structure. Once both extensions are in main, a test can diff the two surfaces and fail on drift; it cannot run today because they are on separate branches.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated no new comments.
Suppressed comments (8)
cli/azd/extensions/azure.ai.dataset/internal/cmd/root.go:45
- [azd-code-reviewer] None of the leaf commands registers its supported
--outputvalues, so the SDK neither validates the flag nor publishes valid values in command metadata; for example,-o jsnosilently falls back to human output. Registeroutputoptions on each executable command, as inazure.ai.projects/internal/cmd/project_show.go:59-63; the SDK contract is documented inpkg/azdext/extension_command.go:250-260.
rootCmd.AddCommand(
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
- [azd-code-reviewer] This still returns an error when the service represents an unknown dataset with 404, contradicting the documented empty-success behavior and
TestCLIVersionsListOfAnUnknownNameSucceeds. Convert only a not-found response into an empty list; preserve other failures.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:377
- [azd-code-reviewer] Returning
DatasetVersionNotFoundmakes deletion non-idempotent, while the PR's intended contract andTestCLIDeleteIsIdempotentrequire a missing version to succeed. Ignore only 404 and continue to the normal success output; keep surfacing all other failures.
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
}
cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:357
- [azd-code-reviewer] This message always claims
--no-promptis active, but both create and delete call it whenever a required flag is absent, including ordinary interactive invocations. Remove the mode claim unless the caller actually checked that flag.
cli/azd/extensions/azure.ai.dataset/README.md:22 - [azd-code-reviewer] This documented command always fails because
deleterequires--version. Include the required flag so the command table is executable as written.
| `azd ai dataset delete <name>` | Delete a dataset version |
cli/azd/extensions/azure.ai.dataset/README.md:41
- [azd-code-reviewer] The documented resolution order omits two implemented sources: the
AZURE_AI_PROJECT_ENDPOINTfallback at both environment levels and the persisted global project context between them. Users debugging why a flagless command selected an endpoint would get the wrong precedence from this section.
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:17
- [azd-code-reviewer] No command exposes content download and
DownloadDatasetContenthas no production caller, so this release-note bullet advertises a capability users cannot invoke. Remove the bullet or add the corresponding command surface.
- Reads dataset content back, whether the service hands out a blob URI or the
container holding it.
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:309
- [azd-code-reviewer] An unknown dataset can produce 404 from the version-list call (the API tests explicitly model this), so
show <unknown>currently returns the wrapped HTTP response instead of the briefDatasetNotFounderror promised byTestCLIUnknownDatasetIsBrief. Translate not-found here before wrapping other listing failures.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ResolvingLatestDatasetVersion(name, err)
The shared redaction package arrived with the convergence commit but its name did not, so cspell failed on every import of it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated no new comments.
Suppressed comments (8)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:375
- azd-code-reviewer: This contradicts
TestCLIDeleteIsIdempotent: deleting a missing version returns an error here. Treat the service's 404 as successful no-op deletion so cleanup scripts remain idempotent.
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
}
cli/azd/extensions/azure.ai.dataset/README.md:41
- azd-code-reviewer: The documented endpoint cascade omits the
AZURE_AI_PROJECT_ENDPOINTfallbacks and the globalextensions.ai-agents.project.contextsource thatprojectctx.Resolvechecks before host variables. Users can therefore be surprised by an endpoint taking precedence over the three documented sources.
Every command resolves the Foundry project endpoint in this order:
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/README.md:22
- azd-code-reviewer: The documented delete invocation always fails because the implementation requires
--version. Include the required flag so the command table contains a runnable example.
| `azd ai dataset delete <name>` | Delete a dataset version |
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
- azd-code-reviewer: A 404 still takes this error path, so
versions list <unknown>exits nonzero instead of returning the empty result promised by the new tests and command comment. Convert onlyIsNotFound(err)to an emptyDatasetList; preserve other service failures.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29
- azd-code-reviewer: Any output value other than
jsonis silently treated as human output, so a typo such as-o jsnsucceeds and emits a table instead of rejecting an unsupported format. Register per-commandazdext.FlagOptionswith the supported values/defaults, as required for SDK-managed inherited flags.
v, err := cmd.Flags().GetString("output")
if err != nil {
return ""
}
return strings.ToLower(v)
cli/azd/extensions/azure.ai.dataset/README.md:6
- azd-code-reviewer: This onboarding command is guaranteed to fail in the state introduced by this PR: lines 54-57 explicitly say the missing registry entry prevents
azd extension installfrom resolving. Replace it with working local pack/publish instructions until the registry entry lands, or add that entry before publishing this README.
This issue also appears on line 22 of the same file.
$ azd extension install azure.ai.dataset
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:52
- azd-code-reviewer: Cleanup silently returns when listing or decoding fails, and it also ignores failed delete results. A transient service failure can therefore leave live datasets behind while the suite remains green; report cleanup failures and continue attempting the remaining versions.
cli/azd/extensions/azure.ai.dataset/CHANGELOG.md:17 - azd-code-reviewer: No command calls
DownloadDatasetContent; only unit tests exercise it, andshowreturns metadata rather than file content. This release note advertises a user-visible capability the extension does not expose. Remove the bullet or add the corresponding command surface.
- Reads dataset content back, whether the service hands out a blob URI or the
container holding it.
This reported only the base name, so --from-file ./data/golden.jsonl failing came back as eading golden.jsonl, which is ambiguous the moment a project holds more than one file by that name and is not what the caller typed. The parameter is called path and the message reads eading %s, so a base name was the odd one out; the sibling call in the same function passes a directory entry's name, where the name is all that is meaningful. Thirteenth and last behavioural divergence between the duplicated packages: the two copies now differ only in the User-Agent each sends, which is correct, and one intermediate variable.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
versions liststill returns an error when the service represents an unknown dataset as 404. That contradicts the new empty-success contract andTestCLIVersionsListOfAnUnknownNameSucceeds; the mocked service inversion_selection_test.go:76-78confirms this response shape. Translate onlyIsNotFound(err)to an emptyDatasetList, while preserving other failures.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/internal/cmd/output.go:29
- Unsupported output values silently fall through to human-readable output: for example,
-o yamlexits successfully but emits a table/detail instead of rejecting the typo. That is unsafe for scripts expecting machine-readable output. Register the supported values throughazdext.RegisterFlagOptions, as sibling extensions do (for exampleazure.ai.skills/internal/cmd/skill_list.go:51-53), so invalid formats fail before execution.
v, err := cmd.Flags().GetString("output")
if err != nil {
return ""
}
return strings.ToLower(v)
cli/azd/extensions/azure.ai.dataset/README.md:41
- The documented endpoint order omits two sources that the implementation checks: the shared
extensions.ai-agents.project.context.endpointconfig and theAZURE_AI_PROJECT_ENDPOINTfallback at both environment levels. This can make users troubleshoot or override the wrong source. Document the same four-level precedence used byprojectctx.Resolve.
Every command resolves the Foundry project endpoint in this order:
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/operations.go:584
- Malformed or truncated Blob Storage XML is converted into an empty successful page. The caller then reports that the dataset has no downloadable file, hiding the actual service/transport response problem. Return the XML unmarshal error from
parseBlobPageand propagate it throughreadBlobPageinstead of treating parse failure as an empty container.
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:47 - Cleanup silently returns when listing or JSON decoding fails and also ignores delete exit codes, so failed live tests can leave datasets in the shared Foundry project without reporting that leak. Record cleanup failures with
t.Errorfwhile continuing through the remaining versions so leaked service state is visible and actionable.
| envVal, valErr := azdClient.Environment().GetValue(ctx, &azdext.GetEnvRequest{ | ||
| EnvName: envResp.Environment.Name, | ||
| Key: key, | ||
| }) | ||
| if valErr == nil && envVal.GetValue() != "" { |
Redaction covered what http.Client.Do returns and stopped there. Two earlier paths were left: url.Parse of the container SAS at the top of UploadBlob, DownloadBlob and ListContainerBlobs, and the request construction that follows. Both hand back an error naming the URL, so a SAS the parser refuses reached the user's screen and the debug log with its signature intact. Seven sites across the two copies now wrap with urlsafe.Error, which rebuilds a *url.Error without the URL. A test drives all four entry points with a URL the parser refuses that still carries a sig, and asserts neither the signature nor the parameter name survives. Worth noting how it was found: reading the code caught the construction sites, and the test caught the parse sites -- three of its four cases failed on first run against the fix I had just written.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 65 changed files in this pull request and generated no new comments.
Suppressed comments (8)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:309
- azd-code-reviewer: A latest-version lookup that returns 404 currently surfaces the raw listing failure, even though this command promises the brief
DatasetNotFounderror for an unknown name. Handle not-found here as well as the empty-200 form; otherwise the result depends on which absence response the service returns.
if version == "" {
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ResolvingLatestDatasetVersion(name, err)
}
cli/azd/extensions/azure.ai.dataset/README.md:42
- azd-code-reviewer: The documented cascade omits two implemented sources: the persisted
extensions.ai-agents.project.context.endpointvalue and theAZURE_AI_PROJECT_ENDPOINTfallback at both environment levels. This can make users diagnose the wrong source when a command targets an unexpected project. Document all four levels in their actual precedence order.
Every command resolves the Foundry project endpoint in this order:
1. `--project-endpoint`
2. `FOUNDRY_PROJECT_ENDPOINT` in the active azd environment
3. the host environment variable of the same name
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:374
- azd-code-reviewer: This explicitly turns a 404 into failure, so delete is not actually idempotent when the service reports an absent version with not-found. Ignore that status and continue to the normal success output; the live test's current backend behavior should not be what guarantees idempotence.
if err := ec.datasetClient.DeleteDatasetVersion(
ctx, name, version, ProjectEndpointAPIVersion,
); err != nil {
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:243
- azd-code-reviewer: The API can represent an unknown dataset with a 404 (the new version-selection test models exactly that), but this branch returns the error before
renderDatasetscan produce the documented empty successful result. Normalize not-found to an empty list soversions list <unknown>reliably matches its test and CLI contract.
This issue also appears on line 305 of the same file.
list, err := ec.datasetClient.ListDatasetVersions(ctx, name, ProjectEndpointAPIVersion)
if err != nil {
return messages.ListingDatasetVersions(name, err)
}
cli/azd/extensions/azure.ai.dataset/internal/pkg/dataset_api/models.go:44
- azd-code-reviewer:
Datasetis both the service wire model and the value emitted by-o json. Because it has separate camelCase and snake_case URI fields, the CLI's JSON schema changes with the response spelling (dataUriversusdata_uri) even thoughResolvedBlobURItreats them as one value. Emit a dedicated canonical output DTO for create/update/list/show instead of serializing this compatibility model directly.
cli/azd/extensions/azure.ai.dataset/internal/messages/messages.go:357 - azd-code-reviewer: Every caller uses
requireFlagwhenever a required flag is absent, without checking whether--no-promptwas supplied. Interactive invocations such asdataset create nametherefore falsely claim they were run with--no-prompt. Either make the message unconditional (--<name> is required) or pass the actual no-prompt state and only add this qualifier when true.
cli/azd/extensions/azure.ai.dataset/internal/foundry/projectctx/resolver.go:139 - azd-code-reviewer: This adds a host read of
AZURE_AI_PROJECT_ENDPOINT, but that variable is absent from the repository's environment-variable reference, and the existingFOUNDRY_PROJECT_ENDPOINTentry says it is only used byazure.ai.agents. Updatecli/azd/docs/environment-variables.md, the required single source of truth, for both variables and this extension.
cli/azd/extensions/azure.ai.dataset/README.md:8 - azd-code-reviewer: This installation command cannot work in the PR's intended state because the registry entry is deliberately excluded; the same README confirms that at lines 54-57. Lead with the supported local-source workflow (or defer the install instruction until registry publication) so the first documented command is runnable.
```console
$ azd extension install azure.ai.dataset
$ azd ai dataset --help
</details>
azidentity gives the azd subprocess a fixed 10 second timeout and discards its stderr, so an azd that overruns surfaces as exit status 1 with no cause and the command fails. The evaluations extension wraps its credential to retry once; this one built the same credential and handed it over raw, so every dataset command carried a flake the sibling had already fixed. Ported with the structural shape that keeps it wired: newAzdTokenCredential returns the wrapper, so the raw credential is never in scope. That matters -- in the sibling an earlier version assigned the wrapper and then built its clients from the raw one, and four tests passed with the retry entirely dead. The ported tests include the one that guards exactly that. Also documents the endpoint cascade as implemented. The README listed three levels and the resolver has four: it omitted the AZURE_AI_PROJECT_ENDPOINT fallback and the machine-wide global config that azure.ai.agents writes, which outranks the host environment variable. Copilot raised this on the PR and I resolved the thread without fixing it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 66 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cli/azd/extensions/azure.ai.dataset/README.md:22
- [azd-code-reviewer] This documented invocation always fails because
deleterequires--version. Include the required flag so users can copy a working command.
| `azd ai dataset delete <name>` | Delete a dataset version |
cli/azd/extensions/azure.ai.dataset/internal/cmd/dataset.go:375
- [azd-code-reviewer] This 404 branch contradicts the command's documented idempotence: if the service returns not found,
deleteexits nonzero even though deleting an unregistered version is intended to succeed. Treat a 404 as an already-deleted success and continue to the normal output path.
if err := ec.datasetClient.DeleteDatasetVersion(
ctx, name, version, ProjectEndpointAPIVersion,
); err != nil {
if dataset_api.IsNotFound(err) {
return messages.DatasetVersionNotFound(name, version)
}
cli/azd/extensions/azure.ai.dataset/README.md:6
- [azd-code-reviewer] The quick start cannot work in this revision: the TODO below states that
azure.ai.datasethas no registry entry, so this install command cannot resolve. Either add the registry entry before presenting this command or replace the quick start with complete local pack/publish/install steps.
This issue also appears on line 22 of the same file.
$ azd extension install azure.ai.dataset
cli/azd/extensions/azure.ai.dataset/tests/cli/dataset_test.go:51
- [azd-code-reviewer] Cleanup silently returns when listing or decoding fails and ignores every delete result, so failed live tests can leave persistent datasets in the shared Foundry project while reporting no cleanup failure. Assert the listing, JSON decoding, and each delete so leaked state is visible and actionable.
Adds
azd ai datasetΓÇö a standalone extension for registering and versioning Foundry datasets.Why its own extension
A dataset is a general Foundry asset. Fine-tuning and other scenarios need one as much as evaluation does, so the CRUD group does not belong inside the evaluation extension. This extension has its own Go module, lint workflow and release pipeline, and no dependency on
azure.ai.evaluations.The line drawn with the evaluation extension: CRUD moved, generation stayed.
generatewrites thedatasets:entry into the evaluation configuration, so moving it would strand a config write across an extension boundary ΓÇö a generated dataset registered with the service and absent from configuration, whichazd upwould not reconcile.create/update/list/show/delete/versionsneed no configuration at all, which is why they could move.Surface
Notable
internal/messages/messages.go) so the whole voice of the CLI can be reviewed in one sitting. 69 messages.TYPE, notFORMAT. The API acceptsformaton upload and never returns it, so the previous column was blank for every dataset ever listed.azd-ext-azure-ai-evaluations) and debug log name (azd-ai-eval-*.log), so a dataset request could not be told from an eval one in a service log or a local one.Verified
go build,go vet -tags live,hero,go test ./...andgofmtclean. Exercised live against a Foundry project: create → 1.0, update → 2.0,versions list,show,list, and published to a local registry for a bug bash through realazd.Not included, deliberately
No
registry.jsonentry and nomicrosoft.foundrybundle dependency. Both need published artifacts; listing an unpublished extension in the bundle breaksazd extension install microsoft.foundryfor everyone. Those land after the first release.Open question for reviewers
azd ai dataset versions list <unknown-name>errors and exits 1 here, while the same command in the evaluation extension (#9500) lists nothing and exits 0. I tried making the eval side match this one and it broke two tests whose names are the argument against it ΓÇö "an unknown name lists nothing rather than failing" and "deleting an unregistered dataset is idempotent, not an error", the second because delete idempotence is verified by listing what is left.A list is a filter rather than a lookup, so empty-and-successful is defensible and
-o jsoncallers range over the array instead of branching on an error. By that reasoning this extension'sversions listis the one that should move.dataset showon an unknown name is not in question: that one is a lookup and should fail, as it does.Draft ΓÇö not requesting reviewers yet.
Fixes #9550