feat(sessions): reassociate commits with their sessions after a rebase - #901
Open
matt2e wants to merge 8 commits into
Open
feat(sessions): reassociate commits with their sessions after a rebase#901matt2e wants to merge 8 commits into
matt2e wants to merge 8 commits into
Conversation
![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: dba9dad58c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tertiaryMeta: showAuthor ? commit.author : undefined, | ||
| deleting: isDeleting, | ||
| timestamp: commit.timestamp, | ||
| timestamp: commit.sortTimestamp, |
There was a problem hiding this comment.
For users who already have a branch timeline cached from before this response-shape change, cached commits do not contain sortTimestamp, and CACHE_SCHEMA_VERSION is still unchanged. getBranchTimelineWithRevalidation can render that cached value immediately and may skip the network request while the entry is fresh, so using commit.sortTimestamp here produces undefined sort keys; comparisons against notes/reviews become NaN, which can leave commits in the wrong position and break review anchoring until the cache expires. Defaulting to commit.sortTimestamp ?? commit.timestamp or invalidating/normalizing the cached schema avoids the regression.
Useful? React with 👍 / 👎.
A rebase gives every commit on the branch a new SHA, which orphaned each `commits` row keyed by the old one: the timeline lost the authoring session, the head commit got mis-attributed to the mechanical "Rebase branch" session, and reviews keyed by `commit_sha` disappeared behind `review_is_visible_in_timeline`. The mapping is recoverable from the DB plus git alone, with no pre-rebase capture — `git rebase` preserves author email, author date, and subject, and the old objects stay resolvable — so the reconciler also survives an app restart mid-pipeline. New `commit_reassociation` module reads the old identities back from the orphaned SHAs, pairs them against the rewritten commits (oldest-to-oldest when identities collide, skipping SHAs a row already owns), and `Store::remap_commit_shas` repoints the rows and their reviews in one transaction. Both rebase completion paths call it before the pending row is resolved, so the existing "target SHA already owned" branch drops the duplicate pending row instead of stealing the head commit. Covered by matcher unit tests, store tests for the remap and its collision guard, and integration tests driving a real `git rebase --signoff` through each completion path. Signed-off-by: Matt Toohey <[email protected]>
Reassociating commits with their sessions after a rebase (f5103d9) restored each commit's ownership but not its place in the timeline. The branch timeline is one list sorted by timestamp, and the two clocks it merges diverge under a rebase: notes, reviews, and images carry DB times, while commits carried git's committer date, which a rebase rewrites to "now". So a timeline that read commit A -> plan note -> commit B came back as plan note -> A' -> B', with every commit sunk below every note. Author date is the rebase-stable key the reassociation matcher already trusts, so extend it from ownership to position: `git log` now carries both clocks (`BRANCH_COMMIT_LOG_FORMAT`, with a six-field fallback so a stray old-format producer still parses), and `CommitTimelineItem` takes its timestamp from `%at`. Every item on the timeline now sorts by a key a rebase never touches. `CommitInfo.timestamp` stays committer time for `latest_git_commit_ms`, where a rebase *should* read as new activity. Session transcripts get the same `%ct` -> `%at` switch on both the local and remote paths, where the prefix is purely an ordering key. Author dates aren't monotonic along a branch the way committer dates are — a cherry-pick keeps its week-old date — so `build_branch_timeline` clamps them with a running max in branch order, keeping the rendered order matching `git log`. Repo-browse listings and parent-branch commits stay on committer time; they don't interleave with notes. No schema or frontend change. Covered by parse tests for both field counts, clamp unit tests, and an integration test driving a real `git rebase --signoff` over a commit/note/commit branch and asserting the interleaving survives. Signed-off-by: Matt Toohey <[email protected]>
…-stable sort Sorting commits by author date (aa8220f) kept notes interleaved across a rebase, but review e4f7f39 found the ordering key leaking into places it shouldn't and the reassociation trusting a HEAD it shouldn't. `BRANCH_COMMIT_LOG_FORMAT` still kept `%s` mid-line, so a subject containing `|` shifted every field after it — a `%at` parse failure that the new clamp then masked by pulling the commit up to its predecessor's time. Subject moves last, as `commit_reassociation`'s format already had it, and one shared `parse_branch_commit_line` takes it as the remainder. That replaces four hand-rolled index parsers (branch commits, remote commits, parent-branch commits, repo-browse) and the six-field fallback: the last old-format producer, `BATCH_FAST_SCRIPT`, now emits the shared field list too, with a test tying the script to the const. The `%ct` -> `%at` switch had landed in `build_branch_context` without the monotonic clamp, so a cherry-picked commit whose author date predates its parent's could reorder the agent-facing section titled "Branch History (oldest first)" (`order` only breaks ties, so it can't recover an inverted timestamp). The clamp is now `timeline::clamp_timestamps_ monotonic`, shared by both timelines. Reassociation fired on any HEAD move for a rebase-pipeline session, with nothing checking that the rebase had finished. A turn that ends with conflicts unresolved leaves HEAD detached on a partially applied commit, and repointing every row plus its reviews there is worse than leaving them orphaned — `git rebase --abort` restores the originals and the rows would then name commits on no branch at all. `reassociate_after_rebase` now returns 0 unless HEAD is attached to the branch it was asked about, which is the same question as "is the rebase over?" since a rebase only moves the branch ref at the end. Finally the clamp overwrote the field the frontend renders, so a week-old cherry-pick displayed as its successor's time. `CommitTimeline Item` carries `sortTimestamp` for order and keeps `timestamp` truthful; the merge, the commit anchors, `BranchCard`'s latest-item check and the hashtag list sort on the former, the rendered date reads the latter. Adding the field is the reviewer's own suggestion, per AGENTS.md. Covered by parse tests for the new field order and a piped subject, a clamp test asserting the rendered dates stay put, a branch-history clamp test, and an integration test driving a real `git rebase` into a conflict and asserting the stopped rebase leaves every row and review untouched (it repoints them onto the throwaway SHAs without the guard). Signed-off-by: Matt Toohey <[email protected]>
…nside them Review d051cfc0 on dba9dad pointed out that "the only field that can contain the delimiter" wasn't airtight: subjects were the only field *expected* to contain `|`, but git also permits it in `user.name` (and technically in emails). An author configured as `Foo | Bar` shifted every field after it — `committer_timestamp` tried to parse the email (0 via `unwrap_or`), `author_timestamp` silently picked up the `%ct` value, and the monotonic clamp then hid the damage — the same masking the piped-subject fix in dba9dad closed. Since the field order is a documented invariant shared by four producers, close the whole class as the reviewer suggested: separate fields with `%x1f` (the unit separator) instead of relying on field order to keep `|` harmless. `BRANCH_COMMIT_LOG_FIELDS` and the four producers behind it pick the change up from the one const; `BATCH_FAST_SCRIPT`'s inlined copy is updated in kind and stays ASCII-safe because git expands `%x1f` itself, with the existing drift test still tying the script to the const. `commit_reassociation`'s sibling format gets the same treatment — its `%ae` sits directly before `%at`, exactly the shape the review's failure corrupts, and there a shifted timestamp breaks identity matching, not just ordering. The subject still goes last, so even the separator byte itself in a subject can't reach the timestamps. Covered by the reworked parse tests: a fixture with `|` in the name, the email, and the subject asserting nothing shifts, plus the separator-in-subject remainder case, in both parsers. Signed-off-by: Matt Toohey <[email protected]>
Review 0a5af850 on 9ffc931 flagged that the commit before it had spent effort keeping a dead shell script in sync: `BATCH_FAST_SCRIPT`'s inlined `git log` format was rewritten to the `%x1f` delimiter and pinned with a drift test, for a script nothing runs. Note be1f03ab traced when it died. Commit 9318b97 ("fix: eliminate timeline disruption when adopting auto-review", PR #700, May 2026) removed fetching from timeline loading, which retired the two-stream split the family implemented: a fast local-only stream (HEAD, branch, status, commits — no fetch) that emitted a partial timeline with a placeholder git state, front-running a slow stream that blocked on `git fetch`. Once the build never fetches there is no latency to front-run, so that commit deleted every call site, in Rust and in the frontend's `timeline-partial` handling, and left the definitions behind. Nothing has called them in the fifteen months since. So delete the whole family from `git/state.rs`: `FastGitState` and `into_placeholder_git_state`, `compute_fast_local_git_state`, `complete_local_git_state`, `BATCH_FAST_SCRIPT` with `BatchFastOutput` / `parse_batch_fast_output` / `into_fast_git_state` / `compute_fast_git_state_batched`, plus the two-stream scheduling helpers `needs_fetch` (whose doc names the path it chose between) and `local_git_state_cache_key`, which 9318b97 orphaned in the same edit. The `git/mod.rs` re-exports go with them. `BRANCH_COMMIT_LOG_FIELDS` goes too: it existed only so the script could inline the field list that `BRANCH_COMMIT_LOG_FORMAT` passes as an argument, so with the script gone its sole remaining consumer was the test asserting the two match. Keeping it would leave behind a fresh instance of exactly what the review objected to. The producer count for the shared commit-log format drops from four to three — worktree.rs's doc comment naming the script is gone, and the three surviving producers all take the format as an argument. No behaviour change; nothing referenced any of this. Verified with `cargo check --all-targets` (no new warnings), `cargo clippy --all-targets` (only the pre-existing warnings in test_utils.rs, store/tests.rs and the acp_stream_probe example), `cargo fmt --check`, and `cargo test --lib git::` — 82 passed. Signed-off-by: Matt Toohey <[email protected]>
…rebase Review d1db4957 on 0837bb0 flagged the last open gap on this branch: reassociation was guarded against a mid-rebase HEAD (dba9dad), but the pending-row completion right below it wasn't. A rebase-handoff turn that ends with conflicts unresolved leaves HEAD detached on a partially applied commit; `current_head != pre_sha` fired, and `complete_pending_commit_sha` handed that throwaway SHA to the rebase session's pending row. If the rebase was later finished with `--continue`, the pending row owned the rewritten first commit, so the matcher skipped it as claimed and the authoring session's row stayed orphaned — the exact mis-attribution this branch set out to fix, moved one commit down. If it was aborted, the row named a commit on no branch at all. Either way `recorded` also triggered the auto-review follow-up on a mid-rebase state. Implement plan note 2c4270b8: extend the mid-rebase skip from the reassociation call to the whole detection arm. `commit_reassociation` gains a public `head_is_attached_to_branch` — the same attached-HEAD question `reassociate_after_rebase` already answers internally, now sharing its branch-resolution and git-runner prologue via `branch_git_runner` (errors read as "not attached", the safe direction). `run_post_completion_hooks` asks it once for rebase-pipeline sessions and, mid-rebase, skips reassociation, the pending claim, the amend path, and `committed_branch_id`, so no row is touched and no auto-review fires. Deferring needs no cleanup pass: resumed sessions re-capture `pre_head_sha` at turn start, so the next turn lands back in the hooks with HEAD attached. After `--continue` finishes, reassociation repoints the authored rows first and the pending row drops in `complete_pending_commit_sha`'s duplicate-SHA branch — identical to the no-conflict pipeline path; after `--abort`, the restored head hits the same branch. A session never resumed leaves the pending row `sha IS NULL`, an ordinary failed commit attempt. The conflict test's setup moves into a shared `conflicted_rebase` fixture, and the test now asserts what the review noted it didn't: the pending row stays unclaimed and the hooks return no branch to review. Two new integration tests drive the deferred row through both exits — resolve-and-`--continue` (rows reassociated, review follows, pending dropped as a duplicate) and `--abort` (everything back on the original SHAs, pending dropped). Still out of scope, as the plan note records: a final handoff turn whose HEAD ends where that turn's snapshot started (e.g. `git rebase --skip` dropping the conflicted commit as the last act) takes the HEAD-unchanged arm, so earlier rewritten commits wait for reassociation until the next head-moving turn. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings), `cargo fmt --check`, and `cargo test --lib` — 546 passed. Signed-off-by: Matt Toohey <[email protected]>
matt2e
force-pushed
the
reassociate-sessions-after-rebase
branch
from
August 5, 2026 03:26
a0a8d2a to
b2e5df3
Compare
…call Review 5b5ce229 on b2e5df3 flagged duplicated work on the turn-completion path. A rebase-handoff turn whose HEAD moved made two back-to-back calls into `commit_reassociation`: `head_is_attached_to_branch` to decide whether the rebase was over, then — if it was — `reassociate_after_rebase`, which re-resolved the branch and re-ran the same `symbolic-ref` before doing its real work. On a Blox workspace that second `symbolic-ref` is a full `ws exec` round trip to a cloud workstation, on every successful handoff turn. (One correction to the review's cost estimate: `resolve_branch_workspace_subpath` is DB-only, so the duplicated remote call was exactly one, not each of the three.) The review suggested an internal entry point taking the already-built `(branch, git)` pair. That shape forks badly: keep its own attached-HEAD check and the duplicate `symbolic-ref` remains; drop it and the dba9dad invariant "no rows are touched unless HEAD is attached" stops being enforced by the module. Unnecessary, because the gate and the reassociation are 1:1 — every gate-true reassociates, every gate-false skips, and the other caller `finalize_rebase_pipeline_without_ai` wants check-then-reassociate too. So merge them: `reassociate_after_rebase` now returns a `Reassociation` (`MidRebase` | `Done { remapped }`), resolving once, checking once, and reporting which way it went. The check still happens exactly once inside the module — now immediately before rows are touched rather than two calls earlier, holding the reviewer's safety property more tightly. `head_is_attached_to_branch` goes; the gate was its only caller. `run_post_completion_hooks` replaces its gate plus its inner reassociation with the single call, and `mid_rebase` keeps gating the whole arm (pending claim, amend path, auto-review) as before. The load-bearing ordering — reassociate before `complete_pending_commit_sha` claims the new HEAD — is now true by construction. `branch_git_runner` failures adopt the gate's old error mapping and read as `MidRebase`, the safe direction, and the unattached-HEAD log drops from `warn` to `info` since a mid-rebase handoff turn hits it routinely. `Err` is reserved for failures after the check passed, so the `session_runner` helper folds it into `Done` — detection must proceed, exactly today's best-effort semantics. Remote cost for the attached case: two branch resolves and two `symbolic-ref` calls become one each. No behaviour change, so no new tests: nothing called either function directly, and the existing suite is the regression net — both completion paths' reassociation tests and the three `conflicted_rebase` tests (mid-rebase defers everything; `--continue` and `--abort` still settle every row). A "symbolic-ref runs once" assertion would need injection machinery worth more than the check; the win is structural, the second call site no longer exists. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings), `cargo fmt --check`, and `cargo test --lib` — 593 passed. Signed-off-by: Matt Toohey <[email protected]>
Plan note f6c9e34c: Staged read git HEAD on a Blox workspace two different ways, and on a multi-repo workspace they answered about two different checkouts. Every HEAD snapshot went through a bare `sq blox ws exec <ws> -- git rev-parse HEAD`, which runs at whatever cwd a bare exec lands in, while `commit_reassociation`'s runner went through `run_workspace_git`, which prepends `-C /home/bloxer/<clone>[/<subpath>]` from the branch row. One project gets one workspace and additional repos are cloned into it as siblings, so at most one clone can be the bare-exec cwd and every branch on the others was read at the wrong checkout. The mid-rebase guard (b2e5df3) is where that disagreement first became load-bearing: `head_is_on_branch` returns false on a wrong-branch answer or any error, both of which a foreign checkout can produce, so the hooks would take the defer arm forever — the pending row never gets its SHA and no auto-review fires, with one `log::info!` per turn as the only trace. The inverse holds too: a clean primary repo masks a genuinely mid-rebase branch repo. But the divergence predates the guard. `pre_head_sha` and `current_head` are consistent with each other and both about the wrong repo, so an unrelated commit landing in the primary repo makes `current_head != pre_sha` fire for a session that committed nothing, while a real commit on the branch repo reads as "HEAD unchanged". The resolved form is what the rest of the remote path already uses — the rebase itself (`run_remote_pipeline_command` cds into `remote_working_dir`), the agent's cwd, timeline commit deletion, diff collection — so the bare reads were the outliers, not the guard. `branches.rs` gains the runner next to the resolution it needs: `GitRunner`, `git_runner` for an already-resolved directory, and `branch_git_runner`, which resolves one from the branch row (the same call `remote_working_dir` is built from, so the two agree byte for byte). `head_sha` reads HEAD through a runner so no caller hand-rolls `rev-parse`, and `branch_head_sha` wraps it for async callers. Both `ws_exec` argv builders now share `workspace_git_args`, extracted pure so the one rule everything here rests on is testable without a workstation. `commit_reassociation` sheds the workspace knowledge: its runner moves out and `reassociate_after_rebase` takes `(store, &Branch, &GitRunner)`. Both callers hold a config carrying `remote_working_dir`, so — one deviation from the plan — the branch-id wrapper it proposed keeping would have had no callers, and a second entry point would have forced `reassociate_rebased_commits` to duplicate its logging per shape. The attached-HEAD check still runs exactly once, inside the module, immediately before any row is touched. `run_post_completion_hooks` takes `remote_working_dir` and builds one runner that the HEAD read and the reassociation share, dropping the second branch resolution the reviewer flagged; a branch row that won't load now reads as `MidRebase`, which is where the old resolution failure already landed. `current_pipeline_head` reads `config.remote_working_dir` through the same primitive, with no signature churn at its three call sites, and `finalize_rebase_pipeline_without_ai` shares one runner the same way. The capture sites move in the same commit, because they must: if `current_head` resolves and `pre_head_sha` doesn't, every remote commit session on a multi-repo workspace compares two unrelated SHAs and reports a phantom commit. That's the resume path, the start path, the queued path and the web-server start path, all via `branch_head_sha`. The three review-anchor sites go too — strictly beyond the commit-detection problem, but the identical one-line change through the identical helper, and a bare-read SHA stored as `reviews.commit_sha` matches nothing in `review_is_visible_in_timeline`, so the review stays hidden. Each site's own error split (a remote failure degrades, a local one fails the command) is preserved in `commit_pre_head_sha` / `review_tip_sha` rather than hand-rolled four more times. `diff_cache` needed nothing: it already resolves the clone dir itself and inherits the corrected SHA. `session_commands::run_blox_blocking` had no callers left and is deleted. Branches with `project_repo_id: None` still resolve to `None` and run bare, byte-identical to today. The local path only swaps `cli::run` for `cli_run_smart` — same cwd, one lite-env attempt before the captured one. There is no remote test harness, so the new coverage is the three `workspace_git_args` tests pinning the rule: a branch with a repo yields `git -C /home/bloxer/<clone>/<subpath> …`, one without runs bare, and an absolute `remote_working_dir` resolves to the same `-C` as the `home:` form. The rebase suite (both completion paths' reassociation tests, the three `conflicted_rebase` exits) all run with `workspace_name: None` and stays green unchanged as the regression net for the working path. Not done: the plan's `sq blox ws exec <ws> -- pwd` check, which would have told us whether the bare cwd is the primary clone (so single-repo projects see no change) or `$HOME` (so remote commit detection has been a silent no-op throughout). `sq blox ws list` reports no workstations on this machine, so there was nothing live to ask. Both readings point at this same fix; they differ only in how much it turns on. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings in test_utils.rs, store/tests.rs and the acp_stream_probe example), `cargo fmt --check`, and `cargo test --lib` — 596 passed. Signed-off-by: Matt Toohey <[email protected]>
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
1 participant
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.
A rebase rewrites every SHA on the branch, which orphaned each
commitsrow keyed by the old one: the timeline lost the authoring session, the head commit got mis-attributed to the mechanical "Rebase branch" session, and reviews keyed bycommit_shavanished behindreview_is_visible_in_timeline. Even once ownership was restored, commits sank below every note because the timeline sorts on committer date, which a rebase resets to "now".Changes
Reassociation — new
commit_reassociationmodule recovers the mapping from the DB plus git alone, with no pre-rebase capture:git rebasepreserves author email, author date, and subject, and the old objects stay resolvable, so the reconciler also survives an app restart mid-pipeline. It reads the old identities back from the orphaned SHAs, pairs them against the rewritten commits (oldest-to-oldest on identity collisions, skipping SHAs a row already owns), andStore::remap_commit_shasrepoints the rows and their reviews in one transaction. Both rebase completion paths call it before the pending row is resolved, so the existing "target SHA already owned" branch drops the duplicate pending row instead of stealing the head commit.It bails out unless HEAD is attached to the branch it was asked about — the same question as "is the rebase over?", since a rebase only moves the branch ref at the end. A turn ending with unresolved conflicts leaves HEAD detached on a partially applied commit, and repointing rows there is worse than leaving them orphaned.
Ordering — commit timeline items now sort by author date (
%at), the rebase-stable key the matcher already trusts, so notes stay interleaved instead of floating above the commits they followed. Author dates aren't monotonic along a branch (a cherry-pick keeps its week-old date), so both the branch timeline and the agent-facing branch history clamp with a shared running max in branch order, keeping the rendered order matchinggit log.CommitTimelineItemcarries a separatesortTimestampso the clamp drives order while the displayed date stays truthful.CommitInfo.timestampstays committer time forlatest_git_commit_ms, where a rebase should read as new activity; repo-browse and parent-branch listings stay on committer time too.Parsing —
BRANCH_COMMIT_LOG_FORMATmoved the subject last and one sharedparse_branch_commit_linetakes it as the remainder, replacing four hand-rolled index parsers. Previously a subject containing|shifted every field after it, corrupting the timestamp parse.No schema change.
Testing
Matcher unit tests, store tests for the remap and its collision guard, parse tests for the field order and a piped subject, clamp tests for both timelines, and integration tests driving a real
git rebase --signoffthrough each completion path plus one into a conflict, asserting the stopped rebase leaves every row and review untouched.🤖 Generated with Claude Code