Add export/import DB commands and data-dir helper#442
Add export/import DB commands and data-dir helper#442nolan-at-pieces wants to merge 1 commit intomainfrom
Conversation
Introduce ExportCommand and ImportCommand to export/import the com.pieces.os database (zip export, optional compression, full export, and restore from zip or extracted dir). Commands include progress reporting, validation, backups of existing production on import, and require PiecesOS to be quit. Register the new commands and mark them as onboarding/bypass-login-safe in the CLI. Add get_pieces_os_data_dir() to constants to locate the Pieces OS data directory per platform and export it from the module.
nolan-at-pieces
requested review from
bishoy-at-pieces,
Copilot,
mack-at-pieces and
mark-at-pieces
There was a problem hiding this comment.
Pull request overview
Adds CLI support for exporting and importing the PiecesOS database (com.pieces.os) to/from a zip (optionally compressed) or an extracted directory, plus a helper to locate the PiecesOS data directory per platform. This fits into the CLI command system by registering new commands and updating onboarding/bypass logic so users can run export/import without onboarding or requiring PiecesOS to be running.
Changes:
- Add
get_pieces_os_data_dir()to locate the PiecesOS data directory by platform. - Introduce
ExportCommandandImportCommand(aliasrestore) with progress reporting, validation, and backup behavior. - Register new commands in the command interface and exempt them from onboarding / PiecesOS startup gating.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/pieces/config/constants.py |
Adds a new helper to resolve the PiecesOS data directory and exports it via __all__. |
src/pieces/command_interface/db_commands.py |
New export/import command implementations (zip creation/extraction, copying, progress UI, backup behavior). |
src/pieces/command_interface/__init__.py |
Exposes the new commands so they are imported and registered by the CLI. |
src/pieces/app.py |
Updates onboarding and “needs PiecesOS” gating to include export/import/restore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| and not onboarded | ||
| and not ignore_onboarding | ||
| and not self.command == "completion" | ||
| and self.command not in ("completion", "export", "import", "restore") |
There was a problem hiding this comment.
self.command is referenced here, but it is never assigned anywhere in PiecesCLI.run() (only the local command = args.command is set). On first-run (when onboarding is triggered) this will raise AttributeError and break onboarding. Use the local command variable in this condition or assign self.command = command before this block.
| ) | ||
| bytes_so_far = 0 | ||
| for m in members: | ||
| zf.extract(m, tmp_path) |
There was a problem hiding this comment.
ZipFile.extract() is used directly on untrusted archive members. This is vulnerable to path traversal (Zip Slip) if a zip contains entries like ../... or absolute paths, allowing writes outside the temp directory. Validate each ZipInfo.filename resolves within tmp_path (and consider rejecting absolute/parent paths) before extracting.
| zf.extract(m, tmp_path) | |
| # Prevent Zip Slip / path traversal by validating the target path | |
| member_path = Path(m.filename) | |
| try: | |
| target_path = (tmp_path / member_path).resolve() | |
| except Exception as e: | |
| Settings.logger.print( | |
| f"[red]Invalid export entry {m.filename!r}: {e}[/red]" | |
| ) | |
| return 1 | |
| # Ensure the resolved path is still within the temporary directory | |
| if not target_path.is_relative_to(tmp_path): | |
| Settings.logger.print( | |
| f"[red]Invalid export entry {m.filename!r}: path traversal detected.[/red]" | |
| ) | |
| return 1 | |
| target_path.parent.mkdir(parents=True, exist_ok=True) | |
| with zf.open(m, "r") as src, open(target_path, "wb") as dst: | |
| shutil.copyfileobj(src, dst) |
| backup_path = pieces_os_dir / backup_name | ||
| Settings.logger.print( | ||
| f"[yellow]Backing up existing database to {backup_name}...[/yellow]" | ||
| ) | ||
| shutil.move(str(production_dst), str(backup_path)) |
There was a problem hiding this comment.
The import flow is described as replacing the current database, but only production/ is backed up/moved. Other existing subdirectories/files under pieces_os_dir (e.g., staging, old backups, stray files) will remain and can be mixed with the restored data. Consider backing up and replacing the entire pieces_os_dir (or deleting all existing contents except the backup) to ensure a clean restore.
| rel = item.relative_to(source) | ||
| dst = pieces_os_dir / rel | ||
| dst.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(item, dst) | ||
| bytes_so_far += size |
There was a problem hiding this comment.
Files are copied into pieces_os_dir without first removing pre-existing files (other than the production folder move above). This can leave stale files in place when restoring from a production-only export, producing an inconsistent on-disk state. Consider restoring into a temporary directory and then atomically swapping/moving into place, or explicitly cleaning the destination tree before copying.
| if system == "Windows": | ||
| return home / "Documents" / "com.pieces.os" |
There was a problem hiding this comment.
get_pieces_os_data_dir() hardcodes platform paths under Path.home() (e.g., ~/Documents on Windows). On Windows/macOS these folders can be redirected/relocated (OneDrive, custom home layouts), so this may point at a non-existent directory even when PiecesOS is installed. Consider using platformdirs APIs / OS-known-folder resolution, or allow an override (env var/config) and fall back to the hardcoded defaults.
| class ExportCommand(BaseCommand): | ||
| """Export Pieces OS database to a zip archive.""" | ||
|
|
||
| def get_name(self) -> str: | ||
| return "export" | ||
|
|
||
| def get_help(self) -> str: | ||
| return "Export Pieces OS database to a zip archive" |
There was a problem hiding this comment.
This PR introduces substantial new filesystem/archiving behavior (exporting to zip, importing from zip/dir, backup/restore semantics) but there are no tests covering the success path or failure/validation paths. Given the risk of data loss, add unit tests using a temporary directory to validate: export creates expected zip structure, import restores correctly, and invalid inputs (bad zip, missing production/) are rejected.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| except (OSError, shutil.Error) as e: | ||
| Settings.logger.print(f"[red]Import failed: {e}[/red]") | ||
| return 1 |
There was a problem hiding this comment.
Bug: The database import command fails to restore the original database from its backup if an error occurs midway through the copy process, potentially leading to data loss.
Severity: CRITICAL
Suggested Fix
In the except block for OSError and shutil.Error in the _copy_to_pieces_os function, add logic to restore the database from the backup. This could involve deleting the partially copied production directory and moving the backup directory back to its original production name. Also, inform the user that the rollback has occurred.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/pieces/command_interface/db_commands.py#L462-L464
Potential issue: In the `_copy_to_pieces_os` function, if an `OSError` or `shutil.Error`
occurs during the file copy process (e.g., due to a full disk), the exception is caught,
an error is printed, and the function exits. However, the original `production`
database, which was moved to a backup location at the start of the process, is not
restored. This leaves the application with an incomplete or corrupt `production`
directory, and the user's original data is left in a backup folder without any
instructions on how to manually restore it, creating a potential data loss scenario.
Did we get this right? 👍 / 👎 to inform future reviews.
| if system == "Windows": | ||
| return home / "Documents" / "com.pieces.os" |
There was a problem hiding this comment.
Bug: The hardcoded Windows path for the Pieces OS data directory in get_pieces_os_data_dir is incorrect, causing import/export commands to fail for all Windows users.
Severity: CRITICAL
Suggested Fix
Update the get_pieces_os_data_dir function to use the correct path for Windows, which is ~/AppData/Local/Mesh Intelligent Technologies, Inc/Pieces OS/com.pieces.os/. Consider using a more robust method to locate the directory, such as checking environment variables or the registry, if available, to avoid future breakages.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/pieces/config/constants.py#L28-L29
Potential issue: The `get_pieces_os_data_dir` function hardcodes the Pieces OS data
directory path for Windows as `~/Documents/com.pieces.os`. However, the actual data
directory on Windows is typically located at `~/AppData/Local/Mesh Intelligent
Technologies, Inc/Pieces OS/com.pieces.os/`. This incorrect path will cause the `import`
and `export` commands to fail for all Windows users, as they will be unable to locate
the Pieces OS data directory, rendering the feature non-functional on that platform.
Did we get this right? 👍 / 👎 to inform future reviews.
| # Verify fidelity: production folder file count and byte count must match | ||
| prod_dst = pieces_os_dir / "production" | ||
| prod_src = source / "production" | ||
| if prod_dst.exists() and prod_src.exists(): | ||
| dst_files = [p for p in prod_dst.rglob("*") if p.is_file()] |
There was a problem hiding this comment.
Bug: The import command returns a success code and reports completion even if the post-copy verification step detects a data mismatch, ignoring the verification failure.
Severity: HIGH
Suggested Fix
Modify the logic after the verification step. If files_match or bytes_match is False, the function should not print a success message. Instead, it should return a non-zero exit code to indicate failure and provide clear instructions to the user on how to proceed, such as suggesting a manual check or re-running the import.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/pieces/command_interface/db_commands.py#L432-L436
Potential issue: After the database import process completes, a verification step checks
if the source and destination `production` directories match in file count and total
size. If a mismatch is detected, a warning is printed to the console. However, the
function's control flow ignores the result of this verification. It proceeds to print a
success message and returns an exit code of 0, indicating success, even when the
verification has failed. This misleads the user into believing the import was successful
when it may have been incomplete or corrupt.
Did we get this right? 👍 / 👎 to inform future reviews.
Introduce ExportCommand and ImportCommand to export/import the com.pieces.os database (zip export, optional compression, full export, and restore from zip or extracted dir).
Commands include progress reporting, validation, backups of existing production on import, and require PiecesOS to be quit. Register the new commands and mark them as onboarding/bypass-login-safe in the CLI. Add get_pieces_os_data_dir() to constants to locate the Pieces OS data directory per platform and export it from the module.