Skip to content

fix: point pr-review workflow at the smart review recipes#36

Merged
Jercik merged 1 commit into
mainfrom
fix/smart-review-recipes
Jul 2, 2026
Merged

fix: point pr-review workflow at the smart review recipes#36
Jercik merged 1 commit into
mainfrom
fix/smart-review-recipes

Conversation

@Jercik

@Jercik Jercik commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

The server deleted these pr-review recipe ids; the wrapper workflows here still passed them, so both pull_request_target review jobs would fail on the next run:

  • pr-review-approach-1, pr-review-approach-4 → replaced by two draws of pr-review-approach-smart
  • pr-review-code-1, pr-review-code-2 → replaced by two draws of pr-review-code-smart

pr-review-approach-2 and pr-review-approach-3 were retained unchanged and still run alongside the new smart draws.

Changes

Ran j4k-align --fix from a checkout pinned to the exact template revision that introduced this fix (j4k/align#53). It consolidated .github/workflows/pr-review-on-open.yml and .github/workflows/pr-review-on-push.yml into a single .github/workflows/pr-review.yml using the current recipe ids, and also refreshed other managed files that had drifted from the current templates:

  • .github/workflows/pr-review-on-open.yml, .github/workflows/pr-review-on-push.yml → removed
  • .github/workflows/pr-review.yml → added (consolidated approach + code jobs, smart recipes)
  • .github/workflows/checks.yml, .github/workflows/dedupe-check.yml → strip untrusted install config, verify/configure private registry auth, --ignore-scripts --ignore-pnpmfile
  • .github/workflows/commit-msg.yml → validate both squash-merge PR title and rebase-merge commits, pass PR title via env instead of inline interpolation
  • .github/workflows/release-npm.yml → configure private registry auth for install, public npm registry for publish
  • .githooks/commit-msg, .githooks/pre-commit → sync to current hardened hooks
  • release.config.mjs → added (was missing)
  • package.jsonpackageManager bumped to pnpm@11.8.0
  • pnpm-workspace.yaml → add registries.default: https://npm.j4k.dev/
  • .oxfmtrc.json → ignore .clawpatch/, .codex/
  • .vscode/settings.json, tsconfig.app.json → drift fixes

No forge-side (GitHub) settings changes were applied by this run.

Note

This PR's own review checks may stay red until merged — pull_request_target runs the base-branch copy of the workflow, so the fixed recipe ids only take effect once this lands on main.

Test plan

  • pr-review.yml runs successfully with the smart recipe ids after merge
  • CI (checks.yml, dedupe-check.yml, commit-msg.yml) passes on this PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8dc0369919

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
run: |
set -eu
if [ -z "${NODE_AUTH_TOKEN:-}" ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid requiring NPM_TOKEN on forked PR checks

For PRs opened from forks, secrets.NPM_TOKEN is empty because GitHub does not pass repository secrets to fork-triggered workflows except GITHUB_TOKEN (docs). Since this workflow runs on every pull_request, the new unconditional guard fails before install for any outside contributor PR; dedupe-check.yml has the same pattern. Consider gating the private-registry install to trusted PRs or using a public/non-secret fallback for forked PR checks.

Useful? React with 👍 / 👎.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach Review: The approach looks good. Using a template synchronization tool to consolidate workflows, apply security enhancements, and update configurations is standard and effective.


Approach review by Approach Review 2 (gemini-3.1-pro-preview)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach Review: This is a j4k-align --fix template-sync run, not a bespoke solution designed in this repo — the approach is dictated by the upstream j4k/align template revision that introduced the smart recipes. There's no fundamentally better alternative to evaluate: the actual fix (consolidating pr-review-on-open.yml + pr-review-on-push.yml into a single pr-review.yml pointed at pr-review-approach-smart / pr-review-code-smart) is the only way to recover from the deleted recipe ids, and the rest is hardened CI hygiene (strip untrusted install config, registry auth setup, --ignore-scripts --ignore-pnpmfile) that comes straight from the template. Approach looks good.


Approach review by Approach Review 3 (OpenCode Wafer) (GLM-5.2)

@Jercik Jercik merged commit cc40dfb into main Jul 2, 2026
7 of 11 checks passed
@Jercik Jercik deleted the fix/smart-review-recipes branch July 2, 2026 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant