feat(cli): add backup subcommands (backup, backup list, backup restore)#568
feat(cli): add backup subcommands (backup, backup list, backup restore)#568
Conversation
Add three CLI subcommands for managing backups via the backend REST API: - `synthorg backup` (and `backup create`): trigger manual backup, display manifest - `synthorg backup list`: show available backups in formatted table - `synthorg backup restore <id> --confirm`: restore from backup with safety gate Includes 17 tests covering helpers (formatSize, isValidBackupID, componentsString, parseAPIResponse), happy paths, and error cases (conflict, not found, invalid manifest, unreachable backend, invalid ID, missing confirm flag). Closes #542 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-reviewed by 5 agents, 17 findings addressed: - Extract helpers to keep functions under 50-line limit (printManifest, printBackupTable, apiErrorMessage, renderRestoreSuccess, writeTestConfig) - Use url.JoinPath for defense-in-depth URL construction - Remove dead `_ = data` assignments, collapse identical switch cases - Move safeStateDir validation before API call for consistency with stop.go - Add restart_required=true test covering handleRestartAfterRestore path - Add t.Parallel() prohibition comment for global rootCmd test safety - Update CLAUDE.md, operations.md, user_guide.md, README.md with backup cmd Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new CLI backup subsystem: Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as Backup CLI
participant API as Backend API
participant Docker as Docker/Compose
rect rgba(100, 150, 200, 0.5)
Note over User,API: Backup Create Flow
User->>CLI: synthorg backup
CLI->>API: POST /api/v1/admin/backups
API-->>CLI: 200 OK + manifest JSON
CLI-->>User: Display manifest details
end
rect rgba(100, 200, 150, 0.5)
Note over User,API: Backup List Flow
User->>CLI: synthorg backup list
CLI->>API: GET /api/v1/admin/backups
API-->>CLI: 200 OK + backups array
CLI-->>User: Display formatted table
end
rect rgba(200, 150, 100, 0.5)
Note over User,Docker: Backup Restore Flow
User->>CLI: synthorg backup restore <id> --confirm
CLI->>CLI: Validate backup ID & local state
CLI->>Docker: Stop containers (if required)
Docker-->>CLI: Containers stopped
CLI->>API: POST /api/v1/admin/backups/restore
API-->>CLI: 200 OK + restore response
CLI->>Docker: Restart containers (if required)
CLI-->>User: Display restore result & safety backup ID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI by introducing robust backup management capabilities. Users can now easily create, list, and restore system backups directly from the command line, improving data resilience and operational control. The changes ensure a streamlined experience for managing the application's state, with built-in safeguards and clear user guidance for critical operations like restoring data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive backup and restore functionality to the CLI, including commands to create, list, and restore backups via the backend API. The implementation is robust, with thorough test coverage for various success and failure scenarios. My review focuses on enhancing the consistency of error handling to improve the overall user experience when interacting with the CLI.
| if err := json.Unmarshal(data, &manifest); err != nil { | ||
| return fmt.Errorf("parsing backup manifest: %w", err) | ||
| } |
There was a problem hiding this comment.
There's an inconsistency in error handling. API-level errors (like non-2xx status codes or malformed response envelopes) are handled gracefully by printing a clean error message and exiting. However, an error from parsing the data payload of a successful response (e.g., a malformed backup manifest) returns a Go error, which can lead to a non-zero exit code and a stack trace. For a more consistent and user-friendly experience, this type of API data parsing error should also be handled gracefully.
| if err := json.Unmarshal(data, &manifest); err != nil { | |
| return fmt.Errorf("parsing backup manifest: %w", err) | |
| } | |
| if err := json.Unmarshal(data, &manifest); err != nil { | |
| out.Error(fmt.Sprintf("parsing backup manifest: %v", err)) | |
| return nil | |
| } |
| if err := json.Unmarshal(data, &backups); err != nil { | ||
| return fmt.Errorf("parsing backup list: %w", err) | ||
| } |
There was a problem hiding this comment.
Similar to other command handlers in this file, there's an opportunity to make error handling more consistent. When parsing the list of backups from the API response fails, the command currently returns a Go error. To align with the handling of other API-related errors, it would be better to print a user-friendly error message and exit gracefully.
| if err := json.Unmarshal(data, &backups); err != nil { | |
| return fmt.Errorf("parsing backup list: %w", err) | |
| } | |
| if err := json.Unmarshal(data, &backups); err != nil { | |
| out.Error(fmt.Sprintf("parsing backup list: %v", err)) | |
| return nil | |
| } |
| if err := json.Unmarshal(data, &resp); err != nil { | ||
| return fmt.Errorf("parsing restore response: %w", err) | ||
| } |
There was a problem hiding this comment.
For consistency with how other API response errors are handled in this file, it would be better to treat a failure in parsing the restore response payload as a user-facing error rather than a programmatic one. This means printing a clean error message and returning nil instead of propagating a Go error, which would provide a more uniform experience for the CLI user.
| if err := json.Unmarshal(data, &resp); err != nil { | |
| return fmt.Errorf("parsing restore response: %w", err) | |
| } | |
| if err := json.Unmarshal(data, &resp); err != nil { | |
| out.Error(fmt.Sprintf("parsing restore response: %v", err)) | |
| return nil | |
| } |
| if statusCode == http.StatusNotFound { | ||
| out.Error(fmt.Sprintf("Backup not found: %s", backupID)) | ||
| out.Hint("Run 'synthorg backup list' to see available backups") | ||
| } else { | ||
| out.Error(msg) | ||
| } |
There was a problem hiding this comment.
When a 404 Not Found error occurs, the current implementation discards the specific error message from the API and displays a generic one. The API's message might contain more context (e.g., why the backup was not found). It would be better to use the API's error message when it's available and only fall back to a generic message if the API response is unhelpful.
| if statusCode == http.StatusNotFound { | |
| out.Error(fmt.Sprintf("Backup not found: %s", backupID)) | |
| out.Hint("Run 'synthorg backup list' to see available backups") | |
| } else { | |
| out.Error(msg) | |
| } | |
| if statusCode == http.StatusNotFound { | |
| // The API may provide a more specific error message. Use it, but | |
| // fall back to a default if the API returns a generic one. | |
| if msg != "restore failed" { | |
| out.Error(msg) | |
| } else { | |
| out.Error(fmt.Sprintf("Backup not found: %s", backupID)) | |
| } | |
| out.Hint("Run 'synthorg backup list' to see available backups") | |
| } else { | |
| out.Error(msg) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/backup_test.go`:
- Around line 206-224: The test helper runBackupCmd resets the persistent
--confirm flag via backupRestoreCmd.Flags().Set("confirm", "false") to avoid
Cobra's sticky global flag state; add a brief explanatory comment above that
line inside runBackupCmd describing this pattern (that Cobra preserves flag
values across Execute() calls and any new persistent flags on the backup
subcommands will need the same reset) so future maintainers know to clear added
flags like --confirm when extending tests.
In `@cli/cmd/backup.go`:
- Around line 268-277: Add a short explanatory comment inside runBackupCreate
(around the statusCode check and parseAPIResponse handling) stating that errors
are printed with out.Error but not returned so the CLI process exits with code 0
because the command successfully communicated with the backend (referencing the
same pattern documented in handleRestoreError); also verify and mention
consistency with other CLI commands or update docs if this behavior differs.
Ensure the comment sits immediately before the blocks that call out.Error and
return nil so future readers understand the intentional choice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ceab9bfc-dfbd-46b1-867d-2ef19f6f3360
📒 Files selected for processing (6)
CLAUDE.mdREADME.mdcli/cmd/backup.gocli/cmd/backup_test.godocs/design/operations.mddocs/user_guide.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/{*.py,*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Handle errors explicitly, never silently swallow them
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Go CLI: entry point atcli/main.go, Cobra commands incli/cmd/, internal modules incli/internal/
Go linting: golangci-lint + go vet
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go testing: native
testing.Ffuzz functions for property-based testing; fuzz targets run seed corpus only without-fuzzflag
Files:
cli/cmd/backup_test.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go CLI commands with root flags:
--data-dir,--skip-verify; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
{src/synthorg/**/*.py,tests/**/*.py,**/*.md,web/**/{*.ts,*.js,*.vue}}
📄 CodeRabbit inference engine (CLAUDE.md)
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases. Vendor names only in: (1) Operations design page, (2).claude/files, (3) third-party import paths/modules
Files:
docs/user_guide.mdCLAUDE.mddocs/design/operations.mdREADME.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Markdown: use for all documentation files (
docs/,site/, README, etc.)
Files:
docs/user_guide.mdCLAUDE.mddocs/design/operations.mdREADME.md
docs/design/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
docs/design/*.md: When approved deviations occur, update the relevantdocs/design/page to reflect the new reality
Design spec pages: 7 pages indocs/design/— index, agents, organization, communication, engine, memory, operations
Files:
docs/design/operations.md
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/cmd/**/*.go : Go CLI commands with root flags: `--data-dir`, `--skip-verify`; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
docs/user_guide.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to docker/Dockerfile.sandbox : Docker sandbox: `synthorg-sandbox` — Python 3.14 + Node.js + git, non-root (UID 10001), agent code execution sandbox
Applied to files:
docs/user_guide.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/cmd/**/*.go : Go CLI commands with root flags: `--data-dir`, `--skip-verify`; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Applied to files:
cli/cmd/backup.goCLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/**/*.go : Go CLI: entry point at `cli/main.go`, Cobra commands in `cli/cmd/`, internal modules in `cli/internal/`
Applied to files:
cli/cmd/backup.goCLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/backup.goCLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/go.mod : CLI dependencies in `cli/go.mod`: Go 1.26+, Cobra, charmbracelet/huh, charmbracelet/lipgloss, sigstore-go, go-containerregistry, go-tuf
Applied to files:
cli/cmd/backup.goCLAUDE.mddocs/design/operations.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Shell commands: for Go CLI work, cd cli is an exception because Go tooling requires working directory to be the module root. Go commands require `cd cli` for other work, never use `cd`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-18T21:35:45.199Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.199Z
Learning: Go requires: `cd cli` because Go module is in `cli/` — this is an exception to the "never use cd" rule
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/**/*.go : Go linting: golangci-lint + go vet
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to docker/{Dockerfile*,compose.yml} : Docker: Backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened. Web uses nginxinc/nginx-unprivileged, Vue 3 SPA with PrimeVue + Tailwind CSS, SPA routing, API/WebSocket proxy to backend.
Applied to files:
README.md
🧬 Code graph analysis (2)
cli/cmd/backup_test.go (1)
cli/cmd/root.go (1)
Execute(101-107)
cli/cmd/backup.go (3)
cli/internal/ui/ui.go (2)
UI(44-53)NewUI(58-70)cli/internal/config/state.go (1)
Load(53-91)cli/internal/docker/client.go (1)
Detect(34-66)
🪛 GitHub Check: CodeQL
cli/cmd/backup.go
[failure] 426-426: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (10)
docs/user_guide.md (1)
27-27: LGTM!Documentation correctly updated to include the new
synthorg backupcommand in the CLI commands list.docs/design/operations.md (1)
975-976: LGTM!Design documentation correctly updated to reflect the new
backupCLI command with its three operations (create/list/restore).CLAUDE.md (1)
154-154: LGTM!CLAUDE.md correctly updated to include the
backupcommand in the CLI command list, maintaining consistency with the implementation.cli/cmd/backup_test.go (1)
1-18: LGTM!Good test organization with clear separation between unit tests for helper functions and integration tests for command flows. The note about avoiding
t.Parallel()due to shared globalrootCmdis important documentation for maintainability.cli/cmd/backup.go (5)
26-71: LGTM!Well-structured Cobra command setup following the existing CLI patterns. The default behavior of
backup(equivalent tobackup create) is intuitive, and the--confirmflag as a safety gate for restore is a good practice.
76-117: LGTM!API response types correctly mirror the Python backend models. Using
json.RawMessagefor the data field provides flexibility in parsing different response payloads.
156-188: LGTM!The
backupAPIRequestfunction has good defensive practices:
- Timeout bounds the request
- Context allows cancellation
- Response body limited to 1MB to prevent memory exhaustion
- Clear error wrapping with "backend unreachable" message
331-380: LGTM — validation order is correct.The validation sequence follows the expected pattern (consistent with stop.go per the commit message):
- Validate backup ID format
- Check
--confirmflag- Load config
- Validate state directory paths
421-446: No action needed—path sanitization is already in place.The CodeQL concern at line 426 is addressed:
safeDiris validated throughsafeStateDir()(line 356), which callsconfig.SecurePath(). This function:
- Cleans the path lexically via
filepath.Clean()to remove..sequences- Validates it is absolute via
filepath.IsAbs(), blocking relative traversal attemptsThe path is then safely joined with
"compose.yml"(a hardcoded relative name). Comprehensive tests (TestSecurePath_DotDotRelative,FuzzSecurePath) confirm traversal paths are rejected. The implementation satisfies the CodeQLgo/path-injectioncheck.README.md (1)
155-155: LGTM!README correctly updated to include "backup management" in the CLI capabilities list.
| if statusCode < 200 || statusCode >= 300 { | ||
| out.Error(apiErrorMessage(body, "backup failed")) | ||
| return nil | ||
| } | ||
|
|
||
| data, err := parseAPIResponse(body) | ||
| if err != nil { | ||
| out.Error(err.Error()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify why API errors return nil instead of propagating as Go errors.
The pattern of displaying an error via out.Error() but returning nil (lines 269-271, 275-277) is intentional (as noted in handleRestoreError docstring), but it may be worth adding a brief comment here for consistency, especially since runBackupCreate doesn't have such documentation.
This pattern means the CLI exit code is 0 even when the backend reports an error. If this is intentional (CLI successfully communicated), consider documenting it or verifying this matches the behavior of other CLI commands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/backup.go` around lines 268 - 277, Add a short explanatory comment
inside runBackupCreate (around the statusCode check and parseAPIResponse
handling) stating that errors are printed with out.Error but not returned so the
CLI process exits with code 0 because the command successfully communicated with
the backend (referencing the same pattern documented in handleRestoreError);
also verify and mention consistency with other CLI commands or update docs if
this behavior differs. Ensure the comment sits immediately before the blocks
that call out.Error and return nil so future readers understand the intentional
choice.
API error responses (409, 404, 422, 500) now return a non-nil error from RunE so the CLI exits non-zero, enabling correct behavior in scripts: `synthorg backup && upload.sh` will not proceed on failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/backup_test.go`:
- Around line 211-213: The call to backupRestoreCmd.Flags().Set("confirm",
"false") currently discards any error; instead check its returned error and fail
the test when it’s non-nil (e.g., if err :=
backupRestoreCmd.Flags().Set("confirm", "false"); err != nil { t.Fatalf("reset
flag confirm: %v", err) } or use require.NoError(t, err)). Update the test setup
in backup_test.go where backupRestoreCmd.Flags().Set is invoked to assert the
Set succeeded so stale Cobra flag state cannot silently persist.
In `@cli/cmd/backup.go`:
- Around line 428-431: The os.Stat call for composePath currently only checks
for os.ErrNotExist and silently continues on other errors; change the logic in
the block around os.Stat(composePath) so that if os.Stat returns an error that
is NOT os.ErrNotExist you propagate or return that error (instead of falling
through to docker.Detect()), while preserving the existing out.Hint("Run
'synthorg start'...") and return nil behavior for the os.ErrNotExist case;
update the code that references composePath, os.Stat, errors.Is, os.ErrNotExist,
out.Hint and the subsequent docker.Detect() call to ensure permission/I/O errors
are handled explicitly (returning the error) before proceeding.
- Around line 344-349: Handle the GetBool error and make missing --confirm a
hard failure: check the error returned by cmd.Flags().GetBool("confirm") instead
of discarding it, return that error (or wrap it) if non-nil, and when confirm is
false call out.Error/out.Hint as before but return a non-nil error (not nil) to
cause a non-zero exit; update the test TestBackupRestore_MissingConfirm to
expect a non-nil error. Ensure you reference the existing cmd.Flags().GetBool,
out.Error, out.Hint, and TestBackupRestore_MissingConfirm symbols when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5f747ab-e1e1-40b2-8681-3221b683106a
📒 Files selected for processing (2)
cli/cmd/backup.gocli/cmd/backup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/{*.py,*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Handle errors explicitly, never silently swallow them
Files:
cli/cmd/backup.gocli/cmd/backup_test.go
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Go CLI: entry point atcli/main.go, Cobra commands incli/cmd/, internal modules incli/internal/
Go linting: golangci-lint + go vet
Files:
cli/cmd/backup.gocli/cmd/backup_test.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go CLI commands with root flags:
--data-dir,--skip-verify; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Files:
cli/cmd/backup.gocli/cmd/backup_test.go
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go testing: native
testing.Ffuzz functions for property-based testing; fuzz targets run seed corpus only without-fuzzflag
Files:
cli/cmd/backup_test.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/cmd/**/*.go : Go CLI commands with root flags: `--data-dir`, `--skip-verify`; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Applied to files:
cli/cmd/backup.gocli/cmd/backup_test.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/**/*.go : Go CLI: entry point at `cli/main.go`, Cobra commands in `cli/cmd/`, internal modules in `cli/internal/`
Applied to files:
cli/cmd/backup.gocli/cmd/backup_test.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/backup.gocli/cmd/backup_test.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/go.mod : CLI dependencies in `cli/go.mod`: Go 1.26+, Cobra, charmbracelet/huh, charmbracelet/lipgloss, sigstore-go, go-containerregistry, go-tuf
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to **/{*.py,*.go} : Handle errors explicitly, never silently swallow them
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.199Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.199Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on `cli/**/*.go`); fast gate before push, skipped in pre-commit.ci
Applied to files:
cli/cmd/backup_test.go
🧬 Code graph analysis (2)
cli/cmd/backup.go (4)
cli/internal/version/version.go (1)
Version(9-9)cli/internal/ui/ui.go (2)
UI(44-53)NewUI(58-70)cli/internal/config/state.go (1)
Load(53-91)cli/internal/docker/client.go (1)
Detect(34-66)
cli/cmd/backup_test.go (1)
cli/cmd/root.go (1)
Execute(101-107)
🪛 GitHub Check: CodeQL
cli/cmd/backup.go
[failure] 428-428: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (2)
cli/cmd/backup_test.go (1)
21-204: Comprehensive backup command coverage looks strong.The helper/unit tests plus integration-style create/list/restore scenarios cover the key success and failure paths well.
Also applies to: 228-578
cli/cmd/backup.go (1)
268-278: Good improvement: create/list now fail with non-zero on backend/API errors.Returning non-nil errors after printing user-facing messages here is the right behavior for scripting safety.
Also applies to: 306-316
cli/cmd/backup.go
Outdated
| confirm, _ := cmd.Flags().GetBool("confirm") | ||
| if !confirm { | ||
| out.Error("Restore requires the --confirm flag as a safety gate") | ||
| out.Hint(fmt.Sprintf("Run 'synthorg backup restore %s --confirm' to proceed", backupID)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the backup.go file and read the specific lines
fd backup.go cli/cmd/ | head -5Repository: Aureliolo/synthorg
Length of output: 79
🏁 Script executed:
# Read the specific lines to verify the code snippet
sed -n '340,355p' cli/cmd/backup.goRepository: Aureliolo/synthorg
Length of output: 486
🏁 Script executed:
# Look for the test that verifies missing confirm behavior
rg "TestBackupRestore_MissingConfirm" cli/ -A 15Repository: Aureliolo/synthorg
Length of output: 1013
🏁 Script executed:
# Find the restore command handler function
rg "func.*[Rr]estore.*cmd" cli/cmd/backup.go -A 2Repository: Aureliolo/synthorg
Length of output: 453
🏁 Script executed:
# Check the full context around the restore function to ensure no other mitigating factors
sed -n '320,360p' cli/cmd/backup.goRepository: Aureliolo/synthorg
Length of output: 1094
Make missing --confirm a hard failure and handle flag-read errors.
Line 344 discards GetBool errors with _, and line 348 returns nil when the required safety flag is absent. This causes the command to exit with code 0 (success) despite blocking the restore operation, violating the coding guideline that errors must be handled explicitly.
Suggested fix
- confirm, _ := cmd.Flags().GetBool("confirm")
+ confirm, err := cmd.Flags().GetBool("confirm")
+ if err != nil {
+ return fmt.Errorf("reading --confirm flag: %w", err)
+ }
if !confirm {
out.Error("Restore requires the --confirm flag as a safety gate")
out.Hint(fmt.Sprintf("Run 'synthorg backup restore %s --confirm' to proceed", backupID))
- return nil
+ return errors.New("restore aborted: missing required --confirm flag")
}Update TestBackupRestore_MissingConfirm to expect a non-nil error instead of nil.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| confirm, _ := cmd.Flags().GetBool("confirm") | |
| if !confirm { | |
| out.Error("Restore requires the --confirm flag as a safety gate") | |
| out.Hint(fmt.Sprintf("Run 'synthorg backup restore %s --confirm' to proceed", backupID)) | |
| return nil | |
| } | |
| confirm, err := cmd.Flags().GetBool("confirm") | |
| if err != nil { | |
| return fmt.Errorf("reading --confirm flag: %w", err) | |
| } | |
| if !confirm { | |
| out.Error("Restore requires the --confirm flag as a safety gate") | |
| out.Hint(fmt.Sprintf("Run 'synthorg backup restore %s --confirm' to proceed", backupID)) | |
| return errors.New("restore aborted: missing required --confirm flag") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/backup.go` around lines 344 - 349, Handle the GetBool error and make
missing --confirm a hard failure: check the error returned by
cmd.Flags().GetBool("confirm") instead of discarding it, return that error (or
wrap it) if non-nil, and when confirm is false call out.Error/out.Hint as before
but return a non-nil error (not nil) to cause a non-zero exit; update the test
TestBackupRestore_MissingConfirm to expect a non-nil error. Ensure you reference
the existing cmd.Flags().GetBool, out.Error, out.Hint, and
TestBackupRestore_MissingConfirm symbols when making the change.
- Route error/warning output to stderr via dual UI pattern (out + errOut) - Return non-zero exit code when --confirm flag is missing - Handle GetBool error explicitly instead of discarding with _ - Add JWT auth header (HS256) for admin endpoint authentication - Add styled errOut.Error() before json.Unmarshal error returns - Sanitize server-originated error strings (strip ANSI escape sequences) - Extract writeConfigJSON helper to deduplicate test config logic - Use package-level HTTP client with context.WithTimeout - Use strconv.Itoa instead of fmt.Sprintf for int conversion - Restrict backupAPIRequest path parameter to known values - Add NOTE comment about Cobra flag reset pattern in test helper - Use port 0 in no-network test (conventional for absent server) - Use API error message for 404 when more specific than fallback Sources: go-reviewer, security-reviewer, go-conventions-enforcer, CodeRabbit, Gemini Code Assist Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Handle flag reset error with t.Fatalf instead of discarding with _ - Handle non-ErrNotExist os.Stat errors in handleRestartAfterRestore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/backup_test.go`:
- Around line 344-355: The test TestBackupCreate_Unreachable hardcodes port
19999 which is inconsistent with other tests; update the call to writeTestConfig
in TestBackupCreate_Unreachable to use port 0 (like
TestBackupRestore_MissingConfirm) so the test reliably targets an unreachable
backend, leaving the rest of the test logic (runBackupCmd and the error
assertions) unchanged; locate TestBackupCreate_Unreachable and change the port
argument passed to writeTestConfig from 19999 to 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 079aeb6f-02db-4fc4-80a6-493d57528d4d
📒 Files selected for processing (2)
cli/cmd/backup.gocli/cmd/backup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CLI Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (4)
**/{*.py,*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Handle errors explicitly, never silently swallow them
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: Go CLI: entry point atcli/main.go, Cobra commands incli/cmd/, internal modules incli/internal/
Go linting: golangci-lint + go vet
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
cli/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go testing: native
testing.Ffuzz functions for property-based testing; fuzz targets run seed corpus only without-fuzzflag
Files:
cli/cmd/backup_test.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go CLI commands with root flags:
--data-dir,--skip-verify; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Files:
cli/cmd/backup_test.gocli/cmd/backup.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/cmd/**/*.go : Go CLI commands with root flags: `--data-dir`, `--skip-verify`; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/**/*.go : Go CLI: entry point at `cli/main.go`, Cobra commands in `cli/cmd/`, internal modules in `cli/internal/`
Applied to files:
cli/cmd/backup_test.gocli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.199Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.199Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on `cli/**/*.go`); fast gate before push, skipped in pre-commit.ci
Applied to files:
cli/cmd/backup_test.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to cli/** : CLI: Go 1.26+, dependencies in cli/go.mod (Cobra, charmbracelet/huh).
Applied to files:
cli/cmd/backup_test.gocli/cmd/backup.go
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Pre-push hooks: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on cli/**/*.go) (fast gate before push, skipped in pre-commit.ci — dedicated CI jobs already run these).
Applied to files:
cli/cmd/backup_test.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/cmd/**/*.go : Go CLI commands with root flags: `--data-dir`, `--skip-verify`; Cobra command structure with subcommands (init, start, stop, status, logs, doctor, update, uninstall, version, config, completion-install)
Applied to files:
cli/cmd/backup_test.gocli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/**/*_test.go : Go testing: native `testing.F` fuzz functions for property-based testing; fuzz targets run seed corpus only without `-fuzz` flag
Applied to files:
cli/cmd/backup_test.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/backup/**/*.py : Backup package (backup/): scheduled/manual/lifecycle backups of persistence DB, agent memory, company config. BackupService orchestrator, BackupScheduler (periodic asyncio task), RetentionManager (count + age pruning), tar.gz compression, SHA-256 checksums, manifest tracking, validated restore with atomic rollback and safety backup. handlers/ subpackage: ComponentHandler protocol + concrete handlers (PersistenceComponentHandler, MemoryComponentHandler, ConfigComponentHandler)
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to cli/go.mod : CLI dependencies in `cli/go.mod`: Go 1.26+, Cobra, charmbracelet/huh, charmbracelet/lipgloss, sigstore-go, go-containerregistry, go-tuf
Applied to files:
cli/cmd/backup.go
📚 Learning: 2026-03-18T21:35:45.198Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:35:45.198Z
Learning: Applies to **/{*.py,*.go} : Handle errors explicitly, never silently swallow them
Applied to files:
cli/cmd/backup.go
🧬 Code graph analysis (1)
cli/cmd/backup.go (3)
cli/internal/ui/ui.go (2)
UI(44-53)NewUI(58-70)cli/internal/config/state.go (1)
Load(53-91)cli/internal/docker/client.go (1)
Detect(34-66)
🪛 GitHub Check: CodeQL
cli/cmd/backup.go
[failure] 482-482: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (14)
cli/cmd/backup_test.go (4)
1-45: LGTM!The imports are appropriate, and the
TestFormatSizeunit test provides good coverage of edge cases including boundary values at KB, MB, and GB thresholds.
47-188: LGTM!The unit tests for helper functions are comprehensive and follow Go testing best practices with table-driven tests. Good coverage of edge cases for ID validation, component formatting, API response parsing, ANSI sanitization, and JWT structure verification.
192-266: LGTM!Test helpers are well-structured with proper cleanup handling, error propagation, and documented workarounds for Cobra's global flag state. The flag reset error handling at lines 252-254 correctly addresses the past review feedback.
268-624: LGTM!The integration tests provide excellent coverage of the backup command flows including success paths, error handling (conflict, server error, not found, invalid manifest), client-side validation (invalid ID, missing --confirm), and the restart-required behavior. The mock server implementations correctly simulate the backend API responses.
cli/cmd/backup.go (10)
1-26: LGTM!Imports are well-organized and appropriate for the functionality. Standard library packages for crypto, encoding, HTTP, and file operations are correctly imported alongside the internal packages and Cobra.
30-75: LGTM!The Cobra command structure follows the existing CLI patterns with appropriate use, short, and long descriptions. The
--confirmflag on restore provides the required safety gate, and command registration ininit()is correctly structured.
79-121: LGTM!The API types are well-defined with appropriate JSON tags. Using
json.RawMessagefor the data field inapiEnvelopeprovides flexibility for different response payloads. The struct definitions mirror the Python models as documented.
125-183: LGTM!Helper functions are well-implemented:
- Compiled regex for backup ID validation is efficient
- Size formatting handles appropriate thresholds
- ANSI sanitization provides defense-in-depth against potentially malicious server responses
- JWT generation uses standard HS256 with appropriate short expiry (60s)
185-230: LGTM!The
backupAPIRequestfunction is well-designed with:
- Explicit path validation (lines 190-192) restricting to known paths
- Proper context timeout handling with deferred cancel
- 1MB response body limit for DoS protection
- Conditional JWT authentication header
- Clear error wrapping with context
232-289: LGTM!Response parsing and rendering helpers are well-implemented with proper error handling. The
parseAPIResponsefunction correctly handles the success/error envelope pattern, and the rendering functions produce user-friendly output.
293-377: LGTM!Both
runBackupCreateandrunBackupListfollow consistent patterns with explicit error handling:
- Config loading errors are propagated
- API errors return non-nil error for non-zero exit codes
- Response parsing errors are properly handled
- The empty backup list case correctly returns nil (it's not an error, just informational)
379-432: LGTM!The
runBackupRestoreimplementation correctly:
- Validates backup ID format before any API call (fail-fast)
- Handles
GetBoolerrors explicitly (lines 390-393)- Returns a non-nil error when
--confirmis missing (line 397)- Uses
safeStateDirfor path validation before filesystem operations- Delegates error and success handling to dedicated functions
434-475: LGTM!The
renderRestoreSuccessandhandleRestoreErrorfunctions are well-implemented:
handleRestoreErroralways returns non-nil error for proper exit codes (documented at line 460)- The 404 case provides a helpful hint to run
backup listrenderRestoreSuccesscorrectly delegates tohandleRestartAfterRestorewhen restart is required- All parse errors are properly propagated
477-508: CodeQL path traversal concern is properly addressed; no changes needed.The
safeDirparameter comes fromsafeStateDir(state)which validates paths throughconfig.SecurePath(), explicitly satisfying CodeQL's path-injection detection. The error handling foros.Statat lines 482–490 correctly distinguishes between missing files and other errors, providing appropriate user guidance in both cases.
| func TestBackupCreate_Unreachable(t *testing.T) { | ||
| // Use a port where nothing is listening. | ||
| dir := writeTestConfig(t, 19999) | ||
|
|
||
| _, err := runBackupCmd(t, dir) | ||
| if err == nil { | ||
| t.Fatal("expected error for unreachable backend") | ||
| } | ||
| if !strings.Contains(err.Error(), "backend unreachable") { | ||
| t.Errorf("error %q does not mention unreachable backend", err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using port 0 consistently for unreachable backend tests.
The TestBackupCreate_Unreachable test uses hardcoded port 19999, while TestBackupRestore_MissingConfirm uses port 0. Although port 19999 is unlikely to be in use, using port 0 would be more consistent and reliable for "no network" scenarios, since connecting to port 0 will also fail.
🔧 Suggested change
func TestBackupCreate_Unreachable(t *testing.T) {
// Use a port where nothing is listening.
- dir := writeTestConfig(t, 19999)
+ dir := writeTestConfig(t, 0)
_, err := runBackupCmd(t, dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestBackupCreate_Unreachable(t *testing.T) { | |
| // Use a port where nothing is listening. | |
| dir := writeTestConfig(t, 19999) | |
| _, err := runBackupCmd(t, dir) | |
| if err == nil { | |
| t.Fatal("expected error for unreachable backend") | |
| } | |
| if !strings.Contains(err.Error(), "backend unreachable") { | |
| t.Errorf("error %q does not mention unreachable backend", err.Error()) | |
| } | |
| } | |
| func TestBackupCreate_Unreachable(t *testing.T) { | |
| // Use a port where nothing is listening. | |
| dir := writeTestConfig(t, 0) | |
| _, err := runBackupCmd(t, dir) | |
| if err == nil { | |
| t.Fatal("expected error for unreachable backend") | |
| } | |
| if !strings.Contains(err.Error(), "backend unreachable") { | |
| t.Errorf("error %q does not mention unreachable backend", err.Error()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/backup_test.go` around lines 344 - 355, The test
TestBackupCreate_Unreachable hardcodes port 19999 which is inconsistent with
other tests; update the call to writeTestConfig in TestBackupCreate_Unreachable
to use port 0 (like TestBackupRestore_MissingConfirm) so the test reliably
targets an unreachable backend, leaving the rest of the test logic (runBackupCmd
and the error assertions) unchanged; locate TestBackupCreate_Unreachable and
change the port argument passed to writeTestConfig from 19999 to 0.
🤖 I have created a release *beep* *boop* --- ## [0.3.6](v0.3.5...v0.3.6) (2026-03-19) ### Features * **cli:** add backup subcommands (backup, backup list, backup restore) ([#568](#568)) ([4c06b1d](4c06b1d)) * **engine:** implement execution loop auto-selection based on task complexity ([#567](#567)) ([5bfc2c6](5bfc2c6)) ### Bug Fixes * activate structured logging pipeline -- wire 8-sink system, integrate Uvicorn, suppress spam ([#572](#572)) ([9b6bf33](9b6bf33)) * **cli:** bump grpc-go v1.79.3 -- CVE-2026-33186 auth bypass ([#574](#574)) ([f0171c9](f0171c9)) * resolve OpenAPI schema validation warnings for union/optional fields ([#558](#558)) ([5d96b2b](5d96b2b)) ### CI/CD * bump codecov/codecov-action from 5.5.2 to 5.5.3 in the minor-and-patch group ([#571](#571)) ([267f685](267f685)) * ignore chainguard/python in Dependabot docker updates ([#575](#575)) ([1935eaa](1935eaa)) ### Maintenance * bump the major group across 1 directory with 2 updates ([#570](#570)) ([b98f82c](b98f82c)) * bump the minor-and-patch group across 2 directories with 4 updates ([#569](#569)) ([3295168](3295168)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
/api/v1/admin/backups)synthorg backup(andbackup create): trigger manual backup, display manifest with key-value pairssynthorg backup list: show available backups in a formatted lipgloss table (ID, timestamp, trigger, components, size, compressed)synthorg backup restore <id> --confirm: restore from backup with safety gate, auto-stop containers if restart requiredTest plan
go vet ./...cleangolangci-lint run0 issuesgo build ./...succeedsReview coverage
Pre-reviewed by 5 agents (go-reviewer, security-reviewer, go-conventions-enforcer, docs-consistency, issue-resolution-verifier). 17 findings addressed -- extracted helpers to meet 50-line function limit, used url.JoinPath for URL construction, removed dead assignments, added restart_required test path, updated 4 documentation files.
Closes #542
🤖 Generated with Claude Code