diff --git a/.bot/config.yaml b/.bot/config.yaml index dab384f3f..3450f8715 100644 --- a/.bot/config.yaml +++ b/.bot/config.yaml @@ -86,6 +86,10 @@ author: issue_url: ISSUE_URL context_files: - issue_body.txt # {{issue_body}} + # Close the loop: the author phase READS this log (newest-first, capped) into its + # system prompt as accumulated lessons, so retrospective learnings actually steer + # future fixes. MUST match retrospective.log_path below (write half ↔ read half). + knowledge_log: .claude/knowledge/learning-log.md # Engine orchestration for the author phase. `bug-fix` runs the plan → # author_tests → fix pipeline (write a failing test → fix the code → re-run to @@ -94,3 +98,20 @@ author: # testing specifics (commands, layout, fixtures) live in prompts/engineer/ # system.md, not here. flow: bug-fix + +# Daily-cron learning extraction (retrospective flow). Run by engineer-bot-learning.yml +# via `python -m databricks_bot_engine.engineer_bot.retrospective`: over an adaptive +# look-back window the engine ITSELF gathers merged PRs (diff + review comments) AND +# recent engineer-bot author-run logs, and if the model finds durable learnings opens +# ONE rolling PR appending a dated section to log_path. Human-gated — never commits the +# canonical log directly. Omitting this block makes the retrospective a no-op. +# +# No `system_prompt` override: the engine ships the authoritative batch-aware base +# (engineer_prompts.RETRO_SYSTEM_PROMPT); a `.bot/prompts/` override is optional. +# No `context_files` — the daily-cron engine enumerates its own sources via the +# GitHub API (context_files is an AUTHOR-phase key only). +retrospective: + log_path: .claude/knowledge/learning-log.md # MUST match author.knowledge_log above + branch_prefix: ai/learning-pr- # rolling PR lands on the derived stable branch `ai/learning-pr` + pr_label: engineer-bot-learning + author_workflow: engineer-bot.yml # Track B: which workflow's author runs to mine diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md new file mode 100644 index 000000000..0e0c2f2e9 --- /dev/null +++ b/.claude/knowledge/learning-log.md @@ -0,0 +1,11 @@ +# Engineer-bot learning log + +Durable, reusable engineering learnings distilled by the daily retrospective +(`engineer-bot-learning.yml` → `databricks_bot_engine.engineer_bot.retrospective`). +Each daily run appends one dated section of 0..N learnings to a single rolling PR +until a human merges it. The engineer author phase reads this log (see +`.bot/config.yaml` `author.knowledge_log`) so past lessons steer future fixes. + +## Entries + +--- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* --- diff --git a/.github/actions/bot-prelude/action.yml b/.github/actions/bot-prelude/action.yml index b2c0a112d..aa825648e 100644 --- a/.github/actions/bot-prelude/action.yml +++ b/.github/actions/bot-prelude/action.yml @@ -31,7 +31,7 @@ inputs: # value to move every bot to a new engine commit; never @main. description: 'Engine commit SHA (full 40-char) to install.' required: false - default: '5368de773858bef9b2a8bbf3099d08a4132a8487' + default: 'd05dcb113332401b4aee8d6aa05c7107399ad44f' engine-repo: description: 'owner/name of the engine repo.' required: false diff --git a/.github/workflows/engineer-bot-learning.yml b/.github/workflows/engineer-bot-learning.yml new file mode 100644 index 000000000..620a2f8dc --- /dev/null +++ b/.github/workflows/engineer-bot-learning.yml @@ -0,0 +1,122 @@ +# Engineer Bot — learning (retrospective) extraction — DAILY CRON. +# +# Over an adaptive look-back window the engine gathers merged PRs (diff + review +# comments) AND recent engineer-bot author-run console logs ITSELF via the GitHub +# API — no in-workflow context gathering, no per-PR trigger — and if the model +# finds durable, reusable learnings, opens ONE ROLLING PR on a stable branch +# (`ai/learning-pr`), appending a dated section per day until a human merges it. +# Human-gated by design: it NEVER commits the canonical log directly. +# +# Own job (NOT `uses: databricks/databricks-bot-engine/...`): an external repo +# can't resolve the internal engine's reusable workflows ("not found"). It shares +# the SAME prelude the other bots use — ./.github/actions/bot-prelude (tokens + +# Node + pinned engine install) — so the engine pin stays single-sourced in +# bot-prelude's `engine-ref` default (no second SHA to drift). +# +# Opt-in is purely via the `retrospective:` block in .bot/config.yaml + this +# workflow; absent that block the engine phase is a clean no-op. +name: Engineer Bot — Learning + +on: + schedule: + # 17:23 UTC daily — off-peak, off-:00 minute (GitHub delays/drops on-the-hour crons). + - cron: "23 17 * * *" + workflow_dispatch: + inputs: + since: + description: 'ISO lower bound to shorten the window and recover a wedged flow. Empty = adaptive cursor.' + type: string + default: '' + window-hours: + # STRING, not number: a `type: number` workflow_dispatch input fails the + # whole run at startup ("workflow file issue") when combined with the + # `schedule` trigger. argparse coerces it to int downstream. + description: 'Fallback look-back window (hours) used only when there is no prior successful run.' + type: string + default: '24' + +permissions: + contents: write # push the learning branch / open the learning PR + pull-requests: write + actions: read # Track B lists engineer-bot author runs + logs via the App token; + # the engineer-bot App installation must ALSO carry actions:read + # (a missing scope surfaces as a 403 that fails the whole run — + # list_author_runs raises, no escape hatch). + id-token: write # JFrog OIDC exchange for the engine/SDK/CLI install + +concurrency: + # One learning run at a time; a queued run waits rather than racing the rolling + # PR's branch. Not keyed on a PR number (this is a cron, no PR event). + group: engineer-bot-learning-cron + cancel-in-progress: false + +jobs: + learning: + environment: azure-prod # DATABRICKS_HOST / DATABRICKS_TOKEN live here + runs-on: + group: databricks-protected-runner-group + labels: [linux-ubuntu-latest] + timeout-minutes: 20 + steps: + # Checkout the default branch (the learning PR is cut from it) FIRST, so the + # local `./` composites below resolve. persist-credentials:false — the + # retrospective sets its own authenticated push remote (see the run step), + # so no token is left in .git/config. + - name: Checkout default branch (learning PR is cut from it) + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 + persist-credentials: false + + # No JFrog setup step here: install-bot-engine (via bot-prelude) does its own + # keyless OIDC→JFrog mint and passes the credential through job-local files + + # per-command --index-url flags. It deliberately never reads PIP_INDEX_URL / + # JFROG_ACCESS_TOKEN from the environment, so a setup-jfrog step would be both + # redundant AND a credential leak (setup-jfrog exports a token-bearing + # PIP_INDEX_URL to $GITHUB_ENV, exposing it to every later step — including the + # one that runs the model). Mirrors the read-only sibling reviewer-bot.yml. + - name: Setup Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 + with: + python-version: '3.11' + + # Shared prelude: mint the engineer-bot token (opens the learning PR) + the + # engine-scoped token, set up Node, install the pinned engine (PAT-free). The + # engine pin comes from bot-prelude's `engine-ref` default — the SINGLE source + # of truth for every bot; there is no second SHA in this file to drift. + - name: Bot prelude (tokens + Node + engine install) + id: prelude + uses: ./.github/actions/bot-prelude + with: + app-id: ${{ secrets.ENGINEER_BOT_APP_ID }} + private-key: ${{ secrets.ENGINEER_BOT_APP_PRIVATE_KEY }} + + # NOTE: no git-identity step — the engine's retrospective configures the git + # user AND DCO sign-off itself from .bot/config.yaml `bot_login_prefix`. + # NOTE: no context-gather step — the daily-cron engine enumerates merged PRs + # + author runs itself over the adaptive window. + - name: Extract learnings + open rolling PR + env: + GH_TOKEN: ${{ steps.prelude.outputs.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + # Only the `/serving-endpoints/` prefix matters: + # sdk_agent.translate_endpoint rewrites this to `.../serving-endpoints/anthropic` + # and discards the model path segment. The effective model comes from + # .bot/config.yaml `retrospective.model` (or the engine default). + MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/anthropic/invocations + DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} + RUNNER_TEMP: ${{ runner.temp }} + SINCE: ${{ inputs.since }} + WINDOW_HOURS: ${{ inputs.window-hours }} + # The retrospective pushes the learning branch with a plain `git push + # origin`, and the checkout ran persist-credentials:false — so set an + # authenticated push remote from the minted App token first, mirroring + # engineer-bot.yml's publish step. --since / --window-hours are passed only + # when provided via workflow_dispatch (the schedule trigger leaves them + # empty → the adaptive cursor drives the window). + run: | + git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" + args=(--repo-dir "$GITHUB_WORKSPACE") + [ -n "$SINCE" ] && args+=(--since "$SINCE") + [ -n "$WINDOW_HOURS" ] && args+=(--window-hours "$WINDOW_HOURS") + python -m databricks_bot_engine.engineer_bot.retrospective "${args[@]}" diff --git a/.github/workflows/skip-checks-reporter.yml b/.github/workflows/skip-checks-reporter.yml new file mode 100644 index 000000000..65f92de6d --- /dev/null +++ b/.github/workflows/skip-checks-reporter.yml @@ -0,0 +1,124 @@ +name: Report Integration Test Skip + +# Posts the PR-open "skipped" placeholder for the required +# `Python Integration Tests` check on FORK PRs, as the driver-test GitHub App. +# +# Division of labour: +# - INTERNAL PRs: the placeholder is posted inline by trigger-integration-tests.yml's +# `skip-integration-tests-pr` job (it can mint the app token directly). Posting it +# there means it works from the PR branch immediately, without waiting for this +# workflow_run file to reach the default branch. +# - FORK PRs: a fork's `pull_request` run has a READ-ONLY GITHUB_TOKEN and no secrets, +# so it cannot mint the app token or post any check on its own head. This +# workflow_run workflow instead runs in THIS (base) repo's context with full secret +# access even for fork-triggered runs, so it can post the app-attributed placeholder +# on a fork PR's head. This job self-guards to fork runs to avoid double-posting on +# internal PRs (which the inline job already covers). +# +# Why the app (not github.token): the ruleset pins the required +# `Python Integration Tests` check to the driver-test app's integration id. Only a +# check posted BY that app satisfies the gate — a github.token (github-actions) check +# of the same name is a different context and does NOT. +# +# SECURITY: this workflow runs with secrets in a privileged context. It MUST NOT check +# out or execute any PR/fork-controlled content. It only calls checks.create with a +# static body; the sole fork-controlled input is `workflow_run.head_sha`, an opaque +# commit SHA passed to the API. Do not add `actions/checkout` or a `run:` step that +# executes repo content here. +# +# The real integration suite is unaffected: it runs as the required gate on the +# `merge_group` commit (and as a label preview on internal PRs), dispatched by +# trigger-integration-tests.yml. Mirrors databricks-sql-go / databricks-sql-nodejs. + +on: + workflow_run: + workflows: ["Trigger Integration Tests"] + types: [requested] + +jobs: + report-skip: + # Fork PR-triggered runs only. Internal PRs are posted inline by + # trigger-integration-tests.yml; the merge_group run posts the real required check. + if: >- + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.head_repository.full_name != github.event.workflow_run.repository.full_name + runs-on: + group: databricks-protected-runner-group + labels: linux-ubuntu-latest + permissions: + checks: write + contents: read + pull-requests: read + steps: + # This job is fork-only (see the job `if:`), so the placeholder is always + # appropriate EXCEPT when the fork PR is already closed. A fork PR's label + # preview cannot dispatch the real suite (no secret access), so nothing + # else posts the required check for it — keep the placeholder even when + # labeled. Skip only closed PRs. Resolve the PR by SHA because a fork's + # workflow_run payload has an empty pull_requests array. Read-only lookup + # (no checkout / no execution of PR content) — does not weaken the SECURITY + # note above. + - name: Decide whether to post the placeholder + id: gate + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 + env: + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + with: + script: | + let prs = context.payload.workflow_run.pull_requests || []; + let number = prs.length ? prs[0].number : null; + if (number === null) { + // Fork PRs: workflow_run.pull_requests is empty. Resolve by SHA. + const { data } = await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner: context.repo.owner, + repo: context.repo.repo, + commit_sha: process.env.HEAD_SHA, + }); + number = data.length ? data[0].number : null; + } + if (number === null) { + // No PR resolvable (unexpected) — default to posting so the + // required check isn't left unfulfilled. + core.setOutput('post', 'true'); + return; + } + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: number, + }); + const isClosed = pr.state === 'closed'; + console.log(`PR #${number} closed=${isClosed} -> post=${!isClosed}`); + core.setOutput('post', (!isClosed).toString()); + + - name: Generate GitHub App token (this repo) + id: app-token + if: steps.gate.outputs.post == 'true' + uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0 + with: + app-id: ${{ secrets.INTEGRATION_TEST_APP_ID }} + private-key: ${{ secrets.INTEGRATION_TEST_PRIVATE_KEY }} + owner: databricks + repositories: databricks-sql-python + + - name: Post skipped Python Integration Tests check + if: steps.gate.outputs.post == 'true' + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 + env: + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + with: + github-token: ${{ steps.app-token.outputs.token }} + script: | + await github.rest.checks.create({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'Python Integration Tests', + head_sha: process.env.HEAD_SHA, + status: 'completed', + conclusion: 'success', + completed_at: new Date().toISOString(), + output: { + title: 'Skipped on PR — runs in merge queue', + summary: 'Python Integration Tests are skipped on ordinary PR events and run as the required gate in the merge queue (dispatched to databricks-driver-test). Add the `integration-test` label to preview them on this PR. (Label previews cannot run on fork PRs, which lack secret access; fork PRs are exercised by the required merge-queue run.)', + }, + }); diff --git a/.github/workflows/trigger-integration-tests.yml b/.github/workflows/trigger-integration-tests.yml index b8bb90ba2..e4ab7a6bb 100644 --- a/.github/workflows/trigger-integration-tests.yml +++ b/.github/workflows/trigger-integration-tests.yml @@ -117,54 +117,60 @@ jobs: }); # ============================================================================= - # For PRs: Always pass the Python Integration Tests check on - # non-label events. The real run happens in the merge queue (or via - # explicit label preview). Without this, the required - # `Python Integration Tests` check would block every PR that doesn't - # bother labelling. + # For internal PRs: post the "skipped" placeholder for the required + # `Python Integration Tests` check on non-label events. The real run happens in + # the merge queue (or via explicit label preview). + # + # CRITICAL: post as the driver-test APP, not github.token. The ruleset pins the + # required check to that app's integration id; a github.token (github-actions) + # check of the same name is a DIFFERENT context and does NOT satisfy the gate, + # so the required check would sit "waiting for status" forever. + # + # Fork PRs are handled by the companion `skip-checks-reporter.yml` (workflow_run): + # a fork's `pull_request` run has a read-only token and no secrets, so it can + # neither mint the app token nor post any check here. This job self-guards to + # internal PRs (head repo == base repo); the reporter covers forks from the + # base-repo context. Internal PRs are posted HERE (not via the reporter) so the + # placeholder appears without depending on the workflow_run copy being on the + # default branch. # ============================================================================= skip-integration-tests-pr: - if: github.event_name == 'pull_request' && github.event.action != 'labeled' && github.event.action != 'closed' + if: | + github.event_name == 'pull_request' && + github.event.action != 'labeled' && + github.event.action != 'closed' && + github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name runs-on: group: databricks-protected-runner-group labels: linux-ubuntu-latest - permissions: - checks: write steps: + - name: Generate GitHub App token (this repo) + id: app-token + uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0 + with: + app-id: ${{ secrets.INTEGRATION_TEST_APP_ID }} + private-key: ${{ secrets.INTEGRATION_TEST_PRIVATE_KEY }} + owner: databricks + repositories: databricks-sql-python + - name: Skip Python Integration Tests uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: - github-token: ${{ github.token }} + github-token: ${{ steps.app-token.outputs.token }} script: | - // On FORK PRs GitHub forces GITHUB_TOKEN to read-only regardless of - // the declared `checks: write`, so checks.create 403s ("Resource - // not accessible by integration"). Expected — a fork can't post - // check-runs on the base repo. Swallow the 403 for forks so this - // poster doesn't show a spurious failure; the real Python - // Integration Tests required check is posted by the merge_group run - // (full perms) when a maintainer queues the PR. Other errors fail loudly. - const isFork = context.payload.pull_request.head.repo.fork; - try { - await github.rest.checks.create({ - owner: context.repo.owner, - repo: context.repo.repo, - name: 'Python Integration Tests', - head_sha: context.payload.pull_request.head.sha, - status: 'completed', - conclusion: 'success', - completed_at: new Date().toISOString(), - output: { - title: 'Skipped on PR — runs in merge queue', - summary: 'Python Integration Tests are skipped on PRs and run as a required gate in the merge queue. Add the `integration-test` label to preview them on this PR.' - } - }); - } catch (e) { - if (isFork && e.status === 403) { - core.notice('Fork PR: cannot post the Python Integration Tests check-run (read-only token). It will be posted by the merge queue at merge time.'); - } else { - throw e; + await github.rest.checks.create({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'Python Integration Tests', + head_sha: context.payload.pull_request.head.sha, + status: 'completed', + conclusion: 'success', + completed_at: new Date().toISOString(), + output: { + title: 'Skipped on PR — runs in merge queue', + summary: 'Python Integration Tests are skipped on PRs and run as the required gate in the merge queue. Add the `integration-test` label to preview them on this PR.' } - } + }); # ============================================================================= # For PRs: Dispatch real tests when integration-test label is added. @@ -333,6 +339,41 @@ jobs: with: fetch-depth: 0 + # Mint the driver-test App token UNCONDITIONALLY. The required + # `Python Integration Tests` check is pinned in the ruleset to this app's + # integration id, so EVERY check this job posts — the real result (posted + # back by driver-test on dispatch), the no-op auto-pass, and the + # dispatch-failure — must be attributed to the same app, or it lands on a + # different check context and never satisfies the pinned gate (leaving the + # merge queue stuck). merge_group runs on the base repo with full secret + # access, so the mint normally succeeds here. + # + # If the mint ITSELF fails (secret rotation, app uninstall, transient + # create-github-app-token error) the job aborts before it can post any + # check. That case is uncloseable in-workflow — only this app can post to + # the app-pinned context, so no fallback identity (github.token included) + # can substitute; a same-named github.token check lands on a DIFFERENT + # context and neither satisfies nor fails the gate. It is fail-CLOSED, not a + # bypass: the merge queue's check_response_timeout evicts an entry whose + # required check never reports (it does not merge), and a mint failure also + # surfaces as a red workflow run. Mitigation is out-of-band (alerting on the + # token-mint step), not another in-workflow check. + - name: Generate GitHub App Token (internal repo) + id: app-token + uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0 + with: + app-id: ${{ secrets.INTEGRATION_TEST_APP_ID }} + private-key: ${{ secrets.INTEGRATION_TEST_PRIVATE_KEY }} + owner: databricks + # Two repos: dispatch targets databricks-driver-test, while the + # auto-pass / dispatch-failure steps post the pinned check-run onto + # THIS repo (context.repo == databricks-sql-python in a merge_group + # run). A token scoped to driver-test alone 403s on those + # checks.create calls, leaving the required gate unposted. + repositories: | + databricks-driver-test + databricks-sql-python + - name: Check if driver files changed id: changed env: @@ -352,9 +393,9 @@ jobs: if: steps.changed.outputs.changed != 'true' uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: - # Default workflow token — see the trigger-tests-pr job's - # equivalent step above for the rationale. - github-token: ${{ github.token }} + # App token, not github.token — the pinned required check is only + # satisfied by a check posted BY the driver-test app. + github-token: ${{ steps.app-token.outputs.token }} script: | await github.rest.checks.create({ owner: context.repo.owner, @@ -386,16 +427,6 @@ jobs: exit 1 fi - - name: Generate GitHub App Token (internal repo) - if: steps.changed.outputs.changed == 'true' - id: app-token - uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0 - with: - app-id: ${{ secrets.INTEGRATION_TEST_APP_ID }} - private-key: ${{ secrets.INTEGRATION_TEST_PRIVATE_KEY }} - owner: databricks - repositories: databricks-driver-test - - name: Dispatch Python tests if: steps.changed.outputs.changed == 'true' uses: peter-evans/repository-dispatch@ff45666b9427631e3450c54a1bcbee4d9ff4d7c0 # v3.0.0 @@ -418,9 +449,10 @@ jobs: if: failure() && steps.changed.outputs.changed == 'true' uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: - # Use the default workflow token, not the App token — see - # the rationale in the trigger-tests-pr job above. - github-token: ${{ github.token }} + # App token, not github.token — a github-actions failure check lands on + # a different context than the pinned gate, leaving the required check + # pending until the queue times out instead of failing fast. + github-token: ${{ steps.app-token.outputs.token }} script: | await github.rest.checks.create({ owner: context.repo.owner,