Update goose-pr-reviewer.yml - #9
Conversation
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
SummaryThe workflow gives maintainers an easy way to trigger goose-based PR reviews, but there are a couple of security gaps that should be addressed before relying on it. 🔴 Blocking Issues
🟡 WarningsNone found 🟢 SuggestionsNone ✅ Highlights
Review generated by goose |
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
1 similar comment
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
|
no oppppppp |
SummaryThe workflow change is minimal, but I reviewed the entire 🔴 Blocking IssuesNone found. 🟡 Warnings
🟢 Suggestions
✅ Highlights
Review generated by goose |
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
|
|
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
SummaryThis PR adds a security warning comment and improves shell variable handling by moving PR_NUM to environment variables. However, the entire workflow file has critical security vulnerabilities from shell injection via untrusted PR metadata and user comments, plus an invalid default model name. These issues must be fixed before this workflow can safely run. 🔴 Blocking Issues1. Critical: Shell Injection via PR Title and Comment (HIGH SEVERITY)Location: echo "$GOOSE_RECIPE" | envsubst '$PR_NUMBER $PR_TITLE $REVIEW_INSTRUCTIONS' > /tmp/recipe.yamlProblem: Or a comment like: These would be directly interpolated into the YAML recipe and potentially executed. Impact: Arbitrary code execution in the GitHub Actions runner with write access to the repository. Fix: Properly escape these variables or use a safer templating mechanism: # Option 1: Use jq to safely construct the YAML
jq -n \
--arg pr "$PR_NUMBER" \
--arg title "$PR_TITLE" \
--arg instructions "$REVIEW_INSTRUCTIONS" \
--rawfile recipe <(echo "$GOOSE_RECIPE") \
'$recipe | gsub("\\$\\{PR_NUMBER\\}"; $pr) | gsub("\\$\\{PR_TITLE\\}"; $title) | gsub("\\$\\{REVIEW_INSTRUCTIONS\\}"; $instructions)' \
> /tmp/recipe.yaml
# Option 2: Use a Python/Ruby script for safe substitution
# Option 3: Export as env vars and let goose handle them directly2. Critical: Invalid Default Model NameLocation: # - GOOSE_MODEL: Model name (default: gpt-5.1-codex)
...
GOOSE_MODEL: ${{ vars.GOOSE_MODEL || 'gpt-5.1-codex' }}Problem: Evidence: Other workflows use Fix: Use a valid model name: GOOSE_MODEL: ${{ vars.GOOSE_MODEL || 'gpt-4o' }}
# or
GOOSE_MODEL: ${{ vars.GOOSE_MODEL || 'gpt-4-turbo' }}3. Critical: No Validation of PR MetadataLocation: gh api /repos/${{ github.repository }}/pulls/$PR_NUM > /tmp/pr.json
gh pr diff $PR_NUM --repo ${{ github.repository }} > /tmp/pr.diffProblem: No validation that the API calls succeeded or that the PR exists. If they fail silently, the workflow continues with empty/invalid files, potentially causing confusing behavior or exposing the agent to malformed inputs. Fix: Add error checking: if ! gh api /repos/${{ github.repository }}/pulls/$PR_NUM > /tmp/pr.json; then
echo "Error: Failed to fetch PR #$PR_NUM"
exit 1
fi
if ! gh pr diff $PR_NUM --repo ${{ github.repository }} > /tmp/pr.diff; then
echo "Error: Failed to fetch diff for PR #$PR_NUM"
exit 1
fi🟡 Warnings1. Shallow Checkout Limits Review ContextLocation: fetch-depth: 1Problem: Fetching only the latest commit limits the reviewer's ability to understand git history, see blame information, or understand the broader context of changes. This reduces review quality. Recommendation: Use fetch-depth: 0 # Full history
# OR
fetch-depth: 50 # Last 50 commits2. Concurrency Group Logic is ConfusingLocation: concurrency:
group: goose-pr-review-${{ github.event.issue.number || github.event.inputs.pr_number }}-${{ startsWith(github.event.comment.body || '', '/goose') }}
cancel-in-progress: trueProblem: Including Recommendation: Simplify to just the PR number: concurrency:
group: goose-pr-review-${{ github.event.issue.number || github.event.inputs.pr_number }}
cancel-in-progress: true3. No Timeout on Agent ExecutionLocation: goose run --recipe /tmp/recipe.yamlProblem: While the job has a 15-minute timeout (line 142), the goose command itself has no timeout. If goose hangs but keeps the process alive, it will run for the full 15 minutes. Recommendation: Add a timeout command: timeout 12m goose run --recipe /tmp/recipe.yaml || {
echo "Goose review timed out after 12 minutes"
exit 1
}4. Recipe Embedded in Workflow (Maintainability)Location: Problem: The entire goose recipe is embedded as a multi-line YAML string in the workflow. This makes it:
Recommendation: Extract to a separate file: # Create .github/recipes/pr-review.yaml
# Then in the workflow:
- name: Prepare recipe
run: |
envsubst < .github/recipes/pr-review.yaml > /tmp/recipe.yaml5. Inconsistent Model Defaults Across WorkflowsLocation: Problem: Different goose workflows use different default models:
Recommendation: Standardize on a single valid model across all workflows, or document why they differ. 🟢 Suggestions1. Consider Using GitHub Job SummariesLocation: Add a job summary for better visibility without cluttering PR comments: - name: Add job summary
if: steps.goose.outputs.has_review == 'true'
run: |
cat /tmp/pr_review.md >> $GITHUB_STEP_SUMMARY2. Add Review MetadataConsider adding metadata to the review comment: echo "<!-- goose-review: pr=$PR_NUM run=${{ github.run_id }} model=$GOOSE_MODEL -->" >> /tmp/pr_review.mdThis helps track which run produced which review and with what configuration. 3. Extract Instructions Parsing to Dedicated StepThe - name: Extract review instructions
uses: actions/github-script@v7
id: instructions
with:
result-encoding: string
script: |
const body = context.payload.comment?.body || '';
const match = body.match(/^\/goose\s+(.+)$/s);
return match ? match[1].trim() : 'No specific instructions - perform a general code review.';4. Consider Rate Limiting ProtectionIf this workflow is triggered frequently, consider adding a check to prevent API rate limit exhaustion: - name: Check rate limit
run: |
REMAINING=$(gh api /rate_limit --jq '.rate.remaining')
if [ "$REMAINING" -lt 100 ]; then
echo "Warning: Only $REMAINING API calls remaining"
fi✅ Highlights1. Security Warning AddedLocation: The addition of the security warning comment is excellent: # Security: PR content could prompt-inject the agent; only trigger on PRs you trust.This acknowledges the risk, though the underlying vulnerability still needs to be fixed. 2. Proper SHA Pinning for ActionsLocation: uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # pin@v4Using SHA pins instead of tags prevents supply chain attacks via tag manipulation. 3. Environment Variable Approach for PR_NUMLocation: The changes in this PR move env:
PR_NUM: ${{ steps.pr.outputs.number }}
run: |
gh pr comment "$PR_NUM" ...This is the right pattern and should be extended to other variables. 4. Proper Permission ScopingLocation: permissions:
contents: read
pull-requests: writeFollowing the principle of least privilege by only granting necessary permissions. 5. Author Association CheckLocation: contains(fromJSON('["OWNER", "MEMBER"]'), github.event.comment.author_association)Properly restricts who can trigger the workflow to prevent abuse. Review generated by goose |
|
/goose review this entire workflow file. Not just the little change here. I just made the change to enable this review process. I'm concerned about the whole file |
SummaryThis PR adds 🔴 Blocking Issues1. Reference to non-existent step output (.github/workflows/goose-pr-reviewer.yml:223, 231)Problem: Both lines reference # Line 223
PR_NUM: ${{ steps.pr.outputs.number }}
# Line 231
PR_NUM: ${{ steps.pr.outputs.number || github.event.issue.number || github.event.inputs.pr_number }}Impact: This will cause a workflow error since the output doesn't exist. Fix: Remove these PR_NUM declarations entirely, or if you need the PR number in an environment variable, use: PR_NUM: ${{ github.event.issue.number }}2. Reference to non-existent workflow input (.github/workflows/goose-pr-reviewer.yml:231)Problem: The fallback chain includes # Line 231
PR_NUM: ${{ steps.pr.outputs.number || github.event.issue.number || github.event.inputs.pr_number }}Impact: The Fix: Remove 🟡 Warnings3. Unused environment variable (.github/workflows/goose-pr-reviewer.yml:223, 231)Problem: # Line 224-225 - uses github.event.issue.number directly, not $PR_NUM
run: |
gh pr comment ${{ github.event.issue.number }} --repo ${{ github.repository }} --body-file /tmp/pr_review.mdImpact: No functional impact, but adds confusion and maintenance burden. Fix: Either:
4. Potential issues with PR_BODY in envsubst (.github/workflows/goose-pr-reviewer.yml:209)Problem: # Line 209
echo "$GOOSE_RECIPE" | envsubst '$PR_NUMBER $PR_TITLE $PR_BODY $REVIEW_INSTRUCTIONS' > /tmp/recipe.yamlContext: Looking at line 198, Impact: Could cause the recipe generation to fail or inject unintended variables if a PR body contains shell metacharacters. Fix: Consider whether PR_BODY is actually needed in the recipe. If so, pre-process it or use a different method to inject it (like writing it to a file and reading from there). 🟢 Suggestions5. Inconsistency with security comment (.github/workflows/goose-pr-reviewer.yml:21)Observation: The comment states "Do not add workflow_dispatch: API calls fetch mutable data, enabling TOCTOU attacks." However, Context: The TOCTOU concern is valid - when triggered via workflow_dispatch, the PR content could change between when the workflow is dispatched and when it runs. However, goose-issue-solver.yml shows it's possible to support workflow_dispatch safely for debugging/manual triggers. Suggestion: Either:
6. Timeout consideration (.github/workflows/goose-pr-reviewer.yml:135)Observation: This workflow has a 15-minute timeout while # Line 135
timeout-minutes: 15Context: Code reviews might need more time for large PRs with many files. Suggestion: Consider if 15 minutes is sufficient. If reviewers report timeouts, increase to 20-30 minutes. 7. Recipe description uses PR_NUMBER before it's definedMinor issue: Line 33 in the embedded recipe uses ✅ HighlightsExcellent security design
Good operational practices
Well-documented workflow
Review generated by goose |
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
|
This pull request has been automatically closed due to inactivity. Why was this closed?
Want to reopen?
Thank you for your contribution! We appreciate your effort. 🙏 |
1 participant
No description provided.