Sync checkpoints when creating a trail - #2239
Merged
Merged
Conversation
Entire-Checkpoint: 01M1GP428SCGQVC50PEVDGKXX9
Entire-Checkpoint: 01M1GPD05P10B7QHGNR4TACYFG
gtrrz-victor
previously approved these changes
Sep 2, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, aligns with the stated goal (checkpoint publication via pre-push), and includes a targeted regression test covering the new behavior.
Pull request overview
This PR ensures checkpoint data is published when creating a trail by making trail-branch pushes behave like a normal user git push (i.e., not bypassing Git’s pre-push hook, where Entire publishes captured checkpoints).
Changes:
- Stop bypassing
pre-pushhooks when pushing the newly-created trail branch to the remote. - Add an end-to-end test that verifies trail-branch creation triggers the
pre-pushhook and that the branch lands on the remote. - Add a defensive
returnaftert.Fatalin a contexts unit test.
File summaries
| File | Description |
|---|---|
| internal/entireclient/contexts/contexts_test.go | Adds an explicit return after a fatal assertion for clarity/defensiveness in the missing-file test. |
| cmd/entire/cli/trail_cmd.go | Updates trail branch push behavior to run pre-push hooks (enabling checkpoint publication during trail creation). |
| cmd/entire/cli/trail_cmd_test.go | Adds a regression test that verifies prepareTrailCreateBranch does not bypass pre-push hooks. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Dropping --no-verify made the pre-push hook run, but the push was still an internal subprocess: CombinedOutput() with nil stdin and a two-minute budget. Two consequences, both of which defeat the point of running the hook at all. Entire's hook reports on stderr both when it pushes checkpoint refs and when it withholds them, and on the git-refs backend it withholds them while still exiting zero. With that output read only on failure, trail create could print "Pushed branch" for a push that published nothing — the failure this path exists to prevent, with its only diagnostic discarded. The OPF prompt became unanswerable in practice rather than skipped. CanPromptInteractively() decides from a /dev/tty probe and never consults stdio, so it still said yes; bubbletea falls back to /dev/tty for prompt input when stdin is not a terminal, but has no such fallback for output, which is unconditionally os.Stdout. The form rendered into the captured buffer while reading real keystrokes, so the push blocked on a prompt nobody could see. stdout and stderr now stream to the caller's writers, and stdin is inherited so git can prompt for credentials as on a push the user typed. The returned error carries the exit status alone, since git's own message has already reached the user. The timeout goes to 10 minutes. It now covers the hook, and OPF is ~30s in the steady state and bounded only by history on the first push after a repo enables it; at two minutes exec would SIGKILL git mid-rewrite. A bound remains because trail create is multi-step and a wedged push must not hang it. Because the push is a precondition for creating the trail, a hook that says no now fails trail creation — Entire's own, or one the repo installed for its own reasons. The hint offered only auth and non-fast-forward, so it now names a rejected hook and points at the output above. A test covers that path: the branch lands on neither side, and the hook's reason reaches the caller. deleteBranchFromRemote keeps --no-verify, now with a comment saying why, so the asymmetry is not read as an oversight: it retracts a branch on a failed create, so there is nothing to publish, and running checkpoint sync while unwinding is work in the wrong direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1GWXCP0KVZTA6JQK1CVCZA8
t.Fatal calls runtime.Goexit(), so the added return is unreachable and guards nothing. It was not silencing a linter either — mise run lint is clean on main without it. Unrelated file, no effect. This reverts commit a9e95a1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1GWXM4DSR87AJJ4ECWHVCK2
gtrrz-victor
self-requested a review
September 2, 2026 11:24
Cleanup pass over the previous commit. No behaviour change. The timeout comment had the OPF cost model wrong. It claimed the hook's worst case was "bounded only by history" on a first run, which is not how OPF runs: both rewrite paths make exactly one shell-out per push, bounded by redaction.openai_privacy_filter.timeout_seconds (30s by default), and an oversized first run is rejected by BootstrapTooLargeError or OPFRawBytesTooLargeError rather than allowed to run long. The genuinely unbounded wait is the OPF prompt, which waits on a person — which is what ten minutes is actually sized for. The comment now says that, and records the limitation it implies: timeout_seconds is validated only as >= 0, so a value above this bound is truncated and git is killed mid-rewrite. Deriving the bound from the setting is the fix if anyone hits it. The doc comment's closing paragraph repeated what the inline comment at the return already said, so it is gone. In its place is the property both fixes actually rest on and which nothing else records: os/exec passes an *os.File's descriptor straight to the child, so the hook inherits the terminal, while any other io.Writer gets an os.Pipe and the child's stdout stops being a tty. Cobra's OutOrStdout/ErrOrStderr fall through to os.Stdout/os.Stderr and nothing on the path to trail create calls SetOut/SetErr, so a pager or output tee on the root command would silently undo this. The two hook scripts move to backtick raw strings, which is what the rest of the repo's tests use for shell scripts with embedded quotes (setup_test.go, explain_test.go, session_adopt_test.go, external_test.go). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1GZ5D8XKQHN62E0KBS3H8D5
dipree
enabled auto-merge
September 7, 2026 12:18
Soph
previously approved these changes
Sep 7, 2026
The known-limitation note on trailBranchPushTimeout claimed expiry kills git mid-rewrite. It kills git, but not the rewrite: CommandContext's default Cancel is Process.Kill() on the child PID, no Setpgid is set, and the OPF re-redaction and ref CAS-updates run in the pre-push hook — a grandchild. The hook is orphaned, not interrupted, so the StatusWalkBudget failure mode (SIGKILL on the process holding the lock) does not transfer here. Record the orphan as the cost that is actually being accepted, and why killing the group would be worse. Second note: the *os.File requirement is load-bearing for more than the tty. With a pipe interposed, Cmd.Wait blocks on its copy goroutine until every holder of the write end closes, an orphaned hook holds one, and WaitDelay is unset — so a pager on the root command would leave this call unbounded, which is the one thing the timeout exists to prevent. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses a finding on the trail. The justification the comment gave — git prompting for credentials as it would on a push the user typed — does not hold. git push has no --stdin mode; git's own credential prompt uses /dev/tty or askpass; the OPF prompt reads /dev/tty via bubbletea's fallback; and the hook never receives our stdin anyway, since git hands it a pipe carrying the ref list. So the only effect was to replace an immediate EOF with a real blocking read. A non-interactive trail create — an agent whose stdin is a long-lived pipe rather than a closed fd — could park on a prompt until trailBranchPushTimeout instead of failing fast. Left at /dev/null, which is where it was before the streaming change. Deliberately does not touch cancellation: the orphaned-hook cost is recorded as accepted in trailBranchPushTimeout's note, and killing the process group would reverse that decision, not refine it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1Y7QH20GVSQGG0CRGYC1DJ0
gtrrz-victor
approved these changes
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/1219
Ensure the branch push performed by entire trail create runs the checkpoint synchronization path, so already-captured sessions are published with the new trail branch.