feat: add backfill-projects admin CLI command#1324
Conversation
geoffjay
left a comment
There was a problem hiding this comment.
Review: feat: add backfill-projects admin CLI command
Stack position: PR #1324 is stacked on epic-1310 (PR #1323, open). Review that parent first if you want the full picture of the migration; this diff is clean in isolation. Note also that the grandparent feature/projects-to-core shows (needs restack) in git-spice — once that resolves, the whole stack will need an upstack restack.
Blocking — error verb displayed in green (line 475 of admin.rs)
In output_backfill_projects_results, a single .green().bold() is applied to verb for all statuses:
println!(" {} {:<40} {}", icon, r.name.bright_black(), verb.green().bold());When r.status == ProjectBackfillStatus::Error, this prints the word "error" in green — the same colour as a successful insert. An operator scanning terminal output after a live migration will see green text everywhere and may not register the failure. The error detail one line below is red, but the per-row status verb should match. Fix by branching on status:
let colored_verb = match r.status {
ProjectBackfillStatus::Error => verb.red().bold(),
_ => verb.green().bold(),
};
println!(" {} {:<40} {}", icon, r.name.bright_black(), colored_verb);Non-blocking suggestions
Test count discrepancy. The PR description says "6 new tests" but the diff contains 5 #[tokio::test] functions for backfill_projects. The natural missing case is an insert that fails due to a name collision (a project with the same name but a different id already exists in core). The ProjectBackfillStatus::Error arm is otherwise untested. Worth adding:
#[tokio::test]
async fn backfill_projects_name_collision_reports_error() {
// Seed core with a project using the same name but different UUID already in core,
// then run backfill. Expect Error status, not a panic.
}Test helper uses string interpolation for SQL (lines 700–705). seed_projects constructs its INSERT via format! with inline values — would break silently on a name containing a single quote, and is inconsistent with the parameterised queries in production paths. Using hardcoded literal values with execute_unprepared is fine; avoid the format!-plus-variable-data pattern even in test helpers.
What is done well
- Clean separation of I/O (
backfill_projects) from logic (run_backfill_projects) — the inner function is easy to test and the boundary is clearly documented. - Idempotency via pre-loaded ID set is correct and efficient for migration volumes.
- The org-id filter correctly includes NULL rows (legacy data) — and that behaviour is verified by a dedicated test.
- Error propagation uses
anyhow::Contextthroughout; no bareunwrapon fallible production paths. - The empty-
--org-idguard fires before any database I/O — good fail-fast behaviour.
| ("❌", "error") | ||
| } | ||
| }; | ||
| println!(" {} {:<40} {}", icon, r.name.bright_black(), verb.green().bold()); |
There was a problem hiding this comment.
verb.green().bold() is applied to every status including Error. This prints the word "error" in green — the same colour as a success — which will mislead operators reading migration output. Branch on status so the error arm uses .red():
let colored_verb = match r.status {
ProjectBackfillStatus::Error => verb.red().bold(),
_ => verb.green().bold(),
};
println!(\" {} {:<40} {}\", icon, r.name.bright_black(), colored_verb);|
This change is part of the following stack: Change managed by git-spice. |
Adds the
agent admin backfill-projectscommand to copy project rows from orchestrator's SQLite database into core's SQLite database, preserving UUIDs.Implemented:
BackfillProjectsvariant added toAdminCommandenum with--dry-runand optional--org-idflagsrun_backfill_projectsfunction operates on open DB connections for testabilityprojectstable (with optional org-id filter including NULL rows)projectstable preserving original UUIDs; skips existing rows by id (idempotent)--jsonoutput as array of{id, name, status, error?}would_insertstatus without writingCloses #1310