diff --git a/backend/app/routes/github.py b/backend/app/routes/github.py index dff1a2aea..142f57001 100644 --- a/backend/app/routes/github.py +++ b/backend/app/routes/github.py @@ -1032,15 +1032,165 @@ def _parse_pr_number(url: str) -> int | None: return int(m.group(1)) if m else None +def _reviewed_pr_labels(plan: dict) -> list[str]: + """Return only the two labels the owner could see in Contribute review.""" + raw = plan.get("labels") + if not isinstance(raw, list): + return [] + # Mirror Contribute's review surface: it filters malformed/blank values, + # trims them, and then shows at most two. Security validation and duplicate + # folding happen only after that visibility boundary, so an unseen third + # label can never replace a visible-but-unusable one at submit time. + visible = [] + for value in raw: + if not isinstance(value, str): + continue + label = value.strip() + if not label: + continue + visible.append(label) + if len(visible) == 2: + break + labels = [] + seen = set() + for label in visible: + folded = label.casefold() + if len(label) > 50 or "\n" in label or folded in seen: + continue + seen.add(folded) + labels.append(label) + return labels + + +def _apply_reviewed_pr_labels( + repo: Path, + upstream_repo: str, + number: int | None, + labels: list[str], +) -> dict: + """Best-effort add reviewed labels that already exist in the target repo. + + Labeling is deliberately secondary to PR creation: a missing repository + label, permission restriction, or transient API failure must not turn an + already-open pull request into an apparent failed submission. The outcome is + persisted so the review never claims an unavailable label was applied. + """ + if not labels: + return {} + patch = { + "last_submit_labels_requested": labels, + "last_submit_labels_applied": [], + } + if number is None: + return { + **patch, + "last_submit_labels_note": "GitHub did not return a PR number for labeling.", + } + + try: + available = _gh( + repo, + "api", "--paginate", + f"repos/{upstream_repo}/labels?per_page=100", + "--jq", ".[].name", + check=False, + ) + except subprocess.TimeoutExpired: + return { + **patch, + "last_submit_labels_note": ( + "Timed out while checking repository labels; the pull request is " + "open without confirmed labels." + ), + } + except OSError: + return { + **patch, + "last_submit_labels_note": ( + "Could not start the GitHub label lookup; the pull request is open " + "without confirmed labels." + ), + } + if available.returncode != 0: + return { + **patch, + "last_submit_labels_note": ( + "Could not verify the repository labels; the pull request is open " + "without confirmed labels." + ), + } + by_name = {} + for raw_name in (available.stdout or "").splitlines(): + name = raw_name.strip() + if name: + by_name[name.casefold()] = name + applicable = [by_name[label.casefold()] for label in labels + if label.casefold() in by_name] + missing = [label for label in labels if label.casefold() not in by_name] + if not applicable: + return { + **patch, + "last_submit_labels_missing": missing, + "last_submit_labels_note": "The reviewed labels do not exist in this repository.", + } + + try: + applied = _gh( + repo, + "api", "--method", "POST", + f"repos/{upstream_repo}/issues/{number}/labels", + *(part for label in applicable for part in ("-f", f"labels[]={label}")), + check=False, + ) + except subprocess.TimeoutExpired: + return { + **patch, + "last_submit_labels_missing": missing, + "last_submit_labels_note": ( + "Timed out while applying reviewed labels; the pull request is open, " + "but GitHub did not confirm the label result." + ), + } + except OSError: + return { + **patch, + "last_submit_labels_missing": missing, + "last_submit_labels_note": ( + "Could not start the GitHub label update; the pull request is open " + "without confirmed labels." + ), + } + if applied.returncode != 0: + return { + **patch, + "last_submit_labels_missing": missing, + "last_submit_labels_note": ( + "GitHub did not confirm these labels were applied; the pull request " + "is still open." + ), + } + result = { + **patch, + "last_submit_labels_applied": applicable, + } + if missing: + result["last_submit_labels_missing"] = missing + result["last_submit_labels_note"] = "Some reviewed labels no longer exist." + return result + + def _find_existing_pr( repo: Path, upstream_repo: str, login: str, branch: str, *, + expected_head_sha: str, base_branch: str | None = None, same_repo: bool = False, ) -> str | None: + if not _GIT_SHA.match(str(expected_head_sha or "")): + return None head = branch if same_repo else f"{login}:{branch}" args = [ "pr", "list", @@ -1049,22 +1199,32 @@ def _find_existing_pr( ] if base_branch: args.extend(("--base", _validate_branch(base_branch))) - args.extend(("--state", "open", "--json", "url", "--limit", "1")) - proc = _gh( - repo, - *args, - check=False, - ) + args.extend(( + "--state", "open", "--json", "url,headRefOid", "--limit", "10", + )) + try: + proc = _gh( + repo, + *args, + check=False, + ) + except (subprocess.TimeoutExpired, OSError): + return None if proc.returncode != 0: return None try: rows = json.loads(proc.stdout or "[]") except ValueError: return None - if isinstance(rows, list) and rows: - url = rows[0].get("url") if isinstance(rows[0], dict) else None - if isinstance(url, str) and url.startswith("https://github.com/"): - return url + if isinstance(rows, list): + for row in rows: + if not isinstance(row, dict): + continue + if str(row.get("headRefOid") or "") != expected_head_sha: + continue + url = row.get("url") + if isinstance(url, str) and url.startswith("https://github.com/"): + return url return None @@ -1621,6 +1781,14 @@ def _submit_prepared_pr( record_patch = _record_patch_with(record_patch, merge_patch) except ContributionSubmitError as exc: raise _merge_error_patch(exc, record_patch) from exc + # The merge preflight proves one exact upstream base. Pin that same branch + # into both create and ambiguous-response recovery. Without an explicit + # standalone --base, gh may honor stale branch..gh-merge-base config + # from the durable staging checkout and publish the reviewed diff against a + # different target. + submit_base = direct_base or _validate_branch( + str(merge_patch.get("last_submit_upstream_branch") or "") + ) push_source = "HEAD" if direct_base: @@ -1674,6 +1842,20 @@ def _submit_prepared_pr( ), "last_pushed_branch_url": pushed_branch_url, } + pushed_sha = str( + pushed_patch.get("last_submit_push_sha") + or pushed_patch.get("head_sha") + or plan.get("head_sha") + or "" + ).strip() + if not _GIT_SHA.match(pushed_sha): + pushed_sha = _git(repo, "rev-parse", push_source).stdout.strip() + if not _GIT_SHA.match(pushed_sha): + raise ContributionSubmitError( + "Could not verify the exact reviewed commit after pushing this branch.", + record_patch=pushed_patch, + ) + pushed_patch["last_submit_push_sha"] = pushed_sha with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False) as f: f.write(body) @@ -1687,23 +1869,51 @@ def _submit_prepared_pr( "--title", title, "--body-file", body_file, ] - if direct_base: - create_args.extend(("--base", direct_base)) - pr = _gh(repo, *create_args, check=False) - if pr.returncode != 0: + create_args.extend(("--base", submit_base)) + create_transport_error = None + try: + pr = _gh(repo, *create_args, check=False) + except subprocess.TimeoutExpired: + pr = None + create_transport_error = ( + "Timed out while waiting for GitHub to confirm pull request creation." + ) + except OSError: + pr = None + create_transport_error = ( + "Could not start the GitHub pull request creation command." + ) + if pr is None or pr.returncode != 0: # Retried sends commonly arrive after GitHub already created the PR. - # Pay for the list lookup only on this uncommon recovery path. + # A create transport failure is also ambiguous: GitHub may have + # accepted the request before the local process lost its response. + # Probe the reviewed branch and require its exact pushed commit before + # treating the PR as open. Never issue a second create in this call. existing = _find_existing_pr( repo, upstream_repo, login, branch, - base_branch=direct_base, + expected_head_sha=pushed_sha, + base_branch=submit_base, same_repo=bool(direct_base), ) if existing: - return existing, _parse_pr_number(existing), pushed_patch - detail = (pr.stderr or pr.stdout or "GitHub command failed.").strip() + existing_number = _parse_pr_number(existing) + label_patch = _apply_reviewed_pr_labels( + repo, + upstream_repo, + existing_number, + _reviewed_pr_labels(plan), + ) + return ( + existing, + existing_number, + _record_patch_with(pushed_patch, label_patch), + ) + detail = create_transport_error or ( + pr.stderr or pr.stdout or "GitHub command failed." + ).strip() raise ContributionSubmitError(detail[:600] or "GitHub command failed.") except ContributionSubmitError as exc: raise ContributionSubmitError( @@ -1723,7 +1933,14 @@ def _submit_prepared_pr( f"to {pushed_branch_url}.", record_patch=pushed_patch, ) - return url, _parse_pr_number(url), pushed_patch + number = _parse_pr_number(url) + label_patch = _apply_reviewed_pr_labels( + repo, + upstream_repo, + number, + _reviewed_pr_labels(plan), + ) + return url, number, _record_patch_with(pushed_patch, label_patch) finally: if checkout_back: _git(repo, "checkout", "-q", checkout_back, check=False) diff --git a/backend/tests/test_github_routes.py b/backend/tests/test_github_routes.py index 293aee143..135e4c074 100644 --- a/backend/tests/test_github_routes.py +++ b/backend/tests/test_github_routes.py @@ -748,6 +748,97 @@ def test_graphql_mutation_as_string_literal_allowed(client, auth, monkeypatch): # --- contribution submit (approval button path) ----------------------- +def test_reviewed_pr_labels_are_bounded_to_the_visible_two(): + assert github_routes._reviewed_pr_labels({ + "labels": [" bug ", "area: ui", "hidden-third"], + }) == ["bug", "area: ui"] + assert github_routes._reviewed_pr_labels({ + "labels": ["bug", "BUG", "area: ui"], + }) == ["bug"] + assert github_routes._reviewed_pr_labels({ + "labels": [None, "", "bug", "area: ui", "hidden-third"], + }) == ["bug", "area: ui"] + assert github_routes._reviewed_pr_labels({"labels": "bug"}) == [] + + +def test_pr_labels_apply_only_existing_names_and_preserve_missing( + monkeypatch, tmp_path, +): + calls = [] + + def fake_gh(repo, *args, check=True): + calls.append(args) + if "--paginate" in args: + return _cp("bug\narea: ui\n") + return _cp("[]") + + monkeypatch.setattr(github_routes, "_gh", fake_gh) + patch = github_routes._apply_reviewed_pr_labels( + tmp_path, + "mobius-os/mobius", + 123, + ["Bug", "area: backend"], + ) + + assert patch["last_submit_labels_requested"] == ["Bug", "area: backend"] + assert patch["last_submit_labels_applied"] == ["bug"] + assert patch["last_submit_labels_missing"] == ["area: backend"] + assert "Some reviewed labels" in patch["last_submit_labels_note"] + apply_call = calls[-1] + assert apply_call[:3] == ("api", "--method", "POST") + assert "labels[]=bug" in apply_call + assert "labels[]=area: backend" not in apply_call + + +def test_pr_label_permission_failure_does_not_fail_an_open_pr( + monkeypatch, tmp_path, +): + def fake_gh(repo, *args, check=True): + if "--paginate" in args: + return _cp("bug\n") + return _cp("forbidden", returncode=1) + + monkeypatch.setattr(github_routes, "_gh", fake_gh) + patch = github_routes._apply_reviewed_pr_labels( + tmp_path, + "someone/example", + 7, + ["bug"], + ) + + assert patch["last_submit_labels_applied"] == [] + assert "did not confirm" in patch["last_submit_labels_note"] + + +@pytest.mark.parametrize( + "label_failure", + [ + subprocess.TimeoutExpired(["gh", "api"], timeout=30), + OSError("gh could not start"), + ], + ids=["apply-timeout", "apply-launch-error"], +) +def test_pr_label_apply_transport_failure_is_nonfatal( + monkeypatch, tmp_path, label_failure, +): + def fake_gh(repo, *args, check=True): + if "--paginate" in args: + return _cp("bug\n") + raise label_failure + + monkeypatch.setattr(github_routes, "_gh", fake_gh) + patch = github_routes._apply_reviewed_pr_labels( + tmp_path, + "someone/example", + 7, + ["bug"], + ) + + assert patch["last_submit_labels_requested"] == ["bug"] + assert patch["last_submit_labels_applied"] == [] + assert "pull request is open" in patch["last_submit_labels_note"] + + def _write_contribution(app_id, record_id, record, diff_text=""): base = Path(get_settings().data_dir) / "apps" / str(app_id) / "contributions" base.mkdir(parents=True, exist_ok=True) @@ -1708,12 +1799,21 @@ def _commit_metadata( ) -def test_submit_contribution_creates_review_ready_pr_from_prepared_record( - client, owner_token, monkeypatch, +@pytest.mark.parametrize( + "failure_kind", + ["timeout", "launch-error"], +) +def test_submit_contribution_keeps_accepted_pr_open_on_label_transport_failure( + client, owner_token, monkeypatch, failure_kind, ): + label_failure = ( + subprocess.TimeoutExpired(["gh", "api"], timeout=30) + if failure_kind == "timeout" + else OSError("gh could not start") + ) _write_token(login="octocat") app_id, app_token = _app_token(client, owner_token, github_access=True) - record_id = "rec-pr-1" + record_id = f"rec-pr-label-{failure_kind}" repo = Path(get_settings().data_dir) / "contributions" / record_id / "repo" (repo / ".git").mkdir(parents=True) diff_text = "diff --git a/index.jsx b/index.jsx\n+hello\n" @@ -1738,6 +1838,7 @@ def test_submit_contribution_creates_review_ready_pr_from_prepared_record( "base_sha": base, "head_sha": head, "diff_sha256": hashlib.sha256(diff_text.encode()).hexdigest(), + "labels": ["bug"], }, } _write_contribution(app_id, record_id, record, diff_text) @@ -1803,6 +1904,8 @@ def fake_gh(repo_path, *args, check=True): return _cp("[]") if args[:2] == ("pr", "create"): return _cp("https://github.com/mobius-os/app-demo/pull/42\n") + if args[:2] == ("api", "--paginate"): + raise label_failure return _cp("") monkeypatch.setattr("app.routes.github._git", fake_git) @@ -1818,11 +1921,15 @@ def fake_gh(repo_path, *args, check=True): assert body["number"] == 42 assert body["record"]["status"] == "open" assert body["record"]["url"] == body["url"] + assert body["record"]["last_submit_labels_requested"] == ["bug"] + assert body["record"]["last_submit_labels_applied"] == [] + assert "pull request is open" in body["record"]["last_submit_labels_note"] assert ("repo", "fork", "--remote", "--remote-name", "fork") in gh_calls assert not any(call[:2] == ("remote", "set-url") for call in git_calls) create_call = next(call for call in gh_calls if call[:2] == ("pr", "create")) assert "--draft" not in create_call assert "octocat:fix/demo-polish" in create_call + assert create_call[-2:] == ("--base", "main") assert ("push", "fork", "HEAD:refs/heads/fix/demo-polish") in git_calls assert sum(call[:1] == ("fetch",) for call in git_calls) == 1 assert not any(call[:2] == ("pr", "list") for call in gh_calls) @@ -1837,6 +1944,154 @@ def fake_gh(repo_path, *args, check=True): assert stored["status"] == "open" assert stored["number"] == 42 assert stored["head_repository"] == "octocat/app-demo-1" + assert stored["last_submit_labels_requested"] == ["bug"] + assert stored["last_submit_labels_applied"] == [] + assert stored["last_submit_labels_note"] == body["record"]["last_submit_labels_note"] + + +@pytest.mark.parametrize( + ("failure_kind", "existing_mode"), + [ + ("timeout", "match"), + ("launch-error", "match"), + ("timeout", "absent"), + ("launch-error", "absent"), + ("timeout", "wrong-head"), + ], +) +def test_submit_contribution_recovers_ambiguous_create_by_exact_pushed_head( + client, owner_token, monkeypatch, failure_kind, existing_mode, +): + """A lost create response probes once and never creates a second PR.""" + create_failure = ( + subprocess.TimeoutExpired(["gh", "pr", "create"], timeout=30) + if failure_kind == "timeout" + else OSError("gh could not start") + ) + _write_token(login="octocat") + app_id, app_token = _app_token(client, owner_token, github_access=True) + record_id = f"rec-pr-create-{failure_kind}-{existing_mode}" + repo = Path(get_settings().data_dir) / "contributions" / record_id / "repo" + (repo / ".git").mkdir(parents=True) + diff_text = "diff --git a/index.jsx b/index.jsx\n+hello\n" + base = "b" * 40 + head = "a" * 40 + record = { + "id": record_id, + "type": "pr", + "repo": "mobius-os/app-demo", + "status": "prepared", + "title": "Polish demo", + "branch": "fix/demo-polish", + "created_at": "2026-07-09T00:00:00Z", + "updated_at": "2026-07-09T00:00:00Z", + "plan": { + "action": "pr", + "repo": "mobius-os/app-demo", + "title": "Polish demo", + "body_draft": "## What\n\nPolishes the demo.", + "branch": "fix/demo-polish", + "repo_path": str(repo), + "base_sha": base, + "head_sha": head, + "diff_sha256": hashlib.sha256(diff_text.encode()).hexdigest(), + "labels": ["bug"], + }, + } + _write_contribution(app_id, record_id, record, diff_text) + + monkeypatch.setattr("app.routes.github.shutil.which", lambda name: f"/bin/{name}") + monkeypatch.setattr( + "app.routes.github._assert_fresh", + lambda *_args, **_kwargs: (base, head, record["plan"]["diff_sha256"]), + ) + monkeypatch.setattr("app.routes.github._assert_coauthor_trailer", lambda *_args: None) + monkeypatch.setattr("app.routes.github._assert_clean_worktree", lambda *_args: None) + monkeypatch.setattr( + "app.routes.github._normalize_head_attribution", + lambda *_args, **_kwargs: {}, + ) + monkeypatch.setattr( + "app.routes.github._assert_merges_with_upstream", + lambda *_args, **_kwargs: { + "last_submit_upstream_branch": "main", + "last_submit_upstream_sha": base, + }, + ) + monkeypatch.setattr( + "app.routes.github._ensure_owner_fork_remote", + lambda *_args, **_kwargs: "octocat/app-demo-1", + ) + monkeypatch.setattr( + "app.routes.github._push_reviewed_topic", + lambda *_args, **kwargs: ("HEAD", kwargs["record_patch"]), + ) + + git_calls = [] + + def fake_git(repo_path, *args, check=True): + git_calls.append(args) + if args == ("rev-parse", "--abbrev-ref", "HEAD"): + return _cp("develop\n") + if args == ("rev-parse", "HEAD"): + return _cp(head + "\n") + return _cp("") + + gh_calls = [] + + def fake_gh(repo_path, *args, check=True): + gh_calls.append(args) + if args[:2] == ("pr", "create"): + raise create_failure + if args[:2] == ("pr", "list"): + if existing_mode == "absent": + return _cp("[]") + found_head = head if existing_mode == "match" else "c" * 40 + return _cp(json.dumps([{ + "url": "https://github.com/mobius-os/app-demo/pull/42", + "headRefOid": found_head, + }])) + if args[:2] == ("api", "--paginate"): + return _cp("bug\n") + if args[:3] == ("api", "--method", "POST"): + return _cp("[]") + return _cp("") + + monkeypatch.setattr("app.routes.github._git", fake_git) + monkeypatch.setattr("app.routes.github._gh", fake_gh) + + response = client.post( + f"/api/github/contributions/{app_id}/{record_id}/submit", + headers={"Authorization": f"Bearer {app_token}"}, + ) + + creates = [call for call in gh_calls if call[:2] == ("pr", "create")] + probes = [call for call in gh_calls if call[:2] == ("pr", "list")] + assert len(creates) == 1, "an ambiguous response must never trigger a second create" + assert len(probes) == 1 + assert creates[0][-2:] == ("--base", "main") + assert "url,headRefOid" in probes[0] + assert "octocat:fix/demo-polish" in probes[0] + assert probes[0][probes[0].index("--base") + 1] == "main" + assert ("checkout", "-q", "develop") in git_calls + + stored = json.loads( + (Path(get_settings().data_dir) / "apps" / str(app_id) / + "contributions" / f"{record_id}.json").read_text() + ) + if existing_mode == "match": + assert response.status_code == 200, response.text + assert response.json()["url"].endswith("/pull/42") + assert stored["status"] == "open" + assert stored["url"].endswith("/pull/42") + assert stored["last_submit_push_sha"] == head + assert stored["last_submit_labels_applied"] == ["bug"] + else: + assert response.status_code == 409, response.text + assert stored["status"] == "prepared" + assert stored["last_submit_stage"] == "pushed" + assert stored["last_submit_push_sha"] == head + assert "url" not in stored def test_submit_contribution_normalizes_fallback_author_before_push(