Ai reviewer#2668
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Findings (status as of supersession)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if [[ ! -s "/tmp/ai-review-trusted/$f" \ | ||
| && -s ".github/ai-review/$f" ]]; then | ||
| echo "::warning::Base branch missing $f; using PR-side copy (bootstrap mode)." | ||
| cp ".github/ai-review/$f" "/tmp/ai-review-trusted/$f" |
There was a problem hiding this comment.
[CRITICAL] Bootstrap fallback executes PR-controlled helpers with review tokens
When the base branch does not yet contain these helper files, this fallback copies prefetch.sh, post_review.py, and the schema/persona files from the PR worktree into /tmp/ai-review-trusted. That is exactly the bootstrap state for this PR, and the copied prefetch.sh is then run with GH_TOKEN at line 218; the same pattern is repeated in the Auditor job, where the token has contents: write. A malicious change to the PR-side helper could run arbitrary shell before the Skeptic has reviewed anything, tamper with the pre-fetched context, post misleading reviews, or in the Auditor path use the write-capable token. Bootstrap should fail closed or use helper code supplied from a trusted source outside the PR worktree.
| @@ -0,0 +1,39 @@ | |||
| # Subtensor AI Review — Shared Context | |||
There was a problem hiding this comment.
[HIGH] Reviewer policy is PR-controlled in this bootstrap change
This PR adds .github/ai-review/* relative to the trusted base copies. Per the bootstrap rule, these files are untrusted prompt/policy material: they define future reviewer behavior and must not be treated as self-approved by the automated reviewer. Land trusted policy/helper files through a manually reviewed bootstrap path, then require future PRs to diff against and execute only the base-branch copies.
Findings (status as of supersession)
|
| git commit -m "chore: auditor auto-fix" | ||
| git -c "http.https://github.com/.extraheader=AUTHORIZATION: bearer $PUSH_TOKEN" \ | ||
| push origin "HEAD:$HEAD_REF" |
There was a problem hiding this comment.
[HIGH] Auditor auto-fix push runs git hooks with the write token
The auditor step intentionally lets Codex and any build/test commands operate on the PR worktree before this later token-bearing step runs. A malicious same-repo PR, prompt injection, or build script can plant .git/hooks/* or set core.hooksPath; then git commit / git push execute those hooks while PUSH_TOKEN is in the environment, giving the PR-controlled code a fresh write token after the supposedly credential-free phase. Disable hooks for the commit and push and push to an explicit repository URL so PR-side git config cannot affect the token-bearing operation.
| git commit -m "chore: auditor auto-fix" | |
| git -c "http.https://github.com/.extraheader=AUTHORIZATION: bearer $PUSH_TOKEN" \ | |
| push origin "HEAD:$HEAD_REF" | |
| git -c core.hooksPath=/dev/null commit -m "chore: auditor auto-fix" | |
| git -c core.hooksPath=/dev/null \ | |
| -c "http.https://github.com/.extraheader=AUTHORIZATION: bearer $PUSH_TOKEN" \ | |
| push "https://github.com/$REPO.git" "HEAD:$HEAD_REF" |
| if [[ ! -s "/tmp/ai-review-trusted/$f" \ | ||
| && -s ".github/ai-review/$f" ]]; then | ||
| echo "::warning::Base branch missing $f; using PR-side copy (bootstrap mode)." | ||
| cp ".github/ai-review/$f" "/tmp/ai-review-trusted/$f" |
There was a problem hiding this comment.
[HIGH] [BOOTSTRAP] Bootstrap fallback executes PR-controlled helpers with review tokens
When the base branch lacks .github/ai-review/*, this copies prefetch.sh and post_review.py from the PR worktree into /tmp/ai-review-trusted; the following prefetch/post steps run those files with the review token. This unsafe path is structurally unreachable after the first merge because the base branch will then have trusted copies, and the mitigation for this PR is the one nucleus-approved CI run plus heightened human scrutiny of these .github/ changes. A future PR reintroducing a PR-side helper fallback would be a strong red flag. The same fallback appears in the auditor job at line 399 and needs the same treatment.
| @@ -0,0 +1,49 @@ | |||
| # Subtensor AI Review — Shared Context | |||
There was a problem hiding this comment.
[HIGH] [BOOTSTRAP] Reviewer policy is PR-controlled in this bootstrap change
This PR adds the reviewer policy under .github/ai-review/*, and the workflow bootstrap fallback copies missing base-branch policy/schema files from the PR into /tmp/ai-review-trusted. That means the introducing run can be governed by PR-controlled review instructions and schema rather than an already-trusted base copy. This is a bootstrap-only risk: after merge the base copy should be used, with the current mitigation being nucleus-approved CI plus manual scrutiny of the .github/ additions; any later PR that makes policy fall back to PR-side files should be treated as a steady-state trust-boundary failure.
Findings (status as of supersession)
|
Findings (status as of supersession)
|
| auditor: | ||
| name: auditor | ||
| needs: [decide, skeptic] | ||
| if: needs.decide.outputs.run_auditor == 'true' && needs.skeptic.result == 'success' |
There was a problem hiding this comment.
[MEDIUM] Standalone auditor dispatch is always skipped
workflow_dispatch allows persona: auditor, and decide sets RUN_SKEPTIC=false; RUN_AUDITOR=true for that mode. However, the auditor job still requires needs.skeptic.result == 'success'; when the skeptic job is intentionally skipped, this condition is false, so the standalone Auditor path advertised in the PR never runs. Use always() and explicitly allow the skipped skeptic result for auditor-only dispatch while still blocking when Skeptic fails.
| if: needs.decide.outputs.run_auditor == 'true' && needs.skeptic.result == 'success' | |
| if: always() && needs.decide.outputs.run_auditor == 'true' && (needs.skeptic.result == 'success' || needs.skeptic.result == 'skipped') |
| fi | ||
| echo "Detected auto-fix changes:" | ||
| git status --short | ||
| git add -A |
There was a problem hiding this comment.
[LOW] Auto-fix commits can include review artifacts
The Codex action writes auditor-output.json in the workspace, and the persona can also write auditor-proposed-pr-body.md. git diff --quiet ignores untracked files, but once a real auto-fix exists this git add -A will sweep those artifacts into the chore: auditor auto-fix commit. Exclude the generated review files or add them to .gitignore so auto-fix commits contain only intended source changes.
| git add -A | |
| git add -A -- ':!auditor-output.json' ':!auditor-proposed-pr-body.md' |
Findings (status as of supersession)
|
Findings (status as of supersession)
|
| **If the body is empty or trivial** (less than ~3 sentences of substantive content; just a checked checklist with no description; only template boilerplate): | ||
|
|
||
| - Generate a detailed description covering: motivation, what changed, files of interest, behavioral impact, migration / spec_version implications, testing performed. | ||
| - **In CI**, write the proposed description to `auditor-proposed-pr-body.md` in the workspace. The workflow will detect this file and update the PR body via the post-comment step. Note in your verdict: "PR description was empty; I have proposed one in this comment — please review." |
There was a problem hiding this comment.
[LOW] PR body auto-fill is not implemented
This tells the Auditor to write auditor-proposed-pr-body.md and says the workflow will update the PR body, but the workflow only excludes that file from auto-fix commits and post_review.py never reads or patches the PR body. In steady state, empty PR descriptions will remain empty despite the Auditor believing it handed off an auto-fill. Either implement the body update in the post step, or change this instruction so the proposed body is placed in summary_markdown instead.
| - **In CI**, write the proposed description to `auditor-proposed-pr-body.md` in the workspace. The workflow will detect this file and update the PR body via the post-comment step. Note in your verdict: "PR description was empty; I have proposed one in this comment — please review." | |
| - **In CI**, include the proposed description in `summary_markdown` and note: "PR description was empty; I have proposed one in this comment — please review." |
Findings (status as of supersession)
|
| name: ai-review | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
[HIGH] Fork PRs cannot run the required Codex review path
This workflow still runs on pull_request, but fork-triggered pull_request runs do not receive repository secrets such as OPENAI_API_KEY. Both Codex steps require secrets.OPENAI_API_KEY, and the auditor prompt still has explicit is_fork behavior, so a fork PR will not be able to execute the required review checks once ai-review / skeptic and ai-review / auditor are added to branch protection. workflow_dispatch can help maintainers inspect a PR manually, but it does not make the automatic required check path work for fork PRs. Either implement a secure maintainer-approved fork path that gets secrets while evaluating the PR head safely, or document/enforce that fork PRs are unsupported before these checks become required.
Findings (status as of supersession)
|
|
VERDICT: SAFE BASELINE scrutiny: author is a long-established repo admin with matching commit authorship; no Gittensor allowlist hit; branch Supersedes previous review. No runtime, pallet, Cargo, or lockfile changes are present; review focused on the new AI-review workflows, token boundaries, helper scripts, action pinning, and trusted-instruction handling. The PR-side FindingsNo findings. ConclusionI found no malicious behavior or security vulnerability in the current diff. The previously flagged mutable-action/token-boundary concern remains addressed by immutable action pinning, trusted base extraction, and separation of token-bearing steps from PR-controlled execution. |
|
VERDICT: 👍 Established repo admin / nucleus-caliber author; Gittensor association UNKNOWN from trusted allowlists, with no incentive adjustment applied. Supersedes previous review. Bootstrap-only trusted-file fallback paths exist on this introducing PR; per the trusted review rules, they become structurally unreachable after merge and are not counted as findings. Description discrepancy: the PR body still says Static validation passed for the added Python, shell, YAML, and JSON files ( The overlapping-PR signal points at unrelated open PRs with only one shared file each, so there is no duplicate-work recommendation. FindingsNo findings. Prior-comment reconciliation
ConclusionThe previously-blocking fork path has been resolved by explicitly skipping automatic AI review for fork PRs and documenting manual |
| if [[ "$EVENT_NAME" == "pull_request" && "$IS_FORK" == "true" ]]; then | ||
| echo "::notice::Fork PR auto-trigger — skipping AI review. Use workflow_dispatch to review manually." | ||
| RUN_SKEPTIC=false; RUN_AUDITOR=false |
There was a problem hiding this comment.
[HIGH] Fork PRs bypass the required AI-review checks
On pull_request events from forks, this sets both persona jobs to skipped. The workflow comment immediately above notes that skipped jobs satisfy required checks, so the contributor class that most needs the Skeptic/Auditor gate can merge without either review unless a maintainer remembers to run workflow_dispatch. This is steady-state, not bootstrap-only.
Do not make the required checks green by skipping them for forks. The fork path should leave the required review check failing/pending until a trusted manual run completes, or the workflow should be redesigned around a safe pull_request_target/two-stage pattern that prefetches trusted context from base and runs untrusted head code only in the tokenless Codex step.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: author is a long-established repo admin; branch The PR-side FindingsNo findings. Prior-comment reconciliation
ConclusionNo malicious pattern or security vulnerability was found in the current diff. The prior fork-required-check bypass remains addressed, and the remaining bootstrap-only trust path becomes structurally unreachable after this introducing PR merges. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor UNKNOWN from trusted allowlists; author has admin permission and a long substantive subtensor history, so review was calibrated toward workflow/domain correctness rather than intent. Bootstrap-only trusted-file fallback paths exist on this introducing PR; per the trusted review rules, they become structurally unreachable after merge and are not counted as findings. Description discrepancy: the PR body still says Spec version is bumped from Validation performed: FindingsNo findings. ConclusionThe trusted-base extraction, token separation, fork fail-fast/manual-review path, structured output posting, and generated-weight/spec-version updates are coherent for the intended bootstrap rollout. I found no blocking domain issues. |
| # Fork auto-trigger: secrets (incl. OPENAI_API_KEY) are NOT available | ||
| # to `pull_request` runs from forks, and the default token is | ||
| # read-only. The Codex steps would simply fail. Short-circuit to a | ||
| # clean skip so the required checks resolve as `skipped` (satisfied) | ||
| # and fork PRs are not blocked from merging by the AI-review gate. | ||
| # Human nucleus review is the trust mechanism for fork PRs. A | ||
| # maintainer who specifically wants AI review on a fork PR can | ||
| # invoke this workflow via workflow_dispatch (runs in base context | ||
| # with secrets available). | ||
| if [[ "$EVENT_NAME" == "pull_request" && "$IS_FORK" == "true" ]]; then | ||
| echo "::notice::Fork PR auto-trigger — skipping AI review. Use workflow_dispatch to review manually." | ||
| RUN_SKEPTIC=false; RUN_AUDITOR=false |
There was a problem hiding this comment.
[HIGH] Fork PRs bypass the required AI-review checks
This still sets both required persona jobs to skipped on fork pull_request events. GitHub treats skipped required jobs as successful, so after these checks are added to branch protection a fork PR can merge without either AI review unless a maintainer remembers to manually dispatch. That is a steady-state bypass of the required security gate. Keep required checks failing or pending for fork auto-runs and let workflow_dispatch on the same head SHA be the explicit unblock path, or use a safe pull_request_target design that never checks out untrusted code with secrets.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
5 participants
Description
Adds an AI-powered PR review pipeline driven by
openai/codex-action, with two distinct personas and parallel local-mode skills for both Claude Code and Codex CLI.The two personas
.github/ai-review/skeptic.md) — security-only review. Static analysis only; never compiles or runs PR code. Outputs one ofVERDICT: [SAFE],[VULNERABLE], or[MALICIOUS]. Includes contributor-risk tiering based on account age, contribution graph, prior PR cadence (karma-farming detection), and Gittensor association. Sandbox:read-only..github/ai-review/auditor.md) — domain review, only runs after the Skeptic clears the PR. Applies.github/copilot-instructions.mdin full. May compile, test, and (when the PR is not from a fork) push auto-fix commits for lint failures, missing spec_version bumps, and staleCargo.lock. OutputsVERDICT: 👍/👎. Sandbox:workspace-write.Both personas use sticky comments (markers
<!-- ai-review:skeptic -->and<!-- ai-review:auditor -->), so reruns on subsequent pushes overwrite their previous verdict and explicitly reason about whether prior concerns were addressed.Branch routing
testnet → main: Skeptic only (final security gate before mainnet — Auditor is unnecessary at this stage).workflow_dispatchlets a maintainer fire either persona standalone on any PR number.Gittensor incentive awareness
The Auditor flags duplicate work from Gittensor (SN74) miners, who have a financial incentive to land merges. Detection has three tiers:
index_gittensor.py+ai-review-index-gittensor.yml) — daily cron walks completed issues in theissues-v0ink! contract and asks GitHub which merged PR closed each bountied issue. Output:known-gittensor-accounts.json.gittensor-accounts.txt) — nucleus-maintained list for miners the indexer can't catch (e.g. PAT-only farmers).Local skills
Same persona files, invokable as slash commands locally:
/skeptic,/auditorvia.claude/skills//skeptic,/auditorvia.agents/skills/Local mode writes output to terminal +
.skeptic-suggestions.patch/.auditor-suggestions.patch/.auditor-pr-description.md. No GitHub side effects.Related Issue(s)
N/A — internal tooling.
Type of Change
Breaking Change
None.
Setup required after merge
OPENAI_API_KEY(already done).ai-review / skepticandai-review / auditorto required checks in branch protection.ai-review-index-gittensoronce manually viaworkflow_dispatchto seedknown-gittensor-accounts.json; daily cron takes over after that.Checklist