Repository navigation
Fix slash-command PR prefetch caching and error handling - #63678
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number }} | ||
| PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number }} |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The dedicated cache producer still suppresses diff failures, and slash-command cache keys cannot be refreshed after a head change.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Hardens PR-review prefetching for centralized slash commands and refreshes generated workflows.
Changes:
- Resolves PR numbers from
aw_contextand updates cache keys. - Refreshes review comments and surfaces diff-fetch failures.
- Corrects Matt Pocock skill discovery and adds contract coverage.
| File | Description |
|---|---|
pkg/cli/pr_prefetch_cache_contract_test.go |
Extends prefetch contract assertions. |
.github/workflows/shared/pr-diff-data-fetch.md |
Updates PR resolution, cache validation, and fetching. |
.github/workflows/pr-code-quality-reviewer.md |
Adds slash-command cache fallback. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Regenerates compiled workflow. |
.github/workflows/ponytail-reviewer.md |
Adds slash-command cache fallback. |
.github/workflows/ponytail-reviewer.lock.yml |
Regenerates compiled workflow. |
.github/workflows/mattpocock-skills-reviewer.md |
Fixes caching and skill discovery. |
.github/workflows/mattpocock-skills-reviewer.lock.yml |
Regenerates compiled workflow. |
.github/workflows/impeccable-skills-reviewer.lock.yml |
Incorporates shared prefetch changes. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if [ -n "$CURRENT_HEAD_SHA" ] && [ "$CURRENT_HEAD_SHA" = "$CACHE_HEAD_SHA" ] && [ -f /tmp/gh-aw/agent/pr-diff.patch ] && [ -f /tmp/gh-aw/agent/pr-meta.json ] && [ -f /tmp/gh-aw/agent/pr-review-comments.json ]; then | ||
| fetch_review_comments | ||
| # Skip diff and metadata fetch only when cache data matches current PR head commit. | ||
| if [ -n "$CURRENT_HEAD_SHA" ] && [ "$CURRENT_HEAD_SHA" = "$CACHE_HEAD_SHA" ] && [ -f /tmp/gh-aw/agent/pr-diff.patch ] && [ -f /tmp/gh-aw/agent/pr-meta.json ]; then |
| --- | ||
| cache: | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number }} | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number }} |
| - DietrichGebert/ponytail/skills/ponytail-review@2ed6c52c9d7e5e56942508591085fd45dea277d3 | ||
| cache: | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number }} | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number }} |
| memory-id: pr-code-quality-reviewer | ||
| cache: | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number }} | ||
| key: pr-prefetch-${{ github.event.pull_request.head.sha || github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number }} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness regression before merge.
📋 Key Themes & Highlights
Key Themes
- Missing
item_typeguard onaw_contextfallback (.github/workflows/shared/pr-diff-data-fetch.md:26): per ADR 31820 (docs/adr/31820-aw-context-fallback-for-injected-github-routing-context.md), anyaw_context-derived entity number MUST be guarded byitem_type == 'pull_request'. The newPR_NUMBERfallback here omits that guard, so an issue-routed slash-command run could resolvePR_NUMBERfrom an unrelateditem_number. This is the same root cause an automated security scanner already flagged inline on the affected line. - Brittle test literal (
pkg/cli/pr_prefetch_cache_contract_test.go:32): the new assertion pins an exact multi-line whitespace/comment string rather than the underlying "review comments refresh before cache check" behavior, making it fragile to unrelated formatting changes.
Positive Highlights
- ✅
gh pr difffailures are now correctly captured viaDIFF_EXITand abort the step instead of silently producing an empty patch (|| trueremoved). - ✅ Review comments are now refreshed on every run instead of being gated by the diff/metadata cache, fixing stale-duplicate-detection risk.
- ✅ Regression tests were extended to cover the new fallback, error path, and skill discovery path changes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 159.7 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number }} | ||
| PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number }} |
There was a problem hiding this comment.
[/diagnosing-bugs] The aw_context fallback for PR_NUMBER is missing the item_type == 'pull_request' guard required by ADR 31820 (docs/adr/31820-aw-context-fallback-for-injected-github-routing-context.md). Without it, an issue-routed slash-command run (where aw_context.item_type == 'issue') could still populate PR_NUMBER from item_number, causing this step to fetch/comment on the wrong PR.
💡 Fix
Guard the fallback the same way the compiler does elsewhere in this repo (see .github/workflows/copilot-opt.lock.yml):
PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number || (fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}') .item_type == 'pull_request' && fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number) }}The existing security-bot comment on this line flags the same root cause; the current PR fixes the missing-PR-number bug but reintroduces a related correctness risk for mixed-entity slash-command routing.
@copilot please address this.
| assert.Contains(t, text, "Cache hit: using pre-fetched PR data for head", "Shared PR prefetch should verify cache by current head SHA") | ||
| assert.Contains(t, text, "fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number", "Shared PR prefetch should resolve centralized slash-command PR numbers") | ||
| assert.Contains(t, text, "::error::Unable to determine the pull request number from the event context.", "Shared PR prefetch should reject missing PR context") | ||
| assert.Contains(t, text, "\n fetch_review_comments\n # Skip diff and metadata fetch", "Shared PR prefetch should refresh review comments before checking cached diff data") |
There was a problem hiding this comment.
[/tdd] This assertion pins the exact literal "\n fetch_review_comments\n # Skip diff and metadata fetch", coupling the test to indentation and a comment string rather than the behavior (refresh-before-cache-check ordering).
💡 Suggested improvement
Any reformatting of pr-diff-data-fetch.md (e.g. reindent, reword the comment) will silently pass or fail this test for unrelated reasons, and a future refactor that keeps the ordering correct but renames the comment will falsely fail CI. Consider asserting on the relative position of fetch_review_comments vs. the cache-hit if condition using strings.Index, independent of exact whitespace/comment text:
fetchIdx := strings.Index(text, "fetch_review_comments\n")
cacheCheckIdx := strings.Index(text, `if [ -n "$CURRENT_HEAD_SHA" ]`)
require.True(t, fetchIdx >= 0 && cacheCheckIdx >= 0)
assert.Less(t, fetchIdx, cacheCheckIdx, "review comments must be refreshed before the cache-hit check")@copilot please address this.
|
@copilot There is still forward progress needed on this PR. Please address the newest unresolved review feedback first:
After updating, please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
🎉 This pull request is included in a new release. Release: |
Centralized slash-command runs could lose the target PR number, reuse stale review comments, or silently interpret a failed diff fetch as an empty review. The Matt Pocock reviewer also searched the wrong skill installation path.
Changes
aw_context.item_numberwhen standard event fields are absent.Cache correctness
Failure handling
gh pr diffexit status.Skill discovery
${GITHUB_WORKSPACE}/.github/skills.Regression coverage