diff --git a/.changeset/patch-sort-safe-output-temporary-ids.md b/.changeset/patch-sort-safe-output-temporary-ids.md new file mode 100644 index 0000000000..8ffb9d4921 --- /dev/null +++ b/.changeset/patch-sort-safe-output-temporary-ids.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Sort safe output tool messages by their temporary ID dependencies before dispatching them so single-pass handlers can resolve every reference without multiple retries. diff --git a/.changeset/patch-temporary-id-module.md b/.changeset/patch-temporary-id-module.md new file mode 100644 index 0000000000..ecafdebaa5 --- /dev/null +++ b/.changeset/patch-temporary-id-module.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Split the temporary ID helpers into their own `temporary_id.cjs` module and adjust the associated tests. diff --git a/actions/setup/js/safe_output_topological_sort.cjs b/actions/setup/js/safe_output_topological_sort.cjs new file mode 100644 index 0000000000..1198d516fd --- /dev/null +++ b/actions/setup/js/safe_output_topological_sort.cjs @@ -0,0 +1,288 @@ +// @ts-check +/// + +/** + * Topological Sort for Safe Output Tool Calls + * + * This module provides topological sorting of safe output messages based on + * temporary ID dependencies. Messages that create entities without referencing + * temporary IDs are processed first, followed by messages that depend on them. + * + * This enables resolution of all temporary IDs in a single pass for acyclic + * dependency graphs (graphs without loops). + */ + +const { extractTemporaryIdReferences, getCreatedTemporaryId } = require("./temporary_id.cjs"); + +/** + * Build a dependency graph for safe output messages + * Returns: + * - dependencies: Map of message index -> Set of message indices it depends on + * - providers: Map of temporary ID -> message index that creates it + * + * @param {Array} messages - Array of safe output messages + * @returns {{dependencies: Map>, providers: Map}} + */ +function buildDependencyGraph(messages) { + /** @type {Map>} */ + const dependencies = new Map(); + + /** @type {Map} */ + const providers = new Map(); + + // First pass: identify which messages create which temporary IDs + for (let i = 0; i < messages.length; i++) { + const message = messages[i]; + const createdId = getCreatedTemporaryId(message); + + if (createdId !== null) { + if (providers.has(createdId)) { + // Duplicate temporary ID - this is a problem + // We'll let the handler deal with this, but note it + if (typeof core !== "undefined") { + core.warning(`Duplicate temporary_id '${createdId}' at message indices ${providers.get(createdId)} and ${i}. ` + `Only the first occurrence will be used.`); + } + } else { + providers.set(createdId, i); + } + } + + // Initialize dependencies set for this message + dependencies.set(i, new Set()); + } + + // Second pass: identify dependencies + for (let i = 0; i < messages.length; i++) { + const message = messages[i]; + const referencedIds = extractTemporaryIdReferences(message); + + // For each temporary ID this message references, find the provider + for (const tempId of referencedIds) { + const providerIndex = providers.get(tempId); + + if (providerIndex !== undefined) { + // This message depends on the provider message + const deps = dependencies.get(i); + if (deps) { + deps.add(providerIndex); + } + } + // If no provider, the temp ID might be from a previous step or be unresolved + // We don't add a dependency in this case + } + } + + return { dependencies, providers }; +} + +/** + * Detect cycles in the dependency graph using iterative mark-and-sweep algorithm + * Returns an array of message indices that form a cycle, or empty array if no cycle + * + * @param {Map>} dependencies - Dependency graph + * @returns {Array} Indices of messages forming a cycle, or empty array + */ +function detectCycle(dependencies) { + const WHITE = 0; // Not visited + const GRAY = 1; // Visiting (on stack) + const BLACK = 2; // Visited (completed) + + const colors = new Map(); + const parent = new Map(); + + // Initialize all nodes as WHITE + for (const node of dependencies.keys()) { + colors.set(node, WHITE); + parent.set(node, null); + } + + // Try to find cycle starting from each WHITE node + for (const startNode of dependencies.keys()) { + if (colors.get(startNode) !== WHITE) { + continue; + } + + // Use a stack for iterative DFS + // Each stack entry: [node, iterator, isReturning] + /** @type {Array<[number, any, boolean]>} */ + const stack = [[startNode, null, false]]; + + while (stack.length > 0) { + const entry = stack.pop(); + if (!entry) continue; + + const [node, depsIterator, isReturning] = entry; + + if (isReturning) { + // Returning from exploring this node - mark as BLACK + colors.set(node, BLACK); + continue; + } + + // Mark node as GRAY (being explored) + colors.set(node, GRAY); + + // Push node again to mark BLACK when we return + stack.push([node, null, true]); + + // Get dependencies for this node + const deps = dependencies.get(node) || new Set(); + + // Process each dependency + for (const dep of deps) { + const depColor = colors.get(dep); + + if (depColor === WHITE) { + // Not visited yet - explore it + parent.set(dep, node); + stack.push([dep, null, false]); + } else if (depColor === GRAY) { + // Found a back edge - cycle detected! + // Reconstruct the cycle from dep to current node + const cycle = [dep]; + let current = node; + while (current !== dep && current !== null) { + cycle.unshift(current); + current = parent.get(current); + } + return cycle; + } + // If BLACK, it's already fully explored - no cycle through this path + } + } + } + + return []; +} + +/** + * Perform topological sort on messages using Kahn's algorithm + * Messages without dependencies come first, followed by their dependents + * + * @param {Array} messages - Array of safe output messages + * @param {Map>} dependencies - Dependency graph + * @returns {Array} Array of message indices in topologically sorted order + */ +function topologicalSort(messages, dependencies) { + // Calculate in-degree (number of dependencies) for each message + const inDegree = new Map(); + for (let i = 0; i < messages.length; i++) { + const deps = dependencies.get(i) || new Set(); + inDegree.set(i, deps.size); + } + + // Queue of messages with no dependencies + const queue = []; + for (let i = 0; i < messages.length; i++) { + if (inDegree.get(i) === 0) { + queue.push(i); + } + } + + const sorted = []; + + while (queue.length > 0) { + // Process nodes in order of appearance for stability + // This preserves the original order when there are no dependencies + const node = queue.shift(); + if (node !== undefined) { + sorted.push(node); + + // Find all messages that depend on this one + for (const [other, deps] of dependencies.entries()) { + if (deps.has(node)) { + // Reduce in-degree + const currentDegree = inDegree.get(other); + if (currentDegree !== undefined) { + inDegree.set(other, currentDegree - 1); + + // If all dependencies satisfied, add to queue + if (inDegree.get(other) === 0) { + queue.push(other); + } + } + } + } + } + } + + // If sorted.length < messages.length, there's a cycle + if (sorted.length < messages.length) { + const unsorted = []; + for (let i = 0; i < messages.length; i++) { + if (!sorted.includes(i)) { + unsorted.push(i); + } + } + + if (typeof core !== "undefined") { + core.warning(`Topological sort incomplete: ${sorted.length}/${messages.length} messages sorted. ` + `Messages ${unsorted.join(", ")} may be part of a dependency cycle.`); + } + } + + return sorted; +} + +/** + * Sort safe output messages in topological order based on temporary ID dependencies + * Messages that don't reference temporary IDs are processed first, followed by + * messages that depend on them. This enables single-pass resolution of temporary IDs. + * + * If a cycle is detected, the original order is preserved and a warning is logged. + * + * @param {Array} messages - Array of safe output messages + * @returns {Array} Messages in topologically sorted order + */ +function sortSafeOutputMessages(messages) { + if (!Array.isArray(messages) || messages.length === 0) { + return messages; + } + + // Build dependency graph + const { dependencies, providers } = buildDependencyGraph(messages); + + if (typeof core !== "undefined") { + const messagesWithDeps = Array.from(dependencies.entries()).filter(([_, deps]) => deps.size > 0); + core.info(`Dependency analysis: ${providers.size} message(s) create temporary IDs, ` + `${messagesWithDeps.length} message(s) have dependencies`); + } + + // Check for cycles + const cycle = detectCycle(dependencies); + if (cycle.length > 0) { + if (typeof core !== "undefined") { + const cycleMessages = cycle.map(i => { + const msg = messages[i]; + const tempId = getCreatedTemporaryId(msg); + return `${i} (${msg.type}${tempId ? `, creates ${tempId}` : ""})`; + }); + core.warning(`Dependency cycle detected in safe output messages: ${cycleMessages.join(" -> ")}. ` + `Temporary IDs may not resolve correctly. Messages will be processed in original order.`); + } + // Return original order if there's a cycle + return messages; + } + + // Perform topological sort + const sortedIndices = topologicalSort(messages, dependencies); + + // Reorder messages according to sorted indices + const sortedMessages = sortedIndices.map(i => messages[i]); + + if (typeof core !== "undefined" && sortedIndices.length > 0) { + // Check if order changed + const orderChanged = sortedIndices.some((idx, i) => idx !== i); + if (orderChanged) { + core.info(`Topological sort reordered ${messages.length} message(s) to resolve temporary ID dependencies. ` + `New order: [${sortedIndices.join(", ")}]`); + } else { + core.info(`Topological sort: Messages already in optimal order (no reordering needed)`); + } + } + + return sortedMessages; +} + +module.exports = { + buildDependencyGraph, + detectCycle, + topologicalSort, + sortSafeOutputMessages, +}; diff --git a/actions/setup/js/safe_output_topological_sort.test.cjs b/actions/setup/js/safe_output_topological_sort.test.cjs new file mode 100644 index 0000000000..eaff66be2d --- /dev/null +++ b/actions/setup/js/safe_output_topological_sort.test.cjs @@ -0,0 +1,671 @@ +// @ts-check +import { describe, it, expect, beforeEach, vi } from "vitest"; + +// Mock core for logging +const mockCore = { + info: vi.fn(), + warning: vi.fn(), + debug: vi.fn(), +}; +global.core = mockCore; + +describe("safe_output_topological_sort.cjs", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("buildDependencyGraph", () => { + it("should build graph with simple dependency", async () => { + const { buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Parent" }, + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment" }, + ]; + + const { dependencies, providers } = buildDependencyGraph(messages); + + expect(providers.size).toBe(1); + expect(providers.get("aw_dddddd111111")).toBe(0); + + expect(dependencies.get(0).size).toBe(0); // Message 0 has no dependencies + expect(dependencies.get(1).size).toBe(1); // Message 1 depends on message 0 + expect(dependencies.get(1).has(0)).toBe(true); + }); + + it("should build graph with chain of dependencies", async () => { + const { buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "First" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", body: "Ref #aw_dddddd111111" }, + { type: "create_issue", temporary_id: "aw_ffffff333333", body: "Ref #aw_eeeeee222222" }, + ]; + + const { dependencies, providers } = buildDependencyGraph(messages); + + expect(providers.size).toBe(3); + + expect(dependencies.get(0).size).toBe(0); // No dependencies + expect(dependencies.get(1).size).toBe(1); // Depends on 0 + expect(dependencies.get(1).has(0)).toBe(true); + expect(dependencies.get(2).size).toBe(1); // Depends on 1 + expect(dependencies.get(2).has(1)).toBe(true); + }); + + it("should handle multiple dependencies", async () => { + const { buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Issue 1" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", title: "Issue 2" }, + { + type: "create_issue", + temporary_id: "aw_ffffff333333", + body: "See #aw_dddddd111111 and #aw_eeeeee222222", + }, + ]; + + const { dependencies, providers } = buildDependencyGraph(messages); + + expect(dependencies.get(2).size).toBe(2); + expect(dependencies.get(2).has(0)).toBe(true); + expect(dependencies.get(2).has(1)).toBe(true); + }); + + it("should warn on duplicate temporary IDs", async () => { + const { buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_abc111def222", title: "First" }, + { type: "create_issue", temporary_id: "aw_abc111def222", title: "Second" }, + ]; + + const { providers } = buildDependencyGraph(messages); + + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Duplicate temporary_id 'aw_abc111def222'")); + + // Verify only the first occurrence is used as provider + expect(providers.get("aw_abc111def222")).toBe(0); + expect(providers.size).toBe(1); + }); + + it("should handle messages without temporary IDs", async () => { + const { buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", title: "No temp ID" }, + { type: "add_comment", issue_number: 123, body: "Regular issue" }, + ]; + + const { dependencies, providers } = buildDependencyGraph(messages); + + expect(providers.size).toBe(0); + expect(dependencies.get(0).size).toBe(0); + expect(dependencies.get(1).size).toBe(0); + }); + }); + + describe("detectCycle", () => { + it("should detect simple cycle", async () => { + const { detectCycle } = await import("./safe_output_topological_sort.cjs"); + + // Create a cycle: 0 -> 1 -> 0 + const dependencies = new Map([ + [0, new Set([1])], + [1, new Set([0])], + ]); + + const cycle = detectCycle(dependencies); + + expect(cycle.length).toBeGreaterThan(0); + }); + + it("should detect complex cycle", async () => { + const { detectCycle } = await import("./safe_output_topological_sort.cjs"); + + // Create a cycle: 0 -> 1 -> 2 -> 0 + const dependencies = new Map([ + [0, new Set([1])], + [1, new Set([2])], + [2, new Set([0])], + ]); + + const cycle = detectCycle(dependencies); + + expect(cycle.length).toBeGreaterThan(0); + }); + + it("should return empty array for acyclic graph", async () => { + const { detectCycle } = await import("./safe_output_topological_sort.cjs"); + + // Acyclic: 0 -> 1 -> 2 + const dependencies = new Map([ + [0, new Set()], + [1, new Set([0])], + [2, new Set([1])], + ]); + + const cycle = detectCycle(dependencies); + + expect(cycle.length).toBe(0); + }); + + it("should handle disconnected components", async () => { + const { detectCycle } = await import("./safe_output_topological_sort.cjs"); + + // Two separate chains: 0 -> 1 and 2 -> 3 + const dependencies = new Map([ + [0, new Set()], + [1, new Set([0])], + [2, new Set()], + [3, new Set([2])], + ]); + + const cycle = detectCycle(dependencies); + + expect(cycle.length).toBe(0); + }); + }); + + describe("topologicalSort", () => { + it("should sort messages with simple dependency", async () => { + const { topologicalSort, buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment" }, + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Parent" }, + ]; + + const { dependencies } = buildDependencyGraph(messages); + const sorted = topologicalSort(messages, dependencies); + + // Message 1 (create_issue) should come before message 0 (add_comment) + expect(sorted).toEqual([1, 0]); + }); + + it("should preserve original order when no dependencies", async () => { + const { topologicalSort, buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Issue 1" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", title: "Issue 2" }, + { type: "create_issue", temporary_id: "aw_ffffff333333", title: "Issue 3" }, + ]; + + const { dependencies } = buildDependencyGraph(messages); + const sorted = topologicalSort(messages, dependencies); + + expect(sorted).toEqual([0, 1, 2]); + }); + + it("should sort dependency chain correctly", async () => { + const { topologicalSort, buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_ffffff333333", body: "Ref #aw_eeeeee222222" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", body: "Ref #aw_dddddd111111" }, + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "First" }, + ]; + + const { dependencies } = buildDependencyGraph(messages); + const sorted = topologicalSort(messages, dependencies); + + // Should be: message 2 (first), then 1 (second), then 0 (third) + expect(sorted).toEqual([2, 1, 0]); + }); + + it("should handle multiple independent messages", async () => { + const { topologicalSort, buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Independent 1" }, + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment on 1" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", title: "Independent 2" }, + { type: "add_comment", issue_number: "aw_eeeeee222222", body: "Comment on 2" }, + ]; + + const { dependencies } = buildDependencyGraph(messages); + const sorted = topologicalSort(messages, dependencies); + + // Creates should come before their comments + expect(sorted.indexOf(0)).toBeLessThan(sorted.indexOf(1)); // Issue 1 before comment on 1 + expect(sorted.indexOf(2)).toBeLessThan(sorted.indexOf(3)); // Issue 2 before comment on 2 + }); + + it("should handle complex dependency graph", async () => { + const { topologicalSort, buildDependencyGraph } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "Parent" }, + { type: "create_issue", temporary_id: "aw_bbbbbb111111", body: "Parent: #aw_aaaaaa111111" }, + { type: "create_issue", temporary_id: "aw_cccccc222222", body: "Parent: #aw_aaaaaa111111" }, + { type: "link_sub_issue", parent_issue_number: "aw_aaaaaa111111", sub_issue_number: "aw_bbbbbb111111" }, + { type: "link_sub_issue", parent_issue_number: "aw_aaaaaa111111", sub_issue_number: "aw_cccccc222222" }, + ]; + + const { dependencies } = buildDependencyGraph(messages); + const sorted = topologicalSort(messages, dependencies); + + // Parent must come first + expect(sorted[0]).toBe(0); + // Children must come after parent + const childIndices = [sorted.indexOf(1), sorted.indexOf(2)]; + expect(Math.min(...childIndices)).toBeGreaterThan(0); + // Links must come after all creates + expect(sorted.indexOf(3)).toBeGreaterThan(Math.max(...childIndices)); + expect(sorted.indexOf(4)).toBeGreaterThan(Math.max(...childIndices)); + }); + }); + + describe("sortSafeOutputMessages", () => { + it("should return empty array for empty input", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const sorted = sortSafeOutputMessages([]); + + expect(sorted).toEqual([]); + }); + + it("should return original messages for non-array input", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const input = null; + const sorted = sortSafeOutputMessages(input); + + expect(sorted).toBe(input); + }); + + it("should sort messages without temporary IDs first", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment" }, + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Issue" }, + { type: "create_issue", title: "No temp ID" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Messages without dependencies should come first + expect(sorted[0].type).toBe("create_issue"); + expect(sorted[0].title).toBe("Issue"); + expect(sorted[1].type).toBe("create_issue"); + expect(sorted[1].title).toBe("No temp ID"); + expect(sorted[2].type).toBe("add_comment"); + }); + + it("should handle cross-references between issues, PRs, and discussions", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_pull_request", temporary_id: "aw_fedcba111111", body: "Fixes #aw_dddddd111111" }, + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Bug report" }, + { type: "create_discussion", temporary_id: "aw_abcdef111111", body: "See #aw_fedcba111111" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Issue should come first, then PR (which references issue), then discussion (which references PR) + expect(sorted[0].type).toBe("create_issue"); + expect(sorted[1].type).toBe("create_pull_request"); + expect(sorted[2].type).toBe("create_discussion"); + }); + + it("should return original order when cycle is detected", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", body: "See #aw_eeeeee222222" }, + { type: "create_issue", temporary_id: "aw_eeeeee222222", body: "See #aw_dddddd111111" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Should return original order due to cycle + expect(sorted).toEqual(messages); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Dependency cycle detected")); + }); + + it("should log info about reordering", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment" }, + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Issue" }, + ]; + + sortSafeOutputMessages(messages); + + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Topological sort reordered")); + }); + + it("should log info when order doesn't change", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd111111", title: "Issue" }, + { type: "add_comment", issue_number: "aw_dddddd111111", body: "Comment" }, + ]; + + sortSafeOutputMessages(messages); + + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("already in optimal order")); + }); + + it("should handle complex real-world scenario", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Simulate a real workflow: create parent issue, create sub-issues, link them, add comments + const messages = [ + { type: "add_comment", issue_number: "aw_aaaaaa111111", body: "Status update" }, + { type: "link_sub_issue", parent_issue_number: "aw_aaaaaa111111", sub_issue_number: "aw_cccccc222222" }, + { type: "create_issue", temporary_id: "aw_bbbbbb111111", title: "Sub-task 1", body: "Parent: #aw_aaaaaa111111" }, + { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "Epic" }, + { type: "link_sub_issue", parent_issue_number: "aw_aaaaaa111111", sub_issue_number: "aw_bbbbbb111111" }, + { type: "create_issue", temporary_id: "aw_cccccc222222", title: "Sub-task 2", body: "Parent: #aw_aaaaaa111111" }, + { type: "add_comment", issue_number: "aw_bbbbbb111111", body: "Work started" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Verify parent is created first + const parentIndex = sorted.findIndex(m => m.temporary_id === "aw_aaaaaa111111"); + expect(parentIndex).toBe(0); + + // Verify children come after parent + const child1Index = sorted.findIndex(m => m.temporary_id === "aw_bbbbbb111111"); + const child2Index = sorted.findIndex(m => m.temporary_id === "aw_cccccc222222"); + expect(child1Index).toBeGreaterThan(parentIndex); + expect(child2Index).toBeGreaterThan(parentIndex); + + // Verify links come after all creates + const link1Index = sorted.findIndex(m => m.type === "link_sub_issue" && m.sub_issue_number === "aw_bbbbbb111111"); + const link2Index = sorted.findIndex(m => m.type === "link_sub_issue" && m.sub_issue_number === "aw_cccccc222222"); + expect(link1Index).toBeGreaterThan(child1Index); + expect(link2Index).toBeGreaterThan(child2Index); + + // Verify comments come after their targets + const parentCommentIndex = sorted.findIndex(m => m.type === "add_comment" && m.issue_number === "aw_aaaaaa111111"); + const child1CommentIndex = sorted.findIndex(m => m.type === "add_comment" && m.issue_number === "aw_bbbbbb111111"); + expect(parentCommentIndex).toBeGreaterThan(parentIndex); + expect(child1CommentIndex).toBeGreaterThan(child1Index); + }); + + it("should handle messages referencing external (already resolved) temp IDs", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Message references a temp ID that's not created in this batch + // (might be from a previous step) + const messages = [ + { type: "create_issue", temporary_id: "aw_abc123456789", title: "New Issue" }, + { type: "add_comment", issue_number: "aw_def987654321", body: "Comment on external" }, + { type: "add_comment", issue_number: "aw_abc123456789", body: "Comment on new" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // New issue should come before its comment + expect(sorted[0].temporary_id).toBe("aw_abc123456789"); + expect(sorted[2].issue_number).toBe("aw_abc123456789"); + + // External reference can be anywhere (no dependency in this batch) + // It should appear but we don't enforce ordering relative to unrelated items + expect(sorted.some(m => m.issue_number === "aw_def987654321")).toBe(true); + }); + + it("should handle large graphs with many dependencies", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Create a large tree: 1 root, 10 level-1 children, each with 5 level-2 children + const messages = []; + const rootId = "aw_root00000000"; + messages.push({ type: "create_issue", temporary_id: rootId, title: "Root" }); + + for (let i = 0; i < 10; i++) { + const level1Id = `aw_lv1_${i.toString().padStart(7, "0")}`; + messages.push({ + type: "create_issue", + temporary_id: level1Id, + body: `Parent: #${rootId}`, + }); + + for (let j = 0; j < 5; j++) { + const level2Id = `aw_lv2_${i}_${j.toString().padStart(4, "0")}`; + messages.push({ + type: "create_issue", + temporary_id: level2Id, + body: `Parent: #${level1Id}`, + }); + } + } + + const sorted = sortSafeOutputMessages(messages); + + // Verify root comes first + expect(sorted[0].temporary_id).toBe(rootId); + + // Verify each level-1 item comes before its level-2 children + for (let i = 0; i < 10; i++) { + const level1Id = `aw_lv1_${i.toString().padStart(7, "0")}`; + const level1Index = sorted.findIndex(m => m.temporary_id === level1Id); + + for (let j = 0; j < 5; j++) { + const level2Id = `aw_lv2_${i}_${j.toString().padStart(4, "0")}`; + const level2Index = sorted.findIndex(m => m.temporary_id === level2Id); + expect(level1Index).toBeLessThan(level2Index); + } + } + + // Verify root comes before all level-1 items + const rootIndex = sorted.findIndex(m => m.temporary_id === rootId); + for (let i = 0; i < 10; i++) { + const level1Id = `aw_lv1_${i.toString().padStart(7, "0")}`; + const level1Index = sorted.findIndex(m => m.temporary_id === level1Id); + expect(rootIndex).toBeLessThan(level1Index); + } + }); + + it("should handle deeply nested linear dependencies", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Create a chain: A -> B -> C -> D -> E -> F + const messages = []; + const ids = ["aw_aaaaaa111111", "aw_bbbbbb222222", "aw_cccccc333333", "aw_dddddd444444", "aw_eeeeee555555", "aw_ffffff666666"]; + + // Add in reverse order to test sorting + for (let i = ids.length - 1; i >= 0; i--) { + messages.push({ + type: "create_issue", + temporary_id: ids[i], + body: i > 0 ? `Depends on #${ids[i - 1]}` : "First issue", + }); + } + + const sorted = sortSafeOutputMessages(messages); + + // Verify they're sorted in dependency order + for (let i = 0; i < ids.length; i++) { + expect(sorted[i].temporary_id).toBe(ids[i]); + } + }); + + it("should handle multiple disconnected dependency chains", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + // Chain 1: A -> B + { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "A" }, + { type: "create_issue", temporary_id: "aw_bbbbbb222222", body: "Ref #aw_aaaaaa111111" }, + + // Chain 2: C -> D + { type: "create_issue", temporary_id: "aw_cccccc333333", title: "C" }, + { type: "create_issue", temporary_id: "aw_dddddd444444", body: "Ref #aw_cccccc333333" }, + + // Chain 3: E -> F + { type: "create_issue", temporary_id: "aw_eeeeee555555", title: "E" }, + { type: "create_issue", temporary_id: "aw_ffffff666666", body: "Ref #aw_eeeeee555555" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Within each chain, verify dependency order + const aIndex = sorted.findIndex(m => m.temporary_id === "aw_aaaaaa111111"); + const bIndex = sorted.findIndex(m => m.temporary_id === "aw_bbbbbb222222"); + expect(aIndex).toBeLessThan(bIndex); + + const cIndex = sorted.findIndex(m => m.temporary_id === "aw_cccccc333333"); + const dIndex = sorted.findIndex(m => m.temporary_id === "aw_dddddd444444"); + expect(cIndex).toBeLessThan(dIndex); + + const eIndex = sorted.findIndex(m => m.temporary_id === "aw_eeeeee555555"); + const fIndex = sorted.findIndex(m => m.temporary_id === "aw_ffffff666666"); + expect(eIndex).toBeLessThan(fIndex); + }); + + it("should handle diamond dependency pattern", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Diamond: A -> B, A -> C, B -> D, C -> D + const messages = [ + { type: "create_issue", temporary_id: "aw_dddddd444444", body: "Needs #aw_bbbbbb222222 and #aw_cccccc333333" }, + { type: "create_issue", temporary_id: "aw_bbbbbb222222", body: "Child of #aw_aaaaaa111111" }, + { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "Root" }, + { type: "create_issue", temporary_id: "aw_cccccc333333", body: "Child of #aw_aaaaaa111111" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + const aIndex = sorted.findIndex(m => m.temporary_id === "aw_aaaaaa111111"); + const bIndex = sorted.findIndex(m => m.temporary_id === "aw_bbbbbb222222"); + const cIndex = sorted.findIndex(m => m.temporary_id === "aw_cccccc333333"); + const dIndex = sorted.findIndex(m => m.temporary_id === "aw_dddddd444444"); + + // A must come first + expect(aIndex).toBe(0); + + // B and C must come after A + expect(bIndex).toBeGreaterThan(aIndex); + expect(cIndex).toBeGreaterThan(aIndex); + + // D must come after both B and C + expect(dIndex).toBeGreaterThan(bIndex); + expect(dIndex).toBeGreaterThan(cIndex); + }); + + it("should handle messages with multiple temporary ID references in body", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { + type: "create_issue", + temporary_id: "aw_ffffff666666", + body: "Blocks #aw_aaaaaa111111, #aw_bbbbbb222222, and #aw_cccccc333333", + }, + { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "First" }, + { type: "create_issue", temporary_id: "aw_bbbbbb222222", title: "Second" }, + { type: "create_issue", temporary_id: "aw_cccccc333333", title: "Third" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // All referenced IDs must come before the message that references them + const fIndex = sorted.findIndex(m => m.temporary_id === "aw_ffffff666666"); + const aIndex = sorted.findIndex(m => m.temporary_id === "aw_aaaaaa111111"); + const bIndex = sorted.findIndex(m => m.temporary_id === "aw_bbbbbb222222"); + const cIndex = sorted.findIndex(m => m.temporary_id === "aw_cccccc333333"); + + expect(fIndex).toBeGreaterThan(aIndex); + expect(fIndex).toBeGreaterThan(bIndex); + expect(fIndex).toBeGreaterThan(cIndex); + }); + + it("should handle empty messages array gracefully", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const sorted = sortSafeOutputMessages([]); + + expect(sorted).toEqual([]); + }); + + it("should handle single message", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [{ type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "Solo" }]; + + const sorted = sortSafeOutputMessages(messages); + + expect(sorted).toEqual(messages); + }); + + it("should preserve message object references", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const msg1 = { type: "create_issue", temporary_id: "aw_aaaaaa111111", title: "First" }; + const msg2 = { type: "create_issue", temporary_id: "aw_bbbbbb222222", body: "Ref #aw_aaaaaa111111" }; + + const sorted = sortSafeOutputMessages([msg2, msg1]); + + // Objects should be the same references, not copies + expect(sorted[0]).toBe(msg1); + expect(sorted[1]).toBe(msg2); + }); + + it("should handle cycle with 3 nodes", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + // Cycle: A -> B -> C -> A + const messages = [ + { type: "create_issue", temporary_id: "aw_aaaaaa111111", body: "Needs #aw_cccccc333333" }, + { type: "create_issue", temporary_id: "aw_bbbbbb222222", body: "Needs #aw_aaaaaa111111" }, + { type: "create_issue", temporary_id: "aw_cccccc333333", body: "Needs #aw_bbbbbb222222" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Should preserve original order when cycle detected + expect(sorted).toEqual(messages); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Dependency cycle detected")); + }); + + it("should handle messages with no type field", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { temporary_id: "aw_aaaaaa111111", title: "No type" }, + { type: "create_issue", temporary_id: "aw_bbbbbb222222", title: "Has type" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // Should handle gracefully without crashing + expect(sorted.length).toBe(2); + }); + + it("should handle mixed types (issues, PRs, discussions, comments)", async () => { + const { sortSafeOutputMessages } = await import("./safe_output_topological_sort.cjs"); + + const messages = [ + { type: "create_pull_request", temporary_id: "aw_aaaaaa111111", title: "PR" }, + { type: "add_comment", issue_number: "aw_aaaaaa111111", body: "Comment on PR" }, + { type: "create_discussion", temporary_id: "aw_bbbbbb222222", body: "See PR #aw_aaaaaa111111" }, + { type: "create_issue", temporary_id: "aw_cccccc333333", body: "Related to #aw_bbbbbb222222" }, + ]; + + const sorted = sortSafeOutputMessages(messages); + + // PR comes first + expect(sorted[0].type).toBe("create_pull_request"); + // Comment on PR comes after PR + const prIndex = sorted.findIndex(m => m.type === "create_pull_request"); + const commentIndex = sorted.findIndex(m => m.type === "add_comment"); + expect(commentIndex).toBeGreaterThan(prIndex); + // Discussion referencing PR comes after PR + const discussionIndex = sorted.findIndex(m => m.type === "create_discussion"); + expect(discussionIndex).toBeGreaterThan(prIndex); + // Issue referencing discussion comes after discussion + const issueIndex = sorted.findIndex(m => m.type === "create_issue"); + expect(issueIndex).toBeGreaterThan(discussionIndex); + }); + }); +}); diff --git a/actions/setup/js/safe_output_unified_handler_manager.cjs b/actions/setup/js/safe_output_unified_handler_manager.cjs index aa1a1c87c2..74f76f9c09 100644 --- a/actions/setup/js/safe_output_unified_handler_manager.cjs +++ b/actions/setup/js/safe_output_unified_handler_manager.cjs @@ -22,6 +22,7 @@ const { setCollectedMissings } = require("./missing_messages_helper.cjs"); const { writeSafeOutputSummaries } = require("./safe_output_summary.cjs"); const { getIssuesToAssignCopilot } = require("./create_issue.cjs"); const { getCampaignLabelsFromEnv } = require("./campaign_labels.cjs"); +const { sortSafeOutputMessages } = require("./safe_output_topological_sort.cjs"); const { loadCustomSafeOutputJobTypes } = require("./safe_output_helpers.cjs"); /** @@ -342,10 +343,14 @@ function collectMissingMessages(messages) { } /** - * Process all messages from agent output in the order they appear + * Process all messages from agent output in topologically sorted order * Dispatches each message to the appropriate handler while maintaining shared state (unified temporary ID map) * Tracks outputs created with unresolved temporary IDs and generates synthetic updates after resolution * + * Messages are sorted topologically based on temporary ID dependencies before processing. + * This ensures items without temporary IDs are created first, enabling single-pass resolution + * of temporary IDs in acyclic dependency graphs. + * * The unified temporary ID map stores both issue/PR references and project URLs: * - Issue/PR: temporary_id -> {repo: string, number: number} * - Project: temporary_id -> {projectUrl: string} @@ -367,6 +372,10 @@ async function processMessages(messageHandlers, messages, projectOctokit = null) // Load custom safe output job types that are processed by dedicated custom jobs const customSafeOutputJobTypes = loadCustomSafeOutputJobTypes(); + // Sort messages topologically based on temporary ID dependencies + // This ensures messages that create entities are processed before messages that reference them + const sortedMessages = sortSafeOutputMessages(messages); + // Initialize unified temporary ID map // This will be populated by handlers as they create entities with temporary IDs // Stores both issue/PR references ({repo, number}) and project URLs ({projectUrl}) @@ -393,11 +402,11 @@ async function processMessages(messageHandlers, messages, projectOctokit = null) /** @type {Array<{type: string, message: any, messageIndex: number, handler: Function}>} */ const deferredMessages = []; - core.info(`Processing ${messages.length} message(s) in order of appearance...`); + core.info(`Processing ${sortedMessages.length} message(s) in topologically sorted order...`); - // Process messages in order of appearance - for (let i = 0; i < messages.length; i++) { - const message = applyCampaignLabelsToMessage(messages[i], campaignLabels); + // Process messages in topologically sorted order + for (let i = 0; i < sortedMessages.length; i++) { + const message = applyCampaignLabelsToMessage(sortedMessages[i], campaignLabels); const messageType = message.type; if (!messageType) { diff --git a/actions/setup/js/temporary_id.cjs b/actions/setup/js/temporary_id.cjs index 7c0280fa68..482aac72ff 100644 --- a/actions/setup/js/temporary_id.cjs +++ b/actions/setup/js/temporary_id.cjs @@ -352,6 +352,86 @@ function replaceTemporaryProjectReferences(text, tempProjectMap) { }); } +/** + * Extract all temporary ID references from a message + * Checks fields that commonly contain temporary IDs: + * - body (for create_issue, create_discussion, add_comment) + * - parent_issue_number, sub_issue_number (for link_sub_issue) + * - issue_number (for add_comment, update_issue, etc.) + * - discussion_number (for create_discussion, update_discussion) + * + * @param {any} message - The safe output message + * @returns {Set} Set of normalized temporary IDs referenced by this message + */ +function extractTemporaryIdReferences(message) { + const tempIds = new Set(); + + if (!message || typeof message !== "object") { + return tempIds; + } + + // Check text fields for #aw_XXXXXXXXXXXX references + const textFields = ["body", "title", "description"]; + for (const field of textFields) { + if (typeof message[field] === "string") { + let match; + while ((match = TEMPORARY_ID_PATTERN.exec(message[field])) !== null) { + tempIds.add(normalizeTemporaryId(match[1])); + } + } + } + + // Check direct ID reference fields + const idFields = ["parent_issue_number", "sub_issue_number", "issue_number", "discussion_number", "pull_request_number"]; + + for (const field of idFields) { + const value = message[field]; + if (value !== undefined && value !== null) { + // Strip # prefix if present + const valueStr = String(value).trim(); + const valueWithoutHash = valueStr.startsWith("#") ? valueStr.substring(1) : valueStr; + + if (isTemporaryId(valueWithoutHash)) { + tempIds.add(normalizeTemporaryId(valueWithoutHash)); + } + } + } + + // Check items array for bulk operations (e.g., add_comment with multiple targets) + if (Array.isArray(message.items)) { + for (const item of message.items) { + if (item && typeof item === "object") { + const itemTempIds = extractTemporaryIdReferences(item); + for (const tempId of itemTempIds) { + tempIds.add(tempId); + } + } + } + } + + return tempIds; +} + +/** + * Get the temporary ID that a message will create (if any) + * Only messages with a temporary_id field will create a new entity + * + * @param {any} message - The safe output message + * @returns {string|null} Normalized temporary ID that will be created, or null + */ +function getCreatedTemporaryId(message) { + if (!message || typeof message !== "object") { + return null; + } + + const tempId = message.temporary_id; + if (tempId && isTemporaryId(String(tempId))) { + return normalizeTemporaryId(String(tempId)); + } + + return null; +} + module.exports = { TEMPORARY_ID_PATTERN, generateTemporaryId, @@ -367,4 +447,6 @@ module.exports = { serializeTemporaryIdMap, loadTemporaryProjectMap, replaceTemporaryProjectReferences, + extractTemporaryIdReferences, + getCreatedTemporaryId, }; diff --git a/actions/setup/js/temporary_id.test.cjs b/actions/setup/js/temporary_id.test.cjs index 87d8871187..b4abe36f83 100644 --- a/actions/setup/js/temporary_id.test.cjs +++ b/actions/setup/js/temporary_id.test.cjs @@ -464,4 +464,184 @@ describe("temporary_id.cjs", () => { expect(map.get("aw_abc123def456")).toBe("https://github.com/orgs/myorg/projects/123"); }); }); + + describe("extractTemporaryIdReferences", () => { + it("should extract temporary IDs from body field", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + title: "Test Issue", + body: "See #aw_abc123def456 and #aw_111222333444 for details", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(2); + expect(refs.has("aw_abc123def456")).toBe(true); + expect(refs.has("aw_111222333444")).toBe(true); + }); + + it("should extract temporary IDs from title field", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + title: "Follow up to #aw_abc123def456", + body: "Details here", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(1); + expect(refs.has("aw_abc123def456")).toBe(true); + }); + + it("should extract temporary IDs from direct ID fields", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "link_sub_issue", + parent_issue_number: "aw_aaaaaa123456", + sub_issue_number: "aw_bbbbbb123456", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(2); + expect(refs.has("aw_aaaaaa123456")).toBe(true); + expect(refs.has("aw_bbbbbb123456")).toBe(true); + }); + + it("should handle # prefix in ID fields", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "add_comment", + issue_number: "#aw_abc123def456", + body: "Comment text", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(1); + expect(refs.has("aw_abc123def456")).toBe(true); + }); + + it("should normalize temporary IDs to lowercase", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + body: "See #AW_ABC123DEF456", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(1); + expect(refs.has("aw_abc123def456")).toBe(true); + }); + + it("should extract from items array for bulk operations", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "add_comment", + items: [ + { issue_number: "aw_dddddd111111", body: "Comment 1" }, + { issue_number: "aw_eeeeee222222", body: "Comment 2" }, + ], + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(2); + expect(refs.has("aw_dddddd111111")).toBe(true); + expect(refs.has("aw_eeeeee222222")).toBe(true); + }); + + it("should return empty set for messages without temp IDs", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + title: "Regular Issue", + body: "No temporary IDs here", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(0); + }); + + it("should ignore invalid temporary ID formats", async () => { + const { extractTemporaryIdReferences } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + body: "Invalid: #aw_short #aw_toolongxxxxxxxx #temp_123456789012", + }; + + const refs = extractTemporaryIdReferences(message); + + expect(refs.size).toBe(0); + }); + }); + + describe("getCreatedTemporaryId", () => { + it("should return temporary_id when present and valid", async () => { + const { getCreatedTemporaryId } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + temporary_id: "aw_abc123def456", + title: "Test", + }; + + const created = getCreatedTemporaryId(message); + + expect(created).toBe("aw_abc123def456"); + }); + + it("should normalize created temporary ID to lowercase", async () => { + const { getCreatedTemporaryId } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + temporary_id: "AW_ABC123DEF456", + title: "Test", + }; + + const created = getCreatedTemporaryId(message); + + expect(created).toBe("aw_abc123def456"); + }); + + it("should return null when temporary_id is missing", async () => { + const { getCreatedTemporaryId } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + title: "Test", + }; + + const created = getCreatedTemporaryId(message); + + expect(created).toBe(null); + }); + + it("should return null when temporary_id is invalid", async () => { + const { getCreatedTemporaryId } = await import("./temporary_id.cjs"); + + const message = { + type: "create_issue", + temporary_id: "invalid_id", + title: "Test", + }; + + const created = getCreatedTemporaryId(message); + + expect(created).toBe(null); + }); + }); });