ci(connectors-ddl): deliver drift as a stacked regen PR; retire the reminder - #2133
Conversation
…eminder The drift gate used to fail with "regenerate locally" — an instruction most contributors cannot follow: dump-ddl.sh needs a full bootstrap first, and the HubSpot/Salesforce `discover` in it calls live APIs with credentials they do not have. New regen-pr job, on PR drift only: it takes the freshly dumped snapshot (uploaded by the gate as an artifact), force-pushes it to regen-ddl/<head-branch> with a signed-off bot commit, opens — or reuses — a stacked PR against the PR's own branch, and keeps a sticky comment on the original PR pointing at it. The author reviews the DDL diff and merges; that merge is a human push, so required checks re-run normally (a GITHUB_TOKEN push to the PR branch itself would trigger nothing and strand the PR on dead checks — the reason this is a stacked PR and not an auto-commit). Trust boundary: the gate job keeps contents: read and executes PR code; regen-pr holds the write tokens but never executes anything from the PR — it only checks out the branch as a git base and copies artifact files. Convergence terminates because a re-dump on an unchanged tree is byte-identical: once the regen commit is in, the next run finds no drift. Snapshot replacement is rm + cp, not overlay, so relations the code no longer produces disappear. Drift on a main push stays a red run — there is no PR branch to stack onto. connectors-ddl-reminder.yml is retired: it predates the gate (a sticky comment asking authors to remember what CI now enforces and repairs). Needs the repository setting "Allow GitHub Actions to create and approve pull requests" (Settings -> Actions); if it is off, `gh pr create` in regen-pr fails loudly with GraphQL: GitHub Actions is not permitted to create or approve pull requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
|
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe connectors DDL workflow now exposes drift status, uploads regenerated snapshots, and automatically creates or updates a stacked regeneration pull request for pull-request drift. Main-branch drift still fails the workflow. The previous reminder workflow was removed. ChangesConnector DDL regeneration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant connectors-ddl
participant regen-pr
participant GitHub
PullRequest->>connectors-ddl: validate connector DDL snapshot
connectors-ddl->>regen-pr: provide drift status and snapshot artifact
regen-pr->>GitHub: push regeneration branch
regen-pr->>GitHub: create or reuse stacked pull request
regen-pr->>GitHub: update sticky drift comment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 (3)
.github/workflows/connectors-ddl.yml (3)
228-231: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConfirm the persisted-credentials tradeoff is acceptable.
zizmor flags this checkout for not setting
persist-credentials: false(artipacked). The inline comment explains this is intentional because the push step reuses the checkout's credentials, and the job never executes PR-controlled code, which limits the practical exposure to this ephemeral runner. Given that context, this is a low-risk, accepted tradeoff rather than a defect, but it is worth confirming that no later step (for example a debug step, cache upload, or added tooling) is introduced that could expose.git/configfrom this checkout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/connectors-ddl.yml around lines 228 - 231, Confirm that the checkout credentials intentionally remain persisted for the subsequent push and that no later step executes PR-controlled code or exposes the checkout’s .git/config, including debug, cache, or tooling steps. Preserve the existing configuration and comment unless this review identifies such exposure.Source: Linters/SAST tools
211-227: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider guarding against overlapping
regen-prruns for the same PR.Each new push to the PR head branch can re-trigger
connectors-ddland, on drift,regen-pr. Without a concurrency group, two overlapping runs for the same PR can both reach the "Open or reuse the stacked PR" step at nearly the same time: both may find no existing PR viagh pr listand both attemptgh pr create, and only one succeeds, failing the other run. The--forcepush itself is safe, but the PR-creation race is not.Add a
concurrencyblock scoped to the PR to serialize regen-pr runs:Proposed fix
regen-pr: name: Open a regen PR on snapshot drift needs: connectors-ddl + concurrency: + group: regen-pr-${{ github.event.pull_request.number }} + cancel-in-progress: true if: >-🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/connectors-ddl.yml around lines 211 - 227, Add a workflow-level concurrency configuration to the regen-pr job, using a group keyed by the pull request number and cancelling or queuing runs according to the intended serialization behavior. Keep the existing regen-pr conditions and steps unchanged, ensuring runs for the same PR cannot concurrently reach PR creation.
196-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSticky comment is not cleared when drift resolves.
The design comment explains the regen-pr flow well, but there is no corresponding cleanup step in the
connectors-ddljob for the success path. If a later run finds no drift (for example the author fixed it directly, bypassing the stacked PR), the earlier sticky comment on the original PR still links to a now-stale or already-merged regen PR. Nothing marks it resolved or removes it.Add a small step to the
connectors-ddljob, conditioned onsuccess() && github.event_name == 'pull_request', that finds the<!-- connectors-ddl-regen-pr -->marker comment and updates it (or deletes it) to reflect that drift is resolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/connectors-ddl.yml around lines 196 - 210, Update the success path of the connectors-ddl job to add a step conditioned on success() && github.event_name == 'pull_request'. Locate the PR comment containing the <!-- connectors-ddl-regen-pr --> marker and update or delete it so previously reported snapshot drift is shown as resolved, including when no regen PR remains relevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/connectors-ddl.yml:
- Around line 261-271: Update the PR body constructed in the “Open or reuse the
stacked PR + sticky comment” step to remove the claim that the regen PR closes
automatically when #${PR_NUMBER} merges. Replace it with accurate guidance that
the PR may remain open after ${HEAD_REF} is deleted and must be updated against
the appropriate new base, without changing the surrounding PR creation behavior.
---
Nitpick comments:
In @.github/workflows/connectors-ddl.yml:
- Around line 228-231: Confirm that the checkout credentials intentionally
remain persisted for the subsequent push and that no later step executes
PR-controlled code or exposes the checkout’s .git/config, including debug,
cache, or tooling steps. Preserve the existing configuration and comment unless
this review identifies such exposure.
- Around line 211-227: Add a workflow-level concurrency configuration to the
regen-pr job, using a group keyed by the pull request number and cancelling or
queuing runs according to the intended serialization behavior. Keep the existing
regen-pr conditions and steps unchanged, ensuring runs for the same PR cannot
concurrently reach PR creation.
- Around line 196-210: Update the success path of the connectors-ddl job to add
a step conditioned on success() && github.event_name == 'pull_request'. Locate
the PR comment containing the <!-- connectors-ddl-regen-pr --> marker and update
or delete it so previously reported snapshot drift is shown as resolved,
including when no regen PR remains relevant.
🪄 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 Plus
Run ID: 539a4f3c-1bec-4bee-bab9-074b89283c92
📒 Files selected for processing (3)
.github/workflows/connectors-ddl-reminder.yml.github/workflows/connectors-ddl.ymlsrc/ingestion/scripts/bootstrap-db/README.md
💤 Files with no reviewable changes (1)
- .github/workflows/connectors-ddl-reminder.yml
| - name: Open or reuse the stacked PR + sticky comment | ||
| if: steps.commit.outputs.changed == 'true' | ||
| run: | | ||
| set -euo pipefail | ||
| url="$(gh pr list --head "${REGEN_BRANCH}" --base "${HEAD_REF}" --state open --json url -q '.[0].url // empty')" | ||
| if [[ -z "${url}" ]]; then | ||
| url="$(gh pr create --base "${HEAD_REF}" --head "${REGEN_BRANCH}" \ | ||
| --title "chore(connectors-ddl): regenerate the snapshot for ${HEAD_REF}" \ | ||
| --body "The connectors-ddl gate on #${PR_NUMBER} found the committed snapshot stale against what this branch's connectors + dbt + migrations actually produce. This PR carries the regenerated snapshot — review the DDL diff and merge it into \`${HEAD_REF}\`; the gate then re-runs and goes green. Closes automatically if #${PR_NUMBER} is merged without it (the base branch disappears).")" | ||
| fi | ||
| echo "regen PR: ${url}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does GitHub retarget or close a pull request when its base branch is deleted after a merge?
💡 Result:
No, GitHub does not automatically retarget (update the base branch of) an open pull request if its base branch is deleted [1][2]. Instead, if a pull request's base branch is deleted, the pull request is typically closed automatically [2]. It is important to distinguish this behavior from GitHub's automatic retargeting of "dependent" pull requests [1]: 1. Automatic Retargeting of Dependent Pull Requests: When a pull request is merged and its head branch is deleted, GitHub automatically checks for other open pull requests in the same repository that used that now-deleted branch as their base [3][4]. If it finds any, GitHub automatically updates their base branch to the merged pull request's new base branch [3][4]. This is a specific feature designed to maintain dependency chains [1][4]. 2. Deletion of a Base Branch: If you manually delete a branch that is currently serving as the base for an open pull request (not necessarily a head branch that was just merged), GitHub does not attempt to "guess" a new base and retarget the pull request [2]. Because the pull request no longer has a valid target branch, it is automatically closed [2]. If you find yourself in a situation where a pull request has been closed due to its base branch being deleted, you can restore the base branch, update the pull request's base to a different, active branch, and then re-open the pull request [2].
Citations:
- 1:
gh pr merge --delete-branch: GitHub does not update base of dependent PRs cli/cli#1168 - 2: Unable to change base of PR if base branch has been deleted isaacs/github#1557
- 3: https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/merging-a-pull-request
- 4: https://stackoverflow.com/questions/73558273/why-is-the-behavior-of-pr-base-branch-deletion-different-between-github-and-aws
🌐 Web query:
GitHub docs dependent pull requests automatically base updated when head branch merged deleted
💡 Result:
Yes, when a pull request is merged and its head branch is deleted, GitHub automatically updates any open pull requests in the same repository that were using that deleted branch as their base [1]. In such cases, GitHub re-targets those dependent pull requests to the merged pull request's base branch [1]. This behavior occurs automatically when the deletion is performed via the GitHub website (for example, using the "Delete branch" button after merging) or if the repository is configured to automatically delete head branches upon merge [1][2]. However, there are important limitations and nuances to this process: 1. CLI and API behavior: If the branch is deleted via the Git command line or certain API methods (like manually calling a delete reference endpoint) rather than through GitHub's standard merge/delete UI, GitHub may close the dependent pull requests instead of re-targeting their base branch [3][2]. 2. Stacked Pull Requests: GitHub now has native support for stacked pull requests [4][5]. In a stack, when a middle pull request is merged, the pull requests above it are automatically updated and re-targeted to the stack's base branch [4]. 3. Manual intervention: If a dependent pull request is not automatically updated, you can manually change its base branch through the GitHub interface by selecting "Edit" next to the pull request title [6]. In summary, GitHub's automated base branch update is designed to keep dependent workflows intact when branches are removed during standard repository cleanup, provided the deletion is handled through GitHub's native tools [1][2].
Citations:
- 1: https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/merging-a-pull-request
- 2: Mark pull request as depending on another isaacs/github#959
- 3:
gh pr merge --delete-branch: GitHub does not update base of dependent PRs cli/cli#1168 - 4: https://docs.github.com/en/pull-requests/get-started/about-stacked-prs
- 5: https://docs.github.com/en/pull-requests/reference/stacked-pull-requests
- 6: https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/changing-the-base-branch-of-a-pull-request
🌐 Web query:
GitHub dependent pull requests base branch automatically updated documentation
💡 Result:
GitHub does not have a native feature that automatically updates standard pull request branches with changes from the base branch on a continuous, background basis [1][2]. While GitHub provides tools to manage synchronization, the action remains manual or triggered by specific workflow configurations [1][3]. Key mechanisms for keeping pull requests up to date include: 1. Manual Updates via UI: On a pull request page, if the branch is out of sync and there are no merge conflicts, users can click the Update branch button [1][2]. This performs either a traditional merge (merging the base branch into the head branch) or a rebase, depending on the user's choice [1][4]. 2. Repository Settings: Administrators can enable the Always suggest updating pull request branches setting [3][5]. When enabled, this ensures the Update branch button is always available on the pull request page whenever the branch is behind, rather than only when required by branch protection rules [3][5]. 3. Specialized Handling for Dependabot: Dependabot attempts to automatically rebase its pull requests when they become out of date, though this is a best-effort behavior and not guaranteed to be instantaneous [6][7]. 4. Stacked (Dependent) Pull Requests: GitHub now supports stacked pull requests [8]. When you merge a pull request at the bottom of a stack, the remaining branches are automatically rebased so that they target the updated base branch [8]. 5. Automated Workflows: For general pull requests, teams often implement custom automation—such as GitHub Actions—to automatically update or "retarget" pull request branches when the base branch changes [9]. This is commonly done using the GitHub REST API (specifically pulls.updateBranch) to perform an automatic update [10]. In summary, GitHub provides the interface to request an update but relies on manual user action, explicit repository settings, or custom automation to perform the synchronization for standard pull requests [1][3][9].
Citations:
- 1: https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/keeping-your-pull-request-in-sync-with-the-base-branch
- 2: https://github.com/github/docs/blob/main/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/keeping-your-pull-request-in-sync-with-the-base-branch.md
- 3: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches
- 4: https://docs.github.com/en/enterprise-server@3.19/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/keeping-your-pull-request-in-sync-with-the-base-branch
- 5: https://github.blog/changelog/2022-02-02-more-ways-to-keep-your-pull-request-branch-up-to-date/
- 6: Document how and when Dependabot triggers auto rebase github/docs#42298
- 7: Rebase when "This branch is out-of-date with the base branch" is blocking merge dependabot/dependabot-core#13740
- 8: https://docs.github.com/en/pull-requests/get-started/about-stacked-prs
- 9: https://github.com/mathiasbourgoin/Sarek/blob/bd7a60ad6a705bd0c99772d111f2e689d1a523c0/.github/workflows/retarget-prs.yml
- 10: https://github.github.com/gh-aw/reference/safe-outputs-pull-requests/
Remove the “Closes automatically” statement from the regenerated PR body.
When ${HEAD_REF} is deleted after the original PR merges, GitHub does not close the stacked regen PR because its base branch now points at a deleted branch. Update the text to describe that the regenerated PR may remain open and needs to be updated against the new base, or verify the repository’s stacked PR/base-branch behavior before relying on GitHub actions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/connectors-ddl.yml around lines 261 - 271, Update the PR
body constructed in the “Open or reuse the stacked PR + sticky comment” step to
remove the claim that the regen PR closes automatically when #${PR_NUMBER}
merges. Replace it with accurate guidance that the PR may remain open after
${HEAD_REF} is deleted and must be updated against the appropriate new base,
without changing the surrounding PR creation behavior.
Semgrep flags mutable action tags across the repo and the standing decision has been to keep the repo-wide tag convention — for read-only jobs, where a repointed tag buys an attacker little. That reasoning does not hold for the regen-pr chain: the job holds contents: write and pull-requests: write, and the gate's regenerated-snapshot upload produces the artifact regen-pr commits into a branch. A repointed checkout, download-artifact or upload-artifact tag would execute inside — or feed — a write-capable job. Pin exactly those three to reviewed tag commits (checkout v5, download-artifact v8, upload-artifact v7); every read-only step keeps its tag per the existing convention, matching the repo's precedent of SHA-pinning only sensitive actions (docker/build-push-action). Resolves code-scanning findings 1253-1255. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
3 participants
Why
When the connectors-ddl gate finds snapshot drift, the failure message says "regenerate locally" — an instruction most contributors cannot follow:
dump-ddl.shneeds a fullbootstrap-db.shfirst, and the HubSpot/Salesforcediscoverinside it calls live APIs with credentials they do not have. The result is a red gate only a handful of people can turn green.What
New
regen-prjob inconnectors-ddl.yml, triggered only when the gate fails with drift on a PR:connectors-ddl-regenerated) and exposes adriftoutput (written before itsexit 1— step outputs survive the step's failure).regen-prreplacessrc/ingestion/scripts/connectors-ddl/with the artifact (rm+cp, not overlay — relations the code no longer produces must disappear), commits with a signed-off bot identity, and force-pushes toregen-ddl/<head-branch>.The author reviews the DDL diff in the stacked PR and merges it into their branch. That merge is a human push, so all required checks re-run normally on the new commit.
Why a stacked PR and not an auto-commit
GITHUB_TOKENpush into the PR branch triggers no workflows (GitHub's recursion guard) — the commit would land with dead checks and strand the PR on required statuses. A human merging the stacked PR avoids that without a PAT or a GitHub App.Trust boundary
The gate job keeps
contents: readand executes PR code (bootstrap runs repo scripts).regen-prholds the write tokens (contents: write,pull-requests: write) but never executes anything from the PR — it checks out the branch purely as a git base and copies artifact files. Write tokens and PR-code execution never coexist in one job.Convergence terminates: a re-dump on an unchanged tree is byte-identical (the property the gate is built on), so once the regen commit is merged the next run finds no drift and
regen-prstays skipped. Fork PRs never reach it (the gate itself is skipped there). Drift on amainpush stays a plain red run — there is no PR branch to stack onto.Lifecycle: the regen branch is bot-owned and force-pushed on every drifting run, so the stacked PR always reflects the branch as of the last completed gate run; when the original PR merges and its branch is deleted, GitHub auto-closes the stacked PR.
Retired:
connectors-ddl-reminder.ymlIt predates the gate — a sticky comment on every
src/ingestion/**PR asking authors to remember to regenerate. The gate now enforces what the reminder asked for, and the regen PR repairs it; a parallel "don't forget" comment is noise.Prerequisite
The repository setting "Allow GitHub Actions to create and approve pull requests" (Settings → Actions → General) must be enabled — I could not verify it via API (403 on the admin endpoint). If it is off,
regen-prfails loudly atgh pr createwith "GitHub Actions is not permitted to create or approve pull requests"; nothing is silently skipped.Verification
run:block passesbash -n.driftoutput / artifact plumbing mirrors the existing failure path (Upload the drift diffalready runs on failure and proved artifacts upload from a red gate run).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation