Skip to content

Pull request lookup follow-ups from #16793 #16807

Closed as not planned
Closed as not planned

Description

@berry-13

Follow-ups from the review of #16793 (conversation pull request lookup). None blocks the feature; each is a known limit.

  1. Epoch and workspace come from separate snapshots. resolveCodeExecutionWorkspaceContext captures the attachment, then getConvoLaneContext reads the attachment epoch. An A to B to A move between the two reads would satisfy both fences. Fix: return the epoch with the persisted workspace decision (shared decision path in packages/api/src/code).
  2. Fork pull request check runs. Checks are read from the base repository. Sampled on this repo: Add Deploy to Railyard button #16787 has its check runs on the base repo, Port company upload-menu customizations to v0.8.7 release baseline #16776 on the fork. A fork PR whose checks live on the fork reads as none. Fix: keep the head repository from the pull detail and read checks there, under an explicit authorization scope.
  3. Proxy-aware transport. The route passes global fetch to createGitHubPullRequestSource. Deployments behind a proxy need the proxy dispatcher injected at that one argument.
  4. History search bound. A branch name reused more than maxCandidatePullRequests times can hide an older matching pull request. Pagination within a configured budget would remove the limit.
  5. Subagent lane context is two reads. getConvoLaneContext reads the thread, then its root. Resolving both in one operation needs a tenant-safe approach, since the tenant plugin only filters the base collection of an aggregate $lookup.
  6. Headerless secondary limit on a 429. The 429 branch is classified before the body-based detector, so it gets the 10 second cooldown, not 60. Fix: run the body check for 429 too.
  7. Check-run 404 reads as no checks. getJson maps 404 to null, so a head that vanished between detail and checks reports none (or a partial passing) and is cached. Fix: treat a check-run 404 as UPSTREAM_ERROR.
  8. Unborn lane (head: null) can match an old PR. With a branch and no commit, every candidate is accepted and closed history can show a previous incarnation. Fix: return null when the reported head is null.
  9. Shared-token capacity. Two principals with the same token and different cacheMaxEntries share one partition, and the smaller trims it. Fix: keep the largest capacity seen per token.
  10. Open fork PRs vs closed base PRs. Fork candidates are searched after the closed base list, so a closed base PR can beat an open fork PR. Fix: search open fork candidates before closed ones.
  11. Identical lanes across concurrent batch requests. Conversations on one repository, branch and head share one slot within a request, but two concurrent requests each take their own slot before the lookup's shared in-flight promise joins them. With a low maxConcurrentLookups a second request for the same lane waits instead of joining. Fix: keep a handler-wide in-flight map keyed by the lookup input and take a slot only for the request that starts the upstream work.
  12. 404 fallback concurrency. When the batch POST reaches a replica without the route, the client falls back to the single route four calls at a time. An old replica cannot tell the client its maxConcurrentLookups, and the single route has no batch limiter, so the fallback ignores a lower configured limit for the length of a rolling upgrade. Fix: advertise the configured limit with the batch version in the startup config, or serialize the fallback.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions