diff --git a/.github/workflows/integration-nightly.yml b/.github/workflows/integration-nightly.yml new file mode 100644 index 000000000..86e37e0bc --- /dev/null +++ b/.github/workflows/integration-nightly.yml @@ -0,0 +1,462 @@ +name: integration-nightly + +# Nightly live-instance integration sweep over open PRs targeting `dev` (#1896). +# +# ~93 of the test files in `tests/` root need a RUNNING Trinity: the fixtures +# create and delete real agents against `TRINITY_API_URL`. No CI job ran them, +# so they gated nothing — the sibling of #1895 (which relocates the files that +# can pass under `tests/unit/`). +# +# Membership is directory-based, not a file list: `--ignore=unit +# --ignore=process_engine`, exactly as `tests/run-core.sh:22` already defines +# it. So every file #1895 relocates leaves this job's scope automatically and +# this workflow needs no edit — which is also why this did not have to wait for +# #1895 to land, despite the issue's sequencing note. +# +# Architecture is inherited verbatim from backend-unit-nightly.yml, because the +# same rule applies (Codex finding 5 — untrusted PR code must never share a job +# with a write-capable token): +# +# discover (RO, lists open PRs) +# └─> test (RO, no creds, boots a stack, runs untrusted PR code) +# └─> comment (write, never checks out PR code) +# +# Artifacts (diff Markdown + status JSON) are how `test` hands work to +# `comment` without ever sharing a write token with the PR code. +# +# WHY NIGHTLY rather than per-PR (#1896 AC 2): the suite MUTATES its target — +# it creates and deletes real agents — and needs ~3-5 min of runtime on top of +# a ~2-3 min stack boot, twice (base and head). Putting that in the merge path +# would add ~10 min to every backend PR and park a mutating flake surface in +# front of every merge. Nightly buys the base-vs-head attribution the ACs +# require for free, because that tooling already exists for this exact shape. +# +# RUNTIME BUDGET (#1896 AC 5): `timeout-minutes: 45` per PR, expected ~15-20 +# (boot + suite, twice). If a PR routinely hits the ceiling the documented +# escalation is, in order: (1) drop to `-m "smoke"` for the head run, (2) shard +# the suite by marker across two jobs, (3) reduce `discover`'s PR limit. Do NOT +# raise the timeout without doing one of those first — a 45-minute mutating job +# that nobody reads is the failure mode this issue exists to fix. + +on: + schedule: + - cron: '30 6 * * *' # 06:30 UTC — after backend-unit-nightly (06:00) so + # the two nightlies do not contend for runners + workflow_dispatch: + inputs: + pr_number: + description: 'Run for a single PR number (blank = all open PRs)' + required: false + type: string + +# No top-level permissions — each job declares its own narrow scope. +permissions: {} + +concurrency: + group: integration-nightly + cancel-in-progress: false + +jobs: + discover: + name: discover open PRs + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + pull-requests: read + outputs: + matrix: ${{ steps.list.outputs.matrix }} + has_prs: ${{ steps.list.outputs.has_prs }} + steps: + - id: list + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + SINGLE_PR: ${{ inputs.pr_number }} + run: | + set -euo pipefail + if [ -n "${SINGLE_PR:-}" ]; then + prs=$(gh pr view "$SINGLE_PR" \ + --repo "${{ github.repository }}" \ + --json number,headRefName,headRefOid | jq '[.]') + else + # Lower limit than the unit nightly (50): each entry here boots two + # full stacks, so the fleet cost is an order of magnitude higher. + prs=$(gh pr list \ + --repo "${{ github.repository }}" \ + --base dev \ + --state open \ + --limit 20 \ + --json number,headRefName,headRefOid,isCrossRepository) + fi + # Defence in depth for the same exposure: fork PRs are excluded from + # the matrix, so PR code from outside the org never runs in a + # scheduled job that can see repository secrets or the runner's + # environment. Logged, not silent — a skipped fork must be visible as + # a coverage gap rather than look like a passing run. + forks=$(echo "$prs" | jq -r '[.[] | select(.isCrossRepository)] | map(.number|tostring) | join(", ")') + if [ -n "$forks" ]; then + echo "::warning::skipping fork PR(s) $forks — untrusted code is not run in the scheduled integration job" + fi + prs=$(echo "$prs" | jq '[.[] | select(.isCrossRepository | not)]') + count=$(echo "$prs" | jq 'length') + if [ "$count" = "0" ]; then + echo "has_prs=false" >> "$GITHUB_OUTPUT" + echo "matrix={\"include\":[]}" >> "$GITHUB_OUTPUT" + exit 0 + fi + echo "has_prs=true" >> "$GITHUB_OUTPUT" + matrix=$(echo "$prs" | jq -c '{include: [.[] | {pr_number: .number, head_sha: .headRefOid}]}') + echo "matrix=$matrix" >> "$GITHUB_OUTPUT" + + test: + name: integration (PR #${{ matrix.pr_number }}) + needs: discover + if: needs.discover.outputs.has_prs == 'true' + runs-on: ubuntu-latest + # See RUNTIME BUDGET above before changing this. + timeout-minutes: 45 + permissions: + contents: read # deliberately no write scope — this job runs PR code + strategy: + fail-fast: false + # Each entry boots two stacks; keep the fleet from saturating the runners. + max-parallel: 3 + matrix: ${{ fromJSON(needs.discover.outputs.matrix) }} + + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 # #1941: the merge below needs a common ancestor + persist-credentials: false + + - name: Stash trusted diff script (pre-merge) + # Workspace is unmodified `dev` at this point. Copy the diff utility to + # a stable path BEFORE the merge, so a malicious PR editing + # scripts/ci/diff-pytest-failures.py cannot make the gate green. + run: | + mkdir -p "$HOME/.ci-tools" + cp scripts/ci/diff-pytest-failures.py "$HOME/.ci-tools/diff-pytest-failures.py" + + - uses: actions/setup-python@v7 + with: + # Must equal the `FROM python:` pin in the image Dockerfiles (#1891) — + # a suite validated on an older minor cannot catch the stdlib-removal + # class of defect that has already shipped twice here. Guarded by + # tests/unit/test_1891_python_version_parity.py, which scans EVERY + # workflow precisely so a newly added one cannot drift in unnoticed. + # It caught this file at '3.11' before merge. + python-version: '3.13' + cache: 'pip' + cache-dependency-path: tests/requirements-test.txt + + - name: Install test dependencies + run: | + python -m pip install --upgrade pip + pip install -r tests/requirements-test.txt + # `test_cli_*.py` imports `trinity_cli`, which is deliberately NOT in + # requirements-test.txt (an `-e ./src/cli` line there breaks GitHub's + # dependency-graph updater — see tests/setup-env.sh). Install it the + # same on-demand way the local wrapper does, or those files fail to + # import and their tests silently never run. + pip install -e ./src/cli + + - name: Fetch PR head and attempt the merge + id: merge + run: | + set -euo pipefail + git config user.email "ci@trinity.local" + git config user.name "trinity-ci" + # #1941: no --depth. Both sides need enough history to reach the + # ancestor they share, or the merge fails as "unrelated histories" + # and every PR reads as conflicting. + git fetch origin "pull/${{ matrix.pr_number }}/head:pr-head" + git switch --detach origin/dev + git branch -f base-side HEAD + if git merge --no-edit --no-ff pr-head; then + echo "merge_conflict=false" >> "$GITHUB_OUTPUT" + git branch -f head-side HEAD + else + # #1941: only unmerged paths prove a real conflict; any other git + # failure is infrastructure and must not be reported as one. + if [ -n "$(git diff --name-only --diff-filter=U)" ]; then + echo "merge_conflict=true" >> "$GITHUB_OUTPUT" + git merge --abort || true + else + git merge --abort || true + echo "::error::git merge failed with no conflicted paths — infrastructure failure, not a PR conflict" + exit 1 + fi + fi + + - name: Generate CI admin password + if: steps.merge.outputs.merge_conflict == 'false' + # Mirrors frontend-e2e.yml: a per-run random value so the repo never + # publishes a default admin credential, even one only reachable from + # the runner sandbox. The `Aa1!` suffix guarantees the OWASP ASVS 2.1 + # complexity classes on the random branch. + run: | + set -euo pipefail + PASS="$(openssl rand -base64 24 | tr -d '/+=\n')Aa1!" + echo "::add-mask::$PASS" + echo "ADMIN_PASSWORD=$PASS" >> "$GITHUB_ENV" + + - name: Run the live suite on BOTH sides + if: steps.merge.outputs.merge_conflict == 'false' + env: + # NO `secrets.*` here, deliberately (#1896 review). `schedule` exposes + # repository secrets to fork PRs, and this step runs arbitrary shell + # from the merged PR tree (`boot_stack` runs the PR's own + # scripts/deploy/start.sh), so any secret named here is readable by + # anyone who opens a PR. `backend-unit-nightly.yml` — the workflow + # whose security architecture this one inherits — keeps its only + # secret in `discover`, and its test job is genuinely credential-free; + # this one is too, now. Tests needing a real key are skip-marked + # rather than paid for with a fleet-wide exposure. If a live-LLM + # nightly is ever wanted, it needs a different shape (a + # `pull_request_target`-free, same-repo-only workflow with an + # environment approval), not a secret added to this line. + ANTHROPIC_API_KEY: placeholder + # `tests/conftest.py` aliases TRINITY_TEST_PASSWORD from + # ADMIN_PASSWORD, so the credential does not need restating. + TRINITY_API_URL: http://localhost:8000 + # Safe here and nowhere else: the sweep refuses a non-localhost + # target, and this instance is a throwaway (#1558). + TRINITY_TEST_CLEANUP_SWEEP: '1' + run: | + set -uo pipefail + + boot_stack() { + cp .env.example .env + echo "ADMIN_PASSWORD=$ADMIN_PASSWORD" >> .env + echo "ANTHROPIC_API_KEY=$ANTHROPIC_API_KEY" >> .env + echo "SECRET_KEY=$(openssl rand -hex 32)" >> .env + # Zero user agents at baseline (learnings 2026-07-08) so the suite's + # own fixtures are the only agents on the instance. + echo "TRINITY_DEFAULT_SYSTEM_MANIFEST=disabled" >> .env + ./scripts/deploy/start.sh + for i in $(seq 1 90); do + if curl -fsS http://localhost:8000/health >/dev/null 2>&1; then + echo "backend healthy"; return 0 + fi + sleep 2 + done + echo "::error::backend never became healthy"; return 1 + } + + teardown_stack() { + # `-v` is the point: the suite mutates its instance, so the next + # side must not inherit agents, volumes or a dirty DB from the + # previous one. The runner VM is destroyed afterwards regardless, + # which is what makes the instance disposable (#1896 AC 4). + docker compose down -v --remove-orphans || true + } + + run_side() { + side="$1"; ref="$2" + git switch --detach "$ref" + if ! boot_stack; then + # Tear down whatever half-came-up, so the OTHER side still gets a + # clean instance instead of inheriting this one's wreckage. + teardown_stack + return 1 + fi + # Membership matches tests/run-core.sh:22 — directory-based, so + # #1895's relocations shrink this automatically. + # `-m "not slow"`: the 99 slow-marked tests are excluded to keep the + # budget above honest; they are a documented gap, not an oversight. + # `--continue-on-collection-errors` is load-bearing, not defensive + # tidiness: the root suite currently has 3 modules that fail to + # IMPORT (two git-sync files wanting `utils.credential_sanitizer`, + # and tests/integration's conftest). Without the flag pytest aborts + # the whole session with "Interrupted: N errors during collection" + # and writes NO JUnit XML at all — so this job would fail every + # night for every PR while reporting nothing useful. With it, the + # errors are recorded on BOTH sides, cancel out in the base-vs-head + # diff, and a NEW collection error introduced by a PR still shows up + # as a new failure. Those 3 are pre-existing and belong to the + # #1895/#1896 cleanup, not to whichever PR runs first. + ( cd tests && python -m pytest -m "not slow" \ + --ignore=unit --ignore=process_engine \ + --continue-on-collection-errors \ + --junit-xml="$GITHUB_WORKSPACE/junit-${side}-pr${{ matrix.pr_number }}.xml" \ + --tb=no -q ) || true + teardown_stack + } + + # Base first, then head, each against a PRISTINE stack. + run_side base "$(git rev-parse base-side)" + run_side head "$(git rev-parse head-side)" + + # #1941's lesson, applied before it can bite: the diff script is + # fail-closed on a missing input XML, so a stack that never booted + # would surface as "this PR introduced regressions" — an + # infrastructure failure wearing a PR's name. Fail the job loudly + # instead. + # + # This comment used to end "the `comment` step then posts nothing for + # this PR rather than something false", and that was not true when it + # was written — #2029. Exiting here skipped the verdict, the status + # step still ran under `if: always()`, the unset output stringified to + # a clean result, and the sticky said ✅. The guard in `Write status + # JSON` below is what finally makes the sentence accurate. + for side in base head; do + xml="$GITHUB_WORKSPACE/junit-${side}-pr${{ matrix.pr_number }}.xml" + if [ ! -s "$xml" ]; then + echo "::error::no JUnit XML for the $side side — the stack never ran the suite. Not reporting a regression." + exit 1 + fi + done + + - name: Run regression diff (trusted copy) + if: steps.merge.outputs.merge_conflict == 'false' + id: diff + run: | + set +e + python "$HOME/.ci-tools/diff-pytest-failures.py" \ + --base junit-base-pr${{ matrix.pr_number }}.xml \ + --head junit-head-pr${{ matrix.pr_number }}.xml \ + --out "diff-pr${{ matrix.pr_number }}.md" + rc=$? + if [ $rc -eq 0 ]; then + echo "regression=false" >> "$GITHUB_OUTPUT" + else + echo "regression=true" >> "$GITHUB_OUTPUT" + fi + + - name: Collect stack logs on failure + if: failure() + run: docker compose logs --no-color --tail=300 > trinity-logs-pr${{ matrix.pr_number }}.txt || true + + - name: Write status JSON + if: always() + run: | + merge_conflict='${{ steps.merge.outputs.merge_conflict }}' + regression='${{ steps.diff.outputs.regression }}' + + # #2029: absence of a verdict is its own state, not "clean". + # + # This step is `if: always()`, so it also runs when the job died + # BEFORE a verdict existed — a failed checkout, a failed PR-head + # fetch, a non-conflict merge failure, or the missing-JUnit guard + # above. `merge_conflict` is then '', `'' == "true"` is false, and the + # JSON says `{merge_conflict: false, regression: false}` — identical + # to a genuinely clean run, which the comment job renders as a green + # tick for a suite that never ran. + # + # Writing no file is the honest answer: the comment job enumerates + # `status-pr*.json`, so a missing one leaves that PR's sticky exactly + # as it was. + if [ -z "$merge_conflict" ]; then + echo "::warning::merge verdict unknown for PR ${{ matrix.pr_number }} — no status written, sticky left untouched" + exit 0 + fi + + # `regression` is legitimately unset when the merge conflicted (the + # diff never ran). Any other empty value means the diff step was + # skipped by a job failure — the same unknown verdict as above. + if [ -z "$regression" ]; then + if [ "$merge_conflict" = "true" ]; then + regression="false" + else + echo "::warning::regression verdict unknown for PR ${{ matrix.pr_number }} — no status written, sticky left untouched" + exit 0 + fi + fi + jq -n \ + --arg pr "${{ matrix.pr_number }}" \ + --arg head_sha "${{ matrix.head_sha }}" \ + --arg merge_conflict "$merge_conflict" \ + --arg regression "$regression" \ + '{pr_number: ($pr|tonumber), head_sha: $head_sha, + merge_conflict: ($merge_conflict == "true"), + regression: ($regression == "true")}' \ + > "status-pr${{ matrix.pr_number }}.json" + + - name: Upload artifacts + if: always() + uses: actions/upload-artifact@v7 + with: + name: integration-pr-${{ matrix.pr_number }} + path: | + diff-pr${{ matrix.pr_number }}.md + status-pr${{ matrix.pr_number }}.json + junit-*-pr${{ matrix.pr_number }}.xml + trinity-logs-pr${{ matrix.pr_number }}.txt + if-no-files-found: warn + retention-days: 14 + + comment: + name: post sticky comments + needs: [discover, test] + if: always() && needs.discover.outputs.has_prs == 'true' + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + issues: write # PR conversation comments are issue comments + pull-requests: write + steps: + - name: Download all integration artifacts + uses: actions/download-artifact@v8 + with: + pattern: integration-pr-* + merge-multiple: true + + - name: Post or update sticky comment per PR + uses: actions/github-script@v9 + env: + MARKER: '' + with: + script: | + const fs = require('fs'); + const marker = process.env.MARKER; + const paths = fs.readdirSync('.').filter(f => /^status-pr\d+\.json$/.test(f)); + if (paths.length === 0) { + core.info('No status JSONs found — nothing to comment on.'); + return; + } + + for (const p of paths) { + try { + const status = JSON.parse(fs.readFileSync(p, 'utf8')); + const pr = status.pr_number; + const diffPath = `diff-pr${pr}.md`; + + let body; + if (status.merge_conflict) { + body = `${marker}\n⚠️ **Live-instance suite skipped — merge conflict against \`dev\`.**\n\nResolve by merging \`dev\` locally and pushing the result; the next nightly re-tests.`; + } else if (status.regression) { + let diffMd = '_diff artifact missing_'; + if (fs.existsSync(diffPath)) { + diffMd = fs.readFileSync(diffPath, 'utf8'); + } + body = `${marker}\n⚠️ **Live-instance integration suite found regressions when this PR is merged into \`dev\`.**\n\n
\nRegression details (head_sha: \`${status.head_sha}\`)\n\n${diffMd}\n\n
\n\n_Reproduce locally against a running stack:_ \`tests/run-core.sh\``; + } else { + body = `${marker}\n✅ **Live-instance integration suite clean** when this PR is merged into \`dev\` (head_sha: \`${status.head_sha}\`).`; + } + + // Find the existing sticky comment by marker (paginated). The + // bot-author filter is required: without it a user comment + // quoting the marker matches, updateComment 403s, and no fresh + // sticky is ever created. + const existing = await github.paginate( + github.rest.issues.listComments, + { owner: context.repo.owner, repo: context.repo.repo, issue_number: pr, per_page: 100 } + ); + const mine = existing.find(c => + c.body && c.body.includes(marker) && c.user && c.user.type === 'Bot'); + + if (mine) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, repo: context.repo.repo, + comment_id: mine.id, body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, + issue_number: pr, body, + }); + } + } catch (e) { + core.warning(`Failed to comment for ${p}: ${e.message}`); + } + } diff --git a/tests/registry.json b/tests/registry.json index fc59a215e..240d5ed8d 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -33,6 +33,17 @@ ], "description": "A2A exposed-skills filter (ent#180): the open-core seam that narrows a card's skills[] to what an agent may advertise. OSS-unchanged by construction (no provider → identity, same list object); None = no opinion = advertise all (the unconfigured default, so an exposed agent's card is byte-identical across upgrade) vs [] = explicit advertise-nothing; stale/unknown stored ids are inert (the selection only subtracts, the template stays the source of truth); fail-open on provider error AND on a malformed return (a str would otherwise iterate to chars and silently empty the card — fail-closed, invisible); both card surfaces filter via one router helper (structural guard against a route rebuilding an unfiltered card). Disclosure control only — message/send dispatches free-form text, so this never narrows what a caller may ask for." }, + { + "file": "unit/test_1896_integration_nightly_workflow.py", + "feature": "#1896", + "added": "2026-08-03", + "categories": [ + "ci", + "unit", + "guard" + ], + "description": "Static guards on integration-nightly.yml (#1896), the CI home for the live-instance root suite (~93 files needing a running Trinity; nothing ran them, so they gated nothing). Pins the properties whose absence is invisible until the night it matters: membership is DIRECTORY-based (--ignore=unit, matching tests/run-core.sh) so #1895's relocations shrink it with no workflow edit and no file list to drift; base and head both run for attribution; a missing JUnit fails the job LOUDLY instead of being reported to the author as a regression (the diff script is fail-closed on missing XML, so a failed stack boot would otherwise wear a PR's name — #1941's class); each side gets a pristine stack via `down -v` since the suite mutates its instance; the merge is full-depth with the conflict verdict gated on unmerged paths (#1941 again); the job running untrusted PR code holds only contents:read while the write-scoped comment job never checks out PR code; it is not on the pull_request path; the runtime budget and its escalation are written down; --continue-on-collection-errors is present (measured: 3 root modules currently fail to import, and without the flag pytest writes no XML at all); and src/cli is installed, since test_cli_*.py imports trinity_cli which is deliberately absent from requirements-test.txt." + }, { "file": "unit/test_ent15_import_intents.py", "feature": "trinity-enterprise#15", diff --git a/tests/unit/test_1896_integration_nightly_workflow.py b/tests/unit/test_1896_integration_nightly_workflow.py new file mode 100644 index 000000000..57f23d40e --- /dev/null +++ b/tests/unit/test_1896_integration_nightly_workflow.py @@ -0,0 +1,280 @@ +"""#1896 — the live-instance suite has a CI home, and it stays honest. + +~93 of the `test_*.py` files in `tests/` root need a RUNNING Trinity (the +fixtures create and delete real agents against `TRINITY_API_URL`), and no CI +job ran them, so they gated nothing. `integration-nightly.yml` is that home. + +These are static guards on the workflow, in the shape of +`test_1941_nightly_merge_depth.py` — the properties below are ones whose +absence is invisible until the night it matters: + +* the suite membership is DIRECTORY-based, so #1895's relocations shrink it + automatically and nobody has to remember to edit a file list; +* the job that runs untrusted PR code holds no write token; +* an infrastructure failure cannot be reported to an author as "your PR + introduced regressions" (#1941's class, which this workflow could reproduce + through the diff script's fail-closed-on-missing-XML behaviour); +* the merge is not shallow (#1941 again — the same trap, a second workflow). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +yaml = pytest.importorskip("yaml") + +WORKFLOW = ( + Path(__file__).resolve().parents[2] + / ".github" + / "workflows" + / "integration-nightly.yml" +) + + +def _doc(): + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +def _test_job(): + return _doc()["jobs"]["test"] + + +def _runs(job) -> str: + return "\n".join( + str(s.get("run", "")) for s in job["steps"] if isinstance(s, dict) + ) + + +def _commands(text: str) -> str: + """Shell body minus comment lines — assert on what executes, not on prose.""" + return "\n".join( + line for line in text.splitlines() if not line.strip().startswith("#") + ) + + +def test_the_workflow_exists_and_parses(): + assert WORKFLOW.exists(), "the live suite lost its CI home again (#1896)" + assert _doc()["jobs"].keys() >= {"discover", "test", "comment"} + + +def test_membership_is_directory_based_not_a_file_list(): + """The AC that keeps this from rotting: `--ignore=unit`, exactly as + `tests/run-core.sh` defines the same suite. A hand-listed set would drift + the moment #1895 moves a file, and the drift is silent.""" + cmds = _commands(_runs(_test_job())) + assert "--ignore=unit" in cmds + assert "--ignore=process_engine" in cmds + assert "pytest" in cmds + # No per-file enumeration: a `test_*.py` literal in the invocation would + # mean the membership is hand-maintained again. + import re + + assert not re.search(r"test_\w+\.py", cmds), ( + "the suite is being enumerated file by file; keep it directory-based" + ) + + +def test_the_suite_runs_on_both_sides_for_attribution(): + """AC 3: base and head, so a pre-existing failure does not read as new.""" + cmds = _commands(_runs(_test_job())) + assert "run_side base" in cmds and "run_side head" in cmds + assert "diff-pytest-failures.py" in cmds + + +def test_missing_junit_fails_the_job_instead_of_blaming_the_pr(): + """`diff-pytest-failures.py` is fail-closed on a missing input XML, so a + stack that never booted would otherwise surface to the author as "this PR + introduced regressions" — an infrastructure failure wearing a PR's name, + which is exactly what #1941 was filed about.""" + cmds = _commands(_runs(_test_job())) + assert "-s \"$xml\"" in cmds or "! -s \"$xml\"" in cmds, ( + "the JUnit-existence check is gone; a failed boot would be reported as " + "a regression (#1896 / #1941)" + ) + assert "Not reporting a regression" in _runs(_test_job()) + + +def test_collection_errors_do_not_zero_out_the_run(): + """Measured, not defensive: the root suite currently has 3 modules that fail + to IMPORT. Without `--continue-on-collection-errors` pytest aborts the whole + session ("Interrupted: N errors during collection") and writes NO JUnit XML, + so this job would fail every night for every PR and report nothing. With it, + the errors land on both sides and cancel in the diff, while a NEW collection + error a PR introduces still surfaces.""" + cmds = _commands(_runs(_test_job())) + assert "--continue-on-collection-errors" in cmds + + +def test_the_cli_package_is_installed_for_test_cli_modules(): + """`test_cli_*.py` imports `trinity_cli`, which is deliberately kept out of + requirements-test.txt (an `-e ./src/cli` line there breaks GitHub's + dependency-graph updater — tests/setup-env.sh explains it). Without the + on-demand install those modules fail to import and their tests never run, + which is the same silent-no-coverage this issue exists to end.""" + cmds = _commands(_runs(_test_job())) + assert "src/cli" in cmds + + +def test_each_side_gets_a_pristine_stack(): + """AC 4: the suite MUTATES its instance, so the head side must not inherit + agents, volumes or a dirty DB from the base side.""" + cmds = _commands(_runs(_test_job())) + assert "docker compose down -v" in cmds, ( + "without `-v` the second side inherits the first side's volumes" + ) + + +def test_the_merge_is_not_shallow(): + """#1941, in a second workflow: two shallow fetches share no ancestor, and + every PR reads as conflicting.""" + job = _test_job() + checkouts = [ + s for s in job["steps"] + if isinstance(s, dict) and str(s.get("uses", "")).startswith("actions/checkout") + ] + assert checkouts, "the merge job no longer checks out a base" + for step in checkouts: + assert (step.get("with") or {}).get("fetch-depth") == 0 + + for line in _commands(_runs(job)).splitlines(): + if "git fetch" in line and "pull/" in line: + assert "--depth" not in line, f"shallow PR-head fetch: {line.strip()}" + + +def test_conflict_verdict_requires_unmerged_paths(): + """Same rule as #1941: a non-zero `git merge` is not proof of a conflict.""" + cmds = _commands(_runs(_test_job())) + assert "merge_conflict=true" in cmds + before = cmds.split("merge_conflict=true")[0] + assert "--diff-filter=U" in before + + +def test_the_job_running_pr_code_has_no_write_token(): + """The inherited security architecture: untrusted PR code must never share + a job with a write-capable token. `comment` holds the write scope and never + checks out PR code.""" + test_perms = _test_job().get("permissions") or {} + assert test_perms == {"contents": "read"}, ( + f"the PR-code job gained scope beyond contents:read: {test_perms}" + ) + + comment = _doc()["jobs"]["comment"] + steps = str(comment["steps"]) + assert "actions/checkout" not in steps, ( + "the write-scoped comment job must not check out PR code" + ) + + +def test_the_job_running_pr_code_holds_no_secrets(): + """Token SCOPE was guarded; secret REFERENCES were not. + + The original version of this test asserted only on `permissions:`, so + `ANTHROPIC_API_KEY: ${{ secrets.E2E_ANTHROPIC_API_KEY || 'placeholder' }}` + sat in the same job unremarked — and `schedule` (unlike `pull_request`) + exposes repository secrets to fork PRs, while this job runs arbitrary shell + from the merged PR tree. The guard read as if it covered that and did not. + """ + import re + + job = str(_test_job()) + referenced = set(re.findall(r"secrets\.([A-Za-z_][A-Za-z0-9_]*)", job)) + assert not referenced, ( + f"the job that runs untrusted PR code references secret(s): " + f"{sorted(referenced)} — a scheduled run hands those to any fork PR" + ) + + +def test_fork_prs_are_not_run(): + """Defence in depth for the same exposure, and visible when it bites.""" + discover = str(_doc()["jobs"]["discover"]) + assert "isCrossRepository" in discover, ( + "discover no longer filters fork PRs — untrusted code would run in a " + "scheduled job with access to the repository's environment" + ) + assert "select(.isCrossRepository | not)" in discover, ( + "the fork field is fetched but not filtered on" + ) + assert "::warning::skipping fork PR" in discover, ( + "a skipped fork must be logged — a silent skip looks like a pass" + ) + + +def test_it_is_not_wired_into_the_pull_request_merge_path(): + """AC 2: nightly, not per-PR. A mutating ~93-file suite in the merge path + is a cost and a flake source; `workflow_dispatch` keeps it runnable on + demand for a single PR.""" + triggers = _doc()[True] if True in _doc() else _doc()["on"] + assert "schedule" in triggers + assert "workflow_dispatch" in triggers + assert "pull_request" not in triggers + + +def test_runtime_is_bounded_and_the_escalation_is_written_down(): + """AC 5: a stated budget, and a documented answer for exceeding it — so the + next person raises the ceiling only after sharding, not instead of it.""" + job = _test_job() + assert isinstance(job.get("timeout-minutes"), int) + assert job["timeout-minutes"] <= 60, "an unbounded mutating nightly is the bug" + header = WORKFLOW.read_text(encoding="utf-8") + assert "RUNTIME BUDGET" in header + assert "shard" in header.lower() + + +def test_local_workflow_is_untouched(): + """AC 6: `tests/run-core.sh` remains the supported local path. This issue + adds a CI home; it does not migrate the local one.""" + run_core = WORKFLOW.parents[2] / "tests" / "run-core.sh" + assert run_core.exists() + body = run_core.read_text(encoding="utf-8") + assert "--ignore=unit" in body, ( + "run-core.sh no longer defines the same suite the nightly runs — the " + "two must agree or the local path stops reproducing CI (#1896)" + ) + + +# --------------------------------------------------------------------------- +# #2029 — an absent verdict must not render as "clean" +# --------------------------------------------------------------------------- + + +def _status_step(): + """The step that writes `status-pr*.json`, located by what it writes rather + than by name, so renaming it doesn't silently skip these assertions.""" + for _name, job in _doc()["jobs"].items(): + for step in job.get("steps") or []: + if isinstance(step, dict) and "status-pr" in str(step.get("run", "")): + return step + pytest.fail("no step writes status-pr*.json any more") + + +def test_an_unknown_verdict_writes_no_status_file(): + """This workflow's own missing-JUnit guard (`exit 1`) routes straight into + the #2029 defect: the job dies, `Write status JSON` still runs under + `if: always()`, the unset merge output stringifies to a clean verdict, and + the sticky comment says ✅ for a suite that never ran. + + The guard makes the earlier comment's claim — "posts nothing for this PR + rather than something false" — actually true. + """ + body = _commands(_status_step()["run"]) + assert 'if [ -z "$merge_conflict" ]' in body, ( + "the status step writes a verdict even when none was produced (#2029)" + ) + assert "exit 0" in body[body.index('if [ -z "$merge_conflict" ]'):].split("fi")[0] + + +def test_regression_defaults_to_false_only_on_a_real_conflict(): + body = _commands(_status_step()["run"]) + branch = body[body.index('if [ -z "$regression" ]'):] + assert '"$merge_conflict" = "true"' in branch[:400], ( + "regression falls back to false without checking the merge actually " + "conflicted, so a skipped diff reads as clean (#2029)" + ) + + +def test_the_status_step_still_runs_on_failure(): + """`if: always()` is what lets the step see the failed case at all — + removing it would suppress genuine conflict verdicts too.""" + assert _status_step().get("if") == "always()"