Fix: SDK installer macOS slug selection and diagnostics

[gavin-at-pieces]

Summary

• Fix macOS Pieces OS installer package slug selection for Intel, Apple Silicon, and Rosetta cases.
• Add optional installer failure diagnostics to existing download callbacks without removing existing callback fields.
• Serialize Linux stderr diagnostics to avoid concurrent callback invocation from multiple reader threads.
• Add focused tests for macOS slug resolution, callback compatibility, and installer diagnostic events.

Tests

• python -m pytest tests/test_installation.py -v
• python -m py_compile src/pieces_os_client/wrapper/installation.py tests/test_installation.py

Full Suite

• Full suite validation was previously blocked by missing local dependencies in the fresh environment.
• Focused installer validation passes with 11 tests.

Risks

• Installer launch return codes remain platform-dependent.
• Diagnostic fields are additive on DownloadModel and should preserve existing callback consumers.

Rollback

• Revert src/pieces_os_client/wrapper/installation.py.
• Remove tests/test_installation.py.

[gavin-at-pieces]

This PR is intentionally scoped to the public Python SDK installer path.

It fixes:

• macOS Pieces OS installer package slug selection for Intel, Apple Silicon, and Rosetta cases.
• Additive installer diagnostics on DownloadModel.
• Serialized Linux stderr diagnostics to avoid concurrent callback invocation from multiple reader threads.

Focused validation:

• python -m pytest tests/test_installation.py -v → 11 passed
• python -m py_compile src/pieces_os_client/wrapper/installation.py tests/test_installation.py → passed

The broader installer/runtime reliability items from the audit are intentionally not included here.

[gavin-at-pieces]

Validation update from my side:

• Re-checked this through the CLI vendored SDK path, not just direct SDK import.
• Combined this PR's installation.py with the full wrapper from fix: add SDK local readiness diagnostics for invalid ports #145 in a temporary CLI vendoring smoke.
• CLI import smoke passed.
• Core CLI command smoke passed: pieces --help, pieces version, pieces install --help, pieces mcp --help, and pieces open --help.
• Also validated the combined Fix: SDK installer macOS slug selection and diagnostics #144/fix: add SDK local readiness diagnostics for invalid ports #145 SDK source path against plugin_neovim import/module smoke with a temp pynvim target.

Not claiming full CLI pytest or a full Neovim debug session here — this is scoped vendored-wrapper + plugin import-surface validation.

[mark-at-pieces]

Extensive Multi-Dimension Review — PR #144

Reviewed across 6 dimensions (Correctness, Threading, API Compatibility, Security, Testing, Edge Cases) using parallel analysis. 31 findings: 5 High, 10 Medium, 8 Low, 8 Positive.

---

🔴 Must Fix — Introduced by this PR

H1. update_progress signature change breaks subclass overrides
_emit_failure now calls self.update_progress(message=..., error=..., error_code=..., ...). Any consumer subclass that overrides update_progress with the old 2-parameter signature (bytes_received, total_bytes) will crash with TypeError: unexpected keyword argument. PosInstaller is exported and consumers can subclass it.

Suggestion: Accept **kwargs in the base update_progress to future-proof, or route diagnostic emission through a separate _emit_diagnostic method that bypasses the override surface.

H2. Intel slug URL may not exist on the server
The old code embedded the arch directly: pkg-pos-launch-only-{arch}. Because the old check was buggy (sys.maxsize > 2**32 is always true on 64-bit Python), it always resolved to arm64 — Intel Macs never actually got an Intel URL. The new Intel slug is pkg-pos-launch-only (no arch suffix). Since the old code never served Intel users correctly, this slug may never have been tested on the server side.

Suggestion: Verify the server serves pkg-pos-launch-only (no suffix) for Intel builds. Add a smoke test that both resolved URLs return HTTP 200.

H5. Thread joins without timeout can block forever
stdout_thread.join() and stderr_thread.join() have no timeout. If the subprocess spawns a child that inherits pipe file descriptors (e.g., pkexec), the pipe stays open after the parent exits. readline() blocks indefinitely, and the join never returns — hanging the installer permanently.

Suggestion: Add a timeout to both joins (e.g., 30s). If the join times out, log a warning and continue to the return_code check.

---

🟡 Pre-existing Issues Worth Fixing (surrounding code was touched)

H3. extract_linux_regex returns None → TypeError in callback (pre-existing)
extract_linux_regex returns Optional[Tuple[int, int]] but execute_command unpacks with bytes_received, total_bytes = callback(line). When the regex doesn't match, None is returned → TypeError. The except block catches this but treats it as a warning, silently dropping progress updates.

Suggestion: Return (0, 0) on no-match instead of None, or check for None before unpacking.

H4. Stall timeout in install_using_web is dead code (pre-existing)
last_data_time = time.time() is set, then response.read() blocks, then the stall check runs — but last_data_time was just updated. The 5-second STALL_TIMEOUT can never trigger.

Suggestion: Move the stall check to a watchdog thread, or use socket-level timeouts via urllib.request.urlopen(url, timeout=N).

---

🟠 Medium Severity (10 items)

M1. Exception handler skips thread join — concurrent callbacks possible
If an exception occurs after threads start but before join(), the except handler calls _emit_failure without joining. read_stdout may still be invoking the user's callback concurrently.
→ Join threads in a finally block before _emit_failure.

M2. cancel_download races with failure path
cancel_download sets state=IDLE via update_progress_stop() while the download thread sees exit code -9 and calls _emit_failure setting state=FAILED. Final state is non-deterministic.
→ Add a _cancelled flag checked before _emit_failure.

M3. Callbacks fire from background thread — undocumented
Progress callbacks from execute_command now fire from the stdout reader thread (child of download thread), not the download thread itself. UI frameworks (Tk, Qt) require main-thread callbacks.
→ Document that the callback may be invoked from any background thread.

M4. execute_command never sets COMPLETED on success (pre-existing)
When execute_command succeeds, self.state remains DOWNLOADING. No COMPLETED callback fires for Linux installs.
→ Set self.state = DownloadState.COMPLETED and call self.update_progress() before returning True.

M5. Errors are silent without callback
Old code called print(f"Error: {e}"). New code only calls _emit_failure which only invokes the callback. If callback is None, errors are completely swallowed.
→ Add logging.error() fallback when no callback is registered.

M6. COMPLETED callback timing changed
Old: fired BEFORE launching installer. New: fires AFTER installer launch succeeds, or emits FAILED on non-zero exit. Consumers reacting to COMPLETED will see different timing.
→ Document callback ordering change.

M7. Hardcoded /tmp path — symlink/TOCTOU risk
/tmp/Pieces-OS-Launch.pkg is predictable and world-writable. Concurrent installers clobber each other. File not cleaned up on failure.
→ Use tempfile.mkstemp(suffix='.pkg').

M8. shell=True on Windows
subprocess.run(command, shell=True) passes through cmd.exe /c. gettempdir() is runtime-dependent.
→ Use os.startfile(tmp_pkg_path) instead.

M9. sysctl timeout → silent architecture downgrade
On a heavily loaded system, 2s timeout on sysctl means _is_apple_silicon_hardware returns False. Rosetta x86_64 Python then gets the Intel package on ARM hardware.
→ Log a warning on timeout. Consider increasing to 5s or retrying.

M10. stderr now batched — no real-time error callbacks
Old code emitted ERROR callback per stderr line. New code batches and delivers once at exit.
→ Document that stderr is now batched.

---

🔵 Low Severity (8 items)

• L1: getattr(result, 'returncode', 0) masks bugs by defaulting to success → use result.returncode directly
• L2: i386/i686 in slug resolver are dead code on modern macOS (dropped in Catalina)
• L3: self.product not URL-encoded in f-string URL construction → use urllib.parse.quote
• L4: Error codes are plain strings scattered across 5 call sites → define InstallerErrorCode(str, Enum)
• L5: DownloadModel has 11 manually-assigned fields → convert to @dataclass
• L6: self.print is a no-op → replace with logging.getLogger(__name__).debug()
• L7: Missing tests for i386/i686, hyphen normalization (arm-64), and Rosetta+amd64
• L8: Diagnostic fields leak internal details (bash scripts, CDN URLs, stack traces) to callbacks → document or add sanitized_error

---

✅ Positive Observations (8 items)

Area	Observation
Bug Fix	Old sys.maxsize > 2**32 was fundamentally broken — always selected ARM on 64-bit Python. Correctly fixed with platform.machine() + sysctl.
Rosetta	sysctl -n hw.optional.arm64 is the canonical detection method. Absolute path, timeout, defensive exception handling — textbook correct.
Deadlock Fix	New two-thread model fixes latent pipe deadlock where heavy stderr output could block the single-threaded loop. Significant correctness improvement.
Serialization	stderr callback serialization claim is correct — read_stderr only appends to a list, failure callback fires after stdout_thread.join().
Security	Package slug constrained to two class constants. No attacker-controlled data reaches URL path.
Decoding	errors='replace' is the right choice for subprocess output — prevents daemon thread crashes on non-UTF-8 bytes.
Testing	All 11 tests properly isolate platform behavior through mocking. Rosetta test correctly patches both sys.platform and subprocess.check_output.
Architecture	_emit_failure centralizes error handling, replacing 3 copy-paste sites with consistent diagnostic shape.

---

Overall: The core architecture detection fix is correct and important — the old code was genuinely broken for Intel Mac users. The threading refactor solves a real deadlock risk. The diagnostic callback additions are well-designed. The main blockers are H1 (subclass override breakage), H2 (Intel slug URL verification), and H5 (thread join timeout). Address those three and this is ready to merge.