From fb6daf3d293516d75e3967144a54a84928af59bc Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Thu, 1 Oct 2026 02:31:18 +0000 Subject: [PATCH 1/3] fix(local-review): a profile it cannot read says so, and the self-test catches a broken setup A profile: that is not a literal (an expression, a value it cannot parse) silently became general, so the local review could differ from CI's; it now says so and points at --profile, and a comment after the value is read. The self-test built its scratch repositories with errexit off, so a broken setup passed unnoticed; it runs in a shell of its own now. One case passed --no-claude twice. The README and claude-review.yml say that central-ref must follow a pinned uses: line. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 4 +++- README.md | 5 ++++- scripts/local-review.sh | 25 ++++++++++++++++++------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index fef4aab..508708a 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -39,7 +39,9 @@ name: Claude review # (policy/review-policy.default.yml here, .github/review-policy.yml in the # repository, read from its base branch). Inputs: profile (the stack profile), # central-ref (where the profiles, the policy and scripts/review-brief.sh -# are read from, default v2), +# are read from, default v2: a caller that pins `uses:` to a version, say +# @v2.7.0, passes the same version here, or the workflow runs at its pin +# while the policy and the brief follow v2), # max-auto-reviews. # # on: diff --git a/README.md b/README.md index 22d3a13..6863224 100644 --- a/README.md +++ b/README.md @@ -159,7 +159,10 @@ templates fails to start once it points at `v2`: ## Reusable workflows Call them pinned to `@v2`; a breaking change ships as `v2`. A stack suffix -(`-wp`) appears only when the steps are specific to that stack. +(`-wp`) appears only when the steps are specific to that stack. A workflow +with a `central-ref` input reads the scripts, profiles and policy at that +ref (default `v2`), not at the ref of its `uses:` line: a caller that pins +`uses:` to a version passes the same version as `central-ref`. | Workflow | Does | Inputs | | --- | --- | --- | diff --git a/scripts/local-review.sh b/scripts/local-review.sh index 6a04b6c..e5a8e5c 100755 --- a/scripts/local-review.sh +++ b/scripts/local-review.sh @@ -38,10 +38,14 @@ test_case() { local dir out got dir=$(mktemp -d) out=$( - cd "$dir" && git init -q && git config user.email t@t && git config user.name t - git commit -q --allow-empty -m "chore: base" && git branch -q -M main && git update-ref refs/remotes/origin/main HEAD - git checkout -q -b docs/change - eval "$setup" + cd "$dir" || exit 97 + # The scratch repository is built in a shell of its own: errexit is off + # inside $( ) && …, so a broken setup would otherwise go unnoticed. + bash -euo pipefail -c ' + git init -q && git config user.email t@t && git config user.name t + git commit -q --allow-empty -m "chore: base" && git branch -q -M main && git update-ref refs/remotes/origin/main HEAD + git checkout -q -b docs/change + eval "$1"' _ "$setup" || { echo "the setup failed"; exit 97; } bash "$CENTRAL/scripts/local-review.sh" --no-claude "$@" 2>&1 ) && got=0 || got=$? rm -rf "$dir" @@ -59,6 +63,8 @@ if [ "${1:-}" = "--test" ]; then test_case "no profile, no origin remote: general, and it runs to the brief" 0 "Brief:" "$commit" || fail=1 test_case "a profile the workflow passes is used" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" || fail=1 test_case "a quoted profile is read without quotes" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: \'plugin-wp\'\n')" --title "docs(readme): two commits" || fail=1 + test_case "a profile with a comment after it is used" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp # the stack\n')" --title "docs(readme): two commits" || fail=1 + test_case "a profile that is not a literal is general, and says so" 0 "is not a literal" "$(wf $'jobs:\n review:\n with:\n profile: ${{ inputs.profile }}\n')" --title "docs(readme): two commits" || fail=1 test_case "--profile wins over the workflow" 0 "profile general" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" --profile general || fail=1 test_case "no commit on the branch" 1 "adds no commit" ":" || fail=1 test_case "two commits and no --title" 64 "say the pull request title" "$commit && echo b > b.md && git add b.md && git commit -q -m 'docs(readme): another'" || fail=1 @@ -66,7 +72,7 @@ if [ "${1:-}" = "--test" ]; then bodies=$(mktemp -d); printf %s "$good" > "$bodies/good.md"; printf %s "$empty" > "$bodies/empty.md" test_case "a description with an empty Why" 1 "section is empty" "$commit" --body-file "$bodies/empty.md" || fail=1 test_case "a description filled in" 0 "Conventions OK." "$commit" --body-file "$bodies/good.md" || fail=1 - test_case "the origin remote names the repository" 0 "Review brief: o/r," "git remote add origin https://github.com/o/r.git && $commit" --no-claude || fail=1 + test_case "the origin remote names the repository" 0 "Review brief: o/r," "git remote add origin https://github.com/o/r.git && $commit" || fail=1 rm -rf "$bodies" test_case "an unknown option" 64 "usage:" "$commit" --nope || fail=1 @@ -158,8 +164,13 @@ if [ -n "$body_file" ]; then body=$(cat "$body_file"); fi if [ -z "$profile" ]; then # The first `profile:` a workflow passes, quoted or not. No match is the # usual case outside a plugin: `|| true`, or set -e ends the script here. - profile=$( { grep -hE '^[[:space:]]+profile:' .github/workflows/*.yml 2>/dev/null || true; } | head -1 \ - | sed -E "s/^[[:space:]]+profile:[[:space:]]*['\"]?([A-Za-z0-9_-]*).*/\\1/") + line=$( { grep -hE '^[[:space:]]+profile:' .github/workflows/*.yml 2>/dev/null || true; } | head -1) + profile=$(sed -E "s/^[[:space:]]+profile:[[:space:]]*['\"]?([A-Za-z0-9_-]*).*/\\1/" <<<"$line") + raw=$(sed -E 's/^[[:space:]]+profile:[[:space:]]*//; s/[[:space:]]+#.*$//; s/[[:space:]]+$//' <<<"$line") + if [ -n "$line" ] && { [ -z "$profile" ] || [ "${raw//[\'\"]/}" != "$profile" ]; }; then + echo "The workflow's profile: ($raw) is not a literal this script can read: the review uses general. Pass --profile to say which." >&2 + profile='' + fi profile=${profile:-general} fi work=$(mktemp -d "${TMPDIR:-/tmp}/dx-review.XXXXXX") From 38de68995b6f543f4708988433bd6dbe75ebd698 Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Thu, 1 Oct 2026 02:49:30 +0000 Subject: [PATCH 2/3] fix(local-review): only a profile: with a value is read, so an input's declaration is skipped The local review found that a bare profile: key, as claude-review.yml declares its input, raised the new warning and could hide the caller's value. Co-Authored-By: Claude Opus 5.5 --- scripts/local-review.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/local-review.sh b/scripts/local-review.sh index e5a8e5c..2600455 100755 --- a/scripts/local-review.sh +++ b/scripts/local-review.sh @@ -64,6 +64,7 @@ if [ "${1:-}" = "--test" ]; then test_case "a profile the workflow passes is used" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" || fail=1 test_case "a quoted profile is read without quotes" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: \'plugin-wp\'\n')" --title "docs(readme): two commits" || fail=1 test_case "a profile with a comment after it is used" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp # the stack\n')" --title "docs(readme): two commits" || fail=1 + test_case "an input declaring profile: is not the value" 0 "profile plugin-wp" "$(wf $'on:\n workflow_call:\n inputs:\n profile:\n type: string\njobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" || fail=1 test_case "a profile that is not a literal is general, and says so" 0 "is not a literal" "$(wf $'jobs:\n review:\n with:\n profile: ${{ inputs.profile }}\n')" --title "docs(readme): two commits" || fail=1 test_case "--profile wins over the workflow" 0 "profile general" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" --profile general || fail=1 test_case "no commit on the branch" 1 "adds no commit" ":" || fail=1 @@ -164,7 +165,8 @@ if [ -n "$body_file" ]; then body=$(cat "$body_file"); fi if [ -z "$profile" ]; then # The first `profile:` a workflow passes, quoted or not. No match is the # usual case outside a plugin: `|| true`, or set -e ends the script here. - line=$( { grep -hE '^[[:space:]]+profile:' .github/workflows/*.yml 2>/dev/null || true; } | head -1) + # Only a `profile:` with a value: a bare key is an input's declaration. + line=$( { grep -hE '^[[:space:]]+profile:[[:space:]]*[^[:space:]#]' .github/workflows/*.yml 2>/dev/null || true; } | head -1) profile=$(sed -E "s/^[[:space:]]+profile:[[:space:]]*['\"]?([A-Za-z0-9_-]*).*/\\1/" <<<"$line") raw=$(sed -E 's/^[[:space:]]+profile:[[:space:]]*//; s/[[:space:]]+#.*$//; s/[[:space:]]+$//' <<<"$line") if [ -n "$line" ] && { [ -z "$profile" ] || [ "${raw//[\'\"]/}" != "$profile" ]; }; then From 241b8f3e669d45c60e6bc506bf96a62b56db778a Mon Sep 17 00:00:00 2001 From: Pablo Ariel Di Loreto Date: Thu, 1 Oct 2026 02:50:39 +0000 Subject: [PATCH 3/3] fix(local-review): a flow-style input declaration of profile: is skipped too Co-Authored-By: Claude Opus 5.5 --- scripts/local-review.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/local-review.sh b/scripts/local-review.sh index 2600455..dd7583d 100755 --- a/scripts/local-review.sh +++ b/scripts/local-review.sh @@ -65,6 +65,7 @@ if [ "${1:-}" = "--test" ]; then test_case "a quoted profile is read without quotes" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: \'plugin-wp\'\n')" --title "docs(readme): two commits" || fail=1 test_case "a profile with a comment after it is used" 0 "profile plugin-wp" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp # the stack\n')" --title "docs(readme): two commits" || fail=1 test_case "an input declaring profile: is not the value" 0 "profile plugin-wp" "$(wf $'on:\n workflow_call:\n inputs:\n profile:\n type: string\njobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" || fail=1 + test_case "a flow-style input declaration is not the value" 0 "profile plugin-wp" "$(wf $'on:\n workflow_call:\n inputs:\n profile: { type: string }\njobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" || fail=1 test_case "a profile that is not a literal is general, and says so" 0 "is not a literal" "$(wf $'jobs:\n review:\n with:\n profile: ${{ inputs.profile }}\n')" --title "docs(readme): two commits" || fail=1 test_case "--profile wins over the workflow" 0 "profile general" "$(wf $'jobs:\n review:\n with:\n profile: plugin-wp\n')" --title "docs(readme): two commits" --profile general || fail=1 test_case "no commit on the branch" 1 "adds no commit" ":" || fail=1 @@ -165,8 +166,9 @@ if [ -n "$body_file" ]; then body=$(cat "$body_file"); fi if [ -z "$profile" ]; then # The first `profile:` a workflow passes, quoted or not. No match is the # usual case outside a plugin: `|| true`, or set -e ends the script here. - # Only a `profile:` with a value: a bare key is an input's declaration. - line=$( { grep -hE '^[[:space:]]+profile:[[:space:]]*[^[:space:]#]' .github/workflows/*.yml 2>/dev/null || true; } | head -1) + # Only a `profile:` with a value: a bare key, or a `{ type: … }` map, is an + # input's declaration. + line=$( { grep -hE '^[[:space:]]+profile:[[:space:]]*[^[:space:]#{]' .github/workflows/*.yml 2>/dev/null || true; } | head -1) profile=$(sed -E "s/^[[:space:]]+profile:[[:space:]]*['\"]?([A-Za-z0-9_-]*).*/\\1/" <<<"$line") raw=$(sed -E 's/^[[:space:]]+profile:[[:space:]]*//; s/[[:space:]]+#.*$//; s/[[:space:]]+$//' <<<"$line") if [ -n "$line" ] && { [ -z "$profile" ] || [ "${raw//[\'\"]/}" != "$profile" ]; }; then