fix: preserve abort interrupt provenance#710
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ 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 |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/renderer-diagnostics-sanitize.test.ts, packages/desktop-electron/src/main/renderer-diagnostics-sanitize.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request introduces metadata tracking for session interruptions and cancellations by adding a fallback mechanism to the Runner utility. This ensures that interrupts occurring without explicit metadata, such as those triggered by scope closure, are correctly annotated with a source, reason, and timestamp. Additionally, the diagnostics sanitizer was updated to include the session.action.abort event, and new tests were added to verify the behavior of the interruption logic and the run-state finalizer. I have no feedback to provide.
Port upstream PR anomalyco/opencode#25114's `changed` byte-comparison gate to both `Config.update` and `Config.updateGlobal` so that a no-op config write no longer tears down running instances. ## Root cause Without the gate, `Config.update` (project) and `Config.updateGlobal` (global) call `Instance.dispose()` / `Instance.disposeAll()` on every write. A write of identical bytes — e.g., a UI mount that re-emits the existing setting — still disposes every running instance, which closes each `SessionRunState` scope mid-turn. The user sees the active assistant turn end with `MessageAbortedError` plus an `Tool execution aborted` tool result, and has to resend. Upstream fixed the same class of bug in opencode commit `a9d399699` ("Prevent Model response Interruption when opening settings dialog"). The fix is not in PawWork's `dev` history (`git branch -a --contains a9d399699` returns only `remotes/upstream/dev`). This PR ports just the `changed` gate, not the broader anomalyco/opencode#25475 instance-lifecycle decoupling refactor. ## Change boundary `packages/opencode/src/config/config.ts` only. `Config.update` and `Config.updateGlobal` now compute a `changed: boolean` from byte comparison; `writeConfigTextAtomic` / `fs.writeFileString`, `Instance.dispose()` / `invalidate()` only fire when `changed` is true. `updateGlobal` keeps a `!fileExisted` clause so the seed-migration path still materializes the file on a fresh install. Regression test: `no-op global config write does not rewrite the file` in `test/config/pawwork-global-config.test.ts`. Uses a sentinel mtime — if the gate ever regresses, the second `saveGlobal` would rewrite the file and bump mtime past the sentinel, failing the test. ## Verification ``` typecheck: bun --cwd packages/opencode run typecheck — pass targeted tests: bun --cwd packages/opencode test test/config/pawwork-global-config.test.ts test/config/config.test.ts — 154 pass, 0 fail CI: typecheck, unit-opencode, smoke-macos-arm64, e2e-artifacts all green ``` ## Review follow-ups - Reviewer flagged a P2: if the disk config is updated externally and our write happens to produce bytes identical to disk, `changed=false` skips `invalidate()` and the in-memory `cachedGlobal` may lag disk. Push back: PawWork is single-instance (`requestSingleInstanceLock`), upstream `a9d399699` and #25475 both accept the same trade-off, and decoupling cache refresh from instance dispose is closer to a #25475-scale refactor than to this PR's "remove one known trigger" boundary. No follow-up opened. See PR comment for full reasoning. ## Residual risk Low. Behavior only suppresses dispose when the write would have produced identical bytes; any real config change still disposes as before. No public API change, no platform-specific concern. ## Related Candidate mitigation for #721. Deliberately not a closing keyword: post-#710 abort diagnostics on the next user reproduction will confirm or refute whether the silent writer was the actual trigger in the reported cases.
Summary
Preserve abort provenance in the minimal diagnostic paths needed for first-turn tool interruption triage.
session.action.abortdiagnostics through the desktop sanitizer with onlysource,mode, andresult.onInterruptwithout source metadata.SessionRunStatescope-close interruptions a session-specific fallback source.Why
The GPT-5.5 first-submit interruption logs narrowed the failure to a runner/session interruption, but the exported diagnostics could not identify who triggered the abort. The renderer already emitted abort diagnostics, but the main-process sanitizer dropped them. On the backend side, direct runner cancel and scope-driven fiber interruption could still arrive at
onInterruptwith no provenance.This PR does not try to change prompt execution behavior. It only makes the next exported session tell us whether the interruption came from the renderer abort path, an unattributed runner cancel, or the session run-state scope closing.
Related Issue
No GitHub issue yet. This came from user-provided diagnostic logs for recurring first-turn tool aborts.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please focus on whether the new provenance labels are stable and specific enough for future exported-session debugging, especially the distinction between
runner.cancel_without_meta,runner.interrupt_without_meta,session.run_state.finalizer, andsession.run_state.scope.Risk Notes
Low behavior risk. The change does not alter which runs are cancelled or when they are cancelled; it only attaches metadata to existing interrupt paths and allows an already-emitted renderer abort event through sanitization. No visible UI change.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI change.
Checklist
bug,enhancement,task, ordocumentation), at least one primary routing label (app,ui,platform,harness, orci), and exactly one priority label (P0toP3), or I requested maintainer labelingdev, and my PR title and commit messages use Conventional Commits in English