Fix blueprint secret gap, improve --show-secret and setup all reliability (#407)

* Add --show-secret to setup blueprint and display secret at creation time (fixes #406)

- Print the blueprint client secret to the terminal at creation time with a
  'copy this now' warning so customers are not left without the value
- Add 'setup blueprint --show-secret': reads a365.generated.config.json,
  decrypts via SecretProtectionHelper, and prints the plaintext; exits 1 with
  instructions if no secret is found
- On Windows, --show-secret requires the same machine and user account that
  ran setup (DPAPI constraint); plaintext on Linux/macOS

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Improve --show-secret UX and add ownership checks/tests

- Clarify --show-secret help text and error guidance
- Refactor config loading and error handling for --show-secret
- Remove DPAPI info log; simplify user-facing output
- Add ownership check before client secret creation with warnings
- Refine post-creation instructions for secret retrieval
- Skip clientAppId validation for --show-secret (offline support)
- Lower authentication log level to Debug
- Add unit tests for ownership and --show-secret scenarios
- Extend test infrastructure for file and mock handling

* Address Copilot review comments on PR #407

- Fix [displayed to terminal] appearing in console output (LogDebug instead of LogInformation)
- Change IsApplicationOwnerAsync to return bool? so Graph/token errors return null (indeterminate) instead of false, eliminating false-positive ownership warnings
- Update callers: owner-assignment fallback uses == true; pre-creation warning uses == false
- Update tests: error-path assertions now expect null; mock setups use Task.FromResult<bool?>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: sellakumaran

.claude/agents/pr-code-reviewer.md

@@ -207,6 +207,31 @@ For each changed file, analyze:
    - Run `Grep` for the old string in `src/Tests/**/*.cs`
    - If test files assert `Contains("old string")` and the old string is gone, the test will silently pass with the wrong message — flag HIGH
 
+   **D. `catch (SomeException ex)` sets `context.ExitCode = <literal>` → verify literal matches exception class**
+   When the diff adds or modifies a catch block that sets `context.ExitCode` to a literal integer:
+   - Use `Grep` or `Read` to find the exception class definition (look for `class <ExceptionType>` in `Exceptions/` or `Commands/`)
+   - Check whether the class has an `ExitCode` property override (e.g., `public override int ExitCode => 2;`)
+   - If the literal does not match the property's return value, flag **HIGH** — the command will report a wrong exit code to scripts and CI pipelines that check `$LASTEXITCODE`
+   - **Fix pattern**: replace `context.ExitCode = 1;` with `context.ExitCode = ex.ExitCode;` so the exit code is always authoritative from the exception definition
+   - **Real example (PR #406, Comments 5 & 6)**: `ConfigFileNotFoundException.ExitCode => 2`, but two catch blocks in `AllSubcommand.cs` hardcoded `context.ExitCode = 1`. Scripts expecting exit code 2 on configuration errors received 1 instead.
+
+   **D2. Cross-method "catch-returns-null / caller-sets-ExitCode" pattern — same exit-code problem, harder to spot**
+   Rule 9-D catches *inline* catch blocks. This variant spans two methods and is easy to miss:
+   ```csharp
+   // Helper:
+   catch (ConfigFileNotFoundException) { logger.Log...; return null; }
+   // Caller:
+   var config = await LoadConfigAsync(...);
+   if (config == null) { context.ExitCode = 1; return; }   // ← wrong exit code
+   ```
+   Detection:
+   - When a diff adds a private helper method that catches a typed exception, logs guidance, and **returns null**, search the file for every call site of that helper
+   - At each call site, find the null-check guard and read the hardcoded `context.ExitCode` literal
+   - Look up the exception class's `ExitCode` override as in Rule 9-D
+   - If the literals differ, flag **HIGH**
+   - Also check whether the PR already fixed this pattern in a *sibling* command (e.g., `AllSubcommand.cs` was fixed to use `ex.ExitCode`). If so, grep all files in `Commands/` for the same helper+null-check pattern — any remaining hardcoded literal is a miss.
+   - **Fix pattern**: Change `return null;` to `throw;` in the helper; add `catch (ExceptionType ex) { context.ExitCode = ex.ExitCode; }` before the outer `catch (Exception ex)` at each call site. This matches the AllSubcommand.cs pattern.
+
 ### Step 3: Generate Findings
 
 For each issue found, provide:
@@ -880,6 +905,25 @@ When a string property represents a discrete set of values (e.g., `"obo"`, `"s2s
   ```
 - **Real example (PR #391, Comments 7 & 8)**: `SetupContext.AuthMode` and `NonDwBlueprintSetupOrchestrator.effectiveMode` both used `?.ToLowerInvariant()`, allowing `""` to disable OBO without any visible error.
 
+### 29. `args.Contains("--flag")` Misses `--flag=true` Form in System.CommandLine Preflight
+
+`System.CommandLine` normalises boolean options: a user can pass `--show-secret` (bare) **or** `--show-secret=true` / `--show-secret false`. Raw `args.Contains("--show-secret")` only matches the bare form; the `=true` variant slips through and re-enables any middleware that was meant to be skipped.
+
+- **Pattern to catch**: `args.Contains("--some-flag")` used in a middleware or startup guard (e.g., `isShowSecret`, `isHelpOrVersion`) — especially when it's a boolean `Option<bool>` in the command definition.
+- **Severity**: `medium` — wrong branch taken when the option is supplied as `--flag=true`; can silently re-enable Graph/network calls on commands that should work offline.
+- **Check**: Read `Program.cs` (or wherever the preflight guard lives) for every `args.Contains("--")` expression in the diff. If it guards a middleware skip, verify both forms are handled.
+- **Fix**:
+  ```csharp
+  // Wrong — misses --show-secret=true
+  var isShowSecret = args.Contains("--show-secret");
+
+  // Correct — handles bare and =value forms
+  var isShowSecret = args.Any(a =>
+      a.Equals("--show-secret", StringComparison.Ordinal)
+      || a.StartsWith("--show-secret=", StringComparison.Ordinal));
+  ```
+- **Real example (PR #406, Comment 1)**: `isShowSecret = args.Contains("--show-secret")` in `Program.cs` would not skip the Graph preflight when the user passed `--show-secret=true`, accidentally triggering an online call on an offline-only command.
+
 ## Example Invocation
 
 When you receive a request like "Review PR #253", you should:

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

@@ -1,36 +1,38 @@
 ---
 name: review-staged
-description: Generate structured code review for staged files (git staged changes) using Claude Code agents. Provides feedback before committing to catch issues early.
+description: Generate structured code review for staged files AND all branch commits since main, using Claude Code agents. Provides full PR-equivalent coverage before pushing.
 allowed-tools: Bash(git:*), Bash(dotnet:*), Bash(cd:*), Read, Write
 ---
 
 # Review Staged Files Skill
 
-Generate AI-powered code review comments for your staged files (git staged changes) before committing. Catch issues early in the development process using the same rigorous review standards as PR reviews.
+Generate AI-powered code review comments covering **both** your currently staged changes and all commits on the branch since it diverged from `main`. This gives the same full-branch view that Copilot and other PR reviewers see — not just what you're about to commit.
 
 ## Usage
 
 ```bash
-/review-staged              # Review all staged files
+/review-staged              # Review staged changes + full branch diff against main
 /review-staged --verbose    # Show detailed analysis
 ```
 
 Examples:
-- `/review-staged` - Review all currently staged files
+- `/review-staged` - Review staged changes and all branch commits not yet on main
 - `/review-staged --verbose` - Show detailed analysis with full context
 
 ## What this skill does
 
 1. **Checks for staged files** using `git diff --staged --name-only`
 2. **Fetches staged changes** using `git diff --staged`
-3. **Performs architectural review**: Questions design decisions, checks for scope creep, validates use cases
-4. **Analyzes changes** for security, testing, design patterns, and code quality issues
-5. **Differentiates contexts**: CLI code vs GitHub Actions code (different standards)
-6. **Creates actionable feedback**: Specific refactoring suggestions based on file names and patterns
-7. **Verifies documentation completeness** — detects user-visible surface changes (new CLI flags, renamed public types, payload contract changes, etc.) and verifies `CHANGELOG.md` + relevant `README.md`/`design.md` are updated in the same diff. For renames, greps all `*.md` files for stale references. See the Documentation Completeness section in `.claude/agents/pr-code-reviewer.md` for detection rules and severity calibration.
-8. **Runs the test suite and measures per-test timing** — flags any test taking > 1 second as a performance regression
-9. **Generates structured review document** saved to a markdown file
-10. **Shows summary** of all issues found organized by severity
+3. **Fetches full branch diff** using `git diff $(git merge-base HEAD origin/main)...HEAD` — covers all commits on the branch, not just staged files. This prevents issues in prior commits from going unreviewed.
+4. **Combines both diffs** for review: staged diff = what you're about to add; branch diff = full PR picture that Copilot will see
+5. **Performs architectural review**: Questions design decisions, checks for scope creep, validates use cases
+6. **Analyzes changes** for security, testing, design patterns, and code quality issues
+7. **Differentiates contexts**: CLI code vs GitHub Actions code (different standards)
+8. **Creates actionable feedback**: Specific refactoring suggestions based on file names and patterns
+9. **Verifies documentation completeness** — detects user-visible surface changes (new CLI flags, renamed public types, payload contract changes, etc.) and verifies `CHANGELOG.md` + relevant `README.md`/`design.md` are updated in the same diff. For renames, greps all `*.md` files for stale references. See the Documentation Completeness section in `.claude/agents/pr-code-reviewer.md` for detection rules and severity calibration.
+10. **Runs the test suite and measures per-test timing** — flags any test taking > 1 second as a performance regression
+11. **Generates structured review document** saved to a markdown file
+12. **Shows summary** of all issues found organized by severity
 
 ## Engineering Review Principles
 
@@ -102,8 +104,8 @@ Generated review is saved to:
 ```
 
 The review includes:
-- **Summary**: Overview of changes and key concerns
-- **Critical Issues**: Blocking issues that must be fixed
+- **Summary**: Overview of changes and key concerns — includes both staged and branch coverage
+- **Critical Issues**: Blocking issues that must be fixed (labeled `[staged]` or `[branch]`)
 - **High Priority**: Important issues that should be addressed
 - **Medium Priority**: Issues that improve code quality
 - **Low Priority**: Suggestions for enhancement
@@ -117,8 +119,17 @@ The skill uses **Claude Code directly** for semantic code analysis (same as revi
 2. Claude Code reads `.github/copilot-instructions.md` for coding standards
 3. Claude Code gets staged files: `git diff --staged --name-only`
 4. Claude Code gets staged changes: `git diff --staged`
-5. **Claude Code reads the complete current content of every staged file** (not just diff lines) to enable full-file semantic analysis.
-5a. **If any staged file is a test file** (path contains `.Tests.` or filename ends in `Tests.cs`), Claude Code MUST also invoke the `pr-test-analyzer` agent as a subagent to review test coverage quality. Pass the staged diff and list of staged test files as context. The `pr-test-analyzer` agent focuses on:
+5. **Claude Code gets the full branch diff against main:**
+   ```bash
+   git diff $(git merge-base HEAD origin/main)...HEAD --name-only   # files changed on branch
+   git diff $(git merge-base HEAD origin/main)...HEAD               # full branch diff
+   ```
+   This covers ALL commits on the branch since it diverged from `origin/main` — including prior commits that are no longer in the staged diff. The union of staged files and branch-diff files is the full review surface. This is the same view Copilot and PR reviewers see.
+
+   **When there are no staged files**, skip step 4 and use only the branch diff. When both exist, label findings clearly — `[staged]` for changes only in the staged diff, `[branch]` for changes from prior commits, so the developer knows which issues are still ahead of them vs. already committed.
+
+6. **Claude Code reads the complete current content of every file that appears in either diff** (staged or branch) to enable full-file semantic analysis.
+6a. **If any file in the combined diff is a test file** (path contains `.Tests.` or filename ends in `Tests.cs`), Claude Code MUST also invoke the `pr-test-analyzer` agent as a subagent to review test coverage quality. Pass the combined diff and list of test files as context. The `pr-test-analyzer` agent focuses on:
    - NSubstitute predicate precision (predicates on mutable reference types evaluated at assertion time vs. call time)
    - Missing test scenarios for new orchestration code paths (e.g., 409 idempotent path, tri-state verification results)
    - Assertions that track implementation rather than requirements
@@ -128,15 +139,14 @@ The skill uses **Claude Code directly** for semantic code analysis (same as revi
    - Parallel code structures that should be consolidated (e.g., a method building the same spec list as a shared helper)
    - Unused or dead code that was already there but not touched by the diff
    - Missing calls to shared helpers — where the diff adds a new use but existing code still has the old duplicate pattern
-   For each file path returned in step 3, Claude Code must `Read` the full file before performing analysis.
-6. Claude Code performs semantic analysis using its own capabilities
-7. Claude Code identifies specific issues with line numbers and code references
-8. **Claude Code runs the full test suite with per-test timing:**
+7. Claude Code performs semantic analysis using its own capabilities
+8. Claude Code identifies specific issues with line numbers and code references
+9. **Claude Code runs the full test suite with per-test timing:**
    ```bash
    cd src && dotnet test tests.proj --configuration Release --logger "console;verbosity=normal" 2>&1
    ```
    Parse the output for lines matching `[X s]` or `[X,XXX ms]` patterns. Extract test class name, method name, and duration. Flag any test method taking **> 1 second**. Group findings by test class and include the measured times in the review.
-8. Claude Code writes markdown file to `.codereviews/claude-staged-<timestamp>.md`
+10. Claude Code writes markdown file to `.codereviews/claude-staged-<timestamp>.md`
 
 **Test timing output format** (from `dotnet test --logger "console;verbosity=normal"`):
 ```
@@ -157,8 +167,8 @@ Any line showing `[X s]` where X ≥ 1 is a slow test. Report all such tests in
 1. **Stage your changes**: `git add <files>`
 
 2. **Review staged files**: `/review-staged`
-   - Analyzes all staged changes
-   - Generates review document
+   - Analyzes staged changes AND all commits on the branch since `main`
+   - Generates review document with `[staged]` / `[branch]` labels
    - Shows summary of issues
 
 3. **Address issues**: Fix any blocking or high-priority issues
@@ -169,15 +179,15 @@ Any line showing `[X s]` where X ≥ 1 is a slow test. Report all such tests in
 
 ## When to Use
 
-- **Before committing**: Catch issues early
-- **Before creating a PR**: Ensure quality before sharing
-- **After addressing PR comments**: Verify fixes are correct
+- **Before any commit**: Catch issues before they land in the branch history
+- **Before creating a PR**: Get the same full-branch view that Copilot will see — no surprises
+- **After addressing PR comments**: Verify fixes across the entire branch, not just the latest staged diff
 - **During code cleanup**: Validate refactoring changes
 - **When learning**: Get feedback on coding patterns
 
 ## Requirements
 
-- Git repository with staged changes
+- Git repository (staged changes optional — branch diff is always produced)
 - Repository must follow Agent365 DevTools coding standards
 - `.claude/agents/pr-code-reviewer.md` must exist (for review guidelines)
 - `.github/copilot-instructions.md` must exist (for coding standards)

CHANGELOG.md

@@ -23,6 +23,8 @@ 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
+- `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.
 - `setup requirements` Global Administrator path: when the well-known CLI client app is not found in a new tenant, Global Admins are prompted to create the app and grant admin consent automatically (enter an app ID or type `C` to create).
 - `--authmode obo|s2s|both` option on `setup all` — controls how the agent identity service principal receives permissions:
@@ -40,6 +42,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
 - 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.
 
 ### Fixed
+- Error messages for commands run without required configuration no longer expose internal file paths. `setup all`, `cleanup`, and `create-instance` without `--agent-name` now show actionable guidance with the exact command to run. `develop addpermissions` and `develop gettoken` without `--app-id` now prompt for the application ID directly.
 - `setup all` no longer inherits stale resource IDs when the user switches tenants between runs (`az logout` + `az login` to a different tenant). The CLI detects the tenant change before loading configuration, silently backs up files from the previous run, and prompts the user to re-run with `--agent-name` for a clean setup in the new tenant.
 - `setup permissions bot` no longer emits "Bot API permissions configured successfully" when any S2S app-role assignment fails; shows a warning with retry instructions instead.
 - Consent-required message "You are running as a non-admin user and cannot grant admin consent" replaced with "An administrator must grant tenant-wide consent to proceed" — the message fires when tenant-wide consent for S2S scopes has not yet been granted, not when the caller lacks admin rights.