Deduplicate safe-output body attribution handling - #51648
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage
Draft, no CI runs yet, 0 reviews. Small (+78/-59) dedup of update-entity body attribution logic. Group with #51649, #51647 for batch review.
|
|
Great work! 🎉 This refactoring elegantly solves the code duplication problem identified in #51645 by extracting the shared body-update pipeline into a buildUpdatedBody helper. The changes are clean and focused: ✅ What's working well:
This PR is ready for review and looks aligned with the project's contribution guidelines. The refactoring reduces future maintenance burden by eliminating the drift risk mentioned in the original issue.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51648 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
There was a problem hiding this comment.
Pull request overview
Centralizes safe-output body attribution and history-link generation for issue and pull-request updates.
Changes:
- Added shared
buildUpdatedBodyhelper. - Updated both handlers to use the shared pipeline.
- Added cross-repository attribution coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_pr_description_helpers.cjs |
Adds shared body-update orchestration. |
actions/setup/js/update_pr_description_helpers.test.cjs |
Tests cross-repository links. |
actions/setup/js/update_issue.cjs |
Uses the shared helper. |
actions/setup/js/update_pull_request.cjs |
Uses the shared helper. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Clean refactor — duplicated workflow attribution logic extracted into buildUpdatedBody. Behavior is preserved, and the new test covers the cross-repo attribution case. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16 AIC · ⌖ 7.08 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — three targeted suggestions; no blocking correctness issues.
📋 Key Themes & Highlights
Issues Found
- Test cleanup fragility:
delete process.env.GH_AW_CALLER_WORKFLOW_IDis inline — env leaks if an assertion throws. - Missing default-path test:
workflowRepoomitted/undefined(the common case) is untested. - Lost invariant doc: The cross-repo attribution comment was in both callers; it wasn't brought into
buildUpdatedBody.
Positive Highlights
- ✅ Clean DRY extraction — 58 lines of near-identical code replaced by a single well-documented helper
- ✅ Cross-repo contract (different repos for run URL vs. history URL) is correctly preserved
- ✅ New integration test validates the two-repo case with concrete URL assertions
- ✅ Unused imports (
buildWorkflowRunUrl,generateHistoryUrl) correctly removed from handler files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.2 AIC · ⌖ 7.74 AIC · ⊞ 7.1K
Comment /matt to run again
| itemType: "issue", | ||
| }); | ||
|
|
||
| expect(result).toContain("https://github.example/workflow/repository/actions/runs/123"); |
There was a problem hiding this comment.
[/tdd] delete process.env.GH_AW_CALLER_WORKFLOW_ID runs inline — if an assertion throws first, the env var leaks into subsequent tests and can cause false passes or spurious failures.
💡 Use afterEach to guarantee cleanup
Move env setup/teardown to beforeEach/afterEach at the describe block level:
describe('buildUpdatedBody', () => {
const originalCallerId = process.env.GH_AW_CALLER_WORKFLOW_ID;
afterEach(() => {
if (originalCallerId === undefined) delete process.env.GH_AW_CALLER_WORKFLOW_ID;
else process.env.GH_AW_CALLER_WORKFLOW_ID = originalCallerId;
});
// ...
});This guarantees cleanup even if assertions throw.
@copilot please address this.
|
|
||
| describe("buildIslandStartMarker", () => { | ||
| it("should build island start marker with workflow ID", () => { | ||
| const marker = buildIslandStartMarker("test-workflow"); |
There was a problem hiding this comment.
[/tdd] No test for the workflowRepo omitted/undefined case — this is the default path for non-cross-repo calls and is the most common usage.
💡 Suggested test
it('falls back to context.repo when workflowRepo is omitted', () => {
const result = buildUpdatedBody({
context: { repo: { owner: 'myorg', repo: 'myrepo' }, serverUrl: 'https://github.com', runId: 1 },
currentBody: '',
newContent: 'Body',
operation: 'append',
includeFooter: true,
itemType: 'issue',
// workflowRepo intentionally omitted
});
// run URL should use context.repo
expect(result).toContain('https://github.com/myorg/myrepo/actions/runs/1');
});This guards against a regression in buildWorkflowRunUrl(context, workflowRepo || context.repo) where undefined isn't handled the same way in all environments.
@copilot please address this.
| generateHistoryUrl({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| itemType, |
There was a problem hiding this comment.
[/codebase-design] The critical cross-repo invariant — "use workflowRepo for the run URL, but context.repo for the history link" — was documented in both callers but is absent from buildUpdatedBody. Callers can no longer see this distinction; it should live here.
💡 Suggested JSDoc addition
Add to the function's JSDoc (above @param params):
* `@remarks`
* Cross-repository attribution contract:
* - Run URL uses `workflowRepo` (falls back to `context.repo`) so attribution
* always points to the originating workflow, not the target repository.
* - History URL uses `context.repo` so links target the item being updated.@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (1 test)
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, review the latest maintainer-facing feedback, and run the Open items (newest first):
Branch refresh was requested.
|
PR TriageCategory: refactor · Risk: low · Priority: medium · Score: 55/100 Score breakdown
Recommended action: Batch: Automated triage via PR Triage Agent.
|
|
🎉 This pull request is included in a new release. Release: |
Issue and pull-request update handlers duplicated workflow attribution, history-link generation, and body-update assembly.
buildUpdatedBodyto centralize footer attribution, history URL generation, andupdateBodyinvocation.update_issueandupdate_pull_requestto use the shared helper.