Refactor duplicated issue/PR update payload normalization into shared helper#40624
Conversation
|
Hey A couple of things to address before this is ready for review:
If you'd like a hand finishing this up, you can assign the prompt below to your coding agent:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40624 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). The changes touch skill scripts, shared MCP server, and workflow lock files outside enforced business-logic paths. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR reduces drift risk between update_issue and update_pull_request by extracting their shared update-payload normalization (title sanitization, body gating/operation shaping, and footer toggle parsing) into a single helper while keeping entity-specific semantics local to each handler.
Changes:
- Added
buildCommonEntityUpdateData(...)shared helper to normalize title/body/footer update fields consistently across entity update handlers. - Updated
update_issue.cjsandupdate_pull_request.cjsto delegate common normalization to the shared helper while preserving their distinct defaults and behaviors. - Added focused unit tests covering shared defaults, operation fallback behavior, and disallowed-body callback handling.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pull_request.cjs | Uses the shared helper for title/body/footer normalization; keeps PR-specific fields (state/base/draft/update_branch) handling local. |
| actions/setup/js/update_issue.cjs | Uses the shared helper for title/body/footer normalization; preserves issue-specific defaults and warning-on-disallowed-body behavior. |
| actions/setup/js/update_entity_helpers.cjs | Introduces shared normalization helper buildCommonEntityUpdateData for title/body/footer fields. |
| actions/setup/js/update_entity_helpers.test.cjs | Adds unit tests for the shared helper’s defaulting, operation fallback, and disallowed-body callback path. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 0 ( 📝 Analysis NotesTest 1 — Test 2 — Test 3 — Coverage gaps (informational — not blocking)
Verdict
References:
|
![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 /tdd and /zoom-out — requesting changes on test coverage gaps and two readability improvements.
The consolidation itself is clean and the semantic equivalence is well-preserved for both callers. The main concern is that the shared helper — now a single point of failure for both update_issue and update_pull_request — has test coverage gaps that leave the allowTitle: false branch completely unguarded.
📋 Key Themes & Highlights
Issues Found
- Missing
allowTitle: falsetest — this is the primary guard inupdate_pull_request.cjs; a regression here silently affects both handlers with no failing test - Second test covers two behaviors — operation-fallback precedence and
includeBodyInApiDatapassthrough should be split into separate cases - Test names describe implementation, not behavior — per
/tdd, names should read as specifications hasCommonUpdatesasymmetry — discarded in issue handler, used in PR handler; invisible at the call site without a comment_includeFooterunconditional — intentional and correct, but worth a brief comment to prevent well-meaning future "fixes"
Positive Highlights
- ✅ Clean extraction — the helper interface is narrow and the JSDoc is thorough
- ✅
configDefaultOperationintentionally omitted from the issue handler, correctly preservingitem.operation || 'append'semantics - ✅
onBodyDisallowedcallback is a good pattern — keeps the warning side-effect out of the helper - ✅ Tests exist and cover the happy path, operation fallback, and the disallowed-body callback
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 78.3 AIC · ⌖ 7.79 AIC · ⊞ 6.9K
| expect(result.updateData._rawBody).toBeUndefined(); | ||
| expect(result.hasCommonUpdates).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing allowTitle: false test — the key opt-out branch is unguarded.
The allowTitle option controls whether the title is included in the payload. If this logic regresses, both update_issue.cjs and update_pull_request.cjs would silently emit unwanted title updates with no failing test to catch it.
💡 Suggested test
it('skips title when allowTitle is false', () => {
const result = buildCommonEntityUpdateData(
{ title: 'Should not appear' },
{},
{ allowTitle: false }
);
expect(result.updateData.title).toBeUndefined();
expect(result.hasCommonUpdates).toBe(false);
});This is the primary guard used by update_pull_request.cjs (config.allow_title !== false) — a direct specification test makes the contract explicit.
There was a problem hiding this comment.
Added "skips title when allowTitle is false and does not set hasCommonUpdates" test in 8cf7e35.
| expect(result.hasCommonUpdates).toBe(true); | ||
| }); | ||
|
|
||
| it("uses configured operation fallback and can include body api field", () => { |
There was a problem hiding this comment.
[/tdd] This test verifies two distinct behaviors in one case — operation fallback precedence and includeBodyInApiData passthrough. Per the red-green-refactor loop, each it block should exercise exactly one behavior so failures point precisely to the broken contract.
💡 Suggested split
it('prefers configDefaultOperation over defaultOperation for body operation', () => {
const result = buildCommonEntityUpdateData(
{ body: 'Body text' },
{},
{ defaultOperation: 'append', configDefaultOperation: 'replace' }
);
expect(result.updateData._operation).toBe('replace');
});
it('includes body in api data when includeBodyInApiData is true', () => {
const result = buildCommonEntityUpdateData(
{ body: 'Body text' },
{},
{ defaultOperation: 'append', includeBodyInApiData: true }
);
expect(result.updateData.body).toBe('Body text');
});There was a problem hiding this comment.
Split into two separate it blocks — one for configDefaultOperation precedence, one for includeBodyInApiData passthrough — in 8cf7e35.
| const { buildCommonEntityUpdateData } = require("./update_entity_helpers.cjs"); | ||
|
|
||
| describe("update_entity_helpers.cjs - buildCommonEntityUpdateData", () => { | ||
| it("builds shared title/body/footer fields with defaults", () => { |
There was a problem hiding this comment.
[/tdd] The test title reads as an implementation summary rather than a behavioral specification. Per the /tdd principle, test names should read like requirements — "returns true for hasCommonUpdates when title and body are provided" communicates the contract more precisely than "builds shared title/body/footer fields with defaults".
A good smell-check: if you deleted the implementation and read only this test name, would you know what the system must do?
There was a problem hiding this comment.
Renamed to "returns hasCommonUpdates true and populates title, body fields, and footer when title and body are provided" in 8cf7e35.
| onBodyDisallowed(); | ||
| } | ||
|
|
||
| updateData._includeFooter = parseBoolTemplatable(config.footer, true); |
There was a problem hiding this comment.
[/zoom-out] _includeFooter is always written to updateData — even when both title and body are absent and hasCommonUpdates will be false. This matches pre-refactor behavior in both callers (intentional), but a brief comment here would protect against a future maintainer wrapping it in an if (hasCommonUpdates) guard and inadvertently breaking the footer on no-op calls.
💡 Suggested inline comment
// Always populate _includeFooter: downstream executeUpdate reads it regardless of
// whether title/body changed, matching pre-refactor behavior in both callers.
updateData._includeFooter = parseBoolTemplatable(config.footer, true);There was a problem hiding this comment.
Added the inline comment in 8cf7e35.
| @@ -131,23 +130,13 @@ const resolveIssueNumber = createStandardResolveNumber({ | |||
| * @returns {{success: true, data: Object} | {success: false, error: string}} Update data result | |||
| */ | |||
There was a problem hiding this comment.
[/zoom-out] hasCommonUpdates is returned by the helper but silently discarded here, while update_pull_request.cjs uses it to gate an early-return path. The asymmetry is invisible at the call site and could trip up a future maintainer who copies the PR pattern here expecting the same guard.
Consider either documenting why it's unused (/* hasCommonUpdates not needed: issue handler always continues to check entity-specific fields */) or destructuring with a comment: const { updateData } = ... is fine as-is, but the intent of the discarded value deserves a note.
There was a problem hiding this comment.
Added // hasCommonUpdates is not needed here: the issue handler always continues to check entity-specific fields (state, labels, assignees, milestone, title prefix). in 8cf7e35.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — three medium issues that should be addressed before merge.
Blocking themes
1. _operation undefined footgun in the helper contract
defaultOperation is documented as optional, but if it (and configDefaultOperation and item.operation) are all absent, updateData._operation silently becomes undefined. Current callers are safe because they always pass defaultOperation, but the optional typing makes future callers fragile. Either harden the guard or remove the optionality. (See inline comment on update_entity_helpers.cjs:33.)
2. Operation-priority chain partially untested
The test covers configDefaultOperation > defaultOperation but never exercises item.operation winning over both. A single precedence-order swap regression would go undetected. (See inline comment on update_entity_helpers.test.cjs:22.)
3. allowTitle: false completely untested
The option that feeds the PR handler's hasUpdates / skip path is never tested with false. The hasCommonUpdates contract is only half-verified. (See inline comment on update_entity_helpers.test.cjs:36.)
🔎 Code quality review by PR Code Quality Reviewer · 75.6 AIC · ⌖ 7.95 AIC · ⊞ 5.1K
|
|
||
| const canUpdateBody = config.allow_body !== false; | ||
| if (item.body !== undefined && canUpdateBody) { | ||
| updateData._operation = item.operation || configDefaultOperation || defaultOperation; |
There was a problem hiding this comment.
_operation can be silently undefined: when item.operation, configDefaultOperation, and defaultOperation are all absent or falsy, updateData._operation is set to undefined, which propagates to the body-update executor.
💡 Details
The JSDoc marks defaultOperation as optional ([options.defaultOperation]), so callers can legally omit it. If they also pass no item.operation and no configDefaultOperation, the expression:
updateData._operation = item.operation || configDefaultOperation || defaultOperation;resolves to undefined. Downstream, update_handler_factory calls updateBody with that undefined operation, and the body-update path may silently behave incorrectly.
Fix options:
- Remove the
[...]optionality from the JSDoc and guard defensively:
const resolvedOperation = item.operation || configDefaultOperation || defaultOperation;
if (!resolvedOperation) throw new Error("buildCommonEntityUpdateData: defaultOperation is required when body may be present");
updateData._operation = resolvedOperation;- Or document that
undefinedis intentional and verify downstream handles it.
There was a problem hiding this comment.
Added a runtime guard that throws "buildCommonEntityUpdateData: defaultOperation is required when body may be present" if all three sources resolve to falsy. Also removed the [...] optional brackets from the JSDoc in 8cf7e35.
| }); | ||
|
|
||
| it("uses configured operation fallback and can include body api field", () => { | ||
| const result = buildCommonEntityUpdateData( |
There was a problem hiding this comment.
item.operation priority is never exercised in this test: the test only verifies configDefaultOperation > defaultOperation fallback but never tests that an explicit item.operation wins over both.
💡 Suggested addition
The resolution chain item.operation || configDefaultOperation || defaultOperation has three levels, but this test only covers the bottom two. Add a case:
it("item.operation takes precedence over configDefaultOperation and defaultOperation", () => {
const result = buildCommonEntityUpdateData(
{ body: "Body text", operation: "prepend" },
{},
{
defaultOperation: "append",
configDefaultOperation: "replace",
}
);
expect(result.updateData._operation).toBe("prepend");
});Without this, a regression that swaps the precedence order (e.g., configDefaultOperation || item.operation || defaultOperation) would go undetected.
There was a problem hiding this comment.
Added "item.operation takes precedence over configDefaultOperation and defaultOperation" test covering all three levels of the resolution chain in 8cf7e35.
| expect(result.updateData.body).toBe("Body text"); | ||
| }); | ||
|
|
||
| it("invokes onBodyDisallowed when body updates are blocked", () => { |
There was a problem hiding this comment.
allowTitle: false is entirely untested: the test suite never passes allowTitle: false, so neither the title-skip behavior nor its effect on hasCommonUpdates is verified.
💡 Suggested addition
hasCommonUpdates controls whether the PR handler skips the update entirely (returns skipped: true). If allowTitle is false and a title is present, that title must not count toward hasCommonUpdates. Without a test, a future regression (e.g., hasCommonUpdates = true despite the title block) would silently affect the PR skip path.
it("blocks title update when allowTitle is false and does not set hasCommonUpdates", () => {
const result = buildCommonEntityUpdateData(
{ title: "Should be ignored" },
{},
{ allowTitle: false, defaultOperation: "append" }
);
expect(result.updateData.title).toBeUndefined();
expect(result.hasCommonUpdates).toBe(false);
});There was a problem hiding this comment.
Covered by the same allowTitle: false test added in 8cf7e35 — verifies both updateData.title is undefined and hasCommonUpdates is false.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…check error Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit — the |
update_issueandupdate_pull_requestboth implemented near-identical normalization fortitle,body, operation selection, and footer toggles, creating drift risk. This change consolidates shared normalization while keeping entity-specific fields and semantics in their original handlers.Shared normalization extraction
actions/setup/js/update_entity_helpers.cjswithbuildCommonEntityUpdateData(...).allow_bodygating_operation/_rawBodyshaping_includeFooterparsingIssue handler integration
update_issue.cjsnow uses the shared helper for title/body/footer normalization."append"state/status, labels, assignees, milestone, title prefix) remain localPR handler integration
update_pull_request.cjsnow uses the shared helper for title/body/footer normalization.allow_titlecontrol"replace"bodypassthrough for API payloadstate,base,draft,update_branch) remain localFocused helper coverage
update_entity_helpers.test.cjsto validate shared defaults, operation fallback, and disallowed-body callback handling.