fix: accept multi-channel bot.yaml keys with platform field#1772
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>
Review Summary by QodoAccept multi-channel bot.yaml keys with platform field
WalkthroughsDescription• Accept multi-channel bot.yaml keys with explicit platform field • Extend ChannelConfigSchema with platform, allowed_users, routes fields • Resolve platform from channel.platform when present, fallback to key • Add regression test for telegram_ multi-channel keys Diagramflowchart LR
A["bot.yaml with<br/>telegram_cfo key"] -->|"platform: telegram<br/>field present"| B["ChannelConfigSchema<br/>with platform field"]
B -->|"validate_channels_exist<br/>resolves platform"| C["Validation passes<br/>for custom keys"]
D["Unknown platform<br/>e.g. signal"] -->|"validation check"| E["Validation fails<br/>as expected"]
File Changes1. src/praisonai/praisonai/bots/_config_schema.py
|
Code Review by Qodo
1. Platform remains unset
|
Greptile SummaryThis PR fixes
Confidence Score: 4/5The core channel-key resolution fix is correct and well-tested; the two new schema fields have minor type and validation gaps that are non-breaking but warrant a follow-up. The platform-resolution logic and
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["validate_bot_config(raw)"] --> B["BotYamlSchema(**raw)"]
B --> C["ChannelConfigSchema per key"]
C --> D{"channel.platform set?"}
D -- "Yes" --> E["platform = channel.platform.lower()"]
D -- "No" --> F["platform = key_name.lower()"]
E --> G{"platform in valid_platforms?"}
F --> G
G -- "Yes" --> H["Validation passes ✓"]
G -- "No" --> I["ValueError: Unknown channel '...' (platform '...')"]
Reviews (1): Last reviewed commit: "fix: use monkeypatch for env isolation i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| group_policy: str = "respond_all" # respond_all, mention_only, command_only | ||
| allowlist: List[str] = Field(default_factory=list) | ||
| blocklist: List[str] = Field(default_factory=list) | ||
| allowed_users: Optional[str] = None |
There was a problem hiding this comment.
allowed_users is typed Optional[str], but the field name and the PR description ("channels can now be configured with allowed_users restrictions") both imply a list of identifiers. The existing allowlist: List[str] field on the same model serves a comparable purpose. If a user writes allowed_users: [user1, user2] in their YAML, Pydantic will reject it with a type error because a list cannot be coerced to str. The type should be List[str] with an empty default to match intent and avoid silent single-entry-only behaviour.
| allowed_users: Optional[str] = None | |
| allowed_users: List[str] = Field(default_factory=list) |
|
|
||
| class ChannelConfigSchema(BaseModel): | ||
| """Schema for a single channel configuration.""" | ||
| platform: Optional[str] = None |
There was a problem hiding this comment.
platform field on ChannelConfigSchema accepts any arbitrary string and is only validated later by the model-level validator in BotYamlSchema. A misconfigured standalone ChannelConfigSchema (e.g. constructed directly in tests or other code) will silently accept platform: "signal" without complaint. Adding a field-level validator here would catch errors earlier, regardless of how the schema is instantiated.
| platform: Optional[str] = None | |
| platform: Optional[str] = None | |
| @field_validator("platform") | |
| @classmethod | |
| def validate_platform(cls, v: Optional[str]) -> Optional[str]: | |
| if v is None: | |
| return v | |
| valid = {"telegram", "discord", "slack", "whatsapp", "email", "agentmail"} | |
| if v.lower() not in valid: | |
| raise ValueError( | |
| f"Unknown platform '{v}'. Supported: {', '.join(sorted(valid))}" | |
| ) | |
| return v.lower() |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR updates the bot YAML validation schema to support multi-channel configurations where channel keys are custom identifiers (e.g. telegram_cfo) and the actual platform is specified via a platform: field—matching the praisonai onboard output introduced in #1757. It also adds a regression test to ensure these configs validate successfully.
Changes:
- Extend
ChannelConfigSchemato includeplatform(and accept gateway-style fields likeroutes/allowed_users). - Update channel validation to validate against the resolved platform (
channel.platformwhen present, else the channel key). - Add a unit test covering
telegram_<role>channel keys withplatform: telegram.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/praisonai/praisonai/bots/_config_schema.py |
Accept custom channel keys by resolving/validating platform via platform field; expands per-channel schema fields. |
src/praisonai/tests/unit/test_bot_gap_features.py |
Adds regression test ensuring onboard-style multi-channel YAML validates and env tokens resolve. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| platform: Optional[str] = None | ||
| token: str = "" | ||
| mode: str = "poll" # poll | ws | webhook | hybrid | ||
| group_policy: str = "respond_all" # respond_all, mention_only, command_only | ||
| allowlist: List[str] = Field(default_factory=list) |
| for name, channel in self.channels.items(): | ||
| platform = (channel.platform or name).lower() | ||
| if platform not in valid_platforms: |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| for name, channel in self.channels.items(): | ||
| platform = (channel.platform or name).lower() | ||
| if platform not in valid_platforms: |
There was a problem hiding this comment.
1. Platform remains unset 🐞 Bug ≡ Correctness
validate_channels_exist() derives a platform from (channel.platform or name) for validation but doesn’t persist it back onto the ChannelConfigSchema, so configs that omit platform leave channel.platform == None. praisonai doctor later calls platform.upper() and will crash with AttributeError for these common configs.
Agent Prompt
## Issue description
`BotYamlSchema.validate_channels_exist()` accepts channels whose key is the platform name (e.g., `telegram`) but does not populate `ChannelConfigSchema.platform` when it is omitted. Downstream code (doctor checks) assumes `platform` is a string and calls `.upper()`, which will crash when `platform` is `None`.
## Issue Context
This PR made `platform` optional to support custom channel keys like `telegram_cfo`, but it also needs to preserve backward compatibility for the long-standing `{channels: {telegram: {...}}}` format.
## Fix Focus Areas
- src/praisonai/praisonai/bots/_config_schema.py[93-118]
- src/praisonai/praisonai/bots/_config_schema.py[24-36]
- src/praisonai/praisonai/cli/features/doctor/checks/bot_checks.py[233-286]
## Suggested implementation
- In `validate_channels_exist()`, after computing `platform = (channel.platform or name).lower()`, assign it back to the model (e.g., `channel.platform = platform`) so all channels have a normalized non-null platform.
- Consider normalizing with `.strip().lower()` to avoid whitespace issues.
- Add a defensive fallback in doctor check (`platform = channel_config.platform or channel_name`) to avoid future crashes if `platform` is ever missing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.