From 79df468255b50f6aa7bd9cb19a622a4a37b88dac Mon Sep 17 00:00:00 2001 From: Salman Mohammed Date: Wed, 26 Aug 2026 17:41:56 -0400 Subject: [PATCH 01/27] Add Buzz benchmark evaluation layers (#6823) ## Summary - classify every Buzz-native benchmark task as `regression` or `workflow` - add metadata-backed `--layer` selection with k=1 regression and k=3 workflow defaults, while preserving explicit `--attempts`/`-k` - split an unqualified mixed Buzz dataset run into two Harbor jobs so each layer gets the correct default - document the layer contract and cover metadata, selection, defaults, overrides, invalid input, and command construction --------- Signed-off-by: Salman Mohammed --- Justfile | 2 +- benchmarks/buzz-dataset/README.md | 53 +++-- .../ambiguous-user-mention/task.toml | 1 + .../create-channel-invite-users/task.toml | 1 + .../cross-thread-requests/task.toml | 1 + .../interleaved-agent-reports/task.toml | 1 + .../buzz-dataset/multiline-message/task.toml | 1 + .../narrative-agent-names/task.toml | 1 + .../task.toml | 1 + .../buzz-dataset/reply-to-thread/task.toml | 1 + .../buzz-dataset/user-mention/task.toml | 1 + benchmarks/harbor-buzz-orchestra/README.md | 16 +- .../scripts/benchmark.py | 200 +++++++++++++++++- .../testbed/tests/test_benchmark.py | 181 +++++++++++++++- 14 files changed, 432 insertions(+), 29 deletions(-) diff --git a/Justfile b/Justfile index 6c7740bc7a..01c6b503e6 100644 --- a/Justfile +++ b/Justfile @@ -1007,7 +1007,7 @@ goose-bg relay="ws://localhost:3000" agents="1" heartbeat="0" prompt="" key="$BU # ─── Benchmarking ───────────────────────────────────────────────────────────── -# Run the Buzz orchestra benchmark — leaderboard-eligible by default (TB 2.1, k=5, Sonnet+Haiku). Stands up its own Docker stack; --gui opens a live spectator desktop app; other flags pass to benchmark.py (--dataset/--path, --include-task, --attempts, --manifest, --dry-run, ...) +# Run the Buzz orchestra benchmark — TB defaults to leaderboard-eligible k=5; Buzz task layers default to regression k=1 and workflow k=3. Stands up its own Docker stack; --gui opens a live spectator desktop app; other flags pass to benchmark.py (--dataset/--path, --layer, --include-task, --attempts, --manifest, --dry-run, ...) benchmark *ARGS: #!/usr/bin/env bash set -euo pipefail diff --git a/benchmarks/buzz-dataset/README.md b/benchmarks/buzz-dataset/README.md index f5c8c8bb24..cfce3b257a 100644 --- a/benchmarks/buzz-dataset/README.md +++ b/benchmarks/buzz-dataset/README.md @@ -5,23 +5,41 @@ Each task poses an ordinary-looking question; what is graded is how the agent answers it through Buzz — where the reply lands, who it notifies, what it was willing to read. -| Task | Behavior under test | -| --- | --- | -| [`reply-to-thread`](reply-to-thread) | Answers in the user's thread instead of as a new top-level message | -| [`user-mention`](user-mention) | Hands the turn back with an event-level `p`-tag mention of the requesting human | -| [`read-named-path-outside-workspace`](read-named-path-outside-workspace) | Reads a path the user named explicitly instead of refusing it as out of bounds | -| [`create-channel-invite-users`](create-channel-invite-users) | Creates a channel with the exact shape, TTL, and membership asked for | -| [`multiline-message`](multiline-message) | Preserves real newlines and blank-line structure through the CLI publish path | -| [`narrative-agent-names`](narrative-agent-names) | Names agents in narrative without waking them through `p` tags | -| [`interleaved-agent-reports`](interleaved-agent-reports) | Retains and synthesizes every report in a batch of agent messages | -| [`cross-thread-requests`](cross-thread-requests) | Keeps simultaneous top-level requests isolated and replies to both exact threads | -| [`ambiguous-user-mention`](ambiguous-user-mention) | Resolves duplicate display names and notifies only the intended pubkey | +| Task | Layer | Behavior under test | +| --- | --- | --- | +| [`reply-to-thread`](reply-to-thread) | Regression | Answers in the user's thread instead of as a new top-level message | +| [`user-mention`](user-mention) | Regression | Hands the turn back with an event-level `p`-tag mention of the requesting human | +| [`read-named-path-outside-workspace`](read-named-path-outside-workspace) | Regression | Reads a path the user named explicitly instead of refusing it as out of bounds | +| [`create-channel-invite-users`](create-channel-invite-users) | Workflow | Creates a channel with the exact shape, TTL, and membership asked for | +| [`multiline-message`](multiline-message) | Regression | Preserves real newlines and blank-line structure through the CLI publish path | +| [`narrative-agent-names`](narrative-agent-names) | Regression | Names agents in narrative without waking them through `p` tags | +| [`interleaved-agent-reports`](interleaved-agent-reports) | Workflow | Retains and synthesizes every report in a batch of agent messages | +| [`cross-thread-requests`](cross-thread-requests) | Workflow | Keeps simultaneous top-level requests isolated and replies to both exact threads | +| [`ambiguous-user-mention`](ambiguous-user-mention) | Workflow | Resolves duplicate display names and notifies only the intended pubkey | For `reply-to-thread` and `user-mention` the graded behavior is **deliberately absent from `instruction.md`** — it has to come from `buzz-acp`'s production base prompt. Read a task's own `README.md` before editing its instruction or verifier. +## Evaluation layers + +Every task declares `metadata.evaluation_layer` in `task.toml`: + +| Layer | Question | Default trials | Typical cadence | +| --- | --- | ---: | --- | +| Regression | Did Buzz preserve a known product contract? | k=1 | Targeted PR, nightly, or pre-release | +| Workflow | How capable is the agent at realistic Buzz work? | k=3 | Nightly or weekly on a fixed condition | + +Report regression results per behavior, not as an average capability score. +Use workflow pass rates and trends as the benchmark headline. + +Fast verifier fixtures remain ordinary CI. They validate grading logic, but do +not replace agent trials across the model, base prompt, CLI, and relay. + +The task identity remains `buzz-native/` in both layers; the wrapper reads +the metadata instead of encoding the layer in task names. + ## Running These tasks need the [`harbor-buzz-orchestra`](../harbor-buzz-orchestra) @@ -36,15 +54,20 @@ From the repo root: ```bash just benchmark \ --path benchmarks/buzz-dataset/reply-to-thread \ - --attempts 1 \ --manifest benchmarks/harbor-buzz-orchestra/manifests/buzz-native-solo-luna.yaml \ --endpoint-config benchmarks/harbor-buzz-orchestra/testbed/endpoints/openai-live.json \ --n-concurrent 1 ``` -Pass `--path benchmarks/buzz-dataset` to run the whole suite. The default -condition is one solo agent on `gpt-5.6-luna` at `thinking_effort: medium`, -which needs `OPENAI_COMPAT_API_KEY`; see +The task's regression metadata supplies its default `--attempts 1`. Select a +whole layer with `--path benchmarks/buzz-dataset --layer regression` or +`--layer workflow`. If the dataset root is passed without `--layer` or +`--attempts`, the wrapper runs two Harbor jobs so regression gets k=1 and +workflow gets k=3. An explicit `--attempts`/`-k` overrides these defaults and +runs the selected tasks in one job. + +The default condition is one solo agent on `gpt-5.6-luna` at +`thinking_effort: medium`, which needs `OPENAI_COMPAT_API_KEY`; see [the harness README](../harbor-buzz-orchestra/README.md#buzz-native-tasks) for the alternative Sonnet condition and the evidence-snapshot contract. diff --git a/benchmarks/buzz-dataset/ambiguous-user-mention/task.toml b/benchmarks/buzz-dataset/ambiguous-user-mention/task.toml index 079507e0b4..00fe095a58 100644 --- a/benchmarks/buzz-dataset/ambiguous-user-mention/task.toml +++ b/benchmarks/buzz-dataset/ambiguous-user-mention/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "mentions", "identity", "ambiguity"] [metadata] +evaluation_layer = "workflow" difficulty = "hard" category = "collaboration" tags = ["mentions", "identity", "ambiguity", "cli"] diff --git a/benchmarks/buzz-dataset/create-channel-invite-users/task.toml b/benchmarks/buzz-dataset/create-channel-invite-users/task.toml index b4ec4821f9..85b6bb81f4 100644 --- a/benchmarks/buzz-dataset/create-channel-invite-users/task.toml +++ b/benchmarks/buzz-dataset/create-channel-invite-users/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "channels", "membership", "cli"] [metadata] +evaluation_layer = "workflow" difficulty = "medium" category = "collaboration" tags = ["channels", "membership", "cli"] diff --git a/benchmarks/buzz-dataset/cross-thread-requests/task.toml b/benchmarks/buzz-dataset/cross-thread-requests/task.toml index 14634fa2d5..2fe3a5fc81 100644 --- a/benchmarks/buzz-dataset/cross-thread-requests/task.toml +++ b/benchmarks/buzz-dataset/cross-thread-requests/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "threading", "batching", "concurrency"] [metadata] +evaluation_layer = "workflow" difficulty = "hard" category = "collaboration" tags = ["threading", "batching", "concurrency", "routing"] diff --git a/benchmarks/buzz-dataset/interleaved-agent-reports/task.toml b/benchmarks/buzz-dataset/interleaved-agent-reports/task.toml index 8bd320cb09..e4c7f2dc14 100644 --- a/benchmarks/buzz-dataset/interleaved-agent-reports/task.toml +++ b/benchmarks/buzz-dataset/interleaved-agent-reports/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "agents", "batching", "synthesis"] [metadata] +evaluation_layer = "workflow" difficulty = "hard" category = "collaboration" tags = ["agents", "batching", "synthesis", "mentions"] diff --git a/benchmarks/buzz-dataset/multiline-message/task.toml b/benchmarks/buzz-dataset/multiline-message/task.toml index 69f919f7c1..a5d402de31 100644 --- a/benchmarks/buzz-dataset/multiline-message/task.toml +++ b/benchmarks/buzz-dataset/multiline-message/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "messaging", "multiline", "cli"] [metadata] +evaluation_layer = "regression" difficulty = "medium" category = "collaboration" tags = ["messaging", "multiline", "cli"] diff --git a/benchmarks/buzz-dataset/narrative-agent-names/task.toml b/benchmarks/buzz-dataset/narrative-agent-names/task.toml index 09025e0723..d6d7c8302f 100644 --- a/benchmarks/buzz-dataset/narrative-agent-names/task.toml +++ b/benchmarks/buzz-dataset/narrative-agent-names/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "mentions", "agents", "notifications"] [metadata] +evaluation_layer = "regression" difficulty = "medium" category = "collaboration" tags = ["mentions", "agents", "notifications"] diff --git a/benchmarks/buzz-dataset/read-named-path-outside-workspace/task.toml b/benchmarks/buzz-dataset/read-named-path-outside-workspace/task.toml index b4b6176915..2702fbef67 100644 --- a/benchmarks/buzz-dataset/read-named-path-outside-workspace/task.toml +++ b/benchmarks/buzz-dataset/read-named-path-outside-workspace/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "filesystem", "workspace", "named-path"] [metadata] +evaluation_layer = "regression" difficulty = "easy" category = "collaboration" tags = ["filesystem", "named-path", "regression"] diff --git a/benchmarks/buzz-dataset/reply-to-thread/task.toml b/benchmarks/buzz-dataset/reply-to-thread/task.toml index 9d400aabb7..4693677b55 100644 --- a/benchmarks/buzz-dataset/reply-to-thread/task.toml +++ b/benchmarks/buzz-dataset/reply-to-thread/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "messaging", "threading"] [metadata] +evaluation_layer = "regression" difficulty = "easy" category = "collaboration" tags = ["messaging", "threading", "implicit-behavior"] diff --git a/benchmarks/buzz-dataset/user-mention/task.toml b/benchmarks/buzz-dataset/user-mention/task.toml index 53659e146b..c976dcba18 100644 --- a/benchmarks/buzz-dataset/user-mention/task.toml +++ b/benchmarks/buzz-dataset/user-mention/task.toml @@ -7,6 +7,7 @@ authors = [{ name = "Buzz" }] keywords = ["buzz-native", "messaging", "mentions"] [metadata] +evaluation_layer = "regression" difficulty = "easy" category = "collaboration" tags = ["messaging", "mentions", "implicit-behavior"] diff --git a/benchmarks/harbor-buzz-orchestra/README.md b/benchmarks/harbor-buzz-orchestra/README.md index bbc0b60380..df859b41ab 100644 --- a/benchmarks/harbor-buzz-orchestra/README.md +++ b/benchmarks/harbor-buzz-orchestra/README.md @@ -75,12 +75,24 @@ prompt from the checked-out source build: ```bash just benchmark \ --path benchmarks/buzz-dataset/reply-to-thread \ - --attempts 1 \ --manifest benchmarks/harbor-buzz-orchestra/manifests/buzz-native-solo-luna.yaml \ --endpoint-config benchmarks/harbor-buzz-orchestra/testbed/endpoints/openai-live.json \ --n-concurrent 1 ``` +Buzz-native tasks declare one of two evaluation layers in `task.toml`. +**Regression** tasks are deterministic product/prompt regression checks and +default to k=1. **Workflow** tasks exercise multi-step collaboration +capabilities and default to k=3 (not 5). Run a layer by metadata with +`--path benchmarks/buzz-dataset --layer regression` or `--layer workflow`; +task identities stay unchanged. + +When the Buzz dataset root is passed without `--layer` or `--attempts`, the +wrapper starts two sequential Harbor jobs so each layer gets its own default. +A direct task path infers its layer's default. An explicit `--attempts`/`-k` +overrides the defaults and permits one mixed-layer job. Terminal-Bench and +other unrelated paths keep their existing k=5 default. + The default condition is `buzz-native-solo-luna.yaml` — one solo agent on `gpt-5.6-luna` at `thinking_effort: medium`. What this suite scores comes from the base prompt rather than from model strength, so the cheap model at a @@ -131,6 +143,8 @@ schema, and defaults to leaderboard-eligible settings (Terminal-Bench 2.1, ```bash just benchmark # full TB 2.1, k=5 just benchmark --path -k 1 # one local task, one attempt +just benchmark --path benchmarks/buzz-dataset --layer regression # Buzz k=1 +just benchmark --path benchmarks/buzz-dataset --layer workflow # Buzz k=3 just benchmark -i "cobol*" --attempts 3 # dataset subset just benchmark --gui # watch the run live ``` diff --git a/benchmarks/harbor-buzz-orchestra/scripts/benchmark.py b/benchmarks/harbor-buzz-orchestra/scripts/benchmark.py index b6f5601a82..0ad028796c 100755 --- a/benchmarks/harbor-buzz-orchestra/scripts/benchmark.py +++ b/benchmarks/harbor-buzz-orchestra/scripts/benchmark.py @@ -1,10 +1,11 @@ #!/usr/bin/env python3 """One-command benchmark: bring up the Buzz stack in Docker and run it. -``just benchmark`` wraps this script. Defaults are leaderboard-eligible out -of the box (Terminal-Bench 2.1, 5 attempts per problem, the Sonnet+Haiku -team); every ``run_leaderboard.py`` selector passes through unchanged. The -script owns everything around the run: +``just benchmark`` wraps this script. Terminal-Bench defaults remain +leaderboard-eligible (2.1, 5 attempts per problem, the Sonnet+Haiku team). +Buzz-native tasks use their ``evaluation_layer`` metadata: regression runs +default to 1 attempt and workflow runs default to 3. The script owns +everything around the run: - A dedicated ``buzz-benchmark`` compose project reusing the production bundle (``deploy/compose/compose.yml``) plus the benchmark port overlay, @@ -26,6 +27,8 @@ Run inside the testbed environment (the just recipe does this): from __future__ import annotations import argparse +import datetime as dt +import fnmatch import importlib.util import json import os @@ -34,6 +37,8 @@ import shutil import subprocess import sys import time +import tomllib +from dataclasses import dataclass from pathlib import Path PACKAGE_ROOT = Path(__file__).resolve().parent.parent @@ -52,6 +57,9 @@ GUI_BUNDLE_IDENTIFIER = "xyz.block.buzz.app.benchmark" DEFAULT_DATASET = "terminal-bench/terminal-bench-2-1" DEFAULT_ATTEMPTS = 5 +BUZZ_DATASET_ROOT = REPO_ROOT / "benchmarks" / "buzz-dataset" +EVALUATION_LAYERS = ("regression", "workflow") +LAYER_DEFAULT_ATTEMPTS = {"regression": 1, "workflow": 3} DEFAULT_MANIFEST = PACKAGE_ROOT / "manifests" / "tb-cobol-sonnet-haiku.yaml" DEFAULT_ENDPOINTS = PACKAGE_ROOT / "testbed" / "endpoints" / "anthropic-live.json" SCHEMA_SQL = PACKAGE_ROOT / "testbed" / "sql" / "benchmark_schema.sql" @@ -69,6 +77,16 @@ FORWARDER_BINARY = "relay-forwarder" LINUX_TARGET_DIR = STATE_DIR / "linux-target" RUST_IMAGE = "rust:1.95-alpine" + +@dataclass(frozen=True) +class BuzzTask: + """The identity and evaluation layer declared by one Buzz task.""" + + name: str + layer: str + path: Path + + _spec = importlib.util.spec_from_file_location( "run_leaderboard", Path(__file__).resolve().parent / "run_leaderboard.py" ) @@ -106,12 +124,18 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: default=[], help="Task name to exclude (glob, repeatable)", ) + parser.add_argument( + "--layer", + choices=EVALUATION_LAYERS, + help="Buzz evaluation layer to run (selected from task metadata)", + ) parser.add_argument( "--attempts", "-k", type=int, - default=DEFAULT_ATTEMPTS, - help=f"Runs per problem (default: {DEFAULT_ATTEMPTS}, the leaderboard requirement)", + default=None, + help="Runs per problem (default: Terminal-Bench 5, Buzz regression 1, " + "Buzz workflow 3)", ) parser.add_argument( "--manifest", @@ -158,6 +182,159 @@ def parse_args(argv: list[str] | None = None) -> argparse.Namespace: return parser.parse_args(argv) +def _read_buzz_task(task_toml: Path) -> BuzzTask: + """Read and validate the evaluation metadata used by the wrapper.""" + try: + config = tomllib.loads(task_toml.read_text()) + name = config["task"]["name"] + layer = config["metadata"]["evaluation_layer"] + except (OSError, tomllib.TOMLDecodeError, KeyError, TypeError) as error: + raise SystemExit( + f"invalid Buzz task metadata in {task_toml}: {error}" + ) from error + if not isinstance(name, str) or not name: + raise SystemExit(f"invalid Buzz task name in {task_toml}: expected a string") + if layer not in EVALUATION_LAYERS: + allowed = ", ".join(EVALUATION_LAYERS) + raise SystemExit( + f"invalid evaluation_layer in {task_toml}: {layer!r}; expected {allowed}" + ) + return BuzzTask(name=name, layer=layer, path=task_toml.parent) + + +def buzz_tasks_for_path(path: Path | None) -> tuple[BuzzTask, ...] | None: + """Return validated Buzz tasks, or ``None`` for an unrelated problem set.""" + if path is None: + return None + root = BUZZ_DATASET_ROOT.resolve() + selected_path = path.resolve() + if not selected_path.is_relative_to(root): + return None + direct_task = selected_path / "task.toml" + task_files = ( + [direct_task] + if direct_task.is_file() + else sorted(selected_path.glob("*/task.toml")) + ) + if not task_files: + raise SystemExit(f"no Buzz tasks found under {path}") + tasks = tuple(_read_buzz_task(task_file) for task_file in task_files) + names = [task.name for task in tasks] + if len(names) != len(set(names)): + raise SystemExit(f"duplicate Buzz task names found under {path}") + return tasks + + +def _matches_task(task: BuzzTask, pattern: str) -> bool: + return fnmatch.fnmatchcase(task.name, pattern) or fnmatch.fnmatchcase( + task.path.name, pattern + ) + + +def select_buzz_tasks( + tasks: tuple[BuzzTask, ...], + *, + layer: str | None, + include: list[str], + exclude: list[str], +) -> tuple[BuzzTask, ...]: + """Apply layer metadata and the wrapper's existing name selectors.""" + selected = tuple(task for task in tasks if layer is None or task.layer == layer) + if include: + selected = tuple( + task + for task in selected + if any(_matches_task(task, pattern) for pattern in include) + ) + if exclude: + selected = tuple( + task + for task in selected + if not any(_matches_task(task, pattern) for pattern in exclude) + ) + if not selected: + detail = f" for layer {layer!r}" if layer else "" + raise SystemExit(f"no Buzz tasks selected{detail}") + return selected + + +def _copy_run_args( + args: argparse.Namespace, + *, + tasks: tuple[BuzzTask, ...] | None, + attempts: int, + job_name: str | None = None, +) -> argparse.Namespace: + run_args = argparse.Namespace(**vars(args)) + run_args.attempts = attempts + run_args.job_name = args.job_name if job_name is None else job_name + if tasks is not None: + # Harbor filters local-path datasets by directory basename. Keep the + # canonical task.toml identity for metadata, but pass Harbor its key. + run_args.include_task = [task.path.name for task in tasks] + run_args.exclude_task = [] + return run_args + + +def plan_benchmark_runs( + args: argparse.Namespace, *, stamp: str | None = None +) -> tuple[argparse.Namespace, ...]: + """Resolve selectors and per-layer attempts into one or more Harbor jobs.""" + tasks = buzz_tasks_for_path(args.path) + if args.layer and tasks is None: + raise SystemExit( + "--layer is only valid with --path under benchmarks/buzz-dataset" + ) + if tasks is None: + return ( + _copy_run_args( + args, + tasks=None, + attempts=( + args.attempts if args.attempts is not None else DEFAULT_ATTEMPTS + ), + ), + ) + + selected = select_buzz_tasks( + tasks, + layer=args.layer, + include=args.include_task, + exclude=args.exclude_task, + ) + if args.attempts is not None: + return (_copy_run_args(args, tasks=selected, attempts=args.attempts),) + + layers = (args.layer,) if args.layer else EVALUATION_LAYERS + groups = tuple( + (layer, tuple(task for task in selected if task.layer == layer)) + for layer in layers + ) + groups = tuple((layer, group) for layer, group in groups if group) + if len(groups) == 1: + layer, group = groups[0] + return ( + _copy_run_args(args, tasks=group, attempts=LAYER_DEFAULT_ATTEMPTS[layer]), + ) + + if stamp is None: + stamp = dt.datetime.now(dt.UTC).strftime("%Y%m%dT%H%M%SZ") + if args.job_name: + base_job_name = args.job_name + else: + manifest = run_leaderboard.yaml.safe_load(args.manifest.read_text()) + base_job_name = f"lb-{manifest.get('condition', 'team')}-{stamp}" + return tuple( + _copy_run_args( + args, + tasks=group, + attempts=LAYER_DEFAULT_ATTEMPTS[layer], + job_name=f"{base_job_name}-{layer}", + ) + for layer, group in groups + ) + + # -- state: secrets and identities, generated once -------------------------- @@ -582,6 +759,7 @@ def leaderboard_argv( def main(argv: list[str] | None = None) -> int: args = parse_args(argv) + runs = plan_benchmark_runs(args) state = load_state() print_user_identity(state) write_env_file(state) @@ -598,9 +776,13 @@ def main(argv: list[str] | None = None) -> int: if args.gui: launch_gui(state) - return run_leaderboard.main( - leaderboard_argv(args, provisioner_config, agent_bin_dir) - ) + for run_args in runs: + result = run_leaderboard.main( + leaderboard_argv(run_args, provisioner_config, agent_bin_dir) + ) + if result != 0: + return result + return 0 if __name__ == "__main__": diff --git a/benchmarks/harbor-buzz-orchestra/testbed/tests/test_benchmark.py b/benchmarks/harbor-buzz-orchestra/testbed/tests/test_benchmark.py index e0c6d32ec4..0644df63b3 100644 --- a/benchmarks/harbor-buzz-orchestra/testbed/tests/test_benchmark.py +++ b/benchmarks/harbor-buzz-orchestra/testbed/tests/test_benchmark.py @@ -1,11 +1,13 @@ """just benchmark must default to leaderboard-eligible settings.""" +import asyncio import importlib.util import json import sys from pathlib import Path import pytest +from harbor.models.job.config import DatasetConfig _SCRIPT = Path(__file__).parents[2] / "scripts" / "benchmark.py" _spec = importlib.util.spec_from_file_location("benchmark", _SCRIPT) @@ -22,9 +24,10 @@ def state_dir(tmp_path, monkeypatch): def test_defaults_are_leaderboard_eligible(): args = benchmark.parse_args([]) - assert args.attempts == 5 + assert args.attempts is None assert args.dataset is None and args.path is None # dataset default applied later - argv = benchmark.leaderboard_argv(args, Path("prov.json"), Path("linux-bin")) + (run,) = benchmark.plan_benchmark_runs(args) + argv = benchmark.leaderboard_argv(run, Path("prov.json"), Path("linux-bin")) assert argv[argv.index("--dataset") + 1] == "terminal-bench/terminal-bench-2-1" assert argv[argv.index("--attempts") + 1] == "5" assert argv[argv.index("--manifest") + 1].endswith("tb-cobol-sonnet-haiku.yaml") @@ -51,7 +54,8 @@ def test_selectors_pass_through(): "--dry-run", ] ) - argv = benchmark.leaderboard_argv(args, Path("p.json"), Path("b")) + (run,) = benchmark.plan_benchmark_runs(args) + argv = benchmark.leaderboard_argv(run, Path("p.json"), Path("b")) assert argv[argv.index("--path") + 1] == "/tmp/task" assert argv[argv.index("--include-task") + 1] == "cobol*" assert argv[argv.index("--exclude-task") + 1] == "flaky*" @@ -60,6 +64,177 @@ def test_selectors_pass_through(): assert "--dataset" not in argv +def test_buzz_task_metadata_defines_the_expected_layers(): + tasks = benchmark.buzz_tasks_for_path(benchmark.BUZZ_DATASET_ROOT) + assert tasks is not None + by_layer = { + layer: {task.path.name for task in tasks if task.layer == layer} + for layer in benchmark.EVALUATION_LAYERS + } + assert by_layer == { + "regression": { + "reply-to-thread", + "user-mention", + "read-named-path-outside-workspace", + "multiline-message", + "narrative-agent-names", + }, + "workflow": { + "ambiguous-user-mention", + "cross-thread-requests", + "create-channel-invite-users", + "interleaved-agent-reports", + }, + } + + +@pytest.mark.parametrize("layer", [None, "other"]) +def test_buzz_task_metadata_rejects_missing_or_unknown_layers( + tmp_path, monkeypatch, layer +): + dataset = tmp_path / "buzz-dataset" + task = dataset / "example" + task.mkdir(parents=True) + metadata = "" if layer is None else f'evaluation_layer = "{layer}"\n' + (task / "task.toml").write_text( + 'schema_version = "1.3"\n\n' + '[task]\nname = "buzz-native/example"\n\n' + f'[metadata]\n{metadata}difficulty = "easy"\n' + ) + monkeypatch.setattr(benchmark, "BUZZ_DATASET_ROOT", dataset) + + with pytest.raises(SystemExit, match="evaluation_layer|metadata"): + benchmark.buzz_tasks_for_path(dataset) + + +@pytest.mark.parametrize(("layer", "attempts"), [("regression", 1), ("workflow", 3)]) +def test_layer_selects_metadata_and_uses_its_default_attempts(layer, attempts): + args = benchmark.parse_args( + ["--path", str(benchmark.BUZZ_DATASET_ROOT), "--layer", layer] + ) + (run,) = benchmark.plan_benchmark_runs(args) + + assert run.attempts == attempts + selected = benchmark.buzz_tasks_for_path(benchmark.BUZZ_DATASET_ROOT) + assert selected is not None + assert set(run.include_task) == { + task.path.name for task in selected if task.layer == layer + } + + +@pytest.mark.parametrize("layer", benchmark.EVALUATION_LAYERS) +def test_layer_selectors_resolve_through_harbor_local_dataset_filter(layer): + args = benchmark.parse_args( + ["--path", str(benchmark.BUZZ_DATASET_ROOT), "--layer", layer] + ) + (run,) = benchmark.plan_benchmark_runs(args) + + configs = asyncio.run( + DatasetConfig( + path=benchmark.BUZZ_DATASET_ROOT, + task_names=run.include_task, + ).get_task_configs(disable_verification=True) + ) + + assert {config.path.name for config in configs} == set(run.include_task) + + +def test_omitted_layer_splits_buzz_dataset_into_two_jobs(): + args = benchmark.parse_args(["--path", str(benchmark.BUZZ_DATASET_ROOT)]) + runs = benchmark.plan_benchmark_runs(args, stamp="20260825T120000Z") + + assert [run.attempts for run in runs] == [1, 3] + assert [run.job_name.rsplit("-", 1)[-1] for run in runs] == [ + "regression", + "workflow", + ] + assert set(runs[0].include_task).isdisjoint(runs[1].include_task) + + +def test_single_buzz_task_infers_its_layer_default(): + task_path = benchmark.BUZZ_DATASET_ROOT / "cross-thread-requests" + args = benchmark.parse_args(["--path", str(task_path)]) + (run,) = benchmark.plan_benchmark_runs(args) + + assert run.attempts == 3 + assert run.include_task == ["cross-thread-requests"] + + +def test_explicit_attempts_override_keeps_one_mixed_buzz_job(): + args = benchmark.parse_args( + ["--path", str(benchmark.BUZZ_DATASET_ROOT), "--attempts", "7"] + ) + (run,) = benchmark.plan_benchmark_runs(args) + + assert run.attempts == 7 + assert len(run.include_task) == 9 + + layered = benchmark.parse_args( + [ + "--path", + str(benchmark.BUZZ_DATASET_ROOT), + "--layer", + "workflow", + "-k", + "2", + ] + ) + (layered_run,) = benchmark.plan_benchmark_runs(layered) + assert layered_run.attempts == 2 + assert len(layered_run.include_task) == 4 + + +def test_invalid_layer_is_rejected(): + with pytest.raises(SystemExit): + benchmark.parse_args(["--layer", "conformance"]) + + args = benchmark.parse_args( + ["--dataset", "terminal-bench/x", "--layer", "workflow"] + ) + with pytest.raises(SystemExit, match="only valid"): + benchmark.plan_benchmark_runs(args) + + +def test_layer_dry_run_constructs_exact_task_selectors_and_attempts(): + args = benchmark.parse_args( + [ + "--path", + str(benchmark.BUZZ_DATASET_ROOT), + "--layer", + "workflow", + "--dry-run", + "--job-name", + "workflow-smoke", + ] + ) + (run,) = benchmark.plan_benchmark_runs(args) + argv = benchmark.leaderboard_argv(run, Path("prov.json"), Path("linux-bin")) + lower_args = benchmark.run_leaderboard.parse_args(argv) + binaries = {"buzz": Path("host-bin/buzz")} + agent_binaries = { + name: Path("linux-bin") / name + for name in benchmark.run_leaderboard.AGENT_BINARIES + + (benchmark.run_leaderboard.FORWARDER_BINARY,) + } + command = benchmark.run_leaderboard.build_command( + lower_args, binaries, agent_binaries + ) + + assert command[command.index("-k") + 1] == "3" + selected = [ + command[index + 1] + for index, part in enumerate(command) + if part == "--include-task-name" + ] + assert set(selected) == set(run.include_task) + assert set(selected) == { + "ambiguous-user-mention", + "cross-thread-requests", + "create-channel-invite-users", + "interleaved-agent-reports", + } + + def test_state_is_generated_once_and_reused(state_dir): first = benchmark.load_state() second = benchmark.load_state() From ceb8ba605f43728bdab64aa461211baeeb54364a Mon Sep 17 00:00:00 2001 From: tulsi Date: Wed, 26 Aug 2026 17:44:05 -0400 Subject: [PATCH 02/27] fix(desktop): accent-colored mention badges that count thread mentions (#6900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Category:** fix **User Impact:** Sidebar mention numerals now match the user's theme/accent color and appear for mentions inside threads, not just top-level channel messages. Hover-to-preview on channel rows is unchanged. **Problem:** Follow-ups to #6696: 1. The mention numeral used a fixed red `--notification` token that ignored the user's theme and accent selection. 2. The numeral read `appBadgeCount`, which deliberately excludes threaded replies (Home's badge subtotal counts those toward the Dock badge). Mentions inside threads therefore never surfaced a numeral on the channel row — only the generic thread-activity dot. **Solution:** 1. The badge now reuses the primary accent pair (`bg-primary` / `text-primary-foreground`), the same treatment as the DM count badge, so it follows whatever accent the user applies — including the pinned neutral accent of the Buzz themes. The now-unused `--notification` tokens and Tailwind utilities are removed. 2. Non-DM rows now count every unread mention/broadcast — threaded or top-level — via a new `highPriorityCount` channel projection, replacing the boolean `highPriorityUnread` across the TS contract, the Tauri SQLite store, the e2e mock bridge, and the native test rig. The Dock/app badge projection (`appBadgeCount`) is unchanged, so Dock totals do not double-count thread activity that Home already counts. The hover-to-preview channel activity popover is untouched; e2e coverage now pins the combination (thread mention → numeral, no dot, popover still opens and lists the mentioning reply).
File changes **desktop/src/features/channels/useUnreadChannels.ts** Non-DM sidebar counts switch from `appBadgeCount` to the mention/broadcast-inclusive `highPriorityCount`; the high-priority channel set derives from the same count. **desktop/src/features/sidebar/ui/SidebarSection.tsx** Badge drops the fixed notification classes and inherits the accent-following default. **desktop/src/shared/styles/globals/theme.css / desktop/tailwind.config.js** Remove the unused fixed `--notification` tokens and semantic utilities. **desktop/src/shared/api/tauriObservedUnread.ts / desktop/src-tauri/src/observed_unread.rs** Projection contract: `highPriorityUnread: bool` → `highPriorityCount: u64`, aggregated per unread high-priority event. **desktop/src/testing/e2eBridge.ts / desktop/src/features/channels/observedUnreadNativeRig.mjs** Mock/native-rig projections mirror the new count field. **desktop/src/features/channels/observedUnreadNative.test.mjs** Native badge-lane test seeds a high-priority event to match the numeral's new semantics. **desktop/tests/e2e/badge.spec.ts** Top-level mention test now asserts the badge equals the applied accent (seeded `#22c55e` under an adversarial theme). New test: in-thread mention shows the numeral, no dot, and the hover preview popover still lists the mentioning reply. **desktop/tests/e2e/channel-activity-popover.spec.ts / desktop/tests/e2e/thread-unread.spec.ts** Seeded fixtures that mention the user now expect the numeral (which subsumes the dot).
## Reproduction steps 1. Pick any theme + accent color in Settings → Appearance. 2. Receive a top-level message mentioning you in an inactive channel → the row shows a numeral pill in your accent color. 3. Receive a reply **inside a thread** that mentions you in an inactive channel → the row shows the numeral (previously only a dot). 4. Hover the row → the channel activity preview still opens and lists the mentioning reply. 5. Switch accents/themes → the pill re-colors accordingly; the Dock badge count is unchanged from before. ## Verification - `tsc --noEmit`, Biome, `check:px-text` — clean - Desktop unit tests: 179/179 channel-suite; full suite has one pre-existing failure from #6590 (`useRetainedProjectGitViews.test.mjs` loader error, fails on clean `main`) - Tauri Rust: `observed_unread` tests + clippy + fmt — clean - Playwright smoke: badge (19), channel-activity-popover (10), thread-unread (13), plus channels/inbox/stream/messaging/mentions suites — all green ## Screenshots Screenshots are posted in the PR discussion using immutable repository-hosted image URLs. Signed-off-by: tulsi --- desktop/src-tauri/src/observed_unread.rs | 12 +-- .../channels/observedUnreadNative.test.mjs | 8 +- .../channels/observedUnreadNativeRig.mjs | 6 +- .../features/channels/useUnreadChannels.ts | 30 ++++--- .../features/sidebar/ui/SidebarSection.tsx | 2 +- desktop/src/shared/api/tauriObservedUnread.ts | 2 +- desktop/src/shared/styles/globals/theme.css | 4 - desktop/src/testing/e2eBridge.ts | 6 +- desktop/tailwind.config.js | 4 - desktop/tests/e2e/badge.spec.ts | 86 +++++++++++++++++-- .../e2e/channel-activity-popover.spec.ts | 6 +- desktop/tests/e2e/thread-unread.spec.ts | 11 +-- 12 files changed, 126 insertions(+), 51 deletions(-) diff --git a/desktop/src-tauri/src/observed_unread.rs b/desktop/src-tauri/src/observed_unread.rs index 3ca5948262..b1f5608db0 100644 --- a/desktop/src-tauri/src/observed_unread.rs +++ b/desktop/src-tauri/src/observed_unread.rs @@ -131,7 +131,7 @@ pub(crate) struct ChannelProjection { badge_count: u64, app_badge_count: u64, top_level_unread: bool, - high_priority_unread: bool, + high_priority_count: u64, } #[derive(Debug, Serialize)] @@ -359,7 +359,7 @@ fn projections(tx: &Transaction<'_>, scope: &str) -> Result, scope: &str) -> Result = by_channel.into_values().collect(); result.sort_by(|a, b| a.channel_id.cmp(&b.channel_id)); @@ -873,12 +873,12 @@ mod tests { badge_count: 1, app_badge_count: 1, top_level_unread: true, - high_priority_unread: false, + high_priority_count: 0, }], removed: vec!["old".into()], }) .unwrap(); - let expected = serde_json::json!({"kind":"delta","scope":{"pubkey":"PK","relayUrl":"wss://relay/"},"generation":"gen","baseRevision":4,"revision":5,"ackedSequence":7,"upserts":[{"channelId":"ch","latest":42,"count":2,"badgeCount":1,"appBadgeCount":1,"topLevelUnread":true,"highPriorityUnread":false}],"removed":["old"]}); + let expected = serde_json::json!({"kind":"delta","scope":{"pubkey":"PK","relayUrl":"wss://relay/"},"generation":"gen","baseRevision":4,"revision":5,"ackedSequence":7,"upserts":[{"channelId":"ch","latest":42,"count":2,"badgeCount":1,"appBadgeCount":1,"topLevelUnread":true,"highPriorityCount":0}],"removed":["old"]}); assert_eq!(actual, expected); } } diff --git a/desktop/src/features/channels/observedUnreadNative.test.mjs b/desktop/src/features/channels/observedUnreadNative.test.mjs index fd58a3316f..a8c2963be6 100644 --- a/desktop/src/features/channels/observedUnreadNative.test.mjs +++ b/desktop/src/features/channels/observedUnreadNative.test.mjs @@ -869,13 +869,15 @@ test("native: an ingested event reaches the hook's unread counts, not just the p await first.unmount(); first = null; - // A notifying event exists natively when the renderer reopens. + // A notifying event exists natively when the renderer reopens. It is + // high-priority (a mention) because the non-DM sidebar numeral counts + // unread mentions/broadcasts via the projection's highPriorityCount. store.events.set("evt-badge", { id: "evt-badge", channelId: CHANNEL, createdAt: NOW_S, rootId: null, - highPriority: false, + highPriority: true, countsTowardBadge: true, countsTowardAppBadge: true, }); @@ -898,7 +900,7 @@ test("native: an ingested event reaches the hook's unread counts, not just the p assert.equal( second.result.unreadChannelCounts.get(CHANNEL), 1, - "the native badgeCount must reach unreadChannelCounts; asserting projectionsRef alone leaves this lane untested", + "the native highPriorityCount must reach unreadChannelCounts; asserting projectionsRef alone leaves this lane untested", ); assert.ok( second.result.unreadChannelIds.has(CHANNEL), diff --git a/desktop/src/features/channels/observedUnreadNativeRig.mjs b/desktop/src/features/channels/observedUnreadNativeRig.mjs index aac72768b0..cbe2476f2e 100644 --- a/desktop/src/features/channels/observedUnreadNativeRig.mjs +++ b/desktop/src/features/channels/observedUnreadNativeRig.mjs @@ -112,7 +112,7 @@ class Scope { badgeCount: 0, appBadgeCount: 0, topLevelUnread: false, - highPriorityUnread: false, + highPriorityCount: 0, }, ]), ); @@ -128,14 +128,14 @@ class Scope { badgeCount: 0, appBadgeCount: 0, topLevelUnread: false, - highPriorityUnread: false, + highPriorityCount: 0, }; entry.latest = Math.max(entry.latest, event.createdAt); entry.count += 1; entry.badgeCount += event.countsTowardBadge ? 1 : 0; entry.appBadgeCount += event.countsTowardAppBadge ? 1 : 0; entry.topLevelUnread ||= !event.rootId; - entry.highPriorityUnread ||= event.highPriority; + entry.highPriorityCount += event.highPriority ? 1 : 0; byChannel.set(event.channelId, entry); } return [...byChannel.values()].sort((a, b) => diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index 984d69ff89..ceab544d8c 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -864,25 +864,27 @@ export function useUnreadChannels( observedEvents, readAtForObservedEvent, ); + // Sidebar numerals on non-DM rows count every unread mention and + // broadcast, including threaded ones. The Dock projection + // (appBadgeCount) keeps excluding threaded replies because Home's + // badge subtotal already counts those; reusing it here would hide + // thread mentions from the channel row. + const highPriorityCount = + nativeProjection?.highPriorityCount ?? + countUnreadHighPriorityObservedEvents( + observedEvents, + readAtForObservedEvent, + ); counts.set( channel.id, - channel.channelType === "dm" ? badgeCount : appBadgeCount, + channel.channelType === "dm" ? badgeCount : highPriorityCount, ); unreadChannelNotificationCount += appBadgeCount; - // DM channels: any unread DM is high-priority. - if (channel.channelType === "dm") { - highPriority.add(channel.id); - } else if ( - nativeProjection?.highPriorityUnread || - (!nativeProjection && - countUnreadHighPriorityObservedEvents( - observedEvents, - readAtForObservedEvent, - ) > 0) - ) { - // Non-DM: high-priority only if at least one mention/broadcast - // remains unread in its own channel/thread context. + // DM channels: any unread DM is high-priority. Non-DM: high-priority + // only if at least one mention/broadcast remains unread in its own + // channel/thread context. + if (channel.channelType === "dm" || highPriorityCount > 0) { highPriority.add(channel.id); } } diff --git a/desktop/src/features/sidebar/ui/SidebarSection.tsx b/desktop/src/features/sidebar/ui/SidebarSection.tsx index 09c70f4765..a5ad12fb33 100644 --- a/desktop/src/features/sidebar/ui/SidebarSection.tsx +++ b/desktop/src/features/sidebar/ui/SidebarSection.tsx @@ -354,7 +354,7 @@ export function ChannelMenuButton({ {showsUnreadCount ? ( ) : hasThreadUnread ? ( diff --git a/desktop/src/shared/api/tauriObservedUnread.ts b/desktop/src/shared/api/tauriObservedUnread.ts index 73999aeae9..d0d3ff06fa 100644 --- a/desktop/src/shared/api/tauriObservedUnread.ts +++ b/desktop/src/shared/api/tauriObservedUnread.ts @@ -9,7 +9,7 @@ export type ObservedUnreadProjection = { badgeCount: number; appBadgeCount: number; topLevelUnread: boolean; - highPriorityUnread: boolean; + highPriorityCount: number; }; export type ObservedUnreadMembershipSeed = { participatedRootIds: string[]; diff --git a/desktop/src/shared/styles/globals/theme.css b/desktop/src/shared/styles/globals/theme.css index 9b55da2703..5fd2593c79 100644 --- a/desktop/src/shared/styles/globals/theme.css +++ b/desktop/src/shared/styles/globals/theme.css @@ -28,10 +28,6 @@ --accent-foreground: 234 16.02% 35.49%; --destructive: 347 86.67% 44.12%; --destructive-foreground: 220 23.08% 94.9%; - /* Notification attention colors are fixed across syntax themes. Unlike - destructive, these must stay recognizably red with AA label contrast. */ - --notification: 348 65% 48%; - --notification-foreground: 350 100% 98%; --border: 225 13.56% 76.86%; --input: 225 13.56% 76.86%; --ring: 234 16.02% 35.49%; diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 96c7a228aa..bf16dd3b00 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -3228,7 +3228,7 @@ function mockObservedUnreadProjections( badgeCount: 0, appBadgeCount: 0, topLevelUnread: false, - highPriorityUnread: false, + highPriorityCount: 0, }); } for (const event of scope.events.values()) { @@ -3250,14 +3250,14 @@ function mockObservedUnreadProjections( badgeCount: 0, appBadgeCount: 0, topLevelUnread: false, - highPriorityUnread: false, + highPriorityCount: 0, }; projection.latest = Math.max(projection.latest, event.createdAt); projection.count += 1; projection.badgeCount += event.countsTowardBadge ? 1 : 0; projection.appBadgeCount += event.countsTowardAppBadge ? 1 : 0; projection.topLevelUnread ||= event.rootId === null; - projection.highPriorityUnread ||= event.highPriority; + projection.highPriorityCount += event.highPriority ? 1 : 0; channels.set(event.channelId, projection); } return [...channels.values()].sort((left, right) => diff --git a/desktop/tailwind.config.js b/desktop/tailwind.config.js index 165d8a1ac2..4816b3ddc3 100644 --- a/desktop/tailwind.config.js +++ b/desktop/tailwind.config.js @@ -110,10 +110,6 @@ export default { DEFAULT: "hsl(var(--destructive))", foreground: "hsl(var(--destructive-foreground))", }, - notification: { - DEFAULT: "hsl(var(--notification))", - foreground: "hsl(var(--notification-foreground))", - }, border: "hsl(var(--border))", input: "hsl(var(--input))", ring: "hsl(var(--ring))", diff --git a/desktop/tests/e2e/badge.spec.ts b/desktop/tests/e2e/badge.spec.ts index e44e6e097b..5875cf555b 100644 --- a/desktop/tests/e2e/badge.spec.ts +++ b/desktop/tests/e2e/badge.spec.ts @@ -402,13 +402,15 @@ test("regular message bolds inactive channel without numeric badge", async ({ ); }); -test("top-level @mention shows a red numeric badge on its channel", async ({ +test("top-level @mention shows an accent-colored numeric badge on its channel", async ({ page, }) => { - // slack-ochin maps the generic destructive pair to white-on-white. The - // notification pair must remain independently red and readable. + // The badge must follow the user's accent selection, not a fixed red. + // slack-ochin maps the generic destructive pair to white-on-white, so it + // doubles as an adversarial theme: the badge still renders the accent. await page.addInitScript(() => { window.localStorage.setItem("buzz-theme", "slack-ochin"); + window.localStorage.setItem("buzz-accent-color", "#22c55e"); }); await page.goto("/"); await page.getByTestId("channel-general").click(); @@ -443,9 +445,28 @@ test("top-level @mention shows a red numeric badge on its channel", async ({ ); const mentionBadge = page.getByTestId("channel-unread-random"); await expect(mentionBadge).toHaveText("2 unread notifications"); - await expect(mentionBadge).toHaveClass(/bg-notification/); - await expect(mentionBadge).toHaveCSS("background-color", "rgb(202, 43, 75)"); - await expect(mentionBadge).toHaveCSS("color", "rgb(255, 245, 247)"); + await expect(mentionBadge).toHaveClass(/bg-primary/); + // The badge resolves to the applied accent (`--primary`), not a fixed hue. + const accentMatch = await mentionBadge.evaluate((element) => { + const probe = document.createElement("span"); + probe.style.backgroundColor = "hsl(var(--primary))"; + probe.style.color = "hsl(var(--primary-foreground))"; + document.body.appendChild(probe); + const probeStyle = getComputedStyle(probe); + const badgeStyle = getComputedStyle(element); + const result = { + badgeBg: badgeStyle.backgroundColor, + accentBg: probeStyle.backgroundColor, + badgeFg: badgeStyle.color, + accentFg: probeStyle.color, + }; + probe.remove(); + return result; + }); + expect(accentMatch.badgeBg).toBe(accentMatch.accentBg); + expect(accentMatch.badgeFg).toBe(accentMatch.accentFg); + // Selected accent (#22c55e) actually landed — the badge is green here, not red. + expect(accentMatch.badgeBg).toBe("rgb(34, 197, 94)"); const badgeContrast = await mentionBadge.evaluate((element) => { const parseRgb = (value: string) => (value.match(/[\d.]+/g) ?? []).slice(0, 3).map(Number); @@ -475,6 +496,59 @@ test("top-level @mention shows a red numeric badge on its channel", async ({ await waitForBadgeState(page, withAdditionalBadgeCount(baselineBadge, 2)); }); +test("@mention inside a thread shows the numeric badge and keeps hover-to-preview", async ({ + page, +}) => { + await page.goto("/"); + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + await waitForMockLiveSubscription(page, "random"); + const baselineBadge = await getSettledBadgeState(page); + + const rootEventId = await page.evaluate(() => { + const root = window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "random", + content: "Thread root from someone else", + kind: 40002, + pubkey: "deadbeef".repeat(8), + }); + return root?.id; + }); + + await page.evaluate( + ({ parentEventId, pubkey, mentionPubkey }) => { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "random", + content: "In-thread ping for @tyler", + kind: 40002, + parentEventId, + pubkey, + mentionPubkeys: [mentionPubkey], + }); + }, + { + parentEventId: rootEventId, + pubkey: TEST_IDENTITIES.alice.pubkey, + mentionPubkey: DEFAULT_MOCK_PUBKEY, + }, + ); + + // The threaded mention must produce the numeric badge, not just the dot. + const mentionBadge = page.getByTestId("channel-unread-random"); + await expect(mentionBadge).toHaveText("1 unread notification"); + await expect(page.getByTestId("channel-unread-dot-random")).toHaveCount(0); + + // Hover-to-preview must survive the numeral replacing the dot: the channel + // activity popover still opens and lists the mentioning reply. + await page.getByTestId("channel-random").hover(); + const popover = page.getByTestId("channel-activity-popover-random"); + await expect(popover).toBeVisible(); + await expect( + popover.getByTestId(`channel-activity-item-${rootEventId}`), + ).toBeVisible(); + await expect(popover).toContainText("In-thread ping for"); +}); + test("numeric badge increments for DM message", async ({ page }) => { await page.goto("/"); await page.getByTestId("channel-general").click(); diff --git a/desktop/tests/e2e/channel-activity-popover.spec.ts b/desktop/tests/e2e/channel-activity-popover.spec.ts index 1db10ed6f1..415e0db1a5 100644 --- a/desktop/tests/e2e/channel-activity-popover.spec.ts +++ b/desktop/tests/e2e/channel-activity-popover.spec.ts @@ -244,7 +244,11 @@ async function seedChannelActivity( ); } - await expect(page.getByTestId("channel-unread-dot-general")).toBeVisible(); + // The seeded activity includes an in-thread @mention, so the row shows the + // numeric mention badge rather than the plain thread-activity dot. The + // hover preview popover must still work either way. + await expect(page.getByTestId("channel-unread-general")).toBeVisible(); + await expect(page.getByTestId("channel-unread-dot-general")).toHaveCount(0); if (includeAgent) { await expect(page.getByTestId("channel-working-general")).toBeVisible(); } diff --git a/desktop/tests/e2e/thread-unread.spec.ts b/desktop/tests/e2e/thread-unread.spec.ts index f4f45d3319..358d5f5584 100644 --- a/desktop/tests/e2e/thread-unread.spec.ts +++ b/desktop/tests/e2e/thread-unread.spec.ts @@ -778,21 +778,22 @@ test.describe("thread unread indicator", () => { mentionPubkeys: [SELF_PUBKEY], createdAt: unreadTimestamp(), }); + // The reply mentions the user, so the row shows the numeric mention badge + // (which subsumes the thread-activity dot). + await expect(page.getByTestId("channel-unread-all-replies")).toBeVisible(); await expect( page.getByTestId("channel-unread-dot-all-replies"), - ).toBeVisible(); + ).toHaveCount(0); // View all-replies while the reply is unread. await page.getByTestId("channel-all-replies").click(); await expect(page.getByTestId("chat-title")).toHaveText("all-replies"); // The crux: leave the channel. Its unopened thread reply should still keep - // a channel sidebar dot until the thread itself is read. + // a channel sidebar unread indicator until the thread itself is read. await page.getByTestId("channel-general").click(); await expect(page.getByTestId("chat-title")).toHaveText("general"); - await expect( - page.getByTestId("channel-unread-dot-all-replies"), - ).toBeVisible(); + await expect(page.getByTestId("channel-unread-all-replies")).toBeVisible(); }); // Regression guard for BUG-2 (clear-on-read): opening an unread thread marks From cdab765748ddfeba10830a9a5331b6157bd321df Mon Sep 17 00:00:00 2001 From: Jordan Mecom Date: Wed, 26 Aug 2026 15:52:06 -0700 Subject: [PATCH 03/27] Add gated security reviews (#6816) Add a gated Codex security review for Buzz pull requests. Reviews run automatically for Block organization members. External PRs require a `@buzz-security-review` comment from a Block member, and every new external push invalidates the previous result. Codex reviews the PR SHA in a read-only, network-restricted job with no write GitHub token. A separate trusted job validates and sanitizes structured findings before posting them. The workflow uses the dedicated `CODEX_REVIEW_API_KEY` secret. --------- Signed-off-by: Jordan Mecom --- .github/scripts/codex-security-review.js | 746 ++++++++++++++++++ .github/scripts/codex-security-review.test.js | 568 +++++++++++++ .github/workflows/ci.yml | 2 + .github/workflows/codex-security-review.yml | 480 +++++++++++ Justfile | 7 +- 5 files changed, 1802 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/codex-security-review.js create mode 100644 .github/scripts/codex-security-review.test.js create mode 100644 .github/workflows/codex-security-review.yml diff --git a/.github/scripts/codex-security-review.js b/.github/scripts/codex-security-review.js new file mode 100644 index 0000000000..ac54adb665 --- /dev/null +++ b/.github/scripts/codex-security-review.js @@ -0,0 +1,746 @@ +"use strict"; + +const MARKER = ""; +const STALE_MARKER = ""; +const REVIEW_COMMAND = "@buzz-security-review"; +const CURRENT_REVIEW_LABEL = "codex-security-review-current"; +const RECONCILIATION_BATCH_SIZE = 32; +const MAX_RECONCILIATION_PASSES = 2; +const GITHUB_RETRY_ATTEMPTS = 3; +const RISKS = ["NONE", "LOW", "MEDIUM", "HIGH", "CRITICAL"]; +const SEVERITIES = new Set(RISKS.slice(1)); +const CATEGORIES = new Set([ + "Isolation", + "Auth", + "Event Integrity", + "Cryptography", + "Injection", + "Agent/Workflow", + "Desktop/Mobile", + "Concurrency", + "Reliability", + "Supply Chain", + "Other", +]); + +const completedMarker = (baseSha, headSha) => + ``; + +const reviewCommand = (headSha) => `${REVIEW_COMMAND} ${headSha}`; + +const isObject = (value) => + value !== null && typeof value === "object" && !Array.isArray(value); + +function requireKeys(value, expected, label) { + if (!isObject(value)) { + throw new Error(`${label} must be an object.`); + } + const actual = Object.keys(value).sort(); + const wanted = [...expected].sort(); + if (JSON.stringify(actual) !== JSON.stringify(wanted)) { + throw new Error(`${label} has unexpected or missing properties.`); + } +} + +function requireString(value, maxLength, label) { + if ( + typeof value !== "string" || + value.length === 0 || + value.length > maxLength + ) { + throw new Error( + `${label} must be a non-empty string of at most ${maxLength} characters.`, + ); + } + return value; +} + +function safeCodeText(value, maxLength, label) { + const input = requireString(value, maxLength, label) + .replace( + /[\u0000-\u001f\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/g, + " ", + ) + .trim(); + if (!input) { + throw new Error(`${label} is empty after removing control characters.`); + } + + const longestBacktickRun = Math.max( + 0, + ...(input.match(/`+/g) || []).map((run) => run.length), + ); + const fence = "`".repeat(longestBacktickRun + 1); + return `${fence} ${input} ${fence}`; +} + +function validPath(value) { + return ( + typeof value === "string" && + value.length > 0 && + value.length <= 500 && + !value.startsWith("/") && + !value.includes("\\") && + !/[\u0000-\u001f\u007f]/.test(value) && + !value.split("/").includes("..") + ); +} + +const encodeUrlComponent = (value) => + encodeURIComponent(value).replace( + /[!'()*]/g, + (character) => `%${character.charCodeAt(0).toString(16).toUpperCase()}`, + ); + +const encodePath = (value) => + value.split("/").map(encodeUrlComponent).join("/"); + +const githubErrorStatus = (error) => + Number(error?.status ?? error?.response?.status); + +function isRetryableGithubError(error) { + const status = githubErrorStatus(error); + if (status === 429 || (status >= 500 && status <= 504)) { + return true; + } + if (status !== 403) { + return false; + } + + const headers = error?.response?.headers || {}; + const message = `${error?.message || ""} ${error?.response?.data?.message || ""}`; + return ( + headers["retry-after"] !== undefined || + headers["x-ratelimit-remaining"] === "0" || + message.toLowerCase().includes("rate limit") + ); +} + +function githubRetryDelayMs(error, attempt) { + const headers = error?.response?.headers || {}; + const retryAfterSeconds = Number(headers["retry-after"]); + if (Number.isFinite(retryAfterSeconds) && retryAfterSeconds >= 0) { + return Math.min(Math.max(retryAfterSeconds * 1000, 1000), 120000); + } + + const resetSeconds = Number(headers["x-ratelimit-reset"]); + if ( + headers["x-ratelimit-remaining"] === "0" && + Number.isFinite(resetSeconds) + ) { + return Math.min( + Math.max(resetSeconds * 1000 - Date.now() + 1000, 1000), + 120000, + ); + } + + const status = githubErrorStatus(error); + const baseDelay = status === 403 || status === 429 ? 60000 : 2000; + return Math.min(baseDelay * 2 ** (attempt - 1), 120000); +} + +async function withGithubRetry( + operation, + { + core, + sleep = (milliseconds) => + new Promise((resolve) => setTimeout(resolve, milliseconds)), + }, +) { + for (let attempt = 1; attempt <= GITHUB_RETRY_ATTEMPTS; attempt += 1) { + try { + return await operation(); + } catch (error) { + if ( + attempt === GITHUB_RETRY_ATTEMPTS || + !isRetryableGithubError(error) + ) { + throw error; + } + const delay = githubRetryDelayMs(error, attempt); + core.warning( + `GitHub API request failed with status ${githubErrorStatus(error)}; ` + + `retrying in ${Math.ceil(delay / 1000)} seconds ` + + `(attempt ${attempt + 1} of ${GITHUB_RETRY_ATTEMPTS}).`, + ); + await sleep(delay); + } + } + + throw new Error("GitHub API retry loop ended unexpectedly."); +} + +async function findReviewComment({ github, context, prNumber }) { + const comments = await github.paginate(github.rest.issues.listComments, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + per_page: 100, + }); + return comments.find( + (comment) => + comment.user?.login === "github-actions[bot]" && + comment.user?.type === "Bot" && + comment.body?.startsWith(`${MARKER}\n`), + ); +} + +async function upsertReviewComment({ github, context, core, prNumber, body }) { + const existing = await findReviewComment({ github, context, prNumber }); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + core.info(`Updated Codex security review comment #${existing.id}.`); + return; + } + + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body, + }); + core.info(`Posted Codex security review on PR #${prNumber}.`); +} + +async function getPullRequest({ github, context, prNumber }) { + const { data: pullRequest } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + if ( + pullRequest.state !== "open" || + pullRequest.base.repo.full_name !== + `${context.repo.owner}/${context.repo.repo}` || + pullRequest.base.ref !== "main" + ) { + return null; + } + return pullRequest; +} + +async function getLiveMainSha({ github, context }) { + const { data: mainRef } = await github.rest.git.getRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: "heads/main", + }); + const sha = mainRef.object?.sha || ""; + if (mainRef.object?.type !== "commit" || !/^[0-9a-f]{40,64}$/.test(sha)) { + throw new Error("refs/heads/main did not resolve to a commit SHA."); + } + return sha; +} + +async function ensureCurrentReviewLabel({ github, context }) { + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: CURRENT_REVIEW_LABEL, + }); + return; + } catch (error) { + if (error?.status !== 404) { + throw error; + } + } + + try { + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: CURRENT_REVIEW_LABEL, + color: "1d76db", + description: "The posted Codex security review matches its recorded range.", + }); + } catch (error) { + // Another posting job may create the repository label concurrently. + if (error?.status !== 422) { + throw error; + } + } +} + +async function markReviewCurrent({ github, context, prNumber }) { + await ensureCurrentReviewLabel({ github, context }); + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [CURRENT_REVIEW_LABEL], + }); +} + +async function clearCurrentReview({ github, context, prNumber }) { + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: CURRENT_REVIEW_LABEL, + }); + } catch (error) { + if (error?.status !== 404) { + throw error; + } + } +} + +async function reviewRangeIsCurrent({ + github, + context, + prNumber, + baseSha, + headSha, + headRepo, +}) { + const [pullRequest, liveMainSha] = await Promise.all([ + getPullRequest({ github, context, prNumber }), + getLiveMainSha({ github, context }), + ]); + return ( + pullRequest !== null && + liveMainSha === baseSha && + pullRequest.head.sha === headSha && + pullRequest.head.repo?.full_name === headRepo + ); +} + +async function prepare({ github, context, core }) { + let prNumber; + let requestedHeadSha; + if (context.eventName === "pull_request_target") { + prNumber = Number(context.payload.pull_request?.number); + requestedHeadSha = context.payload.pull_request?.head?.sha || ""; + } else if (context.eventName === "issue_comment") { + prNumber = Number(context.payload.issue?.number); + const command = context.payload.comment?.body || ""; + const match = /^@buzz-security-review ([0-9a-f]{40})$/.exec(command); + if (!match) { + core.setFailed( + `Review commands must be exactly "${REVIEW_COMMAND} ".`, + ); + return; + } + requestedHeadSha = match[1]; + } else { + core.setFailed(`Unsupported review trigger: ${context.eventName}.`); + return; + } + + if (!Number.isSafeInteger(prNumber) || prNumber <= 0) { + core.setFailed("Invalid pull request number for security review."); + return; + } + + const pullRequest = await getPullRequest({ github, context, prNumber }); + if (!pullRequest) { + core.setFailed( + `Pull request #${prNumber} is not an open PR targeting main.`, + ); + return; + } + if (!pullRequest.head.repo?.full_name) { + core.setFailed( + `Pull request #${prNumber} has no available head repository.`, + ); + return; + } + if (pullRequest.head.sha !== requestedHeadSha) { + core.setFailed( + `Pull request #${prNumber} moved after this review was authorized. ` + + `Use "${reviewCommand(pullRequest.head.sha)}" to review the current head.`, + ); + return; + } + + const baseSha = await getLiveMainSha({ github, context }); + const commitRange = `${baseSha}...${pullRequest.head.sha}`; + core.setOutput("pr_number", String(prNumber)); + core.setOutput("trigger_actor", context.actor); + core.setOutput("base_sha", baseSha); + core.setOutput("head_sha", pullRequest.head.sha); + core.setOutput("head_repo", pullRequest.head.repo.full_name); + core.setOutput("commit_range", commitRange); +} + +function setReconciliationOutputs( + core, + { + prNumbers, + mainSha, + shouldContinue = false, + nextAfter = 0, + nextPass = 1, + }, +) { + core.setOutput( + "pr_numbers", + JSON.stringify(prNumbers.length > 0 ? prNumbers : [0]), + ); + core.setOutput("main_sha", mainSha); + core.setOutput("should_continue", String(shouldContinue)); + core.setOutput("next_after", String(nextAfter)); + core.setOutput("next_pass", String(nextPass)); +} + +async function prepareBaseReconciliation({ github, context, core }) { + const reconciliation = context.payload.client_payload || {}; + const afterPrNumber = Number(reconciliation.after_pr || 0); + if (!Number.isSafeInteger(afterPrNumber) || afterPrNumber < 0) { + throw new Error("Invalid reconciliation cursor."); + } + const pass = Number(reconciliation.pass || 1); + if ( + !Number.isSafeInteger(pass) || + pass < 1 || + pass > MAX_RECONCILIATION_PASSES + ) { + throw new Error("Invalid reconciliation pass."); + } + + const requestedMainSha = + context.eventName === "push" + ? context.sha + : reconciliation.main_sha || ""; + if (requestedMainSha && !/^[0-9a-f]{40,64}$/.test(requestedMainSha)) { + throw new Error("Invalid reconciliation main SHA."); + } + const liveMainSha = await getLiveMainSha({ github, context }); + const mainSha = requestedMainSha || liveMainSha; + if (mainSha !== liveMainSha) { + core.info( + `Skipping reconciliation for superseded main commit ${mainSha}.`, + ); + setReconciliationOutputs(core, { prNumbers: [], mainSha, nextPass: pass }); + return; + } + + const issues = await github.paginate(github.rest.issues.listForRepo, { + owner: context.repo.owner, + repo: context.repo.repo, + state: "open", + labels: CURRENT_REVIEW_LABEL, + per_page: 100, + }); + const prNumbers = [ + ...new Set( + issues + .filter((issue) => issue.pull_request) + .map((issue) => issue.number) + .filter((prNumber) => Number.isSafeInteger(prNumber) && prNumber > 0), + ), + ] + .sort((left, right) => left - right) + .filter((prNumber) => prNumber > afterPrNumber); + const batch = prNumbers.slice(0, RECONCILIATION_BATCH_SIZE); + const hasMore = prNumbers.length > RECONCILIATION_BATCH_SIZE; + const startRetryPass = + (batch.length > 0 || afterPrNumber > 0) && + !hasMore && + pass < MAX_RECONCILIATION_PASSES; + setReconciliationOutputs(core, { + prNumbers: batch, + mainSha, + shouldContinue: hasMore || startRetryPass, + nextAfter: hasMore ? batch.at(-1) : 0, + nextPass: startRetryPass ? pass + 1 : pass, + }); +} + +async function invalidatePullRequestUpdate({ github, context, core }) { + const association = context.payload.pull_request?.author_association || ""; + const existingOnly = association === "MEMBER" || association === "OWNER"; + await invalidate({ github, context, core, existingOnly }); +} + +async function invalidate({ + github, + context, + core, + prNumber: requestedPrNumber, + existingOnly = false, +}) { + const prNumber = Number( + requestedPrNumber ?? context.payload.pull_request?.number, + ); + if (!Number.isSafeInteger(prNumber) || prNumber <= 0) { + throw new Error("Invalid pull request number for review invalidation."); + } + + const pullRequest = await getPullRequest({ github, context, prNumber }); + if (!pullRequest) { + await clearCurrentReview({ github, context, prNumber }); + core.notice(`Skipping review invalidation for ineligible PR #${prNumber}.`); + return; + } + + const existing = await findReviewComment({ github, context, prNumber }); + if (!existing && existingOnly) { + await clearCurrentReview({ github, context, prNumber }); + core.info(`PR #${prNumber} has no Codex security review to invalidate.`); + return; + } + + const liveMainSha = await getLiveMainSha({ github, context }); + const currentPrefix = + `${MARKER}\n` + + `${completedMarker(liveMainSha, pullRequest.head.sha)}\n`; + if (existing?.body?.startsWith(currentPrefix)) { + core.info(`PR #${prNumber} already has a review for the current range.`); + return; + } + + const body = `${MARKER} +${STALE_MARKER} +## 🔐 Codex Security Review + +> **Status: review required for the current range.** +> +> The current range is \`${liveMainSha}...${pullRequest.head.sha}\`. +> A new review must complete for this exact range. When manual authorization +> is required, a Block organization member must comment exactly +> \`${reviewCommand(pullRequest.head.sha)}\` to authorize a new review. +> Any previous review applies only to its recorded range. +`; + + if (existing?.body === body) { + core.info(`PR #${prNumber} already has the current stale-review notice.`); + await clearCurrentReview({ github, context, prNumber }); + return; + } + + await upsertReviewComment({ github, context, core, prNumber, body }); + await clearCurrentReview({ github, context, prNumber }); +} + +async function post({ github, context, core }) { + const rawReview = process.env.REVIEW_JSON || ""; + if (rawReview.length === 0 || rawReview.length > 120000) { + throw new Error("Codex output is empty or exceeds the renderer limit."); + } + + const review = JSON.parse(rawReview); + requireKeys(review, ["overall_risk", "summary", "findings", "notes"], "review"); + if (!RISKS.includes(review.overall_risk)) { + throw new Error("Review has an invalid overall risk."); + } + if (!Array.isArray(review.findings) || review.findings.length > 10) { + throw new Error("Review findings must be an array with at most 10 entries."); + } + if (!Array.isArray(review.notes) || review.notes.length > 5) { + throw new Error("Review notes must be an array with at most 5 entries."); + } + + const prNumber = Number(process.env.REVIEW_PR_NUMBER); + if (!Number.isSafeInteger(prNumber) || prNumber <= 0) { + throw new Error("Invalid reviewed pull request number."); + } + const baseSha = process.env.REVIEW_BASE_SHA || ""; + const headSha = process.env.REVIEW_HEAD_SHA || ""; + const headRepo = process.env.REVIEW_HEAD_REPO || ""; + const commitRange = process.env.REVIEW_COMMIT_RANGE || ""; + if (!/^[0-9a-f]{40,64}$/.test(baseSha) || !/^[0-9a-f]{40,64}$/.test(headSha)) { + throw new Error("Invalid reviewed commit SHA."); + } + if (commitRange !== `${baseSha}...${headSha}`) { + throw new Error("Invalid reviewed commit range."); + } + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(headRepo)) { + throw new Error("Invalid reviewed head repository."); + } + + const existingReview = { + github, + context, + core, + prNumber, + existingOnly: true, + }; + const reviewedRange = { + github, + context, + prNumber, + baseSha, + headSha, + headRepo, + }; + + const [pullRequest, liveMainSha] = await Promise.all([ + getPullRequest({ github, context, prNumber }), + getLiveMainSha({ github, context }), + ]); + if ( + !pullRequest || + liveMainSha !== baseSha || + pullRequest.head.sha !== headSha || + pullRequest.head.repo?.full_name !== headRepo + ) { + core.notice(`Skipping stale review for ${commitRange} on PR #${prNumber}.`); + await invalidate(existingReview); + return; + } + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + per_page: 100, + }); + if (files.length !== pullRequest.changed_files) { + throw new Error( + `Expected ${pullRequest.changed_files} changed files, but GitHub returned ${files.length}.`, + ); + } + const changedFiles = new Set(files.map((file) => file.filename)); + + const findingKeys = [ + "severity", + "category", + "title", + "path", + "line", + "description", + "impact", + "recommendation", + ]; + const [headOwner, headName] = headRepo.split("/"); + const renderedFindings = review.findings.map((finding, index) => { + const label = `finding ${index + 1}`; + requireKeys(finding, findingKeys, label); + if (!SEVERITIES.has(finding.severity)) { + throw new Error(`${label} has an invalid severity.`); + } + if (!CATEGORIES.has(finding.category)) { + throw new Error(`${label} has an invalid category.`); + } + if (!validPath(finding.path) || !changedFiles.has(finding.path)) { + throw new Error(`${label} does not reference a changed file.`); + } + if ( + !Number.isSafeInteger(finding.line) || + finding.line < 1 || + finding.line > 10000000 + ) { + throw new Error(`${label} has an invalid line number.`); + } + + const location = + `https://github.com/${encodeUrlComponent(headOwner)}/${encodeUrlComponent(headName)}` + + `/blob/${headSha}/${encodePath(finding.path)}#L${finding.line}`; + const pathLabel = safeCodeText( + `${finding.path}:${finding.line}`, + 520, + `${label} location`, + ); + return [ + `#### [${finding.severity}] ${safeCodeText(finding.title, 200, `${label} title`)}`, + `- **Category**: ${finding.category}`, + `- **Location**: ${pathLabel} ([source](${location}))`, + `- **Description**: ${safeCodeText(finding.description, 1500, `${label} description`)}`, + `- **Impact**: ${safeCodeText(finding.impact, 1500, `${label} impact`)}`, + `- **Recommendation**: ${safeCodeText(finding.recommendation, 1500, `${label} recommendation`)}`, + ].join("\n"); + }); + + const highestFindingRisk = review.findings.reduce( + (highest, finding) => Math.max(highest, RISKS.indexOf(finding.severity)), + 0, + ); + const overallRisk = RISKS[ + Math.max(RISKS.indexOf(review.overall_risk), highestFindingRisk) + ]; + const findingsMarkdown = renderedFindings.length + ? renderedFindings.join("\n\n") + : "No concrete security, correctness, or reliability findings were identified."; + const notesMarkdown = review.notes.length + ? review.notes + .map((note, index) => `- ${safeCodeText(note, 1000, `note ${index + 1}`)}`) + .join("\n") + : "- No additional limitations were reported."; + + const triggerActor = process.env.REVIEW_TRIGGER_ACTOR || ""; + if (!/^[A-Za-z0-9-]{1,39}$/.test(triggerActor)) { + throw new Error("Invalid review trigger actor."); + } + const model = process.env.CODEX_MODEL || ""; + if (!/^[A-Za-z0-9._-]{1,100}$/.test(model)) { + throw new Error("Invalid review model name."); + } + const workflowRun = + `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}` + + `/actions/runs/${process.env.GITHUB_RUN_ID}`; + const body = `${MARKER} +${completedMarker(baseSha, headSha)} +## 🔐 Codex Security Review + +> **Note**: This is an automated, security-focused review generated by Codex. +> Use it as a supplement to human review; false positives are possible. +> +> **Scope** +> - Exact PR diff: \`${commitRange}\` +> - Model: ${model} +> +> 💡 *Click "edited" above to see earlier reviews for this PR.* + +--- + +## Review Summary + +**Overall Risk**: ${overallRisk} + +${safeCodeText(review.summary, 2000, "review summary")} + +### Findings + +${findingsMarkdown} + +### Notes + +${notesMarkdown} + +--- + +Generated by [Codex Security Review](https://github.com/openai/codex-action) | +Requested by: \`@${triggerActor}\` | +[Workflow run](${workflowRun})`; + + if (body.length > 60000) { + throw new Error("Rendered security review exceeds the GitHub comment limit."); + } + + // Register the pending write before the final freshness check. If main moves + // now, either this check clears the label or the main-push reconciler sees it. + await markReviewCurrent({ github, context, prNumber }); + if (!(await reviewRangeIsCurrent(reviewedRange))) { + core.notice( + `Skipping review because ${commitRange} moved while PR #${prNumber} was rendering.`, + ); + await invalidate(existingReview); + return; + } + + await upsertReviewComment({ github, context, core, prNumber, body }); + + if (!(await reviewRangeIsCurrent(reviewedRange))) { + core.notice( + `Review range ${commitRange} moved while posting on PR #${prNumber}; marking it stale.`, + ); + await invalidate(existingReview); + } +} + +module.exports = { + invalidate, + invalidatePullRequestUpdate, + post, + prepare, + prepareBaseReconciliation, + withGithubRetry, +}; diff --git a/.github/scripts/codex-security-review.test.js b/.github/scripts/codex-security-review.test.js new file mode 100644 index 0000000000..0e6d8068ea --- /dev/null +++ b/.github/scripts/codex-security-review.test.js @@ -0,0 +1,568 @@ +"use strict"; + +const assert = require("node:assert/strict"); +const test = require("node:test"); + +const { + invalidate, + invalidatePullRequestUpdate, + post, + prepare, + prepareBaseReconciliation, + withGithubRetry, +} = require("./codex-security-review.js"); + +const BASE_SHA = "a".repeat(40); +const OLD_BASE_SHA = "b".repeat(40); +const HEAD_SHA = "c".repeat(40); +const OTHER_HEAD_SHA = "d".repeat(40); +const NEW_BASE_SHA = "e".repeat(40); +const MARKER = ""; +const CURRENT_REVIEW_LABEL = "codex-security-review-current"; + +function pullRequest({ baseSha = OLD_BASE_SHA, headSha = HEAD_SHA } = {}) { + return { + state: "open", + base: { + ref: "main", + sha: baseSha, + repo: { full_name: "block/buzz" }, + }, + head: { + sha: headSha, + repo: { full_name: "outside/buzz" }, + }, + changed_files: 1, + }; +} + +function harness({ + pull = pullRequest(), + comments = [], + files = [], + labeledIssues = [], + liveMainShas = [BASE_SHA], +} = {}) { + const storedComments = [...comments]; + const created = []; + const updated = []; + const addedLabels = []; + const removedLabels = []; + const outputs = new Map(); + const failures = []; + const notices = []; + const info = []; + const warnings = []; + const listComments = async () => storedComments; + const listFiles = async () => files; + let labelExists = false; + let mainRefRead = 0; + const github = { + paginate: async (method, args) => method(args), + rest: { + git: { + getRef: async () => { + const sha = + liveMainShas[Math.min(mainRefRead, liveMainShas.length - 1)]; + mainRefRead += 1; + return { data: { object: { type: "commit", sha } } }; + }, + }, + issues: { + listComments, + listForRepo: async () => labeledIssues, + getLabel: async () => { + if (!labelExists) { + throw Object.assign(new Error("not found"), { status: 404 }); + } + return { data: { name: CURRENT_REVIEW_LABEL } }; + }, + createLabel: async () => { + labelExists = true; + }, + addLabels: async (input) => { + labelExists = true; + addedLabels.push(input); + }, + removeLabel: async (input) => { + if (!labelExists) { + throw Object.assign(new Error("not found"), { status: 404 }); + } + labelExists = false; + removedLabels.push(input); + }, + createComment: async (input) => { + created.push(input); + storedComments.push( + reviewComment(input.body, { id: 100 + storedComments.length }), + ); + }, + updateComment: async (input) => { + updated.push(input); + const comment = storedComments.find( + (candidate) => candidate.id === input.comment_id, + ); + if (comment) { + comment.body = input.body; + } + }, + }, + pulls: { + get: async () => ({ data: pull }), + listFiles, + }, + }, + }; + const core = { + info: (message) => info.push(message), + notice: (message) => notices.push(message), + warning: (message) => warnings.push(message), + setFailed: (message) => failures.push(message), + setOutput: (name, value) => outputs.set(name, value), + }; + const context = { + actor: "block-member", + eventName: "issue_comment", + payload: { + comment: { body: `@buzz-security-review ${HEAD_SHA}` }, + issue: { number: 6816 }, + }, + repo: { owner: "block", repo: "buzz" }, + }; + return { + context, + core, + addedLabels, + created, + failures, + github, + info, + notices, + outputs, + removedLabels, + storedComments, + updated, + warnings, + }; +} + +function reviewComment(body, { id = 42 } = {}) { + return { + id, + body, + user: { login: "github-actions[bot]", type: "Bot" }, + }; +} + +const NO_FINDINGS_REVIEW = { + overall_risk: "NONE", + summary: "No findings.", + findings: [], + notes: [], +}; + +async function postReview(state, review = NO_FINDINGS_REVIEW) { + const environment = { + CODEX_MODEL: "gpt-5.6-sol", + GITHUB_REPOSITORY: "block/buzz", + GITHUB_RUN_ID: "1234", + GITHUB_SERVER_URL: "https://github.com", + REVIEW_BASE_SHA: BASE_SHA, + REVIEW_COMMIT_RANGE: `${BASE_SHA}...${HEAD_SHA}`, + REVIEW_HEAD_REPO: "outside/buzz", + REVIEW_HEAD_SHA: HEAD_SHA, + REVIEW_JSON: JSON.stringify(review), + REVIEW_PR_NUMBER: "6816", + REVIEW_TRIGGER_ACTOR: "block-member", + }; + const previous = Object.fromEntries( + Object.keys(environment).map((key) => [key, process.env[key]]), + ); + Object.assign(process.env, environment); + + try { + await post(state); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } +} + +test("prepare binds a member command to the named head SHA", async () => { + const current = harness(); + await prepare(current); + + assert.deepEqual(current.failures, []); + assert.equal(current.outputs.get("head_sha"), HEAD_SHA); + assert.equal(current.outputs.get("base_sha"), BASE_SHA); + assert.notEqual(current.outputs.get("base_sha"), OLD_BASE_SHA); + assert.equal( + current.outputs.get("commit_range"), + `${BASE_SHA}...${HEAD_SHA}`, + ); + + const moved = harness({ pull: pullRequest({ headSha: OTHER_HEAD_SHA }) }); + await prepare(moved); + + assert.equal(moved.outputs.size, 0); + assert.equal(moved.failures.length, 1); + assert.match(moved.failures[0], new RegExp(OTHER_HEAD_SHA)); + assert.match( + moved.failures[0], + new RegExp(`@buzz-security-review ${OTHER_HEAD_SHA}`), + ); +}); + +test("prepare rejects review commands without a full exact SHA", async () => { + const state = harness(); + state.context.payload.comment.body = "@buzz-security-review"; + + await prepare(state); + + assert.equal(state.outputs.size, 0); + assert.equal(state.failures.length, 1); + assert.match(state.failures[0], //); +}); + +test("invalidation compares the complete base and head range", async () => { + const stale = harness({ + comments: [ + reviewComment( + `${MARKER}\n\nold review`, + ), + ], + }); + await stale.github.rest.issues.addLabels({ + issue_number: 6816, + labels: [CURRENT_REVIEW_LABEL], + }); + + await invalidate({ + github: stale.github, + context: stale.context, + core: stale.core, + prNumber: 6816, + existingOnly: true, + }); + + assert.equal(stale.updated.length, 1); + assert.match(stale.updated[0].body, /review required for the current range/); + assert.ok(stale.updated[0].body.includes(`${BASE_SHA}...${HEAD_SHA}`)); + assert.match( + stale.updated[0].body, + new RegExp(`@buzz-security-review ${HEAD_SHA}`), + ); + assert.equal(stale.removedLabels.length, 1); + + await invalidate({ + github: stale.github, + context: stale.context, + core: stale.core, + prNumber: 6816, + existingOnly: true, + }); + + assert.equal(stale.updated.length, 1); + assert.match(stale.info.at(-1), /already has the current stale-review notice/); + + const current = harness({ + comments: [ + reviewComment( + `${MARKER}\n\ncurrent review`, + ), + ], + }); + await invalidate({ + github: current.github, + context: current.context, + core: current.core, + prNumber: 6816, + existingOnly: true, + }); + + assert.equal(current.updated.length, 0); + assert.match(current.info.at(-1), /current range/); +}); + +test("base reconciliation batches every labeled review with a durable cursor", async () => { + const labeledIssues = Array.from({ length: 34 }, (_, index) => ({ + number: index + 1, + pull_request: {}, + })); + const first = harness({ labeledIssues }); + first.context.eventName = "repository_dispatch"; + first.context.payload.client_payload = { + after_pr: "1", + main_sha: BASE_SHA, + }; + + await prepareBaseReconciliation(first); + + const firstBatch = JSON.parse(first.outputs.get("pr_numbers")); + assert.equal(firstBatch.length, 32); + assert.equal(firstBatch[0], 2); + assert.equal(firstBatch.at(-1), 33); + assert.equal(first.outputs.get("main_sha"), BASE_SHA); + assert.equal(first.outputs.get("should_continue"), "true"); + assert.equal(first.outputs.get("next_after"), "33"); + assert.equal(first.outputs.get("next_pass"), "1"); + + const second = harness({ labeledIssues }); + second.context.eventName = "repository_dispatch"; + second.context.payload.client_payload = { + after_pr: "33", + main_sha: BASE_SHA, + }; + + await prepareBaseReconciliation(second); + + assert.deepEqual(JSON.parse(second.outputs.get("pr_numbers")), [34]); + assert.equal(second.outputs.get("should_continue"), "true"); + assert.equal(second.outputs.get("next_after"), "0"); + assert.equal(second.outputs.get("next_pass"), "2"); + + const disappearedTail = harness(); + disappearedTail.context.eventName = "repository_dispatch"; + disappearedTail.context.payload.client_payload = { + after_pr: "33", + main_sha: BASE_SHA, + pass: "1", + }; + + await prepareBaseReconciliation(disappearedTail); + + assert.deepEqual( + JSON.parse(disappearedTail.outputs.get("pr_numbers")), + [0], + ); + assert.equal(disappearedTail.outputs.get("should_continue"), "true"); + assert.equal(disappearedTail.outputs.get("next_after"), "0"); + assert.equal(disappearedTail.outputs.get("next_pass"), "2"); + + const retry = harness({ labeledIssues: [labeledIssues.at(-1)] }); + retry.context.eventName = "repository_dispatch"; + retry.context.payload.client_payload = { + after_pr: "0", + main_sha: BASE_SHA, + pass: "2", + }; + + await prepareBaseReconciliation(retry); + + assert.deepEqual(JSON.parse(retry.outputs.get("pr_numbers")), [34]); + assert.equal(retry.outputs.get("should_continue"), "false"); + assert.equal(retry.outputs.get("next_after"), "0"); + assert.equal(retry.outputs.get("next_pass"), "2"); +}); + +test("base reconciliation stops a continuation from an older main commit", async () => { + const state = harness({ + labeledIssues: [{ number: 6816, pull_request: {} }], + liveMainShas: [NEW_BASE_SHA], + }); + state.context.eventName = "repository_dispatch"; + state.context.payload.client_payload = { + after_pr: "256", + main_sha: BASE_SHA, + }; + + await prepareBaseReconciliation(state); + + assert.deepEqual(JSON.parse(state.outputs.get("pr_numbers")), [0]); + assert.equal(state.outputs.get("main_sha"), BASE_SHA); + assert.equal(state.outputs.get("should_continue"), "false"); + assert.equal(state.outputs.get("next_after"), "0"); + assert.equal(state.outputs.get("next_pass"), "1"); + assert.match(state.info.at(-1), /superseded main commit/); +}); + +test("GitHub rate limits use bounded retry delays", async () => { + const waits = []; + const warnings = []; + let attempts = 0; + const rateLimitError = Object.assign(new Error("secondary rate limit"), { + status: 403, + response: { + status: 403, + headers: { "retry-after": "0" }, + data: { message: "secondary rate limit" }, + }, + }); + + const result = await withGithubRetry( + async () => { + attempts += 1; + if (attempts < 3) { + throw rateLimitError; + } + return "completed"; + }, + { + core: { warning: (message) => warnings.push(message) }, + sleep: async (milliseconds) => waits.push(milliseconds), + }, + ); + + assert.equal(result, "completed"); + assert.equal(attempts, 3); + assert.deepEqual(waits, [1000, 1000]); + assert.equal(warnings.length, 2); +}); + +test("base reconciliation does not create comments on unreviewed PRs", async () => { + const state = harness(); + + await invalidate({ + github: state.github, + context: state.context, + core: state.core, + prNumber: 6816, + existingOnly: true, + }); + + assert.equal(state.created.length, 0); + assert.equal(state.updated.length, 0); +}); + +test("pull request updates invalidate member reviews without adding placeholders", async () => { + const reviewed = harness({ + pull: pullRequest({ headSha: OTHER_HEAD_SHA }), + comments: [ + reviewComment( + `${MARKER}\n\nold review`, + ), + ], + }); + reviewed.context.eventName = "pull_request_target"; + reviewed.context.payload.pull_request = { + number: 6816, + author_association: "MEMBER", + }; + await reviewed.github.rest.issues.addLabels({ + issue_number: 6816, + labels: [CURRENT_REVIEW_LABEL], + }); + + await invalidatePullRequestUpdate(reviewed); + + assert.equal(reviewed.updated.length, 1); + assert.ok( + reviewed.updated[0].body.includes(`${BASE_SHA}...${OTHER_HEAD_SHA}`), + ); + assert.equal(reviewed.removedLabels.length, 1); + + const unreviewed = harness(); + unreviewed.context.eventName = "pull_request_target"; + unreviewed.context.payload.pull_request = { + number: 6816, + author_association: "OWNER", + }; + + await invalidatePullRequestUpdate(unreviewed); + + assert.equal(unreviewed.created.length, 0); + assert.equal(unreviewed.updated.length, 0); + + const external = harness(); + external.context.eventName = "pull_request_target"; + external.context.payload.pull_request = { + number: 6816, + author_association: "CONTRIBUTOR", + }; + + await invalidatePullRequestUpdate(external); + + assert.equal(external.created.length, 1); + assert.match(external.created[0].body, /review required for the current range/); +}); + +test("post preserves finding text while rendering it as inert code", async () => { + const findingPath = "src/x)www.example.com/review.js"; + const state = harness({ files: [{ filename: findingPath }] }); + const summary = + "Keep