Repository navigation
Conversation
.github/workflows/repo-eval.yml (on: workflow_call) runs, on the caller's checkout, `repo validate` (a gate), `repo scan` (report only, job summary and artifact) and `repo tasks check` as a matrix over the selected tasks: `changed` (the tasks a PR touches, plus a smoke set when the harness, graders, manifest, calling workflow or `smoke-paths` change), `all`, or a list, minus `exclude-tasks`. dokimi is installed from a pinned commit SHA or tag (a branch is refused), with that commit's requirements.lock.txt as constraints. repo-eval-self.yml calls it on dokimi's own .agent-eval/ with the commit under test, when .agent-eval/, the repository mode or either workflow changes. The end of contract §9 documents the inputs and the caller snippet. Part of intel#41 (step D8). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Part of intel#41 (step D8). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
- Coverage 91.29% 91.28% -0.01%
==========================================
Files 216 216
Lines 26565 26568 +3
Branches 4337 4338 +1
==========================================
+ Hits 24252 24253 +1
- Misses 1645 1646 +1
- Partials 668 669 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Task selection can silently skip affected tasks, and uploaded results omit stderr diagnostics.
2 open issues
What changed in this PR
Centralizes no-LLM repository evaluation in a reusable workflow, advancing step D8 of #41.
Changes:
- Adds pinned dokimi installation, validation, scanning, and selected task checks.
- Runs the workflow against dokimi’s own evals.
- Documents workflow inputs and caller setup.
| File | Description |
|---|---|
dokimi/docs/repo-evaluation-contract.md |
Documents reusable CI usage and pinning. |
.github/workflows/repo-eval.yml |
Implements reusable checks, task selection, summaries, and artifacts. |
.github/workflows/repo-eval-self.yml |
Checks dokimi’s evals using the commit under test. |
Review effort: Balanced
…r, include .err files in artifacts Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if r.returncode: | ||
| print(f"::warning::git diff {' '.join(rng)} failed, checking every task: {r.stderr.strip()}") | ||
| return None | ||
| return r.stdout.split('\0') if r.stdout else [] |
There was a problem hiding this comment.
Fixed in 631ba99. The tests no longer use a local copy of the parser: they lift the Select tasks script out of repo-eval.yml and run it in a scratch git repo with GitHub's env. Cases: a task directory with a space, a quoted file name, a move between tasks (both sides selected), shared helpers and _shared/ under the tasks dir, a deleted task, the eval-dir harness plus a changed task, and a manifest given as ./.... Reverting the -z split to whitespace or dropping --no-renames now fails 4 and 1 tests.
There was a problem hiding this comment.
Confirmed on the current head: the tests in test_repo_eval_task_selection.py extract the Select tasks step with yaml.safe_load and run it in a scratch git repo (631ba99), so reverting the NUL split fails them. No change in this round.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ffbb6d1. The workflow reads well: pinning, permissions and the env: routing are right, and the summary and upload steps match what repo validate, repo scan and repo tasks check write. Two things are worth fixing before merge: the new tests do not exercise the selector, and a shared file in the tasks directory selects nothing.
What I ran, on a Linux review host at ffbb6d1:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network); 5186 tests.zizmor --offline --min-severity medium .github/workflows: no findings.- The
Select tasksscript extracted from the step and run against throwaway git repos (spaces, quotes, unicode, renames, deletions, push and schedule events, exclusions, unknown ids): the NUL and--no-renameshandling is correct. - The new tests against a copy of the workflow with line 219 reverted to
r.stdout.split(): 7 passed.
Checked, no issues
- Pinning: a 40-digit SHA is taken as is, a peeled
^{}tag line wins over a lightweight one, short SHAs, branches and injection-shaped values are refused. - Artifacts:
check/check.json,check/*/grade.json,*.logand*.errmatch the layoutrepo tasks check --outwrites. - Gate:
tasks != '[]'skips the matrix for an empty selection and runs it for["a"]. - Docs: the new paragraph matches the inputs and defaults in the workflow.
| from pathlib import Path | ||
|
|
||
|
|
||
| def parse_changed_files(output: str) -> list[str]: |
There was a problem hiding this comment.
These tests never run the workflow's selector, so the regressions this PR fixed can come back unnoticed. I agree with @copilot-pull-request-reviewer here. New evidence: I reverted .github/workflows/repo-eval.yml:219 to r.stdout.split() in a copy and ran this file: 7 passed. A task directory named my task is then split in two and never selected, and the suite stays green.
parse_changed_filesis defined in this module, so the four NUL tests check a copy of the logic.test_no_renames_flag_prevents_omissionandtest_err_files_in_upload_pathare substring checks on the YAML text. The*.errcheck passes if the line is moved into a comment.test_err_files_purposeassertslen([...literal list of 3...]) == 3and cannot fail.- Nothing covers the
harness()rules, the smoke union,exclude-tasks, the deleted-task filter or the push and git-failure fallbacks.
Suggestion: load the workflow with yaml.safe_load, take the step with id: select, extract the python - <<'PY' body, and run it in a temp git repo with a fake validate.json and the step's env vars (EVENT, BASE_REF, MANIFEST, TASKS, SMOKE_TASKS, RUNNER_TEMP, GITHUB_OUTPUT, ...). Assert the emitted tasks= for a path with a space, a rename between two tasks, a harness change (smoke set), an .md change in the eval dir, an excluded task and a deleted task. A prototype of this kills the .split() mutant and the mutants that drop the .md exemption, the smoke union and exclude-tasks. Drop test_err_files_purpose.
There was a problem hiding this comment.
Fixed in 631ba99: the substring and copy-of-the-parser tests are replaced by tests that run the workflow's Select tasks step against a scratch git repo (path splitting, renames, smoke-set union, shared files, deleted task, manifest normalisation); the upload test now parses the YAML step instead of grepping text. Mutation checks: split() instead of NUL split and no --no-renames both fail. Shared files and manifest normalisation themselves were fixed in 48d103e.
There was a problem hiding this comment.
Partly addressed. 631ba99 and 48d103e cover the YAML-step extraction, the rename, smoke-set union, shared files, deleted task and manifest cases. 4670ea0 adds the missing selector cases: explicit task lists, all with exclude-tasks, and push with and without a previous tip. Still not covered: the push/git-diff failure fallback and the empty-BEFORE edge beyond the all-zeros case. Left open for a follow-up.
…st path - Treat files under tasks dir whose first segment is not a task id as harness changes (e.g., tasks/common.py shared by graders, deleted task directories). They now trigger the smoke set as intended. - Normalize manifest path with posixpath.normpath() so changes to manifests like './repo-eval.yaml' are correctly detected (git reports normalized paths). - Add regression tests for shared files and path normalization. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…it repo Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
git ls-remote, the workflow only checks thatshais non-empty, not… · Newpull_request, task selection depends onorigin/<BASE_REF>existing locally. If… · Newpushevents (BEFORE..HEAD), explicittasks… · New
Review effort: Lite
| with open(wf_path, encoding="utf-8") as f: | ||
| workflow = f.read() | ||
| # Find the upload artifact section for check results | ||
| assert "repo-eval-check-" in workflow |
There was a problem hiding this comment.
Added in 4670ea0: explicit task list (TASKS=t1), all with exclude-tasks, and push with a real previous tip and with an all-zeros BEFORE. The TASKS with a task id containing a space case is not added; it depends on the whitespace-id question in the thread on ids(). Leaving this open for that.
There was a problem hiding this comment.
🟢 Approval recommended
Prior selection, artifact, and regression-test issues are addressed, with no unresolved blocking findings.
5 open issues
pushevents (BEFORE..HEAD), explicittasks…pull_request, task selection depends onorigin/<BASE_REF>existing locally. If…git ls-remote, the workflow only checks thatshais non-empty, not…
Review effort: Balanced
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 631ba99. The three threads from my last review are fixed in the code, and the new tests run the real Select tasks step. The test file still leaves most of the selector's branches unchecked, and the new first-segment rule has one inconsistency with the documented Markdown exemption.
What I ran, on a Linux review host at 631ba99:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network).- The
Select tasksstep extracted and run against throwaway git repos, withvalidate.jsonfaked: new task, task name that prefixes another, push with an all-zero and with an unknownBEFORE, deleted task, renamed task, file in a non-task directory, tasks dir equal to the eval dir. Only the Markdown case below gave a wrong result. - A mutation run over the selector: 24 mutants, 15 of which pass the new test file. The
-zsplit,--no-renames,normpathand first-segment mutants are killed. zizmor --offline --min-severity medium .github/workflows: no findings.
Still open from my earlier review:
dokimi/tests/test_repo_eval_task_selection.py:148: the file now runs the real step, but these mutants still pass all 11 tests: dropping the.mdexemption, droppingexclude-tasks, ignoring an explicittasks:list or itsno such task(s)error, dropping theSMOKE_PATHScheck, dropping the calling-workflow comparison, breaking thepushrange (BEFORE..HEAD), and deleting thegit difffailure fallback. Every test usesEVENT=pull_requestandTASKS=changed.
I agree with @copilot-pull-request-reviewer that ids() (repo-eval.yml:193) cannot address a task directory with a space in tasks, smoke-tasks or exclude-tasks, although the new tests select one.
Checked, no issues
- Shared files:
tasks/common.pyandtasks/_shared/lib.pynow select the smoke set; a task added in the PR selects only itself. - Manifest:
./repo-eval.yamland./.agent-eval/../repo-eval.yamlcompare equal to git's path afternormpath. - Fallbacks: a push with an all-zero
BEFORE, and one with an unknownBEFORE, both check every task, the second with a warning. - Artifacts: the upload test now reads the step through
yaml.safe_load, so a*.errline in a comment is no longer counted (by reading the test; I did not run that mutant).
One nit inline.
…ks-dir Markdown The changed-task diff uses github.event.pull_request.base.sha instead of a local origin/<base> ref, dokimi-ref must resolve to a full 40-digit SHA, and Markdown under the eval dir (the tasks dir included) no longer selects the smoke set. Tests cover explicit lists, all with exclude-tasks, and push with and without a previous tip; the contract says a deleted task's files select the smoke set. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The workflow's tasks, smoke-tasks and exclude-tasks inputs are split on whitespace, so a task directory with a space could never be named there. Reject such ids in repo validate and document the rule in the contract. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…arent base.sha is the base at PR-open time, so BASE_SHA...HEAD on the merge checkout also counts every commit the base gained since as the PR's change and selects those tasks (and the smoke set). HEAD^1 is the base tip the merge was made on; base.sha stays the fallback for a non-merge HEAD. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A caller forwarding an unset dispatch input passes '' on pull_request, which selected an explicit empty list: nothing checked, tasks-check skipped, run green. Empty, blank or separator-only now means the documented default. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Both are what a caller gets without configuring anything, and dropping `or all_ids` or the guidance-only early exit left every test green. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5e6874f. The merge-commit range, the empty tasks rule and the whitespace-id check are right, and the new tests pin them. Two behaviour bugs and one nit are left; the selector's test gaps from my earlier thread are still open.
What I ran, on a Linux review host at 5e6874f:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network).- The
Select tasksstep extracted and run against throwaway git repos with a fakedvalidate.json(smoke-paths inside the eval dir, comma ids, merge commits). Reproductions for both bugs below. - A mutation run over the step's script: 37 mutants, 12 of which pass all 22 tests in
test_repo_eval_task_selection.py. zizmor --offline --min-severity medium .github/workflows: no findings.
Still open from my earlier review:
dokimi/tests/test_repo_eval_task_selection.py:148: the file covers more now, but these mutants still pass all 22 tests: dropping thesmoke-pathscheck (repo-eval.yml:246), dropping the calling-workflow comparison (:246, alsoremoveprefixon:238), deleting either unknown-idsys.exit(:262,:268), reversing the push range to[HEAD, BEFORE]or usingBEFORE...HEAD(:215), deleting thegit difffailure fallback or making it return[](:219-221), and weakening theBEFORE.strip("0")guard to a bare truthiness test or tostrip()(:214). Each needs one_selectcase; the unknown-id ones need a helper that accepts a non-zero exit.
I agree with @xaleryb that the repo-eval-self path filter and smoke-paths miss parts of the repository-mode code.
Checked, no issues
- Pull request range: with a merge commit as HEAD,
HEAD^1...HEADignores what the base gained after the PR opened; without one,BASE_SHA...HEADis used. Mutants that break either leg fail the tests. - Empty
tasks:""," "and","selectchanged. - Pin check: the resolved SHA must match
^[0-9a-f]{40}$under the defaultbashshell; neither workflow setsshell:. - Manifest rule: with the whitespace check removed in a copy,
test_repo_manifest.pyfails on bothmy taskandtab\tid. The repo's own ten tasks still validate. - Docs: the contract's section 9 paragraph matches the workflow's inputs and defaults.
Three inline comments.
…ommas in task ids A Markdown file named in smoke-paths selected nothing because the eval-dir Markdown exemption ran first. The explicit smoke path check now runs before it. The workflow splits its task inputs on commas as well as whitespace, so a task id containing a comma passed validation but could not be named, and exclude-tasks silently ignored it. repo validate now refuses commas too. The fragment moves to Changed, since this is a new validation error. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 279db0f. The three fixes from my last review hold: a Markdown file named in smoke-paths now selects the smoke set, repo validate refuses a,b, and the changelog fragment is under ### Changed. The selector's test gaps are still open. One new point is worth fixing before merge, and there is one nit.
What I ran, on a Linux review host at 279db0f:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network); 5206 tests, 1 skipped; changed-line coverage 100%.- The
Select tasksstep extracted and run against 45 throwaway git repos with a fakedvalidate.json(smoke-pathswith and without a trailing slash,.mdfiles under and outside the eval dir, manifest and calling-workflow paths, push and schedule events, tasks dir equal to the eval dir).check_repoon a task dira,breturnsok=Falsewith the new message. - A mutation run over the step: 24 mutants, 8 of which pass the whole test file. The new
smoke-pathstest kills the reordering mutant and fails on 5e6874f (2 failed, 22 passed).
Still open from my earlier review:
dokimi/tests/test_repo_eval_task_selection.py:148: thesmoke-pathscheck is now pinned, but these mutants still pass all 24 tests: dropping thecallercomparison or itsremoveprefix(repo-eval.yml:238,:244), deleting either unknown-idsys.exit(:262,:268), weakening theBEFORE.strip("0")guard to a bare truthiness test (:214), andBEFORE...HEADas the push range (:215). The one that matters most is thegit difffailure fallback (:219-221): changingreturn Nonetoreturn []passes, and then a push whose previous tip is missing from the clone selects no task and the run is green with nothing checked. Each needs one_selectcase; the unknown-id ones need a helper that accepts a non-zero exit.
Both inline.
| the eval directory outside its tasks (Markdown anywhere in it excepted), the manifest, the | ||
| calling workflow file or a path under `smoke-paths`. A file in the tasks directory that is |
There was a problem hiding this comment.
Nit: the contract does not say that a smoke-paths entry wins over the Markdown exemption. The paragraph reads "the eval directory outside its tasks (Markdown anywhere in it excepted), the manifest, the calling workflow file or a path under smoke-paths", and a reader takes the exception to cover all four. harness() checks the manifest, the calling workflow and smoke-paths first (repo-eval.yml:244), so with smoke-paths: .agent-eval/prompts/ a PR that changes only .agent-eval/prompts/p.md selects the smoke set (tasks=["t1"]), and the same PR without that entry selects nothing.
Suggestion: add "(Markdown excepted, unless it is the manifest, the calling workflow or under a smoke-paths entry)", and say that smoke-paths entries are space-separated raw prefixes.
There was a problem hiding this comment.
Fixed in 84fd3e8. The contract paragraph now says Markdown is excepted only unless it is the manifest, the calling workflow file or under a smoke-paths entry, and that smoke-paths entries are space- or comma-separated prefixes normalised like the manifest path (a trailing slash kept for a directory).
| manifest_normalized = posixpath.normpath(env["MANIFEST"]) | ||
|
|
||
| def harness(f): | ||
| if f in (manifest_normalized, caller) or any(f.startswith(r) for r in roots): |
There was a problem hiding this comment.
smoke-paths entries are raw string prefixes, and a malformed one selects nothing without a message, so the run is green with nothing checked. Reproduced with tasks=changed on a pull request:
- root
.agent-eval/prompts(no trailing slash) with a change to.agent-eval/prompts-old/x.mdselects the smoke set; with.agent-eval/prompts/it selects[]. ./vendor/,.and/with a change undervendor/select[]. The manifest goes throughposixpath.normpath(:241), the roots do not.a/,b/(comma-separated, assmoke-tasksandexclude-tasksallow) matches nothing; the same two paths separated by a space match.
Suggestion: normalise each root with posixpath.normpath and split on [\s,]+ as ids() does, or document in the input description that entries need a trailing /, no ./, and spaces only.
There was a problem hiding this comment.
Fixed in 84fd3e8. smoke-paths entries are now split like ids() (whitespace or commas) and normalised with posixpath.normpath, keeping a trailing slash, so ./vendor/ and vendor/,other/ select the smoke set. The two regression tests fail on 279db0f. A root without a trailing slash stays a raw prefix, as the contract now says.
A smoke-paths entry with a ./ prefix or a comma separator matched nothing, so a change under it ran no task and the run stayed green. Entries are now split like the other id lists and normalised like the manifest path, with the trailing slash of a directory kept. The contract says so. Tests cover the calling workflow, the separators, unknown explicit and smoke task ids, and an unreadable previous push tip. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Re the review at 279db0f (test gaps, pushed in 84fd3e8):
|
|
Status against the comment, on the current head 84fd3e8:
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/docs/repo-evaluation-contract.md
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Part of #41 (step D8).
Summary
A reusable workflow that runs dokimi's no-LLM repository checks on the caller's checkout, with dokimi pinned. It replaces the hand-written self-check workflow a repository carries today (oneDAL's
agent-evals.ymlwas the model).Task selection (
tasks):Pinning:
dokimi-refmust be a 40-digit SHA or a tag (resolved to its commit once, invalidate); a branch is refused. Every job installsso the SHA pins the dependency closure too. Moving the pin edits the calling workflow, which selects the smoke set.
What a repository adds (oneDAL's after #3818 is trimmed):
Security:
permissions: {}at the top of both files,contents: readper job, actions pinned by SHA,persist-credentials: false, every input and event field reachesrun:throughenv:, nopull_request_target, no secrets.Evidence
Before: no reusable workflow; each repository copies the select/matrix/summary logic and calls its own harness.
After:
repo-eval-selfon this PR (this PR edits both workflow files, sochangedselects the smoke setfix-degrade-preandreview-pr-conventions): green on 056814f (run).Artifacts:
repo-eval-scan-dokimi,repo-eval-check-dokimi-<task>(check.json, grade.json, logs; 4 KB).Install: in a clean Python 3.12 venv,
pip install -c <lock@sha> "dokimi @ git+https://github.com/napetrov/dokimi@<this branch's sha>#subdirectory=dokimi"installs,dokimi repo validate --helpruns, and the resolvedlitellm,ragas,anthropicmatch the lock. The same works for a pull request's merge commit fetched from intel/dokimi, which is what the self-call installs.Selection, run locally on the step's script against
validate --jsonof this tree:.agent-eval/tasks/legacy-keys/task.json+.agent-eval/README.mdlegacy-keys.agent-eval/run.pyfix-degrade-pre legacy-keys(smoke)all, excludefeat-static-metric,docs-new-pagescheduleevent,changedlegacy-keys, use-repo-scan nosuch/ smokenosuchnosuchdokimi-ref: a SHA passes;main,v*anda;bare refused before anything installs.Locally,
repo validateOK (10 tasks),repo scan258 claims (0 fail),repo tasks checkonfix-degrade-pre,review-pr-conventions,use-repo-scan: 3/3 OK.zizmor --offline --min-severity medium .github/workflows(ci.yml's invocation, 1.30.1): no findings;actionlintclean; the docs tests (test_docs_code_refs,test_docs_links,test_docs_hooks) pass.Merge Danger
Door: two-way: two new workflow files and an appended doc paragraph; deleting them undoes it, and no caller exists yet.
Blast Radius: CI-only
.agent-eval/**,dokimi/dokimi/repos/**or either workflow get arepo-eval-selfrun: validate, scan, and one docker job per selected task (each builds the eval image and runs the suite twice, 4 to 15 min per task on this PR).smoke-tasksempty means every task, the safe default; a heavy repository should set it.Checklist
pytestpasses (no Python changed; the self-call run is the test, docs tests pass)ruff,mypy, and schema checks pass (no Python changed).github/.licenserc.yaml)🤖 Generated with Claude Code