fix(pi): bind writes to acknowledged runtime sessions - #734
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughPi native session-attributed writes now use only acknowledged runtime session IDs. Registration failures remain retryable, unavailable runtime identity stops writes, and model-supplied ChangesPi runtime session binding
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change restricts Pi writes to acknowledged native runtime sessions and makes failed registration retryable; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant PiNativeTool
participant SessionManager
participant ensureSession
participant EngramSessionAPI
participant EngramWriteAPI
PiNativeTool->>SessionManager: getSessionId()
PiNativeTool->>ensureSession: ensure runtime session
ensureSession->>EngramSessionAPI: register session
EngramSessionAPI-->>ensureSession: acknowledgement
PiNativeTool->>EngramWriteAPI: persist with runtime session ID
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The runtime-owned identity direction is correct, but the first-use registration path at plugin/pi/index.ts:419-428 still needs concurrency coordination. Two parallel writes can both miss knownSessions, issue separate registrations, and let one call fail after the other has already acknowledged and cached the same session, which rejects a valid write; the focused probe reproduced two registration attempts with only one completed write. Use one in-flight promise per project/session key, cache only successful acknowledgement, clear the promise after failure, and add parallel success plus failure-retry coverage. Please also narrow docs/AGENT-SETUP.md:89 to the four wrappers covered by #733, then rebase after #730 and retain both runtime-binding paragraphs.
|
Addressed your requested changes in 72366f0:
Validation is green: 46/46 Pi tests, repository unit and E2E checks, and CodeRabbit with no actionable comments. Could you take another look? |
🔗 Linked Issue
Closes #733
🏷️ PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance, dependencies, toolingtype:breaking-change— Breaking change📝 Summary
📂 Changes
plugin/pi/index.tsplugin/pi/test/index-source.test.mjsplugin/pi/test/native-tool-contract.test.mjsdocs/AGENT-SETUP.md🧪 Test Plan
npm testinplugin/pi— 44/44 passnpx --yes @biomejs/biome@2.5.6 lint plugin/pi/index.ts— 0 errorsgo test -tags e2e ./internal/server/... -count=1git diff --check🔗 Chain Context
Strategy: sequential PRs to
mainmainat8058269✅ Contributor Checklist
type:*labelCo-Authored-Bytrailers💬 Notes for Reviewers
Pi native wrappers intentionally remain for their UI. This slice removes lifecycle policy from model arguments: the host runtime ID is authoritative, registration must be acknowledged, and failures stop the write without poisoning retry state.
Summary by CodeRabbit
Bug Fixes
Documentation