From 22bf0f085cf13f8c88ccb2c5ca5eae88865d3286 Mon Sep 17 00:00:00 2001 From: "vashbrightfire[bot]" Date: Tue, 8 Sep 2026 12:22:42 -0500 Subject: [PATCH 1/5] =?UTF-8?q?ci:=20bf-pr-context=20=E2=80=94=20represent?= =?UTF-8?q?ative-tree=20CI=20for=20patch=20PRs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/ 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). --- .../brightfire-ci/expected-test-failures.json | 4 + .github/workflows/bf-pr-context.yml | 293 ++++++++++++++++++ docs/brightfire-ci.md | 98 ++++++ 3 files changed, 395 insertions(+) create mode 100644 .github/brightfire-ci/expected-test-failures.json create mode 100644 .github/workflows/bf-pr-context.yml create mode 100644 docs/brightfire-ci.md diff --git a/.github/brightfire-ci/expected-test-failures.json b/.github/brightfire-ci/expected-test-failures.json new file mode 100644 index 0000000000000..15a7716bb8873 --- /dev/null +++ b/.github/brightfire-ci/expected-test-failures.json @@ -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/ — 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": [] +} diff --git a/.github/workflows/bf-pr-context.yml b/.github/workflows/bf-pr-context.yml new file mode 100644 index 0000000000000..7d119457a761c --- /dev/null +++ b/.github/workflows/bf-pr-context.yml @@ -0,0 +1,293 @@ +name: "BF: PR Context CI" + +# Assembles the REPRESENTATIVE tree for patch-branch PRs — the same tree +# bf-build-stable would build on merge: upstream base + all manifest +# patches (at their recorded SHAs) + this PR's head — and runs the +# upstream test suite against THAT, instead of the raw base+one-patch +# tree that upstream PR CI sees. +# +# Why: PRs against brightfire/ inherit a background failure set +# from the missing patch context (other patches' skips and fixes don't +# apply), which buries the signal ("did MY change break this?"). A +# failure on the assembled tree means "stable will fail if this merges". +# +# Raw-tree upstream CI still runs on patch PRs and catches patch-local +# hygiene (lint, knip, max-lines). This workflow answers the integration +# question. Failures are compared against the expected-failures ledger +# (.github/brightfire-ci/expected-test-failures.json); only NEW failures +# gate. See docs/brightfire-ci.md. + +on: + pull_request: + types: [opened, synchronize, reopened, closed] + branches: + - "brightfire/**" + - "!brightfire/ci" + +concurrency: + group: pr-context-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: + contents: write + +env: + PATCHES_FILE: BRIGHTFIRE_PATCHES.md + PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: "false" + +jobs: + # ======================================================================== + # Assemble: base + all manifest patches (recorded SHAs) + PR head, + # regenerate artifacts, compute the test matrix, push to a temp ref. + # ======================================================================== + assemble: + name: Assemble PR context tree + if: >- + github.event_name == 'pull_request' && + github.event.action != 'closed' && + github.event.pull_request.base.ref != 'brightfire/ci' + runs-on: ubuntu-24.04 + timeout-minutes: 30 + outputs: + context_ref: ${{ steps.push.outputs.ref }} + test_shards: ${{ steps.test-shards.outputs.shards }} + steps: + - name: Checkout brightfire/ci + uses: actions/checkout@v5 + with: + fetch-depth: 0 + + - name: Parse manifest (patches + base branch) + id: patches + run: | + python3 scripts/bf/parse-patches.py "$PATCHES_FILE" "$GITHUB_OUTPUT" + + - name: Read upstream version from manifest + id: upstream-version + run: | + VERSION=$(grep -oP '(?<=Upstream version:\*\*\s\x60v)[^\x60]+' "$PATCHES_FILE") + if [ -z "$VERSION" ]; then + echo "::error::Could not read Upstream version from $PATCHES_FILE _meta" + exit 1 + fi + echo "version=$VERSION" >> "$GITHUB_OUTPUT" + + # Preserve every CI-side input from this exact brightfire/ci revision + # before switching to the assembled tree (same pattern as + # bf-build-stable). + - name: Preserve Brightfire CI inputs + run: | + INPUTS=/tmp/bf-ci-inputs + mkdir -p "$INPUTS/scripts" + cp "$PATCHES_FILE" "$INPUTS/BRIGHTFIRE_PATCHES.md" + cp -a scripts/bf "$INPUTS/scripts/" + echo "BF_CI_INPUTS_DIR=$INPUTS" >> "$GITHUB_ENV" + echo "PATCHES_FILE=$INPUTS/BRIGHTFIRE_PATCHES.md" >> "$GITHUB_ENV" + + - name: Fetch patch branches and PR head + run: | + git fetch origin '+refs/heads/brightfire/*:refs/remotes/origin/brightfire/*' + git fetch origin "${{ github.event.pull_request.head.sha }}" + + - name: Prepare base checkout + env: + BASE_COMMIT: ${{ steps.patches.outputs.base_commit }} + run: | + git config user.name "brightfire-ci" + git config user.email "ci@brightfire.net" + git checkout -B pr-context-work "$BASE_COMMIT" + + # Merge every patch at its RECORDED manifest SHA (the contract), + # then the PR head on top — exactly what the next build-stable + # would assemble after this PR merges and register-patch records it. + - name: Merge patches in manifest order + env: + PATCHES: ${{ steps.patches.outputs.list }} + VERSION: ${{ steps.upstream-version.outputs.version }} + run: /tmp/bf-ci-inputs/scripts/bf/build-stable/merge-patches.sh + + - name: Merge PR head + run: | + git merge --no-edit "${{ github.event.pull_request.head.sha }}" + echo "Assembled: $(git rev-parse HEAD)" + + - name: Setup Node environment + uses: ./.github/actions/setup-node-env + with: + node-version: "24" + install-bun: "false" + cache-mode: "restore" + install-deps: "false" + + - name: Install dependencies + run: pnpm install --no-frozen-lockfile + + - name: Regenerate build artifacts + run: /tmp/bf-ci-inputs/scripts/bf/build-stable/regenerate-artifacts.sh + + - name: Compute test matrix + id: test-shards + run: | + node --import tsx -e " + const plan = await import('./scripts/lib/ci-node-test-plan.mts'); + const create = typeof plan.createNodeTestShardBundles === 'function' + ? plan.createNodeTestShardBundles + : plan.createNodeTestShards; + const shards = await create({ runnerBackend: 'github' }); + const runnerMap = { + 'blacksmith-4vcpu-ubuntu-2404': 'ubuntu-24.04', + 'blacksmith-8vcpu-ubuntu-2404': 'ubuntu-24.04', + 'blacksmith-16vcpu-ubuntu-2404': 'ubuntu-24.04', + }; + const matrix = shards.map(s => ({ + shard_name: s.shardName ?? s.checkName, + groups: s.groups ?? null, + configs: s.configs ?? [], + runner: runnerMap[s.runner] ?? s.runner ?? 'ubuntu-24.04', + timeout_minutes: s.timeoutMinutes ?? 60, + env: s.env ?? {}, + includePatterns: s.includePatterns ?? [], + targets: s.targets ?? [], + plan_concurrency: s.planConcurrency ?? null, + pretest_build_mode: s.pretestBuildMode ?? null, + requires_dist: s.requiresDist ?? false, + requires_go: s.requiresGo ?? false, + requires_ripgrep: s.requiresRipgrep ?? false, + })); + console.log(JSON.stringify(matrix)); + " > /tmp/test-matrix.json + SHARDS=$(cat /tmp/test-matrix.json) + echo "shards=$SHARDS" >> "$GITHUB_OUTPUT" + SHARD_COUNT=$(echo "$SHARDS" | python3 -c "import json,sys; print(len(json.load(sys.stdin)))") + echo "Test shards ($SHARD_COUNT)" + + # GITHUB_TOKEN push: does not trigger downstream workflows (fine — + # nothing triggers on pr-context/*), and branch protection doesn't + # cover pr-context/* refs. Force-push overwrites stale assemblies. + - name: Push context ref + id: push + run: | + REF="pr-context/${{ github.event.pull_request.number }}" + git push --force origin "HEAD:refs/heads/$REF" + echo "ref=$REF" >> "$GITHUB_OUTPUT" + + # ======================================================================== + # Test: the extracted suite against the assembled ref. cache-mode stays + # at its default (restore): PR runs never write the dist cache — warming + # release caches is the release lane's job. + # ======================================================================== + test: + name: Run upstream CI test suite (assembled tree) + needs: assemble + uses: ./.github/workflows/ci-test-suite.yml + with: + test_ref: ${{ needs.assemble.outputs.context_ref }} + test_matrix: ${{ needs.assemble.outputs.test_shards }} + + # ======================================================================== + # Summary: compare failing test FILES against the expected-failures + # ledger. Only NEW failing files gate; ledger entries are informational. + # ======================================================================== + summary: + name: Compare failures with expected ledger + needs: [assemble, test] + if: always() && needs.assemble.result == 'success' + runs-on: ubuntu-24.04 + timeout-minutes: 10 + env: + GITHUB_TOKEN: ${{ github.token }} + steps: + - name: Checkout brightfire/ci + uses: actions/checkout@v5 + + - name: Collect failing test files + id: failures + run: | + BASE_COMMIT=$(python3 scripts/bf/parse-patches.py BRIGHTFIRE_PATCHES.md /dev/stdout | grep -oP '(?<=base_commit=).*' | head -1) + 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 + 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 + done + sort -u "$FAILED_FILES" > /tmp/prctx/actual-failures.txt || true + ACTUAL_COUNT=$(wc -l < /tmp/prctx/actual-failures.txt) + echo "actual_count=$ACTUAL_COUNT" >> "$GITHUB_OUTPUT" + cat /tmp/prctx/actual-failures.txt + + - name: Diff against ledger + id: ledger + 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' + import json, sys + ledger_path, base, actual_path = sys.argv[1], sys.argv[2], sys.argv[3] + ledger = json.load(open(ledger_path)) + known = set(ledger.get(base, [])) + try: + actual = set(line.strip() for line in open(actual_path) if line.strip()) + except FileNotFoundError: + actual = set() + new = sorted(actual - known) + stale = sorted(known - actual) + json.dump({"new": new, "stale": stale, "known_count": len(known & actual)}, open("/tmp/prctx/result.json", "w")) + print(f"new={len(new)}") + EOF + NEW_COUNT=$(python3 -c "import json; print(len(json.load(open('/tmp/prctx/result.json'))['new']))") + echo "new_count=$NEW_COUNT" >> "$GITHUB_OUTPUT" + if [ "$NEW_COUNT" -gt 0 ]; then + echo "new_failures=true" >> "$GITHUB_OUTPUT" + else + echo "new_failures=false" >> "$GITHUB_OUTPUT" + fi + + - name: Comment on PR + run: | + PR="${{ github.event.pull_request.number }}" + NEW_COUNT="${{ steps.ledger.outputs.new_count }}" + ACTUAL_COUNT="${{ steps.failures.outputs.actual_count }}" + NEW_LIST=$(python3 -c "import json; print('\n'.join('- '+f for f in json.load(open('/tmp/prctx/result.json'))['new']))") + STALE_LIST=$(python3 -c "import json; print('\n'.join('- '+f for f in json.load(open('/tmp/prctx/result.json'))['stale'][:10]))") + if [ "$NEW_COUNT" -gt 0 ]; then + gh pr comment "$PR" --body "$(cat < ref when the PR closes. + # ======================================================================== + cleanup: + name: Delete context ref + if: github.event.action == 'closed' + runs-on: ubuntu-24.04 + timeout-minutes: 5 + env: + GITHUB_TOKEN: ${{ github.token }} + steps: + - name: Delete pr-context ref + run: | + 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" + echo "Deleted $REF" + else + echo "No context ref to delete" + fi diff --git a/docs/brightfire-ci.md b/docs/brightfire-ci.md new file mode 100644 index 0000000000000..5ccbd58d12e4c --- /dev/null +++ b/docs/brightfire-ci.md @@ -0,0 +1,98 @@ +# Brightfire CI Guide + +How CI works on the Brightfire fork, how to read a red PR, and where test +changes belong. + +## The two questions CI answers + +A patch PR gets two kinds of signal: + +1. **"Is my patch clean on its own?"** — upstream CI runs against the raw + PR tree (base + this patch only). Catches patch-local hygiene: lint, + knip, max-lines, formatting. Runs on every PR; failures here are + usually about the patch itself. +2. **"Will the integration work?"** — `bf-pr-context.yml` assembles the + representative tree (upstream base + ALL manifest patches at their + recorded SHAs + the PR head — exactly what `bf-build-stable` would + build on merge) and runs the upstream test suite against that. A + failure here means stable will fail if this merges. + +The raw-tree checks will show failures from _missing patch context_ (see +taxonomy below) — those are noise on question 1 and are the reason +question 2 exists. The PR-context summary gates only on **new** failures +relative to the ledger. + +## Failure taxonomy + +Every red check is one of: + +| Class | Example | Where the fix goes | +| ------------------------ | -------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | +| PR's own change | a test your patch actually breaks; a workflow guard your edit violates | the PR itself | +| Patch contract change | upstream test asserts the old contract your patch deliberately reverses | the owning patch, per the doctrine below | +| Pre-existing at base | fails on the clean upstream tree too | `upstream-test-fixes` (skip, with clean-base evidence) | +| CI-environment-sensitive | passes locally, fails on GH-hosted runners (browser engines, API shapes, timing) | `upstream-test-fixes` (skip, with CI-run evidence) | +| Missing patch context | fails only on raw-tree PR CI because other patches' skips/fixes aren't applied | nowhere — this is PR-context CI's job to suppress; do not "fix" it per-PR | +| Infra flake | artifact-service 403, runner failures | rerun; do not code around | + +## Test-change doctrine (per-assertion) + +When a patch deliberately changes a contract (e.g. bundle-all-plugins ships +all plugins in the tarball), evaluate failing tests **per assertion**, not +per file: + +1. The whole case asserts the old contract (externality/exclusion) → + invalid on the patched tree → skip the case (or delete it if the patch + erroneously carried it). +2. The old contract is one condition inside a broader valid test → + surgically remove that condition; keep the test active. +3. The test asserts presence/correctness of what the patch now ships → + update its expectations to the patched tree's truth; the test now + **enforces** the new contract. +4. Ambiguous → skip, and say why in the resolution note. + +**Skip placement:** env/pre-existing failures go in `upstream-test-fixes` +(applies first, so every later tree inherits the green baseline). Contract +changes go in the patch that changes the contract. CI infrastructure goes +on `brightfire/ci`. Never maintain the same skip in two places — if you +find yourself copying a skip between the patch stack and a branch, the +assembly step is missing (see PR-context CI). + +**Before skipping anything because "the patch changed the contract":** +check what the test actually pins. A test that fails because of a patch +can still be flagging a real bug **in** the patch (example: the +`cli-http-fallback` probe test caught the /ready fallback masking +explicitly rejected tokens — the fix was gating the patch, not skipping +the test). + +## Triage playbook + +1. **Never reason from absolute red/green.** Diff the PR's failing checks + against the base branch's last CI run, or against the previous head's + run. New-vs-known is the only signal. +2. For each new failing test, classify it against the taxonomy above. + Reproduce on the clean base before calling anything "pre-existing". +3. Route the fix per the table. Patch PRs should not accumulate skips for + classes that PR-context CI already covers. + +## The expected-failures ledger + +`.github/brightfire-ci/expected-test-failures.json` lists, per pinned +base commit, the test files expected to fail on the **assembled** tree. +PR-context CI gates only on failing files not in the ledger. Rules: + +- Add an entry only with evidence that the failure exists on the + assembled tree independent of the PR under review. +- Remove entries as they are fixed — the list must trend to zero. +- Entries are file-level for now; case-level granularity can be added + when file-level proves too coarse. + +## Merge order and intermediate red builds + +`bf-register-patch` commits manifest bumps to `brightfire/ci` on every +patch-PR merge, and those commits touch `BRIGHTFIRE_PATCHES.md` — which +triggers `bf-build-stable`. Multi-PR sets therefore produce **intermediate +red build-stable runs** while the set is landing. That is expected: use +those runs as incremental verification of what has landed so far, and +expect the final PR in the set (usually the `brightfire/ci` one) to +trigger the first fully green run. From 96a6525f3b2b97079572a154245f11dc111a732f Mon Sep 17 00:00:00 2001 From: "vashbrightfire[bot]" Date: Tue, 8 Sep 2026 12:25:14 -0500 Subject: [PATCH 2/5] docs: wrap brightfire-ci.md taxonomy table to 180 cols --- docs/brightfire-ci.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/brightfire-ci.md b/docs/brightfire-ci.md index 5ccbd58d12e4c..a22d3d3ae04b3 100644 --- a/docs/brightfire-ci.md +++ b/docs/brightfire-ci.md @@ -26,14 +26,14 @@ relative to the ledger. Every red check is one of: -| Class | Example | Where the fix goes | -| ------------------------ | -------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | -| PR's own change | a test your patch actually breaks; a workflow guard your edit violates | the PR itself | -| Patch contract change | upstream test asserts the old contract your patch deliberately reverses | the owning patch, per the doctrine below | -| Pre-existing at base | fails on the clean upstream tree too | `upstream-test-fixes` (skip, with clean-base evidence) | -| CI-environment-sensitive | passes locally, fails on GH-hosted runners (browser engines, API shapes, timing) | `upstream-test-fixes` (skip, with CI-run evidence) | -| Missing patch context | fails only on raw-tree PR CI because other patches' skips/fixes aren't applied | nowhere — this is PR-context CI's job to suppress; do not "fix" it per-PR | -| Infra flake | artifact-service 403, runner failures | rerun; do not code around | +| Class | Example | Where the fix goes | +| ------------------------ | ------------------------------------------------------ | ---------------------------------------------------------------------- | +| PR's own change | a test your patch actually breaks; a guard it violates | the PR itself | +| Patch contract change | upstream test asserts the old contract the patch flips | the owning patch, per the doctrine below | +| Pre-existing at base | fails on the clean upstream tree too | `upstream-test-fixes` (skip, with clean-base evidence) | +| CI-environment-sensitive | passes locally, fails on GH-hosted runners | `upstream-test-fixes` (skip, with CI-run evidence) | +| Missing patch context | fails only on raw-tree PR CI — other patches missing | nowhere — PR-context CI suppresses it; do not "fix" it per-PR | +| Infra flake | artifact-service 403, runner failures | rerun; do not code around | ## Test-change doctrine (per-assertion) From f95d1300c08a4833938f285849f8e76b8f131778 Mon Sep 17 00:00:00 2001 From: "vashbrightfire[bot]" Date: Tue, 8 Sep 2026 12:27:14 -0500 Subject: [PATCH 3/5] docs: reflow brightfire-ci.md prose to 180-col wrap --- docs/brightfire-ci.md | 107 +++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 70 deletions(-) diff --git a/docs/brightfire-ci.md b/docs/brightfire-ci.md index a22d3d3ae04b3..294d6a52e593c 100644 --- a/docs/brightfire-ci.md +++ b/docs/brightfire-ci.md @@ -1,98 +1,65 @@ # Brightfire CI Guide -How CI works on the Brightfire fork, how to read a red PR, and where test -changes belong. +How CI works on the Brightfire fork, how to read a red PR, and where test changes belong. ## The two questions CI answers A patch PR gets two kinds of signal: -1. **"Is my patch clean on its own?"** — upstream CI runs against the raw - PR tree (base + this patch only). Catches patch-local hygiene: lint, - knip, max-lines, formatting. Runs on every PR; failures here are - usually about the patch itself. -2. **"Will the integration work?"** — `bf-pr-context.yml` assembles the - representative tree (upstream base + ALL manifest patches at their - recorded SHAs + the PR head — exactly what `bf-build-stable` would - build on merge) and runs the upstream test suite against that. A - failure here means stable will fail if this merges. - -The raw-tree checks will show failures from _missing patch context_ (see -taxonomy below) — those are noise on question 1 and are the reason -question 2 exists. The PR-context summary gates only on **new** failures -relative to the ledger. +1. **"Is my patch clean on its own?"** — upstream CI runs against the raw PR tree (base + this patch only). Catches patch-local hygiene: lint, knip, max-lines, formatting. Runs on + every PR; failures here are usually about the patch itself. +2. **"Will the integration work?"** — `bf-pr-context.yml` assembles the representative tree (upstream base + ALL manifest patches at their recorded SHAs + the PR head — exactly + what `bf-build-stable` would build on merge) and runs the upstream test suite against that. A failure here means stable will fail if this merges. + +The raw-tree checks will show failures from _missing patch context_ (see taxonomy below) — those are noise on question 1 and are the reason question 2 exists. The PR-context +summary gates only on **new** failures relative to the ledger. ## Failure taxonomy Every red check is one of: -| Class | Example | Where the fix goes | -| ------------------------ | ------------------------------------------------------ | ---------------------------------------------------------------------- | -| PR's own change | a test your patch actually breaks; a guard it violates | the PR itself | -| Patch contract change | upstream test asserts the old contract the patch flips | the owning patch, per the doctrine below | -| Pre-existing at base | fails on the clean upstream tree too | `upstream-test-fixes` (skip, with clean-base evidence) | -| CI-environment-sensitive | passes locally, fails on GH-hosted runners | `upstream-test-fixes` (skip, with CI-run evidence) | -| Missing patch context | fails only on raw-tree PR CI — other patches missing | nowhere — PR-context CI suppresses it; do not "fix" it per-PR | -| Infra flake | artifact-service 403, runner failures | rerun; do not code around | +| Class | Example | Where the fix goes | +| ------------------------ | ------------------------------------------------------ | ------------------------------------------------------------- | +| PR's own change | a test your patch actually breaks; a guard it violates | the PR itself | +| Patch contract change | upstream test asserts the old contract the patch flips | the owning patch, per the doctrine below | +| Pre-existing at base | fails on the clean upstream tree too | `upstream-test-fixes` (skip, with clean-base evidence) | +| CI-environment-sensitive | passes locally, fails on GH-hosted runners | `upstream-test-fixes` (skip, with CI-run evidence) | +| Missing patch context | fails only on raw-tree PR CI — other patches missing | nowhere — PR-context CI suppresses it; do not "fix" it per-PR | +| Infra flake | artifact-service 403, runner failures | rerun; do not code around | ## Test-change doctrine (per-assertion) -When a patch deliberately changes a contract (e.g. bundle-all-plugins ships -all plugins in the tarball), evaluate failing tests **per assertion**, not -per file: - -1. The whole case asserts the old contract (externality/exclusion) → - invalid on the patched tree → skip the case (or delete it if the patch - erroneously carried it). -2. The old contract is one condition inside a broader valid test → - surgically remove that condition; keep the test active. -3. The test asserts presence/correctness of what the patch now ships → - update its expectations to the patched tree's truth; the test now - **enforces** the new contract. +When a patch deliberately changes a contract (e.g. bundle-all-plugins ships all plugins in the tarball), evaluate failing tests **per assertion**, not per file: + +1. The whole case asserts the old contract (externality/exclusion) → invalid on the patched tree → skip the case (or delete it if the patch erroneously carried it). +2. The old contract is one condition inside a broader valid test → surgically remove that condition; keep the test active. +3. The test asserts presence/correctness of what the patch now ships → update its expectations to the patched tree's truth; the test now **enforces** the new contract. 4. Ambiguous → skip, and say why in the resolution note. -**Skip placement:** env/pre-existing failures go in `upstream-test-fixes` -(applies first, so every later tree inherits the green baseline). Contract -changes go in the patch that changes the contract. CI infrastructure goes -on `brightfire/ci`. Never maintain the same skip in two places — if you -find yourself copying a skip between the patch stack and a branch, the -assembly step is missing (see PR-context CI). +**Skip placement:** env/pre-existing failures go in `upstream-test-fixes` (applies first, so every later tree inherits the green baseline). Contract changes go in the patch that +changes the contract. CI infrastructure goes on `brightfire/ci`. Never maintain the same skip in two places — if you find yourself copying a skip between the patch stack and a +branch, the assembly step is missing (see PR-context CI). -**Before skipping anything because "the patch changed the contract":** -check what the test actually pins. A test that fails because of a patch -can still be flagging a real bug **in** the patch (example: the -`cli-http-fallback` probe test caught the /ready fallback masking -explicitly rejected tokens — the fix was gating the patch, not skipping -the test). +**Before skipping anything because "the patch changed the contract":** check what the test actually pins. A test that fails because of a patch can still be flagging a real bug +**in** the patch (example: the `cli-http-fallback` probe test caught the /ready fallback masking explicitly rejected tokens — the fix was gating the patch, not skipping the test). ## Triage playbook -1. **Never reason from absolute red/green.** Diff the PR's failing checks - against the base branch's last CI run, or against the previous head's - run. New-vs-known is the only signal. -2. For each new failing test, classify it against the taxonomy above. - Reproduce on the clean base before calling anything "pre-existing". -3. Route the fix per the table. Patch PRs should not accumulate skips for - classes that PR-context CI already covers. +1. **Never reason from absolute red/green.** Diff the PR's failing checks against the base branch's last CI run, or against the previous head's run. New-vs-known is the only + signal. +2. For each new failing test, classify it against the taxonomy above. Reproduce on the clean base before calling anything "pre-existing". +3. Route the fix per the table. Patch PRs should not accumulate skips for classes that PR-context CI already covers. ## The expected-failures ledger -`.github/brightfire-ci/expected-test-failures.json` lists, per pinned -base commit, the test files expected to fail on the **assembled** tree. -PR-context CI gates only on failing files not in the ledger. Rules: +`.github/brightfire-ci/expected-test-failures.json` lists, per pinned base commit, the test files expected to fail on the **assembled** tree. PR-context CI gates only on failing +files not in the ledger. Rules: -- Add an entry only with evidence that the failure exists on the - assembled tree independent of the PR under review. -- Remove entries as they are fixed — the list must trend to zero. -- Entries are file-level for now; case-level granularity can be added - when file-level proves too coarse. +- Add an entry only with evidence that the failure exists on the assembled tree independent of the PR under review. +- Remove entries as they are fixed — the list must trend to zero. - Entries are file-level for now; case-level granularity can be added when file-level proves too coarse. ## Merge order and intermediate red builds -`bf-register-patch` commits manifest bumps to `brightfire/ci` on every -patch-PR merge, and those commits touch `BRIGHTFIRE_PATCHES.md` — which -triggers `bf-build-stable`. Multi-PR sets therefore produce **intermediate -red build-stable runs** while the set is landing. That is expected: use -those runs as incremental verification of what has landed so far, and -expect the final PR in the set (usually the `brightfire/ci` one) to -trigger the first fully green run. +`bf-register-patch` commits manifest bumps to `brightfire/ci` on every patch-PR merge, and those commits touch `BRIGHTFIRE_PATCHES.md` — which triggers `bf-build-stable`. Multi-PR +sets therefore produce **intermediate red build-stable runs** while the set is landing. That is expected: use those runs as incremental verification of what has landed so far, and +expect the final PR in the set (usually the `brightfire/ci` one) to trigger the first fully green run. From 5939ada73716b73ea2069471a30feb054b45e27e Mon Sep 17 00:00:00 2001 From: "vashbrightfire[bot]" Date: Tue, 8 Sep 2026 12:27:54 -0500 Subject: [PATCH 4/5] =?UTF-8?q?ci:=20drop=20expected-failures=20ledger=20?= =?UTF-8?q?=E2=80=94=20green=20means=20green?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../brightfire-ci/expected-test-failures.json | 4 -- .github/workflows/bf-pr-context.yml | 61 +++---------------- 2 files changed, 10 insertions(+), 55 deletions(-) delete mode 100644 .github/brightfire-ci/expected-test-failures.json diff --git a/.github/brightfire-ci/expected-test-failures.json b/.github/brightfire-ci/expected-test-failures.json deleted file mode 100644 index 15a7716bb8873..0000000000000 --- a/.github/brightfire-ci/expected-test-failures.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "$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/ — 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": [] -} diff --git a/.github/workflows/bf-pr-context.yml b/.github/workflows/bf-pr-context.yml index 7d119457a761c..f797cfd5cfd24 100644 --- a/.github/workflows/bf-pr-context.yml +++ b/.github/workflows/bf-pr-context.yml @@ -13,9 +13,8 @@ name: "BF: PR Context CI" # # Raw-tree upstream CI still runs on patch PRs and catches patch-local # hygiene (lint, knip, max-lines). This workflow answers the integration -# question. Failures are compared against the expected-failures ledger -# (.github/brightfire-ci/expected-test-failures.json); only NEW failures -# gate. See docs/brightfire-ci.md. +# question. Green means green: any failing test on the assembled tree +# gates the PR. Nothing is ledgered or exempted. See docs/brightfire-ci.md. on: pull_request: @@ -185,11 +184,11 @@ jobs: test_matrix: ${{ needs.assemble.outputs.test_shards }} # ======================================================================== - # Summary: compare failing test FILES against the expected-failures - # ledger. Only NEW failing files gate; ledger entries are informational. + # Summary: any failing test on the assembled tree gates the PR. Green + # means green — no ledger, no exemptions. # ======================================================================== summary: - name: Compare failures with expected ledger + name: Report assembled-tree result needs: [assemble, test] if: always() && needs.assemble.result == 'success' runs-on: ubuntu-24.04 @@ -197,14 +196,9 @@ jobs: env: GITHUB_TOKEN: ${{ github.token }} steps: - - name: Checkout brightfire/ci - uses: actions/checkout@v5 - - name: Collect failing test files id: failures run: | - BASE_COMMIT=$(python3 scripts/bf/parse-patches.py BRIGHTFIRE_PATCHES.md /dev/stdout | grep -oP '(?<=base_commit=).*' | head -1) - 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 @@ -218,56 +212,21 @@ jobs: echo "actual_count=$ACTUAL_COUNT" >> "$GITHUB_OUTPUT" cat /tmp/prctx/actual-failures.txt - - name: Diff against ledger - id: ledger - 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' - import json, sys - ledger_path, base, actual_path = sys.argv[1], sys.argv[2], sys.argv[3] - ledger = json.load(open(ledger_path)) - known = set(ledger.get(base, [])) - try: - actual = set(line.strip() for line in open(actual_path) if line.strip()) - except FileNotFoundError: - actual = set() - new = sorted(actual - known) - stale = sorted(known - actual) - json.dump({"new": new, "stale": stale, "known_count": len(known & actual)}, open("/tmp/prctx/result.json", "w")) - print(f"new={len(new)}") - EOF - NEW_COUNT=$(python3 -c "import json; print(len(json.load(open('/tmp/prctx/result.json'))['new']))") - echo "new_count=$NEW_COUNT" >> "$GITHUB_OUTPUT" - if [ "$NEW_COUNT" -gt 0 ]; then - echo "new_failures=true" >> "$GITHUB_OUTPUT" - else - echo "new_failures=false" >> "$GITHUB_OUTPUT" - fi - - name: Comment on PR run: | PR="${{ github.event.pull_request.number }}" - NEW_COUNT="${{ steps.ledger.outputs.new_count }}" ACTUAL_COUNT="${{ steps.failures.outputs.actual_count }}" - NEW_LIST=$(python3 -c "import json; print('\n'.join('- '+f for f in json.load(open('/tmp/prctx/result.json'))['new']))") - STALE_LIST=$(python3 -c "import json; print('\n'.join('- '+f for f in json.load(open('/tmp/prctx/result.json'))['stale'][:10]))") - if [ "$NEW_COUNT" -gt 0 ]; then + if [ "$ACTUAL_COUNT" -gt 0 ]; then + FAIL_LIST=$(sed 's/^/- /' /tmp/prctx/actual-failures.txt) gh pr comment "$PR" --body "$(cat < Date: Tue, 8 Sep 2026 12:30:27 -0500 Subject: [PATCH 5/5] =?UTF-8?q?docs:=20remove=20expected-failures=20ledger?= =?UTF-8?q?=20section=20=E2=80=94=20green=20means=20green?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/brightfire-ci.md | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/docs/brightfire-ci.md b/docs/brightfire-ci.md index 294d6a52e593c..64a3c1e779945 100644 --- a/docs/brightfire-ci.md +++ b/docs/brightfire-ci.md @@ -12,7 +12,8 @@ A patch PR gets two kinds of signal: what `bf-build-stable` would build on merge) and runs the upstream test suite against that. A failure here means stable will fail if this merges. The raw-tree checks will show failures from _missing patch context_ (see taxonomy below) — those are noise on question 1 and are the reason question 2 exists. The PR-context -summary gates only on **new** failures relative to the ledger. +summary gates on any failing test: green means green, with no known-failure exemption list. A failure that exists independent of the PR under review gets fixed or +skipped at the proper layer (see taxonomy), never bookkept. ## Failure taxonomy @@ -50,14 +51,6 @@ branch, the assembly step is missing (see PR-context CI). 2. For each new failing test, classify it against the taxonomy above. Reproduce on the clean base before calling anything "pre-existing". 3. Route the fix per the table. Patch PRs should not accumulate skips for classes that PR-context CI already covers. -## The expected-failures ledger - -`.github/brightfire-ci/expected-test-failures.json` lists, per pinned base commit, the test files expected to fail on the **assembled** tree. PR-context CI gates only on failing -files not in the ledger. Rules: - -- Add an entry only with evidence that the failure exists on the assembled tree independent of the PR under review. -- Remove entries as they are fixed — the list must trend to zero. - Entries are file-level for now; case-level granularity can be added when file-level proves too coarse. - ## Merge order and intermediate red builds `bf-register-patch` commits manifest bumps to `brightfire/ci` on every patch-PR merge, and those commits touch `BRIGHTFIRE_PATCHES.md` — which triggers `bf-build-stable`. Multi-PR