fix: isolate scheduler suggestions and jobs per gateway user - #3786
fix: isolate scheduler suggestions and jobs per gateway user#3786praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…3785) Thread the already-existing `principal` identity through the user-facing scheduler surfaces so a multi-user gateway isolates each end-user's automations, while CLI / single-user behaviour stays global (unchanged): - wrapper SuggestionEngine: propose/pending/accept/dismiss accept `principal` - agent-callable schedule_add/list/remove: default `principal` from SessionContext.unified_user_id, stamp job owner, scope dup-name check - gateway _automations.py: resolve per-turn identity, refresh stale scope note Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
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? |
✅ Action performedReview finished.
|
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 and automation flows now support principal-based isolation. Principals resolve from explicit arguments or session context. Schedule and suggestion operations scope ownership checks by principal, while missing identities retain global behavior. ChangesPrincipal isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-bot/praisonai_bot/bots/_automations.py (1)
189-227: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake global-suggestion acceptance consistent and atomic.
An authenticated automation call permits a global suggestion, but
SuggestionStore.accept()rejects it when passed that user's principal. The job is created before the failed result is checked.
src/praisonai-bot/praisonai_bot/bots/_automations.py#L189-L227: reject global suggestions for authenticated users, or use an explicit one-time claim operation.src/praisonai-agents/praisonaiagents/tools/schedule_tools.py#L219-L225: check the acceptance result and prevent or roll back job creation when the claim fails.🤖 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-bot/praisonai_bot/bots/_automations.py` around lines 189 - 227, The suggestion acceptance flow must be atomic and consistent for global suggestions. In src/praisonai-bot/praisonai_bot/bots/_automations.py lines 189-227, reject global suggestions when principal is authenticated, or route them through an explicit one-time claim operation. In src/praisonai-agents/praisonaiagents/tools/schedule_tools.py lines 219-225, make the schedule_add acceptance path check the claim result and prevent or roll back job creation when claiming fails.
🤖 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/tools/schedule_tools.py`:
- Around line 26-41: The _caller_principal function must not let agent-supplied
explicit values override the authenticated session principal; update
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py lines 26-41 to use
SessionContext.unified_user_id for agent authorization, moving trusted overrides
to a separate non-agent path. Update
src/praisonai-agents/tests/unit/test_schedule_principal.py lines 284-299 to
replace the override assertion with coverage proving a session user cannot
impersonate another principal through the tool API.
In `@src/praisonai-agents/tests/unit/test_schedule_principal.py`:
- Around line 238-299: Add an appropriate e2e test that uses an Agent to call
agent.start() with a real scheduling prompt, invokes the LLM, and prints the
complete response/output. Keep the existing direct schedule_tools tests
unchanged, and ensure the new test exercises the session-isolation behavior
through the agent-facing workflow rather than calling scheduling functions
directly.
- Around line 230-236: Update the test setup around _fresh_store to capture both
the tool-level and canonical default schedule stores before calling
schedule_tools.set_store, then restore both in a fixture finalizer after each
test. Ensure cleanup leaves global stores unchanged and prevents later tests
from using the temporary-directory store.
---
Outside diff comments:
In `@src/praisonai-bot/praisonai_bot/bots/_automations.py`:
- Around line 189-227: The suggestion acceptance flow must be atomic and
consistent for global suggestions. In
src/praisonai-bot/praisonai_bot/bots/_automations.py lines 189-227, reject
global suggestions when principal is authenticated, or route them through an
explicit one-time claim operation. In
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py lines 219-225, make
the schedule_add acceptance path check the claim result and prevent or roll back
job creation when claiming fails.
🪄 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: b749eb5c-6716-4b7c-80f2-d4f363cf2d3b
📒 Files selected for processing (4)
src/praisonai-agents/praisonaiagents/tools/schedule_tools.pysrc/praisonai-agents/tests/unit/test_schedule_principal.pysrc/praisonai-bot/praisonai_bot/bots/_automations.pysrc/praisonai/praisonai/scheduler/suggestion_engine.py
| def _caller_principal(explicit: str = "") -> Optional[str]: | ||
| """Resolve the calling end-user's canonical identity. | ||
|
|
||
| Prefers an ``explicit`` override, then the per-turn | ||
| ``SessionContext.unified_user_id`` set by the bot session manager on a | ||
| multi-user gateway. Returns ``None`` when no identity is resolved so the | ||
| scheduler stores fall back to their global, single-tenant behaviour | ||
| (CLI / single-user deployments are unchanged). | ||
| """ | ||
| if explicit: | ||
| return explicit | ||
| try: | ||
| from ..session.context import get_session_context | ||
| return get_session_context().unified_user_id or None | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not allow an agent tool argument to override the authenticated principal.
The shared root cause is that principal is treated as caller-controlled data on an agent-callable surface. This bypasses tenant isolation, and the test enforces that unsafe contract.
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py#L26-L41: useSessionContext.unified_user_idfor agent tool authorization. Move trusted overrides to a separate non-agent path.src/praisonai-agents/tests/unit/test_schedule_principal.py#L284-L299: replace the override assertion with a test that a session user cannot impersonate another principal through the tool API.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 40-40: Do not catch blind exception: Exception
(BLE001)
📍 Affects 2 files
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py#L26-L41(this comment)src/praisonai-agents/tests/unit/test_schedule_principal.py#L284-L299
🤖 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/tools/schedule_tools.py` around lines 26
- 41, The _caller_principal function must not let agent-supplied explicit values
override the authenticated session principal; update
src/praisonai-agents/praisonaiagents/tools/schedule_tools.py lines 26-41 to use
SessionContext.unified_user_id for agent authorization, moving trusted overrides
to a separate non-agent path. Update
src/praisonai-agents/tests/unit/test_schedule_principal.py lines 284-299 to
replace the override assertion with coverage proving a session user cannot
impersonate another principal through the tool API.
| def _fresh_store(self, tmp_dir): | ||
| from praisonaiagents.scheduler.store import FileScheduleStore | ||
| from praisonaiagents.tools import schedule_tools | ||
|
|
||
| store = FileScheduleStore(store_dir=tmp_dir) | ||
| schedule_tools.set_store(store) | ||
| return store |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the global schedule stores after each test.
set_store() changes both the tool-level store and the canonical default store. The final test leaves them pointing at a deleted temporary directory. Later tests can depend on execution order or use this stale store.
Capture and restore both stores in a fixture finalizer. As per coding guidelines, “keep tests deterministic without dependence on timing or external state.”
🤖 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/tests/unit/test_schedule_principal.py` around lines 230
- 236, Update the test setup around _fresh_store to capture both the tool-level
and canonical default schedule stores before calling schedule_tools.set_store,
then restore both in a fixture finalizer after each test. Ensure cleanup leaves
global stores unchanged and prevents later tests from using the
temporary-directory store.
Source: Coding guidelines
| def test_tools_isolate_by_session_context(self): | ||
| from praisonaiagents.session.context import ( | ||
| set_session_context, | ||
| clear_session_context, | ||
| ) | ||
| from praisonaiagents.tools.schedule_tools import ( | ||
| schedule_add, | ||
| schedule_list, | ||
| schedule_remove, | ||
| ) | ||
|
|
||
| with tempfile.TemporaryDirectory() as d: | ||
| store = self._fresh_store(d) | ||
|
|
||
| tok = set_session_context(unified_user_id="alice") | ||
| try: | ||
| schedule_add("brief", "daily", message="Alice brief") | ||
| finally: | ||
| clear_session_context(tok) | ||
|
|
||
| tok = set_session_context(unified_user_id="bob") | ||
| try: | ||
| # Bob only sees his own (empty) list … | ||
| assert "No schedules found" in schedule_list() | ||
| # … cannot remove Alice's job by name … | ||
| assert "not found" in schedule_remove("brief") | ||
| # … and can add his own under the same name (isolated). | ||
| assert "added" in schedule_add("brief", "daily", message="Bob brief") | ||
| listed = schedule_list() | ||
| assert "Bob brief" in listed and "Alice brief" not in listed | ||
| finally: | ||
| clear_session_context(tok) | ||
|
|
||
| # The underlying store still holds both, tagged per owner. | ||
| assert {j.principal for j in store.list()} == {"alice", "bob"} | ||
|
|
||
| def test_tools_global_without_identity(self): | ||
| from praisonaiagents.tools.schedule_tools import schedule_add, schedule_list | ||
|
|
||
| with tempfile.TemporaryDirectory() as d: | ||
| self._fresh_store(d) | ||
| # No session context → no identity → global pool (CLI behaviour). | ||
| schedule_add("cli-job", "daily", message="cli") | ||
| listed = schedule_list() | ||
| assert "cli-job" in listed | ||
|
|
||
| def test_explicit_principal_overrides_context(self): | ||
| from praisonaiagents.session.context import ( | ||
| set_session_context, | ||
| clear_session_context, | ||
| ) | ||
| from praisonaiagents.tools.schedule_tools import schedule_add, schedule_list | ||
|
|
||
| with tempfile.TemporaryDirectory() as d: | ||
| store = self._fresh_store(d) | ||
| tok = set_session_context(unified_user_id="alice") | ||
| try: | ||
| schedule_add("j", "daily", principal="carol") | ||
| finally: | ||
| clear_session_context(tok) | ||
| assert store.get_by_name("j").principal == "carol" | ||
| assert "j" in schedule_list(principal="carol") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add the required real agentic test.
These tests call scheduling functions directly. They do not verify that an Agent calls agent.start() with a real prompt, invokes the LLM, and prints the full output. Add that coverage in the appropriate e2e category.
As per coding guidelines, “Every feature requires both smoke tests and a real agentic test in which an Agent calls agent.start() with a real prompt, invokes the LLM, and prints the full output.”
🤖 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/tests/unit/test_schedule_principal.py` around lines 238
- 299, Add an appropriate e2e test that uses an Agent to call agent.start() with
a real scheduling prompt, invokes the LLM, and prints the complete
response/output. Keep the existing direct schedule_tools tests unchanged, and
ensure the new test exercises the session-isolation behavior through the
agent-facing workflow rather than calling scheduling functions directly.
Source: Coding guidelines
Greptile SummaryThe PR threads scheduler ownership through suggestion and schedule surfaces, but callback-driven automation actions still lack the identity context required to enforce that isolation.
Confidence Score: 3/5The PR is not yet safe to merge because callback-driven automation actions can still accept or dismiss another user's suggestion without an ownership check. Platform callback dispatch retains the platform user ID only in InteractiveContext and does not install SessionContext, while the changed handlers resolve identity solely from SessionContext; this sends principal=None to store operations that deliberately skip ownership enforcement. Files Needing Attention: src/praisonai-bot/praisonai_bot/bots/_automations.py and the platform callback dispatch paths
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/tools/schedule_tools.py | Authenticated session identity now overrides the agent-visible principal argument and scopes schedule operations. |
| src/praisonai-bot/praisonai_bot/bots/_automations.py | Principal forwarding is added, but callback handlers still reach these functions without the ambient identity they require. |
| src/praisonai/praisonai/scheduler/suggestion_engine.py | The wrapper consistently forwards optional principals to the existing principal-aware store. |
| src/praisonai-agents/tests/unit/test_schedule_principal.py | Tests cover schedule-tool context defaulting, authenticated precedence, isolation, and context-free compatibility. |
| src/praisonai-bot/tests/unit/bots/test_automations_commands.py | Test doubles support principal-aware APIs, but callback dispatch without SessionContext is not covered. |
Sequence Diagram
sequenceDiagram
participant User
participant Adapter as Platform callback adapter
participant Handler as Automation handler
participant Context as SessionContext
participant Store as SuggestionStore
User->>Adapter: Tap Accept or Dismiss
Adapter->>Handler: Dispatch InteractiveContext
Note over Adapter,Handler: SessionContext is not installed
Handler->>Context: Read unified_user_id
Context-->>Handler: Empty / None
Handler->>Store: "accept/dismiss(id, principal=None)"
Store-->>Handler: Mutation without ownership check
Reviews (2): Last reviewed commit: "fix: make session identity authoritative..." | Re-trigger Greptile
|
@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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
MervinPraison
added
pipeline/awaiting-merge-gate
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job FINAL Architecture Review — PR #3786 ✅Verdict: legitimate feature + regression-fix. The PR's isolation intent is correct and adds real SDK value (per-tenant scheduler isolation, backward-compatible). I fixed one critical security gap and the 3 CI failures at their root cause. Pushed to Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Fixes pushed
Test results (local)
Skipped / follow-up (out of minimal scope)
Phase 3 — Verdict✅ Approve (non-blocking follow-ups noted). CI root causes fixed, tenant-isolation hardened without weakening tests or touching core SDK contracts. Not modified: Job run · Branch: |
…CI test doubles Security: _caller_principal now treats the authenticated SessionContext. unified_user_id as authoritative — an agent-supplied `principal` tool argument can no longer override it, closing the cross-tenant impersonation gap flagged by CodeRabbit/Greptile. Explicit principal still applies on the trusted no-session path (CLI / gateway bridge). CI regressions (stale test doubles): - _FakeEngine now accepts principal= (matches the real principal-aware SuggestionEngine API the gateway threads through). - async_tui parity fake _execute now accepts read_only= (param added by the earlier /code-review PR); pre-existing failure unrelated to this feature. Tests: rewrote impersonation coverage (session wins), added no-session explicit path test, and restore both global schedule stores in teardown for determinism. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
added
the
pipeline/blocked:stale-final
MervinPraison
removed
the
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Failure Fix — PR #3786Verdict: already fixed — no new changes required. The failures were reported against
Local results on
|
| Test | Result |
|---|---|
test_automations_commands.py::test_dismiss_suggestion |
✅ pass |
test_automations_commands.py::test_list_suggestions_renders_buttons |
✅ pass |
test_interactive_surfaces_parity.py::test_async_tui_expands_at_mentions |
✅ pass |
test_automations_commands.py (full) |
✅ 13 passed |
test_interactive_surfaces_parity.py (full) |
✅ 2 passed, 2 skipped |
test_schedule_principal.py (full) |
✅ 20 passed |
Why the failures happened (root cause, per commit 78df9566f)
- bots-gateway — the PR correctly calls
engine.pending(principal=…)/dismiss(…, principal=…), but the stale test doubles (_FakeEngine,_FakeSuggestion) didn't acceptprincipal, so they raisedTypeError(swallowed → empty list). The fakes were updated to match the real API. No test weakening — the fakes now faithfully mirror the principal-aware contract. - cli —
test_async_tui_expands_at_mentions's fake_execute_promptdidn't accept theread_only=kwarg introduced by an earlier PR (unrelated to scheduler). The fake signature was updated. Pre-existing staleness, not a scheduler regression.
Both fixes correct the test doubles to the real interfaces, so the tests continue to guard backward compatibility and hot-path behaviour rather than masking a break.
The CI run that reported these was against the older commit; a re-run on HEAD 78df9566f should be green. Working tree is clean — no commit needed.
Job run · Branch: claude/issue-3785-20260807-0946 · Commit 78df9566f
| @@ -165,11 +180,18 @@ def accept_suggestion(suggestion_id: str, deliver: str = "") -> str: | |||
| if engine is None: | |||
There was a problem hiding this comment.
When an authorized Telegram, Slack, or Discord user taps an Accept or Dismiss callback, callback dispatch does not install SessionContext, so _principal() returns None. The changed handlers therefore skip the cross-owner guard and invoke globally scoped store operations, allowing the callback to accept or dismiss another user's suggestion; acceptance also creates an unowned schedule.
Knowledge Base Used: praisonai-bot
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3785
Summary
On a multi-user gateway, the scheduler's automation suggestions and scheduled jobs were shared across all users: every user saw/accepted/dismissed the same global suggestion pool, and
schedule_list/schedule_removeoperated across everyone's jobs.The isolation contract already existed in core (
principalonSuggestion,SuggestionStore,ScheduleStore) — the defect was pure wiring: the user-facing surfaces never threaded identity through. This PR closes that gap, additively and backward-compatibly.Changes
SuggestionEngine(praisonai/scheduler/suggestion_engine.py):proposestampsprincipal;pending/accept/dismissaccept and forwardprincipal.praisonaiagents/tools/schedule_tools.py): new_caller_principal()helper defaults the owner fromSessionContext.unified_user_id.schedule_addstamps the job owner and scopes the duplicate-name check;schedule_list/schedule_removefilter by the caller. All accept an explicitprincipaloverride.praisonai-bot/.../bots/_automations.py): resolves the per-turn identity once and threads it throughpending/accept/dismiss+schedule_add; refuses cross-owner suggestion reads; refreshed the stale scope note.Backward compatibility
No identity resolved (CLI / single-user) ⇒
principal=None⇒ global pool ⇒ today's behaviour, byte-for-byte. No protocol changes — only defaulting-from-context at the surfaces.Tests
Added
TestScheduleToolsDefaultPrincipalcovering context-defaulting, isolation, global fallback, and explicit override. Full scheduler/suggestion unit suite: 143 passed.Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes