Skip to content

fix: protect cron output root from cleanup - #49271

Closed
martinramos002-bot wants to merge 1 commit into
NousResearch:mainfrom
martinramos002-bot:fix/disk-cleanup-cron-output-root
Closed

martinramos002-bot wants to merge 1 commit into
NousResearch:mainfrom
martinramos002-bot:fix/disk-cleanup-cron-output-root

Conversation

@martinramos002-bot

Copy link
Copy Markdown
Contributor

Summary

  • Do not classify the cron/output directory itself as disposable cron output.
  • Continue allowing individual artifacts below cron/output/... to be cleaned by retention policy.
  • Add regression coverage for category detection and stale tracked entries that point at the output root.

Rationale

The scheduler output directory is container state for retained job history. Deleting that root directory wholesale is broader than deleting an expired individual run artifact and can remove all retained outputs at once.

Test plan

  • python -m pytest tests/plugins/test_disk_cleanup_plugin.py -q -o addopts=''

Only classify files below cron/output/ as disposable cron output.
The cron/output directory itself is a durable container for retained
job history and should not be tracked or deleted wholesale.

Add regression coverage for both category detection and cleanup of a
stale tracked entry pointing at the output root.
Copilot AI review requested due to automatic review settings June 19, 2026 21:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR prevents the disk-cleanup plugin from treating the cron/output root directory as disposable “cron-output” (while still allowing cleanup of individual artifacts under cron/output/...), and adds regression tests for both category detection and stale tracked entries that point at cron/output.

Changes:

  • Update guess_category() so only paths below cron/output/ are categorized as cron-output (excluding cron/output itself).
  • Extend the cron protection guard to include the cron/output root as a never-delete path.
  • Add tests covering cron/output root categorization and a stale tracked.json entry pointing at cron/output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
plugins/disk-cleanup/disk_cleanup.py Tightens cron-output categorization and adds cron/output root to the protected cron paths list.
tests/plugins/test_disk_cleanup_plugin.py Adds regression tests ensuring cron/output root is not tracked and cannot be deleted via stale tracked entries.
Comments suppressed due to low confidence (1)

plugins/disk-cleanup/disk_cleanup.py:186

  • _is_protected_cron_path() builds _PROTECTED_CRON_PATHS from get_hermes_home() without resolving, but compares against str(p.resolve()). If HERMES_HOME contains symlinks (or the tracked paths were stored resolved), the string comparison can fail and the protection guard won’t trigger, allowing cron control-plane paths to be deleted in the defense-in-depth path.
    if not _PROTECTED_CRON_PATHS:
        hermes_home = get_hermes_home()
        for parent in ("cron", "cronjobs"):
            base = hermes_home / parent
            _PROTECTED_CRON_PATHS.add(str(base))
            _PROTECTED_CRON_PATHS.add(str(base / "output"))
            _PROTECTED_CRON_PATHS.add(str(base / "jobs.json"))
            _PROTECTED_CRON_PATHS.add(str(base / ".tick.lock"))
    resolved = str(p.resolve())
    return resolved in _PROTECTED_CRON_PATHS

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins comp/cron Cron scheduler and job management P1 High — major feature broken, no workaround labels Jun 19, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jun 21, 2026
@kshitijk4poor

Copy link
Copy Markdown
Contributor

Superseded by #56266, which carries your fix verbatim (re-authored to your GitHub identity — the original commit was authored as a generic 'Hermes Agent' identity, so I set it to martinramos002-bot so you get the git credit) rebased onto current main (this PR was ~592 commits behind).

I verified the fix end-to-end (the len(rel.parts)>=3 gate is correct: root protected, real artifacts still cleaned; the protected-paths entry backstops stale tracked.json entries). Ran hermes-agent-dev + hermes-pr-review Phase 2c (0 Critical); folded one doc-clarity follow-up. Full credit to you. Closing in favor of #56266 — feel free to push back.

hashbender added a commit to hashbender/hermes-agent that referenced this pull request Jul 1, 2026
…sResearch#49271) (#271)

Co-authored-by: qbit-mirror-bot <qbit-mirror-bot@users.noreply.github.com>
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 comp/plugins Plugin system and bundled plugins P1 High — major feature broken, no workaround sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants