AIP-18: Build resolve-pr-reference skill - #268
jodavis-claude wants to merge 3 commits into
Conversation
Resolves a PR reference (bare number/#123, full GitHub PR URL, or work-item ID) to exactly one owner/repo#number, or a structured not_found/ambiguous/access_denied failure. Adds resolve_pr_reference.py handling every deterministic path (ref-shape classification, gh pr view checks, use-context-file read, GitHub-search fallback, getLinkedPullRequests GraphQL lookup); the resolve-pr-reference skill wraps it with the one Jira getJiraIssueRemoteIssueLinks MCP call the script itself cannot make.
Adds the getJiraIssueRemoteIssueLinks row to work-with-Jira-tasks's Operations table and the getLinkedPullRequests row to work-with-GitHub-issues's Common Operations table, per resolve-pr-reference's dependency on both. Also adds resolve_pr_reference.py test coverage for a malformed provider regex pattern, a work-item provider with no dispatch logic, and a context file with an empty pr_url falling through to provider dispatch.
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against AIP-18's exit criteria and _spec_ReviewPrGuide.md. All 36 pytest tests pass locally (python3 -m pytest plugins/dev-team/skills/resolve-pr-reference/scripts/). Most of the deterministic-path logic (URL, bare number, context-file short-circuit, GitHub getLinkedPullRequests path, gh pr view access/not-found classification) looks correct and well tested.
However, I found a blocking correctness issue in the Jira work-item path: the script has no way to distinguish "--jira-links not supplied yet" (step 1, before the skill has queried Jira) from "--jira-links supplied as an empty array" (step 2, after Jira returned nothing) — both collapse to jira_links=[], so step 1 alone already runs the GitHub-search fallback and returns a terminal result before getJiraIssueRemoteIssueLinks is ever called. See inline comments for details and a couple of secondary reliability notes (unbounded gh pr list/gh search prs result pages, silently swallowed JSON-decode errors).
- Fix blocking correctness bug: resolve_pr_reference() no longer collapses jira_links=None (skill's step 1, before getJiraIssueRemoteIssueLinks has been called) into the same empty-list path as jira_links=[] (step 2, after Jira returned nothing). The Jira dispatch now returns a distinct status: "needs_jira_links" when no --jira-links flag was supplied, instead of eagerly running the GitHub-search fallback before the MCP call ever happens. SKILL.md's steps 1/2 and output contract updated to match, replacing the unfollowable "check the source field" instruction with a check on the new status value. - Add explicit --limit 100 to both gh search prs and gh pr list calls in the GitHub-search fallback, so a genuine match beyond gh's default 30-result page isn't silently missed. - Surface gh JSONDecodeError failures as warnings appended to the final not_found detail instead of silently discarding them, so a real gh failure (rate limiting, auth hiccup, unexpected output) is distinguishable from a genuine empty result.
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — AIP-18: resolve-pr-reference
All 4 prior review threads addressed satisfactorily and resolved:
jira_linkstri-state collapse (Priority 1 — correctness) — Fixed._resolve_jira_work_itemnow distinguishesjira_links is None(not yet supplied) fromjira_links == [](supplied, empty), returning a newneeds_jira_linksstatus instead of eagerly running the GitHub-search fallback. Verified with the newmock_run.assert_not_called()test.- SKILL.md step 1/2 instructions (Priority 1/4 — correctness & documentation) — Fixed together with #1; steps now key off the real
status: "needs_jira_links"value instead of a nonexistentsourcefield. - Missing
--limitongh search prs/gh pr list(Priority 1/3 — reliability) — Fixed. Both calls now pass--limit 100; verified by a new flag-presence test. - Silent
except json.JSONDecodeError: pass(Priority 1 — fault tolerance) — Fixed._github_search_fallbacknow returns(matches, warnings), surfacing a specific warning into the finalnot_founddetail instead of swallowing it silently.
Re-ran the full suite: python3 -m pytest plugins/dev-team/skills/resolve-pr-reference/scripts/ — 40/40 passing.
Scanned the 3 files modified since the last review push (resolve_pr_reference.py, SKILL.md, test_resolve_pr_reference.py) for new issues — none found. The diff is scoped tightly to the 4 flagged issues, with no unrelated changes.
Sign-off: approved.
Work item: AIP-18 — Build
resolve-pr-reference, a skill that resolves a PR reference (bare number/#123, full GitHub PR URL, or a work-item ID) to exactly oneowner/repo#number, or a structurednot_found/ambiguous/access_deniedfailure, so every later Code Review Helper deliverable can start from a single resolved PR instead of each re-implementing reference resolution.Changes:
plugins/dev-team/skills/resolve-pr-reference/SKILL.md(new) — skill definition documenting the CLI contract, resolution-path priority (full URL → bare number/#123→ work-item-id pattern match, with a context-file check first), the one inlinegetJiraIssueRemoteIssueLinkstool-use call the skill itself makes for the Jira path before re-invoking the script with--jira-links, and the full JSON success/failure shapes.plugins/dev-team/skills/resolve-pr-reference/scripts/resolve_pr_reference.py(new) — the deterministic-path script implementing: full-URL classification; bare-number/#123classification resolved against the current repo's origin; work-item-id pattern matching againstwork-tracking.<provider>.issue-key-pattern/recognize-patterns; ause-context-filecontext-file check that short-circuits provider dispatch whenpr_urlis already set; the Jira path (remote-links-then-GitHub-search-fallback); the GitHub path (gh api graphqlreadingclosedByPullRequestsReferences);gh pr viewexistence/access checks shared by the url and number paths; and a thinmain()CLI wrapper.plugins/dev-team/skills/resolve-pr-reference/scripts/test_resolve_pr_reference.py(new) — colocated pytest suite, 36 tests, all passing, mockinggh/gitviasubprocess.run.plugins/dev-team/skills/work-with-Jira-tasks/SKILL.md(modified) — added thegetJiraIssueRemoteIssueLinksrow to the Operations table, noting it hits the Remote Issue Links API, not the Development panel's dev-status API.plugins/dev-team/skills/work-with-GitHub-issues/SKILL.md(modified) — added thegetLinkedPullRequestsrow to the Common Operations table, noting it's a plaingh api graphqlcall, not an MCP tool.Design decisions:
refas given for the context-file lookup (trusting the caller passes the canonical form), and extracts a numeric issue number for the GitHub path via a generic\d+search rather than an undocumented prefix-reconstruction scheme.gh pr viewaccess-denied vs. not-found is decided by a keyword heuristic (403/permission/access/authenticat, case-insensitive) against stderr, sincegh's own error text isn't a stable contract.ghcalls merged and deduped by PR number:gh search prs <issue_key> --repo <owner>/<repo>(title/body match) plusgh pr list --repo <owner>/<repo> --state all --json number,headRefNamefiltered locally for the issue key inheadRefName(branch-name match).jira/github(no dispatch logic) is treated asnot_foundwith a clear detail message rather than raising — out of scope for this task but tested so it fails gracefully.plugins/dev-team/.claude-plugin/plugin.json, currently1.4.1) was left untouched, pending reviewer/user confirmation on whether it's needed now.Testing completed:
python3 -m pytest plugins/dev-team/skills/resolve-pr-reference/scripts/— 36 tests, all passing, covering every resolution path except the Jira remote-links MCP branch itself (which no script can reach).