fix(local-surface): expose prepared persona execution ownership - #339
Conversation
Session-Id: 01a09aa7-26ec-7411-ae96-559372458f12
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe local surface now exposes prepared persona execution resources and an optional ownership callback. The callback runs before broker delegation. Failure paths dispose resources and remove scratch directories. Tests and documentation cover ownership, cleanup, coalescing, and mounted project contents. ChangesPrepared execution ownership
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Host
participant launchResolvedPersona
participant PersonaExecutor
participant Broker
Host->>launchResolvedPersona: provide onExecutionPrepared
launchResolvedPersona->>PersonaExecutor: prepare execution
launchResolvedPersona->>Host: await callback with handle and scratchDir
Host-->>launchResolvedPersona: retain execution ownership
launchResolvedPersona->>Broker: delegate persona spawn
Broker-->>Host: launched worker
Host->>Broker: release launched worker
Host->>PersonaExecutor: dispose handle
Host->>PersonaExecutor: remove scratchDir
Merge Risk: ⚪ Minimal · up to The new prepared-execution callback and cleanup lifecycle are documented and covered without an identified current-head regression, so the change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit guards the handle bright, Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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 4 files
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Session-Id: 01a09aa7-26ec-7411-ae96-559372458f12
2 participants
A host serving
spawn:personacannot retain the executor's actual handle before delegation or dispose a successful persona mount through the factory's public API. Add an optional, awaitedonExecutionPrepared(name, execution)callback exposing that handle and its scratch directory before a worker can start.The callback runs once for coalesced requests. Rejection prevents delegation and triggers factory cleanup. Following a successful spawn, the documented host contract is to release and verify the specific worker first, then dispose the retained handle and remove its scratch directory. Failure cleanup now attempts scratch removal even if disposal throws. Export the receipt type as
WorkforcePersonaExecution.Validation under native Node v22.22.1:
pnpm --filter @agentworkforce/local-surface... run buildpassed.pnpm --filter @agentworkforce/local-surface testpassed: 19 tests, including async ownership ordering, coalescing, callback/delegation/disposal failures, and real executor mount cleanup after success.Scope is limited to the local-surface callback, exported type, lifecycle documentation, and tests. This draft has no live fleet/provider acceptance claim.
Note
Medium Risk
Changes persona spawn lifecycle and resource ownership for hosts that opt into the callback; optional API preserves default behavior but incorrect host cleanup could leak mounts or dispose too early.
Overview
Adds an optional
onExecutionPrepared(name, execution)hook ondefineWorkforcePersonaSpawnNodeandworkforcePersonaSpawnCapabilityso hosts can keep the real executorExecutionHandleand factoryscratchDirbefore the broker spawns a worker. The factory awaits this callback once per prepared launch (including coalesced duplicate requests); rejection skips delegation and triggers cleanup.Exports
WorkforcePersonaExecutionfor typing the receipt. Error paths now remove the scratch directory in afinallyblock even whenhandle.dispose()throws. README documents the host contract after success: release the worker first, then dispose the retained handle and deletescratchDir. Omitting the callback leaves prior successful-execution lifetime unchanged.Tests cover delegation ordering vs. ownership, coalescing, failure cleanup (ownership, delegation, dispose), and real mount teardown after success.
Reviewed by Cursor Bugbot for commit a248dc1. Bugbot is set up for automated code reviews on this repo. Configure here.