chore(proto): auto-commit generated artifacts for internal PRs#3490
chore(proto): auto-commit generated artifacts for internal PRs#3490lspassos1 wants to merge 5 commits into
Conversation
|
@lspassos1 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR teaches the proto freshness workflow to auto-commit generated artifacts back to internal PR branches via
Confidence Score: 3/5Not safe to merge as-is — fork PRs now run untrusted Makefile code on CI runners, reversing the intentional security gate of the original workflow. A P1 security regression (fork code execution) is present in the core changed file; the fix is straightforward but requires restructuring the job or adding per-step guards before the issue is resolved. .github/workflows/proto-check.yml — specifically the missing
|
| Filename | Overview |
|---|---|
| .github/workflows/proto-check.yml | Adds auto-commit of generated artifacts for internal PRs and splits checkout/verify steps by repo origin; security regression: removing the job-level fork guard now runs untrusted fork Makefile code on CI runners |
| CONTRIBUTING.md | Adds documentation section explaining the generated-artifact auto-commit contract for internal vs. fork PRs; accurate and consistent with the workflow changes |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pull_request event] --> B{Internal PR?}
B -- Yes --> C[Checkout branch with token]
B -- No --> D[Checkout fork merge commit]
C --> E[setup-go + cache]
D --> E
E --> F[make install-buf install-plugins]
F --> G[make generate]
G --> H{Internal PR?}
H -- Yes --> I{Generated files changed?}
I -- No --> J[Exit 0 — up to date]
I -- Yes --> K[git add + commit + push to PR branch]
K --> L[Bot commit triggers new sync event → 2nd workflow run]
H -- No --> M{git diff exit-code?}
M -- Clean --> N[Exit 0 — up to date]
M -- Dirty --> O[Fail with fork instructions]
style D fill:#f99,stroke:#c00
style F fill:#f99,stroke:#c00
style G fill:#f99,stroke:#c00
style L fill:#ffd,stroke:#aa0
Comments Outside Diff (1)
-
.github/workflows/proto-check.yml, line 44-51 (link)Fork PR code execution — security regression
The original workflow guarded the entire job with
if: github.event.pull_request.head.repo.full_name == github.repository, so fork PRs never ran any steps. After this PR, the job-level gate is gone, meaningmake install-buf install-plugins(line 45) andmake generate(line 51) execute the fork's version of theMakefileon a CI runner for every external contributor PR. A malicious fork PR could modifyMakefileto run arbitrary shell commands — network exfiltration, resource abuse, or probing the runner environment — none of which is prevented by thecontents: readtoken restriction that fork PRs receive.The fork-PR verification steps at lines 69–94 correctly block pushing, but they run after the untrusted code has already executed. If the intent is to keep validation-only behaviour for forks, the safe pattern is to add
if: github.event.pull_request.head.repo.full_name == github.repositoryto theInstall buf and protoc pluginsandRun proto generationsteps (or restore the job-level condition and use a separate job with apull_request_targettrigger for forks).
Reviews (1): Last reviewed commit: "fix(ci): serialize proto freshness runs" | Re-trigger Greptile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3dff9ad74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if: github.event.pull_request.head.repo.full_name == github.repository | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.ref }} |
There was a problem hiding this comment.
Checking out github.event.pull_request.head.ref makes the internal path generate artifacts from the branch tip instead of the PR merge result. When main has newer proto-related commits that the PR branch has not rebased yet, this step can auto-commit src/generated//docs/api/ files that are clean for the head branch but stale for what will actually merge, which undermines the freshness gate. Please validate against the merge commit (or add a separate merge-ref verification step) before pushing generated artifacts.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| permissions: | ||
| contents: read | ||
| contents: write |
There was a problem hiding this comment.
contents: write is broader than needed
contents: write is declared at the top of the workflow and therefore applies to every step, including those that run for fork PRs (where GitHub's own security model restricts it to read-only in practice). Following the principle of least privilege, it is better to keep the workflow-level permission as contents: read and override to contents: write at the job level. This makes the intent explicit and limits blast radius if a future refactor moves steps around.
| - name: Commit generated artifacts for internal PRs | ||
| if: github.event.pull_request.head.repo.full_name == github.repository | ||
| env: | ||
| PR_HEAD_REF: ${{ github.event.pull_request.head.ref }} | ||
| run: | | ||
| if git diff --quiet -- src/generated/ docs/api/ && [ -z "$(git ls-files --others --exclude-standard src/generated/ docs/api/)" ]; then | ||
| echo "Proto-generated code is up to date." | ||
| exit 0 | ||
| fi | ||
|
|
||
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| git add src/generated/ docs/api/ | ||
| git commit -m "chore(proto): update generated artifacts" | ||
| git push origin HEAD:"$PR_HEAD_REF" |
There was a problem hiding this comment.
When the bot pushes chore(proto): update generated artifacts, GitHub fires a new pull_request synchronize event, causing this workflow to run a second time for the same PR. The second run will call make generate, find no diff, and exit cleanly — but it burns CI minutes and can confuse reviewers who see two consecutive proto-freshness runs. Adding [skip ci] to the bot commit message (or checking for github.actor == 'github-actions[bot]' at the job level) prevents the redundant run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddac67158c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| internal-merge-freshness: | ||
| needs: internal-auto-generate | ||
| if: github.event.pull_request.head.repo.full_name == github.repository && needs.internal-auto-generate.outputs.pushed != 'true' |
There was a problem hiding this comment.
In .github/workflows/proto-check.yml, the internal-merge-freshness job is skipped whenever internal-auto-generate sets pushed=true, but the preceding push is done with GITHUB_TOKEN; GitHub does not create new workflow runs for events triggered by that token, so the expected follow-up pull_request run may never happen. For PRs where codegen changes are auto-committed, this can leave the merge-result freshness check unexecuted and allow stale generated artifacts relative to base to pass.
Useful? React with 👍 / 👎.
| PROTO_CHANGED=$(printf '%s\n' "$CHANGED_FILES" | grep -E '^proto/' || true) | ||
| GENERATED_CHANGED=$(printf '%s\n' "$CHANGED_FILES" | grep -E '^(src/generated/|docs/api/)' || true) | ||
|
|
||
| if [ -n "$PROTO_CHANGED" ] && [ -z "$GENERATED_CHANGED" ]; then |
There was a problem hiding this comment.
In the fork safety path, CHANGED_FILES includes Makefile, but the failure gate only checks PROTO_CHANGED, so fork PRs that modify generator behavior via Makefile (for example plugin/version changes) can pass without committing the resulting src/generated/ or docs/api/ updates. That creates a false green path for stale generated outputs whenever codegen inputs change outside proto/.
Useful? React with 👍 / 👎.
|
Follow-up pushed in Changes:
Validation:
|
1 participant
Summary
Moves generated proto/OpenAPI artifact churn out of normal internal PR review by teaching the proto freshness workflow to run
make generateand push generated artifacts back to same-repository PR branches. Fork PRs keep a safe validation-only path with an explicit failure message because CI cannot safely push to contributor branches.cc @koala73
Closes #3340
Type of change
Affected areas
Root cause
The existing proto freshness check could detect stale generated files, but contributors still had to include generated artifacts manually in every PR. That creates noisy reviews and makes code changes harder to evaluate.
Changes
make generatein.github/workflows/proto-check.yml.src/generated/anddocs/api/back to the PR branch.make generatelocally.CONTRIBUTING.md.Validation
ruby -e "require 'yaml'; YAML.load_file('.github/workflows/proto-check.yml'); puts 'yaml ok'"git diff --checkRisk
Moderate. The workflow gets write permissions for internal PR branches only. Forks remain validation-only, and the push target is quoted to avoid shell evaluation of branch names.