--- name: code-learnings description: > Durable index of lessons learned in past BioMysteryBench harness sessions, numbered L-01, L-02, …, L-NN. Every session must read it at startup (mandated by CLAUDE.md) and append to it at session-end when a non-obvious bug, workflow constraint, or successful unusual approach was discovered. Use this skill whenever the user asks to "add a learning", "record a lesson", "save what we just learned", to write up a post-mortem finding, or to reference a specific learning by ID ("L-12", "L-21"). Trigger even when the user just says "this is worth remembering" or "we should write this down for future agents". Do not invent learnings — every entry must cite a specific PR, commit, or incident. --- # code-learnings skill This file records lessons learned from past agent sessions. Read it after `claude-progress.txt` and `documents/features.md`, before implementing anything. Update this file at the end of any session where a non-obvious mistake was made or a non-obvious approach turned out to be right. --- ## L-01: Cross-provider client routing — always build separate clients **Lesson:** When a harness component (critic, judge, scorer) calls a model that may be on a *different* provider than the agent, it must use its own `Provider` client — never `self.client` (the agent's client). **The bug (PRs #28, #30, #31):** The critic was given a separate `critic_client` in `run_eval.py` and stored in `AgentRun.__init__`, but `_run_critic()` still called `self.client.chat()`. The Cerebras endpoint rejected the Claude model name; the exception was silently swallowed, so the critic appeared to work but produced nothing. The same bug existed independently in the scorer's LLM judge. **Rule:** Any time you add a `*_client` parameter to `AgentRun.__init__`, grep immediately for every call site of the corresponding method and confirm it uses `self.*_client`, not `self.client`. **Pattern (from harness/agent.py and harness/scorer.py):** ```python # In AgentRun.__init__: self.critic_client = critic_client or client # falls back to agent client for same-provider # In _run_critic: response = self.critic_client.chat(model=critic_model, ...) # NOT self.client ``` --- ## L-02: Silent exception handling is a debugging trap **Lesson:** `except Exception: return ""` hides every failure mode. If a component can fail (network error, wrong model name, wrong endpoint), the failure must be observable without reading code. **The bug (PR #26 → fixed PR #31):** `_run_critic()` caught all exceptions and returned `""`. Routing failures were invisible — no crash, no log entry, no warning. **Rule:** Always log exceptions before returning a safe default: ```python except Exception as e: self.logger.log("critic_error", {"error": str(e), "critic_model": critic_model}) return "" ``` Add the `critic_error` / `judge_error` role to the trajectory log schema when implementing any new LLM-calling component. --- ## L-03: Model names drift — verify before coding, add preflight ping **Lesson:** Anthropic deprecates model aliases. `claude-3-5-haiku-20241022` returned 404 in May 2026; the working model is `claude-haiku-4-5-20251001`. During a long debugging session with multiple simultaneous failures, the agent flipped the model name in the wrong direction (PR #29) and required a correction (PR #32). **Rules:** - Before hardcoding any `claude-*` model string, verify it against the current Anthropic model list (or use `_ping_model()` which is already in `run_eval.py`). - Do not change a model name without also running the preflight ping to confirm the new name returns a valid response. - The preflight ping (`_ping_model()` in `scripts/run_eval.py`) must cover every distinct client/model pair used in the run (agent, judge, critic). **Current known-good names (as of 2026-05-17):** - `claude-sonnet-4-6` — agent default - `claude-haiku-4-5-20251001` — judge/critic default - `qwen-3-235b-a22b-instruct-2507` — Cerebras Qwen3 --- ## L-04: cache_control must not be sent on empty system prompts **Lesson:** `AnthropicProvider.chat()` always wrapped `system` in a `cache_control: ephemeral` block, including when `system=""`. The Anthropic API returns 400 `invalid_request_error: cache_control cannot be set for empty text blocks`. **The bug (fixed PR #33):** The agent path always has a non-empty system prompt, so this was never triggered there. The critic/judge path passes `system=""` and hit the error. **Rule:** Guard `cache_control` blocks on non-empty content: ```python if system: kwargs["system"] = [{"type": "text", "text": system, "cache_control": {"type": "ephemeral"}}] ``` This guard is already in `harness/llm.py` (PR #33). Apply the same pattern to any future provider that wraps system prompts. --- ## L-05: Elephant carpaccio — sub-slice before implementing new harness components **Lesson:** The critic feature was one 140-line PR covering config, CLI flags, trajectory formatting, prompt, injection logic, token cost, and cross-provider routing. This made bugs hard to isolate and produced five follow-on fix PRs. **Rule:** Before implementing any new harness component, break it into sub-slices in `documents/features.md`. Each sub-slice must have: 1. A testable condition (unit test with a mock, not just "tested via integration") 2. A single clear scope boundary **Example decomposition for a new LLM-calling component:** | Sub-slice | Scope | Test | |-----------|-------|------| | X-1: Same-provider path | Basic call through `self.client` | Mock client, assert called | | X-2: Cross-provider path | Separate `x_client`; plumb through `__init__` and call site | Assert `x_client.chat()` called, not `self.client.chat()` | | X-3: Error observability | Log exceptions as `x_error` role | Assert logger called on exception | | X-4: Edge cases | Empty inputs, empty system prompts, etc. | Unit test each | Do not merge X-2 without a test that would have caught the "built the client but called the wrong one" bug described in L-01. --- ## L-06: New LLM-calling components need unit tests, not just integration tests **Lesson:** The critic had no dedicated unit tests ("tested via integration"). Integration tests require live API calls and live Docker — they are not run in CI and rarely caught in-session before a PR is merged. **Rule:** Every new `Provider`-calling method must have at least one unit test using a mock `Provider`: ```python class MockProvider: def __init__(self): self.calls = [] def chat(self, *, model, messages, **kwargs): self.calls.append((model, messages)) return SimpleNamespace(text="mock response", usage=SimpleNamespace(...)) ``` Test: (a) the right client is called, (b) exceptions are logged not swallowed, (c) the return value is wired into the caller correctly. --- ## L-07: After fixing a routing bug, grep for the same pattern elsewhere **Lesson:** The cross-provider routing bug existed independently in both the critic (`harness/agent.py`) and the judge (`harness/scorer.py`). Fixing one without searching for the other left the second bug in place until a separate run exposed it (PRs #28 and #30 were sequential, not simultaneous). **Rule:** When you fix a structural bug (wrong client used, wrong model name, missing guard), immediately grep the whole codebase for the same pattern: ```bash grep -rn "self\.client\.chat\|client\.chat" harness/ ``` Fix all instances before declaring the bug resolved. --- ## L-08: Diagnose 429s before choosing a fix — queue congestion ≠ quota exhaustion **Lesson (2026-05-18):** Cerebras returned `429 queue_exceeded` on the Qwen3 benchmark run. The initial instinct was "we hit a rate limit". In fact, actual usage was 5.7 RPM and ~35K TPM against limits of 200 RPM and 200K TPM — well under 10% of quota. The error was infrastructure queue congestion, not per-account exhaustion. **Diagnostic steps before implementing a fix:** 1. Calculate actual RPM/TPM from the trajectory (steps ÷ wall minutes, tokens ÷ wall minutes). 2. Compare against account limits — read `x-ratelimit-limit-*` headers via `scripts/check_cerebras_limits.py` (makes a live minimal call and prints all headers). 3. Check the Cerebras status page (https://isdown.app/status/cerebras-inference) for active incidents affecting the model you are using. **`queue_exceeded` (code `queue_exceeded`, param `queue`):** transient infrastructure congestion. Exponential backoff (60s/120s/240s) is the right fix — already implemented in `OpenAIProvider.chat()`. **Genuine quota exhaustion:** `x-ratelimit-remaining-*` headers near zero. Backoff won't help; you need to wait for the bucket to replenish (`x-ratelimit-reset-*` tells you when) or reduce parallelism. --- ## L-09: Always run pytest from the worktree directory, not the main repo **Lesson (2026-05-18):** When working in a git worktree, running `pytest` from `/path/to/main-repo` collects tests from the *main repo's* `tests/` directory, not the worktree's. New tests added to the worktree are silently ignored and the count looks unchanged (168 passed instead of 173). **Rule:** Always `cd` to the worktree root before running tests, or use the absolute path explicitly: ```bash # Wrong — runs main repo tests, misses worktree changes: cd /Users/ian/Documents/Claude/bio-mystery-bench && python3 -m pytest tests/ # Right — runs worktree tests: python3 -m pytest tests/ # from inside the worktree (default shell cwd) ``` The worktree's default shell working directory is already the worktree root, so plain `python3 -m pytest tests/` is correct. Only add an explicit `cd` if you need the main repo. --- ## L-10: --resume re-runs resource_abort attempts, corrupting scores.json totals **Lesson (2026-05-18):** `is_attempt_complete()` in `harness/logger.py` does not treat `resource_abort` as a terminal status. When `--resume` is used, any attempt that ended in `resource_abort` is re-run and appended to the `attempts` array in `scores.json`. This leaves `total_attempts` and `pass_at_N` keys inconsistent with the actual attempt count — e.g. hb020 showed `total_attempts: 4` and `pass_at_4: true` but had 6 entries in its `attempts` array. **Rule:** When reading `scores.json` after a resumed run, do not trust `total_attempts` or `pass_at_N` keys if any attempt had `resource_abort` status. Count the `attempts` array length manually, or read pass/fail results from the monitor output captured during the run. The pending fix is slice SC-4 / the `--resume` scoring bug. **Detection:** If `len(attempts) != total_attempts` in any problem's scores.json entry, the totals are stale from a pre-resume snapshot. --- ## L-11: _clean_answer() strips bare underscores, silently corrupting predicted values **Lesson (2026-05-18):** `harness/scorer.py` `_clean_answer()` applies: ```python text = re.sub(r"\*{1,2}|_{1,2}", "", text) ``` This is intended to strip markdown bold/italic markers (`**`, `__`, `*`, `_`) but also destroys any bare underscore that is part of an identifier (e.g. `Sample_01` becomes `Sample01`). The corruption is silent — `scores.json` shows the corrupted value as `predicted`, so it looks like the model gave the wrong format when it actually gave the right answer. **Confirmed impact:** hb022 attempts 2 and 5 both output `[Sample_01, ..., Sample_08]` (correct) but were scored wrong because the extractor mangled them to `[Sample01, ...]`. **Rule:** When an attempt is marked wrong but the trajectory's FINAL ANSWER looks visually correct, check whether `_clean_answer()` is corrupting the extracted value before concluding the model was wrong. The fix (SC-1) replaces the blanket underscore strip with paired-delimiter matching. --- ## L-12: rc=0 with empty stdout from a tool means no results, not tool absent **Lesson (2026-05-18):** When `blastn -db nt -remote` returned rc=0 with empty stdout (network timeout / no BLAST hits), the Qwen3 agent concluded "blastn not available" and spent ~15 steps per attempt attempting to reinstall it. rc=0 means the binary ran successfully — empty output means the query returned nothing. **Rule:** Do not confuse empty output with tool absence. The canonical tool-availability check is `which ` or ` --version`, not inference from an empty result. The system prompt now includes (pending slice SP-1) an explicit rule: > *A command that exits rc=0 with empty output ran successfully but produced no results.* > *Use ` --version` to confirm availability before reinstalling.* **Corollary:** When adding a new tool to the Docker image, add a `which ` smoke test (ENV-3) so future agents can quickly verify availability without ambiguity. --- ## L-13: conda/micromamba tools are not on the default container PATH **Lesson (2026-05-18):** Tools installed inside the micromamba environment (`python3`, `pip`, `bedtools`) are not on `$PATH` when the agent's bash commands run. `which python3` returns rc=1; `pip` returns rc=127. The system prompt lists these as pre-installed, creating a false expectation. **Confirmed impact:** recqgsfxqqodhjens aborted 4/7 attempts citing missing Python and bedtools. hb022 attempt 3 also hit rc=127 for python3. **Root cause:** The Dockerfile does not add `/opt/conda/bin` to `$PATH` and the entrypoint does not activate the base environment before the agent's commands run. **Rule:** Do not claim a tool is pre-installed in the system prompt unless `which ` confirms rc=0 in a fresh container shell. The fix (ENV-1/ENV-2) adds the conda bin directory to `$PATH` in the Dockerfile. Until that fix is merged, agents working in the container should prefix commands with the full path `/opt/conda/bin/python3` or run `export PATH=/opt/conda/bin:$PATH` as a first step. --- ## L-14: Unquoted env vars silently drop CLI arguments, causing cryptic parse errors **Lesson (2026-05-19):** When `$CEREBRAS_API_KEY` was unset, the shell expanded it to nothing, turning `--api-key $CEREBRAS_API_KEY --model qwen-3-...` into `--api-key --model qwen-3-...`. Click consumed `--model` as the *value* of `--api-key`, then saw `qwen-3-235b-a22b-instruct-2507` as an unexpected positional argument: ``` Error: Got unexpected extra argument (qwen-3-235b-a22b-instruct-2507) ``` No warning was emitted about the missing key — the error looked like a model-name bug. **Rule:** Always double-quote env vars in shell commands: `--api-key "$CEREBRAS_API_KEY"`. With quotes, an unset var passes an empty string to the option (which gives a clear "empty api key" error) rather than silently collapsing the argument list. --- ## L-15: API keys live in the main repo root .env, not in the worktree **Lesson (2026-05-19):** `CEREBRAS_API_KEY` (and other provider keys) are stored in `/Users/ian/Documents/Claude/bio-mystery-bench/.env`. Worktrees do not have their own `.env` — the file is gitignored at the repo root. **Rule:** When running benchmark commands from a fresh shell or a worktree, source the main repo `.env` first: ```bash set -a && source /Users/ian/Documents/Claude/bio-mystery-bench/.env && set +a ``` Alternatively, pass the key explicitly: `--api-key "$CEREBRAS_API_KEY"` after sourcing. Do not assume the key is already exported in the shell environment. --- ## L-16: A constant suspicious value across many rows signals an early-return code path **Lesson (2026-05-21):** `data_desc` showed "no data directory found" for 20/25 Sonnet rows and 17/25 Qwen3 rows in the trajectory analysis. The uniformity was the tell — a genuine "no data" result would not be this consistent across problems that clearly used data files. **Diagnosis pattern:** When an analysis column shows the same fallback string in the majority of rows, grep for where that string is returned and look for an early-return triggered by a None/missing input. Here: `_list_data_files(None)` returned immediately without reading the filesystem because `data_dir=None` was hardcoded at the call site. **Rule:** If an analysis script computes per-row metadata that requires a file path, auto-detect the path rather than passing `None` and relying on a fallback message. Walk up from the output directory to find data caches, config files, etc. If auto- detection fails, print a warning to stderr rather than silently returning a misleading default value. --- ## L-17: Post-hoc analysis scripts and live harness components are independent pipelines **Lesson (2026-05-21):** The `data_desc` bug in `analyze_trajectories.py` (L-16) prompted the question: did the wrong `data_desc` corrupt the critic's behavior during the actual runs? **Answer: no.** The critic runs *during* evaluation by reading `self.messages` — the live conversation history. `analyze_trajectories.py` is a post-hoc script that reads JSONL trajectories *after* the run completes. The two pipelines share no state. **Consequence for debugging:** When trajectory analysis output looks suspicious, first determine whether the suspicious column comes from: 1. **Deterministic extraction** from JSONL records (e.g. `critics_response`, `number_API_backoffs_fired`) — these accurately reflect what happened in the run. 2. **LLM-generated fields** (e.g. `data_desc`, `notes`, `objectively_correct`) — these can be wrong if the analysis script passed bad inputs to the LLM. A bug in LLM-generated analysis fields cannot retroactively change what the critic or agent did. Only a bug in the harness itself (`harness/agent.py`, `harness/scorer.py`) can do that. --- ## L-18: Rubric format mismatches need scorer normalisation, not upstream dataset fixes **Lesson (2026-05-21):** The hb022 rubric is malformed — the intended answer was a Python list `['Sample_01', ..., 'Sample_08']` but the leading `['` was stripped when the dataset was built, leaving `Sample_01', 'Sample_02', ...`. Models correctly followed the question's example format and output `[Sample_01, ..., Sample_08]`. Neither exact-match nor the LLM judge recognised the two formats as equivalent, so every correct attempt scored wrong. **Why not fix the dataset?** Patching a HuggingFace dataset record requires a new dataset commit and all users pulling a new revision. The scorer fix is local, testable, and covers any future problem with the same format mismatch. **Pattern (from harness/scorer.py, PR #53):** Add a list-normalisation step that strips brackets/quotes, splits by comma, sorts, and compares: ```python def _normalize_list(text: str) -> Optional[list[str]]: cleaned = text.strip().strip("[]").replace("'", "").replace('"', "") items = [item.strip() for item in cleaned.split(",")] items = [item for item in items if item and item != "..."] return sorted(items) if len(items) >= 2 else None ``` **Return contract for format-normalisation steps:** Return `True`/`False`/`None`, not `bool`. Only `True` should short-circuit `score_answer`; `False` and `None` fall through to the LLM judge. This is safe: wrong answers and ambiguous formats still get judged; only confirmed matches skip the API call. **When to add a new normalisation step:** when you observe a pattern where the model's predicted format and the rubric format differ structurally but are semantically equivalent (list syntax, unit differences, synonym resolution). Add the step before the LLM judge, unit-test it with the real rubric string, and update `documents/code_walkthroughs/3.Accommodating_OpenAI_models.md` section §7. --- ## L-19: New behavioral constraints on the end_turn path break existing mock fixtures **Lesson (2026-05-22):** FA-2 added FINAL ANSWER marker enforcement to `_loop`'s `end_turn` branch. This immediately broke two CR2 tests whose mock responses used strings like `"answer0"` with no marker. The re-prompt path consumed an extra `client.chat` call from the `side_effect` list; once exhausted, `MagicMock` raised `StopIteration` (caught as a generic exception), causing `AgentRun.run()` to return `status="error"` instead of `"success"`. The tests appeared to test the right thing but gave the wrong status — a subtle failure with no obvious traceback pointing at the fixture. **Rule:** Any mock response that drives an `end_turn` path in `_loop` must satisfy **all active behavioral constraints on that path**. Currently those are: 1. Critic injection (rounds < max_critic_rounds, injection point enabled) 2. FINAL ANSWER marker (`_has_final_answer_marker` must return `True`, or an extra `client.chat` call is consumed for the re-prompt) When you add a new constraint to the `end_turn` branch, grep for existing tests that mock `client.chat` with `end_turn` responses and update their fixtures to satisfy the new constraint. The fix is usually one line — e.g. prefix mock text with `"FINAL ANSWER: "`. **Detection:** if a test that previously returned `status="success"` starts returning `status="error"` after a seemingly unrelated change to `_loop`, suspect that the new code consumed an extra loop iteration that the `side_effect` list didn't budget for. --- ## L-20: In a git worktree, `git checkout main` fails — branch from origin/main instead **Lesson (2026-05-22):** Worktrees check out branches independently of the primary repo. Running `git checkout main` inside a worktree raises: ``` fatal: 'main' is already used by worktree at '/path/to/primary-repo' ``` **Rule:** Never attempt `git checkout main` from inside a worktree. Instead, create feature branches directly from the remote: ```bash git fetch origin git checkout -b claude/ origin/main ``` This sets up tracking automatically and ensures the branch starts from the latest main without needing to check out main locally. --- ## L-21: Parallel agents must keep `__init__` and `_loop` in sync — partition by attribute, not by file **Lesson (2026-05-22):** During the four-agent Qwen3 post-mortem, the orchestration brief told each agent to touch `AgentRun.__init__` only for its own field (Agent C: `_critic_rounds`, Agent B: `_blast_versions`, Agent A: `_final_answer_reprompted`). The intent was to keep PRs disjoint and avoid merge conflicts. The mechanism worked for the conflict-avoidance goal — BE only conflicted once with CR2, on the line itself, and the resolution was trivial. It failed for a different reason. Agent A's FA PR (#58) added a reference to `self._final_answer_reprompted` inside `_loop` but never assigned it in `__init__`. The PR passed CI in isolation because FA's own tests stubbed `AgentRun` in a way that avoided the bug. The moment FA merged, every other test in the suite that drove `AgentRun.run()` through the `end_turn` path started raising `AttributeError`. Four tests on `main` broke between FA's merge and the next PR's merge — including two of Agent B's BE-4 integration tests that had been green when written. CP-1 had to fold in the one-line fix (`self._final_answer_reprompted: bool = False`) before its own tests could pass. **Rule:** When parallel agents partition `__init__` work by attribute, every PR that adds a `self.X` *reference* in `_loop` must also add the matching `self.X = ...` *initialiser* in `__init__` — even if the brief tells the agent not to touch `__init__`. Initialisers are not optional, they are part of the reference. **Detection (until a CI guard exists):** - After any merge that touches `_loop`, run the full suite from `origin/main` before starting the next branch. A single `AttributeError: 'AgentRun' object has no attribute …` in unrelated tests is the signature. - Grep before pushing: `grep -oE 'self\._[a-z_]+' harness/agent.py | sort -u` should be a subset of `grep -oE 'self\._[a-z_]+ *=' harness/agent.py | sort -u`. **Proposed CI guard (see `claude-progress.txt` → Next steps):** an AST-based unit test that walks `AgentRun.__init__` for `self.X = …` assignments and `AgentRun._loop` for `self.X` reads, then asserts the read set is a subset of the assign set. **Generalised lesson:** "Disjoint file edits" is not the same as "disjoint semantic surface area." When the surface is a single class, partition by attribute ownership *and* require each PR to keep that class internally consistent — references without initialisers are a defect, even if they sit on different lines from someone else's new field. --- ## L-22: A brief's COPY directives can outrun its build-context instructions — flag the gap, don't silently widen scope **Lesson (2026-05-22):** The GM brief (PR #60) told Agent A to add `COPY SKILLS/deg-functional-enrichment/SKILL.md /workspace/skills/...` to `docker/Dockerfile` *and* explicitly forbade touching `scripts/run_eval.py`, `README.md`, or any file outside a 6-item allow-list. But the `COPY SKILLS/...` paths only resolve when the Docker build context is the repo root — they fail with "SKILLS/...: not found" when the context is `docker/`, which is exactly what `run_eval.py:ensure_docker_image()` and the README quick-start pass. The brief itself referenced the old invocation (`docker build -t bio-mystery-bench:latest docker/`) in its own verification steps, so the contradiction was internal to the brief. The temptation was to "just fix" `ensure_docker_image()` to use repo-root context. That would have been correct technically but out of scope per the brief, and it would have created a downstream conflict for Agents B/C, who owned `run_eval.py`. The right call was: 1. Implement the COPY directives literally as specified. 2. Use the new build invocation (`-f docker/Dockerfile .` from repo root) for the local verification rebuild only. 3. Flag the latent bug explicitly in the PR body and in `claude-progress.txt` → Next steps, with a one-line repro: the next `docker image inspect` failure will trigger a build that errors out until `ensure_docker_image()` and the README are updated. The cached image kept live runs working in the meantime, so the latent bug was bounded — not a regression, just a deferred fix. **Rule:** When a multi-agent brief constrains your file list, and you discover that its own instructions create a latent inconsistency in code you cannot touch, do **not** silently widen scope. Resolve in this order: 1. Pause and ask the user before implementing — present the contradiction and the resolution options. (Agent A did this for GM-5; the user confirmed the literal- COPY-directives approach.) 2. Implement the brief's formal directive (the COPY paths), not its colloquial command (`docker build ... docker/`). Formal artefacts are unambiguous; commands in prose are often shorthand. 3. Verify locally using the corrected invocation. 4. Document the gap in the PR body and in `claude-progress.txt` → Next steps as a distinct, prioritised follow-up. If the gap is a latent bug (will break under some specific condition), say what that condition is so the next agent can triage urgency. **Detection:** When a brief lists allowed files but the work touches Docker build inputs, grep the codebase for every existing `docker build` invocation and confirm they are compatible with the new Dockerfile's COPY paths: ```bash grep -rn "docker build\|dockerfile_dir\|build_image" scripts/ README.md harness/ ``` If any invocation would break, that is the gap to flag. **Generalised lesson:** "Don't modify outside the allowed list" is a real constraint in multi-agent workflows — it protects the orchestration's conflict-avoidance plan. But it is the *agent's* responsibility to recognise when respecting the constraint creates a latent bug, and to make that bug visible to humans and to the next agent. Silent compliance + silent breakage is the worst outcome; loud compliance + flagged follow-up is the right one. --- ## L-23: Parallel agents adding test classes at a shared insertion site will conflict — concatenate, don't re-order **Lesson (2026-05-22):** During the parallel Qwen3 post-mortem, both Agent A (GM) and Agent C (CP) appended a new test class to `tests/test_agent_helpers.py` immediately after `TestBlastToolDefinition::test_extra_args_property_present` — the natural insertion point because it was the file's tail. Each PR was clean against its base. When Agent A's PR #60 was rebased onto main (after CP PR #59 had merged), git flagged a conflict in exactly those lines: both branches had inserted a new `# ---------------------------------------------------------------------------` header and a new class definition where the previous EOF had been. The conflict was trivial to resolve — both blocks should coexist; neither replaces the other. The agent kept Agent A's GM blocks first (already in the branch) and appended Agent C's `TestCriticPromptAlternatives` block after them. Tests went from 296 to 296 (one rebase commit, no test count change). **Rule:** In a multi-agent batch, parallel agents adding test classes to the same test file should expect a conflict at the shared insertion site (almost always EOF or a section boundary). When resolving: 1. Both blocks coexist — neither side wins. 2. Order doesn't matter functionally, so just concatenate in the order the conflict shows them, with a single `# ---` separator between adjacent classes. 3. Re-run the full suite immediately after resolving. The count should be the union of both sides' new tests; if it's lower, a class was accidentally dropped during marker removal. **Detection (proactive):** Before opening a parallel-agent PR, check whether other agents in the same batch are also adding to the same test file: ```bash git log --oneline origin/main..HEAD -- tests/test_agent_helpers.py | head -5 git log --oneline HEAD..origin/main -- tests/test_agent_helpers.py | head -5 ``` If both show new commits, expect a conflict at rebase time. Pre-emptively merging main before pushing surfaces it earlier (and avoids a force-push to resolve). **Generalised lesson:** When parallel agents share a "natural" insertion point in a file (EOF, section end, last-of-its-kind class), the safe assumption is that they will all pick that same point. Brief authors can pre-empt this by assigning explicit insertion anchors per agent ("Agent A: after `TestBlastToolDefinition`, Agent C: before `TestExtractText`"), but in the absence of such guidance, expect the conflict and resolve it by stacking. --- ## L-24: FA structural gap — `abort` tool and step-limit-during-`tool_use` bypass the `end_turn` FA check **Lesson (2026-05-25 — SL slice):** The FINAL ANSWER marker enforcement (FA-2) only fires on `end_turn` responses. Two paths completely bypass it: 1. **`abort` tool** — when the agent calls `abort`, `_handle_abort()` returns immediately with `status="resource_abort"` and no FA check is applied. This is intentional — abort runs don't produce a final answer. 2. **Step-limit during `tool_use`** — when `max_steps` is reached at the top of the while loop, the run terminates with `status="max_steps"` before the agent produces an `end_turn` response. Confirmed in RERUN-5: `recq_a1` ended mid-tool-use (step limit hit during `pip install`), leaving no extractable final answer. **SL remediation (PR for SL slice):** When `self.steps >= self.config.max_steps` AND `response.stop_reason == "tool_use"` AND `not self._step_limit_prompted`: 1. Set `self._step_limit_prompted = True`. 2. Append `STEP_LIMIT_PROMPT` as a user message after the tool results. 3. Make one extra `client.chat()` call with `max_tokens=512`. 4. If the extra call returns `end_turn` → apply a one-shot FA check and return `status="success"`. 5. If the extra call returns `tool_use` again → return `_result("max_steps", start)` (guard fires). **Key design choice:** The extra call is handled inline inside the `tool_use` branch, not via `continue` + the top-of-loop step check. This avoids the "steps > max_steps at top of loop" problem that would fire immediately before the next API call. **Rule:** Any new `end_turn` path added to `_loop()` must explicitly check `_has_final_answer_marker()` or document why it is exempt (e.g. `abort` is exempt by design). Similarly, any new early-exit path that bypasses `end_turn` must be audited for whether FA enforcement is needed there. --- ## L-25: Suffix-injection patterns break tests that assert exact equality on composed strings **Lesson (2026-05-25 — CA slice):** CA-3 wired `_load_critic_skill()` into `_run_critic()` so that the system prompt passed to the critic became `CRITIC_SYSTEM_PROMPT + skill_body` instead of `CRITIC_SYSTEM_PROMPT` alone. This immediately broke `TestCriticMultiRound.test_second_critic_uses_followup_prompt`, which asserted `captured_system == CRITIC_SYSTEM_PROMPT`. The extra skill body appended after the base constant caused the equality to fail — even though the test's intent (confirm the right base prompt was used) was satisfied. **Pattern:** Any time a method changes from returning/passing a *constant* to returning a *constant + dynamic suffix*, every test that compares against the constant with `==` will fail, even though the semantic intent is unchanged. **Rule:** When writing tests for methods that produce a string *based on* a known constant, prefer `str.startswith(CONSTANT)` or `CONSTANT in result` over `result == CONSTANT`: ```python # Fragile — breaks if the implementation appends extra content: assert captured_system == CRITIC_SYSTEM_PROMPT # Robust — survives suffix injection: assert captured_system.startswith(CRITIC_SYSTEM_PROMPT) ``` Apply the same principle to `messages[0]["content"]` assertions when the content might be augmented (e.g. environment context prepended, skill listing appended). **Corollary:** When adding a suffix-injection feature (e.g. `_load_critic_skill()`, `_get_environment_context()` appending skill listing), immediately grep for tests that assert equality on the string being extended and convert them to `startswith` or `in` checks: ```bash grep -n "== CRITIC_SYSTEM_PROMPT\|== CRITIC_FOLLOWUP_PROMPT\|== system_prompt" tests/ ``` **Cross-references:** L-19 (new behavioral constraints on end_turn break existing mock fixtures) — both stem from the same root cause: adding code to a path that existing tests assumed was simpler than it now is. --- ## L-26: Bash pipeline exit codes are silently swallowed — always use pipefail or pre-check **Lesson:** In bash, `cmd1 | cmd2` returns the exit code of `cmd2`, not `cmd1`. When the harness runs `blastn ... | tee file` via `container.exec_command()` and `blastn` is missing (rc=127), `tee` still exits 0 — so the harness sees `rc=0`, treats empty stdout as "no hits", and never surfaces the "command not found" error. **The incident (RERUN-6, 2026-05-26):** BLAST+ was never added to the Dockerfile. Every `blast_search` harness tool call returned "No hits at default parameters" on all five hb002 attempts. Agents wasted 20–40 steps retrying BLAST variants before falling back to unreliable manual 16S motif matching. Result: hb002 0/5 even though the genome matches Bacillus licheniformis at 100% identity (confirmed via `Bio.Blast.NCBIWWW`). **Rules:** 1. **Never pipe harness-dispatched commands without `set -o pipefail`** (or equivalent). Prepend `set -o pipefail; ` to any command string passed to `exec_command()` that uses a pipe, so upstream failures propagate. 2. **Pre-check binary availability before running** — call `tool -version` first; if it fails, return a clear error immediately rather than running and misinterpreting empty output. 3. **Any new tool added to the harness that wraps a binary must also add that binary to the Dockerfile** — don't assume it's present. Add a smoke test assertion for `binary -version` returning rc=0. 4. **CI guard:** add a `_preflight_container_tools()` check to `scripts/run_eval.py` that starts a throwaway container and verifies each required binary before committing to an eval run (BF-3). **Pattern (from harness/agent.py blast_search dispatch after BF-2):** ```python # Pre-check before running — surfaces missing binary immediately version = _get_blast_version(container, program) if not version: return f"BLAST binary '{program}' not found — install with: micromamba install -c bioconda blast" # Use pipefail so the pipe propagates non-zero exit from the binary command = f"set -o pipefail; {program} -db {database} ... | tee {out_file}" ``` **Codebase sweep (2026-05-26):** no other harness tool uses piped `exec_command()` calls — the `blast_search` dispatch was the only instance. All bash tool calls surface exit codes via `_format_result()`. Still, apply rule 1 proactively to any future tool that pipes. --- ## L-27: macOS VirtioFS EDEADLK — copy data into overlay FS, do not bind-mount **Lesson (2026-05-28 — IO-1, PR #85):** Docker Desktop on macOS uses **VirtioFS** for bind mounts. On long-running sessions VirtioFS can enter a broken state where file metadata is visible (file sizes appear correctly in `ls -l`, `du` shows non-zero size) but every read syscall returns errno 35 "Resource deadlock avoided" (EDEADLK). The harness saw this as agents failing immediately with "Resource deadlock avoided" on every read of the problem's data files. **Fingerprint:** `ls -lh /workspace/data` inside a broken VirtioFS container prints `total 0` as the block count even though file sizes are non-zero. This is the earliest reliable indicator — check it before assuming the data itself is corrupt. **Root cause:** VirtioFS is the macOS Docker Desktop filesystem virtualization layer. It proxies filesystem calls between the container and the host. Under load or after many hours of uptime it can enter a deadlock state affecting specific mount points. **Why `/workspace/scratch` was immune:** The scratch directory was created fresh for each container run (new UUID path each time). VirtioFS's deadlock was mount-point-specific; a fresh bind mount started in a healthy state. But `/workspace/data` was always mounted from the same host path (the problem's extracted data directory), so it stayed broken across restarts. **Fix (IO-1, `harness/container.py`):** Remove the read-only bind mount for `data_dir` entirely. After `container.start()` launches the container, call `_copy_data_to_container()`: build a tar archive in memory from `data_dir` and call `self._container.put_archive("/workspace/data", buf)`. The Docker daemon writes the archive directly into the container's **overlay FS** — which lives inside the Docker VM's virtual disk and is never accessed through VirtioFS. From the container's perspective `/workspace/data` is a normal directory with no host involvement. **Rule:** - **Never bind-mount data files that are read-only from the container's perspective.** Use `put_archive` into the container's overlay FS instead. - **The scratch mount (read-write, per-run)** is still a bind mount — it must be accessible from the host for artifact collection. The VirtioFS bug has not affected scratch mounts because they are fresh per run. - **Diagnosis:** when a container run fails instantly with "Resource deadlock avoided", check `ls -lh` block count inside the container. `total 0` = VirtioFS deadlock, not data corruption. **Detection:** ```bash # Inside a failing container — VirtioFS deadlock fingerprint: ls -lh /workspace/data # shows: total 0 (but file sizes are non-zero) # Healthy container (overlay FS via put_archive): ls -lh /workspace/data # shows: total 8.0K (or similar — non-zero blocks) ``` **Cross-reference:** §L-26 (bash pipeline exit codes silently swallowed) — both stem from the same pattern: silent failure that produces misleading "success" signals. --- ## L-28: SKILL files must not include problem-specific prompts **Rule:** New or updated SKILL files must not include problem-specific prompts. Prohibited content: specific gene names, pathway names, organism names, or methodological guidance that applies to a particular benchmark question rather than as a general approach to that biological question type. **Why:** Problem-specific SKILL content artificially inflates performance on seen questions (the agent essentially has the answer pre-loaded) but does not generalise to unseen questions of the same type. This makes benchmark results misleading. **Examples of what NOT to include:** - "Use 16S rRNA BLAST to identify Bacillus species" (problem-specific organism + gene) - "Look for TP53 mutations in the DEG list" (problem-specific gene) - "The heat-stress response involves HSP70 genes" (problem-specific answer) **Examples of what IS acceptable:** - "Use `blastn` with `extra_args='-evalue 1'` to relax the hit threshold" (general parameter advice) - "For enrichment analysis, use ORA/GSEA against MSigDB hallmarks or GO" (general method) - "hg38/mm10 are the standard assemblies for human/mouse ChIP-seq data" (general methodology) **How to apply:** When writing or updating a SKILL file, ask: "Does this recipe work for *any* question of this biological type, or only for the specific question I just solved?" If the latter, generalise or omit it. **Cross-reference:** §L-12 (empty BLAST results vs missing binary) — the 16S BLAST recipe removed from blast-search SKILL.md in BP-4 was the motivating case. --- ## L-29: For gpt-oss-120b via Cerebras, msg.content is empty during tool calls — reasoning is in msg.reasoning **Lesson (2026-05-28):** After the hb002 validation run with gpt-oss-120b on Cerebras, all trajectory files showed blank "Agent reasoning" sections. The root cause: gpt-oss-120b uses OpenAI's Harmony format, which routes output to three channels — `analysis` (CoT), `commentary` (tool calls), and `final` (text response). The Cerebras Chat Completions API collapses these: `msg.content` carries only the `final` channel text, which is always empty during tool-call steps. The CoT is returned separately in `msg.reasoning` (Cerebras `text_parsed` format). **Rule:** - For gpt-oss-120b via Cerebras, always read `getattr(msg, "reasoning", None)` in addition to `msg.content`. Never treat `msg.content = ""` during tool calls as "the model produced no reasoning." - When using the Chat Completions endpoint (not native Harmony format), pass `reasoning_effort="high"` as an API-level parameter to activate maximum CoT depth. The Harmony-format equivalent (`Reasoning: high` in the system message) can be added as belt-and-suspenders but is not the primary mechanism via Chat Completions. - Providers that handle Harmony internally (Cerebras, Ollama, vLLM, HuggingFace) do not require any developer-side tokenisation of Harmony delimiters. **Detection:** ```python # In openai_response_to_llm_response(), always extract reasoning alongside content: reasoning = getattr(msg, "reasoning", None) or getattr(msg, "reasoning_content", None) or "" ``` **Cross-reference:** §L-26 (BLAST silent failure patterns) — a similar "empty output ≠ no result" diagnostic trap. --- ## L-30: Defensive getattr() needed when Provider methods are tested via __new__ **Lesson (2026-05-28):** When new instance variables are added to a Provider class `__init__` (e.g. `self._reasoning_effort`), existing tests that construct the Provider using `OpenAIProvider.__new__(OpenAIProvider)` bypass `__init__` entirely. Any method that reads those variables will raise `AttributeError` on those test instances. **Rule:** In any `Provider.chat()` or other method that reads an instance variable that was added after the class was first written, use `getattr(self, "_var_name", default)` rather than direct attribute access. This makes the method forward-compatible with `__new__`-based test stubs without requiring every test fixture to be updated. **How to apply:** ```python # Fragile — breaks tests that use __new__: if self._reasoning_effort: ... # Defensive — works with __new__ stubs: effort = getattr(self, "_reasoning_effort", "") if effort: ... ``` **Detection:** ```bash # If tests for a Provider method suddenly fail with AttributeError after adding __init__ vars: grep -n "__new__(OpenAIProvider)\|__new__(AnthropicProvider)" tests/ # These test stubs need either: (a) the attribute set explicitly, or (b) the method uses getattr() ``` **Cross-reference:** §L-21 (AgentRun.__init__ coordination) — a similar class where new attributes added by one agent need the CI guard. --- ## L-31: When remote BLAST times out, use NCBI efetch — do NOT download a genome and run minimap2 **Lesson (2026-05-29 — hb002 revalidation attempt 0):** When remote BLAST times out (rc=-1) twice in a row, the agent fell back to downloading the E. coli K-12 genome (~4.6 Mb), building a local BLAST database, running minimap2, and interpreting the alignment against E. coli as a species confirmation — concluding "Escherichia coli" for a genome that is actually *Bacillus licheniformis* (46% GC vs E. coli's 51%). **Why the fallback was wrong:** minimap2 alignment percentage (PAF `de` field) is an alignment *quality* metric, not a taxonomic identity metric. Any two bacteria of similar genome size will produce some alignment. The model saw a PAF output with mapped reads and inferred species identity — a category error. The actual identity came only from the BLAST hit accession fetched via NCBI efetch. **The correct fallback chain (in order):** 1. **If BLAST succeeded but Species = N/A**: read `/workspace/scratch/blast_results.txt` for the top accession ID and run `curl efetch` to get the GenBank record organism name: ```bash curl -s "https://eutils.ncbi.nlm.nih.gov/entrez/eutils/efetch.fcgi?db=nuccore&id=&rettype=gb&retmode=text" | head -20 ``` 2. **If BLAST timed out (rc=-1)**: try `Bio.Blast.NCBIWWW.qblast()` from within the container — this uses the Python NCBI BLAST API which has a different timeout path than the command-line `-remote` flag. 3. **Never**: download a reference genome, build a local DB, and run minimap2 or local blastn to "confirm" species. A 21% minimap2 divergence rate (`de:f:0.21`) does not mean ~79% identity — it includes indels and structural variation that are meaningless for species-level classification. **Detection in trajectories:** If an agent downloads a genome (`wget` + `*.fna`), builds a blast database, and then runs `minimap2 ... | tee alignment.bam`, it has likely fallen into this trap. The correct signal is `curl efetch` after a successful (even N/A-species) BLAST hit. **Root cause of attempt-0's timeout:** Remote NCBI BLAST server load varies by time of day; the 474 bp and 150 bp queries both hit the 600s per-command wall. Attempt-1 ran later and the same 500 bp query completed in ~210s. The blast-search SKILL.md's "Try a different region, not a larger one" guidance was present but the agent tried a smaller region (150 bp) rather than switching to the efetch fallback path after the first timeout. **Cross-reference:** §L-26 (bash pipeline exit codes silently swallowed) — both arise from misinterpreting tool output. --- ## L-32: Distinguish "BLAST hit with N/A species" from "BLAST timeout" — they require different fallbacks **Lesson (2026-05-29 — BT-5, PR #91):** Two BLAST failure modes are easy to conflate in guidance text but require completely different recovery paths: 1. **rc=0, Species = N/A**: BLAST ran, found hits, and returned accession IDs — but the `sscinames` NCBI column is absent for newer assemblies. Recovery: read the accession from `/workspace/scratch/blast_results.txt` and run `curl efetch` to fetch the GenBank record's `SOURCE: ORGANISM` line. This works because a successful hit exists. 2. **rc=-1, no results**: BLAST network stalled — no hits, no accession IDs returned. Recovery: try `Bio.Blast.NCBIWWW.qblast()` (the Python NCBI BLAST API, which uses a different HTTP timeout path than the `-remote` flag) or `sleep 290` and retry with a ≤150 bp query. `curl efetch` is useless here — there is no accession to look up. **Rule:** Never propose `curl efetch` as a fallback for rc=-1 timeouts. Efetch requires a BLAST-returned accession ID, which only exists after a successful (rc=0) BLAST call. `Bio.Blast.NCBIWWW.qblast()` is the correct timeout fallback. **Root cause of the confusion in BT-5:** The hb002 revalidation attempt-0 failure was documented in §L-31 as "use efetch". That guidance correctly applies to the N/A-species case but was written into the rc=-1 message — the wrong failure mode. BT-5 fixed the rc=-1 message to point to `qblast()` and removed the efetch suggestion from both the error message and the system prompt (efetch will get its own SKILL file later). **Cross-reference:** §L-31 (do NOT use minimap2 for species identification). --- ## L-33: RERUN-7 (gpt-oss-120b, 5×5) — BLAST fixes vindicated; hb022 model-level bias **Lesson (2026-05-29 — RERUN-7, results/gpt-oss-rerun7-5x5/):** Full 5×5 benchmark (hb002/hb022/hb053, 5 attempts each, gpt-oss-120b, `reasoning_effort=high`, two-round critic) completed. **Results (all 5 preview problems):** | Problem | human_solvable | Correct | pass@1 | pass@5 | Notes | |---------|---------------|---------|--------|--------|-------| | hb002 | True | 5/5 | ✅ | ✅ | BLAST fixes (BT-1..5) fully working | | hb020 | True | 4/5 | ❌ | ✅ | Attempt 1: Pan troglodytes; attempts 2–5: Homo sapiens | | recq.. | True | 5/5 | ✅ | ✅ | CTCF (ChIP-seq TF identification) | | hb022 | False | 0/5 | ❌ | ❌ | Model-level SLC7A11 bias (see below) | | hb053 | False | 0/5 | ❌ | ❌ | Various wrong stress types; correct: heat stress | pass@1=40%, pass@5=60%, brittle=0%. Human-solvable: 14/15 attempts correct (93%). **vs RERUN-6 (Qwen3/Cerebras, hb002/hb022/hb053 only):** | Problem | RERUN-6 | RERUN-7 | Change | |---------|---------|---------|--------| | hb002 | 0/5 | 5/5 | +5 (BLAST fixes working) | | hb022 | 3/5 | 0/5 | −3 (model-level bias, see below) | | hb053 | 0/5 | 0/5 | unchanged | **hb002 (human_solvable=True):** Perfect 5/5. The BT-1..5 fixes (BLAST timeout remediation, qblast fallback, no-minimap2 rule) fully eliminated the BLAST failure mode that caused 0/5 in RERUN-6. Attempt times ranged 136–803 s (2–13 min); BLAST is completing within the per-command timeout. **hb022 (human_NOT_solvable=False) — systematic wrong-half bias in gpt-oss-120b:** All 5 attempts predicted [Sample_09–16]; correct answer is [Sample_01–08]. The agent correctly clusters the 16 samples into two groups but assigns the wrong cluster as "Erastin-treated." The bias stems from how the model interprets SLC7A11 expression direction: it assumes SLC7A11 should be *higher* in Erastin-treated cells (compensatory upregulation hypothesis), but Sample_01–08 actually have *lower* SLC7A11-AS1 expression (≈ −0.3 to −1.9), which is the correct Erastin signature. Sample_09–16 have higher SLC7A11 expression (~+0.3 to +0.6) and are the control group. RERUN-6/Qwen3 got 3/5 correct, suggesting this is a model-level knowledge difference, not a harness issue. Since hb022 is "human_NOT_solvable", no harness fix is warranted. **hb053 (human_NOT_solvable=False):** Agent BLASTs the scrubbed FASTA sequences, identifies gene functions, but consistently assigns wrong stress types (pathogen, drought, phosphate starvation). Correct answer is "heat stress". Step counts high (26–91 steps per attempt). Consistent failure across 5 attempts with different wrong answers suggests the agent is finding some genes enriched for other stress responses and failing to weight HSP/heat-stress markers correctly. No harness fix is warranted; this is a knowledge/reasoning limitation. --- ## L-34: Cerebras run_eval.py CLI flags — three gotchas **Lesson (2026-05-30 — RERUN-7 launch):** Three flag errors hit in quick succession when launching the RERUN-7 benchmark run via `run_eval.py`: 1. **`--problems` does not exist** — the flag is `--problem-ids`. 2. **`--attempts` does not exist** — the flag is `--n-attempts`. 3. **`--problem-ids` is comma-separated, not space-separated** — passing multiple IDs space-separated (e.g. `--problem-ids hb002 hb022`) raises "Got unexpected extra arguments"; the correct form is `--problem-ids hb002,hb022,hb053`. 4. **Cerebras API key is not auto-discovered** — the `openai` provider looks for `OPENAI_API_KEY` in the environment, but Cerebras uses `CEREBRAS_API_KEY`. Always pass `--api-key "$CEREBRAS_API_KEY"` explicitly when using Cerebras. **Correct Cerebras invocation:** ```bash set -a && source .env && set +a && echo "y" | python3.12 scripts/run_eval.py \ --problem-ids hb002,hb022,hb053 \ --n-attempts 5 \ --provider openai \ --model gpt-oss-120b \ --api-base-url https://api.cerebras.ai/v1 \ --api-key "$CEREBRAS_API_KEY" \ --reasoning-effort high \ --critic-injection-points after_final_answer \ --critic-injection-points after_critic_response \ --results-dir results/ ``` **Rule:** When in doubt, run `python3.12 scripts/run_eval.py --help` first to verify flag names.