feat(web): report the runtime inputs a connector declares - #2132
feat(web): report the runtime inputs a connector declares#2132AlexLiu190625 wants to merge 2 commits into
Conversation
Two owner-scoped read endpoints report which runtime inputs an agent's connectors declare and which of them a task already has: one keyed by agent for a pre-flight check, one keyed by task for the in-chat prompt. Both return declared key names, their normalized type and whether a value is already stored - never a stored value, and never a connector's URL, headers, environment or authentication configuration. Both live on the chat router, next to the task-keyed runtime-extensions endpoint they are shaped after, and both reuse the predicate task creation already applies to an agent id rather than introducing a second authorization path for the same resource. The team scope both endpoints resolve is pinned to the one tool loading uses at run time, so a team-shared connector that the run-time gate demands a value for is always one the caller can also see here. Neither endpoint asserts that required values are present: reporting what is missing is the whole point, and raising on a missing value belongs to the per-turn gate that runs later.
Task creation now returns the same requirements report as the read endpoints, so a client can prompt for the missing values before sending the first message instead of after a failed turn. The request body is unchanged and still ignores any runtime values a caller smuggles in. The report reuses the connector resolution the creation path already performs, so it costs no extra query. The field is null rather than a report on the public widget and share-link create paths: those callers are anonymous guests who never see a connector's declared key names, so evaluating requirements for them would hand that information to a new audience this change does not intend to reach. The field still always appears in the response body either way.
![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 introduces endpoints and services to manage connector runtime requirements and values, including GET endpoints for agent and task requirements, and a POST endpoint to merge context values with a compare-and-swap mechanism. It also adds comprehensive unit and integration tests, including PostgreSQL-specific concurrency tests. The review feedback suggests optimizing dictionary and object comparisons in the service layer by replacing redundant JSON serialization-based comparisons with Python's native equality operators, which improves performance.
4bcb602 to
10d04f4
Compare
AlexLiu190625
changed the title
|
The value-submission endpoint (POST /api/chat/task/{task_id}/connector-runtime-values) |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR adds two read-only GET endpoints (agent-keyed and task-keyed) that report which runtime inputs a connector declares (e.g. auth_token) and whether each already has a stored value, plus a connector_runtime_requirements field on the task-create response. It's explicitly scoped as discovery/reporting only for #1251 -- the actual fix (UI dialog + value-submission endpoint) ships in a follow-up PR. The read-only layer is a reasonable prerequisite for that dialog, and the negative-path/auth handling is careful, but the PR's core positive-case output (satisfied: true) has zero test coverage anywhere in the suite.
Blocking: yes -- recommended event: REQUEST_CHANGES -- the boolean this PR exists to compute (satisfied: true for a stored, required runtime input) has no test asserting it, on either new endpoint or the task-create response.
Approach verdict: acceptable-with-reservations
- Top-level
satisfiedis permanently unachievable for any connector with a required secret in this phase -- it ANDs together sections that are structurally never satisfiable yet. That's a documented, intentional constant per the PR's own Disclosure 2, but there's no per-input discriminator letting a future consumer distinguish "not yet collected" from "not collectable this phase." Worth considering before the follow-up UI builds logic on top of this shape. - The task-keyed endpoint resolves the task's agent via a raw query instead of the established
_load_agent_for_task_runtimehelper that the real per-turn execution path uses (see finding #2 below). For workforce-manager agents or agents whose visibility has since changed, this can make the report list team-shared connectors the real per-turn execution would never resolve. Over-reporting only, narrow blast radius, but worth aligning report-vs-reality. - The two new endpoints disagree on admin-bypass scope (task-keyed denies admins, agent-keyed inherits an existing unconditional admin bypass) with no stated principle for the asymmetry. The bypass itself is pre-existing/inherited and already disclosed by the author, not introduced here -- but the inconsistency between sibling endpoints is worth a documented rationale or unification.
- The task-create response's
connector_runtime_requirementsfield is functionally redundant with what the agent-keyed GET already reports for the same agent/user, and there's currently no write path that could act on it post-create (context is immutable after creation). Not a defect in this PR, but worth flagging as design input for whoever builds the follow-up's value-write endpoint. - The PR's framing ("part of fixing #1251") is honest about scope, but worth a one-line reminder: #1251's actual failure mode (task created despite unmet requirement, later turns 400) is 100% unchanged by this PR alone -- this PR only adds visibility, not prevention.
Findings by severity
Major
1. [MAJOR, BLOCKING] src/xagent/web/services/connector_runtime.py:909-913, and tests/web/test_connector_runtime_entrypoints_e2e.py (entire file) -- No test anywhere asserts satisfied: true for any per-key input on the task-keyed endpoint or the task-create response. Every satisfied assertion in the suite checks False except one vacuous zero-connectors True case. The one real TaskConnectorRuntimeContext row ever created in tests is deliberately routed to the agent-keyed endpoint (which always reports False by contract) to test scope isolation -- never to the task-keyed endpoint where the stored-context branch actually matters. Hardcoding satisfied = False unconditionally in _build_connector_report would currently pass the entire suite. This is the exact value the planned follow-up UI dialog will gate re-prompting on; a silent regression here ships undetected and produces an incorrect user-visible result. Needs a test: task with a stored TaskConnectorRuntimeContext row for a required key -> task-keyed GET (and/or task-create response) reports that key satisfied: true, and top-level satisfied: true when it's the only required key.
Minor
2. src/xagent/web/api/chat.py:5363-5367 -- Task-keyed endpoint resolves the task's agent via a raw db.query(Agent).filter(Agent.id == task.agent_id).first() instead of _load_agent_for_task_runtime (services/llm_utils.py:1250-1306), which the real per-turn execution path uses and which returns None for a workforce-generated manager agent or since-revoked-visibility agent (falling back to personal-only connectors). The new endpoint always uses the raw agent's real team_id, so it can report team-shared connectors the actual turn would never resolve. Untested for this scenario on the task-keyed endpoint. Recommend routing through _load_agent_for_task_runtime for report/reality parity, plus a regression test.
6. src/xagent/web/services/connector_runtime.py:435 (docstring of the new resolve_agent_runtime_requirements, referencing prepare_connector_runtime_selection_snapshot) vs the new resolve_agent_runtime_requirements -- both independently derive the same connector-ref set via the same underlying calls, which the new function's own docstring warns against ("do not derive the refs any other way"). The old function is still live with 4 unchanged callers. A regression test catches drift after the fact but doesn't prevent it structurally. Recommend having the old function delegate to the new one. Separately, the new function's docstring claims refs are persisted "verbatim... in exactly that order," but both write (_sort_connector_refs) and read (_load_task_selected_refs) paths re-sort by (connector_type, connector_id) -- the actual invariant is canonical sorting, not order-preservation. Please correct the docstring.
7. src/xagent/web/services/connector_runtime.py:897 (_build_connector_report, via the pre-existing _has_runtime_declaration) -- runtime_input_schema OR runtime_bindings means a connector with runtime_bindings but no runtime_input_schema produces a report entry with only name+ref and an empty inputs: [] -- inert noise, doesn't affect satisfied or leak anything new. Separately, no test anywhere exercises a custom_api-type connector on these new endpoints -- every test uses MCPServer fixtures only. Worth adding coverage.
8. src/xagent/web/services/connector_runtime.py:908 (_build_connector_report, vs. the pre-existing _require_context_values) -- validates declared-key syntax via validate_runtime_source_key and raises on a malformed key, but _build_connector_report runs no equivalent validation, so a connector with a malformed declared key is reported normally (possibly satisfied: true) while the per-turn execution gate would hard-fail. Practically reachable since runtime_input_schema has no key-format validation at connector create/update time (custom_api.py, mcp.py). Recommend applying the same validation (or at least flagging invalid keys) in the report path.
9. src/xagent/web/services/connector_runtime.py:441-445 (resolve_agent_runtime_requirements docstring) -- the satisfied field means different things depending on which endpoint returns it: agent-keyed answers "has no required input at all" (per this docstring), task-keyed answers "every required input has a stored value." Same field name, same schema, undocumented semantic difference, while the module docstring implies both endpoints return the same shape of report. Please document the distinction explicitly.
10. src/xagent/web/services/connector_runtime.py:902-906 -- no test exercises the auth_selector section at all (neither the MCP branch that emits it, nor the custom_api skip-branch here). Removing this continue wouldn't fail any existing test (no fixture stores an auth_selector key in a custom_api connector's schema). Not a security issue -- the report only shows declared key names from the connector owner's own schema, and _validate_values_against_schema independently rejects any real auth_selector value submitted for a non-MCP connector regardless of what the report shows -- but worth a follow-up test to catch drift.
11. tests/web/test_connector_runtime_entrypoints_e2e.py:1380 -- expired (always False in this phase, per the schema's own docstring -- a deliberate frozen-contract placeholder, not dead flexibility) is never asserted anywhere in this file despite being a wire field. Recommend adding an assertion for it here, mirroring the existing secrets_expires_at is None assertion pattern used at this and other call sites. Optionally consider pinning secrets_expires_at's wire format (ISO string vs epoch) now, since the contract is meant to be frozen.
12. tests/web/test_connector_runtime_entrypoints_e2e.py:1982 -- this is currently the only satisfied: true assertion in the file, and it's the vacuous zero-connectors case. Consider adding a real positive case: an agent-keyed GET where the connector's only runtime inputs are optional (not required), which should also report satisfied: true but currently has no test. Also untested elsewhere: a task with agent_id IS NULL on the task-keyed endpoint (degrades gracefully, just unverified), and a genuinely nonexistent task_id/agent_id vs. one that exists but isn't owned by the caller.
13. tests/web/test_connector_runtime_entrypoints_e2e.py:2254-2256 -- dangling section header comment (# A4: POST /task/{task_id}/connector-runtime-values.) with zero tests under it, for an endpoint confirmed (per the author's own PR comment) to have moved to the follow-up PR. Please drop until that PR lands. Separately, test_agent_requirements_endpoint_bypassing_team_resolver_hides_shared_connector (line 1677) is a near-duplicate of the first half of test_team_shared_connector_visible_across_read_endpoints (line 1525), and its own docstring admits it can't actually express the "bypass team resolver" mutation it's named after. Consider folding it in or renaming/clarifying it.
Note on prior review
Prior gemini-code-assist findings (2 inline comments about _canonical_json comparison redundancy in a POST value-merge endpoint) concerned code that has since moved to a follow-up PR -- confirmed no POST endpoint or merge logic exists anywhere in this PR's current diff. Not re-opened or re-litigated here.
Simplification opportunities
shrink: hoist the repeated 3-line comment +connector_runtime_requirements=None(src/xagent/web/api/public_chat_access.py:1039-1042, 1147-1150, 1220-1223, 1312-1315) into a single module-level constant_NO_CONNECTOR_RUNTIME_REQUIREMENTS = None, referenced at eachTaskCreateResponse(...)call site.
net: -12 lines possible.
Blocking status & recommended decision
- Blocking: yes
- Recommended event: REQUEST_CHANGES
- Blocking issues:
src/xagent/web/services/connector_runtime.py:909-913&tests/web/test_connector_runtime_entrypoints_e2e.py, major, thesatisfied=Trueboolean this PR exists to compute has zero positive-branch test coverage -- a regression here ships undetected, [new].
| for key, declaration in declarations.items(): | ||
| satisfied = ( | ||
| key in context_stored | ||
| if section_name == RUNTIME_INPUT_CONTEXT |
There was a problem hiding this comment.
[MAJOR, BLOCKING] No test anywhere asserts satisfied: true for a stored, required runtime input on this branch (satisfied = key in context_stored if section_name == RUNTIME_INPUT_CONTEXT else False). Every satisfied assertion in tests/web/test_connector_runtime_entrypoints_e2e.py checks False except one vacuous zero-connectors True case; the one real TaskConnectorRuntimeContext row ever created in tests is routed to the agent-keyed endpoint instead, which always reports False by contract. Hardcoding satisfied = False here would currently pass the whole suite. This is the exact value the planned follow-up UI dialog gates re-prompting on. Please add a test: task with a stored TaskConnectorRuntimeContext row for a required key -> task-keyed GET (and/or task-create response) reports that key satisfied: true.
| if task is None: | ||
| raise HTTPException(status_code=404, detail="Task not found") | ||
| agent = ( | ||
| db.query(Agent).filter(Agent.id == task.agent_id).first() |
There was a problem hiding this comment.
[MINOR] This resolves the task's agent via a raw db.query(Agent).filter(Agent.id == task.agent_id).first() instead of the established _load_agent_for_task_runtime (services/llm_utils.py:1250-1306) used by the real per-turn execution path. That helper returns None for a workforce-generated manager agent or an agent whose visibility has since changed, falling back to personal-only connectors -- this endpoint always uses the raw agent's real team_id, so it can report team-shared connectors the real turn would never resolve. Untested for this scenario on this endpoint. Recommend routing through _load_agent_for_task_runtime for report/reality parity plus a regression test.
| Calls ``resolve_agent_selected_connectors`` exactly once. The returned | ||
| refs are ``_runtime_declared_refs`` applied to that same call's result | ||
| -- same filter, same order -- because a caller creating a task persists | ||
| them verbatim into ``Task.connector_runtime_selected_refs``, and every |
There was a problem hiding this comment.
[MINOR] resolve_agent_runtime_requirements (added by this PR) and this pre-existing prepare_connector_runtime_selection_snapshot (still live, 4 unchanged callers) independently derive the same connector-ref set via the same underlying calls -- the new function's own docstring warns against exactly that ("do not derive the refs any other way"). A regression test catches drift after the fact but doesn't prevent it structurally. Recommend having this function delegate to the new one. Also, the new function's docstring claims refs are persisted "verbatim... in exactly that order", but both write (_sort_connector_refs) and read (_load_task_selected_refs) paths re-sort by (connector_type, connector_id); the actual invariant is canonical sorting, not order-preservation. Please correct that docstring.
| schema = _runtime_input_schema(connector) | ||
| context_stored = stored_context or {} | ||
| inputs: list[ConnectorRuntimeInputModel] = [] | ||
| for section_name in ( |
There was a problem hiding this comment.
[MINOR] runtime_input_schema OR runtime_bindings means a connector with runtime_bindings but no runtime_input_schema produces a report entry with only name+ref and an empty inputs: [] -- inert noise. Doesn't affect satisfied or leak anything new, but worth tightening. Separately, no test in the suite exercises a custom_api-type connector on these new endpoints -- every test uses MCPServer fixtures only. Please add coverage.
| ): | ||
| continue | ||
| declarations = _schema_section(schema, section_name) | ||
| for key, declaration in declarations.items(): |
There was a problem hiding this comment.
[MINOR] _require_context_values validates declared-key syntax via validate_runtime_source_key and raises on a malformed key, but _build_connector_report runs no equivalent validation -- a connector with a malformed declared key is reported normally (possibly satisfied: true) while the per-turn execution gate would hard-fail. Practically reachable since runtime_input_schema has no key-format validation at connector create/update time (custom_api.py, mcp.py). Recommend applying the same validation in the report path, or at least flagging invalid keys in the response.
| RUNTIME_INPUT_AUTH_SELECTOR, | ||
| ): | ||
| if ( | ||
| section_name == RUNTIME_INPUT_AUTH_SELECTOR |
There was a problem hiding this comment.
[MINOR] No test exercises the auth_selector section at all -- neither the MCP branch that emits it nor this custom_api skip-branch. Removing the continue here wouldn't fail any existing test (no fixture stores an auth_selector key in a custom_api connector's schema). Not a security issue -- the report only shows declared key names from the connector owner's own schema, and _validate_values_against_schema independently rejects real auth_selector values for non-MCP connectors -- but worth a follow-up test to catch drift.
| assert leaked not in response.text | ||
|
|
||
| payload = response.json() | ||
| assert payload["secrets_expires_at"] is None |
There was a problem hiding this comment.
[MINOR] expired (always False in this phase, per the schema's own docstring -- a deliberate frozen-contract placeholder, not dead flexibility) is never asserted anywhere in this file despite being a wire field. Recommend adding an assertion for it here, mirroring the existing secrets_expires_at is None assertion pattern used at this and other call sites.
| payload = response.json() | ||
| assert "connector_runtime_requirements" in payload | ||
| assert payload["connector_runtime_requirements"] == { | ||
| "satisfied": True, |
There was a problem hiding this comment.
[MINOR] This is currently the only satisfied: true assertion in the file, and it's the vacuous zero-connectors case. Consider adding a real positive case here or nearby: an agent-keyed GET where the connector's only runtime inputs are optional (not required), which should also report satisfied: true but currently has no test. Also untested elsewhere: a task with agent_id IS NULL on the task-keyed endpoint (degrades gracefully, just unverified), and a genuinely nonexistent task_id/agent_id vs. one that exists but isn't owned by the caller.
| assert persisted_refs == expected_wire | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
[MINOR] Dangling section header comment ("A4: POST /task/{task_id}/connector-runtime-values") with zero tests under it, for an endpoint confirmed to have moved to the follow-up PR. Please drop this until that PR lands. Separately, test_agent_requirements_endpoint_bypassing_team_resolver_hides_shared_connector (line 1677) is a near-duplicate of the first half of test_team_shared_connector_visible_across_read_endpoints (line 1525), and its own docstring admits it can't express the "bypass team resolver" mutation it's named after. Consider folding it in or renaming/clarifying it.
| else None, | ||
| channel_id=task.channel_id, | ||
| channel_name=task.channel_name, | ||
| # This path never resolves connector-runtime requirements for |
There was a problem hiding this comment.
shrink: This 3-line comment + connector_runtime_requirements=None, is repeated verbatim at 4 TaskCreateResponse(...) call sites (here, 1147-1150, 1220-1223, 1312-1315). Consider hoisting into a single module-level constant, e.g. _NO_CONNECTOR_RUNTIME_REQUIREMENTS = None with the comment attached once, referenced at each site. net: -12 lines possible, no readability loss.
3 participants
What
Server-side support for reporting the runtime inputs a connector
declares: two read endpoints, and the same report on the task-create
response.
Why
Part of #1251. A connector can declare a required runtime
input (for example an auth token), and today nothing in the web chat path
ever asks the caller for that input's value: the task is created, the
first turn is queued, and only then does execution fail because the value
was never collected. The caller's only workaround is to remove the
connector.
Scope: no UI in this PR
The chat dialog that calls these endpoints ships in a following PR. This
PR is deliberately server-only, so that the concurrency contract on the
value endpoint and the browser-side interaction get separate review
passes. The value-submission endpoint itself ships in the PR that follows
this one, once this one merges.
The two endpoints below already have a concrete consumer: the dialog in
that follow-up PR, by way of the value-submission endpoint that precedes
it there. Their response shape, field types, and empty-value behavior are
fixed by this PR, so this is not a "build it now, wire it up later"
endpoint -- the follow-up PR's implementation is constrained by what
ships here, not the other way around.
Splitting the work this way is intentional: the value endpoint's
concurrency contract (per-key merge, conditional update, bounded retry)
and the browser-side interaction are two different things to review, and
mixing them into one PR would dilute both.
How
declares and which of them already have a value:
GET /api/chat/agent/{agent_id}/connector-runtime-requirements(beforea task exists) and
GET /api/chat/task/{task_id}/connector-runtime-requirements(for atask that already exists). Neither returns a stored value itself, and
neither returns a connector's URL, headers, environment, or
authentication configuration.
/api/chat/agent/{agent_id}/...rather than under/api/agents, nextto the existing task-keyed
/api/chat/task/{task_id}/runtime-extensionsendpoint it is shaped after. It reuses the same predicate task creation
already applies to an agent id, so it needs no second authorization path
for the same resource, and it does not require moving that predicate out
of the module it already lives in.
POST /api/chat/task/create) now returns the same reporton its response, reusing the connector resolution it already performs,
so this costs no extra query. The request body is unchanged. The new
response field,
connector_runtime_requirements, is always present onevery task-create response: it carries a real report on this path, and
is
nullon the public widget and share-link create paths, whererequirements are never evaluated for the caller -- those callers are
anonymous visitors who should not receive a connector's key names.
reporting what is missing is the point, and raising on a missing value
belongs to the per-turn execution gate that already exists and is
unchanged by this PR.
Disclosures
These two new read endpoints return a connector's runtime key names
(for example
auth_token,tenant_id) to their caller. They neverreturn a stored value, and never return a connector's URL, headers,
environment variables, command, or authentication configuration.
Four groups of caller can reach these key names: the agent's owner; a
teammate the agent is shared with at the team level; any admin,
unconditionally, through a query path with no additional filter; and
any user a deployment's own policy implementation chooses to allow --
the default policy implementation in this repository allows no one, so
this group is empty by default, and its width is defined by the
deployment, not by this repository.
There is also a more direct path to the same information: any
logged-in user who can create a task against an already-published agent
becomes that task's owner on creation, and the task-keyed read
endpoint's ownership check passes for them without going through the
agent-keyed endpoint at all. This reuses task creation's existing
authorization behavior; this PR neither widens nor narrows it.
Key names are not a protected field: a connector's owner can write
anything into a key name, including a word like
passwordorapi_key, and the declared schema only validates a key name's syntax-- the system has no notion of a public or confidential key name.
Accepting this audience is a deliberate tradeoff: a caller who does not
know which keys to fill cannot use the connector in chat at all, which
is the problem this change exists to fix.
A connector that declares a required
secretsinput can never bereported as satisfied at this phase. No secret store exists yet, so
every
secrets(and, for an MCP connector,auth_selector) input isbuilt with
satisfied=Falseregardless of anything stored for thetask, and the report's top-level
satisfiedis the logical AND ofevery required input across every section. The practical effect: for
as long as a connector declares any required
secretsinput, thetop-level
satisfiedon both read endpoints and on the task-createresponse is always
false, andsecrets_expires_atis alwaysnull-- both are constants of this phase, not a bug in this PR. A later
phase that adds a real secret store gives both fields a real value
without changing their meaning or making either optional.
Testing
pytest tests/web/test_connector_runtime_entrypoints_e2e.py tests/web/api/test_websocket_preview.py -q-- 47 passed.ruff check .-- clean on the files this PR touches.