Background dispatch threads, the periodic scheduler scan, and HTTP
handlers all followed a load_tasks() → modify → save_tasks() pattern
without any mutual exclusion. The atomic_json_write only protects the
write itself; it does NOT prevent a concurrent reader from loading a
stale snapshot and overwriting another thread's changes.
Scenario (before this fix):
1. HTTP handler calls load_tasks() — gets snapshot A
2. Dispatch thread calls load_tasks() — gets the same snapshot A
3. HTTP handler modifies task X, calls save_tasks() — writes A'
4. Dispatch thread modifies task Y, calls save_tasks() — writes A''
(based on A, not A'), silently losing the HTTP handler's changes
to task X
This is a classic TOCTOU (Time-of-Check-Time-of-Use) race. The project
already has atomic_json_update() in file_lock.py that holds an exclusive
lock for the entire read-modify-write cycle, but none of the task
mutation paths used it.
Fix:
- Add modify_tasks(modifier) wrapper around atomic_json_update +
refresh trigger
- Add modify_task(task_id, updater) convenience wrapper for single-task
mutations
- Convert _update_task_scheduler (called from dispatch daemon threads)
to use modify_task
- Convert handle_scheduler_scan (periodic background scanner) to use
modify_tasks, with dispatch side-effects deferred until after the
lock is released
- Convert handle_scheduler_retry and handle_scheduler_rollback to use
modify_task
HTTP handler paths (handle_task_action, handle_review_action, etc.)
still use load_tasks/save_tasks for now — they run in the main thread
and are lower risk — but can be migrated incrementally.
Add 17 regression tests covering: atomic API correctness, scheduler
update persistence, scan stall detection, concurrent write survival
(the actual race), source-level audit of critical paths, and backward
compatibility of load_tasks/save_tasks.
fix: apply allowed_roots check to file:// URLs in add_remote_skill (CWE-22)\n\nAdds .resolve() and allowed_roots validation to the file:// URL branch\nin add_remote_skill(), closing a path traversal vulnerability.\nIncludes 3 regression tests.
Two new test cases in test_sync_symlinks.py:
1. TestSyncScriptSymlink.test_skips_self_referential_via_directory_symlink
Unit test: verifies _sync_script_symlink() returns False and leaves
the real source file intact when dst_file is accessed through a
directory-level symlink that points back to the project scripts/ dir.
2. TestSyncScriptsToWorkspaces.test_no_self_referential_symlinks_when_workspace_scripts_is_dir_symlink
Integration test: simulates the install.sh scenario where
workspace-main/scripts -> project/scripts, then confirms that
sync_scripts_to_workspaces() does not convert any real source file
into a self-referential symlink."
sync_scripts_to_workspaces() previously used physical file copies. Scripts
that derive project root from __file__ (e.g. kanban_update.py) therefore
resolved to the workspace directory when run as a copied file, causing
tasks_source.json writes to land in the wrong location while the Dashboard
reads from the canonical data/ directory.
Replace write_bytes() with os.symlink() so __file__ always resolves back to
the project scripts/ directory. This ensures that all path-derived constants
(TASKS_FILE, DATA, etc.) point to the single canonical data/ folder regardless
of which agent workspace runs the script.
Added _sync_script_symlink() helper with:
- Idempotent re-runs (skip if link already correct)
- Automatic cleanup of stale physical copies and broken symlinks
- Full test suite (10 tests) covering creation, idempotency, replacement
of physical copies, broken symlinks, __file__ resolution, etc.
Closes#56
Co-authored-by: cft0808 <41196455+cft0808@users.noreply.github.com>
