Skip to content

Phase 0 of the 2026-09-28 review: review docs, test-suite fixes, exit-status CI gate - #268

Open
aradanmn wants to merge 16 commits into
mainfrom
claude/codebase-review-report-3611su
Open

Phase 0 of the 2026-09-28 review: review docs, test-suite fixes, exit-status CI gate#268
aradanmn wants to merge 16 commits into
mainfrom
claude/codebase-review-report-3611su

Conversation

@aradanmn

@aradanmn aradanmn commented Nov 2, 2026

Copy link
Copy Markdown
Owner

Phase 0 of the 2026-09-28 code review (#200): the review documents, the fix plan, and the test-suite fixes that have to land before anything else, so the CI gate can be trusted when the product fixes start.

What's in it

Production code changed: modules/controller_proxy.sh (for #208's guarded readonly block) and nothing else. Everything else is tests, CI and docs.

Testing

  • tests/test_uninstall_purge.sh: 22/22 locally on a checkout outside /tmp.
  • Mutation checks:
    • Disabling the Steam-running guard in remove-from-steam.py turns T5.1 red.
    • Renaming the fake process turns T5.1 red with the fixture message.
  • shellcheck -S warning is clean on the changed test.
  • CI on this push: see the checks below.
  • Not yet done (the plan's Phase 0 exit check):
    • Run the unit suites next to a live Deck session, to confirm T1 and T3 leave it alone.
    • Push a deliberately failing test to a scratch branch, to confirm CI goes red.

Found while testing, not fixed here

uninstall-minecraft-splitscreen.sh:444 deletes the helper by path match ([[ "$helper" == /tmp/* ]] && rm -f), so a checkout under /tmp loses its own remove-from-steam.py on an uninstall run, including from the test suite. That's a product bug, out of scope for this test-only PR; it belongs with F14 in #260.

Fixes #201
Fixes #202
Fixes #203
Fixes #204
Fixes #205
Fixes #206
Fixes #207
Fixes #208
Fixes #225
Refs #200

🤖 Generated with Claude Code

claude and others added 16 commits September 28, 2026 16:24
@claude
docs: deep code review 2026-09-28 (177 findings, 3 critical)
028f11e 
Consolidated, de-duplicated review of the launcher, all runtime and installer
modules, the Steam helpers, system/ hooks, CI workflows and the test tree.
Per-module reports with full reasoning are under docs/reviews/2026-09-28/.

Top findings: the startup guard's `pkill -9 -f PolyMC` matches the launcher's
own cmdline; SLOT_DIED frees the slot before teardown so hung JVMs are never
killed on the default proxy path; the dex XWindowAttributes struct omits
colormap so map_state reads the wrong field.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
docs: phased fix & hardware-verification plan for the 2026-09-28 review
a05c0c5 
Six phases (CI trust first, then the three Criticals, blast radius, session
stability flag-gated, installer, security, system hooks), dependency graph,
per-PR checklist, and the HW-3 Deck validation session mapped to
tests/hardware stages. Tracking issue: #200.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
docs: drafted issue bodies with suggested diffs for the 2026-09-28 re…
228d9ea 
…view

One file per Critical/High/Medium finding plus one grouped Low/Info
checklist per module. Each carries a unified diff that was syntax-checked
and `git apply --check`ed against the pristine tree, the unit test and
mutation to add, and the hardware step that proves the fix. These are the
bodies posted as sub-issues of tracking issue #200.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
docs: link the fix plan to the opened issues (#200, #201-#267)
db149f0 
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(orchestrator): keep T6.3's proxy path off the real $XDG_RUNTIME_…
7b6e219 
…DIR/mcss

Export MCSS_HELPER_DIR / MCSS_PROXY_PADS_DIR / MCSS_PROXY_VIRT_DIR under the
suite tmpdir before sourcing the launcher, stub proxy_stop_slot/slot_free in
T6.3, and refuse to run if the helper dir ever resolves outside the tmpdir.
Since #150 flipped MCSS_CONTROLLER_PROXY to 1, the SLOT_DIED handler's real
proxy_stop_slot could kill a live session's slot-2 evsieve during a Deck run.

Fixes #203

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(instance_lifecycle): fixture PIDs above pid_max; geom cache unde…
13f4387 
…r the tmpdir

T4.5/T4.8 drive the real teardown_instance, which process-group-kills the
state file's bwrap_pid; the old literals (99998/99999, 11110..44440) were
below kernel.pid_max, so a reachable PGID could be SIGTERM/SIGKILLed on an
un-namespaced run. Same guard as tests/test_orchestrator.sh. The two fixtures
also point MCSS_GEOM_DIR at the tmpdir so a run no longer deletes the live
/tmp/mcss-geom/slotN cache (T12).

Fixes #202

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(hardware): reap stale sessions by PID, never by name (PRINCIPLES #7
fae7e49 
)

hw_reap_stale_session ran four unconditional name-matched `pkill -9 -f`
sweeps (kwin_wayland, latestUpdate, bwrap.*PolyMC, SteamLaunch.*) on every
stage launch/stop. On SteamOS Desktop Mode and Bazzite hosts kwin_wayland is
the operator's whole session, and `-f latestUpdate` matched any
`tail -f .../latestUpdate-1/latest.log` in another terminal.

Replace them with _hw_session_pids: marked processes (SPLITSCREEN_DEBUG_LOG=
in environ), their descendants (covers the unmarked kwin_wayland under a
marked wrapper), and the Steam reaper identified as the unmarked parent of a
marked launcher, minus the harness ancestor chain. One ps snapshot, SIGKILL
in a bounded 5-sweep loop.

New tests/test_hw_helpers_reap.sh builds that tree from renamed sleeps and
asserts the set, the exclusions, the reap, and that no pkill/killall
invocation remains. Mutation: re-adding the latestUpdate pkill fails T1.1
and T1.6.

Fixes #201

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(window_manager): assert the count-based grid contract, not the r…
7898c53 
…etired one

T3.1/T3.9 still encoded the highest-slot rule that compute_grid_mode
deliberately replaced (2026-06-23), so the suite was red on correct code
(7/9, tolerated by a CI baseline of 7) and went 9/9 green if the fix was
reverted. Rewrite the table to the count contract ({2}->full, {1,3}->half,
{2,4}->half, {3}->full), assert the scale-down cell mapping in T3.9, and
raise the CI baseline to 9. Mutation: reverting compute_grid_mode to
highest-slot now fails 5 T3.1 cases and T3.9.

Fixes #204

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(preflight): make the mocks record their pid so _mock_cleanup act…
353803f 
…ually reaps

_mock_cleanup looped over $MOCKBIN/*.pid files that nothing wrote, so every
`sleep 30` mock dialog outlived its test (6-8 orphans per run) and T3.2/T3.3
found the mock with a name-matched pgrep. The mock now writes $$ and execs
the sleep, T3.2/T3.3 read the pid from the file, and a new T3.5 asserts the
cleanup reaps by pid. Mutations: dropping the exec or the .pid write go red.

Fixes #205

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(uhid_pad): validate an axis value before storing it
0710698 
UhidPad.axis committed the value to self._axes and only then hit the range
check in encode_report, so one `axis LX 300` poisoned the pad: every later
press/hat/release re-raised on the stored value until `neutral`. Validate
first, like press()/hat(); expose it in --self-test and assert it in T5.4.
Mutation: reverting axis() to store-then-send reports rejected,axes=1.

Fixes #206

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(hardware): derive hw_expected_slot_geometry from the product's o…
ac5f49b 
…wn rule

The helper carried a hand copy of the grid/cell policy that had drifted
from compute_grid_mode/apply_layout (any slot>=3 -> quad; half cells by slot
number instead of order): 10 of 32 (subset, slot) pairs disagreed, hidden
only because every stage passes a contiguous set. It now sources
window_manager.sh lazily and uses compute_grid_mode + compute_slot_geometry
with the apply_layout cell rule. New tests/test_hw_helpers_geometry.sh
sweeps all 32 pairs against an independent oracle (red on the old copy;
mutation that maps quad cells by order goes red).

Fixes #207

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(module_resource): parse the installer module list; capture the s…
99e84d2 
…ubshell's stderr

The suite's hand-copied installer list had drifted (version_stamp.sh, #89,
was never added) and its `2>&1` sat outside the command substitution, so a
`readonly variable` line from a double-source escaped to the terminal and
the case still passed. Parse INSTALLER_MODULE_FILES from the installer and
move the redirect inside. With stderr captured the suite exposed
controller_proxy.sh's unguarded top-level readonly block, which now uses the
house _*_CONSTANTS_LOCKED guard. Mutation: a readonly appended to
version_stamp.sh goes red.

Fixes #208

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(hardware): skip zombies in the session sweep
858d26e 
_hw_session_pids listed zombies (already dead, unsignallable), which kept
the 5-sweep loop re-running until PID 1 reaped them; on a container init
that made test_hw_helpers_reap.sh T1.4 flaky (3/5 runs). Skip stat Z in
the ps snapshot and treat a zombie as dead in the test.

Refs #201

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
ci: gate every suite on its exit status instead of a pass-count baseline
79acfda 
The old gate compared only `passed >= BASELINE` and discarded each suite's
exit code, so test_uhid_pad.sh could lose 5 of 23 tests with CI green, a
suite that recorded [FAIL] lines but still printed a full count passed, and
seven CI-safe suites were not gated at all. Now every tests/test_*.sh found
by glob runs under a per-suite timeout with output to a file (never a $(...)
capture, #80/#103), and the gate is the exit status plus the presence of a
standard "X/Y tests passed" summary. A KNOWN_FAIL map holds the exact
summary a known-red suite must print (empty since #204). The three suites
that printed "[OK] ..." get the standard footer; test_phase_b_lifecycle.sh
is the one explicit SKIP. Job timeout 30 min.

Fixes #225

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@claude
test(evsieve_management): keep fuse-overlayfs out of the isolated PATH
8d8bc4f 
T13 asserts the "fuse-overlayfs not available -> single create, no retry"
path, but _build_isolated_base symlinked every real /usr/bin tool, so on
GitHub's ubuntu runner (which ships fuse-overlayfs) the module found it and
retried: 19/20 on the first gated run of #225. Exclude it like
git/distrobox/podman/cargo/sha256sum; T12/T14 already add their own
placeholder. Verified 20/20 with a fake /usr/bin/fuse-overlayfs present.

Refs #225

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013yYdp31rf3ha67LhUaJZuS
@aradanmn @claude
test(uninstall_purge): fake Steam with a script, not a copy of /bin/s…
9f3837e 
…leep

ubuntu-latest moved to 26.04, whose coreutils is a multi-call binary
(uutils): a copy named "steam" dispatches on argv[0], prints "coreutils:
unknown program 'steam'" and exits, so T5.1 ran with no Steam process and
failed on the first CI run gated by exit status (#225). A #! script gets
its comm from the script file, so it is "steam" on any coreutils.

T5.1 now also checks the fixture before asserting, so a broken fake
reports as a fixture failure instead of a guard regression. Mutation
checked: disabling the guard in remove-from-steam.py turns T5.1 red;
renaming the fake turns it red with the fixture message.

Refs #225

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment