Skip to content

fix: fail account sync when backup fails - #127

Merged
steipete merged 3 commits into
mainfrom
fix/account-sync-backup-status
Aug 19, 2026
Merged

steipete merged 3 commits into
mainfrom
fix/account-sync-backup-status

Conversation

@steipete

@steipete steipete commented Aug 18, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • make scheduled account sync fail when its configured backup sync returns a failure
  • preserve newer numeric profile identity and handle ownership when backup sync merges a prior valid generation after live updates
  • use imported profile snapshots to let genuinely newer backup state update an older live database
  • safely adopt proven legacy selected-account profile_me rows into the canonical numeric profile after imported references exist
  • make related profile snapshot, bio-entity, and affiliation conflict merges timestamp-aware

Proof

  • 1,442 tests passed
  • format, lint, typecheck, client/SSR/CLI builds passed
  • stale-backup, newer-backup, and production-shaped legacy-profile regressions pass
  • contradictory and account-mismatch legacy aliases remain separate
  • Codex autoreview reported no actionable findings

Live discovery

Production rehearsals completed all six data steps and correctly exited 1 while surfacing two backup-import bugs: stale handle ownership and a legacy profile_me collision with the canonical numeric selected account. In both cases the scheduler stayed unloaded, no lock remained, and the clean backup checkout was unchanged. This PR fixes the status propagation and both import paths before scheduling is re-enabled.

@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 18, 2026
@clawsweeper

clawsweeper Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 18, 2026, 7:40 PM ET / 23:40 UTC.

ClawSweeper review

What this changes

The PR makes scheduled account sync report backup-export failures and adds timestamp-aware reconciliation for imported profile identities, snapshots, affiliations, and bio entities.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

No actionable patch defect was found. This owner-authored PR is cleanly mergeable with passing CI and targeted regression coverage; it remains open for routine maintainer merge review because it intentionally changes scheduled-job failure signaling and backup-import conflict behavior. Likely related people: steipete (feature owner and recent area contributor, high confidence).

Priority: P2
Reviewed head: 1353329ec9c2e472e2869767051b960d699453e6

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch is focused, CI is green, and the substantial compatibility-sensitive logic has targeted regression coverage.
Proof confidence 🌊 off-meta tidepool Not applicable: Not applicable: this is an OWNER-authored PR; its body also reports production rehearsals and passing validation, but the external-contributor proof gate does not apply.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Not applicable: this is an OWNER-authored PR; its body also reports production rehearsals and passing validation, but the external-contributor proof gate does not apply.
Evidence reviewed 5 items Current-main boundary: Current main is 1b94a3f, whose parent is this PR's base 15f831b; the intervening main commit does not touch the PR's account-sync or backup/profile-identity paths, so the central behavior is not already implemented on main.
Scheduled failure propagation: The account-sync audit result now includes backup.ok, so a configured backup failure reaches the CLI's nonzero result while a disabled backup remains successful.
Import reconciliation coverage: The merge path prepares profile rows using imported snapshot recency before inserting them, then finalizes proven legacy identity adoption after referenced rows are present; focused tests cover stale, newer, handle-handoff, and legacy selected-account cases.
Findings None None.
Security None None.

Live Verification

Command: bun --no-env-file src/cli.ts jobs sync-account --help

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: Unsupported package manager specification (bun@1.4.0-canary.1+f972c287f)

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: Unsupported package manager specification (bun@1.4.0-canary.1+f972c287f)

Assertions:

  • FAIL expect_output: Usage:

How this fits together

Scheduled account sync refreshes local X data and then runs configured backup synchronization, returning an audit result to launchd or other schedulers. Backup import merges portable JSONL profile data into SQLite, where profile identity and history rows feed search and downstream reads.

flowchart LR
A[Scheduled account sync] --> B[Live account refresh]
B --> C[Backup auto-sync]
C --> D[Job audit and exit status]
E[Backup JSONL profiles] --> F[Profile recency and identity checks]
F --> G[SQLite profile merge]
G --> H[Search and profile reads]
Loading

Before merge

  • Resolve merge risk (P1) - Existing scheduled account jobs with backup auto-sync will now exit nonzero on backup failure, intentionally changing launchd alert and retry behavior.
  • Resolve merge risk (P1) - Backup imports will resolve profile conflicts by recorded snapshot recency and legacy-identity evidence, which is compatibility-sensitive for existing SQLite data.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus regression coverage production +452, tests +916, docs +2 Most new code is accompanied by focused coverage for the backup-import and scheduler cases it changes.

Merge-risk options

Maintainer options:

  1. Accept the deliberate upgrade behavior (recommended)
    Merge with the understanding that configured backup failures now produce scheduler failures and imports resolve profile state by timestamped evidence.

Technical review

Best possible solution:

Keep backup failures visible to schedulers while preserving local profile state unless newer imported snapshot evidence and proven identity links justify an update.

Do we have a high-confidence way to reproduce the issue?

Yes—on the pre-PR path, account sync calculated success from sync steps alone despite a failed backup result; the focused job and CLI regressions exercise that path, and import tests cover the affected profile states.

Is this the best way to solve the issue?

Yes—the patch keeps the failure signal at the job boundary and uses existing snapshot history for import decisions rather than introducing a separate backup mode or data store.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 1b94a3fb8dc8.

Labels

Label justifications:

  • P2: The PR corrects scheduled backup-failure reporting and local profile import behavior with a bounded operator and data-merge impact.
  • merge-risk: 🚨 compatibility: The patch intentionally changes existing scheduler exit status and persisted-profile conflict resolution during backup imports.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Not applicable: this is an OWNER-authored PR; its body also reports production rehearsals and passing validation, but the external-contributor proof gate does not apply.

Evidence

What I checked:

  • Current-main boundary: Current main is 1b94a3f, whose parent is this PR's base 15f831b; the intervening main commit does not touch the PR's account-sync or backup/profile-identity paths, so the central behavior is not already implemented on main. (1b94a3fb8dc8)
  • Scheduled failure propagation: The account-sync audit result now includes backup.ok, so a configured backup failure reaches the CLI's nonzero result while a disabled backup remains successful. (src/lib/account-sync-job.ts:393, 1353329ec9c2)
  • Import reconciliation coverage: The merge path prepares profile rows using imported snapshot recency before inserting them, then finalizes proven legacy identity adoption after referenced rows are present; focused tests cover stale, newer, handle-handoff, and legacy selected-account cases. (src/lib/backup.ts:3283, 1353329ec9c2)
  • Feature history: The original scheduled backup call dates to the released v0.12.1 lineage, while the current PR head adds the propagation and reconciliation behavior; the same contributor has recent profile/backup hardening history. (src/lib/account-sync-job.ts:391, b676647c1c8e)
  • Repository policy: No target-repository AGENTS.md was present; no matching maintainer-note file was found for this surface. (1353329ec9c2)

Likely related people:

  • steipete: Current PR commits, the released scheduled-job lineage, and the recent profile/backup-hardening commit all attribute this area to steipete. (role: feature owner and recent area contributor; confidence: high; commits: f2531ea91c18, 131f6a1a7074, 1353329ec9c2; files: src/lib/account-sync-job.ts, src/lib/backup.ts, src/lib/profile-identity.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-18T20:29:33.906Z sha 5c6f3dd :: needs maintainer review before merge. :: none
  • reviewed 2026-08-18T22:44:03.526Z sha 4ef618b :: needs maintainer review before merge. :: none
  • reviewed 2026-08-18T22:54:23.099Z sha e33b509 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Aug 18, 2026
@steipete
steipete force-pushed the fix/account-sync-backup-status branch from 4ef618b to e33b509 Compare August 18, 2026 22:50
@steipete
steipete merged commit c0ab136 into main Aug 19, 2026
5 checks passed
@steipete
steipete deleted the fix/account-sync-backup-status branch August 19, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant