feat: enhance PRAuthor to manage branch existence

service/src/github/pr-author.test.ts

@@ -36,12 +36,6 @@ describe('PRAuthor.commitFiles', () => {
       patch: vi.fn(),
     };
 
-    // 1. baseSha tree resolution
-    api.get.mockImplementationOnce(async (url: string) => {
-      expect(url).toBe('/repos/kenil27/band/git/commits/sha-base');
-      return { data: { tree: { sha: 'tree-base' } } };
-    });
-
     // refExists: branch lookup returns 404
     api.get.mockImplementationOnce(async (url: string) => {
       expect(url).toBe('/repos/kenil27/band/git/ref/heads/openreview/tests/pr-1');
@@ -51,6 +45,12 @@ describe('PRAuthor.commitFiles', () => {
       throw err;
     });
 
+    // baseSha tree resolution
+    api.get.mockImplementationOnce(async (url: string) => {
+      expect(url).toBe('/repos/kenil27/band/git/commits/sha-base');
+      return { data: { tree: { sha: 'tree-base' } } };
+    });
+
     api.post
       .mockResolvedValueOnce({ data: { sha: 'blob-a' } }) // create blob
       .mockResolvedValueOnce({ data: { sha: 'tree-new' } }) // create tree
@@ -86,6 +86,16 @@ describe('PRAuthor.commitFiles', () => {
       ],
     });
 
+    // Check commit parents on the feature tip for a brand-new branch
+    const commitCall = api.post.mock.calls[2];
+    expect(commitCall[0]).toBe('/repos/kenil27/band/git/commits');
+    expect(commitCall[1]).toEqual(
+      expect.objectContaining({
+        message: 'test: add a',
+        parents: ['sha-base'],
+      }),
+    );
+
     // Check ref creation used the fully qualified ref string and POST (not PATCH)
     const refCall = api.post.mock.calls[3];
     expect(refCall[0]).toBe('/repos/kenil27/band/git/refs');
@@ -96,12 +106,16 @@ describe('PRAuthor.commitFiles', () => {
     expect(api.patch).not.toHaveBeenCalled();
   });
 
-  it('force-updates an existing branch with PATCH', async () => {
+  it('appends a commit when the feature branch has not advanced', async () => {
     const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() };
 
     api.get
-      .mockResolvedValueOnce({ data: { tree: { sha: 'tree-base' } } })
-      .mockResolvedValueOnce({ data: { ref: 'refs/heads/x' } }); // ref exists
+      .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // refExists
+      .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // getRefSha
+      .mockResolvedValueOnce({
+        data: { merge_base_commit: { sha: 'sha-base' } },
+      }) // isAncestor
+      .mockResolvedValueOnce({ data: { tree: { sha: 'tree-head' } } }); // headSha tree
 
     api.post
       .mockResolvedValueOnce({ data: { sha: 'blob-1' } })
@@ -118,9 +132,64 @@ describe('PRAuthor.commitFiles', () => {
       commitMessage: 'msg',
     });
 
+    const treeCall = api.post.mock.calls[1];
+    expect(treeCall[1]).toEqual({
+      base_tree: 'tree-head',
+      tree: [
+        {
+          path: 'tests/a.test.ts',
+          mode: '100644',
+          type: 'blob',
+          sha: 'blob-1',
+        },
+      ],
+    });
+
+    const commitCall = api.post.mock.calls[2];
+    expect(commitCall[1]).toEqual(
+      expect.objectContaining({
+        parents: ['tip-old'],
+      }),
+    );
+
     expect(api.patch).toHaveBeenCalledWith(
       '/repos/kenil27/band/git/refs/heads/openreview/tests/pr-42',
-      { sha: 'commit-1', force: true },
+      { sha: 'commit-1' },
+    );
+    expect(api.patch.mock.calls[0][1]).not.toHaveProperty('force');
+  });
+
+  it('creates a merge commit when the feature branch has advanced', async () => {
+    const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() };
+
+    api.get
+      .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // refExists
+      .mockResolvedValueOnce({ data: { object: { sha: 'tip-old' } } }) // getRefSha
+      .mockResolvedValueOnce({
+        data: { merge_base_commit: { sha: 'merge-old' } },
+      }) // isAncestor — feature tip is NOT merged
+      .mockResolvedValueOnce({ data: { tree: { sha: 'tree-head' } } }); // headSha tree
+
+    api.post
+      .mockResolvedValueOnce({ data: { sha: 'blob-1' } })
+      .mockResolvedValueOnce({ data: { sha: 'tree-1' } })
+      .mockResolvedValueOnce({ data: { sha: 'commit-1' } });
+
+    api.patch.mockResolvedValueOnce({ data: {} });
+
+    const author = new PRAuthor(makeClient(api));
+    await author.commitFiles({
+      branch: 'openreview/tests/pr-42',
+      baseSha: 'sha-head',
+      files: [{ path: 'tests/a.test.ts', content: 'noop' }],
+      commitMessage: 'msg',
+    });
+
+    const commitCall = api.post.mock.calls[2];
+    expect(commitCall[1]).toEqual(
+      expect.objectContaining({
+        parents: ['tip-old', 'sha-head'],
+      }),
     );
   });
 });
@@ -145,6 +214,7 @@ describe('PRAuthor.openOrUpdatePR', () => {
       url: 'https://github.com/kenil27/band/pull/99',
       number: 99,
       created: true,
+      updated: false,
     });
 
     expect(api.get).toHaveBeenCalledWith(
@@ -169,25 +239,54 @@ describe('PRAuthor.openOrUpdatePR', () => {
     );
   });
 
-  it('returns the existing PR without creating a new one', async () => {
-    const api = { get: vi.fn(), post: vi.fn() };
+  it('PATCHes title and body on an existing open PR', async () => {
+    const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() };
     api.get.mockResolvedValueOnce({
       data: [{ number: 7, html_url: 'https://github.com/kenil27/band/pull/7' }],
     });
+    api.patch.mockResolvedValueOnce({ data: {} });
 
     const author = new PRAuthor(makeClient(api));
     const res = await author.openOrUpdatePR({
       base: 'feature/foo',
       head: 'openreview/tests/pr-1',
-      title: 't',
-      body: 'b',
+      title: 'updated title',
+      body: 'updated body',
     });
 
     expect(res).toEqual({
       url: 'https://github.com/kenil27/band/pull/7',
       number: 7,
       created: false,
+      updated: true,
+    });
+    expect(api.patch).toHaveBeenCalledWith('/repos/kenil27/band/pulls/7', {
+      title: 'updated title',
+      body: 'updated body',
     });
     expect(api.post).not.toHaveBeenCalled();
   });
 });
+
+describe('PRAuthor.branchExists', () => {
+  it('returns true when the branch ref is present', async () => {
+    const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() };
+    api.get.mockResolvedValueOnce({ data: { ref: 'refs/heads/openreview/tests/pr-1' } });
+
+    const author = new PRAuthor(makeClient(api));
+    await expect(author.branchExists('openreview/tests/pr-1')).resolves.toBe(true);
+    expect(api.get).toHaveBeenCalledWith(
+      '/repos/kenil27/band/git/ref/heads/openreview/tests/pr-1',
+    );
+  });
+
+  it('returns false when the branch ref is missing', async () => {
+    const api = { get: vi.fn(), post: vi.fn(), patch: vi.fn() };
+    api.get.mockRejectedValueOnce(
+      Object.assign(new Error('not found'), { response: { status: 404 } }),
+    );
+
+    const author = new PRAuthor(makeClient(api));
+    await expect(author.branchExists('openreview/tests/pr-99')).resolves.toBe(false);
+  });
+});

service/src/github/pr-author.ts

@@ -30,11 +30,19 @@ export class PRAuthor {
   }
 
   /**
-   * Commit one or more files atomically on top of `baseSha`, creating or
-   * fast-forwarding `branch` to point at the new commit.
+   * Commit generated test files on `branch`.
    *
-   * Empty `files` is a no-op that returns `null` so the caller can decide
-   * whether to skip PR creation.
+   * Stacked test PRs target the feature branch (`headRef`) and should only
+   * *show* test-file deltas in the PR diff. We always build the commit tree
+   * from the current feature tip (`baseSha`) plus test overlays — never
+   * re-import an older snapshot of source files.
+   *
+   * History strategy:
+   * - **First push:** single parent = feature tip.
+   * - **Refresh, feature unchanged:** single parent = test-branch tip.
+   * - **Refresh, feature advanced:** merge commit with parents
+   *   `[test-branch tip, feature tip]` so the branch stays aligned with the
+   *   feature branch and GitHub's PR diff does not resurrect `.js` changes.
    */
   async commitFiles(opts: {
     branch: string;
@@ -44,13 +52,24 @@ export class PRAuthor {
   }): Promise<{ branchRef: string; commitSha: string } | null> {
     if (opts.files.length === 0) return null;
 
-    // 1) Resolve the tree SHA at baseSha so we can extend it.
-    const { data: baseCommit } = await this.api.get(
-      `${this.repoPath}/git/commits/${opts.baseSha}`,
-    );
-    const baseTreeSha = baseCommit.tree.sha as string;
+    const refPath = `heads/${opts.branch}`;
+    const fullyQualified = `refs/heads/${opts.branch}`;
+    const headSha = opts.baseSha;
+    const branchExists = await this.refExists(refPath);
+
+    let parents: string[];
+    if (!branchExists) {
+      parents = [headSha];
+    } else {
+      const branchTip = await this.getRefSha(refPath);
+      const headMerged =
+        branchTip === headSha ||
+        (await this.isAncestor(headSha, branchTip));
+      parents = headMerged ? [branchTip] : [branchTip, headSha];
+    }
+
+    const baseTreeSha = await this.getCommitTreeSha(headSha);
 
-    // 2) Upload each file as a blob.
     const blobs = await Promise.all(
       opts.files.map(async (f) => {
         const { data } = await this.api.post(`${this.repoPath}/git/blobs`, {
@@ -61,7 +80,6 @@ export class PRAuthor {
       }),
     );
 
-    // 3) Build a new tree referencing the new blobs.
     const { data: newTree } = await this.api.post(
       `${this.repoPath}/git/trees`,
       {
@@ -75,25 +93,19 @@ export class PRAuthor {
       },
     );
 
-    // 4) Create the commit pointing at the new tree, parented on baseSha.
     const { data: newCommit } = await this.api.post(
       `${this.repoPath}/git/commits`,
       {
         message: opts.commitMessage,
         tree: newTree.sha,
-        parents: [opts.baseSha],
+        parents,
       },
     );
     const commitSha = newCommit.sha as string;
 
-    // 5) Create the branch ref, or fast-forward an existing one.
-    const refPath = `heads/${opts.branch}`;
-    const fullyQualified = `refs/heads/${opts.branch}`;
-    const exists = await this.refExists(refPath);
-    if (exists) {
+    if (branchExists) {
       await this.api.patch(`${this.repoPath}/git/refs/${refPath}`, {
         sha: commitSha,
-        force: true,
       });
     } else {
       await this.api.post(`${this.repoPath}/git/refs`, {
@@ -105,20 +117,35 @@ export class PRAuthor {
     return { branchRef: fullyQualified, commitSha };
   }
 
+  /** Whether `refs/heads/<branch>` already exists (used to pick a commit message). */
+  async branchExists(branch: string): Promise<boolean> {
+    return this.refExists(`heads/${branch}`);
+  }
+
   /**
-   * Open a PR with `head` -> `base`, or return the open one if it exists.
-   * The {@link created} flag tells the caller whether to post a fresh
-   * "tests generated" comment or to skip it.
+   * Open a PR with `head` -> `base`, or refresh title/body on the open one.
+   *
+   * When a matching open PR exists, its title and body are PATCHed so
+   * re-runs after `pull_request.synchronize` show up-to-date coverage stats.
    */
   async openOrUpdatePR(opts: {
     base: string;
     head: string;
     title: string;
     body: string;
-  }): Promise<{ url: string; number: number; created: boolean }> {
+  }): Promise<{ url: string; number: number; created: boolean; updated: boolean }> {
     const existing = await this.findOpenPR(opts.head, opts.base);
     if (existing) {
-      return { url: existing.url, number: existing.number, created: false };
+      await this.api.patch(`${this.repoPath}/pulls/${existing.number}`, {
+        title: opts.title,
+        body: opts.body,
+      });
+      return {
+        url: existing.url,
+        number: existing.number,
+        created: false,
+        updated: true,
+      };
     }
 
     const { data } = await this.api.post(`${this.repoPath}/pulls`, {
@@ -132,11 +159,36 @@ export class PRAuthor {
       url: data.html_url as string,
       number: data.number as number,
       created: true,
+      updated: false,
     };
   }
 
   /* ---- Internal helpers --------------------------------------------- */
 
+  private async getRefSha(refPath: string): Promise<string> {
+    const { data } = await this.api.get(`${this.repoPath}/git/ref/${refPath}`);
+    return data.object.sha as string;
+  }
+
+  private async getCommitTreeSha(commitSha: string): Promise<string> {
+    const { data } = await this.api.get(
+      `${this.repoPath}/git/commits/${commitSha}`,
+    );
+    return data.tree.sha as string;
+  }
+
+  /** True when `ancestorSha` is reachable from `descendantSha` (inclusive). */
+  private async isAncestor(
+    ancestorSha: string,
+    descendantSha: string,
+  ): Promise<boolean> {
+    if (ancestorSha === descendantSha) return true;
+    const { data } = await this.api.get(
+      `${this.repoPath}/compare/${ancestorSha}...${descendantSha}`,
+    );
+    return (data.merge_base_commit?.sha as string | undefined) === ancestorSha;
+  }
+
   private async refExists(refPath: string): Promise<boolean> {
     try {
       await this.api.get(`${this.repoPath}/git/ref/${refPath}`);