Append persisted eval results across workflow runs - #50766
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the "state file" and the "append state file" are the same file. It's a JSONL where one simply appends new entries. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (only 1 addition in business logic paths). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
🟡 Changes recommended
The implementation breaks existing file-list semantics and can discard malformed historical records.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Updates eval persistence to preserve history and merge concurrent results.
Changes:
- Adds append-only JSONL merging and deduplication.
- Integrates merging into copy and rebase-conflict paths.
- Adds history and concurrency tests.
File summaries
| File | Description |
|---|---|
pkg/workflow/compiler_jobs_test.go |
Verifies generated eval-state configuration. |
actions/setup/js/push_experiment_state.test.cjs |
Tests append and deduplication behavior. |
actions/setup/js/push_experiment_state.cjs |
Implements append-only persistence and conflict merging. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| const appendFiles = new Set( | ||
| (process.env.GH_AW_STATE_FILES || "") | ||
| .split(",") | ||
| .map(name => name.trim()) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
Fixed in 51472ea. GH_AW_STATE_FILES remains the candidate file list, and append-only merge now applies only to additional *.jsonl files (excluding state.jsonl), so state.json replacement semantics are preserved.
| function mergeAppendOnlyJSONL(remoteContent, localContent) { | ||
| const merged = []; | ||
| const seen = new Set(); | ||
| for (const content of [remoteContent, localContent]) { | ||
| for (const line of content.split(/\r?\n/)) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) { | ||
| continue; | ||
| } | ||
| let entry; | ||
| try { | ||
| entry = JSON.parse(trimmed); | ||
| } catch { | ||
| core.warning(`mergeAppendOnlyJSONL: skipping unparseable line during merge`); | ||
| continue; | ||
| } | ||
| const key = stableJSONStringify(entry); | ||
| if (!seen.has(key)) { | ||
| seen.add(key); | ||
| merged.push(entry); | ||
| } | ||
| } | ||
| } | ||
| return merged.length > 0 ? `${merged.map(entry => JSON.stringify(entry)).join("\n")}\n` : ""; | ||
| } |
There was a problem hiding this comment.
Fixed in 51472ea. mergeAppendOnlyJSONL now preserves malformed nonblank lines as opaque entries (with a warning) instead of dropping them, while still deduplicating valid JSON records.
| const result = mergeAppendOnlyJSONL(remote, local) | ||
| .trim() | ||
| .split("\n") | ||
| .map(line => JSON.parse(line)); | ||
|
|
||
| expect(result.map(entry => entry.id)).toEqual(["shared", "remote", "local"]); |
There was a problem hiding this comment.
Fixed in 51472ea. Added concurrent-push integration coverage for evals.jsonl in actions/setup/js/push_signed_commits.test.cjs, validating resolver stage reads, append-file env handling, dedupe, and staged output in the signed-commit payload.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel 🧪SummaryThis PR contains no new or modified behavioral tests — only test cleanup and assertion adjustments to reflect production code refactoring. Files Analyzed
Test Coverage StatusExisting test suite (18 JS test cases):
Go tests:
AssessmentScore: N/A — Infrastructure Only No new behavioral test functions were added or modified. The PR refactors the test environment to match production code changes (consolidating The existing test suite provides comprehensive coverage of:
No violations — assertions include descriptive failure messages; test infrastructure is sound.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.4 AIC · ⊞ 5.3K
| } | ||
| } | ||
|
|
||
| for (const file of conflictedFiles.filter(name => appendFiles.has(name))) { |
There was a problem hiding this comment.
Potential double-processing of state.jsonl/state.json: If a user passes GH_AW_STATE_FILES=state.jsonl (or state.json), the file is handled first by the hardcoded block above (lines 280–289 or 268–278) using mergeExperimentStateJSONL/mergeExperimentStateJSON, and then immediately overwritten by this loop using mergeAppendOnlyJSONL. The two strategies behave differently (JSONL prunes stale runs; append-only does not), so the winner is whichever runs last.
Consider guarding the loop to skip files already handled by the hardcoded blocks:
for (const file of conflictedFiles.filter(name => appendFiles.has(name) && name !== "state.json" && name !== "state.jsonl")) {@copilot please address this.
There was a problem hiding this comment.
Addressed in 51472ea. state.jsonl is no longer in the append-only conflict set, so the generic append loop does not overwrite the dedicated state.jsonl merge result.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
- Double-merge bug: when
state.jsonlis inGH_AW_STATE_FILES(the default), the rebase conflict resolver writes it twice — first withmergeExperimentStateJSONL(pruning-aware), then overwritten bymergeAppendOnlyJSONL(append-only). One of the two merge strategies is silently lost. - Overly broad append scope:
appendFilesinmain()is identical tocandidateFiles, so every file inGH_AW_STATE_FILES— includingstate.jsonandassignments.json— is treated as append-only JSONL on copy. This is a semantic change to the env var that may break existing callers. - Missing integration-level test: the
main()merge-on-copy branch (line 476) has no test; onlymergeAppendOnlyJSONLitself is covered.
Positive Highlights
- ✅
mergeAppendOnlyJSONLis cleanly factored and handles unparseable lines gracefully - ✅ New tests cover both the >512-entry history preservation and concurrent-update deduplication paths
- ✅ Go compiler test correctly asserts the absence of
GH_AW_STATE_APPEND_FILESin generated YAML
| } | ||
| } | ||
|
|
||
| for (const file of conflictedFiles.filter(name => appendFiles.has(name))) { |
There was a problem hiding this comment.
[/diagnosing-bugs] state.jsonl may be merged twice if it appears in GH_AW_STATE_FILES. The existing block at line 280 already resolves state.jsonl conflicts with mergeExperimentStateJSONL; this new loop then overwrites that result with mergeAppendOnlyJSONL when state.jsonl is in appendFiles.
💡 Suggested fix
Filter appendFiles to exclude files handled by the specific-file blocks:
for (const file of conflictedFiles.filter(
name => appendFiles.has(name) && name !== "state.json" && name !== "state.jsonl" && name !== "assignments.json"
)) {Or remove the hard-coded state.jsonl block and let appendFiles drive it uniformly.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 51472ea. state.jsonl is excluded from append-only conflict handling, so it is merged only by the dedicated mergeExperimentStateJSONL path and not processed twice.
| .split(",") | ||
| .map(name => name.trim()) | ||
| .filter(Boolean) | ||
| ); |
There was a problem hiding this comment.
[/codebase-design] appendFiles in main() equals candidateFiles — every file in GH_AW_STATE_FILES is treated as append-only. The doc comment change on line 15 says all files are now JSONL to append, but the default value of GH_AW_STATE_FILES is state.jsonl,state.json,assignments.json. Non-JSONL files (state.json, assignments.json) would be passed to mergeAppendOnlyJSONL, which would parse them fine but silently ignore non-top-level-object keys via JSON.parse, producing unexpected behaviour.
💡 Suggested fix
Introduce a separate GH_AW_STATE_APPEND_FILES env var (or filter to .jsonl suffix by convention) so the append semantic is explicit and callers using the old default value are not broken. The PR description itself already references evals.jsonl as a single append file.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 51472ea. GH_AW_STATE_FILES is treated as the copy candidate list again, and append behavior is limited to non-state.jsonl *.jsonl files (for evals.jsonl), avoiding append-JSONL handling for state.json and assignments.json.
| const existingContent = fs.readFileSync(dest, "utf8"); | ||
| const newContent = fs.readFileSync(src, "utf8"); | ||
| fs.writeFileSync(dest, mergeAppendOnlyJSONL(existingContent, newContent), "utf8"); | ||
| } else { |
There was a problem hiding this comment.
[/tdd] The append-merge path in main() (merge-on-copy when the destination already exists) has no direct test. The new unit tests cover mergeAppendOnlyJSONL in isolation, but no test exercises the branch at line 476 where appendFiles.has(name) && fs.existsSync(dest) triggers. A failure here (e.g. permission error on readFileSync) silently falls through to core.setFailed rather than being caught at the unit level.
💡 Suggested test sketch
it("main: merges append-only files when destination exists", async () => {
// write existing content to workspaceDir/evals.jsonl
// write new content to stateDir/evals.jsonl
// run main() with GH_AW_STATE_FILES=evals.jsonl
// assert merged result in workspaceDir/evals.jsonl
});@copilot please address this.
There was a problem hiding this comment.
Addressed by adding integration coverage in 51472ea for the newly wired retry resolver path (evals.jsonl concurrent-push case in push_signed_commits.test.cjs). This verifies append-file env wiring and that the resolved file is staged into the commit payload.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Persisting
evals.jsonlreplaced the existing eval branch history on every run. New results should append without losing prior or concurrent entries.Persistence
evals.jsonlas append-only in the generated eval-state job.Concurrency
Coverage