Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion apps/ade-cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ ade prs create --lane lane-id --base main --close-linear-issue-on-merge
ade prs list-open --text
ade prs github-snapshot --include-external-closed --history-page-limit 4
ade prs github-snapshot --include-state-counts --no-revalidate
ade prs checks pr-id --text
ade prs checks pr-id --text # header carries the canonical rollup (checksStatus/checksCounts); "not run" means nothing verified the commit, whatever the rows say
ade prs comments pr-id --text
ade shell start --lane lane-id -- npm test
ade terminal list --lane lane-id --text
Expand Down
36 changes: 36 additions & 0 deletions apps/ade-cli/src/adeRpcServer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,42 @@ function createFakePathExecutable(dir: string, name: string): string {
}

describe("adeRpcServer", () => {
it("does not report a green rollup to agents when only third-party bots reported", async () => {
// ADE-135: `summarizePrChecks` initialised `overall` to "passing" and was
// producer-blind, so PR #988's three bot successes came back as green on
// the exact surface an autonomous agent reads before deciding to merge.
const { runtime } = createRuntime();
runtime.prService.getChecks = vi.fn(async () => [
{ name: "CodeRabbit", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "coderabbitai" },
{ name: "Vercel", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "vercel" },
{ name: "Vercel Preview Comments", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "vercel" },
]);
const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" });
await initialize(handler, { role: "agent", chatSessionId: "session-1" });

const result = await callTool(handler, "pr_get_checks", { prId: "pr-1" });
const payload = result.structuredContent ?? result;

expect(payload.checksStatus).toBe("not_run");
expect(payload.checksCounts.passing).toBe(0);
expect(payload.checksCounts.total).toBe(3);
expect(payload.checks[0].appSlug).toBe("coderabbitai");
});

it("reports zero checks as none rather than defaulting to passing", async () => {
const { runtime } = createRuntime();
runtime.prService.getChecks = vi.fn(async () => []);
const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" });
await initialize(handler, { role: "agent", chatSessionId: "session-1" });

const result = await callTool(handler, "pr_get_checks", { prId: "pr-1" });
const payload = result.structuredContent ?? result;

expect(payload.checksStatus).not.toBe("passing");
expect(payload.checksStatus).toBe("none");
expect(payload.checksCounts.total).toBe(0);
});

it("exposes direct PTY RPC methods with enriched create/list responses", async () => {
const { runtime } = createRuntime();
const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" });
Expand Down
73 changes: 60 additions & 13 deletions apps/ade-cli/src/adeRpcServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { getDefaultModelDescriptor } from "../../desktop/src/shared/modelRegistr
import { buildAdeCliInlineGuidance } from "../../desktop/src/shared/adeCliGuidance";
import { buildDeeplink, isValidCommitSha, isValidRepoRelativePath } from "../../desktop/src/shared/deeplinks";
import { resolveStableLaneBaseBranch } from "../../desktop/src/shared/laneBaseResolution";
import { rollupPrChecks } from "../../desktop/src/shared/prChecksRollup";
import {
ADE_AGENT_SKILLS_DIRS_ENV,
getAdeAgentSkillRootsForPrompt,
Expand All @@ -41,7 +42,7 @@ import {
type MergeMethod,
type AppNavigationRequest,
} from "../../desktop/src/shared/types";
import type { PrCheck, PrComment, PrReviewThread } from "../../desktop/src/shared/types/prs";
import type { PrCheck, PrChecksStatus, PrComment, PrReviewThread } from "../../desktop/src/shared/types/prs";
import type { CtoLinearQuickView } from "../../desktop/src/shared/types/cto";
import type { LinearConnectionStatus } from "../../desktop/src/shared/types/linearSync";
import { resolveAdeLayout } from "../../desktop/src/shared/adeLayout";
Expand Down Expand Up @@ -1860,20 +1861,43 @@ function requirePrService(runtime: AdeRuntime): NonNullable<AdeRuntime["prServic
return runtime.prService;
}

function summarizePrChecks(checks: PrCheck[]): { overall: "failing" | "pending" | "passing"; counts: { passing: number; failing: number; pending: number; total: number } } {
const passing = checks.filter((check) => check.conclusion === "success").length;
const failing = checks.filter((check) => check.conclusion === "failure").length;
const pending = checks.filter((check) => check.status !== "completed").length;

let overall: "failing" | "pending" | "passing" = "passing";
if (failing > 0) overall = "failing";
else if (pending > 0) overall = "pending";

return { overall, counts: { passing, failing, pending, total: checks.length } };
function summarizePrChecks(checks: PrCheck[]): {
overall: PrChecksStatus;
counts: { passing: number; failing: number; pending: number; total: number };
} {
// ADE-135: this used to carry its own pass/fail rule and got it wrong twice
// over — it initialised `overall` to "passing", so zero checks read green,
// and it was producer-blind, so a single rate-limited CodeRabbit `success`
// also read green. That is the ticket's bug on the surface agents read.
// The shared rollup is the only authority now.
const { status, counts } = rollupPrChecks(checks);
return {
overall: status,
counts: {
passing: counts.passing,
failing: counts.failing,
pending: counts.pending,
total: counts.total,
},
};
}

function mapCheckToSummary(check: PrCheck): { name: string; status: string; conclusion: string | null; url: string | null } {
return { name: check.name, status: check.status, conclusion: check.conclusion, url: check.detailsUrl };
function mapCheckToSummary(check: PrCheck): {
name: string;
status: string;
conclusion: string | null;
url: string | null;
appSlug: string | null;
} {
// ADE-135: without the producer, an agent reading three rows of
// `conclusion: "success"` has no way to know that not one of them is CI.
return {
name: check.name,
status: check.status,
conclusion: check.conclusion,
url: check.detailsUrl,
appSlug: check.appSlug ?? null,
};
}

function summarizePrReviewComments(
Expand Down Expand Up @@ -5191,9 +5215,32 @@ async function runTool(args: {
const prId = assertNonEmptyString(toolArgs.prId, "prId");
const prSvc = requirePrService(runtime);
const checks = await prSvc.getChecks(prId);
// The aggregate travels with the rows so an agent does not have to
// re-derive "was this verified?" and get it wrong, which is the bug this
// ticket exists to fix.
//
// The PERSISTED verdict wins when we have it. A row-level rollup only sees
// the flattened checks: it cannot see required contexts that never
// reported, the merge-state corroboration, or the grace window, all of
// which live in `computeStatus`. Reporting the row tally here would let
// this tool say "passing" while every human surface says "not run" — on
// precisely the surface an autonomous agent reads before merging.
const { overall, counts } = summarizePrChecks(checks);
// Best-effort: `listAll` is a local DB read, but a degraded runtime may not
// expose it. Falling back to the row tally is still better than throwing.
let summary: { checksStatus?: string; checksReason?: string | null; checksMissingRequired?: string[] | null } | null = null;
try {
summary = prSvc.listAll?.().find((entry) => entry.id === prId) ?? null;
} catch {
summary = null;
}
return {
success: true,
prId,
checksStatus: summary?.checksStatus ?? overall,
checksReason: summary?.checksReason ?? null,
checksMissingRequired: summary?.checksMissingRequired ?? [],
checksCounts: counts,
checks: checks.map(mapCheckToSummary),
};
}
Expand Down
47 changes: 47 additions & 0 deletions apps/ade-cli/src/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2784,6 +2784,53 @@ describe("ADE CLI", () => {
});
});

// ADE-135: `ade prs checks` printed nothing but the row table, so three
// third-party bot rows each rendering `OK` read as a passing commit. The
// canonical rollup now travels with the rows and leads the output.
it("leads `prs checks --text` with the canonical rollup, not the row tally", () => {
const opts = {
...baseResolveOpts(),
projectRoot: null,
workspaceRoot: null,
text: true,
};
const notRun = formatOutput(
{
success: true,
prId: "pr-988",
checksStatus: "not_run",
checksCounts: { passing: 0, failing: 0, pending: 0, total: 3 },
checks: [
{ name: "CodeRabbit", status: "completed", conclusion: "success", appSlug: "coderabbitai" },
{ name: "Vercel — Preview", status: "completed", conclusion: "success", appSlug: "vercel" },
{ name: "changeset-bot", status: "completed", conclusion: "success", appSlug: "changeset-bot" },
],
},
opts,
"pr-checks",
);
expect(notRun).toContain("ADE PR checks - not run");
expect(notRun).toContain("3 checks reported");
// The raw enum must never reach the reader — the phrase is the point.
expect(notRun).not.toContain("not_run");

const passing = formatOutput(
{
success: true,
prId: "pr-42",
checksStatus: "passing",
checksCounts: { passing: 2, failing: 0, pending: 0, total: 2 },
checks: [
{ name: "ci / unit", status: "completed", conclusion: "success", appSlug: "github-actions" },
{ name: "ci / lint", status: "completed", conclusion: "success", appSlug: "github-actions" },
],
},
opts,
"pr-checks",
);
expect(passing).toContain("ADE PR checks - passing (2 passing");
});

describe("chat create parent lineage", () => {
const savedParentEnv = process.env.ADE_CHAT_SESSION_ID;
afterEach(() => {
Expand Down
43 changes: 41 additions & 2 deletions apps/ade-cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1608,7 +1608,7 @@ const HELP_BY_COMMAND: Record<string, string> = {
$ ade prs create --lane <lane> --base main Open and map a GitHub PR; prints GitHub + ADE URLs
$ ade prs create --lane <lane> --close-linear-issue-on-merge
$ ade prs link --lane <lane> --url <pr-url> Map an existing GitHub PR to a lane
$ ade prs checks <pr> --text Show check status
$ ade prs checks <pr> --text Show the CI rollup + per-check rows ("not run" = nothing verified the commit)
$ ade prs comments <pr> --text Show unresolved review work
$ ade prs github-snapshot --include-external-closed --history-page-limit 4
Include bounded closed PR history in the GitHub snapshot
Expand Down Expand Up @@ -17599,6 +17599,10 @@ function statusWord(value: unknown): string {
return "FAIL";
if (["pending", "running", "in_progress", "queued", "active"].includes(raw))
return "WAIT";
// ADE-135: `not_run` is a checks rollup, not a job conclusion, and it must
// never read as a raw enum — the sentence it stands for is "nothing verified
// this commit".
if (raw === "not_run") return "NOT RUN";
return raw.toUpperCase();
}

Expand Down Expand Up @@ -17715,10 +17719,45 @@ function formatPrCreate(value: unknown): string {
]);
}

/**
* Header verdict for `ade prs checks`.
*
* ADE-135: the table alone is the bug. Three third-party bot rows each render
* `OK`, and a reader — human or agent — concludes the commit passed CI. The
* canonical rollup now travels with the rows as `checksStatus`/`checksCounts`,
* so the verdict leads. `not_run` is spelled out rather than leaked as a raw
* enum: it is the one status whose whole point is that nothing verified the
* commit.
*/
function formatPrChecksVerdict(value: unknown): string | null {
if (!isRecord(value)) return null;
const status = asString(value.checksStatus);
if (!status) return null;
// `none` means nothing reported and nothing was expected. Printing the bare
// enum beside an empty table reads as a value, not a sentence — the same
// complaint that motivates the `not_run` relabel below.
if (status === "none") return null;
const label = status === "not_run" ? "not run" : status;
const counts = isRecord(value.checksCounts) ? value.checksCounts : null;
const parts: string[] = [];
for (const noun of ["passing", "failing", "pending"] as const) {
const count = counts ? counts[noun] : null;
if (typeof count === "number" && count > 0) parts.push(`${count} ${noun}`);
}
const total = counts ? counts.total : null;
if (typeof total === "number" && total > 0) {
parts.push(`${total} check${total === 1 ? "" : "s"} reported`);
}
return parts.length > 0 ? `${label} (${parts.join(", ")})` : label;
}

function formatPrChecks(value: unknown): string {
const checks = firstArray(value, ["checks", "items"]);
const summary = isRecord(value) ? value.summary : null;
const header = summary
const verdict = formatPrChecksVerdict(value);
const header = verdict
? `ADE PR checks - ${verdict}`
: summary
? `ADE PR checks - ${cell(summary, 80)}`
: "ADE PR checks";
return `${header}\n${renderTable(
Expand Down
30 changes: 30 additions & 0 deletions apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,36 @@ describe("Drawer PR pill", () => {
expect(frame).toContain("[#168 ≋2/3 ·4/6]");
});

// ADE-135: the counts are producer-blind, so a PR whose only checks are
// preview/review bots arrives here as N/N. The pill must not spend a number
// on that at all — "no ci" is the fact.
it("renders no-ci in the PR pill when the rollup says nothing verified the commit", () => {
const frame = stripAnsi(render(
<Drawer
lanes={[lane("lane-1", "opt prs tab", "ade/opt-prs-tab", "2026-05-12T11:55:00.000Z")]}
sessions={[]}
activeLaneId={null}
activeSessionId={null}
browsingLaneId={null}
selectedLaneIndex={0}
selectedChatIndex={-1}
panelHeight={20}
prByLaneId={{
"lane-1": {
number: 988,
state: "open",
checksPassed: 3,
checksTotal: 3,
checksStatus: "not_run",
},
}}
/>,
).lastFrame() ?? "");

expect(frame).toContain("[#988 ·no ci]");
expect(frame).not.toContain("3/3");
});

it("does not render closed or merged PR pills", () => {
for (const state of ["closed", "merged"] as const) {
const frame = stripAnsi(render(
Expand Down
29 changes: 29 additions & 0 deletions apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,35 @@ describe("RightPane lane-details", () => {
expect(frame).not.toContain("RUN");
});

// ADE-135: with three bot rows the counts read 0 passing / 0 failing /
// 0 pending / 3 total, and the old line called that "checks passing".
it("does not claim checks are passing when the rollup says CI never ran", () => {
const result = render(
<RightPane
content={{
kind: "lane-details",
...baseLaneDetails,
pr: {
number: 988,
state: "open",
url: "https://github.com/example/ADE/pull/988",
checksPassed: 0,
checksTotal: 3,
checksPending: 0,
checksFailed: 0,
checksStatus: "not_run",
},
}}
focused
/>,
);
const frame = stripAnsi(result.lastFrame() ?? "");

expect(frame).toContain("CI not run");
expect(frame).not.toContain("checks passing");
expect(frame).not.toContain("passing");
});

it("shows the PR GitHub link when the PR row is selected", () => {
const result = render(
<RightPane
Expand Down
Loading