Add invigilator: anti-cheat trace audit for leave-one-out runs

[cmungall]

What

An anti-cheat invigilator (src/folds/invigilator.py) that audits an agent run's tool-call trace (agent-<id>.jsonl) and flags any file the agent read or touched outside the allowed roots. This is the audit backstop for the isolation model: the .runs/<cfg>/ env removes the held-out cluster's answers (absence), the agent is told the boundary (instruction), and this verifies it after the fact (audit).

Policy: root-based

An access is in-bounds iff it resolves under the run env (.runs/<cfg>/) or the shared raw-data dir (~/ess-dive_wfsfa_soil_datasets/). This works because every answer (the real data/gold/, the full mapping with the held-out entry, any answer-key dir) lives outside the env, while every legitimate input (skills, ablated code, filtered mapping, the held-out dataset's metadata via the env symlink, raw CSVs) lives inside one of those roots — so no fragile per-identifier rules are needed. Reads MANIFEST.json to pick up the held-out dataset_identifiers (used to annotate violations) and exits non-zero on any violation, so it can gate a run.

Two correctness details (both caught by building it)

• cwd-aware Bash parsing. Agents cd into the env and use relative paths (cd .runs/<cfg> && cat data/processed/…). A substring match false-positives; we track cd per command and resolve relative tokens against the effective cwd. (A naive first pass flagged these; this is the fix.)
• Lexical containment, not realpath. The env symlinks shared inputs in (data/external/ess-dive_meta); following symlinks would point those reads back outside the env and wrongly flag them. We normalize paths lexically but don't follow symlinks, so accessing an input via the env is in-bounds while reaching it by its real outside path is flagged.
• Also ignores /dev|/proc|/sys tokens (e.g. 2>/dev/null) and surfaces every Bash command verbatim for human review — it never claims a command was clean when it couldn't parse it.

Validation

• 8 unit tests + doctest (synthetic traces: clean run, real-gold/answer-key detection, the cwd false-positive fix, held-out-identifier annotation, /dev/null redirects, CLI exit codes). Full suite: 48 green.
• Run against the real fold_ds07 subagent trace: CLEAN, exit 0, 7/7 reads in-bounds — reproduces the earlier manual audit.

Use

python -m src.folds.invigilator --trace <agent-id>.jsonl --env .runs/<cfg> [--show-bash]

Reusable by the Step-3 runner (gate each run) and Step-4 scorer (record the audit verdict per run). The runner should also persist each run's trace next to its outputs so this doesn't depend on ~/.claude/projects/….

Builds toward the leakage-audit dimension of the comparison-strategies discussion.

[claude]

Claude finished @cmungall's task —— View job

---

PR Review: Add invigilator: anti-cheat trace audit for leave-one-out runs

Tasks:

• Gather context
• Read source files
• Analyze code quality, bugs, performance, security, test coverage
• Post review feedback

---

Overall this is well-designed, clearly documented, and makes the right tradeoffs (lexical over realpath, cwd-tracking in Bash). The tests are solid. Below are the issues worth addressing before merging.

---

Bugs / Correctness

1. _PATH_TOKEN regex matches short single-component names (invigilator.py:55)

_PATH_TOKEN = re.compile(r"(?:~|\.{1,2}|/)?/?[\w.@+\-]+(?:/[\w.@+\-]+)+")

A token requires at least one / in the path component, so true single-component paths like secrets.json or ~/.config (after cd) are missed. More importantly, ~/ess-dive_wfsfa_soil_datasets resolves as expected because ~ anchors it — but a Bash command like cat /etc/passwd is caught since / is the prefix, so that case is fine. Low severity, but worth noting: a bare ~ token alone (without a path component after it) is also not matched.

2. _CD regex is too strict — misses many real cd invocations (invigilator.py:56)

_CD = re.compile(r"^\s*cd\s+(?:--\s+)?['\"]?([^'\"&;|]+?)['\"]?\s*$")

The ^...$ anchors the regex against the segment after splitting on &&|;|.... For a command like:

cd .runs/cfg && cat data/gold/secret

after splitting, the segment is cd .runs/cfg (with trailing space) — this does match. But:

cd ".runs/cfg with spaces"

The [^'\"&;|]+? group stops at " because " is excluded from the character class. The outer ['\"]? only strips one quote. A path with spaces inside double quotes won't track correctly.

This is minor for current use, but could cause false-negatives (missed cd → wrong cwd → wrong path resolution for subsequent tokens).

3. && || token filter is unreachable (invigilator.py:200-201)

for tok in _PATH_TOKEN.findall(seg):
    if tok in ("&&", "||"):
        continue

_PATH_TOKEN requires at least one /, so && and || can never match it. This dead code suggests an earlier regex version was used; the guard is harmless but misleading.

4. Write and Edit tools don't count toward n_reads — this is intentional and fine — but reads_in_bounds only increments for Read not Write/Edit. The stat is correctly named but the CLI output says reads (N in-bounds) which doesn't include write violations. Minor wording issue, not a bug.

5. Bash multi-line command show_bash truncation (invigilator.py:242)

typer.echo(f"  $ {c.splitlines()[0] if c else ''}")

Only the first line of multi-line commands is shown in --show-bash mode. Multi-line Bash heredocs or scripts will be silently truncated in output. The audit itself processes all lines (the \n split at line 194 handles this), but the human-review display is misleading — a command that spans 20 lines shows as 1. Consider printing with a \n marker or the full command.

---

Design / API Concerns

6. repo_root defaults to Path.cwd() at import time (invigilator.py:214)

repo_root: Path = typer.Option(Path.cwd(), "--repo-root", ...)

Path.cwd() is evaluated at function-definition time in Python (it's a default argument). If the CLI is invoked from a different directory at runtime than when the module was first loaded, this will silently use the wrong root. Use None as the default and resolve inside the function body, as is already done in audit():

repo_root: Optional[Path] = typer.Option(None, "--repo-root", ...),

7. Grep tool auditing uses path but Grep also has glob parameter

The Grep tool call has both path (directory to search) and glob (file pattern). The audit checks the path but not the glob field. A glob pattern like /data/gold/**/*.py could leak information about answer key paths without path being set. This is low risk for the current use case, but worth flagging.

8. LS tool is not audited

The code handles Read, Write, Edit, NotebookEdit, Grep, Glob, Bash — but LS (list directory) is not covered. An agent could ls the gold directory to discover held-out cluster identifiers without triggering a violation. Given the PR's goal of detecting any out-of-bounds access, this seems like a gap.

---

Performance

9. Recursive walk() on every JSONL line (invigilator.py:63-76)

The nested walk() recurses into every dict/list in each trace line. For large transcripts this is fine, but deeply nested JSON (e.g. full Claude API responses with long content arrays) could be slow. Given this is an offline audit tool, not a hot path, it's acceptable — just noting it.

---

Test Coverage

10. Missing test: LS tool (see issue #8 above)

11. Missing test: multi-segment cd chains

cd /tmp && cd .runs/cfg && cat data/gold/secret

Multiple cd calls in one command should accumulate correctly. No test covers the chaining case.

12. Missing test: ~ home-dir expansion in Bash tokens

A command like cat ~/ess-dive_wfsfa_soil_datasets/data.csv should be in-bounds; cat ~/data/gold/secret.csv should not. This isn't tested.

13. test_clean_run uses string open() inside -c (tests/test_invigilator.py:53)

("Bash", {"command": f"cd {env} && python3 -c \"open('data/processed/m.json')\""}),

The path data/processed/m.json inside a Python string literal inside a -c flag won't be extracted by _PATH_TOKEN (it searches the Bash command line, not string literals within it). The test passes because data/processed/m.json isn't matched at all, not because it's correctly identified as in-bounds. This test doesn't actually exercise what the comment claims.

---

Minor / Nits

• Optional[Path] / Optional[list[Path]] can use Path | None / list[Path] | None since from __future__ import annotations is already imported (lines 44).
• The _abs() inner function in audit() duplicates logic from lexical_resolve() — could reuse it.
• context: str in Violation is set to raw[:200] — good truncation, but the field name context is ambiguous (is it the surrounding file context or the Bash command?). source_command or raw_input would be clearer.

---

Summary

Category	Count
Bugs / Correctness	5
Design gaps	3
Missing tests	4
Minor/nits	3

The most actionable issues are #6 (Path.cwd() default argument), #8 (missing LS audit), and #13 (misleading test). The rest are lower severity. The core design and the cwd-tracking + lexical-containment approach are solid.

--- | Branch: feat/invigilator

[cmungall]

Thanks for the review — addressed in 680dfca. Summary:

Fixed

• build_env + mapping filter for leave-one-cluster-out run environments #6 repo_root default-arg bug — real bug. Now Option(None), resolved in the body. ✅
• Refactor expert harmonizer from monolith to modular package #8 LS not audited — added; LS.path is now checked. ✅
• Connect to skills #1 single-component reads after cd — closed the realistic leak: a cd into an answer location (answer key / real data/gold / data/processed) is now flagged as Bash(cd), so cd /tmp/eval_answer_keys && cat expert_dataset_07.py is caught even though expert_dataset_07.py (no /) isn't tokenized. ✅
• Add Claude Code AI workflows for automated assistance #7 Grep glob — now also tokenizes glob/pattern (e.g. glob: data/gold/**/*.py). ✅
• Cluster-ablation splicer for leave-one-cluster-out #5 --show-bash truncation — prints full multi-line commands now. ✅
• Add LinkML schema for harmonized soil moisture data #3 dead && || guard — removed. ✅
• New tests for all of the above + multi-cd chains and ~ expansion (8 → 13 tests, suite 53 green; real fold_ds07 trace still CLEAN).

Deliberately deferred (low value for current use, noted)

• Populate ground-truth data #2 quoted-space cd — env/run paths have no spaces; the cd-target check above is the higher-value fix. Left as-is.
• Cluster-ablation script for leave-one-cluster-out (blocked on raw-data staging) #4 "reads (N in-bounds)" wording — the line is specifically about reads; writes/bash violations are listed separately. Left as-is.
• [codex] Fix raw data staging fallbacks #9 recursive walk() perf — offline tool, acceptable as you noted.

The _PATH_TOKEN "requires a /" behavior (#1's first half) is intentional — matching bare single-component tokens would false-positive on nearly every command word. The cd-target check is the targeted mitigation instead.

[claude]

Claude finished @cmungall's task —— View job

---

Claude finished @cmungall's task —— View job

---

PR Review: Follow-up on invigilator.py after 680dfca

Tasks:

• Gather context
• Read source files
• Analyze code quality, bugs, performance, security, test coverage
• Post review feedback

---

The changes in 680dfca address all the major issues from the first review. This is in good shape. A few remaining observations:

---

Still Present (Previously Noted, Deferred)

#2 Quoted-space cd (invigilator.py:56)
Still deferred. The _CD regex's [^'\"&;|]+? character class stops at ", so cd ".runs/with spaces" doesn't track. Author noted env paths have no spaces — acceptable deferral, but worth a comment in the code since it's a known limitation.

#13 Misleading test (tests/test_invigilator.py:53)

("Bash", {"command": f"cd {env} && python3 -c \"open('data/processed/m.json')\""}),

The path data/processed/m.json lives inside a Python string literal inside a -c flag. _PATH_TOKEN scans the Bash command line, so it never extracts this token — the test passes because the path is not matched, not because it's correctly classified as in-bounds. The test doesn't exercise what the comment implies. Either remove this entry or replace it with a Bash-native read (e.g. cat data/processed/m.json after the cd).

---

New Issues

_reason() and _forbidden() duplicate answer-key detection logic (invigilator.py:128-147)

Both functions independently encode the same answer-key detection pattern:

"eval_answer_keys" in s or re.search(r"expert_dataset_\d+|expert_mapping_entry", s)

and:

under(p, repo_root / "data" / "gold") or under(p, repo_root / "data" / "processed")

If the answer-key naming convention changes, both functions need updating. Consider extracting a shared _is_answer_location(p, repo_root) -> bool helper that both call.

Relative path in Grep/Glob/LS resolves against repo_root, not tracked cwd (invigilator.py:201)

check(name, lexical_resolve(str(p), str(repo_root)), str(p))

Bash paths are resolved against the tracked cwd (correct), but Grep/Glob/LS path arguments are always resolved against repo_root. If an agent runs cd .runs/cfg && grep -r pat data/gold/ in a Bash command that's a Bash violation, but a separate Grep(path="data/gold/") tool call after a prior cd would not track the cd (since cd only updates cwd within the Bash command loop). This is inherent to the per-tool-call design and probably acceptable — agents using the Grep tool likely pass absolute paths anyway. Worth a comment acknowledging this limitation.

---

Minor Nit

• Optional[Path] / Optional[list[Path]] at lines 44/154 — since from __future__ import annotations is imported, these can be Path | None / list[Path] | None (noted in original review, not addressed, minor).

---

Summary

Category	Count
Previously deferred, still present	2
New issues found	2
Nits	1

The core audit logic, cwd-tracking, lexical-containment, LS coverage, and multi-cd chain handling are all correct. The only actionable items before merge are the misleading test (#13) and optionally extracting the shared answer-key predicate. Everything else is low-severity.

--- | Branch: feat/invigilator