feat(bots): onboard reviewer + engineer bots to databricks-bot-engine#464
feat(bots): onboard reviewer + engineer bots to databricks-bot-engine#464eric-wang-1990 wants to merge 4 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Onboards both engine bots to databricks-sql-nodejs, PAT-free (App-auth), pinned to engine SHA d05dcb11. Single PR, adapted for the TypeScript/npm toolchain. Reviewer bot (read-only): fires on PR open/update + workflow_dispatch; Python interpreter + JFrog + engine install; posts inline findings. No existing Claude automation in this repo, so it's the sole autonomous reviewer — no conflict. Engineer bot (bug-fix flow, live E2E discipline): on a maintainer `engineer-bot` label. Installs connector deps (npm ci via JFrog mirror; never npm install, which would bump package-lock to v3 and fail CI lint), then requires a live tests/e2e repro against a warehouse over the Thrift backend before fixing. Follow-up runs the mocked npm test unit suite only (no live creds). .bot/: engineer config + prompts for TS/Node; reviewer additive prompt. denied_subpaths blocks .github/, native/ (napi kernel binding), thrift/ (generated stubs). tests/e2e/utils/config.ts gains a token fallback so E2E_ACCESS_TOKEN survives the engine's env scrub (the *_TOKEN vars are stripped from the agent subprocess; without it config.ts process.exit(1)s the whole suite); env var still wins, so normal CI/local dev is unchanged. Two local composites (bot-prelude, install-bot-engine) mirror the sql-python / odbc onboarding — the engine can't be `uses:`-d cross-repo, so it's pip-installed in REF mode. Runs on the protected runner group + JFrog mirror like the other internal drivers. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
3211be6 to
98d93d5
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
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: 3 High
Onboarding scaffolding looks solid overall; the config.ts token fallback is correctly gated so normal CI/local dev is unchanged. One high concern repeated across three files: DATABRICKS_HOST/DATABRICKS_TOKEN are azure-prod-environment secrets (per engineer-bot.yml's own comment and main.yml/kernel-e2e.yml), but the reviewer, reviewer-followup, and engineer-followup jobs reference them without declaring environment: azure-prod, so they'll resolve empty and break the model call.
| PR_NUMBER: ${{ steps.inputs.outputs.pr_number }} | ||
| HEAD_SHA: ${{ steps.inputs.outputs.head_sha }} | ||
| DRY_RUN: ${{ steps.inputs.outputs.dry_run }} | ||
| MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations |
There was a problem hiding this comment.
🟠 High — This job references secrets.DATABRICKS_HOST and secrets.DATABRICKS_TOKEN to build MODEL_ENDPOINT and authenticate the model, but the review job declares no environment:. In this repo those secrets are scoped to the azure-prod GitHub environment — every other consumer (main.yml:110, kernel-e2e.yml:179, and the engineer author job at engineer-bot.yml:50, whose own comment says "DATABRICKS_HOST / DATABRICKS_TOKEN ... live here") declares environment: azure-prod to reach them. Without it, environment-scoped secrets resolve to the empty string, yielding MODEL_ENDPOINT=https:///serving-endpoints/... and an empty token — the reviewer run fails at the model call. Add environment: azure-prod to this job (as the engineer author job does).
There was a problem hiding this comment.
Nope those 2 are also in the default environment
There was a problem hiding this comment.
I hear you that these may also be available in the default environment — but I can't verify secret scoping from the source, and the code still declares no environment: here while three sibling jobs (main.yml:110, kernel-e2e.yml:179, engineer-bot.yml:50) all set environment: azure-prod specifically to reach these same two secrets. If they truly resolved in the default environment, those jobs wouldn't need the declaration. Since this hinges on the actual GitHub environment config, which isn't visible in the repo, I'll leave this open for a human to confirm the secret scoping before dismissing it. If the secrets do resolve in the default environment, this is a safe no-op; if not, the reviewer run fails at the model call with an empty host/token.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
The comment targets .github/workflows/reviewer-bot.yml, which is a denied path in this job (no edits under .github/ are possible here). Beyond that, resolving it hinges on a repo-settings fact I can't verify from the tree: the bot claims DATABRICKS_HOST/DATABRICKS_TOKEN are scoped only to the azure-prod environment, while the PR author (eric-wang-1990) states they are also present in the default environment — in which case no environment: azure-prod is needed. Confirming which is correct requires inspecting the repo's GitHub environment/secret configuration, an admin/out-of-band check only a human can perform. Flagging for a human to confirm secret scoping and, if needed, add environment: azure-prod in a separate change.
| TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations |
There was a problem hiding this comment.
🟠 High — Same issue as the reviewer author workflow: the followup job uses secrets.DATABRICKS_HOST / secrets.DATABRICKS_TOKEN for MODEL_ENDPOINT/auth but declares no environment:. Since those secrets live in the azure-prod environment in this repo, they resolve empty here and the model call fails. Add environment: azure-prod to the job.
There was a problem hiding this comment.
Nope those 2 are also in the default environment
There was a problem hiding this comment.
I can't verify secret scoping from the source alone — it's repo/environment config that isn't visible in the workflow file. My concern was specifically that these two secrets resolve empty without environment: azure-prod. If they're genuinely also defined in the default environment, this is a non-issue; but since a wrong assumption here means the model call silently fails at runtime, I'd rather leave this open for a human to confirm the default-environment secret setup before dismissing it. If they're present there, please close this out.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
Cannot action here: the comment targets .github/workflows/reviewer-bot-followup.yml, and .github/ is a denied/non-writable path in this PR, so no edit is possible. Separately, the correctness of the finding depends on whether DATABRICKS_HOST/DATABRICKS_TOKEN are populated in the repo's default environment (as the PR author asserts) or only in the azure-prod environment (as the bot claims) — that's a repo secrets/environment configuration fact that can't be verified from the code and needs a human with repo-admin access. A human should confirm the secrets resolve in the default environment; if they do, the bot's finding is a false positive and no environment: azure-prod is needed. Thread has converged on the author's disagreement.
| PR_TITLE: ${{ github.event.pull_request.title }} | ||
| PR_URL: ${{ github.event.pull_request.html_url }} | ||
| TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} | ||
| MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations |
There was a problem hiding this comment.
🟠 High — The engineer author workflow declares environment: azure-prod (engineer-bot.yml:50) precisely because DATABRICKS_HOST/DATABRICKS_TOKEN are environment-scoped there, but this follow-up job references the same two secrets for MODEL_ENDPOINT/DATABRICKS_TOKEN with no environment: declared. They will resolve to empty strings and the follow-up model invocation will fail. Add environment: azure-prod to the followup job.
There was a problem hiding this comment.
Nope those 2 are also in the default environment
There was a problem hiding this comment.
The file still declares no environment:, and I can't view secret scoping from the source. If DATABRICKS_HOST/DATABRICKS_TOKEN are truly repository-level (default) secrets, this resolves fine and my concern is void — please confirm against the repo/org secret settings, since engineer-bot.yml opting into environment: azure-prod implies environment scoping for at least one of them. Keeping this open for a human to verify the scoping before merge.
There was a problem hiding this comment.
⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.
No code change is possible or warranted here. The comment targets .github/workflows/engineer-bot-followup.yml, which is a denied (non-writable) path for this job, so the requested environment: azure-prod edit cannot be applied. More fundamentally, the review-bot's premise — that DATABRICKS_HOST/DATABRICKS_TOKEN resolve to empty without an environment: declaration — depends on GitHub default-vs-environment-scoped secret configuration that cannot be read or verified from this runner. The PR author (@eric-wang-1990) has already authoritatively refuted the concern ("those 2 are also in the default environment"), so the thread has converged. Flagging for a human maintainer with repo settings access to confirm the default-environment secrets; nothing to change in this PR.
eric-wang-1990
added
the
engineer-bot
Applies the reviewer-bot hardening finding (raised on the odbc onboarding PR) to this repo: - Reviewer + reviewer-followup: drop the redundant `setup-jfrog` step. The engine install (install-bot-engine, use-jfrog=true) already mints + scopes its own JFrog creds in job-local 0600 files wiped in an EXIT trap, and the reviewer runs no npm install of its own. The shared setup-jfrog exported JFROG_ACCESS_TOKEN to $GITHUB_ENV and wrote a tokened ~/.npmrc, both persisting into the model-driven Run reviewer step. - Engineer author + followup: keep setup-jfrog (npm ci needs it) but add a "Scrub JFrog credentials" step after npm ci and before the agent — rm ~/.npmrc + blank JFROG_ACCESS_TOKEN. The agent's test commands (npm test / npm run e2e) don't need the registry, so this is safe. The shared .github/actions/setup-jfrog composite is untouched (real CI depends on it). Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-documented bot-onboarding PR that is almost entirely CI/prompt scaffolding; the only executable change (tokenFromConfigFile fallback in config.ts) preserves normal CI/local behavior (env var still wins). One low note: the new helper's catch collapses "missing file" and "malformed file" into the same silent undefined, which can produce a misleading "token is missing" abort.
Addresses: - #3655446201 at tests/e2e/utils/config.ts:78 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — this is a well-documented bots-onboarding PR that is almost entirely CI/YAML and prompt prose; the sole executable change (tests/e2e/utils/config.ts token fallback) is correct and leaves normal CI/local dev unchanged. One low-severity note about logging the raw parse exception in the credential-config path.
Addresses: - #3655491620 at tests/e2e/utils/config.ts:80 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 3 High
Mostly solid onboarding PR; the config.ts token fallback is careful (env var wins, credential-safe error logging) and the workflow security gates are well-reasoned. One repeated concern worth confirming: three of the four new bot workflows use secrets.DATABRICKS_HOST/DATABRICKS_TOKEN but omit environment: azure-prod, which the engineer-bot author job and main.yml's e2e job both declare to reach those same (apparently environment-scoped) secrets — if so, those three jobs will get empty values and fail.
Summary
Onboards both databricks-bot-engine bots to
databricks-sql-nodejs, PAT-free (App-auth), pinned to engine SHAd05dcb11. Adapted for this repo's TypeScript / npm toolchain. Structured to land in one PR (per the engine's one-PR onboarding checklist).tests/e2etest against a warehouse (Thrift backend), fixes the TS, and opens a PR; follows up on review comments.The engine is
pip-installed in REF mode (it can't beuses:-d cross-repo) via two local composites that mirror thedatabricks-sql-python/databricks-odbconboardings.What's in this PR
.github/actions/install-bot-engine/.github/actions/bot-prelude/.github/workflows/reviewer-bot.yml+-followup.yml.github/workflows/engineer-bot.yml+-followup.ymlnpm ci(JFrog mirror) so the agent can build & test; author requires live e2e, followup runs mockednpm testonly.bot/config.yaml+.bot/prompts/**tests/e2e/utils/config.tsTypeScript-specific adaptations
setup-jfrogruns before everynpm ci;id-token: writeon all jobs.npm ci, nevernpm install. The allowlist and workflows usenpm cionly —npm installwould rewritepackage-lock.jsonto lockfileVersion 3 and fail the repo's lint gate. No tracked-file churn otherwise (theprepare→tscbuild outputs to gitignoreddist/+lib/version.ts), so no revert step is needed.tests/e2e/utils/config.tsreads the PAT fromE2E_ACCESS_TOKENandprocess.exit(1)s if any of its 7 vars is missing. The engine scrubs*_TOKEN-shaped env vars from the agent subprocess, so the token can't ride an env var — the author workflow writes it to$RUNNER_TEMP/e2e-connection.jsonand passes the path inDATABRICKS_TEST_CONFIG_FILE(a name the scrub preserves);config.tsfalls back to it only whenE2E_ACCESS_TOKENis absent. Env var still wins → normal CI and local dev are byte-for-byte unchanged. (Verified:config.tstypechecks under the repo's pinned TS 5.5.4/strict and is Prettier-clean.)databricks-sql-kernelrepo + Rust + napi), so the bug-fix prompt targets the Thrift-backedtests/e2e/*.test.tssuite; kernel-only bugs reportblocked.denied_subpathsblocks.github/,native/(napi kernel binding), andthrift/(generated stubs).bash_allowlistis npm/mocha (npm test,npm run e2e,npx mocha --config …,npm run build/type-check/lint/prettier,npm ci,git status).The workflows will fail at the first token-mint step until these are in place (this is exactly what happened on the odbc onboarding until the Apps were installed):
contents/pull-requests/issues: write:REVIEW_BOT_APP_ID+REVIEW_BOT_APP_PRIVATE_KEYENGINEER_BOT_APP_ID+ENGINEER_BOT_APP_PRIVATE_KEY(None exist in this repo today — only
INTEGRATION_TEST_APP_*.)databricks-sql-nodejsAND ondatabricks-bot-engine(the latter withcontents: read) — the engine install mints an engine-scoped token, so the App must see the engine repo. (Missing the repo install →create-github-app-tokenreturns 404 at the prelude.)DATABRICKS_HOST/DATABRICKS_TOKEN(already present, used by the e2e job) point at a workspace hosting thedatabricks-claude-opus-4-8serving endpoint;TEST_PECO_WAREHOUSE_HTTP_PATHpresent (used forE2E_PATH).engineer-botlabel has been created on this repo as part of this change. The reviewer needs no label — it fires on PR open.issues/workflow_dispatchtriggers only register from the default branch, so the engineer workflows go live only after this PR merges.Verification
.bot/config.yamlparse; actionlint clean (only the knownlinux-ubuntu-latestself-hosted-label false positive, same labelmain.ymluses).tests/e2e/utils/config.tstypechecks under the repo's pinned TypeScript 5.5.4 (strict, ES2018, commonjs, esModuleInterop, node types) and passes Prettier with the repo's.prettierrc.name/marker_namespace/pr_body_template; fork gates +id-token: writepresent on all applicable jobs; no reuse of the app-pinnedNode.js Integration Tests/Kernel E2Echeck names.Bug-typed issueengineer-bot(author).This pull request and its description were written by Isaac.
This PR was created with GitHub MCP.