diff --git a/doc/plans/2026-10-27-connection-grants-batch-verification.md b/doc/plans/2026-10-27-connection-grants-batch-verification.md new file mode 100644 index 00000000000..04b3d3de940 --- /dev/null +++ b/doc/plans/2026-10-27-connection-grants-batch-verification.md @@ -0,0 +1,127 @@ +# Connection-grants batch verification + +Date: 2026-10-27. Implements the +[selected design](2026-10-27-connection-grants-capability-batch.md). + +## Change and authority boundary + +`connectionManagementService.describeAgentInstallCapabilities` prepares a direct +saved connection once, checks its retained installation targets once, and checks +each additional candidate in order. It uses the same private target evaluator as +ordinary authorization. The grants GET replaces only its editable-agent loop. + +The optimized shape is board actors, non-AI saved connections, canonical retained +and candidate IDs, and no contextual or proposed-resource selectors. Delegated +agents and +complex resources finish the first already prepared evaluation and then use the +ordinary evaluator. Preparation is not restarted. Configure/use capabilities, +grant identity filtering, audience disclosure, writes, and human continuations +retain their existing authority paths. + +Reuse lasts one descriptive call. It is not an atomic snapshot: a retained +permission or target removed mid-call can leave a stale displayed choice. The +next description and actual authorization must read fresh authority. No context, +mutation authority, or reusable cache escapes the batch. + +## Reproduction and measurements + +Pinned baseline: `f6bc653f7f745adaafa26393852fb2f5a99256a4`. +Treatment service SHA-256: +`f5fca127e8a669d343b61455187e102dc6705d87389544ff4edfa5a066185656`. +The benchmark loads the actual baseline service from Git, verifies that its other +runtime dependencies and lockfile match, and compares it with the new production +batch. Every positional capability, reason, and operation must match. + +```sh +node --import ./server/node_modules/tsx/dist/esm/index.mjs \ + scripts/benchmark-connection-install-capabilities.ts \ + --baseline-ref=f6bc653f7f745adaafa26393852fb2f5a99256a4 +``` + +The fixture is a Cloud board member who is not the creator, with a target-scoped +grant covering the synthetic roster. Company installs retain the whole roster; +partial installs retain one quarter, with at least one retained agent. Each arm +has two warmups and five measured samples in alternating order. Counts below are +per invocation. Full positional equality passed in every iteration. Individual +SQL shapes are unchanged; the benchmark saves statement counts, parameters, rows, +and treatment `EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON)` outside timed samples. + +| Agents / installation | Old median ms (range) | Batch median ms (range) | SQL calls old → batch | Target lookups old → batch | +| --- | ---: | ---: | ---: | ---: | +| 1 / company | 5.13 (4.53–7.45) | 4.82 (4.38–5.08) | 8 → 8 | 1 → 1 | +| 1 / partial | 5.16 (4.01–5.54) | 5.26 (4.83–6.13) | 7 → 7 | 1 → 1 | +| 8 / company | 231.79 (221.63–238.96) | 31.11 (24.79–35.86) | 344 → 43 | 64 → 8 | +| 8 / partial | 128.84 (114.42–160.09) | 43.07 (39.36–45.69) | 126 → 42 | 22 → 8 | +| 32 / company | 2,804.04 (2,516.50–4,928.42) | 77.19 (75.56–132.76) | 5,216 → 163 | 1,024 → 32 | +| 32 / partial | 697.51 (676.38–732.57) | 73.92 (73.21–77.78) | 1,464 → 162 | 280 → 32 | +| 100 / company | 24,753.29 (23,849.21–25,019.72) | 246.39 (234.94–278.72) | 50,300 → 503 | 10,000 → 100 | +| 100 / partial | 6,338.58 (6,239.68–6,526.36) | 236.37 (228.40–288.54) | 13,075 → 502 | 2,575 → 100 | + +At 100 agents, returned Drizzle rows fell from 60,200 to 602 for company installs +and from 15,475 to 526 for partial installs. The one-agent controls are neutral, +including a small overlapping timing regression for the partial case. The table +retains every sample range; do not extrapolate a latency curve from these medians. + +The host could not start native PostgreSQL because SysV semaphore allocation +failed, including an unsandboxed low-resource initdb attempt. Local proof used +the existing test constructor seam with a scratch Docker provider: PostgreSQL +18.6 Debian/aarch64 Linux, Node 26.4.0, loopback-only networking, tmpfs storage, +and no simulated database delay. Each provider instance removed only its own +container. Existing database creation, migrations, clients, service code and +cleanup remained in use. The normal embedded package targets Darwin PostgreSQL +18.1; minor version, platform, networking, and compute differ. Defaults included +128 MiB shared buffers, 4 MiB work memory, and JIT enabled. + +## Correctness evidence and limits + +- Focused real-policy regressions cover scoped grants without installs, retained + missing/denied targets, mixed and duplicate candidates, 403 versus reached + 409/500 faults, earlier denial masking an unreachable fault, forced mutation + mode, fresh calls and reauthorization, actor/company isolation, + deliberate within-call staleness, and ordinary fallback preparation counts. +- Real PostgreSQL differential coverage includes company and partial installs, + empty/duplicate candidates, foreign targets, uppercase UUID comparisons, + profile/proposed-provider fallback, and an injected SQL division-by-zero fault + that must escape only when reached. +- The assembled grants GET compares complete old-loop and batch responses across + creator, manager, member, viewer, inactive, and administrator fixtures. It + verifies personal identity filtering, audience disclosure, ordered editable + IDs, and an actual allowed install PUT followed by revocation, fresh denied + capabilities, and a denied PUT with saved installs unchanged. +- Independent review compared the pinned and extracted ordinary evaluator in + 18 context/query/lock-order cases and 18 complete batch cases. Additional + checks covered delegated fallback, recursive profiles/gateways/AI pools, run + capture reuse, a succeeded-run continuation, and fresh revocation. This + supplemental proof uses synthetic database transport, not real lock contention. + +Initial test failures were incorrect fixture expectations: inactive access hides +the resource with 404, database roster order is not insertion order, and the +existing final install mutation gate requires an unscoped agent-configuration +grant. These were corrected in tests. Complete old/new GET equality passed before +these extra assertions. No product behavior was changed to satisfy them. + +Final guard review also found a real defect before merge: saved target IDs are +text, and an empty saved target makes the legacy check depend on the current +candidate. An executable regression returned `[false, true]` in the original +service and `[false, false]` in the first batch. Real PostgreSQL reproduced it. +The guard now requires canonical, nonempty retained IDs as well as candidates; +unusual saved selectors keep ordinary evaluation. The fixed path matches both +the complete capabilities and a later reached infrastructure fault. No stored +target is dropped or normalized, and legacy selector semantics stay unchanged. + +The bound is retained-plus-candidate target evaluations, not total CPU, SQL rows, +or endpoint latency. Scoped grant arrays and company hierarchy reads can still +grow inside each policy decision. Recursive and delegated fallback retain their +existing costs. The benchmark measures the descriptive service component; +hosted request attribution and production latency improvement remain unverified. + +Local coverage contains 33 batch regressions and 83 connection-management +integration cases, using disposable Docker PostgreSQL. +The required workspace typecheck and build passed using pinned pnpm 9.15.4. +The standard full Vitest workflow was attempted, then stopped after native +database fixtures failed to initialize. A separately isolated failing native +runner case reproduced the same PostgreSQL startup failure. This is not a full +local test pass; the critical connection suites use the disposable Docker +provider described above, and the complete CI suite remains a separate gate. +Final test/CI outcomes are recorded in [PR #17945](https://github.com/paperclipai/paperclip/pull/17945); this document does not +substitute for a final-head check or deployed acceptance. diff --git a/doc/plans/2026-10-27-connection-grants-capability-batch.md b/doc/plans/2026-10-27-connection-grants-capability-batch.md new file mode 100644 index 00000000000..84cdaac6caa --- /dev/null +++ b/doc/plans/2026-10-27-connection-grants-capability-batch.md @@ -0,0 +1,220 @@ +# Connection-grants capability fan-out: design and safety proof + +Date: 2026-10-27. This records the selected design before implementation. +See [implementation verification](2026-10-27-connection-grants-batch-verification.md) +for measured results and completed acceptance checks. + +## Recommendation + +Add one narrow, service-owned batch operation for describing which agents may +receive a connection installation. Factor the common installed-agent checks out +of the per-candidate loop. Keep authorization policy in its existing owner and +keep actual mutations on the existing fresh authorization path. + +Start with board actors describing a direct, saved, non-AI connection. Preserve +the ordinary evaluator for delegated agents and complex resources until their +additional obligations have executable parity evidence. This is a bounded first +fix, not a general authorization cache or a rewrite of connection permissions. + +The endpoint currently asks, for each candidate agent, whether the actor may +change the connection's installations. Each answer checks **all retained +installed agents**, followed by the candidate if it is not already installed. +A company installation expands to the company roster. For 100 candidates with +100 retained targets, this means 10,000 target evaluations before the other route +work. The proposed eligible path needs 100 retained-target evaluations, with +extra evaluations only for candidates outside that set. + +Those retained checks are real policy obligations. The fix removes their repeated +execution; it must not remove their authority requirements. + +## What the evidence establishes + +The prior observation window contains two successful grants GETs lasting +56.506 and 66.388 seconds. There is no matching request trace attributing their +duration to this loop. See [the measurement record](2026-10-27-next-bottlenecks.md). +The source and local probes establish amplification, not the fraction of hosted +latency it causes or the eventual improvement. + +The production path is: + +- `server/src/routes/tool-access.ts`, `describeConnectionCapabilities`: selects + the company roster and calls `connections:profiles` once per agent, with + mutation authority and `readOnly: false`. +- `server/src/services/connection-management.ts`, `authorize`: resolves the + saved connection, expands its retained installations, then checks the ordered + target set. `capability` converts only errors with status 403 into denials; + other faults escape. +- `server/src/services/authorization.ts`: owns membership and scoped permission + decisions. Some subtree grants can read the whole company hierarchy for each + target, so linear target evaluations do not guarantee linear rows read. + +Three independent designs were compared: structural factoring, general lazy +request caching, and a narrow guarded batch. A separate judge selected the narrow +batch. The final design incorporates the structural candidate's preparation +continuation and differential test approach, plus the cache candidate's careful +first-visit ordering and separate work counters. + +| Local experiment | Observed result | Limit | +| --- | --- | --- | +| Existing exported service, all-installed fixtures | 4 agents caused 16 target reads; 8 caused 64 | Synthetic database transport, not PostgreSQL | +| Handwritten factored terminal phase compared with the current service | Six static output/fault cases matched; target reads fell 16 to 4 and 64 to 8 | Treatment omits preparation and is not the proposed production implementation | +| General read-reuse experiment, 32 agents | Physical transport reads fell 3,168 to 36, but 1,024 target invocations remained | Demonstrates why caching alone can retain quadratic work | +| Existing policy with a grant scoped to one agent and no installations | Named candidate allowed; targetless request denied | Rules out an extra targetless authorization preflight | +| Existing service with an unavailable retained target | Normal capability denied; `mutation: false` shortcut allowed | Rules out bypassing retained installation checks | +| Existing continuation after membership removal | Fresh `reauthorize` denied | Does not prove a future batch's freshness | + +All three local proof scripts were rerun successfully by the primary agent and +the independent judge. They execute unchanged exported services over synthetic +Drizzle transport. They are level-4 evidence for the exercised current-code +facts, not proof of the unimplemented batch, assembled HTTP equivalence, +PostgreSQL performance, or delegated-actor parity. + +## Owning API and eligibility + +The new operation belongs in `connectionManagementService`, beside `capability`. +Its only initial consumer replaces the `editableAgentIds` loop. Use a fixed +`connections:profiles` action and mutation-mode semantics rather than exposing +general action or read-only knobs. Accept the original actor, base connection +resource, ordered candidate IDs, and existing read context. Return positional +capability descriptions; the route derives the existing editable-ID list. + +Do not return an authorization context, audit authority, reusable session, cache +handle, or continuation. Do not change the wire schema. Keep manager checks, +configure/use capabilities, membership gates, grant identity filtering, and +audience disclosure on their existing paths. + +The initial optimized shape is deliberately explicit: + +- Board actor; one direct saved connection whose actual saved purpose is not AI. +- No non-null profile, gateway, routine, project, or issue selector. +- No own `requestedProvider` or `requestedInstallCompanyIds` property, and no + `selfServiceAgentInstalls: true` request. +- Validated, nonempty roster IDs in their original spelling and input order. + Retained target IDs must also be canonical and nonempty. Unusual identifiers + use the ordinary path rather than normalization: a saved empty target is a + candidate-dependent selector in the legacy evaluator. + +Use the saved connection's provider, purpose, and creator derivation, including +its ordinary personal-grant handling. Caller labels cannot establish eligibility. +Unsupported shapes retain the existing per-candidate evaluator. In particular, +AI pools, profile bindings and selectors, current/proposed gateway resources, +and delegated actor contexts keep all recursive checks. + +## Evaluation algorithm + +Extract private preparation, target evaluation, and completion primitives from +the existing evaluator. Both the ordinary and batch paths must consume these +same primitives; do not copy policy into the route or implement a second policy +engine. Ordinary authorization always starts with fresh private state. + +1. An empty candidate list returns an empty result without authorization, as + the current loop does. +2. Begin preparation for the first actual candidate in the current order. + Discover eligibility during the normal saved-connection lookup. The private + preparation frame is consumed exactly once. If ineligible, finish that same + evaluation through the ordinary path, then evaluate the remaining candidates + normally. Do not restart preparation to fall back. +3. For eligible inputs, form the same ordered set of retained installation + targets, including the existing company-install expansion. Evaluate each + retained target once, using the original check order: connection policy, + applicable proposed-provider/resource trust checks, target existence, and + principal-specific target authority. The shared primitive preserves these + obligations even where the initial eligibility guard excludes some branches. +4. A retained-target 403 denies every candidate with the same current denial. + A reached non-403 fault aborts the operation. Do not inspect later targets to + discover a fault the original evaluator would not reach. +5. After the retained prefix succeeds, visit candidates in input order. A + candidate already in the retained set uses the final retained decision, + matching the current ordered-set behavior and success reason. An additional + candidate uses the shared target primitive. Its 403 denies that candidate; + a reached non-403 fault aborts. Return one result per input position. +6. With no retained installations, the candidate is the policy target. Do not + add a targetless preflight: a grant scoped to that candidate may correctly + allow it while denying an unspecified target. + +For N candidate positions and S distinct retained targets, the target phase is +bounded by S + N evaluations, and often fewer when candidates are retained. +This is not a promise of S + N SQL statements. Authorization may still read +membership, grants, and hierarchy for each distinct target; complex fallback +keeps its present cost. Measure those separately before widening the change. + +Avoid eager prefetch and `Promise.all` here. A denied retained target currently +prevents later target lookups. Loading those targets first changes which faults +are reached. Parallelizing the old loop also retains its total work. + +## Two safety facts and the blast radius + +**1. A displayed permission must retain every existing installation obligation +and its own target scope.** The batch cannot turn a denied or absent retained +target into editable siblings, substitute a targetless decision, or reuse an +allowed candidate's decision for a different candidate. Preserve positional +results, reason, operation, ordered deduplication, and the 403/fatal-fault boundary. +Existing-code counterexamples prove these shortcuts are unsafe. Actual old/new +service and route comparisons remain required to prove the proposed change. + +**2. A description must never become reusable mutation authority.** Batch state +exists only inside one call and cannot cross actors, companies, connections, +requests, operations, or human continuations. Actual writes and `reauthorize` +start fresh. The batch returns descriptions only; identity disclosure does not +consume them as authority. + +There is a deliberate consistency change: reused facts and retained decisions +are observed once during the description rather than reread for every candidate. +This is neither a database snapshot nor an atomic revocation boundary. A revoke +or target deletion during the call can leave a briefly stale displayed choice; +a second GET and an actual mutation must observe fresh authority. Sequential +first visits preserve static-fact failure order, not every possible failure or +revocation between removed duplicate reads. Document and test this honestly. + +Delegated agents remain on the ordinary path initially. `captureRunIdentity` +locks and can reconcile pending steering identity; it is not a pure read. A +future extension must prove run ownership, work mode, API-key status, responsible +human and external-human ceilings, and pending-identity behavior. Preserve the +original actor reference because existing read-context reuse checks identity by +reference. Do not memoize capture or attach batch state to that context. + +The intended change touches the connection-management owner, its route consumer, +and focused tests. Existing connection policy is the authority. There is no +schema migration, provider protocol change, background worker, persistent cache, +UI contract change, or blanket creator/manager allow rule in this design. + +## Implementation and acceptance gates + +1. **Shared-helper refactor:** preserve ordinary evaluation before enabling the + batch. Prove preparation runs exactly once for the first candidate on both + eligible and fallback paths. Keep its continuation private and consumed. +2. **Actual service differential:** compare the new production batch against the + old full evaluator with a disposable PostgreSQL fixture. Compare every + positional `allowed`, `reason`, and `operation`, not just editable counts. + Cover empty/duplicate candidates; no, partial, and company installations; + scoped grants with no installations; missing/foreign retained and candidate + targets; local board and Cloud creator, owner, administrator, operator, + ordinary member, viewer, and inactive membership. +3. **Failures and fallback:** retained denial before an unreachable fault; + candidate denial followed by a reached later fault; preparation/target 500 + and relevant reconciliation 409; only 403 converted. Exercise every excluded + shape, including empty AI pools, explicit null selectors, proposed providers, + company-install changes, profile selectors, and linked gateway obligations. +4. **Full grants GET:** compare complete assembled responses, including order, + editable IDs, configure/use flags, personal identity filtering, manager + visibility, audience members, and route access gates. Exercise the existing + connection Permissions and agent installation UI consumers. +5. **Freshness and isolation:** controlled mid-batch revoke/delete; fresh second + GET; GET then revoke then real mutation; delayed human/OAuth continuation; + concurrent actors, companies, and connections. Agent fast-path enablement + additionally requires real run/key/steering and lock-contention fixtures. +6. **Performance:** at roster sizes 1, 8, 32, and 100 or larger, count preparation, + target evaluations, policy decisions, SQL calls, returned rows, and wall time + separately. Test subtree grants and unrelated-request responsiveness. Measure + assembled route phases and database/pool delay before attributing savings. + +After implementation, run the relevant regression suite and the repository's +required typecheck, tests, and build before PR hand-off. Require passing final-head +CI and review before hosted rollout. Repeat acceptance on the exact serving +revision, verify both response behavior and latency with enough samples, and +retain a way to restore the ordinary evaluator if parity fails. + +This design pass changed no application behavior and performed no hosted mutation. +It establishes a preferred implementation and executable current-code safety +constraints. Complete batch parity and production latency improvement remain +unproved until the implementation and acceptance gates above pass. diff --git a/doc/plans/2026-10-27-next-bottlenecks.md b/doc/plans/2026-10-27-next-bottlenecks.md new file mode 100644 index 00000000000..063bca09e4c --- /dev/null +++ b/doc/plans/2026-10-27-next-bottlenecks.md @@ -0,0 +1,254 @@ +# Next bottleneck batch + +## Scope and observation + +Find the next bounded batch after the three merged slow-query fixes. This is a +diagnosis pass: rank fresh costs, identify actual callers and existing work, +and define the smallest performance and correctness experiments. No application +code, schema, hosted configuration or deployment changes are part of this pass. + +The reported workspace's Cloud, Neon database/runtime role, and Railway +deployment were revalidated from the live registry. The staging app served +`8548173fc9efdc0fa07644fb92dcc0b6851a95e5` throughout the query window. Upstream +`master` was refreshed to `50b932a142216331b8d476042b454bc123739c40`. Private +resource mapping, normalized SQL and raw logs remain outside Git. + +After that stable measurement window, the final serving check at 18:49:08 UTC +observed revision `6e781e0ab20cdaeb6570cebdb5f14dbb8557ba04`. +The grants route/policy/authorization paths, wake-notice owner and skills +route/service are byte-identical between that source and the measured source. +The counts below describe the earlier window, not a post-rollout measurement. +Re-baseline the assembled application before accepting a later performance fix. + +The fresh query window is 2026-10-27 **18:32:51.270–18:38:07.464 UTC**, or +316.194 seconds. Both snapshots read every statement for the exact application +database OID and runtime-role OID through the existing monitoring view, using +bounded read-only transactions. There is no top-100 or query-family filter. + +- Statistics reset stayed at 14:21:16.446 UTC; eviction count stayed zero. +- There were 4,573 starting and 4,590 ending entries, with no missing entries or + invalid matched counters. Entry-level epochs were checked. +- The 1,147 matched entries with positive call deltas account for 121,299 calls + and 596.418 seconds of summed SQL execution time. They are all top-level + entries, so this total does not add nested statement timings. +- Seventeen new entries contribute 25 calls and 75.941 ms as separate lower + bounds. They are excluded from the matched ranking. +- I/O timing is disabled. Zero timing fields do not establish zero I/O latency. + Point-in-time wait samples cannot establish sustained pressure or its absence. +- The probe comparator passed eight controlled cases, including global/entry + resets, decreasing counters, missing/new entries, evictions and scope changes. + An initial diagnostic assertion about a pooler's default read-only setting + was rejected before capture; the successful probe checks the actual transaction. + +The matched HTTP read contains 997 completed log records from the same deployment +and exact time window, below its 2,000-record cap. One WebSocket connection is +excluded from ordinary request latency. The remaining 996 include 603 HTTP 304 +responses and one HTTP 500 from issue summaries; this is an observed traffic +sample, not a clean acceptance run. Duration units follow +[Railway's log contract](https://docs.railway.com/observability/logs). + +SQL execution time can overlap across requests and background workers. Its sum +is neither elapsed time nor endpoint latency. Honeycomb queries using the +verified host, forwarded host and stack fields found no matching request spans +in the checked dataset/window; the SQL share of these HTTP requests is unproved. + +## Fresh rankings + +| SQL caller | Calls | Mean SQL time | Total SQL time | Returned/affected rows | Disposition | +| --- | ---: | ---: | ---: | ---: | --- | +| Chat lifecycle milestone candidates | 167 | 1,497.65 ms | 250.108 s | 0 | Exact caller overlaps open draft #17606 | +| Terminal workspace last activity | 245 | 591.64 ms | 144.951 s | 245 | No qualified open fix for these reads found | +| Inbound wake-notice candidates | 647 | 48.99 ms | 31.694 s | 0 | Exact duty overlaps open draft #17647 | +| GitHub stale-review repair UPDATE | 12 | 1,658.12 ms | 19.897 s | 0 | Exact duty overlaps open draft #17664 | +| GitHub review source-run fallback | 82 | 230.25 ms | 18.880 s | 1 | Distinct query in GitHub maintenance owner | +| GitHub stopped-response recovery | 12 | 1,340.53 ms | 16.086 s | 120 | Exact duty overlaps open draft #17661 | + +The first two families still account for **66.24%** of matched SQL time after +the earlier query improvements. Their existing optimized SQL shapes are active. +The historical pre-fix fingerprints had no new calls in this window. + +Guarded membership upserts used **41.660 ms across 84 calls**. The advisory-lock +statement with a large historical maximum used **0.950 ms across 174 calls**. +Neither historical cumulative total belongs on the new bottleneck shortlist. + +| Ordinary HTTP route | Requests | Mean | Sample p95 | Maximum | Relevant result | +| --- | ---: | ---: | ---: | ---: | --- | +| GET connection grants | 2 | 61.447 s | 66.388 s | 66.388 s | Both 200; minimum 56.506 s; very small sample | +| GET company skills | 20 | 2.733 s | 6.015 s | 13.945 s | All 304 | +| GET company issues | 135 | 1.079 s | 2.957 s | 9.671 s | 112 returned 304 | +| GET sidebar badges | 197 | 482 ms | 1.440 s | 5.851 s | All 304 | +| GET run events | 203 | 346 ms | 1.063 s | 4.273 s | 137 returned 304 | + +Percentiles use nearest rank within the observed sample. Upstream request +duration is almost identical to total duration for these routes. That locates +the observed delay behind the edge, but does not distinguish application CPU, +database round trips, pool wait or other dependencies. + +## Recommended next work + +### 1. Connection-grants capability fan-out + +This is the largest newly observed individual request delay. The route is +`server/src/routes/tool-access.ts:1754`. Listing grants itself uses batched +database reads. The route also describes connection capabilities by inspecting +every company agent. Each inspection asks the connection-management policy for +a capability; its default mutation-authority calculation expands existing +company-wide installations to their agents and checks each target serially. +This creates quadratic authorization fan-out in the reproduced service path. No provider HTTP call +was found in this listing path. The relevant route and policy paths are +unchanged between the measured source, later serving source and refreshed +upstream master. The per-agent helper is `tool-access.ts:665–692`; the repeated +install expansion and target checks are `connection-management.ts:143–158` and +`:225–249`. It deliberately checks retained installed targets as well as the +candidate target. + +The fresh SQL window contains thousands of individually fast membership, +project, issue and run-authority reads. Their counts alone do not attribute them +to these two requests. A scratch local reproduction called the real exported +`connectionManagementService.capability` → `authorize` → authorization service +with a synthetic database transport and an implicit local board actor. Four +agents caused 16 real target probes / 28 transport queries; eight agents caused +64 probes / 88 queries. The outer iteration follows the private route helper's +source; the helper itself was not exported or executed by the harness. This +proves the policy amplification through real code (blast-radius evidence level +4), not its share of the observed hosted request latency or a faster treatment. +The Cloud connection-creator case also reproduced 16 → 64 target probes, with +60 → 216 transport queries and 32 → 128 membership reads. A missing installed +target negative case denied all capabilities. These are synthetic transport +counts through real policy code, not real PostgreSQL or HTTP measurements. +Next measure the assembled route with representative installed scopes and +controlled database round-trip latency. The fix should make capability description share equivalent +request-local facts while retaining each target's authorization decision. + +**Safety fact to prove:** any reused authority fact belongs to the same actor, +company, connection and request; a denied or absent target cannot inherit an +allowed sibling's capability. Cover board, user and agent callers, personal and +company installations, project/resource restrictions, removed grants, missing +agents and mixed allowed/denied targets. Preserve current membership/revocation +reads between requests. Do not make a one-minute route appear fast by skipping +its checks. Do not bypass installed-target checks by passing `mutation:false` or +`readOnly:true`; a missing installed target currently denies every candidate. + +### 2. Qualify existing inbound wake-notice work + +The measured query is owned by +`server/src/services/chat-channels/inbound/wakeup-notices.ts`, in +`createInboundWakeNotices.enqueueInboundWakeupPublications`. This selects +historical notice eligibility; it is not the execution-admission gate. It ran +647 times at approximately 2.05 calls per second and returned no rows. Its +31.694 seconds of SQL time included 12,973,945 shared-buffer hits and no +shared-block reads or temporary blocks in this window. The deployed and current +upstream query paths are identical. + +Direct overlap is confirmed in +[#17647](https://github.com/paperclipai/paperclip/pull/17647), draft head +`ca8f2d17c64fc7cecff4526817d1ac6e1d479ff4`. It changes this exact selector and +removes its normal publication-maintenance call. Its coordinator reconstructs +durable scopes at startup, retains cursors, and consumes committed-change +notifications. Explicit/manual publication drains still call the selector, so +eliminating all 647 observed calls is not established. The PR is stacked on +`codex/sleep-cancel-contention` rather than master. Advance and qualify this +existing work, its dependencies and remaining callers before creating a +competing implementation. This pass did not run its tests or qualify its CI. + +For remaining query work, capture the actual SQL and parameters from this service and +reproduce long retained action/publication/comment history with no pending +notice, followed by a single real eligible notice. Compare plans and all output +columns before deciding whether to narrow the query or wake it from concrete +committed obligations. A quiet path must still reconstruct unresolved work at +startup and recover missed notifications. + +**Safety fact to prove:** a narrower candidate path still finds every notice +authorized by the original action/publication relationship, with unchanged +company scope and deduplication. Cover legacy/native lineage, malformed or +missing context, delayed commits, resolved/expired actions, failed publication, +duplicate delivery and drain/resume. Removing the recurring call requires proof +for every producer and restart/recovery path, not only an empty-queue test. + +### 3. Skills-list reconciliation and repeated resolution + +`server/src/routes/company-skills.ts:467` calls the service list at +`server/src/services/company-skills.ts:3269`. Every request first awaits +`ensureSkillInventoryCurrent` at line 3235: bundled imports, bundled-release +verification, folder reconciliation and local-source inspection. The shared +promise coalesces concurrent work but is removed when it finishes, so a later +request repeats the refresh. A GET can therefore perform filesystem and +database reconciliation before listing any skills. + +The list then resolves every agent's desired skills again for each displayed +skill (`company-skills.ts:3351`). Reference resolution performs searches over +the skill list at line 2152. This is a concrete repeated-work lead, but its share +of the measured 2.733-second HTTP mean remains unproved. These service/route +files are identical in the deployed source, local integration and current +upstream master. + +First profile the real service with representative skill and agent counts, +separating inventory refresh, database reads, desired-skill resolution and +serialization. Compute each agent's desired-key set once per request as the +first small candidate, comparing complete results. Treat any longer-lived +inventory cache as a separate change requiring freshness proof. + +**Safety fact to prove:** each agent contributes at most once to each selected +skill, with identical exact-ID/key/slug resolution and ambiguous/missing alias +handling. Preserve visibility, folder/subtree/search filtering, sort order, +last-editor results and changed local-source discovery. A cached authorized +inventory cannot be reused for another principal's visibility. + +## Existing work and deferred candidates + +- [#17606](https://github.com/paperclipai/paperclip/pull/17606) owns the exact chat + lifecycle milestone path. Its draft branch moves lifecycle work to committed + changes and reconstructs pending work. Audit its producer/recovery proof and + drive its checks before starting a competing polling rewrite. Its draft + status and source design are not proof that it is safe or merge-ready. +- [#17664](https://github.com/paperclipai/paperclip/pull/17664) and + [#17661](https://github.com/paperclipai/paperclip/pull/17661) overlap the two + measured GitHub maintenance duties. The source-run fallback is a distinct + delivery/link/run lookup in `chat-github-checks.ts:181–202`, used when the + review lacks a resolvable run. It still merits a local plan/equality experiment, + coordinated with those owner changes. A previous scalar delivery lookup fix + in #17669 is already deployed and must not be proposed again. +- Workspace activity is called at + `server/src/services/execution-workspaces.ts:3576` through the shared terminal + workspace worker, already migrated by #16282. Its query is at + `server/src/services/git-workspace-expiration.ts:153–160`. + [#17505](https://github.com/paperclipai/paperclip/pull/17505) adjusts cursor and + retry deadlines but leaves this query/call intact. A second pass could batch + repeated activity reads; fresh destructive eligibility, live leases and + concurrent reopen checks must remain authoritative. No qualified open fix + reducing these activity reads was found in the scoped check. +- [#17896](https://github.com/paperclipai/paperclip/pull/17896), + [#17894](https://github.com/paperclipai/paperclip/pull/17894), and + [#17893](https://github.com/paperclipai/paperclip/pull/17893) are already merged + after the measured deployment. They narrow explicit-continuation run reads, + avoid duplicate GitHub webhook body transfer and narrow review-wait JSON. + The final 18:49:08 serving source contains all three. Evaluate their impact in + a new post-rollout window before duplicating those changes. +- [#17907](https://github.com/paperclipai/paperclip/pull/17907) keeps the skill + cache lock alive during slow preparation. Its documented scope does not + explain why preparation is slow or remove the skills-list work above. + +The prior Slack continuation's 353.059-second delay is confirmed by an exact +deployed-version `heartbeat.queue` span without children. Executor startup begins +after that wait. A periodic recovery chain stayed in flight for at least +240.086 seconds and suppressed eight ticks, but an earlier recovery pass reached +its post-resume stage while the continuation was still queued. Source cleanup +had completed before the answer arrived. This narrows the failure to queue / +admission or recovery scheduling; it does not establish the blocking gate, a +lost wake, or SQL as the cause. Instrument gate decisions and recovery stages +before choosing a repair. No task was replayed or re-woken for diagnosis. + +## Finish conditions for the next implementation batch + +Take at most the three priorities above, advancing existing work where it +overlaps. Pin the deployed baseline, prove +the suspected cost with the real caller, define the performance criterion, then +compare complete behavior and adversarial authority/recovery cases. Keep +independent changes in independent PRs with green final-head checks. No migration +or boot script is justified by this discovery pass. Repeat hosted acceptance and +a fresh comparable counter/request window after an authorized rollout. + +The private evidence set retains all raw inputs, comparator validation, exact +source inventories, request summaries and query snapshots. This document is the +sanitized discovery handoff. Correctness of proposed optimizations is unproved +until their real-code comparisons and regressions run. diff --git a/scripts/benchmark-connection-install-capabilities.ts b/scripts/benchmark-connection-install-capabilities.ts new file mode 100644 index 00000000000..0e6f13e8bc9 --- /dev/null +++ b/scripts/benchmark-connection-install-capabilities.ts @@ -0,0 +1,181 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { createHash, randomUUID } from "node:crypto"; +import { readFile, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { createRequire } from "node:module"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { + agents, companies, companyMemberships, createDb, principalPermissionGrants, + startEmbeddedPostgresTestDatabase, toolApplications, toolConnections, toolConnectionInstalls, type Db, +} from "../packages/db/src/index.js"; +import { connectionManagementService } from "../server/src/services/connection-management.js"; +import type { AuthorizationActor } from "../server/src/services/authorization.js"; + +// Run explicitly; this benchmark never reads DATABASE_URL or contacts a hosted DB. +// node --import ./server/node_modules/tsx/dist/esm/index.mjs scripts/benchmark-connection-install-capabilities.ts --baseline-ref= +const baselineRef = process.argv.find(arg => arg.startsWith("--baseline-ref="))?.slice(15); +assert.ok(baselineRef, "Supply an explicit pre-fix --baseline-ref"); +const repository = fileURLToPath(new URL("../", import.meta.url)); +const git = (...args: string[]) => execFileSync("git", args, { cwd: repository, encoding: "utf8" }).trim(); +const baselineRevision = git("rev-parse", "--verify", `${baselineRef}^{commit}`); +const treatmentRevision = git("rev-parse", "HEAD"); +const servicePath = "server/src/services/connection-management.ts"; +const routePath = "server/src/routes/tool-access.ts"; +const runtimeChanges = git("diff", baselineRevision, "--name-only", "--", "server/src", "packages/db/src", "packages/shared/src") + .split("\n").filter(Boolean).filter(name => !name.includes("/__tests__/") && !/\.test\.[cm]?ts$/.test(name)); +assert.deepEqual(runtimeChanges.filter(name => name !== servicePath && name !== routePath), [], + "Baseline imports current dependencies: isolate any additional changed runtime dependencies first"); +assert.equal(git("diff", baselineRevision, "--", "pnpm-lock.yaml"), "", "Dependency lock must match the pinned baseline"); +const oldSource = execFileSync("git", ["show", `${baselineRevision}:${servicePath}`], { cwd: repository, encoding: "utf8" }); +const requireFromServer = createRequire(new URL("../server/package.json", import.meta.url)); +const serviceUrl = new URL(`../${servicePath}`, import.meta.url); +const rewritten = oldSource.replace(/from "([^"]+)"/g, (_match, specifier: string) => { + const resolved = specifier.startsWith(".") + ? fileURLToPath(new URL(specifier, serviceUrl)) + : requireFromServer.resolve(specifier).replace(/\.cjs$/, ".js"); + return `from ${JSON.stringify(resolved)}`; +}); +const baselineDirectory = await mkdtemp(path.join(tmpdir(), "connection-install-baseline-")); +const baselineFile = path.join(baselineDirectory, "connection-management.mts"); +await writeFile(baselineFile, rewritten); +const baselineModule = await import(pathToFileURL(baselineFile).href) as typeof import("../server/src/services/connection-management.js"); + +type Statement = { sql: string; params: unknown[]; calls: number; rows: number }; +type Counts = { calls: number; rows: number; statements: Map }; +let activeCounts: Counts | undefined; +function instrument(db: Db): Db { + return new Proxy(db, { get(target, key, receiver) { + if (key !== "select") return Reflect.get(target, key, receiver); + return (...fields: Parameters) => { + const builder = target.select(...fields); + const from = builder.from.bind(builder); + builder.from = ((...args: Parameters) => { + const query = from(...args), execute = query.execute.bind(query); + query.execute = (async (...executeArgs: Parameters) => { + const counts = activeCounts; + const compiled = counts ? query.toSQL() : undefined; + const result = await execute(...executeArgs); + if (counts && compiled) { + counts.calls++; + const rows = Array.isArray(result) ? result.length : 0; + counts.rows += rows; + const statement = counts.statements.get(compiled.sql) ?? { ...compiled, calls: 0, rows: 0 }; + statement.calls++; + statement.rows += rows; + counts.statements.set(compiled.sql, statement); + } + return result; + }) as typeof execute; + return query; + }) as typeof builder.from; + return builder; + }; + } }); +} +function summary(samples: number[]) { + const sorted = [...samples].sort((a, b) => a - b); + return { samplesMs: samples, medianMs: sorted[Math.floor(sorted.length / 2)], minMs: sorted[0], maxMs: sorted.at(-1) }; +} + +const keepAlive = setInterval(() => {}, 1_000); +let database: Awaited> | undefined; +let db: Db | undefined; +try { + database = await startEmbeddedPostgresTestDatabase("connection-install-benchmark-"); + db = createDb(database.connectionString); + const observedDb = instrument(db); + const old = baselineModule.connectionManagementService(observedDb); + const treatment = connectionManagementService(observedDb); + assert.equal(typeof treatment.describeAgentInstallCapabilities, "function", "Implement the production batch first"); + const results = []; + for (const n of [1, 8, 32, 100]) { + for (const installs of ["company", "partial"] as const) { + const companyId = randomUUID(), connectionId = randomUUID(), applicationId = randomUUID(), userId = randomUUID(); + const candidateIds = Array.from({ length: n }, () => randomUUID()); + await db.insert(companies).values({ id: companyId, name: `Capability benchmark ${n} ${installs}`, issuePrefix: `B${results.length}` }); + await db.insert(agents).values(candidateIds.map((id, index) => ({ id, companyId, name: `Agent ${index}`, adapterType: "process" }))); + await db.insert(companyMemberships).values({ companyId, principalType: "user", principalId: userId, status: "active", membershipRole: "member" }); + await db.insert(principalPermissionGrants).values({ companyId, principalType: "user", principalId: userId, + permissionKey: "tools:manage_profiles", scope: { targetAgentIds: candidateIds } }); + await db.insert(toolApplications).values({ id: applicationId, companyId, name: "Synthetic benchmark", type: "mcp_http" }); + await db.insert(toolConnections).values({ id: connectionId, applicationId, companyId, uid: connectionId, + name: "Synthetic benchmark", transport: "mcp_remote", authKind: "none", config: { provider: "fixture" } }); + await db.insert(toolConnectionInstalls).values(installs === "company" + ? [{ companyId, connectionId, targetType: "company", targetId: companyId }] + : candidateIds.slice(0, Math.max(1, Math.floor(n / 4))).map(targetId => ({ companyId, connectionId, targetType: "agent" as const, targetId }))); + await db.$client.unsafe("ANALYZE"); + const actor: AuthorizationActor = { type: "board", source: "cloud_tenant", userId }; + const resource = { type: "connection_management" as const, companyId, connectionId, operation: "configure", readOnly: false }; + const baseline = async () => { + const capabilities = []; + for (const targetAgentId of candidateIds) capabilities.push(await old.capability(actor, "connections:profiles", { ...resource, targetAgentId })); + return capabilities; + }; + const batch = () => treatment.describeAgentInstallCapabilities(actor, resource, candidateIds); + const samples = { baseline: [] as number[], treatment: [] as number[] }; + const counters: Record = {}; + let captured: Record = {}; + for (let iteration = 0; iteration < 7; iteration++) { + const order = iteration % 2 ? ["treatment", "baseline"] as const : ["baseline", "treatment"] as const; + const outputs: Record = {}; + for (const name of order) { + activeCounts = { calls: 0, rows: 0, statements: new Map() }; + const start = performance.now(); + outputs[name] = await (name === "baseline" ? baseline() : batch()); + const elapsed = performance.now() - start; + const counts = activeCounts; + activeCounts = undefined; + if (iteration >= 2) samples[name].push(elapsed); + const previous = counters[name]; + const current = { calls: counts.calls, rows: counts.rows }; + if (previous) assert.deepEqual(current, previous, "Stable fixture must generate stable work counts"); + counters[name] = current; + if (iteration === 6) captured[name] = [...counts.statements.values()]; + } + assert.deepEqual(outputs.treatment, outputs.baseline, `${installs} N=${n}: full positional capabilities`); + assert.equal((outputs.baseline as Array<{ allowed: boolean }>).length, n); + assert.ok((outputs.baseline as Array<{ allowed: boolean }>).every(capability => capability.allowed), "The benchmark must exercise every authorized retained target"); + } + assert.ok(counters.treatment.calls < counters.baseline.calls || n === 1, "Batch must remove repeated SQL"); + // This is a call-fan-out change. Individual statement plans are controls, + // not the explanation of the gain; capture them outside measured samples. + const plans = []; + for (const statement of captured.treatment) { + const [plan] = await db.$client.unsafe(`EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) ${statement.sql}`, statement.params as never[]); + plans.push({ ...statement, plan: plan["QUERY PLAN"] }); + } + const before = summary(samples.baseline), after = summary(samples.treatment); + const targetLookups = (statements: Statement[]) => statements.filter(statement => statement.sql.includes('"agents"."id" =')).reduce((sum, statement) => sum + statement.calls, 0); + const expectedRetained = installs === "company" ? n : Math.max(1, Math.floor(n / 4)); + assert.equal(targetLookups(captured.baseline), n * expectedRetained + n - expectedRetained); + assert.equal(targetLookups(captured.treatment), n); + const result = { n, installs, principal: "cloud noncreator member with target-scoped grant", completePositionalEquality: true, + baseline: { ...before, ...counters.baseline, targetLookups: targetLookups(captured.baseline) }, + treatment: { ...after, ...counters.treatment, targetLookups: targetLookups(captured.treatment) }, + speedup: before.medianMs! / after.medianMs!, statements: captured, treatmentPlans: plans }; + results.push(result); + console.error(JSON.stringify({ n, installs, baselineMs: before.medianMs, treatmentMs: after.medianMs, counts: counters })); + } + } + const [version] = await db.$client.unsafe("SELECT version() AS version"); + const settings = await db.$client.unsafe("SELECT name, setting, unit FROM pg_settings WHERE name IN ('shared_buffers','work_mem','effective_cache_size','jit') ORDER BY name"); + console.log(JSON.stringify({ baselineRevision, treatmentRevision, + benchmarkSourceSha256: createHash("sha256").update(await readFile(fileURLToPath(import.meta.url))).digest("hex"), + treatmentSourceSha256: createHash("sha256").update(await readFile(new URL(`../${servicePath}`, import.meta.url))).digest("hex"), + uncommittedDiffSha256: createHash("sha256").update(git("diff", "HEAD")).digest("hex"), + dependencyLockfileSha256: createHash("sha256").update(await readFile(new URL("../pnpm-lock.yaml", import.meta.url))).digest("hex"), + baselineIsolation: { copiedModule: servicePath, imports: "Absolute current paths; all other server/shared/db runtime sources and lockfile verified unchanged from baseline", runtimeChanges }, + node: process.version, postgres: version.version, settings, warmups: 2, samples: 5, alternatingOrder: true, + measurement: "Real service evaluation with real local PostgreSQL; select statement executions and returned Drizzle rows; no simulated transport latency", + limits: "Descriptive service batch only; no hosted latency attribution, agent fast path, route timing, concurrent revocation, or subtree hierarchy scaling claim", + results }, null, 2)); +} finally { + activeCounts = undefined; + try { + await db?.$client.end({ timeout: 0 }); + await database?.cleanup(); + await rm(baselineDirectory, { recursive: true, force: true }); + } finally { clearInterval(keepAlive); } +} diff --git a/server/src/__tests__/connection-install-capabilities.test.ts b/server/src/__tests__/connection-install-capabilities.test.ts new file mode 100644 index 00000000000..dc330ee227b --- /dev/null +++ b/server/src/__tests__/connection-install-capabilities.test.ts @@ -0,0 +1,417 @@ +import { describe, expect, it } from "vitest"; +import { PgDialect } from "drizzle-orm/pg-core"; +import type { SQL } from "drizzle-orm"; +import { + agents, + aiConnectionPools, + companyMemberships, + connectionGrants, + principalPermissionGrants, + routines, + toolConnections, + toolConnectionInstalls, + toolMcpGateways, + toolProfiles, + type Db, +} from "@paperclipai/db"; +import { connectionManagementService } from "../services/connection-management.js"; +import type { AuthorizationActor } from "../services/authorization.js"; +import type { ConnectionManagementResource } from "../services/connection-management-policy.js"; + +const companyId = "00000000-0000-4000-8000-000000000001"; +const connectionId = "00000000-0000-4000-8000-000000000002"; +const userId = "capability-test-user"; +const agentId = (index: number) => `00000000-0000-4000-8000-${String(index + 10).padStart(12, "0")}`; +const actor: AuthorizationActor = { type: "board", source: "cloud_tenant", userId }; +const localActor: AuthorizationActor = { type: "board", source: "local_implicit" }; +const resource: ConnectionManagementResource = { + type: "connection_management", companyId, connectionId, operation: "configure", readOnly: false, +}; +const dialect = new PgDialect(); + +/** Only the query transport is synthetic. Both paths use the real services and policy evaluator. */ +function fixture(count = 3) { + const agentRows = Array.from({ length: count }, (_, index) => ({ + id: agentId(index), companyId, status: "idle", role: "engineer", permissions: {}, reportsTo: null, + })); + const state = { + activeMembership: true, + membershipRole: "member", + creatorId: userId, + connectionPurpose: "tool", + provider: null as string | null, + connectionExists: true, + missingConnections: new Set(), + agentStatus: "idle", + installs: [] as Array<{ targetType: string; targetId: string }>, + missingAgents: new Set(), + targetErrors: new Map(), + grants: new Map | null>(), + poolMembers: [] as Array<{ binding: { connectionId: string } }>, + afterTargetRead: undefined as ((targetId: string) => void) | undefined, + }; + const reads = { total: 0, targetIds: [] as string[], connections: 0, pools: 0, profiles: 0, gateways: 0 }; + const db = { + select() { + return { + from(table: unknown) { + return { + async where(condition: SQL) { + reads.total++; + const query = dialect.sqlToQuery(condition); + if (table === toolConnections) { + reads.connections++; + const selectedId = String(query.params[0]); + return state.connectionExists && query.params[1] === companyId && !state.missingConnections.has(selectedId) ? [{ + id: selectedId, companyId, createdByUserId: state.creatorId, + connectionPurpose: selectedId === connectionId ? state.connectionPurpose : "tool", + credentialPolicy: "shared", config: state.provider ? { provider: state.provider } : {}, transportConfig: {}, + }] : []; + } + if (table === toolConnectionInstalls) return state.installs; + if (table === companyMemberships) return state.activeMembership && query.params[0] === companyId + && query.params[2] === userId ? [{ + companyId, principalType: "user", principalId: userId, + status: "active", membershipRole: state.membershipRole, + }] : []; + if (table === principalPermissionGrants) { + const permissionKey = String(query.params[3]); + return state.grants.has(permissionKey) ? [{ + companyId, principalType: "user", principalId: userId, + permissionKey, scope: state.grants.get(permissionKey), + }] : []; + } + if (table === connectionGrants) return []; + if (table === aiConnectionPools) { + reads.pools++; + return [{ id: connectionId, config: { members: state.poolMembers } }]; + } + if (table === toolProfiles) { reads.profiles++; return []; } + if (table === toolMcpGateways) { reads.gateways++; return []; } + if (table === routines) return []; + if (table === agents) { + if (!query.sql.includes('"agents"."id" =')) return agentRows; + const targetId = String(query.params[0]); + reads.targetIds.push(targetId); + const error = state.targetErrors.get(targetId); + if (error) throw error; + const result = state.missingAgents.has(targetId) ? [] : agentRows + .filter(row => row.id === targetId) + .map(row => ({ ...row, status: state.agentStatus })); + state.afterTargetRead?.(targetId); + return result; + } + throw new Error(`Unexpected capability fixture query: ${query.sql}`); + }, + }; + }, + }; + }, + } as unknown as Db; + const management = connectionManagementService(db); + const candidates = agentRows.map(row => row.id); + const baseline = async ( + ids = candidates, + probeResource = resource, + probeActor = actor, + ) => { + const results = []; + for (const targetAgentId of ids) { + results.push(await management.capability(probeActor, "connections:profiles", { + ...probeResource, readOnly: false, targetAgentId, + })); + } + return results; + }; + return { state, reads, management, candidates, baseline }; +} + +describe("connection agent-install capability batches", () => { + it("does no database work for an empty candidate list", async () => { + const f = fixture(); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, [])).toEqual([]); + expect(f.reads.total).toBe(0); + }); + + it("preserves candidate order, duplicate results and target-scoped grants without retained installs", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", { targetAgentIds: [agentId(1)] }); + const candidates = [agentId(1), agentId(0), agentId(1), agentId(2)]; + const expected = await f.baseline(candidates); + expect(expected.map(result => result.allowed)).toEqual([true, false, true, false]); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, candidates)).toEqual(expected); + }); + + it("preserves a mixture of existing and missing candidates", async () => { + const f = fixture(); + const candidates = [agentId(1), agentId(99), agentId(0)]; + const expected = await f.baseline(candidates); + expect(expected.map(result => result.allowed)).toEqual([true, false, true]); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, candidates)).toEqual(expected); + }); + + it("denies every candidate when a retained installed target is unavailable", async () => { + const f = fixture(); + f.state.installs = [{ targetType: "agent", targetId: agentId(99) }]; + const expected = await f.baseline(); + expect(expected.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + }); + + it("does not hide a retained-target policy denial behind candidate-specific permission", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", { targetAgentIds: [agentId(0)] }); + f.state.installs = [{ targetType: "agent", targetId: agentId(1) }]; + const expected = await f.baseline([agentId(0)]); + expect(expected[0]).toMatchObject({ allowed: false }); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)])).toEqual(expected); + }); + + it("preserves candidate-dependent policy checks for an empty retained agent selector", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", { targetAgentIds: [agentId(1)] }); + f.state.installs = [{ targetType: "agent", targetId: "" }]; + const expected = await f.baseline(); + expect(expected.map(result => result.allowed)).toEqual([false, true, false]); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + const laterFault = new Error("later allowed candidate database fault"); + f.state.targetErrors.set(agentId(1), laterFault); + await expect(f.baseline()).rejects.toBe(laterFault); + await expect(f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).rejects.toBe(laterFault); + }); + + it("preserves output for retained candidates, new candidates and duplicates in the same batch", async () => { + const f = fixture(); + f.state.installs = [{ targetType: "agent", targetId: agentId(0) }]; + const candidates = [agentId(0), agentId(2), agentId(0), agentId(99), agentId(1)]; + const expected = await f.baseline(candidates); + expect(expected.map(result => result.allowed)).toEqual([true, true, true, false, true]); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, candidates)).toEqual(expected); + }); + + it("stops at an earlier retained denial without visiting a later infrastructure fault", async () => { + const f = fixture(); + f.state.installs = [ + { targetType: "agent", targetId: agentId(99) }, + { targetType: "agent", targetId: agentId(1) }, + ]; + f.state.targetErrors.set(agentId(1), new Error("later database fault")); + const expected = await f.baseline(); + f.reads.targetIds.length = 0; + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + expect(f.reads.targetIds).not.toContain(agentId(1)); + }); + + it("propagates an earlier retained infrastructure fault before any later denial", async () => { + const f = fixture(); + f.state.installs = [ + { targetType: "agent", targetId: agentId(0) }, + { targetType: "agent", targetId: agentId(99) }, + ]; + const fault = new Error("first database fault"); + f.state.targetErrors.set(agentId(0), fault); + await expect(f.baseline()).rejects.toBe(fault); + f.reads.targetIds.length = 0; + await expect(f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).rejects.toBe(fault); + expect(f.reads.targetIds).toEqual([agentId(0)]); + }); + + it("converts only 403 faults to denied descriptions and continues in candidate order", async () => { + const f = fixture(); + f.state.targetErrors.set(agentId(1), Object.assign(new Error("synthetic forbidden target"), { status: 403 })); + const expected = await f.baseline(); + expect(expected.map(result => result.allowed)).toEqual([true, false, true]); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + }); + + it.each([409, 500])("propagates a %s fault without evaluating later candidates", async status => { + const f = fixture(); + const fault = Object.assign(new Error("synthetic infrastructure failure"), { status }); + f.state.targetErrors.set(agentId(1), fault); + await expect(f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).rejects.toBe(fault); + expect(f.reads.targetIds).not.toContain(agentId(2)); + }); + + it("always probes mutation authority even when a caller supplies readOnly or mutation:false", async () => { + const f = fixture(); + f.state.installs = [{ targetType: "agent", targetId: agentId(99) }]; + expect(await f.management.capability(localActor, "connections:profiles", { + ...resource, targetAgentId: agentId(0), + }, { mutation: false })).toMatchObject({ allowed: true }); + const extraCallerOptions = { mutation: false, readContext: undefined }; + expect(await f.management.describeAgentInstallCapabilities(localActor, { ...resource, readOnly: true }, + [agentId(0)], extraCallerOptions)).toEqual(await f.baseline([agentId(0)], resource, localActor)); + }); + + it("ignores a caller's targetAgentId when evaluating each supplied candidate", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", { targetAgentIds: [agentId(0)] }); + expect(await f.management.describeAgentInstallCapabilities(actor, { ...resource, targetAgentId: agentId(0) }, + [agentId(1)])).toEqual(await f.baseline([agentId(1)])); + }); + + it("keeps later batches and actual authorization fresh after membership revocation", async () => { + const f = fixture(); + const description = await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]); + expect(description[0]).toMatchObject({ allowed: true }); + expect(description[0]).not.toHaveProperty("reauthorize"); + const context = await f.management.authorize(actor, "connections:profiles", { ...resource, targetAgentId: agentId(0) }); + f.state.activeMembership = false; + expect(await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)])) + .toEqual(await f.baseline([agentId(0)])); + expect((await f.baseline([agentId(0)]))[0]).toMatchObject({ allowed: false }); + await expect(f.management.authorize(actor, "connections:profiles", { ...resource, targetAgentId: agentId(0) })) + .rejects.toMatchObject({ status: 403 }); + await expect(context.reauthorize()).rejects.toMatchObject({ status: 403 }); + }); + + it("reloads target-scoped grants on the next batch", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", { targetAgentIds: [agentId(0)] }); + expect((await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]))[0].allowed).toBe(true); + f.state.grants.delete("tools:manage_profiles"); + expect((await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]))[0].allowed).toBe(false); + }); + + it("reloads a deleted connection on the next batch", async () => { + const f = fixture(); + expect((await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]))[0].allowed).toBe(true); + f.state.connectionExists = false; + const result = await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]); + expect(result).toEqual(await f.baseline([agentId(0)])); + expect(result[0]).toMatchObject({ allowed: false, reason: "Connection is unavailable in this company" }); + }); + + it("does not reuse a prior actor's or company's descriptions", async () => { + const f = fixture(); + expect((await f.management.describeAgentInstallCapabilities(actor, resource, [agentId(0)]))[0].allowed).toBe(true); + const otherActor: AuthorizationActor = { ...actor, userId: "another-user" }; + const otherResource = { ...resource, companyId: agentId(98) }; + const actorResult = await f.management.describeAgentInstallCapabilities(otherActor, resource, [agentId(0)]); + const companyResult = await f.management.describeAgentInstallCapabilities(actor, otherResource, [agentId(0)]); + expect(actorResult).toEqual(await f.baseline([agentId(0)], resource, otherActor)); + expect(companyResult).toEqual(await f.baseline([agentId(0)], otherResource)); + expect(actorResult[0].allowed).toBe(false); + expect(companyResult[0].allowed).toBe(false); + }); + + it("allows descriptive staleness after a retained check while fresh authorization observes deletion", async () => { + const f = fixture(2); + f.state.installs = [{ targetType: "agent", targetId: agentId(0) }]; + const targetResource = { ...resource, targetAgentId: agentId(1) }; + const context = await f.management.authorize(actor, "connections:profiles", targetResource); + f.state.afterTargetRead = targetId => { + if (targetId === agentId(0)) f.state.missingAgents.add(targetId); + }; + const described = await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates); + expect(described.map(result => result.allowed)).toEqual([true, true]); + await expect(f.management.authorize(actor, "connections:profiles", targetResource)) + .rejects.toMatchObject({ status: 403 }); + await expect(context.reauthorize()).rejects.toMatchObject({ status: 403 }); + const freshDescriptions = await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates); + expect(freshDescriptions.every(result => !result.allowed)).toBe(true); + }); + + it.each([4, 8])("checks company-installed targets once instead of N squared for N=%s", async count => { + const f = fixture(count); + f.state.installs = [{ targetType: "company", targetId: companyId }]; + const expected = await f.baseline(); + expect(f.reads.targetIds).toHaveLength(count * count); + f.reads.targetIds.length = 0; + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + expect(f.reads.targetIds).toHaveLength(count); + expect(new Set(f.reads.targetIds)).toEqual(new Set(f.candidates)); + }); + + it.each([ + "AAAAAAAA-0000-4000-8000-000000000001", + "", + "not-a-uuid", + ])("uses ordinary evaluation without repeating preparation for candidate %j", async candidate => { + const f = fixture(2); + f.state.installs = [{ targetType: "company", targetId: companyId }]; + const candidates = [agentId(0), candidate, agentId(1)]; + const expected = await f.baseline(candidates); + // The ordinary evaluator treats an empty selector as omitted. + expect(expected.map(result => result.allowed)).toEqual([true, candidate === "", true]); + f.reads.connections = 0; + expect(await f.management.describeAgentInstallCapabilities(actor, resource, candidates)).toEqual(expected); + expect(f.reads.connections).toBe(candidates.length); + }); + + it.each([ + "AAAAAAAA-0000-4000-8000-000000000001", + "not-a-uuid", + ])("uses ordinary evaluation without repeating preparation for retained selector %j", async targetId => { + const f = fixture(); + f.state.installs = [{ targetType: "agent", targetId }]; + const expected = await f.baseline(); + expect(expected.every(result => !result.allowed)).toBe(true); + f.reads.connections = 0; + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + expect(f.reads.connections).toBe(f.candidates.length); + }); + + it.each(["profileId", "gatewayId", "routineId"] as const)("retains ordinary denial behavior for a missing %s selector", async selector => { + const f = fixture(); + const selected = { ...resource, [selector]: agentId(99) }; + const expected = await f.baseline(f.candidates, selected); + expect(expected.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, selected, f.candidates)).toEqual(expected); + }); + + it("retains recursive AI-pool checks through the ordinary path", async () => { + const f = fixture(2); + f.state.connectionPurpose = "ai"; + f.state.poolMembers = [{ binding: { connectionId: agentId(99) } }]; + const expected = await f.baseline(); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(expected); + expect(f.reads.pools).toBeGreaterThan(0); + f.state.missingConnections.add(agentId(99)); + const denied = await f.baseline(); + expect(denied.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, resource, f.candidates)).toEqual(denied); + }); + + it("retains proposed-provider scope checks through the ordinary path", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.provider = "current-provider"; + f.state.grants.set("tools:manage_profiles", { providers: ["current-provider"] }); + const selected = { ...resource, requestedProvider: "replacement-provider" }; + const expected = await f.baseline(f.candidates, selected); + expect(expected.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, selected, f.candidates)).toEqual(expected); + f.state.grants.set("tools:manage_profiles", { providers: ["current-provider", "replacement-provider"] }); + const allowed = await f.baseline(f.candidates, selected); + expect(allowed.every(result => result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, selected, f.candidates)).toEqual(allowed); + }); + + it("retains configure escalation for a proposed company installation", async () => { + const f = fixture(); + f.state.creatorId = "another-user"; + f.state.grants.set("tools:manage_profiles", null); + expect((await f.baseline())[0].allowed).toBe(true); + const selected = { ...resource, requestedInstallCompanyIds: [companyId] }; + const expected = await f.baseline(f.candidates, selected); + expect(expected.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(actor, selected, f.candidates)).toEqual(expected); + }); + + it("retains inactive-agent rejection through the ordinary path", async () => { + const f = fixture(); + f.state.agentStatus = "paused"; + const agentActor: AuthorizationActor = { + type: "agent", source: "agent_key", companyId, agentId: agentId(0), onBehalfOfUserId: userId, + }; + const expected = await f.baseline(f.candidates, resource, agentActor); + expect(expected.every(result => !result.allowed)).toBe(true); + expect(await f.management.describeAgentInstallCapabilities(agentActor, resource, f.candidates)).toEqual(expected); + }); +}); diff --git a/server/src/__tests__/connection-management.integration.test.ts b/server/src/__tests__/connection-management.integration.test.ts index 2a91b321306..bbf43adfc18 100644 --- a/server/src/__tests__/connection-management.integration.test.ts +++ b/server/src/__tests__/connection-management.integration.test.ts @@ -6,7 +6,7 @@ import { join } from "node:path"; import { createServer } from "node:http"; import { and, eq, sql } from "drizzle-orm"; import { beforeAll, afterAll, describe, expect, it, vi } from "vitest"; -import { createDb, activityLog, agentApiKeys, companies, companySecrets, companySecretVersions, chatConversations, chatDeliveries, chatEndpoints, chatExternalPrincipals, chatIdentityLinks, toolPolicies, toolRateLimitCounters, connectionGrants, connectionGrantDelegations, toolAccessAuditEvents, toolOauthStates, issueThreadInteractions, connectionIntentDeliveries, agents, companyMemberships, heartbeatRuns, issues, principalPermissionGrants, projects, toolConnections, toolMcpGateways, toolMcpGatewayTokens, toolActionRequests, toolCatalogEntries, toolInvocations, toolCallEvents, toolProfileBindings, plugins, adapterAuthSessions, environments, routines, chatGitHubReviews } from "@paperclipai/db"; +import { createDb, authUsers, activityLog, agentApiKeys, companies, companySecrets, companySecretVersions, chatConversations, chatDeliveries, chatEndpoints, chatExternalPrincipals, chatIdentityLinks, toolPolicies, toolRateLimitCounters, connectionGrants, connectionGrantMembers, connectionGrantDelegations, toolAccessAuditEvents, toolOauthStates, issueThreadInteractions, connectionIntentDeliveries, agents, companyMemberships, heartbeatRuns, issues, principalPermissionGrants, projects, toolApplications, toolConnections, toolConnectionInstalls, toolMcpGateways, toolMcpGatewayTokens, toolActionRequests, toolCatalogEntries, toolInvocations, toolCallEvents, toolProfiles, toolProfileBindings, plugins, adapterAuthSessions, environments, routines, chatGitHubReviews } from "@paperclipai/db"; import { findSatisfiedToolConnection, satisfiedConnectionIntentService } from "../services/satisfied-connection-intents.js"; import { connectionManagementService, continuingConnectionManagement } from "../services/connection-management.js"; import { redactRunnerApiPayloads } from "../services/native-runtime/runner-api-log-redaction.js"; @@ -25,6 +25,8 @@ import { secretService } from "../services/secrets.js"; import { localEncryptedProvider } from "../secrets/local-encrypted-provider.js"; import { instanceSettingsService } from "../services/instance-settings.js"; import { PaperclipRunnerToolAuthority } from "../services/native-runtime/paperclip-runner-tool-authority.js"; +import type { AuthorizationActor } from "../services/authorization.js"; +import type { ConnectionManagementResource } from "../services/connection-management-policy.js"; vi.mock("../services/agent-lifecycle-driver.js", () => ({ createLifecycleDriver: () => ({ requiredPluginIds: async () => [], runPlugin: async () => "complete", runHost: async () => "complete" }), @@ -85,6 +87,172 @@ describe("connection management with production default agents", () => { authority: new PaperclipRunnerToolAuthority(server.db, { companyId: base.companyId, agentId: agent.id, issueId: base.issueId, runId: base.runId, apiUrl: server.apiUrl, storage: server.storage }) }; } + it("matches ordinary install capabilities on real PostgreSQL for scoped, retained and fallback resources", async () => { + const f = await server.fixture({ connectionScenario: "fresh" }); + const candidateIds = [f.agentId, randomUUID(), randomUUID()]; + await server.db.insert(agents).values(candidateIds.slice(1).map((id, index) => ({ id, companyId: f.companyId, name: `Capability target ${index}`, adapterType: "process" }))); + const [application] = await server.db.insert(toolApplications).values({ companyId: f.companyId, name: "Capability differential", type: "mcp_http" }).returning(); + const [connection] = await server.db.insert(toolConnections).values({ companyId: f.companyId, applicationId: application.id, + uid: randomUUID(), name: "Capability differential", transport: "mcp_remote", authKind: "none", createdByUserId: "different-creator" }).returning(); + await server.db.insert(principalPermissionGrants).values({ companyId: f.companyId, principalType: "user", principalId: f.responsibleUserId!, + permissionKey: "tools:manage_profiles", scope: { targetAgentIds: candidateIds.slice(0, 2) } }); + const service = connectionManagementService(server.db); + const actor: AuthorizationActor = { type: "board", userId: f.responsibleUserId, source: "cloud_tenant" }; + const resource = { type: "connection_management" as const, companyId: f.companyId, connectionId: connection.id, operation: "configure", readOnly: false }; + const compare = async (base: ConnectionManagementResource = resource, candidates: readonly string[] = candidateIds, principal = actor) => { + const baseline = []; + for (const targetAgentId of candidates) baseline.push(await service.capability(principal, "connections:profiles", { ...base, targetAgentId })); + const batch = await service.describeAgentInstallCapabilities(principal, base, candidates); + expect(batch).toEqual(baseline); + return batch; + }; + expect((await compare()).map(result => result.allowed)).toEqual([true, true, false]); + expect(await service.capability(actor, "connections:profiles", resource)).toMatchObject({ allowed: false }); + expect(await compare(resource, [])).toEqual([]); + expect((await compare(resource, [candidateIds[1], candidateIds[0], candidateIds[1]])).map(result => result.allowed)).toEqual([true, true, true]); + // A saved empty text target leaves the candidate ID in the ordinary + // evaluator's resolved policy input, so it is not a shared target prefix. + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: "" }); + const emptyTargetCandidates = [candidateIds[2], candidateIds[0]]; + const emptyTargetBaseline = []; + for (const targetAgentId of emptyTargetCandidates) { + emptyTargetBaseline.push(await service.capability(actor, "connections:profiles", { ...resource, targetAgentId })); + } + expect(emptyTargetBaseline.map(capability => capability.allowed)).toEqual([false, true]); + expect(await service.describeAgentInstallCapabilities(actor, resource, emptyTargetCandidates)).toEqual(emptyTargetBaseline); + await server.db.delete(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connection.id)); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: candidateIds[0] }); + expect((await compare()).map(result => result.allowed)).toEqual([true, true, false]); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: candidateIds[2] }); + expect((await compare()).every(result => !result.allowed)).toBe(true); + await server.db.delete(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connection.id)); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "company", targetId: f.companyId }); + expect((await compare()).every(result => !result.allowed)).toBe(true); + // JavaScript target sets distinguish UUID spelling; PostgreSQL's uuid + // lookup does not. Noncanonical candidates must retain ordinary evaluation. + for (const spellings of [candidateIds.map(id => id.toUpperCase()), + [candidateIds[0].toUpperCase(), candidateIds[1], candidateIds[0], candidateIds[1].toUpperCase(), candidateIds[0].toUpperCase()]]) { + expect((await compare(resource, spellings, { type: "board", source: "local_implicit" })).map(result => result.allowed)) + .toEqual(spellings.map(() => true)); + } + await server.db.delete(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connection.id)); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: randomUUID() }); + expect((await compare(resource, candidateIds, { type: "board", source: "local_implicit" })).map(result => result.reason)) + .toEqual(candidateIds.map(() => "Target agent is outside this company")); + await server.db.delete(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connection.id)); + const [foreign] = await server.db.insert(agents).values({ companyId: f.foreignCompanyId, name: "Foreign capability target", adapterType: "process" }).returning(); + expect((await compare(resource, [candidateIds[0], foreign.id], { type: "board", source: "local_implicit" })).map(result => result.allowed)) + .toEqual([true, false]); + const [profile] = await server.db.insert(toolProfiles).values({ companyId: f.companyId, name: "Fallback profile", profileKey: randomUUID(), defaultAction: "deny" }).returning(); + await compare({ ...resource, profileId: profile.id }); + // Proposed changes also stay on the full evaluator, including own-property presence. + await compare({ ...resource, requestedProvider: null }); + const faultingDb = new Proxy(server.db, { get(target, key, receiver) { + if (key !== "select") return Reflect.get(target, key, receiver); + return (...fields: Parameters) => { + const builder = target.select(...fields), from = builder.from.bind(builder); + builder.from = ((...args: Parameters) => { + const query = from(...args), execute = query.execute.bind(query); + query.execute = (async (...executeArgs: Parameters) => { + const compiled = query.toSQL(); + if (compiled.sql.includes('"agents"."id" =') && compiled.params[0] === candidateIds[1]) { + // A real database failure at the visited target boundary, rather + // than a fake permission denial or a mocked query result. + await server.db.$client.unsafe("SELECT 1 / 0"); + } + return execute(...executeArgs); + }) as typeof execute; + return query; + }) as typeof builder.from; + return builder; + }; + } }); + const faulting = connectionManagementService(faultingDb); + const localActor: AuthorizationActor = { type: "board", source: "local_implicit" }; + await expect(faulting.capability(localActor, "connections:profiles", { ...resource, targetAgentId: candidateIds[1] })).rejects.toThrow(/division by zero/); + await expect(faulting.describeAgentInstallCapabilities(localActor, resource, [candidateIds[1]])).rejects.toThrow(/division by zero/); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: randomUUID() }); + const denied = await faulting.capability(localActor, "connections:profiles", { ...resource, targetAgentId: candidateIds[1] }); + expect(denied).toMatchObject({ allowed: false, reason: "Target agent is outside this company" }); + expect(await faulting.describeAgentInstallCapabilities(localActor, resource, [candidateIds[1]])).toEqual([denied]); + }); + + it("preserves the complete grants GET across human roles and denies a real install mutation after revocation", async () => { + const f = await server.fixture({ connectionScenario: "fresh" }); + const candidateIds = [f.agentId, randomUUID(), randomUUID()]; + await server.db.insert(agents).values(candidateIds.slice(1).map((id, index) => ({ id, companyId: f.companyId, name: `Grant target ${index}`, adapterType: "process" }))); + const users = { creator: f.responsibleUserId!, manager: randomUUID(), member: randomUUID(), viewer: randomUUID(), inactive: randomUUID(), + admin: randomUUID(), owner: randomUUID(), operator: randomUUID() }; + for (const [name, userId] of Object.entries(users)) { + if (name !== "creator") { + await server.db.insert(authUsers).values({ id: userId, name, email: `${userId}@fixture.invalid`, emailVerified: true, createdAt: new Date(), updatedAt: new Date() }); + await server.db.insert(companyMemberships).values({ companyId: f.companyId, principalType: "user", principalId: userId, + membershipRole: ["viewer", "admin", "owner", "operator"].includes(name) ? name : "member", status: name === "inactive" ? "inactive" : "active" }); + } + } + for (const permissionKey of ["tools:manage_connections", "tools:manage_profiles", "tools:use", "agents:configure"]) { + await server.db.insert(principalPermissionGrants).values([users.manager, users.admin].map(principalId => ({ companyId: f.companyId, principalType: "user", principalId, permissionKey }))); + } + await server.db.insert(principalPermissionGrants).values(["tools:manage_profiles", "agents:configure"].map(permissionKey => ({ companyId: f.companyId, + principalType: "user", principalId: users.member, permissionKey, + scope: permissionKey === "tools:manage_profiles" ? { targetAgentIds: candidateIds.slice(0, 2) } : null }))); + const [application] = await server.db.insert(toolApplications).values({ companyId: f.companyId, name: "Grants response parity", type: "mcp_http" }).returning(); + const [connection] = await server.db.insert(toolConnections).values({ companyId: f.companyId, applicationId: application.id, + uid: randomUUID(), name: "Grants response parity", transport: "mcp_remote", authKind: "none", createdByUserId: users.creator }).returning(); + const grants = await server.db.insert(connectionGrants).values([ + { companyId: f.companyId, connectionId: connection.id, kind: "organization", createdByUserId: users.creator, isDefault: true }, + ...[users.creator, users.manager, users.member].map((subjectUserId, index) => ({ companyId: f.companyId, connectionId: connection.id, kind: "user" as const, + subjectUserId, createdByUserId: subjectUserId, updatedAt: new Date(`2026-10-2${index + 1}T12:00:00.000Z`) })), + ]).returning(); + await server.db.insert(connectionGrantMembers).values({ companyId: f.companyId, grantId: grants[0].id, subjectType: "user", subjectId: users.member }); + await server.db.insert(toolConnectionInstalls).values({ companyId: f.companyId, connectionId: connection.id, targetType: "agent", targetId: candidateIds[0] }); + const cookies = Object.fromEntries(await Promise.all(Object.entries(users).map(async ([name, userId]) => [name, await server.humanSession(userId)]))); + const request = async (name: keyof typeof users, method = "GET", body?: unknown) => { + const response = await fetch(`${server.apiUrl}/api/tool-connections/${connection.id}/${method === "GET" ? "grants" : "installs"}`, { + method, headers: { cookie: cookies[name], origin: server.apiUrl, "Content-Type": "application/json" }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + return { status: response.status, body: await response.json() }; + }; + const original = connectionManagementModule.connectionManagementService; + const legacy = vi.spyOn(connectionManagementModule, "connectionManagementService").mockImplementation(db => { + const service = original(db); + return { ...service, describeAgentInstallCapabilities: async (actor, base, candidates, options) => { + const result = []; + for (const targetAgentId of candidates) result.push(await service.capability(actor, "connections:profiles", { ...base, targetAgentId, readOnly: false }, options)); + return result; + } }; + }); + const baseline = new Map>>(); + try { + for (const name of Object.keys(users) as Array) baseline.set(name, await request(name)); + } finally { legacy.mockRestore(); } + const actual = new Map>>(); + for (const name of Object.keys(users) as Array) { + actual.set(name, await request(name)); + expect(actual.get(name), name).toEqual(baseline.get(name)); + } + expect(actual.get("manager")).toMatchObject({ status: 200, body: { capabilities: { canConfigure: true, canViewOtherPersonalIdentities: true, editableAgentIds: expect.arrayContaining(candidateIds) } } }); + expect(actual.get("manager")!.body.grants).toHaveLength(4); + expect(actual.get("creator")!.body.capabilities).toMatchObject({ canConfigure: true, canViewOtherPersonalIdentities: false }); + expect(actual.get("creator")!.body.members.length).toBeGreaterThan(0); + expect(actual.get("member")!.body.capabilities).toMatchObject({ canConfigure: false, canViewOtherPersonalIdentities: false, editableAgentIds: expect.arrayContaining(candidateIds.slice(0, 2)) }); + expect(actual.get("member")!.body.capabilities.editableAgentIds).toHaveLength(2); + expect(actual.get("member")!.body.grants.map((grant: { id: string }) => grant.id)).toEqual([grants[0].id, grants[3].id]); + expect(actual.get("member")!.body.members).toEqual([]); + expect(actual.get("viewer")!.body.capabilities).toMatchObject({ canConfigure: false, canManageAgentInstalls: false, editableAgentIds: [] }); + expect(actual.get("inactive")!.status).toBe(404); + const installs = [{ targetType: "agent", targetId: candidateIds[0] }, { targetType: "agent", targetId: candidateIds[1] }]; + const beforeRevoke = await request("member", "PUT", { installs }); + expect(beforeRevoke, JSON.stringify(beforeRevoke)).toMatchObject({ status: 200 }); + await server.db.delete(principalPermissionGrants).where(and(eq(principalPermissionGrants.companyId, f.companyId), eq(principalPermissionGrants.principalId, users.member))); + expect(await request("member")).toMatchObject({ status: 200, body: { capabilities: { editableAgentIds: [], canManageAgentInstalls: false } } }); + expect(await request("member", "PUT", { installs: [{ targetType: "agent", targetId: candidateIds[0] }] })).toMatchObject({ status: 403 }); + const savedInstalls = await server.db.select().from(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connection.id)); + expect(savedInstalls.map(install => install.targetId)).toEqual(expect.arrayContaining(candidateIds.slice(0, 2))); + expect(savedInstalls).toHaveLength(2); + }); + it("creates and configures tool connections and profiles with fresh grants and actual key/JWT authentication", async () => { const f = await fixture(); const grants = await server.db.select().from(principalPermissionGrants).where(eq(principalPermissionGrants.principalId, f.agentId)); diff --git a/server/src/routes/tool-access.ts b/server/src/routes/tool-access.ts index 1218ff4809d..3f3bb760e17 100644 --- a/server/src/routes/tool-access.ts +++ b/server/src/routes/tool-access.ts @@ -687,9 +687,9 @@ function connectorEnrollmentPrincipal(req: Request): string { const editableAgentIds: string[] = []; if (mutationCapable) { const companyAgents = await db.select({ id: agents.id }).from(agents).where(eq(agents.companyId, connection.companyId)); - for (const agent of companyAgents) { - const capability = await management.capability(req.actor, "connections:profiles", { ...resource, targetAgentId: agent.id }, { readContext: connectionRequestContext(req) }); - if (capability.allowed) editableAgentIds.push(agent.id); + const capabilities = await management.describeAgentInstallCapabilities(req.actor, resource, companyAgents.map(agent => agent.id), { readContext: connectionRequestContext(req) }); + for (let i = 0; i < companyAgents.length; i++) { + if (capabilities[i].allowed) editableAgentIds.push(companyAgents[i].id); } } return { diff --git a/server/src/services/connection-management.ts b/server/src/services/connection-management.ts index 0c1740f04ba..cf65d2afb34 100644 --- a/server/src/services/connection-management.ts +++ b/server/src/services/connection-management.ts @@ -20,6 +20,22 @@ export type ConnectionManagementContext = { type CapturedConnectionRun = RunIdentityCapture<"task_authorization">; const connectionReadRuns = new WeakMap(); +type AuthorizationOptions = { targetAgentIds?: string[]; mutation?: boolean; afterHumanAction?: boolean; expandedProfile?: boolean; gatewayContextChecked?: boolean; readContext?: ConnectionManagementContext; capabilityProbe?: boolean }; +type PreparedAuthorization = { + actor: AuthorizationActor; + action: ConnectionManagementAction; + resource: ConnectionManagementResource; + options: AuthorizationOptions; + responsibleUserId: string | null; + workContext: ConnectionManagementContext["workContext"]; + capturedRun: CapturedConnectionRun | undefined; + expandedTargets: Set; + referencedConnections: string[]; + currentGatewayResource: ConnectionManagementResource | undefined; + savedNonAiConnection: boolean; +}; +type ConnectionCapability = { allowed: boolean; reason: string; operation: string }; + export function responsibleConnectionUser(actor: AuthorizationActor): string | null { return (actor.type === "agent" ? actor.onBehalfOfUserId : actor.userId) ?? null; } @@ -28,12 +44,12 @@ export function responsibleConnectionUser(actor: AuthorizationActor): string | n export function connectionManagementService(db: Db) { const authorization = authorizationService(db); - async function authorize( + async function prepareAuthorization( actor: AuthorizationActor, action: ConnectionManagementAction, resource: ConnectionManagementResource, - options: { targetAgentIds?: string[]; mutation?: boolean; afterHumanAction?: boolean; expandedProfile?: boolean; gatewayContextChecked?: boolean; readContext?: ConnectionManagementContext; capabilityProbe?: boolean } = {}, - ): Promise { + options: AuthorizationOptions, + ): Promise { const responsibleUserId = responsibleConnectionUser(actor); let workContext: ConnectionManagementContext["workContext"] = { runId: null, issueId: null, projectId: null }; let capturedRun: CapturedConnectionRun | undefined; @@ -72,6 +88,7 @@ export function connectionManagementService(db: Db) { const expandedTargets = new Set(options.targetAgentIds ?? []); const referencedConnections: string[] = []; let currentGatewayResource: ConnectionManagementResource | undefined; + let savedNonAiConnection = false; if (resource.gatewayId && !options.gatewayContextChecked) { const [gateway] = await db.select().from(toolMcpGateways).where(and(eq(toolMcpGateways.id, resource.gatewayId), eq(toolMcpGateways.companyId, resource.companyId))); if (!gateway) throw forbidden("Gateway is unavailable in this company"); @@ -142,6 +159,7 @@ export function connectionManagementService(db: Db) { if (resource.connectionId) { const [connection] = await db.select().from(toolConnections).where(and(eq(toolConnections.id, resource.connectionId), eq(toolConnections.companyId, resource.companyId))); if (!connection) throw forbidden("Connection is unavailable in this company"); + savedNonAiConnection = connection.connectionPurpose !== "ai"; resource = { ...resource, selfServiceUserId: connection.createdByUserId, purpose: connection.connectionPurpose === "ai" ? "ai" : connection.connectionPurpose === "channel" ? "chat" : "tool", provider: typeof connection.config.sourceTemplateKey === "string" ? connection.config.sourceTemplateKey @@ -222,34 +240,47 @@ export function connectionManagementService(db: Db) { if (issue.workMode === "skill_test" || ((options.mutation ?? action !== "connections:read") && issue.workMode !== "standard")) throw forbidden("Connection changes require a standard task"); } } + return { actor, action, resource, options, responsibleUserId, workContext, capturedRun, + expandedTargets, referencedConnections, currentGatewayResource, savedNonAiConnection }; + } + + async function evaluateTarget(prepared: PreparedAuthorization, targetAgentId: string | undefined): Promise { + const { actor, action, resource, responsibleUserId } = prepared; + const resolved = { ...resource, ...(targetAgentId ? { targetAgentId } : {}) }; + const decision = await authorization.decide({ actor, action, resource: resolved, scope: { ...resolved } }); + if (!decision.allowed) throw forbidden(decision.explanation, { reason: decision.reason, code: decision.code, operation: resource.operation }); + if (Object.hasOwn(resource, "requestedProvider") && resource.requestedProvider !== resource.provider) { + const proposed = { ...resolved, provider: resource.requestedProvider }; + const proposedDecision = await authorization.decide({ actor, action, resource: proposed, scope: { ...proposed } }); + if (!proposedDecision.allowed) throw forbidden(proposedDecision.explanation, { reason: proposedDecision.reason, code: proposedDecision.code, operation: resource.operation }); + } + if (actor.type === "agent" && actor.runId && (resolved.projectId || resolved.issueId)) { + const resourceTrust = await authorization.decide({ actor: { ...actor, runId: undefined }, action, resource: resolved, scope: { ...resolved } }); + if (!resourceTrust.allowed) throw forbidden(resourceTrust.explanation); + } + if (targetAgentId) { + const [target] = await db.select({ id: agents.id }).from(agents).where(and(eq(agents.id, targetAgentId), eq(agents.companyId, resource.companyId))); + if (!target) throw forbidden("Target agent is outside this company"); + // Explicitly enforce both principals even when unrelated authorization is in shadow mode. + // Existing human routes retain their domain-specific target authority. + // Delegation additionally checks both the acting agent and its human. + const principals: AuthorizationActor[] = actor.type === "agent" || resource.selfServiceAgentInstalls ? [actor] : []; + if (actor.type === "agent") principals.push({ type: "board", userId: responsibleUserId, source: "session", ignoreInstanceAdmin: true }); + for (const principal of principals) { + const targetDecision = await authorization.decide({ actor: principal, action: "agent_config:update", resource: { type: "agent", companyId: resource.companyId, agentId: targetAgentId }, scope: { targetAgentId } }); + if (!targetDecision.allowed) throw forbidden(targetDecision.explanation); + } + } + return decision; + } + + async function finishAuthorization(prepared: PreparedAuthorization): Promise { + const { actor, action, resource, options, responsibleUserId, workContext, capturedRun, + expandedTargets, referencedConnections, currentGatewayResource } = prepared; const targets = [...new Set([...expandedTargets, ...(resource.targetAgentId ? [resource.targetAgentId] : [])])]; let decision: AuthorizationDecision | undefined; for (const targetAgentId of targets.length ? targets : [undefined]) { - const resolved = { ...resource, ...(targetAgentId ? { targetAgentId } : {}) }; - decision = await authorization.decide({ actor, action, resource: resolved, scope: { ...resolved } }); - if (!decision.allowed) throw forbidden(decision.explanation, { reason: decision.reason, code: decision.code, operation: resource.operation }); - if (Object.hasOwn(resource, "requestedProvider") && resource.requestedProvider !== resource.provider) { - const proposed = { ...resolved, provider: resource.requestedProvider }; - const proposedDecision = await authorization.decide({ actor, action, resource: proposed, scope: { ...proposed } }); - if (!proposedDecision.allowed) throw forbidden(proposedDecision.explanation, { reason: proposedDecision.reason, code: proposedDecision.code, operation: resource.operation }); - } - if (actor.type === "agent" && actor.runId && (resolved.projectId || resolved.issueId)) { - const resourceTrust = await authorization.decide({ actor: { ...actor, runId: undefined }, action, resource: resolved, scope: { ...resolved } }); - if (!resourceTrust.allowed) throw forbidden(resourceTrust.explanation); - } - if (targetAgentId) { - const [target] = await db.select({ id: agents.id }).from(agents).where(and(eq(agents.id, targetAgentId), eq(agents.companyId, resource.companyId))); - if (!target) throw forbidden("Target agent is outside this company"); - // Explicitly enforce both principals even when unrelated authorization is in shadow mode. - // Existing human routes retain their domain-specific target authority. - // Delegation additionally checks both the acting agent and its human. - const principals: AuthorizationActor[] = actor.type === "agent" || resource.selfServiceAgentInstalls ? [actor] : []; - if (actor.type === "agent") principals.push({ type: "board", userId: responsibleUserId, source: "session", ignoreInstanceAdmin: true }); - for (const principal of principals) { - const targetDecision = await authorization.decide({ actor: principal, action: "agent_config:update", resource: { type: "agent", companyId: resource.companyId, agentId: targetAgentId }, scope: { targetAgentId } }); - if (!targetDecision.allowed) throw forbidden(targetDecision.explanation); - } - } + decision = await evaluateTarget(prepared, targetAgentId); } if (action === "connections:profiles" && resource.requestedInstallCompanyIds && !resource.selfServiceAgentInstalls) { await authorize(actor, "connections:configure", resource, options); @@ -275,6 +306,15 @@ export function connectionManagementService(db: Db) { return context; } + async function authorize( + actor: AuthorizationActor, + action: ConnectionManagementAction, + resource: ConnectionManagementResource, + options: AuthorizationOptions = {}, + ): Promise { + return finishAuthorization(await prepareAuthorization(actor, action, resource, options)); + } + async function authorizeBindingTarget(actor: AuthorizationActor, action: ConnectionManagementAction, resource: ConnectionManagementResource, binding: { targetType: string; targetId: string }, options: NonNullable[3]> = {}): Promise { @@ -303,17 +343,96 @@ export function connectionManagementService(db: Db) { return authorize(actor, action, resolved, checkOptions); } - async function capability(...args: Parameters) { + function deniedCapability(error: unknown, operation: string): ConnectionCapability { + // Database/provider faults are not authorization denials. + if (!(error instanceof Error && "status" in error && error.status === 403)) throw error; + return { allowed: false, reason: error.message, operation }; + } + + async function capability(...args: Parameters): Promise { try { const context = await authorize(args[0], args[1], args[2], { ...args[3], capabilityProbe: true }); return { allowed: true, reason: context.decision.reason, operation: args[2].operation }; } catch (error) { - // Database/provider faults are not authorization denials. - if (!(error instanceof Error && "status" in error && error.status === 403)) throw error; - return { allowed: false, reason: error.message, operation: args[2].operation }; + return deniedCapability(error, args[2].operation); } } - return { authorize, authorizeBindingTarget, capability }; + + /** Descriptions only: retained installation authority is checked once per + * call. Mutations and human continuations always start a fresh authorization. + * Complex resources and delegated actors retain the ordinary evaluator. */ + async function describeAgentInstallCapabilities( + actor: AuthorizationActor, + resource: ConnectionManagementResource, + candidateAgentIds: readonly string[], + options: { readContext?: ConnectionManagementContext } = {}, + ): Promise { + if (!candidateAgentIds.length) return []; + const candidates = [...candidateAgentIds]; + const base = { ...resource, readOnly: false }; + const probeOptions = { readContext: options.readContext, capabilityProbe: true, mutation: true }; + const results: ConnectionCapability[] = []; + const describeRemaining = async (start: number) => { + for (let i = start; i < candidates.length; i++) { + results.push(await capability(actor, "connections:profiles", { ...base, targetAgentId: candidates[i] }, probeOptions)); + } + return results; + }; + + let prepared: PreparedAuthorization; + try { + prepared = await prepareAuthorization(actor, "connections:profiles", { ...base, targetAgentId: candidates[0] }, probeOptions); + } catch (error) { + results.push(deniedCapability(error, base.operation)); + return describeRemaining(1); + } + const resolved = prepared.resource; + const isCanonicalAgentId = (id: string) => isUuidLike(id) && id === id.toLowerCase(); + const eligible = actor.type === "board" && prepared.savedNonAiConnection + && [resolved.profileId, resolved.gatewayId, resolved.routineId, resolved.projectId, resolved.issueId].every(id => id == null) + && !Object.hasOwn(resolved, "requestedProvider") && !Object.hasOwn(resolved, "requestedInstallCompanyIds") + && !resolved.selfServiceAgentInstalls + && !prepared.referencedConnections.length && !prepared.currentGatewayResource + && candidates.every(isCanonicalAgentId) + // A saved empty selector resolves against the current candidate in the + // ordinary evaluator. Such retained checks cannot be shared. + && [...prepared.expandedTargets].every(isCanonicalAgentId); + if (!eligible) { + // Consume the already prepared first evaluation. Restarting it can repeat + // recursive checks or run-identity reconciliation and its locking effects. + try { + const context = await finishAuthorization(prepared); + results.push({ allowed: true, reason: context.decision.reason, operation: base.operation }); + } catch (error) { + results.push(deniedCapability(error, base.operation)); + } + return describeRemaining(1); + } + + let retainedDecision: AuthorizationDecision | undefined; + try { + for (const targetAgentId of prepared.expandedTargets) { + retainedDecision = await evaluateTarget(prepared, targetAgentId); + } + } catch (error) { + const denied = deniedCapability(error, base.operation); + return candidates.map(() => ({ ...denied })); + } + for (const targetAgentId of candidates) { + try { + // With no retained targets, the named candidate is the first policy + // target: a targetless preflight can incorrectly deny scoped grants. + const decision = prepared.expandedTargets.has(targetAgentId) + ? retainedDecision! + : await evaluateTarget(prepared, targetAgentId); + results.push({ allowed: true, reason: decision.reason, operation: base.operation }); + } catch (error) { + results.push(deniedCapability(error, base.operation)); + } + } + return results; + } + return { authorize, authorizeBindingTarget, capability, describeAgentInstallCapabilities }; } /** A pending provider login can finish after a successful yielding run. This