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