feat(connectors): add admin-managed app runtime - #2068
Conversation
# Conflicts: # wework/scripts/ai-verify-environment.test.mjs
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
📝 WalkthroughWalkthroughChangesConnector Apps and Runtime
Verification environment isolation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant Backend
participant CloudClient
participant LocalExecutor
participant UpstreamConnector
Admin->>Backend: create or update connector app
CloudClient->>Backend: request scoped connector token
CloudClient->>LocalExecutor: configure connector gateway
CloudClient->>Backend: list connected connector apps
CloudClient->>LocalExecutor: synchronize connector skills
LocalExecutor->>Backend: request tools or invoke tool
Backend->>UpstreamConnector: discover or execute connector tool
UpstreamConnector-->>Backend: return tool result
Backend-->>LocalExecutor: return normalized result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
wework/scripts/ai-verify-environment.test.mjs (2)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
WEGENT_EXECUTOR_APP_IPC_ADDRto test coverage.The constant
INHERITED_EXECUTOR_ENV_KEYSin the source file includesWEGENT_EXECUTOR_APP_IPC_ADDR, but it is currently omitted from the test's input environment. Consider adding it to ensure complete coverage of the filtered keys.♻️ Proposed fix
{ PATH: '/usr/bin', + WEGENT_EXECUTOR_APP_IPC_ADDR: '127.0.0.1:8080', WEGENT_EXECUTOR_APP_IPC_ADDR_FILE: '/tmp/foreign.addr', WEGENT_EXECUTOR_APP_IPC_SOCKET: '/tmp/legacy.sock', WEGENT_EXECUTOR_BINARY: '/tmp/foreign-executor',🤖 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 `@wework/scripts/ai-verify-environment.test.mjs` around lines 7 - 15, Update the test input environment in the relevant ai-verify-environment test to include WEGENT_EXECUTOR_APP_IPC_ADDR alongside the other inherited executor environment keys, ensuring the filtering behavior covers every key listed by INHERITED_EXECUTOR_ENV_KEYS.
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that
WEGENT_EXECUTOR_APP_IPC_ADDRis isolated.Include the assertion for the missing key to complete the verification.
♻️ Proposed fix
expect(environment.WEWORK_EXECUTOR_ISOLATION_OVERRIDE).toBe('true') + expect(environment.WEGENT_EXECUTOR_APP_IPC_ADDR).toBeUndefined() expect(environment.WEGENT_EXECUTOR_APP_IPC_ADDR_FILE).toBeUndefined() expect(environment.WEGENT_EXECUTOR_APP_IPC_SOCKET).toBeUndefined() expect(environment.WEGENT_EXECUTOR_BINARY).toBeUndefined()🤖 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 `@wework/scripts/ai-verify-environment.test.mjs` around lines 37 - 42, Add an expectation in the environment isolation test alongside the existing WEGENT_EXECUTOR_* assertions to verify that environment.WEGENT_EXECUTOR_APP_IPC_ADDR is undefined, preserving the test’s missing-key verification pattern.wework/src/features/cloud-connection/LocalExecutorCloudBridge.test.tsx (2)
111-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the assertion payload.
This assertion should be updated to match the inclusion of
descriptionin the sync payload.🛠️ Proposed fix
- expect(mocks.request).toHaveBeenCalledWith('runtime.connectors.apps.sync', { - apps: [{ slug: 'tickets', name: 'Tickets' }], - }) + expect(mocks.request).toHaveBeenCalledWith('runtime.connectors.apps.sync', { + apps: [{ slug: 'tickets', name: 'Tickets', description: 'Manage tickets' }], + })🤖 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 `@wework/src/features/cloud-connection/LocalExecutorCloudBridge.test.tsx` around lines 111 - 113, Update the assertion in the LocalExecutorCloudBridge test for the runtime.connectors.apps.sync request to include the expected description field in the tickets app payload, while preserving the existing slug and name values.
43-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
descriptionto the test mock payload.If you update
LocalExecutorCloudBridgeto include thedescriptionfield in the payload, you should also update this mock data and the corresponding test assertion.🛠️ Proposed fix
mocks.listApps.mockResolvedValue([ { id: 1, slug: 'tickets', name: 'Tickets', + description: 'Manage tickets', connection: { status: 'connected' }, },🤖 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 `@wework/src/features/cloud-connection/LocalExecutorCloudBridge.test.tsx` around lines 43 - 49, Update the listApps mock in the LocalExecutorCloudBridge test to include the description field expected by the revised payload, and update the corresponding assertion to verify that description is propagated correctly.backend/app/services/connector_runtime.py (1)
449-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting
_refresh_oauth_if_neededinto smaller helpers.The method spans ~90 lines and mixes locking/double-check, HTTP token exchange, response validation, and persistence. Extracting the token-request and token-application steps would improve readability. The double-checked
with_for_update()locking itself is a solid concurrency choice.As per coding guidelines: "Keep functions focused, preferably under 50 lines."
🤖 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 `@backend/app/services/connector_runtime.py` around lines 449 - 540, Split _refresh_oauth_if_needed into focused helpers while preserving its existing double-checked with_for_update locking and refresh behavior. Extract the OAuth token request/error handling into one helper and token validation/application/persistence into another, leaving _refresh_oauth_if_needed responsible for eligibility checks, locking, and orchestration; keep each function under roughly 50 lines.Source: Coding guidelines
🤖 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 `@backend/alembic/versions/20260720_f8a9b0c1d2e3_add_connector_http_tools.py`:
- Around line 21-24: Update the http_tools column definition in the migration to
use a MySQL-compatible JSON default expression or backfill existing rows before
enforcing non-nullability, replacing the bare server_default string while
preserving the empty-array default for new records.
In `@backend/app/core/config.py`:
- Around line 227-229: Validate CONNECTOR_OAUTH_CALLBACK_BASE_URL as an absolute
HTTP(S) URL without query or fragment components before using it for OAuth
redirect construction. Require HTTPS unless running in the existing
local/development environment, and reject invalid values through the
configuration validation path.
In `@backend/app/schemas/connector.py`:
- Around line 84-114: Update
ConnectorHttpToolDefinition.validate_argument_locations so the set of path
placeholders in self.path must exactly match the names in argument_locations
whose location is "path". Reject both placeholders missing a corresponding path
argument and path arguments missing placeholders, while preserving the existing
schema-property and required-property validation.
In `@executor/src/connector_gateway.rs`:
- Around line 44-93: Update the timeout configured in ConnectorGateway::request
to align with the downstream MCP tool/session timeout of 180 seconds, or source
it from the existing configurable timeout setting if one is available. Preserve
the existing request construction and connector_gateway_unavailable error
handling.
In `@frontend/src/features/admin/components/ConnectorAppList.tsx`:
- Around line 346-368: Update the edit and disable buttons in the
ConnectorAppList action group to provide at least a 44px × 44px touch target on
mobile, using responsive sizing that may retain the existing 32px dimensions at
md and larger. Preserve their current behavior, styling, labels, and test IDs.
- Around line 687-696: Update the cancel and save button labels in the
ConnectorAppList component to use the admin translation keys at the correct
namespace, replacing the current common.cancel and common.save lookups with
t('cancel') and t('save') (or the established shared common namespace if
intentionally configured).
In `@wework/src/e2e/automation.ts`:
- Around line 208-209: Update the authToken initialization in the E2E setup to
require a non-empty VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN value instead of using the
committed fallback token. Fail setup immediately with a clear configuration
error when the variable is missing, and ensure the variable is provided only
through isolated E2E build configuration.
In `@wework/src/features/cloud-connection/LocalExecutorCloudBridge.tsx`:
- Around line 84-88: Update the payload mapping in LocalExecutorCloudBridge to
include each connected app’s description alongside slug and name, matching the
runtime.connectors.apps.sync payload used by WorkbenchProvider.
---
Nitpick comments:
In `@backend/app/services/connector_runtime.py`:
- Around line 449-540: Split _refresh_oauth_if_needed into focused helpers while
preserving its existing double-checked with_for_update locking and refresh
behavior. Extract the OAuth token request/error handling into one helper and
token validation/application/persistence into another, leaving
_refresh_oauth_if_needed responsible for eligibility checks, locking, and
orchestration; keep each function under roughly 50 lines.
In `@wework/scripts/ai-verify-environment.test.mjs`:
- Around line 7-15: Update the test input environment in the relevant
ai-verify-environment test to include WEGENT_EXECUTOR_APP_IPC_ADDR alongside the
other inherited executor environment keys, ensuring the filtering behavior
covers every key listed by INHERITED_EXECUTOR_ENV_KEYS.
- Around line 37-42: Add an expectation in the environment isolation test
alongside the existing WEGENT_EXECUTOR_* assertions to verify that
environment.WEGENT_EXECUTOR_APP_IPC_ADDR is undefined, preserving the test’s
missing-key verification pattern.
In `@wework/src/features/cloud-connection/LocalExecutorCloudBridge.test.tsx`:
- Around line 111-113: Update the assertion in the LocalExecutorCloudBridge test
for the runtime.connectors.apps.sync request to include the expected description
field in the tickets app payload, while preserving the existing slug and name
values.
- Around line 43-49: Update the listApps mock in the LocalExecutorCloudBridge
test to include the description field expected by the revised payload, and
update the corresponding assertion to verify that description is propagated
correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c680d47-bd21-458a-9db7-7b15efc4e8a1
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
backend/alembic/versions/20260716_e7f8a9b0c1d2_add_connector_apps.pybackend/alembic/versions/20260720_f8a9b0c1d2e3_add_connector_http_tools.pybackend/app/api/api.pybackend/app/api/endpoints/admin/connector_apps.pybackend/app/api/endpoints/admin/router.pybackend/app/api/endpoints/connector_apps.pybackend/app/api/endpoints/connector_runtime.pybackend/app/core/config.pybackend/app/models/__init__.pybackend/app/models/connector.pybackend/app/schemas/connector.pybackend/app/services/connector_apps.pybackend/app/services/connector_runtime.pybackend/pyproject.tomlbackend/tests/api/test_connector_apps_api.pybackend/tests/services/test_connector_runtime.pydocs/en/developer-guide/connector-apps.mddocs/zh/developer-guide/connector-apps.mdexecutor/src/bin/wegent-executor.rsexecutor/src/connector_gateway.rsexecutor/src/connector_mcp.rsexecutor/src/lib.rsexecutor/src/runtime_work/connectors.rsexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/mod.rsexecutor/tests/config_contract.rsfrontend/src/__tests__/features/admin/components/ConnectorAppList.test.tsxfrontend/src/apis/admin.tsfrontend/src/app/admin/page.tsxfrontend/src/features/admin/components/AdminTabNav.tsxfrontend/src/features/admin/components/ConnectorAppList.tsxfrontend/src/i18n/locales/en/admin.jsonfrontend/src/i18n/locales/zh-CN/admin.jsonwework/scripts/ai-verify-environment.mjswework/scripts/ai-verify-environment.test.mjswework/src/api/cloud/connectorApps.tswework/src/components/chat/composer/composerMentionCandidates.test.tswework/src/components/chat/composer/composerMentionCandidates.tswework/src/e2e/automation.tswework/src/features/cloud-connection/LocalExecutorCloudBridge.test.tsxwework/src/features/cloud-connection/LocalExecutorCloudBridge.tsxwework/src/features/cloud-connection/localExecutorCloudConnection.tswework/src/features/workbench/WorkbenchProvider.tsxwework/src/types/api.ts
| op.add_column( | ||
| "connector_apps", | ||
| sa.Column("http_tools", sa.JSON(), nullable=False, server_default="[]"), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
MySQL JSON column literal DEFAULT value not allowed 8.0.13 expression default
💡 Result:
In MySQL, JSON columns can have a default value starting from version 8.0.13 [1][2][3]. Prior to this version, it was not possible to assign a non-NULL default value to a JSON column [1][4]. As of MySQL 8.0.13, you can assign a default value to a JSON column, but it must be specified as an expression, even if the value itself is a literal constant [5][4]. To achieve this, you must enclose the default value within parentheses [5][6]. Example of correct syntax: CREATE TABLE t1 ( j JSON DEFAULT (JSON_ARRAY)); Or for a JSON object: CREATE TABLE t1 ( j JSON DEFAULT ('{"key": "value"}')); If you do not use parentheses, or if you attempt to use a literal without an expression, MySQL will raise an error because JSON, BLOB, TEXT, and GEOMETRY data types do not support literal default values [5][4][6]. Note on ALTER TABLE: When using ALTER TABLE... ALTER COLUMN to set a default value, some versions of MySQL may exhibit inconsistent behavior if the expression is interpreted as a simple string literal rather than a functional expression. Using MODIFY COLUMN is generally recommended to ensure the default is correctly stored as an expression default [7].
Citations:
- 1: https://dev.mysql.com/doc/refman/8.0/en/json.html
- 2: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-13.html
- 3: https://dev.mysql.com/blog-archive/the-mysql-8-0-13-maintenance-release-is-generally-available/
- 4: https://www.percona.com/blog/cant-we-assign-a-default-value-to-the-blob-text-geometry-and-json-data-types/
- 5: https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html
- 6: https://dev.mysql.com/doc/refman/8.1/en/data-type-defaults.html
- 7: https://stackoverflow.com/questions/78790171/unable-to-set-default-on-json-column-in-mysql-8-0-31
Use a MySQL-safe JSON default
MySQL JSON columns need an expression default (DEFAULT (...)), not a bare string literal, so server_default="[]" will break on MySQL. Use a backfill or a server default expression instead.
🤖 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 `@backend/alembic/versions/20260720_f8a9b0c1d2e3_add_connector_http_tools.py`
around lines 21 - 24, Update the http_tools column definition in the migration
to use a MySQL-compatible JSON default expression or backfill existing rows
before enforcing non-nullability, replacing the bare server_default string while
preserving the empty-array default for new records.
| # Optional public Backend root used for third-party connector OAuth callbacks. | ||
| # Configure this when Backend is behind a reverse proxy with a different origin. | ||
| CONNECTOR_OAUTH_CALLBACK_BASE_URL: str = "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config.py excerpt =="
sed -n '200,250p' backend/app/core/config.py
echo
echo "== connector_apps.py excerpt =="
sed -n '1,240p' backend/app/api/endpoints/connector_apps.py
echo
echo "== dependency declarations mentioning pydantic =="
rg -n "pydantic|pydantic-settings|BaseSettings|HttpUrl|AnyUrl|HttpUrl" pyproject.toml backend/ -SRepository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config.py file stats =="
wc -l backend/app/core/config.py
echo "== connector_apps.py file stats =="
wc -l backend/app/api/endpoints/connector_apps.pyRepository: wecode-ai/Wegent
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config class outline =="
ast-grep outline backend/app/core/config.py --view expanded | sed -n '1,220p'
echo
echo "== config validators and URL-related types =="
rg -n "field_validator|model_validator|AnyHttpUrl|HttpUrl|AnyUrl|UrlConstraints|CONNECTOR_OAUTH_CALLBACK_BASE_URL|WEWORK_AUTHORIZE_BASE_URL|SITES_API_BASE_URL" backend/app/core/config.py backend/app/schemas/site.py backend/app/schemas/connector.py -S
echo
echo "== relevant pydantic-settings usage =="
rg -n "BaseSettings|SettingsConfigDict|env_prefix|env_file" backend/app/core/config.py -SRepository: wecode-ai/Wegent
Length of output: 13585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("backend/app/core/config.py")
text = path.read_text()
for key in [
"class Settings",
"CONNECTOR_OAUTH_CALLBACK_BASE_URL",
"field_validator",
"model_validator",
"BaseSettings",
"SettingsConfigDict",
]:
print(f"{key}: {text.find(key)}")
PYRepository: wecode-ai/Wegent
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config.py class and field definitions (selected lines) =="
nl -ba backend/app/core/config.py | sed -n '1,260p'
echo
echo "== site schema URL field types =="
nl -ba backend/app/schemas/site.py | sed -n '1,120p'
echo
echo "== connector schema types =="
nl -ba backend/app/schemas/connector.py | sed -n '1,220p'Repository: wecode-ai/Wegent
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config.py outline =="
ast-grep outline backend/app/core/config.py --view expanded | sed -n '1,160p'
echo
echo "== precise lines around settings class =="
nl -ba backend/app/core/config.py | sed -n '1,120p'
echo
echo "== precise lines around the OAuth callback field =="
nl -ba backend/app/core/config.py | sed -n '210,240p'Repository: wecode-ai/Wegent
Length of output: 8524
🌐 Web query:
Pydantic v2 AnyHttpUrl query fragment allowed base url validation http https no query fragment
💡 Result:
In Pydantic v2, AnyHttpUrl and HttpUrl do not explicitly restrict the presence of query strings or URL fragments by default [1][2]. These types are designed to accept valid HTTP or HTTPS URLs, which naturally support optional components like query parameters and fragments [1][3]. When you use AnyHttpUrl or HttpUrl in a model, Pydantic parses the input URL into its constituent parts—scheme, host, port, path, query, and fragment—and makes them available as properties [1][2]. Because these components are optional parts of a standard URL, Pydantic does not reject a URL simply because it contains a query string or a fragment [1][2]. If you specifically need to disallow query strings or fragments, you must implement custom validation. You can achieve this by adding a validator to your model field that checks the query and fragment properties of the parsed URL object [1][2]. Example implementation: from pydantic import BaseModel, HttpUrl, field_validator, ValidationInfo class MyModel(BaseModel): url: HttpUrl @field_validator('url') @classmethod def validate_no_query_or_fragment(cls, v: HttpUrl) -> HttpUrl: if v.query is not None or v.fragment is not None: raise ValueError('Query strings and fragments are not allowed') return v This approach allows you to enforce stricter validation rules than the default Pydantic network types provide [1][2].
Citations:
- 1: https://pydantic.dev/docs/validation/latest/api/pydantic/networks/
- 2: https://github.com/pydantic/pydantic/blob/main/pydantic/networks.py
- 3: https://pydantic.dev/docs/validation/2.12/api/pydantic/networks/
Validate CONNECTOR_OAUTH_CALLBACK_BASE_URL
It’s concatenated directly into the OAuth redirect URI. Reject non-absolute or non-HTTP(S) values, forbid query/fragment components, and require HTTPS outside local/dev environments so a bad config can’t break OAuth or expose auth codes.
🤖 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 `@backend/app/core/config.py` around lines 227 - 229, Validate
CONNECTOR_OAUTH_CALLBACK_BASE_URL as an absolute HTTP(S) URL without query or
fragment components before using it for OAuth redirect construction. Require
HTTPS unless running in the existing local/development environment, and reject
invalid values through the configuration validation path.
| @model_validator(mode="after") | ||
| def validate_argument_locations(self) -> "ConnectorHttpToolDefinition": | ||
| properties = self.input_schema.get("properties", {}) | ||
| unknown = set(self.argument_locations) - set(properties) | ||
| if unknown: | ||
| raise ValueError( | ||
| "argument_locations must reference input_schema properties: " | ||
| + ", ".join(sorted(unknown)) | ||
| ) | ||
| path_arguments = { | ||
| name | ||
| for name, location in self.argument_locations.items() | ||
| if location == "path" | ||
| } | ||
| missing_placeholders = { | ||
| name for name in path_arguments if "{" + name + "}" not in self.path | ||
| } | ||
| if missing_placeholders: | ||
| raise ValueError( | ||
| "path arguments require matching placeholders: " | ||
| + ", ".join(sorted(missing_placeholders)) | ||
| ) | ||
| placeholders = set(re.findall(r"\{([A-Za-z0-9_.-]+)\}", self.path)) | ||
| required = set(self.input_schema.get("required", [])) | ||
| invalid_placeholders = placeholders - set(properties) | ||
| optional_placeholders = placeholders - required | ||
| if invalid_placeholders or optional_placeholders: | ||
| raise ValueError( | ||
| "path placeholders must be required input_schema properties" | ||
| ) | ||
| return self |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Path placeholders can bypass argument_locations location check.
The validator confirms placeholders are required schema properties, but never confirms the reverse: a property whose argument_locations entry is "query"/"body" can still appear as a {name} placeholder in path without any error. That configuration is accepted, yet at request-build time it's ambiguous whether {name} gets substituted into the path or sent as a query/body param, likely leaving a literal {name} token in the outgoing URL.
🐛 Proposed fix: require path arguments and path placeholders to match exactly
`@model_validator`(mode="after")
def validate_argument_locations(self) -> "ConnectorHttpToolDefinition":
properties = self.input_schema.get("properties", {})
unknown = set(self.argument_locations) - set(properties)
if unknown:
raise ValueError(
"argument_locations must reference input_schema properties: "
+ ", ".join(sorted(unknown))
)
- path_arguments = {
- name
- for name, location in self.argument_locations.items()
- if location == "path"
- }
- missing_placeholders = {
- name for name in path_arguments if "{" + name + "}" not in self.path
- }
- if missing_placeholders:
- raise ValueError(
- "path arguments require matching placeholders: "
- + ", ".join(sorted(missing_placeholders))
- )
placeholders = set(re.findall(r"\{([A-Za-z0-9_.-]+)\}", self.path))
required = set(self.input_schema.get("required", []))
invalid_placeholders = placeholders - set(properties)
optional_placeholders = placeholders - required
if invalid_placeholders or optional_placeholders:
raise ValueError(
"path placeholders must be required input_schema properties"
)
+ path_arguments = {
+ name
+ for name, location in self.argument_locations.items()
+ if location == "path"
+ }
+ if path_arguments != placeholders:
+ raise ValueError(
+ "path arguments and path placeholders must match exactly"
+ )
return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @model_validator(mode="after") | |
| def validate_argument_locations(self) -> "ConnectorHttpToolDefinition": | |
| properties = self.input_schema.get("properties", {}) | |
| unknown = set(self.argument_locations) - set(properties) | |
| if unknown: | |
| raise ValueError( | |
| "argument_locations must reference input_schema properties: " | |
| + ", ".join(sorted(unknown)) | |
| ) | |
| path_arguments = { | |
| name | |
| for name, location in self.argument_locations.items() | |
| if location == "path" | |
| } | |
| missing_placeholders = { | |
| name for name in path_arguments if "{" + name + "}" not in self.path | |
| } | |
| if missing_placeholders: | |
| raise ValueError( | |
| "path arguments require matching placeholders: " | |
| + ", ".join(sorted(missing_placeholders)) | |
| ) | |
| placeholders = set(re.findall(r"\{([A-Za-z0-9_.-]+)\}", self.path)) | |
| required = set(self.input_schema.get("required", [])) | |
| invalid_placeholders = placeholders - set(properties) | |
| optional_placeholders = placeholders - required | |
| if invalid_placeholders or optional_placeholders: | |
| raise ValueError( | |
| "path placeholders must be required input_schema properties" | |
| ) | |
| return self | |
| `@model_validator`(mode="after") | |
| def validate_argument_locations(self) -> "ConnectorHttpToolDefinition": | |
| properties = self.input_schema.get("properties", {}) | |
| unknown = set(self.argument_locations) - set(properties) | |
| if unknown: | |
| raise ValueError( | |
| "argument_locations must reference input_schema properties: " | |
| ", ".join(sorted(unknown)) | |
| ) | |
| placeholders = set(re.findall(r"\{([A-Za-z0-9_.-]+)\}", self.path)) | |
| required = set(self.input_schema.get("required", [])) | |
| invalid_placeholders = placeholders - set(properties) | |
| optional_placeholders = placeholders - required | |
| if invalid_placeholders or optional_placeholders: | |
| raise ValueError( | |
| "path placeholders must be required input_schema properties" | |
| ) | |
| path_arguments = { | |
| name | |
| for name, location in self.argument_locations.items() | |
| if location == "path" | |
| } | |
| if path_arguments != placeholders: | |
| raise ValueError( | |
| "path arguments and path placeholders must match exactly" | |
| ) | |
| return self |
🤖 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 `@backend/app/schemas/connector.py` around lines 84 - 114, Update
ConnectorHttpToolDefinition.validate_argument_locations so the set of path
placeholders in self.path must exactly match the names in argument_locations
whose location is "path". Reject both placeholders missing a corresponding path
argument and path arguments missing placeholders, while preserving the existing
schema-property and required-property validation.
| pub(crate) async fn request( | ||
| &self, | ||
| method: Method, | ||
| path: &str, | ||
| body: Option<Value>, | ||
| ) -> Result<Value, ConnectorGatewayError> { | ||
| if self.expires_at_ms <= now_ms() + 5_000 { | ||
| return Err(ConnectorGatewayError::new( | ||
| "connector_token_expired", | ||
| "Wegent connector authorization expired; reconnect cloud", | ||
| )); | ||
| } | ||
| let url = format!( | ||
| "{}/connector-runtime/{}", | ||
| self.api_base_url, | ||
| path.trim_start_matches('/') | ||
| ); | ||
| let client = reqwest::Client::new(); | ||
| let mut request = client | ||
| .request(method, &url) | ||
| .bearer_auth(&self.connector_token) | ||
| .timeout(Duration::from_secs(75)); | ||
| if let Some(body) = body { | ||
| request = request.json(&body); | ||
| } | ||
| let response = request.send().await.map_err(|error| { | ||
| ConnectorGatewayError::new( | ||
| "connector_gateway_unavailable", | ||
| format!("Wegent connector gateway is unavailable: {error}"), | ||
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let value = response.json::<Value>().await.map_err(|error| { | ||
| ConnectorGatewayError::new( | ||
| "connector_gateway_invalid_response", | ||
| format!("Invalid connector gateway response: {error}"), | ||
| ) | ||
| })?; | ||
| if !status.is_success() { | ||
| let message = value | ||
| .get("detail") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("Connector gateway request failed"); | ||
| return Err(ConnectorGatewayError::new( | ||
| "connector_gateway_error", | ||
| format!("{message} (HTTP {status})"), | ||
| )); | ||
| } | ||
| Ok(value) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential timeout-layering mismatch on the gateway request.
The gateway request timeout is fixed at 75s, but the MCP child is registered with tool_timeout_sec: 180 (in runtime_work/connectors.rs) and the backend MCP session uses 180s reads. A backend tool that runs between 75s and 180s will be aborted here as connector_gateway_unavailable even though Codex would still be waiting. Consider aligning this timeout with the downstream tool timeout (or making it configurable).
🤖 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 `@executor/src/connector_gateway.rs` around lines 44 - 93, Update the timeout
configured in ConnectorGateway::request to align with the downstream MCP
tool/session timeout of 180 seconds, or source it from the existing configurable
timeout setting if one is available. Preserve the existing request construction
and connector_gateway_unavailable error handling.
| <div className="flex shrink-0 gap-1"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8" | ||
| onClick={() => openEdit(app)} | ||
| data-testid={`edit-connector-app-${app.id}`} | ||
| aria-label={t('connector_apps.edit')} | ||
| > | ||
| <Pencil className="h-4 w-4" /> | ||
| </Button> | ||
| {app.enabled ? ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8 hover:text-error" | ||
| onClick={() => void disable(app)} | ||
| data-testid={`disable-connector-app-${app.id}`} | ||
| aria-label={t('connector_apps.disable')} | ||
| > | ||
| <Power className="h-4 w-4" /> | ||
| </Button> | ||
| ) : null} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Card action buttons are below the mobile touch-target minimum.
The edit/disable icon buttons render at h-8 w-8 (32px). The admin surface is mobile-responsive (the tab nav switches to a dropdown on mobile), so these controls should meet the 44px minimum on mobile (e.g. responsive sizing that enlarges the hit area under md).
As per coding guidelines: "Mobile controls must be at least 44px × 44px."
🤖 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 `@frontend/src/features/admin/components/ConnectorAppList.tsx` around lines 346
- 368, Update the edit and disable buttons in the ConnectorAppList action group
to provide at least a 44px × 44px touch target on mobile, using responsive
sizing that may retain the existing 32px dimensions at md and larger. Preserve
their current behavior, styling, labels, and test IDs.
Source: Coding guidelines
| {t('common.cancel')} | ||
| </Button> | ||
| <Button | ||
| variant="primary" | ||
| onClick={onSave} | ||
| disabled={saving} | ||
| data-testid="save-connector-app-button" | ||
| > | ||
| {saving ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null} | ||
| {t('common.save')} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether admin.json defines a top-level `common` block with cancel/save
fd -t f 'admin.json' frontend/src/i18n/locales --exec sh -c 'echo "== $1 =="; jq -e ".common // empty | {cancel, save}" "$1" 2>/dev/null || echo "no common.cancel/common.save"' _ {}Repository: wecode-ai/Wegent
Length of output: 329
Use the admin translation keys here
frontend/src/i18n/locales/en/admin.json and zh-CN/admin.json define cancel and save at the top level, not under common, so these buttons should use t('cancel') / t('save') (or a shared common: namespace if that’s the intent).
🤖 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 `@frontend/src/features/admin/components/ConnectorAppList.tsx` around lines 687
- 696, Update the cancel and save button labels in the ConnectorAppList
component to use the admin translation keys at the correct namespace, replacing
the current common.cancel and common.save lookups with t('cancel') and t('save')
(or the established shared common namespace if intentionally configured).
Source: Coding guidelines
| const authToken = | ||
| import.meta.env.VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN?.trim() || 'wework-desktop-e2e-cloud-token' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the committed auth-token fallback.
Line 209 keeps a credential-like token in source and silently uses it when the environment is misconfigured. Require VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN and fail the E2E setup if it is missing, while ensuring this variable is only supplied in isolated E2E builds.
As per coding guidelines, credentials must not be committed and secrets must come from environment configuration.
Proposed fix
- const authToken =
- import.meta.env.VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN?.trim() || 'wework-desktop-e2e-cloud-token'
+ const authToken = import.meta.env.VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN?.trim()
+ if (!authToken) {
+ throw new Error('VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN is required')
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const authToken = | |
| import.meta.env.VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN?.trim() || 'wework-desktop-e2e-cloud-token' | |
| const authToken = import.meta.env.VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN?.trim() | |
| if (!authToken) { | |
| throw new Error('VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN is required') | |
| } |
🤖 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 `@wework/src/e2e/automation.ts` around lines 208 - 209, Update the authToken
initialization in the E2E setup to require a non-empty
VITE_WEWORK_E2E_CLOUD_AUTH_TOKEN value instead of using the committed fallback
token. Fail setup immediately with a clear configuration error when the variable
is missing, and ensure the variable is provided only through isolated E2E build
configuration.
Source: Coding guidelines
| await requestLocalExecutor('runtime.connectors.apps.sync', { | ||
| apps: apps | ||
| .filter(app => app.connection.status === 'connected') | ||
| .map(app => ({ slug: app.slug, name: app.name })), | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Include description in the sync payload to match WorkbenchProvider.
The payload provided here omits description, while WorkbenchProvider includes it when calling runtime.connectors.apps.sync (i.e., description: app.description). This discrepancy could result in the Executor occasionally receiving empty descriptions for Connector Apps when synchronized from the background.
🛠️ Proposed fix
await requestLocalExecutor('runtime.connectors.apps.sync', {
apps: apps
.filter(app => app.connection.status === 'connected')
- .map(app => ({ slug: app.slug, name: app.name })),
+ .map(app => ({ slug: app.slug, name: app.name, description: app.description })),
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await requestLocalExecutor('runtime.connectors.apps.sync', { | |
| apps: apps | |
| .filter(app => app.connection.status === 'connected') | |
| .map(app => ({ slug: app.slug, name: app.name })), | |
| }) | |
| await requestLocalExecutor('runtime.connectors.apps.sync', { | |
| apps: apps | |
| .filter(app => app.connection.status === 'connected') | |
| .map(app => ({ slug: app.slug, name: app.name, description: app.description })), | |
| }) |
🤖 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 `@wework/src/features/cloud-connection/LocalExecutorCloudBridge.tsx` around
lines 84 - 88, Update the payload mapping in LocalExecutorCloudBridge to include
each connected app’s description alongside slug and name, matching the
runtime.connectors.apps.sync payload used by WorkbenchProvider.
1 participant
Summary
wegent_apps) without changing CodexSecurity boundaries
connectors:invokeJWTValidation
$menu -> generated Connector Skill -> Codex ->wegent_appsstdio MCP -> scoped Backend Runtime -> HTTP adapter -> internal HTTP service -> assistant resultWEGENT_EXECUTOR_HOMEpropagation to the Codex-launched MCP childSummary by CodeRabbit