Conversation
malithsen
requested review from
a team,
daledupreez and
wjrosa
and removed request for
a team
wjrosa
left a comment
wjrosa
left a comment
There was a problem hiding this comment.
Awesome idea! Looks good to me 👍
daledupreez
left a comment
daledupreez
left a comment
There was a problem hiding this comment.
This is looking good to me, and I think is totally worth trying!
I have some fairly minor comments and suggestions, but none of them are blocking.
Once we get things working, it may be worth splitting up the code into separate composable actions with clear inputs and outputs, as there is a lot going on across all the steps and common state.
| # Step 2: Check for existing fix PR & extract logs | ||
| # ────────────────────────────────────────────── | ||
| - name: Check for existing fix PR | ||
| if: steps.resolve_pr.outputs.found == 'true' && steps.resolve_pr.outputs.is_fork != 'true' |
There was a problem hiding this comment.
Nit: Why not check that is_fork == 'false'? I think it makes what we are trying to find a bit clearer.
| if: steps.resolve_pr.outputs.found == 'true' && steps.resolve_pr.outputs.is_fork != 'true' | |
| if: steps.resolve_pr.outputs.found == 'true' && steps.resolve_pr.outputs.is_fork == 'false' |
| const failedJobs = jobs.data.jobs.filter(j => j.conclusion === 'failure'); | ||
| const failedJobNames = failedJobs.map(j => j.name); |
There was a problem hiding this comment.
Nit: can we use job in the filters?
| const failedJobs = jobs.data.jobs.filter(j => j.conclusion === 'failure'); | |
| const failedJobNames = failedJobs.map(j => j.name); | |
| const failedJobs = jobs.data.jobs.filter(job => job.conclusion === 'failure'); | |
| const failedJobNames = failedJobs.map(job => job.name); |
| // Download logs for each failed job (truncated to last 200 lines each, max 3 jobs) | ||
| let allLogs = ''; | ||
| for (const job of failedJobs.slice(0, 3)) { |
There was a problem hiding this comment.
Out of interest, why only 3 jobs and why 200 lines?
| }); | ||
| const logLines = log.data.split('\n'); | ||
| const truncated = logLines.slice(-200).join('\n'); | ||
| allLogs += `\n--- Job: ${job.name} ---\n${truncated}\n`; |
There was a problem hiding this comment.
Might it be worth adding more explicit separators between jobs?
| allLogs += `\n--- Job: ${job.name} ---\n${truncated}\n`; | |
| allLogs += `\n--- Job: ${job.name} ---\n${truncated}\n--- /end job ${job.name} ---\n`; |
| steps.resolve_pr.outputs.is_fork != 'true' && | ||
| steps.check_existing.outputs.exists != 'true' | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ steps.resolve_pr.outputs.pr_branch }} | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Analyze test failure (Phase 1) | ||
| if: > | ||
| steps.resolve_pr.outputs.found == 'true' && | ||
| steps.resolve_pr.outputs.is_fork != 'true' && | ||
| steps.check_existing.outputs.exists != 'true' |
There was a problem hiding this comment.
As noted earlier, might it make sense to check for == 'false' rather than != 'true'?
| steps.resolve_pr.outputs.is_fork != 'true' && | |
| steps.check_existing.outputs.exists != 'true' | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ steps.resolve_pr.outputs.pr_branch }} | |
| fetch-depth: 0 | |
| - name: Analyze test failure (Phase 1) | |
| if: > | |
| steps.resolve_pr.outputs.found == 'true' && | |
| steps.resolve_pr.outputs.is_fork != 'true' && | |
| steps.check_existing.outputs.exists != 'true' | |
| steps.resolve_pr.outputs.is_fork == 'false' && | |
| steps.check_existing.outputs.exists == 'false' | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ steps.resolve_pr.outputs.pr_branch }} | |
| fetch-depth: 0 | |
| - name: Analyze test failure (Phase 1) | |
| if: > | |
| steps.resolve_pr.outputs.found == 'true' && | |
| steps.resolve_pr.outputs.is_fork == 'false' && | |
| steps.check_existing.outputs.exists == 'false' |
| ## Run URL: ${{ steps.logs.outputs.run_url }} | ||
|
|
||
| ## Error Logs: | ||
| ${{ steps.logs.outputs.logs }} |
There was a problem hiding this comment.
Might it be worth adding a ``` boundary here to explicitly wrap the log content in a markdown-native way?
| ${{ steps.logs.outputs.logs }} | |
| ``` | |
| ${{ steps.logs.outputs.logs }} | |
| ``` |
|
|
||
| if (analysis.affected_files && analysis.affected_files.length > 0) { | ||
| body += `**Affected files:**\n`; | ||
| analysis.affected_files.forEach(f => { body += `- \`${f}\`\n`; }); |
There was a problem hiding this comment.
To match the other code that is body-oriented, we could switch this around to use map() and join(). (Not at all blocking.)
| analysis.affected_files.forEach(f => { body += `- \`${f}\`\n`; }); | |
| body += analysis.affected_files.map(filename => `- \`${f}\``).join( '\n' ); |
|
|
||
| ## Failed Workflow: ${{ steps.logs.outputs.workflow_name }} | ||
| ## Error Logs: | ||
| ${{ steps.logs.outputs.logs }} |
There was a problem hiding this comment.
As above RE wrapping the log output in ```.
| ${{ steps.logs.outputs.logs }} | |
| ``` | |
| ${{ steps.logs.outputs.logs }} | |
| ``` |
| Fixes failing tests from [${{ steps.logs.outputs.workflow_name }}](${{ steps.logs.outputs.run_url }}) on PR #${{ steps.resolve_pr.outputs.pr_number }}. | ||
|
|
||
| --- | ||
| *Auto-generated by Claude Code*" |
There was a problem hiding this comment.
Maybe mention the workflow here?
| *Auto-generated by Claude Code*" | |
| *Auto-generated by Claude Code via the claude-fix-tests workflow*" |
3 participants
Changes proposed in this Pull Request:
This is an experiment to use Claude Code Action to automatically analyze test failures on PRs and open fix PRs when the failure is due to an issue in the test iteself (eg: #5005)
How it works:
workflow_runwhen PHP tests, JS tests, or E2E tests workflows fail on a PRtest_drift,test_bug,application_bug, orenvironmentusing structured JSON output, then posts an analysis comment on the PRtest_driftortest_bugwith high/medium confidence, Claude creates a separate fix PR targeting the original PR's branchSome guardrails:
claude/branches to avoid potential loops.ghandgitcommandsTesting instructions
Primarily looking for a code-review and feedback on the approach. We'd need to merge it to
developto actually test it.I've tested a slightly modified version of this workflow in fork of the repo by opening a PR that introducing a failing PR.
PR: malithsen#2
Automated fix: malithsen#3
This test doesn't perfectly capture the intended scenario, but it does validate the end-to-end flow failure detection, analysis, comment posting, and fix PR creation.
Changelog entry
Changelog Entry Comment
Comment
Post merge