feat: add Calibre CLI harness for e-book library management#223
feat: add Calibre CLI harness for e-book library management#223OGRLEAF wants to merge 13 commits intoHKUDS:mainfrom
Conversation
- Add calibre/agent-harness with 58 tests (38 unit + 20 e2e) - Wraps calibredb, ebook-convert, ebook-meta for library operations - Includes SKILL.md for agent skill discovery - Update registry.json with calibre entry - Update .gitignore to track calibre/agent-harness/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Calibre “agent-harness” package to the cli-anything ecosystem, providing a Click CLI + REPL wrapper around Calibre’s real binaries (calibredb, ebook-convert, ebook-meta) plus unit/E2E tests and skill discovery docs.
Changes:
- Register the new
calibreharness inregistry.jsonand update root.gitignoreto track/calibre/agent-harness/. - Introduce a full Calibre CLI implementation (session handling, library ops, metadata ops, format conversion, export/catalog, custom columns) and a shared REPL skin.
- Add documentation + skill metadata (
README.md,CALIBRE.md,SKILL.md) and a sizeable pytest suite (unit + E2E/subprocess).
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
registry.json |
Adds a calibre entry to the registry for discovery/installation. |
calibre/agent-harness/setup.py |
Defines the Python package + console entry point for cli-anything-calibre. |
calibre/agent-harness/cli_anything/calibre/utils/repl_skin.py |
Adds a shared REPL “skin” (branding/prompt helpers). |
calibre/agent-harness/cli_anything/calibre/utils/calibre_backend.py |
Implements subprocess discovery/execution helpers for Calibre binaries. |
calibre/agent-harness/cli_anything/calibre/utils/__init__.py |
Marks utils package. |
calibre/agent-harness/cli_anything/calibre/tests/test_full_e2e.py |
Adds E2E tests running real Calibre tools + subprocess CLI verification. |
calibre/agent-harness/cli_anything/calibre/tests/test_core.py |
Adds unit tests for core modules and Click commands. |
calibre/agent-harness/cli_anything/calibre/tests/__init__.py |
Marks tests package. |
calibre/agent-harness/cli_anything/calibre/tests/TEST.md |
Documents the test plan and recorded results. |
calibre/agent-harness/cli_anything/calibre/skills/SKILL.md |
Adds skill metadata for agent discovery. |
calibre/agent-harness/cli_anything/calibre/core/session.py |
Implements persisted session state (library path). |
calibre/agent-harness/cli_anything/calibre/core/metadata.py |
Wraps Calibre metadata operations + OPF parsing. |
calibre/agent-harness/cli_anything/calibre/core/library.py |
Wraps calibredb library operations (list/search/add/remove/info/check). |
calibre/agent-harness/cli_anything/calibre/core/formats.py |
Wraps format listing/add/remove + conversion via ebook-convert. |
calibre/agent-harness/cli_anything/calibre/core/export.py |
Wraps export/catalog + chapter-to-PDF export pipeline. |
calibre/agent-harness/cli_anything/calibre/core/custom.py |
Wraps Calibre custom column management. |
calibre/agent-harness/cli_anything/calibre/core/__init__.py |
Marks core package. |
calibre/agent-harness/cli_anything/calibre/calibre_cli.py |
Implements the Click CLI entrypoint + REPL dispatcher. |
calibre/agent-harness/cli_anything/calibre/__main__.py |
Enables python -m cli_anything.calibre. |
calibre/agent-harness/cli_anything/calibre/__init__.py |
Defines package version. |
calibre/agent-harness/cli_anything/calibre/README.md |
Adds user-facing CLI documentation and examples. |
calibre/agent-harness/CALIBRE.md |
Adds harness SOP/architecture notes. |
calibre/agent-harness/.gitignore |
Adds harness-local ignores (Python build/test artifacts). |
.gitignore |
Updates root ignore rules to allow tracking /calibre/agent-harness/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Determine output path | ||
| if output_path is None: | ||
| output_path = str(Path(tmpdir) / f"converted.{output_fmt.lower()}") | ||
| else: | ||
| os.makedirs(Path(output_path).parent, exist_ok=True) |
There was a problem hiding this comment.
When output_path is not provided, it is created inside TemporaryDirectory() (tmpdir). After the with block exits, that file is deleted, so callers receive an output_path that no longer exists (and --no-add would silently discard the conversion output). Use a non-auto-deleted temp file (e.g., NamedTemporaryFile(delete=False)) or require/compute a persistent output location when add_to_library is false.
| # Parse and dispatch via Click | ||
| args = raw.split() | ||
| try: |
There was a problem hiding this comment.
The REPL command parser uses raw.split(), which does not respect quoting/escaping. Commands like meta set 1 title "My Book Title" will be split into multiple args and fail. Use shlex.split() (as other harnesses do) to correctly parse user input.
| file_count_before = sum(1 for _ in Path(output_dir).rglob("*") if _.is_file()) | ||
|
|
||
| cmd = ["export", "--to-dir", output_dir] | ||
| if export_all: | ||
| cmd.append("--all") | ||
| if formats: | ||
| cmd.extend(["--formats", ",".join(formats)]) | ||
| if template: | ||
| cmd.extend(["--template", template]) | ||
| if progress: | ||
| cmd.append("--progress") | ||
| if not export_all: | ||
| ids_str = ",".join(str(i) for i in book_ids) | ||
| cmd.append(ids_str) | ||
|
|
||
| result = run_calibredb(cmd, library_path=library_path) | ||
|
|
||
| exported_files = [ | ||
| str(p) for p in Path(output_dir).rglob("*") if p.is_file() | ||
| ] | ||
| new_files = exported_files[file_count_before:] | ||
|
|
There was a problem hiding this comment.
new_files = exported_files[file_count_before:] is not a reliable way to compute newly-exported files: Path.rglob() ordering is not guaranteed, and the new files will not necessarily appear contiguously at the end. This can produce incorrect exported_files/count results. Track the set of file paths before and after export and compute a set-difference instead.
|
|
||
| for f in new_files: | ||
| print(f" Exported: {f} ({Path(f).stat().st_size:,} bytes)") | ||
|
|
||
| return { | ||
| "output_dir": output_dir, | ||
| "book_ids": book_ids if not export_all else "all", | ||
| "exported_files": new_files, |
There was a problem hiding this comment.
export_books() prints per-file messages from the core module. Like convert_format(), this will break --json mode by emitting non-JSON text to stdout and makes the core API noisy for programmatic use. Move user-facing output into the Click command (or return it as structured fields).
| for f in new_files: | |
| print(f" Exported: {f} ({Path(f).stat().st_size:,} bytes)") | |
| return { | |
| "output_dir": output_dir, | |
| "book_ids": book_ids if not export_all else "all", | |
| "exported_files": new_files, | |
| exported_file_details = [ | |
| { | |
| "path": f, | |
| "size_bytes": Path(f).stat().st_size, | |
| } | |
| for f in new_files | |
| ] | |
| return { | |
| "output_dir": output_dir, | |
| "book_ids": book_ids if not export_all else "all", | |
| "exported_files": new_files, | |
| "exported_file_details": exported_file_details, |
| click.echo(f" {'ID':<5} {'Title':<40} {'Authors':<30} {'Formats'}") | ||
| click.echo(f" {'─'*5} {'─'*40} {'─'*30} {'─'*20}") | ||
| for b in book_list: | ||
| click.echo( | ||
| f" {b.get('id', ''):<5} " | ||
| f"{str(b.get('title', ''))[:40]:<40} " | ||
| f"{str(b.get('authors', ''))[:30]:<30} " | ||
| f"{b.get('formats', '')}" | ||
| ) |
There was a problem hiding this comment.
books list prints a “Formats” column (b.get('formats')), but core.library.list_books() intentionally excludes formats from its default field set. As a result, the Formats column will always be empty unless the user explicitly passes --fields=...,formats. Either include formats in the default fields used by this command (and adjust parsing accordingly), or drop the Formats column from the default human output.
| src_path = epub_dir / chapter["src"] | ||
| if not src_path.exists(): | ||
| print(f" ⚠ Missing source file: {chapter['src']}, skipping") | ||
| continue | ||
|
|
||
| out_name = _safe_chapter_filename(chapter["title"], chapter["order"]) + ".pdf" | ||
| out_path = Path(output_dir) / out_name | ||
| print(f" [{i}/{total}] {chapter['title']} → {out_name}") | ||
|
|
||
| try: | ||
| run_ebook_convert([str(src_path), str(out_path)]) | ||
| except RuntimeError as exc: | ||
| print(f" ⚠ Conversion failed (chapter {chapter['order']}): {exc}") | ||
| continue | ||
|
|
||
| if out_path.exists(): | ||
| exported_pdfs.append({ | ||
| "index": chapter["order"], | ||
| "title": chapter["title"], | ||
| "file": str(out_path), | ||
| "size": out_path.stat().st_size, | ||
| }) | ||
|
|
||
| print(f"\n Exported {len(exported_pdfs)}/{total} chapters to {output_dir}") |
There was a problem hiding this comment.
export_chapters_pdf() prints progress/warnings and a final summary using print(). When the CLI is run with --json, these writes will appear on stdout before the JSON payload and break machine parsing. Consider removing these prints (or routing them to stderr / a logger) and letting the Click command decide what to display.
| import json | ||
| import os | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from cli_anything.calibre.utils.calibre_backend import ( | ||
| run_calibredb, | ||
| find_calibredb, | ||
| ) | ||
|
|
||
|
|
||
| def library_info(library_path: str) -> dict: | ||
| """Return library statistics: book count, format counts, total size.""" | ||
| find_calibredb() # Ensure installed | ||
|
|
||
| # Count books: search with empty string returns all book IDs | ||
| result = run_calibredb( | ||
| ["search", ""], | ||
| library_path=library_path, | ||
| ) | ||
| id_output = result["stdout"].strip() | ||
| if id_output: | ||
| book_ids = [p.strip() for p in id_output.split(",") if p.strip().isdigit()] | ||
| book_count = len(book_ids) | ||
| else: | ||
| book_count = 0 | ||
|
|
||
| # Count format types by scanning library directory for book files | ||
| format_counts: dict[str, int] = {} | ||
| lib = Path(library_path) | ||
| skip_exts = {".db", ".jpg", ".png", ".opf", ".sqlite", ""} | ||
| for p in lib.rglob("*"): | ||
| if p.is_file(): | ||
| ext = p.suffix.upper().lstrip(".") | ||
| if ext and ext not in {"DB", "JPG", "PNG", "OPF", "SQLITE", "WEBP", "GIF"}: | ||
| format_counts[ext] = format_counts.get(ext, 0) + 1 |
There was a problem hiding this comment.
Unused imports/variables add noise and may fail linting: json, os, and Any are imported but unused, and skip_exts is defined but never referenced. Remove them (or use skip_exts in the extension filter) to keep the module minimal.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for f in new_files: | ||
| print(f" Exported: {f} ({Path(f).stat().st_size:,} bytes)") | ||
|
|
There was a problem hiding this comment.
This loop prints exported-file lines to stdout. When the CLI runs with --json, these prints will corrupt JSON output. Consider removing prints from core modules and emitting progress from the Click command layer, or writing progress logs to stderr.
| click.echo(f" {'ID':<5} {'Title':<40} {'Authors':<30} {'Formats'}") | ||
| click.echo(f" {'─'*5} {'─'*40} {'─'*30} {'─'*20}") | ||
| for b in book_list: | ||
| click.echo( | ||
| f" {b.get('id', ''):<5} " | ||
| f"{str(b.get('title', ''))[:40]:<40} " | ||
| f"{str(b.get('authors', ''))[:30]:<30} " | ||
| f"{b.get('formats', '')}" | ||
| ) |
There was a problem hiding this comment.
The CLI prints a “Formats” column, but list_books() defaults omit the formats field (by design), so this column will be empty/misleading in the default books list output. Either remove the Formats column from the default human table, or change the backend/fields parsing so a usable formats summary is included by default.
| export_result = run_calibredb( | ||
| ["export", "--to-dir", tmpdir, "--formats", input_fmt, str(book_id)], | ||
| library_path=library_path, | ||
| ) |
There was a problem hiding this comment.
In convert_format, the calibredb export command includes --all, which will export the entire library and ignore the provided book_id. This can be extremely slow and can also pick up the wrong input file for conversion. Drop --all and export only the requested book ID (or use a per-format export flow).
| continue | ||
|
|
||
| out_name = _safe_chapter_filename(chapter["title"], chapter["order"]) + ".pdf" | ||
| out_path = Path(output_dir) / out_name | ||
| print(f" [{i}/{total}] {chapter['title']} → {out_name}") | ||
|
|
||
| try: | ||
| run_ebook_convert([str(src_path), str(out_path)]) | ||
| except RuntimeError as exc: | ||
| print(f" ⚠ Conversion failed (chapter {chapter['order']}): {exc}") | ||
| continue | ||
|
|
There was a problem hiding this comment.
This function prints progress/warnings directly to stdout (including emoji). When invoked via the CLI with --json, these writes will corrupt JSON output. Consider routing progress to stderr (or behind a verbose flag), and keep stdout reserved for machine-readable JSON when --json is set.
| # Count format types by scanning library directory for book files | ||
| format_counts: dict[str, int] = {} | ||
| lib = Path(library_path) | ||
| skip_exts = {".db", ".jpg", ".png", ".opf", ".sqlite", ""} | ||
| for p in lib.rglob("*"): | ||
| if p.is_file(): | ||
| ext = p.suffix.upper().lstrip(".") | ||
| if ext and ext not in {"DB", "JPG", "PNG", "OPF", "SQLITE", "WEBP", "GIF"}: | ||
| format_counts[ext] = format_counts.get(ext, 0) + 1 | ||
|
|
There was a problem hiding this comment.
library_info walks the entire library directory with rglob("*") to compute format_counts. On large Calibre libraries this can be very slow and I/O-heavy for a common command. Consider deriving format counts from calibredb list / database queries (or make format scanning optional / behind a flag).
| # Count format types by scanning library directory for book files | |
| format_counts: dict[str, int] = {} | |
| lib = Path(library_path) | |
| skip_exts = {".db", ".jpg", ".png", ".opf", ".sqlite", ""} | |
| for p in lib.rglob("*"): | |
| if p.is_file(): | |
| ext = p.suffix.upper().lstrip(".") | |
| if ext and ext not in {"DB", "JPG", "PNG", "OPF", "SQLITE", "WEBP", "GIF"}: | |
| format_counts[ext] = format_counts.get(ext, 0) + 1 | |
| # Count format types from calibredb metadata instead of recursively scanning | |
| # the library directory, which is slow and I/O-heavy on large libraries. | |
| format_counts: dict[str, int] = {} | |
| formats_result = run_calibredb( | |
| ["list", "--fields=formats", "--for-machine"], | |
| library_path=library_path, | |
| ) | |
| formats_output = formats_result["stdout"].strip() | |
| if formats_output: | |
| try: | |
| format_rows = json.loads(formats_output) | |
| except json.JSONDecodeError: | |
| format_rows = [] | |
| if isinstance(format_rows, dict): | |
| rows = format_rows.values() | |
| elif isinstance(format_rows, list): | |
| rows = format_rows | |
| else: | |
| rows = [] | |
| for row in rows: | |
| if not isinstance(row, dict): | |
| continue | |
| formats_value = row.get("formats") | |
| if not formats_value: | |
| continue | |
| if isinstance(formats_value, str): | |
| formats = [f.strip() for f in formats_value.split(",") if f.strip()] | |
| elif isinstance(formats_value, list): | |
| formats = [str(f).strip() for f in formats_value if str(f).strip()] | |
| else: | |
| continue | |
| for fmt in formats: | |
| ext = fmt.upper() | |
| if ext and ext not in {"JPG", "PNG", "OPF", "SQLITE", "WEBP", "GIF"}: | |
| format_counts[ext] = format_counts.get(ext, 0) + 1 | |
| lib = Path(library_path) |
| Returns: | ||
| list of (style, text) tuples for prompt_toolkit. | ||
| """ | ||
| accent_hex = _ANSI_256_TO_HEX.get(self.accent, "#5fafff") |
There was a problem hiding this comment.
prompt_tokens() computes accent_hex but never uses it, which is dead code and can mislead future changes. Either remove the unused variable or use it to style the prompt tokens consistently.
| accent_hex = _ANSI_256_TO_HEX.get(self.accent, "#5fafff") |
| zf.extractall(epub_dir) | ||
|
|
||
| chapters = _parse_epub_chapters(epub_dir) |
There was a problem hiding this comment.
ZipFile.extractall() is used on EPUB content without validating member paths. A malicious EPUB can use path traversal entries (e.g. ../../.ssh/config) to write outside epub_dir (Zip Slip). Even though this is a temp dir, it can still overwrite arbitrary files. Consider implementing a safe extraction that rejects absolute paths and .. segments before writing files.
| setup( | ||
| name="cli-anything-calibre", | ||
| version="1.0.0", | ||
| description="CLI harness for Calibre e-book manager — part of the cli-anything toolkit", | ||
| long_description=open("cli_anything/calibre/README.md").read(), | ||
| long_description_content_type="text/markdown", | ||
| packages=find_namespace_packages(include=["cli_anything.*"]), | ||
| install_requires=[ | ||
| "click>=8.0.0", | ||
| "prompt-toolkit>=3.0.0", | ||
| ], |
There was a problem hiding this comment.
This setup.py doesn’t include package_data / include_package_data for non-.py assets (notably skills/SKILL.md). Other harness packages ship skills via package_data (e.g. adguardhome/agent-harness/setup.py:18-22). Without this, SKILL.md may be missing from installed distributions.
| def setUpClass(cls): | ||
| calibredb = shutil.which("calibredb") | ||
| if not calibredb: | ||
| raise RuntimeError( | ||
| "calibredb not found in PATH.\n" | ||
| "Calibre is a hard dependency — install it:\n" | ||
| " sudo apt-get install calibre" | ||
| ) | ||
|
|
There was a problem hiding this comment.
These E2E tests hard-fail when calibredb is missing by raising RuntimeError in setUpClass, which can break CI/test runs on environments without Calibre. Other harnesses typically skip E2E tests when external dependencies aren’t available (e.g. openscreen/.../test_full_e2e.py:81 skips if ffmpeg is missing). Consider raising unittest.SkipTest (or using pytest skips) unless an env var explicitly requests hard failure.
|
Thanks for the Calibre harness. I checked this against CONTRIBUTING.md, the PR template, and our CLI review requirements. File layout looks mostly correct: I can confirm the E2E file exists and is wired against the real Calibre tools. In my review environment I could not fully verify the test pass path because Could you please paste the full test results from a complete Calibre environment, including:
I also could not find a root I did not find an obvious serious bug from the diff review, but I’d like to see the full passing test evidence from a real Calibre environment before approving. |
- Replace repl_skin.py with unmodified canonical copy from cli-anything-plugin/ (adds skill_path auto-detection and banner display) - Add package_data to setup.py so skills/SKILL.md ships with pip install - Remove incorrect --all flag from convert_format() which exported the entire library instead of the specified book Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-Anything into feat/add-calibre-cli
… current working directory
… export tracking - Remove print() calls from convert_format(), export_books(), and export_chapters_pdf() to prevent stdout pollution in --json mode - Replace slice-based new file detection with set difference in export_books() for reliable results regardless of rglob() ordering - Capture skipped/failed chapters in skipped_chapters field instead of printing warnings, making errors accessible in JSON mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Add calibre accent color | ||
| skin.accent = "\033[38;5;166m" # warm brown-orange for calibre | ||
|
|
There was a problem hiding this comment.
The REPL overrides skin.accent manually, but ReplSkin’s prompt_toolkit style uses the internal accent mapping, so the REPL can end up with inconsistent colors. Prefer adding a 'calibre' entry in ReplSkin’s accent maps and removing this per-CLI override.
| # Add calibre accent color | |
| skin.accent = "\033[38;5;166m" # warm brown-orange for calibre |
| # Software accent colors — each software gets a unique accent | ||
| _ACCENT_COLORS = { | ||
| "gimp": "\033[38;5;214m", # warm orange | ||
| "blender": "\033[38;5;208m", # deep orange | ||
| "inkscape": "\033[38;5;39m", # bright blue |
There was a problem hiding this comment.
ReplSkin doesn’t include an accent color for 'calibre' in _ACCENT_COLORS, which forces calibre_cli.py to override skin.accent and still leaves prompt_toolkit styling to fall back to the default hex color. Add a 'calibre' entry here to keep behavior consistent.
| click.echo(f" {'ID':<5} {'Title':<40} {'Authors':<30} {'Formats'}") | ||
| click.echo(f" {'─'*5} {'─'*40} {'─'*30} {'─'*20}") | ||
| for b in book_list: | ||
| click.echo( | ||
| f" {b.get('id', ''):<5} " |
There was a problem hiding this comment.
books list prints a “Formats” column, but core.library.list_books() defaults to fields that don’t include 'formats', so this column will be empty unless users pass --fields including formats. Either include 'formats' in the default fields (and handle its parsing) or remove/replace the Formats column in the human output.
| _ANSI_256_TO_HEX = { | ||
| "\033[38;5;33m": "#0087ff", # audacity navy blue | ||
| "\033[38;5;35m": "#00af5f", # shotcut teal | ||
| "\033[38;5;39m": "#00afff", # inkscape bright blue | ||
| "\033[38;5;40m": "#00d700", # libreoffice green |
There was a problem hiding this comment.
If you add a Calibre ANSI accent color, also add it to _ANSI_256_TO_HEX so prompt_toolkit uses the intended accent (otherwise get_prompt_style() falls back to #5fafff).
| @click.argument("input_fmt") | ||
| @click.argument("output_fmt") | ||
| @click.option("--output", "-o", default=None, | ||
| help="Output file path (defaults to temp dir, auto-added to library)") |
There was a problem hiding this comment.
The --output option help says it defaults to a temp dir, but core.formats.convert_format() defaults to writing into the current working directory when output_path is None. Update the help text (or the implementation) so the CLI’s documented behavior matches what actually happens.
| help="Output file path (defaults to temp dir, auto-added to library)") | |
| help="Output file path (defaults to current working directory, auto-added to library)") |
| with zipfile.ZipFile(epub_files[0]) as zf: | ||
| # Validate each member path to prevent Zip Slip directory traversal attacks | ||
| for member in zf.infolist(): | ||
| member_path = (epub_dir / member.filename).resolve() | ||
| if not str(member_path).startswith(str(epub_dir.resolve())): |
There was a problem hiding this comment.
Zip Slip protection uses a string prefix check (startswith) on resolved paths, which can be bypassed by paths that share a prefix (e.g., /tmp/epub vs /tmp/epub_evil) and is brittle across platforms. Use Path.is_relative_to(epub_dir.resolve()) (Py3.10+) or a commonpath comparison to guarantee the member stays within epub_dir before extracting.
| with zipfile.ZipFile(epub_files[0]) as zf: | |
| # Validate each member path to prevent Zip Slip directory traversal attacks | |
| for member in zf.infolist(): | |
| member_path = (epub_dir / member.filename).resolve() | |
| if not str(member_path).startswith(str(epub_dir.resolve())): | |
| resolved_epub_dir = epub_dir.resolve() | |
| with zipfile.ZipFile(epub_files[0]) as zf: | |
| # Validate each member path to prevent Zip Slip directory traversal attacks | |
| for member in zf.infolist(): | |
| member_path = (resolved_epub_dir / member.filename).resolve() | |
| if not member_path.is_relative_to(resolved_epub_dir): |
|
|
||
| | Task | Tool | Command Pattern | | ||
| |------|------|----------------| | ||
| | Library operations | `calibredb` | `calibredb <cmd> --library-path <lib>` | |
There was a problem hiding this comment.
This SOP documents calibredb as using --library-path, but the harness/tests use Calibre’s actual flag --with-library. Update the documentation to match the real command-line usage to avoid confusing contributors/users.
| | Library operations | `calibredb` | `calibredb <cmd> --library-path <lib>` | | |
| | Library operations | `calibredb` | `calibredb --with-library <lib> <cmd>` | |
| <td>E-book Library Management</td> | ||
| <td><code>cli-anything-calibre</code></td> | ||
| <td>calibredb + ebook-convert + ebook-meta</td> | ||
| <td align="center">✅ <a href="calibre/agent-harness/">38</a></td> |
There was a problem hiding this comment.
The README lists Calibre as having “38” tests, but this PR includes 38 unit + 20 E2E tests (58 total). Update the displayed count (or clarify that it’s unit tests only) to keep README in sync with the test suite.
| <td align="center">✅ <a href="calibre/agent-harness/">38</a></td> | |
| <td align="center">✅ <a href="calibre/agent-harness/">58</a></td> |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| added_id = None | ||
| if add_to_library: | ||
| add_result = run_calibredb( | ||
| ["add_format", str(book_id), output_path], | ||
| library_path=library_path, |
There was a problem hiding this comment.
added_id and add_result are assigned but never used when add_to_library is true. Consider returning the add_format stdout (and/or re-reading formats) or removing these unused variables to avoid dead code and improve debuggability.
| click.echo(f" {'ID':<5} {'Title':<40} {'Authors':<30} {'Formats'}") | ||
| click.echo(f" {'─'*5} {'─'*40} {'─'*30} {'─'*20}") | ||
| for b in book_list: | ||
| click.echo( | ||
| f" {b.get('id', ''):<5} " | ||
| f"{str(b.get('title', ''))[:40]:<40} " | ||
| f"{str(b.get('authors', ''))[:30]:<30} " | ||
| f"{b.get('formats', '')}" | ||
| ) |
There was a problem hiding this comment.
The books list table always prints a “Formats” column, but _library.list_books() does not include formats in its default fields. This makes the header misleading for the common case where users don’t pass --fields.
| click.echo(f" {'ID':<5} {'Title':<40} {'Authors':<30} {'Formats'}") | |
| click.echo(f" {'─'*5} {'─'*40} {'─'*30} {'─'*20}") | |
| for b in book_list: | |
| click.echo( | |
| f" {b.get('id', ''):<5} " | |
| f"{str(b.get('title', ''))[:40]:<40} " | |
| f"{str(b.get('authors', ''))[:30]:<30} " | |
| f"{b.get('formats', '')}" | |
| ) | |
| requested_fields = {f.strip().lower() for f in field_list} if field_list else set() | |
| show_formats = "formats" in requested_fields | |
| header = f" {'ID':<5} {'Title':<40} {'Authors':<30}" | |
| separator = f" {'─'*5} {'─'*40} {'─'*30}" | |
| if show_formats: | |
| header += f" {'Formats'}" | |
| separator += f" {'─'*20}" | |
| click.echo(header) | |
| click.echo(separator) | |
| for b in book_list: | |
| row = ( | |
| f" {b.get('id', ''):<5} " | |
| f"{str(b.get('title', ''))[:40]:<40} " | |
| f"{str(b.get('authors', ''))[:30]:<30}" | |
| ) | |
| if show_formats: | |
| row += f" {b.get('formats', '')}" | |
| click.echo(row) |
| timeout: int = 120, | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Run calibredb with the given args and optional --library-path. |
There was a problem hiding this comment.
Docstring says “optional --library-path”, but the implementation appends --with-library. Update the docstring so it reflects the actual flag users should expect (and aligns with docs like CALIBRE.md).
| Run calibredb with the given args and optional --library-path. | |
| Run calibredb with the given args and optional --with-library. |
| | `test_parse_opf_multiple_authors` | Multiple dc:creator elements parsed as list | | ||
| | `test_parse_opf_no_namespace` | Handles OPF without explicit namespace declarations | |
There was a problem hiding this comment.
test_parse_opf_no_namespace is listed here, but it doesn’t exist in test_core.py currently. Either add that test case or remove/rename it in the inventory to keep TEST.md accurate.
| # Export only the target book and requested input format to get the source file | ||
| export_result = run_calibredb( | ||
| ["export", "--to-dir", tmpdir, "--formats", input_fmt, str(book_id)], | ||
| library_path=library_path, | ||
| ) |
There was a problem hiding this comment.
export_result is assigned but never used. Consider either removing the assignment, or incorporating its stdout/stderr into the returned payload (or error context) so failures in the export step are easier to debug.
| "gimp": "\033[38;5;214m", # warm orange | ||
| "blender": "\033[38;5;208m", # deep orange | ||
| "inkscape": "\033[38;5;39m", # bright blue | ||
| "audacity": "\033[38;5;33m", # navy blue |
There was a problem hiding this comment.
There’s no calibre entry in _ACCENT_COLORS, which leads the CLI to patch skin.accent at runtime. Add a calibre accent here so the banner/prompt colors are consistent without per-CLI overrides.
| "audacity": "\033[38;5;33m", # navy blue | |
| "audacity": "\033[38;5;33m", # navy blue | |
| "calibre": "\033[38;5;141m", # lavender |
omerarslan0
left a comment
omerarslan0
left a comment
There was a problem hiding this comment.
Harness architecture and code quality look solid, but SKILL.md is still at the old in-package path. The repo moved all skill files under top-level skills/ on 2026-04-18 and added a root-skill validation CI — as-is, CI will fail.
Before merge:
- Create
skills/cli-anything-calibre/SKILL.mdand updateskill_mdinregistry.json - Reconcile the test count discrepancy across README / PR description / TEST.md (38 vs 50 vs 58) into a single source
- Factor out the repeated
ctx.obj.get("as_json"/"session")boilerplate incalibre_cli.pyinto a small decorator/helper - Add at least smoke E2E coverage for the gaps flagged in TEST.md (
catalog,embed_metadata, custom columns)
Summary
Test plan
pytest calibre/agent-harness/cli_anything/calibre/tests/test_core.py -vpytest calibre/agent-harness/cli_anything/calibre/tests/test_full_e2e.py -vpip install git+https://github.com/OGRLEAF/CLI-Anything.git#subdirectory=calibre/agent-harnesscli-anything-calibre --help🤖 Generated with Claude Code