Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,57 @@
4. If the assertion is on a security-sensitive, protocol-level, or external-API contract (OAuth URLs, HTTP headers, encoding format): flag as **CRITICAL** — require explicit documented justification.
- **Example of the failure mode** (from project history): Consent URL tests asserted `redirect_uri=` was present. When URL encoding was changed, tests were updated to match. No one asked whether `redirect_uri` was still required by the AAD protocol. The regression (`AADSTS500113`) reached the user before any test caught it.

### Rule 3: Verify Copyright Headers
### Rule 3: Input Validation on User-Controlled Values
- **Description**: Any CLI argument, config value, or external string that reaches a file system operation (path construction, file name interpolation, `File.ReadAllText`, `Directory.EnumerateFiles`) must be validated before use.
- **Action**: Flag as **HIGH** if:
- A user-supplied value is passed directly to `Path.Combine`, `File.Exists`, or similar without an allowlist check
- A string parameter that must be non-empty relies only on a non-nullable type — `ArgumentException.ThrowIfNullOrWhiteSpace` must also be present before the first use of the value inside a try/catch block
- **Pattern to require**:
```csharp
// Validate CLI argument before use in file path
if (!Regex.IsMatch(commandName, @"^[a-z0-9-]+$"))
{
logger.LogError("Invalid command name '{Name}'.", commandName);
context.ExitCode = 1;
return;
}

// Guard non-nullable string parameters against empty/whitespace before try/catch
ArgumentException.ThrowIfNullOrWhiteSpace(generatedConfigPath);
```

### Rule 4: CLI Command Exit Code Completeness
- **Description**: Every failure branch in a `SetHandler` command handler must set `context.ExitCode = 1` before exiting. This includes `continue` statements inside loops and early `return` statements, not just the final else block.
- **Action**: Flag as **HIGH** if:
- A `logger.LogWarning` or `logger.LogError` is followed by `continue` or `return` without setting `context.ExitCode = 1`
- A post-loop summary block sets exit code only for one case (e.g. auto-discovery) but not another (e.g. named command with missing file)
- **Pattern to require**:
```csharp
// Every failure exit must set ExitCode
logger.LogWarning("No log found for '{Name}'.", name);
context.ExitCode = 1; // required before continue or return
continue;
```

### Rule 5: Data Flow Consistency for Transformations
- **Description**: When a transformation (redaction, sanitization, encoding) is applied to a value, it must be applied everywhere that value is written — including header lines, summary output, and file names, not just the primary content block.
- **Action**: Flag as **HIGH** if:
- A value is sanitized/redacted in one output path but written verbatim in another (e.g. redacted in log content but included raw in a file header)
- A new sanitization step is added to content processing but the same value also flows into a header, summary line, or secondary output that was not updated

### Rule 6: CLI Option Whitespace Validation
- **Description**: `Option<string?>` values are null when omitted but can be explicitly passed as empty or whitespace. Code that uses `?? defaultValue` to handle null will pass empty/whitespace through to file system calls, producing confusing errors like "Output directory does not exist: {Dir}" with a blank Dir.
- **Action**: Flag as **HIGH** if an `Option<string?>` value is used in a file system call without first checking `string.IsNullOrWhiteSpace`. A targeted error message must be emitted when the option is explicitly provided as empty or whitespace.

### Rule 7: Command Handler Branch Coverage
- **Description**: Every `SetHandler` implementation defines a CLI contract (valid/invalid inputs, exit codes, output files). Without invocation tests, refactors silently break this contract.
- **Action**: Flag as **MEDIUM** if a new `SetHandler` block has no corresponding `System.CommandLine` invocation tests. The following branches must be covered:
- Invalid input argument → exit code 1
- Nonexistent or whitespace output path → exit code 1
- Resource not found (missing file, etc.) → exit code 1
- Successful path → exit code 0 and expected side effect (file written, correct name)

### Rule 8: Verify Copyright Headers
- **Description**: Ensure all C# files have proper Microsoft copyright headers
- **Action**: If a `.cs` file is missing a copyright header:
- Add the Microsoft copyright header at the top of the file
Expand All @@ -144,7 +194,10 @@
### Implementation Guidelines

#### When Reviewing Code:
1. **Kairo Check**:
1. **Input validation** (Rule 3): For every user-supplied value used in a file path or file name, confirm an allowlist pattern check and/or `ThrowIfNullOrWhiteSpace` guard is present before use.
2. **Exit code completeness** (Rule 4): Scan every `continue` and early `return` in command handlers — each one on a failure path must be preceded by `context.ExitCode = 1`.
3. **Data flow consistency** (Rule 5): When redaction or sanitization is added, check header lines and summary output for the same raw value.
4. **Kairo Check** (Rule 1):
- Search for case-insensitive matches of "Kairo"
- Review context to determine if it's:
- A class name
Expand Down
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
**Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output.

### Added
- `logs export [command] [--output <dir>]` — exports a redacted copy of a CLI diagnostic log safe to share with Microsoft support. Redacts JWT tokens, email addresses, OS-path usernames, and tenant-specific GUIDs; replaces identical values with consistent aliases so log correlation is preserved. Preserves diagnostic IDs that aren't sensitive but are useful for debugging — `TraceId`, `CorrelationId`, Microsoft Graph `request-id` and `client-request-id` values, and well-known public Microsoft / Agent 365 resource appIds (such as the Microsoft Graph appId `00000003-0000-0000-c000-000000000000`). Omit `[command]` to export all available logs at once.
- `setup blueprint --show-secret` — displays the blueprint client secret stored in `a365.generated.config.json` in plaintext without re-running any setup steps. On Windows, decryption requires the same machine and user account that ran setup (DPAPI). When no secret is found, the command prints instructions to run `a365 setup blueprint --agent-name <name>`.
- Blueprint client secret is now printed to the terminal at creation time with a "copy this value now" warning. Use `a365 setup blueprint --show-secret` to retrieve it afterwards.
- Version check: stable-channel users now see an informational notice when a newer preview release exists above the current stable version, without triggering the update-required banner.
Expand All @@ -40,12 +41,18 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
- `--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.
- 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.
- 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.
- `--yes` / `-y` option on `cleanup blueprint`, `cleanup azure`, and `cleanup instance` — skips confirmation prompts.
- `--dry-run` option on top-level `a365 cleanup` — previews resources that would be deleted across blueprint, Azure, and instance steps without making changes.

### Changed
- 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.
- `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.
- `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`.
- Graph error bodies in `[DBG]` logs compressed to `{code}: {message}` instead of the full JSON envelope.

### Fixed
- `setup blueprint` / `setup all` on macOS: `agentBlueprintClientSecret` was written to the wrong file when symlinks were present in the working directory path. `Environment.CurrentDirectory` (getcwd, symlink-resolved) diverged from `config.DirectoryName` (unresolved), causing the secret save to target a different file than `LoadAsync` reads back — resulting in `agentBlueprintClientSecret: null` on subsequent runs. Fixed by threading the resolved `generatedConfigPath` explicitly through `CreateBlueprintClientSecretAsync`.
- CLI log file now captures `[DBG]` messages by default. Previously `SetMinimumLevel` was applied globally, preventing Debug-level messages from reaching the file logger even though `FileLoggerProvider` was configured to accept Trace and above.
- `setup all` with `--authmode obo --aiteammate` no longer exits with an error. `obo` is the default for AI Teammate agents and is accepted with a warning; `--authmode s2s` and `--authmode both` remain incompatible with `--aiteammate`.
- `setup all` summary now shows **Messaging endpoint — not configured** (with a link to the Teams Developer Portal to register the endpoint manually) instead of **failed — see Action Required** when the messaging endpoint URL is absent from config. Real failures (bad URL, contract mismatch) continue to show the error path.
- `setup blueprint` no longer silently loses the blueprint client secret on re-runs when `agentBlueprintClientSecret` was previously `null` in `a365.generated.config.json`. Null dynamic properties are now omitted from the generated config file instead of being written as explicit nulls (fixes macOS secret-loss regression introduced in the issue #408 fix).
Expand All @@ -58,6 +65,14 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
- `setup permissions bot` now returns a non-zero exit code when an S2S app role assignment fails, so callers and scripts can detect the failure.
- `setup all --agent-registration-only` reliability fixes: stored IDs are now correctly read in bootstrap (`--agent-name`) mode; falls back to a Graph API lookup when `agenticAppId` is missing; skips identity, permission, and project-settings steps that don't apply.
- `setup permissions bot` help text and final "Next step" log no longer suggest the non-existent `a365 deploy` command; both now point at `a365 publish` (the actual next command in the workflow).
- `setup permissions mcp/bot/custom` now print the admin consent URL when tenant-wide consent is missing.
- `setup permissions custom --resource-app-id <guid> --scopes <scopes>` exits 1 with admin-consent guidance on 403 instead of logging the raw Graph error and exiting silently.
- `query-entra instance-scopes` no longer claims "admin consent has not been granted" when `oauth2PermissionGrants` is unreadable by the caller; redirects to the Entra portal instead.
- "Grant admin consent now?" prompt in `setup all` (non-DW) is skipped for non-admin developers; admin consent URL is printed for hand-off.
- `JsonDocument` returned by `GraphGetWithResponseAsync` is now disposed on every path in `setup blueprint` SP-propagation retry and `ClientAppValidator.GetConsentedPermissionsAsync`.
- `logs export` header `# Original:` line now redacts emails, GUIDs, and JWT tokens (not just usernames).
- `cleanup --agent-name <typo>` no longer silently deletes via a stale local generated config; the CLI errors with clear guidance when the Entra-resolved blueprint doesn't match the local `a365.generated.config.json`.
- `setup permissions custom --resource-app-id <guid> --scopes <scopes>` now validates the inline arguments before loading config — a bad GUID or empty scopes produces a precise error instead of the confusing "Agent name required" from the resolver.

### Removed
- `a365 config` command family (`config init`, `config display`, `config permissions`) — replaced by `a365 setup all --agent-name` and `a365 setup permissions custom`.
Expand Down
49 changes: 49 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,50 @@ src/Microsoft.Agents.A365.DevTools.Cli/
- All `IDisposable` objects must be disposed (especially `HttpResponseMessage`)
- Cross-platform compatibility required (Windows, macOS, Linux)

### Input Validation
User-controlled input that reaches file system operations must be validated before use. This applies to CLI arguments, config values read from disk, and any value whose origin is outside this process.

- Validate against an explicit allowlist pattern (e.g. `^[a-z0-9-]+$`) before interpolating into file names or paths
- Reject inputs containing path separators, `..` segments, or characters outside the allowed set
- Apply `ArgumentException.ThrowIfNullOrWhiteSpace` for string parameters where an empty or whitespace value would cause a silent failure — a non-nullable type alone is insufficient
- CLI `Option<string?>` values default to null when omitted but can be explicitly passed as empty or whitespace by the user — check `IsNullOrWhiteSpace` on option values before use, and emit a targeted error rather than falling through to a confusing downstream failure (e.g. `Directory.Exists("")`)

```csharp
// Correct: validate user input before using it in a path
if (!Regex.IsMatch(commandName, @"^[a-z0-9-]+$"))
{
logger.LogError("Invalid command name '{Name}'. Use lowercase letters, digits, and hyphens only.", commandName);
context.ExitCode = 1;
return;
}

// Correct: guard non-nullable string parameters against empty/whitespace
ArgumentException.ThrowIfNullOrWhiteSpace(generatedConfigPath);
```

### CLI Command Exit Codes
Every failure path in a command handler must set `context.ExitCode = 1` before returning or continuing. This includes `continue` statements inside loops, not just top-level `return` statements.

- Trace every branch — `if (!condition) { log; continue; }` is a failure path and needs `ExitCode = 1`
- Verify the post-loop summary block covers all zero-export cases, not just the auto-discovery case

```csharp
// Wrong: warning logged but ExitCode left at 0
logger.LogWarning("No log found for '{Name}'.", name);
continue;

// Correct: set exit code before every failure exit
logger.LogWarning("No log found for '{Name}'.", name);
context.ExitCode = 1;
continue;
```

### Data Flow Consistency
When a transformation (redaction, sanitization, encoding) is applied to a value, identify every place that same value is written and apply the transformation consistently. Header lines, log output, and file names are common places where the same data appears a second time without the transformation.

- After writing redaction or sanitization logic, ask: "where else does this raw value appear in the output?"
- The source file path, for example, may appear both in log content and in a header — both need the same redaction pass

### Error Handling
- Use centralized error codes from `Constants/ErrorCodes.cs`
- Use centralized messages from `Constants/ErrorMessages.cs`
Expand Down Expand Up @@ -145,3 +189,8 @@ Central NuGet package management in `src/Directory.Packages.props`. Key dependen
3. Ensure SOLID principles are followed
4. Resource disposal for all IDisposable objects
5. Cross-platform compatibility
6. **Input validation:** Any CLI argument or external value used in a file path or file name is validated against an allowlist pattern before use
7. **Value safety:** String parameters that must be non-empty use `ArgumentException.ThrowIfNullOrWhiteSpace`, not just a non-nullable type
8. **Exit code completeness:** Every failure branch in a command handler (including `continue` inside loops) sets `context.ExitCode = 1`
9. **Data flow consistency:** Any transformation applied to a value (redaction, sanitization) is applied everywhere that value is written — including headers, summaries, and log lines, not just the primary content
10. **Command handler branch tests:** Every new `SetHandler` command handler must have invocation tests covering: invalid input → exit code 1, missing resource → exit code 1, bad output path → exit code 1, and successful path → exit code 0 with expected side effect (file written, message logged)
Loading
Loading