Normalize (or refuse) in publish_task's raw-apply fallback (#58)

## Problem

Observed on transformers PR
[#47281](huggingface/transformers#47281): a
serge task fixed a `glm46v` integration failure by editing
`modular_glm4v.py` (the modular *source*), but the opened PR contained
**only** that one file — the regenerated `modeling_glm4v.py` /
`modeling_glm46v.py` were missing. So the fix never reached the
generated file that actually runs, and the PR trips transformers' own
`repo-consistency` / `modular_conversion` CI.

## Root cause

The task ran the normalizer (`… utils/checkers.py …,modular_conversion,…
--fix`) inside the in-loop validation gate (`_validate_patch`), which
regenerates the `modeling_*.py` files into the worktree. But that task
exhausted its correction budget (`TASK_NORMALIZE_MAX_RETRIES`) without a
passing validation, so:

1. `prepare_task` saw `outcome["prepared"] == False` and **reset the
worktree** (discarding the regenerated files), and
2. `publish_task` took its **raw-apply fallback** — re-applying
`plan.patch` to a pristine checkout **without running the normalizer** —
so `collect_changes` saw only the one hand-edited file (`Change touches
1 file(s)`).

The fallback existed for "no normalizer configured / validation
abandoned", but for a repo with a modular/generated-file invariant it
silently shipped a repo-inconsistent patch.

## Fix

In `publish_task`'s fallback path, when a normalizer **is** configured:
- run it after applying the patch so any regenerated files are staged
into the commit, and
- **refuse** (return `no_change`) rather than open a PR the normalizer
rejects — never commit a raw, repo-inconsistent patch.

The shared `run_normalize` invocation is extracted into
`_run_repo_normalizer` (returncode `None` = infrastructure failure →
best-effort accept, preserving prior behavior) and reused from both
`_validate_patch` and `publish_task`.

## Tests

`PublishFallbackNormalizeTests` covers:
- normalizer regenerates a file → **both** patched + generated files
ride along in the commit;
- normalizer exits non-zero → **no PR**, worktree reset;
- no normalizer configured → raw patch committed unchanged (no
regression).

`pytest tests/test_tasks.py` → 34 passed. `ruff format` + `ruff check`
clean.

## Follow-ups (not in this PR)

- The task also burned its entire input-token budget *exploring* before
writing a patch, leaving no room to iterate — worth guarding.
- Consider bumping `TASK_NORMALIZE_MAX_RETRIES` so genuinely-fixable
patches converge before the budget runs out.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tarekziade

reviewbot/tasks.py

@@ -316,6 +316,45 @@ def format_pr_files_diff(files: list[dict[str, Any]], *, limit: int = 30000) ->
 # ---------------------------------------------------------------------------
 # Agentic loop → patch (with in-loop normalize validation)
 # ---------------------------------------------------------------------------
+def _run_repo_normalizer(
+    cfg: Config,
+    checkout: Checkout,
+    emit: Callable[[str, str], None],
+) -> tuple[Optional[int], str]:
+    """Run the configured repo normalizer (``cfg.task_normalize_command``) over
+    the current worktree with its fixers enabled, writing any regenerated files
+    (e.g. transformers' ``modeling_*.py`` from ``modular_*.py``) back in place.
+
+    Returns ``(returncode, tail)``. ``returncode`` is ``None`` when the
+    normalizer could not run at all (sandbox unavailable / timeout / launch
+    failure) — infrastructure, not the patch's fault, which callers treat as a
+    best-effort pass. Assumes ``cfg.task_normalize_command`` is set."""
+    command = cfg.task_normalize_command
+    assert command is not None
+    emit("step", "normalize")
+    emit("log", f"Running the repo normalizer: `{' '.join(command)}`…")
+    try:
+        return run_normalize(
+            command,
+            workdir=checkout.path,
+            write_root=checkout.path,
+            backend=cfg.task_sandbox_backend,
+            image=cfg.task_normalize_image,
+            mode=cfg.helper_sandbox,
+            timeout=cfg.task_normalize_timeout,
+            memory=cfg.task_normalize_memory,
+        )
+    except NormalizeError as exc:
+        # Infrastructure problem (sandbox unavailable, timeout) — not the
+        # model's fault. Signal best-effort with a None returncode; CI still
+        # catches anything the normalizer would have.
+        log.warning("normalizer unavailable: %s", exc)
+        emit(
+            "log", f"Normalizer unavailable ({exc}); accepting the patch un-normalized."
+        )
+        return None, ""
+
+
 def _validate_patch(
     cfg: Config,
     *,
@@ -366,27 +405,10 @@ def _validate_patch(
             False,
         )
 
-    emit("step", "normalize")
-    emit("log", f"Validating the patch with `{' '.join(command)}`…")
-    try:
-        returncode, tail = run_normalize(
-            command,
-            workdir=checkout.path,
-            write_root=checkout.path,
-            backend=cfg.task_sandbox_backend,
-            image=cfg.task_normalize_image,
-            mode=cfg.helper_sandbox,
-            timeout=cfg.task_normalize_timeout,
-            memory=cfg.task_normalize_memory,
-        )
-    except NormalizeError as exc:
-        # Infrastructure problem (sandbox unavailable, timeout) — not the
-        # model's fault. Accept the applied patch best-effort rather than
-        # blaming the LLM; CI still catches anything the normalizer would have.
-        log.warning("normalizer unavailable during validation: %s", exc)
-        emit(
-            "log", f"Normalizer unavailable ({exc}); accepting the patch un-normalized."
-        )
+    returncode, tail = _run_repo_normalizer(cfg, checkout, emit)
+    if returncode is None:
+        # Normalizer could not run (infra) — accept the applied patch
+        # best-effort rather than blaming the LLM.
         return None, True
 
     if returncode != 0:
@@ -756,7 +778,9 @@ def publish_task(
     (:func:`_validate_patch`) already applied + normalized the worktree, so we
     just stage and commit it. Otherwise we apply ``plan.patch`` here (the path
     taken when no normalizer is configured, or when validation was abandoned
-    and left a clean checkout)."""
+    and left a clean checkout) and, if a normalizer *is* configured, re-run it
+    so regenerated files ride along — refusing to open a PR it rejects rather
+    than committing a raw, repo-inconsistent patch."""
 
     def _emit(kind: str, text: str) -> None:
         if emit is not None:
@@ -780,6 +804,30 @@ def _emit(kind: str, text: str) -> None:
             stderr = (exc.stderr or b"").decode("utf-8", errors="replace")[:800]
             raise TaskError(f"patch did not apply cleanly: {stderr}", status_code=422)
 
+        # When a normalizer is configured, the committed tree must satisfy it —
+        # most consequentially transformers' modular/generated-file coupling,
+        # where editing a ``modular_*.py`` must regenerate its ``modeling_*.py``.
+        # The in-loop gate (:func:`_validate_patch`) enforces that, but when its
+        # correction budget is exhausted prepare_task resets the worktree and we
+        # land here with only the raw LLM patch. Re-run the normalizer so any
+        # regenerated files ride along in the commit, and refuse rather than open
+        # a PR the repo's own consistency CI would immediately reject.
+        if cfg.task_normalize_command:
+            returncode, tail = _run_repo_normalizer(cfg, checkout, _emit)
+            if returncode not in (None, 0):
+                clone_cache.reset_worktree(checkout)
+                _emit("log", "Normalizer rejected the patch; not opening a PR.")
+                return TaskResult(
+                    mode=req.mode,
+                    no_change=True,
+                    message=(
+                        "The proposed patch does not pass the repository's "
+                        f"normalizer (exit {returncode}), so no PR was opened — "
+                        "the in-loop correction budget was exhausted before a "
+                        f"clean patch was found:\n\n{tail}"
+                    ),
+                )
+
     clone_cache.stage_all(checkout)
     changes = clone_cache.collect_changes(checkout)
     if not changes:

tests/test_tasks.py

@@ -521,6 +521,130 @@ def test_serge_identity_used_for_loop_cap_consistency(self):
         self.assertTrue(SERGE_GIT_EMAIL)
 
 
+class PublishFallbackNormalizeTests(unittest.TestCase):
+    """When validation's correction budget is exhausted, publish_task lands on
+    the raw-apply fallback. With a normalizer configured it must re-run it so
+    regenerated files (e.g. transformers' modeling_*.py) ride along, and refuse
+    to open a PR the normalizer rejects — never commit a raw, un-normalized
+    patch. Uses the bwrap backend + sandbox off so the command runs directly."""
+
+    _PATCH = (
+        "diff --git a/hello.txt b/hello.txt\n"
+        "--- a/hello.txt\n"
+        "+++ b/hello.txt\n"
+        "@@ -1 +1 @@\n"
+        "-hi from main\n"
+        "+hi patched\n"
+    )
+
+    def setUp(self) -> None:
+        self._tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self._tmp.cleanup)
+        root = self._tmp.name
+        self.src = os.path.join(root, "src")
+        os.makedirs(self.src)
+        _git(self.src, "init", "--quiet", "-b", "main")
+        with open(os.path.join(self.src, "hello.txt"), "w") as f:
+            f.write("hi from main\n")
+        _git(self.src, "add", "-A")
+        _git(self.src, "commit", "--quiet", "-m", "main commit")
+        self.cache = CloneCache(os.path.join(root, "cache"))
+
+    def _checkout(self):
+        return self.cache.acquire_ref(
+            token="",
+            owner="acme",
+            repo="widget",
+            ref="main",
+            job_id="abcd1234",
+            remote_url=self.src,
+        )
+
+    def _req(self):
+        return TaskRequest(
+            owner="acme",
+            repo="widget",
+            base_ref="main",
+            instruction="fix",
+            context="",
+            mode="new_pr",
+        )
+
+    def _cfg(self, **overrides):
+        base = dict(helper_sandbox="off", task_sandbox_backend="bwrap")
+        base.update(overrides)
+        return _make_cfg(**base)
+
+    def test_fallback_reruns_normalizer_and_ships_regenerated_files(self):
+        # A raw patch (worktree_prepared=False) whose normalizer regenerates an
+        # extra file: the commit must include BOTH the patched and generated
+        # files, mirroring transformers' modular_*.py -> modeling_*.py flow.
+        co = self._checkout()
+        cfg = self._cfg(
+            task_normalize_command=["sh", "-c", "echo generated > extra.txt"],
+            task_normalize_timeout=30,
+        )
+        plan = TaskPlan(title="Fix hello", body="desc", patch=self._PATCH)
+        gh = _FakeGH()
+        with patch("reviewbot.tasks.post_task_pr_created_notification"):
+            result = publish_task(
+                cfg,
+                gh,
+                self._req(),
+                plan,
+                checkout=co,
+                clone_cache=self.cache,
+                job_id="abcd1234",
+            )
+        self.assertFalse(result.no_change)
+        self.assertEqual(sorted(result.changed_files), ["extra.txt", "hello.txt"])
+        self.assertIsNotNone(gh.created_pr)
+
+    def test_fallback_refuses_when_normalizer_rejects(self):
+        # Normalizer exits non-zero on the raw patch -> no PR, worktree reset.
+        co = self._checkout()
+        cfg = self._cfg(
+            task_normalize_command=["sh", "-c", "echo boom >&2; exit 3"],
+            task_normalize_timeout=30,
+        )
+        plan = TaskPlan(title="Fix hello", body="desc", patch=self._PATCH)
+        gh = _FakeGH()
+        result = publish_task(
+            cfg,
+            gh,
+            self._req(),
+            plan,
+            checkout=co,
+            clone_cache=self.cache,
+            job_id="abcd1234",
+        )
+        self.assertTrue(result.no_change)
+        self.assertIsNone(gh.created_pr)
+        self.assertIn("normalizer", result.message.lower())
+        self.assertIn("exit 3", result.message)
+        # Worktree restored to the pristine checkout.
+        self.assertEqual(self.cache.collect_changes(co), [])
+
+    def test_no_normalizer_configured_commits_raw_patch(self):
+        # Without a normalizer the fallback still commits the raw patch as-is.
+        co = self._checkout()
+        cfg = self._cfg()  # no task_normalize_command
+        plan = TaskPlan(title="Fix hello", body="desc", patch=self._PATCH)
+        gh = _FakeGH()
+        with patch("reviewbot.tasks.post_task_pr_created_notification"):
+            result = publish_task(
+                cfg,
+                gh,
+                self._req(),
+                plan,
+                checkout=co,
+                clone_cache=self.cache,
+                job_id="abcd1234",
+            )
+        self.assertFalse(result.no_change)
+        self.assertEqual(result.changed_files, ["hello.txt"])
+
+
 class ValidatePatchTests(unittest.TestCase):
     """The in-loop verification gate (_validate_patch), against a real
     worktree. Runs with the bwrap backend + sandbox off so the normalize