feat(bots): onboard reviewer-bot to pinned databricks-bot-engine#862
feat(bots): onboard reviewer-bot to pinned databricks-bot-engine#862eric-wang-1990 wants to merge 6 commits into
Conversation
|
🚧 Held as draft pending an engine-side fix. This onboarding is blocked on databricks/databricks-bot-engine#97: the reusable
The engineer-bot onboarding (bug-fix flow, PAT-free via its own job) will be Prerequisite for either bot: the review/engineer GitHub App must be installed on |
There was a problem hiding this comment.
Verdict: 1 Medium · 2 Low
Onboarding PR — workflows, local composite action, and .bot prompts/config only; no connector source touched. Looks mostly solid, but one stray dead action file at a doubly-nested path (setup-jfrog/setup-jfrog/) should be removed, and two engine-output-name assumptions in engineer-bot.yml (no_change case arm, coverage_pr_url) are worth confirming against the pinned engine since a mismatch would silently misreport outcomes on the tracking issue. Nit: the reviewer-bot.yml header comment still says the App token is passed "as engine-pat to install-bot-engine", but the local composite's input is engine-token — stale wording worth fixing.
✅ Live verification progress (reviewer)After installing both bot Apps on
This validates the whole onboarding chain: local-composite install (no cross-repo |
There was a problem hiding this comment.
Verdict: 1 High · 1 Medium · 2 Low
Mostly workflow/config plumbing for onboarding the review + engineer bots; no product (src/) code changes. Main concerns: a TEMP e2e workflow the PR itself marks DO-NOT-MERGE is still in the diff (F1), a stray doubly-nested setup-jfrog/setup-jfrog action that nothing references and duplicates the real one (F2), plus a couple of comment/outcome-string mismatches (F3, F4).
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Onboarding PR is almost entirely CI/workflow/config with no connector src/ changes, so risk is scoped to the GitHub Actions. Two issues worth addressing before merge: a self-declared TEMP "DO NOT MERGE" e2e workflow that is still in the diff (medium), and an orphaned double-nested setup-jfrog/setup-jfrog/action.yml duplicate that nothing references (low). Nit: the reviewer-bot.yml header comment still says the token is passed "as engine-pat to install-bot-engine", but the composite input is engine-token — stale wording, worth a one-line fix.
External consumers cannot `uses:` the internal engine's actions/reusable workflows cross-repo (GitHub: "not found"). This local composite instead pip-installs the pinned engine over HTTPS with a short-lived, engine-scoped GitHub App token (no stored BOT_ENGINE_PAT), routing pip/npm through the internal JFrog mirror for the egress-blocked protected runner. Shared by all four bot workflows. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Onboard the PR reviewer to the upstream engine (pinned SHA b6205fb), replacing the previously-vendored scripts/reviewer_bot. Own-job workflows: mint the review-bot App token, install the engine via the local composite (engine-scoped App token, no PAT), run reviewer_bot.run_review / .followup. Trigger: every non-fork PR. .bot/prompts/review/system.md is repo-specific ADDITIVE guidance appended to the engine-owned base prompt. Verified live — posted a review as peco-review-bot on this PR. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Add the engineer bot: author (label an issue) reproduces a bug with a failing test, fixes the connector, opens a PR; followup addresses review comments on engineer-bot-labeled PRs. Own jobs (they build/run the connector): mint tokens, setup-poetry for connector deps (JFrog mirror, egress-safe self-verify env), install the engine via the local composite (engine-scoped App token, no PAT), run engineer_bot.run. .bot/config.yaml (flow: bug-fix, poetry pytest allowlist) + engineer/engineer-followup prompts, adapted for src/databricks/sql/ + mocked tests/unit. Verified live — the author agent ran end-to-end to a correct structured outcome. Design + first-consumer findings in docs/superpowers/specs. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
✅ Ready for review — both bots onboarded and verified liveSquashed to 3 signed-off commits (DCO green). Both bots run against the upstream Verified end-to-end:
Key design point (first clean consumer): an external repo cannot Superseded: the vendored reviewer-bot stack (#832/#833/#834) — closed. |
There was a problem hiding this comment.
Verdict: 1 Medium
Mostly workflow/config plumbing that reads carefully and is well-commented. One medium concern: the engineer-bot issue-comment case matches no_change, while the prompt and the design doc's live-verification note both use the token no_change_needed — a mismatch that would mislabel a real no-change outcome as a failure. Worth confirming the engine's exact outcome-enum string before merge.
…kflows The four bot workflows duplicated ~4 setup steps (mint bot token, mint engine-scoped token, Setup Node, install engine). Factor them into a local ./.github/actions/bot-prelude composite (inputs: app-id/private-key + engine-ref; output: token). Each workflow keeps only what genuinely differs: its checkout, Setup Python (reviewer — interpreter only) vs setup-poetry (engineer — connector deps for pytest self-verify), and its run/publish tail. bot-prelude calls the existing local install-bot-engine. Behavior unchanged; token refs now come from steps.prelude.outputs.token. Headers refreshed to match. Engine feedback on the duplicated install-bot-engine noted in issue #104. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid infra onboarding PR — no security or correctness issues in the token-minting / gating logic, which is carefully done. One medium concern: the issue-comment case in engineer-bot.yml matches no_change but the author actually emits no_change_needed (per this PR's own prompt and design doc), so no-change runs post a misleading "could not complete a fix" comment. Plus one low: the coverage_pr_url output name looks like a copy-paste from a coverage flow and should be confirmed against the engine's bug-fix publish output.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall — this is CI/workflow + .bot/ prompt plumbing with no src/ changes, so the correctness/test axes are limited. Vendored-code removal is clean (grep confirms no dangling scripts.reviewer_bot/scripts.shared references), token handling in install-bot-engine is careful (job-local git extraheader, masking, SHA pin validation), and the security gates are sound. One low-severity gate inconsistency noted inline.
Nits (no inline anchor needed): (1) install-bot-engine/action.yml hardcodes sdk-version: 0.2.102 / cli-version: 2.1.61 defaults that must be manually kept in sync with the engine at the pinned SHA — the comments flag this, but it's a standing drift risk worth a periodic check. (2) The JFrog OIDC block relies on the friendly ACCESS_TOKEN empty-check for error messaging, but under set -euo pipefail a missing ACTIONS_ID_TOKEN_REQUEST_TOKEN would abort at the curl | jq pipe before that check fires; since a
[...truncated to keep verdict scannable]
…re correct Two review findings (raised repeatedly) assumed the workflow was wrong; both are false positives verified against the engine source. Add clarifying comments so they don't recur: - `case ... no_change)`: the engine maps the agent's `no_change_needed` to the emitted $GITHUB_OUTPUT value `no_change` (run_author.py), so the arm is correct. - `coverage_pr_url`: engineer_bot.publish emits this key for ALL flows (legacy name), not just coverage — correct for bug-fix too. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall — a clean, well-documented bot-onboarding PR (workflow YAML, local composite actions, prompts, config; no product code touched). Two low-severity consistency notes: the bot-prelude engine-repo input isn't honored by the engine-token mint step, and the initial reviewer workflow lacks the per-PR concurrency group its siblings have. Security-sensitive paths (untrusted issue/PR input via files + env heredocs, digits-only validation, PAT-free git-auth masking, fork gates) are handled carefully.
…n, reviewer concurrency Three low-severity review findings (all verified real against the branch): - engineer-bot.yml: add `github.event.sender.type != 'Bot'` to the author `if:`, matching the followup gate — defense-in-depth since a GitHub App with `issues: write` could apply the trigger label without a human. - bot-prelude: derive owner/name from the `engine-repo` input for the engine-scoped token mint instead of hardcoding databricks/databricks-bot-engine, so the overridable input is honored (a fork/rename gets a correctly-scoped token rather than a confusing auth failure). - reviewer-bot.yml: add a `concurrency:` group (cancel-in-progress) so rapid pushes to a PR collapse to one review run, matching the other three workflows. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-documented onboarding PR. Vendored-bot deletions are confirmed (no dangling references), the four workflows are internally consistent (shared bot-prelude/install-bot-engine composites, PAT-free App-auth, consistent MODEL_ENDPOINT, bot-login prefixes matching .bot/config.yaml), and the untrusted-input handling (digits-only issue/PR validation, heredoc-delimiter collision guard, issue body via file not shell) is careful. One low-confidence note about the workflow_dispatch checkout ref for the reviewer, posted inline.
| persist-credentials: false | ||
|
|
||
| # The reviewer reads code, it doesn't run the connector — so it needs only | ||
| # a Python interpreter (to run the engine), NOT the connector's deps. |
There was a problem hiding this comment.
🔵 Low — On the workflow_dispatch path this job checks out the default branch (the checkout step uses no ref:, so on dispatch it resolves to the repo default branch rather than the target PR), while HEAD_SHA for the run is separately resolved via gh pr view "$RAW_PR" --json headRefOid. On the pull_request trigger the default ref is the PR merge ref (correct), but on a manual dispatch of an arbitrary pr_number the working tree and HEAD_SHA diverge — the reviewer's read_paths/grep would then read default-branch code while the diff/anchors reference the PR head.
If run_review fetches/reads content purely via the GitHub API keyed on HEAD_SHA, this is harmless; if it relies on the local checkout for context, a dispatched review targets the wrong tree. Worth confirming against the engine's run_review behavior, or checking out HEAD_SHA explicitly on the dispatch path. (Low: the automatic pull_request path — the normal case — is correct; only manual dispatch is affected.)
Summary
Onboards both bots on
databricks-sql-pythonto the upstreamdatabricks/databricks-bot-engine, pinned to engine SHAb6205fband fullyPAT-free (App-auth). This repo is the first clean consumer of the engine;
onboarding friction is captured as feedback (see the design doc + engine #97).
Design doc:
docs/superpowers/specs/2026-07-14-onboard-bot-engine-design.md.Reviewer bot (was vendored → now engine reusables, App-auth)
scripts/reviewer_bot+scripts/sharedwith thincallers of the engine's
reviewer-bot.reusable.yml/-followup.reusable.yml.engine-auth: app+ noengine-pat: the reusable mints an engine-scoped Apptoken from the review-bot App creds (engine PR sqlalchemy > 2.0.0 #100).
Engineer bot (new — bug-fix flow)
engineer-bot.yml(author, label an issue) +engineer-bot-followup.yml(reply on an
engineer-bot-labeled PR). Own jobs (they build/run theconnector), each minting an engine-scoped App token for a PAT-free REF install.
environment: azure-prod; explicitpoetry installso the agent can runpoetry run python -m pytest tests/unitto self-verify its red→green fix..bot/config.yaml+ engineer prompts, ported from the engine dogfood andrewritten for the connector (
src/databricks/sql/, mockedtests/unit).Secrets (all provisioned)
REVIEW_BOT_APP_ID/PRIVATE_KEY,ENGINEER_BOT_APP_ID/PRIVATE_KEY,DATABRICKS_HOST,DATABRICKS_TOKEN. NoBOT_ENGINE_PAT— App-authreplaces it.
The review-bot and engineer-bot GitHub Apps must be installed on
databricks-bot-enginewithcontents: read, or the App-auth engine installfails for both bots.
Verification
.bot/config.yamlparse;.bot/tree complete; vendoredcode fully removed with no dangling references.
First live signals: a test PR (reviewer) and a labelled throwaway issue
(engineer author). Will report actual outcomes, not assume success.
This pull request and its description were written by Isaac.