feat(artifacts): verify, preserve, and attribute evaluation artifacts - #288
feat(artifacts): verify, preserve, and attribute evaluation artifacts#288Murali Chillakuru (mchillakuru) wants to merge 3 commits into
Conversation
finalize_artifact_plan records a sha256 for every output in file_hashes, but the cache activated a version on an existence check alone. An artifact altered after it was written - by an interrupted run, a stray editor save, or anyone with write access to the suite directory - was reused as though it were the computed result, and every score derived from it looked legitimate. _metadata_outputs_exist becomes _metadata_outputs_valid and now compares each output against its recorded hash. A mismatch is a cache miss, not an error: the stage recomputes, which is what the operator wanted anyway, and the mismatch is logged with the file name and both hash prefixes. Metadata without file_hashes predates hash recording and still passes on the existence check, so caches built by earlier versions are not discarded. ASSERT_SKIP_CACHE_VERIFY=1 skips hashing for very large artifacts, at the cost of an unverified cache. _is_local_edit already hashed the suite-root working copies, but that protects hand-edits from being overwritten and never checked the cached artifact itself.
… version Resume and --force-stage called unlink() on inference_set.jsonl and scores.jsonl. Those files are the evaluation evidence - every transcript, every verdict - and a config hash mismatch is not always what the operator intended, so an accidental edit cost a full re-run with no record that anything had been destroyed. archive_artifact() renames the file to <name>.<UTC timestamp>.bak beside the original and logs the path. Backups are never pruned automatically, because pruning evidence is the behaviour being fixed; ASSERT_DISCARD_STALE_ARTIFACTS=1 restores the previous delete. A failed rename falls back to removal with a warning rather than failing the stage. Only metrics.json carried a schema_version, so an artifact written by a newer ASSERT was consumed silently by an older viewer or analysis script. taxonomy.json and manifest.json now carry one inline, and the JSONL artifacts get a .schema.json sidecar. The sidecar is deliberate: a version header inside the JSONL would be parsed as a data record by any reader that does not know about it, including the previous version of ASSERT, so the stamp would corrupt the first row on revert. A sidecar is ignored harmlessly instead. Artifacts with no stamp predate it and are treated as compatible.
Stages hand work to each other through files, and no stage could tell whether its input came from the previous stage, from a cache hit, or from someone editing the file. Only system_prompt_sha crossed a stage boundary; the taxonomy, test set, inference set and scores carried no provenance at all. The schema sidecar added in the previous commit is extended rather than introducing a second mechanism: it now records the producing stage, the assert-ai version, the run id, the target model where relevant, and a sha256 of the artifact's bytes. verify_artifact_provenance() reports when a file no longer matches what its producer recorded, naming the stage. The version is read from package metadata rather than hardcoded, so a provenance record cannot claim a version the running code is not; the CLI's separate hardcoded 0.1.0 is left alone as a follow-up. core.artifact_cache had its own file_sha256 that read whole files into memory. Both now use one chunked implementation in core.io, so the cache digest and the provenance digest cannot drift apart. Unstamped artifacts predate this and still verify, so existing runs keep working.
Chang Liu (changliu2)
left a comment
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
The cache-hash check (commit 1) is a genuine improvement — it closes a case where a modified cached artifact was reused as if it were computed output. The preservation change is also straightforwardly better than unlink().
Two things need to change before this merges, and one claim in the description needs to be walked back.
-
verify_artifact_provenance()andcheck_artifact_schema()have no callers inassert_ai/— only intests/. The provenance sidecars are written by three stages and read by nothing. As it stands this commit records provenance; it does not verify it, and no stage can yet "tell whether its input came from the previous stage, a cache hit, or someone editing the file" as the description says. Either wire the check into the consumers (judge readinginference_set.jsonl, inference readingtest_set.jsonl) behind a warn-only path, or retitle the commit and drop the SEC-021 closure until a follow-up does the wiring. -
Sidecars go stale whenever the artifact they describe is mutated in place, and both mutation paths in this PR do exactly that:
archive_artifact()renamesscores.jsonlaside but leavesscores.jsonl.schema.jsonbehind, now describing bytes that are no longer there — and the preserved.bakcarries no provenance at all, which defeats the point of preserving evidence.- The judge and inference resume paths
append_jsonl_row()into an existing artifact and only rewrite the sidecar at the end ofrun(). A run that dies mid-append leaves a sidecar whose digest disagrees with the file. Once (1) is wired up, that surfaces as a false tamper report on a perfectly ordinary crash-and-resume.
Sidecars should move with the file inarchive_artifact(), and be deleted (or rewritten) before an artifact is appended to, not only after.
-
On the security framing: the digest lives next to the data, unsigned, and every fallback is permissive — missing
file_hashespasses, a non-str recorded hash is skipped,ASSERT_SKIP_CACHE_VERIFY=1skips everything, andartifact.jsonitself is not protected. Anyone with write access to the suite directory (the threat named in the description and in SEC-005) can edit the artifact and then edit or delete the recorded hash beside it. This is solid corruption/accident detection and I'd merge it as that. It is not tamper resistance, and I don't think SEC-005/SEC-021 should be marked closed on it.
The tests are good where they exist — the tamper, skip-flag, and legacy-metadata cases are the right ones. What's missing is any test that exercises these paths through an actual stage: every new test calls the helpers directly, so the fact that nothing calls the verifiers is invisible to the suite.
Inline notes
assert_ai/core/io.py:119 — verify_artifact_provenance (also check_artifact_schema, ~line 148). Dead code — callers only in tests/test_artifact_preservation.py. Fix: call verify_artifact_provenance(inference_set_path) in judge.run_judge after resolving its input, and on test_set.jsonl in inference, logging a warning on False. Or split into a follow-up and adjust the PR description.
assert_ai/core/io.py:173 — archive_artifact. The .schema.json sidecar is neither moved with the file nor removed. Fix: if path.with_name(path.name + ".schema.json") exists, rename it to backup.name + ".schema.json" alongside the artifact.
assert_ai/core/io.py:~200 — collision loop + path.rename(backup). The while backup.exists() loop is not atomic with the rename, and on POSIX rename silently overwrites the target. Two runs archiving the same suite artifact within the same second can destroy one backup. Fix: use os.link + unlink, or os.rename guarded by O_EXCL creation of the target, and retry on collision rather than pre-checking.
assert_ai/stages/judge.py:523 and assert_ai/stages/inference.py:1383. The sidecar is written only after run() completes, while the resume paths append to the existing artifact in place. A crash mid-append leaves a sidecar digest that no longer matches the file. Fix: delete the sidecar before entering the append/resume path.
assert_ai/core/artifact_cache.py:~875 — _metadata_outputs_valid. Verification covers the version-dir copy, but refresh_compatibility_files copies outputs to suite-root working copies, and _is_local_edit deliberately preserves a suite-root file whose contents differ. Consumers that read the suite root directly still read an unverified — specifically known-divergent — file. Fix: at minimum log the divergence; ideally verify the suite-root copy at read time.
assert_ai/core/artifact_cache.py:~880 — ASSERT_SKIP_CACHE_VERIFY and the file_hashes-missing fallback. Both are unauthenticated downgrades; artifact.json is not integrity-protected. Fix: log at WARNING (not silently return True) when verification is skipped for either reason.
assert_ai/core/io.py:~155 — check_artifact_schema. Returns False for an artifact stamped with an older version, but True for an unstamped (genuinely older) artifact — same situation, opposite answer, and the docstring says the return value means "safe to read." Fix: return True for older-but-known versions and warn, or document the return as "matches this build's version."
assert_ai/stages/systematize.py:131 — taxonomy_json.setdefault("schema_version", ...). Injects a key into the model's structured output before writing. TAXONOMY_SCHEMA declares additionalProperties: False, so the written file no longer validates against the schema it was generated under; it also invalidates every downstream cache entry once, worth calling out under "Risk and rollback." Fix: write the version into the sidecar as with the JSONL artifacts, keeping the artifact byte-identical to the schema it was generated against.
assert_ai/stages/test_set.py:1180 — write_artifact_schema(out_path, artifact="test_set"). No produced_by, so test_set.jsonl gets a schema stamp but no stage/version/run_id — inconsistent with inference and judge. It's also the one provenance-stamped artifact the cache copies between directories, and the sidecar is not in _OUTPUT_FILES, so it is not copied into the version dir and is not restored by refresh_compatibility_files. Fix: pass produced_by, and either include the sidecar in _OUTPUT_FILES for test_set or drop the sidecar during cache activation rather than leaving a stale one.
Verdict: Request Changes — the cache-integrity commit is worth merging, but the provenance commit ships verifiers that nothing calls and sidecars that go stale on the two paths this PR itself introduces, so the headline claim ("attribute") isn't yet true in running code.
Must fix before merge
- Wire up
verify_artifact_provenance/check_artifact_schema, or descope the commit and the SEC-021 closure claim. - Move/remove the sidecar in
archive_artifact. - Invalidate the sidecar before append/resume mutates an artifact.
- Correct the SEC-005/SEC-021 framing in the description — this is corruption detection, not tamper resistance.
Nice to have
- Atomic backup naming.
- Warn on skipped/unverifiable cache verification.
check_artifact_schemaolder-version return-value inconsistency.- Taxonomy
schema_versioninjection vs.additionalProperties: False, plus a note about the one-time cache invalidation. produced_byand cache handling for thetest_setsidecar.- One end-to-end test through a stage rather than only direct helper calls.
2 participants
Summary
Three commits.
Cache integrity.
finalize_artifact_planrecords a sha256 for every output, but thecache activated a version on an existence check alone. An artifact altered after it was
written — by an interrupted run, a stray editor save, or anyone with write access to the
suite directory — was reused as though it were the computed result, and every score
derived from it looked legitimate. A mismatch is now a cache miss, so the stage
recomputes, which is what the operator wanted anyway.
Preservation. Resume and
--force-stagecalledunlink()oninference_set.jsonland
scores.jsonl. Those files are the evaluation evidence, and a config hash mismatchis not always what the operator intended, so an accidental edit cost a full re-run with
no record that anything had been destroyed. They are now renamed aside with a timestamp.
Provenance. Stages hand work to each other through files, and no stage could tell
whether its input came from the previous stage, a cache hit, or someone editing the
file. A sidecar now records the producing stage, the assert-ai version, the run id, and
a digest of the artifact's bytes.
Closes: SEC-005 (cache reused without integrity verification) · SEC-021 (implicit inter-stage trust) · AP-010 (no schema version) · AP-013 (destructive resume)
Commits
Testing
Full suite green on this branch;
9 files changed, 612 insertions(+), 19 deletions(-). Every change has a regression test, and thesuite was re-run after each commit rather than only at the end.
Notes for reviewer
The schema stamp is a sidecar, not a header line, and that is deliberate. A version
header inside a JSONL file would be parsed as a data record by any reader that does not
know about it — including the previous version of ASSERT — so the stamp would corrupt the
first row on revert. A sidecar is ignored harmlessly instead.
Backups are never pruned automatically, because pruning evidence is the behaviour being
fixed.
ASSERT_DISCARD_STALE_ARTIFACTS=1restores the previous delete.Artifacts with no stamp predate this and are still treated as compatible, so existing
runs keep working.
core.artifact_cachehad its ownfile_sha256that read whole files into memory; bothnow share one chunked implementation so the cache digest and the provenance digest cannot
drift apart.
Risk and rollback
Each commit is a single concern and can be reverted independently. See the notes above
for anything that does not revert cleanly.