Skip to content

Fix accuracy defects from the review follow-up batch - #75

Merged
heznpc merged 2 commits into
mainfrom
fix/review-followup-accuracy
Aug 13, 2026
Merged

Fix accuracy defects from the review follow-up batch#75
heznpc merged 2 commits into
mainfrom
fix/review-followup-accuracy

Conversation

@heznpc

@heznpc heznpc commented Aug 13, 2026

Copy link
Copy Markdown
Owner

The lower-severity half of the same adversarial review as #74. None of these crashes or claims a false success, but each reports something that is not quite true. All were reproduced before being fixed.

Login item names: comma ambiguity

osascript serializes a list as ", "-joined text, so an item named Backup, Inc. is byte-identical to two items Backup and Inc.. Two consequences, both confirmed:

  • That item could never be matched → permanently unremovable through this tool.
  • With both Foo and Foo, Bar present, removing Foo succeeded but the post-delete recheck still matched the Foo fragment of the survivor → reported failed after a successful removal.

Names are now read one per line (text item delimiters → linefeed), which cannot collide since names carrying tab/newline are rejected upstream.

lsof escapes beyond \x20

lsof escapes every byte it deems unprintable. #73 decoded only \x20, so a tab arrived as a literal ev\x09il and a non-ASCII name as a run of \xNN. Both the awk (network_watch.sh) and JXA (scanner_helper.jxa.js) decoders now handle whole runs — verified that \xed\x95\x9c\xea\xb8\x80 reassembles to 한글 in both engines rather than becoming per-byte mojibake. Control bytes become a space, since they would break the TSV the protocol rides on.

Pinned Homebrew packages were silently dropped

brew appends [pinned at X] for a pinned formula or cask; the regex anchored the latest version to end-of-line, so those lines returned null and the row disappeared. The collector's own wc -l still counted them, so the displayed "N개 업데이트" total disagreed with the rows shown, and a pinned package's available update was invisible. Rows now survive and carry pinned, surfaced in the UI. A pytest guard drives the real regex through JavaScriptCore against all five real brew output shapes.

SpaceGoalSelection

  • Determinism: the doc comment promised the same selection regardless of scan order, but label falls back to kind, so two same-size rows of one kind tied completely and the result depended on emit order. Path breaks the remaining tie.
  • Float boundary: 2.4 + 0.3 + 0.3 is exactly 3.0 in decimal but accumulates to 2.9999999999999996, so the greedy walked past the set that met the goal, took an extra item, and then labelled the result short of the goal it had reached. (I searched for a genuine case rather than trusting the reviewer's example, which did not actually reproduce.)
  • Non-finite sizes are clamped at the parse boundary, where they used to poison every sum and trip a ClosedRange precondition.

Concurrency guards

Login item removal and the observation window now respect isBusy in both directions. Removal could previously start a second concurrent ScanPipeline over the same scan_result.json/report files during a scan — and since that run isn't held in scanTask, 검사 취소 could not stop it. Conversely a scan started under an observation landed in that observation's own results (the scanner's du/lsof as top "real CPU use", VirusTotal lookups as "new connections").

Mothball reported inspection failures as "nothing to archive"

It used scan() instead of scanReport(), discarding failures — and MothballCore's own API comment warns that this exact thing makes "found but could not inspect" indistinguishable from "nothing found". The page now distinguishes them.

Test plan

  • pytest tests/ — 369 passed
  • swift test — 168 passed
  • release_smoke.py --check-only — ok
  • Every new test proven against the pre-fix code first. One of them (the float-boundary case) initially had no teeth — my first fixture passed against the old code — so I searched for a combination that genuinely reproduces and replaced it rather than keeping a test that proved nothing.
  • Real rescan on this machine: all 59 devtool rows carry pinned, and no escaped process names remain in the network section.

Follows #74.

🤖 Generated with Claude Code

heznpc added 2 commits August 13, 2026 21:50
The lower-severity half of the same adversarial review as #74. None of
these crashes or claims a false success, but each reports something
that is not quite true.

Login item names are now read one per line instead of relying on
osascript's ", " list serialization. That default is ambiguous: an item
named "Backup, Inc." serialized exactly like two items "Backup" and
"Inc.", so it could never be matched and was permanently unremovable
through this tool; and a surviving "Foo, Bar" kept producing a "Foo"
fragment, so removing a genuinely separate "Foo" re-matched on the
post-delete recheck and reported failed after a successful removal.

lsof escapes every byte it deems unprintable, not just the space that
motivated the earlier fix -- a tab arrives as \x09 and a non-ASCII name
as a run of \xNN. Both the awk and the JXA decoders now handle whole
runs, so a multi-byte UTF-8 name reassembles instead of leaking
literal escapes into the UI and into anything matching on the name;
control bytes become a space because they would break the TSV.

Pinned Homebrew packages were dropped outright by the devtoolUpdates
regex, which anchored the latest version to end-of-line while brew
appends " [pinned at X]". The row vanished while the collector's line
count still included it, so the displayed total disagreed with the
list and a pinned package's update was invisible. Rows now survive and
say they are pinned.

SpaceGoalSelection's documented "same set regardless of scan order"
promise did not hold when size and label both tie -- label falls back
to kind, so two same-size rows of one kind tied completely and the
result depended on emit order. Path breaks the remaining tie. The
goal comparison also gained a tolerance: 2.4 + 0.3 + 0.3 is exactly
3.0 in decimal but accumulates to 2.9999999999999996, so the greedy
walked past the set that met the goal, took an extra item, and then
called the result short. A non-finite sizeGB is clamped at the parse
boundary, where it used to poison every sum and trip a range
precondition.

Login item removal and the observation window now respect isBusy in
both directions: removal could previously start a second concurrent
ScanPipeline over the same output files during a scan (untracked by
scanTask, so 검사 취소 could not stop it), and a scan started under an
observation landed in that observation's own results.

Mothball used scan() rather than scanReport(), discarding inspection
failures -- MothballCore's own API comment warns that this makes
"found but could not inspect" indistinguishable from "nothing found".
The page now says which it is.
Same class of miss as the network_watch skipif: the probe runs the
real regex through /usr/bin/osascript, which Linux runners lack.
@heznpc
heznpc merged commit bca023d into main Aug 13, 2026
8 checks passed
@heznpc
heznpc deleted the fix/review-followup-accuracy branch August 13, 2026 13:04
heznpc added a commit that referenced this pull request Aug 13, 2026
Six items the #74/#75 reviewers classified low/informational --
nothing here is reachable in a shipped build today; each is a seam a
future change could silently reopen.

privacy.sh's PCH_TCC_DB_PATH test seam now opens only under
PCH_TEST_MODE=1, matching PCH_TEST_BREW_BIN and PCH_TEST_OSASCRIPT_BIN.
It was the one injection point in the new modules without the gate,
and ungated it doubles as an arbitrary-sqlite-read redirect for any
future caller that forwards the variable.

login_items.sh now refuses a manifest whose protocol version differs
from its own -- fields this version reads could mean something else
under another one. cleanup.sh cross-checks its whole manifest; this is
the minimum equivalent.

The bundled-runtime completeness guard now also covers non-.sh runtime
dependencies (rules/*.json, data/*.json, report_i18n, *.jxa.js, and
shipped .py), with an explicit build/reference-only exclusion list
mirroring the .sh guard's. Proven the same way: removing
rules/network.json from RUNTIME_FILES trips it.

RuntimeWorkspace's bundled branch now demands the development opt-in
whenever the injected resourceURL is not the running app's own signed
bundle. That combination skips the code-signature payload comparison
and is constructible only through parameter injection; without the
gate, a future refactor decoupling the parameters would run unsigned
code with no test noticing. The revalidation tests declare the opt-in
they were implicitly relying on.

Raw pinned-file values now refuse on key collision instead of
silently clobbering whichever module payload merged first -- the
exact asymmetry behind the approval_token/approval_token_module
near-miss fixed in #67.

Observation timeout slack grows from 20s to 60s past the window (the
snapshots run exactly when the machine is busiest), and a timeout now
says it timed out instead of "status 124".

Co-authored-by: Heznpc <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant