Expand end-to-end coverage across Agents surfaces#880
Conversation
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
Co-authored-by: Nick Misasi <[email protected]>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR expands E2E infrastructure with two CI shards, richer agent and System Console helpers, stronger provider and conversation coverage, and new tests for agent access, MCP tools, RHS workflows, interactive questions, search, uploads, and persistence. ChangesShared E2E infrastructure
Agent workflows
Reliability and conversations
RHS and System Console
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (8)
e2e/tests/multiple-bot-conversations/bot-switching.spec.ts (1)
151-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnvalidated cast of unknown response to
AIThreadSummary[].
response as AIThreadSummary[](line 163) trusts the plugin API response shape without checking it. If a field is renamed/removed server-side, this would surface as a confusing downstream assertion failure instead of a clear type error at the source.🤖 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 `@e2e/tests/multiple-bot-conversations/bot-switching.spec.ts` around lines 151 - 178, Replace the unchecked response cast in waitForTitledThreads with runtime validation of each thread object and its required fields, especially title, before assigning to threads. Throw a descriptive error when the API shape is invalid, then use the validated result as AIThreadSummary[] so malformed responses fail at the source.e2e/tests/agent-mention-reminder/loop-in.spec.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFile name uses kebab-case, not snake_case.
Per coding guidelines, TypeScript files should use
snake_case.ts. This new file isloop-in.spec.ts. Note most other e2e spec files/directories in this cohort (agent-mention-reminder,meeting-summary,bot-switching.spec.ts) also use kebab-case, so renaming only this file would create inconsistency with the established e2e naming pattern.As per coding guidelines, "Use
snake_casefor file names (snake_case.go/snake_case.ts(x))."🤖 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 `@e2e/tests/agent-mention-reminder/loop-in.spec.ts` at line 1, Rename the e2e spec file from kebab-case to snake_case as required by the coding guidelines, and update any imports, scripts, or references that point to the existing filename so the test suite continues to discover and run it.Source: Coding guidelines
e2e/tests/rhs-core/context-usage.spec.ts (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the exported
AIMOCK_BOT_NAMEconstant instead of the hardcoded'aimock'string.
e2e/helpers/plugincontainer.tsalready exportsAIMOCK_BOT_NAME = 'aimock', and other specs in this PR (e.g.,file-upload-drag-drop.spec.ts) import and use it. Hardcoding the literal here risks drift if the constant changes.🛠️ Proposed fix
-import {RunAIMockContainer} from 'helpers/plugincontainer'; +import {AIMOCK_BOT_NAME, RunAIMockContainer} from 'helpers/plugincontainer';- const botUser = await client.getUserByUsername('aimock'); + const botUser = await client.getUserByUsername(AIMOCK_BOT_NAME);Also applies to: 59-59
🤖 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 `@e2e/tests/rhs-core/context-usage.spec.ts` at line 20, Replace the hardcoded “aimock” values in context-usage.spec.ts with the exported AIMOCK_BOT_NAME constant, importing it alongside RunAIMockContainer from helpers/plugincontainer.webapp/src/commands.test.ts (1)
18-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the success and thrown-error paths of
handleAskChannelCommand.Only the blank-query validation branch is tested. Consider adding cases that assert
doRunSearch/doSelectPost/showRHSPluginare invoked on a non-blank query, and that the catch block returns{error: {message: 'Failed to process search request'}}whendoRunSearchrejects.🤖 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 `@webapp/src/commands.test.ts` around lines 18 - 38, Add tests in the handleAskChannelCommand suite for a non-blank query that assert doRunSearch, doSelectPost, and showRHSPlugin are invoked with the expected arguments, and add a rejection case where mockedDoRunSearch rejects and the command returns {error: {message: 'Failed to process search request'}}.e2e/tests/channel-summarization/basic-summarization.spec.ts (1)
124-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting this persisted-conversation polling helper to a shared e2e helper.
The same "poll bot DM until a post contains
expectedTextwith a stringconversation_id" pattern is duplicated near-verbatim infindPersistedDMConversationID(e2e/tests/rhs-core/context-usage.spec.ts) andexpectPersistedAndRestored(e2e/tests/rhs-core/new-messages-rhs.spec.ts). Extracting a single parametrized helper intoe2e/helpers/would reduce drift risk if the persistence contract changes.🤖 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 `@e2e/tests/channel-summarization/basic-summarization.spec.ts` around lines 124 - 150, Extract the polling logic from waitForPersistedBotResult into a shared parametrized helper under e2e/helpers/, then update waitForPersistedBotResult, findPersistedDMConversationID, and expectPersistedAndRestored to use it. Preserve the existing bot username, expected text, conversation_id validation, timeout, intervals, and return behavior while removing the duplicated implementations.e2e/tests/interactive-questions/ask-user-question.spec.ts (1)
142-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEscape regex-special characters consistently when building
RegExpfrom labels.
escapeRegExpis already defined and used at lines 161, 163, and 382 for the sameselectedOption/otherOptionvalues, but lines 142 and 375 buildnew RegExp(selectedOption)unescaped. This works today only because the generated labels happen not to contain regex metacharacters; using the same escaping consistently avoids a latent break if that ever changes. The static analysis ReDoS warning itself is a false positive here (input is test-controlled, not user-supplied), but the escaping inconsistency is worth fixing.🛠️ Proposed fix
- await botPost.getByRole('button', {name: new RegExp(selectedOption)}).click(); + await botPost.getByRole('button', {name: new RegExp(escapeRegExp(selectedOption))}).click();- await requesterBotPost.getByRole('button', {name: new RegExp(selectedOption)}).click(); + await requesterBotPost.getByRole('button', {name: new RegExp(escapeRegExp(selectedOption))}).click();Also applies to: 375-375
🤖 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 `@e2e/tests/interactive-questions/ask-user-question.spec.ts` at line 142, Escape regex-special characters consistently when constructing label matchers: update the `getByRole` calls around the `selectedOption` usages, including both the reported line and the corresponding occurrence near line 375, to pass `escapeRegExp(selectedOption)` to `new RegExp`. Preserve the existing `escapeRegExp` helper and matching behavior.Source: Linters/SAST tools
e2e/helpers/mm.ts (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded team slug
testin a shared login helper.
confirmLoginNavigationrequires the post-login URL to contain the literal segment/test/channels/. Every container in the current codebase provisions a team namedtest, so this works today, but it silently assumes every future caller does the same. If a container ever uses a different team name, this shared helper (used across the whole e2e suite) would misreport a generic "auth failure" or time out instead of clearly indicating a team-name mismatch.Consider generalizing the match to any team segment.
♻️ Proposed generalization
- await this.page.waitForURL(/.*\/test\/channels\/.*/, { timeout: this.remainingMs(deadline) }); + await this.page.waitForURL(/\/channels\//, { timeout: this.remainingMs(deadline) });Also applies to: 105-122
🤖 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 `@e2e/helpers/mm.ts` around lines 89 - 93, Generalize confirmLoginNavigation so it does not require the hardcoded /test/channels/ URL segment; match any valid team-name segment before /channels/ while preserving the existing navigation and timeout behavior. Update the related URL validation logic around confirmLoginNavigation and its callers so different container team names are accepted without weakening the channel route check.e2e/helpers/openai-mock.ts (1)
372-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
titlePromptand reuse it in the specs.
This keeps the title-prompt text in one place and avoids drift betweene2e/helpers/openai-mock.ts,e2e/tests/advanced-error-scenarios/network-errors.spec.ts, ande2e/tests/multiple-bot-conversations/bot-switching.spec.ts.🤖 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 `@e2e/helpers/openai-mock.ts` around lines 372 - 381, Export the existing titlePrompt constant from the openai mock helper, then update the title-related specs to import and reuse it instead of duplicating the prompt text. Ensure buildTitleMockRule continues referencing the exported constant so all title prompt matching stays centralized.
🤖 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 `@e2e/helpers/openai-mock.ts`:
- Around line 117-151: Update e2e/tsconfig.json to explicitly set target and lib
to ES2022 or newer, ensuring ErrorOptions and the cause property used in
getHistory are supported during type-checking.
---
Nitpick comments:
In `@e2e/helpers/mm.ts`:
- Around line 89-93: Generalize confirmLoginNavigation so it does not require
the hardcoded /test/channels/ URL segment; match any valid team-name segment
before /channels/ while preserving the existing navigation and timeout behavior.
Update the related URL validation logic around confirmLoginNavigation and its
callers so different container team names are accepted without weakening the
channel route check.
In `@e2e/helpers/openai-mock.ts`:
- Around line 372-381: Export the existing titlePrompt constant from the openai
mock helper, then update the title-related specs to import and reuse it instead
of duplicating the prompt text. Ensure buildTitleMockRule continues referencing
the exported constant so all title prompt matching stays centralized.
In `@e2e/tests/agent-mention-reminder/loop-in.spec.ts`:
- Line 1: Rename the e2e spec file from kebab-case to snake_case as required by
the coding guidelines, and update any imports, scripts, or references that point
to the existing filename so the test suite continues to discover and run it.
In `@e2e/tests/channel-summarization/basic-summarization.spec.ts`:
- Around line 124-150: Extract the polling logic from waitForPersistedBotResult
into a shared parametrized helper under e2e/helpers/, then update
waitForPersistedBotResult, findPersistedDMConversationID, and
expectPersistedAndRestored to use it. Preserve the existing bot username,
expected text, conversation_id validation, timeout, intervals, and return
behavior while removing the duplicated implementations.
In `@e2e/tests/interactive-questions/ask-user-question.spec.ts`:
- Line 142: Escape regex-special characters consistently when constructing label
matchers: update the `getByRole` calls around the `selectedOption` usages,
including both the reported line and the corresponding occurrence near line 375,
to pass `escapeRegExp(selectedOption)` to `new RegExp`. Preserve the existing
`escapeRegExp` helper and matching behavior.
In `@e2e/tests/multiple-bot-conversations/bot-switching.spec.ts`:
- Around line 151-178: Replace the unchecked response cast in
waitForTitledThreads with runtime validation of each thread object and its
required fields, especially title, before assigning to threads. Throw a
descriptive error when the API shape is invalid, then use the validated result
as AIThreadSummary[] so malformed responses fail at the source.
In `@e2e/tests/rhs-core/context-usage.spec.ts`:
- Line 20: Replace the hardcoded “aimock” values in context-usage.spec.ts with
the exported AIMOCK_BOT_NAME constant, importing it alongside RunAIMockContainer
from helpers/plugincontainer.
In `@webapp/src/commands.test.ts`:
- Around line 18-38: Add tests in the handleAskChannelCommand suite for a
non-blank query that assert doRunSearch, doSelectPost, and showRHSPlugin are
invoked with the expected arguments, and add a rejection case where
mockedDoRunSearch rejects and the command returns {error: {message: 'Failed to
process search request'}}.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ccfc6443-6436-4fee-9684-749a8e84234f
📒 Files selected for processing (41)
.github/workflows/ci.ymle2e/helpers/agent-api.tse2e/helpers/agent-page.tse2e/helpers/agent_preferences.tse2e/helpers/aimock-citation-harness.tse2e/helpers/mm.tse2e/helpers/openai-mock.tse2e/helpers/system-console-container.tse2e/helpers/system-console.tse2e/scripts/ci-test-groups.mjse2e/tests/advanced-error-scenarios/network-errors.spec.tse2e/tests/agent-mention-reminder/loop-in.spec.tse2e/tests/agents/access-control.spec.tse2e/tests/agents/configuration-details.spec.tse2e/tests/agents/crud.spec.tse2e/tests/agents/mcp-tools.spec.tse2e/tests/agents/provider-config.spec.tse2e/tests/channel-summarization/basic-summarization.spec.tse2e/tests/interactive-questions/ask-user-question.spec.tse2e/tests/llmbot-post-component/edge-cases.spec.tse2e/tests/login-helper/channel-view-recovery.spec.tse2e/tests/meeting-summary/summary-persistence.spec.tse2e/tests/multiple-bot-conversations/bot-switching.spec.tse2e/tests/rhs-core/basic.spec.tse2e/tests/rhs-core/context-usage.spec.tse2e/tests/rhs-core/file-upload-drag-drop.spec.tse2e/tests/rhs-core/new-messages-rhs.spec.tse2e/tests/system-console/mcp-panel.spec.tse2e/tests/system-console/search-and-observability.spec.tswebapp/src/commands.test.tswebapp/src/components/agents/agent_config_view.test.tsxwebapp/src/components/agents/agent_row.tsxwebapp/src/components/question_card.test.tsxwebapp/src/components/question_card.tsxwebapp/src/components/rhs/rhs.tsxwebapp/src/components/rhs/thread_item.test.tsxwebapp/src/components/rhs/thread_item.tsxwebapp/src/components/system_console/config.tsxwebapp/src/components/system_console/embedding_search/embedding_search_panel.tsxwebapp/src/components/system_console/panel.tsxwebapp/src/components/system_console/web_search/web_search_panel.tsx
💤 Files with no reviewable changes (1)
- e2e/tests/llmbot-post-component/edge-cases.spec.ts
Co-authored-by: nick.misasi <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4cc32df40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function currentUserMessagePattern(message: string): string { | ||
| return `(?s)"role"\\s*:\\s*"user"\\s*,\\s*"content"\\s*:\\s*"${escapeRegExp(message)}"\\s*}\\s*]\\s*}`; |
There was a problem hiding this comment.
The matcher ends with ]\s*}, so it only accepts requests where messages is the final top-level JSON property. Normal streaming chat-completions payloads include fields such as stream after messages, causing each Smocker rule that uses currentUserMessagePattern in this new spec to miss the provider request and fail the shard instead of returning the expected agent reply. Match the target user message without anchoring it to the end of the request object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This claim doesn't hold for this stack. The plugin sends provider requests through Bifrost (github.com/maximhq/bifrost/core v1.5.18), whose OpenAI provider uses a custom MarshalJSON that shadows the messages field so it serializes as the last top-level property — stream, stream_options, model, etc. all come before messages. Verified empirically against the pinned version:
{"model":"gpt-4o","max_completion_tokens":8192,"stream_options":{"include_usage":true},"temperature":1,"stream":true,"messages":[...,{"role":"user","content":"hello world"}]}CI confirms this: all three tests in this spec passed in e2e-shard-6 on the current head (run). Each rule uses times: 1 with bodyMatches, so a matcher miss would time out the test — passing tests prove the pattern matches real provider requests.
The end-of-body anchor is also intentional: it ensures the rule matches only when the given message is the current (final) user message, not an earlier occurrence in the thread history of a follow-up request.
2 participants
Summary
Expands deterministic Playwright coverage across previously untested Agents surfaces:
AskUserQuestioncards, requester/onlooker privacy, answers, skips, and persistenceEach new or materially rewritten spec received a delegated
/test-quality-reviewerreview; all actionable findings were fixed and the affected tests rerun before moving on.CI E2E execution expands from four to six shards. Measured final local Chromium shard times are balanced from 8.3–9.6 minutes:
Validation:
GOTOOLCHAIN=go1.26.4 make checkTicket Link
NONE
Screenshots
N/A — test coverage and test-facing accessibility/stable locator improvements only.
Release Note
Summary by CodeRabbit
New Features
Bug Fixes
Tests / Chores