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 10 commits into
Conversation
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
|
🚧 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 |
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
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.
Adopt the engine's Issue-Type flow selection (engine PR #107, merged) instead of running a fixed config-default flow: - engineer-bot.yml ctx step: fetch the issue via `gh api repos/{o}/{r}/issues/{n}` (REST carries `.type.name`; `gh issue view --json type` does not), reject PR numbers via `.pull_request`, and derive `--flow` from a pinned Type→flow case (`Bug` ⇒ bug-fix, `Feature` ⇒ enhancement; any other/none ⇒ omit). - author step: pass `--flow` only when derived (empty would fail the CLI's `choices`); omitting it keeps the `.bot/config.yaml` `flow: bug-fix` default. - Bump the pinned engine SHA b6205fb → d24ca21 (includes #107) across all four bot workflows so the consumer runs the engine version that ships this behavior. Note: Issue Types are org-level; databricks-sql-python is org-owned, so this works. A no-type / other-type issue falls through to the bug-fix config default. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Onboarding-only PR: four bot workflows + two local composite actions + .bot/ config/prompts, no connector source touched. Auth (engine-scoped App token via git extraheader, PAT-free), input hardening (digits-only validation, untrusted issue body → file, heredoc-delimiter collision check), and the fork/open-PR/label gates all look sound. One low finding: the actually-pinned engine SHA (d24ca217...) matches nowhere in the PR description or design doc (which cite b6205fb / d780b2d), and is duplicated across all four workflows.
…step Review follow-up (F2): the workflows pin engine d24ca21 but the design doc still said b6205fb, so a reviewer couldn't confirm which engine is installed. - Update the design doc's operative "Pinned to" line to d24ca21 (post-#107, issue-Type flow selection), noting the earlier b6205fb draft. - Add a comment at each of the four engine-ref pin sites noting the SHA is duplicated across all four bot workflows and must be bumped in lockstep. 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 — bot/CI onboarding only, no connector source touched; auth flow, fork gates, and untrusted-input handling are careful and well-commented. One low inline note: engineer-bot-followup.yml passes PR_BASE_SHA but (unlike reviewer-bot-followup.yml) never ensures the base commit is fetched. Meta nit for the author, not inline: the PR description states the engine is pinned to SHA b6205fb, but all four workflows pin d24ca2171d191a652a67f4f43995f0959c9a5791 (the design doc's later update confirms b6205fb was an earlier draft) — update the description so reviewers/maintainers see the engine version that actually runs.
Review follow-up (F1): reviewer flagged that engineer-bot-followup passes PR_BASE_SHA without the base-fetch step the reviewer follow-up has. Verified against the engine: the engineer follow-up (followup_runner.py / run.py) never reads PR_BASE_SHA — unlike the reviewer, it does no `git rev-list base..head` anchor verification. So it was dead env, not a missing-base risk. Remove it rather than adding an unused base-fetch step. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Solid, carefully-written bot-onboarding infra — vendored code is fully removed (no dangling refs outside the design doc), setup-poetry supplies the interpreter the engine install needs, and security handling (40-char SHA pin validation, masked tokens, git extraheader instead of in-URL creds, argv-prefix bash allowlist, untrusted-input-to-file) is thorough. One low error-handling nit inline. Nits (summary-only): the PR title/description states the engine is pinned to b6205fb, but all four workflows pin d24ca2171d191a652a67f4f43995f0959c9a5791 (the description's later "Update" sections clarify b6205fb was an earlier draft — worth aligning the title). Similarly, the design doc's "Engine pin" section still names d780b2da… while its own later updates use d24ca217…; harmless but stale.
Review follow-up: the JFrog OIDC exchange only validated ACCESS_TOKEN, so a failed ID_TOKEN mint (missing id-token:write, metadata hiccup → empty or the string "null") was still POSTed as subject_token and surfaced one step later as the misleading "Could not extract JFrog access token". Add an empty/null guard right after the OIDC curl so the diagnostic points at the real cause. Fails closed either way; this just improves the error message. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, carefully-gated onboarding PR — no connector source changes, and the workflow/token/security logic is well-reasoned and thoroughly commented. One medium consistency gap: the .bot/config.yaml templates (name, branch_prefix, pr_title_template, commit_message_template) are all bug-fix-only, but engineer-bot.yml now routes Feature issues to the enhancement flow, which would emit fix:/bugfix-labelled PRs, commits, and branches for enhancements.
Summary
Onboards both bots on
databricks-sql-pythonto the upstreamdatabricks/databricks-bot-engine, pinned to engine SHAd24ca21(post-#107 —author flow selected from the issue Type) and fully PAT-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 — flow from issue Type)
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.
Bug⇒bug-fix,Feature⇒enhancement, any other type / none ⇒ the.bot/config.yamlflow: bug-fixdefault.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.
Prerequisite (engine-repo admin) — ✅ done
The review-bot and engineer-bot GitHub Apps are installed on
databricks-bot-enginewithcontents: read(required, else the App-authengine install fails for both bots).
Verification
.bot/config.yamlparse;.bot/tree complete; vendoredcode fully removed with no dangling references.
engine-auth: appinstall path validated end-to-end (engine PR sqlalchemy > 2.0.0 #100).workflow_dispatch(manual) reviewer path theexplore-tool read root falls back to the default branch — the automatic
pull_requestpath (the normal case) is unaffected. Tracked separately.This pull request and its description were written by Isaac.