Fix silent failures: dry-run commit, undefined RELEASE_BASE, unchecked download#30
Conversation
- Remove --porcelain from git commit: the flag implies a dry-run, so the 'add LaTeX environment' commit was never actually created - Replace undefined RELEASE_BASE with REPOSITORY_NAME-BRANCH so the extracted directory is cleaned up after integration - Add error handling for curl (-f), unzip, and cd so download or extraction failures stop the script with a clear error message
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -117,14 +127,14 @@ for i in $(find . -type f); do | |||
| done | |||
|
|
|||
| cd .. | |||
There was a problem hiding this comment.
🚨 [HIGH] cd .. はエラーハンドリングがなく、shellcheckでもSC2164として警告される可能性があります。cd .. が失敗した場合、続く rm -rf "${REPOSITORY_NAME}-${BRANCH}" が予期しないディレクトリに対して実行されるリスクがあります。
cd .. || { echo "Error: failed to return to parent directory" >&2; exit 1; }とすることを強く推奨します。
| rm -f "${ZIP_NAME}" | ||
| exit 1 | ||
| fi | ||
| rm "${ZIP_NAME}" |
There was a problem hiding this comment.
rm -f "${ZIP_NAME}" はunzip失敗時のクリーンアップとして追加されていますが、rm "${ZIP_NAME}" （成功時）はエラーハンドリングがありません。一貫性のため、成功時も rm -f "${ZIP_NAME}" とするか、失敗時に exit 1 する前に rm が失敗した場合の処理を考慮してください。
また、cd .. (119行目付近) の後の rm -rf "${REPOSITORY_NAME}-${BRANCH}" も、cd が失敗した場合に意図しないディレクトリを削除するリスクがあります。cd .. にも || { echo ... >&2; exit 1; } を追加することを推奨します。
- Add error handling to 'cd ..' so rm -rf never runs in an unexpected directory if the cd fails (review HIGH) - Use rm -f for the zip on the success path for consistency with the failure-path cleanup (review MEDIUM)
|
|
||
| cd .. | ||
| rm -rf ${RELEASE_BASE} | ||
| cd .. || { |
There was a problem hiding this comment.
cd .. の失敗ハンドリングについて
cd .. が失敗するケースは実質的にほぼ存在しません（親ディレクトリが消えるなど極めて異常な状況）。それよりも問題なのは、cd でサブディレクトリに移動した後にエラーが発生した場合（例：ファイル除外処理中の失敗）、cd .. に到達せずスクリプトが終了し、呼び出し元のカレントディレクトリが変わったまま残る可能性があることです。
より堅牢な設計として、cd でのディレクトリ移動の代わりに絶対パスを使用するか、pushd/popd を使用するか、あるいはサブシェル ( cd "${REPOSITORY_NAME}-${BRANCH}" && ... ) で処理を囲む方法を検討してください。
| if [ "$QUIET" = "false" ]; then | ||
| git add . | ||
| git commit --quiet --porcelain -m "add LaTeX environment" | ||
| git commit --quiet -m "add LaTeX environment" |
There was a problem hiding this comment.
ℹ️ [LOW] ### --porcelain フラグ削除（重大バグ修正）
git commit --porcelain はdry-runとして機能するため、コミットが一度も作成されていなかったという重大なバグの修正です。この修正は正しいです。
ただし、git add 後に変更がない場合（例：LaTeX環境のファイルが既に存在する場合）、git commit は非ゼロの終了コードを返します。QUIET=false の場合はスクリプトがそこで止まる可能性があります（set -e が有効な場合）。git diff --cached --quiet || git commit --quiet -m "add LaTeX environment" のように、コミットが必要な場合のみ実行する形も検討してください。
| fi | ||
| if ! unzip -q "${ZIP_NAME}"; then | ||
| echo "Error: failed to extract ${ZIP_NAME}" >&2 | ||
| rm -f "${ZIP_NAME}" |
There was a problem hiding this comment.
ℹ️ [LOW] ### unzip 失敗時の rm -f について
unzip 失敗時に rm -f "${ZIP_NAME}" でZIPファイルを削除してから exit 1 しています。これは適切なクリーンアップですが、curl 失敗時（ファイルが存在しない、または不完全な場合）には rm -f が実行されません。curl 失敗時にも念のため rm -f "${ZIP_NAME}" を追加することを検討してください（-f フラグがあっても部分的なファイルが残る場合があります）。
|
レビュー指摘 2 件に af6bec7 で対応しました。 対応内容🚨 HIGH:
|
- Remove possibly partial zip when curl fails (review LOW) - Only run git commit when there are staged changes, avoiding a confusing 'nothing to commit' failure (review LOW)
|
再レビューの指摘 3 件に対応しました（eabe81e）。 対応した指摘ℹ️ LOW: curl 失敗時の部分ファイルcurl 失敗時の分岐に ℹ️ LOW: 変更が無い場合の
|
|
|
||
| cd .. | ||
| rm -rf ${RELEASE_BASE} | ||
| cd .. || { |
There was a problem hiding this comment.
cd .. の失敗ハンドリングは実質的に不要
cd .. は通常のファイルシステムでは失敗しません（親ディレクトリは常に存在する）。一方で、もし何らかの理由で失敗した場合、その後の rm -rf "${REPOSITORY_NAME}-${BRANCH}" が誤ったディレクトリで実行される危険性があります。
より安全な実装として、cd に依存せず絶対パスや変数でディレクトリを管理する方法を検討してください。例えば、展開先ディレクトリを変数に保存しておき、処理後に rm -rf "${WORK_DIR}" のように絶対パスで削除する方が堅牢です。
| if [ "$QUIET" = "false" ]; then | ||
| git add . | ||
| git commit --quiet --porcelain -m "add LaTeX environment" | ||
| git diff --cached --quiet || git commit --quiet -m "add LaTeX environment" |
There was a problem hiding this comment.
ℹ️ [LOW] [LOW] git diff --cached --quiet によるステージング確認の意図が不明確
git diff --cached --quiet でステージされた変更がない場合にコミットをスキップする実装になっています。これは「何も変更がなければコミットしない」という意図だと思われますが、git add . の直後にステージされた変更がない状況（＝LaTeX環境のファイルが一切追加・変更されなかった）は、むしろ異常系として扱うべきではないでしょうか。
サイレントにスキップするのではなく、警告メッセージを出力することを検討してください：
if git diff --cached --quiet; then
echo "Warning: no changes to commit after adding LaTeX environment" >&2
else
git commit --quiet -m "add LaTeX environment"
fi
|
eabe81e に対する再レビュー 2 件は、いずれもコード変更なしで見送りとします。
|
resolve #29
修正内容
1.
git commit --porcelainの削除（重大バグ）--porcelainはgit commitでは dry-run を意味するため、「add LaTeX environment」コミットは実際には一度も作成されていなかった（変更はステージされたまま）。フラグを削除し、コミットが実際に作成されるようにした。2. 未定義変数
RELEASE_BASEの修正rm -rf ${RELEASE_BASE}は空文字に展開され、展開済みのlatex-environment-release/ディレクトリが削除されずに残っていた。"${REPOSITORY_NAME}-${BRANCH}"に修正。3. ダウンロード系のエラーハンドリング追加
curlに-fを追加し、curl/unzip/cdの失敗時に stderr へ明確なエラーメッセージを出してexit 1するようにした。従来はダウンロード失敗後も処理が継続し、分かりにくいエラーになっていた。テスト結果
bash -n: 構文 OKcd失敗未処理）が解消、新規警告なし（残存警告は既存・本 PR 対象外）Error: failed to download LaTeX environment from ...を表示し exit 1🤖 Generated with Claude Code