mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 13:20:54 +01:00
fix(server): schedule governed review response recovery (#17702)
## Thinking Path > - Paperclip manages agents, tasks and their external chat responses. > - A completed chat turn can owe a response while an earlier review gate remains pending. > - Periodic recovery revisits that response history even when no work can advance. > - This change uses the existing delivery coordinator and saved retry deadlines. > - Approval, interaction and authority changes wake the affected issue or run after commit. > - The existing presentation owner verifies the durable result and current authority. > - Settled and dormant history becomes quiet without losing future eligible responses. ## Linked Issues or Issue Description Refs #17696 (merged) and #15625. This is slice 49, now based on master. It owns governed-review response presentation. Accepted responses and pending decision effects keep their existing owners. **What happened?** Native finalization recovery repeatedly inspects committed review responses. Removing the scan without covering approval, interaction, task-policy and authority changes would strand responses that become eligible later. **Expected behavior** Recover an owed response once from existing durable facts. Enforce authority at publication, wake after relevant outer transactions commit, retain real retry deadlines and restart/drain recovery, and stop recurring SQL when no response can advance. **Steps to reproduce** Complete an external chat turn while an earlier completion review remains pending. Block its response behind an approval or interaction, then resolve that blocker. The response must appear once without a periodic scan or resolving the original review gate. ## What Changed - Add a scoped governed-review worker using the existing coordinator, finalizations, decisions and comment receipts. No queue table or transaction wrapper is added. - Keep recovery and publication policy in the native finalizer. Preserve binding, source/session authority, ownership, deadline and deleted-comment fences; match coordinator-before-issue lock ordering. - Notify through actual approval, interaction, OAuth, task-policy and lifecycle producers before their owned writes. Serialize approval resolution with task linking so a concurrent link retains its wake. Nested transactions still settle at outer commit. - Scope ordinary milestones, finalization and coalesced input to their issue/run. Use the explicit company authority boundary for genuine global restoration, including membership changes. Both response workers share this boundary. - Register under maintenance and drain gates, disable the two periodic repair entrances, and retain fresh finalization and explicit recovery. Add the concurrent 0413 comment-receipt index and migration registration; retain five snapshots. - Preserve the producer inventory, source qualifications and earlier failures in the owner audit. Retain a real Photon regression proving routine ownership assertions stay quiet and concurrent authority restoration wakes each worker once. ## Verification - Scoped repair source `2341714720b5c273f0ce011b48345af0ef39227f` passes **2053 distinct cases**: 1703 in 15 complete producer/authority/recovery files, including all 273 governed-response cases and 95 wake-queue adapter cases, plus 350 selected chat cases. Another 1463 chat cases are outside that selection. Thirteen fresh PostgreSQL 17 fixtures migrate through unchanged 0413; the one temporary queue-fixture import restores exactly. All sixteen captured hashes match. - Two actual repeated coarse-hint SQL regressions fail before repair and pass afterward. Actual membership restoration passes before and after. The same assertions and time limits prove dormant work stays quiet and real authority restoration still publishes once. Real coalesced input covers both queues after commit. - Combined source `586f5674cd9798b33cd22437255e0480b5726d20`, including independent master #18129, passes **1297 distinct cases**: 1093 in five complete server files, 185 in five runner files, 14 Node protocol cases and five UI registry cases. This includes the complete 22-case Photon file for both response workers. The external Photon transport is mocked; Paperclip callbacks, transaction fences and observed SQL are real. The earlier and combined case counts are separate, with no cumulative double-counting. - A normal merge retains both complete owned and independent master binary patches. All twelve new-context hashes and sixteen earlier repair hashes match. Three dedicated fresh 0413 fixtures plus the live-event helper's own fresh database use PostgreSQL 17 without source adaptations. Database build, server types/build, boundaries, full-SHA migration order and generated TS/Rust profiles pass. The new inherited Runner vendor inputs rebuild successfully. - Database bytes remain identical to the previously qualified 0413 source. The recorded populated 0412-to0413 upgrade retains all 411 prior receipts and 38 companies, appends one receipt and none on replay; the index is valid and ready. The earlier 2828-case acceptance, CLI journey, setup failures and their repairs retain their recorded source scope in `doc/plans/2026-10-26-governed-review-response-sleep-audit.md`. - Parent #17696 merged with 58 clean checks, Greptile 5/5 and Ponytail 5/5. Its Photon warning was disproved by real callback/SQL evidence and the security check became successful. This PR still requires its own fresh exact-head CI and review gates. No deployed sleep, external alarm or fleet savings claim is made. ## Risks - Notifications use the existing process-local writer contract. Independent SQL writers and uncoordinated replicas are outside its coverage. - Genuine startup and company authority reconstruction remain proportional to the owed backlog. Queries page 100 rows, but discovery accumulates the candidates; this is not a constant-cost global operation. - Current publication authorization, durable binding, ownership and lock ordering remain mandatory. The worker does not resolve the original review gate. - The concurrent index uses existing migration replay handling. Other recovery duties and Cloud sleep eligibility remain separate. ## Model Used OpenAI Codex, GPT-6 family, with reasoning, repository tools, shell execution and PostgreSQL tests. The runtime does not expose a more specific serving snapshot or 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> Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
40 files changed
+1953
-145
No files matched your search
@@ -13,13 +13,14 @@ import {
|
||||
} from "./helpers/embedded-postgres.js";
|
||||
import { createStoredZipArchive } from "./helpers/zip.js";
|
||||
import { removeTestHome } from "../../../tests/helpers/remove-test-home.js";
|
||||
import { PAPERCLIP_RUNTIME_INFO_FILENAME, readRuntimeInfo } from "../runtime-info.js";
|
||||
|
||||
const execFileAsync = promisify(execFile);
|
||||
type ServerProcess = ReturnType<typeof spawn>;
|
||||
|
||||
async function getAvailablePort(): Promise<number> {
|
||||
async function reserveRequestedPort(): Promise<{ port: number; release: () => Promise<void> }> {
|
||||
return await new Promise((resolve, reject) => {
|
||||
const server = net.createServer();
|
||||
const server = net.createServer((socket) => socket.destroy());
|
||||
server.unref();
|
||||
server.on("error", reject);
|
||||
server.listen(0, "127.0.0.1", () => {
|
||||
@@ -29,9 +30,11 @@ async function getAvailablePort(): Promise<number> {
|
||||
return;
|
||||
}
|
||||
const { port } = address;
|
||||
server.close((error) => {
|
||||
if (error) reject(error);
|
||||
else resolve(port);
|
||||
resolve({
|
||||
port,
|
||||
release: () => new Promise<void>((resolveClose, rejectClose) => {
|
||||
server.close((error) => error ? rejectClose(error) : resolveClose());
|
||||
}),
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -247,7 +250,8 @@ async function runCliJson<T>(
|
||||
}
|
||||
|
||||
async function waitForServer(
|
||||
apiBase: string,
|
||||
instanceId: string,
|
||||
runtimeInfoPath: string,
|
||||
child: ServerProcess,
|
||||
output: { stdout: string[]; stderr: string[] },
|
||||
) {
|
||||
@@ -259,18 +263,22 @@ async function waitForServer(
|
||||
);
|
||||
}
|
||||
|
||||
try {
|
||||
const res = await fetch(`${apiBase}/api/health`);
|
||||
if (res.ok) return;
|
||||
} catch {
|
||||
// Server is still starting.
|
||||
const runtime = readRuntimeInfo(instanceId, runtimeInfoPath);
|
||||
if (runtime && runtime.pid === child.pid) {
|
||||
const apiBase = `http://127.0.0.1:${runtime.port}`;
|
||||
try {
|
||||
const res = await fetch(`${apiBase}/api/health`, { signal: AbortSignal.timeout(1_000) });
|
||||
if (res.ok) return apiBase;
|
||||
} catch {
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
await new Promise((resolve) => setTimeout(resolve, 250));
|
||||
}
|
||||
|
||||
throw new Error(
|
||||
`Timed out waiting for ${apiBase}/api/health.\nstdout:\n${output.stdout.join("")}\nstderr:\n${output.stderr.join("")}`,
|
||||
`Timed out waiting for the child server recorded in ${runtimeInfoPath}.\nstdout:\n${output.stdout.join("")}\nstderr:\n${output.stderr.join("")}`,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -283,6 +291,7 @@ describeEmbeddedPostgres("paperclipai company import/export e2e", () => {
|
||||
let cliShellHome = "";
|
||||
let paperclipInstanceId = "";
|
||||
let serverProcess: ServerProcess | null = null;
|
||||
let requestedPortReservation: Awaited<ReturnType<typeof reserveRequestedPort>> | null = null;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
|
||||
beforeAll(async () => {
|
||||
@@ -297,9 +306,11 @@ describeEmbeddedPostgres("paperclipai company import/export e2e", () => {
|
||||
|
||||
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-company-cli-db-");
|
||||
|
||||
const port = await getAvailablePort();
|
||||
|
||||
|
||||
requestedPortReservation = await reserveRequestedPort();
|
||||
const { port } = requestedPortReservation;
|
||||
writeTestConfig(configPath, tempRoot, port, tempDb.connectionString);
|
||||
apiBase = `http://127.0.0.1:${port}`;
|
||||
|
||||
const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../../..");
|
||||
const output = { stdout: [] as string[], stderr: [] as string[] };
|
||||
@@ -324,11 +335,18 @@ describeEmbeddedPostgres("paperclipai company import/export e2e", () => {
|
||||
output.stderr.push(String(chunk));
|
||||
});
|
||||
|
||||
await waitForServer(apiBase, child, output);
|
||||
apiBase = await waitForServer(
|
||||
paperclipInstanceId,
|
||||
path.join(paperclipHome, "instances", paperclipInstanceId, PAPERCLIP_RUNTIME_INFO_FILENAME),
|
||||
child,
|
||||
output,
|
||||
);
|
||||
expect(new URL(apiBase).port).not.toBe(String(port));
|
||||
}, 60_000);
|
||||
|
||||
afterAll(async () => {
|
||||
await stopServerProcess(serverProcess);
|
||||
await requestedPortReservation?.release();
|
||||
await tempDb?.cleanup();
|
||||
if (tempRoot) {
|
||||
removeTestHome(tempRoot);
|
||||
|
||||
Reference in new issue
Block a user