mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-18 16:02:10 +00:00
main
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ruben Fiszel
|
b4c834f3cd |
ci: run Codex/Pi review on fork PRs when a maintainer triggers it (#10069)
* ci: run Codex review on fork PRs when a maintainer triggers it The fork skip in codex-pr-review.yml unconditionally bailed on cross-repository PRs, so even a maintainer's /codex or /review comment (routed through pr-review-commands.yml via workflow_call, gated by check-write-access) skipped external PRs. Gate the skip on the automatic pull_request trigger only, detected via an empty INPUT_PR_NUMBER (the metadata step already branches on this at the same step). The workflow_call path now reviews fork PRs; the auto pull_request trigger still skips them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run Pi review on fork PRs when a maintainer triggers it Apply the same fork-skip gating as the Codex review: skip fork PRs only on the automatic pull_request trigger (empty INPUT_PR_NUMBER), so a maintainer's /pi or /review comment (workflow_call, gated by check-write-access) reviews external PRs. Claude's pr-ready-review.yml needs no change: it has no fork skip, checks out main (not the fork ref), and reviews via gh pr diff/view with a restricted tool allowlist, so it already handles fork PRs on the command path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: harden fork-review path against secret exfiltration Addresses the CI review of the fork-review enablement. On the fork path (maintainer-triggered workflow_call for a cross-repository PR), the reviewer ran an autonomous agent over the attacker-controlled merge checkout with the EE token present, full-access sandbox, and the review prompt itself read from that untrusted checkout — so a malicious fork could rewrite the reviewer's own instructions to exfiltrate secrets. For fork PRs only (detected via the is_fork step output): - withhold WINDMILL_EE_PRIVATE_ACCESS: skip the EE access/checkout/ substitution steps, so the private-repo token is never in the env. - read REVIEW.md and the prompt file from the trusted base ref (git show origin/<base>:...) instead of the merge checkout. - restrict the agent: Codex runs with -s workspace-write (network off) instead of danger-full-access; Pi drops the bash tool. Non-fork PRs are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: redact provider credentials from fork review comments The model call needs the provider credential in its environment/config, so a network-disabled sandbox alone can't stop a prompt-injected fork review from reading the key (Codex: $HOME/.codex/auth.json; Pi: /proc/self/environ) and emitting it in the final message, which both workflows post verbatim. GitHub Actions log masking does not cover comments posted via the API. Strip the known credential values (OpenAI key + raw Codex auth JSON and its nested tokens; DeepSeek key) from the review body before posting, closing the comment as an exfiltration channel. Applied unconditionally since a credential should never appear in a review comment regardless of trigger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: don't persist github.token in fork review checkout actions/checkout writes github.token into .git/config (http.extraheader) by default. The review agent can read the checked-out tree, so on the fork path a prompt injection could exfiltrate that token (issue/PR write) via .git/config — the provider-credential redaction added earlier didn't cover it. Set persist-credentials: false on the merge-ref checkout so the token is never written to disk. Safe on both paths: the only later git op is an unauthenticated fetch from the public origin, EE checkout uses its own token, and gh uses GH_TOKEN. Also redact github.token from the posted comment as defense-in-depth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: disable Pi project-local discovery on fork reviews Pi auto-discovers and executes project-local .pi extensions (.ts/.js) at startup with DEEPSEEK_API_KEY in its environment — before the --tools allowlist applies — so a fork could add an extension that exfiltrates the key over the network, which output redaction can't catch. On the fork path (cwd is the fork checkout), pass --no-extensions to disable extension discovery, plus --no-skills/--no-prompt-templates/--no-themes/ --no-context-files so fork-controlled skills, templates, themes, and AGENTS.md/CLAUDE.md aren't auto-loaded into the reviewer's prompt as an injection vector. Non-fork behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: use unguessable delimiter for untrusted PR metadata outputs The PR title/body were written to $GITHUB_OUTPUT with a fixed heredoc terminator (PR_BODY_EOF). A fork author could embed that terminator in their PR body to close the heredoc early and append their own output lines — e.g. is_fork=false, which (last-write-wins) overrides the real is_fork=true and puts fork code back on the trusted path (EE checkout + substitute_ee_code.sh with the private token, full-access agent). Generate a per-run random delimiter (128 bits from /dev/urandom) for the title and body heredocs so the terminator can't be predicted or embedded. Everything else in the block is single-line and newline-free, so this closes the injection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: set PI_OFFLINE=1 on fork Pi reviews to block package resolution --no-extensions only filters which resources are *loaded*; Pi still resolves packages declared in a fork's .pi/settings.json first, running `npm install` / the configured npmCommand and lifecycle scripts with DEEPSEEK_API_KEY in env and network available — before the extension filter applies. Set PI_OFFLINE=1 on the fork path so the resolver's installMissing() short- circuits (returns false) for every missing package, skipping all install/clone/ lifecycle execution. It gates only startup network ops (installs, helper-binary downloads), not the provider inference call, so the review still runs. Verified: a fork .pi/settings.json with a malicious npmCommand does not execute under the flag. Non-fork path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run fork Pi review from an isolated dir to cut off project config Root cause of the recurring fork-review exposure: Pi resolves every project config from <cwd>/.pi — settings/packages, extensions, skills, themes, prompts, SYSTEM.md, APPEND_SYSTEM.md — so running inside the fork checkout let a fork inject any of them to execute code or rewrite the reviewer's system prompt with DEEPSEEK_API_KEY in env. Per-flag opt-outs (--no-extensions, PI_OFFLINE, ...) only covered discovered vectors one at a time (SYSTEM.md wasn't covered). Discovery is cwd-based (single level, no walk-up; global fallback is the trusted runner home), so run Pi from a fresh mktemp dir where no fork .pi/* is on the path. The fork agent has no shell, so pre-compute the diff (base...head SHAs are trusted) into the context file it reads; it may still read fork files by absolute path for extra context — reads are safe, only config discovery and code execution were the risk. Outputs now use absolute workspace paths since cwd moved. The --no-* flags and PI_OFFLINE stay as belt-and-suspenders. Non-fork path unchanged. Verified: a fork .pi/SYSTEM.md sentinel is not discovered from the isolated cwd. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: keep review artifacts outside the checkout to defeat symlink writes Both workflows wrote generated files (final message, event stream, review context, prior-comments) into $GITHUB_WORKSPACE. On the fork path the merge tree is attacker-controlled, so a fork could commit any of those paths as a symlink (e.g. codex-final-message.md -> ../../_actions/actions/github-script/v7/dist/ index.js). Our write would follow it and overwrite the next action's code, which then executes with the provider credential and the write-capable GitHub token — no prompt injection required. Route every generated file through $RUNNER_TEMP, which is runner-created and outside the checkout, so no fork-committed symlink is on the path: - prior-comments.json and pr-review-context.md are written to RUNNER_TEMP; the context step reads prior-comments from there. - The agent is given the context file's absolute RUNNER_TEMP path (appended to the prompt); prompt files updated to reference it instead of a checkout- relative path. Pi (no shell on forks) gets the diff pre-computed into that context file; the isolated-cwd hardening is retained. - Codex writes -o to RUNNER_TEMP; Pi writes its events/final message there; both post steps read from RUNNER_TEMP. Non-fork behavior is functionally unchanged (trusted checkout; same review inputs, now sourced from RUNNER_TEMP). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: condense fork-review comments to the 4-line limit AGENTS.md requires each invariant stated in <=4 lines. Trim the security comments added in this branch (fork-skip rationale, output delimiter, isolated cwd, RUNNER_TEMP artifacts, credential redaction) to comply without dropping the constraint each one records. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ruben Fiszel
|
11b60e5b95 |
ci: share review policy across Claude/Codex/Pi via review-prompt-shared.md (#9035)
* ci: share review policy across Claude/Codex/Pi via review-prompt-shared.md All three reviewers now consume a single canonical policy document (.github/review-prompt-shared.md) covering AGENTS.md compliance, severity triage (P0/P1/P2), and a checklist for new public surfaces (auth contract, module placement, half-finished pub fns, input validation). Each tool's own prompt file shrinks to just its output-format quirks, and each workflow concatenates shared + tool-specific at runtime before invoking the model. Drops the suppressive "Prefer at most 10 findings" / "Keep the review high signal. If there is no clear issue, return no findings" wording from Codex and Pi, which was clipping P1 and P2 findings (e.g. half-finished pub fn, blocking I/O, wrong module placement). Replaces it with severity triage so both reviewers report all P0/P1 and surface P2 when the diff invites it. Also makes AGENTS.md authoritative for Codex (was CLAUDE.md, which is just @AGENTS.md in this repo) and adds an explicit "new public function" checklist that covers the missing-auth-check failure mode none of the three reviewers flagged on the test PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: move test-coverage assessment to shared prompt, slim per-tool prompts - Replace per-tool 'Reproduction instructions' with a single shared 'Test coverage assessment' section that asks each reviewer to evaluate automated coverage (sufficient / thin / appropriate) and describe what manual verification remains, if any. - Slim per-tool prompts to the absolute minimum: just where to read context, the comment header, severity tagging, and the Pi-only 'no preamble' constraint. Everything else lives in the shared policy. - Drop the model name from Pi's title ('Pi Review (DeepSeek V4)' → 'Pi Review') — the title's job is to let the bot find its own prior comment when re-reviewing; the model is irrelevant to the reader. The titles ('## Codex Review', '## Pi Review') stay because Codex and Pi both post as github-actions[bot], so the heading is the only discriminator the bot can use to find its own past comment in the prior-discussion context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: scope test-coverage assessment to layers actually changed Don't ask reviewers about integration tests on a frontend-only diff or about playwright tests on a backend-only diff. The shared 'Test coverage' section now lists categories (backend / frontend / CI-docs) and tells the reviewer to skip the ones the PR does not touch — only ask about Rust integration tests when backend handlers/workers/queues were modified, only ask about frontend tests when components or state machines were touched, and explicitly call out 'no automated tests expected' for CI/docs/config diffs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: don't ask reviewers to flag missing frontend component tests The Windmill frontend codebase doesn't generally test Svelte components — existing tests cover pure-logic utilities only (flowDiff, previousResults, copilot logic, dbtable queries, etc.). Asking reviewers to flag every new component for lacking a test would produce noise inconsistent with the established convention. Limit the frontend test-coverage check to new pure-logic utilities (files that would naturally have a sibling *.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: point local-review skill at the shared review policy Codex flagged (and Pi confirmed on its second pass) that slimming .claude/review-prompt.md to output-only broke the local-review skill contract — the skill still told Claude to read only that file for the review criteria, so /local-review would no longer apply severity triage, the public-surface checklist, or AGENTS.md compliance. Update the skill to read .github/review-prompt-shared.md as the policy source and .claude/review-prompt.md only for Claude output preferences. Also align the local output format with the severity-tag convention used by the workflow reviewers, and replace the lingering 'CLAUDE.md compliance' wording with 'AGENTS.md compliance'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Ruben Fiszel
|
505f78bd29 |
ci: re-review on push, thread prior PR comments into reviewer context (#9032)
* ci: re-review on push, thread prior PR comments into reviewer context - Add 'synchronize' to all three review workflow triggers so each push to a PR branch re-runs Claude/Codex/Pi. Existing cancel-in-progress concurrency groups ensure only the latest push's review actually executes. - Fetch the most recent up to 20 PR comments before each review and inject them into the prompt context so the reviewer can recognize its own previous review, focus on what changed, and avoid repeating findings the human already addressed. - Update the three review prompts (Claude, Codex, Pi) to instruct the reviewer to honor the prior-discussion section when present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump codex CLI to 0.128.0 for gpt-5.5 support Codex 0.117.0 rejects the gpt-5.5 model with 'requires a newer version of Codex'. 0.128.0 is the current stable release on npm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: limit synchronize re-trigger to pi review only Re-running Claude and Codex on every push gets expensive fast on busy PRs. Pi (DeepSeek-V4) is cheap enough to re-run per push, while Claude/Codex remain on opened/ready_for_review and re-trigger via slash commands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Ruben Fiszel
|
1ee73c51ba |
ci: multi-tool PR reviews (Codex/Pi/Claude) with slash commands (#9026)
* ci: add Pi+DeepSeek-V4 review and slash command dispatcher Auto-reviews now fan out to Claude (Opus), Codex (gpt-5.4), and Pi (DeepSeek-V4-Pro) on PR open/ready. PR comments support /review (all three), /codex, /pi, /claude with optional extra context appended to the prompt. All review workflows now substitute EE code before review and gate the auto-trigger path on org membership of the PR author. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: fix command parser whitespace, align checkout v5, broaden PR perms - Trim leading/trailing whitespace from comment first line so /review with leading space parses correctly (caught by Pi review) - Standardize EE checkout step on actions/checkout@v5 across all three review workflows (caught by Pi review) - Bump pull-requests permission to write to satisfy GitHub's PR comment endpoint when issues=write alone is rejected Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
 centdix
|
2862c1cf56 |
add codex PR review workflow (#8626)
* feat: add codex PR review workflow * refactor: simplify codex PR review comments * chore: use ubicloud for codex review * fix: harden codex review workflow * chore: use chatgpt auth for codex review |