mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 22:13:59 +01:00
1eff07f29e63efa2ce10543aea3e1fdf19c58c1d
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 DottaandPaperclip
|
5d226ca08a |
perf(chat): reuse bound-run authorization query templates (#18966)
This PR depends on #18969. Merge that fixture-only prerequisite first, then retarget this PR to master. No merge or deployment is requested in this work. Its base is `codex/fix-recovery-published-workspace-fixture`; the six-line recovery fixture repair is excluded from this diff. ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - External chat authorization binds each run to current messages and conversation policy. > - Review wait and attention-retention paths repeat the same four-table read. > - Each call rebuilt the Drizzle query even when its SQL shape stayed the same. > - This pull request reuses the compiled template for the exact database handle and lock mode. > - Fresh parameters and current database reads preserve authorization while reducing Node CPU work. ## Linked Issues or Issue Description **What existing behavior does this improve?** The `authorizeChatConversationForBoundRun` links read used by external chat waits and attachment reuse. **Current behavior** At baseline `e54f9fbf828e06e385e0c62dc381ccb46190aa30`, each call rebuilt and serialized a four-table query. The observed staging shape accounted for 16.2% of sampled SQL serialization CPU across 32 samples. This is a share of query compilation CPU, not total server CPU. **Proposed behavior** Compile each read, blocking-lock or NOWAIT variant once per exact live database or transaction object. Bind new company, issue, agent, provider and comment IDs on every execution. Continue all current database reads and authorization checks. **Reason and benefit** Repeated authorized service calls used 26.91% less Node process CPU in the matched local fixture. All seven CPU pairs improved. This reduces repeated compilation on the shared service path. Related: #18896 retains task Retry intent through identity contention. It changes a different workflow and does not reuse this query template. A public issue and PR search found no duplicate template change. ROADMAP.md has no conflicting planned feature. ## What Changed - Keep compiled queries in a `WeakMap` keyed by the exact supplied database or transaction object. Create only the requested lock variant. Never cache a result or authority decision. - Bind the comment list as `ANY(uuid[])`. This supports changing list sizes with only three templates per live handle. Keep all joins, selected fields, policy checks and lock clauses. - Add real PostgreSQL regression tests, a reusable fixture, an actual service benchmark and a dated performance note. ## Verification - Passed 17 real PostgreSQL tests: `node node_modules/vitest/vitest.mjs run --project @paperclipai/server server/src/__tests__/prepared-chat-authorization.integration.test.ts server/src/services/native-runtime/chat-attachment-reuse.test.ts`. - After rebasing, seven selected existing external-chat-wait integration cases passed (266 deselected). They cover small current review waits, answered-chat identity advisory locks, connection-disable serialization and contended endpoint proof retry. - New tests cover changed parameters and list sizes, duplicate/empty inputs, invalid UUID errors, company/agent/provider binding, current setup generation and credential fingerprints, root/transaction/savepoint isolation, rollback, read mode during a held row lock, blocking timeout and NOWAIT contention. - Actual service benchmark: `node --import ./server/node_modules/tsx/dist/loader.mjs server/scripts/benchmark-bound-chat-authorization.ts --baseline-ref=e54f9fbf828e06e385e0c62dc381ccb46190aa30`. Node 24.22.2, macOS arm64, real PostgreSQL, 500 warmup calls per implementation, seven alternating pairs of 500 calls. Every result matched. - Pinned measured treatment `8386eb78d118faaf6c2fd7f7471b38c0f4b2ef54`. Its source is unchanged after rebasing onto master. Median Node CPU per 500 calls: 379.033 ms to 277.048 ms, down 26.91%. Every CPU pair improved by 13.54–33.52%. Median wall time: 742.109 ms to 648.005 ms. One wall pair increased by 0.51%. - A separate diagnostic after timing found five SQL compilations per baseline call and four per warm treatment call. All five database reads still run. - The final four-test PostgreSQL authorization suite passed again after rebasing and strengthening the lock proof. It holds only a message-link row, so principal advisory contention cannot mask a missing joined-query lock. - Independent source review found no correctness issue in exact-handle ownership, fresh binding or the three lock variants. - Preserve setup failures: the first test attempt lacked plugin SDK build output. Shared and SDK builds fixed it. Later attempts could not start embedded PostgreSQL because its local dependency directory lacked bundled native-library aliases. `DYLD_LIBRARY_PATH` did not fix it. Restoring package-local aliases fixed the environment; the final 17 tests then passed. No dependency or lockfile change is included. - The first full typecheck found new test-only errors: PostgreSQL transaction handles lack the root database `$client` type, and runtime setup test data includes fields outside the saved setup interface. Commit `40a4645750` uses the existing explicit transaction test cast and SQL JSON fixtures. No production contract changed. The full check also found local plugin SDK link prerequisites, which the coordinator is repairing separately. - Preserve the first Linux CI failure: [server shard 9](https://github.com/paperclipai/paperclip/actions/runs/41837775564/job/126602280304) failed the new setup-fence fixture's allowed case. The fixture supplied a client `processedAt` while PostgreSQL supplied default `receivedAt`; this could violate the required processing order. The test now uses explicit ordered timestamps and no wall-clock window. The four-test PostgreSQL suite passed again in 19.93 seconds. Production guards were not weakened. - [Server shard 2](https://github.com/paperclipai/paperclip/actions/runs/41837775564/job/126602280291) also failed five workspace-recovery cases with `remote_workspace_published_path_invalid`. Those failures reproduced on both pure master and the exact CI merge with the chat optimization removed. #18969 fixes the canonical path in that separate fixture, with five failing cases before and five passing after. This PR is stacked on that independent repair for final CI. - The additional Paperclip review reported a node-postgres statement-name collision and requested explicit pool cleanup. This repository uses postgres-js; locked Drizzle 0.45.2 accepts the Drizzle label but does not use or forward it. [The review response](https://github.com/paperclipai/paperclip/pull/18966#issuecomment-6632666599) links primary driver source and the database factory. Commit `c5d0d84f36` adds explicit pool teardown in `try/finally` and a real same-transaction `read → blocking → nonblocking → read` case. All five authorization tests passed in 6.47 seconds. The original separate row-lock test remains. Production SQL and code did not change. - Final combined local validation at `4ab58ff70b` passed full `pnpm -r typecheck` (39.864 seconds) and 35 real PostgreSQL cases across the chat, GitHub and shared prepared-query suites (11.378 seconds). Full `pnpm build` passed earlier with byte-identical production source; later changes affect tests only. The production file SHA matches the benchmark and this PR. The original full `pnpm test:run` was interrupted after all six chat shards passed and general tests had partly run, so it is not a full local pass. Final current-head CI provides the broader coverage. A sandbox retry failed with EPERM before assertions; the identical escalated commands then passed. Final head passed CI in [workflow 41843806060](https://github.com/paperclipai/paperclip/actions/runs/41843806060), including all build, typecheck, general, serialized and e2e test gates. The previous head `40a464575009dfc3cce7afa1eb4fffa5540dcaf6` passed Greptile 5/5, security review, typecheck, build and canary validation. The prior stacked head `74cb36370162750f8e135ed091603d3629d23b0d` passed full CI, Greptile 5/5, policy and security checks. Final head `c5d0d84f369d897a66d9b6441bab10cb79f593b9` passed a fresh Greptile 5/5 review with no actionable findings, security and PR policy checks. Its complete [current-head CI workflow passed](https://github.com/paperclipai/paperclip/actions/runs/41843806060). All 54 required CI, Greptile, policy and security checks passed. The optional Paperclip Review check is queued for this head; no current-head score is claimed. Production source SHA-256 remains `72ce9d2317e5c97b36e299e24e30fc1aa63020d8c266e495fc45dbfdd5331e93`, identical to the measured code. ## Risks - `ANY(uuid[])` relies on postgres-js array binding and UUID casting. Real PostgreSQL tests cover single and multiple valid values and invalid UUID errors. - A new transaction or savepoint pays its first compilation cost. The weak cache cannot reuse a root session for it. This benchmark covers repeated calls on one root handle. - The team paused other work during the benchmark. Unrelated Rust compilation was active elsewhere on the host. Process CPU excludes those processes, but host contention can affect caches, scheduling and wall time. This source-mode local result does not prove production request latency or whole-server CPU savings. - No migration or API contract changes. No deployment or merge is included. Monitor authorization failures and lock-contention behavior after an authorized rollout; revert the narrow service change if those regress. ## Model Used OpenAI `gpt-6.1-sol` through Codex, with reasoning, repository editing, tool use and test execution. The host did not expose a context-window size or deployment snapshot ID. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
2cbfc35a07 |
perf(github): reuse the inherited review wait query template (#18965)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - External review waits check current GitHub review and publication bindings. > - The GitHub service rebuilds the same seven-table Drizzle query on each check. > - A sampled production window attributed 6.3% of SQL serialization CPU to this query shape. > - This pull request prepares that query once per exact database or transaction object. > - Each call still reads current provenance, review, connection and publication state. > - The local service benchmark reduces median client CPU per read by 24.35%. ## Linked Issues or Issue Description **What existing behavior does this improve?** Repeated `readInheritedGitHubReviewWait` calls from external review wait reads and attention retention. **Subsystem affected** Server GitHub chat review waits. **Current behavior** Every call constructs and serializes the same joined SELECT. The database driver already supports prepared statements. That setting does not reuse the JavaScript query template. **Proposed behavior** Compile the existing selector once for each supplied database, transaction or savepoint object. Bind company, issue, agent and comment IDs on each execution. Keep every current database read and check. **Reason and benefit** Remove repeated query construction and serialization CPU. Do not cache rows or authorization decisions. Exact open-PR searches found no duplicate query-template change. ## What Changed - Add an exact-handle `WeakMap` for the GitHub review selector and execute it with fresh placeholders. - Preserve the SQL projection, joins, JSON types, latest-review order and all current authority checks. - Extend real PostgreSQL tests for new bindings, warm-cache revocation, latest heads and transaction/savepoint rollback. - Capture prepared executions in the existing test helper. - Add an actual-service benchmark and a report with every timing sample and setup failure. ## Verification - `github-review-wait.test.ts`: 16 tests passed on real PostgreSQL. - `external-chat-wait.integration.test.ts -t 'reads small current external review waits without saved prompts'`: four affected cases passed; 269 unrelated cases skipped. - Pinned baseline: `e54f9fbf828e06e385e0c62dc381ccb46190aa30`. Final source SHA256: `fe647e7fe2aed82609e8f1d66a0109e7a10ee28313ae2ef218ba3ab39b5cd105`. - Exclusive-window benchmark: 500 warmups per version, nine alternating pairs of 100 complete service reads on disposable PostgreSQL. Median client CPU: 1.00336 → 0.75908 ms/read (24.35% less). Median wall time: 2.87089 → 2.38731 ms/read (16.84% less). CPU excludes the PostgreSQL process. - Every generated SQL statement, parameter, query count and complete result matched. Warm joined-selector compilations per ten service reads: 10 → 0. - See `doc/performance/2026-11-04-prepared-github-review-wait.md` for reproduction, all samples and measurement limits. - Setup record: loopback required execution outside the sandbox. The fresh embedded-Postgres dependency lacked macOS dylib aliases; aliases to existing identical bytes restored the fixture. The SDK import needed a current workspace SDK/shared build. A broad external-chat suite run was interrupted and replaced by the affected test filter. Those attempts are not counted as passing suites. - Greptile: 5/5 on current head `d2e6ae8814c3a56c380bd5d60c47e6f98642f085`, with no actionable issues or inline comments. - Current-head CI is green at `d2e6ae8814c3a56c380bd5d60c47e6f98642f085`: 52 successful checks, three intentional skips and the successful Snyk status. This includes typecheck, build, the full test matrix, end-to-end shards and the aggregate gate. - Combined local validation at `4ab58ff70b` includes both measured production templates and final test fixes. Full recursive `pnpm -r typecheck` passed in 39.864 seconds. Three real PostgreSQL files passed 35/35 tests (five chat cases, 16 GitHub review cases and 14 shared prepared-query cases). The combined build passed earlier with the same production template bytes. - The initial combined typecheck failed on a separate chat test typing error and build setup. The later passing run resolves that attempt. The original full local `pnpm test:run` was deliberately interrupted after six chat shards passed and partial general tests ran; it is not a passing full local suite. Each PR has independent current-head CI for broad coverage. The initial loopback sandbox restriction was resolved by an escalated run of the same source. - Local benchmark/test dependencies were locked at the baseline; the rebase adds the upstream lockfile refresh and UI paste fix only. ## Risks - A prepared query captures its session. The exact supplied handle is the cache key so transactions and savepoints do not borrow the root session. - Placeholder mistakes could cross bindings. Tests alternate independent company/issue/agent/comment fixtures on the same handle. - The benchmark has one synthetic eligible fixture with warm sequential reads. It does not predict production endpoint latency or whole-server CPU savings. - No schema, index, database driver setting or public API changes. ## Model Used OpenAI `gpt-6.1-sol` through Codex. The context-window size is not exposed by the session. Used reasoning, tool calls, source editing and code execution. Independent Codex review found no correctness issues. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
dd02595390 |
fix(db): retain hosted census construction callers (#18725)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Operators use a bounded database census to find expensive read paths. > - The hosted server combines application code and capture wrappers in generated files. > - Generic HTTP owners suppressed caller capture. Generated paths also failed the caller allowlist. > - This PR retains trusted query construction locations without source maps. > - Operators can distinguish application query sites while exact owners, privacy limits and capture bounds stay intact. ## Linked Issues or Issue Description Refs #17522, #17564. **What happened?** A real map-free bundle with two application query sites produces one callerless census group. This affects unowned queries and queries under the generic `http.request` owner. **Expected behavior** The census should distinguish trusted query construction sites. It must keep unknown calls unknown and preserve specific semantic owners without stack capture. **Steps to reproduce** Run the committed bundle test and pinned comparison helper. The helper bundles the actual capture module and real Drizzle, with a fake postgres-js client. It does not assign a fake path to an isolated mock caller. **Paperclip version or commit** Control: `09162fe893b464dd2ccdafd14bd3ef1611202a86`. The branch is rebased onto `1e3ab690b072bbf6a1c1f1d0454ea49315b2074c`; neither intervening commit changes this owner. ## What Changed - Keep the exact generic HTTP owner and add bounded caller capture only during the active census window. - Exclude capture wrapper frames with a local V8 stack boundary. Keep one static generated construction anchor for trusted root builders. - Keep unscoped bundled driver and relational paths unknown. Their first frame can belong to the ORM. - Cover exact application lines, alternate parameters, prepared builders, transactions, nested savepoints, privacy and inactive windows in a real bundled scaffold. - Add a portable pinned comparison helper and retained measurements. Existing group, page, byte, expiry and output-rate caps remain in force. ## Verification - PASS: 54 focused unit, real-bundle, existing runtime bundler and real-PostgreSQL tests. - PASS: database TypeScript check. - PASS at final head `b270b92821`: full local `pnpm -r typecheck` and `pnpm build`. - The first local `pnpm test:run` was stopped after two chat partitions passed (668 cases), because source changed during the run. This is partial evidence only. The fresh final-head full local command passed all six chat partitions (1,829 cases). Its general phase reported one failure in the unchanged workspace port-conflict test. The exact case then passed alone (1 pass, 178 skipped), and the unchanged full file passed 179/179. The original failure did not reproduce; its cause is unknown. The general phase was stopped after its remaining serial queue was measured at roughly three hours. Cancellation did not print the original assertion diff. The full command is failed/incomplete, not passed. Remaining general files, workspace-project phases and 164 serialized files were not reached. Fresh final-head CI passes all partitions. - Final-head code CI passes. Greptile is 5/5 with its thread resolved. Paperclip Review is delayed by an external review workspace recovery failure; it has not completed on this head. - Five serial alternating rounds per mode, 1,000 queries per sample, 40 retained samples, Node v24.22.2. Generic and unowned attribution rises from 0/1,000 to 1,000/1,000 across two exact application anchors. Specific owners stay stack-free. - HTTP median wall cost is 10.528 → 15.169 µs/query; process CPU is 15.461 → 17.662 µs/query, with noisy overlapping ranges. This is census overhead, not a product speedup. - Review failure retained: the existing runtime bundler test first failed because the public parser accepted arbitrary generated frames. The fix restores default rejection and permits generated frames only from the trusted builder boundary. The unchanged existing test now passes. - Full ranges, raw samples, proof boundaries and reproduction commands: https://github.com/paperclipai/paperclip/blob/b270b92821ae47c456a1b61d78c1593c5348ede7/doc/performance/2026-11-03-db-capture-hosted-callers.md ## Risks - Active generic HTTP census now captures stacks. More groups can reach the existing 2,048-group cap sooner. - Anchors identify construction sites in one exact generated build. They do not identify later async consumers, TypeScript lines or CPU owners. - Unscoped driver calls can remain unknown. This is intentional. - The no-network local fixture excludes deployed Sentry/OTel stack formatting and traffic effects. It does not establish exact hosted overhead. - Wrappers remain installed after expiry. Disabled, future and expired windows do not capture stacks. No schema, authorization, provider, or query behavior changes. ## Model Used OpenAI GPT-6.1 Sol (`gpt-6.1-sol`) with reasoning, code execution and repository tools. The host does not expose the context-window size. Independent review used the same model family. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
bf7f77d8cd |
fix: avoid response recovery self-contention (#18715)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The server must present accepted agent responses as comments under current chat authority. > - The recovery worker can check four tasks at the same time. > - Different tasks can share a chat endpoint or external principal. Their authorization transactions then compete for endpoint or identity locks. > - A failed transaction leaves the accepted response pending and repeats its source queries on a later retry. > - This pull request groups checks that share a task, endpoint or principal before dispatch. It keeps every transactional authority check and lock. > - The benefit is less failed recovery work and prompt, exactly-once presentation of valid accepted responses. ## Linked Issues or Issue Description Refs #17714 for a separate publication-commit lock-order fix. Refs #18686 for separate run-list JSON work. Neither changes this recovery admission path. **What happened?** A response-presentation worker reported `could not obtain lock on row in relation "chat_endpoints"`. Four real PostgreSQL recovery fixtures reproduced three such failures in one pass with no external ingress. The worker grouped by task, while authorization locks shared endpoints and principals. Review also exposed two valid Slack endpoints in one workspace that share one principal and compete for its try-advisory lock. **Expected behavior** Recovery checks that share an endpoint or principal should retain task order without competing with one another. Unrelated endpoints should make progress. The server must still reject stale authority and retry legitimate external contention. **Steps to reproduce** Run the shared-endpoint regression or the portable comparison helper linked below. The fixture creates four accepted historical responses for distinct tasks and principals on one actual endpoint. ## What Changed - Read optional endpoint and principal scheduling hints through a company-scoped join to the canonical wake action. The join does not change eligibility or grant authority. - Extend existing bounded check groups to merge intersecting resource keys. Preserve task order, transitive conflicts, independent-group progress, shutdown, drain and pending scopes. - Add real PostgreSQL controls for shared endpoints, a principal shared across two endpoints, an observed blocked transaction with independent endpoint progress, external locks, exact selected output, and endpoint revocation despite a forged hint. - Add a portable helper that materializes the pinned baseline with `git show`, runs the existing real fixture, retains results, and restores temporary files. ## Verification - Final principal-extension head `57047441b28765ed1cf49b8924efc69f5c03dcb4`: 34 focused scheduler/chat controls passed on Node 24.22.2 / pnpm 9.15.4. Independent exact-head source review is green. Full local `pnpm -r typecheck` and `pnpm build` pass on this head. Fresh Greptile and Paperclip reviews are both 5/5 with no findings. All 59 final-head contexts are complete: 55 success, 3 skipped, and the neutral Paperclip review at 5/5. There are no failures or pending checks. - Earlier endpoint-only head `eea437e7edbe7eb41eb880d4d8fd6d99a9c247da`: full local typecheck/build and all remote CI passed. The redundant local full suite was interrupted for this review fix after 956 passing chat cases; it is not a full local pass. - Final shared-principal counts-only comparison completed the same two exact responses: product calls 165 → 147, rows 114 → 100, outer contention failures 1 → 0. SQL query errors stayed zero; advisory-lock refusal is not a failed SQL query. - The earlier endpoint-only study completed five alternating pairs / ten measured runs: median calls 443 → 293, rows 314 → 200, query errors 6 → 0 for four exact responses. Its instrumented shared-host CPU data predates the principal extension and is not a final-head or hosted CPU claim. Retry clocks were accelerated; wall time is not hosted latency. - [Raw samples, final principal work counts, portable commands and failure accounting](https://github.com/paperclipai/paperclip/blob/57047441b28765ed1cf49b8924efc69f5c03dcb4/doc/performance/2026-11-03-response-presentation-contention.md). - Initial server4 CI reported three provider calls instead of two in the unchanged Anthropic identity fixture. The exact local case passed; the full 88-case file passed with observations showing the two expected identity-route calls. The third CI call's origin remains unknown. One bounded rerun of the failed job and its dependent CI aggregate passed. No assertion or source change was made for that failure. - The causal benchmark baseline worker/query is `82c033ed127d07862e6765aa4b69a3cf339bde81`. The PR base is `36df8933f0`. They are not claims about the currently serving revision. ## Risks - Connected resource groups can reduce parallelism when tasks link several endpoints. This preserves the existing task ordering contract. - Missing or stale endpoint/principal hints retain current transactional checks. Other workers and ingress can still cause legitimate NOWAIT retries. - There is no schema, API, permission or provider-transport change. Endpoint revocation and company isolation remain enforced by their existing owners. ## Model Used OpenAI GPT-6.1 Sol (`gpt-6.1-sol`), with tool use and code execution, produced and reviewed this change. The runtime does not expose the context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
554a770c6f |
perf(apps): scope connection profile configuration reads (#18687)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The Apps Permissions page reads saved access profiles for one connection. > - The page fetched every company profile, rule, and binding. > - It used only the app profile and eligible task-granted profiles for that connection. > - This pull request adds a saved configuration read owned by the profile service. > - The route keeps current connection and per-profile management checks. > - The benefit is fewer database rows, less Node work, and a smaller response for the same displayed configuration. ## Linked Issues or Issue Description **Subsystem affected** Server profile configuration reads and the Apps Permissions and Review data caller. **Problem or motivation** One connection page loaded and serialized unrelated company profiles. In the local fixture, this produced a 32.35 MB response with 76,001 rule rows. **Proposed solution** Select the displayed connection's app profile and exact eligible connection-intent profiles before loading their entries and bindings. Return an explicit configuration DTO with complete selected arrays. Keep summary and pending-review work on the existing full API. **Alternatives considered** Filtering the full response in the browser retains the database and Node cost. Truncating the full API would break other clients. Filtering entries to the connection would lose selectors and bindings needed by saves. **Roadmap alignment** This improves the existing governed Apps capability. It does not add a new access policy. Related merged work: #18673 and #18681. Related harness cleanup: #18683. Public PR searches found no duplicate connection configuration projection. ## What Changed - Add `ToolProfileConfiguration` and `GET /api/tool-connections/{connectionId}/profile-configuration`. - Select `app:<connectionId>` at any status and exact active, correctly bound `connection_intent` profiles. Run the existing per-profile visibility filter before configuration queries. - Preserve complete ordered entries and bindings. Keep all save behavior, empty-binding install fallback, and the existing full profile API. - Switch AppDetail and AgentConnectionAccess to the configuration DTO. Use a distinct cache key below the existing profiles prefix so mutation and reconnect invalidations still refresh it. - Add real PostgreSQL differential and authenticated HTTP tests, UI caller coverage, a reproducible benchmark, and access-map documentation. ## Verification Baseline: `3f8ebfff8aa9febd0733c6f65b07485dcadfd0e2`, after both #18673 and #18681. Final head: `b40c1ed7c0833d403e20ee8a5ff9d09f1bad6d22`. Current implementation and reproduction details are in [the performance note](https://github.com/paperclipai/paperclip/blob/b40c1ed7c0833d403e20ee8a5ff9d09f1bad6d22/doc/performance/2026-11-02-connection-profile-configuration.md). - AppDetail: 79 tests passed. Existing access/action saves, install fallback, grant removal, errors, and refresh cases use the new caller. - Real PostgreSQL differential: passed. Full ordered selected configuration equals the old full-list selection. Cases cover extra bindings, sibling selectors, wrong source/key/status/binding, null and non-string agent metadata, fresh mutations, empty app bindings, visibility filtering, and company scope. - Production HTTP authentication: API key and run JWT cases passed. Hidden personal accounts omit spanning profiles. Private connection reads fail. Restored ownership restores visibility. Revoked management grants deny fresh calls. Final UID canonicalization and foreign-company 404 assertions passed on the current head. - Map structural check, token gate, and `git diff --check`: passed. - Required local `pnpm -r typecheck` and `pnpm build`: passed on the current head. `pnpm test:run` was started once. Its first chat shard passed (337 tests, 151.87 seconds); the second shard was intentionally interrupted after the complete final-head CI suite passed. This partial local attempt is not a full local test pass. The same stable-runner general and serialized partitions and Runner lanes passed in [final-head CI](https://github.com/paperclipai/paperclip/actions/runs/41538805231), including aggregate `ci / verify`. No local assertion failures were observed before interruption. - Final-head focused acceptance: four files, 85 tests passed. Full token gates, maps, and module boundaries passed. - Actual browser acceptance passed on the current checkout with isolated real PostgreSQL and a local trusted board. An action saved as Off persisted after reload. Both app agent bindings stayed intact. Removing an agent-only task grant persisted after reload. Requests used only the narrow profile endpoint. No browser exceptions occurred. No responses were mocked. No provider calls or hosted changes were made. The helper stopped the owned runtime and database after the check. - Final exact-head checks: 54 success, one clean neutral Paperclip Review, three skipped, zero pending or failures. Legacy Snyk status: success. No inline review comments. - Greptile reviewed exact head `b40c1ed7c0833d403e20ee8a5ff9d09f1bad6d22`: 5/5, no findings, completed successful check. Five alternating unprofiled pairs ran with real PostgreSQL on macOS/arm64, Node v24.22.2, and forced GC. The fixture has 210 profiles (200 heavy profiles plus ten eligibility cases), 76,001 rules, 1,900 active tools, 695 removed tools, and 12 agents. The selected configuration has two profiles and 1,391 rules. Each pair asserts complete ordered selected DTO equality. | Measurement | Full list | Configuration read | | --- | --- | --- | | Service wall ms, median [min, max] | 200.927 [193.319, 213.390] | 6.414 [5.508, 6.973] | | Node CPU ms, median [min, max] | 262.751 [246.727, 271.573] | 27.525 [22.576, 29.308] | | Select queries / returned rows | 7 / 78,139 | 3 / 1,397 | | Actual response JSON bytes | 32,353,534 | 592,233 | | Serialization wall ms, median [min, max] | 90.714 [88.715, 99.304] | 1.644 [1.637, 1.745] | | Serialization CPU ms, median [min, max] | 90.644 [88.569, 96.778] | 1.654 [1.637, 1.807] | Reproduce: `node --expose-gc --import ./server/node_modules/tsx/dist/loader.mjs server/scripts/benchmark-connection-profile-configuration.ts 5`. The benchmark measures the service and actual response JSON separately. It excludes HTTP and authorization work through an identity visibility callback. Node CPU does not include PostgreSQL CPU. No hosted or foreground-under-load improvement is claimed. Preserved failures: offline install lacked a locked tarball; frozen online install passed. Sandbox localhost and tsx IPC restrictions blocked initial DB/typecheck attempts; permitted retries use the same checks. The first HTTP suite needed the unbuilt plugin SDK; its owning build helper fixed setup. Shared typecheck caught a missing barrel export, now fixed. A broad typecheck inherited the host's unpinned nested pnpm executable; the owned stalled process was stopped and restarted with pinned pnpm 9. The complete current-head CI suite and aggregate verify passed. The separate Paperclip agent review published 5/5 with no findings at this head and completed with a clean neutral conclusion. An added foreign-company assertion expected 403; the existing guard correctly returned 404. Only the test expectation changed. The first real browser attempt saved and reloaded the action correctly, but grant deletion returned 422: the stress fixture had a company binding, which makes that profile the company default. The UI showed the existing deletion error. The second browser seed used the production agent-only intent shape and passed removal and reload. This does not change the stress fixture or product deletion gates. Initial external browser script attempts also needed an ESM extension and the correct dependency path. Logs, first-failure screenshots, traces, and result files are retained in the local evidence directory. ## Risks - SQL selection must stay aligned with connection-intent eligibility. Differential fixtures cover the exact current predicate. - A selected connection can still have many rules. This change preserves all selected configuration. - Per-profile authorization still reads current selected resources and can add work. Connection visibility does not bypass that check. - No migration, default-allow, effective-access, policy-review, provider, or deployment behavior changes. The old endpoint remains available. Rollback restores the old AppDetail caller and removes the additive endpoint. ## Model Used OpenAI Codex, `gpt-6.1-sol`, with code execution and repository tools. The host does not expose the context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
3f7811eef0 |
fix(runner): keep requests responsive during retired backup cleanup (#18619)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work.
> - Remote native runs keep durable harness backups for continuation and
recovery.
> - Publishing a new backup retires earlier backup directories in the
server process.
> - The synchronous permission walk and recursive removal block
unrelated requests.
> - This pull request makes that cleanup asynchronous and keeps
publication waiting for completion.
> - The benefit is continued request progress while large retired
backups are removed.
## Linked Issues or Issue Description
**What happened?**
Publishing a backup with many small files blocks the Node event loop
during directory permission repair and removal. A local comparison with
20,000 retired files reproduced 1.37–1.95 second event-loop stalls. This
is a pause candidate. A short hosted profile has not established it as
the dominant source of sustained CPU use.
**Expected behavior**
Unrelated requests can progress during cleanup. Publication still waits
for complete removal, preserves rollback on a failed lease stamp, and
leaves live files and symlink targets unchanged.
**Steps to reproduce**
Run the benchmark in `doc/performance/native-backup-publication.md`. It
compiles the actual publication service from a pinned baseline and the
working tree, prepares immutable retired backups, and sends foreground
HTTP probes from a separate process.
**Paperclip version or commit**
Measured baseline: `774ddc72713d6aa7cb516691c71f1fc23bcfa5ff`. The
measured treatment source is committed in this branch. Current head:
`0a69cf2d43c2f9fcf032d96c1dceb56163bf21ba`. The runtime patch is
unchanged from the measured source; later commits only strengthen the
test and benchmark evidence paths.
**Deployment mode**
Local compiled service on macOS arm64 with Node v24.22.2. No hosted
rollout has occurred.
Related work: #18380 changed retention scheduling. #13676 included the
existing immutable-backup cleanup behavior. Neither change makes
publication cleanup asynchronous. The GitHub duplicate search also found
#11929, which concerns database dumps rather than native harness
backups. No matching open event-loop cleanup issue was found.
## What Changed
- Use asynchronous filesystem operations for retired-backup cleanup.
Visit directories serially and skip unnecessary metadata calls for file
entries.
- Await cleanup before publication proceeds, during rollback, after
successful rotation, and in executor finalization.
- Exercise immutable current and previous backups in the publication
test. Preserve the existing symlink-isolation and rollback tests. Add a
real separate-process HTTP probe that must complete while publication is
still active.
- Add a repeatable service benchmark and document its fixture, metrics,
and acceptance limits.
## Verification
Draft status: focused correctness tests, the local comparison, and
repository typecheck pass. The complete local test gate is incomplete.
The build passed. Complete CI, hosted acceptance, and final-head review
remain pending.
- Final focused Vitest run: 2 files, 4 tests passed in 649 ms with the
normal server configuration. The regression warms a separate Node
client, waits for its HTTP request during the stamp callback, queues the
response for the next event-loop turn, and then allows final
retired-tree removal to start. The async treatment serves that response
before publication completes. The original synchronous cleanup fails the
same final regression in a separate source fixture. That expected
control failure is retained.
- Syntax and `git diff --check` passed.
- `pnpm -r typecheck` passed on head `332c57b88c` in 152.18 seconds,
including the native Runner build. Validation uses Node v24.22.2 and the
repository-pinned pnpm v9.15.4 with isolated dependencies.
- `pnpm test:run` was intentionally stopped after 1,390.24 seconds (exit
143). All six chat shards passed, covering all 1,817 parameterized cases
across their exact partitions. The general server phase was advancing
but incomplete. The repository timing manifest estimates another roughly
two hours for the serial remainder, so validation moved to final-head
parallel CI. This is not a full local test pass. The command began on
`332c57b88c`; later changes only refine the regression and benchmark,
and the final regression has its own focused pass.
- Benchmark failure/metadata checks pass in a tiny isolated Git fixture:
an injected worker exit 17 is reported in 0.44 seconds without hanging,
and the result includes a staged source change when the unstaged diff is
empty.
- `pnpm build` passed in 41.80 seconds on `9f7e4e669b`. The final commit
only releases a pending test probe on failure; runtime and build sources
are unchanged. An injected client exit verifies that final test failure
path exits promptly in 0.97 seconds instead of hanging.
- Three alternating baseline/treatment pairs used fresh fixtures with
100 directories × 100 files in each of two retired backups. Each
publication removed 20,000 files. A separate process sent HTTP probes at
fixed 5 ms intervals, including overdue arrivals.
- Every run verified the replacement manifest, complete cleanup, and
unchanged symlink-target contents and mode. All 2,395 HTTP requests
completed without errors. The largest measured generator delay was 8.8
ms.
| Metric | Baseline median (range), ms | Treatment median (range), ms |
| --- | ---: | ---: |
| Foreground request p95 | 1,357.7 (1,287.4–2,014.0) | 1.8 (1.7–25.9) |
| Maximum event-loop delay | 1,439.7 (1,371.5–1,954.5) | 19.9
(17.4–40.7) |
| Publication completion | 1,429.1 (1,360.2–1,952.9) | 1,439.5
(897.9–1,649.6) |
| Process CPU during publication | 1,194.1 (1,041.8–1,225.5) | 2,423.1
(1,995.7–2,514.7) |
The treatment served 180–329 requests while publication was active. The
baseline served zero during that interval. Process CPU includes
foreground work that the baseline cannot serve until cleanup completes.
The result supports responsiveness, with a CPU tradeoff. It does not
establish lower total CPU or a hosted latency gain.
Reproduce with Node v24.22.2:
```sh
pnpm --filter @paperclipai/server exec vitest run src/services/native-runtime/native-harness-backup-cleanup.test.ts src/services/native-runtime/native-harness-backup-publication.test.ts
node server/scripts/benchmark-native-backup-publication.mjs --baseline-ref
|