mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 16:27:18 +02:00
fix: address memory service review findings
This commit is contained in:
1 parent
c3238242e8
commit
d4321f8182
6 files changed
+156
-2
No files matched your search
@@ -144,7 +144,8 @@ CREATE INDEX "memory_extraction_jobs_company_binding_created_idx" ON "memory_ext
|
||||
CREATE INDEX "memory_local_records_company_binding_created_idx" ON "memory_local_records" USING btree ("company_id","binding_id","created_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_local_records_company_agent_created_idx" ON "memory_local_records" USING btree ("company_id","scope_agent_id","created_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_local_records_company_issue_created_idx" ON "memory_local_records" USING btree ("company_id","scope_issue_id","created_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_local_records_fts_idx" ON "memory_local_records" USING gin (to_tsvector('english', coalesce("title", '') || ' ' || "content"));--> statement-breakpoint
|
||||
CREATE INDEX "memory_operations_company_occurred_idx" ON "memory_operations" USING btree ("company_id","occurred_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_operations_company_binding_occurred_idx" ON "memory_operations" USING btree ("company_id","binding_id","occurred_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_operations_company_issue_occurred_idx" ON "memory_operations" USING btree ("company_id","scope_issue_id","occurred_at");--> statement-breakpoint
|
||||
CREATE INDEX "memory_operations_company_run_occurred_idx" ON "memory_operations" USING btree ("company_id","scope_run_id","occurred_at");
|
||||
CREATE INDEX "memory_operations_company_run_occurred_idx" ON "memory_operations" USING btree ("company_id","scope_run_id","occurred_at");
|
||||
@@ -10530,6 +10530,21 @@
|
||||
"concurrently": false,
|
||||
"method": "btree",
|
||||
"with": {}
|
||||
},
|
||||
"memory_local_records_fts_idx": {
|
||||
"name": "memory_local_records_fts_idx",
|
||||
"columns": [
|
||||
{
|
||||
"expression": "to_tsvector('english', coalesce(\"title\", '') || ' ' || \"content\")",
|
||||
"isExpression": true,
|
||||
"asc": true,
|
||||
"nulls": "last"
|
||||
}
|
||||
],
|
||||
"isUnique": false,
|
||||
"concurrently": false,
|
||||
"method": "gin",
|
||||
"with": {}
|
||||
}
|
||||
},
|
||||
"foreignKeys": {
|
||||
@@ -14556,4 +14571,4 @@
|
||||
"schemas": {},
|
||||
"tables": {}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,3 +1,4 @@
|
||||
import { sql } from "drizzle-orm";
|
||||
import { pgTable, uuid, text, timestamp, jsonb, index } from "drizzle-orm/pg-core";
|
||||
import { companies } from "./companies.js";
|
||||
import { agents } from "./agents.js";
|
||||
@@ -52,5 +53,9 @@ export const memoryLocalRecords = pgTable(
|
||||
table.scopeIssueId,
|
||||
table.createdAt,
|
||||
),
|
||||
contentSearchIdx: index("memory_local_records_fts_idx").using(
|
||||
"gin",
|
||||
sql`to_tsvector('english', coalesce(${table.title}, '') || ' ' || ${table.content})`,
|
||||
),
|
||||
}),
|
||||
);
|
||||
@@ -0,0 +1,121 @@
|
||||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { errorHandler } from "../middleware/index.js";
|
||||
import { memoryRoutes } from "../routes/memory.js";
|
||||
|
||||
const companyA = "11111111-1111-4111-8111-111111111111";
|
||||
const companyB = "22222222-2222-4222-8222-222222222222";
|
||||
const bindingId = "33333333-3333-4333-8333-333333333333";
|
||||
|
||||
const mockMemoryService = vi.hoisted(() => ({
|
||||
providers: vi.fn(),
|
||||
listBindings: vi.fn(),
|
||||
listTargets: vi.fn(),
|
||||
createBinding: vi.fn(),
|
||||
getBindingById: vi.fn(),
|
||||
updateBinding: vi.fn(),
|
||||
setCompanyDefault: vi.fn(),
|
||||
resolveBinding: vi.fn(),
|
||||
setAgentOverride: vi.fn(),
|
||||
query: vi.fn(),
|
||||
capture: vi.fn(),
|
||||
forget: vi.fn(),
|
||||
listRecords: vi.fn(),
|
||||
getRecord: vi.fn(),
|
||||
listOperations: vi.fn(),
|
||||
listExtractionJobs: vi.fn(),
|
||||
}));
|
||||
|
||||
const mockAgentService = vi.hoisted(() => ({
|
||||
getById: vi.fn(),
|
||||
}));
|
||||
|
||||
const mockLogActivity = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock("../services/index.js", () => ({
|
||||
agentService: () => mockAgentService,
|
||||
logActivity: mockLogActivity,
|
||||
memoryService: () => mockMemoryService,
|
||||
}));
|
||||
|
||||
function createApp(actor: Record<string, unknown>) {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
(req as any).actor = actor;
|
||||
next();
|
||||
});
|
||||
app.use("/api", memoryRoutes({} as any));
|
||||
app.use(errorHandler);
|
||||
return app;
|
||||
}
|
||||
|
||||
describe("memory routes", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockMemoryService.getBindingById.mockResolvedValue({
|
||||
id: bindingId,
|
||||
companyId: companyA,
|
||||
key: "primary",
|
||||
name: "Primary",
|
||||
providerKey: "local_basic",
|
||||
config: {},
|
||||
enabled: true,
|
||||
createdAt: new Date("2026-04-01T00:00:00.000Z"),
|
||||
updatedAt: new Date("2026-04-01T00:00:00.000Z"),
|
||||
});
|
||||
mockMemoryService.updateBinding.mockResolvedValue({
|
||||
id: bindingId,
|
||||
companyId: companyA,
|
||||
key: "primary",
|
||||
name: "Primary",
|
||||
providerKey: "local_basic",
|
||||
config: {},
|
||||
enabled: false,
|
||||
createdAt: new Date("2026-04-01T00:00:00.000Z"),
|
||||
updatedAt: new Date("2026-04-02T00:00:00.000Z"),
|
||||
});
|
||||
mockLogActivity.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
it("blocks binding updates for board users outside the binding company", async () => {
|
||||
const app = createApp({
|
||||
type: "board",
|
||||
userId: "board-user",
|
||||
source: "session",
|
||||
companyIds: [companyB],
|
||||
isInstanceAdmin: false,
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.patch(`/api/memory/bindings/${bindingId}`)
|
||||
.set("Origin", "http://localhost:3100")
|
||||
.send({ enabled: false });
|
||||
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body).toEqual({ error: "User does not have access to this company" });
|
||||
expect(mockMemoryService.getBindingById).toHaveBeenCalledWith(bindingId);
|
||||
expect(mockMemoryService.updateBinding).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows binding updates when the board user can access the binding company", async () => {
|
||||
const app = createApp({
|
||||
type: "board",
|
||||
userId: "board-user",
|
||||
source: "session",
|
||||
companyIds: [companyA],
|
||||
isInstanceAdmin: false,
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.patch(`/api/memory/bindings/${bindingId}`)
|
||||
.set("Origin", "http://localhost:3100")
|
||||
.send({ enabled: false });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockMemoryService.getBindingById).toHaveBeenCalledWith(bindingId);
|
||||
expect(mockMemoryService.updateBinding).toHaveBeenCalledWith(bindingId, { enabled: false });
|
||||
expect(mockLogActivity).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
@@ -87,6 +87,9 @@ export function memoryRoutes(db: Db) {
|
||||
router.patch("/memory/bindings/:bindingId", validate(updateMemoryBindingSchema), async (req, res) => {
|
||||
assertBoard(req);
|
||||
const bindingId = req.params.bindingId as string;
|
||||
const existing = await memory.getBindingById(bindingId);
|
||||
if (!existing) throw notFound("Memory binding not found");
|
||||
assertCompanyAccess(req, existing.companyId);
|
||||
const binding = await memory.updateBinding(bindingId, req.body);
|
||||
const actor = getActorInfo(req);
|
||||
await logActivity(db, {
|
||||
|
||||
@@ -558,6 +558,15 @@ export function memoryService(db: Db) {
|
||||
const service = {
|
||||
providers: async () => [LOCAL_BASIC_PROVIDER],
|
||||
|
||||
getBindingById: async (bindingId: string) => {
|
||||
const row = await db
|
||||
.select()
|
||||
.from(memoryBindings)
|
||||
.where(eq(memoryBindings.id, bindingId))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
return row ? mapBinding(row) : null;
|
||||
},
|
||||
|
||||
listBindings: async (companyId: string) => {
|
||||
const rows = await db
|
||||
.select()
|
||||
|
||||
Reference in new issue
Block a user