Add PowerForge server recovery CLI#341
Conversation
Code Review — PR #341: Add PowerForge server recovery CLIThis is a substantial addition (~2,500 lines). The overall architecture is clear and the design doc is well-written. Below is feedback organized by severity. Critical / Bugs1. Process I/O deadlock in 2. 3. Stale error message in 4. Logic inversion in if (!manifest.BackupTarget?.Encryption?.Equals("age", ...) == true)When if (manifest.BackupTarget?.Encryption is not null &&
!manifest.BackupTarget.Encryption.Equals("age", StringComparison.OrdinalIgnoreCase))5. Hardcoded Evotec-specific symlink paths in 6. Schema / model mismatches
Security7. No validation of 8. 9. Schema Quality10. 11. Missing constraints
Code Quality12. No error handling in 13. No SSH connection timeout in 14. 15. One SSH connection per inspect check 16. 17. Cloudflare detection via 18. Unrelated changes bundled in the PR SummaryThe feature is well thought-out and the design document is excellent. The main items to address before merging are: the process I/O deadlock (item 1), the Evotec-specific content in the engine (items 5, 16), the logic inversion bug (item 4), the schema/model mismatches (item 6), and wrapping manifest loading in a try/catch (item 12). The security note about manifest trust (item 7) and the |
Code Review — PR #341: Add PowerForge server recovery CLIThis is a substantial addition (~2,540 lines across 9 new files). The architecture is well-structured and the security intent is clearly thought-through. Comments below are ordered by severity. Critical / Correctness1.
Fix: change the default to 2. Unhandled exceptions from Both throw Fix: wrap the main handler body (or the individual calls) in a 3. Capture command failures are collected as warnings but do not affect the exit code. Unlike Fix: add a Schema / Model Consistency4.
Fix: make 5. The dual-source fallback ( Fix: add a computed helper, e.g. Error Handling6. No SSH command timeout beyond
Fix: propagate a timeout (perhaps via 7. This surfaces Fix: either Code Quality8. The dispatcher maps Fix: either document the alias explicitly, or remove it and add a dedicated 9. Standard socket exhaustion anti-pattern. For a small CLI tool with a handful of URLs this is unlikely to cause issues, but it is worth noting. Consider passing an 10.
SecurityOverall the security posture is good. Specific positive notes:
One minor concern: Documentation
Summary
The most important fixes before merge are items 1–3 above. The rest are lower-priority quality improvements. |
Code Review — PR #341: Add PowerForge server recovery CLIOverall: This is a well-structured, security-conscious addition of a server recovery subsystem. The architecture is clean and the secret-handling design is thoughtful. Below is detailed feedback organized by area. What this PR doesAdds Strengths
Issues and suggestionsSecurity / correctness
Code quality
Schema
Documentation
Minor / nitpicks
SummaryThe implementation is solid and the security model is well thought-out. The main actionable items are:
The rest are low-priority clarity improvements. |
Code Review — PR #341: Add PowerForge server recovery CLIOverviewThis PR introduces a comprehensive What is Well Done
Security Concerns1. SSH host key verification is not enforced
2.
3. Glob characters allowed in capture paths
4. Hardcoded The mode for restored secret files is inferred from path string matching ( Bugs / Correctness5. No process execution timeout
6.
7.
8. The JSON schema defines Performance / Design9. A new 10. Duplicated output path resolution logic
11. Silent deduplication in
Test CoverageNo tests are added for this feature. The following helpers are high-value candidates for unit tests:
Minor Notes
SummaryThe feature is coherent, well-structured, and the security-sensitive paths (shell quoting, archive path traversal, temp dir cleanup) are handled carefully. Main actionable items before shipping:
Generated with Claude Code |
Summary
powerforge-web serverrecovery actions for plan, inspect, capture, deploy dry-run/run, verify, bootstrap-plan, and restore-secrets-planValidation
dotnet build C:\Support\GitHub\PSPublishModule\PowerForge.Web.Cli\PowerForge.Web.Cli.csproj -c Release --no-restoreTest-Json -Path C:\Support\GitHub\Website\deploy\linux\evotec.serverrecovery.json -SchemaFile C:\Support\GitHub\PSPublishModule\Schemas\powerforge.web.serverrecovery.schema.jsonPowerForge.Web.Cli.exe server plan --manifest C:\Support\GitHub\Website\deploy\linux\evotec.serverrecovery.json --output jsonPowerForge.Web.Cli.exe server deploy --manifest C:\Support\GitHub\Website\deploy\linux\evotec.serverrecovery.json --dry-run --fail-on-failure --output json