Add pack+sideload Teams app script to activity-protocol postdeploy (#9172)#9188
Add pack+sideload Teams app script to activity-protocol postdeploy (#9172)#9188v1212 wants to merge 36 commits into
Conversation
…zure#9172) For activity-protocol agents, `azd deploy` already writes TEAMS_APP_SETUP.md. This additive change also emits a runnable pack-and-sideload script (pack-and-sideload-teams-app.ps1 and .sh) next to the agent source. The script builds the Teams app package with the bot id baked in from the deploy and installs it for the current user via `atk install --scope Personal` (no Teams admin required) in one command. It checks for and installs the atk CLI, launches `atk auth login` when needed, embeds the required icons, is idempotent (a stable per-agent Teams app id means re-runs update the same app instead of duplicating), and honors SKIP_TEAMS_INSTALL=1. The guide now points to the script as the fast path and lists its prerequisites. Purely additive: no change to existing bot provisioning or guide behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Split the two base64 icon constants into <=150-char chunks joined with '+' to satisfy the lll (line-length 220) linter; reassembled value is identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Wrap the icon base64 const block with cspell:disable/enable so the encoded PNG data does not trip the spell checker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
There was a problem hiding this comment.
Pull request overview
Adds generated Teams packaging and sideloading scripts for activity-protocol agents.
Changes:
- Generates Bash and PowerShell scripts with deterministic app IDs and embedded icons.
- Integrates scripts into postdeploy guidance.
- Adds tests, documentation, and a changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
teams_sideload_script.go |
Generates and selects sideload scripts. |
teams_sideload_script_test.go |
Tests script generation and IDs. |
listen_activity.go |
Adds scripts to postdeploy output. |
teams_pack_sideload.sh |
Bash packaging and installation flow. |
teams_pack_sideload.ps1 |
PowerShell packaging and installation flow. |
teams_app_setup_guide.md |
Documents the generated scripts. |
CHANGELOG.md |
Records the enhancement. |
- teams_pack_sideload.{sh,ps1}: move the SKIP_TEAMS_INSTALL guard to after the
package is built so it is a true build-only mode (was skipping packaging).
- Bound Teams manifest name.short (<=30) and description.short (<=80) per v1.19,
since agent names may be up to 63 chars.
- sh: make the TitleId grep tolerate no match (|| true) so the manual-sideload
fallback stays reachable under 'set -euo pipefail'.
- listen_activity.go: emit a shell-safe run hint via sideloadRunCommand (quotes
the path, pwsh call operator for .ps1) per cli/azd/AGENTS.md.
- Remove the CHANGELOG entry; per azure.ai.agents/AGENTS.md the CHANGELOG is
only updated in the dedicated version-bump release PR.
- Add tests for build-only ordering, manifest-field truncation, fallback
reachability, and shell-safe run command.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
Assemble expected values from the input path instead of a single long string literal so gosec no longer flags the test as a potential hardcoded credential. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
Address three Copilot review threads on the activity-protocol Teams
sideload generation:
- Surface the single-tenant bot's M365 tenant id in the generated guide so
the user signs in / sideloads with an account in the right tenant (the
bot is SingleTenant; an install from another tenant fails confusingly).
Threads tenantID through writeTeamsSetupGuide/teamsSetupGuideContent.
- Emit the guide's `cd <service>` hint pre-quoted per shell so paths with
spaces, an apostrophe, or backslashes stay a single literal argument:
PowerShell single-quote (''-escaped), POSIX single-quote ('\'' idiom)
with backslashes normalized to forward slashes.
- Make Teams sideload script generation all-or-nothing: writeOwnedGenerated
File now reports whether it freshly created a file, and writeTeamsSideload
Scripts rolls back files it created this run if the .ps1/.sh pair cannot
be completed (name collision or a different agent owning one slot), so a
lone script carrying a bot id its missing partner lacks is never left
behind. Pre-existing/refreshed files are untouched.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
…und 22) Address three review threads on the activity-protocol Teams sideload output: - The .ps1 run hint is printed to whatever shell launched azd, which on Windows may be cmd.exe (single quotes are not quoting there, %VAR% expands) rather than PowerShell, breaking a spaced path. Encode a full PowerShell command (`& '<path>'`) as UTF-16LE base64 and pass it via -EncodedCommand: the path rides inside the base64 payload, so it runs identically from cmd.exe, powershell.exe, and pwsh. - The manifest version-lock no longer silently falls back to an epoch-only version on timeout (two runs timing out in the same second could pick the same version and have Teams reject the later update as not newer). It now reclaims a lock ONLY when a timestamp marker proves it stale (holder crashed; a live holder finishes in milliseconds) and otherwise FAILS with cleanup guidance rather than proceeding without serialization. Applies to both teams_pack_sideload.sh and .ps1; the .ps1 also now samples the epoch after acquiring the lock, not before the wait. Verified in wsl (bash) and Windows PowerShell: normal acquire, stale-lock reclaim (monotonic counter advances, lock released), and fresh-lock hard fail (exit 1 with guidance, planted lock preserved). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
…k read (round 23)
Address four review threads on the activity-protocol Teams sideload output:
- Make the .ps1/.sh pair generation a real transaction. Snapshot each
script's prior contents before writing; if the full pair cannot be
written (a name collision, or a refresh whose partner then fails), restore
every touched file to its exact pre-call state -- remove a file we created,
restore the prior bytes of one we refreshed -- so a redeploy can never
leave one script with new ids and the other with old ids. Replaces the
earlier created-only rollback.
- writeOwnedGeneratedFile now removes the file it exclusively created if the
post-create Chmod fails, matching the write/close failure path, so a
non-executable half-pair is never stranded. Its unused `created` return is
dropped now that the caller snapshots prior state itself.
- Scope legacy-guide adoption to the agent: isLegacyGeneratedGuide now also
requires the guide's agent-specific H1 ("# Connect <agent> to Microsoft
Teams"), so two activity services sharing a source dir cannot overwrite
each other's pre-marker guide on upgrade.
- teams_pack_sideload.sh reads the lock's stale-marker with `|| true` so a
missing marker (the expected mkdir/marker-write window, or a crash there)
no longer aborts the script under `set -euo pipefail` before the wait /
cleanup-guidance path.
Verified in wsl: a no-marker lock now waits the full timeout and hard-fails
with guidance instead of aborting early.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
…d 24) Address two review threads on the activity-protocol Teams sideload output: - The guide's fast-path prose said to run from the folder containing the guide (the agent source folder), but the commands then `cd <service path>`, which from that folder resolves to src/src and fails. Reworded to start from the project root (where azd deploy runs) and cd into the source folder, so the cd targets and the stated starting directory agree (a root-level service renders `cd '.'`, a harmless no-op). - teams_pack_sideload.ps1 now wraps the `npm install -g atk` in try/catch so a nonzero npm exit -- promoted to a terminating error by the top-level $ErrorActionPreference = "Stop" together with $PSNativeCommandUseErrorActionPreference (default on PowerShell 7.4+) -- no longer stops the script before the Get-Command atk check emits the actionable manual-install message, mirroring the bash script's `|| true`. Verified in pwsh that the promoted native error is caught and the fallback check is reached. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
trangevi
left a comment
There was a problem hiding this comment.
Just marking as request changes while we come to an agreement on the script/command handling as we've been discussing offline
jongio
left a comment
There was a problem hiding this comment.
Adding some data to the bigger question @trangevi raised about whether azd should own these generated scripts, since that seems to be the real decision here.
The review history is the clearest evidence for the maintenance concern. Landing the two scripts took dozens of rounds surfacing platform-specific failure modes: GNU vs BSD base64 flags, PowerShell Restricted execution policy, $PSNativeCommandUseErrorActionPreference promoting an npm exit to fatal, cmd.exe not honoring single quotes, matching atk's exact error strings, the Teams manifest per-component version ceiling of 65535, lock-directory races, and stale-lock reclaim. Every one of those depends on an external surface azd doesn't control (the atk CLI, the Teams manifest schema, npm, per-OS shell quoting). When atk changes an error string or Teams shifts a manifest limit, the generated script breaks on a user's machine with azd named as the cause, and we own that debugging.
The remaining open items point the same direction. The pair-write transaction is in-process only, so two concurrent deploys of the same service can still interleave into a split pair, and the fix for that is yet more state and locking. The scripts also mutate the user's source tree (a persisted .teams-app-version counter, lock directories, overwriting prior generated files by marker), which is a lot of stateful behavior for a convenience wrapper.
The iterations did address the concrete bugs, so this isn't about the code as written. It's input for the offline call: whether the one-command sideload lives in azd itself, where every external-tool change becomes our support load, or in docs plus a separately versioned helper. I'd defer the merge decision to that discussion rather than unblock it here.
|
Thanks @trangevi and @jongio — fully aligned. The extension's scope is the Foundry agent lifecycle (dev/test/deploy). Anything downstream like the Teams app should at most be best-effort, fully decoupled from the extension's code, and reference-only — not something azd owns and debugs when an external surface (atk, Teams manifest, npm, shell quoting) shifts. This PR tried to take one step past the existing guide — a runnable sideload script — at the service owner's request. But as your reviews show, it's not a simple additive change. So I'll open a thread for all stakeholders to agree on where the one-command sideload belongs (azd vs. docs + a separately versioned helper), and come back to this PR once that's decided. Thanks! |
|
/azp run azure-dev - live - ext - azure.ai.agents |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
jongio
left a comment
There was a problem hiding this comment.
The offline thread has landed on a decision: @v1212 confirmed the Teams sideload should be best-effort, decoupled from the extension's code, and reference-only, not something azd owns and debugs. The current diff still does the opposite.
listen_activity.go calls writeTeamsSideloadScripts(...) in the postdeploy path, and teams_sideload_script.go plus the embedded teams_pack_sideload.ps1 / .sh assets make azd the owner of the generated scripts. That is the coupling everyone agreed to avoid, so every atk error-string or Teams manifest change becomes azd's support load, which is the maintenance concern from my earlier review.
To match the agreement, the script generation and the writer wiring should come out of the extension's postdeploy path. Keep the setup guide as the reference, and if the one-command helper survives, ship it as a separate, independently versioned artifact rather than extension-owned code.
The code itself was hardened well across the review rounds, so this isn't about bugs in the scripts. It's that the implementation hasn't caught up to the ownership decision reached in the thread. I don't think this should merge as-is until the decoupling lands. Once that's done I'm happy to look again.
Make the activity-protocol Teams pack+sideload output a genuinely lightweight, stateless addition to the existing setup guide, addressing the maintenance/ownership concerns raised in review. Scripts (teams_pack_sideload.sh/.ps1): - Remove the persisted version counter, lock directory, and stale-lock reclaim. Derive the manifest version from epoch seconds each run, so the scripts write NO state into the user's source tree. The stable Teams app id still lets `atk install` update the same installed app. Generator (teams_sideload_script.go, listen_activity.go): - Drop the ownership-marker / transactional pair-write / legacy-guide machinery. Write each script and the guide with a plain best-effort os.WriteFile, matching the existing guide-write pattern on main. Tests: - Remove the ownership/transaction tests that no longer apply; keep the content, app-id stability, preferred-script, run-command, and exec tests. Net: -570 lines. Generation stays a best-effort, additive output alongside the guide; non-activity agents are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/teams_sideload_script.go:154
os.WriteFiletruncates any existing file, so this overwrites a user's same-named script instead of preserving it as the new caller, guide, and tests promise. It also makes the collision/fallback path effectively unreachable whenever the existing file is writable. Distinguish an azd-generated script from an arbitrary existing file before replacing it (and add a collision regression test), while still allowing generated scripts to refresh their version-scoped bot ID. [azd-code-reviewer]
if err := os.WriteFile(scriptPath, []byte(content), s.mode); err != nil {
jongio
left a comment
There was a problem hiding this comment.
Reviewed the current HEAD. CI is green, the generated scripts and the end-to-end exec test look solid, and the shell-safe run hint (base64 -EncodedCommand on Windows, single-quote literals on POSIX) holds up. One thing stood out in the final slim-down commit that I think is worth a conscious decision before merge; left it inline.
|
@jongio — quick context first: in the separate group discussion, shipping a script is currently seen as the acceptable option for now, so this PR's goal is to add the script output additively, complementing the existing setup guide. This push also targets your maintenance concern directly: the scripts are now stateless — I dropped the version counter, lock mutex, and transactional/ownership machinery. They write no state into the user's tree, and both scripts and guide now use a plain best-effort Generation still lives in postdeploy, so I understand the PR stays blocked pending the full decoupling call — happy to align on that. |
Follow-up cleanup so the code matches its actual behavior after the
lightweight rewrite:
- teams_sideload_script.go: remove the unused teamsSideloadData.BotName
field (the script templates never reference {{.BotName}}).
- listen_activity.go: the scripts and guide are now (re)written with a
plain best-effort os.WriteFile, so correct the comments that still
claimed a same-named user file is "preserved, not overwritten", and
reword the next-steps note to describe a write failure rather than a
file collision.
- teams_app_setup_guide.md: the {{else}} note no longer claims a
pre-existing file was left untouched; it explains the script was simply
not written this time.
- listen_activity_test.go: update a stale comment.
No behavior change; build/test/lint/cspell all green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/teams_sideload_script.go:155
- [azd-code-reviewer]
os.WriteFileappliess.modeonly when creating a file; it preserves an existing file's permissions. A Bash script first generated on Windows, restored from source control without its executable bit, or affected by a restrictive umask is still appended towrittenand advertised as runnable via./pack-and-sideload-teams-app.sh, but that command fails with permission denied. Applyos.Chmodafter every successful write and count the script only if that succeeds.
if err := os.WriteFile(scriptPath, []byte(content), s.mode); err != nil {
log.Printf("postdeploy: failed to write Teams sideload script %q: %v", scriptPath, err)
continue
}
jongio
left a comment
There was a problem hiding this comment.
Incremental look at a27b1ac: it is comment/doc cleanup plus dropping the unused BotName field from teamsSideloadData. I confirmed that field is not referenced by either pack-sideload template (only the separate setup-guide template uses .BotName, through a different struct), so the removal is safe, and the extension builds and its internal/cmd tests pass. No new code issues in this delta.
The remaining open item is the ownership question from the earlier thread, not the code.
jongio
left a comment
There was a problem hiding this comment.
Left one note inline on the PowerShell sign-in path: a nonzero exit from atk auth login m365 can terminate the script on pwsh 7.4+ before the manual-fallback guidance runs, whereas the bash script tolerates it under set +e.
On PowerShell 7.4+ $PSNativeCommandUseErrorActionPreference defaults to true, so with the top-level $ErrorActionPreference = "Stop" a nonzero exit from `atk auth login m365` (e.g. the user cancels the sign-in) became a terminating error that killed the script before the retry probe and the graceful "Could not confirm the per-user install" fallback below. Wrap the login in non-terminating error handling so a cancelled/failed sign-in falls through, mirroring the bash script's tolerant set +e block and the existing npm-install/install-probe guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b480c5f-6c14-4d28-a19a-634f28621671
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/teams_sideload_script.go:152
os.WriteFiletreats any existing path as azd-owned: it truncates a user-authored script with the same name, and it preserves that file's old mode, so an existing0644shell script remains non-executable even though the guide invokes it with./.... Check for the generated marker before overwriting; skip and log unowned files, and explicitlyChmodowned files after writing. Add coverage for both collision and pre-existing-mode cases. [azd-code-reviewer]
if err := os.WriteFile(scriptPath, []byte(content), s.mode); err != nil {
jongio
left a comment
There was a problem hiding this comment.
The new commit wraps atk auth login m365 in try/catch/finally, sets $ErrorActionPreference to Continue for the native call, and restores it to Stop in the finally block. That resolves the sign-in concern I raised: a cancelled or failed login now falls through to the same graceful probe fallback instead of terminating the script under the top-level Stop, and it lines up with how the npm install and Invoke-AtkInstallProbe already handle native exits. The retry probe right below it is guarded the same way, so the flow stays tolerant end to end. No new code findings on this incremental change.
The remaining blocker isn't a code issue. Generation still runs in postdeploy, so the decoupling/ownership question @trangevi raised is what's still outstanding, and that's being settled offline. I'm not clearing that here.
|
Hi @therealjohn @trangevi @jongio Following up on the "azd supporting Teams app" discussion, and asking for a re-review of this PR, thanks.
|
jongio
left a comment
There was a problem hiding this comment.
I went through the current revision. The two things I'd flagged earlier are handled: the unconditional (re)write is intentional and now consistent with how TEAMS_APP_SETUP.md is already written into the same folder, and the atk auth login m365 call is wrapped so a cancelled or failed sign-in falls through to the retry and the graceful fallback instead of killing the script under $PSNativeCommandUseErrorActionPreference on PS 7.4+. Build and the internal/cmd tests (including the end-to-end packaging test) pass locally on 718c633.
No code concerns left from me on the implementation.
The open item is still the one @trangevi raised: whether azd should own and maintain the pack-and-sideload path at all, or whether this stays a reference-only pointer to a Teams-owned aka.ms doc so azd isn't on the hook to debug it when atk, the manifest schema, or npm shift underneath. That's still where I land too, and it's the gate here, not the code. I'd hold for @therealjohn on that ownership call before this merges.
Summary
Purely additive enhancement (issue #9172). For activity-protocol agents,
azd deployalready writesTEAMS_APP_SETUP.mdnext to the agent source. This PR also emits a runnable pack-and-sideload script (pack-and-sideload-teams-app.ps1andpack-and-sideload-teams-app.sh) so the user can go from a fresh deploy to a Teams-installed app with one command, instead of following the manual guide steps. The guide still ships and now points to the script as the fast path.What the script does
atk install --scope Personal— no Teams admin / org catalog approval needed.atkCLI vianpmif missing; launchesatk auth login m365and retries the install when not signed in.msaAppId) means re-runs update the same app instead of duplicating. The manifest version is derived from the current epoch each run (strictly increasing) — no state file, lock, or counter is left in the user's tree.atk auth login/ install does not abort the script — it falls through to a graceful "could not confirm the per-user install" fallback with manual sideload steps. The.ps1and.shhandle this symmetrically (PowerShell non-terminating error handling mirrors bash'sset +eblock).SKIP_TEAMS_INSTALL=1to only build the package.Additive / no behavior change
writeTeamsSideloadScriptswrites the two scripts best-effort with plainos.WriteFile, the exact same way this extension already writesTEAMS_APP_SETUP.mdinto the same folder (re-written on every deploy with the current ids). The script filenames are azd-specific.listen_activity.gois touched only to call the new writer and pass the script path / ascriptsGeneratedflag into the existingprintTeamsNextStepspointer and the guide template. The guide falls back to the manual steps when a script write fails.Testing
go build ./...,go vet ./...,go test ./internal/cmd/...,gofmt,golangci-lint, andcspellall green.teams_sideload_exec_test.go) that runs the generated script to build and validate a real zip (manifestbots[].botId == msaAppID, valid icon dimensions, zip entries at package root).End-to-end test (this PR)
Ran a genuine deploy → generated-script → Teams reply on a live activity echo agent, using the extension built from this branch so the emitted script is exactly the PR's:
azure.ai.agentsextension from this branch.azd deployof an activity echo agent (echosl071701, reusing an existing Foundry project + Azure Bot). Postdeploy emittedpack-and-sideload-teams-app.ps1/.sh+TEAMS_APP_SETUP.mdnext to the source, with the real bot id and the deterministic Teams app id baked in (no leftover placeholders).pack-and-sideload-teams-app.ps1as-is (pwsh -NoProfile -File ...): it built the package and installed it for the current user viaatk install --scope Personal, returned aTitleId, and exited0— no edits needed.hi→Echo: hi).Result: the postdeploy-generated script is directly runnable and completes the last-mile Teams sideload with no manual steps.
Closes #9172