refactor: generalize caller distribution (workflow-agnostic)#42
Conversation
Replace distribute-claude-review.sh with a generic distribute-workflow.sh
that knows nothing about Claude, reviews, secrets, or models. Distribute any
caller defined under scripts/callers/; adding a new shared workflow needs only
a template (and optional PR note), no script changes.
- distribute-workflow.sh: caller-agnostic; --ref + generic --var KEY=VALUE
substitution (__REF__/__KEY__ tokens; GitHub ${{ }} left intact). --model
and --language are thin aliases for --var. PR body is generic; per-caller
details come from scripts/callers/<caller>.pr-note.md. Only prerequisite is
gh write access — no secret assumptions.
- scripts/callers/: caller templates (claude-code-review, claude-mention) and
their .pr-note.md (the ANTHROPIC_API_KEY requirement lives here, not in the
script).
- scripts/README.md: rewritten workflow-agnostic; per-caller requirements in a
table, with a guide for adding new shared workflows.
| # GitHub ${{ ... }} expressions contain no __X__ tokens and are left intact. | ||
| SED_ARGS=(-e "s|__REF__|${REF}|g") | ||
| for kv in "${VARS[@]}"; do | ||
| SED_ARGS+=(-e "s|__${kv%%=*}__|${kv#*=}|g") |
There was a problem hiding this comment.
scripts/distribute-workflow.sh の --var KEY=VALUE オプションで渡される値が sed コマンドで置換される際、VALUE に | や & といった sed の特殊文字が含まれていると、スクリプトがエラーになったり、意図しない置換が発生したりする可能性があります。汎用スクリプトとして、より堅牢な sed エスケープ処理を導入することを検討してください。
| @@ -0,0 +1,3 @@ | |||
| PR への自動レビュー(Claude)を有効化します。 | |||
|
|
|||
| - 動作には org シークレット `ANTHROPIC_API_KEY` がこのリポジトリで利用可能である必要があります(未配布なら安全にスキップ) | |||
There was a problem hiding this comment.
ℹ️ [LOW] 以前の distribute-claude-review.sh のPR本文にあった「draft はスキップ(ready_for_review で起動)」「fork PR では secret が渡らないため安全にスキップ」といった運用上の重要な注意点を追記すると、配布するユーザーにとってより親切になるでしょう。これらの情報は、ワークフローの動作を理解する上で役立ちます。
| # GitHub ${{ ... }} expressions contain no __X__ tokens and are left intact. | ||
| SED_ARGS=(-e "s|__REF__|${REF}|g") | ||
| for kv in "${VARS[@]}"; do | ||
| SED_ARGS+=(-e "s|__${kv%%=*}__|${kv#*=}|g") |
There was a problem hiding this comment.
[warning] --var の VALUE に sed のデリミタ | や特殊文字(&・\)が含まれると、sed 式が壊れるか意図しない置換が起きます。
具体的な挙動:
--var MODEL=foo|bar→s|__MODEL__|foo|bar|gは sed 構文エラー(デリミタが4つになる)--var MODEL=claude-&-model→&が sed の「マッチ文字列」(__MODEL__)に展開されclaude-__MODEL__-modelになる--var MODEL=foo\nbar→ GNU sed では\nが改文字に化けて YAML が壊れる
壊れた YAML がそのまま各リポジトリへ配布される点が問題です。
修正案: printf '%s\n' + |→/ への変換か、awk -v による置換に切り替える。最小コスト案は sed -e "s|...|...|g" を同等な Python ワンライナーや envsubst に置き換えることです。少なくとも | が VALUE に含まれていないかチェックするバリデーションを追加してください。
| model: __MODEL__ | ||
| review_language: __LANGUAGE__ |
There was a problem hiding this comment.
[warning] --model / --language を省略してスクリプトを実行すると、これらのトークンが未置換のまま(リテラルの __MODEL__・__LANGUAGE__)各リポジトリへ配布されます。
旧スクリプトはデフォルトを sonnet / 日本語 にしていましたが、新スクリプトではデフォルト値もバリデーションも除去されています。未置換の値が reusable ワークフローへ渡ると、不明モデル名として API コールが失敗します。
同じ問題は scripts/callers/claude-mention.yml の 32〜33 行目にも存在します。
対処案(いずれか):
- スクリプト側でデプロイ時に未置換トークン(
__[A-Z_]+__)が残っていないか検証してエラーにする - テンプレート自体に YAML デフォルト値を持たせ、トークン方式をやめる(例: reusable 側の
with.modelinput にデフォルト値がある場合はwith:ブロック自体を省略する) - 旧スクリプト同様にスクリプト側でデフォルトを復活させる
| gh api -X POST "repos/${repo}/git/refs" \ | ||
| -f ref="refs/heads/${PR_BRANCH}" -f sha="${head_sha}" >/dev/null |
There was a problem hiding this comment.
[warning] set -euo pipefail 下でこの gh api -X POST(ブランチ作成)が失敗した場合、スクリプト全体が即座に終了します。他のリポジトリの処理が中断されるため、複数リポジトリに配布する際にリスクがあります。
失敗しうるケース:ネットワーク瞬断・GitHub API レート制限・並行実行による競合(TOCTOU: 存在チェックと作成の間に他のプロセスが同名ブランチを作成)。
旧スクリプトからの引き継ぎですが、失敗時は continue で次のリポジトリへ進む方が設計意図(per-repo エラーカウント)に合っています。
| gh api -X POST "repos/${repo}/git/refs" \ | |
| -f ref="refs/heads/${PR_BRANCH}" -f sha="${head_sha}" >/dev/null | |
| if ! gh api -X POST "repos/${repo}/git/refs" \ | |
| -f ref="refs/heads/${PR_BRANCH}" -f sha="${head_sha}" >/dev/null 2>&1; then | |
| echo " ✗ failed to create branch ${PR_BRANCH}"; failed=$((failed+1)); continue | |
| fi |
Address review findings on the generalized distributor: - Escape sed replacement so --var values with | & \ substitute literally (a generic --var may carry anything); validate token keys - Refuse to distribute a template with unsubstituted __TOKEN__ left (e.g. a forgotten --model) — fail once, before touching any repo - Restore per-caller defaults via scripts/callers/<caller>.defaults (CLI --var/--model/--language still override); claude callers default to sonnet / 日本語 - Don't let one repo's gh API failure abort the batch: guard the head-sha read and branch creation and continue to the next repo - pr-note: document draft/fork graceful-skip for the review caller
レビュー対応(commit 943aab4)Claude・Gemini 両レビューの指摘に対応しました。いずれも汎用化に伴う妥当な指摘でした。 [warning] sed 特殊文字で置換が壊れる(Claude・Gemini)
[warning]
|
| Claude Code Review の caller ワークフローを、指定したリポジトリ群へ配布します。 | ||
| caller は共有 reusable | ||
| `smkwlab/.github/.github/workflows/claude-code-review.yml@<ref>` を呼び出します。 | ||
| 共有 caller ワークフローを、指定したリポジトリ群へ配布する**汎用**スクリプト。 |
There was a problem hiding this comment.
✨ [POSITIVE] ワークフロー配布スクリプトの汎用化に伴い、READMEが非常に分かりやすく更新されています。特に、callerテンプレートの表と新しいワークフローを追加する手順は、今後の運用と拡張性を大きく助けるでしょう。
|
|
||
| jobs: | ||
| review: | ||
| uses: smkwlab/.github/.github/workflows/claude-code-review.yml@__REF__ |
There was a problem hiding this comment.
✨ [POSITIVE] Claude Code Reviewワークフローのcallerテンプレートが適切に定義されています。__REF__、__MODEL__、__LANGUAGE__といったプレースホルダーの使用は、スクリプトの汎用性を保ちつつ、caller固有の設定を可能にする良い設計です。
| claude: | ||
| # Run only when @claude is mentioned AND the author has repository access | ||
| # (OWNER/MEMBER/COLLABORATOR), so external users cannot trigger a write run. | ||
| if: > |
There was a problem hiding this comment.
✨ [POSITIVE] Claude Mentionワークフローのcallerテンプレートも適切に定義されています。特に、if条件で@claudeメンションとauthor_associationをチェックしている点は、セキュリティ面で非常に良い実装です。
|
|
||
| # Refuse to distribute a template with any unsubstituted __TOKEN__ (e.g. a | ||
| # forgotten --model). Caught once here, before touching any repository. | ||
| leftover=$(render "$TEMPLATE" | grep -oE '__[A-Za-z0-9_]+__' | sort -u | tr '\n' ' ' || true) |
There was a problem hiding this comment.
✨ [POSITIVE] このスクリプトは、以前の特定ワークフロー向けスクリプトから完全に汎用化されており、設計思想が明確に反映されています。特に、--varオプションによる柔軟な置換、.defaultsファイルからのデフォルト値読み込み、そして未置換トークンのチェック(144行目)は、スクリプトの堅牢性と安全性を大きく高めています。
| @@ -0,0 +1,4 @@ | |||
| PR への自動レビュー(Claude)を有効化します。 | |||
There was a problem hiding this comment.
✨ [POSITIVE] 各callerの.pr-note.mdファイルは、生成されるPRの本文に重要な情報(必要なシークレットなど)を自動的に含めることができ、非常に有用です。これにより、配布されたワークフローの前提条件が明確になり、利用者の混乱を防ぎます。
|
@claude 最新コミット(943aab4)で distribute-workflow.sh をレビュー指摘に沿って堅牢化しました(sed エスケープ・未置換トークン検出・caller 別 defaults・エラー時の継続化)。最新の変更を改めてレビューして、残課題や見落としがあれば指摘してください。 |
|
Claude finished @toshi0806's task in 2m 44s —— View job コードレビュー(943aab4)
全体評価: 前回レビューで指摘した4点はいずれも的確に対処されており、スクリプトの堅牢性が大きく向上しています。このままマージして問題ないと判断します。以下に各修正の評価と残る細かい指摘を記します。 前回指摘への対処評価✅ sed エスケープ(L134–139)esc=$(printf '%s' "$val" | sed -e 's/[\\&|]/\\&/g')
SED_ARGS+=(-e "s|__${key}__|${esc}|g")
✅ 未置換トークン検出(L144–145)leftover=$(render "$TEMPLATE" | grep -oE '__[A-Za-z0-9_]+__' | sort -u | tr '\n' ' ' || true)
[[ -z "${leftover// /}" ]] || die "..."テンプレートファイルを配布前に一度だけ検証し、どのリポジトリにも触れる前に fail する設計は正確です。 ✅
|
Follow-up to the @claude re-review: - [LOW] Extend the unsubstituted-__TOKEN__ check to the caller's pr-note, not just the template, so a future caller with tokens in its note can't ship them unrendered. - [INFO] README: note that --var overrides --ref/--model/--language (e.g. --var REF=x beats --ref).
再レビュー指摘への対応(commit 20c8a79)
[LOW] pr-note の未置換トークンが未検査配布前の for f in "$TEMPLATE" "$NOTE_FILE"; do
[[ -f "$f" ]] || continue
leftover=$(render "$f" | grep -oE '__[A-Za-z0-9_]+__' | sort -u | tr '\n' ' ' || true)
[[ -z "${leftover// /}" ]] || die "$(basename "$f") has unsubstituted tokens: ..."
done
[INFO]
|
| for f in "$TEMPLATE" "$NOTE_FILE"; do | ||
| [[ -f "$f" ]] || continue | ||
| leftover=$(render "$f" | grep -oE '__[A-Za-z0-9_]+__' | sort -u | tr '\n' ' ' || true) | ||
| [[ -z "${leftover// /}" ]] || die "$(basename "$f") has unsubstituted tokens: ${leftover}(provide them via --var / --model / --language or a ${CALLER}.defaults file)" |
There was a problem hiding this comment.
✨ [POSITIVE] 配布前にテンプレートやPR注記に未置換の__TOKEN__が残っていないかチェックする機能は、非常に堅牢な安全対策です。これにより、不完全なワークフローが誤って配布されることを防ぎ、汎用スクリプトとしての信頼性を高めています。
| for kv in "${ORDERED[@]}"; do | ||
| key="${kv%%=*}"; val="${kv#*=}" | ||
| [[ "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]] || die "invalid token key: '$key'" | ||
| esc=$(printf '%s' "$val" | sed -e 's/[\\&|]/\\&/g') |
There was a problem hiding this comment.
✨ [POSITIVE] --varオプションで指定される値に含まれる特殊文字(\、&、|)がsedで適切にエスケープされるように処理されている点は、非常に堅牢な設計です。これにより、任意の値を安全に置換できるため、スクリプトの汎用性が高まります。
| | `--list-candidates` | — | 非アーカイブの smkwlab リポジトリを表示して終了 | | ||
| | `-h`, `--help` | — | ヘルプ | | ||
|
|
||
| ### 新しい共有ワークフローを配布対象に追加するには |
There was a problem hiding this comment.
✨ [POSITIVE] scripts/README.mdに「新しい共有ワークフローを配布対象に追加するには」というセクションが追加されたことで、将来的に新しいワークフローを追加する際のプロセスが明確になり、非常に分かりやすくなりました。これは、この汎用化されたスクリプトの利用を促進する上で重要な改善です。
概要
レビュー専用だった
distribute-claude-review.shを、特定ワークフロー・シークレットに依存しない汎用配布スクリプトdistribute-workflow.shに作り直す。今後の共有ワークフロー(レビューやANTHROPIC_API_KEYと無関係なものも含む)を、テンプレートを1つ追加するだけで配布できるようにする。変更内容
scripts/distribute-workflow.sh: caller 非依存。<caller>テンプレートを各リポジトリの.github/workflows/<caller>.ymlとして設置し、同名 reusable を@<ref>で呼ぶ--ref(__REF__)+ 汎用--var KEY=VALUE(__KEY__)。GitHub の${{ }}式は不変--model/--languageは--var MODEL=/--var LANGUAGE=の別名(Claude caller 用の利便)ANTHROPIC_API_KEY/ model のハードコードなし。唯一の前提は gh の write 権限scripts/callers/: caller テンプレート(claude-code-review.yml/claude-mention.yml)と、各 PR 注記*.pr-note.mdANTHROPIC_API_KEY必須という固有事情は pr-note と README の caller 表に分離(スクリプトには書かない)scripts/README.md: ワークフロー非依存に再構成。caller 別の前提を表で明示+「新しい共有ワークフローの追加手順」を記載設計の要点
ワークフロー固有の事情(必要シークレット・可変値・PR 注記)は caller 側に置き、スクリプトは「テンプレートを置換して PR を作る」汎用機構に徹する。レビュー・mention 以外のワークフローもそのまま配布対象にできる。
検証
bash -n/shellcheckクリーン--list-callers、dry-run、生成 caller の YAML 妥当性、PR 本文(汎用+pr-note)すべて確認後続
本 PR マージ後、この汎用スクリプトで
claude-mentionを6テンプレートへ配布します(別途)。