Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions cron/jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,12 +557,27 @@ def update_job(job_id: str, updates: Dict[str, Any]) -> Optional[Dict[str, Any]]

updated = _apply_skill_fields({**job, **updates})
schedule_changed = "schedule" in updates
repeat_changed = "repeat" in updates

if "skills" in updates or "skill" in updates:
normalized_skills = _normalize_skill_list(updated.get("skill"), updated.get("skills"))
updated["skills"] = normalized_skills
updated["skill"] = normalized_skills[0] if normalized_skills else None

if repeat_changed:
# The API may pass repeat as a raw integer (e.g. {"repeat": 2})
# instead of a pre-parsed dict. Normalize it to dict format
# matching create_job() behavior: {"times": n, "completed": count}
updated_repeat = updated["repeat"]
if isinstance(updated_repeat, int):
# Normalize non-positive values to None (matches create_job behavior)
if updated_repeat <= 0:
updated["repeat"] = None
else:
# Preserve existing completed count if job already has repeat data
existing_completed = job.get("repeat", {}).get("completed", 0)
updated["repeat"] = {"times": updated_repeat, "completed": existing_completed}

if schedule_changed:
updated_schedule = updated["schedule"]
# The API may pass schedule as a raw string (e.g. "every 10m")
Expand Down
5 changes: 4 additions & 1 deletion gateway/platforms/whatsapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,15 @@ async def connect(self) -> bool:
if not (bridge_dir / "node_modules").exists():
print(f"[{self.name}] Installing WhatsApp bridge dependencies...")
try:
# Read timeout from environment variable, default to 300 seconds (5 minutes)
# to accommodate slower systems like Unraid NAS
npm_install_timeout = int(os.environ.get("WHATSAPP_NPM_INSTALL_TIMEOUT", "300"))
install_result = subprocess.run(
["npm", "install", "--silent"],
cwd=str(bridge_dir),
capture_output=True,
text=True,
timeout=60,
timeout=npm_install_timeout,
)
if install_result.returncode != 0:
print(f"[{self.name}] npm install failed: {install_result.stderr}")
Expand Down
109 changes: 109 additions & 0 deletions tests/gateway/test_api_server_jobs_update_repeat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
"""
Regression test for API job repeat updates corrupting cron repeat state.

This test ensures that PATCH /api/jobs/{job_id} with repeat field
correctly normalizes integer values into dict format and preserves completed count.

Related issue: #15582
"""

import tempfile
from pathlib import Path
import pytest

from cron import jobs


def test_update_job_repeat_normalizes_integer_to_dict():
"""
Test that updating a job's repeat count via API (integer) correctly
normalizes into the internal dict format while preserving completed count.

This prevents mark_job_run() from crashing with AttributeError when it
calls .get() on job["repeat"].
"""
with tempfile.TemporaryDirectory() as td:
tmp = Path(td)
jobs.CRON_DIR = tmp / "cron"
jobs.JOBS_FILE = jobs.CRON_DIR / "jobs.json"
jobs.OUTPUT_DIR = jobs.CRON_DIR / "output"

# Create a job with repeat=5
job = jobs.create_job(
prompt="test prompt",
schedule="every 1h",
name="test-job",
repeat=5,
deliver="local",
)
job_id = job["id"]

# Verify initial repeat format
assert isinstance(job["repeat"], dict)
assert job["repeat"]["times"] == 5
assert job["repeat"]["completed"] == 0

# Simulate first successful run (marks completed=1)
jobs.mark_job_run(job_id, success=True)
jobs_after_first_run = jobs.load_jobs()
updated_job = [j for j in jobs_after_first_run if j["id"] == job_id][0]
assert updated_job["repeat"]["completed"] == 1

# Update repeat via API (integer form, as PATCH does)
updated = jobs.update_job(job_id, {"repeat": 2})
assert isinstance(updated["repeat"], dict), (
f"Repeat should be dict after update, got {type(updated['repeat'])}"
)
# The key fix: integer 2 should be normalized to dict format
assert updated["repeat"]["times"] == 2, (
f"Repeat times should be 2, got {updated['repeat']['times']}"
)
# Critical: completed count should be preserved from before update
assert updated["repeat"]["completed"] == 1, (
f"Repeat completed should be preserved as 1, got {updated['repeat']['completed']}"
)

# Verify mark_job_run doesn't crash after update
# This should not raise AttributeError: 'int' object has no attribute 'get'
jobs.mark_job_run(job_id, success=True)
jobs_after_second_run = jobs.load_jobs()
# After this run, completed should be 2 (>= times=2), so job is removed
# This is expected behavior for repeat limit
# Find the job in the list before mark_job_run removed it
# by checking if the job file still contains the job_id
if any(j["id"] == job_id for j in jobs_after_second_run):
final_job = [j for j in jobs_after_second_run if j["id"] == job_id][0]
assert final_job["repeat"]["completed"] == 2
else:
# Job was removed because completed >= times, which is correct
pass


def test_update_job_repeat_negative_or_zero_is_rejected():
"""
Test that updating repeat to non-positive values is rejected.

This matches create_job() behavior where repeat <= 0 is treated as None.
"""
with tempfile.TemporaryDirectory() as td:
tmp = Path(td)
jobs.CRON_DIR = tmp / "cron"
jobs.JOBS_FILE = jobs.CRON_DIR / "jobs.json"
jobs.OUTPUT_DIR = jobs.CRON_DIR / "output"

job = jobs.create_job(
prompt="test prompt",
schedule="every 1h",
name="test-job",
repeat=5,
deliver="local",
)
job_id = job["id"]

# Attempt to set repeat=0 via update (should be normalized to None)
updated = jobs.update_job(job_id, {"repeat": 0})
# After update, repeat should be None (normalized)
assert updated is not None
assert updated.get("repeat") is None, (
f"Repeat=0 should be normalized to None, got {updated.get('repeat')}"
)
37 changes: 37 additions & 0 deletions tests/tools/test_file_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,4 +323,41 @@ def test_truncated_hint_with_nonzero_offset(self, mock_get):
assert "offset=100" in raw


class TestPatchSchema:
"""Tests for PATCH_SCHEMA to ensure required parameters are properly declared."""

def test_patch_schema_includes_all_required_params(self):
"""PATCH_SCHEMA should include all parameters that are conditionally required."""
from tools.file_tools import PATCH_SCHEMA

# Verify schema structure
assert "parameters" in PATCH_SCHEMA
assert "required" in PATCH_SCHEMA["parameters"]

# All parameters that are mode-specific should be in required list
required = PATCH_SCHEMA["parameters"]["required"]
assert "mode" in required
assert "path" in required
assert "old_string" in required
assert "new_string" in required
assert "patch" in required

# replace_all is optional (has default), so it should NOT be in required
assert "replace_all" not in required

def test_patch_schema_description_mentions_mode_specific_requirements(self):
"""PATCH_SCHEMA description should explain mode-specific requirements."""
from tools.file_tools import PATCH_SCHEMA

description = PATCH_SCHEMA.get("description", "")

# Description should mention mode-specific requirements
assert "mode-specific" in description.lower() or "IMPORTANT:" in description

# Should mention both modes
assert "mode='replace'" in description
assert "mode='patch'" in description




12 changes: 6 additions & 6 deletions tools/file_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -908,18 +908,18 @@ def _check_file_reqs():

PATCH_SCHEMA = {
"name": "patch",
"description": "Targeted find-and-replace edits in files. Use this instead of sed/awk in terminal. Uses fuzzy matching (9 strategies) so minor whitespace/indentation differences won't break it. Returns a unified diff. Auto-runs syntax checks after editing.\n\nReplace mode (default): find a unique string and replace it.\nPatch mode: apply V4A multi-file patches for bulk changes.",
"description": "Targeted find-and-replace edits in files. Use this instead of sed/awk in terminal. Uses fuzzy matching (9 strategies) so minor whitespace/indentation differences won't break it. Returns a unified diff. Auto-runs syntax checks after editing.\n\nIMPORTANT: Parameters are mode-specific:\n- For mode='replace': provide path, old_string, new_string (optionally replace_all)\n- For mode='patch': provide patch content",
"parameters": {
"type": "object",
"properties": {
"mode": {"type": "string", "enum": ["replace", "patch"], "description": "Edit mode: 'replace' for targeted find-and-replace, 'patch' for V4A multi-file patches", "default": "replace"},
"path": {"type": "string", "description": "File path to edit (required for 'replace' mode)"},
"old_string": {"type": "string", "description": "Text to find in the file (required for 'replace' mode). Must be unique in the file unless replace_all=true. Include enough surrounding context to ensure uniqueness."},
"new_string": {"type": "string", "description": "Replacement text (required for 'replace' mode). Can be empty string to delete the matched text."},
"path": {"type": "string", "description": "File path to edit (required when mode='replace')"},
"old_string": {"type": "string", "description": "Text to find in file (required when mode='replace'). Must be unique in file unless replace_all=true. Include enough surrounding context to ensure uniqueness."},
"new_string": {"type": "string", "description": "Replacement text (required when mode='replace'). Can be empty string to delete the matched text."},
"replace_all": {"type": "boolean", "description": "Replace all occurrences instead of requiring a unique match (default: false)", "default": False},
"patch": {"type": "string", "description": "V4A format patch content (required for 'patch' mode). Format:\n*** Begin Patch\n*** Update File: path/to/file\n@@ context hint @@\n context line\n-removed line\n+added line\n*** End Patch"}
"patch": {"type": "string", "description": "V4A format patch content (required when mode='patch'). Format:\n*** Begin Patch\n*** Update File: path/to/file\n@@ context hint @@\n context line\n-removed line\n+added line\n*** End Patch"}
},
"required": ["mode"]
"required": ["mode", "path", "old_string", "new_string", "patch"]
}
}

Expand Down