From 7d9d410f8a8eb82268ec89d170ee5d13a70f696f Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:00:08 +0300 Subject: [PATCH 1/7] docs(planning): spec the benchmark PR-comment surfacing Design for posting the existing bench-check sweep table as a sticky PR comment (and run Summary) via a new `benchmarks check --markdown` mode and a permission-scoped benchmarks.yml. Gate and baseline unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-07-17.03-benchmark-pr-comment.md | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 planning/changes/2026-07-17.03-benchmark-pr-comment.md diff --git a/planning/changes/2026-07-17.03-benchmark-pr-comment.md b/planning/changes/2026-07-17.03-benchmark-pr-comment.md new file mode 100644 index 0000000..dd07487 --- /dev/null +++ b/planning/changes/2026-07-17.03-benchmark-pr-comment.md @@ -0,0 +1,155 @@ +--- +summary: Post the existing `bench-check` sweep table as a sticky PR comment (and run Summary). A new `benchmarks check --markdown` mode owns the comment body; a scoped `benchmarks.yml` workflow captures it via stdout and posts with `gh pr comment`. The counter gate is unchanged; the bench job moves out of `_checks.yml` so `pull-requests: write` stays scoped. +--- + +# Design: Surface the benchmark gate as a PR comment + +## Summary + +The benchmark gate (`just bench-check`) already computes a per-message sweep +table on every PR, but its output dies in the CI log where no one reads it. This +change surfaces that table as a **sticky PR comment** and on the run's Summary +page, without touching the gate itself. Python gains a `benchmarks check +--markdown` mode that renders the table plus a pass/fail verdict as a Markdown +document; a new, permission-scoped `benchmarks.yml` workflow captures that +document from stdout and posts it with `gh pr comment`. No third-party actions, +no new metrics, no timing trend. + +## Motivation + +The prompt was "modern-di comments its benchmark results on PRs — can we do the +same?" modern-di's mechanism +(`benchmark-action/github-action-benchmark@v1`) does **not** transfer: it speaks +pytest-benchmark *timing* JSON, keeps a weak cached-from-`main` baseline, and +alerts on a 150% wall-clock threshold — exactly the timing-gate model +[the harness design](2026-07-14.01-benchmark-harness.md) rejected as "a flake +generator." Our gate is the stronger design: deterministic Postgres counters +against a committed, reviewed `baseline.json`. + +So only the **PR comment** is worth copying, not the machinery behind it. Today +`bench-check`'s `format_table` output — the sweep a reviewer would actually want +when weighing a perf change — is visible only to whoever expands the CI log. + +## Design + +### `benchmarks check --markdown` + +`report.py` gains `format_markdown(results, failures)` beside the existing +`format_table`. `__main__.py`'s `check` command takes a `--markdown` flag; when +set, `_check` writes a Markdown document to stdout **instead of** the plain +table, and returns the **same** exit code (0 pass / 1 fail) — the gate is +untouched. The document: + +- an `## Benchmark gate` heading; +- a verdict line — `✅ gate passed` or `❌ gate FAILED`, followed by the failure + bullets `compare()` already produces; +- the sweep reflowed as a GitHub Markdown table (same columns as `format_table`); +- a condensed gated-vs-informational footnote. + +Python owns the whole comment body, so it is unit-testable alongside the existing +`format_table` test and the YAML never builds strings. + +### `benchmarks.yml` + +A new top-level workflow — **not** an edit to `_checks.yml`. `_checks.yml` is a +reusable workflow `uses:`-called from `ci.yml`; granting it `pull-requests: +write` would widen the token for every job in that file (lint, docs, pytest). A +separate workflow scopes the write token to the one job that needs it (this is +why modern-di is structured the same way). + +```yaml +name: benchmarks +on: + push: { branches: [main] } + pull_request: {} +permissions: + contents: read + pull-requests: write +jobs: + bench: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v6 + - uses: extractions/setup-just@v4 + - uses: astral-sh/setup-uv@v8.2.0 + - run: uv lock # uv.lock is git-ignored; compose build needs it + - name: Run gate, capture markdown + shell: bash # -eo pipefail: tee must not mask the gate's exit + run: just bench-report | tee "$GITHUB_STEP_SUMMARY" + - name: Comment on PR + if: always() && github.event_name == 'pull_request' + continue-on-error: true # fork PRs get a read-only token; degrade quietly + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh pr comment "${{ github.event.pull_request.number }}" \ + --edit-last --create-if-none \ + --body-file "$GITHUB_STEP_SUMMARY" +``` + +The `bench` job is **deleted from `_checks.yml`** and lands here (its `uv lock` +step and 20-minute timeout carry over verbatim) so it does not run twice. `main` +is unprotected with no required status checks, so moving the job breaks nothing. + +`tee "$GITHUB_STEP_SUMMARY"` puts the table on the run's Summary page for free — +readable even on `push` and fork PRs where no comment posts — and the same file +feeds `--body-file`. `--edit-last --create-if-none` makes the comment sticky +(all three flags verified present in the runner's preinstalled gh 2.96.0). + +### `bench-report` recipe + +``` +# Gate + emit the markdown report to stdout (CI PR comment). -T keeps stdout clean. +bench-report: down && down + docker compose run -T application uv run python -m benchmarks check --markdown +``` + +`-T` disables the pseudo-TTY, so the container's stdout (the Markdown doc, and +only that in `--markdown` mode) forwards straight to host stdout, while compose +chatter, `just`'s recipe-echo, and the `down` steps all go to stderr — the `| +tee` pipe captures pure Markdown. The exit-code chain that keeps the gate biting: +`benchmarks check` returns 1 → `docker compose run` → `just` → `pipefail` makes +`| tee` return 1 → the step fails. `bench-check` stays as-is for local dev; +`bench-report` is the CI entry point. + +**Capture, not a written file.** An earlier idea wrote the Markdown to a file in +the `.:/code` bind mount. Rejected: the Dockerfile runs as `USER runner`, whose +uid differs from the Linux runner's workspace owner, so that write can hit +permission-denied in CI (it works on macOS only because Docker Desktop's FUSE +mount ignores uid/gid). stdout capture sidesteps it entirely. + +## Non-goals + +- **No `benchmark-action` / timing trend.** Wall-clock stays ungated; the design + doc's reasoning holds. +- **No new metrics or gate change.** Same `EXACT_KEYS` / `TOLERANT_KEYS`, same + `baseline.json`, same exit codes. +- **No `pull_request_target`.** It would let fork PRs comment but runs untrusted + PR code with a write token. With 0 forks and 40/40 same-repo PRs, the fork case + is theoretical; the comment step `continue-on-error`s and the Summary page + still shows the table. + +## Testing + +- **Unit** (`tests/test_benchmarks.py`, no Postgres): `format_markdown` renders a + passing run with `✅` and a GitHub table; a failing run renders `❌` plus the + failure bullets. Beside the existing `test_format_table_*`. +- **Local integration**: `just bench-report` prints clean Markdown to stdout and + exits 0 against the committed baseline. +- **Gate still bites**: reuse the harness doc's method (extra `SELECT 1` per + message) → `bench-report` exits non-zero and the Markdown carries `❌`; revert. +- `--cov-fail-under=100` undisturbed: `benchmarks/*` stays in `[tool.coverage.run] + omit`; new tests import the pure functions like the existing ones. +- **Not verifiable locally**: the live comment post (no fork; fires only on a real + PR). The first PR after merge validates the comment step end to end. + +## Risk + +- **`tee` masking the gate exit** (medium × high) — Actions' default `bash -e` + returns tee's status, silently disabling the gate. Mitigated by `shell: bash` + (`-eo pipefail`) and asserted by the gate-still-bites test. +- **TTY chatter polluting the captured body** (low × medium) — mitigated by `-T` + and `--markdown` writing only the doc to stdout. +- **Comment step failing the build on fork PRs** (low × low today) — mitigated by + `continue-on-error` + the Summary-page fallback. From b417b33469f1904730154eb38e02dceb005c7ef2 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:15:26 +0300 Subject: [PATCH 2/7] feat(benchmarks): markdown output mode for the gate `benchmarks check --markdown` renders the sweep table plus a pass/fail verdict as a GitHub Markdown document on stdout, so CI can post it as a PR comment. Gate logic and exit codes unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- benchmarks/__main__.py | 13 +++++++++---- benchmarks/report.py | 31 +++++++++++++++++++++++++++++++ tests/test_benchmarks.py | 21 ++++++++++++++++++++- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/benchmarks/__main__.py b/benchmarks/__main__.py index 7e5483c..7c67b8e 100644 --- a/benchmarks/__main__.py +++ b/benchmarks/__main__.py @@ -12,7 +12,7 @@ import sys from benchmarks.config import DEFAULT_DSN, RunConfig -from benchmarks.report import compare, format_table, to_baseline +from benchmarks.report import compare, format_markdown, format_table, to_baseline from benchmarks.workload import RunResult, make_engine, run_consumer, run_producer @@ -66,7 +66,7 @@ async def _run_sweep(dsn: str, messages: int, repeats: int) -> list[RunResult]: return results -def _check(dsn: str) -> int: +def _check(dsn: str, *, markdown: bool = False) -> int: """Run the sweep at the baseline's message count and gate the counters.""" if not BASELINE_PATH.exists(): sys.stdout.write(f"no baseline at {BASELINE_PATH}; run `just bench --write-baseline` first\n") @@ -77,8 +77,12 @@ def _check(dsn: str) -> int: # so repeats=1 -- repeating them only burns CI time. messages = int(next(iter(baseline["runs"].values()))["messages"]) results = asyncio.run(_run_sweep(dsn, messages, repeats=1)) - sys.stdout.write(format_table(results) + "\n") failures = compare(to_baseline(results), baseline) + if markdown: + # Python owns the whole PR-comment body; the workflow just posts stdout. + sys.stdout.write(format_markdown(results, failures) + "\n") + return 1 if failures else 0 + sys.stdout.write(format_table(results) + "\n") if failures: sys.stdout.write("\nBENCHMARK GATE FAILED:\n") for failure in failures: @@ -106,12 +110,13 @@ def main() -> int: # uses 1: the gated counters are deterministic, so repeating them only burns CI time. parser.add_argument("--repeats", type=int, default=3) parser.add_argument("--write-baseline", action="store_true") + parser.add_argument("--markdown", action="store_true") args = parser.parse_args() dsn = os.environ.get("POSTGRES_DSN", DEFAULT_DSN) if args.command == "check": - return _check(dsn) + return _check(dsn, markdown=args.markdown) return _run(dsn, args.messages, args.repeats, write_baseline=args.write_baseline) diff --git a/benchmarks/report.py b/benchmarks/report.py index 073f055..ee683e0 100644 --- a/benchmarks/report.py +++ b/benchmarks/report.py @@ -127,6 +127,37 @@ def format_table(results: list[RunResult]) -> str: return "\n".join(lines) +def format_markdown(results: list[RunResult], failures: list[str]) -> str: + """Render the sweep as a GitHub-flavored Markdown comment body. + + Heading + verdict + table + a condensed gated-vs-informational footnote. + ``failures`` is ``compare()``'s output: empty means the gate passed. This + function only formats -- the exit code is the caller's, so the comment can be + posted on both pass and fail. + """ + verdict = "✅ gate passed" if not failures else "❌ gate FAILED" + lines = ["## Benchmark gate", "", verdict, ""] + if failures: + lines.extend(f"- {failure}" for failure in failures) + lines.append("") + lines.append("| scenario | msg/s | delete/msg | WALrec/msg | WALB/msg | fpi | upd | del | dead_tup |") + lines.append("| --- | --: | --: | --: | --: | --: | --: | --: | --: |") + for r in results: + m = normalize(r) + lines.append( + f"| {_key(r)} | {m['msgs_per_second']:.0f} | {m['delete_calls_per_msg']:.3f} " + f"| {m['wal_records_per_msg']:.2f} | {m['wal_bytes_per_msg']:.0f} | {m['wal_fpi']:.0f} " + f"| {m['tup_upd']:.0f} | {m['tup_del']:.0f} | {m['dead_tup']:.0f} |", + ) + lines.append("") + lines.append( + "_Gated (fails the build): `delete_calls` + tuple counters (upd/del/ins) + the " + "producer's `insert_calls`/`select_calls`, exact; `wal_records` within a 10% band. " + "msg/s, WAL bytes and total calls are informational (timing/FPI noise)._", + ) + return "\n".join(lines) + + def compare(current: dict[str, typing.Any], baseline: dict[str, typing.Any]) -> list[str]: """Diff current raw totals against the baseline. Empty list means pass. diff --git a/tests/test_benchmarks.py b/tests/test_benchmarks.py index 9cfb2d8..73a3b08 100644 --- a/tests/test_benchmarks.py +++ b/tests/test_benchmarks.py @@ -11,7 +11,7 @@ from benchmarks.config import RunConfig from benchmarks.probes import ProbeResult -from benchmarks.report import EXACT_KEYS, compare, format_table, normalize, to_baseline +from benchmarks.report import EXACT_KEYS, compare, format_markdown, format_table, normalize, to_baseline from benchmarks.workload import RunResult @@ -193,3 +193,22 @@ def test_format_table_labels_gated_vs_informational() -> None: assert "INFORMATIONAL" in table assert "wal_records" in table assert "delete_calls" in table + + +def test_format_markdown_renders_passing_verdict_and_table() -> None: + body = format_markdown([_result(), _producer()], []) + assert body.startswith("## Benchmark gate") + assert "✅ gate passed" in body + # Both runs render as rows. + assert "consumer/w1/b100" in body + assert "producer/w1/b100" in body + # GitHub table shape: a header row and a delimiter row. + assert "| scenario |" in body + assert "| --- |" in body + + +def test_format_markdown_renders_failing_verdict_and_bullets() -> None: + failures = ["consumer/w1/b100: delete_calls changed (exact-gated): baseline 500 -> current 501"] + body = format_markdown([_result()], failures) + assert "❌ gate FAILED" in body + assert "- consumer/w1/b100: delete_calls changed" in body From 4d0e8098927783309ecf1db8e582e65078eddf98 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:23:21 +0300 Subject: [PATCH 3/7] ci(benchmarks): post the gate table as a sticky PR comment Move the bench job into its own permission-scoped benchmarks.yml, run it via `just bench-report`, tee the markdown to the run Summary, and post it with `gh pr comment --edit-last --create-if-none`. Scopes `pull-requests: write` to the one job instead of widening the reusable _checks.yml token. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/_checks.yml | 14 ----------- .github/workflows/benchmarks.yml | 40 ++++++++++++++++++++++++++++++++ Justfile | 4 ++++ 3 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 .github/workflows/benchmarks.yml diff --git a/.github/workflows/_checks.yml b/.github/workflows/_checks.yml index 536972f..12a3f71 100644 --- a/.github/workflows/_checks.yml +++ b/.github/workflows/_checks.yml @@ -25,20 +25,6 @@ jobs: - uses: astral-sh/setup-uv@v8.2.0 - run: just docs-build - bench: - runs-on: ubuntu-latest - timeout-minutes: 20 - steps: - - uses: actions/checkout@v6 - - uses: extractions/setup-just@v4 - - uses: astral-sh/setup-uv@v8.2.0 - # uv.lock is git-ignored (repo convention), so a clean checkout lacks it. - # `just bench-check` builds the compose `application` image, whose Dockerfile - # does `COPY uv.lock` + `uv sync --frozen`; regenerate the lock first so that - # build has it. (The pytest job runs `uv sync` directly and needs no compose.) - - run: uv lock - - run: just bench-check - pytest: runs-on: ubuntu-latest strategy: diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml new file mode 100644 index 0000000..7151509 --- /dev/null +++ b/.github/workflows/benchmarks.yml @@ -0,0 +1,40 @@ +name: benchmarks + +on: + push: + branches: [main] + pull_request: {} + +permissions: + contents: read + pull-requests: write + +jobs: + bench: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v6 + - uses: extractions/setup-just@v4 + - uses: astral-sh/setup-uv@v8.2.0 + # uv.lock is git-ignored (repo convention), so a clean checkout lacks it. + # `just bench-report` builds the compose `application` image, whose Dockerfile + # does `COPY uv.lock` + `uv sync --frozen`; regenerate the lock first. + - run: uv lock + - name: Run the gate and capture the markdown report + # `shell: bash` gives -eo pipefail so the `| tee` cannot mask the gate's + # non-zero exit -- without pipefail the step would report tee's success. + shell: bash + run: just bench-report | tee "$GITHUB_STEP_SUMMARY" + - name: Comment the report on the PR + # always(): comment even when the gate failed -- that is when it matters most. + # continue-on-error: fork PRs get a read-only token and cannot comment; the + # gate above already decided the build, and the run Summary still has the table. + if: always() && github.event_name == 'pull_request' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh pr comment "${{ github.event.pull_request.number }}" \ + --edit-last --create-if-none \ + --body-file "$GITHUB_STEP_SUMMARY" diff --git a/Justfile b/Justfile index 7c1272b..1653e3a 100644 --- a/Justfile +++ b/Justfile @@ -52,6 +52,10 @@ bench *args: down && down bench-check: down && down docker compose run application uv run python -m benchmarks check +# Gate + emit the markdown report to stdout (CI PR comment). -T keeps stdout clean. +bench-report: down && down + docker compose run -T application uv run python -m benchmarks check --markdown + # Serve docs at http://127.0.0.1:8000 with hot-reload on save. docs-serve: uvx --with-requirements docs/requirements.txt mkdocs serve From 4b1f5c7b214b04b4fbcc24a6cf6ab0fa078586d4 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:43:58 +0300 Subject: [PATCH 4/7] fix(ci): post the captured report file, not the per-step summary GITHUB_STEP_SUMMARY is unique per step, so the comment step read its own empty summary and gh pr comment silently failed on a blank body. Tee the report to a RUNNER_TEMP scratch file that survives across steps and post that. Add a concurrency group so rapid PR pushes don't race the sticky comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/benchmarks.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 7151509..9966860 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -9,6 +9,10 @@ permissions: contents: read pull-requests: write +concurrency: + group: benchmarks-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + jobs: bench: runs-on: ubuntu-latest @@ -25,7 +29,7 @@ jobs: # `shell: bash` gives -eo pipefail so the `| tee` cannot mask the gate's # non-zero exit -- without pipefail the step would report tee's success. shell: bash - run: just bench-report | tee "$GITHUB_STEP_SUMMARY" + run: just bench-report | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" - name: Comment the report on the PR # always(): comment even when the gate failed -- that is when it matters most. # continue-on-error: fork PRs get a read-only token and cannot comment; the @@ -37,4 +41,4 @@ jobs: run: | gh pr comment "${{ github.event.pull_request.number }}" \ --edit-last --create-if-none \ - --body-file "$GITHUB_STEP_SUMMARY" + --body-file "$RUNNER_TEMP/bench-report.md" From df85e806b7bcae3abb6464cfcfc325765212f40c Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:45:56 +0300 Subject: [PATCH 5/7] docs(planning): correct the PR-comment file handoff + note scheduled gap GITHUB_STEP_SUMMARY is per-step, so the comment must read a RUNNER_TEMP scratch file the capture step also tees to, not the summary path. Record accepting the lost weekly scheduled counter gate as the cost of the scoped write token. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-07-17.03-benchmark-pr-comment.md | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/planning/changes/2026-07-17.03-benchmark-pr-comment.md b/planning/changes/2026-07-17.03-benchmark-pr-comment.md index dd07487..3654ea4 100644 --- a/planning/changes/2026-07-17.03-benchmark-pr-comment.md +++ b/planning/changes/2026-07-17.03-benchmark-pr-comment.md @@ -76,7 +76,7 @@ jobs: - run: uv lock # uv.lock is git-ignored; compose build needs it - name: Run gate, capture markdown shell: bash # -eo pipefail: tee must not mask the gate's exit - run: just bench-report | tee "$GITHUB_STEP_SUMMARY" + run: just bench-report | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" - name: Comment on PR if: always() && github.event_name == 'pull_request' continue-on-error: true # fork PRs get a read-only token; degrade quietly @@ -85,17 +85,25 @@ jobs: run: | gh pr comment "${{ github.event.pull_request.number }}" \ --edit-last --create-if-none \ - --body-file "$GITHUB_STEP_SUMMARY" + --body-file "$RUNNER_TEMP/bench-report.md" ``` The `bench` job is **deleted from `_checks.yml`** and lands here (its `uv lock` step and 20-minute timeout carry over verbatim) so it does not run twice. `main` is unprotected with no required status checks, so moving the job breaks nothing. -`tee "$GITHUB_STEP_SUMMARY"` puts the table on the run's Summary page for free — -readable even on `push` and fork PRs where no comment posts — and the same file -feeds `--body-file`. `--edit-last --create-if-none` makes the comment sticky -(all three flags verified present in the runner's preinstalled gh 2.96.0). +`tee` fans the report to **two** sinks: `$GITHUB_STEP_SUMMARY` puts the table on +the run's Summary page for free (readable even on `push` and fork PRs where no +comment posts), and `$RUNNER_TEMP/bench-report.md` is the file the comment step +reads. The scratch file is load-bearing: `GITHUB_STEP_SUMMARY` is a **different +path in every step**, so a comment step reading it would find its own empty +summary and `gh pr comment` would fail on a blank body (silently, under +`continue-on-error`). `RUNNER_TEMP` persists across steps within the job and sits +outside the checked-out tree, so it leaves no stray workspace file. +`--edit-last --create-if-none` makes the comment sticky (all flags verified +present in the runner's preinstalled gh 2.96.0). A top-level `concurrency` group +(`benchmarks-${{ github.head_ref || github.run_id }}`, `cancel-in-progress`) +stops rapid PR pushes from racing two `--edit-last` posts. ### `bench-report` recipe @@ -129,6 +137,13 @@ mount ignores uid/gid). stdout capture sidesteps it entirely. PR code with a write token. With 0 forks and 40/40 same-repo PRs, the fork case is theoretical; the comment step `continue-on-error`s and the Summary page still shows the table. +- **No weekly scheduled run.** `benchmarks.yml` triggers only on `push`-to-main + and `pull_request`, not on `scheduled.yml`'s Monday cron. Moving `bench` out of + the reusable `_checks.yml` (to scope the write token) means the weekly + dependency-drift check no longer runs the counter gate — a dep bump that + regresses the counters is caught on the next PR, not on Monday. Accepted + deliberately as the cost of the scoped token; re-add a `schedule` trigger here + if drift regressions start slipping through. ## Testing From 4bf36ca63140535e5d289bc91df01fbbc797833b Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:54:37 +0300 Subject: [PATCH 6/7] fix(ci): slice the report from its sentinel, not raw compose stdout `docker compose run` leaks buildkit image-build output onto stdout in CI, so the first sticky comment was ~120 lines of build noise before the table. The report always starts with the `## Benchmark gate` heading and the container prints it last, so capture from that sentinel to EOF. Manage the exit code by hand (set +e, re-exit with the gate status) so a failing gate still posts its report yet still fails the build. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/benchmarks.yml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 9966860..c53034e 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -26,10 +26,21 @@ jobs: # does `COPY uv.lock` + `uv sync --frozen`; regenerate the lock first. - run: uv lock - name: Run the gate and capture the markdown report - # `shell: bash` gives -eo pipefail so the `| tee` cannot mask the gate's - # non-zero exit -- without pipefail the step would report tee's success. shell: bash - run: just bench-report | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + run: | + # `docker compose run` leaks buildkit image-build output onto stdout in + # CI, so a naive capture mixes it into the comment. The report always + # begins with the `## Benchmark gate` sentinel and the container prints + # it last, so slice from that heading to EOF -- independent of how + # compose routes its own noise. Own the exit code manually: `set +e` + # (shell: bash runs with -e) so a failing gate still lets us post the + # report, then re-exit with the gate's status so the build still fails. + set +e + just bench-report > "$RUNNER_TEMP/raw.log" + status=$? + sed -n '/^## Benchmark gate/,$p' "$RUNNER_TEMP/raw.log" \ + | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + exit "$status" - name: Comment the report on the PR # always(): comment even when the gate failed -- that is when it matters most. # continue-on-error: fork PRs get a read-only token and cannot comment; the From 24f8eb1ecff6fb4295df2d97f7ed9a762d4acb39 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Fri, 17 Jul 2026 18:57:27 +0300 Subject: [PATCH 7/7] docs(planning): record the compose-stdout-noise capture hazard The first live PR comment was buried under buildkit build output because `docker compose run` leaks it to stdout in CI. Document slicing the report from its `## Benchmark gate` sentinel and owning the exit code by hand, and correct the earlier claim that -T alone yields clean stdout. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-07-17.03-benchmark-pr-comment.md | 75 ++++++++++++------- 1 file changed, 50 insertions(+), 25 deletions(-) diff --git a/planning/changes/2026-07-17.03-benchmark-pr-comment.md b/planning/changes/2026-07-17.03-benchmark-pr-comment.md index 3654ea4..c1ec8f4 100644 --- a/planning/changes/2026-07-17.03-benchmark-pr-comment.md +++ b/planning/changes/2026-07-17.03-benchmark-pr-comment.md @@ -75,8 +75,14 @@ jobs: - uses: astral-sh/setup-uv@v8.2.0 - run: uv lock # uv.lock is git-ignored; compose build needs it - name: Run gate, capture markdown - shell: bash # -eo pipefail: tee must not mask the gate's exit - run: just bench-report | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + shell: bash + run: | + set +e # own the exit code; -e would skip the post + just bench-report > "$RUNNER_TEMP/raw.log" + status=$? # the gate's real result + sed -n '/^## Benchmark gate/,$p' "$RUNNER_TEMP/raw.log" \ + | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + exit "$status" # re-fail the build on drift - name: Comment on PR if: always() && github.event_name == 'pull_request' continue-on-error: true # fork PRs get a read-only token; degrade quietly @@ -92,14 +98,29 @@ The `bench` job is **deleted from `_checks.yml`** and lands here (its `uv lock` step and 20-minute timeout carry over verbatim) so it does not run twice. `main` is unprotected with no required status checks, so moving the job breaks nothing. -`tee` fans the report to **two** sinks: `$GITHUB_STEP_SUMMARY` puts the table on -the run's Summary page for free (readable even on `push` and fork PRs where no -comment posts), and `$RUNNER_TEMP/bench-report.md` is the file the comment step -reads. The scratch file is load-bearing: `GITHUB_STEP_SUMMARY` is a **different -path in every step**, so a comment step reading it would find its own empty -summary and `gh pr comment` would fail on a blank body (silently, under -`continue-on-error`). `RUNNER_TEMP` persists across steps within the job and sits -outside the checked-out tree, so it leaves no stray workspace file. +Two hazards, both hit on the first live PR and both handled above: + +1. **Compose leaks build noise onto stdout.** `-T` suppresses the TTY but does + **not** separate `docker compose run`'s buildkit image-build output, which in + CI lands on stdout — the first live comment was ~120 lines of `#8 extracting …` + before the table. So the capture does not trust stdout hygiene: it slices the + report from its `## Benchmark gate` sentinel to EOF. The container prints the + report last and nothing follows it on stdout (compose's post-`down` writes to + stderr), so heading-to-EOF is exactly the Markdown, independent of how compose + routes its own noise. +2. **`GITHUB_STEP_SUMMARY` is per-step.** It is a **different path in every + step**, so a comment step reading it directly would find its own empty summary + and `gh pr comment` would fail on a blank body (silently, under + `continue-on-error`). The sliced report is teed to two sinks: + `$GITHUB_STEP_SUMMARY` (the run's Summary page, free and visible even on `push` + and fork PRs where no comment posts) and `$RUNNER_TEMP/bench-report.md`, the + file the comment step reads. `RUNNER_TEMP` persists across steps within the job + and sits outside the checked-out tree, so it leaves no stray workspace file. + +The exit code is owned by hand — `set +e`, capture the gate's status from the +redirect, re-`exit` it after teeing — because piping through `sed`/`tee` would +otherwise mask a failing gate, and because a failing gate must still post its +`❌` report (the slice runs regardless) before failing the build. `--edit-last --create-if-none` makes the comment sticky (all flags verified present in the runner's preinstalled gh 2.96.0). A top-level `concurrency` group (`benchmarks-${{ github.head_ref || github.run_id }}`, `cancel-in-progress`) @@ -113,19 +134,19 @@ bench-report: down && down docker compose run -T application uv run python -m benchmarks check --markdown ``` -`-T` disables the pseudo-TTY, so the container's stdout (the Markdown doc, and -only that in `--markdown` mode) forwards straight to host stdout, while compose -chatter, `just`'s recipe-echo, and the `down` steps all go to stderr — the `| -tee` pipe captures pure Markdown. The exit-code chain that keeps the gate biting: -`benchmarks check` returns 1 → `docker compose run` → `just` → `pipefail` makes -`| tee` return 1 → the step fails. `bench-check` stays as-is for local dev; -`bench-report` is the CI entry point. +`-T` disables the pseudo-TTY so the Markdown carries no terminal control +characters. It does **not** by itself guarantee clean stdout — see hazard 1 +above — which is why the workflow slices the report rather than trusting the +recipe's stdout wholesale. `benchmarks check` still returns 1 on drift, and the +workflow re-exits that status, so the gate keeps biting. `bench-check` stays +as-is for local dev; `bench-report` is the CI entry point. **Capture, not a written file.** An earlier idea wrote the Markdown to a file in the `.:/code` bind mount. Rejected: the Dockerfile runs as `USER runner`, whose uid differs from the Linux runner's workspace owner, so that write can hit permission-denied in CI (it works on macOS only because Docker Desktop's FUSE -mount ignores uid/gid). stdout capture sidesteps it entirely. +mount ignores uid/gid). Capturing the container's stdout sidesteps the uid +problem; the sentinel slice then handles compose's stdout noise. ## Non-goals @@ -156,15 +177,19 @@ mount ignores uid/gid). stdout capture sidesteps it entirely. message) → `bench-report` exits non-zero and the Markdown carries `❌`; revert. - `--cov-fail-under=100` undisturbed: `benchmarks/*` stays in `[tool.coverage.run] omit`; new tests import the pure functions like the existing ones. -- **Not verifiable locally**: the live comment post (no fork; fires only on a real - PR). The first PR after merge validates the comment step end to end. +- **Live comment post**: validated on this feature's own PR (#148). The first + attempt surfaced hazard 1 (compose build noise on stdout); after the sentinel + slice the sticky comment renders the clean table. ## Risk -- **`tee` masking the gate exit** (medium × high) — Actions' default `bash -e` - returns tee's status, silently disabling the gate. Mitigated by `shell: bash` - (`-eo pipefail`) and asserted by the gate-still-bites test. -- **TTY chatter polluting the captured body** (low × medium) — mitigated by `-T` - and `--markdown` writing only the doc to stdout. +- **Compose build noise on stdout** (materialized) — `docker compose run` puts + buildkit output on stdout in CI, so a naive capture posts a wall of build logs. + Mitigated by slicing from the `## Benchmark gate` sentinel to EOF, which is + independent of compose's stream routing. +- **Masking the gate exit** (medium × high) — piping through `sed`/`tee`, or + Actions' default `bash -e`, would drop the gate's non-zero status. Mitigated by + owning the exit code by hand (`set +e`, capture from the stdout redirect, + re-`exit`) and asserted by the gate-still-bites test. - **Comment step failing the build on fork PRs** (low × low today) — mitigated by `continue-on-error` + the Summary-page fallback.