sdk: migrate persona spawn to @agentworkforce/persona-kit#973
Conversation
Replaces @agentworkforce/harness-kit + @agentworkforce/workload-router with @agentworkforce/persona-kit@^3, and rewrites AgentRelay.spawnPersona to run the full persona lifecycle (skills install, mount policy, CLAUDE.md / AGENTS.md sidecars, harness config files, persona inputs) via persona-kit's buildPersonaSpawnPlan + executePersonaSpawnPlan. All side effects are reversed when the spawned agent exits. The relay-side personas module now only owns the search-dir cascade and file discovery; parsing, plan construction, and execution belong to persona-kit so relay and the workforce CLI launch personas identically. Adds AgentRelay.getPersonaSpawnPlan(id) for dry-run inspection of the plan without filesystem writes or subprocess spawns. Breaking: the persona tier system is dropped, along with the legacy relay-side PersonaFile / PersonaTier / ResolvedPersona / PersonaSpawnSpec types and the buildPersonaSpawnSpec / materializePersonaConfigFiles / restorePersonaConfigFiles helpers. loadPersona now returns the canonical PersonaSpec; spawnPersona's `persona` override takes a PersonaSpec; the `tier` option is gone. Closes #832.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates the SDK to ChangesPersona-kit migration and spawn-plan refactor
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AgentRelay.spawnPersona
participant buildPersonaSpawnPlan
participant executePersonaSpawnPlan
participant spawnPty
participant Agent
Caller->>AgentRelay.spawnPersona: personaId/options
AgentRelay.spawnPersona->>buildPersonaSpawnPlan: load/resolve persona
buildPersonaSpawnPlan-->>AgentRelay.spawnPersona: PersonaSpawnPlan
AgentRelay.spawnPersona->>executePersonaSpawnPlan: plan, mount, skillsInstallRoot
executePersonaSpawnPlan-->>AgentRelay.spawnPersona: ExecutionHandle (cwd)
AgentRelay.spawnPersona->>spawnPty: harness args, cwd
spawnPty-->>AgentRelay.spawnPersona: Agent (PTY)
Agent->>Agent: running
Agent-->>AgentRelay.spawnPersona: exit
AgentRelay.spawnPersona->>executePersonaSpawnPlan: schedule handle.dispose()
executePersonaSpawnPlan-->>AgentRelay.spawnPersona: cleanup result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: d2fbe67af0
ℹ️ 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".
| } catch (err) { | ||
| restorePersonaConfigFiles(writes); | ||
| await handle.dispose(); | ||
| throw err; |
There was a problem hiding this comment.
If spawnPty throws and handle.dispose() also throws, the cleanup error replaces the original spawn failure because await handle.dispose() runs before throw err. In failure cases (for example, invalid CLI args plus unmount/cleanup failure), callers lose the actionable root cause from spawnPty, which makes retries and diagnosis much harder. Wrap dispose in its own try/catch (or suppress secondary errors) so the original spawn exception is always propagated.
Useful? React with 👍 / 👎.
| ...(options.envOverrides !== undefined ? { envOverrides: options.envOverrides } : {}), | ||
| ...(options.inputValues !== undefined ? { inputValues: options.inputValues } : {}), | ||
| }; | ||
| return getPersonaSpawnPlanImpl(personaId, callOptions); |
There was a problem hiding this comment.
getPersonaSpawnPlan accepts SpawnPersonaOptions, which includes a persona override, but this method always reloads by personaId and never uses options.persona. Callers that pre-load/modify a persona in memory (the same workflow supported by spawnPersona) will get a plan for on-disk JSON instead of their override, causing dry-run validation to diverge from actual spawn behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/relay.ts (1)
311-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
spawnPersona()silently ignoresoptions.model.
SpawnPersonaOptionsstill exposesmodelthroughSpawnOptions, but this path always forwardsplan.persona.model. Existing callers that override the model will now get the persona's model instead.Suggested fix
- model: plan.persona.model, + model: options.model ?? plan.persona.model,Also applies to: 972-980
🤖 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/sdk/src/relay.ts` around lines 311 - 352, spawnPersona silently ignores caller-provided model because the code always uses plan.persona.model (from the loaded persona) instead of honoring options.model from SpawnPersonaOptions/SpawnOptions; update spawnPersona so when constructing or normalizing the plan it prefers options.model (if provided and non-empty) over the persona's model and avoid overwriting plan.persona.model with the persona default when options.model exists; specifically, adjust the code paths that read/assign plan.persona.model (and the similar block referenced around lines 972-980) to conditionally set model = options.model ?? persona.model (or skip assignment when options.model is present).
🧹 Nitpick comments (1)
packages/sdk/src/__tests__/personas.test.ts (1)
234-236: 💤 Low valueConsider adding intermediate assertions for nested config access.
Lines 235-236 access
parsed.agent['opencode-nano']directly without verifying thatparsed.agentexists. While the current code will throw a TypeError if the structure is malformed (which is acceptable for tests), adding explicit assertions would make failures clearer:assert.ok(parsed.agent, 'parsed config should have agent field'); assert.equal(parsed.agent['opencode-nano'].model, 'opencode/gpt-5-nano');🤖 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/sdk/src/__tests__/personas.test.ts` around lines 234 - 236, The test directly accesses parsed.agent['opencode-nano'] which can produce an unclear TypeError if parsed.agent is missing; add an explicit assertion that parsed.agent exists (e.g., assert.ok(parsed.agent, 'parsed config should have agent field')) before asserting on parsed.agent['opencode-nano'].model and parsed.agent['opencode-nano'].prompt so failures report a clear message referencing parsed and parsed.agent.
🤖 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 `@packages/sdk/src/personas.ts`:
- Around line 217-223: The code currently throws if a readable candidate file
contains invalid JSON or fails parsePersonaJson validation, which prevents
falling through the cascade; in findPersona, after reading candidateBytes (and
before calling parsePersonaJson), wrap JSON.parse and parsePersonaJson in a
try/catch that treats parse/validation errors as a non-fatal skip (do not
rethrow) so the function continues searching other directories; specifically
update the block that uses readFileSync, candidateBytes,
JSON.parse(candidateBytes) and parsePersonaJson(...) to catch errors from
JSON.parse and parsePersonaJson and ignore them (leaving candidateBytes
undefined or continuing) so bad shadow files don’t block lower-priority persona
matches.
In `@packages/sdk/src/relay.ts`:
- Around line 331-352: The dry-run helper getPersonaSpawnPlan (and the other
dry-run helpers used by spawnPersona) currently ignores options.persona and
always reloads the persona by personaId; change the logic so that if
options.persona is provided it is used as the source PersonaSpec instead of
reloading from personaId, and ensure that downstream plan construction respects
that persona and still merges/overrides inputValues, envOverrides,
skillsInstallRoot and cleanupSkillsOnDispose exactly as spawnPersona would;
update getPersonaSpawnPlan and the corresponding helper(s) referenced by
spawnPersona to prefer options.persona (highest precedence) and fall back to
loading by personaId only when absent.
- Around line 954-956: The dropped-env computation currently compares plan.env
against process.env; change it to compare against the broker's effective startup
env (this.clientOptions.env ?? process.env) so a relay created with options.env
uses the broker's actual env for warnings. Update the filter that builds
droppedEnv to use (this.clientOptions.env ?? process.env)[key] in place of
process.env[key], keeping the rest of the check against plan.env as-is; ensure
you reference the existing variables droppedEnv, plan.env and
this.clientOptions.env in relay.ts when making the change.
- Around line 992-994: The catch block after spawnPty() currently awaits
handle.dispose() and then throws the caught err, but if handle.dispose() itself
throws the dispose error replaces the original spawn error; update the catch to
call handle.dispose() inside its own try/catch (or use finally-like safe
dispose) so any disposal error is caught/logged and does not overwrite the
original err, then rethrow the original spawn error from the outer catch;
reference spawnPty() and handle.dispose() to locate and change that catch block.
---
Outside diff comments:
In `@packages/sdk/src/relay.ts`:
- Around line 311-352: spawnPersona silently ignores caller-provided model
because the code always uses plan.persona.model (from the loaded persona)
instead of honoring options.model from SpawnPersonaOptions/SpawnOptions; update
spawnPersona so when constructing or normalizing the plan it prefers
options.model (if provided and non-empty) over the persona's model and avoid
overwriting plan.persona.model with the persona default when options.model
exists; specifically, adjust the code paths that read/assign plan.persona.model
(and the similar block referenced around lines 972-980) to conditionally set
model = options.model ?? persona.model (or skip assignment when options.model is
present).
---
Nitpick comments:
In `@packages/sdk/src/__tests__/personas.test.ts`:
- Around line 234-236: The test directly accesses parsed.agent['opencode-nano']
which can produce an unclear TypeError if parsed.agent is missing; add an
explicit assertion that parsed.agent exists (e.g., assert.ok(parsed.agent,
'parsed config should have agent field')) before asserting on
parsed.agent['opencode-nano'].model and parsed.agent['opencode-nano'].prompt so
failures report a clear message referencing parsed and parsed.agent.
🪄 Autofix (Beta)
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: ec10bc3a-41d2-4bb8-af74-5714c5e442f0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
CHANGELOG.mdpackages/sdk/package.jsonpackages/sdk/src/__tests__/personas.test.tspackages/sdk/src/personas.tspackages/sdk/src/relay.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Fixes flagged by codex, CodeRabbit, and cubic on PR #973: - spawnPersona: wrap handle.dispose() in its own try/catch on the spawnPty failure path so a dispose error never masks the original spawn error (P2, three reviewers). - getPersonaSpawnPlan: honor options.persona so dry-run plans match what spawnPersona would execute for the same options (P2, three reviewers). - spawnPersona: compute dropped-env warning against the broker's effective env (this.clientOptions.env ?? process.env) instead of always process.env (P3, two reviewers). - findPersona: tolerate a malformed shadow file at the conventional <id>.json path so it doesn't block a valid persona lower in the cascade — matches the directory-scan path's existing behavior. Adds tests for getPersonaSpawnPlan({ persona }) and the findPersona shadow-file cascade fallthrough; updates the parse-error test to assert the new cascade semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/sdk/src/__tests__/personas.test.ts`:
- Line 333: The test currently uses assert.notMatch(found?.path ?? '', new
RegExp(shadowDir)) which treats filesystem characters as regex; replace that
assertion with a direct string comparison so the path is compared literally
(e.g., change the assert.notMatch call to a direct inequality check using
assert.notStrictEqual or assert.notEqual against shadowDir), referencing the
found?.path and shadowDir variables and updating the assertion in
personas.test.ts accordingly.
🪄 Autofix (Beta)
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: 6efceb77-8a21-4898-91c2-27da4f604788
📒 Files selected for processing (3)
packages/sdk/src/__tests__/personas.test.tspackages/sdk/src/personas.tspackages/sdk/src/relay.ts
persona-kit 3.0.20 made PersonaSpec.harness / .model / .systemPrompt optional for handler-style (onEvent-driven) personas. Relay only spawns interactive personas, so resolvePersona now narrows those fields with explicit runtime guards that throw a clear error instead of letting tsc fail or the cast silently drop the fields. Bumps the SDK's persona-kit range to ^3.0.20 so the guard's narrowing is actually exercised against the new optional shape. Fixes the 'Publish Fresh Install Build' CI break that pulled 3.0.20 via the semver range and failed tsc on the previous resolvePersona which assigned `Harness | undefined` into a `Harness` field.
|
@copilot resolve the merge conflicts in this pull request. also respond/fix any comments made on PR |
Co-authored-by: willwashburn <957608+willwashburn@users.noreply.github.com>
Agent-Logs-Url: https://github.com/AgentWorkforce/relay/sessions/97e9d7f3-e4dc-4e56-984b-5eb5936a17a3 Co-authored-by: willwashburn <957608+willwashburn@users.noreply.github.com>
Resolved. I merged latest |
Summary
@agentworkforce/harness-kit+@agentworkforce/workload-routerwith@agentworkforce/persona-kit@^3in@agent-relay/sdk.AgentRelay.spawnPersonato run the full persona-kit lifecycle (skill installs, mount policy,CLAUDE.md/AGENTS.mdsidecars, harness config files, persona inputs) viabuildPersonaSpawnPlan+executePersonaSpawnPlan. All side effects are reversed in LIFO order when the spawned agent exits.AgentRelay.getPersonaSpawnPlan(id)(and a module-levelgetPersonaSpawnPlanexport) for dry-run inspection of a persona's resolved harness argv, skill installs, mount policy, sidecars, and inputs — no fs writes, no subprocesses.[Unreleased]inCHANGELOG.mdwith the new behavior, the breaking changes, and migration guidance.Closes #832.
Why
@agent-relay/sdk's previousspawnPersonaonly translated a persona's harness/model/system-prompt and silently dropped the rest of the schema — skills, mount policy, sidecar markdown, inputs. The header comment of the oldpersonas.tsdocumented this explicitly. As #832 framed it: a persona stripped of skills and mounts is just a system prompt, not a persona. This PR consolidates relay onto the same@agentworkforce/persona-kitlibrary theagentworkforceCLI uses, so a persona launched through relay behaves identically to one launched through the CLI.Scope decisions
tieroption,PersonaTier,PersonaTierSpec, and tieredPersonaFileare gone.loadPersonareturns persona-kit's canonicalPersonaSpec; the relay-specificPersonaFile/ResolvedPersona/PersonaSpawnSpecshapes are removed.defaultPersonaSearchDirs,listPersonas,findPersona, andloadPersonastay in@agent-relay/sdk/personas. Everything else (parsing, plan building, side-effect execution) delegates to persona-kit.getPersonaSpawnPlan(id)+ manualspawnPty.Breaking changes
@agentworkforce/harness-kit,@agentworkforce/workload-router.@agent-relay/sdk/personas:PersonaTier,PersonaTierSpec,PersonaFile,ResolvedPersona(relay's local shape),PersonaSpawnSpec,MaterializedConfigFile,buildPersonaSpawnSpec,materializePersonaConfigFiles,restorePersonaConfigFiles.loadPersona(id)now returnsPersonaSpec(persona-kit's canonical shape), not the previous relay-localResolvedPersona.SpawnPersonaOptions:tieris gone;personanow takes aPersonaSpec; new options areskillsInstallRoot,inputValues,envOverrides,cleanupSkillsOnDispose,mount.tiers.*need to be flattened to top-levelharness/model/systemPrompt.Known limitation
The broker's
spawnPtyAPI does not yet accept a per-spawnenv, so persona-kit'splan.env(persona-author env + resolved input bindings) cannot be forwarded to the spawned PTY today.spawnPersonaemits aconsole.warnlisting any env keys it had to drop so the caller can set them in the spawning process env or viaoptions.envOverrides. Wiringenvthrough the broker SDK is a separate task.Test plan
npm run check(workspace@agent-relay/sdk) — clean.npm run build— clean.npx vitest run src/__tests__/personas.test.ts— 14/14 passing. Covers: search-dir cascade, JSON discovery,loadPersonahappy path + missing-file error + parse-error surfacing,resolvePersonaprojection,getPersonaSpawnPlanfor claude / codex / opencode, plan JSON round-trip,AgentRelay.spawnPersona+getPersonaSpawnPlanhonoringpersonaDirs, per-callsearchDirsoverride.packages/openclaw,packages/acp-bridge,packages/gateway, etc.) don't import the persona module — onlypackages/sdkitself does — so no downstream code changes are required.Out of scope
personaCatalog(workforce-CLI internals; relay doesn't need them — that's why@agentworkforce/workload-routerwas dropped, not replaced).plan.envthroughspawnPty— needs a broker-side change toSpawnPtyInput.docs/mirror (out of scope per.claude/rules/docs-sync.md; the docs directory will be handled separately).https://claude.ai/code/session_01498supaqXmpq7yWvmoYawJ
Generated by Claude Code