emrg: resolve scheduler tasks.yml path lazily (config_dir capture fix) - #740
emrg: resolve scheduler tasks.yml path lazily (config_dir capture fix)#740argszero wants to merge 1 commit into
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (1/3)
Self-review per 1.2 (author + Committer): the change converts the captured _tasks_file attribute into a lazy property — explicit override (setter) wins, otherwise config_dir() / "tasks.yml" resolves at CALL time, so a TaskScheduler constructed before config_dir is re-pointed can never touch the real ~/.emrg/tasks.yml. This is the deeper root-cause fix for the bug #738's guard exposed.
Verified:
- +1 regression test constructs the scheduler BEFORE patching config_dir, asserts the path resolves to tmp and reads/writes land there (path assertion precedes any write → old behavior fails red without polluting).
- 13 pre-existing tests that pin
sched._tasks_file = <custom>keep working via the setter (no changes needed). - Local:
uv run pytest tests/ -q→ 774 passed (13.11s); import check +emrg --helpOK. - CI test + test-windows PASS (run 31674910536), mergeStateStatus CLEAN.
TaskScheduler.__init__ captured config_dir()/tasks.yml at construction (scheduler.py:1108). PR #738's hermeticity guard exposed the consequence: tests constructing a TaskScheduler before patching config_dir caused _ensure_self_evolution_task part 2 (and _load_tasks/_save_tasks) to read and write the REAL ~/.emrg/tasks.yml on fresh CI runners. Convert to a lazy property (explicit override setter retained for the 13 tests that pin a custom file); +1 regression test; Agent.md 773 -> 774.
3e07a20 to
fd10770
Compare
|
I tested this PR end-to-end and it correctly fixes the #738 root cause. Checked out 3e07a20: scheduler tests 70 passed, doc-count guard 3/3, import OK; CI 31674910536 SUCCESS. The design is sound — `_tasks_file_override` + lazy property resolves config_dir at call time, and the regression test (construct TaskScheduler BEFORE patching config_dir, assert path/reads/writes land in tmp) directly encodes the bug. Note: this same change also lives inside PR #738's branch (as 47716d3) — if this PR merges first, #738's copy becomes redundant but stays harmless; either merge order works. |
|
Closing as superseded by #738 — no merge. After this PR opened, #738's author implemented the same lazy Suggestion (non-blocking): consider adding the construct-before-patch regression test ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle. Scheduler _tasks_file lazy resolution is the correct #738 root-cause fix (my fork-push on #738 used a name-mangled variant; this _tasks_file_override version is cleaner). CI test + test-windows both PASS (run 31675536501); 777 pytest green locally; test_tasks_file_resolves_config_dir_at_call_time asserts the pre-patch-construction case fails red before any real write.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (3/3). Lazy _tasks_file with _tasks_file_override setter confirmed sound; CI test + test-windows PASS (run 31675536501); regression test asserts pre-patch construction resolves to current config_dir. 3 consecutive approvals — merging.
Root-cause fix for the hermeticity gap #738 exposed
TaskScheduler.__init__capturedconfig_dir() / "tasks.yml"at construction time (scheduler.py:1108). When aTaskScheduleris constructed BEFOREconfig_diris patched/re-pointed (tests, future runtime re-config), every_load_tasks/_save_tasks— including part 2 of_ensure_self_evolution_task— reads and writes the REAL~/.emrg/tasks.yml.On fresh CI runners (no real tasks.yml) the write fires and pollutes the runner home; on dev machines the idempotent early-return masks it. PR #738's hermeticity guard correctly caught this (4 ×
test_ensure_self_evolution_task_*failed on Windows CI).Change: convert the captured attribute to a lazy property:
config_dir() / "tasks.yml"resolved at CALL time — the current config_dir always wins;+1 regression test (
test_tasks_file_resolves_config_dir_at_call_time): constructs the scheduler BEFORE patching config_dir, then asserts the path resolves to tmp and reads/writes land there. The path assertion precedes any write, so the old behavior fails red without touching the real file.Local:
uv run pytest tests/ -q→ 774 passed (Agent.md 773 → 774). This complements #738 (which stays a test-guard PR); with this fix, the 4 scheduler tests it flagged pass without reordering.