Skip to content

test(511): pin the measured WebView2 handle-provenance mechanism and re-scope the #511/#571 claim - #603

Merged
drmoisan merged 7 commits into
mainfrom
bug/winformspumphost-suite-determinism-511-exec
Aug 24, 2026
Merged

test(511): pin the measured WebView2 handle-provenance mechanism and re-scope the #511/#571 claim#603
drmoisan merged 7 commits into
mainfrom
bug/winformspumphost-suite-determinism-511-exec

Conversation

@drmoisan

@drmoisan drmoisan commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Suggested title

test(511): pin the measured WebView2 handle-provenance mechanism and re-scope the #511/#571 claim

Summary

Why

The original plan rested on a premise that measurement disproved.

It assumed WinFormsPumpHost.RunPumpThread left the fixture ItemViewer without a window handle, so Control.Invoke would throw, and that forcing the handle on the pump thread would remove a race. Four measured configurations showed otherwise: ItemViewer's InitializeComponent runs the Designer-emitted ISupportInitialize.EndInit() calls on both WebView2 children, that creates the child handles, and WinForms creates a parent's handle when a child's handle is created. A bare new QuickFiler.ItemViewer() on the pump thread — no harness, no SaveParameters, no .Handle read — already reports both children as handle-created.

Three consequences followed, all accepted rather than argued away:

  1. The viewer.Handle read is a no-op with respect to the state it claimed to establish.
  2. The only genuine pre-change failure observed was a 60,000 ms PumpTimeoutMs expiry under CPU contention, a different root cause. A missing handle makes Control.Invoke throw immediately; it does not hang for sixty seconds.
  3. The 30-of-30 post-change green record is statistically consistent with the change having no effect — roughly a one-in-four outcome under the null hypothesis.

The durable value of the work is therefore the mechanism finding and the tests that pin it, not a repair. That is what this PR delivers.

What Changed

Tests (QuickFiler.Test/Controllers/, 3 files, +124 lines)

  • QfcItemController.InitializationTests.Part3.cs — adds regression tests including BuildPumpHarness_ForcesTheViewerWindowHandleOnThePumpThread and BuildPumpHarness_DoesNotCreateTheWebViewChildHandles. The latter asserts both WebView2 children are handle-created, pinning the inherited state; it fails if a future change makes them handle-less and so invalidates the assumption the fixture rests on.
  • QfcItemController.InitializationTests.Part2.cs and QfcItemController.ViewerSetupTests.cs — the comment above each viewer.Handle read now states the measured truth: the children, and therefore the parent, are already handle-created when construction returns, and the read is redundant today, retained deliberately as a defensive measure so the fixture does not silently depend on a third-party side effect this repository neither controls nor observes. The read itself is retained by decision, not by oversight.

Specification

Documentation and evidence

  • A decision record explaining why the work was re-scoped rather than landed as a repair or abandoned.
  • The four-configuration handle measurement, the pre-change and post-change determinism records, and the toolchain gate artifacts.
  • An evidence/.gitignore excluding raw *.trx, *.coverage and vstest scratch directories, plus a record of the 1,180.6 MB of raw output deleted in favour of the distilled Markdown.

Architecture / How It Fits Together

Nothing in the production wiring changes. WinFormsPumpHost still starts a real WinForms message pump on a dedicated STA thread, and QfcItemController still reaches Control.Invoke through InvokeBeginInvoke unchanged. The only structural addition is at the test fixture boundary: the pump harness now has tests asserting the handle state it inherits from ItemViewer construction, which converts an undocumented third-party dependency into a checked invariant.

Verification

Completed on this branch

Gate Result
CSharpier format check EXIT 0
MSBuild rebuild with analyzers 0 errors; 0 skipped CoreCompile targets
MSBuild rebuild with warnings-as-errors 0 errors
Nine-assembly test run with coverage and isolation 6459 total, 6459 passed, 0 failed
Repo-wide line coverage 85.59% (from 85.55%)
Repo-wide branch coverage 79.06% (from 79.03%)
QuickFiler package line coverage 81.08% (from 80.93%)
Acceptance criteria 14 of 14
Feature review (policy, code, feature audits) 0 blocking findings

The toolchain passed in a single clean pass with zero loop restarts. Coverage moved up; no changed-line regression is possible because zero production lines changed.

Recommended for a reviewer

Run the repository's documented C# toolchain in order — CSharpier check, the analyzer rebuild, the warnings-as-errors rebuild, then the nine-assembly test run with coverage. The exact commands are in CLAUDE.md under "C# Toolchain".

Backward Compatibility / Migration Notes

None. No public API, production file, project file, or workflow is touched. The three changed code files are test files, and no existing test was weakened, renamed, or removed.

Risks and Mitigations

Review Guide

  1. Start with the decision record in the feature folder — it explains why this PR does not claim a repair.
  2. Then the four-configuration handle measurement, which is the evidence everything else rests on.
  3. Then the three code files: Part3.cs for the new tests, and the two corrected comment blocks.
  4. Then the specification's acceptance criteria and scope revisions.
  5. The remaining files are documentation and evidence artifacts and are mechanical.

The diff against main is additions-only: 77 files added, 23 modified, zero deleted or renamed.

Follow-ups

GitHub Auto-close

  • None

drmoisan and others added 7 commits August 23, 2026 20:13
@drmoisan @claude
wip(511): preserve halted #511/#571 investigation — NOT a fix
787f22c 
This branch is preserved for evidence and is not merged. The remedy it
contains is a measured no-op; this commit does NOT repair #511 or #571.

The premise was that WinFormsPumpHost.RunPumpThread calls
Application.Run(new ApplicationContext()) without ever adding a form or
control, so no window handle exists when Control.Invoke is reached. That
premise is falsified by measurement recorded in
evidence/regression-testing/webview-child-handle-measurement.2026-08-21T18-10.md:

  - ItemViewer's constructor calls InitializeComponent, which runs
    ((ISupportInitialize)_l0v2h2_WebView2).BeginInit()/EndInit() and the
    same pair for _l0vhBreadcrumb_WebView2
    (QuickFiler/Viewers/ItemViewer.Designer.cs:89-90 and :6166-6167).
  - EndInit creates the child window handles, and WinForms creates a
    parent's handle when a child's is created, so the viewer's handle
    exists the instant construction returns.
  - A bare `new QuickFiler.ItemViewer()` on the pump thread, with no
    harness, no SaveParameters and no .Handle read, already reports both
    children handle-created. With the inserted statement commented out the
    state is identical.

The inserted `_ = await host.InvokeAsync(() => viewer.Handle)` therefore
forces a handle that already exists.

Three consequences, each verified independently:

1. The observed defect has a different root cause. The one genuine pre-fix
   failure was seven expiries at the 60,000 ms PumpTimeoutMs under machine
   load. A missing handle makes Control.Invoke throw immediately; it does
   not hang for sixty seconds.
2. The post-fix evidence does not demonstrate efficacy. Pre-fix run-level
   failure rate is about 1 in 21, so thirty consecutive clean runs has
   probability about 0.23 under the null hypothesis of no effect. Under the
   17-node contention that reproduced the original failure, supplementary
   pass B was 8 of 10 green: the suite still fails under load.
3. Comments inserted at Part2.cs and ViewerSetupTests.cs assert the children
   "stay handle-less", which the measurement contradicts. They are preserved
   here unaltered so the record is exact, and must not be carried into any
   future fix.

Scope lock held: both coverage-bearing production files hash to their exact
merge-base blobs, so #571's coverage is untouched. No production file was
modified. The spec.md diff is nine checkbox flips with zero text edits.

Spec AC 6 ("both WebView2 children remain handle-less") is unsatisfiable as
worded and was correctly left unchecked.

The 56 raw .trx result files are deliberately excluded and left on disk
only: they total about 359 MB and 1.86 million lines of XML. The 36 markdown
artifacts under evidence/ carry the Command, EXIT_CODE and Output Summary
for every run, which is what the evidence conventions require, and the .trx
are regenerable by re-running the recorded commands.

Refs #511, #571.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UHj7wjLweuwfAP8NDA4iiP
@drmoisan @claude
docs(511): scrub host identifiers from artifacts; record re-scope dec…
0d8a1cc 
…ision

Two changes, neither touching production or test code.

Host-identifier sanitization, at maintainer instruction, on the standing rule
that no file may embed an absolute host path or host identifier:

- Renamed 140 untracked evidence paths to strip the vstest.console.exe default
  <account>_<HOST>_ filename prefix. Raw .trx/.coverage remain uncommitted per
  the ratified evidence-retention decision.
- Stripped that prefix from the 10 tracked markdown evidence files that cited
  those filenames, keeping the citations accurate against the renamed files.
- Replaced 91 absolute-path occurrences across 27 tracked files with portable
  placeholders <repo-root>, <user-profile>, <user> and <host>, applied
  longest-first so the substitution is idempotent. Commands in the evidence
  records stay readable and reproducible.
- Recorded the convention at .claude/agent-memory/_shared_no_absolute_host_paths.md
  and indexed it from five agent memory indexes, including the vstest default
  TRX-naming trap that introduced the prefix.

Scope note: roughly 146 tracked files in other and archived feature folders
still carry the prefix, and about 157 carry the bare host name, including
.claude/settings.json and .vscode/settings.json. Sanitizing them here would
break this child's scope-lock acceptance criterion, so the remainder is left
for its own issue.

Decision record: the #511 child halted because measurement falsified its
premise -- the remedy forces a window handle that ItemViewer construction has
already created via third-party ISupportInitialize.EndInit. The maintainer
elected to re-scope: keep the fixture hardening and the mechanism finding,
correct the false comments, revise the unsatisfiable acceptance criterion,
narrow the nine-assembly zero gate, file follow-ups for the load-induced pump
timeout, and open a PR that does not claim to repair #511. The orchestrator
checkpoint is gitignored, so decision-record.2026-08-23T20-40.md carries the
record in git history.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drmoisan @claude
docs(511): open remediation cycle 1; delete 1.2 GB of raw vstest output
6263ad1 
Opens the R1 remediation cycle for the descoped #511/#571 child and clears the
raw machine artifacts out of the worktree. No production or test code changes.

Requirements and plan:

- remediation-inputs.2026-08-23T20-57.md records the six halt findings (A-F),
  the two maintainer decisions (HI-1, HI-2), four orchestrator scope decisions,
  and Part 1, a ground-truth table re-verified against this worktree rather
  than carried over from a prior agent summary.
- remediation-plan.2026-08-23T20-57.md is the R1 plan: 41 tasks across five
  phases, passing the MCP plan validator including acceptance-gate rules
  G1-G6.

Four orchestrator scope decisions recorded in the inputs:

- The pull request will target main, not the epic integration branch. That
  branch (e7b4824) is a strict ancestor of main; PR #595 already merged it and
  main is seven commits ahead. Targeting it would produce an 85-file diff
  carrying unrelated main commits and would receive zero checks, because
  ci.yml triggers pull_request on [main, development] only. Targeting main
  makes CI a real gate instead of a vacuous one.
- The measured no-op handle read is retained per HI-1; its comment is corrected
  to state both the measured truth and the read's present redundancy.
- Raw vstest output is gitignored at the evidence root, then deleted.
- Follow-up issues #592, #594 and #597 already exist and are cited rather than
  duplicated. This branch makes no repair claim for #511 or #571, both of which
  are already CLOSED as NOT_PLANNED, superseded by #592.

Raw artifact deletion:

- Removed 56 .trx (358.3 MB) and 42 .coverage (822.2 MB), 1,180.6 MB total,
  and pruned 188 empty scratch directories. The evidence tree is now 236 KB of
  markdown.
- Before deleting, the per-run totals, the identity of run 5's single failure
  in the sibling-owned UtilitiesCS.Test assembly, and the 10-of-10 outcomes of
  this child's four named tests were re-derived directly from the TRX XML and
  matched the committed distillation exactly, so the raw files carried no audit
  value that is not already in the repository.
- evidence/.gitignore now excludes *.trx, *.coverage and the vstest scratch
  directories, so the remaining toolchain runs cannot leak them into git add -A.
  Repository-root .gitignore already excluded *.coverage but not *.trx.

Also records an atomic-planner memory note: when a finding falsifies a spec
premise, sweep every spec section that asserts it, not only the acceptance
criteria.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8mtjthdSZFmNGKR4bcS1Q
@drmoisan @claude
docs(511): remediation cycle 1 - correct falsified claims, complete f…
0714308 
…inal QC

Remediation cycle 1 for the halted #511 / #571 investigation. This branch makes no repair
claim for either of those two issues. Both were CLOSED as NOT_PLANNED on 2026-08-23 and are
superseded by #592, which carries the genuine defect: the load-induced 60,000 ms
PumpTimeoutMs expiry cascade under machine load. #594 carries the three pre-existing
UtilitiesCS.Test flakes, and #597 the repository-wide analyzer version skew. No GitHub issue
is created by this cycle; all three already exist.

Executable code is unchanged. The two .cs edits are comment-block rewrites only, verified by
filtering `git diff --numstat 02983a7` to non-comment lines and getting the empty set.

- QfcItemController.InitializationTests.Part2.cs and QfcItemController.ViewerSetupTests.cs:
  both comment blocks above the retained defensive `viewer.Handle` read now state the
  measured truth - both WebView2 children, and therefore the parent ItemViewer, are already
  handle-created when construction returns, via the Designer-emitted
  ISupportInitialize.EndInit() calls - and state that the read is redundant today and is
  retained deliberately as a defensive measure. (remediation Finding D)
- spec.md: acceptance criterion 6 revised to the measured inherited state (Finding E);
  criterion 3 revised to the owned-class QuickFiler.Test scope citing #594 (Finding F); the
  three falsified "In scope" bullets and the out-of-scope visible-window bullet revised per
  remediation-inputs Part 6; "Rollout & Follow-up" now names #592 as the filed follow-up.
  All 14 acceptance criteria are checked with cited evidence.
- plan.2026-08-21T18-10.md: P4-T2's absolute-zero condition narrowed to the QuickFiler.Test
  assembly per the ratified owned-class scoping precedent; P6-T1 and P6-T20 recorded as
  discharged by the pre-existing #592 and #597.
- evidence/.gitignore: appended `r1-p*-t*/` to cover the Phase 3 per-run scratch directory.

Final QC loop, single consecutive pass, zero restarts:

- dotnet tool restore: EXIT 0, csharpier 1.2.6
- csharpier format, scoped to the three touched files: EXIT 0, 0 files rewritten by
  SHA-256 before/after comparison
- csharpier check .: EXIT 0, 1519 files checked, 0 unformatted
- msbuild /t:Rebuild with analyzers: EXIT 0, 0 errors, 0 `Skipping target "CoreCompile"`
- msbuild /t:Rebuild with TreatWarningsAsErrors: EXIT 0, 0 errors, no /p:Nullable=enable
- vstest 9 assemblies /EnableCodeCoverage /InIsolation: EXIT 0, 6459 total, 6459 passed,
  0 failed; all four owned named tests passed; QuickFiler.Test failed count 0
- coverage: 85.59% line (baseline 85.55%), 79.06% branch (baseline 79.03%), QuickFiler
  package 81.08% (baseline 80.93%, delta +0.15pp), changed-module 86.34% (unchanged)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8mtjthdSZFmNGKR4bcS1Q
@drmoisan @claude
docs(511): delete raw vstest output from the evidence tree (cycle 1 f…
2404363 
…inal task)

Final task of remediation cycle 1, per orchestrator Decision 3 step 3. Deletes the raw vstest
artifacts newly produced by the Phase 3 loop: 1 `.trx` and 2 `.coverage` files under
`evidence/qa-gates/r1-p3-t6/`, plus the 5 scratch directories they emptied. Recursive counts of
`*.trx` and `*.coverage` under the evidence tree are now 0 and 0, and the tree contains no empty
directory.

All three deleted files were untracked or ignored - the `r1-p*-t*/` line appended to
`evidence/.gitignore` covers the whole subtree and the repository-root `.gitignore` already
excludes `*.coverage` - so no committed content is lost. The distilled Markdown records are the
evidence of record; the P3-T6 run is captured in full, including its verbatim
`ResultSummary/Counters` block and per-assembly counts, in
`evidence/qa-gates/remediation-suite-run.2026-08-23T20-57.md`.

Also records the P4-T9 and P4-T10 check-offs in `remediation-plan.2026-08-23T20-57.md`, completing
all 42 tasks of the cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8mtjthdSZFmNGKR4bcS1Q
@drmoisan @claude
docs(511): record remediation cycle 1 re-audit artifacts
303b79f 
Adds the three feature-review artifacts that close remediation cycle 1, plus
the reviewer's memory note. No code or specification changes.

Audit outcome: zero blocking findings across all three artifacts, so the
cycle-1 exit gate is satisfied and no further cycle is opened.

- policy-audit.2026-08-24T00-01.md
- code-review.2026-08-24T00-01.md
- feature-audit.2026-08-24T00-01.md

Scope audited was the full branch diff against main at merge-base f85a36f:
three C# test files under QuickFiler.Test/Controllers/ totalling 124 added
lines, 68 feature-folder documentation and evidence files, and 26 agent-memory
files. Zero production code, project-file, or workflow changes.

Verified in the audit: the corrected comment blocks state the measured
inherited-handle behaviour including the required redundancy statement; spec
AC 3, AC 6 and the Scope bullets match the measured record; the original
plan's P4-T2 gate is narrowed to the classes this child owns per the ratified
owned-class precedent; the toolchain passed in a single clean pass; the
evidence .gitignore is present with zero raw TRX or .coverage files tracked or
on disk; and no commit message binds a closing keyword to #511 or #571.

C# coverage measured repo-wide at 85.59% line and 79.06% branch, read directly
from artifacts/csharp/coverage.xml and matching the committed evidence. No
changed-line regression is possible because zero production lines changed.
Acceptance criteria are 14 of 14 passing under full-bug mode with spec.md as
the sole source.

The reviewer also corrected a generator defect: artifacts/pr_context.summary.txt
had misclassified the branch as "Core logic changes: 0 files" and omitted the
three .cs files, which would have suppressed the C# gates. It was corrected in
place and the full C# gates were applied.

Non-blocking residuals recorded for follow-up: the spec's Root Cause Analysis
section still narrates the pre-measurement claims without a revision marker,
and the AC 1 and AC 3 "ten TRX stored" wording predates the maintainer-directed
raw-artifact deletion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8mtjthdSZFmNGKR4bcS1Q
@drmoisan @claude
chore(memory): record the negated-closing-keyword trap and the spent-…
67be0c1 
…integration-branch corollary

Two orchestration lessons from the #511 remediation cycle.

GitHub's auto-close parser does not understand negation: a commit message
reading "this commit does NOT fix #511" contains the literal "fix #511" and
closes #511 when it lands on the default branch. Two commits on the #511 branch
carried exactly that shape, written specifically to disclaim a repair; merging
them would have auto-closed the issue and could have flipped its state_reason
from NOT_PLANNED to COMPLETED. Records the scan command, the safe phrasings, the
non-interactive filter-branch repair, and the fact that file contents are never
parsed so prose inside committed specs needs no editing.

Also extends the epic-child-PR note: the zero-CI-checks rule holds only while
the integration branch is live. Once the epic has completed and its integration
branch is a strict ancestor of main, that branch is spent and a leftover child
must retarget main, which converts a zero-check PR into a real five-check gate.
Records the ancestry test, the epic_mode and ci_gate consequences, and the
additions-only diff verification that guards against a stale base silently
deleting what main gained.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8mtjthdSZFmNGKR4bcS1Q
@drmoisan
drmoisan merged commit 54c9350 into main Aug 24, 2026
5 checks passed
@drmoisan
drmoisan deleted the bug/winformspumphost-suite-determinism-511-exec branch August 28, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@drmoisan