mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 06:04:11 +01:00
fix(evals): accept editor escapes and cancel sensitive QA requests (#18244)
## Thinking Path > - Paperclip manages AI agents that do work for users. > - Product E2E journeys check real browser actions and saved results. > - A rich editor can store plain underscores as escaped Markdown. > - The QA reply check treated that valid saved message as a failure. > - A private target adapter also needs to cancel credential-safe HTTP requests before cleanup. > - This PR fixes reply comparison and adds abort and timeout options to the existing sensitive transport. ## Linked Issues or Issue Description Refs #18222. **What happened?** The artifact revision and Stop journeys rejected the correct single user reply because its underscore was stored as `\_`. Sensitive connection setup also lacked a way to pass its lifecycle signal to the actual HTTP request. **Expected behavior** The check accepts the editor's underscore escape and still rejects changed, duplicate, missing, or foreign replies. A cancelled sensitive request closes before setup cleanup proceeds. **Steps to reproduce** Run an artifact revision with an underscore in the reply. For the transport case, abort a pending sensitive request or let its deadline expire. ## What Changed - Compare the fixture reply after undoing only underscore escaping. Keep exact text, count, and author checks. - Let `ConnectionApi.postSensitive` accept a bounded timeout and abort signal. Check cancellation again after cookie loading and before dispatch. - Test the retained reply failure and both cancellation paths with an actual loopback HTTP server. Document the adapter contract. ## Verification - Focused Product E2E tests: 30 passed across two files. - Product E2E TypeScript check passed. After rebase, a final check first found four diagnostics from stale runner/plugin SDK declarations; rebuilding those two workspace packages cleared all four, and the unchanged E2E check passed. - The first sandbox run failed because loopback `listen` returned `EPERM`. The unchanged HTTP tests passed with loopback permission. The test now reports listener errors directly. - Both independent reviewers found a possible pre-arrival deadline-test hang. The follow-up races arrival against request settlement and adds one second of deadline headroom. All 30 affected tests passed after that fix. - Full current-head CI passed in run 40982914525. Greptile and Paperclip Review both scored 5/5 at `f0526f5fbbf13de5ea43e868eb012069ca3d8907`; all review threads are resolved. Live QA requalification follows the merged public and private fixes; no live pass is claimed here. ## Risks The text normalization applies only to plain fixture replies and only removes underscore escapes. It does not remove arbitrary Markdown. Sensitive calls retain their existing 30-second default and closed error messages. API wrappers must forward the optional signal and await the actual request. ## Model Used OpenAI Codex, GPT-6 family. The serving model ID and context window are not exposed by this session. Used reasoning, code editing, shell execution, and test inspection. ## 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
ed8fae8dbb
commit
8941db9ce6
5 files changed
+65
-4
No files matched your search
@@ -57,6 +57,11 @@ See [PROVIDER-CONNECTIONS.md](PROVIDER-CONNECTIONS.md) for the explicit-only
|
||||
`provider-connections` suite, local/staging target ownership, dedicated browser
|
||||
profiles, credential handoffs, private evidence, and cleanup contract.
|
||||
|
||||
`ConnectionApi.postSensitive` keeps credential bodies outside Playwright traces.
|
||||
Adapters must preserve this transport when wrapping APIs. Its optional abort
|
||||
signal and bounded timeout stop the actual HTTP request; setup cleanup must
|
||||
wait for that request to retire before it certifies resource absence.
|
||||
|
||||
Provider-connection results may retain closed diagnostic codes and terminal run
|
||||
status before managed-instance teardown. Only error records and failed tool
|
||||
receipts are inspected; raw error messages, child stderr, model output and
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { afterEach, expect, it, vi } from "vitest";
|
||||
import type { APIRequestContext } from "@playwright/test";
|
||||
import { ConnectionApi } from "./connection-target.js";
|
||||
import { createServer } from "node:http";
|
||||
|
||||
afterEach(() => vi.unstubAllGlobals());
|
||||
it("uses same-origin session cookies without sending fixture credentials through the traced request transport", async () => {
|
||||
@@ -39,3 +40,35 @@ it("withholds provider bodies and thrown transport details on sensitive failures
|
||||
vi.stubGlobal("fetch", async () => { throw new Error("SECRET transport"); });
|
||||
await expect(api.postSensitive("/api/connection", {})).rejects.toThrow("sensitive_api_request_failed");
|
||||
});
|
||||
|
||||
it.each(["cancel", "deadline"])("retires the actual sensitive HTTP request on %s", async mode => {
|
||||
let entered!: () => void, retired!: () => void;
|
||||
const received = new Promise<void>(resolve => { entered = resolve; });
|
||||
const closed = new Promise<void>(resolve => { retired = resolve; });
|
||||
const server = createServer((_req, response) => { response.once("close", retired); entered(); });
|
||||
await new Promise<void>((resolve, reject) => { server.once("error", reject); server.listen(0, "127.0.0.1", resolve); });
|
||||
try {
|
||||
const address = server.address() as { port: number };
|
||||
const request = { storageState: async () => ({ cookies: [] }) } as unknown as APIRequestContext;
|
||||
const controller = new AbortController();
|
||||
const send = new ConnectionApi(request, `http://127.0.0.1:${address.port}`).postSensitive("/api/owned", { fixture: true },
|
||||
{ signal: controller.signal, timeoutMs: mode === "deadline" ? 1000 : 3000 });
|
||||
const rejected = expect(send).rejects.toThrow("sensitive_api_request_failed");
|
||||
// A heavily delayed dispatch can time out before reaching the listener.
|
||||
// Report that failure and still clean up, rather than hanging on `received`.
|
||||
const first = await Promise.race([received.then(() => "received"), send.then(() => "settled", () => "settled")]);
|
||||
if (mode === "cancel") controller.abort();
|
||||
await rejected;
|
||||
expect(first).toBe("received");
|
||||
await closed;
|
||||
} finally { server.closeAllConnections(); await new Promise<void>(resolve => server.close(() => resolve())); }
|
||||
});
|
||||
|
||||
it("does not dispatch a sensitive mutation if cancellation happens while loading cookies", async () => {
|
||||
const controller = new AbortController();
|
||||
const request = { storageState: async () => { controller.abort(); return { cookies: [] }; } } as unknown as APIRequestContext;
|
||||
const send = vi.fn(); vi.stubGlobal("fetch", send);
|
||||
await expect(new ConnectionApi(request, "https://qa.example.test").postSensitive("/api/owned", {}, { signal: controller.signal }))
|
||||
.rejects.toThrow("sensitive_api_request_failed");
|
||||
expect(send).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -35,16 +35,20 @@ export class ConnectionApi {
|
||||
async delete(route: string) { await this.response(route, "DELETE"); }
|
||||
async bytes(route: string) { return (await this.response(route)).body(); }
|
||||
/** Keep fixture credentials out of Playwright request traces. */
|
||||
async postSensitive<T = any>(route: string, data: unknown): Promise<T> {
|
||||
async postSensitive<T = any>(route: string, data: unknown, options: { timeoutMs?: number; signal?: AbortSignal } = {}): Promise<T> {
|
||||
const target = new URL(route, this.origin);
|
||||
if (!route.startsWith("/api/") || target.origin !== this.origin || !target.pathname.startsWith("/api/")) throw new ConnectionFailure("invalid_api_route");
|
||||
const timeout = options.timeoutMs ?? 30_000;
|
||||
if (!Number.isSafeInteger(timeout) || timeout < 1 || timeout > 120_000) throw new ConnectionFailure("invalid_api_timeout");
|
||||
const signal = AbortSignal.any([AbortSignal.timeout(timeout), ...(options.signal ? [options.signal] : [])]);
|
||||
const { cookies } = await this.request.storageState();
|
||||
const cookie = cookies.filter(c => (target.hostname === c.domain || c.domain.startsWith(".") && target.hostname.endsWith(c.domain))
|
||||
&& (target.pathname === c.path || target.pathname.startsWith(c.path.endsWith("/") ? c.path : `${c.path}/`))
|
||||
&& (!c.secure || target.protocol === "https:")
|
||||
&& (c.expires === -1 || c.expires * 1000 > Date.now())).map(c => `${c.name}=${c.value}`).join("; ");
|
||||
try {
|
||||
const response = await fetch(target, { method: "POST", redirect: "error", signal: AbortSignal.timeout(30_000),
|
||||
signal.throwIfAborted();
|
||||
const response = await fetch(target, { method: "POST", redirect: "error", signal,
|
||||
headers: { "content-type": "application/json", Origin: this.origin, ...(cookie ? { Cookie: cookie } : {}) }, body: JSON.stringify(data) });
|
||||
if (!response.ok) { await response.body?.cancel(); throw new ConnectionFailure(`api_status_${response.status}`); }
|
||||
return await response.json() as T;
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { validateAgentTaskState } from "./agent-flows.js";
|
||||
import { fixtureReplyStoredOnce, validateAgentTaskState } from "./agent-flows.js";
|
||||
import { SLUGIFY_REVISION } from "../everyday-cases.js";
|
||||
import { activeNativeTurnStarted, nativeCancellationHasNoLaterWork, nativeStopAcknowledged, nativeWorkEvents } from "../stop-redirect-flow.js";
|
||||
import { parseTargetConfig } from "./catalog.js";
|
||||
import type { JourneyContext } from "./types.js";
|
||||
@@ -8,6 +9,16 @@ import { agentJourneyRuns, journeyRequiredChecks } from "./success-contract.js";
|
||||
|
||||
const context = { company: { id: "qa-company" }, environmentId: "qa-daytona",
|
||||
agent: { id: "owned-agent", provider: "opencode", model: "qa-model", runtimeMode: "native" } } as JourneyContext;
|
||||
|
||||
it("accepts the QA editor's escaped underscore while rejecting lost, duplicate, foreign or changed replies", () => {
|
||||
const before = [{ id: "old", authorUserId: "user", body: SLUGIFY_REVISION }];
|
||||
const reply = { id: "new", authorUserId: "user", body: SLUGIFY_REVISION.replaceAll("_", "\\_") };
|
||||
expect(fixtureReplyStoredOnce(before, [...before, reply], "user", SLUGIFY_REVISION)).toBe(true);
|
||||
for (const after of [before, [...before, reply, { ...reply, id: "duplicate" }], [...before, { ...reply, authorUserId: "other" }],
|
||||
[...before, { ...reply, body: reply.body.replace("optional", "required") }], [...before, { ...reply, body: reply.body + " extra" }]]) {
|
||||
expect(fixtureReplyStoredOnce(before, after, "user", SLUGIFY_REVISION)).toBe(false);
|
||||
}
|
||||
});
|
||||
function state() {
|
||||
return { currentIssue: { id: "owned-task", companyId: "qa-company", assigneeAgentId: "owned-agent", status: "done" },
|
||||
taskRuns: [{ id: "run", issueId: "owned-task", companyId: "qa-company", agentId: "owned-agent", status: "succeeded", runtimeMode: "native",
|
||||
|
||||
@@ -22,6 +22,14 @@ interface TaskState { currentIssue: Row & { id: string; status: string }; taskRu
|
||||
interface Options { deadlineAt: number; directory: string; capture(id: string, label: string, file: string): Promise<void> }
|
||||
const active = (run: Row) => ["queued", "running"].includes(run.status);
|
||||
|
||||
/** These fixture replies are plain text. The rich editor escapes underscores in
|
||||
* its persisted Markdown; compare the intended text while retaining exact count
|
||||
* and author checks. Do not strip arbitrary markup or accept partial messages. */
|
||||
export function fixtureReplyStoredOnce(before: Row[], after: Row[], userId: string, message: string) {
|
||||
const added = after.filter(row => row.authorUserId === userId && !before.some(old => old.id === row.id));
|
||||
return added.length === 1 && typeof added[0].body === "string" && added[0].body.replaceAll("\\_", "_") === message;
|
||||
}
|
||||
|
||||
/** Owned-task observations only: other work in a persistent QA company cannot affect this grade. */
|
||||
export function validateAgentTaskState(context: JourneyContext, issueId: string, state: TaskState, maxRuns: number, allowedCancelled: string[] = []) {
|
||||
const agent = context.agent!;
|
||||
@@ -91,7 +99,7 @@ export async function runAgentJourney(id: AgentJourney, context: JourneyContext,
|
||||
const after = await waitFor(Math.min(options.deadlineAt, Date.now() + 30_000), () => read(issue), state =>
|
||||
state.comments.some(row => row.authorUserId === context.user.id && !before.some(old => old.id === row.id)), "reply_not_persisted");
|
||||
const added = after.comments.filter(row => row.authorUserId === context.user.id && !before.some(old => old.id === row.id));
|
||||
check("user-reply-stored-once", added.length === 1 && added[0].body === message);
|
||||
check("user-reply-stored-once", fixtureReplyStoredOnce(before, after.comments, context.user.id, message));
|
||||
return added[0];
|
||||
};
|
||||
const download = async (issue: Row, state: TaskState, phase: string, mode: "base" | "separator", preserved: string[] = []) => {
|
||||
|
||||
Reference in new issue
Block a user