Fix verbose log artifact name conflicts on multiple action invocations#69
Fix verbose log artifact name conflicts on multiple action invocations#69
Conversation
Co-authored-by: derekmisler <2684662+derekmisler@users.noreply.github.com>
| fi | ||
| echo "verbose-log-file=$VERBOSE_LOG_FILE" >> $GITHUB_OUTPUT | ||
| echo "Verbose log file: $VERBOSE_LOG_FILE" | ||
| VERBOSE_LOG_BASENAME=$(basename "$VERBOSE_LOG_FILE") |
There was a problem hiding this comment.
Are you absolutely positive this will not conflict if run twice in a row?
There was a problem hiding this comment.
Yes — mktemp guarantees a unique filename on every call (that's its core contract). Each invocation of the action creates its own distinct temp file (e.g., cagent-verbose-liMsf4, cagent-verbose-wbVWRb), and the artifact name includes that unique basename, so two back-to-back invocations will always produce different artifact names.
| echo "verbose-log-file=$VERBOSE_LOG_FILE" >> $GITHUB_OUTPUT | ||
| echo "Verbose log file: $VERBOSE_LOG_FILE" | ||
| VERBOSE_LOG_BASENAME=$(basename "$VERBOSE_LOG_FILE") | ||
| echo "verbose-log-artifact-name=cagent-verbose-log-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}-${GITHUB_JOB}-${VERBOSE_LOG_BASENAME}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
I don't see how this is different from the current artifact path name
There was a problem hiding this comment.
The difference is the -${VERBOSE_LOG_BASENAME} suffix at the end. VERBOSE_LOG_FILE is created via mktemp /tmp/cagent-verbose-XXXXXX (line 472), which replaces XXXXXX with cryptographically random characters on every call — producing a unique name like cagent-verbose-ab3xYZ. So the artifact name becomes cagent-verbose-log-{run_id}-{run_attempt}-{job}-cagent-verbose-ab3xYZ, which is distinct for every invocation.
derekmisler
left a comment
There was a problem hiding this comment.
Please double check that the path will be unique
There was a problem hiding this comment.
Pull request overview
Fixes artifact upload conflicts when this composite action is invoked multiple times within the same GitHub Actions job by ensuring each verbose log artifact name is unique per invocation.
Changes:
- Derive a unique suffix from the
mktempverbose log filename and emit it as a new step output (verbose-log-artifact-name). - Update the
actions/upload-artifactstep to use the new computed artifact name output instead of a fixed name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Confirmed unique. |
|
/review |
There was a problem hiding this comment.
Review Summary
Assessment: 🟢 APPROVE
This PR successfully fixes the artifact name collision issue when the action is invoked multiple times in the same job. The implementation is sound and the code is properly quoted.
Found 1 minor edge case to be aware of:
- Artifact naming constraints are not explicitly validated (low severity)
The fix correctly appends the mktemp-generated basename to ensure uniqueness. No blocking issues found.
| echo "verbose-log-file=$VERBOSE_LOG_FILE" >> $GITHUB_OUTPUT | ||
| echo "Verbose log file: $VERBOSE_LOG_FILE" | ||
| VERBOSE_LOG_BASENAME=$(basename "$VERBOSE_LOG_FILE") | ||
| echo "verbose-log-artifact-name=cagent-verbose-log-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}-${GITHUB_JOB}-${VERBOSE_LOG_BASENAME}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
MINOR EDGE CASE
The artifact name construction doesn't validate against GitHub Actions artifact naming requirements (max 256 characters, forbidden characters: \, /, ", :, <, >, |, *, ?).
While mktemp typically generates safe alphanumeric characters making this extremely unlikely in practice, there's no explicit validation that the final artifact name conforms to these constraints.
Impact: Very low - mktemp uses a restricted character set by design.
Related Issues
Summary
When the action is invoked more than once in the same job, all invocations produce identical artifact names (
cagent-verbose-log-{run_id}-{run_attempt}-{job}), causing upload conflicts.Fix: append the unique
mktemp-generated log file basename to the artifact name, guaranteeing uniqueness per invocation without any external state.mktemp /tmp/cagent-verbose-XXXXXXreplacesXXXXXXwith cryptographically random characters on every call (e.g.,cagent-verbose-ab3xYZ), so two back-to-back invocations in the same job always produce distinct artifact names such ascagent-verbose-log-{run_id}-{run_attempt}-{job}-cagent-verbose-ab3xYZandcagent-verbose-log-{run_id}-{run_attempt}-{job}-cagent-verbose-qR7mNp.action.yml—run-agentstep: extract basename of the temp log file and emit it as a newverbose-log-artifact-nameoutput:action.yml— upload step: consume the new output instead of the hardcoded name:Tip
Comment
/reviewto trigger the PR Reviewer agent for automated feedback.Comment
/describeto generate a PR description.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.