diff --git a/.github/workflows/guardian-reusable.yml b/.github/workflows/guardian-reusable.yml new file mode 100644 index 00000000..d215e6a4 --- /dev/null +++ b/.github/workflows/guardian-reusable.yml @@ -0,0 +1,280 @@ +name: Guardian Review (reusable) + +# Called by a repository's own guardian.yml on `/guardian review` (#739): +# +# jobs: +# review: +# if: github.event.issue.pull_request != null && github.event.comment.body == '/guardian review' +# uses: zaebee/codegraph-brain/.github/workflows/guardian-reusable.yml@ +# with: +# pr_number: ${{ github.event.issue.number }} +# tooling_ref: +# source_root: src +# secrets: +# ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} +# GUARDIAN_APP_PRIVATE_KEY: ${{ secrets.GUARDIAN_APP_PRIVATE_KEY }} +# permissions: {contents: read, pull-requests: write, issues: write} +# +# `secrets: inherit` reaches a called workflow only within one organisation or +# enterprise; any other caller passes the secrets declared below by name. +# Variables are always the caller's: GUARDIAN_PROVIDER, GUARDIAN_MODEL, +# GUARDIAN_ANTHROPIC_EFFORT, GUARDIAN_FEATURES, GUARDIAN_SKEPTIC, +# GUARDIAN_SKEPTIC_MODEL, GUARDIAN_APP_CLIENT_ID. +# +# The caller's Actions policy must allow this workflow and the actions it uses +# (actions/*, astral-sh/setup-uv, peter-evans/create-or-update-comment). +# Pin `uses:` and `tooling_ref` to the SAME commit SHA, never a tag: the +# tooling runs on the caller's runner, and a tag can move. + +on: + workflow_call: + inputs: + pr_number: + description: The pull request to review. + type: number + required: true + tooling_ref: + description: >- + The zaebee/codegraph-brain commit whose tooling runs. Pass the same ref + the caller's `uses:` names, so the job and its scripts agree. + type: string + required: true + source_root: + description: The directory inside the reviewed repository to build the graph from. + type: string + default: src + runner: + description: The job's runs-on, as JSON (a self-hosted label list works). + type: string + default: '["ubuntu-latest"]' + save_metrics: + description: >- + Restore guardian_metrics.jsonl from codegraph-brain's own data/guardian-metrics + branch. For codegraph-brain's caller only; any other caller leaves it false. + type: boolean + default: false + secrets: + ANTHROPIC_API_KEY: + required: false + GEMINI_API_KEY: + required: false + MISTRAL_API_KEY: + required: false + GUARDIAN_APP_PRIVATE_KEY: + required: false + +permissions: {} + +jobs: + review: + runs-on: ${{ fromJSON(inputs.runner) }} + # Below the app token's one-hour lifetime (#734): a review cannot outlive + # the token it posts with. + timeout-minutes: 45 + permissions: + contents: read + pull-requests: write + issues: write + + steps: + # Here rather than in each caller, so no caller can forget it. + - name: Check commenter has write access + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 + with: + script: | + const { data: perm } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: context.payload.comment.user.login, + }); + const allowed = ['admin', 'write']; + if (!allowed.includes(perm.permission)) { + core.setFailed(`@${context.payload.comment.user.login} needs write access to trigger Guardian Review (has: ${perm.permission})`); + } + + - name: React to trigger comment + env: + GH_TOKEN: ${{ github.token }} + COMMENT_ID: ${{ github.event.comment.id }} + run: | + gh api repos/$GITHUB_REPOSITORY/issues/comments/$COMMENT_ID/reactions \ + -X POST -f content=eyes + + - name: Validate inputs and get base branch + env: + EVENT_NAME: ${{ github.event_name }} + PR_NUMBER: ${{ inputs.pr_number }} + SOURCE_ROOT: ${{ inputs.source_root }} + GH_TOKEN: ${{ github.token }} + run: | + if [ "$EVENT_NAME" != issue_comment ]; then + echo "Call this on issue_comment (a /guardian review comment), not $EVENT_NAME" && exit 1 + fi + if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "Invalid PR number: $PR_NUMBER" && exit 1 + fi + # The same rule as guardian_review.py: `..` only as a path component, + # no absolute path, drive, backslash or whitespace. + case "$SOURCE_ROOT" in /*) SOURCE_ROOT_BAD=1 ;; esac + case "/$SOURCE_ROOT/" in + */../*|*\\*|*:*|*[[:space:]]*) SOURCE_ROOT_BAD=1 ;; + esac + case "${SOURCE_ROOT_BAD:-}" in + 1) + echo "source_root must be a plain path inside the repository: '$SOURCE_ROOT'" && exit 1 ;; + esac + echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV + BASE_BRANCH=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json baseRefName --jq '.baseRefName') + echo "BASE_BRANCH=$BASE_BRANCH" >> $GITHUB_ENV + + # The PR is data, never code (#730). Everything that runs — uv sync, the + # guardian scripts, cgis itself — comes from codegraph-brain at + # tooling_ref, checked out here; the PR lands in pr/ and is only diffed, + # read and parsed. This step and the next keep the job's provider keys + # out of reach of whoever wrote the PR. + - name: Checkout trusted tooling (codegraph-brain) + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + repository: zaebee/codegraph-brain + ref: ${{ inputs.tooling_ref }} + persist-credentials: false + + # tooling_ref must name the commit the caller's `uses:` does: the steps + # here and the scripts they run then agree. An older checkout fails here, + # before any spend, instead of on an unknown flag. + - name: Check the tooling matches this workflow + run: | + grep -q -- '"--source-root"' scripts/guardian_review.py || { + echo "tooling_ref predates this workflow: pin uses: and tooling_ref to the same SHA" && exit 1 + } + + - name: Checkout PR branch as data + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + repository: ${{ github.repository }} + # PR_NUMBER validated as digits-only in a previous step + ref: refs/pull/${{ env.PR_NUMBER }}/head + path: pr + fetch-depth: 0 + persist-credentials: false + + # The caller's GITHUB_TOKEN, passed for this one command through git's + # environment config (not argv, which other processes on a self-hosted + # runner can read) and never stored in pr/.git/config. Masked as + # actions/checkout masks it. A private repository needs it; a public one + # ignores it. + - name: Fetch base branch for diff + working-directory: pr + env: + FETCH_TOKEN: ${{ github.token }} + run: | + cred=$(printf 'x-access-token:%s' "$FETCH_TOKEN" | base64 | tr -d '\n') + echo "::add-mask::$cred" + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=http.https://github.com/.extraheader \ + GIT_CONFIG_VALUE_0="AUTHORIZATION: basic $cred" git fetch origin "$BASE_BRANCH" + + - name: Install uv + uses: astral-sh/setup-uv@caf0cab7a618c569241d31dcd442f54681755d39 # v3 + + - name: Install dependencies + run: uv sync --frozen --extra guardian + + # diff_check (#733) reads data keys, which the graph has only when + # ingested with --data-keys; without the feature the graph is as before. + - name: Build code graph + env: + GUARDIAN_FEATURES: ${{ vars.GUARDIAN_FEATURES }} + SOURCE_ROOT: ${{ inputs.source_root }} + run: | + flags="" + # Same reading as parse_features: comma-separated, any whitespace ignored. + case ",$(printf '%s' "$GUARDIAN_FEATURES" | tr -d '[:space:]')," in + *,diff_check,*) flags="--data-keys" ;; + esac + uv run --frozen cgis ingest "pr/$SOURCE_ROOT" --output graph.db $flags + + # Fetches anonymously: the tooling checkout keeps no credentials (#730), + # and the metrics branch is codegraph-brain's own. Off for other callers. + - name: Restore metrics from data branch + if: inputs.save_metrics + run: | + git fetch origin data/guardian-metrics 2>/dev/null && \ + git show origin/data/guardian-metrics:guardian_metrics.jsonl \ + > guardian_metrics.jsonl 2>/dev/null || true + + # Post as the codegraph-brain GitHub App when it is configured (#734): + # set vars.GUARDIAN_APP_CLIENT_ID and secrets.GUARDIAN_APP_PRIVATE_KEY. + # With the variable unset this step is skipped and posting uses + # GITHUB_TOKEN, as before. With it set, a missing key, a wrong key or an + # app not installed here fails the job before any LLM call: no silent + # fallback to another identity. The app's key reaches every repository + # the app is installed on, so the token is narrowed to this one and to + # pull-requests: write. Two steps read it, both of which post: "Run + # Guardian Review", which posts the inline review itself, and the fallback + # comment. Both run only this repository's tooling, never PR code. It + # lives one hour; the job's timeout-minutes keeps the review inside that. + - name: Mint GitHub App token + id: app-token + if: vars.GUARDIAN_APP_CLIENT_ID != '' + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.GUARDIAN_APP_CLIENT_ID }} + private-key: ${{ secrets.GUARDIAN_APP_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + repositories: ${{ github.event.repository.name }} + permission-pull-requests: write + + - name: Run Guardian Review + id: guardian + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} + MISTRAL_API_KEY: ${{ secrets.MISTRAL_API_KEY }} + GUARDIAN_PROVIDER: ${{ vars.GUARDIAN_PROVIDER }} + GUARDIAN_MODEL: ${{ vars.GUARDIAN_MODEL }} + # Thinking depth for an anthropic finder or skeptic; unset = the + # provider's default ("low"). #246 J measured the finder at "medium". + GUARDIAN_ANTHROPIC_EFFORT: ${{ vars.GUARDIAN_ANTHROPIC_EFFORT }} + GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + GUARDIAN_FEATURES: ${{ vars.GUARDIAN_FEATURES }} + GUARDIAN_SKEPTIC: ${{ vars.GUARDIAN_SKEPTIC }} + GUARDIAN_SKEPTIC_MODEL: ${{ vars.GUARDIAN_SKEPTIC_MODEL }} + SOURCE_ROOT: ${{ inputs.source_root }} + # GUARDIAN_EVIDENCE (#401) is deliberately not passed: the evidence + # pass runs `uv run mypy/ruff` inside the reviewed checkout, which is + # the PR, while this step holds the provider keys (#730). + # guardian_review.py refuses that combination with --repo-root. + run: | + uv run --frozen python scripts/guardian_review.py \ + --output guardian_report.md \ + --db graph.db \ + --pr "$PR_NUMBER" \ + --metrics guardian_metrics.jsonl \ + --record-finder guardian_finder.json \ + --inline \ + --repo-root pr \ + --source-root "$SOURCE_ROOT" \ + --base-branch "$BASE_BRANCH" + + - name: Upload metrics artifact + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: guardian-metrics-${{ env.PR_NUMBER }} + path: guardian_metrics.jsonl + retention-days: 7 + + - name: Upload finder recording artifact + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: guardian-finder-${{ env.PR_NUMBER }} + path: guardian_finder.json + retention-days: 7 + + - name: Post review as PR comment + if: steps.guardian.outputs.posted_inline != 'true' + uses: peter-evans/create-or-update-comment@a111a2c3bacd7be7898ee22d0d71d9aec2bb972c # v4 + with: + token: ${{ steps.app-token.outputs.token || github.token }} + issue-number: ${{ env.PR_NUMBER }} + body-path: guardian_report.md diff --git a/.github/workflows/guardian.yml b/.github/workflows/guardian.yml index 9478b7d7..c69cea86 100644 --- a/.github/workflows/guardian.yml +++ b/.github/workflows/guardian.yml @@ -7,182 +7,25 @@ on: permissions: {} jobs: + # The review itself is the reusable workflow (#739), the same one other + # repositories call; this repository calls it at the commit the event ran on. review: # job-level if: uses GitHub expression engine, not shell — safe for comment.body comparison if: | github.event.issue.pull_request != null && github.event.comment.body == '/guardian review' - runs-on: ubuntu-latest - # Below the app token's one-hour lifetime (#734): a review cannot outlive - # the token it posts with. - timeout-minutes: 45 + uses: ./.github/workflows/guardian-reusable.yml + with: + pr_number: ${{ github.event.issue.number }} + tooling_ref: ${{ github.sha }} + source_root: src + save_metrics: true + secrets: inherit permissions: contents: read pull-requests: write issues: write - steps: - - name: Check commenter has write access - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 - with: - script: | - const { data: perm } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner: context.repo.owner, - repo: context.repo.repo, - username: context.payload.comment.user.login, - }); - const allowed = ['admin', 'write']; - if (!allowed.includes(perm.permission)) { - core.setFailed(`@${context.payload.comment.user.login} needs write access to trigger Guardian Review (has: ${perm.permission})`); - } - - - name: React to trigger comment - env: - GH_TOKEN: ${{ github.token }} - COMMENT_ID: ${{ github.event.comment.id }} - run: | - gh api repos/$GITHUB_REPOSITORY/issues/comments/$COMMENT_ID/reactions \ - -X POST -f content=eyes - - - name: Validate PR number and get base branch - env: - PR_NUMBER: ${{ github.event.issue.number }} - GH_TOKEN: ${{ github.token }} - run: | - if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then - echo "Invalid PR number: $PR_NUMBER" && exit 1 - fi - echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV - BASE_BRANCH=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json baseRefName --jq '.baseRefName') - echo "BASE_BRANCH=$BASE_BRANCH" >> $GITHUB_ENV - - # The PR is data, never code (#730). Everything that runs — uv sync, the - # guardian scripts, cgis itself — comes from the default branch checked - # out here; the PR lands in pr/ and is only diffed, read and parsed. - # This step and the next keep the job's provider keys out of reach of - # whoever wrote the PR. - - name: Checkout trusted tooling (default branch) - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - with: - ref: ${{ github.event.repository.default_branch }} - persist-credentials: false - - - name: Checkout PR branch as data - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - with: - # PR_NUMBER validated as digits-only in previous step - ref: refs/pull/${{ env.PR_NUMBER }}/head - path: pr - fetch-depth: 0 - persist-credentials: false - - # Anonymous, like the checkout above it: fine for this public repository. - # A copy of this job for a private one must pass a token here, or the - # fetch fails the job. - - name: Fetch base branch for diff - working-directory: pr - run: git fetch origin "$BASE_BRANCH" - - - name: Install uv - uses: astral-sh/setup-uv@caf0cab7a618c569241d31dcd442f54681755d39 # v3 - - - name: Install dependencies - run: uv sync --frozen --extra guardian - - # diff_check (#733) reads data keys, which the graph has only when - # ingested with --data-keys; without the feature the graph is as before. - - name: Build code graph - env: - GUARDIAN_FEATURES: ${{ vars.GUARDIAN_FEATURES }} - run: | - flags="" - # Same reading as parse_features: comma-separated, any whitespace ignored. - case ",$(printf '%s' "$GUARDIAN_FEATURES" | tr -d '[:space:]')," in - *,diff_check,*) flags="--data-keys" ;; - esac - uv run --frozen cgis ingest pr/src --output graph.db $flags - - # Fetches anonymously: the root checkout keeps no credentials (#730). - # Fine while this repository is public; on a private one this restore - # fails into `|| true` and save-metrics would overwrite the history. - - name: Restore metrics from data branch - run: | - git fetch origin data/guardian-metrics 2>/dev/null && \ - git show origin/data/guardian-metrics:guardian_metrics.jsonl \ - > guardian_metrics.jsonl 2>/dev/null || true - - # Post as the codegraph-brain GitHub App when it is configured (#734): - # set vars.GUARDIAN_APP_CLIENT_ID and secrets.GUARDIAN_APP_PRIVATE_KEY. - # With the variable unset this step is skipped and posting uses - # GITHUB_TOKEN, as before. With it set, a missing key, a wrong key or an - # app not installed here fails the job before any LLM call: no silent - # fallback to another identity. The app's key reaches every repository - # the app is installed on, so the token is narrowed to this one and to - # pull-requests: write, and only the two posting steps read it. It lives - # one hour; the job's timeout-minutes keeps the review inside that. - - name: Mint GitHub App token - id: app-token - if: vars.GUARDIAN_APP_CLIENT_ID != '' - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - with: - client-id: ${{ vars.GUARDIAN_APP_CLIENT_ID }} - private-key: ${{ secrets.GUARDIAN_APP_PRIVATE_KEY }} - repositories: ${{ github.event.repository.name }} - permission-pull-requests: write - - - name: Run Guardian Review - id: guardian - env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} - MISTRAL_API_KEY: ${{ secrets.MISTRAL_API_KEY }} - GUARDIAN_PROVIDER: ${{ vars.GUARDIAN_PROVIDER }} - GUARDIAN_MODEL: ${{ vars.GUARDIAN_MODEL }} - # Thinking depth for an anthropic finder or skeptic; unset = the - # provider's default ("low"). #246 J measured the finder at "medium". - GUARDIAN_ANTHROPIC_EFFORT: ${{ vars.GUARDIAN_ANTHROPIC_EFFORT }} - GH_TOKEN: ${{ steps.app-token.outputs.token || github.token }} - GITHUB_REPOSITORY: ${{ github.repository }} - GUARDIAN_FEATURES: ${{ vars.GUARDIAN_FEATURES }} - GUARDIAN_SKEPTIC: ${{ vars.GUARDIAN_SKEPTIC }} - GUARDIAN_SKEPTIC_MODEL: ${{ vars.GUARDIAN_SKEPTIC_MODEL }} - # GUARDIAN_EVIDENCE (#401) is deliberately not passed: the evidence - # pass runs `uv run mypy/ruff` inside the reviewed checkout, which is - # the PR, while this step holds the provider keys (#730). - # guardian_review.py refuses that combination with --repo-root. - run: | - uv run --frozen python scripts/guardian_review.py \ - --output guardian_report.md \ - --db graph.db \ - --pr "$PR_NUMBER" \ - --metrics guardian_metrics.jsonl \ - --record-finder guardian_finder.json \ - --inline \ - --repo-root pr \ - --base-branch "$BASE_BRANCH" - - - name: Upload metrics artifact - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 - with: - name: guardian-metrics-${{ env.PR_NUMBER }} - path: guardian_metrics.jsonl - retention-days: 7 - - - name: Upload finder recording artifact - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 - with: - name: guardian-finder-${{ env.PR_NUMBER }} - path: guardian_finder.json - retention-days: 7 - - - name: Post review as PR comment - if: steps.guardian.outputs.posted_inline != 'true' - uses: peter-evans/create-or-update-comment@a111a2c3bacd7be7898ee22d0d71d9aec2bb972c # v4 - with: - token: ${{ steps.app-token.outputs.token || github.token }} - issue-number: ${{ env.PR_NUMBER }} - body-path: guardian_report.md - save-metrics: needs: review runs-on: ubuntu-latest diff --git a/scripts/guardian_review.py b/scripts/guardian_review.py index 450d9f06..c5cdecce 100644 --- a/scripts/guardian_review.py +++ b/scripts/guardian_review.py @@ -4,7 +4,7 @@ import asyncio import os from collections.abc import Mapping -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath import structlog @@ -56,6 +56,30 @@ def reviewed_root(requested: Path | None, project_root: Path, env: Mapping[str, return root +def relative_source_root(raw: str) -> str: + """`--source-root` as a plain relative path; `..` or an absolute path is refused. + + It arrives from a workflow input (#739). Reads are already confined to the + checkout (#731), so this is about failing loudly on a wrong value rather + than reviewing with no graph context. + """ + path = PurePosixPath(raw or ".") + # A backslash or a drive has no use in a repository path, and on another + # platform each hides an escape. Whitespace is refused rather than stripped: + # the workflow passes the raw value to the ingest, which does not strip it. + if ( + path.is_absolute() + or PureWindowsPath(raw).is_absolute() + or PureWindowsPath(raw).drive + or ".." in path.parts + or "\\" in raw + or any(c.isspace() for c in raw) + ): + _msg = f"--source-root must be a path inside the checkout: {raw!r}" + raise ValueError(_msg) + return "" if str(path) == "." else str(path) + + async def main() -> None: """Run Guardian review and write the result to stdout or a file.""" parser = argparse.ArgumentParser(description="Run CGIS Guardian AI code review.") @@ -103,6 +127,14 @@ async def main() -> None: "checkout here, so the PR is read and parsed but none of its code runs (#730)." ), ) + parser.add_argument( + "--source-root", + default="src", + help=( + "The ingest root inside --repo-root that graph.db was built from (default: src); " + "changed-file paths are stripped of it before graph lookups (#739)." + ), + ) parser.add_argument( "--inline", action="store_true", @@ -121,6 +153,7 @@ async def main() -> None: project_root = Path(__file__).parent.parent.absolute() repo_root = reviewed_root(args.repo_root, project_root, os.environ) + source_root = relative_source_root(args.source_root) provider, model = build_provider(os.environ) # Was an isinstance sniff that returned "gemini" for an Ollama provider, @@ -136,7 +169,11 @@ async def main() -> None: active = resolve_active_providers(provider.name, skeptic[0].name if skeptic else None) review_fingerprint = compute_fingerprint(disk_reader(project_root), active) collector = ContextCollector( - project_root=repo_root, db_path=args.db, base_branch=args.base_branch, features=features + project_root=repo_root, + db_path=args.db, + base_branch=args.base_branch, + source_root=source_root, + features=features, ) log.info("Running guardian review...", model=model) report, posted_inline = await run_guardian( diff --git a/src/cgis/guardian/collector.py b/src/cgis/guardian/collector.py index c3041f71..f8ee657b 100644 --- a/src/cgis/guardian/collector.py +++ b/src/cgis/guardian/collector.py @@ -245,7 +245,7 @@ def __init__(self, project_root: Path, db_path: Path | None, source_root: str = """Set the graph DB, the ingest root its FQNs are relative to, and the project root. source_root must match the ingest root used to build the graph DB (CI - runs `cgis ingest pr/src` with project_root `pr`): changed-file paths + runs `cgis ingest pr/` with project_root `pr`): changed-file paths are stripped of it before lookup. project_root locates docs/ontology/patterns.yaml for drift. """ diff --git a/tests/unit/test_guardian_review_script.py b/tests/unit/test_guardian_review_script.py index 6c39dff2..10e59d8d 100644 --- a/tests/unit/test_guardian_review_script.py +++ b/tests/unit/test_guardian_review_script.py @@ -136,3 +136,79 @@ def _fingerprint(reader: object, _active: object) -> str: await gr.main() assert seen["root"] == expected_root assert seen["fingerprint_reader"] == this_repository + + +@pytest.mark.asyncio +async def test_the_source_root_reaches_the_collector( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Another repository's sources live elsewhere (`backend/app`), and lookups follow.""" + seen: dict[str, object] = {} + + class _Provider: + name = "stub" + + def _collector(**kwargs: object) -> None: + seen.update(kwargs) + raise _CollectedError + + _argv(monkeypatch, "--repo-root", str(tmp_path), "--source-root", "backend/app") + monkeypatch.setenv("GUARDIAN_SKEPTIC", "off") + monkeypatch.setattr(gr, "build_provider", lambda _env: (_Provider(), "stub-model")) + monkeypatch.setattr(gr, "build_skeptic_provider", lambda _env, **_kw: None) + monkeypatch.setattr(gr, "disk_reader", lambda root: root) + monkeypatch.setattr(gr, "compute_fingerprint", lambda _r, _a: "fp") + monkeypatch.setattr(gr, "ContextCollector", _collector) + with pytest.raises(_CollectedError): + await gr.main() + assert seen["source_root"] == "backend/app" + + +@pytest.mark.parametrize("bad", ["../elsewhere", "..\\elsewhere", "C:\\absolute", "a/../.."]) +@pytest.mark.asyncio +async def test_a_source_root_that_climbs_out_is_refused( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, bad: str +) -> None: + """A workflow input: `..`, a backslash or an absolute path is refused before any spend.""" + _argv(monkeypatch, "--repo-root", str(tmp_path), "--source-root", bad) + with pytest.raises(ValueError, match="--source-root"): + await gr.main() + + +@pytest.mark.asyncio +async def test_an_absolute_source_root_is_refused( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """An absolute path would point the graph lookups outside the checkout.""" + _argv(monkeypatch, "--repo-root", str(tmp_path), "--source-root", "/etc") + with pytest.raises(ValueError, match="--source-root"): + await gr.main() + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("src", "src"), + ("src/", "src"), + ("./src", "src"), + (".", ""), + ("", ""), + ("my..pkg", "my..pkg"), + ], +) +def test_source_root_normalises_to_what_the_graph_strips(raw: str, expected: str) -> None: + """The value the collector strips from changed paths; `..` only counts as a component.""" + assert gr.relative_source_root(raw) == expected + + +@pytest.mark.parametrize("bad", [" src", "src ", "s rc", "C:/absolute"]) +def test_source_root_refuses_whitespace_and_drive_paths(bad: str) -> None: + """Whitespace is refused, not stripped: the workflow's ingest would not strip it.""" + with pytest.raises(ValueError, match="--source-root"): + gr.relative_source_root(bad) + + +def test_the_real_collector_takes_the_source_root(tmp_path: Path) -> None: + """The keyword the script passes is one ContextCollector accepts, not only a stub's.""" + collector = gr.ContextCollector(project_root=tmp_path, source_root="backend/app") + assert collector.graph.source_root == "backend/app" diff --git a/tests/unit/test_guardian_workflow.py b/tests/unit/test_guardian_workflow.py index c2cd4d92..6c3d75e6 100644 --- a/tests/unit/test_guardian_workflow.py +++ b/tests/unit/test_guardian_workflow.py @@ -16,13 +16,24 @@ import pytest import yaml -WORKFLOW = Path(__file__).parent.parent.parent / ".github" / "workflows" / "guardian.yml" +WORKFLOWS = Path(__file__).parent.parent.parent / ".github" / "workflows" +#: The review job itself, reusable by any repository (#739). +WORKFLOW = WORKFLOWS / "guardian-reusable.yml" +#: This repository's caller of it. +CALLER = WORKFLOWS / "guardian.yml" PR_DIR = "pr" #: The only command allowed to run inside the PR checkout. -PR_FETCH = 'git fetch origin "$BASE_BRANCH"' +#: The only commands allowed to run inside the PR checkout: build and mask the +#: credential, then one fixed fetch that reads it from git's environment config. +PR_FETCH = [ + "cred=$(printf 'x-access-token:%s' \"$FETCH_TOKEN\" | base64 | tr -d '\\n')", + 'echo "::add-mask::$cred"', + "GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=http.https://github.com/.extraheader \\", + 'GIT_CONFIG_VALUE_0="AUTHORIZATION: basic $cred" git fetch origin "$BASE_BRANCH"', +] #: The only two ways a command may name the PR checkout: parse it, review it. ALLOWED_MENTIONS = ( - re.compile(rf"cgis ingest {PR_DIR}/src(?=\s)"), + re.compile(rf'cgis ingest "{PR_DIR}/\$SOURCE_ROOT"(?=\s)'), re.compile(rf"--repo-root[ =]{PR_DIR}(?=\s)"), ) #: Ways a command could reach into the PR checkout, after the allowed mentions are removed. @@ -76,18 +87,24 @@ def test_the_pr_is_checked_out_into_its_own_directory(steps: list[dict[str, Any] assert _with(pr[0]).get("persist-credentials") is False -def test_the_root_checkout_is_the_default_branch(steps: list[dict[str, Any]]) -> None: +def test_the_root_checkout_is_codegraph_brain_at_the_tooling_ref( + steps: list[dict[str, Any]], +) -> None: """The tooling that runs must come from reviewed, merged code.""" root = [s for s in _checkouts(steps) if not _with(s).get("path")] assert len(root) == 1 - assert _with(root[0]).get("ref") == "${{ github.event.repository.default_branch }}" + assert _with(root[0]).get("repository") == "zaebee/codegraph-brain" + assert _with(root[0]).get("ref") == "${{ inputs.tooling_ref }}" def test_only_the_base_fetch_runs_inside_the_pr_checkout(steps: list[dict[str, Any]]) -> None: """Exactly one fixed git command runs in pr/; `git fetch … && uv sync` must not pass.""" inside = [s for s in steps if _in_pr(s.get("working-directory"))] - assert [str(s.get("run", "")).strip() for s in inside] == [PR_FETCH] - assert not any(_holds_secret(s) for s in inside) + assert [ + [line.strip() for line in str(s.get("run", "")).strip().splitlines()] for s in inside + ] == [PR_FETCH] + # The one credential there is the caller's GITHUB_TOKEN, for that fetch; no provider key. + assert [s.get("env") for s in inside] == [{"FETCH_TOKEN": "${{ github.token }}"}] def test_no_action_runs_from_or_inside_the_pr_checkout(steps: list[dict[str, Any]]) -> None: @@ -123,7 +140,8 @@ def test_the_graph_is_built_from_the_pr_sources(steps: list[dict[str, Any]]) -> ingest = [s for s in steps if "cgis ingest" in str(s.get("run", ""))] assert len(ingest) == 1 commands = [line for line in str(ingest[0]["run"]).splitlines() if "cgis ingest" in line] - assert [c.startswith(f"uv run --frozen cgis ingest {PR_DIR}/src ") for c in commands] == [True] + expected = f'uv run --frozen cgis ingest "{PR_DIR}/$SOURCE_ROOT" ' + assert [c.startswith(expected) for c in commands] == [True] assert "working-directory" not in ingest[0] @@ -148,6 +166,7 @@ def test_data_keys_follow_the_diff_check_feature( fake.chmod(0o755) env = {**os.environ, "PATH": f"{tmp_path}:{os.environ.get('PATH', '')}"} env["GUARDIAN_FEATURES"] = features + env["SOURCE_ROOT"] = "src" done = subprocess.run( ["bash", "-c", str(ingest["run"])], env=env, capture_output=True, text=True, check=True ) @@ -192,6 +211,7 @@ def test_the_app_token_reaches_this_repository_and_pull_requests_only( ) -> None: """Scope and permissions are named, not inherited from whatever the app was granted.""" granted = _with(_app_token_step(steps)) + assert granted["owner"] == "${{ github.repository_owner }}" assert granted["repositories"] == "${{ github.event.repository.name }}" permissions = {k: v for k, v in granted.items() if k.startswith("permission-")} assert permissions == {"permission-pull-requests": "write"} @@ -227,3 +247,94 @@ def test_the_app_token_is_minted_after_every_pr_step(steps: list[dict[str, Any]] def test_the_review_job_ends_before_the_app_token_expires(workflow: dict[Any, Any]) -> None: """An installation token lives 60 minutes; a longer job would post with an expired one.""" assert workflow["jobs"]["review"]["timeout-minutes"] < 60 + + +# The caller (#739): this repository's guardian.yml hands the review to the +# reusable workflow and keeps its own metrics jobs. + + +@pytest.fixture(scope="module") +def caller() -> dict[Any, Any]: + """The parsed guardian.yml.""" + result: dict[Any, Any] = yaml.safe_load(CALLER.read_text(encoding="utf-8")) + return result + + +def test_the_caller_hands_the_review_to_the_reusable_workflow(caller: dict[Any, Any]) -> None: + """At the commit the event ran on, with this repository's sources and its metrics.""" + review = caller["jobs"]["review"] + assert review["uses"] == "./.github/workflows/guardian-reusable.yml" + assert review["with"]["tooling_ref"] == "${{ github.sha }}" + assert review["with"]["source_root"] == "src" + assert review["with"]["save_metrics"] is True + assert review["secrets"] == "inherit" + assert " ".join(review["if"].split()) == ( + "github.event.issue.pull_request != null && github.event.comment.body == '/guardian review'" + ) + assert review["permissions"] == { + "contents": "read", + "pull-requests": "write", + "issues": "write", + } + + +def test_the_reusable_workflow_is_only_callable(workflow: dict[Any, Any]) -> None: + """No trigger of its own: a comment reaches it only through a caller's `if`.""" + triggers = workflow.get("on") or workflow.get(True) + assert set(triggers) == {"workflow_call"} + assert workflow["permissions"] == {} + + +def test_metrics_are_restored_only_when_asked(steps: list[dict[str, Any]]) -> None: + """The restore reads codegraph-brain's own branch; other callers never get it.""" + restore = next(s for s in steps if s.get("name") == "Restore metrics from data branch") + assert restore.get("if") == "inputs.save_metrics" + + +def test_cross_org_callers_can_pass_the_secrets(workflow: dict[Any, Any]) -> None: + """`secrets: inherit` stops at the organisation; the rest must be declared to be passable.""" + trigger = (workflow.get("on") or workflow.get(True))["workflow_call"] + declared = trigger["secrets"] + assert set(declared) == { + "ANTHROPIC_API_KEY", + "GEMINI_API_KEY", + "MISTRAL_API_KEY", + "GUARDIAN_APP_PRIVATE_KEY", + } + assert all(v == {"required": False} for v in declared.values()) + + +@pytest.mark.parametrize( + ("event", "source_root", "ok"), + [ + ("issue_comment", "src", True), + ("issue_comment", "backend/app", True), + ("issue_comment", "../elsewhere", False), + ("issue_comment", "/abs", False), + ("issue_comment", "..\\elsewhere", False), + ("issue_comment", "my..pkg", True), + ("issue_comment", "a/..", False), + ("issue_comment", " src", False), + ("issue_comment", "", True), + ("issue_comment", "./src", True), + ("pull_request", "src", False), + ], +) +def test_the_inputs_are_checked_by_the_steps_own_shell( + steps: list[dict[str, Any]], tmp_path: Path, event: str, source_root: str, ok: bool +) -> None: + """Run the validation step itself, with a stand-in `gh`, as the ingest test does.""" + validate = next(s for s in steps if s.get("name") == "Validate inputs and get base branch") + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\necho main\n") + fake.chmod(0o755) + out = tmp_path / "github_env" + env = {**os.environ, "PATH": f"{tmp_path}:{os.environ.get('PATH', '')}"} + env |= {"EVENT_NAME": event, "PR_NUMBER": "7", "SOURCE_ROOT": source_root} + env |= {"GITHUB_ENV": str(out), "GITHUB_REPOSITORY": "o/r"} + done = subprocess.run( + ["bash", "-c", str(validate["run"])], env=env, capture_output=True, text=True, check=False + ) + assert (done.returncode == 0) is ok + if ok: + assert "BASE_BRANCH=main" in out.read_text()