Merge pull request #6781 from Jazzcort/fix-block-duplication

Fix: Remove duplicate blocks and provide config warnings

Author: RomneyDa

packages/config-yaml/src/__tests__/index.test.ts

@@ -93,9 +93,11 @@ describe("E2E Scenarios", () => {
   const registry: Registry = {
     getContent: async function (id: PackageIdentifier): Promise<string> {
       const slug = packageIdentifierToShorthandSlug(id);
-      return fs
-        .readFileSync(`./src/__tests__/packages/${slug}.yaml`)
-        .toString();
+      const filePath =
+        id.uriType === "slug"
+          ? `./src/__tests__/packages/${slug}.yaml`
+          : id.filePath;
+      return fs.readFileSync(filePath).toString();
     },
   };
 
@@ -230,7 +232,7 @@ describe("E2E Scenarios", () => {
         orgScopeId: "test-org",
         currentUserSlug: "test-user",
         onPremProxyUrl: null,
-        // Add an injected block
+        // Add injected blocks
         injectBlocks: [
           {
             uriType: "slug",
@@ -240,11 +242,16 @@ describe("E2E Scenarios", () => {
               versionSlug: "latest",
             },
           },
+          {
+            uriType: "file",
+            filePath: "./src/__tests__/local-files/rules.yaml",
+          },
         ],
       },
     );
 
     const config = unrolledConfig.config;
+    const errors = unrolledConfig.errors;
 
     // The original rules array should have two items
     expect(config?.rules?.length).toBe(3); // Now 3 with the injected block
@@ -255,7 +262,72 @@ describe("E2E Scenarios", () => {
     );
 
     // Check the injected doc block was added
-    expect(config?.rules?.[2]).toBe("Be kind");
+    expect(
+      typeof config?.rules?.[2] !== "string" &&
+        config?.rules?.[2]?.rule === "Be humble",
+    );
+
+    // Check if we receive one error that is caused by duplicate rules
+    expect(errors?.length).toBe(1);
+    expect(errors?.[0].message.includes("Duplicate rules detected"));
+  });
+
+  it("duplicate detection should happen in the assistant config first and then the intected blocks", async () => {
+    const unrolledConfig = await unrollAssistant(
+      {
+        uriType: "file",
+        filePath: "./src/__tests__/local-files/duplicate-test-assistant.yaml",
+      },
+      registry,
+      {
+        renderSecrets: true,
+        platformClient,
+        orgScopeId: "test-org",
+        currentUserSlug: "test-user",
+        onPremProxyUrl: null,
+        // Add injected blocks
+        injectBlocks: [
+          {
+            uriType: "file",
+            filePath: "./src/__tests__/local-files/rules.yaml",
+          },
+          {
+            uriType: "file",
+            filePath: "./src/__tests__/local-files/mcpServer.yaml",
+          },
+          {
+            uriType: "file",
+            filePath: "./src/__tests__/local-files/prompt.yaml",
+          },
+        ],
+      },
+    );
+
+    const config = unrolledConfig.config;
+    const errors = unrolledConfig.errors;
+
+    // Check if all the duplicate blocks get removed
+    expect(config?.models?.length).toBe(1);
+    expect(config?.context?.length).toBe(1);
+    expect(config?.mcpServers?.length).toBe(1);
+    expect(config?.rules?.length).toBe(1);
+    expect(config?.prompts?.length).toBe(1);
+    expect(config?.docs?.length).toBe(1);
+
+    // Check if there are 8 duplication detected
+    expect(errors?.length).toBe(8);
+
+    // Beginning of the assistant config duplication check
+    expect(
+      errors?.[0].message.includes("Duplicate models named gpt-5 detected"),
+    ).toBe(true);
+    // Beginning of the injected blocks duplication check
+    expect(errors?.[4].message.includes("Duplicate rules detected")).toBe(true);
+    expect(
+      errors?.[6].message.includes(
+        "Duplicate mcpServers named Browser search detected",
+      ),
+    ).toBe(true);
   });
 
   it.skip("should prioritize org over user / package secrets", () => {});

packages/config-yaml/src/__tests__/local-files/duplicate-test-assistant.yaml

@@ -0,0 +1,39 @@
+name: Duplicate Test Assistant
+version: 0.0.1
+schema: v1
+
+docs:
+  - name: React
+    startUrl: https://react.dev
+  - name: React
+    startUrl: https://react.dev
+
+rules:
+  - Be humble
+  - uses: ./src/__tests__/local-files/rules.yaml
+
+models:
+  - name: gpt-5
+    provider: openai
+    model: gpt-5
+    apiKey: ${{ secrets.OPENAI_API_KEY }}
+    roles:
+      - chat
+
+  - name: gpt-5
+    provider: openai
+    model: gpt-5
+    apiKey: ${{ secrets.OPENAI_API_KEY }}
+    roles:
+      - chat
+
+mcpServers:
+  - uses: ./src/__tests__/local-files/mcpServer.yaml
+
+prompts:
+  - uses: ./src/__tests__/local-files/prompt.yaml
+  - uses: ./src/__tests__/local-files/prompt.yaml
+
+context:
+  - provider: doc
+  - provider: doc

packages/config-yaml/src/__tests__/local-files/mcpServer.yaml

@@ -0,0 +1,8 @@
+name: Local MCP Server
+version: 0.0.1
+schema: v1
+mcpServers:
+  - name: Browser search
+    command: npx
+    args:
+      - "@playwright/mcp@latest"

packages/config-yaml/src/__tests__/local-files/prompt.yaml

@@ -0,0 +1,7 @@
+name: Local prompt
+version: 0.0.1
+schema: v1
+prompts:
+  - name: duplication test
+    description: test purpose
+    prompt: Please check that this prompt should only appear once

packages/config-yaml/src/__tests__/local-files/rules.yaml

@@ -0,0 +1,6 @@
+name: Rules
+version: 0.0.1
+schema: v1
+
+rules:
+  - Be humble

packages/config-yaml/src/load/blockDuplicationDetector.ts

@@ -0,0 +1,76 @@
+import { BLOCK_TYPES, BlockType } from "./getBlockType.js";
+
+export class BlockDuplicationDetector {
+  private records: Map<string, Set<string>>;
+
+  constructor() {
+    this.records = new Map();
+    for (const blockType of BLOCK_TYPES) {
+      this.records.set(blockType, new Set());
+    }
+  }
+
+  private isRuleDuplicated(rule: any): boolean {
+    if (typeof rule === "string") {
+      return this.check(rule, "rules");
+    } else {
+      return this.check(rule.name, "rules");
+    }
+  }
+
+  private isContextDuplicated(context: any): boolean {
+    return this.check(context.provider, "context");
+  }
+
+  private isCommonBlockDuplicated(block: any, blockType: BlockType): boolean {
+    return this.check(block.name, blockType);
+  }
+
+  private check(identifier: string, blockType: BlockType): boolean {
+    if (this.records.get(blockType)!.has(identifier)) {
+      return true;
+    } else {
+      this.records.get(blockType)!.add(identifier);
+      return false;
+    }
+  }
+
+  // Check if the name is duplicated within the same blockType
+  isDuplicated(
+    block: any,
+    blockType: BlockType,
+    injectError?: (errorMsg: string) => void,
+  ): boolean {
+    // Not checking any null or undefined object
+    if (block === null || block === undefined) {
+      return false;
+    }
+
+    switch (blockType) {
+      case "rules":
+        if (this.isRuleDuplicated(block)) {
+          injectError?.(
+            `Duplicate rules${typeof block === "string" ? "" : ` named ${block.name}`} detected. The duplicate one has been deleted for preventing unexpected behavior.`,
+          );
+          return true;
+        }
+        return false;
+      case "context":
+        if (this.isContextDuplicated(block)) {
+          injectError?.(
+            `Duplicate ${block.provider} context providers detected. The duplicate one has been deleted for preventing unexpected behavior.`,
+          );
+          return true;
+        }
+        return false;
+      default:
+        if (this.isCommonBlockDuplicated(block, blockType)) {
+          injectError?.(
+            `Duplicate ${blockType} named ${block.name} detected. The duplicate one has been deleted for preventing unexpected behavior.`,
+          );
+          return true;
+        }
+        return false;
+    }
+  }
+}

packages/config-yaml/src/load/unroll.ts

@@ -20,6 +20,7 @@ import {
   Rule,
 } from "../schemas/index.js";
 import { ConfigResult, ConfigValidationError } from "../validation.js";
+import { BlockDuplicationDetector } from "./blockDuplicationDetector.js";
 import {
   packageIdentifierToShorthandSlug,
   useProxyForUnrenderedSecrets,
@@ -306,6 +307,13 @@ export async function unrollBlocks(
 ): Promise<ConfigResult<AssistantUnrolled>> {
   const errors: ConfigValidationError[] = [];
 
+  function injectDuplicationError(errorMsg: string) {
+    errors.push({
+      fatal: false,
+      message: errorMsg,
+    });
+  }
+
   const unrolledAssistant: AssistantUnrolled = {
     name: assistant.name,
     version: assistant.version,
@@ -484,7 +492,7 @@ export async function unrollBlocks(
         const injectedResults = await Promise.all(injectedBlockPromises);
         const injectedErrors: ConfigValidationError[] = [];
         const injectedBlocks: {
-          blockType: string;
+          blockType: BlockType;
           resolvedBlock: any;
           source?: string;
         }[] = [];
@@ -521,16 +529,27 @@ export async function unrollBlocks(
   errors.push(...rulesResult.errors);
   errors.push(...injectedResult.errors);
 
+  const detector = new BlockDuplicationDetector();
+
   // Assign section results
   for (const sectionResult of sectionResults) {
     if (sectionResult.blocks) {
-      unrolledAssistant[sectionResult.section] = sectionResult.blocks;
+      unrolledAssistant[sectionResult.section] = sectionResult.blocks.filter(
+        (block) =>
+          !detector.isDuplicated(
+            block,
+            sectionResult.section,
+            injectDuplicationError,
+          ),
+      );
     }
   }
 
   // Assign rules result
   if (rulesResult.rules) {
-    unrolledAssistant.rules = rulesResult.rules;
+    unrolledAssistant.rules = rulesResult.rules.filter(
+      (rule) => !detector.isDuplicated(rule, "rules", injectDuplicationError),
+    );
   }
 
   // Add injected blocks
@@ -539,16 +558,20 @@ export async function unrollBlocks(
     resolvedBlock,
     source,
   } of injectedResult.injectedBlocks) {
-    const key = blockType as BlockType;
+    const key = blockType;
     if (!unrolledAssistant[key]) {
       unrolledAssistant[key] = [];
     }
-    const blocksWithSourceFiles = injectLocalSourceFile(
+
+    const filteredBlocks = injectLocalSourceFile(
       key,
       resolvedBlock,
       source,
+    ).filter(
+      (block: any) =>
+        !detector.isDuplicated(block, blockType, injectDuplicationError),
     );
-    unrolledAssistant[key]?.push(...blocksWithSourceFiles);
+    unrolledAssistant[key]?.push(...filteredBlocks);
   }
 
   const configResult: ConfigResult<AssistantUnrolled> = {