Migrate workforce CLI to consume @agentworkforce/persona-kit#78
Conversation
Drops `@agentworkforce/harness-kit` from the CLI's dependency surface and rewires every persona-shape import (constants, types, parsers, sidecar resolver, env-ref/MCP/inputs helpers, harness detection, buildInteractiveSpec) to come from `@agentworkforce/persona-kit`. The CLI now imports only `personaCatalog`, `listBuiltInPersonas`, and `routingProfiles` from `@agentworkforce/workload-router`. - packages/cli/src/cli.ts: imports rewired to persona-kit. The `useSelection().install` shape used by the spawn flow is now built inline via persona-kit's pure helpers (`materializeSkills` + `buildInstallArtifacts` + `buildCleanupArtifacts`) so the CLI keeps its existing spinner-driven install/cleanup orchestration. `runPersonaImprover` no longer wraps `useRunnableSelection`; instead it composes `resolvePersonaInputs` + `buildNonInteractiveSpec` from persona-kit and spawns the improver subprocess directly with stderr capture and timeout enforcement. - packages/cli/src/local-personas.ts: persona-shape symbols imported from persona-kit; only `personaCatalog`/`listBuiltInPersonas` remain from workload-router. - packages/cli/package.json: drops the `@agentworkforce/harness-kit` workspace dependency. - packages/persona-kit: adds `buildNonInteractiveSpec` (and its `NonInteractiveSpec` type) next to `buildInteractiveSpec`. The harness-specific one-shot argv translation that previously lived in `harness-kit/runner.ts` now has a canonical home in persona-kit so the CLI can build non-interactive spawns without depending on harness-kit. Per issue #67, harness-kit stays in the monorepo (gets removed in 6/8); this PR just removes the CLI's dependency on it. https://claude.ai/code/session_01SV82aorUkAHjh3hS34kKzY
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CLI removes direct harness-kit selection/runnable usage and adopts persona-kit builders. Persona-kit adds NonInteractiveSpec and buildNonInteractiveSpec. CLI now builds install context via persona-kit helpers and spawns non-interactive harness runs with config materialization and timeout-aware stderr capture. ChangesHarness-Kit Migration and Non-Interactive Spec Support
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: 0ee1f8f853
ℹ️ 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".
| timeoutMs !== undefined | ||
| ? setTimeout(() => { | ||
| child.kill('SIGTERM'); | ||
| }, timeoutMs) |
There was a problem hiding this comment.
The new timeout path only sends SIGTERM and then waits for 'close', so runPersonaImprover can hang forever if the harness process traps or ignores SIGTERM (spinner never finishes, proposals flow is blocked). This is a regression from the previous useRunnableSelection path, which used spawnCapture with a SIGKILL fallback after a grace period (packages/harness-kit/src/runner.ts). Add a post-timeout force-kill (or equivalent bounded wait) so timeout enforcement is guaranteed.
Useful? React with 👍 / 👎.
If the improver harness traps or ignores SIGTERM, the timeout path would hang forever — the spinner never finishes and the post-session proposals flow is blocked. Mirror the prior spawnCapture behavior in harness-kit by following the SIGTERM with a 1s grace window before sending SIGKILL. https://claude.ai/code/session_01SV82aorUkAHjh3hS34kKzY
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }); | ||
| // SIGTERM first; if the harness traps or ignores it, escalate to | ||
| // SIGKILL after a 1s grace so the timeout is actually enforced | ||
| // (matches the previous spawnCapture behavior in harness-kit). | ||
| const timeout = | ||
| timeoutMs !== undefined |
There was a problem hiding this comment.
🔴 Missing SIGKILL escalation after timeout SIGTERM can hang the CLI indefinitely
The refactored runPersonaImprover subprocess spawn sends only SIGTERM on timeout, but never escalates to SIGKILL if the child process ignores SIGTERM. The old spawnCapture in packages/harness-kit/src/runner.ts:377-386 sent SIGTERM followed by a SIGKILL after a 1-second grace period (FORCE_KILL_GRACE_MS). Without this escalation, if the harness binary (e.g. claude --print) becomes stuck (unresponsive API call, kernel-level I/O wait) and doesn't exit on SIGTERM, the await new Promise(...) never resolves, hanging the CLI indefinitely. The old code had an explicit test for this scenario (useRunnableSelection force-kills a child that ignores timeout SIGTERM).
| }); | |
| // SIGTERM first; if the harness traps or ignores it, escalate to | |
| // SIGKILL after a 1s grace so the timeout is actually enforced | |
| // (matches the previous spawnCapture behavior in harness-kit). | |
| const timeout = | |
| timeoutMs !== undefined | |
| const timeout = | |
| timeoutMs !== undefined | |
| ? setTimeout(() => { | |
| child.kill('SIGTERM'); | |
| setTimeout(() => { | |
| if (!settled) child.kill('SIGKILL'); | |
| }, 1_000); | |
| }, timeoutMs) | |
| : undefined; | |
| let settled = false; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const captureResult = await new Promise<{ exitCode: number | null; stderr: string }>( | ||
| (resolveResult) => { | ||
| const child = spawn(spec.bin, [...spec.args], { | ||
| cwd, | ||
| env: childEnv, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| shell: false | ||
| }); | ||
| let stderrBuf = ''; | ||
| let forceKillTimeout: NodeJS.Timeout | undefined; | ||
| child.stdout?.setEncoding('utf8'); | ||
| child.stderr?.setEncoding('utf8'); | ||
| child.stderr?.on('data', (chunk: string) => { | ||
| stderrBuf += chunk; | ||
| }); | ||
| // SIGTERM first; if the harness traps or ignores it, escalate to | ||
| // SIGKILL after a 1s grace so the timeout is actually enforced | ||
| // (matches the previous spawnCapture behavior in harness-kit). | ||
| const timeout = | ||
| timeoutMs !== undefined | ||
| ? setTimeout(() => { | ||
| child.kill('SIGTERM'); | ||
| forceKillTimeout = setTimeout(() => { | ||
| if (!child.killed) child.kill('SIGKILL'); | ||
| }, 1000); | ||
| }, timeoutMs) | ||
| : undefined; | ||
| const clearTimers = () => { | ||
| if (timeout) clearTimeout(timeout); | ||
| if (forceKillTimeout) clearTimeout(forceKillTimeout); | ||
| }; | ||
| child.on('error', (err) => { | ||
| clearTimers(); | ||
| resolveResult({ exitCode: 1, stderr: `${stderrBuf}${err.message}\n` }); | ||
| }); | ||
| child.on('close', (code, signal) => { | ||
| clearTimers(); |
There was a problem hiding this comment.
🟡 Config files written to user's cwd not restored if spawn promise rejects
The restoreConfigWrites() call at packages/cli/src/cli.ts:3261 is outside any try/finally block. The old code in packages/harness-kit/src/runner.ts:228-229 used try/finally to guarantee config file restoration. If spawn() throws synchronously inside the promise constructor (e.g., due to invalid env values), the promise rejects, the await throws, and restoreConfigWrites() is never reached — leaving orphaned config files (e.g. opencode.json) in the user's working directory. While synchronous spawn throws are rare with these hardcoded options, the defensive pattern from the original code should be preserved.
Prompt for agents
In runPersonaImprover (packages/cli/src/cli.ts around line 3229), the config file restoration (restoreConfigWrites) is called after the await but not inside a try/finally. The original code in packages/harness-kit/src/runner.ts:193-229 used try/finally to guarantee restoration. Wrap the spawn+await+restore in a try/finally so that restoreConfigWrites() always runs, even if the promise unexpectedly rejects. The exit code check and error throw (currently at lines 3262-3265) should remain outside the finally block but still after restoreConfigWrites.
Was this helpful? React with 👍 or 👎 to provide feedback.
If `spawn()` throws synchronously inside the promise constructor (or the await rejects for any unexpected reason), the prior layout would skip `restoreConfigWrites()` and leave the persona-supplied `opencode.json` (or future configFile producers) sitting in the user's real working directory. Wrap the spawn-and-await in try/finally so restoration is guaranteed before the exit-code check rethrows. https://claude.ai/code/session_01SV82aorUkAHjh3hS34kKzY
2 participants
Closes #67 (the part of 4/8 that removes the CLI's hard dependency on
@agentworkforce/harness-kit).Summary
packages/cli/to come from@agentworkforce/persona-kit. The CLI now imports onlypersonaCatalog,listBuiltInPersonas, androutingProfilesfrom@agentworkforce/workload-router, per the issue's "keep the routing-profile lookup" constraint.@agentworkforce/harness-kitfrompackages/cli/package.json.grep '@agentworkforce/harness-kit' packages/cli/returns zero hits after this PR.useSelection().install(from workload-router) inside the spawn flow with an inline CLI helper that calls persona-kit's pure helpers (materializeSkills+buildInstallArtifacts+buildCleanupArtifacts). This keeps the CLI's spinner-driven install/cleanup UX intact while moving the persona-shape logic onto persona-kit.useRunnableSelection(from harness-kit) insiderunPersonaImproverwith a direct composition:resolvePersonaInputs+buildNonInteractiveSpec+spawn, with stderr capture, timeout enforcement, and config-file restore. Behavior is preserved (same task body, same input env-bindings, same failure-on-non-zero-exit semantics).buildNonInteractiveSpec(andNonInteractiveSpec) to persona-kit next tobuildInteractiveSpec. The harness-specific one-shot argv translation that previously lived inharness-kit/runner.tsnow has a canonical home in persona-kit so the CLI can build non-interactive spawns without depending on harness-kit.What this PR does not do
The issue describes a deeper rewrite that swaps the entire spawn flow over to
buildPersonaSpawnPlan+executePersonaSpawnPlan. The CLI's mount-branch UX (setup spinner, three-phase SIGINT handler, autosync abort/syncback with partial-sync messaging,configureGitForMount, sidecarextendmode that reads from the real cwd while writing into the mount cwd) is not covered by persona-kit's current executor. Migrating that would require either expandingapplyPersonaMount/executePersonaSpawnPlanwith a progress-and-lifecycle API, or losing CLI UX — both of which felt out of scope for "make the CLI a consumer of persona-kit's surface" and risky to land in one shot.Instead, this PR achieves the issue's explicit no-imports-from-harness-kit goal and keeps the door open for a follow-up that swaps the orchestration onto
executePersonaSpawnPlanonce the executor grows the hooks the CLI needs. The next sibling issue (5/8) is the natural place for that.Test plan
pnpm -r build— clean across all packages.pnpm --filter @agentworkforce/cli test— 147/147 pass.pnpm --filter @agentworkforce/persona-kit test— 82/82 pass.pnpm -r test— all workspace tests pass.grep '@agentworkforce/harness-kit' packages/cli/→ zero hits.https://claude.ai/code/session_01SV82aorUkAHjh3hS34kKzY
Generated by Claude Code