Skip to content

fix(sdks/python-cli): preserve structured JSON errors for unreadable and directory paths in from-segments - #13355

Open
CreamPle wants to merge 1 commit into
BasedHardware:mainfrom
CreamPle:fix/cli-from-segments-file-validation
Open

fix(sdks/python-cli): preserve structured JSON errors for unreadable and directory paths in from-segments#13355
CreamPle wants to merge 1 commit into
BasedHardware:mainfrom
CreamPle:fix/cli-from-segments-file-validation

Conversation

@CreamPle

@CreamPle CreamPle commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes omi conversation from-segments crashing with unhandled unexpected error: ... when passed a directory or an unreadable file.

  • Checks segments_file.is_dir() and emits a structured UsageError before attempting file reads.
  • Catches OSError (such as PermissionError) on reading the file and routes it to UsageError.
  • Preserves the machine-readable JSON error contract in --json mode with stable exit_code = 1.

Resolves #13013

Changes

  • sdks/python-cli/omi_cli/commands/conversation.py: Validate file vs directory and catch OSError in from_segments.
  • sdks/python-cli/tests/test_conversation.py: Add unit tests asserting JSON error output when targeting a directory or unreadable file.

Verification

  • Unit tests in sdks/python-cli/tests/test_conversation.py pass (13/13).
  • Clean code formatting verified with ruff check.
  • Verified diff hygiene with git diff --check HEAD (clean).

Failure-Class: none

Review in cubic

…and directory paths in from-segments

Catch directory and file read errors before loading transcript segments in conversation from-segments, emitting a UsageError so JSON error contracts are preserved.

Resolves BasedHardware#13013

Co-authored-by: aceletapp <326262726+aceletapp@users.noreply.github.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @CreamPle — clean, focused fix that fully resolves the structured-error gap from #13013.

sdks/python-cli/omi_cli/commands/conversation.py

  • The new is_dir() guard sits in the right place (after the exists() check, before any read) and emits a structured UsageError with actionable detail, so --json consumers get a parseable error with the stable exit code 1 instead of the raw IsADirectoryError.
  • Wrapping segments_file.read_bytes() in except OSError and routing to a Cannot read file UsageError correctly covers PermissionError and friends; detail=str(exc) keeps the OS context without breaking the JSON envelope. Since Path.exists()/is_dir() suppress OSError, an unreadable parent dir lands in the File not found branch and an unreadable file lands here — both structured, no gaps left.
  • Dropping the now-unused import json is correct; nothing else in the module references it.

sdks/python-cli/tests/test_conversation.py

  • Both new tests assert the full contract: exit code 1, machine-readable JSON on stderr with the expected message, and not respx_mock.calls (no network attempted). Invoking main() with a patched sys.argv exercises the real error-rendering path rather than the test-runner shortcut, which is the right level for verifying JSON error output.
  • The monkeypatch.setattr(Path, "read_bytes", fail_read) patch is class-level and therefore broad, but it is scoped to the test and reverted by monkeypatch; I ran the full tests/test_conversation.py suite in a clean container (13/13 pass) and the broader CI suite shows no fallout from it.

On the red required checks (Windows ACL / Python 3.12, Linux / Python 3.10, Linux / Python 3.12): all three fail on tests/test_auth_api_key.py::test_transport_failure_during_login_leaves_saved_config_unchanged, which is untouched by this PR and fails identically on the latest runs of main. So it is a pre-existing failure unrelated to this change, not something you need to fix here — but it will need to clear before merge.

Nice first contribution. Leaving for human maintainer review and merge once the pre-existing CI failure is sorted.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added positive-signal Automation verified a genuine fix/quality contribution python labels Sep 9, 2026

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug fix (crash on directory/unreadable path in conversation from-segments), closes #13013, small scoped diff with tests. Not merging: hard-floor CI gate (Linux/Windows Python matrix red, pre-existing/unrelated main-branch failure).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

positive-signal Automation verified a genuine fix/quality contribution python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI transcript import breaks JSON errors for unreadable or invalid-UTF-8 files

3 participants