docs(agents): add infra layer design spec - #9363
Conversation
|
Azure Pipelines: 22 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.
🟡 Not ready to approve
The proposed deterministic resource-group name risks destructive collisions, and the environment-key migration and atomicity guarantees need clarification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Defines a layered IaC design for safely ejecting Foundry infrastructure into projects with existing infrastructure.
Changes:
- Specifies project-shape behavior and isolated Foundry layer ownership.
- Defines safety, migration, validation, and rollout requirements.
File summaries
| File | Description |
|---|---|
docs/specs/agent-init-infra-layers/spec.md |
Adds the infrastructure-layer design specification. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| ```text | ||
| rg-${AZURE_ENV_NAME}-foundry | ||
| ``` |
| - Files are generated in a temporary directory, installed atomically, and | ||
| removed if the `azure.yaml` update fails. |
| It publishes `AZURE_FOUNDRY_RESOURCE_GROUP` rather than replacing the existing | ||
| application's `AZURE_RESOURCE_GROUP`. This keeps targeted `azd down foundry` |
There was a problem hiding this comment.
🟡 Not ready to approve
The design has unresolved custom-provider migration, resource-group contract, and atomic-install concerns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (3)
docs/specs/agent-init-infra-layers/spec.md:113
- The new key needs an input and consumer migration contract, not only an output rename. Current eject parameters feed
${AZURE_RESOURCE_GROUP}to Terraform (init_infra.go:601-605), generated Bicep/Terraform emit that key, and activity-agent bot creation and cleanup read it (listen_activity.go:103,257). In a composed project that value belongs to the sibling application, so provisioning or bot operations can target the wrong resource group unless generated inputs and all consumers useAZURE_FOUNDRY_RESOURCE_GROUPwith a legacy fallback for root projects. Add those changes and coverage to the design.
It publishes `AZURE_FOUNDRY_RESOURCE_GROUP` rather than replacing the existing
application's `AZURE_RESOURCE_GROUP`. This keeps targeted `azd down foundry`
from deleting sibling-layer resources.
docs/specs/agent-init-infra-layers/spec.md:56
- Preserving this shape requires more than per-layer
provider. Layers currently require a non-emptypath(cli/azd/pkg/infra/provisioning/provider.go:206-208; both schemas also require it) and reject undeclared fields, while custom providers receive provider-specificconfig(cli/azd/internal/grpcserver/external_provisioning_provider.go:353-362). A root custom/fileless provider withinfra.configtherefore cannot be moved into a valid layer without inventing a path or dropping configuration. Define the synthetic path and add per-layerconfigschema support, or make these projects a refusal case.
| Existing custom/fileless provider | Preserve it as a layer and add `foundry` |
docs/specs/agent-init-infra-layers/spec.md:127
- This all-or-nothing guarantee conflicts with the allowed non-empty
infra/foundrytarget on line 60. A staged directory can be atomically renamed only when azd owns the whole destination; merging several files into an existing user directory requires multiple operations, so interruption can leave partial output and rollback must distinguish preexisting files. Either restrict reusable targets to absent/empty directories and rename once, or define per-file atomicity plus a recovery/rollback manifest rather than promising atomic installation of the entire set.
- Files are generated in a temporary directory, installed atomically, and
removed if the `azure.yaml` update fails.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The design permits unintended Terraform merging and does not define how existing consumers safely adopt the new resource-group key.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
docs/specs/agent-init-infra-layers/spec.md:112
- azd-code-reviewer: The new environment key needs a compatibility and consumer contract. The
microsoft.foundryprovider currently initializes and destroys fromAZURE_RESOURCE_GROUP(foundry_provisioning_provider.go:458,1324), and agent flows such aslisten_activity.go:103,257also read that key. In a composed project they would target the application's sibling resource group instead of Foundry's. Specify that Foundry lifecycle code and consumers preferAZURE_FOUNDRY_RESOURCE_GROUPwith anAZURE_RESOURCE_GROUPfallback for root-layout projects, including how the value exists before first provision and which key targeted teardown invalidates.
It publishes `AZURE_FOUNDRY_RESOURCE_GROUP` rather than replacing the existing
application's `AZURE_RESOURCE_GROUP`. This keeps targeted `azd down foundry`
from deleting sibling-layer resources.
docs/specs/agent-init-infra-layers/spec.md:60
- azd-code-reviewer: Terraform loads every
.tffile in a directory as one module, so adding generated files to a non-empty, unownedinfra/foundrydirectory performs the semantic merge rejected by this design even when filenames do not collide. Refuse a non-empty target (or at minimum any existing Terraform configuration) unless it is already the declared compatible layer.
| Existing `infra/foundry` directory without a layer | Add only non-conflicting files; refuse generated-file collisions |
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two checks are red and both block merge.
cspell-lint fails on fileless (line 56). CI runs cspell lint '**/*' --config ./.vscode/cspell.misc.yaml over docs/**, and that word isn't in the global dictionary, has no override for this path, and doesn't appear anywhere else in the repo. Fix is in the inline comment.
PR Governance fails with "PR must be linked to a GitHub issue." .github/scripts/pr-governance-issue-check.js only accepts a closing keyword (Fixes #123, Closes, Resolves) or a sidebar development link. "Design for #9126" and the Source issue link in the body don't match either form. Since this spec on its own doesn't close #9126, the sidebar link is probably the better choice over Fixes.
The rest are design questions, inline.
| |---|---| | ||
| | New or Foundry-only project | Generate the existing root layout under `infra/` | | ||
| | Existing single-layer Bicep or Terraform project | Preserve it as a layer and add `foundry` at `infra/foundry` | | ||
| | Existing custom/fileless provider | Preserve it as a layer and add `foundry` | |
There was a problem hiding this comment.
fileless is what's breaking cspell-lint on this PR. It isn't in .vscode/cspell.global.yaml, there's no override covering docs/specs/agent-init-infra-layers/, and the word doesn't appear anywhere else in the repo.
The repo pattern for one-off terms is a file-scoped override in .vscode/cspell.misc.yaml:
- filename: ./docs/specs/agent-init-infra-layers/**
words:
- filelessAlternatively reword the row, if there's already an established term for a provider that ships no local templates.
| path: infra/foundry | ||
| provider: microsoft.foundry | ||
| dependsOn: | ||
| - infra |
There was a problem hiding this comment.
Why does foundry depend on infra? Per the section right below, the Foundry layer owns an isolated resource group and publishes its own env var instead of reading the app's. If it consumes nothing from infra, this edge only serializes two layers that could otherwise provision in parallel.
The dependsOn description in schemas/alpha/azure.yaml.json scopes it to hook-mediated edges the static analyzer can't infer from .bicepparam / .parameters.json. It also doesn't help teardown ordering, since cli/azd/cmd/down.go reverses the declared layer array rather than walking the graph.
Worth either dropping it from the example or naming the hidden edge, because this snippet is what people will copy.
| ``` | ||
|
|
||
| The existing `infra/main.bicep` remains unchanged. Foundry files are generated | ||
| under `infra/foundry/`. |
There was a problem hiding this comment.
The motivating scenario is "add a Foundry agent to an existing web application or API," but the design stops at file layout and resource-group ownership. It never says how the existing infra layer or the app's services get the Foundry outputs (project endpoint, connection info) once Foundry sits in its own layer with its own resource group.
That's the part the user actually cares about after eject. If the wiring is expected to be manual, it belongs in Out of scope. If azd is supposed to plumb it, it changes both the safety rules and the dependsOn direction above.
|
Two new things beyond what's already open here.
The "Before" shape only has
Layer items are Worth saying explicitly that migration moves root My earlier comments on the |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
One new thing since my last pass, on the root-to-layers migration.
| infra: | ||
| layers: | ||
| - name: infra | ||
| path: infra |
There was a problem hiding this comment.
The migration example only covers a project using the defaults, so it doesn't say what happens to infra.path and infra.module.
Both schemas hard-forbid those keys once layers is non-empty. From schemas/v1.0/azure.yaml.json:
"allOf": [{
"if": { "required": ["layers"], "properties": { "layers": { "minItems": 1 } } },
"then": { "properties": { "path": false, "module": false } }
}]schemas/alpha/azure.yaml.json adds "deploymentStacks": false to the same then.
So for a project like this:
infra:
provider: bicep
path: iac
module: deploythe migration can't leave those at the root, it has to move them into the preserved layer. This example hardcodes path: infra, which is the value an implementer will copy, and the layer module defaults to main. Follow the example literally and that project's templates at iac/deploy.bicep stop being found, with a valid-looking azure.yaml and no error.
The already-layered path says "preserve existing layer bodies", but the root-to-layers path never states the equivalent. Worth saying explicitly that path, module, and deploymentStacks carry into the preserved layer, and adding a Safety Rule for it, since none of the eight listed cover field carryover.
There was a problem hiding this comment.
🟡 Not ready to approve
Resource ownership and cross-layer output collisions remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
docs/specs/agent-init-infra-layers/spec.md:114
- [azd-code-reviewer] Publishing
AZURE_FOUNDRY_RESOURCE_GROUPis not sufficient to establish isolated ownership. Themicrosoft.foundryprovider currently reads and destroysAZURE_RESOURCE_GROUP; in a composed project that value belongs to the application, soazd down foundrycould delete the sibling resource group. Define the layered Bicep/Terraform parameter, provider initialize/destroy, and invalidation contract around the dedicated key while preserving root-project compatibility.
It publishes `AZURE_FOUNDRY_RESOURCE_GROUP` rather than replacing the existing
application's `AZURE_RESOURCE_GROUP`. This keeps targeted `azd down foundry`
from deleting sibling-layer resources.
docs/specs/agent-init-infra-layers/spec.md:128
- [azd-code-reviewer] This atomic-install guarantee conflicts with the supported pre-existing
infra/foundrydirectory case: a staged directory cannot be atomically renamed into a non-empty destination while preserving unrelated files. Specify exclusive per-file creation plus a rollback journal, or reject any pre-existing target directory, so a mid-install failure cannot leave a partial layer.
- Files are generated in a temporary directory, installed atomically, and
removed if the `azure.yaml` update fails.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| The Foundry layer is independent by default. Eject does not add `dependsOn`; | ||
| projects can declare a dependency explicitly when their Foundry template | ||
| actually consumes another layer's outputs. |
| The Foundry layer is independent by default. Eject does not add `dependsOn`; | ||
| projects can declare a dependency explicitly when their Foundry template | ||
| actually consumes another layer's outputs. |
There was a problem hiding this comment.
"Independent by default" is doing more work here than it reads, because azd's layer dependency analyzer doesn't see this layer at all.
isBicepLayer (cli/azd/pkg/infra/provisioning/bicep/layer_deps.go:24-27) returns true only for provider: bicep or an unset provider. microsoft.foundry is an extension provider, so the Foundry layer is skipped in both phases of AnalyzeLayerDependencies:
- Phase 1 (outputs): its outputs never land in
outputProviders. - Phase 2 (inputs): its
.bicepparam/.parameters.jsonare never scanned, and thehasUnknown"depend on all earlier layers" fallback never fires for it.
The paragraph covers the direction where the Foundry template consumes another layer. The reverse direction is the one the Problem section actually motivates: an existing app layer that wants the Foundry project endpoint. That layer is bicep so it does get scanned, but a ${AZURE_AI_PROJECT_ENDPOINT} reference resolves to "no in-graph producer, externally supplied", no edge is added, and both layers fan out in parallel. On first provision the app layer reads an empty value.
mergeLayerOutputsLocked only logs a warning when two layers write the same key, so nothing fails loudly either.
Worth stating outright that no dependency is inferred in either direction for a microsoft.foundry layer, and that a layer consuming Foundry outputs has to declare dependsOn: [foundry] itself.
Summary
Design for #9126. Defines how
azd ai agent init --infracan add editable Foundry IaC to projects with existing infrastructure without merging or overwriting user-owned files.Changes
agent-init-infra-layers spec: documents project-shape behavior, isolated Foundry layer ownership, safety rules, scope, decisions, and rollout validation.Manual Validation