Repository navigation
🔗 fix: Detect Pull Request Links in Command Output for Conversation Lanes - #16982
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 63528b9e40
ℹ️ 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".
| return reservation.then((reserved) => | ||
| reserved == null ? false : write(laneGit, pullRequest, reserved), | ||
| ); |
There was a problem hiding this comment.
When two reports for the same lane overlap, this chain orders only sequence reservations, not the resulting writes. If the first report contains a PR link but its write is slow, a later no-link report can write the higher sequence first; its database pipeline sees no existing link to preserve, and the linked report is then rejected by the sequence fence, permanently losing the detected PR. This can occur with rapid commands or sibling/replica reports, so the preservation logic needs to remain correct when writes land out of order.
Useful? React with 👍 / 👎.
| if ( | ||
| pull.headRef === branch && | ||
| (sameRepository || (recorded != null && (await contains(pull.headSha)))) |
There was a problem hiding this comment.
When a retained link points to a fork PR that no longer contains the recorded commit, validating it consumes one of maxHeadComparisons before falling back to the normal branch search. With the supported value maxHeadComparisons: 1, a valid branch candidate whose newer head contains the recorded commit can no longer be compared and is incorrectly missed; higher limits similarly search one fewer candidate. Since the contract says an invalid link falls back as if no link had been seen, this validation should not deplete the fallback search budget.
Useful? React with 👍 / 👎.
| ], | ||
| }, | ||
| { $set: { laneGit: next } }, | ||
| [{ $set: { laneGit: next } }], |
There was a problem hiding this comment.
This array-form update is an aggregation-pipeline update and also evaluates $$REMOVE; supported Amazon DocumentDB 5.0 rejects both, so every setConvoLaneGit call fails there and conversation lanes never persist their branch or PR state. The repository's existing src/methods/documentdb.spec.ts confirms this regression by reporting this updateOne, both $$REMOVE uses, and the $set stage, so the required static-check gate also fails until this is expressed with compatible update operations.
AGENTS.md reference: AGENTS.md:L67-L69
Useful? React with 👍 / 👎.
| if ( | ||
| pull.headRef === branch && | ||
| (sameRepository || (recorded != null && (await contains(pull.headSha)))) | ||
| ) { | ||
| return report(pull); |
There was a problem hiding this comment.
When a linked PR has been merged and the same branch name is later reused for a new open PR, the stored link is retained because the branch and repository did not change. This early return accepts the old merged PR solely from its matching head ref/repository and never reaches the open-first search below, even when the new open PR contains the recorded head, so the conversation permanently displays the stale merged PR unless a command happens to print the new URL.
Useful? React with 👍 / 👎.
| branch: { $literal: laneGit.branch }, | ||
| head: { $literal: laneGit.head }, | ||
| ...(repo ? { repo: { $literal: repo } } : {}), | ||
| pullRequest: pullRequest != null ? { $literal: pullRequest } : keptPullRequest, |
There was a problem hiding this comment.
Any same-repository PR URL found in command output unconditionally replaces the stored link, before GitHub verifies that the PR belongs to the current branch. If a lane already stores its valid PR and a later command merely prints a URL for another branch's PR, the unrelated number replaces the valid one; lookup then rejects it for the branch mismatch, but the previous number is gone, and an unpushed current head prevents the fallback search from rediscovering the correct PR. Preserve the prior association until the new candidate is validated, or retain the candidate separately from the last validated link.
Useful? React with 👍 / 👎.
A conversation's pull request was found only by searching its branch for a PR whose head contains the recorded commit, so a single unpushed local commit hid an existing PR. The attached bash tool now hands its output to the lane recorder, which records a github.com pull request link to the lane's repository with the lane state. The link is kept across later reports on the same branch and repository and dropped when either changes. The GitHub source reads a linked PR directly and accepts it when its head is the lane branch in the lane repository; a fork's PR must carry the recorded commit. The single and batch routes both pass the link, and the batch route keys lanes by it.
63528b9 to
98548ce
Compare
Pull Request
Summary
A conversation's pull request is found only by searching its lane branch on GitHub for a pull request whose head contains the commit the lane last reported. The first commit the agent makes after opening the pull request is one GitHub does not know yet, so the comparison fails and the pull request disappears until the next push. The command output that already names the pull request, such as the URL
gh pr createprints, is never used.The attached bash tool now hands its output to the lane recorder, which records a
github.compull request link to the lane's own repository alongside the branch and head. The link is kept across later reports on the same branch and repository and dropped when either changes. The lookup reads a linked pull request directly and accepts it when its head is the lane branch in the lane repository; a fork's pull request must still carry the recorded commit. The single and batch pull request routes both pass the link. Anything else falls back to the existing branch search.Related to #16793
Type of change
Testing
Tested environments/configuration:
endpoints.agents.pullRequests.enabled: truewith an attached code workspace; MongoDB throughmongodb-memory-serverAutomated tests:
packages/api:npx jest src/pulls src/codepasses (1062), with new cases for link detection, the direct read, same-repository and fork verification, cache and batch keying, and the recorder hand-offpackages/data-schemas:npx jest src/methods/conversation.spec.tspasses (367), with new cases for keeping, replacing and dropping the link, the batch read and a sequence-fenced writenpx tsc --noEmitinpackages/apiandpackages/data-schemas, ESLint, Prettier and import sorting on the changed filesScreenshots / recordings
No user-facing change
Risk / compatibility
setConvoLaneGitnow writes with an update pipeline so a report without a link can keep the stored one; branch, head and repository values are wrapped in$literal. The newlaneGit.pullRequestfield is optional and absent on existing documents. The linked read adds at most one GitHub request per uncached lookup.Checklist