From c2efced41d2d96b7b45ec8d8e9d8f13bf66f9bdb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:17:26 +0000 Subject: [PATCH 01/18] Add design doc for CI test log grouping and failure surfacing Plans GitHub Actions log grouping for the default (non-bucketed) full test run, plus ::error annotations and GITHUB_STEP_SUMMARY failure tables. Documents runner constraints (no nested groups, annotation caps, summary size limits), spike-verified marker placement in Deno's test output framing, phased implementation, invariants, and a verification plan. No behavior change. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd --- dev-docs/ci-test-log-grouping-design.md | 280 ++++++++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 dev-docs/ci-test-log-grouping-design.md diff --git a/dev-docs/ci-test-log-grouping-design.md b/dev-docs/ci-test-log-grouping-design.md new file mode 100644 index 0000000000..c28efc661d --- /dev/null +++ b/dev-docs/ci-test-log-grouping-design.md @@ -0,0 +1,280 @@ +# Design: GitHub Actions log grouping and failure surfacing for the full (non-bucketed) test run + +Status: **proposal** (not implemented). This document plans the work; it +deliberately ships no behavior change. + +Base: written on top of `test/smoke-tests-built-version` (the built-version +testing branch — see `llm-docs/built-version-testing-architecture.md`). The +non-bucketed full runs this design targets are exactly the runs that branch +adds: the smoke/playwright/ff-matrix legs of `test-smokes-built.yml` all call +`test-smokes.yml` with `buckets` empty (or a single-file bucket), landing in +the flat-log path described below. The design is mode-agnostic: in both dev +and binary mode the harness runs in the same `deno test` process, so marker +emission points are identical. + +## Problem + +PR [#13787](https://github.com/quarto-dev/quarto-cli/pull/13787) and +[#13807](https://github.com/quarto-dev/quarto-cli/pull/13807) made bucketed CI +runs navigable by wrapping each test file in a `::group::` at the **workflow +level** (`.github/workflows/test-smokes.yml`, the "Run Smoke Tests as a +bucket on Linux/Windows" per-file loops). That works because the YAML loop +runs `run-tests.sh ` at a time, so group markers stay contiguous. + +The **default path** — `buckets == ''`, the "Run all Smoke Tests +Linux/Windows" steps in `test-smokes.yml` — runs one giant `deno test` over +every discovered test file (in binary mode, `run-tests.sh` defaults this to +`smoke/`). No code on that path emits a single workflow command, so the log is +thousands of flat lines and finding a failure means scrolling or text search. +Every leg of `test-smokes-built.yml` (smoke, playwright, ff-matrix) reaches +this path. + +Additional gaps that apply to *both* paths: + +- No `::error` annotations per failed test → failures are not visible on the + run summary page or PR checks tab (only the playwright integration test + emits one today, in `tests/integration/playwright-tests.test.ts`). +- Nothing writes `GITHUB_STEP_SUMMARY` → no at-a-glance failure table. +- `src/tools/github.ts` already contains a complete, correctly-escaping + workflow-command toolkit (`startGroup`/`endGroup`, `error`/`warning`/`notice` + with `escapeData`/`escapeProperty`, `isGitHubActions()` gating) that is dead + code for tests. + +## Hard constraints (GitHub Actions runner behavior) + +These drive the whole design; sources at the end. + +1. **Groups cannot nest.** A second `::group::` acts as an implicit + `::endgroup::` for the first; content after the inner group ends up + *outside any group*. Still unfixed + ([actions/toolkit#1001](https://github.com/actions/toolkit/issues/1001), + [actions/runner#802](https://github.com/actions/runner/issues/802)). + Consequence: **exactly one owner of grouping per code path**. +2. A stray `::endgroup::` with no open group is harmless — safe to emit + defensively. (Shell caveat: `echo "::endgroup::"` clobbers `$?`; capture + exit codes first. Not an issue in TypeScript.) +3. **Groups cannot be force-expanded** in the UI + ([actions/runner#1036](https://github.com/actions/runner/issues/1036)). + Failure detail must therefore land *outside* groups. +4. **Annotation caps**: 10 `::error` + 10 `::warning` per **step**, 50 total + per **job**; excess silently dropped. Emit at most ~9 per-test error + annotations per step, then one aggregate. +5. **`GITHUB_STEP_SUMMARY`**: 1 MiB per step, max 20 step summaries shown per + job. Content just under the limit can be *silently* dropped + ([actions/runner#4337](https://github.com/actions/runner/issues/4337)) — + self-truncate around ~512 KiB. +6. Annotation messages do **not** render ANSI escapes — strip color codes + before putting captured output into `::error` text or the step summary. + ANSI *inside* group bodies is fine (the log viewer renders it). +7. Parsing is line-based on stdout: commands work from any child process, but + must start at column 0 with a trailing newline. Multi-line messages use + `%0A` escaping (`escapeData` in `src/tools/github.ts` already does this). +8. **Interleaved parallel output corrupts grouping** (process B's `::group::` + implicitly closes process A's). Today this is a non-issue: `run-tests.sh` + line 164 invokes `deno test` **without `--parallel`**, so test files run + serially in one process. If `--parallel` is ever added, grouping must move + to buffer-and-flush emission (see Non-goals). + +## Spike results (verified with the pinned Deno v2.7.14) + +A standalone experiment (two test files, markers emitted from inside +`Deno.test` bodies, one seeded failure) confirmed: + +- Marker lines printed from a test body are replayed **verbatim at column 0** + inside Deno's `------- output -------` / `------- post-test output -------` + framing, so the runner parses them. +- Opening a group inside the *first test of a file* yields this net structure + (as the runner parses it): + + ```text + running N tests from ./smoke/foo.test.ts <- visible (outside groups) + [smoke] > first test name ... <- visible + ::group::./smoke/foo.test.ts <- group opens + ...all per-test output and "... ok" lines of the file... + ::endgroup:: <- emitted by the next file's first test + running M tests from ./smoke/bar.test.ts <- visible + ``` + + i.e. per-file grouping collapses exactly the noise (per-test output blocks + and `ok` lines) while keeping Deno's own file headers visible as structure. +- **On failure**, emitting `::endgroup::` from the failure path *before* + throwing puts the `FAILED` result line **outside** any group (visible in the + collapsed view), with the `::error` annotation adjacent. The next passing + test simply re-opens a group with the same file title (flat sequential + groups — allowed). +- An `unload` listener's `::endgroup::` flushes **before** Deno's terminal + `ERRORS` / `FAILURES` / summary sections, so the end-of-run failure details + are never swallowed by the last group. + +Caveat: the exact attribution of output to Deno's `output` vs `post-test +output` blocks is reporter behavior that may shift across Deno upgrades. It +does not affect correctness of the net grouping shown above, but the +verification checker (below) should run again on any Deno bump. + +## Design + +Three phases, each independently shippable and independently revertable. + +### Phase 1 — failure surfacing (no grouping; zero nesting risk) + +All changes in `tests/test.ts`, in the existing failure path that builds the +`━━━ TEST FAILURE:` block (around line 327), reusing `src/tools/github.ts` +helpers (gated on `isGitHubActions()`, so local runs are byte-identical): + +1. **`::error` annotation per failed test**: + `error(message, { file, title })` with + - `file`: repo-relative path of the test file (or the smoke-all `.qmd` + document when the failing test is a smoke-all doc — that is the file a + developer needs to open); + - `title`: the test name (`[smoke] > ...`); + - message: the repro command (`./run-tests.sh `) plus the first ~20 + lines of ANSI-stripped captured output, `%0A`-escaped. + - **Cap**: a module-level counter stops at 9 annotations; the 10th failure + emits a single aggregate + `::error title=More test failures::N additional failures — see step summary`. +2. **Step summary row per failed test**: append to the file at + `$GITHUB_STEP_SUMMARY` (plain `Deno.writeTextFileSync(..., { append: true })`; + add a small `stepSummary()` helper to `src/tools/github.ts`): + - a table header once per process (guard with a module flag), then one row + per failure: test file / test name / duration, followed by a + `
output` block with the ANSI-stripped log + excerpt and the repro command; + - a byte budget (~512 KiB per process) after which rows degrade to + name-only lines, to stay clear of the silent-drop bug. + +This works identically in bucket mode and default mode (each bucket job gets +its own annotations and summary), and interacts with nothing that exists. + +### Phase 2 — per-file grouping for the default path (env-gated harness emission) + +Owner rule: **the harness owns grouping only when nothing else does.** + +- New env switch `QUARTO_TESTS_NO_LOG_GROUP`. The two YAML bucket loops in + `test-smokes.yml` set it (`=1`) alongside their existing + `::group::Testing ${file}` — the #13807 bucket log format does not change at + all. Anyone else wrapping `run-tests.sh` in their own group can use the same + switch. +- In `tests/test.ts`, a small module (or an extension of the existing + `src/tools/github.ts` import) tracks the current test-file origin: + - at the start of each test's `execute`, resolve the declaring file (the + harness already knows it — `testQuartoCmd`/`unitTest` call sites — or via + the registration call's captured stack); + - on file change: `endGroup()` (defensive, harmless if none open) then + `startGroup(relativePath)`; + - on the failure path (before `fail()` throws): `endGroup()` and clear the + "open" flag, so the `FAILED` line and annotation land outside groups + (verified by the spike); + - `globalThis.addEventListener("unload", ...)`: close any open group so the + terminal `ERRORS`/summary sections are never grouped (verified); + - every emission gated on + `isGitHubActions() && !Deno.env.get("QUARTO_TESTS_NO_LOG_GROUP")`. +- Granularity is **per test file**, matching Deno's own "running N tests + from ..." structure. Per-test granularity is the documented fallback if real + logs show files whose single collapsed group is still too coarse (e.g. + `smoke-all.test.ts` runs many documents as separate tests — if needed, treat + each smoke-all *document* as the boundary instead; the harness knows the + document path). + +### Phase 3 (optional follow-ups) + +- Run-level footer in the step summary: pass/fail/skip counts and total + duration (from an `unload` hook), so a green run also leaves a one-line + receipt. +- Group the setup portions of the default-path steps in YAML (precedent: the + R-setup steps already do this, `test-smokes.yml` lines 179–216). +- Add the final marker-emission conventions to `llm-docs/testing-patterns.md` + so future automated changes preserve the invariants below. + +## Invariants (must hold after any change) + +1. At most one group open at any time per job step; every `::group::` is + closed before the next one opens (defensive `endGroup()` first). +2. No `::group::`/`::endgroup::` emitted while an outer YAML group is open + (`QUARTO_TESTS_NO_LOG_GROUP` contract). +3. `FAILED` result lines, the `ERRORS`/`FAILURES` sections, and the run + summary are never inside a group. +4. ≤ 9 per-test `::error` annotations per step + 1 aggregate; ≤ 50 total per + job. +5. Everything is a no-op unless `GITHUB_ACTIONS == "true"`: local output is + byte-identical. +6. No change to which tests run, their order, or exit codes. +7. All interpolated text goes through `escapeData`/`escapeProperty`; ANSI is + stripped from annotation messages and step-summary content. + +## Verification plan + +1. **Marker checker script** (`tests/tools/check-gha-log.ts` or similar, + dev-only): reads a captured log and asserts invariants 1–3 mechanically + (balanced groups, no nesting, markers at column 0, `FAILED`/`ERRORS` lines + outside groups). Run it locally on + `GITHUB_ACTIONS=true ./run-tests.sh | tee log.txt`. Re-run on every + Deno version bump (reporter framing is version-sensitive). +2. **Unit tests** for the new pure logic: annotation cap counter, ANSI + stripping, summary byte budget/truncation, escaping. +3. **Trial CI runs** on a fork via `workflow_dispatch`, with one deliberately + failing test committed temporarily, covering all four cells: + {bucketed, default} × {Linux, Windows}. Check in the real UI: + - groups collapse and titles read well; bucket-mode logs unchanged; + - annotations appear on the run summary (and PR checks view when opened as + a PR), pointing at the right file; + - step summary table renders; truncation path exercised once by seeding + many failures; + - job still fails with the same exit semantics as before. +4. **Rollback safety**: Phase 1 and Phase 2 are separate commits; each is + revertable without touching the other. `QUARTO_TESTS_NO_LOG_GROUP=1` in the + environment is a global kill switch for grouping without a revert. + +## Coverage limitation: tests that bypass `tests/test.ts` + +Phases 1–2 hook the harness (`testQuartoCmd`/`unitTest`/`test`), so they only +cover tests registered through it. Tests that call `Deno.test` directly get +neither groups nor failure annotations. Today that is chiefly the +extension-subtree tests copied into the tree by +`.github/actions/merge-extension-tests` (`smoke/julia-engine/*.test.ts` from +the `PumasAI/quarto-julia-engine` subtree — self-contained, `jsr:` imports +only, raw `Deno.Command("quarto")` spawns). + +Known wrinkle of the lazy per-file closure: the harness only closes a group +when its *next* test starts (or at `unload`), so a non-harness file running +between two harness files executes while the previous file's group is still +open — its output, including inline `FAILED` lines, is swallowed into that +stale group under a misleading title. Acceptable because Deno's end-of-run +`ERRORS`/`FAILURES` sections always repeat the full failure detail outside +any group (spike-verified), so such failures remain findable; but the doc'd +alternative if this bites is eager closure (end each group when the test that +opened it ends — safe against foreign tests, at the cost of one collapsed +block per test instead of per file). Richer coverage (own groups + +annotations) requires adopting the markers upstream in the subtree repo — +subtree files must not be edited in quarto-cli; see +`.claude/rules/extension-subtrees.md`. + +## Non-goals / explicitly deferred + +- **Nested visual hierarchy** (file ▸ test ▸ render step): impossible on the + runner; fake it with title prefixes only if ever needed. +- **`deno test --parallel`**: out of scope; if adopted, grouping must switch + to buffer-and-flush (capture each test's output, emit + group+body+endgroup atomically) or move to matrix sharding. +- **`::stop-commands::` armoring** of rendered-document output: our tests + render repo-controlled documents, so command injection via test output is + not a current threat; revisit if tests ever render untrusted input (note it + would also disable our own markers while active). +- **JUnit/structured reporters**: `deno test --reporter=junit` could feed a + richer summary later, but Phase 1 gets equivalent value from data the + harness already has, without changing the reporter everyone reads today. +- **Local parallel runs** (`run-parallel-tests.ts` local mode) interleave + freely today; grouping there is meaningless (`GITHUB_ACTIONS` unset) and + stays untouched. + +## References + +- Workflow commands: +- Nesting: actions/toolkit#1001, actions/runner#802, actions/runner#1477 +- Annotation limits: github/community discussions #26680, #68471 +- Step summary limits: GitHub docs + actions/runner#4337 +- Escaping rules: `@actions/core` `command.ts` (mirrored by + `src/tools/github.ts` `escapeData`/`escapeProperty`) +- Prior art in this repo: PR #13787, PR #13807, `test-smokes.yml` bucket + loops, R-setup grouping (`test-smokes.yml`, "Setup R packages" step) +- Built-version testing context: `llm-docs/built-version-testing-architecture.md` + (the CI legs whose logs this design targets) From 9d98634ebdcc22ef0ef11028556665769e43f96e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:36:28 +0000 Subject: [PATCH 02/18] docs: cross-reference the julia-engine binary-mode gate from the grouping design The gate itself is applied on the built-version testing branch; this only retains the design doc's pointer to the follow-up plan that documents how the gate is eventually removed. --- dev-docs/ci-test-log-grouping-design.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dev-docs/ci-test-log-grouping-design.md b/dev-docs/ci-test-log-grouping-design.md index c28efc661d..127e9e6157 100644 --- a/dev-docs/ci-test-log-grouping-design.md +++ b/dev-docs/ci-test-log-grouping-design.md @@ -232,7 +232,11 @@ neither groups nor failure annotations. Today that is chiefly the extension-subtree tests copied into the tree by `.github/actions/merge-extension-tests` (`smoke/julia-engine/*.test.ts` from the `PumasAI/quarto-julia-engine` subtree — self-contained, `jsr:` imports -only, raw `Deno.Command("quarto")` spawns). +only, raw `Deno.Command("quarto")` spawns). Those spawns also leak the +harness dev env into the built quarto, so the merge action currently skips +them in binary mode entirely; the un-gating plan (upstream env +sanitization) is `dev-docs/ci-julia-engine-binary-mode-followup.md` — in +dev shards they still run, ungrouped. Known wrinkle of the lazy per-file closure: the harness only closes a group when its *next* test starts (or at `unload`), so a non-harness file running From 4e88441a588eee07f8ea78637352289d5aab5d19 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 14:54:50 +0000 Subject: [PATCH 03/18] docs: resolve design-review findings on the CI log grouping plan Review findings addressed: - Annotation cap was defined per process but GitHub's limit is per step; in bucket mode many run-tests.sh processes share one step. The harness now emits annotations only when it owns the step (default path), gated by QUARTO_TESTS_GHA_ORCHESTRATED (replacing QUARTO_TESTS_NO_LOG_GROUP, which also covers grouping); bucket mode keeps the YAML per-file errors, and the step summary is documented as the complete record in all modes. - Step-summary size budget is now stat-based on the shared summary file so it holds across processes within a step. - The lazy-vs-eager group closure choice is now explicit (lazy chosen), invariant 3 is scoped to harness-registered tests, and the eager fallback is defined next to the policy instead of as an aside. - The julia-engine env-strip-list sync gains a mechanical drift check: a quarto-cli unit test comparing kStripEnvVars with the subtree helper after the pull, added to the follow-up procedure and acceptance. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd --- dev-docs/ci-test-log-grouping-design.md | 118 ++++++++++++++++-------- 1 file changed, 79 insertions(+), 39 deletions(-) diff --git a/dev-docs/ci-test-log-grouping-design.md b/dev-docs/ci-test-log-grouping-design.md index 127e9e6157..82f63921a1 100644 --- a/dev-docs/ci-test-log-grouping-design.md +++ b/dev-docs/ci-test-log-grouping-design.md @@ -129,9 +129,19 @@ helpers (gated on `isGitHubActions()`, so local runs are byte-identical): - `title`: the test name (`[smoke] > ...`); - message: the repro command (`./run-tests.sh `) plus the first ~20 lines of ANSI-stripped captured output, `%0A`-escaped. - - **Cap**: a module-level counter stops at 9 annotations; the 10th failure - emits a single aggregate - `::error title=More test failures::N additional failures — see step summary`. + - **Cap and ownership**: GitHub's 10-errors cap is per *workflow step*, + but bucket mode runs many `run-tests.sh` processes inside one step — a + per-process counter cannot implement a per-step budget there, and the + YAML bucket loop already emits its own `::error` per failed file. + Resolution: **the harness emits annotations only when it owns the step** + (`QUARTO_TESTS_GHA_ORCHESTRATED` unset — the default path, where one + `deno test` process *is* the whole step, so a module-level counter is + exactly a step budget: stop at 9, then one aggregate + `::error title=More test failures::N additional failures — see step summary`). + In bucket mode the YAML loop keeps its per-file `::error` as today. + Known, accepted: if more than 10 bucket files fail, GitHub silently + drops the excess YAML annotations too — the step summary (below) is the + complete record in all modes; annotations are best-effort navigation. 2. **Step summary row per failed test**: append to the file at `$GITHUB_STEP_SUMMARY` (plain `Deno.writeTextFileSync(..., { append: true })`; add a small `stepSummary()` helper to `src/tools/github.ts`): @@ -139,21 +149,29 @@ helpers (gated on `isGitHubActions()`, so local runs are byte-identical): per failure: test file / test name / duration, followed by a `
output` block with the ANSI-stripped log excerpt and the repro command; - - a byte budget (~512 KiB per process) after which rows degrade to - name-only lines, to stay clear of the silent-drop bug. + - the size budget must also be per *step*, not per process (bucket mode: + many processes append to the same summary file): before each append, + `stat` the summary file and degrade to name-only rows once it exceeds + ~512 KiB — the file size itself is the cross-process coordination, and + staying at ~half the 1 MiB limit dodges the silent-drop bug. -This works identically in bucket mode and default mode (each bucket job gets -its own annotations and summary), and interacts with nothing that exists. +Summary rows are emitted in **all** modes (cap-free, cross-process safe); +annotations follow the ownership rule above. Each bucket *job* still gets its +own summary page and annotation set. ### Phase 2 — per-file grouping for the default path (env-gated harness emission) -Owner rule: **the harness owns grouping only when nothing else does.** - -- New env switch `QUARTO_TESTS_NO_LOG_GROUP`. The two YAML bucket loops in - `test-smokes.yml` set it (`=1`) alongside their existing - `::group::Testing ${file}` — the #13807 bucket log format does not change at - all. Anyone else wrapping `run-tests.sh` in their own group can use the same - switch. +Owner rule: **the harness owns groups and annotations only when nothing else +does.** + +- One env switch, `QUARTO_TESTS_GHA_ORCHESTRATED=1`, set by the two YAML + bucket loops in `test-smokes.yml` alongside their existing + `::group::Testing ${file}` — meaning "an outer orchestrator owns workflow + commands for this step": the harness emits **no groups and no + annotations** (step-summary rows are always emitted; they are cap-free and + size-coordinate via the summary file, see Phase 1). The #13807 bucket log + format does not change at all. Anyone else wrapping `run-tests.sh` in their + own group can use the same switch. - In `tests/test.ts`, a small module (or an extension of the existing `src/tools/github.ts` import) tracks the current test-file origin: - at the start of each test's `execute`, resolve the declaring file (the @@ -167,7 +185,18 @@ Owner rule: **the harness owns grouping only when nothing else does.** - `globalThis.addEventListener("unload", ...)`: close any open group so the terminal `ERRORS`/summary sections are never grouped (verified); - every emission gated on - `isGitHubActions() && !Deno.env.get("QUARTO_TESTS_NO_LOG_GROUP")`. + `isGitHubActions() && !Deno.env.get("QUARTO_TESTS_GHA_ORCHESTRATED")`. +- **Closure policy (explicit choice): lazy per-file closure.** A group is + closed on the next harness test's file change, on the failure path, and at + `unload` — not when each test ends. Consequence, accepted: a direct + `Deno.test` file (no harness registration) running between two harness + files executes inside the previous file's still-open group (see "Coverage + limitation" below); invariant 3 is therefore scoped to harness-registered + tests. The documented fallback, **eager closure** (end the group when the + test that opened it ends; reopen on the next test of the same file), trades + one collapsed block per file for one per test but holds invariant 3 for + foreign tests too — switch to it if non-harness test files ever become + common in grouped runs. - Granularity is **per test file**, matching Deno's own "running N tests from ..." structure. Per-test granularity is the documented fallback if real logs show files whose single collapsed group is still too coarse (e.g. @@ -189,12 +218,18 @@ Owner rule: **the harness owns grouping only when nothing else does.** 1. At most one group open at any time per job step; every `::group::` is closed before the next one opens (defensive `endGroup()` first). -2. No `::group::`/`::endgroup::` emitted while an outer YAML group is open - (`QUARTO_TESTS_NO_LOG_GROUP` contract). -3. `FAILED` result lines, the `ERRORS`/`FAILURES` sections, and the run - summary are never inside a group. -4. ≤ 9 per-test `::error` annotations per step + 1 aggregate; ≤ 50 total per - job. +2. No `::group::`/`::endgroup::` and no harness `::error` emitted while an + outer orchestrator owns the step (`QUARTO_TESTS_GHA_ORCHESTRATED` + contract). +3. For **harness-registered** tests: `FAILED` result lines, the + `ERRORS`/`FAILURES` sections, and the run summary are never inside a + group. (Direct `Deno.test` files are exempt under lazy closure — see the + closure policy in Phase 2 and "Coverage limitation"; their failures + remain visible in the ungrouped end-of-run `ERRORS` section.) +4. Per-test `::error` annotations only when the harness owns the step, and + then ≤ 9 + 1 aggregate per step (one process = one step in that mode); + ≤ 50 annotations total per job including the YAML bucket loop's own. + The step summary is the complete failure record in every mode. 5. Everything is a no-op unless `GITHUB_ACTIONS == "true"`: local output is byte-identical. 6. No change to which tests run, their order, or exit codes. @@ -209,11 +244,16 @@ Owner rule: **the harness owns grouping only when nothing else does.** outside groups). Run it locally on `GITHUB_ACTIONS=true ./run-tests.sh | tee log.txt`. Re-run on every Deno version bump (reporter framing is version-sensitive). -2. **Unit tests** for the new pure logic: annotation cap counter, ANSI - stripping, summary byte budget/truncation, escaping. +2. **Unit tests** for the new pure logic: annotation cap counter, the + orchestrated-mode gate (no annotations/groups when + `QUARTO_TESTS_GHA_ORCHESTRATED` is set), ANSI stripping, the stat-based + summary size budget (including the degrade-to-name-only path), escaping. 3. **Trial CI runs** on a fork via `workflow_dispatch`, with one deliberately failing test committed temporarily, covering all four cells: - {bucketed, default} × {Linux, Windows}. Check in the real UI: + {bucketed, default} × {Linux, Windows} — plus one bucket run with **more + than 10 failing files**, to observe GitHub's silent annotation dropping + and confirm the step summary still lists every failure. Check in the real + UI: - groups collapse and titles read well; bucket-mode logs unchanged; - annotations appear on the run summary (and PR checks view when opened as a PR), pointing at the right file; @@ -221,8 +261,9 @@ Owner rule: **the harness owns grouping only when nothing else does.** many failures; - job still fails with the same exit semantics as before. 4. **Rollback safety**: Phase 1 and Phase 2 are separate commits; each is - revertable without touching the other. `QUARTO_TESTS_NO_LOG_GROUP=1` in the - environment is a global kill switch for grouping without a revert. + revertable without touching the other. `QUARTO_TESTS_GHA_ORCHESTRATED=1` + in the environment is a global kill switch for harness grouping and + annotations without a revert (summary rows remain). ## Coverage limitation: tests that bypass `tests/test.ts` @@ -238,19 +279,18 @@ them in binary mode entirely; the un-gating plan (upstream env sanitization) is `dev-docs/ci-julia-engine-binary-mode-followup.md` — in dev shards they still run, ungrouped. -Known wrinkle of the lazy per-file closure: the harness only closes a group -when its *next* test starts (or at `unload`), so a non-harness file running -between two harness files executes while the previous file's group is still -open — its output, including inline `FAILED` lines, is swallowed into that -stale group under a misleading title. Acceptable because Deno's end-of-run -`ERRORS`/`FAILURES` sections always repeat the full failure detail outside -any group (spike-verified), so such failures remain findable; but the doc'd -alternative if this bites is eager closure (end each group when the test that -opened it ends — safe against foreign tests, at the cost of one collapsed -block per test instead of per file). Richer coverage (own groups + -annotations) requires adopting the markers upstream in the subtree repo — -subtree files must not be edited in quarto-cli; see -`.claude/rules/extension-subtrees.md`. +Known consequence of the chosen **lazy per-file closure** (see the closure +policy in Phase 2): the harness only closes a group when its *next* test +starts (or at `unload`), so a non-harness file running between two harness +files executes while the previous file's group is still open — its output, +including inline `FAILED` lines, is swallowed into that stale group under a +misleading title. This is the documented exemption in invariant 3, accepted +because Deno's end-of-run `ERRORS`/`FAILURES` sections always repeat the +full failure detail outside any group (spike-verified), so such failures +remain findable; the eager-closure fallback defined in Phase 2 removes the +exemption if it ever bites. Richer coverage (own groups + annotations) +requires adopting the markers upstream in the subtree repo — subtree files +must not be edited in quarto-cli; see `.claude/rules/extension-subtrees.md`. ## Non-goals / explicitly deferred From 3a780861cce26f8f8d8b0bb5b28418bdfb6865df Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Mon, 20 Jul 2026 17:36:45 +0200 Subject: [PATCH 04/18] Add GitHub Actions failure-reporting helpers to github.ts The workflow-command toolkit in github.ts (error/escapeData/escapeProperty, group markers) was dead code for the test harness. Phase 1 of the CI test-log design (dev-docs/ci-test-log-grouping-design.md) needs a few more primitives so the harness can surface test failures as annotations and a step-summary table: - stripAnsi: annotation and step-summary text do not render ANSI, so color codes from captured output must be removed first. - harnessOwnsStep: the harness emits per-test annotations only when no outer orchestrator (the bucket-loop YAML) has claimed the step; parameterized so unit tests need not mutate process-global env. - AnnotationBudget: GitHub silently drops past 10 errors per step, so cap at 9 per-test annotations plus one aggregate. - stepSummary/stepSummarySize + a 512 KiB budget: content near the 1 MiB per-step limit can be silently dropped, and bucket mode has many processes appending to one file, so the file size is the cross-process coordinator. - summary table/detail formatters: 
 with HTML-escaped bodies so hostile
  test output cannot break out of the markdown table or a code fence.

All gate on GITHUB_ACTIONS or take an injectable path, so nothing changes off
CI. Unit tests cover the cap/aggregate counter, the ownership gate, ANSI
stripping, the stat-based budget including the degrade path, and escaping of
hostile test names.
---
 src/tools/github.ts                         | 126 ++++++++++++++++
 tests/unit/github-actions-reporting.test.ts | 155 ++++++++++++++++++++
 2 files changed, 281 insertions(+)
 create mode 100644 tests/unit/github-actions-reporting.test.ts

diff --git a/src/tools/github.ts b/src/tools/github.ts
index 947bbdc8cd..131c8cbce3 100644
--- a/src/tools/github.ts
+++ b/src/tools/github.ts
@@ -141,6 +141,132 @@ export async function group(
   return await withGroupAsync(title, fn);
 }
 
+// GitHub Actions failure reporting (used by the test harness — see
+// dev-docs/ci-test-log-grouping-design.md). All of these are pure helpers or
+// gate on GITHUB_ACTIONS so callers stay byte-identical off CI.
+
+// Strip ANSI escape sequences. Annotation messages and step-summary content
+// do NOT render ANSI (only group bodies in the log viewer do), so color codes
+// must be removed before embedding captured output in either.
+// deno-lint-ignore no-control-regex
+const kAnsiPattern = /\x1b\[[0-9;?]*[A-Za-z]/g;
+export function stripAnsi(s: string): string {
+  return s.replace(kAnsiPattern, "");
+}
+
+// The harness emits per-test ::error annotations only when it owns the
+// workflow step: on CI and when no outer orchestrator has claimed the step
+// via QUARTO_TESTS_GHA_ORCHESTRATED (the bucket-loop YAML sets it and emits
+// its own per-file ::error). Arguments default to the live environment;
+// unit tests pass them explicitly to avoid mutating process-global env.
+export function harnessOwnsStep(
+  githubActions: boolean = isGitHubActions(),
+  orchestrated: string | undefined = Deno.env.get(
+    "QUARTO_TESTS_GHA_ORCHESTRATED",
+  ),
+): boolean {
+  return githubActions && !orchestrated;
+}
+
+// GitHub caps annotations at 10 ::error per workflow STEP; excess is silently
+// dropped. In the harness-owned (non-orchestrated) path one `deno test`
+// process IS the step, so this module-level budget is exactly a per-step
+// budget: allow `max` per-test annotations, leaving room for one aggregate.
+export class AnnotationBudget {
+  private emitted = 0;
+  private suppressed = 0;
+  constructor(private readonly max = 9) {}
+
+  // true  → caller should emit a per-test ::error;
+  // false → over budget, counts toward the aggregate instead.
+  recordFailure(): boolean {
+    if (this.emitted < this.max) {
+      this.emitted++;
+      return true;
+    }
+    this.suppressed++;
+    return false;
+  }
+
+  suppressedCount(): number {
+    return this.suppressed;
+  }
+}
+
+// GITHUB_STEP_SUMMARY is 1 MiB per step and content just under the limit can
+// be silently dropped (actions/runner#4337). Stay at ~half so bucket mode —
+// many processes appending to the same file — has headroom; the file size
+// itself is the cross-process coordinator.
+export const kStepSummaryBudgetBytes = 512 * 1024;
+
+// Append markdown to the GitHub Actions step summary. No-op when the file is
+// unset (local runs, or steps without a summary). `path` is injectable for
+// unit tests.
+export function stepSummary(
+  markdown: string,
+  path: string | undefined = Deno.env.get("GITHUB_STEP_SUMMARY"),
+): void {
+  if (!path) return;
+  Deno.writeTextFileSync(path, markdown, { append: true });
+}
+
+// Current size of the step-summary file (0 when unset/missing). Callers
+// compare against kStepSummaryBudgetBytes to decide whether to degrade.
+export function stepSummarySize(
+  path: string | undefined = Deno.env.get("GITHUB_STEP_SUMMARY"),
+): number {
+  if (!path) return 0;
+  try {
+    return Deno.statSync(path).size;
+  } catch {
+    return 0;
+  }
+}
+
+function htmlEscape(s: string): string {
+  return s
+    .replaceAll("&", "&")
+    .replaceAll("<", "<")
+    .replaceAll(">", ">");
+}
+
+// A table cell must not contain a raw `|` (column separator) or newline (row
+// terminator); HTML-escape the rest so angle brackets in test names render.
+function summaryCell(s: string): string {
+  return htmlEscape(s).replaceAll("|", "\\|").replace(/\r?\n/g, " ");
+}
+
+function formatDuration(ms: number): string {
+  return `${(ms / 1000).toFixed(2)}s`;
+}
+
+export function summaryTableHeader(): string {
+  return "| Test file | Test | Duration |\n| :-- | :-- | --: |\n";
+}
+
+export function summaryTableRow(
+  file: string,
+  name: string,
+  durationMs: number,
+): string {
+  return `| \`${file}\` | ${summaryCell(name)} | ${formatDuration(durationMs)} |\n`;
+}
+
+// Degraded row emitted once the summary file is over budget: the failure is
+// still recorded by name (the complete record), just without duration/output.
+export function summaryTableRowNameOnly(file: string, name: string): string {
+  return `| \`${file}\` | ${summaryCell(name)} | |\n`;
+}
+
+// Expandable output block for one failure. Uses 
 (not a ``` fence) with
+// HTML-escaped content so backticks/pipes in captured output can't break out.
+// GFM ends a table at the first non-row line, so these blocks are flushed
+// after all table rows, never interleaved between them.
+export function summaryDetailBlock(repro: string, excerpt: string): string {
+  const body = htmlEscape(`${repro}\n\n${excerpt}`);
+  return `\n
output\n\n
\n${body}\n
\n
\n\n`;
+}
+
 // GitHub API
 
 // A Github Release for a Github Repo
diff --git a/tests/unit/github-actions-reporting.test.ts b/tests/unit/github-actions-reporting.test.ts
new file mode 100644
index 0000000000..a59e90c8d2
--- /dev/null
+++ b/tests/unit/github-actions-reporting.test.ts
@@ -0,0 +1,155 @@
+/*
+ * github-actions-reporting.test.ts
+ *
+ * Tests for the GitHub Actions failure-surfacing helpers used by the test
+ * harness (Phase 1 of dev-docs/ci-test-log-grouping-design.md): the
+ * annotation cap/aggregate counter, the orchestrated-mode gate, ANSI
+ * stripping, the stat-based step-summary size budget (including the
+ * degrade-to-name-only path), and workflow-command escaping of hostile
+ * test names.
+ *
+ * Copyright (C) 2020-2026 Posit Software, PBC
+ */
+
+import { unitTest } from "../test.ts";
+import { assert, assertEquals } from "testing/asserts";
+import {
+  AnnotationBudget,
+  escapeData,
+  escapeProperty,
+  harnessOwnsStep,
+  kStepSummaryBudgetBytes,
+  stepSummary,
+  stepSummarySize,
+  stripAnsi,
+  summaryDetailBlock,
+  summaryTableRow,
+  summaryTableRowNameOnly,
+} from "../../src/tools/github.ts";
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - annotation budget caps at 9 then aggregates", async () => {
+  const budget = new AnnotationBudget();
+  const emitted: boolean[] = [];
+  for (let i = 0; i < 12; i++) {
+    emitted.push(budget.recordFailure());
+  }
+  // exactly 9 per-test annotations, leaving room for one aggregate under
+  // GitHub's 10-per-step cap
+  assertEquals(emitted.filter((x) => x).length, 9);
+  assertEquals(emitted.slice(0, 9).every((x) => x), true);
+  assertEquals(emitted.slice(9).some((x) => x), false);
+  // the 3 failures past the cap are counted for the aggregate
+  assertEquals(budget.suppressedCount(), 3);
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - annotation budget honors a custom cap", async () => {
+  const budget = new AnnotationBudget(2);
+  assertEquals(budget.recordFailure(), true);
+  assertEquals(budget.recordFailure(), true);
+  assertEquals(budget.recordFailure(), false);
+  assertEquals(budget.suppressedCount(), 1);
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - budget with no failures aggregates nothing", async () => {
+  const budget = new AnnotationBudget();
+  assertEquals(budget.suppressedCount(), 0);
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - harnessOwnsStep gate", async () => {
+  // owns the step: on CI with no orchestrator claiming it
+  assertEquals(harnessOwnsStep(true, undefined), true);
+  assertEquals(harnessOwnsStep(true, ""), true);
+  // an outer orchestrator owns the step
+  assertEquals(harnessOwnsStep(true, "1"), false);
+  // never emits off CI
+  assertEquals(harnessOwnsStep(false, undefined), false);
+  assertEquals(harnessOwnsStep(false, "1"), false);
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - stripAnsi removes color codes", async () => {
+  assertEquals(stripAnsi("\x1b[31mred\x1b[0m"), "red");
+  assertEquals(stripAnsi("\x1b[1m\x1b[32mbold green\x1b[0m done"), "bold green done");
+  // plain text is untouched
+  assertEquals(stripAnsi("no color here"), "no color here");
+  // newlines are preserved (only escapes are stripped)
+  assertEquals(stripAnsi("line1\n\x1b[31mline2\x1b[0m"), "line1\nline2");
+});
+
+unitTest("gha-reporting - step summary size budget and degrade path", async () => {
+  const tmp = Deno.makeTempFileSync({ suffix: ".md" });
+  try {
+    // fresh file: size is 0, well under budget
+    assertEquals(stepSummarySize(tmp), 0);
+
+    // append accumulates and stat reflects the growth
+    stepSummary("x".repeat(100), tmp);
+    assertEquals(stepSummarySize(tmp), 100);
+    assert(stepSummarySize(tmp) < kStepSummaryBudgetBytes, "still under budget");
+
+    // push the file past the budget → the degrade decision flips
+    stepSummary("y".repeat(kStepSummaryBudgetBytes), tmp);
+    assert(
+      stepSummarySize(tmp) > kStepSummaryBudgetBytes,
+      "now over budget → callers degrade to name-only rows",
+    );
+
+    // no-op (does not throw) when the summary file is unset
+    stepSummary("ignored", "");
+    stepSummary("ignored", undefined);
+    assertEquals(stepSummarySize(""), 0);
+    assertEquals(stepSummarySize(undefined), 0);
+  } finally {
+    Deno.removeSync(tmp);
+  }
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - summary rows escape table-breaking characters", async () => {
+  // pipes are escaped, newlines collapsed, angle brackets HTML-escaped so a
+  // hostile test name cannot break the markdown table
+  const row = summaryTableRow(
+    "tests/docs/smoke-all/x.qmd",
+    "a|b\nc ",
+    1234,
+  );
+  assert(row.includes("\\|"), "pipe escaped");
+  assert(!row.includes("\n c") && !row.includes("b\nc"), "newline collapsed");
+  assert(row.includes("<tag>"), "angle brackets escaped");
+  assert(row.includes("1.23s"), "duration formatted");
+
+  // degraded row records the failure by name with an empty duration cell
+  const nameOnly = summaryTableRowNameOnly("tests/docs/smoke-all/x.qmd", "a|b");
+  assert(nameOnly.includes("\\|"), "pipe escaped in name-only row");
+  assert(nameOnly.trimEnd().endsWith("| |"), "empty duration cell");
+});
+
+// deno-lint-ignore require-await
+unitTest("gha-reporting - detail block escapes HTML so output cannot break out", async () => {
+  const block = summaryDetailBlock(
+    "./run-tests.sh docs/smoke-all/x.qmd",
+    "boom