From 24abf11dabcdb6050c5db3173389edd157391ed8 Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 9 Jun 2026 23:01:42 -0500 Subject: [PATCH] fix(ci): block BM Bossbot approval on unresolved review threads; theme PR images on PR content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two BM Bossbot fixes: 1. Unresolved review threads now block approval. The review prompt only sees metadata+diff, so an approve verdict said nothing about outstanding feedback — #932 merged with two open Codex P2 threads. finalize now counts unresolved reviewThreads via GraphQL and fails the status when any remain. A new recheck command, triggered by pull_request_review / review_comment / review_thread events, flips the status to failure when feedback arrives after approval and restores a previously earned approval for the same head SHA once every thread is resolved. Review and recheck jobs use separate job-level concurrency groups so neither cancels the other. 2. PR images depict the PR's content, not the review outcome. The image prompt previously used the BM Bossbot review summary (verdict/status) as its only source material, so every image rendered an APPROVED stamp. The prompt now sources the PR title and the author's own description (managed bot blocks stripped) and explicitly bans approval stamps, verdict wording, badges, checkmarks, and Bossbot branding. Theme selection seeds on PR number+title for stability across re-reviews. Signed-off-by: phernandez --- .github/workflows/bm-bossbot.yml | 56 ++++- scripts/bm_bossbot_status.py | 170 +++++++++++++ scripts/generate_pr_infographic.py | 78 ++++-- tests/scripts/test_bm_bossbot_status.py | 237 +++++++++++++++++- tests/scripts/test_generate_pr_infographic.py | 87 ++++--- 5 files changed, 564 insertions(+), 64 deletions(-) diff --git a/.github/workflows/bm-bossbot.yml b/.github/workflows/bm-bossbot.yml index 95ea2455..26d358f3 100644 --- a/.github/workflows/bm-bossbot.yml +++ b/.github/workflows/bm-bossbot.yml @@ -11,6 +11,19 @@ name: BM Bossbot pr_number: description: Pull request number to review required: true + # Review-thread activity re-evaluates the approval status without re-running + # the full LLM review: new feedback flips the gate to failure, resolving the + # last thread restores a previously earned approval for the same head SHA. + pull_request_review: + types: + - submitted + pull_request_review_comment: + types: + - created + pull_request_review_thread: + types: + - resolved + - unresolved permissions: contents: read @@ -18,10 +31,6 @@ permissions: statuses: write issues: read -concurrency: - group: bm-bossbot-${{ github.event.workflow_run.pull_requests[0].number || inputs.pr_number }} - cancel-in-progress: true - env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" BM_BOSSBOT_STATUS_CONTEXT: "BM Bossbot Approval" @@ -29,9 +38,15 @@ env: jobs: review: name: BM Bossbot Review + # Job-level concurrency (not workflow-level): thread-recheck events for the + # same PR must never cancel an in-flight review run, and vice versa. + concurrency: + group: bm-bossbot-review-${{ github.event.workflow_run.pull_requests[0].number || inputs.pr_number }} + cancel-in-progress: true if: | github.event_name == 'workflow_dispatch' || ( + github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.pull_requests[0].number != '' ) @@ -298,8 +313,10 @@ jobs: run: | set -euo pipefail gh pr view "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" --json body --jq '.body // ""' > "${RUNNER_TEMP}/bm-bossbot-pr-body.md" + pr_title="$(gh pr view "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" --json title --jq '.title // ""')" uv run --script scripts/generate_pr_infographic.py \ --pr-number "${PR_NUMBER}" \ + --pr-title "${pr_title}" \ --pr-body-file "${RUNNER_TEMP}/bm-bossbot-pr-body.md" \ --provenance-output "${RUNNER_TEMP}/bm-bossbot-image-provenance.md" \ --output "docs/assets/infographics/pr-${PR_NUMBER}.webp" @@ -375,3 +392,34 @@ jobs: Path(output_path).write_text(body, encoding="utf-8") PY gh pr edit "${PR_NUMBER}" --repo "${GITHUB_REPOSITORY}" --body-file "${updated_body}" + + recheck: + name: BM Bossbot Thread Recheck + if: | + github.event_name == 'pull_request_review' || + github.event_name == 'pull_request_review_comment' || + github.event_name == 'pull_request_review_thread' + runs-on: ubuntu-latest + # Job-level concurrency: collapse bursts of thread events for one PR while + # staying isolated from the review job's group so neither cancels the other. + concurrency: + group: bm-bossbot-recheck-${{ github.event.pull_request.number }} + cancel-in-progress: true + steps: + - name: Checkout trusted base ref + uses: actions/checkout@v6 + with: + ref: ${{ github.event.repository.default_branch }} + fetch-depth: 1 + + - name: Set up uv + uses: astral-sh/setup-uv@v3 + + - name: Re-evaluate approval from review-thread state + env: + GITHUB_TOKEN: ${{ github.token }} + run: | + uv run --script scripts/bm_bossbot_status.py recheck \ + --pr-number "${{ github.event.pull_request.number }}" \ + --repo "${GITHUB_REPOSITORY}" \ + --run-url "${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" diff --git a/scripts/bm_bossbot_status.py b/scripts/bm_bossbot_status.py index 9d1c78f7..172e8424 100755 --- a/scripts/bm_bossbot_status.py +++ b/scripts/bm_bossbot_status.py @@ -97,6 +97,66 @@ def pull_request_event( ) +def count_unresolved_review_threads(*, token: str, repo: str, number: int) -> int: + """Count unresolved review threads (e.g. open Codex inline comments) on a PR. + + Review threads are the canonical 'outstanding feedback' signal: bot reviewers + submit COMMENTED reviews that never flip reviewDecision, so thread resolution + is the only deterministic way to know feedback was addressed. + """ + owner, _, name = repo.partition("/") + if not owner or not name: + raise SystemExit(f"Invalid repository: {repo}") + + query = """ + query($owner: String!, $name: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { isResolved } + } + } + } + } + """ + + unresolved = 0 + cursor: str | None = None + while True: + response = _github_request( + method="POST", + path="/graphql", + token=token, + payload={ + "query": query, + "variables": {"owner": owner, "name": name, "number": number, "cursor": cursor}, + }, + ) + if not isinstance(response, Mapping) or response.get("errors"): + raise SystemExit(f"GitHub GraphQL reviewThreads query failed: {response}") + try: + threads = response["data"]["repository"]["pullRequest"]["reviewThreads"] + nodes = threads["nodes"] + page_info = threads["pageInfo"] + except (KeyError, TypeError): + raise SystemExit( + "GitHub GraphQL reviewThreads response was missing expected fields" + ) from None + unresolved += sum(1 for node in nodes if not node.get("isResolved")) + if not page_info.get("hasNextPage"): + return unresolved + cursor = page_info.get("endCursor") + + +def unresolved_threads_result(count: int) -> ApprovalResult: + return ApprovalResult( + False, + "failure", + f"BM Bossbot found {count} unresolved review thread(s)", + ) + + def validate_review(payload: Mapping[str, Any], *, expected_head_sha: str) -> ApprovalResult: required = { "reviewed_head_sha", @@ -245,6 +305,19 @@ def finalize_review( review = {} result = validate_review(review, expected_head_sha=event.head_sha) + # Trigger: the LLM review approved, but reviewers (human or bot, e.g. Codex + # inline comments) still have unresolved threads on the PR. + # Why: the review prompt only sees metadata+diff, never review threads, so an + # approve verdict says nothing about outstanding feedback (#932 merged + # with two open P2 threads because of exactly this gap). + # Outcome: unresolved threads turn an approve into a failure status; the + # recheck command restores approval once all threads are resolved. + if result.approved: + unresolved = count_unresolved_review_threads( + token=token, repo=event.repo, number=event.number + ) + if unresolved > 0: + result = unresolved_threads_result(unresolved) current_body = get_pull_request_body(token=token, repo=event.repo, number=event.number) updated_body = upsert_summary_block(current_body, render_summary(review, result)) update_pull_request_body(token=token, repo=event.repo, number=event.number, body=updated_body) @@ -262,6 +335,89 @@ def finalize_review( return result +def get_pull_request_head_sha(*, token: str, repo: str, number: int) -> str: + response = _github_request( + method="GET", + path=f"/repos/{repo}/pulls/{number}", + token=token, + ) + if not isinstance(response, Mapping): + raise SystemExit("GitHub API response for pull request was invalid") + head = response.get("head") + head_sha = _string(head.get("sha")) if isinstance(head, Mapping) else "" + if not head_sha: + raise SystemExit("GitHub API response was missing pull request head SHA") + return head_sha + + +def head_sha_was_approved(*, token: str, repo: str, sha: str) -> bool: + """Return whether a full BM Bossbot review previously approved this head SHA. + + Commit statuses are append-only history, so the approval record survives a + later thread-failure status for the same SHA. + """ + response = _github_request( + method="GET", + path=f"/repos/{repo}/commits/{sha}/statuses?per_page=100", + token=token, + ) + if not isinstance(response, list): + raise SystemExit("GitHub API response for commit statuses was invalid") + return any( + isinstance(status, Mapping) + and status.get("context") == STATUS_CONTEXT + and status.get("state") == "success" + and status.get("description") == APPROVED_DESCRIPTION + for status in response + ) + + +def recheck_threads( + *, + repo: str, + number: int, + run_url: str, + token_env: str, +) -> None: + """Re-evaluate the approval status when review threads change. + + Trigger: pull_request_review / review_comment / review_thread events. + Why: the full review runs once per head SHA after Tests; feedback that + arrives later (or gets resolved later) must move the gate without + re-running the LLM review. + Outcome: unresolved threads flip the status to failure; once every thread + is resolved, a previously earned approval for the same head SHA is + restored. Without a prior approval the status is left untouched so a + pending/failed review cannot be upgraded by thread resolution alone. + """ + token = _token(token_env) + head_sha = get_pull_request_head_sha(token=token, repo=repo, number=number) + unresolved = count_unresolved_review_threads(token=token, repo=repo, number=number) + + if unresolved > 0: + result = unresolved_threads_result(unresolved) + elif head_sha_was_approved(token=token, repo=repo, sha=head_sha): + result = ApprovalResult(True, "success", APPROVED_DESCRIPTION) + else: + typer.echo( + f"All review threads resolved but no prior approval exists for {head_sha}; " + "leaving status unchanged" + ) + return + + set_commit_status( + token=token, + repo=repo, + sha=head_sha, + payload=build_status_payload( + state=result.state, + description=result.description, + target_url=run_url, + ), + ) + typer.echo(f"Marked {STATUS_CONTEXT} {result.state} for {head_sha} ({result.description})") + + def _github_request( *, method: str, @@ -378,6 +534,20 @@ def finalize( raise typer.Exit(1) +@app.command("recheck") +def recheck( + pr_number: Annotated[int, typer.Option("--pr-number", min=1, help="Pull request number.")], + run_url: Annotated[str, typer.Option("--run-url", help="Workflow run URL.")], + repo: Annotated[str, typer.Option("--repo", help="owner/name repository.")], + token_env: Annotated[ + str, + typer.Option("--token-env", help="Environment variable containing a GitHub token."), + ] = "GITHUB_TOKEN", +) -> None: + """Re-evaluate BM Bossbot Approval from current review-thread state.""" + recheck_threads(repo=repo, number=pr_number, run_url=run_url, token_env=token_env) + + def main() -> None: app() diff --git a/scripts/generate_pr_infographic.py b/scripts/generate_pr_infographic.py index f240e978..728fd9a2 100755 --- a/scripts/generate_pr_infographic.py +++ b/scripts/generate_pr_infographic.py @@ -43,6 +43,17 @@ THEME_START = "" THEME_END = "" PROVENANCE_START = "" PROVENANCE_END = "" +IMAGE_START = "" +IMAGE_END = "" +# Managed blocks are bot-written artifacts (review verdict, image embed, +# provenance). They must never feed the image: sourcing the review summary is +# what made every image an "APPROVED" stamp instead of depicting the change. +MANAGED_BLOCKS = ( + (SUMMARY_START, SUMMARY_END), + (THEME_START, THEME_END), + (PROVENANCE_START, PROVENANCE_END), + (IMAGE_START, IMAGE_END), +) app = typer.Typer( add_completion=False, help="Generate a non-gating BM Bossbot PR image.", @@ -104,15 +115,17 @@ BM_IMAGE_THEME_POOL = ( ) -def extract_bossbot_summary(pr_body: str) -> str: - pattern = re.compile( - rf"{re.escape(SUMMARY_START)}\s*(.*?)\s*{re.escape(SUMMARY_END)}", - flags=re.DOTALL, - ) - match = pattern.search(pr_body) - if not match: - raise ValueError("PR body is missing the BM Bossbot summary block") - return match.group(1).strip() +def extract_pr_content(pr_body: str) -> str: + """Return the author's own PR description with all managed bot blocks removed.""" + content = pr_body + for start, end in MANAGED_BLOCKS: + content = re.sub( + rf"{re.escape(start)}.*?{re.escape(end)}", + "", + content, + flags=re.DOTALL, + ) + return content.strip() def extract_infographic_theme(pr_body: str) -> str | None: @@ -130,7 +143,7 @@ def extract_infographic_theme(pr_body: str) -> str | None: def select_image_theme( *, pr_number: int, - summary: str, + pr_title: str, pr_body: str, theme_override: str | None, ) -> ThemeSelection: @@ -139,7 +152,9 @@ def select_image_theme( body_theme = extract_infographic_theme(pr_body) if body_theme: return ThemeSelection(theme=body_theme, source=ThemeSource.PR_BODY) - seed = f"{pr_number}\n{summary}".encode("utf-8") + # Seed on author-owned PR identity, not the review summary, so the pick is + # stable across re-reviews of the same PR. + seed = f"{pr_number}\n{pr_title}".encode("utf-8") index = int.from_bytes(hashlib.sha256(seed).digest()[:2], byteorder="big") % len( BM_IMAGE_THEME_POOL ) @@ -193,7 +208,8 @@ def upsert_managed_block(body: str, *, block: str, start: str, end: str) -> str: def build_infographic_prompt( *, pr_number: int, - summary: str, + pr_title: str, + pr_content: str, theme: str, theme_source: ThemeSource, ) -> str: @@ -206,18 +222,28 @@ def build_infographic_prompt( return f""" Create a polished landscape WebP editorial image for Basic Memory PR #{pr_number}. -This is a non-gating visual asset. The authoritative merge gate is the -GitHub commit status named BM Bossbot Approval, not this image. +Your subject is the CONTENT of the pull request — what the change does and why +it matters — described in the title and description below. Express the theme of +the whole change as a visual story. -Use the BM Bossbot review summary below as source material. Preserve the -concrete before/after value story without inventing facts or turning -implementation details into clutter. +This image is decoration for the PR conversation. It is NOT a review artifact: +do not depict review verdicts, approval, or process. Never render approval +stamps, "APPROVED"/"SUCCESS"/"VERDICT" wording, rubber stamps, wax seals of +approval, badges, checkmarks, checklists, status lines, SHA strings, or +BM Bossbot itself. If the composition needs text, draw it from the change's +subject matter only. + +Pull request title: +{pr_title} + +Pull request description: +{pr_content} {theme_label}: {theme} Treat the visual direction as style inspiration only. Do not let it override -facts, readability, source material, or the non-gating status of this image. +facts, readability, source material, or the prohibition on review imagery. Use image-first composition: create a scene, movie poster, editorial painting, classic photograph, cover image, symbolic tableau, staged artifact, or another @@ -236,9 +262,6 @@ bullet-list panel, data panel, or dense explanatory diagram. Avoid fake screenshots, code blocks, invented claims, copyrighted characters, logos, named fictional universes, direct band logos, album art, celebrity likenesses, or decorations that obscure content. - -BM Bossbot summary: -{summary} """.strip() @@ -248,6 +271,10 @@ def generate( int, typer.Option("--pr-number", min=1, help="Pull request number."), ], + pr_title: Annotated[ + str, + typer.Option("--pr-title", help="Pull request title (the subject of the image)."), + ], pr_body_file: Annotated[ Path, typer.Option( @@ -284,18 +311,19 @@ def generate( ), ] = False, ) -> None: - """Generate the canonical PR image from a BM Bossbot summary block.""" + """Generate the canonical PR image from the PR's own title and description.""" pr_body = pr_body_file.read_text(encoding="utf-8") - summary = extract_bossbot_summary(pr_body) + pr_content = extract_pr_content(pr_body) theme_selection = select_image_theme( pr_number=pr_number, - summary=summary, + pr_title=pr_title, pr_body=pr_body, theme_override=theme, ) prompt = build_infographic_prompt( pr_number=pr_number, - summary=summary, + pr_title=pr_title, + pr_content=pr_content, theme=theme_selection.theme, theme_source=theme_selection.source, ) diff --git a/tests/scripts/test_bm_bossbot_status.py b/tests/scripts/test_bm_bossbot_status.py index c9f327b9..66a244a3 100644 --- a/tests/scripts/test_bm_bossbot_status.py +++ b/tests/scripts/test_bm_bossbot_status.py @@ -136,8 +136,11 @@ def test_finalize_review_fetches_current_pr_body_before_upserting( statuses.append(payload) monkeypatch.setattr(bm_bossbot_status, "get_pull_request_body", fake_get_pull_request_body) - monkeypatch.setattr(bm_bossbot_status, "update_pull_request_body", fake_update_pull_request_body) + monkeypatch.setattr( + bm_bossbot_status, "update_pull_request_body", fake_update_pull_request_body + ) monkeypatch.setattr(bm_bossbot_status, "set_commit_status", fake_set_commit_status) + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", lambda **_: 0) result = bm_bossbot_status.finalize_review( event_path=event_path, @@ -153,6 +156,234 @@ def test_finalize_review_fetches_current_pr_body_before_upserting( assert statuses[0]["state"] == "success" +def test_finalize_review_blocks_approval_on_unresolved_review_threads( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + event_path = tmp_path / "event.json" + review_path = tmp_path / "review.json" + event_path.write_text(json.dumps(_event_payload()), encoding="utf-8") + review_path.write_text(json.dumps(_review_payload()), encoding="utf-8") + monkeypatch.setenv("GITHUB_TOKEN", "token") + + statuses: list[Mapping[str, str]] = [] + + monkeypatch.setattr(bm_bossbot_status, "get_pull_request_body", lambda **_: "Body") + monkeypatch.setattr(bm_bossbot_status, "update_pull_request_body", lambda **_: None) + monkeypatch.setattr( + bm_bossbot_status, + "set_commit_status", + lambda *, token, repo, sha, payload: statuses.append(payload), + ) + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", lambda **_: 2) + + result = bm_bossbot_status.finalize_review( + event_path=event_path, + review_path=review_path, + repo=None, + run_url="https://github.com/basicmachines-co/basic-memory/actions/runs/1", + token_env="GITHUB_TOKEN", + ) + + assert result.approved is False + assert result.state == "failure" + assert result.description == "BM Bossbot found 2 unresolved review thread(s)" + assert statuses[0]["state"] == "failure" + + +def test_finalize_review_skips_thread_count_when_review_already_failed( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + event_path = tmp_path / "event.json" + review_path = tmp_path / "review.json" + event_path.write_text(json.dumps(_event_payload()), encoding="utf-8") + review_path.write_text( + json.dumps(_review_payload(verdict="changes_requested")), encoding="utf-8" + ) + monkeypatch.setenv("GITHUB_TOKEN", "token") + + monkeypatch.setattr(bm_bossbot_status, "get_pull_request_body", lambda **_: "Body") + monkeypatch.setattr(bm_bossbot_status, "update_pull_request_body", lambda **_: None) + monkeypatch.setattr(bm_bossbot_status, "set_commit_status", lambda **_: None) + + def fail_count(**_: object) -> int: + raise AssertionError("thread count must not run when the review already failed") + + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", fail_count) + + result = bm_bossbot_status.finalize_review( + event_path=event_path, + review_path=review_path, + repo=None, + run_url="https://github.com/basicmachines-co/basic-memory/actions/runs/1", + token_env="GITHUB_TOKEN", + ) + + assert result.approved is False + assert result.description == "BM Bossbot requested changes" + + +def test_count_unresolved_review_threads_pages_through_graphql_results( + monkeypatch: pytest.MonkeyPatch, +) -> None: + pages = [ + { + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "pageInfo": {"hasNextPage": True, "endCursor": "CUR"}, + "nodes": [{"isResolved": False}, {"isResolved": True}], + } + } + } + } + }, + { + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "pageInfo": {"hasNextPage": False, "endCursor": None}, + "nodes": [{"isResolved": False}], + } + } + } + } + }, + ] + cursors: list[object] = [] + + def fake_github_request( + *, method: str, path: str, token: str, payload: Mapping[str, object] | None = None + ) -> object: + assert method == "POST" + assert path == "/graphql" + assert payload is not None + variables = payload["variables"] + assert isinstance(variables, Mapping) + cursors.append(variables["cursor"]) + return pages.pop(0) + + monkeypatch.setattr(bm_bossbot_status, "_github_request", fake_github_request) + + count = bm_bossbot_status.count_unresolved_review_threads( + token="token", repo="basicmachines-co/basic-memory", number=925 + ) + + assert count == 2 + assert cursors == [None, "CUR"] + + +def test_recheck_marks_failure_when_threads_are_unresolved( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "token") + statuses: list[Mapping[str, str]] = [] + + monkeypatch.setattr(bm_bossbot_status, "get_pull_request_head_sha", lambda **_: "abc123") + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", lambda **_: 3) + monkeypatch.setattr( + bm_bossbot_status, + "set_commit_status", + lambda *, token, repo, sha, payload: statuses.append({"sha": sha, **payload}), + ) + + bm_bossbot_status.recheck_threads( + repo="basicmachines-co/basic-memory", + number=925, + run_url="https://github.com/basicmachines-co/basic-memory/actions/runs/2", + token_env="GITHUB_TOKEN", + ) + + assert statuses[0]["sha"] == "abc123" + assert statuses[0]["state"] == "failure" + assert "3 unresolved review thread(s)" in statuses[0]["description"] + + +def test_recheck_restores_prior_approval_when_threads_resolve( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "token") + statuses: list[Mapping[str, str]] = [] + + monkeypatch.setattr(bm_bossbot_status, "get_pull_request_head_sha", lambda **_: "abc123") + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", lambda **_: 0) + monkeypatch.setattr(bm_bossbot_status, "head_sha_was_approved", lambda **_: True) + monkeypatch.setattr( + bm_bossbot_status, + "set_commit_status", + lambda *, token, repo, sha, payload: statuses.append(payload), + ) + + bm_bossbot_status.recheck_threads( + repo="basicmachines-co/basic-memory", + number=925, + run_url="https://github.com/basicmachines-co/basic-memory/actions/runs/2", + token_env="GITHUB_TOKEN", + ) + + assert statuses[0]["state"] == "success" + assert statuses[0]["description"] == "BM Bossbot approved this head SHA" + + +def test_recheck_leaves_status_alone_without_prior_approval( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "token") + + monkeypatch.setattr(bm_bossbot_status, "get_pull_request_head_sha", lambda **_: "abc123") + monkeypatch.setattr(bm_bossbot_status, "count_unresolved_review_threads", lambda **_: 0) + monkeypatch.setattr(bm_bossbot_status, "head_sha_was_approved", lambda **_: False) + + def fail_set_status(**_: object) -> None: + raise AssertionError("status must not change without a prior approval") + + monkeypatch.setattr(bm_bossbot_status, "set_commit_status", fail_set_status) + + bm_bossbot_status.recheck_threads( + repo="basicmachines-co/basic-memory", + number=925, + run_url="https://github.com/basicmachines-co/basic-memory/actions/runs/2", + token_env="GITHUB_TOKEN", + ) + + +def test_head_sha_was_approved_matches_only_the_approval_record( + monkeypatch: pytest.MonkeyPatch, +) -> None: + history = [ + { + "context": "BM Bossbot Approval", + "state": "failure", + "description": "BM Bossbot found 2 unresolved review thread(s)", + }, + { + "context": "BM Bossbot Approval", + "state": "success", + "description": "BM Bossbot approved this head SHA", + }, + {"context": "license/cla", "state": "success", "description": "ok"}, + ] + monkeypatch.setattr(bm_bossbot_status, "_github_request", lambda **_: history) + + assert ( + bm_bossbot_status.head_sha_was_approved( + token="token", repo="basicmachines-co/basic-memory", sha="abc123" + ) + is True + ) + + monkeypatch.setattr(bm_bossbot_status, "_github_request", lambda **_: history[:1]) + assert ( + bm_bossbot_status.head_sha_was_approved( + token="token", repo="basicmachines-co/basic-memory", sha="abc123" + ) + is False + ) + + def test_finalize_cli_marks_failure_when_review_file_is_missing( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -181,7 +412,9 @@ def test_finalize_cli_marks_failure_when_review_file_is_missing( statuses.append(payload) monkeypatch.setattr(bm_bossbot_status, "get_pull_request_body", fake_get_pull_request_body) - monkeypatch.setattr(bm_bossbot_status, "update_pull_request_body", fake_update_pull_request_body) + monkeypatch.setattr( + bm_bossbot_status, "update_pull_request_body", fake_update_pull_request_body + ) monkeypatch.setattr(bm_bossbot_status, "set_commit_status", fake_set_commit_status) result = CliRunner().invoke( diff --git a/tests/scripts/test_generate_pr_infographic.py b/tests/scripts/test_generate_pr_infographic.py index b50ada16..4ee496e2 100644 --- a/tests/scripts/test_generate_pr_infographic.py +++ b/tests/scripts/test_generate_pr_infographic.py @@ -25,6 +25,7 @@ def test_generate_pr_infographic_cli_help_exposes_useful_options() -> None: assert result.exit_code == 0 assert "--pr-number" in help_text + assert "--pr-title" in help_text assert "--pr-body-file" in help_text assert "--output" in help_text assert "--theme" in help_text @@ -33,26 +34,38 @@ def test_generate_pr_infographic_cli_help_exposes_useful_options() -> None: assert "--dry-run" in help_text -def test_extract_bossbot_summary_from_pr_body() -> None: +def test_extract_pr_content_strips_managed_bot_blocks() -> None: body = "\n".join( [ - "Before", + "## Summary", + "Adds per-workspace rclone remotes for Team push/pull.", "", "Reviewed SHA: abc123", "Verdict: approve", "", - "After", + "", + "![BM Bossbot image for PR #42](https://example.test/img.webp)", + "", + "", + "provenance details", + "", + "## Test plan", + "pytest passes.", ] ) - summary = generate_pr_infographic.extract_bossbot_summary(body) + content = generate_pr_infographic.extract_pr_content(body) - assert summary == "Reviewed SHA: abc123\nVerdict: approve" + assert "Adds per-workspace rclone remotes" in content + assert "pytest passes." in content + assert "Verdict: approve" not in content + assert "Reviewed SHA" not in content + assert "provenance details" not in content + assert "BM Bossbot image for PR" not in content -def test_extract_bossbot_summary_requires_managed_block() -> None: - with pytest.raises(ValueError, match="BM Bossbot summary block"): - generate_pr_infographic.extract_bossbot_summary("No managed summary") +def test_extract_pr_content_handles_body_without_managed_blocks() -> None: + assert generate_pr_infographic.extract_pr_content("Plain description") == "Plain description" def test_extract_infographic_theme_from_pr_body() -> None: @@ -86,19 +99,19 @@ def test_select_image_theme_reports_source() -> None: from_body = generate_pr_infographic.select_image_theme( pr_number=42, - summary="Summary: Adds a merge gate.", + pr_title="feat(ci): add a merge gate", pr_body=body, theme_override=None, ) from_cli = generate_pr_infographic.select_image_theme( pr_number=42, - summary="Summary: Adds a merge gate.", + pr_title="feat(ci): add a merge gate", pr_body=body, theme_override="80's action movies", ) from_auto = generate_pr_infographic.select_image_theme( pr_number=42, - summary="Summary: Adds a merge gate.", + pr_title="feat(ci): add a merge gate", pr_body="No theme", theme_override=None, ) @@ -111,34 +124,38 @@ def test_select_image_theme_reports_source() -> None: assert from_auto.source == generate_pr_infographic.ThemeSource.AUTO -def test_build_infographic_prompt_uses_summary_without_making_gate_claims() -> None: +def test_build_infographic_prompt_depicts_pr_content_not_review_outcome() -> None: prompt = generate_pr_infographic.build_infographic_prompt( pr_number=42, - summary="Verdict: approve\nSummary: Adds a merge gate.", + pr_title="feat(sync): stream large files during cloud sync", + pr_content="Streams PDFs in chunks instead of loading them fully into memory.", theme="WWII propaganda posters with home-front logistics routes", theme_source=generate_pr_infographic.ThemeSource.CLI, ) assert "PR #42" in prompt - assert "Adds a merge gate" in prompt + assert "stream large files during cloud sync" in prompt + assert "Streams PDFs in chunks" in prompt assert "WWII propaganda posters" in prompt assert "User-supplied visual direction" in prompt assert "style inspiration only" in prompt assert "polished landscape WebP editorial image" in prompt assert "image-first composition" in prompt - assert "scene" in prompt - assert "poster" in prompt - assert "painting" in prompt - assert "classic photograph" in prompt assert "symbolic tableau" in prompt - assert "before/after value story" in prompt assert "Do not render an infographic" in prompt assert "dashboard" in prompt assert "flowchart" in prompt assert "copyrighted characters" in prompt - assert "restrained" not in prompt - assert "non-gating" in prompt - assert "BM Bossbot Approval" in prompt + # The subject is the change itself; review-process imagery is banned. + assert "CONTENT of the pull request" in prompt + assert "do not depict review verdicts" in prompt + assert "approval" in prompt.lower() + assert "stamps" in prompt + assert "checkmarks" in prompt + # The old prompt fed the review summary and named the approval status, + # which produced literal "BOSSBOT APPROVED" stamp images. + assert "BM Bossbot summary:" not in prompt + assert "BM Bossbot Approval" not in prompt def test_build_infographic_provenance_block_includes_image_choices_without_prompt() -> None: @@ -204,13 +221,14 @@ def test_upsert_managed_block_appends_and_replaces() -> None: def test_build_infographic_prompt_uses_auto_theme_as_visual_direction() -> None: theme = generate_pr_infographic.select_image_theme( pr_number=42, - summary="Verdict: approve\nSummary: Adds a merge gate.", + pr_title="feat(ci): add a merge gate", pr_body="No theme", theme_override=None, ) prompt = generate_pr_infographic.build_infographic_prompt( pr_number=42, - summary="Verdict: approve\nSummary: Adds a merge gate.", + pr_title="feat(ci): add a merge gate", + pr_content="Adds a deterministic merge gate for pull requests.", theme=theme.theme, theme_source=theme.source, ) @@ -242,9 +260,10 @@ def test_generate_pr_infographic_can_print_prompt_without_image_call( body_file.write_text( "\n".join( [ + "Adds a deterministic merge gate for pull requests.", "", "Verdict: approve", - "Summary: Adds a merge gate.", + "Summary: review artifact that must not reach the image.", "", "", "space exploration and astronomy", @@ -267,6 +286,8 @@ def test_generate_pr_infographic_can_print_prompt_without_image_call( [ "--pr-number", "42", + "--pr-title", + "feat(ci): add a merge gate", "--pr-body-file", str(body_file), "--output", @@ -277,14 +298,15 @@ def test_generate_pr_infographic_can_print_prompt_without_image_call( assert result.exit_code == 0, result.output assert ( - "Create a polished landscape WebP editorial image for Basic Memory PR #42" - in result.output + "Create a polished landscape WebP editorial image for Basic Memory PR #42" in result.output ) - assert "Adds a merge gate" in result.output + assert "feat(ci): add a merge gate" in result.output + assert "Adds a deterministic merge gate" in result.output assert "space exploration and astronomy" in result.output assert "image-first composition" in result.output assert "Do not render an infographic" in result.output - assert "BM Bossbot Approval" in result.output + assert "Verdict: approve" not in result.output + assert "must not reach the image" not in result.output assert not output.exists() @@ -296,10 +318,7 @@ def test_generate_pr_infographic_writes_provenance_after_image_generation( body_file.write_text( "\n".join( [ - "", - "Verdict: approve", - "Summary: Adds a merge gate.", - "", + "Adds a merge gate.", "", "paintings: Rembrandt-inspired merge gate", "", @@ -329,6 +348,8 @@ def test_generate_pr_infographic_writes_provenance_after_image_generation( [ "--pr-number", "42", + "--pr-title", + "feat(ci): add a merge gate", "--pr-body-file", str(body_file), "--output",