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>
5.4 KiB
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:
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).