feat(close-issue): add duplicate_of for native GitHub duplicate marking#43152
duplicate_of for native GitHub duplicate marking#43152Conversation
When `duplicate_of` is set and `state_reason` is DUPLICATE, the handler now calls the GitHub `markAsDuplicate` GraphQL mutation to create a native "marked this as a duplicate of #X" timeline event, instead of relying solely on a text comment. Supports bare numbers, #NUMBER, owner/repo#NUMBER, and full URLs. Failures in the mutation are non-fatal (logged as warnings). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
duplicate_of for native GitHub duplicate marking
There was a problem hiding this comment.
Pull request overview
Adds first-class native GitHub duplicate relationships to the close_issue safe-output by introducing a duplicate_of field, so “marked as duplicate of #X” timeline events can be created via GraphQL (with a non-fatal fallback on failure).
Changes:
- Add
duplicate_ofto the close-issue safe-output schema (runtime + compiler copies) and TypeScript types. - Implement parsing + GraphQL
markAsDuplicatemutation inclose_issue.cjs, and add targeted tests. - Update safe-outputs docs/spec to describe the new field, accepted formats, and fallback behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Adds duplicate_of to the close_issue tool schema (compiler/runtime copy). |
| actions/setup/js/safe_outputs_tools.json | Adds duplicate_of to the close_issue tool schema (action/runtime copy). |
| actions/setup/js/types/safe-outputs.d.ts | Extends CloseIssueItem with duplicate_of?: number | string. |
| actions/setup/js/close_issue.cjs | Implements parseDuplicateOf() and native duplicate marking via GraphQL mutation. |
| actions/setup/js/close_issue.test.cjs | Adds unit coverage for parsing and native duplicate marking behavior. |
| docs/src/content/docs/specs/safe-outputs-specification.md | Updates the spec to define semantics and fallback behavior for duplicate_of. |
| docs/src/content/docs/reference/safe-outputs.md | Documents duplicate_of usage in the close-issue reference section. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/7 changed files
- Comments generated: 4
- Review effort level: Low
| // Bare number or "#NUMBER" | ||
| const bareMatch = str.match(/^#?(\d+)$/); | ||
| if (bareMatch) { | ||
| return { owner: defaultOwner, repo: defaultRepo, issueNumber: parseInt(bareMatch[1], 10) }; | ||
| } | ||
|
|
||
| // "owner/repo#NUMBER" | ||
| const refMatch = str.match(/^([^/]+)\/([^#]+)#(\d+)$/); | ||
| if (refMatch) { | ||
| return { owner: refMatch[1], repo: refMatch[2], issueNumber: parseInt(refMatch[3], 10) }; | ||
| } | ||
|
|
||
| // GitHub issue URL: https://github.com/owner/repo/issues/NUMBER | ||
| const urlMatch = str.match(/^https?:\/\/(?:[^/]+)\/([^/]+)\/([^/]+)\/issues\/(\d+)/); | ||
| if (urlMatch) { | ||
| return { owner: urlMatch[1], repo: urlMatch[2], issueNumber: parseInt(urlMatch[3], 10) }; | ||
| } |
There was a problem hiding this comment.
Fixed in commit 005a054: the bare/#N patterns now use [1-9]\d* (rejects #0), and the owner/repo#N regex uses [\w.-]+ for both segments (prevents / in the repo part). Additionally, commit fix: add length pre-check adds a > 15 digit length guard before parseInt in all three branches, plus a Number.isSafeInteger check after, to prevent silent precision loss on arbitrarily long strings.
| // When duplicate_of is provided and state_reason is DUPLICATE, create the native duplicate relationship | ||
| const stateReasonUpper = stateReason.toUpperCase(); | ||
| if (item.duplicate_of !== undefined && item.duplicate_of !== null && stateReasonUpper === "DUPLICATE") { | ||
| const parsed = parseDuplicateOf(item.duplicate_of, owner, repo); | ||
| if (parsed) { | ||
| core.info(`Marking issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`); | ||
| return closePromise.then(async closedEntity => { | ||
| try { | ||
| await markIssueAsDuplicate(github, owner, repo, entityNumber, parsed.owner, parsed.repo, parsed.issueNumber); | ||
| core.info(`✓ Marked issue #${entityNumber} as duplicate of #${parsed.issueNumber}`); | ||
| } catch (dupError) { | ||
| core.warning(`Failed to mark native duplicate relationship for #${entityNumber}: ${getErrorMessage(dupError)}`); | ||
| } | ||
| return closedEntity; | ||
| }); | ||
| } else { | ||
| core.warning(`duplicate_of value "${item.duplicate_of}" could not be parsed; skipping native duplicate marking`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in commit 005a054: after parseDuplicateOf resolves the canonical ref, validateRepo is called against defaultTargetRepo + allowedRepos before any REST or GraphQL calls. Cross-repo canonical refs are rejected unless the repo is in the allowlist (warning logged, close still succeeds). A same-issue guard was also added to early-exit if parsed.owner/repo/number matches the issue being closed.
| async function markIssueAsDuplicate(github, duplicateOwner, duplicateRepo, duplicateNumber, canonicalOwner, canonicalRepo, canonicalNumber) { | ||
| // Fetch node IDs for both issues via REST (node_id is included in issues.get response) | ||
| const [{ data: duplicateData }, { data: canonicalData }] = await Promise.all([ | ||
| github.rest.issues.get({ owner: duplicateOwner, repo: duplicateRepo, issue_number: duplicateNumber }), | ||
| github.rest.issues.get({ owner: canonicalOwner, repo: canonicalRepo, issue_number: canonicalNumber }), | ||
| ]); | ||
|
|
||
| const duplicateNodeId = duplicateData.node_id; | ||
| const canonicalNodeId = canonicalData.node_id; | ||
|
|
||
| await github.graphql( | ||
| `mutation($duplicateId: ID!, $canonicalId: ID!) { | ||
| markAsDuplicate(input: { duplicateId: $duplicateId, canonicalId: $canonicalId }) { | ||
| duplicate { | ||
| ... on Issue { | ||
| id | ||
| number | ||
| } | ||
| } | ||
| } | ||
| }`, | ||
| { duplicateId: duplicateNodeId, canonicalId: canonicalNodeId } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fixed in commit 005a054: markIssueAsDuplicate now accepts duplicateNodeId directly from closedEntity.node_id (returned by issues.update) instead of re-fetching. Explicit guards throw with actionable messages when either node_id is missing or the two node IDs are equal (same-issue).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work on the
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (194 new lines) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (15 tests)
i️ NotesTest inflation (2.41×): The test file grew 248 lines vs 103 in the production file. Above the 2:1 flag threshold but acceptable — the Duplicate cluster: Mocking approach: Tests use global injection ( Missing coverage: No test verifies failure when the Sampling note: Analysis focused on 15 new tests in Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"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.
REQUEST_CHANGES — six issues flagged, two high-severity (security-adjacent + correctness), four medium.
### Blocking findings summary
High: allowed_repos bypass via duplicate_of (line 263)
The canonical issue referenced by duplicate_of is fetched via issues.get and passed to GraphQL without any allowed_repos validation. In agentic workflows where issue output can be LLM-influenced, this allows an injection that reads arbitrary repos with the handler's token. The close-issue handler enforces allowed_repos for the issue being closed but skips it for the canonical target. Fix: validate parsed.owner/parsed.repo against the same allowlist, or restrict to same-repo by default.
High: refMatch regex accepts / in repo segment (line 40)
[^#]+ matches any non-# character, including /. Input like org/repo/extra#5 silently parses as a malformed repo slug and proceeds to issues.get, causing a confusing failure downstream. Fix: change to [^/#]+.
Medium: issueNumber: 0 accepted (line 36)
GitHub issues start at 1. parseDuplicateOf returns { issueNumber: 0 } for "#0", which will hit an invalid API call. Add if (num < 1) return null after each parseInt.
Medium: Missing node_id null check (line 88)
If issues.get returns a response without node_id (e.g. deprecated API version), the GraphQL mutation receives undefined and throws an opaque variable-type error. Add an explicit guard before the mutation call.
Medium: Redundant issues.get for the closing issue (line 69)
markIssueAsDuplicate re-fetches the duplicate issue to get its node_id, but that issue was already fetched by getDetails earlier in the handler pipeline. This doubles the REST calls and increases rate-limit exposure on high-volume workflows.
Medium: Doc heading duplicate-of vs field name duplicate_of (docs, line 271)
The reference doc uses kebab-case duplicate-of as the heading. Users following the doc will emit the wrong field; it will be silently ignored with no warning.
🔎 Code quality review by PR Code Quality Reviewer · 156.9 AIC · ⌖ 6.27 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
|
|
||
| // "owner/repo#NUMBER" | ||
| const refMatch = str.match(/^([^/]+)\/([^#]+)#(\d+)$/); |
There was a problem hiding this comment.
[^#]+ in the refMatch regex allows / in the repo segment, producing bogus parses for inputs like org/repo/subpath#5.
💡 Suggested fix
The current pattern [^#]+ matches any character except #, including /. So org/some/nested/path#42 parses as owner=org, repo=some/nested/path, issueNumber=42 — an invalid repo slug that will cause the subsequent issues.get REST call to fail with a confusing error, or silently hit the wrong endpoint.
Change [^#]+ to [^/#]+:
// before
const refMatch = str.match(/^([^/]+)\/([^#]+)#(\d+)$/);
// after
const refMatch = str.match(/^([^/]+)\/([^/#]+)#(\d+)$/);This ensures repo cannot contain a /, matching the same constraint applied to owner by [^/]+.
There was a problem hiding this comment.
Fixed in commit 005a054: the refMatch regex now uses [\w.-]+ for both the owner and repo segments, which prevents / from appearing in either part.
| // Bare number or "#NUMBER" | ||
| const bareMatch = str.match(/^#?(\d+)$/); | ||
| if (bareMatch) { | ||
| return { owner: defaultOwner, repo: defaultRepo, issueNumber: parseInt(bareMatch[1], 10) }; |
There was a problem hiding this comment.
parseDuplicateOf accepts issueNumber: 0, which is an invalid GitHub issue number and will produce a confusing 404 from the GraphQL mutation.
💡 Suggested fix
parseInt('#0', 10) returns 0. GitHub issues are 1-indexed, so 0 is never a valid issue number. Passing node ID for issue #0 to markAsDuplicate will fail with an error from GitHub's API.
Add a guard after parseInt:
const num = parseInt(bareMatch[1], 10);
if (num < 1) return null;
return { owner: defaultOwner, repo: defaultRepo, issueNumber: num };Same guard should be applied to the refMatch and urlMatch branches.
There was a problem hiding this comment.
Already fixed: the regex [1-9]\d* (introduced in commit 005a054) rejects 0, #0, and "0" — only positive integers starting with a non-zero digit are accepted. Tests covering this were added at lines 911–915 of close_issue.test.cjs.
| */ | ||
| async function markIssueAsDuplicate(github, duplicateOwner, duplicateRepo, duplicateNumber, canonicalOwner, canonicalRepo, canonicalNumber) { | ||
| // Fetch node IDs for both issues via REST (node_id is included in issues.get response) | ||
| const [{ data: duplicateData }, { data: canonicalData }] = await Promise.all([ |
There was a problem hiding this comment.
markIssueAsDuplicate re-fetches the duplicate issue via issues.get even though closeEntity is called after getDetails has already retrieved it — this is a redundant API call that increases latency and rate-limit consumption.
💡 Suggested fix
The closeEntity callback already has entityNumber and owner/repo in scope. The issue object returned by getDetails (which fires earlier in the handler pipeline) includes node_id. Passing that node ID through avoids the extra issues.get:
// In closeEntity, receive the already-fetched entity detail:
closedEntityDetail.node_id // available from getDetails resultAlternatively, the closeEntity callback signature could be extended to receive the entity detail, or markIssueAsDuplicate could accept a pre-resolved duplicateNodeId argument so callers that already have the node_id can skip the REST call.
There was a problem hiding this comment.
Fixed in commit 005a054: markIssueAsDuplicate now receives duplicateNodeId directly as a parameter (sourced from closedEntity.node_id returned by issues.update), eliminating the redundant issues.get for the duplicate issue. Only the canonical issue is fetched via REST.
| } | ||
| } | ||
| }`, | ||
| { duplicateId: duplicateNodeId, canonicalId: canonicalNodeId } |
There was a problem hiding this comment.
duplicateNodeId and canonicalNodeId are passed to GraphQL with no null check; if node_id is absent from the REST response the mutation receives undefined and throws an opaque error.
💡 Suggested fix
The REST issues.get response includes node_id in practice, but it is not formally guaranteed by all versions of the GitHub API. If either is undefined, the GraphQL call will throw Variable '$duplicateId' was provided invalid value — which is already caught, but the warning message won't be actionable.
Add explicit guards:
const duplicateNodeId = duplicateData.node_id;
const canonicalNodeId = canonicalData.node_id;
if (!duplicateNodeId || !canonicalNodeId) {
throw new Error(`Missing node_id: duplicate=${duplicateNodeId}, canonical=${canonicalNodeId}`);
}Also note: if duplicateOwner/duplicateRepo/duplicateNumber equals canonicalOwner/canonicalRepo/canonicalNumber, the same node_id is sent for both arguments. GitHub's API will reject a self-duplicate (duplicate == canonical), producing a non-actionable warning. A pre-call guard would give a clearer failure message.
There was a problem hiding this comment.
Already fixed in commit 005a054: explicit guards throw "node_id missing for duplicate issue" and "node_id missing for canonical issue …" respectively before the GraphQL call. A same-node guard throws "Cannot mark issue as a duplicate of itself" when both IDs are equal.
| core.info(`Marking issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`); | ||
| return closePromise.then(async closedEntity => { | ||
| try { | ||
| await markIssueAsDuplicate(github, owner, repo, entityNumber, parsed.owner, parsed.repo, parsed.issueNumber); |
There was a problem hiding this comment.
parseDuplicateOf can resolve to an arbitrary owner/repo that is never validated against allowed_repos, enabling the handler to fetch issues from repos outside its configured trust boundary.
💡 Why this matters
The handler already enforces allowed_repos for the issue being closed, but there is no equivalent check on the canonical issue referenced by duplicate_of. When duplicate_of is attacker-org/attacker-repo#1, the handler will:
- Call
issues.get({ owner: 'attacker-org', repo: 'attacker-repo', issue_number: 1 })to fetch the node ID — this is a network request to an arbitrary repository. - Pass that node ID to the
markAsDuplicateGraphQL mutation.
This means the GitHub token (which may have broad read permissions) is used to read arbitrary repos when processing untrusted agent output. In workflows where agent output can be influenced by untrusted content (e.g., issue bodies processed by an LLM), this is an injection vector.
Consider validating that parsed.owner/parsed.repo is allowed either by the same allowed_repos list or by defaulting to same-repo-only for the canonical issue when cross-repo is not explicitly configured.
There was a problem hiding this comment.
Fixed in commit 005a054: validateRepo(canonicalRepoSlug, defaultTargetRepo, allowedRepos) is called on the parsed canonical ref before any REST or GraphQL calls are made.
|
|
||
| **State Reasons**: `completed`, `not_planned`, `duplicate` (default: `completed`). Can also be set per-item in agent output. | ||
|
|
||
| **`duplicate-of`**: When closing as a duplicate (agent sets `state_reason: duplicate`), the agent may also supply a `duplicate_of` field pointing to the canonical issue. Accepts a bare number (`123`), a `#`-prefixed number (`#123`), an `owner/repo#number` reference, or a full GitHub issue URL. When provided, creates a native GitHub **"marked this as a duplicate of #X"** relationship in the timeline — no separate comment needed for the linkage. Falls back gracefully (logs a warning) if the duplicate marking fails. |
There was a problem hiding this comment.
Field name in docs says duplicate-of (kebab-case) but the actual schema and implementation use duplicate_of (snake_case) — users following this doc will emit the wrong field name and get no duplicate marking with no error.
💡 Fix
-**`duplicate-of`**: When closing as a duplicate ...
+**`duplicate_of`**: When closing as a duplicate ...The wrong name is silently ignored by the handler (it just won't find item.duplicate_of), so there will be no warning or error to guide users.
There was a problem hiding this comment.
Fixed in commit 005a054: the docs heading was corrected from duplicate-of to duplicate_of.
| // Bare number or "#NUMBER" | ||
| const bareMatch = str.match(/^#?(\d+)$/); | ||
| if (bareMatch) { | ||
| return { owner: defaultOwner, repo: defaultRepo, issueNumber: parseInt(bareMatch[1], 10) }; |
There was a problem hiding this comment.
parseInt on an arbitrarily long digit string silently corrupts the issue number due to IEEE 754 double precision loss — issue 99999999999999999999 becomes 100000000000000000000 and silently references the wrong issue.
💡 Suggested fix
parseInt returns a floating-point double. For strings longer than 15-16 significant digits, precision is lost:
parseInt('99999999999999999999', 10) // → 100000000000000000000 (wrong)Add a Number.isSafeInteger guard after each parseInt:
const num = parseInt(bareMatch[1], 10);
if (!Number.isSafeInteger(num) || num < 1) return null;Apply the same guard in the refMatch and urlMatch branches.
There was a problem hiding this comment.
Fixed: a > 15 digit length pre-check is applied before parseInt in all three branches, followed by a Number.isSafeInteger guard. A test covering 16-digit inputs was added ("should return null for issue numbers with more than 15 digits").
| } | ||
|
|
||
| // GitHub issue URL: https://github.com/owner/repo/issues/NUMBER | ||
| const urlMatch = str.match(/^https?:\/\/(?:[^/]+)\/([^/]+)\/([^/]+)\/issues\/(\d+)/); |
There was a problem hiding this comment.
URL regex has no end anchor — https://github.com/owner/repo/issues/42abc matches and returns issueNumber: 42, treating a malformed URL as valid.
💡 Suggested fix
The current regex:
/^https?:\/\/(?:[^/]+)\/([^/]+)\/([^/]+)\/issues\/(\d+)/has no trailing $. A URL like https://github.com/owner/repo/issues/42?foo=bar or https://github.com/owner/repo/issues/42abc will match and extract 42, silently accepting a URL that doesn't reference a clean issue endpoint.
Fix: add (?:[?#].*)?$ to accept optional query/fragment but reject garbage:
/^https?:\/\/(?:[^/]+)\/([^/]+)\/([^/]+)\/issues\/(\d+)(?:[?#][^\s]*)?$/Or simply add $ to strictly require the number is the last path segment.
There was a problem hiding this comment.
Fixed in commit 005a054: the URL regex uses (?:[?#/].*)?$ at the end, which anchors the match at the issue number and rejects URLs with appended path segments like /extra.
|
|
||
| // When duplicate_of is provided and state_reason is DUPLICATE, create the native duplicate relationship | ||
| const stateReasonUpper = stateReason.toUpperCase(); | ||
| if (item.duplicate_of !== undefined && item.duplicate_of !== null && stateReasonUpper === "DUPLICATE") { |
There was a problem hiding this comment.
When duplicate_of is present but state_reason is not DUPLICATE, the field is silently dropped with no warning — callers have no indication their intent was ignored.
💡 Suggested fix
Currently, if an agent emits:
{"type": "close_issue", "issue_number": 100, "body": "Done", "duplicate_of": 50, "state_reason": "COMPLETED"}The duplicate_of: 50 is completely ignored because stateReasonUpper !== 'DUPLICATE'. The issue closes as COMPLETED and no native duplicate relationship is created — but the caller gets no warning.
Add a core.warning for this case:
if (item.duplicate_of !== undefined && item.duplicate_of !== null && stateReasonUpper !== 'DUPLICATE') {
core.warning(`duplicate_of is set but state_reason is ${stateReason} (not DUPLICATE); native duplicate marking will be skipped`);
}There was a problem hiding this comment.
Fixed: core.warning("duplicate_of is set but state_reason is … (not DUPLICATE); native duplicate marking will be skipped") is now emitted in closeEntity when duplicate_of is present but stateReasonUpper !== "DUPLICATE". A test for this ("should warn when duplicate_of is set but state_reason is not DUPLICATE") was added.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 84.9 AIC · ⌖ 6.26 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
actions/setup/js/close_issue.cjs:40
The [^#]+ pattern in refMatch permits forward slashes inside the repo segment, so org/sub/repo#123 would parse as { owner: "org", repo: "sub/repo", issueNumber: 123 } — an invalid repo name. Change the character class to [^/#]+ to enforce a standard owner/repo#N format:
const refMatch = str.match(/^\([^/]+\)\/\([^/#]+\)#(\d+)$/);@copilot please address this.
actions/setup/js/close_issue.cjs:65
The markIssueAsDuplicate function calls issues.get for the duplicate issue to obtain its node_id, but the main handler already called closeIssue (which internally calls issues.update) immediately before this. By the time markIssueAsDuplicate runs the issue is already closed. The extra REST call for the duplicate issue is therefore redundant — the node_id returned by issues.update or already fetched by getIssueDetails (earlier in the handler) could be threaded through to av…
actions/setup/js/close_issue.cjs:107
The success log message says #${parsed.issueNumber} regardless of whether the canonical issue is cross-repo. For cross-repo duplicates this omits the owner/repo context and can be misleading. Use the fuller reference from parsed:
core.info(`✓ Marked issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`);@copilot please address this.
Review SummaryApplied 📋 Issues raised (6 inline comments)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 151.4 AIC · ⌖ 8.74 AIC · ⊞ 6.6K
Comment /matt to run again
| // Bare number or "#NUMBER" | ||
| const bareMatch = str.match(/^#?(\d+)$/); | ||
| if (bareMatch) { | ||
| return { owner: defaultOwner, repo: defaultRepo, issueNumber: parseInt(bareMatch[1], 10) }; |
There was a problem hiding this comment.
[/tdd] Missing validation: parseDuplicateOf accepts 0 as a valid issue number (e.g., duplicate_of: 0), but GitHub issue numbers start at 1 — this will produce a confusing API error downstream instead of a clean warning-and-skip.
💡 Suggested fix + test
Add a positive-number guard after parsing:
const bareMatch = str.match(/^#?(\d+)$/);
if (bareMatch) {
const issueNumber = parseInt(bareMatch[1], 10);
if (issueNumber < 1) return null;
return { owner: defaultOwner, repo: defaultRepo, issueNumber };
}And add a test:
it('should return null for issue number 0', () => {
expect(parseDuplicateOf(0, 'owner', 'repo')).toBeNull();
});The same guard should apply to the refMatch and urlMatch branches.
@copilot please address this.
There was a problem hiding this comment.
Already addressed: parseDuplicateOf uses [1-9]\d* which rejects 0 at the regex level, and the test at lines 911–915 covers parseDuplicateOf(0, …), parseDuplicateOf("#0", …), and parseDuplicateOf("0", …) all returning null.
| return closePromise.then(async closedEntity => { | ||
| try { | ||
| await markIssueAsDuplicate(github, owner, repo, entityNumber, parsed.owner, parsed.repo, parsed.issueNumber); | ||
| core.info(`✓ Marked issue #${entityNumber} as duplicate of #${parsed.issueNumber}`); |
There was a problem hiding this comment.
[/codebase-design] The success log drops the canonical repo context in cross-repository cases — #${parsed.issueNumber} is ambiguous when parsed.owner/parsed.repo differs from the current repo.
💡 Suggested fix
Use the same format as the earlier info log to stay consistent and unambiguous:
core.info(`✓ Marked issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`);This mirrors the pattern on the line above (line 260) and removes any ambiguity for cross-repo duplicates.
@copilot please address this.
There was a problem hiding this comment.
Fixed: the success log now uses ${parsed.owner}/${parsed.repo}#${parsed.issueNumber} to match the pre-call info log and provide unambiguous context for cross-repo duplicates.
|
|
||
| **State Reasons**: `completed`, `not_planned`, `duplicate` (default: `completed`). Can also be set per-item in agent output. | ||
|
|
||
| **`duplicate-of`**: When closing as a duplicate (agent sets `state_reason: duplicate`), the agent may also supply a `duplicate_of` field pointing to the canonical issue. Accepts a bare number (`123`), a `#`-prefixed number (`#123`), an `owner/repo#number` reference, or a full GitHub issue URL. When provided, creates a native GitHub **"marked this as a duplicate of #X"** relationship in the timeline — no separate comment needed for the linkage. Falls back gracefully (logs a warning) if the duplicate marking fails. |
There was a problem hiding this comment.
[/grill-with-docs] The heading **duplicate-of** uses a hyphen-cased format that looks like a YAML config key (same style as allow-body: false), but duplicate_of is an agent output field (JSON, underscore-cased). This is confusing for readers.
💡 Suggested fix
Change the heading to match the actual field name so readers don't look for it in YAML frontmatter:
**`duplicate_of`**: When closing as a duplicate ...Compare with how allow-body: false is documented — it deliberately uses hyphen+colon because it IS a YAML key. duplicate_of is not.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 005a054: the heading was corrected from duplicate-of to duplicate_of.
| return {}; | ||
| }; | ||
|
|
||
| const result = await handler({ issue_number: 200, body: "Duplicate", state_reason: "DUPLICATE", duplicate_of: "#99" }, {}); |
There was a problem hiding this comment.
[/tdd] The duplicate marking integration tests only test with uppercase "DUPLICATE" for item-level state_reason. The docs and existing close-issue tests show that agents typically send lowercase "duplicate" — there is no test confirming the .toUpperCase() normalisation actually enables the mutation in that case.
💡 Suggested test
it('should call markAsDuplicate when state_reason is lowercase "duplicate"', async () => {
const handler = await main({ max: 10 });
const graphqlCalls = [];
// ... (same setup as other tests) ...
const result = await handler(
{ issue_number: 200, body: 'Duplicate', state_reason: 'duplicate', duplicate_of: '#99' },
{}
);
expect(result.success).toBe(true);
expect(graphqlCalls.length).toBe(1);
});Also worth adding: config-level state_reason: 'duplicate' (lowercase) with duplicate_of set.
@copilot please address this.
There was a problem hiding this comment.
Fixed: added "should call markAsDuplicate when state_reason is lowercase 'duplicate'" — it passes state_reason: "duplicate" with duplicate_of: "#99" and asserts graphqlCalls.length === 1 and canonicalId === "node_99", confirming .toUpperCase() normalisation works correctly.
| * @param {number} canonicalNumber - Issue number that is the canonical original | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function markIssueAsDuplicate(github, duplicateOwner, duplicateRepo, duplicateNumber, canonicalOwner, canonicalRepo, canonicalNumber) { |
There was a problem hiding this comment.
[/codebase-design] markIssueAsDuplicate has 7 positional parameters — this is hard to call correctly and impossible to read at the call site without counting arguments.
💡 Suggested refactor
The function already has a logical grouping (two issue coordinates). Consider a two-object signature:
async function markIssueAsDuplicate(github, duplicate, canonical) {
// duplicate: { owner, repo, issueNumber }
// canonical: { owner, repo, issueNumber }
const [{ data: duplicateData }, { data: canonicalData }] = await Promise.all([
github.rest.issues.get({ owner: duplicate.owner, repo: duplicate.repo, issue_number: duplicate.issueNumber }),
github.rest.issues.get({ owner: canonical.owner, repo: canonical.repo, issue_number: canonical.issueNumber }),
]);
// ...
}The call site in closeEntity already has parsed (canonical coords) and the current owner/repo/entityNumber (duplicate coords), so passing two small structs is straightforward.
@copilot please address this.
There was a problem hiding this comment.
The current function signature is markIssueAsDuplicate(github, duplicateNodeId, canonicalOwner, canonicalRepo, canonicalNumber) — 5 parameters, not 7. This thread appears to reference an older version of the code and is now outdated.
| expect(mockCore.warnings.some(w => w.includes("could not be parsed"))).toBe(true); | ||
| }); | ||
|
|
||
| it("should continue close successfully when markAsDuplicate mutation fails", async () => { |
There was a problem hiding this comment.
[/tdd] The non-fatal error test only covers GraphQL mutation failures. The try/catch in closeEntity also wraps the rest.issues.get calls inside markIssueAsDuplicate — a network error or 404 on either node-ID fetch is equally realistic (e.g., the canonical issue is private or doesn't exist). There is no test confirming that REST-layer errors are also non-fatal.
💡 Suggested test
it('should warn and succeed when node-ID REST fetch fails for canonical issue', async () => {
const handler = await main({ max: 10, state_reason: 'DUPLICATE' });
// leave issues.get in error state:
mockGithub.rest.issues.get = async ({ issue_number }) => {
if (issue_number === 10) throw new Error('Not Found');
return { data: { node_id: `node_${issue_number}`, ... } };
};
const result = await handler({ issue_number: 600, body: 'Duplicate', duplicate_of: 10 }, {});
expect(result.success).toBe(true);
expect(mockCore.warnings.some(w => w.includes('Failed to mark native duplicate'))).toBe(true);
});@copilot please address this.
There was a problem hiding this comment.
Fixed: added "should warn and succeed when REST fetch for canonical issue fails" — it overrides issues.get to throw "Not Found" for the canonical issue number while returning normally for the issue being closed, then asserts result.success === true and that the "Failed to mark native duplicate" warning was emitted.
|
@copilot please run the
|
…cs rename Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all blocking feedback:
Commits: |
|
@copilot please run the
|
🤖 PR Triage
Score Breakdown
|
…nd new tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…inputs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved review threads addressed. Summary of changes since last run:
All 61 unit tests pass. CI needs to be re-triggered by a maintainer to confirm green. |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in commit |
When
close-issuecloses an issue as a duplicate, it previously could only recordstate_reason: duplicateand post a plain text comment — the native GitHub "marked this as a duplicate of #X" timeline relationship was never created.Changes
close_issue.cjsparseDuplicateOf()— resolvesduplicate_ofto{owner, repo, issueNumber}from bare numbers,#N,owner/repo#N, or full GitHub URLsmarkIssueAsDuplicate()— fetches node IDs via REST then calls themarkAsDuplicateGraphQL mutationcloseEntitycallback: whenitem.duplicate_ofis set and effectivestate_reasonisDUPLICATE, calls the mutation after closing; mutation failures are non-fatal (warning logged, close still succeeds)MCP tool schema (
safe_outputs_tools.json— both runtime and compiler copies)duplicate_of: number | stringproperty toclose_issueTypes (
safe-outputs.d.ts)duplicate_of?: number | stringtoCloseIssueItemDocs — reference page and spec updated with semantics, accepted formats, and the non-fatal fallback behaviour
Usage
{"type": "close_issue", "issue_number": 1234, "state_reason": "DUPLICATE", "duplicate_of": "github/gh-aw#22853", "body": "Duplicate of #22853."}This produces a native
marked_as_duplicatetimeline event linking the canonical issue instead of a free-text comment carrying the linkage.