Repository navigation
Conversation
5959410 to
824495b
Compare
The PR description documents sufficient testing: it includes a built real-CLI verification record and representative E2E results. Deferred provider/agent coverage is not blocking. Reviewed only the specified diff; ran no PR code. |
The earlier exact The PR description documents sufficient testing, including a built real-CLI run. Deferred provider/agent coverage is not blocking. Reviewed only the specified diff; ran no PR code, builds, or installs. |
Earlier findings: The exact-submodule case, dot-prefixed path bypass, and local-tool deadline omission are resolved in the current diff. Testing: The PR description documents sufficient testing, including a built real-CLI run. Deferred provider/agent coverage is not blocking. Reviewed only the specified diff; ran no PR code, builds, or installs. |
Earlier findings: All are resolved: protected-submodule handling, dot-prefixed paths, escaped globs, local-tool deadlines, and both languages’ timeout guidance. Testing: The PR description includes sufficient built real-CLI verification. Deferred provider/agent coverage is not blocking. Reviewed only the specified diff; ran no PR code, builds or installs. |
621a620 to
79b2e12
Compare
e3d7f4a to
f7a7737
Compare
Earlier findings: All are resolved in the current diff, including the protected-submodule bypasses, local-tool deadline, bilingual timeout guidance and installation deadline. Testing: The PR description documents sufficient built real-CLI verification. Deferred provider/agent coverage is not blocking. Reviewed only the specified diff; ran no PR code, builds or installs. |
|
No new confident findings in the current diff. Earlier findings: All are resolved, including the protected-submodule bypasses, local-tool deadline, bilingual timeout guidance, installation deadline, judging deadline, and malformed-glob validation crash. Testing: The PR description documents sufficient testing, including built real-CLI verification. Coverage deferred to CI is not blocking. Reviewed only the specified diff against the trusted base review criteria. Ran no PR code, builds, tests, or installs. |
STiFLeR7
left a comment
STiFLeR7
left a comment
There was a problem hiding this comment.
Summary. Adds --benchmark to teamai workflow start: runs YAML-defined benchmark suites (tasks with a starting commit, an optional known-good fix, and a check or success-criteria judge) across tool/model "arms" and repetitions, each trial in an isolated clone with secrets stripped and git hooks/filters/fsmonitor neutered, scored and tabulated. Adds --try to validate one task file's check against its starting commit and fix before it joins a suite. Rebased on #1304, closes #1180.
Verified.
- Secret stripping genuinely happens before any tool/driver launch and can't be reintroduced by a step's
requires.env:negotiate()(src/workflow/requirements.ts:272-276) filters a step'srequires.envto exclude every name in the trial's withhold list (stripped) before building the granted lease'senvScope.provide— soprovideandwithholdare disjoint by construction, not just by call order instepEnvironment(src/env-exec.ts:347-360).TRIAL_WITHHELD_ENV(terms.ts:24-26) is threaded into the plan, prompt/setup scope, and--try. Confirmed by readinge2e/benchmark-env.test.tsin full: none of the withheld names appear in setup/tool/check env even when the team env declares them, and no git hook/fsmonitor/clean filter in the clone sees them either. - Isolation is real:
freshClone(trial.ts:482-504)git inits a fresh repo, fetches onlystartingCommitby SHA (never a branch ref) from the run's checkout or a commit cache, and detaches HEAD there — nogit remote addis ever called, and a SHA-only fetch cannot pull descendant commits, so the fix commit is never reachable in the trial's clone during the prompt phase.check.add/restoreProtectedonly write fix-derived bytes in after the prompt's report is already committed, i.e. after the agent's tool process has exited. check.add/source.fixverification is real, not a naive copy:writeAdded(trial.ts:799-823) andrestoreProtected(trial.ts:699-751) read the source commit into a private index viaread-tree, write withcheckout-index --stdin, and verify every written path byte-for-byte against the commit's blob hash viaholds()(trial.ts:840-853), including the executable bit and symlink-vs-file kind. Submodule paths are explicitly refused.- Path traversal is blocked at the schema level:
RepoPathSchema(suite.ts:30-33) rejects empty/./..segments, backslashes, and a leading:.check.addpaths must additionally fall under acheck.protectglob. - The per-copy lock genuinely serializes prepare/report/check/sweep on the same trial copy without serializing across different trials — confirmed via
e2e/benchmark-trial-copy.test.ts(a second concurrentnext --preparegetsbusyrather than a corrupted/double clone; a killed report can be retried cleanly). - Judge-at-timeout doesn't double-score (
e2e/benchmark-judges.test.ts: exactly one judge launch, run ends timeout, trialunscored); "Not logged in" doesn't corrupt subsequent trials (verified both by the PR's real-CLI evidence and the general error path intakePrompt/settle). - Spot-checked 3 of the Evidence table's test-file references against their actual bodies (
benchmark-env.test.ts,benchmark-checks.test.ts,benchmark-judges.test.ts) — all three test exactly what their row claims, including several of the listed adversarial cases. npx tsc --noEmitandnpx oxlint --type-aware --deny-warnings --report-unused-disable-directivesboth ran clean on this branch.
Findings. None I can back with a concrete failure scenario or a cited rule violation. I specifically looked for races in the per-copy lock, double-scoring at judge timeout, "Not logged in" corrupting later trials, and path traversal via check.protect/check.add — all are handled and tested.
Not covered / unverified.
- Could not execute
npm run buildorvitestmyself in my environment (a sandbox policy against running an unreviewed PR's own code) — statictsc/oxlintare clean, but I don't have independent confirmation of the claimed578 files, 16565 passed/383 passede2e numbers beyond reading the relevant test sources directly. - Didn't re-audit the pre-existing, unmodified
src/update.tsfile-lock internals (acquireLock/releaseLock) — treated as trusted existing infrastructure since this PR doesn't touch it. - Didn't review the other ~110 changed files outside
src/workflow/benchmark/*.ts(pull/review/doctor wiring, dashboard, docs) at the same depth — focused the time on the new module and its safety-critical claims. TRIAL_WITHHELD_ENVonly covers git/CI credential-shaped names, not generic cloud credentials (AWS_*,NPM_TOKEN, etc.). Looks like a deliberate scope choice perterms.ts's own comment ("credentials a trial could push, publish or authenticate with") rather than an oversight — flagging it as a question rather than a finding, in case it's worth widening in a follow-up.
Verdict. Comment (approve-leaning, no blocking findings). The two most safety-critical claims — the secret-strip-wins-over-requires.env guarantee and the no-remote/ancestors-only clone isolation — both check out at the actual git-plumbing and negotiation-logic level, not just by the PR description, and the adversarial test cases listed genuinely exist and test what they claim.
|
@STiFLeR7 thanks for the thorough review. You found nothing blocking, so could you approve it? |
STiFLeR7
left a comment
STiFLeR7
left a comment
There was a problem hiding this comment.
Confirmed — nothing blocking. Approving.
…ing a same-name workflow
…nd task A team that runs a suite every week needs more than the last run's table: whether an arm's lead holds across runs, which task splits the arms, and what changed when a pass rate moved. BenchmarkHistory turns ended run records into that view: suites with their trend, arm series keyed by name, tool, model and effort, pooled pass rates with a 95% Wilson interval over the runs of the current suite version, median usage, task by arm and task by run rates, and the runs list. Tested with literal expectations in benchmark-history.test.ts (Wilson bounds, usage medians, unstable task counts, suite selection).
… the team reports GET /api/benchmarks reads the ended benchmark runs on this machine and the run records on teamai-reports, keeps this machine's copy of a run that is in both, and returns BenchmarkHistory for a window of 30, 90 or 365 days and a suite. Records are cached by path and file stamp, and a file without a benchmark field is skipped before it is parsed, so a reload reads only what changed. Tested in dashboard-benchmarks.test.ts: a local cancelled run with published records (benchmark, other, broken), a re-read after a file changes or is removed, and the query parser.
Suites with their trend; the leading arm and whether its interval separates it from the next; pass rate by arm with the interval; pass rate against tokens, duration or cost with the arms nothing beats on both; pass rate per run with the runs where the suite or the team assets changed; tasks by arm with unstable runs and tasks not measured; tasks per run; the last run's trials by result; and the runs list. Tables become labelled cards on a phone, the charts are drawn at the width they show, and every label has a zh-CN translation. Tested in dashboard-ui.test.ts and by screenshots at 1440, 834 and 390 px in dark and light themes.
…age through the real CLI After teamai workflow start fix --benchmark, teamai dashboard's /api/benchmarks lists suite fix with its one run from this machine, the arm's pooled pass rate, the task, and the last run's table.
Six questions a team asks of its benchmark runs, each with the panel that answers it and what it showed, then how the numbers are counted. benchmarks.md links to the page from Results.
…ad, and check its run id A read that failed was kept as not a benchmark run under the file's size and modification time, so a published record, which never changes, stayed hidden until the dashboard restarted. Now only a file that was read is kept. A record's run id is checked as the other readers check it, since the page opens the run by it. Tested: the new case in dashboard-benchmarks.test.ts fails with the previous reader and passes with this one.
…e counts them The Benchmarks page counted an arm's cost over every trial that was not not_run, while a run's table counts the trials whose prompt ran. A trial that errored before its prompt ran made the pooled cost incomplete and turned the Cost axis off, while the last run's table showed a known cost. table.ts now exports the one rule, observedTrials, usageOf and deviationsOf, and the history pools with it. The page also shows an arm's pooled deviations and trials not verified, how many trials each median used, unknown tokens as unknown, and costs to four decimals as the table prints them. A task a run ended before measuring counts as not measured, as its hatched cell already showed. The scatter no longer changes the chosen axis while it draws, and history.ts is a declared entry of the benchmark folder. Tested: benchmark-history.test.ts compares the pooled usage with the run table's and counts deviations and unmeasured tasks; benchmark-table, dashboard-ui and workflow-module-boundaries pass.
The Benchmarks page took a suite's version from the hash of suite.yaml alone, so a run after a task file changed was pooled with the runs before it, and the history drew no Suite changed line between them. A version is now the run's sourceHash, over the suite file and each task file, from the same runSourceHash a run is started with. The cost frontier also skips a weaker arm at the same usage as a stronger one, and the e2e reads the unmeasured counts by the names the API returns. Tested: a new case in benchmark-history.test.ts (same suite.yaml, edited task: two versions); dashboard-workflow-runs e2e passes.
…eadline untrustedGit lists the hooks a checkout's config names before it runs the requested git, and only the requested git was bounded by the deadline. A config that includes a pipe no one writes, which setup or a tool can configure, held that listing, and so the step's report and cleanup, forever. Tested: the new case in workflow-setup.test.ts timed out at 20 s before this change and settles at the 2 s deadline with it; benchmark-limits and benchmark-checks e2e pass.
… matrix
A suite could compare tools and models, but not what teamai itself distributes: every arm ran on the team revision this checkout pulled. An arm now sets team: pulled, none, a branch, tag or commit of the team repo, or { without: [<skill>, ...] }. A suite can list matrix instead of arms, and teamai crosses its tool, model and team values into one arm each, so two arms that differ in one value compare that value alone.
Start resolves each arm's team before the run exists, and refuses a ref the team repo does not have, a skill the pulled revision lacks, and a none arm on a machine whose user-scope install delivers to the arm's tool, where its trials would read team assets from the home directory. A none arm installs nothing; a ref arm installs that revision; a without arm excludes the skills through the selection the install already honours. The record and the run's table carry each arm's team, and a trial on other team assets than its arm's counts as a deviation.
Tested: benchmark-suite, benchmark-plan and benchmark-table cover the new keys, names and refusals; the new benchmark-team-arms e2e runs a tool x team matrix, a ref arm and the three refusals through the built CLI.
…w a tool across team revisions The history pooled each arm on its own, so it could not say how much the team's assets added, or whether a change to them helped. It now lists, for the suite's current version, every two arms that differ only in their team assets, tool or model, pairs their trials by run, task and repetition, and gives the difference a 95% interval for paired proportions (Newcombe, method 10). It also pools each tool and model by the team revision it ran on, for the configurations that ran on more than one, and records each run's tool versions. Tested: benchmark-history.test.ts covers the comparisons of a tool x team matrix, the paired interval against worked values, team revisions across runs and within one, and tool versions per run.
…one, with the files it changed A team revision is a SHA in a run record. The Benchmarks API now reads each revision the selected suite ran on from this machine's team clone: its commit date and subject, and the files outside benchmarks/ it changed since the revision before it, relative to the team directory. The revisions are in commit order when the clone has every one, and in run order otherwise. What git answered is kept for the process, since a commit never changes; a revision the clone lacked is asked again, as a pull may bring it. Tested: two cases in dashboard-benchmarks.test.ts, on a real team clone: revisions that ran newest first come back in commit order with their changed files, and one the clone lacks keeps its run order and no name.
…page The Benchmarks page answered questions about tools and models. Two tabs beside the existing Arms panels now answer the questions about the team's own assets, under the same sidebar entry. Compare picks a dimension (team assets, tool or model) and shows the chosen pair's difference with its interval, pass rate, cost per passed trial and tasks up or down; a forest plot of every pair; arrows from one arm to the other on pass rate against tokens, duration or cost; and each task's change. Team changes picks a tool and model and draws its pass rate per team revision in commit order, marking a revision told apart from the one before it; clicking one lists the team files it changed and each task's rate before and after. Pass rate per run also marks a tool update. Each tab explains, with a suite example, how to get data into it when it has none. The chosen tab is remembered, and every new label has its zh-CN text. Tested: dashboard-ui checks every label has a zh-CN translation; dashboard-workflow-runs and dashboard-unified e2e pass; the tabs were checked at desktop, tablet and phone widths, dark and light, on a seeded workspace.
… changes tabs Examples first: a suite with an arm on each kind of team assets, a tool x team matrix with the arms it makes, and the questions each new tab answered on a seeded workspace. English and zh-CN.
…team ref did not resolve A record kept only the model each arm's alias resolved to, so claude and codex on alias strong had different models, and Compare never compared the tools of a suite that named a model, which is what the suite parser asks for when the tool varies. The plan and the record now keep the alias as alias, and two arms on one alias differ only in tool. Start reported every failure to resolve an arm's team ref as no branch, tag or commit. It now names the team repo's branches when the ref is not one, and refuses with arm_team_unreadable, with git's message, when git could not read the team repo, also for a without arm whose skills git could not list. Tested: a new case in benchmark-history.test.ts (two tools on alias strong compare by tool; codex on strong and fast by model) failed before the change; benchmark-models e2e checks the alias in the record, and benchmark-team-arms e2e the new message.
…ool ran on before Team changes compared a revision with the one the chosen tool and model ran on before it, but listed the files changed since the revision before it among all, which another tool may have run alone. The API now returns teamDiffs by <before>..<after> for each two revisions a tool and model ran on in turn, and the panel reads the pair it shows. Also: the twelve arms a matrix can make get twelve colours, a difference that rounds to zero prints 0 pp without a sign, a cost change from zero shows no percentage, and the change column and the told-apart tags have their zh-CN text. Tested: a new case in dashboard-benchmarks.test.ts (claude on A and C, codex on B and C: A..C and B..C, not A..B) failed before the change; dashboard-ui passes.
…lows the arms' team assets
Start looked for skills only at skills/<name>/SKILL.md, so an arm without a skill the team keeps under a role or project namespace, skills/<namespace>/<name>/, was refused though pull delivers it by that name. A skill's name is now the directory that holds its SKILL.md, at any depth under skills/. Tested: a new benchmark-team-arms e2e case (without: [nested], with skills/dev/nested/SKILL.md) was refused before the change and runs with it.
…ials Compare drew its arrows and its cost and token cards from each arm's pooled series, which also counts runs the other arm did not run, while its difference counted the paired trials only: the two could disagree. A comparison now carries each arm's usage over the paired trials, as the run table counts it, and the arrows and cards read its paired rates and usage. Cost per passed trial is the cost of those trials over the ones that passed, not the mean cost of a launched trial over the pass rate of planned ones, which inflated it when a trial never launched. Tested: a new case in benchmark-history.test.ts (claude alone in one run, claude and codex in the next: the comparison's usage is the second run's) failed before the change; dashboard-ui passes, and the Compare tab was checked on a seeded workspace.
asks stepEnvironment now takes the team env's values and facts, so a provided key that lost its value gets the env add advice for an undeclared key. A trial's setup, check and pull pass the run's team env; a step environment that only withholds keys passes no team env, and --try the environment's keys as present. Tested: tsc and lint clean; the benchmark e2e and unit suites run in the gates below.
…encent#1534 lets it fail init initHttp no longer swallows a local agent setup that fails, and this file's fs mock has no readJson, so the case that checks a re-init keeps the member's workflow settings failed on the setup it does not test. Tested: init.test.ts passes (85).
7ae6cf5 to
259c6be
Compare
Summary
A team could not tell whether a tool, a model or its own assets fix its real tasks better. Now a suite in the team repo runs as a workflow: every arm works the same tasks in fresh clones and is scored by a check it never sees. An arm can change the tool, the model or the team assets, and a
matrixcrosses them. The dashboard's Benchmarks page charts every run, and compares arms that differ in one thing only.How much do the team's assets help each tool? A
matrixcrosses its values into one arm each:Does the team's next branch beat main, and what does one skill add? Arms on other team assets:
Is the cheap model good enough once it has our assets?
Which tool does best on one model alias, with today's assets and with the last release's?
onboarding-docsclaude-pulled,claude-none,codex-pulled,codex-nonefix-authmain,next,no-auth-tokens,baremigrate-dbstrong-pulled,strong-none,fast-pulled,fast-nonereview-prclaude-pulled,claude-v2-3-0,codex-pulled,codex-v2-3-0,opencode-pulled,opencode-v2-3-0Two arms that differ in more than one thing are not compared. Start refuses a ref the team repo lacks, a skill the pulled revision lacks, and
noneon a machine whose user-scope install would hand the trial the team's assets.Compare: how much the team's assets add, trial by trial (
onboarding-docs, a seeded workspace).Team changes: one tool and model across the team revisions it ran on, with the files each changed.
Arms: every arm over time, as before.
+41 pp · 95% +16 pp to +59 ppover 27 paired trials,−60%cost per passed trial+15 pp, interval crosses zero:not separated67ee2cechanged 3 files,session-fixation1/12 to 3/12claude 67% · 95% 57%–75% · not separated from codexmigrate-db?Light theme, Cost axis, zh-CN, tablet and phone
The Error, Unscored, Not run, Stopped by a limit, Deviations and Not verified columns, all 0, are left out here. The Team column appears once an arm runs on other assets than the pulled revision.
How a run is built, how an arm gets its team assets, and how the page compares
check.addforms and trial limits per tooltimeoutmax_turns--max-turns->limit: max_turnsunenforcedunenforcedunenforcedbudgetstep_finishunenforced; a final total past it recordslimit: budgetType of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpassesReal CLI, built
dist/index.js:--driver local --sandboxwith the host's logged-inclaude, on a task whose hidden test comes fromcheck.add { from, to }:trial{ max_turns: 1 }max_turns{ budget: { tokens: 150 } }budgetTeam arms,
e2e/benchmark-team-arms.test.ts, through the built CLI with a stub tool:team: [pulled, none, { without: [fixer] }]matrix: each arm's trial saw its own skills, andnonesaw no.claude/;without;noneunder a user-scope install.Each real run: baseline
valid, the member'sgit statusunchanged, no trial clone left. Codex and OpenCode with real models, thegitlabandgithubproviders, and Windows are left to CI.Issue criteria and the tests that cover them
review --branchbenchmark-suite.test.ts,review-branch.test.ts,e2e/benchmark-pull.test.tsbenchmark-plan.test.ts,e2e/benchmark-lifecycle.test.tse2e/benchmark-team-assets.test.ts,e2e/benchmark-env.test.tsbenchmark-check.test.ts,e2e/benchmark-checks.test.tse2e/benchmark-models.test.ts,benchmark-table.test.tse2e/benchmark-judges.test.ts,e2e/benchmark-local.test.tse2e/benchmark-lifecycle.test.ts,e2e/benchmark-driver-contract.test.ts--try: each outcome and refusale2e/benchmark-try.test.tscheck.add: fix paths, suite files and directories, refusalse2e/benchmark-checks.test.ts,benchmark-suite.test.ts,benchmark-plan.type-test.tse2e/benchmark-limits.test.ts,workflow-local-tool-adapter.test.ts,workflow-setup.test.tsteamandmatrix: names, refusals, install per arm, record, Team columnbenchmark-suite.test.ts,benchmark-table.test.ts,e2e/benchmark-team-arms.test.tsbenchmark-history.test.ts,dashboard-benchmarks.test.ts,dashboard-ui.test.ts,e2e/dashboard-workflow-runs.test.tsAdversarial cases, each red before its fix: a protected test hidden with
assume-unchanged, a clean filter,git replaceor a nested repository; a hook in the clone; twonext --preparecalls on one trial; a report killed before acceptance; a protected path inside a submodule; a tool or judge that hangs past the suite timeout; a tool stopped holding git's locks; a config that includes a pipe no one writes; two tools on one model alias; a revision only another tool ran on; a run only one arm of a comparison ran.Related Issues
Closes #1180, closes #1400, closes #1401. Built on #1304: benchmark runs are routed inside
WorkflowClient.next/report. #1402 builds on--try,source.fixandcheck.add.Notes for Reviewers
source.fix,check.add,trial,teamandmatrixship in this first release. Blast radius: moderate. Everything new sits behind--benchmark,--tryorbenchmarks/. Shared paths that change for ordinary runs:review --branchchecks every selection's sync, a re-init keepsworkflowssettings, and the git around workflowsetupruns without hooks or fsmonitor and stops at the step deadline.not separated). teamai prepares the trial, so a driver does not runinitorpull --team-ref.src/workflow/benchmark/plan.ts(what a run is), thentrial.ts(what a trial gets),table.ts(how results count) andhistory.ts(the page, pure).