mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 22:56:58 +01:00
DottaandPaperclip
2cbfc35a07
## 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>
110 lines
5.4 KiB
Markdown
110 lines
5.4 KiB
Markdown
# Prepared GitHub review-wait read
|
||
|
||
Date: 2026-11-04.
|
||
|
||
`readInheritedGitHubReviewWait` builds a fixed seven-table selector on every
|
||
read. A sampled production window attributed 6.3% of SQL serialization CPU to
|
||
the matching shape (29 samples, mean 204 microseconds). This is a share of
|
||
serialization CPU, not total server CPU.
|
||
|
||
## Change and preserved contract
|
||
|
||
The GitHub owner prepares the existing Drizzle selector once per exact supplied
|
||
`Db` object. A `WeakMap` retains only the compiled query and decoders. Company,
|
||
issue, agent and source-comment IDs are placeholders filled on every execution.
|
||
Database transactions and nested savepoints have different object identities
|
||
and therefore own separate prepared queries and sessions. No database rows,
|
||
results, promises or authorization decisions are cached.
|
||
|
||
Provenance still runs before the selector, and publication still runs after it.
|
||
The SQL, field order, JSON `->'reviewedCommit'` projection, latest-review ordering,
|
||
company boundaries, connection/repository/agent checks and publication lookup
|
||
are unchanged. This does not change postgres.js driver preparation settings.
|
||
|
||
## Reproduction
|
||
|
||
Install the locked dependencies, then run with the repository's supported Node:
|
||
|
||
```sh
|
||
node --import ./server/node_modules/tsx/dist/loader.mjs \
|
||
server/scripts/benchmark-prepared-github-review-wait.ts \
|
||
--baseline-ref=e54f9fbf828e06e385e0c62dc381ccb46190aa30
|
||
```
|
||
|
||
The script imports the complete baseline service from that explicit commit,
|
||
verifies the unchanged provenance dependency, seeds disposable real PostgreSQL,
|
||
and compares the complete result and every generated query, parameter and query
|
||
count. It records main-selector compilation counts separately from timed runs.
|
||
The timed path calls the actual owning service, including provenance and
|
||
publication reads, with no replay driver or substituted authorization result.
|
||
|
||
## Accepted measurement
|
||
|
||
Baseline: `e54f9fbf828e06e385e0c62dc381ccb46190aa30`.
|
||
Treatment source SHA256: `fe647e7fe2aed82609e8f1d66a0109e7a10ee28313ae2ef218ba3ab39b5cd105`.
|
||
Runtime: v24.22.2, darwin/arm64.
|
||
Database: PostgreSQL 18.1 on x86_64-apple-darwin23.6.0.
|
||
Fixture: one synthetic completed review and its full live binding/publication.
|
||
|
||
The final measurement used an exclusive build/benchmark window, 500 warmup
|
||
reads per version and nine alternating pairs of 100 sequential reads each.
|
||
CPU measures the Node process; it excludes PostgreSQL process CPU. It includes
|
||
SQL construction, driver work, row decoding and the rest of the service.
|
||
|
||
| Median per complete service read | Baseline | Treatment | Reduction |
|
||
| --- | ---: | ---: | ---: |
|
||
| Process CPU | 1.00336 ms | 0.75908 ms | 24.35% |
|
||
| Wall time | 2.87089 ms | 2.38731 ms | 16.84% |
|
||
|
||
Main-selector compilations per ten warm service reads: **10 → 0**. All service
|
||
results, generated SQL, bound parameters and query counts matched. Connection
|
||
revocation was observed by both versions after the cache was warm.
|
||
|
||
Each sample below totals 100 service reads; odd pairs run baseline first and even
|
||
pairs run treatment first. No samples were discarded.
|
||
|
||
| Pair | Baseline CPU ms | Treatment CPU ms | Baseline wall ms | Treatment wall ms |
|
||
| --- | ---: | ---: | ---: | ---: |
|
||
| 1 | 109.301 | 82.314 | 301.860 | 326.252 |
|
||
| 2 | 110.681 | 98.892 | 310.177 | 248.191 |
|
||
| 3 | 102.627 | 94.743 | 314.427 | 317.478 |
|
||
| 4 | 98.198 | 78.979 | 287.089 | 323.037 |
|
||
| 5 | 100.336 | 75.908 | 288.454 | 232.344 |
|
||
| 6 | 91.043 | 63.248 | 247.297 | 238.731 |
|
||
| 7 | 72.471 | 59.875 | 223.578 | 206.735 |
|
||
| 8 | 93.542 | 54.337 | 252.195 | 219.263 |
|
||
| 9 | 119.050 | 65.356 | 265.733 | 220.582 |
|
||
|
||
The smallest and largest CPU samples per read were
|
||
0.72471–1.19050 ms for baseline and
|
||
0.54337–0.98892 ms for treatment.
|
||
These are local synthetic service measurements. They do not predict a
|
||
production request latency percentage or measure database execution savings.
|
||
|
||
## Correctness and setup record
|
||
|
||
The focused real-PostgreSQL suite covers malformed JSON and strict type equality,
|
||
current connection/publication/provenance changes, company and agent boundaries,
|
||
repository/thread checks, a newer queued head after a warm completed review,
|
||
changing company/issue/agent/comment bindings on a shared handle, and independent
|
||
transaction/savepoint visibility through rollback.
|
||
|
||
An initial sandbox run could not bind loopback. The next run exposed missing
|
||
macOS library aliases in the freshly unpacked embedded-Postgres dependency.
|
||
The fixture ran after restoring aliases to existing identical library bytes in
|
||
`node_modules`; no application source or package lock changed. The first short
|
||
warmup benchmark and the next long warmup benchmark overlapped another fixture
|
||
and are diagnostic only. The accepted numbers above come from the final
|
||
exclusive-window run. Setup failures are not counted as passing tests.
|
||
|
||
The first shared-consumer integration import and direct server typecheck lacked
|
||
built workspace SDK/Runner outputs. Building `@paperclipai/plugin-sdk` (including
|
||
its shared-package prerequisite) resolved the SDK import. An overly broad run of
|
||
the full external-chat integration file was interrupted to run only its affected
|
||
consumer test. That interrupted run is not a passing suite. Full repository
|
||
validation is tracked separately from the focused tests and benchmark.
|
||
|
||
Focused verification passed: `github-review-wait.test.ts` (16 tests), plus
|
||
`external-chat-wait.integration.test.ts -t 'reads small current external review waits without saved prompts'`
|
||
(four affected consumer cases; 269 unrelated cases skipped).
|