microsoft / microsoft/microsoft-ui-reactor

spec 044 audit: all eight cited migration commit SHAs are dangling

Open
#975 0 comments 0 reactions 0 assignees View on GitHub
documentation
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

Split out of #959 / #973, where it was found but deliberately left out of scope.

## The problem

`docs/specs/044/swallowed-error-audit.md` says of its per-file entries:

> Each entry also names the migration commit so the verdict is auditable against the working code.

That claim is currently unbacked. **Every one of the eight cited commit SHAs is dangling** — all fail `git rev-parse`:

| SHA | Cited for |
|---|---|
| `7312ce73` | `ReactorWindow.cs` COM-teardown narrowing |
| `e2a755b2` | `WindowPlacementCodec.cs` |
| `21e22e1c` | `JsonFileStore.cs` |
| `054c53ef` | `PackagedSettingsStore.cs` |
| `21cd6ef9` | `RenderContext.cs` |
| `90d516b0` | `Reconciler.cs` |
| `b761a7a1` | Shell / `HResultFailed` |
| `301593bc` | `ConnectedAnimation` |

They are almost certainly pre-squash SHAs from PR branches: this repo squash-merges, so the commit that actually landed has a different hash and the original is unreachable once the branch is deleted.

## Why it matters

The SHA is the *only* link from a verdict to the change that implemented it. With all eight dangling, a reviewer who wants to check "was this catch really narrowed the way the row claims?" has nothing to follow — the audit is self-referential. That was one of the conditions that let the counts in #959 drift unnoticed for as long as they did.

## Suggested fix

Re-resolve each to a durable reference and swap it in. Options, best first:

1. **PR number** (`#123`) — survives squash, renders as a link in GitHub, and gets you the review discussion as well as the diff. Preferred.
2. **The squashed commit SHA on `main`** — durable, but less context than the PR.

Recovery path for each: search `main`'s history for the file + change (`git log --oneline --follow -- `, or `git log --grep`), or find the PR that introduced the narrowing. Cross-check the resulting diff actually matches what the row claims before swapping the reference in — a wrong-but-resolvable SHA is worse than a dangling one.

## Consider gating it

`tests/Reactor.Tests/Docs/SwallowedErrorAuditTests.cs` (added in #973) already parses every ledger row. A tenth assertion could extract any `[0-9a-f]{7,40}` in a `Notes` cell and fail if `git rev-parse --verify ^{commit}` doesn't resolve — which would have caught this class at the point of introduction.

Two caveats worth thinking about before adding it:

- It would need to be a **no-op outside a git checkout** (shallow clones, source packages) rather than a failure.
- If the fix standardises on PR numbers instead of SHAs, the gate should assert the *reference format* rather than shelling out to git at all — cheaper, hermetic, and it can't be defeated by a shallow clone.

## Scope

Mechanical but not trivial: eight lookups, each needing verification that the recovered commit matches the claimed verdict. No product code. Independent of #973 — that PR fixes the counts and the structure; this fixes the provenance links.

Contributor guide

Open the contributing guide

Research direction

Start with docs/specs/044/swallowed-error-audit.md and use git log --follow or git log --grep to trace each of the eight cited changes, checking that the recovered reference matches its ledger row. Done means every citation resolves to a durable PR or main-branch commit; review tests/Reactor.Tests/Docs/SwallowedErrorAuditTests.cs if adding the proposed provenance check.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp, git
Domain
documentation, testing-qa
Issue type
Documentation
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.