Fix artifact download: handle .pkg extension in artifact names and add shellcheck infrastructure#11
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
The google/mozc workflow now uploads artifacts with names that already include the .pkg extension (e.g., Mozc_arm64.pkg). The download script was adding another .pkg extension, resulting in files like Mozc_arm64.pkg.pkg. This fix: - Checks if artifact directory name already ends with .pkg - Moves the file appropriately to avoid double extension - Maintains backward compatibility with legacy artifact names - Uses temporary file to avoid "source and destination same" error Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
- Add verification of expected artifact files after download - Improve error messages to help diagnose artifact structure changes - Fix shellcheck warnings (SC2001, SC2035) - Use bash parameter expansion instead of sed - Add -- to ls command to handle filenames with dashes Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
- Clarify why temporary file is needed for mv operation - Add quotes to array length expansion for consistency - Improve comments explaining the directory name conflict Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the artifact download workflow that was failing when google/mozc changed their artifact naming convention to include .pkg extensions. The script now handles both the new naming pattern (artifacts with .pkg already included) and maintains backward compatibility with the legacy pattern.
Key changes:
- Added conditional logic to detect if artifact names already contain
.pkgextension - Implemented temporary file approach to avoid name conflicts during artifact processing
- Enhanced validation with specific file existence checks and detailed warnings
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "" | ||
| echo "Warning: Some expected artifacts are missing:" | ||
| for missing_file in "${missing_files[@]}"; do | ||
| echo " - $missing_file" |
There was a problem hiding this comment.
The loop variable should be quoted to prevent word splitting. Change for missing_file in \"${missing_files[@]}\" to properly quote the array expansion.
scripts/download-mozc-artifacts.sh
Outdated
| done | ||
| echo "" | ||
| echo "Available artifacts:" | ||
| ls -lh -- *.pkg 2>/dev/null || echo " No .pkg files found!" |
There was a problem hiding this comment.
The glob pattern *.pkg should be quoted to prevent it from being expanded by the shell before being passed to ls. Use ls -lh -- '*.pkg' or use a safer alternative like find . -maxdepth 1 -name '*.pkg' -ls.
| ls -lh -- *.pkg 2>/dev/null || echo " No .pkg files found!" | |
| ls -lh -- '*.pkg' 2>/dev/null || echo " No .pkg files found!" |
There was a problem hiding this comment.
Fixed in 5a5e5b6. Used compgen -G to safely test if glob pattern matches any files before running ls, which prevents expansion issues and handles both cases (files exist/don't exist) correctly.
|
@copilot apply changes based on the comments in this thread |
- Use compgen -G to test if *.pkg files exist before ls - Prevents glob expansion issues when no files match - Addresses code review feedback on line 453 Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
Applied the suggested changes in commit 5a5e5b6. The glob pattern is now safely checked using |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/download-mozc-artifacts.sh
Outdated
| echo "" | ||
| echo "Available artifacts:" | ||
| if compgen -G "*.pkg" > /dev/null; then | ||
| ls -lh -- *.pkg |
There was a problem hiding this comment.
The compgen -G check verifies the glob pattern matches files, but ls on line 454 is still using an unquoted glob pattern. If the glob expands to nothing (which shouldn't happen after the check, but is theoretically possible due to race conditions or command execution timing), ls would interpret *.pkg as a literal filename. Consider using ls -lh -- *.pkg only within the success block where the glob is guaranteed to expand, or use a more explicit iteration pattern like for f in *.pkg; do ls -lh -- "$f"; done.
| ls -lh -- *.pkg | |
| for f in *.pkg; do ls -lh -- "$f"; done |
There was a problem hiding this comment.
Fixed in db5e363. Changed to use explicit loop for f in *.pkg; do ls -lh -- "$f"; done to safely handle glob expansion within the success block.
scripts/download-mozc-artifacts.sh
Outdated
| mv "$artifact_dir/Mozc.pkg" "${artifact_name}.tmp" | ||
| rmdir "$artifact_dir" | ||
| mv "${artifact_name}.tmp" "$artifact_name" |
There was a problem hiding this comment.
This three-step process using a temporary file could fail if the .tmp file already exists from a previous interrupted run. Consider checking for or removing any existing .tmp files before the move, or using a more unique temporary name like ${artifact_name}.tmp.$$ (with process ID) to avoid conflicts.
| mv "$artifact_dir/Mozc.pkg" "${artifact_name}.tmp" | |
| rmdir "$artifact_dir" | |
| mv "${artifact_name}.tmp" "$artifact_name" | |
| tmpfile="${artifact_name}.tmp.$$" | |
| rm -f "$tmpfile" | |
| mv "$artifact_dir/Mozc.pkg" "$tmpfile" | |
| rmdir "$artifact_dir" | |
| mv "$tmpfile" "$artifact_name" |
There was a problem hiding this comment.
Fixed in db5e363. Now using process ID in temporary filename (${artifact_name}.tmp.$$) and adding rm -f "$tmpfile" before the move operation to prevent conflicts from interrupted runs.
|
@copilot apply changes based on the comments in this thread |
- Use process ID ($$) in temporary filename to avoid conflicts from interrupted runs - Add rm -f to clean any existing tmp file before use - Use explicit loop for ls to avoid unquoted glob expansion issues - Addresses review comments on lines 401-403 and 454 Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
Applied both suggestions in commit db5e363. The temporary file now uses process ID for uniqueness, and the ls command uses an explicit loop for safer glob handling. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/download-mozc-artifacts.sh
Outdated
| echo "" | ||
| echo "Available artifacts:" | ||
| if compgen -G "*.pkg" > /dev/null; then | ||
| for f in *.pkg; do ls -lh -- "$f"; done |
There was a problem hiding this comment.
Using ls in a script for programmatic purposes is generally discouraged. Consider using a direct file listing approach instead: printf '%s\n' *.pkg | xargs -I {} ls -lh -- {} or simply iterate and use stat or file test operators to display information.
| for f in *.pkg; do ls -lh -- "$f"; done | |
| for f in *.pkg; do stat -f " %N: %z bytes, modified %Sm" "$f"; done |
There was a problem hiding this comment.
Fixed in 799fc22. Replaced ls with a portable approach using printf and wc -c to display file information. This avoids platform-specific stat syntax differences and is more suitable for scripting.
|
@copilot apply changes based on the comments in this thread それから、実装に対応するようなbatsのテストコードをつくれるならつくってほしいです |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/download-mozc-artifacts.bats
Outdated
| artifact_name="${artifact_dir%/}" | ||
| if [ -f "$artifact_dir/Mozc.pkg" ]; then | ||
| if [[ "$artifact_name" == *.pkg ]]; then | ||
| tmpfile=$(mktemp -u "${artifact_name}.tmp.XXXXXX") |
There was a problem hiding this comment.
Using mktemp -u creates a race condition vulnerability. The -u flag generates a filename without creating the file, leaving a window where another process could create a file with the same name. Remove the -u flag to have mktemp create the file atomically, ensuring safe temporary file handling.
| tmpfile=$(mktemp -u "${artifact_name}.tmp.XXXXXX") | |
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") |
There was a problem hiding this comment.
Fixed in 25ec8b7. Removed the -u flag from mktemp to eliminate the race condition vulnerability. Now mktemp creates the file atomically, we remove the empty file, then move the source to that guaranteed-unique name. This prevents other processes from creating files with the same name between generation and usage.
|
@copilot apply changes based on the comments in this thread |
- Remove -u flag from mktemp to avoid race condition vulnerability - mktemp now creates the file atomically, then we remove it before use - This ensures no other process can create a file with the same name - Maintains the same functionality with improved security - Addresses security review comment about race condition Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
Applied in commit 25ec8b7. The mktemp race condition has been fixed by removing the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/download-mozc-artifacts.bats
Outdated
| if [ -f "$artifact_dir/Mozc.pkg" ]; then | ||
| if [[ "$artifact_name" == *.pkg ]]; then | ||
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") | ||
| rm "$tmpfile" # Remove the empty file created by mktemp |
There was a problem hiding this comment.
Removing the file created by mktemp introduces a race condition vulnerability. Between the rm and mv operations, another process could create a file with the same name. Keep the file created by mktemp and write directly to it, or use mktemp -u with proper error handling if an empty placeholder is truly needed.
| rm "$tmpfile" # Remove the empty file created by mktemp |
There was a problem hiding this comment.
Fixed in 67545c0. Changed to use cat to copy content to the mktemp-created file instead of removing it. The file created by mktemp remains throughout the process, eliminating the race condition.
scripts/download-mozc-artifacts.sh
Outdated
| # 4. Rename temp file to final name | ||
| # Use mktemp to create a unique temporary file safely (without -u to avoid race condition) | ||
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") | ||
| rm "$tmpfile" # Remove the empty file created by mktemp |
There was a problem hiding this comment.
Removing the file created by mktemp introduces a race condition vulnerability. Between the rm and mv operations, another process could create a file with the same name. Keep the file created by mktemp and write directly to it, or use mktemp -u with proper error handling if an empty placeholder is truly needed.
| rm "$tmpfile" # Remove the empty file created by mktemp |
There was a problem hiding this comment.
Fixed in 67545c0. Now using cat "$artifact_dir/Mozc.pkg" > "$tmpfile" to copy content to the mktemp-created file, keeping it atomically created throughout the entire process.
tests/download-mozc-artifacts.bats
Outdated
| # Verify the final file exists | ||
| [ -f "Mozc_arm64.pkg" ] | ||
| # Verify no temp files with mktemp pattern remain (should be cleaned up) | ||
| ! ls Mozc_arm64.pkg.tmp.* 2>/dev/null | grep -v "abc123" |
There was a problem hiding this comment.
This assertion uses ls in a pipe which can be fragile. Consider using compgen -G or a for loop with file existence checks for more robust file matching, similar to the pattern used elsewhere in the test suite.
| ! ls Mozc_arm64.pkg.tmp.* 2>/dev/null | grep -v "abc123" | |
| ! compgen -G 'Mozc_arm64.pkg.tmp.*' | grep -v "abc123" |
There was a problem hiding this comment.
Fixed in 67545c0. Replaced ls pipe with compgen -G for more robust file pattern matching: ! compgen -G "Mozc_arm64.pkg.tmp.*" | grep -v "abc123" > /dev/null
tests/README.md
Outdated
| - **Artifact processing with .pkg extension**: Validates that artifact directories named `Mozc_arm64.pkg/` are correctly processed to create `Mozc_arm64.pkg` files | ||
| - **Legacy artifact processing**: Tests backward compatibility with artifact directories without `.pkg` extension (e.g., `Mozc_arm64/`) | ||
| - **Multiple artifacts**: Verifies that multiple artifacts (arm64, intel64, universal_binary) are processed correctly | ||
| - **Temporary file handling**: Tests that temporary files use process ID for uniqueness and don't conflict |
There was a problem hiding this comment.
The documentation states temporary files use 'process ID for uniqueness', but the implementation actually uses mktemp's XXXXXX random suffix pattern. Update this description to accurately reflect the mktemp-based approach.
| - **Temporary file handling**: Tests that temporary files use process ID for uniqueness and don't conflict | |
| - **Temporary file handling**: Tests that temporary files use mktemp's XXXXXX random suffix pattern for uniqueness and don't conflict |
There was a problem hiding this comment.
Fixed in 67545c0. Updated documentation to accurately reflect mktemp's XXXXXX random suffix pattern instead of process ID.
|
@copilot apply changes based on the comments in this thread |
- Use cat to copy content to mktemp-created file instead of removing it - Eliminates race condition between rm and mv operations - mktemp file remains atomically created throughout the process - Update test assertion to use compgen instead of ls for pattern matching - Update documentation to reflect mktemp XXXXXX pattern instead of process ID - Addresses all review comments about race conditions and test robustness Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
Applied all suggestions in commit 67545c0. Fixed race condition by using cat instead of removing mktemp file, updated test assertions to use compgen, and corrected documentation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/download-mozc-artifacts.sh
Outdated
| # Artifact name already has .pkg extension (e.g., Mozc_arm64.pkg) | ||
| # Since the directory name matches the target filename, we need a temporary rename: | ||
| # Use mktemp to create a unique temporary file atomically for safe handling | ||
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") |
There was a problem hiding this comment.
The mktemp template pattern should use a literal string, not a variable interpolation. The current pattern \"${artifact_name}.tmp.XXXXXX\" may not work as expected because mktemp requires the XXXXXX pattern to be at the end of a fixed template. Use mktemp -t \"${artifact_name}.tmp.XXXXXX\" or ensure the pattern follows mktemp's template requirements.
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") | |
| tmpfile=$(mktemp -t "${artifact_name}.tmp.XXXXXX") |
There was a problem hiding this comment.
Fixed in 10b0c09. Simplified to use mktemp without a template, which creates a unique temp file in the default temp directory. This avoids template pattern issues and is more straightforward.
tests/download-mozc-artifacts.bats
Outdated
| [ -f "Mozc_arm64.pkg" ] | ||
| # Verify no temp files with mktemp pattern remain (should be cleaned up) | ||
| # Use compgen for robust pattern matching | ||
| ! compgen -G "Mozc_arm64.pkg.tmp.*" | grep -v "abc123" > /dev/null |
There was a problem hiding this comment.
This assertion logic is convoluted and may not reliably test what's intended. The negated command with a pipe through grep makes it unclear what condition is being tested. Consider restructuring to explicitly check that new temp files were cleaned up: [ ! -f \"Mozc_arm64.pkg.tmp.\"* ] || [ $(compgen -G \"Mozc_arm64.pkg.tmp.*\" | grep -c -v \"abc123\") -eq 0 ]
| ! compgen -G "Mozc_arm64.pkg.tmp.*" | grep -v "abc123" > /dev/null | |
| [ "$(compgen -G "Mozc_arm64.pkg.tmp.*" | grep -v "abc123" | wc -l)" -eq 0 ] |
There was a problem hiding this comment.
Fixed in 10b0c09. Simplified the test to focus on functionality - verifies the final file exists with correct content and the directory was removed, rather than checking implementation details of temp file patterns.
scripts/download-mozc-artifacts.sh
Outdated
| # Use mktemp to create a unique temporary file atomically for safe handling | ||
| tmpfile=$(mktemp "${artifact_name}.tmp.XXXXXX") | ||
| # Copy content to the temporary file, then remove source and directory | ||
| cat "$artifact_dir/Mozc.pkg" > "$tmpfile" |
There was a problem hiding this comment.
Using cat with output redirection is less efficient than using cp for copying file contents. Consider replacing with cp \"$artifact_dir/Mozc.pkg\" \"$tmpfile\" for better performance and clearer intent.
| cat "$artifact_dir/Mozc.pkg" > "$tmpfile" | |
| cp "$artifact_dir/Mozc.pkg" "$tmpfile" |
There was a problem hiding this comment.
Fixed in 10b0c09. Replaced cat with cp for better performance and clearer intent when copying file contents.
|
yellow-seed/dotfiles#43 を参考に、shellcheck の機構と、それを利用するGitHub Actionsを構築してほしい。 Shell に対するLintがないとレビューの指摘点が安定しない。 |
Code improvements: - Replace cat with cp for better performance in artifact processing - Use mktemp without template for simpler temp file creation - Simplify test assertions for mktemp cleanup verification - Update test to focus on functionality rather than implementation Shellcheck infrastructure: - Add .shellcheckrc configuration file for consistent linting - Create dedicated shellcheck.yml GitHub Actions workflow - Update test-scripts.yml to use shellcheck GitHub Action - Separate shellcheck and test jobs with dependency - Document linting setup in tests/README.md Addresses review comments and user request for stable shellcheck configuration Co-authored-by: yellow-seed <42381113+yellow-seed@users.noreply.github.com>
Created comprehensive shellcheck infrastructure in commit 10b0c09: Configuration:
GitHub Actions:
Documentation:
This ensures stable, consistent shellcheck behavior and prevents review comment variations. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
.pkgsuffix in their names.pkgextensionSummary
Fixed the "Mozc_arm64.pkg not found" error in the Download Mozc Artifacts workflow by properly handling artifact names that already include the
.pkgextension. Additionally, established comprehensive shellcheck infrastructure to ensure consistent code quality and stable linting behavior.Root Cause
The google/mozc repository's macOS workflow uploads artifacts with names like
Mozc_arm64.pkg,Mozc_intel64.pkg, etc. When downloaded viagh run download, these create directories with the.pkgsuffix already included. The previous code was unconditionally adding another.pkgextension, resulting in files likeMozc_arm64.pkg.pkg.Changes Made
Main Fix
.pkg.pkg)Improvements
sedcompgen -Gto safely check for glob pattern matcheslsfor script-friendly operationsShellcheck Infrastructure
.shellcheckrc: Configuration file for consistent linting rules across environments.github/workflows/shellcheck.yml: Dedicated workflow usingludeeus/action-shellcheckfor automated linting.github/workflows/test-scripts.yml: Separated shellcheck and test jobs with dependency chaintests/README.mdwith local and CI usage examplesTesting & CI
bash -ntests/README.mdwith documentation and CI integration examples.github/workflows/test-scripts.ymlfor automated testing:.github/workflows/shellcheck.ymlfor dedicated linting workflowImpact
Original prompt
This section details on the original issue you should resolve
<issue_title>Download Mozc Artifacts workflow fails: Mozc_arm64.pkg not found</issue_title>
<issue_description>## Problem Description
The "Download Mozc Artifacts from google/mozc" GitHub Actions workflow is failing with the following error:
Workflow run: https://github.com/yellow-seed/brew_mozc/actions/runs/19810535498/job/56751996811
Root Cause Analysis
The error occurs in the
create-update-pr.shscript when it tries to calculate the SHA256 checksum forMozc_arm64.pkg. This indicates that the precedingdownload-mozc-artifacts.shscript failed to download or properly save the artifact.Possible Causes
Workflow file not found: The script looks for
macos.yamlworkflow in google/mozc repositoryWORKFLOW_FILE="macos.yaml"Artifact naming pattern mismatch: The script downloads artifacts matching pattern
Mozc_*Mozc_arm64/containingMozc.pkgArtifact expiration: GitHub Actions artifacts typically expire after 90 days
validate_artifactsstepNo successful workflow runs: There might be no successful runs of the macOS workflow
--status success --limit 1RUN_IDwould be emptyImpact
Proposed Solution
Investigation Steps
Verify google/mozc workflow status:
macos.yamlexists in google/mozc repositoryInspect artifact structure:
Add better error handling:
download-mozc-artifacts.shto check if artifacts were successfully extractedRecommended Fixes
Option 1: Update workflow file name detection
Option 2: Improve artifact validation
Option 3: Add workflow dependencies
Additional Context
scripts/create-update-pr.shdownload-mozc-artifacts.shscript includes validation functions (validate_artifacts,validate_pkg_files) but they might not be catching all edge cases0 2 * * 1)Tasks
download-mozc-artifacts.shwith correct patternsworkflow_dispatch💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.