feat: expose SDK-backed persona fleet spawning - #307
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds the ChangesPersona platform
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FleetNode
participant SpawnCapability
participant PersonaRegistry
participant PersonaPlan
participant PTYAgent
FleetNode->>SpawnCapability: Submit spawn:persona request
SpawnCapability->>PersonaRegistry: Resolve persona for cwd
PersonaRegistry-->>SpawnCapability: Return selection and warnings
SpawnCapability->>PersonaPlan: Prepare isolated mount with autosync
PersonaPlan-->>SpawnCapability: Return execution handle
SpawnCapability->>PTYAgent: Spawn configured persona agent
PTYAgent-->>FleetNode: Return readiness and spawn metadata
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb86fe7d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
packages/local-surface/src/index.ts (1)
11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the test hook from the public entry point.
Line 13 re-exports
__setPersonaSpawnImplementationsForTest. The hook replaces module-level implementation slots for the whole process.packages/local-surface/src/persona-spawn.test.tsline 9 imports it from./persona-spawn.jsdirectly, so the public re-export is not needed for the tests. Removing it keeps the swap reachable only from inside the package.♻️ Proposed change
export { WORKFORCE_PERSONA_SPAWN_CAPABILITY, - __setPersonaSpawnImplementationsForTest, defineWorkforcePersonaSpawnNode,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-surface/src/index.ts` around lines 11 - 21, Remove __setPersonaSpawnImplementationsForTest from the public export list in the package entry point, while leaving it available through the direct persona-spawn module import used by tests. Preserve all other exports, including defineWorkforcePersonaSpawnNode and workforcePersonaSpawnCapability.packages/local-surface/src/persona-spawn.test.ts (1)
49-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for resource cleanup and for the Fleet version gate.
This file has one test. Two behaviors that the PR introduces have no coverage:
- Cleanup. The test stubs
disposeas a no-op at line 63 and never asserts that it runs. No test covers scratch-directory removal or handle disposal after a successful spawn. A test here would have exposed the unreleased handles described in the comment onpackages/local-surface/src/persona-spawn.tslines 212-217.- The version gate. Line 59 stubs
checkFleetCompatibilityaway in every test. No test asserts that the default gate rejects an unsupported Fleet runtime with the documented error.Do you want me to draft both tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-surface/src/persona-spawn.test.ts` around lines 49 - 115, Add coverage in persona-spawn.test.ts for the successful spawn cleanup path and Fleet compatibility gate. Update the existing test or add focused tests around defineWorkforcePersonaSpawnNode/invokeNodeHandler to make dispose observable and assert it runs, along with scratch-directory removal after success. Restore the real checkFleetCompatibility behavior in a separate test and assert an unsupported Fleet runtime is rejected with the documented error, while keeping the existing coalescing assertions intact.packages/persona-registry/src/index.test.ts (1)
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the tests from ambient host configuration and clean up the temp directory.
loadPersonaSourceConfigderivesconfigPathfromdefaultWorkforceHomeDir(), which readsHOMEandAGENT_WORKFORCE_HOME. These two tests do not passworkforceHomeDir, so they read the real user config file. Test 3 also omitscwd, so it scans<repo>/.agentworkforce/workforce/personas. Both make the results host-dependent. Test 1 also leaves itsmkdtempSyncdirectory on disk.💚 Proposed fix for isolation and cleanup
test('resolves a built-in persona to an interactive selection', () => { - const resolved = resolvePersonaReference('persona-maker', { - cwd: mkdtempSync(join(tmpdir(), 'persona-registry-built-in-')), - personaDirs: [] - }); - - assert.equal(resolved.source, 'built-in'); - assert.equal(resolved.spec.id, 'persona-maker'); - assert.equal(resolved.selection.personaId, 'persona-maker'); - assert.equal(resolved.selection.harness, resolved.spec.harness); - assert.equal(resolved.selection.model, resolved.spec.model); + const root = mkdtempSync(join(tmpdir(), 'persona-registry-built-in-')); + try { + const resolved = resolvePersonaReference('persona-maker', { + cwd: root, + workforceHomeDir: join(root, 'home'), + personaDirs: [] + }); + + assert.equal(resolved.source, 'built-in'); + assert.equal(resolved.spec.id, 'persona-maker'); + assert.equal(resolved.selection.personaId, 'persona-maker'); + assert.equal(resolved.selection.harness, resolved.spec.harness); + assert.equal(resolved.selection.model, resolved.spec.model); + } finally { + rmSync(root, { recursive: true, force: true }); + } });Apply the same
cwdandworkforceHomeDirisolation to thedoes-not-existtest.Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-registry/src/index.test.ts` around lines 9 - 13, Update the affected persona resolution tests to provide isolated temporary cwd and workforceHomeDir values, including the does-not-exist case, so they do not read ambient configuration or repository persona directories. Track the temporary directories created by mkdtempSync in the built-in test and clean them up after each test using the existing test lifecycle utilities.packages/persona-registry/src/index.ts (1)
133-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing built-in lookup helper.
findInLibraryinpackages/persona-registry/src/local-personas.ts(lines 730-737) already implements this exact precedence: catalog by intent, then built-in list by id. Two copies can drift, and then path-based inheritance and direct resolution would disagree about which built-in wins. Export the helper and call it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-registry/src/index.ts` around lines 133 - 134, Replace the duplicated byIntent/builtIn resolution in the surrounding lookup flow with the exported findInLibrary helper from local-personas.ts. Update that helper’s export as needed, and pass the existing persona catalog and lookupId so direct resolution uses the same catalog-then-built-in precedence as path-based inheritance.packages/persona-registry/package.json (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompiled tests ship in the published tarball.
tsconfig.jsonincludessrc/**/*.ts, sodist/index.test.jsis produced, andfilesships all ofdist. Consider a separate test tsconfig, or exclude*.test.*from the publishedfileslist. Addinglicenseanddescriptionalso removes npm publish warnings for a public package.♻️ Proposed metadata and file-list change
"files": [ "dist", + "!dist/**/*.test.*", "README.md", "package.json" ],Also applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-registry/package.json` around lines 14 - 18, Update the package metadata and publication configuration for packages/persona-registry: prevent compiled test artifacts such as dist/index.test.js from being included in the published tarball by using a test-specific TypeScript configuration or excluding test files from the published dist contents, and add the package license and description metadata to avoid npm publish warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-14: Update the [Unreleased] Added entry in CHANGELOG.md by
splitting the combined description into separate wrapped bullets for
`@agentworkforce/persona-registry` and `@agentworkforce/local-surface`. In the
local-surface bullet, explicitly record the Agent Relay 11.5 requirement
enforced by assertFleetCompatibility in persona-spawn.ts, while preserving the
existing capability details.
In `@packages/local-surface/package.json`:
- Around line 35-38: Update the `@agent-relay/fleet` dependency range in the
package manifest to require the minimum version accepted by
assertFleetCompatibility, 11.5 or newer, so incompatible 11.4.x installations
are rejected at install time.
In `@packages/local-surface/README.md`:
- Around line 26-29: Update the deduplication wording in the README to include
the agent name alongside node, project, and persona, matching the corrected key
in persona-spawn.ts. Ensure it states that only requests sharing all four values
reuse one launch.
In `@packages/local-surface/src/persona-spawn.test.ts`:
- Around line 105-107: Replace the setImmediate wait in the persona spawn test
with a deterministic barrier tied to the spawnAgent stub’s resolution. Configure
or capture the promise returned by the spawnAgent stub, await that promise
before asserting executeCalls and spawnCalls, and preserve the existing
call-count expectations.
In `@packages/local-surface/src/persona-spawn.ts`:
- Around line 212-217: The successful spawn path in launchResolvedPersona must
release resources when the launched agent exits: dispose the stored
ExecutionHandle, remove its scratchDir, and dispose any existing active entry
before overwriting the same key. Update
packages/local-surface/src/persona-spawn.ts lines 212-217 accordingly; retain
the teardown-flush sentence in packages/local-surface/README.md lines 30-31 once
implemented.
- Around line 118-123: Update the in-flight deduplication key in
personaSpawnRequest to include input.name alongside the node, cwd, and persona
identity, so distinct Relay identities receive separate launches and capacity
requests. Adjust the concurrent-request test to use the same name for both
requests, preserving deduplication only for identical identities.
- Around line 234-243: Remove assertFleetCompatibility and its untyped
FLEET_DYNAMIC_SPAWN_DELEGATION runtime check; eliminate any now-unused
references. Ensure spawn:persona relies only on the supported typed
`@agent-relay/fleet` API and retains its existing behavior without this
undocumented guard.
In `@packages/persona-kit/src/mount.test.ts`:
- Around line 62-68: Update the test around handle.dispose() so it specifically
covers disposal sync-back rather than autosync polling: rename the test to
reflect that behavior and use configuration that does not imply running autosync
coverage, or otherwise separate/skip the shutdown reconciliation while testing a
small non-zero scan interval. Keep the persisted result assertion for the
disposal-sync case.
In `@packages/persona-kit/src/mount.ts`:
- Around line 103-109: Ensure autosync lifecycle failures always remove the
mount in packages/persona-kit/src/mount.ts: for the startAutoSync and ready()
flow at lines 103-109, wrap startup and await Promise.resolve(autoSync?.ready())
in try/catch, call handle.cleanup() on failure, then rethrow; for disposal at
lines 111-122, attach handle.cleanup() in a finally block around await
autoSync?.stop() so cleanup runs even when stopping rejects.
In `@packages/persona-registry/README.md`:
- Around line 7-12: Update the README example’s resolvePersonaReference call to
use one of the documented built-in intents, persona-authoring or
persona-improvement, instead of code-reviewer; do not add a new persona unless
that is required to preserve the example’s intended behavior.
In `@packages/persona-registry/src/index.ts`:
- Around line 73-89: Update the selector classification around explicitPath in
the persona resolution flow so a bare selector is treated as a path only when
candidatePath exists as a JSON file; directories or non-JSON matches must
continue through registry lookup. Preserve the existing invalid_reference errors
for selectors identified by looksLikePath, including missing or non-JSON path
references.
In `@packages/persona-registry/src/local-personas.ts`:
- Around line 430-435: Normalize validated raw.id and raw.extends by trimming
their string values before returning the parsed override. Update the parser’s
returned persona override so readLayerDir and findOverrideIdInLayer use these
normalized values consistently with resolvePersonaReference, while preserving
undefined extends behavior.
- Around line 246-259: Update dedupeDirs to accept the caller’s base directory
and pass it to normalizePersonaDir, then thread the appropriate options.cwd
through every dedupeDirs call, including the path around the additional
referenced lines. Preserve absolute config-file directories while resolving
caller-supplied relative personaDirs against options.cwd instead of
process.cwd().
---
Nitpick comments:
In `@packages/local-surface/src/index.ts`:
- Around line 11-21: Remove __setPersonaSpawnImplementationsForTest from the
public export list in the package entry point, while leaving it available
through the direct persona-spawn module import used by tests. Preserve all other
exports, including defineWorkforcePersonaSpawnNode and
workforcePersonaSpawnCapability.
In `@packages/local-surface/src/persona-spawn.test.ts`:
- Around line 49-115: Add coverage in persona-spawn.test.ts for the successful
spawn cleanup path and Fleet compatibility gate. Update the existing test or add
focused tests around defineWorkforcePersonaSpawnNode/invokeNodeHandler to make
dispose observable and assert it runs, along with scratch-directory removal
after success. Restore the real checkFleetCompatibility behavior in a separate
test and assert an unsupported Fleet runtime is rejected with the documented
error, while keeping the existing coalescing assertions intact.
In `@packages/persona-registry/package.json`:
- Around line 14-18: Update the package metadata and publication configuration
for packages/persona-registry: prevent compiled test artifacts such as
dist/index.test.js from being included in the published tarball by using a
test-specific TypeScript configuration or excluding test files from the
published dist contents, and add the package license and description metadata to
avoid npm publish warnings.
In `@packages/persona-registry/src/index.test.ts`:
- Around line 9-13: Update the affected persona resolution tests to provide
isolated temporary cwd and workforceHomeDir values, including the does-not-exist
case, so they do not read ambient configuration or repository persona
directories. Track the temporary directories created by mkdtempSync in the
built-in test and clean them up after each test using the existing test
lifecycle utilities.
In `@packages/persona-registry/src/index.ts`:
- Around line 133-134: Replace the duplicated byIntent/builtIn resolution in the
surrounding lookup flow with the exported findInLibrary helper from
local-personas.ts. Update that helper’s export as needed, and pass the existing
persona catalog and lookupId so direct resolution uses the same
catalog-then-built-in precedence as path-based inheritance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2a60958-bb82-4c83-9d6c-2c89db56901b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/publish.yml.github/workflows/verify-publish.ymlCHANGELOG.mdREADME.mdpackages/cli/package.jsonpackages/cli/src/local-personas.tspackages/local-surface/README.mdpackages/local-surface/package.jsonpackages/local-surface/src/index.tspackages/local-surface/src/persona-spawn.test.tspackages/local-surface/src/persona-spawn.tspackages/persona-kit/src/mount.test.tspackages/persona-kit/src/mount.tspackages/persona-registry/README.mdpackages/persona-registry/package.jsonpackages/persona-registry/src/index.test.tspackages/persona-registry/src/index.tspackages/persona-registry/src/local-personas.tspackages/persona-registry/tsconfig.json
![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 20 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/persona-registry/src/index.test.ts (1)
49-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert inherited values against
persona-maker.Line 58 compares two fields from the same resolved result. This can pass if inheritance is ignored and both fields are
undefined. Resolvepersona-makerseparately and compare the inherited fields with that result.Proposed test improvement
const resolved = resolvePersonaReference(path, { cwd: project, personaDirs: [] }); + const parent = resolvePersonaReference('persona-maker', { + cwd: project, + personaDirs: [] + }); assert.equal(resolved.source, 'path'); assert.equal(resolved.path, path); assert.equal(resolved.spec.id, 'review-via-path'); assert.equal(resolved.spec.description, 'Path-selected reviewer'); + assert.equal(resolved.spec.harness, parent.spec.harness); + assert.equal(resolved.spec.model, parent.spec.model); assert.equal(resolved.selection.harness, resolved.spec.harness);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/persona-registry/src/index.test.ts` around lines 49 - 58, Update the test around resolvePersonaReference to resolve the persona-maker reference separately, then compare the inherited fields on the path-selected result against the corresponding values from the persona-maker result. Replace the self-comparison of resolved.selection.harness and resolved.spec.harness so the test fails when inheritance is ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/persona-registry/src/index.test.ts`:
- Around line 49-58: Update the test around resolvePersonaReference to resolve
the persona-maker reference separately, then compare the inherited fields on the
path-selected result against the corresponding values from the persona-maker
result. Replace the self-comparison of resolved.selection.harness and
resolved.spec.harness so the test fails when inheritance is ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15d03b0b-6f6e-4219-992a-4811645a1df1
📒 Files selected for processing (10)
CHANGELOG.mdpackages/local-surface/README.mdpackages/local-surface/package.jsonpackages/local-surface/src/persona-spawn.test.tspackages/local-surface/src/persona-spawn.tspackages/persona-kit/src/mount.tspackages/persona-registry/README.mdpackages/persona-registry/src/index.test.tspackages/persona-registry/src/index.tspackages/persona-registry/src/local-personas.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/persona-registry/README.md
- packages/local-surface/src/persona-spawn.test.ts
- packages/local-surface/README.md
- packages/local-surface/package.json
- packages/persona-registry/src/index.ts
- packages/persona-kit/src/mount.ts
- packages/local-surface/src/persona-spawn.ts
- packages/persona-registry/src/local-personas.ts
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
2 participants
Summary
@agentworkforce/persona-registrySDK, including exact highest-priority JSON-path resolution and typed errorsdefineWorkforcePersonaSpawnNode()/spawn:personato local-surface, using persona-kit in process for harness, model, standing instructions, installed skills, MCP servers, and harness settingsThis is the Workforce part of #306. There is no
agentworkforcesubprocess or output parsing in the launch path.Verification
Stack
Depends on AgentWorkforce/relay#1464 and its Agent Relay 11.5+ publication. The manifest remains installable against the published 11.4 line, but
spawn:personachecks Relay's new runtime compatibility marker and fails loudly on older Fleet versions instead of silently delegating to the wrong harness. Fresh installs will select 11.5 once it is published.