Fix temporary ID substitution in safe-output comments - #57955
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
||
| // Handle add_comment which returns an array of comments | ||
| // Handle the current add_comment result shape. | ||
| if (messageType === "add_comment" && result?.commentId && result?.repo) { |
There was a problem hiding this comment.
@copilot review all other safe outputs for different shapes
There was a problem hiding this comment.
Reviewed the safe-output result conventions and updated add_comment tracking to use the current object body while preserving its metadata, with legacy array support retaining tracking metadata and optional body. Regression coverage now exercises the synthetic update request. Changes are in 5a6bff2 and 6d81653.
|
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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality 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 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
- "github.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
The update can discard posted comment metadata, and the regression test does not validate the synthetic update path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds support for resolving temporary issue IDs in comments emitted before issue creation.
Changes:
- Tracks object-shaped
add_commentresults. - Preserves comment metadata for synthetic updates.
- Adds regression coverage for forward references.
File summaries
| File | Review |
|---|---|
actions/setup/js/safe_output_handler_manager.cjs |
Must track the returned comment body, with legacy fallback, to preserve generated markers and annotations during updates. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Must exercise the synthetic update and verify ID replacement while preserving generated metadata. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| // Handle the current add_comment result shape. | ||
| if (messageType === "add_comment" && result?.commentId && result?.repo) { | ||
| const contentToCheck = getContentToCheck(messageType, message, result); | ||
| if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) { | ||
| core.info(`Comment ${result.commentId} on ${result.repo}#${result.itemNumber} was created with unresolved temporary IDs - tracking for update`); | ||
| outputsWithUnresolvedIds.push({ | ||
| type: messageType, | ||
| message, | ||
| result: { | ||
| commentId: result.commentId, | ||
| itemNumber: result.itemNumber, | ||
| repo: result.repo, | ||
| isDiscussion: result.isDiscussion, | ||
| }, | ||
| originalTempIdMapSize: tempIdMapSizeBefore, | ||
| }); | ||
| } | ||
| } |
| { type: "create_issue", temporary_id: "aw_track1", title: "Tracking issue" }, | ||
| ]; | ||
|
|
||
| const result = await processMessages(handlers, messages); |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
COMMENT — this patch fixes the object-shaped add_comment result regression and the new test covers the comment-before-producer ordering that was broken.
Reviewed themes
- Confirmed the synthetic update path already consumes
commentId,itemNumber,repo, andisDiscussion, so the new tracked object shape is wired through consistently. - Checked for adjacent shape mismatches in the unresolved-ID tracking and synthetic update flow; I did not find another changed-line bug worth blocking here.
- The existing review note about checking other safe outputs still looks like follow-up work rather than a defect introduced by this patch.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 18.5 AIC · ⌖ 7.32 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
One small simplification stands out in the new add_comment tracking logic: the object and array cases are duplicated side by side instead of sharing one normalized path. net: -8 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57955 · codex · mai10 · 3.07 AIC · ⌖ 0.422 AIC · ⊞ 14K
Comment /ponytail to run again
|
|
||
| // Handle add_comment which returns an array of comments | ||
| // Handle the current add_comment result shape. | ||
| if (messageType === "add_comment" && result?.commentId && result?.repo) { |
There was a problem hiding this comment.
actions/setup/js/safe_output_handler_manager.cjs:L1170: shrink: two near-duplicate add_comment branches for object and array results. One normalized branch or helper handles both shapes once.
Test Quality Sentinel 🧪PR #57955: "Fix temporary ID substitution in safe-output comments" Analysis SummaryScope: 1 JavaScript test file modified (
Test Changes Identified
Key Test CoveragePrimary Test: "tracks a comment emitted before its temporary-ID producer" Tests the exact regression scenario from PR description: // add_comment with unresolved temporary ID reference
{ type: "add_comment", body: "Tracking issue: #aw_track1" }
// followed by create_issue that resolves the ID
{ type: "create_issue", temporary_id: "aw_track1" }Assertions:
Design Invariant: Comments emitted before their referenced issues must be tracked so the synthetic update pass can replace Quality Assessment✅ Targeted regression test - covers the exact ordering scenario from the PR description Test Quality Score: 85/100 ✅ Excellent
Recommendation: ✅ APPROVE The PR adds focused regression coverage for a critical behavior: tracking comments with unresolved temporary ID references for later update. The test properly verifies the new code path (current 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
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100. Test quality is excellent — 1/1 new tests are design tests covering high-value behavioral contracts. Inflation ratio 1.9:1 (acceptable). No violations detected. The PR adds focused regression coverage for tracking comments with unresolved temporary ID references for synthetic update.
There was a problem hiding this comment.
Reviewed the temporary-ID fix for add_comment result tracking.
The fix correctly matches the actual handler behavior: add_comment.cjs returns a single object {success, commentId, itemNumber, repo, isDiscussion, ...}, not an array, so the previous code that only checked Array.isArray(result) never matched and comments created before their referenced temp-ID producer were never tracked for synthetic update. The new branch correctly extracts commentId/itemNumber/repo/isDiscussion from the object result, and the added regression test exercises the exact ordering scenario described in the PR.
One non-blocking follow-up: the legacy Array.isArray(result) branch appears to now be dead code for add_comment (see inline comment) — worth a cleanup pass if no other code path returns an array.
No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 58.7 AIC · ⌖ 14.4 AIC · ⊞ 8.3K
| } | ||
|
|
||
| // Handle the legacy add_comment result shape. | ||
| if (messageType === "add_comment" && Array.isArray(result)) { |
There was a problem hiding this comment.
Now that add_comment always returns a single object (never an array — confirmed in add_comment.cjs, which never returns createdComments as an array to the caller), this Array.isArray(result) legacy branch appears to be dead code for the add_comment handler. Worth confirming whether any other handler registered for add_comment-type messages can still return an array; if not, consider removing this branch (and the corresponding _tracking bookkeeping in add_comment.cjs) in a follow-up to avoid maintaining an unreachable code path. Not blocking this fix. @copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs. This is a targeted, well-tested fix.
📋 Analysis
The root cause was correctly identified: processMessages only checked the legacy array-shaped add_comment result (comment._tracking), but the current add_comment.cjs handler returns a flat object ({ success, commentId, itemNumber, repo, isDiscussion }). Comments emitted before their referenced temporary ID was resolved were never added to outputsWithUnresolvedIds, so the synthetic update pass never ran for them.
- The new branch (lines 1170–1186) mirrors the existing object-shaped handling used for
create_issue/create_discussion(result.number && result.repo), and reusesgetContentToCheck/hasUnresolvedTemporaryIdsconsistently. - The regression test in
safe_output_handler_manager.test.cjsreproduces the exact reported ordering (add_commentbeforecreate_issueresolvesaw_track1) and asserts bothtemporaryIdMapandoutputsWithUnresolvedIdsend up correct — good use of Arrange/Act/Assert and a descriptive test name. - The legacy array-shape branch is preserved untouched, so no regression risk for callers still returning that shape (if any remain).
One reviewer comment on the PR already flags a good follow-up: auditing other safe-output handlers (create_pull_request, comment_memory, etc.) for similar shape mismatches. That's out of scope for this fix but worth a tracked follow-up issue since getContentToCheck shows at least comment_memory/create_pull_request rely on result?.managedBody, which should be double-checked against their current handler outputs.
Positive Highlights
- ✅ Root cause (shape mismatch) addressed directly, not just the symptom
- ✅ Regression test covers the exact failure scenario from the bug report
- ✅ No changes to unrelated legacy-shape handling
No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 36.6 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please review the current PR state, refresh the branch if needed, verify the current checks, and run the
|
|
🎉 This pull request is included in a new release. Release: |
Comments emitted before their referenced issue was created retained temporary IDs such as
#aw_track1. The safe-output manager only tracked the legacy array result fromadd_comment, while the current handler returns an object.Changes
add_commentresult shape when tracking unresolved references.{"type":"add_comment","body":"Tracking issue: #aw_track1"} {"type":"create_issue","temporary_id":"aw_track1"}After issue creation resolves the ID, the synthetic update pass can replace the placeholder in the existing comment.
Run: https://github.com/github/gh-aw/actions/runs/33641605189