Conversation
gaoanze888
left a comment
There was a problem hiding this comment.
Thanks for the unusually thorough benchmark and lifecycle validation. I ran npm run check locally (18 files / 145 tests, typecheck, and package dry-run all passed), then added adversarial lifecycle probes. Two correctness gaps remain before the trajectory can be treated as authoritative:
-
A retryable HTTP response finalizes the whole logical request too early.
after_provider_responseupdates and clears the single active request on the first callback. Pi providers can invoke the response callback for a retryable 429/5xx and then succeed on a later attempt. With the event sequencebefore_provider_request → HTTP 429 → HTTP 200, the durable request span ends permanently aserror / HTTP 429; the 200 has no active span to update. Please retain the logical request across retryable attempts or model attempts separately, and add a429 → 200regression. -
Failed/cancelled compactions remain
runningforever. The compaction span is closed only bysession_compact, but Pi does not emit that extension event when native compaction throws or is cancelled. A latersession_before_compactoverwrites the reference, leaving the earlier durable record unmatched. I reproducedsession_before_compact → agent_settled → shutdownwith no update for the running span. Please choose a representation that does not promise a completion hook the public extension API cannot provide (for example an attempt/info record, or an explicit fallback closure), and cover failure/cancellation plus a later successful compaction.
These are important because an observability feature that reports a recovered request as failed or leaves completed attempts running can send debugging in the wrong direction. The concurrent tool-call pairing and the normal transport-error fallback looked correct in my review.
A separate product-level question for maintainers: the widget tail is bounded, but events.jsonl has no retention/rotation policy. The submitted benchmark measures about 8.8 KiB for very short sessions, so long-running trajectories should have an explicit storage bound or documented retention policy before this is recommended broadly.
|
Addressed both lifecycle findings:
Validation: |
gaoanze888
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up at f75131d5d. Both requested lifecycle fixes are addressed: request spans remain open through retry-related response callbacks and close on assistant completion with a turn-end fallback; compaction starts are now informational attempts and only session_compact emits a separate success record, so failure/cancellation cannot leave a running span. npm run check is green (18 files / 149 tests, typecheck, package dry-run), and the retention limitation is now explicit in the docs. One wording nuance: provider callback granularity is provider-dependent (the bundled OpenAI/Anthropic retry helper may expose only the final response), so “individual response attempts” should not be read as universal; this is non-blocking for the corrected lifecycle behavior.
Adds an opt-in live trajectory widget and a metadata-only session log so users can follow model requests, tools, and compaction as an agent runs.
/trajectorytoggles the view. Prompts, assistant text, tool arguments, and tool output are omitted.Validation on Pi 0.84.2:
See the method and results and all 50 measured pairs. The reproducible benchmark uses a deterministic local HTTP provider; the live-model check is a separate single smoke test.