diff --git a/agents/build-repair/Dockerfile b/agents/build-repair/Dockerfile index e1890dc740..13b18719ce 100644 --- a/agents/build-repair/Dockerfile +++ b/agents/build-repair/Dockerfile @@ -17,6 +17,18 @@ RUN --mount=type=cache,target=/root/.cache/uv \ --mount=type=bind,target=/app,rw \ uv sync --locked --no-dev --no-editable --package hackbot-agent-build-repair +# treeherder-cli (MPL-2.0) lets the agent query Firefox CI: what failed on a push, +# when a failure started, how often a job passes. Built from source because upstream +# only ships prebuilt Linux binaries for x86_64, and `cargo install` also works for an +# arm64 developer build. A bookworm builder is safe against either python:3.12 base +# (same glibc, or older building for newer); rustls means no OpenSSL to link. +FROM rust:1-slim-bookworm AS treeherder-cli +ARG TREEHERDER_CLI_VERSION=0.2.17 +RUN --mount=type=cache,target=/usr/local/cargo/registry \ + --mount=type=cache,target=/build/target \ + CARGO_TARGET_DIR=/build/target \ + cargo install treeherder-cli --version ${TREEHERDER_CLI_VERSION} --locked --root /out + FROM python:3.12 AS base COPY --from=builder /opt/venv /opt/venv @@ -36,6 +48,12 @@ RUN useradd --create-home --shell /bin/bash agent \ && mkdir -p /workspace \ && chown agent:agent /workspace +# Not ~/.cargo/bin, where `mach bootstrap` installs the Rust toolchain at runtime: +# that directory is rustup-managed and inside the agent's HOME, so a root-owned copy +# made at build time does not belong in it. /usr/local/bin is on PATH for every user +# and nothing at runtime writes to it. +COPY --from=treeherder-cli /out/bin/treeherder-cli /usr/local/bin/treeherder-cli + # `mach bootstrap` installs the toolchain here at runtime; put it on PATH so the # agent's own `./mach build` (and the build_firefox tool) find rustc/clang. ENV PATH="/home/agent/.cargo/bin:/home/agent/.mozbuild/clang/bin:${PATH}" diff --git a/agents/build-repair/hackbot_agents/build_repair/__main__.py b/agents/build-repair/hackbot_agents/build_repair/__main__.py index 64f0eda165..ffa37b7eee 100644 --- a/agents/build-repair/hackbot_agents/build_repair/__main__.py +++ b/agents/build-repair/hackbot_agents/build_repair/__main__.py @@ -2,7 +2,7 @@ from pydantic_settings import BaseSettings, SettingsConfigDict from .agent import BuildRepairResult, run_build_repair -from .resolve import resolve_git_commits +from .resolve import resolve_push class AgentInputs(BaseSettings): @@ -32,7 +32,8 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: # The first is the failure commit the tree is checked out at; the rest let # the agent blame the culprit. task_id = next(iter(inputs.failure_tasks.values())) - git_commits = resolve_git_commits(task_id, inputs.git_commit) + push = resolve_push(task_id, inputs.git_commit) + git_commits = push.git_commits # Pin the checkout to the failure commit and fetch deep enough to include the # whole push, so the agent can `git show` every commit in it. @@ -47,6 +48,8 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: fx_ctx=ctx.firefox, bug_id=inputs.bug_id, git_commits=git_commits, + project=push.project, + hg_revision=push.hg_revision, failure_tasks=inputs.failure_tasks, run_try_push=inputs.run_try_push, model=inputs.model, diff --git a/agents/build-repair/hackbot_agents/build_repair/agent.py b/agents/build-repair/hackbot_agents/build_repair/agent.py index ad644238b3..d4fc64c16b 100644 --- a/agents/build-repair/hackbot_agents/build_repair/agent.py +++ b/agents/build-repair/hackbot_agents/build_repair/agent.py @@ -33,7 +33,6 @@ ToolUseBlock, UserMessage, ) -from hackbot_agents.build_repair.logs import download_failure_logs from hackbot_agents.build_repair.try_push import TRY_TOOLS from hackbot_runtime import AgentError, HackbotAgentResult from hackbot_runtime.claude import Reporter @@ -56,8 +55,11 @@ FIX_TEMPLATE, PUSH_COMMIT_LINE, PUSH_CONTEXT, + TREEHERDER_STEP, + TREEHERDER_STEP_NO_PUSH, TRY_PUSH_INSTRUCTIONS, ) +from .resolve import task_push TARGET_SOFTWARE = "Mozilla Firefox" @@ -143,6 +145,8 @@ async def run_build_repair( fx_ctx: FirefoxContext, bug_id: int | None = None, git_commits: list[str], + project: str | None = None, + hg_revision: str | None = None, failure_tasks: dict[str, str], run_try_push: bool = False, model: str | None = None, @@ -163,18 +167,9 @@ async def run_build_repair( print(f"[build_repair] repairing {label} at {failure_commit}", file=sys.stderr) scratch_dir = Path(tempfile.mkdtemp(prefix=f"build-repair-{bug_id or 'nobug'}-")) - scratch_in = scratch_dir / "in" scratch_out = scratch_dir / "out" - scratch_in.mkdir(parents=True, exist_ok=True) scratch_out.mkdir(parents=True, exist_ok=True) - task_logs = await download_failure_logs(failure_tasks, scratch_in) - failure_logs = "\n".join( - f"- {name}: sanitized errors at {tl.sanitized} (start here); " - f"full log at {tl.full}" - for name, tl in task_logs.items() - ) - firefox_tools = [*firefox.TOOLS, *TRY_TOOLS] if run_try_push else firefox.TOOLS firefox_server = build_sdk_server("firefox", fx_ctx, firefox_tools) mcp_servers: dict[str, McpServerConfig] = { @@ -189,12 +184,30 @@ async def run_build_repair( ] task_name = next(iter(failure_tasks), "") + if not (project and hg_revision) and failure_tasks: + # The eval harness drives the agent from git commits alone, but the logs + # only come from Treeherder, so the push has to be resolved either way. + try: + project, hg_revision = task_push(next(iter(failure_tasks.values()))) + except Exception: + print("[build_repair] could not resolve the push", file=sys.stderr) + treeherder_step = ( + TREEHERDER_STEP.format( + project=project, + hg_revision=hg_revision, + task_name=task_name, + scratch_out=scratch_out, + ) + if project and hg_revision + else TREEHERDER_STEP_NO_PUSH.format(scratch_out=scratch_out) + ) analysis_prompt = ANALYSIS_TEMPLATE.format( target_software=TARGET_SOFTWARE, git_commit=failure_commit, + source_repo=source_repo, push_context=_push_context(git_commits), + treeherder_step=treeherder_step, blame_step=_blame_step(git_commits, scratch_out), - failure_logs=failure_logs, scratch_out=scratch_out, bug_context=BUG_CONTEXT.format(bug_id=bug_id) if bug_id is not None else "", bug_step=BUG_ANALYSIS_STEP.format(bug_id=bug_id) if bug_id is not None else "", @@ -202,6 +215,7 @@ async def run_build_repair( ) fix_prompt = FIX_TEMPLATE.format( target_software=TARGET_SOFTWARE, + source_repo=source_repo, scratch_out=scratch_out, try_push=( TRY_PUSH_INSTRUCTIONS.format(task_name=task_name) if run_try_push else "" @@ -231,6 +245,7 @@ async def run_build_repair( reporter, analysis_opts, analysis_prompt, captured, tracked ) _check(result_msg, label, "analysis") + _check_blocked(scratch_out) total_cost += result_msg.total_cost_usd or 0.0 total_turns += result_msg.num_turns or 0 @@ -316,6 +331,26 @@ async def _run_session( return result_msg +def _check_blocked(scratch_out: Path) -> None: + """Fail the run unless the agent actually retrieved a failure log. + + Analysing a failure whose log never arrived produces a confident verdict + invented from the diff alone (bug 6665), so a missing log is an error rather + than a verdict, and the error reaches the API and the UI. The agent is asked to + report the blocker itself in error.txt; the log check is what makes it a + guarantee rather than an instruction. + """ + blocker = scratch_out / "error.txt" + if blocker.exists(): + raise AgentError(blocker.read_text().strip() or "agent reported it was blocked") + logs = scratch_out / "logs" + if not any(logs.glob("**/*live_backing_log*")): + raise AgentError( + "no failure log was retrieved: treeherder-cli left nothing under " + f"{logs}, so any verdict would be guesswork" + ) + + def _check(result_msg: ResultMessage | None, label: str, stage: str) -> None: if result_msg is None: raise AgentError(f"{label}: {stage} stage produced no result message") diff --git a/agents/build-repair/hackbot_agents/build_repair/logs.py b/agents/build-repair/hackbot_agents/build_repair/logs.py deleted file mode 100644 index 52c8ff932f..0000000000 --- a/agents/build-repair/hackbot_agents/build_repair/logs.py +++ /dev/null @@ -1,106 +0,0 @@ -# -*- coding: utf-8 -*- -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -"""Download and sanitize Taskcluster build-failure logs. - -The agent is given a mapping of ``task-name -> Taskcluster task ID``. Before the -Claude SDK is invoked we fetch each task's latest-run ``live_backing.log`` and -write two files to the scratch dir: the full log and a sanitized companion that keeps only -the ``ERROR -`` / ``FATAL -`` lines. The agent is told to start from the -sanitized log (so its context isn't drowned by tens of MB of build output) and -fall back to the full log for surrounding detail. -""" - -from __future__ import annotations - -import asyncio -import logging -import re -from pathlib import Path -from typing import NamedTuple - -import requests - -logger = logging.getLogger(__name__) - -# Use the queue's getLatestArtifact endpoint (no run index): it 303-redirects to -# the most recent run's artifact, so we get the failing run's log even when the -# task was retried (run 0 was an infra exception with no live_backing.log, etc.). -ARTIFACT_URL = ( - "https://firefox-ci-tc.services.mozilla.com/api/queue/v1/" - "task/{task_id}/artifacts/public/logs/live_backing.log" -) -_HEADERS = {"User-Agent": "hackbot-build-repair/1.0"} -_TIMEOUT = 120 -_MAX_LINES = 2000 - -_ERROR_RE = re.compile(r"(?:ERROR|FATAL) -") - - -class TaskLogs(NamedTuple): - """Paths to the two log files written for one failing task.""" - - sanitized: Path - full: Path - - -def _safe_filename(task_name: str) -> str: - return re.sub(r"[^A-Za-z0-9._-]+", "_", task_name).strip("_") or "task" - - -def sanitize_log(text: str) -> str: - """Keep only ``ERROR -`` / ``FATAL -`` lines, deduping consecutive repeats and capping size.""" - kept: list[str] = [] - previous: str | None = None - for line in text.splitlines(): - if not _ERROR_RE.search(line): - continue - stripped = line.rstrip() - if stripped == previous: - continue - previous = stripped - kept.append(stripped) - if len(kept) >= _MAX_LINES: - kept.append(f"... (truncated at {_MAX_LINES} error lines)") - break - return "\n".join(kept) - - -def _fetch_and_write(task_name: str, task_id: str, dest_dir: Path) -> TaskLogs: - safe = _safe_filename(task_name) - full_path = dest_dir / f"{safe}.log" - sanitized_path = dest_dir / f"{safe}.errors.txt" - url = ARTIFACT_URL.format(task_id=task_id) - try: - resp = requests.get(url, headers=_HEADERS, timeout=_TIMEOUT) - resp.raise_for_status() - full_path.write_text(resp.text) - sanitized = sanitize_log(resp.text) - sanitized_path.write_text( - sanitized if sanitized else f"(no ERROR -/FATAL - lines matched in {url})\n" - ) - except requests.exceptions.RequestException as exc: - logger.warning("Failed to download log for %s (%s): %s", task_name, url, exc) - note = f"(failed to download {url}: {exc})\n" - full_path.write_text(note) - sanitized_path.write_text(note) - return TaskLogs(sanitized=sanitized_path, full=full_path) - - -async def download_failure_logs( - failure_tasks: dict[str, str], dest_dir: Path -) -> dict[str, TaskLogs]: - """Download the full log and write a sanitized companion for each task concurrently. - - Returns a mapping of task name to its :class:`TaskLogs` (sanitized + full paths). - """ - names = list(failure_tasks) - logs = await asyncio.gather( - *( - asyncio.to_thread(_fetch_and_write, name, failure_tasks[name], dest_dir) - for name in names - ) - ) - return dict(zip(names, logs)) diff --git a/agents/build-repair/hackbot_agents/build_repair/prompts.py b/agents/build-repair/hackbot_agents/build_repair/prompts.py index 947a9e7941..0438b5cc81 100644 --- a/agents/build-repair/hackbot_agents/build_repair/prompts.py +++ b/agents/build-repair/hackbot_agents/build_repair/prompts.py @@ -7,14 +7,15 @@ ANALYSIS_TEMPLATE = """You are an expert {target_software} engineer tasked with analyzing and fixing a build failure. -Investigate why the {target_software} build broke at commit {git_commit}. The source tree -is already checked out at that commit (your working directory). +Investigate why the {target_software} build broke at commit {git_commit}. The source +tree is at {source_repo} (your working directory), checked out at that commit. Stay +in it: write scratch files by absolute path rather than `cd`-ing elsewhere. If a +command reports "not a git repository" you have moved -- run `git -C {source_repo}` +rather than hunting for the tree. {push_context}{bug_context} Analyze the following: 1. The git diff of commit {git_commit} (use `git show {git_commit}`). -{bug_step}{logs_num}. The Taskcluster build failure logs. Each failing task has a sanitized log (only the ERROR -/FATAL - lines) and the full log. Start from the sanitized log -- it usually pinpoints the failing file and line. The full log can be tens of thousands of lines, so grep it for that file/line rather than reading it sequentially: -{failure_logs} - +{bug_step}{logs_num}.{treeherder_step} Create these documents: 1. {scratch_out}/analysis.md -- the developer's reference, readable in under a minute. Under 40 lines, in exactly these sections, each a short paragraph or a @@ -44,6 +45,65 @@ PUSH_COMMIT_LINE = "- {commit}" +TREEHERDER_STEP = r"""\ + The build failure logs, via `treeherder-cli`. This push is {project} revision + {hg_revision}, and the failing task is '{task_name}'. Start with the error lines: + `treeherder-cli {hg_revision} --repo {project} --filter '{task_name}' + --include-intermittent --fetch-logs --pattern '\b(?:ERROR|FATAL) -' + --cache-dir {scratch_out}/logs | head -100` + Anchor short patterns on a word boundary: bare `ERROR -` also matches inside + `-DHAVE_STRERROR -D...` and buries the real errors in compiler command lines. + Each hit prints as `live_backing_log:` and the full logs stay under + {scratch_out}/logs, where those line numbers apply, so read a window around one + to get the whole diagnostic: + `sed -n '165280,165320p' {scratch_out}/logs/job_/live_backing_log.log | cut -c1-200` + An `ERROR -` line is usually only the first line of a compiler error -- the + offending source and the `^` caret follow it. Never read or cat a whole log; + they run to six figures of lines. + Clip the width as well as the line count: log lines run to thousands of + characters, so append `| cut -c1-200` to any grep or sed over a log -- 40 wpt + lines alone came to 38 KB without it. + Two things can stop that command finding the job, and both are recoverable: + - Treeherder sometimes returns a malformed response ("error decoding response + body"). Retry the same command once; it usually succeeds. + - "N passing jobs ... no failures found" almost always means + `--include-intermittent` was missing: a failure a sheriff has already + classified is hidden without it, and that covers most of them. Check the flag + is there and re-run. + Once the retry has also failed there is nothing to analyse. Write what you ran + and what it reported to {scratch_out}/error.txt and stop: do not write the other + documents, guess a cause, or propose a fix from the diff alone. Do not fetch the + artifact from Taskcluster yourself either: after a retry or rerun the latest + artifact can be a passing run's log, so a wrong log is worse than none. The run + is meant to fail here. + + The same command answers CI questions about the push. `--compare ` + says whether the failure is new here or was already failing earlier; + `--lookback 50 --suspects` finds the push window a failure started in; + `--similar-history ` gives a job's recent pass rate, which separates a + real bustage from infrastructure flakiness. + + Every revision you pass has to be a real hg revision that treeherder-cli + printed. It rejects anything else with "No push found for revision" -- both the + git shas of the push commits above and placeholders like `parent`. To compare + against a neighbouring push, get its revision from `--context 3` first, which + lists the pushes either side of this one, then pass that to `--compare`. + + `--help` lists the rest. The default markdown output is the compact one -- only + add `--json` when you will parse it. To see more of a log widen `--pattern`. + Always pass `--filter` and pipe through `head`: unfiltered, one push can print + hundreds of megabytes straight into your context. Never pass `--watch` or + `--stream-failures` -- they block until CI finishes. +""" + + +TREEHERDER_STEP_NO_PUSH = """\ + This push could not be resolved on Treeherder, so the failure logs cannot be + reached. Write that to {scratch_out}/error.txt and stop: do not write the other + documents or propose a fix from the diff alone. +""" + + BLAME_STEP = """4. {scratch_out}/blame.json naming the commit that introduced the failure, as JSON: {{"blamed_commit": "", "reason": ""}}. Use one of the push commits listed above when there are several, otherwise the checked-out @@ -66,7 +126,17 @@ 1. {scratch_out}/analysis.md -- your analysis of what caused the failure 2. {scratch_out}/planning.md -- your fixing plan -Edit the source files in the working directory to repair the build. A mozconfig +Edit the source files in {source_repo} (your working directory) to repair the build. +Editing: use Edit on a file that already exists -- Write refuses until the file has +been read, which costs a turn. To see how a commit handled comparable files, run +`git show -- ` rather than guessing a sibling's name. + +Working in this tree: review your own edits with `git diff --stat` and `git diff -- +`, never `git status` -- after a build the objdir adds millions of untracked +files and the output runs to tens of MB. Logs already fetched sit under +{scratch_out}/logs; when you grep or sed one, cap the width as well as the line +count (`| head -40 | cut -c1-200`), because a single build-log line can be 10 KB. + A mozconfig that mirrors the failing CI configuration (release milestone, warnings-as-errors) is already set up. Verify the fix compiles with the build_firefox tool, passing the directory of the file you changed as `target` (e.g. 'docshell/base') for a diff --git a/agents/build-repair/hackbot_agents/build_repair/resolve.py b/agents/build-repair/hackbot_agents/build_repair/resolve.py index 2488a96243..430fc04b6c 100644 --- a/agents/build-repair/hackbot_agents/build_repair/resolve.py +++ b/agents/build-repair/hackbot_agents/build_repair/resolve.py @@ -15,6 +15,7 @@ from __future__ import annotations import logging +from dataclasses import dataclass import requests @@ -71,17 +72,37 @@ def _push_git_commits(project: str, rev: str) -> list[str]: return commits -def resolve_git_commits(task_id: str, git_commit: str | None = None) -> list[str]: - """Resolve a failing task into its push commits, failure commit first. +@dataclass(frozen=True) +class PushInfo: + """The push a failing task belongs to. + + ``project`` and ``hg_revision`` are what Treeherder is keyed on, so they are + kept alongside the git commits rather than discarded after the lookup. + """ + + project: str | None + hg_revision: str | None + git_commits: list[str] + + +def task_push(task_id: str) -> tuple[str | None, str | None]: + """The ``(project, hg_revision)`` a task ran on; what Treeherder is keyed on.""" + task = _get_json(_TC_TASK_URL.format(task_id=task_id)) + tags = task.get("tags") or {} + return ( + tags.get("project"), + (task.get("payload") or {}).get("env", {}).get("GECKO_HEAD_REV"), + ) + + +def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: + """Resolve a failing task into its push, failure commit first. ``git_commit`` overrides the failure commit (skipping the lando lookup); the task is still fetched for its revision. Raises on network errors or when the failure commit cannot be determined. """ - task = _get_json(_TC_TASK_URL.format(task_id=task_id)) - tags = task.get("tags") or {} - project = tags.get("project") - hg_rev = (task.get("payload") or {}).get("env", {}).get("GECKO_HEAD_REV") + project, hg_rev = task_push(task_id) push = _push_git_commits(project, hg_rev) if hg_rev and project else [] @@ -93,4 +114,8 @@ def resolve_git_commits(task_id: str, git_commit: str | None = None) -> list[str ) failure_commit = _hg_to_git(hg_rev) - return [failure_commit] + [c for c in push if c != failure_commit] + return PushInfo( + project=project, + hg_revision=hg_rev, + git_commits=[failure_commit] + [c for c in push if c != failure_commit], + ) diff --git a/agents/test-repair/Dockerfile b/agents/test-repair/Dockerfile index a03d3ca373..2bb4958feb 100644 --- a/agents/test-repair/Dockerfile +++ b/agents/test-repair/Dockerfile @@ -17,6 +17,18 @@ RUN --mount=type=cache,target=/root/.cache/uv \ --mount=type=bind,target=/app,rw \ uv sync --locked --no-dev --no-editable --package hackbot-agent-test-repair +# treeherder-cli (MPL-2.0) lets the agent query Firefox CI: what failed on a push, +# when a failure started, how often a job passes. Built from source because upstream +# only ships prebuilt Linux binaries for x86_64, and `cargo install` also works for an +# arm64 developer build. A bookworm builder is safe against either python:3.12 base +# (same glibc, or older building for newer); rustls means no OpenSSL to link. +FROM rust:1-slim-bookworm AS treeherder-cli +ARG TREEHERDER_CLI_VERSION=0.2.17 +RUN --mount=type=cache,target=/usr/local/cargo/registry \ + --mount=type=cache,target=/build/target \ + CARGO_TARGET_DIR=/build/target \ + cargo install treeherder-cli --version ${TREEHERDER_CLI_VERSION} --locked --root /out + FROM python:3.12 AS base COPY --from=builder /opt/venv /opt/venv @@ -56,6 +68,12 @@ RUN useradd --create-home --shell /bin/bash agent \ && mkdir -p /tmp/.X11-unix \ && chmod 1777 /tmp/.X11-unix +# Not ~/.cargo/bin, where `mach bootstrap` installs the Rust toolchain at runtime: +# that directory is rustup-managed and inside the agent's HOME, so a root-owned copy +# made at build time does not belong in it. /usr/local/bin is on PATH for every user +# and nothing at runtime writes to it. +COPY --from=treeherder-cli /out/bin/treeherder-cli /usr/local/bin/treeherder-cli + # `mach bootstrap` installs the toolchain here at runtime; put it on PATH so the # agent's own `./mach build` (and the build_firefox tool) find rustc/clang. ENV PATH="/home/agent/.cargo/bin:/home/agent/.mozbuild/clang/bin:${PATH}" diff --git a/agents/test-repair/hackbot_agents/test_repair/__main__.py b/agents/test-repair/hackbot_agents/test_repair/__main__.py index 347c36f94c..44bfd5bb58 100644 --- a/agents/test-repair/hackbot_agents/test_repair/__main__.py +++ b/agents/test-repair/hackbot_agents/test_repair/__main__.py @@ -8,7 +8,6 @@ from .agent import TestRepairResult from .config import SKIP_FIREFOX_BUILD, SLACK_CHANNEL -from .logs import download_failure_logs from .notify import build_message, resolve_culprit_author, sheriff_action_required from .resolve import Investigation, resolve_investigation @@ -45,14 +44,9 @@ async def main(ctx: HackbotContext) -> TestRepairResult: investigation: Investigation = resolve_investigation(task_id) scratch_dir = Path(tempfile.mkdtemp(prefix="test-repair-")) - scratch_in = scratch_dir / "in" scratch_out = scratch_dir / "out" - scratch_in.mkdir(parents=True, exist_ok=True) scratch_out.mkdir(parents=True, exist_ok=True) - logger.info("Downloading failure logs for %d task(s)", len(inputs.failure_tasks)) - task_logs = await download_failure_logs(inputs.failure_tasks, scratch_in) - bugzilla_mcp_server = ( {"type": "http", "url": inputs.bugzilla_mcp_url} if inputs.bugzilla_mcp_url @@ -67,7 +61,6 @@ async def main(ctx: HackbotContext) -> TestRepairResult: source_repo=source_repo, fx_ctx=ctx.firefox, investigation=investigation, - task_logs=task_logs, scratch_out=scratch_out, skip_firefox_build=inputs.skip_firefox_build, model=inputs.model, diff --git a/agents/test-repair/hackbot_agents/test_repair/agent.py b/agents/test-repair/hackbot_agents/test_repair/agent.py index f3df3ed521..ed400abea9 100644 --- a/agents/test-repair/hackbot_agents/test_repair/agent.py +++ b/agents/test-repair/hackbot_agents/test_repair/agent.py @@ -42,15 +42,12 @@ FIX_MODEL, SKIP_FIREFOX_BUILD, ) -from .logs import TaskLogs from .prompts import ( ANALYSIS_TEMPLATE, - CANDIDATE_INTRO_COMPLETE, - CANDIDATE_INTRO_PARTIAL, + CANDIDATE_INTRO, ENVIRONMENT_NOTE, FIX_TEMPLATE, KNOWN_INTERMITTENTS_LINE, - LAST_GREEN_LINE, MAX_CANDIDATE_COMMITS, MAX_TESTS_PER_GROUP, VERIFY_LOCAL, @@ -71,7 +68,6 @@ class TestRepairResult(HackbotAgentResult): candidate_commits: list[str] = [] culprit_bug: int | None = None confidence: float = 0.0 - last_green_revision: str | None = None intermittent_bug: int | None = None proposed_patch: bool = False summary: str = "" @@ -115,6 +111,26 @@ async def _run_session( return result_msg +def _check_blocked(scratch_out: Path) -> None: + """Fail the run unless the agent actually retrieved a failure log. + + Analysing a failure whose log never arrived produces a confident verdict + invented from the diff alone (bug 6665), so a missing log is an error rather + than a verdict, and the error reaches the API and the UI. The agent is asked to + report the blocker itself in error.txt; the log check is what makes it a + guarantee rather than an instruction. + """ + blocker = scratch_out / "error.txt" + if blocker.exists(): + raise AgentError(blocker.read_text().strip() or "agent reported it was blocked") + logs = scratch_out / "logs" + if not any(logs.glob("**/*live_backing_log*")): + raise AgentError( + "no failure log was retrieved: treeherder-cli left nothing under " + f"{logs}, so any verdict would be guesswork" + ) + + def _check(result_msg: ResultMessage | None, stage: str) -> None: if result_msg is None: raise AgentError(f"{stage} stage produced no result message") @@ -247,7 +263,6 @@ def _assemble_result( *, verdict: dict, source_repo: Path, - last_green_revision: str | None, total_turns: int, total_cost: float, publish_file: Callable[[str, Path, str | None], str] | None, @@ -267,7 +282,6 @@ def _assemble_result( culprit_bug=_as_int(verdict.get("culprit_bug")), intermittent_bug=_as_int(verdict.get("intermittent_bug")), confidence=_as_float(verdict.get("confidence")), - last_green_revision=last_green_revision, proposed_patch=bool(verdict.get("proposed_patch")), summary=_read_doc(scratch_out, "summary", publish_file), analysis=_read_doc(scratch_out, "analysis", publish_file), @@ -277,9 +291,7 @@ def _assemble_result( def _range_expr(commit_range: CommitRange) -> str: - """A git revision range for ``git log``, falling back to the clone depth.""" - if commit_range.base: - return f"{commit_range.base}..{commit_range.head}" + """The range for `git log`, relative to the checked-out head.""" return f"HEAD~{commit_range.span}..HEAD" @@ -308,7 +320,6 @@ async def run_test_repair( source_repo: Path, fx_ctx: FirefoxContext, investigation: Investigation, - task_logs: dict[str, TaskLogs], scratch_out: Path, skip_firefox_build: bool = SKIP_FIREFOX_BUILD, model: str | None = None, @@ -334,16 +345,6 @@ async def run_test_repair( mcp_servers["bugzilla"] = bugzilla_mcp_server allowed_tools += BUGZILLA_READ_TOOLS - failure_logs = "\n".join( - f"- {name}: sanitized failures at {tl.sanitized} (start here); " - f"full log at {tl.full}" - for name, tl in task_logs.items() - ) - last_green_line = ( - LAST_GREEN_LINE.format(last_green_revision=investigation.last_green_revision) - if investigation.last_green_revision - else "" - ) known_intermittents_line = ( KNOWN_INTERMITTENTS_LINE.format( bugs=", ".join(str(bug) for bug in investigation.known_intermittent_bugs) @@ -352,22 +353,21 @@ async def run_test_repair( else "" ) range_expr = _range_expr(commit_range) - intro = ( - CANDIDATE_INTRO_COMPLETE if commit_range.complete else CANDIDATE_INTRO_PARTIAL - ) analysis_prompt = ANALYSIS_TEMPLATE.format( failing_tests=_failing_tests(investigation), harness=investigation.harness, platform=investigation.platform or "unknown", label=investigation.label or "unknown", + project=investigation.project, + hg_revision=investigation.hg_revision, source_repo=source_repo, failure_commit=failure_commit, - candidate_intro=intro.format(commit_range=range_expr, span=commit_range.span), + candidate_intro=CANDIDATE_INTRO.format( + commit_range=range_expr, span=commit_range.span + ), commit_range=range_expr, max_candidates=MAX_CANDIDATE_COMMITS, - last_green_line=last_green_line, known_intermittents_line=known_intermittents_line, - failure_logs=failure_logs, scratch_out=scratch_out, ) @@ -393,6 +393,7 @@ async def run_test_repair( ) result_msg = await _run_session(reporter, analysis_opts, analysis_prompt) _check(result_msg, "analysis") + _check_blocked(scratch_out) total_cost += result_msg.total_cost_usd or 0.0 total_turns += result_msg.num_turns or 0 @@ -444,7 +445,6 @@ async def run_test_repair( scratch_out, verdict=verdict, source_repo=source_repo, - last_green_revision=investigation.last_green_revision, total_turns=total_turns, total_cost=total_cost, publish_file=publish_file, diff --git a/agents/test-repair/hackbot_agents/test_repair/logs.py b/agents/test-repair/hackbot_agents/test_repair/logs.py deleted file mode 100644 index bebbc6cb03..0000000000 --- a/agents/test-repair/hackbot_agents/test_repair/logs.py +++ /dev/null @@ -1,103 +0,0 @@ -# -*- coding: utf-8 -*- -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -"""Download and sanitize a failing test task's log. - -Before invoking Claude we fetch each failing task's latest-run -``live_backing.log`` and write two files to the scratch dir: the full log and a -sanitized companion that keeps only the interesting lines -- the test harness's -``TEST-UNEXPECTED-*`` result lines plus ``ERROR -`` / ``FATAL -`` lines. The -agent starts from the sanitized log so its context isn't drowned by tens of MB -of output, and falls back to the full log for surrounding detail. -""" - -from __future__ import annotations - -import asyncio -import logging -import re -from pathlib import Path -from typing import NamedTuple - -import requests - -logger = logging.getLogger(__name__) - -ARTIFACT_URL = ( - "https://firefox-ci-tc.services.mozilla.com/api/queue/v1/" - "task/{task_id}/artifacts/public/logs/live_backing.log" -) -_HEADERS = {"User-Agent": "hackbot-test-repair/1.0"} -_TIMEOUT = 120 -_MAX_LINES = 2000 - -_INTERESTING_RE = re.compile(r"TEST-UNEXPECTED-|(?:ERROR|FATAL) -") - - -class TaskLogs(NamedTuple): - """Paths to the two log files written for one failing task.""" - - sanitized: Path - full: Path - - -def _safe_filename(task_name: str) -> str: - return re.sub(r"[^A-Za-z0-9._-]+", "_", task_name).strip("_") or "task" - - -def sanitize_log(text: str) -> str: - """Keep only failure/error lines, deduping consecutive repeats and capping size.""" - kept: list[str] = [] - previous: str | None = None - for line in text.splitlines(): - if not _INTERESTING_RE.search(line): - continue - stripped = line.rstrip() - if stripped == previous: - continue - previous = stripped - kept.append(stripped) - if len(kept) >= _MAX_LINES: - kept.append(f"... (truncated at {_MAX_LINES} lines)") - break - return "\n".join(kept) - - -def _fetch_and_write(task_name: str, task_id: str, dest_dir: Path) -> TaskLogs: - safe = _safe_filename(task_name) - full_path = dest_dir / f"{safe}.log" - sanitized_path = dest_dir / f"{safe}.failures.txt" - url = ARTIFACT_URL.format(task_id=task_id) - try: - resp = requests.get(url, headers=_HEADERS, timeout=_TIMEOUT) - resp.raise_for_status() - full_path.write_text(resp.text) - sanitized = sanitize_log(resp.text) - sanitized_path.write_text( - sanitized if sanitized else f"(no failure lines matched in {url})\n" - ) - except requests.exceptions.RequestException as exc: - logger.warning("Failed to download log for %s (%s): %s", task_name, url, exc) - note = f"(failed to download {url}: {exc})\n" - full_path.write_text(note) - sanitized_path.write_text(note) - return TaskLogs(sanitized=sanitized_path, full=full_path) - - -async def download_failure_logs( - failure_tasks: dict[str, str], dest_dir: Path -) -> dict[str, TaskLogs]: - """Download and sanitize each task's log concurrently. - - Returns a mapping of task name to its :class:`TaskLogs`. - """ - names = list(failure_tasks) - logs = await asyncio.gather( - *( - asyncio.to_thread(_fetch_and_write, name, failure_tasks[name], dest_dir) - for name in names - ) - ) - return dict(zip(names, logs)) diff --git a/agents/test-repair/hackbot_agents/test_repair/notify.py b/agents/test-repair/hackbot_agents/test_repair/notify.py index bba498a058..82e89b5ed2 100644 --- a/agents/test-repair/hackbot_agents/test_repair/notify.py +++ b/agents/test-repair/hackbot_agents/test_repair/notify.py @@ -113,13 +113,10 @@ def _jobs_line(investigation: Investigation, task_id: str) -> str: def _push_line(investigation: Investigation) -> str: - line = ( + return ( f"Push: {investigation.project} {_hg_link(investigation.hg_revision)}" f" / {_commit_link(investigation.failure_commit)}" ) - if investigation.last_green_revision: - line += f", last green {_hg_link(investigation.last_green_revision)}" - return line def _culprit_line(result: TestRepairResult, culprit_author: str | None) -> str: diff --git a/agents/test-repair/hackbot_agents/test_repair/prompts.py b/agents/test-repair/hackbot_agents/test_repair/prompts.py index 60ae2c306e..421df27cea 100644 --- a/agents/test-repair/hackbot_agents/test_repair/prompts.py +++ b/agents/test-repair/hackbot_agents/test_repair/prompts.py @@ -21,15 +21,61 @@ to this configuration rather than to the platform. The source tree is at {source_repo} (your working directory), checked out at the -failure commit {failure_commit}. Scratch and log paths are outside it. Search it -with `git grep`, never `grep -r`, which hits the Bash timeout on a tree this big. +failure commit {failure_commit}. Stay in it: write scratch files by absolute path +rather than `cd`-ing elsewhere, or the next git command fails with "not a git +repository". Search the tree with `git grep`, never `grep -r`, which hits the Bash +timeout on a tree this big. {candidate_intro} -{last_green_line}{known_intermittents_line} -Failure logs (start with the sanitized failures file, fall back to the full log): -{failure_logs} - -Steps: -1. Read the sanitized failure lines to see exactly how the test failed. +{known_intermittents_line}Steps: +1. See exactly how the test failed. `treeherder-cli` queries Firefox CI directly, + which is stronger evidence than the logs alone. This failure is on {project} at + hg revision {hg_revision} -- an hg node, so pass it to treeherder-cli, never to + git: + `treeherder-cli {hg_revision} --repo {project} --filter '{label}' + --include-intermittent --fetch-logs --pattern 'TEST-UNEXPECTED-' + --cache-dir {scratch_out}/logs | head -100` + Anchor short patterns on a word boundary: bare `ERROR -` also matches inside + `-DHAVE_STRERROR -D...`. Each hit prints as `live_backing_log:` and the + full logs stay under {scratch_out}/logs, where those line numbers apply, so read + a window around one to get the assertion, stack or diff that follows: + `sed -n '5890,5930p' {scratch_out}/logs/job_/live_backing_log.log | cut -c1-200` + Never read or cat a whole log; they run to six figures of lines. + Clip the width as well as the line count: log lines run to thousands of + characters, so append `| cut -c1-200` to any grep or sed over a log -- 40 wpt + lines alone came to 38 KB without it. + Two things can stop that command finding the job, and both are recoverable: + - Treeherder sometimes returns a malformed response ("error decoding response + body"). Retry the same command once; it usually succeeds. + - "N passing jobs ... no failures found" almost always means + `--include-intermittent` was missing: a failure a sheriff has already + classified is hidden without it, and that covers most of them. Check the flag + is there and re-run. + Once the retry has also failed there is nothing to analyse. Write what you ran + and what it reported to {scratch_out}/error.txt and stop: do not write the other + documents, guess a culprit, or reason from the diffs alone. Do not fetch the + artifact from Taskcluster yourself either: after a retry or rerun the latest + artifact can be a passing run's log, so a wrong log is worse than none. The run + is meant to fail here. + + The same command answers the rest of the CI questions: + - `--lookback 50 --suspects` -- the push window each failure started in, with + the last push it passed on. Use it when the range above does not reach back to + a green run; it finds the first failing push even when this one is not the + culprit. + - `--similar-history ` -- the job's recent pass rate. A low one means + intermittent; a job that passed consistently until now points at a regression. + - `--group-by test` -- whether these tests fail on other platforms or only here. + - `--compare ` -- whether the failure is new relative to another push. + Every revision you pass has to be a real hg revision that treeherder-cli + printed. It rejects anything else with "No push found for revision" -- both git + shas and placeholders like `parent`. To compare against a neighbouring push, get + its revision from `--context 3` first, which lists the pushes either side of + this one, then pass that to `--compare`. + `--help` lists the rest. The default markdown output is the compact one -- only + add `--json` when you will parse it. To see more of a log widen `--pattern`. + Always pass `--filter` and pipe through `head`: unfiltered, one push can print + hundreds of megabytes straight into your context. Never pass `--watch` or + `--stream-failures` -- they block until CI finishes. 2. Enumerate the candidates with `git log --oneline {commit_range}`. Path filtering on the failing test and the source it exercises tells you what to `git show` first, but never clears anyone: build config, shared headers, @@ -81,20 +127,12 @@ Do not edit any source files in this step. """ -CANDIDATE_INTRO_COMPLETE = """\ -The {span} commits in `{commit_range}` landed since this test was last green -- -the culprit is one of them.""" - -CANDIDATE_INTRO_PARTIAL = """\ -`{commit_range}` is the {span} most recent commits, and is NOT known to reach back -to a green run, so the culprit may predate it. If nothing in it plausibly caused -the failure, say so and leave "culprit_commit" null and "candidate_commits" empty -rather than naming the least implausible commit.""" - -LAST_GREEN_LINE = ( - "The test was last green at hg revision {last_green_revision} (not a git" - " object; the base of the range above is its git equivalent).\n" -) +CANDIDATE_INTRO = """\ +`{commit_range}` is the {span} most recent commits. It is not known to reach back +to a green run, so the culprit may predate it -- `--suspects` below settles that. +If nothing in it plausibly caused the failure, say so and leave "culprit_commit" +null and "candidate_commits" empty rather than naming the least implausible +commit.""" KNOWN_INTERMITTENTS_LINE = ( "Treeherder matches these failure lines to these bugs: {bugs}. Check whether" @@ -111,6 +149,15 @@ The source tree is at {source_repo} (your working directory). Search it with `git grep`, never `grep -r`. +Editing: use Edit on a file that already exists -- Write refuses until the file has +been read, which costs a turn. To see how the culprit handled comparable files, +run `git show {culprit_commit} -- ` rather than guessing a sibling's name. + +Working in this tree: review your own edits with `git diff --stat` and `git diff -- +`, never `git status` -- after a build the objdir adds millions of untracked +files and the output runs to tens of MB. Logs already fetched sit under +{scratch_out}/logs; when you grep or sed one, cap the width as well as the line +count (`| head -40 | cut -c1-200`), because a single build-log line can be 10 KB. 1. Make the smallest change that addresses the root cause. {verify_step} diff --git a/agents/test-repair/hackbot_agents/test_repair/resolve.py b/agents/test-repair/hackbot_agents/test_repair/resolve.py index a4f0971371..8ae7effcf8 100644 --- a/agents/test-repair/hackbot_agents/test_repair/resolve.py +++ b/agents/test-repair/hackbot_agents/test_repair/resolve.py @@ -20,9 +20,7 @@ import mozci.push # noqa: F401 (imported so mozci registers its data sources) import requests from mozci import data -from mozci.errors import ParentPushNotFound -from mozci.push import Push -from mozci.task import Status, is_no_groups_suite +from mozci.task import is_no_groups_suite logger = logging.getLogger(__name__) @@ -42,16 +40,11 @@ _TREEHERDER = "https://treeherder.mozilla.org/api/project" _FAILURE_LINE_PREFIX = "TEST-UNEXPECTED" _INTERMITTENT_KEYWORD = "intermittent-failure" -# The walk stops at the first decisive ancestor, so the depth is only paid in full -# when the task ran on none of them -- a coalesced or low-frequency config, which is -# exactly the case worth reaching. Measured at ~3s per ancestor, so a full-depth walk -# is a few minutes. -LAST_GREEN_MAX_DEPTH = 100 -# Matches the walk: having failed to find green within that many pushes, the blind -# window handed over instead covers the same span rather than a narrower one. -FALLBACK_RANGE_PUSHES = LAST_GREEN_MAX_DEPTH -# Bounds the shallow clone depth. Must stay above what those pushes can hold -# (~1.9 commits/push on autoland) or the cap discards the base found above. +# How far back the candidate window reaches. The agent narrows it itself with +# `treeherder-cli --lookback N --suspects`, which reports the push a failure actually +# started in -- including when that predates the push under investigation. +RANGE_PUSHES = 100 +# Bounds the shallow clone depth, and so the commits the agent can reach. MAX_RANGE_COMMITS = 500 @@ -65,14 +58,10 @@ class FailingGroup: @dataclass(frozen=True) class CommitRange: - """The git commit range to search for the culprit.""" + """The commits to search for the culprit: ``span`` back from ``head``.""" head: str - # Exclusive; None when unknown or outside the cap. - base: str | None span: int - # Whether the culprit is provably inside the range. - complete: bool @dataclass @@ -85,7 +74,6 @@ class Investigation: # Taskcluster ``test-platform`` tag, e.g. "linux1804-64-qr/debug". platform: str failing_groups: list[FailingGroup] - last_green_revision: str | None commit_range: CommitRange # Carries the test variant and chunk, unlike ``platform``. label: str = "" @@ -199,100 +187,6 @@ def _known_intermittent_bugs(project: str, task_id: str) -> list[int]: return bugs -def _same_platform(task, platform: str) -> bool: - return task.platform == platform or platform in (task.label or "") - - -def _test_status(push: Push, group: str, tests: list[str], platform: str) -> str | None: - """'passed'/'failed'/None for ``tests`` of ``group`` on ``platform``. - - Restricted to the failing tests and platform, since ``GroupSummary`` aggregates - across both. None means non-decisive. - """ - summary = push.group_summaries.get(group) - if summary is None: - return None - wanted = set(tests) - statuses = set() - for task in summary.tasks: - if not _same_platform(task, platform): - continue - for result in task.results: - if result.group != group: - continue - if result.ok: - statuses.add("passed") - continue - failed = {test for test, _type in task.failure_types.get(group, [])} - statuses.add("failed" if not wanted or wanted & failed else "passed") - if "failed" in statuses: - return "failed" - return "passed" if statuses else None - - -def _label_status(push: Push, label: str) -> str | None: - """'passed'/'failed'/None for a whole task label, for suites without manifests.""" - summary = push.label_summaries.get(label) - if summary is None: - return None - if summary.status == Status.PASS: - return "passed" - if summary.status == Status.FAIL: - return "failed" - # INTERMITTENT: it both passed and failed here, so it cannot anchor a green. - return None - - -def _walk_ancestors( - branch: str, rev: str, status_of, max_depth: int = LAST_GREEN_MAX_DEPTH -) -> str | None: - """Most recent ancestor revision that ``status_of`` reports as 'passed'. - - Best effort: None when no green ancestor is found within ``max_depth``, the - failure was already there upstream, or mozci errors. - """ - try: - ancestor = Push(rev, branch=branch) - for _ in range(max_depth): - try: - ancestor = ancestor.parent - except ParentPushNotFound: - break - status = status_of(ancestor) - if status == "passed": - return ancestor.rev - if status == "failed": - return None - except Exception: - logger.exception("Could not determine last-green at %s", rev) - return None - - -def _last_green( - branch: str, - rev: str, - failing: FailingGroup, - platform: str, - max_depth: int = LAST_GREEN_MAX_DEPTH, -) -> str | None: - """Most recent ancestor revision where the failing tests were green.""" - return _walk_ancestors( - branch, - rev, - lambda push: _test_status(push, failing.group, failing.tests, platform), - max_depth, - ) - - -def _last_green_label( - branch: str, rev: str, label: str, max_depth: int = LAST_GREEN_MAX_DEPTH -) -> str | None: - """Most recent ancestor revision where the whole failing task was green.""" - return _walk_ancestors( - branch, rev, lambda push: _label_status(push, label), max_depth - ) - - def _count_commits(pushes: dict) -> int: """Total changesets across the pushlog pushes.""" return sum(len(p.get("changesets") or []) for p in pushes.values()) @@ -307,27 +201,22 @@ def _pushlog(pushlog_url: str, query: str) -> dict: return {} -def _fallback_pushes(pushlog_url: str, head_rev: str) -> dict: - """The head push plus the ``FALLBACK_RANGE_PUSHES`` pushes before it.""" +def _range_pushes(pushlog_url: str, head_rev: str) -> dict: + """The head push plus the ``RANGE_PUSHES`` pushes before it.""" head = _pushlog(pushlog_url, f"changeset={head_rev}") if not head: return {} head_id = max(int(push_id) for push_id in head) # startID is exclusive, endID inclusive. - start_id = max(head_id - FALLBACK_RANGE_PUSHES, 0) + start_id = max(head_id - RANGE_PUSHES, 0) return _pushlog(pushlog_url, f"startID={start_id}&endID={head_id}") or head -def _resolve_range( - project: str, - head_rev: str, - last_green_rev: str | None, - max_commits: int, -) -> CommitRange | None: - """Resolve ``(last_green_rev, head_rev]`` into git endpoints and a commit count. +def _resolve_range(project: str, head_rev: str, max_commits: int) -> CommitRange | None: + """The window of commits to search, as a git head plus a depth. - None when the head cannot be mapped. ``base`` is dropped, and the range marked - incomplete, when it is unknown or outside ``max_commits``. + Deliberately open-ended: pinning the base needs a last-green lookup, and the + agent gets a better one on demand from ``treeherder-cli --suspects``. """ head_git = _hg_to_git(head_rev) if not head_git: @@ -335,33 +224,9 @@ def _resolve_range( return None path = _REPO_PATHS.get(project, project) - pushlog_url = f"{_HG_BASE}/{path}/json-pushes" - if last_green_rev: - pushes = _pushlog( - pushlog_url, f"fromchange={last_green_rev}&tochange={head_rev}" - ) - else: - pushes = _fallback_pushes(pushlog_url, head_rev) - + pushes = _range_pushes(f"{_HG_BASE}/{path}/json-pushes", head_rev) span = max(_count_commits(pushes), 1) - if not last_green_rev or not pushes: - return CommitRange(head_git, None, min(span, max_commits), False) - - if span > max_commits: - logger.warning( - "Range %s..%s has %d commits; capping the clone to the newest %d", - last_green_rev, - head_rev, - span, - max_commits, - ) - return CommitRange(head_git, None, max_commits, False) - - base_git = _hg_to_git(last_green_rev) - if not base_git: - logger.warning("Could not resolve a git hash for last-green %s", last_green_rev) - return CommitRange(head_git, None, span, False) - return CommitRange(head_git, base_git, span, True) + return CommitRange(head_git, min(span, max_commits)) def resolve_investigation( @@ -389,13 +254,6 @@ def resolve_investigation( ) platform = tags.get("test-platform") or "" - if groups: - last_green = _last_green(project, hg_revision, groups[0], platform) - elif not group_based and label: - last_green = _last_green_label(project, hg_revision, label) - else: - last_green = None - logger.info("Last-green revision: %s", last_green or "not found") intermittent_bugs = _known_intermittent_bugs(project, task_id) logger.info( @@ -403,15 +261,13 @@ def resolve_investigation( ", ".join(str(bug) for bug in intermittent_bugs) or "none matched", ) - commit_range = _resolve_range(project, hg_revision, last_green, max_commits) + commit_range = _resolve_range(project, hg_revision, max_commits) if commit_range is None: raise ValueError(f"could not resolve a git commit for task {task_id}") logger.info( - "Range %s..%s spans %d commit(s), complete: %s", - commit_range.base or "(unknown)", - commit_range.head, + "Searching the %d commit(s) before %s", commit_range.span, - commit_range.complete, + commit_range.head, ) return Investigation( @@ -420,7 +276,6 @@ def resolve_investigation( harness=_harness(tags), platform=platform, failing_groups=groups, - last_green_revision=last_green, commit_range=commit_range, label=label, group_based=group_based, diff --git a/agents/test-repair/tests/test_agent_loop.py b/agents/test-repair/tests/test_agent_loop.py index 5c0c259db5..677a39398d 100644 --- a/agents/test-repair/tests/test_agent_loop.py +++ b/agents/test-repair/tests/test_agent_loop.py @@ -3,6 +3,7 @@ import subprocess from types import SimpleNamespace +import pytest from hackbot_agents.test_repair import agent from hackbot_agents.test_repair.config import BUILD_TOOL, SKIP_FIREFOX_BUILD from hackbot_agents.test_repair.prompts import MAX_TESTS_PER_GROUP @@ -11,6 +12,7 @@ FailingGroup, Investigation, ) +from hackbot_runtime import AgentError def _result_msg(is_error=False): @@ -56,8 +58,6 @@ def git(*args): def _investigation( head, - base, - complete=True, platform="linux1804-64/opt", groups=None, group_based=True, @@ -72,8 +72,7 @@ def _investigation( failing_groups=groups if groups is not None else [FailingGroup("dom/base/test/mochitest.ini", ["dom/base/test/a.js"])], - last_green_revision="greenhg", - commit_range=CommitRange(head=head, base=base, span=2, complete=complete), + commit_range=CommitRange(head=head, span=2), label=label, group_based=group_based, known_intermittent_bugs=known_intermittent_bugs or [], @@ -92,7 +91,6 @@ def _run( tmp_path, verdicts, monkeypatch, - complete=True, results=None, platform="linux1804-64/opt", groups=None, @@ -110,6 +108,11 @@ def _run( async def fake_session(reporter, options, prompt): calls.append(prompt) + # A compliant run fetches the log into scratch_out/logs; the agent loop + # fails the run when nothing is there (bug 6665). + job = scratch_out / "logs" / "job_1" + job.mkdir(parents=True, exist_ok=True) + (job / "live_backing_log.log").write_text("TEST-UNEXPECTED-FAIL | a.js") if options_out is not None: options_out.append(options) verdict = verdicts.pop(0) @@ -146,14 +149,11 @@ async def fake_bootstrap(firefox_dir): fx_ctx=_fx_ctx(tmp_path), investigation=_investigation( head, - base if complete else None, - complete, platform, groups, group_based, known_intermittent_bugs=known_intermittent_bugs, ), - task_logs={}, scratch_out=scratch_out, skip_firefox_build=skip_firefox_build, verbose=False, @@ -183,7 +183,6 @@ def test_unsure_culprit_runs_fix_stage(tmp_path, monkeypatch): # A patch is developer advice; the sheriff still backs the regression out. assert result.recommendation == "backout" assert result.proposed_patch is True - assert result.last_green_revision == "greenhg" assert result.num_turns == 6 @@ -340,21 +339,12 @@ def test_failed_fix_stage_still_publishes_analysis(tmp_path, monkeypatch): assert result.analysis == "the reasoning" -def test_complete_range_prompt_gives_a_base_anchored_range(tmp_path, monkeypatch): - _result, calls, head = _run(tmp_path, [{"culprit_commit": None}], monkeypatch) - assert "culprit is one of them" in calls[0] - # The agent enumerates the range itself rather than being handed every sha. - assert "git log --oneline" in calls[0] - assert f"..{head}" in calls[0] - - -def test_incomplete_range_prompt_does_not_assert_the_culprit(tmp_path, monkeypatch): - _result, calls, _head = _run( - tmp_path, [{"culprit_commit": None}], monkeypatch, complete=False - ) +def test_range_prompt_does_not_assert_the_culprit(tmp_path, monkeypatch): + # The range is always depth-bounded rather than anchored on a green run, so the + # prompt must not claim the culprit is inside it. + _result, calls, _head = _run(tmp_path, [{"culprit_commit": None}], monkeypatch) assert "may predate" in calls[0] - assert "culprit is one of them" not in calls[0] - # Without a last-green base the range falls back to the clone depth. + assert "git log --oneline" in calls[0] assert "HEAD~2..HEAD" in calls[0] @@ -365,7 +355,6 @@ def test_assemble_defaults_on_empty_verdict(tmp_path): out, verdict={}, source_repo=tmp_path, - last_green_revision=None, total_turns=1, total_cost=0.0, publish_file=None, @@ -388,7 +377,6 @@ def test_assemble_tolerates_malformed_verdict_fields(tmp_path): "culprit_bug": "n/a", }, source_repo=tmp_path, - last_green_revision=None, total_turns=1, total_cost=0.0, publish_file=None, @@ -407,7 +395,6 @@ def test_assemble_reports_intermittent_classification(tmp_path): out, verdict={"classification": "intermittent", "intermittent_bug": 42}, source_repo=tmp_path, - last_green_revision=None, total_turns=1, total_cost=0.0, publish_file=None, @@ -475,10 +462,8 @@ def test_resolve_culprit_normalizes_against_the_checkout(tmp_path): assert agent._resolve_culprit(repo, " ") is None -def test_range_expr_prefers_the_last_green_base(): - anchored = CommitRange(head="h" * 40, base="b" * 40, span=5, complete=True) - assert agent._range_expr(anchored) == f"{'b' * 40}..{'h' * 40}" - assert agent._range_expr(CommitRange("h" * 40, None, 5, False)) == "HEAD~5..HEAD" +def test_range_expr_is_relative_to_the_checkout(): + assert agent._range_expr(CommitRange("h" * 40, 5)) == "HEAD~5..HEAD" def test_both_stages_name_the_checkout_path(tmp_path, monkeypatch): @@ -667,7 +652,7 @@ def test_non_linux_failure_still_runs_the_test_but_discounts_a_pass( def _mozconfig_for(tmp_path, platform): tmp_path.mkdir(parents=True, exist_ok=True) fx = _fx_ctx(tmp_path) - agent._write_mozconfig(fx, _investigation("h", None, platform=platform)) + agent._write_mozconfig(fx, _investigation("h", platform=platform)) return fx.mozconfig.read_text() @@ -702,12 +687,12 @@ def test_verify_step_states_the_container_limits(tmp_path, monkeypatch): assert "could not verify" in calls[1] -def test_last_green_is_labelled_as_an_hg_revision(tmp_path, monkeypatch): +def test_the_hg_revision_is_labelled_as_one(tmp_path, monkeypatch): # Every other revision in the prompt is a git hash; an unlabelled hg node # makes the agent run git commands against it and get exit 128. _result, calls, _head = _run(tmp_path, [{"culprit_commit": None}], monkeypatch) - assert "hg revision greenhg" in calls[0] - assert "not a git object" in calls[0] + assert "hg revision" in _flat(calls[0]) + assert "never to git" in _flat(calls[0]) def test_mozconfig_overwrites_a_foreign_one(tmp_path): @@ -719,8 +704,113 @@ def test_mozconfig_overwrites_a_foreign_one(tmp_path): "ac_add_options --enable-release\n" "mk_add_options MOZ_OBJDIR=/workspace/firefox/objdir-build-repair\n" ) - agent._write_mozconfig(fx, _investigation("h", None, platform="linux1804-64/opt")) + agent._write_mozconfig(fx, _investigation("h", platform="linux1804-64/opt")) written = fx.mozconfig.read_text() assert "objdir-build-repair" not in written assert "--enable-release" not in written assert str(fx.objdir) in written + + +def test_analysis_prompt_offers_treeherder_cli(tmp_path, monkeypatch): + _result, calls, _head = _run( + tmp_path, + [ + {"recommendation": "backout", "culprit_commit": "HEAD", "confidence": 0.5}, + {"recommendation": "backout", "culprit_commit": "HEAD", "confidence": 0.5}, + ], + monkeypatch, + ) + prompt = _flat(calls[0]) + # The command has to be runnable as printed: repo and revision, not placeholders. + assert "treeherder-cli hgrev --repo autoland" in prompt + assert "--suspects" in prompt + assert "--similar-history" in prompt + # The two ways to waste a run: unbounded output and a blocking flag. + assert "Always pass `--filter`" in prompt + assert "Never pass `--watch`" in prompt + + +def test_a_reported_blocker_fails_the_run(tmp_path, monkeypatch): + # The agent could not get the log, so there is nothing to analyse. Failing the + # run is what reaches the API as an error and skips the Slack notification; + # a verdict here would be a fix invented from the diff alone (bug 6665). + repo = tmp_path / "src" + repo.mkdir() + _git_repo(repo) + scratch_out = tmp_path / "out" + scratch_out.mkdir() + + async def fake_session(reporter, options, prompt): + (scratch_out / "error.txt").write_text( + "treeherder-cli --fetch-logs returned no jobs for " + "test-linux1804-64/opt-mochitest-1" + ) + return _result_msg() + + monkeypatch.setattr(agent, "_run_session", fake_session) + monkeypatch.setattr(agent, "build_sdk_server", lambda *a, **k: {"type": "sdk"}) + + with pytest.raises(AgentError) as excinfo: + asyncio.run( + agent.run_test_repair( + bugzilla_mcp_server=None, + source_repo=repo, + fx_ctx=_fx_ctx(tmp_path), + investigation=_investigation("headsha"), + scratch_out=scratch_out, + verbose=False, + log=None, + ) + ) + assert "returned no jobs" in str(excinfo.value) + + +def test_no_blocker_file_means_the_run_continues(tmp_path, monkeypatch): + result, calls, _head = _run( + tmp_path, + [ + {"recommendation": "backout", "culprit_commit": "HEAD", "confidence": 0.5}, + {"recommendation": "backout", "culprit_commit": "HEAD", "confidence": 0.5}, + ], + monkeypatch, + ) + assert len(calls) == 2 + assert result.recommendation == "backout" + + +def test_a_verdict_without_a_retrieved_log_fails_the_run(tmp_path, monkeypatch): + # The prompt asks the agent to report a blocker itself, but a run against a + # push whose job treeherder-cli could not reach produced a full verdict + # instead. The log check is what makes it a guarantee: no log, no verdict, + # and the error reaches the API and the UI (bug 6665). + repo = tmp_path / "src" + repo.mkdir() + _git_repo(repo) + scratch_out = tmp_path / "out" + scratch_out.mkdir() + + async def fake_session(reporter, options, prompt): + # a confident verdict, but nothing was ever fetched into scratch_out/logs + (scratch_out / "verdict.json").write_text( + json.dumps({"recommendation": "backout", "culprit_commit": "HEAD"}) + ) + (scratch_out / "summary.md").write_text("the verdict") + (scratch_out / "analysis.md").write_text("the reasoning") + return _result_msg() + + monkeypatch.setattr(agent, "_run_session", fake_session) + monkeypatch.setattr(agent, "build_sdk_server", lambda *a, **k: {"type": "sdk"}) + + with pytest.raises(AgentError) as excinfo: + asyncio.run( + agent.run_test_repair( + bugzilla_mcp_server=None, + source_repo=repo, + fx_ctx=_fx_ctx(tmp_path), + investigation=_investigation("headsha"), + scratch_out=scratch_out, + verbose=False, + log=None, + ) + ) + assert "no failure log was retrieved" in str(excinfo.value) diff --git a/agents/test-repair/tests/test_notify.py b/agents/test-repair/tests/test_notify.py index fbf9257cb1..cdae9a5c21 100644 --- a/agents/test-repair/tests/test_notify.py +++ b/agents/test-repair/tests/test_notify.py @@ -8,16 +8,13 @@ HG_REVISION = "341517e50536aabbccddeeff00112233445566" GIT_REVISION = "7b15e34863cf6b30b613ffadf9d6431fe5a55585" -LAST_GREEN = "c338a2c1c8d3695b7dec835125af624282555b7e" TASK_ID = "JfAGrrtoQPS3fXrwZmq1Pg" GIT_URL = f"https://github.com/mozilla-firefox/firefox/commit/{GIT_REVISION}" HG_URL = f"https://hg.mozilla.org/mozilla-unified/rev/{HG_REVISION}" -def _investigation( - groups=None, last_green=LAST_GREEN, label="test-linux1804-64/opt-xpcshell-1" -): +def _investigation(groups=None, label="test-linux1804-64/opt-xpcshell-1"): return Investigation( project="autoland", hg_revision=HG_REVISION, @@ -26,8 +23,7 @@ def _investigation( failing_groups=groups if groups is not None else [FailingGroup("toolkit/modules/tests/xpcshell/xpcshell.toml", ["a.js"])], - last_green_revision=last_green, - commit_range=CommitRange(head=GIT_REVISION, base="base", span=4, complete=True), + commit_range=CommitRange(head=GIT_REVISION, span=4), label=label, ) @@ -94,9 +90,7 @@ def test_reports_the_verdict_and_its_context_in_five_lines(): f"?repo=autoland&revision={HG_REVISION}&selectedTaskRun={TASK_ID}|Treeherder>, " f"", - f"Push: autoland <{HG_URL}|hg 341517e50536> / <{GIT_URL}|github 7b15e34863cf>, " - f"last green ", + f"Push: autoland <{HG_URL}|hg 341517e50536> / <{GIT_URL}|github 7b15e34863cf>", f"Culprit: <{GIT_URL}|github 7b15e34863cf> by standard8@mozilla.com " "()", "", @@ -180,9 +174,3 @@ def test_lists_every_failing_group(): def test_falls_back_when_groups_and_label_are_unknown(): message = _message(investigation=_investigation(groups=[], label="")) assert "Failing: tests not resolved in `xpcshell on linux1804-64/opt`" in message - - -def test_omits_the_last_green_revision_when_unknown(): - message = _message(investigation=_investigation(last_green=None)) - assert "last green" not in message - assert message.splitlines()[3].endswith("|github 7b15e34863cf>") diff --git a/agents/test-repair/tests/test_resolve.py b/agents/test-repair/tests/test_resolve.py index bbb1aa0610..204a2b3cc5 100644 --- a/agents/test-repair/tests/test_resolve.py +++ b/agents/test-repair/tests/test_resolve.py @@ -1,8 +1,6 @@ import pytest from hackbot_agents.test_repair import resolve from hackbot_agents.test_repair.resolve import FailingGroup -from mozci.errors import ParentPushNotFound -from mozci.task import Status GROUP = "dom/base/test/mochitest.ini" TESTS = ["dom/base/test/test_a.js"] @@ -16,184 +14,16 @@ def _investigation(**kwargs): harness="mochitest", platform=PLATFORM, failing_groups=[], - last_green_revision=None, - commit_range=resolve.CommitRange("head", None, 1, False), + commit_range=resolve.CommitRange("head", 1), ) return resolve.Investigation(**{**defaults, **kwargs}) -class FakeResult: - def __init__(self, group, ok): - self.group = group - self.ok = ok - - -class FakeTask: - def __init__(self, platform, ok, failed_tests=()): - self.platform = platform - self.label = f"test-{platform}-mochitest-1" - self.results = [FakeResult(GROUP, ok)] - self.failure_types = {GROUP: [(t, "timeout") for t in failed_tests]} - - -class FakeSummary: - def __init__(self, *tasks): - self.tasks = list(tasks) - - -class FakeLabelSummary: - def __init__(self, status): - self.status = status - - -class FakePush: - def __init__(self, rev, summaries=None, parent=None, labels=None): - self.rev = rev - self.group_summaries = summaries or {} - self.label_summaries = labels or {} - self._parent = parent - - @property - def parent(self): - if self._parent is None: - raise ParentPushNotFound(f"no parent for {self.rev}") - return self._parent - - -def _failing(): - return FailingGroup(GROUP, TESTS) - - -def test_harness_detection(): - assert resolve._harness({"test-suite": "xpcshell"}) == "xpcshell" - assert resolve._harness({"label": "test-linux/opt-xpcshell-4"}) == "xpcshell" - assert resolve._harness({"test-suite": "mochitest-browser-chrome"}) == "mochitest" - # Every Firefox test task has kind=="test", so the suite must win over it. - assert ( - resolve._harness({"kind": "test", "test-suite": "web-platform-tests"}) - == "web-platform-tests" - ) - assert resolve._harness({}) == "unknown" - - -def test_platform_derived_flags(): - inv = _investigation(platform="linux1804-64-qr/debug") - assert inv.debug_build is True - assert inv.is_linux is True - win = _investigation(platform="windows11-64-24h2/opt") - assert win.debug_build is False - assert win.is_linux is False - - -def test_last_green_returns_first_passing_ancestor(monkeypatch): - green = FakePush("greenrev", {GROUP: FakeSummary(FakeTask(PLATFORM, ok=True))}) - head = FakePush("headrev", {}, parent=green) - monkeypatch.setattr(resolve, "Push", lambda rev, branch=None: head) - assert ( - resolve._last_green("autoland", "headrev", _failing(), PLATFORM) == "greenrev" - ) - - -def test_last_green_none_when_already_failing_upstream(monkeypatch): - parent = FakePush( - "parentrev", - {GROUP: FakeSummary(FakeTask(PLATFORM, ok=False, failed_tests=TESTS))}, - ) - head = FakePush("headrev", {}, parent=parent) - monkeypatch.setattr(resolve, "Push", lambda rev, branch=None: head) - assert resolve._last_green("autoland", "headrev", _failing(), PLATFORM) is None - - -def test_last_green_skips_pushes_that_only_ran_elsewhere(monkeypatch): - # The group passed on Windows but never ran on the failing Linux platform, so - # it cannot anchor a last-green -- the walk must continue past it. - green = FakePush("greenrev", {GROUP: FakeSummary(FakeTask(PLATFORM, ok=True))}) - other = FakePush( - "otherrev", - {GROUP: FakeSummary(FakeTask("windows11-64-24h2/opt", ok=True))}, - parent=green, - ) - head = FakePush("headrev", {}, parent=other) - monkeypatch.setattr(resolve, "Push", lambda rev, branch=None: head) - assert ( - resolve._last_green("autoland", "headrev", _failing(), PLATFORM) == "greenrev" - ) - - -def test_test_status_ignores_failures_of_other_tests(): - # The manifest failed, but not for the test we are investigating. - summary = FakeSummary( - FakeTask(PLATFORM, ok=False, failed_tests=["dom/base/test/test_other.js"]) - ) - push = FakePush("rev", {GROUP: summary}) - assert resolve._test_status(push, GROUP, TESTS, PLATFORM) == "passed" - assert resolve._test_status(push, GROUP, [], PLATFORM) == "failed" - - -def test_test_status_none_when_group_absent(): - assert resolve._test_status(FakePush("rev"), GROUP, TESTS, PLATFORM) is None - - -def test_label_status_maps_the_summary_status(): - label = "test-macosx1500-aarch64/opt-gtest-1proc" - passed = FakePush("rev", labels={label: FakeLabelSummary(Status.PASS)}) - failed = FakePush("rev", labels={label: FakeLabelSummary(Status.FAIL)}) - flaky = FakePush("rev", labels={label: FakeLabelSummary(Status.INTERMITTENT)}) - assert resolve._label_status(passed, label) == "passed" - assert resolve._label_status(failed, label) == "failed" - # Both passed and failed here, so it cannot anchor a green. - assert resolve._label_status(flaky, label) is None - assert resolve._label_status(FakePush("rev"), label) is None - - -def test_last_green_label_walks_to_a_green_task(monkeypatch): - # gtest reports no manifests, so the whole task is the finest granularity. - label = "test-macosx1500-aarch64/opt-gtest-1proc" - green = FakePush("greenrev", labels={label: FakeLabelSummary(Status.PASS)}) - # Did not run here at all: non-decisive, so the walk must continue. - absent = FakePush("absentrev", labels={}, parent=green) - head = FakePush("headrev", labels={}, parent=absent) - monkeypatch.setattr(resolve, "Push", lambda rev, branch=None: head) - assert resolve._last_green_label("autoland", "headrev", label) == "greenrev" - - -def test_last_green_label_none_when_already_failing_upstream(monkeypatch): - label = "test-macosx1500-aarch64/opt-gtest-1proc" - parent = FakePush("parentrev", labels={label: FakeLabelSummary(Status.FAIL)}) - head = FakePush("headrev", labels={}, parent=parent) - monkeypatch.setattr(resolve, "Push", lambda rev, branch=None: head) - assert resolve._last_green_label("autoland", "headrev", label) is None - - -def test_last_green_fails_soft_on_error(monkeypatch): - def boom(rev, branch=None): - raise RuntimeError("mozci exploded") - - monkeypatch.setattr(resolve, "Push", boom) - assert resolve._last_green("autoland", "headrev", _failing(), PLATFORM) is None - - def _hg2git(rev): return {"hgA": "gitA", "hgB": "gitB", "hgC": "gitC"}.get(rev) -def test_resolve_range_maps_only_the_endpoints(monkeypatch): - pushes = {"2": {"changesets": [{"node": "hgB"}, {"node": "hgC"}]}} - looked_up = [] - - def fake_hg_to_git(rev): - looked_up.append(rev) - return _hg2git(rev) - - monkeypatch.setattr(resolve, "_get_json", lambda url: {"pushes": pushes}) - monkeypatch.setattr(resolve, "_hg_to_git", fake_hg_to_git) - rng = resolve._resolve_range("autoland", "hgC", "hgA", 100) - assert (rng.head, rng.base, rng.span, rng.complete) == ("gitC", "gitA", 2, True) - # Two lando lookups regardless of range width; no per-commit mapping to fail. - assert looked_up == ["hgC", "hgA"] - - -def test_resolve_range_without_last_green_widens_to_ancestor_pushes(monkeypatch): +def test_resolve_range_widens_to_ancestor_pushes(monkeypatch): # The head push alone was one l10n commit, which both hid the real culprit and # left the shallow clone with nothing to enumerate. urls = [] @@ -211,26 +41,22 @@ def fake_get_json(url): monkeypatch.setattr(resolve, "_get_json", fake_get_json) monkeypatch.setattr(resolve, "_hg_to_git", _hg2git) - rng = resolve._resolve_range("autoland", "hgA", None, 100) - assert f"startID={500 - resolve.FALLBACK_RANGE_PUSHES}&endID=500" in urls[1] + rng = resolve._resolve_range("autoland", "hgA", 100) + assert f"startID={500 - resolve.RANGE_PUSHES}&endID=500" in urls[1] assert rng.head == "gitA" - assert rng.base is None assert rng.span == 40 - # Wider, but the culprit is still not provably inside it. - assert rng.complete is False -def test_resolve_range_fallback_window_is_capped(monkeypatch): +def test_resolve_range_window_is_capped(monkeypatch): pushes = {str(i): {"changesets": [{"node": f"hg{i}"}]} for i in range(1, 40)} monkeypatch.setattr(resolve, "_get_json", lambda url: {"pushes": pushes}) monkeypatch.setattr(resolve, "_hg_to_git", _hg2git) - rng = resolve._resolve_range("autoland", "hgA", None, 10) + rng = resolve._resolve_range("autoland", "hgA", 10) # The span drives the clone depth, so the cap has to hold here too. assert rng.span == 10 - assert rng.complete is False -def test_resolve_range_fallback_keeps_the_head_push_when_widening_fails(monkeypatch): +def test_resolve_range_keeps_the_head_push_when_widening_fails(monkeypatch): def fake_get_json(url): if "changeset=hgA" in url: return {"pushes": {"500": {"changesets": [{"node": "hgA"}]}}} @@ -238,38 +64,14 @@ def fake_get_json(url): monkeypatch.setattr(resolve, "_get_json", fake_get_json) monkeypatch.setattr(resolve, "_hg_to_git", _hg2git) - rng = resolve._resolve_range("autoland", "hgA", None, 100) + rng = resolve._resolve_range("autoland", "hgA", 100) assert rng.head == "gitA" assert rng.span == 1 - assert rng.complete is False - - -def test_resolve_range_capped_drops_the_base(monkeypatch): - pushes = {str(i): {"changesets": [{"node": f"hg{i}"}]} for i in range(1, 6)} - monkeypatch.setattr(resolve, "_get_json", lambda url: {"pushes": pushes}) - monkeypatch.setattr(resolve, "_hg_to_git", lambda rev: "gitC") - rng = resolve._resolve_range("autoland", "hgC", "hgA", 2) - # The base falls outside the capped clone, so it can no longer anchor it. - assert rng.base is None - assert rng.span == 2 - assert rng.complete is False def test_resolve_range_none_when_head_unresolvable(monkeypatch): monkeypatch.setattr(resolve, "_hg_to_git", lambda rev: None) - assert resolve._resolve_range("autoland", "hgHEAD", "hgOLD", 100) is None - - -def test_resolve_range_incomplete_when_base_unresolvable(monkeypatch): - pushes = {"1": {"changesets": [{"node": "hgB"}]}} - monkeypatch.setattr(resolve, "_get_json", lambda url: {"pushes": pushes}) - monkeypatch.setattr( - resolve, "_hg_to_git", lambda rev: "gitB" if rev == "hgB" else None - ) - rng = resolve._resolve_range("autoland", "hgB", "hgA", 100) - assert rng.head == "gitB" - assert rng.base is None - assert rng.complete is False + assert resolve._resolve_range("autoland", "hgHEAD", 100) is None def test_resolve_range_survives_pushlog_failure(monkeypatch): @@ -278,11 +80,9 @@ def boom(url): monkeypatch.setattr(resolve, "_get_json", boom) monkeypatch.setattr(resolve, "_hg_to_git", lambda rev: "gitHEAD") - rng = resolve._resolve_range("autoland", "hgHEAD", "hgOLD", 100) + rng = resolve._resolve_range("autoland", "hgHEAD", 100) assert rng.head == "gitHEAD" - assert rng.base is None assert rng.span == 1 - assert rng.complete is False def test_resolve_investigation_assembles_context(monkeypatch): @@ -298,11 +98,8 @@ def test_resolve_investigation_assembles_context(monkeypatch): monkeypatch.setattr( resolve, "_failing_groups", lambda tid: [FailingGroup(GROUP, TESTS)] ) - monkeypatch.setattr(resolve, "_last_green", lambda *a: "greenrev") monkeypatch.setattr( - resolve, - "_resolve_range", - lambda *a: resolve.CommitRange("gitHead", "gitBase", 2, True), + resolve, "_resolve_range", lambda *a: resolve.CommitRange("gitHead", 2) ) inv = resolve.resolve_investigation("TASK") @@ -311,10 +108,7 @@ def test_resolve_investigation_assembles_context(monkeypatch): assert inv.harness == "mochitest" assert inv.platform == "linux1804-64-qr/debug" assert inv.debug_build is True - assert inv.last_green_revision == "greenrev" assert inv.failure_commit == "gitHead" - assert inv.commit_range.base == "gitBase" - assert inv.commit_range.complete is True assert inv.commit_range.span == 2 @@ -336,52 +130,13 @@ def no_group_lookup(task_id): monkeypatch.setattr(resolve, "_failing_groups", no_group_lookup) monkeypatch.setattr( - resolve, "_last_green_label", lambda branch, rev, lbl: f"green-{lbl}" - ) - monkeypatch.setattr( - resolve, - "_resolve_range", - lambda *a: resolve.CommitRange("gitHead", "gitBase", 2, True), + resolve, "_resolve_range", lambda *a: resolve.CommitRange("gitHead", 2) ) inv = resolve.resolve_investigation("TASK") assert inv.group_based is False assert inv.failing_groups == [] assert inv.label == label - assert inv.last_green_revision == f"green-{label}" - - -def test_resolve_investigation_keeps_group_level_last_green_for_grouped_suites( - monkeypatch, -): - task = { - "tags": { - "project": "autoland", - "test-suite": "mochitest-browser-chrome", - "test-platform": PLATFORM, - "label": f"test-{PLATFORM}-mochitest-browser-chrome-1", - }, - "payload": {"env": {"GECKO_HEAD_REV": "hghead"}}, - } - monkeypatch.setattr(resolve, "_get_json", lambda url: task) - monkeypatch.setattr( - resolve, "_failing_groups", lambda tid: [FailingGroup(GROUP, TESTS)] - ) - monkeypatch.setattr(resolve, "_last_green", lambda *a: "greenrev") - - def no_label_lookup(*a): - raise AssertionError("label-level last-green is only for group-less suites") - - monkeypatch.setattr(resolve, "_last_green_label", no_label_lookup) - monkeypatch.setattr( - resolve, - "_resolve_range", - lambda *a: resolve.CommitRange("gitHead", "gitBase", 2, True), - ) - - inv = resolve.resolve_investigation("TASK") - assert inv.group_based is True - assert inv.last_green_revision == "greenrev" def test_resolve_investigation_requires_a_git_commit(monkeypatch): diff --git a/agents/test-repair/tests/test_scaffold.py b/agents/test-repair/tests/test_scaffold.py index 32b31e6526..c70445bbda 100644 --- a/agents/test-repair/tests/test_scaffold.py +++ b/agents/test-repair/tests/test_scaffold.py @@ -1,34 +1,8 @@ -from hackbot_agents.test_repair import logs from hackbot_agents.test_repair.__main__ import _checkout_pin from hackbot_agents.test_repair.agent import TestRepairResult from hackbot_agents.test_repair.resolve import CommitRange, Investigation -def test_sanitize_log_keeps_failure_and_error_lines(): - raw = "\n".join( - [ - "INFO - starting test", - "TEST-UNEXPECTED-FAIL | dom/test_a.js | assertion failed", - "some noise", - "12:00 ERROR - linker error", - "TEST-PASS | dom/test_b.js", - "FATAL - crash", - ] - ) - out = logs.sanitize_log(raw).splitlines() - assert any("TEST-UNEXPECTED-FAIL" in line for line in out) - assert any("ERROR - linker error" in line for line in out) - assert any("FATAL - crash" in line for line in out) - # Passing/info lines are dropped. - assert all("TEST-PASS" not in line for line in out) - assert all("starting test" not in line for line in out) - - -def test_sanitize_log_dedupes_consecutive_repeats(): - raw = "\n".join(["TEST-UNEXPECTED-FAIL | x | boom"] * 3) - assert len(logs.sanitize_log(raw).splitlines()) == 1 - - def _investigation(**kwargs): defaults = dict( project="autoland", @@ -36,8 +10,7 @@ def _investigation(**kwargs): harness="mochitest", platform="linux1804-64-qr/opt", failing_groups=[], - last_green_revision="greenhg", - commit_range=CommitRange(head="headsha", base="basesha", span=3, complete=True), + commit_range=CommitRange(head="headsha", span=3), ) return Investigation(**{**defaults, **kwargs})