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: 5 additions & 0 deletions .changeset/bot-prs-sweep-progress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@giantswarm/backstage-plugin-bot-prs': minor
---

Bot PRs: **Classify now**, **Approve and merge** and **Sweep** show where each team's run stands while marge works (`34 of 79 PRs done`), and a run lasts as long as marge keeps reporting instead of ending at the muster server's timeout. A run whose answer is cut on the way back is reported as an answer that never arrived, with the labels read again.
7 changes: 7 additions & 0 deletions .changeset/muster-call-with-progress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@giantswarm/backstage-plugin-muster': minor
'@giantswarm/backstage-plugin-muster-backend': minor
'@giantswarm/backstage-plugin-gs-node': minor
---

Tool calls that report progress: `MusterApi.callToolWithProgress` hands each `notifications/progress` to `onProgress` as it arrives and rejects with `MusterAnswerCutError` when the answer ends before its result. muster-backend serves it on `POST /call/progress` as newline-delimited JSON (`progress` lines, then one `result` or `error` line); a caller that goes away cancels the call. gs-node's `MusterMcpClient.callToolWithProgress` runs the call on the official MCP SDK (`@modelcontextprotocol/sdk` 1.32.1), restarting its wait on every notification, up to 65 minutes.
34 changes: 31 additions & 3 deletions plugins/bot-prs/src/apis/MargeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ export class MargeClient {
} catch (error) {
throw classifyMargeError(error);
}
return this.parse<T>(tool, result);
}

private parse<T>(tool: MargeTool, result: unknown): T {
if (typeof result === 'string') {
try {
return JSON.parse(result) as T;
Expand Down Expand Up @@ -99,9 +103,33 @@ export class MargeClient {
return answer;
}

/** The sweep, whole team or narrowed; `dry_run` is the preview. */
sweep(args: MargeSweepArgs): Promise<MargeResult> {
return this.call<MargeResult>(MARGE_TOOLS.sweep, { ...args });
/**
* The sweep, whole team or narrowed; `dry_run` is the preview. marge
* reports its progress while it works (`34 of 79 PRs done`), and the call
* lasts as long as it keeps reporting, however long the team's queue.
*/
async sweep(
args: MargeSweepArgs,
options: { onProgress?: (message: string) => void } = {},
): Promise<MargeResult> {
let result: unknown;
try {
result = await this.musterApi.callToolWithProgress(
margeToolName(MARGE_TOOLS.sweep),
{ ...args },
{
installation: this.installation,
onProgress: ({ message }) => {
if (message) {
options.onProgress?.(message);
}
},
},
);
} catch (error) {
throw classifyMargeError(error);
}
return this.parse<MargeResult>(MARGE_TOOLS.sweep, result);
}

/** An ai-rescue marker on one PR, as the person. */
Expand Down
12 changes: 11 additions & 1 deletion plugins/bot-prs/src/components/BotPrsPage/BotPrsPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,17 @@ jest.mock('../../hooks/useMarge', () => ({

const callTool = jest.fn();
const listServers = jest.fn();
const musterApi = { callTool, listServers } as unknown as MusterApi;
// A sweep streams its progress; the stand-in answers it as callTool does.
const callToolWithProgress = (
name: string,
args: Record<string, unknown>,
{ installation }: { installation?: string },
) => callTool(name, args, installation);
const musterApi = {
callTool,
callToolWithProgress,
listServers,
} as unknown as MusterApi;

const summary = {
total: 3,
Expand Down
5 changes: 5 additions & 0 deletions plugins/bot-prs/src/components/BotPrsPage/BotPrsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
import { BotPrsFilters } from '../BotPrsFilters';
import { ClassificationLegend } from '../ClassificationLegend';
import { BotPrsTable } from '../BotPrsTable';
import { SweepProgress } from '../SweepProgress';
import { ConnectMargeAlert } from '../ConnectMargeAlert';
import { MarkBlockedDialog } from '../MarkBlockedDialog';
import { MergeGreenDialog } from '../MergeGreenDialog';
Expand Down Expand Up @@ -398,6 +399,10 @@ function Queue({
title={`Classifying ${queue.classifying.length > 0 ? queue.classifying.join(', ') : 'the PRs in view'}`}
description="The engine reads every PR's checks and writes the class it decides to the PR's label. One check read per PR, so a team of many repositories takes a minute. The table reloads when it is done."
/>
<SweepProgress
progress={queue.classifyProgress}
showTeam={queue.classifying.length > 1}
/>
</Box>
) : null}
{queue.classifyError instanceof MargeAnswerLostError ? (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,17 @@ import { MergeGreenDialog } from './MergeGreenDialog';

const callTool = jest.fn();
const listServers = jest.fn();
const musterApi = { callTool, listServers } as unknown as MusterApi;
// A sweep streams its progress; the stand-in answers it as callTool does.
const callToolWithProgress = (
name: string,
args: Record<string, unknown>,
{ installation }: { installation?: string },
) => callTool(name, args, installation);
const musterApi = {
callTool,
callToolWithProgress,
listServers,
} as unknown as MusterApi;

const summary = {
total: 2,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { greenByTeam, refsByTeam } from '../../lib/rows';
import { ConnectMargeAlert } from '../ConnectMargeAlert';
import { LostAnswerAlert } from '../LostAnswerAlert';
import { OutcomeTable } from '../OutcomeTable';
import { SweepProgress } from '../SweepProgress';

export type MergeGreenDialogProps = {
installation: string;
Expand Down Expand Up @@ -173,6 +174,9 @@ export function MergeGreenDialog({
its team&apos;s policy. A PR that is no longer green is left alone.
</Text>
)}
{sweeps.isPending ? (
<SweepProgress progress={sweeps.progress} showTeam={teams.length > 1} />
) : null}
{notConnected ? (
<ConnectMargeAlert
installation={installation}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,17 @@ import { SweepDialog } from './SweepDialog';

const callTool = jest.fn();
const listServers = jest.fn();
const musterApi = { callTool, listServers } as unknown as MusterApi;
// A sweep streams its progress; the stand-in answers it as callTool does.
const callToolWithProgress = (
name: string,
args: Record<string, unknown>,
{ installation }: { installation?: string },
) => callTool(name, args, installation);
const musterApi = {
callTool,
callToolWithProgress,
listServers,
} as unknown as MusterApi;

const summary = {
total: 1,
Expand Down
4 changes: 4 additions & 0 deletions plugins/bot-prs/src/components/SweepDialog/SweepDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { refsByTeam } from '../../lib/rows';
import { ConnectMargeAlert } from '../ConnectMargeAlert';
import { LostAnswerAlert } from '../LostAnswerAlert';
import { OutcomeTable } from '../OutcomeTable';
import { SweepProgress } from '../SweepProgress';

export type SweepDialogProps = {
installation: string;
Expand Down Expand Up @@ -207,6 +208,9 @@ export function SweepDialog({
</Flex>
</Flex>
) : null}
{sweeps.isPending ? (
<SweepProgress progress={sweeps.progress} showTeam={teams.length > 1} />
) : null}
{notConnected ? (
<ConnectMargeAlert
installation={installation}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { render, screen } from '@testing-library/react';
import { SweepProgress } from './SweepProgress';

describe('SweepProgress', () => {
it('says where each team stands, naming the team when there are several', () => {
render(
<SweepProgress
progress={{
honeybadger: '34 of 79 PRs done',
atlas: '2 of 3 PRs done',
}}
showTeam
/>,
);

expect(screen.getByRole('status')).toHaveTextContent(
'atlas: 2 of 3 PRs donehoneybadger: 34 of 79 PRs done',
);
});

it('renders nothing before the first report', () => {
const { container } = render(<SweepProgress progress={{}} />);
expect(container).toBeEmptyDOMElement();
});
});
32 changes: 32 additions & 0 deletions plugins/bot-prs/src/components/SweepProgress/SweepProgress.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { Flex, Text } from '@backstage/ui';

export type SweepProgressProps = {
/** marge's last report per team, such as `34 of 79 PRs done`. */
progress: Record<string, string>;
/** Name each team; a run of one team needs no name. */
showTeam?: boolean;
};

/**
* Where a run in flight stands, per team, as marge last reported it. The
* reports arrive while marge works, so a long run shows its advance instead
* of a spinner alone.
*/
export function SweepProgress({
progress,
showTeam = false,
}: SweepProgressProps) {
const teams = Object.keys(progress).sort((a, b) => a.localeCompare(b, 'en'));
if (teams.length === 0) {
return null;
}
return (
<Flex direction="column" gap="1" role="status" aria-live="polite">
{teams.map(team => (
<Text key={team} variant="body-small" color="secondary">
{showTeam ? `${team}: ${progress[team]}` : progress[team]}
</Text>
))}
</Flex>
);
}
2 changes: 2 additions & 0 deletions plugins/bot-prs/src/components/SweepProgress/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { SweepProgress } from './SweepProgress';
export type { SweepProgressProps } from './SweepProgress';
94 changes: 93 additions & 1 deletion plugins/bot-prs/src/hooks/useMarge.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,31 @@ const queuesOf = (byTeam: Record<string, MargeResult | string>) => ({

const callTool = jest.fn();
const listServers = jest.fn();
const musterApi = { callTool, listServers } as unknown as MusterApi;
// What marge reports before a sweep answers, per call.
let sweepProgress: string[] = [];
// A sweep streams its progress; the stand-in reports `sweepProgress`, then
// answers as callTool does.
const callToolWithProgress = (
name: string,
args: Record<string, unknown>,
{
installation,
onProgress,
}: {
installation?: string;
onProgress: (progress: { progress: number; message?: string }) => void;
},
) => {
sweepProgress.forEach((message, index) =>
onProgress({ progress: index + 1, message }),
);
return callTool(name, args, installation);
};
const musterApi = {
callTool,
callToolWithProgress,
listServers,
} as unknown as MusterApi;

const summary = {
total: 1,
Expand Down Expand Up @@ -95,6 +119,7 @@ const client = () =>
beforeEach(() => {
callTool.mockReset();
listServers.mockReset();
sweepProgress = [];
});

describe('useBotPrs', () => {
Expand Down Expand Up @@ -132,6 +157,38 @@ describe('useBotPrs', () => {
expect(result.current.queues[0].result).toBeUndefined();
});

it('shows where each team’s classification stands until it answers', async () => {
sweepProgress = ['120 of 290 PRs done'];
let answer: (result: MargeResult) => void = () => {};
callTool
.mockResolvedValueOnce(queuesOf({ honeybadger: stored }))
.mockReturnValueOnce(
new Promise<MargeResult>(resolve => {
answer = resolve;
}),
)
.mockResolvedValue(queuesOf({ honeybadger: live }));
const { result } = renderHook(() => useBotPrs('gazelle', ['honeybadger']), {
wrapper: wrapperWith(client()),
});
await waitFor(() =>
expect(result.current.queues[0].result).toEqual(stored),
);

act(() => result.current.classify());

await waitFor(() =>
expect(result.current.classifyProgress).toEqual({
honeybadger: '120 of 290 PRs done',
}),
);
expect(result.current.isClassifying).toBe(true);

await act(async () => answer(live));
await waitFor(() => expect(result.current.isClassifying).toBe(false));
expect(result.current.classifyProgress).toEqual({});
});

it('classifies only when asked, and writes the class to each label', async () => {
callTool
.mockResolvedValueOnce(queuesOf({ bumblebee: stored, atlas: stored }))
Expand Down Expand Up @@ -278,6 +335,41 @@ describe('useBotPrs', () => {
});

describe('useMargeTeamSweeps', () => {
it('shows where each team stands while the run is in flight, and nothing after', async () => {
sweepProgress = ['finding the PRs', '34 of 79 PRs done'];
let answer: (result: MargeResult) => void = () => {};
callTool.mockReturnValue(
new Promise<MargeResult>(resolve => {
answer = resolve;
}),
);
const { result } = renderHook(() => useMargeTeamSweeps('gazelle'), {
wrapper: wrapperWith(client()),
});

let run: Promise<unknown> = Promise.resolve();
act(() => {
run = result.current.run({
teams: ['honeybadger'],
actions: 'approve,merge,mark',
dryRun: false,
});
});

await waitFor(() =>
expect(result.current.progress).toEqual({
honeybadger: '34 of 79 PRs done',
}),
);
expect(result.current.isPending).toBe(true);

await act(async () => {
answer(live);
await run;
});
await waitFor(() => expect(result.current.progress).toEqual({}));
});

it('calls marge once per team with that team\u2019s PRs, and invalidates only on a write', async () => {
callTool.mockResolvedValue(live);
const queryClient = client();
Expand Down
Loading
Loading