fix(briefs): address review feedback

This commit is contained in:
Dotta committed 2026-05-24 02:46:35 +00:00
1 parent 6676b48b6c
commit 7c304a8943
5 files changed
+175 -21

No files matched your search

+53 -21
View File
@@ -59,8 +59,11 @@ type BriefSourceRow = {
is_intra_tree_blocked: boolean | null;
event_at: string;
metadata: unknown;
total_source_count?: number;
};
const SOURCES_PER_CARD_LIMIT = 200;
function table(namespace: string, name: string): string {
return `${namespace}.${name}`;
}
@@ -107,7 +110,12 @@ function toSource(row: BriefSourceRow): BriefCardSource {
};
}
function toCard(row: BriefCardRow, snapshot: BriefSnapshot, sources: BriefCardSource[]): BriefCard {
function toCard(
row: BriefCardRow,
snapshot: BriefSnapshot,
sources: BriefCardSource[],
totalSourceCount = sources.length,
): BriefCard {
return {
id: row.id,
companyId: row.company_id,
@@ -126,7 +134,7 @@ function toCard(row: BriefCardRow, snapshot: BriefSnapshot, sources: BriefCardSo
lastMeaningfulEventAt: row.last_meaningful_event_at,
snapshot,
sources,
moreSourceCount: Math.max(0, sources.length - snapshot.taskRows.length),
moreSourceCount: Math.max(0, totalSourceCount - snapshot.taskRows.length),
};
}
@@ -148,29 +156,53 @@ export function createBriefsStore(db: PluginDatabaseClient) {
[input.companyId, input.userId, Boolean(input.includeHidden), input.limit ?? 50],
);
const snapshotIds = cardRows
.map((card) => card.latest_snapshot_id)
.filter((id): id is string => Boolean(id));
if (snapshotIds.length === 0) return [];
const snapshots = await db.query<BriefSnapshotRow>(
`SELECT id, company_id, user_id, card_id, summary_paragraph, summary_status, summary_model,
summary_tokens_in, summary_tokens_out, summary_failure_reason, task_rows, evidence_source_ids,
generated_by_agent_id, generated_by_run_id, deterministic_state_inputs, created_at
FROM ${snapshotsTable}
WHERE company_id = $1 AND user_id = $2 AND id = ANY($3::uuid[])`,
[input.companyId, input.userId, snapshotIds],
);
const snapshotsById = new Map(snapshots.map((snapshot) => [snapshot.id, snapshot]));
const cardIds = cardRows.map((card) => card.id);
const sourceRows = await db.query<BriefSourceRow>(
`WITH ranked_sources AS (
SELECT id, company_id, user_id, card_id, source_kind, source_id, issue_id, identifier, title_line,
right_tag, link_path, is_intra_tree_blocked, event_at, metadata,
count(*) OVER (PARTITION BY card_id)::int AS total_source_count,
row_number() OVER (PARTITION BY card_id ORDER BY event_at DESC, id DESC) AS source_rank
FROM ${sourcesTable}
WHERE company_id = $1 AND user_id = $2 AND card_id = ANY($3::uuid[])
)
SELECT id, company_id, user_id, card_id, source_kind, source_id, issue_id, identifier, title_line,
right_tag, link_path, is_intra_tree_blocked, event_at, metadata, total_source_count
FROM ranked_sources
WHERE source_rank <= $4
ORDER BY card_id, event_at DESC, id DESC`,
[input.companyId, input.userId, cardIds, SOURCES_PER_CARD_LIMIT],
);
const sourcesByCardId = new Map<string, BriefCardSource[]>();
const sourceCountsByCardId = new Map<string, number>();
for (const row of sourceRows) {
const sources = sourcesByCardId.get(row.card_id) ?? [];
sources.push(toSource(row));
sourcesByCardId.set(row.card_id, sources);
sourceCountsByCardId.set(row.card_id, row.total_source_count ?? sources.length);
}
const cards: BriefCard[] = [];
for (const card of cardRows) {
if (!card.latest_snapshot_id) continue;
const snapshots = await db.query<BriefSnapshotRow>(
`SELECT id, company_id, user_id, card_id, summary_paragraph, summary_status, summary_model,
summary_tokens_in, summary_tokens_out, summary_failure_reason, task_rows, evidence_source_ids,
generated_by_agent_id, generated_by_run_id, deterministic_state_inputs, created_at
FROM ${snapshotsTable}
WHERE company_id = $1 AND user_id = $2 AND card_id = $3 AND id = $4
LIMIT 1`,
[input.companyId, input.userId, card.id, card.latest_snapshot_id],
);
const snapshot = snapshots[0];
const snapshot = snapshotsById.get(card.latest_snapshot_id);
if (!snapshot) continue;
const sources = await db.query<BriefSourceRow>(
`SELECT id, company_id, user_id, card_id, source_kind, source_id, issue_id, identifier, title_line,
right_tag, link_path, is_intra_tree_blocked, event_at, metadata
FROM ${sourcesTable}
WHERE company_id = $1 AND user_id = $2 AND card_id = $3
ORDER BY event_at DESC, id DESC`,
[input.companyId, input.userId, card.id],
);
cards.push(toCard(card, toSnapshot(snapshot), sources.map(toSource)));
const sources = sourcesByCardId.get(card.id) ?? [];
cards.push(toCard(card, toSnapshot(snapshot), sources, sourceCountsByCardId.get(card.id)));
}
return cards;
},
@@ -614,6 +614,7 @@ const plugin = definePlugin({
const userId = "userId" in variables && typeof variables.userId === "string" && variables.userId.trim()
? variables.userId.trim()
: scopedUserId(input, request);
assertTrustedBridgeUserScope(userId, request);
const nextVariables: Record<string, string | number | boolean> = { ...variables, userId };
if (routineKey === MANUAL_REFRESH_ROUTINE_KEY && typeof nextVariables.rootIssueId !== "string") {
throw new Error("rootIssueId is required for manual card refresh");
@@ -275,6 +275,11 @@ describe("Briefs managed resources", () => {
...victimParams,
cadence: "daily",
}, context)).rejects.toThrow("Briefs user scope mismatch");
await expect(harness.performAction("run-managed-routine", {
companyId,
routineKey: BRIEFS_MANAGED_ROUTINE_KEYS[0],
variables: { userId: "victim-user" },
}, context)).rejects.toThrow("Briefs user scope mismatch");
});
});
@@ -64,6 +64,9 @@ type CardRow = {
hidden: boolean;
};
const cardId = "33333333-3333-4333-8333-333333333333";
const snapshotId = "44444444-4444-4444-8444-444444444444";
function createFakeDb(): PluginDatabaseClient {
const cards = new Map<string, CardRow>();
const key = (company: unknown, user: unknown, slug: unknown) => `${company}:${user}:${slug}`;
@@ -102,6 +105,100 @@ function createFakeDb(): PluginDatabaseClient {
};
}
function createListFakeDb(): { db: PluginDatabaseClient; queries: Array<{ sql: string; params: unknown[] }> } {
const queries: Array<{ sql: string; params: unknown[] }> = [];
return {
queries,
db: {
namespace: "plugin_briefs_test",
async query<T>(sql: string, params: unknown[] = []): Promise<T[]> {
queries.push({ sql, params });
if (sql.includes("FROM plugin_briefs_test.briefs_cards")) {
return [{
id: cardId,
company_id: companyId,
user_id: userId,
slug: "pap-1",
title: "Briefs plugin planning",
grouping_description: "Issue tree rooted at PAP-1",
root_issue_id: rootIssueId,
state: "live",
summary_status: "ok",
pinned: false,
hidden: false,
stale_at: "2026-05-22T12:00:00.000Z",
expires_at: null,
latest_snapshot_id: snapshotId,
last_meaningful_event_at: "2026-05-22T12:00:00.000Z",
}] as T[];
}
if (sql.includes("FROM plugin_briefs_test.briefs_card_snapshots")) {
return [{
id: snapshotId,
company_id: companyId,
user_id: userId,
card_id: cardId,
summary_paragraph: "The work is moving.",
summary_status: "ok",
summary_model: "test",
summary_tokens_in: 10,
summary_tokens_out: 20,
summary_failure_reason: null,
task_rows: [],
evidence_source_ids: [],
generated_by_agent_id: null,
generated_by_run_id: null,
deterministic_state_inputs: {},
created_at: "2026-05-22T12:00:00.000Z",
}] as T[];
}
if (sql.includes("WITH ranked_sources AS")) {
return [
{
id: "55555555-5555-4555-8555-555555555555",
company_id: companyId,
user_id: userId,
card_id: cardId,
source_kind: "issue",
source_id: rootIssueId,
issue_id: rootIssueId,
identifier: "PAP-1",
title_line: "Briefs plugin planning",
right_tag: "live",
link_path: "/PAP/issues/PAP-1",
is_intra_tree_blocked: false,
event_at: "2026-05-22T12:00:00.000Z",
metadata: {},
total_source_count: 250,
},
{
id: "66666666-6666-4666-8666-666666666666",
company_id: companyId,
user_id: userId,
card_id: cardId,
source_kind: "run",
source_id: "run-1",
issue_id: rootIssueId,
identifier: "PAP-1",
title_line: "Agent run",
right_tag: "queued",
link_path: "/PAP/issues/PAP-1",
is_intra_tree_blocked: false,
event_at: "2026-05-22T11:59:00.000Z",
metadata: {},
total_source_count: 250,
},
] as T[];
}
return [];
},
async execute(): Promise<{ rowCount: number }> {
return { rowCount: 0 };
},
},
};
}
describe("Briefs store", () => {
it("keeps dismissed cards hidden when the same card is regenerated", async () => {
const store = createBriefsStore(createFakeDb());
@@ -129,4 +226,20 @@ describe("Briefs store", () => {
expect(regenerated.hidden).toBe(true);
expect(regenerated.pinned).toBe(false);
});
it("loads card snapshots and capped sources in batches", async () => {
const { db, queries } = createListFakeDb();
const store = createBriefsStore(db);
const cards = await store.listCards({ companyId, userId });
expect(cards).toHaveLength(1);
expect(cards[0]?.sources).toHaveLength(2);
expect(cards[0]?.moreSourceCount).toBe(250);
expect(queries.filter((query) => query.sql.includes("FROM plugin_briefs_test.briefs_card_snapshots"))).toHaveLength(1);
const sourceQuery = queries.find((query) => query.sql.includes("WITH ranked_sources AS"));
expect(sourceQuery?.params[2]).toEqual([cardId]);
expect(sourceQuery?.params[3]).toBe(200);
expect(sourceQuery?.sql).toContain("row_number() OVER (PARTITION BY card_id");
});
});
+3
View File
@@ -2,6 +2,9 @@ packages:
- packages/*
- packages/adapters/*
- packages/plugins/*
# Keep the new Briefs plugin installable as a local first-party package while
# this PR intentionally avoids root pnpm-lock.yaml churn.
- "!packages/plugins/plugin-briefs"
# Keep sandbox-provider plugins installable as standalone packages without
# forcing root pnpm-lock.yaml churn for their third-party deps.
- "!packages/plugins/sandbox-providers/**"