Skip to content

fix(server): keep Pi notification identities distinct across threads - #15791

Open
aliceisjustplaying wants to merge 1 commit into
pingdotgg:mainfrom
aliceisjustplaying:upstream-prep/pi-notice-ids
Open

aliceisjustplaying wants to merge 1 commit into
pingdotgg:mainfrom
aliceisjustplaying:upstream-prep/pi-notice-ids

Conversation

@aliceisjustplaying

Copy link
Copy Markdown

Two Pi turns can emit notifications or extension errors with the same native item ID. Persisted identities derive from the driver and native ID, so one thread's output can overwrite another's.

Include the provider-turn ID in those IDs. The regression opens two threads and checks that notifications and errors produce four distinct IDs.

Verification: 51 Pi adapter tests and the server typecheck pass. Restoring nightly's adapter makes the regression fail with two IDs instead of four. No dependency on YSK or PR #15368; that PR changes presentation, not identity. This fits the small, obvious-bug exception.

Focused verification commands (repository root, dependencies installed):

./node_modules/.bin/vp test run apps/server/src/orchestration-v2/Adapters/PiAdapterV2.test.ts
(cd apps/server && ../../node_modules/.bin/tsc --noEmit)

Prepared with GPT-6 in Codex.

…eir turn

Turn item and node ids derive from the driver and native item id alone. The Pi
adapter named notify and extension-error items `notify:<ordinal>` and
`extension-error:<ordinal>`, and every thread's first turn starts at the same
ordinal, so an item in one thread overwrote or took over the item with the same
name in another. Both now carry the provider turn id, as compaction items do.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/internals/effect-services.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 47365979-6094-4160-8cfc-9b90b10b22af
📥 Commits

Reviewing files that changed from the base of the PR and between efecd3c and ae913cb.

📒 Files selected for processing (2)
  • apps/server/src/orchestration-v2/Adapters/PiAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Notification and extension-error item IDs now include the active provider-turn ID and item ordinal. A regression test checks that items from two threads have unique IDs and remain associated with their respective threads.

Changes

PiAdapterV2 item identity

Layer / File(s) Summary
Create turn-scoped item IDs
apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/PiAdapterV2.test.ts
Notification and extension-error item IDs now include the provider-turn ID and item ordinal. A regression test verifies that four items from two threads have distinct IDs and belong to the correct threads.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to ae913

No actionable issue remains; the change is mergeable after normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to ae913

The change affects 1 system.

Changed systems: apps/server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/PiAdapterV2.test.ts: Adds a two-thread regression test for item identity: each thread receives a notify and an extension_error during its first turn, and the test checks that each emitted item is associated with the current thread and all four item IDs are unique.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts: Notification items now use a native ID containing the provider-turn ID and next item ordinal; previously, the ID contained only the ordinal.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/PiAdapterV2.ts: Extension-error items now use a native ID containing the provider-turn ID and next item ordinal; previously, the ID contained only the ordinal.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the fix: keeping Pi notification identities distinct across threads.
Description check ✅ Passed The description covers the problem, change, regression test, verification commands and results, and explains why the focused fix qualifies for the small, obvious-bug exception. It also names the agent…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@macroscopeapp

macroscopeapp Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at ae913cb

Macroscope's review found this PR approvable — This is a small, isolated Pi adapter bug fix that makes notification and extension-error identities unique per provider turn, with focused regression coverage across threads. It introduces no schema, deployment, configuration, security, billing, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@kushaldotdev

Copy link
Copy Markdown

Heads-up: this is now conflicting with main (mergeable: false) after Pi moved into packages/provider-pi (#17302) and the provider refactors that followed (#17542, #17427, #17381, #17375).

Would you be able to rebase and push? Macroscope's verdict was recorded against the old head, so a new push restarts the review path. Both notification PRs are in the same state; rebasing them as a pair should be enough.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants