Skip to content

fix(cron): make skip_memory configurable via config.yaml (salvage of #9825 by @nightq) - #52897

Closed
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/9825-cron-skip-memory
Closed

fix(cron): make skip_memory configurable via config.yaml (salvage of #9825 by @nightq)#52897
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:salvage/9825-cron-skip-memory

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvage of #9825 by @nightq — rebased onto current origin/main. Closes #9763.

Cron jobs hardcode skip_memory=True when constructing the agent, so external memory providers (e.g. mem0) are unusable from cron — there's no way to opt in.

Root Cause

Symptom — A user who wants their cron jobs to read/write an external memory provider (mem0, Honcho, etc.) can't: cron always passes skip_memory=True regardless of config.

Root cause — In run_job (cron/scheduler.py), the AIAgent(...) construction sets skip_memory=True literally, with a comment that cron system prompts would corrupt user representations. That's a sensible default, but it's wired as an unconditional constant — there's no config knob.

Evidence — The added tests exercise _resolve_cron_skip_memory, the new resolver that feeds the kwarg; without the fix the symbol doesn't exist and the value was a hardcoded constant.

Fix + why this level — Read cron.skip_memory from config.yaml, defaulting to True (backward compatible). I centralized the resolution into _resolve_cron_skip_memory(cfg) rather than inlining cfg.get(...), so the behavior is unit-testable at a real seam (the original PR only asserted dict semantics in isolation, which couldn't fail without the source change). This is the correct level — one resolver feeds the single cron agent construction; the default preserves the "don't corrupt user representations" intent while letting power users opt in.

Scope / risk — One helper + one wired kwarg. Default behavior is unchanged (still True when unset/cron section missing/non-dict). Only cron.skip_memory: false changes behavior, which is exactly the opt-in this enables.

Changes from original

  • Rebased onto current main (clean single commit).
  • Centralized resolution in _resolve_cron_skip_memory (a genuine test seam) and replaced the original's tautological dict-semantics tests with tests that import and exercise the real resolver — they fail without the fix (ImportError / hardcoded constant).

Verification

python3 -m pytest tests/cron/test_scheduler_skip_memory.py -q
6 passed

# without the fix (stashed):
ImportError: cannot import name '_resolve_cron_skip_memory'

Real behavior proof

# _resolve_cron_skip_memory(cfg) feeds the cron AIAgent skip_memory kwarg:
{}                                -> True   (default, backward compatible)
{"cron": {"skip_memory": False}}  -> False  (mem0 etc. now usable in cron)
{"cron": {"skip_memory": True}}   -> True
{"cron": None}                    -> True   (tolerant)

Credit: salvage of #9825 by @nightq — rebased onto current main with guardrail tests.

@alt-glitch alt-glitch added type/feature New feature or request comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jun 26, 2026
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Plate-clear rebase (2026-07-11)

  • Rebased onto upstream/main (6142203bd).
  • Conflicts resolved: 1 file(s) (feature side preferred for intentional patches when conflicted).
  • Force-with-lease push to Bartok9 branch salvage/9825-cron-skip-memory.
  • Intent preserved; still awaiting CI green before merge.

@Bartok9
Bartok9 force-pushed the salvage/9825-cron-skip-memory branch from 3702f26 to bab4eec Compare July 11, 2026 17:10
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Correction rebase — previous automated conflict resolution incorrectly dropped large upstream sections (took entire feature-side file). Rebuilt from original patch intent onto current main. Please re-check diff size.

@Bartok9
Bartok9 force-pushed the salvage/9825-cron-skip-memory branch from bab4eec to d08c4bf Compare July 11, 2026 17:11
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Production + tests re-applied cleanly on current main (prior rebuild dropped source hunks). Diff should be small and intentional again.

@Bartok9
Bartok9 force-pushed the salvage/9825-cron-skip-memory branch from d08c4bf to d5ee850 Compare July 11, 2026 17:12
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Clean re-apply: source helper + skip_memory=_resolve_cron_skip_memory(_cfg) + tests; removed accidental .rej files from prior automated mess.

@Bartok9
Bartok9 force-pushed the salvage/9825-cron-skip-memory branch from d5ee850 to 75dc0be Compare July 11, 2026 17:12
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for salvaging the original report. The current-main premise is real: cron/scheduler.py:3073 still hard-codes skip_memory=True.

Problems

  • skip_memory=False enables both local and external memory: agent/agent_init.py:1363-1395 gates MEMORY.md/USER.md and MemoryManager initialization on the same flag. The original guard was added explicitly to prevent cron prompts corrupting user representations (005e0ec4f81563a9b7f94cbb4c77f190caf3de55), and the fix: make cron skip_memory configurable via config.yaml #9825 discussion identifies this as a concern. Please resolve whether provider-only access is required before merging.
  • The new tests exercise only _resolve_cron_skip_memory; they do not prove run_job passes the value to AIAgent. tests/cron/test_cron_workdir.py:273-285 already has a suitable constructor-kwargs capture seam.
  • This user-facing setting is absent from hermes_cli/config.py:2636's cron defaults and documentation still says cron always uses skip_memory=True at website/docs/user-guide/features/spotify.md:218.

Suggested changes

  • Separate provider access from local-memory injection, or explicitly document and approve the broader opt-in.
  • Add a run_job wiring test, the default config entry, and matching docs.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@Bartok9

Bartok9 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the memory-scope concern is the right gate to close before merge. You're correct that skip_memory=False re-enables both local injection (agent/agent_init.py:1363-1395) and MemoryManager, which the original guard (005e0ec) deliberately blocked. I'll re-scope so cron gets provider-only access without re-enabling MEMORY.md/USER.md injection, rather than a blanket opt-in, and:

  • add a run_job wiring test using the constructor-kwargs capture seam at tests/cron/test_cron_workdir.py:273-285 to prove the resolved value reaches AIAgent;
  • add the cron.skip_memory default to hermes_cli/config.py:2636;
  • fix the stale docs claim at website/docs/user-guide/features/spotify.md:218.

Will push the revised version rather than merge as-is.

@Bartok9

Bartok9 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: provider-only scope (per teknium review)

Addressed the merge gate without a blanket memory opt-in:

  1. Split gates in agent/agent_init.py / AIAgent: new skip_local_memory controls MEMORY.md/USER.md only; skip_memory still gates external MemoryManager providers.
  2. Cron wiring: run_job still uses _resolve_cron_skip_memory for providers, and always passes skip_local_memory=True so cron system prompts cannot re-corrupt on-disk user representations (005e0ec).
  3. Config + docs: cron.skip_memory default documented in hermes_cli/config.py; Spotify note updated (default skip; false = providers only).
  4. Tests (tests/cron/test_scheduler_skip_memory.py, 8 passed):
    • existing _resolve_cron_skip_memory cases
    • run_job wiring via the FakeAgent constructor-kwargs capture pattern from test_cron_workdir.py — proves default skip_memory=True + always skip_local_memory=True, and opt-in skip_memory=False + still skip_local_memory=True

Ready for re-review / merge when convenient. Credit remains with #9825/@nightq for the original premise.

@Bartok9

Bartok9 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt onto latest main via patch re-apply (force-push). Please re-run CI.

— Bartok9 public PR hygiene 2026-08-01

@Bartok9
Bartok9 force-pushed the salvage/9825-cron-skip-memory branch from de63235 to 1866cfd Compare August 1, 2026 17:36
…tok9

Per-PR attribution so check-attribution passes on this branch (Teknium).
@teknium1

Copy link
Copy Markdown
Contributor

Closing — the underlying change landed in PR #91447 (merged as ef04d84): cron agents now run with memory enabled unconditionally rather than behind a config toggle. Credit to @nightq (#9825, earliest) and thanks @Bartok9 for the salvage effort.

@teknium1 teknium1 closed this Aug 21, 2026
@Bartok9

Bartok9 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @teknium1 — glad the underlying fix landed in #91447. Appreciate the credit to @nightq and the close note. No further action needed on this salvage.

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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cron jobs hardcode skip_memory=True, making external memory providers (e.g. mem0) unusable

3 participants