chore(webapi-aot-demo): capture native linker error on AOT-link failure - #11
Conversation
The macOS NativeAOT native-link step has intermittently flaked with `clang exited with code 1`, but the publish ran at `-v quiet`, which swallows the linker's (clang/ld) stderr -- the failing CI run showed only MSB3073 with no root cause. On failure, re-publish once at `-v detailed` purely to capture that stderr into the log the verifier already prints. The happy path stays `-v quiet` (byte-identical) and the outcome is unchanged (the original failure still fails the gate). Verified locally: webapi-aot-demo-ok qyl_warnings=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe NativeAOT verification script now routes ChangesNativeAOT publish retry flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/verify-webapi-aot-demo.py`:
- Around line 157-160: The retry logging in verify_webapi_aot_demo currently
overwrites the original failed publish output, so preserve the first publish’s
stdout/stderr and append the retry diagnostics instead of replacing them. Update
the failure handling around the publish/diagnostic flow in
verify-webapi-aot-demo.py so fail() reports the original quiet output from the
first failed run while still including the later detailed retry output for
context, using the existing diagnostic and completed values to keep the logs
aligned with the actual failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06d67b30-4530-40af-a592-4c574abaf437
📒 Files selected for processing (1)
tools/verify-webapi-aot-demo.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke (qyl-macos)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
AGENTS.md
**: # Qyl.OpenTelemetry.AutoInstrumentation agent rulesMission
This repository is the runtime AOT auto-instrumentation lane for qyl. Keep it separate from:
- semantic-convention package generation,
- the old CLR-profiler/OpenTelemetry auto-instrumentation substrate,
- unrelated compile-time tracing experiments.
The product goal is .NET 10 NativeAOT-compatible zero-code instrumentation through managed
build assets, source generation, DiagnosticListener consumption, and module-initializer boot.Clean slate before work
Before implementation work, confirm:
git worktree list git branch --show-current git diff --cached --name-only git stash list git status --shortWork from
mainunless the task explicitly asks for a topic branch, and hand the tree back as
clean as you found it — no stale local branches, stashes, staged files, or unrelated untracked
files left behind.Build and test reality
- SDK is pinned by
global.json(10.0.300,rollForward: latestFeature).- Build everything:
dotnet build Qyl.OpenTelemetry.AutoInstrumentation.slnx.TreatWarningsAsErrorsis on repo-wide with a heavy analyzer stack (trim/AOT/single-file
analyzers, ErrorProne.NET, Roslynator, PublicApiAnalyzers on packaged projects). A clean
build is the validation floor; analyzer regressions fail the build by design.- There are no
dotnet testprojects. Behavior is proven by the Python verifiers intools/
and the snapshot fixture undertests/Qyl.OpenTelemetry.AutoInstrumentation.SourceGenerators.Snapshots
(compare againstverified/). Route changes through the validation table below.- Public API changes require updating the
PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt
baselines next to each packaged project (python3 tools/verify-public-api-baseline.py).- CI runs
tools/smoketest.shon pull requests and pushes tomain, plus the OTLP collector
fixture and WebAPI AOT demo workflows under.github/workflows/.- CI runs o...
Files:
tools/verify-webapi-aot-demo.py
⚙️ CodeRabbit configuration file
**: Operating principles (solo-dev, agentic SDLC — reviews are advisory, agents act on them):
- LAZY: one self-contained, correct review beats ten partial ones. Every finding is
definitive — concrete evidence with file:line, a concrete fix, no "consider maybe",
no open or ambiguous questions back to the author. If you cannot decide a point
from the diff plus repo context, stay silent on it. Never cite a source, API, or
version you have not verified; an unverifiable claim is a dropped claim.- IMPATIENT: never stall a PR. There are no compatibility obligations here — internal
and dogfooding code has NO public-API contract; removing shims, breaking signatures,
and deleting dead paths are normal, desirable changes. Do not flag backward
compatibility, deprecation ceremony, or migration paths. (SemVer applies only to
commercially sold libraries — this repo has none.)- EGO: hold the bar of the best reviewer on the market — flag real correctness,
security, data-loss, and structural problems precisely; produce zero noise.
Files:
tools/verify-webapi-aot-demo.py
🪛 ast-grep (0.44.0)
tools/verify-webapi-aot-demo.py
[error] 120-144: Command coming from incoming request
Context: subprocess.run(
[
"dotnet",
"publish",
str(project),
"-c",
"Release",
"-r",
runtime_identifier(),
"-p:PublishAot=true",
"-p:TreatWarningsAsErrors=false",
"--self-contained",
"true",
"-o",
str(output),
"-v",
verbosity,
],
cwd=project.parent,
env=env,
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
| log.write_text(diagnostic.stdout, encoding="utf-8") | ||
| fail( | ||
| "NativeAOT web API publish failed\n" | ||
| f"exit={completed.returncode}\nlog={log}\n{completed.stdout}" | ||
| f"exit={completed.returncode}\nlog={log}\n{diagnostic.stdout}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not overwrite the first failed publish output.
If Line 149 fails and Line 156 succeeds, Line 157 replaces the only stdout from the failing publish with a successful retry. The verifier then fails using exit={completed.returncode} while log and the emitted body describe a different run, which breaks the diagnostic-only purpose of this change for intermittent flakes. Preserve the original quiet output and append the detailed retry instead of replacing it.
Suggested fix
completed = _publish(project, output, env, "quiet")
log.write_text(completed.stdout, encoding="utf-8")
if completed.returncode != 0:
# The macOS NativeAOT native-link step has intermittently flaked here with clang exit 1,
# and "-v quiet" swallows the linker's (clang/ld) stderr, so the cause is invisible in CI.
# Re-publish once at detailed verbosity purely to capture that error for diagnosis. This
# does not change the outcome -- the original failure still fails the gate.
diagnostic = _publish(project, output, env, "detailed")
- log.write_text(diagnostic.stdout, encoding="utf-8")
+ combined_output = (
+ "=== initial quiet publish (failed) ===\n"
+ f"{completed.stdout}\n"
+ "=== diagnostic detailed retry ===\n"
+ f"{diagnostic.stdout}"
+ )
+ log.write_text(combined_output, encoding="utf-8")
fail(
"NativeAOT web API publish failed\n"
- f"exit={completed.returncode}\nlog={log}\n{diagnostic.stdout}"
+ f"exit={completed.returncode}\nlog={log}\n{combined_output}"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.write_text(diagnostic.stdout, encoding="utf-8") | |
| fail( | |
| "NativeAOT web API publish failed\n" | |
| f"exit={completed.returncode}\nlog={log}\n{completed.stdout}" | |
| f"exit={completed.returncode}\nlog={log}\n{diagnostic.stdout}" | |
| combined_output = ( | |
| "=== initial quiet publish (failed) ===\n" | |
| f"{completed.stdout}\n" | |
| "=== diagnostic detailed retry ===\n" | |
| f"{diagnostic.stdout}" | |
| ) | |
| log.write_text(combined_output, encoding="utf-8") | |
| fail( | |
| "NativeAOT web API publish failed\n" | |
| f"exit={completed.returncode}\nlog={log}\n{combined_output}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/verify-webapi-aot-demo.py` around lines 157 - 160, The retry logging in
verify_webapi_aot_demo currently overwrites the original failed publish output,
so preserve the first publish’s stdout/stderr and append the retry diagnostics
instead of replacing them. Update the failure handling around the
publish/diagnostic flow in verify-webapi-aot-demo.py so fail() reports the
original quiet output from the first failed run while still including the later
detailed retry output for context, using the existing diagnostic and completed
values to keep the logs aligned with the actual failure.
Sources: Coding guidelines, Path instructions
…trument (CODE RED #1/#2/#10) (#22) An adversarial OTel-compliance + honesty audit rated this code 3/10. This lands the verified subset of the confirmed HIGH/MEDIUM defects. #1 (HIGH, ~0-duration lie): the HttpClient/AspNetCore/gRPC DiagnosticListeners created their span on the framework's *.Stop event via the 2-arg StartActivity (StartTimeUtc=now) and immediately disposed it, so every emitted span had ~0 duration instead of the real operation latency. New internal QylActivitySource.StartAtAmbientStart(name, kind) stamps the span to the ambient framework Activity's real StartTimeUtc (parented to it for trace correlation), with a now-stamped fallback when there is no ambient activity. All three listeners switched to it. #2 (HIGH, dishonest doc): DiagnosticListenerSubscriber claimed it "publishes the same span shapes" — false while durations were fabricated. Doc now states the actual mechanism (reacts on *.Stop, stamps to ambient start for real duration). #10 (MEDIUM, metrics-semconv): dotnet.thread_pool.thread.count was an unitless ObservableGauge<int>; per OTel semconv it is an (Observable)UpDownCounter with UCUM unit {thread}. Switched instrument type + added unit. Verified: core.slnf Release 0/0; verify-webapi-aot-demo passes (fixture unchanged — the attribute shape is identical; note the fixture is duration-insensitive, which is itself tracked as a separate finding). No PublicAPI change (all internal/private). NOT in this commit (separate verified increments): #3 double-count between the listener and interceptor lanes (+ the fixture that masks it), #4/#11 OTLP Events/Links/Status.Message dropped on ingest, #6 url.scheme, #7 http.request.method_original on the interceptor path, #9 Azure span name ignores methodName. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
PR #10's
webapi-aot-demo (qyl-macos)leg failed once with a NativeAOT native-link error (clang … exited with code 1, MSB3073), then went green on the merge-to-main run — a transient flake. The problem: the publish runs at-v quiet, so MSBuild swallowed clang/ld's actual stderr; the CI log showed only the command + exit code, no root cause. If it flakes again we're still blind.What
tools/verify-webapi-aot-demo.py: extract the publish into a_publish(…, verbosity)helper. The happy path still runs-v quiet(byte-identical). On failure only, re-publish once at-v detailedpurely to capture the linker stderr into the log the verifier already prints. No change to pass/fail semantics — the original failure still fails the gate, and the qyl-owned-warning scan still runs against the quiet output.Validation
python3 -m py_compileclean.webapi-aot-demo-ok qyl_warnings=0(happy path unchanged).Diagnostic-only hardening for a known-flaky native-link leg; revert once the flake's root cause is identified.
🤖 Generated with Claude Code