Repository navigation
feat(skill-evals): journal every live eval run and generate Run history from it - #1928
Conversation
github-actions
Bot
added
family:tools
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Generating the Run history table from an append-only journal is the right fix for hand-kept tables drifting. Two things need fixing before it can land: the unsandboxed eval wrapper now appends to whatever runs.jsonl points at, and two PRs that both record a run conflict on that file.
Commands outside the sandbox (tools/skill-evals/src/skill_evals/journal.py:140)
Commands outside the sandbox. Entries in
sandbox.excludedCommands(gh, the vetted-ops entry points) and any call run with the sandbox turned off are not filtered.
with journal.open("a", encoding="utf-8") as fh:
fh.write(line + "\n")
~/.claude/scripts/magpie-run-evals.sh runs the runner outside the sandbox. That is why it resolves its target and refuses anything outside evals/ ("refusing to run it unsandboxed"), and why the runner checks containment on every fixture read. The journal write gets no such check. append_record opens evals/<skill>/runs.jsonl in append mode and follows a symlink, so a runs.jsonl symlink the agent can create inside the tree (the tree is writable in the sandbox) makes the next wrapper run append one JSON line, unsandboxed, to any file the user can write. The line's content is partly agent-shaped: case labels come from directory names.
Suggested fix: Before appending, require that the resolved journal path stays under the resolved evals root and that runs.jsonl itself is not a symlink, and open it with O_NOFOLLOW; add a test with a planted symlink.
Flagged by both the primary and adversarial reviewers.
worth running on every change (tools/skill-evals/src/skill_evals/journal.py:132)
Self-eval — the authoring model grading itself — is a smoke test for the cheap failure class (invalid JSON, missing fields, off-spec shape, fixture / prompt drift) and is worth running on every change; run a cross-model pass for substantive changes (new steps, prompt restructures, behaviour changes that cross a classification boundary).
def append_record(journal: Path, record: dict) -> None:
"""Append one record as a single JSON line. Never rewrites the file."""
Every PR that runs a suite, which AGENTS.md asks for on every change, now appends a line to the end of that suite's tracked runs.jsonl. Two open PRs that both ran the same suite each add a line after the same last line, so the second to merge conflicts. Tried with plain git: two branches that each append one record to a one-line runs.jsonl fail with Automatic merge failed. Each conflict needs a hand resolution of an append-only file, and the README table has to be regenerated after it.
With runs.jsonl merge=union in the attributes, the same merge keeps both lines with no conflict; after a rebase the branch's records land after main's, so check --against origin/main still sees a prefix.
Suggested fix: Add tools/skill-evals/evals/*/runs.jsonl merge=union to .gitattributes, with a test that merges two appended journals and passes check --against.
Smaller observations
- See inline comments on
tools/skill-evals/src/skill_evals/journal.py:503,tools/skill-evals/src/skill_evals/journal.py:262,tools/skill-evals/src/skill_evals/runner.py:1620,tools/skill-evals/src/skill_evals/runner.py:1651,tools/skill-evals/src/skill_evals/runner.py:1601,tools/skill-evals/src/skill_evals/journal.py:211.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
3a0f66b to
594c743
Compare
|
Thanks Jarek. Fixed: the symlink write, the non-git crash, the case counts, manual-only rows, unrecorded-model rows keyed by CLI, merge=union for runs.jsonl, a deleted journal failing check --against, and the snapshot taken before the first case. I've kept journaling on by default, since recording every live run is the point of the change; --no-journal covers smoke runs. Wiring check --against into CI needs a base ref that exists in CI and locally, so I'd do that as a follow-up. |
…ry from it The Run history table in evals/<skill>/README.md was maintained by hand, so it could drift from what was actually run and no row could be audited back to a run. Replace the hand edit with an append-only journal. - Every `--cli` run whose path lies under tools/skill-evals/evals/<skill>/ appends one JSON record to that suite's runs.jsonl: skill, ref, branch, dirty flag, model, the exact --cli / --grader-cli strings, path and tag filters, per-outcome counts, the labels of every failed or errored case, cost (wall seconds, CLI calls, grader calls) and a --journal-note. --journal FILE, --journal-model NAME and --no-journal adjust it; a path outside the repository's evals tree is never journaled by default. - New `skill-eval-journal` CLI: `render` regenerates each README's Run history table from its journal (one row per date/ref/branch/model batch, newest first, `case failed k of n` per failing case); `check` verifies journals parse, tables match, and with --against <ref> that committed records were only appended to; `import-readme` migrates a hand-written table into legacy records. - The eight existing tables are imported; each renders back byte for byte. - `check-eval-run-history` prek hook fails a table that does not match its journal. - README "Recording runs" and the meta-and-quality-tooling spec describe the new rules. Generated-by: Claude Code (Fable 5.1)
…check The runs.jsonl journals are JSON Lines data with no comment syntax, so they cannot carry a licence header; the rat workflow flagged all eight. List them in .rat-excludes beside the existing *.jsonl exclusion. Generated-by: Claude Code (Fable 5.1)
…ding --against CodeQL flagged `root` in skill-eval-journal main() as possibly used before assignment: it is set inside a try whose except calls parser.error(), which exits but is opaque to the analyser. Declare both variables up front and raise after parser.error so the exit is explicit. Also refuse an --against value that starts with a dash: it is interpolated into `git show <ref>:<path>` and would otherwise be read by git as an option rather than a revision. Generated-by: Claude Code (Fable 5.1)
…hat graded them Review fixes: - The eval wrapper runs the runner outside the sandbox, and the journal write followed a symlink, so a planted runs.jsonl link could append a line to any file the user can write. append_record now refuses a symlinked journal, opens with O_NOFOLLOW, and the default journal must resolve inside the evals tree. - find_repo_root raises RuntimeError, not FileNotFoundError, so a suite run outside any git checkout died in the journal step after finishing its cases. Both lookups now catch it and skip the journal. - "failed k of n" used the batch's record count as n. Records now carry graded_cases and n counts the runs that graded that case; older records still count every case. A batch where nothing was graded renders as manual, not pass, and runs with no recorded model are keyed and shown by their CLI so two tools never share a row. Generated-by: Claude Code (Opus 5.5)
…hot before the first case Review follow-ups: - Two PRs that each recorded a run appended after the same last line and conflicted. .gitattributes now merges runs.jsonl with merge=union, which keeps both lines; a test merges two appended journals using the repository's own attribute line. - check --against skipped a journal missing from the working copy, so deleting a whole runs.jsonl passed the append-only check. It now fails when the ref has a journal the working copy lacks. - The git snapshot was taken after the last case, so a commit or branch switch during a long run was recorded instead of the checkout whose prompts were loaded. It is now taken before the first case. Generated-by: Claude Code (Opus 5.5)
…le permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
7a85231 to
c186044
Compare
The journal records the --cli and --grader-cli strings and is committed to a public repository, so a CLI spelled with an inline secret (ANTHROPIC_API_KEY=sk-… claude -p, tool --api-key …, -H 'Authorization: …') would have been published. Inline VAR=value assignments and the values of credential and header flags are now recorded as <redacted>; --model and the rest of the command are kept. Generated-by: Claude Opus 5.5
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — every blocking point from my review is fixed, and I checked each against the code rather than the reply: the journal write refuses a symlinked runs.jsonl, opens with O_NOFOLLOW and stays under the evals root (the eval wrapper never forwards --journal, so the explicit-path branch cannot be reached unsandboxed); a non-git suite no longer crashes after its cases; the snapshot is taken before the first case; a case's denominator is the runs that graded it; unrecorded-model rows are keyed by CLI; and runs.jsonl union-merges. Keeping journaling on by default, with --no-journal for smoke runs, is fine. I rebased the branch onto main and pushed one fixup commit, 76a0ea5 (please pull before pushing again): the journal records the --cli / --grader-cli strings and is committed to a public repository, so a CLI spelled with an inline secret (ANTHROPIC_API_KEY=sk-… claude -p, --api-key …, -H 'Authorization: …') would have been published. redact_cli now records inline VAR=value assignments and the values of credential and header flags as <redacted>, keeping --model and the rest; tested, and noted in the README.
Please open a tracking issue for wiring skill-eval-journal check --against into CI, so the append-only rule is enforced rather than documented. When this is squashed, the Copilot Autofix Co-authored-by: trailer and the Claude git authorship on the other commits are left out of the merge commit, per the project's no-AI-co-author rule.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
github-actions
Bot
added
the
family:dev
Review fixups, after moving the change onto main now that the journal (#1928) has merged: - flaky counted every record in a batch as a run of every case, so a --path / --tag filtered run or a manual-only run added to a case's n without grading it, and a case that failed every run that graded it was reported as flaky, at the top of the list. A case's denominator is now the runs that graded it, as in the Run history table: graded_cases for new records, every case for records from before the field, and the Result text's own n for legacy rows. - A batch with no recorded model is keyed on the CLI internally; the report now names that CLI ("(unrecorded, `claude -p`)", and "cli" in --json) instead of printing the internal key. - The README and --help say a batch is the same date, ref, branch and model. Generated-by: Claude Opus 5.5
Summary
evals/<skill>/README.mdwas hand-maintained, so it could drift from what was actually run and no row could be audited back to a run.--clieval run now appends one record (skill, ref, branch, model, exact CLI strings, inputs, per-case outcome, cost, notes) to an append-onlyevals/<skill>/runs.jsonl; the README table is generated from it by a newskill-eval-journalCLI and guarded by acheck-eval-run-historyprek hook.Type of change
tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)prek, workflows, validators)Test plan
prek run --files <changed files>passes (every hook exceptskill-token-count, which needs a tokenizer download the authoring sandbox blocked; noSKILL.mdis touched here)uv run --directory tools/skill-evals --group dev pytest: 100 passed (26 new intests/test_journal.py);ruff checkandruff format --checkcleanskill-eval-journal render tools/skill-evals/evals --checkandskill-eval-journal check tools/skill-evals/evalsboth exit 0 against the imported journals--clirun under the evals tree journals one record with the failed-case labels; a run outside the tree writes nothing;--no-journal,--journal FILEand--journal-modelbehave as documentedclaude -prun to confirm the record lands in a real suite (not possible in the authoring sandbox)RFC-AI-0004 compliance
git rev-parse/git statuslocallyLinked issues
Refs #1626 (the hand-maintained Run history convention this replaces).
Notes for reviewers (optional)
check --against origin/mainfails when a committed journal was edited or truncated, so a wrong record is corrected by appending another one.claude -pcarries no--model, so the record'smodelis null and the table shows(unrecorded)unless--journal-modelis passed.magpie-run-evals.shis deliberately left unchanged in this PR; wiring--journal-modelinto it would change the user-scope copy's hash.`sha` (branch), replacing the old "runs on a branch are not logged" rule, because a pre-merge run is exactly the evidence a reviewer wants.Generated-by:trailer.