Fix inline review comments on centralized reviewer reruns - #52960
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work on the fix! 🎉 This PR addresses a regression where centralized slash-command dispatches were losing inline comment context—the inline-comment handler was inspecting the raw event instead of the reconstructed pull request payload. What looks good:
This is ready for maintainer review. Once the draft status is lifted and any final checks pass, it's in good shape for merge.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Diff is a straightforward fix (adds effectivePayload/effectiveEventName resolution via existing resolveInvocationContext helper, updates references, adds one focused regression test). No speculative abstractions, dead flexibility, or reinvented stdlib found.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52960 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly applies resolveInvocationContext to derive effectiveEventName and effectivePayload, enabling inline review comments to work during centralized reviewer reruns (workflow_dispatch). Fallbacks to the raw context values preserve existing behavior. The new test covers the dispatch scenario end-to-end. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.7 AIC · ⌖ 7.86 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Pull request overview
Fixes inline review comments during centralized reviewer reruns by reconstructing the original pull request context.
Changes:
- Resolves effective event and payload data for dispatched workflows.
- Uses reconstructed context for PR targeting and footer metadata.
- Adds regression coverage for centralized slash-command dispatches.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_pr_review_comment.cjs |
Applies reconstructed invocation context to review comments. |
actions/setup/js/create_pr_review_comment.test.cjs |
Tests dispatched PR-context handling. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two observations worth addressing before merge, otherwise LGTM.
📋 Key Themes & Highlights
Key Themes
- Missing fallback-path test: the new test proves
workflow_dispatchworks but doesn't cover the case whereresolveInvocationContextreturns{}, leaving the||guard untested. - No error boundary on
resolveInvocationContext: a malformedevent_payloadJSON inworkflow_dispatchinputs would throw synchronously and abortmain()before processing any message.
Positive Highlights
- ✅ Root cause properly addressed — all six
context.eventName/context.payloadcall-sites replaced consistently. - ✅ Regression test added in the same PR with a realistic
workflow_dispatchpayload shape. - ✅ Minimal diff; no unrelated churn.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.8 AIC · ⌖ 8.58 AIC · ⊞ 7.7K
Comment /matt to run again
| }); | ||
| expect(buffer.getBufferedCount()).toBe(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The new test covers the workflow_dispatch happy path but not the fallback: if resolveInvocationContext returns {}, the code should fall back to raw context.eventName/context.payload. A test for that case would pin the || guard behaviour and prevent a silent regression if invocation_context_helpers changes its return shape.
💡 Suggested additional test skeleton
it("falls back to raw context when invocationContext is empty", async () => {
// resolveInvocationContext returns no overrides
jest.mock("./invocation_context_helpers.cjs", () => ({
resolveInvocationContext: () => ({}),
}));
global.context = mockContext; // standard PR trigger
const handler = await createHandler({ target: "triggering" });
const result = await handler(prReviewMessage, {});
expect(result.success).toBe(true);
});@copilot please address this.
There was a problem hiding this comment.
Addressed in ec00158 by adding regression coverage for fallback behavior when invocation context resolution yields empty/partial context (raw context fallback path is now explicitly tested).
| @@ -38,6 +39,9 @@ async function main(config = {}) { | |||
| const legacyBuffer = registry ? null : config._prReviewBuffer || null; | |||
| const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); | |||
| const githubClient = await createAuthenticatedGitHubClient(config); | |||
There was a problem hiding this comment.
[/diagnosing-bugs] effectivePayload is derived once at startup (line 41) before the per-message loop, but the invocation context is static anyway so this is fine. However, if resolveInvocationContext ever throws (e.g. malformed event_payload JSON), the entire main() will reject before processing any message. Consider wrapping the call in a try/catch and falling back to raw context, so a bad dispatch input degrades gracefully rather than taking down the whole handler.
💡 Defensive pattern
let invocationContext = {};
try {
invocationContext = resolveInvocationContext(context);
} catch (e) {
core.warning(`resolveInvocationContext failed, using raw context: ${e.message}`);
}
const effectiveEventName = invocationContext.eventName || context.eventName;
const effectivePayload = invocationContext.eventPayload || context.payload;@copilot please address this.
There was a problem hiding this comment.
Addressed in ec00158 by adding defensive handling around invocation-context resolution: non-validation failures now fall back to raw context, while ERR_VALIDATION failures are still re-thrown.
|
@copilot run pr-finisher skill |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Matt reviewer reruns submitted the overall review but dropped inline comments. Centralized slash commands execute through
workflow_dispatch, while the inline-comment handler inspected the raw event instead of the reconstructed pull request context.Context handling
Regression coverage