Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion plugins/github/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ it reports needs-configuration. No tokens are stored by the plugin.

- Every BB project source whose checkout has a GitHub `origin` remote
(repo → project mapping is also how spawn picks the project).
- Plus the `extraRepos` setting: comma-separated `owner/repo` list.
- Plus the `extraRepos` setting: comma-separated `owner/repo` list. Entries that
are not `owner/repo` — a `owner/*` wildcard, a bare owner, a typo — are not
tracked; `bb github repos` names them on stderr and the plugin log warns once
per distinct set. Wildcards are not supported.
- `defaultProject` setting: where threads spawn for repos with no project.

```
Expand Down
49 changes: 47 additions & 2 deletions plugins/github/server.rpc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,16 +195,61 @@ afterEach(() => {
rmSync(binDir, { recursive: true, force: true });
});

async function loadPlugin() {
async function loadPlugin(extraRepos = "acme/widgets") {
const host = createFakePluginHost({
pluginId: "github",
settings: { extraRepos: "acme/widgets" },
settings: { extraRepos },
});
await plugin(host.bb);
return host;
}

describe("github plugin RPC behavior", () => {
// A stored-but-unusable extraRepos entry used to look exactly like one that
// matched nothing: no log line, and `bb github repos` listed the usable names
// without mentioning what it had dropped.
it("reports extraRepos entries it cannot honor instead of dropping them", async () => {
const { harness } = await loadPlugin("acme/widgets, ACME-ORG/*, nonsense");

await expect(harness.runCli(["repos"])).resolves.toEqual({
exitCode: 0,
stdout: "acme/widgets",
stderr:
'ignoring 2 extraRepos entries that are not "owner/repo": ACME-ORG/*, nonsense\n',
});
expect(harness.logEntries).toEqual(
expect.arrayContaining([
expect.objectContaining({
level: "warn",
message: 'ignoring 2 extraRepos entries that are not "owner/repo": ACME-ORG/*, nonsense',
}),
]),
);

// The warning is emitted per distinct set, not per discovery: `repos`
// forces a fresh read every time and a background sync lapses the cache on
// its own, so warning per pass would repeat this line forever.
await harness.runCli(["repos"]);
expect(
harness.logEntries.filter(
(entry) => entry.level === "warn" && entry.message.includes("extraRepos"),
),
).toHaveLength(1);
});

it("says nothing about extraRepos when every entry is usable", async () => {
const { harness } = await loadPlugin("acme/widgets");

await expect(harness.runCli(["repos"])).resolves.toEqual({
exitCode: 0,
stdout: "acme/widgets",
stderr: "",
});
expect(
harness.logEntries.filter((entry) => entry.message.includes("extraRepos")),
).toEqual([]);
});

it("syncs, filters, mutates, and exposes the same cached issue across surfaces", async () => {
const { harness } = await loadPlugin();

Expand Down
25 changes: 25 additions & 0 deletions plugins/github/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { createFakePluginHost } from "@get-bb/plugin-sdk/testing";
import {
fetchRepoItems,
githubRpcContract,
parseExtraRepos,
parsePaginatedGhApi,
validateGithubCliArgs,
} from "./server";
Expand Down Expand Up @@ -128,6 +129,30 @@ describe("GitHub RPC contract", () => {
);
});

it("separates usable extraRepos entries from ones it cannot honor", () => {
expect(parseExtraRepos("get-bb/bb, nonsense")).toEqual({
repos: ["get-bb/bb"],
ignored: ["nonsense"],
});
// The wildcard from the report: stored, never matched, previously silent.
expect(parseExtraRepos("SOME-ORG/*")).toEqual({
repos: [],
ignored: ["SOME-ORG/*"],
});
// Separators and blanks are structure, not entries — an empty setting has
// nothing to complain about.
expect(parseExtraRepos("")).toEqual({ repos: [], ignored: [] });
expect(parseExtraRepos(" ,, \n ")).toEqual({ repos: [], ignored: [] });
expect(parseExtraRepos(" acme/one\nacme/two , acme/one ")).toEqual({
repos: ["acme/one", "acme/two"],
ignored: [],
});
expect(parseExtraRepos("bad/repo/shape acme").ignored).toEqual([
"bad/repo/shape",
"acme",
]);
});

it("rejects CLI arguments that would otherwise broaden a repository query", () => {
expect(validateGithubCliArgs(["issues", "get-bb/bb"])).toBeNull();
expect(validateGithubCliArgs(["issues", "bad/repo/shape"])).toContain(
Expand Down
62 changes: 59 additions & 3 deletions plugins/github/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,36 @@ function isRepoName(value: unknown): value is string {
return typeof value === "string" && /^[\w.-]+\/[\w.-]+$/.test(value);
}

/**
* Split the `extraRepos` setting into the names we track and the entries we
* cannot. Anything that is not `owner/repo` — a `SOME-ORG/*` wildcard, a typo,
* a bare owner — is reported rather than dropped, so a setting that was stored
* but not honored is distinguishable from one that matched nothing.
*/
export function parseExtraRepos(raw: string): {
repos: string[];
ignored: string[];
} {
const repos: string[] = [];
const ignored: string[] = [];
for (const entry of raw.split(/[\s,]+/)) {
if (entry === "") continue;
if (isRepoName(entry)) {
if (!repos.includes(entry)) repos.push(entry);
} else if (!ignored.includes(entry)) {
ignored.push(entry);
}
}
return { repos, ignored };
}

function describeIgnoredExtraRepos(ignored: string[]): string {
return (
`ignoring ${ignored.length} extraRepos ${ignored.length === 1 ? "entry" : "entries"} ` +
`that ${ignored.length === 1 ? "is" : "are"} not "owner/repo": ${ignored.join(", ")}`
);
}

function run(
file: string,
args: string[],
Expand Down Expand Up @@ -578,6 +608,11 @@ export default async function plugin(bb: BbPluginApi) {
// ------------------------------------------------------------------
let repoCache: { repos: RepoInfo[]; fetchedAt: number } | null = null;

// The extraRepos entries the last discovery could not use, kept so `bb github
// repos` can say so on the surface the reporter looked at.
let ignoredExtraRepos: string[] = [];
let lastIgnoredExtraReposKey: string | null = null;

async function discoverRepos(force = false): Promise<RepoInfo[]> {
if (!force && repoCache !== null && Date.now() - repoCache.fetchedAt < 60_000) {
return repoCache.repos;
Expand Down Expand Up @@ -609,11 +644,23 @@ export default async function plugin(bb: BbPluginApi) {
);
}
const { extraRepos } = await settings.get();
for (const raw of extraRepos.split(/[\s,]+/)) {
if (isRepoName(raw) && !byRepo.has(raw)) {
const parsed = parseExtraRepos(extraRepos);
for (const raw of parsed.repos) {
if (!byRepo.has(raw)) {
byRepo.set(raw, { repo: raw, projectId: null });
}
}
// Warn on the set rather than on every discovery: discoverRepos re-reads
// settings whenever the 60s cache lapses, and a background sync would
// otherwise repeat the same line forever.
const ignoredKey = parsed.ignored.join(",");
if (ignoredKey !== lastIgnoredExtraReposKey) {
lastIgnoredExtraReposKey = ignoredKey;
if (parsed.ignored.length > 0) {
bb.log.warn(describeIgnoredExtraRepos(parsed.ignored));
}
}
ignoredExtraRepos = parsed.ignored;
const repos = [...byRepo.values()];
repoCache = { repos, fetchedAt: Date.now() };
return repos;
Expand Down Expand Up @@ -1567,14 +1614,23 @@ export default async function plugin(bb: BbPluginApi) {
}
if (sub === "repos") {
const repos = await discoverRepos(true);
const warning =
ignoredExtraRepos.length > 0
? { stderr: `${describeIgnoredExtraRepos(ignoredExtraRepos)}\n` }
: {};
if (repos.length === 0) {
return { exitCode: 0, stdout: "No tracked repos. Attach a project with a GitHub remote or set extraRepos." };
return {
exitCode: 0,
stdout: "No tracked repos. Attach a project with a GitHub remote or set extraRepos.",
...warning,
};
}
return {
exitCode: 0,
stdout: repos
.map((entry) => `${entry.repo}${entry.projectId !== null ? `\t(${entry.projectId})` : ""}`)
.join("\n"),
...warning,
};
}
if (sub === "issues" || sub === "prs") {
Expand Down
Loading