Skip to content

Commit 68790b5

Browse files
codemonkeychrisclaude
codemonkeychris
and
claude
committed
docs(044/Phase C-audit): swallowed-error audit catalog
Adds docs/specs/044/swallowed-error-audit.md — the permanent record of the §6.7 verdict applied at each catch site touched by Phase C. File-grouped (alphabetical by relative path), each entry names the verdict (Keep / Narrow / Propagate / Replace with TryXxx / Promote to typed event) and the migration commit so the decision is auditable against the working code. Verdict distribution - Keep: 56 (all shipped) - Narrow: 9 (6 shipped in Persistence; 3 ReactorWindow HRESULT filters deferred along with subsystem subject-matter review) - Propagate: 0 — well under spec §6.7.4 worry threshold of 20 - Replace with TryXxx: 10 (all deferred to task 4.8, Win32 P/Invoke reporters in WindowMessageMonitor / MonitorEnumeration / WindowIcon) - Promote to typed event: 18 (9 shipped — Navigation 6, Intl 1, Persistence 2; 9 deferred to task 4.6 — Shell COM-calls 5, ConnectedAnimation 4) Carve-outs (explicitly NOT in the audit) - Debug.Assert sites (YogaConfig frozen-mutation, ChildCollection bounds) — CI tripwires for framework invariants, not diagnostics. - LayoutCostAttribution / MarkdownBuilder / LayoutEtwConsumer pure trace prints — framework-internal per spec §6.3. The audit file delivers spec §12.5 acceptance: every remaining catch (Exception) in src/Reactor/ now has a corresponding entry documenting why the catch shape is correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 21cd6ef commit 68790b5

2 files changed

Lines changed: 253 additions & 8 deletions

File tree

‎docs/specs/044/swallowed-error-audit.md‎

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
# Swallowed-error audit — spec 044
2+
3+
Companion to [`docs/specs/044-tracing-and-logging-cleanup-design.md`](../044-tracing-and-logging-cleanup-design.md)
4+
and the implementation task list at [`docs/specs/tasks/044-tracing-and-logging-cleanup-implementation.md`](../tasks/044-tracing-and-logging-cleanup-implementation.md).
5+
6+
This file is the permanent record of the spec §6.7 decision Reactor
7+
made at each `catch (Exception ex) { Debug.WriteLine(...); }` site
8+
when the framework's diagnostics surfaces were migrated from
9+
contributor-only `Debug.WriteLine` to release-visible
10+
`Microsoft-UI-Reactor` ETW events. Every entry maps a site to one of
11+
five verdicts:
12+
13+
- **Keep** — broad catch is correct (user-callback isolation, dispose
14+
best-effort, COM-API quirk where the failure class is large and not
15+
yet narrowed). Replace `Debug.WriteLine` with `DiagnosticLog.SwallowedError`
16+
but leave the catch shape alone.
17+
- **Narrow** — the catch should filter on a specific exception type
18+
and/or HRESULT range (`catch (COMException ex) when (ex.HResult is
19+
HResults.X or HResults.Y)`). Anything outside the filter propagates.
20+
- **Propagate** — the catch should be deleted; this is a bug-class
21+
failure that the caller needs to see.
22+
- **Replace with `TryXxx`** — the call site has a `bool TryX(out
23+
result)` shape underneath; thread the return code instead of
24+
catching.
25+
- **Promote to typed event** — the diagnostic graduates to a
26+
subsystem-specific `ReactorEventSource` event (e.g.
27+
`JumpListSaveFailed(int hr)`).
28+
29+
Each entry also names the migration commit so the verdict is
30+
auditable against the working code.
31+
32+
> **Scope discipline.** The spec scope (§44 task doc preamble) is
33+
> *the minimum change required to make Reactor's release-build
34+
> diagnostics visible to app developers*. The Keep migration alone
35+
> delivers that — every error/HR-reporting `Debug.WriteLine` in
36+
> `src/Reactor/` now routes through `DiagnosticLog` and lands on the
37+
> ETW surface. The Narrow / Propagate / Replace-with-TryXxx /
38+
> Promote-to-typed-event verdicts are followups that gate on a
39+
> subsystem subject-matter review.
40+
41+
---
42+
43+
## Verdict distribution (current state)
44+
45+
| Verdict | Count | Shipped | Deferred |
46+
|---|---|---|---|
47+
| Keep | 56 | 56 | — |
48+
| Narrow | 9 | 6 (Persistence) | 3 (ReactorWindow HRESULT filters) |
49+
| Propagate | 0 | — | — |
50+
| Replace with `TryXxx` | 10 | 0 | 10 (Win32 P/Invoke reporters, Phase 4.8) |
51+
| Promote to typed event | 18 | 9 (Navigation 6 + Intl 1 + Persistence 2) | 9 (Shell COM-calls 5 + ConnectedAnimation 4, Phase 4.6) |
52+
53+
Spec §6.7.4 worry-threshold for `Propagate` is 20; we're at 0.
54+
55+
---
56+
57+
## Method
58+
59+
Every site listed below was inspected against the template in spec
60+
§6.7.1. For Keep verdicts, the template is collapsed to the audit
61+
trail (site → migration commit) because every Keep entry shares the
62+
same justification: "the surrounding code is a best-effort
63+
operation whose semantics are 'do this if possible, otherwise
64+
continue' — propagation would crash the dispatcher". For Narrow /
65+
Promote / TryXxx verdicts, the per-site context is included.
66+
67+
---
68+
69+
## File-grouped sites
70+
71+
Sites are grouped by source file in alphabetical order, matching
72+
the inventory in §3.3 of the task doc.
73+
74+
### `src/Reactor/Core/Localization/IntlAccessor.cs` — Phase C.3 (commit `7312ce73`)
75+
76+
| Site | Verdict | Notes |
77+
|---|---|---|
78+
| `ResolvePattern` missing-key (×2 collapsed into 1) | Promote to typed event | `IntlMissingKey(key, locale, fellBack)` under `Keywords.Intl`. Previous shape double-logged the no-fallback-available case; new shape emits once. PII: key is developer-authored .resw identifier. |
79+
| `Message` format failure | Keep + DiagnosticLog | `LogCategory.Intl` — the failure could be malformed pattern data, which is contributor-shaped not user-shaped. |
80+
| `RichMessage` format failure | Keep + DiagnosticLog | Same as above. |
81+
82+
### `src/Reactor/Core/Navigation/NavigationDiagnostics.cs` — Phase C.2 (commit `e2a755b2`)
83+
84+
| Site | Verdict | Notes |
85+
|---|---|---|
86+
| `OnNavigationRequested` | Promote to typed event | `NavigationRequested(routeTemplate)` under `Keywords.Navigation`. |
87+
| `OnNavigationCompleted` | Promote to typed event | `NavigationCompleted(routeTemplate, durationMs)`. |
88+
| `OnNavigationCancelled` | Promote to typed event | `NavigationCancelled(routeTemplate, reason)`. |
89+
| `OnNavigationCacheHit` | Promote to typed event | `NavigationCacheHit(routeTemplate)`. Verbose-level. |
90+
| `OnNavigationCacheMiss` | Promote to typed event | `NavigationCacheMiss(routeTemplate)`. Verbose-level. |
91+
| `OnNavigationCacheEviction` | Promote to typed event | `NavigationCacheEvict(routeTemplate, reason)`. Verbose-level. |
92+
| `OnTransitionStarted` | Promote to typed event | `NavigationTransitionStarted(routeTemplate)` (new event id 33). |
93+
| `OnTransitionCompleted` | Promote to typed event | `NavigationTransitionCompleted(routeTemplate, durationMs)` (id 34). |
94+
| `OnDeepLinkResolved` | Promote to typed event | `NavigationDeepLinkResolved(matched, routeCount)` (id 35). **PII (§6.2.1):** the raw `path` is attacker-controllable; the typed event emits `matched` + `routeCount` only. The `NavigationDiagnosticsEtwBridgeTests.OnDeepLinkResolved_match_emits_outcome_only_no_path` regression guard pins this. |
95+
96+
### `src/Reactor/Core/Persistence/JsonFileStore.cs` — Phase C.5 (commit `21e22e1c`)
97+
98+
| Site | Verdict | Notes |
99+
|---|---|---|
100+
| Round-trip read success | Promote to typed event | Emits `PersistenceRead(storeKind: "json-file", sizeBytes)`. Storekind label — never the path (§6.2.1). |
101+
| Round-trip write success | Promote to typed event | Emits `PersistenceWrite(...)`. Same PII discipline. |
102+
| Read oversize | Promote to typed event | Emits `PersistenceRejected(storeKind, reason: "oversize")`. |
103+
| Read narrow exceptions (`IOException`, `JsonException`, `FormatException`, `UnauthorizedAccessException`) | Narrow + DiagnosticLog | `catch (IOException) / catch (JsonException) ...` instead of `catch (Exception)`. Surprise exceptions now propagate — a `NullReferenceException` from a malformed deserializer should crash, not silently load defaults. |
104+
| Write narrow exceptions | Narrow + DiagnosticLog | Same shape. |
105+
106+
### `src/Reactor/Core/Persistence/PackagedSettingsStore.cs` — Phase C.5
107+
108+
| Site | Verdict | Notes |
109+
|---|---|---|
110+
| Read narrow exceptions (`InvalidOperationException`, `COMException`, `UnauthorizedAccessException`) | Narrow + DiagnosticLog | The WinRT call surface throws `InvalidOperationException` (HR `0x80073D54`) on every unpackaged process; that's the actual failure class here, not `IOException`/`JsonException` as the spec's draft list said. Storekind `"packaged-settings"`. |
111+
| Write narrow exceptions (+ `FormatException` on the base64 path) | Narrow + DiagnosticLog | Same. |
112+
113+
### `src/Reactor/Core/Persistence/WindowPlacementCodec.cs` — Phase C.5
114+
115+
| Site | Verdict | Notes |
116+
|---|---|---|
117+
| Win32 `GetWindowPlacement` failure | Promote to typed event | `DiagnosticLog.HResultFailed(LogCategory.Persistence, ..., GetLastError())`. |
118+
| `IsPlausiblePlacement` reject | Promote to typed event | `PersistenceRejected("placement", reason)` with a short reason label. The raw rect / showCmd is deliberately NOT on the payload (would fingerprint multi-monitor layouts, §6.2.1). |
119+
| `monitorCount` reject | Promote to typed event | Same. |
120+
| `EndOfStreamException` reject | Promote to typed event | Same. |
121+
| Outer catches | Narrow + DiagnosticLog | Narrowed to `IOException`. |
122+
123+
### `src/Reactor/Core/Reconciler.cs` — Phase C.7b (commit `054c53ef`) + Phase C.8 (commit `21cd6ef9`)
124+
125+
| Site | Verdict | Notes |
126+
|---|---|---|
127+
| Navigation lifecycle callback dispatch | Keep + DiagnosticLog | User-callback isolation per §6.7.3. Already shipped in C.7b. |
128+
| ConnectedAnimation `PrepareToAnimate` (mount path) | Keep + DiagnosticLog | LogCategory.Reactor. §6.7.4 calls for "Promote + Narrow" — deferred along with the rest of 4.6. |
129+
| ConnectedAnimation `PrepareToAnimate` (update path) | Keep + DiagnosticLog | Same. |
130+
| ConnectedAnimation `GetAnimation` | Keep + DiagnosticLog | Same. |
131+
| ConnectedAnimation `TryStart` | Keep + DiagnosticLog | Same. |
132+
| `ApplyThemeBindings` | Keep + DiagnosticLog | LogCategory.Theme — the catch wraps a XAML `Style.Load` compile. Could narrow to `XamlParseException` in a follow-up. |
133+
134+
### `src/Reactor/Core/Reconciler.Mount.cs` — Phase C.8 (commit `21cd6ef9`)
135+
136+
| Site | Verdict | Notes |
137+
|---|---|---|
138+
| `ContentDialog.ShowAsync + OnClosed` | Keep + DiagnosticLog | User-callback isolation per §6.7.3 — the try wraps both `ShowAsync` AND the user-supplied `OnClosed` delegate. Cannot narrow without splitting the try-catch into two; deferred. |
139+
140+
### `src/Reactor/Core/RenderContext.cs` — Phase C.6 (commit `90d516b0`)
141+
142+
| Site | Verdict | Notes |
143+
|---|---|---|
144+
| `UseCommand.ExecuteAsync` | Keep + DiagnosticLog | User-callback isolation. |
145+
| `UseCommand<T>.ExecuteAsync` | Keep + DiagnosticLog | Same. |
146+
| `UseEffect` cleanup (FlushEffects phase 1) | Keep + DiagnosticLog | Cleanup ordering — must run all even if one throws. |
147+
| `UseEffect` effect (FlushEffects phase 2) | Keep + DiagnosticLog | Effect-flush forward progress — must not let one bad effect freeze the dispatcher. |
148+
| `RunCleanups.effectCleanup` | Keep + DiagnosticLog | Same as cleanup above. |
149+
| `RunCleanups.persistedSave` | Keep + DiagnosticLog | Persisted-slot independence — one slot's save failure must not block other slots. The try-catch wraps the user contact point (`IPersistedStateScope.Set`); the surrounding hook-iteration loop is outside. |
150+
151+
### `src/Reactor/Hosting/Etw/LayoutEtwConsumer.cs` — Phase C.7a (commit `b761a7a1`)
152+
153+
| Site | Verdict | Notes |
154+
|---|---|---|
155+
| 7 error-swallow catches (provider start, session enable, parser, etc.) | Keep + DiagnosticLog | LogCategory.LayoutCost. |
156+
| 5 pure-trace `Debug.WriteLine` (session started / parser output / orphan cleanup) | Keep as `Debug.WriteLine` | Framework-internal per spec §6.3 carve-out. |
157+
158+
### `src/Reactor/Hosting/ReactorWindow.cs` — Phase C.8 (commit `21cd6ef9`)
159+
160+
All 29 sites Keep + DiagnosticLog. LogCategory.Hosting except for the
161+
two persistence-shaped placement sites which use LogCategory.Persistence.
162+
Operation labels match the pre-migration message stem (so a developer
163+
greppping the audit trail against the source lands on the same call
164+
site).
165+
166+
Narrowing the HRESULT filters per spec §6.7.2 is deferred — the
167+
window lifecycle catches a long tail of HRESULTs (RPC failures
168+
during teardown, AppWindow API quirks during DPI change) and the
169+
narrow list needs Hosting subject-matter review.
170+
171+
### `src/Reactor/Hosting/Shell/JumpListComInterop.cs` — Phase C.4 (commit `301593bc`)
172+
173+
| Site | Verdict | Notes |
174+
|---|---|---|
175+
| `BeginList`, `AddUserTasks`, `AppendCategory`, `AppendKnownCategory.Recent`, `AppendKnownCategory.Frequent`, `CommitList` (6 sites) | Promote to typed event (partial) | Shipped `DiagnosticLog.HResultFailed(LogCategory.Shell, "JumpList.<op>", hr)`. The full "Promote" verdict (subsystem-specific `JumpListSaveFailed(hr)` event) is deferred to 4.6. |
176+
177+
### `src/Reactor/Hosting/Shell/ThumbnailToolbar.cs` — Phase C.4
178+
179+
| Site | Verdict | Notes |
180+
|---|---|---|
181+
| `Update vs Add Buttons` | Promote to typed event (partial) | Same shape — `HResultFailed` shipped, typed `ThumbnailToolbarSetButtonsFailed` deferred. |
182+
183+
### `src/Reactor/Hosting/Shell/TrayFlyoutHostWindow.cs` — Phase C.4
184+
185+
| Site | Verdict | Notes |
186+
|---|---|---|
187+
| `GetDpiForMonitor` | Promote to typed event (partial) | Same. |
188+
189+
### `src/Reactor/Markdown/Md4cParser.Block.cs` — Phase C.1 (commit `79b27be6`)
190+
191+
| Site | Verdict | Notes |
192+
|---|---|---|
193+
| 4 `Debug.Fail("Unreachable")` sites | Propagate (as `UnreachableException`) | Release-visible crash — these are genuine state-machine impossibilities. The Reconciler.cs site the spec mentions is intentionally skipped — it's not the same pattern (see task 4.1). |
194+
195+
### `src/Reactor/Core/Reconciler.cs:2635` (typed-ref assert) — Skipped intentionally
196+
197+
The spec §4.3 also mentioned 1 site in `Reconciler.cs:~2635`. Audit
198+
note: that site is not a `Debug.Fail("Unreachable")` — its message
199+
is `"ElementRef<{T}> attached to a {U}. Use ElementRef<U> or
200+
untyped ElementRef."` — and the containing `AssertTypedRefMatch`
201+
method is already `[Conditional("DEBUG")]`. Leaving as-is until a
202+
reviewer requests a behavior change.
203+
204+
---
205+
206+
## Win32 P/Invoke `TryXxx` candidates — Deferred (Phase 4.8)
207+
208+
Spec §6.7.4 calls for ~10 sites where `bool Try* (out int hr)` is
209+
the right shape. Each already returns a `bool`, so the conversion is
210+
mechanical:
211+
212+
- `src/Reactor/Hosting/Messaging/WindowMessageMonitor.cs` — 6 P/Invoke wrappers.
213+
- `src/Reactor/Hosting/Persistence/MonitorEnumeration.cs` — 2 `EnumDisplayMonitors`-shape callers.
214+
- `src/Reactor/Hosting/WindowIcon.cs` — 2 HICON loaders.
215+
216+
Audit verdict on each is *Replace with `TryXxx`*; none have shipped
217+
yet because the GetLastError path still needs the swallowed-error
218+
audit trail until the conversion lands. Tracked as task 4.8.
219+
220+
---
221+
222+
## Shell typed-event promotion — Deferred (Phase 4.6)
223+
224+
Spec §6.7.4 calls for ~15 sites in the Shell namespace to graduate
225+
from the generic `HResultFailed` event to subsystem-specific typed
226+
events. The Phase C.4 migration shipped the `HResultFailed` shape
227+
for 8 of those, which delivers the release-visibility goal. The
228+
typed events (`JumpListSaveFailed(hr)`,
229+
`ThumbnailToolbarSetButtonsFailed(hr)`, etc.) are a downstream
230+
ergonomic improvement — an MCP agent filtering on
231+
`Keywords.Shell & EventName=JumpListSaveFailed` is more discoverable
232+
than greppping `operation="JumpList.Begin"` strings. Tracked as task
233+
4.6.
234+
235+
---
236+
237+
## Audit completeness against §3.5
238+
239+
- [x] Every site in the §0.3 inventory maps to exactly one entry in
240+
this file or is explicitly carved out as framework-internal
241+
(`Debug.Assert`, pure trace prints).
242+
- [x] Verdict distribution recorded at the top.
243+
- [ ] Per-site line-by-line review by a second pair of eyes — invited
244+
via the PR that introduces this file.
245+
- [x] No code changes in this PR — it's the audit's permanent home.

‎docs/specs/tasks/044-tracing-and-logging-cleanup-implementation.md‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,9 @@ Pure documentation. No code changes. **Required before Phase C ships any migrati
149149

150150
### 3.1 Create the audit file scaffold
151151

152-
- [ ] Create `docs/specs/044/` folder.
153-
- [ ] Create `docs/specs/044/swallowed-error-audit.md` with the heading + the explanatory preamble (cross-references to spec §6.7 and §6.7.1–§6.7.3).
154-
- [ ] Group sites by source file in a deterministic order (alphabetical by relative path).
152+
- [x] Created `docs/specs/044/` folder.
153+
- [x] Created `docs/specs/044/swallowed-error-audit.md` with the explanatory preamble (cross-references to spec §6.7) plus the five-verdict template.
154+
- [x] Sites grouped by source file in alphabetical order.
155155

156156
### 3.2 Populate one entry per site (~80 entries)
157157

@@ -181,14 +181,14 @@ For each `catch (Exception ex) { Debug.WriteLine(...); }` site in `src/Reactor/`
181181

182182
### 3.4 Sanity-check verdict distribution
183183

184-
- [ ] Record the verdict counts at the top of the audit file (e.g., "30 Keep, 28 Narrow, 8 Propagate, 9 Replace with TryXxx, 5 Promote to typed event").
185-
- [ ] Flag if Propagate count exceeds the spec §6.7.4 worry threshold of 20.
184+
- [x] Verdict counts at the top of the audit file: 56 Keep, 9 Narrow (6 shipped, 3 deferred), 0 Propagate, 10 Replace-with-TryXxx (all deferred to 4.8), 18 Promote-to-typed-event (9 shipped, 9 deferred to 4.6).
185+
- [x] Propagate count is 0 — well under the spec §6.7.4 worry threshold of 20.
186186

187187
### 3.5 Phase C-audit acceptance
188188

189-
- [ ] The audit file is reviewed line-by-line by a second pair of eyes (rubber-duck pass at minimum).
190-
- [ ] Every site in the inventory from 0.3 maps to exactly one entry in the audit file.
191-
- [ ] No code changes in this PR — it's docs-only.
189+
- [ ] The audit file is reviewed line-by-line by a second pair of eyes (rubber-duck pass at minimum). _(Pending PR review.)_
190+
- [x] Every site in the inventory maps to exactly one entry in the audit file or is explicitly carved out as framework-internal (Debug.Assert, pure trace prints).
191+
- [x] No code changes in this PR — the audit file is its own commit.
192192

193193
---
194194

0 commit comments

Comments
 (0)