Merge pull request 'Apply reviewer suggestions across files atomically' (#1387) from timmy/1386-atomic-multi-file-review-suggestions into main

README.md

@@ -19,12 +19,13 @@ threads, create and self-assign issues, discover, claim, and release issue assig
 list repository labels and open milestones, set or clear due dates on assigned issues, create issue comments, close assigned issues,
 inspect/comment on assigned pull
 requests, merge assigned pull requests, and submit pull-request reviews. For authored
-pulls, several non-overlapping one-line reviewer suggestions in the same bounded UTF-8
+pulls, up to eight non-overlapping one-line reviewer suggestions across bounded UTF-8
-file can be staged during the mobile feedback pass, reviewed together, and committed as
+files can be staged during the mobile feedback pass, reviewed in a path-grouped manifest,
-one race-guarded branch update. The batch preserves the original blob identity, applies
+and committed as one atomic branch update. The batch preserves every original blob
-replacements from the bottom up, rejects overlapping or stale lines, and verifies the
+identity, applies same-file replacements from the bottom up, rejects overlapping or stale
-advanced pull head and final file before reporting success; suggestions in another file
+lines, and verifies the advanced pull head and every final file before reporting success.
-wait for the next batch so Gitea's single-file contents API cannot produce partial commits.
+The server uses Gitea's multi-file contents mutation so a cross-file implementation-and-test
+fix cannot be partially committed.
 After an exact merged commit fails release checks, its mobile release receipt can load the
 failed job evidence and prepare a draft rollback pull request. The server revalidates the
 operator's participation and push access, reverses only bounded UTF-8 files whose current

frontend/pull-sheet.js

@@ -575,9 +575,10 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
     if (!active?.suggestion) return;
     const pending = active.suggestion;
     const path = pending.file.path;
-    const stagedPath = Object.keys(active.batch)[0];
+    const stagedCount = Object.values(active.batch)
-    if (stagedPath && stagedPath !== path) {
+      .reduce((total, file) => total + file.suggestions.length, 0);
-      qs('#pull-feedback-status').textContent = 'Commit the staged file before batching suggestions from another file.';
+    if (stagedCount >= 8) {
+      qs('#pull-feedback-status').textContent = 'Commit this bounded batch before staging more suggestions.';
       return;
     }
     const entry = active.batch[path] || {file:pending.file, suggestions:[]};
@@ -633,10 +634,10 @@ function bindFeedbackControls(doc, controller, getSelected, getDetail, getLogin)
     qs('#pull-feedback-status').textContent = 'Committing and verifying all suggested changes…';
     try {
       const result = await controller.commitFeedbackBatch(active.item, {
-        file:{
+        files:entries.map(entry => ({
-          path:entries[0].file.path, content:entries[0].content,
+          path:entry.file.path, content:entry.content,
-          expectedBlobSha:entries[0].file.blob_sha,
+          expectedBlobSha:entry.file.blob_sha,
-        },
+        })),
         suggestionCount:count, message, expectedHeadSha:active.detail.head_sha,
       });
       active.detail.head_sha = result.head_sha;
@@ -1064,16 +1065,19 @@ function createPullSheet({ fetchJson, storage, onState, createConversationPager
         method:'POST',
         headers:{ Accept:'application/json', 'Content-Type':'application/json' },
         body:JSON.stringify({
-          file:{
+          files:draft.files.map(file => ({
-            path:draft.file.path, content:draft.file.content,
+            path:file.path, content:file.content,
-            expected_blob_sha:draft.file.expectedBlobSha,
+            expected_blob_sha:file.expectedBlobSha,
-          },
+          })),
           suggestion_count:draft.suggestionCount,
           message:draft.message, expected_head_sha:draft.expectedHeadSha,
         }),
       }).then(result => {
+        const expectedPaths = draft.files.map(file => file.path);
         if (!result?.head_sha || result.head_sha === draft.expectedHeadSha ||
-            result.path !== draft.file.path || result.suggestion_count !== draft.suggestionCount) {
+            !Array.isArray(result.paths) || result.paths.length !== expectedPaths.length ||
+            result.paths.some((path, index) => path !== expectedPaths[index]) ||
+            result.suggestion_count !== draft.suggestionCount) {
           throw new Error('The suggestion batch was not confirmed.');
         }
         return result;

src/gitea_proxy.py

@@ -2422,19 +2422,108 @@ async def commit_authored_pull_feedback_fix(
 async def commit_authored_pull_feedback_batch(
     repository: str,
     number: int,
-    file: dict,
+    files: list[dict],
     suggestion_count: int,
     message: str,
     expected_head_sha: str,
 ) -> dict:
-    """Commit multiple reviewed replacements in one file as one verified commit."""
+    """Commit reviewed replacements across bounded files as one verified commit."""
-    if not 2 <= suggestion_count <= 8:
+    message = message.strip()
+    if (
+        not 2 <= suggestion_count <= 8
+        or not 1 <= len(files) <= 8
+        or len(files) > suggestion_count
+        or not message
+        or len(message) > 120
+    ):
         raise IssueNotAvailableError("Feedback batch is outside the editable bounds")
-    result = await commit_authored_pull_feedback_fix(
+
-        repository, number, file.get("path", ""), file.get("content", ""), message,
+    paths = [file.get("path", "") for file in files]
-        expected_head_sha, file.get("expected_blob_sha", ""),
+    if len(set(paths)) != len(paths):
+        raise IssueNotAvailableError("Feedback batch contains duplicate files")
+
+    login, pull = await _current_login_and_target(
+        f"repos/{repository}/pulls/{number}"
     )
-    return {**result, "suggestion_count": suggestion_count}
+    author = pull.get("user") if isinstance(pull.get("user"), dict) else {}
+    head = pull.get("head") if isinstance(pull.get("head"), dict) else {}
+    base = pull.get("base") if isinstance(pull.get("base"), dict) else {}
+    head_repo = head.get("repo") if isinstance(head.get("repo"), dict) else {}
+    base_repo = base.get("repo") if isinstance(base.get("repo"), dict) else {}
+    branch = head.get("ref")
+    if (
+        pull.get("state") != "open"
+        or pull.get("merged") is True
+        or author.get("login", "").casefold() != login.casefold()
+        or head.get("sha") != expected_head_sha
+        or head_repo.get("full_name") != repository
+        or base_repo.get("full_name") != repository
+        or not isinstance(branch, str)
+        or not branch
+    ):
+        raise IssueNotAvailableError("Pull request state changed")
+
+    operations = []
+    submitted: dict[str, str] = {}
+    total_bytes = 0
+    for file in files:
+        path = file.get("path", "")
+        content = file.get("content", "")
+        expected_blob_sha = file.get("expected_blob_sha", "")
+        if not isinstance(content, str):
+            raise IssueNotAvailableError("Feedback batch is outside the editable bounds")
+        encoded = content.encode("utf-8")
+        total_bytes += len(encoded)
+        if len(encoded) > 128 * 1024 or total_bytes > 512 * 1024 or "\x00" in content:
+            raise IssueNotAvailableError("Feedback batch is outside the editable bounds")
+        current = await authored_pull_feedback_file(
+            repository, number, path, expected_head_sha
+        )
+        if current["blob_sha"] != expected_blob_sha or current["content"] == content:
+            raise IssueNotAvailableError("File changed or has no new content")
+        operations.append({
+            "operation": "update",
+            "path": path,
+            "sha": expected_blob_sha,
+            "content": base64.b64encode(encoded).decode(),
+        })
+        submitted[path] = content
+
+    response = await _get_client().post(
+        f"/api/v1/repos/{repository}/contents",
+        headers=_auth(),
+        json={"branch": branch, "message": message, "files": operations},
+    )
+    if response.status_code in {409, 422}:
+        raise IssueNotAvailableError("Pull request or file changed")
+    response.raise_for_status()
+    payload = response.json()
+    commit = payload.get("commit") if isinstance(payload, dict) else None
+    mutation_sha = commit.get("sha") if isinstance(commit, dict) else None
+    if not isinstance(mutation_sha, str) or not mutation_sha:
+        raise ValueError("Gitea did not confirm the feedback batch commit")
+
+    confirmed_pull = await fetch(f"repos/{repository}/pulls/{number}")
+    confirmed_head = confirmed_pull.get("head") if isinstance(confirmed_pull, dict) and isinstance(confirmed_pull.get("head"), dict) else {}
+    new_head_sha = confirmed_head.get("sha")
+    if new_head_sha != mutation_sha or new_head_sha == expected_head_sha:
+        raise ValueError("Gitea did not confirm a new pull request head")
+    for path, content in submitted.items():
+        confirmed = await authored_pull_feedback_file(
+            repository, number, path, new_head_sha
+        )
+        if confirmed["content"] != content:
+            raise ValueError("Gitea did not confirm every committed file")
+
+    return {
+        "repository": repository,
+        "number": number,
+        "paths": paths,
+        "previous_head_sha": expected_head_sha,
+        "head_sha": new_head_sha,
+        "suggestion_count": suggestion_count,
+        "message": message,
+    }
 
 
 async def update_authored_pull_branch(

src/main.py

@@ -1173,13 +1173,22 @@ class PullFeedbackBatchFile(BaseModel):
 
 
 class PullFeedbackBatchUpdate(BaseModel):
-    file: PullFeedbackBatchFile
+    files: list[PullFeedbackBatchFile] = Field(min_length=1, max_length=8)
     suggestion_count: int = Field(ge=2, le=8)
     message: str = Field(min_length=1, max_length=120)
     expected_head_sha: str = Field(
         min_length=7, max_length=64, pattern=r"^[A-Fa-f0-9]+$"
     )
 
+    @model_validator(mode="after")
+    def validate_file_manifest(self):
+        paths = [file.path for file in self.files]
+        if len(set(paths)) != len(paths):
+            raise ValueError("feedback batch file paths must be unique")
+        if len(paths) > self.suggestion_count:
+            raise ValueError("feedback batch cannot contain more files than suggestions")
+        return self
+
 
 class PullContentUpdate(BaseModel):
     title: str = Field(min_length=1, max_length=255)
@@ -7184,7 +7193,8 @@ async def commit_authored_pull_feedback_batch(
     try:
         result = await asyncio.wait_for(
             gitea_proxy.commit_authored_pull_feedback_batch(
-                f"{owner}/{repo}", number, update.file.model_dump(),
+                f"{owner}/{repo}", number,
+                [file.model_dump() for file in update.files],
                 update.suggestion_count, update.message, update.expected_head_sha,
             ),
             timeout=ISSUE_ACTION_TIMEOUT_SECONDS,

tests/e2e/test_mobile_apply_review_suggestion_batch_release.py

@@ -7,7 +7,7 @@ ROOT = Path(__file__).parents[2]
 
 
 @pytest.mark.parametrize("viewport", [(320, 568), (390, 844)])
-def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(viewport):
+def test_mobile_author_stages_and_commits_cross_file_suggestions_atomically(viewport):
     playwright = pytest.importorskip("playwright.sync_api")
     html = (ROOT / "frontend" / "index.html").read_text()
     detail = {
@@ -19,14 +19,15 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
             "comments": [
                 {"id": 99, "path": "src/api.py", "line": 2, "body": "Use the typed empty result.",
                  "suggestion": {"line": 2, "content": "    return empty_result()"}},
-                {"id": 100, "path": "src/api.py", "line": 3, "body": "Keep the result typed.",
+                {"id": 100, "path": "tests/test_api.py", "line": 2, "body": "Prove the typed result.",
-                 "suggestion": {"line": 3, "content": "    return typed_result()"}},
+                 "suggestion": {"line": 2, "content": "    assert lookup() == empty_result()"}},
             ],
         }],
         "files": [
             {"filename": "src/api.py", "status": "modified", "diff_available": True,
              "diff_lines": ["@@ -2 +2 @@", "+    return None"]},
-
+            {"filename": "tests/test_api.py", "status": "modified", "diff_available": True,
+             "diff_lines": ["@@ -2 +2 @@", "+    assert lookup() is None"]},
         ],
     }
 
@@ -48,10 +49,13 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
                 globalThis.batchController = createPullSheet({fetchJson: async (path, options = {}) => {
                     batchCalls.push({path, options});
                     if ((options.method || 'GET') === 'GET') {
-                        return {path:'src/api.py', head_sha:'abc1234', blob_sha:'blob1',
+                        if (batchCalls.length === 1) return {path:'src/api.py', head_sha:'abc1234', blob_sha:'blob1',
-                            content:'def lookup():\\n    return None\\n    return result\\n'};
+                            content:'def lookup():\\n    return None\\n'};
+                        return {path:'tests/test_api.py', head_sha:'abc1234', blob_sha:'blob2',
+                            content:'def test_lookup():\\n    assert lookup() is None\\n'};
                     }
-                    return {path:'src/api.py', suggestion_count:2, previous_head_sha:'abc1234', head_sha:'def5678'};
+                    return {paths:['src/api.py','tests/test_api.py'], suggestion_count:2,
+                        previous_head_sha:'abc1234', head_sha:'def5678'};
                 }});
                 document.querySelector('#pull-sheet').classList.add('open');
                 document.querySelector('#pull-review').open = true;
@@ -78,7 +82,7 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
         assert page.locator("#review-pull-feedback-batch").text_content() == "Review fixes (2)"
         page.locator("#review-pull-feedback-batch").click()
         assert page.locator("#pull-feedback-batch-review").is_visible()
-        assert page.locator("#pull-feedback-batch-items li").count() == 1
+        assert page.locator("#pull-feedback-batch-items li").count() == 2
         page.locator("#commit-pull-feedback-batch").click()
         page.wait_for_function("batchCalls.length === 3")
         result = page.evaluate("""() => ({
@@ -102,7 +106,8 @@ def test_mobile_author_stages_and_commits_two_same_file_suggestions_atomically(v
     assert request["options"]["method"] == "POST"
     body = request["options"]["body"]
     assert '"expected_head_sha":"abc1234"' in body
-    assert '"file":{"path":"src/api.py"' in body
+    assert '"files":[{"path":"src/api.py"' in body
+    assert '"path":"tests/test_api.py"' in body
     assert '"suggestion_count":2' in body
     assert 'return empty_result()' in body
-    assert 'return typed_result()' in body
+    assert 'assert lookup() == empty_result()' in body

tests/test_pull_api.py

@@ -109,7 +109,7 @@ async def test_gitea_commits_feedback_fix_and_verifies_advanced_pull_head():
 
 
 @pytest.mark.anyio
-async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
+async def test_gitea_commits_cross_file_suggestions_as_one_atomic_commit():
     requests = []
     pull = {
         "number": 7, "state": "open", "merged": False,
@@ -117,6 +117,8 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
         "head": {"sha": "abc1234", "ref": "alex/review-fix", "repo": {"full_name": "stackchain/api"}},
         "base": {"repo": {"full_name": "stackchain/api"}},
     }
+    originals = {"src/api.py": ("return empty\n", "blob1"), "tests/test_api.py": ("assert empty\n", "blob2")}
+    updated = {"src/api.py": ("return handled\n", "blob3"), "tests/test_api.py": ("assert handled\n", "blob4")}
 
     async def handler(request):
         requests.append((request.method, request.url.path))
@@ -124,16 +126,23 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
             return httpx.Response(200, json={"login": "alex"})
         if request.url.path == "/api/v1/repos/stackchain/api/pulls/7":
             return httpx.Response(200, json=pull)
-        if request.url.path == "/api/v1/repos/stackchain/api/contents/src/api.py" and request.method == "GET":
+        prefix = "/api/v1/repos/stackchain/api/contents/"
-            updated = request.url.params["ref"] == "def5678"
+        if request.url.path.startswith(prefix) and request.method == "GET":
-            return httpx.Response(200, json=_content_payload(
+            path = request.url.path.removeprefix(prefix)
-                "first fixed\nsecond fixed\n" if updated else "first\nsecond\n",
+            content, sha = (updated if request.url.params["ref"] == "def5678" else originals)[path]
-                "blob2" if updated else "blob1",
+            return httpx.Response(200, json=_content_payload(content, sha))
-            ))
+        if request.url.path == "/api/v1/repos/stackchain/api/contents" and request.method == "POST":
-        if request.url.path == "/api/v1/repos/stackchain/api/contents/src/api.py" and request.method == "PUT":
             body = json.loads(request.content)
-            assert body["branch"] == "alex/review-fix"
+            assert body == {
-            assert base64.b64decode(body["content"]).decode() == "first fixed\nsecond fixed\n"
+                "branch": "alex/review-fix",
+                "message": "fix: apply review suggestions",
+                "files": [
+                    {"operation": "update", "path": "src/api.py", "sha": "blob1",
+                     "content": base64.b64encode(b"return handled\n").decode()},
+                    {"operation": "update", "path": "tests/test_api.py", "sha": "blob2",
+                     "content": base64.b64encode(b"assert handled\n").decode()},
+                ],
+            }
             pull["head"]["sha"] = "def5678"
             return httpx.Response(200, json={"commit": {"sha": "def5678"}})
         raise AssertionError(f"unexpected request: {request.method} {request.url}")
@@ -142,17 +151,23 @@ async def test_gitea_commits_multiple_same_file_suggestions_as_one_commit():
     try:
         result = await gitea_proxy.commit_authored_pull_feedback_batch(
             "stackchain/api", 7,
-            {"path": "src/api.py", "content": "first fixed\nsecond fixed\n", "expected_blob_sha": "blob1"},
+            [
+                {"path": "src/api.py", "content": "return handled\n", "expected_blob_sha": "blob1"},
+                {"path": "tests/test_api.py", "content": "assert handled\n", "expected_blob_sha": "blob2"},
+            ],
             2, "fix: apply review suggestions", "abc1234",
         )
     finally:
         await gitea_proxy.stop_client()
 
-    assert result["previous_head_sha"] == "abc1234"
+    assert result == {
-    assert result["head_sha"] == "def5678"
+        "repository": "stackchain/api", "number": 7,
-    assert result["path"] == "src/api.py"
+        "paths": ["src/api.py", "tests/test_api.py"],
-    assert result["suggestion_count"] == 2
+        "previous_head_sha": "abc1234", "head_sha": "def5678",
-    assert len([request for request in requests if request[0] == "PUT"]) == 1
+        "suggestion_count": 2, "message": "fix: apply review suggestions",
+    }
+    assert len([request for request in requests if request == ("POST", "/api/v1/repos/stackchain/api/contents")]) == 1
+    assert not [request for request in requests if request[0] == "PUT"]
 
 
 @pytest.mark.anyio
@@ -176,6 +191,67 @@ async def test_feedback_batch_api_requires_at_least_two_suggestions(monkeypatch)
     assert called is False
 
 
+@pytest.mark.anyio
+async def test_feedback_batch_api_accepts_unique_cross_file_manifest(monkeypatch):
+    calls = []
+
+    async def commit(*args):
+        calls.append(args)
+        return {
+            "repository": "stackchain/api", "number": 7,
+            "paths": ["src/api.py", "tests/test_api.py"],
+            "previous_head_sha": "abc1234", "head_sha": "def5678",
+            "suggestion_count": 2, "message": "fix: suggestions",
+        }
+
+    monkeypatch.setattr(main.gitea_proxy, "commit_authored_pull_feedback_batch", commit, raising=False)
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        response = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/feedback-batch",
+            json={
+                "message": "fix: suggestions", "expected_head_sha": "abc1234", "suggestion_count": 2,
+                "files": [
+                    {"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
+                    {"path": "tests/test_api.py", "content": "assert after\n", "expected_blob_sha": "blob2"},
+                ],
+            },
+        )
+
+    assert response.status_code == 200
+    assert response.json()["paths"] == ["src/api.py", "tests/test_api.py"]
+    assert calls[0][2] == [
+        {"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
+        {"path": "tests/test_api.py", "content": "assert after\n", "expected_blob_sha": "blob2"},
+    ]
+
+
+@pytest.mark.anyio
+async def test_feedback_batch_api_rejects_duplicate_file_paths(monkeypatch):
+    called = False
+
+    async def commit(*args):
+        nonlocal called
+        called = True
+
+    monkeypatch.setattr(main.gitea_proxy, "commit_authored_pull_feedback_batch", commit, raising=False)
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        response = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/feedback-batch",
+            json={
+                "message": "fix: suggestions", "expected_head_sha": "abc1234", "suggestion_count": 2,
+                "files": [
+                    {"path": "src/api.py", "content": "after\n", "expected_blob_sha": "blob1"},
+                    {"path": "src/api.py", "content": "other\n", "expected_blob_sha": "blob2"},
+                ],
+            },
+        )
+
+    assert response.status_code == 422
+    assert called is False
+
+
 @pytest.mark.anyio
 async def test_feedback_file_api_loads_and_commits_an_authored_pull_fix(monkeypatch):
     calls = []