Skip to content

Track failed sync hydration attempts#90

Open
TurboTheTurtle wants to merge 4 commits into
openclaw:mainfrom
TurboTheTurtle:track-sync-failures-84
Open

Track failed sync hydration attempts#90
TurboTheTurtle wants to merge 4 commits into
openclaw:mainfrom
TurboTheTurtle:track-sync-failures-84

Conversation

@TurboTheTurtle

@TurboTheTurtle TurboTheTurtle commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Closes #84.

Adds a sync-attempt failure ledger for PR hydration errors, exposes it through a read-only CLI command, marks failures resolved after a later successful hydration, and treats pre-ledger read-only databases as an empty ledger instead of failing on a missing table.

Behavior proof:

gitcrawl sync openclaw/gitcrawl --numbers 90 --with pr-details
list pull request review threads for #90: github POST http://127.0.0.1:<redacted>/graphql failed with status 503: {"message":"forced review-thread outage for proof"}
{"failures":[{"number":90,"operation":"pull_review_threads","error_class":"error","retry_count":0}],"include_resolved":false,"repository":"openclaw/gitcrawl"}
{"repository":"openclaw/gitcrawl","pull_requests_synced":1,"pr_details_synced":1,"pr_files_synced":1,"pr_commits_synced":1}
{"failures":[{"number":90,"operation":"pull_review_threads","resolved_at":"2026-07-01T19:41:33.14832Z"}],"include_resolved":true,"repository":"openclaw/gitcrawl"}

The proof run used the actual CLI against a throwaway local archive and a local GitHub-compatible endpoint that forced one review-thread hydration failure, then allowed the retry to succeed.

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 11:37 AM ET / 15:37 UTC.

Summary
The branch adds a sync_attempt_failures SQLite ledger, records and resolves PR-detail hydration failures, exposes a read-only sync-failures CLI command, and includes unresolved failure counts in coverage JSON.

Reproducibility: not applicable. as a feature PR: the linked issue requests a new durable ledger rather than reporting broken existing behavior. The PR body still provides high-confidence live CLI proof using a forced hydration failure followed by a successful retry.

Review metrics: 3 noteworthy metrics.

  • Persistent schema: schemaVersion 4 -> 5; 1 table added. The one-way SQLite version bump is the main compatibility decision before merge.
  • PR surface: 14 files changed, +755/-15. The patch spans store schema, syncer persistence, CLI output, and focused tests.
  • User command surface: 1 command added; coverage JSON extended. The new read-only command and JSON fields become user-facing output contracts.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #84
Summary: This PR is the candidate implementation for the open failed sync/hydration attempt ledger issue it closes.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Maintainer should approve, tweak, or defer the v5 SQLite ledger and JSON output contract before merge.

Risk before merge

  • [P1] Merging bumps the durable SQLite schema from v4 to v5; once a write-path migration occurs, older v0.7.0-era binaries can reject the upgraded database as newer than supported.
  • [P1] The sync-failures command and coverage JSON fields become user-facing agent contracts, so field names and retry/resolved semantics should be intentionally accepted before release.

Maintainer options:

  1. Approve the v5 ledger and merge (recommended)
    Maintainers can accept the one-way schema bump and JSON surface as the intended contract once checks remain green.
  2. Require an upgrade-contract tweak
    Ask for the exact compatibility guard, docs note, or proof needed before allowing the schema change to ship.
  3. Defer the storage change
    Pause or close this PR if the durable ledger is not worth a new SQLite schema version yet.

Next step before merge

  • [P2] No automation repair is indicated; maintainer signoff is needed for the v5 SQLite schema and CLI JSON contract before merge.

Maintainer decision needed

  • Question: Should Gitcrawl accept the v5 sync_attempt_failures SQLite ledger and the new sync-failures/coverage JSON contract as the supported direction for the linked failure-ledger request?
  • Rationale: The implementation appears narrow and correct, but durable schema versions and CLI JSON fields are compatibility and product commitments that automation should not choose alone.
  • Likely owner: Peter Steinberger — Peter is the strongest current-history routing candidate for the schema baseline, sync hydration path, and release compatibility choice.
  • Options:
    • Approve the v5 ledger contract (recommended): Accept the schemaVersion 5 migration plus the new read-only CLI and coverage JSON fields, then merge after normal checks remain green.
    • Request a contract adjustment: Ask for the specific compatibility guard, documentation note, or JSON shape change maintainers need before the schema ships.
    • Defer durable storage: Pause or close this PR if the project is not ready to add a persistent failure-ledger table and command surface right now.

Security
Cleared: The diff adds local SQLite persistence and read-only CLI reporting without changing dependencies, CI, secrets, auth, permissions, or remote code execution paths.

Review details

Best possible solution:

Land the ledger after explicit maintainer approval of the v5 SQLite schema and read-only CLI/coverage JSON contract, preserving the pre-ledger read-only compatibility behavior already in the patch.

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

Not applicable as a feature PR: the linked issue requests a new durable ledger rather than reporting broken existing behavior. The PR body still provides high-confidence live CLI proof using a forced hydration failure followed by a successful retry.

Is this the best way to solve the issue?

Unclear until maintainer approval: the implementation is a narrow fit for the requested ledger, but the permanent schema and CLI JSON contract need owner signoff before merge.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 59cffd97b6b3.

Label changes

Label justifications:

  • P2: This is a normal-priority archive observability improvement with limited runtime blast radius but meaningful maintainer workflow value.
  • merge-risk: 🚨 compatibility: The PR changes the durable SQLite schema version and can make upgraded archives unreadable by older binaries.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-change live CLI output from a real gitcrawl run against a throwaway archive and forced local GitHub-compatible failure, followed by unresolved and resolved ledger output with private details redacted.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-change live CLI output from a real gitcrawl run against a throwaway archive and forced local GitHub-compatible failure, followed by unresolved and resolved ledger output with private details redacted.
Evidence reviewed

What I checked:

  • Current main does not implement the ledger: Current main remains at schemaVersion 4 and has no sync_attempt_failures table or sync-failures command; only placeholder coverage failure fields exist. (internal/store/store.go:16, 59cffd97b6b3)
  • PR adds the durable schema: The PR head adds sync_attempt_failures with repo, thread, number, operation, error class/message, timestamps, retry count, and resolved timestamp fields. (internal/store/schema.go:404, 055cbf7ef546)
  • PR implements record, resolve, and read-only list behavior: RecordSyncAttemptFailure upserts retry rows, ResolveSyncAttemptFailures marks active failures resolved, and ListSyncAttemptFailures treats pre-ledger read-only stores as an empty ledger. (internal/store/sync_failures.go:32, 055cbf7ef546)
  • PR wires hydration failures into sync: PR review-thread and pull-detail fetch errors are recorded before returning the original sync error, and later successful PR-detail persistence resolves outstanding rows for that PR number. (internal/syncer/syncer.go:158, 055cbf7ef546)
  • Coverage and CLI surface are covered: ArchiveCoverage counts unresolved sync_attempt_failures when the table exists, and the PR adds CLI coverage for unresolved/history sync-failures output. (internal/store/archive_coverage.go:84, 055cbf7ef546)
  • Linked issue remains the canonical work: Track failed sync and hydration attempts #84 requests queryable failed sync/hydration attempts, coverage exposure of known failures, resolved retry state, and JSON detail; this PR is its open closing candidate.

Likely related people:

  • Peter Steinberger: Current-main blame points the schemaVersion 4 release baseline and PR-detail sync error path to Peter, and prior history includes sync hydration atomicity work on internal/syncer/syncer.go. (role: recent area contributor and schema/sync routing candidate; confidence: high; commits: 0bc269b078e3, 5232d51cdde2, a4e08f49b640; files: internal/store/store.go, internal/store/schema.go, internal/syncer/syncer.go)
  • Vincent Koc: Visible repository history includes adjacent CLI/archive and diagnostic work touching the command/runtime surface this PR extends. (role: adjacent area contributor; confidence: medium; commits: e5546915e491, 0908f8b85337; files: internal/cli/app.go)
  • Josh McKinney: Josh authored the canonical failure-ledger request and has adjacent merged store/schema history around PR detail storage compatibility. (role: adjacent contributor and workflow reporter; confidence: medium; commits: dca4f0d7b6df; files: internal/store/schema.go, internal/store/store.go, internal/store/pull_requests.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (5 earlier review cycles)
  • reviewed 2026-07-01T19:51:59.982Z sha 62069ed :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T12:11:04.399Z sha 62069ed :: found issues before merge. :: [P2] Wire the ledger into coverage reporting
  • reviewed 2026-07-06T18:35:34.972Z sha 6743734 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T16:39:26.529Z sha 6743734 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T18:15:04.462Z sha 055cbf7 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 1, 2026
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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 Jul 6, 2026
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Merged main to clear the conflict and wired the new failure ledger into coverage so unresolved hydration failures are counted in the coverage JSON totals. Focused store/CLI coverage and sync-failure tests pass locally.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 6, 2026
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. proof: sufficient Contributor real behavior proof is sufficient. 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.

Track failed sync and hydration attempts

1 participant