App 1009 remind docs#2219
Conversation
|
👋 @ofek1weiss |
📝 WalkthroughWalkthroughMultiple GitHub Actions workflows were changed to add an explicit empty top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/remind-docs-and-tests.yml (1)
14-14: Pinactions/github-scriptto a full commit SHA for supply-chain security.Using a mutable tag (
@v7) is functional, but pinning to a 40-char commit SHA improves workflow security and matches the practice intriage-labels.ymlandbump-version.yml. The latest v7 (v7.1.0) corresponds to commitf28e40c.🔒 Suggested hardening diff
- - uses: actions/github-script@v7 + - uses: actions/github-script@f28e40c # v7.1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/remind-docs-and-tests.yml at line 14, Replace the mutable reference "uses: actions/github-script@v7" with a pinned full 40-character commit SHA for actions/github-script (the v7.1.0 commit begins with f28e40c), i.e. change the uses line to "uses: actions/github-script@<full-40-char-commit-sha>" so the workflow is pinned to that exact commit for supply-chain security.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/remind-docs-and-tests.yml:
- Line 14: Replace the mutable reference "uses: actions/github-script@v7" with a
pinned full 40-character commit SHA for actions/github-script (the v7.1.0 commit
begins with f28e40c), i.e. change the uses line to "uses:
actions/github-script@<full-40-char-commit-sha>" so the workflow is pinned to
that exact commit for supply-chain security.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95fb903c-a9a5-40cc-86be-0b5622fdbb8f
📒 Files selected for processing (1)
.github/workflows/remind-docs-and-tests.yml
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
was unused before, as we do not push the image in this workflow
| images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} | ||
|
|
||
| - name: Build and push Docker image | ||
| - name: Build Docker image |
There was a problem hiding this comment.
we really do not push, name was misleading
| env: | ||
| EVENT_BODY: ${{ github.event_name == 'issues' && github.event.issue.body || github.event.pull_request.body }} | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == 'issues' ]]; then |
There was a problem hiding this comment.
there was no need for all of the curling, all data was avaliable in the event data
| Thank you for raising your pull request. | ||
| Please make sure to add tests and document all user-facing changes. | ||
| You can do this by editing the `docs` files in this pull request. | ||
| script: | |
There was a problem hiding this comment.
changed to a simple 3 line script to remove external dependancy
| permissions: | ||
| actions: write | ||
| contents: write # only for delete-branch option | ||
| # Add `contents: write` only if enabling `delete-branch: true`. |
There was a problem hiding this comment.
the delete-branch option option of the workflow is not used, so this permission is not needed
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-release.yml (1)
63-77:⚠️ Potential issue | 🟡 MinorThe failure recommendation path is unreachable because the
recommendationjob is skipped whenvalidate-upgrade-clifails.By default, GitHub Actions skips dependent jobs when their prerequisite jobs fail. Since
recommendationlistsvalidate-upgrade-cliinneedswithout a job-levelif: ${{ always() }}, the entire job is skipped when any matrix variant ofvalidate-upgrade-clifails. This means the second step usingif: ${{ failure() }}never executes.Add
if: ${{ always() }}at the job level to force execution, then drive the two steps fromneeds.validate-upgrade-cli.result:Suggested fix
recommendation: + if: ${{ always() }} runs-on: ubuntu-latest needs: [get-latest-release-tags, validate-upgrade-cli] permissions: contents: read steps: - name: recommend unbreaking change upgrade + if: ${{ needs.validate-upgrade-cli.result == 'success' }} run: | echo "Everything ran successfully, you can bump version to ${{ needs.get-latest-release-tags.outputs.py-bumped-number }}" - name: recommend breaking change upgrade - if: ${{ failure() }} + if: ${{ needs.validate-upgrade-cli.result == 'failure' }} uses: actions/github-script@v8 with: script: | core.setFailed("Upgrade validation failed. Make sure you do anything to keep backwards compatibility - If not possible, update minor.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-release.yml around lines 63 - 77, The recommendation job is skipped when validate-upgrade-cli fails, so add a job-level guard by adding if: ${{ always() }} to the recommendation job (symbol: recommendation) and then replace the per-step failure logic with checks against needs.validate-upgrade-cli.result: run the "recommend unbreaking change upgrade" step only when needs.validate-upgrade-cli.result == 'success' and run the "recommend breaking change upgrade" step only when needs.validate-upgrade-cli.result != 'success' (keep the breaking step using actions/github-script and core.setFailed as currently defined).
🤖 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/close_pylon_issue.yml:
- Line 21: The pylon_ticket_id extraction can cause the step to fail when grep
finds no match; modify the assignment that uses grep -oP '(?<=<!--
pylon-ticket-id:
)\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)' so it
never returns a non‑zero exit (for example append a safe fallback like "|| true"
or capture the output conditionally) and ensure pylon_ticket_id is set to empty
when there’s no match; this keeps the variable check later (pylon_ticket_id)
from being short‑circuited by a grep error.
- Around line 18-19: The EVENT_BODY env currently allows author-controlled PR
body into a privileged workflow; restrict it by only using pull request body
when the PR originates from the repository (not a fork). Update the EVENT_BODY
expression to use github.event.issue.body for issues, and for pull requests use
github.event.pull_request.body only if
github.event.pull_request.head.repo.full_name == github.repository (otherwise
default to an empty string or a safe value). Ensure the change modifies the
EVENT_BODY environment assignment so forked PRs cannot inject author-controlled
content into privileged actions.
In @.github/workflows/test-release.yml:
- Around line 11-12: The reusable-workflow call permissions are not explicit for
the bump-version job; add an explicit job-level permissions block to the
bump-version job (the job named "bump-version") so the called reusable workflow
receives only the intended GITHUB_TOKEN scopes (e.g., add permissions: {} or the
minimum required scopes) instead of inheriting repository defaults—update the
bump-version job definition to include the permissions block similar to
validate-upgrade-cli.
---
Outside diff comments:
In @.github/workflows/test-release.yml:
- Around line 63-77: The recommendation job is skipped when validate-upgrade-cli
fails, so add a job-level guard by adding if: ${{ always() }} to the
recommendation job (symbol: recommendation) and then replace the per-step
failure logic with checks against needs.validate-upgrade-cli.result: run the
"recommend unbreaking change upgrade" step only when
needs.validate-upgrade-cli.result == 'success' and run the "recommend breaking
change upgrade" step only when needs.validate-upgrade-cli.result != 'success'
(keep the breaking step using actions/github-script and core.setFailed as
currently defined).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d568dae-5a3f-48ed-82c5-724e2de7ad78
📒 Files selected for processing (8)
.github/workflows/assign-devin-prs.yml.github/workflows/build-docker-image.yml.github/workflows/cleanup-stale-schemas.yml.github/workflows/close_pylon_issue.yml.github/workflows/run-precommit.yml.github/workflows/stale.yml.github/workflows/test-github-action.yml.github/workflows/test-release.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/assign-devin-prs.yml
| env: | ||
| EVENT_BODY: ${{ github.event_name == 'issues' && github.event.issue.body || github.event.pull_request.body }} |
There was a problem hiding this comment.
Restrict privileged close actions for pull_request_target events.
Line 19 uses PR body content (author-controlled) to drive a secret-backed external API write. With pull_request_target, fork PRs can influence which Pylon ticket gets closed.
🔐 Suggested guard (trust boundary)
jobs:
close_pylon_ticket:
+ if: ${{ github.event_name == 'issues' || github.event.pull_request.head.repo.full_name == github.repository }}
runs-on: ubuntu-latest
permissions: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/close_pylon_issue.yml around lines 18 - 19, The EVENT_BODY
env currently allows author-controlled PR body into a privileged workflow;
restrict it by only using pull request body when the PR originates from the
repository (not a fork). Update the EVENT_BODY expression to use
github.event.issue.body for issues, and for pull requests use
github.event.pull_request.body only if
github.event.pull_request.head.repo.full_name == github.repository (otherwise
default to an empty string or a safe value). Ensure the change modifies the
EVENT_BODY environment assignment so forked PRs cannot inject author-controlled
content into privileged actions.
There was a problem hiding this comment.
I think we do intentionally want this workflow to run on forks, but the security issue is valid.
I think though that the move to use env vars prevents script injection issues, no?
There was a problem hiding this comment.
@haritamar — great question, and you're partially right. It's worth distinguishing between two separate security concerns here:
1. Script injection / shell command injection — ✅ Mitigated by the env var change
The primary defense against command injection is binding inputs to environment variables. By referencing the env var using standard shell syntax (e.g., "$MY_VAR"), the shell automatically treats the variable's content as a literal string. So yes — the move from ${{ github.event.pull_request.body }} directly inline in the run: block to env: EVENT_BODY: ${{ ... }} / "$EVENT_BODY" in shell does prevent shell command injection. Using workflow input as environment variables instead of interpolation prevents script injection.
The grep -oP regex further constrains what can be extracted to a strict UUID format, which adds another layer of safety.
2. Unauthorized Pylon ticket closure from fork PRs —
pull_request_target events execute based on user-supplied pull requests that can come from external forks, and are executed with access to action secrets. Care should be taken to ensure untrusted input is not being used in a way that could influence execution.
A fork PR author who knows a valid Pylon ticket UUID could craft their PR body with the exact <!-- pylon-ticket-id: UUID --> comment format and trigger the closure of that ticket when their PR is closed. This is not shell injection — it's logic/data manipulation.
That said, the practical risk is low because:
- Pylon ticket UUIDs are not publicly guessable
- The only capability granted is closing a specific Pylon ticket (not a high-impact action)
- The regex enforces a strict UUID format, so no other content can be used
Summary: The env var change fully resolves the shell injection risk. The residual concern is limited to an attacker with knowledge of a Pylon ticket UUID being able to close it — which is low-severity given the context. If you're comfortable accepting that risk given the intentional fork support, the current implementation is reasonable. If you want to be extra safe, the job-level if guard I suggested would prevent it entirely.
| "https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}" | jq -r '.body') | ||
| pylon_ticket_id=$(echo "$PR_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)') | ||
| fi | ||
| pylon_ticket_id=$(echo "$EVENT_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)') |
There was a problem hiding this comment.
Handle “no match” safely in ticket extraction.
If no ticket marker exists, grep exits non-zero and this step can fail before your Line 25 conditional has a chance to skip the close step.
🛠️ Suggested fix
- pylon_ticket_id=$(echo "$EVENT_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)')
+ pylon_ticket_id=$(echo "$EVENT_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)' || true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pylon_ticket_id=$(echo "$EVENT_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)') | |
| pylon_ticket_id=$(echo "$EVENT_BODY" | grep -oP '(?<=<!-- pylon-ticket-id: )\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)' || true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/close_pylon_issue.yml at line 21, The pylon_ticket_id
extraction can cause the step to fail when grep finds no match; modify the
assignment that uses grep -oP '(?<=<!-- pylon-ticket-id:
)\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b(?= -->)' so it
never returns a non‑zero exit (for example append a safe fallback like "|| true"
or capture the output conditionally) and ensure pylon_ticket_id is set to empty
when there’s no match; this keeps the variable check later (pylon_ticket_id)
from being short‑circuited by a grep error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test-release.yml:
- Around line 64-67: The reusable workflow call in the validate-upgrade-cli job
is missing the required secret AWS_OIDC_ROLE_ARN declared required in
.github/workflows/test-warehouse.yml; update the secrets block where
CI_WAREHOUSE_SECRETS/CI_SLACK_WEBHOOK/CI_SLACK_TOKEN are passed (in the
test-release.yml reusable workflow call) to also pass AWS_OIDC_ROLE_ARN from the
repository secrets (add AWS_OIDC_ROLE_ARN: ${{ secrets.AWS_OIDC_ROLE_ARN }}) so
the called workflow receives the required secret.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cb6e2d1-9b3b-4183-9f66-95123a0a3839
📒 Files selected for processing (2)
.github/workflows/cleanup-stale-schemas.yml.github/workflows/test-release.yml
| repository: elementary-data/dbt-data-reliability | ||
| # Pinned to dbt-data-reliability master HEAD at update time (2026-04-27). | ||
| # Refresh intentionally when updating cleanup macro behavior. | ||
| ref: ea58f5331e334d3a6774cc90f91f299b9929dada |
There was a problem hiding this comment.
Specifically not sure about this one - I think using master makes sense, it is a protected branch
There was a problem hiding this comment.
was thinking it might be overkill myself, but it would help stop propogation of issues in dbt-data-reliability, and i dont suspect that the logic of the clear macro will change much, so i believe its worth it
| env: | ||
| EVENT_BODY: ${{ github.event_name == 'issues' && github.event.issue.body || github.event.pull_request.body }} |
There was a problem hiding this comment.
I think we do intentionally want this workflow to run on forks, but the security issue is valid.
I think though that the move to use env vars prevents script injection issues, no?
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/test-release.yml (1)
11-12:⚠️ Potential issue | 🟠 Major
permissions: {}now requires explicit permissions onbump-versionreusable call.With workflow-level permissions set to none,
bump-versionshould declare job-levelpermissions(at leastcontents: read), otherwise the called reusable workflow can be under-scoped and fail when requestingcontents: read.🔧 Proposed fix
bump-version: needs: [get-latest-release-tags, recommendation] if: ${{ inputs.should-bump-on-success }} + permissions: + contents: read uses: ./.github/workflows/bump-version.yml with: cli-version: ${{ needs.get-latest-release-tags.outputs.py-bumped-number }} dbt-package-version: ${{ needs.get-latest-release-tags.outputs.dbt }}#!/bin/bash set -euo pipefail echo "Caller job block (test-release.yml):" awk 'NR>=84 && NR<=96 {printf "%4d %s\n", NR, $0}' .github/workflows/test-release.yml echo echo "Called workflow permissions/job requirements (bump-version.yml):" awk 'NR>=10 && NR<=35 {printf "%4d %s\n", NR, $0}' .github/workflows/bump-version.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-release.yml around lines 11 - 12, The workflow-level permissions are set to none (permissions: {}), so the job that invokes the reusable workflow "bump-version" must declare job-level permissions; update the job that calls uses: ./.github/workflows/bump-version.yml in test-release.yml to include a permissions block (at minimum permissions: contents: read) so the called reusable workflow can access repository contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/test-release.yml:
- Around line 11-12: The workflow-level permissions are set to none
(permissions: {}), so the job that invokes the reusable workflow "bump-version"
must declare job-level permissions; update the job that calls uses:
./.github/workflows/bump-version.yml in test-release.yml to include a
permissions block (at minimum permissions: contents: read) so the called
reusable workflow can access repository contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74fb13e1-ca00-4373-91e7-25e477b3d822
📒 Files selected for processing (1)
.github/workflows/test-release.yml
2 participants
Summary by CodeRabbit