Don't fail safe_outputs job when a resolve-review-thread ID hits an unrelated repo - #53124
Conversation
|
Hi Since this PR is still in draft with no code changes yet, here are the things to watch for as the implementation progresses:
The PR is properly on-topic and follows the core-team contribution process. Once code is added, we can assess focus and completeness more fully. Looking forward to seeing the fix!
|
…_outputs job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ropped Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed PR #53124 diff for over-engineering; changes are minimal and targeted (added skipped flag, warning message tweak, one prompt-hardening line, matching test updates). Nothing to cut.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
Non-blocking from this pass: the changed lines look internally consistent, and the new skipped-vs-fatal behavior is covered by targeted tests.
Highlights
- The handler still fails closed by refusing the cross-repo resolution, but now marks the legacy-mode mismatch as skipped so one bad thread ID does not sink the entire safe_outputs job.
- The manager test covers the important accounting edge case: skipped thread-resolution results are excluded from fatal failures while still counting toward skipped item status.
- I did not find a changed-line regression that clearly warrants blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.81 AIC · ⌖ 5.44 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Prevents unrelated review-thread IDs from failing the entire safe_outputs job while preserving repository access restrictions.
Changes:
- Treats legacy-mode repository mismatches as skipped operations.
- Hardens PR Sous Chef instructions against fabricated thread IDs.
- Adds coverage for handler and failure-partitioning behavior.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/resolve_pr_review_thread.cjs |
Skips disallowed legacy-mode thread IDs. |
actions/setup/js/resolve_pr_review_thread.test.cjs |
Tests the new skip behavior. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Verifies skipped results remain non-fatal. |
.github/workflows/pr-sous-chef.md |
Requires verbatim thread IDs. |
.github/workflows/pr-sous-chef.lock.yml |
Updates the compiled workflow metadata hash. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is targeted, well-reasoned, and backed by good tests. Approving with one minor observation.
📋 Key Themes & Highlights
Key Themes
- Root cause vs. symptom: The fix correctly identifies that a legacy-mode repo mismatch almost always means a hallucinated/stale ID, not a genuine cross-repo access attempt. Downgrading to
skippedrather than fatal is the right call. - Fail-closed preserved: Explicit cross-repo misconfigurations (via
target/allowed-repos) still fail fatally — the safety boundary is maintained. - Test coverage: Both unit files are updated: the handler test asserts
skipped: true, and the manager test confirmsskippedresults flow intoitemsSkippedrather thanfatalFailures.
Positive Highlights
- ✅
isFailedProcessingResultalready gates onclassifySafeOutputResult === "failed", soskipped: trueresults are automatically excluded fromfatalFailureswithout a special-case inpartitionFailureResults. - ✅ Prompt hardening instruction is precise and actionable.
- ✅ Test names read as specifications.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.4 AIC · ⌖ 8.36 AIC · ⊞ 7.7K
Comment /matt to run again
There was a problem hiding this comment.
The fix is correct and well-tested.
The skipped: true flag on the legacy cross-repo result correctly routes through classifySafeOutputResult → "skipped" → excluded from partitionFailureResults fatal failures. Tests cover both the handler-level and manager-level behavior. The companion workflow instruction to copy thread IDs verbatim is a sensible preventive addition.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.6 AIC · ⌖ 7.94 AIC · ⊞ 5.6K
|
@copilot PR #53124 looks ready for a final pass. Quick triage
Next step
|
|
@copilot PR #53124 looks ready for a final pass. Quick triage
Next step
Run: https://github.com/github/gh-aw/actions/runs/31953024314
|
|
@copilot PR #53124 looks ready for a final pass. Quick triage
Next step
Run: https://github.com/github/gh-aw/actions/runs/31953623248
|
|
🎉 This pull request is included in a new release. Release: |
The
safe_outputsjob in PR Sous Chef failed because aresolve_pull_request_review_threadcall was made with a thread ID that resolved to an unrelated repository (juju/juju), tripping the allowed-repos check. Since the workflow never configured cross-repo resolution, this was almost certainly a stale or fabricated GraphQL node ID — but the handler treated the mismatch as a fatal error, failing the entire job even though four other safe outputs in the same run had already succeeded.Handler behavior
resolve_pr_review_thread.cjs: in legacy (default, non-cross-repo) mode, a thread resolving to a disallowed repo is now markedskipped: trueinstead of a hard failure. The action still isn't performed (fail-closed is preserved), but a single bad ID no longer takes down the wholesafe_outputsjob — same treatment as already-resolved/stale threads. Explicit cross-repo misconfigurations (viatarget/allowed-repos) still fail fatally, since those indicate real setup errors.Prompt hardening
pr-sous-chef.md: instructions now explicitly require copyingPRRT_...thread IDs verbatim fromreviewThreadsoutput rather than guessing, truncating, or extending them, to reduce the chance of malformed IDs being submitted in the first place.Tests
resolve_pr_review_thread.test.cjsto assert the legacy cross-repo mismatch case is now skipped rather than fatal.safe_output_handler_manager.test.cjsconfirming a skippedresolve_pull_request_review_threadresult is excluded fromfatalFailureswhile still being counted in the overall skip/failure status (not silently dropped).Run: https://github.com/github/gh-aw/actions/runs/31950301208> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.5 AIC · ⌖ 5.77 AIC · ⊞ 8.7K · ◷