bug(controller): _shell joins shell command but tests register split argv tokens

[hah23255]

bug(controller): _shell joins shell command but tests register split argv tokens

Metadata

Field	Value
Severity	🟡 Medium
Class	Test/code coupling — test setup never matched production behavior
Component	adb_android_control/controller.py::_shell, tests/unit/test_controller.py::TestErrorClassification
Found by	First public CI run (test never executed before)
First failing CI run	https://github.com/hah23255/adb-android-control/actions/runs/25394406444
Affected versions	2.0.0 (test was authored in 2.0.0-rc1)
Target fix version	2.0.1
Special process	Needs Doctrine Law 1 exception PR (modifying tests)

Symptom

Two failing tests:

FAILED tests/unit/test_controller.py::TestErrorClassification::test_device_offline_stderr_raises_device_offline_error
       — tests.conftest.UnmockedADBCallError: Test failed: code shelled out to an unregistered argv.
FAILED tests/unit/test_controller.py::TestErrorClassification::test_device_unauthorized_also_raises_device_offline_error
       — tests.conftest.UnmockedADBCallError: Test failed: code shelled out to an unregistered argv.

Both fail at the mock layer, not the assertion layer — meaning
the production code reached for an argv tuple the test forgot to
register.

Reproduce

pytest tests/unit/test_controller.py::TestErrorClassification -v

The Poison-Pill mock (tests/conftest.py::PoisonPillADB) raises
UnmockedADBCallError because the actual argv (["adb", "shell", "getprop ro.product.model"]) doesn't match any registered argv.

Root cause

Production code (correct, do not change)

adb_android_control/controller.py:

def _shell(self, cmd: str, timeout: int = DEFAULT_TIMEOUT_S) -> str:
    """Run a shell command on the device."""
    return self._run(["shell", cmd], timeout=timeout)

_run then prepends adb and (if set) -s <serial>. So a call to
ctrl.get_property("ro.product.model") results in:

subprocess.run(["adb", "shell", "getprop ro.product.model"], ...)
#               ^^^^^  ^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#               1      2        3 — single argv element

This is architecturally correct. Real adb shell takes the
shell command as a single argument and forwards it to sh -c on
the device. Splitting getprop ro.product.model into two argv
tokens would change runtime semantics (especially around quoting
and special characters).

Test code (wrong)

tests/unit/test_controller.py::TestErrorClassification:

def test_device_offline_stderr_raises_device_offline_error(
    self, mock_adb: PoisonPillADB
) -> None:
    mock_adb.register(["adb", "version"], stdout="v1\n")
    mock_adb.register(
        ["adb", "shell", "getprop", "ro.product.model"],   # ← 4 tokens, WRONG
        stderr="error: device offline",
        returncode=1,
    )
    ctrl = ADBController()
    with pytest.raises(DeviceOfflineError):
        ctrl.get_property("ro.product.model")

The mock expects 4 argv tokens; production sends 3. They never match
→ UnmockedADBCallError.

The test was authored in good faith but never executed before the
public flip (no CI). The test-file-integrity gate is now blocking
direct fixes to tests/, which is correct behavior — modifying a
test to make CI green is exactly what Law 1 forbids.

Decision matrix

Option	Pros	Cons
A. Fix the test (use 3-element argv)	Minimal change, ~5 lines edited, semantically correct	Doctrine Law 1 friction: test-file-integrity CI gate must allow this commit. Requires reviewer sign-off + a [doctrine-exception] tag in the PR title.
B. Change production _shell to split argv	Test passes as-written	Wrong: changes runtime ADB semantics; would break real-world quoting/escaping; would also need updates to many other tests that use the 3-token form. Net regression.
C. Add a parameter that allows splitting	Most flexible	Over-engineering for a test bug. Increases public API surface for no real benefit.

Recommended: A, with explicit reviewer sign-off.

Proposed fix (Option A)

 def test_device_offline_stderr_raises_device_offline_error(
     self, mock_adb: PoisonPillADB
 ) -> None:
     mock_adb.register(["adb", "version"], stdout="v1\n")
     mock_adb.register(
-        ["adb", "shell", "getprop", "ro.product.model"],
+        ["adb", "shell", "getprop ro.product.model"],
         stderr="error: device offline",
         returncode=1,
     )
     ctrl = ADBController()
     with pytest.raises(DeviceOfflineError):
         ctrl.get_property("ro.product.model")

Same diff for test_device_unauthorized_also_raises_device_offline_error.

Process for landing the fix (Doctrine Law 1 exception)

1. Open PR with title prefix: [doctrine-exception] test(controller): align mock argv …
2. PR description includes:
   • Link to this issue (bug(controller): _shell joins shell command but tests register split argv tokens #5)
   • Quote of Doctrine Law 1
   • Justification: test never executed before public CI flip;
     mock-registration form was the bug, not the production code.
   • Reviewer sign-off requirement: at least one repo maintainer
     explicitly approves with comment Law 1 exception accepted: <reason>.
3. CI's test-file-integrity job will fail; the PR template's
   override section must be filled in to bypass it.
4. After merge, the rule remains: no future test edits to make CI
   green without similar process.

Doctrine alignment

• Law 1 — ⚠️ Test modification, but justified via the documented
  exception process. The doctrine spirit ("fix the code, not the
  screwdriver") is preserved because the production code is correct;
  the test was never validated.
• Lesson "v1.0.0 PII gate" — same lesson applies: the rule
  matters most when it's enforced even on edge cases. We've now seen
  that the test-file-integrity gate did its job (correctly blocked
  this PR class until a process exists).

Test impact

After the fix:

• test_device_offline_stderr_raises_device_offline_error ✅
• test_device_unauthorized_also_raises_device_offline_error ✅

No production code changes; no other tests affected.

Acceptance criteria

• PR titled with [doctrine-exception] prefix
• PR description references this issue and quotes Law 1
• Reviewer adds Law 1 exception accepted: … comment
• Both failing tests pass after merge
• No production code modified in this PR
• CHANGELOG.md [Unreleased] updated under ### Fixed

Effort estimate

~30 min including the exception-process paperwork.

Related

• META tracker: META: 5 test failures from first public CI run — tracking #8
• Doctrine: docs/TESTING_DOCTRINE.md § Law 1
• CI gate: .github/workflows/ci.yml::test-file-integrity