Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 48 additions & 13 deletions actions/setup/js/safe_output_handler_manager.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -325,7 +325,7 @@ function formatManifestLogMessage(item) {
* @param {Map<string, Function>} messageHandlers - Map of message handler functions
* @param {Array<Object>} 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<any>, temporaryIdMap: Object, outputsWithUnresolvedIds: Array<any>, missings: Object, codePushFailures: Array<{type: string, error: string}>}>}
* @returns {Promise<{success: boolean, results: Array<any>, temporaryIdMap: Object, artifactUrlMap: Map<string, string>, outputsWithUnresolvedIds: Array<any>, missings: Object, codePushFailures: Array<{type: string, error: string}>}>}
*/
async function processMessages(messageHandlers, messages, onItemCreated = null) {
const results = [];
Expand All @@ -338,6 +338,12 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
/** @type {Map<string, {repo: string, number: number}>} */
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<string, string>} */
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}>} */
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -555,6 +572,19 @@ 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);
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
// 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) {
Expand All @@ -568,7 +598,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) {
Expand All @@ -590,7 +620,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,
Expand Down Expand Up @@ -707,7 +737,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,
Expand Down Expand Up @@ -754,6 +784,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
success: true,
results,
temporaryIdMap: temporaryIdMapObj,
artifactUrlMap,
outputsWithUnresolvedIds,
missings,
codePushFailures,
Expand Down Expand Up @@ -910,24 +941,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<string, {repo: string, number: number}>} temporaryIdMap - Current temporary ID map
* @param {Map<string, string>} [artifactUrlMap] - Optional artifact URL map for resolving artifact references
* @returns {Promise<number>} 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) {
Expand All @@ -936,8 +970,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 = replaceArtifactUrlReferences(contentToCheck, artifactUrlMap);
updatedContent = replaceTemporaryIdReferences(updatedContent, temporaryIdMap, tracked.result.repo);

// Update based on the original type
switch (tracked.type) {
Expand Down Expand Up @@ -1090,7 +1125,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
Expand Down
17 changes: 17 additions & 0 deletions actions/setup/js/safe_outputs_action_outputs.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
*/
Expand Down Expand Up @@ -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 };
48 changes: 48 additions & 0 deletions actions/setup/js/safe_outputs_action_outputs.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
5 changes: 5 additions & 0 deletions actions/setup/js/safe_outputs_tools.json
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,11 @@
},
"additionalProperties": false
},
"temporary_id": {
"type": "string",
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upload_artifact.inputSchema.temporary_id is missing a pattern constraint even though other temporary_id fields in this file include one (e.g. create_issue and add_comment). Adding a pattern here would provide consistent schema-level validation and clearer feedback to callers about the accepted format.

Suggested change
"type": "string",
"type": "string",
"pattern": "^aw_[A-Za-z0-9_]{3,12}$",

Copilot uses AI. Check for mistakes.
"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": {
"type": "string",
"description": "Confidentiality level of the artifact content (e.g., \"public\", \"internal\", \"private\")."
Expand Down
49 changes: 45 additions & 4 deletions actions/setup/js/temporary_id.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, RepoIssuePair>|Object} tempIdMap - Map or object of temporary_id to {repo, number}
* @param {Map<string, string>} [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;
}
Expand All @@ -409,15 +411,53 @@ 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;
}
}

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<string, string>|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) {
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) {
// Replace #aw_XXXX with the URL directly (no '#' prefix)
return url;
}
return match;
});
Comment on lines +436 to +458
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaceArtifactUrlReferences currently does not emit any warning for malformed #aw_... tokens that won't match TEMPORARY_ID_PATTERN (e.g. #aw_bad-id). replaceTemporaryIdReferences in the same module does a candidate scan via TEMPORARY_ID_CANDIDATE_PATTERN to warn about these cases; consider mirroring that logic here so artifact references fail loudly/diagnosably rather than silently remaining broken.

Copilot uses AI. Check for mistakes.
}

/**
* Serialize the temporary ID map to JSON for output
* @param {Map<string, RepoIssuePair>} tempIdMap - Map of temporary_id to {repo, number}
Expand Down Expand Up @@ -642,6 +682,7 @@ module.exports = {
replaceTemporaryIdReferences,
replaceTemporaryIdReferencesInPatch,
replaceTemporaryIdReferencesLegacy,
replaceArtifactUrlReferences,
loadTemporaryIdMap,
loadTemporaryIdMapFromResolved,
resolveIssueNumber,
Expand Down
Loading