test(coverage): uplift Reactor.dll line coverage 79.52% → 80.79%#333
Merged
Conversation
Adds: - docs/reports/coverage-uplift-85.md — self-contained tracking doc so any agent on any machine can pick up the work without re-learning the recipe. - tools/coverage/run-coverage.ps1 — wraps the CONTRIBUTING.md recipe (build → unit → instrument → selftest → merge) into one command. - tools/coverage/report-gaps.ps1 — parses merged cobertura output and writes a ranked hot-spot table to coverage/gap-report.md. - tools/coverage/README.md — usage notes. - .gitignore — carve tools/coverage/ out of the coverage/ ignore. No product or test changes yet; baseline measurement lands next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ran baseline merged coverage (unit + selftest). Captured the result and the per-file analysis in docs/reports/coverage-uplift-85.md so future sessions can pick up cold. - Baseline: 79.52% line / 67.25% branch (gap to 85%: ~5,562 lines). - Hot-spot worklist organized by test tier (U/S/U+S) — pre-classifies every top-40 file so the next agent skips the "is this unit-testable?" rediscovery step. - Audit findings: SetExtensionsCoverageTests.cs (~345 lines) is pure vanity (asserts setter-delegate count, never the side effect). Plan recorded to replace with selftest fixtures rather than delete first. - Honest deferral candidates flagged: PreviewCaptureServer, JumpListComInterop, TrayFlyoutHostWindow, TaskbarOverlay, ChartAutomationPeer — ~1,639 lines inherently hard to test from a headless harness; user decision pending. - Fixed report-gaps.ps1 PowerShell parser issue (mid-string backtick). No product/test code changes in this commit — diagnosis-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds 11 unit tests to DevtoolsPropertyToolTests exercising previously- uncovered branches of the pure parser/formatter helpers. Intended as a worked example of the audit-first workflow described in docs/reports/coverage-uplift-85.md — each test ties to an observable product contract: - IFormattable invariant-culture formatting (decimal) - 2-value Thickness via ParseValue (not just direct TryParseThickness) - Comma-implies-Thickness branch when targetType is null - Generic enum path via FlowDirection - Mixed-case bool parsing - 8-digit color with A=0x00 (alpha preservation) - Lowercase hex - 5-digit / empty hex rejection - Negative Thickness acceptance - Pinned discovered behavior: TryParseCornerRadius propagates ArgumentException on negatives (the Try* name is misleading; a future fix to catch + return false would be an intentional API change). All 60 tests in the file pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add 20 real tests for TeeTextWriter / BufferTraceListener / LogCaptureInstall.Install in tests/Reactor.Tests/Devtools/ LogCaptureBufferTests.cs. Each test names a product bug it would catch: stdio-MCP JSON-RPC frame corruption when forwardConsole=false is ignored, phantom blank entries from Flush() with no pending payload, CRLF leaking '\r' into entries, the char[]/char/string Write overloads each going through the wrong branch, idempotent install adding a second BufferTraceListener. Per-file: LogCaptureInstall.cs 58.8% → 100% line, 38% → ~93% branch. Merged unit+selftest: 79.50% → 79.57% line / 67.23% → 67.29% branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add 85 real tests in tests/Reactor.Tests/OwnPropsEqualTests.cs covering every switch arm of Element.OwnPropsEqual (was 19% line / 17% branch with zero direct tests, despite gating every reconcile-highlight overlay decision). Each test names a concrete product bug: stale WinUI value when own-prop change is dropped, vs highlight overlay strobing when descendant changes leak into the parent's own-props compare. Focus on decision-point variation gave a branch% swing roughly 2× the line% swing — recorded that pacing lesson in the tracking doc. Per-method: OwnPropsEqual 19% → ~100% line, 17% → ~100% branch. Merged unit+selftest: 79.57% → 79.80% line (+0.23) / 67.29% → 67.80% branch (+0.51). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 79.80% → 79.82% Add 34 real tests across DragDataTests.cs covering Phase 6b lazy providers (WithUri/WithHtml/WithRtf/WithCustomFormat × sync+async), AvailableFormats/HasFormat key-mapping, last-write-wins overwrite semantics, and the previously-untested in-memory transfer registry (Register/Resolve/Unregister) used by same-process DnD. Also add a new DragDataFormatEntryTests class pinning the resolve state machine: HasEager invariant, eager > async > sync precedence in ResolveAsync, eager > sync precedence in ResolveSync, the critical UI-thread no-block contract (ResolveSync on async-only must return null without invoking the provider — a regression to .GetAwaiter().GetResult() here would freeze the dispatcher on every lazy-async drop). Also add tools/coverage/rank-branch-gap.ps1 — ranks files by how much branch% trails line%, the highest-ROI signal for the next iteration. Captures the heuristic from prior session. Per-file: DragData.cs 45.5% → 52.8% line, 18.9% → 27.0% branch. Merged unit+selftest: 79.80% → 79.82% line / 67.80% → 67.85% branch. TryGetSafeLocalFiles (UNC/DOS/reparse safety filter) flagged as a deferral candidate — requires IStorageItem mocks or a selftest fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…79.82% → 80.04% 66 new tests in tests/Reactor.Tests/Controls/EditorsBehaviorTests.cs cover the Editors factory catalog's pure-C# paths: numeric round-trip (15 tests across ToDouble/FromDouble switch arms), Text/CheckBox/Toggle (8), Date variants (8), Time variants (8), Uri (6), Combo/EnumCombo (8), Color (2). Each test invokes the factory and exercises OnXxx callbacks on the returned Element record — no WinUI activation required. Per-file Editors.cs: 29.7% → 67.6% line. Merged: 79.82% → 80.04% line (+0.22), 67.85% → 68.24% branch (+0.39). Branch swing ≈ 2× line — the catalog is a branch-shaped target (large switch arms). ColorCompact paths are not unit-reachable: `.Background(hex)` builds a WinUI SolidColorBrush eagerly. Deferral candidate flagged in status log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… — 80.04% → 80.10% 12 new tests in tests/Reactor.Tests/RenderContextHookOrderTests.cs covering the loud-failure paths that HookStateRefactorTests didn't reach: - HookOrderException at UseReducer<T>, UseReducer<TState,TAction>, UseMemo<T>, and UseRef<T> slots (7 tests) - UseStateSetterByIndex happy / out-of-range / type-mismatch (3 tests) - UseColorScheme + UseIsDarkTheme null-Application paths (2 tests) Merged: 80.04% → 80.10% line (+0.06), 68.24% → 68.29% branch (+0.05). Doc updates: TreeChartsTests vanity audit entry (~20 Same(chart, result) fluent-return tests flagged, kept until a selftest fixture can replace them); deferral-candidate proposal for ~1,639 lines of inherently host-bound code (PreviewCaptureServer, JumpListComInterop, ChartAutomationPeer, TrayFlyoutHostWindow, TaskbarOverlay) — pending explicit user approval per doc's no-vanity rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
26 new tests in tests/Reactor.Tests/Controls/TypedColumnsBehaviorTests.cs replace the Assert.NotNull(factory) bar with invoke-and-assert: - TypeRegistry resolutions for 9 primitive types (DateTime, DateTimeOffset, DateOnly, TimeSpan, TimeOnly, Uri, Color, Bool-Standard, Bool-Compact) - ReflectionTypeMetadataProvider [DataType.Url] branches (string vs Uri) - TypedColumns factories: NumberColumn int/decimal/long, CheckBox vs ToggleSwitch, DateColumn for all 3 type-switch arms (the DTO + DateOnly arms were previously uncovered), TimeColumn TimeSpan vs TimeOnly, HyperlinkColumn Uri vs string (string arm was uncovered), ComboBoxColumn typed onChange, ColorColumn wiring. Surfaced contract pin: [DataType(Url)] on string property commits string (the Hyperlink display happens at render-time via CellRenderers.Hyperlink, not by routing through Editors.Uri). Pinned faithfully. Merged: 80.10% → 80.14% line (+0.04), 68.29% → 68.34% branch (+0.05). Small because TypedColumns is a thin wrapper over Editors (already high coverage); the real value is the regression net for layer-boundary wiring that NotNull-only tests silently passed through. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…% → 80.18% 24 new tests in tests/Reactor.Tests/Controls/CellRenderersTests.cs cover the pure-C# renderers: Text / Number / Date / Time / Enum / Hyperlink. Brush-shaped renderers (CheckMark, ToggleIndicator, ColorSwatch) build SolidColorBrush eagerly via .Foreground/.Background and are documented as unit-unreachable (same trap as Editors.ColorCompact in iteration 4). Hyperlink's 3 if-chain branches (Uri → button, parseable string → button, non-Uri string → TextBlock fallback) are each pinned. Bug shape: a regression that emitted HyperlinkButton with null NavigateUri would crash WinUI on click. Surfaced product sharp edge: CellRenderers.Time's default "t" format throws FormatException when value is TimeSpan (TimeSpan custom formats need escaped colons: `hh\:mm\:ss`). Documented as a deferred follow-up in the status log; not fixing the product here. Merged: 80.14% → 80.18% line (+0.04), 68.34% → 68.41% branch (+0.07). Branch swing > line swing — the Hyperlink if-chain coverage is the win. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
19 new tests covering JsonFileStore + PackagedSettingsStore failure paths: JsonFileStore (+13): - Write empty-id / null-data / empty-id-read guards (3) - Per-entry corruption: malformed base64 (FormatException catch), non-string entry, non-object root, empty-string entry (4) - Merge-over-tampered-file with array root (1) - AppendQuotedString escape arms: quote, backslash, \n+\t, control chars below 0x20 (Unicode escape), \r+\b+\f (5) PackagedSettingsStore (+6, new file): - Early-return guards on empty id and null data (3) - WinRT-unavailable catch arms for TryRead and Write — in xUnit's unpackaged context, ApplicationData.Current throws but the catch must swallow (2) - IsAvailable() returns false in unpackaged context without throwing (1) Merged: 80.18% → 80.26% line (+0.08), 68.41% → 68.50% branch (+0.09). Best yield since iteration 4 — PackagedSettingsStore was at 0% with no incidental coverage to share, and JsonFileStore's catch arms are branch-dense. Status log surfaces an insight: "host-bound" sometimes means "WinUI XAML controls only" — pure WinRT projections that throw deterministically in unpackaged context (like ApplicationData.Current) are still unit-testable for their catch-arm contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
17 new tests across two new files, revisiting two of the previously-deferred shell-API files with the catch-arm lesson from iteration 8 (WinRT projections that throw deterministically in unpackaged xUnit are still unit-testable for their error-handling contract). TaskbarOverlayTests (+9): - Constructor + Icon/AccessibleDescription getter/setter round-trips - Setters after dispose are silent no-ops via the _isDisposed() guard - Setters when live but TaskbarComSingleton.TryGet() is null also no-op - LoadIconFor private static via reflection: null icon → 0, IsResource icon → 0 (ms-appx:/// can't become HICON), missing file path → 0 without throw JumpListUpdateValidationTests (+8): - UpdateAsync null/per-entry validation: null collection, null entry, empty title on task/custom (separator with empty title permitted) - Unpackaged AppUserModelId gate throws InvalidOperationException - ClearAsync inherits the same gate (no special-case for empty input) - Valid items + AppUserModelId set: Task completes without throwing even when the inner COM call fails (catch swallows) Renamed to avoid collision with existing JumpListStateTests class that covers basic static state round-trip. Merged: 80.26% → 80.55% line (+0.29), 68.50% → 68.70% branch (+0.20). Strongest single-iteration jump since iteration 4 — TaskbarOverlay was at 0% so every test net-new covered lines, plus JumpList's async validation paths carry ~30 previously-uncovered lines. Pacing note in status log: PreviewCaptureServer is the best remaining catch-arm scout from the deferral cluster; TrayFlyoutHostWindow and ChartAutomationPeer remain genuinely host-bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
17 new tests in tests/Reactor.Tests/PreviewCaptureServerTests.cs cover
the security-critical helpers reachable via reflection:
- GenerateToken (2): 43-char url-safe base64, RNG uniqueness across calls
- IsAllowedOrigin (Theory × 13): all accept/reject arms
- ReadCappedBody (4): TASK-023 4 MB cap enforcement
- AcquireFreePortHolding (1): loopback bind
- BearerMatches (6): constant-time XOR comparison contract, missing
prefix rejection, wrong-length early-out, leading-whitespace trim
- IsAllowedHost (5): TASK-020 DNS-rebinding defense
Surfaced a real product security finding: IsAllowedOrigin uses
StartsWith("http://localhost", OrdinalIgnoreCase) which matches
http://localhost.evil.com. Currently backstopped by the IsAllowedHost
host-header check (the actual security fence), but the advisory CORS
allow-list is over-broad. Test
IsAllowedOrigin_StartsWith_Has_Known_Subdomain_Gap pins current
behavior so a tightening fix reverts predictably. Filed as deferred
follow-up in status log.
Merged: 80.55% → 80.68% line (+0.13), 68.70% → 68.85% branch (+0.15).
Hand-off note: the catch-arm scout across deferred files is now
thorough. Remaining 0% files (TrayFlyoutHostWindow, ChartAutomationPeer,
rest of PreviewCaptureServer's HTTP machinery, JumpListComInterop) are
genuinely host-bound. Next big metric move requires either the
deferral approval (~1.6%) or selftest fixtures for Reconciler.Mount /
Reconciler.Update.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…80.70% 10 new tests strengthen the catch-arm + helper coverage of LayoutEtwConsumer without standing up a real ETW session: - Initial state: counters zero, IsRunning/IsUnavailable false (1) - Public constants: SessionNamePrefix + XamlProviderGuid (2) - Stop without Start + Dispose idempotency (2) - IsProcessAlive private static via reflection: non-positive PID early-out, current process true, bogus high PID falls through to the ArgumentException catch (5) Merged: 80.68% → 80.70% line (+0.02), branch -0.02 from selftest noise on platform-bound ETW branches. Smallest yield this session — the unit-test runway is genuinely tapped on this file. Status log hand-off: session ran 79.82% → 80.70% (+0.88%) across 8 iterations. The remaining 4.30 pts to 85% requires either deferral approval (~1.2 pts immediately) or selftest fixtures for Reconciler .Mount/.Update (each fixture adds 30-100 covered lines). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
45 new tests targeting the pure-C# core of PropertyGridComponent.cs: PropertyGridDefaultsTests.cs (14, picked up from prior session): - PropertyLabelTemplate: DisplayName/Name fallback, AutomationName prefix, indent×4 margin, tooltip propagation - PropertyRowTemplate: FlexRow shape, Editor: prefix, indent×16 padding - ArrayItemTemplate: expand glyph ▶/▼, toggle inversion, [N] bracket format, 3/4/6-child action branches, ✕ callback wiring EditChainTests.cs (31, new): - BuildPath: empty-chain + multi-level dot-joining (pin: regression to "/" breaks every saved expand-state key) - CannotPropagate (6 branches): direct SetValue short-circuit; Compose-less + no-callback → true; OnRootChanged-only → false; root Compose-only → false; chain-entry SetValue makes leaf editable; mid-chain SetValue=null+Compose=null terminates true - PropagateNewOwner (4): empty path → OnRootChanged; mutable ancestor SetValue absorbs and stops (pin: no redundant root reassignment); multi-level immutable Compose chain to root; dead-end entry silently drops - PropagateImmutableEdit (2): mutable-ancestor SetValue absorbs Compose'd child (lines 379-384); no-chain + no-root-Compose silently drops without NRE - RenderReadOnlyValue via reflection (6): bool→ToggleSwitch disabled; bool null→IsOn=false; string→TextField disabled; string null→""; other→TextBlock.ToString(); other null→"(null)" - IsPrimitiveOrEnum via reflection (Theory×11): primitives, string, decimal, enum = true; class/record = false Merged: 80.70% → 80.79% line (+0.09), 68.83% → 69.00% branch (+0.17). Branch swing 1.9× line — branch-shaped-target heuristic holds again. 8,053/8,099 unit suite pass (one transient WindowPersistedScopeIsolation flake on first run; passes on retry, unrelated to this iteration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both PreviewCaptureServer.IsAllowedOrigin and DevtoolsMcpServer.IsAllowedOrigin used origin.StartsWith( "http://localhost", …) to gate the advisory CORS allow-list. That matched http://localhost.evil.com because the malicious host genuinely starts with "localhost"; same shape for http://127.0.0.1.evil.com. The host-header fence in IsAllowedHost backstopped the actual security boundary, but the advisory CORS layer was over-broad against direct browser-driven attacks. Switched both to Uri.TryCreate + uri.Host exact-match. System.Uri decomposes scheme/host/port per RFC 3986, so localhost.evil.com parses as Host="localhost.evil.com" and fails the equality check. Allow-list shapes preserved: - PreviewCaptureServer accepts vscode-webview, http://127.0.0.1, http://localhost, https://localhost - DevtoolsMcpServer accepts vscode-webview, http://127.0.0.1, http://localhost (no https — MCP traffic is HTTP only) Tests added/expanded: - PreviewCaptureServerTests: flipped the gap-pin test (was asserting http://localhost.evil.com WAS accepted) into rejection theory rows; added boundary-delimiter accepts (trailing /, ?, #) and userinfo- trick rejects (http://localhost@evil.com) - DevtoolsMcpServerOriginTests (new): sister coverage with the same suffix-attack rejection set, plus pins for the https-rejected contract distinct from PreviewCaptureServer Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… parse The original comment claimed Uri.TryCreate was "too lenient" because it parses `javascript:alert(1)` as a valid Uri. That's true, but the SafeUrlSchemes allow-list filter on uri.Scheme runs *after* parsing and rejects `javascript` (and every other non-http/https/mailto scheme) the same way. So the System.Uri version is equally safe and avoids the hand-rolled RFC 3986 scheme-character validator that was the actual maintenance risk. Behavior preserved: - http/https/mailto pass through unchanged (allow-list, case-insensitive) - javascript:/data:/vbscript:/file:/ftp:/ssh: → "about:blank" - Relative URLs (no scheme) pass through unchanged - Empty input passes through unchanged - unsafeAllowed=true bypasses the filter entirely Tests added (new file, 25 tests): TASK-045 XSS-fence had zero direct coverage before this commit. The theory rows pin each known XSS-vector scheme and the case-insensitive normalization contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR raises Reactor test coverage while tightening URL/origin validation in Markdown rendering, PreviewCaptureServer, and Devtools MCP CORS handling.
Changes:
- Adds coverage helper scripts and documentation for repeatable merged coverage runs and gap reporting.
- Adds broad unit coverage across devtools, persistence, drag data, property grid/editors, shell helpers, ETW, Markdown URL sanitization, and CORS origin helpers.
- Replaces prefix-based origin checks and Markdown URL scheme parsing with
System.Uri-based parsing.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Keeps generated coverage output ignored while allowing tracked coverage tooling. |
src/Reactor/Hosting/Devtools/DevtoolsMcpServer.cs |
Updates MCP CORS origin validation to parse origins with Uri. |
src/Reactor/Hosting/PreviewCaptureServer.cs |
Updates preview CORS origin validation to parse origins with Uri. |
src/Reactor/Markdown/MarkdownHtml.cs |
Switches Markdown URL sanitization to Uri.TryCreate plus scheme allow-list. |
tests/Reactor.Tests/Controls/CellRenderersTests.cs |
Adds tests for text/number/date/time/enum/hyperlink cell renderers. |
tests/Reactor.Tests/Controls/EditChainTests.cs |
Adds tests for PropertyGrid edit propagation and read-only rendering helpers. |
tests/Reactor.Tests/Controls/EditorsBehaviorTests.cs |
Adds behavior tests for editor factories and value conversions. |
tests/Reactor.Tests/Controls/PropertyGridDefaultsTests.cs |
Adds tests for default PropertyGrid templates. |
tests/Reactor.Tests/Controls/TypedColumnsBehaviorTests.cs |
Adds typed column/editor/renderer wiring tests. |
tests/Reactor.Tests/Devtools/DevtoolsMcpServerOriginTests.cs |
Adds MCP origin allow-list/security regression tests. |
tests/Reactor.Tests/Devtools/DevtoolsPropertyToolTests.cs |
Adds parsing/formatting edge coverage for devtools property tools. |
tests/Reactor.Tests/Devtools/LogCaptureBufferTests.cs |
Expands log capture, console tee, and install/reset coverage. |
tests/Reactor.Tests/DragDataTests.cs |
Adds lazy provider, format entry, and transfer registry tests. |
tests/Reactor.Tests/Hosting/Etw/LayoutEtwConsumerTests.cs |
Adds initial state, constants, stop/dispose, and process-liveness coverage. |
tests/Reactor.Tests/JsonFileStoreTests.cs |
Adds persistence guard, malformed data, merge, and JSON escaping tests. |
tests/Reactor.Tests/JumpListUpdateValidationTests.cs |
Adds JumpList validation and unpackaged guard tests. |
tests/Reactor.Tests/Markdown/SanitizeUrlTests.cs |
Adds Markdown URL scheme allow-list tests. |
tests/Reactor.Tests/OwnPropsEqualTests.cs |
Adds broad Element.OwnPropsEqual switch-arm coverage. |
tests/Reactor.Tests/PackagedSettingsStoreTests.cs |
Adds packaged settings fallback/error handling tests. |
tests/Reactor.Tests/PreviewCaptureServerTests.cs |
Adds preview server helper and security validation tests. |
tests/Reactor.Tests/RenderContextHookOrderTests.cs |
Adds hook order and devtools state setter tests. |
tests/Reactor.Tests/TaskbarOverlayTests.cs |
Adds TaskbarOverlay setter/dispose/helper tests. |
tools/coverage/README.md |
Documents local coverage helper workflow. |
tools/coverage/rank-branch-gap.ps1 |
Adds branch-gap ranking helper. |
tools/coverage/report-gaps.ps1 |
Adds merged Cobertura gap report generator. |
tools/coverage/run-coverage.ps1 |
Adds one-shot merged unit+selftest coverage runner. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/27 changed files
- Comments generated: 5
Five fixes from the Copilot review of the URI-refactor + coverage PR: 1. PreviewCaptureServer.IsAllowedOrigin (real regression): the Uri-based refactor accepted any http-or-https + (localhost or 127.0.0.1). The pre-refactor allow-list rejected `https://127.0.0.1` (only `http://127.0.0.1` was accepted; `https://` was reserved for `localhost`). Restored the shape-specific check + added explicit rejection theory rows for `https://127.0.0.1[:port]`. 2. CellRenderersTests.FormatValue_Invariant_Format_With_Decimal: `"R"` format is documented for Single/Double/Half, not decimal — behavior is implementation-defined and has historically thrown FormatException. Switched to `"G"` which is explicitly decimal- supported, same observable output ("1.5"). 3. EditorsBehaviorTests.DateOnly_Null_Defaults_To_Today: previous test would flake if midnight crossed between the factory call and the expected-value evaluation. Now captures `DateTime.Today` both before AND after, accepting either as a valid match. 4. EditorsBehaviorTests.Uri_OnChange_Truly_Invalid_String_Does_Not_Commit: the source string had an embedded raw NUL byte (0x00) in addition to the visible `\x01` escape. Replaced the raw NUL with an explicit `\x00` escape so the test input is visible in source and stable across editors. 5. JsonFileStoreTests.Write_Id_With_Control_Char_Below_0x20_Uses_Unicode_Escape: identifier literal had embedded raw SOH (0x01) and US (0x1F) bytes. Replaced with explicit `\x01` / `\x1F` escapes for the same visibility reason. 210 affected tests pass. Co-Authored-By: Claude Opus 4.7 (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
Two things land here:
Reactor.dll. Started at 79.52% line / 67.25% branch; this PR lands at 80.79% line / 69.00% branch (+1.27 pts line, +1.75 pts branch) across 12 focused iterations.System.Uri-based parsing. The audit started from a real CORS suffix-attack found while writing the coverage tests; surfacing it surfaced two more.Security fix detail
Issue 1 + 2: CORS allow-list suffix attack (FIXED)
PreviewCaptureServer.IsAllowedOriginandDevtoolsMcpServer.IsAllowedOriginboth used:That matches
http://localhost.evil.combecause the malicious host string genuinely starts withlocalhost. Same shape forhttp://127.0.0.1.evil.com. The host-header check inIsAllowedHostwas the actual security fence, but the advisory CORS layer was over-broad against direct browser-driven attacks.Fix:
Uri.TryCreate(origin, UriKind.Absolute, out var uri)+ exact-match onuri.Host.System.Uridecomposes scheme/host per RFC 3986, solocalhost.evil.comparses asHost = "localhost.evil.com"and fails the equality check.Allow-list shapes preserved between the two servers (
DevtoolsMcpServeris HTTP-only;PreviewCaptureServeralso acceptshttps://localhost).Issue 3: MarkdownHtml.SanitizeUrl manual scheme parser (FIXED)
XSS fence on Markdown-emitted
href/srcattributes. Original code hand-rolled the RFC 3986 scheme grammar (ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )) because the author thoughtUri.TryCreatewas "too lenient — it would parsejavascript:alert(1)as a valid Uri." That's literally true, but the allow-list filter onuri.Schemeruns after parsing and rejectsjavascriptthe same way. Net effect: same behavior, less hand-rolled parsing surface.Allow-list:
http,https,mailto. Everything else →about:blank. Tests pin:javascript:,JAVASCRIPT:,data:text/html,…,data:image/svg+xml,…,vbscript:,file://,ftp://,ssh://, and case-insensitive variants.Coverage uplift detail
~440 new tests across 17 test files in
tests/Reactor.Tests/, each targeting real product behavior (every assertion names a concrete bug it would catch — noSetters.Count == 1/Assert.NotNull(factory)vanity).Tooling:
tools/coverage/run-coverage.ps1(one-shot wrapper for the CONTRIBUTING.md recipe),tools/coverage/report-gaps.ps1(ranked gap table; recomputes branch% from per-line condition-coverage since dotnet-coverage hard-codes the cobertura root),tools/coverage/rank-branch-gap.ps1(surfaces files where branch% trails line% by the widest margin).Tracking doc:
docs/reports/coverage-uplift-85.md— append-only status log of every iteration. Self-contained so any agent on any machine can pick up cold.What got covered
Other notable findings surfaced by the audit-first workflow
CellRenderers.Time(format = "t"): the default"t"is not a valid TimeSpan format string — callingTime()on a TimeSpan column would throw at render time. Pinned the workaround (hh\:mm\:ss); filed product-side follow-up.TryParseCornerRadiuspropagatesArgumentExceptionwhen components are negative (WinUI's CornerRadius ctor validates). TheTryParse*name is misleading. Test pins current contract; future fix to catch + return false would be an intentional API change.SetExtensionsCoverageTests.cs(~345 lines of.Set(...).Setters.Count == 1patterns),TypedEditorsTests.cs(Assert.NotNull(factory)against a delegate that can't be null), andTreeChartsTests.cs(Assert.Same(chart, chart.Foo(...))over fluent setters) are documented in the tracking doc with replacement strategies. No deletions in this PR — replacements need selftest fixtures.Deferral candidates still pending user approval
Per the doc's "no vanity coverage, including no excluding-just-for-the-metric" rule, no
[ExcludeFromCodeCoverage]was applied in this PR. The following ~1,228 lines are documented as genuinely host-bound and would jump the metric ~1.2 pts when approved:PreviewCaptureServer's remaining HTTP-listener machinery (~390 lines after this PR's helper coverage)JumpListComInterop.csCOM body (~300 lines)ChartAutomationPeer.cs(308 lines)TrayFlyoutHostWindow.cs(230 lines)Not yet at 85%
Stopping at 80.79% rather than letting commits trickle indefinitely. The doc's hand-off log explains the remaining 4.21-pt gap and the next-session triage path.
Test plan
WindowPersistedScopeIsolationTestsflake on the parallel run that passes on retry (pre-existing — seetools/flake-loop.ps1+.flake-runs/tracking)tools/coverage/run-coverage.ps1produces merged.cobertura.xmltools/coverage/report-gaps.ps1regeneratescoverage/gap-report.mdhttp://localhost.evil.com,http://127.0.0.1.evil.com, userinfo tricks (http://localhost@evil.com), suffix-with-hyphen (http://localhost-evil.com), and the canonical XSS schemes (javascript:,data:text/html,…,vbscript:,file://).🤖 Generated with Claude Code