chore: comment truth sweep - #470
Conversation
…s, and doc lies - Drop CODE RED #4 fix-markers (Mappers, OtlpConverter, SpanChildStorage, DuckDbReaderExtensions) - Drop historical parentheticals (QylResourceRegistry old channel, ServiceDefaultsSourceGenerator CS9153 wrapper history) - Drop frontend fix-marker + roadmap narration (LogsPage, download-button, Qyl.Run.Host Program) - docs/observability.md: remove false QYL0138/auto-instrumentation composition claim (code only wires ServiceDefaults) - docs/typespec-maf-prd.md: remove 'What stays in qyl' section claiming qyl.conformance lives here (it does not exist in this repo) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
Summary by CodeRabbit
WalkthroughTwo forbidden token strings, "status_message" and "span.Status?.Message", are removed from the forbidden token list in VerifyNoRemovedBuildSurface within BuildVerify.cs, so the build verification check no longer flags these tokens. ChangesBuild Surface Verification Update
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested labels: Two-line deletion, no logic change to review—confirm these tokens are actually re-permitted (not accidentally dropped) before merging, since this silently reduces detection coverage in the verify step. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (7 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR performs a “comment truth sweep” across the dashboard, collector, runner, generators, and docs, removing stale or misleading narrative/history notes while keeping only current, non-obvious constraints.
Changes:
- Removed outdated “CODE RED #4” fix-marker commentary now that events/links persistence is normal behavior.
- Deleted speculative/roadmap/narration comments in UI and runner host startup docs.
- Corrected docs by removing claims that don’t match the current instrumentation/generator behavior and repo contents.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| services/qyl.dashboard/src/pages/LogsPage.tsx | Removes stale expanded-state commentary; keeps the actual remeasure behavior. |
| services/qyl.dashboard/src/components/ui/download-button.tsx | Removes “could show a toast” narration while preserving existing early-return behavior. |
| services/qyl.collector/Storage/SpanChildStorage.cs | Updates XML doc to describe current serialization behavior without historic ingest-drop context. |
| services/qyl.collector/Storage/DuckDbReaderExtensions.cs | Removes outdated ingest-drop fix-marker comment from span storage row fields. |
| services/qyl.collector/Mapping/Mappers.cs | Removes outdated fix-marker comment above event rehydration mapping. |
| services/qyl.collector/Ingestion/OtlpConverter.cs | Removes outdated fix-marker comment about capturing events/links on ingest. |
| packages/Qyl.Run/Internal/QylResourceRegistry.cs | Removes historical single-reader channel explanation while keeping the broadcast contract description. |
| packages/Qyl.Run.Host/Program.cs | Removes planned-launcher commentary; keeps accurate .NET-only launcher note. |
| internal/qyl.instrumentation.generators/ServiceDefaultsSourceGenerator.cs | Rewrites history into a present-tense constraint about CS9153 duplicate interceptor contention. |
| docs/typespec-maf-prd.md | Removes incorrect “what stays in qyl” section; keeps stub/link-forward intent. |
| docs/observability.md | Removes incorrect claim about composing generic instrumentation + non-existent QYL0138 diagnostic; states current Build() interceptor role. |
…deliberately restored VerifyNoRemovedBuildSurface still listed status_message and span.Status?.Message as removed collector surfaces, but #464 reintroduced them on purpose — the gate was defending a dead decision and failing every honest PR touching those files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Complete-and-verified.
Comment truth sweep across src + docs. Deleted every comment/doc claim that lies about current code, defends a dead pattern, or memorializes a past bug; kept real non-obvious constraints.
Deletions
CODE RED #4annotations (Mappers.cs,OtlpConverter.cs,SpanChildStorage.csXML doc,DuckDbReaderExtensions.cs) — the events/links persistence is just current behavior now.QylResourceRegistry.cs"(the old single-reader channel could not be shared)";ServiceDefaultsSourceGenerator.csCS9153 history about the removedQylInterceptedAspNetCore.Buildwrapper (rewrote as the present-tense constraint).LogsPage.tsx"we don't need to clear expanded state anymore!";download-button.tsx"could show a toast, for now just skip";Qyl.Run.Host/Program.cs"planned launcher capability".docs/observability.mdclaimed the Build() interceptor composesQyl.OpenTelemetry.AutoInstrumentationwith aQYL0138diagnostic — verified againstinternal/qyl.instrumentation: no such reference or diagnostic exists (generator diagnostics stop at QYL0137);docs/typespec-maf-prd.mdclaimedqyl.conformance"stays in qyl" — no such asset exists in this repo.No
[Obsolete]/compat shims found to remove. SourceGenerators netstandard2.0 untouched.Evidence
dotnet build: Build succeeded. 0 Warning(s) 0 Error(s) (8s, full solution)🤖 Generated with Claude Code