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)
|
Findings (status as of supersession)
|
ai-squad-reviewer
left a comment
ai-squad-reviewer
left a comment
There was a problem hiding this comment.
Substantial new feature; mostly additive (workflows, scripts, prompts) with no runtime/consensus changes. Security model is well thought-through (base-branch trusted copies, token isolation, App-token shielding, bootstrap exception). A few minor nits / defensive concerns inline — none are blockers.
|
|
||
| - name: Run Codex (auditor persona) | ||
| id: codex | ||
| uses: openai/codex-action@v1 |
There was a problem hiding this comment.
🔵 [LOW] /tmp/pre-codex-head.txt is written here but never read by any subsequent step. The auto-fix detection step uses git diff --quiet instead. Either wire it in (e.g. compare HEAD to the snapshot before pushing) or drop the step.
| echo "No auto-fix changes." | ||
| exit 0 | ||
| fi | ||
| echo "Detected auto-fix changes:" |
There was a problem hiding this comment.
🔵 [LOW] git diff --quiet && git diff --cached --quiet does not detect untracked files. If Codex ever creates a new file as part of an auto-fix (e.g. a missing test added at the persona's discretion, or a new migration scaffolded as part of a spec_version bump), this check will say "No auto-fix changes" and the file will be silently dropped. git status --porcelain would be safer. Today the persona spec only mentions edits to existing files, so this is latent — but worth tightening since the failure mode is silent.
|
|
||
| # Cross-reference: which open PRs touch any of the same files as this PR? | ||
| THIS_PR_FILES=$(jq -c '.files | map(.path)' "$OUTPUT_DIR/pr-files.json") | ||
| echo "[]" > "$OUTPUT_DIR/overlapping-prs.json" |
There was a problem hiding this comment.
🔵 [LOW] This is a serial gh pr view per open PR (up to 100). On a repo with many open PRs that's ~minutes of wall-clock before Codex even starts. Consider doing the overlap calculation with a single GraphQL query (pullRequests(states: OPEN) { files { path } }) or, at minimum, running these in parallel with xargs -P so the prefetch stays well under a minute.
| "addressed": "✅ Addressed", | ||
| "no_longer_applies": "⏭️ No longer applies", | ||
| "not_addressed": f"➡️ Carried forward", | ||
| } |
There was a problem hiding this comment.
⚪ [NIT] Stray f-prefix — no interpolation here, should be a plain string literal.
|
|
||
| # 1. Detect + extract auditor changes from the dirty workspace. | ||
| cd "$PR_DIRTY" | ||
| git "${SAFE_GIT_OPTS[@]}" add -A -- ':!auditor-output.json' |
There was a problem hiding this comment.
[HIGH] Credentialed auto-push still runs PR-mutated git state
This step still has PUSH_TOKEN in its environment while it runs git add/git diff inside the PR-mutated checkout. The SAFE_GIT_OPTS block does not fully neutralize repository-controlled Git behavior: core.attributesFile=/dev/null does not disable checked-in .gitattributes, and Codex or any auditor-run build script can leave .git/config changes behind before this step. git add can invoke clean filters and git diff can invoke external diff/textconv drivers, so a malicious PR can arrange for those helpers to execute with the write token in env.
Split patch extraction from credentialed pushing: generate /tmp/auto-fix.patch in a separate step/job with no push token in the environment, then run only the fresh-clone/apply/commit/push portion in the token-bearing step. The token-bearing step should not execute Git at all in $github.workspace / $PR_DIRTY.
Findings (status as of supersession)
|
Findings (status as of supersession)
|
| gh api "repos/$REPO/issues/$PR_NUMBER/comments" --paginate \ | ||
| > "$OUTPUT_DIR/pr-comments.json" |
There was a problem hiding this comment.
[LOW] Paginated issue comments can produce invalid JSON
gh api --paginate writes one JSON array per page unless the result is slurped. On PRs with more than one page of issue comments, pr-comments.json can become concatenated arrays, and the following jq commands will fail before either persona runs. Slurp and merge the pages into one array.
| gh api "repos/$REPO/issues/$PR_NUMBER/comments" --paginate \ | |
| > "$OUTPUT_DIR/pr-comments.json" | |
| gh api "repos/$REPO/issues/$PR_NUMBER/comments?per_page=100" \ | |
| --paginate --slurp --jq 'add' \ | |
| > "$OUTPUT_DIR/pr-comments.json" |
| comments = gh_api("GET", f"repos/{repo}/issues/{pr}/comments?per_page=100") | ||
| if not isinstance(comments, list): | ||
| return (None, "", "") | ||
| best: tuple[int | None, str, str] = (None, "", "") | ||
| for c in comments: | ||
| body = c.get("body", "") | ||
| if marker_live in body and marker_dead not in body: | ||
| best = (int(c["id"]), body, c.get("html_url", "")) | ||
| return best |
There was a problem hiding this comment.
[LOW] Prior sticky lookup only scans the first comment page
This fetches only the first 100 issue comments in the API's default order. On long-running or noisy PRs, the live sticky can fall outside that page, causing the script to miss it and post duplicate live stickies instead of superseding the old one. Fetch the most recent comments first and return the first live marker found.
| comments = gh_api("GET", f"repos/{repo}/issues/{pr}/comments?per_page=100") | |
| if not isinstance(comments, list): | |
| return (None, "", "") | |
| best: tuple[int | None, str, str] = (None, "", "") | |
| for c in comments: | |
| body = c.get("body", "") | |
| if marker_live in body and marker_dead not in body: | |
| best = (int(c["id"]), body, c.get("html_url", "")) | |
| return best | |
| comments = gh_api( | |
| "GET", | |
| f"repos/{repo}/issues/{pr}/comments?per_page=100&sort=created&direction=desc", | |
| ) | |
| if not isinstance(comments, list): | |
| return (None, "", "") | |
| for c in comments: | |
| body = c.get("body", "") | |
| if marker_live in body and marker_dead not in body: | |
| return (int(c["id"]), body, c.get("html_url", "")) | |
| return (None, "", "") |
Findings (status as of supersession)
|
| else | ||
| PR='${{ github.event.pull_request.number }}' | ||
| PERSONA="" | ||
| fi |
There was a problem hiding this comment.
[HIGH] Manual auditor dispatch can satisfy the required Skeptic check without running it
The PR body instructs maintainers to make ai-review / skeptic and ai-review / auditor required checks, but this workflow_dispatch branch lets a maintainer choose persona=auditor, setting RUN_SKEPTIC=false. The skeptic job is then skipped for the PR head SHA, and GitHub skipped-job conclusions can satisfy required checks. A single manual auditor-only run can therefore produce green required AI-review checks without executing the security persona. Keep standalone/manual runs on distinct non-required check names, or make the required PR workflow always execute Skeptic before Auditor for non-testnet -> main PRs.
Findings (status as of supersession)
|
Findings (status as of supersession)
|
Findings (status as of supersession)
|
| # --------------------------------------------------------------------- | ||
| - name: Run Codex (skeptic persona) | ||
| id: codex | ||
| uses: openai/codex-action@v1 |
There was a problem hiding this comment.
[HIGH] Token-bearing actions are pulled by mutable tags
This workflow runs openai/codex-action@v1 in steps that receive OPENAI_API_KEY (also at line 416), and uses actions/create-github-app-token@v2 in steps that receive the GitHub App private key (lines 122 and 325). Those are mutable major-version tags, so a retag or upstream release-chain compromise would execute attacker-controlled action code with the review system's credentials on every approved AI-review run. Pin token-bearing actions to immutable commit SHAs and update them only via reviewed PRs.
Findings (status as of supersession)
|
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 is triggered with pull_request, but both Codex steps require ${{ secrets.OPENAI_API_KEY }} and the pre/post steps require write-capable GitHub credentials. On PRs from forks, repository secrets are not exposed to pull_request workflows, and the default token is read-only. That means the is_fork handling later in the workflow is unreachable in practice: fork PRs will fail before the Auditor can emit suggestions, and once ai-review / skeptic and ai-review / auditor are required checks, external fork PRs will be blocked or left outside the intended review gate.
Do not fix this by blindly switching the whole job to pull_request_target while checking out PR code with secrets available. Either document that fork PRs are unsupported and remove the fork-specific path, or split the design so the secret-bearing target workflow only fetches trusted metadata/comments while any PR-head checkout or build runs without repository secrets.
Findings (status as of supersession)
|
sam0x17
added
skip-cargo-audit
|
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. Prior-comment reconciliation
ConclusionI found no malicious behavior or security vulnerability in the current diff. The previously flagged mutable-action/token-boundary issue remains addressed by immutable action pinning and separation of token-bearing steps from PR-controlled execution. |
| 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.
|
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. 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. Findings
Prior-comment reconciliation
ConclusionThe core reviewer pipeline remains close, but the fork-PR path is still non-functional even though the workflow and persona prompts explicitly model fork behavior. Resolve that path or explicitly scope forks out before making these checks required. |
3 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