Address code review comments: validation, logging, and constants

This commit addresses three review feedback items:

1. **Input Validation for Progress Methods**
   - Added validation in Progress.set_total() - must be positive
   - Added validation in Progress.set() - must be non-negative and <= total
   - Raises ValueError with descriptive messages
   - Added 4 tests to verify validation behavior

2. **Improved Lua Script Error Handling**
   - Added logging when mark_task_completed() encounters missing keys
   - Lua script returns 0 when keys don't exist (unchanged)
   - Now logs WARNING when result is 0, with key details
   - Added DEBUG logging when script is evicted and reloaded
   - Updated test to verify warning is logged

3. **Constant for Default Progress Total**
   - Added DEFAULT_PROGRESS_TOTAL = 100 constant
   - Used in ProgressInfo dataclass default
   - Used in ProgressInfo.from_record() fallback
   - Used in Progress.__aenter__() initialization
   - Eliminates hardcoded 100 throughout codebase

Changes:
- src/docket/state.py: Added constant, logging, improved error handling
- src/docket/dependencies.py: Added validation, used constant
- tests/test_dependencies.py: Added 4 validation tests
- tests/test_state.py: Updated test to verify logging

Test results: 58 tests passed, 96% coverage for state.py

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Author: desertaxle

src/docket/dependencies.py

@@ -680,12 +680,14 @@ def __init__(self) -> None:
         self._current: int = 0
 
     async def __aenter__(self) -> "Progress":
+        from docket.state import DEFAULT_PROGRESS_TOTAL
+
         execution = self.execution.get()
         docket = self.docket.get()
 
         self._key = execution.key
         self._docket = docket
-        self._total = 100
+        self._total = DEFAULT_PROGRESS_TOTAL
         self._current = 0
         self._store = TaskStateStore(docket, docket.record_ttl)
 
@@ -708,8 +710,13 @@ async def set_total(self, total: int) -> None:
         """Set the total expected progress value.
 
         Args:
-            total: Total expected progress value
+            total: Total expected progress value (must be positive)
+
+        Raises:
+            ValueError: If total is not positive
         """
+        if total <= 0:
+            raise ValueError(f"Progress total must be positive, got {total}")
         self._total = total
         await self._store.set_task_progress(
             self._key, ProgressInfo(current=self._current, total=self._total)
@@ -727,8 +734,17 @@ async def set(self, current: int) -> None:
         """Set the current progress value directly.
 
         Args:
-            current: Current progress value
+            current: Current progress value (must be non-negative and <= total)
+
+        Raises:
+            ValueError: If current is negative or exceeds total
         """
+        if current < 0:
+            raise ValueError(f"Progress current must be non-negative, got {current}")
+        if current > self._total:
+            raise ValueError(
+                f"Progress current ({current}) cannot exceed total ({self._total})"
+            )
         self._current = current
         await self._store.set_task_progress(
             self._key, ProgressInfo(current=self._current, total=self._total)

src/docket/state.py

@@ -1,3 +1,4 @@
+import logging
 from dataclasses import dataclass, field
 from datetime import datetime, timezone
 from typing import TYPE_CHECKING, Any, cast
@@ -7,6 +8,12 @@
 if TYPE_CHECKING:
     from docket import Docket
 
+logger: logging.Logger = logging.getLogger(__name__)
+
+
+# Default total value for progress tracking
+DEFAULT_PROGRESS_TOTAL = 100
+
 
 @dataclass
 class ProgressInfo:
@@ -18,7 +25,7 @@ class ProgressInfo:
     """
 
     current: int = field(default=0)
-    total: int = field(default=100)
+    total: int = field(default=DEFAULT_PROGRESS_TOTAL)
 
     @property
     def percentage(self) -> float:
@@ -34,7 +41,7 @@ def to_record(self) -> dict[str, int]:
     def from_record(cls, record: dict[str, int]) -> "ProgressInfo":
         return cls(
             current=record.get("current", 0),
-            total=record.get("total", 100),
+            total=record.get("total", DEFAULT_PROGRESS_TOTAL),
         )
 
 
@@ -286,7 +293,7 @@ async def mark_task_completed(self, key: str) -> None:
 
             try:
                 # Execute using cached SHA
-                await redis.evalsha(  # pyright: ignore[reportUnknownMemberType,reportGeneralTypeIssues]
+                result = await redis.evalsha(  # pyright: ignore[reportUnknownMemberType,reportGeneralTypeIssues]
                     TaskStateStore._completion_script_sha,
                     2,  # number of keys
                     progress_key,
@@ -295,15 +302,27 @@ async def mark_task_completed(self, key: str) -> None:
                     self.record_ttl,
                 )
             except NoScriptError:
+                # Script was evicted from Redis, reload and retry
+                logger.debug("Lua script evicted from Redis, reloading for key %s", key)
                 TaskStateStore._completion_script_sha = cast(
                     str,
                     await redis.script_load(self._COMPLETION_SCRIPT),  # pyright: ignore[reportUnknownMemberType,reportGeneralTypeIssues]
                 )
-                await redis.evalsha(  # pyright: ignore[reportUnknownMemberType,reportGeneralTypeIssues]
+                result = await redis.evalsha(  # pyright: ignore[reportUnknownMemberType,reportGeneralTypeIssues]
                     TaskStateStore._completion_script_sha,
                     2,  # number of keys
                     progress_key,
                     state_key,
                     now,
                     self.record_ttl,
                 )
+
+            # Log if task state didn't exist (script returns 0)
+            if result == 0:
+                logger.warning(
+                    "Task state not found when marking completed: %s "
+                    "(progress key: %s, state key: %s)",
+                    key,
+                    progress_key,
+                    state_key,
+                )

tests/test_dependencies.py

@@ -515,3 +515,88 @@ async def task_with_progress_context(progress: Progress = Progress()):
 
     assert entered
     assert exited
+
+
+async def test_progress_set_total_validation(docket: Docket, worker: Worker):
+    """Progress.set_total() should validate input."""
+    from docket.dependencies import Progress
+
+    validation_error = None
+
+    async def task_with_invalid_total(progress: Progress = Progress()):
+        nonlocal validation_error
+        try:
+            await progress.set_total(-10)
+        except ValueError as e:
+            validation_error = e
+
+    docket.register(task_with_invalid_total)
+    await docket.add(task_with_invalid_total, key="validation-test")()
+    await worker.run_until_finished()
+
+    assert validation_error is not None
+    assert "must be positive" in str(validation_error)
+
+
+async def test_progress_set_total_zero_validation(docket: Docket, worker: Worker):
+    """Progress.set_total() should reject zero."""
+    from docket.dependencies import Progress
+
+    validation_error = None
+
+    async def task_with_zero_total(progress: Progress = Progress()):
+        nonlocal validation_error
+        try:
+            await progress.set_total(0)
+        except ValueError as e:
+            validation_error = e
+
+    docket.register(task_with_zero_total)
+    await docket.add(task_with_zero_total, key="zero-validation-test")()
+    await worker.run_until_finished()
+
+    assert validation_error is not None
+    assert "must be positive" in str(validation_error)
+
+
+async def test_progress_set_negative_validation(docket: Docket, worker: Worker):
+    """Progress.set() should validate negative values."""
+    from docket.dependencies import Progress
+
+    validation_error = None
+
+    async def task_with_negative_current(progress: Progress = Progress()):
+        nonlocal validation_error
+        try:
+            await progress.set(-5)
+        except ValueError as e:
+            validation_error = e
+
+    docket.register(task_with_negative_current)
+    await docket.add(task_with_negative_current, key="negative-validation-test")()
+    await worker.run_until_finished()
+
+    assert validation_error is not None
+    assert "must be non-negative" in str(validation_error)
+
+
+async def test_progress_set_exceeds_total_validation(docket: Docket, worker: Worker):
+    """Progress.set() should validate current doesn't exceed total."""
+    from docket.dependencies import Progress
+
+    validation_error = None
+
+    async def task_with_exceeding_current(progress: Progress = Progress()):
+        nonlocal validation_error
+        await progress.set_total(100)
+        try:
+            await progress.set(150)
+        except ValueError as e:
+            validation_error = e
+
+    docket.register(task_with_exceeding_current)
+    await docket.add(task_with_exceeding_current, key="exceeds-validation-test")()
+    await worker.run_until_finished()
+
+    assert validation_error is not None
+    assert "cannot exceed total" in str(validation_error)

tests/test_state.py

@@ -2,6 +2,7 @@
 
 from datetime import datetime, timezone
 
+import pytest
 
 from docket import Docket, Worker
 from docket.state import ProgressInfo, TaskState, TaskStateStore
@@ -290,12 +291,21 @@ async def test_get_task_state_missing_progress_key(self, docket: Docket):
         state = await store.get_task_state("test-task-key")
         assert state is None
 
-    async def test_mark_task_completed_nonexistent(self, docket: Docket):
-        """Marking nonexistent task as completed should not error."""
+    async def test_mark_task_completed_nonexistent(
+        self, docket: Docket, caplog: pytest.LogCaptureFixture
+    ):
+        """Marking nonexistent task as completed should not error but should log warning."""
+        import logging
+
         store = TaskStateStore(docket, record_ttl=3600)
 
         # Should not raise an exception
-        await store.mark_task_completed("nonexistent-key")
+        with caplog.at_level(logging.WARNING):
+            await store.mark_task_completed("nonexistent-key")
+
+        # Should log a warning about missing task state
+        assert "Task state not found when marking completed" in caplog.text
+        assert "nonexistent-key" in caplog.text
 
     async def test_mark_task_completed_missing_total(self, docket: Docket):
         """Marking task completed with missing total field should not error."""