feat(engineer-bot): require a live E2E repro for bug fixes (not unit-only)

[eric-wang-1990]

Why

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 (the retry max→min change) 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 literally said "treat the unit suite as your only executable verification" — the opposite of the sibling adbc-drivers/databricks bot, which requires a live E2E repro and treats reproduction as a hard gate.

Same engine flow: bug-fix in both repos; the discipline lives in the consumer prompt + whether the workflow passes live-warehouse creds. This ports adbc's proven model here.

What

Prompt (.bot/prompts/engineer/system.md):

• A tests/e2e/ test that reproduces the bug (red) against the live warehouse and passes (green) post-fix is REQUIRED; a mocked unit test alone is not sufficient. blocked (not a unit substitute) if the behavior genuinely isn't e2e-observable.
• Test-first, hard gate — blocked if it can't fail-for-the-right-reason after a focused effort.
• Don't rewrite existing tests to agree with the fix (the Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860) #868 failure mode) — add a new failing test; justify any existing-assertion change.
• Ground truth in an external authority (issue/spec, or the JDBC reference driver via context-repo) — not the current connector code.
• 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.

Cost note

In-loop e2e adds live-warehouse time (~10 min) + occasional 429 flakiness per bug-fix run; the blocked-if-can't-repro hard gate bounds wasted effort.

Related

The engine-side callout (bot-engine: consumers whose bug-fix flow requires E2E must wire the connection secrets into the run steps) is a separate PR in databricks-bot-engine.

This pull request and its description were written by Isaac.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-motivated change — the four connection vars map correctly to the secrets conftest.py/the e2e suite read, and mirror code-coverage.yml's env exactly. Two concerns worth addressing: (1) medium — the e2e environment doesn't fully mirror code-coverage because the engineer-bot setup-poetry steps omit --all-extras, so pyarrow is absent and arrow-dependent e2e tests (a common bug class) can't run red-for-the-right-reason under the now-mandatory E2E requirement; (2) low — the prompt directs the agent to fetch_context_repo databricks-jdbc, which doesn't appear to be configured anywhere in the repo.

[peco-review-bot]

Verdict: 1 Low

Looks good — config/prompt-only change with no source edits. Verified against conftest.py that the four added env vars (DATABRICKS_SERVER_HOSTNAME/HTTP_PATH/CATALOG/USER) plus the pre-existing DATABRICKS_TOKEN fully satisfy the e2e connection_details fixture, and that the workflow env + --all-extras faithfully mirror code-coverage.yml. One low note on the E2E-only hard gate potentially over-blocking legitimately offline-only bug fixes.

[peco-review-bot]

Verdict: 2 Low

Looks good — the change is a prompt/workflow policy update to require live E2E repros for bug fixes, and it's well-reasoned and heavily documented. Env var names match conftest.py and code-coverage.yml exactly, --all-extras is a valid setup-poetry input, and the realkernel marker/test class both exist as referenced. Two low operational notes below; no correctness blockers.

[peco-review-bot]

Verdict: 1 Medium

Solid, well-documented change. The live-warehouse env var names in engineer-bot.yml are verified correct against conftest.py and mirror code-coverage.yml (DATABRICKS_TOKEN is already present for the model endpoint and doubles as the e2e access token). One medium concern: the --all-extras justification comment in the followup workflow was copy-pasted and describes an e2e rationale that contradicts that job's own no-e2e design.

[peco-review-bot]

Verdict: 1 Low

Solid, well-documented prompt/workflow change that ports the adbc "require a live E2E repro" discipline here. I verified the connection-env mapping is complete: the e2e suite reads exactly DATABRICKS_SERVER_HOSTNAME/DATABRICKS_HTTP_PATH/DATABRICKS_TOKEN/DATABRICKS_USER/DATABRICKS_CATALOG via the root conftest.py, and the author step now provides all five (DATABRICKS_TOKEN was already present), matching code-coverage.yml. No correctness blockers — one low-severity footgun worth noting on the followup workflow.

[eric-wang-1990]

Re: the --all-extras / realkernel blocked thread (now resolved) — addressed in c4e2dc0. The residual 'prompt-discipline only' risk is now backed by documented convention + explicit bot rules: a CONTRIBUTING.md → "Backends and test tiers" section (Thrift default / use_sea=True / use_kernel=True, where each backend's tests live, kernel as an opt-in extra, and -m "not realkernel" for broad runs — matching code-coverage.yml / code-quality-checks.yml), plus a backend-selection step 0 in engineer/system.md and a pointer in engineer-followup/system.md (whose stale 'don't run tests/e2e' line is also corrected now that the followup job has live creds). Keeping --all-extras so kernel-backend bugs remain reproducible.

[peco-review-bot]

Verdict: 1 Low

Looks good — the functional change (wiring the four live-warehouse env vars into engineer-bot.yml's author step) is correct: the names/secrets exactly match what conftest.py's fixtures read and what code-coverage.yml's e2e job uses, and DATABRICKS_TOKEN was already present. The --all-extras install + -m "not realkernel" guidance is consistent between both workflows and their prompts, and the follow-up job correctly documents why it deliberately withholds live creds. One low note inline about the unset DATABRICKS_SCHEMA fallback. Nit: the RUNNING TESTS line in engineer/system.md still says "poetry install has already run" though the runner now uses --all-extras; the following paragraph clarifies this, so it's cosmetic.

[peco-review-bot]

Verdict: 1 High · 1 Medium

Prompt/workflow docs PR. Two genuine internal contradictions introduced here: (1) the follow-up prompt now advertises a live warehouse + runnable e2e, but the follow-up workflow deliberately wires no e2e env and its NOTE says the opposite (High); (2) both prompts reference a use_sea=True third backend and point at the new CONTRIBUTING "Backends and test tiers" matrix, which documents only two backends and omits SEA (Medium). The engineer-bot.yml env wiring itself correctly mirrors code-coverage.yml.

[peco-review-bot]

✅ No issues identified by the review bot.