From 701eb4bae47a809e6da34c62b8e250ac6379db53 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 3 Feb 2026 18:15:38 +0000 Subject: [PATCH] fix(cli): prevent branch-specific items from being marked for deletion on pull (#7781) The elementsToMap function was incorrectly skipping remote base files that were configured as branch-specific, causing pull to mark them for deletion. Root cause: PR #7643 added a check to skip base files when configured as branch-specific, but this was applied to both local AND remote sources. Remote workspace files only have base paths (e.g., TestVar.variable.yaml), not branch-specific paths (e.g., TestVar.staging.variable.yaml). Fix: Add isRemote parameter to elementsToMap to distinguish remote vs local processing. Only skip base files for local sources where we expect the branch-specific version to exist. Co-authored-by: Claude Opus 4.5 --- cli/src/commands/sync/sync.ts | 13 +- .../elements_to_map_branch_specific.test.ts | 476 ++++++++++++++++++ 2 files changed, 485 insertions(+), 4 deletions(-) create mode 100644 cli/test/elements_to_map_branch_specific.test.ts diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 55543e7b4d..251ef4148b 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -1280,6 +1280,7 @@ export async function elementsToMap( skips: Skips, specificItems?: SpecificItemsConfig, branchOverride?: string, + isRemote?: boolean, ): Promise<{ [key: string]: string }> { const map: { [key: string]: string } = {}; const processedBasePaths = new Set(); @@ -1446,7 +1447,8 @@ export async function elementsToMap( continue; } // Skip base file if it's configured as branch-specific (expect branch version) - if (isSpecificItem(path, specificItems)) { + // Only for LOCAL files - remote workspace only has base paths + if (!isRemote && isSpecificItem(path, specificItems)) { continue; } map[path] = content; @@ -1486,13 +1488,14 @@ async function compareDynFSElement( ignoreCodebaseChanges: boolean, specificItems?: SpecificItemsConfig, branchOverride?: string, + isEls1Remote?: boolean, ): Promise { const [m1, m2] = els2 ? await Promise.all([ - elementsToMap(els1, ignore, json, skips, specificItems, branchOverride), - elementsToMap(els2, ignore, json, skips, specificItems, branchOverride), + elementsToMap(els1, ignore, json, skips, specificItems, branchOverride, isEls1Remote), + elementsToMap(els2, ignore, json, skips, specificItems, branchOverride, !isEls1Remote), ]) - : [await elementsToMap(els1, ignore, json, skips, specificItems, branchOverride), {}]; + : [await elementsToMap(els1, ignore, json, skips, specificItems, branchOverride, isEls1Remote), {}]; const changes: Change[] = []; @@ -1995,6 +1998,7 @@ export async function pull( true, specificItems, opts.branch, + true, // els1 (remote) is the remote source ); log.info( @@ -2486,6 +2490,7 @@ export async function push( false, specificItems, opts.branch, + false, // els1 (local) is not the remote source ); const rawWorkspaceDependencies = await getRawWorkspaceDependencies(); diff --git a/cli/test/elements_to_map_branch_specific.test.ts b/cli/test/elements_to_map_branch_specific.test.ts new file mode 100644 index 0000000000..03af7d52ba --- /dev/null +++ b/cli/test/elements_to_map_branch_specific.test.ts @@ -0,0 +1,476 @@ +import { assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts"; + +// Import the function we need to test +import { elementsToMap } from "../src/commands/sync/sync.ts"; +import type { SpecificItemsConfig } from "../src/core/specific_items.ts"; + +// ============================================================================= +// elementsToMap TESTS FOR BRANCH-SPECIFIC ITEMS +// Tests for the regression where remote base files were incorrectly skipped +// when configured as branch-specific, causing them to be marked for deletion +// on pull operations. +// +// Regression: PR #7643 (commit 287b7e7d9, Jan 21, 2026) +// ============================================================================= + +/** + * Mock DynFSElement implementation for testing + */ +interface MockFile { + path: string; + content: string; + isDirectory?: boolean; +} + +function createMockDynFSElement(files: MockFile[]): { + isDirectory: boolean; + path: string; + getContentText(): Promise; + getChildren(): AsyncIterable<{ + isDirectory: boolean; + path: string; + getContentText(): Promise; + getChildren(): AsyncIterable; + }>; +} { + return { + isDirectory: true, + path: "", + async getContentText() { + return ""; + }, + async *getChildren() { + for (const file of files) { + yield { + isDirectory: file.isDirectory ?? false, + path: file.path, + async getContentText() { + return file.content; + }, + async *getChildren() { + // No children for files + }, + }; + } + }, + }; +} + +const noIgnore = () => false; +const defaultSkips = {}; + +// ============================================================================= +// REGRESSION TEST: Remote base files should NOT be skipped +// ============================================================================= + +Deno.test("elementsToMap: remote base file is NOT skipped when configured as branch-specific (isRemote=true)", async () => { + // This is the key regression test. + // When pulling from remote, the workspace only has base paths (e.g., TestVar.variable.yaml) + // These should NOT be skipped even if configured as branch-specific, because the remote + // workspace doesn't have branch-specific file naming. + + const config: SpecificItemsConfig = { + variables: ["f/Shared/Variable/**"], + }; + + const remoteFiles: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.variable.yaml", + content: "value: test\nis_secret: false", + }, + ]; + + const mockElement = createMockDynFSElement(remoteFiles); + + // When isRemote=true, base files should be included even if they match branch-specific config + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", // branchOverride + true, // isRemote = true + ); + + // The base file should be in the map + assertEquals( + Object.keys(result).includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Remote base file should NOT be skipped when isRemote=true" + ); +}); + +Deno.test("elementsToMap: local base file IS skipped when configured as branch-specific (isRemote=false)", async () => { + // When processing local files, if a base file is configured as branch-specific, + // it should be skipped because we expect the branch-specific version to be used instead. + + const config: SpecificItemsConfig = { + variables: ["f/Shared/Variable/**"], + }; + + const localFiles: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.variable.yaml", + content: "value: test\nis_secret: false", + }, + ]; + + const mockElement = createMockDynFSElement(localFiles); + + // When isRemote=false, base files should be skipped if configured as branch-specific + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", // branchOverride + false, // isRemote = false + ); + + // The base file should NOT be in the map (skipped because branch-specific expected) + assertEquals( + Object.keys(result).includes("f/Shared/Variable/TestVar.variable.yaml"), + false, + "Local base file SHOULD be skipped when isRemote=false and configured as branch-specific" + ); +}); + +Deno.test("elementsToMap: local branch-specific file is mapped to base path (isRemote=false)", async () => { + // When processing local files with branch-specific naming, they should be mapped to base paths + + const config: SpecificItemsConfig = { + variables: ["f/Shared/Variable/**"], + }; + + const localFiles: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.staging.variable.yaml", + content: "value: staging-test\nis_secret: false", + }, + ]; + + const mockElement = createMockDynFSElement(localFiles); + + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", // branchOverride + false, // isRemote = false + ); + + // The branch-specific file should be mapped to the base path + assertEquals( + Object.keys(result).includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Branch-specific file should be mapped to base path" + ); + assertEquals( + result["f/Shared/Variable/TestVar.variable.yaml"], + "value: staging-test\nis_secret: false", + ); +}); + +// ============================================================================= +// PULL SCENARIO: Remote (base path) vs Local (branch-specific path) +// This simulates the actual pull scenario where: +// - Remote has: f/Shared/Variable/TestVar.variable.yaml +// - Local has: f/Shared/Variable/TestVar.staging.variable.yaml +// - Expected: No deletion, the files should match +// ============================================================================= + +Deno.test("elementsToMap: pull scenario - remote and local maps should align correctly", async () => { + const config: SpecificItemsConfig = { + variables: ["f/Shared/Variable/**"], + }; + + // Remote workspace has base path + const remoteFiles: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.variable.yaml", + content: "value: test\nis_secret: false", + }, + ]; + + // Local has branch-specific path + const localFiles: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.staging.variable.yaml", + content: "value: staging-test\nis_secret: false", + }, + ]; + + const remoteElement = createMockDynFSElement(remoteFiles); + const localElement = createMockDynFSElement(localFiles); + + // Process remote (isRemote=true) + const remoteMap = await elementsToMap( + remoteElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + true, // isRemote + ); + + // Process local (isRemote=false) + const localMap = await elementsToMap( + localElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + false, // isRemote + ); + + // Both maps should have the same base path key + const remoteKeys = Object.keys(remoteMap); + const localKeys = Object.keys(localMap); + + assertEquals( + remoteKeys.includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Remote map should include base path" + ); + assertEquals( + localKeys.includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Local map should include base path (mapped from branch-specific)" + ); +}); + +// ============================================================================= +// NON-CONFIGURED ITEMS: Should work the same regardless of isRemote +// ============================================================================= + +Deno.test("elementsToMap: non-configured items included regardless of isRemote", async () => { + const config: SpecificItemsConfig = { + variables: ["f/Other/**"], // Only "Other" folder is branch-specific + }; + + const files: MockFile[] = [ + { + path: "f/Shared/Variable/TestVar.variable.yaml", + content: "value: test\nis_secret: false", + }, + ]; + + const mockElement = createMockDynFSElement(files); + + // Test with isRemote=true + const remoteResult = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + true, + ); + + // Test with isRemote=false + const localResult = await elementsToMap( + createMockDynFSElement(files), + noIgnore, + false, + defaultSkips, + config, + "staging", + false, + ); + + // Both should include the file since it's not in the branch-specific config + assertEquals( + Object.keys(remoteResult).includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Non-configured item should be included when isRemote=true" + ); + assertEquals( + Object.keys(localResult).includes("f/Shared/Variable/TestVar.variable.yaml"), + true, + "Non-configured item should be included when isRemote=false" + ); +}); + +// ============================================================================= +// RESOURCE TYPE TESTS +// ============================================================================= + +Deno.test("elementsToMap: remote resource base file not skipped when configured", async () => { + const config: SpecificItemsConfig = { + resources: ["f/db/**"], + }; + + const remoteFiles: MockFile[] = [ + { + path: "f/db/connection.resource.yaml", + content: "value: { host: localhost }", + }, + ]; + + const mockElement = createMockDynFSElement(remoteFiles); + + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + true, // isRemote + ); + + assertEquals( + Object.keys(result).includes("f/db/connection.resource.yaml"), + true, + "Remote resource base file should NOT be skipped" + ); +}); + +// ============================================================================= +// TRIGGER TYPE TESTS +// ============================================================================= + +Deno.test("elementsToMap: remote trigger base file not skipped when configured", async () => { + const config: SpecificItemsConfig = { + triggers: ["f/webhooks/**"], + }; + + const remoteFiles: MockFile[] = [ + { + path: "f/webhooks/handler.http_trigger.yaml", + content: "path: /webhook", + }, + ]; + + const mockElement = createMockDynFSElement(remoteFiles); + + const result = await elementsToMap( + mockElement, + noIgnore, + false, + { includeTriggers: true }, // Must include triggers explicitly + config, + "staging", + true, // isRemote + ); + + assertEquals( + Object.keys(result).includes("f/webhooks/handler.http_trigger.yaml"), + true, + "Remote trigger base file should NOT be skipped" + ); +}); + +// ============================================================================= +// SETTINGS TYPE TESTS +// ============================================================================= + +Deno.test("elementsToMap: remote settings.yaml not skipped when configured", async () => { + const config: SpecificItemsConfig = { + settings: true, + }; + + const remoteFiles: MockFile[] = [ + { + path: "settings.yaml", + content: "openai_resource_path: null", + }, + ]; + + const mockElement = createMockDynFSElement(remoteFiles); + + const result = await elementsToMap( + mockElement, + noIgnore, + false, + { includeSettings: true }, + config, + "staging", + true, // isRemote + ); + + assertEquals( + Object.keys(result).includes("settings.yaml"), + true, + "Remote settings.yaml should NOT be skipped" + ); +}); + +// ============================================================================= +// FOLDER TYPE TESTS +// ============================================================================= + +Deno.test("elementsToMap: remote folder meta not skipped when configured", async () => { + const config: SpecificItemsConfig = { + folders: ["f/env_*"], + }; + + const remoteFiles: MockFile[] = [ + { + path: "f/env_staging/folder.meta.yaml", + content: "display_name: Staging Environment", + }, + ]; + + const mockElement = createMockDynFSElement(remoteFiles); + + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + true, // isRemote + ); + + assertEquals( + Object.keys(result).includes("f/env_staging/folder.meta.yaml"), + true, + "Remote folder meta should NOT be skipped" + ); +}); + +// ============================================================================= +// BACKWARD COMPATIBILITY: isRemote undefined behaves like local (false) +// ============================================================================= + +Deno.test("elementsToMap: isRemote undefined behaves like local (backward compatible)", async () => { + const config: SpecificItemsConfig = { + variables: ["f/**"], + }; + + const files: MockFile[] = [ + { + path: "f/test.variable.yaml", + content: "value: test\nis_secret: false", + }, + ]; + + const mockElement = createMockDynFSElement(files); + + // When isRemote is undefined (backward compatibility), it should behave like local + const result = await elementsToMap( + mockElement, + noIgnore, + false, + defaultSkips, + config, + "staging", + // isRemote omitted + ); + + // Base file should be skipped (same behavior as isRemote=false) + assertEquals( + Object.keys(result).includes("f/test.variable.yaml"), + false, + "isRemote undefined should behave like isRemote=false (skip base file)" + ); +});