fix: add SDK local readiness diagnostics for invalid ports

[gavin-at-pieces]

Summary

• Add SDK-local readiness diagnostics for invalid, unset, out-of-range, scan-failed, health-failed, and ready local PiecesOS port states.
• Guard local host construction so invalid, empty, None, or "null" ports do not produce REST or websocket URLs.
• Preserve existing behavior shapes: is_pieces_running() still returns bool, and startup failures still raise ValueError with clearer diagnostic text.
• Add focused tests for invalid ports, scan failure, health failure, ready probes, and port = None reset safety.

Tests

• python -m pytest tests/test_client_readiness.py -v
• python -m py_compile src/pieces_os_client/wrapper/client.py tests/test_client_readiness.py

Scope

This PR is intentionally limited to Python SDK/CLI-facing local readiness diagnostics.

Out of scope:

• MCP-specific readiness
• Windows elevated repair
• updater rollback
• AVX2/vector-search preflight
• DB recovery/quarantine
• Desktop/plugin UI changes
• docs cleanup
• broad multi-repo readiness contract implementation

Risks

• This changes diagnostic text around startup/readiness failures, but preserves the existing bool/ValueError behavior.
• The broader .port.txt runtime contract remains upstream runtime evidence and is not implemented directly in this SDK PR.

Rollback

• Revert src/pieces_os_client/wrapper/client.py.
• Remove tests/test_client_readiness.py.

[gavin-at-pieces]

Follow-up to the installer reliability work.

This PR is intentionally narrower than the broader readiness contract:

• It only guards Python SDK local host/port construction.
• It prevents invalid/empty/None/"null" ports from producing REST or websocket URLs.
• It preserves existing behavior shapes: is_pieces_running() still returns bool, and startup failures still raise ValueError with clearer diagnostics.
• It does not read .port.txt directly and does not implement the broader runtime readiness contract.

Focused validation:

• python -m pytest tests/test_client_readiness.py -v → 24 passed
• python -m py_compile src/pieces_os_client/wrapper/client.py tests/test_client_readiness.py → passed

[gavin-at-pieces]

Validation update from my side:

• Validated this through the full-wrapper vendoring path rather than a single-file client.py copy.
• Combined fix: add SDK local readiness diagnostics for invalid ports #145's full wrapper with Fix: SDK installer macOS slug selection and diagnostics #144's installation.py 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.
• plugin_neovim direct SDK symbol smoke passed against the combined Fix: SDK installer macOS slug selection and diagnostics #144/fix: add SDK local readiness diagnostics for invalid ports #145 source tree.
• plugin_neovim module import smoke also passed after adding pynvim in a temporary target: pieces_python, settings, startup, utils, auth, and main.

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

[mark-at-pieces]

Extensive PR Review — 6-Dimension Parallel Analysis

Verdict: Approve with suggestions — 1 high-severity issue, 11 medium, solid overall.

The PR's intent is sound: guarding against invalid ports and providing actionable diagnostics is a genuine improvement. The old host property fallback (return "http://127.0.0.1:39300") was verified to be dead code (the self.port getter always either returns a truthy string or raises ValueError from _port_scanning()), so removing it is a cleanup, not a regression. The port normalization actually fixes a pre-existing bug where invalid ports like "abc" were stored and produced malformed URLs like http://127.0.0.1:abc.

---

Should fix before merge

ARCH-2 (High) — Inconsistent tuple element ordering between _normalize_port and _probe_pieces_os_port

_normalize_port returns (port, code, message) but _probe_pieces_os_port returns (code, port, message) — the first two elements are swapped. Call sites destructure correctly today, but this is a maintenance trap: anyone copying a destructuring pattern from one call site to the other will silently swap code and port. Both return Tuple[Optional[str], ...] so no type error would catch the swap.

Suggestion: Align the ordering, or use NamedTuples to make field access unambiguous:

class NormResult(NamedTuple):
    port: Optional[str]
    code: str
    message: str

class ProbeResult(NamedTuple):
    code: Optional[str]
    port: Optional[str]
    error: Optional[str]

SEC-1 (Medium) — isdigit() accepts Unicode superscript digits, causing unhandled crash

In _normalize_port, raw_port.isdigit() returns True for Unicode superscripts like '²' and '³', but int('²') raises ValueError — an unguarded crash path. Verified with Python:

• '²'.isdigit() → True
• int('²') → ValueError
• '²'.isdecimal() → False (correct rejection)

Suggestion: Replace raw_port.isdigit() with raw_port.isdecimal(), or wrap the int() call in a try/except.

---

Suggested improvements

COR-1 (Medium) — Non-200 success codes fall through without explicit return in is_pieces_running

When the health probe returns a non-200 status, the diagnostic is set but no return False follows — execution falls through to the next loop iteration without sleeping. Note: urllib.request.urlopen raises HTTPError for 4xx/5xx codes, so this only triggers for non-200 success codes (201, 204) which health endpoints rarely return. Still, an explicit return False would make the control flow clearer.

BC-1 (Medium) — __str__/__repr__ can crash when PiecesOS isn't running (pre-existing)

Both methods call self.host, which can raise ValueError. This was already the case in the old code (the ValueError came from _port_scanning() propagating through the port getter). Good opportunity to add try/except guards since Python convention is that __repr__ should never raise:

def __repr__(self) -> str:
    try:
        return f"<PiecesClient(host={self.host})>"
    except ValueError:
        return f"<PiecesClient(port={self._port!r}, not connected)>"

TEST-1 through TEST-4 (Medium) — Missing test coverage for several new code paths

• No direct tests for _host_validation_error (6 branches, only indirectly tested through 4 malformed host cases)
• No tests for _normalize_port with integer input (type hint accepts Union[str, int, None])
• No test for the valid port happy path (setting "39300" → verify connect_apis called correctly)
• No test for is_pieces_running retry logic (maxium_retries > 1)

COR-2 (Medium) — Redundant _normalize_port call in host getter

The host getter calls _normalize_port(self.port), but the port setter already normalizes and stores only valid values. The getter's validation is redundant — it could simply check if self._port: and return _host_from_port(self._port).

---

Nice to have (follow-up PR)

• ARCH-1: Upgrade _PORT_* string constants to a str Enum and the diagnostic dict to a dataclass for type safety and IDE autocomplete
• ARCH-3: Extract readiness/diagnostic logic into a mixin or helper class — PiecesClient is growing large and this is a cohesive unit

---

Positive observations

• The new urllib.parse.urlparse-based host validation in _host_validation_error is a significant security improvement over the old host.startswith("http") check (which accepted malformed URLs like "httpevil.com")
• Good parametrized test coverage for invalid port variants — 9 cases across 3 diagnostic codes including non-obvious inputs like "39300/tcp"
• Replacing the bare except: in is_pieces_running with specific except ValueError / except Exception is a welcome fix (bare except swallowed KeyboardInterrupt)

---

Reviewed via 6-agent parallel analysis covering correctness, architecture, security, testing, breaking changes, and performance — with a 3-agent verification pass on the highest-impact findings.