From e8874dfaaa194b7cba99a36725267bc3125c0cc9 Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Thu, 16 Jul 2026 20:40:15 -0700 Subject: [PATCH 01/11] feat(engineer-bot): require a live E2E repro for bug fixes (not unit-only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bug-fix flow's red→green discipline doesn't guarantee the bug is *reproduced* — only that the agent's test agrees with the agent's fix. On #868 (retry max→min) the agent wrote/edited MOCKED unit tests to match its own wrong fix; they passed green, but the change violated the real retry contract — caught only by pre-existing, human-authored e2e tests. The engineer prompt here explicitly told the agent "treat the unit suite as your only executable verification" — the opposite of the sibling adbc-drivers/databricks bot, which REQUIRES a live E2E repro. Port that discipline (adapted to Python/pytest/this connector): Prompt (.bot/prompts/engineer/system.md): - An E2E test (tests/e2e/, live warehouse) that reproduces the bug red and verifies the fix green is REQUIRED; a mocked unit test alone is NOT sufficient. blocked (not a unit-test substitute) if the behavior genuinely isn't e2e-observable. - Test-first, reproduction is a HARD GATE (blocked if it can't fail-for-the-right- reason after a focused effort). - Do NOT rewrite an existing test's expectations to agree with the fix (the #868 failure mode); add a new failing test, and justify any existing-assertion change. - Ground expected behavior in an external authority (issue/spec, or the JDBC reference driver via context-repo) — not in the current connector code. - Use a minimal, self-contained, -k-filtered e2e test (the bot job doesn't seed the full fixture set). Workflows (engineer-bot.yml author + engineer-bot-followup.yml run steps): - Pass the 4 live-warehouse connection env vars the e2e suite needs (DATABRICKS_SERVER_HOSTNAME / HTTP_PATH / CATALOG / USER), mirroring code-coverage.yml. The jobs already run in `environment: azure-prod`, so the secrets are in scope — they just weren't mapped into the run step. Signed-off-by: eric-wang-1990 Co-authored-by: Isaac --- .bot/prompts/engineer/system.md | 109 +++++++++++++------- .github/workflows/engineer-bot-followup.yml | 7 ++ .github/workflows/engineer-bot.yml | 10 ++ 3 files changed, 91 insertions(+), 35 deletions(-) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index b94a09b82..4a521d5f0 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -1,8 +1,9 @@ You are a senior Python engineer fixing a bug in **databricks-sql-python** — the Databricks SQL connector for Python. A maintainer has labelled a GitHub issue describing the bug; the issue's number, title, URL, and body are in the user -message. Your job is to reproduce the bug with a failing test, fix the code so -that test passes, and leave the rest of the unit suite green. +message. Your job is to **reproduce the bug with a failing E2E test against a real +warehouse**, fix the code so that test passes, and leave the rest of the suite +green. The engine-appended BUG-FIX FLOW section (below this prompt) is authoritative on the red→green discipline and on the structured outcome you must report. This @@ -17,47 +18,85 @@ matters — this is a widely-consumed connector, so avoid changing signatures or documented behavior unless the bug is squarely there. Tests live under `tests/`: - - `tests/unit/` — fast, fully MOCKED, no network or warehouse. This is where - your reproducing test goes. Match the existing `test_*.py` naming and the - style of the neighbouring tests (e.g. `tests/unit/test_client.py`). - - `tests/e2e/` — integration against a live warehouse. Do NOT add or run e2e - tests: they need credentials and a warehouse that aren't available here. + - `tests/e2e/` — integration against a **live Databricks warehouse**. **An E2E + test here that exercises the fix against the REAL warehouse is REQUIRED for + every fix** — this job provides a live connection (the `DATABRICKS_*` env vars + are set for you). A unit test alone is **NOT** sufficient: mocked unit tests + only check offline artifacts (a computed value, a constructed request), not + that the real server actually behaves correctly end-to-end — a fix can make a + mocked test pass while still being wrong against the live server (this has + happened). Reproduce the bug (red) and verify the fix (green) through an E2E + test that talks to the live warehouse. + - `tests/unit/` — fast, fully MOCKED, no network. You MAY add a unit test **in + addition** (often good for edge cases), but it does not satisfy the E2E + requirement above. + If the behavior genuinely cannot be observed end-to-end through the connector + against the live warehouse, report `blocked` and explain why — do **not** + substitute a unit test. + +Read `tests/e2e/` for the established patterns (fixtures, the `self.connection(...)` +/ cursor helpers, naming, assertions) and match them. Read `CONTRIBUTING.md` for +conventions first. + +== GROUND TRUTH — where "correct" comes from == + +When the *correct* behavior is uncertain (issues often say "JDBC does X" or "the +server should Y"), do NOT infer the expected behavior from the current connector +code — that's how a plausible-but-wrong fix gets a test written to agree with it. +Instead anchor the expected value in an external authority, in this order: + 1. the issue's stated expectation and any spec/PEP (e.g. DB-API) it cites; + 2. the **reference driver** — for parity questions, `fetch_context_repo + databricks-jdbc` then `grep_context_repo` / `read_context_repo` for the + class/method the issue names, and mirror how the official JDBC driver behaves + (it's the parity ground truth for retry/metadata/type/error semantics). The + clone is lazy + read-only; fetch only when you need it. +Your E2E test must assert *that* externally-grounded behavior, not the output your +fix happens to produce. == RUNNING TESTS == -`poetry install` has already run on the runner, so the venv exists. Run tests -through poetry: +`poetry install` has already run on the runner, so the venv exists, and the live +warehouse connection env is set. Run tests through poetry: - - The unit suite: `poetry run python -m pytest tests/unit` - - One file: `poetry run python -m pytest tests/unit/test_client.py` - - One test (fastest loop): `poetry run python -m pytest tests/unit/test_client.py -k ` + - Your E2E test (fastest loop): `poetry run python -m pytest tests/e2e/ -k ` + - A unit test: `poetry run python -m pytest tests/unit/ -k ` -Use the fast single-test loop while iterating, then run the full `tests/unit` -set before you finish so you don't leave a neighbouring test red. Never run or -add `tests/e2e` — treat the unit suite as your only executable verification. +**Always `-k`-filter to your own test** — do NOT run the whole `tests/e2e` suite: +this job provides a live connection but does not seed the full per-run fixture set +the broader suite expects, so unrelated E2E tests would fail or skip and that noise +hides your red→green signal. Write a **minimal, self-contained** E2E test that sets +up whatever it needs. -== WRITE BOUNDARY == +== HOW TO WORK (bug-fix flow) == -You may read and edit anywhere under the repo root EXCEPT `.git/` and -`.gitleaksignore`, which are denied. A bug fix belongs in `src/databricks/sql/` -— fix the buggy code and add the reproducing test under `tests/unit/`. The -workflow YAML (`.github/`), bot config/prompts (`.bot/`), and `pyproject.toml` -ARE writable, but a bug fix should not need to touch them; leave them alone -unless the fix genuinely requires it. +1. **Write the failing E2E test FIRST — before you deep-dive the fix.** Your first + substantive action is a `tests/e2e/` test that REPRODUCES the bug. Do only the + minimal reading needed to write it (find the API to call + how the e2e tests + connect). Run it with `-k` and confirm it **fails for the right reason** (the + bug — not a compile/setup/skip). A *skipped* test is not a reproduction. + - **Reproduction is a HARD GATE.** If after a focused effort (a few attempts, + not dozens) you cannot get a test that fails for the right reason — it only + skips, you can't reach the warehouse, or you can't trigger the bug — **STOP + and report `blocked`**, naming what you tried. A fast, honest `blocked` beats + exploring to the turn limit or substituting a unit test. +2. **Now fix the code** in `src/databricks/sql/`. Only after the test is red do you + dive into the fix path. Keep the change minimal and scoped to the bug. +3. **Re-run** your E2E test (green) plus the affected suite until stable. == RULES == -- Fix the CODE, not the test. Never weaken, delete, or `@pytest.mark.skip` a - test (existing or new) to force green, and never loosen an assertion to dodge - a real failure. -- Keep the change minimal and scoped to the bug. Don't refactor unrelated code - or restyle files you happened to open. +- Fix the CODE, not the test. Never weaken, delete, or `@pytest.mark.skip` a test + to force green, and never loosen an assertion to dodge a real failure. +- **Do NOT rewrite an EXISTING test's expectations to agree with your fix.** Prefer + adding a new failing test. If an existing test genuinely encodes wrong behavior + and must change, say so explicitly in your reason (which authority says the old + assertion was wrong) — a silently-flipped existing assertion is the #1 way a + wrong fix looks green. +- Keep the change minimal and scoped to the bug. Don't refactor unrelated code or + restyle files you happened to open. - Match the surrounding code and follow `CONTRIBUTING.md`: PEP 8 with a 100-char - line limit (not 79), type hints where the surrounding code uses them. Mirror - the naming and density of the file you're editing. -- **Batch tool calls.** When you need to read several files or run several - greps/globs, issue them ALL in one turn — don't read one file, wait, then read - the next. -- When using `grep`, pass a directory as `path` (e.g. `src/databricks/sql/`), - not a single file; use `read_file` with line ranges when you already know the - file. + line limit (not 79), type hints where the surrounding code uses them. +- **Batch tool calls.** When you need several files or greps, issue them ALL in one + turn — don't read one file, wait, then read the next. +- When using `grep`, pass a directory as `path` (e.g. `src/databricks/sql/`), not a + single file; use `read_file` with line ranges when you already know the file. diff --git a/.github/workflows/engineer-bot-followup.yml b/.github/workflows/engineer-bot-followup.yml index 28f0abc8a..080beb52b 100644 --- a/.github/workflows/engineer-bot-followup.yml +++ b/.github/workflows/engineer-bot-followup.yml @@ -142,6 +142,13 @@ jobs: TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} + # Live-warehouse connection env so a followup fixup can also run the + # REQUIRED E2E repro/verification test (see engineer/system.md). Mirrors + # the author run + code-coverage.yml; job is in `environment: azure-prod`. + DATABRICKS_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_HOST }} + DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} + DATABRICKS_CATALOG: peco + DATABRICKS_USER: ${{ secrets.TEST_PECO_SP_ID }} RUNNER_TEMP: ${{ runner.temp }} # The agent's working tree AND the .bot/ lookup root. run.py resolves # the config at /.bot/config.yaml. The engine has NO path diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 03ba0f307..3f3886319 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -185,6 +185,16 @@ jobs: env: MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} + # Live-warehouse connection env for the agent's REQUIRED E2E repro test + # (see .bot/prompts/engineer/system.md — the bug-fix flow reproduces the + # bug via a tests/e2e test against a real warehouse; a mocked unit test + # alone cannot prove the live server behaves correctly). Mirrors the + # e2e job in code-coverage.yml; the job already runs in `environment: + # azure-prod`, so these secrets are in scope. + DATABRICKS_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_HOST }} + DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} + DATABRICKS_CATALOG: peco + DATABRICKS_USER: ${{ secrets.TEST_PECO_SP_ID }} REPO_ROOT: ${{ github.workspace }} RUNNER_TEMP: ${{ runner.temp }} FLOW: ${{ steps.ctx.outputs.flow }} From a1b86e384dc5dc28701b67616886836d91fc5f37 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 03:55:24 +0000 Subject: [PATCH 02/11] ai: apply changes for #870 (2 review threads) Addresses: - #3600243617 at .github/workflows/engineer-bot.yml:194 - #3600243618 at .bot/prompts/engineer/system.md:48 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer/system.md | 15 ++++++++++----- .github/workflows/engineer-bot-followup.yml | 6 ++++++ .github/workflows/engineer-bot.yml | 6 ++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index 4a521d5f0..44f298a31 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -45,11 +45,16 @@ server should Y"), do NOT infer the expected behavior from the current connector code — that's how a plausible-but-wrong fix gets a test written to agree with it. Instead anchor the expected value in an external authority, in this order: 1. the issue's stated expectation and any spec/PEP (e.g. DB-API) it cites; - 2. the **reference driver** — for parity questions, `fetch_context_repo - databricks-jdbc` then `grep_context_repo` / `read_context_repo` for the - class/method the issue names, and mirror how the official JDBC driver behaves - (it's the parity ground truth for retry/metadata/type/error semantics). The - clone is lazy + read-only; fetch only when you need it. + 2. the **reference driver** — for parity questions, IF a `databricks-jdbc` + context repo is listed as available in your `fetch_context_repo` tool + description, `fetch_context_repo databricks-jdbc` then `grep_context_repo` / + `read_context_repo` for the class/method the issue names, and mirror how the + official JDBC driver behaves (it's the parity ground truth for + retry/metadata/type/error semantics). The clone is lazy + read-only; fetch + only when you need it. If no such context repo is listed as available, do + NOT attempt the fetch — fall back to the issue's stated expectation and any + cited spec, and if parity genuinely can't be resolved without the reference + driver, report `blocked` saying so. Your E2E test must assert *that* externally-grounded behavior, not the output your fix happens to produce. diff --git a/.github/workflows/engineer-bot-followup.yml b/.github/workflows/engineer-bot-followup.yml index 080beb52b..11dd06037 100644 --- a/.github/workflows/engineer-bot-followup.yml +++ b/.github/workflows/engineer-bot-followup.yml @@ -99,6 +99,12 @@ jobs: uses: ./.github/actions/setup-poetry with: python-version: '3.11' + # Match code-coverage.yml's e2e job: install optional extras (notably + # pyarrow, declared optional=true behind the `pyarrow` extra) so the + # agent's REQUIRED live E2E repro test runs against the same runtime as + # the e2e suite it mirrors. Without this, arrow-touching e2e tests + # ModuleNotFoundError/skip instead of failing red-for-the-right-reason. + install-args: "--all-extras" # setup-poetry runs `poetry lock` (to reconcile the lock with the internal # JFrog source it injects), which REWRITES tracked poetry.lock / pyproject.toml diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 3f3886319..4910a9d25 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -80,6 +80,12 @@ jobs: uses: ./.github/actions/setup-poetry with: python-version: '3.11' + # Match code-coverage.yml's e2e job: install optional extras (notably + # pyarrow, declared optional=true behind the `pyarrow` extra) so the + # agent's REQUIRED live E2E repro test runs against the same runtime as + # the e2e suite it mirrors. Without this, arrow-touching e2e tests + # ModuleNotFoundError/skip instead of failing red-for-the-right-reason. + install-args: "--all-extras" # setup-poetry runs `poetry lock` (to reconcile the lock with the internal # JFrog source it injects), which REWRITES tracked poetry.lock / pyproject.toml From b93b5276d8ff1622ef6fd72ed558895a8e30e20a Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:04:20 +0000 Subject: [PATCH 03/11] ai: apply changes for #870 (1 review thread) Addresses: - #3600282324 at .bot/prompts/engineer/system.md:33 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer/system.md | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index 44f298a31..502f55a08 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -30,9 +30,21 @@ Tests live under `tests/`: - `tests/unit/` — fast, fully MOCKED, no network. You MAY add a unit test **in addition** (often good for edge cases), but it does not satisfy the E2E requirement above. - If the behavior genuinely cannot be observed end-to-end through the connector - against the live warehouse, report `blocked` and explain why — do **not** - substitute a unit test. + There is ONE carve-out. Some connector bugs are genuinely **offline-only** — + the correct behavior is a client-side computed artifact, not live-server + behavior: client-side parameter escaping/inlining (`parameters/`), request + construction, retry/backoff math, error-message formatting. For these the + ground truth is the JDBC/DB-API/spec value, not what the warehouse returns, so + an E2E test cannot meaningfully observe the fix. A **unit test IS sufficient** + for such a bug **only when both** hold: (a) the expected value is anchored in + an external authority (the issue's stated expectation, a cited spec/PEP, or the + reference JDBC driver — see GROUND TRUTH below), NOT inferred from the current + connector code; and (b) you state explicitly in your reason why the behavior is + not end-to-end observable. Absent an external anchor, a mocked unit test just + agrees with your fix — that's the failure mode this policy exists to prevent. + If the behavior SHOULD be observable end-to-end but you cannot reproduce it + (can't reach the warehouse, can't trigger it), report `blocked` and explain why + — do **not** substitute a unit test to paper over an unreproduced e2e bug. Read `tests/e2e/` for the established patterns (fixtures, the `self.connection(...)` / cursor helpers, naming, assertions) and match them. Read `CONTRIBUTING.md` for From c4f3c76097469de817673ffc3672ef3aab432039 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:15:41 +0000 Subject: [PATCH 04/11] ai: apply changes for #870 (2 review threads) Addresses: - #3600313097 at .github/workflows/engineer-bot-followup.yml:155 - #3600313099 at .github/workflows/engineer-bot.yml:88 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer/system.md | 11 +++++++++++ .github/workflows/engineer-bot-followup.yml | 10 +++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index 502f55a08..a27cc0027 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -78,6 +78,17 @@ warehouse connection env is set. Run tests through poetry: - Your E2E test (fastest loop): `poetry run python -m pytest tests/e2e/ -k ` - A unit test: `poetry run python -m pytest tests/unit/ -k ` +**This runner installs `--all-extras`, so the REAL `databricks-sql-kernel` wheel +is present.** The unit suite fakes `databricks_sql_kernel` in `sys.modules` +(`tests/unit/test_kernel_client.py`), which shadows the real wheel in a shared +session — and the `@pytest.mark.realkernel` routing test +(`tests/unit/test_session.py::TestUseKernelRoutesThroughRealWheel`) `pytest.fail`s +loudly when it detects that shadowing. So whenever you run a BROADER unit +selection than a single `-k` test — a whole file, or `tests/unit` — append +`-m "not realkernel"` (matching how `.github/workflows/code-coverage.yml` guards +the same `--all-extras` install). Skipping this produces a confusing false red +that has nothing to do with your fix. + **Always `-k`-filter to your own test** — do NOT run the whole `tests/e2e` suite: this job provides a live connection but does not seed the full per-run fixture set the broader suite expects, so unrelated E2E tests would fail or skip and that noise diff --git a/.github/workflows/engineer-bot-followup.yml b/.github/workflows/engineer-bot-followup.yml index 11dd06037..4974841ac 100644 --- a/.github/workflows/engineer-bot-followup.yml +++ b/.github/workflows/engineer-bot-followup.yml @@ -148,9 +148,13 @@ jobs: TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} - # Live-warehouse connection env so a followup fixup can also run the - # REQUIRED E2E repro/verification test (see engineer/system.md). Mirrors - # the author run + code-coverage.yml; job is in `environment: azure-prod`. + # Live-warehouse connection env, provisioned for parity with the author + # run + code-coverage.yml (job is in `environment: azure-prod`). NOTE: + # unlike the author phase, the followup prompt + # (.bot/prompts/engineer-followup/system.md) tells the agent to run only + # the mocked `tests/unit` suite and NOT tests/e2e. These vars are wired + # for consistency with the author run, not an E2E step the followup runs; + # if followups should ever repair/run E2E repros, update that prompt too. DATABRICKS_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_HOST }} DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} DATABRICKS_CATALOG: peco From baa3103c9f84ebf49df0e89499fc07ec8ff0cb36 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:22:33 +0000 Subject: [PATCH 05/11] ai: apply changes for #870 (1 review thread) Addresses: - #3600339404 at .github/workflows/engineer-bot-followup.yml:107 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer-followup/system.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.bot/prompts/engineer-followup/system.md b/.bot/prompts/engineer-followup/system.md index 92461e073..1ad18b335 100644 --- a/.bot/prompts/engineer-followup/system.md +++ b/.bot/prompts/engineer-followup/system.md @@ -14,6 +14,16 @@ Your job: pass: `poetry run python -m pytest tests/unit/ -k ` (and the affected file's full set before you finish). Never weaken or skip a test to go green. + - This runner installs `--all-extras`, so the REAL `databricks-sql-kernel` + wheel is present. The unit suite fakes `databricks_sql_kernel` in + `sys.modules` (`tests/unit/test_kernel_client.py`), which shadows the + real wheel in a shared session — and the `@pytest.mark.realkernel` + routing test (`tests/unit/test_session.py::TestUseKernelRoutesThroughRealWheel`) + `pytest.fail`s loudly on that shadowing. So whenever you run a selection + BROADER than a single `-k` test — a whole file, or `tests/unit` — append + `-m "not realkernel"` (matching how `.github/workflows/code-coverage.yml` + guards the same `--all-extras` install). Skipping this produces a + confusing false red unrelated to your fix. 4. End with a short summary of what changed. Repo facts you need: From 048eef6f7bb7530463261008ec9fa97d3fa07b8d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:28:39 +0000 Subject: [PATCH 06/11] ai: apply changes for #870 (1 review thread) Addresses: - #3600361719 at .bot/prompts/engineer/system.md:98 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer/system.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index a27cc0027..460a1994e 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -122,6 +122,10 @@ up whatever it needs. wrong fix looks green. - Keep the change minimal and scoped to the bug. Don't refactor unrelated code or restyle files you happened to open. +- **Write boundary.** `.git/` and `.gitleaksignore` are denied paths (they return + "Path denied or invalid"). While `.github/`, `.bot/`, and `pyproject.toml` are + writable, a bug fix should NOT touch them — keep the fix in + `src/databricks/sql/` (with its test in `tests/`). - Match the surrounding code and follow `CONTRIBUTING.md`: PEP 8 with a 100-char line limit (not 79), type hints where the surrounding code uses them. - **Batch tool calls.** When you need several files or greps, issue them ALL in one From 2a474fdd493eacb5e33ef48ceb7b0a13d6993030 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:36:14 +0000 Subject: [PATCH 07/11] ai: apply changes for #870 (1 review thread) Addresses: - #3600385831 at .github/workflows/engineer-bot-followup.yml:158 Signed-off-by: peco-engineer-bot[bot] --- .github/workflows/engineer-bot-followup.yml | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/.github/workflows/engineer-bot-followup.yml b/.github/workflows/engineer-bot-followup.yml index 4974841ac..24c47ce31 100644 --- a/.github/workflows/engineer-bot-followup.yml +++ b/.github/workflows/engineer-bot-followup.yml @@ -148,17 +148,14 @@ jobs: TRIGGER_COMMENT_ID: ${{ github.event.comment.id }} MODEL_ENDPOINT: https://${{ secrets.DATABRICKS_HOST }}/serving-endpoints/databricks-claude-opus-4-8/invocations DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} - # Live-warehouse connection env, provisioned for parity with the author - # run + code-coverage.yml (job is in `environment: azure-prod`). NOTE: - # unlike the author phase, the followup prompt - # (.bot/prompts/engineer-followup/system.md) tells the agent to run only - # the mocked `tests/unit` suite and NOT tests/e2e. These vars are wired - # for consistency with the author run, not an E2E step the followup runs; - # if followups should ever repair/run E2E repros, update that prompt too. - DATABRICKS_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_HOST }} - DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} - DATABRICKS_CATALOG: peco - DATABRICKS_USER: ${{ secrets.TEST_PECO_SP_ID }} + # NOTE: no live-warehouse connection env (DATABRICKS_SERVER_HOSTNAME / + # DATABRICKS_HTTP_PATH / DATABRICKS_CATALOG / DATABRICKS_USER) is wired + # here. The followup prompt (.bot/prompts/engineer-followup/system.md) + # runs only the mocked `tests/unit` suite and forbids tests/e2e, so the + # agent never consumes those vars — provisioning them would add live + # credentials to an LLM-driven step for no functional benefit. If + # followups should ever repair/run E2E repros, add them back here AND + # update that prompt. RUNNER_TEMP: ${{ runner.temp }} # The agent's working tree AND the .bot/ lookup root. run.py resolves # the config at /.bot/config.yaml. The engine has NO path From 7c0ca1b720fc8570ceab2a86096aa4e80a0ac5df Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 04:40:43 +0000 Subject: [PATCH 08/11] ai: apply changes for #870 (1 review thread) Addresses: - #3600405525 at .github/workflows/engineer-bot-followup.yml:104 Signed-off-by: peco-engineer-bot[bot] --- .github/workflows/engineer-bot-followup.yml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/engineer-bot-followup.yml b/.github/workflows/engineer-bot-followup.yml index 24c47ce31..c29a06ff8 100644 --- a/.github/workflows/engineer-bot-followup.yml +++ b/.github/workflows/engineer-bot-followup.yml @@ -99,11 +99,15 @@ jobs: uses: ./.github/actions/setup-poetry with: python-version: '3.11' - # Match code-coverage.yml's e2e job: install optional extras (notably - # pyarrow, declared optional=true behind the `pyarrow` extra) so the - # agent's REQUIRED live E2E repro test runs against the same runtime as - # the e2e suite it mirrors. Without this, arrow-touching e2e tests - # ModuleNotFoundError/skip instead of failing red-for-the-right-reason. + # Match code-coverage.yml's --all-extras install so the agent's mocked + # `tests/unit` self-verify runs against the same runtime as CI. The key + # extra here is the REAL `databricks-sql-kernel` wheel: with it present, + # broader unit selections must pass `-m "not realkernel"` (see the + # followup prompt, .bot/prompts/engineer-followup/system.md), otherwise + # the realkernel routing test fails loudly on the sys.modules fake that + # shadows the wheel. (Unlike engineer-bot.yml, this job runs NO e2e test + # — it wires no live-warehouse env, per the NOTE below — so the e2e + # repro is NOT the reason extras are installed here.) install-args: "--all-extras" # setup-poetry runs `poetry lock` (to reconcile the lock with the internal From c4e2dc0681f61f1b3f2ff91f64b87e917ce51bca Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Thu, 16 Jul 2026 23:47:00 -0700 Subject: [PATCH 09/11] docs(bots): teach backend selection (Thrift/SEA/kernel) + realkernel tiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the #870 review thread on --all-extras: the bot needs to know, per issue, WHICH backend the bug is on and reproduce on that one — a Thrift bug won't reproduce on a kernel connection, and a broad unit run with the real kernel wheel present false-reds unless realkernel is deselected. That knowledge was tribal; write it down. - CONTRIBUTING.md: add a "Backends and test tiers" section — the three backends (Thrift default / SEA `use_sea=True` / kernel `use_kernel=True`), where each backend's tests live, that kernel is an opt-in extra, and the rule that `realkernel` tests run in their own invocation (`-m "not realkernel"` for broad runs), matching how CI (code-coverage.yml / code-quality-checks.yml) splits them. - engineer/system.md: add step 0 — pick the backend the bug is on and reproduce there; point to the CONTRIBUTING matrix. - engineer-followup/system.md: correct the stale "do NOT run tests/e2e" line (the followup job now has live creds via #870) and point at the same backend matrix. Keeps --all-extras (both backends supported); the residual "prompt-discipline only" risk the reviewer flagged is now backed by a documented, human-shared convention plus explicit bot rules. Signed-off-by: eric-wang-1990 Co-authored-by: Isaac --- .bot/prompts/engineer-followup/system.md | 10 ++++++--- .bot/prompts/engineer/system.md | 21 ++++++++++++++---- CONTRIBUTING.md | 27 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/.bot/prompts/engineer-followup/system.md b/.bot/prompts/engineer-followup/system.md index 1ad18b335..aaf6de34d 100644 --- a/.bot/prompts/engineer-followup/system.md +++ b/.bot/prompts/engineer-followup/system.md @@ -27,9 +27,13 @@ Your job: 4. End with a short summary of what changed. Repo facts you need: - - `poetry`-managed, Python 3.8+; `poetry install` has run on the runner, so - `poetry run python -m pytest tests/unit` runs the fully-mocked unit suite - with no warehouse. Do NOT run or add `tests/e2e` (needs live credentials). + - `poetry`-managed, Python 3.8+; `poetry install --all-extras` has run on the + runner, and the live-warehouse connection env is set — so both + `poetry run python -m pytest tests/unit` (mocked) AND `tests/e2e` (live) are + runnable. If a reviewer's ask is about connector behavior that only an E2E test + can verify, add/adjust a `tests/e2e` test on the RIGHT backend for the change + (Thrift default / `use_sea=True` / `use_kernel=True` — see CONTRIBUTING.md → + "Backends and test tiers"), don't settle for a mocked unit test. - Source is under `src/databricks/sql/`; unit tests under `tests/unit/`. Follow `CONTRIBUTING.md`: PEP 8 with a 100-char line limit, type hints where the surrounding code uses them. This is a widely-consumed connector — keep diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index 460a1994e..a789d8e8d 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -97,11 +97,24 @@ up whatever it needs. == HOW TO WORK (bug-fix flow) == +0. **Pick the BACKEND the bug is on — reproduce on that one.** The connector has + three backends and a bug on one won't reproduce on another. Choose from the + issue: + - **kernel** (`use_kernel=True`) — only if the issue is specifically about the + Rust kernel / `use_kernel`. Repro in `tests/e2e/test_kernel_backend.py` and run + it alone (it needs the real wheel; see the `-m "not realkernel"` note above). + - **SEA** (`use_sea=True`) — if the issue implicates SEA, or the area is + backend-parametrized (add the `{"use_sea": True}` param case). + - **Thrift** (the default, no kwarg) — everything else; this is the common case. + See CONTRIBUTING.md → "Backends and test tiers" for the full matrix (selection + kwarg + where each backend's tests live). + 1. **Write the failing E2E test FIRST — before you deep-dive the fix.** Your first - substantive action is a `tests/e2e/` test that REPRODUCES the bug. Do only the - minimal reading needed to write it (find the API to call + how the e2e tests - connect). Run it with `-k` and confirm it **fails for the right reason** (the - bug — not a compile/setup/skip). A *skipped* test is not a reproduction. + substantive action is a `tests/e2e/` test (on the backend from step 0) that + REPRODUCES the bug. Do only the minimal reading needed to write it (find the API + to call + how the e2e tests connect). Run it with `-k` and confirm it **fails for + the right reason** (the bug — not a compile/setup/skip). A *skipped* test is not + a reproduction. - **Reproduction is a HARD GATE.** If after a focused effort (a few attempts, not dozens) you cannot get a test that fails for the right reason — it only skips, you can't reach the warehouse, or you can't trigger the bug — **STOP diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0cb258769..a0646d7b9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,6 +144,33 @@ The `PySQLStagingIngestionTestSuite` namespace requires a cluster running DBR ve The suites marked `[not documented]` require additional configuration which will be documented at a later time. +#### Backends and test tiers + +The connector has **three execution backends**, selected per connection. When you +reproduce or fix a bug, use the backend the bug is actually on — a Thrift bug won't +reproduce on a kernel connection, and vice versa: + +| Backend | Select via (connect kwarg / `extra_params`) | Where its tests live | +| --- | --- | --- | +| **Thrift** (default) | *(nothing — the default path)* | the general `tests/e2e` suite (the `{}` parametrize case) and mocked `tests/unit` | +| **SEA** | `use_sea=True` | the general `tests/e2e` suite (the `{"use_sea": True}` parametrize case) | +| **Kernel** (Rust, optional) | `use_kernel=True` | the dedicated `tests/e2e/test_kernel_backend.py` / `test_kernel_tls.py`, plus the offline routing test `tests/unit/test_session.py -m realkernel` | + +Notes that matter when running the suite: + +- **Kernel is an opt-in extra**, not part of the default install. `use_kernel=True` + needs `pip install "databricks-sql-connector[kernel]"` (or `poetry install + --all-extras`); without it the connector raises a clear "install the `[kernel]` + extra" error. Most bugs are on the **Thrift/SEA** path — reproduce those there; + only reach for kernel when the issue is specifically about `use_kernel`. +- **`realkernel` tests must run in their own pytest invocation.** When the real + kernel wheel is installed (`--all-extras`), several unit tests fake + `databricks_sql_kernel` in `sys.modules`; the `@pytest.mark.realkernel` guard test + detects that shadowing and **fails loudly**. So a *broad* unit run with the wheel + present must **deselect it**: `poetry run python -m pytest tests/unit -m "not + realkernel"`, and run the real-wheel tests separately with `-m realkernel` (this is + exactly how CI splits them — see `code-coverage.yml` / `code-quality-checks.yml`). + ### Code formatting From 55d1f12cc6dd294792a2063f66f10c370d8954b9 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 06:55:24 +0000 Subject: [PATCH 10/11] ai: apply changes for #870 (2 review threads) Addresses: - #3600996714 at .github/workflows/engineer-bot.yml:200 - #3601002346 at CONTRIBUTING.md:156 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer/system.md | 8 ++++++++ CONTRIBUTING.md | 5 ++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.bot/prompts/engineer/system.md b/.bot/prompts/engineer/system.md index a789d8e8d..2c10da0e7 100644 --- a/.bot/prompts/engineer/system.md +++ b/.bot/prompts/engineer/system.md @@ -89,6 +89,14 @@ selection than a single `-k` test — a whole file, or `tests/unit` — append the same `--all-extras` install). Skipping this produces a confusing false red that has nothing to do with your fix. +**Only the `default` schema is provisioned for this job.** The e2e connection env +sets `DATABRICKS_SCHEMA` implicitly to `"default"` (it is intentionally left +unset, mirroring `code-coverage.yml`, so the `schema` fixture falls back to +`"default"`). Write your repro against the `default` schema — do NOT assume a +seeded/non-default schema (e.g. staging-ingestion / UC-volume style tests, which +also require `ingestion_user`), or the test will confusingly fail on a missing +schema rather than on the bug. + **Always `-k`-filter to your own test** — do NOT run the whole `tests/e2e` suite: this job provides a live connection but does not seed the full per-run fixture set the broader suite expects, so unrelated E2E tests would fail or skip and that noise diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a0646d7b9..1b9b09acb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -146,14 +146,13 @@ The suites marked `[not documented]` require additional configuration which will #### Backends and test tiers -The connector has **three execution backends**, selected per connection. When you +The connector has **two execution backends**, selected per connection. When you reproduce or fix a bug, use the backend the bug is actually on — a Thrift bug won't reproduce on a kernel connection, and vice versa: | Backend | Select via (connect kwarg / `extra_params`) | Where its tests live | | --- | --- | --- | | **Thrift** (default) | *(nothing — the default path)* | the general `tests/e2e` suite (the `{}` parametrize case) and mocked `tests/unit` | -| **SEA** | `use_sea=True` | the general `tests/e2e` suite (the `{"use_sea": True}` parametrize case) | | **Kernel** (Rust, optional) | `use_kernel=True` | the dedicated `tests/e2e/test_kernel_backend.py` / `test_kernel_tls.py`, plus the offline routing test `tests/unit/test_session.py -m realkernel` | Notes that matter when running the suite: @@ -161,7 +160,7 @@ Notes that matter when running the suite: - **Kernel is an opt-in extra**, not part of the default install. `use_kernel=True` needs `pip install "databricks-sql-connector[kernel]"` (or `poetry install --all-extras`); without it the connector raises a clear "install the `[kernel]` - extra" error. Most bugs are on the **Thrift/SEA** path — reproduce those there; + extra" error. Most bugs are on the **Thrift** path — reproduce those there; only reach for kernel when the issue is specifically about `use_kernel`. - **`realkernel` tests must run in their own pytest invocation.** When the real kernel wheel is installed (`--all-extras`), several unit tests fake From 6e31ad5dea5f231cf7be614304e954e533760fe7 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 17 Jul 2026 07:02:33 +0000 Subject: [PATCH 11/11] ai: apply changes for #870 (2 review threads) Addresses: - #3601040104 at .bot/prompts/engineer-followup/system.md:31 - #3601040111 at .bot/prompts/engineer/system.md:121 Signed-off-by: peco-engineer-bot[bot] --- .bot/prompts/engineer-followup/system.md | 11 +++++------ CONTRIBUTING.md | 5 +++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.bot/prompts/engineer-followup/system.md b/.bot/prompts/engineer-followup/system.md index aaf6de34d..3d95f2bbc 100644 --- a/.bot/prompts/engineer-followup/system.md +++ b/.bot/prompts/engineer-followup/system.md @@ -28,12 +28,11 @@ Your job: Repo facts you need: - `poetry`-managed, Python 3.8+; `poetry install --all-extras` has run on the - runner, and the live-warehouse connection env is set — so both - `poetry run python -m pytest tests/unit` (mocked) AND `tests/e2e` (live) are - runnable. If a reviewer's ask is about connector behavior that only an E2E test - can verify, add/adjust a `tests/e2e` test on the RIGHT backend for the change - (Thrift default / `use_sea=True` / `use_kernel=True` — see CONTRIBUTING.md → - "Backends and test tiers"), don't settle for a mocked unit test. + runner, but this follow-up job wires NO live-warehouse connection env — so + only `poetry run python -m pytest tests/unit` (fully mocked) runs here. Do + NOT run or add `tests/e2e` (needs live credentials this job does not have). + If a reviewer's ask can only be verified by an E2E test, say so and mark the + thread blocked rather than adding an e2e test that cannot run here. - Source is under `src/databricks/sql/`; unit tests under `tests/unit/`. Follow `CONTRIBUTING.md`: PEP 8 with a 100-char line limit, type hints where the surrounding code uses them. This is a widely-consumed connector — keep diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b9b09acb..cf5b6210b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -146,13 +146,14 @@ The suites marked `[not documented]` require additional configuration which will #### Backends and test tiers -The connector has **two execution backends**, selected per connection. When you +The connector has **three execution backends**, selected per connection. When you reproduce or fix a bug, use the backend the bug is actually on — a Thrift bug won't -reproduce on a kernel connection, and vice versa: +reproduce on a SEA or kernel connection, and vice versa: | Backend | Select via (connect kwarg / `extra_params`) | Where its tests live | | --- | --- | --- | | **Thrift** (default) | *(nothing — the default path)* | the general `tests/e2e` suite (the `{}` parametrize case) and mocked `tests/unit` | +| **SEA** (Statement Execution API) | `use_sea=True` | the general `tests/e2e` suite (the `{"use_sea": True}` parametrize case, e.g. `tests/e2e/test_driver.py`) and mocked `tests/unit` | | **Kernel** (Rust, optional) | `use_kernel=True` | the dedicated `tests/e2e/test_kernel_backend.py` / `test_kernel_tls.py`, plus the offline routing test `tests/unit/test_session.py -m realkernel` | Notes that matter when running the suite: