diff --git a/config/repo_config.py b/config/repo_config.py index c68b4dff96..5343a1a704 100644 --- a/config/repo_config.py +++ b/config/repo_config.py @@ -364,21 +364,27 @@ def validate_checks(checks: list[Any]) -> list[dict[str, str]]: """Validate and normalize a list of check command entries. Filters out malformed entries and coerces values to strings. + Mirrors ``egg_config.validators.validate_checks``, including + the optional ``fix`` auto-remediation command (#3409). Args: checks: Raw list of check entries (e.g. from YAML or JSON). Returns: - List of {"name": "...", "command": "..."} dicts with only - valid entries retained. + List of {"name": "...", "command": "..."} dicts (plus + "fix" when configured) with only valid entries retained. """ if not isinstance(checks, list): return [] - return [ - {"name": str(c["name"]), "command": str(c["command"])} - for c in checks - if isinstance(c, dict) and "name" in c and "command" in c - ] + result = [] + for c in checks: + if not (isinstance(c, dict) and "name" in c and "command" in c): + continue + entry = {"name": str(c["name"]), "command": str(c["command"])} + if c.get("fix"): + entry["fix"] = str(c["fix"]) + result.append(entry) + return result def reload_config() -> None: @@ -572,14 +578,16 @@ def get_repo_checks(repo: str) -> list[dict[str, str]]: These are the commands to run during the SDLC pipeline implement phase checker step. Each check has a "name" (display label) and "command" - (shell command to execute). They run sequentially. + (shell command to execute). They run sequentially. A check may also + carry an optional "fix" command — an auto-remediation the per-slice + green gate runs when the check is red at the slice tip (#3409). Args: repo: Repository in "owner/repo" format Returns: - List of {"name": "...", "command": "..."} dicts, - or empty list if no checks configured. + List of {"name": "...", "command": "..."} dicts (plus "fix" + when configured), or empty list if no checks configured. """ checks = get_repo_setting(repo, "checks", []) result: list[dict[str, str]] = validate_checks(checks) diff --git a/config/repositories.yaml.example b/config/repositories.yaml.example index 84b42c78d1..d1d6063ece 100644 --- a/config/repositories.yaml.example +++ b/config/repositories.yaml.example @@ -77,6 +77,13 @@ readable_repos: # - checks: List of check commands for the SDLC pipeline implement phase # Each entry has "name" (display label) and "command" (shell command) # These run sequentially during the checker step +# An entry may also set "fix" (shell command): when the per-slice green +# gate finds that check red at the slice tip, it runs the fix command, +# re-runs the check, and — if the fix turned it green — commits and +# pushes the result to the slice integration branch as the orchestrator +# (#3409). Only configure deterministic auto-remediations here, e.g. +# "make lint-fix" for a format/lint check. Checks without "fix" route +# red verdicts back to the slice team unchanged. # - build_commands: Commands to run during Docker image build to install # project-specific dependencies. Results are baked into the image so # containers start with dependencies pre-installed (critical for private @@ -138,6 +145,7 @@ repo_settings: # checks: # - name: lint # command: make lint + # fix: make lint-fix # optional green-gate auto-remediation (#3409) # - name: test # command: make test # some-org/external-repo: diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index 7a0e26e8b1..9be75cd4a6 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -555,7 +555,20 @@ shape: rollout via `EGG_SLICE_GREEN_GATE` — default `on` (a red verdict withholds the slice PR), `log` runs the checks and logs the verdict without blocking; fail-open on infra errors, including - infra-signature-tagged reds inside check execution, #3417) — calls + infra-signature-tagged reds inside check execution, #3417. + **The gate can also write to the integration branch**: when every + genuine red carries an optional `fix:` command in + `repositories.yaml` (e.g. `lint: {fix: make lint-fix}`), the + runner applies the fixes in its worktree and the orchestrator + commits them as `egg-green-gate` and pushes to the integration + branch via the launcher-authed gateway push route, #3409. This is + the one place the orchestrator authors commits on a slice branch; + it fires only in `on` mode, and only when the runner proves the + exact tree `git add -u` will stage is green — one full re-run of + *every* configured check against the all-fixes-applied tree + (`final_verification.all_ok`) plus a no-new-untracked-files + check. Any failure to commit or push blocks the slice exactly + like an unfixed red) — calls `GatewayClient.create_slice_pr` with `base` resolved from the slice's DAG parent (root → latest completed chain tip, else the pipeline branch (#3541); child → parent's diff --git a/orchestrator/routes/pipelines/__init__.py b/orchestrator/routes/pipelines/__init__.py index e510bd937c..829ccd96c2 100644 --- a/orchestrator/routes/pipelines/__init__.py +++ b/orchestrator/routes/pipelines/__init__.py @@ -467,11 +467,15 @@ def get_repo_checks(repo: str) -> list[dict[str, str]]: # type: ignore[misc] def validate_checks(checks: list) -> list[dict[str, str]]: # type: ignore[misc] if not isinstance(checks, list): return [] - return [ - {"name": str(c["name"]), "command": str(c["command"])} - for c in checks - if isinstance(c, dict) and "name" in c and "command" in c - ] + result = [] + for c in checks: + if not (isinstance(c, dict) and "name" in c and "command" in c): + continue + entry = {"name": str(c["name"]), "command": str(c["command"])} + if c.get("fix"): + entry["fix"] = str(c["fix"]) + result.append(entry) + return result pipelines_bp = Blueprint("pipelines", __name__, url_prefix="/api/v1/pipelines") diff --git a/orchestrator/slice_green_gate.py b/orchestrator/slice_green_gate.py index 420ee72647..a313935903 100644 --- a/orchestrator/slice_green_gate.py +++ b/orchestrator/slice_green_gate.py @@ -146,6 +146,21 @@ Someone should watch the first wave directly rather than discovering the cost from a slice-throughput drop later; ``off`` is the escape hatch. +Stage A (#3409) adds config-driven auto-remediation on top of the Stage +B block: a check in ``repositories.yaml`` may carry an optional ``fix`` +command (e.g. egg's ``lint`` check gets ``fix: make lint-fix``). When +the gate finds such a check red, the runner executes the fix inside its +worktree and re-runs the check. If every failed check re-ran green, the +orchestrator stages the fix from the shared hostPath worktree, commits +it as ``egg-green-gate``, and pushes it to the slice integration branch +through the launcher-authed gateway push route before any close side +effect, then lets the slice close: the runner already re-validated the +identical tree, so no second runner pass is needed, and dependent +slices fork from the remote tip after the fix commit. In ``log`` mode +the fix still runs in the runner (soak signal) but nothing is committed +or pushed. Checks without a ``fix``, or whose re-run stays red, block +exactly like Stage B. + The check toolchain is the **repo-defined** one, not the sandbox image's: ``repositories.yaml::build_commands`` builds the repo's pinned dev environment at image build (e.g. egg's ``make sandbox-deps`` → @@ -168,6 +183,7 @@ import json import os +import subprocess import time import uuid from typing import TYPE_CHECKING, Any, Literal @@ -320,6 +336,23 @@ # runner pod by this selector (mirrors ``egg.io/probe-id``). _GATE_ID_LABEL = "egg.io/green-gate-id" +# Cap on the informational ``changed_files`` list a fix result carries +# in the verdict; a repo-wide format sweep can touch hundreds of files +# and the verdict must stay a single parseable log line. +_FIX_CHANGED_FILES_CAP = 100 + +# Identity for the orchestrator-authored autofix commit (#3409). +# Precedent: agent_salvage's ``egg-salvage`` system identity and the +# git-route orchestrator attribution from #2919; the commit must read +# as pipeline infrastructure in the history, not as a phantom coder. +_AUTOFIX_COMMIT_NAME = "egg-green-gate" +_AUTOFIX_COMMIT_EMAIL = "egg-green-gate@localhost" + +# Per-git-invocation ceiling for the autofix stage/commit sequence. The +# operations are local (no network); a format sweep staging hundreds of +# files finishes in single-digit seconds. +_AUTOFIX_GIT_TIMEOUT_SECONDS = 120 + # The runner program, executed as ``python3 -c`` in the pod. Restores # the repo's prebuilt build_commands artifacts (its pinned ``.venv``) # into the worktree, then reads the check list (JSON) and repo dir from @@ -332,6 +365,15 @@ # proceeding would red every check with "command not found" and block # the slice for a toolchain-packaging problem that is not its fault. # +# #3409 Stage A: when a check fails and carries a configured ``fix`` +# command, the runner executes the fix in the worktree and re-runs the +# check, reporting a ``fix`` sub-object in that check's verdict entry. +# The check's ``ok`` stays false: the slice tip as pushed is still red; +# only the orchestrator (the sanctioned writer) may turn the fixed tree +# into a commit on the integration branch. The runner itself never +# pushes; the fix mutates the hostPath-mounted worktree, which the +# orchestrator stages and commits after the pod exits. +# # Infra classification (#3417) happens here, runner-side, because only # the runner sees a check's *full* output: the verdict carries a # truncated tail, and an early gateway error (e.g. the test selector's @@ -345,6 +387,7 @@ checks = json.loads(os.environ["EGG_GREEN_GATE_CHECKS"]) repo_dir = os.environ["EGG_GREEN_GATE_REPO_DIR"] tail = int(os.environ.get("EGG_GREEN_GATE_OUTPUT_TAIL", "4000")) +changed_files_cap = int(os.environ.get("EGG_GREEN_GATE_CHANGED_FILES_CAP", "100")) infra_signatures = json.loads(os.environ.get("EGG_GREEN_GATE_INFRA_SIGNATURES", "{}")) infra_line_signatures = infra_signatures.get("line", []) infra_substring_signatures = infra_signatures.get("substring", []) @@ -428,33 +471,148 @@ def copy_if_missing(src, dst, **kwargs): ) sys.exit(1) -results = [] -for check in checks: - started = time.monotonic() +def run_cmd(command): try: proc = subprocess.run( - ["bash", "-c", check["command"]], + ["bash", "-c", command], cwd=repo_dir, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, errors="replace", ) - rc, out = proc.returncode, proc.stdout or "" + return proc.returncode, proc.stdout or "" except Exception as exc: - rc, out = -1, f"runner failed to execute check: {exc}" - results.append( - { - "name": check["name"], - "ok": rc == 0, - "exit_code": rc, - "duration_seconds": round(time.monotonic() - started, 1), - "output_tail": out[-tail:], - "infra": classify_infra(rc, out) if rc != 0 else None, + return -1, f"runner failed to execute command: {exc}" + + +def tracked_changed_files(): + # Informational only (#3409): tracked modifications the fix left in + # the worktree, via the sandbox's gateway-routed git. The + # orchestrator stages from the shared worktree itself, so a failure + # here (git wrapper hiccup) degrades to an unreported file list, + # never a wrong commit. + try: + proc = subprocess.run( + ["git", "diff", "--name-only"], + cwd=repo_dir, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + errors="replace", + ) + except Exception: + return None + if proc.returncode != 0: + return None + return [line for line in (proc.stdout or "").splitlines() if line.strip()] + + +def untracked_files(): + # Non-ignored untracked paths (#3409). ``--exclude-standard`` honours + # .gitignore, so check droppings (caches, selection JSON) that live + # in .gitignore are excluded; a fix that *creates* a new source file + # (codegen, a formatter that splits a module) shows up here. The + # orchestrator stages with ``git add -u``, which never picks up + # untracked files, so a fix that produces one would push a tree the + # final re-run never validated as committed — the caller refuses + # autofix when this set grew. ``None`` on failure degrades to unsafe. + try: + proc = subprocess.run( + ["git", "ls-files", "--others", "--exclude-standard"], + cwd=repo_dir, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + errors="replace", + ) + except Exception: + return None + if proc.returncode != 0: + return None + return sorted(line for line in (proc.stdout or "").splitlines() if line.strip()) + + +untracked_before = untracked_files() +any_fix_applied = False + +results = [] +for check in checks: + started = time.monotonic() + rc, out = run_cmd(check["command"]) + entry = { + "name": check["name"], + "ok": rc == 0, + "exit_code": rc, + "output_tail": out[-tail:], + # #3417: tag the red with an infra signature (if any) from the + # *first* run's full output — the run that produced the verdict + # this check is failing on. + "infra": classify_infra(rc, out) if rc != 0 else None, + } + fix_cmd = check.get("fix") + if rc != 0 and fix_cmd: + # #3409: attempt the configured auto-remediation, then re-run + # the check against the fixed tree. The re-run verdict, not the + # fix command's exit code, decides success: a fixer may exit + # non-zero while still having repaired everything the check + # tests (or exit zero while leaving unfixable findings). + any_fix_applied = True + fix_rc, fix_out = run_cmd(fix_cmd) + rerun_rc, rerun_out = run_cmd(check["command"]) + changed = tracked_changed_files() + entry["fix"] = { + "command": fix_cmd, + "exit_code": fix_rc, + "check_ok_after_fix": rerun_rc == 0, + "changed_files": (changed[:changed_files_cap] if changed is not None else None), + "changed_file_count": (len(changed) if changed is not None else None), + "output_tail": fix_out[-tail:], + "recheck_output_tail": rerun_out[-tail:], } - ) + entry["duration_seconds"] = round(time.monotonic() - started, 1) + results.append(entry) + +# #3409: once any fix has run, the per-check re-runs above each validated +# an *intermediate* tree — check i re-ran before check i+1's fix, and +# originally-green checks were validated pre-fix and never re-run. The +# orchestrator, however, commits one tree with *all* fixes applied. To +# make "the committed tip is validated green" true in general (not just +# for a single-check config), re-run every check once more against the +# final fixed tree and report the aggregate. The orchestrator gates +# autofix on this pass, not on the per-check re-runs. +verdict = {"checks": results} +if any_fix_applied: + final_checks = [] + for check in checks: + rc, out = run_cmd(check["command"]) + final_checks.append( + { + "name": check["name"], + "ok": rc == 0, + "exit_code": rc, + "output_tail": out[-tail:], + } + ) + untracked_after = untracked_files() + if untracked_before is None or untracked_after is None: + # Best-effort git failed on either side: we cannot prove the fix + # created no new untracked file, so report the count as unknown + # (``None``) and let the orchestrator refuse autofix. + new_untracked = None + else: + new_untracked = sorted(set(untracked_after) - set(untracked_before)) + verdict["final_verification"] = { + "ran": True, + "all_ok": all(c["ok"] for c in final_checks), + "failed": [c["name"] for c in final_checks if not c["ok"]], + "new_untracked_files": ( + new_untracked[:changed_files_cap] if new_untracked is not None else None + ), + "new_untracked_count": (len(new_untracked) if new_untracked is not None else None), + } -print("EGG_GREEN_GATE_VERDICT:" + json.dumps({"checks": results}), flush=True) +print("EGG_GREEN_GATE_VERDICT:" + json.dumps(verdict), flush=True) """ @@ -614,6 +772,7 @@ def _build_runner_job_manifest( full_env["EGG_GREEN_GATE_CHECKS"] = json.dumps(checks) full_env["EGG_GREEN_GATE_REPO_DIR"] = repo_dir full_env["EGG_GREEN_GATE_OUTPUT_TAIL"] = str(_VERDICT_OUTPUT_TAIL_CHARS) + full_env["EGG_GREEN_GATE_CHANGED_FILES_CAP"] = str(_FIX_CHANGED_FILES_CAP) full_env["EGG_GREEN_GATE_INFRA_SIGNATURES"] = json.dumps( { "line": list(_INFRA_LINE_SIGNATURES), @@ -877,6 +1036,198 @@ def _format_failed_checks(failed: list[dict[str, Any]]) -> str: return "\n\n".join(parts) +def _all_failed_checks_fixed(failed: list[dict[str, Any]]) -> bool: + """True when every failed check carries a fix whose re-run went green (#3409). + + This is the first gate inside :func:`_autofix_ready`, factored out so + the caller can distinguish "no fix was available" — where the reds in + the failure message are genuinely the operator's to fix and the + message is self-explanatory — from "every red *was* fixed and the + gate still refused", where the message needs to say why or it + contradicts what the operator sees when they re-run the fix. + """ + return all( + isinstance(c.get("fix"), dict) and c["fix"].get("check_ok_after_fix") for c in failed + ) + + +def _autofix_ready(verdict: dict[str, Any], failed: list[dict[str, Any]]) -> tuple[bool, str]: + """Decide whether the runner's fixed tree is safe to commit + push (#3409). + + Returns ``(ready, reason)``. ``reason`` is empty when ready and a + short human-readable explanation of the block otherwise (logged, so + an operator can see *why* a fixable-looking verdict routed to the + slice team instead of self-healing). + + The bar is deliberately stronger than "every failed check's own + re-run went green": that only validates intermediate trees (each + check re-ran before later fixes, and originally-green checks were + never re-run), whereas the orchestrator commits one tree with all + fixes applied. Autofix is safe only when: + + * every failed check carries a fix whose re-run went green (a partial + fix would push a tree that is red by construction); **and** + * the runner's ``final_verification`` — one full re-run of *every* + configured check against the final fixed tree — went green; **and** + * that final tree created no new untracked files. ``git add -u`` (the + orchestrator's stage step) never picks up untracked files, so a + fix that emits a new source file would push a tree the final + re-run never validated as committed. Unknown (``None``) counts — + a best-effort git failure in the runner — are treated as unsafe. + """ + if not _all_failed_checks_fixed(failed): + return False, "a failed check has no fix or its re-run stayed red" + + final = verdict.get("final_verification") + if not isinstance(final, dict) or not final.get("ran"): + return False, "runner reported no final full re-run verdict" + if not final.get("all_ok"): + blocked = ", ".join(str(n) for n in (final.get("failed") or [])) or "unknown" + return False, f"final full re-run of all checks was not green (red: {blocked})" + + new_untracked = final.get("new_untracked_count") + if new_untracked is None: + return False, "runner could not determine whether the fix created untracked files" + if new_untracked: + sample = ", ".join(str(p) for p in (final.get("new_untracked_files") or [])[:5]) + return False, ( + f"fix created {new_untracked} untracked file(s) that git add -u would drop" + f"{f' (e.g. {sample})' if sample else ''}" + ) + + return True, "" + + +def _commit_and_push_autofix( + gateway: Any, + *, + pipeline_id: str, + slice_id: str, + worktree_path: str, + integration_branch: str, + gateway_mode: Literal["public", "private"], + fixed_checks: list[dict[str, Any]], +) -> str | None: + """Commit the runner's fix output and push it to the integration branch (#3409). + + The runner executed each failed check's configured ``fix`` command + inside the hostPath-mounted gateway worktree and re-ran the checks + green, so the tree at ``worktree_path`` is exactly the tree the + checks validated. This stages the tracked modifications + (``git add -u``: untracked check droppings such as caches and + selection JSON are never picked up), commits them under the + orchestrator's green-gate identity. Every git invocation disables + hooks with ``-c core.hooksPath=/dev/null`` — the state-store + precedent (``StateStore._run_git`` / ``agent_salvage._run_git``) for + running git on an agent-controlled worktree — and the commit adds + ``--no-verify`` on top; ``--no-verify`` alone would leave + ``post-commit`` and ``git add``'s hooks free to run. The push goes + via the launcher-authed gateway push route, the sanctioned writer: + the runner never pushes. + + The commit needs no second runner pass because the runner's final + full-check re-run (``final_verification``) validated *this* tree — + every configured check, after all fixes were applied — and the caller + only reaches this function when that re-run went green with no + fix-created untracked files, so the ``git add -u`` tip is exactly the + validated tree. + + The push lands before any slice-close side effect, so dependent + slices (which fork from the integration branch's remote tip when + they start) fork after the format commit and cannot inherit + unformatted code that would re-trip the gate. + + Returns ``None`` on success, or a human-readable error string; the + caller then blocks the slice exactly like an unfixed red (Stage B + behavior) with the error appended to the failure message. + """ + + def _git(*args: str, identity: bool = False) -> subprocess.CompletedProcess[str]: + # ``core.hooksPath=/dev/null`` neutralizes every hook (state-store + # precedent, ``StateStore._run_git`` / ``agent_salvage._run_git``): + # the worktree tree is agent-produced integration-branch code, and + # the orchestrator must never execute its hooks. ``--no-verify`` on + # the commit is not enough — it suppresses only ``pre-commit`` / + # ``commit-msg``, leaving ``post-commit`` (and ``git add``'s hooks) + # free to run whatever the tree's ``core.hooksPath`` resolves to. + cmd = ["git", "-c", "core.hooksPath=/dev/null", "-C", worktree_path] + if identity: + cmd += [ + "-c", + f"user.name={_AUTOFIX_COMMIT_NAME}", + "-c", + f"user.email={_AUTOFIX_COMMIT_EMAIL}", + ] + cmd += list(args) + return subprocess.run( + cmd, + capture_output=True, + text=True, + check=False, + timeout=_AUTOFIX_GIT_TIMEOUT_SECONDS, + ) + + fixed_names = ", ".join(str(c.get("name")) for c in fixed_checks) + try: + add = _git("add", "-u") + if add.returncode != 0: + return f"git add -u failed: {(add.stderr or add.stdout or '').strip()}" + + staged = _git("diff", "--cached", "--name-only") + if staged.returncode != 0: + return ( + "git diff --cached --name-only failed: " + f"{(staged.stderr or staged.stdout or '').strip()}" + ) + staged_files = [line for line in (staged.stdout or "").splitlines() if line.strip()] + if not staged_files: + # The re-run went green without the fix modifying any + # tracked file (a flaky first run). Nothing committable can + # make the remote tip green, so refuse rather than pass a + # tip whose red verdict stands as pushed. + return ( + "fix commands re-ran the checks green but left no tracked modifications to commit" + ) + + message = ( + f"Apply configured check autofix at the green gate: {fixed_names}\n\n" + f"Automated commit for pipeline {pipeline_id}, slice {slice_id} " + f"(#3409). The green-gate runner found the named checks red at " + f"the {integration_branch} tip, ran their configured fix " + f"commands, and re-ran the checks green." + ) + commit = _git("commit", "--no-verify", "-m", message, identity=True) + if commit.returncode != 0: + return f"git commit failed: {(commit.stderr or commit.stdout or '').strip()}" + except (OSError, subprocess.SubprocessError) as exc: + return f"autofix git operation raised: {exc}" + + try: + push = gateway.push_worktree_branch( + pipeline_id, + repo_path=worktree_path, + branch=integration_branch, + mode=gateway_mode, + ) + except Exception as exc: # noqa: BLE001 - push failure blocks like an unfixed red + return f"autofix push to {integration_branch} raised: {exc}" + if not getattr(push, "ok", False): + return ( + f"autofix push to {integration_branch} failed " + f"({getattr(push, 'category', 'unknown')}): {getattr(push, 'detail', '')}" + ) + + logger.info( + "Green gate autofix committed and pushed (#3409)", + pipeline_id=pipeline_id, + slice_id=slice_id, + integration_branch=integration_branch, + fixed_checks=fixed_names, + staged_file_count=len(staged_files), + ) + return None + + def run_slice_green_gate( pipeline_id: str, spawner: "KubernetesSpawner", # noqa: UP037 @@ -890,8 +1241,9 @@ def run_slice_green_gate( Runs after slice consensus and the #3125 evidence gate, before any close side effect. Returns ``None`` when the slice may close: checks - green, gate off/log-mode, an infrastructure failure (fail-open), or - a red verdict where every failed check carries an infra tag (#3417). + green, gate off/log-mode, an infrastructure failure (fail-open), a + red verdict where every failed check carries an infra tag (#3417), + or a red verdict fully remediated by the #3409 autofix commit. Otherwise returns a human-readable failure string naming the genuinely red checks; the caller records the slice failure with it, routing through the existing cascade + OVERSEER_ALERT machinery @@ -990,10 +1342,14 @@ def run_slice_green_gate( repo_mounts: dict[str, str] = {} repo_dir = "" + repo_host_dir = "" for host_path in wt_result.worktrees.values(): container_path = f"/home/egg/repos/{os.path.basename(host_path)}" repo_mounts[container_path] = host_path repo_dir = container_path + # Orchestrator-side path of the same worktree: the #3409 + # autofix stages/commits here after the runner pod exits. + repo_host_dir = host_path from kubernetes_spawner import GATEWAY_K8S_URL @@ -1101,6 +1457,22 @@ def run_slice_green_gate( # is infra-tagged; when genuine and infra reds mix, block on the # genuine reds only so the failure routed to the cascade doesn't # send anyone chasing an infra ghost. + # + # This runs *before* the #3409 autofix decision, so autofix is + # judged on the genuine reds only. Note an infra-tagged red *can* + # carry a fix that cleared it: ``infra`` is tagged from the check's + # first run and never recomputed, while the runner applies a + # configured ``fix`` to any red, so a check that was SIGKILLed and + # then re-ran clean after its fix has both ``infra`` set and + # ``check_ok_after_fix: True``. Dropping it here deliberately + # discards that evidence: the fail-open path returns before any + # push, so the proven fix is thrown away with the worktree rather + # than committed on the strength of a run we already classified as + # untrustworthy. The narrowing is safe in the push direction — + # ``_autofix_ready`` gates on ``final_verification.all_ok``, which + # is computed over *every* configured check including infra-tagged + # ones, so removing reds from this list can only remove reasons to + # push, never add one. genuine_failed = failed if _infra_fail_open_enabled(): infra_failed = [c for c in failed if c.get("infra")] @@ -1125,6 +1497,12 @@ def run_slice_green_gate( return None failed_names = ", ".join(str(c.get("name")) for c in genuine_failed) + # #3409 Stage A: the gate can self-heal only when it can prove the + # tree the orchestrator will commit (all fixes applied) is green. + # ``_autofix_ready`` gates on the runner's final full re-run of + # every check plus a no-new-untracked-files check, not just each + # failed check's own intermediate re-run — see its docstring. + autofix_ready, autofix_block_reason = _autofix_ready(verdict, genuine_failed) logger.error( "Green gate red: configured checks failed at the slice tip (#3398)", pipeline_id=pipeline_id, @@ -1132,14 +1510,65 @@ def run_slice_green_gate( gate_id=gate_id, integration_branch=integration_branch, failed_checks=failed_names, + autofix_ready=autofix_ready, + autofix_block_reason=autofix_block_reason or None, mode=mode, ) + autofix_note = "" + if autofix_ready and mode == "on" and repo_host_dir: + autofix_error = _commit_and_push_autofix( + spawner.gateway, + pipeline_id=pipeline_id, + slice_id=slice_id, + worktree_path=repo_host_dir, + integration_branch=integration_branch, + gateway_mode=gateway_mode, + fixed_checks=genuine_failed, + ) + if autofix_error is None: + # The fixed tree the runner re-validated green is now + # the integration-branch tip; the slice may close. + return None + logger.warning( + "Green gate autofix failed; blocking slice like an unfixed red (#3409)", + pipeline_id=pipeline_id, + slice_id=slice_id, + gate_id=gate_id, + integration_branch=integration_branch, + error=autofix_error, + ) + autofix_note = ( + f"\n\nThe configured fix commands turned the checks green in " + f"the runner, but committing/pushing the fix failed: " + f"{autofix_error}" + ) + elif autofix_ready and mode == "log": + logger.info( + "Green gate log mode: autofix available but not applied (#3409)", + pipeline_id=pipeline_id, + slice_id=slice_id, + gate_id=gate_id, + failed_checks=failed_names, + ) + elif not autofix_ready and _all_failed_checks_fixed(genuine_failed): + # #3409: every listed red had a fix that worked, yet the gate + # still refuses to self-heal — the real blocker is something + # the failure message alone doesn't name (a fix that regressed + # a sibling check; an infra red filtered out above; a fix that + # emitted an untracked file). Without this line the operator + # re-runs the fix command, watches it succeed, and has no way + # to see why the gate said no. + autofix_note = ( + f"\n\nThe configured fix commands cleared the checks above in " + f"the runner, but the gate did not self-heal: {autofix_block_reason}." + ) if mode == "log": return None return ( f"slice {slice_id}: green gate failed — configured checks are red " f"at integration branch {integration_branch} tip: {failed_names}.\n\n" - f"{_format_failed_checks(genuine_failed)}\n\n" + f"{_format_failed_checks(genuine_failed)}" + f"{autofix_note}\n\n" f"Fix the failures on {integration_branch} and restart the slice; " f"set {GREEN_GATE_ENV_VAR}=off to bypass." ) diff --git a/orchestrator/tests/test_slice_green_gate.py b/orchestrator/tests/test_slice_green_gate.py index 91d8d722e3..8869f535de 100644 --- a/orchestrator/tests/test_slice_green_gate.py +++ b/orchestrator/tests/test_slice_green_gate.py @@ -11,8 +11,12 @@ * ``_RUNNER_PROGRAM`` — executed for real in a subprocess: check execution + verdict shape, output tails, the prebuilt-deps restore (copy-if-missing), the required-but-missing infra exit, and the - #3417 infra tagging (signature match over full output, SIGKILL exit, - green checks never tagged). + #3409 fix flow (fix executed only on a red check, re-run verdict, + changed-files reporting + cap, no-git degrade), and the #3417 infra + tagging (signature match over full output, SIGKILL exit, green checks + never tagged). +* ``_commit_and_push_autofix`` — real-git stage/commit + gateway push + wiring, the no-tracked-changes refusal, and push-failure reporting. * ``_build_runner_job_manifest`` — labels (NetworkPolicy component label present; monitor/agent-supervision labels absent), env, mounts, deadline. @@ -323,8 +327,8 @@ def test_invalid_falls_back(self, gate_env: pytest.MonkeyPatch, value: str) -> N # ---------------------------------------------------------------------- -def _verdict_line(checks: list[dict[str, Any]]) -> str: - return sgg.VERDICT_SENTINEL + json.dumps({"checks": checks}) +def _verdict_line(checks: list[dict[str, Any]], **extra: Any) -> str: + return sgg.VERDICT_SENTINEL + json.dumps({"checks": checks, **extra}) class TestParseVerdict: @@ -368,6 +372,7 @@ def _run_runner( *, require_prebuilt: str = "0", prebuilt_base: Path | None = None, + extra_env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess[str]: repo_dir = tmp_path / "egg" repo_dir.mkdir(exist_ok=True) @@ -389,6 +394,7 @@ def _run_runner( ), } ) + env.update(extra_env or {}) return subprocess.run( [sys.executable, "-c", sgg._RUNNER_PROGRAM], capture_output=True, @@ -611,6 +617,181 @@ def test_optional_prebuilt_missing_proceeds(self, tmp_path: Path) -> None: assert verdict["checks"][0]["ok"] is True +def _git(repo_dir: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], + cwd=repo_dir, + capture_output=True, + text=True, + check=True, + ) + + +def _init_git_repo(repo_dir: Path) -> None: + repo_dir.mkdir(exist_ok=True) + _git(repo_dir, "init", "-q", ".") + (repo_dir / "file.txt").write_text("bad\n") + _git(repo_dir, "add", "file.txt") + _git(repo_dir, "commit", "-q", "-m", "init") + + +class TestRunnerFixFlow: + """#3409 — the runner's fix execution + re-run reporting.""" + + FIXABLE_CHECK = { + "name": "lint", + "command": "grep -q good file.txt", + "fix": "printf 'good\\n' > file.txt", + } + + def test_red_check_with_fix_reports_fix_result(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + (tmp_path / "egg" / "junk.log").write_text("untracked check dropping") + proc = _run_runner(tmp_path, [dict(self.FIXABLE_CHECK)]) + assert proc.returncode == 0 + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + entry = verdict["checks"][0] + # The tip as pushed is still red; only the orchestrator commit + # may turn the verdict green. + assert entry["ok"] is False + fix = entry["fix"] + assert fix["command"] == self.FIXABLE_CHECK["fix"] + assert fix["exit_code"] == 0 + assert fix["check_ok_after_fix"] is True + # Tracked modification reported; untracked droppings are not. + assert fix["changed_files"] == ["file.txt"] + assert fix["changed_file_count"] == 1 + + def test_fix_that_does_not_repair_reports_red_rerun(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + proc = _run_runner( + tmp_path, + [{"name": "lint", "command": "grep -q good file.txt", "fix": "true"}], + ) + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + fix = verdict["checks"][0]["fix"] + assert fix["check_ok_after_fix"] is False + assert fix["changed_files"] == [] + + def test_green_check_never_runs_fix(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + proc = _run_runner( + tmp_path, + [{"name": "lint", "command": "true", "fix": "touch fix-ran.marker"}], + ) + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + assert "fix" not in verdict["checks"][0] + assert not (tmp_path / "egg" / "fix-ran.marker").exists() + + def test_red_check_without_fix_reports_plain_red(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + proc = _run_runner(tmp_path, [{"name": "lint", "command": "false"}]) + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + entry = verdict["checks"][0] + assert entry["ok"] is False + assert "fix" not in entry + + def test_changed_files_capped_but_count_exact(self, tmp_path: Path) -> None: + repo_dir = tmp_path / "egg" + _init_git_repo(repo_dir) + for i in range(3): + (repo_dir / f"extra{i}.txt").write_text("bad\n") + _git(repo_dir, "add", f"extra{i}.txt") + _git(repo_dir, "commit", "-q", "-m", "more files") + proc = _run_runner( + tmp_path, + [ + { + "name": "lint", + "command": "grep -q good file.txt", + "fix": "for f in file.txt extra0.txt extra1.txt extra2.txt; " + "do printf 'good\\n' > \"$f\"; done", + } + ], + extra_env={"EGG_GREEN_GATE_CHANGED_FILES_CAP": "2"}, + ) + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + fix = verdict["checks"][0]["fix"] + assert fix["check_ok_after_fix"] is True + assert len(fix["changed_files"]) == 2 + assert fix["changed_file_count"] == 4 + + def test_no_git_repo_degrades_changed_files_to_none(self, tmp_path: Path) -> None: + # No git init: the gateway-routed git diff is best-effort and a + # failure must degrade to an unreported list, not a crash. + # + # Assumption: pytest's ``tmp_path`` (under the system temp root, + # e.g. ``/tmp``) is NOT nested inside any git repository, so the + # runner's ``git diff`` / ``git ls-files`` genuinely fail. If the + # tmp root ever moves under a checkout, git would succeed against + # the enclosing repo and these ``is None`` assertions would flip — + # a confusing failure that this note is here to explain. + repo_dir = tmp_path / "egg" + repo_dir.mkdir(exist_ok=True) + (repo_dir / "file.txt").write_text("bad\n") + proc = _run_runner(tmp_path, [dict(self.FIXABLE_CHECK)]) + assert proc.returncode == 0 + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + fix = verdict["checks"][0]["fix"] + assert fix["check_ok_after_fix"] is True + assert fix["changed_files"] is None + assert fix["changed_file_count"] is None + # Best-effort git failed, so the untracked delta is unknown and + # the orchestrator will refuse autofix (fail-safe). + final = verdict["final_verification"] + assert final["new_untracked_count"] is None + + def test_final_verification_green_for_fixed_tree(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + proc = _run_runner(tmp_path, [dict(self.FIXABLE_CHECK)]) + assert proc.returncode == 0 + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + final = verdict["final_verification"] + assert final["ran"] is True + # The final full re-run of every check against the fixed tree is + # green, and the fix only touched a tracked file. + assert final["all_ok"] is True + assert final["failed"] == [] + assert final["new_untracked_count"] == 0 + + def test_final_verification_flags_fix_created_untracked_file(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + # The fix repairs file.txt AND emits a new, non-ignored source + # file that git add -u would never stage. + proc = _run_runner( + tmp_path, + [ + { + "name": "lint", + "command": "grep -q good file.txt", + "fix": "printf 'good\\n' > file.txt; printf 'x\\n' > generated.py", + } + ], + ) + assert proc.returncode == 0 + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + final = verdict["final_verification"] + assert final["all_ok"] is True + assert final["new_untracked_count"] == 1 + assert final["new_untracked_files"] == ["generated.py"] + + def test_no_fix_applied_omits_final_verification(self, tmp_path: Path) -> None: + _init_git_repo(tmp_path / "egg") + proc = _run_runner(tmp_path, [{"name": "lint", "command": "true"}]) + verdict = sgg.parse_verdict(proc.stdout) + assert verdict is not None + # No fix ran, so there is no combined tree to re-validate. + assert "final_verification" not in verdict + + # ---------------------------------------------------------------------- # _build_runner_job_manifest # ---------------------------------------------------------------------- @@ -1307,4 +1488,480 @@ def test_manifest_env_marks_prebuilt_required( } assert env["EGG_GREEN_GATE_REQUIRE_PREBUILT"] == "1" assert env["EGG_SESSION_TOKEN"] == "tok-123" + assert env["EGG_GREEN_GATE_CHANGED_FILES_CAP"] == str(sgg._FIX_CHANGED_FILES_CAP) assert json.loads(env["EGG_GREEN_GATE_CHECKS"]) == CHECKS + + +# ---------------------------------------------------------------------- +# _commit_and_push_autofix (#3409) +# ---------------------------------------------------------------------- + + +def _autofix_repo(tmp_path: Path) -> Path: + repo_dir = tmp_path / "egg" + _init_git_repo(repo_dir) + return repo_dir + + +def _run_autofix(repo_dir: Path, gateway: MagicMock) -> str | None: + return sgg._commit_and_push_autofix( + gateway, + pipeline_id=PIPELINE_ID, + slice_id=SLICE_ID, + worktree_path=str(repo_dir), + integration_branch=INTEGRATION_BRANCH, + gateway_mode="public", + fixed_checks=[{"name": "lint", "fix": {"check_ok_after_fix": True}}], + ) + + +class TestCommitAndPushAutofix: + def test_stages_commits_and_pushes(self, tmp_path: Path) -> None: + repo_dir = _autofix_repo(tmp_path) + (repo_dir / "file.txt").write_text("good\n") + (repo_dir / "junk.log").write_text("untracked check dropping") + gateway = MagicMock() + gateway.push_worktree_branch.return_value = SimpleNamespace(ok=True) + + assert _run_autofix(repo_dir, gateway) is None + + head = _git(repo_dir, "log", "-1", "--format=%an|%ae|%s") + author, email, subject = head.stdout.strip().split("|") + assert author == "egg-green-gate" + assert email == "egg-green-gate@localhost" + assert "lint" in subject + # Untracked droppings never enter the commit. + shown = _git(repo_dir, "show", "--name-only", "--format=", "HEAD") + assert shown.stdout.split() == ["file.txt"] + gateway.push_worktree_branch.assert_called_once_with( + PIPELINE_ID, + repo_path=str(repo_dir), + branch=INTEGRATION_BRANCH, + mode="public", + ) + + def test_no_tracked_changes_refuses_without_push(self, tmp_path: Path) -> None: + repo_dir = _autofix_repo(tmp_path) + gateway = MagicMock() + error = _run_autofix(repo_dir, gateway) + assert error is not None + assert "no tracked" in error + gateway.push_worktree_branch.assert_not_called() + + def test_push_failure_is_reported(self, tmp_path: Path) -> None: + repo_dir = _autofix_repo(tmp_path) + (repo_dir / "file.txt").write_text("good\n") + gateway = MagicMock() + gateway.push_worktree_branch.return_value = SimpleNamespace( + ok=False, category="auth_failed", detail="denied" + ) + error = _run_autofix(repo_dir, gateway) + assert error is not None + assert "auth_failed" in error + assert "denied" in error + + def test_push_raising_is_reported(self, tmp_path: Path) -> None: + repo_dir = _autofix_repo(tmp_path) + (repo_dir / "file.txt").write_text("good\n") + gateway = MagicMock() + gateway.push_worktree_branch.side_effect = RuntimeError("gateway down") + error = _run_autofix(repo_dir, gateway) + assert error is not None + assert "gateway down" in error + + def test_not_a_git_repo_is_reported(self, tmp_path: Path) -> None: + repo_dir = tmp_path / "not-a-repo" + repo_dir.mkdir() + gateway = MagicMock() + error = _run_autofix(repo_dir, gateway) + assert error is not None + gateway.push_worktree_branch.assert_not_called() + + +# ---------------------------------------------------------------------- +# run_slice_green_gate — #3409 autofix wiring +# ---------------------------------------------------------------------- + + +def _fixed(ok: bool = True) -> dict[str, Any]: + return { + "command": "make lint-fix", + "exit_code": 0, + "check_ok_after_fix": ok, + "changed_files": ["a.py"], + "changed_file_count": 1, + "output_tail": "", + "recheck_output_tail": "", + } + + +def _final_verification( + *, + all_ok: bool = True, + failed: list[str] | None = None, + new_untracked_count: int | None = 0, + new_untracked_files: list[str] | None = None, +) -> dict[str, Any]: + return { + "ran": True, + "all_ok": all_ok, + "failed": failed or [], + "new_untracked_files": (new_untracked_files if new_untracked_files is not None else []), + "new_untracked_count": new_untracked_count, + } + + +def _red_lint_verdict(*, fix: dict[str, Any] | None, final: Any = "default") -> str: + entry: dict[str, Any] = { + "name": "lint", + "ok": False, + "exit_code": 1, + "output_tail": "would reformat a.py", + } + if fix is not None: + entry["fix"] = fix + checks = [entry, {"name": "test", "ok": True, "exit_code": 0, "output_tail": ""}] + # A fixable verdict carries the runner's final full re-run by default; + # pass ``final=None`` to model an old/degraded runner that omitted it. + if final == "default": + final = _final_verification() if fix is not None else None + if final is not None: + return _verdict_line(checks, final_verification=final) + return _verdict_line(checks) + + +class TestGreenGateAutofixWiring: + def test_fixed_red_verdict_pushes_and_passes( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=_red_lint_verdict(fix=_fixed())), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix", return_value=None) as autofix, + ): + assert _run_gate(spawner) is None + autofix.assert_called_once() + kwargs = autofix.call_args.kwargs + # The autofix stages the SAME hostPath worktree the runner + # mutated, and pushes to the slice integration branch. + assert kwargs["worktree_path"] == "/home/host/.egg-worktrees/runner/egg" + assert kwargs["integration_branch"] == INTEGRATION_BRANCH + assert kwargs["gateway_mode"] == "public" + assert [c["name"] for c in kwargs["fixed_checks"]] == ["lint"] + # Cleanup still runs after the autofix path. + spawner.gateway.delete_session_by_container.assert_called_once() + spawner.gateway.delete_worktrees.assert_called_once() + + def test_autofix_failure_blocks_with_note( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=_red_lint_verdict(fix=_fixed())), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix", return_value="push exploded"), + ): + failure = _run_gate(spawner) + assert failure is not None + assert "lint" in failure + assert "push exploded" in failure + + def test_rerun_still_red_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object( + sgg, "_read_runner_log", return_value=_red_lint_verdict(fix=_fixed(ok=False)) + ), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + failure = _run_gate(spawner) + assert failure is not None + autofix.assert_not_called() + + def test_partially_fixable_verdict_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + spawner = _spawner() + log = _verdict_line( + [ + {"name": "lint", "ok": False, "exit_code": 1, "output_tail": "", "fix": _fixed()}, + {"name": "test", "ok": False, "exit_code": 2, "output_tail": "FAILED"}, + ] + ) + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + failure = _run_gate(spawner) + assert failure is not None + autofix.assert_not_called() + + def test_log_mode_never_pushes_a_fix( + self, gate_env: pytest.MonkeyPatch, configured_checks: None + ) -> None: + gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, "log") + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=_red_lint_verdict(fix=_fixed())), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + assert _run_gate(spawner) is None + autofix.assert_not_called() + + def _assert_blocks_without_autofix(self, spawner: MagicMock, log: str) -> None: + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + failure = _run_gate(spawner) + assert failure is not None + autofix.assert_not_called() + + def test_final_rerun_red_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # #3409: every failed check's own re-run went green, but the final + # full re-run of all checks against the combined tree is red (a + # fix broke another check) — the committed tip would be red. + log = _red_lint_verdict( + fix=_fixed(), final=_final_verification(all_ok=False, failed=["test"]) + ) + self._assert_blocks_without_autofix(_spawner(), log) + + def test_fix_created_untracked_files_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # #3409: the fix emitted a new source file. ``git add -u`` would + # drop it, so the pushed tip omits it and the check is red as + # pushed even though the runner's on-disk re-run was green. + log = _red_lint_verdict( + fix=_fixed(), + final=_final_verification(new_untracked_count=1, new_untracked_files=["gen/new.py"]), + ) + self._assert_blocks_without_autofix(_spawner(), log) + + def test_unknown_untracked_count_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # #3409: a best-effort git failure left the untracked delta + # unknown; the gate refuses rather than risk a red pushed tip. + log = _red_lint_verdict(fix=_fixed(), final=_final_verification(new_untracked_count=None)) + self._assert_blocks_without_autofix(_spawner(), log) + + def test_missing_final_verification_blocks_without_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # #3409: an old/degraded runner that omitted final_verification + # cannot prove the committed tree is green — refuse autofix. + log = _red_lint_verdict(fix=_fixed(), final=None) + self._assert_blocks_without_autofix(_spawner(), log) + + +def _infra_plus_fixable_verdict(*, final: dict[str, Any]) -> str: + """An infra-tagged red (#3417) co-occurring with a fixable red (#3409).""" + return _verdict_line( + [ + { + "name": "test", + "ok": False, + "exit_code": 137, + "output_tail": "GATEWAY SIDECAR NOT AVAILABLE", + "infra": "GATEWAY SIDECAR NOT AVAILABLE", + }, + { + "name": "lint", + "ok": False, + "exit_code": 1, + "output_tail": "would reformat a.py", + "infra": None, + "fix": _fixed(), + }, + ], + final_verification=final, + ) + + +class TestInfraFailOpenAutofixComposition: + """#3417 infra fail-open composed with #3409 autofix. + + The gate narrows ``failed`` to ``genuine_failed`` *before* the autofix + decision, so ``_autofix_ready`` and ``_commit_and_push_autofix`` both + see only the non-infra reds. Neither #3417 nor #3409 alone exercises + this: the infra tests build verdicts with no ``fix`` block and the + autofix tests build verdicts with no ``infra`` field. + """ + + def test_infra_red_alongside_fixable_red_pushes_only_the_genuine_fix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # The infra-tagged red is filtered out, the genuine red's fix went + # green, and the final full re-run — which covers *every* check, + # infra-tagged ones included — is green, so the tip is provably + # green and the autofix pushes. + log = _infra_plus_fixable_verdict(final=_final_verification()) + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix", return_value=None) as autofix, + ): + assert _run_gate(spawner) is None + autofix.assert_called_once() + # Only the genuine red is reported as fixed — the infra red never + # reaches the commit path even though it was red at the tip. + assert [c["name"] for c in autofix.call_args.kwargs["fixed_checks"]] == ["lint"] + + def test_infra_red_still_red_in_final_rerun_blocks_the_fixable_red( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # The infra red is filtered from the *presented* failures, but the + # final full re-run still covers it — so a persistent infra fault + # blocks the push rather than letting a tree only partly proven + # green reach the integration branch. + log = _infra_plus_fixable_verdict(final=_final_verification(all_ok=False, failed=["test"])) + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + failure = _run_gate(spawner) + autofix.assert_not_called() + assert failure is not None + # #3409: every red the operator is shown had a working fix, so the + # message must say why the gate refused to self-heal anyway — + # otherwise they re-run `make lint-fix`, watch it succeed, and see + # no reason for the block. The hidden check is named as the cause + # of the *autofix refusal*, not routed as a slice failure: its + # output tail stays out of the presented failure list (#3417). + assert "did not self-heal" in failure + assert "final full re-run of all checks was not green (red: test)" in failure + assert "GATEWAY SIDECAR NOT AVAILABLE" not in failure + + def test_no_note_when_the_genuine_red_had_no_fix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # Contrast: the genuine red carries no fix at all, so the reds in + # the message are the operator's own to fix and the failure text + # is self-explanatory. No autofix explanation is appended. + log = _verdict_line( + [ + { + "name": "lint", + "ok": False, + "exit_code": 1, + "output_tail": "infra", + "infra": "ENOSPC", + }, + {"name": "test", "ok": False, "exit_code": 2, "output_tail": "FAILED"}, + ] + ) + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + ): + failure = _run_gate(spawner) + assert failure is not None + assert "did not self-heal" not in failure + + def test_fail_open_switch_off_makes_the_infra_red_block_the_autofix( + self, enabled_gate: pytest.MonkeyPatch, configured_checks: None + ) -> None: + # With the #3417 switch off there is no narrowing, so the + # unfixable infra-tagged red stays in the set `_autofix_ready` + # judges and the verdict is only partially fixable — no push. + # This pins the narrowing itself as what enables the push above. + enabled_gate.setenv(sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, "off") + log = _infra_plus_fixable_verdict(final=_final_verification()) + spawner = _spawner() + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + patch.object(sgg, "_commit_and_push_autofix") as autofix, + ): + failure = _run_gate(spawner) + autofix.assert_not_called() + assert failure is not None + assert "test" in failure + + +class TestAutofixReady: + """#3409 — ``_autofix_ready`` gating on the final full re-run.""" + + LINT_FAILED = [{"name": "lint", "fix": {"check_ok_after_fix": True}}] + + def test_ready_when_final_green_and_no_new_untracked(self) -> None: + verdict = {"checks": [], "final_verification": _final_verification()} + ready, reason = sgg._autofix_ready(verdict, self.LINT_FAILED) + assert ready is True + assert reason == "" + + def test_not_ready_when_a_failed_check_is_unfixable(self) -> None: + failed = [{"name": "lint", "fix": {"check_ok_after_fix": True}}, {"name": "test"}] + verdict = {"checks": [], "final_verification": _final_verification()} + ready, reason = sgg._autofix_ready(verdict, failed) + assert ready is False + assert "no fix" in reason + + def test_not_ready_when_final_missing(self) -> None: + ready, reason = sgg._autofix_ready({"checks": []}, self.LINT_FAILED) + assert ready is False + assert "final full re-run" in reason + + def test_not_ready_when_final_red(self) -> None: + verdict = { + "checks": [], + "final_verification": _final_verification(all_ok=False, failed=["test"]), + } + ready, reason = sgg._autofix_ready(verdict, self.LINT_FAILED) + assert ready is False + assert "test" in reason + + def test_not_ready_when_untracked_created(self) -> None: + verdict = { + "checks": [], + "final_verification": _final_verification( + new_untracked_count=2, new_untracked_files=["a.py", "b.py"] + ), + } + ready, reason = sgg._autofix_ready(verdict, self.LINT_FAILED) + assert ready is False + assert "untracked" in reason + assert "a.py" in reason + + def test_not_ready_when_untracked_count_unknown(self) -> None: + verdict = { + "checks": [], + "final_verification": _final_verification(new_untracked_count=None), + } + ready, reason = sgg._autofix_ready(verdict, self.LINT_FAILED) + assert ready is False + assert "untracked" in reason diff --git a/scripts/file-size-allowlist.yaml b/scripts/file-size-allowlist.yaml index 865d85777b..ee54979e8d 100644 --- a/scripts/file-size-allowlist.yaml +++ b/scripts/file-size-allowlist.yaml @@ -32,3 +32,9 @@ files: issue: "3587" orchestrator/routes/pipelines/_slice_state.py: issue: "3586" + # Went over the cap only on the #3609 x #3409 merge: main (1165) and + # the Stage-A autofix branch (1421) each sat under it, the union + # (1594) does not. Most of the growth is module-docstring prose from + # both sides. + orchestrator/slice_green_gate.py: + issue: "3627" diff --git a/shared/egg_config/validators.py b/shared/egg_config/validators.py index c9b94009a2..60e52173cb 100644 --- a/shared/egg_config/validators.py +++ b/shared/egg_config/validators.py @@ -168,20 +168,30 @@ def validate_checks(checks: list[Any]) -> list[dict[str, str]]: Used by config, orchestrator, and compose to validate check definitions from YAML config or JSON env vars. + Each entry requires ``name`` and ``command``. An optional ``fix`` + key names a shell command that auto-remediates a failing check + (e.g. ``make lint-fix`` for a ``lint`` check); the per-slice green + gate runs it at the slice tip and commits the result (#3409). A + ``fix`` that is present but empty/None is dropped from the entry. + Args: checks: Raw list of check entries (e.g. from YAML or JSON). Returns: - List of {"name": "...", "command": "..."} dicts with only - valid entries retained. + List of {"name": "...", "command": "..."} dicts (plus "fix" + when configured) with only valid entries retained. """ if not isinstance(checks, list): return [] - return [ - {"name": str(c["name"]), "command": str(c["command"])} - for c in checks - if isinstance(c, dict) and "name" in c and "command" in c - ] + result = [] + for c in checks: + if not (isinstance(c, dict) and "name" in c and "command" in c): + continue + entry = {"name": str(c["name"]), "command": str(c["command"])} + if c.get("fix"): + entry["fix"] = str(c["fix"]) + result.append(entry) + return result def validate_port(port: int | str) -> tuple[bool, str | None]: diff --git a/tests/egg_config/test_validators.py b/tests/egg_config/test_validators.py index d54059e8e2..2bf5e03604 100644 --- a/tests/egg_config/test_validators.py +++ b/tests/egg_config/test_validators.py @@ -5,6 +5,7 @@ from egg_config.validators import ( mask_secret, validate_anthropic_key, + validate_checks, validate_email, validate_github_token, validate_non_empty, @@ -13,6 +14,61 @@ ) +class TestValidateChecks: + """Tests for validate_checks function.""" + + def test_valid_entries(self): + """Name and command are retained and coerced to strings.""" + result = validate_checks([{"name": "lint", "command": "make lint"}]) + assert result == [{"name": "lint", "command": "make lint"}] + + def test_non_list_input(self): + """Non-list input yields an empty list.""" + assert validate_checks({"name": "lint"}) == [] + assert validate_checks(None) == [] + + def test_malformed_entries_dropped(self): + """Entries missing name or command are filtered out.""" + result = validate_checks( + [ + {"name": "lint"}, + {"command": "make test"}, + "make lint", + {"name": "ok", "command": "true"}, + ] + ) + assert result == [{"name": "ok", "command": "true"}] + + def test_values_coerced_to_strings(self): + """Non-string values are coerced to strings.""" + result = validate_checks([{"name": 1, "command": 2, "fix": 3}]) + assert result == [{"name": "1", "command": "2", "fix": "3"}] + + def test_fix_key_preserved(self): + """The optional fix auto-remediation command survives (#3409).""" + result = validate_checks( + [ + {"name": "lint", "command": "make lint", "fix": "make lint-fix"}, + {"name": "test", "command": "make test"}, + ] + ) + assert result == [ + {"name": "lint", "command": "make lint", "fix": "make lint-fix"}, + {"name": "test", "command": "make test"}, + ] + + def test_empty_fix_dropped(self): + """A present-but-empty fix is dropped from the entry.""" + for empty in ("", None): + result = validate_checks([{"name": "lint", "command": "make lint", "fix": empty}]) + assert result == [{"name": "lint", "command": "make lint"}] + + def test_unknown_keys_dropped(self): + """Keys outside the schema never pass through.""" + result = validate_checks([{"name": "lint", "command": "make lint", "extra": "x"}]) + assert result == [{"name": "lint", "command": "make lint"}] + + class TestValidateUrl: """Tests for validate_url function."""