Skip to content

feat(web): accept connector runtime context values for a task - #2237

Open
AlexLiu190625 wants to merge 10 commits into
xorbitsai:mainfrom
AlexLiu190625:feat/connector-runtime-web-values
Open

feat(web): accept connector runtime context values for a task#2237
AlexLiu190625 wants to merge 10 commits into
xorbitsai:mainfrom
AlexLiu190625:feat/connector-runtime-web-values

Conversation

@AlexLiu190625

@AlexLiu190625 AlexLiu190625 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

What

A task's owner can now submit values for the runtime inputs a connector
declares, one key at a time, through a dedicated endpoint.

Why

Part of #1251. Depends on #2132, which is
already merged.

#2132 added two read endpoints and a task-create response
field: together they let a caller see which runtime inputs a connector
still needs. Nothing in the web chat path let the caller actually supply
them -- the only way to fill a value was to remove the connector and
start over.

Scope: no UI in this PR

The chat dialog that calls this endpoint ships in a following PR. This PR
is deliberately server-only, so that the endpoint's concurrency contract
and the browser-side interaction are reviewed separately.

This endpoint already has a concrete consumer: the dialog in that
follow-up PR. Its response shape and field types are the ones
#2132 fixed -- this endpoint returns that same model,
built by the same code. What this PR defines is the request shape and
the error codes the dialog will handle.

How

  • POST /api/chat/task/{task_id}/connector-runtime-values accepts
    context values for a task's connectors, merged key by key: a key not
    yet stored is written, one already stored with the same value is a
    no-op, one already stored with a different value fails the whole
    request (409, runtime_context_immutable). There is no override switch
    of any kind.
  • Task ownership is checked by the route before the service function is
    ever called, on the same predicate the task-keyed read endpoint applies
    -- the caller's own user id, with no admin exception -- so a task that
    does not exist and one that is not the caller's own answer the same
    404. Everything after that runs through a fixed order of validation
    checks: a non-empty request with no empty item, a size cap on each
    key's value and on the whole batch, no ref repeated in the same batch,
    every ref visible and selected for the task, every key declared and
    syntactically valid, every value matching its declared type and carrying content, and only
    then the per-key merge itself. A gate reserved for a future secrets
    section sits between type-checking and the merge and does nothing yet,
    because this request shape has no secrets field to check. This
    endpoint never asserts that a required key is present -- only that what
    was sent is valid and free of conflicts.
  • The connector scope this endpoint works in is the scope a turn would
    run in. The task's agent is resolved by the same two calls the per-turn
    tool build makes (resolve_workforce_task_runtime, then
    _load_agent_for_task_runtime), which is what the task-keyed read
    endpoint already does, rather than by loading the agent row directly.
    That matters here twice over: the resolved agent's team decides both
    what the response lists and which connectors a caller may write to at
    all. A connector reachable only through the team of a workforce manager
    agent whose run the runtime does not find is therefore neither writable
    nor listed, exactly as a turn would find it.
  • The report this endpoint returns follows the rule feat(web): report the runtime inputs a connector declares #2132
    established for the read endpoints, because it is built by the same
    code: the top-level satisfied is false whenever any listed key
    carries a name the per-turn gate would reject, independently of whether
    that key is required. A caller cannot read a 200 as "this task can
    now run".
  • Concurrency is closed with a compare-and-swap at the write: the update
    only takes effect if a row's content, as the database itself renders
    it to text, still matches what was read moments earlier. A miss is
    retried exactly once, after a full reread and remerge; a second miss
    gives up and returns a 503 with no named reason. A 503 that does carry
    a named reason (for example a team-scope resolution failure) means
    something else failed underneath, and retrying will not help -- that
    distinction is what tells a caller whether to retry.
  • Every failure -- a validation error, a stored-value conflict, or a
    losing compare-and-swap -- goes through the same
    {"error": {"code", "message", "details"}} envelope, not a plain-text
    detail string, so a caller gets a structured code and
    details.reason to decide how to recover.
  • On success, the response is the same requirements report the read
    endpoints return, built from the state the database holds right after
    the write -- not an echo of the request, and not whatever the read
    endpoints would have said before this call.
  • A successful write is recorded once in the application log, one line
    per connector that actually gained a key: the task id, the connector
    reference, a count of keys written, and the key names themselves --
    never a submitted value.

Disclosures

  1. There is no way to change a context value from web chat once it is
    stored.
    A key that already has a value is fixed for that task: a
    resubmission with the same value is a no-op, one with a different value
    is rejected. There is no override flag anywhere in the request shape.
    Changing a value means starting a new task. The case of a value that
    must rotate while the secrets section is still unsupported is tracked
    in Rotating connector runtime inputs have no repair path while the secrets section is unsupported #2249.

  2. This endpoint and the existing SDK append path are not shaped the
    same.
    Both hold "an already-stored value is never modified", but this
    endpoint accepts new keys through a per-key merge, while the SDK path
    compares the whole submitted dictionary against what is already stored.
    This is a pre-existing asymmetry between the two paths, and this PR
    does not change it. This asymmetry is tracked at
    prepare_append_connector_runtime rejects new runtime context keys on an existing binding #2140.

  3. Type validation against a connector's declared input schema only
    happens on this new endpoint.
    The existing SDK, /v1, and
    scheduled-trigger paths continue to accept values without checking
    their type against the declaration. This inconsistency is tracked at
    runtime_input_schema declares a type for every runtime input, but no validation path ever checks it #1874.

  4. The value table has no updated_at column, so a fill is recorded
    only in the application log, not in a column that can be queried later.
    Those log lines carry key names, the connector reference, and a count
    of keys written -- never a submitted value.

  5. This endpoint does not serialize concurrent requests; it performs a
    conditional update.
    Two windows submitting to the same connector at
    the same time may have one write rejected and retried automatically. A
    503 response with no named reason means the retry itself was exhausted
    -- a real collision between two writers, safe to retry. A 503 that does
    carry a named reason (for example a team-scope resolution failure)
    means something else failed underneath, and retrying will not help.

  6. This endpoint's error responses also carry key names. A conflict returns 409 with a reason of the form conflict.<section>.<key>,
    a type mismatch returns 400 with a reason of the form
    type_mismatch.<section>.<key>, and a value that is blank -- an empty or
    whitespace-only string, or an empty object -- returns 400 with a reason of
    the form empty_value.<section>.<key>. Both responses go only to the caller
    who already passed the task-ownership check, and that caller can
    already read the same key names from the read endpoints introduced in
    feat(web): report the runtime inputs a connector declares #2132, so this adds no new exposure. The details
    object on that 409 also carries the connector_ref of the connector
    the conflict is about; that ref is already part of every read
    endpoint's own response, so it is the same information the caller can
    read there. This is a separate channel from the broadcast failure
    event every connection under a task receives, including anonymous
    widget and share-link visitors; that broadcast carries neither a key
    name nor a connector_ref.

  7. Known limit: a connector that also declares a required secrets
    input can have its context values stored through this endpoint, but a
    chat turn against it will still fail
    -- support for secrets ships
    in a later PR. The failure is a specific, readable error, not an
    unreadable one. The value endpoint itself does not check whether a
    connector's full set of requirements is satisfied; it only checks
    whether the values it was sent can be merged in.

  8. A connector whose owner declared a key with a name the runtime
    rejects leaves a form nobody can complete.
    The requirements report
    lists such a key, so a client will render a field for it; this endpoint
    validates every submitted key with the same validator the per-turn gate
    uses and answers 400 for that one. The report already refuses to call
    the task satisfied while such a key is declared, so nothing here
    reports success falsely -- but the key cannot be filled from any client
    either. The real fix is validating key syntax when a connector is
    created or updated, which is outside this PR.

  9. The report this endpoint returns does not consult a runtime resolver
    hook a deployment may have installed
    , because it is the same report
    the read endpoints build; on such a deployment it can report a task
    unsatisfied while its turns run fine. See Connector runtime requirements report ignores the runtime resolver hook #2176.

  10. The two request size caps count a value in its stored form, not by
    its raw UTF-8 length.
    Both measure json.dumps(value) with the
    default ensure_ascii=True, so a character outside ASCII counts as
    the six-character escape sequence JSON writes it as. That is the same
    form SQLAlchemy's JSON column puts in the row, and the same text the
    conditional update compares against, so the caps bound what the row
    will actually hold. A value made of CJK characters reaches a cap at
    roughly half the character count an ASCII value does, and one made of
    emoji at roughly a third.

Testing

  • pytest tests/web/test_connector_runtime_entrypoints_e2e.py -q -- 93
    passed.
  • pytest tests/web -q -- 9881 passed, 257 skipped, no failures.
  • ruff check and ruff format -- clean on the four files this PR
    touches; mypy (the repo's --package xagent gate) -- clean on the
    three source files.
  • The compare-and-swap on POST .../connector-runtime-values is exercised
    here on SQLite, in tests/web/test_connector_runtime_entrypoints_e2e.py,
    which runs on every PR. The PostgreSQL half of the same proof -- the two
    cases that depend on the database's own text rendering of the JSON
    column -- is a separate follow-up PR that adds a Postgres-only test file
    and wires it into the Test Database Migrations workflow; it depends on
    this PR and opens once this one merges, tracked in Add PostgreSQL-backed coverage for the runtime-values conditional update #2269.

A task owner can now supply the runtime context values their connectors
declare. Values merge key by key: a key the task does not have yet is
written, a key already stored with the same value is a no-op, and a key
already stored with a different value fails the whole request. A stored
value is never replaced, and the request body has no override switch.

Concurrency is closed at the write itself with a compare-and-swap on the
value the row held when it was read, so two windows filling the same
connector cannot lose either one's keys on either database backend. The
expected old value is the text the database itself renders, never a
re-serialization, so a row written by anything other than this path stays
writable. The PostgreSQL half of that proof lands in a follow-up change
that wires a Postgres-only test file into the migration workflow.

Submitted values never reach a log line: the logs carry key names, the
connector ref and counts, and a sentinel-value test keeps it that way.
A deployment-side failure surfaces as the 503 its typed exception already
carries rather than being flattened into a 400.
The endpoint that accepts connector runtime context values loaded the
task's agent with a direct row query, while the task-keyed requirements
read endpoint resolves it through resolve_workforce_task_runtime and
_load_agent_for_task_runtime -- the same pair the per-turn tool build
uses. The two therefore derived different connector team scopes for the
same task: a connector shared only through a workforce manager agent's
team was writable here, and listed in this endpoint's own report, even
where a turn resolves that agent to None and never reaches it.

Use the same two calls here. The scope on this endpoint decides not only
what the response lists but which connectors a caller may write to at
all, so a ref outside the turn's scope now answers the same uniform 404
as any other invisible ref.

Also pin three properties of the endpoint that had no test:
- a declared key whose name the per-turn gate rejects holds the 200
  body's top-level satisfied at false, even when it is optional and the
  submitted key was written;
- an admin posting to another user's task gets the same "Task not found"
  404 the task-keyed read endpoint gives, with the owner's 200 as the
  control;
- the 200 body's constant wire fields -- expired, secrets_expires_at and
  the section every context input carries.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new POST endpoint /task/{task_id}/connector-runtime-values to accept and merge caller-supplied context values for a task's connectors. It implements a robust multi-tier validation process (including size caps, duplicate checks, schema validation, and type validation) and a compare-and-swap mechanism to handle concurrent writes safely. Additionally, extensive end-to-end and concurrency tests are added. Feedback suggests defining the new endpoint as synchronous (def) instead of asynchronous (async def) since it performs blocking database operations, which would otherwise block FastAPI's event loop.

Comment thread src/xagent/web/api/chat.py Outdated
`post_task_connector_runtime_values` performs only synchronous work: it
queries and commits through the request-scoped Session and reaches the
connector visibility hook an embedder may install. Declared `async def`,
all of that ran on the event loop thread, stalling every other request the
process was serving while a query or the hook was in flight.

Declare it as a plain `def` so FastAPI dispatches it to the threadpool.
This follows the boundary set for synchronous routes in xorbitsai#965; moving the
body to a worker thread instead is not an option here because the Session
is request-scoped and must not cross thread ownership.

The two read-only runtime-requirements endpoints share this shape and are
tracked in xorbitsai#2192; they are left unchanged here.
@AlexLiu190625

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new POST endpoint /task/{task_id}/connector-runtime-values that allows callers to submit context values for a task's connectors. The values are merged key-by-key without replacing existing values. The implementation includes robust request validation, size caps, duplicate ref checks, type checking, and a compare-and-swap (CAS) mechanism to handle concurrent writes. Comprehensive E2E tests have been added to cover validation failures, concurrency races, and team-shared connectors. There are no review comments to address, so I have no feedback to provide.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical

  • src/xagent/web/services/connector_runtime.py:1346-1352_validate_context_value_types accepts empty/whitespace-only strings and empty objects for required keys; combined with immutable-once-set storage (_merge_context_values, ~L1371) and presence-only satisfaction checks (_all_required_inputs_satisfied, ~L1186), a blank submission permanently satisfies a required key while satisfied reports true, with no repair route. Trigger: POST .../connector-runtime-values with {"auth_token": ""} on a required key; impact: permanent silent breakage plus a false satisfied: true in the API's own success contract. Reject blank/whitespace-only strings and empty dicts in _validate_context_value_types.

Major

  • src/xagent/web/services/connector_runtime.py:1638-1639_canonical_json round-trips through json.loads, so the no-op/immutability comparisons (~L665, ~L1371) use plain Python ==, under which 1 == 1.0 == True. Trigger: resubmit an object-typed value differing only by JSON numeric/boolean coercion (e.g. {"a": 1} vs {"a": 1.0}); impact: silently treated as a no-op instead of the conflict the immutability contract is supposed to raise, no write and no error. Compare the canonicalized JSON strings directly instead of decoding back to Python objects.
  • src/xagent/web/services/connector_runtime.py:1415-1435 — The CAS mechanism's Postgres-specific cast-to-TEXT path (needed because json has no equality operator per its own docstring) is untested; the new suite only runs against a SQLite e2e_db fixture and isn't wired into the Postgres CI job. Add Postgres-backed coverage before this ships to a Postgres deployment.
  • No route updates or deletes a single stored task_connector_runtime_contexts key; values are immutable forever (only whole-task deletion via FK cascade removes them). Runtime-input keys can be credential-shaped (per PR #2132), so a typo'd or rotated value permanently breaks the connector with no repair path short of deleting the task.
  • No test asserts the top-level satisfied field becomes true once all required runtime inputs are filled via this endpoint; existing assertions only check per-key satisfaction or satisfied: False.

Minor

  • src/xagent/web/services/connector_runtime.py:590-710 — The hand-rolled CAS/retry/reread machinery in apply_task_connector_runtime_context_values defends against contention that can only occur between two concurrent calls to this same endpoint, whose benign outcome the immutability rule already guarantees. SELECT ... FOR UPDATE on the already-loaded task row, or SQLAlchemy's version_id_col, would be simpler and would also supply the separately-missing updated_at audit column.
  • The CAS-miss retry-exhaustion branch's HTTP 503 response body (details shape) is never asserted at the HTTP level, unlike its IntegrityError sibling branch.
  • src/xagent/web/services/connector_runtime.py:1294_reject_oversized_context_payload measures size via json.dumps(value) with default ensure_ascii=True, so non-ASCII text is measured by its escaped length, roughly halving the effective cap for CJK/emoji content; not documented. Pass ensure_ascii=False.

Simplification

  • src/xagent/web/services/connector_runtime.py:1051_validate_payload_refs only iterates dict keys, never reads .context/.secrets/.auth_selector; building a full ConnectorRuntimePayload per ref is unnecessary. Pass dict.fromkeys(payload_by_ref) or change the signature to accept Iterable[ConnectorRef].
  • src/xagent/web/services/connector_runtime.py:679-709 — The CAS-miss branch and the IntegrityError branch are byte-identical except for the trigger (same rollback, same 503-on-first-attempt, same refetch-and-continue). Extract one helper, e.g. _retry_or_503(attempt, connector_user_id, agent_team_id) -> dict[ConnectorRef, Any], called from both branches.
    net: -10 lines possible

Blocking: yes — recommended event: REQUEST_CHANGES

The values endpoint checked a submitted value's type but not whether it
held anything, so an empty or whitespace-only string, or an empty object,
was stored like any other value. A stored value is never replaced, so that
blank marked its key filled for the life of the task while carrying
nothing a connector could use, and the requirements report called the task
satisfied.

Reject a blank value where the type is checked, for optional keys as well
as required ones, with a 400 whose reason reads
`empty_value.<section>.<key>`. The per-turn gate, the requirements report,
and the task-create path keep their current behaviour.
The no-op check and the per-key immutability check both decoded each side
back into Python objects before comparing them, and Python reads 1, 1.0
and True as equal. Resubmitting an object-typed value that differed only
in JSON form was therefore answered 200 with nothing written, where the
immutability contract calls for a 409.

Add a canonical-text helper and compare through it at both sites. The
helper that feeds the create path keeps returning a dict, since that value
is written to the column directly.
Three gaps in the values endpoint's suite: nothing asserted the top-level
satisfied flag turns true once every required key is filled through this
endpoint, and neither branch that exhausts the bounded retry asserted its
503 response at the HTTP layer -- the conditional-update branch had no
coverage at any layer.
_validate_payload_refs reads nothing but the refs it iterates, so the
values endpoint built a payload object per ref purely to call it. Widen
the parameter to an iterable of refs; the create and append callers pass
their existing mappings unchanged.

Both ways a write is lost inside the bounded retry loop -- a conditional
update that matched no row, and an IntegrityError on a first-ever row --
were recovered by two identical blocks. Extract one helper for them.
The per-key and whole-batch caps measure a value as json.dumps renders it
with ensure_ascii on, which is the form the JSON column stores and the
text the conditional update compares against. Non-ASCII text therefore
reaches a cap at roughly half, or a third, of the character count ASCII
text does. Say so where the caps are enforced.
@AlexLiu190625

Copy link
Copy Markdown
Collaborator Author

Thanks for the review -- all ten items are addressed below; fixes are in c27f081, ca8bf5a, f712150, 4170fe6, and f04f7d8.

1.

_validate_context_value_types accepts empty/whitespace-only strings and empty objects for required keys; combined with immutable-once-set storage ... and presence-only satisfaction checks ..., a blank submission permanently satisfies a required key while satisfied reports true, with no repair route.

Fixed in c27f081. _validate_context_value_types now rejects a blank value right after the type check:

        blank = not value if declared_type == "object" else not value.strip()
        if blank:
            _raise_runtime_error(
                ERROR_INVALID_RUNTIME_CONTEXT,
                ref,
                reason=f"empty_value.{RUNTIME_INPUT_CONTEXT}.{key}",
            )

An empty or whitespace-only string and an empty object both fail with 400 and reason empty_value.context.<key>, for optional keys as well as required ones -- a stored value is never replaced, so a blank one would be permanent regardless of whether the key is required.

This is deliberately scoped to the write path this PR adds. The per-turn gate (_require_context_values), the requirements report's satisfied computation, and the task-create/append paths are unchanged: they keep their presence-only semantics by design, so the create path can still store a blank value. This PR closes the one entry point it introduces, not the others.

Tests cover a blank string in three forms, an optional key, an empty object, and the recovery path: a rejected blank does not block a later real value, after which top-level satisfied is true.

2.

_canonical_json round-trips through json.loads, so the no-op/immutability comparisons use plain Python ==, under which 1 == 1.0 == True. ... impact: silently treated as a no-op instead of the conflict the immutability contract is supposed to raise, no write and no error.

Fixed in ca8bf5a. A new _canonical_json_text returns the canonical JSON text, and both comparison sites now compare that text instead of decoding back to Python objects:

    return json.dumps(value, sort_keys=True, separators=(",", ":"))

used at the no-op check and at the per-key conflict check, so 1, 1.0, and true are distinct and a resubmission differing only in JSON form is now a 409, not a silent no-op. _canonical_json_value still returns a dict, since the create path stores its result directly.

This also corrects something we said on #2132: comparing after a JSON round-trip does not answer "would storing this change the stored JSON" -- Python's == unifies those values, which is exactly the bug here. The text comparison answers that question correctly.

A test for this exact case (an object-typed value resubmitted with a JSON-form-only difference) was added.

3.

The CAS mechanism's Postgres-specific cast-to-TEXT path ... is untested; the new suite only runs against a SQLite e2e_db fixture and isn't wired into the Postgres CI job.

Not addressed in this PR; a follow-up PR is ready to open once this one merges. The PostgreSQL-only test file and the CI wiring live on a separate branch so this PR stays scoped to the write path itself.

Worth flagging so the green check here isn't misread as coverage: the Test PostgreSQL Migrations job in this PR's CI run does not include this PR's test file in the list it runs, so that check passing is not evidence this path has Postgres coverage.

4.

No route updates or deletes a single stored task_connector_runtime_contexts key; values are immutable forever ... Runtime-input keys can be credential-shaped ..., so a typo'd or rotated value permanently breaks the connector with no repair path short of deleting the task.

Kept as designed; the gap is tracked in a new issue rather than fixed here. Immutability of a context value is the intended contract -- a stored key is never replaced, and changing it means starting a new task -- and the design's home for a value that needs to change over time is the secrets section, not context. The real gap is that secrets support isn't shipped end to end yet, so today a value that needs to rotate has no working home at all. Adding a mutation path to context here would make this endpoint the only one of the three entry points able to change an already-stored value, which is a bigger change than this finding calls for. The window is tracked in a new issue instead: #2249.

5.

No test asserts the top-level satisfied field becomes true once all required runtime inputs are filled via this endpoint; existing assertions only check per-key satisfaction or satisfied: False.

Test added in f712150: filling every required key through this endpoint asserts the write response's top-level satisfied is true, and a subsequent GET of the same task confirms it.

6.

The hand-rolled CAS/retry/reread machinery ... defends against contention that can only occur between two concurrent calls to this same endpoint, whose benign outcome the immutability rule already guarantees. SELECT ... FOR UPDATE on the already-loaded task row, or SQLAlchemy's version_id_col, would be simpler and would also supply the separately-missing updated_at audit column.

Not adopted. The premise that concurrent outcomes are benign doesn't hold: two callers writing different new keys are both individually legal, and a read-modify-write of the whole JSON column would let the second writer silently overwrite the first's key (a lost update). The existing test test_concurrent_overwrite_of_an_existing_row pins exactly this case, asserting update_results == [False, True] rather than both succeeding.

SELECT ... FOR UPDATE is a no-op on SQLite -- this repo's own comments on that pattern elsewhere (workforces.py, mcp.py) say so directly -- and the e2e suite this endpoint is tested under runs on SQLite, so that suggestion would silently stop testing what it claims to guarantee. version_id_col has no precedent anywhere in this repo. The missing updated_at column is correct and is already listed in the PR's disclosures.

7.

The CAS-miss retry-exhaustion branch's HTTP 503 response body (details shape) is never asserted at the HTTP level, unlike its IntegrityError sibling branch.

Tests added in f712150 for both retry-exhaustion branches (a conditional update that matches no row, twice; an insert that collides, twice), each asserting the full 503 response body and the attempt count at the HTTP layer.

One correction on the framing: the IntegrityError branch had no HTTP-level assertion either before this -- both branches were previously only covered at the service layer, and the conditional-update branch had no coverage at any layer. The gap was larger than "one sibling branch is behind."

8.

_reject_oversized_context_payload measures size via json.dumps(value) with default ensure_ascii=True, so non-ASCII text is measured by its escaped length, roughly halving the effective cap for CJK/emoji content; not documented.

Documented rather than changed, in f04f7d8. The measurement is intentional: it counts the value in the exact form it is stored in and compared in -- json.dumps with ensure_ascii=True is the same escaping SQLAlchemy's JSON column writes to the row, and the same escaping the conditional update's text comparison sees. Switching to ensure_ascii=False would let a value through the cap that is 2-3x the size of what the column actually ends up holding, which is the wrong direction for a size cap to move. The docstring now states this explicitly, and the PR description has a new disclosure covering the CJK/emoji effect.

9.

_validate_payload_refs only iterates dict keys, never reads .context/.secrets/.auth_selector; building a full ConnectorRuntimePayload per ref is unnecessary.

Fixed in 4170fe6. The signature now takes Iterable[ConnectorRef] directly, and the call site no longer builds a temporary payload object per ref -- all three callers (this endpoint, create, append) pass their existing ref-keyed mappings unchanged.

One note on the suggested alternative: dict.fromkeys(payload_by_ref) against the old signature does not pass this repo's mypy gate (dict[ConnectorRef, Any | None] isn't accepted where dict[ConnectorRef, ConnectorRuntimePayload] is declared), which is why the signature itself was widened instead.

10.

The CAS-miss branch and the IntegrityError branch are byte-identical except for the trigger ... Extract one helper, e.g. _retry_or_503(attempt, connector_user_id, agent_team_id) -> dict[ConnectorRef, Any], called from both branches.

Fixed in 4170fe6, using the name suggested. The helper's signature also takes db, since it needs to call db.rollback() and reload the visible connector set; both call sites now pass identical arguments. The two original blocks were not quite byte-identical -- one carried a comment explaining the rollback/refetch reasoning -- that comment now lives in the helper's docstring instead of being duplicated at both call sites. Behavior is unchanged.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major

src/xagent/web/services/connector_runtime.py:1395-1447 — the CAS turns on CAST(context AS TEXT) text equality, and src/xagent/models/task.py:692 declares context = Column(JSON, ...) with no .with_variant(JSONB, "postgresql") (unlike TRACE_PAYLOAD_JSON at src/xagent/models/task.py:41). That is correct only while the Postgres column stays plain json (no input normalization) and SQLAlchemy is the sole writer; a later JSONB migration — already precedent here via #1248 — would silently break the equality check, and no Postgres-backed test guards it (the Test PostgreSQL Migrations job's file list does not include this PR's test file, and no tracking issue is cited for the promised follow-up). Add a version INTEGER optimistic-lock column and compare on that instead: dialect-independent, testable on SQLite, and it supplies the audit column currently disclosed as missing.

src/xagent/web/api/chat.py:5479-5488 — the try/except ConnectorRuntimeError covers neither resolve_workforce_task_runtime/_load_agent_for_task_runtime at :5479-5480 (a bare KeyError/TypeError from unguarded snapshot subscripting in src/xagent/web/services/workforce_snapshot.py:182-199, or anything an installed get_agent_team_scope hook raises, src/xagent/web/services/agent_team_scope.py:95) nor db.commit() at :5488; inside the service only IntegrityError is caught (src/xagent/web/services/connector_runtime.py:676-680), so an OperationalError/DataError mid-write escapes too. All three yield a bare 500 — the global handler re-raises for this path (src/xagent/web/app.py:1030-1078) — instead of the documented {"error":{code,message,details}}, under reachable conditions (schema skew, flaky DB, third-party hook). Widen the guard over the whole request path and roll back explicitly on commit failure.

src/xagent/web/api/chat.py:5309-5332 — internal ERROR_* module constants (src/xagent/core/tools/adapters/vibe/connector_runtime.py:48-57) are emitted verbatim as the wire code with no remapping, unlike the /v1 twin (src/xagent/web/api/v1/tasks.py:307-317) which re-parses through V1ErrorCode with a safe fallback. Renaming an internal constant then becomes a silent breaking change for any client switching on code. Route this envelope through an explicit wire-code enum as well.

src/xagent/web/services/connector_runtime.py:636-680 — the write loop iterates payload_by_ref in request-payload order, while the sibling bind_connector_runtime_selection_snapshot (:712-714) already imposes canonical order through _sort_connector_refs (:1017) for exactly this multi-row-write case. Two concurrent same-task requests touching the same 2+ rows in opposite order can deadlock on Postgres, and the resulting OperationalError is not caught (:22,681), so it escapes as a 500 rather than the endpoint's designed 503. Call _sort_connector_refs before the loop.

Minor

src/xagent/web/services/connector_runtime.py:598-611 — the documented validation order is not what runs: ConnectorRef.from_wire inside _parse_context_value_items (:1241) precedes the empty-item check (:606), so a garbage connector_type plus an empty context answers 404 connector_not_found rather than 400 empty_item_payload; and the size cap (:610, over the undeduplicated list) precedes the duplicate-ref check (:611), so the same ref twice with individually-legal values that only breach the aggregate answers payload_too_large rather than duplicate_ref. Neither path is tested.

src/xagent/web/services/connector_runtime.py:1471-1476, :728-733, :1622-1627 — all three 503 sites carry an identical code and message, so a client's only retry/don't-retry signal is whether details.reason exists; anyone later adding a reason to the exhaustion path silently flips client behavior with no test failing. Give the CAS-exhaustion case its own code.

src/xagent/web/services/connector_runtime.py:1283-1298 — the per-key cap measures json.dumps(value), which includes the string's own wrapping quotes, so the true ASCII ceiling is 65534 characters rather than the 65536 the constant name implies (and no test sits on the nominal cap). The aggregate cap sums value bytes only, never key names or JSON structure: with unbounded key length (^[A-Za-z0-9_-]+$, src/xagent/core/tools/adapters/vibe/connector_runtime.py:29) and a free-form runtime_input_schema (src/xagent/web/api/mcp.py:119-145), a self-registered connector can carry a request well past 256KB while passing the check.

src/xagent/web/schemas/connector_runtime.py:25-29,136ConnectorRuntimeRefModel lacks model_config = ConfigDict(extra="forbid") that both sibling models carry, so a typo'd field nested in connector_ref is silently dropped instead of 422; connector_type: str should be Literal["mcp","custom_api"], connector_id: int accepts "7" under lax coercion, and items has neither min_length=1 nor an upper bound, leaving the empty list to a service-level check.

tests/web/test_connector_runtime_entrypoints_e2e.py:4092-4141,4144-4191 — both 503-exhaustion tests monkeypatch away the mechanism under test (_update_context_row forced to False; _load_task_context_row_snapshots forced to {}), so a broken CAS WHERE clause would pass both. test_concurrent_first_write_has_one_winner and test_concurrent_overwrite_of_an_existing_row do drive the real path and partly mitigate this, but at least one exhaustion test should exercise the real predicate or a real IntegrityError.

tests/web/test_connector_runtime_entrypoints_e2e.py — this endpoint's section has no coverage for: items: [] and an empty per-item context; the connector_not_selected 400 branch; an unrecognized connector_type or non-integer/non-positive connector_id; one request writing 2+ distinct connectors successfully (every multi-ref test expects failure); a connector also declaring required secrets; the runtime-resolver deployment hook; any custom_api connector (all tests use "mcp"); and the 422 body shape.

src/xagent/web/services/connector_runtime.py:636,1471 — the retry bound is split across two unconnected literals (range(2) in the caller, attempt == 1 in _retry_or_503), so changing the retry count means editing both in sync. Extract a shared _MAX_RETRY_ATTEMPTS = 2.

Simplification

L1692: dead-indirection _canonical_json is a one-line pass-through with a single caller. Inline it so _canonical_json_value returns json.loads(_canonical_json_text(value)).
L1239: redundant-validation ConnectorRef.from_wire(item.connector_ref.model_dump()) re-checks what the schema layer already covers. Construct ConnectorRef directly behind one ALLOWED_CONNECTOR_TYPES membership check.
L123 (src/xagent/web/schemas/connector_runtime.py): pointless-optional context: dict[str, object] | None = None plus the item.context or {} fallback (:1256) — absent, None and {} all reject identically. Make it required and drop the fallback.
L5309 (src/xagent/web/api/chat.py): duplicate-shape _connector_runtime_error_response rebuilds field-by-field what exc.to_public_error() already returns 1:1, for one call site. Inline as JSONResponse(exc.status_code, {"error": exc.to_public_error()}).
L633: dead-init rows/merged_by_ref/written_keys_by_ref are reassigned on every path before any read. Delete the three pre-loop assignments.
L1238: dead-branch for item in payload_items or () on a required field. Use for item in payload_items (the same shape in _parse_payload_items is load-bearing — leave it).
L528: over-documentation ~60-line docstring covering unimplemented tiers, plus the tier-7b secrets-gate entry at :556-558/:627-630 that has no backing code. Trim to the tier order and the "never commits" contract; re-add when secrets ships.
L2881 (test file): unused-param _setup_context_task's server_name is passed by none of its ~17 call sites. Drop it and inline the default.
L3101 (test file): unused-arg three payload_builder lambdas ignore their server_id. Store plain dicts and drop the indirection at :3136.
L4352 (test file): duplicate-test test_concurrent_first_write_retry_is_bounded is a strict subset of :4144. Delete it.
L3540 (test file): duplicate-test the standalone 409 case is already step 2 of :3390. Move its details["connector_ref"] assertion there and delete it.
net: -180 lines possible

Blocking: no — recommended event: APPROVE

The connector-runtime values endpoint guarded only the service call, so
two things outside that guard could end a request with the batch already
flushed into an open transaction: the agent/workforce resolution that
runs before the write, and `db.commit()` itself.

Widen the guard over the whole path. A `ConnectorRuntimeError` still
answers with the documented error envelope, and now does so wherever on
the path it is raised. Anything else is rolled back and re-raised
unchanged, so it still surfaces as a bare 500 rather than being dressed
up in an envelope whose `code` promises the caller a condition they can
act on.

Add an end-to-end test that makes the commit raise and asserts the
explicit rollback runs and nothing of the batch is left in the table.
The values endpoint's write loop walked the batch in the order the
request listed its connector refs, so two concurrent requests naming the
same two rows in opposite order could take those rows' locks in opposite
order and deadlock on PostgreSQL. The resulting error is not one this
path catches, so it would escape as a 500 rather than the 503 the
endpoint designs for a lost race.

Issue the write in the canonical ref order `_sort_connector_refs`
already establishes for the task's selection snapshot. Validation still
walks the batch as it arrived, so which of several bad refs is reported
does not change.

Add an end-to-end test that submits two connectors in reverse canonical
order and asserts the conditional updates go out in canonical order.
@AlexLiu190625

Copy link
Copy Markdown
Collaborator Author

Two small follow-ups pushed after the approval, both from the non-blocking notes:

  • 0aa7e79 — the endpoint now keeps the runtime resolution and the commit inside its error handling: a ConnectorRuntimeError anywhere on the path returns the documented envelope, and any other failure (including a failed commit) rolls back before propagating, so a failed request never leaves a partially written row. Test added.
  • e3f0a7b — the write loop updates rows in the canonical ref order from _sort_connector_refs, the same order the selection snapshot and visible-connector map already use, so two concurrent requests touching the same rows acquire locks in the same order on PostgreSQL. Test added.

The PostgreSQL-backed coverage for the conditional update is now tracked in #2269, and the rotating-value window from the earlier discussion in #2249; both are referenced from the description. The remaining suggestions (version column, wire error codes, validation order, the simplifications) are noted for a follow-up rather than this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants