Remove dead code and enforce two guards that only claimed to exist - #67
Conversation
An audit pass over the codebase. The mechanical detectors came back almost
empty -- ruff clean under the project config, pyflakes and bugbear both zero,
vulture finding nothing above 60% confidence -- so this is the small set that
survived verification, not a sweep.
Dead:
magisk_system._list_dir a thin wrapper over _list_dir_typed with no
production caller. It existed only because a test
tested it. That test's real coverage (skipping
"." / ".." and stripping symlink targets) was not
duplicated in the typed test, so it moved there
rather than being deleted with the wrapper.
MODULE_SIZE (x2) a pin nothing validated. SHA-256 already covers
integrity, so a byte count added nothing.
Claimed but not implemented:
MIN_MAGISK_VER_CODE its own comment said it was "surfaced here so a
Magisk version bump that dips below is caught
rather than failing mid-flash" -- and nothing read
it. ReZygisk's customize.sh aborts partway through
its own install when the daemon is too old, which
is exactly the half-written state the constant
claimed to prevent. install_module now takes
min_magisk_ver_code and refuses before anything is
pushed. An UNREADABLE version still proceeds: a
gate that fired on "couldn't tell" would block
flashing on a healthy instance, which is worse than
the problem it prevents. Its comment also said the
payload was 27001; it is 31000.
APP_ID built but never applied. Without
SetCurrentProcessExplicitAppUserModelID, Windows
groups the window under the host process
(python.exe from source), so the taskbar shows the
wrong icon and a pinned shortcut opens a second
button. Set before the first window exists, which
is the only point at which it takes effect.
MODULE_VERSION (x2) human-readable version that nothing displayed. Now
in the download progress line, so a user can see
which ReZygisk/LSPosed they are getting.
tests/test_module_version_gate.py covers the new gate, including the direction
that is easy to invert in a refactor: unreadable version proceeds, it does not
block. Mutation-checked -- disabling the comparison fails the refusal test.
ruff clean, vulture clean (bar a pytest fixture and a Qt override it cannot see
are called), 307 passed.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc1c7c8ba5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| out = ((cp.stdout or "") + (cp.stderr or "")).strip() | ||
| m = re.search(r"\b(\d{4,6})\b", out) | ||
| return int(m.group(1)) if m else None |
There was a problem hiding this comment.
When the version query fails but its diagnostic contains a 4–6 digit number, this ignores cp.returncode and parses that number as the Magisk version. For example, a transient ADB disconnect can report device '127.0.0.1:5555' not found, causing the ReZygisk install to claim that Magisk 5555 is installed and incorrectly instruct the user to update the root payload, rather than treating the version as unreadable as intended. Return None on a nonzero status and only parse successful version output.
Useful? React with 👍 / 👎.
| ctypes.windll.shell32.SetCurrentProcessExplicitAppUserModelID( | ||
| constants.APP_ID) |
There was a problem hiding this comment.
For users who pin this release and later replace it with an updated executable, constants.APP_ID includes APP_VERSION, so applying it here gives every release a different Windows taskbar identity. The pinned shortcut retains the old identity and the updated process therefore opens under a separate taskbar button—the behavior this change is intended to prevent. Use a version-independent AppUserModelID for this call.
Useful? React with 👍 / 👎.
1 participant
Audit pass over the codebase. Reporting the honest result first: the mechanical
detectors came back almost empty.
So this is the set that survived verification, not a sweep. Two of the seven turned
out not to be dead at all but unfinished, which is the more interesting half.
Dead
magisk_system._list_dir— a thin wrapper over_list_dir_typedwith noproduction caller. It existed only because a test tested it. That test's real
coverage (skipping
./.., stripping symlink targets) was not duplicated inthe typed test, so it moved there rather than being deleted along with the wrapper.
MODULE_SIZE×2 — a pin nothing validated. SHA-256 already covers integrity;a byte count added nothing.
Claimed but never implemented
MIN_MAGISK_VER_CODE— the sharpest finding. Its own comment said it was"surfaced here so a Magisk version bump that dips below is caught rather than
failing mid-flash", and nothing read it. ReZygisk's
customize.shenforces theminimum itself and aborts partway through, leaving exactly the half-written module
the constant claimed to prevent.
install_modulenow takesmin_magisk_ver_codeand refuses before anything ispushed. Note the failure direction: an unreadable version still proceeds. A gate
that fired on "couldn't tell" would block flashing on a healthy instance whose shell
was briefly unavailable — worse than the problem it prevents.
(Its comment also said the payload was 27001. It's 31000.)
APP_ID— built, never applied. WithoutSetCurrentProcessExplicitAppUserModelID, Windows groups the window under the hostprocess (
python.exewhen run from source), so the taskbar shows the wrong icon anda pinned shortcut opens a second button. Now set before the first window exists,
which is the only point where it takes effect.
MODULE_VERSION×2 — human-readable version nothing displayed. Now in thedownload progress line, so users can see which ReZygisk/LSPosed they're getting.
Tests
tests/test_module_version_gate.pycovers the gate, including the direction that'seasy to invert in a refactor: unreadable version proceeds, it does not block.
Mutation-checked — replacing the comparison with
if False:failstest_refuses_when_daemon_is_older_than_the_module_requiresand nothing else, sothe test guards real behaviour rather than passing vacuously.
ruff clean, 307 passed. Vulture's two remaining hits are false positives it
can't resolve: an autouse pytest fixture and a Qt
closeEventoverride.What I deliberately did not do
Opting into ruff's wider rule families reports 1,045 hits, but ~95% are style
opinions the project doesn't hold: 300
%-vs-f-string (the%form is correct forlogging), 199 line-length against a default the project never set, ~260
os.path→pathlib, 125 unused-lambda-args that are Qt signal handlers. Applying them means adiff touching every file, guaranteed conflicts against in-flight branches, and no
bug caught. Available as its own isolated commit if wanted.