Hashing and rerun logic by AaronAspinwall123 · Pull Request #32 · responsibleai/ASSERT

Aaron Aspinwall (AaronAspinwall123) · 2026-05-08T20:20:29Z

No description provided.

Real correctness fixes: - _relative_to_suite: force POSIX separators on the os.path.relpath fallback so manifests written on Windows for paths outside the suite root no longer mix \ and / separators. - rollout.run_rollout: never overwrite a seed file that lives under <suite>/artifacts/<stage>/v####/ even when callers pass rewrite_seed_path=True; tightened the wrapper in rollout.run() so rewriting only happens when there is no cached artifact ref. - activate_latest_artifacts: when latest.json points at a missing or partially-written artifact directory, emit a warning to stderr and recover to the most recent intact version directory for that stage. Maintainability: - Restructured the runner stage-selection block as if/elif on module.SCOPE so the legacy file-exists path is no longer guarded by the negation of the cache predicate. - artifact_ref / finalize_artifact_plan: dropped the duplicated relative_path / relative_metadata_path keys (canonical forms are now path / metadata_path); omit concept_hash when null. Readers stay tolerant of the legacy aliases. - Expanded the artifact_cache module docstring to describe the layout, sidecar schema, latest.json, and reuse contract. Test coverage added: - _relative_to_suite POSIX fallback. - Round-trip reuse: v0001 -> v0002 (config change) -> revert -> reuse v0001. - concept_hash absent for design/seeds. - artifact_ref no longer emits relative_path aliases. - activate_latest_artifacts recovery and silent-skip behavior. - Force-stage policy and design cascade. - Reuse path restores legacy compatibility files at the suite root. - run_rollout refuses to mutate a versioned seed artifact. - Viewer loadSuiteSnapshot skips the suite-level artifacts/ cache dir. All 551 tests pass; viewer svelte-check is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Five issues raised in the Copilot Code Review and confirmed by manual inspection. All 560 Python tests pass (+9 new) and viewer `npm run check` reports 0 errors / 0 warnings. #1 (security, viewer/src/lib/server/artifacts.ts): manifestRelativePath now filters out '..' parts and manifestArtifactPath returns null when the path would escape the suite directory; runSeedRows already falls back to the legacy seeds.jsonl in that case. #2 (bug, viewer/src/lib/server/artifacts.ts): listRunIds now also filters entries through isSafeArtifactId so the run list cannot include names that requireSafeId would later reject with a 500. #3 (critical, p2m/runner.py + p2m/core/artifact_cache.py): when artifact caching is active, the runner now overrides save_dir/save_path in raw_cfg via override_cacheable_output_paths so user YAML cannot redirect cacheable stage outputs outside the versioned artifact dir. finalize_artifact_plan would otherwise fail to find the outputs. #4 (security, p2m/viewer_read_model.py): _manifest_relative_path now rejects '..' segments and returns None; _seed_artifact_path falls back to suite_dir/seeds.jsonl when the helper rejects the manifest path. #5 (operational, p2m/core/artifact_cache.py): _load_json_object now catches json.JSONDecodeError and OSError, prints a stderr warning, and returns None so a corrupt latest.json/artifact.json degrades to a cache miss instead of aborting the pipeline. _resolve_ref_path also rejects '..' segments as defense in depth. New tests: - test_artifact_cache.py: corrupt JSON gracefully ignored, non-object payload rejected, _resolve_ref_path traversal rejection, override_cacheable_output_paths for policy save_dir, seeds save_path, and unknown stage no-op. - test_runner_artifact_cache.py: end-to-end test that user-supplied save_dir/save_path in raw_cfg is overridden so artifacts still land in the versioned cache directory. - test_viewer_server_artifacts.py: _manifest_relative_path traversal rejection (in both the existing TS-gated class and a new always-run ViewerReadModelHelpersTest class) and _seed_artifact_path fallback on malicious manifests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Four follow-up issues raised by Copilot on commit cc96e06. All 561 Python tests pass (+1 net new) and viewer `npm run check` reports 0 errors / 0 warnings. #1 (moderate, bug, p2m/core/artifact_cache.py): activate_latest_artifacts now detects whether the artifact_dir or metadata_path in latest.json was stale (resolved path missing) and, when it is, rebuilds the ref via _ref_from_metadata using the on-disk paths and persists the corrected ref through update_latest. Without this, ctx and downstream run manifests silently propagated paths that no longer exist. #2 (critical, security, viewer/src/lib/server/artifacts.ts): manifestArtifactPath now refuses absolute paths from manifest.json (with a console warning) and returns null, so runSeedRows falls back to the legacy seeds.jsonl. Without this, an absolute path bypassed the relative '..' defense. #3 (critical, security, p2m/viewer_read_model.py): _seed_artifact_path now refuses absolute paths from manifest.json (with a stderr warning) and falls back to suite_dir/seeds.jsonl, matching the TS fix. #4 (moderate, maintainability, tests/test_viewer_server_artifacts.py): removed the duplicated _seed_artifact_path traversal test from the TS-gated ViewerServerArtifactsTest; the always-run ViewerReadModelHelpersTest is the single home for these helper assertions and now also covers the absolute-path defense. New test: - tests/test_artifact_cache.py: test_activate_latest_rebuilds_ref_when_recorded_paths_are_stale - regression for #1; mutates latest.json to point at MISSING paths and asserts both ctx and the persisted latest.json are updated to the on-disk version directory. - tests/test_viewer_server_artifacts.py: ViewerReadModelHelpersTest.test_seed_artifact_path_rejects_absolute_paths - regression for #3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

… a directory Two follow-up Copilot comments on commit a9115ad. Both identify a real edge case I introduced in round 2 when `_manifest_relative_path` / `manifestRelativePath` started returning `base_dir` for normalized-empty parts. A manifest path of `"."`, `"./"`, or `"/."` would resolve to the suite directory itself, then the JSONL reader would EISDIR. #1 (moderate, security, viewer/src/lib/server/artifacts.ts): manifestRelativePath now rejects paths that normalize to no segments and returns null, so runSeedRows falls back to the legacy seeds.jsonl. #2 (moderate, security, p2m/viewer_read_model.py): same fix in Python; _manifest_relative_path returns None for normalized-empty paths and _seed_artifact_path falls back to suite_dir/seeds.jsonl. New test: - tests/test_viewer_server_artifacts.py: ViewerReadModelHelpersTest.test_seed_artifact_path_rejects_paths_that_normalize_to_directory exercises `"."`, `"./"`, `"/."`, `"./."`, `"././"` and asserts both _manifest_relative_path returns None and _seed_artifact_path falls back to suite_dir/seeds.jsonl. Validation: 562 passed (+1 new), 14 skipped; viewer npm run check 0 errors / 0 warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds a suite-level artifact cache with versioned outputs (policy/design/seeds), records resolved artifact references into run manifests, and updates the viewer/read-model pipeline to consume versioned seed artifacts safely (including path-traversal defenses).

Changes:

Introduce p2m.core.artifact_cache to version and reuse suite-scoped artifacts via stable hashing and latest.json.
Record artifact_versions into manifest.json (and artifacts.json) and wire runner/stages to honor cache-managed output paths.
Update viewer (TS + Python read model builder) to load seeds from manifest-selected artifact paths, with directory-escape protections; add broad test coverage for reuse/regressions.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
viewer/src/lib/types.ts	Extends viewer manifest typing to include `artifact_versions`.
viewer/src/lib/server/artifacts.ts	Loads versioned seed artifacts from manifest; filters suite run IDs to exclude suite-level `artifacts/`.
tests/test_viewer_server_artifacts.py	Adds tests for manifest-selected seed artifacts and run ID filtering; adds Python-only regression tests for path traversal defenses.
tests/test_runner_artifact_cache.py	Adds runner-level integration tests for artifact reuse/versioning and forced-stage cascade behavior.
tests/test_rollout_stage.py	Adds tests ensuring rollout never mutates versioned seed artifacts (even when rewrite is requested).
tests/test_artifact_cache.py	Adds unit tests for hashing, plan selection, latest recovery, path resolution, and output-path overrides.
p2m/viewer_read_model.py	Selects seeds path from `manifest.json` with absolute/path-traversal defenses.
p2m/stages/seeds.py	Prefers context-provided artifact paths (from cache activation) when resolving stage inputs/outputs.
p2m/stages/rollout.py	Prevents rewriting immutable versioned seed artifacts; hardens progress logging against `OSError`.
p2m/stages/policy.py	Uses cache-provided artifact directory when present.
p2m/stages/judge.py	Prefers context-provided `policy_path` to follow cached policy artifacts.
p2m/stages/design.py	Prefers context-provided `policy_path` and cache-provided design artifact directory when present.
p2m/runner.py	Integrates artifact cache planning/activation/finalization; records artifact refs into run manifest + sidecar; hardens progress logging.
p2m/core/config_model.py	Adds `artifact_versions` to `RunManifest` and omits it from serialized output when empty.
p2m/core/artifact_cache.py	New module implementing artifact versioning, hashing, `latest.json` management, and legacy compatibility file refresh.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

… disk Resolves Copilot review (round 4): activate_latest_artifacts indexed output_paths[next(iter(_OUTPUT_FILES[stage_name]))] under the assumption that _metadata_output_paths returned every expected key, but it only returned what metadata['files'] listed (with the canonical default as a fallback only when the entire dict was empty). A corrupt or legacy artifact.json missing the primary key (e.g. policy.json) would pass _metadata_outputs_exist (which only checked files mentioned in metadata) and then trigger a KeyError during recovery. _metadata_output_paths now starts from _output_paths(stage_name, artifact_dir) and overlays metadata-provided filenames so the returned dict is always a superset of the canonical keyset. _metadata_outputs_exist now takes stage_name and verifies every expected output exists on disk via the merged path map, so a tampered metadata that omits the primary key cannot trick the cache into activating an artifact whose primary file is missing. Threads stage_name through _latest_matching_metadata and _recover_latest_valid_version. Adds two regression tests covering recovery with a partial metadata.files and refusal to activate when the canonical primary file is absent on disk. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Resolves conflicts in p2m/runner.py and p2m/stages/rollout.py introduced by main's centralized-logging refactor (PR #22) and policy/rollout error-handling improvements (PR #24). p2m/runner.py: kept the artifact-cache stage path (prepare_artifact_plan / activate_artifact_plan / override_cacheable_output_paths) and combined it with main's logging style. The two stage-skip messages now use log.info with the new '[stage] Skipped' prefix instead of the deleted _progress() helper, and _progress() itself was removed since main's logging configuration writes to sys.__stderr__ via RichHandler, neutralizing the original Phoenix sys.stderr-wrapping concern. p2m/stages/rollout.py: dropped the sys.__stderr__ rollout progress writer in favor of main's log.info / log.warning calls (same rationale). Kept the 're' import that the cache code uses for _VERSIONED_ARTIFACT_RE; dropped the now-unused 'sys' import. Verified: pytest passes for tests/test_artifact_cache.py, tests/test_runner_artifact_cache.py, tests/test_runner_progress.py, and tests/test_viewer_server_artifacts.py (56 passed, 2 skipped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Jake Present (jakepresent)

Real piece of work, the versioned cache + hash chain is the right structure. Two concerns I'd want addressed before this lands:

The Copilot findings on _metadata_outputs_exist and _metadata_output_paths are legitimate. metadata['files'] should be validated as basenames before use, and activate_latest_artifacts should fall back when the primary output key is missing from metadata - both are real failure modes, not theoretical.
If a stage fails after prepare_artifact_plan has allocated v#### but before finalize_artifact_plan runs, we leave a partial version directory with no sidecar. Subsequent runs handle this fine (the sidecar absence skips it), but it's a disk leak. Worth either cleaning up on failure or adding a p2m results gc later.

Two non-blocking observations:

The concept hash is only computed for the policy stage; downstream stages pick it up transitively through the dependency chain. That's correct but assumes dependency chain coverage. A comment in _stage_descriptor about why this is safe would help.
override_cacheable_output_paths silently overrides save_dir / save_path from user YAML. Should log when overriding so customers don't get confused why their save_dir: ~/runs/foo is ignored.

Tests look solid. Approving once the Copilot findings are addressed.

…user save_dir override Addresses Jake's review on the artifact-cache PR. 1. Disk-leak on stage failure (Jake's #2): when a cacheable suite stage failed after prepare_artifact_plan allocated vNNNN/ but before finalize_artifact_plan wrote the sidecar, the partial directory stayed on disk forever. _next_version kept incrementing past abandoned slots and the stage_root accumulated dead version directories on every failed run. Adds discard_artifact_plan(ctx, plan) to remove the version directory and pop the orphaned ctx['artifact_versions'] entry. Wired into runner.py's stage exception path. No-ops for reused plans so a downstream failure cannot blow away a healthy upstream cache hit. latest.json is left untouched (finalize is its only writer for non-reused plans, so a discarded plan never touched it). 2. Silent override of user save_dir (Jake's non-blocking #2): override_cacheable_output_paths now log.warnings whenever it replaces a user-supplied save_dir/save_path with the versioned cache location. Customers who set save_dir in YAML get a clear actionable message instead of seeing their value silently ignored. No warning when no user value was set. 3. Concept-hash transitive flow (Jake's non-blocking #1): added a comment in _stage_descriptor explaining that concept_hash is computed only for policy and propagates through the dependency chain to design and seeds via _dependency_descriptor. Notes the safety invariant (every cacheable stage must depend on its upstream) so a future stage that breaks the chain triggers a code-review flag rather than silent stale-cache reads after a concept edit. Note: Copilot findings on _metadata_outputs_exist / _metadata_output_paths primary-key KeyError and basename validation (Jake's #1) were already addressed in de9a32a (round 4) — _metadata_output_paths now overlays metadata onto the canonical _output_paths default keyset and _metadata_outputs_exist verifies every expected file exists on disk via the merged path map. _is_safe_artifact_basename rejects unsafe filenames before they become Path components. Tests: +7 regression tests (4 for discard helper covering missing dir, reused plan, ctx cleanup, version-slot reuse; 2 for override warning behavior; 1 runner integration test that fails design mid-stage and asserts vNNNN is cleaned up plus next run reuses the freed slot). Verified: 585 passed, 14 skipped (2 pre-existing Windows logging tempdir flakes from main's PR #22 deselected). npm --prefix viewer run check: 0 errors / 0 warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Chang Liu (changliu2) · 2026-05-08T22:36:58Z

Audit (PR-merge readiness)

Pulled pr-32 locally and ran the full suite + diff review against origin/main (merge-base da841a8, merges cleanly). Net: design is solid, recommend approve with one ask before merge.

Tests: 73 new artifact-cache tests pass; 578 passed / 14 skipped on the full suite. The 2 test_logging_config.py failures are pre-existing on main (Windows tempfile cleanup, introduced by #22), not from this PR.

False alarm I want to flag so reviewers don't chase it: the rollout.py:1018 change from "\u2713" to "✓" looks scary in git diff (mojibake on cp1252 terminals) but the on-disk bytes are clean UTF-8 (E2 9C 93). Same Python string, just a literal-vs-escape stylistic change. Worth a squash-on-merge so it doesn't confuse future reviewers — see (5) below.

P1 — please address before merge

Race in _next_version (p2m/core/artifact_cache.py:675). Two concurrent p2m run invocations on the same suite both read max(numbers) + 1, both pick v0001, second clobbers first. No file lock or mkdir(exist_ok=False) retry. Single-process is fine, but anyone running parallel pipelines on a shared suite (e.g. CI matrix, batch sweeps) will silently corrupt each other's artifacts and latest.json. Cheap fix: wrap the allocation in a small loop that does mkdir(exist_ok=False) and bumps the version on FileExistsError. Alternatively, document "single-process per suite" as a constraint.
refresh_compatibility_files silently overwrites suite-root files (artifact_cache.py:371). shutil.copy2(path, suite_root / path.name) runs unconditionally on every reuse and finalize. A user who hand-edits <suite>/policy.json between runs has it silently destroyed the next time they hit a cache. At minimum, hash-check the destination before copy and warn if it diverges from the cached version (which is the user's signal that they've edited it).

P2 — minor, can be follow-ups

_PROMPT_FILES is hardcoded (artifact_cache.py:83). Only the named default templates are hashed. If a stage config points at a custom prompt file, the cache won't invalidate when that file changes. Either scan the stage config for *_path keys ending in .md and include their hashes, or add a doc note.
_record_run_artifacts runs 3+ times per pipeline (runner.py:438, 481, 517). Idempotent and tiny, but wasteful. Could move to a single call at the end of run_pipeline since artifact_versions is fully resolved before any run-scope stage starts.
Squash on merge. History has 10 commits including 4 "Address Copilot review round N" + 2 "Potential fix for pull request finding" + the cosmetic \u2713→✓ change mixed in. A squash would give us one clean entry against main and remove the false-alarm-bait.

Things I checked that look correct

_metadata_outputs_exist requires both policy.json AND systematization.json for a policy cache hit. Verified run_systematization writes its file unconditionally, so this won't permanently miss.
manifest is None guard at the end of run_pipeline (runner.py:514) is in place — the new final _record_run_artifacts call is reachable only when manifest is non-None.
Viewer listRunIds correctly filters out the new artifacts/ directory so it doesn't show up as a phantom run.
rewrite_seed_path's two-layer protection (the boolean from absent artifact_versions["seeds"] PLUS the _is_versioned_seed_artifact_path check inside run_rollout) prevents clobbering versioned cache files. Backwards-compatible with users who supply their own seed file.
Path-traversal defense is layered in three places (_resolve_ref_path, _seed_artifact_path in viewer_read_model.py, manifestArtifactPath in viewer/src/lib/server/artifacts.ts) — relative .. rejected, absolute paths must resolve inside suite. Good.

Branch state

PR is rebased/merged with main via the merge commit ee30386 and git merge-tree --write-tree exits clean — GitHub merge will succeed. No action needed there.

…e-root copies Two concurrency/data-loss findings from review (round 6). 1) Race in _next_version (silent corruption on concurrent runs) _next_version read max(vNNNN) + 1 with no atomic reservation, and the chosen directory was created lazily by the stage's own writes (or by finalize_artifact_plan, both with exist_ok=True). Two p2m run invocations on the same suite (CI matrix, batch sweeps) could both pick the same slot, both write into it, and silently corrupt each other's outputs and artifact.json sidecars. Replaced with _allocate_version_dir(stage_root) -> (version, artifact_dir) which loops up to _MAX_VERSION_ALLOCATION_RETRIES=100 calling mkdir(parents=False, exist_ok=False) on the candidate path. On FileExistsError it rescans and retries with the new max. On exhaustion it raises RuntimeError loudly rather than misallocating silently. The atomic reservation also tightens discard_artifact_plan's safety guarantee: each non-reused plan now provably owns its artifact_dir, so rmtree on failure can never wipe a sibling process's work. Updated the discard docstring to reflect this. Note in the docstring that this only fixes allocation; update_latest's read-modify-write on latest.json and refresh_compatibility_files's suite-root copies remain last-writer-wins. For fully concurrent pipelines on the same suite, run each in its own suite directory. 2) refresh_compatibility_files silently destroys hand-edited suite-root files shutil.copy2 ran unconditionally on every reuse, finalize, and activate-latest path. A user who hand-edited <suite>/policy.json between runs had it silently overwritten on the next cache hit. Now the destination is hashed and only overwritten when either (a) it matches the source content or (b) it matches a previously cached version's recorded hash for the same filename. Branch (b) is what keeps --force-stage <stage> working transparently: when a fresh vNNNN is produced and the suite-root copy still holds the prior version's content (because the user did not edit it), the prior version's file_hashes recognize it as cache-derived and the new content overwrites cleanly. When neither check passes, log.warning explains the situation and names the file, the cached source path, and the --force-stage remediation. New helpers: _is_local_edit, _was_cached_artifact. Tests: - AllocateVersionDirTest: empty stage_root -> v0001; existing v0001/v0002 -> v0003; race-injected sibling pre-creates v0001 between scan and mkdir -> retry picks v0002; pathological always-collide -> RuntimeError with diagnostic message. - RefreshCompatibilityFilesTest: copies when destination missing; no-op when destination matches source (no spurious warning); preserves user edit and warns; --force-stage path overwrites when destination matches prior cached version; per-file isolation across multi-output stages. - Updated existing DiscardArtifactPlanTest.test_discard_handles_missing_directory_silently to reflect that prepare now creates the dir atomically (we explicitly rmtree before discarding to exercise the missing-dir branch). - Updated docstring/comment references to _next_version -> _allocate_version_dir. Validation: 594 passed, 14 skipped, 13 subtests (2 pre-existing test_logging_config Windows flakes deselected). Viewer check: 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Chang Liu (changliu2) · 2026-05-09T00:05:32Z

Re-audit (post `d12e7de` + `ba31928`)

Pulled the latest two commits, re-ran tests, and verified both P1 fixes against the round-1 audit.

Tests: 594 passed / 14 skipped (+16 new tests since round 1; the 2 deselected test_logging_config.py Windows tempdir failures are pre-existing on main from #22, unchanged here).

P1 (1) Race in `_next_version` → ✅ fixed

Replaced with _allocate_version_dir(stage_root):

Loops up to _MAX_VERSION_ALLOCATION_RETRIES=100, calls mkdir(parents=False, exist_ok=False) on the candidate.
On FileExistsError, rescans and retries (handles both "concurrent allocator beat us" and "leftover empty dir from a crashed run").
On exhaustion, raises RuntimeError with diagnostic context (loud failure, not silent misnumbering).
Docstring is honest about the remaining last-writer-wins gaps in update_latest and refresh_compatibility_files, and recommends "one suite directory per concurrent pipeline" as the strong-isolation answer. That's the right scope.

Also tightens discard_artifact_plan's safety: each non-reused plan now provably owns its artifact_dir (reserved by atomic mkdir), so failure-path rmtree can never wipe a sibling process's work.

Test coverage (AllocateVersionDirTest):

Empty stage_root → v0001
Existing v0001/v0002 → v0003
Race injection: monkeypatched _iter_version_dirs reports empty on the first scan and pre-creates v0001 mid-call so our mkdir raises FileExistsError; loop rescans and picks v0002. (test_retries_when_candidate_dir_exists)
Pathological collisions: monkeypatched mkdir to always FileExistsError → RuntimeError. (test_raises_after_exhausting_retries)

P1 (2) `refresh_compatibility_files` silent clobber → ✅ fixed

New _is_local_edit(suite_root, stage_name, dest, source) runs before every overwrite. Conservative: returns False (safe to copy) when ANY of (a) dest missing, (b) dest not regular file, (c) dest_hash == source_hash, or (d) _was_cached_artifact(dest_hash) is True. Only when all four fail do we conclude "user hand-edited the suite-root copy", log a warning naming the file + cached source + --force-stage remediation, and skip.

The (d) check (walking every vNNNN/artifact.json for matching file_hashes) is what keeps --force-stage <stage> transparent: when a fresh vNNNN is produced and the suite-root copy still holds the prior version's content, the prior version's recorded hash recognizes it as cache-derived and the new content overwrites cleanly. Nice — that's the subtle case I would have asked about, and the doc and the test both call it out explicitly.

Test coverage (RefreshCompatibilityFilesTest):

Copies when destination missing
No-op when destination matches source (no spurious warning)
Preserves user edit and warns — the actual P1 case
--force-stage path overwrites when destination matches prior cached version — the transparency case
Per-file isolation across multi-output stages

Bonus fixes from `d12e7de` (Jake's review)

Disk-leak on stage failure → discard_artifact_plan wired into the runner exception path. No-ops for reused plans (correctly: a downstream failure must not blow away a healthy upstream cache hit). latest.json left alone (correctly: finalize_artifact_plan is its only writer for non-reused plans, so a discarded plan never touched it). Tests cover missing-dir, reused-plan no-op, ctx cleanup, and version-slot reuse after discard.
Silent override of user save_dir → override_cacheable_output_paths now log.warnings per overridden key with stage, key, user value, cache location, and --force-stage remediation. Skipped when no user value was set. Tests cover both branches.
Concept-hash transitive flow → comment in _stage_descriptor documents that concept_hash is computed only for policy and propagates to design/seeds via _dependency_descriptor, with the safety invariant ("every cacheable stage must depend on its upstream") spelled out so a future cacheable stage that breaks the chain trips a code-review flag rather than silently reusing stale outputs after a concept edit.

Carried forward from round 1 (P2, fine as follow-ups)

_PROMPT_FILES is still hardcoded — custom prompt paths supplied via stage config won't invalidate the cache. Documentation note would suffice.
_record_run_artifacts still writes 3+ times per pipeline. Idempotent and tiny; not worth blocking.
History is now 12 commits including 4 "Address Copilot review round N" + 2 "Potential fix for pull request finding" + the cosmetic \u2713→✓ change. Still recommend squash on merge — gives main one clean entry and removes the false-alarm-bait diff.

Verdict

P1 issues from round 1 are fully addressed with code, comments, AND focused regression tests. LGTM, ship it. Squash-on-merge if you can.

Chang Liu (changliu2) · 2026-05-11T14:05:32Z

Round 3 — re-audit after `main` advanced (PR #27 + PR #26 merged)

PR head unchanged at ba31928. Verified the would-be merged state still ships clean:

Merge: git merge-tree --write-tree origin/main pr-32 exit 0. Local test-merge of pr-32 into origin/main (26dd5af) succeeds with no conflicts.
Tests on merged tree: 594 passed, 14 skipped, 2 deselected, 13 subtests passed in 42.93s — identical to round 2. The 2 deselects are the pre-existing Windows logging flakes from PR feat: centralized logging with --verbose/--quiet/--log-file/--output json CLI flags #22 (also fail on main, unrelated).
Ruff: 12 errors, all pre-existing on main — none introduced by this PR.
Pyright: 6 errors in artifact_cache.py are typing-narrowing nits (e.g., Optional[Path] not narrowed through ternary branches; one dataclass asdict ClassVar quirk). Not runtime bugs. No CI gate on pyright today, so non-blocking — worth a follow-up cleanup if/when pyright gets enforced.

Verdict unchanged: ship it (squash-on-merge). Nothing in the recent main advance affects this PR's correctness.

tangym · 2026-05-11T17:05:49Z

+                print(
+                    f"[artifact-cache] warning: latest.json {stage_name} entry "


Nit: change print(file=sys.stderr) to log.warning()

PR #22 moved all warning/error output to log = logging.getLogger(__name__) so --verbose/--quiet/--log-file/--output json work uniformly. This module already uses log.warning() in discard_artifact_plan and refresh_compatibility_files, but several other sites still use print(..., file=sys.stderr) which bypasses log level filtering, --quiet, and JSON output mode. Switching these to log.warning() with structured placeholders (like the existing calls) would keep the module consistent with the rest of the codebase.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

+function manifestRelativePath(baseDir: string, rawPath: string): string | null {
+	const parts = rawPath.split(/[\\/]+/).filter((part) => part.length > 0 && part !== '.');
+	if (parts.length === 0) {
+		console.warn(
+			`[viewer] refusing manifest path that normalizes to no segments: ${rawPath}`
+		);
+		return null;
+	}
+	if (parts.some((part) => part === '..')) {
+		console.warn(
+			`[viewer] refusing manifest path with parent-directory segments: ${rawPath}`
+		);
+		return null;
+	}
+	return path.join(baseDir, ...parts);
+}
+
+function manifestArtifactPath(suiteDir: string, rawPath: unknown): string | null {
+	if (typeof rawPath !== 'string' || rawPath.length === 0) return null;
+	if (path.isAbsolute(rawPath)) {
+		// A tampered or corrupted manifest.json must not be able to redirect
+		// viewer reads outside the suite directory via an absolute path,
+		// which would otherwise bypass the relative-path '..' defense.
+		console.warn(`[viewer] refusing absolute manifest artifact path: ${rawPath}`);
+		return null;
+	}
+	return manifestRelativePath(suiteDir, rawPath);
+}


 	return path.join(runDir, VIEWER_CACHE_DIR, fileName);
 }

+function manifestRelativePath(baseDir: string, rawPath: string): string | null {


+	return path.join(baseDir, ...parts);
+}


+	artifact_versions?: Record<string, {
+		version?: string;
+		path?: string;
+		relative_path?: string;
+		artifact_dir?: string;
+		metadata_path?: string;
+		relative_metadata_path?: string;
+	}>;


        target_model = target_obj.model.name or ""
    return {
        "transcripts_path": result["transcripts_path"],
+        "seed_artifact_version": seed_artifact_ref,


…ng the main merge Earlier merge commit (81258cf) brought main into terminology-migration-pr21 and resolved the 11 base-conflict files. This commit lands the follow-on rename sweep that the merge surfaced but didn't itself apply, so the rename is consistent across the merged code: * run_systematization_to_policy -> run_systematization_to_taxonomy * "Rollout worker" log strings -> "Inference worker" * tests/test_exception_handling.py: JudgePolicyParseErrorTest -> JudgeTaxonomyParseErrorTest; RolloutWorkerLoggingTest -> InferenceWorkerLoggingTest; imports updated to TesterConfig / InferenceConfig / run_inference / run_systematization_to_taxonomy. Conflict-resolution rule (uniform): take main's structure/logic (try/except, ctx fallbacks, runSeedRows helper, rewrite_seed_path logic), keep HEAD's renames (taxonomy_path, taxonomy_json, inference, tester, spec). 613 unit tests pass with the 4 known Windows-only flakes deselected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Absorb second wave from main: PR #34 (exception handling) plus PR #32 trailing commits (artifact-cache atomic version + manifest path validation, rewrite_seed_path inference flag). Apply renames consistently to all incoming code: - "Rollout worker" -> "Inference worker" - run_systematization_to_policy -> run_systematization_to_taxonomy - JudgePolicyParseErrorTest -> JudgeTaxonomyParseErrorTest - RolloutWorkerLoggingTest -> InferenceWorkerLoggingTest - rollout module / log tag -> inference - policy_path / policy_json -> taxonomy_path / taxonomy_json - auditor -> tester, concept -> spec Conflict rule: take main's structure/logic (try/except, ctx fallbacks, runSeedRows helper, rewrite_seed_path, atomic version allocation), keep HEAD's renames. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Catch up the viewer-fix branch with main (17 commits behind, including PR #32 artifact-cache + PR #34 exception handling + PR #40 seeds minItems schema fix). Conflicts: - p2m/viewer_read_model.py: kept HEAD's bumped SCHEMA_VERSION = 2 + GENERATOR_VERSION = "viewer-read-model-v2", added main's log = logging.getLogger(__name__). - viewer/src/lib/server/artifacts.ts: same (kept v2 + added main's SUITE_ARTIFACTS_DIR = 'artifacts'). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Aaron Aspinwall (AaronAspinwall123) and others added 7 commits May 8, 2026 09:27

Hasing and rerun logic

a6ded14

Potential fix for pull request finding

6ac6eec

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

a9115ad

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Aaron Aspinwall (AaronAspinwall123) requested a review from Copilot May 8, 2026 20:20

Copilot started reviewing on behalf of Aaron Aspinwall (AaronAspinwall123) May 8, 2026 20:21 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Comment thread p2m/core/artifact_cache.py Outdated

Comment thread p2m/core/artifact_cache.py Outdated

Aaron Aspinwall (AaronAspinwall123) and others added 3 commits May 8, 2026 13:27

Potential fix for pull request finding

5794bc4

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Jake Present (jakepresent) reviewed May 8, 2026

View reviewed changes

Chang Liu (changliu2) requested review from Copilot and Jake Present (jakepresent) May 11, 2026 17:00

tangym reviewed May 11, 2026

View reviewed changes

tangym approved these changes May 11, 2026

View reviewed changes

Copilot AI reviewed May 11, 2026

View reviewed changes

Copilot started reviewing on behalf of Chang Liu (changliu2) May 11, 2026 17:16 View session

Aaron Aspinwall (AaronAspinwall123) merged commit 9d41eb1 into main May 11, 2026
7 checks passed

Jake Present (jakepresent) mentioned this pull request May 11, 2026

fix: improve exception handling with try-except, debug logging, and specific error types #34

Merged

Chang Liu (changliu2) mentioned this pull request May 11, 2026

ci(science): regression-gate workflow + 6 efficacy metrics + Holm-Bonferroni gate #41

Closed

Navigation Menu

Uh oh!

Hashing and rerun logic - #32

Hashing and rerun logic#32
Aaron Aspinwall (AaronAspinwall123) merged 12 commits into
mainfrom
aaspinwall/hashing

Aaron Aspinwall (AaronAspinwall123) commented May 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Jake Present (jakepresent) left a comment

Uh oh!

Chang Liu (changliu2) commented May 8, 2026

Uh oh!

Chang Liu (changliu2) commented May 9, 2026

Uh oh!

Chang Liu (changliu2) commented May 11, 2026

Uh oh!

tangym May 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		print(
		f"[artifact-cache] warning: latest.json {stage_name} entry "

Uh oh!

Conversation

Aaron Aspinwall (AaronAspinwall123) commented May 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Jake Present (jakepresent) left a comment

Choose a reason for hiding this comment

Uh oh!

Chang Liu (changliu2) commented May 8, 2026

Audit (PR-merge readiness)

P1 — please address before merge

P2 — minor, can be follow-ups

Things I checked that look correct

Branch state

Uh oh!

Chang Liu (changliu2) commented May 9, 2026

Re-audit (post d12e7de + ba31928)

P1 (1) Race in _next_version → ✅ fixed

P1 (2) refresh_compatibility_files silent clobber → ✅ fixed

Bonus fixes from d12e7de (Jake's review)

Carried forward from round 1 (P2, fine as follow-ups)

Verdict

Uh oh!

Chang Liu (changliu2) commented May 11, 2026

Round 3 — re-audit after main advanced (PR #27 + PR #26 merged)

Uh oh!

tangym May 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Re-audit (post `d12e7de` + `ba31928`)

P1 (1) Race in `_next_version` → ✅ fixed

P1 (2) `refresh_compatibility_files` silent clobber → ✅ fixed

Bonus fixes from `d12e7de` (Jake's review)

Round 3 — re-audit after `main` advanced (PR #27 + PR #26 merged)