Add logs export command, fix macOS secret-null bug, fix file logger Debug capture#411
Merged
Merged
Conversation
…ebug capture - `a365 logs export [command]`: new top-level command that produces a redacted copy of a CLI log file safe to share with Microsoft support. Redacts JWT tokens, email addresses, and GUIDs with consistent aliases so log correlation is preserved. Includes LogRedactionService with 15 unit tests. - Fix `agentBlueprintClientSecret: null` on macOS: `CreateBlueprintClientSecretAsync` was saving the secret via `Environment.CurrentDirectory` (getcwd, symlink-resolved) which diverged from `config.DirectoryName` (unresolved) on systems with symlinks in the project path. Fixed by threading the resolved `generatedConfigPath` explicitly through the method. Same fix applied to `AllSubcommand.SaveStateAsync`. `generatedConfigPath` is now required (throws `ArgumentNullException` if null). - Fix CLI log file not capturing `[DBG]` messages: `SetMinimumLevel` was applied globally, blocking Debug from reaching `FileLoggerProvider`. Fixed by setting global minimum to `Debug` and adding a per-provider `AddFilter<ConsoleLoggerProvider>` so the console still gates at Information (or Debug with -v). - Add SP-403 troubleshooting guide at docs/troubleshooting.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
bug
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the Agent365 CLI with a new logs export command (with redaction), and fixes two reliability issues: macOS blueprint secret persistence when symlink paths are involved, and Debug-level log capture to file.
Changes:
- Added
a365 logs export [command] [--output <dir>]to export redacted diagnostic logs. - Fixed
setup blueprint/setup allpersistence by saving blueprint client secret state to the explicit generated-config path. - Adjusted logging configuration so the file logger receives Debug messages by default while keeping console verbosity gated.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/LogRedactionServiceTests.cs | Adds unit tests for log redaction behavior and invariants. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/LogRedactionService.cs | Implements JWT/email/GUID redaction with consistent aliasing and a redaction header. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/FileLoggerProvider.cs | Updates provider commentary to reflect new global/per-provider filtering behavior. |
| src/Microsoft.Agents.A365.DevTools.Cli/Program.cs | Registers logs command + redaction service; changes logging filters so file captures Debug. |
| src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs | Adds the logs command name constant. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs | Threads generatedConfigPath into secret persistence and tightens argument validation. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs | Saves setup state to the explicit generated-config path. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/LogsCommand.cs | Implements a365 logs export command and output handling. |
| CHANGELOG.md | Documents the new command and the two fixes. |
- LogsCommand: filter out *.redacted.log files from auto-discovery,
broaden exception catch to include UnauthorizedAccessException and
SecurityException, unify structured-log key to {CliCommand} across
all three log calls in the export loop
- BlueprintSubcommand: make generatedConfigPath non-nullable required
(string instead of string?) — compile-time enforcement replaces
runtime ArgumentNullException guard
- LogRedactionService: replace em dash with plain hyphen in header
- BlueprintSubcommandTests: update 6 call sites for required parameter
- LogRedactionServiceTests: add because: clauses to security/privacy
invariant assertions
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Redacts OS usernames embedded in log file paths (/Users/<name>/, /home/<name>/, C:\Users\<name>\) with consistent <username-N> aliases. Adds UsernamesRedacted count to LogRedactionResult and the export summary header and console output. Adds 5 unit tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ajmfehr
previously approved these changes
May 8, 2026
…empty path guard - Exit code 1 when a named log is not found (was silently 0) - Validate commandName against ^[a-z0-9-]+$ before using it in file paths to prevent path traversal via user-controlled input - Redact OS path username in the 'Original:' header line using the same alias map as content, so the exported log is fully safe to share - ArgumentException.ThrowIfNullOrWhiteSpace guard on generatedConfigPath in CreateBlueprintClientSecretAsync before the try/catch block Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds four explicit standards derived from recurring Copilot review comments on PR #411: input validation before file path use, exit code completeness on every failure branch, data flow consistency for transformations, and value safety beyond non-nullable types. Added to both CLAUDE.md (for code writing) and .github/copilot-instructions.md (for PR review), so both Claude and Copilot check against the same criteria going forward. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ler tests
- Reject --output values that are explicitly empty or whitespace with a
targeted error rather than falling through to Directory.Exists("")
- Add LogsCommandTests covering all handler branches: invalid command
name, whitespace output, nonexistent output dir, missing log file
(exit code 1), successful export (exit code 0, correct file name),
and auto-discovery exclusion of *.redacted.log files (7 tests)
- Extend CLAUDE.md and copilot-instructions.md with rules for Option
whitespace validation and mandatory command handler branch coverage
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…8-secret-null-macos # Conflicts: # CHANGELOG.md
biswapm
previously approved these changes
May 11, 2026
- Setup blueprint no longer hangs ~8 min for developers on the strict
7-permission set. The SP-propagation check probes oauth2PermissionGrants,
which 403's permanently for callers lacking DelegatedPermissionGrant.Read.All.
The retry helper's `result => !result` predicate was treating the 403 as
transient and burning 12 attempts × backoff. Both sites (existing-blueprint
SP-recreate and fresh-SP-propagation) now detect 403 inside the retry
callback, set a captured flag, and short-circuit. The fresh-SP site
switched from GraphGetAsync to GraphGetWithResponseAsync so the status
code is visible to the predicate.
- Removed a false-positive "Tenant-wide admin consent may be needed for
the CLI client app" warning from ClientAppValidator. The check probes
oauth2PermissionGrants which requires DelegatedPermissionGrant.Read.All;
developers don't have that scope by design, so the GET always 403's and
the warning fired on every developer run regardless of actual consent
state. Real consent failures still surface via the operations that need
them.
- Removed a duplicate "Admin consent may not have been detected" block
in BlueprintSubcommand that printed the same admin consent URL a second
time after the role-check warning. Inner emissions are now self-sufficient
(DoesNotHaveRole path: labeled warning + URL on its own line;
browser-consent-failed path: gained the URL on its own line).
- Relabeled the DoesNotHaveRole consent warning to make the scope
explicit ("Tenant-wide admin consent is needed for the agent blueprint's
delegated scopes (per-blueprint)") so a developer can distinguish it
from any future tenant-level consent message.
- Removed 4 stale doc-comment references to a non-existent A365SetupRunner
type in BlueprintSubcommand (addresses Copilot review comment on PR #411).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…velopers
`ClientAppValidator.GetConsentedPermissionsAsync` queries
`/v1.0/oauth2PermissionGrants?$filter=clientId eq '{spId}'` to determine which
permissions the custom client app has tenant-wide consent for. That endpoint
requires `DelegatedPermissionGrant.Read.All`, which the strict 7-permission set
intentionally excludes — so non-admin developers always 403 on this query.
Before this fix the method returned an empty consented set on any null response
(including 403). `GetUnconsentedRequiredPermissionsAsync` then reported ALL 7
required permissions as unconsented and `EnsureConsentWithPromptAsync` in
`NonDwBlueprintSetupOrchestrator` prompted the developer to grant admin consent
— even when the admin had already granted it. False positive on every non-admin
`setup all` run.
The fix switches from `GraphGetAsync` (null-on-any-failure, opaque) to
`GraphGetWithResponseAsync` (status code visible). On `StatusCode == 403` the
method now returns the full `RequiredClientAppPermissions` set — presume
consented when we can't verify, rather than presume unconsented. Real consent
failures still surface from the operations that actually need the scopes.
Other failure modes (404, 5xx, network exceptions) continue to return an empty
set, preserving the existing conservative behavior for those paths.
Validated end-to-end: `setup all --agent-name SellakAdminTest --authmode s2s`
as Agent ID Developer on the strict 7-set no longer prompts. Bootstrap proceeds
straight from requirements check to blueprint creation. Unit suite: 1419 pass /
12 skipped, ~3 s.
Sibling call sites in ClientAppValidator (TryExtendConsentGrantScopesAsync,
HasPrincipalOnlyConsentGrantAsync, UpgradePrincipalGrantsToAllPrincipalsAsync)
have the same `oauth2PermissionGrants` read pattern and may have similar
regressions in code paths not yet exercised. Deferred to a follow-on audit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit (baebc95) switched `ClientAppValidator.GetConsentedPermissionsAsync` from `GraphGetAsync` to `GraphGetWithResponseAsync` so the method can detect 403 (caller lacks DelegatedPermissionGrant.Read.All) and fail-open. The test helpers `SetupConsentGrantForAgentIdentityCreate` and `SetupAdminConsentGrantsEmpty` were only mocking the old `GraphGetAsync` method on the oauth2PermissionGrants endpoint, so the new production path read a default (null) GraphResponse instead of the synthetic grants the 7 affected tests rely on for beta-permission resolution. Result: 7 ClientAppValidatorTests failed on PR #411 CI. Adds parallel `GraphGetWithResponseAsync` mocks to both helpers, returning the same JSON content wrapped in a `GraphResponse { StatusCode = 200, Json = ... }`. The existing `GraphGetAsync` mocks are kept to cover other call sites that still use the old method. Verified with fresh build (no --no-build): 1419/1419 tests pass. ClientAppValidatorTests specifically: 30/30 pass (was 23/7-fail). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gwharris7
previously approved these changes
May 11, 2026
… cleanup flags
Setup:
- setup blueprint: no longer hangs ~8 minutes when the developer cannot read
tenant-wide consent grants (was retrying a permanent 403 as transient)
- setup all (non-DW): "Grant admin consent now?" prompt skipped for non-admin
developers; admin consent URL printed for hand-off
- setup permissions mcp/bot/custom: print admin consent URL when tenant-wide
consent is missing
- setup permissions custom --resource-app-id <guid> --scopes <scopes>:
clear admin-consent guidance on 403; inline args validated before loading config
- setup blueprint --m365 alone: prints a clarifying note (the flag only takes
effect with --endpoint-only or --update-endpoint); help text updated to match
Cleanup:
- cleanup blueprint/azure/instance: accept --yes / -y for non-interactive use
- a365 cleanup --dry-run: previews resources across blueprint/azure/instance
- cleanup --agent-name <typo> no longer silently deletes via a stale local
generated config — refuses to proceed and points at clear next steps
Diagnostics:
- query-entra instance-scopes: stops claiming "admin consent has not been
granted" when oauth2PermissionGrants is unreadable; redirects to Entra portal
- Graph error bodies in [DBG] logs compressed to {code}: {message} instead of
the full JSON envelope
- logs export header "# Original:" line now redacts emails, GUIDs, and JWTs
(not just usernames)
- JsonDocument from GraphGetWithResponseAsync disposed on every path in
setup blueprint retry loop and ClientAppValidator.GetConsentedPermissionsAsync
Tests: 1419 passed / 0 failed / 12 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ValidateAdminConsentAsync previously used GraphGetAsync, which returns null for many failure modes (token acquisition, network errors, 5xx, 401, 403, etc). The "Skipping tenant-wide consent verification - caller lacks DelegatedPermissionGrant.Read.All" debug message attributed all of those to the missing-scope case - misleading and potentially hiding real failures. Switch to GraphGetWithResponseAsync so the missing-scope message only fires when the status is actually 403. Other failures log the status code and reason phrase, so real failures are diagnosable from the log. Addresses Copilot AI review comment on PR #411. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ent; isolate log tests Address three Copilot AI review comments on PR #411: 1. Surface the Graph HTTP status code and error code from oauth2PermissionGrant failures through SetupValidationException.Context (new keys: graphStatusCode, graphErrorCode). PermissionsSubcommand now branches on the structured signal ("graphStatusCode" == 403 OR graphErrorCode == "Authorization_RequestDenied") before falling back to substring matching of the exception message. Reduces the risk of misclassifying network or other failures as "admin action required." - GraphApiService.CreateOrUpdateOauth2PermissionGrantWithDetailsAsync: new overload returning (bool Success, int StatusCode, string ErrorCode). - Existing bool-returning overload retained as a thin wrapper for backward compatibility with all other callers. - SetupHelpers.EnsureResourcePermissionsAsync uses the new overload and populates the exception context with the captured status / error code. 2. Correct the FileLoggerProvider comment about log level filtering: a provider- specific AddFilter cannot lower the effective minimum below the global SetMinimumLevel. To capture Trace in the log file, the global minimum must be Trace and other providers (such as the console) should be filtered up to the desired minimum level. 3. LogsCommandTests now uses GUID-suffixed command names so the temporary log files written into the developer's real logs directory cannot collide with existing local logs or with concurrent test runs. Tests: 1419 passed / 0 failed / 12 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ajmfehr
previously approved these changes
May 11, 2026
The CLI's logs export command previously aliased every GUID in a log file to a placeholder like <id-N>. That made the redacted log nearly useless for debugging: TraceId and CorrelationId values lost their identity, and public well-known appIds (Microsoft Graph, Agent 365 resource APIs) were indistinguishable from tenant-specific service principal IDs. Preserve verbatim: - TraceId and CorrelationId values that appear after those markers. - Microsoft Graph request-id and client-request-id values inside error bodies (Microsoft support uses these to look up server-side traces). - A fixed allowlist of public Microsoft and Agent 365 resource appIds (Microsoft Graph, Messaging Bot API, Observability API, Power Platform Connectivity API, Agent 365 Tools production audience). Tenant-specific GUIDs (tenant ID, custom client appId, service principal object IDs) remain redacted through the existing alias mechanism. Tests: 7 new cases covering CorrelationId, TraceId, request-id (with the JSON quoting form), client-request-id, Microsoft Graph appId, the rare CorrelationId/tenant-ID collision case, and a mixed scenario verifying that only sensitive IDs are aliased. Full suite: 1426 passed, 0 failed, 12 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gwharris7
approved these changes
May 11, 2026
Sunil-Garg
approved these changes
May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentBlueprintClientSecret: nullon macOS (#408):CreateBlueprintClientSecretAsyncsaved the secret usingEnvironment.CurrentDirectory(getcwd, symlink-resolved), which diverged fromconfig.DirectoryName(unresolved) on systems with symlinks in the project path. Fixed by threading the resolvedgeneratedConfigPathexplicitly through the method andAllSubcommand.SaveStateAsync. The parameter now throwsArgumentNullExceptionif null to prevent silent regression.[DBG]messages:SetMinimumLevelwas applied globally, blocking Debug from ever reachingFileLoggerProvider. Fixed by setting global minimum to Debug and adding a per-providerAddFilter<ConsoleLoggerProvider>— console still gates at Information (or Debug with-v).a365 logs export [command]: produces a redacted copy of a CLI log file safe to share with Microsoft support. Redacts JWT tokens, email addresses, GUIDs, and OS path usernames with consistent aliases so log correlation is preserved. 20 unit tests included.Sample output —
a365 logs exportThe exported file begins with a redaction summary header followed by the sanitised log:
What is redacted:
aarthi-dev@agent365002.onmicrosoft.com<email-1>48e7c63c-15f8-42ff-9df9-7adb43889e34<id-1>eyJhbGci...<JWT-TOKEN>/Users/aarthisaravanakumar/.../Users/<username-1>/...Same value always maps to the same alias across all lines, so log correlation is preserved.
Test plan
dotnet test tests.proj— 1413 passed, 0 faileda365 logs export setup— producesa365.setup.redacted.logwith no raw emails, GUIDs, JWT tokens, or OS usernamesa365 logs export(no arg) — exports all available logsa365 logs export nonexistent— prints "No log found" warning, exits 1a365 logs export --output /bad/path— prints error, exits 1agentBlueprintClientSecretis non-null ina365.generated.config.json[DBG]entries appear in log file after running any command (no-vneeded)