Capture and invigilate harmonization eval traces

[hmworsham]

Summary

• capture Claude Code execution logs in isolated eval workspaces
• automatically run the invigilator before eval branch staging
• retain the trace and audit report with each staged run
• accept both JSON and JSONL trace formats

Validation

• python -m pytest tests/test_invigilator.py tests/test_build_env.py -o addopts=''
• validated workflow trace-capture ordering

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @hmworsham's task —— View job

---

PR Review

• Gather context
• Read changed files
• Provide review feedback

---

This is a well-structured PR that adds trace capture and invigilator enforcement to the evaluation pipeline. The security design is sound. Here's my detailed feedback:

Strengths

• Clean isolation model: Preserving the invigilator outside the agent workspace before rsync --delete is a solid isolation technique. The agent can't tamper with its own auditor.
• Correct if: always() placement: Running the invigilator before branch staging ensures audit results are included in the staged eval run — good ordering.
• Dual JSON/JSONL support in load_tool_uses: The approach of attempting full JSON parse then falling back to JSONL is correct and handles the format difference cleanly.
• Thorough test coverage: The new test_load_tool_uses_accepts_action_execution_json test covers the new code path well, and the existing suite is comprehensive.

Issues and Observations

1. Audit step doesn't fail the workflow on violations (run-eval.yml:196-199)

The invigilator exits with code 1 on violations, but the tee pipe's exit code dominates in some shells. More critically: the step has no explicit exit propagation from the audit command through tee. In bash with set -euo pipefail, pipefail should catch this, but only if pipefail is set — which it is (set -euo pipefail on line 188). This is fine, but worth explicitly documenting that pipefail is what makes the audit result fail the step. Currently a reader might think the tee output is the final command and overlook this.

2. Trace file copied but not the JSONL format (run-eval.yml:195)

The action's execution_file is JSON (single document), copied as audit/claude-execution.json. The load_tool_uses function now handles both formats, but the filename .json extension is semantically correct here. No bug — just confirming this is intentional.

3. env_path output points to GITHUB_WORKSPACE, not the build root (run-eval.yml:99)

out.write(f"env_path={os.environ['GITHUB_WORKSPACE']}\n")

This is used in the artifact upload step (path: ${{ steps.resolve.outputs.env_path }}/). After rsync --delete, GITHUB_WORKSPACE contains the isolated fold, so this is correct. But the variable is named env_path while pointing to the workspace, not the build root path — slightly confusing naming.

4. load_tool_uses error handling could be more specific (invigilator.py:87-95)

text = Path(trace_path).read_text().strip()
if not text:
    raise ValueError(f"trace is empty: {trace_path}")
try:
    records = [json.loads(text)]
except json.JSONDecodeError:
    records = [json.loads(line) for line in text.splitlines() if line.strip()]

If the JSONL fallback also fails (malformed lines), the error from json.loads will be raised without context about which line failed. Consider wrapping with a more informative error, e.g.:

try:
    records = [json.loads(line) for line in text.splitlines() if line.strip()]
except json.JSONDecodeError as e:
    raise ValueError(f"trace is not valid JSON or JSONL: {trace_path}") from e

5. Audit step hard-fails on missing trace but agent step can be skipped (run-eval.yml:190-194)

if [ -z "$EXECUTION_FILE" ] || [ ! -f "$EXECUTION_FILE" ]; then
  echo "ERROR: ..." | tee audit/invigilator_report.txt
  exit 1
fi

This is correct defensive behavior. One edge case: if the harmonize step was cancelled (not just failed), execution_file may be empty. The current check handles this properly. Good.

6. git add -f .claude/settings.local.json (run-eval.yml:227)

The comment explains why (Claude Code ignores project-local settings), but force-adding a typically gitignored file in an orphan commit to a public repo is worth a second look — make sure this file contains no secrets or tokens. Assuming it only contains tool restrictions, this is fine.

7. Test for JSONL format coverage (test_invigilator.py)

The existing write_trace helper writes JSONL, and most tests use it. The new test test_load_tool_uses_accepts_action_execution_json covers the JSON path. Consider adding a test that verifies a malformed trace (neither valid JSON nor valid JSONL) raises an appropriate error — currently this path has no test coverage.

Minor Nits

• invigilator.py:367: The --trace help text says agent-<id>.jsonl but the workflow now saves it as claude-execution.json. Minor doc drift.
• The Upload run outputs step uses env_path (which is GITHUB_WORKSPACE) while the staging step uses GITHUB_WORKSPACE directly — slight inconsistency, but both resolve to the same value post-rsync.

Summary

The core implementation is correct and the security design is sound. The main actionable items are:

1. Add a test for malformed/invalid trace files in load_tool_uses
2. Update --trace help text to reflect that both .json and .jsonl are accepted
3. Consider clarifying that pipefail is what propagates audit failures (documentation/comment improvement)

---

View job