From 5f20bfbd7faf2c89d673773a8c41d7642c0953ca Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 11:31:24 -0400 Subject: [PATCH 01/22] fix(ci): fail the status verification only on components the run snaps or tags A real scope carries components with tag blockers (circular dependencies on teambit.api-reference). The global verification halted every snap in the repository, while bit snap itself scopes its checks to the snapped components. The verification now fails only on issues in listTagPendingIds; bit ci verify stays global. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 34 +++++++++++++++++++++++ scopes/git/ci/ci.main.runtime.ts | 46 ++++++++++++++++++++++++-------- 2 files changed, 69 insertions(+), 11 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 797ced629d78..4f747382beab 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1120,6 +1120,40 @@ describe('bit ci sync', function () { }); }); + // A real scope carries components with tag blockers (e.g. circular dependencies). The snap only + // includes the lane's pending components, so a blocker on an untouched component must not halt it. + describe('a snap-blocking issue on a component the lane never touches', () => { + const LANE = 'clean-lane'; + let defaultBranch: string; + let devPath: string; + + before(() => { + ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); + // a circular pair on main: a tag blocker on components no lane sync will ever snap + helper.fs.outputFile('comp3/index.js', `require('@${helper.scopes.remote}/comp4');`); + helper.fs.outputFile('comp4/index.js', `require('@${helper.scopes.remote}/comp3');`); + helper.command.addComponent('comp3'); + helper.command.addComponent('comp4'); + helper.command.install(); + helper.command.tagAllWithoutBuild('--ignore-issues="CircularDependencies"'); + helper.command.export(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "add a circular pair to main"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); + seedSync(LANE); + branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); + }); + + it('snaps the dev commit onto the lane although the untouched pair has a tag blocker', () => { + const { output, exitCode } = syncRun(LANE); + expect(exitCode, `bit ci sync output:\n${output}`).to.equal(0); + expect(output).to.not.include('Workspace status verification failed'); + expect(output).to.include(`${LANE} -> export-branch`); + expect(laneTipFile(devPath, 'comp1/index.js')).to.include('dev-commit-1'); + }); + }); + describe('a stale bit-sync/main that conflicts with the default branch', () => { const SYNC_BRANCH = 'bit-sync/main'; let defaultBranch: string; diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index 183077f012c7..0aa50b2607fc 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -1,6 +1,6 @@ import type { RuntimeDefinition, SlotRegistry } from '@teambit/harmony'; import { Slot } from '@teambit/harmony'; -import { CLIAspect, type CLIMain, MainRuntime } from '@teambit/cli'; +import { CLIAspect, type CLIMain, MainRuntime, formatWarningSummary } from '@teambit/cli'; import { LoggerAspect, type LoggerMain, type Logger } from '@teambit/logger'; import { WorkspaceAspect, type Workspace } from '@teambit/workspace'; import { BuilderAspect, type BuilderMain } from '@teambit/builder'; @@ -434,22 +434,46 @@ export class CiMain { return 'chore: update .bitmap and lockfiles as needed [skip ci]'; } - private async verifyWorkspaceStatusInternal(strict: boolean = false) { + /** + * `scopeToPendingComponents`: fail only on issues in the components a snap/tag would include + * (`listTagPendingIds`). A real scope carries components with tag blockers (e.g. circular + * dependencies), and a global failure would block every snap in the repo β€” including snaps that + * never touch the blocked components. The snap itself still refuses its own components' blockers. + */ + private async verifyWorkspaceStatusInternal( + strict: boolean = false, + { scopeToPendingComponents = false }: { scopeToPendingComponents?: boolean } = {} + ) { this.logger.console('πŸ“Š Workspace Status'); this.logger.console(chalk.blue('Verifying status of workspace')); + const formatOptions = strict + ? { strict: true, warnings: true } // When strict=true, fail on both issues and warnings + : { failOnError: true, warnings: false }; // By default, fail only on errors (tag blockers) const status = await this.status.status({ lanes: true }); - const { data: statusOutput, code } = await this.status.formatStatusOutput( - status, - strict - ? { strict: true, warnings: true } // When strict=true, fail on both errors and warnings - : { failOnError: true, warnings: false } // By default, fail only on errors (tag blockers) - ); + const { data: statusOutput, code } = await this.status.formatStatusOutput(status, formatOptions); // Log the formatted status output this.logger.console(statusOutput); - if (code !== 0) { + let effectiveCode = code; + if (code !== 0 && scopeToPendingComponents) { + const pending = ComponentIdList.fromArray(await this.workspace.listTagPendingIds()); + const scoped = { + ...status, + componentsWithIssues: status.componentsWithIssues.filter((c) => pending.hasWithoutVersion(c.id)), + }; + ({ code: effectiveCode } = await this.status.formatStatusOutput(scoped, formatOptions)); + if (effectiveCode === 0) { + this.logger.console( + formatWarningSummary( + 'The issues above are on components this run does not snap or tag β€” they do not block it' + ) + ); + } + } + + if (effectiveCode !== 0) { throw new Error('Workspace status verification failed'); } @@ -868,7 +892,7 @@ export class CiMain { const laneId = await this.lanes.parseLaneId(laneIdStr); - await this.verifyWorkspaceStatusInternal(strict); + await this.verifyWorkspaceStatusInternal(strict, { scopeToPendingComponents: true }); await this.importer .import({ @@ -1652,7 +1676,7 @@ export class CiMain { ); } - const { status } = await this.verifyWorkspaceStatusInternal(strict); + const { status } = await this.verifyWorkspaceStatusInternal(strict, { scopeToPendingComponents: true }); const hasSoftTaggedComponents = status.softTaggedComponents.length > 0; From 4540f7f24ecf2915c286becf2f0ba22c1c39da02 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 13:31:20 -0400 Subject: [PATCH 02/22] feat(ci): pure classification helpers for dependency-context drift --- scopes/git/ci/sync/context-drift.spec.ts | 76 ++++++++++++++++++++++++ scopes/git/ci/sync/context-drift.ts | 52 ++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 scopes/git/ci/sync/context-drift.spec.ts create mode 100644 scopes/git/ci/sync/context-drift.ts diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts new file mode 100644 index 000000000000..ae8167c4ea91 --- /dev/null +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -0,0 +1,76 @@ +import { expect } from 'chai'; +import { classifyPayloadDiff, convergenceMessage, blockerNamesUnion } from './context-drift'; + +describe('classifyPayloadDiff', () => { + const base = { + files: [{ file: 'aaa', relativePath: 'index.js' }], + mainFile: 'index.js', + packageDependencies: { 'is-odd': '1.0.0' }, + devPackageDependencies: {}, + peerPackageDependencies: {}, + log: { date: '1', username: 'a' }, + }; + + it('classifies a package-range-only change as depOnly', () => { + const fromFs = { ...base, packageDependencies: { 'is-odd': '3.0.1' }, log: { date: '2', username: 'b' } }; + const res = classifyPayloadDiff(base, fromFs); + expect(res.depOnly).to.equal(true); + expect(res.changedKeys).to.deep.equal(['packageDependencies']); + }); + + it('classifies a dev/peer reclassification as depOnly', () => { + const recorded = { ...base, peerDependencies: [{ id: 'scope/link' }], dependencies: [] }; + const fromFs = { ...base, peerDependencies: [], dependencies: [{ id: 'scope/link' }] }; + expect(classifyPayloadDiff(recorded, fromFs).depOnly).to.equal(true); + }); + + it('rejects a file change even when deps also changed', () => { + const fromFs = { + ...base, + files: [{ file: 'bbb', relativePath: 'index.js' }], + packageDependencies: { 'is-odd': '3.0.1' }, + }; + const res = classifyPayloadDiff(base, fromFs); + expect(res.depOnly).to.equal(false); + expect(res.changedKeys).to.include('files'); + }); + + it('rejects an extensions (config) change', () => { + const fromFs = { ...base, extensions: [{ name: 'teambit.envs/envs', config: { env: 'x' } }] }; + expect(classifyPayloadDiff(base, fromFs).depOnly).to.equal(false); + }); +}); + +describe('convergenceMessage', () => { + it('names the recorded and running bit versions', () => { + const msg = convergenceMessage(['1.12.61', '1.12.61', undefined], '2.0.69'); + expect(msg).to.equal('chore: align dependency context (recorded with bit 1.12.61, workspace runs bit 2.0.69)'); + }); + it('lists distinct recorded versions', () => { + const msg = convergenceMessage(['1.12.61', '2.0.10'], '2.0.69'); + expect(msg).to.include('1.12.61, 2.0.10'); + }); + it('handles no recorded versions', () => { + expect(convergenceMessage([undefined], '2.0.69')).to.equal( + 'chore: align dependency context (workspace runs bit 2.0.69)' + ); + }); +}); + +describe('blockerNamesUnion', () => { + const entry = (idStr: string, names: string[], blocker: boolean) => ({ + id: { toStringWithoutVersion: () => idStr }, + issues: { getAllIssueNames: () => names, hasTagBlockerIssues: () => blocker }, + }); + + it('unions blocker issue names of in-set components only', () => { + const res = blockerNamesUnion( + [entry('s/a', ['CircularDependencies'], true), entry('s/b', ['MissingDists'], true)], + new Set(['s/a']) + ); + expect(res).to.equal('CircularDependencies'); + }); + it('returns undefined when no in-set component has blockers', () => { + expect(blockerNamesUnion([entry('s/a', ['X'], false)], new Set(['s/a']))).to.equal(undefined); + }); +}); diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts new file mode 100644 index 000000000000..35bffcd2598d --- /dev/null +++ b/scopes/git/ci/sync/context-drift.ts @@ -0,0 +1,52 @@ +import { isEqual, omit } from 'lodash'; + +export const DRIFT_FIELDS = [ + 'dependencies', + 'devDependencies', + 'peerDependencies', + 'extensionDependencies', + 'flattenedDependencies', + 'packageDependencies', + 'devPackageDependencies', + 'peerPackageDependencies', +] as const; + +// Keys that legitimately differ between a recorded Version and one rebuilt +// from the filesystem, independent of any user change. +const VOLATILE_FIELDS = ['log', 'parents', 'squashed', 'origin'] as const; + +const EXCLUDED = [...DRIFT_FIELDS, ...VOLATILE_FIELDS]; + +export function classifyPayloadDiff( + recorded: Record, + fromFs: Record +): { depOnly: boolean; changedKeys: string[] } { + const keys = new Set([...Object.keys(recorded), ...Object.keys(fromFs)]); + const changedKeys = [...keys].filter( + (k) => !(VOLATILE_FIELDS as readonly string[]).includes(k) && !isEqual(recorded[k], fromFs[k]) + ); + const depOnly = isEqual(omit(recorded, EXCLUDED), omit(fromFs, EXCLUDED)); + return { depOnly, changedKeys }; +} + +export function convergenceMessage(recordedBitVersions: (string | undefined)[], runningBitVersion: string): string { + const distinct = [...new Set(recordedBitVersions.filter(Boolean))] as string[]; + const recordedPart = distinct.length ? `recorded with bit ${distinct.join(', ')}, ` : ''; + return `chore: align dependency context (${recordedPart}workspace runs bit ${runningBitVersion})`; +} + +export function blockerNamesUnion( + componentsWithIssues: { + id: { toStringWithoutVersion(): string }; + issues: { getAllIssueNames(): string[]; hasTagBlockerIssues(): boolean }; + }[], + inSet: Set +): string | undefined { + const names = new Set(); + for (const entry of componentsWithIssues) { + if (!inSet.has(entry.id.toStringWithoutVersion())) continue; + if (!entry.issues.hasTagBlockerIssues()) continue; + entry.issues.getAllIssueNames().forEach((n) => names.add(n)); + } + return names.size ? [...names].join(',') : undefined; +} From deaff85c722c0182790e9b0287146f14f7ceaf82 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 15:02:16 -0400 Subject: [PATCH 03/22] feat(ci): lane sync snaps pending minus dependency-context drift; verification scopes to the snap set Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 49 ++++ scopes/git/ci/ci.main.runtime.ts | 292 ++++--------------- scopes/git/ci/sync/context-drift-detector.ts | 50 ++++ scopes/git/ci/sync/lane-sync-executor.ts | 19 ++ scopes/git/ci/sync/main-config-sync.ts | 246 ++++++++++++++++ 5 files changed, 416 insertions(+), 240 deletions(-) create mode 100644 scopes/git/ci/sync/context-drift-detector.ts create mode 100644 scopes/git/ci/sync/main-config-sync.ts diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 4f747382beab..7236750b0b61 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1154,6 +1154,55 @@ describe('bit ci sync', function () { }); }); + // The engine-bump analogue reproducible with one bit binary: the committed root policy moves a + // recorded package range. The lane run must snap only the git-authored change and report the + // drifted component instead of sweeping it into the dev's snap. + describe('dependency-context drift is excluded from the lane snap', () => { + const LANE = 'drift-lane'; + let defaultBranch: string; + let devPath: string; + + before(() => { + ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); + helper.fs.outputFile('comp2/index.js', `require('is-odd');\nmodule.exports = () => 'comp2: with-pkg';\n`); + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '1.0.0' } }); + helper.command.install(); + helper.command.tagAllWithoutBuild(); + helper.command.export(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "comp2 records is-odd 1.0.0"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + // Lane creation must happen while the policy still matches comp2's recorded range β€” otherwise + // the dev's own (unscoped) `bit snap` would sweep the drift in too, and there'd be nothing left + // for `bit ci sync` to exclude. + devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); + seedSync(LANE); + branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); + // The default branch's own resolution context moves AFTER the lane forked β€” the analogue of an + // engine bump: `bit ci sync` boots on the default branch, and a workspace-level policy/engine + // aggregate is resolved once at that boot (mid-run branch checkouts don't re-read it β€” see + // `Workspace._reloadConsumer`, which reloads the consumer/bitmap but not this). So the run's + // *actual* resolution context is whatever is in effect here, regardless of which branch it + // later checks out β€” exactly the drift a real engine bump produces on an untouched component. + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); + helper.command.install(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "bump is-odd policy (engine-bump analogue)"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + }); + + it('snaps the dev commit, reports the drifted component, and keeps it off the lane', () => { + const before = remoteLaneFingerprint(LANE); + expect(before).to.not.include('comp2'); + const { output, exitCode } = syncRun(LANE); + expect(exitCode, `bit ci sync output:\n${output}`).to.equal(0); + expect(output).to.include('dependency-context drift'); + expect(output).to.include('comp2'); + expect(laneTipFile(devPath, 'comp1/index.js')).to.include('dev-commit-1'); + expect(remoteLaneFingerprint(LANE)).to.not.include('comp2'); + }); + }); + describe('a stale bit-sync/main that conflicts with the default branch', () => { const SYNC_BRANCH = 'bit-sync/main'; let defaultBranch: string; diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index 0aa50b2607fc..51a05bda1e50 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -13,9 +13,6 @@ import { ExportAspect, type ExportMain } from '@teambit/export'; import { ImporterAspect, type ImporterMain } from '@teambit/importer'; import { CheckoutAspect, checkoutOutput, type CheckoutMain } from '@teambit/checkout'; import type { MergeStrategy } from '@teambit/component.modules.merge-helper'; -import { getDivergeData } from '@teambit/component.snap-distance'; -import { ComponentConfigMerger } from '@teambit/config-merger'; -import { DependencyResolverAspect } from '@teambit/dependency-resolver'; import execa from 'execa'; import chalk from 'chalk'; import type { ReleaseType } from 'semver'; @@ -28,18 +25,20 @@ import { CiSyncCmd } from './commands/sync.cmd'; import { git } from './git'; import { ComponentIdList } from '@teambit/component-id'; import type { ComponentID } from '@teambit/component-id'; -import { compact, isEqual } from 'lodash'; +import { isEqual } from 'lodash'; import type { Version, LaneComponent, Lane } from '@teambit/objects'; import { Ref } from '@teambit/objects'; import type { LaneId } from '@teambit/lane-id'; import type { ConsumerComponent } from '@teambit/legacy.consumer-component'; import { SourceBranchDetector } from './source-branch-detector'; import { generateRandomStr } from '@teambit/toolbox.string.random'; -import { pMapPool } from '@teambit/toolbox.promise.map-pool'; -import { concurrentComponentsLimit } from '@teambit/harmony.modules.concurrency'; import { extractSkipTasksFromMessage } from './skip-tasks-from-message'; import { isPullRequestRef } from './pull-request-ref'; import { adoptAndRetrySwitch, isLaneMissingComponentError } from './sync/adopt-lane-new-components'; +import { getBitVersion } from '@teambit/bit.get-bit-version'; +import { detectContextDrift as detectContextDriftImpl } from './sync/context-drift-detector'; +import type { ContextDriftReport } from './sync/context-drift-detector'; +import { syncConfigFromMain as syncConfigFromMainImpl } from './sync/main-config-sync'; export type CiSwitchLaneOptions = SwitchLaneOptions & { /** after an adoption retry, write the adopted components' files (`checkout --reset`) */ @@ -58,6 +57,8 @@ import { isValidGitBranchName } from './sync/sync-config'; */ export type GitHostProviderSlot = SlotRegistry; +export type { ContextDriftReport }; + // Two distinct conflicts can surface from the remote on a concurrent `bit ci pr` race. // LANE_HASH_MISMATCH fires when both runners called `Lane.create` (the lane didn't exist on // the remote yet), so they each minted a random `sha1(v4())` hash β€” `sources.mergeLane` then @@ -435,15 +436,12 @@ export class CiMain { } /** - * `scopeToPendingComponents`: fail only on issues in the components a snap/tag would include - * (`listTagPendingIds`). A real scope carries components with tag blockers (e.g. circular - * dependencies), and a global failure would block every snap in the repo β€” including snaps that - * never touch the blocked components. The snap itself still refuses its own components' blockers. + * `snapIds`: fail only on issues in the components this run actually snaps. A real scope carries + * components with tag blockers (e.g. circular dependencies), and a global failure would block every + * snap in the repo β€” including snaps that never touch the blocked components. The snap itself still + * refuses its own components' blockers. */ - private async verifyWorkspaceStatusInternal( - strict: boolean = false, - { scopeToPendingComponents = false }: { scopeToPendingComponents?: boolean } = {} - ) { + private async verifyWorkspaceStatusInternal(strict: boolean = false, { snapIds }: { snapIds?: ComponentID[] } = {}) { this.logger.console('πŸ“Š Workspace Status'); this.logger.console(chalk.blue('Verifying status of workspace')); @@ -457,18 +455,16 @@ export class CiMain { this.logger.console(statusOutput); let effectiveCode = code; - if (code !== 0 && scopeToPendingComponents) { - const pending = ComponentIdList.fromArray(await this.workspace.listTagPendingIds()); + if (code !== 0 && snapIds) { + const inSet = ComponentIdList.fromArray(snapIds); const scoped = { ...status, - componentsWithIssues: status.componentsWithIssues.filter((c) => pending.hasWithoutVersion(c.id)), + componentsWithIssues: status.componentsWithIssues.filter((c) => inSet.hasWithoutVersion(c.id)), }; ({ code: effectiveCode } = await this.status.formatStatusOutput(scoped, formatOptions)); if (effectiveCode === 0) { this.logger.console( - formatWarningSummary( - 'The issues above are on components this run does not snap or tag β€” they do not block it' - ) + formatWarningSummary('The issues above are on components this run does not snap β€” they do not block it') ); } } @@ -556,226 +552,12 @@ export class CiMain { }).sync(opts); } - /** - * Sync *config-only* changes from main onto the lane β€” without a full `bit lane merge`. - * - * In this workflow git is the source of truth for files: the PR author merges the default branch - * into their PR branch, so source changes arrive via git. The one thing git can't carry is - * config that's already been *tagged into objects* on main β€” e.g. another PR ran `bit env set` / - * `bit deps set`; those records lived in `.bitmap`, rode git into main, and `bit ci merge` baked - * them into the component's Version (clearing them from `.bitmap`). A long-running PR's lane - * would otherwise miss them. - * - * A full lane merge is the wrong tool here: it does a 3-way *file* merge and refuses to run while - * the workspace has modified components β€” but in `bit ci pr` the workspace is always dirty (the - * PR's changes, not yet snapped). So instead we do a per-component 3-way merge of the aspect - * *config only* (base = common ancestor, ours = lane, theirs = main), keeping the PR's config on - * conflict, and stash the result on an `unmergedComponents` entry's `mergedConfig`. The - * subsequent `snap` reads it (via the aspects-merger on component load) and bakes main's config - * into the new snap, while the snap's files still come from the workspace (git). No file - * checkout, so no clean-workspace requirement. - */ + /** Config-only sync from main onto the lane; see `sync/main-config-sync.ts`. */ private async syncConfigFromMain(laneId: LaneId) { - const legacyScope = this.workspace.scope.legacyScope; - const repo = legacyScope.objects; - const mainLaneId = this.lanes.getDefaultLaneId(); - const currentLane = await this.lanes.getCurrentLane(); - if (!currentLane) return; - const workspaceIds = this.workspace.listIds(); - - this.logger.console(chalk.blue(`Syncing config changes from ${mainLaneId.toString()} into ${laneId.toString()}`)); - - // Resolve each lane component's head on main once, keeping only those that are on main and whose - // lane head differs from it (the rest have nothing to sync). This single pass feeds both the - // pre-fetch below and the merge loop, so we never load the same ModelComponent twice. - const componentsToSync = compact( - await Promise.all( - currentLane.components.map(async (laneComp) => { - try { - const modelComponent = await legacyScope.getModelComponentIfExist(laneComp.id); - const mainHead = modelComponent?.head; // the component's head on main - if (!modelComponent || !mainHead || mainHead.isEqual(laneComp.head)) return undefined; - return { laneComp, modelComponent, mainHead }; - } catch (e: any) { - // Best-effort per component (same contract as the merge loop below): one component's - // load failure shouldn't reject Promise.all and abort the whole config sync. - this.logger.console( - chalk.yellow( - ` ${laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})` - ) - ); - return undefined; - } - }) - ) + await syncConfigFromMainImpl( + { workspace: this.workspace, lanes: this.lanes, importer: this.importer, logger: this.logger }, + laneId ); - - // The lane import (switchToLane) brought each component's lane history plus the lightweight - // version-history (the parent graph) β€” that's enough for the diverge check below to see that - // main is ahead β€” but NOT the full Version object for main's head wherever main advanced past - // the lane's fork point. Those objects live only on main and were never fetched. Without them - // `loadVersion(mainHead)` throws VersionNotFoundOnFS, the per-component catch swallows it as - // "skipping config sync from main", and the sync silently degrades to a no-op for every - // diverged component. Pre-fetch main's head objects in one batched remote call (mirroring the - // lane-merge flows β€” see merge-status-provider / merge-lanes). Pass the *specific* main-head - // version so `cache: true` still fetches it: the component already exists locally at its lane - // version, so a version-less id would look satisfied and skip the remote. - const mainHeadIds = componentsToSync.map(({ laneComp, mainHead }) => - laneComp.id.changeVersion(mainHead.toString()) - ); - await this.prefetchFromMainForConfigSync(mainHeadIds, 'head objects'); - - // Resolve each component's diverge state up front β€” before the merge loop β€” so we can also - // pre-fetch the common-ancestor objects below. getDivergeData only walks the parent graph, - // which is already local (switchToLane brings the version-history, and the head pre-fetch above - // reinforced it), so no full Version object is needed yet. Keep only components where main is - // actually ahead or diverged; the rest have nothing to bring in from main. Bound the fan-out - // (getDivergeData traverses each component's version graph) so a lane with many components - // doesn't spawn one unbounded burst of concurrent graph walks. - const componentsToMerge = compact( - await pMapPool( - componentsToSync, - async (item) => { - try { - const divergeData = await getDivergeData({ - repo, - modelComponent: item.modelComponent, - sourceHead: item.laneComp.head, - targetHead: item.mainHead, - throws: false, - }); - if (!divergeData.isTargetAhead() && !divergeData.isDiverged()) return undefined; - return { ...item, divergeData }; - } catch (e: any) { - // Best-effort per component (same contract as the merge loop below). - this.logger.console( - chalk.yellow( - ` ${item.laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})` - ) - ); - return undefined; - } - }, - { concurrency: concurrentComponentsLimit() } - ) - ); - - // The head pre-fetch above brought main's head Version plus the version-history (parent graph), - // but NOT the full Version object of the common ancestor (the lane's fork point) for components - // where the lane and main have BOTH snapped since the fork. The 3-way config merge below loads - // that base Version (`baseVersion.extensions`); without it, `loadVersion(baseSnap)` throws - // VersionNotFoundOnFS, the per-component catch swallows it as "skipping config sync from main", - // and the sync silently no-ops for every diverged component. The fork point lives on main and, - // like the head, was never fetched (includeVersionHistory carries the graph, not each ancestor's - // Version). Batch-fetch the bases in one call β€” same best-effort contract as the head pre-fetch. - const baseIds = compact( - componentsToMerge.map(({ laneComp, divergeData }) => { - const baseSnap = divergeData.commonSnapBeforeDiverge; - return baseSnap ? laneComp.id.changeVersion(baseSnap.toString()) : undefined; - }) - ); - await this.prefetchFromMainForConfigSync(baseIds, 'common-ancestor objects'); - - const syncedIds: ComponentID[] = []; - for (const { laneComp, modelComponent, mainHead, divergeData } of componentsToMerge) { - try { - const laneHead = laneComp.head; - const currentVersion = await modelComponent.loadVersion(laneHead.toString(), repo); - const otherVersion = await modelComponent.loadVersion(mainHead.toString(), repo); - // base = common ancestor. When the lane is strictly behind main (no divergence) the common - // ancestor IS the lane head, so the lane's own aspects serve as the base. - const baseSnap = divergeData.commonSnapBeforeDiverge; - const baseVersion = baseSnap ? await modelComponent.loadVersion(baseSnap.toString(), repo) : currentVersion; - - const configMerger = new ComponentConfigMerger( - laneComp.id.toStringWithoutVersion(), - workspaceIds, - undefined, // merging from main (the default lane) β€” there's no Lane object for it - currentVersion.extensions, - baseVersion.extensions, - otherVersion.extensions, - laneId.toString(), - mainLaneId.toString(), - this.logger, - 'ours' as MergeStrategy // keep the PR's config on a genuine conflict - ); - const mergedConfig = configMerger.merge().getSuccessfullyMergedConfig(); - if (!mergedConfig || !Object.keys(mergedConfig).length) continue; - - // Strip dependency deletion markers (version: '-'); the aspects-merger applies mergedConfig - // as-is, so a leftover '-' would land in the policy. - this.filterDeletedDependenciesFromConfig(mergedConfig); - - // Upsert: addEntry throws if an entry for this component already exists. A prior - // --keep-lane run that crashed mid-snap (or otherwise left unmerged.json entries behind) - // would otherwise make every later run throw here, skip the component, and keep serving - // stale config. Remove any existing entry first so repeated runs converge on main's latest. - legacyScope.objects.unmergedComponents.removeComponent(laneComp.id); - legacyScope.objects.unmergedComponents.addEntry({ - id: { scope: laneComp.id.scope, name: laneComp.id.fullName }, - head: mainHead, - laneId: mainLaneId, - mergedConfig, - }); - syncedIds.push(laneComp.id); - this.logger.console( - chalk.blue( - ` ${laneComp.id.toStringWithoutVersion()}: applying main's config (${Object.keys(mergedConfig).join(', ')})` - ) - ); - } catch (e: any) { - // Best-effort per component: one component's config-merge quirk shouldn't abort the whole - // `bit ci pr`. Log and move on β€” the build just won't reflect that component's main-side - // config this run. - this.logger.console( - chalk.yellow(` ${laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})`) - ); - } - } - - if (!syncedIds.length) { - this.logger.console(chalk.blue('No config changes from main to sync')); - return; - } - await legacyScope.objects.unmergedComponents.write(); - // The components were already loaded (and their aspects cached) earlier in this run, before the - // unmergedComponents entries existed. Clear the cache so the upcoming `snap` reloads them and - // the aspects-merger folds in the synced `mergedConfig`. - this.workspace.clearAllComponentsCache(); - this.logger.console(chalk.green(`Synced config from main for ${syncedIds.length} component(s)`)); - } - - /** - * Batch-fetch main-side Version objects the config merge needs (heads, then common ancestors), - * mirroring the lane-merge flows (merge-status-provider / merge-lanes). Best-effort: a fetch - * hiccup shouldn't abort `bit ci pr` β€” the merge loop still runs and any component whose object is - * still missing just logs the existing per-component skip. `label` names which objects for the log. - */ - private async prefetchFromMainForConfigSync(ids: ComponentID[], label: string) { - if (!ids.length) return; - try { - await this.importer.importObjectsFromMainIfExist(ids, { cache: true }); - } catch (e: any) { - this.logger.console( - chalk.yellow(`Could not pre-fetch main's ${label} for config sync (continuing): ${e?.message || e}`) - ); - } - } - - /** - * Copied from `merging.main.runtime` (`filterDeletedDependenciesFromConfig`): the config merge - * can emit deletion markers (`version: '-'`) for deps removed on main. The aspects-merger applies - * `mergedConfig` verbatim, so strip those here to avoid writing a policy entry with version '-'. - */ - private filterDeletedDependenciesFromConfig(mergeConfig?: Record): void { - const policy: Record> | undefined = - mergeConfig?.[DependencyResolverAspect.id]?.policy; - if (!policy) return; - Object.keys(policy).forEach((depType) => { - const filtered = policy[depType].filter((dep) => dep.version !== '-'); - if (filtered.length === 0) delete policy[depType]; - else policy[depType] = filtered; - }); } /** @@ -816,6 +598,19 @@ export class CiMain { } } + /** The bit binary this process runs β€” compared against a drifted component's `recordedBitVersion`. */ + getRunningBitVersion(): string { + return getBitVersion(); + } + + /** + * Split the tag-pending set into git-authored changes and dependency-context drift. + * See `sync/context-drift-detector.ts` for what counts as drift. + */ + async detectContextDrift(): Promise { + return detectContextDriftImpl(this.workspace, this.logger); + } + async verifyWorkspaceStatus() { await this.verifyWorkspaceStatusInternal(); @@ -843,6 +638,7 @@ export class CiMain { skipCleanup, skipTasks, noDestructiveRecovery, + snapIds, }: { laneIdStr: string; message: string; @@ -860,6 +656,8 @@ export class CiMain { * stale-lane case throws instead, which the sync executor surfaces as a halt for a human. */ noDestructiveRecovery?: boolean; + /** Snap only these ids (no version), not every tag-pending component; unset for `bit ci pr` (global). */ + snapIds?: string[]; }) { // The post-export cleanup switches the workspace back to main, which re-checks-out main's HEAD // and re-imports every workspace component β€” pointless when the workspace is about to be @@ -892,7 +690,13 @@ export class CiMain { const laneId = await this.lanes.parseLaneId(laneIdStr); - await this.verifyWorkspaceStatusInternal(strict, { scopeToPendingComponents: true }); + const resolvedSnapIds = snapIds ? await this.workspace.resolveMultipleComponentIds(snapIds) : undefined; + if (resolvedSnapIds && !resolvedSnapIds.length) { + this.logger.console(chalk.yellow('No git-authored changes to snap (only dependency-context drift is pending)')); + return 'No changes detected, nothing to snap'; + } + + await this.verifyWorkspaceStatusInternal(strict, { snapIds: resolvedSnapIds }); await this.importer .import({ @@ -920,6 +724,7 @@ export class CiMain { skipCleanup: resolvedSkipCleanup, skipTasks: resolvedSkipTasks, noDestructiveRecovery, + snapIds: resolvedSnapIds, }); } return this.snapAndExportWithTempLane({ @@ -930,6 +735,7 @@ export class CiMain { dryRun, skipCleanup: resolvedSkipCleanup, skipTasks: resolvedSkipTasks, + snapIds: resolvedSnapIds, }); } @@ -992,6 +798,7 @@ export class CiMain { skipCleanup, skipTasks, noDestructiveRecovery, + snapIds, }: { laneId: LaneId; originalLane: Lane | undefined; @@ -1001,6 +808,7 @@ export class CiMain { skipCleanup: boolean; skipTasks?: string; noDestructiveRecovery?: boolean; + snapIds?: ComponentID[]; }) { // Query the remote (by name, to avoid fetching all lanes) so we know whether to reuse or create const existingLanes = await this.lanes.getLanes({ remote: laneId.scope, name: laneId.name }).catch((e) => { @@ -1163,6 +971,7 @@ export class CiMain { build, exitOnFirstFailedTask: true, skipTasks, + legacyBitIds: snapIds ? ComponentIdList.fromArray(snapIds) : undefined, }); if (!results) { @@ -1209,6 +1018,7 @@ export class CiMain { dryRun, skipCleanup, skipTasks, + snapIds, }: { laneId: LaneId; originalLane: Lane | undefined; @@ -1217,6 +1027,7 @@ export class CiMain { dryRun?: boolean; skipCleanup: boolean; skipTasks?: string; + snapIds?: ComponentID[]; }) { // Use unique temp lane name to avoid race conditions when multiple CI jobs run concurrently const tempLaneName = `${laneId.name}-${generateRandomStr(5)}`; @@ -1248,6 +1059,7 @@ export class CiMain { build, exitOnFirstFailedTask: true, skipTasks, + legacyBitIds: snapIds ? ComponentIdList.fromArray(snapIds) : undefined, }); if (!results) { @@ -1676,7 +1488,7 @@ export class CiMain { ); } - const { status } = await this.verifyWorkspaceStatusInternal(strict, { scopeToPendingComponents: true }); + const { status } = await this.verifyWorkspaceStatusInternal(strict); const hasSoftTaggedComponents = status.softTaggedComponents.length > 0; diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts new file mode 100644 index 000000000000..8fa6158d9c82 --- /dev/null +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -0,0 +1,50 @@ +import chalk from 'chalk'; +import type { ComponentID } from '@teambit/component-id'; +import type { Workspace } from '@teambit/workspace'; +import type { Logger } from '@teambit/logger'; +import { classifyPayloadDiff } from './context-drift'; + +export type ContextDriftReport = { + /** dep-only diff vs the recorded version β€” never snapped by a lane run */ + drift: { id: ComponentID; recordedBitVersion?: string; changedKeys: string[] }[]; + /** pending minus drift: new components and file/config-diff components */ + gitAuthored: ComponentID[]; +}; + +/** + * Split the tag-pending set into git-authored changes and dependency-context drift. + * Drift = the diff against the recorded version is confined to dependency data; on a + * pristine checkout that means git did not touch the component β€” the resolution + * context (env template of the pinned engine, root policy) moved instead. + */ +export async function detectContextDrift(workspace: Workspace, logger: Logger): Promise { + const pending = await workspace.listTagPendingIds(); + const legacyScope = workspace.scope.legacyScope; + const repo = legacyScope.objects; + const drift: ContextDriftReport['drift'] = []; + const gitAuthored: ComponentID[] = []; + for (const id of pending) { + if (!id.hasVersion()) { + gitAuthored.push(id); // new component: git-authored by definition + continue; + } + try { + const modelComponent = await legacyScope.getModelComponent(id); + const recorded = await modelComponent.loadVersion(id.version as string, repo); + const comp = await workspace.get(id); + const consumerComp = comp.state._consumer.clone(); + consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified + const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); + // Version.id() serializes to a JSON string (used for hashing) β€” parse both sides so the pure + // helper gets plain objects. + const { depOnly, changedKeys } = classifyPayloadDiff(JSON.parse(recorded.id()), JSON.parse(fromFs.id())); + if (depOnly) drift.push({ id, recordedBitVersion: recorded.bitVersion, changedKeys }); + else gitAuthored.push(id); + } catch (e: any) { + // best-effort per component: an unreadable model must not kill the run β€” treat as git-authored + logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: drift check skipped (${e?.message || e})`)); + gitAuthored.push(id); + } + } + return { drift, gitAuthored }; +} diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 4be0260411b0..056fa99f49bd 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -766,10 +766,28 @@ export class LaneSyncExecutor { * stale-lane recovery (delete + re-fork the remote lane) into a throw. The lane object must be * imported BEFORE delegating: a switch onto the lane the workspace is already on no-ops before any * fetch, so it never warms a cold scope. + * + * Pending components are split into git-authored changes and dependency-context drift (a recorded + * dep range moved under the workspace's current resolution context, not under a dev's commit) before + * snapping: only the git-authored subset is passed as `snapIds`, so drift is never swept into a lane + * snap it never touched. Main-side convergence consumes drift separately (not this run's job). */ private async snapAndExportOntoLane(laneIdStr: string, message: string): Promise { try { await ensureCurrentLaneObject(this.deps.lanes); + const { drift, gitAuthored } = await this.deps.ci.detectContextDrift(); + if (drift.length) { + const running = this.deps.ci.getRunningBitVersion(); + const recorded = [...new Set(drift.map((d) => d.recordedBitVersion).filter(Boolean))].join(', '); + this.deps.logger.console( + `${drift.length} component(s) carry dependency-context drift` + + `${recorded ? ` (recorded with bit ${recorded}, running bit ${running})` : ''} β€” ` + + `main convergence consumes this; not snapped here:` + ); + drift.forEach((d) => + this.deps.logger.console(` ${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})`) + ); + } await this.deps.ci.snapPrCommit({ laneIdStr, message, @@ -778,6 +796,7 @@ export class LaneSyncExecutor { keepLane: true, skipCleanup: true, noDestructiveRecovery: true, + snapIds: gitAuthored.map((id) => id.toStringWithoutVersion()), }); return undefined; } catch (e: any) { diff --git a/scopes/git/ci/sync/main-config-sync.ts b/scopes/git/ci/sync/main-config-sync.ts new file mode 100644 index 000000000000..e1ba2d64e83e --- /dev/null +++ b/scopes/git/ci/sync/main-config-sync.ts @@ -0,0 +1,246 @@ +import chalk from 'chalk'; +import { compact } from 'lodash'; +import type { ComponentID } from '@teambit/component-id'; +import type { Workspace } from '@teambit/workspace'; +import type { LanesMain } from '@teambit/lanes'; +import type { ImporterMain } from '@teambit/importer'; +import type { Logger } from '@teambit/logger'; +import type { LaneId } from '@teambit/lane-id'; +import type { MergeStrategy } from '@teambit/component.modules.merge-helper'; +import { getDivergeData } from '@teambit/component.snap-distance'; +import { ComponentConfigMerger } from '@teambit/config-merger'; +import { DependencyResolverAspect } from '@teambit/dependency-resolver'; +import { pMapPool } from '@teambit/toolbox.promise.map-pool'; +import { concurrentComponentsLimit } from '@teambit/harmony.modules.concurrency'; + +export type MainConfigSyncDeps = { + workspace: Workspace; + lanes: LanesMain; + importer: ImporterMain; + logger: Logger; +}; + +/** + * Batch-fetch main-side Version objects the config merge needs (heads, then common ancestors), + * mirroring the lane-merge flows (merge-status-provider / merge-lanes). Best-effort: a fetch + * hiccup shouldn't abort `bit ci pr` β€” the merge loop still runs and any component whose object is + * still missing just logs the existing per-component skip. `label` names which objects for the log. + */ +async function prefetchFromMainForConfigSync( + { importer, logger }: MainConfigSyncDeps, + ids: ComponentID[], + label: string +) { + if (!ids.length) return; + try { + await importer.importObjectsFromMainIfExist(ids, { cache: true }); + } catch (e: any) { + logger.console( + chalk.yellow(`Could not pre-fetch main's ${label} for config sync (continuing): ${e?.message || e}`) + ); + } +} + +/** + * Copied from `merging.main.runtime` (`filterDeletedDependenciesFromConfig`): the config merge + * can emit deletion markers (`version: '-'`) for deps removed on main. The aspects-merger applies + * `mergedConfig` verbatim, so strip those here to avoid writing a policy entry with version '-'. + */ +function filterDeletedDependenciesFromConfig(mergeConfig?: Record): void { + const policy: Record> | undefined = + mergeConfig?.[DependencyResolverAspect.id]?.policy; + if (!policy) return; + Object.keys(policy).forEach((depType) => { + const filtered = policy[depType].filter((dep) => dep.version !== '-'); + if (filtered.length === 0) delete policy[depType]; + else policy[depType] = filtered; + }); +} + +/** + * Sync *config-only* changes from main onto the lane β€” without a full `bit lane merge`. + * + * In this workflow git is the source of truth for files: the PR author merges the default branch + * into their PR branch, so source changes arrive via git. The one thing git can't carry is + * config that's already been *tagged into objects* on main β€” e.g. another PR ran `bit env set` / + * `bit deps set`; those records lived in `.bitmap`, rode git into main, and `bit ci merge` baked + * them into the component's Version (clearing them from `.bitmap`). A long-running PR's lane + * would otherwise miss them. + * + * A full lane merge is the wrong tool here: it does a 3-way *file* merge and refuses to run while + * the workspace has modified components β€” but in `bit ci pr` the workspace is always dirty (the + * PR's changes, not yet snapped). So instead we do a per-component 3-way merge of the aspect + * *config only* (base = common ancestor, ours = lane, theirs = main), keeping the PR's config on + * conflict, and stash the result on an `unmergedComponents` entry's `mergedConfig`. The + * subsequent `snap` reads it (via the aspects-merger on component load) and bakes main's config + * into the new snap, while the snap's files still come from the workspace (git). No file + * checkout, so no clean-workspace requirement. + */ +export async function syncConfigFromMain(deps: MainConfigSyncDeps, laneId: LaneId) { + const { workspace, lanes, logger } = deps; + const legacyScope = workspace.scope.legacyScope; + const repo = legacyScope.objects; + const mainLaneId = lanes.getDefaultLaneId(); + const currentLane = await lanes.getCurrentLane(); + if (!currentLane) return; + const workspaceIds = workspace.listIds(); + + logger.console(chalk.blue(`Syncing config changes from ${mainLaneId.toString()} into ${laneId.toString()}`)); + + // Resolve each lane component's head on main once, keeping only those that are on main and whose + // lane head differs from it (the rest have nothing to sync). This single pass feeds both the + // pre-fetch below and the merge loop, so we never load the same ModelComponent twice. + const componentsToSync = compact( + await Promise.all( + currentLane.components.map(async (laneComp) => { + try { + const modelComponent = await legacyScope.getModelComponentIfExist(laneComp.id); + const mainHead = modelComponent?.head; // the component's head on main + if (!modelComponent || !mainHead || mainHead.isEqual(laneComp.head)) return undefined; + return { laneComp, modelComponent, mainHead }; + } catch (e: any) { + // Best-effort per component (same contract as the merge loop below): one component's + // load failure shouldn't reject Promise.all and abort the whole config sync. + logger.console( + chalk.yellow( + ` ${laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})` + ) + ); + return undefined; + } + }) + ) + ); + + // The lane import (switchToLane) brought each component's lane history plus the lightweight + // version-history (the parent graph) β€” that's enough for the diverge check below to see that + // main is ahead β€” but NOT the full Version object for main's head wherever main advanced past + // the lane's fork point. Those objects live only on main and were never fetched. Without them + // `loadVersion(mainHead)` throws VersionNotFoundOnFS, the per-component catch swallows it as + // "skipping config sync from main", and the sync silently degrades to a no-op for every + // diverged component. Pre-fetch main's head objects in one batched remote call (mirroring the + // lane-merge flows β€” see merge-status-provider / merge-lanes). Pass the *specific* main-head + // version so `cache: true` still fetches it: the component already exists locally at its lane + // version, so a version-less id would look satisfied and skip the remote. + const mainHeadIds = componentsToSync.map(({ laneComp, mainHead }) => laneComp.id.changeVersion(mainHead.toString())); + await prefetchFromMainForConfigSync(deps, mainHeadIds, 'head objects'); + + // Resolve each component's diverge state up front β€” before the merge loop β€” so we can also + // pre-fetch the common-ancestor objects below. getDivergeData only walks the parent graph, + // which is already local (switchToLane brings the version-history, and the head pre-fetch above + // reinforced it), so no full Version object is needed yet. Keep only components where main is + // actually ahead or diverged; the rest have nothing to bring in from main. Bound the fan-out + // (getDivergeData traverses each component's version graph) so a lane with many components + // doesn't spawn one unbounded burst of concurrent graph walks. + const componentsToMerge = compact( + await pMapPool( + componentsToSync, + async (item) => { + try { + const divergeData = await getDivergeData({ + repo, + modelComponent: item.modelComponent, + sourceHead: item.laneComp.head, + targetHead: item.mainHead, + throws: false, + }); + if (!divergeData.isTargetAhead() && !divergeData.isDiverged()) return undefined; + return { ...item, divergeData }; + } catch (e: any) { + // Best-effort per component (same contract as the merge loop below). + logger.console( + chalk.yellow( + ` ${item.laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})` + ) + ); + return undefined; + } + }, + { concurrency: concurrentComponentsLimit() } + ) + ); + + // The head pre-fetch above brought main's head Version plus the version-history (parent graph), + // but NOT the full Version object of the common ancestor (the lane's fork point) for components + // where the lane and main have BOTH snapped since the fork. The 3-way config merge below loads + // that base Version (`baseVersion.extensions`); without it, `loadVersion(baseSnap)` throws + // VersionNotFoundOnFS, the per-component catch swallows it as "skipping config sync from main", + // and the sync silently no-ops for every diverged component. The fork point lives on main and, + // like the head, was never fetched (includeVersionHistory carries the graph, not each ancestor's + // Version). Batch-fetch the bases in one call β€” same best-effort contract as the head pre-fetch. + const baseIds = compact( + componentsToMerge.map(({ laneComp, divergeData }) => { + const baseSnap = divergeData.commonSnapBeforeDiverge; + return baseSnap ? laneComp.id.changeVersion(baseSnap.toString()) : undefined; + }) + ); + await prefetchFromMainForConfigSync(deps, baseIds, 'common-ancestor objects'); + + const syncedIds: ComponentID[] = []; + for (const { laneComp, modelComponent, mainHead, divergeData } of componentsToMerge) { + try { + const laneHead = laneComp.head; + const currentVersion = await modelComponent.loadVersion(laneHead.toString(), repo); + const otherVersion = await modelComponent.loadVersion(mainHead.toString(), repo); + // base = common ancestor. When the lane is strictly behind main (no divergence) the common + // ancestor IS the lane head, so the lane's own aspects serve as the base. + const baseSnap = divergeData.commonSnapBeforeDiverge; + const baseVersion = baseSnap ? await modelComponent.loadVersion(baseSnap.toString(), repo) : currentVersion; + + const configMerger = new ComponentConfigMerger( + laneComp.id.toStringWithoutVersion(), + workspaceIds, + undefined, // merging from main (the default lane) β€” there's no Lane object for it + currentVersion.extensions, + baseVersion.extensions, + otherVersion.extensions, + laneId.toString(), + mainLaneId.toString(), + logger, + 'ours' as MergeStrategy // keep the PR's config on a genuine conflict + ); + const mergedConfig = configMerger.merge().getSuccessfullyMergedConfig(); + if (!mergedConfig || !Object.keys(mergedConfig).length) continue; + + // Strip dependency deletion markers (version: '-'); the aspects-merger applies mergedConfig + // as-is, so a leftover '-' would land in the policy. + filterDeletedDependenciesFromConfig(mergedConfig); + + // Upsert: addEntry throws if an entry for this component already exists. A prior + // --keep-lane run that crashed mid-snap (or otherwise left unmerged.json entries behind) + // would otherwise make every later run throw here, skip the component, and keep serving + // stale config. Remove any existing entry first so repeated runs converge on main's latest. + legacyScope.objects.unmergedComponents.removeComponent(laneComp.id); + legacyScope.objects.unmergedComponents.addEntry({ + id: { scope: laneComp.id.scope, name: laneComp.id.fullName }, + head: mainHead, + laneId: mainLaneId, + mergedConfig, + }); + syncedIds.push(laneComp.id); + logger.console( + chalk.blue( + ` ${laneComp.id.toStringWithoutVersion()}: applying main's config (${Object.keys(mergedConfig).join(', ')})` + ) + ); + } catch (e: any) { + // Best-effort per component: one component's config-merge quirk shouldn't abort the whole + // `bit ci pr`. Log and move on β€” the build just won't reflect that component's main-side + // config this run. + logger.console( + chalk.yellow(` ${laneComp.id.toStringWithoutVersion()}: skipping config sync from main (${e?.message || e})`) + ); + } + } + + if (!syncedIds.length) { + logger.console(chalk.blue('No config changes from main to sync')); + return; + } + await legacyScope.objects.unmergedComponents.write(); + // The components were already loaded (and their aspects cached) earlier in this run, before the + // unmergedComponents entries existed. Clear the cache so the upcoming `snap` reloads them and + // the aspects-merger folds in the synced `mergedConfig`. + workspace.clearAllComponentsCache(); + logger.console(chalk.green(`Synced config from main for ${syncedIds.length} component(s)`)); +} From 6ac980a941b47387a15ebe69197db71814e63920 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 15:18:21 -0400 Subject: [PATCH 04/22] fix(ci): exclude local-only components from dependency-context drift detection legacyBitIds bypasses Snapping own local-only filtering; the detector must subtract workspace.filter.byLocalOnly itself or a local-only dev edit gets snapped via snapIds and then fails at export. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/sync/context-drift-detector.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 8fa6158d9c82..6674b235bd2a 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -1,4 +1,5 @@ import chalk from 'chalk'; +import { ComponentIdList } from '@teambit/component-id'; import type { ComponentID } from '@teambit/component-id'; import type { Workspace } from '@teambit/workspace'; import type { Logger } from '@teambit/logger'; @@ -18,7 +19,12 @@ export type ContextDriftReport = { * context (env template of the pinned engine, root policy) moved instead. */ export async function detectContextDrift(workspace: Workspace, logger: Logger): Promise { - const pending = await workspace.listTagPendingIds(); + const pendingIds = await workspace.listTagPendingIds(); + // Local-only components are excluded from the pending set everywhere a snap would run (mirrors + // Snapping.getTagPendingComponentsIds) β€” `export` refuses them, and a bare `legacyBitIds` snap + // (this run's `snapIds` path) skips the pending-list computation that normally does this filtering. + const localOnly = ComponentIdList.fromArray(workspace.filter.byLocalOnly(pendingIds)); + const pending = pendingIds.filter((id) => !localOnly.hasWithoutVersion(id)); const legacyScope = workspace.scope.legacyScope; const repo = legacyScope.objects; const drift: ContextDriftReport['drift'] = []; From 9af6dff254183972af128738189e0e1b2f29f6b3 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 15:41:29 -0400 Subject: [PATCH 05/22] feat(ci): main sync converges dependency-context drift with a patch tag Adds convergeContextDrift on CiMain: tags exactly the drifted set with an explicit id list, patch release, ignoreIssues scoped to blockers already on the recorded heads, then exports. Wired into syncMain between the checkoutByCLIValues step and driftFiles() so the .bitmap/lockfile bump rides the existing commit + bit-sync/main flow. Dry-run detects and reports but tags nothing. --- e2e/harmony/ci-sync.e2e.ts | 56 ++++++++++++++++++++++++ scopes/git/ci/ci.main.runtime.ts | 46 +++++++++++++++++++ scopes/git/ci/sync/main-sync-executor.ts | 8 ++++ 3 files changed, 110 insertions(+) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 7236750b0b61..e9d8318e6ac0 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1203,6 +1203,62 @@ describe('bit ci sync', function () { }); }); + // Convergence consumes the drift on main: one patch tag, exported, .bitmap bump riding the + // bit-sync/main flow. The circular pair also drifts, so the tag must tolerate the blocker that + // already exists on the recorded heads (it was tagged with --ignore-issues originally). + describe('main reconcile converges dependency-context drift', () => { + const SYNC_BRANCH = 'bit-sync/main'; + let defaultBranch: string; + + before(() => { + ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); + helper.fs.outputFile('comp2/index.js', `require('is-odd');\nmodule.exports = () => 'comp2: with-pkg';\n`); + helper.fs.outputFile('comp3/index.js', `require('is-odd');\nrequire('@${helper.scopes.remote}/comp4');`); + helper.fs.outputFile('comp4/index.js', `require('@${helper.scopes.remote}/comp3');`); + helper.command.addComponent('comp3'); + helper.command.addComponent('comp4'); + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '1.0.0' } }); + helper.command.install(); + helper.command.tagAllWithoutBuild('--ignore-issues="CircularDependencies"'); + helper.command.export(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "record deps under is-odd 1.0.0"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); + // Task 2 finding: a bare workspace.jsonc edit is invisible to a running process β€” only a real + // `install()` re-run actually moves what gets resolved from disk (node_modules/lockfile). + helper.command.install(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "bump is-odd policy"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + }); + + it('dry-run reports the convergence and tags nothing', () => { + const { output, exitCode } = syncRun('--main --dry-run'); + expect(exitCode, output).to.equal(0); + expect(output).to.include('dependency-context drift'); + expect(output).to.include('dry-run'); + const list = helper.command.listRemoteScopeParsed(); + const comp2 = list.find((c: any) => c.id.includes('comp2')); + // comp2 was already recorded at 0.0.2 by the setup's own tag (is-odd 1.0.0) β€” the dry-run's + // job is to NOT advance it any further, not to leave it below 0.0.2. + expect(comp2.localVersion || comp2.currentVersion).to.equal('0.0.2'); + }); + + it('converges: one patch tag with the alignment message, exported, .bitmap bump on the sync branch', () => { + const { output, exitCode } = syncRun('--main'); + expect(exitCode, output).to.equal(0); + expect(output).to.include('align dependency context'); + expect(fileOnBranch(SYNC_BRANCH, '.bitmap')).to.include('0.0.2'); + }); + + it('the next run finds a converged pair and no-ops', () => { + const { output, exitCode } = syncRun('--main'); + expect(exitCode, output).to.equal(0); + expect(output).to.match(/converged/i); + }); + }); + describe('a stale bit-sync/main that conflicts with the default branch', () => { const SYNC_BRANCH = 'bit-sync/main'; let defaultBranch: string; diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index 51a05bda1e50..e191f0b0b3ee 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -38,6 +38,7 @@ import { adoptAndRetrySwitch, isLaneMissingComponentError } from './sync/adopt-l import { getBitVersion } from '@teambit/bit.get-bit-version'; import { detectContextDrift as detectContextDriftImpl } from './sync/context-drift-detector'; import type { ContextDriftReport } from './sync/context-drift-detector'; +import { convergenceMessage, blockerNamesUnion } from './sync/context-drift'; import { syncConfigFromMain as syncConfigFromMainImpl } from './sync/main-config-sync'; export type CiSwitchLaneOptions = SwitchLaneOptions & { @@ -611,6 +612,51 @@ export class CiMain { return detectContextDriftImpl(this.workspace, this.logger); } + /** + * Consume dependency-context drift on main: one patch tag of exactly the drifted set, + * tolerating only blockers that already exist on the recorded heads, then export. + * The .bitmap/lockfile updates are left in the working tree for the caller's + * mainSync commit flow to pick up. + */ + async convergeContextDrift({ dryRun }: { dryRun?: boolean } = {}): Promise<{ converged: number; summary: string }> { + const { drift } = await this.detectContextDrift(); + if (!drift.length) return { converged: 0, summary: 'no dependency-context drift' }; + const running = this.getRunningBitVersion(); + this.logger.console(chalk.blue(`${drift.length} component(s) carry dependency-context drift:`)); + drift.forEach((d) => + this.logger.console( + ` ${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})` + + `${d.recordedBitVersion && d.recordedBitVersion !== running ? ` recorded with bit ${d.recordedBitVersion}` : ''}` + ) + ); + const idStrs = drift.map((d) => d.id.toStringWithoutVersion()); + if (dryRun) { + return { converged: 0, summary: `dry-run: would converge ${drift.length} component(s)` }; + } + const message = convergenceMessage( + drift.map((d) => d.recordedBitVersion), + running + ); + const status = await this.status.status({ lanes: true }); + const ignoreIssues = blockerNamesUnion(status.componentsWithIssues, new Set(idStrs)); + const results = await this.snapping.tag({ + ids: idStrs, + message, + releaseType: 'patch', + autoTagReleaseType: 'patch', + ignoreIssues, + build: undefined, + persist: false, + failFast: true, + }); + if (!results) return { converged: 0, summary: 'no dependency-context drift' }; + this.logger.console(chalk.blue(message)); + await this.exporter.export(); + const count = results.taggedComponents.length; + this.logger.console(chalk.green(`Converged ${count} component(s)`)); + return { converged: count, summary: `converged ${count} component(s)` }; + } + async verifyWorkspaceStatus() { await this.verifyWorkspaceStatusInternal(); diff --git a/scopes/git/ci/sync/main-sync-executor.ts b/scopes/git/ci/sync/main-sync-executor.ts index 2198ea023ffd..0c70265b1934 100644 --- a/scopes/git/ci/sync/main-sync-executor.ts +++ b/scopes/git/ci/sync/main-sync-executor.ts @@ -129,6 +129,14 @@ export class MainSyncExecutor { ); } + // Consume dependency-context drift before diffing: the tag's .bitmap/lockfile writes then + // ride the same file-diff `driftFiles()` computes below, with no separate commit path. + await this.deps.ci.reloadWorkspaceFromDisk(); + const convergence = await this.deps.ci.convergeContextDrift({ dryRun: opts.dryRun }); + // `converged` alone misses the dry-run case (it never tags, so it's always 0) β€” the no-op + // case is the only one that shouldn't print. + if (convergence.summary !== 'no dependency-context drift') logger.console(convergence.summary); + const drift = await this.driftFiles(); // Direct-push stays bare: asking the host about `mainSyncBranch`'s PR would be the one // interaction with it this mode promises not to make. From 3cd57a87270c960c70dce5de368ee6afcf52b28c Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 16:02:07 -0400 Subject: [PATCH 06/22] fix(ci): distinguish detected-but-not-taggable drift, fix misleading dry-run summary, tighten e2e convergence assertion convergeContextDrift returns an additive detected flag so callers stop string-matching a summary sentinel across the module boundary, and the tag-returned-null anomaly gets its own distinguishable summary instead of being reported as "no dependency-context drift". syncMain dry-run now returns the would-converge line instead of a contradictory converged summary when driftFiles sees no file diff. The main-convergence e2e cell now asserts the actual convergence bump and the real push summary string, instead of a check that was already true before any run. --- e2e/harmony/ci-sync.e2e.ts | 5 ++++- scopes/git/ci/ci.main.runtime.ts | 22 +++++++++++++++++----- scopes/git/ci/sync/main-sync-executor.ts | 11 +++++++---- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index e9d8318e6ac0..4097fb76c136 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1249,7 +1249,10 @@ describe('bit ci sync', function () { const { output, exitCode } = syncRun('--main'); expect(exitCode, output).to.equal(0); expect(output).to.include('align dependency context'); - expect(fileOnBranch(SYNC_BRANCH, '.bitmap')).to.include('0.0.2'); + expect(output).to.include('main -> pushed sync commit to'); + // comp2's own convergence bump (0.0.2 -> 0.0.3) β€” 0.0.2 alone is already true at the fork + // point and would pass whether or not this run converged anything. + expect(fileOnBranch(SYNC_BRANCH, '.bitmap')).to.include('0.0.3'); }); it('the next run finds a converged pair and no-ops', () => { diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index e191f0b0b3ee..0a5d8c76a8c0 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -618,9 +618,13 @@ export class CiMain { * The .bitmap/lockfile updates are left in the working tree for the caller's * mainSync commit flow to pick up. */ - async convergeContextDrift({ dryRun }: { dryRun?: boolean } = {}): Promise<{ converged: number; summary: string }> { + async convergeContextDrift({ dryRun }: { dryRun?: boolean } = {}): Promise<{ + converged: number; + detected: boolean; + summary: string; + }> { const { drift } = await this.detectContextDrift(); - if (!drift.length) return { converged: 0, summary: 'no dependency-context drift' }; + if (!drift.length) return { converged: 0, detected: false, summary: 'no dependency-context drift' }; const running = this.getRunningBitVersion(); this.logger.console(chalk.blue(`${drift.length} component(s) carry dependency-context drift:`)); drift.forEach((d) => @@ -631,7 +635,7 @@ export class CiMain { ); const idStrs = drift.map((d) => d.id.toStringWithoutVersion()); if (dryRun) { - return { converged: 0, summary: `dry-run: would converge ${drift.length} component(s)` }; + return { converged: 0, detected: true, summary: `dry-run: would converge ${drift.length} component(s)` }; } const message = convergenceMessage( drift.map((d) => d.recordedBitVersion), @@ -649,12 +653,20 @@ export class CiMain { persist: false, failFast: true, }); - if (!results) return { converged: 0, summary: 'no dependency-context drift' }; + // Drift was detected but the tag call produced nothing to export β€” detector and tag disagree. + // Distinct from "no dependency-context drift" (drift.length === 0): here `detected` stays true. + if (!results) { + return { + converged: 0, + detected: true, + summary: `drift detected but nothing was taggable (${drift.length} component(s))`, + }; + } this.logger.console(chalk.blue(message)); await this.exporter.export(); const count = results.taggedComponents.length; this.logger.console(chalk.green(`Converged ${count} component(s)`)); - return { converged: count, summary: `converged ${count} component(s)` }; + return { converged: count, detected: true, summary: `converged ${count} component(s)` }; } async verifyWorkspaceStatus() { diff --git a/scopes/git/ci/sync/main-sync-executor.ts b/scopes/git/ci/sync/main-sync-executor.ts index 0c70265b1934..61cf1db665e4 100644 --- a/scopes/git/ci/sync/main-sync-executor.ts +++ b/scopes/git/ci/sync/main-sync-executor.ts @@ -133,14 +133,17 @@ export class MainSyncExecutor { // ride the same file-diff `driftFiles()` computes below, with no separate commit path. await this.deps.ci.reloadWorkspaceFromDisk(); const convergence = await this.deps.ci.convergeContextDrift({ dryRun: opts.dryRun }); - // `converged` alone misses the dry-run case (it never tags, so it's always 0) β€” the no-op - // case is the only one that shouldn't print. - if (convergence.summary !== 'no dependency-context drift') logger.console(convergence.summary); + if (convergence.detected) logger.console(convergence.summary); const drift = await this.driftFiles(); // Direct-push stays bare: asking the host about `mainSyncBranch`'s PR would be the one // interaction with it this mode promises not to make. - if (!drift.length) return directPush ? CONVERGED_SUMMARY : await this.convergedSummary(branch); + if (!drift.length) { + // A dry-run tags nothing, so `driftFiles()` sees no file diff even when convergence was + // detected β€” the CONVERGED summary would contradict the "would converge" line just logged. + if (opts.dryRun && convergence.detected) return `main -> ${convergence.summary}`; + return directPush ? CONVERGED_SUMMARY : await this.convergedSummary(branch); + } logger.console( formatWarningSummary(`main -> drift in ${drift.length} file(s): ${drift.slice(0, 20).join(', ')}`) From 275b65ab8fca62509fbb44027bcc75f1b62d4455 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 16:10:09 -0400 Subject: [PATCH 07/22] test(ci): pin the dry-run summary return value, not just the mid-run log line The mid-run log for detected drift passes with either summary branch syncMain returns through, so round 1s fix to the actual returned would-converge line had no regression coverage until now. --- e2e/harmony/ci-sync.e2e.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 4097fb76c136..19637e17880a 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1238,6 +1238,9 @@ describe('bit ci sync', function () { expect(exitCode, output).to.equal(0); expect(output).to.include('dependency-context drift'); expect(output).to.include('dry-run'); + // Pin the actual returned summary line (not just the mid-run log, which would pass either + // way) β€” the count is left out since it's not the stable part. + expect(output).to.include('main -> dry-run: would converge'); const list = helper.command.listRemoteScopeParsed(); const comp2 = list.find((c: any) => c.id.includes('comp2')); // comp2 was already recorded at 0.0.2 by the setup's own tag (is-odd 1.0.0) β€” the dry-run's From ee688fdc0360dd352f05b719976913d5a0b896bf Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 17:19:19 -0400 Subject: [PATCH 08/22] docs(ci): dependency-context drift and how sync consumes it --- scopes/git/ci/ci.docs.mdx | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/scopes/git/ci/ci.docs.mdx b/scopes/git/ci/ci.docs.mdx index 42177b3c1a03..ee0c116f50b6 100644 --- a/scopes/git/ci/ci.docs.mdx +++ b/scopes/git/ci/ci.docs.mdx @@ -395,6 +395,20 @@ pull-request diff, and a person rejects it: close the pull request. With `mainSy command commits the same drift on the default branch, and uses no sync branch and no pull request. The push is a plain push, so the run stops if the default branch moved during the run. +### Dependency-context drift + +A mirror workspace can show modified components when git has no changes. The +cause is a moved resolution context: the pinned bit engine ships new env +dependency templates, or a committed root policy changes a recorded range. +This is a real dependency change that the repository introduces. + +`bit ci sync` consumes it. A main run tags the drifted components (patch bump) +with the message `align dependency context`, and exports. A lane run never +snaps drifted components; it snaps only the components with git-authored +changes and reports the drift. Pin the engine in `workspace.jsonc` +(`"teambit.harmony/bit": { "engine": "" }`) so the context moves only +when a commit moves it. + ### Git host providers and credentials The command uses plain git for every git operation. The command uses a `GitHostProvider` for every From b71df2b9d9ca6fe4cdecb5c2558c218d512f2389 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 18:07:24 -0400 Subject: [PATCH 09/22] fix(ci): classify overrides as drift, fix stale snapIds after config sync, report auto-snapped drift dependents - add overrides (env-computed dep data) to DRIFT_FIELDS so a real engine bump classifies as drift instead of no-oping the feature; unit case added - recompute drift after syncConfigFromMain and extend snapIds with any newly git-authored ids, so a component the config sync just changed is not missed - report a drifted id that auto-snaps as a dependent of a snapped component, and fix the lane log line's wording to match - fix the detected-but-nothing-taggable dry-run summary, neutral snapIds log wording, and the convergeContextDrift docstring's actual blocker-tolerance behavior - correct the docs' tag message shape and lane auto-snap behavior; add a noop-cell assertion and drop leftover process vocabulary from an e2e comment - trim comment narration added by this branch to ASD-STE100 style Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 51 ++++++++-------- scopes/git/ci/ci.docs.mdx | 11 ++-- scopes/git/ci/ci.main.runtime.ts | 61 ++++++++++++++++---- scopes/git/ci/sync/context-drift-detector.ts | 18 +++--- scopes/git/ci/sync/context-drift.spec.ts | 6 ++ scopes/git/ci/sync/context-drift.ts | 4 ++ scopes/git/ci/sync/lane-sync-executor.ts | 13 +++-- scopes/git/ci/sync/main-sync-executor.ts | 12 ++-- 8 files changed, 116 insertions(+), 60 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 19637e17880a..69e731e55e80 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1120,8 +1120,8 @@ describe('bit ci sync', function () { }); }); - // A real scope carries components with tag blockers (e.g. circular dependencies). The snap only - // includes the lane's pending components, so a blocker on an untouched component must not halt it. + // A real scope carries components with tag blockers (e.g. circular dependencies). The snap + // covers only the lane's pending components. A blocker on an untouched component must not halt it. describe('a snap-blocking issue on a component the lane never touches', () => { const LANE = 'clean-lane'; let defaultBranch: string; @@ -1154,9 +1154,9 @@ describe('bit ci sync', function () { }); }); - // The engine-bump analogue reproducible with one bit binary: the committed root policy moves a - // recorded package range. The lane run must snap only the git-authored change and report the - // drifted component instead of sweeping it into the dev's snap. + // This reproduces an engine bump with one bit binary: a committed root policy moves a recorded + // package range. The lane run snaps only the git-authored change, and reports the drifted + // component instead of sweeping it into the dev's snap. describe('dependency-context drift is excluded from the lane snap', () => { const LANE = 'drift-lane'; let defaultBranch: string; @@ -1172,18 +1172,18 @@ describe('bit ci sync', function () { helper.command.runCmd('git add -A'); helper.command.runCmd('git commit -m "comp2 records is-odd 1.0.0"'); helper.command.runCmd(`git push origin ${defaultBranch}`); - // Lane creation must happen while the policy still matches comp2's recorded range β€” otherwise - // the dev's own (unscoped) `bit snap` would sweep the drift in too, and there'd be nothing left - // for `bit ci sync` to exclude. + // Create the lane while the policy still matches comp2's recorded range. Otherwise the dev's + // own (unscoped) `bit snap` sweeps the drift in too, and leaves nothing for `bit ci sync` to + // exclude. devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); seedSync(LANE); branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); - // The default branch's own resolution context moves AFTER the lane forked β€” the analogue of an - // engine bump: `bit ci sync` boots on the default branch, and a workspace-level policy/engine - // aggregate is resolved once at that boot (mid-run branch checkouts don't re-read it β€” see - // `Workspace._reloadConsumer`, which reloads the consumer/bitmap but not this). So the run's - // *actual* resolution context is whatever is in effect here, regardless of which branch it - // later checks out β€” exactly the drift a real engine bump produces on an untouched component. + // The default branch's resolution context moves after the lane forks β€” the engine-bump + // analogue: `bit ci sync` boots on the default branch and resolves the workspace policy/engine + // aggregate once, at boot. Mid-run branch checkouts do not re-read it (`Workspace._reloadConsumer` + // reloads the consumer and bitmap, not this). The run's resolution context is fixed at boot, + // regardless of which branch it later checks out β€” the same drift a real engine bump produces + // on an untouched component. helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); helper.command.install(); helper.command.runCmd('git add -A'); @@ -1203,9 +1203,9 @@ describe('bit ci sync', function () { }); }); - // Convergence consumes the drift on main: one patch tag, exported, .bitmap bump riding the - // bit-sync/main flow. The circular pair also drifts, so the tag must tolerate the blocker that - // already exists on the recorded heads (it was tagged with --ignore-issues originally). + // Convergence on main consumes the drift: one patch tag, exported, with the .bitmap bump riding + // the bit-sync/main flow. The circular pair also drifts. The tag must tolerate the blocker + // already present on the recorded heads (tagged with --ignore-issues originally). describe('main reconcile converges dependency-context drift', () => { const SYNC_BRANCH = 'bit-sync/main'; let defaultBranch: string; @@ -1225,8 +1225,8 @@ describe('bit ci sync', function () { helper.command.runCmd('git commit -m "record deps under is-odd 1.0.0"'); helper.command.runCmd(`git push origin ${defaultBranch}`); helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); - // Task 2 finding: a bare workspace.jsonc edit is invisible to a running process β€” only a real - // `install()` re-run actually moves what gets resolved from disk (node_modules/lockfile). + // A bare workspace.jsonc edit is invisible to a running process. Only a real `install()` + // re-run moves what gets resolved from disk (node_modules/lockfile). helper.command.install(); helper.command.runCmd('git add -A'); helper.command.runCmd('git commit -m "bump is-odd policy"'); @@ -1238,13 +1238,13 @@ describe('bit ci sync', function () { expect(exitCode, output).to.equal(0); expect(output).to.include('dependency-context drift'); expect(output).to.include('dry-run'); - // Pin the actual returned summary line (not just the mid-run log, which would pass either - // way) β€” the count is left out since it's not the stable part. + // Pin the returned summary line, not just the mid-run log β€” that would pass either way. + // The count is left out; it is not the stable part. expect(output).to.include('main -> dry-run: would converge'); const list = helper.command.listRemoteScopeParsed(); const comp2 = list.find((c: any) => c.id.includes('comp2')); - // comp2 was already recorded at 0.0.2 by the setup's own tag (is-odd 1.0.0) β€” the dry-run's - // job is to NOT advance it any further, not to leave it below 0.0.2. + // comp2 is already recorded at 0.0.2 from the setup's own tag (is-odd 1.0.0). The dry-run + // must not advance it further; it need not leave it below 0.0.2. expect(comp2.localVersion || comp2.currentVersion).to.equal('0.0.2'); }); @@ -1253,8 +1253,8 @@ describe('bit ci sync', function () { expect(exitCode, output).to.equal(0); expect(output).to.include('align dependency context'); expect(output).to.include('main -> pushed sync commit to'); - // comp2's own convergence bump (0.0.2 -> 0.0.3) β€” 0.0.2 alone is already true at the fork - // point and would pass whether or not this run converged anything. + // Checks comp2's convergence bump (0.0.2 -> 0.0.3). 0.0.2 alone is already true at the fork + // point, so it would pass regardless of convergence. expect(fileOnBranch(SYNC_BRANCH, '.bitmap')).to.include('0.0.3'); }); @@ -1262,6 +1262,7 @@ describe('bit ci sync', function () { const { output, exitCode } = syncRun('--main'); expect(exitCode, output).to.equal(0); expect(output).to.match(/converged/i); + expect(output).to.not.include('align dependency context'); }); }); diff --git a/scopes/git/ci/ci.docs.mdx b/scopes/git/ci/ci.docs.mdx index ee0c116f50b6..6f31e3423959 100644 --- a/scopes/git/ci/ci.docs.mdx +++ b/scopes/git/ci/ci.docs.mdx @@ -402,10 +402,13 @@ cause is a moved resolution context: the pinned bit engine ships new env dependency templates, or a committed root policy changes a recorded range. This is a real dependency change that the repository introduces. -`bit ci sync` consumes it. A main run tags the drifted components (patch bump) -with the message `align dependency context`, and exports. A lane run never -snaps drifted components; it snaps only the components with git-authored -changes and reports the drift. Pin the engine in `workspace.jsonc` +`bit ci sync` consumes it. A main run tags the drifted components with a +patch bump and a message of the shape +`chore: align dependency context (recorded with bit X, workspace runs bit Y)`, +then exports. A lane run snaps only the components with git-authored changes +and reports the drift; it does not snap a drifted component directly, but a +drifted component that a snapped component depends on is auto-snapped as +that dependent. Pin the engine in `workspace.jsonc` (`"teambit.harmony/bit": { "engine": "" }`) so the context moves only when a commit moves it. diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index 0a5d8c76a8c0..ef68905a1407 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -437,10 +437,9 @@ export class CiMain { } /** - * `snapIds`: fail only on issues in the components this run actually snaps. A real scope carries - * components with tag blockers (e.g. circular dependencies), and a global failure would block every - * snap in the repo β€” including snaps that never touch the blocked components. The snap itself still - * refuses its own components' blockers. + * `snapIds` scopes the status failure to the components this run snaps. A global failure would + * block every snap in the repo, including one that never touches a blocked component (e.g. a + * circular dependency). The snap itself still enforces blockers on its own components. */ private async verifyWorkspaceStatusInternal(strict: boolean = false, { snapIds }: { snapIds?: ComponentID[] } = {}) { this.logger.console('πŸ“Š Workspace Status'); @@ -613,10 +612,12 @@ export class CiMain { } /** - * Consume dependency-context drift on main: one patch tag of exactly the drifted set, - * tolerating only blockers that already exist on the recorded heads, then export. - * The .bitmap/lockfile updates are left in the working tree for the caller's - * mainSync commit flow to pick up. + * Consume dependency-context drift on main. Tag exactly the drifted set with one patch + * bump, then export. The tag ignores blockers on the drifted components: their files and + * config match the recorded head, so a blocker reflects the recorded content under the + * current context. A blocker type the context itself introduces is tolerated too. The + * .bitmap/lockfile updates stay in the working tree for the caller's mainSync commit flow + * to pick up. */ async convergeContextDrift({ dryRun }: { dryRun?: boolean } = {}): Promise<{ converged: number; @@ -653,8 +654,8 @@ export class CiMain { persist: false, failFast: true, }); - // Drift was detected but the tag call produced nothing to export β€” detector and tag disagree. - // Distinct from "no dependency-context drift" (drift.length === 0): here `detected` stays true. + // The tag call produced nothing, though drift was detected β€” detector and tag disagree. This + // differs from "no dependency-context drift" (drift.length === 0): here `detected` stays true. if (!results) { return { converged: 0, @@ -697,6 +698,7 @@ export class CiMain { skipTasks, noDestructiveRecovery, snapIds, + driftIds, }: { laneIdStr: string; message: string; @@ -716,6 +718,8 @@ export class CiMain { noDestructiveRecovery?: boolean; /** Snap only these ids (no version), not every tag-pending component; unset for `bit ci pr` (global). */ snapIds?: string[]; + /** Ids excluded from `snapIds` as dependency-context drift; used only to report a dependent auto-snap. */ + driftIds?: string[]; }) { // The post-export cleanup switches the workspace back to main, which re-checks-out main's HEAD // and re-imports every workspace component β€” pointless when the workspace is about to be @@ -750,7 +754,9 @@ export class CiMain { const resolvedSnapIds = snapIds ? await this.workspace.resolveMultipleComponentIds(snapIds) : undefined; if (resolvedSnapIds && !resolvedSnapIds.length) { - this.logger.console(chalk.yellow('No git-authored changes to snap (only dependency-context drift is pending)')); + // Neutral wording: this method does not know whether drift caused the empty set or nothing + // was pending at all β€” the caller (e.g. the lane sync executor) reports drift separately. + this.logger.console(chalk.yellow('No git-authored changes to snap')); return 'No changes detected, nothing to snap'; } @@ -783,6 +789,7 @@ export class CiMain { skipTasks: resolvedSkipTasks, noDestructiveRecovery, snapIds: resolvedSnapIds, + driftIds, }); } return this.snapAndExportWithTempLane({ @@ -857,6 +864,7 @@ export class CiMain { skipTasks, noDestructiveRecovery, snapIds, + driftIds, }: { laneId: LaneId; originalLane: Lane | undefined; @@ -867,6 +875,7 @@ export class CiMain { skipTasks?: string; noDestructiveRecovery?: boolean; snapIds?: ComponentID[]; + driftIds?: string[]; }) { // Query the remote (by name, to avoid fetching all lanes) so we know whether to reuse or create const existingLanes = await this.lanes.getLanes({ remote: laneId.scope, name: laneId.name }).catch((e) => { @@ -908,6 +917,15 @@ export class CiMain { ); } else { await this.syncConfigFromMain(laneId); + // `snapIds` was resolved before this call. `syncConfigFromMain` clears the component + // cache, so a component it just re-configured can become git-authored only now β€” add + // any such id, or this run's snap would miss it. Never add a drift id. + if (snapIds) { + const { gitAuthored } = await this.detectContextDrift(); + const known = new Set(snapIds.map((id) => id.toStringWithoutVersion())); + const missing = gitAuthored.filter((id) => !known.has(id.toStringWithoutVersion())); + if (missing.length) snapIds = [...snapIds, ...missing]; + } } } else { // Switch failed even though the remote lane exists. The destructive recovery below @@ -1037,7 +1055,26 @@ export class CiMain { return 'No changes detected, nothing to snap'; } - const { snappedComponents }: SnapResults = results; + const { snappedComponents, autoSnappedResults }: SnapResults = results; + + // A drifted id excluded from `snapIds` can still be auto-snapped, as a dependent of a + // component this run did snap β€” that auto-snap consumes its drift. Report it; the caller + // logged the drift as "not snapped here" before this run knew the outcome. + if (driftIds?.length) { + const driftSet = new Set(driftIds); + const autoSnappedDrift = [ + ...new Set( + autoSnappedResults + .filter((r) => driftSet.has(r.component.id.toStringWithoutVersion())) + .map((r) => r.component.id.toStringWithoutVersion()) + ), + ]; + if (autoSnappedDrift.length) { + this.logger.console( + chalk.blue(`Auto-snapped as a dependent, consuming its drift: ${autoSnappedDrift.join(', ')}`) + ); + } + } const snapOutput = snapResultOutput(results); this.logger.console(snapOutput); diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 6674b235bd2a..474b99d4e762 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -13,16 +13,16 @@ export type ContextDriftReport = { }; /** - * Split the tag-pending set into git-authored changes and dependency-context drift. - * Drift = the diff against the recorded version is confined to dependency data; on a - * pristine checkout that means git did not touch the component β€” the resolution - * context (env template of the pinned engine, root policy) moved instead. + * Split the tag-pending set into git-authored changes and dependency-context drift. Drift means + * the diff against the recorded version is confined to dependency data. On a pristine checkout, + * git did not touch the component; the resolution context (env template of the pinned engine, + * root policy) moved instead. */ export async function detectContextDrift(workspace: Workspace, logger: Logger): Promise { const pendingIds = await workspace.listTagPendingIds(); - // Local-only components are excluded from the pending set everywhere a snap would run (mirrors - // Snapping.getTagPendingComponentsIds) β€” `export` refuses them, and a bare `legacyBitIds` snap - // (this run's `snapIds` path) skips the pending-list computation that normally does this filtering. + // Exclude local-only components, matching every snap path (mirrors + // Snapping.getTagPendingComponentsIds). `export` refuses them, and a bare `legacyBitIds` snap + // (this run's `snapIds` path) skips the pending-list computation that normally filters them out. const localOnly = ComponentIdList.fromArray(workspace.filter.byLocalOnly(pendingIds)); const pending = pendingIds.filter((id) => !localOnly.hasWithoutVersion(id)); const legacyScope = workspace.scope.legacyScope; @@ -41,8 +41,8 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): const consumerComp = comp.state._consumer.clone(); consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); - // Version.id() serializes to a JSON string (used for hashing) β€” parse both sides so the pure - // helper gets plain objects. + // Version.id() serializes to a JSON string for hashing. Parse both sides so the pure helper + // gets plain objects. const { depOnly, changedKeys } = classifyPayloadDiff(JSON.parse(recorded.id()), JSON.parse(fromFs.id())); if (depOnly) drift.push({ id, recordedBitVersion: recorded.bitVersion, changedKeys }); else gitAuthored.push(id); diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index ae8167c4ea91..dfe92129b0c0 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -39,6 +39,12 @@ describe('classifyPayloadDiff', () => { const fromFs = { ...base, extensions: [{ name: 'teambit.envs/envs', config: { env: 'x' } }] }; expect(classifyPayloadDiff(base, fromFs).depOnly).to.equal(false); }); + + it('classifies an overrides-only change (env-computed dep data) as depOnly', () => { + const recorded = { ...base, overrides: { devDependencies: { '@types/react': '^17.0.0' } } }; + const fromFs = { ...base, overrides: { devDependencies: { '@types/react': '^19.0.0' } } }; + expect(classifyPayloadDiff(recorded, fromFs).depOnly).to.equal(true); + }); }); describe('convergenceMessage', () => { diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 35bffcd2598d..bd5e9364a5c3 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -9,6 +9,10 @@ export const DRIFT_FIELDS = [ 'packageDependencies', 'devPackageDependencies', 'peerPackageDependencies', + // env-computed dependency data (force:true env policies, e.g. the core react env's dependency + // template). Keep `extensions` OUT of this list: a git-side policy source that reaches + // `overrides` without also touching `extensions` would otherwise go undetected as drift. + 'overrides', ] as const; // Keys that legitimately differ between a recorded Version and one rebuilt diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 056fa99f49bd..ba70f023f7fb 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -767,10 +767,12 @@ export class LaneSyncExecutor { * imported BEFORE delegating: a switch onto the lane the workspace is already on no-ops before any * fetch, so it never warms a cold scope. * - * Pending components are split into git-authored changes and dependency-context drift (a recorded - * dep range moved under the workspace's current resolution context, not under a dev's commit) before - * snapping: only the git-authored subset is passed as `snapIds`, so drift is never swept into a lane - * snap it never touched. Main-side convergence consumes drift separately (not this run's job). + * Pending components split into git-authored changes and dependency-context drift before snapping + * (a recorded dep range moved under the workspace's current resolution context, not under a dev's + * commit). Only the git-authored subset passes as `snapIds`; drift never rides into a lane snap it + * did not touch directly. A drifted component that a snapped component depends on can still be + * auto-snapped as that dependent β€” `snapPrCommit` reports that case. Main-side convergence consumes + * drift not auto-snapped this way. */ private async snapAndExportOntoLane(laneIdStr: string, message: string): Promise { try { @@ -782,7 +784,7 @@ export class LaneSyncExecutor { this.deps.logger.console( `${drift.length} component(s) carry dependency-context drift` + `${recorded ? ` (recorded with bit ${recorded}, running bit ${running})` : ''} β€” ` + - `main convergence consumes this; not snapped here:` + `not snapped directly by this run (a dependent may auto-snap it):` ); drift.forEach((d) => this.deps.logger.console(` ${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})`) @@ -797,6 +799,7 @@ export class LaneSyncExecutor { skipCleanup: true, noDestructiveRecovery: true, snapIds: gitAuthored.map((id) => id.toStringWithoutVersion()), + driftIds: drift.map((d) => d.id.toStringWithoutVersion()), }); return undefined; } catch (e: any) { diff --git a/scopes/git/ci/sync/main-sync-executor.ts b/scopes/git/ci/sync/main-sync-executor.ts index 61cf1db665e4..2ebb41914f9e 100644 --- a/scopes/git/ci/sync/main-sync-executor.ts +++ b/scopes/git/ci/sync/main-sync-executor.ts @@ -129,8 +129,8 @@ export class MainSyncExecutor { ); } - // Consume dependency-context drift before diffing: the tag's .bitmap/lockfile writes then - // ride the same file-diff `driftFiles()` computes below, with no separate commit path. + // Consume dependency-context drift before diffing. The tag's .bitmap/lockfile writes then + // ride the same file diff `driftFiles()` computes below; there is no separate commit path. await this.deps.ci.reloadWorkspaceFromDisk(); const convergence = await this.deps.ci.convergeContextDrift({ dryRun: opts.dryRun }); if (convergence.detected) logger.console(convergence.summary); @@ -139,9 +139,11 @@ export class MainSyncExecutor { // Direct-push stays bare: asking the host about `mainSyncBranch`'s PR would be the one // interaction with it this mode promises not to make. if (!drift.length) { - // A dry-run tags nothing, so `driftFiles()` sees no file diff even when convergence was - // detected β€” the CONVERGED summary would contradict the "would converge" line just logged. - if (opts.dryRun && convergence.detected) return `main -> ${convergence.summary}`; + // Two cases produce no file diff even though convergence was detected: a dry-run tags + // nothing, and a detected-but-nothing-taggable convergence (converged: 0) exports nothing + // either. Report `convergence.summary` in both, or the CONVERGED summary would contradict + // the line just logged. + if (convergence.detected && (opts.dryRun || !convergence.converged)) return `main -> ${convergence.summary}`; return directPush ? CONVERGED_SUMMARY : await this.convergedSummary(branch); } From c5a397e5e05be7a4a81da773a9ab92812a5ffd52 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 18:24:30 -0400 Subject: [PATCH 10/22] docs(ci): fix inverted auto-snap direction, state the real DRIFT_FIELDS/extensions rationale - auto-snap runs on a component that depends on a snapped one, not the reverse; fix the docs and the lane-sync-executor comment to say so (ci.main.runtime.ts's comment was already correct) - replace the vacuous DRIFT_FIELDS/extensions rationale with the load-bearing case: bit deps set writes both extensions and overrides, so extensions must stay comparable for that change to classify as git-authored Co-Authored-By: Claude Fable 5 --- scopes/git/ci/ci.docs.mdx | 4 ++-- scopes/git/ci/sync/context-drift.ts | 5 +++-- scopes/git/ci/sync/lane-sync-executor.ts | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/scopes/git/ci/ci.docs.mdx b/scopes/git/ci/ci.docs.mdx index 6f31e3423959..c08c808b060a 100644 --- a/scopes/git/ci/ci.docs.mdx +++ b/scopes/git/ci/ci.docs.mdx @@ -407,8 +407,8 @@ patch bump and a message of the shape `chore: align dependency context (recorded with bit X, workspace runs bit Y)`, then exports. A lane run snaps only the components with git-authored changes and reports the drift; it does not snap a drifted component directly, but a -drifted component that a snapped component depends on is auto-snapped as -that dependent. Pin the engine in `workspace.jsonc` +drifted component that depends on a snapped component is auto-snapped as +its dependent. Pin the engine in `workspace.jsonc` (`"teambit.harmony/bit": { "engine": "" }`) so the context moves only when a commit moves it. diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index bd5e9364a5c3..50a965a737c5 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -10,8 +10,9 @@ export const DRIFT_FIELDS = [ 'devPackageDependencies', 'peerPackageDependencies', // env-computed dependency data (force:true env policies, e.g. the core react env's dependency - // template). Keep `extensions` OUT of this list: a git-side policy source that reaches - // `overrides` without also touching `extensions` would otherwise go undetected as drift. + // template). Keep `extensions` out of this list: a git-side `bit deps set` writes both the + // component's aspect config (extensions) and the computed overrides β€” extensions must stay + // comparable so that change classifies as git-authored. 'overrides', ] as const; diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index ba70f023f7fb..f0260d636be2 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -770,8 +770,8 @@ export class LaneSyncExecutor { * Pending components split into git-authored changes and dependency-context drift before snapping * (a recorded dep range moved under the workspace's current resolution context, not under a dev's * commit). Only the git-authored subset passes as `snapIds`; drift never rides into a lane snap it - * did not touch directly. A drifted component that a snapped component depends on can still be - * auto-snapped as that dependent β€” `snapPrCommit` reports that case. Main-side convergence consumes + * did not touch directly. A drifted component that depends on a snapped component can still be + * auto-snapped as its dependent β€” `snapPrCommit` reports that case. Main-side convergence consumes * drift not auto-snapped this way. */ private async snapAndExportOntoLane(laneIdStr: string, message: string): Promise { From ef7036f2b38093bc52d8b91e325b4997f3d69ad6 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 19:04:29 -0400 Subject: [PATCH 11/22] fix(ci): scope drift export to tagged ids, normalize file order, pool drift checks Qodo review fixes for #10574: export() no longer sweeps every staged component, a file-order-only diff no longer misclassifies as drift, the per-component drift check runs with bounded concurrency, and the new drift-report lines use the shared CLI formatting toolkit. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/ci.main.runtime.ts | 46 +++++++++---- scopes/git/ci/sync/context-drift-detector.ts | 69 ++++++++++++------- scopes/git/ci/sync/context-drift.spec.ts | 71 +++++++++++++++++++- scopes/git/ci/sync/context-drift.ts | 18 ++++- scopes/git/ci/sync/lane-sync-executor.ts | 14 ++-- 5 files changed, 173 insertions(+), 45 deletions(-) diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index ef68905a1407..5a22662edbba 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -1,6 +1,14 @@ import type { RuntimeDefinition, SlotRegistry } from '@teambit/harmony'; import { Slot } from '@teambit/harmony'; -import { CLIAspect, type CLIMain, MainRuntime, formatWarningSummary } from '@teambit/cli'; +import { + CLIAspect, + type CLIMain, + MainRuntime, + formatWarningSummary, + formatSuccessSummary, + formatSection, + formatItem, +} from '@teambit/cli'; import { LoggerAspect, type LoggerMain, type Logger } from '@teambit/logger'; import { WorkspaceAspect, type Workspace } from '@teambit/workspace'; import { BuilderAspect, type BuilderMain } from '@teambit/builder'; @@ -612,10 +620,13 @@ export class CiMain { } /** - * Consume dependency-context drift on main. Tag exactly the drifted set with one patch - * bump, then export. The tag ignores blockers on the drifted components: their files and - * config match the recorded head, so a blocker reflects the recorded content under the - * current context. A blocker type the context itself introduces is tolerated too. The + * Consume dependency-context drift on main. The tag seeds exactly the drifted set with one + * patch bump; bit's auto-tag then bumps each drifted component's dependents so their + * recorded dependencies follow. Skipping auto-tag would leave those dependents' recorded + * deps stale, and the next run would re-detect the same drift β€” a convergence cascade that + * never settles. The tag ignores blockers on the drifted components: their files and config + * match the recorded head, so a blocker reflects the recorded content under the current + * context. A blocker type the context itself introduces is tolerated too. The * .bitmap/lockfile updates stay in the working tree for the caller's mainSync commit flow * to pick up. */ @@ -627,11 +638,16 @@ export class CiMain { const { drift } = await this.detectContextDrift(); if (!drift.length) return { converged: 0, detected: false, summary: 'no dependency-context drift' }; const running = this.getRunningBitVersion(); - this.logger.console(chalk.blue(`${drift.length} component(s) carry dependency-context drift:`)); - drift.forEach((d) => - this.logger.console( - ` ${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})` + - `${d.recordedBitVersion && d.recordedBitVersion !== running ? ` recorded with bit ${d.recordedBitVersion}` : ''}` + this.logger.console( + formatSection( + 'dependency-context drift', + '', + drift.map((d) => + formatItem( + `${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})` + + `${d.recordedBitVersion && d.recordedBitVersion !== running ? ` recorded with bit ${d.recordedBitVersion}` : ''}` + ) + ) ) ); const idStrs = drift.map((d) => d.id.toStringWithoutVersion()); @@ -664,9 +680,13 @@ export class CiMain { }; } this.logger.console(chalk.blue(message)); - await this.exporter.export(); + const exportIds = [ + ...results.taggedComponents.map((c) => c.id.toString()), + ...results.autoTaggedResults.map((r) => r.component.id.toString()), + ]; + await this.exporter.export({ ids: exportIds }); const count = results.taggedComponents.length; - this.logger.console(chalk.green(`Converged ${count} component(s)`)); + this.logger.console(formatSuccessSummary(`Converged ${count} component(s)`)); return { converged: count, detected: true, summary: `converged ${count} component(s)` }; } @@ -756,7 +776,7 @@ export class CiMain { if (resolvedSnapIds && !resolvedSnapIds.length) { // Neutral wording: this method does not know whether drift caused the empty set or nothing // was pending at all β€” the caller (e.g. the lane sync executor) reports drift separately. - this.logger.console(chalk.yellow('No git-authored changes to snap')); + this.logger.console(formatWarningSummary('No git-authored changes to snap')); return 'No changes detected, nothing to snap'; } diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 474b99d4e762..9291cf53bc59 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -3,7 +3,13 @@ import { ComponentIdList } from '@teambit/component-id'; import type { ComponentID } from '@teambit/component-id'; import type { Workspace } from '@teambit/workspace'; import type { Logger } from '@teambit/logger'; -import { classifyPayloadDiff } from './context-drift'; +import { pMapPool } from '@teambit/toolbox.promise.map-pool'; +import { concurrentComponentsLimit } from '@teambit/harmony.modules.concurrency'; +import { classifyPayloadDiff, normalizePayload } from './context-drift'; + +type DriftCheckResult = + | { id: ComponentID; kind: 'git-authored' } + | { id: ComponentID; kind: 'drift'; recordedBitVersion?: string; changedKeys: string[] }; export type ContextDriftReport = { /** dep-only diff vs the recorded version β€” never snapped by a lane run */ @@ -27,30 +33,47 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): const pending = pendingIds.filter((id) => !localOnly.hasWithoutVersion(id)); const legacyScope = workspace.scope.legacyScope; const repo = legacyScope.objects; + // Bounded concurrency (same pattern as sync/main-config-sync.ts): each component's check loads + // its recorded Version and rebuilds it from the filesystem, which a large pending set shouldn't + // fire off unbounded. pMapPool preserves input order in its results, so the split below stays + // deterministic regardless of which component's check resolves first. + const results = await pMapPool( + pending, + async (id) => { + if (!id.hasVersion()) { + return { id, kind: 'git-authored' }; // new component: git-authored by definition + } + try { + const modelComponent = await legacyScope.getModelComponent(id); + const recorded = await modelComponent.loadVersion(id.version as string, repo); + const comp = await workspace.get(id); + const consumerComp = comp.state._consumer.clone(); + consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified + const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); + // Version.id() serializes to a JSON string for hashing. Parse both sides so the pure helper + // gets plain objects, then normalize file order β€” the recorded Version was sorted by + // consumer.ts's sortProperties at persist time, but consumerComponentToVersion's output + // here is not, so a pure ordering difference would otherwise misclassify as drift. + const { depOnly, changedKeys } = classifyPayloadDiff( + normalizePayload(JSON.parse(recorded.id())), + normalizePayload(JSON.parse(fromFs.id())) + ); + if (depOnly) return { id, kind: 'drift', recordedBitVersion: recorded.bitVersion, changedKeys }; + return { id, kind: 'git-authored' }; + } catch (e: any) { + // best-effort per component: an unreadable model must not kill the run β€” treat as git-authored + logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: drift check skipped (${e?.message || e})`)); + return { id, kind: 'git-authored' }; + } + }, + { concurrency: concurrentComponentsLimit() } + ); const drift: ContextDriftReport['drift'] = []; const gitAuthored: ComponentID[] = []; - for (const id of pending) { - if (!id.hasVersion()) { - gitAuthored.push(id); // new component: git-authored by definition - continue; - } - try { - const modelComponent = await legacyScope.getModelComponent(id); - const recorded = await modelComponent.loadVersion(id.version as string, repo); - const comp = await workspace.get(id); - const consumerComp = comp.state._consumer.clone(); - consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified - const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); - // Version.id() serializes to a JSON string for hashing. Parse both sides so the pure helper - // gets plain objects. - const { depOnly, changedKeys } = classifyPayloadDiff(JSON.parse(recorded.id()), JSON.parse(fromFs.id())); - if (depOnly) drift.push({ id, recordedBitVersion: recorded.bitVersion, changedKeys }); - else gitAuthored.push(id); - } catch (e: any) { - // best-effort per component: an unreadable model must not kill the run β€” treat as git-authored - logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: drift check skipped (${e?.message || e})`)); - gitAuthored.push(id); - } + for (const r of results) { + if (r.kind === 'drift') + drift.push({ id: r.id, recordedBitVersion: r.recordedBitVersion, changedKeys: r.changedKeys }); + else gitAuthored.push(r.id); } return { drift, gitAuthored }; } diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index dfe92129b0c0..a49be8adec96 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -1,5 +1,5 @@ import { expect } from 'chai'; -import { classifyPayloadDiff, convergenceMessage, blockerNamesUnion } from './context-drift'; +import { classifyPayloadDiff, convergenceMessage, blockerNamesUnion, normalizePayload } from './context-drift'; describe('classifyPayloadDiff', () => { const base = { @@ -45,6 +45,75 @@ describe('classifyPayloadDiff', () => { const fromFs = { ...base, overrides: { devDependencies: { '@types/react': '^19.0.0' } } }; expect(classifyPayloadDiff(recorded, fromFs).depOnly).to.equal(true); }); + + it('normalizing file order before comparing does not mask a real file change', () => { + const recorded = { + ...base, + files: [ + { file: 'aaa', relativePath: 'index.js' }, + { file: 'bbb', relativePath: 'utils.js' }, + ], + }; + const fromFs = { + ...base, + // same files, reversed order, plus a dep change + files: [ + { file: 'bbb', relativePath: 'utils.js' }, + { file: 'aaa', relativePath: 'index.js' }, + ], + packageDependencies: { 'is-odd': '3.0.1' }, + }; + const res = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFs)); + expect(res.depOnly).to.equal(true); + + const fromFsWithFileChange = { + ...fromFs, + files: [ + { file: 'bbb', relativePath: 'utils.js' }, + { file: 'ccc', relativePath: 'index.js' }, + ], + }; + const resWithFileChange = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFsWithFileChange)); + expect(resWithFileChange.depOnly).to.equal(false); + }); +}); + +describe('normalizePayload', () => { + it('sorts files by relativePath', () => { + const payload = { + files: [ + { file: 'bbb', relativePath: 'z.js' }, + { file: 'aaa', relativePath: 'a.js' }, + ], + }; + expect(normalizePayload(payload).files).to.deep.equal([ + { file: 'aaa', relativePath: 'a.js' }, + { file: 'bbb', relativePath: 'z.js' }, + ]); + }); + + it("sorts each file's dists by relativePath when present", () => { + const payload = { + files: [ + { + relativePath: 'a.js', + dists: [ + { relativePath: 'z.js.map', file: 'x' }, + { relativePath: 'a.js.map', file: 'y' }, + ], + }, + ], + }; + expect(normalizePayload(payload).files[0].dists).to.deep.equal([ + { relativePath: 'a.js.map', file: 'y' }, + { relativePath: 'z.js.map', file: 'x' }, + ]); + }); + + it('leaves a payload without a files array untouched', () => { + const payload = { mainFile: 'index.js' }; + expect(normalizePayload(payload)).to.deep.equal(payload); + }); }); describe('convergenceMessage', () => { diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 50a965a737c5..1621470560f0 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -1,4 +1,4 @@ -import { isEqual, omit } from 'lodash'; +import { isEqual, omit, sortBy } from 'lodash'; export const DRIFT_FIELDS = [ 'dependencies', @@ -22,6 +22,22 @@ const VOLATILE_FIELDS = ['log', 'parents', 'squashed', 'origin'] as const; const EXCLUDED = [...DRIFT_FIELDS, ...VOLATILE_FIELDS]; +/** + * Sort a `Version.id()` payload's `files` (and each file's `dists`, if present) by + * `relativePath`. Mirrors `sortProperties` in consumer.ts (the recorded-vs-filesystem + * modified check), so a pure ordering difference does not read as drift or as a + * git-authored change. + */ +export function normalizePayload(payload: Record): Record { + if (!Array.isArray(payload.files)) return payload; + return { + ...payload, + files: sortBy(payload.files, 'relativePath').map((file: Record) => + Array.isArray(file.dists) ? { ...file, dists: sortBy(file.dists, 'relativePath') } : file + ), + }; +} + export function classifyPayloadDiff( recorded: Record, fromFs: Record diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index f0260d636be2..6317020a3f9f 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -1,5 +1,5 @@ import chalk from 'chalk'; -import { formatWarningSummary } from '@teambit/cli'; +import { formatWarningSummary, formatSection, formatItem } from '@teambit/cli'; import type { Logger } from '@teambit/logger'; import type { LanesMain } from '@teambit/lanes'; import type { LaneData } from '@teambit/legacy.scope'; @@ -782,12 +782,12 @@ export class LaneSyncExecutor { const running = this.deps.ci.getRunningBitVersion(); const recorded = [...new Set(drift.map((d) => d.recordedBitVersion).filter(Boolean))].join(', '); this.deps.logger.console( - `${drift.length} component(s) carry dependency-context drift` + - `${recorded ? ` (recorded with bit ${recorded}, running bit ${running})` : ''} β€” ` + - `not snapped directly by this run (a dependent may auto-snap it):` - ); - drift.forEach((d) => - this.deps.logger.console(` ${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})`) + formatSection( + 'dependency-context drift', + `not snapped directly by this run (a dependent may auto-snap it)` + + `${recorded ? ` β€” recorded with bit ${recorded}, running bit ${running}` : ''}`, + drift.map((d) => formatItem(`${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})`)) + ) ); } await this.deps.ci.snapPrCommit({ From 09dc3a5f5a1acf2a314c602bd3ccb992d2733e71 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 19:18:16 -0400 Subject: [PATCH 12/22] fix(ci): strip deprecated file name/test props in drift normalization consumer.isComponentModified aligns these before comparing; normalizePayload must too, or a stale value on an old recorded Version reads as a file change. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/sync/context-drift.spec.ts | 19 +++++++++++++++++++ scopes/git/ci/sync/context-drift.ts | 20 +++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index a49be8adec96..31887eec3285 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -76,6 +76,25 @@ describe('classifyPayloadDiff', () => { const resWithFileChange = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFsWithFileChange)); expect(resWithFileChange.depOnly).to.equal(false); }); + + it('stripping the deprecated name/test file props does not mask a real file change', () => { + const recorded = { + ...base, + files: [{ file: 'aaa', relativePath: 'index.js', name: 'index.js', test: false }], + }; + const fromFs = { + ...base, + // same file content, stale deprecated props, plus a dep change + files: [{ file: 'aaa', relativePath: 'index.js', name: 'old-name.js', test: true }], + packageDependencies: { 'is-odd': '3.0.1' }, + }; + const res = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFs)); + expect(res.depOnly).to.equal(true); + + const fromFsWithFileChange = { ...fromFs, files: [{ ...fromFs.files[0], file: 'ccc' }] }; + const resWithFileChange = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFsWithFileChange)); + expect(resWithFileChange.depOnly).to.equal(false); + }); }); describe('normalizePayload', () => { diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 1621470560f0..d67c0f883680 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -1,5 +1,10 @@ import { isEqual, omit, sortBy } from 'lodash'; +// Deprecated per-file props. consumer.isComponentModified copies them from the model onto the +// filesystem side before comparing, so they must not classify as a file change here either β€” an +// old recorded Version's `name`/`test` can differ from a rebuild for reasons unrelated to drift. +const DEPRECATED_FILE_PROPS = ['name', 'test'] as const; + export const DRIFT_FIELDS = [ 'dependencies', 'devDependencies', @@ -24,17 +29,22 @@ const EXCLUDED = [...DRIFT_FIELDS, ...VOLATILE_FIELDS]; /** * Sort a `Version.id()` payload's `files` (and each file's `dists`, if present) by - * `relativePath`. Mirrors `sortProperties` in consumer.ts (the recorded-vs-filesystem - * modified check), so a pure ordering difference does not read as drift or as a + * `relativePath`, and strip the deprecated `name`/`test` file props. Mirrors `sortProperties` / + * the deprecated-prop alignment in consumer.ts's recorded-vs-filesystem modified check, so + * neither a pure ordering difference nor a stale `name`/`test` value reads as drift or as a * git-authored change. */ export function normalizePayload(payload: Record): Record { if (!Array.isArray(payload.files)) return payload; + const stripDeprecated = (file: Record) => omit(file, DEPRECATED_FILE_PROPS); return { ...payload, - files: sortBy(payload.files, 'relativePath').map((file: Record) => - Array.isArray(file.dists) ? { ...file, dists: sortBy(file.dists, 'relativePath') } : file - ), + files: sortBy(payload.files, 'relativePath').map((file: Record) => { + const stripped = stripDeprecated(file); + return Array.isArray(file.dists) + ? { ...stripped, dists: sortBy(file.dists, 'relativePath').map(stripDeprecated) } + : stripped; + }), }; } From 31ae69b1da35e08b8f522ae2f9e26f9448001a5c Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 19:33:34 -0400 Subject: [PATCH 13/22] fix(ci): re-verify status when config sync expands the snap set An id added after syncConfigFromMain never passed the scoped status gate that ran before the expansion; re-run it over the final set. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/ci.main.runtime.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index 5a22662edbba..df1c2ee8dbad 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -804,6 +804,7 @@ export class CiMain { originalLane, message: resolvedMessage, build, + strict, dryRun, skipCleanup: resolvedSkipCleanup, skipTasks: resolvedSkipTasks, @@ -879,6 +880,7 @@ export class CiMain { originalLane, message, build, + strict, dryRun, skipCleanup, skipTasks, @@ -890,6 +892,7 @@ export class CiMain { originalLane: Lane | undefined; message: string; build: boolean | undefined; + strict: boolean | undefined; dryRun?: boolean; skipCleanup: boolean; skipTasks?: string; @@ -944,7 +947,13 @@ export class CiMain { const { gitAuthored } = await this.detectContextDrift(); const known = new Set(snapIds.map((id) => id.toStringWithoutVersion())); const missing = gitAuthored.filter((id) => !known.has(id.toStringWithoutVersion())); - if (missing.length) snapIds = [...snapIds, ...missing]; + if (missing.length) { + snapIds = [...snapIds, ...missing]; + // The added ids never went through `snapPrCommit`'s scoped verify β€” that ran before + // this expansion, over the pre-expansion set. Re-run it over the final set, or an + // added id's blocker surfaces later at snap instead of at this gate. + await this.verifyWorkspaceStatusInternal(strict, { snapIds }); + } } } } else { From 62d8e76deddd128b33445f34edeed568157c828b Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Thu, 6 Aug 2026 19:44:31 -0400 Subject: [PATCH 14/22] fix(ci): blockerNamesUnion carries only tag-blocker issue names A non-blocker issue on an in-set component leaked its name into the ignore union; scope the union to issues where isTagBlocker is true. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/sync/context-drift.spec.ts | 19 +++++++++++++++---- scopes/git/ci/sync/context-drift.ts | 12 ++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index 31887eec3285..fe17a1cea2ad 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -152,19 +152,30 @@ describe('convergenceMessage', () => { }); describe('blockerNamesUnion', () => { - const entry = (idStr: string, names: string[], blocker: boolean) => ({ + const issue = (name: string, isTagBlocker: boolean) => ({ isTagBlocker, constructor: { name } }); + const entry = (idStr: string, issues: { isTagBlocker: boolean; constructor: { name: string } }[]) => ({ id: { toStringWithoutVersion: () => idStr }, - issues: { getAllIssueNames: () => names, hasTagBlockerIssues: () => blocker }, + issues: { + getAllIssues: () => issues, + hasTagBlockerIssues: () => issues.some((i) => i.isTagBlocker), + }, }); it('unions blocker issue names of in-set components only', () => { const res = blockerNamesUnion( - [entry('s/a', ['CircularDependencies'], true), entry('s/b', ['MissingDists'], true)], + [entry('s/a', [issue('CircularDependencies', true)]), entry('s/b', [issue('MissingDists', true)])], new Set(['s/a']) ); expect(res).to.equal('CircularDependencies'); }); it('returns undefined when no in-set component has blockers', () => { - expect(blockerNamesUnion([entry('s/a', ['X'], false)], new Set(['s/a']))).to.equal(undefined); + expect(blockerNamesUnion([entry('s/a', [issue('X', false)])], new Set(['s/a']))).to.equal(undefined); + }); + it('carries only the blocker issue name, not a non-blocker issue on the same component', () => { + const res = blockerNamesUnion( + [entry('s/a', [issue('CircularDependencies', true), issue('MissingDists', false)])], + new Set(['s/a']) + ); + expect(res).to.equal('CircularDependencies'); }); }); diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index d67c0f883680..846fccf05644 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -69,7 +69,10 @@ export function convergenceMessage(recordedBitVersions: (string | undefined)[], export function blockerNamesUnion( componentsWithIssues: { id: { toStringWithoutVersion(): string }; - issues: { getAllIssueNames(): string[]; hasTagBlockerIssues(): boolean }; + issues: { + getAllIssues(): { isTagBlocker: boolean; constructor: { name: string } }[]; + hasTagBlockerIssues(): boolean; + }; }[], inSet: Set ): string | undefined { @@ -77,7 +80,12 @@ export function blockerNamesUnion( for (const entry of componentsWithIssues) { if (!inSet.has(entry.id.toStringWithoutVersion())) continue; if (!entry.issues.hasTagBlockerIssues()) continue; - entry.issues.getAllIssueNames().forEach((n) => names.add(n)); + // Only the tag-blocker issues need ignoring β€” a non-blocker issue name in `ignoreIssues` is a + // no-op, so the union should carry exactly what it's there to suppress. + entry.issues + .getAllIssues() + .filter((issue) => issue.isTagBlocker) + .forEach((issue) => names.add(issue.constructor.name)); } return names.size ? [...names].join(',') : undefined; } From 558f45153e943bd8d5bd9834cde17ad46aeaa25a Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 11:09:09 -0400 Subject: [PATCH 15/22] docs(ci): trim narration from this branch's comments Comment-only pass over the sync/drift additions: drop process narration and justification chains, keep the load-bearing constraints and traps. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 25 +++++------- scopes/git/ci/ci.main.runtime.ts | 41 +++++++++----------- scopes/git/ci/sync/context-drift-detector.ts | 24 ++++-------- scopes/git/ci/sync/context-drift.ts | 23 +++++------ scopes/git/ci/sync/lane-sync-executor.ts | 10 ++--- scopes/git/ci/sync/main-sync-executor.ts | 7 ++-- 6 files changed, 53 insertions(+), 77 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 69e731e55e80..4caf1d914532 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1172,18 +1172,15 @@ describe('bit ci sync', function () { helper.command.runCmd('git add -A'); helper.command.runCmd('git commit -m "comp2 records is-odd 1.0.0"'); helper.command.runCmd(`git push origin ${defaultBranch}`); - // Create the lane while the policy still matches comp2's recorded range. Otherwise the dev's - // own (unscoped) `bit snap` sweeps the drift in too, and leaves nothing for `bit ci sync` to - // exclude. + // Create the lane before the policy bump, or the dev's own (unscoped) `bit snap` sweeps the + // drift in too, leaving nothing for `bit ci sync` to exclude. devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); seedSync(LANE); branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); - // The default branch's resolution context moves after the lane forks β€” the engine-bump - // analogue: `bit ci sync` boots on the default branch and resolves the workspace policy/engine - // aggregate once, at boot. Mid-run branch checkouts do not re-read it (`Workspace._reloadConsumer` - // reloads the consumer and bitmap, not this). The run's resolution context is fixed at boot, - // regardless of which branch it later checks out β€” the same drift a real engine bump produces - // on an untouched component. + // `bit ci sync` resolves the workspace policy/engine aggregate once, at boot on the default + // branch; `Workspace._reloadConsumer` (used on later branch checkouts) reloads the consumer + // and bitmap but not this. Bumping the policy here after the lane forks is the engine-bump + // analogue: the same drift on an untouched component that a real engine bump produces. helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); helper.command.install(); helper.command.runCmd('git add -A'); @@ -1238,13 +1235,12 @@ describe('bit ci sync', function () { expect(exitCode, output).to.equal(0); expect(output).to.include('dependency-context drift'); expect(output).to.include('dry-run'); - // Pin the returned summary line, not just the mid-run log β€” that would pass either way. - // The count is left out; it is not the stable part. + // Asserts on the returned summary, not just the mid-run log line. expect(output).to.include('main -> dry-run: would converge'); const list = helper.command.listRemoteScopeParsed(); const comp2 = list.find((c: any) => c.id.includes('comp2')); - // comp2 is already recorded at 0.0.2 from the setup's own tag (is-odd 1.0.0). The dry-run - // must not advance it further; it need not leave it below 0.0.2. + // comp2 is already recorded at 0.0.2 from the setup's own tag (is-odd 1.0.0); the dry-run + // must not advance it further. expect(comp2.localVersion || comp2.currentVersion).to.equal('0.0.2'); }); @@ -1253,8 +1249,7 @@ describe('bit ci sync', function () { expect(exitCode, output).to.equal(0); expect(output).to.include('align dependency context'); expect(output).to.include('main -> pushed sync commit to'); - // Checks comp2's convergence bump (0.0.2 -> 0.0.3). 0.0.2 alone is already true at the fork - // point, so it would pass regardless of convergence. + // 0.0.3, not 0.0.2: 0.0.2 is already true at the fork point and would pass regardless. expect(fileOnBranch(SYNC_BRANCH, '.bitmap')).to.include('0.0.3'); }); diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index df1c2ee8dbad..dc7d9355358b 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -445,9 +445,9 @@ export class CiMain { } /** - * `snapIds` scopes the status failure to the components this run snaps. A global failure would - * block every snap in the repo, including one that never touches a blocked component (e.g. a - * circular dependency). The snap itself still enforces blockers on its own components. + * `snapIds` scopes the status failure to the components this run snaps β€” a global failure would + * otherwise block every snap in the repo. The snap itself still enforces blockers on its own + * components. */ private async verifyWorkspaceStatusInternal(strict: boolean = false, { snapIds }: { snapIds?: ComponentID[] } = {}) { this.logger.console('πŸ“Š Workspace Status'); @@ -620,15 +620,12 @@ export class CiMain { } /** - * Consume dependency-context drift on main. The tag seeds exactly the drifted set with one - * patch bump; bit's auto-tag then bumps each drifted component's dependents so their - * recorded dependencies follow. Skipping auto-tag would leave those dependents' recorded - * deps stale, and the next run would re-detect the same drift β€” a convergence cascade that - * never settles. The tag ignores blockers on the drifted components: their files and config - * match the recorded head, so a blocker reflects the recorded content under the current - * context. A blocker type the context itself introduces is tolerated too. The - * .bitmap/lockfile updates stay in the working tree for the caller's mainSync commit flow - * to pick up. + * Consume dependency-context drift on main with one patch tag over the drifted set. Auto-tag + * then carries each drifted component's dependents; skipping it would leave their recorded deps + * stale and the next run would re-detect the same drift. Ignores blockers on the drifted + * components, including ones the context change itself introduces β€” their files and config + * already match the recorded head. Leaves .bitmap/lockfile updates in the working tree for the + * caller's mainSync commit flow. */ async convergeContextDrift({ dryRun }: { dryRun?: boolean } = {}): Promise<{ converged: number; @@ -670,8 +667,8 @@ export class CiMain { persist: false, failFast: true, }); - // The tag call produced nothing, though drift was detected β€” detector and tag disagree. This - // differs from "no dependency-context drift" (drift.length === 0): here `detected` stays true. + // Drift detected but the tag produced nothing (detector and tag disagree); `detected` stays + // true here, unlike the empty-drift case above. if (!results) { return { converged: 0, @@ -774,8 +771,8 @@ export class CiMain { const resolvedSnapIds = snapIds ? await this.workspace.resolveMultipleComponentIds(snapIds) : undefined; if (resolvedSnapIds && !resolvedSnapIds.length) { - // Neutral wording: this method does not know whether drift caused the empty set or nothing - // was pending at all β€” the caller (e.g. the lane sync executor) reports drift separately. + // This method can't tell drift-caused emptiness from nothing pending; the caller (e.g. the + // lane sync executor) reports drift separately. this.logger.console(formatWarningSummary('No git-authored changes to snap')); return 'No changes detected, nothing to snap'; } @@ -940,18 +937,18 @@ export class CiMain { ); } else { await this.syncConfigFromMain(laneId); - // `snapIds` was resolved before this call. `syncConfigFromMain` clears the component - // cache, so a component it just re-configured can become git-authored only now β€” add - // any such id, or this run's snap would miss it. Never add a drift id. + // `syncConfigFromMain` clears the component cache; a component it just reconfigured + // can only now show as git-authored. Add it here, or the snap misses it. Never add a + // drift id. if (snapIds) { const { gitAuthored } = await this.detectContextDrift(); const known = new Set(snapIds.map((id) => id.toStringWithoutVersion())); const missing = gitAuthored.filter((id) => !known.has(id.toStringWithoutVersion())); if (missing.length) { snapIds = [...snapIds, ...missing]; - // The added ids never went through `snapPrCommit`'s scoped verify β€” that ran before - // this expansion, over the pre-expansion set. Re-run it over the final set, or an - // added id's blocker surfaces later at snap instead of at this gate. + // The added ids skipped `snapPrCommit`'s scoped verify, which ran before this + // expansion. Re-verify over the final set, or an added id's blocker surfaces at + // snap instead of here. await this.verifyWorkspaceStatusInternal(strict, { snapIds }); } } diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 9291cf53bc59..fe766b26024b 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -19,24 +19,18 @@ export type ContextDriftReport = { }; /** - * Split the tag-pending set into git-authored changes and dependency-context drift. Drift means - * the diff against the recorded version is confined to dependency data. On a pristine checkout, - * git did not touch the component; the resolution context (env template of the pinned engine, - * root policy) moved instead. + * Split the tag-pending set into git-authored changes and dependency-context drift. Drift = the + * diff against the recorded version is confined to dependency data; files and config are identical. */ export async function detectContextDrift(workspace: Workspace, logger: Logger): Promise { const pendingIds = await workspace.listTagPendingIds(); - // Exclude local-only components, matching every snap path (mirrors - // Snapping.getTagPendingComponentsIds). `export` refuses them, and a bare `legacyBitIds` snap - // (this run's `snapIds` path) skips the pending-list computation that normally filters them out. + // `export` refuses local-only components; a `legacyBitIds` snap bypasses the pending-path filter + // that normally removes them (Snapping.getTagPendingComponentsIds), so filter here. const localOnly = ComponentIdList.fromArray(workspace.filter.byLocalOnly(pendingIds)); const pending = pendingIds.filter((id) => !localOnly.hasWithoutVersion(id)); const legacyScope = workspace.scope.legacyScope; const repo = legacyScope.objects; - // Bounded concurrency (same pattern as sync/main-config-sync.ts): each component's check loads - // its recorded Version and rebuilds it from the filesystem, which a large pending set shouldn't - // fire off unbounded. pMapPool preserves input order in its results, so the split below stays - // deterministic regardless of which component's check resolves first. + // pMapPool preserves input order, keeping the split deterministic. const results = await pMapPool( pending, async (id) => { @@ -50,10 +44,8 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): const consumerComp = comp.state._consumer.clone(); consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); - // Version.id() serializes to a JSON string for hashing. Parse both sides so the pure helper - // gets plain objects, then normalize file order β€” the recorded Version was sorted by - // consumer.ts's sortProperties at persist time, but consumerComponentToVersion's output - // here is not, so a pure ordering difference would otherwise misclassify as drift. + // Version.id() returns a JSON string. Normalization is required on both sides: the recorded + // Version was sorted at persist time (consumer.ts sortProperties); the rebuilt one is not. const { depOnly, changedKeys } = classifyPayloadDiff( normalizePayload(JSON.parse(recorded.id())), normalizePayload(JSON.parse(fromFs.id())) @@ -61,7 +53,7 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): if (depOnly) return { id, kind: 'drift', recordedBitVersion: recorded.bitVersion, changedKeys }; return { id, kind: 'git-authored' }; } catch (e: any) { - // best-effort per component: an unreadable model must not kill the run β€” treat as git-authored + // an unreadable model must not kill the run; degrade to git-authored logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: drift check skipped (${e?.message || e})`)); return { id, kind: 'git-authored' }; } diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 846fccf05644..74790cc08fb0 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -1,8 +1,7 @@ import { isEqual, omit, sortBy } from 'lodash'; -// Deprecated per-file props. consumer.isComponentModified copies them from the model onto the -// filesystem side before comparing, so they must not classify as a file change here either β€” an -// old recorded Version's `name`/`test` can differ from a rebuild for reasons unrelated to drift. +// Deprecated per-file props. consumer.isComponentModified copies them from the model before +// comparing; strip them here too, or a stale `name`/`test` value misclassifies as drift. const DEPRECATED_FILE_PROPS = ['name', 'test'] as const; export const DRIFT_FIELDS = [ @@ -14,10 +13,9 @@ export const DRIFT_FIELDS = [ 'packageDependencies', 'devPackageDependencies', 'peerPackageDependencies', - // env-computed dependency data (force:true env policies, e.g. the core react env's dependency - // template). Keep `extensions` out of this list: a git-side `bit deps set` writes both the - // component's aspect config (extensions) and the computed overrides β€” extensions must stay - // comparable so that change classifies as git-authored. + // env-computed dependency data (force:true env policies). Excludes `extensions`: `bit deps set` + // writes both extensions and overrides, and extensions must stay comparable so that change + // classifies as git-authored. 'overrides', ] as const; @@ -28,11 +26,9 @@ const VOLATILE_FIELDS = ['log', 'parents', 'squashed', 'origin'] as const; const EXCLUDED = [...DRIFT_FIELDS, ...VOLATILE_FIELDS]; /** - * Sort a `Version.id()` payload's `files` (and each file's `dists`, if present) by - * `relativePath`, and strip the deprecated `name`/`test` file props. Mirrors `sortProperties` / - * the deprecated-prop alignment in consumer.ts's recorded-vs-filesystem modified check, so - * neither a pure ordering difference nor a stale `name`/`test` value reads as drift or as a - * git-authored change. + * Sort a `Version.id()` payload's `files` (and each file's `dists`) by `relativePath`, and strip + * the deprecated `name`/`test` props. Mirrors consumer.ts's sortProperties / deprecated-prop + * alignment, so ordering and stale props don't misclassify as drift. */ export function normalizePayload(payload: Record): Record { if (!Array.isArray(payload.files)) return payload; @@ -80,8 +76,7 @@ export function blockerNamesUnion( for (const entry of componentsWithIssues) { if (!inSet.has(entry.id.toStringWithoutVersion())) continue; if (!entry.issues.hasTagBlockerIssues()) continue; - // Only the tag-blocker issues need ignoring β€” a non-blocker issue name in `ignoreIssues` is a - // no-op, so the union should carry exactly what it's there to suppress. + // A non-blocker issue name in `ignoreIssues` is a no-op; only tag-blocker names belong here. entry.issues .getAllIssues() .filter((issue) => issue.isTagBlocker) diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 6317020a3f9f..8ebcbb606392 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -767,12 +767,10 @@ export class LaneSyncExecutor { * imported BEFORE delegating: a switch onto the lane the workspace is already on no-ops before any * fetch, so it never warms a cold scope. * - * Pending components split into git-authored changes and dependency-context drift before snapping - * (a recorded dep range moved under the workspace's current resolution context, not under a dev's - * commit). Only the git-authored subset passes as `snapIds`; drift never rides into a lane snap it - * did not touch directly. A drifted component that depends on a snapped component can still be - * auto-snapped as its dependent β€” `snapPrCommit` reports that case. Main-side convergence consumes - * drift not auto-snapped this way. + * Splits pending components into git-authored changes and dependency-context drift before + * snapping. Only the git-authored subset passes as `snapIds`; drift rides into a lane snap only + * as an auto-snapped dependent of a snapped component (`snapPrCommit` reports that case). + * Main-side convergence consumes drift not auto-snapped this way. */ private async snapAndExportOntoLane(laneIdStr: string, message: string): Promise { try { diff --git a/scopes/git/ci/sync/main-sync-executor.ts b/scopes/git/ci/sync/main-sync-executor.ts index 2ebb41914f9e..5984c536924e 100644 --- a/scopes/git/ci/sync/main-sync-executor.ts +++ b/scopes/git/ci/sync/main-sync-executor.ts @@ -139,10 +139,9 @@ export class MainSyncExecutor { // Direct-push stays bare: asking the host about `mainSyncBranch`'s PR would be the one // interaction with it this mode promises not to make. if (!drift.length) { - // Two cases produce no file diff even though convergence was detected: a dry-run tags - // nothing, and a detected-but-nothing-taggable convergence (converged: 0) exports nothing - // either. Report `convergence.summary` in both, or the CONVERGED summary would contradict - // the line just logged. + // A dry-run, or a detected-but-nothing-taggable convergence (converged: 0), also produces + // no file diff. Return `convergence.summary` for both, or the CONVERGED summary + // contradicts the line just logged. if (convergence.detected && (opts.dryRun || !convergence.converged)) return `main -> ${convergence.summary}`; return directPush ? CONVERGED_SUMMARY : await this.convergedSummary(branch); } From 1a0484dc0b1f05c491eb7286fc78381dab9b2743 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 11:41:31 -0400 Subject: [PATCH 16/22] refactor(ci): drift detector diffs via the diff engine, not a hand-rolled comparator diffBetweenComponentsObjects (verbose) replaces the clone + consumerComponentToVersion + JSON-parse-and-normalize payload comparator. File-content truth is a hash compare, kept independent of the field diff since a files/specs field can fire on unchanged content (deprecated per-file props). recordedBitVersion lookup narrows to drift-classified components only, and degrades to undefined rather than affecting the verdict. Co-Authored-By: Claude Fable 5 --- scopes/git/ci/sync/context-drift-detector.ts | 61 ++++++-- scopes/git/ci/sync/context-drift.spec.ts | 147 ++++--------------- scopes/git/ci/sync/context-drift.ts | 72 ++++----- 3 files changed, 103 insertions(+), 177 deletions(-) diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index fe766b26024b..05fb351b6c29 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -3,9 +3,13 @@ import { ComponentIdList } from '@teambit/component-id'; import type { ComponentID } from '@teambit/component-id'; import type { Workspace } from '@teambit/workspace'; import type { Logger } from '@teambit/logger'; +import type { Scope as LegacyScope } from '@teambit/legacy.scope'; +import type { Repository } from '@teambit/objects'; +import type { SourceFile } from '@teambit/component.sources'; import { pMapPool } from '@teambit/toolbox.promise.map-pool'; import { concurrentComponentsLimit } from '@teambit/harmony.modules.concurrency'; -import { classifyPayloadDiff, normalizePayload } from './context-drift'; +import { diffBetweenComponentsObjects } from '@teambit/legacy.component-diff'; +import { classifyDiffFields } from './context-drift'; type DriftCheckResult = | { id: ComponentID; kind: 'git-authored' } @@ -18,6 +22,37 @@ export type ContextDriftReport = { gitAuthored: ComponentID[]; }; +/** relativePath -> content hash, using the same hashing the model persists files with. */ +function fileHashes(files: SourceFile[]): Map { + return new Map(files.map((file) => [file.relativePath, file.toSourceAsLinuxEOL().hash().hash])); +} + +/** True if any file was added, removed, or its content hash changed. Ignores non-content props. */ +function filesContentChanged(recordedFiles: SourceFile[], workspaceFiles: SourceFile[]): boolean { + const recorded = fileHashes(recordedFiles); + const workspace = fileHashes(workspaceFiles); + if (recorded.size !== workspace.size) return true; + for (const [relativePath, hash] of recorded) { + if (workspace.get(relativePath) !== hash) return true; + } + return false; +} + +/** Attribution only: which bit version recorded this component. A lookup failure must not affect the verdict. */ +async function getRecordedBitVersion( + legacyScope: LegacyScope, + repo: Repository, + id: ComponentID +): Promise { + try { + const modelComponent = await legacyScope.getModelComponent(id); + const recorded = await modelComponent.loadVersion(id.version as string, repo); + return recorded.bitVersion; + } catch { + return undefined; + } +} + /** * Split the tag-pending set into git-authored changes and dependency-context drift. Drift = the * diff against the recorded version is confined to dependency data; files and config are identical. @@ -38,20 +73,18 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): return { id, kind: 'git-authored' }; // new component: git-authored by definition } try { - const modelComponent = await legacyScope.getModelComponent(id); - const recorded = await modelComponent.loadVersion(id.version as string, repo); const comp = await workspace.get(id); - const consumerComp = comp.state._consumer.clone(); - consumerComp.log = recorded.log; // same normalization as consumer.isComponentModified - const { version: fromFs } = await legacyScope.sources.consumerComponentToVersion(consumerComp); - // Version.id() returns a JSON string. Normalization is required on both sides: the recorded - // Version was sorted at persist time (consumer.ts sortProperties); the rebuilt one is not. - const { depOnly, changedKeys } = classifyPayloadDiff( - normalizePayload(JSON.parse(recorded.id())), - normalizePayload(JSON.parse(fromFs.id())) - ); - if (depOnly) return { id, kind: 'drift', recordedBitVersion: recorded.bitVersion, changedKeys }; - return { id, kind: 'git-authored' }; + const consumerComp = comp.state._consumer; + const fromModel = consumerComp.componentFromModel; + if (!fromModel) return { id, kind: 'git-authored' }; // nothing recorded to diff against + const filesChanged = filesContentChanged(fromModel.files, consumerComp.files); + const fieldsDiff = await diffBetweenComponentsObjects(fromModel, consumerComp, { verbose: true }); + const fieldNames = (fieldsDiff ?? []).map((f) => f.fieldName); + const { drift, changedKeys, anomaly } = classifyDiffFields(fieldNames, filesChanged); + if (anomaly) logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: ${anomaly}`)); + if (!drift) return { id, kind: 'git-authored' }; + const recordedBitVersion = await getRecordedBitVersion(legacyScope, repo, id); + return { id, kind: 'drift', recordedBitVersion, changedKeys }; } catch (e: any) { // an unreadable model must not kill the run; degrade to git-authored logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: drift check skipped (${e?.message || e})`)); diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index fe17a1cea2ad..e069aaa95794 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -1,137 +1,44 @@ import { expect } from 'chai'; -import { classifyPayloadDiff, convergenceMessage, blockerNamesUnion, normalizePayload } from './context-drift'; +import { classifyDiffFields, convergenceMessage, blockerNamesUnion } from './context-drift'; -describe('classifyPayloadDiff', () => { - const base = { - files: [{ file: 'aaa', relativePath: 'index.js' }], - mainFile: 'index.js', - packageDependencies: { 'is-odd': '1.0.0' }, - devPackageDependencies: {}, - peerPackageDependencies: {}, - log: { date: '1', username: 'a' }, - }; - - it('classifies a package-range-only change as depOnly', () => { - const fromFs = { ...base, packageDependencies: { 'is-odd': '3.0.1' }, log: { date: '2', username: 'b' } }; - const res = classifyPayloadDiff(base, fromFs); - expect(res.depOnly).to.equal(true); - expect(res.changedKeys).to.deep.equal(['packageDependencies']); - }); - - it('classifies a dev/peer reclassification as depOnly', () => { - const recorded = { ...base, peerDependencies: [{ id: 'scope/link' }], dependencies: [] }; - const fromFs = { ...base, peerDependencies: [], dependencies: [{ id: 'scope/link' }] }; - expect(classifyPayloadDiff(recorded, fromFs).depOnly).to.equal(true); - }); - - it('rejects a file change even when deps also changed', () => { - const fromFs = { - ...base, - files: [{ file: 'bbb', relativePath: 'index.js' }], - packageDependencies: { 'is-odd': '3.0.1' }, - }; - const res = classifyPayloadDiff(base, fromFs); - expect(res.depOnly).to.equal(false); - expect(res.changedKeys).to.include('files'); +describe('classifyDiffFields', () => { + it('classifies a change confined to dependency-ish fields as drift', () => { + const res = classifyDiffFields(['packageDependencies', 'dependencies'], false); + expect(res.drift).to.equal(true); + expect(res.changedKeys).to.deep.equal(['packageDependencies', 'dependencies']); }); - it('rejects an extensions (config) change', () => { - const fromFs = { ...base, extensions: [{ name: 'teambit.envs/envs', config: { env: 'x' } }] }; - expect(classifyPayloadDiff(base, fromFs).depOnly).to.equal(false); + it('classifies overridesDevDependencies alone as drift', () => { + expect(classifyDiffFields(['overridesDevDependencies'], false).drift).to.equal(true); }); - it('classifies an overrides-only change (env-computed dep data) as depOnly', () => { - const recorded = { ...base, overrides: { devDependencies: { '@types/react': '^17.0.0' } } }; - const fromFs = { ...base, overrides: { devDependencies: { '@types/react': '^19.0.0' } } }; - expect(classifyPayloadDiff(recorded, fromFs).depOnly).to.equal(true); + it('rejects overridesPackageJsonProps as a non-dependency field', () => { + expect(classifyDiffFields(['overridesPackageJsonProps'], false).drift).to.equal(false); }); - it('normalizing file order before comparing does not mask a real file change', () => { - const recorded = { - ...base, - files: [ - { file: 'aaa', relativePath: 'index.js' }, - { file: 'bbb', relativePath: 'utils.js' }, - ], - }; - const fromFs = { - ...base, - // same files, reversed order, plus a dep change - files: [ - { file: 'bbb', relativePath: 'utils.js' }, - { file: 'aaa', relativePath: 'index.js' }, - ], - packageDependencies: { 'is-odd': '3.0.1' }, - }; - const res = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFs)); - expect(res.depOnly).to.equal(true); - - const fromFsWithFileChange = { - ...fromFs, - files: [ - { file: 'bbb', relativePath: 'utils.js' }, - { file: 'ccc', relativePath: 'index.js' }, - ], - }; - const resWithFileChange = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFsWithFileChange)); - expect(resWithFileChange.depOnly).to.equal(false); - }); - - it('stripping the deprecated name/test file props does not mask a real file change', () => { - const recorded = { - ...base, - files: [{ file: 'aaa', relativePath: 'index.js', name: 'index.js', test: false }], - }; - const fromFs = { - ...base, - // same file content, stale deprecated props, plus a dep change - files: [{ file: 'aaa', relativePath: 'index.js', name: 'old-name.js', test: true }], - packageDependencies: { 'is-odd': '3.0.1' }, - }; - const res = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFs)); - expect(res.depOnly).to.equal(true); - - const fromFsWithFileChange = { ...fromFs, files: [{ ...fromFs.files[0], file: 'ccc' }] }; - const resWithFileChange = classifyPayloadDiff(normalizePayload(recorded), normalizePayload(fromFsWithFileChange)); - expect(resWithFileChange.depOnly).to.equal(false); + it('rejects an aspect configuration field name', () => { + const res = classifyDiffFields(['teambit.envs/envs configuration'], false); + expect(res.drift).to.equal(false); + expect(res.changedKeys).to.deep.equal(['teambit.envs/envs configuration']); }); -}); -describe('normalizePayload', () => { - it('sorts files by relativePath', () => { - const payload = { - files: [ - { file: 'bbb', relativePath: 'z.js' }, - { file: 'aaa', relativePath: 'a.js' }, - ], - }; - expect(normalizePayload(payload).files).to.deep.equal([ - { file: 'aaa', relativePath: 'a.js' }, - { file: 'bbb', relativePath: 'z.js' }, - ]); + it('rejects any field set once file content changed, regardless of which fields fired', () => { + const res = classifyDiffFields(['packageDependencies'], true); + expect(res.drift).to.equal(false); + expect(res.changedKeys).to.deep.equal(['packageDependencies']); }); - it("sorts each file's dists by relativePath when present", () => { - const payload = { - files: [ - { - relativePath: 'a.js', - dists: [ - { relativePath: 'z.js.map', file: 'x' }, - { relativePath: 'a.js.map', file: 'y' }, - ], - }, - ], - }; - expect(normalizePayload(payload).files[0].dists).to.deep.equal([ - { relativePath: 'a.js.map', file: 'y' }, - { relativePath: 'z.js.map', file: 'x' }, - ]); + it('classifies files+specs only, with unchanged content, as drift from deprecated file props', () => { + const res = classifyDiffFields(['files', 'specs'], false); + expect(res.drift).to.equal(true); + expect(res.changedKeys).to.deep.equal(['deprecated-file-props']); }); - it('leaves a payload without a files array untouched', () => { - const payload = { mainFile: 'index.js' }; - expect(normalizePayload(payload)).to.deep.equal(payload); + it('flags an empty field list with unchanged content as an anomaly, not drift', () => { + const res = classifyDiffFields([], false); + expect(res.drift).to.equal(false); + expect(res.changedKeys).to.deep.equal([]); + expect(res.anomaly).to.equal('modified without a visible diff'); }); }); diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 74790cc08fb0..8dd1695d20e5 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -1,59 +1,45 @@ -import { isEqual, omit, sortBy } from 'lodash'; - -// Deprecated per-file props. consumer.isComponentModified copies them from the model before -// comparing; strip them here too, or a stale `name`/`test` value misclassifies as drift. -const DEPRECATED_FILE_PROPS = ['name', 'test'] as const; - -export const DRIFT_FIELDS = [ +// Field names diffBetweenComponentsObjects (verbose) can emit for a dependency-only change. Any +// other field name means the change is git-authored, not workspace/engine drift. +export const DRIFT_FIELD_NAMES = [ 'dependencies', 'devDependencies', 'peerDependencies', 'extensionDependencies', - 'flattenedDependencies', 'packageDependencies', 'devPackageDependencies', 'peerPackageDependencies', - // env-computed dependency data (force:true env policies). Excludes `extensions`: `bit deps set` - // writes both extensions and overrides, and extensions must stay comparable so that change - // classifies as git-authored. - 'overrides', + 'overridesDependencies', + 'overridesDevDependencies', + 'overridesPeerDependencies', ] as const; -// Keys that legitimately differ between a recorded Version and one rebuilt -// from the filesystem, independent of any user change. -const VOLATILE_FIELDS = ['log', 'parents', 'squashed', 'origin'] as const; +// files/specs field diffs on unchanged file content come from deprecated per-file props (name, +// test); content truth is the hash compare done by the caller, not this field name. +const CONTENT_FIELDS = ['files', 'specs']; -const EXCLUDED = [...DRIFT_FIELDS, ...VOLATILE_FIELDS]; +export type DriftClassification = { + drift: boolean; + changedKeys: string[]; + /** set when the component changed but the diff engine names nothing that explains it */ + anomaly?: string; +}; /** - * Sort a `Version.id()` payload's `files` (and each file's `dists`) by `relativePath`, and strip - * the deprecated `name`/`test` props. Mirrors consumer.ts's sortProperties / deprecated-prop - * alignment, so ordering and stale props don't misclassify as drift. + * Classify a diffBetweenComponentsObjects field-name list as dependency-context drift or + * git-authored. `filesChanged` must come from a content hash compare, not from this field list: + * a files/specs field diff can fire on unchanged content (deprecated per-file props). */ -export function normalizePayload(payload: Record): Record { - if (!Array.isArray(payload.files)) return payload; - const stripDeprecated = (file: Record) => omit(file, DEPRECATED_FILE_PROPS); - return { - ...payload, - files: sortBy(payload.files, 'relativePath').map((file: Record) => { - const stripped = stripDeprecated(file); - return Array.isArray(file.dists) - ? { ...stripped, dists: sortBy(file.dists, 'relativePath').map(stripDeprecated) } - : stripped; - }), - }; -} - -export function classifyPayloadDiff( - recorded: Record, - fromFs: Record -): { depOnly: boolean; changedKeys: string[] } { - const keys = new Set([...Object.keys(recorded), ...Object.keys(fromFs)]); - const changedKeys = [...keys].filter( - (k) => !(VOLATILE_FIELDS as readonly string[]).includes(k) && !isEqual(recorded[k], fromFs[k]) - ); - const depOnly = isEqual(omit(recorded, EXCLUDED), omit(fromFs, EXCLUDED)); - return { depOnly, changedKeys }; +export function classifyDiffFields(fieldNames: string[], filesChanged: boolean): DriftClassification { + if (filesChanged) return { drift: false, changedKeys: fieldNames }; + const effective = fieldNames.filter((f) => !CONTENT_FIELDS.includes(f)); + if (!effective.length) { + // files/specs fired with unchanged content: a deprecated per-file prop (name/test) differs. + if (fieldNames.length) return { drift: true, changedKeys: ['deprecated-file-props'] }; + // no field diff at all, yet the caller reached us because the component is tag-pending. + return { drift: false, changedKeys: [], anomaly: 'modified without a visible diff' }; + } + const drift = effective.every((f) => (DRIFT_FIELD_NAMES as readonly string[]).includes(f)); + return { drift, changedKeys: effective }; } export function convergenceMessage(recordedBitVersions: (string | undefined)[], runningBitVersion: string): string { From 74dfd796d4f7cedf28aa69846a7452e5392c4c53 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 12:05:18 -0400 Subject: [PATCH 17/22] fix(ci): drift detector reads SourceFile.relative, not a nonexistent relativePath The index signature on SourceFile hid the typo from tsc: every file hashed to the same undefined key, collapsing a multi-file component's per-file compare to one entry. Also: narrow the deprecated-file-props default to a provably-equal path set, skip the diff engine once content is known to differ, diff against a clone so the live cached component is never mutated, and guard the file arrays. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 8 +++- .../ci/sync/context-drift-detector.spec.ts | 46 +++++++++++++++++++ scopes/git/ci/sync/context-drift-detector.ts | 44 +++++++++++++----- scopes/git/ci/sync/context-drift.spec.ts | 25 ++++++---- scopes/git/ci/sync/context-drift.ts | 26 +++++++++-- 5 files changed, 124 insertions(+), 25 deletions(-) create mode 100644 scopes/git/ci/sync/context-drift-detector.spec.ts diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 4caf1d914532..4d1fe9015c1f 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1164,7 +1164,13 @@ describe('bit ci sync', function () { before(() => { ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); - helper.fs.outputFile('comp2/index.js', `require('is-odd');\nmodule.exports = () => 'comp2: with-pkg';\n`); + // A second file makes comp2 a multi-file component: the drift run's file-content compare + // must hash every file, not just one, to see that neither one changed. + helper.fs.outputFile('comp2/utils.js', `module.exports = () => 'comp2-util';\n`); + helper.fs.outputFile( + 'comp2/index.js', + `require('is-odd');\nrequire('./utils');\nmodule.exports = () => 'comp2: with-pkg';\n` + ); helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '1.0.0' } }); helper.command.install(); helper.command.tagAllWithoutBuild(); diff --git a/scopes/git/ci/sync/context-drift-detector.spec.ts b/scopes/git/ci/sync/context-drift-detector.spec.ts new file mode 100644 index 000000000000..60f786cf27c5 --- /dev/null +++ b/scopes/git/ci/sync/context-drift-detector.spec.ts @@ -0,0 +1,46 @@ +import { expect } from 'chai'; +import type { SourceFile } from '@teambit/component.sources'; +import { compareFiles } from './context-drift-detector'; + +/** + * Minimal SourceFile stand-in: only `.relative` and the hash chain `compareFiles` reads. + * A regression fixture for the bug this covers: SourceFile has no `.relativePath` (Vinyl exposes + * `.relative`); reading the wrong property collapsed every file to one `undefined`-keyed map + * entry, so only the last file's hash was ever compared. + */ +function file(relative: string, hash: string): SourceFile { + return { relative, toSourceAsLinuxEOL: () => ({ hash: () => ({ hash }) }) } as unknown as SourceFile; +} + +describe('compareFiles', () => { + const recorded = [file('index.js', 'hash-index'), file('utils.js', 'hash-utils')]; + + // same paths, same content hash β€” a real fixture's deprecated `test`/`name` prop discrepancy + // plays no part here, since this function reads only path and hash. That is what lets the + // caller attribute an unchanged-content, files/specs-only field diff to those deprecated props. + it('reports unchanged for identical multi-file sets', () => { + const workspace = [file('index.js', 'hash-index'), file('utils.js', 'hash-utils')]; + expect(compareFiles(recorded, workspace)).to.deep.equal({ filesChanged: false, pathSetsEqual: true }); + }); + + it('catches a content edit on a NON-last file β€” the exact shape the relative/relativePath bug hid', () => { + const workspace = [file('index.js', 'hash-index-EDITED'), file('utils.js', 'hash-utils')]; + const res = compareFiles(recorded, workspace); + expect(res.filesChanged).to.equal(true); + expect(res.pathSetsEqual).to.equal(true); + }); + + it('catches a file added, keeping the recorded pair otherwise identical', () => { + const workspace = [file('index.js', 'hash-index'), file('utils.js', 'hash-utils'), file('new.js', 'hash-new')]; + const res = compareFiles(recorded, workspace); + expect(res.filesChanged).to.equal(true); + expect(res.pathSetsEqual).to.equal(false); + }); + + it('catches a file deleted, keeping the remaining file identical', () => { + const workspace = [file('index.js', 'hash-index')]; + const res = compareFiles(recorded, workspace); + expect(res.filesChanged).to.equal(true); + expect(res.pathSetsEqual).to.equal(false); + }); +}); diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 05fb351b6c29..6a29aeaf0dc1 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -6,9 +6,11 @@ import type { Logger } from '@teambit/logger'; import type { Scope as LegacyScope } from '@teambit/legacy.scope'; import type { Repository } from '@teambit/objects'; import type { SourceFile } from '@teambit/component.sources'; +import { pathNormalizeToLinux } from '@teambit/toolbox.path.path'; import { pMapPool } from '@teambit/toolbox.promise.map-pool'; import { concurrentComponentsLimit } from '@teambit/harmony.modules.concurrency'; import { diffBetweenComponentsObjects } from '@teambit/legacy.component-diff'; +import type { FileComparison } from './context-drift'; import { classifyDiffFields } from './context-drift'; type DriftCheckResult = @@ -22,20 +24,35 @@ export type ContextDriftReport = { gitAuthored: ComponentID[]; }; +// SourceFile is Vinyl-based: the path property is `.relative`, not `.relativePath` (that name +// belongs to SourceFileModel). Both `componentFromModel.files` and the workspace component's +// `.files` are SourceFile β€” `Component.toConsumerComponent` already converts model files to it. +function relPath(file: SourceFile): string { + return pathNormalizeToLinux(file.relative); +} + /** relativePath -> content hash, using the same hashing the model persists files with. */ function fileHashes(files: SourceFile[]): Map { - return new Map(files.map((file) => [file.relativePath, file.toSourceAsLinuxEOL().hash().hash])); + return new Map(files.map((file) => [relPath(file), file.toSourceAsLinuxEOL().hash().hash])); +} + +function pathsEqual(a: SourceFile[], b: SourceFile[]): boolean { + const bPaths = new Set(b.map(relPath)); + return a.length === b.length && a.every((file) => bPaths.has(relPath(file))); } -/** True if any file was added, removed, or its content hash changed. Ignores non-content props. */ -function filesContentChanged(recordedFiles: SourceFile[], workspaceFiles: SourceFile[]): boolean { +/** + * Content truth, independent of the field diff: a files/specs field diff can fire on unchanged + * content (deprecated per-file props). `pathSetsEqual` is computed structurally, never derived + * from the hash compare, so a broken hash compare can't fake it. Exported for unit coverage β€” + * this is the multi-file content compare, not the diff engine. + */ +export function compareFiles(recordedFiles: SourceFile[], workspaceFiles: SourceFile[]): FileComparison { const recorded = fileHashes(recordedFiles); const workspace = fileHashes(workspaceFiles); - if (recorded.size !== workspace.size) return true; - for (const [relativePath, hash] of recorded) { - if (workspace.get(relativePath) !== hash) return true; - } - return false; + const filesChanged = + recorded.size !== workspace.size || [...recorded].some(([path, hash]) => workspace.get(path) !== hash); + return { filesChanged, pathSetsEqual: pathsEqual(recordedFiles, workspaceFiles) }; } /** Attribution only: which bit version recorded this component. A lookup failure must not affect the verdict. */ @@ -77,10 +94,15 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): const consumerComp = comp.state._consumer; const fromModel = consumerComp.componentFromModel; if (!fromModel) return { id, kind: 'git-authored' }; // nothing recorded to diff against - const filesChanged = filesContentChanged(fromModel.files, consumerComp.files); - const fieldsDiff = await diffBetweenComponentsObjects(fromModel, consumerComp, { verbose: true }); + const fileComparison = compareFiles(fromModel.files ?? [], consumerComp.files ?? []); + // content alone settles it; skip the diff engine (it shells out to `git diff --no-index` + // per differing aspect config) rather than spend that cost on an already-decided verdict. + if (fileComparison.filesChanged) return { id, kind: 'git-authored' }; + // sortById (inside diffBetweenComponentsObjects) reorders extension config in place; diff + // against a clone so the live, possibly-cached workspace component is never touched. + const fieldsDiff = await diffBetweenComponentsObjects(fromModel, consumerComp.clone(), { verbose: true }); const fieldNames = (fieldsDiff ?? []).map((f) => f.fieldName); - const { drift, changedKeys, anomaly } = classifyDiffFields(fieldNames, filesChanged); + const { drift, changedKeys, anomaly } = classifyDiffFields(fieldNames, fileComparison); if (anomaly) logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: ${anomaly}`)); if (!drift) return { id, kind: 'git-authored' }; const recordedBitVersion = await getRecordedBitVersion(legacyScope, repo, id); diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index e069aaa95794..2c59e3210b3b 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -2,40 +2,49 @@ import { expect } from 'chai'; import { classifyDiffFields, convergenceMessage, blockerNamesUnion } from './context-drift'; describe('classifyDiffFields', () => { + const unchanged = { filesChanged: false, pathSetsEqual: true }; + const changed = { filesChanged: true, pathSetsEqual: true }; + it('classifies a change confined to dependency-ish fields as drift', () => { - const res = classifyDiffFields(['packageDependencies', 'dependencies'], false); + const res = classifyDiffFields(['packageDependencies', 'dependencies'], unchanged); expect(res.drift).to.equal(true); expect(res.changedKeys).to.deep.equal(['packageDependencies', 'dependencies']); }); it('classifies overridesDevDependencies alone as drift', () => { - expect(classifyDiffFields(['overridesDevDependencies'], false).drift).to.equal(true); + expect(classifyDiffFields(['overridesDevDependencies'], unchanged).drift).to.equal(true); }); it('rejects overridesPackageJsonProps as a non-dependency field', () => { - expect(classifyDiffFields(['overridesPackageJsonProps'], false).drift).to.equal(false); + expect(classifyDiffFields(['overridesPackageJsonProps'], unchanged).drift).to.equal(false); }); it('rejects an aspect configuration field name', () => { - const res = classifyDiffFields(['teambit.envs/envs configuration'], false); + const res = classifyDiffFields(['teambit.envs/envs configuration'], unchanged); expect(res.drift).to.equal(false); expect(res.changedKeys).to.deep.equal(['teambit.envs/envs configuration']); }); it('rejects any field set once file content changed, regardless of which fields fired', () => { - const res = classifyDiffFields(['packageDependencies'], true); + const res = classifyDiffFields(['packageDependencies'], changed); expect(res.drift).to.equal(false); expect(res.changedKeys).to.deep.equal(['packageDependencies']); }); - it('classifies files+specs only, with unchanged content, as drift from deprecated file props', () => { - const res = classifyDiffFields(['files', 'specs'], false); + it('classifies files+specs only, with unchanged content and equal path sets, as drift from deprecated file props', () => { + const res = classifyDiffFields(['files', 'specs'], unchanged); expect(res.drift).to.equal(true); expect(res.changedKeys).to.deep.equal(['deprecated-file-props']); }); + it('refuses the deprecated-file-props default when the path sets are not provably equal', () => { + const res = classifyDiffFields(['files', 'specs'], { filesChanged: false, pathSetsEqual: false }); + expect(res.drift).to.equal(false); + expect(res.changedKeys).to.deep.equal(['files', 'specs']); + }); + it('flags an empty field list with unchanged content as an anomaly, not drift', () => { - const res = classifyDiffFields([], false); + const res = classifyDiffFields([], unchanged); expect(res.drift).to.equal(false); expect(res.changedKeys).to.deep.equal([]); expect(res.anomaly).to.equal('modified without a visible diff'); diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 8dd1695d20e5..7af2a32e2a49 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -1,5 +1,8 @@ // Field names diffBetweenComponentsObjects (verbose) can emit for a dependency-only change. Any // other field name means the change is git-authored, not workspace/engine drift. +// `extensionDependencies` alone (a config-less aspect added or removed) also classifies as +// drift: inherent ambiguity, but load-bearing for a config-less aspect version bump that comes +// from an engine template rather than a developer edit. export const DRIFT_FIELD_NAMES = [ 'dependencies', 'devDependencies', @@ -24,17 +27,30 @@ export type DriftClassification = { anomaly?: string; }; +export type FileComparison = { + /** any file added, removed, or hash-changed β€” from a content compare, never from field names */ + filesChanged: boolean; + /** the two file sets cover the exact same relative paths, independently of any hash */ + pathSetsEqual: boolean; +}; + /** * Classify a diffBetweenComponentsObjects field-name list as dependency-context drift or - * git-authored. `filesChanged` must come from a content hash compare, not from this field list: - * a files/specs field diff can fire on unchanged content (deprecated per-file props). + * git-authored. `filesChanged`/`pathSetsEqual` must come from a content compare, not from this + * field list: a files/specs field diff can fire on unchanged content (deprecated per-file props). */ -export function classifyDiffFields(fieldNames: string[], filesChanged: boolean): DriftClassification { +export function classifyDiffFields( + fieldNames: string[], + { filesChanged, pathSetsEqual }: FileComparison +): DriftClassification { if (filesChanged) return { drift: false, changedKeys: fieldNames }; const effective = fieldNames.filter((f) => !CONTENT_FIELDS.includes(f)); if (!effective.length) { - // files/specs fired with unchanged content: a deprecated per-file prop (name/test) differs. - if (fieldNames.length) return { drift: true, changedKeys: ['deprecated-file-props'] }; + // files/specs fired with unchanged content: only a deprecated per-file prop (name/test) can + // explain it, and only if the two sides name the exact same files. Without that second check, + // this branch is a blanket default that would paper over a broken content compare. + if (fieldNames.length && pathSetsEqual) return { drift: true, changedKeys: ['deprecated-file-props'] }; + if (fieldNames.length) return { drift: false, changedKeys: fieldNames }; // no field diff at all, yet the caller reached us because the component is tag-pending. return { drift: false, changedKeys: [], anomaly: 'modified without a visible diff' }; } From 96de0d8e492806ad90f51a1b8f8c4dda66c49ffc Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 12:10:38 -0400 Subject: [PATCH 18/22] fix(ci): drop the pointless clone before diffing, correct the comment Component.clone() shares ExtensionDataList entries with the original, so it never shielded the live component from sortById()'s in-place config mutation -- it only paid a deep copy of every file's contents Buffer per drift candidate. Pass the live consumerComp; the mutation is shallow and hash-idempotent (Version.id() sorts the same way). Co-Authored-By: Claude Fable 5 --- scopes/git/ci/sync/context-drift-detector.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 6a29aeaf0dc1..774a74443fdd 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -98,9 +98,10 @@ export async function detectContextDrift(workspace: Workspace, logger: Logger): // content alone settles it; skip the diff engine (it shells out to `git diff --no-index` // per differing aspect config) rather than spend that cost on an already-decided verdict. if (fileComparison.filesChanged) return { id, kind: 'git-authored' }; - // sortById (inside diffBetweenComponentsObjects) reorders extension config in place; diff - // against a clone so the live, possibly-cached workspace component is never touched. - const fieldsDiff = await diffBetweenComponentsObjects(fromModel, consumerComp.clone(), { verbose: true }); + // diffBetweenComponentsObjects sorts extension config keys in place; the mutation is + // shallow and hash-idempotent (Version.id() sorts the same way), so it is acceptable on + // the live object. + const fieldsDiff = await diffBetweenComponentsObjects(fromModel, consumerComp, { verbose: true }); const fieldNames = (fieldsDiff ?? []).map((f) => f.fieldName); const { drift, changedKeys, anomaly } = classifyDiffFields(fieldNames, fileComparison); if (anomaly) logger.console(chalk.yellow(` ${id.toStringWithoutVersion()}: ${anomaly}`)); From 1e98903b2135c74198fb35419ddac0c1d5cac046 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 13:30:00 -0400 Subject: [PATCH 19/22] feat(ci): lanes carry the dependency-context fan-out A lane run now snaps the full pending set (the git-authored change and any dependency-context drift) in one snap under the developer's message, instead of excluding drift. Status verification still scopes to the git-authored subset; the drifted subset's pre-existing blockers are tolerated at snap time via the union of their tag-blocker issue names. Adds a PR comment surfacing the drifted components as a side effect of the committed context, upserted in place via a new optional GitHostProvider.upsertComment. snapPrCommit drops the snapIds scoping mechanism (snap reverts to bit's own tag-pending resolution) in favor of verifyIds (verification scope) and driftIds (blocker-tolerance union, computed against the status the verification step already loaded). bit ci pr passes neither, so its production path is unchanged. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 67 ++++++++++++-- scopes/git/ci/ci.docs.mdx | 12 +-- scopes/git/ci/ci.main.runtime.ts | 110 ++++++++++------------- scopes/git/ci/sync/context-drift.ts | 49 ++++++++++ scopes/git/ci/sync/git-host-provider.ts | 8 ++ scopes/git/ci/sync/github-client.spec.ts | 48 ++++++++++ scopes/git/ci/sync/github-client.ts | 35 ++++++++ scopes/git/ci/sync/lane-sync-executor.ts | 77 +++++++++++----- 8 files changed, 307 insertions(+), 99 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 4d1fe9015c1f..259084ab2fd3 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1120,8 +1120,10 @@ describe('bit ci sync', function () { }); }); - // A real scope carries components with tag blockers (e.g. circular dependencies). The snap - // covers only the lane's pending components. A blocker on an untouched component must not halt it. + // A real scope carries components with tag blockers (e.g. circular dependencies). This pair is + // untouched in the strongest sense: no diff at all, so it is never part of the run's tag-pending + // set, and its blocker must not halt the run. Contrast the drift cell below, where a pair IS + // pending (a dep-only diff) and its blocker is tolerated instead of avoided. describe('a snap-blocking issue on a component the lane never touches', () => { const LANE = 'clean-lane'; let defaultBranch: string; @@ -1155,9 +1157,11 @@ describe('bit ci sync', function () { }); // This reproduces an engine bump with one bit binary: a committed root policy moves a recorded - // package range. The lane run snaps only the git-authored change, and reports the drifted - // component instead of sweeping it into the dev's snap. - describe('dependency-context drift is excluded from the lane snap', () => { + // package range. Lanes carry the fan-out (spec decision 6): the lane run snaps the drifted + // component together with the git-authored change, in one snap under the developer's own + // message, and reports it as a surfaced side effect β€” git does not show this change, the run + // log and the PR report do. + describe('dependency-context drift rides the lane snap as a surfaced side effect', () => { const LANE = 'drift-lane'; let defaultBranch: string; let devPath: string; @@ -1179,7 +1183,7 @@ describe('bit ci sync', function () { helper.command.runCmd('git commit -m "comp2 records is-odd 1.0.0"'); helper.command.runCmd(`git push origin ${defaultBranch}`); // Create the lane before the policy bump, or the dev's own (unscoped) `bit snap` sweeps the - // drift in too, leaving nothing for `bit ci sync` to exclude. + // change in too, leaving nothing for `bit ci sync` to detect as drift. devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); seedSync(LANE); branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); @@ -1194,7 +1198,7 @@ describe('bit ci sync', function () { helper.command.runCmd(`git push origin ${defaultBranch}`); }); - it('snaps the dev commit, reports the drifted component, and keeps it off the lane', () => { + it('snaps the dev commit, carries the drifted component onto the lane, and reports it', () => { const before = remoteLaneFingerprint(LANE); expect(before).to.not.include('comp2'); const { output, exitCode } = syncRun(LANE); @@ -1202,7 +1206,54 @@ describe('bit ci sync', function () { expect(output).to.include('dependency-context drift'); expect(output).to.include('comp2'); expect(laneTipFile(devPath, 'comp1/index.js')).to.include('dev-commit-1'); - expect(remoteLaneFingerprint(LANE)).to.not.include('comp2'); + expect(remoteLaneFingerprint(LANE)).to.include('comp2'); + }); + }); + + // The case that used to halt production: a drifted component's pre-existing tag blocker must not + // block a lane run now that lanes carry the whole fan-out in one snap. Reuses the circular-pair + + // policy-bump recipe from the main-convergence describe below, on a lane instead of main. + describe('a drifted component with a pre-existing blocker rides the lane snap without halting', () => { + const LANE = 'drift-with-blocker-lane'; + let defaultBranch: string; + let devPath: string; + + before(() => { + ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); + // A circular pair, recorded under a tag-blocker override β€” the blocker the drifted set's + // tolerance (`snapIgnoreIssues`) must carry through the lane snap. + helper.fs.outputFile('comp3/index.js', `require('is-odd');\nrequire('@${helper.scopes.remote}/comp4');`); + helper.fs.outputFile('comp4/index.js', `require('@${helper.scopes.remote}/comp3');`); + helper.command.addComponent('comp3'); + helper.command.addComponent('comp4'); + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '1.0.0' } }); + helper.command.install(); + helper.command.tagAllWithoutBuild('--ignore-issues="CircularDependencies"'); + helper.command.export(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "record circular pair under is-odd 1.0.0"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + // Create the lane before the policy bump, for the same reason as the drift cell above. + devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); + seedSync(LANE); + branchSideCommit(LANE, defaultBranch, 'comp1/index.js', comp1Src('dev-commit-1'), 'dev commit on comp1'); + // engine-bump analogue: drifts the circular pair, blocker and all + helper.workspaceJsonc.addPolicyToDependencyResolver({ dependencies: { 'is-odd': '3.0.1' } }); + helper.command.install(); + helper.command.runCmd('git add -A'); + helper.command.runCmd('git commit -m "bump is-odd policy (engine-bump analogue)"'); + helper.command.runCmd(`git push origin ${defaultBranch}`); + }); + + it('exits 0, snaps the dev commit, and carries the drifted pair onto the lane', () => { + const { output, exitCode } = syncRun(LANE); + expect(exitCode, `bit ci sync output:\n${output}`).to.equal(0); + expect(output).to.not.include('Workspace status verification failed'); + expect(output).to.include('dependency-context drift'); + expect(laneTipFile(devPath, 'comp1/index.js')).to.include('dev-commit-1'); + const laneFingerprint = remoteLaneFingerprint(LANE); + expect(laneFingerprint).to.include('comp3'); + expect(laneFingerprint).to.include('comp4'); }); }); diff --git a/scopes/git/ci/ci.docs.mdx b/scopes/git/ci/ci.docs.mdx index c08c808b060a..1763f3bc3064 100644 --- a/scopes/git/ci/ci.docs.mdx +++ b/scopes/git/ci/ci.docs.mdx @@ -405,12 +405,12 @@ This is a real dependency change that the repository introduces. `bit ci sync` consumes it. A main run tags the drifted components with a patch bump and a message of the shape `chore: align dependency context (recorded with bit X, workspace runs bit Y)`, -then exports. A lane run snaps only the components with git-authored changes -and reports the drift; it does not snap a drifted component directly, but a -drifted component that depends on a snapped component is auto-snapped as -its dependent. Pin the engine in `workspace.jsonc` -(`"teambit.harmony/bit": { "engine": "" }`) so the context moves only -when a commit moves it. +then exports. A lane run snaps the drift together with the git-authored +change, in one snap under the developer's own message, and reports the +drifted components on the pull request β€” main convergence is for context +drift that reaches main without a pull request. Pin the engine in +`workspace.jsonc` (`"teambit.harmony/bit": { "engine": "" }`) so the +context moves only when a commit moves it. ### Git host providers and credentials diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index dc7d9355358b..d670b38d4525 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -445,11 +445,14 @@ export class CiMain { } /** - * `snapIds` scopes the status failure to the components this run snaps β€” a global failure would + * `verifyIds` scopes the status failure to the components this run snaps β€” a global failure would * otherwise block every snap in the repo. The snap itself still enforces blockers on its own * components. */ - private async verifyWorkspaceStatusInternal(strict: boolean = false, { snapIds }: { snapIds?: ComponentID[] } = {}) { + private async verifyWorkspaceStatusInternal( + strict: boolean = false, + { verifyIds }: { verifyIds?: ComponentID[] } = {} + ) { this.logger.console('πŸ“Š Workspace Status'); this.logger.console(chalk.blue('Verifying status of workspace')); @@ -463,8 +466,8 @@ export class CiMain { this.logger.console(statusOutput); let effectiveCode = code; - if (code !== 0 && snapIds) { - const inSet = ComponentIdList.fromArray(snapIds); + if (code !== 0 && verifyIds) { + const inSet = ComponentIdList.fromArray(verifyIds); const scoped = { ...status, componentsWithIssues: status.componentsWithIssues.filter((c) => inSet.hasWithoutVersion(c.id)), @@ -714,7 +717,7 @@ export class CiMain { skipCleanup, skipTasks, noDestructiveRecovery, - snapIds, + verifyIds, driftIds, }: { laneIdStr: string; @@ -733,9 +736,18 @@ export class CiMain { * stale-lane case throws instead, which the sync executor surfaces as a halt for a human. */ noDestructiveRecovery?: boolean; - /** Snap only these ids (no version), not every tag-pending component; unset for `bit ci pr` (global). */ - snapIds?: string[]; - /** Ids excluded from `snapIds` as dependency-context drift; used only to report a dependent auto-snap. */ + /** + * Scope the status verification to these ids (no version) instead of every pending component; + * unset for `bit ci pr` (production: global). The snap itself is never scoped β€” it always snaps + * whatever bit's own tag-pending resolution finds. + */ + verifyIds?: string[]; + /** + * Dependency-context drift ids riding along in this snap (lane runs only). Consumed only here, + * to compute the snap's `ignoreIssues`: a drifted component's files and config already equal its + * recorded head, so a tag-blocker already present on it is not new β€” see `blockerNamesUnion`. + * Never forwarded past this method. + */ driftIds?: string[]; }) { // The post-export cleanup switches the workspace back to main, which re-checks-out main's HEAD @@ -769,15 +781,17 @@ export class CiMain { const laneId = await this.lanes.parseLaneId(laneIdStr); - const resolvedSnapIds = snapIds ? await this.workspace.resolveMultipleComponentIds(snapIds) : undefined; - if (resolvedSnapIds && !resolvedSnapIds.length) { - // This method can't tell drift-caused emptiness from nothing pending; the caller (e.g. the - // lane sync executor) reports drift separately. - this.logger.console(formatWarningSummary('No git-authored changes to snap')); - return 'No changes detected, nothing to snap'; - } + const resolvedVerifyIds = verifyIds ? await this.workspace.resolveMultipleComponentIds(verifyIds) : undefined; + + const { status } = await this.verifyWorkspaceStatusInternal(strict, { verifyIds: resolvedVerifyIds }); - await this.verifyWorkspaceStatusInternal(strict, { snapIds: resolvedSnapIds }); + // The union of tag-blocker issue names already present on the drifted set β€” tolerated because a + // drifted component's files and config equal its recorded head, so its blockers are not new. + // Computed here (against the status this call already loaded) rather than in the caller, so a + // lane run doesn't pay for a second full `status()` pass. + const snapIgnoreIssues = driftIds?.length + ? blockerNamesUnion(status.componentsWithIssues, new Set(driftIds)) + : undefined; await this.importer .import({ @@ -801,13 +815,11 @@ export class CiMain { originalLane, message: resolvedMessage, build, - strict, dryRun, skipCleanup: resolvedSkipCleanup, skipTasks: resolvedSkipTasks, noDestructiveRecovery, - snapIds: resolvedSnapIds, - driftIds, + snapIgnoreIssues, }); } return this.snapAndExportWithTempLane({ @@ -818,7 +830,7 @@ export class CiMain { dryRun, skipCleanup: resolvedSkipCleanup, skipTasks: resolvedSkipTasks, - snapIds: resolvedSnapIds, + snapIgnoreIssues, }); } @@ -877,25 +889,22 @@ export class CiMain { originalLane, message, build, - strict, dryRun, skipCleanup, skipTasks, noDestructiveRecovery, - snapIds, - driftIds, + snapIgnoreIssues, }: { laneId: LaneId; originalLane: Lane | undefined; message: string; build: boolean | undefined; - strict: boolean | undefined; dryRun?: boolean; skipCleanup: boolean; skipTasks?: string; noDestructiveRecovery?: boolean; - snapIds?: ComponentID[]; - driftIds?: string[]; + /** Tag-blocker issue names to tolerate on this snap; see `snapPrCommit`'s `driftIds`. */ + snapIgnoreIssues?: string; }) { // Query the remote (by name, to avoid fetching all lanes) so we know whether to reuse or create const existingLanes = await this.lanes.getLanes({ remote: laneId.scope, name: laneId.name }).catch((e) => { @@ -936,22 +945,11 @@ export class CiMain { ) ); } else { - await this.syncConfigFromMain(laneId); // `syncConfigFromMain` clears the component cache; a component it just reconfigured - // can only now show as git-authored. Add it here, or the snap misses it. Never add a - // drift id. - if (snapIds) { - const { gitAuthored } = await this.detectContextDrift(); - const known = new Set(snapIds.map((id) => id.toStringWithoutVersion())); - const missing = gitAuthored.filter((id) => !known.has(id.toStringWithoutVersion())); - if (missing.length) { - snapIds = [...snapIds, ...missing]; - // The added ids skipped `snapPrCommit`'s scoped verify, which ran before this - // expansion. Re-verify over the final set, or an added id's blocker surfaces at - // snap instead of here. - await this.verifyWorkspaceStatusInternal(strict, { snapIds }); - } - } + // can only now show as tag-pending. Nothing to re-verify or expand here: the snap below + // always resolves its own tag-pending set fresh (no `legacyBitIds` scoping), so it picks + // up any component this sync just reconfigured without a staleness window. + await this.syncConfigFromMain(laneId); } } else { // Switch failed even though the remote lane exists. The destructive recovery below @@ -1073,7 +1071,7 @@ export class CiMain { build, exitOnFirstFailedTask: true, skipTasks, - legacyBitIds: snapIds ? ComponentIdList.fromArray(snapIds) : undefined, + ignoreIssues: snapIgnoreIssues, }); if (!results) { @@ -1081,26 +1079,7 @@ export class CiMain { return 'No changes detected, nothing to snap'; } - const { snappedComponents, autoSnappedResults }: SnapResults = results; - - // A drifted id excluded from `snapIds` can still be auto-snapped, as a dependent of a - // component this run did snap β€” that auto-snap consumes its drift. Report it; the caller - // logged the drift as "not snapped here" before this run knew the outcome. - if (driftIds?.length) { - const driftSet = new Set(driftIds); - const autoSnappedDrift = [ - ...new Set( - autoSnappedResults - .filter((r) => driftSet.has(r.component.id.toStringWithoutVersion())) - .map((r) => r.component.id.toStringWithoutVersion()) - ), - ]; - if (autoSnappedDrift.length) { - this.logger.console( - chalk.blue(`Auto-snapped as a dependent, consuming its drift: ${autoSnappedDrift.join(', ')}`) - ); - } - } + const { snappedComponents }: SnapResults = results; const snapOutput = snapResultOutput(results); this.logger.console(snapOutput); @@ -1139,7 +1118,7 @@ export class CiMain { dryRun, skipCleanup, skipTasks, - snapIds, + snapIgnoreIssues, }: { laneId: LaneId; originalLane: Lane | undefined; @@ -1148,7 +1127,8 @@ export class CiMain { dryRun?: boolean; skipCleanup: boolean; skipTasks?: string; - snapIds?: ComponentID[]; + /** Tag-blocker issue names to tolerate on this snap; see `snapPrCommit`'s `driftIds`. */ + snapIgnoreIssues?: string; }) { // Use unique temp lane name to avoid race conditions when multiple CI jobs run concurrently const tempLaneName = `${laneId.name}-${generateRandomStr(5)}`; @@ -1180,7 +1160,7 @@ export class CiMain { build, exitOnFirstFailedTask: true, skipTasks, - legacyBitIds: snapIds ? ComponentIdList.fromArray(snapIds) : undefined, + ignoreIssues: snapIgnoreIssues, }); if (!results) { diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index 7af2a32e2a49..b12e0af92766 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -86,3 +86,52 @@ export function blockerNamesUnion( } return names.size ? [...names].join(',') : undefined; } + +/** Marks the one PR comment `bit ci sync` owns for the drift report (spec 5.6). Not shown to a reader. */ +export const DRIFT_COMMENT_MARKER = ''; + +/** How many drifted components a report lists before it summarizes the rest, to stay under a git host's comment size limit. */ +const MAX_LISTED_DRIFT_COMPONENTS = 50; + +type DriftEntry = { id: { toStringWithoutVersion(): string }; recordedBitVersion?: string; changedKeys: string[] }; + +/** + * The PR comment body for a lane snap that carried dependency-context drift (spec 5.6). States the + * cause, lists the affected components and their changed fields, and points at the change request + * for the full diff. `driftClearedCommentBody` is the counterpart once the drift is gone. + */ +export function driftReportCommentBody(drift: DriftEntry[], runningBitVersion: string): string { + const recorded = [...new Set(drift.map((d) => d.recordedBitVersion).filter(Boolean))].join(', '); + const causeLine = recorded + ? `Cause: the dependency context changed (recorded with bit ${recorded}, running bit ${runningBitVersion}).` + : `Cause: the dependency context changed.`; + const entries = drift.map((d) => `- \`${d.id.toStringWithoutVersion()}\` β€” ${d.changedKeys.join(', ')}`); + const list = + entries.length > MAX_LISTED_DRIFT_COMPONENTS + ? [ + ...entries.slice(0, MAX_LISTED_DRIFT_COMPONENTS), + `- …and ${entries.length - MAX_LISTED_DRIFT_COMPONENTS} more`, + ] + : entries; + return [ + DRIFT_COMMENT_MARKER, + '### Dependency-context drift', + '', + causeLine, + 'This snap carries the components below as a side effect of the committed context. Git does not show this change.', + '', + ...list, + '', + 'The change request on Bit Cloud shows the full diff.', + ].join('\n'); +} + +/** The drift-report comment, updated in place once its drift clears. Never deleted (spec 5.6). */ +export function driftClearedCommentBody(): string { + return [ + DRIFT_COMMENT_MARKER, + '### Dependency-context drift', + '', + 'The dependency-context drift this comment reported is gone. The lane matches its dependency context.', + ].join('\n'); +} diff --git a/scopes/git/ci/sync/git-host-provider.ts b/scopes/git/ci/sync/git-host-provider.ts index ae6705add384..a484347eb9e9 100644 --- a/scopes/git/ci/sync/git-host-provider.ts +++ b/scopes/git/ci/sync/git-host-provider.ts @@ -33,6 +33,14 @@ export interface GitHostProvider { comment(prNumber: number, body: string): Promise; addLabel(prNumber: number, label: string): Promise; + + /** + * Update the comment containing `marker` in place, or post a new one when none exists yet + * (`options.createIfAbsent` defaults to true). Optional: a provider that doesn't implement this + * is treated as not supporting the drift-report surface β€” callers skip rather than fall back to a + * plain, never-updated `comment`, which would leave one stale copy per push. + */ + upsertComment?(prNumber: number, marker: string, body: string, options?: { createIfAbsent?: boolean }): Promise; } /** The git host to use this run, or nothing plus the reason PR operations are being skipped. */ diff --git a/scopes/git/ci/sync/github-client.spec.ts b/scopes/git/ci/sync/github-client.spec.ts index 93ab4d18a0b4..a329dd608453 100644 --- a/scopes/git/ci/sync/github-client.spec.ts +++ b/scopes/git/ci/sync/github-client.spec.ts @@ -171,4 +171,52 @@ describe('GitHubClient', () => { const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl: fakeFetch }); expect(await client.findPrByBranch('lane-x')).to.equal(undefined); }); + + describe('upsertIssueComment', () => { + function fakeFetchOver(existingComments: Array<{ id: number; body: string }>) { + const calls: Array<{ url: string; init: any }> = []; + const fetchImpl = (async (url: any, init: any) => { + calls.push({ url: String(url), init }); + const method = init?.method ?? 'GET'; + if (method === 'GET') { + return new Response(JSON.stringify(existingComments), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + } + return new Response(JSON.stringify({}), { status: 200, headers: { 'content-type': 'application/json' } }); + }) as typeof fetch; + return { calls, fetchImpl }; + } + + it('posts a new comment when no marked comment exists', async () => { + const { calls, fetchImpl } = fakeFetchOver([]); + const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); + await client.upsertIssueComment(7, '', '\nbody'); + const posts = calls.filter((c) => c.init?.method === 'POST'); + expect(posts).to.have.lengthOf(1); + expect(posts[0].url).to.equal('https://api.github.com/repos/acme/shop/issues/7/comments'); + }); + + it('patches the marked comment in place, and never posts a second one', async () => { + const { calls, fetchImpl } = fakeFetchOver([ + { id: 42, body: 'unrelated comment' }, + { id: 99, body: '\nold report' }, + ]); + const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); + await client.upsertIssueComment(7, '', '\nnew report'); + const patches = calls.filter((c) => c.init?.method === 'PATCH'); + expect(patches).to.have.lengthOf(1); + expect(patches[0].url).to.equal('https://api.github.com/repos/acme/shop/issues/comments/99'); + expect(JSON.parse(patches[0].init.body)).to.deep.equal({ body: '\nnew report' }); + expect(calls.some((c) => c.init?.method === 'POST')).to.equal(false); + }); + + it('skips silently when createIfAbsent is false and no marked comment exists', async () => { + const { calls, fetchImpl } = fakeFetchOver([]); + const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); + await client.upsertIssueComment(7, '', 'cleared', { createIfAbsent: false }); + expect(calls.some((c) => c.init?.method === 'POST' || c.init?.method === 'PATCH')).to.equal(false); + }); + }); }); diff --git a/scopes/git/ci/sync/github-client.ts b/scopes/git/ci/sync/github-client.ts index 82a07c1e535f..f09c79f6c432 100644 --- a/scopes/git/ci/sync/github-client.ts +++ b/scopes/git/ci/sync/github-client.ts @@ -135,6 +135,32 @@ export class GitHubClient implements GitHostProvider { async addLabel(prNumber: number, label: string): Promise { await this.request('POST', `/issues/${prNumber}/labels`, { labels: [label] }); } + + private async listIssueComments(prNumber: number): Promise<{ id: number; body: string }[]> { + const list = await this.request('GET', `/issues/${prNumber}/comments`); + return (list ?? []).map((c: any) => ({ id: c.id, body: c.body ?? '' })); + } + + /** + * Find the comment carrying `marker` (an HTML comment embedded in the body) and replace its body, + * or post `body` as a new comment when none exists and `options.createIfAbsent` is not false. + * GitHub's comment PATCH endpoint is `/issues/comments/{id}`, not `/issues/{pr}/comments/{id}` β€” + * a comment id is unique per repository, not scoped under the issue/PR that carries it. + */ + async upsertIssueComment( + prNumber: number, + marker: string, + body: string, + options: { createIfAbsent?: boolean } = {} + ): Promise { + const existing = (await this.listIssueComments(prNumber)).find((c) => c.body.includes(marker)); + if (existing) { + await this.request('PATCH', `/issues/comments/${existing.id}`, { body }); + return; + } + if (options.createIfAbsent === false) return; + await this.comment(prNumber, body); + } } /** @@ -191,6 +217,15 @@ export class GitHubHostProvider implements GitHostProvider { return this.requireClient().addLabel(prNumber, label); } + async upsertComment( + prNumber: number, + marker: string, + body: string, + options?: { createIfAbsent?: boolean } + ): Promise { + return this.requireClient().upsertIssueComment(prNumber, marker, body, options); + } + private resolveClient(remoteUrl?: string): GitHubClient | undefined { if (!this.client) this.client = GitHubClient.fromEnv(remoteUrl ?? this.remoteHint, this.onWarning); return this.client; diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 8ebcbb606392..673d69e62dec 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -6,9 +6,10 @@ import type { LaneData } from '@teambit/legacy.scope'; import { getCloudDomain } from '@teambit/legacy.constants'; import { FileStatus, type MergeStrategy } from '@teambit/component.modules.merge-helper'; import { git } from '../git'; -import type { CiMain } from '../ci.main.runtime'; +import type { CiMain, ContextDriftReport } from '../ci.main.runtime'; import type { CiSyncConfig, LaneTarget } from './sync-config'; import { laneNameToBranch } from './sync-config'; +import { DRIFT_COMMENT_MARKER, driftClearedCommentBody, driftReportCommentBody } from './context-drift'; import type { BranchSyncState } from './sync-state'; import { CONFLICT_LABEL, @@ -610,7 +611,7 @@ export class LaneSyncExecutor { await this.checkoutFromRemote(branch, `origin/${branch}`); try { - const exportErr = await this.snapAndExportOntoLane(laneIdStr, message); + const { error: exportErr, drift } = await this.snapAndExportOntoLane(laneIdStr, message); if (exportErr) { // Halt rather than propagate: one lane's failed snap/export must not abort the lanes after it. return await this.executeHalt({ @@ -621,6 +622,7 @@ export class LaneSyncExecutor { pr: await this.findPr(branch), }); } + await this.surfaceDriftOnPr(branch, drift); const laneHead = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); if (!laneHead) { @@ -728,7 +730,7 @@ export class LaneSyncExecutor { } // ---- step 2: snap + export the merged tree onto the lane --------------------------------- - const exportErr = await this.snapAndExportOntoLane( + const { error: exportErr, drift } = await this.snapAndExportOntoLane( laneIdStr, `merge remote lane ${laneIdStr} into ${branch} ${SYNC_COMMIT_MARKER}` ); @@ -738,6 +740,7 @@ export class LaneSyncExecutor { `result failed: ${exportErr.message}` ); } + await this.surfaceDriftOnPr(branch, drift); // ---- step 3: record the new lane head on the branch -------------------------------------- const laneHead = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); @@ -759,20 +762,25 @@ export class LaneSyncExecutor { } /** - * Snap the workspace's current tree onto the lane and export it; returns the error so the caller can - * halt. Snaps WHATEVER is in the workspace (the switch uses `forceOurs` and never merges files), so - * a diverged tree must already hold the merged content. `keepLane` preserves the lane's history, - * `skipCleanup` keeps the snap's `.bitmap` for the caller to commit, `noDestructiveRecovery` turns - * stale-lane recovery (delete + re-fork the remote lane) into a throw. The lane object must be - * imported BEFORE delegating: a switch onto the lane the workspace is already on no-ops before any - * fetch, so it never warms a cold scope. + * Snap the workspace's current tree onto the lane and export it; returns the error (if any) and the + * drift the run detected, so the caller can halt or surface the report. Snaps WHATEVER is in the + * workspace (the switch uses `forceOurs` and never merges files), so a diverged tree must already + * hold the merged content. `keepLane` preserves the lane's history, `skipCleanup` keeps the snap's + * `.bitmap` for the caller to commit, `noDestructiveRecovery` turns stale-lane recovery (delete + + * re-fork the remote lane) into a throw. The lane object must be imported BEFORE delegating: a + * switch onto the lane the workspace is already on no-ops before any fetch, so it never warms a + * cold scope. * - * Splits pending components into git-authored changes and dependency-context drift before - * snapping. Only the git-authored subset passes as `snapIds`; drift rides into a lane snap only - * as an auto-snapped dependent of a snapped component (`snapPrCommit` reports that case). - * Main-side convergence consumes drift not auto-snapped this way. + * Lanes carry the fan-out (spec decision 6): the snap covers the full pending set β€” the + * git-authored change and any dependency-context drift β€” under the developer's own message. Status + * verification still scopes to the git-authored subset (`verifyIds`); the drifted subset's + * pre-existing blockers are tolerated at snap time via `driftIds` (see `blockerNamesUnion`), never + * at verification time β€” a *new* blocker on a component the developer touched still fails the run. */ - private async snapAndExportOntoLane(laneIdStr: string, message: string): Promise { + private async snapAndExportOntoLane( + laneIdStr: string, + message: string + ): Promise<{ error?: Error; drift: ContextDriftReport['drift'] }> { try { await ensureCurrentLaneObject(this.deps.lanes); const { drift, gitAuthored } = await this.deps.ci.detectContextDrift(); @@ -782,8 +790,9 @@ export class LaneSyncExecutor { this.deps.logger.console( formatSection( 'dependency-context drift', - `not snapped directly by this run (a dependent may auto-snap it)` + - `${recorded ? ` β€” recorded with bit ${recorded}, running bit ${running}` : ''}`, + `${drift.length} component(s) carry dependency-context drift` + + `${recorded ? ` (recorded with bit ${recorded}, running bit ${running})` : ''}` + + ` β€” snapped as side effects of the committed context; see the PR report:`, drift.map((d) => formatItem(`${d.id.toStringWithoutVersion()} (${d.changedKeys.join(', ')})`)) ) ); @@ -796,12 +805,40 @@ export class LaneSyncExecutor { keepLane: true, skipCleanup: true, noDestructiveRecovery: true, - snapIds: gitAuthored.map((id) => id.toStringWithoutVersion()), + verifyIds: gitAuthored.map((id) => id.toStringWithoutVersion()), driftIds: drift.map((d) => d.id.toStringWithoutVersion()), }); - return undefined; + return { drift }; } catch (e: any) { - return e instanceof Error ? e : new Error(String(e?.message ?? e)); + return { error: e instanceof Error ? e : new Error(String(e?.message ?? e)), drift: [] }; + } + } + + /** + * Upsert the drift-report comment on the branch's PR (spec 5.6): the enumerated effect of the + * fan-out the snap just committed. Silently a no-op with no configured git host (the run-log + * report above is the fallback surface), no open PR yet, or a provider that doesn't implement + * comment upserts. Never fails the lane β€” a report is a courtesy, not a gate. + */ + private async surfaceDriftOnPr(branch: string, drift: ContextDriftReport['drift']): Promise { + const { gitHost, logger } = this.deps; + const upsertComment = gitHost?.upsertComment?.bind(gitHost); + if (!upsertComment) return; + const pr = await this.findPr(branch); + if (!pr) return; + try { + if (drift.length) { + const running = this.deps.ci.getRunningBitVersion(); + await upsertComment(pr.number, DRIFT_COMMENT_MARKER, driftReportCommentBody(drift, running)); + } else { + // Only update a report that already exists β€” a lane with no drift that never had one must + // not gain a "drift cleared" comment out of nowhere. + await upsertComment(pr.number, DRIFT_COMMENT_MARKER, driftClearedCommentBody(), { createIfAbsent: false }); + } + } catch (e: any) { + logger.consoleWarning( + `Could not update the dependency-context drift report on PR #${pr.number}: ${e?.message || e}` + ); } } From 71eb04b001d14671f1e69ba45f7d05fd2f631208 Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 14:21:34 -0400 Subject: [PATCH 20/22] fix(ci): drift-report pagination, comment anchors, and the noop short-circuit - GitHubClient.upsertComment now paginates (per_page=100, follows Link rel="next") so a marked comment past page 1 is found instead of duplicated on every push. Renamed from upsertIssueComment to match GitHostProvider.upsertComment exactly, so a directly-registered client can't silently no-op. - surfaceDriftOnPr now runs after recordLaneHeadOnBranch (its anchors - branch tip sha, lane head - don't exist before that commit+push lands) and the drift-report body names the synced branch/lane pair. - snapAndExportOntoLane short-circuits before snapPrCommit when nothing is git-authored or drifted (the old cheap path). Restricted to executeExportBranch: executeMergeDiverged's merge step can rewrite .bitmap/content to match an already-recorded version with nothing new to snap, but still needs its branch commit - ignoring noop there was a real bug caught by the lane-wins e2e cell. - Reworded stale comments/messages that predated the fan-out flip (context-drift-detector.ts, verifyWorkspaceStatusInternal), and noted the snap-wide ignoreIssues coarseness (spec 5.2). Co-Authored-By: Claude Fable 5 --- scopes/git/ci/ci.main.runtime.ts | 15 ++- scopes/git/ci/sync/context-drift-detector.ts | 7 +- scopes/git/ci/sync/context-drift.spec.ts | 44 +++++++- scopes/git/ci/sync/context-drift.ts | 25 ++++- scopes/git/ci/sync/github-client.spec.ts | 48 ++++++++- scopes/git/ci/sync/github-client.ts | 46 ++++++-- scopes/git/ci/sync/lane-sync-executor.spec.ts | 100 ++++++++++++++++++ scopes/git/ci/sync/lane-sync-executor.ts | 73 ++++++++++--- 8 files changed, 320 insertions(+), 38 deletions(-) diff --git a/scopes/git/ci/ci.main.runtime.ts b/scopes/git/ci/ci.main.runtime.ts index d670b38d4525..48a033d94a4f 100644 --- a/scopes/git/ci/ci.main.runtime.ts +++ b/scopes/git/ci/ci.main.runtime.ts @@ -445,9 +445,10 @@ export class CiMain { } /** - * `verifyIds` scopes the status failure to the components this run snaps β€” a global failure would - * otherwise block every snap in the repo. The snap itself still enforces blockers on its own - * components. + * `verifyIds` scopes the status failure to a subset of what this run may snap β€” the git-authored + * change on a lane run, not the drifted components riding along with it (those are tolerated at + * snap time instead, via `driftIds`/`snapIgnoreIssues`). A global failure would otherwise block + * every snap in the repo. The snap itself still enforces blockers on every component it snaps. */ private async verifyWorkspaceStatusInternal( strict: boolean = false, @@ -475,7 +476,9 @@ export class CiMain { ({ code: effectiveCode } = await this.status.formatStatusOutput(scoped, formatOptions)); if (effectiveCode === 0) { this.logger.console( - formatWarningSummary('The issues above are on components this run does not snap β€” they do not block it') + formatWarningSummary( + "The issues above are on components outside this run's git-authored change β€” they do not fail this gate" + ) ); } } @@ -789,6 +792,10 @@ export class CiMain { // drifted component's files and config equal its recorded head, so its blockers are not new. // Computed here (against the status this call already loaded) rather than in the caller, so a // lane run doesn't pay for a second full `status()` pass. + // Coarse by construction (spec 5.2): `ignoreIssues` applies to the whole snap, not per component, + // so a blocker `syncConfigFromMain` freshly introduces on some OTHER component is also shadowed + // whenever its type happens to match one already tolerated for the drifted set. No re-verify + // step recovers from this; per-component granularity needs a snapping API change (out of scope). const snapIgnoreIssues = driftIds?.length ? blockerNamesUnion(status.componentsWithIssues, new Set(driftIds)) : undefined; diff --git a/scopes/git/ci/sync/context-drift-detector.ts b/scopes/git/ci/sync/context-drift-detector.ts index 774a74443fdd..80e74277b5e5 100644 --- a/scopes/git/ci/sync/context-drift-detector.ts +++ b/scopes/git/ci/sync/context-drift-detector.ts @@ -18,7 +18,7 @@ type DriftCheckResult = | { id: ComponentID; kind: 'drift'; recordedBitVersion?: string; changedKeys: string[] }; export type ContextDriftReport = { - /** dep-only diff vs the recorded version β€” never snapped by a lane run */ + /** dep-only diff vs the recorded version β€” a lane run snaps these too, as surfaced side effects */ drift: { id: ComponentID; recordedBitVersion?: string; changedKeys: string[] }[]; /** pending minus drift: new components and file/config-diff components */ gitAuthored: ComponentID[]; @@ -76,8 +76,9 @@ async function getRecordedBitVersion( */ export async function detectContextDrift(workspace: Workspace, logger: Logger): Promise { const pendingIds = await workspace.listTagPendingIds(); - // `export` refuses local-only components; a `legacyBitIds` snap bypasses the pending-path filter - // that normally removes them (Snapping.getTagPendingComponentsIds), so filter here. + // `export` refuses local-only components, and so does the snap's own tag-pending resolution + // (`Snapping.getTagPendingComponentsIds`). Filtering them here keeps this split aligned with what + // the snap actually snaps, or the drift/git-authored ids reported wouldn't match its outcome. const localOnly = ComponentIdList.fromArray(workspace.filter.byLocalOnly(pendingIds)); const pending = pendingIds.filter((id) => !localOnly.hasWithoutVersion(id)); const legacyScope = workspace.scope.legacyScope; diff --git a/scopes/git/ci/sync/context-drift.spec.ts b/scopes/git/ci/sync/context-drift.spec.ts index 2c59e3210b3b..9452868ecab2 100644 --- a/scopes/git/ci/sync/context-drift.spec.ts +++ b/scopes/git/ci/sync/context-drift.spec.ts @@ -1,5 +1,12 @@ import { expect } from 'chai'; -import { classifyDiffFields, convergenceMessage, blockerNamesUnion } from './context-drift'; +import { + classifyDiffFields, + convergenceMessage, + blockerNamesUnion, + driftReportCommentBody, + driftClearedCommentBody, + DRIFT_COMMENT_MARKER, +} from './context-drift'; describe('classifyDiffFields', () => { const unchanged = { filesChanged: false, pathSetsEqual: true }; @@ -95,3 +102,38 @@ describe('blockerNamesUnion', () => { expect(res).to.equal('CircularDependencies'); }); }); + +describe('driftReportCommentBody', () => { + const drift = [ + { + id: { toStringWithoutVersion: () => 'scope/comp2' }, + recordedBitVersion: '1.12.61', + changedKeys: ['packageDependencies'], + }, + ]; + const anchors = { + branch: 'drift-lane', + branchTipSha: 'abcdef0123456789', + laneIdStr: 'scope/drift-lane', + laneHead: '0123456789abcdef', + }; + + it('carries the marker, the cause, the synced pair, and the drifted component', () => { + const body = driftReportCommentBody(drift, '2.0.69', anchors); + expect(body).to.include(DRIFT_COMMENT_MARKER); + expect(body).to.include('recorded with bit 1.12.61, running bit 2.0.69'); + // the anchors: which push, which lane snap + expect(body).to.include('drift-lane'); + expect(body).to.include('abcdef012'); + expect(body).to.include('scope/drift-lane'); + expect(body).to.include('0123456789ab'.slice(0, 9)); + expect(body).to.include('scope/comp2'); + expect(body).to.include('packageDependencies'); + }); +}); + +describe('driftClearedCommentBody', () => { + it('carries the marker, so the upsert can find and update this exact comment', () => { + expect(driftClearedCommentBody()).to.include(DRIFT_COMMENT_MARKER); + }); +}); diff --git a/scopes/git/ci/sync/context-drift.ts b/scopes/git/ci/sync/context-drift.ts index b12e0af92766..6a6764b1ae6c 100644 --- a/scopes/git/ci/sync/context-drift.ts +++ b/scopes/git/ci/sync/context-drift.ts @@ -95,16 +95,34 @@ const MAX_LISTED_DRIFT_COMPONENTS = 50; type DriftEntry = { id: { toStringWithoutVersion(): string }; recordedBitVersion?: string; changedKeys: string[] }; +/** The synced pair a drift report names, so the comment identifies which push and which lane snap it covers. */ +export type DriftReportAnchors = { + branch: string; + /** the branch's tip commit after this run's sync commit */ + branchTipSha: string; + laneIdStr: string; + /** the lane's content fingerprint after this run's export (see `laneHeadFingerprint`) */ + laneHead: string; +}; + /** * The PR comment body for a lane snap that carried dependency-context drift (spec 5.6). States the - * cause, lists the affected components and their changed fields, and points at the change request - * for the full diff. `driftClearedCommentBody` is the counterpart once the drift is gone. + * cause, names the synced branch/lane pair, lists the affected components and their changed + * fields, and points at the change request for the full diff. `driftClearedCommentBody` is the + * counterpart once the drift is gone. */ -export function driftReportCommentBody(drift: DriftEntry[], runningBitVersion: string): string { +export function driftReportCommentBody( + drift: DriftEntry[], + runningBitVersion: string, + anchors: DriftReportAnchors +): string { const recorded = [...new Set(drift.map((d) => d.recordedBitVersion).filter(Boolean))].join(', '); const causeLine = recorded ? `Cause: the dependency context changed (recorded with bit ${recorded}, running bit ${runningBitVersion}).` : `Cause: the dependency context changed.`; + const anchorLine = + `Branch \`${anchors.branch}\` @ \`${anchors.branchTipSha.slice(0, 9)}\` synced onto lane ` + + `\`${anchors.laneIdStr}\` @ \`${anchors.laneHead.slice(0, 9)}\`.`; const entries = drift.map((d) => `- \`${d.id.toStringWithoutVersion()}\` β€” ${d.changedKeys.join(', ')}`); const list = entries.length > MAX_LISTED_DRIFT_COMPONENTS @@ -118,6 +136,7 @@ export function driftReportCommentBody(drift: DriftEntry[], runningBitVersion: s '### Dependency-context drift', '', causeLine, + anchorLine, 'This snap carries the components below as a side effect of the committed context. Git does not show this change.', '', ...list, diff --git a/scopes/git/ci/sync/github-client.spec.ts b/scopes/git/ci/sync/github-client.spec.ts index a329dd608453..964dec694364 100644 --- a/scopes/git/ci/sync/github-client.spec.ts +++ b/scopes/git/ci/sync/github-client.spec.ts @@ -172,7 +172,9 @@ describe('GitHubClient', () => { expect(await client.findPrByBranch('lane-x')).to.equal(undefined); }); - describe('upsertIssueComment', () => { + describe('upsertComment', () => { + // named `upsertComment`, matching `GitHostProvider.upsertComment` exactly β€” see the method's + // own doc comment for why a differently-named method would be a silent-no-op trap. function fakeFetchOver(existingComments: Array<{ id: number; body: string }>) { const calls: Array<{ url: string; init: any }> = []; const fetchImpl = (async (url: any, init: any) => { @@ -192,7 +194,7 @@ describe('GitHubClient', () => { it('posts a new comment when no marked comment exists', async () => { const { calls, fetchImpl } = fakeFetchOver([]); const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); - await client.upsertIssueComment(7, '', '\nbody'); + await client.upsertComment(7, '', '\nbody'); const posts = calls.filter((c) => c.init?.method === 'POST'); expect(posts).to.have.lengthOf(1); expect(posts[0].url).to.equal('https://api.github.com/repos/acme/shop/issues/7/comments'); @@ -204,7 +206,7 @@ describe('GitHubClient', () => { { id: 99, body: '\nold report' }, ]); const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); - await client.upsertIssueComment(7, '', '\nnew report'); + await client.upsertComment(7, '', '\nnew report'); const patches = calls.filter((c) => c.init?.method === 'PATCH'); expect(patches).to.have.lengthOf(1); expect(patches[0].url).to.equal('https://api.github.com/repos/acme/shop/issues/comments/99'); @@ -215,8 +217,46 @@ describe('GitHubClient', () => { it('skips silently when createIfAbsent is false and no marked comment exists', async () => { const { calls, fetchImpl } = fakeFetchOver([]); const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); - await client.upsertIssueComment(7, '', 'cleared', { createIfAbsent: false }); + await client.upsertComment(7, '', 'cleared', { createIfAbsent: false }); expect(calls.some((c) => c.init?.method === 'POST' || c.init?.method === 'PATCH')).to.equal(false); }); + + it('lists at per_page=100 and follows Link-header pagination to find a comment past page 1', async () => { + // GitHub defaults to per_page=30; a naive single-page list would miss this comment and post a + // duplicate report on every push instead of updating it. + const calls: Array<{ url: string; init: any }> = []; + const fetchImpl = (async (url: any, init: any) => { + calls.push({ url: String(url), init }); + const method = init?.method ?? 'GET'; + if (method === 'GET' && !String(url).includes('page=2')) { + return new Response(JSON.stringify([{ id: 1, body: 'unrelated' }]), { + status: 200, + headers: { + 'content-type': 'application/json', + link: '; rel="next"', + }, + }); + } + if (method === 'GET') { + return new Response(JSON.stringify([{ id: 99, body: '\nold report' }]), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + } + return new Response(JSON.stringify({}), { status: 200, headers: { 'content-type': 'application/json' } }); + }) as typeof fetch; + const client = new GitHubClient({ token: 'tok', repo: 'acme/shop', fetchImpl }); + await client.upsertComment(7, '', '\nnew report'); + + const gets = calls.filter((c) => (c.init?.method ?? 'GET') === 'GET'); + expect(gets).to.have.lengthOf(2); + expect(gets[0].url).to.include('per_page=100'); + expect(gets[1].url).to.equal('https://api.github.com/repos/acme/shop/issues/7/comments?per_page=100&page=2'); + const patches = calls.filter((c) => c.init?.method === 'PATCH'); + expect(patches).to.have.lengthOf(1); + expect(patches[0].url).to.equal('https://api.github.com/repos/acme/shop/issues/comments/99'); + // the found-on-page-2 comment was updated, not duplicated + expect(calls.some((c) => c.init?.method === 'POST')).to.equal(false); + }); }); }); diff --git a/scopes/git/ci/sync/github-client.ts b/scopes/git/ci/sync/github-client.ts index f09c79f6c432..d652a4b45b2f 100644 --- a/scopes/git/ci/sync/github-client.ts +++ b/scopes/git/ci/sync/github-client.ts @@ -32,6 +32,13 @@ export function isGitHubRemote(remoteUrl: string): boolean { const API = 'https://api.github.com'; +/** The `rel="next"` URL out of a GitHub `Link` response header, or undefined on the last page. */ +function nextPageUrl(linkHeader: string | null): string | undefined { + if (!linkHeader) return undefined; + const next = linkHeader.split(',').find((part) => part.includes('rel="next"')); + return next?.match(/<([^>]+)>/)?.[1]; +} + /** Warning sink; a plain callback rather than a `Logger` so this module needs no logger aspect. */ export type WarnFn = (message: string) => void; @@ -85,8 +92,10 @@ export class GitHubClient implements GitHostProvider { return Boolean(this.token && this.repo); } - private async request(method: string, path: string, body?: unknown): Promise { - const res = await this.fetchImpl(`${API}/repos/${this.repo}${path}`, { + /** `pathOrUrl` may be a path relative to this repo, or an absolute URL (a `Link` header's next page). */ + private async requestRaw(method: string, pathOrUrl: string, body?: unknown): Promise { + const url = /^https?:\/\//i.test(pathOrUrl) ? pathOrUrl : `${API}/repos/${this.repo}${pathOrUrl}`; + const res = await this.fetchImpl(url, { method, headers: { authorization: `Bearer ${this.token}`, @@ -98,8 +107,13 @@ export class GitHubClient implements GitHostProvider { }); if (!res.ok) { const text = await res.text().catch(() => ''); - throw new Error(`GitHub API ${method} ${path} failed: ${res.status} ${text}`); + throw new Error(`GitHub API ${method} ${pathOrUrl} failed: ${res.status} ${text}`); } + return res; + } + + private async request(method: string, path: string, body?: unknown): Promise { + const res = await this.requestRaw(method, path, body); return res.status === 204 ? undefined : res.json(); } @@ -136,9 +150,22 @@ export class GitHubClient implements GitHostProvider { await this.request('POST', `/issues/${prNumber}/labels`, { labels: [label] }); } + /** + * Every comment on the PR, across all pages. GitHub defaults `per_page` to 30; a marked comment + * living past page 1 would otherwise read as absent, and `upsertComment` would post a duplicate on + * every push instead of updating the existing one. Follows the `Link: rel="next"` header β€” a page + * with no such link is the last one. + */ private async listIssueComments(prNumber: number): Promise<{ id: number; body: string }[]> { - const list = await this.request('GET', `/issues/${prNumber}/comments`); - return (list ?? []).map((c: any) => ({ id: c.id, body: c.body ?? '' })); + const comments: { id: number; body: string }[] = []; + let next: string | undefined = `/issues/${prNumber}/comments?per_page=100`; + while (next) { + const res: Response = await this.requestRaw('GET', next); + const page = (await res.json()) as any[]; + comments.push(...page.map((c: any) => ({ id: c.id, body: c.body ?? '' }))); + next = nextPageUrl(res.headers.get('link')); + } + return comments; } /** @@ -146,8 +173,13 @@ export class GitHubClient implements GitHostProvider { * or post `body` as a new comment when none exists and `options.createIfAbsent` is not false. * GitHub's comment PATCH endpoint is `/issues/comments/{id}`, not `/issues/{pr}/comments/{id}` β€” * a comment id is unique per repository, not scoped under the issue/PR that carries it. + * + * Named to match `GitHostProvider.upsertComment` exactly (not e.g. `upsertIssueComment`): a + * `GitHubClient` registered directly as a provider must satisfy the interface's optional method + * under its real name, or callers that feature-test via `gitHost.upsertComment` would silently + * treat a fully-capable client as unsupported. */ - async upsertIssueComment( + async upsertComment( prNumber: number, marker: string, body: string, @@ -223,7 +255,7 @@ export class GitHubHostProvider implements GitHostProvider { body: string, options?: { createIfAbsent?: boolean } ): Promise { - return this.requireClient().upsertIssueComment(prNumber, marker, body, options); + return this.requireClient().upsertComment(prNumber, marker, body, options); } private resolveClient(remoteUrl?: string): GitHubClient | undefined { diff --git a/scopes/git/ci/sync/lane-sync-executor.spec.ts b/scopes/git/ci/sync/lane-sync-executor.spec.ts index 60082a2d2be7..e346840fc392 100644 --- a/scopes/git/ci/sync/lane-sync-executor.spec.ts +++ b/scopes/git/ci/sync/lane-sync-executor.spec.ts @@ -15,6 +15,7 @@ import { laneSyncPrBody, } from './lane-sync-executor'; import { resolveSyncConfig } from './sync-config'; +import { DRIFT_COMMENT_MARKER } from './context-drift'; type LaneComponents = Parameters[0]; @@ -256,6 +257,105 @@ describe('syncLane outer catch under --dry-run', () => { }); }); +describe('surfaceDriftOnPr', () => { + const ANCHORS = { + branch: 'drift-lane', + branchTipSha: 'abcdef0123456789', + laneIdStr: 'acme.shop/drift-lane', + laneHead: '0123456789abcdef', + }; + const DRIFT = [{ id: { toStringWithoutVersion: () => 'acme.shop/comp2' }, changedKeys: ['packageDependencies'] }]; + + /** `upsertCalls` records every `upsertComment` invocation; `findPrCalls` every PR lookup. */ + function executorWith(opts: { + gitHost?: 'none' | 'no-upsert' | 'ok' | 'throws'; + upsertCalls?: any[]; + findPrCalls?: string[]; + }) { + const upsertCalls = opts.upsertCalls ?? []; + const findPrCalls = opts.findPrCalls ?? []; + const warnings: string[] = []; + const noopLogger = { + console: () => {}, + consoleWarning: (m: string) => warnings.push(m), + error: () => {}, + debug: () => {}, + }; + const findPrByBranch = async (branch: string) => { + findPrCalls.push(branch); + return { number: 7, htmlUrl: 'https://example.test/pr/7', labels: [] }; + }; + const gitHost = + opts.gitHost === 'none' + ? undefined + : opts.gitHost === 'no-upsert' + ? ({ name: 'stub', findPrByBranch } as any) + : ({ + name: 'stub', + findPrByBranch, + upsertComment: async (...args: any[]) => { + upsertCalls.push(args); + if (opts.gitHost === 'throws') throw new Error('rate limited'); + }, + } as any); + const executor = new LaneSyncExecutor({ + lanes: {} as any, + ci: { getRunningBitVersion: () => '2.0.69' } as any, + logger: noopLogger as any, + gitHost, + cfg: resolveSyncConfig({}), + defaultScope: 'acme.shop', + }); + return { executor, upsertCalls, findPrCalls, warnings }; + } + + it('drift present: upserts a report body naming the synced branch/lane pair and the drifted component', async () => { + const { executor, upsertCalls } = executorWith({ gitHost: 'ok' }); + await (executor as any).surfaceDriftOnPr({ ...ANCHORS, drift: DRIFT }); + expect(upsertCalls).to.have.lengthOf(1); + const [prNumber, marker, body, options] = upsertCalls[0]; + expect(prNumber).to.equal(7); + expect(marker).to.equal(DRIFT_COMMENT_MARKER); + expect(body).to.include(ANCHORS.branch); + expect(body).to.include(ANCHORS.branchTipSha.slice(0, 9)); + expect(body).to.include(ANCHORS.laneIdStr); + expect(body).to.include(ANCHORS.laneHead.slice(0, 9)); + expect(body).to.include('acme.shop/comp2'); + expect(options).to.equal(undefined); // createIfAbsent defaults to true β€” a report must always land + }); + + it('drift empty: upserts the cleared body with createIfAbsent false β€” never posts a fresh "cleared" comment', async () => { + const { executor, upsertCalls } = executorWith({ gitHost: 'ok' }); + await (executor as any).surfaceDriftOnPr({ ...ANCHORS, drift: [] }); + expect(upsertCalls).to.have.lengthOf(1); + const [, marker, , options] = upsertCalls[0]; + expect(marker).to.equal(DRIFT_COMMENT_MARKER); + expect(options).to.deep.equal({ createIfAbsent: false }); + }); + + it('no configured git host: skips silently, without even looking up the PR', async () => { + const { executor, upsertCalls, findPrCalls } = executorWith({ gitHost: 'none' }); + await (executor as any).surfaceDriftOnPr({ ...ANCHORS, drift: DRIFT }); + expect(upsertCalls).to.deep.equal([]); + expect(findPrCalls).to.deep.equal([]); + }); + + it('a provider without upsertComment: skips silently, the same as no provider', async () => { + const { executor, upsertCalls, findPrCalls } = executorWith({ gitHost: 'no-upsert' }); + await (executor as any).surfaceDriftOnPr({ ...ANCHORS, drift: DRIFT }); + expect(upsertCalls).to.deep.equal([]); + expect(findPrCalls).to.deep.equal([]); + }); + + it('the git host API throws: warns, and does not fail the caller', async () => { + const { executor, warnings } = executorWith({ gitHost: 'throws' }); + await (executor as any).surfaceDriftOnPr({ ...ANCHORS, drift: DRIFT }); // must not reject + expect(warnings).to.have.lengthOf(1); + expect(warnings[0]).to.include('rate limited'); + expect(warnings[0]).to.include('#7'); + }); +}); + // bit's lane-name charset admits `$`, `-`, `_` and `!`, so an unquoted runbook shell-expands when pasted. describe('haltCommentBody', () => { const LANE_NAME = 'fix-$home-and-!bang'; diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 673d69e62dec..0bc8a3e53e3e 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -10,6 +10,7 @@ import type { CiMain, ContextDriftReport } from '../ci.main.runtime'; import type { CiSyncConfig, LaneTarget } from './sync-config'; import { laneNameToBranch } from './sync-config'; import { DRIFT_COMMENT_MARKER, driftClearedCommentBody, driftReportCommentBody } from './context-drift'; +import type { DriftReportAnchors } from './context-drift'; import type { BranchSyncState } from './sync-state'; import { CONFLICT_LABEL, @@ -611,7 +612,7 @@ export class LaneSyncExecutor { await this.checkoutFromRemote(branch, `origin/${branch}`); try { - const { error: exportErr, drift } = await this.snapAndExportOntoLane(laneIdStr, message); + const { error: exportErr, drift, noop } = await this.snapAndExportOntoLane(laneIdStr, message); if (exportErr) { // Halt rather than propagate: one lane's failed snap/export must not abort the lanes after it. return await this.executeHalt({ @@ -622,10 +623,12 @@ export class LaneSyncExecutor { pr: await this.findPr(branch), }); } - await this.surfaceDriftOnPr(branch, drift); + if (noop) { + return `${laneName} -> export-branch (branch ${branch} has no bit-tracked change; nothing snapped onto lane ${laneIdStr})`; + } - const laneHead = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); - if (!laneHead) { + const recorded = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); + if (!recorded) { return await this.executeHalt({ laneName, laneIdStr, @@ -634,6 +637,10 @@ export class LaneSyncExecutor { pr: await this.findPr(branch), }); } + const { laneHead, branchTipSha } = recorded; + // After recordLaneHeadOnBranch, not before: the report names the branch tip and lane head this + // run just produced, and neither exists until the commit+push above lands. + await this.surfaceDriftOnPr({ branch, branchTipSha, laneIdStr, laneHead, drift }); return `${laneName} -> export-branch (lane ${laneIdStr} @ ${laneHead.slice(0, 9)}, branch ${branch} updated)`; } finally { await this.restoreWorkspace(defaultBranch); @@ -730,6 +737,11 @@ export class LaneSyncExecutor { } // ---- step 2: snap + export the merged tree onto the lane --------------------------------- + // `noop` (see `snapAndExportOntoLane`) is deliberately IGNORED here, unlike in + // `executeExportBranch`: step 1's checkout can rewrite `.bitmap` and file content to match an + // ALREADY-recorded version (e.g. a full-file conflict resolved by "theirs") β€” nothing new to + // snap, but a real git-level change the branch does not have yet. Skipping the branch commit + // on `noop` would silently drop that change. const { error: exportErr, drift } = await this.snapAndExportOntoLane( laneIdStr, `merge remote lane ${laneIdStr} into ${branch} ${SYNC_COMMIT_MARKER}` @@ -740,13 +752,15 @@ export class LaneSyncExecutor { `result failed: ${exportErr.message}` ); } - await this.surfaceDriftOnPr(branch, drift); // ---- step 3: record the new lane head on the branch -------------------------------------- - const laneHead = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); - if (!laneHead) { + const recorded = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); + if (!recorded) { return await halt(`lane ${laneIdStr} could not be read back from the remote after the merge export`); } + const { laneHead, branchTipSha } = recorded; + // After recordLaneHeadOnBranch, not before β€” see executeExportBranch for why. + await this.surfaceDriftOnPr({ branch, branchTipSha, laneIdStr, laneHead, drift }); return ( `${laneName} -> merge-diverged (${policyClause}merged lane into branch, then exported; lane ${laneIdStr} @ ` + `${laneHead.slice(0, 9)}, branch ${branch} updated)` @@ -776,14 +790,24 @@ export class LaneSyncExecutor { * verification still scopes to the git-authored subset (`verifyIds`); the drifted subset's * pre-existing blockers are tolerated at snap time via `driftIds` (see `blockerNamesUnion`), never * at verification time β€” a *new* blocker on a component the developer touched still fails the run. + * + * `noop: true` when neither set has anything (the old cheap path, restored): a docs-only commit or + * an already-converged merge touches no bit component, and `snapPrCommit` β€” full status, import, + * lane creation/reuse, config sync β€” is pure overhead for that case, minutes of it on a large + * workspace. The caller skips its own follow-up (drift comment, `.bitmap` sync commit) too: there + * is nothing to report and nothing new to record on the branch. */ private async snapAndExportOntoLane( laneIdStr: string, message: string - ): Promise<{ error?: Error; drift: ContextDriftReport['drift'] }> { + ): Promise<{ error?: Error; drift: ContextDriftReport['drift']; noop?: boolean }> { try { await ensureCurrentLaneObject(this.deps.lanes); const { drift, gitAuthored } = await this.deps.ci.detectContextDrift(); + if (!gitAuthored.length && !drift.length) { + this.deps.logger.console(chalk.yellow('No changes detected, nothing to snap')); + return { drift: [], noop: true }; + } if (drift.length) { const running = this.deps.ci.getRunningBitVersion(); const recorded = [...new Set(drift.map((d) => d.recordedBitVersion).filter(Boolean))].join(', '); @@ -818,9 +842,17 @@ export class LaneSyncExecutor { * Upsert the drift-report comment on the branch's PR (spec 5.6): the enumerated effect of the * fan-out the snap just committed. Silently a no-op with no configured git host (the run-log * report above is the fallback surface), no open PR yet, or a provider that doesn't implement - * comment upserts. Never fails the lane β€” a report is a courtesy, not a gate. + * comment upserts. Never fails the lane β€” a report is a courtesy, not a gate. `anchors` must come + * from `recordLaneHeadOnBranch`'s result β€” the branch tip and lane head it names don't exist + * until that commit+push lands, so callers invoke this AFTER it, never before. */ - private async surfaceDriftOnPr(branch: string, drift: ContextDriftReport['drift']): Promise { + private async surfaceDriftOnPr({ + branch, + branchTipSha, + laneIdStr, + laneHead, + drift, + }: DriftReportAnchors & { drift: ContextDriftReport['drift'] }): Promise { const { gitHost, logger } = this.deps; const upsertComment = gitHost?.upsertComment?.bind(gitHost); if (!upsertComment) return; @@ -829,7 +861,11 @@ export class LaneSyncExecutor { try { if (drift.length) { const running = this.deps.ci.getRunningBitVersion(); - await upsertComment(pr.number, DRIFT_COMMENT_MARKER, driftReportCommentBody(drift, running)); + await upsertComment( + pr.number, + DRIFT_COMMENT_MARKER, + driftReportCommentBody(drift, running, { branch, branchTipSha, laneIdStr, laneHead }) + ); } else { // Only update a report that already exists β€” a lane with no drift that never had one must // not gain a "drift cleared" comment out of nowhere. @@ -845,18 +881,20 @@ export class LaneSyncExecutor { /** * Record on the branch which lane state it now mirrors: re-query the lane (the export just moved it, * so any earlier fingerprint is stale), commit β€” crucially the `.bitmap` the snap rewrote β€” and push. - * Returns undefined when the lane can no longer be read, in which case the caller halts. + * Returns undefined when the lane can no longer be read, in which case the caller halts. The + * returned `branchTipSha` is the pushed commit β€” the anchor a drift report names alongside + * `laneHead`, so the two only exist together once this call succeeds (see `surfaceDriftOnPr`). */ private async recordLaneHeadOnBranch( target: LaneTarget, laneIdStr: string, branch: string - ): Promise { + ): Promise<{ laneHead: string; branchTipSha: string } | undefined> { const remoteLane = await this.getRemoteLane(target); if (!remoteLane) return undefined; const laneHead = laneHeadFingerprint(remoteLane.components); - await this.commitAllAndPush(branch, buildSyncCommitMessage(laneIdStr, laneHead)); - return laneHead; + const branchTipSha = await this.commitAllAndPush(branch, buildSyncCommitMessage(laneIdStr, laneHead)); + return { laneHead, branchTipSha }; } /** @@ -1270,13 +1308,16 @@ export class LaneSyncExecutor { * push means someone pushed concurrently and the next run should re-plan rather than clobber. * `--allow-empty` is only insurance against `git commit` failing the lane outright. */ - private async commitAllAndPush(branch: string, message: string) { + /** Returns the pushed commit's sha β€” the branch tip anchor a drift report names. */ + private async commitAllAndPush(branch: string, message: string): Promise { await addAllExceptScopeAndModules(); await commitWithIdentity(message, { extraArgs: ['--allow-empty'] }); + const tipSha = (await git.revparse(['HEAD'])).trim(); // `HEAD:refs/heads/`: a full-ref destination cannot be resolved as a tag or reinterpreted β€” // the configured branch name is user input (see `sync-config.ts`). await git.push(['origin', `HEAD:refs/heads/${branch}`]); this.deps.logger.console(chalk.green(`Pushed ${branch}`)); + return tipSha; } private async openPrForLane({ From d39f360ee86a96e88dcd47498de441b9f19a97ab Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 14:50:06 -0400 Subject: [PATCH 21/22] fix(ci): settle the docs-only-commit loop, cap pagination pages A commit touching no bit-tracked file (docs, CI config) never advances stateCommit (sync-state.ts derives it from .bitmap's content, never commit messages), so the nothing-pending noop kept re-planning export-branch forever, one per scheduled run. Writing the sync-ledger commit alone doesn't fix it: an --allow-empty commit with no staged diff can't make a path-filtered `git log -- .bitmap` see a change. Fix: reconcileLane recognizes its own commit as the branch tip (isSyncAuthoredMessage) and settles on "noop (converged)" before redoing export-branch's work, instead of loosening hasDevCommits itself (the ownership/retirement path also reads it and must stay strict). executeExportBranch still writes the ledger commit on the noop path so the tip actually advances to something recognizable. Also caps GitHubClient's comment-pagination loop at 20 pages, a defensive bound against a malformed or adversarial Link header. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 35 ++++++++++++++++++++++++ scopes/git/ci/sync/github-client.ts | 8 ++++-- scopes/git/ci/sync/lane-sync-executor.ts | 26 ++++++++++++++---- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 259084ab2fd3..87d53386544c 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1156,6 +1156,41 @@ describe('bit ci sync', function () { }); }); + // The nothing-pending short-circuit (executeExportBranch) still writes the sync-ledger commit, or + // a docs-only commit (touches no bit-tracked file) would leave `stateCommit` (sync-state.ts, derived + // from `.bitmap`'s content, never commit messages) stuck behind it forever β€” `hasDevCommits` would + // stay true on every future run. The reconciler settles instead by recognizing its OWN tip: once + // that ledger commit is the branch's tip, a later run stops before redoing any export-branch work. + describe('a commit that touches no bit-tracked file settles instead of looping', () => { + const LANE = 'docs-only-lane'; + let defaultBranch: string; + + before(() => { + ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); + createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); + seedSync(LANE); + branchSideCommit(LANE, defaultBranch, 'NOTES.md', '# notes\n', 'docs: add notes'); + }); + + it('reports nothing to snap once, then settles β€” the second run does not redo export-branch work', () => { + const first = syncRun(LANE); + expect(first.exitCode, `bit ci sync output:\n${first.output}`).to.equal(0); + // pins the noop summary's exact wording + expect(first.output).to.include( + `${LANE} -> export-branch (branch ${LANE} has no bit-tracked change; nothing snapped onto lane` + ); + + const second = syncRun(LANE); + expect(second.exitCode, `bit ci sync output:\n${second.output}`).to.equal(0); + // pins the settled summary's exact wording + expect(second.output).to.include( + `${LANE} -> noop (converged; branch tip is already this reconciler's own sync commit)` + ); + // executeExportBranch's own work (the checkout, the snap attempt) never ran a second time + expect(second.output).to.not.include('Exporting branch'); + }); + }); + // This reproduces an engine bump with one bit binary: a committed root policy moves a recorded // package range. Lanes carry the fan-out (spec decision 6): the lane run snaps the drifted // component together with the git-authored change, in one snap under the developer's own diff --git a/scopes/git/ci/sync/github-client.ts b/scopes/git/ci/sync/github-client.ts index d652a4b45b2f..9f3e3c3e419c 100644 --- a/scopes/git/ci/sync/github-client.ts +++ b/scopes/git/ci/sync/github-client.ts @@ -159,10 +159,12 @@ export class GitHubClient implements GitHostProvider { private async listIssueComments(prNumber: number): Promise<{ id: number; body: string }[]> { const comments: { id: number; body: string }[] = []; let next: string | undefined = `/issues/${prNumber}/comments?per_page=100`; - while (next) { + // Defensive cap: 20 pages (2,000 comments) is far past any real PR; without it a malformed or + // adversarial `Link` header could loop this call forever. + for (let page = 0; next && page < 20; page += 1) { const res: Response = await this.requestRaw('GET', next); - const page = (await res.json()) as any[]; - comments.push(...page.map((c: any) => ({ id: c.id, body: c.body ?? '' }))); + const body = (await res.json()) as any[]; + comments.push(...body.map((c: any) => ({ id: c.id, body: c.body ?? '' }))); next = nextPageUrl(res.headers.get('link')); } return comments; diff --git a/scopes/git/ci/sync/lane-sync-executor.ts b/scopes/git/ci/sync/lane-sync-executor.ts index 0bc8a3e53e3e..68050859cdf1 100644 --- a/scopes/git/ci/sync/lane-sync-executor.ts +++ b/scopes/git/ci/sync/lane-sync-executor.ts @@ -447,6 +447,17 @@ export class LaneSyncExecutor { pr, }); case 'export-branch': + // The tip is already this reconciler's own commit β€” it already confirmed everything up to + // it (that is what writing it means), and `hasDevCommits`/`stateCommit` cannot tell a real + // dev commit from one that touches no bit-tracked file (docs, CI config): `stateCommit` is + // derived from `.bitmap`'s content, never from commit messages (sync-state.ts), so a commit + // that leaves `.bitmap` byte-identical never advances it, however many runs re-confirm + // "nothing to snap" on top. Recognizing our own tip here β€” not by loosening `hasDevCommits` + // itself, which the ownership/retirement path also reads and must stay strict β€” is what + // makes that settle instead of re-planning `export-branch` forever. + if (tipIsSyncCommit) { + return `${laneName} -> noop (converged; branch tip is already this reconciler's own sync commit)`; + } return this.executeExportBranch({ target, laneIdStr, branch, defaultBranch }); case 'merge-diverged': return this.executeMergeDiverged({ target, laneIdStr, branch, defaultBranch }); @@ -623,20 +634,25 @@ export class LaneSyncExecutor { pr: await this.findPr(branch), }); } - if (noop) { - return `${laneName} -> export-branch (branch ${branch} has no bit-tracked change; nothing snapped onto lane ${laneIdStr})`; - } - + // Still write the sync ledger on a noop: without it, `stateCommit` (sync-state.ts) never + // advances past a commit that touches no bit-tracked file (docs, CI config), and the planner + // re-plans `export-branch` on every future run forever β€” cheap per run, but a standing loop + // that never settles. `recordLaneHeadOnBranch`'s commit is `--allow-empty`. const recorded = await this.recordLaneHeadOnBranch(target, laneIdStr, branch); if (!recorded) { return await this.executeHalt({ laneName, laneIdStr, branch, - reason: `lane ${laneIdStr} could not be read back from the remote after export`, + reason: noop + ? `lane ${laneIdStr} could not be read back from the remote while recording the no-op sync ledger` + : `lane ${laneIdStr} could not be read back from the remote after export`, pr: await this.findPr(branch), }); } + if (noop) { + return `${laneName} -> export-branch (branch ${branch} has no bit-tracked change; nothing snapped onto lane ${laneIdStr})`; + } const { laneHead, branchTipSha } = recorded; // After recordLaneHeadOnBranch, not before: the report names the branch tip and lane head this // run just produced, and neither exists until the commit+push above lands. From 1c2261eab3390b5d9dd3b87538deadc157e9972f Mon Sep 17 00:00:00 2001 From: Luv Kapur Date: Fri, 7 Aug 2026 14:56:37 -0400 Subject: [PATCH 22/22] test(ci): pin that a real dev commit clears the settled-tip withhold Extends the docs-only-lane e2e cell: after the second run settles on "noop (converged; branch tip is already this reconciler's own sync commit)", a real dev commit on comp1 must still export normally on the next run (the tip is no longer the reconciler's own). Settles, not traps. Also notes on isSyncAuthoredMessage's doc comment that the strict probe now also feeds the export-branch withhold, not only branch deletion. Co-Authored-By: Claude Fable 5 --- e2e/harmony/ci-sync.e2e.ts | 21 ++++++++++++++++++++- scopes/git/ci/sync/sync-state.ts | 5 +++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/e2e/harmony/ci-sync.e2e.ts b/e2e/harmony/ci-sync.e2e.ts index 87d53386544c..c3f07054a039 100644 --- a/e2e/harmony/ci-sync.e2e.ts +++ b/e2e/harmony/ci-sync.e2e.ts @@ -1164,10 +1164,11 @@ describe('bit ci sync', function () { describe('a commit that touches no bit-tracked file settles instead of looping', () => { const LANE = 'docs-only-lane'; let defaultBranch: string; + let devPath: string; before(() => { ({ defaultBranch } = setupSyncWorkspace({ lanes: ['*'] })); - createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); + devPath = createLaneWithSnap(LANE, { 'comp1/index.js': comp1Src('lane-snap-1') }, 'lane snap 1'); seedSync(LANE); branchSideCommit(LANE, defaultBranch, 'NOTES.md', '# notes\n', 'docs: add notes'); }); @@ -1189,6 +1190,24 @@ describe('bit ci sync', function () { // executeExportBranch's own work (the checkout, the snap attempt) never ran a second time expect(second.output).to.not.include('Exporting branch'); }); + + // The withhold settles; it does not trap. A real dev commit on top of the recognized ledger tip + // must still clear it and export normally β€” the tip is no longer the reconciler's own commit. + it('a real dev commit on top of the settled tip clears the withhold and exports again', () => { + branchSideCommit( + LANE, + defaultBranch, + 'comp1/index.js', + comp1Src('dev-commit-after-settle'), + 'dev commit after settling' + ); + const { output, exitCode } = syncRun(LANE); + expect(exitCode, `bit ci sync output:\n${output}`).to.equal(0); + expect(output).to.include('Exporting branch'); + expect(output).to.include(`${LANE} -> export-branch`); + expect(output).to.not.include('branch tip is already this reconciler'); + expect(laneTipFile(devPath, 'comp1/index.js')).to.include('dev-commit-after-settle'); + }); }); // This reproduces an engine bump with one bit binary: a committed root policy moves a recorded diff --git a/scopes/git/ci/sync/sync-state.ts b/scopes/git/ci/sync/sync-state.ts index c0555d39f206..cc33efefeea3 100644 --- a/scopes/git/ci/sync/sync-state.ts +++ b/scopes/git/ci/sync/sync-state.ts @@ -86,8 +86,9 @@ export function hasSyncMarker(message: string): boolean { /** * Strict probe for "we wrote this commit": the marker alone on its own line. This is an input to branch - * deletion, so a developer merely quoting the marker must never count as authorship. `\r?` tolerates CRLF; - * a recognition failure errs toward keeping the branch. + * deletion and to the export-branch withhold (a branch whose tip is already our own commit settles + * instead of re-exporting), so a developer merely quoting the marker must never count as authorship. + * `\r?` tolerates CRLF; a recognition failure errs toward keeping the branch / re-attempting the export. */ export function isSyncAuthoredMessage(message: string): boolean { return new RegExp(`^${SYNC_COMMIT_MARKER.replace(/[[\]]/g, '\\$&')}\\r?$`, 'm').test(message);