fix: accept multi-channel bot.yaml keys with platform field#1772
fix: accept multi-channel bot.yaml keys with platform field#1772cursor[bot] wants to merge 2 commits into
Conversation
…gression) Onboard generates channels like telegram_cfo with platform: telegram. BotYamlSchema rejected any channel key not exactly matching a platform name, so praisonai doctor reported invalid bot.yaml after multi-bot onboard. Validate using channel.platform when present; extend ChannelConfigSchema for gateway-style fields used in generated configs. Co-authored-by: Mervin Praison <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? |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR extends channel configuration to support explicit per-channel platform specification. ChangesChannel Configuration Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/praisonai/tests/unit/test_bot_gap_features.py (1)
296-315: ⚡ Quick winUse
monkeypatchfor env isolation in the new test.Line 296 mutates process env directly; prefer pytest’s
monkeypatch.setenv/delenvto avoid cross-test leakage and simplify cleanup.Proposed diff
- def test_multi_channel_custom_keys_with_platform(self): + def test_multi_channel_custom_keys_with_platform(self, monkeypatch): """Onboard multi-telegram YAML uses telegram_<role> keys plus platform field.""" from praisonai.bots._config_schema import validate_bot_config - os.environ["_TEST_CFO_TOKEN"] = "cfo-token" - try: - raw = { - "channels": { - "telegram_cfo": { - "platform": "telegram", - "token": "${_TEST_CFO_TOKEN}", - "group_policy": "mention_only", - }, - "telegram_ops": { - "platform": "telegram", - "token": "${_TEST_CFO_TOKEN}", - }, - } - } - result = validate_bot_config(raw) - assert result.channels["telegram_cfo"].platform == "telegram" - assert result.channels["telegram_cfo"].token == "cfo-token" - finally: - os.environ.pop("_TEST_CFO_TOKEN", None) + monkeypatch.setenv("_TEST_CFO_TOKEN", "cfo-token") + raw = { + "channels": { + "telegram_cfo": { + "platform": "telegram", + "token": "${_TEST_CFO_TOKEN}", + "group_policy": "mention_only", + }, + "telegram_ops": { + "platform": "telegram", + "token": "${_TEST_CFO_TOKEN}", + }, + } + } + result = validate_bot_config(raw) + assert result.channels["telegram_cfo"].platform == "telegram" + assert result.channels["telegram_cfo"].token == "cfo-token"🤖 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/tests/unit/test_bot_gap_features.py` around lines 296 - 315, The test mutates os.environ directly; change it to use pytest's monkeypatch fixture to isolate env changes: add the monkeypatch parameter to the test function, replace os.environ["_TEST_CFO_TOKEN"] = "cfo-token" and the try/finally cleanup with monkeypatch.setenv("_TEST_CFO_TOKEN", "cfo-token") (or monkeypatch.delenv if needed), and then call validate_bot_config(raw) as before (refer to validate_bot_config and the test function in test_bot_gap_features.py); monkeypatch will automatically restore the environment after the test.
🤖 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.
Nitpick comments:
In `@src/praisonai/tests/unit/test_bot_gap_features.py`:
- Around line 296-315: The test mutates os.environ directly; change it to use
pytest's monkeypatch fixture to isolate env changes: add the monkeypatch
parameter to the test function, replace os.environ["_TEST_CFO_TOKEN"] =
"cfo-token" and the try/finally cleanup with
monkeypatch.setenv("_TEST_CFO_TOKEN", "cfo-token") (or monkeypatch.delenv if
needed), and then call validate_bot_config(raw) as before (refer to
validate_bot_config and the test function in test_bot_gap_features.py);
monkeypatch will automatically restore the environment after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60886665-570a-4135-8202-b1e7673f0e28
📒 Files selected for processing (2)
src/praisonai/praisonai/bots/_config_schema.pysrc/praisonai/tests/unit/test_bot_gap_features.py
|
@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 |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review Complete Phase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
Architecture Review Summary ✅All architecture requirements met:
Issues Fixed
Files Modified
What Was Skipped
Final Verdict: APPROVED ✅This PR successfully fixes the multi-channel bot.yaml validation bug introduced in #1757. The implementation is architecturally sound, maintains backward compatibility, and includes appropriate test coverage. Ready for merge. |
- Replace direct os.environ manipulation with pytest monkeypatch - Ensures test environment isolation and automatic cleanup - Addresses CodeRabbit review feedback Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Bug and impact
After #1757 (multi-Telegram onboard),
praisonai onboardwritesbot.yamlwith channel keys such astelegram_cfoand aplatform: telegramfield.BotYamlSchemaonly allowed channel keys that exactly matched platform names (telegram,discord, …), sopraisonai doctorand any path usingload_and_validate_bot_yamlfailed withUnknown channel 'telegram_cfo'even when the config was valid for the gateway.Root cause
BotYamlSchema.validate_channels_existvalidated the YAML dict key as the platform name and did not modelplatformonChannelConfigSchema.Fix
platform,routes, andallowed_userstoChannelConfigSchema.channel.platformwhen present, otherwise from the channel key.telegram_<role>keys.Validation
signalchannel still rejected; multi-channel keys accepted.Summary by CodeRabbit
platformconfiguration per channel, enabling flexible channel naming patterns.allowed_usersrestrictions.routesmappings.