ci: run CodeQL from a workflow so fork pull requests get analyzed - #70
Conversation
CodeQL default setup never produced an analysis for pull requests from forks. The `code_scanning` rule on `main` requires one, so external contributions were unsatisfiable by construction and #69 could only be merged with an admin override — the gate was theatre for exactly the pull requests it should cover. Replace default setup with an advanced setup workflow. A workflow living in the repository does run for fork pull requests, so the required check now exists for contributors as well as maintainers. Default setup is disabled; leaving it on would make these SARIF uploads conflict. Analysis stays source-based with `build-mode: none` for all three languages, so a pull request pays no system packages, no gpui git checkout, and no full build. Query suite and language set match what default setup was configured with.
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed .github/workflows/codeql.yml against the pinned codeql-action@ff2f1c6 (v4.37.7) sources.
Verified: the analyze action always attempts the SARIF upload on pull_request events (upload-lib.ts uploadPayload has no fork-token guard), and the CodeQL (…) check runs from this PR are already reporting success against the head SHA — so the check exists and can satisfy the code_scanning required check. Workflow-level permissions: contents: read plus job-level security-events: write is the documented advanced-setup shape. build-mode: none is valid for all three languages (actions/js-ts are interpreted; rust none-build is supported), and avoids the gpui/system-dep build. SHA pinning matches the sibling workflows.
Nothing clears the bar for a finding:
- Fork-PR behavior: for same-repo and fork PRs, GitHub forces
GITHUB_TOKENto read-only on pull_request events when the repo sets restrictive defaults, but the explicit job-levelsecurity-events: writegrants the upload, and on fork PRs where the token is downgraded the action surfaces a clear 403-style error rather than silently skipping — the check would fail loudly, not pass vacuously. The PR's own runs (which did upload: "No new alerts in code changed by this pull request") prove the happy path works. - Weekly cron +
push: mainkeepsmainanalyzed between pushes;cancel-in-progressscoped to pull_request is safe since alerts are attributed to themain/scheduled runs. - No MCP stdio, Rust, installer, npm, skill, or docs surface is touched, so none of the repo-specific isolation/stdio/idempotency contracts apply.
Clean change; no inline comments.
Self-reviewCopilot hit its quota on this PR and revuto's check run reports The claim this PR makes is that advanced setup produces the analysis the Three analyses attributed to the pull request ref, tool Default setup is Merging with an admin override for the same reason #69 needed one: solo approval requirement with Copilot out of quota. That's the last fork pull request that should ever need it. |
…lled (#81) `summarizeReviewSteps` set `hasFindings` whenever `post_review` or `post_issue_comment` appeared in a step, without looking at the result. Two false reports came out of that: - A clean review posted through `post_review` with an empty `comments` array failed the check as "Revuto posted one or more findings on the pull request. Address the review comments and push a new head." — on a pull request with no comments at all. Observed on agent-sh/agent-workspace-linux#70, where the review body itself said "Clean change; no inline comments". - A `post_review` the API rejected also set `hasFindings`, so a run that put nothing on the pull request reported posted findings, and a `skip_review` after the failure would have reported a clean review the reviewer never managed to publish. Findings now come from calls that succeeded. A posting call that failed is counted in a new `postFailures`, and the check fails on it with what actually happened rather than claiming findings exist. The clean-review case is settled at the tool instead of guessed at afterwards: `post_review` refuses an empty `comments` array and points at `skip_review`, which is the path that lets the App submit the approval. The system prompt already said not to post a "looks good" review; now the tool holds that line. That refusal carries its own error code, and it is deliberately not a `postFailure` — it drops nothing, so the `skip_review` that follows stays a clean pass instead of moving the same false failure to a new field.
Summary
CodeQL default setup never produced an analysis for pull requests from forks. The
mainruleset requirescode_scanning(CodeQL), so external contributions were unsatisfiable by construction — #69 was fully green and approved and still had to be merged with an admin override. The gate was theatre for exactly the pull requests it should cover.A workflow living in the repository does run for fork pull requests, so this replaces default setup with advanced setup and the required check now exists for contributors as well as maintainers.
actions,javascript-typescript,rust,security-extended.build-mode: nonefor all three, so analysis stays source-based — no system packages, no gpui git checkout, no full build per pull request.mainbetween pushes..github/workflows.Checklist
cargo fmt --checkpasses — N/A, no Rust changecargo clippy --locked -- -D warningspasses — N/A, no Rust changecargo test --lockedpasses — N/A, no Rust changegit diff --checkis cleanscripts/integration_smoke.sh— N/A, no runtime behavior changeNotes for reviewers
The thing to watch on this PR is its own
CodeQL (…)checks: they are the proof the replacement works. Merging is what makes the required check reachable for the next external contribution.