Skip to content

fix(curator): run the background review thread in a copy of the caller's contextvars - #118200

Open
mgandal wants to merge 1 commit into
NousResearch:mainfrom
mgandal:fix/curator-review-thread-context
Open

mgandal wants to merge 1 commit into
NousResearch:mainfrom
mgandal:fix/curator-review-thread-context

Conversation

@mgandal

@mgandal mgandal commented Sep 21, 2026

Copy link
Copy Markdown

What does this PR do?

Runs the curator's asynchronous LLM review thread in a copy of the caller's contextvars context, so it keeps the profile secret scope on a multiplexed gateway.

run_curator_review(synchronous=False) (the default, and what maybe_run_curator uses) starts a bare threading.Thread. A new thread begins with an empty contextvars context, so the profile secret scope the caller installed is gone by the time the fork calls get_secret("ANTHROPIC_TOKEN"). Under gateway.multiplex_profiles that reader is fail-closed, so the review dies with:

curator: auto: 15 marked stale, 13 archived; llm: error: Hermes could not read this profile's ANTHROPIC_TOKEN (an internal profile-scoping bug on the multiplexed gateway ...

(verbatim from a production gateway log, 2026-09-15; the auto-transitions ran, the LLM consolidation never did.)

#116744 made the housekeeping curator tick run under each served profile's scope, which is exactly what makes this one-line drop visible: the scope is now there at the call site and lost one frame later. #111617 had classified this thread as "process-global by design" because at the time every caller was unscoped; that is no longer true after #116744.

Related Issue

Follow-up to #116744 (housekeeping runs per profile scope). No separate issue filed; happy to open one.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • agent/curator.py: start the curator-review daemon thread via contextvars.copy_context().run instead of a bare target.
  • tests/agent/test_curator.py: test_review_thread_inherits_secret_scope installs a scope with set_secret_scope, runs the review asynchronously with a stubbed _run_llm_review, joins the thread and asserts the stub observed the scope. Red on origin/main (seen["scope"] is None), green with the fix.

How to Test

  1. pytest tests/agent/test_curator.py -q — 38 passed.
  2. On a multiplexed gateway (gateway.multiplex_profiles with at least one served profile), wait for the hourly curator tick or run hermes curator run from a scoped caller. Before: the llm: error: Hermes could not read this profile's ANTHROPIC_TOKEN line above. After: the LLM consolidation pass runs.

Checklist

Code

Documentation & Housekeeping

  • Docs — N/A (inline comment explains the copy)
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform — N/A (stdlib contextvars)
  • Tool descriptions/schemas — N/A

🤖 Generated with Claude Code

https://claude.ai/code/session_01AfnxdXwQoheA3hYjA25Exo

…r's contextvars

Under gateway.multiplex_profiles the profile secret scope is a ContextVar. The
asynchronous curator review started a bare threading.Thread, which begins with an
empty contextvars context, so the fork's first get_secret("ANTHROPIC_TOKEN") was
fail-closed ("could not read this profile's ANTHROPIC_TOKEN") even though the caller
had a scope installed. Start the thread through contextvars.copy_context().run.

Adds test_review_thread_inherits_secret_scope, which installs a scope, runs the
review asynchronously and asserts the LLM pass observed it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AfnxdXwQoheA3hYjA25Exo
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Sep 21, 2026
@Finn763

Finn763 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Independent review — the fix is correct and the test has teeth. One live sibling of the same root cause is still unfixed, and two other sites must not get this treatment.

Verified

  • Red on base confirmed. Applied only the test hunk onto merge-base db1f3f4564eb: 1 failed, 37 passed, with AssertionError: assert None == {'ANTHROPIC_TOKEN': '***'} — exactly the empty-context symptom. On the PR head (64bdf792): tests/agent/test_curator.py 38 passed. Neighbours test_housekeeping_profile_scope.py + test_curator_run_hygiene.py: 7 passed.
  • The test is not a smoke test. It asserts the actual contextvar value the thread observed (seen["scope"] == {"ANTHROPIC_TOKEN": "scoped-token"}) after joining, not merely that a thread started.
  • copy_context semantics are safe here. I looked for the classic landmine — the copy is an immutable snapshot, so a set inside the thread does not flow back and a caller-side set after start() is not visible. agent/curator.py has exactly two matches for .set(|.reset(|copy_context|ContextVar: line 967 (this fix) and line 1080 (a comment). The fork sets its own contextvars inside the copy (_reset_background_review_read_marks()), which is the intent and stays per-run. Results travel by closure (save_state(state2), _notify(on_summary, ...)), not by contextvar, so nothing is lost.
  • Bonus correctness, worth stating in the PR body: the review now also inherits the caller's HERMES_HOME override and terminal policy from _profile_runtime_scope, so config.yaml, the skills tree, the .curator_state path and terminal policy resolve against the same profile whose credentials are in use — instead of the launch process's. That is more correct than the secret-scope fix alone.
  • No new race. Lifecycle is unchanged; the copied Context is never mutated across threads. The real concern (an in-flight thread killed mid-save_state() at exit leaving a partial state file) is pre-existing, not introduced, and the fixture already joins curator-review in teardown (test_curator.py:53-55).

Repo-wide sibling scan — one more live instance

git grep 'threading\.Thread(' is 924 hits, 239 outside tests/evals, 215 in the core runtime packages. Of those, 9 already propagate context, 201 are bare. Filtering for the actual defect signature (a contextvar-backed secret read reachable from the thread target) leaves 4, of which two are correct as-is:

Site Verdict
agent/curator.py:966 this PR's fix
tools/tirith_security.py:469 → _background_install → _install_tirith → _download_file → get_secret("GITHUB_TOKEN") genuine unfixed sibling — ensure_installed() runs from turn-scoped callers, the download thread loses the scope, and under a fail-closed secondary profile the GitHub token is unavailable. Lower severity (best-effort, fails open) but same root cause and same one-line fix
gateway/run.py:5439 _start_gateway_housekeeping correct as-is — installs the scope inside the thread per profile per tick (profile_scoped_chore → _profile_runtime_scope()); there is nothing to inherit
tools/browser_tool_lifecycle.py:431 _browser_cleanup_thread_worker correct as-is, and inheriting would be a bug — the janitor is deliberately process-global and must re-enter each session's own owner scope per teardown (_session_owner_scope); inheriting the spawning profile's scope is explicitly wrong here

So the one-liner is right for its path and the framing is honest, but "exactly one bare Thread" is not literally true — tools/tirith_security.py:469 is the same defect. Worth a follow-up issue; happy to take it.

Minor note, not a defect: copy_context() is evaluated at Thread() construction, so a scope installed asynchronously after start() would be missed. No current caller does that.

Approve as-is.

This branch has not been deployed

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

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants