Skip to content

Commit f40b694

Browse files
ashragrawalclaude
andcommitted
Address PR #362 review (gwharris7): fix Copilot install + maturity docs, add faithful invocation exit-code tests
- a365-evaluate-instructions.md: the pipeline invokes the standalone GitHub Copilot CLI (binary `copilot`), not the `gh copilot` extension, so verify is now `copilot --version` and install is `npm install -g @github/copilot` (corrected consistently in the note, verify step, install, and troubleshooting). - a365-evaluate-instructions.md: maturity levels corrected from 1-5 to the actual 0-4 scale (Functional..Exemplary); triage bands remapped to the real MaturityCalculator thresholds. - EvaluateCommandInvocationTests.cs: add faithful invocation-layer exit-code tests (invalid --eval-engine -> 3, missing --server-url -> 1, whitespace --output-dir -> 1, plus arg-propagation) via a parser that mirrors the Program.cs Agent365Exception.ExitCode mapping over a real pipeline with mocked sub-deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e5e7f10 commit f40b694

2 files changed

Lines changed: 106 additions & 9 deletions

File tree

docs/agent365-guided-setup/a365-evaluate-instructions.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ Wait for the answer. Store as `evalEngine`:
4040
- If **3 (Claude Code)**: `evalEngine = "claude-code"`
4141
- If **4 (None)**: `evalEngine = "none"`
4242

43-
> **Note:** Auto and the named engines require the corresponding CLI to be installed on the workstation (`gh copilot` or `claude`). If neither is installed and `evalEngine` is `auto` or a named engine, the pipeline will stop after writing the checklist and print BYO-LLM instructions — that is expected, not an error.
43+
> **Note:** Auto and the named engines require the corresponding CLI to be installed on the workstation (`copilot` or `claude`). If neither is installed and `evalEngine` is `auto` or a named engine, the pipeline will stop after writing the checklist and print BYO-LLM instructions — that is expected, not an error.
4444
4545
After all three questions are answered, create all todos for the path and mark Todo 1 in-progress:
4646

@@ -106,14 +106,14 @@ The evaluation pipeline scores semantic checks (tool-name clarity, description q
106106

107107
### If `evalEngine = "github-copilot"` or `"auto"`
108108

109-
Verify GitHub Copilot CLI is installed:
109+
Verify the GitHub Copilot CLI is installed:
110110

111111
```bash
112-
gh copilot --version
112+
copilot --version
113113
```
114114

115115
- If installed: continue.
116-
- If not installed: install with `gh extension install github/gh-copilot` (requires `gh` CLI). If `gh` itself is missing, see [GitHub CLI install](https://cli.github.com/).
116+
- If not installed: install with `npm install -g @github/copilot` (requires Node.js 18 or later). This is the standalone GitHub Copilot CLI (binary `copilot`) that the pipeline invokes — it is a different tool from the `gh copilot` GitHub CLI extension, which does not accept the flags the pipeline uses.
117117

118118
### If `evalEngine = "claude-code"` or `"auto"` (and Copilot was not found)
119119

@@ -224,15 +224,15 @@ The CLI logs progress in numbered steps `[1/5]` through `[5/5]`:
224224
The report has:
225225

226226
- **Overall score** (0–100). Higher is better.
227-
- **Maturity level** (1–5): Level 1 (Initial) → Level 5 (Optimized).
227+
- **Maturity level** (0–4): Level 0 (Functional) → Level 4 (Exemplary).
228228
- **Per-tool scores** with category breakdowns: schema quality, semantic clarity, parameter quality, return-shape quality.
229229
- **Action item list**, ordered by impact.
230230

231231
### Triage with the user
232232

233-
1. **If overall score is below 60 or maturity is Level 1–2**, walk through the top 3 action items with the user. These usually cluster around: tool names that don't describe the action, descriptions that are stubs, parameter names that are abbreviations.
234-
2. **If overall score is 60–80 or maturity is Level 3**, the schema is shippable but has room. Pick 1–2 high-impact action items to address.
235-
3. **If overall score is above 80 or maturity is Level 4–5**, summarize the strengths and surface any low-hanging fixes.
233+
1. **If overall score is below 60 or maturity is Level 0–1 (Functional/Described)**, walk through the top 3 action items with the user. These usually cluster around: tool names that don't describe the action, descriptions that are stubs, parameter names that are abbreviations.
234+
2. **If overall score is 60–74 or maturity is Level 2 (Consistent)**, the schema is shippable but has room. Pick 1–2 high-impact action items to address.
235+
3. **If overall score is 75 or above or maturity is Level 3–4 (Optimized for AI/Exemplary)**, summarize the strengths and surface any low-hanging fixes.
236236

237237
### Re-running after fixes
238238

@@ -267,7 +267,7 @@ If any step results in an error, stop and analyze the error message carefully.
267267
|---|---|---|
268268
| `Unauthorized` from `tools/list` | Wrong or expired bearer token | Re-acquire the token (Question 2). |
269269
| `Could not parse server URL` | URL doesn't include the protocol | Add `http://` or `https://` to the URL. |
270-
| `No coding agent detected` after `[2/5]` | Neither `gh copilot` nor `claude` is on PATH | Install one (Step 2) or pass `--eval-engine none`. |
270+
| `No coding agent detected` after `[2/5]` | Neither `copilot` nor `claude` is on PATH | Install one (Step 2) or pass `--eval-engine none`. |
271271
| `Failed to write report to <path>` | Output dir not writable | Choose a different `--output-dir` or fix permissions. |
272272
| Telemetry warning at debug level | Non-blocking — the marker call failed | Ignore. The evaluation runs regardless. |
273273

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/EvaluateCommandInvocationTests.cs

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,16 @@
22
// Licensed under the MIT License.
33

44
using System.CommandLine;
5+
using System.CommandLine.Builder;
6+
using System.CommandLine.Invocation;
7+
using System.CommandLine.Parsing;
58
using FluentAssertions;
69
using Microsoft.Agents.A365.DevTools.Cli.Commands;
10+
using Microsoft.Agents.A365.DevTools.Cli.Exceptions;
711
using Microsoft.Agents.A365.DevTools.Cli.Services;
812
using Microsoft.Agents.A365.DevTools.Cli.Services.Evaluate;
913
using Microsoft.Extensions.Logging;
14+
using Microsoft.Extensions.Logging.Abstractions;
1015
using NSubstitute;
1116
using Xunit;
1217

@@ -101,4 +106,96 @@ await pipeline.Received(1).RunAsync(
101106
Environment.SetEnvironmentVariable(AuthTokenEnvVar, original);
102107
}
103108
}
109+
110+
// The bare Command.InvokeAsync(string) shortcut used above wraps every thrown exception
111+
// as exit code 1 via System.CommandLine's default handler, so it cannot observe an
112+
// EvaluationException's ExitCode (3). The tests below build a parser that mirrors the
113+
// exception-to-exit-code mapping in Program.cs (Agent365Exception.ExitCode) plus its
114+
// parse-error short-circuit, and drive a real EvaluationPipelineService (sub-dependencies
115+
// mocked) so ParseEvalEngine and the output-dir guard actually run. This pins the
116+
// invocation-layer exit codes against a refactor of the SetHandler wiring that would
117+
// otherwise break those branches with all tests still green.
118+
private static Parser BuildFaithfulParser(Command evaluate) =>
119+
new CommandLineBuilder(evaluate)
120+
.UseExceptionHandler((exception, context) =>
121+
context.ExitCode = exception is Agent365Exception agentEx ? agentEx.ExitCode : 1)
122+
.AddMiddleware(async (context, next) =>
123+
{
124+
// Mirror Program.cs: a parse error (e.g. a missing required option) exits
125+
// non-zero before the command handler runs.
126+
if (context.ParseResult.Errors.Count > 0)
127+
{
128+
context.ExitCode = 1;
129+
return;
130+
}
131+
132+
await next(context);
133+
}, MiddlewareOrder.ErrorReporting)
134+
.Build();
135+
136+
// Real pipeline so ParseEvalEngine and the output-dir guard execute; the sub-dependencies
137+
// are mocked because the failure paths under test throw or return before reaching them.
138+
private static IEvaluationPipelineService RealPipeline() =>
139+
new EvaluationPipelineService(
140+
NullLogger<EvaluationPipelineService>.Instance,
141+
Substitute.For<ISchemaDiscoveryService>(),
142+
Substitute.For<IChecklistGenerator>(),
143+
Substitute.For<IChecklistEvaluator>(),
144+
Substitute.For<IEvaluationAnalyzer>(),
145+
Substitute.For<IReportGenerator>(),
146+
Substitute.For<IAgent365ToolingService>());
147+
148+
[Fact]
149+
public async Task InvokeAsync_InvalidEvalEngine_ReturnsEvaluationExceptionExitCode3()
150+
{
151+
var evaluate = GetEvaluateSubcommand(Substitute.For<ILogger>(), RealPipeline());
152+
var parser = BuildFaithfulParser(evaluate);
153+
154+
var exitCode = await parser.InvokeAsync(
155+
new[] { "--server-url", "http://localhost/mcp", "--eval-engine", "bogus" });
156+
157+
exitCode.Should().Be(3, because: "an unknown --eval-engine raises an EvaluationException, whose ExitCode (3) the real exception handler must propagate to the process");
158+
}
159+
160+
[Fact]
161+
public async Task InvokeAsync_MissingServerUrl_ReturnsExitCode1()
162+
{
163+
var evaluate = GetEvaluateSubcommand(Substitute.For<ILogger>(), PipelineReturning(0));
164+
var parser = BuildFaithfulParser(evaluate);
165+
166+
var exitCode = await parser.InvokeAsync(new[] { "--eval-engine", "none" });
167+
168+
exitCode.Should().Be(1, because: "--server-url is a required option; the missing-required-option parse error must exit non-zero without invoking the handler");
169+
}
170+
171+
[Fact]
172+
public async Task InvokeAsync_WhitespaceOutputDir_ReturnsExitCode1()
173+
{
174+
var evaluate = GetEvaluateSubcommand(Substitute.For<ILogger>(), RealPipeline());
175+
var parser = BuildFaithfulParser(evaluate);
176+
177+
var exitCode = await parser.InvokeAsync(
178+
new[] { "--server-url", "http://localhost/mcp", "--output-dir", " ", "--eval-engine", "none" });
179+
180+
exitCode.Should().Be(1, because: "an empty or whitespace --output-dir must fail fast with exit code 1 through the real handler wiring, not a deep exception later in the run");
181+
}
182+
183+
[Fact]
184+
public async Task InvokeAsync_ValidArguments_ForwardsEachOptionToRunAsync()
185+
{
186+
var pipeline = PipelineReturning(0);
187+
var evaluate = GetEvaluateSubcommand(Substitute.For<ILogger>(), pipeline);
188+
var parser = BuildFaithfulParser(evaluate);
189+
190+
var exitCode = await parser.InvokeAsync(
191+
new[] { "--server-url", "http://localhost:5000/mcp", "--output-dir", "out", "--eval-engine", "claude-code" });
192+
193+
exitCode.Should().Be(0, because: "RunAsync returned 0, which the handler must propagate as the process exit code");
194+
await pipeline.Received(1).RunAsync(
195+
"http://localhost:5000/mcp",
196+
"out",
197+
"claude-code",
198+
Arg.Any<string?>(),
199+
Arg.Any<CancellationToken>());
200+
}
104201
}

0 commit comments

Comments
 (0)