feat(cli): clearer PiecesOS readiness wording#453
Conversation
384e9e9 to
b6970c2
Compare
mark-at-pieces
left a comment
There was a problem hiding this comment.
Review: Approve
Clean, focused PR that solves a real UX problem (misleading error messages) with appropriate defensive architecture and strong testing. Ship it.
Correctness
The old messages ("PiecesOS is not installed", "PiecesOS is not running") overclaimed what open_pieces_os() can actually determine — it only knows the service is unreachable, not why. The new "not reachable" framing is factually accurate and gives users actionable next steps without false certainty.
All three edited files only replace string arguments in existing print/progress calls. No control flow changes, no new branches, no changed return values.
Architecture
Placing the helper at src/pieces/readiness_messages.py (top-level, stdlib-only) is the correct call. Every pieces.core module imports Settings at module top, so placing this under pieces.core would create a pieces.settings → pieces.core.readiness_messages → pieces.settings cycle. The chosen location mirrors the existing pieces.urls pattern — proven safe in this codebase. The module docstring clearly documents these constraints for future maintainers.
Testing
18 tests is thorough for a wording change:
- Subprocess circular-import smoke test — particularly well-designed. Running in a subprocess ensures Python's module cache can't mask a real cycle (in-process tests would pass even with a cycle because modules are already cached).
- Parametrized regression guards (3 files × 2 forbidden strings) prevent future drift back to overclaiming language.
- Excluded tests (
test_e2e.py,links_test.py) are justified as pre-existing environmental failures on clean main.
Two minor suggestions (non-blocking)
-
Self-referential suggestion in
open_command.py: When a user runspieces open --copilotand PiecesOS fails to launch, the readiness message suggests "Launch PiecesOS:pieces open" as step 2 — which is what just failed. In practice this is probably fine since plainpieces open(without flags) follows a different code path, and the generic message serves all call sites well. Just noting the mild awkwardness. -
Unclosed Rich tag in
pieces_os_not_reachable_short(): The string starts with[red]but never closes with[/red]. Rich auto-applies unclosed tags to the rest of the string (works correctly here), but explicit closure would prevent accidental bleed if the string is ever concatenated with other content.
Performance / Security / Dependencies / Breaking Changes
No concerns across any of these dimensions. Stdlib-only module, no new deps, no API changes, no input handling affected.
Nice work on the defensive module docstring and the subprocess import test — both demonstrate good engineering instincts around preventing regression in a codebase with known import-order sensitivity.
Summary
pieces.readiness_messageshelper to avoidpieces.corecircular import risk..port.txtclaims, no install-state overclaim, and subprocess import smoke.Non-goals
install_pieces_os.pychanges.PiecesInstaller,open_pieces_os, orSettings.startup.Why a top-level helper
Every module under
pieces.coreimportsSettingsat module top, sopieces.settings -> pieces.core.<x>would re-enter a partially-loadedpieces.settingsand raiseImportError. The helper lives at top-levelsrc/pieces/readiness_messages.pywith stdlib-only imports, mirroring howpieces.urlsis safely imported bypieces.settings. A subprocess import-smoke test intests/test_readiness_messages.pyguards against regressions.Wording
Long-form (used by
open_command.pyand the onboarding headline; rendered viaSettings.logger.print(Markdown(...))):Short-form (used by the
settings.pyRichProgressdescription; must stay single-line):Files
Edited (3):
Added (2):
Validation
Why `test_e2e.py` and `links_test.py` are excluded
Both files reproduce as pre-existing environmental failures on clean main, not caused by this PR. Verified by re-running each on `main` with this branch's changes stashed:
Including them would mask the real signal from this PR's targeted suites, so they are skipped only for the full-suite run. The targeted MCP gateway validation suites (`test_validation_core.py`, `test_validation_advanced.py`) are run separately and pass.
Manual smoke
Forced the open_command failure branch via the `copilot` kwarg path (the one explicitly called out for manual smoke) by mocking `open_pieces_os` to return False, since PiecesOS is currently running on the dev box. Output is the rendered long-form Markdown block with bullets and numbered next steps; short-form renders as a single-line red Rich progress description.
Test plan