Skip to content

fix(disk-cleanup): protect cron output root from cleanup (salvage #49271) - #56266

Merged
kshitijk4poor merged 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-49271
Jul 1, 2026
Merged

kshitijk4poor merged 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage-49271

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Summary

Salvage of #49271 by @martinramos002-bot (rebased onto current main). Stops the disk-cleanup plugin from deleting the cron output root (~/.hermes/cron/output) while still letting individual run artifacts below it be cleaned by retention policy.

The bug

The disk-cleanup plugin classifies anything under cron/output/ as disposable cron-output. That includes the cron/output root directory itself — deleting it wholesale erases every job's retained run history at once, far broader than expiring one run artifact.

The fix

  • Add str(base / "output") to _PROTECTED_CRON_PATHS in _is_protected_cron_path (for both cron and cronjobs parents) — a hard block on deleting the output root.
  • In guess_category, tighten the gate from len(rel.parts) >= 2 to >= 3: rel is relative to hermes_home, so cron/output is ('cron','output') (len 2) → no longer categorized deletable, while a real artifact at cron/output/<file> (len 3) or deeper stays cron-output and is still cleaned.

Two independent defenses for the same invariant (root not deletable): the category gate, and the protected-paths hard block as defense-in-depth.

Verification

Verified against current main (applied + behaviorally probed): cron/output root → protected; cron/output/run.md (len 3 file), cron/output/job_1/run.md (len 4), cron/output/job_1 (len 3 dir) all still deleted; a stale tracked.json entry pointing at the root is dropped by quick()'s re-validation and hard-blocked by _is_protected_cron_path (both canonicalize via .resolve()). No off-by-one, no over-broad protection, no residual deletion path.

Ran hermes-agent-dev + hermes-pr-review Phase 2c (4-part structured) on the final salvage.

Tests

tests/plugins/test_disk_cleanup_plugin.py — output-root-not-tracked, stale-entry-for-output-root-not-deleted, plus existing artifact-still-cleaned coverage. (CI runs the full suite.)

Supersedes #49271. Full credit to @martinramos002-bot.

martinramos002-bot and others added 2 commits July 1, 2026 15:27
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.
Review follow-up: the _is_protected_cron_path docstring listed output/ next
to jobs.json/.tick.lock as 'the directory itself', which is slightly
ambiguous. Spell out that the match is EXACT-path only and must not be
'simplified' into a blanket cron/output/* guard (children stay cleanable) —
prevents a future editor from re-introducing the wholesale-delete bug this
fix closes.
@kshitijk4poor

Copy link
Copy Markdown
Contributor Author

Ran hermes-agent-dev + hermes-pr-review Phase 2c (4-part structured). 0 Critical, 0 Warnings. The off-by-one gate is correct (root protected, artifacts still cleaned), the two defenses (category gate + protected-paths set) are legitimate defense-in-depth (gate stops new tracking; set stops deletion of stale tracked.json entries), both .resolve()-consistent, both cron/cronjobs parents covered. Folded one doc-clarity follow-up (9ddd7c8): spell out that the output-root protection is exact-match-only so a future editor can't 'simplify' it into a blanket cron/output/* guard.

@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) July 1, 2026 10:05
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P1 High — major feature broken, no workaround labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Salvage of #49271 (rebased onto current main). related_to the disk-cleanup cron-protection cluster (#39067, merged stale-entry revalidation). Not a duplicate; this is the output-root-protection facet.

@kshitijk4poor
kshitijk4poor merged commit ede5c09 into NousResearch:main Jul 1, 2026
29 checks passed
@kshitijk4poor
kshitijk4poor deleted the salvage-49271 branch August 5, 2026 07:09
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 P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants