Repository navigation
feat: add fix agent for automated PR review remediation #337
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,8 +36,8 @@ func TestWorkflowsLayer_Install_WritesAllFiles(t *testing.T) { | |
| require.NoError(t, err) | ||
|
|
||
| // Should have created scaffold files + CODEOWNERS in the .fullsend repo | ||
| require.True(t, len(client.CreatedFiles) >= len(managedFiles), | ||
| "expected at least %d files (scaffold + CODEOWNERS), got %d", len(managedFiles), len(client.CreatedFiles)) | ||
|
|
||
| paths := make(map[string]string) // path -> content | ||
| for _, f := range client.CreatedFiles { | ||
|
|
@@ -49,6 +49,7 @@ func TestWorkflowsLayer_Install_WritesAllFiles(t *testing.T) { | |
| assert.Contains(t, paths, ".github/workflows/triage.yml") | ||
| assert.Contains(t, paths, ".github/workflows/code.yml") | ||
| assert.Contains(t, paths, ".github/workflows/review.yml") | ||
| assert.Contains(t, paths, ".github/workflows/fix.yml") | ||
| assert.Contains(t, paths, ".github/workflows/repo-maintenance.yml") | ||
| assert.Contains(t, paths, "CODEOWNERS") | ||
|
|
||
|
Member
There was a problem hiding this comment. The hardcoded require.True(t, len(client.CreatedFiles) >= 23,
"expected at least 23 files (22 scaffold + CODEOWNERS), got %d", len(client.CreatedFiles))With 10 new scaffold files the actual count is ~58, so
Contributor
Author
There was a problem hiding this comment.
|
||
|
|
@@ -111,8 +112,9 @@ func TestWorkflowsLayer_Install_CODEOWNERSOptional(t *testing.T) { | |
| // Install should succeed even though CODEOWNERS write failed | ||
| require.NoError(t, err) | ||
|
|
||
| // All scaffold files should have been created (CODEOWNERS excluded since it failed). | ||
| // Count = managedFiles - 1 (CODEOWNERS). | ||
| assert.Len(t, client.created, len(managedFiles)-1) | ||
| } | ||
|
|
||
| func TestWorkflowsLayer_Install_Error(t *testing.T) { | ||
|
|
@@ -160,7 +162,7 @@ func TestWorkflowsLayer_Analyze_AllPresent(t *testing.T) { | |
|
|
||
| assert.Equal(t, "workflows", report.Name) | ||
| assert.Equal(t, StatusInstalled, report.Status) | ||
| assert.Len(t, report.Details, len(managedFiles)) | ||
| } | ||
|
|
||
| func TestWorkflowsLayer_Analyze_NonePresent(t *testing.T) { | ||
|
|
@@ -174,7 +176,7 @@ func TestWorkflowsLayer_Analyze_NonePresent(t *testing.T) { | |
|
|
||
| assert.Equal(t, "workflows", report.Name) | ||
| assert.Equal(t, StatusNotInstalled, report.Status) | ||
| assert.Len(t, report.WouldInstall, len(managedFiles)) | ||
| } | ||
|
|
||
| func TestWorkflowsLayer_Analyze_Partial(t *testing.T) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,325 @@ | ||
| name: Fix | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| event_type: | ||
| required: true | ||
| type: string | ||
| source_repo: | ||
| required: true | ||
| type: string | ||
| event_payload: | ||
| required: true | ||
| type: string | ||
| trigger_source: | ||
| required: true | ||
| type: string | ||
| description: '"bot" (review agent) or "human" (/fix command)' | ||
| pr_number: | ||
| required: false | ||
| type: string | ||
| description: "PR number (workflow_dispatch manual fallback)" | ||
| instruction: | ||
| required: false | ||
| type: string | ||
| description: "Human instruction (workflow_dispatch manual fallback)" | ||
|
|
||
| # Single concurrency group per PR. A human /fix cancels any running fix | ||
| # (bot or human) so the human's instruction takes immediate effect. | ||
| # Bot-triggered runs also cancel previous bot runs on the same PR. | ||
| concurrency: | ||
| group: >- | ||
| fullsend-fix-${{ | ||
| fromJSON(inputs.event_payload).pull_request.number | ||
| || fromJSON(inputs.event_payload).issue.number | ||
| || inputs.pr_number | ||
| }} | ||
|
ascerra marked this conversation as resolved.
Resolved
|
||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| fix: | ||
| name: Fix | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| actions: write | ||
| contents: write | ||
| id-token: write | ||
| issues: write | ||
| packages: read | ||
| pull-requests: write | ||
|
|
||
| steps: | ||
| - name: Checkout .fullsend repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Validate source repo is enrolled | ||
| env: | ||
| SOURCE_REPO: ${{ inputs.source_repo }} | ||
| run: | | ||
| if [[ ! "$SOURCE_REPO" =~ ^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$ ]]; then | ||
| echo "::error::Invalid source_repo format: must be owner/repo" | ||
| exit 1 | ||
| fi | ||
| REPO_OWNER="${SOURCE_REPO%%/*}" | ||
| if [[ "$REPO_OWNER" != "$GITHUB_REPOSITORY_OWNER" ]]; then | ||
| echo "::error::source_repo owner does not match org" | ||
| exit 1 | ||
| fi | ||
| REPO_NAME="${SOURCE_REPO#*/}" | ||
| ENABLED=$(yq ".repos.\"$REPO_NAME\".enabled" config.yaml 2>/dev/null) | ||
| if [[ "$ENABLED" != "true" ]]; then | ||
| echo "::error::repo is not enabled in config.yaml" | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Extract target repo name | ||
| id: repo-parts | ||
| env: | ||
| SOURCE_REPO: ${{ inputs.source_repo }} | ||
| run: echo "name=${SOURCE_REPO##*/}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| - name: Generate sandbox token (read-only) | ||
| id: sandbox-token | ||
| uses: actions/create-github-app-token@v3 | ||
| with: | ||
| client-id: ${{ vars.FULLSEND_CODER_CLIENT_ID }} | ||
| private-key: ${{ secrets.FULLSEND_CODER_APP_PRIVATE_KEY }} | ||
| owner: ${{ github.repository_owner }} | ||
| repositories: ${{ steps.repo-parts.outputs.name }} | ||
| permission-contents: read | ||
| permission-issues: read | ||
| permission-pull-requests: read | ||
| permission-metadata: read | ||
|
|
||
| - name: Block fork PRs | ||
| env: | ||
| GH_TOKEN: ${{ steps.sandbox-token.outputs.token }} | ||
| EVENT_PAYLOAD: ${{ inputs.event_payload }} | ||
| SOURCE_REPO: ${{ inputs.source_repo }} | ||
| PR_NUM_INPUT: ${{ inputs.pr_number }} | ||
|
ralphbean marked this conversation as resolved.
Resolved
|
||
| run: | | ||
| # Fork PRs must not trigger the fix agent (security: prevents | ||
| # forked repos from running code with org-level tokens). | ||
| HEAD_REPO="$(echo "${EVENT_PAYLOAD}" | jq -r '.pull_request.head.repo.full_name // empty')" | ||
| BASE_REPO="$(echo "${EVENT_PAYLOAD}" | jq -r '.pull_request.base.repo.full_name // empty')" | ||
| if [[ -n "${HEAD_REPO}" && -n "${BASE_REPO}" && "${HEAD_REPO}" != "${BASE_REPO}" ]]; then | ||
| echo "::error::Fork PRs are not allowed to trigger the fix agent" | ||
| exit 1 | ||
| fi | ||
| # Fallback for issue_comment events (payload has .issue, not .pull_request). | ||
| if [[ -z "${HEAD_REPO}" && -z "${BASE_REPO}" ]]; then | ||
| PR_NUM="$(echo "${EVENT_PAYLOAD}" | jq -r '.issue.number // empty')" | ||
| PR_NUM="${PR_NUM:-${PR_NUM_INPUT:-}}" | ||
| if [[ -n "${PR_NUM}" ]]; then | ||
| IS_FORK="$(gh pr view "${PR_NUM}" --repo "${SOURCE_REPO}" \ | ||
| --json isCrossRepository --jq '.isCrossRepository' 2>/dev/null || echo "false")" | ||
| if [[ "${IS_FORK}" == "true" ]]; then | ||
| echo "::error::Fork PRs are not allowed to trigger the fix agent" | ||
| exit 1 | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| - name: Extract PR number and context | ||
| id: context | ||
| env: | ||
| GH_TOKEN: ${{ steps.sandbox-token.outputs.token }} | ||
| EVENT_PAYLOAD: ${{ inputs.event_payload }} | ||
| INPUT_PR_NUMBER: ${{ inputs.pr_number }} | ||
| INPUT_INSTRUCTION: ${{ inputs.instruction }} | ||
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| SOURCE_REPO: ${{ inputs.source_repo }} | ||
| run: | | ||
| # Extract PR number from event payload or workflow_dispatch input. | ||
| PR_NUM="$(echo "${EVENT_PAYLOAD}" | jq -r '.pull_request.number // empty')" | ||
| if [[ -z "${PR_NUM}" ]]; then | ||
| PR_NUM="$(echo "${EVENT_PAYLOAD}" | jq -r '.issue.number // empty')" | ||
| fi | ||
| if [[ -z "${PR_NUM}" && -n "${INPUT_PR_NUMBER}" ]]; then | ||
| PR_NUM="${INPUT_PR_NUMBER}" | ||
| fi | ||
| if [[ -z "${PR_NUM}" || ! "${PR_NUM}" =~ ^[1-9][0-9]*$ ]]; then | ||
| echo "::error::Could not extract valid PR number from event" | ||
| exit 1 | ||
| fi | ||
| echo "pr_number=${PR_NUM}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| # Resolve the PR head branch. For pull_request_review events the | ||
| # payload has .pull_request.head.ref. For /fix comment events | ||
| # (issue_comment) it does not — query the GitHub API instead. | ||
| HEAD_REF="$(echo "${EVENT_PAYLOAD}" | jq -r '.pull_request.head.ref // empty')" | ||
| BASE_REF="$(echo "${EVENT_PAYLOAD}" | jq -r '.pull_request.base.ref // empty')" | ||
| if [[ -z "${HEAD_REF}" ]]; then | ||
| HEAD_REF="$(gh pr view "${PR_NUM}" --repo "${SOURCE_REPO}" --json headRefName --jq '.headRefName' 2>/dev/null || true)" | ||
| fi | ||
| if [[ -z "${BASE_REF}" ]]; then | ||
| BASE_REF="$(gh pr view "${PR_NUM}" --repo "${SOURCE_REPO}" --json baseRefName --jq '.baseRefName' 2>/dev/null || echo 'main')" | ||
| fi | ||
| if [[ -z "${HEAD_REF}" ]]; then | ||
| echo "::error::Could not resolve PR head branch" | ||
| exit 1 | ||
| fi | ||
| echo "head_ref=${HEAD_REF}" >> "${GITHUB_OUTPUT}" | ||
| echo "base_ref=${BASE_REF}" >> "${GITHUB_OUTPUT}" | ||
| echo "PR #${PR_NUM}: ${HEAD_REF} → ${BASE_REF}" | ||
|
|
||
| # Extract human instruction (from /fix comment or workflow_dispatch). | ||
| # Default to "none" so the env var is always non-empty (the fullsend | ||
| # binary rejects empty runner_env values). The agent checks | ||
| # TRIGGER_SOURCE before reading this value. | ||
| INSTRUCTION="none" | ||
| if [[ "${TRIGGER_SOURCE}" == "human" ]]; then | ||
| COMMENT_BODY="$(echo "${EVENT_PAYLOAD}" | jq -r '.comment.body // empty')" | ||
| if [[ -n "${COMMENT_BODY}" ]]; then | ||
| # Strip the /fix prefix to get the instruction. | ||
| INSTRUCTION="$(echo "${COMMENT_BODY}" | sed 's|^/fix[[:space:]]*||')" | ||
| fi | ||
| if [[ -z "${INSTRUCTION}" && -n "${INPUT_INSTRUCTION}" ]]; then | ||
| INSTRUCTION="${INPUT_INSTRUCTION}" | ||
| fi | ||
| if [[ -z "${INSTRUCTION}" ]]; then | ||
| INSTRUCTION="none" | ||
| fi | ||
| fi | ||
| # Use a random heredoc delimiter so a /fix comment containing | ||
| # the literal delimiter string cannot inject GITHUB_OUTPUT values. | ||
| DELIM="INSTRUCTION_$(openssl rand -hex 8)" | ||
| { | ||
| echo "instruction<<${DELIM}" | ||
| echo "${INSTRUCTION}" | ||
| echo "${DELIM}" | ||
| } >> "${GITHUB_OUTPUT}" | ||
|
|
||
| # Count previous fix-agent commits on this PR to determine iteration. | ||
| # Uses the API (no checkout needed) to count commits authored by the | ||
| # fix agent identity. Falls back to ITERATION_CAP if the query fails. | ||
| # Note: TOCTOU — another run could push between this count and the | ||
| # agent execution, allowing at most +1 overshoot. The concurrency | ||
| # group's cancel-in-progress mostly prevents this. | ||
| FIX_COMMITS="$(gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/commits" \ | ||
| --paginate --jq '[.[] | select(.commit.author.name == "fullsend-fix")] | length' 2>/dev/null)" \ | ||
| || { echo "::warning::Could not count prior fix commits — defaulting to cap"; FIX_COMMITS="${ITERATION_CAP:-5}"; } | ||
| ITERATION=$(( FIX_COMMITS + 1 )) | ||
| echo "Fix iteration: ${ITERATION} (${FIX_COMMITS} previous fix commits)" >&2 | ||
| echo "iteration=${ITERATION}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| - name: Generate push token (write) | ||
| id: push-token | ||
| uses: actions/create-github-app-token@v3 | ||
| with: | ||
| client-id: ${{ vars.FULLSEND_CODER_CLIENT_ID }} | ||
| private-key: ${{ secrets.FULLSEND_CODER_APP_PRIVATE_KEY }} | ||
| owner: ${{ github.repository_owner }} | ||
| repositories: ${{ steps.repo-parts.outputs.name }} | ||
| permission-contents: write | ||
| permission-pull-requests: write | ||
|
|
||
| - name: Checkout target repository at PR HEAD | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: ${{ inputs.source_repo }} | ||
| token: ${{ steps.sandbox-token.outputs.token }} | ||
| path: target-repo | ||
| fetch-depth: 0 | ||
| persist-credentials: false | ||
| ref: ${{ steps.context.outputs.head_ref }} | ||
|
|
||
| - name: Record pre-agent HEAD | ||
| id: pre-agent | ||
| working-directory: target-repo | ||
| run: | | ||
| SHA="$(git rev-parse HEAD)" | ||
| echo "Pre-agent HEAD: ${SHA}" | ||
| echo "head=${SHA}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| - name: Pre-fetch review body | ||
| id: review-body | ||
| env: | ||
| GH_TOKEN: ${{ steps.sandbox-token.outputs.token }} | ||
| SOURCE_REPO: ${{ inputs.source_repo }} | ||
| PR_NUM: ${{ steps.context.outputs.pr_number }} | ||
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| ORG_NAME: ${{ github.repository_owner }} | ||
| run: | | ||
| REVIEW_FILE="${GITHUB_WORKSPACE}/review-body.txt" | ||
| REVIEW_BOT="${ORG_NAME}-review[bot]" | ||
| gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/reviews" \ | ||
| --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and .user.login == \"${REVIEW_BOT}\")] | last | .body // \"\"" \ | ||
| > "${REVIEW_FILE}" 2>/dev/null || echo "" > "${REVIEW_FILE}" | ||
| BYTE_COUNT="$(wc -c < "${REVIEW_FILE}")" | ||
| echo "Pre-fetched review body: ${BYTE_COUNT} bytes" | ||
| MAX_REVIEW_BYTES=1048576 # 1 MB | ||
| if [[ "${BYTE_COUNT}" -gt "${MAX_REVIEW_BYTES}" ]]; then | ||
| echo "::error::Review body is ${BYTE_COUNT} bytes (max: ${MAX_REVIEW_BYTES})" | ||
| exit 1 | ||
| fi | ||
| if [[ "${TRIGGER_SOURCE}" == "bot" && "${BYTE_COUNT}" -le 1 ]]; then | ||
| echo "::error::Bot-triggered run but review body is empty — nothing to fix" | ||
| exit 1 | ||
|
Comment on lines
+257
to
+259
Member
There was a problem hiding this comment. Medium: missing review body length cap The
Suggest adding a max size check after the existing MAX_REVIEW_BYTES=1048576 # 1MB
if [[ "${BYTE_COUNT}" -gt "${MAX_REVIEW_BYTES}" ]]; then
echo "::error::Review body is ${BYTE_COUNT} bytes (max: ${MAX_REVIEW_BYTES})"
exit 1
fiThe review agent's structured output is schema-validated with
Contributor
Author
There was a problem hiding this comment. Fixed — added a 1 MB cap ( |
||
| fi | ||
| echo "review_file=${REVIEW_FILE}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| - name: Validate inputs | ||
| env: | ||
| PR_NUMBER: ${{ steps.context.outputs.pr_number }} | ||
| REPO_FULL_NAME: ${{ inputs.source_repo }} | ||
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| FIX_ITERATION: ${{ steps.context.outputs.iteration }} | ||
| HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} | ||
| run: bash scripts/pre-fix.sh | ||
|
|
||
| - name: Authenticate to Google Cloud (WIF) | ||
| if: vars.FULLSEND_GCP_AUTH_MODE == 'wif' | ||
| uses: google-github-actions/auth@v3 | ||
| with: | ||
| workload_identity_provider: ${{ secrets.FULLSEND_GCP_WIF_PROVIDER }} | ||
| service_account: ${{ secrets.FULLSEND_GCP_WIF_SA_EMAIL }} | ||
|
|
||
| - name: Authenticate to Google Cloud (SA key) | ||
| if: vars.FULLSEND_GCP_AUTH_MODE != 'wif' | ||
| uses: google-github-actions/auth@v3 | ||
|
Member
There was a problem hiding this comment. [important] This workflow only has SA key auth, but all other agent workflows (
See PR #448 diff (commits
Contributor
Author
There was a problem hiding this comment.
Pattern matches |
||
| with: | ||
| credentials_json: ${{ secrets.FULLSEND_GCP_SA_KEY_JSON }} | ||
|
|
||
| - name: Mask GCP credential file paths | ||
| run: | | ||
| for var in GOOGLE_GHA_CREDS_PATH GOOGLE_APPLICATION_CREDENTIALS CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE; do | ||
| val="${!var:-}" | ||
| if [[ -n "${val}" ]]; then | ||
| echo "::add-mask::${val}" | ||
| fi | ||
| done | ||
|
|
||
| - name: Prepare sandbox credentials | ||
| run: bash scripts/prepare-sandbox-credentials.sh | ||
|
|
||
| - name: Setup agent environment | ||
| env: | ||
| AGENT_PREFIX: FIX_ | ||
| FIX_GH_TOKEN: ${{ steps.sandbox-token.outputs.token }} | ||
| FIX_TARGET_REPO_DIR: target-repo | ||
| FIX_ANTHROPIC_VERTEX_PROJECT_ID: ${{ secrets.FULLSEND_GCP_PROJECT_ID }} | ||
| FIX_CLOUD_ML_REGION: ${{ vars.FULLSEND_GCP_REGION }} | ||
| FIX_PR_NUMBER: ${{ steps.context.outputs.pr_number }} | ||
| FIX_TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| FIX_HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} | ||
| FIX_FIX_ITERATION: ${{ steps.context.outputs.iteration }} | ||
| FIX_REPO_FULL_NAME: ${{ inputs.source_repo }} | ||
| run: bash .github/scripts/setup-agent-env.sh | ||
|
|
||
| - name: Run fix agent | ||
| uses: ./.github/actions/fullsend | ||
| env: | ||
| PR_NUMBER: ${{ steps.context.outputs.pr_number }} | ||
| REPO_FULL_NAME: ${{ inputs.source_repo }} | ||
| PUSH_TOKEN: ${{ steps.push-token.outputs.token }} | ||
| PUSH_TOKEN_SOURCE: github-app | ||
| TARGET_BRANCH: ${{ steps.context.outputs.base_ref }} | ||
| TRIGGER_SOURCE: ${{ inputs.trigger_source }} | ||
| HUMAN_INSTRUCTION: ${{ steps.context.outputs.instruction }} | ||
| FIX_ITERATION: ${{ steps.context.outputs.iteration }} | ||
| REVIEW_BODY_FILE: ${{ steps.review-body.outputs.review_file }} | ||
| PRE_AGENT_HEAD: ${{ steps.pre-agent.outputs.head }} | ||
| with: | ||
| agent: fix | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.