[malibio]

Closes #3788

What

• Provenance: readGuidanceProvenance reads each seeded agent-guidance and skill root's _seed.key, config_version and guidance_version from the served database. The CLI deliberately never exposes _seed, and the stamp has no version field, so the old node query readback could never succeed. An empty readback for any seeded type now fails the run (exit 2) instead of printing a warning. The cross-rep drift check compares both versions.
• Contention: preflight lists open files with lsof and exits 2 naming every other process (pid, command, working directory, model file) that holds a generative model open. The daemon under test is recognised by its listening socket; embedding models are ignored; remote-provider runs skip the check. Fails closed when lsof cannot run.
• Runbook (nodespace-docs development/agent-eval.md) updated: the check is automatic and points at the poll-every-20-minutes rule.

Tests

bun run test:changed passes (scripts + rust tiers). New unit tests cover the row parser, the empty/partial/unreadable readback failures, the lsof parser (daemon exclusion, embedding exclusion, Ollama blobs, dedupe) and the fail-closed path. The lsof -F shape and the _seed field names were checked against a live machine and the seeding source.

🤖 Generated with Claude Code

[malibio]

Code Review Summary

Net positive and correct in design. Reading _seed from the served DB is the right call given the CLI deliberately hides it, and the column paths match packages/core/src/markdown/mod.rs (_seed.key/config_version/guidance_version on the root node) and the node(node_type, properties) DDL. Empty readback now fails with exit 2 instead of warning. The lsof parser is sound on real output: I ran lsof -nP -F pcfn against the live daemon here and confirmed the embedding GGUF and the plain-path socket name both appear as n lines, and exit 0 with ~640 KB parses fine. 144 tests pass, check-node-type-literals passes, no compat shims, no suppressions.

Requirements (issue #3788)

• Run records key + both versions of every seeded guidance/skill node; drift check fingerprints them (formatSeedEntry).
• Empty provenance fails the run (exit 2 via EnvironmentError/abortOnEnvironment); unreadable DB, missing sqlite3, missing type all covered by tests.
• [~] Another model holder exits 2 and is named (pid, command, cwd, model). Works at each rep's start only; see I1/I2.
• Runbook replaced/pointed at the automatic check (docs b3315bb).

Findings

Critical Issues

None.

Suggested Improvements

• [Improvement] scripts/eval/preflight.ts defaultListOpenFiles: a timed-out lsof is accepted. Bun.spawnSync with timeout kills the process and returns exitCode: null with whatever stdout was flushed; the only guard is stdout === "". A truncated listing then parses as "no other holder", which is the fail-open the function claims to avoid. Require exitCode === 0 || exitCode === 1 (or no signal) before trusting output; otherwise throw.
• [Improvement] scripts/eval/runner.ts gate(): contention is checked only at the start of each rep. The failure the issue describes (a queued merge's Tauri-seam test loading E4B mid-run, Metal OOM, tool-less turns scoring green) can start after the check. Cheap hardening: re-run assertNoModelContention after the scenarios and before writing results (or per group), so a run that overlapped a second model does not produce a results file. Also note the check precedes awaitSkillIndex, which can wait up to 120 s.
• [Improvement] findModelHolders identifies the daemon by exact string equality of env.socket with an lsof n name. A differently spelled but equivalent path (symlinked /tmp, trailing component, relative NODESPACED_SOCKET) makes the daemon under test look like contention. It fails safe (exit 2 naming your own daemon) but would confuse; normalize with realpathSync on the socket's dirname, or compare basename + dir realpath.
• [Improvement] Unverified assumption worth one manual check: that a loaded chat model (mmap'd, fd possibly closed after load) shows up in macOS lsof. I could only confirm this for the embedding GGUF. Run once with E4B loaded and record the f value seen (likely txt/mem) in a test fixture so the parser is pinned to real output.

Nitpicks

• Nit: scripts/eval/runner.ts ~L1562: the entries.length === 0 ? "none found — cannot confirm..." branch is now unreachable (empty throws earlier). Delete it (dead code).
• Nit: scripts/eval/types.ts ~L526: the rewritten comment line is over 100 chars and breaks the block's wrapping; reflow. Several new test lines (runner.test.ts objects, preflight.test.ts proc helper) are also over-long; prettier --check flags these files.
• Nit: no checkGuidanceDrift test for a changed guidanceVersion alone, the half of the stamp that is new. One case would pin it.
• Nit: the sqlite3 readback could pass -readonly so a provenance read can never create -shm/-wal side files or take a write lock against the daemon's DB.
• Nit: lsof only sees the invoking user's processes; a root-owned holder is invisible. Acceptable, but one sentence in the runbook would set expectations.

Recommendation

APPROVE (as a comment; self-authored PR). No blockers. Address the lsof timeout fail-open (I1) before merge since it contradicts the function's stated fail-closed contract; the rest can be folded in at author's discretion.

[malibio]

Verdict: APPROVE with one recommended fix (lsof timeout accepted as complete output; see summary comment). No blockers; the GitHub API rejects approve on a self-authored PR, so this is the real verdict.

[malibio]

Review addressed (93ff5c6)

Addressed

• I1: a timed-out/killed lsof (null exit code, partial output) now throws instead of reading as no holder; only exit 0 or 1 is trusted.
• I2: contention is re-checked after every rep, before any results file is written.
• I3: the daemon socket is matched by its configured and canonical (realpath) spelling.
• Nits: sqlite3 -readonly for the provenance read; dead 'none found' branch removed; added a guidanceVersion-only drift test; runbook notes the post-rep re-check and that lsof sees only the invoking user's processes (nodespace-docs 461a1bd).

Skipped

• I4 (pin a real mmapped chat-model fd type in a fixture): needs a chat model loaded on the GPU. The parser matches on the path, not the fd type, so it does not depend on it. Revisit if a held chat model is ever missed in practice.

Scripts tier: 1559 pass.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED
Rationale: small, low-risk changes that each implement the reviewer's own suggestion, with tests; no new design.