mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-16 02:40:39 +09:00
Harden control-plane safety and issue identifiers (#5292)
## Thinking Path > - Paperclip relies on issue identifiers, execution policies, and agent heartbeat rules to keep autonomous work auditable. > - Safety checks need to reject ambiguous agent handoffs, and identifier parsing needs to support Cloud tenant prefixes. > - Agent instructions also need to make final-disposition rules explicit so work does not stall in vague states. > - This pull request isolates backend correctness and governance hardening from the UI and recovery-system-notice branches. > - The benefit is safer in-review transitions, better identifier compatibility, and clearer agent operating contracts. ## What Changed - Fixed run-aware confirmation ordering and interrupted-run state cleanup. - Added Cloud tenant identity bootstrap and alphanumeric issue identifier support across shared parsing and server routes. - Guarded agent-authored `in_review` updates unless a real review path exists. - Tightened heartbeat disposition instructions in adapter utilities/default AGENTS/Paperclip skill. ## Verification - `pnpm install --frozen-lockfile` - `pnpm exec vitest run packages/shared/src/issue-references.test.ts server/src/__tests__/issue-identifier-routes.test.ts server/src/__tests__/issue-execution-policy-routes.test.ts packages/adapter-utils/src/server-utils.test.ts` initially had the first execution-policy test hit Vitest's 5s timeout under the parallel bundle while the rest passed. - `pnpm exec vitest run server/src/__tests__/issue-execution-policy-routes.test.ts --testTimeout=20000` passed with 10/10 tests. - Follow-up: `pnpm run typecheck:build-gaps` passed. - Follow-up: `pnpm --filter @paperclipai/ui typecheck` passed. - Follow-up: `pnpm vitest run server/src/__tests__/issue-comment-reopen-routes.test.ts server/src/__tests__/company-portability.test.ts server/src/__tests__/costs-service.test.ts` passed. - Follow-up: `pnpm vitest run ui/src/context/LiveUpdatesProvider.test.ts ui/src/lib/issue-chat-messages.test.ts ui/src/lib/issue-reference.test.ts ui/src/lib/issue-timeline-events.test.ts` passed. ## Risks - Medium control-plane risk: in-review update validation changes agent behavior. The error message is explicit and tests cover allowed review paths. ## Model Used - OpenAI GPT-5 Codex via Paperclip `codex_local` adapter, with shell/git/GitHub CLI tool use. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
a1b30c9f35
commit
68f69975a4
17 changed files with 875 additions and 90 deletions
|
|
@ -65,7 +65,7 @@ import {
|
|||
workProductService,
|
||||
} from "../services/index.js";
|
||||
import { logger } from "../middleware/logger.js";
|
||||
import { conflict, forbidden, HttpError, notFound, unauthorized } from "../errors.js";
|
||||
import { conflict, forbidden, HttpError, notFound, unauthorized, unprocessable } from "../errors.js";
|
||||
import { assertBoard, assertCompanyAccess, getActorInfo } from "./authz.js";
|
||||
import {
|
||||
assertNoAgentHostWorkspaceCommandMutation,
|
||||
|
|
@ -227,6 +227,36 @@ async function listSuccessfulRunHandoffStates(
|
|||
return states;
|
||||
}
|
||||
|
||||
const ACTIVE_REVIEW_APPROVAL_STATUSES = new Set(["pending", "revision_requested"]);
|
||||
|
||||
const INVALID_AGENT_IN_REVIEW_DISPOSITION_MESSAGE =
|
||||
"invalid_issue_disposition: Agent-authored updates that move an issue to in_review must include a real review path. " +
|
||||
"This request would leave the issue in_review without anyone or anything owning the next action. " +
|
||||
"Keep working instead of moving to review, create a request_confirmation or ask_user_questions interaction, " +
|
||||
"link or request a pending approval, assign a human reviewer with assigneeUserId, set a typed executionState.currentParticipant through an execution policy, " +
|
||||
"or schedule an issue monitor for an external review/check. After creating one of those review paths, retry the status update.";
|
||||
|
||||
function hasExecutionParticipant(value: unknown) {
|
||||
const state = parseIssueExecutionState(value);
|
||||
if (!state || state.status !== "pending") return false;
|
||||
const participant = state.currentParticipant;
|
||||
if (!participant) return false;
|
||||
if (participant.type === "agent") return Boolean(participant.agentId);
|
||||
if (participant.type === "user") return Boolean(participant.userId);
|
||||
return false;
|
||||
}
|
||||
|
||||
function hasScheduledMonitor(input: {
|
||||
existingMonitorNextCheckAt?: Date | null;
|
||||
patchMonitorNextCheckAt?: unknown;
|
||||
executionPolicy?: unknown;
|
||||
}) {
|
||||
if (input.patchMonitorNextCheckAt instanceof Date && !Number.isNaN(input.patchMonitorNextCheckAt.getTime())) return true;
|
||||
if (input.patchMonitorNextCheckAt === undefined && input.existingMonitorNextCheckAt) return true;
|
||||
const policy = normalizeIssueExecutionPolicy(input.executionPolicy ?? null);
|
||||
return Boolean(policy?.monitor?.nextCheckAt);
|
||||
}
|
||||
|
||||
function executionPrincipalsEqual(
|
||||
left: ParsedExecutionState["currentParticipant"] | null,
|
||||
right: ParsedExecutionState["currentParticipant"] | null,
|
||||
|
|
@ -642,6 +672,59 @@ export function issueRoutes(
|
|||
);
|
||||
}
|
||||
|
||||
async function assertAgentInReviewReviewPath(input: {
|
||||
existing: {
|
||||
id: string;
|
||||
companyId: string;
|
||||
status: string;
|
||||
assigneeUserId?: string | null;
|
||||
executionState?: unknown;
|
||||
monitorNextCheckAt?: Date | null;
|
||||
};
|
||||
updateFields: Record<string, unknown>;
|
||||
actorType: string;
|
||||
}) {
|
||||
const nextStatus = typeof input.updateFields.status === "string"
|
||||
? input.updateFields.status
|
||||
: input.existing.status;
|
||||
if (input.actorType !== "agent" || input.existing.status === "in_review" || nextStatus !== "in_review") return;
|
||||
|
||||
const nextAssigneeUserId = input.updateFields.assigneeUserId === undefined
|
||||
? input.existing.assigneeUserId
|
||||
: input.updateFields.assigneeUserId;
|
||||
if (typeof nextAssigneeUserId === "string" && nextAssigneeUserId.trim().length > 0) return;
|
||||
|
||||
const nextExecutionState = input.updateFields.executionState === undefined
|
||||
? input.existing.executionState
|
||||
: input.updateFields.executionState;
|
||||
if (hasExecutionParticipant(nextExecutionState)) return;
|
||||
|
||||
const nextExecutionPolicy = input.updateFields.executionPolicy;
|
||||
if (hasScheduledMonitor({
|
||||
existingMonitorNextCheckAt: input.existing.monitorNextCheckAt ?? null,
|
||||
patchMonitorNextCheckAt: input.updateFields.monitorNextCheckAt,
|
||||
executionPolicy: nextExecutionPolicy,
|
||||
})) return;
|
||||
|
||||
const interactions = await issueThreadInteractionService(db).listForIssue(input.existing.id);
|
||||
if (interactions.some((interaction) => interaction.status === "pending")) return;
|
||||
|
||||
const approvals = await issueApprovalsSvc.listApprovalsForIssue(input.existing.id);
|
||||
if (approvals.some((approval) => ACTIVE_REVIEW_APPROVAL_STATUSES.has(String(approval.status)))) return;
|
||||
|
||||
throw unprocessable(INVALID_AGENT_IN_REVIEW_DISPOSITION_MESSAGE, {
|
||||
code: "invalid_issue_disposition",
|
||||
missing: "review_path",
|
||||
validReviewPaths: [
|
||||
"pending_issue_thread_interaction",
|
||||
"linked_pending_approval",
|
||||
"human_assignee_user_id",
|
||||
"typed_execution_state_current_participant",
|
||||
"scheduled_issue_monitor",
|
||||
],
|
||||
});
|
||||
}
|
||||
|
||||
async function logExpiredRequestConfirmations(input: {
|
||||
issue: { id: string; companyId: string; identifier?: string | null };
|
||||
interactions: Array<{ id: string; kind: string; status: string; result?: unknown }>;
|
||||
|
|
@ -849,6 +932,23 @@ export function issueRoutes(
|
|||
return true;
|
||||
}
|
||||
|
||||
function assertStructuredCommentFieldsAllowed(
|
||||
req: Request,
|
||||
res: Response,
|
||||
input: { presentation?: unknown; metadata?: unknown },
|
||||
) {
|
||||
const hasStructuredFields = input.presentation !== undefined || input.metadata !== undefined;
|
||||
if (!hasStructuredFields) return true;
|
||||
if (req.actor.type === "board") return true;
|
||||
res.status(403).json({
|
||||
error: "Only board users may set structured comment presentation or metadata",
|
||||
details: {
|
||||
securityPrinciples: ["Least Privilege", "Secure Defaults", "Complete Mediation"],
|
||||
},
|
||||
});
|
||||
return false;
|
||||
}
|
||||
|
||||
async function assertExplicitResumeIntentAllowed(
|
||||
req: Request,
|
||||
res: Response,
|
||||
|
|
@ -2403,6 +2503,12 @@ export function issueRoutes(
|
|||
}
|
||||
}
|
||||
|
||||
await assertAgentInReviewReviewPath({
|
||||
existing,
|
||||
updateFields,
|
||||
actorType: req.actor.type,
|
||||
});
|
||||
|
||||
const nextAssigneeAgentId =
|
||||
updateFields.assigneeAgentId === undefined ? existing.assigneeAgentId : (updateFields.assigneeAgentId as string | null);
|
||||
const nextAssigneeUserId =
|
||||
|
|
@ -3785,6 +3891,10 @@ export function issueRoutes(
|
|||
}
|
||||
assertCompanyAccess(req, issue.companyId);
|
||||
if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
|
||||
if (!assertStructuredCommentFieldsAllowed(req, res, {
|
||||
presentation: req.body.presentation,
|
||||
metadata: req.body.metadata,
|
||||
})) return;
|
||||
const closedExecutionWorkspace = await getClosedIssueExecutionWorkspace(issue);
|
||||
if (closedExecutionWorkspace) {
|
||||
respondClosedIssueExecutionWorkspace(res, closedExecutionWorkspace);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue