test(e2e): restore a green e2e suite on main — fixture rot + assertions updated for merged UI changes

[goyamegh]

Why

The e2e-tests CI job has been red on main itself — latest main run: 5 failed / 3 flaky; on current HEAD (ac025b03d) 7 failed / 5 flaky — so every open PR inherits the same failures regardless of what it touches, and e2e-gate can't be trusted as a merge gate.

Each failing spec was reproduced locally against a fresh file-storage server (exactly CI's setup: CI=1, no OPENSEARCH_*), root-caused, and repaired on the right seam. No test was deleted or skipped; no assertion was loosened without the reason in the commit. Two of the failures turned out to be real product regressions.

Per-spec root cause

Spec	Class	Cause → fix
judge-tab-whyfix	Product regression	#442's merge conflict resolution took the pre-#454 MatcherResultsPanel.tsx wholesale, silently reverting #454's Why/Fix judge-row redesign on main (duplicated reasoning + fabricated "score 0%" were back; #454's unit suite replaced; judgeReasoningParse.ts orphaned). Restored the #454 panel and layered #442's notReached rows on top. Unit suite = #454 + #442 + 1 new case.
skills (2 tests)	Product bug (surfaced locally; CI's runner has no user-scope skills so it never saw it)	GET /api/skills/discover lists ~/.claude/skills/* with a ~/… path, but resolveSkillPath() resolved it against cwd → "Directory does not exist" and Run Evaluation stayed disabled. ~, ~/, ~\ now expand to home (~backup/ deliberately not). Unit + integration tests (red against the unfixed server).
benchmark-run-row-link (2)	Stale assertion	#460 replaced the run-name <h2> with InlineRenameField → assert run-inspector-rename-text. Fixture's nonexistent agent is intentional (deterministic "failed before linking" shape) and unchanged.
evalruns-no-left-status-icon	Stale assertion	#460 added the rename pencil <svg> in the name cell → svg:not(.lucide-pencil) count 0 (the #430 status-icon guard still bites).
benchmark-version-link-panel	Stale assertion	#447 replaced the split test-case panel with Cases | Runs tabs. Asserts the Cases tab and (per codex review) the still-routed version-aware page at /benchmarks/:id/runs — the only surface reading versions[current].testCaseIds, where the original bug rendered.
dashboard	Stale assertion + race	#399 added the ready-to-run Overview state (what CI's shared server is almost always in); the loading skeleton also renders dashboard-title (branch-detection race); the tagline string never matched the copy #259 shipped. Waits for a settled marker, handles all three states.
test-case-runs	Flake / vacuous	Clicked "the first card containing 'runs'" on whatever data parallel suites left. Now seeds its own case (+ two reports for the run-card tests) and asserts the real contract — no expect(true) / empty-state fallbacks.
sidebar-hover-flyout	Flake	Hovered mid-collapse-transition → settled rail slid out from under the cursor → synthesized mouseleave (178→80→64px in the CI call log). Waits for the settled width first.
evaluation-runs / evaluation-runner stats	Order-dependent strict-mode	text=Failed also matched the lowercase failed status badge when the newest run was a failed one → getByText(…, { exact: true }).
comparison-search-popover-anchor	Flake	Single-frame boundingBox() during the zoom-in animation (12.35px > 12px) → polls until settled.
comparison-benchmark-free	Stale (#469)	#469 made the run name a second link to the report path → strict-mode; target open-run-<id> testid and assert both hrefs.
run-inspector-no-autoselect / run-report-insights (+3 latent siblings)	Flake (deterministic ordering)	Module-level const reportIds = [] only ever grew; when a worker re-entered the file under fullyParallel, beforeAll re-ran but the array kept the previous invocation's (already-deleted) ids → seeded benchmark pointed at 404 reports → rows rendered PENDING. Reset in beforeAll.
comparison-hover-prompt-preview	Flake (#470)	Link's bottom edge sat 3px past the scroll container in a 720px viewport; focus() auto-scrolled 3px and Radix Tooltip dismisses on any ancestor scroll (instrumented: tooltip.open → scroll(scrollTop=3) → data-state="closed"). Runs in a 1000px viewport.

Verification

Full suite, CI-equivalent (CI=1, 2 workers, fresh file-storage server, no OPENSEARCH_*), two consecutive runs on the final tree:

run 1 (final tree): 541 passed, 0 failed, 0 flaky, 10 skipped (12.7m)
run 2 (final tree): 541 passed, 0 failed, 0 flaky, 10 skipped (12.6m)

Both runs: CI=1 AH_PORT=<port> PLAYWRIGHT_SKIP_WEBSERVER=1 npx playwright test --workers=2 against node server/dist/index.js on fresh file storage, same as .github/workflows/ci.yml.

Unit: MatcherResultsPanel.test.ts 36/36, skills.test.ts 26/26; integration skills.integration.test.ts 10/10 (the new ~/ case is red against the unfixed server). Unit suite otherwise untouched.

Review

codex_review (adversarial, different model family) findings and disposition:

Sev	Finding	Disposition
HIGH	test-case-runs run-card tests accepted the empty state vacuously; navigation test asserted body visible	Applied — each run-card test seeds two reports and asserts PASSED/FAILED labels, exactly one Latest on the newest, per-card scores, and the /runs/<id> navigation
MED	benchmark-version-link-panel Cases-tab assertion reads top-level testCaseIds, so it can't see the version-level regression	Applied — added the version-aware page assertion (still routed, reached from the Overview CTA)
MED	resolveSkillPath only handled ~/, not the Windows ~\ form discover would emit	Applied — regex handles both separators; unit test added
—	MatcherResultsPanel restore + notReached layering	No concrete lost behaviour found

Note: npm audit --audit-level=high reports 15 high on this branch — identical on origin/main (no dependency changes here; package-lock.json untouched).

[goyamegh]

Verification proof

Before — e2e-tests on main HEAD ac025b03d (job is the earlier main run; current-HEAD job 101202876874):

  7 failed
  5 flaky
  10 skipped
  528 passed (29.9m)

After — full suite on this branch, CI-equivalent setup (CI=1, 2 workers, fresh file-storage server started from server/dist/index.js, no OPENSEARCH_*), two consecutive runs on the final tree:

run 1:
10 skipped
541 passed (12.7m)
run 2:
10 skipped
541 passed (12.6m)

(10 skipped are the suites that self-skip without a real OpenSearch / test-endpoints server — unchanged from CI.)

Product-fix tests (red → green):

• tests/unit/components/MatcherResultsPanel.test.ts — 36/36 (the restored fix(judge): no fabricated score-0, no duplicated reasoning; judge rows answer WHY it failed / HOW to fix it #454 suite + Code-SDK & CLI: .eval.ts support, always-recorded metrics, expect.soft, version-level case links, external-cwd DX #442's not-reached cases + 1 new). Before this branch judge-tab-whyfix.spec.ts failed 3/3 retries because the Why/Fix row was gone from main.
• tests/unit/server/routes/skills.test.ts — 26/26 (3 new: ~/, ~\, and ~backup/ NOT expanded).
• tests/integration/server/routes/skills.integration.test.ts — the new ~/ case fails against the unfixed server with Directory does not exist: <cwd>/~/.claude/skills/ahtest-tilde-skill-… and passes against the fixed build.

Ordering-flake root cause (deterministic repro): npx playwright test --workers=2 retry-judgement.spec.ts run-inspector-no-autoselect.spec.ts run-report-insights.spec.ts → 2 failed before, 20/20 after. The failing page's network log showed the seeded benchmark (stamp …714341) referencing report ids from an earlier beforeAll (stamp …712829) that its afterAll had already deleted → GET /api/storage/runs/<id> 404 → rows PENDING.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e00d2d6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Path traversal vulnerability: The resolveSkillPath function in server/routes/skills.ts expands tilde-prefixed paths to the user's home directory but does not validate that the final resolved path stays within safe boundaries. An attacker controlling the inputPath can use sequences like ~/../../../etc/passwd to escape the intended skill directories and access arbitrary files on the filesystem. Since the server already has filesystem access (as noted in the code comment), this may be considered acceptable for a local dev tool, but it should be explicitly documented as a known limitation, or the function should enforce that resolved paths remain within ~/.claude/skills or the project root.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The structuredFacts function iterates over extra?.facts without validating that each element is an object before casting it to any and reading properties. If a malformed judgeExtraFields.facts array contains non-object elements (e.g., strings, numbers, null), the code will attempt to read .fact and .verdict from them, which will silently return undefined and skip the entry. While this may be intentional defensive behavior, it could mask data corruption or schema violations that should be surfaced to the user or logged. function structuredFacts(extra: Record<string, unknown> | undefined): StructuredFact[] { const raw = extra?.facts; if (!Array.isArray(raw)) return []; const out: StructuredFact[] = []; for (const f of raw) { if (!f || typeof f !== 'object') continue; const fact = (f as any).fact; const verdict = (f as any).verdict; if (typeof fact !== 'string' || !fact.trim()) continue; const kind: FactVerdictKind = verdict === 'stated' || verdict === 'partial' || verdict === 'missing' || verdict === 'contradicted' ? verdict : 'partial'; const rationale = typeof (f as any).rationale === 'string' ? (f as any).rationale : undefined; out.push({ fact, verdict: kind, ...(rationale ? { rationale } : {}) }); } return out; } Security Concern The resolveSkillPath function expands tilde paths to the user's home directory without any validation or sanitization of the input path after expansion. An attacker who can control the inputPath parameter could craft a path like ~/../../../etc/passwd which, after tilde expansion, would resolve to /home/user/../../../etc/passwd and then to /etc/passwd via path.resolve. This allows directory traversal outside the intended skill directories. The function should validate that the resolved absolute path remains within allowed directories (e.g., ~/.claude/skills or the project directory). export function resolveSkillPath(inputPath: string): string { // `~`, `~/...` and (Windows) `~\...` — discover builds the display path as // '~' + absDir.slice(home.length), so the separator after `~` is whatever // the platform produced. A `~`-prefixed *name* (e.g. `~backup/skill`) is // deliberately left alone: it is a real relative directory, not home. if (/^~(?:$|[\\/])/.test(inputPath)) { return resolve(homedir(), inputPath.slice(2)); } const cwd = process.cwd(); return resolve(cwd, inputPath); } Possible Issue The seedReports function creates two reports with hardcoded IDs that include a timestamp and random suffix. If two tests run concurrently and generate the same timestamp and random suffix (extremely unlikely but theoretically possible), the second test's seed will overwrite the first test's data, causing both tests to fail or behave unpredictably. While the probability is very low, using a more robust unique ID generation (e.g., UUID) would eliminate this race condition entirely. async function seedReports(request: APIRequestContext, testCaseId: string): Promise<string[]> { const stamp = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; const base = (id: string, passFailStatus: 'passed' | 'failed', ageMs: number, accuracy: number) => ({ id, testCaseId, testCaseVersionId: `${testCaseId}-v1`, agentId: 'demo', agentName: 'Demo Agent', modelId: 'demo-model', modelName: 'demo-model', iteration: 1, status: 'completed', passFailStatus, metricsStatus: 'ready', timestamp: new Date(Date.now() - ageMs).toISOString(), trajectory: [{ type: 'assistant', content: 'answer text' }], metrics: { accuracy, faithfulness: accuracy }, }); const ids = [`report-e2e-tcruns-pass-${stamp}`, `report-e2e-tcruns-fail-${stamp}`]; const res = await request.post('/api/storage/runs/bulk', { data: { runs: [base(ids[0], 'passed', 1_000, 90), base(ids[1], 'failed', 60_000, 20)] }, }); expect(res.ok()).toBeTruthy(); return ids; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e00d2d6

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix stale memo dependency The pFacts memo depends on sFacts.length but sFacts is itself a memoized array. When extra changes but produces the same-length array, sFacts.length stays identical and pFacts won't recompute even though the facts themselves changed. Include sFacts directly in the dependency array instead of sFacts.length. components/MatcherResultsPanel.tsx [319-324] const sFacts = useMemo(() => structuredFacts(extra), [extra]); const sCauses = useMemo(() => structuredCauses(extra), [extra]); const pFacts = useMemo( () => (sFacts.length > 0 ? [] : parseFactVerdicts(result.reasoning)), - [sFacts.length, result.reasoning] + [sFacts, result.reasoning] ); Suggestion importance[1-10]: 7 __ Why: The dependency on sFacts.length instead of sFacts itself could cause stale memoization when extra changes but produces an array of the same length. However, in practice structuredFacts() returns a new array reference on every call when extra changes, so sFacts will be a new reference and sFacts.length will trigger the recomputation. The suggestion is technically more correct but the impact is limited.	Medium
	Replace fixed delay with stable signal The loop condition !(await row.isVisible().catch(() => false)) is evaluated once per iteration, but row.isVisible() checks the DOM state before the previous loadMore.click() has finished loading new rows. The 500ms waitForTimeout may not be enough for the network fetch and render. Wait for a stable signal (e.g., row count change or loading indicator disappearance) instead of a fixed delay. tests/e2e/test-case-runs.spec.ts [72-82] const foundOnFirstPage = await row.waitFor({ state: 'visible', timeout: 3000 }).then(() => true, () => false); if (!foundOnFirstPage) { await search.fill(''); for (let attempt = 0; attempt < 10 && !(await row.isVisible().catch(() => false)); attempt++) { const loadMore = page.getByRole('button', { name: 'Load More' }); if (!(await loadMore.isVisible().catch(() => false))) break; await loadMore.click(); - await page.waitForTimeout(500); + await page.waitForLoadState('networkidle'); } await search.fill(seeded.name); } Suggestion importance[1-10]: 5 __ Why: The suggestion to use waitForLoadState('networkidle') instead of a fixed 500ms delay is a reasonable improvement for test stability. However, the current implementation with the fixed delay is bounded (10 attempts) and already includes a fallback check for row.isVisible(), so the risk of flakiness is mitigated. The improvement is moderate rather than critical.	Low
	Validate bare tilde path When inputPath is exactly , slice(2) produces an empty string and resolve(homedir(), '') returns the home directory itself, not a skills subdirectory. Verify that the caller expects this behavior or add validation to reject a bare path. server/routes/skills.ts [39-49] export function resolveSkillPath(inputPath: string): string { if (/^~(?:$|[\\/])/.test(inputPath)) { - return resolve(homedir(), inputPath.slice(2)); + const remainder = inputPath.slice(2); + if (!remainder) { + throw new Error('Skill path cannot be the home directory itself'); + } + return resolve(homedir(), remainder); } const cwd = process.cwd(); return resolve(cwd, inputPath); } Suggestion importance[1-10]: 3 __ Why: While the suggestion correctly identifies that a bare ~ would resolve to the home directory itself, this is an edge case that the caller (GET /api/skills/discover) never produces—it always appends /.claude/skills/<name>. Adding validation would harden the function but is not addressing a real bug in the current usage.	Low

---

Previous suggestions

Suggestions up to commit 0079810

Category	Suggestion	Impact
General	Memoize facts array to prevent recomputation The whyBullets useMemo depends on facts, which is derived from either sFacts or pFacts. Since facts is not itself memoized, it will be recreated on every render, causing whyBullets to recompute unnecessarily even when the underlying data hasn't changed. Memoize the facts array to prevent unnecessary recalculations of whyBullets. components/MatcherResultsPanel.tsx [325-356] +const facts: Array<{ fact: string; verdict: FactVerdictKind; note?: string }> = useMemo( + () => + sFacts.length > 0 + ? sFacts.map(f => ({ fact: f.fact, verdict: f.verdict, note: f.rationale })) + : pFacts.map((f: ParsedFactVerdict) => ({ fact: f.fact, verdict: f.verdict, note: f.note })), + [sFacts, pFacts] +); + const whyBullets: Array<{ head: string; sub?: string }> = useMemo(() => { if (sCauses.length > 0) return sCauses.map(c => ({ head: c.cause, sub: c.detail })); const out: Array<{ head: string; sub?: string }> = []; if (mismatch) { out.push({ head: 'Wrong source cited', sub: `Answer built from ${shortId(mismatch.cited)} instead of the expected ${shortId(mismatch.expected)}.`, }); } const bad = facts.filter(f => f.verdict !== 'stated'); if (bad.length > 0) { const counts = ['partial', 'missing', 'contradicted'] .map(k => [k, bad.filter(f => f.verdict === k).length] as const) .filter(([, n]) => n > 0) .map(([k, n]) => `${n} ${k}`) .join(' · '); out.push({ head: `Required facts not fully stated (${counts})`, sub: 'See the fact-by-fact checklist below.', }); } return out; }, [sCauses, mismatch, facts]); Suggestion importance[1-10]: 4 __ Why: Valid optimization suggestion. The facts array is currently recreated on every render, causing whyBullets to recompute unnecessarily. Memoizing facts would improve performance, though the impact is moderate since this only affects judge rows that are expanded.	Low
	Fix useMemo dependency chain The pFacts useMemo depends on sFacts.length, but sFacts is itself a memoized value that can change when extra changes. This creates a subtle dependency chain where pFacts may not recompute when sFacts changes from empty to non-empty if only the length happens to match. Depend directly on sFacts instead of sFacts.length to ensure proper reactivity. components/MatcherResultsPanel.tsx [319-324] const sFacts = useMemo(() => structuredFacts(extra), [extra]); const sCauses = useMemo(() => structuredCauses(extra), [extra]); const pFacts = useMemo( () => (sFacts.length > 0 ? [] : parseFactVerdicts(result.reasoning)), - [sFacts.length, result.reasoning] + [sFacts, result.reasoning] ); Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies a dependency issue, but the impact is minimal. Depending on sFacts instead of sFacts.length is more semantically correct, though in practice both will trigger recomputation when sFacts changes since sFacts.length changes when sFacts changes.	Low

Suggestions up to commit cb24525

Category	Suggestion	Impact
General	Validate object properties before access The function uses unsafe type assertions (as any) when accessing properties from untyped objects. If f is an object but lacks the expected properties, accessing f.fact or f.verdict could return undefined, leading to unexpected behavior. Add explicit type guards or validate property existence before accessing them to prevent runtime errors. components/MatcherResultsPanel.tsx [245-262] function structuredFacts(extra: Record<string, unknown> | undefined): StructuredFact[] { const raw = extra?.facts; if (!Array.isArray(raw)) return []; const out: StructuredFact[] = []; for (const f of raw) { if (!f || typeof f !== 'object') continue; - const fact = (f as any).fact; - const verdict = (f as any).verdict; + const obj = f as Record<string, unknown>; + const fact = obj.fact; + const verdict = obj.verdict; if (typeof fact !== 'string' || !fact.trim()) continue; const kind: FactVerdictKind = verdict === 'stated' || verdict === 'partial' || verdict === 'missing' || verdict === 'contradicted' ? verdict : 'partial'; - const rationale = typeof (f as any).rationale === 'string' ? (f as any).rationale : undefined; + const rationale = typeof obj.rationale === 'string' ? obj.rationale : undefined; out.push({ fact, verdict: kind, ...(rationale ? { rationale } : {}) }); } return out; } Suggestion importance[1-10]: 3 __ Why: The suggestion addresses type safety by replacing as any with as Record<string, unknown>, but the actual improvement is minimal. The existing code already validates property types (typeof fact !== 'string', typeof verdict === 'stated'...) before use, so runtime errors are already prevented. The change is a minor style improvement rather than a bug fix.	Low
	Replace unsafe type assertions Similar to structuredFacts, this function uses unsafe as any casts when accessing properties from untyped objects. Replace the type assertions with a typed intermediate variable to safely access properties and prevent potential runtime errors from missing or unexpected properties. components/MatcherResultsPanel.tsx [265-280] function structuredCauses(extra: Record<string, unknown> | undefined): Array<{ cause: string; detail?: string }> { const raw = extra?.failure_causes; if (!Array.isArray(raw)) return []; const out: Array<{ cause: string; detail?: string }> = []; for (const c of raw) { if (typeof c === 'string' && c.trim()) { out.push({ cause: c.trim() }); - } else if (c && typeof c === 'object' && typeof (c as any).cause === 'string') { - out.push({ - cause: (c as any).cause, - ...(typeof (c as any).detail === 'string' ? { detail: (c as any).detail } : {}), - }); + } else if (c && typeof c === 'object') { + const obj = c as Record<string, unknown>; + if (typeof obj.cause === 'string') { + out.push({ + cause: obj.cause, + ...(typeof obj.detail === 'string' ? { detail: obj.detail } : {}), + }); + } } } return out; } Suggestion importance[1-10]: 3 __ Why: Similar to suggestion 1, this replaces as any with as Record<string, unknown>, which is a minor type safety improvement. However, the existing code already validates typeof obj.cause === 'string' before accessing properties, so the risk of runtime errors is already mitigated. The improvement is marginal.	Low

[github-actions]

Persistent review updated to latest commit 0079810

[codecov]

Codecov Report

❌ Patch coverage is 82.30088% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.16%. Comparing base (ac025b0) to head (e00d2d6).

Files with missing lines	Patch %	Lines
components/MatcherResultsPanel.tsx	81.81%	2 Missing and 18 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   70.09%   70.16%   +0.06%     
==========================================
  Files         413      413              
  Lines       34033    34117      +84     
  Branches    10128    10166      +38     
==========================================
+ Hits        23857    23937      +80     
+ Misses       7915     7906       -9     
- Partials     2261     2274      +13     

Flag	Coverage Δ
e2e	48.14% <49.53%> (+0.04%)	⬆️
integration	45.48% <ø> (ø)
unit	77.40% <80.53%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
server/routes/skills.ts	84.23% <100.00%> (+0.21%)	⬆️
components/MatcherResultsPanel.tsx	82.75% <81.81%> (+20.25%)	⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[goyamegh]

CI on this PR head (007981021) — e2e-tests job 101227383168:

10 skipped
541 passed (26.6m)

0 failed, 0 flaky → e2e-gate ✅. Every other check is green too (link-check was also red on main for two CHANGELOG links to files removed by #443/#460 — de-linked here).

[github-actions]

Persistent review updated to latest commit e00d2d6