fix(ci): stabilize Slack live QA harness - #5632
Conversation
github-actions
Bot
added
size: M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtension search guidance now points callers toward explicit connect and outbound delivery targets. Live QA Slack cases now use pairing-code wording, DM targets, and routine-confirmation follow-ups, with updated validation and setup. GitHub latest-release requests can now send a bearer token from configured environment variables. ChangesExtension search guidance changes
Live QA Slack and GitHub request changes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the live QA script and its tests to support a pre-configured Slack DM delivery target and ingress, adjusting several QA prompts to instruct the model not to authenticate or install Slack. It also introduces a helper function _slack_connect_instructions_look_valid to handle both old and new Slack connection instructions. However, a potential issue was identified where the assertion on the page content strictly expects 'pairing code', which will fail if the environment still serves the older instructions. A regex-based assertion is suggested to maintain backward compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| raise AssertionError(f"unexpected Slack connect instructions: {instructions!r}") | ||
| await expect(page.locator("body")).to_contain_text(title, timeout=15000) # type: ignore[attr-defined] | ||
| await expect(page.locator("body")).to_contain_text("Message the Slack app", timeout=15000) # type: ignore[attr-defined] | ||
| await expect(page.locator("body")).to_contain_text("pairing code", timeout=15000) # type: ignore[attr-defined] |
There was a problem hiding this comment.
The helper _slack_connect_instructions_look_valid was introduced to support both the old instructions ("Message the Slack app...") and the new instructions ("Message the IronClaw Reborn app in Slack to get a pairing code..."). However, the strict assertion await expect(page.locator("body")).to_contain_text("pairing code", timeout=15000) will fail if the live environment still serves the old instructions, as they do not contain the substring "pairing code".
To ensure the stabilization is robust and backward-compatible as intended, use a regular expression that matches either instruction variant.
| await expect(page.locator("body")).to_contain_text("pairing code", timeout=15000) # type: ignore[attr-defined] | |
| await expect(page.locator("body")).to_contain_text(re.compile(r"Message the Slack app|pairing code"), timeout=15000) # type: ignore[attr-defined] |
![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/run_live_qa.py`:
- Around line 1932-1934: The Slack instruction validator in
_slack_connect_instructions_look_valid accepts two shapes, but the later
pairing-code text check is always applied and may fail for the “message the
slack app” branch. Update the logic around this helper so the
to_contain_text("pairing code") assertion only runs when the Slack/pairing-code
branch matched, or make that assertion case-insensitive with ignore_case=True.
Use the existing _slack_connect_instructions_look_valid and to_contain_text call
sites to keep the branching consistent.
🪄 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: ad1c2e04-8d14-44cc-9e6d-26e9f99e157a
📒 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 _slack_connect_instructions_look_valid(instructions: str) -> bool: | ||
| text = instructions.lower() | ||
| return "message the slack app" in text or ("slack" in text and "pairing code" in text) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== run_live_qa.py slice ==\n'
sed -n '1910,2005p' scripts/reborn_webui_v2_live_qa/run_live_qa.py
printf '\n== search to_contain_text usages ==\n'
rg -n "to_contain_text\(" scripts/reborn_webui_v2_live_qa/run_live_qa.py scripts -g '!**/node_modules/**' || true
printf '\n== relevant repo guidance ==\n'
rg -n "Playwright|to_contain_text|ignoreCase|ignore_case|pairing code|slack connect" -g 'CLAUDE.md' -g 'AGENTS.md' -g '.claude/**' -g '*.md' .Repository: nearai/ironclaw
Length of output: 18420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Playwright docs quick check ==\n'
python3 - <<'PY'
import urllib.request
url = "https://playwright.dev/python/docs/api/class-locatorassertions#locator-assertions-to-contain-text"
with urllib.request.urlopen(url, timeout=20) as r:
text = r.read().decode("utf-8", "ignore")
for needle in ["ignore_case", "ignoreCase", "Case-insensitive"]:
idx = text.find(needle)
print(f"{needle}: {idx}")
if idx != -1:
print(text[max(0, idx-200): idx+500])
PYRepository: nearai/ironclaw
Length of output: 2782
🌐 Web query:
Playwright Python expect(locator).to_contain_text default case sensitivity ignore_case documentation
💡 Result:
In Playwright Python, the expect(locator).to_contain_text() assertion performs a case-sensitive match by default when using a string [1][2]. To perform a case-insensitive match, you must explicitly set the ignore_case option to True [1][2]: python from playwright.sync_api import expect # Case-insensitive assertion expect(locator).to_contain_text("Expected Text", ignore_case=True) The ignore_case option was added in version 1.23 [1][2]. When provided, it takes precedence over the corresponding flags in regular expressions if one is passed instead of a string [1][2]. Note that this behavior is specific to the expect(locator).to_contain_text() assertion [1]. Other Playwright features, such as page.get_by_text() or the :has-text() selector, have different default behaviors regarding case sensitivity [3][4].
Citations:
- 1: https://playwright.dev/python/docs/next/api/class-locatorassertions
- 2: https://github.com/microsoft/playwright/blob/main/docs/src/api/class-locatorassertions.md
- 3: https://playwright.dev/python/docs/other-locators
- 4: https://playwright.dev/docs/api/class-locator
Align the Slack body check with the accepted instruction shapes. The helper allows either "message the slack app" or "slack" + "pairing code", but the later to_contain_text("pairing code") runs unconditionally and is case-sensitive by default. Gate that assertion on the matched branch or use ignore_case=True.
🤖 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/run_live_qa.py` around lines 1932 - 1934, The
Slack instruction validator in _slack_connect_instructions_look_valid accepts
two shapes, but the later pairing-code text check is always applied and may fail
for the “message the slack app” branch. Update the logic around this helper so
the to_contain_text("pairing code") assertion only runs when the
Slack/pairing-code branch matched, or make that assertion case-insensitive with
ignore_case=True. Use the existing _slack_connect_instructions_look_valid and
to_contain_text call sites to keep the branching consistent.
Reborn integration-tier coverageLine coverage (Reborn crates): 17.19% — 11063 / 64362 lines Per-crate breakdown (11 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. |
|
🚅 Deployed to the ironclaw-pr-5632 environment in ironclaw-ci-preview
|
IronLoop Review StatusHead: Current reviewers:
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py (1)
277-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the legacy Slack copy in
_slack_connect_case
test_slack_connect_instruction_validation_accepts_pairing_copyonly hits_slack_connect_instructions_look_valid; add the oldMessage the Slack app, then enter the code here.path through_slack_connect_casetoo so the unconditionalbodyassertion atrun_live_qa.py:2000is exercised for both instruction variants.🤖 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 277 - 294, The Slack connect test only validates _slack_connect_instructions_look_valid, but it should also exercise _slack_connect_case for the legacy “Message the Slack app, then enter the code here.” copy so the unconditional body assertion is covered. Update test_slack_connect_instruction_validation_accepts_pairing_copy to drive both instruction variants through _slack_connect_case in run_live_qa.py, alongside the existing validation checks, using the _slack_connect_case and _slack_connect_instructions_look_valid symbols to locate the flow.
♻️ Duplicate comments (1)
scripts/reborn_webui_v2_live_qa/run_live_qa.py (1)
1943-2003: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnconditional "pairing code" body assertion still not gated on the matched branch.
_slack_connect_instructions_look_validaccepts either"message the slack app"or"slack"+"pairing code", but Line 2000 asserts the body always contains"pairing code"regardless of which branch matched. If production instructions ever return the legacy "Message the Slack app" copy (no "pairing code" substring), this check fails even though the instructions validator passed. This is the same defect flagged in a prior review round on this helper and was not resolved.♻️ Proposed fix: gate the assertion on the matched branch
- instructions = str(action_body.get("instructions") or "") - if not _slack_connect_instructions_look_valid(instructions): - raise AssertionError(f"unexpected Slack connect instructions: {instructions!r}") - await expect(page.locator("body")).to_contain_text(title, timeout=15000) # type: ignore[attr-defined] - await expect(page.locator("body")).to_contain_text("pairing code", timeout=15000) # type: ignore[attr-defined] + instructions = str(action_body.get("instructions") or "") + if not _slack_connect_instructions_look_valid(instructions): + raise AssertionError(f"unexpected Slack connect instructions: {instructions!r}") + await expect(page.locator("body")).to_contain_text(title, timeout=15000) # type: ignore[attr-defined] + if "pairing code" in instructions.lower(): + await expect(page.locator("body")).to_contain_text( + "pairing code", timeout=15000 + ) # type: ignore[attr-defined]🤖 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/run_live_qa.py` around lines 1943 - 2003, The Slack connect flow in _slack_connect_case still unconditionally checks for “pairing code” even when _slack_connect_instructions_look_valid matches the legacy “message the slack app” branch. Update the assertion logic around the body text checks so it follows the same branch used by the validator, and only require “pairing code” when the instructions actually contain the newer Slack pairing-code copy. Keep the fix localized to _slack_connect_case and the helper _slack_connect_instructions_look_valid.
🤖 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.
Outside diff comments:
In `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 277-294: The Slack connect test only validates
_slack_connect_instructions_look_valid, but it should also exercise
_slack_connect_case for the legacy “Message the Slack app, then enter the code
here.” copy so the unconditional body assertion is covered. Update
test_slack_connect_instruction_validation_accepts_pairing_copy to drive both
instruction variants through _slack_connect_case in run_live_qa.py, alongside
the existing validation checks, using the _slack_connect_case and
_slack_connect_instructions_look_valid symbols to locate the flow.
---
Duplicate comments:
In `@scripts/reborn_webui_v2_live_qa/run_live_qa.py`:
- Around line 1943-2003: The Slack connect flow in _slack_connect_case still
unconditionally checks for “pairing code” even when
_slack_connect_instructions_look_valid matches the legacy “message the slack
app” branch. Update the assertion logic around the body text checks so it
follows the same branch used by the validator, and only require “pairing code”
when the instructions actually contain the newer Slack pairing-code copy. Keep
the fix localized to _slack_connect_case and the helper
_slack_connect_instructions_look_valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 288ae9b5-5ec6-4cff-8ff8-7142d2772ba0
📒 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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py (1)
135-276: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't cover the legacy "message the slack app" instructions against the hard "pairing code" body assertion.
The mocked payload always uses pairing-code copy, so this test can't catch the case where
_slack_connect_instructions_look_validaccepts legacy copy but the unconditionalto_contain_text("pairing code")inrun_live_qa.pywould fail for it. Root cause tracked in the correspondingrun_live_qa.pycomment.🤖 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 135 - 276, The Slack connect test only uses pairing-code copy, so it never exercises the legacy “message the Slack app” instructions path that the UI validation still needs to handle. Update the mocked payload in test_slack_connect_case_uses_extensions_channels_surface to include a legacy instructions variant alongside the current pairing-code variant, and assert behavior for both so the test covers _slack_connect_instructions_look_valid as well as the unconditional pairing code expectation in _slack_connect_case. Keep the existing checks on fetched_paths, expected_texts, and result.details, but add coverage that would fail if legacy copy is treated as valid without the “pairing code” body text.
🤖 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.
Outside diff comments:
In `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 135-276: The Slack connect test only uses pairing-code copy, so it
never exercises the legacy “message the Slack app” instructions path that the UI
validation still needs to handle. Update the mocked payload in
test_slack_connect_case_uses_extensions_channels_surface to include a legacy
instructions variant alongside the current pairing-code variant, and assert
behavior for both so the test covers _slack_connect_instructions_look_valid as
well as the unconditional pairing code expectation in _slack_connect_case. Keep
the existing checks on fetched_paths, expected_texts, and result.details, but
add coverage that would fail if legacy copy is treated as valid without the
“pairing code” body text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8430627a-6c98-4a49-99c2-13a66c4809a8
📒 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
github-actions
Bot
added
size: L
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 `@crates/ironclaw_reborn_composition/src/extension_lifecycle.rs`:
- Around line 224-227: The guidance text in search() is duplicated with the
builtin.extension_search manifest description, so the two hand-maintained
strings can drift. Extract the shared “prefer the configured outbound delivery
target over activating” wording into a shared const/helper and have
extension_lifecycle.rs and extension_lifecycle_capabilities.rs compose their
messages from that single source of truth. Update the related tests to assert
against the shared text via the symbols search() and builtin.extension_search so
both call sites stay aligned.
🪄 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: 49db9186-8ebb-4186-9e95-ae3d4c1b938b
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rsscripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
| response.message = Some( | ||
| "Search found installed external channel results. Search cannot prove the calling user's channel account is personally connected, so do not treat those results as ready for delivery or message access. Call builtin.extension_activate now for the matching extension id; activation surfaces the channel-specific pairing/setup instructions and, for proof-code flows, the user must paste the code into the WebChat connection panel rather than normal chat." | ||
| "Search found installed external channel results. Search cannot prove the calling user's channel account is personally connected. For an explicit connect, pair, authenticate, or account-access request, call builtin.extension_activate for the matching extension id so channel-specific pairing/setup instructions can be surfaced. For routine, trigger, or notification delivery, prefer the configured outbound delivery target when one is available; do not activate the channel just to send to an already configured delivery target." | ||
| .to_string(), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Guidance text duplicated by hand across two files.
This search() message and the builtin.extension_search manifest description in extension_lifecycle_capabilities.rs (line 71) both encode the same "prefer the configured outbound delivery target over activating" rule in independently-worded, hand-maintained strings, only cross-checked via substring assertions in each file's tests. A future edit to one could silently drift from the other since nothing enforces they agree.
Consider extracting the shared clause(s) into a const/helper so both call sites compose from one source of truth.
🤖 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 `@crates/ironclaw_reborn_composition/src/extension_lifecycle.rs` around lines
224 - 227, The guidance text in search() is duplicated with the
builtin.extension_search manifest description, so the two hand-maintained
strings can drift. Extract the shared “prefer the configured outbound delivery
target over activating” wording into a shared const/helper and have
extension_lifecycle.rs and extension_lifecycle_capabilities.rs compose their
messages from that single source of truth. Update the related tests to assert
against the shared text via the symbols search() and builtin.extension_search so
both call sites stay aligned.
This branch was successfully deployed
1 participant
Summary
Verification
Context: main live canary run https://github.com/nearai/ironclaw/actions/runs/28700688649 showed Google fixed and remaining Reborn WebUI v2 failures isolated to Slack pairing/harness behavior.