Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 17 additions & 3 deletions tests_end_to_end/coverage/taxonomy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -506,8 +506,9 @@ areas:
- datasets/dataset-version-counters.spec.ts
- datasets/dataset-version-repeated-item-id.spec.ts
- datasets/dataset-version-concurrent-writes.spec.ts
- datasets/dataset-list-summary-columns.spec.ts
capabilities:
list-datasets: { covered: true, tier: t1-smoke }
list-datasets: { covered: true, tier: t1-smoke, note: "t1 asserts the row renders; t2 adds the computed summary each row carries — dataset_items_count, experiment_count, optimization_count and latest_version asserted per row over four datasets with pairwise-distinct shapes (one empty), list vs detail vs the dataset's own items/versions endpoints, unchanged across two pages; and on screen, Item count plus the two recency columns populated on exactly the right rows" }
create-dataset-ui: { covered: true, tier: t1-smoke }
create-dataset-sdk: { covered: true, tier: t1-smoke }
view-items: { covered: true, tier: t1-smoke }
Expand Down Expand Up @@ -542,8 +543,15 @@ areas:
specs:
- test-suites/test-suites-smoke.spec.ts
- test-suites/test-suite-delete.spec.ts
- test-suites/test-suite-insert-dedup-listed-suite.spec.ts
capabilities:
list-suites: { covered: true, tier: t1-smoke }
# The t2 spec drives `get_test_suites()`, the listing factory nothing else
# in the estate reaches, and asserts that a suite reached through it
# deduplicates an insert of an item it already holds. That behaviour has
# no key of its own here — this one names the listing, which is the path
# the spec exercises, not the dedup it asserts. Worth a dedicated
# capability if anyone extends this area.
list-suites: { covered: true, tier: t1-smoke, note: "t2 additionally reaches a suite through get_test_suites() and asserts insert() deduplicates an item the suite already holds, while a genuinely new item still lands" }
create-suite-ui: { covered: true, tier: t1-smoke }
create-suite-sdk: { covered: true, tier: t1-smoke }
view-suite-items: { covered: true, tier: t1-smoke }
Expand Down Expand Up @@ -645,6 +653,7 @@ areas:
- online-evaluation/online-evaluation-sampling-rate.spec.ts
- online-evaluation/online-evaluation-python-metric-errors.spec.ts
- online-evaluation/online-evaluation-non-object-sections.spec.ts
- online-evaluation/online-evaluation-edit-rule-preserves-model-parameters.spec.ts
capabilities:
create-llm-judge-rule: { covered: true, tier: t1-smoke }
llm-judge-scores: { covered: true, tier: t1-smoke, note: "bimodal safe/unsafe" }
Expand All @@ -656,7 +665,12 @@ areas:
rule-filters: { covered: false }
sampling-rate: { covered: true, tier: t2-cuj, note: "50% rule vs 100% control over one 30-trace batch, binomial band 15-85%; plus a 0%-rate rule at trigger_scope=both, which must skip every SDK trace and still score experiment/playground/optimization ones" }
clone-rule: { covered: false }
edit-rule: { covered: false }
# Scoped on purpose. The spec opens a rule's edit dialog and saves it with
# no field changed, asserting the model's custom_parameters survive the
# round-trip — the payload the form serializes AND the persisted blob.
# It does not drive any individual editor control, so editing a rule's
# prompt, model, variables or scope is still uncovered.
edit-rule: { covered: true, tier: t2-cuj, note: "unedited save only: LLM-judge rule seeded over REST with model.custom_parameters, saved unchanged through the edit dialog, PATCH body and persisted blob both asserted to carry the thinking block and an unrelated free-form key. Editing individual fields is not covered" }
enable-disable-rule: { covered: true, tier: t2-cuj, note: "edit-dialog switch; control rule proves scoring stopped, then resumed" }
delete-rule: { covered: true, tier: t2-cuj, note: "row kebab delete; control rule proves scoring stopped" }
# Deliberately still false. online-evaluation-python-metric-errors.spec.ts
Expand Down
265 changes: 265 additions & 0 deletions tests_end_to_end/e2e/core/backend/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,42 @@ export interface RawApiResult {
/** One row of the dataset's Version history tab. */
export interface DatasetVersionRef {
versionName: string;
/**
* The version's content hash. Nullable because the API's own shape makes it
* optional — a caller that needs it must assert it is there rather than
* compare an absent value to another absent value and call that agreement.
*/
versionHash: string | null;
itemsTotal: number;
itemsAdded: number;
itemsModified: number;
itemsDeleted: number;
isLatest: boolean;
}

/**
* The computed summary the datasets list attaches to each row.
*
* None of these values is stored on the dataset: the backend derives each from
* a separate lookup and zips them onto the row. That makes the failure
* mode a mis-attribution — a real number belonging to a different dataset —
* which renders as a perfectly plausible row rather than an error, so nothing
* here is defaulted. `null` appears only where the API genuinely answers null
* (a dataset with no version, no experiment or no optimization yet); an absent
* count throws in the mapper instead, because a `?? 0` would silently produce
* exactly the value an empty-dataset assertion expects.
*/
export interface DatasetSummaryRef {
id: string;
name: string;
datasetItemsCount: number;
experimentCount: number;
optimizationCount: number;
latestVersionHash: string | null;
mostRecentExperimentAt: string | null;
mostRecentOptimizationAt: string | null;
}

/** The windowed stats one row of the Projects table renders. */
export interface ProjectStatsRef {
projectId: string;
Expand Down Expand Up @@ -159,6 +188,21 @@ export interface AutomationRuleDetail {
triggerScope: string;
}

/**
* The `code.model` block of an `llm_as_judge` rule, exactly as REST stores it.
*
* `customParameters` is the free-form slot the provider config is carried in
* (`thinking`, and anything else a caller persisted alongside it). `null` is
* the API's own answer for "nothing set" and is kept distinct from `{}` here
* on purpose: a serializer that collapses one into the other is precisely what
* this shape is read back to catch.
*/
export interface LlmJudgeModelRef {
name: string;
temperature: number | null;
customParameters: Record<string, unknown> | null;
}

/** One line of a rule's user-facing log stream. */
export interface AutomationRuleLogRef {
level: string;
Expand Down Expand Up @@ -281,6 +325,48 @@ export interface OptimizationRef {
/** Backend discriminator for Dataset vs Test Suite (shared DB table). */
const TEST_SUITE_TYPE = 'evaluation_suite';

/**
* Map one dataset row — from the list or from a detail read, which answer the
* same shape — onto `DatasetSummaryRef`.
*
* Every count is required. `?? 0` is deliberately absent: the empty-dataset
* case is asserted to report zeros, so defaulting a missing count to 0 would
* make that assertion pass on a response that carried no count at all.
*/
function toDatasetSummary(row: unknown): DatasetSummaryRef {
const d = row as {
id?: string;
name?: string;
dataset_items_count?: number;
experiment_count?: number;
optimization_count?: number;
most_recent_experiment_at?: string | null;
most_recent_optimization_at?: string | null;
latest_version?: { version_hash?: string } | null;
};
const requireCount = (value: number | undefined, field: string): number => {
if (typeof value !== 'number' || Number.isNaN(value)) {
throw new Error(
`toDatasetSummary: dataset '${d.name ?? d.id}' returned no numeric ${field}`,
);
}
return value;
};
if (typeof d.id !== 'string' || typeof d.name !== 'string') {
throw new Error('toDatasetSummary: dataset row carried no id/name');
}
return {
id: d.id,
name: d.name,
datasetItemsCount: requireCount(d.dataset_items_count, 'dataset_items_count'),
experimentCount: requireCount(d.experiment_count, 'experiment_count'),
optimizationCount: requireCount(d.optimization_count, 'optimization_count'),
latestVersionHash: d.latest_version?.version_hash ?? null,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Malformed version data passes as empty

latest_version?.version_hash ?? null converts a missing version_hash into null, so an incomplete latest_version is indistinguishable from no latest version and can bypass summary assertions — should we validate version_hash when latest_version is present and reserve null for an absent or explicitly null API value?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
tests_end_to_end/e2e/core/backend/client.ts around lines 364-364, update the
`toDatasetSummary` mapper so `latest_version?.version_hash ?? null` does not treat a
missing hash on a present `latest_version` as valid. Return `null` only when
`latest_version` is absent/null or its hash is explicitly null; otherwise validate that
`version_hash` is a string and throw a descriptive error for missing or invalid values,
consistent with the mapper’s strict count validation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reachable from the consumer. The spec asserts latest[0].versionHash is not.toBeNull() and then compares the listed latestVersionHash against the dataset's actual latest version, so a latest_version present with a missing hash fails either way. The mapper's strict count validation exists because the empty-dataset case asserts zeros — a missing count would otherwise default into a passing assertion; the hash has no equivalent default to hide behind, so ?? null is not doing the same work there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying—the hash is already covered by the explicit assertions, unlike the count defaulting case. I’ll save this to memory once the PR is merged.

mostRecentExperimentAt: d.most_recent_experiment_at ?? null,
mostRecentOptimizationAt: d.most_recent_optimization_at ?? null,
};
}

/** One clause of the `sorting` query param the grids serialise. */
export interface BackendSort {
field: string;
Expand Down Expand Up @@ -783,6 +869,65 @@ export function makeBackendClient(apiKey: string | null = null, workspaceName: s
});
},

/**
* One page of `GET /v1/private/datasets` with the computed summary each row
* carries — the exact read the Datasets list issues.
*
* Through `rawFetch` rather than the pinned SDK because the SDK's dataset
* shape does not surface `latest_version`, and the summary is the whole
* point of the read.
*
* `page`/`size` are exposed so a caller can prove the summary survives
* pagination: the counts are zipped onto the page's rows, so a rewrite that
* zips them in the wrong order is visible only when a page holds a subset.
*/
async listDatasetSummaries(args: {
projectId: string;
page?: number;
size?: number;
}): Promise<{ total: number; rows: DatasetSummaryRef[] }> {
const query = new URLSearchParams({
project_id: args.projectId,
page: String(args.page ?? 1),
size: String(args.size ?? 100),
});
const { status, message, json } = await rawFetch('GET', '/v1/private/datasets', {
query,
Comment on lines +894 to +895

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raw helpers ignore explicit workspace

rawFetch hard-codes Comet-Workspace to env.workspace instead of the selected workspaceName ?? env.workspace, so listDatasetSummaries and the other new helpers target the environment workspace and fail for clients configured with another workspace — should we capture the normalized workspace in makeBackendClient and reuse it for raw headers?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
tests_end_to_end/e2e/core/backend/client.ts around lines 894-895, update
`listDatasetSummaries` and the other new rawFetch-based dataset and automation helpers
so requests use the explicitly selected `workspaceName` when provided, falling back to
`env.workspace` otherwise. Capture this normalized workspace once inside
`makeBackendClient` and pass it through to the raw request header construction instead
of relying on the hard-coded `env.workspace`, ensuring all reads and creates target the
same workspace as the typed client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real, but pre-existing and not reachable from this PR. rawFetch and workspaceHeaders are untouched here — the same hardcoded env.workspace is already on main in three places — and the new helpers are reached through base.fixture.ts's makeBackendClient(envConfig.apiKey), which passes no workspace, so the fallback and the explicit value coincide. The callers that do pass an explicit workspace are the workspace-role paths, which never touch these helpers. Worth fixing across the client rather than in these helpers alone; tracking it with the fixture item.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying the reachability and broader tracking plan. I’ll save this context to memory once the PR is merged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked as #8130 (item 2).

});
if (status !== 200) {
throw new Error(
`listDatasetSummaries: project ${args.projectId} answered ${status}: ${message}`,
);
}
const page = json as { total?: number; content?: unknown[] };
if (typeof page.total !== 'number') {
throw new Error('listDatasetSummaries: response carried no `total`');
}
return {
total: page.total,
rows: (page.content ?? []).map(toDatasetSummary),
};
},

/**
* The same computed summary as it appears on one dataset's own detail read.
*
* The list and the detail are different code paths over the same four
* lookups, so disagreeing is itself the bug: whichever of the two is wrong,
* a user reading a number off the list and then opening the dataset sees
* two different truths.
*/
async getDatasetSummary(datasetId: string): Promise<DatasetSummaryRef> {
const { status, message, json } = await rawFetch(
'GET',
`/v1/private/datasets/${datasetId}`,
);
if (status !== 200) {
throw new Error(`getDatasetSummary: ${datasetId} answered ${status}: ${message}`);
}
return toDatasetSummary(json);
},

async getDatasetItems(datasetId: string): Promise<DatasetItemRef[]> {
const page = await opik.api.datasets.getDatasetItems(datasetId);
const content = page.content ?? [];
Expand All @@ -801,6 +946,7 @@ export function makeBackendClient(apiKey: string | null = null, workspaceName: s
const content = page.content ?? [];
return content.map((v) => ({
versionName: String(v.versionName ?? ''),
versionHash: v.versionHash ?? null,
itemsTotal: Number(v.itemsTotal ?? 0),
itemsAdded: Number(v.itemsAdded ?? 0),
itemsModified: Number(v.itemsModified ?? 0),
Expand Down Expand Up @@ -1480,6 +1626,125 @@ export function makeBackendClient(apiKey: string | null = null, workspaceName: s
return id;
},

/**
* Create an `llm_as_judge` online-evaluation rule and return its id.
*
* Separate from `createAutomationRule` (which builds the
* `user_defined_metric_python` shape) because the two rule types carry
* completely different `code` blocks, and folding both into one signature
* would make every field optional — so a caller could build a rule the
* backend rejects and only find out at runtime.
*
* `customParameters` is passed through verbatim, including `null`: the
* whole point of a spec that round-trips this block is that the value the
* caller chose is the value that comes back, so nothing is defaulted here.
*
* Goes through `rawFetch` for the same two reasons `createAutomationRule`
* does: creation answers 201 with an empty body, so the id exists only in
* the `Location` header, and the id is parsed from there rather than
* recovered by a name lookup that could pick up an earlier run's leftovers.
*/
async createLlmJudgeAutomationRule(args: {
projectId: string;
name: string;
/** Fraction in [0, 1], the backend's own units — not the dialog's percentage. */
samplingRate: number;
/** Provider model id as the picker stores it, e.g. `claude-haiku-4-5-20251001`. */
modelName: string;
temperature: number;
customParameters: Record<string, unknown> | null;
messages: Array<{ role: string; content: string }>;
/** Judge-prompt variable name -> extraction path (e.g. `output.answer`). */
variables: Record<string, string>;
schema: Array<{ name: string; type: string; description: string }>;
enabled?: boolean;
}): Promise<string> {
const { status, message, location } = await rawFetch(
'POST',
'/v1/private/automations/evaluators/',
{
body: {
type: 'llm_as_judge',
action: 'evaluator',
name: args.name,
project_ids: [args.projectId],
sampling_rate: args.samplingRate,
enabled: args.enabled ?? true,
code: {
model: {
name: args.modelName,
temperature: args.temperature,
custom_parameters: args.customParameters,
},
messages: args.messages.map((m) => ({
role: m.role,
content: m.content,
})),
variables: args.variables,
schema: args.schema,
},
},
},
);
if (status !== 201) {
throw new Error(
`createLlmJudgeAutomationRule: expected 201 for '${args.name}', got ${status}: ${message}`,
);
}
const id = location?.split('/').filter(Boolean).pop();
if (!id) {
throw new Error(
`createLlmJudgeAutomationRule: 201 for '${args.name}' carried no usable Location ` +
`header (got '${location}') — cannot address the rule.`,
);
}
return id;
},

/**
* The `code.model` block of an `llm_as_judge` rule.
*
* Throws rather than returning a partial shape when the rule is not an
* llm-judge or carries no model: a caller reading this is asserting on what
* the model block holds, and an `undefined` threaded into that assertion
* would read as "the value changed" when the truth is "the rule is not the
* one you think". `custom_parameters` is the one field allowed to be
* absent, and it is normalised to `null` — the API's own "nothing set".
*/
async getLlmJudgeModel(ruleId: string): Promise<LlmJudgeModelRef> {
const { status, message, json } = await rawFetch(
'GET',
`/v1/private/automations/evaluators/${ruleId}`,
);
if (status !== 200) {
throw new Error(`getLlmJudgeModel: ${ruleId} answered ${status}: ${message}`);
}
const rule = json as {
type?: string;
code?: {
model?: {
name?: string;
temperature?: number;
custom_parameters?: Record<string, unknown> | null;
};
};
};
if (rule.type !== 'llm_as_judge') {
throw new Error(
`getLlmJudgeModel: ${ruleId} is type '${rule.type}', not 'llm_as_judge'`,
);
}
const model = rule.code?.model;
if (!model || typeof model.name !== 'string') {
throw new Error(`getLlmJudgeModel: ${ruleId} returned no code.model.name`);
}
return {
name: model.name,
temperature: typeof model.temperature === 'number' ? model.temperature : null,
customParameters: model.custom_parameters ?? null,
};
},

/** One rule by id, including the `triggerScope` the pinned SDK cannot see. */
async getAutomationRule(ruleId: string): Promise<AutomationRuleDetail> {
const { status, message, json } = await rawFetch(
Expand Down
2 changes: 2 additions & 0 deletions tests_end_to_end/e2e/core/backend/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ export {
type AutomationRuleRef,
type AutomationRuleDetail,
type AutomationRuleLogRef,
type LlmJudgeModelRef,
type DatasetSummaryRef,
type TraceJsonSection,
type AnnotationQueueDetail,
type AnnotationQueueReviewerRef,
Expand Down
9 changes: 9 additions & 0 deletions tests_end_to_end/e2e/core/sdk/python-sdk-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,15 @@ export interface PythonSdkClient {
description?: string;
}>;
workspace?: string;
/**
* Which client factory the suite being inserted into is obtained from.
* `get_or_create` (the bridge's default) is what every other caller wants;
* `list` reaches the suite through `get_test_suites()`. The two build a
* suite object with different local content-hash state, and that state is
* what decides whether an insert of an item the suite already holds is
* deduplicated — so a spec covering dedup has to name the path it means.
*/
resolve_via?: 'get_or_create' | 'list';
}): Promise<{ suite_id: string; inserted: number }>;
runTestSuite(args: {
suite_name: string;
Expand Down
Loading