diff --git a/plugins/compound-engineering/skills/ce-code-review/SKILL.md b/plugins/compound-engineering/skills/ce-code-review/SKILL.md index bc7780420..a108d8a7b 100644 --- a/plugins/compound-engineering/skills/ce-code-review/SKILL.md +++ b/plugins/compound-engineering/skills/ce-code-review/SKILL.md @@ -246,12 +246,24 @@ Then fetch PR metadata. Capture the base branch name and the PR base repository gh pr view --json title,body,baseRefName,headRefName,url,reviews,comments --jq '{title, body, baseRefName, headRefName, url, hasPriorComments: ((.reviews | map(select(.state != "APPROVED" or .body != "")) | length) > 0 or (.comments | length) > 0)}' ``` -Use the repository portion of the returned PR URL as `` (for example, `EveryInc/compound-engineering-plugin` from `https://github.com/EveryInc/compound-engineering-plugin/pull/348`). +Derive both the host and the repository portion from the returned PR URL. Both are needed because remote URLs use the same `host` + `owner/repo` pair as the PR URL but in different shapes (HTTPS, SSH, scp-form), and any host other than `github.com` (GitHub Enterprise, self-hosted) must still match correctly. For example, from `https://github.com/EveryInc/compound-engineering-plugin/pull/348` use `github.com` as `` and `EveryInc/compound-engineering-plugin` as ``. From `https://ghe.acme.com/Org/Repo/pull/12` use `ghe.acme.com` as `` and `Org/Repo` as ``. From `https://ghe.acme.com:8443/Org/Repo/pull/12` preserve the port and use `ghe.acme.com:8443` as ``. Lowercase both before substituting; host and owner/repo are case-insensitive on GitHub and remote URLs may preserve user-typed casing. For scp-form SSH remotes (`git@host:owner/repo.git`), match the host without the PR URL port as a fallback because scp-form syntax does not carry the web UI port. -Then compute a local diff against the PR's base branch so re-reviews also include local fix commits and uncommitted edits. Substitute the PR base branch from metadata (shown here as ``) and the PR base repository identity derived from the PR URL (shown here as ``). Resolve the base ref from the PR's actual base repository, not by assuming `origin` points at that repo: +Then compute a local diff against the PR's base branch so re-reviews also include local fix commits and uncommitted edits. Substitute the PR base branch from metadata (shown here as ``), the PR base host (shown here as ``), and the PR base repository (shown here as ``). Resolve the base ref from the PR's actual base repository, not by assuming `origin` points at that repo: ``` -PR_BASE_REMOTE=$(git remote -v | awk 'index($2, "github.com:") || index($2, "github.com/") {print $1; exit}') +PR_BASE_REMOTE=$(git remote -v | awk -v host="" -v repo="" ' + BEGIN { host_without_port = host; sub(/:[0-9]+$/, "", host_without_port) } + { + url = $2; h = ""; p = ""; scp = 0 + if (match(url, /:\/\/[^\/]+\//)) { h = substr(url, RSTART+3, RLENGTH-4); p = substr(url, RSTART+RLENGTH) } + else if (match(url, /@[^:]+:/)) { h = substr(url, RSTART+1, RLENGTH-2); p = substr(url, RSTART+RLENGTH); scp = 1 } + else next + sub(/^[^@]*@/, "", h) + h = tolower(h) + if (!match(p, /^[^\/]+\/[^\/]+/)) next + pr = substr(p, 1, RLENGTH); sub(/\.git$/, "", pr); pr = tolower(pr) + if ((h == host || (scp && h == host_without_port)) && pr == repo) { print $1; exit } + }') if [ -n "$PR_BASE_REMOTE" ]; then PR_BASE_REMOTE_REF="$PR_BASE_REMOTE/"; else PR_BASE_REMOTE_REF=""; fi PR_BASE_REF=$(git rev-parse --verify "$PR_BASE_REMOTE_REF" 2>/dev/null || git rev-parse --verify 2>/dev/null || true) if [ -z "$PR_BASE_REF" ]; then @@ -259,7 +271,7 @@ if [ -z "$PR_BASE_REF" ]; then git fetch --no-tags "$PR_BASE_REMOTE" :refs/remotes/"$PR_BASE_REMOTE"/ 2>/dev/null || git fetch --no-tags "$PR_BASE_REMOTE" 2>/dev/null || true PR_BASE_REF=$(git rev-parse --verify "$PR_BASE_REMOTE_REF" 2>/dev/null || git rev-parse --verify 2>/dev/null || true) else - if git fetch --no-tags https://github.com/.git 2>/dev/null; then + if git fetch --no-tags https:///.git 2>/dev/null; then PR_BASE_REF=$(git rev-parse --verify FETCH_HEAD 2>/dev/null || true) fi if [ -z "$PR_BASE_REF" ]; then PR_BASE_REF=$(git rev-parse --verify 2>/dev/null || true); fi diff --git a/tests/skills/ce-code-review-base-remote-matcher.test.ts b/tests/skills/ce-code-review-base-remote-matcher.test.ts new file mode 100644 index 000000000..381261d72 --- /dev/null +++ b/tests/skills/ce-code-review-base-remote-matcher.test.ts @@ -0,0 +1,85 @@ +import { readFileSync } from "fs" +import path from "path" +import { describe, expect, test } from "bun:test" + +const SKILL_PATH = path.join( + process.cwd(), + "plugins/compound-engineering/skills/ce-code-review/SKILL.md", +) +const SKILL_BODY = readFileSync(SKILL_PATH, "utf8") + +const awkMatch = SKILL_BODY.match( + /awk -v host="" -v repo="" '\n(?[\s\S]*?\n })'\)/, +) +const AWK_PROGRAM = awkMatch?.groups?.program ?? "" + +async function matchRemote( + remotes: string, + host: string, + repo: string, +): Promise { + const proc = Bun.spawn( + ["awk", "-v", `host=${host}`, "-v", `repo=${repo}`, AWK_PROGRAM], + { + stdin: new TextEncoder().encode(remotes), + stdout: "pipe", + stderr: "pipe", + }, + ) + const stdout = await new Response(proc.stdout).text() + const stderr = await new Response(proc.stderr).text() + const exitCode = await proc.exited + + expect(stderr).toBe("") + expect(exitCode).toBe(0) + return stdout.trim() +} + +describe("ce-code-review PR base remote matcher", () => { + test("extracts the embedded AWK matcher from SKILL.md", () => { + expect(AWK_PROGRAM).not.toBe("") + }) + + test("matches HTTPS remotes on a ported GitHub Enterprise host", async () => { + await expect( + matchRemote( + "base https://ghe.acme.com:8443/Org/Repo.git (fetch)\n", + "ghe.acme.com:8443", + "org/repo", + ), + ).resolves.toBe("base") + }) + + test("matches ssh URL remotes on a ported GitHub Enterprise host", async () => { + await expect( + matchRemote( + "base ssh://git@ghe.acme.com:8443/Org/Repo.git (fetch)\n", + "ghe.acme.com:8443", + "org/repo", + ), + ).resolves.toBe("base") + }) + + test("allows scp-form SSH remotes to omit the PR URL port", async () => { + await expect( + matchRemote( + [ + "wrongport https://ghe.acme.com:9443/Org/Repo.git (fetch)", + "base git@ghe.acme.com:Org/Repo.git (fetch)", + ].join("\n") + "\n", + "ghe.acme.com:8443", + "org/repo", + ), + ).resolves.toBe("base") + }) + + test("does not match a different port for URL-form remotes", async () => { + await expect( + matchRemote( + "wrongport https://ghe.acme.com:9443/Org/Repo.git (fetch)\n", + "ghe.acme.com:8443", + "org/repo", + ), + ).resolves.toBe("") + }) +})