Compose top-level persona-kit orchestration API#74
Merged
Conversation
Introduce the plan/execute API the workforce CLI and relay SDK consume:
- buildPersonaSpawnPlan: pure plan builder composing buildInteractiveSpec,
materializeSkills, resolvePersonaInputs, and the sidecar/mount/input
resolvers into a single JSON-serializable PersonaSpawnPlan.
- executePersonaSpawnPlan: side-effecting executor that runs mount → skills
→ sidecars → configFiles in order with LIFO dispose-on-error. Returns an
ExecutionHandle whose dispose() reverses every side effect (idempotent).
- Piecewise helpers (applyPersonaMount, runSkillInstalls,
writePersonaSidecars, materializePersonaConfigFiles), each returning a
uniform { dispose(): Promise<void> } handle for advanced callers.
Closes #66.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a pure plan builder (buildPersonaSpawnPlan), piecewise helpers (applyPersonaMount, runSkillInstalls, writePersonaSidecars, materializePersonaConfigFiles), a deterministic executor (executePersonaSpawnPlan) with LIFO disposal and idempotent handles, top-level exports, README, package dependency, and comprehensive tests. ChangesPersona Spawn Orchestration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor as executePersonaSpawnPlan
participant Mount as applyPersonaMount
participant Skills as runSkillInstalls
participant Sidecars as writePersonaSidecars
participant Config as materializePersonaConfigFiles
Caller->>Executor: executePersonaSpawnPlan(plan, options)
Executor->>Mount: applyPersonaMount(plan.mount, options)
Mount-->>Executor: mountHandle
Executor->>Skills: runSkillInstalls(plan.skills, options)
Skills-->>Executor: skillsHandle
Executor->>Sidecars: writePersonaSidecars(plan.sidecars, options)
Sidecars-->>Executor: sidecarHandle
Executor->>Config: materializePersonaConfigFiles(plan.configFiles, options)
Config-->>Executor: configHandle
Executor-->>Caller: ExecutionHandle (cwd, dispose)
Note over Executor: On error -> dispose handles in LIFO order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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: c74f1806c5
ℹ️ 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".
Comment on lines
+118
to
+122
| const harness = selection.runtime.harness; | ||
| if (harness === 'claude') { | ||
| if (selection.claudeMdContent) { | ||
| return [ | ||
| { |
There was a problem hiding this comment.
buildPersonaSpawnPlan only emits sidecar writes when claudeMdContent/agentsMdContent are already inlined, so personas that declare claudeMd or agentsMd file paths produce no sidecar entries and lose their operating instructions at runtime. This regresses local/pack personas that rely on path-based sidecars: execution succeeds but launches without the expected CLAUDE.md/AGENTS.md content.
Useful? React with 👍 / 👎.
Comment on lines
+80
to
+83
| mountHandle = await applyPersonaMount(plan.mount, { | ||
| cwd: options.cwd, | ||
| ...(options.mount ?? {}) | ||
| }); |
There was a problem hiding this comment.
When a plan includes a mount policy, executePersonaSpawnPlan forwards only options.mount to applyPersonaMount, so callers must redundantly pass personaId even though plan.persona.personaId is already available. If they omit it (a likely case, since only mountDir is truly unknown), execution throws before any work starts, making mount-enabled plans fail unexpectedly.
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: 6
🤖 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/persona-kit/README.md`:
- Around line 25-28: The README example incorrectly passes an execution option
(cwd) into buildPersonaSpawnPlan; update the example so
buildPersonaSpawnPlan(persona, { inputValues: { TASK: 'tidy up the README' } })
only receives PlanOptions, and demonstrate passing { cwd: process.cwd() } as
part of the ExecuteOptions when actually executing the plan (e.g., when calling
spawn/execute on the returned plan). Reference buildPersonaSpawnPlan and the
plan execution call so readers see the PlanOptions vs ExecuteOptions split
clearly.
In `@packages/persona-kit/src/plan.ts`:
- Around line 78-80: PlanOptions.cwd is documented but never applied; either
remove it or wire it into all plan/harness creation paths. If keeping it, use
PlanOptions.cwd (defaulting to process.cwd()) when constructing the Plan and
when spawning any harness/child process (e.g., inside the Plan constructor,
createPlan/createPlanFromOptions, or spawnHarness/Harness.spawn) so the working
directory is passed to child_process spawn/execa calls; if you prefer to remove
it, delete cwd from PlanOptions and remove any related doc comments and default
handling.
- Around line 212-227: The code currently copies the entire ambient process.env
into the plan via the else branch, which can leak secrets; change the logic so
ambient env is NOT captured by default and only include process.env when
explicitly provided via options.processEnv. Concretely, remove the else block
that iterates over process.env (leave only the if (options.processEnv) branch
that copies string values), and ensure env is populated only from inputBindings,
options.envOverrides, and persona.env unless options.processEnv was passed;
update any related tests or callers that relied on implicit capture to pass
options.processEnv when they truly want full ambient env.
In `@packages/persona-kit/src/sidecars.ts`:
- Around line 40-42: The loop uses sidecar.filename directly when building
target (join(options.cwd, sidecar.filename)), allowing absolute paths or ../
traversal; validate/sanitize filename before joining: either replace
sidecar.filename with path.basename(sidecar.filename) or resolve the candidate
path and ensure it is inside options.cwd (resolve(join(options.cwd,
sidecar.filename)) startsWith resolve(options.cwd) + path.sep or equals it); if
validation fails, reject/skip the sidecar or throw. Update the logic around the
for (const sidecar of sidecars) block and the creation of target (used by
readIfExists and subsequent writes) to use the sanitized filename or validated
resolved path.
In `@packages/persona-kit/src/skill-runner.ts`:
- Around line 102-105: The cleanup loop using plan.installs ->
install.cleanupPaths currently resolves paths with isAbsolute/join and calls
rm(...{recursive:true, force:true}) without safety checks; restrict deletions to
inside the intended workspace by resolving each candidate via
path.resolve(options.cwd, path), verifying the resolved path startsWith the
resolved workspace root (e.g., const workspaceRoot = path.resolve(options.cwd)),
and skipping/throwing for any absolute/path-traversal targets or if the resolved
path equals filesystem root; only call rm on paths that pass this containment
check (reference: the loop iterating plan.installs and install.cleanupPaths and
the rm(...) call).
- Around line 36-39: signalExitCode currently returns a hardcoded 129 for any
signal; update the function to compute 128 + the actual signal number by looking
up the signal numeric code: use os.constants.signals[signal] when available and
otherwise fall back to a small internal mapping of common signals (e.g., SIGHUP,
SIGINT, SIGTERM, SIGQUIT, SIGKILL, SIGABRT) to their numbers; change the return
in signalExitCode to return 128 + signalNumber (defaulting to 1 only if lookup
fails). Ensure you update the signalExitCode function and reference
NodeJS.Signals for the lookup.
🪄 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: 18f7478b-34de-499a-be23-d47e87f0898e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/persona-kit/README.mdpackages/persona-kit/package.jsonpackages/persona-kit/src/config-files.tspackages/persona-kit/src/execute.test.tspackages/persona-kit/src/execute.tspackages/persona-kit/src/index.tspackages/persona-kit/src/mount.tspackages/persona-kit/src/plan.test.tspackages/persona-kit/src/plan.tspackages/persona-kit/src/sidecars.tspackages/persona-kit/src/skill-runner.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| async dispose(): Promise<void> { | ||
| if (disposed) return; | ||
| disposed = true; | ||
| handle.cleanup(); |
Contributor
There was a problem hiding this comment.
🔴 Missing await on potentially-async handle.cleanup() in mount dispose
At packages/persona-kit/src/mount.ts:85, handle.cleanup() is called without await inside an async dispose() method. If createMount from @relayfile/local-mount returns a handle whose cleanup() is async (returns a Promise), then dispose() resolves immediately before the mount teardown actually completes. This breaks the semantic contract of dispose(): callers of executePersonaSpawnPlan (packages/persona-kit/src/execute.ts:112) do await disposeAll(handles) and assume all side effects are reversed when it resolves. A caller that deletes the session directory or reuses the cwd immediately after await handle.dispose() could race against an incomplete mount cleanup.
Comparison with CLI code
The CLI at packages/cli/src/cli.ts:1526 also calls handle?.cleanup() without await, but that's in a process-exit teardown path where fire-and-forget is acceptable. The persona-kit wrapper wraps this in a composable dispose() that's explicitly awaited in orchestration sequences, so the missing await has more impact here.
Suggested change
| handle.cleanup(); | |
| await handle.cleanup(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
- Carry path-backed sidecars through the plan via `ResolvedSidecarWrite.sourcePath` so personas declaring `claudeMd` / `agentsMd` (file path, not inlined content) no longer drop their sidecar at runtime; `writePersonaSidecars` reads the source at write time, keeping the plan JSON-serializable. (codex P1) - Default `applyPersonaMount` `personaId` from `plan.persona.personaId` inside `executePersonaSpawnPlan` so callers don't have to forward it redundantly. (codex P2) - Make ambient `process.env` capture into `plan.env` opt-in via `PlanOptions.includeProcessEnv`; default is to populate `plan.env` only from persona/input/override sources, keeping ambient secrets out of the serialized plan. (coderabbit major) - Validate sidecar filenames as basenames before joining with `cwd`, so hand-built or JSON-deserialized plans cannot escape the cwd via `..` or absolute paths. (coderabbit major) - Constrain `runSkillInstalls` cleanup to paths inside `cwd` and reject any declared path that resolves outside the workspace. (coderabbit major) - Use `os.constants.signals[signal]` for `signalExitCode` so signaled exits produce a meaningful 128+N rather than always 129. (coderabbit minor) - Remove the unused `PlanOptions.cwd` field; cwd belongs to ExecuteOptions. Update README to reflect the PlanOptions vs ExecuteOptions split. (coderabbit minor) - Defensively `await` the relayfile mount handle's `cleanup()` call inside `applyPersonaMount`'s dispose so future async cleanup variants do not break the dispose contract executors rely on. (devin)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/persona-kit/src/execute.test.ts (1)
176-185: ⚡ Quick winThis test never exercises the install-command path.
installsis empty here, sorunSkillInstalls()only covers session-root scaffolding/disposal. A regression in actually spawninginstallCommandwould still pass. Please add one harmless install entry, or rename the test so the coverage matches what it validates.🤖 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-kit/src/execute.test.ts` around lines 176 - 185, The test "runSkillInstalls executes the install command and disposes session install root" never exercises the install-command path because the SkillMaterializationPlan's installs array is empty; update the test so runSkillInstalls actually spawns an install by adding a harmless install entry (e.g., an object with an installCommand like "echo installed" or "true" and any needed metadata) to the plan.installs array before calling runSkillInstalls, ensuring the installCommand path is exercised; alternatively, if you prefer not to run an install, rename the test to reflect it only validates sessionInstallRoot scaffolding/disposal.
🤖 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/persona-kit/src/plan.ts`:
- Around line 217-223: The code currently sets processEnv = options.processEnv
?? process.env which causes resolvePersonaInputs(...) to read host env even when
includeProcessEnv is false; change the logic so processEnv is only set to
options.processEnv or process.env when options.includeProcessEnv is truthy
(e.g., processEnv = options.includeProcessEnv ? (options.processEnv ??
process.env) : undefined) and pass that into resolvePersonaInputs (the call that
uses persona.inputs / persona.inputValues and resolvePersonaInputs) so
plan.inputs and plan.env are not populated from the ambient env unless
includeProcessEnv is explicitly enabled.
In `@packages/persona-kit/src/sidecars.ts`:
- Around line 49-50: The code reads sidecar.sourcePath directly via readFile,
but ResolvedSidecarWrite requires an absolute path; reject any relative paths to
avoid depending on process.cwd. In the function that performs the read (where
sidecar.sourcePath and readFile are used), validate using
path.isAbsolute(sidecar.sourcePath) and throw a clear error referencing
ResolvedSidecarWrite/sidecar.sourcePath if it is not absolute; only call
readFile when the path is confirmed absolute. Ensure the thrown error message
includes the offending path for debugging.
---
Nitpick comments:
In `@packages/persona-kit/src/execute.test.ts`:
- Around line 176-185: The test "runSkillInstalls executes the install command
and disposes session install root" never exercises the install-command path
because the SkillMaterializationPlan's installs array is empty; update the test
so runSkillInstalls actually spawns an install by adding a harmless install
entry (e.g., an object with an installCommand like "echo installed" or "true"
and any needed metadata) to the plan.installs array before calling
runSkillInstalls, ensuring the installCommand path is exercised; alternatively,
if you prefer not to run an install, rename the test to reflect it only
validates sessionInstallRoot scaffolding/disposal.
🪄 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: b5b8b74b-d17d-42d8-81b6-00ffa33528fb
📒 Files selected for processing (8)
packages/persona-kit/README.mdpackages/persona-kit/src/execute.test.tspackages/persona-kit/src/execute.tspackages/persona-kit/src/mount.tspackages/persona-kit/src/plan.test.tspackages/persona-kit/src/plan.tspackages/persona-kit/src/sidecars.tspackages/persona-kit/src/skill-runner.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/persona-kit/src/mount.ts
- packages/persona-kit/src/skill-runner.ts
- Gate ambient process.env capture during input fallback resolution too: resolvePersonaInputs now sees an empty env when includeProcessEnv is off, so plan.inputs and plan.env stay deterministic across hosts and don't leak ambient values via persona inputs that declare an env binding. - Reject relative ResolvedSidecarWrite.sourcePath in writePersonaSidecars so a hand-built or JSON-deserialized plan can't make sidecar resolution depend on the executor's process cwd. - Replace the misnamed runSkillInstalls test with two focused cases: one verifies session scaffolding/disposal, one actually exercises the spawn path against a harmless `sh -c true` install. Add a third case covering SkillInstallError on non-zero exits.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/persona-kit/src/execute.test.ts (1)
164-165: ⚡ Quick winMake spawn-path tests platform-agnostic.
These commands are POSIX-specific and can break on Windows runners. Use Node itself as the spawned command to keep coverage equivalent and deterministic across OSes.
Proposed diff
@@ - installCommand: ['true'], + installCommand: [process.execPath, '-e', 'process.exit(0)'], @@ - installCommand: ['sh', '-c', 'true'], + installCommand: [process.execPath, '-e', 'process.exit(0)'], @@ - installCommand: ['sh', '-c', 'exit 17'], + installCommand: [process.execPath, '-e', 'process.exit(17)'],Also applies to: 205-206, 228-229
🤖 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-kit/src/execute.test.ts` around lines 164 - 165, Tests currently spawn POSIX-only commands (e.g., installCommand: ['true']) which fail on Windows; update the test cases in execute.test.ts (the blocks referencing installCommand and installedDir, including the other occurrences at the same pattern on lines around 205-206 and 228-229) to spawn Node itself instead (use process.execPath as the command) and pass a short Node argument that deterministically succeeds/fails to mirror the original intent so tests become platform-agnostic while keeping installedDir assertions the same.
🤖 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-kit/src/execute.test.ts`:
- Around line 164-165: Tests currently spawn POSIX-only commands (e.g.,
installCommand: ['true']) which fail on Windows; update the test cases in
execute.test.ts (the blocks referencing installCommand and installedDir,
including the other occurrences at the same pattern on lines around 205-206 and
228-229) to spawn Node itself instead (use process.execPath as the command) and
pass a short Node argument that deterministically succeeds/fails to mirror the
original intent so tests become platform-agnostic while keeping installedDir
assertions the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e83eef9-c9d8-4afa-bbbb-7450555bec71
📒 Files selected for processing (3)
packages/persona-kit/src/execute.test.tspackages/persona-kit/src/plan.tspackages/persona-kit/src/sidecars.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/persona-kit/src/plan.ts
This was referenced May 10, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Implements [persona-kit 3/8] (#66) — the top-level orchestration API the workforce CLI and relay SDK will consume.
buildPersonaSpawnPlan— pure plan builder. ComposesbuildInteractiveSpec,materializeSkills,resolvePersonaInputs, and the sidecar/mount/input resolvers into a singlePersonaSpawnPlanvalue. No I/O, no subprocesses, JSON-serializable round-trip verified by tests.executePersonaSpawnPlan— side-effecting executor. RunsapplyPersonaMount→runSkillInstalls→writePersonaSidecars→materializePersonaConfigFilesin order with abort-on-failure. Returns anExecutionHandlewhosedispose()reverses every side effect in LIFO order (idempotent).{ dispose(): Promise<void> }shape so advanced callers can wire their own orchestration.Notes for reviewers
applyPersonaMount,runSkillInstalls,writePersonaSidecars,materializePersonaConfigFiles) were "already lifted in [persona-kit 2/8] Move harness-kit + workload-router persona code into persona-kit (no behavior change) #65" — they were not. [persona-kit 2/8] Move harness-kit + workload-router persona code into persona-kit (no behavior change) #65 only movedharness-kit+workload-routerpersona-shape code; the side-effecting helpers incli.ts(mount, sidecar writes, skill spawn loop) stayed put. This PR introduces minimal first-class versions of those helpers inside persona-kit. The CLI continues to call its existing copies (per [persona-kit 3/8] Compose top-level persona-kit orchestration API #66's "no callsite outside persona-kit needs to change"); the migration to the new API is 4/8.@relayfile/local-mount(added as apersona-kitdependency). If a workerd-style consumer needs the pure plan only, the/planvs/executesub-export split mentioned in [persona-kit 3/8] Compose top-level persona-kit orchestration API #66's tasks can be added in a follow-up — every plan-only path is already isolated inplan.ts.ResolvedPersonais exported as an alias of the existingPersonaSelectionso the orchestration API matches [persona-kit 3/8] Compose top-level persona-kit orchestration API #66's naming without a breaking rename.Files
packages/persona-kit/src/plan.ts— types +buildPersonaSpawnPlanpackages/persona-kit/src/execute.ts—executePersonaSpawnPlanpackages/persona-kit/src/mount.ts—applyPersonaMountpackages/persona-kit/src/sidecars.ts—writePersonaSidecarspackages/persona-kit/src/config-files.ts—materializePersonaConfigFilespackages/persona-kit/src/skill-runner.ts—runSkillInstallspackages/persona-kit/src/{plan,execute}.test.ts— round-trip + happy/failure pathspackages/persona-kit/README.md— end-to-end usageTest plan
pnpm --filter @agentworkforce/persona-kit test— 73 tests pass (8 new).pnpm -r build— every package builds.pnpm -r test— full monorepo test suite passes (no regressions).PersonaSpawnPlan.executePersonaSpawnPlanfailure path: a later step throwing causes prior handles to dispose and restores prior file contents.Closes #66.
Generated by Claude Code