mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-17 04:25:59 +00:00
e612867cf0
* docs: plan windows signals and wait parity (sub-phase 2.11)
`capture_signal_baseline` and `wait_for_menu` are the two adapter methods
still falling to core defaults, so every `wait --event` and `wait --menu`
returns PLATFORM_NOT_SUPPORTED on Windows. v0.8.0 made that load-bearing:
`launch` stopped waiting for a window it did not cause and now points
callers at `wait --event window-opened` instead.
The plan is shaped by four constraints read out of core rather than
assumed. Core's retryable set is closed at three codes, and both shipped
Windows inventories return codes outside it on a race the crate's own
test retries five times. Core discards an Ok that arrives past the
deadline and hands the adapter the whole wait's deadline every poll, so
it is a ceiling and never a budget. `validate_signal_scope` is strict and
terminal, which forces the filter to intersect pid and instance rather
than copy macOS's name-only alternation - that alternation aborts the
wait whenever two processes share an image name, which on Windows is
every second instance of any program. And a missing `process_instance`
does not degrade loudly; it makes the entity invisible to the diff.
Twelve units, fifteen requirements each mapped to a test that fails if it
is violated, twelve key technical decisions, and a probe area (23) for
what would otherwise be assumed: which of two menu-detection sources
fires per UI stack, whether a menu popup passes the window filter, the
mid-walk race rate, and the capture cost the 200ms poll must accommodate.
Reviewed by five personas; nine findings applied. Two were design errors:
an exclusion-flips-completeness rule that would have frozen a false bit
into core's seed baseline and disabled window and focus events for entire
waits on most desktops, and a docs/phases.md line misidentified as a
duplicate of 2.11's exit criterion when it belongs to 3.11 on the Linux
line - correcting it would have edited a Phase 3 sub-phase from a Phase 2
PR.
Also records what merging v0.8.0 proved: the mid-walk identity race fired
in the launch path and is now handled there by the same bounded re-walk
this plan specifies, so U3 mirrors a shipped shape instead of inventing
one.
* docs: record that the canonical app-identifier predicate misses the wait path
v0.8.1 added `AppInfo::matches_identifier`, documented as "the one
predicate every launch-target match uses". It is not: its only callers
are the launch target match in core and macOS. `list_apps_scoped`'s
default and `process_from_baseline` — the two sites `wait --menu` and
`wait --event` actually resolve `--app` through — still compare `name`
with `eq_ignore_ascii_case`.
KTD5 traces `--app` across those sites to conclude the Windows contract
is the process image name, so a reader who assumed the new predicate
now governs `--app` everywhere would mis-trace the decision and expect
bundle-id matching that never runs there.
* feat: measure the signal and menu gaps (sub-phase 2.11 U1)
Probe area 23 settles what the plan would otherwise have assumed, and the
answers are not the ones the plan hypothesised.
**Menu detection needs two complementary sources, and one obvious
candidate is actively wrong.** "A UIA Menu-family element is reachable
for the process" reads true at idle on both Win32 and WPF, because the
standing menu bar is reachable under the main window at rest - a
detector built on it would report a menu open for any app that merely
has a menu bar, so `wait --menu` would return instantly and
`wait --menu-closed` would never fire. What discriminates:
- classic `GetGUIThreadInfo` menu-mode flags, per thread of the target
pid, fire for Win32 menu-bar, context and system menus and are silent
at idle - and never fire for WPF in any state;
- a root-level UIA child of the pid whose HWND carries WS_EX_TOOLWINDOW
and has a Menu-family element reachable at or under it fires for WPF
dropdown and context menus, catches Win32 context menus too, and is
silent at idle on both stacks.
Their union covers every open state measured and neither fires at rest,
verified false -> true -> false across a staged open and close. Two
rejected candidates are recorded with their evidence rather than left
for an implementer to retry: ExpandCollapseState==Expanded never fires
for context menus on either stack, and IsOffscreen plus a non-empty rect
is constant-true because the standing menu bar is already onscreen.
**The spurious-window-opened risk does not materialise.** Every classic
`#32768` popup carries WS_EX_TOOLWINDOW, so the shipped agent-facing
window filter already excludes it and a menu opening cannot fabricate a
window lifecycle event.
Also measured: modal classification agrees across owner,
WS_EX_DLGMODALFRAME, WindowIsModal and control type; capture cost and
both predicate sources sit well inside the 200ms poll and 50ms menu
budgets; and the mid-walk identity race produced 0 hits in 120
iterations at High-integrity Administrator - not a contradiction of the
race being reachable, which was observed directly in the launch path,
but a rate this host could not reproduce, so it defers to a
standard-user runner rather than being claimed either way. Chromium and
WinUI menu staging record measurable:false with named branches.
**Evidence machinery.** A16-3 read 132 processes where its capture
records 133, and the gate built to catch that class audited 20 of 165
rows because a row stating its number in prose rather than a quoted
field:value pair was exempt - the exact shape that let A16-3 through.
The content check now audits 49 of 175, reports audited-versus-total in
its verdict so a coverage regression is visible, and ships a MUST-CATCH
fixture built from A16-3's own prose form. A18-3 and A18-9 gained the
closure tags they lacked.
The phases.md hunk-index bijection is reported rather than enforced, and
the reason is structural: it diffs against `main`, which under the
platform delivery model is an entire phase behind the integration
branch, so the measured count grows with every merged sub-phase while
the index only gains what each sub-phase remembered. Enforcing it would
make one sub-phase answer for every earlier one's doc edits. The half
that carries the invariant's value still fails the build - every indexed
hunk must name a ledger row that exists, and every CONTRADICTS row must
be backed by a hunk - and the 42-hunk shortfall is surfaced in the
verdict. Bringing the index current belongs to 2.15's docs sync.
Area 23 and the ledger gate are both registered in the capability-probe
workflow, so their verdicts fail a build instead of waiting for someone
to run them by hand.
* feat: single-pass signal inventory and menu/dialog fixtures (2.11 U2, U3)
The inventory is a new composition over the shipped primitives rather than
a wrapper over `list_windows_live` / `list_apps_live`, because the signal
path needs different semantics on three axes: a transient race must not be
terminal, the deadline must be honoured, and the error set must stay inside
what core retries. One `EnumWindows` walk plus one ToolHelp snapshot yields
both inventories, with the per-pid token read cached so a process owning
twelve windows costs one read - composing the two shipped inventories would
walk the desktop twice every 200ms, since `list_apps_live` enumerates
windows again internally.
Four properties carry the design, each with a test that can fail:
- Exactly one walk per capture, asserted through a counting seam. A later
refactor to "just call the two shipped inventories" breaks it.
- Every entry carries a process instance. A window whose identity cannot be
read is excluded and counted, never emitted with `None` - core's diff
drops a `None` silently and a process-filtered wait aborts on it outright.
- An identity exclusion does not touch completeness; only a truncated walk
does. This is the sharpest edge in the unit: core seeds its baseline from
the first successful capture and holds it for the wait's lifetime, and the
diff skips window and focus events whenever either side is incomplete, so
a bit set false in the seed disables those events for the entire wait. On
a real desktop at least one window is typically owned by a process whose
token this process cannot read.
- The race is absorbed by a bounded re-walk and reported as a retryable code
on exhaustion, never as the shipped inventories' `WINDOW_NOT_FOUND` or
`INTERNAL`, either of which would abort the whole wait.
Both fixtures are child processes. A menu opened in the test process would
put the test's own thread into menu mode and make a per-process predicate
true for the wrong reason, so the menu fixture re-executes itself and
signals readiness, and its dismiss command is delivered reentrantly into
the tracked menu's own message loop. The modal fixture is an owned window
with WS_EX_DLGMODALFRAME that disables its owner, matching every property
area 23 measured on a real MessageBox, and carries no title any test
asserts on - the discovery test has to prove discovery without naming.
The dialog fixture is a second file rather than a compressed one: both
fixtures at this codebase's density exceeded the 400-line cap, which the
project resolves by splitting.
Two verification defects found while checking this stage, both fixed here:
The stability test demanded two quiescent captures and retried five times
for them, which cannot hold on a shared runner whose sibling tests spawn
and kill windows - it passed alone and failed in the suite. It now asserts
the property the diff actually depends on: an entity present in both
captures reports one identity in both. That is immune to churn and still
fails an inventory that invents identity.
`scripts/check-no-phase-references.sh` could not fail on a real violation.
Its tree scan tested the exit status of a grouped command substitution,
which reports the status of its LAST command, so a hit in crates/ or src/
was invisible whenever the skills/ grep found nothing - the usual case.
Three plan-decision ids reached shipped source with the gate reporting
clean. The scan now tests the collected text, the ids are rewritten to
describe the code, and the scan roots are overridable so the self-test can
drive the real scan against a planted fixture tree. The previous self-test
only ever exercised the line branch, so the tree branch had no coverage at
all; reintroducing the defect now fails the self-test.
* feat: windows signals and wait parity (sub-phase 2.11 U4-U10)
`wait --event` and `wait --menu` now work on Windows. Both previously
returned PLATFORM_NOT_SUPPORTED because the two adapter methods fell to
core defaults; v0.8.0 made that load-bearing when `launch` stopped waiting
for a window and pointed callers at `wait --event window-opened` instead.
Verified end to end against a real desktop: `wait --event window-opened`
discovers a Notepad launch in ~2.9s and reports its app, pid, title and
window id without the caller supplying any of them - discovery purely by
baseline diff, which is what the sub-phase exists to deliver.
The filter intersects rather than alternates. macOS filters by name only
when an app is set; core sets BOTH app and process for every
non-app-launched wait, and two Windows processes routinely share one image
name, so alternation would return a second pid and make
validate_signal_scope abort the wait with a non-retryable STALE_REF.
The menu predicate ships as two measured sources whose union covers every
open state while neither fires at rest. Classic menu-mode flags are read
per thread of the target pid - never thread 0, which asks about the
foreground thread and answers about the wrong process. The UIA source is
gated behind the shipped non-pumping pre-probe, because `wait_for_menu` is
the one path where core makes a single call and owns no retry, so an
in-flight blocking read has no backstop at any layer.
`wait_for_menu` keeps macOS's double identity re-check. It looks redundant
and is not: it closes the window where the target dies between the
predicate read and the return, so a dead target reports STALE_REF instead
of a satisfied wait.
Completeness reports whether the enumeration ran to completion, never
whether every entity was identifiable. Core seeds its baseline from the
first successful capture and holds it for the wait's lifetime, and the diff
skips window and focus events whenever either side is incomplete - so a
false bit in the seed would disable those events for the entire wait on any
desktop with one unreadable window, which is most of them.
U9 consolidated the bounded re-walk onto one shared helper. It had three
candidate homes by then - the signal inventory, launch's window
observation, and the list_windows entry point - and a retry budget that
drifts between call sites is how one of them silently stops retrying.
Four test-isolation defects fixed, each a real race rather than a loosened
assertion. FIXTURE_APP_NAME_LOCK's documented contract - at most one
re-exec'd fixture alive at a time - was false, because several files
spawned fixtures without taking it; a negative control reproduced the
resulting AmbiguousTarget with candidate_count 2. Tests that force OS
foreground now take the on-screen stage lock, which window_ops.rs already
documents as guarding screen state rather than screen real estate. Tests
needing both locks take them in one mandated order, since opposite orders
would trade a flake for a deadlock. And a retry test asserting an exact
attempt count now asserts the property with both bounds, because the
environment can inject a genuine race the test did not force, and a test
that fails when the hazard it guards against actually happens is not a
guard.
Suite: 1133 windows, 1002 core, 140 binary, green across six consecutive
runs. clippy -D warnings, fmt, file-size, no-phase-reference gates pass;
core still names no platform crate.
* refactor: simplify the 2.11 change set and close a snapshot handle leak
Three findings from the simplification pass, two of them converged on by
independent reviewers. Every structure pin survived: the reviewers
explicitly declined to touch the two-phase race detection, the shared
listing-retry helper and the closed error set, reading them as deliberate
rather than redundant.
- Three sites reimplemented `window_identity::live_window_owner` verbatim,
including its unsafe FFI and its non-Windows stub. They now call it.
- Three byte-identical copies of the error-narrowing rule collapse to one
home in `listing_retry.rs`. The mapping is unchanged - this removes a
triplicated implementation of one rule, which is how three copies drift
apart, not an edit to the rule itself.
- `app_scoped_surfaces` called `menu_is_open` once per target, and each
call snapshots every thread on the desktop: `TH32CS_SNAPTHREAD` is not
scoped to a pid. Two processes sharing an image name - two notepad.exe,
two chrome.exe, both ordinary under an app-only filter - therefore cost
two whole-desktop thread walks on every 200ms poll. The sibling
`sheet_surfaces` two lines above had already solved this and documents
the intent, so the menu source simply was not following its neighbour's
rule. One snapshot per call now serves every target. `GetGUIThreadInfo`
still runs per thread and never with 0, both sources still run, and
`menu_is_open`'s single-pid entry point is unchanged because `wait.rs`
calls it directly.
The optimisation shipped without a guard, so it has one now: a test that
fails if the snapshot count stops being exactly one per call.
Separately, the walk leaked a ToolHelp snapshot handle. Its mid-loop
deadline check returned through `?` before the close, so an expired
capture leaked one handle - on a path `wait --event` polls every 200ms,
which exhausts the process over a long wait. The new multi-target path
had this right; the pre-existing single-pid one did not. Both exits now
close, and a test proves opens equal closes on the deadline path.
Reintroducing the early return fails it.
Suite 1135 passing; clippy -D warnings, fmt, file-size and
no-phase-reference gates clean.
* docs: dogfood sub-phase 2.11 and correct what it disproves (U11, U12)
Five findings against real software - Notepad, Explorer, Paint, cmd and an
Electron target - on the release binary. Every one carries a disposition
and none is left at "recorded".
The most consequential is not a Windows defect at all. `wait --event`
seeds a baseline from the first successful capture and diffs every later
poll against that same fixed capture, never advancing it, so an entity
that both appears and disappears inside one wait is absent from the
baseline and absent from the current capture and the diff sees nothing.
Measured: a window opened and closed inside one wait produced no
`window-closed` in 4 of 4 discriminating trials while a pre-existing
window produced one in 4 of 4. The report left the matching
`app-terminated` case as an untested prediction; it is now measured - an
app launched 2s into a 14s wait and terminated 3s later produced TIMEOUT.
The gap generalises to every disappearance event. The fixed baseline is
platform-neutral and macOS behaves identically, so this lands at the gate
that reviews both adapters rather than being patched from a Windows
sub-phase.
Four of five findings land on 2.15 and the report says so plainly rather
than glossing it: three are raised in `crates/core`, shared with macOS,
and fixing them from here would change both adapters' behaviour from
inside a Windows sub-phase - the thing the delivery model exists to
prevent. The concentration is evidence about where cross-platform
contract debt sits, not this sub-phase deflecting work. The one accepted
finding is accepted because acting on a single unreproducible observation
would be guessing, and it is recorded so the next sighting is the second
data point rather than the first.
2.11's own entry is corrected against what shipped. Its exit criterion
named an e2e that the harness sub-phase has not built and which depends
on this one, so it could never have been discharged here; it now names
the fixture-driven analog it actually proves, with 2.12 re-running it
through the harness once that exists. The `~1k LOC` estimate counted only
the two adapter methods and missed everything that makes them correct -
the real figure is ~5.7k of Rust plus ~4.7k of corpus and docs. The scope
now records the two-source menu detector and the deadline and race
hardening of the shipped inventories, and states what stays with 2.12.1.
Receiving sub-phases updated in this same PR, per the deferral rule.
2.12 gains the two menu stacks area 23 could not stage, the race-rate
measurement a standard-user runner owes, and the unidentifiable-window
population its split-integrity rig can finally bound. 2.14 gains the
surface-kind asymmetry: the signal path emits `menu` surfaces while
`supported_surfaces` advertises three kinds, deliberately, because
advertising a fourth would make `snapshot --surface menu` and
`find --surface menu` claim a capability the adapter refuses. 2.15 gains
the baseline-advance decision, the two `app_lookup` error envelopes that
name a recovery the command cannot perform, the hunk-index backlog, and a
third arm on the existing `press --app` divergence - the non-interactive
*caller*, distinct from the non-foreground *target*, which failed to
dismiss a menu in 4 of 4 reproductions from a background job while
succeeding every time from an interactive console.
* fix: keep the two gates 2.11 added green on CI
Both failures were in the verification machinery this sub-phase added, not
in the product.
The tree-mode self-test cleaned up its fixture directory with `rm -rf`,
and `tests/e2e/test_harness_contract.py` forbids `rm` at statement
position anywhere under `scripts/`. That rule is mechanical and absolute,
and the honest options were to comply or to widen the guard for my own
code - widening a guard to admit the thing it was written to catch is the
pattern this sub-phase spent its time removing elsewhere. The fixture now
uses one fixed path, overwritten per run, so it needs no deletion and
leaves one directory rather than one per invocation.
The ledger gate assumed a local `main` ref. A pull-request checkout has
none, so registering the gate in CI made it throw on `git diff -U0 main`.
It now resolves the base - local branch, then remote-tracking ref - and
when neither exists reports the hunk metric as unmeasured rather than
failing: the absence of a base ref says nothing about the ledger, and the
checks carrying this invariant's value do not need one. The capability
lane also fetches the base first, best-effort, so CI measures the metric
instead of only degrading; a network hiccup there must not turn a ledger
gate red.
The no-base-ref branch is exercised, not assumed - driven by pointing the
resolver at a ref that cannot exist, which reports `MeasuredHunkCount: -1`
and still exits 0. The first attempt at that simulation silently did
nothing (a PowerShell double-quoted string expanded the variable inside
the pattern, so the replacement never matched and an unmodified copy ran),
which is worth recording as the reason the check now asserts its own
substitution took effect before trusting the result.
* fix: honest envelope for a target that dies mid-wait (2.11 dogfood F6, F7)
The first dogfood fixed nothing in product code and routed four of five
findings onward. That is a weak result for a gate whose purpose is finding
what this sub-phase got wrong, so a second hostile pass ran along three
axes. It found two defects that are this sub-phase's own, and both are
fixed here.
`wait --menu`/`--menu-closed` reported a target that died mid-wait with
`disposition: {delivery: unknown, retry: unknown}` and no suggestion, while
every other stale-reference error in the same binary sets both.
`DeliverySemantics` defaults to unknown and `stale_process_error` never
opted out, so an agent whose application exited during an ordinary churn
scenario got no machine-readable signal about whether to retry or
re-resolve - for a failure that is completely understood at the point the
error is built. This project's envelope contract permits reading `recovery`
only when the retry disposition is `safe`, which a defaulted `unknown`
never is. It now reports `not_delivered` with a process-oriented recovery,
deliberately not core's ref-and-snapshot wording, which tells the caller to
refresh refs this surface does not have.
`wait_for_menu`'s doc comment promised the predicate is evaluated at least
once "even for a near-zero timeout", so a wait-for-closed against an
already-closed menu succeeds immediately. That is true of the function and
false of the command: the caller resolves the application first, and on a
budget smaller than that resolution costs the command returns the generic
deadline error without ever reaching the loop. Measured at timeout 0 and 1.
The doc now states the guarantee as the function's own and says plainly
that a loop handed an already-spent budget cannot rescue it. A comment
promising behaviour the shipped path cannot deliver is a defect in the same
way a wrong assertion is.
Two further findings are recorded with their evidence and not fixed here. A
scoped wait for a disappearance event fails APP_NOT_FOUND rather than
observing the disappearance when the target dies before application
resolution completes - deterministic when it is already gone, racy and
non-monotonic when it dies during resolution, and isolated by the
discriminating pair where the same transition unscoped hits at the same
timing. That gate is in core, shared with macOS, and its own escape hatch
for the terminated case requires a seeded baseline a plain CLI invocation
never has. And concurrent waits driven through a PowerShell job broker
returned APP_NOT_FOUND on a stable target where direct spawning was clean
across 100+ invocations, which tracks the broker rather than this binary.
What held under hostile load is recorded too, because a report listing only
failures misrepresents what was tested: 25 open/close churn cycles,
concurrent multi-target waits with no cross-contamination, capture cost
inside the poll budget with 27 live windows, the tool-window exclusion
holding so 15 menu rounds never fabricated a window-opened, three-instance
ambiguity degrading correctly, and flat handle, thread and working-set
counts across ~550 polls including the 50ms snapshot cycle.
The consumer axis lost its session mid-run, so shipped skill docs, batch
parity, session/trace interaction and macOS envelope parity are unaudited
by this pass - a coverage gap, not a clean result.
Suite 1136 windows, 1002 core.
* docs: correct the shipped guidance 2.11's own behaviour made wrong
The consumer axis of the adversarial dogfood - re-run after losing its
session - was the most productive of the three. Two of its six findings are
shipped-documentation defects caused by what this sub-phase shipped, and
those are fixed here rather than routed. Skill markdown is include_str!'d
into the binary and served to agents by `agent-desktop skills`, so a wrong
line in it misleads a caller exactly as a wrong error code does.
`wait --event surface-appeared` reports `"surface": "menu"` on Windows,
because the signal path detects an open menu. The obvious follow-up,
`snapshot --surface menu`, returns PLATFORM_NOT_SUPPORTED - Windows serves
window, focused and sheet. The observation doc told agents to use
`--surface menu` with no platform caveat at all. It now marks that flag
macOS-only, tells callers to read `supported_surfaces` from `status` before
requesting one, and names this exact asymmetry: the event says a menu
opened; it is not an invitation to snapshot that menu as a surface here.
Closing the asymmetry itself is 2.14's, but an undocumented asymmetry is a
defect now.
`--app` on an event wait was documented as scoping to an application. It
resolves once, before the poll loop, and pins to the one process instance it
found - so scoping to an app that is not running yet fails APP_NOT_FOUND in
under 100ms while ignoring the timeout, a target that dies during resolution
reports the same code even though its disappearance is what broke the
lookup, and a second process of the same name starting mid-wait is invisible
for the rest of the wait. The flag description and a new note now say what
is true and point a caller at `app-launched` or an unscoped wait for the
racing case.
Four findings are core-owned and written into 2.15's scope in this same PR:
the `--app` resolution semantics themselves - whether a scoped wait
re-resolves, whether appearance events should defer resolution into the
loop, and whether a disappearance event should treat an unresolvable target
as its answer rather than an error - and batch's event-wait pre-seed, which
captures the baseline one entry too early. That pre-seed is a genuine
improvement for launch-then-wait-for-open and defeats
launch-then-wait-for-close completely, so it is wrong in placement rather
than in principle. Both are platform-neutral and reachable on macOS.
One finding is accepted: a global-namespace file-ownership failure that
breaks status and snapshot but provably not this surface, since neither wait
path touches RefStore.
The report's earlier claim that the consumer axis left the shipped docs
unaudited is corrected - it audited them, and found these.
* fix: close the code review's real findings (2.11)
Nine reviewer lenses over the branch diff and its plan produced 14
findings. Each was validated against the code before acting; three were
real defects, one was a documentation promise I had dropped, and one was
correctly attributed to an earlier sub-phase rather than this one.
The duplicated ToolHelp thread walk is consolidated into
`system/thread_walk.rs`. This one had already cost something: the same
walk existed in two copies, and the single-pid copy leaked its snapshot
handle on the mid-loop deadline exit while the multi-pid copy did not.
One owner now holds the snapshot lifetime, the per-entry budget check and
the close on every exit path, and both call sites keep their existing
behaviour - the single-pid source still short-circuits on first match and
the multi-pid source still takes exactly one snapshot for N targets, both
pinned by tests that still pass unmodified.
Two error paths had no coverage and now do. `wait_for_menu` propagates a
non-Timeout predicate error on the first poll rather than spinning to the
deadline, and `observe_window_once` tolerates a Timeout from the
inventory as no-window-observed rather than propagating it. Both are
invert-verified: swallowing the error makes the first test spin the full
ten seconds and return Timeout instead of the injected code, and
narrowing the tolerated set makes the second propagate.
A user-facing suggestion string carried ten literal spaces, delivered
verbatim in the JSON envelope. Four lenses found it independently. It is
now built with `concat!` and guarded by an assertion that the suggestion
contains no run of whitespace.
`docs/phases.md` §2.15 referenced an `--app` stem-matching entry that did
not exist - the plan's KTD5 said U12 would write it and U12 wrote the
dogfood findings instead. The entry is now there with the three-site
trace and the reason a Windows override of `list_apps_scoped` was
rejected: it would change app resolution for every Windows command, not
just this one.
One finding is reattributed rather than fixed. Hide-to-tray fires a false
`app-terminated`, which is true, but the apps population comes from the
window-owning inventory §2.9 shipped, and the signal path matches it
deliberately so the two agree about what an app is. §2.15 already carries
the windowless-application question; it now also carries the event-level
consequence, which is the half an agent actually sees.
Suite 1138 windows, 1002 core. clippy -D warnings, fmt, file-size and
no-phase-reference gates clean.
* fix: use the documented ToolHelp failure sentinel, and catch plan ids the gate could not see
Round 2 of the review found a P1 that round 1, the simplification pass and
a three-axis adversarial dogfood all missed, because nothing guarded it.
`CreateToolhelp32Snapshot` reports failure as INVALID_HANDLE_VALUE, which
is -1. Every call site guarded on `is_null()`, which tests for 0 - so the
error branch was unreachable. A failing snapshot fell through: the
enumeration loop never ran, the walk returned "no thread reported menu
mode", and a native failure was reported as a confident negative answer.
On classic Win32 the consequence is real, because the UIA source
deliberately does not cover menu-bar dropdowns - there is no second
opinion, so `wait --menu` would run its full timeout against an
application whose menu was open the whole time.
The reviewer found one site. It is four, in three files, two of them
predating this sub-phase - a systemic convention error rather than an
isolated slip, so all four are fixed. `OpenProcess` in the same file does
return NULL, which is why the wrong guard looked idiomatic beside it. A
test now pins the two sentinels apart, so a guard cannot silently revert
to the unreachable one.
Shipped source also carried plan requirement ids - `(R5)`, `(R11)` - which
CLAUDE.md forbids for the same reason it forbids phase and KTD ids: they
point into a document that renumbers and mean nothing without it open.
`check-no-phase-references.sh` had rules for sub-phase, phase-number, KTD
and unit ids but none for a bare requirement id, so this shape passed
silently. The rule now exists, and it immediately found eight sites rather
than the four the review reported - including two in files this sub-phase
never touched, which are fixed here rather than left for the rule to trip
over later. Invert-verified: planting `(R5)` back into signals.rs fails
the gate, removing it passes.
That is the third gate this sub-phase has found unable to fail on the
class it was written for. The pattern is consistent enough to be worth
naming: a gate that has never been seen to fail is a hypothesis.
Suite 1139 windows, 1002 core, 140 binary. clippy -D warnings, fmt,
file-size and no-phase-reference gates clean.
* docs: capture the two lessons this sub-phase paid for
The unreachable-error-branch is a new learning, not a variant of an
existing one. `CreateToolhelp32Snapshot` fails with INVALID_HANDLE_VALUE
and four call sites guarded on `is_null()`, so the failure branch could
never run and a failed enumeration returned an empty result - identical
to "looked and found nothing". Win32 splits its handle-failure sentinel
between NULL and INVALID_HANDLE_VALUE, both typed HANDLE, so neither the
compiler nor the surrounding code can tell you which one applies; in
`process_identity.rs` a correct `is_null()` guard on `OpenProcess` sits a
few lines from an incorrect one on the snapshot, which makes imitating
the neighbour actively wrong. The write-up records what made it expensive:
it survived a simplification pass, an adversarial dogfood and a nine-lens
review, and the consolidation that merged the duplicated walk carried it
into the new home unchanged, because consolidation preserves behaviour
including the wrong kind.
The gate write-up gains two faces it did not have. A gate can be blind to
a shape nobody thought to ban - the plan-reference gate enumerated
sub-phase, phase, KTD and unit ids and had no rule for a bare requirement
id, so those sat in shipped source with the gate green through every
review; adding the rule found eight sites, twice what the review reported,
two of them in files this sub-phase never touched. And coverage that is
reported but never compared is the same failure wearing a number: the
ledger content check audited 20 of 165 rows and reported no failures,
which was accurate and useless, because the unaudited rows were where the
defect was. Both now say the operative rule - when a gate enumerates
banned shapes the enumeration is itself an assertion that decays, and a
gate that reports a count it never compares against a total has told you
nothing.
* fix: make the round-2 guards real (round 3)
I reported round 3 as clean without running it. It had not run; only
rounds 1 and 2 existed. Running it found two P1s, both in round 2's own
fixes, which is exactly the gap that claim papered over.
The sentinel test was a tautology. It asserted facts about the
INVALID_HANDLE_VALUE constant and never called any of the four functions
whose guards it claimed to pin, so reverting a guard to `is_null()` left
the entire suite green - verified by doing it. The fix that shipped with
that test was correct; the evidence that it would stay correct was not.
`walk_gui_threads` now has a test-only seam that substitutes the failure
value the API actually returns, and the test drives the real guard through
it. Reverting to `is_null()` now fails.
The requirement-id gate rule was wrong in both directions at once. It
rejected `Windows Server 2012 R2` and a `R2` register name - ordinary
prose for a Windows adapter crate - while missing every real citation
ending in a period or comma, because the pattern required a literal
trailing space. So it blocked legitimate text and let the actual
violations through. It now matches only the two unambiguous shapes: a
parenthesised id, or one introduced by a citation verb. Bare ids are
deliberately not bannable, because `R2` genuinely means three different
things and only context separates them.
Both defects shipped invisibly for the same reason: the rule had no
self-test fixtures, in a script whose own design says every fixture line
runs against every rule. Eight are added, four in each direction,
including the OS version string and the punctuation-terminated citation.
Restoring the old rule now fails the self-test naming both failure modes.
Also decorated the one timeout that could escape wait_for_menu's poll loop
without its direction-specific detail: the deadline can expire between the
loop's own expiry check and the sleep-slice computation, and
`remaining_slice` returns a bare timeout error on that path.
Suite 1139 windows, 1002 core, 140 binary. clippy -D warnings, fmt,
file-size and no-phase-reference gates clean.
* fix: make the gate rule honest about what it can catch, and stop three tests asserting the OS focus policy (round 4)
Round 4 reviewed round 3 and found the requirement-id rule still wrong in
both directions - my second attempt at it. It missed every sentence-initial
"Required by R7" and "Per R7" because only two of its six citation verbs
were capitalised, and it false-fired on "the helper R2 queue" because the
verb alternation had no left boundary, so any word ending in per or see
matched as a substring.
Rather than patch the regex a third time, I checked what the real
violations looked like: nine of the ten found in this tree were
parenthesised, one was bare. That settles the design. The rule now matches
two shapes and says plainly that it will never match a third: a bare id
with no citation verb is not mechanically separable from a CPU register,
from the tail of "Windows Server 2012 R2", or from a slot name, and a
regex that claimed otherwise would be lying in one direction or the other.
Review catches the residue. Both sides of the verb are boundary-anchored
now, and six more self-test fixtures cover exactly the two failure modes
round 4 found - restoring the old rule fails the self-test naming both.
Three live tests were asserting that Windows would grant their fixture the
foreground. It refuses routinely: SetForegroundWindow is advisory, and a
hosted runner declines it. That matters because TrackPopupMenu on a
non-foreground window has its popup dismissed immediately, so the fixture
reports it called the API while nothing is on screen - the predicate then
correctly reports no menu and the test fails on a precondition it never
established. The product was right in every one of these failures. Each
leg now verifies the grant and skips with a stated reason when the OS
declines, which is the crate's existing pattern for an unstageable
precondition; the assertions themselves are untouched. The two focus legs
additionally pin that the only refusal they will tolerate is the
documented one, so a different failure still fails.
A doc comment added in round 3 landed one function too early in
process_identity.rs, describing the ToolHelp failure sentinel above a
function that never calls ToolHelp. Moved to the function that does.
Recorded and deliberately not changed: signal_inventory's walk and
assembly phases raise a bare Timeout where the enumeration loop reports a
truncated inventory. The inconsistency is real, but truncation is caused
by budget expiry, so a truncated Ok always arrives with the deadline
already spent and core discards a late Ok - both paths are observably
identical. Routing the error path into the truncated one would only add
ways to poison the seed baseline, which is the failure the completeness
rule exists to prevent.
Suite 1139 windows, 1002 core. clippy -D warnings, fmt, file-size and
no-phase-reference gates clean.
* fix: stop the focus guards using the product as their own oracle (round 5)
I told the owner this was ready to merge while round 5 was still running.
It found a P1 in round 4's own fix, and it is the exact failure the round
was told to hunt: a guard that converts a real regression into a silent
pass.
Round 4 made two live tests skip when `focus_window` returned
ActionFailed, reasoning that the OS had declined the foreground. But
ActionFailed on that path has exactly one source - the activation budget
exhausting because the window never became foreground - which is also
precisely what a broken `bring_to_foreground` produces. The guard used
the function under test as its own precondition oracle. Worse, no test in
the crate required activation's success path to run at all: the one test
that looks like it does accepts ActionFailed as a valid outcome. A
refactor that made activation a no-op would have shipped with the suite
green.
The precondition is now established independently, with a raw
SetForegroundWindow staged onto a second parking fixture. That answers the
environment question - will this desktop grant foreground at all - without
consulting the product, and it leaves the foreground somewhere other than
the window under test, so the product's own call still has to cause the
transition the test observes. Once the desktop has proven it grants
foreground, a refusal from `focus_window` fails the test rather than
skipping it. Invert-verified: making `bring_to_foreground` a no-op fails
both legs, restoring it passes both.
Two defects of my own surfaced while fixing this, and both are worth
naming because they are the same mistakes this sub-phase has already paid
for once. The first attempt staged foreground directly onto the window
under test, which answered the environment question and performed the
transition at the same time, leaving the product's call decorative - the
test would have passed with activation entirely broken. The second moved
the fixture spawn ahead of the baseline capture, which deleted the
window-opened half of a two-event assertion and made the remainder
timing-dependent: it passed alone and failed in the suite. Correct order
is park, baseline, spawn, activate.
Suite 1139 windows across two consecutive runs, 1002 core, 140 binary.
clippy -D warnings, fmt, file-size and no-phase-reference gates clean.
* test: pin the composed focus assertion to the window activation targeted (round 6)
Round 6's correctness lens returned zero findings and traced the round-5
staging as sound. Two things it did surface are worth acting on and
recording honestly.
The composed-capture test asserted only that *a* focus change appeared in
an unfiltered desktop-wide diff, while its sibling assertion three lines
above pins the window-opened event to the fixture's pid. Since diff_focus
emits at most one focus event system-wide, an activation that focused the
wrong window - a stale handle, or re-activating the parking fixture -
would still produce an event and satisfy the assertion. The guard's pass
condition was looser than what it claimed to verify, in the one test whose
entire purpose is proving the product's own activation works. It now names
the pid activation was asked for.
The second is a limitation rather than a defect, and it deserves stating
plainly because it bounds what the round-5 fix achieves. On a desktop that
declines a raw SetForegroundWindow, both focus tests skip, and a skip is
indistinguishable from a pass in the summary line - so on such a host they
contribute no coverage at all while reporting green. That is real. What
bounds it: the CI Windows lane does not skip, verified by grepping the job
log for the skip messages and finding none, so the path is exercised where
it counts. The local dev box declines because ordinary applications hold
the foreground there. The earlier invert-verification was valid - it ran
the real path at that moment, which is why breaking bring_to_foreground
failed both tests - but the same host now skips, which is exactly the
variability the skip branch exists to absorb.
The eprintln-and-return skip convention is this crate's established
pattern across many live-test files predating this branch, so making it
distinguishable in a summary line is a crate-wide change rather than
something to bolt onto two tests here.
Suite 1139 windows, 1002 core. clippy -D warnings, fmt, file-size and
no-phase-reference gates clean.
|
||
|---|---|---|
| .. | ||
| examples | ||
| src | ||
| Cargo.toml | ||