ci: bf-pr-context — representative-tree CI for patch PRs - #185
vashbrightfire[bot] wants to merge 5 commits into
Conversation
![vashbrightfire[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Patch PRs inherit a background failure set because upstream PR CI tests base + one patch, missing every other patch's skips and fixes. This adds an assembly workflow that checks out exactly what bf-build-stable would build on merge (base + all manifest patches at recorded SHAs + the PR head), runs the extracted test suite against it, and gates only on NEW failures relative to an expected-failures ledger. - assemble: manifest-driven merge (reuses parse-patches.py, merge-patches.sh, regenerate-artifacts.sh, the inline matrix computation), pushes to a pr-context/<N> temp ref (GITHUB_TOKEN) - test: ci-test-suite.yml against the assembled ref; cache-mode stays restore — PRs never warm release caches (ci-workflow-guards compliant) - summary: diffs failing test files against .github/brightfire-ci/expected-test-failures.json, comments on the PR, fails only on new failures; ledger starts empty - cleanup: deletes the temp ref on PR close Also adds docs/brightfire-ci.md: the failure taxonomy, the per-assertion test-change doctrine, skip-placement rules, and the triage playbook. Note: this workflow only takes effect after merge (pull_request workflows must exist on the base branch).
Any failing test on the assembled tree gates the PR. A failure that exists independent of the PR gets fixed or skipped at the proper layer (upstream-test-fixes or the owning patch), never bookkept. Removes the ledger file and simplifies the summary job.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 22bf0f085c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Without an explicit ref, actions/checkout uses the pull-request merge tree rather than the branch named by this step. For example, the manifest-recorded patch commit 43c8b3b545 contains neither BRIGHTFIRE_PATCHES.md nor scripts/bf/parse-patches.py, so a PR against that patch reaches the next step with its required inputs absent; the same omission at the summary checkout also makes the supposedly trusted ledger PR-controlled. Pin both checkouts to the trusted brightfire/ci revision before executing or reading these inputs.
AGENTS.md reference: AGENTS.md:L191-L191
Useful? React with 👍 / 👎.
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
Specifying only contents: write sets omitted GITHUB_TOKEN permissions to none. The summary later lists workflow jobs and downloads logs, which require actions: read, and calls gh pr comment, which requires pull-requests: write; every run reaching those operations therefore fails with an authorization error instead of publishing a verdict.
Useful? React with 👍 / 👎.
| run: | | ||
| BASE="${{ steps.failures.outputs.base }}" | ||
| LEDGER=".github/brightfire-ci/expected-test-failures.json" | ||
| python3 - "$LEDGER" "$BASE" /tmp/prctx/actual-failures.txt > /tmp/prctx/result.json <<'EOF' |
There was a problem hiding this comment.
This redirects Python stdout to /tmp/prctx/result.json while the script separately opens the same path with mode w; the final print() then overwrites the beginning of the JSON through the original stdout descriptor. Reproducing this block yields a file beginning new=0 followed by a JSON suffix, so the json.load on line 240 always raises and the summary never reaches its gate or comment.
Useful? React with 👍 / 👎.
| echo "base=$BASE_COMMIT" >> "$GITHUB_OUTPUT" | ||
| mkdir -p /tmp/prctx | ||
| FAILED_FILES="$(mktemp)" | ||
| for job_id in $(gh api "repos/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID/jobs?per_page=200" --paginate --jq '.jobs[] | select(.conclusion == "FAILURE" and (.name | startswith("Run upstream CI test suite"))) | .databaseId'); do |
There was a problem hiding this comment.
The Actions REST response reports the conclusion as lowercase failure and exposes the job identifier as .id, not uppercase FAILURE and .databaseId. Consequently this query produces no usable job IDs, actual-failures.txt remains empty, and—once the other summary blockers are repaired—a genuinely failing assembled suite is reported as green.
AGENTS.md reference: AGENTS.md:L46-L46
Useful? React with 👍 / 👎.
| uses: ./.github/workflows/ci-test-suite.yml | ||
| with: | ||
| test_ref: ${{ needs.assemble.outputs.context_ref }} | ||
| test_matrix: ${{ needs.assemble.outputs.test_shards }} |
There was a problem hiding this comment.
Any failing matrix shard in ci-test-suite.yml makes this reusable-workflow caller job fail, and a later successful summary job cannot change that job or the overall workflow conclusion. Therefore every ledgered failure still leaves PR-context CI red, contradicting the workflow's central promise that only new failures gate; the test workflow must return machine-readable results without independently failing for ledger-eligible failures.
Useful? React with 👍 / 👎.
| gh api "repos/$GITHUB_REPOSITORY/actions/jobs/$job_id/logs" 2>/dev/null \ | ||
| | awk 'length($0) < 400' \ | ||
| | grep -oE '× +[a-z-]+ +[^ ]+\.test\.[a-z]+' \ | ||
| | awk '{print $NF}' >> "$FAILED_FILES" || true |
There was a problem hiding this comment.
This extraction only recognizes verbose assertion lines containing × ... .test.<ext>. Collection/import errors, beforeAll failures, process timeouts or OOMs, and failures in checkout/setup/build have no such line, so the summary records zero new files and posts an “assembled tree green” result despite a failed shard. Consume a structured per-file result or treat every unclassified failed job as a gating failure rather than inferring success from incomplete logs.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "$comment": "Expected failing test FILES on the assembled PR-context tree, keyed by the manifest's pinned base commit. A file listed here is reported as a known failure and does not gate the PR-context summary; any failing file NOT listed here is treated as a new failure introduced by the PR and fails the summary job. Compare against https://github.com/brightfire/openclaw/blob/stable/ <base> — remove entries as they are fixed; add entries only for failures verified to exist on the assembled tree independent of the PR under review (e.g. pre-existing upstream breakage at this base). Keep this list shrinking.", | |||
| "v2026.8.2-999239d745d": [] | |||
There was a problem hiding this comment.
The workflow extracts the raw base commit 999239d745d and calls ledger.get(base, []), but the only supplied ledger key is v2026.8.2-999239d745d. When expected failures are added under this documented key, none are recognized and every known failure is classified as new; either store raw commit keys or construct the same version-prefixed key in the workflow.
Useful? React with 👍 / 👎.
| REF="heads/pr-context/${{ github.event.pull_request.number }}" | ||
| STATUS=$(gh api -o /dev/null -w "%{http_code}" "repos/$GITHUB_REPOSITORY/git/$REF" 2>/dev/null || echo 404) | ||
| if [ "$STATUS" = "200" ]; then | ||
| gh api -X DELETE "repos/$GITHUB_REPOSITORY/git/$REF" |
There was a problem hiding this comment.
The installed gh api --help lists --silent for output suppression and has no curl-style -o or -w flags. Thus this command exits on unknown shorthand flag: 'o', the || echo 404 fallback hides the error, and every closed PR leaves its pr-context/<N> branch behind; use supported gh api flags and the Git-reference endpoint directly.
Useful? React with 👍 / 👎.
| - name: Merge PR head | ||
| run: | | ||
| git merge --no-edit "${{ github.event.pull_request.head.sha }}" | ||
| echo "Assembled: $(git rev-parse HEAD)" |
There was a problem hiding this comment.
For a PR updating an existing patch that is not last in the manifest, the workflow first applies that patch's old recorded SHA, applies every later patch, and only then merges the PR head. The next stable build instead records the new patch SHA and applies it at the patch's original position before later patches, so overlapping later patches can produce different conflicts or final bytes; substitute the PR revision during the ordered patch assembly rather than appending it afterward.
Useful? React with 👍 / 👎.
c38bf61 to
de0df3b
Compare
1 participant
Why
Every patch PR inherits a background failure set: upstream PR CI tests base + one patch, so the other patches' skips and fixes don't apply — burying "did MY change break this?" under noise that varies by target branch. Today's #182 run demonstrated it: 6 failing checks, only 2 about the PR's own content.
What
bf-pr-context.yml— for PRs targetingbrightfire/<patch>branches (ci excluded), assemble the treebf-build-stablewould build on merge:pr-context/<N>temp refci-test-suite.yml) runs against the assembled ref — a failure means stable will fail if this mergesdocs/brightfire-ci.md— the failure taxonomy (6 classes), the per-assertion test-change doctrine for contract-changing patches, skip-placement rules (upstream-test-fixes vs owning patch vs ci branch), and the triage playbook (diff against the base's last run, never absolute red/green).Effect
Notes