fix: retry atomic snapshot pushes#15
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 19, 2026, 7:57 AM ET / 11:57 UTC. Summary Reproducibility: no. not from a real current-main run in this read-only review. The source path and upstream CrawlKit tests make the rejected atomic push retry path understandable, but the PR still needs real or locally reproduced backup push evidence. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the narrow call-site and CrawlKit bump after removing the changelog hunk and adding redacted real-run proof for a rejected concurrent tagged backup push retry that preserves branch and tag state. Do we have a high-confidence way to reproduce the issue? No, not from a real current-main run in this read-only review. The source path and upstream CrawlKit tests make the rejected atomic push retry path understandable, but the PR still needs real or locally reproduced backup push evidence. Is this the best way to solve the issue? Mostly yes: delegating the retry to CrawlKit is the narrowest maintainable path because backup mirror synchronization already lives there. It is not merge-ready until the changelog hunk is removed and the contributor adds real behavior proof. Full review comments:
Overall correctness: patch is correct AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 4e6fb27ae15f. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a638e0859
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| if pushWithTag { | ||
| if err := mirror.PushAtomic(ctx, mirrorOptions(cfg), "HEAD", "refs/tags/"+tag); err != nil { | ||
| if err := mirror.PushCurrentSnapshot(ctx, mirrorOptions(cfg), tag); err != nil { |
There was a problem hiding this comment.
When a tagged push hits the new retry path, PushCurrentSnapshot receives mirrorOptions(cfg), including cfg.Remote. Unlike the earlier ensureRepo path, this bypasses syncOptions, so for an already-cloned backup repo whose origin intentionally differs from the config/default remote, the helper's sync step can reconfigure origin to cfg.Remote before retrying the push. In the non-fast-forward case this can make backup push --tag fail against, or publish metadata to, the wrong remote; pass the same remote-preserving options used by ensureRepo for existing repos.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Landed in Proof: full tests, race tests, vet, and autoreview passed locally; GitHub tests, lint, dependencies, Docker, release snapshot, CodeQL, and secret scanning passed. |
|
Post-merge audit: PASS — no defect found.
|
1 participant
Summary
Proof
go test ./...go test -race ./...go vet ./...