From b288b16cd3c294e23ca8ad3ad206106293df15d9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Apr 2026 23:15:11 +0000 Subject: [PATCH 1/4] feat: validate and resolve upload_artifact temporary IDs and URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `temporary_id` field to `upload_artifact` tool schema so agents can declare an artifact ID upfront, enabling topological sort ordering and `#aw_ID` reference embedding in subsequent message bodies - Update `upload_artifact.cjs` to use `message.temporary_id` when provided (via `resolveTemporaryArtifactId`), falling back to random generation - Add `replaceArtifactUrlReferences(text, artifactUrlMap)` to `temporary_id.cjs` for replacing `#aw_ID` references with actual artifact download URLs - Update `hasUnresolvedTemporaryIds` to accept optional `artifactUrlMap` so artifact URL references count as resolved - Update `safe_output_handler_manager.cjs`: - Track `artifactUrlMap: Map` alongside `temporaryIdMap` - After successful `upload_artifact`, register `tmpId → artifactUrl` - Pre-process message bodies to replace artifact URL refs before handler calls - Pass `artifactUrlMap` to all `hasUnresolvedTemporaryIds` checks - Update `processSyntheticUpdates` to apply artifact URL replacement - Include `artifactUrlMap` in processing result for synthetic updates - Add tests for new `resolveTemporaryArtifactId`, `replaceArtifactUrlReferences`, and updated `hasUnresolvedTemporaryIds` functionality Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9e280abe-a3c6-4060-a0ec-8fbb2812d072 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../setup/js/safe_output_handler_manager.cjs | 57 +++++++++++---- actions/setup/js/safe_outputs_tools.json | 4 ++ actions/setup/js/temporary_id.cjs | 38 ++++++++-- actions/setup/js/temporary_id.test.cjs | 71 +++++++++++++++++++ actions/setup/js/upload_artifact.cjs | 31 +++++++- actions/setup/js/upload_artifact.test.cjs | 58 +++++++++++++++ pkg/workflow/js/safe_outputs_tools.json | 4 ++ 7 files changed, 245 insertions(+), 18 deletions(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 2ee36e24445..28dc96befec 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -12,7 +12,7 @@ const { loadAgentOutput } = require("./load_agent_output.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { ERR_CONFIG, ERR_PARSE, ERR_VALIDATION } = require("./error_codes.cjs"); -const { hasUnresolvedTemporaryIds, replaceTemporaryIdReferences, normalizeTemporaryId } = require("./temporary_id.cjs"); +const { hasUnresolvedTemporaryIds, replaceTemporaryIdReferences, replaceArtifactUrlReferences, normalizeTemporaryId } = require("./temporary_id.cjs"); const { generateMissingInfoSections } = require("./missing_info_formatter.cjs"); const { setCollectedMissings } = require("./missing_messages_helper.cjs"); const { writeSafeOutputSummaries } = require("./safe_output_summary.cjs"); @@ -325,7 +325,7 @@ function formatManifestLogMessage(item) { * @param {Map} messageHandlers - Map of message handler functions * @param {Array} messages - Array of safe output messages * @param {((item: {type: string, url?: string, number?: number, repo?: string, temporaryId?: string}) => void)|null} [onItemCreated] - Optional callback invoked after each successful create operation (for manifest logging) - * @returns {Promise<{success: boolean, results: Array, temporaryIdMap: Object, outputsWithUnresolvedIds: Array, missings: Object, codePushFailures: Array<{type: string, error: string}>}>} + * @returns {Promise<{success: boolean, results: Array, temporaryIdMap: Object, artifactUrlMap: Map, outputsWithUnresolvedIds: Array, missings: Object, codePushFailures: Array<{type: string, error: string}>}>} */ async function processMessages(messageHandlers, messages, onItemCreated = null) { const results = []; @@ -338,6 +338,12 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) /** @type {Map} */ const temporaryIdMap = new Map(); + // Track artifact URL mappings: normalised tmpId → artifact download URL. + // Populated after each successful upload_artifact call so that subsequent + // messages can have '#aw_ID' references replaced with the real artifact URL. + /** @type {Map} */ + const artifactUrlMap = new Map(); + // Track outputs that were created with unresolved temporary IDs // Format: {type, message, result, originalTempIdMapSize} /** @type {Array<{type: string, message: any, result: any, originalTempIdMapSize: number}>} */ @@ -490,6 +496,17 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } } + // Pre-process: replace any '#aw_ID' artifact URL references in the message body + // with the actual artifact URL so handlers receive the resolved URL directly. + // This is applied to all message types that carry a 'body' field. + if (artifactUrlMap.size > 0 && effectiveMessage.body && typeof effectiveMessage.body === "string") { + const resolvedBody = replaceArtifactUrlReferences(effectiveMessage.body, artifactUrlMap); + if (resolvedBody !== effectiveMessage.body) { + effectiveMessage = { ...effectiveMessage, body: resolvedBody }; + core.info(`Resolved artifact URL reference(s) in ${messageType} body`); + } + } + // Call the message handler with the individual message and resolved temp IDs const result = await messageHandler(effectiveMessage, resolvedTemporaryIds, temporaryIdMap); @@ -555,6 +572,15 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) core.info(`Registered temporary ID: ${result.temporaryId} -> ${result.repo}#${result.number}`); } + // If this was a successful upload_artifact, register the artifact URL so that + // subsequent messages can have '#aw_ID' references replaced with the real URL. + // upload_artifact returns { tmpId, artifactUrl } (not temporaryId/repo/number). + if (messageType === "upload_artifact" && result && result.tmpId && result.artifactUrl) { + const normalizedTmpId = normalizeTemporaryId(result.tmpId); + artifactUrlMap.set(normalizedTmpId, result.artifactUrl); + core.info(`Registered artifact URL: ${result.tmpId} -> ${result.artifactUrl}`); + } + // Track when a code-push operation falls back to a review issue so subsequent // add_comment messages can include a correction note. if (CODE_PUSH_TYPES.has(messageType) && result && result.fallback_used === true && result.issue_number != null && result.issue_url) { @@ -568,7 +594,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) // Handle add_comment which returns an array of comments if (messageType === "add_comment" && Array.isArray(result)) { const contentToCheck = getContentToCheck(messageType, message); - if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap)) { + if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) { // Track each comment that was created with unresolved temp IDs for (const comment of result) { if (comment._tracking) { @@ -590,7 +616,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } else if (result && result.number && result.repo) { // Handle create_issue, create_discussion const contentToCheck = getContentToCheck(messageType, message); - if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap)) { + if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) { core.info(`Output ${result.repo}#${result.number} was created with unresolved temporary IDs - tracking for update`); outputsWithUnresolvedIds.push({ type: messageType, @@ -707,7 +733,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) // This enables synthetic updates to resolve references after all items are created if (result && result.number && result.repo) { const contentToCheck = getContentToCheck(deferred.type, deferred.message); - if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap)) { + if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) { core.info(`Output ${result.repo}#${result.number} was created with unresolved temporary IDs - tracking for update`); outputsWithUnresolvedIds.push({ type: deferred.type, @@ -754,6 +780,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) success: true, results, temporaryIdMap: temporaryIdMapObj, + artifactUrlMap, outputsWithUnresolvedIds, missings, codePushFailures, @@ -910,24 +937,27 @@ async function updateCommentBody(github, context, repo, commentId, updatedBody, * @param {any} context - GitHub Actions context * @param {Array<{type: string, message: any, result: any, originalTempIdMapSize: number}>} trackedOutputs - Outputs that need updating * @param {Map} temporaryIdMap - Current temporary ID map + * @param {Map} [artifactUrlMap] - Optional artifact URL map for resolving artifact references * @returns {Promise} Number of successful updates */ -async function processSyntheticUpdates(github, context, trackedOutputs, temporaryIdMap) { +async function processSyntheticUpdates(github, context, trackedOutputs, temporaryIdMap, artifactUrlMap) { let updateCount = 0; core.info(`\n=== Processing Synthetic Updates ===`); core.info(`Found ${trackedOutputs.length} output(s) with unresolved temporary IDs`); for (const tracked of trackedOutputs) { - // Check if any new temporary IDs were resolved since this output was created - // Only check and update if we have content to check - if (temporaryIdMap.size > tracked.originalTempIdMapSize) { + // Check if any new temporary IDs were resolved since this output was created. + // Also trigger an update when artifact URLs have been registered (artifactUrlMap is non-empty), + // since artifact IDs embedded in the body need to be replaced with their real URLs. + const resolvedArtifacts = artifactUrlMap && artifactUrlMap.size > 0; + if (temporaryIdMap.size > tracked.originalTempIdMapSize || resolvedArtifacts) { const contentToCheck = getContentToCheck(tracked.type, tracked.message); // Only process if we have content to check if (contentToCheck !== null && contentToCheck !== "") { // Check if the content still has unresolved IDs (some may now be resolved) - const stillHasUnresolved = hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap); + const stillHasUnresolved = hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap); const resolvedCount = temporaryIdMap.size - tracked.originalTempIdMapSize; if (!stillHasUnresolved) { @@ -936,8 +966,9 @@ async function processSyntheticUpdates(github, context, trackedOutputs, temporar core.info(`Updating ${tracked.type} ${logInfo} (${resolvedCount} temp ID(s) resolved)`); try { - // Replace temporary ID references with resolved values - const updatedContent = replaceTemporaryIdReferences(contentToCheck, temporaryIdMap, tracked.result.repo); + // Replace artifact URL references first, then issue number references + let updatedContent = artifactUrlMap && artifactUrlMap.size > 0 ? replaceArtifactUrlReferences(contentToCheck, artifactUrlMap) : contentToCheck; + updatedContent = replaceTemporaryIdReferences(updatedContent, temporaryIdMap, tracked.result.repo); // Update based on the original type switch (tracked.type) { @@ -1090,7 +1121,7 @@ async function main() { // Convert temp ID map back to Map const temporaryIdMap = new Map(Object.entries(processingResult.temporaryIdMap)); - syntheticUpdateCount = await processSyntheticUpdates(github, context, processingResult.outputsWithUnresolvedIds, temporaryIdMap); + syntheticUpdateCount = await processSyntheticUpdates(github, context, processingResult.outputsWithUnresolvedIds, temporaryIdMap, processingResult.artifactUrlMap); } // Write step summaries for all processed safe-outputs diff --git a/actions/setup/js/safe_outputs_tools.json b/actions/setup/js/safe_outputs_tools.json index a90502fcfdf..2c303673617 100644 --- a/actions/setup/js/safe_outputs_tools.json +++ b/actions/setup/js/safe_outputs_tools.json @@ -880,6 +880,10 @@ }, "additionalProperties": false }, + "temporary_id": { + "type": "string", + "description": "Optional temporary identifier for this artifact upload. Format: 'aw_' followed by 3 to 12 alphanumeric or underscore characters (e.g., 'aw_chart1', 'aw_img_out'). Declare this ID here if you plan to embed the artifact URL in a subsequent message body using '#aw_ID' — for example '![chart](#aw_chart1)' in a create_discussion body. The safe-outputs processor replaces '#aw_ID' references with the actual artifact download URL after upload. When skip-archive is true the URL points directly to the file and is suitable for inline images." + }, "secrecy": { "type": "string", "description": "Confidentiality level of the artifact content (e.g., \"public\", \"internal\", \"private\")." diff --git a/actions/setup/js/temporary_id.cjs b/actions/setup/js/temporary_id.cjs index cb21069ad54..e3561d4d01f 100644 --- a/actions/setup/js/temporary_id.cjs +++ b/actions/setup/js/temporary_id.cjs @@ -389,12 +389,14 @@ function resolveRepoIssueTarget(value, temporaryIdMap, defaultOwner, defaultRepo /** * Check if text contains unresolved temporary ID references - * An unresolved temporary ID is one that appears in the text but is not in the tempIdMap + * An unresolved temporary ID is one that appears in the text but is not in either + * the tempIdMap (issue/PR/discussion numbers) or the artifactUrlMap (artifact URLs). * @param {string} text - The text to check for unresolved temporary IDs * @param {Map|Object} tempIdMap - Map or object of temporary_id to {repo, number} + * @param {Map} [artifactUrlMap] - Optional map of temporary artifact ID to URL * @returns {boolean} True if text contains any unresolved temporary IDs */ -function hasUnresolvedTemporaryIds(text, tempIdMap) { +function hasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap) { if (!text || typeof text !== "string") { return false; } @@ -409,8 +411,8 @@ function hasUnresolvedTemporaryIds(text, tempIdMap) { const tempId = match[1]; // The captured group (aw_XXXXXXXXXXXX) const normalizedId = normalizeTemporaryId(tempId); - // If this temp ID is not in the map, it's unresolved - if (!map.has(normalizedId)) { + // Resolved if present in either the issue/number map or the artifact URL map + if (!map.has(normalizedId) && !(artifactUrlMap && artifactUrlMap.has(normalizedId))) { return true; } } @@ -418,6 +420,33 @@ function hasUnresolvedTemporaryIds(text, tempIdMap) { return false; } +/** + * Replace temporary artifact ID references in text with actual artifact URLs. + * Handles the case where a temporary ID was declared on an upload_artifact message + * and subsequently embedded in issue/discussion/comment bodies as an image source + * or hyperlink (e.g. ![chart](#aw_chart1) → ![chart](https://…/artifacts/42)). + * + * Unlike issue-number references (which produce #N), artifact references are replaced + * with the full URL string so the '#' prefix is stripped in the output. + * + * @param {string} text - The text to process + * @param {Map} artifactUrlMap - Map of normalised temporary artifact ID to URL + * @returns {string} Text with artifact ID references replaced by their URLs + */ +function replaceArtifactUrlReferences(text, artifactUrlMap) { + if (!artifactUrlMap || artifactUrlMap.size === 0) { + return text; + } + return text.replace(TEMPORARY_ID_PATTERN, (match, tempId) => { + const url = artifactUrlMap.get(normalizeTemporaryId(tempId)); + if (url !== undefined) { + // Replace #aw_XXXX with the URL directly (no '#' prefix) + return url; + } + return match; + }); +} + /** * Serialize the temporary ID map to JSON for output * @param {Map} tempIdMap - Map of temporary_id to {repo, number} @@ -642,6 +671,7 @@ module.exports = { replaceTemporaryIdReferences, replaceTemporaryIdReferencesInPatch, replaceTemporaryIdReferencesLegacy, + replaceArtifactUrlReferences, loadTemporaryIdMap, loadTemporaryIdMapFromResolved, resolveIssueNumber, diff --git a/actions/setup/js/temporary_id.test.cjs b/actions/setup/js/temporary_id.test.cjs index 243a49313e2..f52355a2ad3 100644 --- a/actions/setup/js/temporary_id.test.cjs +++ b/actions/setup/js/temporary_id.test.cjs @@ -565,6 +565,77 @@ describe("temporary_id.cjs", () => { const text = "See #aw_abc123, #aw_test123, and #aw_xyz9"; expect(hasUnresolvedTemporaryIds(text, map)).toBe(true); }); + + it("should treat ID as resolved when present in artifactUrlMap", async () => { + const { hasUnresolvedTemporaryIds } = await import("./temporary_id.cjs"); + const tempIdMap = new Map(); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "![chart](#aw_chart1)"; + expect(hasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap)).toBe(false); + }); + + it("should still detect unresolved ID when not in either map", async () => { + const { hasUnresolvedTemporaryIds } = await import("./temporary_id.cjs"); + const tempIdMap = new Map([["aw_issue1", { repo: "owner/repo", number: 1 }]]); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "See #aw_issue1 and ![chart](#aw_chart1) and #aw_unknown"; + expect(hasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap)).toBe(true); + }); + + it("should return false when all IDs are covered across both maps", async () => { + const { hasUnresolvedTemporaryIds } = await import("./temporary_id.cjs"); + const tempIdMap = new Map([["aw_issue1", { repo: "owner/repo", number: 1 }]]); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "See #aw_issue1 and ![chart](#aw_chart1)"; + expect(hasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap)).toBe(false); + }); + }); + + describe("replaceArtifactUrlReferences", () => { + it("should replace #aw_ID with the artifact URL", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "![chart](#aw_chart1)"; + expect(replaceArtifactUrlReferences(text, artifactUrlMap)).toBe("![chart](https://github.com/owner/repo/actions/runs/1/artifacts/42)"); + }); + + it("should be case-insensitive for the temporary ID", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "![chart](#aw_Chart1)"; + expect(replaceArtifactUrlReferences(text, artifactUrlMap)).toBe("![chart](https://github.com/owner/repo/actions/runs/1/artifacts/42)"); + }); + + it("should leave non-artifact temporary IDs unchanged", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "See #aw_issue1 and ![chart](#aw_chart1)"; + expect(replaceArtifactUrlReferences(text, artifactUrlMap)).toBe("See #aw_issue1 and ![chart](https://github.com/owner/repo/actions/runs/1/artifacts/42)"); + }); + + it("should handle multiple artifact references in the same text", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([ + ["aw_img1", "https://github.com/owner/repo/actions/runs/1/artifacts/10"], + ["aw_img2", "https://github.com/owner/repo/actions/runs/1/artifacts/20"], + ]); + const text = "![before](#aw_img1) and ![after](#aw_img2)"; + expect(replaceArtifactUrlReferences(text, artifactUrlMap)).toBe("![before](https://github.com/owner/repo/actions/runs/1/artifacts/10) and ![after](https://github.com/owner/repo/actions/runs/1/artifacts/20)"); + }); + + it("should return text unchanged when artifactUrlMap is empty", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map(); + const text = "![chart](#aw_chart1)"; + expect(replaceArtifactUrlReferences(text, artifactUrlMap)).toBe(text); + }); + + it("should return text unchanged when artifactUrlMap is null/undefined", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const text = "![chart](#aw_chart1)"; + expect(replaceArtifactUrlReferences(text, null)).toBe(text); + expect(replaceArtifactUrlReferences(text, undefined)).toBe(text); + }); }); describe("replaceTemporaryProjectReferences", () => { diff --git a/actions/setup/js/upload_artifact.cjs b/actions/setup/js/upload_artifact.cjs index 67b2076ca29..3311b3a80e2 100644 --- a/actions/setup/js/upload_artifact.cjs +++ b/actions/setup/js/upload_artifact.cjs @@ -37,6 +37,7 @@ const path = require("path"); const { getErrorMessage } = require("./error_helpers.cjs"); const { globPatternToRegex } = require("./glob_pattern_helpers.cjs"); const { ERR_VALIDATION } = require("./error_codes.cjs"); +const { isTemporaryId, normalizeTemporaryId } = require("./temporary_id.cjs"); /** * Staging directory where the model places files to be uploaded. @@ -64,6 +65,34 @@ function generateTemporaryArtifactId() { return id; } +/** + * Resolve the temporary artifact ID from an upload_artifact message. + * If the message declares a valid `temporary_id`, that value is used (normalised to lowercase). + * Otherwise a random ID is generated. + * + * Honouring `message.temporary_id` is essential for topological ordering: when an agent + * declares a `temporary_id` on the `upload_artifact` message and then embeds `#aw_ID` in a + * subsequent message body, the dependency graph built by the topological sort can order + * the upload before the referencing message, and the handler manager can replace the + * reference with the resolved artifact URL. + * + * @param {Record} message - The upload_artifact message from the model + * @returns {string} The temporary artifact ID to use for this upload + */ +function resolveTemporaryArtifactId(message) { + const declared = message.temporary_id; + if (declared && typeof declared === "string") { + const normalized = declared.trim().startsWith("#") ? declared.trim().substring(1).trim() : declared.trim(); + if (isTemporaryId(normalized)) { + return normalizeTemporaryId(normalized); + } + if (typeof core !== "undefined") { + core.warning(`upload_artifact: invalid temporary_id format '${declared}'. ` + `Temporary IDs must be 'aw_' followed by 3–12 alphanumeric or underscore characters. ` + `A random ID will be generated instead.`); + } + } + return generateTemporaryArtifactId(); +} + /** * Check whether a relative path matches any of the provided glob patterns. * @param {string} relPath - Path relative to the staging root @@ -491,7 +520,7 @@ async function main(config = {}) { // Derive artifact name and generate temporary ID. const artifactName = deriveArtifactName(message, i); - const tmpId = generateTemporaryArtifactId(); + const tmpId = resolveTemporaryArtifactId(message); resolver[tmpId] = artifactName; core.info(`Slot ${i}: artifact="${artifactName}", files=${files.length}, size=${totalSize}B, retention=${retentionDays}d, skip_archive=${skipArchive}, tmp_id=${tmpId}`); diff --git a/actions/setup/js/upload_artifact.test.cjs b/actions/setup/js/upload_artifact.test.cjs index 1512c28d001..f76116d32a4 100644 --- a/actions/setup/js/upload_artifact.test.cjs +++ b/actions/setup/js/upload_artifact.test.cjs @@ -404,6 +404,64 @@ describe("upload_artifact.cjs", () => { }); }); + describe("temporary_id field", () => { + it("uses declared temporary_id from message when valid", async () => { + writeStaging("chart.png", "PNG_DATA"); + + const results = await runHandler(buildConfig({ "skip-archive": true }), [{ type: "upload_artifact", path: "chart.png", temporary_id: "aw_chart1" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toBe("aw_chart1"); + expect(mockCore.setOutput).toHaveBeenCalledWith("slot_0_tmp_id", "aw_chart1"); + }); + + it("normalises declared temporary_id to lowercase", async () => { + writeStaging("chart.png", "PNG_DATA"); + + const results = await runHandler(buildConfig({ "skip-archive": true }), [{ type: "upload_artifact", path: "chart.png", temporary_id: "aw_CHART1" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toBe("aw_chart1"); + }); + + it("strips leading '#' from declared temporary_id", async () => { + writeStaging("chart.png", "PNG_DATA"); + + const results = await runHandler(buildConfig({ "skip-archive": true }), [{ type: "upload_artifact", path: "chart.png", temporary_id: "#aw_chart1" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toBe("aw_chart1"); + }); + + it("generates a random ID when temporary_id is not provided", async () => { + writeStaging("report.json"); + + const results = await runHandler(buildConfig(), [{ type: "upload_artifact", path: "report.json" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toMatch(/^aw_[A-Za-z0-9]{8}$/); + }); + + it("generates a random ID and emits warning when temporary_id format is invalid", async () => { + writeStaging("report.json"); + + const results = await runHandler(buildConfig(), [{ type: "upload_artifact", path: "report.json", temporary_id: "bad-format" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toMatch(/^aw_[A-Za-z0-9]{8}$/); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("invalid temporary_id format")); + }); + + it("uses declared temporary_id in resolver file", async () => { + writeStaging("chart.png", "PNG_DATA"); + + await runHandler(buildConfig({ "skip-archive": true }), [{ type: "upload_artifact", path: "chart.png", temporary_id: "aw_chart1" }]); + + const resolver = JSON.parse(fs.readFileSync(RESOLVER_FILE, "utf8")); + expect(resolver["aw_chart1"]).toBe("chart.png"); + }); + }); + describe("auto-copy from outside staging directory", () => { const WORKSPACE_DIR = "/tmp/gh-aw-test-workspace"; diff --git a/pkg/workflow/js/safe_outputs_tools.json b/pkg/workflow/js/safe_outputs_tools.json index 6543eb652f8..66654963a63 100644 --- a/pkg/workflow/js/safe_outputs_tools.json +++ b/pkg/workflow/js/safe_outputs_tools.json @@ -1615,6 +1615,10 @@ }, "additionalProperties": false }, + "temporary_id": { + "type": "string", + "description": "Optional temporary identifier for this artifact upload. Format: 'aw_' followed by 3 to 12 alphanumeric or underscore characters (e.g., 'aw_chart1', 'aw_img_out'). Declare this ID here if you plan to embed the artifact URL in a subsequent message body using '#aw_ID' — for example '![chart](#aw_chart1)' in a create_discussion body. The safe-outputs processor replaces '#aw_ID' references with the actual artifact download URL after upload. When skip-archive is true the URL points directly to the file and is suitable for inline images." + }, "secrecy": { "type": "string", "description": "Confidentiality level of the artifact content (e.g., \"public\", \"internal\", \"private\")." From a9d5512b93e7c71e76654d222bd53f15d56e4d42 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Apr 2026 23:16:41 +0000 Subject: [PATCH 2/4] fix: address code review feedback - simplify normalization and redundant check Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9e280abe-a3c6-4060-a0ec-8fbb2812d072 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/safe_output_handler_manager.cjs | 2 +- actions/setup/js/upload_artifact.cjs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 28dc96befec..cdbcc512e96 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -967,7 +967,7 @@ async function processSyntheticUpdates(github, context, trackedOutputs, temporar try { // Replace artifact URL references first, then issue number references - let updatedContent = artifactUrlMap && artifactUrlMap.size > 0 ? replaceArtifactUrlReferences(contentToCheck, artifactUrlMap) : contentToCheck; + let updatedContent = replaceArtifactUrlReferences(contentToCheck, artifactUrlMap); updatedContent = replaceTemporaryIdReferences(updatedContent, temporaryIdMap, tracked.result.repo); // Update based on the original type diff --git a/actions/setup/js/upload_artifact.cjs b/actions/setup/js/upload_artifact.cjs index 3311b3a80e2..ea755e43e40 100644 --- a/actions/setup/js/upload_artifact.cjs +++ b/actions/setup/js/upload_artifact.cjs @@ -82,7 +82,8 @@ function generateTemporaryArtifactId() { function resolveTemporaryArtifactId(message) { const declared = message.temporary_id; if (declared && typeof declared === "string") { - const normalized = declared.trim().startsWith("#") ? declared.trim().substring(1).trim() : declared.trim(); + const trimmed = declared.trim(); + const normalized = trimmed.startsWith("#") ? trimmed.substring(1) : trimmed; if (isTemporaryId(normalized)) { return normalizeTemporaryId(normalized); } From d57abae79c384def25f8f338ee2ce5685a07668d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Apr 2026 23:56:09 +0000 Subject: [PATCH 3/4] feat: return temporaryId from upload_artifact handler and emit named action outputs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/21c1222b-06ce-47c5-aa80-05d54d1b0e2a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../setup/js/safe_outputs_action_outputs.cjs | 17 +++++++ .../js/safe_outputs_action_outputs.test.cjs | 48 +++++++++++++++++++ actions/setup/js/temporary_id.cjs | 2 +- actions/setup/js/upload_artifact.cjs | 3 +- actions/setup/js/upload_artifact.test.cjs | 10 ++++ 5 files changed, 78 insertions(+), 2 deletions(-) diff --git a/actions/setup/js/safe_outputs_action_outputs.cjs b/actions/setup/js/safe_outputs_action_outputs.cjs index 2c4a0f63632..561bc7cda0d 100644 --- a/actions/setup/js/safe_outputs_action_outputs.cjs +++ b/actions/setup/js/safe_outputs_action_outputs.cjs @@ -30,6 +30,7 @@ * create_pull_request → created_pr_number, created_pr_url * add_comment → comment_id, comment_url * push_to_pull_request_branch → push_commit_sha, push_commit_url + * upload_artifact → upload_artifact_tmp_id, upload_artifact_url * * @param {ProcessingResult} processingResult - Result from processMessages() */ @@ -93,6 +94,22 @@ function emitSafeOutputActionOutputs(processingResult) { core.info(`Exported push_commit_url: ${r.commit_url}`); } } + + // upload_artifact: upload_artifact_tmp_id, upload_artifact_url + // Returns the temporary ID (generated or agent-declared) and the artifact download URL + // for the first successfully uploaded artifact. + const firstArtifactResult = successfulResults.find(r => r.type === "upload_artifact"); + if (firstArtifactResult?.result && !Array.isArray(firstArtifactResult.result)) { + const r = firstArtifactResult.result; + if (r.temporaryId) { + core.setOutput("upload_artifact_tmp_id", r.temporaryId); + core.info(`Exported upload_artifact_tmp_id: ${r.temporaryId}`); + } + if (r.artifactUrl) { + core.setOutput("upload_artifact_url", r.artifactUrl); + core.info(`Exported upload_artifact_url: ${r.artifactUrl}`); + } + } } module.exports = { emitSafeOutputActionOutputs }; diff --git a/actions/setup/js/safe_outputs_action_outputs.test.cjs b/actions/setup/js/safe_outputs_action_outputs.test.cjs index 3e23a4d4dd1..e160ce588df 100644 --- a/actions/setup/js/safe_outputs_action_outputs.test.cjs +++ b/actions/setup/js/safe_outputs_action_outputs.test.cjs @@ -99,18 +99,66 @@ describe("emitSafeOutputActionOutputs", () => { expect(outputs["push_commit_url"]).toBe("https://github.com/owner/repo/commit/abc123"); }); + it("emits upload_artifact_tmp_id and upload_artifact_url for upload_artifact result", () => { + emitSafeOutputActionOutputs({ + results: [ + { + success: true, + type: "upload_artifact", + // temporaryId is the field used by emitSafeOutputActionOutputs; tmpId is the legacy alias + result: { temporaryId: "aw_chart1", artifactUrl: "https://github.com/owner/repo/actions/runs/1/artifacts/42" }, + }, + ], + }); + + expect(outputs["upload_artifact_tmp_id"]).toBe("aw_chart1"); + expect(outputs["upload_artifact_url"]).toBe("https://github.com/owner/repo/actions/runs/1/artifacts/42"); + }); + + it("emits only the first successful upload_artifact result", () => { + emitSafeOutputActionOutputs({ + results: [ + { success: true, type: "upload_artifact", result: { temporaryId: "aw_first", artifactUrl: "https://github.com/owner/repo/actions/runs/1/artifacts/10" } }, + { success: true, type: "upload_artifact", result: { temporaryId: "aw_second", artifactUrl: "https://github.com/owner/repo/actions/runs/1/artifacts/20" } }, + ], + }); + + expect(outputs["upload_artifact_tmp_id"]).toBe("aw_first"); + expect(outputs["upload_artifact_url"]).toBe("https://github.com/owner/repo/actions/runs/1/artifacts/10"); + }); + + it("emits upload_artifact_tmp_id even when artifactUrl is empty string (staged mode)", () => { + emitSafeOutputActionOutputs({ + results: [{ success: true, type: "upload_artifact", result: { temporaryId: "aw_staged", artifactUrl: "" } }], + }); + + expect(outputs["upload_artifact_tmp_id"]).toBe("aw_staged"); + expect(outputs["upload_artifact_url"]).toBeUndefined(); + }); + + it("emits upload_artifact_tmp_id even when artifactUrl is absent (undefined)", () => { + emitSafeOutputActionOutputs({ + results: [{ success: true, type: "upload_artifact", result: { temporaryId: "aw_staged" } }], + }); + + expect(outputs["upload_artifact_tmp_id"]).toBe("aw_staged"); + expect(outputs["upload_artifact_url"]).toBeUndefined(); + }); + it("emits outputs for multiple different types in a single run", () => { emitSafeOutputActionOutputs({ results: [ { success: true, type: "create_issue", result: { number: 1, url: "https://github.com/owner/repo/issues/1" } }, { success: true, type: "add_comment", result: { commentId: 200, url: "https://github.com/owner/repo/issues/1#issuecomment-200" } }, { success: true, type: "push_to_pull_request_branch", result: { commit_sha: "sha1", commit_url: "https://github.com/owner/repo/commit/sha1" } }, + { success: true, type: "upload_artifact", result: { temporaryId: "aw_chart1", artifactUrl: "https://github.com/owner/repo/actions/runs/1/artifacts/42" } }, ], }); expect(outputs["created_issue_number"]).toBe("1"); expect(outputs["comment_id"]).toBe("200"); expect(outputs["push_commit_sha"]).toBe("sha1"); + expect(outputs["upload_artifact_tmp_id"]).toBe("aw_chart1"); }); it("emits no outputs when there are no successful results", () => { diff --git a/actions/setup/js/temporary_id.cjs b/actions/setup/js/temporary_id.cjs index e3561d4d01f..a3aa7cc809d 100644 --- a/actions/setup/js/temporary_id.cjs +++ b/actions/setup/js/temporary_id.cjs @@ -430,7 +430,7 @@ function hasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap) { * with the full URL string so the '#' prefix is stripped in the output. * * @param {string} text - The text to process - * @param {Map} artifactUrlMap - Map of normalised temporary artifact ID to URL + * @param {Map|null|undefined} artifactUrlMap - Map of normalised temporary artifact ID to URL * @returns {string} Text with artifact ID references replaced by their URLs */ function replaceArtifactUrlReferences(text, artifactUrlMap) { diff --git a/actions/setup/js/upload_artifact.cjs b/actions/setup/js/upload_artifact.cjs index ea755e43e40..c0b7e8846ad 100644 --- a/actions/setup/js/upload_artifact.cjs +++ b/actions/setup/js/upload_artifact.cjs @@ -475,7 +475,7 @@ async function main(config = {}) { * @param {Object} message - The upload_artifact message from the model * @param {Object} resolvedTemporaryIds - Map of already-resolved temporary IDs (unused here) * @param {Map} temporaryIdMap - Shared temp-ID map; the handler does not modify it - * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, artifactName?: string, artifactId?: number, artifactUrl?: string, slotIndex?: number}>} + * @returns {Promise<{success: boolean, error?: string, skipped?: boolean, tmpId?: string, temporaryId?: string, artifactName?: string, artifactId?: number, artifactUrl?: string, slotIndex?: number}>} */ return async function handleUploadArtifact(message, resolvedTemporaryIds, temporaryIdMap) { if (slotIndex >= maxUploads) { @@ -592,6 +592,7 @@ async function main(config = {}) { return { success: true, tmpId, + temporaryId: tmpId, artifactName, artifactId, artifactUrl, diff --git a/actions/setup/js/upload_artifact.test.cjs b/actions/setup/js/upload_artifact.test.cjs index f76116d32a4..12f5b701ccd 100644 --- a/actions/setup/js/upload_artifact.test.cjs +++ b/actions/setup/js/upload_artifact.test.cjs @@ -124,6 +124,16 @@ describe("upload_artifact.cjs", () => { expect(mockCore.setOutput).toHaveBeenCalledWith("upload_artifact_count", "1"); }); + it("returns temporaryId matching tmpId in the result", async () => { + writeStaging("report.json"); + + const results = await runHandler(buildConfig(), [{ type: "upload_artifact", path: "report.json" }]); + + expect(results[0].success).toBe(true); + expect(results[0].tmpId).toBeDefined(); + expect(results[0].temporaryId).toBe(results[0].tmpId); + }); + it("uses default retention of 30 when retention-days not in config", async () => { writeStaging("report.json"); From fb06ae1e6823e242622989dd212baa0e6434d1e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 00:08:43 +0000 Subject: [PATCH 4/4] fix: address all code review comments - patterns, duplicate warnings, malformed ref warnings Agent-Logs-Url: https://github.com/github/gh-aw/sessions/30754141-3e79-4bcd-a5ba-264de1a860c1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../setup/js/safe_output_handler_manager.cjs | 8 +++++-- actions/setup/js/safe_outputs_tools.json | 1 + actions/setup/js/temporary_id.cjs | 11 ++++++++++ actions/setup/js/temporary_id.test.cjs | 17 +++++++++++++++ actions/setup/js/upload_artifact.cjs | 6 +++++- actions/setup/js/upload_artifact.test.cjs | 21 +++++++++++++++++++ pkg/workflow/js/safe_outputs_tools.json | 1 + 7 files changed, 62 insertions(+), 3 deletions(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index cdbcc512e96..52bfc6417de 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -577,8 +577,12 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) // upload_artifact returns { tmpId, artifactUrl } (not temporaryId/repo/number). if (messageType === "upload_artifact" && result && result.tmpId && result.artifactUrl) { const normalizedTmpId = normalizeTemporaryId(result.tmpId); - artifactUrlMap.set(normalizedTmpId, result.artifactUrl); - core.info(`Registered artifact URL: ${result.tmpId} -> ${result.artifactUrl}`); + if (!artifactUrlMap.has(normalizedTmpId)) { + artifactUrlMap.set(normalizedTmpId, result.artifactUrl); + core.info(`Registered artifact URL for temporary ID: ${result.tmpId}`); + } else { + core.warning(`Duplicate artifact temporary ID '${result.tmpId}' encountered; keeping the first registered URL and ignoring the later upload.`); + } } // Track when a code-push operation falls back to a review issue so subsequent diff --git a/actions/setup/js/safe_outputs_tools.json b/actions/setup/js/safe_outputs_tools.json index 2c303673617..3645569a4d8 100644 --- a/actions/setup/js/safe_outputs_tools.json +++ b/actions/setup/js/safe_outputs_tools.json @@ -882,6 +882,7 @@ }, "temporary_id": { "type": "string", + "pattern": "^aw_[A-Za-z0-9_]{3,12}$", "description": "Optional temporary identifier for this artifact upload. Format: 'aw_' followed by 3 to 12 alphanumeric or underscore characters (e.g., 'aw_chart1', 'aw_img_out'). Declare this ID here if you plan to embed the artifact URL in a subsequent message body using '#aw_ID' — for example '![chart](#aw_chart1)' in a create_discussion body. The safe-outputs processor replaces '#aw_ID' references with the actual artifact download URL after upload. When skip-archive is true the URL points directly to the file and is suitable for inline images." }, "secrecy": { diff --git a/actions/setup/js/temporary_id.cjs b/actions/setup/js/temporary_id.cjs index a3aa7cc809d..55610f46d9c 100644 --- a/actions/setup/js/temporary_id.cjs +++ b/actions/setup/js/temporary_id.cjs @@ -437,6 +437,17 @@ function replaceArtifactUrlReferences(text, artifactUrlMap) { if (!artifactUrlMap || artifactUrlMap.size === 0) { return text; } + // Detect and warn about malformed #aw_ references that won't be resolved + let candidate; + TEMPORARY_ID_CANDIDATE_PATTERN.lastIndex = 0; + while ((candidate = TEMPORARY_ID_CANDIDATE_PATTERN.exec(text)) !== null) { + const tempId = `aw_${candidate[1]}`; + if (!isTemporaryId(tempId)) { + core.warning( + `Malformed temporary ID reference '${candidate[0]}' found in body text. This reference will not be replaced with an artifact URL. Temporary IDs must be in format '#aw_' followed by 3 to 12 alphanumeric or underscore characters (A-Za-z0-9_). Example: '#aw_chart1' or '#aw_img_out'` + ); + } + } return text.replace(TEMPORARY_ID_PATTERN, (match, tempId) => { const url = artifactUrlMap.get(normalizeTemporaryId(tempId)); if (url !== undefined) { diff --git a/actions/setup/js/temporary_id.test.cjs b/actions/setup/js/temporary_id.test.cjs index f52355a2ad3..4e98a6c2391 100644 --- a/actions/setup/js/temporary_id.test.cjs +++ b/actions/setup/js/temporary_id.test.cjs @@ -636,6 +636,23 @@ describe("temporary_id.cjs", () => { expect(replaceArtifactUrlReferences(text, null)).toBe(text); expect(replaceArtifactUrlReferences(text, undefined)).toBe(text); }); + + it("should warn about malformed #aw_ references (too short) when map is non-empty", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + // "#aw_ab" has only 2 characters after "aw_" (too short, minimum is 3) + const text = "![bad](#aw_ab)"; + replaceArtifactUrlReferences(text, artifactUrlMap); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("#aw_ab")); + }); + + it("should warn about malformed #aw_ references containing hyphens when map is non-empty", async () => { + const { replaceArtifactUrlReferences } = await import("./temporary_id.cjs"); + const artifactUrlMap = new Map([["aw_chart1", "https://github.com/owner/repo/actions/runs/1/artifacts/42"]]); + const text = "![bad](#aw_bad-id)"; + replaceArtifactUrlReferences(text, artifactUrlMap); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("#aw_bad-id")); + }); }); describe("replaceTemporaryProjectReferences", () => { diff --git a/actions/setup/js/upload_artifact.cjs b/actions/setup/js/upload_artifact.cjs index c0b7e8846ad..a92d14794da 100644 --- a/actions/setup/js/upload_artifact.cjs +++ b/actions/setup/js/upload_artifact.cjs @@ -522,7 +522,11 @@ async function main(config = {}) { // Derive artifact name and generate temporary ID. const artifactName = deriveArtifactName(message, i); const tmpId = resolveTemporaryArtifactId(message); - resolver[tmpId] = artifactName; + if (Object.prototype.hasOwnProperty.call(resolver, tmpId)) { + core.warning(`upload_artifact: duplicate temporary_id "${tmpId}" detected for artifact "${artifactName}". Using the first occurrence. Ensure each artifact has a unique temporary_id.`); + } else { + resolver[tmpId] = artifactName; + } core.info(`Slot ${i}: artifact="${artifactName}", files=${files.length}, size=${totalSize}B, retention=${retentionDays}d, skip_archive=${skipArchive}, tmp_id=${tmpId}`); diff --git a/actions/setup/js/upload_artifact.test.cjs b/actions/setup/js/upload_artifact.test.cjs index 12f5b701ccd..88dd7cf5cb1 100644 --- a/actions/setup/js/upload_artifact.test.cjs +++ b/actions/setup/js/upload_artifact.test.cjs @@ -470,6 +470,27 @@ describe("upload_artifact.cjs", () => { const resolver = JSON.parse(fs.readFileSync(RESOLVER_FILE, "utf8")); expect(resolver["aw_chart1"]).toBe("chart.png"); }); + + it("warns and keeps first mapping when the same temporary_id is used in multiple uploads", async () => { + writeStaging("chart1.png", "PNG_DATA_1"); + writeStaging("chart2.png", "PNG_DATA_2"); + + const results = await runHandler(buildConfig({ "max-uploads": 2 }), [ + { type: "upload_artifact", path: "chart1.png", temporary_id: "aw_chart1" }, + { type: "upload_artifact", path: "chart2.png", temporary_id: "aw_chart1" }, + ]); + + // Both uploads succeed individually + expect(results[0].success).toBe(true); + expect(results[1].success).toBe(true); + + // Resolver should keep the first mapping (chart1.png) + const resolver = JSON.parse(fs.readFileSync(RESOLVER_FILE, "utf8")); + expect(resolver["aw_chart1"]).toBe("chart1.png"); + + // A warning should have been emitted for the duplicate + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining('duplicate temporary_id "aw_chart1"')); + }); }); describe("auto-copy from outside staging directory", () => { diff --git a/pkg/workflow/js/safe_outputs_tools.json b/pkg/workflow/js/safe_outputs_tools.json index 66654963a63..18a72f3316f 100644 --- a/pkg/workflow/js/safe_outputs_tools.json +++ b/pkg/workflow/js/safe_outputs_tools.json @@ -1617,6 +1617,7 @@ }, "temporary_id": { "type": "string", + "pattern": "^aw_[A-Za-z0-9_]{3,12}$", "description": "Optional temporary identifier for this artifact upload. Format: 'aw_' followed by 3 to 12 alphanumeric or underscore characters (e.g., 'aw_chart1', 'aw_img_out'). Declare this ID here if you plan to embed the artifact URL in a subsequent message body using '#aw_ID' — for example '![chart](#aw_chart1)' in a create_discussion body. The safe-outputs processor replaces '#aw_ID' references with the actual artifact download URL after upload. When skip-archive is true the URL points directly to the file and is suitable for inline images." }, "secrecy": {