feat(scheduler): add BaseJoblet

[olivermeyer]

No description provided.

[claude]

Claude finished @olivermeyer's task in 2m 31s —— View job

---

Code Review Complete

Reviewed PR #44: feat(scheduler): add BaseJoblet

Summary

This PR adds BaseJoblet - a well-designed base class for building modular Chancy job units. The implementation is solid, but there are blocking issues that must be addressed before merge.

---

🚫 Blocking Issues

1. Version Rollback

Location: pyproject.toml:3

Issue: Version changed from 0.10.0 → 0.9.0 (rollback)

Impact: This will cause deployment conflicts and package version confusion. Version numbers should only move forward.

Fix: Revert to 0.10.0 or bump to 0.11.0 if this is a new minor feature.

# Current (WRONG):
-version = "0.10.0"
+version = "0.9.0"

# Should be:
version = "0.11.0"  # or keep 0.10.0

---

2. Insufficient Test Coverage

Location: tests/aignostics_foundry_core/scheduler_test.py

Issue:

• Codecov reports 58.23% patch coverage (33 lines missing)
• SonarQube reports 55.0% coverage on new code
• Project requirement: 85% minimum

Missing Coverage Areas:

1. Error paths - No tests for exceptions/failures
2. Edge cases:
   • Multiple queues in get_queues()
   • Multiple jobs in get_jobs()
   • Cron job with and without unique_key in same batch
   • Empty operations list in triggers
3. Logging verification - Log messages aren't tested
4. State changes - Queue state after registration isn't verified

Fix: Add tests for missing paths. Examples:

# Test multiple queues
@pytest.mark.unit
async def test_register_queues_handles_multiple_queues(self) -> None:
    """register_queues declares all queues returned by get_queues()."""
    class _MultiQueueJoblet(BaseJoblet):
        @staticmethod
        def get_queues() -> list[Queue]:
            return [Queue("queue1"), Queue("queue2")]
    
    joblet = _MultiQueueJoblet()
    mock_chancy = MagicMock(spec=Chancy)
    mock_chancy.declare = AsyncMock()
    
    await joblet.register_queues(mock_chancy)
    
    assert mock_chancy.declare.call_count == 2

# Test exception handling in register_crons
@pytest.mark.unit
async def test_register_crons_continues_on_schedule_failure(self) -> None:
    """register_crons continues processing remaining crons if one fails."""
    # Implementation left as exercise

---

3. Code Smell - Artificial Async

Location: src/aignostics_foundry_core/scheduler.py:210

Issue:

async def unregister_jobs(self, chancy: Chancy) -> None:
    """..."""
    _ = chancy
    await asyncio.sleep(0)  # SonarQube wants this function to do something async

Problem: Adding asyncio.sleep(0) just to satisfy SonarQube is a code smell. This indicates the function shouldn't be async.

Fix Options:

1. Make it synchronous (preferred if no callers need async):

def unregister_jobs(self, chancy: Chancy) -> None:
    """Unregister jobs from the scheduler.
    
    Note: Currently no-op as Chancy doesn't provide delete_job API.
    """
    logger.warning(
        "Joblet {} unregister_jobs called, but not implemented as Chancy does not provide delete_job API.",
        self.key(),
    )

2. Keep async with proper justification:

async def unregister_jobs(self, chancy: Chancy) -> None:
    """..."""
    # Async signature maintained for API consistency with other lifecycle methods.
    # Implementation pending Chancy delete_job API.
    logger.warning(...)

---

💡 Suggestions (Non-Blocking)

1. Extract Log Message Constants

Location: Throughout scheduler.py

Current: Log messages are hardcoded strings:

logger.trace("Joblet {} registering queue '{}'.", self.key(), queue.name)
logger.debug("Joblet {} registered queue '{}' with state {}.", self.key(), queue.name, queue.state)

Suggestion: While not duplicated 3+ times (no S1192 violation yet), consider extracting for consistency:

# At module level
_LOG_REGISTERING_QUEUE = "Joblet {} registering queue '{}'."
_LOG_REGISTERED_QUEUE = "Joblet {} registered queue '{}' with state {}."

# Usage
logger.trace(_LOG_REGISTERING_QUEUE, self.key(), queue.name)
logger.debug(_LOG_REGISTERED_QUEUE, self.key(), queue.name, queue.state)

Benefit: Easier to update messages, better for i18n if needed later.

---

2. Document unregister_jobs Limitation More Prominently

Location: scheduler.py:198-214

Current: TODO comment in implementation

Suggestion: Add to class docstring:

class BaseJoblet:
    """Base class for joblets that can be auto-registered with the scheduler.
    
    ...existing docs...
    
    Limitations:
        - unregister_jobs() is currently a no-op as Chancy does not provide
          a delete_job API. Jobs with unique keys will persist until manually
          removed or expired.
    """

---

3. Add Integration Tests

Location: Test suite

Current: Only unit tests (all mocked)

Suggestion: Add integration tests with a real Chancy instance:

@pytest.mark.integration
async def test_full_lifecycle_with_real_chancy() -> None:
    """Test complete register/unregister cycle with real Chancy instance."""
    # Use test database
    # Create real Chancy instance
    # Test actual registration/unregistration

Benefit: Catch integration issues that mocks won't reveal.

---

4. Consider Trigger Matching Logic Robustness

Location: scheduler.py:156-159

Current:

if (
    trigger_config.table_name == table_name
    and set(trigger_config.operations) == set(operations)
    and trigger_config.job_template.func == job_template.func
):

Concern: Matching by func reference may fail if functions are reloaded (e.g., hot reload, module reimport).

Suggestion: Consider adding explicit trigger IDs or matching by qualified name:

# Option: Match by qualified name instead
and trigger_config.job_template.func.__qualname__ == job_template.func.__qualname__

---

✅ What's Good

1. Excellent Documentation: AGENTS.md entry is comprehensive with clear examples
2. Proper Test Markers: All tests correctly use @pytest.mark.unit
3. String Constants in Tests: Follows S1192 rule properly
4. Type Hints: Comprehensive type annotations throughout
5. Clean Architecture: Clear separation of concerns (get_* hooks vs register_* methods)
6. Helpful Logging: Good use of trace/debug/warning levels
7. Conventional Commits: Commit message follows standards
8. Dependency Documentation: Clear dependency requirements in AGENTS.md

---

Action Items

Before Merge:

• Fix version rollback in pyproject.toml
• Add tests to reach 85%+ coverage
• Fix or justify asyncio.sleep(0) code smell
• Run mise run lint and fix any issues
• Run full test suite: mise run all

Recommended (can be follow-up PR):

• Extract log message constants
• Add integration tests
• Enhance unregister_jobs documentation
• Consider trigger matching robustness

---

Note: I cannot run mise run lint or tests myself. Please run these locally:

mise run lint          # Must pass
mise run test_unit     # Must pass with 85%+ coverage
mise run all           # Full validation

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines	Coverage Δ
src/aignostics_foundry_core/scheduler.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
1 Accepted issue

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[olivermeyer]

Claude Code review is outdated.