feat: stateful monitor contract for scheduler (change-detection core) - #3845
feat: stateful monitor contract for scheduler (change-detection core)#3845praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
Extend the core scheduler contract so change-detection monitor automations can be expressed across CLI + YAML + Python, keeping heavy implementation (shell/URL probe, hashing, diffing, persistence) in the wrapper. Additive and backward-compatible: jobs that set neither `monitor` nor per-job state behave exactly as today. - GateResult: add `no_change` (distinct silent-suppress outcome) and `state_updates` (bounded KV a gate persists for the next tick). - JobConditionProtocol.should_run: optional keyword-only `state` so a stateful monitor gate can compare against prior state; existing stateless gates satisfy the protocol unchanged. - New JobStateStoreProtocol: bounded per-job get/set/clear scratchpad. - RunRecord.status: add `no_change` alongside succeeded/failed/skipped. - ScheduleJob: add optional `monitor` source spec with round-trip. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe scheduler now supports state-aware condition gates, per-job state storage contracts, monitor specifications on scheduled jobs, persisted monitor configuration, and a distinct ChangesStateful scheduler monitoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR adds scheduler contracts and serialized model fields for stateful change detection while leaving concrete monitor execution and persistence to the wrapper layer.
Confidence Score: 4/5The PR should not merge until the outstanding callable-contract incompatibility for existing stateless gates is resolved. Existing should_run(self, job) gates still lack the state keyword declared by JobConditionProtocol, so runtime protocol checks pass while static structural conformance fails. Files Needing Attention: src/praisonai-agents/praisonaiagents/scheduler/protocols.py and src/praisonai-bot/praisonai_bot/scheduler/condition_gate.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/scheduler/protocols.py | Adds stateful gate and state-store contracts, but the expanded gate signature remains incompatible with existing stateless implementations under static structural typing. |
| src/praisonai-agents/praisonaiagents/scheduler/models.py | Adds monitor configuration and no-change status fields with additive dictionary serialization. |
| src/praisonai-agents/praisonaiagents/scheduler/init.py | Lazily exports JobStateStoreProtocol consistently through type-checking imports, runtime lookup, caching, and all. |
Sequence Diagram
sequenceDiagram
participant Executor
participant Store as JobStateStore
participant Gate as JobCondition
participant Agent
Executor->>Store: get_state(job_id)
Store-->>Executor: prior state
Executor->>Gate: "should_run(job, state=prior)"
Gate-->>Executor: GateResult
alt source changed
Executor->>Store: set_state(job_id, state_updates)
Executor->>Agent: run with change context
else no change
Executor->>Store: set_state(job_id, state_updates)
Executor-->>Executor: record no_change
end
Reviews (2): Last reviewed commit: "fix: enforce no_change invariant and pre..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| """ | ||
|
|
||
| def should_run(self, job: Any) -> "GateResult": | ||
| """Return a :class:`GateResult` deciding whether ``job`` should run.""" | ||
| def should_run( |
There was a problem hiding this comment.
When an existing stateless gate implements should_run(self, job), it cannot satisfy the expanded callable contract or accept a state-aware call using state=, causing downstream type-checking failures or a runtime TypeError despite the documented backward-compatibility guarantee.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/scheduler/protocols.py (1)
50-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the optional per-job state operations into capability protocols.
JobStateStoreProtocolcurrently requiresget_state,set_state, andclear_state, so aget_state-only store does not satisfy the protocol. Documented optional methods andhasattr()capability checks conflict with this single protocol.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/scheduler/protocols.py` around lines 50 - 79, Split JobStateStoreProtocol into separate capability protocols for reading, writing, and clearing state, so stores can implement only the operations they support. Update each protocol to expose only its corresponding method, and retain the existing job-state documentation and method signatures while making capability detection via hasattr() consistent with the protocol definitions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/scheduler/models.py`:
- Around line 397-400: Update the monitor serialization condition in the model’s
to_dict logic to persist self.monitor whenever it is not None, including an
explicitly configured empty mapping; preserve omission only when monitor is
unset.
In `@src/praisonai-agents/praisonaiagents/scheduler/protocols.py`:
- Around line 43-47: Update the GateResult definition so no_change=True cannot
coexist with the default run=True: force run=False when no_change is set, or
validate and reject the conflicting combination. Preserve normal execution for
results where no_change is false.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/scheduler/protocols.py`:
- Around line 50-79: Split JobStateStoreProtocol into separate capability
protocols for reading, writing, and clearing state, so stores can implement only
the operations they support. Update each protocol to expose only its
corresponding method, and retain the existing job-state documentation and method
signatures while making capability detection via hasattr() consistent with the
protocol definitions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3c1ee43-c932-4fd5-aad0-c7011e3303cb
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/scheduler/__init__.pysrc/praisonai-agents/praisonaiagents/scheduler/models.pysrc/praisonai-agents/praisonaiagents/scheduler/protocols.py
| # Monitor source spec. Only persist when configured so stateless jobs | ||
| # stay byte-for-byte unchanged; the shape is opaque to the core. | ||
| if self.monitor: | ||
| d["monitor"] = self.monitor |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve an explicitly configured empty monitor mapping.
from_dict({"monitor": {}}) restores monitor={}, but to_dict() omits it because {} is falsy. A load/save cycle then changes the value to None and silently changes the job to stateless behavior. Persist the field when it is not None.
Proposed fix
- if self.monitor:
+ if self.monitor is not None:
d["monitor"] = self.monitor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Monitor source spec. Only persist when configured so stateless jobs | |
| # stay byte-for-byte unchanged; the shape is opaque to the core. | |
| if self.monitor: | |
| d["monitor"] = self.monitor | |
| # Monitor source spec. Only persist when configured so stateless jobs | |
| # stay byte-for-byte unchanged; the shape is opaque to the core. | |
| if self.monitor is not None: | |
| d["monitor"] = self.monitor |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/scheduler/models.py` around lines 397 -
400, Update the monitor serialization condition in the model’s to_dict logic to
persist self.monitor whenever it is not None, including an explicitly configured
empty mapping; preserve omission only when monitor is unset.
| run: bool = True | ||
| context: Optional[str] = None | ||
| reason: Optional[str] = None | ||
| no_change: bool = False | ||
| state_updates: Optional[Dict[str, Any]] = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the no_change invariant.
GateResult(no_change=True) keeps run=True from its default value. A runner can then execute and deliver a tick that the contract defines as suppressed. Force run=False when no_change is set, or reject the conflicting combination.
Proposed fix
class GateResult:
+ def __post_init__(self) -> None:
+ if self.no_change:
+ self.run = False
+
run: bool = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: bool = True | |
| context: Optional[str] = None | |
| reason: Optional[str] = None | |
| no_change: bool = False | |
| state_updates: Optional[Dict[str, Any]] = None | |
| def __post_init__(self) -> None: | |
| if self.no_change: | |
| self.run = False | |
| run: bool = True | |
| context: Optional[str] = None | |
| reason: Optional[str] = None | |
| no_change: bool = False | |
| state_updates: Optional[Dict[str, Any]] = None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/scheduler/protocols.py` around lines 43
- 47, Update the GateResult definition so no_change=True cannot coexist with the
default run=True: force run=False when no_change is set, or validate and reject
the conflicting combination. Preserve normal execution for results where
no_change is false.
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3845
Phase 1 — Architecture verdict ✅
Phase 2 — Fixes pushed (commit
|
- GateResult.__post_init__ forces run=False when no_change is set so a
silent-suppress outcome can never let a runner fire a suppressed tick.
- ScheduleJob.to_dict persists monitor when not None (incl. {}) so an
explicitly configured empty spec round-trips instead of reverting to
stateless.
- Document caller contract: detect state capability before passing
state= so legacy stateless gates never raise TypeError.
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
the
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
pipeline/blocked:stale-final
1 participant
Fixes #3844
Summary
Adds the core contract increment that makes the scheduler's condition gates stateful and change-aware, so unattended monitor automations ("wake only when a watched source changed, and tell me only what changed") can be expressed across CLI + YAML + Python — without burning a full model turn on every tick against an unchanged source.
Scoped to core protocols/models only per AGENTS.md (lightweight & powerful; protocols in core, heavy impl in wrapper). Fully additive and backward-compatible: jobs that set neither
monitornor per-job state behave exactly as today.Changes (
praisonaiagents/scheduler)GateResult(protocols.py): addno_change(a distinct silent-suppress "monitor mode" outcome, separate from a genericrun=Falseskip) andstate_updates(bounded KV the gate wants persisted for the next tick — last-seen hash, watermark).JobConditionProtocol.should_run: optional keyword-onlystateparam so a stateful monitor gate can compare against prior per-job state. Existing stateless gates (should_run(self, job)) satisfy the protocol unchanged.JobStateStoreProtocol(new): bounded per-jobget_state/set_state/clear_statescratchpad — a job's durable memory across wake-ups. Contract only; concrete store lives in the wrapper.RunRecord.status(models.py): addno_changealongsidesucceeded/failed/skipped.ScheduleJob: add optionalmonitorsource spec ({"command": ...}/{"url": ...}) withto_dict/from_dictround-trip (omitted from serialization when unset).scheduler/__init__.py.Out of scope (follow-up, wrapper/bot layer)
The heavy
MonitorGate(shell/URL probe + hashing + bounded diff), executor persistence of state + theno_changerecord, and standalone-sender suppression — deliberately kept out of core.Test plan
monitorround-trips and is omitted when unset;GateResult(no_change, state_updates);RunRecord(status="no_change")round-trips; stateless gate still satisfiesJobConditionProtocol; a get/set/clear store satisfiesJobStateStoreProtocol.Generated with Claude Code
Summary by CodeRabbit