mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 04:25:52 +01:00
fix(evals): honor the shared feedback read deadline (#18026)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Product E2E evaluations must retain feedback from each paid attempt. > - The collector reads saved feedback and native tool events before teardown. > - A five-second request limit can stop a valid read before the shared read deadline. > - This pull request gives event reads the remaining shared read budget. > - The total deadline and reserved checkpoint time stay the same. ## Linked Issues or Issue Description **What happened?** A paid Ask attempt passed the product grade but retained partial feedback invocation coverage. The collector recorded `events_read_failed`. Its event endpoint later returned HTTP 200. The retained timing is consistent with a request that exceeded the five-second request limit. This change fixes that deterministic failure path; it does not prove the cause of every historical partial read. **Expected behavior** A valid read that takes eight seconds can complete within the 30-second collection deadline. A stalled read must still time out and leave time to save the available feedback. No read or paid attempt is retried. **Steps to reproduce** 1. Return a terminal native run and an empty saved-feedback list. 2. Make the event read take eight seconds and respect the supplied request timeout. 3. Run `collectAttemptFeedback` with a 30-second deadline. 4. Before this fix, the read times out after five seconds and invocation coverage stays partial. Related accounting and reporting work: Refs #18010. ## What Changed - Use the remaining shared read budget for event API requests. Keep the five-second cap for other reads. - Add regressions for an eight-second event response, a stalled event response that preserves saved feedback, and a stalled early feedback read that still permits the event scan. - Document the read budget, reserved persistence time, and no-retry behavior. ## Verification - Red: the initial two regression tests fail before the fix. The other 42 feedback tests pass. The early-read starvation review regression then fails against the first implementation (44 pass), and passes after narrowing the change. - Green: 58 tests pass across feedback collection, interrupted recovery, process shutdown, and scheduled finalization. - Full Product support run: 4,609 pass, three skip, and the retained embedded PostgreSQL bootstrap test fails on this host. The log remains available. This is not a full local pass. - Local Product typecheck fails on installed workspace export mismatches outside the changed files. Hosted CI must check the exact commit with clean dependencies. No local full repository build pass is claimed. - Original paid grades, partial feedback receipts, and accounting journals remain unchanged. This repair makes zero provider calls. ## Risks - A stalled event read can consume the remaining read budget. Other endpoints keep their five-second cap. The shared deadline still reserves up to five seconds for the final checkpoint. - A slow event page can leave later runs uninspected. Those runs remain partial or unavailable. The change does not treat missing evidence as success. ## Model Used - OpenAI Codex, GPT-6, with reasoning and code execution tools. The exact serving variant and context window are not exposed in this session. ## 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 - [ ] All Paperclip CI gates are green - [ ] 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
25f1070fbd
commit
8fe5d5defc
3 files changed
+99
-5
No files matched your search
@@ -2833,8 +2833,12 @@ Each ordinary live attempt collects company-wide saved feedback after retiring
|
||||
runs and before fixture teardown, even when cancellation or grading fails. This
|
||||
includes feedback from child and background runs. Full paginated bodies and
|
||||
native tool invocation outcomes are checkpointed in the access-controlled
|
||||
`snapshots/agent-feedback.json` artifact. Collection has a 30-second deadline;
|
||||
missing detail pages, truncated run lists, active work, and unavailable APIs
|
||||
`snapshots/agent-feedback.json` artifact. Collection has a 30-second deadline.
|
||||
Event reads use the remaining shared read budget, with up to five seconds
|
||||
reserved for the final checkpoint. Other reads keep a five-second request cap
|
||||
so a stalled earlier endpoint leaves time to inspect run events. All reads
|
||||
remain bounded by the shared read cutoff and are not retried.
|
||||
Missing detail pages, truncated run lists, active work, and unavailable APIs
|
||||
remain explicit partial/unavailable observations. Legacy shell helpers do not
|
||||
have complete native invocation coverage. An empty saved-feedback list does not
|
||||
prove that a tool was available, and a completed tool receipt does not by itself
|
||||
|
||||
@@ -435,3 +435,93 @@ it("bounds raw event inspection while retaining the observed feedback receipt pr
|
||||
expect(summary.invocations.unknown).toBeLessThan(100);
|
||||
expect(retained!.diagnostics).toContain("evidence_limit_reached");
|
||||
});
|
||||
|
||||
|
||||
it("allows a slow event read to finish within the shared collection deadline", async () => {
|
||||
let now = 0, eventReads = 0;
|
||||
const clock = vi.spyOn(Date, "now").mockImplementation(() => now);
|
||||
let retained: FeedbackEvidence | undefined;
|
||||
const slowApi = { get: async (url: string, options: { timeout: number }) => {
|
||||
if (url.includes("agent-commentary?")) return { items: [], total: 0 };
|
||||
if (url === "/api/companies/company/heartbeat-runs?limit=1000") return [{ id: "run", status: "succeeded" }];
|
||||
if (url === "/api/heartbeat-runs/run") return { id: "run", status: "succeeded", runtimeMode: "native" };
|
||||
if (url.includes("/events?")) {
|
||||
eventReads++;
|
||||
now += Math.min(8_000, options.timeout);
|
||||
if (options.timeout < 8_000) throw new Error("request timeout");
|
||||
return [event(1, "tool.execution.completed", { executionId: "call", name: "submit_suggestion", status: "completed" })];
|
||||
}
|
||||
throw new Error("Unexpected read");
|
||||
} } as Pick<RunnerApi, "get">;
|
||||
try {
|
||||
const summary = await collectAttemptFeedback({ companyId: "company", deadlineAt: 30_000, api: slowApi,
|
||||
persist: async evidence => { retained = structuredClone(evidence); },
|
||||
});
|
||||
expect(summary).toMatchObject({ collection: "complete", invocationCollection: "complete", runsInspected: 1,
|
||||
invocations: { completed: 1, failed: 0, unknown: 0 } });
|
||||
expect(retained!.runCoverage).toEqual([{ runId: "run", classification: "native_events" }]);
|
||||
expect(retained!.diagnostics).toEqual([]);
|
||||
expect(eventReads).toBe(1);
|
||||
expect(now).toBe(8_000);
|
||||
} finally { clock.mockRestore(); }
|
||||
});
|
||||
|
||||
it("still times out a stalled event read and reserves time to persist saved feedback", async () => {
|
||||
let now = 0, eventReads = 0;
|
||||
const clock = vi.spyOn(Date, "now").mockImplementation(() => now);
|
||||
let retained: FeedbackEvidence | undefined;
|
||||
const stalledApi = { get: async (url: string, options: { timeout: number }) => {
|
||||
if (url.includes("agent-commentary?")) return { items: [entry("saved")], total: 1 };
|
||||
if (url.includes("agent-commentary/")) return entry("saved");
|
||||
if (url === "/api/companies/company/heartbeat-runs?limit=1000") return [{ id: "run", status: "succeeded" }];
|
||||
if (url === "/api/heartbeat-runs/run") return { id: "run", status: "succeeded", runtimeMode: "native" };
|
||||
if (url.includes("/events?")) {
|
||||
eventReads++;
|
||||
now += options.timeout;
|
||||
throw new Error("request timeout: secret details");
|
||||
}
|
||||
throw new Error("Unexpected read");
|
||||
} } as Pick<RunnerApi, "get">;
|
||||
try {
|
||||
const summary = await collectAttemptFeedback({ companyId: "company", deadlineAt: 30_000, api: stalledApi,
|
||||
persist: async evidence => { retained = structuredClone(evidence); },
|
||||
});
|
||||
expect(now).toBe(25_000);
|
||||
expect(summary).toMatchObject({ collection: "complete", invocationCollection: "partial", bodiesCollected: 1, runsInspected: 0 });
|
||||
expect(retained!.entries).toEqual([entry("saved")]);
|
||||
expect(retained!.diagnostics).toEqual(["events_read_failed"]);
|
||||
expect(JSON.stringify(retained)).not.toContain("secret details");
|
||||
expect(eventReads).toBe(1);
|
||||
} finally { clock.mockRestore(); }
|
||||
});
|
||||
|
||||
|
||||
it("keeps time for event coverage after an earlier feedback endpoint stalls", async () => {
|
||||
let now = 0, eventReads = 0;
|
||||
const clock = vi.spyOn(Date, "now").mockImplementation(() => now);
|
||||
let retained: FeedbackEvidence | undefined;
|
||||
const stalledListApi = { get: async (url: string, options: { timeout: number }) => {
|
||||
if (url.includes("agent-commentary?")) {
|
||||
now += options.timeout;
|
||||
throw new Error("request timeout");
|
||||
}
|
||||
if (url === "/api/companies/company/heartbeat-runs?limit=1000") return [{ id: "run", status: "succeeded" }];
|
||||
if (url === "/api/heartbeat-runs/run") return { id: "run", status: "succeeded", runtimeMode: "native" };
|
||||
if (url.includes("/events?")) {
|
||||
eventReads++;
|
||||
now += Math.min(8_000, options.timeout);
|
||||
if (options.timeout < 8_000) throw new Error("request timeout");
|
||||
return [];
|
||||
}
|
||||
throw new Error("Unexpected read");
|
||||
} } as Pick<RunnerApi, "get">;
|
||||
try {
|
||||
const summary = await collectAttemptFeedback({ companyId: "company", deadlineAt: 30_000, api: stalledListApi,
|
||||
persist: async evidence => { retained = structuredClone(evidence); },
|
||||
});
|
||||
expect(summary).toMatchObject({ collection: "unavailable", invocationCollection: "complete", runsInspected: 1 });
|
||||
expect(retained!.diagnostics).toEqual(["feedback_read_failed"]);
|
||||
expect(eventReads).toBe(1);
|
||||
expect(now).toBe(13_000);
|
||||
} finally { clock.mockRestore(); }
|
||||
});
|
||||
@@ -137,10 +137,10 @@ export async function collectAttemptFeedback(input: {
|
||||
const evidence: FeedbackEvidence = { version: 1, companyId: input.companyId, entries: [], invocations: [], runCoverage, diagnostics: [], summary };
|
||||
let retainedBytes = 0, checkpointBytes = 0, eventBytes = 0, eventCount = 0;
|
||||
const diagnose = (code: FeedbackEvidence["diagnostics"][number]) => { if (!evidence.diagnostics.includes(code)) evidence.diagnostics.push(code); };
|
||||
const get = <T>(url: string) => {
|
||||
const get = <T>(url: string, options?: { useRemainingBudget: boolean }) => {
|
||||
const remaining = readDeadlineAt - Date.now();
|
||||
if (remaining <= 0) { diagnose("deadline_reached"); throw new Error("Feedback collection deadline reached"); }
|
||||
return input.api.get<T>(url, { timeout: Math.min(5_000, remaining) });
|
||||
return input.api.get<T>(url, { timeout: options?.useRemainingBudget ? remaining : Math.min(5_000, remaining) });
|
||||
};
|
||||
const persist = async (force = false) => {
|
||||
summary.bodiesCollected = evidence.entries.length;
|
||||
@@ -252,7 +252,7 @@ export async function collectAttemptFeedback(input: {
|
||||
let eventsComplete = false;
|
||||
try {
|
||||
for (let pageNumber = 0; pageNumber < 100; pageNumber++) {
|
||||
const page = await get<Row[]>(`/api/heartbeat-runs/${run.id}/events?afterSeq=${afterSeq}&limit=100`);
|
||||
const page = await get<Row[]>(`/api/heartbeat-runs/${run.id}/events?afterSeq=${afterSeq}&limit=100`, { useRemainingBudget: true });
|
||||
if (!Array.isArray(page) || page.length > 100) throw new Error("Invalid events");
|
||||
for (const event of page) {
|
||||
if (!Number.isSafeInteger(event.seq) || Number(event.seq) <= afterSeq) throw new Error("Invalid event sequence");
|
||||
|
||||
Reference in new issue
Block a user