[code-simplifier] Simplify add_reaction_and_edit_comment.cjs: remove duplicated endpoint/guard logic - #52222
Conversation
- Extract issueCommentEndpoint() helper to remove 4x duplicated
'POST /repos/{owner}/{repo}/issues/{issue_number}/comments' endpoint
construction across issues/issue_comment/pull_request/
pull_request_review_comment cases.
- Extract requirePayloadField() guard helper (mirrors the equivalent in
add_reaction.cjs) to remove repeated if (!x) { core.setFailed(...); return null; }
blocks in resolveEventEndpoints().
No behavior change - validated with existing vitest suite (107 tests
passed across add_reaction.test.cjs and add_reaction_and_edit_comment.test.cjs)
and tsc --noEmit typecheck.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
👋 Great work on this refactoring! This PR successfully reduces code duplication by extracting The refactoring is focused, well-documented, and maintains backward compatibility — the PR looks ready for review. Nice work!
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. This PR only contains changes to the production file 'actions/setup/js/add_reaction_and_edit_comment.cjs' (41 additions, 40 deletions). Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52222 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
![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 — requesting one change for a correctness regression introduced during the refactor.
📋 Key Themes & Highlights
Issues
- Correctness regression (
discussion_commentcase): the original two-field guard was collapsed into a single&&expression, changing error semantics and potentially masking which field is missing at runtime.
Positive Highlights
- ✅
issueCommentEndpointhelper cleanly eliminates the 4× duplicated REST route literal — good DRY application. - ✅
requirePayloadFieldmatches the shape of the existing helper inadd_reaction.cjs, keeping the pattern consistent across files. - ✅ All other cases in the switch are refactored correctly with separate per-field calls.
- ✅ 107/107 tests pass and
node -cclean.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21 AIC · ⌖ 6.69 AIC · ⊞ 7K
Comment /matt to run again
| core.setFailed(`${ERR_NOT_FOUND}: Discussion or comment information not found in event payload`); | ||
| return null; | ||
| } | ||
| if (!requirePayloadField(discussionNumber && commentId, "Discussion or comment information", ERR_NOT_FOUND)) return null; |
There was a problem hiding this comment.
[/diagnosing-bugs] Correctness regression: requirePayloadField(discussionNumber && commentId, ...) passes a single coerced value instead of two separate checks — the original validated each field independently, giving distinct error messages, but now both fields being absent produces one combined failure that masks which field is actually missing.
💡 Suggested fix
Replace the combined expression with two separate calls, mirroring every other case in the switch:
if (!requirePayloadField(discussionNumber, "Discussion number", ERR_NOT_FOUND)) return null;
if (!requirePayloadField(commentId, "Comment ID", ERR_NOT_FOUND)) return null;This preserves the original per-field specificity and makes failures diagnosable.
@copilot please address this.
There was a problem hiding this comment.
No code change is needed: the pre-refactor guard was also combined (if (!discussionNumber || !commentId)) and emitted the same combined error. requirePayloadField(discussionNumber && commentId, "Discussion or comment information", ERR_NOT_FOUND) preserves those exact failure semantics. The targeted test passes.
There was a problem hiding this comment.
Pull request overview
Simplifies event endpoint resolution without changing behavior or public APIs.
Changes:
- Extracts issue-comment endpoint construction.
- Centralizes required payload-field validation.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/add_reaction_and_edit_comment.cjs |
Reuses endpoint and payload-validation helpers across event cases. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Good simplification. The requirePayloadField helper and issueCommentEndpoint factory eliminate clear duplication with no behavioral changes.
One minor note: line 130 passes discussionNumber && commentId as the value to requirePayloadField. This is semantically equivalent to the original combined guard, but future readers may find checking the two fields separately clearer. Not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.5 AIC · ⌖ 6.29 AIC · ⊞ 5.4K
|
@copilot Please inspect the latest branch state, refresh the branch if needed, address the failed checks below, and run the No unresolved review threads were found.
Branch update was requested automatically for this run when GitHub allows it. Run context: https://github.com/github/gh-aw/actions/runs/31655027162
|
The branch is already refreshed at |
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot Quick triage for this PR:
Run: https://github.com/github/gh-aw/actions/runs/31656629817
|
Triage complete. |
|
This pull request was automatically closed because it expired on 2026-08-13T04:03:52.900Z.
|
Summary
Targeted, low-risk simplification of
actions/setup/js/add_reaction_and_edit_comment.cjs, identified via the code-simplifier scope-filter/simplification-scout pass over recent source-file candidates.Changes
issueCommentEndpoint(owner, repo, issueNumber)helper to remove 4x duplicated construction of thePOST /repos/{owner}/{repo}/issues/{issue_number}/commentsREST endpoint across theissues,issue_comment,pull_request, andpull_request_review_commentcases inresolveEventEndpoints().requirePayloadField(value, fieldName, errorCode)guard helper (mirroring the existing helper of the same name/shape inadd_reaction.cjs) to remove repeatedif (!x) { core.setFailed(...); return null; }boilerplate blocks.No behavior or public API changes: exported functions, error messages, and error codes are unchanged.
Validation
node -csyntax check passed.npx vitest run add_reaction_and_edit_comment.test.cjs add_reaction.test.cjs --no-file-parallelism— 107/107 tests passed.npx tsc --noEmit— no new type errors introduced.make fmt— no additional formatting changes needed.Deterministic pre-processing note
Candidate files, recent PRs/commits, and history summary were supplied via pre-computed deterministic JSON inputs (
/tmp/gh-aw/agent/code-simplifier/*.json), avoiding repeated GitHub API calls. Ascope-filtersub-agent screened the 20 candidate files and flaggedadd_reaction_and_edit_comment.cjs(alongsideadd_comment.cjsandadd_workflow_run_comment.cjs) as having the clearest, lowest-risk duplication. This PR addresses the smallest and safest of the three; the other two involve larger (1000+ line) files better suited for a follow-up pass.Run context: https://github.com/github/gh-aw/actions/runs/31655027162> Generated by 👨🍳 PR Sous Chef · gpt54 · 30.7 AIC · ⌖ 5.28 AIC · ⊞ 8.5K · ◷
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31656629817> Generated by 👨🍳 PR Sous Chef · gpt54 · 4.42 AIC · ⌖ 6.21 AIC · ⊞ 8.5K · ◷