Make the two standing quality thresholds ratchet instead of sit still - #485
Open
JE-Chen wants to merge 19 commits into
Open
Make the two standing quality thresholds ratchet instead of sit still#485JE-Chen wants to merge 19 commits into
JE-Chen wants to merge 19 commits into
Conversation
Their own reason said what they needed: "needs subprocess isolation (see test_actions_menu_gui) ... skip until then". They cover real wiring — a file received on a WebRTC worker thread reaching the GUI thread through a queued signal rather than a thread-affine singleShot, and the admin console's thumbnail poll deleting its QThread each tick instead of leaking one per interval. Skipping was right at the time. Building the WebRTC panel or the admin console and then tearing a worker QThread down aborts the shared pytest process under offscreen Qt, and since deleteLater is a no-op until an event loop runs, the abort lands in some later unrelated file with no traceback. Run all three in one child process that writes a JSON verdict per check and exits without teardown, the shape test_actions_menu_gui already uses. Verdicts are ok / failed / unavailable, so a machine without the webrtc extra reports a skip while one that has it checks the wiring. Confirmed to have teeth rather than assumed: dropping thread.finished.connect(thread.deleteLater) turns the third verdict into "failed: the QThread outlived finish" and leaves the other two green.
Both were promises living only in a pyproject.toml comment, with nothing that would ever move them. Coverage: fail_under was the first measured baseline and never moved while the suite grew past it, leaving ~15 points unguarded — every square of the matrix clears 50% and CI would still have passed a change deleting a third of the tests. The floor is now the lowest square (50.26%, ubuntu-22.04/3.10), and the comment states the rule that was missing. mypy: the scope was two directories, and a path list only grows when someone remembers to grow it — a new module lands outside it by default. The scope is now the whole package minus a shrink-only list, so 862 of 1,017 files are in the contract and new modules join it on arrival. The verify script fails when a listed module starts passing as loudly as when an unlisted one stops. It also runs win32/linux/darwin rather than only the runner's platform, so the Windows and macOS backends are checked from Ubuntu; 13 modules fail only under a Linux target and 3 only under Windows. The gate has to mean one thing everywhere: a dev checkout with [gui]/[webrtc] disagreed with a bare install about 38 modules, so every non-base third-party module is forced to Any. ignore_missing_imports alone still reads the real package when installed, and follow_imports = "skip" is ignored for .pyi files — PySide6 ships inline stubs, so follow_imports_for_stubs is required too, the same trap the numpy override had already paid for.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 77 |
| Duplication | -1 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codacy honours a nosemgrep marker only on the exact line it reports, and the audit rule reports the call line. Same shape as adb_client.py: argv is a list of literals plus one value from the module-level PLATFORMS tuple, no shell.
platform_wrapper re-exports eight names and everything above it is written against those names, but nothing said what they were. mypy bound each one to whichever branch it read first — always the Windows one, on every target — so the layer above the seam was checked against Win32 signatures even on Linux and macOS, and a backend could omit a member entirely without a word. The names are now declared before the branches bind them, three of them with Protocols in wrapper/backend_contract.py, and each _platform_* module annotates what it assigns, so an incomplete backend fails in its own file naming the missing member. keyboard and mouse stay Any: their call shape really is per-platform, and Progress.md records what a per-platform protocol would cost. Turning it on surfaced four None-reaching-a-non-optional bugs and one backend disagreeing with the seam it implements; both are in CHANGELOG.md. Six wrapper modules leave the typing exemption list, and four more outside the cluster went green on their own — they had been failing on the seam's accidental types.
Thirty-three of the forty errors were one sentence: libei reads its resolved entry points straight off an Optional[BoundSymbols]. Every one of those reads is guarded — connect() refuses an unavailable backend, _emit a disconnected one — but the guarantee lived in the call graph rather than anywhere a reader or a checker could see it, and a new call site that skipped a guard would raise AttributeError, which is not an AutoControlException and so escapes every containment boundary. They now go through one _api property that raises LibeiUnavailable, which is what this module says every failure in it raises. _teardown keeps the raw attribute on purpose: it runs from an except BaseException handler, where raising would replace the real failure. The other three were each one honest disagreement — environment probes typed dict while defaulting to os.environ, a writer declared to return None while every caller returns the tool's stdout, and Pillow's getpixel union handed to callers that unpack three ints. No behaviour changes.
Five errors across four modules, one of them a live bug. The X11 listener's record_queue was assigned None with no annotation, so its type *was* None: record() could not fill it and stop_record() promised a Queue while able to return the None it started as. Stopping a recording that was never started reached x11_linux_record, which reads .queue off the result — an AttributeError a frame away from the mistake. It returns an empty queue now, matching what the public stop_record() already returns for the same case. osx_keyboard.press_key tested `keycode in special_key_table`, which narrows the string case into the branch but leaves int | str outside it, so an unknown name reached Quartz as a keycode. A string only ever names a special key here. uinput/_device's O_NONBLOCK is POSIX-only and the contract checks a Windows target too; the flag moved into a sys.platform branch mypy can prune.
Two of these are live defects rather than annotations. window_message did `from ...windows_window_manage import FindWindowW`, and that module exports no such name — FindWindowW is a method on its private user32 handle — so importing window_message raised ImportError on every Windows machine. Nothing caught it because the only importer in the tree is a manual test. It calls the public get_one_window_hwnd now, which is also the one that declares HWND-width argtypes. win32_ctype_input wrote ULONG_PTR into the standard library's own ctypes.wintypes namespace. The name appears exactly once in the tree, on that line, so the only thing the assignment could do was answer for another library in the same process. Its `_fields_: tuple` annotations also redeclared a ctypes ClassVar as an instance variable, and ctypes.POINTER and user32.SendInput were used as types when one is a function and the other a value. Every module under windows/ now type-checks on the win32 target. Eight stay exempt for a reason that is not about them, measured and left in Progress.md as a DECIDE: the gate also checks them as Linux and macOS, where typeshed does not declare the Win32-only corner of ctypes.
The macOS 3.14 grid cell failed on `sent.count("doc.txt") == 1` seeing 2:
the test edited the file in place and then pushed its mtime forward, so
between `write_text` and `os.utime` the file briefly carried a third,
intermediate mtime. A tick landing in that window pushed the file once
for the intermediate mtime and again for the final one — correct engine
behaviour, an over-specified test. Stage the new content outside the
watch dir and swap it in with `os.replace` so one edit is one event.
The other half was the same guess in the other direction: every test
slept a fixed 0.4s hoping the baseline snapshot had been taken, but the
worker takes it asynchronously, so a slow runner could fold the test's
own edit into the baseline and push nothing at all. `wait_until_ready()`
makes that observable, and it is not test-only scaffolding — a caller
that drops files right after `start()` has the same race.
Thirteen modules, 169 errors, and only two shapes between them. Nine were `self._x = None` with no annotation — which makes the attribute's *type* `None`, so every later assignment is "incompatible types" and every later read is "None has no attribute". Several already carried the intended type in a trailing comment; those comments are now annotations, with the classes imported under TYPE_CHECKING so the lazy runtime imports stay lazy. Where the attribute is read back through a closure the receiver binds to a local first, because a closure re-reads the attribute and no narrowing survives that. The other four were mixins reading attributes they do not own. Each one's docstring already listed what it borrows from the host class it is mixed into; that list is now an `if TYPE_CHECKING:` block in the class body, which the runtime strips, so a declaration there cannot shadow what the host binds. Three of the fixes are behaviour rather than annotation: * `WebRTCLoopBridge._run` read the loop back off shared state after `start()` set it. The loop is passed to the thread instead, and `start()` returns it, so submit/call_soon hold what they use. * `_get_cursor_position` did `import sys as _sys` inside the function, which mypy does not recognise as a platform test — so its Win32 branch was checked against Linux and macOS as well. * `totp` caught `base64.binascii.Error`, a name that exists only because `base64` imports binascii itself. It imports binascii by name now.
The accessibility backends, observability, the triggers, chatops, the REST server, the MCP HTTP transport and element_repository, together, because they kept hitting the same causes. Thirty-four of the forty-three accessibility errors were one missing return annotation: `_unsupported` always raises, so every method whose last statement is a call to it looked like it could fall through. It is `NoReturn` now. A catch tuple that is not typed as one catches nothing as far as mypy is concerned. `_uia_errors()` returned `Tuple[type, ...]`, which is not "a tuple of exception classes", so all five `except UIA_ERRORS` sites were errors — on a tuple that exists precisely because `comtypes.COMError` inherits from `Exception` and nothing else. `rest_server` and `chatops.router` had the same shape via `except (…, *SQLITE_ERRORS)`, which mypy cannot follow into an `except`; both now name the set once as an annotated module constant, as `mcp_server._protocol` already did. `parse_content_length` declared `Mapping[str, str]` and no caller ever passed one: all three hand it `BaseHTTPRequestHandler.headers`, an `email.message.Message`, which is not a mapping over its keys and which matches header names case-insensitively — the property that makes `Content-length` work. It takes a one-method protocol now. Four fixes are behaviour: * `Gauge` and `Histogram` borrowed `Counter._labels_key` by assignment, so a gauge's label typo was validated by a method declared to take a counter. The rule is the same for all three and moved to the base, which also gives the registry's `render()` loop a base to call. * IMAP UIDs were decoded at two of the three places that needed them. They are decoded once on arrival now, so the fetch, the mark-seen and the seen-set all speak one type. * `ElementRepository` passed a user-editable locator to the accessibility API as `**kwargs`, so a field that is not a filter came back as a TypeError about keyword arguments from inside the backend. * `_AtspiConnection._call` had no bus to call outside its `with`. `_process_name` is a kernel32 round trip and now says so with a `sys.platform` guard, instead of being type-checked against Linux.
Re-measuring the DECIDE found it was twice the size it recorded and a different shape. Sixteen modules — not eight — pass on --platform win32 and fail on the other two targets for exactly one reason: typeshed declares windll / WinDLL / WINFUNCTYPE / WinError / get_last_error on Windows only. Half of them are nowhere near je_auto_control/windows/ (trash, app_idle, file_assoc, idle_keepawake, lock_session, session_guard, usb/passthrough/key_provider, gui/main_window), which rules out the option the entry had recommended: "measure a platform module on its own platform" cannot be a directory rule when half the affected modules are not in a platform directory. The maintainer chose per-site suppression. It came to 28 lines, not the 58 the entry projected — 58 counted each source line once per non-win32 target. Every one carries its own reason; none is blanket. Two things had to be measured. mypy honours `# type: ignore` only as the first comment on the line, so the two lines that already carried a `# nosec B607` take the marker first and merge the two justifications. And nine lines could not hold the marker inside the 120-char limit, so they are reformatted rather than shortened into meaninglessness: the marker follows an opening paren, and two sites hoist a value into a local first (`last_error` in the DPAPI wrapper, `kernel32` in the input hook), which reads better than the one-liner did. All sixteen were re-imported and exercised on Windows afterwards — a reformat only a type checker has verified is a reformat nobody has.
Both were the same mistake told two ways — a handle whose declared type cannot do what the code asks of it — and both hid a real defect. `winusb_backend._load_dlls` published its three DLL handles one at a time behind `if _setupapi is not None: return`. A failure loading winusb.dll — what happens on a machine with no WinUSB-bound device — left `_setupapi` set, so the guard short-circuited every later attempt and the call sites got `AttributeError: 'NoneType' object has no attribute 'WinUsb_Initialize'` instead of the retry the guard exists to allow. The three now come back from one loader as a NamedTuple, published only after all three load. With them: a device enumerated without an interface path is skipped rather than passed to CreateFileW as None, and a WinUsb_Initialize that reports success with a null handle is a failure rather than an Optional[int] handed to the wrapper. `clipboard_api()` returned `Tuple[object, object]`. `object` has no attributes, so all twelve calls through it were errors — in the one module whose whole docstring is about declaring these prototypes correctly. A ctypes library resolves symbols through `__getattr__`, so `Any` is the only honest promise and is what it says now. Both were exercised against the real thing on Windows: a clipboard text round trip, `clipboard_formats()` on a live clipboard, and the WinUSB backend enumerating an actual bound device.
Thirteen of the fourteen gui modules. Most of the eighty-nine errors were the mixin shape this branch has now fixed three times: six tab mixins read `_tr`, `_translate` and `timer` off a host they never declared, and each said so in its own docstring. Those docstrings are declarations now, in a TYPE_CHECKING block the runtime strips. Three of the fixes fail for a user, not for a checker: * A pixel assertion with one coordinate reported the wrong problem. `assert_pixel(*_parse_ints(xy)[:2], _parse_ints(rgb), match=…)` binds the RGB list to `y` when the star-unpack yields one item, so `match` and `raise_on_fail` collide with positional slots and the user gets a TypeError about duplicate keyword arguments. The count is checked first now, and the message names what is missing. * `_get_mouse_pos` unpacked a value the Windows backend really returns as None — `GetCursorPos` fails on a locked or secure desktop — so the warning box read "cannot unpack non-sequence NoneType object". It raises AutoControlException with a sentence instead. * `multi_language_wrapper` typed its listeners `List[callable]`, the builtin function rather than a type, so every `listener(language)` read as calling something not callable. `recording_edit.editor` came with them: both optional parameters were written `end: int = None`. webrtc_panel.py is the one that stayed. Seven of its errors come from `_build_advanced_group(panel: TranslatableMixin, …)`, which writes five widget attributes back onto a panel that has none of them; the honest type is a Protocol, and the file sits exactly on its 2,555-line cap. Progress.md now records that its exemption is blocked on the split that file already owes — moving that builder out settles both at once.
`AC_normalize_url` and `ac_normalize_url` both forward to
`url_canon.normalize_url` and both passed `drop_fragment=` — the name
they expose to callers. The function's parameter is `strip_fragment`, so
every call raised TypeError, in the executor, in the MCP tool, and from
the Script Builder field that feeds them. The outward name stays (it is
in the action schema and the tool registry); the two call sites pass it
under the name the callee uses. Measured: the MCP tool now returns
{"url": "https://example.com/b"} where it returned an error.
The rest of the cluster was shapes this branch keeps meeting:
* ClientRequestMixin borrowed seven attributes from MCPServer and listed
all seven in its docstring; that list is a declaration now.
* `_DISPATCH_ERRORS` and `_TOOL_INVOKE_ERRORS` — the containment
boundary for the whole stdio loop — were not typed as tuples of
exception classes, so all three `except` sites were errors.
* `_dispatch` fed an Optional[str] method name to `dict.get`. A request
with no method takes the not-found branch explicitly now, with the
same -32601 body it produced by falling through.
* The subscription callback was a default-argument lambda, which mypy
cannot infer against a `Callable[[], None]`. `functools.partial` binds
the same value and states the type.
Two public return annotations were wrong in the safe direction:
`set_mouse_position` and `hotkey` say `... | None` while every path
returns the tuple or raises. Narrowing them is what let the MCP handlers
stop indexing an Optional. `get_mouse_position` keeps its `| None` — the
Windows backend really does return that when GetCursorPos fails.
Thirteen modules of two to six errors each. Three recurring shapes: `callable` used as a type (it is the builtin function, so mypy reads every call through it as calling something not callable — six times in plugin_loader, once in each hotkey backend); `x: SomeType = None` defaults; and tuples that lost their length, where `tuple(r)` and `cv2.boundingRect()` are `tuple[int, ...]` and the fields they feed promise four ints. Two are more than annotations: * `_StabilityTracker` read `now - self._since` on a path where `_since` is non-None only because of what an earlier call did. True today, invisible to a reader, one refactor from a TypeError inside the poll loop. It binds the value and treats "no start time" as "not stable". * `act_when_ready` passed `report.point` to a callback that requires a point. `point` is None whenever the target is invisible, and the guard above tests `report.actionable` — which implies visible, again only through the call graph. The point is checked where it is used. `text_regions` needed one justified suppression, and it is worth recording why: `cv2.MSER_create` exists at runtime in every supported OpenCV but is absent from the .pyi opencv-python ships (measured on 4.13.0). Progress.md now notes the consequence — the mypy config lists cv2 as a base dependency that ships no stubs, but it ships one, so the gate reads it and its verdict can move with the OpenCV version inside the >=4.8,<6 pin.
Eight errors, and two of them were the dispatch table disagreeing with itself. `_android_client_cache` is keyed by the `(serial, adb_path)` pair but declared `Dict[Optional[str], Any]`, and `backend_obj` was assigned an Anthropic backend on one branch and an OpenAI one on the other, so mypy read the second as an error rather than as the sibling implementation of `AgentBackend` it is. `_unwrap_action_list` reassigned its own parameter to the result of `dict.get`, widening a `Union[list, dict]` to include None; it unwraps through a local now, which is also what the `isinstance` checks below it were already assuming. `ensure_state`'s `StateSetter` promised `Callable[[Any], None]` while every real setter in the tree returns a bool the function then ignores — it re-reads instead of trusting what a setter reports, which is the whole point of the helper. The alias says `Any` now, and the comment says why. `RedactionPolicy.with_extra_regions` likewise: it coerces its extras to four ints internally, so it can accept the sequences callers actually have.
`cursor.lastrowid` is None until an INSERT has run on that cursor, and three subsystems returned `int(cursor.lastrowid)` straight out of an insert helper — work_queue, agent_memory and run_history. They now go through `sqlite_support.last_row_id`, which says what it means when the id is not there instead of handing None on as a row id. The rest of the batch is the shapes the tail keeps repeating: * Three `__exit__` methods annotated `-> bool` while every path returns False. `Literal[False]` is what they do. * Five implicit-Optional defaults (`x: str = None`). * `base64.binascii.Error` again, in the admin client — the same import side effect `totp` was catching. * `DeterministicRun._freeze_clock` multiplied `self._freeze_time` by 1e9 without checking it; its only caller guards on the same attribute, so the invariant was real but lived one frame away.
Forty-one modules in one sweep, and the list is now a single line: `gui/remote_desktop/webrtc_panel.py`, blocked on the split its size cap already owes it (recorded in Progress.md). Several were annotations that were simply wrong about what the code returns, and fixing them is what let the callers stop working around it: * `screenshot()` said `-> List[int]` while it returns the captured BGR frame. Every caller that read `.shape` off it was reading through a lie; `screen_record` was the one mypy caught. * `visual_match._nms` was typed for `Match` and reused from `rotated_match`, `edge_match` and `color_match` with their own record types. It takes a `TypeVar` bound to a read-only Protocol of what it actually reads — position, size, score — which is also why the bound had to be properties: the records are frozen dataclasses, and a Protocol attribute demands a writable one. * `logical_frame`'s injectable grabber was typed `Callable[..., Any]` while its own docstring says "ImageGrab-shaped object", so `.grab()` on it was an error. Four fix a `None` the code could really see: `humanize.motion` and `watcher` unpacked `get_mouse_position()` (the Windows backend returns None on a locked desktop), `dag.runner` opened a node's `action_file` without checking a node had one, and `color_match` divided a `None` accumulator when handed no channels. `secret_store._require_unlocked` now returns the `(fernet, vault)` pair it checks, which removed three `type: ignore`s that existed only because the check and the use were separate statements. Two more cv2 names — `VideoWriter_fourcc` and `ORB_create` — join `MSER_create` as present at runtime but absent from the shipped stub.
`je_auto_control` type-checks clean on win32, linux and darwin with nothing exempted. The list that started this branch at 155 modules now holds a header and no entries, and the gate fails if it grows again. The last module was webrtc_panel.py, blocked by its size rather than its types. Seven of its errors came from `_build_advanced_group(panel: TranslatableMixin, …)`, a free function that writes five widget attributes back onto the panel it is handed — none of which TranslatableMixin has. Saying that needs a Protocol, and the file was sitting exactly on the 2,555-line cap it may only shrink from. So the builder moved out, which is the split Progress.md already said that file owed: `advanced_group.py` holds the shared STUN/TURN group, the `AdvancedGroupHost` protocol naming what it reads and what it sets, and the hardware-codec row as its own function. The panel is 2,545 lines — under its cap for the first time — and both panels were rebuilt offscreen to confirm the STUN default, the TURN fields and the host-only codec picker still arrive where they did. Ten more errors there were the pattern the whole sweep kept meeting: a handle that is None until the session starts. `_produce_offer`, `_trust_session_viewer`, `_answer_and_push`, `_produce_answer` and the folder sync all reached through `_multi_host` / `_viewer` without asking. They go through `_require_multi_host()` / `_require_viewer()` now, raising a translated "start hosting first" / "connect to a host first" rather than an AttributeError on None — two new keys in all four language catalogues. Progress.md's mypy entry collapses to what is still worth knowing: the five recurring shapes, that a catch tuple has to be an annotated module constant, that `# type: ignore` only counts as a line's first comment, and that the cv2 stub omits names the library has.
|
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.
Closes the
Progress.mdentry "two thresholds we agreed to climb and never did".Both were promises that lived only in a
pyproject.tomlcomment, with nothinganywhere that would ever move them.
Coverage: the floor was 15 points below what the tests already earn
fail_under = 35was the first measured baseline and then never moved while thesuite grew past it. Every square of the nine-way matrix clears 50% — measured on
the run for #484, from 50.26% (ubuntu-22.04 / 3.10) to 51.69%
(windows-2022 / 3.14) — so CI would have passed a change that deleted a third of
the tests without a word.
The floor is now 50, taken from the lowest square rather than from one
machine, and the comment states the rule that was missing: this is a ratchet,
raised whenever the suite has earned it. 70 is still the destination.
mypy: the scope was 4 files, and could only ever grow by hand
The job checked
je_auto_control/apiandje_auto_control/utils/failure_bundlewhile a comment promised the rest would join "the contract" later. A scope
written as a path list never grows on its own, and every new module lands
outside it by default.
So the scope is inverted: mypy checks the whole package, and the modules that
do not pass yet are named in
test/verify/typing_contract_exempt.txt.inside it the moment it is written.
typing_contract_verify.pyfails when a listedmodule starts passing (delete the line) as loudly as when an unlisted one
stops. Both directions were checked by breaking them.
--fixre-measures the list, so shrinking it is one command.It checks three platforms, not one
mypy resolves
sys.platformbranches against a single target, so the Ubuntu-onlyjob never looked inside the Windows, macOS or platform-gated code — three of the
four backends. That blind spot was real and is now measured: 13 modules fail
only when the target is Linux, 3 only when it is Windows. The script runs
win32,linuxanddarwinand unions the results.The part that took the work: the gate has to mean one thing
A first measurement taken on a dev checkout disagreed with a bare
pip install -e .about 38 modules — 36 Qt modules that pass only becausePySide6 is absent, and 2 that fail only because
babelandpytestare.Committing that list would have reddened CI on arrival and mis-listed the other
36. A gate that flips on
pip installis not a gate.Every third-party module outside the base dependency set is therefore forced to
Any. Two settings were needed, not one:ignore_missing_importsalone is not enough — it still lets mypy read the realpackage when it is installed, which is the environment-dependence itself.
follow_imports = "skip"is silently ignored for.pyifiles. PySide6 shipsinline stubs, so
follow_imports_for_stubswas required too — the same trapthe numpy override in
pyproject.tomlhad already paid for and written down.Verified identical results across three dependency sets: dev checkout with
[gui]+[webrtc], a barepip install -e .(CI's exact set), and bare pluspython-Xlib(the only package a Linux host adds).Verification
architecture_explore.mdupdated to match.typing-stable-apijob id is deliberately unchanged — it is a requiredcheck, and renaming it would silently drop the requirement.
The remaining 155 modules cluster (
utils/remote_desktop13,gui11,wrapper5);Progress.mdnow records clearing them one cluster at a timeinstead of "no plan in between".