Skip to content

fix(cron): add per-job memory_enabled override for memory provider access - #66775

Closed
eulahwu917 wants to merge 2 commits into
NousResearch:mainfrom
eulahwu917:wt/t_6e056e8b-cron-memory-enabled
Closed

fix(cron): add per-job memory_enabled override for memory provider access#66775
eulahwu917 wants to merge 2 commits into
NousResearch:mainfrom
eulahwu917:wt/t_6e056e8b-cron-memory-enabled

Conversation

@eulahwu917

Copy link
Copy Markdown

Problem

Cron sessions pass skip_memory=True to AIAgent, which prevents all cron jobs from using the memory tool or the external memory provider (e.g., hindsight). This is correct for most jobs (daily briefings, reports) but blocks cron jobs whose entire purpose is memory maintenance — like bepop-hindsight-memory-dump.

Root Cause

cron/scheduler.py hardcoded skip_memory=True at the AIAgent construction site (~line 3073). The memory provider plugin and MemoryStore are never initialized when skip_memory=True (agent/agent_init.py, lines 1335-1419), so the memory tool always returns \"Memory is not available.\" in cron sessions.

Fix

Add a per-job memory_enabled override following the same pattern as attach_to_session (already used in the codebase to opt cron jobs into session delivery):

File Change
cron/scheduler.py skip_memory=not bool(job.get(\"memory_enabled\")) — defaults True (backward compatible)
cron/jobs.py Add memory_enabled: Optional[bool] = None to create_job(); persist only when explicitly set as bool
tools/cronjob_tools.py Surface memory_enabled in both create and update paths of the cronjob tool
AGENTS.md Document the opt-in override mechanism

Backward Compatibility

  • Zero impact on existing jobs — the key is absent, so job.get("memory_enabled") returns Noneskip_memory=True (existing behavior).
  • Storage format unchangedmemory_enabled is only persisted when explicitly True or False, keeping existing jobs.json entries byte-identical.
  • No merge or gateway restart required — this is a pure code change; the next cron tick picks it up.

…cess

Cron sessions have always passed skip_memory=True to AIAgent to prevent
cron system prompts from polluting user representations. This is correct
for most jobs (daily briefings, reports, watchdogs) but blocks cron jobs
whose entire purpose is memory maintenance (e.g. memory dump/filing via
the hindsight provider).

Add a memory_enabled field to the job schema following the existing
attach_to_session pattern:

- cron/jobs.py: add memory_enabled parameter to create_job() — persisted
  only when explicitly set as a bool, keeping existing jobs byte-identical.
- cron/scheduler.py: make skip_memory conditional: not bool(job.get(
  "memory_enabled")). Default remains True (backward compatible).
- tools/cronjob_tools.py: surface memory_enabled in both create and
  update paths of the cronjob tool.
- AGENTS.md: document the override mechanism.

Backward compatible: existing cron jobs are unaffected (absence of the
key means skip_memory=True, the existing behavior). Only jobs that
explicitly opt in gain memory access.
@alt-glitch alt-glitch added type/feature New feature or request comp/cron Cron scheduler and job management tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #34098: both implement the same per-job memory_enabled opt-in through cron job creation/update and skip_memory in the scheduler. #34098 is the earlier open focused implementation.

…andler plumbing)

Reviewer approved-with-revisions on PR NousResearch#66775 — the feature was correct
but lacked test coverage.  Add 12 tests following the test_cron_workdir.py
pattern, covering:

1. create_job() param plumbing for memory_enabled (True/False/None stored)
2. default-None preservation (no key written to jobs.json when unset)
3. run_job's skip_memory wiring:
   - memory_enabled=true  -> skip_memory=False (memory active)
   - memory_enabled=false -> skip_memory=True  (memory off, explicit)
   - memory_enabled unset -> skip_memory=True  (backward compat default)
4. cronjob tool create/update JSON round-trip for memory_enabled

While writing the tests, found and fixed two gaps:
- registry handler was NOT forwarding memory_enabled to cronjob()
- CRONJOB_SCHEMA lacked memory_enabled property
- _format_job never surfaced memory_enabled when truthy
@eulahwu917

Copy link
Copy Markdown
Author

Reviewer — test coverage added per your revision request (12 new tests in `tests/cron/test_cron_memory_enabled.py`).\n\nWhile writing the tests, I also found and fixed 3 gaps in the original PR:\n- registry handler was not forwarding `memory_enabled` to `cronjob()` — tool schema calls would silently ignore it\n- CRONJOB_SCHEMA lacked the `memory_enabled` property\n- _format_job never surfaced `memory_enabled` when truthy\n\nAll 33 tests pass (21 existing workdir tests + 12 new memory_enabled tests, no regressions).\n\nReady for final review/merge.

@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.

Reviewed by Hermes Agent

@eulahwu917

Copy link
Copy Markdown
Author

Closing per maintainer triage — confirmed duplicate of #34098, which is further along (passing CI, active review discussion, complementary PR #45769 already layered on top). Thanks for the fast triage.

@eulahwu917 eulahwu917 closed this Jul 18, 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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/memory Memory tool and memory providers type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants