Skip to content

fix(benchmarks): isolate agent-eval user config - #1758

Open
ducanhnguyen223 wants to merge 5 commits into
Nano-Collective:mainfrom
ducanhnguyen223:codex/1744-agent-eval-config-isolation
Open

ducanhnguyen223 wants to merge 5 commits into
Nano-Collective:mainfrom
ducanhnguyen223:codex/1744-agent-eval-config-isolation

Conversation

@ducanhnguyen223

@ducanhnguyen223 ducanhnguyen223 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Give every agent-eval run a fresh NANOCODER_CONFIG_DIR so preference writes are temporary and user-global settings do not enter the run.
  • Copy only provider configuration into the temporary directory; when a provider is selected, copy only matching provider entries. Keep provider environment overrides available and drop environment-based MCP server settings.
  • Add an offline stub-CLI regression test and document the isolation behavior.

Fixes #1744

Validation

  • pnpm exec ava benchmarks/agent/harness.spec.ts — 29 passed.
  • pnpm run test:types and pnpm run test:types:vscode — passed.
  • pnpm run test:format and pnpm run test:lint — completed with Biome emitting an internal UTF-8 diagnostic for source/acp/acp-agent.ts.
  • Changeset status confirms one @nanocollective/nanocoder patch bump; hosted Changeset Validation passes.
  • git diff --check — passed. No live model/provider calls were made.
  • Full pnpm run test:all was attempted on macOS but is not green locally: AVA reported 53 failures and 15 skips. Failures include the local ink package not exporting Text in CLI integration tests and macOS /var versus /private/var temporary-path comparisons. Hosted CI is revalidating the rebased head.

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 1 nit

@ducanhnguyen223 — a few things worth a look, none blocking.

PR #1758 fixes #1744 by giving each agent-eval run a private NANOCODER_CONFIG_DIR and copying only the requested provider block from the user's agents.config.json; MCP server env vars are stripped, preferences written by the CLI are discarded with the temp dir, and the original user files are left untouched. The harness-side implementation in harness.ts (copyProviderConfig, filtered provider write, env construction, finally cleanup) and the new stub-CLI regression test in harness.spec.ts both look correct and the test exercises the actual failure modes the issue described. The change is ready apart from a missing changeset entry, which the project's policy and prior precedent (the original agent-eval-harness.md minor bump, plus sibling fixes like dirty-hotels-care.md for tooling) require.

🟠 important · changeset · .changeset/

No changeset is included in this PR. Per CONTRIBUTING.md and .changeset/config.json, every user-facing PR needs a .changeset/*.md entry whose package name resolves against the workspace. The original benchmarks/agent/ introduction shipped .changeset/agent-eval-harness.md as a minor bump, and similar bug fixes to dev tooling (e.g. .changeset/dirty-hotels-care.md) ship patch changesets. This PR fixes a bug that affects any contributor running pnpm run test:agent-eval (it was rewriting their saved default model and running their global hooks), updates the README to document the new isolation behavior, and resolves an issue labelled bug — that meets the bar for a patch changeset. Suggested entry:

---
'@nanocollective/nanocoder': patch
---

Fix `benchmarks/agent` runs from leaking into the user's real config: each run now gets a private `NANOCODER_CONFIG_DIR` containing only the requested provider block, MCP server env vars are stripped, and any preferences the CLI writes are discarded with the temp dir. Thanks to @ducanhnguyen223. Closes #1744.

⚪ nit · tests · benchmarks/agent/harness.spec.ts:342

The new isolation test exercises the happy path (provider name matches, source agents.config.json exists, MCP env vars set) but not the early-return branches of copyProviderConfig: missing source file, malformed JSON, or a --provider value with no matching entry. The first two are benign edge cases, and the third is the more interesting one — it currently means the CLI is spawned with NANOCODER_CONFIG_DIR pointing at an empty temp dir and would fail to resolve the model. Worth a follow-up assertion that the missing-match case surfaces a clear harness error rather than a misleading spawn failure, but not blocking.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 14, 2026
@ducanhnguyen223
ducanhnguyen223 force-pushed the codex/1744-agent-eval-config-isolation branch from 4f65e80 to 15936fa Compare October 15, 2026 04:04
@ducanhnguyen223

Copy link
Copy Markdown
Contributor Author

Thanks for the review. The current head 15936fa includes a patch changeset for the user-facing isolation fix. It also returns a clear validation failure when the requested provider is not configured, with a regression test confirming the CLI is not spawned for that case. All 17 checks on the current PR head are passing. When convenient, could a maintainer review the updated head?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] The agent-eval harness runs against the user's real config: it rewrites their saved default model and runs their global hooks

1 participant