From db44a9118a0c4d38abd39b3335bf9246144a02ff Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 5 Aug 2026 21:48:58 +0100 Subject: [PATCH 1/5] Move non-compile CI jobs to GitHub-hosted runners The PR label classify and scope jobs, the regression-test check, the Claude Code review, and the weekly dependency audit all ran on `ubicloud-standard-8`. None of them compiles the workspace: they are single-threaded script or API-bound jobs (the audit installs `cargo-audit` as a prebuilt binary and only reads the lockfile), so an 8-vCPU paid runner is wasted on them. Axinite is a public repository, so `ubuntu-latest` runs these jobs at no cost. A July 2026 Ubicloud usage audit attributed roughly 1,600 billed premium-8 minutes per month to these five workflows alone. Compile-bound workflows (tests, staging CI, coverage, CodeScene coverage) stay on Ubicloud. Release-plz is left untouched because its jobs are gated to the `nearai` owner and never execute here. --- .github/workflows/audit.yml | 2 +- .github/workflows/claude-review.yml | 2 +- .github/workflows/pr-label-classify.yml | 2 +- .github/workflows/pr-label-scope.yml | 2 +- .github/workflows/regression-test-check.yml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 861d735e6..ad59b2baf 100644 --- a/.github/workflows/audit.yml +++ b/.github/workflows/audit.yml @@ -16,7 +16,7 @@ permissions: jobs: audit: - runs-on: ubicloud-standard-8 + runs-on: ubuntu-latest steps: - name: Checkout repository uses: actions/checkout@v6 diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index ea8500c58..d12592d50 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -18,7 +18,7 @@ jobs: review: name: Claude Code Review if: contains(github.event.pull_request.labels.*.name, 'staging-promotion') - runs-on: ubicloud-standard-8 + runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 with: diff --git a/.github/workflows/pr-label-classify.yml b/.github/workflows/pr-label-classify.yml index dfd8f7a0c..90f141de7 100644 --- a/.github/workflows/pr-label-classify.yml +++ b/.github/workflows/pr-label-classify.yml @@ -11,7 +11,7 @@ permissions: jobs: classify: - runs-on: ubicloud-standard-8 + runs-on: ubuntu-latest steps: - name: Checkout base branch uses: actions/checkout@v4 diff --git a/.github/workflows/pr-label-scope.yml b/.github/workflows/pr-label-scope.yml index b6fc69dcc..0e6e5147b 100644 --- a/.github/workflows/pr-label-scope.yml +++ b/.github/workflows/pr-label-scope.yml @@ -10,7 +10,7 @@ permissions: jobs: scope: - runs-on: ubicloud-standard-8 + runs-on: ubuntu-latest steps: - uses: actions/labeler@v7 with: diff --git a/.github/workflows/regression-test-check.yml b/.github/workflows/regression-test-check.yml index 3f0e04512..1be75fc56 100644 --- a/.github/workflows/regression-test-check.yml +++ b/.github/workflows/regression-test-check.yml @@ -6,7 +6,7 @@ on: jobs: regression-test: name: Regression test enforcement - runs-on: ubicloud-standard-8 + runs-on: ubuntu-latest steps: - name: Checkout repository uses: actions/checkout@v4 From 0e2bc8812c6a6bc72e719ca761eeb3268b9ce7f0 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 8 Aug 2026 01:48:16 +0200 Subject: [PATCH 2/5] Pin the CI runner split as a contract test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit moved five non-compile jobs to GitHub-hosted runners but left the split as an undocumented convention. The failure mode is silent: a new job copied from an existing workflow inherits whichever runner the template used, and nothing surfaces the mistake until the next billing audit. Add tests/workflow_contracts/runner_policy_test.py, which records the runner for every job in the repository. Adding a workflow or a job, or moving one between pools, now fails until RUNNER_POLICY is updated deliberately. The suite also asserts that no job on the free pool runs a compile command or installs a Rust build cache, and that each of the five migrated jobs kept its triggers, permissions, guards, and steps — so the runner move is provably behaviour-preserving. Reusable-workflow callers are matched by shape, not by SHA value, per the repository's Dependabot guidance. Document the policy in the developer's guide and record the decision, its rationale, and the alternatives considered in ADR 013. --- Makefile | 3 +- ...dr-013-split-ci-runners-by-compile-cost.md | 77 ++++ docs/contents.md | 4 + docs/developers-guide.md | 30 ++ .../workflow_contracts/runner_policy_test.py | 394 ++++++++++++++++++ 5 files changed, 507 insertions(+), 1 deletion(-) create mode 100644 docs/adr-013-split-ci-runners-by-compile-cost.md create mode 100644 tests/workflow_contracts/runner_policy_test.py diff --git a/Makefile b/Makefile index 2ae853904..ade9c90a5 100644 --- a/Makefile +++ b/Makefile @@ -179,7 +179,8 @@ test-matrix-cargo: $(CARGO) test --features postgres,libsql-test-helpers,html-to-markdown -- --nocapture $(CARGO) test --manifest-path $(GITHUB_TOOL_MANIFEST) -- --nocapture -# Validate the mutation-testing caller workflow contract. +# Validate the GitHub Actions workflow contracts: reusable-workflow callers, +# the isolated CodeScene coverage job, and the per-job runner policy. test-workflow-contracts: uv run --with 'pytest>=8' --with 'pyyaml>=6' pytest tests/workflow_contracts -q diff --git a/docs/adr-013-split-ci-runners-by-compile-cost.md b/docs/adr-013-split-ci-runners-by-compile-cost.md new file mode 100644 index 000000000..06464d05c --- /dev/null +++ b/docs/adr-013-split-ci-runners-by-compile-cost.md @@ -0,0 +1,77 @@ +# ADR-013 — Split CI runners by compile cost + +**Status:** Accepted **Date:** 2026-08-05 **Deciders:** `@leynos` + +## Context + +Every Linux job in the repository ran on `ubicloud-standard-8`, an 8-vCPU paid +runner chosen because a full workspace build is the dominant CI cost. That +choice was applied uniformly rather than per job, so it also covered work that +never compiles anything: the two pull-request labelling workflows, the +regression-test check, the Claude Code review, and the weekly dependency +audit. Those jobs are single-threaded shell scripts, an action call, or an +API-bound agent run; the extra vCPUs sit idle. + +A July 2026 Ubicloud usage audit attributed roughly 1,600 billed premium-8 +minutes per month to those five workflows. Axinite is a public repository, so +GitHub-hosted `ubuntu-latest` runners execute the same work at no cost, and +the standard 2-vCPU hosted runner is not the bottleneck for any of them. + +## Decision + +Select the runner per job, from the job's compile cost: + +- A job that compiles the workspace — `cargo build`, `cargo test`, + `cargo nextest`, `cargo clippy`, `cargo llvm-cov`, `cargo component`, or the + `make` targets that wrap them — runs on `ubicloud-standard-8`. +- A job that does not compile runs on GitHub-hosted `ubuntu-latest`. + +The five non-compiling workflows moved accordingly. Windows jobs keep +`windows-latest` and the release workflow keeps its pinned `ubuntu-22.04` +images, both for reproducibility rather than cost. + +The policy is enforced by `tests/workflow_contracts/runner_policy_test.py`, +which records the runner for every job in the repository. Adding a job, or +moving one between pools, fails until the recorded policy is updated +deliberately. The same suite asserts that no job on the free pool runs a +compile command or installs a Rust build cache. + +## Rationale + +Runner selection is a per-job property, not a per-repository one. Paying for +vCPUs a job cannot use is waste with no compensating benefit, and the +alternative — leaving everything on the paid pool because it is simpler — +costs roughly 1,600 billed minutes a month for jobs whose wall-clock time is +dominated by network round trips and process startup. + +Encoding the rule as a contract test rather than a comment matters because the +failure mode is silent: a new job copied from an existing workflow inherits +whichever runner the template used, and nothing surfaces the mistake until the +next billing audit. A test that enumerates every job turns that into a +review-time question. + +`release-plz.yml` is deliberately untouched. Its jobs are gated to the `nearai` +repository owner and never execute here, so moving them would change nothing +observable while diverging from upstream. + +## Consequences + +- Non-compile pull-request feedback moves to the free pool, so it competes for + GitHub's shared hosted-runner concurrency rather than Ubicloud's. These jobs + run on every pull-request event, so any queueing regression is visible + immediately. +- Adding a workflow or a job now requires an edit to `RUNNER_POLICY` in + `tests/workflow_contracts/runner_policy_test.py`. That is the intended + friction: the runner choice becomes an explicit review decision. +- A free-runner job that later grows a build step fails its contract test + rather than silently running a compile on a 2-vCPU machine. + +## Alternatives considered + +- **Leave everything on Ubicloud.** Simplest, and wrong: it keeps paying for + capacity that five workflows demonstrably cannot use. +- **Move every Linux job to `ubuntu-latest`.** Free, but the workspace build + is the reason the paid pool exists; hosted runners lack both the vCPUs and + the disk headroom the coverage and end-to-end jobs need. +- **Document the split without a test.** Rejected for the silent-inheritance + failure mode described above. diff --git a/docs/contents.md b/docs/contents.md index 78761ad57..4fe6c0976 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -254,3 +254,7 @@ - [ADR 012: Monotonic clock seam for build duration measurement](adr-012-monotonic-clock-seam-for-build-duration.md) records why `BuildSoftwareTool` uses an `Instant`-backed duration seam instead of `mockable::Clock` for elapsed-time assertions. +- [ADR 013: Split CI runners by compile cost](adr-013-split-ci-runners-by-compile-cost.md) + records why compile-bound jobs stay on the paid Ubicloud pool while + non-compiling jobs run on GitHub-hosted runners, and how the split is + enforced. diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 54ea15afd..5dd2ab26a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -76,6 +76,36 @@ that assert on printed startup or boot-screen content — for example, the `print_startup_info_matches_snapshot` test in `src/startup/boot.rs`. The crate is compiled only when running tests and has no effect on the production binary. +### Runner selection + +Runners are chosen per job, from the job's compile cost: + +- A job that compiles the workspace runs on `ubicloud-standard-8`. That covers + `test.yml`, `code_style.yml`, `coverage.yml`, `codescene-coverage.yml`, + `e2e.yml`, and `staging-ci.yml` — anything invoking `cargo build`, + `cargo test`, `cargo nextest`, `cargo clippy`, `cargo llvm-cov`, + `cargo component`, or the `make` targets that wrap them. +- A job that does not compile runs on GitHub-hosted `ubuntu-latest`, which is + free for this public repository. That covers `pr-label-classify.yml`, + `pr-label-scope.yml`, `regression-test-check.yml`, `claude-review.yml`, and + `audit.yml`. These are single-threaded shell scripts, action calls, or + API-bound agent runs; the scheduled audit installs `cargo-audit` as a + prebuilt binary and only reads the lockfile. + +Windows jobs use `windows-latest` and `release.yml` pins `ubuntu-22.04`, both +for reproducibility rather than cost. `release-plz.yml` is left on Ubicloud +because its jobs are gated to the `nearai` repository owner and never execute +here. + +`tests/workflow_contracts/runner_policy_test.py` records the runner for every +job in the repository and fails when the mapping drifts, so adding a job or +moving one between pools requires a deliberate edit to `RUNNER_POLICY`. The +same suite asserts that no job on the free pool runs a compile command or +installs a Rust build cache. Run it with `make test-workflow-contracts`. + +The rationale is recorded in +[ADR 013](adr-013-split-ci-runners-by-compile-cost.md). + ### Workflow pins and Dependabot Dependabot owns the upgrade of GitHub Actions and reusable workflows, including diff --git a/tests/workflow_contracts/runner_policy_test.py b/tests/workflow_contracts/runner_policy_test.py new file mode 100644 index 000000000..fb56b0797 --- /dev/null +++ b/tests/workflow_contracts/runner_policy_test.py @@ -0,0 +1,394 @@ +"""Contract tests for the CI runner-selection policy. + +Axinite splits its CI estate across two runner pools. Compile-bound jobs stay +on the paid `ubicloud-standard-8` pool, where the extra vCPUs pay for +themselves against a full workspace build. Glue jobs — labelling scripts, the +regression-test check, the Claude review, and the scheduled dependency audit — +run on GitHub-hosted `ubuntu-latest`, which is free for this public repository +and fast enough for single-threaded script and API-bound work. + +That split is a policy, not an accident, so it is pinned here. The tests fail +when a job's runner drifts, when a new job appears without a recorded runner, +and when a job placed on the free pool starts compiling the workspace (which +would make the free pool the wrong choice for it). + +Run via ``make test-workflow-contracts``. +""" + +from __future__ import annotations + +import re +import typing as typ +from pathlib import Path + +import pytest +import yaml + +WORKFLOW_DIR = Path(__file__).resolve().parents[2] / ".github" / "workflows" + +UBICLOUD = "ubicloud-standard-8" +GITHUB_HOSTED_LINUX = "ubuntu-latest" + +#: Every job in the repository, with the runner it is expected to request. +#: A job that calls a reusable workflow declares no runner of its own and is +#: recorded as ``None``. Adding or moving a job must be a deliberate edit here. +RUNNER_POLICY: typ.Final[dict[str, dict[str, str | None]]] = { + "audit.yml": {"audit": GITHUB_HOSTED_LINUX}, + "claude-review.yml": {"review": GITHUB_HOSTED_LINUX}, + "code_style.yml": { + "format": UBICLOUD, + "clippy": UBICLOUD, + "clippy-windows": "windows-latest", + "code-style": UBICLOUD, + }, + "codescene-coverage.yml": {"coverage-check": UBICLOUD}, + "coverage.yml": { + "coverage": UBICLOUD, + "e2e-coverage": UBICLOUD, + "coverage-gate": UBICLOUD, + }, + "dependabot-automerge.yml": {"automerge": None}, + "e2e.yml": {"build": UBICLOUD, "test": UBICLOUD, "e2e": UBICLOUD}, + "mutation-testing.yml": {"mutation": None}, + "pr-label-classify.yml": {"classify": GITHUB_HOSTED_LINUX}, + "pr-label-scope.yml": {"scope": GITHUB_HOSTED_LINUX}, + "regression-test-check.yml": {"regression-test": GITHUB_HOSTED_LINUX}, + "release-plz.yml": {"release-plz-release": UBICLOUD, "release-plz-pr": UBICLOUD}, + "release.yml": { + "plan": "ubuntu-22.04", + "build-local-artifacts": "${{ matrix.runner }}", + "build-global-artifacts": "ubuntu-22.04", + "build-wasm-extensions": "ubuntu-22.04", + "host": "ubuntu-22.04", + "update-registry-checksums": "ubuntu-22.04", + "announce": "ubuntu-22.04", + }, + "staging-ci.yml": { + "check-changes": UBICLOUD, + "tests": None, + "e2e": None, + "create-promotion-pr": UBICLOUD, + "gate": UBICLOUD, + "update-tag": UBICLOUD, + "report": UBICLOUD, + }, + "test.yml": { + "audit": UBICLOUD, + "tests": UBICLOUD, + "telegram-tests": UBICLOUD, + "windows-build": "windows-latest", + "wasm-wit-compat": UBICLOUD, + "docker-build": UBICLOUD, + "version-check": UBICLOUD, + "run-tests": UBICLOUD, + }, +} + +#: The jobs migrated off the paid pool. These must stay free-runner eligible: +#: no workspace compilation, no Rust build cache. +FREE_RUNNER_JOBS: typ.Final[tuple[tuple[str, str], ...]] = ( + ("audit.yml", "audit"), + ("claude-review.yml", "review"), + ("pr-label-classify.yml", "classify"), + ("pr-label-scope.yml", "scope"), + ("regression-test-check.yml", "regression-test"), +) + +#: Workflows whose jobs compile the workspace and therefore stay on Ubicloud. +COMPILE_BOUND_WORKFLOWS: typ.Final[tuple[str, ...]] = ( + "code_style.yml", + "codescene-coverage.yml", + "coverage.yml", + "e2e.yml", + "staging-ci.yml", + "test.yml", +) + +#: Commands that compile the workspace. `make audit` and `cargo audit` only +#: read the lockfile, so `audit` is deliberately absent from this pattern. +COMPILE_COMMAND_RE: typ.Final[re.Pattern[str]] = re.compile( + r""" + \bcargo(?:\s+\+\S+)?\s+ + (?:build|test|nextest|check|clippy|bench|component|llvm-cov|mutants|doc)\b + | \bmake\s+ + (?:all|build[\w-]*|test[\w-]*|lint[\w-]*|check-fmt|typecheck|install)\b + | \./scripts/build-wasm-extensions\.sh + """, + re.VERBOSE, +) + +#: Actions that only make sense for a compiling job. +COMPILE_ACTION_PREFIXES: typ.Final[tuple[str, ...]] = ( + "Swatinem/rust-cache", + "taiki-e/install-action@cargo-llvm-cov", + "taiki-e/install-action@cargo-nextest", +) + +SHA_RE: typ.Final[re.Pattern[str]] = re.compile(r"^[0-9a-f]{40}$") + + +def _load(name: str) -> dict[str, object]: + """Parse a workflow file into a mapping.""" + workflow = yaml.safe_load((WORKFLOW_DIR / name).read_text(encoding="utf-8")) + assert isinstance(workflow, dict), f"{name} must parse as a mapping" + return workflow + + +def _jobs(name: str) -> dict[str, dict[str, object]]: + """Return a workflow's job mappings, keyed by job identifier.""" + jobs = _load(name).get("jobs") + assert isinstance(jobs, dict), f"{name} must declare a jobs mapping" + assert all(isinstance(job, dict) for job in jobs.values()), ( + f"every job in {name} must be a mapping" + ) + return typ.cast("dict[str, dict[str, object]]", jobs) + + +def _job(name: str, job_id: str) -> dict[str, object]: + """Return a single job mapping.""" + jobs = _jobs(name) + assert job_id in jobs, f"{name} must declare a {job_id!r} job" + return jobs[job_id] + + +def _triggers(workflow: dict[str, object]) -> object: + """Return a workflow's ``on:`` block. + + PyYAML resolves an unquoted ``on`` key to the boolean ``True`` under the + YAML 1.1 rules it implements, so both spellings must be accepted. + """ + if "on" in workflow: + return workflow["on"] + return workflow.get(True) + + +def _steps(job: dict[str, object]) -> list[dict[str, object]]: + """Return a job's ordered step mappings.""" + steps = job.get("steps") + assert isinstance(steps, list), "the job must declare steps" + assert all(isinstance(step, dict) for step in steps), ( + "every step must be a mapping" + ) + return [step for step in steps if isinstance(step, dict)] + + +def _step_identities(job: dict[str, object]) -> list[str]: + """Return each step's name, falling back to the action it invokes.""" + return [str(step.get("name", step.get("uses"))) for step in _steps(job)] + + +def _workflow_files() -> list[str]: + """Return every workflow file name, sorted.""" + return sorted(path.name for path in WORKFLOW_DIR.glob("*.yml")) + + +def test_workflow_inventory_matches_the_recorded_policy() -> None: + """Every workflow file is accounted for in the runner policy.""" + assert _workflow_files() == sorted(RUNNER_POLICY), ( + "a workflow was added or removed without updating RUNNER_POLICY" + ) + + +@pytest.mark.parametrize("name", sorted(RUNNER_POLICY)) +def test_each_workflow_requests_its_recorded_runners(name: str) -> None: + """Each job requests exactly the runner the policy records for it.""" + actual = {job_id: job.get("runs-on") for job_id, job in _jobs(name).items()} + assert actual == RUNNER_POLICY[name], ( + f"{name} runner assignments drifted from the recorded policy; " + "update RUNNER_POLICY only when the change is deliberate" + ) + + +@pytest.mark.parametrize("name", sorted(RUNNER_POLICY)) +def test_jobs_without_a_runner_call_a_pinned_reusable_workflow(name: str) -> None: + """A job may omit ``runs-on`` only when it delegates to another workflow. + + A local caller inherits the callee's runners, which the policy already + covers. A remote caller must be pinned to a commit SHA; the value itself + is Dependabot's to bump, so only its shape is asserted. + """ + for job_id, job in _jobs(name).items(): + if job.get("runs-on") is not None: + continue + uses = job.get("uses") + assert isinstance(uses, str), ( + f"{name}:{job_id} declares no runner, so it must call a reusable " + "workflow via `uses`" + ) + if uses.startswith("./"): + callee = uses.removeprefix("./.github/workflows/") + assert callee in RUNNER_POLICY, ( + f"{name}:{job_id} calls {uses!r}, which is not in RUNNER_POLICY" + ) + continue + assert SHA_RE.match(uses.split("@")[-1]), ( + f"{name}:{job_id} must pin its reusable workflow to a full commit SHA" + ) + + +@pytest.mark.parametrize(("name", "job_id"), FREE_RUNNER_JOBS) +def test_migrated_jobs_use_the_github_hosted_runner(name: str, job_id: str) -> None: + """The migrated glue jobs run on the free GitHub-hosted pool.""" + assert _job(name, job_id).get("runs-on") == GITHUB_HOSTED_LINUX, ( + f"{name}:{job_id} must stay on {GITHUB_HOSTED_LINUX}; it does no " + "workspace compilation, so the paid pool buys nothing" + ) + + +@pytest.mark.parametrize(("name", "job_id"), FREE_RUNNER_JOBS) +def test_migrated_jobs_do_not_compile_the_workspace(name: str, job_id: str) -> None: + """No free-runner job runs a compile command or a Rust build cache.""" + for step in _steps(_job(name, job_id)): + command = step.get("run") + if isinstance(command, str): + match = COMPILE_COMMAND_RE.search(command) + assert match is None, ( + f"{name}:{job_id} runs {match.group(0)!r}, which compiles the " + f"workspace; move the job back to {UBICLOUD} or drop the step" + ) + uses = str(step.get("uses", "")) + assert not uses.startswith(COMPILE_ACTION_PREFIXES), ( + f"{name}:{job_id} uses {uses!r}, which only pays off for a " + f"compiling job; move the job back to {UBICLOUD}" + ) + + +@pytest.mark.parametrize("name", COMPILE_BOUND_WORKFLOWS) +def test_compile_bound_workflows_stay_off_the_free_linux_pool(name: str) -> None: + """Compile-bound Linux jobs keep the paid runner.""" + for job_id, job in _jobs(name).items(): + runner = job.get("runs-on") + assert runner != GITHUB_HOSTED_LINUX, ( + f"{name}:{job_id} compiles the workspace, so it must not move to " + f"{GITHUB_HOSTED_LINUX}" + ) + + +def test_audit_job_reads_the_lockfile_without_building() -> None: + """The scheduled audit installs a prebuilt binary and audits only.""" + workflow = _load("audit.yml") + assert _triggers(workflow) == { + "schedule": [{"cron": "33 7 * * 1"}], + "workflow_dispatch": None, + }, "the audit must remain a weekly scheduled run with a manual trigger" + assert workflow.get("permissions") == {"contents": "read"}, ( + "the audit needs only read access to contents" + ) + + job = _job("audit.yml", "audit") + assert _step_identities(job) == [ + "Checkout repository", + "Install Rust", + "Install cargo-audit", + "Audit dependencies", + ], "the audit job's steps must stay unchanged by the runner move" + + installer = _steps(job)[2] + assert str(installer.get("uses", "")).startswith("taiki-e/install-action@"), ( + "cargo-audit must arrive as a prebuilt binary, not a source build" + ) + assert installer.get("with") == {"tool": "cargo-audit"}, ( + "the installer must fetch cargo-audit" + ) + assert _steps(job)[3].get("run") == "make audit", ( + "the audit step must read the lockfile via `make audit`" + ) + + +def test_claude_review_stays_label_gated_and_build_free() -> None: + """The review job keeps its label guard and its no-build instruction.""" + workflow = _load("claude-review.yml") + assert _triggers(workflow) == {"pull_request": {"types": ["labeled"]}}, ( + "the review must trigger only when a pull request is labelled" + ) + assert workflow.get("permissions") == { + "contents": "read", + "pull-requests": "write", + "issues": "write", + "id-token": "write", + }, "the review's permissions must survive the runner move" + + job = _job("claude-review.yml", "review") + assert job.get("if") == ( + "contains(github.event.pull_request.labels.*.name, 'staging-promotion')" + ), "the review must stay gated on the staging-promotion label" + + action = _steps(job)[1] + assert str(action.get("uses", "")).startswith("anthropics/claude-code-action@"), ( + "the review must run through the Claude Code action" + ) + prompt = action.get("with", {}).get("prompt") + assert isinstance(prompt, str), "the review action must supply a prompt" + assert "Do NOT check build signal or attempt to build/test the code" in prompt, ( + "the review prompt must keep instructing the agent not to build, since " + "the free runner is not provisioned for a workspace compile" + ) + + +def test_label_workflows_run_scripts_without_a_toolchain() -> None: + """Both labelling workflows remain checkout-plus-script jobs.""" + classify = _load("pr-label-classify.yml") + scope = _load("pr-label-scope.yml") + expected_trigger = { + "pull_request_target": {"types": ["opened", "synchronize", "reopened"]} + } + assert _triggers(classify) == expected_trigger, ( + "classify must keep its pull_request_target trigger" + ) + assert _triggers(scope) == expected_trigger, ( + "scope must keep its pull_request_target trigger" + ) + assert classify.get("permissions") == { + "contents": "read", + "pull-requests": "write", + "issues": "read", + }, "classify needs issues read access for the contributor-count search" + assert scope.get("permissions") == { + "contents": "read", + "pull-requests": "write", + }, "scope needs only pull-request write access" + + classify_job = _job("pr-label-classify.yml", "classify") + assert _step_identities(classify_job) == ["Checkout base branch", "Classify PR"], ( + "classify must stay a checkout plus a single script step" + ) + assert _steps(classify_job)[1].get("run") == "bash .github/scripts/pr-labeler.sh", ( + "classify must invoke the labeller script directly" + ) + + scope_job = _job("pr-label-scope.yml", "scope") + scope_steps = _steps(scope_job) + assert len(scope_steps) == 1, "scope must remain a single labeller step" + assert str(scope_steps[0].get("uses", "")).startswith("actions/labeler@"), ( + "scope must delegate to the labeler action" + ) + assert scope_steps[0].get("with") == { + "configuration-path": ".github/labeler.yml", + "sync-labels": False, + }, "scope must stay additive against the checked-in labeller configuration" + + +def test_regression_check_is_a_git_and_grep_job() -> None: + """The regression check needs full history but no toolchain.""" + workflow = _load("regression-test-check.yml") + assert _triggers(workflow) == {"pull_request": None}, ( + "the regression check must run on every pull request" + ) + + job = _job("regression-test-check.yml", "regression-test") + assert _step_identities(job) == [ + "Checkout repository", + "Check for regression tests", + ], "the regression check must stay a checkout plus a single script step" + + checkout = _steps(job)[0] + assert str(checkout.get("uses", "")).startswith("actions/checkout@") + assert checkout.get("with") == {"fetch-depth": 0}, ( + "the check diffs against the base ref, so it needs full history" + ) + + script = _steps(job)[1].get("run") + assert isinstance(script, str), "the check must declare a script" + assert "git diff" in script and "git log" in script, ( + "the check must remain a git-history inspection, not a build" + ) From c8eda3e9deaacefa49042df3245444600638e788 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 8 Aug 2026 02:00:48 +0200 Subject: [PATCH 3/5] Allow upstream GitHub spellings in the runner policy test The runner policy table records GitHub's own job identifiers and trigger values verbatim, so `build-local-artifacts`, `build-global-artifacts`, and the `labeled` pull-request trigger type cannot be respelled without breaking the assertions they anchor. Add narrow ignore patterns for those three literals to the typos overlay rather than excluding the whole file, so the test's prose stays under the Oxford spelling gate. Reword the one occurrence that was prose rather than an upstream identifier. --- tests/workflow_contracts/runner_policy_test.py | 2 +- typos.local.toml | 2 ++ typos.toml | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/workflow_contracts/runner_policy_test.py b/tests/workflow_contracts/runner_policy_test.py index fb56b0797..5471ce6df 100644 --- a/tests/workflow_contracts/runner_policy_test.py +++ b/tests/workflow_contracts/runner_policy_test.py @@ -360,7 +360,7 @@ def test_label_workflows_run_scripts_without_a_toolchain() -> None: scope_steps = _steps(scope_job) assert len(scope_steps) == 1, "scope must remain a single labeller step" assert str(scope_steps[0].get("uses", "")).startswith("actions/labeler@"), ( - "scope must delegate to the labeler action" + "scope must delegate to the upstream labelling action" ) assert scope_steps[0].get("with") == { "configuration-path": ".github/labeler.yml", diff --git a/typos.local.toml b/typos.local.toml index cb081d526..668f67b27 100644 --- a/typos.local.toml +++ b/typos.local.toml @@ -41,6 +41,7 @@ ignore = [ '\bpr-labeler\b', '"labeler test: success"', 'types: \[[^]]*\blabeled\b', + '"types": \["labeled"\]', '\.github/labeler\.yml', '\b(?:EMBEDDED_CATALOG|MAX_REMOTE_TOOL_CATALOG_BYTES|REMOTE_TOOL_CATALOG_PATH|REMOTE_TOOL_CATALOG_ROUTE)\b', '\bgrammers_(?:crypto|mtproto|tl_types)\b', @@ -63,6 +64,7 @@ ignore = [ '\b(?:manifest|self)\.artifacts\b', '\.artifacts\["wasm32-wasip2"\]', '\bartifacts_matrix\b', + '"build-(?:local|global)-artifacts"', "Reddit\\ \u2014\\ Best\\ way\\ to\\ organise\\ tests\\ in\\ Rust,\\ accessed\\ on\\ July\\ 15,\\ 2025,", "\\ \\ \\ \\ \\ \\ \\ \\ \"Analyze\\ an\\ image\\ using\\ a\\ vision\\-capable\\ AI\\ model\\.\\ Provide\\ a\\ workspace\\ path\\ to\\ the\\ image\\ and\\ an\\ optional\\ analysis\\ question\\.\"", "\\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \"artifact_path\":\\ result\\.artifact_path\\.display\\(\\)\\.to_string\\(\\),", diff --git a/typos.toml b/typos.toml index fb17a0552..752f4d4b3 100644 --- a/typos.toml +++ b/typos.toml @@ -84,11 +84,13 @@ locale = "en-gb" extend-ignore-re = [ "\"(?:LEFT|START)\", \"CENTER\", \"(?:RIGHT|END)\"", "\"(?:artifact_path|artifacts|catalog|catalog_count|catalog_error|catalog_version)\"\\s*:", + "\"build-(?:local|global)-artifacts\"", "\"caf%C3%A9\"", "\"color\"\\s*:", "\"hello worl\\.\\.\\.\"", "\"labeler test: success\"", "\"required\": \\[\"inpt\"\\]", + "\"types\": \\[\"labeled\"\\]", "#\\[command\\(color = ColorChoice::Auto\\)\\]", "(?:::|\\.)analyze\\b", "(?:^|[;{]\\s*)color\\s*:", From f872cb09ae93227ee01be82e86f8486680b064cc Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 8 Aug 2026 03:00:04 +0200 Subject: [PATCH 4/5] Make test arrangement helpers fallible for Whitaker `whitaker-installer` 0.2.7 narrowed the test-detection heuristic behind `no_expect_outside_tests`: a function inside a `#[cfg(test)]` module is no longer treated as test code unless it is itself a recognised test. Attributes are gone by the time the lint sees HIR, so rstest fixtures, rstest-bdd steps and plain helper functions all read as production code. That surfaced 102 findings across 32 modules. Convert them the way the suite intends: arrangement is not a verdict, so a helper that can fail returns `Result` and propagates, and only the test body unwraps. No `#[allow]`, no `#[expect]`, and no helper renamed to look like a test. Three cases needed judgement rather than a mechanical rewrite: - The `tokio::spawn`ed test servers cannot propagate from inside the closure, so the fallible work moved out of the closure and the serve result now surfaces through the join handle. Every call site already aborted the handle and discarded the result, so no verdict changes. - Trait-impl methods with fixed signatures (`complete_with_tools`, `set_setting`) were already fallible, so a poisoned lock now maps to the trait's own error rather than panicking. A poisoned lock means the double panicked while holding it, which is worth reporting, not swallowing. - rstest-bdd's return classifier only recognises a bare `Result<..>`, not the `anyhow::Result` alias, so fallible steps spell the type out in full. Aliased steps would have discarded their errors silently. Splitting the pipeline and dispatch test doubles out of their parent modules keeps both within the 400-line module limit that the added error handling pushed them over. The CI pin stays at 0.2.6; this only removes the backlog that a future bump would otherwise surface all at once. --- src/agent/dispatcher/tests/auth.rs | 16 +- src/agent/dispatcher/tests/image_sentinel.rs | 20 +- src/agent/dispatcher/tests/loop_guard.rs | 17 +- src/agent/dispatcher/tests/pipeline.rs | 222 +--------------- .../dispatcher/tests/pipeline/support.rs | 237 ++++++++++++++++++ .../tests/skill_bundle_context_bdd.rs | 80 ++++-- src/agent/dispatcher/tests/skills.rs | 38 +-- src/agent/scheduler/tests/approval.rs | 29 ++- .../thread_ops/document_store/tests/mod.rs | 25 +- src/bootstrap/tests/env_format.rs | 28 ++- src/bootstrap/tests/migration.rs | 18 +- src/bootstrap/tests/migration_disk_to_db.rs | 24 +- src/bootstrap/tests/migration_rename.rs | 16 +- src/bootstrap/tests/migration_support.rs | 68 ++--- .../wasm/wrapper/tests/channel/typing.rs | 21 +- src/channels/wasm/wrapper/tests/dispatch.rs | 204 ++------------- .../wrapper/tests/dispatch/attachments.rs | 182 ++++++++++++++ .../web/handlers/skills/tests/helpers.rs | 62 +++-- .../web/handlers/skills/tests/json.rs | 42 ++-- .../web/handlers/skills/tests/multipart.rs | 150 +++++++---- src/channels/web/server/tests/fixtures.rs | 31 ++- src/channels/web/server/tests/oauth.rs | 41 ++- src/channels/web/server/tests/relay_oauth.rs | 7 +- src/history/migrations/tests.rs | 20 +- .../migrations/tests/postgres_testing.rs | 25 +- .../rig_adapter/tests/unsupported_params.rs | 43 +++- .../api/tests/fixtures/remote_tool_helpers.rs | 29 ++- .../api/tests/remote_tools/execute.rs | 15 +- src/skills/registry/tests/discovery.rs | 90 ++++--- src/skills/registry/tests/fixtures.rs | 48 ++-- src/skills/registry/tests/install.rs | 33 ++- .../registry/tests/install/lifecycle.rs | 23 +- src/skills/registry/tests/install/payloads.rs | 28 ++- src/skills/registry/tests/prop_tests.rs | 37 ++- .../skill_tools/tests/read_file_adapter.rs | 96 ++++--- .../schema_validator/tests/fixture_groups.rs | 10 +- src/worker/api/tests/client_methods.rs | 15 +- src/worker/api/tests/transport_types.rs | 26 +- .../claude_bridge/tests/claude_fs_setup.rs | 19 +- src/worker/container/tests/hosted_fidelity.rs | 16 +- src/worker/container/tests/pre_loop.rs | 27 +- src/worker/container/tests/remote_tools.rs | 15 +- src/worker/container/tests/shutdown.rs | 13 +- 43 files changed, 1332 insertions(+), 874 deletions(-) create mode 100644 src/agent/dispatcher/tests/pipeline/support.rs create mode 100644 src/channels/wasm/wrapper/tests/dispatch/attachments.rs diff --git a/src/agent/dispatcher/tests/auth.rs b/src/agent/dispatcher/tests/auth.rs index e4efa0e07..a3e6c6993 100644 --- a/src/agent/dispatcher/tests/auth.rs +++ b/src/agent/dispatcher/tests/auth.rs @@ -1,5 +1,7 @@ //! Auth detection tests. +use anyhow::Context as _; + use super::super::{check_auth_required, parse_auth_result}; use super::*; @@ -13,14 +15,17 @@ fn check_auth_json(tool_name: &str, json: serde_json::Value) -> Option<(String, /// Assert that an auth-awaiting detection result is `Some`, and that the /// returned name and instructions match the expected values. +/// +/// Returns an error when detection did not fire, leaving the calling test to +/// decide how that failure surfaces. fn assert_auth_detected( detected: Option<(String, String)>, expected_name: &str, expected_instructions_fragment: &str, -) { +) -> anyhow::Result<()> { assert!(detected.is_some(), "expected auth detection to fire"); let (name, instructions) = - detected.expect("expected auth detection to fire and return (name, instructions)"); + detected.context("expected auth detection to fire and return (name, instructions)")?; assert_eq!(name, expected_name); assert!( instructions.contains(expected_instructions_fragment), @@ -28,6 +33,7 @@ fn assert_auth_detected( expected_instructions_fragment, instructions, ); + Ok(()) } #[test] @@ -45,7 +51,8 @@ fn test_detect_auth_awaiting_positive() { ), "telegram", "Telegram Bot API", - ); + ) + .expect("expected auth detection to fire"); } #[test] @@ -129,7 +136,8 @@ fn test_detect_auth_awaiting_tool_activate() { ), "slack", "Slack Bot", - ); + ) + .expect("expected auth detection to fire"); } #[test] diff --git a/src/agent/dispatcher/tests/image_sentinel.rs b/src/agent/dispatcher/tests/image_sentinel.rs index f99e1fb61..90b71b797 100644 --- a/src/agent/dispatcher/tests/image_sentinel.rs +++ b/src/agent/dispatcher/tests/image_sentinel.rs @@ -148,7 +148,14 @@ fn sentinel_json(data: Option<&str>, path: Option<&str>) -> String { serde_json::Value::Object(sentinel).to_string() } -async fn run_image_generate_and_count_statuses(data_url: Option<&str>) -> (bool, usize) { +/// Run `image_generate` through the sentinel path and report whether a +/// sentinel was detected along with the number of statuses broadcast. +/// +/// Returns an error if the captured-status lock is poisoned; a poisoned lock +/// means the stub channel panicked, so the recorded statuses cannot be trusted. +async fn run_image_generate_and_count_statuses( + data_url: Option<&str>, +) -> anyhow::Result<(bool, usize)> { let (channels, statuses) = new_stubbed_channels("test-chan").await; let agent = build_agent_with_stub_channel(channels); let session = Arc::new(Mutex::new(Session::new("test-user"))); @@ -158,8 +165,11 @@ async fn run_image_generate_and_count_statuses(data_url: Option<&str>) -> (bool, let result = delegate .maybe_emit_image_sentinel("image_generate", &output) .await; - let count = statuses.lock().expect("statuses lock poisoned").len(); - (result, count) + let count = statuses + .lock() + .map_err(|_| anyhow::anyhow!("statuses lock poisoned"))? + .len(); + Ok((result, count)) } struct ImageSentinelHarness { @@ -232,7 +242,9 @@ async fn delegate_skips_broadcast_for_invalid_data_urls( #[case] data_url: Option<&str>, #[case] expected_message: &str, ) { - let (result, count) = run_image_generate_and_count_statuses(data_url).await; + let (result, count) = run_image_generate_and_count_statuses(data_url) + .await + .expect("image_generate run should complete without a poisoned lock"); assert!( result, diff --git a/src/agent/dispatcher/tests/loop_guard.rs b/src/agent/dispatcher/tests/loop_guard.rs index bcd96066f..02b72dfba 100644 --- a/src/agent/dispatcher/tests/loop_guard.rs +++ b/src/agent/dispatcher/tests/loop_guard.rs @@ -188,16 +188,23 @@ fn build_test_agent_config(max_tool_iterations: usize) -> AgentConfig { } /// Assert that the timeout-wrapped agentic loop result is a text response. +/// +/// Returns an error if the loop timed out or the dispatcher returned `Err`, +/// leaving the calling test to decide how the failure surfaces. fn assert_agentic_loop_text_response( result: Result, tokio::time::error::Elapsed>, expected_text: &str, -) { +) -> anyhow::Result<()> { assert!( result.is_ok(), "Dispatcher timed out -- max_iterations guard failed to terminate the loop" ); - let inner = result.expect("test timed out or dispatcher context lost"); - match inner.expect("Expected Ok(AgenticLoopResult) but dispatcher returned Err") { + let inner = + result.map_err(|e| anyhow::anyhow!("test timed out or dispatcher context lost: {e}"))?; + let outcome = inner.map_err(|e| { + anyhow::anyhow!("Expected Ok(AgenticLoopResult) but dispatcher returned Err: {e:?}") + })?; + match outcome { super::super::AgenticLoopResult::Response(text) => { assert_eq!(text, expected_text); } @@ -205,6 +212,7 @@ fn assert_agentic_loop_text_response( panic!("Expected text response, got NeedApproval"); } } + Ok(()) } /// Verify that the max_iterations guard terminates the loop even when the @@ -232,5 +240,6 @@ async fn test_dispatcher_terminates_with_max_iterations() { ) .await; - assert_agentic_loop_text_response(result, "forced text response"); + assert_agentic_loop_text_response(result, "forced text response") + .expect("dispatcher should return a text response"); } diff --git a/src/agent/dispatcher/tests/pipeline.rs b/src/agent/dispatcher/tests/pipeline.rs index 8259c7135..200f20417 100644 --- a/src/agent/dispatcher/tests/pipeline.rs +++ b/src/agent/dispatcher/tests/pipeline.rs @@ -1,231 +1,15 @@ //! Integration-style tests for the dispatcher tool-execution pipeline. use std::sync::{Arc, Mutex as StdMutex}; -use std::time::Instant; use crate::agent::session::PendingApproval; -use crate::channels::StatusUpdate; -use crate::context::JobContext; -use crate::llm::{ChatMessage, CompletionResponse, FinishReason, NativeLlmProvider, Role}; -use crate::testing::StubChannel; -use crate::tools::{ApprovalRequirement, NativeTool, ToolError, ToolOutput}; +use crate::tools::ApprovalRequirement; use super::*; -struct TestPipelineTool { - name: &'static str, - description: &'static str, - output_text: &'static str, - approval_requirement: ApprovalRequirement, -} - -impl NativeTool for TestPipelineTool { - fn name(&self) -> &str { - self.name - } - - fn description(&self) -> &str { - self.description - } - - fn parameters_schema(&self) -> serde_json::Value { - serde_json::json!({ - "type": "object", - "properties": { - "message": { "type": "string" } - } - }) - } +mod support; - async fn execute( - &self, - _params: serde_json::Value, - _ctx: &JobContext, - ) -> Result { - Ok(ToolOutput::text(self.output_text, Instant::now().elapsed())) - } - - fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { - self.approval_requirement - } - - fn requires_sanitization(&self) -> bool { - false - } -} - -struct PipelineProvider { - name: &'static str, - tool_calls: Vec, - final_text: &'static str, - observed_tool_message_counts: Arc>>, -} - -impl NativeLlmProvider for PipelineProvider { - fn model_name(&self) -> &str { - self.name - } - - fn cost_per_token(&self) -> (rust_decimal::Decimal, rust_decimal::Decimal) { - (rust_decimal::Decimal::ZERO, rust_decimal::Decimal::ZERO) - } - - async fn complete( - &self, - _request: crate::llm::CompletionRequest, - ) -> Result { - Ok(CompletionResponse { - content: self.final_text.to_string(), - input_tokens: 0, - output_tokens: 0, - finish_reason: FinishReason::Stop, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, - }) - } - - async fn complete_with_tools( - &self, - request: crate::llm::ToolCompletionRequest, - ) -> Result { - let tool_message_count = request - .messages - .iter() - .filter(|message| message.role == Role::Tool) - .count(); - self.observed_tool_message_counts - .lock() - .expect("tool message count lock poisoned") - .push(tool_message_count); - - if tool_message_count >= self.tool_calls.len().max(1) { - Ok(crate::llm::ToolCompletionResponse { - content: Some(self.final_text.to_string()), - tool_calls: Vec::new(), - input_tokens: 0, - output_tokens: 8, - finish_reason: FinishReason::Stop, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, - }) - } else { - Ok(crate::llm::ToolCompletionResponse { - content: None, - tool_calls: self.tool_calls.clone(), - input_tokens: 0, - output_tokens: 8, - finish_reason: FinishReason::ToolUse, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, - }) - } - } -} - -async fn make_stubbed_channels( - name: &str, -) -> ( - Arc, - Arc>>, -) { - let (stub, _sender) = StubChannel::new(name); - let statuses = stub.captured_statuses_handle(); - let channels = Arc::new(ChannelManager::new()); - channels.add(Box::new(stub)).await; - (channels, statuses) -} - -async fn make_pipeline_agent( - provider: Arc, - tools: Vec>, - max_tool_iterations: usize, - auto_approve_tools: bool, -) -> anyhow::Result<(Agent, Arc>>)> { - let (channels, statuses) = make_stubbed_channels("test-chan").await; - let deps = make_agent_deps(provider, false); - deps.tools.register_builtin_tools()?; - for tool in tools { - let _ = deps.tools.register(tool).await; - } - - let agent = Agent::new( - make_agent_config(max_tool_iterations, auto_approve_tools), - deps, - channels, - None, - None, - None, - Some(Arc::new(ContextManager::new(1))), - None, - ); - - Ok((agent, statuses)) -} - -async fn build_run_loop_ctx( - prompt: &str, -) -> ( - Arc>, - uuid::Uuid, - IncomingMessage, - super::super::RunLoopCtx, -) { - let session = Arc::new(Mutex::new(Session::new("test-user"))); - let thread_id = { - let mut sess = session.lock().await; - let thread = sess.create_thread(); - thread.start_turn(prompt); - thread.id - }; - let message = IncomingMessage::new("test-chan", "test-user", prompt); - let ctx = super::super::RunLoopCtx { - session: Arc::clone(&session), - thread_id, - initial_messages: vec![ChatMessage::user(prompt)], - }; - - (session, thread_id, message, ctx) -} - -fn assert_thinking_status(statuses: &[StatusUpdate], expected: &str) { - assert!( - statuses - .iter() - .any(|status| matches!(status, StatusUpdate::Thinking(message) if message == expected)), - "expected Thinking status `{expected}`, got: {statuses:?}" - ); -} - -fn assert_tool_result_status(statuses: &[StatusUpdate], tool_name: &str) { - assert!( - statuses.iter().any(|status| matches!( - status, - StatusUpdate::ToolResult { name, preview } - if name == tool_name && !preview.is_empty() - )), - "expected non-empty ToolResult preview for `{tool_name}`, got: {statuses:?}" - ); -} - -fn assert_tool_completed_status(statuses: &[StatusUpdate], tool_name: &str) { - assert!( - statuses.iter().any(|status| matches!( - status, - StatusUpdate::ToolCompleted { name, success, .. } - if name == tool_name && *success - )), - "expected successful ToolCompleted for `{tool_name}`, got: {statuses:?}" - ); -} - -fn assert_tool_started_status(statuses: &[StatusUpdate], tool_name: &str) { - assert!( - statuses.iter().any( - |status| matches!(status, StatusUpdate::ToolStarted { name } if name == tool_name) - ), - "expected ToolStarted for `{tool_name}`, got: {statuses:?}" - ); -} +use support::*; #[tokio::test] async fn pipeline_runs_inline_for_single_tool() { diff --git a/src/agent/dispatcher/tests/pipeline/support.rs b/src/agent/dispatcher/tests/pipeline/support.rs new file mode 100644 index 000000000..ae585fd69 --- /dev/null +++ b/src/agent/dispatcher/tests/pipeline/support.rs @@ -0,0 +1,237 @@ +//! Test doubles and arrangement helpers for the tool-execution pipeline +//! tests. +//! +//! Split from the parent module so each stays within the repository's +//! module-size limit. + +use std::sync::{Arc, Mutex as StdMutex}; +use std::time::Instant; + +use crate::channels::StatusUpdate; +use crate::context::JobContext; +use crate::llm::{ChatMessage, CompletionResponse, FinishReason, NativeLlmProvider, Role}; +use crate::testing::StubChannel; +use crate::tools::{ApprovalRequirement, NativeTool, ToolError, ToolOutput}; + +use super::*; + +pub(super) struct TestPipelineTool { + pub(super) name: &'static str, + pub(super) description: &'static str, + pub(super) output_text: &'static str, + pub(super) approval_requirement: ApprovalRequirement, +} + +impl NativeTool for TestPipelineTool { + fn name(&self) -> &str { + self.name + } + + fn description(&self) -> &str { + self.description + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "message": { "type": "string" } + } + }) + } + + async fn execute( + &self, + _params: serde_json::Value, + _ctx: &JobContext, + ) -> Result { + Ok(ToolOutput::text(self.output_text, Instant::now().elapsed())) + } + + fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { + self.approval_requirement + } + + fn requires_sanitization(&self) -> bool { + false + } +} + +pub(super) struct PipelineProvider { + pub(super) name: &'static str, + pub(super) tool_calls: Vec, + pub(super) final_text: &'static str, + pub(super) observed_tool_message_counts: Arc>>, +} + +impl NativeLlmProvider for PipelineProvider { + fn model_name(&self) -> &str { + self.name + } + + fn cost_per_token(&self) -> (rust_decimal::Decimal, rust_decimal::Decimal) { + (rust_decimal::Decimal::ZERO, rust_decimal::Decimal::ZERO) + } + + async fn complete( + &self, + _request: crate::llm::CompletionRequest, + ) -> Result { + Ok(CompletionResponse { + content: self.final_text.to_string(), + input_tokens: 0, + output_tokens: 0, + finish_reason: FinishReason::Stop, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + }) + } + + async fn complete_with_tools( + &self, + request: crate::llm::ToolCompletionRequest, + ) -> Result { + let tool_message_count = request + .messages + .iter() + .filter(|message| message.role == Role::Tool) + .count(); + // The trait signature already returns `Result`, so a poisoned + // observation lock is reported as a provider failure rather than + // panicking inside the stub. + self.observed_tool_message_counts + .lock() + .map_err(|_| crate::error::LlmError::RequestFailed { + provider: self.name.to_string(), + reason: "tool message count lock poisoned".to_string(), + })? + .push(tool_message_count); + + if tool_message_count >= self.tool_calls.len().max(1) { + Ok(crate::llm::ToolCompletionResponse { + content: Some(self.final_text.to_string()), + tool_calls: Vec::new(), + input_tokens: 0, + output_tokens: 8, + finish_reason: FinishReason::Stop, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + }) + } else { + Ok(crate::llm::ToolCompletionResponse { + content: None, + tool_calls: self.tool_calls.clone(), + input_tokens: 0, + output_tokens: 8, + finish_reason: FinishReason::ToolUse, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + }) + } + } +} + +pub(super) async fn make_stubbed_channels( + name: &str, +) -> ( + Arc, + Arc>>, +) { + let (stub, _sender) = StubChannel::new(name); + let statuses = stub.captured_statuses_handle(); + let channels = Arc::new(ChannelManager::new()); + channels.add(Box::new(stub)).await; + (channels, statuses) +} + +pub(super) async fn make_pipeline_agent( + provider: Arc, + tools: Vec>, + max_tool_iterations: usize, + auto_approve_tools: bool, +) -> anyhow::Result<(Agent, Arc>>)> { + let (channels, statuses) = make_stubbed_channels("test-chan").await; + let deps = make_agent_deps(provider, false); + deps.tools.register_builtin_tools()?; + for tool in tools { + let _ = deps.tools.register(tool).await; + } + + let agent = Agent::new( + make_agent_config(max_tool_iterations, auto_approve_tools), + deps, + channels, + None, + None, + None, + Some(Arc::new(ContextManager::new(1))), + None, + ); + + Ok((agent, statuses)) +} + +pub(super) async fn build_run_loop_ctx( + prompt: &str, +) -> ( + Arc>, + uuid::Uuid, + IncomingMessage, + crate::agent::dispatcher::core::RunLoopCtx, +) { + let session = Arc::new(Mutex::new(Session::new("test-user"))); + let thread_id = { + let mut sess = session.lock().await; + let thread = sess.create_thread(); + thread.start_turn(prompt); + thread.id + }; + let message = IncomingMessage::new("test-chan", "test-user", prompt); + let ctx = crate::agent::dispatcher::core::RunLoopCtx { + session: Arc::clone(&session), + thread_id, + initial_messages: vec![ChatMessage::user(prompt)], + }; + + (session, thread_id, message, ctx) +} + +pub(super) fn assert_thinking_status(statuses: &[StatusUpdate], expected: &str) { + assert!( + statuses + .iter() + .any(|status| matches!(status, StatusUpdate::Thinking(message) if message == expected)), + "expected Thinking status `{expected}`, got: {statuses:?}" + ); +} + +pub(super) fn assert_tool_result_status(statuses: &[StatusUpdate], tool_name: &str) { + assert!( + statuses.iter().any(|status| matches!( + status, + StatusUpdate::ToolResult { name, preview } + if name == tool_name && !preview.is_empty() + )), + "expected non-empty ToolResult preview for `{tool_name}`, got: {statuses:?}" + ); +} + +pub(super) fn assert_tool_completed_status(statuses: &[StatusUpdate], tool_name: &str) { + assert!( + statuses.iter().any(|status| matches!( + status, + StatusUpdate::ToolCompleted { name, success, .. } + if name == tool_name && *success + )), + "expected successful ToolCompleted for `{tool_name}`, got: {statuses:?}" + ); +} + +pub(super) fn assert_tool_started_status(statuses: &[StatusUpdate], tool_name: &str) { + assert!( + statuses.iter().any( + |status| matches!(status, StatusUpdate::ToolStarted { name } if name == tool_name) + ), + "expected ToolStarted for `{tool_name}`, got: {statuses:?}" + ); +} diff --git a/src/agent/dispatcher/tests/skill_bundle_context_bdd.rs b/src/agent/dispatcher/tests/skill_bundle_context_bdd.rs index 9e69a5d4f..20ceef8b7 100644 --- a/src/agent/dispatcher/tests/skill_bundle_context_bdd.rs +++ b/src/agent/dispatcher/tests/skill_bundle_context_bdd.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; +use anyhow::Context as _; use rstest::fixture; use rstest_bdd_macros::{given, scenario, then, when}; use tempfile::TempDir; @@ -29,11 +30,15 @@ const PROMPT_MARKER: &str = "PROMPT-MARKER"; const REFERENCES_MARKER: &str = "REFERENCES-MARKER"; const ASSETS_MARKER: &str = "ASSETS-MARKER"; +/// Build the bundled skill used by these scenarios. +/// +/// Returns an error if the constructed location or manifest is rejected by +/// [`LoadedSkill::new`]. fn make_loaded_bundle_skill( skill: &str, filesystem_root: PathBuf, prompt_content: &str, -) -> LoadedSkill { +) -> anyhow::Result { LoadedSkill::new(LoadedSkillParts { manifest: SkillManifest { name: skill.to_string(), @@ -55,43 +60,49 @@ fn make_loaded_bundle_skill( PathBuf::from("SKILL.md"), SkillPackageKind::Bundle, ) - .expect("test entrypoint is bundle-relative"), + .map_err(|e| anyhow::anyhow!("test entrypoint is bundle-relative: {e}"))?, content_hash: format!("sha256:{skill}"), compiled_patterns: Vec::new(), lowercased_keywords: vec!["deploy".to_string(), "docs".to_string()], lowercased_exclude_keywords: Vec::new(), lowercased_tags: Vec::new(), }) - .expect("BDD skill location should match manifest") + .map_err(|e| anyhow::anyhow!("BDD skill location should match manifest: {e}")) } #[given("an installed bundled skill with supporting files")] -fn installed_bundled_skill(skill_context_world: &mut SkillContextWorld) { +fn installed_bundled_skill( + skill_context_world: &mut SkillContextWorld, +) -> Result<(), anyhow::Error> { let filesystem_root = PathBuf::from("/tmp/axinite-test-installed/deploy-docs"); skill_context_world.filesystem_root = Some(filesystem_root.clone()); skill_context_world.active_skill = Some(make_loaded_bundle_skill( "deploy-docs", filesystem_root, "Use references/usage.md for deployment details.", - )); + )?); + Ok(()) } #[given("an installed bundled skill with a references file and an assets file")] -fn installed_bundled_skill_with_ancillary_files(skill_context_world: &mut SkillContextWorld) { - let installed_dir = tempfile::tempdir().expect("installed bundle tempdir should be created"); +fn installed_bundled_skill_with_ancillary_files( + skill_context_world: &mut SkillContextWorld, +) -> Result<(), anyhow::Error> { + let installed_dir = + tempfile::tempdir().context("installed bundle tempdir should be created")?; ambient_fs::create_dir_all(installed_dir.path().join("references")) - .expect("references directory should be created"); + .context("references directory should be created")?; ambient_fs::create_dir_all(installed_dir.path().join("assets")) - .expect("assets directory should be created"); + .context("assets directory should be created")?; ambient_fs::write(installed_dir.path().join("SKILL.md"), PROMPT_MARKER) - .expect("SKILL.md should be written"); + .context("SKILL.md should be written")?; ambient_fs::write( installed_dir.path().join("references/usage.md"), REFERENCES_MARKER, ) - .expect("reference file should be written"); + .context("reference file should be written")?; ambient_fs::write(installed_dir.path().join("assets/note.txt"), ASSETS_MARKER) - .expect("asset file should be written"); + .context("asset file should be written")?; let filesystem_root = installed_dir.path().to_path_buf(); skill_context_world.filesystem_root = Some(filesystem_root.clone()); @@ -99,21 +110,25 @@ fn installed_bundled_skill_with_ancillary_files(skill_context_world: &mut SkillC "deploy-docs", filesystem_root, PROMPT_MARKER, - )); + )?); skill_context_world._installed_dir = Some(installed_dir); + Ok(()) } #[when("the skill is selected for an agent turn")] -fn selected_for_agent_turn(skill_context_world: &mut SkillContextWorld) { +fn selected_for_agent_turn( + skill_context_world: &mut SkillContextWorld, +) -> Result<(), anyhow::Error> { let agent = make_test_agent(); let skill = skill_context_world .active_skill .clone() - .expect("Given step should install an active skill"); + .context("Given step should install an active skill")?; let rendered = agent .build_skill_context_block(&[skill]) - .expect("installed bundle skill should produce context"); + .context("installed bundle skill should produce context")?; skill_context_world.rendered_context = Some(rendered); + Ok(()) } #[then("the active skill context names the skill identifier")] @@ -137,19 +152,22 @@ fn context_names_entrypoint(skill_context_world: &SkillContextWorld) { } #[then("the active skill context does not expose the filesystem root")] -fn context_hides_filesystem_root(skill_context_world: &SkillContextWorld) { - let rendered = match skill_context_world.rendered_context.as_deref() { - Some(rendered) => rendered, - None => panic!("When step should render active skill context"), - }; +fn context_hides_filesystem_root( + skill_context_world: &SkillContextWorld, +) -> Result<(), anyhow::Error> { + let rendered = skill_context_world + .rendered_context + .as_deref() + .context("When step should render active skill context")?; let filesystem_root = skill_context_world .filesystem_root .as_ref() - .expect("Given step should record the runtime root"); + .context("Given step should record the runtime root")?; assert!( !rendered.contains(&filesystem_root.to_string_lossy().to_string()), "active skill context must not expose the private runtime root" ); + Ok(()) } #[then("only SKILL.md content is injected into the active skill context")] @@ -179,11 +197,19 @@ fn assets_content_is_absent(skill_context_world: &SkillContextWorld) { assert!(!rendered.contains(ASSETS_MARKER)); } -fn assert_rendered_snapshot(skill_context_world: SkillContextWorld, snapshot_name: &str) { +/// Snapshot the rendered context recorded by the When step. +/// +/// Returns an error if no context was rendered, so the calling scenario body +/// decides how the failure surfaces. +fn assert_rendered_snapshot( + skill_context_world: SkillContextWorld, + snapshot_name: &str, +) -> Result<(), anyhow::Error> { let rendered = skill_context_world .rendered_context - .expect("When step should render active skill context"); + .context("When step should render active skill context")?; insta::assert_snapshot!(snapshot_name, rendered); + Ok(()) } #[scenario( @@ -193,7 +219,8 @@ fn assert_rendered_snapshot(skill_context_world: SkillContextWorld, snapshot_nam fn selected_bundle_skill_exposes_stable_bundle_relative_metadata( skill_context_world: SkillContextWorld, ) { - assert_rendered_snapshot(skill_context_world, "selected_bundle_skill_context_block"); + assert_rendered_snapshot(skill_context_world, "selected_bundle_skill_context_block") + .expect("When step should render active skill context"); } #[scenario( @@ -203,5 +230,6 @@ fn selected_bundle_skill_exposes_stable_bundle_relative_metadata( fn activated_bundle_skill_does_not_eagerly_load_ancillary_files( skill_context_world: SkillContextWorld, ) { - assert_rendered_snapshot(skill_context_world, "activated_bundle_skill_context_block"); + assert_rendered_snapshot(skill_context_world, "activated_bundle_skill_context_block") + .expect("When step should render active skill context"); } diff --git a/src/agent/dispatcher/tests/skills.rs b/src/agent/dispatcher/tests/skills.rs index d9f43683c..67eb79a33 100644 --- a/src/agent/dispatcher/tests/skills.rs +++ b/src/agent/dispatcher/tests/skills.rs @@ -14,12 +14,15 @@ use crate::skills::{ /// Build a [`LoadedSkill`] with the given name, version, description, and /// keyword list, using sensible defaults for the remaining fields. +/// +/// Returns an error if the constructed location or manifest is rejected by +/// [`LoadedSkill::new`]. fn make_test_skill( name: &str, version: &str, description: &str, keywords: Vec, -) -> LoadedSkill { +) -> anyhow::Result { let lowercased_keywords: Vec = keywords.iter().map(|k| k.to_ascii_lowercase()).collect(); LoadedSkill::new(LoadedSkillParts { @@ -45,14 +48,14 @@ fn make_test_skill( PathBuf::from("SKILL.md"), SkillPackageKind::SingleFile, ) - .expect("test entrypoint is bundle-relative"), + .map_err(|e| anyhow::anyhow!("test entrypoint is bundle-relative: {e}"))?, content_hash: format!("{name}-hash"), compiled_patterns: vec![], lowercased_keywords, lowercased_exclude_keywords: vec![], lowercased_tags: vec![], }) - .expect("test skill location should match manifest") + .map_err(|e| anyhow::anyhow!("test skill location should match manifest: {e}")) } /// Insert a skill into `registry` under the given name. @@ -69,11 +72,13 @@ fn install_skill( Ok(()) } -fn make_context_skill(trust: SkillTrust) -> LoadedSkill { - let mut skill = make_test_skill("my-skill", "1.2.3", "Does stuff", vec!["test".to_string()]); +/// Build the shared context-block skill, returning an error if the underlying +/// skill cannot be constructed. +fn make_context_skill(trust: SkillTrust) -> anyhow::Result { + let mut skill = make_test_skill("my-skill", "1.2.3", "Does stuff", vec!["test".to_string()])?; skill.trust = trust; skill.prompt_content = "Use bold & 'quotes' here".to_string(); - skill + Ok(skill) } #[test] @@ -84,7 +89,8 @@ fn test_select_active_skills_returns_empty_when_disabled() { "1.0.0", "Test skill for disabled check", vec!["test".to_string()], - ); + ) + .expect("test skill should be constructible"); install_skill(®istry, "test-skill", skill).expect("install_skill should succeed"); let skills_cfg = SkillsConfig { @@ -107,7 +113,8 @@ fn test_select_active_skills_returns_empty_when_registry_lock_is_poisoned() { "1.0.0", "Skill to ensure non-empty registry before poisoning", vec!["hello".to_string()], - ); + ) + .expect("test skill should be constructible"); install_skill(®istry, "poison-skill", skill).expect("install_skill should succeed"); let poison_registry = Arc::clone(®istry); @@ -141,7 +148,8 @@ fn test_select_active_skills_selects_matching_skill() { "2.1.0", "Provides weather-related assistance", vec!["weather".to_string(), "forecast".to_string()], - ); + ) + .expect("test skill should be constructible"); install_skill(®istry, "weather-helper", skill).expect("install_skill should succeed"); let skills_cfg = SkillsConfig { @@ -166,7 +174,7 @@ fn test_select_active_skills_selects_matching_skill() { #[test] fn test_build_skill_context_block_trusted() { let agent = make_test_agent(); - let skill = make_context_skill(SkillTrust::Trusted); + let skill = make_context_skill(SkillTrust::Trusted).expect("context skill should be built"); let result = agent.build_skill_context_block(&[skill]); assert_snapshot!(result.expect("trusted skill should produce context")); @@ -175,7 +183,7 @@ fn test_build_skill_context_block_trusted() { #[test] fn test_build_skill_context_block_installed() { let agent = make_test_agent(); - let skill = make_context_skill(SkillTrust::Installed); + let skill = make_context_skill(SkillTrust::Installed).expect("context skill should be built"); let result = agent .build_skill_context_block(&[skill]) .expect("installed skill should produce context"); @@ -190,7 +198,8 @@ fn test_build_skill_context_block_installed() { #[test] fn test_build_skill_context_block_includes_bundle_relative_metadata() { let agent = make_test_agent(); - let mut skill = make_context_skill(SkillTrust::Installed); + let mut skill = + make_context_skill(SkillTrust::Installed).expect("context skill should be built"); skill .set_location( LoadedSkillLocation::new( @@ -240,8 +249,9 @@ fn test_build_skill_context_block_includes_bundle_relative_metadata() { #[test] fn test_build_skill_context_block_both_variants() { let agent = make_test_agent(); - let trusted = make_context_skill(SkillTrust::Trusted); - let installed = make_context_skill(SkillTrust::Installed); + let trusted = make_context_skill(SkillTrust::Trusted).expect("context skill should be built"); + let installed = + make_context_skill(SkillTrust::Installed).expect("context skill should be built"); let result = agent.build_skill_context_block(&[trusted, installed]); assert_snapshot!(result.expect("both skills should produce combined context")); diff --git a/src/agent/scheduler/tests/approval.rs b/src/agent/scheduler/tests/approval.rs index 48eb3b9ec..37ae0b024 100644 --- a/src/agent/scheduler/tests/approval.rs +++ b/src/agent/scheduler/tests/approval.rs @@ -107,24 +107,33 @@ async fn setup_tools_and_job() -> Result { }) } +/// Assert that `result` was rejected with an auth-required error for +/// `tool_name`. Shares a signature with [`assert_executed`] so both can be +/// selected per `#[case]`. fn assert_auth_required( result: Result, tool_name: &'static str, msg: &'static str, -) { +) -> Result<()> { match result.expect_err(msg) { Error::Tool(AppToolError::AuthRequired { name }) => assert_eq!(name, tool_name), other => panic!("{msg}: unexpected error {other}"), } + Ok(()) } +/// Assert that `result` executed and produced `expected_text`. +/// +/// Returns an error when execution failed, leaving the calling test to +/// propagate it. fn assert_executed( result: Result, expected_text: &'static str, msg: &'static str, -) { - let output = result.expect(msg); +) -> Result<()> { + let output = result.map_err(|e| anyhow!("{msg}: {e}"))?; assert_eq!(output.result.as_str(), Some(expected_text), "{msg}"); + Ok(()) } #[tokio::test] @@ -134,12 +143,12 @@ async fn test_execute_tool_task_blocks_without_context() -> Result<()> { f.run(None, "soft_gate").await, "soft_gate", "soft_gate should be blocked without context", - ); + )?; assert_auth_required( f.run(None, "hard_gate").await, "hard_gate", "hard_gate should be blocked without context", - ); + )?; Ok(()) } @@ -161,7 +170,11 @@ async fn test_execute_tool_task_blocks_without_context() -> Result<()> { #[tokio::test] async fn test_execute_tool_task_with_approval_context( #[case] ctx: ApprovalContext, - #[case] hard_gate_assert: fn(Result, &'static str, &'static str), + #[case] hard_gate_assert: fn( + Result, + &'static str, + &'static str, + ) -> Result<()>, #[case] hard_gate_expected: &'static str, #[case] soft_gate_msg: &'static str, #[case] hard_gate_msg: &'static str, @@ -171,11 +184,11 @@ async fn test_execute_tool_task_with_approval_context( f.run(Some(ctx.clone()), "soft_gate").await, "soft_ok", soft_gate_msg, - ); + )?; hard_gate_assert( f.run(Some(ctx), "hard_gate").await, hard_gate_expected, hard_gate_msg, - ); + )?; Ok(()) } diff --git a/src/agent/thread_ops/document_store/tests/mod.rs b/src/agent/thread_ops/document_store/tests/mod.rs index fa1c543c7..c92d37ea2 100644 --- a/src/agent/thread_ops/document_store/tests/mod.rs +++ b/src/agent/thread_ops/document_store/tests/mod.rs @@ -33,20 +33,27 @@ fn make_incoming_attachment( } } -async fn make_workspace() -> (tempfile::TempDir, std::sync::Arc) { +/// Create a temporary directory and a workspace backed by a migrated local +/// database. +/// +/// Returns an error if the temporary directory, the backend, or the migrations +/// cannot be prepared. +async fn make_workspace() -> anyhow::Result<(tempfile::TempDir, std::sync::Arc)> { + use anyhow::Context as _; + use crate::db::Database; use std::sync::Arc; - let tmp_dir = tempfile::tempdir().expect("create tempdir"); + let tmp_dir = tempfile::tempdir().context("create tempdir")?; let db_path = tmp_dir.path().join("doc_store_test.db"); let backend = crate::db::libsql::LibSqlBackend::new_local(&db_path) .await - .expect("failed to create local backend"); + .context("failed to create local backend")?; Database::run_migrations(&backend) .await - .expect("failed to run migrations"); + .context("failed to run migrations")?; let workspace = Arc::new(Workspace::new_with_db("test-user", Arc::new(backend))); - (tmp_dir, workspace) + Ok((tmp_dir, workspace)) } fn new_doc(id: &str, filename: &str, text: Option<&str>, size: u64) -> IncomingAttachment { @@ -184,7 +191,9 @@ fn build_document_path_uses_sanitized_id_and_filename() { async fn store_extracted_documents_filters_and_stores_only_usable_documents() { use uuid::Uuid; - let (_tmp_dir, workspace) = make_workspace().await; + let (_tmp_dir, workspace) = make_workspace() + .await + .expect("workspace fixture should be created"); // Build message with: usable doc1, non-document audio1, sentinel doc2, and no-text doc3 let message_id = Uuid::new_v4(); @@ -228,7 +237,9 @@ async fn store_extracted_documents_filters_and_stores_only_usable_documents() { async fn store_extracted_documents_writes_expected_header_and_body() { use uuid::Uuid; - let (_tmp_dir, workspace) = make_workspace().await; + let (_tmp_dir, workspace) = make_workspace() + .await + .expect("workspace fixture should be created"); // Build message with only usable doc1 let message_id = Uuid::new_v4(); diff --git a/src/bootstrap/tests/env_format.rs b/src/bootstrap/tests/env_format.rs index 90ca00f05..5af1d952b 100644 --- a/src/bootstrap/tests/env_format.rs +++ b/src/bootstrap/tests/env_format.rs @@ -4,17 +4,24 @@ use tempfile::tempdir; use super::super::*; -fn assert_env_roundtrip(key: &str, value: &str) { - let dir = tempdir().expect("create temp dir for env round-trip test"); +/// Round-trip `key`/`value` through a temporary `.env` file. +/// +/// Arrangement (temporary directory, write, parse) is fallible and propagates +/// with `?`; the generated tests return the same [`anyhow::Result`] so a +/// failure there is the test verdict. +fn assert_env_roundtrip(key: &str, value: &str) -> anyhow::Result<()> { + use anyhow::Context as _; + + let dir = tempdir().context("create temp dir for env round-trip test")?; let env_path = dir.path().join(".env"); let write_error = format!("write round-trip env at {}", env_path.display()); let parse_error = format!("parse round-trip env at {}", env_path.display()); let vars = [(key, value)]; - upsert_bootstrap_vars_to(&env_path, &vars).expect(write_error.as_str()); + upsert_bootstrap_vars_to(&env_path, &vars).context(write_error)?; let parsed: Vec<(String, String)> = dotenvy::from_path_iter(&env_path) - .expect(parse_error.as_str()) + .context(parse_error)? .filter_map(|result| result.ok()) .collect(); @@ -23,20 +30,23 @@ fn assert_env_roundtrip(key: &str, value: &str) { 1, "{key} round-trip should produce one env var" ); - let found = parsed.iter().find(|(parsed_key, _)| parsed_key == key); - assert!(found.is_some(), "{key} must be present"); + let (_, found_value) = parsed + .iter() + .find(|(parsed_key, _)| parsed_key == key) + .with_context(|| format!("{key} must be present"))?; assert_eq!( - found.expect("round-trip env entry present").1, + found_value.as_str(), value, "{key} must survive .env round-trip" ); + Ok(()) } macro_rules! env_roundtrip_test { ($name:ident, $key:expr, $value:expr) => { #[test] - fn $name() { - assert_env_roundtrip($key, $value); + fn $name() -> anyhow::Result<()> { + assert_env_roundtrip($key, $value) } }; } diff --git a/src/bootstrap/tests/migration.rs b/src/bootstrap/tests/migration.rs index c38a6b6ad..6b3d9c460 100644 --- a/src/bootstrap/tests/migration.rs +++ b/src/bootstrap/tests/migration.rs @@ -11,14 +11,22 @@ fn would_autodetect_libsql(db_path: &std::path::Path) -> bool { std::env::var("DATABASE_BACKEND").is_err() && db_path.exists() } -fn assert_bootstrap_env_written(env_path: &std::path::Path, expected_url: &str) { +/// Assert the migrated `.env` holds `expected_url`. +/// +/// Reading the file is arrangement, so it propagates as an +/// [`std::io::Error`]; the caller unwraps in the test body. +fn assert_bootstrap_env_written( + env_path: &std::path::Path, + expected_url: &str, +) -> std::io::Result<()> { assert!(env_path.exists(), ".env must exist after migration"); - let content = ambient_fs::read_to_string(env_path).expect("read migrated .env"); + let content = ambient_fs::read_to_string(env_path)?; assert_eq!( content, format!("DATABASE_URL=\"{expected_url}\"\n"), ".env content must contain the migrated DATABASE_URL" ); + Ok(()) } fn assert_bootstrap_file_renamed(dir_path: &std::path::Path) { @@ -55,7 +63,8 @@ fn test_migrate_bootstrap_json_to_env() { migrate_bootstrap_json_to_env(&env_path); - assert_bootstrap_env_written(&env_path, "postgres://localhost/axinite_upgrade"); + assert_bootstrap_env_written(&env_path, "postgres://localhost/axinite_upgrade") + .expect("read migrated .env"); assert_bootstrap_file_renamed(dir.path()); } @@ -69,7 +78,8 @@ fn load_axinite_env_migrates_bootstrap_json_to_env() { load_axinite_env(); - assert_bootstrap_env_written(&env_path, "postgres://localhost/axinite_public_boundary"); + assert_bootstrap_env_written(&env_path, "postgres://localhost/axinite_public_boundary") + .expect("read migrated .env"); assert_bootstrap_file_renamed(&base_dir); return; } diff --git a/src/bootstrap/tests/migration_disk_to_db.rs b/src/bootstrap/tests/migration_disk_to_db.rs index dd54aef7e..ec4092463 100644 --- a/src/bootstrap/tests/migration_disk_to_db.rs +++ b/src/bootstrap/tests/migration_disk_to_db.rs @@ -18,37 +18,41 @@ async fn migrate_disk_to_db_from_dir_missing_legacy_file_is_noop() { .await .expect("missing settings migration should succeed"); - assert_store_state(&store, 0, 0); + assert_store_state(&store.state().expect("lock migration store state"), 0, 0); assert!(!dir.path().join("settings.json.migrated").exists()); } #[tokio::test] async fn migrate_disk_to_db_from_dir_renames_when_db_already_has_settings() { let dir = tempdir().expect("create temp dir for stale settings migration"); - let _settings_path = write_legacy_settings(&dir); + let _settings_path = write_legacy_settings(&dir).expect("write legacy settings.json"); let store = MigrationStore::new(Ok(true)); super::super::migration::migrate_disk_to_db_from_dir(&store, "test-user", dir.path()) .await .expect("stale settings migration should succeed"); - assert_store_state(&store, 1, 0); + assert_store_state(&store.state().expect("lock migration store state"), 1, 0); assert_legacy_file_renamed(&dir); } #[tokio::test] async fn migrate_disk_to_db_from_dir_writes_settings_and_renames_legacy_file() { let dir = tempdir().expect("create temp dir for settings migration"); - let _settings_path = write_legacy_settings(&dir); + let _settings_path = write_legacy_settings(&dir).expect("write legacy settings.json"); let store = MigrationStore::new(Ok(false)); super::super::migration::migrate_disk_to_db_from_dir(&store, "test-user", dir.path()) .await .expect("settings migration should succeed"); - assert_store_state(&store, 1, 1); + assert_store_state(&store.state().expect("lock migration store state"), 1, 1); assert_eq!( - store.state().captured_settings.get("onboard_completed"), + store + .state() + .expect("lock migration store state") + .captured_settings + .get("onboard_completed"), Some(&serde_json::Value::Bool(true)) ); assert_legacy_file_renamed(&dir); @@ -57,7 +61,7 @@ async fn migrate_disk_to_db_from_dir_writes_settings_and_renames_legacy_file() { #[tokio::test] async fn migrate_disk_to_db_from_dir_db_failure_leaves_legacy_file_unmigrated() { let dir = tempdir().expect("create temp dir for failed settings migration"); - let _settings_path = write_legacy_settings(&dir); + let _settings_path = write_legacy_settings(&dir).expect("write legacy settings.json"); let store = MigrationStore::with_set_all_error(); let error = @@ -68,14 +72,14 @@ async fn migrate_disk_to_db_from_dir_db_failure_leaves_legacy_file_unmigrated() assert!( matches!(error, MigrationError::Database(ref message) if message.contains("Failed to write settings to DB")) ); - assert_store_state(&store, 1, 1); + assert_store_state(&store.state().expect("lock migration store state"), 1, 1); assert_legacy_file_not_renamed(&dir); } #[tokio::test] async fn migrate_disk_to_db_from_dir_is_ok_after_best_effort_rename_removed_source() { let dir = tempdir().expect("create temp dir for repeated settings migration"); - let _settings_path = write_legacy_settings(&dir); + let _settings_path = write_legacy_settings(&dir).expect("write legacy settings.json"); let store = MigrationStore::new(Ok(false)); super::super::migration::migrate_disk_to_db_from_dir(&store, "test-user", dir.path()) @@ -85,6 +89,6 @@ async fn migrate_disk_to_db_from_dir_is_ok_after_best_effort_rename_removed_sour .await .expect("second settings migration should succeed after source was renamed"); - assert_store_state(&store, 1, 1); + assert_store_state(&store.state().expect("lock migration store state"), 1, 1); assert_legacy_file_renamed(&dir); } diff --git a/src/bootstrap/tests/migration_rename.rs b/src/bootstrap/tests/migration_rename.rs index 752b8985b..57919c5dc 100644 --- a/src/bootstrap/tests/migration_rename.rs +++ b/src/bootstrap/tests/migration_rename.rs @@ -19,8 +19,8 @@ fn rename_to_migrated_cases( #[case] setup: RenameSetup, #[case] expected_error_kind: Option, ) { - let mut fixture = rename_fixture(); - fixture.prepare(setup); + let mut fixture = rename_fixture().expect("create temp dir for rename test"); + fixture.prepare(setup).expect("prepare rename fixture"); let result = super::super::migration::rename_to_migrated(&fixture.path); @@ -41,9 +41,11 @@ fn rename_to_migrated_cases( #[traced_test] #[rstest] fn rename_legacy_bootstrap_success() { - let mut fixture = rename_fixture(); + let mut fixture = rename_fixture().expect("create temp dir for rename test"); fixture.path = fixture.dir.path().join("bootstrap.json"); - fixture.prepare(RenameSetup::ExistingFile); + fixture + .prepare(RenameSetup::ExistingFile) + .expect("prepare rename fixture"); super::super::migration::rename_legacy_bootstrap(fixture.dir.path()); @@ -55,9 +57,11 @@ fn rename_legacy_bootstrap_success() { #[traced_test] #[rstest] fn rename_legacy_bootstrap_permission_denied() { - let mut fixture = rename_fixture(); + let mut fixture = rename_fixture().expect("create temp dir for rename test"); fixture.path = fixture.dir.path().join("bootstrap.json"); - fixture.prepare(RenameSetup::ReadOnlyDirectory); + fixture + .prepare(RenameSetup::ReadOnlyDirectory) + .expect("prepare rename fixture"); super::super::migration::rename_legacy_bootstrap(fixture.dir.path()); diff --git a/src/bootstrap/tests/migration_support.rs b/src/bootstrap/tests/migration_support.rs index e22184eaf..da6abca11 100644 --- a/src/bootstrap/tests/migration_support.rs +++ b/src/bootstrap/tests/migration_support.rs @@ -25,6 +25,10 @@ //! (containing `onboard_completed` and `database_backend` keys) into the //! provided [`TempDir`] and returns the path to the written file. //! +//! Arrangement can fail, so these fixture helpers return [`std::io::Result`] +//! and propagate errors; only the test body unwraps, because a failure there +//! is the test verdict. +//! //! ## In-memory `SettingsStore` mock //! //! [`MigrationStore`] implements [`SettingsStore`] entirely in memory, @@ -41,13 +45,16 @@ //! ## Assertion helpers //! //! [`assert_store_state`] asserts expected call counts for `has_settings` and -//! `set_all_settings` and that `set_setting` was never invoked. +//! `set_all_settings` and that `set_setting` was never invoked. It takes a +//! borrowed [`MigrationStoreState`], so the caller unwraps +//! [`MigrationStore::state`] in the test body. //! //! [`assert_legacy_file_renamed`] and [`assert_legacy_file_not_renamed`] //! assert the post-migration filesystem state: whether `settings.json` has //! been replaced by `settings.json.migrated`. use std::collections::HashMap; +use std::io; use std::sync::Mutex; use tempfile::{TempDir, tempdir}; @@ -103,8 +110,16 @@ impl MigrationStore { } } - pub(super) fn state(&self) -> std::sync::MutexGuard<'_, MigrationStoreState> { - self.state.lock().expect("migration store state lock") + /// Borrow the recorded call state. + /// + /// Locking is arrangement, not a verdict, so a poisoned mutex is reported + /// as a [`DatabaseError`] rather than panicking; test bodies unwrap. + pub(super) fn state( + &self, + ) -> Result, DatabaseError> { + self.state + .lock() + .map_err(|_| DatabaseError::Query("migration store state lock poisoned".to_string())) } } @@ -132,7 +147,7 @@ impl SettingsStore for MigrationStore { _value: &'a serde_json::Value, ) -> DbFuture<'a, Result<(), DatabaseError>> { Box::pin(async { - self.state().set_setting_calls += 1; + self.state()?.set_setting_calls += 1; Ok(()) }) } @@ -165,7 +180,7 @@ impl SettingsStore for MigrationStore { settings: &'a HashMap, ) -> DbFuture<'a, Result<(), DatabaseError>> { Box::pin(async move { - let mut state = self.state(); + let mut state = self.state()?; state.set_all_settings_calls += 1; state.captured_settings = settings.clone(); drop(state); @@ -177,7 +192,7 @@ impl SettingsStore for MigrationStore { fn has_settings<'a>(&'a self, _user_id: UserId) -> DbFuture<'a, Result> { Box::pin(async { - self.state().has_settings_calls += 1; + self.state()?.has_settings_calls += 1; self.has_settings_result .map_err(|message| DatabaseError::Query(message.to_string())) }) @@ -185,31 +200,28 @@ impl SettingsStore for MigrationStore { } impl RenameFixture { - pub(super) fn prepare(&mut self, setup: RenameSetup) { + /// Arrange the filesystem for `setup`, reporting arrangement failures. + pub(super) fn prepare(&mut self, setup: RenameSetup) -> io::Result<()> { match setup { - RenameSetup::ExistingFile => self.write_legacy_file(), + RenameSetup::ExistingFile => self.write_legacy_file()?, RenameSetup::MissingFile => {} #[cfg(unix)] RenameSetup::ReadOnlyDirectory => { - self.write_legacy_file(); - self.make_dir_read_only(); + self.write_legacy_file()?; + self.make_dir_read_only()?; } } + Ok(()) } - fn write_legacy_file(&self) { - ambient_fs::write(&self.path, "{}").expect("write legacy settings file"); + fn write_legacy_file(&self) -> io::Result<()> { + ambient_fs::write(&self.path, "{}") } #[cfg(unix)] - fn make_dir_read_only(&mut self) { - self.original_dir_permissions = Some( - ambient_fs::metadata(self.dir.path()) - .expect("read directory metadata") - .permissions(), - ); + fn make_dir_read_only(&mut self) -> io::Result<()> { + self.original_dir_permissions = Some(ambient_fs::metadata(self.dir.path())?.permissions()); ambient_fs::set_permissions(self.dir.path(), ambient_fs::Permissions::from_mode(0o555)) - .expect("make directory read-only"); } pub(super) fn migrated_path(&self) -> std::path::PathBuf { @@ -228,18 +240,18 @@ impl Drop for RenameFixture { } } -pub(super) fn rename_fixture() -> RenameFixture { - let dir = tempdir().expect("create temp dir for rename test"); +pub(super) fn rename_fixture() -> io::Result { + let dir = tempdir()?; let path = dir.path().join("settings.json"); - RenameFixture { + Ok(RenameFixture { dir, path, #[cfg(unix)] original_dir_permissions: None, - } + }) } -pub(super) fn write_legacy_settings(dir: &TempDir) -> std::path::PathBuf { +pub(super) fn write_legacy_settings(dir: &TempDir) -> io::Result { let settings_path = dir.path().join("settings.json"); ambient_fs::write( &settings_path, @@ -248,17 +260,15 @@ pub(super) fn write_legacy_settings(dir: &TempDir) -> std::path::PathBuf { "database_backend": "libsql" }) .to_string(), - ) - .expect("write legacy settings.json"); - settings_path + )?; + Ok(settings_path) } pub(super) fn assert_store_state( - store: &MigrationStore, + state: &MigrationStoreState, expected_has_settings: usize, expected_set_all_settings: usize, ) { - let state = store.state(); assert_eq!( state.has_settings_calls, expected_has_settings, "unexpected has_settings call count" diff --git a/src/channels/wasm/wrapper/tests/channel/typing.rs b/src/channels/wasm/wrapper/tests/channel/typing.rs index 91b58247f..55dbc43f1 100644 --- a/src/channels/wasm/wrapper/tests/channel/typing.rs +++ b/src/channels/wasm/wrapper/tests/channel/typing.rs @@ -11,12 +11,15 @@ use super::*; /// 3. Send `second_status` and assert the typing task is either /// cancelled (`expect_cancelled = true`) or still live (`false`). /// 4. Shut down cleanly. +/// +/// Channel start-up and shutdown are arrangement, so their errors are +/// propagated for the calling test body to unwrap. async fn assert_typing_task_after_status( second_status: crate::channels::StatusUpdate, expect_cancelled: bool, -) { +) -> Result<(), crate::error::ChannelError> { let channel = create_test_channel(); - let _stream = channel.start().await.expect("Channel should start"); + let _stream = channel.start().await?; let metadata = serde_json::json!({"chat_id": 123}); @@ -44,7 +47,7 @@ async fn assert_typing_task_after_status( ); } - channel.shutdown().await.expect("Shutdown should succeed"); + channel.shutdown().await } #[tokio::test] @@ -74,7 +77,8 @@ async fn test_typing_task_starts_on_thinking() { #[tokio::test] async fn test_typing_task_cancelled_on_done() { assert_typing_task_after_status(crate::channels::StatusUpdate::Status("Done".into()), true) - .await; + .await + .expect("typing-task lifecycle should complete"); } #[tokio::test] @@ -85,7 +89,8 @@ async fn test_typing_task_persists_on_tool_started() { }, false, ) - .await; + .await + .expect("typing-task lifecycle should complete"); } #[tokio::test] @@ -99,7 +104,8 @@ async fn test_typing_task_cancelled_on_approval_needed() { }, true, ) - .await; + .await + .expect("typing-task lifecycle should complete"); } #[tokio::test] @@ -108,7 +114,8 @@ async fn test_typing_task_cancelled_on_awaiting_approval_status() { crate::channels::StatusUpdate::Status("Awaiting approval".into()), true, ) - .await; + .await + .expect("typing-task lifecycle should complete"); } #[tokio::test] diff --git a/src/channels/wasm/wrapper/tests/dispatch.rs b/src/channels/wasm/wrapper/tests/dispatch.rs index 77b7e2ba3..ff861a87a 100644 --- a/src/channels/wasm/wrapper/tests/dispatch.rs +++ b/src/channels/wasm/wrapper/tests/dispatch.rs @@ -5,6 +5,9 @@ use std::sync::Arc; use super::super::dispatch::DispatchContext; use crate::channels::wasm::wrapper::WasmChannel; +/// Poison error surfaced when the recorded-writes lock has been poisoned. +type WritesLockPoisoned<'a> = std::sync::PoisonError>>; + struct RecordingSettingsStore { writes: std::sync::Mutex>, } @@ -16,11 +19,12 @@ impl RecordingSettingsStore { } } - fn writes(&self) -> Vec { - self.writes - .lock() - .expect("settings writes lock poisoned") - .clone() + /// Snapshot the recorded setting keys. + /// + /// A poisoned lock is propagated rather than panicked on, so the calling + /// test body decides the verdict. + fn writes(&self) -> Result, WritesLockPoisoned<'_>> { + Ok(self.writes.lock()?.clone()) } } @@ -58,10 +62,12 @@ impl crate::db::SettingsStore for RecordingSettingsStore { _value: &'a serde_json::Value, ) -> crate::db::DbFuture<'a, Result<(), crate::error::DatabaseError>> { Box::pin(async move { - self.writes - .lock() - .expect("settings writes lock poisoned") - .push(key.to_string()); + // The trait signature is fixed, but it is fallible, so a poisoned + // lock is reported as a database error rather than a panic. + let mut writes = self.writes.lock().map_err(|_| { + crate::error::DatabaseError::Query("settings writes lock poisoned".to_string()) + })?; + writes.push(key.to_string()); Ok(()) }) } @@ -221,179 +227,13 @@ async fn test_dispatch_emitted_messages_rate_limit_does_not_update_metadata() { Err(WasmChannelError::EmitRateLimited { name }) if name == "test-channel" )); assert!(last_broadcast_metadata.read().await.is_none()); - assert!(settings_store.writes().is_empty()); - assert!(rx.try_recv().is_err()); -} - -/// Expected field values for one [`crate::channels::IncomingAttachment`], used -/// by [`assert_attachment`] to keep call sites concise and named. -struct ExpectedAttachment<'a> { - id: &'a str, - mime_type: &'a str, - filename: Option<&'a str>, - size_bytes: Option, - source_url: Option<&'a str>, - storage_key: Option<&'a str>, - extracted_text: Option<&'a str>, -} - -fn assert_attachment( - attachment: &crate::channels::IncomingAttachment, - expected: &ExpectedAttachment<'_>, -) { - assert_eq!(attachment.id, expected.id); - assert_eq!(attachment.mime_type, expected.mime_type); - assert_eq!(attachment.filename.as_deref(), expected.filename); - assert_eq!(attachment.size_bytes, expected.size_bytes); - assert_eq!(attachment.source_url.as_deref(), expected.source_url); - assert_eq!(attachment.storage_key.as_deref(), expected.storage_key); - assert_eq!( - attachment.extracted_text.as_deref(), - expected.extracted_text - ); -} - -fn build_test_attachments() -> Vec { - use crate::channels::wasm::host::Attachment; - - vec![ - Attachment { - id: "photo123".to_string(), - mime_type: "image/jpeg".to_string(), - filename: Some("cat.jpg".to_string()), - size_bytes: Some(50_000), - source_url: Some("https://api.telegram.org/file/photo123".to_string()), - storage_key: None, - extracted_text: None, - data: Vec::new(), - duration_secs: None, - }, - Attachment { - id: "doc456".to_string(), - mime_type: "application/pdf".to_string(), - filename: Some("report.pdf".to_string()), - size_bytes: Some(120_000), - source_url: None, - storage_key: Some("store/doc456".to_string()), - extracted_text: Some("Report contents...".to_string()), - data: Vec::new(), - duration_secs: None, - }, - ] -} - -async fn dispatch_messages_for_test( - messages: Vec, -) -> ( - Result<(), crate::channels::wasm::error::WasmChannelError>, - tokio::sync::mpsc::Receiver, -) { - let (tx, rx) = tokio::sync::mpsc::channel(10); - let message_tx = Arc::new(tokio::sync::RwLock::new(Some(tx))); - - let rate_limiter = Arc::new(tokio::sync::RwLock::new( - crate::channels::wasm::host::ChannelEmitRateLimiter::new( - crate::channels::wasm::capabilities::EmitRateLimitConfig::default(), - ), - )); - - let last_broadcast_metadata = Arc::new(tokio::sync::RwLock::new(None)); - let result = WasmChannel::dispatch_emitted_messages( - "test-channel", - messages, - DispatchContext { - message_tx: message_tx.as_ref(), - rate_limiter: rate_limiter.as_ref(), - last_broadcast_metadata: last_broadcast_metadata.as_ref(), - settings_store: None, - }, - ) - .await; - - (result, rx) -} - -fn assert_preserved_attachments(msg: &crate::channels::IncomingMessage) { - assert_eq!(msg.attachments.len(), 2); - - // Verify first attachment - assert_attachment( - &msg.attachments[0], - &ExpectedAttachment { - id: "photo123", - mime_type: "image/jpeg", - filename: Some("cat.jpg"), - size_bytes: Some(50_000), - source_url: Some("https://api.telegram.org/file/photo123"), - storage_key: None, - extracted_text: None, - }, - ); - - // Verify second attachment - assert_attachment( - &msg.attachments[1], - &ExpectedAttachment { - id: "doc456", - mime_type: "application/pdf", - filename: Some("report.pdf"), - size_bytes: Some(120_000), - source_url: None, - storage_key: Some("store/doc456"), - extracted_text: Some("Report contents..."), - }, + assert!( + settings_store + .writes() + .expect("settings writes lock poisoned") + .is_empty() ); + assert!(rx.try_recv().is_err()); } -#[tokio::test] -async fn test_dispatch_emitted_messages_preserves_attachments() { - use crate::channels::wasm::host::EmittedMessage; - - let messages = vec![ - EmittedMessage::new("user1", "Check these files") - .with_attachments(build_test_attachments()), - ]; - - let (result, mut rx) = dispatch_messages_for_test(messages).await; - - assert!(result.is_ok()); - - let msg = rx.try_recv().expect("Should receive message"); - assert_eq!(msg.content, "Check these files"); - assert_preserved_attachments(&msg); -} - -#[tokio::test] -async fn test_dispatch_emitted_messages_no_attachments_backward_compat() { - use crate::channels::wasm::host::EmittedMessage; - - let (tx, mut rx) = tokio::sync::mpsc::channel(10); - let message_tx = Arc::new(tokio::sync::RwLock::new(Some(tx))); - - let rate_limiter = Arc::new(tokio::sync::RwLock::new( - crate::channels::wasm::host::ChannelEmitRateLimiter::new( - crate::channels::wasm::capabilities::EmitRateLimitConfig::default(), - ), - )); - - let messages = vec![EmittedMessage::new("user1", "Just text, no attachments")]; - - let last_broadcast_metadata = Arc::new(tokio::sync::RwLock::new(None)); - let result = WasmChannel::dispatch_emitted_messages( - "test-channel", - messages, - DispatchContext { - message_tx: message_tx.as_ref(), - rate_limiter: rate_limiter.as_ref(), - last_broadcast_metadata: last_broadcast_metadata.as_ref(), - settings_store: None, - }, - ) - .await; - - assert!(result.is_ok()); - - let msg = rx.try_recv().expect("Should receive message"); - assert_eq!(msg.content, "Just text, no attachments"); - assert!(msg.attachments.is_empty()); -} +mod attachments; diff --git a/src/channels/wasm/wrapper/tests/dispatch/attachments.rs b/src/channels/wasm/wrapper/tests/dispatch/attachments.rs new file mode 100644 index 000000000..90c4ebc6b --- /dev/null +++ b/src/channels/wasm/wrapper/tests/dispatch/attachments.rs @@ -0,0 +1,182 @@ +//! Attachment-preservation tests for WASM channel dispatch. +//! +//! Split from the parent module so each stays within the repository's +//! module-size limit. + +use std::sync::Arc; + +use super::super::super::dispatch::DispatchContext; +use crate::channels::wasm::wrapper::WasmChannel; + +/// Expected field values for one [`crate::channels::IncomingAttachment`], used +/// by [`assert_attachment`] to keep call sites concise and named. +struct ExpectedAttachment<'a> { + id: &'a str, + mime_type: &'a str, + filename: Option<&'a str>, + size_bytes: Option, + source_url: Option<&'a str>, + storage_key: Option<&'a str>, + extracted_text: Option<&'a str>, +} + +fn assert_attachment( + attachment: &crate::channels::IncomingAttachment, + expected: &ExpectedAttachment<'_>, +) { + assert_eq!(attachment.id, expected.id); + assert_eq!(attachment.mime_type, expected.mime_type); + assert_eq!(attachment.filename.as_deref(), expected.filename); + assert_eq!(attachment.size_bytes, expected.size_bytes); + assert_eq!(attachment.source_url.as_deref(), expected.source_url); + assert_eq!(attachment.storage_key.as_deref(), expected.storage_key); + assert_eq!( + attachment.extracted_text.as_deref(), + expected.extracted_text + ); +} + +fn build_test_attachments() -> Vec { + use crate::channels::wasm::host::Attachment; + + vec![ + Attachment { + id: "photo123".to_string(), + mime_type: "image/jpeg".to_string(), + filename: Some("cat.jpg".to_string()), + size_bytes: Some(50_000), + source_url: Some("https://api.telegram.org/file/photo123".to_string()), + storage_key: None, + extracted_text: None, + data: Vec::new(), + duration_secs: None, + }, + Attachment { + id: "doc456".to_string(), + mime_type: "application/pdf".to_string(), + filename: Some("report.pdf".to_string()), + size_bytes: Some(120_000), + source_url: None, + storage_key: Some("store/doc456".to_string()), + extracted_text: Some("Report contents...".to_string()), + data: Vec::new(), + duration_secs: None, + }, + ] +} + +async fn dispatch_messages_for_test( + messages: Vec, +) -> ( + Result<(), crate::channels::wasm::error::WasmChannelError>, + tokio::sync::mpsc::Receiver, +) { + let (tx, rx) = tokio::sync::mpsc::channel(10); + let message_tx = Arc::new(tokio::sync::RwLock::new(Some(tx))); + + let rate_limiter = Arc::new(tokio::sync::RwLock::new( + crate::channels::wasm::host::ChannelEmitRateLimiter::new( + crate::channels::wasm::capabilities::EmitRateLimitConfig::default(), + ), + )); + + let last_broadcast_metadata = Arc::new(tokio::sync::RwLock::new(None)); + let result = WasmChannel::dispatch_emitted_messages( + "test-channel", + messages, + DispatchContext { + message_tx: message_tx.as_ref(), + rate_limiter: rate_limiter.as_ref(), + last_broadcast_metadata: last_broadcast_metadata.as_ref(), + settings_store: None, + }, + ) + .await; + + (result, rx) +} + +fn assert_preserved_attachments(msg: &crate::channels::IncomingMessage) { + assert_eq!(msg.attachments.len(), 2); + + // Verify first attachment + assert_attachment( + &msg.attachments[0], + &ExpectedAttachment { + id: "photo123", + mime_type: "image/jpeg", + filename: Some("cat.jpg"), + size_bytes: Some(50_000), + source_url: Some("https://api.telegram.org/file/photo123"), + storage_key: None, + extracted_text: None, + }, + ); + + // Verify second attachment + assert_attachment( + &msg.attachments[1], + &ExpectedAttachment { + id: "doc456", + mime_type: "application/pdf", + filename: Some("report.pdf"), + size_bytes: Some(120_000), + source_url: None, + storage_key: Some("store/doc456"), + extracted_text: Some("Report contents..."), + }, + ); +} + +#[tokio::test] +async fn test_dispatch_emitted_messages_preserves_attachments() { + use crate::channels::wasm::host::EmittedMessage; + + let messages = vec![ + EmittedMessage::new("user1", "Check these files") + .with_attachments(build_test_attachments()), + ]; + + let (result, mut rx) = dispatch_messages_for_test(messages).await; + + assert!(result.is_ok()); + + let msg = rx.try_recv().expect("Should receive message"); + assert_eq!(msg.content, "Check these files"); + assert_preserved_attachments(&msg); +} + +#[tokio::test] +async fn test_dispatch_emitted_messages_no_attachments_backward_compat() { + use crate::channels::wasm::host::EmittedMessage; + + let (tx, mut rx) = tokio::sync::mpsc::channel(10); + let message_tx = Arc::new(tokio::sync::RwLock::new(Some(tx))); + + let rate_limiter = Arc::new(tokio::sync::RwLock::new( + crate::channels::wasm::host::ChannelEmitRateLimiter::new( + crate::channels::wasm::capabilities::EmitRateLimitConfig::default(), + ), + )); + + let messages = vec![EmittedMessage::new("user1", "Just text, no attachments")]; + + let last_broadcast_metadata = Arc::new(tokio::sync::RwLock::new(None)); + let result = WasmChannel::dispatch_emitted_messages( + "test-channel", + messages, + DispatchContext { + message_tx: message_tx.as_ref(), + rate_limiter: rate_limiter.as_ref(), + last_broadcast_metadata: last_broadcast_metadata.as_ref(), + settings_store: None, + }, + ) + .await; + + assert!(result.is_ok()); + + let msg = rx.try_recv().expect("Should receive message"); + assert_eq!(msg.content, "Just text, no attachments"); + assert!(msg.attachments.is_empty()); +} diff --git a/src/channels/web/handlers/skills/tests/helpers.rs b/src/channels/web/handlers/skills/tests/helpers.rs index 537fb5b97..04ab62976 100644 --- a/src/channels/web/handlers/skills/tests/helpers.rs +++ b/src/channels/web/handlers/skills/tests/helpers.rs @@ -3,6 +3,7 @@ use std::io::Write; use std::sync::Arc; +use anyhow::Context as _; use axum::body::to_bytes; use axum::{Router, routing::post}; use rstest::fixture; @@ -18,22 +19,26 @@ pub(crate) struct SkillsApiFixture { pub(crate) installed_root: std::path::PathBuf, } +/// Build the Skills API test fixture. +/// +/// Arrangement can fail, so the fixture is fallible: consumers take the +/// `Result` and unwrap it in the test body, where a failure is the verdict. #[fixture] -pub(crate) fn skills_api_fixture() -> SkillsApiFixture { - let user_dir = tempfile::tempdir().expect("user tempdir should be created"); - let installed_dir = tempfile::tempdir().expect("installed tempdir should be created"); +pub(crate) fn skills_api_fixture() -> anyhow::Result { + let user_dir = tempfile::tempdir().context("user tempdir should be created")?; + let installed_dir = tempfile::tempdir().context("installed tempdir should be created")?; let installed_root = installed_dir.path().to_path_buf(); let registry = SkillRegistry::new(user_dir.path().to_path_buf()) .with_installed_dir(installed_root.clone()); let registry = Arc::new(std::sync::RwLock::new(registry)); let state = TestGatewayBuilder::new().skill_registry(registry).build(); - SkillsApiFixture { + Ok(SkillsApiFixture { _installed_dir: installed_dir, _user_dir: user_dir, state, installed_root, - } + }) } pub(crate) fn skills_router(state: Arc) -> Router { @@ -46,10 +51,11 @@ pub(crate) fn skill_markdown(name: &str) -> String { format!("---\nname: {name}\n---\n\n# {name}\n") } -pub(crate) fn build_bundle_archive(entries: &[(&str, &[u8])]) -> Vec { - crate::skills::test_support::build_bundle_archive(entries) - .expect("test bundle archive should build") -} +/// Re-exported bundle archive builder. +/// +/// Archive construction can fail, so callers unwrap the `Result` in the test +/// body rather than having the helper panic during arrangement. +pub(crate) use crate::skills::test_support::build_bundle_archive; pub(crate) enum MultipartPart<'a> { File { @@ -67,11 +73,15 @@ pub(crate) enum MultipartPart<'a> { }, } +/// Build a single-file multipart body, returning the content type and payload. +/// +/// Body assembly writes into an in-memory buffer, so the write errors are +/// propagated for the test body to unwrap. pub(crate) fn multipart_file_body( field_name: &str, file_name: &str, bytes: &[u8], -) -> (String, Vec) { +) -> std::io::Result<(String, Vec)> { multipart_body(&[MultipartPart::File { field_name, file_name, @@ -79,7 +89,12 @@ pub(crate) fn multipart_file_body( }]) } -pub(crate) fn multipart_body(parts: &[MultipartPart<'_>]) -> (String, Vec) { +/// Build a multipart body from the supplied parts, returning the content type +/// and payload. +/// +/// Body assembly writes into an in-memory buffer, so the write errors are +/// propagated for the test body to unwrap. +pub(crate) fn multipart_body(parts: &[MultipartPart<'_>]) -> std::io::Result<(String, Vec)> { let boundary = "axinite-skill-boundary"; let mut body = Vec::new(); @@ -93,36 +108,37 @@ pub(crate) fn multipart_body(parts: &[MultipartPart<'_>]) -> (String, Vec) { write!( body, "--{boundary}\r\nContent-Disposition: form-data; name=\"{field_name}\"; filename=\"{file_name}\"\r\nContent-Type: application/octet-stream\r\n\r\n" - ) - .expect("multipart file header should write"); + )?; body.extend_from_slice(bytes); } MultipartPart::FileWithoutFilename { field_name, bytes } => { write!( body, "--{boundary}\r\nContent-Disposition: form-data; name=\"{field_name}\"\r\nContent-Type: application/octet-stream\r\n\r\n" - ) - .expect("multipart file header should write"); + )?; body.extend_from_slice(bytes); } MultipartPart::Text { field_name, value } => { write!( body, "--{boundary}\r\nContent-Disposition: form-data; name=\"{field_name}\"\r\n\r\n{value}" - ) - .expect("multipart text field should write"); + )?; } } - write!(body, "\r\n").expect("multipart separator should write"); + write!(body, "\r\n")?; } - write!(body, "\r\n--{boundary}--\r\n").expect("multipart footer should write"); - (format!("multipart/form-data; boundary={boundary}"), body) + write!(body, "\r\n--{boundary}--\r\n")?; + Ok((format!("multipart/form-data; boundary={boundary}"), body)) } -pub(crate) async fn response_text(response: axum::response::Response) -> String { +/// Read a response body as UTF-8 text. +/// +/// Reading and decoding can both fail, so the errors are propagated for the +/// test body to unwrap. +pub(crate) async fn response_text(response: axum::response::Response) -> anyhow::Result { let bytes = to_bytes(response.into_body(), 1024 * 1024) .await - .expect("response body should be readable"); - String::from_utf8(bytes.to_vec()).expect("response body should be UTF-8") + .context("response body should be readable")?; + String::from_utf8(bytes.to_vec()).context("response body should be UTF-8") } diff --git a/src/channels/web/handlers/skills/tests/json.rs b/src/channels/web/handlers/skills/tests/json.rs index dfd084f67..e7673416e 100644 --- a/src/channels/web/handlers/skills/tests/json.rs +++ b/src/channels/web/handlers/skills/tests/json.rs @@ -12,8 +12,11 @@ use crate::channels::web::handlers::install_helpers::MAX_SKILL_INSTALL_REQUEST_B #[rstest] #[tokio::test] -async fn json_skill_install_rejects_multiple_sources(skills_api_fixture: SkillsApiFixture) { - let response = skills_router(Arc::clone(&skills_api_fixture.state)) +async fn json_skill_install_rejects_multiple_sources( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -33,14 +36,19 @@ async fn json_skill_install_rejects_multiple_sources(skills_api_fixture: SkillsA .expect("request should complete"); assert_eq!(response.status(), StatusCode::BAD_REQUEST); - let body = response_text(response).await; + let body = response_text(response) + .await + .expect("response body should be readable"); assert!(body.contains("Provide exactly one"), "body was: {body}"); } #[rstest] #[tokio::test] -async fn json_skill_install_keeps_inline_content_flow(skills_api_fixture: SkillsApiFixture) { - let response = skills_router(Arc::clone(&skills_api_fixture.state)) +async fn json_skill_install_keeps_inline_content_flow( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -59,26 +67,26 @@ async fn json_skill_install_keeps_inline_content_flow(skills_api_fixture: Skills .expect("request should complete"); assert_eq!(response.status(), StatusCode::OK); - let body: serde_json::Value = - serde_json::from_str(&response_text(response).await).expect("JSON response expected"); + let body = response_text(response) + .await + .expect("response body should be readable"); + let body: serde_json::Value = serde_json::from_str(&body).expect("JSON response expected"); assert_eq!(body["success"], true); - assert!( - skills_api_fixture - .installed_root - .join("inline-docs/SKILL.md") - .exists() - ); + assert!(fixture.installed_root.join("inline-docs/SKILL.md").exists()); } #[rstest] #[tokio::test] -async fn json_skill_install_respects_max_request_size(skills_api_fixture: SkillsApiFixture) { +async fn json_skill_install_respects_max_request_size( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let oversized_content = "a".repeat(MAX_SKILL_INSTALL_REQUEST_BYTES + 1); let body = serde_json::json!({ "content": oversized_content, }); - let response = skills_router(Arc::clone(&skills_api_fixture.state)) + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -94,7 +102,9 @@ async fn json_skill_install_respects_max_request_size(skills_api_fixture: Skills .expect("request should complete"); assert_eq!(response.status(), StatusCode::PAYLOAD_TOO_LARGE); - let body = response_text(response).await; + let body = response_text(response) + .await + .expect("response body should be readable"); assert!( body.contains("Request body exceeds maximum size of 10485760 bytes"), "body was: {body}" diff --git a/src/channels/web/handlers/skills/tests/multipart.rs b/src/channels/web/handlers/skills/tests/multipart.rs index fd834f792..0e3e99d79 100644 --- a/src/channels/web/handlers/skills/tests/multipart.rs +++ b/src/channels/web/handlers/skills/tests/multipart.rs @@ -2,6 +2,7 @@ use std::sync::Arc; +use anyhow::Context as _; use axum::body::Body; use axum::http::{Request, StatusCode}; use rstest::rstest; @@ -11,7 +12,10 @@ use super::helpers::*; #[rstest] #[tokio::test] -async fn upload_skill_bundle_preserves_references_and_assets(skills_api_fixture: SkillsApiFixture) { +async fn upload_skill_bundle_preserves_references_and_assets( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[ ( "deploy-docs/SKILL.md", @@ -19,10 +23,12 @@ async fn upload_skill_bundle_preserves_references_and_assets(skills_api_fixture: ), ("deploy-docs/references/usage.md", b"# Usage\n"), ("deploy-docs/assets/logo.txt", b"logo"), - ]); - let (content_type, body) = multipart_file_body("bundle", "deploy-docs.skill", &archive); + ]) + .expect("test bundle archive should build"); + let (content_type, body) = multipart_file_body("bundle", "deploy-docs.skill", &archive) + .expect("multipart body should build"); - let response = skills_router(Arc::clone(&skills_api_fixture.state)) + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -36,11 +42,13 @@ async fn upload_skill_bundle_preserves_references_and_assets(skills_api_fixture: .expect("request should complete"); assert_eq!(response.status(), StatusCode::OK); - let body: serde_json::Value = - serde_json::from_str(&response_text(response).await).expect("JSON response expected"); + let body = response_text(response) + .await + .expect("response body should be readable"); + let body: serde_json::Value = serde_json::from_str(&body).expect("JSON response expected"); assert_eq!(body["success"], true); - let installed = skills_api_fixture.installed_root.join("deploy-docs"); + let installed = fixture.installed_root.join("deploy-docs"); assert!(installed.join("SKILL.md").exists()); assert!(installed.join("references/usage.md").exists()); assert!(installed.join("assets/logo.txt").exists()); @@ -49,17 +57,21 @@ async fn upload_skill_bundle_preserves_references_and_assets(skills_api_fixture: #[rstest] #[tokio::test] async fn upload_skill_bundle_accepts_case_insensitive_content_type( - skills_api_fixture: SkillsApiFixture, + skills_api_fixture: anyhow::Result, ) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); - let (content_type, body) = multipart_file_body("bundle", "deploy-docs.skill", &archive); + )]) + .expect("test bundle archive should build"); + let (content_type, body) = multipart_file_body("bundle", "deploy-docs.skill", &archive) + .expect("multipart body should build"); let content_type = content_type.replacen("multipart/form-data", "Multipart/Form-Data", 1); - let (status, body) = - post_skill_bundle_install(Arc::clone(&skills_api_fixture.state), content_type, body).await; + let (status, body) = post_skill_bundle_install(Arc::clone(&fixture.state), content_type, body) + .await + .expect("skill bundle install request should complete"); assert_eq!(status, StatusCode::OK); let body: serde_json::Value = serde_json::from_str(&body).expect("JSON response expected"); @@ -68,62 +80,72 @@ async fn upload_skill_bundle_accepts_case_insensitive_content_type( /// Send a multipart POST to the skills install endpoint and return the status /// code and response body text. +/// +/// Request construction and dispatch can fail, so the errors are propagated +/// for the test body to unwrap. async fn post_skill_bundle_install( state: Arc, content_type: String, body: Vec, -) -> (StatusCode, String) { +) -> anyhow::Result<(StatusCode, String)> { + let request = Request::builder() + .method("POST") + .uri("/api/skills/install") + .header("x-confirm-action", "true") + .header("content-type", content_type) + .body(Body::from(body)) + .context("request should build")?; let response = skills_router(state) - .oneshot( - Request::builder() - .method("POST") - .uri("/api/skills/install") - .header("x-confirm-action", "true") - .header("content-type", content_type) - .body(Body::from(body)) - .expect("request should build"), - ) + .oneshot(request) .await - .expect("request should complete"); + .context("request should complete")?; let status = response.status(); - let body = response_text(response).await; - (status, body) + let body = response_text(response).await?; + Ok((status, body)) } -fn body_with_missing_filename(archive: Vec) -> (String, Vec) { +/// Builds a multipart request body, and its content type, from a bundle +/// archive. Body construction is fallible, so the rejection cases below can +/// share one function-pointer shape. +type MultipartBodyBuilder = fn(Vec) -> std::io::Result<(String, Vec)>; + +fn body_with_missing_filename(archive: Vec) -> std::io::Result<(String, Vec)> { multipart_body(&[MultipartPart::FileWithoutFilename { field_name: "bundle", bytes: &archive, }]) } -fn body_with_wrong_extension(archive: Vec) -> (String, Vec) { +fn body_with_wrong_extension(archive: Vec) -> std::io::Result<(String, Vec)> { multipart_file_body("bundle", "deploy-docs.zip", &archive) } #[rstest] #[case::missing_filename( - body_with_missing_filename as fn(Vec) -> (String, Vec), + body_with_missing_filename as MultipartBodyBuilder, "Uploaded skill bundle must include a filename ending with .skill", )] #[case::wrong_extension( - body_with_wrong_extension as fn(Vec) -> (String, Vec), + body_with_wrong_extension as MultipartBodyBuilder, "Uploaded skill bundle filename must end with .skill", )] #[tokio::test] async fn upload_skill_bundle_rejects_invalid_bundle_filename( - skills_api_fixture: SkillsApiFixture, - #[case] make_body: fn(Vec) -> (String, Vec), + skills_api_fixture: anyhow::Result, + #[case] make_body: MultipartBodyBuilder, #[case] expected_error: &'static str, ) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); - let (content_type, body) = make_body(archive); + )]) + .expect("test bundle archive should build"); + let (content_type, body) = make_body(archive).expect("multipart body should build"); - let (status, body) = - post_skill_bundle_install(Arc::clone(&skills_api_fixture.state), content_type, body).await; + let (status, body) = post_skill_bundle_install(Arc::clone(&fixture.state), content_type, body) + .await + .expect("skill bundle install request should complete"); assert_eq!(status, StatusCode::BAD_REQUEST); assert!(body.contains(expected_error), "body was: {body}"); @@ -131,11 +153,15 @@ async fn upload_skill_bundle_rejects_invalid_bundle_filename( #[rstest] #[tokio::test] -async fn upload_skill_bundle_rejects_multiple_bundle_fields(skills_api_fixture: SkillsApiFixture) { +async fn upload_skill_bundle_rejects_multiple_bundle_fields( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); + )]) + .expect("test bundle archive should build"); let (content_type, body) = multipart_body(&[ MultipartPart::File { field_name: "bundle", @@ -147,10 +173,12 @@ async fn upload_skill_bundle_rejects_multiple_bundle_fields(skills_api_fixture: file_name: "second.skill", bytes: &archive, }, - ]); + ]) + .expect("multipart body should build"); - let (status, body) = - post_skill_bundle_install(Arc::clone(&skills_api_fixture.state), content_type, body).await; + let (status, body) = post_skill_bundle_install(Arc::clone(&fixture.state), content_type, body) + .await + .expect("skill bundle install request should complete"); assert_eq!(status, StatusCode::BAD_REQUEST); assert!( @@ -166,14 +194,16 @@ async fn upload_skill_bundle_rejects_multiple_bundle_fields(skills_api_fixture: #[case::slug("slug", "owner/deploy-docs")] #[tokio::test] async fn upload_skill_bundle_rejects_additional_source_fields( - skills_api_fixture: SkillsApiFixture, + skills_api_fixture: anyhow::Result, #[case] field_name: &str, #[case] value: &str, ) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); + )]) + .expect("test bundle archive should build"); let (content_type, body) = multipart_body(&[ MultipartPart::File { field_name: "bundle", @@ -181,9 +211,10 @@ async fn upload_skill_bundle_rejects_additional_source_fields( bytes: &archive, }, MultipartPart::Text { field_name, value }, - ]); + ]) + .expect("multipart body should build"); - let response = skills_router(Arc::clone(&skills_api_fixture.state)) + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -197,7 +228,9 @@ async fn upload_skill_bundle_rejects_additional_source_fields( .expect("request should complete"); assert_eq!(response.status(), StatusCode::BAD_REQUEST); - let body = response_text(response).await; + let body = response_text(response) + .await + .expect("response body should be readable"); assert!( body.contains("Provide exactly one of 'content', 'url', 'name'/'slug', or a .skill upload"), "body was: {body}" @@ -211,14 +244,16 @@ async fn upload_skill_bundle_rejects_additional_source_fields( #[case::slug("slug", " \t ")] #[tokio::test] async fn upload_skill_bundle_ignores_whitespace_only_source_fields( - skills_api_fixture: SkillsApiFixture, + skills_api_fixture: anyhow::Result, #[case] field_name: &str, #[case] value: &str, ) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); + )]) + .expect("test bundle archive should build"); let (content_type, body) = multipart_body(&[ MultipartPart::File { field_name: "bundle", @@ -226,9 +261,10 @@ async fn upload_skill_bundle_ignores_whitespace_only_source_fields( bytes: &archive, }, MultipartPart::Text { field_name, value }, - ]); + ]) + .expect("multipart body should build"); - let response = skills_router(Arc::clone(&skills_api_fixture.state)) + let response = skills_router(Arc::clone(&fixture.state)) .oneshot( Request::builder() .method("POST") @@ -246,15 +282,21 @@ async fn upload_skill_bundle_ignores_whitespace_only_source_fields( #[rstest] #[tokio::test] -async fn upload_skill_bundle_reports_archive_shape_errors(skills_api_fixture: SkillsApiFixture) { +async fn upload_skill_bundle_reports_archive_shape_errors( + skills_api_fixture: anyhow::Result, +) { + let fixture = skills_api_fixture.expect("skills API fixture should build"); let archive = build_bundle_archive(&[ ("first/SKILL.md", skill_markdown("first").as_bytes()), ("second/SKILL.md", skill_markdown("second").as_bytes()), - ]); - let (content_type, body) = multipart_file_body("bundle", "broken.skill", &archive); + ]) + .expect("test bundle archive should build"); + let (content_type, body) = multipart_file_body("bundle", "broken.skill", &archive) + .expect("multipart body should build"); - let (status, body) = - post_skill_bundle_install(Arc::clone(&skills_api_fixture.state), content_type, body).await; + let (status, body) = post_skill_bundle_install(Arc::clone(&fixture.state), content_type, body) + .await + .expect("skill bundle install request should complete"); assert_eq!(status, StatusCode::BAD_REQUEST); assert!(body.contains("invalid_skill_bundle"), "body was: {body}"); diff --git a/src/channels/web/server/tests/fixtures.rs b/src/channels/web/server/tests/fixtures.rs index 566d3c6b0..ef3ef2170 100644 --- a/src/channels/web/server/tests/fixtures.rs +++ b/src/channels/web/server/tests/fixtures.rs @@ -2,6 +2,7 @@ use std::sync::Arc; +use anyhow::Context as _; use axum::{Router, routing::get}; use rstest::fixture; @@ -88,12 +89,18 @@ pub(super) fn test_relay_oauth_router() -> TestRelayOAuthRouterFactory { TestRelayOAuthRouterFactory } -pub(super) fn build_test_secrets_store() -> Arc { - Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( - crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( - TEST_GATEWAY_CRYPTO_KEY.to_string(), - )) - .expect("construct test gateway secrets crypto"), +/// Build an in-memory secrets store backed by the test gateway crypto key. +/// +/// Crypto construction can fail, so the error is propagated for the test body +/// to unwrap. +pub(super) fn build_test_secrets_store() +-> anyhow::Result> { + let crypto = crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( + TEST_GATEWAY_CRYPTO_KEY.to_string(), + )) + .context("construct test gateway secrets crypto")?; + Ok(Arc::new(crate::secrets::InMemorySecretsStore::new( + Arc::new(crypto), ))) } @@ -125,10 +132,14 @@ pub(super) fn build_test_ext_mgr( )) } +/// Build a pending OAuth flow whose creation timestamp is already expired. +/// +/// The backdated timestamp can fail on a freshly booted host, so the error is +/// propagated for the test body to unwrap. pub(super) fn expired_pending_oauth_flow( secrets: Arc, -) -> crate::cli::oauth_defaults::PendingOAuthFlow { - crate::cli::oauth_defaults::PendingOAuthFlow { +) -> anyhow::Result { + Ok(crate::cli::oauth_defaults::PendingOAuthFlow { extension_name: "test_tool".to_string(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), @@ -147,6 +158,6 @@ pub(super) fn expired_pending_oauth_flow( gateway_token: None, created_at: std::time::Instant::now() .checked_sub(std::time::Duration::from_secs(600)) - .expect("system uptime is too low to run expired OAuth flow tests"), - } + .context("system uptime is too low to run expired OAuth flow tests")?, + }) } diff --git a/src/channels/web/server/tests/oauth.rs b/src/channels/web/server/tests/oauth.rs index 7bcb61d23..a973b1de4 100644 --- a/src/channels/web/server/tests/oauth.rs +++ b/src/channels/web/server/tests/oauth.rs @@ -1,5 +1,6 @@ //! Tests for the hosted gateway OAuth callback handler. +use anyhow::Context as _; use axum::body::Body; use rstest::rstest; use tower::ServiceExt; @@ -9,21 +10,30 @@ use super::fixtures::{ expired_pending_oauth_flow, test_gateway_state, test_oauth_router, }; -async fn oauth_failure_html(app: axum::Router, uri: &str, context: &str) -> String { +/// Drive an OAuth callback request and return the rendered HTML page. +/// +/// Request construction, dispatch, and body reading can all fail, so the +/// errors are propagated for the test body to unwrap. `request_context` +/// labels a request-construction failure. +async fn oauth_failure_html( + app: axum::Router, + uri: &str, + request_context: &str, +) -> anyhow::Result { let req = axum::http::Request::builder() .uri(uri) .body(Body::empty()) - .expect(context); + .with_context(|| request_context.to_string())?; let resp = ServiceExt::>::oneshot(app, req) .await - .expect("send OAuth callback failure-path request"); + .context("send OAuth callback failure-path request")?; assert_eq!(resp.status(), axum::http::StatusCode::OK); let body = axum::body::to_bytes(resp.into_body(), 1024 * 64) .await - .expect("read OAuth callback failure-path response body"); - String::from_utf8_lossy(&body).into_owned() + .context("read OAuth callback failure-path response body")?; + Ok(String::from_utf8_lossy(&body).into_owned()) } #[rstest] @@ -37,7 +47,9 @@ async fn test_oauth_callback_basic_failure_paths( ) { let state = test_gateway_state.build(None, None); let app = test_oauth_router.build(state); - let html = oauth_failure_html(app, uri, "build OAuth callback failure-path request").await; + let html = oauth_failure_html(app, uri, "build OAuth callback failure-path request") + .await + .expect("OAuth callback failure path should render an HTML page"); assert!(html.contains("Authorization Failed")); } @@ -54,7 +66,8 @@ async fn test_oauth_callback_stateful_failure_paths( let extension_manager = if without_extension_manager { None } else { - Some(build_test_ext_mgr(build_test_secrets_store())) + let secrets = build_test_secrets_store().expect("test secrets store should build"); + Some(build_test_ext_mgr(secrets)) }; let state = test_gateway_state.build(extension_manager, None); let app = test_oauth_router.build(state); @@ -63,7 +76,8 @@ async fn test_oauth_callback_stateful_failure_paths( uri, "build OAuth callback stateful failure-path request", ) - .await; + .await + .expect("OAuth callback stateful failure path should render an HTML page"); assert!(html.contains("Authorization Failed")); } @@ -73,10 +87,10 @@ async fn test_oauth_callback_expired_flow( test_gateway_state: TestGatewayStateFactory, test_oauth_router: TestOAuthRouterFactory, ) { - let secrets = build_test_secrets_store(); + let secrets = build_test_secrets_store().expect("test secrets store should build"); let ext_mgr = build_test_ext_mgr(std::sync::Arc::clone(&secrets)); - let flow = expired_pending_oauth_flow(secrets); + let flow = expired_pending_oauth_flow(secrets).expect("expired OAuth flow should build"); ext_mgr .pending_oauth_flows() @@ -92,7 +106,8 @@ async fn test_oauth_callback_expired_flow( "/oauth/callback?code=test_code&state=expired_state", "build OAuth callback request for expired flow", ) - .await; + .await + .expect("OAuth callback expired-flow path should render an HTML page"); assert!(html.contains("Authorization Failed")); } @@ -102,10 +117,10 @@ async fn test_oauth_callback_strips_instance_prefix( test_gateway_state: TestGatewayStateFactory, test_oauth_router: TestOAuthRouterFactory, ) { - let secrets = build_test_secrets_store(); + let secrets = build_test_secrets_store().expect("test secrets store should build"); let ext_mgr = build_test_ext_mgr(std::sync::Arc::clone(&secrets)); - let flow = expired_pending_oauth_flow(secrets); + let flow = expired_pending_oauth_flow(secrets).expect("expired OAuth flow should build"); ext_mgr .pending_oauth_flows() diff --git a/src/channels/web/server/tests/relay_oauth.rs b/src/channels/web/server/tests/relay_oauth.rs index 33a511342..484bf14ce 100644 --- a/src/channels/web/server/tests/relay_oauth.rs +++ b/src/channels/web/server/tests/relay_oauth.rs @@ -16,7 +16,8 @@ async fn test_relay_oauth_callback_missing_state_param( test_gateway_state: TestGatewayStateFactory, test_relay_oauth_router: TestRelayOAuthRouterFactory, ) { - let ext_mgr = build_test_ext_mgr(build_test_secrets_store()); + let secrets = build_test_secrets_store().expect("test secrets store should build"); + let ext_mgr = build_test_ext_mgr(secrets); let state = test_gateway_state.build(Some(ext_mgr), None); let app = test_relay_oauth_router.build(state); @@ -46,7 +47,7 @@ async fn test_relay_oauth_callback_wrong_state_param( test_gateway_state: TestGatewayStateFactory, test_relay_oauth_router: TestRelayOAuthRouterFactory, ) { - let secrets = build_test_secrets_store(); + let secrets = build_test_secrets_store().expect("test secrets store should build"); secrets .create( "test", @@ -88,7 +89,7 @@ async fn test_relay_oauth_callback_correct_state_proceeds( test_gateway_state: TestGatewayStateFactory, test_relay_oauth_router: TestRelayOAuthRouterFactory, ) { - let secrets = build_test_secrets_store(); + let secrets = build_test_secrets_store().expect("test secrets store should build"); let nonce = "valid-test-nonce-12345"; secrets diff --git a/src/history/migrations/tests.rs b/src/history/migrations/tests.rs index acdbad468..07b420a92 100644 --- a/src/history/migrations/tests.rs +++ b/src/history/migrations/tests.rs @@ -61,14 +61,20 @@ async fn run_repair_postgres_refinery_history_case( .await .context("Failed to connect to database")?; let mut client = store.conn().await.context("Failed to get connection")?; - create_temp_refinery_history_table(&client).await; + create_temp_refinery_history_table(&client) + .await + .context("Failed to create temp history table")?; match seed { RepairSeed::RenumberedReleaseWindow => { - seed_history_rows(&**client, RENUMBERED_RELEASE_WINDOW_ROWS).await; + seed_history_rows(&**client, RENUMBERED_RELEASE_WINDOW_ROWS) + .await + .context("Failed to seed history row")?; } RepairSeed::LegacyChecksumOnlyV12 => { - seed_legacy_released_rows(&**client).await; + seed_legacy_released_rows(&**client) + .await + .context("Failed to seed history row")?; } } @@ -141,8 +147,12 @@ async fn stage_and_finalize_migration_history_rewrites_two_phases() { .await .expect("Failed to connect to database"); let mut client = store.conn().await.expect("Failed to get connection"); - create_temp_refinery_history_table(&client).await; - seed_history_rows(&**client, RENUMBERED_RELEASE_WINDOW_ROWS).await; + create_temp_refinery_history_table(&client) + .await + .expect("Failed to create temp history table"); + seed_history_rows(&**client, RENUMBERED_RELEASE_WINDOW_ROWS) + .await + .expect("Failed to seed history row"); let rewrites = plan_migration_history_rewrites(&renumbered_release_window_applied_rows()) .expect("released migration identities parse"); diff --git a/src/history/migrations/tests/postgres_testing.rs b/src/history/migrations/tests/postgres_testing.rs index c0d98b7bc..3f68ea883 100644 --- a/src/history/migrations/tests/postgres_testing.rs +++ b/src/history/migrations/tests/postgres_testing.rs @@ -79,8 +79,14 @@ pub(super) fn rows_to_tuples(rows: Vec) -> Vec<(i32, String, String)> { .collect() } +/// Create the temporary refinery history table used by the repair tests. +/// +/// Staging the table is arrangement, so the database error propagates to the +/// caller rather than panicking here. #[cfg(feature = "postgres")] -pub(super) async fn create_temp_refinery_history_table(client: &Client) { +pub(super) async fn create_temp_refinery_history_table( + client: &Client, +) -> Result<(), tokio_postgres::Error> { client .batch_execute( "CREATE TEMP TABLE refinery_schema_history (\ @@ -90,11 +96,14 @@ pub(super) async fn create_temp_refinery_history_table(client: &Client) { checksum VARCHAR(255)) ON COMMIT DROP;", ) .await - .expect("Failed to create temp history table"); } +/// Seed `rows` into the refinery history table, propagating database errors. #[cfg(feature = "postgres")] -pub(super) async fn seed_history_rows(client: &C, rows: &[(i32, &str, u64)]) { +pub(super) async fn seed_history_rows( + client: &C, + rows: &[(i32, &str, u64)], +) -> Result<(), tokio_postgres::Error> { for (version, name, checksum) in rows { client .execute( @@ -107,13 +116,15 @@ pub(super) async fn seed_history_rows(client: &C, rows: &[(i32 &checksum.to_string(), ], ) - .await - .expect("Failed to seed history row"); + .await?; } + Ok(()) } #[cfg(feature = "postgres")] -pub(super) async fn seed_legacy_released_rows(client: &C) { +pub(super) async fn seed_legacy_released_rows( + client: &C, +) -> Result<(), tokio_postgres::Error> { // `LEGACY_RELEASED_ROWS` documents the full rewrite catalogue, but the // real refinery table is keyed by `version`, so only the checksum-only // legacy V12 row can coexist with the canonical V13/V14 rows in one seed. @@ -127,5 +138,5 @@ pub(super) async fn seed_legacy_released_rows(client: &C) { "drop_redundant_wasm_tools_name_index", ), ]; - seed_history_rows(client, &SEED_ROWS).await; + seed_history_rows(client, &SEED_ROWS).await } diff --git a/src/llm/rig_adapter/tests/unsupported_params.rs b/src/llm/rig_adapter/tests/unsupported_params.rs index 1e16559a2..50f4de0d7 100644 --- a/src/llm/rig_adapter/tests/unsupported_params.rs +++ b/src/llm/rig_adapter/tests/unsupported_params.rs @@ -4,8 +4,12 @@ use super::*; use rig::completion::CompletionModel; use rstest::fixture; +/// Build a rig adapter backed by a throwaway OpenAI client. +/// +/// Constructing the client is arrangement and can fail, so the fixture yields +/// a [`Result`] that each test body unwraps. #[fixture] -fn openai_rig_adapter() -> RigAdapter { +fn openai_rig_adapter() -> anyhow::Result> { use rig::client::CompletionClient; use rig::providers::openai; @@ -13,28 +17,34 @@ fn openai_rig_adapter() -> RigAdapter { .api_key("test-key") .base_url("http://localhost:0") .build() - .expect("failed to build test client"); + .map_err(|error| anyhow::anyhow!("failed to build test client: {error}"))?; let client = client.completions_api(); let model = client.completion_model("test-model"); - RigAdapter::new(model, "test-model") + Ok(RigAdapter::new(model, "test-model")) } #[rstest] fn test_with_unsupported_params_populates_set( - openai_rig_adapter: RigAdapter, + openai_rig_adapter: anyhow::Result>, ) { - let adapter = openai_rig_adapter.with_unsupported_params(vec!["temperature".to_string()]); + let adapter = openai_rig_adapter + .expect("build test rig adapter") + .with_unsupported_params(vec!["temperature".to_string()]); assert!(adapter.unsupported_params.contains("temperature")); assert!(!adapter.unsupported_params.contains("max_tokens")); } #[rstest] -fn test_strip_unsupported_completion_params(openai_rig_adapter: RigAdapter) { - let adapter = openai_rig_adapter.with_unsupported_params(vec![ - "temperature".to_string(), - "stop_sequences".to_string(), - ]); +fn test_strip_unsupported_completion_params( + openai_rig_adapter: anyhow::Result>, +) { + let adapter = openai_rig_adapter + .expect("build test rig adapter") + .with_unsupported_params(vec![ + "temperature".to_string(), + "stop_sequences".to_string(), + ]); let mut req = CompletionRequest::new(vec![ChatMessage::user("hi")]); req.temperature = Some(0.7); @@ -52,8 +62,11 @@ fn test_strip_unsupported_completion_params(openai_rig_adapter: RigAdapter) { +fn test_strip_unsupported_tool_params( + openai_rig_adapter: anyhow::Result>, +) { let adapter = openai_rig_adapter + .expect("build test rig adapter") .with_unsupported_params(vec!["temperature".to_string(), "max_tokens".to_string()]); let mut req = ToolCompletionRequest::new(vec![ChatMessage::user("hi")], vec![]); @@ -67,6 +80,10 @@ fn test_strip_unsupported_tool_params(openai_rig_adapter: RigAdapter) { - assert!(openai_rig_adapter.unsupported_params.is_empty()); +fn test_unsupported_params_empty_by_default( + openai_rig_adapter: anyhow::Result>, +) { + let adapter = openai_rig_adapter.expect("build test rig adapter"); + + assert!(adapter.unsupported_params.is_empty()); } diff --git a/src/orchestrator/api/tests/fixtures/remote_tool_helpers.rs b/src/orchestrator/api/tests/fixtures/remote_tool_helpers.rs index 6e6b09d5e..6231c201e 100644 --- a/src/orchestrator/api/tests/fixtures/remote_tool_helpers.rs +++ b/src/orchestrator/api/tests/fixtures/remote_tool_helpers.rs @@ -2,6 +2,7 @@ use std::sync::Arc; +use anyhow::Context as _; use axum::body::Body; use axum::http::{Request, StatusCode}; use tower::ServiceExt; @@ -11,11 +12,15 @@ use crate::orchestrator::api::{OrchestratorApi, OrchestratorState}; use crate::tools::Tool; use crate::worker::api::REMOTE_TOOL_EXECUTE_ROUTE; +/// Register `tool` and return the status of a hosted remote-tool execution. +/// +/// Building and dispatching the request is arrangement, so failures propagate +/// to the caller; the test body unwraps before asserting on the status. pub(crate) async fn execute_remote_tool_status( test_state: OrchestratorState, tool: Arc, tool_name: &str, -) -> StatusCode { +) -> anyhow::Result { if crate::tools::ToolRegistry::is_protected_tool_name(tool.name()) { test_state.tools.register_sync(Arc::clone(&tool)); } else { @@ -25,23 +30,23 @@ pub(crate) async fn execute_remote_tool_status( let token = test_state.token_store.create_token(job_id).await; let router = OrchestratorApi::router(test_state); + let payload = serde_json::to_vec(&serde_json::json!({ + "tool_name": tool_name, + "params": {} + })) + .context("serialize hosted remote-tool execute payload")?; + let req = Request::builder() .method("POST") .uri(REMOTE_TOOL_EXECUTE_ROUTE.replace("{job_id}", &job_id.to_string())) .header("Authorization", format!("Bearer {}", token)) .header("Content-Type", "application/json") - .body(Body::from( - serde_json::to_vec(&serde_json::json!({ - "tool_name": tool_name, - "params": {} - })) - .expect("serialize hosted remote-tool execute payload"), - )) - .expect("build hosted remote-tool execute request"); + .body(Body::from(payload)) + .context("build hosted remote-tool execute request")?; - router + let response = router .oneshot(req) .await - .expect("send hosted remote-tool execute request") - .status() + .context("send hosted remote-tool execute request")?; + Ok(response.status()) } diff --git a/src/orchestrator/api/tests/remote_tools/execute.rs b/src/orchestrator/api/tests/remote_tools/execute.rs index 916c8d4d1..d025fa355 100644 --- a/src/orchestrator/api/tests/remote_tools/execute.rs +++ b/src/orchestrator/api/tests/remote_tools/execute.rs @@ -50,7 +50,8 @@ async fn remote_tool_execute_rejects_non_catalog_tools(test_state: OrchestratorS build_tool_fixture(ToolFixture::ContainerOnly), "remote_tool_execute_container", ) - .await; + .await + .expect("execute hosted remote tool"); assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -71,7 +72,8 @@ async fn remote_tool_execute_rejects_protected_orchestration_tools(test_state: O }), "create_job", ) - .await; + .await + .expect("execute hosted remote tool"); assert_eq!(status, StatusCode::BAD_REQUEST); } @@ -83,7 +85,8 @@ async fn remote_tool_execute_rejects_approval_gated_tools(test_state: Orchestrat build_tool_fixture(ToolFixture::ApprovalGated), "remote_tool_execute_gated", ) - .await; + .await + .expect("execute hosted remote tool"); assert_eq!(status, StatusCode::FORBIDDEN); } @@ -95,7 +98,8 @@ async fn remote_tool_execute_allows_hosted_wasm_tools(test_state: OrchestratorSt build_tool_fixture(ToolFixture::CatalogWasm), "remote_tool_catalog_fixture_wasm", ) - .await; + .await + .expect("execute hosted remote tool"); assert_eq!(status, StatusCode::OK); } @@ -135,7 +139,8 @@ async fn remote_tool_execute_maps_error_statuses( }), tool_name, ) - .await; + .await + .expect("execute hosted remote tool"); assert_eq!(status, expected_status); } diff --git a/src/skills/registry/tests/discovery.rs b/src/skills/registry/tests/discovery.rs index 043420b64..68a888c32 100644 --- a/src/skills/registry/tests/discovery.rs +++ b/src/skills/registry/tests/discovery.rs @@ -93,9 +93,10 @@ async fn test_load_skill_layout( #[case] skill_name: &str, #[case] content: &str, #[case] content_fragment: &str, - fresh_registry_fixture: FreshRegistryFixture, + fresh_registry_fixture: std::io::Result, ) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); let expected_root = match layout { LayoutKind::Flat => dir.path().to_path_buf(), LayoutKind::Subdirectory => dir.path().join(skill_name), @@ -104,6 +105,7 @@ async fn test_load_skill_layout( LayoutKind::Flat => write_skill_flat(dir.path(), content), LayoutKind::Subdirectory => write_skill_subdir(dir.path(), skill_name, content), } + .expect("skill under test should be written"); assert_single_skill_loaded(&mut registry, skill_name, content_fragment).await; let skill = registry .find_by_name(skill_name) @@ -146,13 +148,17 @@ async fn test_workspace_overrides_user() { #[rstest] #[tokio::test] -async fn test_gating_failure_skips_skill(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_gating_failure_skips_skill( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "gated-skill", "---\nname: gated-skill\nmetadata:\n openclaw:\n requires:\n bins: [\"__nonexistent_bin__\"]\n---\n\nGated prompt.\n", - ); + ) + .expect("skill under test should be written"); let loaded = registry.discover_all().await; assert!(loaded.is_empty()); } @@ -181,35 +187,45 @@ async fn test_symlink_rejected() { #[rstest] #[tokio::test] -async fn test_file_size_limit(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_file_size_limit(fresh_registry_fixture: std::io::Result) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); let big_content = format!( "---\nname: big-skill\n---\n\n{}", "x".repeat((crate::skills::MAX_PROMPT_FILE_SIZE + 1) as usize) ); - write_skill_subdir(dir.path(), "big-skill", &big_content); + write_skill_subdir(dir.path(), "big-skill", &big_content) + .expect("skill under test should be written"); let loaded = registry.discover_all().await; assert!(loaded.is_empty()); } #[rstest] #[tokio::test] -async fn test_invalid_skill_md_skipped(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; - write_skill_subdir(dir.path(), "bad-skill", "Just plain text"); +async fn test_invalid_skill_md_skipped( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); + write_skill_subdir(dir.path(), "bad-skill", "Just plain text") + .expect("skill under test should be written"); let loaded = registry.discover_all().await; assert!(loaded.is_empty()); } #[rstest] #[tokio::test] -async fn test_line_ending_normalization(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_line_ending_normalization( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "crlf-skill", "---\r\nname: crlf-skill\r\n---\r\n\r\nline1\r\nline2\r\n", - ); + ) + .expect("skill under test should be written"); registry.discover_all().await; assert_eq!(registry.count(), 1); @@ -219,14 +235,18 @@ async fn test_line_ending_normalization(fresh_registry_fixture: FreshRegistryFix #[rstest] #[tokio::test] -async fn test_token_budget_rejection(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_token_budget_rejection( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); let big_prompt = "word ".repeat(4000); let content = format!( "---\nname: big-prompt\nactivation:\n max_context_tokens: 100\n---\n\n{}", big_prompt ); - write_skill_subdir(dir.path(), "big-prompt", &content); + write_skill_subdir(dir.path(), "big-prompt", &content) + .expect("skill under test should be written"); let loaded = registry.discover_all().await; assert!(loaded.is_empty()); } @@ -237,14 +257,16 @@ async fn test_token_budget_rejection(fresh_registry_fixture: FreshRegistryFixtur #[tokio::test] async fn test_bundle_layout_records_bundle_package_kind( #[case] marker_file: &str, - fresh_registry_fixture: FreshRegistryFixture, + fresh_registry_fixture: std::io::Result, ) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "bundle-skill", "---\nname: bundle-skill\n---\n\nBundle prompt.\n", - ); + ) + .expect("skill under test should be written"); let marker_path = dir.path().join("bundle-skill").join(marker_file); fs::create_dir_all( marker_path @@ -273,14 +295,16 @@ async fn test_bundle_layout_records_bundle_package_kind( #[tokio::test] async fn test_bundle_marker_files_do_not_change_package_kind( #[case] marker_name: &str, - fresh_registry_fixture: FreshRegistryFixture, + fresh_registry_fixture: std::io::Result, ) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "plain-skill", "---\nname: plain-skill\n---\n\nPlain prompt.\n", - ); + ) + .expect("skill under test should be written"); fs::write( dir.path().join("plain-skill").join(marker_name), "not a directory\n", @@ -325,13 +349,17 @@ async fn test_mixed_flat_and_subdirectory_layout() { #[rstest] #[tokio::test] -async fn test_lowercased_fields_populated(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_lowercased_fields_populated( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "case-skill", "---\nname: case-skill\nactivation:\n keywords: [\"Write\", \"EDIT\"]\n tags: [\"Email\", \"PROSE\"]\n---\n\nTest prompt.\n", - ); + ) + .expect("skill under test should be written"); registry.discover_all().await; let skill = registry @@ -343,13 +371,17 @@ async fn test_lowercased_fields_populated(fresh_registry_fixture: FreshRegistryF #[rstest] #[tokio::test] -async fn test_reload_clears_and_rediscovers(fresh_registry_fixture: FreshRegistryFixture) { - let FreshRegistryFixture { dir, mut registry } = fresh_registry_fixture; +async fn test_reload_clears_and_rediscovers( + fresh_registry_fixture: std::io::Result, +) { + let FreshRegistryFixture { dir, mut registry } = + fresh_registry_fixture.expect("fresh registry fixture should be created"); write_skill_subdir( dir.path(), "persist-skill", "---\nname: persist-skill\n---\n\nPrompt.\n", - ); + ) + .expect("skill under test should be written"); registry.discover_all().await; assert_eq!(registry.count(), 1); diff --git a/src/skills/registry/tests/fixtures.rs b/src/skills/registry/tests/fixtures.rs index 284ecb0c8..fab6ed764 100644 --- a/src/skills/registry/tests/fixtures.rs +++ b/src/skills/registry/tests/fixtures.rs @@ -10,6 +10,10 @@ //! - [`skill_markdown`] — generates minimal valid `SKILL.md` content. //! - [`write_skill_subdir`] / [`write_skill_flat`] — write `SKILL.md` into //! a temp directory in subdirectory or flat layout respectively. +//! +//! Arrangement can fail, so every helper here that touches the filesystem is +//! fallible. Callers in test bodies unwrap the returned `Result`; a failure +//! there is the test verdict. use std::path::Path; use rstest::fixture; @@ -31,42 +35,56 @@ pub(super) fn skill_markdown(name: &str) -> String { format!("---\nname: {name}\n---\n\n# {name}\n") } -pub(super) fn build_bundle_archive(entries: &[(&str, &[u8])]) -> Vec { +/// Builds a `.skill` archive from `entries`, propagating archive failures. +pub(super) fn build_bundle_archive( + entries: &[(&str, &[u8])], +) -> Result, zip::result::ZipError> { crate::skills::test_support::build_bundle_archive(entries) - .expect("test bundle archive should build") } +/// Builds a registry with both a user and an installed directory. +/// +/// Returns an error if either temporary directory cannot be created. #[fixture] -pub(super) fn bundle_install_fixture() -> BundleInstallFixture { - let user_dir = tempfile::tempdir().expect("user tempdir should be created for test"); - let installed_dir = tempfile::tempdir().expect("installed tempdir should be created for test"); +pub(super) fn bundle_install_fixture() -> std::io::Result { + let user_dir = tempfile::tempdir()?; + let installed_dir = tempfile::tempdir()?; let registry = SkillRegistry::new(user_dir.path().to_path_buf()) .with_installed_dir(installed_dir.path().to_path_buf()); - BundleInstallFixture { + Ok(BundleInstallFixture { user_dir, installed_dir, registry, - } + }) } +/// Builds a registry backed by a single temporary directory. +/// +/// Returns an error if the temporary directory cannot be created. #[fixture] -pub(super) fn fresh_registry_fixture() -> FreshRegistryFixture { - let dir = tempfile::tempdir().expect("temp dir should be created for test"); +pub(super) fn fresh_registry_fixture() -> std::io::Result { + let dir = tempfile::tempdir()?; let registry = SkillRegistry::new(dir.path().to_path_buf()); - FreshRegistryFixture { dir, registry } + Ok(FreshRegistryFixture { dir, registry }) } /// Writes `content` to `//SKILL.md`, creating the subdirectory. -pub(super) fn write_skill_subdir(root: &Path, skill_name: &str, content: &str) { +/// +/// Returns an error if the subdirectory or the file cannot be written. +pub(super) fn write_skill_subdir( + root: &Path, + skill_name: &str, + content: &str, +) -> std::io::Result<()> { let skill_dir = root.join(skill_name); - ambient_fs::create_dir(&skill_dir).expect("skill subdirectory should be created for test"); + ambient_fs::create_dir(&skill_dir)?; ambient_fs::write(skill_dir.join("SKILL.md"), content) - .expect("SKILL.md should be written for test"); } /// Writes `content` to `/SKILL.md` (flat layout). -pub(super) fn write_skill_flat(root: &Path, content: &str) { +/// +/// Returns an error if the file cannot be written. +pub(super) fn write_skill_flat(root: &Path, content: &str) -> std::io::Result<()> { ambient_fs::write(root.join("SKILL.md"), content) - .expect("flat SKILL.md should be written for test"); } diff --git a/src/skills/registry/tests/install.rs b/src/skills/registry/tests/install.rs index f94dfbdbf..65d09d106 100644 --- a/src/skills/registry/tests/install.rs +++ b/src/skills/registry/tests/install.rs @@ -4,6 +4,8 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use anyhow::Context as _; + mod lifecycle; mod payloads; @@ -29,26 +31,37 @@ fn documented_bundle_entries() -> Vec<(&'static str, &'static [u8])> { ] } -fn collect_installed_files(root: &Path) -> BTreeMap> { - fn visit(base: &Path, current: &Path, files: &mut BTreeMap>) { - for entry in ambient_fs::read_dir(current).expect("installed directory should be readable") - { - let entry = entry.expect("installed directory entry should be readable"); +/// Recursively collects every installed file under `root`, keyed by its +/// path relative to `root`. +/// +/// Returns an error if the tree cannot be walked or a file cannot be read. +fn collect_installed_files(root: &Path) -> anyhow::Result>> { + fn visit( + base: &Path, + current: &Path, + files: &mut BTreeMap>, + ) -> anyhow::Result<()> { + let entries = + ambient_fs::read_dir(current).context("installed directory should be readable")?; + for entry in entries { + let entry = entry.context("installed directory entry should be readable")?; let path = entry.path(); if path.is_dir() { - visit(base, &path, files); + visit(base, &path, files)?; } else { let relative = path .strip_prefix(base) - .expect("installed file should be under bundle root") + .context("installed file should be under bundle root")? .to_path_buf(); - let contents = ambient_fs::read(&path).expect("installed file should be readable"); + let contents = + ambient_fs::read(&path).context("installed file should be readable")?; files.insert(relative, contents); } } + Ok(()) } let mut files = BTreeMap::new(); - visit(root, root, &mut files); - files + visit(root, root, &mut files)?; + Ok(files) } diff --git a/src/skills/registry/tests/install/lifecycle.rs b/src/skills/registry/tests/install/lifecycle.rs index 34d2b0aa0..35e0bc25e 100644 --- a/src/skills/registry/tests/install/lifecycle.rs +++ b/src/skills/registry/tests/install/lifecycle.rs @@ -27,18 +27,19 @@ async fn test_install_duplicate_rejected() { #[rstest] #[tokio::test] async fn test_cleanup_prepared_install_removes_staged_bundle_on_commit_failure( - bundle_install_fixture: BundleInstallFixture, + bundle_install_fixture: std::io::Result, ) { let BundleInstallFixture { user_dir: _user_dir, installed_dir: _installed_dir, mut registry, - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); let archive = build_bundle_archive(&[( "deploy-docs/SKILL.md", skill_markdown("deploy-docs").as_bytes(), - )]); + )]) + .expect("test bundle archive should build"); let first = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), @@ -79,12 +80,14 @@ async fn test_cleanup_prepared_install_removes_staged_bundle_on_commit_failure( #[rstest] #[tokio::test] -async fn test_remove_bundle_skill_allows_reinstall(bundle_install_fixture: BundleInstallFixture) { +async fn test_remove_bundle_skill_allows_reinstall( + bundle_install_fixture: std::io::Result, +) { let BundleInstallFixture { installed_dir, mut registry, .. - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); let archive = build_bundle_archive(&[ ( @@ -93,7 +96,8 @@ async fn test_remove_bundle_skill_allows_reinstall(bundle_install_fixture: Bundl ), ("deploy-docs/references/usage.md", b"# Usage\n"), ("deploy-docs/assets/logo.txt", b"logo"), - ]); + ]) + .expect("test bundle archive should build"); let prepared = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), @@ -133,15 +137,16 @@ async fn test_remove_bundle_skill_allows_reinstall(bundle_install_fixture: Bundl #[rstest] #[tokio::test] async fn test_prepare_install_cleans_staged_dir_when_validation_fails( - bundle_install_fixture: BundleInstallFixture, + bundle_install_fixture: std::io::Result, ) { let BundleInstallFixture { installed_dir, registry, .. - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); - let archive = build_bundle_archive(&[("deploy-docs/SKILL.md", b"not valid skill markdown")]); + let archive = build_bundle_archive(&[("deploy-docs/SKILL.md", b"not valid skill markdown")]) + .expect("test bundle archive should build"); let prepare_result = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), diff --git a/src/skills/registry/tests/install/payloads.rs b/src/skills/registry/tests/install/payloads.rs index ec118c5b8..2d7f78149 100644 --- a/src/skills/registry/tests/install/payloads.rs +++ b/src/skills/registry/tests/install/payloads.rs @@ -45,7 +45,7 @@ async fn test_install_skill_from_content() { )] #[tokio::test] async fn test_archive_payload_preserves_files( - bundle_install_fixture: BundleInstallFixture, + bundle_install_fixture: std::io::Result, #[case] make_payload: impl FnOnce(Vec) -> SkillInstallPayload, #[case] prepare_msg: &'static str, #[case] commit_msg: &'static str, @@ -54,7 +54,7 @@ async fn test_archive_payload_preserves_files( user_dir: _user_dir, installed_dir, mut registry, - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); let archive = build_bundle_archive(&[ ( @@ -63,7 +63,8 @@ async fn test_archive_payload_preserves_files( ), ("deploy-docs/references/usage.md", b"# Usage\n"), ("deploy-docs/assets/logo.txt", b"logo"), - ]); + ]) + .expect("test bundle archive should build"); let prepared = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), @@ -104,17 +105,17 @@ async fn test_archive_payload_preserves_files( #[case::archive_bytes(|b| SkillInstallPayload::ArchiveBytes(b))] #[tokio::test] async fn test_install_preserves_references_and_assets_regression_rfc0003( - bundle_install_fixture: BundleInstallFixture, + bundle_install_fixture: std::io::Result, #[case] make_payload: impl FnOnce(Vec) -> SkillInstallPayload, ) { let BundleInstallFixture { installed_dir, mut registry, .. - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); let entries = documented_bundle_entries(); - let archive = build_bundle_archive(&entries); + let archive = build_bundle_archive(&entries).expect("test bundle archive should build"); let prepared = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), make_payload(archive), @@ -143,7 +144,9 @@ async fn test_install_preserves_references_and_assets_regression_rfc0003( ) }) .collect::>(); - assert_eq!(collect_installed_files(&installed_root), expected); + let installed = + collect_installed_files(&installed_root).expect("installed bundle tree should be readable"); + assert_eq!(installed, expected); let skill = registry .find_by_name("deploy-docs") @@ -155,9 +158,10 @@ async fn test_install_preserves_references_and_assets_regression_rfc0003( #[rstest] #[tokio::test] async fn test_uploaded_archive_bytes_reject_plain_markdown( - bundle_install_fixture: BundleInstallFixture, + bundle_install_fixture: std::io::Result, ) { - let BundleInstallFixture { registry, .. } = bundle_install_fixture; + let BundleInstallFixture { registry, .. } = + bundle_install_fixture.expect("bundle install fixture should be created"); let error = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), @@ -174,12 +178,14 @@ async fn test_uploaded_archive_bytes_reject_plain_markdown( #[rstest] #[tokio::test] -async fn test_downloaded_bytes_accept_plain_markdown(bundle_install_fixture: BundleInstallFixture) { +async fn test_downloaded_bytes_accept_plain_markdown( + bundle_install_fixture: std::io::Result, +) { let BundleInstallFixture { installed_dir, mut registry, .. - } = bundle_install_fixture; + } = bundle_install_fixture.expect("bundle install fixture should be created"); let prepared = SkillRegistry::prepare_install_to_disk( registry.install_target_dir(), diff --git a/src/skills/registry/tests/prop_tests.rs b/src/skills/registry/tests/prop_tests.rs index c56dc0d8b..8eb774e36 100644 --- a/src/skills/registry/tests/prop_tests.rs +++ b/src/skills/registry/tests/prop_tests.rs @@ -4,6 +4,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; use std::path::PathBuf; +use anyhow::Context as _; use proptest::prelude::*; use crate::skills::registry::{SkillInstallPayload, SkillRegistry}; @@ -55,28 +56,39 @@ fn skill_markdown(name: &str) -> Vec { format!("---\nname: {name}\n---\n\n# {name}\n").into_bytes() } -fn collect_installed_files(root: &Path) -> BTreeMap> { - fn visit(base: &Path, current: &Path, files: &mut BTreeMap>) { - for entry in ambient_fs::read_dir(current).expect("installed directory should be readable") - { - let entry = entry.expect("installed directory entry should be readable"); +/// Recursively collects every installed file under `root`, keyed by its +/// path relative to `root`. +/// +/// Returns an error if the tree cannot be walked or a file cannot be read. +fn collect_installed_files(root: &Path) -> anyhow::Result>> { + fn visit( + base: &Path, + current: &Path, + files: &mut BTreeMap>, + ) -> anyhow::Result<()> { + let entries = + ambient_fs::read_dir(current).context("installed directory should be readable")?; + for entry in entries { + let entry = entry.context("installed directory entry should be readable")?; let path = entry.path(); if path.is_dir() { - visit(base, &path, files); + visit(base, &path, files)?; } else { let relative = path .strip_prefix(base) - .expect("installed file should be under bundle root") + .context("installed file should be under bundle root")? .to_path_buf(); - let contents = ambient_fs::read(&path).expect("installed file should be readable"); + let contents = + ambient_fs::read(&path).context("installed file should be readable")?; files.insert(relative, contents); } } + Ok(()) } let mut files = BTreeMap::new(); - visit(root, root, &mut files); - files + visit(root, root, &mut files)?; + Ok(files) } proptest! { @@ -217,7 +229,10 @@ proptest! { .commit_install(prepared) .expect("generated valid bundle should commit"); let installed_root = installed_dir.path().join("deploy-docs"); - prop_assert_eq!(collect_installed_files(&installed_root), expected); + let installed = collect_installed_files(&installed_root).map_err(|error| { + proptest::test_runner::TestCaseError::fail(error.to_string()) + })?; + prop_assert_eq!(installed, expected); Ok(()) })?; diff --git a/src/tools/builtin/skill_tools/tests/read_file_adapter.rs b/src/tools/builtin/skill_tools/tests/read_file_adapter.rs index 1acc417b6..959dda28b 100644 --- a/src/tools/builtin/skill_tools/tests/read_file_adapter.rs +++ b/src/tools/builtin/skill_tools/tests/read_file_adapter.rs @@ -2,6 +2,7 @@ use std::sync::Arc; +use anyhow::Context as _; use rstest::{fixture, rstest}; use rstest_bdd_macros::{given, scenario, then, when}; @@ -31,14 +32,17 @@ fn skill_read_file_world() -> SkillReadFileWorld { SkillReadFileWorld::default() } +/// Builds an empty registry backed by a temporary directory. +/// +/// Returns an error if the temporary directory cannot be created. #[fixture] -fn test_registry() -> TestRegistryHandle { - let dir = tempfile::tempdir().expect("tempdir creation failed"); +fn test_registry() -> std::io::Result { + let dir = tempfile::tempdir()?; let path = dir.path().to_path_buf(); - TestRegistryHandle { + Ok(TestRegistryHandle { _dir: dir, registry: Arc::new(std::sync::RwLock::new(SkillRegistry::new(path))), - } + }) } #[cfg(target_os = "linux")] @@ -58,6 +62,10 @@ fn documented_bundle_entries() -> Vec<(&'static str, &'static [u8])> { ] } +/// Inserts a `deploy-docs` bundle skill rooted at `root` into `registry`. +/// +/// Returns an error if the location is invalid, the skill cannot be built, +/// the registry lock is poisoned, or the skill cannot be committed. fn insert_deploy_docs_bundle( registry: &Arc>, root: &std::path::Path, @@ -68,21 +76,24 @@ fn insert_deploy_docs_bundle( std::path::PathBuf::from("SKILL.md"), SkillPackageKind::Bundle, ) - .expect("bundle location should be valid"); + .context("bundle location should be valid")?; let skill = crate::skills::test_support::TestSkillBuilder::new("deploy-docs") .location(location) .build()?; registry .write() - .expect("registry lock should be writable") + .map_err(|_| anyhow::anyhow!("registry lock should be writable"))? .commit_loaded_skill("deploy-docs", skill) - .expect("skill should be inserted"); + .context("skill should be inserted")?; Ok(()) } #[rstest] #[tokio::test] -async fn skill_read_file_tool_reads_bundle_reference(test_registry: TestRegistryHandle) { +async fn skill_read_file_tool_reads_bundle_reference( + test_registry: std::io::Result, +) { + let handle = test_registry.expect("test registry should be created"); let bundle_dir = tempfile::tempdir().expect("bundle tempdir should be created"); ambient_fs::create_dir_all(bundle_dir.path().join("references")) .expect("references dir should be created"); @@ -90,10 +101,10 @@ async fn skill_read_file_tool_reads_bundle_reference(test_registry: TestRegistry .expect("SKILL.md should be written"); ambient_fs::write(bundle_dir.path().join("references/usage.md"), "# Usage\n") .expect("reference should be written"); - insert_deploy_docs_bundle(&test_registry.registry, bundle_dir.path()) + insert_deploy_docs_bundle(&handle.registry, bundle_dir.path()) .expect("deploy-docs bundle should be inserted"); - let tool = SkillReadFileTool::new(Arc::clone(&test_registry.registry)); + let tool = SkillReadFileTool::new(Arc::clone(&handle.registry)); let output = NativeTool::execute( &tool, serde_json::json!({ @@ -189,8 +200,11 @@ async fn test_skill_read_file_tool_after_install_returns_non_inline_for_png() #[rstest] #[tokio::test] -async fn skill_read_file_tool_reports_unknown_skill(test_registry: TestRegistryHandle) { - let tool = SkillReadFileTool::new(Arc::clone(&test_registry.registry)); +async fn skill_read_file_tool_reports_unknown_skill( + test_registry: std::io::Result, +) { + let handle = test_registry.expect("test registry should be created"); + let tool = SkillReadFileTool::new(Arc::clone(&handle.registry)); let output = NativeTool::execute( &tool, @@ -208,54 +222,62 @@ async fn skill_read_file_tool_reports_unknown_skill(test_registry: TestRegistryH assert_eq!(output.result["error"]["code"], "unknown_skill"); } +/// Arranges a bundle on disk and registers it, so the step is fallible. #[given("a loaded skill bundle with a referenced usage file")] -fn bdd_loaded_skill_bundle(skill_read_file_world: &mut SkillReadFileWorld) { - let bundle_dir = tempfile::tempdir().expect("bundle tempdir should be created"); +fn bdd_loaded_skill_bundle(skill_read_file_world: &mut SkillReadFileWorld) -> anyhow::Result<()> { + let bundle_dir = tempfile::tempdir().context("bundle tempdir should be created")?; ambient_fs::create_dir_all(bundle_dir.path().join("references")) - .expect("references dir should be created"); + .context("references dir should be created")?; ambient_fs::write(bundle_dir.path().join("SKILL.md"), "# Deploy docs\n") - .expect("SKILL.md should be written"); + .context("SKILL.md should be written")?; ambient_fs::write(bundle_dir.path().join("references/usage.md"), "# Usage\n") - .expect("reference should be written"); + .context("reference should be written")?; let registry = Arc::new(std::sync::RwLock::new(SkillRegistry::new( bundle_dir.path().join("unused-user-dir"), ))); insert_deploy_docs_bundle(®istry, bundle_dir.path()) - .expect("deploy-docs bundle should be inserted"); + .context("deploy-docs bundle should be inserted")?; skill_read_file_world.bundle_dir = Some(bundle_dir); skill_read_file_world.registry = Some(registry); + Ok(()) } #[when("the model calls skill_read_file for the usage file")] -fn bdd_model_reads_usage_file(skill_read_file_world: &mut SkillReadFileWorld) { +fn bdd_model_reads_usage_file( + skill_read_file_world: &mut SkillReadFileWorld, +) -> anyhow::Result<()> { execute_bdd_read( skill_read_file_world, serde_json::json!({ "skill": "deploy-docs", "path": "references/usage.md", }), - ); + ) } #[when("the model calls skill_read_file with a traversal path")] -fn bdd_model_reads_traversal_path(skill_read_file_world: &mut SkillReadFileWorld) { +fn bdd_model_reads_traversal_path( + skill_read_file_world: &mut SkillReadFileWorld, +) -> anyhow::Result<()> { execute_bdd_read( skill_read_file_world, serde_json::json!({ "skill": "deploy-docs", "path": "../secrets.txt", }), - ); + ) } #[then("the tool returns the referenced text without a host filesystem path")] -fn bdd_tool_returns_reference_text(skill_read_file_world: &SkillReadFileWorld) { +fn bdd_tool_returns_reference_text( + skill_read_file_world: &SkillReadFileWorld, +) -> anyhow::Result<()> { let output = skill_read_file_world .output .as_ref() - .expect("When step should execute tool"); + .context("When step should execute tool")?; assert_eq!(output["skill"], "deploy-docs"); assert_eq!(output["path"], "references/usage.md"); assert_eq!(output["content"], "# Usage\n"); @@ -263,36 +285,48 @@ fn bdd_tool_returns_reference_text(skill_read_file_world: &SkillReadFileWorld) { let root = skill_read_file_world .bundle_dir .as_ref() - .expect("Given step should create bundle") + .context("Given step should create bundle")? .path() .to_string_lossy(); assert!(!output.to_string().contains(root.as_ref())); + Ok(()) } #[then("the tool returns a skill-scoped path_not_readable error")] -fn bdd_tool_returns_path_not_readable(skill_read_file_world: &SkillReadFileWorld) { +fn bdd_tool_returns_path_not_readable( + skill_read_file_world: &SkillReadFileWorld, +) -> anyhow::Result<()> { let output = skill_read_file_world .output .as_ref() - .expect("When step should execute tool"); + .context("When step should execute tool")?; assert_eq!(output["skill"], "deploy-docs"); assert_eq!(output["path"], "../secrets.txt"); assert_eq!(output["error"]["code"], "path_not_readable"); + Ok(()) } -fn execute_bdd_read(skill_read_file_world: &mut SkillReadFileWorld, params: serde_json::Value) { +/// Runs the tool against the world's registry and records the result. +/// +/// Returns an error if the Given step has not run, the runtime cannot start, +/// or the tool call fails. +fn execute_bdd_read( + skill_read_file_world: &mut SkillReadFileWorld, + params: serde_json::Value, +) -> anyhow::Result<()> { let registry = Arc::clone( skill_read_file_world .registry .as_ref() - .expect("Given step should create registry"), + .context("Given step should create registry")?, ); let tool = SkillReadFileTool::new(registry); - let runtime = tokio::runtime::Runtime::new().expect("tokio runtime should start"); + let runtime = tokio::runtime::Runtime::new().context("tokio runtime should start")?; let output = runtime .block_on(NativeTool::execute(&tool, params, &JobContext::default())) - .expect("skill_read_file should return a tool output"); + .context("skill_read_file should return a tool output")?; skill_read_file_world.output = Some(output.result); + Ok(()) } #[scenario( diff --git a/src/tools/schema_validator/tests/fixture_groups.rs b/src/tools/schema_validator/tests/fixture_groups.rs index 01f0ad622..e8f5e8ece 100644 --- a/src/tools/schema_validator/tests/fixture_groups.rs +++ b/src/tools/schema_validator/tests/fixture_groups.rs @@ -65,7 +65,7 @@ fn job_tool_schemas() -> Vec<(String, serde_json::Value)> { .collect() } -fn skill_tool_schemas() -> Vec<(String, serde_json::Value)> { +fn skill_tool_schemas() -> anyhow::Result> { use std::sync::Arc; use crate::skills::catalog::SkillCatalog; @@ -75,7 +75,7 @@ fn skill_tool_schemas() -> Vec<(String, serde_json::Value)> { SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool, }; - let dir = tempfile::tempdir().expect("tempdir"); + let dir = tempfile::tempdir().context("failed to create temp skills dir")?; let path = dir.path().to_path_buf(); let registry = Arc::new(std::sync::RwLock::new(SkillRegistry::new(path))); let catalogue = Arc::new(SkillCatalog::with_url("http://127.0.0.1:1")); @@ -93,10 +93,10 @@ fn skill_tool_schemas() -> Vec<(String, serde_json::Value)> { Box::new(SkillRemoveTool::new(Arc::clone(®istry))), ]; - tools + Ok(tools .into_iter() .map(|tool| (tool.name().to_string(), tool.parameters_schema())) - .collect() + .collect()) } /// Validate schemas from tools that cannot be easily constructed by @@ -143,7 +143,7 @@ fn validate_named_schemas(schemas: Vec<(String, serde_json::Value)>, context: &s #[rstest] #[case::simple(simple_tool_schemas().expect("simple tool schemas should build"), "simple tool schemas")] #[case::jobs(job_tool_schemas(), "job tool schemas")] -#[case::skills(skill_tool_schemas(), "skill tool schemas")] +#[case::skills(skill_tool_schemas().expect("skill tool schemas should build"), "skill tool schemas")] #[case::complex(complex_tool_schemas().expect("complex tool schema fixtures should load"), "inline schemas")] fn test_schema_fixture_groups( #[case] schemas: Vec<(String, serde_json::Value)>, diff --git a/src/worker/api/tests/client_methods.rs b/src/worker/api/tests/client_methods.rs index 4cd5f1085..fb6f1f900 100644 --- a/src/worker/api/tests/client_methods.rs +++ b/src/worker/api/tests/client_methods.rs @@ -34,16 +34,17 @@ async fn record_auth(headers: &HeaderMap, state: &ClientMethodTestState) { state.auth_headers.lock().await.push(auth); } +/// Bind an ephemeral listener and serve `router` on a background task. +/// +/// Binding is fallible, so the base URL is returned as a `Result`. The spawned +/// task cannot propagate with `?`, so it yields the serve outcome through the +/// join handle rather than unwrapping inside the task. async fn spawn_test_server( router: Router, -) -> anyhow::Result<(String, tokio::task::JoinHandle<()>)> { +) -> anyhow::Result<(String, tokio::task::JoinHandle>)> { let listener = TcpListener::bind("127.0.0.1:0").await?; let addr = listener.local_addr()?; - let handle = tokio::spawn(async move { - axum::serve(listener, router) - .await - .expect("client method test server should run"); - }); + let handle = tokio::spawn(async move { axum::serve(listener, router).await }); Ok((format!("http://{addr}"), handle)) } @@ -57,7 +58,7 @@ async fn setup_for_test( ) -> anyhow::Result<( Arc, WorkerHttpClient, - tokio::task::JoinHandle<()>, + tokio::task::JoinHandle>, )> { let state = Arc::new(ClientMethodTestState::default()); let (base_url, handle) = spawn_test_server(make_router(Arc::clone(&state))).await?; diff --git a/src/worker/api/tests/transport_types.rs b/src/worker/api/tests/transport_types.rs index 3b31ca5cd..ac0b8a8d2 100644 --- a/src/worker/api/tests/transport_types.rs +++ b/src/worker/api/tests/transport_types.rs @@ -15,17 +15,21 @@ use super::fixtures::{ sample_catalog_response, sample_execution_request, sample_execution_response, }; -/// Serialise `value` to JSON and immediately deserialise it back, asserting that the round-trip produces an equal value without field loss. -fn assert_round_trips(value: T) +/// Serialise `value` to JSON and immediately deserialise it back, checking that the round-trip produces an equal value without field loss. +/// +/// Serialisation and deserialisation can both fail, so the helper returns a +/// `Result` and leaves the verdict to the calling test body. +fn assert_round_trips(value: T) -> anyhow::Result<()> where T: Serialize + DeserializeOwned + Debug + PartialEq, { - let serialized = serde_json::to_string(&value).expect("serialise"); - let deserialized: T = serde_json::from_str(&serialized).expect("deserialise"); - assert_eq!( - deserialized, value, - "value must round-trip without field loss" + let serialized = serde_json::to_string(&value)?; + let deserialized: T = serde_json::from_str(&serialized)?; + anyhow::ensure!( + deserialized == value, + "value must round-trip without field loss: {deserialized:?} != {value:?}" ); + Ok(()) } const fn const_str_eq(left: &str, right: &str) -> bool { @@ -118,28 +122,28 @@ fn worker_and_orchestrator_share_remote_tool_route_constants() { fn remote_tool_catalog_response_round_trip_without_field_loss( sample_catalog_response: RemoteToolCatalogResponse, ) { - assert_round_trips(sample_catalog_response); + assert_round_trips(sample_catalog_response).expect("catalog response should round-trip"); } #[rstest] fn remote_tool_execution_request_round_trip_without_field_loss( sample_execution_request: RemoteToolExecutionRequest, ) { - assert_round_trips(sample_execution_request); + assert_round_trips(sample_execution_request).expect("execution request should round-trip"); } #[rstest] fn remote_tool_execution_response_round_trip_without_field_loss( sample_execution_response: RemoteToolExecutionResponse, ) { - assert_round_trips(sample_execution_response); + assert_round_trips(sample_execution_response).expect("execution response should round-trip"); } #[test] fn terminal_result_round_trip_preserves_all_fields() { let result = TerminalResult::success("completed", Some(11)); - assert_round_trips(result); + assert_round_trips(result).expect("terminal result should round-trip"); } #[test] diff --git a/src/worker/claude_bridge/tests/claude_fs_setup.rs b/src/worker/claude_bridge/tests/claude_fs_setup.rs index 663e4df65..c8806636c 100644 --- a/src/worker/claude_bridge/tests/claude_fs_setup.rs +++ b/src/worker/claude_bridge/tests/claude_fs_setup.rs @@ -1,17 +1,22 @@ //! Tests for Claude filesystem setup utilities. +use anyhow::Context as _; use rstest::rstest; use super::{build_permission_settings, copy_dir_recursive}; -fn parse_allow_list(tools: &[String]) -> Vec { - let json_str = build_permission_settings(tools).expect("permission settings should build"); +/// Build the permission settings for `tools` and return the parsed allow list. +/// +/// Building and parsing the settings are both fallible, so the helper +/// propagates failures to the calling test body. +fn parse_allow_list(tools: &[String]) -> anyhow::Result> { + let json_str = build_permission_settings(tools).context("permission settings should build")?; let parsed: serde_json::Value = - serde_json::from_str(&json_str).expect("settings JSON should parse"); - parsed["permissions"]["allow"] + serde_json::from_str(&json_str).context("settings JSON should parse")?; + let allow = parsed["permissions"]["allow"] .as_array() - .expect("allow list should be an array") - .clone() + .context("allow list should be an array")?; + Ok(allow.clone()) } #[rstest] @@ -31,7 +36,7 @@ fn test_build_permission_settings( #[case] expected_len: usize, #[case] expected_entries: Vec>, ) { - let allow = parse_allow_list(&tools); + let allow = parse_allow_list(&tools).expect("allow list should parse"); assert_eq!(allow.len(), expected_len); for (i, expected) in expected_entries.iter().enumerate() { if let Some(val) = expected { diff --git a/src/worker/container/tests/hosted_fidelity.rs b/src/worker/container/tests/hosted_fidelity.rs index 138e4a9e0..620582e7c 100644 --- a/src/worker/container/tests/hosted_fidelity.rs +++ b/src/worker/container/tests/hosted_fidelity.rs @@ -28,8 +28,8 @@ use crate::worker::container::{WorkerConfig, WorkerRuntime}; pub struct HostedCatalogHarness { /// The worker runtime with remote tools registered. pub runtime: WorkerRuntime, - /// Join handle for the background test server. - pub server: tokio::task::JoinHandle<()>, + /// Join handle for the background test server, yielding the serve outcome. + pub server: tokio::task::JoinHandle>, /// Captured proxied LLM tool-completion requests sent by the worker. pub captured_requests: Arc>>, } @@ -94,12 +94,14 @@ async fn capture_llm_complete_with_tools( /// captures proxied LLM tool-completion requests. /// /// Returns the base URL, a handle to the captured-requests buffer, and the -/// server join handle. +/// server join handle. Binding is fallible, so the tuple is returned as a +/// `Result`; the background task cannot propagate with `?`, so it yields the +/// serve outcome through its join handle instead of unwrapping. async fn spawn_hosted_catalog_server() -> Result< ( String, Arc>>, - tokio::task::JoinHandle<()>, + tokio::task::JoinHandle>, ), anyhow::Error, > { @@ -117,11 +119,7 @@ async fn spawn_hosted_catalog_server() -> Result< post(capture_llm_complete_with_tools), ) .with_state(state); - let server = tokio::spawn(async move { - axum::serve(listener, router) - .await - .expect("serve hosted fidelity test router") - }); + let server = tokio::spawn(async move { axum::serve(listener, router).await }); Ok((format!("http://{addr}"), captured_requests, server)) } diff --git a/src/worker/container/tests/pre_loop.rs b/src/worker/container/tests/pre_loop.rs index f6301ca83..429e89ae5 100644 --- a/src/worker/container/tests/pre_loop.rs +++ b/src/worker/container/tests/pre_loop.rs @@ -2,6 +2,7 @@ use std::sync::Arc; +use anyhow::Context as _; use axum::http::StatusCode; use rstest::rstest; use uuid::Uuid; @@ -104,26 +105,32 @@ async fn assert_startup_failure_completions(state: &RuntimeTestState) { assert_eq!(result_event["success"], false); } -async fn assert_startup_failure(state: &RuntimeTestState) { +/// Checks that `state` recorded exactly one sanitized terminal failure. +/// +/// Locating the terminal status update can fail, so the helper returns a +/// `Result` and leaves the verdict to the calling test body. +async fn assert_startup_failure(state: &RuntimeTestState) -> anyhow::Result<()> { let statuses = state.statuses.lock().await; - assert_eq!( - statuses.len(), - 1, + anyhow::ensure!( + statuses.len() == 1, "expected exactly one terminal status update, got {statuses:?}" ); let failed_status = statuses .first() .filter(|status| status.state == WorkerState::Failed) - .expect("expected a terminal failed status update"); - assert_eq!(failed_status.iteration, 0); - assert_eq!( - failed_status.message.as_deref(), - Some("pre-loop failure"), + .context("expected a terminal failed status update")?; + anyhow::ensure!( + failed_status.iteration == 0, + "expected a terminal status update for iteration 0, got {failed_status:?}" + ); + anyhow::ensure!( + failed_status.message.as_deref() == Some("pre-loop failure"), "expected a sanitized pre-loop failure message, got {failed_status:?}" ); drop(statuses); assert_startup_failure_completions(state).await; + Ok(()) } #[rstest] @@ -168,7 +175,7 @@ async fn worker_runtime_reports_failed_status_for_pre_loop_errors( "pre-loop failure should preserve the original error" ); - assert_startup_failure(&state).await; + assert_startup_failure(&state).await?; Ok(()) } diff --git a/src/worker/container/tests/remote_tools.rs b/src/worker/container/tests/remote_tools.rs index 8df7bcc57..0a00c0814 100644 --- a/src/worker/container/tests/remote_tools.rs +++ b/src/worker/container/tests/remote_tools.rs @@ -109,11 +109,12 @@ pub(super) struct TestState; /// Spawns an ephemeral Axum server that serves the remote-tool catalogue route. /// /// `H` is any Axum handler compatible with [`TestState`], and `T` is its -/// extractor tuple. Returns the `http://host:port` base URL and a join -/// handle for the background server task. +/// extractor tuple. Binding is fallible, so the `http://host:port` base URL is +/// returned as a `Result`. The background task cannot propagate with `?`, so +/// it yields the serve outcome through its join handle instead of unwrapping. pub(super) async fn spawn_test_server( handler: H, -) -> Result<(String, tokio::task::JoinHandle<()>), anyhow::Error> +) -> Result<(String, tokio::task::JoinHandle>), anyhow::Error> where H: axum::handler::Handler + Clone + Send + 'static, T: 'static, @@ -123,16 +124,12 @@ where let router = Router::new() .route(REMOTE_TOOL_CATALOG_ROUTE, get(handler)) .with_state(TestState); - let server = tokio::spawn(async move { - axum::serve(listener, router) - .await - .expect("serve router in test server") - }); + let server = tokio::spawn(async move { axum::serve(listener, router).await }); Ok((format!("http://{addr}"), server)) } async fn spawn_hosted_guidance_catalogue_server() --> Result<(String, tokio::task::JoinHandle<()>), anyhow::Error> { +-> Result<(String, tokio::task::JoinHandle>), anyhow::Error> { spawn_test_server(remote_tool_catalogue).await } diff --git a/src/worker/container/tests/shutdown.rs b/src/worker/container/tests/shutdown.rs index a33a7545a..8347d87a9 100644 --- a/src/worker/container/tests/shutdown.rs +++ b/src/worker/container/tests/shutdown.rs @@ -32,19 +32,20 @@ async fn event_handler( StatusCode::OK } +/// Spawns an ephemeral Axum server that records posted job events. +/// +/// Binding is fallible, so the base URL is returned as a `Result`. The spawned +/// task cannot propagate with `?`, so it yields the serve outcome through its +/// join handle instead of unwrapping. async fn spawn_event_server( state: Arc, -) -> Result<(String, tokio::task::JoinHandle<()>)> { +) -> Result<(String, tokio::task::JoinHandle>)> { let listener = TcpListener::bind("127.0.0.1:0").await?; let addr = listener.local_addr()?; let app = Router::new() .route(EVENT_ROUTE, post(event_handler)) .with_state(state); - let handle = tokio::spawn(async move { - axum::serve(listener, app) - .await - .expect("event test server should run"); - }); + let handle = tokio::spawn(async move { axum::serve(listener, app).await }); Ok((format!("http://{addr}"), handle)) } From 1bf74b4e32e9b86bf4b3e38cb1db736da0c7b7ca Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 8 Aug 2026 03:07:07 +0200 Subject: [PATCH 5/5] Withhold converter fixtures from the Markdown formatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `convert_test_pages_to_markdown` has been failing on the Yahoo page since d0efc5d1 adopted `mdformat-all` and normalised every tracked Markdown file. That pass reflowed `tests/test-pages/*/expected.md`, which are not prose: they are a byte-for-byte record of converter output, and the converter does not wrap. The test compares line by line, so a fixture wrapped to 80 columns no longer describes any real conversion. Restore the three fixtures to their pre-reformat content. The test passes against them unchanged, which confirms the converter itself never regressed — only the fixtures did. `.markdownlint-cli2.jsonc` already excludes these paths, with that exact rationale, but `mdformat-all` runs `mdtablefix --wrap` before `markdownlint-cli2` and `mdtablefix` has no exclusion flag. Since both tools discover files through `fd`, withhold the fixtures there instead. `.fdignore` rather than `.ignore`, so ripgrep still searches them. --- .fdignore | 17 ++ tests/test-pages/cnn/expected.md | 37 +-- tests/test-pages/medium/expected.md | 413 ++++++++++++++-------------- tests/test-pages/yahoo/expected.md | 72 +---- 4 files changed, 243 insertions(+), 296 deletions(-) create mode 100644 .fdignore diff --git a/.fdignore b/.fdignore new file mode 100644 index 000000000..84b37f05c --- /dev/null +++ b/.fdignore @@ -0,0 +1,17 @@ +# Paths hidden from `fd`, and therefore from the estate-wide `mdformat-all` +# formatter, which discovers Markdown with `fd` and pipes it through +# `mdtablefix --wrap`. +# +# `mdtablefix` has no exclusion flag, so a file it must not touch has to be +# withheld at discovery. `markdownlint-cli2` already skips the paths below via +# `.markdownlint-cli2.jsonc`; this file closes the other half of `make fmt`. +# +# This is deliberately `.fdignore` rather than `.ignore`: ripgrep does not read +# it, so these paths stay searchable. + +# Expected-output fixtures for the html_to_markdown converter tests. Their +# content is a byte-for-byte record of converter output, which does not wrap +# prose. Reflowing them to 80 columns makes +# `convert_test_pages_to_markdown` fail against a fixture that no longer +# describes any real conversion. +tests/test-pages/ diff --git a/tests/test-pages/cnn/expected.md b/tests/test-pages/cnn/expected.md index 89d75e965..19f320a52 100644 --- a/tests/test-pages/cnn/expected.md +++ b/tests/test-pages/cnn/expected.md @@ -1,33 +1,20 @@ ## The U.S. has long been heralded as a land of opportunity -- a place where anyone can succeed regardless of the economic class they were born into. -But a new report released on Monday by -[Stanford University's Center on Poverty and Inequality](http://web.stanford.edu/group/scspi-dev/cgi-bin/) -calls that into question. +But a new report released on Monday by [Stanford University's Center on Poverty and Inequality](http://web.stanford.edu/group/scspi-dev/cgi-bin/) calls that into question. -The report assessed poverty levels, income and wealth inequality, economic -mobility and unemployment levels among 10 wealthy countries with social welfare -programs. +The report assessed poverty levels, income and wealth inequality, economic mobility and unemployment levels among 10 wealthy countries with social welfare programs. -Among its key findings: the class you're born into matters much more in the -U.S. than many of the other countries. +Among its key findings: the class you're born into matters much more in the U.S. than many of the other countries. -As the -[report states](http://web.stanford.edu/group/scspi-dev/cgi-bin/publications/state-union-report): " -[T]he birth lottery matters more in the U.S. than in most well-off countries." +As the [report states](http://web.stanford.edu/group/scspi-dev/cgi-bin/publications/state-union-report): "[T]he birth lottery matters more in the U.S. than in most well-off countries." -But this wasn't the only finding that suggests the U.S. isn't quite living up -to its reputation as a country where everyone has an equal chance to get ahead -through sheer will and hard work. +But this wasn't the only finding that suggests the U.S. isn't quite living up to its reputation as a country where everyone has an equal chance to get ahead through sheer will and hard work. [Related: Rich are paying more in taxes but not as much as they used to](http://money.cnn.com/2016/01/11/news/economy/rich-taxes/index.html?iid=EL) -The report also suggested the U.S. might not be the "jobs machine" it thinks it -is, when compared to other countries. +The report also suggested the U.S. might not be the "jobs machine" it thinks it is, when compared to other countries. -It ranked near the bottom of the pack based on the levels of unemployment among -men and women of prime working age. The study determined this by taking the -ratio of employed men and women between the ages of 25 and 54 compared to the -total population of each country. +It ranked near the bottom of the pack based on the levels of unemployment among men and women of prime working age. The study determined this by taking the ratio of employed men and women between the ages of 25 and 54 compared to the total population of each country. The overall rankings of the countries were as follows: 1. Finland @@ -41,14 +28,10 @@ The overall rankings of the countries were as follows: 9. Spain 10. United States -The low ranking the U.S. received was due to its extreme levels of wealth and -income inequality and the ineffectiveness of its "safety net" -- social -programs aimed at reducing poverty. +The low ranking the U.S. received was due to its extreme levels of wealth and income inequality and the ineffectiveness of its "safety net" -- social programs aimed at reducing poverty. [Related: Chicago is America's most segregated city](http://money.cnn.com/2016/01/05/news/economy/chicago-segregated/index.html?iid=EL) -The report concluded that the American safety net was ineffective because it -provides only half the financial help people need. Additionally, the levels of -assistance in the U.S. are generally lower than in other countries. +The report concluded that the American safety net was ineffective because it provides only half the financial help people need. Additionally, the levels of assistance in the U.S. are generally lower than in other countries. - CNNMoney (New York) First published February 1, 2016: 1:28 AM ET + CNNMoney (New York) First published February 1, 2016: 1:28 AM ET \ No newline at end of file diff --git a/tests/test-pages/medium/expected.md b/tests/test-pages/medium/expected.md index 0b8ba986c..eaeff01b0 100644 --- a/tests/test-pages/medium/expected.md +++ b/tests/test-pages/medium/expected.md @@ -2,117 +2,116 @@ #### *Better Student Journalism* -We pushed out the first version of the -[Open Journalism site](http://pippinlee.github.io/open-journalism-project/) in -January. Our goal is for the site to be a place to teach students what they -should know about journalism on the web. It should be fun too. - -Topics like -[mapping](http://pippinlee.github.io/open-journalism-project/Mapping/), [security](http://pippinlee.github.io/open-journalism-project/Security/), -command line tools, and -[open source](http://pippinlee.github.io/open-journalism-project/Open-source/) -are all concepts that should be made more accessible, and should be easily -understood at a basic level by all journalists. We’re focusing on students -because we know student journalism well, and we believe that teaching maturing -journalists about the web will provide them with an important lens to view the -world with. This is how we got to where we are now. +We pushed out the first version of the [Open Journalism site](http://pippinlee.github.io/open-journalism-project/) in January. Our goal is for the + site to be a place to teach students what they should know about journalism + on the web. It should be fun too. + +Topics like [mapping](http://pippinlee.github.io/open-journalism-project/Mapping/), [security](http://pippinlee.github.io/open-journalism-project/Security/), command + line tools, and [open source](http://pippinlee.github.io/open-journalism-project/Open-source/) are + all concepts that should be made more accessible, and should be easily + understood at a basic level by all journalists. We’re focusing on students + because we know student journalism well, and we believe that teaching maturing + journalists about the web will provide them with an important lens to view + the world with. This is how we got to where we are now. ### Circa 2011 -In late 2011 I sat in the design room of our university’s student newsroom with -some of the other editors: Kate Hudson, Brent Rose, and Nicholas Maronese. I -was working as the photo editor then—something I loved doing. I was very happy -travelling and photographing people while listening to their stories. +In late 2011 I sat in the design room of our university’s student newsroom + with some of the other editors: Kate Hudson, Brent Rose, and Nicholas Maronese. + I was working as the photo editor then—something I loved doing. I was very + happy travelling and photographing people while listening to their stories. -Photography was my lucky way of experiencing the many types of people my -generation seemed to avoid, as well as many the public spends too much time -discussing. One of my habits as a photographer was scouring sites like Flickr -to see how others could frame the world in ways I hadn’t previously considered. +Photography was my lucky way of experiencing the many types of people + my generation seemed to avoid, as well as many the public spends too much + time discussing. One of my habits as a photographer was scouring sites + like Flickr to see how others could frame the world in ways I hadn’t previously + considered. topleftpixel.com -I started discovering beautiful things the -[web could do with images](http://wvs.topleftpixel.com/13/02/06/timelapse-strips-homewood.htm): -things not possible with print. Just as every generation revolts against -walking in the previous generations shoes, I found myself questioning the -expectations that I came up against as a photo editor. In our newsroom the -expectations were built from an outdated information world. We were expected to -fill old shoes. - -So we sat in our student newsroom—not very happy with what we were doing. Our -weekly newspaper had remained essentially unchanged for 40+ years. Each -editorial position had the same requirement every year. The *big* change -happened in the 80s when the paper started using colour. We’d also stumbled -into having a website, but it was updated just once a week with the release of -the newspaper. - -Information had changed form, but the student newsroom hadn’t, and it was -becoming harder to romanticize the dusty newsprint smell coming from the shoes -we were handed down from previous generations of editors. It was, we were told, -all part of “becoming a journalist.” +I started discovering beautiful things the [web could do with images](http://wvs.topleftpixel.com/13/02/06/timelapse-strips-homewood.htm): + things not possible with print. Just as every generation revolts against + walking in the previous generations shoes, I found myself questioning the + expectations that I came up against as a photo editor. In our newsroom + the expectations were built from an outdated information world. We were + expected to fill old shoes. + +So we sat in our student newsroom—not very happy with what we were doing. + Our weekly newspaper had remained essentially unchanged for 40+ years. + Each editorial position had the same requirement every year. The *big* change + happened in the 80s when the paper started using colour. We’d also stumbled + into having a website, but it was updated just once a week with the release + of the newspaper. + +Information had changed form, but the student newsroom hadn’t, and it + was becoming harder to romanticize the dusty newsprint smell coming from + the shoes we were handed down from previous generations of editors. It + was, we were told, all part of “becoming a journalist.” ### We don’t know what we don’t know -We spent much of the rest of the school year asking “what should we be doing in -the newsroom?”, which mainly led us to ask “how do we use the web to tell -stories?” It was a straightforward question that led to many more questions -about the web: something we knew little about. Out in the real world, -traditional journalists were struggling to keep their jobs in a dying print -world. They wore the same design of shoes that we were supposed to fill. Being -pushed to repeat old, failing strategies and blocked from trying something new -scared us. +We spent much of the rest of the school year asking “what should we be + doing in the newsroom?”, which mainly led us to ask “how do we use the + web to tell stories?” It was a straightforward question that led to many + more questions about the web: something we knew little about. Out in the + real world, traditional journalists were struggling to keep their jobs + in a dying print world. They wore the same design of shoes that we were + supposed to fill. Being pushed to repeat old, failing strategies and blocked + from trying something new scared us. We had questions, so we started doing some research. We talked with student -newsrooms in Canada and the United States, and filled too many Google Doc files -with notes. Looking at the notes now, they scream of fear. We annotated our -notes with naive solutions, often involving scrambled and immature odysseys -into the future of online journalism. + newsrooms in Canada and the United States, and filled too many Google Doc + files with notes. Looking at the notes now, they scream of fear. We annotated + our notes with naive solutions, often involving scrambled and immature + odysseys into the future of online journalism. There was a lot we didn’t know. We didn’t know **how to build a mobile app**. -We didn’t know **if we should build a mobile app**. We didn’t know **how to run -a server**. We didn’t know **where to go to find a server**. We didn’t know -**how the web worked**. We didn’t know **how people used the web to read -news**. We didn’t know **what news should be on the web**. If news is just -information, what does that even look like? - -We asked these questions to many students at other papers to get a consensus of -what had worked and what hadn’t. They reported similar questions and fears -about the web but followed with “print advertising is keeping us afloat so we -can’t abandon it”. - -In other words, we knew that we should be building a newer pair of shoes, but -we didn’t know what the function of the shoes should be. + We didn’t know **if we should build a mobile app**. + We didn’t know **how to run a server**. + We didn’t know **where to go to find a server**. + We didn’t know **how the web worked**. + We didn’t know **how people used the web to read news**. + We didn’t know **what news should be on the web**. + If news is just information, what does that even look like? + +We asked these questions to many students at other papers to get a consensus + of what had worked and what hadn’t. They reported similar questions and + fears about the web but followed with “print advertising is keeping us + afloat so we can’t abandon it”. + +In other words, we knew that we should be building a newer pair of shoes, + but we didn’t know what the function of the shoes should be. ### Common problems in student newsrooms (2011) -Our questioning of other student journalists in 15 student newsrooms brought up -a few repeating issues. +Our questioning of other student journalists in 15 student newsrooms brought + up a few repeating issues. - Lack of mentorship - A news process that lacked consideration of the web - No editor/position specific to the web -- Little exposure to many of the cool projects being put together by - professional newsrooms +- Little exposure to many of the cool projects being put together by professional + newsrooms - Lack of diverse skills within the newsroom. Writers made up 95% of the - personnel. Students with other skills were not sought because journalism was - seen as “a career with words.” The other 5% were designers, designing words on - computers, for print. + personnel. Students with other skills were not sought because journalism + was seen as “a career with words.” The other 5% were designers, designing + words on computers, for print. - Not enough discussion between the business side and web efforts From our 2011 research ### Common problems in student newsrooms (2013) -Two years later, we went back and looked at what had changed. We talked to a -dozen more newsrooms and weren’t surprised by our findings. +Two years later, we went back and looked at what had changed. We talked + to a dozen more newsrooms and weren’t surprised by our findings. - Still no mentorship or link to professional newsrooms building stories for the web - Very little control of website and technology -- The lack of exposure that student journalists have to interactive - storytelling. While some newsrooms are in touch with what’s happening with - the web and journalism, there still exists a huge gap between the student - newsroom and its professional counterpart +- The lack of exposure that student journalists have to interactive storytelling. + While some newsrooms are in touch with what’s happening with the web and + journalism, there still exists a huge gap between the student newsroom + and its professional counterpart - No time in the current news development cycle for student newsrooms to experiment with the web - Lack of skill diversity (specifically coding, interaction design, and @@ -124,189 +123,187 @@ dozen more newsrooms and weren’t surprised by our findings. often locked down -Newsrooms have traditionally been covered in copies of The New York Times or -Globe and Mail. Instead newsrooms should try spend at 20 minutes each week -going over the coolest/weirdest online storytelling in an effort to expose each -other to what is possible. -“[Hey, what has the New York Times R&D lab been up to this week?](http://nytlabs.com/)” +Newsrooms have traditionally been covered in copies of The New York Times + or Globe and Mail. Instead newsrooms should try spend at 20 minutes each + week going over the coolest/weirdest online storytelling in an effort to + expose each other to what is possible. “[Hey, what has the New York Times R&D lab been up to this week?](http://nytlabs.com/)” -Instead of having computers that are locked down, try setting aside a few -office computers that allow students to play and “break”, or encourage editors -to buy their own Macbooks so they’re always able to practice with code and new -tools on their own. +Instead of having computers that are locked down, try setting aside a + few office computers that allow students to play and “break”, or encourage + editors to buy their own Macbooks so they’re always able to practice with + code and new tools on their own. -From all this we realized that changing a student newsroom is difficult. It -takes patience. It requires that the business and editorial departments of the -student newsroom be on the same (web)page. The shoes of the future must be -different from the shoes we were given. +From all this we realized that changing a student newsroom is difficult. + It takes patience. It requires that the business and editorial departments + of the student newsroom be on the same (web)page. The shoes of the future + must be different from the shoes we were given. We need to rethink how long the new shoe design will be valid. It’s more -important that we focus on the process behind making footwear than on actually -creating a specific shoe. We shouldn’t be building a shoe to last 40 years. Our -footwear design process will allow us to change and adapt as technology -evolves. The media landscape will change, so having a newsroom that can change -with it will be critical. + important that we focus on the process behind making footwear than on actually + creating a specific shoe. We shouldn’t be building a shoe to last 40 years. + Our footwear design process will allow us to change and adapt as technology + evolves. The media landscape will change, so having a newsroom that can + change with it will be critical. **We are building a shoe machine, not a shoe.** ### A train or light at the end of the tunnel: are student newsrooms changing for the better? -In our 2013 research we found that almost 50% of student newsrooms had created -roles specifically for the web. **This sounds great, but is still problematic -in its current state.** +In our 2013 research we found that almost 50% of student newsrooms had + created roles specifically for the web. **This sounds great, but is still problematic in its current state.** -**We designed many of these slides to help explain to ourselves what we were -doing** +**We designed many of these slides to help explain to ourselves what we were doing** -When a newsroom decides to create a position for the web, it’s often with the -intent of having content flow steadily from writers onto the web. This is a big -improvement from just uploading stories to the web whenever there is a print -issue. *However…* +When a newsroom decides to create a position for the web, it’s often with + the intent of having content flow steadily from writers onto the web. This + is a big improvement from just uploading stories to the web whenever there + is a print issue. *However…* 1. **The handoff** -Problems arise because web editors are given roles that absolve the rest of the -editors from thinking about the web. All editors should be involved in the -process of story development for the web. While it’s a good idea to have one -specific editor manage the website, contributors and editors should all play -with and learn about the web. Instead of “can you make a computer do XYZ for -me?”, we should be saying “can you show me how to make a computer do XYZ?” +Problems arise because web editors are given roles that absolve the rest + of the editors from thinking about the web. All editors should be involved + in the process of story development for the web. While it’s a good idea + to have one specific editor manage the website, contributors and editors + should all play with and learn about the web. Instead of “can you make + a computer do XYZ for me?”, we should be saying “can you show me how to + make a computer do XYZ?” 2. **Not just social media** -A web editor could do much more than simply being in charge of the social media -accounts for the student paper. Their responsibility could include teaching all -other editors to be listening to what’s happening online. The web editor can -take advantage of live information to change how the student newsroom reports -news in real time. +A + web editor could do much more than simply being in charge of the social + media accounts for the student paper. Their responsibility could include + teaching all other editors to be listening to what’s happening online. + The web editor can take advantage of live information to change how the + student newsroom reports news in real time. 3. **Web (interactive) editor** -The goal of having a web editor should be for someone to build and tell stories -that take full advantage of the web as their medium. Too often the web’s -interactivity is not considered when developing the story. The web then ends up -as a resting place for print words. +The + goal of having a web editor should be for someone to build and tell stories + that take full advantage of the web as their medium. Too often the web’s + interactivity is not considered when developing the story. The web then + ends up as a resting place for print words. -Editors at newsrooms are still figuring out how to convince writers of the -benefit to having their content online. There’s still a stronger draw to -writers seeing their name in print than on the web. Showing writers that their -stories can be told in new ways to larger audiences is a convincing argument -that the web is a starting point for telling a story, not its graveyard. +Editors at newsrooms are still figuring out how to convince writers of + the benefit to having their content online. There’s still a stronger draw + to writers seeing their name in print than on the web. Showing writers + that their stories can be told in new ways to larger audiences is a convincing + argument that the web is a starting point for telling a story, not its + graveyard. -When everyone in the newsroom approaches their website with the intention of -using it to explore the web as a medium, they all start to ask “what is -possible?” and “what can be done?” You can’t expect students to think in terms -of the web if it’s treated as a place for print words to hang out on a web page. +When everyone in the newsroom approaches their website with the intention + of using it to explore the web as a medium, they all start to ask “what + is possible?” and “what can be done?” You can’t expect students to think + in terms of the web if it’s treated as a place for print words to hang + out on a web page. -We’re OK with this problem, if we see newsrooms continue to take small steps -towards having all their editors involved in the stories for the web. +We’re OK with this problem, if we see newsrooms continue to take small + steps towards having all their editors involved in the stories for the + web. -The current Open Journalism site was a few years in the making. This was an -original launch page we use in 2012 +The current Open Journalism site was a few years in the making. This was + an original launch page we use in 2012 ### What we know - **New process** -Our rough research has told us newsrooms need to be reorganized. This includes -every part of the newsroom’s workflow: from where a story and its information -comes from, to thinking of every word, pixel, and interaction the reader will -have with your stories. If I was a photo editor that wanted to re-think my -process with digital tools in mind, I’d start by asking “how are photo -assignments processed and sent out?”, “how do we receive images?”, “what -formats do images need to be exported in?”, “what type of screens will the -images be viewed on?”, and “how are the designers getting these images?” Making -a student newsroom digital isn’t about producing “digital manifestos”, it’s -about being curious enough that you’ll want to to continue experimenting with -your process until you’ve found one that fits your newsroom’s needs. +Our rough research has told us newsrooms need to be reorganized. This + includes every part of the newsroom’s workflow: from where a story and + its information comes from, to thinking of every word, pixel, and interaction + the reader will have with your stories. If I was a photo editor that wanted + to re-think my process with digital tools in mind, I’d start by asking + “how are photo assignments processed and sent out?”, “how do we receive + images?”, “what formats do images need to be exported in?”, “what type + of screens will the images be viewed on?”, and “how are the designers getting + these images?” Making a student newsroom digital isn’t about producing + “digital manifestos”, it’s about being curious enough that you’ll want + to to continue experimenting with your process until you’ve found one that + fits your newsroom’s needs. - **More (remote) mentorship** -Lack of mentorship is still a big problem. -[Google’s fellowship program](http://www.google.com/get/journalismfellowship/) -is great. The fact that it only caters to United States students isn’t. There -are only a handful of internships in Canada where students interested in -journalism can get experience writing code and building interactive stories. -We’re OK with this for now, as we expect internships and mentorship over the -next 5 years between professional newsrooms and student newsrooms will only -increase. It’s worth noting that some of that mentorship will likely be done -remotely. +Lack of mentorship is still a big problem. [Google’s fellowship program](http://www.google.com/get/journalismfellowship/) is great. The fact that it + only caters to United States students isn’t. There are only a handful of + internships in Canada where students interested in journalism can get experience + writing code and building interactive stories. We’re OK with this for now, + as we expect internships and mentorship over the next 5 years between professional + newsrooms and student newsrooms will only increase. It’s worth noting that + some of that mentorship will likely be done remotely. - **Changing a newsroom culture** -Skill diversity needs to change. We encourage every student newsroom we talk -to, to start building a partnership with their school’s Computer Science -department. It will take some work, but you’ll find there are many CS -undergrads that love playing with web technologies, and using data to tell -stories. Changing who is in the newsroom should be one of the first steps -newsrooms take to changing how they tell stories. The same goes with getting -designers who understand the wonderful interactive elements of the web and -students who love statistics and exploring data. Getting students who are -amazing at design, data, code, words, and images into one room is one of the -coolest experience I’ve had. Everyone benefits from a more diverse newsroom. +Skill diversity needs to change. We encourage every student newsroom we + talk to, to start building a partnership with their school’s Computer Science + department. It will take some work, but you’ll find there are many CS undergrads + that love playing with web technologies, and using data to tell stories. + Changing who is in the newsroom should be one of the first steps newsrooms + take to changing how they tell stories. The same goes with getting designers + who understand the wonderful interactive elements of the web and students + who love statistics and exploring data. Getting students who are amazing + at design, data, code, words, and images into one room is one of the coolest + experience I’ve had. Everyone benefits from a more diverse newsroom. ### What we don’t know - **Sharing curiosity for the web** -We don’t know how to best teach students about the web. It’s not efficient for -us to teach coding classes. We do go into newsrooms and get them running their -first code exercises, but if someone wants to learn to program, we can only -provide the initial push and curiosity. We will be trying out “labs” with a few -schools next school year to hopefully get a better idea of how to teach -students about the web. +We don’t know how to best teach students about the web. It’s not efficient + for us to teach coding classes. We do go into newsrooms and get them running + their first code exercises, but if someone wants to learn to program, we + can only provide the initial push and curiosity. We will be trying out + “labs” with a few schools next school year to hopefully get a better idea + of how to teach students about the web. - **Business** -We don’t know how to convince the business side of student papers that they -should invest in the web. At the very least we’re able to explain that having -students graduate with their current skill set is painful in the current job -market. +We don’t know how to convince the business side of student papers that + they should invest in the web. At the very least we’re able to explain + that having students graduate with their current skill set is painful in + the current job market. - **The future** -We don’t know what journalism or the web will be like in 10 years, but we can -start encouraging students to keep an open mind about the skills they’ll need. -We’re less interested in preparing students for the current newsroom climate, -than we are in teaching students to have the ability to learn new tools quickly -as they come and go. +We don’t know what journalism or the web will be like in 10 years, but + we can start encouraging students to keep an open mind about the skills + they’ll need. We’re less interested in preparing students for the current + newsroom climate, than we are in teaching students to have the ability + to learn new tools quickly as they come and go. Another slide from 2012 website ### What we’re trying to share with others - **A concise guide to building stories for the web** There are too many options to get started. We hope to provide an opinionated -guide that follows both our experiences, research, and observations from trying -to teach our peers. + guide that follows both our experiences, research, and observations from + trying to teach our peers. -Student newsrooms don’t have investors to please. Student newsrooms can change -their website every week if they want to try a new design or interaction. As -long as students start treating the web as a different medium, and start -building stories around that idea, then we’ll know we’re moving forward. +Student newsrooms don’t have investors to please. Student newsrooms can + change their website every week if they want to try a new design or interaction. + As long as students start treating the web as a different medium, and start + building stories around that idea, then we’ll know we’re moving forward. ### A note to professional news orgs -We’re also asking professional newsrooms to be more open about their process of -developing stories for the web. You play a big part in this. This means writing -about it, and sharing code. We need to start building a bridge between student -journalism and professional newsrooms. +We’re also asking professional newsrooms to be more open about their process + of developing stories for the web. You play a big part in this. This means + writing about it, and sharing code. We need to start building a bridge + between student journalism and professional newsrooms. 2012 ### This is a start -We going to continue slowly growing the content on -[Open Journalism](http://pippinlee.github.io/open-journalism-project/). We -still consider this the beta version, but expect to polish it, and beef up the -content for a real launch at the beginning of the summer. +We going to continue slowly growing the content on [Open Journalism](http://pippinlee.github.io/open-journalism-project/). We still consider this the beta version, + but expect to polish it, and beef up the content for a real launch at the + beginning of the summer. -We expect to have more original tutorials as well as the beginnings of what a -curriculum may look like that a student newsroom can adopt to start guiding -their transition to become a web first newsroom. We’re also going to be working -with the [Queen’s Journal](http://queensjournal.ca/) and -[The Ubyssey](http://ubyssey.ca/)next school year to better understand how to -make the student newsroom a place for experimenting with telling stories on the -web. If this sound like a good idea in your newsroom, we’re still looking to -add 1 more school. +We expect to have more original tutorials as well as the beginnings of + what a curriculum may look like that a student newsroom can adopt to start + guiding their transition to become a web first newsroom. We’re also going + to be working with the [Queen’s Journal](http://queensjournal.ca/) and [The Ubyssey](http://ubyssey.ca/)next school year to better understand how to make the student + newsroom a place for experimenting with telling stories on the web. If + this sound like a good idea in your newsroom, we’re still looking to add + 1 more school. -We’re trying out some new shoes. And while they’re not self-lacing, and smell a -bit different, we feel lacing up a new pair of kicks can change a lot. +We’re trying out some new shoes. And while they’re not self-lacing, and + smell a bit different, we feel lacing up a new pair of kicks can change + a lot. **Let’s talk. Let’s listen.** -**We’re still in the early stages of what this project will look like, so if -you want to help or have thoughts, let’s talk.** +**We’re still in the early stages of what this project will look like, so if you want to help or have thoughts, let’s talk.** [**pippin@pippinlee.com**](mailto:pippinblee@gmail.com) -*This isn't supposed to be a* ***manifesto™©*** *we just think it's pretty cool -to share what we've learned so far, and hope you'll do the same. We're all in -this together.* +*This isn't supposed to be a* ***manifesto™©*** *we just think it's pretty cool to share what we've learned so far, and hope you'll do the same. We're all in this together.* diff --git a/tests/test-pages/yahoo/expected.md b/tests/test-pages/yahoo/expected.md index 9cadfd02b..94ac42f33 100644 --- a/tests/test-pages/yahoo/expected.md +++ b/tests/test-pages/yahoo/expected.md @@ -1,87 +1,38 @@ -Virtual reality has officially reached the consoles. And it's pretty good! -[Sony's PlayStation VR](http://finance.yahoo.com/news/review-playstation-vr-is-comfortable-and-affordable-but-lacks-must-have-games-165053851.html) -is extremely comfortable and reasonably priced, and while it's lacking killer -apps, it's loaded with lots of interesting ones. +Virtual reality has officially reached the consoles. And it's pretty good! [Sony's PlayStation VR](http://finance.yahoo.com/news/review-playstation-vr-is-comfortable-and-affordable-but-lacks-must-have-games-165053851.html) is extremely comfortable and reasonably priced, and while it's lacking killer apps, it's loaded with lots of interesting ones. -But which ones should you buy? I've played just about every launch game, and -while some are worth your time, others you might want to skip. To help you -decide what's what, I've put together this list of the eight PSVR games worth -considering. +But which ones should you buy? I've played just about every launch game, and while some are worth your time, others you might want to skip. To help you decide what's what, I've put together this list of the eight PSVR games worth considering. ### ["Rez Infinite" ($30)](https://www.playstation.com/en-us/games/rez-infinite-ps4/) -Beloved cult hit "Rez" gets the VR treatment to help launch the PSVR, and the -results are terrific. It includes a fully remastered take on the original "Rez" -– you zoom through a Matrix-like computer system, shooting down enemies to the -steady beat of thumping electronica – but the VR setting makes it incredibly -immersive. It gets better the more you play it, too; unlock the amazing Area X -mode and you'll find yourself flying, shooting and bobbing your head to some of -the trippiest visuals yet seen in VR. +Beloved cult hit "Rez" gets the VR treatment to help launch the PSVR, and the results are terrific. It includes a fully remastered take on the original "Rez" – you zoom through a Matrix-like computer system, shooting down enemies to the steady beat of thumping electronica – but the VR setting makes it incredibly immersive. It gets better the more you play it, too; unlock the amazing Area X mode and you'll find yourself flying, shooting and bobbing your head to some of the trippiest visuals yet seen in VR. ### ["Thumper" ($20)](https://www.playstation.com/en-us/games/thumper-ps4/) -What would happen if Tron, the board game Simon, a Clown beetle, Cthulhu and a -noise band met in VR? Chaos, for sure, and also "Thumper." Called a "violent -rhythm game" by its creators, "Thumper" is, well, a violent rhythm game that's -also a gorgeous, unsettling and totally captivating assault on the senses. With -simple controls and a straightforward premise – click the X button and the -analog stick in time with the music as you barrel down a neon highway — it's -one of the rare games that works equally well both in and out of VR. But since -you have PSVR, play it there. It's marvelous. +What would happen if Tron, the board game Simon, a Clown beetle, Cthulhu and a noise band met in VR? Chaos, for sure, and also "Thumper." Called a "violent rhythm game" by its creators, "Thumper" is, well, a violent rhythm game that's also a gorgeous, unsettling and totally captivating assault on the senses. With simple controls and a straightforward premise – click the X button and the analog stick in time with the music as you barrel down a neon highway — it's one of the rare games that works equally well both in and out of VR. But since you have PSVR, play it there. It's marvelous. ### ["Until Dawn: Rush of Blood" ($20)](https://www.playstation.com/en-us/games/until-dawn-rush-of-blood-ps4/) -Cheeky horror game "Until Dawn" was a breakout hit for the PS4 last year, -channeling the classic "dumb teens in the woods" horror trope into an effective -interactive drama. Well, forget all that if you fire up "Rush of Blood," -because this one sticks you front and center on a rollercoaster ride from Hell. -Literally. You ride through a dimly-lit carnival of terror, dual-wielding -pistols as you take down targets, hideous pig monsters and, naturally, maniac -clowns. Be warned: If the bad guys don't get you, the jump scares will. +Cheeky horror game "Until Dawn" was a breakout hit for the PS4 last year, channeling the classic "dumb teens in the woods" horror trope into an effective interactive drama. Well, forget all that if you fire up "Rush of Blood," because this one sticks you front and center on a rollercoaster ride from Hell. Literally. You ride through a dimly-lit carnival of terror, dual-wielding pistols as you take down targets, hideous pig monsters and, naturally, maniac clowns. Be warned: If the bad guys don't get you, the jump scares will. ### ["Headmaster" ($20)](https://www.playstation.com/en-us/games/headmaster-ps4/) -Soccer meets "Portal" in the weird (and weirdly fun) "Headmaster," a game about -heading soccer balls into nets, targets and a variety of other things while -stuck in some diabolical training facility. While at first it seems a little -basic, increasingly challenging shots and a consistently entertaining narrative -keep it from running off the pitch. Funny, ridiculous and as easy as literally -moving your head back and forth, it's a pleasant PSVR surprise. +Soccer meets "Portal" in the weird (and weirdly fun) "Headmaster," a game about heading soccer balls into nets, targets and a variety of other things while stuck in some diabolical training facility. While at first it seems a little basic, increasingly challenging shots and a consistently entertaining narrative keep it from running off the pitch. Funny, ridiculous and as easy as literally moving your head back and forth, it's a pleasant PSVR surprise. ### ["RIGS: Mechanized Combat League" ($50)](https://www.playstation.com/en-us/games/rigs-mechanized-combat-league-ps4/) -Giant mechs + sports? That's the gist of this robotic blast-a-thon, which pits -two teams of three against one another in gorgeous, explosive and downright fun -VR combat. At its best, "RIGS" marries the thrill of fast-paced competitive -shooters with the insanity of piloting a giant mech in VR. It can, however, be -one of the barfier PSVR games. So pack your Dramamine, you're going to have to -ease yourself into this one. +Giant mechs + sports? That's the gist of this robotic blast-a-thon, which pits two teams of three against one another in gorgeous, explosive and downright fun VR combat. At its best, "RIGS" marries the thrill of fast-paced competitive shooters with the insanity of piloting a giant mech in VR. It can, however, be one of the barfier PSVR games. So pack your Dramamine, you're going to have to ease yourself into this one. ### ["Batman Arkham VR" ($20)](https://www.playstation.com/en-us/games/batman-arkham-vr-ps4/) -"I'm Batman," you will say. And you'll actually be right this time, because you -are Batman in this detective yarn, and you know this because you actually grab -the famous cowl and mask, stick it on your head, and stare into the mirrored -reflection of Rocksteady Games' impressive Dark Knight character model. It -lacks the action of its fellow "Arkham" games and runs disappointingly short, -but it's a high-quality experience that really shows off how powerfully -immersive VR can be. +"I'm Batman," you will say. And you'll actually be right this time, because you are Batman in this detective yarn, and you know this because you actually grab the famous cowl and mask, stick it on your head, and stare into the mirrored reflection of Rocksteady Games' impressive Dark Knight character model. It lacks the action of its fellow "Arkham" games and runs disappointingly short, but it's a high-quality experience that really shows off how powerfully immersive VR can be. ### ["Job Simulator" ($30)](https://www.playstation.com/en-us/games/job-simulator-the-2050-archives-ps4/) -There are a number of good VR ports in the PSVR launch lineup, but the HTC Vive -launch game "Job Simulator" might be the best. Your task? Lots of tasks, -actually, from cooking food to fixing cars to working in an office, all for -robots, because did I mention you were in the future? Infinitely charming and -surprisingly challenging, it's a great showpiece for VR. +There are a number of good VR ports in the PSVR launch lineup, but the HTC Vive launch game "Job Simulator" might be the best. Your task? Lots of tasks, actually, from cooking food to fixing cars to working in an office, all for robots, because did I mention you were in the future? Infinitely charming and surprisingly challenging, it's a great showpiece for VR. ### ["Eve Valkyrie" ($60)](https://www.playstation.com/en-us/games/eve-valkyrie-ps4/) -Already a hit on the Oculus Rift, this space dogfighting game was one of the -first to really show off how VR can turn a traditional game experience into -something special. It's pricey and not quite as hi-res as the Rift version, but -"Eve Valkyrie" does an admirable job filling the void left since "Battlestar -Galactica" ended. Too bad there aren't any Cylons in it (or are there?) +Already a hit on the Oculus Rift, this space dogfighting game was one of the first to really show off how VR can turn a traditional game experience into something special. It's pricey and not quite as hi-res as the Rift version, but "Eve Valkyrie" does an admirable job filling the void left since "Battlestar Galactica" ended. Too bad there aren't any Cylons in it (or are there?) ***More games news:*** @@ -92,5 +43,4 @@ Galactica" ended. Too bad there aren't any Cylons in it (or are there?) - [Review: 'Madden NFL 17' runs hard, plays it safe](https://www.yahoo.com/tech/review-madden-nfl-17-runs-000000394.html) -*Ben Silverman is on Twitter at* -[*ben_silverman*](https://twitter.com/ben_silverman)*.* +*Ben Silverman is on Twitter at* [*ben_silverman*](https://twitter.com/ben_silverman)*.* \ No newline at end of file