Skip to content

Fix cron profile workdir resolution - #52343

Closed
Shoutoutdigitalau wants to merge 1 commit into
NousResearch:mainfrom
Shoutoutdigitalau:fix/cron-profile-workdir-resolution-20260625
Closed

Fix cron profile workdir resolution#52343
Shoutoutdigitalau wants to merge 1 commit into
NousResearch:mainfrom
Shoutoutdigitalau:fix/cron-profile-workdir-resolution-20260625

Conversation

@Shoutoutdigitalau

Copy link
Copy Markdown

Summary

  • Resolve profile home from the selected cron profile instead of the default profile.
  • Expand ~ workdirs against that profile home so profile-scoped cron jobs/imports work on Windows.
  • Add regression coverage for default and named-profile workdir resolution.

Test Plan

  • PYTHONPATH="$(pwd)" python -m pytest tests/cron/test_cron_workdir.py tests/cron/test_cron_script.py -q -o "addopts=" = 61 passed, 1 skipped
  • git diff --check origin/main..HEAD

Safety

  • Branch was created in a clean isolated worktree from origin/main.
  • Only cron/jobs.py and tests/cron/test_cron_workdir.py are included.
  • No unrelated local stock Hermes dirt was mixed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Jun 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #40964 (ContextVar profile-home isolation), the cross-profile-home family incl. #4707, and the merged revert #43956. Heads-up for reviewers: resolve_profile_home()'s docstring says the scheduler imports it “while applying per-job profile scope,” which reintroduces the per-job-profile scoping that #43956 (revert of #28124) deliberately removed. The ~/~\ workdir-expansion change is independently useful, but the profile-scope reintroduction conflicts with main's current direction — worth confirming intent before merge. Not marking duplicate.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

This PR fixes cron profile workdir resolution by adding resolve_profile_home() to map profile names to their Hermes home directories. Cron jobs execute against the profile that created them, but were previously using the ticker's current profile.

Key observations

  • resolve_profile_home() handles the default profile and named profiles
  • Path traversal protection via relative_to() check
  • Regex validation for profile names
  • _normalize_workdir() updated to handle ~ expansion manually for cross-platform compatibility
  • Comprehensive test coverage for both functions

Looks Good

  • Clean, focused fix for a real profile isolation bug
  • Good defensive coding with path traversal protection
  • Test coverage for edge cases (invalid names, missing dirs, relative paths)

Reviewed by Hermes Agent (cron)

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression coverage. This is an automated hermes-sweeper review; current main already implements the profile-scoped cron guarantee.

  • cron/jobs.py:54 anchors every cron store to the active profile's get_hermes_home() and documents the cross-profile credential/config isolation requirement.
  • cron/scheduler.py:543 resolves that active profile home dynamically for execution; tests/cron/test_cron_profile_isolation.py:34 covers storage, locks, and execution-home isolation.
  • Commit d73078e7b036ae75999481fc8ffaa2b82b69cf87 made this behavior intentional and regression-tested after this PR opened.
  • The proposed resolve_profile_home() has no scheduler call site in this PR, so it cannot change execution or workdir resolution. It also describes restoring per-job profile scoping, which 7d8d000b1921cb1c14aa137a41bd96203992e2c0 deliberately removed.
  • The later cross-reference BUG: Cron script resolution uses active profile instead of job's profile #54288 concerns a separate cron script-resolution path; this PR does not modify cron/scheduler.py.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants