AR-273: advance factory issues after PR merge#361
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes. 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 (2)
✨ 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 |
![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 automatic advancement of Linear issues to the 'Done' state when their associated GitHub PR is merged, along with duplicate event suppression and support for re-dispatching issues moved back from 'Human Review' to 'Ready for Agent'. The review feedback highlights three important issues: a bug where newly ingested issues are incorrectly flagged as reopened when the human review state is unconfigured, a performance bottleneck where all issue files are read from disk during PR merge processing, and a potential memory leak from an unbounded cache set.
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 reopenedFromTerminal = previousStateId === this.#config.stateIds.done || | ||
| previousStateId === this.#config.stateIds.humanReview |
There was a problem hiding this comment.
When an issue is ingested for the first time, previousStateId is undefined. If this.#config.stateIds.humanReview is also undefined (which is common if the operator has not configured it), the comparison previousStateId === this.#config.stateIds.humanReview will evaluate to true. This incorrectly flags the issue as reopenedFromTerminal and resets the dispatch attempts. Adding a guard to ensure previousStateId !== undefined prevents this bug.
const reopenedFromTerminal = previousStateId !== undefined &&
(previousStateId === this.#config.stateIds.done ||
previousStateId === this.#config.stateIds.humanReview)| for (const path of await this.#mount.listTree(ISSUE_ROOT)) { | ||
| if (!isIssueFilePath(path)) { | ||
| continue | ||
| } | ||
| const issue = await this.#readIssue(path) | ||
| if (!issue || !upstreamStates.has(issue.stateId)) { | ||
| continue | ||
| } | ||
| if (!isRealLinearIssue(issue) || !isInFactoryScope(issue, this.#config.safety)) { | ||
| continue | ||
| } | ||
| if (prSnapshotReferencesIssue(snapshot, issue.key)) { | ||
| return issue | ||
| } | ||
| } |
There was a problem hiding this comment.
Currently, #findMergeAdvanceIssueForPr reads every single issue file in the repository (await this.#readIssue(path)) to check if it matches the PR snapshot. In repositories with many issues, this O(N) disk/mount I/O operation on every PR merge event will cause significant latency and block the event loop.
Since keyFromPath(path) is a lightweight string operation, we can check if the PR snapshot references the issue key before reading the file. This optimizes the search to O(1) file reads.
for (const path of await this.#mount.listTree(ISSUE_ROOT)) {
if (!isIssueFilePath(path)) {
continue
}
const issueKey = keyFromPath(path)
if (!prSnapshotReferencesIssue(snapshot, issueKey)) {
continue
}
const issue = await this.#readIssue(path)
if (!issue || !upstreamStates.has(issue.stateId)) {
continue
}
if (!isRealLinearIssue(issue) || !isInFactoryScope(issue, this.#config.safety)) {
continue
}
return issue
}| this.#increment('mergedPrAdvanceDuplicatesSuppressed') | ||
| return | ||
| } | ||
| this.#postMergeDoneAdvances.add(advanceKey) |
There was a problem hiding this comment.
The #postMergeDoneAdvances Set is used to suppress duplicate merge events, but it grows indefinitely as the long-running daemon processes more PRs over time. To prevent a slow memory leak, we should bound the Set's size by evicting the oldest entry when it exceeds a reasonable threshold.
this.#postMergeDoneAdvances.add(advanceKey)
if (this.#postMergeDoneAdvances.size > 5000) {
const oldest = this.#postMergeDoneAdvances.values().next().value
if (oldest !== undefined) {
this.#postMergeDoneAdvances.delete(oldest)
}
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26a421beb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.#postMergeDoneAdvances.add(advanceKey) | ||
|
|
||
| try { | ||
| await this.#linear.setState(issue, this.#config.stateIds.done) |
There was a problem hiding this comment.
When a merged PR's branch name does not contain the issue key but its title/body does, record is undefined even though #findMergeAdvanceIssueForPr can still find the Agent Implementing issue from the mount. This direct setState marks Linear Done without going through #completeIssue, so the implementer/reviewer are not released and the batch slot remains in-flight. Please check the matched issue against #batch and complete that record before taking the direct persisted-state path.
Useful? React with 👍 / 👎.
| if (!isRealLinearIssue(issue) || !isInFactoryScope(issue, this.#config.safety)) { | ||
| continue | ||
| } | ||
| if (prSnapshotReferencesIssue(snapshot, issue.key)) { |
There was a problem hiding this comment.
Because the live/default subscription watches all /github/repos/** PR paths, this content-only issue-key match can advance a Human Review or Agent Implementing Linear issue from a merged PR in an unrelated mounted repository that merely mentions the same key. The existing PR resolvers limit candidates to configured repos; carry the repo from githubPullPathParts into this lookup and reject repos outside the factory route/config before accepting the match.
Useful? React with 👍 / 👎.
|
Closing as superseded by #362. |
|
Findings High: packages/factory-sdk/src/orchestrator/factory.ts can treat first-seen issues as reopened from Human Review when Addressed comments
No files were edited. Verification Passed locally: Remote GitHub check completion was not verified, and I did not run the macOS packaging smoke job in this Linux sandbox. |
1 participant
Summary
Tests
Notes
stateIds.humanReviewremains operator-configured rather than baked intoLINEAR_STATE_IDS.