mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 00:56:52 +01:00
fix(runner): require normalizedSessionId when validating PRP events (#19615)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Paperclip Runner reports native session activity as PRP events > - `validatePrpEvent` checks an event against the published event schemas and then returns it as a `PrpEvent` > - The `PrpEvent` type requires `normalizedSessionId`, but the schemas leave it optional, so the validator accepts events that the type forbids > - Every producer sets the field, and the consumers compare it to the expected session > - This pull request makes `validatePrpEvent` reject an event without `normalizedSessionId` > - The benefit is that a validated event now always matches its type ## Linked Issues or Issue Description No issue exists. #18839 listed this under its product questions and did not change it. #19495 fixes the other Runner contract types from that list. **What existing behavior does this improve?** `validatePrpEvent` in `packages/paperclip-runner/src/protocol/replay-contract.ts` returns `{ ok: true, event: PrpEvent }` for an event without `normalizedSessionId`. **Current behavior** - The TypeScript `PrpEvent` type requires the field. The Rust event structs also require it (`normalized_session_id: String`). - The published schemas (`protocol/schemas/event*.schema.json`) leave it optional. - Validated events go into event arrays, reducers and the control-plane callback, and all of them expect the field. A plain type change to optional shows 12 such places in the Runner. **Proposed behavior** After the schema check passes, `validatePrpEvent` rejects an event that has no `normalizedSessionId`. The issue has the code `schema_validation` and the path `/normalizedSessionId`. The schema still checks the field's type and length when the field is present. **Reason and benefit** The validator, the type and both producers then agree. The published schemas, their manifest hashes and the generated standalone validators do not change. **Breaking changes** An event without `normalizedSessionId` now fails validation. No producer in this repository creates such an event. The replay fixtures and golden snapshots include the field. ## What Changed - `packages/paperclip-runner/src/protocol/replay-contract.ts`: `validatePrpEvent` requires `normalizedSessionId` after the schema check. - `packages/paperclip-runner/src/protocol/replay-contract.test.ts`: new test. A fixture event passes, and the same event without `normalizedSessionId` fails with the new issue. ## Verification - Runner, in `packages/paperclip-runner` after `pnpm run test:typescript:prep`: - `pnpm exec vitest run` (the full Runner TypeScript suite): 299 of 309 files passed, and 16 tests in 9 files failed. - I ran those 9 files again by themselves: 231 of 233 tests passed. The 2 remaining failures were 20-second timeouts in `src/drivers/cursor-cloud/artifact-delivery.test.ts` and `src/drivers/cursor-cloud/cursor-cloud-driver.test.ts`. - I ran those 2 files again by themselves: all 117 tests passed. They also pass with the master validator. So all 16 failures were load timeouts, not results of this change. - `pnpm run check:replay-goldens`: "Replay golden snapshots and parity summaries are current." - `tsc --noEmit` passes for the Runner `tsconfig.json`, `tsconfig.surfaces.json` and `tsconfig.test.json`. - Server: - `cd server && pnpm typecheck`: passes. - I ran the 29 server test files that import `acknowledged-native-stop` or `native-session-executor`. 27 files passed. - `dot-runner.test.ts` failed at first because `cargo` was not on PATH. With `cargo` on PATH, it passes. - `heartbeat-process-recovery.test.ts` fails on my machine with timeouts. These failures are not caused by this change: two of the failing tests fail in the same way when I use the master `replay-contract.ts`. No failure output mentions PRP events or `schema_validation`. CI runs this file. - `pnpm verify` selects a broad run for this change (exit 2). Required CI supplies that coverage. ## Risks - Medium-low risk. This is a runtime change to a protocol validator. The server validates stored events again (`native-session-executor.ts`, `acknowledged-native-stop.ts`). A stored event without the field would now fail validation. The producers that wrote those events always set the field, so I expect no such rows. - The Rust event structs already reject an event without the field. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits) and extended thinking. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
a7d3d4b78c
commit
05a19768ae
2 files changed
+30
No files matched your search
@@ -386,6 +386,19 @@ it("accepts marked whole-run usage through the runtime standalone event validato
|
||||
expect(result).toMatchObject({ ok: true });
|
||||
});
|
||||
|
||||
it("rejects an event without normalizedSessionId, which the schemas leave optional", async () => {
|
||||
const fixture = await readFixture();
|
||||
const event = (fixture.events as Array<Record<string, unknown>>)[0]!;
|
||||
expect(validatePrpEvent(event)).toMatchObject({ ok: true });
|
||||
|
||||
const { normalizedSessionId: _omitted, ...withoutSession } = event;
|
||||
expect(validatePrpEvent(withoutSession)).toEqual({
|
||||
ok: false,
|
||||
event: null,
|
||||
issues: [{ code: "schema_validation", path: "/normalizedSessionId", message: "event normalizedSessionId is required" }],
|
||||
});
|
||||
});
|
||||
|
||||
it("declares required incomplete reconciliation evidence in the strict usage contract", () => {
|
||||
const validate = new Ajv2020({ strict: true, allErrors: true }).compile(usageSchema);
|
||||
const measurement = { inputTokens: 120, outputTokens: 12, cacheReadTokens: 0, cacheWriteTokens: 0, activeSeconds: 0, requests: 0, providerCostUsd: 0 };
|
||||
|
||||
@@ -504,6 +504,23 @@ export function validatePrpEvent(value: unknown): EventValidationResult {
|
||||
issues: (eventValidator.errors ?? []).map(ajvIssue),
|
||||
};
|
||||
}
|
||||
// The published schemas leave normalizedSessionId optional, but every
|
||||
// producer sets it (the TypeScript PrpEvent type and the Rust event structs
|
||||
// both require it), and consumers compare it to the expected session. The
|
||||
// schema already limits its type and length when it is present.
|
||||
if (record?.normalizedSessionId === undefined) {
|
||||
return {
|
||||
ok: false,
|
||||
event: null,
|
||||
issues: [
|
||||
{
|
||||
code: "schema_validation",
|
||||
path: "/normalizedSessionId",
|
||||
message: "event normalizedSessionId is required",
|
||||
},
|
||||
],
|
||||
};
|
||||
}
|
||||
return { ok: true, event: value, issues: [] };
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user