feat(evals): add Pier/Harbor adapters and Deep SWE benchmark suite

[flora131]

Summary

Introduces an `evals/` harness for benchmarking Atomic against the Deep SWE task suite via Pier, with a full Pier-native installed-agent adapter, a Harbor-compatible legacy adapter, and a fix for duplicate reftable footer refreshes in the coding agent.

Changes

Pier adapter (evals/atomic_pier.py)

• Implements Atomic(BaseInstalledAgent) with ATIF support; installs Atomic (via npm) and rg/fd search tools inside Pier sandboxes during setup
• Supports version kwarg to select @latest, @next, or a pinned npm version
• Forwards credentials across 10 providers (Anthropic, OpenAI, GitHub Copilot, Google/Vertex, AWS Bedrock, Groq, Mistral, HuggingFace, OpenRouter, xAI) and derives network allowlists from active provider keys
• Routes GitHub Copilot API targets from COPILOT_API_TARGET, GITHUB_COPILOT_BASE_URL, or GITHUB_SERVER_URL — handles github.com, GHEC tenants, and GHES domains; guards against 421 Misdirected Request
• Streams Atomic's JSON event log to /logs/agent/atomic.txt; collects per-turn usage (input/output/cache tokens, cost) with deduplication across the main session and subagent/workflow sessions
• Exports full Pier Trajectory objects (including subagent trajectories), FinalMetrics (aggregated token counts, cost, peak context, summarization count), and populates AgentContext fields

Harbor adapter (evals/atomic_harbor.py)

• Adds a Harbor-compatible Atomic(BaseInstalledAgent) for legacy evaluation flows
• Installs Atomic via NVM + npm, handles the thinking CLI flag, and tees JSON output to a log file

Evaluation workspace

• Adds evals/deep-swe Git submodule pointing to the Deep SWE task suite
• Adds evals/pyproject.toml and evals/uv.lock (uv Python project) with pier and harbor as dependencies
• Adds evals/.python-version pin and .gitignore updates for the evals workspace

Documentation (evals/README.md)

• Documents how to run pier run with the Atomic adapter, select versions, pass Copilot credentials, and force COPILOT_API_TARGET to resolve 421 errors across github.com, GHEC, and GHES endpoints

Bug fix — reftable footer refresh deduplication (packages/coding-agent)

• Eliminates duplicate footer branch refreshes from unchanged tables.list content: computes a size+mtime+ctime+content fingerprint and skips scheduling when the fingerprint matches the last observed value
• Stores the watchFile listener reference so unwatchFile can unregister the exact listener on teardown, preventing listener leaks across watcher resets
• Adds test coverage for the deduplication logic in footer-data-provider.test.ts

Notes

• No changes to existing packages; all new evals files are confined to evals/
• The Harbor adapter is a standalone legacy path and does not depend on the Pier adapter
• The reftable fix is a self-contained improvement to footer-data-provider.ts with no API changes

[claude]

PR Review: feat(evals): add Atomic Pier evaluation adapter

Thanks for this — the harness is well thought out. The provider routing, Copilot/GHES base-URL resolution, and the trajectory/usage export (including workflow + subagent sessions) are all nicely handled, and the README is genuinely useful. A few things worth addressing before merge.

🔴 .gitignore scope — likely the most important issue

The PR appends the full GitHub Python .gitignore template (~220 lines) to the repo-root .gitignore. Several of those patterns are unanchored and match directories at any depth: lib/, build/, dist/, target/, var/, parts/, plus broad globs like *.log, *.so, *.spec, *.pid.

This monorepo already tracks lib/ directories, e.g.:

• .atomic/workflows/lib/*.ts
• packages/workflows/skills/impeccable/scripts/lib/*.mjs

Existing tracked files stay tracked, but new files added under any lib/, build/, dist/, etc. directory would now be silently git add-ignored — a real footgun for contributors. *.log is also broad for a repo of this size.

Recommendation: put these rules in an evals/.gitignore, or anchor them to the eval workspace (/evals/...), rather than dumping the Python template at the repo root. Also: the file ends without a trailing newline.

🟡 Code duplication between the two adapters

atomic_harbor.py and atomic_pier.py carry ~200 lines of byte-identical helpers: _token_count, _cost_total, _assistant_message_fingerprint, _read_session_header/classification, the dedup logic, and the bulk of populate_context_post_run. Only the framework imports (harbor.* vs pier.*) differ, and that helper logic is framework-agnostic. Extracting a shared _atomic_usage.py would keep the two in sync as the accounting evolves.

🟡 populate_context_post_run asymmetry

In atomic_pier.py the method is not decorated @override and is invoked manually at the end of run(). In atomic_harbor.py it is @override (framework-invoked). If Pier's base class also invokes this hook, it will run twice. It's idempotent today (totals are assigned, not accumulated, and the dedup sets are local per call), so it's harmless — but the asymmetry is confusing. Worth a comment confirming intent, or aligning the two.

🟡 Tests / validation

• There are no tests for the usage/trajectory parsing, which is the most intricate part (dedup by id and fingerprint, multi-session counting, content/tool-call splitting, ms to ISO timestamps, cost summing). A handful of cases over small fixture JSONL files would de-risk the math considerably. The repo has no Python test runner configured (bun:test won't cover .py), so this introduces a parallel ecosystem — worth a README line on how to run/lint the Python side.
• The PR description notes no validation was run, and it pulls a git-sourced datacurve-pier plus a deep-swe submodule. Clones now need --recursive, and a quick end-to-end smoke run would be reassuring given nothing was executed.

🟢 Minor

• parse_version: stdout.strip().splitlines()[-1] raises IndexError on empty output. Guard the empty case.
• isinstance(value, int | float) treats bool as numeric (isinstance(True, int) is True). Unlikely in usage fields but a latent surprise if a flag ever lands there.
• The run() pipeline atomic ... 2>&1 | grep -v ... | tee has no set -o pipefail (unlike install_spec), so the pipeline exit code is tee's — Atomic's own crash/exit status is masked. If you want the eval to distinguish "agent crashed" from "agent ran but didn't solve," consider capturing Atomic's status explicitly.
• requires-python = ">=3.14" / .python-version = 3.14 is bleeding-edge; make sure runner images have it.
• atomic_pier.py is 742 lines. The repo's 500-line gate only covers .ts/.js/.rs so this won't fail CI, but the convention still applies — splitting the trajectory builder out would help.
• Stray double blank line after run() in atomic_pier.py.

Security

Looks reasonable: credentials are read from host env and passed via the env dict (not argv), and models.json only stores the base URL. One caveat — the full JSON stream is tee'd to atomic.txt, so anything the model echoes (including secrets) lands in logs. Standard eval tradeoff, just noting it.

Overall this is a solid, isolated addition. The .gitignore scoping is the one I'd treat as a blocker; the rest are quality/maintainability improvements.

Reviewed against the repo's CLAUDE.md conventions.

[claude]

PR Review — feat(evals): add Atomic Pier evaluation adapter

Thanks for this! The PR is well-structured and the description is excellent. The evals harness is cleanly confined to evals/ and the one published-package change (footer-data-provider.ts) is small, focused, and tested. Overall this looks solid. Notes below, ordered by impact.

🔵 Published-package change: footer-data-provider.ts (the part that ships to users)

This is the highest-leverage change since it lands in @bastani/atomic. The logic is correct and the test is good:

• ✅ The fingerprint (size:mtimeMs:ctimeMs:content) correctly guards against content-collision AND same-content-different-mtime, so genuine reftable updates are not dropped.
• ✅ Switching unwatchFile(path) to unwatchFile(path, listener) is a real correctness improvement — the old form removed ALL watchFile listeners on that path, including unrelated ones. Good catch.
• ✅ Falling through to scheduleRefresh() when the fingerprint is null (file missing / read error) is the right safe default.

Two small things worth confirming:

1. Full-file read on every watcher event. readReftableTablesListFingerprint() does a synchronous readFileSync of the whole file on each event. tables.list is tiny so this is fine in practice, but a one-line comment noting that assumption would help, since the dir watcher can fire frequently during compaction.
2. tables.list created after setup. When the file does not exist at setupGitWatcher time, only the directory watcher (not the dedicated file/watchFile watchers) will ever catch it. That matches prior behavior, but subsequent modifications then rely solely on the dir watcher debounce — just calling it out.

🟡 Test coverage gap: the Python adapters are untested

The bulk of new code (atomic_pier.py, 742 lines) contains non-trivial parsing/aggregation logic — fingerprint dedup, trajectory assembly, per-turn usage accounting across main + subagent + workflow sessions — and has NO tests. This is exactly the kind of logic that silently drifts (e.g. a schema field rename in the JSON event log would quietly zero out token/cost metrics). Even a few pure-function unit tests over _metrics_from_usage, _cost_total, _assistant_message_fingerprint, and _should_count_session_file against small fixture transcripts would protect the reporting pipeline cheaply.

🟡 Potential bug: None entries passed to allowlist_from_urls

In network_allowlist(), urls = [self._get_env(key) for key in self._BASE_URL_ENV_KEYS] builds a list that includes None for every unset base-URL env var, then passes it straight to allowlist_from_urls(urls, ...). If that helper does not tolerate None, this throws at runtime whenever any base-URL var is unset (the common case). Recommend filtering: urls = [u for key in self._BASE_URL_ENV_KEYS if (u := self._get_env(key))].

🟡 Minor security: unquoted version kwarg in the install command

version_spec = f"@{self._version}" is interpolated into the npm install -g @bastani/atomic{version_spec} shell command without sanitization, so a value like 1.0.0; <cmd> would inject. This is operator-supplied eval config (low severity), but since it cannot be cleanly shlex.quoted as a partial token, a cheap validation regex (e.g. allow only [A-Za-z0-9._@/-]) would close the gap. instruction, provider, and model are all correctly quoted — nice.

🟢 Nits / observations

• Duplication between atomic_pier.py and atomic_harbor.py: _token_count, _cost_total, _assistant_message_fingerprint, and most of populate_context_post_run are near-identical. I see the design note that Harbor is standalone legacy, so this is a deliberate trade-off — just flagging that the two copies will drift (e.g. the None-filtering fix above would need applying in both).
• The yum install branch installs only curl git — no ripgrep/fd/nodejs/npm — whereas the README states rg/fd are installed during setup. On RHEL/yum sandboxes Atomic search tooling would be degraded. The apk/apt branches are complete; consider matching yum or documenting the limitation.
• Third-party submodule evals/deep-swe points at github.com/datacurve-ai/deep-swe. Fine for an evals suite, just worth being aware of for reproducibility / supply-chain.
• .gitignore gained the full ~220-line standard Python template. Harmless, just heavier than the few entries actually needed for evals/.
• ✅ CHANGELOG entry is specific and user-meaningful (matches the repo do-not-be-lazy rule).
• ✅ The 500-line file-length gate only covers .ts/.tsx/.js/.jsx/.mjs/.cjs/.rs, so the 742-line .py adapter does not violate it — though it is large enough that splitting trajectory-building out of the adapter would aid readability.

Nothing here is blocking the eval-harness goal; the None-filtering case and the Python test gap are the two I would most want addressed before relying on the reported metrics.

[claude]

PR Review — Pier/Harbor eval adapters + reftable footer fix

Reviewed the eval harness (evals/atomic_pier.py, evals/atomic_harbor.py), the Deep SWE wiring, and the shipped bug fix in footer-data-provider.ts. Overall this is well-structured, defensively-coded work — the token de-duplication and credential/network plumbing are thoughtful. Notes below, grouped by severity. The only changes that touch shipped code are the footer fix and its tests, which look correct and are well covered.

🐛 Correctness

1. Shell-injection inconsistency between the two adapters. atomic_pier.py correctly shlex.quotes provider, model, instruction, session_dir, and output_file. atomic_harbor.py does not quote provider/model — model_args = f"--provider {provider} --model {self.model_name.split('/', 1)[1]} " interpolates raw substrings into the shell command. Low severity (model name is operator-supplied), but it's an inconsistency worth closing so the legacy path matches the hardened one.

2. Fingerprint includes mtime/ctime, which slightly weakens the de-dup goal. readReftableTablesListFingerprint() builds ${size}:${mtimeMs}:${ctimeMs}:${content}. The stated intent is to skip refreshes when tables.list content is unchanged. Because content is already in the key, the real duplicate-watcher-event case (dir watch + file watch + watchFile poll all firing for one write) de-dups correctly — good. But a metadata-only touch (same content, new mtime) will still schedule a refresh, which is the case you set out to suppress. If that's intentional (belt-and-suspenders), fine; otherwise consider keying on size:content only. Not a blocker.

🧹 Maintainability

3. Significant duplication between atomic_pier.py and atomic_harbor.py — _token_count, _cost_total, _assistant_message_fingerprint, and the bulk of populate_context_post_run/session-classification logic are near-identical. Since harbor is available transitively (it's a dependency of datacurve-pier in uv.lock), a shared _atomic_usage.py helper module could hold the common accounting logic and keep the two thin adapters in sync.

4. atomic_pier.py is 742 lines. The repo's 500-line gate (check:file-length) only enforces .ts/.tsx/.js/.rs etc., so Python isn't covered — no rule violation — but the file is large for the spirit of that guideline. The trajectory-building helpers (_split_message_content, _step_from_message, _trajectory_from_entries, …) are a natural module to extract.

5. harbor is a transitive-only dependency. evals/pyproject.toml declares only datacurve-pier; the Harbor adapter works because harbor rides in as a transitive dep. If pier ever drops it, atomic_harbor.py breaks silently. If the Harbor path is meant to be supported long-term, consider declaring harbor explicitly.

📦 Repo / supply-chain

6. Third-party git submodule (evals/deep-swe → datacurve-ai/deep-swe). Pinning to a commit is good. Two things to confirm: (a) main-branch / CI clones don't unexpectedly recurse submodules (build slowdown or hard failure if the repo is private/removed), and (b) the README documents --recurse-submodules. Worth noting the session's working tree showed .gitmodules as deleted, which hints the submodule isn't trivially resolvable in all environments.

7. .gitignore dumps the full GitHub Python template at the repo root (and is missing a trailing newline). Since these artifacts are confined to evals/, scoping this into evals/.gitignore would keep the root ignore file focused on the monorepo.

✅ Tests

8. TS fix is well covered — the new "ignores duplicate reftable watcher events" and "refreshes on unnamed reftable directory watcher events" tests exercise both the de-dup path and the unnamed-filename fallthrough. Nice.

9. The Python adapters have no tests. The usage de-duplication (seen-ids vs. fingerprint fallback) and session-classification logic is the trickiest part of this PR and is exactly the kind of thing that drifts silently. A couple of unit tests over _assistant_message_fingerprint / populate_context_post_run against a small fixture transcript would pay for themselves.

👍 Strengths

• The unwatchFile(path, listener) fix (storing the exact listener for targeted removal) is the right call and cleanly prevents listener leaks across watcher resets.
• Credential forwarding is allowlist-driven per provider and only forwards keys that are actually set — minimal blast radius.
• network_allowlist deriving domains from active provider keys + Copilot target resolution (github.com / GHEC / GHES) is well thought through, and the 421 troubleshooting in the README is a nice touch.
• CHANGELOG entry is specific and follows the repo's format conventions.

Nothing here blocks the footer fix from shipping; items 1, 3, and 9 are the highest-value follow-ups.

[claude]

Review: PR #1539 — Pier/Harbor adapters + Deep SWE suite + reftable footer fix

Thanks for this — the adapters are well-structured and the dedup/fingerprint design (both in the TS fix and the Python usage-accounting) is thoughtful. The instruction is shlex.quote-d before it hits the shell, the Copilot models config is written via a quoted heredoc + json.dumps (so no shell expansion / terminator injection), and the TS change ships with a changelog entry and tests. A few things worth addressing before merge.

High

1. The new .gitignore block is repo-global, not scoped to evals/.
The standard Python .gitignore was appended at repo root, so its bare directory patterns (lib/, build/, share/, var/, parts/, plus *.log) match any such directory anywhere in the tree — not just under evals/. We already have tracked lib/ source dirs, e.g.:

• .atomic/workflows/lib/*.ts
• packages/workflows/skills/impeccable/scripts/lib/*.mjs

Existing tracked files stay tracked, but any new file added under one of those lib/ (or build/) directories would be silently ignored and never show in git status — a footgun for future contributors. *.log similarly swallows logs repo-wide.
Recommend scoping the whole block to the eval workspace: prefix each pattern with evals/, or (cleaner) drop these lines from the root .gitignore and add a dedicated evals/.gitignore. Also note dist/ is now duplicated (it already existed at the top of the root ignore).

Medium

2. The agent exit code is swallowed by the output pipe.
In both adapters the run command ends with atomic ... | grep -v ... | stdbuf -oL tee <file>. atomic is not the last stage of the pipeline and there is no set -o pipefail in the run step (unlike the install step), so a crashed/non-zero atomic invocation looks like success to Pier/Harbor. For an eval harness that can quietly skew pass/fail. Consider set -o pipefail for the run command (and checking ${PIPESTATUS[0]}) — noting that grep -v exiting 1 when it filters everything would then also need handling.

3. Test coverage / dependency for the Python adapters.

• atomic_pier.py is ~740 lines with intricate trajectory reconstruction, dedup-by-id/fingerprint, and session classification, but has no Python tests. A few unit tests over _assistant_message_fingerprint, _cost_total, and _copilot_api_base_url_from_server_url (the github.com / .ghe.com / GHES branches) would lock in the trickiest logic cheaply.
• pyproject.toml only declares datacurve-pier, but atomic_harbor.py imports harbor.*. The PR description says both pier and harbor are deps. Unless datacurve-pier re-exports harbor, the Harbor adapter is not importable in this uv workspace — please confirm it resolves or add the dep.

Low / nits

• atomic_harbor.py does not shlex.quote --provider/--model (model_args = f"--provider {provider} --model ..."), whereas atomic_pier.py does. Operator-controlled input so low risk, but worth making consistent.
• Reftable directory-watcher dedup is string-keyed on filename. handleReftableDirectoryEvent compares filename === "tables.list"; on platforms where fs.watch yields a Buffer/null filename it falls through to the non-deduped scheduleRefresh(). Functionally safe (just less dedup), and the fix targets Windows where filename is a string — fine, just noting it.
• reftableTablesListPath is now assigned before confirming tables.list exists. Verified the teardown guard (if (path && listener)) handles the case where watchFile was never registered, so no spurious unwatchFile/leak — good. The listener-leak fix itself (passing the stored listener to unwatchFile) is not directly covered by a test; the two added tests exercise the dedup paths only.
• _read_session_header (Pier adapter) runs the whole file through _read_jsonl (parsing every line) just to read the first entry, where the Harbor version streams only the first line. Minor, but rglob over many session files can add up.
• Minor PEP8: extra blank line after run() in atomic_pier.py.

Nothing here blocks the eval harness itself; the .gitignore scoping (#1) is the one I would most want fixed since it affects the whole repo, not just evals/.