feat(connectors): add an optional team access hook to the connector seam - #1912
Conversation
Standalone xagent keeps MCP servers and custom APIs user-owned. The connector team scope module already lets an application install hooks for delete, rename and visibility; this adds the remaining slot, which answers whether a caller's team links a given connector and whether it may edit it. The new access hook is batched: one call answers a whole collection of (connector_type, connector_id) refs, so a caller never pays one hook call per connector. A ref the caller's team does not link is expressed by leaving it out of the answer map, never by a verdict carrying team_owned=False, and the answer is shape-validated as an authorization input -- a malformed answer raises rather than being coerced or defaulted to empty. Hook invocation is consolidated behind a single internal door so that a hook which leaves a failed statement on the shared session has that session rolled back before the exception reaches the caller. The validation step runs inside the same guard, because a hook can poison the session without raising: fail a statement, swallow that itself, and return a shape this seam then rejects. All five slots now go through that door. A snapshot context manager saves and restores every hook slot, so a test that installs one restores whatever the process had rather than clearing to empty. A discovery-based test enumerates the module's hook globals and asserts the snapshot restores each by identity, so a slot added later that is not snapshotted fails the suite. With no hook installed the module returns empty answers without querying, so a standalone deployment sees no behavior change.
![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 a new ConnectorAccess hook to manage connector team ownership and edit permissions, centralizing hook invocation and session restoration through a new _call_connector_hook_gate function. The changes include robust validation for hook responses and comprehensive testing for session state management. Feedback identifies a potential resource leak in the test fixture's cleanup logic and suggests an optimization to avoid redundant normalization of connector references in the error-handling path.
resolve_connector_access normalized its refs before checking whether an access hook was installed, so a deployment with no hook installed could still be made to raise by a malformed ref -- breaking the seam's first invariant, that standalone xagent behaves identically when no hook is installed. Decide the no-hook case first; the empty-refs check stays after normalization, where it belongs, matching team_connector_ids, which already returns before coercing its team_id. Malformed refs still surface as the ValueError/TypeError they are rather than as the seam's retryable 503: a ref that is not a (str, int-coercible) pair is a defect in the calling route, not an outage of the installing application, and converting it would point an operator at the wrong system. resolve_connector_access_or_raise's docstring promised the opposite; correct it to describe the boundary the code actually draws, and correct its account of the warning log, which prints the normalized tuples rather than the ones the caller passed in. Cover both directions: malformed refs raise raw through the batch entry point and the single-ref wrapper, well-formed refs plus a failing hook still produce the one typed 503, and the no-hook path reads no ref at all. Release the agent-team hook from a finally in the test file's reset scope, so an exception inside a direct with block cannot leak it.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook seam (ConnectorAccess and ConnectorAccessHook) to resolve and validate team ownership and edit permissions for connectors in batches. It also unifies hook execution through a central gate (_call_connector_hook_gate) that automatically rolls back the database session upon hook failures or validation errors. Comprehensive unit tests are added to verify the validation rules, error handling, and session restoration. Feedback suggests refactoring the inline normalization of connector references in resolve_connector_access to reuse a shared normalization helper.
The access seam normalized its ref batch inline in two places with a byte-identical expression. The normalized set is not merely an argument: it is also the baseline the hook's answer is validated against -- every key the hook returns must be a member of it. Two separately written normalizations could drift, leaving the question asked and the yardstick its answer is measured by out of step with nothing failing to say so. Naming the operation once makes that impossible structurally. Behavior is unchanged. The call in resolve_connector_access stays below the no-hook early return, so a deployment with no hook installed still reads no ref at all; the call in resolve_connector_access_or_raise stays outside the try, so a ref the caller built wrong still surfaces as the TypeError/ValueError it is instead of the seam's retryable 503. The normalizer documents that it deliberately does not tolerate an id it cannot coerce -- dropping one would turn a caller's bug into a connector the hook was never asked about -- and records that only the id half is checked at run time: the connector type is carried by the annotation, so a non-str type reaches the hook as passed in. Also corrects a false explanation in the malformed-ref test: the outer resolver normalizes before it consults the hook slot, so it raises with or without a hook installed. The hook is installed there to give the trailing "no hook call" assertion its meaning, not to keep the check from passing vacuously. Siblings: the two normalization sites in this module are the only ones; grep for the expression across src/ and tests/ returns nothing else. The int() coercion in web/services/connector_runtime.py builds a different ConnectorRef dataclass from an ORM row rather than a caller-supplied batch, and is left alone. knowledge_base_team_scope.py's resolver takes a single object and has no ref batch to normalize. The single-ref test's docstring next to the corrected one was checked line by line and makes no false claim, so it is left unchanged.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook mechanism, including the ConnectorAccess dataclass and validation logic to determine team ownership and edit permissions for connectors. It also implements a unified hook invocation gate (_call_connector_hook_gate) that ensures database sessions are safely rolled back upon hook failure. Feedback on the changes highlights a potential TypeError when sorting requested refs in a logging statement if they contain uncomparable types, which could mask the original exception; using a fallback key like key=str is recommended.
The failure arm logs sorted(requested), and the refs a caller hands in are only type-checked statically. A mixed-type collection therefore reaches the handler intact, where a bare sorted() raises TypeError from inside the except block -- replacing the seam's one typed 503 with an untyped error and discarding the hook failure that caused it. Sorting with key=str keeps the arm total.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook mechanism to manage team ownership and edit permissions for connectors, along with a centralized gate function to handle hook execution and database session restoration on failure. Additionally, it updates the test suite to use a new snapshot context manager for cleaner hook teardown. The feedback suggests optimizing resolve_connector_access_or_raise to avoid redundant normalization overhead and double normalization on the happy path when no hook is installed.
… about
resolve_connector_access_or_raise normalized refs before delegating, so a
deployment with no access hook installed still coerced every id and could
raise on a malformed one -- while resolve_connector_access, whose gate runs
before its own normalization, returned {} for that same input. The two
public entry points of one seam disagreed for the same refs.
Gate this entry point the same way, above the normalization that is itself
deliberately above the try, so a standalone deployment reads no ref at all
through either door. The normalization stays outside the try, so an
un-coercible id still surfaces raw once a hook is installed.
The docstring paragraph describing that boundary was true only of the id
half of a ref and only of a deployment with a hook installed; it is rewritten
into three paragraphs that each hold: the no-hook short circuit, the raw-error
boundary for un-coercible ids, and the connector type, which the annotation
carries and nothing re-checks at run time.
Sibling sites: resolve_connector_access already had the gate; the
team-visibility slot reaches the same decision through
team_connector_hook_installed(). Those are the only hook slots in the module.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new ConnectorAccess hook mechanism to manage and validate team-level access and edit permissions for connectors, including validation helpers, session recovery logic, and a batch resolver. Comprehensive unit tests have been added to verify the new hook, validation rules, and session restoration behavior. The review feedback suggests a more idiomatic and concise type check for the can_edit field using isinstance.
…dentity tests
`_validate_connector_access_answer` required `can_edit` to be exactly
`True` or `False` by testing both identities. `isinstance(verdict.can_edit,
bool)` states the same rule directly: `bool` cannot be subclassed and has
exactly two instances, so the two forms accept and reject the same values.
Probed over True/False/1/0/1.0/"x"/None/Decimal("1") -- identical verdicts
on all eight.
It also makes the intent readable next to the line above it. The two
adjacent checks look inconsistent but are not: `team_owned` admits one
specific value and stays an identity check, while `can_edit` admits either
bool and is a type check. The docstring now says that instead of covering
both under one "identity check, not a truthiness check" phrase, which
described only half the pair.
The raised message is unchanged on purpose: it is what tells a hook author
why `1` is refused, and dropping that sentence would remove the only place
the bool-is-a-subclass-of-int reason is stated to the caller.
Sibling sites in this module, all enumerated:
- `_validate_connector_access_answer:379` `team_owned is not True` -- left
as is. `isinstance(..., bool)` would accept `team_owned=False`, which is
the exact answer shape this line exists to reject.
- `:360` connector id and `:228` team-member id already use
`isinstance(x, bool) or not isinstance(x, int)`; unchanged, and this edit
makes the module use one idiom for type checks throughout.
- `git grep "is not True and"` over `src/` returns no other occurrence, so
there is no fourth site.
Behavior is unchanged, so no test is added; the existing pins already cover
both directions -- `test_resolve_connector_access_rejects_a_can_edit_that_is_not_exactly_bool`
("false", 1, 0) and `test_resolve_connector_access_accepts_linked_but_not_editable`
(can_edit=False).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook (_connector_access_hook) along with the ConnectorAccess dataclass to manage team-level access and edit permissions for connectors. It adds robust validation, normalization, and error-handling wrappers, including a unified hook execution gate (_call_connector_hook_gate) that ensures database session rollbacks on failures. The feedback highlights a performance improvement opportunity to avoid redundant normalization of connector references by extracting the core logic of resolve_connector_access into an internal helper that accepts pre-normalized references.
…tion
`resolve_connector_access_or_raise` normalized `refs` above its `try`, then
called `resolve_connector_access`, which normalized the result a second
time. Measured before the change, with one call carrying
`[("mcp", "5"), ("mcp", 5)]`: the raising entry point invoked
`_normalize_connector_refs` twice, the second time over the already
normalized and deduplicated one-element set. Idempotent and never larger
than the first pass, but work no caller asked for.
Both entry points now normalize once and hand the result to a shared
`_resolve_normalized_connector_access`. Same measurement after the change:
one invocation through either entry point.
Normalization stays where it was, above the `try` in the raising entry
point. That boundary is what makes an un-coercible id surface as the
`TypeError`/`ValueError` it is instead of a retryable 503, and the eight
`lets_a_malformed_ref_raise_raw` cases pin it; passing raw `refs` down
would have moved it.
The hook is passed to the helper rather than read from the module global
inside it. Each entry point reads the slot once, checks it for `None`, and
passes down what it read, so the helper cannot act on a hook other than the
one that was just cleared, and a future third caller does not inherit an
unguarded read. This also removes the second global read the non-raising
entry point used to do.
Two pieces of prose stopped being true and were rewritten rather than left:
- The raising entry point's docstring named `resolve_connector_access` as
the source of the answer validation whose `ConnectorRuntimeError` passes
through. That validation lives in `_validate_connector_access_answer`,
which is now what the sentence names.
- The comment on its hook gate said the gate was "not redundant with the
identical gate inside resolve_connector_access". There is no longer a
duplicated gate to be redundant with, so the comment states the invariant
it actually guards: both entry points must answer `{}` for the same input
in a deployment with no hook installed, and that answer comes before
normalization.
Observable behaviour is unchanged, so no existing test can see this. The
new `test_each_entry_point_normalizes_the_refs_exactly_once` counts the
invocations directly, parametrized over both entry points.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook seam, including the ConnectorAccess dataclass, batch and single-ref resolution functions (resolve_connector_access_or_raise), and a unified hook gate (_call_connector_hook_gate) that ensures automatic database session rollback on hook failures. It also adds a context manager for snapshotting hooks in tests and updates existing hooks to use the new gate. Feedback on these changes suggests strictly validating the input reference in resolve_one_connector_access_or_raise to prevent silent coercion of malformed IDs, and explicitly verifying that connector_type is a valid value (such as 'mcp' or 'custom_api') during answer validation.
`_normalize_connector_refs` coerced every id with `int()`. For an id that
was not already an int but would convert, that quietly changed which
connector was asked about: `("mcp", "11")` reached the hook as
`("mcp", 11)`, and the answer came back keyed `("mcp", 11)` while the
caller still held `("mcp", "11")`.
`resolve_one_connector_access_or_raise` unwraps that answer with
`.get(ref)`, so the lookup missed and it returned `None` -- which this
seam defines as "the caller's team does not link this connector", and
which its own docstring promises is "never a failure" -- for a connector
the hook had just granted. Probed at the parent commit against a hook
that grants every ref it is asked about:
ref passed in batch answer keys wrapper returned
('mcp', 11) [('mcp', 11)] granted
('mcp', '11') [('mcp', 11)] None <- lost
('mcp', True) [('mcp', 1)] granted, for connector 1
Ids are now validated instead of coerced, so the ref the hook is asked
about is always the ref the caller passed and an answer key can never
fail to match the ref it answers. Both bad rows above now raise
`TypeError` at both entry points, before the hook is reached. The same
probe after this commit returns `raised TypeError` for both.
`bool` is excluded explicitly, matching `_validate_connector_access_answer`
and `_validate_team_connector_answer`: `isinstance(True, int)` is `True`,
and `("mcp", True)` hashes equal to `("mcp", 1)`, so the third row above
was resolving a different connector's access with nothing failing to say
so.
Both new shapes join `MALFORMED_REFS` rather than getting a new error
category, so the three existing families that parametrize over it --
raise-raw through the batch entry point, raise-raw through the single-ref
wrapper, and returns-{} when no hook is installed -- cover them without
changing what any of those tests assert.
Prose corrected where the old semantics had been written down:
- `_normalize_connector_refs` said it coerced, and said an id that "will
not coerce" is the rejected case. It now says an id that is not already
an int is rejected, and records why coercing was worse than either
coercing or dropping.
- `resolve_connector_access` never stated what its answer is keyed on --
the omission this defect grew in. It now states that the keys are the
caller's own refs, and why that is now guaranteed.
- `resolve_connector_access_or_raise` documented its logged refs as
"the normalized `(connector_type, int(id))` tuples ... `('mcp', '5')`
in, `('mcp', 5)` logged". That example no longer happens; it now says
the logged refs are the caller's own.
- The malformed-ref tests described their subject as "not a
`(str, int-coercible)` pair".
One existing test changed payload: the normalize-once check passed
`[("mcp", "5"), ("mcp", 5)]`, which the seam now rejects. It uses a
duplicated valid ref instead, since it counts passes rather than
exercising what a pass does.
Connector type handling is unchanged: still `isinstance(..., str)` only,
still not checked against the `ConnectorType` literal at run time.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new connector access hook seam (ConnectorAccessHook) and the ConnectorAccess dataclass to manage and validate team-level connector access and edit permissions in a batched manner. It also refactors existing hook invocations to route through a unified gate (_call_connector_hook_gate) that handles automatic database session restoration on failures. Comprehensive unit tests are added to verify the validation logic, error handling, and session rollback behavior. Feedback suggests adding validation to the visible_team_connector_ids hook call to ensure malformed answers are caught early, aligning it with other validated hooks.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
PR Summary
This PR adds the missing "access" slot to the connector_team_scope hook seam — letting a downstream multi-tenant app answer whether a caller's team may view/edit a given connector — alongside a typed 503 failure contract and a session-rollback guard for hook failures. Nothing in this PR is wired to a live route yet: the consuming route is deferred to a follow-up PR, so all new code (including the 503 contract and rollback gate) is currently exercised only by this PR's own test suite. The seam shape and batching contract are sound and consistent with the existing knowledge_base_team_scope precedent; the issues below are mostly unused-surface-area and simplification opportunities rather than defects.
Blocking: no — recommended event: APPROVE
Round 0 — Design Verdict: Over-engineered
The seam shape and batching contract are right, but two of the PR's three headline investments — the typed-503 failure contract and the session-rollback "door" — have no reachable beneficiary at any route today, since this PR wires no caller. The new ConnectorRef = tuple[ConnectorType, int] alias also duplicates the name (but not the validation) of an existing, better-validated ConnectorRef dataclass already used pervasively elsewhere in the same package. None of this is wrong in direction — the gap to a lean version is mostly deletion/reuse of what already exists, not a redesign.
Findings by Severity
MAJOR
1. 503 failure contract never reaches a real client (cross-cutting: src/xagent/web/app.py, src/xagent/web/api/mcp.py, src/xagent/web/api/custom_api.py, src/xagent/web/services/connector_team_scope.py)
app.py registers no exception handler for ConnectorRuntimeError; the four routes that would consume a hook failure via this mechanism (mcp.py's update_mcp_server/delete_mcp_server, custom_api.py's update_custom_api/delete_custom_api) either catch broad except Exception and re-raise a generic HTTPException(500), discarding .status_code, or have no try/except at all and fall through to the global catch-all → generic 500. This is pre-existing (those routers are untouched by this diff) and already applies to the live delete/rename hooks today, so it does not meet the PR-caused blocking bar — but this PR adds an access hook and a whole test suite asserting status_code == 503 on a contract that doesn't reach any real caller, which is worth tracking.
Blocking: no — pre-existing, not introduced by this PR's diff. Suggested follow-up: register a ConnectorRuntimeError handler in app.py, or have mcp.py/custom_api.py explicitly catch-and-translate it ahead of their broad except Exception.
MINOR
2. src/xagent/web/services/connector_team_scope.py:68 — ConnectorRef = tuple[ConnectorType, int] duplicates the name of the existing @dataclass(frozen=True, order=True) class ConnectorRef in src/xagent/core/tools/adapters/vibe/connector_runtime.py (already imported from in this file, and used as a dict key throughout src/xagent/web/services/connector_runtime.py). _normalize_connector_refs (line ~269) re-implements validation the dataclass's __post_init__ already provides; a caller passing an actual connector_runtime.ConnectorRef instance would get a raw TypeError instead of this seam's typed error. No live impact today (zero production callers), but worth adopting the existing dataclass or renaming this alias before the follow-up PR wires in real callers. Blocking: no — no production callers exist yet.
3. src/xagent/web/services/connector_team_scope.py:538 — _call_connector_hook_gate(db, hook, *args, validate=None, **kwargs) erases static arity/type/keyword checking at all 5 hook-invocation call sites, including the keyword-only team_id guarantee that TeamConnectorVisibilityHook's Protocol was specifically designed to enforce. All 5 current sites are correct, so this is not a live bug, but a @contextmanager-based guard would preserve static checking at call sites without losing the "one door" property. Blocking: no — defensible centralization tradeoff, no current call-site defect.
4. src/xagent/web/services/connector_team_scope.py:504 — _restore_session_after_hook_failure's docstring claims rollback happens "after the route's own db.commit()," but at both rename call sites (mcp.py:3369 before db.commit() at :3373; custom_api.py:393 before db.commit() at :406) the hook actually runs pre-commit with route-side mutations still pending. This causes no live data-loss regression since both routes already self-protect independently. Separately, the gate's unconditional db.rollback() is stylistically inconsistent with release_db_connection_if_clean (src/xagent/web/models/database.py:105-137), which conditionally refuses to roll back a dirty session — worth a short comment noting why unconditional rollback is safe here (the request is aborting regardless). Blocking: no — docstring-accuracy issue only, no behavior change.
5. Test-coverage precision gaps (tests/web/services/test_connector_team_scope.py, tests/web/test_team_sharing_hooks.py) — several small gaps, none blocking: (a) the access hook's "hook raises directly" path is tested only with db=None, never against a real SQLAlchemy session, unlike the delete/rename/visibility hooks which each have a dedicated real-session raising-path test; (b) the malformed-answer-vs-hook-DB-failure tests for resolve_connector_access_or_raise never assert on exc.__cause__ to prove the two failure modes stay distinguishable, unlike the sibling resolve_team_connector_ids_or_raise tests; (c) _restore_session_after_hook_failure's getattr(db, "rollback", None) silently no-ops for anything lacking .rollback, so no test can prove real rollback semantics beyond the real-session tests noted above; (d) test_connector_team_scope.py:835 uses with pytest.raises(Exception):, broad enough to mask a broken test setup — recommend narrowing to pytest.raises(SQLAlchemyError); (e) test_team_sharing_hooks.py's connector-hooks teardown (line 70) still uses set_connector_team_hooks() (clear-all) rather than this PR's own snapshot_connector_team_hooks() primitive — exactly the test-isolation hazard that primitive exists to prevent — while the KB half of the same file uses the snapshot pattern a few lines away. Blocking: no — test-precision only, no coverage regression.
6. Documentation-precision items (src/xagent/web/services/connector_team_scope.py, multiple docstrings) — (a) the rationale for rejecting bool/float/Decimal connector-ids in answer keys (near line 269) claims it prevents "resolving a different connector's access," but Python dict/tuple equality means ("mcp", True) and ("mcp", 1) hash identically, and all consumption in this codebase is .get()-based, so misattribution can't actually occur — the rejection is still reasonable as type-contract hygiene, just not for the stated reason; (b) no connector_access_hook_installed() predicate exists, unlike the sibling team_connector_hook_installed(), so a future caller can't distinguish "no hook installed" from "hook installed, answered empty"; (c) ConnectorAccessHook's type alias has no docstring noting hooks receive a deduplicated, unordered frozenset of refs, not a list; (d) two docstring paragraphs are near-duplicated across function pairs (resolve_team_connector_ids_or_raise/resolve_connector_access_or_raise's "session restore" block; visible_team_connector_ids/resolve_connector_access's "nothing cross-checks them" block) and could be stated once; (e) _normalize_connector_refs's docstring narrates an intra-PR-only design reversal (the int("11") coercion added then removed within this PR's own commit history) in detail that will read as permanent documentation post-merge, even though commit 1d4a25ef's message already captures it — consider trimming to the final rationale only. Blocking: no — documentation-only, no code or behavior implication.
Simplification Opportunities
delete:ConnectorAccess.team_owned(line 64) carries zero information — presence in the answer map already means "linked," so the field can only ever beTrue, and it's read nowhere except its own validator. Drop it; makecan_edit: boolthe sole required field with no default, so a bareConnectorAccess()fails at construction instead of via a runtime validator.yagni:resolve_connector_access(line 456, non-raising batch function) has zero production callers and isn't even called internally byresolve_connector_access_or_raise;resolve_one_connector_access_or_raiselikewise has zero callers anywhere. Make private, or defer the public shape until the follow-up PR's actual call pattern justifies it.shrink:snapshot_connector_team_hooks(line 751) hardcodes a 5-tuple of hook-slot names with a manual-sync caveat.test_connector_hook_slot_names_are_discoverablenearby already discovers slots by enumerating module globals ending in_hook— reuse that discovery approach internally to remove the caveat and its tripwire test.shrink:Three near-identical tests intest_connector_team_scope.py(lines 331, 343, 357: rejects a non-tuple key / wrong-length key / non-str connector_type) share identical structure, differing only in the bad key and expected message. Collapse into one@pytest.mark.parametrizetest, matching the pattern already used elsewhere in this file.
net: roughly -45 to -60 lines possible (largest share from the team_owned field removal, once ~20 test call sites and one dedicated rejection test are updated).
Re-review: Prior Findings (gemini-code-assist bot)
All 7 canonical prior findings are resolved as of current HEAD (1d4a25ef), independently re-verified against the code rather than taken on the author's word:
| # | Finding | Status |
|---|---|---|
| 1 | Test fixture cleanup exception-safety (try/finally around hook reset) | FIXED (53c36e16) |
| 2 | Redundant re-normalization of connector refs | FIXED (7e4307e6) |
| 3 | sorted() could raise TypeError on uncomparable ref types, masking real exception |
FIXED (4b6bd01f) |
| 4 | can_edit bool check should use isinstance not manual identity checks |
FIXED (034b3c6a) |
| 5 | Silent int() coercion let bool ids alias, causing "granted" to misreport as "not linked" |
FIXED (1d4a25ef, current HEAD) |
| 6 | connector_type not validated against {"mcp","custom_api"} in answer validator |
WAIVED — non-str types are checked; invalid-but-str literals are caught by CI's mypy Literal enforcement; duplicating the set would risk drift |
| 7 | visible_team_connector_ids's hook-gate call omits validate= |
WAIVED — pre-existing gap predating this PR's base commit, mechanical wrap only, tracked in issue #1878 |
No regressions found on any previously-fixed item.
Non-blocking context (FYI, not a defect in this PR)
The PR's cited precedent, knowledge_base_team_scope.py (touched by this PR only via a docstring edit, no behavior change), has neither of this PR's two headline safety investments: its access hook is invoked with no session-rollback gate against ~14 live endpoints' real sessions, and KnowledgeBaseAccess defaults can_edit=True, can_delete=True (fail-open), unlike the new ConnectorAccess's fail-closed False/False defaults. Out of scope for this PR, but worth a tracked follow-up since it means the more valuable half of "session safety" work sits on the less-exposed of the two twin seams.
Blocking Status & Recommended Decision
Blocking: no — recommended event: APPROVE. Findings above are all minor/non-blocking or already resolved; item 1 (the 503 contract not reaching any real caller) is a real, worth-tracking issue but predates this PR's diff and doesn't meet the PR-caused blocking bar.
…re docstring The session-restore docstring said the rollback happens after the route's own commit "on the post-commit decoration paths". No call site works that way: both rename paths and both delete paths invoke their hook before the route commits. State what actually holds instead -- the rollback can only reach the aborting request's own pending mutations, which the re-raised exception was going to strand anyway -- and record why the rollback is unconditional here, unlike release_db_connection_if_clean, which runs on the success path and must not discard work the request means to commit. Test side: - the hook-door restore test asserted pytest.raises(Exception), broad enough to pass on a TypeError from a mis-built invoke; narrow it to SQLAlchemyError, which is what the primary-key collision raises. - the connector half of the team-sharing hook test reset its slots by clearing all of them, while the knowledge-base half of the same file already used the snapshot primitive. Use the snapshot there too, and keep the clear-all call inside the block where it is the assertion rather than the teardown. - collapse three structurally-malformed-answer-key tests into one parametrized case. Sibling enumeration: roughly two dozen older suites elsewhere under tests/ still reset connector hooks by clearing them. They predate this change and are a separate cleanup, so a comment claiming this file was the last one doing so is corrected rather than acted on.
…auses The access wrapper's raising-hook tests all passed None for the session, so nothing covered the restore on a real one -- unlike the team-visibility wrapper, which has that test. Add the sister case, and assert the typed error's cause on both access failure modes so a hook that raises stays distinguishable from an answer the seam rejects.
|
Thanks for the review. Head is now
Answering the items from the review body that have no inline thread of their own. The typed 503 never reaching a client — filed as #2055. It is real: The knowledge-base twin's fail-open defaults and missing rollback gate — filed as #2056. Confirmed at
No The remaining docstring items — the Rollback semantics being unprovable through Edit: the two commits above were re-recorded as |
93ea597 to
9ced8aa
Compare
main's xorbitsai#1912 added resolve_connector_access_or_raise() to connector_team_scope.py, whose generic-exception fallback arm raises ConnectorRuntimeError with reason "connector_access_resolution_failed". This PR's whitelist-coverage test requires every reason a ConnectorRuntimeError construction can produce to be classified in the same change that adds the raise site, so merging main's new raise site into this branch turned that test red until the reason is classified here. Judged against the two questions this whitelist's comment asks: does the reason name who owns the task, or state what an authorization check concluded? Neither. It only reports that resolving connector access failed, the same as the already-whitelisted team_scope_resolution_failed a few lines above -- same module, same 503, same fallback-arm shape.
…itsai#1919) * feat(web): project connector runtime failures onto a wire-safe error A ConnectorRuntimeError carries a curated, public-safe message and a details payload, but nothing today turns either into something a chat client can read. Add the two functions that do, plus the type that owns what is allowed onto the wire. connector_runtime_client_message adapts the exception's safe_message and falls back to the fixed task-failure text for anything else, so the boundary stays fail-closed even if a future caller passes an incidental exception despite the function's specific name. connector_runtime_public_error projects the exception onto (code, PublicErrorDetails). PublicErrorDetails holds a single field and normalizes it in __post_init__: a reason that is neither a listed enum value nor "<listed prefix>.<declared key name>" becomes None. Putting the whitelist in the constructor rather than in the projector makes "constructing this type" and "passing the whitelist" the same act, so a direct construction from another module cannot carry free text. It nulls rather than raises because every construction site is on the reporting path of an already-failed task. The type has no connector_ref field. The sink for this projection is broadcast_to_task, whose audience includes anonymous widget and share-link visitors, and the same judgement keeps two runtime reasons off the whitelist: runtime_task_identity_mismatch and runtime_owner_mismatch state the task's ownership and the outcome of an authorization check. The four 503 reasons that are listed only state that a server-side component is unavailable. The question that decides this is written into the class docstring so a later addition has to answer it too. Tests derive the reason surface by AST-scanning src/ for every site that constructs a ConnectorRuntimeError -- both construction forms -- rather than from a list of modules, and assert the whitelist neither misses a raise site nor grows an entry nothing produces. * feat(web): carry an error code on the terminal task_error frame create_terminal_task_error_event gains two keyword-only parameters. They are written into the frame only when both are supplied, so the four call sites that pass neither produce a byte-identical frame. The details parameter is annotated PublicErrorDetails, which makes mypy refuse a dict at every call site it can see. That is not the whole door: annotations are not enforced at run time, and a caller routing through Any -- a dict decoded from JSON, a **kwargs splat -- type-checks clean and would only fail deep inside the function on a missing to_wire. The first statement in the body names the contract instead. That check reads `type(details) is not PublicErrorDetails` rather than isinstance. A frozen dataclass can be subclassed, and a subclass that overrides to_wire without reading self.reason satisfies both isinstance and mypy while bypassing the whitelist that lives in __post_init__. Only the class itself carries that guarantee. The client-safe AST guard is untouched: the new parameters are keyword-only so the message argument does not move, the new fields are not in the guard's sensitive-field set, and no producer or error-payload sink is added. Both exact baselines still hold at 30 and 52. * feat(web): classify connector runtime failures at terminal settlement A ConnectorRuntimeError subclasses RuntimeError and is not a RequiredMCPUnavailableError, so the terminal settlement's two-way classification sent it down the else branch and every such failure reached the user as "Task execution failed." -- which the chat client then rendered as an unknown error. The exception's own docstring already commits its message and details to being safe for API callers. Add a third branch between the two existing ones; neither of them changes. It settles with the exception's string, marks the history row client-safe, and broadcasts the curated sentence along with the projected code and details. It also logs one structured record naming the code, the reason and the connector. That log reads the raw exc.details rather than the projection: its audience is operators, the connector identity is what makes the record actionable, and it never leaves the server. The broadcast frame carries neither the connector identity nor any reason the whitelist dropped. The log is unconditional -- an observability record that can be switched off is one that is not there when it is needed -- and all three values are short bounded strings. * fix(frontend): render the terminal error bubble and name the missing key The error and task_error branches dispatched an assistant message without isResult. The conversation panel renders only user, isResult and system-notice messages, so the bubble was filtered out entirely and the turn fell back to a virtual "unknown error" placeholder until the page reloaded -- the server could say whatever it liked and none of it was shown. The task_completed branch already fixed this the same way; copy that precedent, comment included. Read the frame's code and details. When the code is one of the three that mean a connector is missing a value the user can supply, replace the relayed sentence with wording that names the key, parsed off the reason's last segment. A reason that names no key -- a bare enum value, or one the server whitelist dropped -- gets the keyless wording, and connector_runtime_unavailable keeps the plain failure wording because it reports a component being down, which the user cannot act on. Both are i18n keys with en and zh entries, not hardcoded English. The pair is also kept in a new lastConnectorRuntimeError state field. It holds only what the frame is allowed to carry: the frame has no connector identity, deliberately, because its audience includes anonymous widget and share-link visitors. Anything more specific has to come from the owner-only per-task requirements endpoint. * refactor(web): tie each public reason to its own raise site Two follow-ups on the reason whitelist, both narrowing it. The whitelist no longer lists payload_too_large or encryption_unavailable. Both belong to the runtime value-fill endpoint, which does not exist yet; they were listed ahead of it because the whitelist is the contract that endpoint will be written against. That required an exemption in the test asserting no listed reason is unproducible, and an exemption on that assertion is an allowance with no expiry date -- by the time the raising code lands, the audience judgement behind the entry has to be reconstructed from scratch. Each string now arrives with the site that raises it, and the assertion holds with no exemptions at all. The key half of a prefixed reason is now matched against RUNTIME_SOURCE_KEY_RE, the grammar the connector runtime already exports, rather than a second copy of the same pattern compiled here. The module already imports from that file, so there is no new coupling, and the two definitions can no longer drift apart. * docs(web): describe what client_error_messages now holds The module docstring described it as fixed fallback strings, which was accurate when the file held two constants and one adapter. It now also owns PublicErrorDetails and the reason allowlist governing what may ride on a task_error frame, so say so. * fix(web): degrade instead of raising on the terminal error path Three changes to the terminal frame, all in the same direction: this frame is what stands between a failed task and a silent one, so nothing about it should be able to cost the frame itself. The details type check no longer raises. PublicErrorDetails already nulls an unlisted reason rather than raising, and its docstring gives the reason -- every construction site is on the reporting path of an already-failed task. The frame builder sits on that same path, and the one call site passing these arguments evaluates them inside the `except Exception` that only logs "its terminal broadcast failed". A TypeError there meant the task committed FAILED and the client saw nothing at all, which is the exact failure this branch exists to remove: the strict half degraded to a worse outcome than the bug. A rejected value is now dropped, logged at ERROR with its stack, and the frame goes out without it. `code` now passes the same closed set. ConnectorRuntimeError types it as a bare str and assigns it unvalidated, so "only the ten module constants reach here" describes today's raise sites rather than anything the code enforces -- one door locked and the one beside it open. V1ErrorCode is the repository's existing closed set of client-visible codes and carries all ten; it is imported rather than recopied, inside the function because the v1 package's __init__ pulls in routers that import this module. Unknown values follow the same drop-and-log path. The durable half of the settlement is now pinned too. The classification branch writes three things and the tests covered one: deleting `client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE` left every test in the file green while a reloading user dropped back to the generic failure text the live bubble no longer shows. The capture helper hands back the settlement kwargs, and both the curated and the incidental branch assert what they persist. * docs(web): name the /v1 sibling projection and why it differs `_raise_v1_connector_runtime_error` (web/api/v1/tasks.py) already projects this same exception for a client, and it makes the opposite call on both halves: it maps the code through V1ErrorCode with an unknown fallback, and ships `to_public_error()["details"]` whole with `connector_ref` in it. Reading either projector alone, the other looks like a contradiction. It is not: the audiences differ. /v1 answers an API key held by a caller already authorized for the task. This path feeds `broadcast_to_task`, which reaches every connection under the task id, anonymous widget and share-link visitors included -- the fact every other choice here follows from. Say so where someone comparing the two will be standing, and say why they stay two projectors rather than one taking the audience as an argument: output width behind a caller-supplied flag fails open the first time the flag is passed wrong. Also states why the details-shape check exists at all, since __init__ normalizes that attribute: it is a plain public attribute anything can reassign afterwards, and this is the last step before the wire. * fix(frontend): dedup terminal errors by code and reason, drop unread state The dedup key was the server's own sentence, and `_message_for_code` returns one fixed string per error code that does not vary with the missing key. So two turns failing on two different keys inside the 30-second window collapsed into one bubble, and the survivor named whichever key failed first -- one value standing for two facts, the same shape this change set exists to remove, reintroduced a layer up. The comment added alongside it claimed the server message "identifies the failure", which stopped being true the moment the rendered wording began deriving from `reason` instead. `isDuplicateMessage` already takes an `occurrenceIdentity` argument for exactly this. Pass the code and reason. Two tests cover both directions: different keys keep both bubbles, a genuine repeat still collapses. Also drops `lastConnectorRuntimeError`. It was written on every terminal error frame and read by nothing -- the dialog that consumes it belongs to a later PR in this series, so it was a surface with no consumer in the PR that introduced it. The state field, its action type, its reducer case, its TASK_SCOPED_ACTION_TYPES entry and the test probe all go with it. The helper that reads the pair off the frame is renamed `getTaskErrorProjection`: it returns for any frame carrying a code, and the old name promised a connector-specific answer it never checked for. Whether a frame is connector-related is decided by the code, at the one place that asks. * fix(web): drop owner key names from the connector-runtime reason whitelist A reason of shape missing_context.<key> carried the connector owner's own declared field name onto task_error, whose broadcast reaches anonymous widget and share-link visitors. Collapse _is_public_reason to a plain membership check against the fixed-string allowlist and drop the prefix set entirely -- the other five prefixes had no raise site in src/ either. missing_context.<key> is now dropped whole rather than trimmed to its prefix, so details comes back {} and code alone survives. The other two missing-value codes are unaffected: their reasons are the fixed strings not_provided/store_lost, already on the allowlist. Two tests that used to compare a PublicErrorDetails instance against another instance built from the same withheld reason went quietly blind: both sides null out under __post_init__ and the equality still holds. Rewritten to assert through to_wire() instead. Also adds an AST-derived pattern check so a re-added prefix form is caught by shape, not just by its literal value. * fix(frontend): drop the missing-key wording, match the server's dropped reason The server no longer sends a reason built from the connector owner's declared field name (previous commit), so the client-side key parser has nothing left to read. Drop missingRuntimeKeyFromReason and the keyed translation string, and always use the keyless connectorRuntimeMissing wording for the three missing-value codes. Also switches this test file's i18n mock to the variable-aware form already used across 23 other test files (t returns `key:JSON(vars)` when vars are passed). The old key-only mock could not tell an interpolated call from a bare one, so a future regression that starts passing the key back into the wording would go undetected here. The two dedup tests that asserted on the now-removed keyed wording are dropped; their (code, reason) replacements land separately. * fix(frontend): dedup terminal errors on (code, reason), not the fixed sentence The prior comment on errorOccurrenceIdentity argued from a scenario that never occurs on this path: two different missing-value codes sharing one dedup key. Each of the three connector-runtime codes maps to its own fixed server sentence, so two different codes never share a key in the first place. The scenario the (code, reason) identity actually guards is one code failing under two different admitted reasons -- a runtime secret that was never provided, then later found lost from its store. Both share the same server sentence and therefore the same base dedup key, and without the reason folded in, the second turn's result bubble would vanish inside the existing dedup window. Replaces the two dedup tests with that scenario and its inverse (a genuine repeat of one failure still collapses to one bubble). * fix(frontend): only flag the terminal task_error frame as a turn's result The shared "error" / "task_error" handler flagged every bubble it produced as isResult, but the root "error" type is a mixed channel: a rejected chat message, a rejected pause, or a rejected resume all arrive on it while the viewed task is still RUNNING or WAITING_FOR_USER. Flagging one of those closes the conversation panel's live progress indicator and waiting-answer form for a turn that has not actually ended, and drains the turn's accumulated trace events into the rejection bubble. task_error has no such ambiguity: every frame of that type is emitted only after the row has been committed FAILED (task_orchestrator.py's settled branch, and websocket.py's legacy only_if_running helper, which does not broadcast when its update matches no row), and it is also the only frame carrying the structured code/details pair used by the connector-runtime wording and the dedup identity. Gate both on the frame type instead of treating every "error"/"task_error" frame alike. Replaces the parametrized isResult test (which asserted the same, now-wrong, behavior for both types) with one for task_error, and adds two for the root "error" type on a running and a waiting task. * docs(web): name the code set and the fixture's two producers in comments task_orchestrator.py: the elif branch for ConnectorRuntimeError covers more codes than the three missing-value ones (invalid_runtime_context, connector_runtime_unavailable, and the *_resolution_failed codes also land here), so "the three codes" needs a referent to not read as the whole branch. app-context-chat.test.tsx: the waiting-rejection fixture combines `task` from the resume-refusal path (websocket.py:8935) with `error_code` from the pause-refusal path (websocket.py:8491). Neither path emits both today; note the synthesis so it doesn't read as one producer emitting both fields. * refactor(frontend): derive the error frame's display values in one function These 85 lines derive five display values from one frame, and the four conditions each value needs are today recomputed in place with no shared decision point. Two review rounds have found defects in this handler: R1 found isResult mixed across channels and a keyed bubble; R2 found an untrusted-transport wording gap, a wrong dedup identity axis, and an unwitnessed no-version path. Turning the matrix into one function's explicit return value is the precondition for the next defect being visible instead of re-derived and missed again. Pure on purpose: no dispatch, no refs, nothing outside its arguments, so every cell of the matrix is unit-testable without rendering the provider -- the same shape extractTaskControlEnvelope above already uses. Verified equivalent, not just typed the same: a temporary shim in the test file, built by copying the case block's five expressions verbatim, pinned six cells' expected values before this extraction touched any production code (155 passed). After the extraction, the same six cells against the real function produce the same 155 passed with the same expected values. The shim is not part of this commit. * fix(frontend): dedup terminal errors by the frame's own state version The 30-second dedup keys on the server sentence, and one sentence covers a whole code -- so two turns failing under the same code shared a key while being two distinct failures, and that bubble is now the turn's result. Keying on the failure's class instead of on the occurrence cannot tell them apart, whichever class you pick: the sentence, the code, or the code and reason together. What identifies the occurrence is already on the frame. broadcast_to_task stamps every frame of this type with the row's run_id and state_version (task_error is in _VERSIONED_TASK_EVENT_TYPES), and state_version is bumped by each control transition that changes (status, control_state) -- a retry takes the lease FAILED -> RUNNING and settles RUNNING -> FAILED, so the second failure is at least two versions on, while one settlement broadcast twice carries one version. The identity is therefore run_id:state_version, read from the envelope the handler already parses before the switch. No wire field is added, no backend line changes, and the earlier (code, reason) identity is removed rather than kept alongside it. A frame that arrives with no version gets no identity and keys on the text alone, which is the behaviour that predates this change: the version gate at the top of the handler drops such a frame once any versioned event has been seen for the task, and when the task has no versioned event on record either, two such frames key on the same text and the second still collapses. The identity is withheld there rather than guessed -- attaching the state tuple needs the row, and a settled FAILED task has one. * fix(frontend): localize connector runtime codes through the client error table The frame's code, not the relayed sentence, decides the bubble's wording now: the five connector-runtime codes that can reach this frame are listed in the client error-code table this repository already uses for the root error channel's error_code field, each with a translation key and an English fallback. That table is the reason the wording now survives an untrusted transport, where relaying server prose is refused by design (xorbitsai#1938) -- before this, only three codes had curated wording and every other code, connector codes included, read "Unknown error" for an anonymous widget or share-link visitor. Only codes with a producer that can reach this frame today are listed, the same rule the server's reason whitelist already states about itself. Of the other five connector-runtime codes, two have no raise site in this repository at all. The remaining three are raised while a connector-runtime payload is being validated. Nothing that reaches those checks settles a task: a request handler answers the call with an error response (the /v1 task endpoints, and the trigger-config endpoints, which convert the failure into their own service error), and the trigger run-preparation path throws before the task row is created and records the failure on its TriggerRun row. No settled task means no terminal frame. A code the table does not list keeps the generic prefixed wording. This also fixes: the logged-in audience no longer sees a different wording than an untrusted transport gets for connector_runtime_unavailable, since that code now has its own table entry instead of relaying the server's four different English sentences for it. The old single-purpose CONNECTOR_RUNTIME_MISSING_VALUE_CODES set and its one i18n key are gone. * fix(web): type-gate the code before the closed-set membership test N1: the code passed into create_terminal_task_error_event is typed as a bare str but not enforced at runtime -- ConnectorRuntimeError itself types its code the same way and stores it unvalidated. An unhashable value (a list) would raise inside the frozenset membership test on a path whose whole point is that it never raises; a hashable non-string is simply not a member and already takes the drop path either way. The type check now comes first, the same way the neighboring `details` check already does. N3: the operator log line in task_orchestrator reads the same ConnectorRuntimeError.details attribute the wire projector does, for a different reason (an operator gets the raw connector identity; the wire projector filters it out) -- but it read it without the same shape guard. Guarded now with the same isinstance check before reading, at the cost of three lines. The non-dict branch this adds has no test witness in this repository today: triggering it needs a `details` attribute reassigned to a non-dict shape after construction, a shape no raise site produces, and the existing log-line test only covers the dict-shaped path continuing to log correctly. The follow-up (a test that exercises the non-dict branch) is not done in this PR, per approved scope. S1: dropped the @lru_cache on _client_visible_error_codes -- a ~30-member frozenset is cheaper to rebuild per call than to reason about as a cache. The deferred import and its circular-dependency comment are unchanged. S2: deleted the ownership-boundary test for PublicErrorDetails's single construction site; the wire-safety guarantee it was adjacent to (`type(details) is not PublicErrorDetails` in the frame builder) is untouched. `_python_sources` stays -- the reason-whitelist derivation still uses it. The reason-whitelist AST machinery is kept rather than flattened to a hardcoded list -- it exists to catch a future raise site shaped like today's interpolated ones, which a list can't -- and gets three more assertions: the three regex-admitted `undeclared_*_key` reasons are now derived from the same section-name constants the raise site loops over, rather than only regex-matched (this surfaced a second interpolated pattern the scanner finds, `missing_context.<key>`, which is deliberately ungrounded -- it is assembled from a connector owner's declared key name and is already asserted elsewhere to never reach the wire); the two blind spots in the scanner itself (a `details=` argument that isn't a literal dict, and a construction reached through an attribute rather than a bare name) are each pinned to their real occupancy today (one non-literal site, zero attribute constructions); and the five opaque reason expressions this repository raises are pinned to the one shape they take (`str(exc)`), so a future %-format or .format() shows up as a failure here instead of silently widening what the whitelist has to cover. Also added: a parametrized test pinning three non-string code shapes through the type gate, and a frontend test for the code/details-nested-under-data half of the existing root/data fallback. * docs(frontend): fix three positional comment references broken by the C1/C2 moves All three comments were carried over verbatim from where the code used to live and pointed at a fixed position ("above", "below") rather than at what they meant. Moving the code left the words in place while the thing they pointed at moved elsewhere in the file, or left a position reference that still landed on something true but not on what the sentence meant: - The isResult comment in projectErrorFrameForDisplay said "see ADD_MESSAGE above", written when this code lived inside the case block far below the ADD_MESSAGE reducer case. The extraction moved it above that reducer case, so "above" now points at nothing there; named the reducer case directly instead of relying on file position. - The six-cell test's controlEnvelope comment said "the no-version cell below", but that cell is the array literal above the it.each callback the comment sits in, not below it. - The dedup-identity comment said "the version gate above", meaning the version gate's call site in the handler (described elsewhere in this PR as "the version gate at the top of the handler", which sits well below this comment, not above it). "Above" happened to still land on something true -- canAcceptTaskControlVersion's definition is above -- but not on what the sentence meant; named the function directly instead of relying on position. * fix(web): whitelist the access-resolution reason main now raises main's xorbitsai#1912 added resolve_connector_access_or_raise() to connector_team_scope.py, whose generic-exception fallback arm raises ConnectorRuntimeError with reason "connector_access_resolution_failed". This PR's whitelist-coverage test requires every reason a ConnectorRuntimeError construction can produce to be classified in the same change that adds the raise site, so merging main's new raise site into this branch turned that test red until the reason is classified here. Judged against the two questions this whitelist's comment asks: does the reason name who owns the task, or state what an authorization check concluded? Neither. It only reports that resolving connector access failed, the same as the already-whitelisted team_scope_resolution_failed a few lines above -- same module, same 503, same fallback-arm shape. * docs(frontend): anchor two websocket producer references by symbol, not line Merging main shifted websocket.py's line numbers; replaced the two stale websocket.py:<line> citations with the hosting function's name, matching this PR's existing convention of naming the referent instead of relying on a position that moves. * refactor(web): send only the error code on terminal task_error frames The terminal task_error frame no longer carries a details object. The server used to project a connector-runtime failure onto a (code, reason) pair, filtering reason through a fixed allowlist before it reached broadcast_to_task -- whose audience includes anonymous widget and share-link visitors. That allowlist has no consumer today: the client only ever read the code, so the whole reason channel was dead weight carrying review risk with nothing on the other end. create_terminal_task_error_event now takes only a code argument. PublicErrorDetails, the reason allowlist, and the two-value projector are gone; connector_runtime_client_code replaces them with a single-purpose projection from exception to code. The frontend projection follows: TaskErrorProjection carries only code, and getTaskErrorProjection no longer reads a details field. * fix(web): scope the terminal-frame code closed set to connector-runtime codes The terminal task_error frame validated its code argument against V1ErrorCode, the repository's full /v1 error surface -- roughly thirty codes covering everything from rate limiting to workforce archival, most of which have nothing to do with a connector runtime. The frame reaches anonymous widget and share-link visitors, so its own closed set should describe exactly what belongs there, not borrow a much wider one that happens to be a superset. CONNECTOR_RUNTIME_CLIENT_ERROR_CODES replaces the V1ErrorCode lookup with the eight connector-runtime codes this repository actually raises as a ConnectorRuntimeError. The two authorization-outcome codes (mcp_oauth_authorization_failed, delegated_authorization_failed) stay out: nothing raises them today, and each one states the outcome of an authorization check, which this frame must never carry. * test(web): pin where terminal-frame code arguments come from create_terminal_task_error_event's own runtime gate only checks the value of a code argument -- whether it is a member of the closed set -- and has no way to tell a curated projection from an incidental string that happens to collide with a real code today. A future call site could pass str(exc) or read .code straight off an exception and this repository would not notice until the wrong fact reached an anonymous visitor. This AST-based test closes that gap statically: every call site under web/ that passes code= must bind it, in the same function, from a direct call to connector_runtime_client_code -- the one projector this repository trusts for this purpose. Today that is exactly one call site, task_orchestrator.py's _runner. * test(frontend): cover the resume-settlement task_error frame and the trace drain Two terminal task_error producers carry no code today, and neither had coverage: external_task_cancel.py's cancellation broadcast (message only) and websocket.py's resume-settlement broadcast, which carries error_code on the root instead. Both still have to make the frame the turn's result on isTerminal alone, and the cancellation path also has to carry forward whatever trace events accumulated on state.traceEvents before the settlement -- the one place that happens, in ADD_MESSAGE's isResult branch. Adds a resume-settlement case to the projectErrorFrameForDisplay table and a transport-level test that seeds two trace events, delivers a cancellation frame, and checks they land on the settling message while state.traceEvents is cleared. Also pins client-errors.ts's fallback strings against the English locale so the two tables cannot drift. * refactor(web): drop the unused fallback parameter connector_runtime_client_message's fallback parameter has had exactly one caller since it was added, and that caller never overrides the default. The two return sites use CLIENT_SAFE_TASK_FAILURE directly now; required_mcp_unavailable_client_message keeps its own fallback parameter, which does have an overriding caller. * docs: describe protected invariants without review-round references Sweeps the batch for review-round references and stale comments the earlier commits in this sequence left behind. The regex sweep itself found nothing, but a manual read turned up three comments describing a mechanism the batch already removed: one docstring still claimed code was checked against "the same closed set the /v1 surface pins against" after the closed set became a curated subset, and two fixture comments in test_task_orchestrator.py still described a reason value as "public" or "withheld" from the wire after this batch's first commit removed the wire's reason channel entirely. * test(web): name the closed-set producer guard for what it pins and drop a duplicate frame pin
3 participants
Split out of #1661, which is being broken into pieces that can each be reviewed on
their own. This one carries the seam and nothing else; the routing changes that
consume it stay on that branch.
What this changes
Standalone xagent keeps MCP servers and custom APIs user-owned: whoever created a
connector is the only one who can edit it.
connector_team_scopealready exists as theplace where a downstream application that installs the hooks can overlay team ownership
on top of that, and it already carries slots for delete, rename, and two flavours of
visibility.
This adds the one slot that was missing: an access hook, which answers whether the
caller's team links a given connector, and whether the caller may edit it.
Nothing is wired to a route in this PR. This is the type, the validation, the failure
handling, and the tests for that hook — and only those.
The hook contract
resolve_connector_access(db, user_id, refs)takes a collection of(connector_type, connector_id)refs and asks the installed hook once, no matter howmany refs are passed. Batching is the point of the signature: the question a caller
actually has is "what is this caller's team's relationship to these connectors", and
answering it should not cost one hook call per connector.
Two rules make the answer unambiguous:
is left out of the answer map. It is never expressed by returning a verdict with
team_owned=False.ConnectorAccessstill defaults toFalse/Falseso that a bareConnectorAccess()stays a shape the validator rejects, rather than quietly becoming alegitimate "not linked" answer.
can_editis independent of linkage. A team can link a connector without grantingedit rights on it. That is a complete answer on its own, not a partial one.
The answer is treated as an authorization input, not as user-facing data, so a malformed
answer raises instead of being normalized, coerced, or defaulted to empty. Both booleans
are checked by identity rather than truthiness, and connector ids are rejected when they
are
bool—boolsubclassesintin Python, and a truthy value is never a legitimategrant or a legitimate id.
Keys are validated as exact
(str, int)pairs before membership is checked at all.True,1.0andDecimal("1")all compare equal to1, and ordinary tuple equalitycarries that through, so
("mcp", True)would otherwise pass a membership check against("mcp", 1). Since the keys of the answer are the question, a key that merely resemblesa requested ref has to fail loudly rather than be accepted as the connector it aliases.
Session safety
Hooks run on the endpoint's own live session. A hook whose own statement failed leaves
that transaction unusable on PostgreSQL, and a failed ORM
flushleaves it unusable onevery backend — so every later statement in the request, including the ones a degradation
path needs to build its response, would be refused.
Hook invocation is therefore consolidated behind one internal door that rolls the session
back before the exception reaches the caller. Answer validation runs inside the same
guard, because a hook can poison the session without raising: run a statement that
fails, catch that itself, and return a shape this seam then rejects. In that case the
exception is the seam's own, so a guard placed around the call alone would not fire.
All five slots now go through that door, which is what makes the property hold for a slot
added to this module later without that slot's author having to know about it.
One shape stays uncovered on purpose: a hook that poisons the session, swallows its own
failure, and still returns a well-formed answer produces no exception anywhere, so nothing
triggers a restore. Closing that would mean probing session health after every hook call,
which is a different design than a failure path.
Test isolation
snapshot_connector_team_hookssaves every module-level slot and restores it on exit.The slots are process-global, so a test that installs one and "cleans up" by calling the
setter with no arguments restores the empty state, not the state the test found — which
silently drops any hook the process had installed before it.
A discovery-based test enumerates every module global ending in
_hookand asserts thesnapshot restores each one by identity. A slot added later and not added to the snapshot
fails the suite rather than failing silently.
Why merge this with no production callers
There are none, and the grep is honest about it:
Both return nothing. Merged on its own, this PR adds a hook that no route calls and no
in-tree code installs. With no hook installed every function here returns empty without
querying, so a standalone deployment sees no behavior change — but by the same token it
also gains no behavior.
The honest reason to merge it separately is review economics, not urgency. The routing
change that consumes this hook touches connector read and write paths where the failure
mode is a permission check that silently passes. Reviewing the shape of the answer, the
validation rules, and the session-failure handling at the same time as the call sites
means the contract questions and the routing questions compete for attention in one diff,
and the contract questions are the ones that are cheap to get wrong and expensive to
notice later. Landing the contract first makes the follow-up diff a question about call
sites against a contract already settled.
The cost is real and worth stating plainly: if the follow-up never lands, this is dead
code carrying its own test suite. It is a seam whose value is entirely in what is built on
it. Reviewers who would rather see the hook and its first consumer together are asking a
reasonable question, and the answer is a judgement call about review surface, not a
technical constraint.
The precedent is in the tree already:
knowledge_base_team_scopeis the same seam shapefor knowledge bases, including the same snapshot primitive, and this change also updates
that module's docstring where it says the connector seam has no snapshot counterpart —
which is no longer true.