feat: resolve upload_artifact temporary IDs to artifact URLs in safe output bodies#26108
feat: resolve upload_artifact temporary IDs to artifact URLs in safe output bodies#26108
Conversation
- 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<string, string>` 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>
…ant 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>
|
@copilot the safe output upload-artifact should return the generated or provided temporary id as well |
…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>
Done in d57abae. The
|
There was a problem hiding this comment.
Pull request overview
Adds first-class support for stable upload_artifact temporary IDs and resolves #aw_ID references in downstream “safe output” message bodies to the final artifact download URL, preventing broken inline images/links.
Changes:
- Extend the
upload_artifacttool schema(s) with an optionaltemporary_idfield for stable cross-message references. - Teach
upload_artifactto honor a valid declaredtemporary_id(normalize/validate; warn+fallback on invalid). - Track
tmpId → artifactUrlin the handler manager and rewrite message bodies (and synthetic updates) by replacing#aw_IDwith the resolved artifact URL.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Adds upload_artifact.temporary_id to the workflow-side tool schema. |
| actions/setup/js/safe_outputs_tools.json | Adds upload_artifact.temporary_id to the action runtime tool schema. |
| actions/setup/js/upload_artifact.cjs | Resolves a stable tmpId from message.temporary_id when valid; falls back to generated IDs otherwise. |
| actions/setup/js/upload_artifact.test.cjs | Adds coverage for declared/normalized/invalid temporary_id behavior and resolver output. |
| actions/setup/js/temporary_id.cjs | Adds artifact URL replacement and expands unresolved-ID checks to include artifact URL mappings. |
| actions/setup/js/temporary_id.test.cjs | Adds tests for artifact URL replacement and unresolved-ID logic across both maps. |
| actions/setup/js/safe_output_handler_manager.cjs | Maintains an artifactUrlMap, pre-rewrites bodies before dispatch, and resolves artifact refs in synthetic updates. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 5
| "additionalProperties": false | ||
| }, | ||
| "temporary_id": { | ||
| "type": "string", |
There was a problem hiding this comment.
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.
| "type": "string", | |
| "type": "string", | |
| "pattern": "^aw_[A-Za-z0-9_]{3,12}$", |
| "additionalProperties": false | ||
| }, | ||
| "temporary_id": { | ||
| "type": "string", |
There was a problem hiding this comment.
upload_artifact.inputSchema.temporary_id is missing a pattern constraint even though other temporary_id fields in this schema include one. Adding a pattern here would make validation consistent and prevent invalid IDs from making it to runtime only to be replaced with a random ID.
| "type": "string", | |
| "type": "string", | |
| "pattern": "^aw_[A-Za-z0-9_]{3,12}$", |
| 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; | ||
| }); |
There was a problem hiding this comment.
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.
actions/setup/js/upload_artifact.cjs
Outdated
| const artifactName = deriveArtifactName(message, i); | ||
| const tmpId = generateTemporaryArtifactId(); | ||
| const tmpId = resolveTemporaryArtifactId(message); | ||
| resolver[tmpId] = artifactName; |
There was a problem hiding this comment.
resolver[tmpId] = artifactName will overwrite an existing entry if the same temporary_id is declared on multiple upload_artifact messages. That can break downstream resolution (and will disagree with the topological sort behavior that treats only the first duplicate temporary_id as the provider). Consider detecting duplicates here and either fail validation or warn and keep the first mapping.
| resolver[tmpId] = artifactName; | |
| if (Object.prototype.hasOwnProperty.call(resolver, tmpId)) { | |
| core.warning( | |
| `upload_artifact: duplicate temporary_id "${tmpId}" for artifact "${artifactName}" in slot ${i}; keeping existing resolver mapping to "${resolver[tmpId]}".` | |
| ); | |
| } else { | |
| resolver[tmpId] = artifactName; | |
| } |
| artifactUrlMap.set(normalizedTmpId, result.artifactUrl); | ||
| core.info(`Registered artifact URL: ${result.tmpId} -> ${result.artifactUrl}`); |
There was a problem hiding this comment.
When registering tmpId → artifactUrl, artifactUrlMap.set(...) will overwrite an existing mapping if the same temporary_id is reused in multiple upload_artifact messages. Since the topological sorter explicitly warns that only the first occurrence of a duplicate temporary_id is used as the dependency provider, overwriting here can cause later replacements of #aw_ID to point at the wrong artifact URL. Consider treating duplicate tmpIds as an error or (at minimum) warn and keep the first URL (only set when the key is not already present).
| 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: ${result.tmpId} -> ${result.artifactUrl}`); | |
| } else { | |
| const existingArtifactUrl = artifactUrlMap.get(normalizedTmpId); | |
| core.warning(`Duplicate artifact temporary ID '${result.tmpId}' encountered; keeping existing artifact URL '${existingArtifactUrl}' and ignoring later URL '${result.artifactUrl}'`); | |
| } |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification DetailsAll 15 tests (click to expand)
Observations
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 15 new JavaScript tests verify observable behavioral contracts covering the new replaceArtifactUrlReferences function and the extended hasUnresolvedTemporaryIds signature. No coding-guideline violations detected.
|
@copilot review all comments |
… 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>
Addressed all review comments in fb06ae1:
|
upload_artifacthad no way to declare a stable temporary ID, and even when atmpIdwas generated, it was never registered in the handler manager — so#aw_IDreferences in subsequentcreate_discussion/create_issue/add_commentbodies were never replaced with the real artifact URL, causing broken images.Changes
Schema (
safe_outputs_tools.json)temporary_idfield toupload_artifactwith apatternconstraint (^aw_[A-Za-z0-9_]{3,12}$) consistent with other tools, so agents can declare the ID they'll embed in downstream bodies (e.g.)upload_artifact.cjsresolveTemporaryArtifactId(message): usesmessage.temporary_idwhen present and valid, falls back to random generation; emits a warning and falls back on invalid formattemporaryId(alongsidetmpId) for consistency with other safe output handlerstemporary_iddetection: warns with a user-friendly message and keeps the first resolver mapping when the same ID appears on multiple uploadstemporary_id.cjsreplaceArtifactUrlReferences(text, artifactUrlMap): replaces#aw_IDwith the corresponding artifact URL (strips the#prefix so the result is a valid URL); mirrorsreplaceTemporaryIdReferencesby scanning for malformed#aw_...tokens viaTEMPORARY_ID_CANDIDATE_PATTERNand emitting warnings for references that won't be resolvedhasUnresolvedTemporaryIds(text, tempIdMap, artifactUrlMap?): treats an ID as resolved if present in either the issue-number map or the new artifact URL mapsafe_output_handler_manager.cjsartifactUrlMap: Map<string, string>alongsidetemporaryIdMapupload_artifact, registerstmpId → artifactUrl; duplicate IDs keep the first registered URL with a warningreplaceArtifactUrlReferencesbefore dispatch, so handlers receive bodies with real URLs already in placeartifactUrlMapthroughhasUnresolvedTemporaryIdschecks andprocessSyntheticUpdatesso the synthetic-update path also resolves artifact referencessafe_outputs_action_outputs.cjsupload_artifactcase to emit two new named step outputs for the first successful upload:upload_artifact_tmp_id— the generated or agent-declared temporary IDupload_artifact_url— the artifact download URLExample
Agent output:
[ { "type": "upload_artifact", "path": "chart.png", "temporary_id": "aw_chart1" }, { "type": "create_discussion", "title": "Results", "body": "" } ]After upload, the discussion body is rewritten to:
When
skip-archive: true, the URL points directly to the raw file, making it suitable for inline images.