chore: automate Zoekt submodule sync - #1560
Conversation
WalkthroughAdds GitHub Actions automation to update the ChangesZoekt synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant GitHubApp
participant ZoektRemote
participant SourcebotRepository
participant GitHubPullRequestAPI
GitHubActions->>GitHubApp: Generate authentication token
GitHubActions->>SourcebotRepository: Prepare automation branch
GitHubActions->>ZoektRemote: Fetch and validate target commit
GitHubActions->>SourcebotRepository: Update submodule and changelog
GitHubActions->>GitHubPullRequestAPI: Create or update pull request
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.github/scripts/test-zoekt-sync.sh (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRename
test-zoekt-sync.shto camelCase.
test-zoekt-sync.shdoes not use a lowercase camelCase file name. Rename it totestZoektSync.shand update.github/workflows/test.ymlline 22.As per coding guidelines, files must use camelCase names starting with a lowercase letter.
🤖 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/scripts/test-zoekt-sync.sh at line 1, Rename test-zoekt-sync.sh to testZoektSync.sh, then update the corresponding reference in the test workflow so it invokes the renamed script.Source: Coding guidelines
88-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the divergent-history guard directly.
$divergent_shais not reachable fromorigin/main. The updater exits at its reachability check, so this case does not test its divergent-history check. Pinvendor/zoektto$divergent_sha, commit that gitlink, then request$second_shaand assert the divergent-history error.🤖 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/scripts/test-zoekt-sync.sh around lines 88 - 104, Update the divergent-history test around the "$update_script" invocation: after fetching divergent, pin the vendor/zoekt gitlink to "$divergent_sha" and commit that change in "$sourcebot_test", then invoke the updater with "$second_sha" so reachability succeeds but the history is divergent. Keep the assertion focused on the divergent-history error message.
82-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck the staged index after a stale update.
git diff --quietchecks only the worktree. A stale update can leave a stagedvendor/zoektgitlink and still pass this test. The workflow later checks the index withgit diff --cached --quietand can commit that change. Check both the worktree and the staged index.Proposed test update
git -C "$sourcebot_test" diff --quiet || \ fail "a stale event should leave the worktree unchanged" +git -C "$sourcebot_test" diff --cached --quiet || \ + fail "a stale event should leave the index unchanged"🤖 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/scripts/test-zoekt-sync.sh around lines 82 - 86, Update the stale-event assertions in the test flow around current_sha so they verify both the worktree and staged index are unchanged: retain git diff --quiet and also check git diff --cached --quiet, failing with a clear stale-event message if either detects changes.
🤖 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/scripts/add-zoekt-sync-changelog-entry.sh:
- Line 1: Rename the add-zoekt-sync-changelog-entry.sh script to
addZoektSyncChangelogEntry.sh, then update every workflow or repository
reference invoking the old path to use the new camelCase filename.
- Around line 90-97: Update the awk END block in
add-zoekt-sync-changelog-entry.sh to handle an Unreleased section that reaches
EOF without a Changed subsection: when in_unreleased && !found_changed, insert
the ### Changed heading and entry before the existing failure check. Preserve
the current flush_changed_section behavior for sections that already contain
changes and retain the status-2 exit for genuinely missing or unsuccessful
insertions.
- Around line 21-22: Update the temporary-file handling in the changelog update
flow to create the file beside CHANGELOG.md and capture the existing target file
mode before replacement. Before the mv operation, apply that preserved mode to
the temporary file so the replacement retains CHANGELOG.md’s group and other
permissions.
In @.github/scripts/update-zoekt-submodule.sh:
- Line 1: Rename .github/scripts/update-zoekt-submodule.sh to
updateZoektSubmodule.sh and .github/workflows/sync-zoekt.yml to syncZoekt.yml,
then update every workflow and test reference to both paths; no direct content
change is required beyond references.
In @.github/workflows/sync-zoekt.yml:
- Around line 103-107: Update the sync workflow around the staged gitlink check
and rebase handling to track branch-HEAD changes separately from vendor/zoekt
changes. When the rebase produces a new branch HEAD even though the gitlink is
unchanged, push the rebased branch using the existing lease; only report no
changes and skip pushing when neither the branch nor gitlink changed.
- Around line 34-39: Update the create-github-app-token configuration to add
explicit permission-contents: write and permission-pull-requests: write inputs,
restricting the token to the permissions required for branch pushes and pull
request creation or updates.
In `@CHANGELOG.md`:
- Around line 13-14: Rewrite the changelog entry under “Changed” as a complete
single sentence, beginning with an appropriate past-tense action such as
“Added,” while preserving the existing description and pull request link.
---
Nitpick comments:
In @.github/scripts/test-zoekt-sync.sh:
- Line 1: Rename test-zoekt-sync.sh to testZoektSync.sh, then update the
corresponding reference in the test workflow so it invokes the renamed script.
- Around line 88-104: Update the divergent-history test around the
"$update_script" invocation: after fetching divergent, pin the vendor/zoekt
gitlink to "$divergent_sha" and commit that change in "$sourcebot_test", then
invoke the updater with "$second_sha" so reachability succeeds but the history
is divergent. Keep the assertion focused on the divergent-history error message.
- Around line 82-86: Update the stale-event assertions in the test flow around
current_sha so they verify both the worktree and staged index are unchanged:
retain git diff --quiet and also check git diff --cached --quiet, failing with a
clear stale-event message if either detects changes.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d86fcc3-8305-463c-a610-a8dd64aa8ce3
📒 Files selected for processing (6)
.github/scripts/add-zoekt-sync-changelog-entry.sh.github/scripts/test-zoekt-sync.sh.github/scripts/update-zoekt-submodule.sh.github/workflows/sync-zoekt.yml.github/workflows/test.ymlCHANGELOG.md
| @@ -0,0 +1,106 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the script to use camelCase.
Rename this file to addZoektSyncChangelogEntry.sh. Update the workflow invocation that uses the current path.
As per coding guidelines, files should use camelCase names starting with a lowercase letter.
🤖 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/scripts/add-zoekt-sync-changelog-entry.sh at line 1, Rename the
add-zoekt-sync-changelog-entry.sh script to addZoektSyncChangelogEntry.sh, then
update every workflow or repository reference invoking the old path to use the
new camelCase filename.
Source: Coding guidelines
| temporary_file=$(mktemp) | ||
| trap 'rm -f "$temporary_file"' EXIT |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the original file mode when replacing CHANGELOG.md.
Line 21 creates a mode-0600 temporary file. Line 104 replaces CHANGELOG.md with it, so the working-tree changelog loses its existing group and other read permissions. Create the temporary file beside the target and preserve the target mode before mv.
Also applies to: 104-105
🤖 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/scripts/add-zoekt-sync-changelog-entry.sh around lines 21 - 22,
Update the temporary-file handling in the changelog update flow to create the
file beside CHANGELOG.md and capture the existing target file mode before
replacement. Before the mv operation, apply that preserved mode to the temporary
file so the replacement retains CHANGELOG.md’s group and other permissions.
| @@ -0,0 +1,48 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename the new files to camelCase.
Both new paths use kebab-case. Rename the files and update all workflow and test references.
.github/scripts/update-zoekt-submodule.sh#L1-L1: Rename toupdateZoektSubmodule.sh..github/workflows/sync-zoekt.yml#L1-L1: Rename tosyncZoekt.yml.
As per coding guidelines, **/*: “Files should use camelCase names starting with a lowercase letter.”
📍 Affects 2 files
.github/scripts/update-zoekt-submodule.sh#L1-L1(this comment).github/workflows/sync-zoekt.yml#L1-L1
🤖 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/scripts/update-zoekt-submodule.sh at line 1, Rename
.github/scripts/update-zoekt-submodule.sh to updateZoektSubmodule.sh and
.github/workflows/sync-zoekt.yml to syncZoekt.yml, then update every workflow
and test reference to both paths; no direct content change is required beyond
references.
Source: Coding guidelines
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts/testZoektSync.sh:
- Around line 170-179: Strengthen the tests in .github/scripts/testZoektSync.sh
at lines 170-179 by asserting that processing an empty Unreleased section
creates a “### Changed” heading and retains the generated Zoekt PR entry. At
lines 188-195, update the workflow verification to assert that syncZoekt.yml
invokes .github/scripts/addZoektSyncChangelogEntry.sh with "$pr_number".
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff4a9134-fa39-45b1-a4a5-9faa8fa4cf06
📒 Files selected for processing (6)
.github/scripts/addZoektSyncChangelogEntry.sh.github/scripts/testZoektSync.sh.github/scripts/updateZoektSubmodule.sh.github/workflows/syncZoekt.yml.github/workflows/test.ymlCHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- CHANGELOG.md
| empty_changelog_fixture="$test_root/EMPTY_CHANGELOG.md" | ||
| cat > "$empty_changelog_fixture" <<'EOF' | ||
| # Changelog | ||
|
|
||
| ## [Unreleased] | ||
| EOF | ||
| CHANGELOG_PATH="$empty_changelog_fixture" "$changelog_script" 43 | ||
| assert_contains "$empty_changelog_fixture" \ | ||
| "- Updated the bundled Zoekt version. [#43](https://github.com/sourcebot-dev/sourcebot/pull/43)" \ | ||
| "the changelog helper should handle an empty Unreleased section at EOF" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the generated-PR changelog contract.
The fixture does not verify that the helper creates ### Changed. The workflow checks do not verify that it invokes addZoektSyncChangelogEntry.sh. A regression can produce a malformed changelog or omit the generated PR entry without failing this test.
.github/scripts/testZoektSync.sh#L170-L179: Assert that### Changedis present after processing the empty Unreleased section..github/scripts/testZoektSync.sh#L188-L195: Assert thatsyncZoekt.ymlinvokes.github/scripts/addZoektSyncChangelogEntry.sh "$pr_number".
Proposed test coverage
CHANGELOG_PATH="$empty_changelog_fixture" "$changelog_script" 43
+assert_contains "$empty_changelog_fixture" \
+ "### Changed" \
+ "the changelog helper should create a Changed section"
assert_contains "$empty_changelog_fixture" \
"- Updated the bundled Zoekt version. [`#43`](https://github.com/sourcebot-dev/sourcebot/pull/43)" \
"the changelog helper should handle an empty Unreleased section at EOF"
+
+assert_contains "$workflow" \
+ '.github/scripts/addZoektSyncChangelogEntry.sh "$pr_number"' \
+ "the receiver should add the generated pull request to the changelog"📝 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.
| empty_changelog_fixture="$test_root/EMPTY_CHANGELOG.md" | |
| cat > "$empty_changelog_fixture" <<'EOF' | |
| # Changelog | |
| ## [Unreleased] | |
| EOF | |
| CHANGELOG_PATH="$empty_changelog_fixture" "$changelog_script" 43 | |
| assert_contains "$empty_changelog_fixture" \ | |
| "- Updated the bundled Zoekt version. [#43](https://github.com/sourcebot-dev/sourcebot/pull/43)" \ | |
| "the changelog helper should handle an empty Unreleased section at EOF" | |
| empty_changelog_fixture="$test_root/EMPTY_CHANGELOG.md" | |
| cat > "$empty_changelog_fixture" <<'EOF' | |
| # Changelog | |
| ## [Unreleased] | |
| EOF | |
| CHANGELOG_PATH="$empty_changelog_fixture" "$changelog_script" 43 | |
| assert_contains "$empty_changelog_fixture" \ | |
| "### Changed" \ | |
| "the changelog helper should create a Changed section" | |
| assert_contains "$empty_changelog_fixture" \ | |
| "- Updated the bundled Zoekt version. [`#43`](https://github.com/sourcebot-dev/sourcebot/pull/43)" \ | |
| "the changelog helper should handle an empty Unreleased section at EOF" |
| empty_changelog_fixture="$test_root/EMPTY_CHANGELOG.md" | |
| cat > "$empty_changelog_fixture" <<'EOF' | |
| # Changelog | |
| ## [Unreleased] | |
| EOF | |
| CHANGELOG_PATH="$empty_changelog_fixture" "$changelog_script" 43 | |
| assert_contains "$empty_changelog_fixture" \ | |
| "- Updated the bundled Zoekt version. [#43](https://github.com/sourcebot-dev/sourcebot/pull/43)" \ | |
| "the changelog helper should handle an empty Unreleased section at EOF" | |
| assert_contains "$workflow" \ | |
| 'permission-pull-requests: write' \ | |
| "the receiver should restrict its app token to required permissions" | |
| assert_contains "$workflow" \ | |
| '--force-with-lease=' \ | |
| "the receiver should protect updates to its stable automation branch" | |
| assert_contains "$workflow" \ | |
| '.github/scripts/updateZoektSubmodule.sh "$ZOEKT_SHA"' \ | |
| "the receiver should update the Zoekt submodule" | |
| assert_contains "$workflow" \ | |
| '.github/scripts/addZoektSyncChangelogEntry.sh "$pr_number"' \ | |
| "the receiver should add the generated pull request to the changelog" |
📍 Affects 1 file
.github/scripts/testZoektSync.sh#L170-L179(this comment).github/scripts/testZoektSync.sh#L188-L195
🤖 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/scripts/testZoektSync.sh around lines 170 - 179, Strengthen the
tests in .github/scripts/testZoektSync.sh at lines 170-179 by asserting that
processing an empty Unreleased section creates a “### Changed” heading and
retains the generated Zoekt PR entry. At lines 188-195, update the workflow
verification to assert that syncZoekt.yml invokes
.github/scripts/addZoektSyncChangelogEntry.sh with "$pr_number".
Removed the automation for updating the bundled Zoekt version in the CHANGELOG.
|
@brendan-kellam your pull request is missing a changelog! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 85e3602. Configure here.
| git rebase origin/main | ||
| if [[ "$(git rev-parse HEAD)" != "$remote_sha" ]]; then | ||
| branch_changed=true | ||
| fi |
There was a problem hiding this comment.
Rebase conflicts block Zoekt syncs
Medium Severity
When an open sync PR is reused, the workflow rebases automation/sync-zoekt onto origin/main. That branch includes a CHANGELOG.md commit, while nearly every merge to main also edits Unreleased changelog sections. Those edits land at the same Added/Removed boundary where the sync entry is inserted, so the rebase commonly conflicts, the job fails before pushing, and later Zoekt dispatches keep hitting the same conflict until the branch is repaired by hand.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 85e3602. Configure here.
1 participant
Summary
Companion dispatcher: sourcebot-dev/zoekt#20
Validation
Configuration
Requires SOURCEBOT_SYNC_APP_ID and SOURCEBOT_SYNC_APP_PRIVATE_KEY Actions secrets. The app should be installed only on sourcebot-dev/sourcebot with Contents and Pull requests write access.
Note
Medium Risk
Automation can push branches and open PRs with elevated app credentials, but submodule updates are constrained to validated forward commits on Zoekt main rather than arbitrary gitlinks.
Overview
Adds automated sync of
vendor/zoektwhen Zoekt merges land upstream, plus manual retries viaworkflow_dispatch.A new
syncZoekt.ymlworkflow listens forrepository_dispatch(zoekt-pr-merged) or manual inputs, uses a GitHub App token to push toautomation/sync-zoekt, and opens or updates a single PR that bumps the submodule.updateZoektSubmodule.shonly allows forward moves on Zoektmain(rejects stale SHAs, non-main commits, and divergent histories).addZoektSyncChangelogEntry.shinserts an Unreleased Changed line linking to the sync PR without duplicating entries.testZoektSync.shexercises those scripts and workflow expectations;test.ymlruns it in the vulnerability-automation job.Reviewed by Cursor Bugbot for commit 85e3602. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests