From 1c02dcb21709d46b615b8dfff8b0c01711a76390 Mon Sep 17 00:00:00 2001 From: timmy Date: Tue, 25 Aug 2026 08:02:16 +0000 Subject: [PATCH] feat: apply review suggestions across files Closes #1386 --- README.md | 13 ++- frontend/pull-sheet.js | 28 +++-- src/gitea_proxy.py | 103 +++++++++++++++-- src/main.py | 14 ++- ...e_apply_review_suggestion_batch_release.py | 25 ++-- tests/test_pull_api.py | 108 +++++++++++++++--- 6 files changed, 238 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 0bf7265..ae3f807 100644 --- a/README.md +++ b/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 -file can be staged during the mobile feedback pass, reviewed together, and committed as -one race-guarded branch update. The batch preserves the original blob identity, applies -replacements from the bottom up, rejects overlapping or stale lines, and verifies the -advanced pull head and final file before reporting success; suggestions in another file -wait for the next batch so Gitea's single-file contents API cannot produce partial commits. +pulls, up to eight non-overlapping one-line reviewer suggestions across bounded UTF-8 +files can be staged during the mobile feedback pass, reviewed in a path-grouped manifest, +and committed as one atomic branch update. The batch preserves every original blob +identity, applies same-file replacements from the bottom up, rejects overlapping or stale +lines, and verifies the advanced pull head and every final file before reporting success. +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 diff --git a/frontend/pull-sheet.js b/frontend/pull-sheet.js index 270172a..b55604e 100644 --- a/frontend/pull-sheet.js +++ b/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]; - if (stagedPath && stagedPath !== path) { - qs('#pull-feedback-status').textContent = 'Commit the staged file before batching suggestions from another file.'; + const stagedCount = Object.values(active.batch) + .reduce((total, file) => total + file.suggestions.length, 0); + 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:{ - path:entries[0].file.path, content:entries[0].content, - expectedBlobSha:entries[0].file.blob_sha, - }, + files:entries.map(entry => ({ + path:entry.file.path, content:entry.content, + 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:{ - path:draft.file.path, content:draft.file.content, - expected_blob_sha:draft.file.expectedBlobSha, - }, + files:draft.files.map(file => ({ + path:file.path, content:file.content, + 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; diff --git a/src/gitea_proxy.py b/src/gitea_proxy.py index e409c86..c3a7708 100644 --- a/src/gitea_proxy.py +++ b/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.""" - if not 2 <= suggestion_count <= 8: + """Commit reviewed replacements across bounded files as one verified commit.""" + 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, - expected_head_sha, file.get("expected_blob_sha", ""), + + paths = [file.get("path", "") for file in files] + 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( diff --git a/src/main.py b/src/main.py index 37c7b3c..0151c9e 100644 --- a/src/main.py +++ b/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, diff --git a/tests/e2e/test_mobile_apply_review_suggestion_batch_release.py b/tests/e2e/test_mobile_apply_review_suggestion_batch_release.py index c1695d2..09168ae 100644 --- a/tests/e2e/test_mobile_apply_review_suggestion_batch_release.py +++ b/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.", - "suggestion": {"line": 3, "content": " return typed_result()"}}, + {"id": 100, "path": "tests/test_api.py", "line": 2, "body": "Prove the 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', - content:'def lookup():\\n return None\\n return result\\n'}; + if (batchCalls.length === 1) return {path:'src/api.py', head_sha:'abc1234', blob_sha:'blob1', + 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 diff --git a/tests/test_pull_api.py b/tests/test_pull_api.py index 39ee13d..fe34c39 100644 --- a/tests/test_pull_api.py +++ b/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": - updated = request.url.params["ref"] == "def5678" - return httpx.Response(200, json=_content_payload( - "first fixed\nsecond fixed\n" if updated else "first\nsecond\n", - "blob2" if updated else "blob1", - )) - if request.url.path == "/api/v1/repos/stackchain/api/contents/src/api.py" and request.method == "PUT": + prefix = "/api/v1/repos/stackchain/api/contents/" + if request.url.path.startswith(prefix) and request.method == "GET": + path = request.url.path.removeprefix(prefix) + content, sha = (updated if request.url.params["ref"] == "def5678" else originals)[path] + return httpx.Response(200, json=_content_payload(content, sha)) + if request.url.path == "/api/v1/repos/stackchain/api/contents" and request.method == "POST": body = json.loads(request.content) - assert body["branch"] == "alex/review-fix" - assert base64.b64decode(body["content"]).decode() == "first fixed\nsecond fixed\n" + assert body == { + "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["head_sha"] == "def5678" - assert result["path"] == "src/api.py" - assert result["suggestion_count"] == 2 - assert len([request for request in requests if request[0] == "PUT"]) == 1 + assert result == { + "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: 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 = [] -- 2.43.0