fix(cron): scope cron job workdir to own session instead of process-global cwd (#69396) - #70915
Closed
kshitijk4poor wants to merge 4 commits into
Closed
kshitijk4poor wants to merge 4 commits into
kshitijk4poor wants to merge 4 commits into
Conversation
NousResearch#69396) The cron scheduler was mutating process-global state in two places: 1. no_agent path called os.chdir() which changed the global process cwd, leaking into concurrent gateway sessions. 2. The agent path set os.environ['TERMINAL_CWD'] which any gateway session could read during context-file discovery via resolve_context_cwd/build_context_files_prompt. Fix: - no_agent path: pass workdir as subprocess cwd parameter to _run_job_script() instead of os.chdir(). The Python process cwd is never mutated. - Agent path: in addition to the lock-serialized TERMINAL_CWD, also set the per-context _SESSION_CWD ContextVar from agent.runtime_cwd. This ContextVar is scoped to the current thread/context and NEVER leaks into other sessions. resolve_context_cwd() checks _SESSION_CWD first, so the cron's own context file discovery uses the correct workdir, while gateway sessions (which have no override) fall through to their own TERMINAL_CWD.
Eliminates the separate import/set/clear dance for _SESSION_CWD by passing cwd= directly to set_session_vars(), which already handles the ContextVar set internally and clears it via clear_session_vars(). Also includes the exception message in the no_agent error path. Follow-up to salvaged PR NousResearch#70548 (NousResearch#69396).
Follow-up to salvaged PR NousResearch#70548 — _run_job_script now accepts workdir= kwarg, test mocks need to accept it too.
alt-glitch
added
type/bug
Collaborator
|
Merged via #70989 byte-identical — your salvage of webtecnica's fix (authorship preserved via the agent@hermes.dev mapping) reusing set_session_vars(cwd=) was exactly right. Fixes #69396. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cron job workdir no longer leaks into concurrent gateway sessions — the per-context
_SESSION_CWDContextVar is set viaset_session_vars(cwd=...)instead of process-globalos.chdir().Previously, a cron job with
workdirset mutated the process-global cwd (os.chdir()in no_agent path,TERMINAL_CWDenv var in agent path). Any gateway session created while the cron was running inherited that cwd and loaded the wrongAGENTS.md, causing the gateway session to execute the cron's task instead of the user's request. Fixes #69396.Changes
cron/scheduler.py:os.chdir()withsubprocess.run(cwd=workdir)— process cwd is never mutatedcwd=_job_workdirtoset_session_vars(), letting the existing session-context machinery handle_SESSION_CWDset/clear (no separate import/set/clear dance)clear_session_cwd()block —clear_session_vars()already calls it internallytests/cron/test_scheduler.py: updated test stub to accept newworkdirkwargValidation
os.chdir()in no_agent pathCherry-picked from #70548 by @webtecnica. Simplified to use existing
set_session_vars(cwd=)instead of separate set/clear. First commit (#68915 spawn rewrite) already on main as d7512c8 — only the cron workdir fix is salvaged here.