fix(#847): use closing-reference queries for PR-issue matching - #887
Conversation
|
/fs-fix Add unit tests in
All tests should use the existing |
|
🤖 Finished Fix · ✅ Success · Started 11:43 AM UTC · Completed 11:49 AM UTC Commit: |
🔧 Fix agent — iteration 1 (human-triggered)Added 6 unit tests covering all 3 requested gaps: false-positive regression (3 tests for issue #847 scenarios), closed/merged PR filtering (2 tests for MERGED and CLOSED states), and positive closing-keyword match (1 happy-path test). All 33 tests pass. Fixed (3):
Tests: passedNext steps:
|
Add 6 new tests to pre-code-test.sh covering review-requested gaps: - False-positive regression (issue #847): verify issue #1 is not blocked by a PR referencing #12 in its title, issue #42 is not blocked by a "Related: #42" mention without a closing keyword, and issue #1 is not blocked by a PR mentioning #10 (substring false positive). - Closed/merged PR filtering: verify PRs with state MERGED or CLOSED are filtered out by the jq .state == "OPEN" selector. - Positive closing-keyword match: verify an OPEN human PR returned by closedByPullRequestsReferences still blocks (happy path). Addresses review feedback on #887
|
🤖 Finished Review · ✅ Success · Started 11:51 AM UTC · Completed 12:27 PM UTC Commit: |
|
Note: The dispatch-level PR check in |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Next steps:
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 12:29 PM UTC · Completed 12:34 PM UTC Commit: |
🔧 Fix agent — iteration 2 (bot-triggered)Addressed 2 of 3 review findings. Fixed the GitLab closing-keyword regex to support optional colon (matching GitLab's default closing pattern) and renamed search_term to issue_number for consistency with the GitHub implementation. Disagreed with the test-inadequate finding — the fp-issue847-* tests correctly test the empty-response path since filtering happens server-side. All 32 pre-code tests and all post-code tests pass. Fixed (2):
Disagreed (1):
Tests: passedNext steps:
|
- Add :? to the GitLab closing-keyword regex to match GitLab's default closing pattern which allows an optional colon (e.g., "Closes: #42") - Rename search_term to issue_number in the GitLab forge_list_prs_for_issue() to match the GitHub implementation's naming convention Addresses review feedback on #887
|
🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:55 PM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-fix Address the two remaining low-severity review findings:
|
|
🤖 Finished Fix · ✅ Success · Started 2:29 PM UTC · Completed 2:36 PM UTC Commit: |
- Use -F flags to pass owner, name, and issue_number as proper GraphQL variables instead of shell-interpolating them into the query string, eliminating an injection vector in the gh api graphql call. - Rename jq --arg alias from "term" to "issue_number" in the GitLab forge_list_prs_for_issue to match the renamed shell variable. - Propagate both changes to bundled scripts via make script-build. Addresses review feedback on #887
🔧 Fix agent — iteration 3 (human-triggered)Addressed both remaining low-severity review findings: (1) replaced shell interpolation with proper GraphQL variables in the GitHub forge_list_prs_for_issue function, and (2) renamed the jq --arg alias from 'term' to 'issue_number' in the GitLab forge_list_prs_for_issue function. Changes propagated to pre-code.sh and post-code.sh via make script-build. All 32 pre-code tests pass. Fixed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 2:38 PM UTC · Completed 2:57 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Fix · ✅ Success · Started 2:59 PM UTC · Completed 3:08 PM UTC Commit: |
…yword - Extend GitLab closing-keyword regex to match comma-separated issue lists (e.g., "Closes #41, #42") by adding a repeating group for preceding issue refs separated by commas - Remove implement(?:s|ed|ing)? from keyword list — GitLab only recognizes close/fix/resolve variants as closing keywords - Update stale block comment describing the old search strategy - Update SKILL.md to replace stale gh pr list --search pattern with closedByPullRequestsReferences GraphQL query Addresses review feedback on #887
🔧 Fix agent — iteration 4 (bot-triggered)Addressed 4 of 6 review findings with code fixes. Fixed the GitLab closing-keyword regex to handle comma-separated issue lists and removed the invalid 'implement' keyword. Updated stale comments and SKILL.md documentation. Disagreed with 2 findings: regex-injection (caller already validates digits-only) and protected-path (informational, requires human approval). Fixed (4):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Review · Commit: |
Replace the text-search approach in forge_list_prs_for_issue with semantically correct closing-reference queries on both forges: GitHub: Use the closedByPullRequestsReferences GraphQL field on the Issue type, which returns only PRs with closing keywords (Fixes #N, Closes #N, etc.) targeting the issue. This matches the approach already used by the dispatch-level check in reusable-dispatch.yml. GitLab: Change the jq filter from matching any #N reference to matching only closing keywords (Close/Fix/Resolve/Implement variants) before the issue reference. Case-insensitive matching with word boundaries prevents both false positives from unrelated mentions and substring collisions. The previous text-search approach (gh pr list --search "N in:body,title") matched any PR containing the issue number as a substring, causing false positives: issue #1 matched PR #13 (titled "docs(#12): ...") and issue #6358 matched PR #6355 (which mentioned #6358 in a "Related" section). Update pre-code-test.sh mock to handle gh api graphql and use GraphQL response format for test data. Note: pre-commit could not run in sandbox (network restriction). The post-script runs an authoritative pre-commit check on the runner. Closes #847
Add 6 new tests to pre-code-test.sh covering review-requested gaps: - False-positive regression (issue #847): verify issue #1 is not blocked by a PR referencing #12 in its title, issue #42 is not blocked by a "Related: #42" mention without a closing keyword, and issue #1 is not blocked by a PR mentioning #10 (substring false positive). - Closed/merged PR filtering: verify PRs with state MERGED or CLOSED are filtered out by the jq .state == "OPEN" selector. - Positive closing-keyword match: verify an OPEN human PR returned by closedByPullRequestsReferences still blocks (happy path). Addresses review feedback on #887
- Add :? to the GitLab closing-keyword regex to match GitLab's default closing pattern which allows an optional colon (e.g., "Closes: #42") - Rename search_term to issue_number in the GitLab forge_list_prs_for_issue() to match the GitHub implementation's naming convention Addresses review feedback on #887
- Use -F flags to pass owner, name, and issue_number as proper GraphQL variables instead of shell-interpolating them into the query string, eliminating an injection vector in the gh api graphql call. - Rename jq --arg alias from "term" to "issue_number" in the GitLab forge_list_prs_for_issue to match the renamed shell variable. - Propagate both changes to bundled scripts via make script-build. Addresses review feedback on #887
…yword - Extend GitLab closing-keyword regex to match comma-separated issue lists (e.g., "Closes #41, #42") by adding a repeating group for preceding issue refs separated by commas - Remove implement(?:s|ed|ing)? from keyword list — GitLab only recognizes close/fix/resolve variants as closing keywords - Update stale block comment describing the old search strategy - Update SKILL.md to replace stale gh pr list --search pattern with closedByPullRequestsReferences GraphQL query Addresses review feedback on #887
Use -f (string) for owner/name and -F without := for number. The := raw-JSON prefix with -F passes null for integer variables in gh api graphql; -F without := auto-detects the type correctly. Addresses review feedback on #887
Update the build_mock() docstring in pre-code-test.sh to reference "gh api graphql" instead of the stale "gh pr list", and rename the pr_list_output variable to graphql_output throughout the file to match the current mock implementation. Addresses review feedback on #887
d188633 to
e9eff25
Compare
|
🤖 Finished Review · ✅ Success · Started 8:45 AM UTC · Completed 9:02 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pass bot_login and coder_bot_login via jq --arg flags instead of shell interpolation for defense-in-depth consistency with the GitLab implementation. Update the test mock to forward --arg bindings to jq. Signed-off-by: Marta Anon <manon@redhat.com>
|
🤖 Review · Commit: |
4227516 to
a7d0645
Compare
|
🤖 Review · Commit: |
|
🤖 Review · ❌ Terminated · Started 9:15 AM UTC · Ended 9:33 AM UTC Commit: |
|
Addressed review 5 findings: |
|
🤖 Finished Review · ✅ Success · Started 9:15 AM UTC · Completed 9:33 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 10:47 AM UTC · Completed 10:59 AM UTC Commit: |
|
Retro: PR #887 — fix(#847): use closing-reference queries for PR-issue matching PR #887 fixed a false-positive bug in What went well:
Key improvement area: The code agent's initial PR omitted regression tests despite the code-implementation skill mandating "every behavioral change must have a corresponding test change" and despite the triage output recommending specific false-positive test scenarios. This forced an extra Evidence for existing open issues:
Proposals filed
|
2 participants
Summary
gh pr list --search "N in:body,title"with a GraphQL query usingclosedByPullRequestsReferenceson the Issue type. This returns only PRs with closing keywords (Fixes #N,Closes #N, etc.) targeting the issue — the same approach used by the dispatch-level check inreusable-dispatch.yml.#Nreference to matching only closing keywords (Close/Fix/Resolve/Implementvariants) before the issue reference. Case-insensitive matching with word boundaries prevents false positives.pre-code-test.shmock to handlegh api graphqland use GraphQL response format for test data.Root cause
The pre-script's
forge_list_prs_for_issueusedgh pr list --search "${ISSUE_NUMBER} in:body,title"— a raw text search that matched any PR containing the issue number as a substring. This caused false positives:#1matched PR#13(titleddocs(#12): ...) because1appears in#12#6358matched PR#6355because#6355mentioned#6358in a "Related" sectionTesting
pre-code-test.shtests pass (source mode)post-code-test.shtests passmake check-bundleconfirms bundled scripts are in syncCloses #847
Post-script verification
agent/847-fix-pr-issue-match)c981f987117a78816483c96d5256b302b1b1cfca..HEAD)