Conversation
…ntentional artipacked
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
🔇 Additional comments (6)
WalkthroughThis PR updates 20+ GitHub Actions workflows and related configs. Changes include adding Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 8 minutes and 26 seconds.Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/actions/get-image-tag/action.yml (1)
26-56: 💤 Low value
${{ github.sha }}still inline — replace with the pre-existing$GITHUB_SHAenv var.The migration to
INPUTS_TAGin this same step correctly avoids inline expressions, but lines 31 and 36 still inline${{ github.sha }}. SinceGITHUB_SHAis a default runner environment variable available in all steps (including composite action steps), no extraenv:mapping is needed.♻️ Proposed fix
if [[ "${GITHUB_REF_NAME}" == infra-*-* ]]; then env=$(echo ${GITHUB_REF_NAME} | cut -d- -f2) - sha=$(echo ${{ github.sha }} | head -c7) + sha=$(echo ${GITHUB_SHA} | head -c7) ts=$(date +%s) tag=${env}-${sha}-${ts} elif [[ "${GITHUB_REF_NAME}" == re2-*-* ]]; then env=$(echo ${GITHUB_REF_NAME} | cut -d- -f2) - sha=$(echo ${{ github.sha }} | head -c7) + sha=$(echo ${GITHUB_SHA} | head -c7) ts=$(date +%s) tag=${env}-${sha}-${ts}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/get-image-tag/action.yml around lines 26 - 56, Replace the two inline GitHub expression usages of ${{ github.sha }} in the tag-generation branches (the sha assignment used when matching infra-*-* and re2-*-* under the INPUTS_TAG/GITHUB_REF_NAME logic) with the runner environment variable $GITHUB_SHA; update the sha assignments where the script currently sets sha=$(echo ${{ github.sha }} | head -c7) to use sha=$(echo "$GITHUB_SHA" | head -c7) so the composite action uses the pre-existing environment variable rather than an inline expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish-worker.yml:
- Around line 11-15: Declare the optional secrets as job-level environment
variables (set DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} and
DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} under the job env) and guard the
Docker Hub login step with an if conditional (use if: ${{ env.DOCKERHUB_USERNAME
}} ) so the login only runs when credentials are provided; update the login step
referenced as the Docker login step to include this if guard and ensure any
downstream steps that require the image push are likewise conditional or handle
missing credentials.
---
Nitpick comments:
In @.github/actions/get-image-tag/action.yml:
- Around line 26-56: Replace the two inline GitHub expression usages of ${{
github.sha }} in the tag-generation branches (the sha assignment used when
matching infra-*-* and re2-*-* under the INPUTS_TAG/GITHUB_REF_NAME logic) with
the runner environment variable $GITHUB_SHA; update the sha assignments where
the script currently sets sha=$(echo ${{ github.sha }} | head -c7) to use
sha=$(echo "$GITHUB_SHA" | head -c7) so the composite action uses the
pre-existing environment variable rather than an inline expression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35334d25-0746-47fa-8f75-4cb639bf6740
📒 Files selected for processing (24)
.github/actions/get-image-tag/action.yml.github/workflows/changesets-pr.yml.github/workflows/claude-md-audit.yml.github/workflows/claude.yml.github/workflows/docs.yml.github/workflows/e2e-webapp.yml.github/workflows/e2e.yml.github/workflows/helm-prerelease.yml.github/workflows/pr_checks.yml.github/workflows/publish-webapp.yml.github/workflows/publish-worker-v4.yml.github/workflows/publish-worker.yml.github/workflows/publish.yml.github/workflows/release-helm.yml.github/workflows/release.yml.github/workflows/sdk-compat.yml.github/workflows/typecheck.yml.github/workflows/unit-tests-internal.yml.github/workflows/unit-tests-packages.yml.github/workflows/unit-tests-webapp.yml.github/workflows/unit-tests.yml.github/workflows/vouch-check-pr.yml.github/workflows/workflow-checks.yml.github/zizmor.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (22)
.github/workflows/claude-md-audit.yml (1)
33-33: LGTM —persist-credentials: falsecorrectly added..github/workflows/sdk-compat.yml (1)
24-24: LGTM —persist-credentials: falseconsistently applied to all four checkout steps..github/workflows/typecheck.yml (1)
18-18: LGTM.github/workflows/docs.yml (1)
30-31: LGTM.github/workflows/claude.yml (1)
32-32: LGTM.github/workflows/publish-worker-v4.yml (1)
65-81: LGTM — env-var pattern for step outputs correctly applied, consistent withpublish-webapp.yml..github/workflows/e2e.yml (1)
60-60: No action needed on line 60 — Zizmor does not flag statically-defined matrix values.Zizmor's template-injection audit explicitly handles static matrix definitions and will not flag
${{ matrix.package-manager }}as an injection risk when the matrix is fully static (e.g.,["npm", "pnpm"]). The new Zizmor CI gate will not cause a build failure for this usage. While hoisting to an env var is a valid best practice for readability, it is not required to pass Zizmor checks.> Likely an incorrect or invalid review comment..github/workflows/publish-worker.yml (1)
37-38:persist-credentials: false— LGTMThis job performs no
git push, so dropping persisted credentials is correct..github/workflows/unit-tests-webapp.yml (1)
8-12: Optional secrets +persist-credentials: false— LGTMConditional DockerHub login guards are already in place, and neither job pushes to git, so disabling credential persistence is correct.
Also applies to: 57-57, 135-135
.github/workflows/unit-tests-internal.yml (1)
8-12: Optional secrets +persist-credentials: false— LGTMMirrors the correct pattern from
unit-tests-webapp.yml.Also applies to: 57-57, 127-127
.github/workflows/e2e-webapp.yml (1)
8-12: Optional secrets +persist-credentials: false— LGTMConsistent with the pattern used across the other test workflows.
Also applies to: 52-52
.github/workflows/release.yml (4)
36-36:permissions: {}on both passive jobs — LGTM
show-release-summaryonly writes to$GITHUB_STEP_SUMMARY(a file path, no token needed), anddispatch-changelogexclusively usesCROSS_REPO_PAT, notGITHUB_TOKEN. Empty permission grants are appropriate in both cases.Also applies to: 243-243
69-82: Artipacked suppression + template-injection fix — LGTMThe inline comment correctly documents why
persist-credentialsmust stay enabled (subsequentgit pushsteps for version tags), and movinggithub.event.inputs.refintoGITHUB_EVENT_INPUTS_REFeliminates the template-injection risk in the shell comparison.
168-176: Explicitpublish-dockerpermissions and secrets — LGTMReplacing
secrets: inheritwith explicit pass-throughs is the right pattern. Theid-token: writegrant on this caller job is required to letpublish-webapp.yml(called transitively throughpublish.yml) perform OIDC token exchange.
262-308: Prerelease hardening — LGTM
persist-credentials: falseis safe here because the prerelease publish uses--no-git-tag(no push). Movinggithub.event.inputs.prerelease_tag(user-controlled) toGITHUB_EVENT_INPUTS_PRERELEASE_TAGcorrectly prevents shell injection in both thechangeset versionandchangeset publishinvocations..github/workflows/release-helm.yml (2)
32-33:persist-credentials: falsein both jobs — LGTMNeither
lint-and-testnorreleasepushes back to the repository checkout (the Helm chart is pushed viahelm pushto GHCR OCI, not viagit push), so dropping persisted credentials is correct.Also applies to: 73-74
96-136: Version extraction refactor — LGTMUsing
INPUTS_CHART_VERSIONto carry the user-controlledinputs.chart_versioninto the shell is the right fix.GITHUB_REF_NAMEis already a default GitHub Actions environment variable so no explicit mapping is needed. TheSTEPS_VERSION_OUTPUTS_VERSIONenv-var-per-step pattern is correct; the remaining inline${{ env.CHART_NAME }}and${{ github.repository_owner }}references are workflow-level values that cannot be user-controlled and carry no injection risk..github/workflows/helm-prerelease.yml (2)
36-38:persist-credentials: falsein both jobs — LGTMHelm chart publishing goes through
helm pushto GHCR OCI, not agit push, so credential persistence is unnecessary in both jobs.Also applies to: 82-84
119-119:GITHUB_REF_NAMEswitch andSTEPS_VERSION_OUTPUTS_VERSIONenv var pattern — LGTM
GITHUB_REF_NAMEis a default GitHub Actions environment variable (set automatically on every step), so removing the explicit${{ github.ref_name }}interpolation is correct. TheSTEPS_VERSION_OUTPUTS_VERSIONenv-var-per-step pattern is applied consistently across thesed,helm push, and step-summary blocks.Also applies to: 128-170
.github/workflows/unit-tests-packages.yml (1)
8-12: Optional secrets +persist-credentials: false— LGTMConsistent with the pattern applied across all other unit-test reusable workflows in this PR.
Also applies to: 57-57, 127-127
.github/workflows/workflow-checks.yml (1)
37-51: Nice hardening of the new scanner job.Pinned actions,
persist-credentials: false, and job-scoped permissions make this addition fit the rest of the workflow lockdown work well..github/workflows/vouch-check-pr.yml (1)
12-15: This review comment is based on an incorrect understanding of GitHub Actions permissions. Pull request comments in GitHub Actions requirepull-requests: writepermission, notissues: write. The workflow's current permissions—pull-requests: writein bothcheck-vouchandrequire-draft—are sufficient for the auto-close comment posted bymitchellh/vouch/action/check-prand thegh pr close --commentcall. No permission changes are needed.> Likely an incorrect or invalid review comment.
2 participants
Adds zizmor alongside the actionlint job from #3503. Both now run as parallel jobs in a single
.github/workflows/workflow-checks.yml, triggered on.github/workflows/**and.github/actions/**changes.Zizmor is configured with
unpinned-uses: hash-pinpolicy via.github/zizmor.yml, so any future unpinned action will fail CI. Findings upload SARIF to the Security tab alongside CodeQL.Bulk of the diff is cleanup of the findings zizmor surfaced on first run.
zizmor --fix=allhandled most of them mechanically; the rest were judgment calls.