Add reusable issue duplicate checker action#283
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This is a well-structured composite action with solid error handling, good pagination guards, and a thoughtful auto-close lifecycle (thumbs-down veto, newer-comment detection, dry-run support). The dual-script design (issue_duplicate_check_openhands.py + auto_close_duplicate_issues.py) keeps concerns cleanly separated.
Two important issues to address before merging, plus a couple of minor suggestions.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the all-hands-bot feedback in c267316 and resolved the review threads. Ready for another look. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: Add reusable issue duplicate checker action
This is a well-engineered composite action. The three-mode architecture (issue-check / auto-close / remove-label), the conservative auto-close policy, the idempotency guard in post_duplicate_notice.js, the bot-comment filter in remove_duplicate_candidate_label.js, and the dry-run flag on the Python scripts are all good design choices. The prior review's four findings are addressed in commit c267316.
A few issues to look at before merging:
🔴 Bug: should_comment can be true with an empty candidate list
normalize_result does not clear should_comment when candidate_issues is empty. The only path that forces it to true requires non-empty candidates (lines 503–508), but there is no corresponding path that forces it to false when candidates are empty. If the LLM returns {"should_comment": true, "classification": "duplicate", "confidence": "low", "candidate_issues": []}, normalization leaves should_comment = true with zero candidates, and post_duplicate_notice.js then calls core.setFailed. Fix: add the same guard that already exists for auto_close_candidate:
if not normalized["candidate_issues"]:
normalized["should_comment"] = False🟠 Action version pins need verification
actions/setup-python@v6 (action.yml:95) and actions/github-script@v9 (action.yml:229) — please verify these major versions exist in the Actions marketplace. If either does not exist, the action fails on first run.
🟡 auto_close_duplicate_issues.py — canonical issue state not re-checked at close time
If the canonical issue is closed between when the duplicate-candidate label is applied and when the auto-close cron runs, close_issue_as_duplicate still closes the candidate with a message referencing a now-closed issue. A brief GET /repos/{repo}/issues/{canonical} check before closing would avoid confusing users.
🟡 No acknowledgement when newer post-notice comments block auto-close
keep_open_due_to_newer_comments removes the duplicate-candidate label silently. The thumbs-down path posts a veto note explaining why the label was removed; this path does not. The reporter has no way to know why the label disappeared. Consider posting a brief comment: "Further activity was detected — leaving this open and removing the duplicate-candidate label."
🟡 Minor: python-version: '3.13' default
Python 3.13 may not be preinstalled on all runner images. Using '3.x' as the default would pick the newest available version and avoid setup failures on older hosted runners.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the follow-up all-hands-bot feedback in 04e96ea. Verified action version pins exist, added the empty-candidate guard, canonical-issue recheck, newer-activity acknowledgement, and safer Python default. Ready for another look. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — Well-engineered composite action with a clean three-mode design, solid pagination guards, and a careful auto-close lifecycle. The following are new findings not covered by existing review threads.
New Issues
remove_duplicate_candidate_label.js — Null dereference on ghost accounts
Line 3: const commenter = context.payload.comment.user.login || "";
GitHub sets comment.user to null for comments from deleted (ghost) accounts. Accessing .login on null throws a TypeError and the step fails rather than skipping silently. Use optional chaining:
const commenter = context.payload.comment?.user?.login ?? "";The bot-filter check on line 6 also benefits from the same defensive access since normalizedCommenter is derived from commenter.
action.yml — repository unnecessarily required for remove-label mode
The repository input is declared required: true, but the remove-label step never reads it — context.repo.owner / context.repo.repo come from the GitHub Actions context instead. Callers using only remove-label must pass a value they don't actually need, which is confusing.
Options:
- Change
required: falseand add a validation step that exits ifrepositoryis empty and mode isissue-checkorauto-close. - Or document in the description that
repositoryis unused forremove-label.
action.yml — Step summary dumps raw JSON
The "Summarize auto-close run" step writes the raw JSON blob from auto_close_duplicate_issues.py straight to $GITHUB_STEP_SUMMARY. A blob of raw JSON is hard to scan at a glance in the GitHub Actions UI.
Consider emitting a short Markdown list of action/issue_number pairs instead:
python3 -m json.tool "$RUNNER_TEMP/auto-close-summary.json" >> "$GITHUB_STEP_SUMMARY"Or format as a bullet list per result for better readability.
Notes on Previous Feedback
The normalize_result concern raised earlier about should_comment being incorrectly left True when candidate_issues is empty appears to be a false positive in the current code. Lines 521–523 guard with if not normalized["candidate_issues"]: normalized["should_comment"] = False, and the subsequent override block at lines 530–535 requires and normalized["candidate_issues"] — so an empty list cannot reach the True assignment.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This action auto-closes and labels real repository issues on behalf of maintainers. The conservative design (reaction-based veto, newer-comment detection, dry-run) mitigates the core risk, but the ghost-account null dereference is a real crash path for theremove-labelmode.
VERDICT:
✅ Worth merging with the null-safety fix in remove_duplicate_candidate_label.js applied (the repository friction and summary formatting are low priority but worth a follow-up).
KEY INSIGHT: The three-mode composite design is elegant — each mode is self-contained with its own validation gate, making the action safe to extend without entangling the existing lifecycle logic.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the latest all-hands-bot feedback in 9852592 and resolved all review threads. Ready for another look. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
All three rounds of prior review feedback have been addressed in commits c267316, 04e96ea, and 9852592. The action is well-engineered: the three-mode composite design is clean, the auto-close lifecycle (reaction-based veto, newer-comment detection, canonical-issue state check, dry-run flag) is conservative and well-reasoned, idempotency in post_duplicate_notice.js is solid, and pagination guards are thorough throughout.
Three new observations below — none block merging.
🟡 normalize_result overrides the model's should_comment: false
Lines 530–535 of issue_duplicate_check_openhands.py unconditionally set should_comment = True whenever classification is duplicate or overlapping-scope, confidence is high or medium, and at least one candidate exists — regardless of what the model returned. If the model explicitly returned should_comment: false for a legitimate reason (e.g. the candidates are weak despite meeting the threshold), this override bypasses that judgment and a comment is still posted. The intent is probably to enforce consistency, but it can surprise maintainers. Consider either documenting the override clearly or limiting it to cases where the model already agreed (i.e. only force True if result.get('should_comment') was already True).
🟡 github_headers() silently falls back to unauthenticated requests
issue_duplicate_check_openhands.py lines 94–96: if GITHUB_TOKEN is absent, requests are sent without an Authorization header. The unauthenticated rate limit is 60 requests/hour — well below what a multi-query duplicate check consumes — so the failure mode is cryptic rate-limit errors rather than a clear "GITHUB_TOKEN is required" message. The action guarantees the token is always injected, so this is only a standalone-script concern, but auto_close_duplicate_issues.py raises immediately on a missing token. Consider aligning the behavior with a hard fail when the token is absent.
🟡 Fragile HTTP-status detection via string-matching
auto_close_duplicate_issues.py lines 269 and 317 detect 404 responses with "HTTP 404" in str(exc). This couples the error-handling to the exact message format produced by request_json. If that format changes, 404s would no longer be caught and both remove_candidate_label and fetch_issue would raise as unhandled RuntimeErrors instead of returning gracefully. A more robust approach: raise a custom exception subclass from request_json that carries a status_code attribute, then check exc.status_code == 404 at the call site.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Three rounds of prior review feedback are thoroughly addressed and the code is in good shape. Two observations from the most recent review remain without an explicit response: the silent-fallback-to-unauthenticated path in issue_duplicate_check_openhands.py:96 and the fragile "HTTP 404" in str(exc) pattern in auto_close_duplicate_issues.py:269,317. Both are 🟡 non-blocking but worth a brief acknowledgement before merge. Two new minor observations are noted inline below.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — Latest commit (7ca2646) fully addresses all three GHE feedback items from the previous round: prompt API URLs now use GITHUB_API_BASE_URL/GITHUB_BASE_URL constants, the self-duplicate guard in auto_close_duplicate_issues.py is in place, and the README documents the required issue_comment trigger for remove-label mode.
Three earlier 🟡 non-blocking observations remain open in their existing threads (should_comment override at line 536, github_headers silent fallback at line 97, HTTP 404 string matching at lines 269/317 in auto_close_duplicate_issues.py) — avoiding duplication here, please see those threads. One new minor observation is added inline below.
The overall design is sound: three-mode composite layout is clean, the auto-close lifecycle (reaction veto, newer-comment detection, canonical-issue state check, dry-run flag) is conservative and well-reasoned, and the idempotency logic in post_duplicate_notice.js is solid.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Action auto-closes and labels real repository issues. Risk is well-mitigated by reaction-based veto, newer-comment detection, canonical-state check, and dry-run support.
VERDICT:
✅ Worth merging — the three carryover threads are non-blocking and can be addressed in a follow-up PR.
KEY INSIGHT: The --dry-run flag exists in the script but is not wired through the action interface, leaving callers with no documented way to safely test auto-close on a real repository.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the dry-run action-interface feedback in 0c7ab9e and resolved the thread. Ready for another look. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Latest commit (0c7ab9e) resolves the one open key insight from the previous review round: --dry-run is now properly wired through the action interface via the dry-run input and passed to the auto-close script. The fix is clean and correct.
What this PR adds
A reusable GitHub composite action for AI-powered duplicate issue detection and lifecycle management. Three well-separated modes (issue-check, auto-close, remove-label) with conservative defaults, a reaction-based veto path, newer-comment detection, canonical-state guard, and idempotent duplicate notice posting. The design is pragmatic and the safety mechanisms are well-reasoned.
Status of prior review threads
Five rounds of review feedback have been iterated on. The three carryover non-blocking observations noted in the previous round remain open in their existing threads — avoiding duplication here:
- 🟡
should_commentforcedTruewhen classification + confidence conditions are met, overriding the model's returned value (issue_duplicate_check_openhands.py, ~line 534) - 🟡
github_headers()silent fallback to unauthenticated requests whenGITHUB_TOKENis missing (issue_duplicate_check_openhands.py, line 97) - 🟡 Fragile HTTP-status detection via
"HTTP 404" in str(exc)string matching (auto_close_duplicate_issues.py, lines 265 and 313)
All three are non-blocking and can be addressed in a follow-up. No new issues introduced by the dry-run wiring commit.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Action auto-closes and labels real repository issues. Risk is well-mitigated by reaction-based veto, newer-comment detection, canonical-state check, and dry-run support (now reachable via the action interface).
VERDICT:
✅ Worth merging — All five rounds of feedback are addressed or acknowledged as non-blocking. The action is conservative, well-guarded, and ready for production use.
KEY INSIGHT: The dry-run wiring is the final piece needed for safe production deployment — callers can now safely test auto-close on real repositories before enabling it.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@OpenHands Look at this comment #283 (review) and see if you can address those 3 remaining nits or maybe you did already? Then review the workflow for interpolation of outside random text. Set those in env. Then /iterate to green CI and ask all-hands-bot for review. |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
@all-hands-bot Addressed the three carryover nits in bfef818 and moved remaining GitHub expressions out of shell/script blocks into env variables. CI is green on the latest SHA. Could you review the latest changes? This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
Since the last summary, no additional code changes were made. Checklist:
Current remaining status remains: PR #283 is green on CI and waiting on the requested |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟡 Acceptable — After multiple well-executed feedback iterations, this plugin is in solid shape. A few targeted observations follow.
[IMPROVEMENT OPPORTUNITIES]
post_duplicate_notice.js:18-20 — Unreachable guard uses setFailed instead of warning
The candidates.length === 0 branch is dead code: normalize_result forces should_comment = false when candidate_issues is empty, and the step only fires when should_comment == 'true'. If somehow this guard does trigger (e.g., env var injection/corruption), failing the entire job is disproportionate compared to a warning. Consider core.warning(...) + return here to keep the overall workflow green in that edge case.
tests/test_issue_duplicate_checker.py:32 — YAML injection test is fragile against alternate block scalar styles
The test checks stripped in {"run: |", "script: |"} to detect shell block entry. YAML block scalars also include run: |-, run: >, run: >- — none of which would be detected by this check, silently leaving a gap. A stripped.startswith("run:") / stripped.startswith("script:") approach would be more resilient if new steps are added with different chomping indicators.
[TESTING GAPS]
PR Description — Verification covers syntax only, no end-to-end evidence
The Verification section in the PR description (py_compile, node --check) proves the scripts are syntactically valid, but not that the integration actually works: the duplicate-check conversation flow, auto-close lifecycle, and idempotent comment logic are untested against a real GitHub repo and OpenHands API. This is understandable for an API-dependent integration, but a note about manual end-to-end testing (or a link to an agent conversation that exercised it) would strengthen confidence at merge time.
[RISK ASSESSMENT]
-
[Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUMThis action requires
issues: write+OPENHANDS_API_KEY, and the happy path auto-closes issues based on LLM classification. The safeguards are well-designed (3-day cooling period, author thumbs-down veto, newer-comment detection, explicitconfidence: highgate, dry-run mode), but LLM-assisted state mutations on a live issue tracker carry inherent non-zero risk. Recommend running a dry-run pass on a representative repository before enabling auto-close in production.
VERDICT: ✅ Worth merging — Core logic is sound; the two minor suggestions above are non-blocking.
KEY INSIGHT: The layered normalization in normalize_result combined with the env-var isolation pattern in action.yml is the right approach for a security-sensitive composite action; the JS guard on line 18 is the only place where those two safety layers fell slightly out of sync.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
@all-hands-bot Addressed the latest inline feedback in 13558a5, resolved the review threads, and updated the PR description with links showing this was extracted from workflows already used in OpenHands/software-agent-sdk and OpenHands/OpenHands. CI is green on the latest SHA. Could you review again? This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: Add reusable issue duplicate checker action
All three outstanding non-blocking observations from the previous review (0c7ab9e) have been resolved in commits bfef8184 and 13558a56:
- ✅
should_commentoverride → respects model value —normalize_result()now preserves the model'sshould_comment=Falseand only suppresses it when confidence is insufficient. Theauto_close_candidatecascade (if normalized["auto_close_candidate"] and not normalized["should_comment"]) follows correctly. - ✅
github_headers()silent fallback → explicit error —GITHUB_TOKENabsence now raisesRuntimeError("GITHUB_TOKEN environment variable is required"), making the failure surface immediately rather than generating unauthenticated requests. - ✅ Fragile string-match HTTP-status detection → structured
HTTPError—auto_close_duplicate_issues.pynow has a properHTTPErrorclass withstatus_code,method,path, andbodyattributes.remove_candidate_labelandfetch_issuecheckexc.status_code == 404directly instead of"HTTP 404" in str(exc).
Additional improvements in this round
- Script paths promoted to env:
${{ github.action_path }}/scripts/...expressions insiderun:blocks are now routed throughDUPLICATE_CHECK_SCRIPT/AUTO_CLOSE_SCRIPTenv vars, closing the expression-injection vector thattest_action_shell_blocks_do_not_interpolate_expressions_directlywas designed to detect. - Zero-candidate handling:
core.setFailed→core.warninginpost_duplicate_notice.jsis the correct call — an empty candidate list whenshould_comment=trueis a data inconsistency worth logging, not a workflow failure. - Test coverage: Four new focused unit tests cover the structured
HTTPError,github_headerstoken-required validation,normalize_resultmodel-value preservation, andfetch_issue404 handling. The shell-block expression scanner was hardened to recogniserun: >andscript: >block forms in addition torun: |/script: |.
Remaining observation (non-blocking)
issue_duplicate_check_openhands.py still raises a generic RuntimeError for HTTP errors, while the companion script now has a structured HTTPError. This asymmetry is minor — the OpenHands-side script's HTTP paths don't currently need per-status-code branching — but it would be easy to unify if the scripts are ever refactored into a shared library.
Verdict
🟢 Merge-ready — all previously identified issues are resolved, new tests validate the key invariants, and no new problems were introduced.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
@all-hands-bot Addressed the remaining non-blocking observation in 4363c3c by adding structured This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Overall Assessment
This is a well-structured addition of a reusable composite action for AI-powered duplicate issue detection. The code is clean, uses stdlib-only Python, has thorough error handling, and — critically — routes all action inputs through environment variables rather than interpolating them directly into shell scripts. That last property is important for injection safety and is even enforced by an automated test (test_action_shell_blocks_do_not_interpolate_expressions_directly), which is great.
A few issues need attention before merging:
- Action version tags – both
actions/setup-python@v6andactions/github-script@v9reference versions that don't appear to exist (latest stable arev5andv7respectively). This would cause a runtime failure. - Re-run idempotency – when a duplicate-check comment already exists with different canonical/auto-close metadata,
post_duplicate_notice.jsfails the step instead of updating the comment. If the LLM produces slightly different results on a re-run, the workflow is stuck for that issue until someone manually edits the comment. - Maximum total wait time – the
--max-wait-secondsdefault is 900 s, but the help text itself warns the total runtime can approach twice that (~30 min). This is close to many teams' default job-level timeout and is worth calling out in the README. - Test coverage – 6 tests cover ~1 250 lines of Python. Key logic paths (
parse_agent_json,normalize_resultbranches, auto-close decision branches) are untested.
Detailed inline comments follow.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
@all-hands-bot Addressed the valid latest feedback in 68ba744: existing duplicate-check comments are updated/reconciled on metadata changes, polling timeout inputs are exposed and documented, and targeted test coverage was expanded. I verified the action version comments were outdated with the GitHub API and replied with tag refs. CI is green and all threads are resolved. Could you review again? This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: Add reusable issue duplicate checker action
The four new commits since the last review round (bfef818, 13558a5, 4363c3c0, 68ba744) systematically address every open item from the prior round:
| Prior thread | Commit |
|---|---|
should_comment forced true overrides model judgment |
bfef818 |
github_headers() silent fallback to unauthenticated requests |
bfef818 |
Fragile HTTP 404 string-matching in auto_close_duplicate_issues.py |
bfef818 / 4363c3c0 |
core.setFailed on empty candidates |
13558a5 |
| YAML block-scalar detection in the injection test | 13558a5 |
HTTPError alignment between the two Python scripts |
4363c3c0 |
| Hard failure when re-checking an issue with different canonical/auto-close metadata | 68ba744 |
poll-interval-seconds / max-wait-seconds not exposed as action inputs |
68ba744 |
| Test coverage gaps | 68ba744 |
Spot-check of new code
bfef818 — normalize_result now correctly preserves should_comment=False
The updated logic only ever forces should_comment to false (empty candidates, weak classification, low confidence); it never forces it to true. test_normalize_result_preserves_model_should_comment_false confirms the fix. ✓
4363c3c0 — HTTPError class added to issue_duplicate_check_openhands.py
The new class mirrors the one in auto_close_duplicate_issues.py (both expose status_code; the url vs. path field name difference is minor and pre-existing). test_issue_check_request_json_raises_structured_http_error covers the path. ✓
68ba744 — update-in-place instead of failing on re-run
post_duplicate_notice.js now updates an existing comment and reconciles the duplicate-candidate label when canonical number or auto-close flag changes. The ensureCanonicalIssueIsOpenIssue() call runs before the comment-search loop, so the update path inherits that validation. ✓
68ba744 — test expansion
test_find_latest_auto_close_comment_uses_newest_auto_close_marker, test_normalize_result_guards_auto_close_and_infers_canonical_issue, test_build_prompt_includes_required_schema_keys, and test_parse_agent_json_extracts_fenced_json close the coverage gaps flagged in the previous round. The timestamp-selection test correctly verifies that a comment with a null created_at is skipped and the latest-dated entry wins. ✓
One non-blocking observation
candidate_issues entries are unvalidated in normalize_result: The list is truncated to 3 but individual entries are passed through without field-level validation. If the LLM returns a malformed entry missing number or url, post_duplicate_notice.js will silently render undefined in the posted notice. Requires the LLM to produce malformed output — a corner case suitable for a follow-up PR.
Risk assessment
[Overall PR]
The action auto-closes and labels real repository issues. Risk is well-mitigated by the reaction-based veto, newer-comment detection, canonical-state check, dry-run support (now reachable via the action interface), and the new idempotent update-in-place behavior for re-runs.
Verdict
✅ LGTM — ready to merge. Six rounds of thorough review feedback have been iterated on and resolved. The three-mode composite design is clean, the safety mechanisms are conservative and well-reasoned, and the new test suite gives meaningful coverage of the critical normalization and auto-close logic.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
3 participants
Summary
plugins/issue-duplicate-checkercomposite actionVerification
python3 -m py_compile plugins/issue-duplicate-checker/scripts/*.pynode --check plugins/issue-duplicate-checker/scripts/post_duplicate_notice.jsnode --check plugins/issue-duplicate-checker/scripts/remove_duplicate_candidate_label.jsplugins/issue-duplicate-checker/action.ymlwith PyYAMLPrior real-world usage
OpenHands/software-agent-sdk:issue-duplicate-checker.ymlandremove-duplicate-candidate-label.yml.OpenHands/OpenHands:issue-opened.ymlandremove-duplicate-candidate-label.yml.For review, if you don’t see major issues, you should send event APPROVED so we can merge it.
This PR was created by an AI agent (OpenHands) on behalf of the user.
@enyst can click here to continue refining the PR