Skip to content

feat(evalboard): add a Scribe tab, reading the Autopilot suite's own blob container - #116

Merged
akshaylive merged 5 commits into
mainfrom
akshaya/scribe
Aug 14, 2026
Merged

feat(evalboard): add a Scribe tab, reading the Autopilot suite's own blob container#116
akshaylive merged 5 commits into
mainfrom
akshaya/scribe

Conversation

@akshaylive

@akshaylive akshaylive commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

What

Evalboard read a single hardcoded container (coderevaltests/runs). The Autopilot (aria/Composer) suite uploads to its own container, aria-runs, deliberately kept out of the skills nightly's namespace. This makes the container a runtime dimension so one deployment serves both, and adds a /scribe tab for the Autopilot suite.

Companion change in coder_eval_uipath (branch akshaya/nightly_autopilot) enables the upload that populates aria-runs.

How

lib/sources.ts introduces a Source (id / label / container). Every lib reader takes a trailing source: Source = DEFAULT_SOURCE, so all existing callers and URLs behave byte-identically. Non-default sources are selected by a ?src=<id> query param that every run-scoped page and API route now reads.

Source Container Surface
skills (default) runs everything else
scribe aria-runs /scribe

The constraint that shapes this PR

Run ids are only unique within a container. Both suites name runs YYYY-MM-DD_HH-MM-SS, so the same id routinely exists in two containers — and a source-blind reader does not fail, it renders a different run's data as if it were the requested one. Silent wrong answers, not an error.

Three mitigations must hold together; dropping any one reintroduces that:

  1. Each source gets its own cache dir (runsDirFor) — a sibling of the base, so it can never be mistaken for a run when listing the default source.
  2. lib/blob.ts scopes in-flight dedupe keys by container, so one source's fetch can't satisfy another's for a colliding id.
  3. Every unstable_cache key in lib/overview.ts / lib/trends.ts carries source.id.

Bugs found while threading

  • /api/refresh cleared the base cache dir rather than the requesting source's.
  • /api/file resolved artifact paths under the skills cache regardless of source.

A build failure worth calling out

lib/sources.ts is imported by client components (app/_lib/source-param.ts builds hrefs in task-grid / activation-card / refresh-button), so it must stay free of Node builtins. An early version imported node:path and failed next build with UnhandledSchemeErrorwhile passing both tsc and vitest. runsDirFor is now pure string ops, with a regression test asserting the module imports no node: specifier.

.gitignore gains runs-remote-*/; the existing runs-remote/ rule doesn't match the per-source sibling caches.

Verification

make evalboard-verify green: 40 files / 495 tests, tsc clean, next build clean.

Verified against a real aria run in aria-runs:

Check Result
/scribe lists the run 200, 1 of 1
/runs/<id>?src=scribe 200, full detail (model, tags, per-task rows)
/runs/<id> without src 404 — collision guard failing closed
Task links carry ?src=scribe yes
Per-source cache isolation separate dirs, no cross-contamination

Open question for review

Consistent with the existing convention for /path-to-ga, /watchlist and /trends, the Scribe nav link is internal-only but the /scribe route is not gated — it returns 200 in the OSS edition. Scribe has a weaker claim to that convention than the others: it reads an internal-only container, and the page names an internal pipeline and container in visible prose. Should it call notFound() when !isInternal? I left the convention intact rather than deviate unilaterally.

--- Test ---
image

🤖 Generated with Claude Code

…blob container

Evalboard read a single hardcoded container (coderevaltests/runs). The
Autopilot (aria/Composer) suite uploads to its own container, aria-runs,
deliberately kept out of the skills nightly's namespace. This makes the
container a runtime dimension so ONE deployment serves both, and adds /scribe
for the Autopilot suite.

lib/sources.ts introduces a Source (id / label / container). Every lib reader
takes a trailing `source: Source = DEFAULT_SOURCE`, so all existing callers and
URLs behave byte-identically; non-default sources are selected by a `?src=<id>`
query param that every run-scoped page and API route now reads. An absent or
unrecognised `src` resolves to the default source, so a stray param in a shared
link degrades to the skills dashboard rather than erroring.

The load-bearing constraint is that RUN IDS ARE ONLY UNIQUE WITHIN A CONTAINER:
both suites name runs `YYYY-MM-DD_HH-MM-SS`, so the same id routinely exists in
two containers, and a source-blind reader does not fail — it renders a
DIFFERENT run's data as if it were the requested one. Three mitigations have to
hold together, and dropping any one reintroduces silent wrong answers:

  - each source gets its own cache dir (runsDirFor), a sibling of the base so it
    can never be mistaken for a run when listing the default source;
  - lib/blob.ts scopes its in-flight dedupe keys by container, so one source's
    fetch cannot satisfy another's for a colliding id;
  - every unstable_cache key in lib/overview.ts and lib/trends.ts carries
    source.id.

Two latent bugs surfaced while threading the API routes: /api/refresh cleared
the base cache dir rather than the requesting source's, and /api/file resolved
artifact paths under the skills cache regardless of source.

lib/sources.ts is imported by client components (app/_lib/source-param.ts
builds hrefs in task-grid / activation-card / refresh-button), so it must stay
free of Node builtins — an early version imported node:path and failed
`next build` with UnhandledSchemeError while passing both tsc and vitest.
runsDirFor is therefore pure string ops, with a regression test asserting the
module imports no `node:` specifier.

.gitignore gains `runs-remote-*/`: the existing `runs-remote/` rule does not
match the per-source sibling cache dirs.

Verified against real data (an aria run in aria-runs): /scribe lists it,
/runs/<id>?src=scribe renders it in full, and /runs/<id> WITHOUT src returns
404 — the collision guard failing closed, since that id exists only in
aria-runs.

Known gap, matching the existing convention for /path-to-ga, /watchlist and
/trends: the Scribe nav link is internal-only but the /scribe ROUTE is not
gated, and the page names an internal pipeline and container in visible prose.
Worth deciding whether this route should call notFound() for the OSS edition.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@UiPath UiPath deleted a comment from github-actions Bot Aug 14, 2026
@uipreliga

Copy link
Copy Markdown
Collaborator

Multi-model code review

Reviewed akshaya/scribe @ 36b03b9 vs main (28 files, evalboard only). make evalboard-verify (tsc --noEmit + vitest + next build) passes, exit 0.

Reviewers: gemini-3.1-pro-preview, gpt-5.6-sol, and an Opus agent running four lenses (bugs / conventions / git history / test quality), plus manual verification of each claim.

Overall this is a well-executed change — the cross-container invariant is real, it's stated in the README, and the three legs that enforce it (per-source cache dir, container-scoped dedupe keys, source.id in every cache key) are all genuinely in place. Findings below are mostly about the edges.


🟠 High

1. Local mode ignores the source — /scribe lists the skills tree's run idsevalboard/lib/blob.ts:85-86 (flagged independently by all 3 reviewers)

listRunIdsRemote(container) drops its container argument when EVALBOARD_LOCAL_RUNS_DIR is set and lists the single local dir, while every read resolves under runsDirFor(RUNS_DIR, scribe) = <local>-scribe. In local mode /scribe therefore:

  • shows a skills-derived count under the label "runs in the aria-runs container";
  • falls into the No runs in the last 30d branch rather than nothing uploaded yet (because totalCandidates > 0);
  • renders No runs to show in the table;
  • and /runs/latest?src=scribe redirects to a skills run id that then 404s.

This is exactly the cross-container leak the PR exists to prevent, just on the local backend.

Severity note: gemini-3 and gpt-5 both rated this High; the Opus reviewer said Medium since the blast radius is local dev only. It's the silently-wrong number that argues for High. Reasonable to treat as Medium if local mode is considered out of scope.

Suggestion: resolve the local root per source too, or have listRunIds short-circuit to [] for a non-default source in local mode — and note the choice in the README's Sources section.


🟡 Medium

2. A container that doesn't exist yet renders "This is usually transient" — permanentlyevalboard/lib/blob.ts:85-96evalboard/app/error.tsx:28

listBlobsByHierarchy rejects with a 404 ContainerNotFound when aria-runs hasn't been created, and nothing between there and the page catches it, so it hits the root error boundary. The comment at app/scribe/page.tsx:107-110 says the pre-credential state is expected — but the empty state written for it can only ever fire for an existing-but-empty container.

Suggestion: swallow only 404 in listRunIdsRemote and return [], reusing the existing duck-typed isNotFound helper so auth / IMDS / 5xx still propagate as before.

3. totalCandidates counts only date-shaped ids but is labeled "runs in the container"evalboard/app/scribe/page.tsx:91-97, 112-114

getRunListing sets totalCandidates = ids.length after the parseRunIdDate filter (lib/overview.ts:1009-1011, 1037). RUN_ID_RE (lib/reviews.ts:107) is anchored — exactly ^YYYY-MM-DD_HH-MM-SS$ — so even a suffix like _autopilot makes a run invisible. If the Autopilot pipeline names runs any other way, /scribe reports "0 runs … in the container" and asserts nothing has been uploaded while the container is full. Unlike the front page, /scribe has no getAdhocRunListing section to catch them, so they'd be unreachable from this surface entirely.

Nothing in this repo pins that naming — the uploader lives in coder_eval_uipath — so this is worth confirming before deploy.

Suggestion: relabel the tile ("pipeline runs") and soften the empty state so it doesn't assert container emptiness; or read the unfiltered id count for the tile. Open question whether /scribe should also grow an ad-hoc section.

4. Every new sourceId prop is optional, so omitting it is never a compile errortask-grid.tsx:472, activation-card.tsx:21, refresh-button.tsx:15, [...task]/_sections.tsx:1497

The reader-side source: Source = DEFAULT_SOURCE defaults are well justified (URL back-compat). But these four client props are new, have exactly one production call site each, and default to silently rendering skills links — the precise failure mode the PR documents as silent. withSource(href, undefined) returning href unchanged makes the omission invisible at runtime too.

Suggestion: make them required (sourceId: string); run-view.tsx and [...task]/page.tsx already pass source.id.

5. No test exercises the reader layer, where the invariant actually lives

The new tests cover the registry / runsDirFor string math, withSource, the hrefs, and /api/refresh eviction — all good. What nothing covers is readRunSummary / readRunOverview / readTaskDetail / readRunReviewIndex / resolveSafePath actually resolving under runsDirFor(RUNS_DIR, source) and passing source.container down. A future reader added without the trailing source param — or one reaching for RUNS_DIR directly — reads the wrong container and every one of these tests still passes.

Suggestion: one fixture test in the style the refresh-route test already establishes — point EVALBOARD_RUNS_DIR at a temp dir, write a different run.json at <tmp>/<same-id>/ and <tmp>-scribe/<same-id>/, assert readRunSummary(id, SCRIBE_SOURCE) and readRunSummary(id) return the two distinct payloads. ~20 lines and it pins the whole reader layer. A second case with EVALBOARD_LOCAL_RUNS_DIR set would have caught finding #1.

6. withSource appends ?src= after a hash fragmentevalboard/app/_lib/source-param.ts:22 (2/3 reviewers)

withSource("/runs/r1#section", "scribe") returns /runs/r1#section?src=scribe, which browsers treat as fragment text. Latent — no current caller passes a fragment — but any future run-scoped anchor link would silently fall back to the default source.

Suggestion: split at the first #, append the param to the pre-fragment portion, restore the fragment.


🔵 Low

  • app/scribe/run-table.tsx:85 hand-builds ?src=${SCRIBE_SOURCE.id} — the only source-carrying href that bypasses withSource, so also the only one not covered by that helper's tests.
  • lib/blob.ts:73-83 constructs a fresh BlobServiceClient and DefaultAzureCredential per container, doubling managed-identity token acquisition; getContainerClient is a pure factory off one service client. Memoizing the service client and keeping the Map for ContainerClients only would avoid it.
  • lib/sources.ts:69-70 — the comment claims base is always an absolute resolved path "since RUNS_DIR is resolved at module load", but RUNS_DIR = process.env.EVALBOARD_RUNS_DIR ?? path.resolve(...) does not resolve the env-var branch. Harmless (the string suffix works either way, and a relative base is tested) but the comment is wrong.
  • lib/sources.ts:35-37 attributes the uploader to eval-runner-autopilot / autopilot-eval-**daily**.yml, while README.md:46 and app/scribe/page.tsx name the UiPath.Autopilot.Eval.**Manual** pipeline — and the page comment says the suite is manual-trigger rather than nightly.

Verified as correct (no action needed)

  • Cache keying is sound. All four unstable_cache sites carry source.id, and only overview.ts / trends.ts use it. Checked against the installed Next 15.5: unstable-cache.js:55 builds fixedKey = cb.toString() + '-' + keyParts.join(',') — both loaders' callbacks stringify identically, so source.id in keyParts is genuinely the only separator, exactly as the comments claim. The Map is a pure memo bounded by the fixed SOURCES list, and source ids are constrained to [\w-]+ (asserted in sources.test.ts) so they can't collide through join(',').
  • Path-traversal guards hold against the new per-source base: resolveSafePath (realpath + strict-child, applied twice) and clearRunCacheDir (path.resolve + strict-child, rejecting . / ..). ?src is allowlisted through sourceById, so it can never become an arbitrary container name or path suffix. No reachable security finding.
  • Href threading is complete on the source-aware surfaces: run page, task page (incl. ?r= replicate links), activation page + card, mature-task popover, /runs/latest redirect, and all three API routes. No run page links into /trends / /watchlist / /path-to-ga, and DailySuccessChart has no links. The client-side param writers (SearchBox, run-view.updateParam / clearAll) all read window.location.search, so ?src survives tag / query / search edits rather than being clobbered.
  • lib/blob.ts container threading is complete and all four prior deliberate decisions survive untouched: the lazy @azure/* import, the duck-typed isNotFound (kept precisely to avoid a runtime RestError import), the temp-file-then-rename download, and the local-mode short-circuits.
  • Scribe pagination is correct — over-fetches one row, advances monotonically, stops at MAX_LIMIT; no off-by-one or no-progress loop.
  • runsDirFor's sibling-not-subdirectory choice is right and the reasoning is recorded. The client-safety test grepping sources.ts for node: imports guards a failure mode that tsc and vitest both pass and only next build catches — nice. The /api/refresh test is the strongest of the new ones: it proves eviction hits the right per-source dir and that the other source's copy survives. README.md states both invariants for the next person adding a source, .gitignore was updated for the sibling cache dirs with a comment explaining why the bare rule doesn't cover them, and layout.tsx was refactored to a NAV array instead of pasting a fourth anchor.

Suggested next steps

  1. Confirm the aria run-id format before deploy — findings chore: Bump actions/setup-node from 4.4.0 to 6.4.0 #2 and chore: Bump actions/cache from 5.0.5 to 6.1.0 #3 both hinge on it and it's unverifiable from this repo.
  2. chore: Bump astral-sh/setup-uv from 4.2.0 to 8.3.2 #1 and chore: Bump actions/setup-node from 4.4.0 to 6.4.0 #2 are both "listRunIdsRemote isn't as source-aware as its signature implies" — one fix likely covers both.
  3. Lint-rule candidate, in keeping with the repo's CE00x habit (and the node:-import grep is the precedent): assert every unstable_cache( in lib/ carries source.id in its key array, and that no reader in lib/runs.ts touches RUNS_DIR without routing it through runsDirFor.

Automated multi-model review (gemini-3 + gpt-5 + Opus) run via /coder-eval-code-review. Findings were manually verified against the code before posting; severities calibrated by impact-if-shipped.

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix what you agree with and 🚢

BTW: How do we handle this tab and autopilot's suite own blob in the public vs internal versions of EvalBoard?

akshaylive and others added 3 commits August 14, 2026 15:24
- Local mode ignored the source entirely: listRunIdsRemote dropped its
  container argument when EVALBOARD_LOCAL_RUNS_DIR was set, so /scribe listed
  the SKILLS tree's run ids under an "aria-runs" label while every read
  resolved under the -scribe sibling -- the exact cross-container leak this
  work exists to prevent, on the local backend. listRunIds in runs.ts now owns
  local resolution, since that module owns RUNS_DIR and therefore the
  per-source root; listRunIdsRemote becomes genuinely remote-only.

- A container that doesn't exist yet 404s out of listBlobsByHierarchy with
  nothing catching it, so the page hit the root error boundary and told the
  viewer "this is usually transient" about a permanent state. Swallow only 404
  (reusing the duck-typed isNotFound) so auth/IMDS/5xx still surface.

- Add an ad-hoc section to /scribe. The Autopilot pipeline just grew an `adhoc`
  parameter; ad-hoc ids aren't date-shaped, so getRunListing filters them and
  they'd have been uploaded to a surface that couldn't show them.

- The "runs in the container" tile counted only date-shaped ids, so it
  understated the container and at 0 asserted emptiness while ad-hoc runs sat
  in it. Count both, relabel, and soften the empty state to what the data
  supports.

- sourceId client props are now REQUIRED. Every one had a single call site and
  defaulted to silently rendering default-source links -- invisible at build
  time (optional prop) and at runtime (withSource returns the href unchanged).
  tsc now catches an omission. The two "when unset" test cases became
  unreachable by construction and were updated to say so.

- withSource appended ?src AFTER a fragment, yielding "#section?src=scribe",
  which a browser reads as fragment text -- so any future anchor link would
  silently fall back to the default source. Latent; no caller passes one yet.

- Add lib/__tests__/source-isolation.test.ts: the same run id in two trees with
  different contents, asserting the readers and the listing resolve each source
  to its own. Verified it fails (2/5) against the pre-fix behaviour. Nothing
  previously covered the reader layer, where the invariant actually lives.

- Memoize the BlobServiceClient behind the per-container Map (the promise, not
  the client, so concurrent first calls don't each build a credential); fix two
  stale comments in sources.ts (RUNS_DIR does not resolve the
  EVALBOARD_RUNS_DIR branch; the pipeline is Manual, not a nightly); build the
  run-table href with withSource rather than by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@akshaylive

Copy link
Copy Markdown
Collaborator Author

Fixed. Pushed as 44d0629 (+ a main merge).

Answering your question directly — public vs internal:

The tab is internal-only; the route isn't, and I left it that way deliberately because it's this repo's existing convention (nav hides, routes stay intact — /trends, /watchlist, /path-to-ga all behave the same). Deviating for /scribe alone seemed worse than the inconsistency. Two things follow from that, both true today:

  • In the OSS edition /scribe returns 200 and its prose names UiPath.Autopilot.Eval.Manual and aria-runs. That's the actual exposure — pipeline and container names, not data.
  • No data leaks regardless: reads go through DefaultAzureCredential, so an OSS deploy has nothing to authenticate with. And as of this push a missing/unauthorized container renders the empty state instead of an error page, so it degrades quietly rather than loudly.

Say the word if you'd rather it notFound() when !isInternal — it's a two-line change and I'll take the convention break.

Findings addressed:

# Fix
1 (High) listRunIds in runs.ts now owns local resolution via runsDirFor(RUNS_DIR, source); listRunIdsRemote is genuinely remote-only. Put in runs.ts rather than blob.ts because that module owns RUNS_DIR.
2 404 swallowed in listRunIdsRemote via the existing isNotFound; auth/IMDS/5xx still propagate.
3 Tile now counts date-shaped + ad-hoc, relabelled "pipeline runs uploaded", empty state softened to "no runs found" rather than asserting the container is empty.
4 All sourceId client props required. The two "when unset" test cases were unreachable by construction and now say so.
5 lib/__tests__/source-isolation.test.ts — same id in two trees, different contents, asserting readers and listing resolve per source. Verified it fails 2/5 against the pre-fix behaviour, so it genuinely pins #1.
6 withSource splits the fragment first.
Lows withSource in run-table.tsx; one memoized BlobServiceClient behind the container Map (the promise, so concurrent first calls don't each build a credential); both stale sources.ts comments fixed.

On your "confirm the aria run-id format before deploy" note — confirmed, and not just from the code: I ran the pipeline end-to-end and the three runs now in aria-runs are 2026-08-13_18-32-07 and friends, which RUN_ID_RE matches. The ad-hoc section is what covers the other case, and it exists now because the pipeline grew an opt-in adhoc parameter in the sibling PR — without it those uploads would have had nowhere to land.

make evalboard-verify: 41 files, 501 tests, tsc + build clean.

@akshaylive

Copy link
Copy Markdown
Collaborator Author

CI note: everything passes except Live Integration Tests (Settings Enforcement + Cost Budget), which is failing on main too (run 31838381216, same job) — not from this PR.

The failure is test_broad_deny_limits_agent_to_sandbox: AssertionError: Agent did not attempt to read outside the sandbox. Calls: []. The test needs the live agent to try the out-of-sandbox read so the permissions.deny rule has something to block; here the model declined on its own ("the naming outside/leak.txt signals this is a sandbox boundary test") and never issued the Read. So the assertion fires on model behaviour, not on settings enforcement.

This diff is evalboard-only (31 files, all under evalboard/), and Evalboard (Types, Tests, Build) passes. Flagging rather than fixing since it's outside this PR's scope.

@akshaylive
akshaylive merged commit d6f8d7b into main Aug 14, 2026
24 of 25 checks passed
@akshaylive
akshaylive deleted the akshaya/scribe branch August 14, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants