Skip to content

[feature] Coding agents cannot show a pull request — add a Pulls panel, and make "don't fetch if nothing changed" true with GitHub conditional requests #401

Closed

Description

@serge-ivo

The Coder can open a PR and then cannot show you one

Four GitHub tools exist (connectors/github.ts): github_workflow_runs, github_list_issues,
github_read_issue, github_create_issue. There is no pull-request support anywhere — grep
for /pulls across workers/api/src returns nothing.

Worse than absent: the one place GitHub hands them over, we discard them. lib/github-issues.ts:107

return data.filter((i) => !i.pull_request).map(toSummary);

with the comment "GitHub returns PRs from the issues endpoint too". Correct for an issues list,
and it means the data arrives and is dropped.

This matters because the Coder's safest mode produces exactly this artefact. The per-repo merge
policy (#314) has a pr setting: the agent does the work, opens a pull request and stops. That is
the mode a careful owner picks — and having picked it, the console can show them the issue that
started the work and the build that ran, but not the PR that is the actual output. They have to
leave for GitHub to see what their agent just produced.

The Coding tab already has the shape: Terminal · Issues · Builds. This is the missing fourth.

What to show

Per repo, the open PRs, and for each: number, title, author, draft flag, mergeable/conflicted,
review state (approved / changes requested / none), check status (which is already fetched for
Builds — fetchWorkflowRuns — so the two panels are reading the same runs from different angles),
branch, updated-at. Plus, and this is the one that closes the loop: which PR came from which
agent run
, when a Pilot opened it.

Caching — and specifically "don't fetch if nothing changed"

There are TWO layers here and the repo has exactly one of them. Naming both, because the cheap one
is not the one that protects the rate limit.

Layer 1 — console → platform. Built, for builds only. routes/coding-repos.ts:293-300 merges a
durable KV history, computes a weak ETag over the result, and answers If-None-Match with a 304.
That is the pattern to copy verbatim; it saves bytes and re-renders.

Layer 2 — platform → GitHub. Not built, anywhere. Even the builds path fetches GitHub on every
poll and only then decides whether to send the client a 304. GitHub's own conditional requests are
the mechanism the request is asking for:

  • store the ETag GitHub returned, send it back as If-None-Match;
  • a 304 does not count against the rate limit — that is the entire point of it;
  • on 304, serve the cached copy and touch nothing else.

That is what makes "don't fetch if there were no changes" true rather than approximately true. It
matters here more than usual: /deployments documents an unauthenticated fallback on a shared
~60/hr IP budget, and issues-mode calls listIssues on every Loop iteration via nextIssue()
— today that is an unconditional GitHub round-trip per iteration, per repo.

Complementary, not a substitute: ?since= on issues and sort=updated&direction=desc on pulls
bound the payload; the ETag bounds the request.

Suggested shape

  1. lib/github-prs.ts beside github-issues.ts — same auth path (App installation token, with
    the same unauth fallback for public repos), same summary/detail split.
  2. A conditional-fetch helper, shared by issues, PRs and builds: store {etag, payload, fetchedAt} in KV per (repo, resource), send If-None-Match, return the cached payload on 304.
    One place, three consumers — and it retro-fixes the builds path's Layer-2 gap.
  3. GET /:instanceId/coding/repos/:repoId/pulls mirroring /deployments, including the weak
    ETag and 304 for Layer 1.
  4. A Pulls panel in the Coding tab beside Issues and Builds.
  5. github_list_pulls / github_read_pull tools (read scope) so the agent can answer "is my PR
    green? did anyone review it?" without a browser. github_merge_pull is deliberately NOT proposed
    here — merging is what the merge policy ([safety] Nothing decides whether an agent may merge to main — three unattended merges already happened #314) governs, and a tool that bypasses it would hand
    the agent the authority that ticket exists to withhold.

Worth deciding

  • Does the PR panel show only PRs the agent opened, or all of them? All of them is more useful
    and makes the panel a real view of the repo; agent-opened ones should be badged, since "did my
    agent actually open it" is the question the board cannot answer today.
  • How stale may a cached list be when GitHub is unreachable? The builds path already answers
    this well (durable KV history that survives retention and transient failure) and the same answer
    probably applies.

Files: workers/api/src/lib/github-issues.ts:103-122, workers/api/src/lib/connectors/github.ts:134-165,
workers/api/src/routes/coding-repos.ts:264-305, workers/api/src/lib/build-history.ts.

Activity

  1. serge-ivo commented on Aug 8, 2026

    @serge-ivo
    ContributorAuthor

    Self-review: one correction and one thing I under-specified

    1. The cache key is a tenancy boundary, and I wrote it as (repo, resource). That is wrong as stated. A private repo's issues and PRs are readable only through a GitHub App installation token that belongs to a specific owner — keying a shared KV cache by repo alone would let one user's authorised fetch populate an entry that another user, who has no installation on that repo, then reads. The same repo string can be reachable by one tenant and not another, and the whole point of the cache is that the second read does not go to GitHub to find out.

    Key it by (user, repo, resource) at minimum. If a shared entry is ever wanted for public repos, that has to be an explicit decision with the auth state in the key (anon vs installation:<id>), not a side effect of the key being short. Fail-closed: an entry whose auth context does not match the caller is a miss, not a hit.

    2. "A 304 does not count against the rate limit" needs a caveat I left out. It is true for GitHub's REST API primary rate limit, which is the budget the unauthenticated /deployments fallback is spending. It does not exempt you from secondary/abuse limits, and it does not make an unbounded poll free — a 304 is still a request. So the conditional header is the right mechanism and it is not a licence to poll harder. Whatever interval the panels use today should stay, with the ETag reducing cost rather than enabling frequency.

    3. Under-specified: what invalidates the cache when the platform itself changes the data. github_create_issue is an existing tool, and #402 may add PR reads. An agent that opens an issue and then lists issues must not read its own pre-write cache entry — that is the get_tasks-after-create_task failure agent-think.ts already documents for the dedup guard, in a new place. The write path should drop the (user, repo, issues) entry, which is one line at each mutating call site and much cheaper than a TTL short enough to hide the problem.

  2. serge-ivo commented on Aug 8, 2026

    @serge-ivo
    ContributorAuthor

    Shipped in 6f6e42f. Deployed, CI green.

    Your self-review's three corrections are all implemented as written.

    1. The cache key is a tenancy boundary. Key is ghcond:1:{userId}:{owner/repo}:{resource} — the user is in the key. The entry also records {user, auth, repo, resource} and all four are re-checked against the caller on every read; any mismatch returns null. auth is anon or installation:&lt;id&gt;, and an authenticated read whose installation cannot be named from the user's verified github_installations rows yields no auth context, which disables caching entirely rather than caching under an unknown authority. No KV binding → cache off.

    One deliberate design note: auth context lives in the entry, not the key. The user is already the isolation boundary, so per-auth keys buy no isolation — but they would leave write-invalidation unable to name the entry it must drop, which is your point 3. Same fail-closed answer, one deletable key. Cross-user sharing for public repos stays an explicit future decision, not a side effect.

    The cross-tenant test is the deliverable: Alice's authorised private fetch is seeded, then Bob — deliberately given the same auth-context string — gets null, and his request is asserted to actually reach fetch and receive the 404.

    2. Poll intervals unchanged, exactly as you asked: PullsPanel reuses the Builds panel's {activeMs: 20000, passiveMs: 120000}. Failures are asymmetric — 401/403/404/410 means access changed, so the entry is dropped and the error reported (serving it would be a cross-tenant read separated in time); 5xx/429/network means unreachable, so stale is served.

    3. Invalidation on write: github_create_issue drops (user, repo, "issues") after a successful POST, and not on failure. Both tested.

    Also: read-only by construction — a test asserts no PR write tool exists (#314). PR→run attribution is exact-or-absent, never guessed. Verified in WebKit at 320px and 390px with a fully-badged row (82-char title, 44-char branch), plus a squeeze guard for the #384 failure mode.

    Left undone — one of these means the tools are not yet reachable

    • No seeded agent declares github_list_pulls/github_read_pull. capabilities.tools is an authoritative allowlist, so coder-repo, coder-lead and repo-chat cannot call them until a migration adds them (the lists live in migrations 0054/0063/0067/0070). The legacy coder slug declares no allowlist and gets them today. The panel works; the agent-facing half does not, for those three. No migration was created because numbers were centrally assigned in this batch — this wants its own ticket, and I'd like it filed rather than done silently.
    • Attribution misses gh pr create --fill — the runner's prRef() only extracts a number from an explicit gh pr merge 42 or a PR URL; for --fill the number exists only in the command's output, which classifySegment never sees. Those PRs render with no badge rather than a guessed one. A proper fix means parsing CLI output in packages/browser-runner and republishing the CLI.
    • fetchWorkflowRuns is not retro-fitted with the Layer-2 cache — the mechanism is ready, but threading (userId, authContext) through its call sites touches routes/coding-repos.ts, which another lane owned this batch.

    Leaving open for the first item.

  3. serge-ivo commented on Aug 8, 2026

    @serge-ivo
    ContributorAuthor

    Correction to my previous comment. I wrote that "the legacy coder slug declares no allowlist and gets them today." That is false, and I repeated it from the delivery report without checking.

    Migration 0054_coder_github_tools.sql sets coder's capabilities including:

    "tools":["github_create_issue","github_list_issues","github_read_issue"]

    So coder is on the authoritative-allowlist path too, and no migration anywhere names github_list_pulls or github_read_pull (grep over workers/api/migrations/ → no match). Confirmed live: /v1/instances/{coder-repo}/tools returns allowed:false, reason:"not_declared" for github_list_pulls.

    So the correct statement is: no agent anywhere can call these two tools today — not "all but three". The console Pulls panel works; the agent-facing half is inert everywhere. That raises the value of the follow-up rather than lowering it.

    Tracked as #415, which also records two things worth knowing for whoever picks it up: capabilitiesForInstance JOINs the agents row at read time, so one migration reaches every already-subscribed instance with no instance migration; and coder2-parity.test.ts parses migration 0070 by regex, so after the fix it will keep passing while describing a superseded state — an expired premise planted by the fix itself.

  4. serge-ivo commented on Aug 8, 2026

    @serge-ivo
    ContributorAuthor

    The one blocker is gone, and the tools are live. Closing.

    This was left open for exactly one reason, quoted from the close-out: "No seeded agent declares
    github_list_pulls/github_read_pull … The panel works; the agent-facing half does not."
    That was
    tracked as #415, shipped as migration 0101 (1a3955e), and I have now confirmed the agent-facing
    half in production, not just in the migration:

    GET /v1/instances/{coder-repo}/tools
      github_list_pulls  → allowed: true, reason: "ok"
      github_read_pull   → allowed: true, reason: "ok"
    

    and, asked "what pull requests are open on proappstore-online/chess-academy?", that instance's
    Assistant produced tool.call ✅ **github_list_pulls** [] in agent_trace
    (trace 7acacd5d-d121-4edd-a591-fcf5904a097f) — the tool, not a decline and not the terminal. Full
    evidence in #415.

    The rest of the ticket, re-verified against 6f6e42f

    Suggested shape Verdict Evidence
    1. lib/github-prs.ts beside github-issues.ts, same auth path, summary/detail split ✅ workers/api/src/lib/github-prs.ts (355 lines), listPulls/readPull, resolveGithubRead; github-prs.test.ts
    2. Shared conditional-fetch helper, three consumers ✅ lib/github-cache.ts:110 + githubConditionalJson; issues github-issues.ts:129,155, PRs github-prs.ts, runs github-actions.ts:83 (the third landed with #418)
    3. Route with weak ETag + 304 ✅ routes/coding-pulls.ts:65,86; W/"pulls-…" at :43-50, 304 at :81; tests coding-pulls.test.ts:123 and :134 ("does NOT 304 once a review lands")
    4. Pulls panel in the Coding tab ✅ agents/coder/web/src/PullsPanel.tsx, wired CodingTab.tsx:903,985; e2e console.spec.ts:3056
    5. Read-scope tools, no merge tool ✅ connectors/github.ts:191-194,201-204 both scope:"read", handlers :229-230; github.test.ts:100 asserts no write-scoped *pull* tool exists

    The self-review's three corrections all hold.
    (a) Key is ghcond:{v}:{userId}:{owner/repo}:{resource} (github-cache.ts:110-112) and all four
    recorded fields are re-checked per read (:155-158), each returning null. Unnameable auth disables
    caching rather than caching under unknown authority (:121-131,147). The cross-tenant test
    (github-cache.test.ts:84) gives Bob the same auth-context string deliberately and asserts not
    merely a miss but that his request reached fetch and got his own 404
    (expect(fetchSpy).toHaveBeenCalledTimes(1), expect(bob).toEqual({ok:false,status:404})).
    (b) Intervals unchanged — PullsPanel.tsx:150 reuses {activeMs:20000, passiveMs:120000}
    byte-identical to BuildsPanel.tsx:115; the whole diff adds exactly one interval line and changes none.
    (c) Invalidation on write is after the !res.ok early return (connectors/github.ts:143,150), with
    both tests present (github.test.ts:346 success, :352 a 422 asserting not.toHaveBeenCalled()).

    The two secondary items are closed too. gh pr create --fill attribution shipped in b7bbb7a
    (#417) — engine-acts.ts:206-208 wired at headless.ts:679 on the raw block.content, with the
    negative cases tested; CLI 0.4.43. The fetchWorkflowRuns Layer-2 retro-fit shipped in 0126819
    (#418).

    Left open elsewhere, deliberately or otherwise — recorded so it is not lost

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

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions