From c048d2ef79c2ceacc4a39d7cc6c1cf297e4d3528 Mon Sep 17 00:00:00 2001 From: eric-wang-1990 Date: Fri, 17 Jul 2026 13:30:16 -0700 Subject: [PATCH] fix(bots): give the engineer-bot e2e repro a token the env scrub can't strip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bug-fix flow's REQUIRED tests/e2e repro authenticates through conftest.py, which reads the token from DATABRICKS_TOKEN. But the engineer-bot runs pytest in an agent-driven subprocess whose environment is scrubbed of every credential- shaped variable (the engine's shared/env_scrub.py strips *TOKEN*/*SECRET*/... ), so DATABRICKS_TOKEN is gone before pytest starts. The connection is then built with access_token=None and hangs — observed on issue #791: every e2e test (incl. existing, unmodified ones) stalled ~10 min per attempt until the 45-min job timeout. The same warehouse/env is used by code-coverage.yml's e2e job, which passes because it runs pytest directly (no agent, no scrub). Fix (per the engine's centralize-bot-workflows design doc): pass the connection details through a file instead of a credential-named env var. - engineer-bot.yml: a new step writes host/http_path/token/user/catalog to $RUNNER_TEMP/e2e-connection.json (built with jq so the secret is never shell- interpolated; chmod 600) and exports its PATH to the author step as DATABRICKS_TEST_CONFIG_FILE — a name env_scrub deliberately preserves. - conftest.py: each connection fixture now falls back to that file when its env var is absent (env var still wins). Normal CI and local dev leave the path var unset, so the file dict is empty and behavior is byte-for-byte unchanged. Author phase only: the follow-up phase deliberately runs mocked tests/unit and forbids tests/e2e, so it needs no live token (left untouched). Signed-off-by: eric-wang-1990 Co-authored-by: Isaac --- .github/workflows/engineer-bot.yml | 36 +++++++++++++++++++ conftest.py | 57 +++++++++++++++++++++++------- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/.github/workflows/engineer-bot.yml b/.github/workflows/engineer-bot.yml index 4910a9d25..cbaf5c1de 100644 --- a/.github/workflows/engineer-bot.yml +++ b/.github/workflows/engineer-bot.yml @@ -173,6 +173,36 @@ jobs: fi { echo "ISSUE_TITLE<<$DELIM"; printf '%s\n' "$TITLE"; echo "$DELIM"; } >> "$GITHUB_ENV" + - name: Write E2E connection config (token survives the agent env scrub) + id: e2e_config + # The bug-fix flow's REQUIRED tests/e2e repro authenticates via the repo's + # conftest, which reads DATABRICKS_TOKEN. But the author agent runs pytest + # in a subprocess whose env is scrubbed of every credential-shaped var + # (engine shared/env_scrub.py strips *TOKEN*/*SECRET*/…), so DATABRICKS_TOKEN + # never reaches the test — the connection then has access_token=None and + # hangs. Workaround (per the engine's centralize-bot-workflows design doc): + # write the connection details to a file and pass its PATH in + # DATABRICKS_TEST_CONFIG_FILE, a var the scrub deliberately preserves; the + # conftest reads it as a fallback. Written here (an ordinary step, token in + # scope) with jq so the secret is never shell-interpolated. + env: + DATABRICKS_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_HOST }} + DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} + DATABRICKS_TOKEN: ${{ secrets.DATABRICKS_TOKEN }} + DATABRICKS_USER: ${{ secrets.TEST_PECO_SP_ID }} + run: | + CONFIG_PATH="$RUNNER_TEMP/e2e-connection.json" + jq -n \ + --arg host "$DATABRICKS_SERVER_HOSTNAME" \ + --arg http_path "$DATABRICKS_HTTP_PATH" \ + --arg access_token "$DATABRICKS_TOKEN" \ + --arg ingestion_user "$DATABRICKS_USER" \ + --arg catalog "peco" \ + '{host: $host, http_path: $http_path, access_token: $access_token, ingestion_user: $ingestion_user, catalog: $catalog}' \ + > "$CONFIG_PATH" + chmod 600 "$CONFIG_PATH" + echo "path=$CONFIG_PATH" >> "$GITHUB_OUTPUT" + - name: Run author id: author # Run from RUNNER_TEMP so the engine-rendered prompt's context file @@ -201,6 +231,12 @@ jobs: DATABRICKS_HTTP_PATH: ${{ secrets.TEST_PECO_WAREHOUSE_HTTP_PATH }} DATABRICKS_CATALOG: peco DATABRICKS_USER: ${{ secrets.TEST_PECO_SP_ID }} + # The connection TOKEN can't ride a plain env var into the agent's test + # subprocess (env_scrub strips *TOKEN*). Pass the path to the config file + # written above instead — this var name survives the scrub, and the + # conftest reads access_token from it. (host/http_path/catalog/user are + # not credential-shaped, so they still arrive via the env vars above.) + DATABRICKS_TEST_CONFIG_FILE: ${{ steps.e2e_config.outputs.path }} REPO_ROOT: ${{ github.workspace }} RUNNER_TEMP: ${{ runner.temp }} FLOW: ${{ steps.ctx.outputs.flow }} diff --git a/conftest.py b/conftest.py index c8b350bee..0927aced3 100644 --- a/conftest.py +++ b/conftest.py @@ -1,35 +1,68 @@ +import json import os + import pytest +def _test_config_from_file(): + """Connection details from the JSON file named by ``DATABRICKS_TEST_CONFIG_FILE``, + or ``{}`` when the variable is unset/empty/unreadable. + + Why this indirection exists: the engineer-bot (databricks-bot-engine) runs + this e2e suite inside an agent-driven subprocess whose environment has every + credential-shaped variable — anything matching ``*TOKEN*`` / ``*SECRET*`` / + ``*PASSWORD*`` etc. — stripped for safety (the engine's ``shared/env_scrub.py``). + ``DATABRICKS_TOKEN`` is therefore removed before pytest starts, so the token + can't reach the agent's test run as a plain env var. The bot instead writes the + connection details (token included) to a file and points at it with + ``DATABRICKS_TEST_CONFIG_FILE`` — a name the scrub deliberately preserves. + + Normal CI (and local dev) leaves the variable unset, so this returns ``{}`` and + every fixture below resolves purely from its env var, unchanged. + """ + path = os.getenv("DATABRICKS_TEST_CONFIG_FILE") + if not path: + return {} + try: + with open(path) as f: + return json.load(f) + except (OSError, ValueError): + return {} + + +@pytest.fixture(scope="session") +def _test_config(): + return _test_config_from_file() + + @pytest.fixture(scope="session") -def host(): - return os.getenv("DATABRICKS_SERVER_HOSTNAME") +def host(_test_config): + return os.getenv("DATABRICKS_SERVER_HOSTNAME") or _test_config.get("host") @pytest.fixture(scope="session") -def http_path(): - return os.getenv("DATABRICKS_HTTP_PATH") +def http_path(_test_config): + return os.getenv("DATABRICKS_HTTP_PATH") or _test_config.get("http_path") @pytest.fixture(scope="session") -def access_token(): - return os.getenv("DATABRICKS_TOKEN") +def access_token(_test_config): + return os.getenv("DATABRICKS_TOKEN") or _test_config.get("access_token") @pytest.fixture(scope="session") -def ingestion_user(): - return os.getenv("DATABRICKS_USER") +def ingestion_user(_test_config): + return os.getenv("DATABRICKS_USER") or _test_config.get("ingestion_user") @pytest.fixture(scope="session") -def catalog(): - return os.getenv("DATABRICKS_CATALOG") +def catalog(_test_config): + return os.getenv("DATABRICKS_CATALOG") or _test_config.get("catalog") @pytest.fixture(scope="session") -def schema(): - return os.getenv("DATABRICKS_SCHEMA", "default") +def schema(_test_config): + return os.getenv("DATABRICKS_SCHEMA") or _test_config.get("schema") or "default" @pytest.fixture(scope="session", autouse=True)