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
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { render, screen } from "@testing-library/react";
import { describe, expect, it, vi } from "vitest";

// A maintainer session + a path-aware data hook: the dashboard call resolves with reviewability rows (one
// with a slop assessment, one without), every other call stays in loading so sub-panels render harmlessly.
const { useSession } = vi.hoisted(() => ({ useSession: vi.fn() }));
vi.mock("@/lib/api/session", () => ({ useSession: () => useSession() }));

const dashboard = {
metrics: [],
health: [],
reviewability: [
{
pr: "acme/widgets#7",
title: "Tidy things",
author: "alice",
bucket: "review-now",
reason: "cached open PR",
slop: { risk: 72, band: "high" },
},
{
pr: "acme/widgets#8",
title: "Real feature",
author: "bob",
bucket: "watch",
reason: "linked issue #3",
slop: null,
},
],
settingsPreview: { removed: [], added: [] },
};

vi.mock("@/lib/api/use-api-resource", () => ({
useApiResource: (path: string) =>
path.includes("maintainer-dashboard")
? { status: "ready", data: dashboard, reload: () => {}, error: null }
: { status: "loading", data: null, reload: () => {}, error: null },
}));
// AiReviewSettings.load() awaits apiFetch and reads `.ok`; a benign not-ok response keeps it from throwing.
vi.mock("@/lib/api/request", () => ({ apiFetch: vi.fn(async () => ({ ok: false })) }));
vi.mock("@/lib/api/origin", () => ({ getApiOrigin: () => "https://api.test" }));

import { MaintainerPanel } from "@/components/site/app-panels/maintainer-panel";

describe("MaintainerPanel slop score column", () => {
it("renders a slop band + risk pill for an assessed PR and an em-dash for an unassessed one", () => {
useSession.mockReturnValue({
session: { login: "maint", roles: ["maintainer"] },
hydrated: true,
});
render(<MaintainerPanel />);

// The new column header.
expect(screen.getByText("Slop")).toBeTruthy();
// Assessed PR → band + score rendered together.
expect(screen.getByText(/high\s*72/)).toBeTruthy();
// Unassessed PR → a muted em-dash placeholder.
expect(screen.getByText("—")).toBeTruthy();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ const BUCKET_TONE: Record<string, Status> = {
redirect: "blocked",
};

// Deterministic slop band → pill tone. Advisory only (it never blocks); the colour just signals severity.
const SLOP_BAND_TONE: Record<string, Status> = {
clean: "ok",
low: "info",
elevated: "warn",
high: "blocked",
};

type MaintainerDashboard = {
metrics: Array<{ label: string; value: number; spark: number[] }>;
health: Array<{
Expand All @@ -60,6 +68,7 @@ type MaintainerDashboard = {
author: string;
bucket: string;
reason: string;
slop?: { risk: number; band: string } | null;
}>;
settingsPreview: { removed: string[]; added: string[] };
};
Expand Down Expand Up @@ -286,6 +295,7 @@ function MaintainerDashboardView() {
<th className="py-2 pr-3 font-normal">Title</th>
<th className="py-2 pr-3 font-normal">Author</th>
<th className="py-2 pr-3 font-normal">Bucket</th>
<th className="py-2 pr-3 font-normal">Slop</th>
<th className="py-2 font-normal">Reason</th>
</tr>
</thead>
Expand All @@ -305,6 +315,20 @@ function MaintainerDashboardView() {
{row.bucket}
</StatusPill>
</td>
<td className="py-2 pr-3">
{row.slop ? (
<StatusPill status={SLOP_BAND_TONE[row.slop.band] ?? "info"}>
{row.slop.band} {row.slop.risk}
</StatusPill>
) : (
<span
className="text-token-xs text-muted-foreground"
title="Slop detection is off for this repo, or this PR has not been assessed yet."
>
</span>
)}
</td>
<td className="py-2 text-token-xs text-muted-foreground">{row.reason}</td>
</tr>
))}
Expand Down
7 changes: 7 additions & 0 deletions migrations/0035_pull_request_slop_assessment.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- Persist the latest deterministic slop assessment per cached pull request so the maintainer dashboard can
-- surface a slop score row without re-fetching changed files on every load. Written by the public-surface
-- processor ONLY when the repo opted into slop (slop_gate_mode != 'off'); NULL means "not assessed". These
-- are gittensory-COMPUTED signals, deliberately omitted from the GitHub-sync upsert's SET clause so a
-- subsequent sync never clobbers them.
ALTER TABLE pull_requests ADD COLUMN slop_risk INTEGER;
ALTER TABLE pull_requests ADD COLUMN slop_band TEXT;
3 changes: 3 additions & 0 deletions src/api/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,9 @@ export function createApp() {
author: pull.authorLogin ?? "unknown",
bucket: pull.state === "open" ? "review-now" : "watch",
reason: pull.linkedIssues.length > 0 ? `linked issue #${pull.linkedIssues[0]}` : "cached open PR without linked issue",
// Latest deterministic slop assessment for this PR (null unless the repo opted into slop). Lets the
// maintainer panel render a per-PR slop band; never a private/scoreability signal.
slop: typeof pull.slopRisk === "number" && pull.slopBand ? { risk: pull.slopRisk, band: pull.slopBand } : null,
})),
settingsPreview: buildMaintainerSettingsPreview(),
});
Expand Down
20 changes: 20 additions & 0 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3538,9 +3538,29 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
closedAt: payload.closed_at,
labels: parseJson<string[]>(row.labelsJson, []),
linkedIssues: parseJson<number[]>(row.linkedIssuesJson, []),
slopRisk: row.slopRisk,
slopBand: row.slopBand,
};
}

/**
* Persist the latest deterministic slop assessment onto an existing cached PR row. Kept separate from the
* GitHub-sync upsert (whose SET clause never touches these columns) so a later sync cannot clobber the
* score. A no-op when the PR row does not exist yet — the sync upsert creates it first.
*/
export async function updatePullRequestSlopAssessment(
env: Env,
repoFullName: string,
pullNumber: number,
assessment: { slopRisk: number; slopBand: string },
): Promise<void> {
const db = getDb(env.DB);
await db
.update(pullRequests)
.set({ slopRisk: assessment.slopRisk, slopBand: assessment.slopBand, updatedAt: nowIso() })
.where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pullNumber)));
}

function toIssueRecord(repoFullName: string, issue: GitHubIssuePayload): IssueRecord {
/* v8 ignore start -- GitHub REST row normalization covers sparse provider payloads at representative persistence call sites. */
return {
Expand Down
3 changes: 3 additions & 0 deletions src/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,9 @@ export const pullRequests = sqliteTable(
linkedIssuesJson: text("linked_issues_json").notNull().default("[]"),
lastSeenOpenAt: text("last_seen_open_at"),
payloadJson: text("payload_json").notNull().default("{}"),
// Latest deterministic slop assessment (gittensory-computed; written separately from the GitHub sync).
slopRisk: integer("slop_risk"),
slopBand: text("slop_band"),
createdAt: text("created_at").notNull().$defaultFn(() => nowIso()),
updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()),
},
Expand Down
4 changes: 4 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
listPullRequests,
listPullRequestFiles,
listRecentMergedPullRequests,
updatePullRequestSlopAssessment,
listRepoLabels,
listRepoPullRequestFiles,
listRepoSyncStates,
Expand Down Expand Up @@ -1152,6 +1153,9 @@ async function maybePublishPrPublicSurface(
});
slopRisk = slop.slopRisk;
advisory.findings.push(...slop.findings);
// Persist the assessment so the maintainer dashboard can show a per-PR slop score row without
// re-fetching changed files. Best-effort: a write hiccup must not abort gate evaluation.
await updatePullRequestSlopAssessment(env, repoFullName, pr.number, { slopRisk: slop.slopRisk, slopBand: slop.band }).catch(() => undefined);
// AI-assisted slop advisory (#533, opt-in). Reuses the already-fetched files; appends at most one
// advisory-only finding. Deliberately does NOT update slopRisk — only the deterministic core blocks.
if (settings.slopAiAdvisory) {
Expand Down
4 changes: 4 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,10 @@ export type PullRequestRecord = {
closedAt?: string | null | undefined;
labels: string[];
linkedIssues: number[];
/** Latest deterministic slop assessment (0-100) and band, persisted by the public-surface processor when
* the repo opted into slop. `null`/absent = not assessed (slop off, or PR not yet processed). */
slopRisk?: number | null | undefined;
slopBand?: string | null | undefined;
};

export type IssueRecord = {
Expand Down
7 changes: 5 additions & 2 deletions test/integration/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
recordAuditEvent,
upsertIssueFromGitHub,
upsertPullRequestFromGitHub,
updatePullRequestSlopAssessment,
persistScoringModelSnapshot,
upsertRepositoryFromGitHub,
upsertRepositorySettings,
Expand Down Expand Up @@ -2314,14 +2315,16 @@ describe("api routes", () => {
labels: [],
body: "No linked issue here.",
});
// PR2: a persisted slop assessment surfaces on the dashboard row; an unassessed PR carries slop: null.
await updatePullRequestSlopAssessment(env, "entrius/allways-ui", 14, { slopRisk: 80, slopBand: "high" });
const maintainer = await app.request("/v1/app/maintainer-dashboard", { headers: apiHeaders(env) }, env);
expect(maintainer.status).toBe(200);
await expect(maintainer.json()).resolves.toMatchObject({
installations: expect.any(Array),
health: expect.arrayContaining([expect.objectContaining({ status: "healthy" })]),
reviewability: expect.arrayContaining([
expect.objectContaining({ pr: "entrius/allways-ui#12" }),
expect.objectContaining({ pr: "entrius/allways-ui#14", author: "unknown", reason: "cached open PR without linked issue" }),
expect.objectContaining({ pr: "entrius/allways-ui#12", slop: null }),
expect.objectContaining({ pr: "entrius/allways-ui#14", author: "unknown", reason: "cached open PR without linked issue", slop: { risk: 80, band: "high" } }),
]),
settingsPreview: { added: expect.any(Array), removed: expect.any(Array) },
});
Expand Down
21 changes: 21 additions & 0 deletions test/unit/data-spine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
upsertInstallationHealth,
upsertIssueFromGitHub,
upsertPullRequestFromGitHub,
updatePullRequestSlopAssessment,
upsertPullRequestFile,
upsertPullRequestReview,
upsertRecentMergedPullRequest,
Expand Down Expand Up @@ -349,4 +350,24 @@ describe("data spine repositories", () => {
expect(await listContributorPullRequests(env, "jsonbored")).toMatchObject([{ repoFullName: "owner/repo", number: 1 }]);
expect(await listContributorIssues(env, "JSONBORED")).toEqual(expect.arrayContaining([expect.objectContaining({ repoFullName: "owner/repo", number: 10 }), expect.objectContaining({ repoFullName: "owner/repo", number: 11 })]));
});

it("persists a per-PR slop assessment, round-trips it via the cached record, and keeps latest-wins (PR2)", async () => {
const env = createTestEnv();
await upsertPullRequestFromGitHub(env, "owner/sloppr", { number: 5, title: "Churn", state: "open", user: { login: "alice" }, labels: [], body: "x" });
// Unassessed by default (slop off, or PR not yet processed).
expect((await getPullRequest(env, "owner/sloppr", 5))?.slopRisk ?? null).toBeNull();
expect((await getPullRequest(env, "owner/sloppr", 5))?.slopBand ?? null).toBeNull();

await updatePullRequestSlopAssessment(env, "owner/sloppr", 5, { slopRisk: 72, slopBand: "high" });
const assessed = await getPullRequest(env, "owner/sloppr", 5);
expect(assessed?.slopRisk).toBe(72);
expect(assessed?.slopBand).toBe("high");

// Latest assessment wins on the next run.
await updatePullRequestSlopAssessment(env, "owner/sloppr", 5, { slopRisk: 10, slopBand: "low" });
expect((await getPullRequest(env, "owner/sloppr", 5))?.slopBand).toBe("low");

// No-op (no throw) when the PR row does not exist yet.
await expect(updatePullRequestSlopAssessment(env, "owner/sloppr", 999, { slopRisk: 5, slopBand: "low" })).resolves.toBeUndefined();
});
});
5 changes: 5 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,11 @@ describe("queue processors", () => {
gateCheckMode: "enabled",
linkedIssueGateMode: "off",
aiReviewMode: "block",
// Also exercise the opt-in slop advisory in the same surface pass: it persists a per-PR assessment
// and runs the (advisory-only) AI slop pass, but never blocks — the gate still fails on the AI
// consensus defect alone.
slopGateMode: "advisory",
slopAiAdvisory: true,
});
let gatePatchBody: { conclusion?: string; output?: { title?: string; text?: string } } = {};
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
Expand Down
Loading