mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 02:44:50 +01:00
Allow trust-gated direct-parent issue reports (#10098)
## Thinking Path > - Paperclip is the control plane used to coordinate and govern AI-agent companies. > - Agent issue access must preserve company boundaries and trust-policy containment without preventing legitimate task coordination. > - Checked-out standard-trust child runs need a narrow way to report progress directly to their parent issue, but existing authorization treated that report like an arbitrary cross-boundary write. > - Low-trust review runs must remain contained, and stop propagation must not copy potentially untrusted child prose into a higher-trust parent context. > - This pull request adds an audited, one-hop direct-parent comment grant only for standard checked-out runs and a sanitized, idempotent relay for blocked or cancelled child stops. > - The benefit is restored parent/child liveness while retaining least privilege, complete mediation, and low-trust output quarantine. ## Linked Issues or Issue Description ### What happened? A standard-trust agent running a checked-out child issue could not post a progress comment to the direct parent issue because the authorization boundary treated it as an arbitrary cross-issue write. This could stall parent/child coordination. Low-trust review runs also need stop propagation without exposing quarantined child-authored prose. ### Expected behavior A standard checked-out child run may add a comment only to its direct parent issue. The grant must not allow grandparent or sibling access, issue mutation, document writes, reopening, or resuming. Low-trust runs remain denied unless separately mentioned, while blocked/cancelled stops relay only sanitized system metadata once. ### Steps to reproduce 1. Create a parent issue and a child issue assigned to different standard-trust agents. 2. Check out the child issue in a heartbeat run and authenticate as that run. 3. Post a comment to the parent issue and observe the authorization denial before this change. 4. Mark a low-trust child blocked or cancelled and observe that no bounded sanitized parent notification preserves liveness before this change. ### Paperclip version or commit Reproduces on `master` before this PR, including base commit `d36ea13e08`. ### Deployment mode Local dev (`pnpm dev`). ### Installation method Built from source (`pnpm dev` / `pnpm build`). ### Agent adapter(s) involved Not adapter-specific (core authorization and issue-routing behavior). ### Database mode External Postgres in the focused route regression suite; behavior is database-mode independent. ### Access context Agent (bearer API key associated with a checked-out heartbeat run). ### Additional context The implementation deliberately distinguishes a direct-parent report decision from general issue mutation permission and records successful grants in the activity log. ### Privacy checklist - [x] I have reviewed all pasted output for PII, API keys, tokens, company names, and private instance references. ## What Changed - Adds a distinct authorization decision for standard checked-out runs commenting on their direct parent issue. - Keeps low-trust direct-parent reports denied unless an existing explicit mention grant applies. - Forces direct-parent grants to remain comment-only even when a closed parent is unassigned or assigned to the reporting agent. - Audits successful direct-parent report grants in issue activity details. - Adds sanitized, parent-scoped, idempotent system comments and parent wakeups for blocked or cancelled child stops. - Extends the low-trust red-team route suite for allowed parent reports, forbidden upward/sibling writes, closed-parent mutation suppression, and non-laundering stop relays. ## Verification - `pnpm exec vitest run server/src/__tests__/low-trust-red-team-routes.test.ts` — 11 tests passed after the review fix. - `pnpm --filter @paperclipai/server typecheck` — passed after the review fix. - Confirmed the PR changes four files and excludes `pnpm-lock.yaml`, workflow changes, migrations, and unrelated branch commits. ## Risks - This is an authorization behavior change. An overly broad grant could enable cross-boundary writes, while an overly narrow grant could preserve the liveness failure. - The implementation constrains the grant to a standard-trust checked-out run, a direct parent target, and comments only; activity auditing and red-team coverage make regressions observable. - Stop relays intentionally contain only system-generated child identity/status metadata and are deduplicated; child-authored prose is not copied. - SecurityEngineer approval is mandatory before merge. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex using GPT-5.5 with reasoning, repository tool use, shell execution, and test execution. The runtime does not expose the 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>
This commit is contained in:
1 parent
429792f1f3
commit
a17bee98f2
4 files changed
+453
-20
No files matched your search
@@ -3,7 +3,7 @@ import { createServer } from "node:http";
|
||||
import express from "express";
|
||||
import request from "supertest";
|
||||
import { WebSocketServer } from "ws";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { and, eq } from "drizzle-orm";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import {
|
||||
activityLog,
|
||||
@@ -127,6 +127,16 @@ function agentActor(fixture: Fixture, agentId = fixture.agents.lowTrust.id): Exp
|
||||
};
|
||||
}
|
||||
|
||||
function standardReportActor(fixture: Fixture): Express.Request["actor"] {
|
||||
return {
|
||||
type: "agent",
|
||||
agentId: fixture.agents.standard.id,
|
||||
companyId: fixture.company.id,
|
||||
runId: fixture.runs.standardReport.id,
|
||||
source: "agent_jwt",
|
||||
};
|
||||
}
|
||||
|
||||
function skillTestActor(fixture: Fixture, issueId = fixture.issues.assignedReview.id): Express.Request["actor"] {
|
||||
return {
|
||||
type: "agent",
|
||||
@@ -407,12 +417,23 @@ async function seedLowTrustFixture(db: Db) {
|
||||
permissions: {},
|
||||
}).returning();
|
||||
|
||||
const [reviewGrandparent] = await db.insert(issues).values({
|
||||
companyId: company!.id,
|
||||
projectId: allowedProject!.id,
|
||||
title: "Review grandparent",
|
||||
status: "in_progress",
|
||||
priority: "medium",
|
||||
assigneeAgentId: cto!.id,
|
||||
responsibleUserId: "board-user",
|
||||
}).returning();
|
||||
const [reviewRoot] = await db.insert(issues).values({
|
||||
companyId: company!.id,
|
||||
projectId: allowedProject!.id,
|
||||
parentId: reviewGrandparent!.id,
|
||||
title: "Review root",
|
||||
status: "todo",
|
||||
status: "in_progress",
|
||||
priority: "medium",
|
||||
assigneeAgentId: cto!.id,
|
||||
responsibleUserId: "board-user",
|
||||
}).returning();
|
||||
const [assignedReview] = await db.insert(issues).values({
|
||||
@@ -431,6 +452,17 @@ async function seedLowTrustFixture(db: Db) {
|
||||
title: "Same boundary child",
|
||||
status: "todo",
|
||||
priority: "medium",
|
||||
assigneeAgentId: cto!.id,
|
||||
responsibleUserId: "board-user",
|
||||
}).returning();
|
||||
const [standardChild] = await db.insert(issues).values({
|
||||
companyId: company!.id,
|
||||
projectId: allowedProject!.id,
|
||||
parentId: reviewRoot!.id,
|
||||
title: "Assigned standard child",
|
||||
status: "in_progress",
|
||||
priority: "medium",
|
||||
assigneeAgentId: standard!.id,
|
||||
responsibleUserId: "board-user",
|
||||
}).returning();
|
||||
const [siblingOutOfScope] = await db.insert(issues).values({
|
||||
@@ -488,6 +520,12 @@ async function seedLowTrustFixture(db: Db) {
|
||||
status: "running",
|
||||
contextSnapshot: { issueId: assignedReview!.id },
|
||||
}).returning();
|
||||
const [standardReportRun] = await db.insert(heartbeatRuns).values({
|
||||
companyId: company!.id,
|
||||
agentId: standard!.id,
|
||||
status: "running",
|
||||
contextSnapshot: { issueId: standardChild!.id },
|
||||
}).returning();
|
||||
await db.update(issues).set({
|
||||
checkoutRunId: lowTrustRun!.id,
|
||||
executionRunId: lowTrustRun!.id,
|
||||
@@ -496,6 +534,12 @@ async function seedLowTrustFixture(db: Db) {
|
||||
assignedReview!.checkoutRunId = lowTrustRun!.id;
|
||||
assignedReview!.executionRunId = lowTrustRun!.id;
|
||||
assignedReview!.executionPolicy = executionPolicy;
|
||||
await db.update(issues).set({
|
||||
checkoutRunId: standardReportRun!.id,
|
||||
executionRunId: standardReportRun!.id,
|
||||
}).where(eq(issues.id, standardChild!.id));
|
||||
standardChild!.checkoutRunId = standardReportRun!.id;
|
||||
standardChild!.executionRunId = standardReportRun!.id;
|
||||
|
||||
await db.insert(issueComments).values({
|
||||
companyId: company!.id,
|
||||
@@ -630,13 +674,20 @@ async function seedLowTrustFixture(db: Db) {
|
||||
company: company!,
|
||||
agents: { lowTrust: lowTrust!, standard: standard!, collaborator: collaborator!, cto: cto! },
|
||||
projects: { allowed: allowedProject!, outOfScope: outOfScopeProject! },
|
||||
issues: { reviewRoot: reviewRoot!, assignedReview: assignedReview!, sameBoundaryChild: sameBoundaryChild!, siblingOutOfScope: siblingOutOfScope! },
|
||||
issues: {
|
||||
reviewGrandparent: reviewGrandparent!,
|
||||
reviewRoot: reviewRoot!,
|
||||
assignedReview: assignedReview!,
|
||||
standardChild: standardChild!,
|
||||
sameBoundaryChild: sameBoundaryChild!,
|
||||
siblingOutOfScope: siblingOutOfScope!,
|
||||
},
|
||||
approvals: { issueLinkedCanary: approval! },
|
||||
sensitiveRows: {
|
||||
siblingAnnotationThreadId: siblingAnnotationThread!.id,
|
||||
siblingAttachmentId: siblingAttachment!.id,
|
||||
},
|
||||
runs: { lowTrust: lowTrustRun!, standard: standardRun! },
|
||||
runs: { lowTrust: lowTrustRun!, standard: standardRun!, standardReport: standardReportRun! },
|
||||
canaries,
|
||||
};
|
||||
}
|
||||
@@ -727,6 +778,132 @@ describeEmbeddedPostgres("low-trust red-team HTTP route regression suite", () =>
|
||||
});
|
||||
});
|
||||
|
||||
it("allows only standard checked-out runs to comment one hop upward", async () => {
|
||||
const fixture = await seedLowTrustFixture(db);
|
||||
const standardApp = createApp(db, standardReportActor(fixture));
|
||||
const lowTrustApp = createApp(db, agentActor(fixture));
|
||||
|
||||
const parentComment = await request(standardApp)
|
||||
.post(`/api/issues/${fixture.issues.reviewRoot.id}/comments`)
|
||||
.send({ body: "Direct parent report" });
|
||||
expect(parentComment.status, JSON.stringify(parentComment.body)).toBe(201);
|
||||
|
||||
const [audit] = await db
|
||||
.select({ details: activityLog.details })
|
||||
.from(activityLog)
|
||||
.where(and(
|
||||
eq(activityLog.entityId, fixture.issues.reviewRoot.id),
|
||||
eq(activityLog.action, "issue.comment_added"),
|
||||
));
|
||||
expect(audit?.details).toMatchObject({ directParentReportGrant: true });
|
||||
|
||||
const lowTrustParentComment = await request(lowTrustApp)
|
||||
.post(`/api/issues/${fixture.issues.reviewRoot.id}/comments`)
|
||||
.send({ body: "Contained report must not cross" });
|
||||
expect(lowTrustParentComment.status, JSON.stringify(lowTrustParentComment.body)).toBe(403);
|
||||
|
||||
const forbiddenStandardWrites = [
|
||||
request(standardApp)
|
||||
.post(`/api/issues/${fixture.issues.reviewGrandparent.id}/comments`)
|
||||
.send({ body: "No grandparent report" }),
|
||||
request(standardApp)
|
||||
.post(`/api/issues/${fixture.issues.sameBoundaryChild.id}/comments`)
|
||||
.send({ body: "No sibling report" }),
|
||||
request(standardApp)
|
||||
.patch(`/api/issues/${fixture.issues.reviewRoot.id}`)
|
||||
.send({ status: "blocked" }),
|
||||
request(standardApp)
|
||||
.put(`/api/issues/${fixture.issues.reviewRoot.id}/documents/upward-write`)
|
||||
.send({ format: "markdown", body: "No upward document write" }),
|
||||
];
|
||||
for (const forbiddenWrite of forbiddenStandardWrites) {
|
||||
const response = await forbiddenWrite;
|
||||
expect(response.status, JSON.stringify(response.body)).toBe(403);
|
||||
}
|
||||
|
||||
for (const closedParent of [
|
||||
{ assigneeAgentId: null, intent: { reopen: true } },
|
||||
{ assigneeAgentId: fixture.agents.standard.id, intent: { resume: true } },
|
||||
]) {
|
||||
await db
|
||||
.update(issues)
|
||||
.set({ status: "done", assigneeAgentId: closedParent.assigneeAgentId })
|
||||
.where(eq(issues.id, fixture.issues.reviewRoot.id));
|
||||
|
||||
const closedParentComment = await request(standardApp)
|
||||
.post(`/api/issues/${fixture.issues.reviewRoot.id}/comments`)
|
||||
.send({ body: "Comment only on closed parent", ...closedParent.intent });
|
||||
expect(closedParentComment.status, JSON.stringify(closedParentComment.body)).toBe(201);
|
||||
|
||||
const [persistedParent] = await db
|
||||
.select({ status: issues.status })
|
||||
.from(issues)
|
||||
.where(eq(issues.id, fixture.issues.reviewRoot.id));
|
||||
expect(persistedParent?.status).toBe("done");
|
||||
}
|
||||
});
|
||||
|
||||
it("relays blocked and cancelled stops once without laundering child prose", async () => {
|
||||
const fixture = await seedLowTrustFixture(db);
|
||||
const app = createApp(db, boardActor(fixture));
|
||||
|
||||
const blocked = await request(app)
|
||||
.patch(`/api/issues/${fixture.issues.assignedReview.id}`)
|
||||
.send({ status: "blocked", comment: fixture.canaries.raw });
|
||||
expect(blocked.status, JSON.stringify(blocked.body)).toBe(200);
|
||||
|
||||
await request(app).patch(`/api/issues/${fixture.issues.assignedReview.id}`).send({ status: "todo" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.assignedReview.id}`).send({ status: "blocked" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.assignedReview.id}`).send({ status: "todo" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.assignedReview.id}`).send({ status: "cancelled" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.assignedReview.id}`).send({ status: "todo" }).expect(200);
|
||||
await db
|
||||
.update(issues)
|
||||
.set({ parentId: null })
|
||||
.where(eq(issues.id, fixture.issues.assignedReview.id));
|
||||
await request(app)
|
||||
.patch(`/api/issues/${fixture.issues.assignedReview.id}`)
|
||||
.send({ parentId: fixture.issues.reviewGrandparent.id, status: "blocked" })
|
||||
.expect(200);
|
||||
|
||||
await request(app).patch(`/api/issues/${fixture.issues.standardChild.id}`).send({ status: "blocked" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.standardChild.id}`).send({ status: "todo" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.standardChild.id}`).send({ status: "in_review" }).expect(200);
|
||||
await request(app).patch(`/api/issues/${fixture.issues.standardChild.id}`).send({ status: "done" }).expect(200);
|
||||
|
||||
const relayComments = await db
|
||||
.select({ body: issueComments.body, authorType: issueComments.authorType })
|
||||
.from(issueComments)
|
||||
.where(and(
|
||||
eq(issueComments.issueId, fixture.issues.reviewRoot.id),
|
||||
eq(issueComments.authorType, "system"),
|
||||
));
|
||||
expect(relayComments).toHaveLength(2);
|
||||
expect(relayComments.map((comment) => comment.body)).toEqual(expect.arrayContaining([
|
||||
expect.stringContaining(`transitioned to \`blocked\``),
|
||||
expect.stringContaining(`transitioned to \`cancelled\``),
|
||||
]));
|
||||
for (const relay of relayComments) {
|
||||
expect(relay.authorType).toBe("system");
|
||||
expect(relay.body).toContain(fixture.issues.assignedReview.identifier ?? fixture.issues.assignedReview.id);
|
||||
expect(relay.body).not.toContain(fixture.canaries.raw);
|
||||
expect(relay.body).not.toContain("in_review");
|
||||
expect(relay.body).not.toContain("done");
|
||||
expect(relay.body).not.toContain(fixture.issues.standardChild.identifier);
|
||||
}
|
||||
|
||||
const reparentedRelayComments = await db
|
||||
.select({ body: issueComments.body, authorType: issueComments.authorType })
|
||||
.from(issueComments)
|
||||
.where(and(
|
||||
eq(issueComments.issueId, fixture.issues.reviewGrandparent.id),
|
||||
eq(issueComments.authorType, "system"),
|
||||
));
|
||||
expect(reparentedRelayComments).toHaveLength(1);
|
||||
expect(reparentedRelayComments[0]?.body).toContain("transitioned to `blocked`");
|
||||
expect(reparentedRelayComments[0]?.body).not.toContain(fixture.canaries.raw);
|
||||
});
|
||||
|
||||
it("allows mentioned low-trust agents to comment on out-of-bound assigned issues", async () => {
|
||||
const fixture = await seedLowTrustFixture(db);
|
||||
const [targetIssue] = await db.insert(issues).values({
|
||||
|
||||
Reference in new issue
Block a user