Readable grids, network devices, external bufferbloat import, and validated device writes - #12
Merged
Conversation
…e measurement
Three strands, all of them reachable from the same complaint: the app knew
things it was not managing to say.
The interface first. Every proportional column in every grid had no floor, so
once the fixed columns filled the viewport the star columns collapsed — the
adapter Description rendered about twenty-five pixels wide, and the tuning
plan's accepted values and trade-off were each two characters ("Di", "Of"),
which is exactly where a value is chosen. Star columns now carry a minimum and
the grid scrolls instead. Every text cell also carries its full value as a
tooltip, one setter for all twenty grids, because a sentence clipped mid-word
is worse than no sentence. The dashboard's health findings wrap rather than
clip, severities and risks are coloured, and a double click opens the section
that fixes the finding.
The tuning plan's capability grid had nine columns in a thousand pixels. It now
keeps the columns a decision needs and moves keyword, focus, accepted values and
trade-off into a detail panel under the selected row. The plan preview loses its
"Rollback restores" column, which was bound to the same BeforeDisplay as
"Current": one fact, printed twice, costing 130px in a grid that had none spare.
The window header merges into one row — a search box that now says what it is,
Refresh promoted out of the Adapters tab where it was hiding, three exports
collapsed into one menu — and the eighteen flat navigation entries are grouped
under OVERVIEW, INVENTORY, MEASURE, ACT and RECORDS. Group headings are disabled
TabItems, so they are scenery rather than destinations; note that TabItem header
strings are matched as data by the health-finding router, so they are renamed
only deliberately. F5 refreshes, Ctrl+F reaches the search, and the status bar
grew an activity bar driven by a counter rather than a flag, since a diagnosis
and a throughput run can overlap. The reference tools left Preferences, which is
not where anyone looks for a list of measurement services. Two buttons in the
history tab shared Grid.Column="2" and drew on top of each other; the row is a
WrapPanel now and cannot reproduce that.
The dashboard also stopped claiming that "mutations remain disabled in this
preview build", which stopped being true when the transaction path opened. A
stale reassurance is worse than none.
Second: a machine's network devices, and which of them earn their place. The
Interfaces tab lists the real devices — filter and loopback pseudo-interfaces
are excluded, and so is Windows' own RAS plumbing, matched by the INFs that
install it (netrasa, netsstpa, netavpna, kdnic) rather than by vendor, since
"Microsoft" also covers the Hyper-V switch, which is a legitimate thing to
switch off. Advising someone to disable the miniports their VPN runs on would
have been advice that costs a connection and buys nothing. Third-party virtual
adapters keep their own INFs and are unaffected.
Each device gets a role, a verdict and the evidence behind it. One rule is not
advice: the adapter carrying the default route is never offered for disabling,
under any profile, on any machine. It is the way back in. The only judgement the
profile controls is whether a second physical path counts against you.
Two device-level settings join the typed catalogue: adapter.state, which enables
and disables through MSFT_NetAdapter, and adapter.power-saving, which writes the
PnPCapabilities DWORD behind the two power-management checkboxes. Both resolve
against the devices this machine actually has, checked inside the writing
process rather than taken from the plan. Neither has an apply path of its own —
the Interfaces tab queues them onto the tuning plan, where the preview, the
read-back verification, the typed confirmation and the audit already live. There
is one write path in this app and these use it. Disabling an adapter is
classified as interrupting the network, so the remote-session guard covers it.
The write path itself is unexercised: per AGENTS.md nothing mutates the
development host, so the specifications are covered against fakes and the live
tests stop at resolving an address and reading a value back. Applying belongs in
a VM.
Third: an online bufferbloat result can now be read. Waveform's CSV export and a
JSON report carrying per-sample latencies both parse into the same
LoadedLatencyResult a local run produces, so the grade, the router shaping
advice and the receive-window advice all work on it unchanged. The statistics
are recomputed from the file's own samples rather than copied from its summary:
the sites quote a mean increase and this app grades on the median, so copying
the letter would put a number on a scale it was never measured on. The tool's
own grade is kept beside the derived one, because a disagreement is worth
seeing. Warm-up phases are excluded — the queue is still filling there — and a
probe flagged as lost counts as loss rather than becoming a zero that flatters
the median.
Recommendations no longer require a gaming diagnosis. A measurement is enough,
imported or local, which is what makes the import useful rather than decorative.
Both directions are now held rather than one, so a run that measured download
and upload stops throwing half of itself away. "Send all to tuning plan" queues
every locally-applicable action at once; it stops where the manual path stops,
because the confirmation is what makes any of this reversible.
The importer's tests run against committed fixtures, not against research/,
which is gitignored on purpose and holds real measurements from a real
connection. The fixtures are built to fail loudly if warm-up phases ever get
counted. Two tests still read the private corpus and skip themselves when it is
absent, so they check the fixtures have not drifted from the shape of a genuine
file without breaking CI or a clean clone.
One tooling fix came out of the work: driver.ps1 shot was silently capturing
whatever window was on top. SetForegroundWindow returns true and does nothing
when the caller does not own the foreground, so the driver now shares the input
queue of the current foreground window for the length of the call and throws
rather than saving a screenshot of the wrong thing.
660 tests pass, 12 skipped; 672 with the live and corpus sets enabled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
…probe speak
Four small things, each one a place where the app knew something and put it
somewhere the reader could not get at.
Severity and risk carried their meaning in colour alone. That is nothing to a
reader with a colour vision deficiency, and it is nothing in a greyscale
screenshot — which is how these grids are seen by anyone other than the person
running the app, since a screenshot is what ends up attached to a bug report.
Each verdict now carries a one-character glyph as well: "!" act on this, "~"
borderline, "+" fine, "?" worth a look, "i" context, "-" not measured. Plain
ASCII, so it survives any font and any terminal a copied row is pasted into.
The glyph lives in a separate badge property rather than in the existing
*Display strings, because those are also read out in prose ("good idle, not
playable under load") and written into the HTML export, where a stray
punctuation mark would be noise. TextBlock is not a Control and has no Template,
so the glyph could not be added in the style; putting it in the bound string is
what actually works.
The window reopens where it was left, at the size it was left. It is only
restored when the saved geometry still overlaps an attached monitor: a position
saved on a second display that has since been unplugged would otherwise open the
app somewhere unreachable, recoverable only by editing preferences.json. The
saved size is RestoreBounds rather than the live size, so closing while
maximised remembers both the maximised state and the size to return to. A
corrupted or hand-edited file cannot place the window nowhere: non-finite values
are dropped and anything below the window's own minimum is raised back to it.
Navigation gets keys. Twenty sections is more than a mouse should carry:
Ctrl+1..9 select the first nine selectable tabs, and Ctrl+K jumps to the section
matching whatever is in the global search box. The search box doubles as the
input rather than introducing a palette, because this app has no modals and did
not need its first one to be a launcher.
Finally, --probe. It stays exactly as it is — a manual, read-only capture for
contributors who have read the README — but it no longer ends on a message box.
It attaches to the calling console and prints the report path there, which is
where the README already tells people to run it from, and exits. The modal is
kept for whoever double-clicked the exe and has no console to read; that case is
detected rather than assumed. This also retires a documented gotcha: an
automated probe run used to leave a dialog no automation could dismiss and a
process holding it open. Verified: the report path prints, and no SockTuner
process is left behind.
670 tests pass, 12 skipped. The window geometry rules are covered, including the
unplugged-monitor case that is the only one that actually strands anybody.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
…ill applied Two additions, both about the gap between "SockTuner said so" and "it is still true". The recommendations now export as a Markdown checklist. The router half is the reason: those instructions name a parameter, a value and the reason for that value, and the person acting on them is standing in a different device's web interface, or in an SSH session, while they do it. A panel inside a WPF window is the one place that is no use for that. Markdown because it stays readable as plain text, pastes into an issue unchanged, and diffs when the advice is regenerated after a second measurement. Every item keeps its verification step, and each OpenWrt instruction is also emitted as a uci command that can be pasted as it stands. The values come from this app's own catalogue rather than from user input, but a generated shell line is not the place to rely on that, so a value containing a quote is escaped — tested. The export states plainly that nothing in it has been applied yet: the file outlives the session and may be read by someone else. Second, drift. A driver update reinstalls the INF and restores every advanced property to its default. So does a network reset, a vendor utility, and a second tuning tool. None of them announce it, so a user believes a setting is applied because they applied it once, and the app agrees because its audit says it wrote it. The audit records what was written; only a read-back says what is there. "Check for drift" on the tuning plan reads back every setting SockTuner has written and reports the ones that no longer hold it. The expectation is the most recent successful transaction per setting and target, apply or rollback: a rollback is the app deliberately putting a value back, so treating the earlier apply as the expectation would report the app's own rollback as drift. Failed transactions establish nothing and are ignored. A keyword the driver no longer advertises is reported as unreadable rather than quietly counted as unchanged — the setting is gone, not merely different — and absent never matches present, because those are different states rather than different values. The comparison is pure and tested against a fake reader; the UI reads through the same read-only stores the plan preview uses, so the check itself cannot write. 691 tests pass, 12 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
…vises shaping Two features that meet in the middle: one measures whether tuning worked, the other supplies the piece the router advice has been asking for. First, verification. Every tuning tool tells you what it changed and almost none tell you whether it worked, which is how a folklore setting survives fifteen years. Applying a plan now freezes the diagnosis that preceded it as a baseline, and "Re-run and compare" repeats that diagnosis with exactly its parameters — restored into the controls, so what runs is visibly the same run rather than a hidden one — and reports what moved. What makes it worth having is the noise floor. The comparison already existed here; it had no notion of significance, so a 0.3 ms difference between two runs read as an improvement when it was the same measurement twice. The floor is derived rather than chosen: Windows times an ICMP round trip in whole milliseconds, so anything under one is below the instrument no matter how many samples were taken, and a path that swings by its own jitter between packets swings by about that much between runs. The floor is the larger of the two. Loss is counted rather than timed, so its floor is what a single lost packet represents in the baseline — on a hundred probes, a one per cent move is one packet and not a result. Better in one place and worse in another is reported as a trade rather than resolved into a winner. Turning interrupt moderation off really does buy latency and cost jitter on some drivers, and deciding which matters more is the user's call. A genuine regression says so and points at the audit history, which holds the values from before. And an improvement says to run it again before believing it, because one pair of runs is one pair of runs. Second, QoS. The tab was read-only, and the app's own router guidance says to move to layer_cake.qos only if traffic is actually DSCP-marked — advice it could not act on, because nothing here could mark anything. A policy can now be created for one application: protocol, remote ports and DSCP, written in the documented Group Policy format, as a whole key or not at all. Absent is the real "no policy" state, so a rollback removes it rather than blanking its fields. Two constraints matter more than the feature. SockTuner only manages policies it created: every name it accepts carries a fixed prefix and anything else is refused, so a plan cannot be pointed at a policy an administrator deployed and delete it. And the name becomes a registry subkey, so it is restricted to characters that cannot traverse the path rather than merely escaped — both tested, including the traversal attempts. The trade-off text refuses to sell it. A DSCP mark is a request, not a reservation: it does nothing until something on the path is configured to honour it, in practice your own router, and most consumer ISPs rewrite or ignore the field beyond the access link. A test asserts that sentence stays there, because "QoS" is the word this kind of tool is usually sold on. Neither write path applies anything itself. Both queue onto the tuning plan, where the preview, the typed confirmation, the read-back and the audit already live. Per AGENTS.md the QoS write is unexercised on this host: the specification is covered against fakes, and applying belongs in a VM. 735 tests pass, 12 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
The adapter and QoS writes shipped covered only against fakes, because AGENTS.md forbids mutating the development host and a specification test proves what the code believes rather than what Windows accepts. --verify-device-writes closes that, the same way --verify-tcp-writes already did for the TCP templates: it applies each device-level setting for real and puts it back, through the same snapshot, apply, read-back and rollback engine the tuning plan uses. Exercising a different path would prove nothing about the one that ships. It is gated behind the existing SOCKTUNER_VM_WRITE_TEST=1 rather than a new switch, so the rule stays one rule: a mode that writes needs the guest to be armed deliberately, and the argument alone is never enough. What it attempts is chosen by what it can afford to get wrong. A QoS policy only marks packets and is created and removed under a name this app owns. The power-management DWORD takes effect at the adapter's next restart, which this run deliberately does not trigger, so writing it drops nothing. Disabling an adapter is not safe anywhere, so it runs only against an adapter that is up and carries no default route, and never against the one carrying traffic; on a machine with a single NIC it is skipped with a note saying to add a second one. A validation run that strands the machine it was validating is worse than an untested path. A refusal is recorded as a result rather than thrown, because finding out what the platform declines is the point. A setting that is not restored is the headline of the verdict rather than a footnote — a machine left changed by a validation run is the worst thing this can produce, so it is the first thing anybody reads, and it names the checkpoint as the way out. The report is written as JSON next to the TCP one. Adapter targets are interface GUIDs, which identify a NIC on that machine and nothing about a person, so there is nothing to redact. Staged against D:\VmLab\SockTuner-Win11-Base: checkpointed, a spare NIC hot-added so the enable/disable path has something safe to act on, and the self-contained build copied in. Running it needs guest credentials, which this session does not hold; PowerShell Direct is reachable and answers "the credential is invalid", so that is the only step outstanding. 735 tests pass, 12 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
The validation run did what a specification test cannot: it used the write path against real Windows, and the write path did not work. Three defects, two of them in code that already shipped. The first is the serious one. CimAdapterSettingStore fetched an adapter with `SELECT InterfaceGuid FROM MSFT_NetAdapter` and then called InvokeMethod on the result. A WQL projection that omits the key properties returns objects whose __PATH is empty, and a method call on a pathless object throws "Operation is not valid due to the current state of the object". That exception was caught by the restart loop and recorded as a warning, so applying an NDIS keyword has been reporting a restart warning instead of restarting the adapter — which means the value did not take effect until the machine was rebooted, and the app said so in a sentence nobody had reason to read as a failure. Both stores now select the whole row, with a comment saying why a narrower select is not an optimisation, and a test fails if anyone narrows it again. The second was hiding behind the first. MSFT_NetAdapter declares its methods as `Disable(Instance CmdletOutput)` — they take a parameter — so the two-argument `InvokeMethod(name, null)` throws a NullReferenceException before reaching WMI at all. The parameters object has to be built from the class definition even when nothing is passed in. Restart had the same bug, stacked behind the same call, so the restart path was broken twice over. A non-zero ReturnValue is now raised rather than ignored, since a method that reports failure while the call succeeds would look exactly like a write that worked. The third came from checking the machine rather than the report. Every setting reported "restored", and the read-back verification agreed, because it only ever looks at the setting's own value. Comparing the guest's registry either side of the run showed an empty `…\Windows\QoS` container left behind: writing the first policy creates it as part of the path, and removing the policy only removed the policy. A key with no subkeys and no values carries no information, so it is now removed too; a container with anything in it is left alone. The verifier also skipped power management entirely on the first run, because it looked for a physical adapter and a VM has none. The setting cares about the class key, not the classification, so that is what it asks for now. Result on Windows 11 26200, after the fixes: three of three settings written, verified by read-back and restored exactly — and the guest's adapter states, all twelve PnPCapabilities values and the QoS hive are byte-identical to a capture taken before the run. The skill file gains the procedure and four gotchas that cost real time: a stale VM registration breaks every -Name Hyper-V cmdlet on this host so the object has to be piped; PowerShell Direct runs over VMBus and therefore survives disabling the guest's networking, which is what makes this test safe to run remotely; a console-mode run has no console over PSDirect, so the completion message falls back to a modal nothing can dismiss and the JSON has to be polled for instead; and the report must not be trusted on its own, because it is what missed the third defect. 739 tests pass, 12 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
A second model reviewed the three write paths read-only. Eleven findings; these are the ones verified against the code and acted on. The rest are recorded below rather than quietly dropped. The one that mattered most was mine, and my own test was hiding it. A rollback audit entry stores the apply's snapshot unreversed — SaveRollback persists the same snapshot and RestoreAsync writes each change's Before — so after a successful rollback the machine holds Before, not After. The drift analyzer read After for every successful entry, which inverted every verdict following a rollback: it called the restored value drift and the undone value "holding". The test that should have caught this passed, because I had fabricated a reversed rollback entry in the fixture, a shape the store never produces. The fixture was wrong in exactly the way the code was wrong, so the two agreed. That test is deleted rather than corrected, and replaced with one built from the real shape. Adapter state is now refused in the writing process when the target holds a default route. The rule existed only in the surface that offers the change, and SECURITY.md has the elevated worker treat its caller as compromised — so a rule the UI enforces is not enforced. The store also now re-resolves its address, as the other two already did; checking only the setting id would let a forged address name any adapter WMI can match. The QoS cleanup added in the previous commit is reverted. Removing the shared …\Windows\QoS container when it ends up empty does return the machine to the state it was found in, but this app cannot tell a container it created from one that was already there, and a key can carry an ACL or be relied on by something else. Leaving an inert empty container is the smaller wrong. The verifier now says so in its own report, so the difference a reader will see comparing the registry either side of a run is a documented one rather than a mystery. Power-management values must now be canonical: "024" passed validation, was written as a DWORD and read back as "24", failing verification for no reason a user could act on. And a registry or WMI read failure during a drift check now becomes an Unreadable result instead of escaping and taking the whole report with it. Re-validated in the VM after each change. The default-route refusal fired correctly when both of the guest's adapters carried a route; with the spare NIC moved to an isolated switch so it is up and carries nothing, all three settings write, verify and restore, and the guest's adapter states and every PnPCapabilities value are identical to a capture taken before the run. The only difference is the QoS container, which is now the documented one. Not acted on, and worth a look: rolling back a QoS policy writes this app's four fields rather than restoring companion values it did not record, and a pre-existing key carrying our name prefix but no DSCP value would be deleted outright; the writer omits the documented Local IP Prefix Length and Remote IP Prefix entries, and treats a missing Protocol or Remote Port as "*", so read-back can pass against a corrupted policy; a pre-existing PnPCapabilities of the wrong registry kind reads as absent and is deleted rather than restored; an adapter in the Testing administrative state is snapshotted as Disabled; and the registry writes within one policy are not atomic. 743 tests pass, 12 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EkaiB8KHRYeZTbQVTw4TU8
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PrimeBuild-pc
deleted the
feature/interfaces-and-external-bufferbloat
branch
September 2, 2026 00:58
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
1 participant
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.
Seven commits. Three strands of feature work, then the write paths proven in a VM and a second-model review acted on.
Interface
Every proportional grid column had no floor, so once the fixed columns filled the viewport the star columns collapsed — the adapter Description rendered ~25px wide and the tuning plan's accepted values and trade-off were two characters each, exactly where a value is chosen. Star columns now carry a minimum and the grid scrolls; every text cell carries its full value as a tooltip. Health findings wrap instead of clipping, verdicts and risks are coloured and glyphed so they survive a greyscale screenshot, and the 18 flat navigation entries are grouped. The window remembers its size and position; F5, Ctrl+F, Ctrl+K and Ctrl+1..9 work.
Network devices
A new Interfaces tab lists the real devices — filter, loopback and Windows' own RAS plumbing excluded, matched by the INFs that install it. Each gets a role, a verdict and its evidence. One rule is a refusal, not advice: the adapter carrying the default route is never offered for disabling.
Two device settings join the typed catalogue:
adapter.stateandadapter.power-saving. Neither applies anything itself — both queue onto the tuning plan, where preview, typed confirmation, read-back and audit already live.External bufferbloat results
Waveform's CSV export and a JSON report with per-sample latencies both parse into the same
LoadedLatencyResulta local run produces, so the grade, the router shaping advice and the receive-window advice work on them unchanged. Statistics are recomputed from the file's own samples rather than copied from its summary — the sites quote a mean, this app grades on the median. Recommendations no longer require a gaming diagnosis, and export as a Markdown checklist with paste-readyucicommands.Also: verify-after-apply with a derived noise floor, drift detection against the audit, and QoS/DSCP policy creation.
Validated, not assumed
--verify-device-writesapplies each device setting for real in a VM and puts it back. It found three defects, two already shipped:CimAdapterSettingStorefetched adapters with a WQL projection, which returns objects with an empty__PATH; everyInvokeMethodon them fails. The restart loop caught it as a warning, so applying an NDIS keyword never restarted the adapter and the value did not take effect until a reboot.MSFT_NetAdapterdeclaresDisable(Instance CmdletOutput)— the methods take a parameter, soInvokeMethod(name, null)threw before reaching WMI.Restarthad the same bug behind the same call.Result on Windows 11 26200: three of three settings written, verified by read-back and restored exactly; adapter states and all twelve
PnPCapabilitiesbyte-identical to a capture taken before the run.Review
A second model reviewed the write paths. The most serious finding was that a rollback audit entry stores the snapshot unreversed, so the drift analyzer — reading
Afterfor every entry — inverted every verdict after a rollback. The test that should have caught it passed, because its fixture was wrong in the same way the code was. Also fixed: the default-route refusal now lives in the writing process rather than only the UI,AdapterStateStorere-resolves its address, power values must be canonical, and a registry read failure becomes a result instead of aborting the drift report. Six findings remain open and are listed in the final commit.743 tests pass, 12 skipped.