Repository navigation
fix(guardian): review the PR as data, never run its code beside the keys - #731
Conversation
`/guardian review` checked the PR out at the root and ran its lockfile, its scripts and (with GUARDIAN_EVIDENCE=1) its checkers while the provider keys were in the environment. The write-access check limits who triggers a review, not whose code runs, and the repository is public. - The job now checks out the default branch for all tooling and the PR into pr/, which is only diffed, read and parsed. - guardian_review.py gains --repo-root for the reviewed checkout; the review fingerprint is still computed from this repository. It refuses GUARDIAN_EVIDENCE=1 with a foreign checkout, and the workflow no longer passes the flag. - tests/unit/test_guardian_workflow.py pins the shape: PR in its own directory without credentials, default branch at the root, only git runs in pr/, and the one step holding secrets reviews pr/ without evidence. The same flag is what reviewing another repository (owner-api) needs. Closes #730 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --repo-root option to guardian_review.py to safely review foreign pull requests as data rather than executing their code, addressing security concerns regarding untrusted code execution alongside provider keys. It also adds comprehensive unit tests, including workflow validation tests in test_guardian_workflow.py. The review feedback suggests optimizing path resolution in reviewed_root by resolving project_root once, and improving the robustness of the workflow tests by using safe dictionary access instead of direct indexing to avoid potential KeyError or TypeError exceptions.
…ghter Review of #731 found that reading the PR as data was not yet enough: the collector followed symlinks. A PR replacing CONTRIBUTING.md (read on every review) with a link to /proc/self/environ would put the provider keys in the prompt, and a prompt injection could carry them into the public comment. - collector.contained(): read_file, collect_full_files and the drift patterns.yaml read only paths that resolve inside the checkout; links that stay inside still work, as with `cgis ingest`. - test_guardian_workflow.py now fails on each regression the review tried: `git fetch && uv sync` in pr/, `uv sync --directory pr`, `python pr/scripts/...`, a dropped working-directory, job-level GUARDIAN_EVIDENCE, and the PR back at the root. Gemini: safe `.get("with")`. - guardian_review.reviewed_root resolves this repository once (Gemini). - Comments that said production runs GUARDIAN_EVIDENCE=1 or that CI ingests ./src now say what is true since #730; the metrics restore notes that it fetches anonymously. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/guardian review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: Checked the path containment in contained() against symlinks, '..' and loops, and it holds. Checked the --repo-root gating and the test fixtures, and they are consistent with the code. Checked the workflow guard regexes for ways to reach into pr/ and found no bypass of the listed forms.
🤖 claude-haiku-5-5 · 21,472 prompt + 6,839 completion = 28,311 tokens · graph 2/9 files (22%)
…nymous fetch From guardian's own review of this PR (Haiku, first production run): - the drift path's refusal of a symlinked patterns.yaml had no test; it now has one, seen failing with the guard reverted; - the base-branch fetch in pr/ is anonymous, which a private-repository copy of the job must replace with a token; said next to the step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… collector Re-review of #731: - working-directory is compared as a path, so `./pr` or `pr/` counts as the PR checkout; no step may use a local action (`uses: ./...`) or point an action's working-directory at pr/; `--project ./pr` and `pushd pr` are caught too. The allowed mentions are anchored instead of space-suffixed, so `--repo-root=pr` reads as allowed. Each respelling was seen failing. - collect_full_files skips a changed path that is not a file, and both readers replace undecodable bytes: a PR could otherwise end a review with IsADirectoryError or UnicodeDecodeError. - read_file's docstring says it also refuses paths outside the checkout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/gemini review |
|
/guardian review |
There was a problem hiding this comment.
Summary: Checked the symlink containment in contained() and its use in read_file, collect_full_files and collect_drift: links that leave the checkout return None and reads stay inside, and the new tests cover those cases. Checked the --repo-root and GUARDIAN_EVIDENCE refusal logic in reviewed_root, which handles the foreign-checkout case correctly. Checked the workflow guard regexes in test_guardian_workflow.py against the new run lines and found no false pass or false failure.
🤖 claude-haiku-5-5 · 23,169 prompt + 5,818 completion = 28,987 tokens · graph 2/9 files (22%)
| - name: Checkout trusted tooling (default branch) | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| with: | ||
| ref: ${{ github.event.repository.default_branch }} |
There was a problem hiding this comment.
🟠 [Logic Bug] — Root checkout drops credentials that the metrics save step may still rely on
The root checkout now sets persist-credentials: false, so any later step in this job that pushes to data/guardian-metrics with git and relied on the persisted token would fail, and the metrics history would stop updating; the save step is not in this diff, so this cannot be verified here.
Fix: Confirm the save-metrics step passes an explicit token for its push, or restore persist-credentials on the step that pushes, and add a test asserting that push step's credentials.
There was a problem hiding this comment.
Same as the round-1 thread above: save-metrics is a separate job with its own checkout of main, default persist-credentials: true and contents: write. Only the review job's two checkouts drop credentials, and that job pushes nothing. No change.
| source_root must match the ingest root used to build the graph DB (CI | ||
| runs `cgis ingest ./src`): changed-file paths are stripped of it before | ||
| lookup. project_root locates docs/ontology/patterns.yaml for drift. | ||
| runs `cgis ingest pr/src` with project_root `pr`): changed-file paths |
There was a problem hiding this comment.
🟡 [Library Contract] — source_root docstring changed to pr/src but no code path in the diff updates the stripping prefix
Changed-file paths from git are repo-relative and are stripped of source_root before graph lookup; if the caller still passes the old ingest root, FQN lookups may miss under the new pr/src ingest layout.
Fix: Verify the caller that constructs GraphContextCollector passes the root that matches the pr/src ingest, and add an integration test that exercises the workflow's ingest root.
There was a problem hiding this comment.
Verified in the first independent review of this PR. cgis ingest pr/src run from the root and cgis ingest ./src run inside pr/ produce the same 3197 node ids. The collector, built with project_root=pr and the default source_root="src", strips src/ from repo-relative changed paths and found the graph for a changed file. The caller is guardian_review.py, which leaves source_root at its default. Only the docstring changed. No change.
| from typing import Any | ||
|
|
||
| import pytest | ||
| import yaml |
There was a problem hiding this comment.
🟡 [Test Coverage] — Test depends on PyYAML, which the diff does not add to dependencies
If PyYAML is not in the project's dev or guardian extras, the new workflow test fails at import in CI.
Fix: Confirm pyyaml is declared in the dev dependency group, or parse the workflow without it.
There was a problem hiding this comment.
PyYAML is a main dependency (pyyaml>=6.0.2, pyproject.toml:48), and CI ran these tests green. No change.
There was a problem hiding this comment.
Code Review
This pull request introduces security measures to treat pull requests as untrusted data rather than code, preventing the execution of untrusted scripts or checkers while provider keys are present. It adds a --repo-root option to specify the checkout directory, restricts evidence collection on foreign checkouts, and implements a path containment check (contained) to prevent symlink directory traversal attacks. Feedback on these changes suggests returning the resolved canonical path in the containment check and catching ValueError to handle invalid path characters robustly.
| try: | ||
| if path.resolve().is_relative_to(root.resolve()): | ||
| return path | ||
| except (OSError, RuntimeError): # a loop of links | ||
| return None |
There was a problem hiding this comment.
To improve robustness and prevent potential issues with unresolved relative paths or symlinks in downstream components (such as DriftScorer), consider resolving the path once and returning the resolved canonical path. Additionally, catching ValueError is recommended to handle cases where path operations might fail due to invalid characters (e.g., embedded null bytes on certain platforms).
| try: | |
| if path.resolve().is_relative_to(root.resolve()): | |
| return path | |
| except (OSError, RuntimeError): # a loop of links | |
| return None | |
| try: | |
| resolved = path.resolve() | |
| if resolved.is_relative_to(root.resolve()): | |
| return resolved | |
| except (OSError, RuntimeError, ValueError): # a loop of links or invalid path | |
| return None |
There was a problem hiding this comment.
Applied in 0a2bddf. contained() returns the resolved path it checked, and also catches ValueError. Tests: test_contained_returns_the_resolved_path and test_read_file_refuses_a_path_with_a_null_byte. Both were seen failing first.
… null byte From Gemini's second review of #731. The caller now reads the resolved path that passed the check rather than a spelling that resolves again, and a path with a null byte (ValueError from resolve) reads as absent instead of crashing the review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
* ci(guardian): a reusable review workflow other repositories call The review job moves to .github/workflows/guardian-reusable.yml (`on: workflow_call`). guardian.yml becomes its first caller, at the commit the event ran on; its save-metrics and rate jobs are unchanged. The workflow takes these inputs: pr_number, tooling_ref (the codegraph-brain commit whose tooling runs), source_root, runner (JSON runs-on) and save_metrics. Secrets and vars are the caller's, so an organisation can set them once. The shape from #731 and #734 is kept: - tooling is codegraph-brain at tooling_ref, and the PR is data in pr/; - only one fixed git fetch runs there, now authenticated with the caller's GITHUB_TOKEN for this one command, so private repositories work; - the app token is scoped to the caller's repository and to pull-requests: write. The write-access check lives in the reusable job, so no caller can drop it. guardian_review.py gains --source-root, which refuses an absolute path or `..`. The workflow tests now pin the reusable file and the caller. Closes #739 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(guardian): let a caller in another org pass the secrets, and tighten the job Review of #740: - `secrets: inherit` stops at the organisation, so an Ownima caller of this user-owned workflow got no keys and could not pass any. The four secrets are now declared under workflow_call (optional), and the header shows the explicit form, the Actions policy the caller needs, and the same-SHA pin for uses: and tooling_ref. - A step fails fast when tooling_ref predates the workflow (no --source-root in the script), instead of on an unknown flag. - The base fetch takes the token from git's environment config, not argv, scoped to github.com and masked as actions/checkout masks it. - The job refuses an event other than issue_comment with a plain message. - save_metrics says it reads codegraph-brain's own branch. - Tests: the pr/ step's commands and env are pinned exactly (the old no-secret check no longer looked at github.token); the validation step's own shell runs for five cases; the restore's `if`, the declared secrets, the app token's owner, the caller's whole `if` and an absolute --source-root are covered. Each new pin was seen failing on its mutation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(guardian): refuse a backslash in source_root From Gemini's review of #740: a backslash has no use in a repository path and would hide a `..` on a platform that treats it as a separator. Refused in guardian_review.py and in the workflow's own check, with tests for `..\\x`, `C:\\x` and `a/../..`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(guardian): one source_root rule in shell and Python, portable base64 From the second review round of #740 (guardian and Gemini): - The workflow's shell check and relative_source_root now apply the same rule. `..` counts only as a path component (`my..pkg` is fine); an absolute path, a drive (C:/x), a backslash or any whitespace is refused. Whitespace is refused, not stripped, because the ingest gets the raw value. An empty value (the whole repository) is accepted by both. - The credential is encoded with `base64 | tr -d '\n'` rather than the GNU-only `-w0`, since the runner input allows self-hosted machines. - Tests: normalisation of '', '.', './src', 'src/'; refusals of whitespace and drives; the real ContextCollector built with source_root; the shell check for my..pkg, a/.., ' src', '' and ./src. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(guardian): name the two steps that read the app token From guardian's third review of #740: "only the two posting steps" read as excluding the review step, which is the one that posts the inline review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #730.
Problem
/guardian reviewchecked the PR out at the repository root. With that checkout, the job ran the PR's own lockfile (uv sync), the PR'sscripts/guardian_review.pyand, withGUARDIAN_EVIDENCE=1,uv run mypy/ruffunder the PR's config. All of this happened whileANTHROPIC_API_KEY,MISTRAL_API_KEYandGEMINI_API_KEYwere in the environment.The write-access check limits who triggers a review, not whose code runs. The repository is public.
Change
pr/withpersist-credentials: falseand is only diffed, read and parsed (cgis ingest pr/src);pr/isgit fetch;GUARDIAN_EVIDENCEis no longer passed.guardian_review.py --repo-root:GUARDIAN_EVIDENCE=1together with a foreign checkout;tests/unit/test_guardian_workflow.pypins the workflow shape; 4 of its 5 tests fail on the old workflow;test_guardian_review_script.py; the collector-root case was seen failing with the wiring reverted.evidence.pydocstring no longer claims thatguardian.ymlruns the evidence pass.The same flag is what reviewing another repository (owner-api) needs.
Behaviour change
The
GUARDIAN_EVIDENCE=1repository variable is now ignored by this workflow. The evidence pass only fed the skeptic, which is off in production.A second, intended consequence. A PR that changes guardian itself (its prompt, finder, collector or scripts) is now reviewed by the guardian on
main, not by its own new code. Its behaviour can be checked locally beforehand, the way the owner-api trial was run, or on the next PR after merge. The workflow YAML was already taken from the default branch forissue_commentevents, so only the scripts change here, and they are exactly the path the keys leaked through.🤖 Generated with Claude Code