[codex] Fix Reborn WebUI v2 Slack connect canary - #5485
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Slack connect QA case in the Live QA runner was rewritten to validate the extensions "connectable channels" page and API instead of the chat composer flow, checking for the Slack inbound_proof_code strategy UI fields. A corresponding unit test was added to verify the new navigation and detail assertions. ChangesSlack Connect Extensions Channels QA
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Review flags (Rust invariants n/a): This diff touches Python QA scripts only; no Rust sandbox/trust/secrets/egress/migration surfaces are implicated. No CLAUDE.md/AGENTS.md rule violations observed in scope. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
size: M
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 89-227: Add a negative-path test for `_slack_connect_case` to
cover the new failure branch when the connectable channels API does not return
an `inbound_proof_code` Slack entry. Reuse the existing test scaffolding in
`test_slack_connect_case_uses_extensions_channels_surface` with `FakePage`,
`fake_fetch_webui_json`, and `LiveQaContext`, but have the fake API response
omit that strategy and assert the case returns `success=False` with the expected
missing-entry AssertionError in `result.details`.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: df33982c-de2b-449c-b441-b5a4781cdb7e
📒 Files selected for processing (2)
scripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
| def test_slack_connect_case_uses_extensions_channels_surface(self): | ||
| class FakePage: | ||
| def __init__(self) -> None: | ||
| self.gotos: list[tuple[str, str | None]] = [] | ||
|
|
||
| async def goto(self, url: str, wait_until: str | None = None) -> None: | ||
| self.gotos.append((url, wait_until)) | ||
|
|
||
| def locator(self, selector: str) -> str: | ||
| return selector | ||
|
|
||
| class FakeExpectation: | ||
| def __init__(self, selector: str) -> None: | ||
| self.selector = selector | ||
|
|
||
| async def to_contain_text( | ||
| self, | ||
| text: str, | ||
| timeout: int | None = None, | ||
| ) -> None: | ||
| expected_texts.append((self.selector, text, timeout)) | ||
|
|
||
| def fake_expect(selector: str) -> FakeExpectation: | ||
| return FakeExpectation(selector) | ||
|
|
||
| async def fake_with_page( | ||
| _output_dir: Path, | ||
| _case_name: str, | ||
| action, | ||
| ) -> None: | ||
| await action(fake_page) | ||
|
|
||
| async def fake_fetch_webui_json(_page: object, path: str) -> dict[str, object]: | ||
| fetched_paths.append(path) | ||
| return { | ||
| "channels": [ | ||
| { | ||
| "channel": "slack", | ||
| "display_name": "Slack", | ||
| "strategy": "admin_managed_channels", | ||
| "action": {"title": "Choose Slack channel"}, | ||
| }, | ||
| { | ||
| "channel": "slack", | ||
| "display_name": "Slack", | ||
| "strategy": "inbound_proof_code", | ||
| "action": { | ||
| "title": "Slack account connection", | ||
| "instructions": "Message the Slack app, then enter the code here.", | ||
| }, | ||
| }, | ||
| ] | ||
| } | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| output_dir = Path(tmpdir) | ||
| (output_dir / "preflight.json").write_text( | ||
| json.dumps( | ||
| { | ||
| "checks": { | ||
| "slack": { | ||
| "enabled_in_config": True, | ||
| "env_present": True, | ||
| "auth_test": { | ||
| "ok": True, | ||
| "team_id": "T123", | ||
| "user_id": "U123", | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| ), | ||
| encoding="utf-8", | ||
| ) | ||
| fake_page = FakePage() | ||
| fetched_paths: list[str] = [] | ||
| expected_texts: list[tuple[str, str, int | None]] = [] | ||
| playwright_module = types.ModuleType("playwright") | ||
| playwright_async_api = types.ModuleType("playwright.async_api") | ||
| playwright_async_api.expect = fake_expect | ||
| ctx = run_live_qa.LiveQaContext( | ||
| base_url="http://127.0.0.1:3000", | ||
| output_dir=output_dir, | ||
| reborn_home=output_dir / "reborn-home", | ||
| env={}, | ||
| ) | ||
|
|
||
| with ( | ||
| patch.dict( | ||
| sys.modules, | ||
| { | ||
| "playwright": playwright_module, | ||
| "playwright.async_api": playwright_async_api, | ||
| }, | ||
| ), | ||
| patch.object(run_live_qa, "_with_page", new=fake_with_page), | ||
| patch.object( | ||
| run_live_qa, | ||
| "_fetch_webui_json", | ||
| new=fake_fetch_webui_json, | ||
| ), | ||
| ): | ||
| result = asyncio.run( | ||
| run_live_qa._slack_connect_case( | ||
| ctx, | ||
| case_name="qa_3a_slack_connect", | ||
| ) | ||
| ) | ||
|
|
||
| self.assertTrue(result.success, result.details) | ||
| self.assertEqual( | ||
| fake_page.gotos, | ||
| [ | ||
| ( | ||
| "http://127.0.0.1:3000/v2/extensions/channels?" | ||
| f"token={run_live_qa.AUTH_TOKEN}", | ||
| "domcontentloaded", | ||
| ) | ||
| ], | ||
| ) | ||
| self.assertEqual( | ||
| fetched_paths, | ||
| ["/api/webchat/v2/channels/connectable"], | ||
| ) | ||
| observed_expectations = [text for _selector, text, _timeout in expected_texts] | ||
| self.assertIn("Channels", observed_expectations) | ||
| self.assertIn("Slack account connection", observed_expectations) | ||
| self.assertIn("Message the Slack app", observed_expectations) | ||
| self.assertNotIn("Connect Slack", observed_expectations) | ||
| self.assertFalse(any("/v2/chat" in url for url, _wait in fake_page.gotos)) | ||
| self.assertEqual( | ||
| result.details["slack_connect_surface"], | ||
| "/v2/extensions/channels", | ||
| ) | ||
| self.assertEqual( | ||
| result.details["slack_connect_title"], | ||
| "Slack account connection", | ||
| ) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
LGTM! Solid regression coverage for the extensions-channels rewrite; the assertNotIn("Connect Slack", ...) check directly guards against reverting to the removed chat flow.
Optional: a companion case where the fake API omits the inbound_proof_code strategy (verifying _slack_connect_case returns success=False with the "missing" AssertionError) would round out coverage for the new failure branch, but not blocking.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 145-159: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"checks": {
"slack": {
"enabled_in_config": True,
"env_present": True,
"auth_test": {
"ok": True,
"team_id": "T123",
"user_id": "U123",
},
}
}
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 169-169: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://127.0.0.1:3000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
🤖 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 `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py` around lines 89 - 227,
Add a negative-path test for `_slack_connect_case` to cover the new failure
branch when the connectable channels API does not return an `inbound_proof_code`
Slack entry. Reuse the existing test scaffolding in
`test_slack_connect_case_uses_extensions_channels_surface` with `FakePage`,
`fake_fetch_webui_json`, and `LiveQaContext`, but have the fake API response
omit that strategy and assert the case returns `success=False` with the expected
missing-entry AssertionError in `result.details`.
|
🚅 Deployed to the ironclaw-pr-5485 environment in ironclaw-ci-preview
|
This branch was successfully deployed
1 participant
Summary
/api/webchat/v2/channels/connectableexposes Slack inbound proof-code metadataConnect Slackcard flowRoot cause
The canary still drove the old chat-triggered Slack connect prompt and waited for
Connect Slack. WebUI v2 now exposes Slack connect through Extensions > Channels, so the scheduled canary could send the prompt to the model and fail waiting for the obsolete card.Validation
python3 -m py_compile scripts/reborn_webui_v2_live_qa/run_live_qa.py scripts/reborn_webui_v2_live_qa/test_run_live_qa.pypython3 scripts/reborn_webui_v2_live_qa/test_run_live_qa.py