Repository navigation
fix(server): preserve Pi notifications between turns #15792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -469,6 +469,7 @@ export function makePiAdapterV2( | |||||
| // dialog's own resolution updates. | ||||||
| const sessionEventPermit = yield* Semaphore.make(1); | ||||||
| let threadState: PiThreadState | null = null; | ||||||
| let noticeOrdinal = 0; | ||||||
| let registrationAttempted = false; | ||||||
| let lastNativeThreadId: string | null = null; | ||||||
| // User Stop intentionally tears down this RPC process after aborting. | ||||||
|
|
@@ -1178,7 +1179,37 @@ export function makePiAdapterV2( | |||||
| const state = threadState; | ||||||
| const turn = state?.activeTurn ?? null; | ||||||
| const message = recordString(event, "message") ?? ""; | ||||||
| if (turn === null || message.length === 0) return; | ||||||
| if (state === null || message.length === 0) return; | ||||||
| if (turn === null) { | ||||||
| const now = yield* DateTime.now; | ||||||
| const nativeItemId = `notify:${input.providerSessionId}:idle:${nativeRequestId ?? DateTime.toEpochMillis(now)}:${noticeOrdinal++}`; | ||||||
| yield* emit({ | ||||||
| type: "turn_item.updated", | ||||||
| driver: PI_PROVIDER, | ||||||
| turnItem: { | ||||||
| id: idAllocator.derive.turnItemFromProviderItem({ | ||||||
| driver: PI_PROVIDER, | ||||||
| nativeItemId, | ||||||
| }), | ||||||
| threadId: input.threadId, | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: rg -n 'forkThread|targetThreadId|appThreadId|providerThreadId|idleNotice|system_notice' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts
sed -n '1160,1225p' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.tsRepository: pingdotgg/t3code Length of output: 4610 🏁 Script executed: rg -n -C 4 'registerThread|threadState\s*=|threadState:' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts
sed -n '2595,2650p' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts
sed -n '2735,2870p' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.tsRepository: pingdotgg/t3code Length of output: 12934 🏁 Script executed: sed -n '2027,2132p' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts
rg -n -C 2 'appThreadId \\?\\? input\\.threadId|runless|threadId: input\\.threadId' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.tsRepository: pingdotgg/t3code Length of output: 5228 🏁 Script executed: sed -n '1245,1315p' apps/server/src/orchestration-v2/Adapters/PiAdapterV2.tsRepository: pingdotgg/t3code Length of output: 3155 Use the registered thread ID for idle notices. After 🐛 Suggested fix- threadId: input.threadId,
+ threadId: state.providerThread.appThreadId ?? input.threadId,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| runId: null, | ||||||
| nodeId: null, | ||||||
| providerThreadId: state.providerThread.id, | ||||||
| providerTurnId: null, | ||||||
| nativeItemRef: providerRef(nativeItemId), | ||||||
| parentItemId: null, | ||||||
| ordinal: noticeOrdinal, | ||||||
| startedAt: now, | ||||||
| updatedAt: now, | ||||||
| completedAt: now, | ||||||
| status: "completed", | ||||||
| title: message, | ||||||
| type: "system_notice", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High Runless 🤖 Copy this AI Prompt to have your agent fix this: |
||||||
| message, | ||||||
| }, | ||||||
| }); | ||||||
| return; | ||||||
| } | ||||||
| const emittedAt = yield* DateTime.now; | ||||||
| const nativeItemId = `notify:${turn.nextItemOrdinal}`; | ||||||
| yield* emitItemNode(turn, nativeItemId, "system", "completed", emittedAt, emittedAt); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
Adapters/PiAdapterV2.ts:1194Idle notifications after
registerThreadare persisted on the sourceinput.threadIdinstead of the active provider thread'sappThreadId, so a forked or resumed thread displays the notice on the wrong thread with a foreignproviderThreadId. Usestate.providerThread.appThreadIdfor this runless notification.🤖 Copy this AI Prompt to have your agent fix this: