mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-24 18:19:05 +02:00
refactor(heartbeat): extract run preparation (#15591)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The heartbeat service prepares context and configuration before agent runs. > - The workspace extraction left the main file at about 27,000 lines. > - Preparation helpers form another large boundary with explicit inputs and one database dependency. > - This pull request moves those helpers into one module and preserves their callers. > - The benefit is a 24,000-line orchestration file and one place to maintain run preparation. ## Linked Issues or Issue Description **What existing behavior does this improve?** It improves the structure and test coverage of heartbeat run preparation. **Current behavior** `server/src/services/heartbeat.ts` mixes context and configuration preparation with execution, queueing, and recovery in 27,048 lines. **Proposed behavior** Move preparation into `server/src/services/heartbeat/run-preparation.ts`. The main file loses 3,048 lines and ends at 24,000 lines. Keep the existing imports and behavior. This continues #15568, #15573, and #15578. **Reason and benefit** A larger domain extraction makes useful progress toward a few manageable files. Context, identity, environment, and tool preparation can be reviewed together without searching the run engine. **Breaking changes** None. Existing public helpers and the configuration-incomplete error class retain their identity. ## What Changed - Move wake payloads, comments, attachments, skill mentions, adapter environment configuration, and MCP/tool setup into `heartbeat/run-preparation.ts`. - Bind issue context, pinned routine snapshots, organization rows, and responsible-user resolution through `createHeartbeatRunPreparation(db)`. - Keep factory construction free of database work. Keep queueing, dispatch, retries, cancellation, and execution order in `heartbeat.ts`. - Preserve all 140 existing exports. A syntax-tree comparison confirms that the 47 extracted function bodies and 25 moved declarations are unchanged. The private string normalizer is also unchanged. - Add six tests for legacy export identity, independent database binding, company scope, pinned routine configuration, wake author selection, company-owner fallback, and credential preflight. - Document the preparation boundary in `doc/DEVELOPING.md`. ## Verification - Passed: 183 focused tests across ten suites, including the six new tests. Command: `pnpm exec vitest run server/src/services/heartbeat/run-preparation.test.ts server/src/__tests__/heartbeat-context-summary.test.ts server/src/__tests__/heartbeat-agent-session-message.test.ts server/src/__tests__/heartbeat-runtime-mcp-servers.test.ts server/src/__tests__/heartbeat-runtime-skills.test.ts server/src/__tests__/heartbeat-project-env.test.ts server/src/__tests__/heartbeat-responsible-user-invariant.test.ts server/src/__tests__/heartbeat-comment-wake-batching.test.ts server/src/__tests__/run-secret-redaction.test.ts server/src/__tests__/low-trust-red-team-routes.test.ts`. - One runtime-skills case reported a Postgres deadlock in its after-test `TRUNCATE` cleanup. A focused rerun passed both runtime-skills cases. The other nine suites passed in the combined run. - Passed: `pnpm -r typecheck`. - Passed: `pnpm build`. - Passed: all 54 GitHub checks on `8d0cb9a251b10be5735fa13035e8a31e50b132e7`. The two optional Storybook jobs were skipped. The complete CI test matrix is green. - The full local `pnpm test:run` was started and then stopped after the complete CI test matrix passed. It had no failures reported before stopping and did not complete locally. - Greptile: 5/5 on the same commit, with no inline comments or actionable findings. ## Risks The extraction crosses context, identity, credential, and tool-access module boundaries. Existing company filters, secret redaction, low-trust rules, and error classes stay intact. The new factory captures only the service database and does no database work during construction. Existing integration tests cover responsible-user authority, credential boundaries, MCP tokens, and quarantine. New tests cover the factory wiring and legacy identity. A syntax-tree comparison confirms that orchestration changed only to bind the extracted loaders. ## Model Used OpenAI GPT-6 via Codex. The exact serving model ID and context window were not exposed in this session. Used reasoning, repository inspection, shell tools, and code execution. ## 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>
This commit is contained in:
1 parent
9f4acc8f14
commit
0d57b9b98e
4 files changed
+3475
-3136
No files matched your search
@@ -1236,6 +1236,15 @@ all service instances share in-flight materialization. Existing public helpers
|
||||
and the workspace validation error class remain available from `heartbeat.ts`.
|
||||
Keep workspace policy changes separate from scheduling and run execution changes.
|
||||
|
||||
Run preparation is in `server/src/services/heartbeat/run-preparation.ts`. It owns
|
||||
issue and wake context, responsible-user resolution, routine environment snapshots,
|
||||
skill mentions, adapter environment configuration, and MCP/tool access setup.
|
||||
`createHeartbeatRunPreparation(db)` binds the context loaders to a service's database
|
||||
without doing database work during construction. `heartbeat.ts` keeps queueing,
|
||||
dispatch, retries, cancellation, and execution order, and re-exports the existing
|
||||
public helpers and configuration-incomplete error class. Keep preparation policy
|
||||
changes in this module and its tests.
|
||||
|
||||
## Wake Context Delivery
|
||||
|
||||
Built-in adapters deliver wake context through the run prompt, including structured
|
||||
|
||||
+88
-3136
File diff suppressed because it is too large.
Load diff
@@ -0,0 +1,141 @@
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { PgDialect } from "drizzle-orm/pg-core";
|
||||
import type { SQL } from "drizzle-orm";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
buildPaperclipRuntimeMcpServers as legacyBuildRuntimeMcpServers,
|
||||
buildPaperclipWakePayload as legacyBuildWakePayload,
|
||||
ConfigurationIncompleteFailure as LegacyConfigurationIncompleteFailure,
|
||||
resolveExecutionRunAdapterConfig as legacyResolveAdapterConfig,
|
||||
} from "../heartbeat.js";
|
||||
import {
|
||||
buildPaperclipRuntimeMcpServers,
|
||||
buildPaperclipWakePayload,
|
||||
ConfigurationIncompleteFailure,
|
||||
createHeartbeatRunPreparation,
|
||||
resolveExecutionRunAdapterConfig,
|
||||
} from "./run-preparation.js";
|
||||
|
||||
// Existing integration suites exercise the real database and dispatch path.
|
||||
// These results isolate the factory binding and preparation query order.
|
||||
function databaseWithResults(...results: unknown[][]) {
|
||||
const scopes: unknown[][] = [];
|
||||
const select = vi.fn(() => {
|
||||
const rows = results.shift() ?? [];
|
||||
const query = {
|
||||
from: () => query,
|
||||
leftJoin: () => query,
|
||||
where: (condition: SQL) => {
|
||||
scopes.push(new PgDialect().sqlToQuery(condition).params);
|
||||
return query;
|
||||
},
|
||||
orderBy: () => query,
|
||||
limit: () => query,
|
||||
then: (resolve: (value: unknown[]) => unknown) => Promise.resolve(rows).then(resolve),
|
||||
};
|
||||
return query;
|
||||
});
|
||||
return { db: { select } as unknown as Db, select, scopes };
|
||||
}
|
||||
|
||||
const emptyRoutineContext = { routineId: null, env: null, responsibleUserId: null };
|
||||
|
||||
describe("heartbeat run preparation module", () => {
|
||||
it("preserves the legacy error class and public preparation functions", () => {
|
||||
expect(LegacyConfigurationIncompleteFailure).toBe(ConfigurationIncompleteFailure);
|
||||
expect(legacyResolveAdapterConfig).toBe(resolveExecutionRunAdapterConfig);
|
||||
expect(legacyBuildWakePayload).toBe(buildPaperclipWakePayload);
|
||||
expect(legacyBuildRuntimeMcpServers).toBe(buildPaperclipRuntimeMcpServers);
|
||||
const failure = new ConfigurationIncompleteFailure("missing credentials", { reason: "test" });
|
||||
expect(failure).toBeInstanceOf(LegacyConfigurationIncompleteFailure);
|
||||
expect(failure.code).toBe("configuration_incomplete");
|
||||
});
|
||||
|
||||
it("binds each context loader to its database and company scope", async () => {
|
||||
const first = databaseWithResults([{ id: "issue-one", title: "First company" }]);
|
||||
const second = databaseWithResults([{ id: "issue-two", title: "Second company" }]);
|
||||
const firstPreparation = createHeartbeatRunPreparation(first.db);
|
||||
const secondPreparation = createHeartbeatRunPreparation(second.db);
|
||||
expect(first.select).not.toHaveBeenCalled();
|
||||
expect(second.select).not.toHaveBeenCalled();
|
||||
const [firstIssue, secondIssue] = await Promise.all([
|
||||
firstPreparation.getIssueExecutionContext("company-one", "issue-one"),
|
||||
secondPreparation.getIssueExecutionContext("company-two", "issue-two"),
|
||||
]);
|
||||
expect(firstIssue).toMatchObject({ id: "issue-one", title: "First company" });
|
||||
expect(secondIssue).toMatchObject({ id: "issue-two", title: "Second company" });
|
||||
expect(first.scopes[0]).toEqual(expect.arrayContaining(["company-one", "issue-one"]));
|
||||
expect(second.scopes[0]).toEqual(expect.arrayContaining(["company-two", "issue-two"]));
|
||||
expect(first.select).toHaveBeenCalledTimes(1);
|
||||
expect(second.select).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("keeps the pinned routine environment and original run identity", async () => {
|
||||
const database = databaseWithResults(
|
||||
[{ routineRevisionId: "revision-one", responsibleUserId: "run-owner" }],
|
||||
[{ snapshot: { version: 1, routine: { env: { PINNED_FLAG: "original" }, responsibleUserId: "snapshot-owner" } }, responsibleUserId: "revision-owner" }],
|
||||
);
|
||||
const result = await createHeartbeatRunPreparation(database.db).getRoutineEnvForExecutionIssue(
|
||||
"company-one",
|
||||
{ originKind: "routine_execution", originId: "routine-one", originRunId: "routine-run-one" },
|
||||
);
|
||||
expect(result).toEqual({ routineId: "routine-one", env: { PINNED_FLAG: "original" }, responsibleUserId: "run-owner" });
|
||||
expect(database.scopes[0]).toEqual(expect.arrayContaining(["company-one", "routine-one", "routine-run-one"]));
|
||||
expect(database.scopes[1]).toEqual(expect.arrayContaining(["company-one", "routine-one", "revision-one"]));
|
||||
expect(database.select).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("uses the latest wake comment's author even when query rows arrive out of order", async () => {
|
||||
const database = databaseWithResults([
|
||||
{ id: "comment-two", authorUserId: "latest-author" },
|
||||
{ id: "comment-one", authorUserId: "older-author" },
|
||||
]);
|
||||
const contextSnapshot: Record<string, unknown> = {
|
||||
wakeCommentIds: ["comment-one", "comment-two"],
|
||||
responsibleUserId: "previous-owner",
|
||||
executionIdentityCause: "company_default",
|
||||
};
|
||||
const owner = await createHeartbeatRunPreparation(database.db).resolveResponsibleUserIdForRunSeed({
|
||||
companyId: "company-one", contextSnapshot,
|
||||
issueContext: { id: "issue-one", responsibleUserId: "issue-owner", parentId: null },
|
||||
routineEnvContext: emptyRoutineContext,
|
||||
});
|
||||
expect(owner).toBe("latest-author");
|
||||
expect(contextSnapshot).not.toHaveProperty("executionIdentityCause");
|
||||
expect(database.scopes[0]).toEqual(expect.arrayContaining(["company-one", "issue-one"]));
|
||||
expect(database.select).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("resolves a system wake through the same company's active owner", async () => {
|
||||
const database = databaseWithResults([{ defaultResponsibleUserId: null }], [{ userId: "company-owner" }]);
|
||||
const contextSnapshot: Record<string, unknown> = {};
|
||||
const owner = await createHeartbeatRunPreparation(database.db).resolveResponsibleUserIdForRunSeed({
|
||||
companyId: "company-one", contextSnapshot, issueContext: null,
|
||||
routineEnvContext: emptyRoutineContext, requestedByActorType: "system",
|
||||
});
|
||||
expect(owner).toBe("company-owner");
|
||||
expect(contextSnapshot.executionIdentityCause).toBe("company_default");
|
||||
expect(database.scopes[0]).toContain("company-one");
|
||||
expect(database.scopes[1]).toEqual(expect.arrayContaining(["company-one", "user", "active", "owner"]));
|
||||
expect(database.select).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("keeps missing scoped credentials as a pre-dispatch failure", async () => {
|
||||
const secretsSvc = {
|
||||
resolveAdapterConfigForRuntime: vi.fn(),
|
||||
resolveEnvBindings: vi.fn(),
|
||||
collectMissingRuntimeBindings: vi.fn(),
|
||||
collectMissingAdapterConfigRuntimeBindings: vi.fn(),
|
||||
};
|
||||
await expect(resolveExecutionRunAdapterConfig({
|
||||
companyId: "company-one", executionRunConfig: { env: {} }, projectEnv: null,
|
||||
requiredScopedEnvBinding: {
|
||||
keys: ["GH_TOKEN"], consumerScopes: ["agent", "project"],
|
||||
reason: "github_credentials_missing", remediation: "Configure GitHub access.",
|
||||
},
|
||||
secretsSvc,
|
||||
})).rejects.toBeInstanceOf(LegacyConfigurationIncompleteFailure);
|
||||
expect(secretsSvc.resolveAdapterConfigForRuntime).not.toHaveBeenCalled();
|
||||
expect(secretsSvc.resolveEnvBindings).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
File diff suppressed because it is too large.
Load diff
Reference in new issue
Block a user