fix(interactions): gate the system-notice signal on server-derived provenance
Greptile and Superagent both landed on the same objection, and the security framing is the sharper one: `presentation` is caller-supplied, so letting it alone suppress expiry lets a genuine user hold their own decision card open indefinitely. Two changes close that without reopening the original bug. Add `derivedAuthorAgentId` to machine provenance. The server writes it from run logs and no caller can set it, and the tier exists precisely because agents post under the `local-board` sentinel — the case this guard has to catch. Honour the `system_notice` marker only from a non-human sentinel author. That id comes from authentication, never from a request body, so the pair (sentinel author + notice framing) is evidence a real signup cannot manufacture. Board-token automation still has no run context and is still forced to `authorType: "user"`, so it stays excluded and SBK-167's regression stays fixed; a genuine user's marked comment now supersedes as it always did. `NON_HUMAN_SENTINEL_AUTHOR_USER_IDS` moves to shared constants so both services read one definition instead of two. Tests: a genuine user cannot suppress expiry with the marker, and a server-derived agent attribution does not supersede while the next plain board comment still does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a7bb79bc62
commit
c481262320
|
|
@ -232,6 +232,13 @@ export type SummarySlotStatus = (typeof SUMMARY_SLOT_STATUSES)[number];
|
|||
export const ISSUE_COMMENT_AUTHOR_TYPES = ["user", "agent", "system"] as const;
|
||||
export type IssueCommentAuthorType = (typeof ISSUE_COMMENT_AUTHOR_TYPES)[number];
|
||||
|
||||
// Author sentinels that agents and local automation post under. `local-board` is also
|
||||
// materialized as a row in the `user` table (it is the implicit board admin), so a plain
|
||||
// "exists in the user table" check does not identify it. The id is assigned by
|
||||
// authentication and never read from a request body, which makes it safe to treat as
|
||||
// provenance. Genuine human users — real signups with their own ids — are never in here.
|
||||
export const NON_HUMAN_SENTINEL_AUTHOR_USER_IDS = new Set<string>(["local-board"]);
|
||||
|
||||
export const ISSUE_COMMENT_PRESENTATION_KINDS = ["message", "system_notice"] as const;
|
||||
export type IssueCommentPresentationKind = (typeof ISSUE_COMMENT_PRESENTATION_KINDS)[number];
|
||||
|
||||
|
|
|
|||
|
|
@ -401,6 +401,7 @@ export {
|
|||
SUMMARY_SLOT_KEYS,
|
||||
SUMMARY_SLOT_STATUSES,
|
||||
ISSUE_COMMENT_AUTHOR_TYPES,
|
||||
NON_HUMAN_SENTINEL_AUTHOR_USER_IDS,
|
||||
ISSUE_COMMENT_METADATA_ROW_TYPES,
|
||||
ISSUE_COMMENT_PRESENTATION_KINDS,
|
||||
ISSUE_COMMENT_PRESENTATION_TONES,
|
||||
|
|
|
|||
|
|
@ -3196,12 +3196,61 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => {
|
|||
expect(rows[0]?.status).toBe("pending");
|
||||
});
|
||||
|
||||
it("treats a system notice as a non-answer even when a human posted it, and still supersedes on their next plain comment", async () => {
|
||||
// `presentation` is caller-supplied, so it cannot prove authorship — and it does not
|
||||
// have to. A comment framed as a status notice is not an answer to a pending question
|
||||
// whoever wrote it, so it must not consume the card. The person keeps every way of
|
||||
// answering: resolving the card, or simply commenting normally.
|
||||
const { companyId, issueId } = await seedConfirmationIssue("Human-authored system notice is not an answer");
|
||||
it("does not let a genuine user suppress expiry with a system-notice marker", async () => {
|
||||
// `presentation` is caller-supplied, so it is only honoured from a non-human sentinel
|
||||
// author. A real signup that marks its comment as a system notice must not be able to
|
||||
// hold its own pending card open indefinitely — that comment still supersedes.
|
||||
const { companyId, issueId } = await seedConfirmationIssue("Genuine user cannot suppress expiry");
|
||||
|
||||
const created = await interactionsSvc.create({
|
||||
id: issueId,
|
||||
companyId,
|
||||
}, {
|
||||
kind: "request_confirmation",
|
||||
payload: {
|
||||
version: 1,
|
||||
prompt: "Proceed with the current draft?",
|
||||
},
|
||||
}, {
|
||||
userId: "local-board",
|
||||
});
|
||||
|
||||
const markedCommentId = randomUUID();
|
||||
const expired = await interactionsSvc.expireRequestConfirmationsSupersededByComment({
|
||||
id: issueId,
|
||||
companyId,
|
||||
}, {
|
||||
id: markedCommentId,
|
||||
createdAt: new Date(new Date(created.createdAt).getTime() + 1_000),
|
||||
authorUserId: "genuine-signup-user",
|
||||
authorType: "user",
|
||||
createdByRunId: null,
|
||||
presentation: {
|
||||
kind: "system_notice",
|
||||
tone: "info",
|
||||
detailsDefaultOpen: false,
|
||||
},
|
||||
}, {
|
||||
userId: "genuine-signup-user",
|
||||
});
|
||||
|
||||
expect(expired).toHaveLength(1);
|
||||
expect(expired[0]).toMatchObject({
|
||||
id: created.id,
|
||||
status: "expired",
|
||||
result: {
|
||||
version: 1,
|
||||
outcome: "superseded_by_comment",
|
||||
commentId: markedCommentId,
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("does not supersede on a server-derived agent attribution, and still supersedes on the next plain board comment", async () => {
|
||||
// `derivedAuthorAgentId` is written by the server from run logs, not by the caller. It
|
||||
// exists because agents post under the `local-board` sentinel, which is exactly the
|
||||
// case this guard has to catch.
|
||||
const { companyId, issueId } = await seedConfirmationIssue("Derived agent attribution is machine-authored");
|
||||
|
||||
const created = await interactionsSvc.create({
|
||||
id: issueId,
|
||||
|
|
@ -3226,11 +3275,7 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => {
|
|||
authorUserId: "local-board",
|
||||
authorType: "user",
|
||||
createdByRunId: null,
|
||||
presentation: {
|
||||
kind: "system_notice",
|
||||
tone: "info",
|
||||
detailsDefaultOpen: false,
|
||||
},
|
||||
derivedAuthorAgentId: randomUUID(),
|
||||
}, {
|
||||
userId: "local-board",
|
||||
})).resolves.toHaveLength(0);
|
||||
|
|
|
|||
|
|
@ -71,6 +71,7 @@ import {
|
|||
connectionIntentResultSchema,
|
||||
createIssueThreadInteractionSchema,
|
||||
legacyIssueThreadInteractionResolverPolicyAlias,
|
||||
NON_HUMAN_SENTINEL_AUTHOR_USER_IDS,
|
||||
normalizeIssueThreadInteractionResolverPolicy,
|
||||
rejectIssueThreadInteractionSchema,
|
||||
requestCheckboxConfirmationPayloadSchema,
|
||||
|
|
@ -818,35 +819,48 @@ function shouldSupersedeInteractionOnUserComment(interaction: UserCommentSuperse
|
|||
return interaction.payload.supersedeOnUserComment === true;
|
||||
}
|
||||
|
||||
// Authenticated provenance: the comment demonstrably came from a machine. `createdByRunId`
|
||||
// covers anything posted from a heartbeat run; `authorType: "system"` covers the internal
|
||||
// producers (pipeline monitors, watchdogs, recovery) that insert notices directly.
|
||||
// Server-derived provenance: the comment demonstrably came from a machine, on evidence no
|
||||
// caller can forge. `createdByRunId` covers anything posted from a heartbeat run;
|
||||
// `authorType: "system"` covers the internal producers (pipeline monitors, watchdogs,
|
||||
// recovery) that insert notices directly; `derivedAuthorAgentId` covers the comments the
|
||||
// server later reattributed to an agent from run logs — the tier that exists precisely
|
||||
// because agents post under the `local-board` sentinel.
|
||||
function isMachineAuthoredComment(comment: {
|
||||
authorType?: IssueCommentAuthorType | null;
|
||||
createdByRunId?: string | null;
|
||||
derivedAuthorAgentId?: string | null;
|
||||
}) {
|
||||
return Boolean(comment.createdByRunId) || comment.authorType === "system";
|
||||
return Boolean(comment.createdByRunId)
|
||||
|| comment.authorType === "system"
|
||||
|| Boolean(comment.derivedAuthorAgentId);
|
||||
}
|
||||
|
||||
// Declared intent: the comment is a status notice, not an answer to anything. That is a
|
||||
// separate question from who wrote it, and deliberately so — `presentation` is caller-
|
||||
// supplied and cannot prove authorship. It does not need to. Local automation that
|
||||
// authenticates with a board/user token has no run context and is forced to
|
||||
// `authorType: "user"`, so at this decision point its comments are byte-identical to a
|
||||
// human's, and the notice framing is the only thing the caller told us. Reading that as
|
||||
// "not an answer" holds either way: an automated "no PR yet, your move" nudge must not
|
||||
// expire the very question the work is waiting on, and a person who deliberately posts a
|
||||
// system notice has not answered the question either. Answering stays available and
|
||||
// explicit — resolve the card.
|
||||
function isNonAnswerComment(comment: { presentation?: IssueCommentPresentation | null }) {
|
||||
return comment.presentation?.kind === "system_notice";
|
||||
// Declared intent: the comment is a status notice, not an answer to a pending question.
|
||||
// `presentation` is caller-supplied, so on its own it proves nothing — a genuine user could
|
||||
// set it and stall their own card indefinitely. It is therefore only honoured from a
|
||||
// non-human sentinel author, which is assigned by authentication and never read from a
|
||||
// request body. That combination is what local automation looks like and what a real signup
|
||||
// cannot produce: automation on a board token has no run context and is forced to
|
||||
// `authorType: "user"` by assertIssueCommentAuthorTypeAllowed, so the sentinel id plus the
|
||||
// notice framing is the only evidence left that its "no PR yet, your move" nudge is not an
|
||||
// answer to the very question the work is waiting on.
|
||||
function isNonAnswerComment(comment: {
|
||||
authorUserId?: string | null;
|
||||
presentation?: IssueCommentPresentation | null;
|
||||
}) {
|
||||
if (comment.presentation?.kind !== "system_notice") return false;
|
||||
const authorUserId = comment.authorUserId;
|
||||
if (!authorUserId) return false;
|
||||
return NON_HUMAN_SENTINEL_AUTHOR_USER_IDS.has(authorUserId);
|
||||
}
|
||||
|
||||
// A pending decision card waits on a human answer, so only a comment that could be one may
|
||||
// consume it.
|
||||
function commentCanSupersedeDecision(comment: {
|
||||
authorUserId?: string | null;
|
||||
authorType?: IssueCommentAuthorType | null;
|
||||
createdByRunId?: string | null;
|
||||
derivedAuthorAgentId?: string | null;
|
||||
presentation?: IssueCommentPresentation | null;
|
||||
}) {
|
||||
return !isMachineAuthoredComment(comment) && !isNonAnswerComment(comment);
|
||||
|
|
@ -4147,14 +4161,15 @@ export function issueThreadInteractionService(
|
|||
authorUserId?: string | null;
|
||||
authorType?: IssueCommentAuthorType | null;
|
||||
createdByRunId?: string | null;
|
||||
derivedAuthorAgentId?: string | null;
|
||||
presentation?: IssueCommentPresentation | null;
|
||||
},
|
||||
actor: InteractionActor,
|
||||
) => {
|
||||
if (!comment.authorUserId) return [];
|
||||
// Local-CLI adapters and board-token automation both post under user auth, so
|
||||
// authorUserId can't tell a human from a machine, and a status notice is not an
|
||||
// answer whoever wrote it. Only a comment that could be a human answer supersedes.
|
||||
// authorUserId alone can't tell a human from a machine. Only a comment that could be
|
||||
// a human answer supersedes.
|
||||
if (!commentCanSupersedeDecision(comment)) return [];
|
||||
|
||||
const rows = await db
|
||||
|
|
|
|||
|
|
@ -93,6 +93,7 @@ import {
|
|||
issueCommentMetadataSchema,
|
||||
issueCommentPresentationSchema,
|
||||
isUuidLike,
|
||||
NON_HUMAN_SENTINEL_AUTHOR_USER_IDS,
|
||||
normalizeIssueIdentifier as normalizeIssueReferenceIdentifier,
|
||||
} from "@paperclipai/shared";
|
||||
import { conflict, HttpError, notFound, unprocessable } from "../errors.js";
|
||||
|
|
@ -215,11 +216,6 @@ const ISSUE_LIST_RELATED_QUERY_CHUNK_SIZE = 500;
|
|||
export const MAX_CHILD_ISSUES_CREATED_BY_HELPER = 25;
|
||||
const MAX_CHILD_COMPLETION_SUMMARIES = 20;
|
||||
const CHILD_COMPLETION_SUMMARY_BODY_MAX_CHARS = 500;
|
||||
// Non-human author sentinels that agents post under. These ARE eligible for
|
||||
// agent-attribution derivation even though `local-board` is also materialized
|
||||
// as a row in the `user` table (it is the implicit board admin). Genuine human
|
||||
// users — real signups with their own ids — are never reattributed.
|
||||
const NON_HUMAN_SENTINEL_AUTHOR_USER_IDS = new Set<string>(["local-board"]);
|
||||
const ISSUE_COMMENT_RUN_LOG_DERIVATION_MAX_LOG_BYTES = 2_000_000;
|
||||
const ISSUE_COMMENT_RUN_LOG_DERIVATION_CHUNK_BYTES = 256_000;
|
||||
const ISSUE_COMMENT_RUN_LOG_DERIVATION_END_SLACK_MS = 60_000;
|
||||
|
|
|
|||
Loading…
Reference in New Issue