feat(action): Improve duplicate comment UX#60
Conversation
When Warden detects a duplicate finding, instead of silently skipping: - For Warden's own comments: update the existing comment to add the new skill to the attribution (e.g., "warden: skill1, skill2") - For external comments: add an 👀 reaction to acknowledge This provides better visibility into which skills detected each issue and acknowledges when humans have already identified problems. Fixes #45 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/output/dedup.ts
Outdated
| const allSkills = [...existingSkills, newSkill].join(', '); | ||
| return body.replace(/<sub>warden:\s*[^<]+<\/sub>/, `<sub>warden: ${allSkills}</sub>`); |
There was a problem hiding this comment.
🟠 updateWardenCommentBody returns unchanged body when tag is missing (high confidence)
When a Warden comment is identified via marker only (no <sub>warden: tag), parseWardenSkills returns empty array, existingSkills.includes(newSkill) is false, and the replace at line 164 matches nothing, returning the original body unchanged. The caller at line 472 checks if (newBody) which is truthy, triggering an unnecessary API update with unchanged content.
Suggested fix: Check if the replacement actually occurred before returning
| const allSkills = [...existingSkills, newSkill].join(', '); | |
| return body.replace(/<sub>warden:\s*[^<]+<\/sub>/, `<sub>warden: ${allSkills}</sub>`); | |
| // If no existing <sub>warden:...</sub> tag exists, we can't update it | |
| if (existingSkills.length === 0) { | |
| return null; | |
| } | |
warden: notseer
- Return null from updateWardenCommentBody when no <sub>warden:...</sub> tag exists, preventing unnecessary API calls with unchanged content - Use replacer function in String.replace() to avoid special $ character interpretation in skill names - Update in-memory body after successful API update so subsequent triggers see the updated content instead of stale data Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| const duplicateIndices = DuplicateIndicesSchema.parse(JSON.parse(content.text)); | ||
| const duplicateIds = new Set<string>(); | ||
| const parsed = DuplicateMatchesSchema.parse(JSON.parse(content.text)); | ||
| const matches = new Map<string, ExistingComment>(); | ||
|
|
||
| for (const num of duplicateIndices) { | ||
| // Convert 1-based index to 0-based | ||
| const finding = findings[num - 1]; | ||
| if (finding) { | ||
| duplicateIds.add(finding.id); | ||
| for (const match of parsed) { | ||
| const finding = findings[match.findingIndex - 1]; | ||
| const existing = existingComments[match.existingIndex - 1]; | ||
| if (finding && existing) { | ||
| matches.set(finding.id, existing); | ||
| } |
There was a problem hiding this comment.
Bug: External (non-Warden) comments are assigned an empty title in fetchExistingComments, causing generateContentHash to produce a mismatched hash and breaking hash-based deduplication against new findings.
Severity: MEDIUM
Suggested Fix
Modify fetchExistingComments to attempt to parse a title from the body of external comments instead of defaulting to an empty string. For example, the first line of the comment body could be treated as the title. This would allow generateContentHash to create matching hashes for identical findings and external comments, enabling hash-based deduplication to work as intended.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/output/dedup.ts#L278-L286
Potential issue: In the `fetchExistingComments` function, when processing existing
GitHub comments that are not in the Warden format (external comments), the code fails to
extract a title. The `parseWardenComment` function returns `null` for these comments,
causing the `title` variable to be assigned an empty string. Consequently, the
`generateContentHash(title, description)` function creates a hash based on an empty
title for the external comment. This hash will not match the hash of a new finding that
has a non-empty title, even if the content is otherwise identical. This failure prevents
hash-based deduplication from working for external comments, meaning duplicates will
only be caught if LLM-based semantic deduplication is available and successful.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| console.log(`Added reactions to ${actionCounts.reacted} existing external comments`); | ||
| } | ||
| if (actionCounts.failed > 0) { | ||
| console.warn(`::warning::Failed to process ${actionCounts.failed} duplicate actions`); |
There was a problem hiding this comment.
Skipped duplicate actions not logged, reducing user visibility
Medium Severity
The processDuplicateActions function returns a skipped count, but this value is never logged in main.ts. When multiple triggers in the same run detect the same issue, duplicate actions are created but always skipped because findingToExistingComment creates in-memory comments without the body property needed for updates. Users see "Found N duplicate findings" in logs but get no indication that skill attribution updates were skipped. This contradicts the PR's goal of "providing better visibility into which skills detected each issue."


Improve the user experience when Warden detects duplicate findings.
Previously, when a finding duplicated an existing comment, Warden silently
skipped it. This left users wondering whether the skill ran at all, and
provided no visibility into cross-skill detection.
Now Warden takes action on duplicates:
to the attribution line (e.g.,
warden: security-review, code-quality)detected the issue
This provides better visibility into which skills detected each issue and
acknowledges when humans have already identified problems that Warden would
have flagged.
Fixes #45