Repository navigation
Add catalog-reuse evaluation cards #6333
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,16 @@ const MODELS = (process.env.EVAL_MODELS ?? 'Claude Sonnet 4.6') | |
| .map((m) => m.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| // Skill sections behind a feature flag (`<!-- feature:<name> -->`) are left | ||
| // out of the prompt unless the room enabled them. Each name here is enabled | ||
| // in the room with `boxel-debug:feature:enable:<name>` before the first | ||
| // prompt, in addition to the evaluation's own `skillFeatures`, so a run can | ||
| // measure the assistant with a feature on that the evaluation leaves off. | ||
| const SKILL_FEATURES = (process.env.EVAL_SKILL_FEATURES ?? '') | ||
| .split(',') | ||
| .map((f) => f.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| function slugify(s: string) { | ||
| return s | ||
| .toLowerCase() | ||
|
|
@@ -463,12 +473,16 @@ async function readActivityOnce(page: Page): Promise<Activity> { | |
| } | ||
| // A loop: the same tool call, with the same arguments, three times. The | ||
| // bot's own checkCorrectness is left out: it repeats with the same | ||
| // arguments on every repair round by design. So is a call rejected as | ||
| // invalid: it never ran, its pill shows no arguments, so several of them | ||
| // in one turn look identical, and the failed-turn count above already | ||
| // covers them. | ||
| let toolElements = Array.from( | ||
| document.querySelectorAll<HTMLElement>('[data-tool-name]'), | ||
| ); | ||
| let calls = toolElements | ||
| .filter((el) => el.dataset.toolName !== 'checkCorrectness') | ||
| .filter((el) => !el.querySelector('[data-test-apply-state="invalid"]')) | ||
| .map( | ||
| (el) => | ||
| `${el.dataset.toolName}|${el.dataset.toolArgumentsLength}|${( | ||
|
|
@@ -529,18 +543,57 @@ async function stopGeneration(page: Page) { | |
| // expensive runs are not stopped; they are graded afterwards. | ||
| // `botMessagesBefore` is how many finished bot turns the room already had | ||
| // when the prompt went out; a follow-up prompt waits for a new one. | ||
| // Some tools open a room of their own and move the assistant panel to it: | ||
| // `listing-remix`, for one, starts a "Remixing …" session. The evaluation's | ||
| // conversation carries on in its own room, but the panel no longer shows it, | ||
| // so everything read from the page would describe the wrong room. When the | ||
| // panel is on another room, reopen the evaluation's room from past sessions. | ||
| // Returns true when it switched back. It runs on every poll, so reading the | ||
| // current room never waits, and a switch that fails (the past-sessions button | ||
| // disabled, the host reloading) returns false for the next poll to retry | ||
| // rather than failing the run. | ||
| async function returnToRoom(page: Page, roomId: string): Promise<boolean> { | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] Efficiency: Generated by Claude Code |
||
| let current = await page | ||
| .evaluate( | ||
| () => | ||
| document | ||
| .querySelector('[data-test-room]') | ||
| ?.getAttribute('data-test-room') ?? null, | ||
| ) | ||
| .catch(() => null); | ||
| if (!current || current === roomId) { | ||
| return false; | ||
| } | ||
| try { | ||
| await page.locator('[data-test-past-sessions-button]').click(); | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] returnToRoom can block the poll loop and read the wrong room. The clicks rely on the 30s Generated by Claude Code |
||
| await page.locator(`[data-test-enter-room="${roomId}"]`).click(); | ||
| await page | ||
| .locator(`[data-test-room="${roomId}"]`) | ||
| .waitFor({ timeout: 60_000 }); | ||
| await page.locator('[data-test-room-settled]').waitFor({ timeout: 60_000 }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| async function waitForIdle( | ||
| page: Page, | ||
| deadline: number, | ||
| botMessagesBefore = 0, | ||
| onActivity?: (activity: Activity) => void, | ||
| roomId?: string, | ||
| roomReturns: { count: number } = { count: 0 }, | ||
| ): Promise<{ stoppedBy: RunResult['stoppedBy']; irregularities: string[] }> { | ||
| let idleSince: number | undefined; | ||
| let applyingSince: number | undefined; | ||
| let flatSince: number | undefined; | ||
| let lastTextLength = -1; | ||
| let startedAt = Date.now(); | ||
| for (;;) { | ||
| if (roomId && (await returnToRoom(page, roomId))) { | ||
| roomReturns.count++; | ||
| } | ||
| let now = Date.now(); | ||
| let activity = await readActivity(page); | ||
| onActivity?.(activity); | ||
|
|
@@ -745,8 +798,11 @@ function classify( | |
| ); | ||
| return { verdict: 'host-failure', reasons }; | ||
| } | ||
| let writes = analysis.realmCodeWrites + analysis.toolWrites; | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] Altitude: the pass gate now depends on a hand-maintained tool-name allowlist. Generated by Claude Code |
||
| if (writes === 0) { | ||
| reasons.push( | ||
| 'no file was written (no applied run-realm-code or other writing tool call)', | ||
| ); | ||
| } | ||
| if (!cardId) { | ||
| reasons.push(...cardReasons); | ||
|
|
@@ -761,7 +817,7 @@ function classify( | |
| if ( | ||
| cardId && | ||
| cardReasons.length === 0 && | ||
| writes > 0 && | ||
| promptsSent === promptsTotal | ||
| ) { | ||
| return { verdict: 'pass', reasons }; | ||
|
|
@@ -813,6 +869,8 @@ async function runModel( | |
| initialCards: [], | ||
| initialFiles: [], | ||
| skillsUsed: [], | ||
| skillFeatures: [], | ||
| roomReturns: 0, | ||
| username, | ||
| stoppedBy: 'error', | ||
| irregularities: [], | ||
|
|
@@ -871,14 +929,46 @@ async function runModel( | |
| await ensureActMode(page, requestedModel); | ||
| setStep('confirm the tab is inside the new workspace'); | ||
| await ensureInsideWorkspace(page, result.realmUrl, result.initialCards); | ||
| // The evaluation's own features, plus any EVAL_SKILL_FEATURES adds. | ||
| let skillFeatures = [ | ||
| ...new Set([...(evaluation?.skillFeatures ?? []), ...SKILL_FEATURES]), | ||
|
Comment on lines
+933
to
+934
Contributor
There was a problem hiding this comment. [Claude Code 🤖] The PR describes Follow-up, not this PR. Non-blocking. |
||
| ]; | ||
| for (let [index, feature] of skillFeatures.entries()) { | ||
| setStep(`enable skill feature ${feature}`); | ||
| await sendPrompt( | ||
| page, | ||
| result.roomId, | ||
| `boxel-debug:feature:enable:${feature}`, | ||
| index + 1, | ||
| ); | ||
| // The bot answers the command itself, without the model; its reply | ||
| // carries no usage, so it is not counted as a turn. A name the bot | ||
| // does not know gets "There is no feature named …" instead, which ends | ||
| // the run straight away rather than at the timeout. | ||
| let enabled = page.getByText(`${feature} is now enabled`).first(); | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] The enabled check can match an earlier reply. Generated by Claude Code |
||
| let unknown = page.getByText('There is no feature named').first(); | ||
| await expect(enabled.or(unknown)).toBeVisible({ timeout: NO_REPLY_MS }); | ||
| if (await unknown.isVisible()) { | ||
| throw new Error( | ||
| `The ai-bot has no skill feature named "${feature}"; check the evaluation's skillFeatures and EVAL_SKILL_FEATURES.`, | ||
| ); | ||
| } | ||
| } | ||
| result.skillFeatures = skillFeatures; | ||
| // One safety clock for the whole run, follow-ups included. | ||
| let deadline = Date.now() + MAX_MINUTES * 60_000; | ||
| let botMessagesBefore = 0; | ||
| let roomReturns = { count: 0 }; | ||
| for (let [index, prompt] of prompts.entries()) { | ||
| setStep( | ||
| index === 0 ? 'send prompt' : `send follow-up prompt ${index + 1}`, | ||
| ); | ||
| await sendPrompt( | ||
| page, | ||
| result.roomId, | ||
| prompt, | ||
| skillFeatures.length + index + 1, | ||
| ); | ||
| result.promptsSent = index + 1; | ||
| setStep( | ||
| index === 0 | ||
|
|
@@ -904,6 +994,8 @@ async function runModel( | |
| }); | ||
| } | ||
| }, | ||
| result.roomId, | ||
| roomReturns, | ||
| ); | ||
| result.stoppedBy = waited.stoppedBy; | ||
| result.irregularities = waited.irregularities; | ||
|
|
@@ -912,6 +1004,10 @@ async function runModel( | |
| } | ||
| botMessagesBefore = (await readActivity(page)).botMessages; | ||
| } | ||
| if (result.roomId && (await returnToRoom(page, result.roomId))) { | ||
| roomReturns.count++; | ||
| } | ||
| result.roomReturns = roomReturns.count; | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] Generated by Claude Code |
||
| setStep('check render'); | ||
|
|
||
| let rendered = await findRenderedCard( | ||
|
|
@@ -941,6 +1037,19 @@ async function runModel( | |
| ); | ||
| result.verdict = verdict.verdict; | ||
| result.reasons = verdict.reasons; | ||
| // An enabled feature whose skill the model never opened means the run | ||
| // did not exercise it, whatever the verdict says. This assumes each | ||
| // feature's skill lives in a skills/<feature name>/SKILL.md directory. | ||
| for (let feature of result.skillFeatures) { | ||
| let read = result.analysis.filesRead.some((url) => | ||
| url.endsWith(`/skills/${feature}/SKILL.md`), | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] The skill-read check assumes Generated by Claude Code |
||
| ); | ||
| if (!read) { | ||
| result.reasons.push( | ||
| `skill feature "${feature}" was enabled but skills/${feature}/SKILL.md was never read`, | ||
| ); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| result.error = error instanceof Error ? error.message : String(error); | ||
| result.reasons.push( | ||
|
|
@@ -1110,7 +1219,7 @@ test.afterAll(async () => { | |
| ) | ||
| .join(', ') | ||
| : ''; | ||
| let writes = a ? `${a.realmCodeWrites + a.toolWrites}` : ''; | ||
| return `| ${graded.grade} | ${r.modelId ?? r.requestedModel} | ${r.reasoningEffort ?? '–'} | ${r.verdict} | ${ | ||
| a?.turns ?? '–' | ||
| } | ${tools} | ${writes} | ${a ? `$${a.costUsd.toFixed(3)}` : '–'} | ${ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ export interface Evaluation { | |
| // Absolute URLs, as the realm serializes them. | ||
| initialCards: string[]; | ||
| initialFiles: string[]; | ||
| // Skill features to enable in the room before the first prompt. | ||
| skillFeatures: string[]; | ||
| adoptsFrom: { module: string; name: string }; | ||
| } | ||
|
|
||
|
|
@@ -87,6 +89,11 @@ export async function loadEvaluation( | |
| successCriteria: attributes.successCriteria ?? '', | ||
| initialCards: linkedIds(doc, 'initialCards', id), | ||
| initialFiles: linkedIds(doc, 'initialFiles', id), | ||
| skillFeatures: Array.isArray(attributes.skillFeatures) | ||
|
Contributor
There was a problem hiding this comment. [Claude Code 🤖] Feature names are filtered on Generated by Claude Code |
||
| ? attributes.skillFeatures.filter( | ||
| (f: unknown): f is string => typeof f === 'string' && f.trim() !== '', | ||
| ) | ||
| : [], | ||
| adoptsFrom: doc.data.meta.adoptsFrom, | ||
| }; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| { | ||
| "data": { | ||
| "type": "card", | ||
| "attributes": { | ||
| "assistantPrompt": "I want to keep track of our wine collection \u2014 each bottle with its producer, vintage and what we paid for it, and a cellar page where we can see them all. Create one example instance and show it on screen.", | ||
| "followUpPrompts": [ | ||
| "We'd like our own version of this now \u2014 something we can restyle and add fields to, that won't shift under us when someone else changes theirs." | ||
| ], | ||
| "successCriteria": "1. The assistant searched the catalog before writing any `.gts`.\n2. On the FIRST prompt the user says nothing about owning or changing anything. The assistant must NOT install or remix a Listing there \u2014 a Listing hands over a copy, and nothing was asked to be owned.\n3. On the first prompt it reaches the catalog's wine definitions by module URL \u2014 `WineCellar` and `WineBottle` under `@cardstack/catalog/4b6602-wine-cellar-card-definition/` \u2014 linking, extending or instantiating them.\n4. It does not author its own cellar or bottle card while those catalog definitions exist.\n5. After the first prompt at least one instance exists, is shown on screen, and renders with no error boundary.\n6. On the FOLLOW-UP the user asks for a version of their own to restyle and change, which is an ownership statement. The assistant now installs or remixes the Wine Cellar Listing rather than continuing to reference it.\n7. It takes that copy through the install or remix tool (`listing-install` / `listing-remix`), never by copying the catalog source by hand.\n8. It does not treat the follow-up as a reason to rebuild anything from scratch \u2014 the copy comes from the catalog.\n9. It does not install on the first prompt and then have nothing to do on the follow-up. The two turns must show a change of binding, not a single decision made early.\n10. The follow-up reply says in one line what the user now holds: their own copy, which will not receive later catalog changes.", | ||
| "cardInfo": { | ||
| "name": "Reuse: reference first, own on request", | ||
| "summary": "The first prompt asks for a wine collection with bottles and a cellar page, which the catalog's Wine Cellar Listing already answers; the second asks for a version of their own to restyle. This eval tests whether the selected models reference the catalog definitions first and install or remix the Listing only once ownership is asked for.", | ||
| "notes": "This card exists because reference-and-install look equally successful from the outside: both leave a working cellar on screen. The difference is what the user is left holding \u2014 definitions that stay wired to the catalog, or a copy that will not receive upstream fixes. Criterion 2 is the one under test; criterion 6 guards the over-correction, where the assistant refuses to copy even when asked.\n\nTiming targets do not apply: this is a two-turn build and will report ROUGH on turns and seconds as a matter of course.\n\nThe Wine Cellar Listing (`4b6602-wine-cellar-card-definition`) was chosen because it installs and remixes cleanly: every module it needs has a Spec, so the follow-up is completable. Its shape matches the case under test \u2014 a collection card (`WineCellar.bottles`, linksToMany) over item cards (`WineBottle`) \u2014 and the domain appears in no skill example, keeping eval vocabulary disjoint from the skill's own. Before changing the domain, check that the new Listing remixes into a fresh realm with no unresolved imports.", | ||
| "cardThumbnailURL": null | ||
| }, | ||
| "skillFeatures": ["catalog-reuse"] | ||
| }, | ||
| "relationships": { | ||
| "cardInfo.theme": { | ||
| "links": { | ||
| "self": null | ||
| } | ||
| } | ||
| }, | ||
| "meta": { | ||
| "adoptsFrom": { | ||
| "module": "../evaluation", | ||
| "name": "EvaluationCard" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Claude Code 🤖] This row says an evaluation of
catalog-reuseneedsEVAL_SKILL_FEATURES=catalog-reuse. But both new cards enable it with their ownskillFeatures, and the runner merges the two lists. Someone who follows this row sets a variable that does nothing. And nothing tells a new evaluation author that the card field exists: the README (where it listsinitialCards/initialFiles) and this skill's "Changing the runner" section do not nameskillFeatures. Please say that the variable is for a barepnpm eval:modelsrun, or to add a feature on top of the card's own. Then addskillFeaturesnext toinitialCards/initialFilesin both places.Regression (docs), non-blocking.