fix(ci sync): probe a state commit that bundles source edits instead of trusting convergence - #10598
fix(ci sync): probe a state commit that bundles source edits instead of trusting convergence#10598luvkapur wants to merge 1 commit into
Conversation
PR Summary by QodoFix
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit e4cf976 |
|
Code review by qodo was updated up to the latest commit 1348de3 |
1348de3 to
c961235
Compare
|
Code review by qodo was updated up to the latest commit c961235 |
…of trusting convergence A converged branch/lane pair is not proof there is no work: a developer can commit their own `.bitmap` write together with source edits, which reads as the reconciler's own state commit. Detect a state commit that bundles source changes and probe it rather than short-circuiting to noop. The probe records the sync ledger whichever way it answers. Skipping the ledger on a clean answer left the developer's commit as the tip, so the same probe -- a checkout, a status and a snap -- ran again on every later run and never settled. The ledger commit moves the tip, which is what lets the existing sync-authored-tip check settle the next run before any workspace work, the same way a commit touching no bit-tracked file settles.
c961235 to
699aefe
Compare
| const probedClean = Boolean(probeOnly) && exported.status === 'noop'; | ||
|
|
||
| const recorded = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); | ||
| if (recorded.status === 'raced') return racedLedgerPushSummary(laneName); |
There was a problem hiding this comment.
1. Probe ledger contract conflict 🐞 Bug ≡ Correctness
When probeOnly is set and the snap/export no-ops, executeExportBranch() still records a sync ledger commit, which will advance the branch tip (commit uses --allow-empty). This conflicts with the new sync-planner.ts comment (claims no ledger commit on a clean probe) and with e2e expectations that both (a) tip must not move and (b) tip must move to include a ledger trailer, so the PR’s behavior/tests/docs cannot all be correct at once.
Agent Prompt
## Issue description
`probeOnly` clean probes currently still record the sync ledger commit (advancing the branch tip), but the PR adds conflicting contracts:
- `sync-planner.ts` comments say a clean probe settles **without** the ledger commit.
- `ci-sync-state.e2e.ts` asserts the branch tip is unchanged after a clean probe.
- `ci-sync.e2e.ts` (clean-probe suite) asserts a clean probe **must** record the ledger trailer to prevent re-probing.
This inconsistency needs to be resolved by choosing one behavior and updating code/comments/tests accordingly.
## Issue Context
`commitAllAndPush()` always uses `--allow-empty`, so recording the ledger on a clean probe will still create a new commit and move the remote branch tip.
## Fix Focus Areas
- scopes/git/ci/sync/lane-sync-executor.ts[708-735]
- scopes/git/ci/sync/lane-sync-executor.ts[1486-1500]
- scopes/git/ci/sync/sync-planner.ts[61-67]
- e2e/harmony/ci-sync-state.e2e.ts[63-91]
- e2e/harmony/ci-sync.e2e.ts[972-1007]
## Suggested resolution options
1) **If clean probe should NOT write:**
- In `executeExportBranch()`, when `probeOnly && exported.status === 'noop'`, return the converged/noop message **without** calling `recordLaneHeadOnBranch()`.
- Update/replace the "clean-probe" e2e that currently requires a ledger trailer, since it would re-probe on later runs unless another settle mechanism is added.
2) **If clean probe SHOULD write a settling ledger commit (current executor behavior):**
- Update the `sync-planner.ts` comment to remove/replace “WITHOUT the ledger commit”.
- Update `ci-sync-state.e2e.ts` to stop asserting `branchTipSha()` is unchanged after the clean probe and instead assert the expected settle mechanism (e.g., sync marker / lane-head trailer on the new tip).
Pick one contract and make all three (executor, planner docs, e2e suites) consistent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 699aefe |
|
Superseded by #10581, which now targets master and carries this branch's commit plus the rework that replaced its settle mechanism (the message-based withhold) with a read-only status probe. Reviewing there covers everything here. |
…, not commit messages (#10581) ## Proposed Changes - Fix two data-loss bugs: `bit ci sync` could report a branch as converged and silently drop its work. - Decide convergence from content: before an export, run `bit status` and compare the branch's files with the lane. - Post one maintained run-summary comment on the branch's pull request after a successful export. *(Replaces #10598 — this branch contains its commit and reworks its mechanism.)* ## Bug 1: a commit that bundles source edits with a `.bitmap` write was invisible The sync reads "work on the branch" as "commits after the commit that last wrote `.bitmap`". A single commit that carries a source edit **and** a `.bitmap` write is that state commit itself, so the count is zero. The run reported converged and the edit never reached the lane. The conflict-resolution recipe (import, fix by hand, one commit) produces exactly this shape. The fix detects this shape: a state commit that also touched other files is suspected work, and the run must check it instead of trusting the fingerprints. ## Bug 2: the check trusted commit messages, and a squash-merge forges them The first version of that check wrote a marker commit and recognized it later by its message (`[bit-sync]` on its own line). But a squash-merge copies the messages of the squashed commits into the new commit's body, and a synced branch always contains sync commits with that line. So a squashed developer commit looked machine-written. The run reported `noop (converged)`. The developer's edits never reached the lane. There was no halt, no label, and no warning. ## The fix The run no longer reads commit messages to decide convergence. It asks the workspace instead. Each run, the planner picks `export-branch` when the lane did not move but the branch may hold new work. Git metadata alone cannot confirm the work, so the executor now checks first: 1. Check out the branch's files. 2. Switch the workspace onto the lane. Keep the branch's files on disk. 3. Run one `bit status`. This compares the branch's actual files with the versions the lane records. If status shows nothing, the branch and the lane hold the same content. The run reports converged and writes nothing: no snap, no commit, no push. If status shows changes, the run snaps and exports exactly as before. A clean check writes nothing, so it is safe to repeat on every run. This removes the old settle mechanism: an empty ledger commit pushed to the developer's branch, recognized on the next run by its own message. The marker check remains in one place only — branch deletion after the lane is gone, where no content exists to compare. Two behavior changes, both intended: - A converged branch whose tip bundles the lane's files (every branch that `import-lane` created) re-checks on every run. This is a read. It pushes nothing and triggers no CI run. - When the lane moves and the branch tip bundles files, the plan is now `merge-diverged`, not `import-lane`. An import would write the lane's files over the branch's bundled work. That is the same bug in a second shape. A merge that finds nothing new says so in its summary instead of claiming it exported. ## The run-summary comment A sync run can change a lane component — a new snap, a dependency range — while the branch's own diff shows nothing. The reviewer cannot see this in the pull request. After a successful export (plain or merge), the run now writes one comment on the branch's pull request: the components it snapped, with their new versions, and the synced branch/lane anchors. The run updates the same comment in place on every export. It never adds a second one. This uses a new optional provider capability, `GitHostProvider.upsertComment`. A provider without it is skipped. A missing git host, a missing pull request, or a comment API error never fail the run. The comment search follows GitHub's pagination but never follows a `Link` target off the API host (the bearer token must stay home), stops at the first marker match, and treats an exhausted page budget as "unknown" rather than "absent" — so it never posts a duplicate. ## Tests - New e2e: a squashed branch whose body quotes the marker exports its work onto the lane; a bundled commit exports instead of reading as converged; a clean check pushes nothing, twice over; a docs-only commit checks without writing, run after run. - Planner spec: the branch-deletion decision table, including bundled sources on the reconciler's own tip. - `github-client.spec.ts`: `upsertComment` posts, patches in place, honors `createIfAbsent: false`, follows pagination, refuses off-host `Link` targets, and never posts through an exhausted page budget. - `lane-sync-executor.spec.ts`: the summary-comment helpers, and merge-diverged's exported vs nothing-new split. - Full `e2e/harmony/ci-sync.e2e.ts`: 52 passing.
1 participant
Proposed Changes
bit ci syncwrongly reporting "converged" when one git commit contains both a source edit and a.bitmapchange. The edit never reached the lane, and the next lane update would overwrite it on the branch.The bug
Sync decides "does the branch have new work?" by counting commits AFTER the last commit that touched
.bitmap. When a single commit changes.bitmapAND source files, that count is zero — so sync reportsnoop (converged)even though the source edit was never snapped. The lane never gets the edit, and a laterimport-laneoverwrites it.This is exactly the commit shape sync's own conflict-resolution instructions produce (
bit lane import, fix the files, commit once). Found live while testing the halt → resolve → resume flow.The fix
Git alone can't tell whether the bundled files are already inside the recorded snap (a dev who snapped, exported, and committed everything at once) or were never snapped. So instead of guessing, sync checks with bit:
Where a wrong "no work" answer could lose something (branch deletion, divergence, first contact), the bundled commit counts as work — the safe direction. Sync's own ledger commits are exempt (they legitimately bundle merged sources).
Tests
noop (converged)), green with the fix (the lane gets the edit; the next run converges).ci-sync-state.e2e.tscells that had locked the old behavior are updated: the converged-dev case still ends with zero writes, the invisible-edit case exports immediately.Note
Master (with #10593 merged) is merged in. At first contact, a bundled commit routes to
adopt-branch— adoption already checks with bit before writing — and the different-lane guard now also fires for bundled commits.