Skip to content

Commit 9fb8936

Browse files
authored
Add --messaging-endpoint flag; defer unset M365 endpoint; clean up setup output (#440)
* Add --messaging-endpoint flag; defer unset M365 endpoint; clean up setup output - Add optional `--messaging-endpoint <url>` to `setup all` and `setup blueprint --endpoint-only`, with an interactive prompt on `setup all --m365` when none is configured. - Treat an unset M365 messaging endpoint as deferred (a post-deploy artifact) rather than a failure: the setup summary reports success with a single Next steps pointer instead of an Action Required item. - Normalize setup output to one indentation convention (section header at column 0, content nested via Indent scopes, console prompts aligned); collapse consecutive blank lines in the console formatter. - Rework "backend configuration" jargon to "messaging endpoint" wording across registration and cleanup. - Fix the duplicate "Agent identity created" log line; demote the "no tenant ID" and "no project detected" messages to debug/info. - Add SetupContext.NonInteractive so the endpoint-prompt step defers deterministically (fixes a Visual Studio test-host hang). Tests and CHANGELOG updated. Full suite green (1649 passed). * Address Copilot review comments on PR #440 - `--messaging-endpoint`: fail fast when supplied without its precondition (`--m365` on `setup all`; `--endpoint-only` on `setup blueprint`) instead of silently ignoring it. - Distinguish "option omitted" from "explicitly passed empty" — an empty `--messaging-endpoint ""` now errors with exit 1 rather than being treated as omitted (which would prompt/defer/no-op). - `setup blueprint --endpoint-only` now captures the registration result and exits non-zero when the endpoint did not register, so scripting is reliable. - Add handler-invocation tests for the three guards. - review-staged skill: add generic Rules W (CLI option precondition enforcement), X (Option<string?> empty-vs-omitted conflation), and Y (ignored result-bearing call / missing exit code) so future reviews catch this class of issue.
1 parent 243bfec commit 9fb8936

22 files changed

Lines changed: 801 additions & 339 deletions

.claude/skills/review-staged/SKILL.md

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,27 @@ Nine checks that static per-file analysis tends to miss — each requires tracin
134134

135135
Implementation: at the start of the review, emit the list of branch-level files and treat them as the review surface. At the end, verify each file was read or explicitly justified as "no rule applies."
136136

137-
Full detection rules and real examples are in `.claude/agents/pr-code-reviewer.md` Step 9, Rules N through V.
137+
- **CLI option precondition enforcement (Rule W)**: **Added 2026-06-02 after PR #440's Copilot review.** When a new or modified `Option<...>` has a description that constrains its applicability — phrases like `(--X only)`, `use with --Y`, `Only meaningful with --Z`, `requires an existing ...` — the handler **must validate that precondition and fail fast** (`logger.LogError(...) + context.ExitCode = 1 + return`) when the option is supplied outside that context. A flag that is accepted and then silently ignored (because the code path it feeds is skipped) is a bug: the user gets exit 0 and assumes it took effect.
138+
139+
Detection: for every option whose description contains a constraint keyword, grep the handler for a guard that errors when the option is present but the precondition is false. Use `context.ParseResult.CommandResult.FindResultFor(<option>) != null` to detect "supplied" (not the value, which can't tell omitted from empty). If no such guard exists, flag it. **Severity: MEDIUM.** PR #440 example: `--messaging-endpoint` was documented `(--m365 only)` and `with --endpoint-only`, but both handlers accepted it unconditionally and dropped it when the messaging step / endpoint-only path was skipped.
140+
141+
- **`Option<string?>` empty-vs-omitted conflation (Rule X)**: **Added 2026-06-02 after PR #440.** When a string option is read as `GetValueForOption(opt)?.Trim()` and then gated only with `string.IsNullOrWhiteSpace(...)`, an **explicitly-passed empty value** (`--opt ""` or `--opt " "`) is indistinguishable from **omitted**. This is a defect whenever "omitted" triggers a *different* behavior than "empty should" — e.g. omitted falls back to config / prompts / defers, so `--opt ""` silently does the fallback instead of erroring.
142+
143+
Detection: for each `Option<string?>` whose omitted-path does something non-trivial (prompt, default, defer, config fallback), verify the handler distinguishes "specified" via `FindResultFor(opt) != null` and emits a targeted error + `ExitCode = 1` when specified-but-whitespace. This is the CLAUDE.md "Input Validation" rule applied to the explicit-empty case. **Severity: MEDIUM.** PR #440 example: `--messaging-endpoint ""` was `.Trim()`ed to `""`, read as omitted, and triggered prompting/deferral instead of a clear error.
144+
145+
- **Ignored result-bearing call / missing exit code (Rule Y)**: **Added 2026-06-02 after PR #440.** Extends the exit-code-completeness check (CLAUDE.md item 8) to *discarded return values*. When a command handler `await`s an operation that returns a success/failure-bearing result (an enum like `*Result`, a `bool`, a tuple with an outcome) and **does not capture or branch on it**, any failure that result encodes will still exit 0 — breaking scripting/CI.
146+
147+
Detection: in every command handler, find `await SomeOp(...)` calls whose return type is non-`void`/non-`Task` and whose result is not assigned or checked. For each, confirm the failure values map to `context.ExitCode = 1`. **Severity: MEDIUM** (HIGH if the command is commonly scripted). PR #440 example: the `--endpoint-only` path called `RegisterEndpointAndSyncAsync(...)` (returns `EndpointRegistrationResult`) and discarded it, so `NotConfigured` / contract-mismatch / `Failed` all exited 0.
148+
149+
- **Dead local / unused assignment the compiler won't flag (Rule Z)**: **Added 2026-06-03 after PR #440's second review.** A local that is assigned but never read is dead code. The trap is that `TreatWarningsAsErrors` does **not** catch most of these: the C# unused-variable warning (CS0219) fires only for **constant** initializers. When the initializer is a property access, method call, LINQ expression, or `new`, the compiler stays silent (it can't assume the access is side-effect-free), so the build is green and the dead local survives. This means a green build/test run — the skill's one objective signal — gives **no** indication, so the sweep must be done by reading.
150+
151+
Detection: for every local declared in the diff (and the surrounding method), confirm it is read at least once after assignment. Pay special attention to locals whose initializer is `x.Count` / `x.ToList()` / `SomeMethod(...)` / `new T(...)` — these are exactly the ones the compiler won't warn about. A common shape is a "summary" or "counter" local that was meant to be used in an output/log line but the line ended up using the individual counters instead. **Severity: LOW** (dead code, not a runtime defect). Fix by deleting the local, unless the intent was to surface it (e.g. a total in a summary line) — in which case wire it in. PR #440 example: `var totalChecks = requirementChecks.Count;` in `RequirementsSubcommand` was never read (the summary used `passedChecks`/`warningChecks`/`failedChecks`); the build stayed green because `.Count` is a property access, and the skill walked past it.
152+
153+
- **CHANGELOG entry crispness (Rule AA)**: `CHANGELOG.md` `[Unreleased]` feeds straight into the nuget.org release notes, so each entry must be one crisp sentence about the user-visible change — not an essay. Flag entries that explain internal mechanism, name classes/methods/internals, restate rationale, or run multiple sentences of background. Fix by cutting to what a package consumer needs to know. **Severity: LOW.**
154+
155+
- **Code comment crispness (Rule BB)**: Comments added in the diff must be crisp and follow .NET conventions — `///` XML doc on public members, short `//` only on non-obvious logic. Flag multi-line rationale essays, comments that restate what the code already says, and commit-message-style narration in source. A comment states *why* in one line; it does not retell the change (that belongs in the commit/PR). **Severity: LOW.**
156+
157+
Full detection rules and real examples are in `.claude/agents/pr-code-reviewer.md` Step 9, Rules N through V (plus W–BB above).
138158

139159
### Context Awareness
140160
The skill differentiates between:

CHANGELOG.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
4040
- `setup requirements` runs without `a365.config.json` — system checks (PowerShell modules, Frontier enrollment) always run; client app checks run when a config file or Azure CLI session is available.
4141
- `--agent-name` and `--tenant-id` options added to `setup blueprint`, `setup permissions` (all subcommands), `create-instance`, `publish`, and `query-entra` — all commands can now resolve configuration from Entra ID without requiring `a365.config.json`.
4242
- `setup permissions custom --resource-app-id <guid> --scopes <scopes>` — apply inline custom API permissions to the agent blueprint without editing `a365.config.json`.
43-
- `--m365` opt-in flag on `a365 setup blueprint`, `a365 cleanup blueprint`, and `a365 setup all` — when set, the CLI registers or clears the agent blueprint's messaging endpoint via the Teams Graph backend configuration endpoint on MCP Platform. Default is **off**: without `--m365`, endpoint registration is skipped and the CLI points users at the Teams Developer Portal (https://learn.microsoft.com/en-us/microsoft-agent-365/developer/create-instance#1-configure-agent-in-teams-developer-portal) for manual configuration. Intended for M365 agents; opt-in because the Teams Graph rollout on MCP Platform is ongoing.
43+
- `--m365` opt-in flag on `a365 setup all` registers the agent blueprint's messaging endpoint via Teams Graph (MCP Platform); off by default, the step is skipped and you're pointed at the Teams Developer Portal. `setup blueprint --endpoint-only` / `--update-endpoint` and `cleanup blueprint --endpoint-only` always use this path and don't need the flag.
44+
- `--messaging-endpoint <url>` on `setup all` and `setup blueprint --endpoint-only` — supply the agent's HTTPS messaging endpoint directly; interactive `setup all --m365` prompts for it when none is configured.
4445
- Messaging endpoint row added to `a365 setup all` summary output, with "registered"/"reused"/"skipped (non-M365)"/"manual config required"/"failed" states. When registration can't complete, the summary surfaces an "Action Required" entry with the Teams Developer Portal URL so the user knows exactly what to do next.
4546
- Defensive fallback when the server rejects the new request with a known contract-mismatch signature — the CLI logs `"Automated messaging endpoint registration is not available for this tenant yet. You'll need to configure it manually."` and directs the user to the Teams Developer Portal. Same user-facing path is reused when registration fails because the signed-in user is not a blueprint owner.
4647
- `--yes` / `-y` option on `cleanup blueprint`, `cleanup azure`, and `cleanup instance` — skips confirmation prompts.
@@ -98,6 +99,9 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
9899
- Agent identity creation now uses Blueprint app-only credentials (`AgentIdentity.CreateAsManager`, auto-granted to all Blueprint apps). The custom CLI app no longer requires `AgentIdentity.Create.All` or `DelegatedPermissionGrant.ReadWrite.All`. Administrators can remove these permissions from the CLI app registration. See [Custom client app registration](https://learn.microsoft.com/microsoft-agent-365/developer/custom-client-app-registration) for the updated permission list.
99100
- `setup all` now retries agent identity creation and blueprint token acquisition with exponential back-off (delay doubles up to a 60-second cap; agent identity retries up to 5 times, blueprint token up to 12 times — worst case is several minutes per call when Entra replication lag is severe) when Entra replication lag causes transient 401/AADSTS errors on fresh blueprint setups. Retry progress is logged at `Debug` level only.
100101
- `setup blueprint --m365` now prints a note when passed alone — the flag only takes effect with `--endpoint-only` or `--update-endpoint`; otherwise use `setup all --m365`.
102+
- `setup all --m365` with no messaging endpoint now reports it as deferred ("configure after you deploy") instead of a failure — the run succeeds with a Next steps pointer rather than an Action Required item.
103+
- The "no project detected" message during `setup` is now informational instead of a warning.
104+
- `setup` output reformatted for consistent indentation and clearer, jargon-free messaging.
101105
- Graph error bodies in `[DBG]` logs compressed to `{code}: {message}` instead of the full JSON envelope.
102106
- `a365.config.json` and `a365.generated.config.json` are no longer mirrored into the machine-global config folder (`%LocalAppData%\Microsoft.Agents.A365.DevTools.Cli` on Windows, `~/.config/a365` on Linux/macOS). Config is read from and written to the project directory only. Cleanup commands no longer fall back to `a365.generated.config.json` in the global folder when no project-local copy exists. MSAL token caches, CLI logs, and the version/notice caches continue to live in the global folder.
103107
- `setup all` and `setup permissions` now issue the S2S app role assignment and delegated `AllPrincipals` OAuth2 consent via `az rest` against the operator's existing `az login` session, replacing the previous `Connect-MgGraph` PowerShell fallback (issue #429). `pwsh` and the Microsoft.Graph PowerShell modules are no longer required for this path. The per-prompt `[y/N]` confirmation is unchanged.

src/Microsoft.Agents.A365.DevTools.Cli/Commands/CleanupCommand.cs

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -196,10 +196,10 @@ private static Command CreateBlueprintCleanupCommand(
196196

197197
var m365Option = new Option<bool>(
198198
new[] { "--m365" },
199-
description: "Only meaningful with --endpoint-only. When set, clears the messaging endpoint from " +
200-
"Teams Graph via MCP Platform. Default is false (opt-in). Ignored (with a warning) " +
201-
"for full blueprint cleanup, since deleting the blueprint application cascades to " +
202-
"the backend configuration on the server side.");
199+
description: "Treat this agent as an M365 agent. Optional — --endpoint-only already clears the " +
200+
"messaging endpoint via Teams Graph automatically. Ignored (with a warning) for full " +
201+
"blueprint cleanup, since deleting the blueprint application removes the messaging " +
202+
"endpoint on the server side.");
203203

204204
var dryRunOption = new Option<bool>("--dry-run", "Show what would be deleted without making any changes");
205205

@@ -224,6 +224,10 @@ private static Command CreateBlueprintCleanupCommand(
224224
var dryRun = context.ParseResult.GetValueForOption(dryRunOption);
225225
var endpointOnly = context.ParseResult.GetValueForOption(endpointOnlyOption);
226226
var isM365 = context.ParseResult.GetValueForOption(m365Option);
227+
// Endpoint clearing goes through Teams Graph (the M365 path), so infer --m365 for
228+
// --endpoint-only — users shouldn't have to pass it (mirrors 'setup blueprint').
229+
if (endpointOnly)
230+
isM365 = true;
227231
var yes = context.ParseResult.GetValueForOption(yesOption);
228232
var ct = context.GetCancellationToken();
229233

@@ -276,15 +280,9 @@ private static Command CreateBlueprintCleanupCommand(
276280
agentBlueprintService.CustomClientAppId = config.ClientAppId;
277281
}
278282

279-
// If endpoint-only mode, only delete the messaging endpoint — gated on --m365.
283+
// Endpoint-only: clear just the messaging endpoint (--m365 is inferred above).
280284
if (endpointOnly)
281285
{
282-
if (!isM365)
283-
{
284-
SetupSubcommands.BlueprintSubcommand.LogNonM365EndpointGuidance(logger, "clear");
285-
return;
286-
}
287-
288286
await ExecuteEndpointOnlyCleanupAsync(logger, config, backendConfigurator, correlationId: correlationId);
289287
return;
290288
}
@@ -294,9 +292,9 @@ private static Command CreateBlueprintCleanupCommand(
294292
if (isM365)
295293
{
296294
logger.LogWarning(
297-
"--m365 has no effect on full blueprint cleanup. The Teams Graph backend " +
298-
"configuration is removed automatically when the blueprint is deleted. " +
299-
"Use 'a365 cleanup blueprint --endpoint-only --m365' to clear the endpoint " +
295+
"--m365 has no effect on full blueprint cleanup. The messaging endpoint is " +
296+
"removed automatically when the blueprint is deleted. " +
297+
"Use 'a365 cleanup blueprint --endpoint-only' to clear the endpoint " +
300298
"while preserving the blueprint.");
301299
}
302300

@@ -529,10 +527,10 @@ private static Command CreateBlueprintCleanupCommand(
529527

530528
logger.LogInformation("Agent blueprint application deleted successfully");
531529

532-
// Teams Graph backend configuration is a child resource of the blueprint and is
533-
// removed on the server side when the blueprint is deleted. No separate clear
534-
// call is needed here. Use `a365 cleanup blueprint --endpoint-only --m365` to
535-
// clear just the backend configuration while preserving the blueprint.
530+
// The messaging endpoint is a child resource of the blueprint and is removed on the
531+
// server side when the blueprint is deleted. No separate clear call is needed here.
532+
// Use `a365 cleanup blueprint --endpoint-only` to clear just the messaging endpoint
533+
// while preserving the blueprint.
536534
PrintOrphanSummary(logger, failedResources);
537535

538536
// Clear configuration after successful blueprint deletion
@@ -1219,24 +1217,24 @@ private static async Task<bool> DeleteMessagingEndpointAsync(
12191217
{
12201218
if (string.IsNullOrWhiteSpace(config.AgentBlueprintId))
12211219
{
1222-
logger.LogError("Agent Blueprint ID not found. Agent Blueprint ID is required for clearing the backend configuration.");
1220+
logger.LogError("Agent Blueprint ID not found. It is required to remove the messaging endpoint.");
12231221
return false;
12241222
}
12251223

1226-
logger.LogInformation("Clearing backend configuration...");
1224+
logger.LogInformation("Removing messaging endpoint...");
1225+
using (logger.Indent())
1226+
{
1227+
// The service logs the "Removed successfully." / "already removed" outcome under this header.
1228+
var cleared = await backendConfigurator.ClearBackendConfigurationAsync(
1229+
config.AgentBlueprintId,
1230+
correlationId: correlationId);
12271231

1228-
var cleared = await backendConfigurator.ClearBackendConfigurationAsync(
1229-
config.AgentBlueprintId,
1230-
correlationId: correlationId);
1232+
if (cleared)
1233+
return true;
12311234

1232-
if (cleared)
1233-
{
1234-
logger.LogInformation("Backend configuration cleared successfully");
1235-
return true;
1235+
logger.LogWarning("Failed to remove the messaging endpoint.");
1236+
return false;
12361237
}
1237-
1238-
logger.LogWarning("Failed to clear backend configuration");
1239-
return false;
12401238
}
12411239

12421240
/// <summary>

0 commit comments

Comments
 (0)