mirror of
https://github.com/addyosmani/agent-skills.git
synced 2026-10-26 15:05:04 +00:00
fix(evals): remove stale grading.json when grading is rejected
RESULTS_DIR is only ever created and never cleared, and the output path is derived from skill name and eval id alone. When a run's grading is rejected (parseGrading returns null), the runner writes .grading.raw.txt but leaves any .grading.json from a previous successful run in place. A collector reading that directory gets the old score beside the raw output of the run that actually happened. Extract persistGradingOutcome so the write-and-cleanup is a testable unit, and unlink the stale .grading.json before writing the raw file.
This commit is contained in:
1 parent
c004a74784
commit
f646694706
2 files changed
+68
-5
No files matched your search
@@ -8,7 +8,7 @@ const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const test = require('node:test');
|
||||
const { materializeWorkspace, parseGrading } = require('./run-evals');
|
||||
const { materializeWorkspace, parseGrading, persistGradingOutcome } = require('./run-evals');
|
||||
|
||||
const RUNNER = path.join(__dirname, 'run-evals.js');
|
||||
|
||||
@@ -432,6 +432,59 @@ test('replaces paraphrased grader text with the declared expectation', () => {
|
||||
assert.equal(result.expectations.find((r) => r.id === 2).text, 'second expectation');
|
||||
});
|
||||
|
||||
// ---------- persistGradingOutcome stale-cleanup tests ----------
|
||||
|
||||
test('rejected grading removes a stale grading.json from a prior run', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grading-cleanup-'));
|
||||
try {
|
||||
const base = path.join(dir, 'my-skill.eval-1');
|
||||
// Simulate a previous successful run that left a .grading.json
|
||||
fs.writeFileSync(`${base}.grading.json`, '{"previous":"run"}\n');
|
||||
|
||||
const result = persistGradingOutcome(base, null, 'unparseable grader output');
|
||||
|
||||
assert.equal(result, false);
|
||||
assert.equal(fs.existsSync(`${base}.grading.json`), false, 'stale grading.json must be removed');
|
||||
assert.equal(fs.existsSync(`${base}.grading.raw.txt`), true, 'raw output must be written');
|
||||
assert.equal(fs.readFileSync(`${base}.grading.raw.txt`, 'utf8'), 'unparseable grader output');
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('rejected grading succeeds even when no prior grading.json exists', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grading-cleanup-'));
|
||||
try {
|
||||
const base = path.join(dir, 'my-skill.eval-1');
|
||||
|
||||
const result = persistGradingOutcome(base, null, 'bad output');
|
||||
|
||||
assert.equal(result, false);
|
||||
assert.equal(fs.existsSync(`${base}.grading.raw.txt`), true);
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('accepted grading writes grading.json', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'grading-cleanup-'));
|
||||
try {
|
||||
const base = path.join(dir, 'my-skill.eval-1');
|
||||
const grading = {
|
||||
expectations: [{ id: 1, text: 'x', passed: true, evidence: 'y' }],
|
||||
summary: { passed: 1, failed: 0, total: 1, pass_rate: 1 },
|
||||
};
|
||||
|
||||
const result = persistGradingOutcome(base, grading, 'unused');
|
||||
|
||||
assert.equal(result, true);
|
||||
const written = JSON.parse(fs.readFileSync(`${base}.grading.json`, 'utf8'));
|
||||
assert.deepEqual(written, grading);
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('materializes a git baseline and applies a working-tree patch', () => {
|
||||
const workspace = materializeWorkspace({ files: ['git-workflow-and-versioning'] });
|
||||
try {
|
||||
|
||||
+14
-4
@@ -473,6 +473,18 @@ function parseGrading(raw, expectations) {
|
||||
return g;
|
||||
}
|
||||
|
||||
function persistGradingOutcome(base, grading, raw) {
|
||||
if (!grading) {
|
||||
// Remove stale grading from a prior run so collectors don't read
|
||||
// it beside the raw output of the run that actually happened.
|
||||
try { fs.unlinkSync(`${base}.grading.json`); } catch { /* may not exist */ }
|
||||
fs.writeFileSync(`${base}.grading.raw.txt`, raw);
|
||||
return false;
|
||||
}
|
||||
fs.writeFileSync(`${base}.grading.json`, JSON.stringify(grading, null, 2) + '\n');
|
||||
return true;
|
||||
}
|
||||
|
||||
// Skill name must be a valid kebab-case identifier — no path separators,
|
||||
// no "..", no absolute paths. Without this, --behavioral "../../x" would
|
||||
// resolve to files outside the project tree for both reads and writes.
|
||||
@@ -558,13 +570,11 @@ function runBehavioral(skillName, dryRun) {
|
||||
const raw = execFileSync('claude', ['-p'], { input: graderPrompt, encoding: 'utf8', maxBuffer: 16 * 1024 * 1024, timeout: GRADER_TIMEOUT_MS });
|
||||
const grading = parseGrading(raw, ev.expectations);
|
||||
const base = path.join(RESULTS_DIR, `${skillName}.eval-${ev.id}`);
|
||||
if (!grading) {
|
||||
fs.writeFileSync(`${base}.grading.raw.txt`, raw);
|
||||
if (!persistGradingOutcome(base, grading, raw)) {
|
||||
console.log(` ✗ eval ${ev.id}: grader returned invalid JSON — raw saved to ${path.relative(ROOT, base)}.grading.raw.txt`);
|
||||
failures++;
|
||||
continue;
|
||||
}
|
||||
fs.writeFileSync(`${base}.grading.json`, JSON.stringify(grading, null, 2) + '\n');
|
||||
console.log(`eval ${ev.id}: ${grading.summary.passed}/${grading.summary.total} expectations passed -> ${path.relative(ROOT, base)}.grading.json`);
|
||||
if (grading.summary.passed < grading.summary.total) failures++;
|
||||
} finally {
|
||||
@@ -603,4 +613,4 @@ function main(args = process.argv.slice(2)) {
|
||||
|
||||
if (require.main === module) main();
|
||||
|
||||
module.exports = { materializeWorkspace, parseGrading };
|
||||
module.exports = { materializeWorkspace, parseGrading, persistGradingOutcome };
|
||||
Reference in new issue
Block a user