feat(opencode-plugin): make the git return path awaitable and guard sandbox deletion - #49
Merged
Merged
Conversation
…andbox deletion OpenCode dispatches the plugin event hook without awaiting it, so the git sync started on session.idle is invisible to hosts: session.idle can be observed (and the run closed) while sandbox changes are still being pulled, and sync failures only surface in the plugin log and TUI toasts. - Track in-flight syncs per session and serialize them, so overlapping idle events cannot race each other. - session.deleted: wait for any in-flight sync, then pull remaining changes from a running sandbox before deleting it. If unsynced changes cannot be pulled, deletion is aborted and the sandbox is preserved instead of destroying the only copy of the work. - Add a gitSync tool as an explicit, awaitable completion boundary: it returns only after changes are in the local repository and surfaces git failures as structured tool errors that hosts can observe. - Drain in-flight syncs in the dispose hook so a graceful shutdown does not abandon a running sync (ignored by OpenCode versions without dispose support). Fixes part of #46 Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review follow-ups on the awaitable git return path: - Pull by tip comparison, not commit creation: autoCommitAndPull now pulls whenever the sandbox HEAD differs from the local opencode/N ref. A prior sync that committed in the sandbox but failed to pull left stranded commits that the status-only check skipped forever - and the delete guard would then have destroyed them as 'no changes'. - Track the whole idle pipeline: the session.idle handler enqueues its entire operation (sandbox resolution included) synchronously, so a dispose() starting mid-resolution can no longer observe an empty queue and let the process exit before the sync even registers. - Make deletion a queue barrier: the final sync and sandbox.delete() run as one queue entry, so no sync can slot in between the last pull and destruction. - Abort deletion when the local repo is inaccessible: a missing worktree now aborts deletion with a clear message instead of silently reporting 'no changes' and destroying the only copy of unsynced work. - Bound the shutdown drain to 60s (with a warning) so a sync stalled on an unreachable sandbox cannot wedge process exit; the delete path stays unbounded on purpose - deleting mid-sync loses data. Verified with a live end-to-end run against a real sandbox: fresh edit sync, no-change convergence, stranded-commit recovery, and re-convergence all pass; typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…andbox git errors Second review round on the sync lifecycle: - Tombstone deleting/deleted sessions: getSandbox creates sandboxes on demand, so a sync queued behind a deletion (or any late event for a deleted session) would have resurrected a fresh sandbox that nothing tracks or cleans up. deleteSandbox now records the session before teardown (removed again on failure), getSandbox refuses tombstoned sessions, hasSandbox reports them as absent, and the idle pipeline re-checks inside its queue entry. - getHeadOid distinguishes an unborn HEAD from git failures: it returned '' for both, and callers treat '' as nothing-to-pull, so a git error during the delete-path check could have destroyed unsynced commits. Unborn still yields ''; any other failure now propagates and aborts deletion. Live end-to-end re-run against a real sandbox: unborn-repo no-op, fresh edit sync, tip convergence, stranded-commit recovery all pass; typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… races Third review round on the sync lifecycle: - Re-check the deletion tombstone at every registration point that follows an await in getSandbox: a deletion completing while a sandbox was being refreshed, reconnected, or created could otherwise persist a mapping (or a brand-new sandbox) for a session that no longer exists. A creation that loses the race discards its unregistered sandbox before throwing. - Deduplicate concurrent deleteSandbox calls through a shared per-session promise: a second teardown racing the first would observe the already- deleted sandbox, throw, and wrongly clear the tombstone, reopening the resurrection window. Typecheck clean; sync-path behavior unchanged (live E2E from the previous rounds still applies). Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…lization - Re-check the tombstone before returning a freshly created sandbox: a deletion completing during initializeAndSync owns and removes the registered sandbox, and returning it would hand callers a destroyed sandbox that fails confusingly on first use. - If discarding a creation that lost the deletion race itself fails, surface the sandbox id in the log and error so the orphan can be cleaned up manually instead of vanishing untracked. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Core fix for #46: OpenCode dispatches the plugin
eventhook without awaiting it, so the git sync started onsession.idleis invisible to hosts —session.idlecan be observed (and the run closed) while sandbox changes are still being pulled, and sync failures only surface in the plugin log and TUI toasts. Worse, asession.deletedarriving right after idle deleted the sandbox without syncing, potentially destroying the only copy of unsynced work mid-flight.Since the
session.idlecontract itself cannot be made awaitable from a plugin, this PR adds the completion boundaries #46 asks for:Changes
SessionGitManager): syncs are registered and serialized per session, so overlapping idle events cannot race and other code paths can wait on them.deleteSandbox): waits for any in-flight sync, then pulls remaining changes from a running sandbox before deletion. If unsynced changes cannot be pulled, deletion is aborted and the sandbox preserved — a failed sync no longer silently destroys work. A stopped sandbox is deleted without being started (already synced while running, or intentionally abandoned by the explicit delete).gitSynctool: an explicit, awaitable, failure-bearing sync. Returns only after commits are in the localopencode/Nbranch; git failures surface as structured tool errors observable through the session message stream. Supervisors driving OpenCode through the SDK can make the agent run it as the final step and check the result, instead of trustingsession.idle.disposehook: drains in-flight syncs so a graceful shutdown does not abandon a running sync. (disposeis newer than the published@opencode-ai/pluginHooks type; OpenCode versions without it simply ignore the extra key.)Testing
tsc --noEmitclean; build covered by CIgitSyncerrors propagate as tool errorsNote: textually adjacent to #48 in
session-events.tsbut semantically independent; either merges first.Summary by cubic
Makes git sync completion observable and prevents sandbox resurrection or data loss during delete. Previously
session.idlelaunched an unawaited sync andsession.deletedcould remove a sandbox mid-sync; now syncs queue per session, deletion is a queue barrier with a final pull, and late events cannot recreate sandboxes for deleted sessions.SessionGitManager.enqueueSessionSync; addwaitForPendingSync, and drain with a 60s-boundedwaitForAllPendingSyncsindispose.session.idlepipeline before any await; re-check a deleting session inside the queued task to avoid reviving it.sandbox.delete()as one queue entry; abort if the local repo is inaccessible or the pull fails.hasSandboxreports tombstoned sessions as absent.opencode/Nref;getHeadOiddistinguishes unborn HEADs from git errors; add hostgetRefOid.gitSynctool: explicit, awaitable sync that surfaces git errors and completes only after the local branch updates; update system prompt and README.Rollout
gitSyncas the final step and treat failures as sync failures.disposeignore it.Written for commit 9760a06. Summary will update on new commits.