Fix synthetic probe PR close matching#280
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kjgbot
added
the
no-agent-relay-review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a scoring mechanism to match and close synthetic probe PRs based on issue key references in the branch name, title, or body, and adds a new issue-key-match utility. The review feedback highlights two logical bugs in the regular expression lookaheads within issue-key-match.ts that could cause incorrect matches, and suggests a performance optimization in factory.ts to parallelize sequential I/O operations when reading PR candidates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| const prefix = escapeRegex(parts[1] ?? '') | ||
| const number = escapeRegex(parts[2] ?? '') | ||
| return new RegExp(`(^|[^A-Za-z0-9])${prefix}-${number}(?=$|[^A-Za-z0-9]|-(?!\\d))`, 'i').test(value) |
There was a problem hiding this comment.
The regular expression lookahead (?=$|[^A-Za-z0-9]|-(?!\d)) contains a logical bug. Because [^A-Za-z0-9] matches any non-alphanumeric character (including hyphens), any hyphen will immediately satisfy this part of the lookahead, completely bypassing and rendering the -(?!\d) check redundant.
To fix this and ensure that a hyphen followed by a digit (e.g., ar-22-9) does not incorrectly match AR-22, the hyphen must be excluded from the character class.
Suggest updating the lookahead to (?=$|[^A-Za-z0-9-]|-(?!\d)).
| return new RegExp(`(^|[^A-Za-z0-9])${prefix}-${number}(?=$|[^A-Za-z0-9]|-(?!\\d))`, 'i').test(value) | |
| return new RegExp(`(^|[^A-Za-z0-9])${prefix}-${number}(?=$|[^A-Za-z0-9-]|-(?!\\d))`, 'i').test(value) |
|
|
||
| const prefix = escapeRegex(parts[1] ?? '') | ||
| const number = escapeRegex(parts[2] ?? '') | ||
| const issue = `${prefix}-${number}(?=$|[^A-Za-z0-9]|-(?!\\d))` |
There was a problem hiding this comment.
The lookahead in the explicit issue reference regex has the same logical bug where [^A-Za-z0-9] matches hyphens, making the -(?!\d) check redundant.
Suggest updating the lookahead to (?=$|[^A-Za-z0-9-]|-(?!\d)) to correctly prevent partial matches on hyphens followed by digits.
| const issue = `${prefix}-${number}(?=$|[^A-Za-z0-9]|-(?!\\d))` | |
| const issue = `${prefix}-${number}(?=$|[^A-Za-z0-9-]|-(?!\\d))` |
| for (const repo of reposFromConfig(config)) { | ||
| for (const path of await mount.listTree(githubPullRoot(repo))) { | ||
| if (!path.endsWith('.json')) continue | ||
| const pr = await readProbePrCandidate(mount, path) | ||
| if (!pr || !issuePrMatchesIssue(pr, issue, opts.titleMarker ?? config.safety.requireTitlePrefix, opts)) continue | ||
| candidates.push({ repo, prNumber: pr.number }) | ||
| const score = pr | ||
| ? issuePrMatchScore(pr, issue, opts.titleMarker ?? config.safety.requireTitlePrefix, opts) | ||
| : 0 | ||
| if (!pr || score <= 0) continue | ||
| candidates.push({ repo, prNumber: pr.number, score }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Reading each PR candidate sequentially with await readProbePrCandidate(mount, path) inside a nested loop can cause significant performance bottlenecks due to sequential I/O operations (especially if mount.readFile involves network latency).
Suggest parallelizing the PR candidate reading using Promise.all to fetch and score the candidates concurrently.
for (const repo of reposFromConfig(config)) {
const paths = (await mount.listTree(githubPullRoot(repo))).filter((path) => path.endsWith('.json'))
const results = await Promise.all(
paths.map(async (path) => {
const pr = await readProbePrCandidate(mount, path)
if (!pr) return null
const score = issuePrMatchScore(pr, issue, opts.titleMarker ?? config.safety.requireTitlePrefix, opts)
return score > 0 ? { repo, prNumber: pr.number, score } : null
})
)
for (const cand of results) {
if (cand) candidates.push(cand)
}
}
|
Implemented one safety fix in the PR scope:
Addressed comments
Advisory Notes
VerificationLocal commands run:
Remote PR metadata currently reports PR #280 is already merged and closed, so I’m not marking this as needing human-ready handoff. |
1 participant
Summary
Fixes the synthetic close-probe path so completed
[factory-e2e]issues close their PRs even when real agents use natural PR titles.#isSyntheticProbeIssueas the safety boundary: only synthetic issue titles enter close-never-merge handling.ar-229-is-positive, with numeric-prefix protection soAR-22does not matchAR-229.Linear: AR-235.closeProbePrstrict unless the issue-gated caller explicitly opts out of title-marker enforcement.Mutation / Regression Proofs
Linear: AR-235(Add isOdd factory SDK util #277 shape)AR-236: ...(AR-236: add square utility #278 shape)ar-229-is-positive(Add isPositive factory SDK util #279 shape)[factory-e2e]issue does not close a PR.AR-22/AR-229prefix collisions and loose body mentions do not win over the exact branch-convention PR.Validation
npm run typecheck:nodenpx vitest run packages/factory-sdk