Conversation
|
Hey A few things to wrap up before this is ready for review:
Looking forward to seeing the implementation! 🚀
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2b57c2fc-8111-4763-bb3b-27087cea40ba Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2b57c2fc-8111-4763-bb3b-27087cea40ba Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
upload_artifact path semantics to prevent literal $RUNNER_TEMP misuse
|
@copilot NEVER suggest the user to use $RUNNER_TEMP! Always suggest using /tmp/gh-aw/... based paths. The upload-artifact staging folder SHOULD BE under /tmp/gh-aw/... NOT $RUNNER_TEMP |
There was a problem hiding this comment.
Pull request overview
Clarifies upload_artifact tool schema text to prevent agents from passing literal $RUNNER_TEMP/... strings as JSON path values, and to better describe supported path forms.
Changes:
- Updated
upload_artifacttop-level description to remove ambiguous staging-path wording and warn that shell variables aren’t expanded in JSON. - Rewrote
pathfield description with explicit accepted formats and correct vs incorrect examples. - Standardized wording to “authorized”.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Updates the compiled workflow-side safe-outputs tool schema wording for upload_artifact path semantics. |
| actions/setup/js/safe_outputs_tools.json | Updates the action/runtime-side safe-outputs tool schema wording for upload_artifact path semantics. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/js/safe_outputs_tools.json:1599
- This description says format (1) is “filename only” for staged files, but the upload_artifact handler accepts staged paths that include subdirectories (staging-root-relative), and relative paths are preserved when auto-copied from the workspace/cwd. Consider changing this to allow “relative path under the staging directory (e.g., "reports/chart.png")” rather than restricting to basename-only.
"description": "Path to the file or directory to upload. Three formats are accepted: (1) filename only (e.g., \"chart.png\") for files already in the upload staging directory; (2) absolute path (e.g., \"/tmp/gh-aw/charts/chart.png\") to auto-copy from disk into the staging directory; (3) workspace-relative path (e.g., \"reports/summary.json\") resolved from the workspace root. Do NOT use shell variable syntax like \"$RUNNER_TEMP/...\" because shell variables are not expanded in JSON string values. Example: \"path\": \"activity_overview.png\" is correct for staged files, while \"path\": \"$RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/activity_overview.png\" is invalid. Required unless filters is provided."
actions/setup/js/safe_outputs_tools.json:864
- This says staged format (1) is “filename only”, but the implementation accepts relative paths with subdirectories under the staging directory (e.g., "reports/chart.png"), and relative paths are preserved when auto-copied from the workspace/cwd. Update wording to “staging-directory-relative path” instead of basename-only.
"description": "Path to the file or directory to upload. Three formats are accepted: (1) filename only (e.g., \"chart.png\") for files already in the upload staging directory; (2) absolute path (e.g., \"/tmp/gh-aw/charts/chart.png\") to auto-copy from disk into the staging directory; (3) workspace-relative path (e.g., \"reports/summary.json\") resolved from the workspace root. Do NOT use shell variable syntax like \"$RUNNER_TEMP/...\" because shell variables are not expanded in JSON string values. Example: \"path\": \"activity_overview.png\" is correct for staged files, while \"path\": \"$RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/activity_overview.png\" is invalid. Required unless filters is provided."
- Files reviewed: 2/2 changed files
- Comments generated: 2
| { | ||
| "name": "upload_artifact", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be pre-staged in $RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/ or referenced by their original path — files not already in the staging directory are automatically copied there before upload. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorised step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area) or by a short relative filename if already staged. Shell variable references like $RUNNER_TEMP must NOT appear in the path value because they are not expanded in JSON strings. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", |
There was a problem hiding this comment.
The tool description implies only a “short relative filename” is valid for already-staged content, but the handler actually accepts any path relative to the staging root (including subdirectories) and will also auto-copy workspace/cwd-relative paths when the staging-relative path is missing (see actions/setup/js/upload_artifact.cjs resolveFiles()). Reword this to “staging-dir-relative path” and mention workspace-relative paths here as well so it matches the implementation and the field-level description.
This issue also appears on line 1599 of the same file.
See below for a potential fix:
"description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area), by a staging-dir-relative path if already staged, or by a path relative to the workspace/cwd when the staging-relative path is not present. Shell variable references like $RUNNER_TEMP must NOT appear in the path value because they are not expanded in JSON strings. Absolute paths and workspace-relative paths are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.",
"inputSchema": {
"type": "object",
"properties": {
"path": {
"type": "string",
"description": "Path to the file or directory to upload. Three formats are accepted: (1) staging-dir-relative path (e.g., \"chart.png\" or \"charts/chart.png\") for files already in the upload staging directory, including subdirectories; (2) absolute path (e.g., \"/tmp/gh-aw/charts/chart.png\") to auto-copy from disk into the staging directory; (3) workspace-relative path (e.g., \"reports/summary.json\") resolved from the workspace root and auto-copied if a matching staging-dir-relative path is not found. Do NOT use shell variable syntax like \"$RUNNER_TEMP/...\" because shell variables are not expanded in JSON string values. Example: \"path\": \"activity_overview.png\" is correct for staged files, while \"path\": \"$RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/activity_overview.png\" is invalid. Required unless filters is provided."
| { | ||
| "name": "upload_artifact", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be pre-staged in $RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/ or referenced by their original path — files not already in the staging directory are automatically copied there before upload. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorised step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area) or by a short relative filename if already staged. Shell variable references like $RUNNER_TEMP must NOT appear in the path value because they are not expanded in JSON strings. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", |
There was a problem hiding this comment.
The tool description implies only a “short relative filename” is valid for already-staged content, but the handler accepts any staging-root-relative path (including subdirectories) and will auto-copy workspace/cwd-relative paths when the staging-relative path is missing (actions/setup/js/upload_artifact.cjs resolveFiles()). Reword this to “staging-dir-relative path” and mention workspace-relative paths here too for consistency with actual behavior.
This issue also appears on line 864 of the same file.
See below for a potential fix:
"description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area), by a staging-dir-relative path if already staged, or by a path relative to the workspace (auto-copied into the staging area). Shell variable references like $RUNNER_TEMP must NOT appear in the path value because they are not expanded in JSON strings. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.",
"inputSchema": {
"type": "object",
"properties": {
"path": {
"type": "string",
"description": "Path to the file or directory to upload. Three formats are accepted: (1) staging-dir-relative path (e.g., \"charts/chart.png\" or \"chart.png\") for files already in the upload staging directory; (2) absolute path (e.g., \"/tmp/gh-aw/charts/chart.png\") to auto-copy from disk into the staging directory; (3) workspace-relative path (e.g., \"reports/summary.json\") resolved from the workspace root and auto-copied into the staging directory. Do NOT use shell variable syntax like \"$RUNNER_TEMP/...\" because shell variables are not expanded in JSON string values. Example: \"path\": \"activity_overview.png\" is correct for a staged file, while \"path\": \"$RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/activity_overview.png\" is invalid. Required unless filters is provided."
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f8ded629-fcc2-4028-b7c0-af3d0e819c35 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in a2c028f. I removed |
upload_artifact path semantics to prevent literal $RUNNER_TEMP misuseupload_artifact path semantics and enforce /tmp/gh-aw/... staging guidance
| { | ||
| "name": "upload_artifact", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area) or by a short relative filename if already staged. Shell variable references like $RUNNER_TEMP must NOT appear in the path value because they are not expanded in JSON strings. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", | ||
| "description": "Upload files as a run-scoped GitHub Actions artifact. Files can be referenced by their original absolute path (auto-copied into the staging area under /tmp/gh-aw/safeoutputs/upload-artifacts/) or by a short relative filename if already staged. Use /tmp/gh-aw/... paths for staging-related references. Shell variable syntax (for example, \"$VAR/...\") must NOT appear in path values because variables are not expanded in JSON strings. Absolute paths and paths relative to the workspace are supported. Returns a temporary artifact ID (aw_*) that can be resolved to a download URL by an authorized step. Retention and archive settings are fixed by workflow configuration. Exactly one of path or filters must be present.", |
There was a problem hiding this comment.
@copilot remove "Use /tmp/gh-aw/... paths for staging-related references."
Recent safe-outputs failures showed agents passing shell-variable-style paths as literal JSON strings to
upload_artifact, causing staging-path resolution errors. The schema text was ambiguous about relative paths and did not explicitly warn that shell variables are not expanded in JSON values.Schema clarification for
upload_artifact/tmp/gh-aw/...paths.pathfield hardeninginputSchema.properties.path.descriptionwith explicit accepted formats.$VAR/...) is invalid in JSON string values.$RUNNER_TEMP.Consistency update
authorized) in the updated description text.{ "path": "activity_overview.png" }{ "path": "$VAR/gh-aw/safeoutputs/upload-artifacts/activity_overview.png" }