Repository navigation
feat: Native AOT build and PR smoke test (PR 2 of 3) - #192
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…parity cases Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Spectre.Console's GitHub enricher turns on ANSI colour under GITHUB_ACTIONS=true, which children inherit, so the stats timing row survived NormalizeStats and Stats_* failed on every aot-smoke run; strip escapes only to decide which line to drop. Also give every MCP request in McpParityTests a 2-minute CancellationToken so a deadlocked server fails the test instead of hanging, add timeout-minutes: 30 to the aot-smoke CI job, require diagram parity cases to produce non-empty output, and add --help parity cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CONTRIBUTING.md's Native AOT prerequisite sentence was missing zlib1g-dev, which the CI job installs, and didn't note the commands are linux-x64. SmokeCommand.cs's "same SDK" comment was inaccurate for a self-contained .dll, which uses its app-local runtime. The plan document is updated to match: the git grep form that actually distinguishes intentional csproj comments from a real PublishAot property, the GITHUB_ACTIONS=true env needed to exercise Spectre's GitHub Actions ANSI path locally, the artifacts/native deviation, and the CliParityTests/McpParityTests code blocks as changed by the fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are reliability/correctness issues in the new test harness around timeout handling (combined MCP timeout budget and process timeout cleanup) that should be fixed before relying on the new CI gate.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR implements the second stage of the Native AOT effort by making the CLI and MCP server publishable as Native AOT (opt-in via -p:PublishAot=true), and adding a smoke test suite plus a dedicated CI job that validates native vs JIT parity for the same commit.
Changes:
- Added
Tests.Smoke.Aotparity suite that runs the CLI and MCP server in both native and JIT modes and compares outcomes. - Enabled trim/AOT analyzers in
ProjGraph.CliandProjGraph.Mcp, added necessary trimmer roots and analyzer suppressions for Spectre reflection usage. - Added an
aot-smokeGitHub Actions job that publishes native executables and runs the parity suite on PRs.
File summaries
| File | Description |
|---|---|
| tests/ProjGraph.Tests.Smoke.Aot/ProjGraph.Tests.Smoke.Aot.csproj | New smoke test project definition and dependencies |
| tests/ProjGraph.Tests.Smoke.Aot/CliParityTests.cs | CLI native-vs-JIT parity test cases |
| tests/ProjGraph.Tests.Smoke.Aot/McpParityTests.cs | MCP native-vs-JIT parity test cases |
| tests/ProjGraph.Tests.Smoke.Aot/Helpers/AotSmokeFactAttribute.cs | Skippable fact attribute controlled by PROJGRAPH_SMOKE_* env vars |
| tests/ProjGraph.Tests.Smoke.Aot/Helpers/McpServerPair.cs | Shared native/reference MCP server clients for test classes |
| tests/ProjGraph.Tests.Smoke.Aot/Helpers/ProcessRunner.cs | Process execution helper capturing exit code/stdout/stderr |
| tests/ProjGraph.Tests.Smoke.Aot/Helpers/SmokeCommand.cs | Launch abstraction supporting exe vs dotnet <dll> |
| tests/ProjGraph.Tests.Smoke.Aot/Helpers/SmokeEnvironment.cs | Env var resolution + repo-root discovery for smoke runs |
| tests/ProjGraph.Tests.Smoke.Aot/Fixtures/legacy.sln | Legacy solution fixture for CLI parsing coverage |
| src/ProjGraph.Cli/ProjGraph.Cli.csproj | Trim/AOT analyzers, ILC warning policy, and trimmer roots for Spectre |
| src/ProjGraph.Cli/Program.cs | Suppression for IL3050 on Main with AOT justification |
| src/ProjGraph.Cli/Infrastructure/DependencyInjection.cs | Suppression for IL2067 on Spectre type registration path |
| src/ProjGraph.Mcp/ProjGraph.Mcp.csproj | Trim/AOT analyzers and ILC warning policy for native publish |
| ProjGraph.slnx | Adds the new smoke test project to the solution |
| .github/workflows/ci.yml | Adds aot-smoke job that publishes native and runs parity suite |
| docs/superpowers/plans/2026-09-14-native-aot-pr2-native-build-smoke.md | Implementation plan document for PR2 |
| CONTRIBUTING.md | Documents the smoke suite and how to run it locally |
| CLAUDE.md | Updates test-project list to include the smoke suite |
| ARCHITECTURE.md | Updates repo/test layout docs to include the smoke suite |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+61
| catch (OperationCanceledException) | ||
| { | ||
| process.Kill(entireProcessTree: true); | ||
| throw new TimeoutException( | ||
| $"'{command.FileName} {string.Join(' ', arguments)}' did not exit within {Timeout}."); | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 685a29f. The reads no longer take the timeout token. On timeout the process tree is killed, its exit is awaited, and both reads are awaited, so neither task goes unobserved. Killing closes the pipes, so the reads complete right away, and the captured stderr (plus the full command, including the dotnet <dll> prefix) goes into the TimeoutException message. I checked this with a throwaway program running a hung bash -c '…; sleep 60' child under a 2 s timeout: the reads completed with the partial output at ~2.08 s, and no child process was left running.
Comment on lines
+124
to
+129
| var (native, reference) = await servers.GetClientsAsync(); | ||
| using var timeout = new CancellationTokenSource(RequestTimeout); | ||
|
|
||
| var referenceResult = await request(reference, timeout.Token); | ||
| AssertSameJson(await request(native, timeout.Token), referenceResult); | ||
| return referenceResult; |
Owner
Author
There was a problem hiding this comment.
Fixed in 685a29f. A new SendWithTimeoutAsync helper gives each request its own CancellationTokenSource(RequestTimeout), so the reference and native calls each get the full 2 minutes and a slow reference call can't shorten the native budget. The native suite still passes 28/28, with and without GITHUB_ACTIONS=true.
CodeQL autobuild selected the new tests/ProjGraph.Tests.Smoke.Aot/Fixtures/legacy.sln fixture instead of ProjGraph.slnx, compiled nothing, and failed. Manual build mode builds the real solution regardless of fixture files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…parately - ProcessRunner: the stdout/stderr reads no longer take the timeout token. On a timeout the process tree is killed, its exit awaited, and both reads awaited, so the tasks are observed and the captured stderr is included in the TimeoutException. - McpParityTests: the reference and native requests each get their own 2-minute CancellationTokenSource instead of sharing one budget. Addresses Copilot review comments on #192. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Second of three PRs from the Native AOT design (
docs/superpowers/specs/2026-09-14-native-aot-design.md). The CLI and MCP server can now be published as Native AOT executables, and a new CI job checks every PR against the JIT build of the same commit. AOT stays opt-in: no project setsPublishAot, sodotnet packandpublish.ymlare unchanged.ProjGraph.Cli):TrimmerRootAssemblyforProjGraph.CliandSpectre.Console.Cli, which Spectre reflects over for commands and settings.[UnconditionalSuppressMessage]attributes whose justifications cite the rooting and the smoke suite: IL3050 onProgram.Main(CommandApp) and IL2067 onTypeRegistrar.Register.EnableTrimAnalyzer/EnableAotAnalyzeron every build, andIlcTreatWarningsAsErrors=falseso third-party ILC warnings don't fail publish.ProjGraph.Mcp): the same analyzers andIlcTreatWarningsAsErrors=false. PR 1 already removed the runtime blockers.tests/ProjGraph.Tests.Smoke.Aot:PROJGRAPH_SMOKE_CLI_NATIVE/_CLI_REFERENCE/_MCP_NATIVE/_MCP_REFERENCEare set, so the existing matrix reports them as skipped. WithPROJGRAPH_SMOKE_REQUIRED=truethey never skip.aot-smokejob onubuntu-latestinstallsclang/zlib1g-dev, builds Release, publishes both executables with-p:PublishAot=true, and runs the suite. It has a 30-minute timeout.CLAUDE.md,CONTRIBUTING.md(including how to run the suite locally), andARCHITECTURE.md.The branch also carries the implementation plan (
docs/superpowers/plans/2026-09-14-native-aot-pr2-native-build-smoke.md).Results (linux-x64, SDK 10.0.401)
ProjGraph.Cli36.6 MB andProjGraph.Mcp42.9 MB.GITHUB_ACTIONS=true.--helpcases were added.dotnet formatis clean..dllpasses./bin/truefails.PROJGRAPH_SMOKE_REQUIRED=truefails instead of skipping.Could not get settings type for command of type 'ProjGraph.Cli.Commands.VisualizeCommand', and the suite caught it.Deviations from the spec
PublishSingleFilecondition on the MCP project. With it left unconditional, the AOT publish succeeds and produces the same 42,946,344-byte binary, so the condition is a no-op. PR 3 removesPublishSingleFileentirely.--artifacts-path.ProjGraph.Mcpis self-contained, so an in-place AOT publish reusesbin/Release/net10.0/linux-x64. That deletes the JIT apphost and rewrites itsruntimeconfig.jsonwith AOT feature switches, which corrupts the reference build..dllor an executable. A.dllruns through thedotnethost. The MCP reference isProjGraph.Mcp.dllfrom the self-contained build output.PROJGRAPH_SMOKE_REQUIRED. Without it, a misspelled variable in CI would skip every test and leave the required job green.--outputruns share one path. Spectre wraps theSaved to <path>stderr line at 80 columns, so both builds write to the same path one after the other.isError != true. Diagram cases also require non-empty output. A wrong path therefore can't pass as parity.visualize,stats, and MCPget_project_statswarnings);.slnthat lists a project twice;ConcurrentDictionary/Barrier/[Required]types in a class diagram;prompts/get;--helpandvisualize --help.options.inheritanceis actuallyoptions.includeInheritance..slnfixture. It also includes a solution folder,NestedProjects, and a classic C# project-type GUID.artifacts/native/*instead of the spec'sout/*, becauseout/isn't git-ignored.Found and fixed during review
GITHUB_ACTIONS=true. Thestatsnormalizer then missed theAnalysis timerow, and both stats cases would have failed on the first CI run; local runs couldn't show it. The normalizer now strips escape sequences before deciding which row to drop, and the local verification step setsGITHUB_ACTIONS=true.timeout-minutes: 30, so a hung native server can't block CI for hours.Notes
ILC: Method '[Spectre.Console.Cli]OpenCli.OpenCliParser.Parse(...)' will always throw because: Failed to load assembly 'NJsonSchema'. It comes from rooting Spectre's unused OpenCLI parser. It isn't a warning and doesn't affect any command.--helpcases.CreateApp()helper;Exit criteria (spec, PR 2)
git grep -n "<PublishAot" -- '*.csproj' '*.props' '*.targets'is empty.aot-smokeis green on this PR. Its first run on a hosted runner is this PR.aot-smokeas a required status check ondevelop.🤖 Generated with Claude Code