feat(cli): deploy a persona by id, not just by path - #320
Conversation
`agentworkforce deploy proposal-agent` resolves through the registry cascade, so an agent kept in `.agentworkforce/workforce/agents/<name>/` deploys by name from anywhere in the repo instead of by spelling out its path. Syntax decides which a selector is: path separators, a leading `.`/`/`/`~`, or a persona-source extension mean a path; anything else is an id. The filesystem does not get a vote, so a bare `proposal-agent` cannot mean the persona in one directory and a same-named folder in another. A built-in id resolves but has no file to deploy, and says so rather than failing on a path that was never going to exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used the included review currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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)
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: 01f746a99a
ℹ️ 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".
|
|
||
| let resolved; | ||
| try { | ||
| resolved = resolvePersonaReference(selector); |
There was a problem hiding this comment.
When a valid handler-based deploy persona omits harness, model, or systemPrompt, this call fails before returning its file path because resolvePersonaReference unconditionally builds an interactive PersonaSelection, whose validator rejects those omissions. Deploy's persona parser explicitly permits these fields to be absent when onEvent is present, so the same persona deploys by path but cannot deploy by the newly supported bare ID; use a registry lookup that does not require the interactive projection.
Useful? React with 👍 / 👎.
| 'Pass a path to a persona.json or persona.ts instead.' | ||
| ); | ||
| } | ||
| return resolved.path; |
There was a problem hiding this comment.
When the selected ID is defined by a partial registry overlay, returning its declaring path discards the merged resolved.spec that made the persona valid. Deploy subsequently rereads the raw file in compileAgentSource; for example, the new test fixture containing only id and extends fails with missing top-level "intent", and other inherited deploy fields are similarly lost. The ID path needs to deploy a materialized merged spec, while preserving the declaring directory for relative handler and asset paths.
Useful? React with 👍 / 👎.
![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 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/deploy-command.ts">
<violation number="1" location="packages/cli/src/deploy-command.ts:342">
P1: Resolve ID selectors without constructing an interactive `PersonaSelection`; otherwise handler personas that omit `harness`, `model`, or `systemPrompt` fail before deploy can obtain their path.</violation>
<violation number="2" location="packages/cli/src/deploy-command.ts:355">
P1: When a higher-priority partial overlay shadows an agent persona, `resolved.path` points to the overlay instead of the merged agent file. Deploy then rereads only that overlay, loses inherited `cloud`/`onEvent`, and rejects the ID deployment before bundling; pass the merged registry result and the handler-owning path into deploy, or resolve the owning file before returning.</violation>
</file>
<file name="packages/cli/src/deploy-command.test.ts">
<violation number="1" location="packages/cli/src/deploy-command.test.ts:548">
P3: The built-in-id test is not isolated from ambient developer configuration, so it can fail (or false-pass) depending on the machine it runs on. `resolveDeployPersonaSelector('persona-maker')` resolves through the registry cascade, where a local persona named `persona-maker` under the runner's cwd (`process.cwd()`) or in the configurable persona dirs (default `~/.agentworkforce/workforce/personas`) wins over the built-in catalog and returns a real file path, so the `/no file to deploy/` assertion fails even though the behavior under test is correct. The other new selector test isolates this by chdir'ing into a fresh mkdtemp root; this one leaves cwd and the ambient config untouched. Run the assertion from an isolated temporary cwd (and remove it in finally) so only the built-in resolution drives the outcome.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 'Pass a path to a persona.json or persona.ts instead.' | ||
| ); | ||
| } | ||
| return resolved.path; |
There was a problem hiding this comment.
P1: When a higher-priority partial overlay shadows an agent persona, resolved.path points to the overlay instead of the merged agent file. Deploy then rereads only that overlay, loses inherited cloud/onEvent, and rejects the ID deployment before bundling; pass the merged registry result and the handler-owning path into deploy, or resolve the owning file before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/deploy-command.ts, line 355:
<comment>When a higher-priority partial overlay shadows an agent persona, `resolved.path` points to the overlay instead of the merged agent file. Deploy then rereads only that overlay, loses inherited `cloud`/`onEvent`, and rejects the ID deployment before bundling; pass the merged registry result and the handler-owning path into deploy, or resolve the owning file before returning.</comment>
<file context>
@@ -303,6 +312,49 @@ Flags:
+ 'Pass a path to a persona.json or persona.ts instead.'
+ );
+ }
+ return resolved.path;
+}
+
</file context>
|
|
||
| let resolved; | ||
| try { | ||
| resolved = resolvePersonaReference(selector); |
There was a problem hiding this comment.
P1: Resolve ID selectors without constructing an interactive PersonaSelection; otherwise handler personas that omit harness, model, or systemPrompt fail before deploy can obtain their path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/deploy-command.ts, line 342:
<comment>Resolve ID selectors without constructing an interactive `PersonaSelection`; otherwise handler personas that omit `harness`, `model`, or `systemPrompt` fail before deploy can obtain their path.</comment>
<file context>
@@ -303,6 +312,49 @@ Flags:
+
+ let resolved;
+ try {
+ resolved = resolvePersonaReference(selector);
+ } catch (err) {
+ if (err instanceof PersonaResolutionError) {
</file context>
| const trap = trapExit(); | ||
| try { | ||
| assert.throws( | ||
| () => resolveDeployPersonaSelector('persona-maker'), |
There was a problem hiding this comment.
P3: The built-in-id test is not isolated from ambient developer configuration, so it can fail (or false-pass) depending on the machine it runs on. resolveDeployPersonaSelector('persona-maker') resolves through the registry cascade, where a local persona named persona-maker under the runner's cwd (process.cwd()) or in the configurable persona dirs (default ~/.agentworkforce/workforce/personas) wins over the built-in catalog and returns a real file path, so the /no file to deploy/ assertion fails even though the behavior under test is correct. The other new selector test isolates this by chdir'ing into a fresh mkdtemp root; this one leaves cwd and the ambient config untouched. Run the assertion from an isolated temporary cwd (and remove it in finally) so only the built-in resolution drives the outcome.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/deploy-command.test.ts, line 548:
<comment>The built-in-id test is not isolated from ambient developer configuration, so it can fail (or false-pass) depending on the machine it runs on. `resolveDeployPersonaSelector('persona-maker')` resolves through the registry cascade, where a local persona named `persona-maker` under the runner's cwd (`process.cwd()`) or in the configurable persona dirs (default `~/.agentworkforce/workforce/personas`) wins over the built-in catalog and returns a real file path, so the `/no file to deploy/` assertion fails even though the behavior under test is correct. The other new selector test isolates this by chdir'ing into a fresh mkdtemp root; this one leaves cwd and the ambient config untouched. Run the assertion from an isolated temporary cwd (and remove it in finally) so only the built-in resolution drives the outcome.</comment>
<file context>
@@ -480,3 +482,74 @@ test('runLogin canonicalizes origin.agentrelay.cloud apiUrl before resolving the
+ const trap = trapExit();
+ try {
+ assert.throws(
+ () => resolveDeployPersonaSelector('persona-maker'),
+ /__exit_trap__/
+ );
</file context>
|
Both P1s are real. Reproduced each before changing anything, and I've converted this to draft. Interactive projection blocks handler personas. Built a handler persona from Same file, two selectors, opposite outcomes. The rejection is deeper than the Overlay shadowing loses the agent file. A partial overlay at
The fix is the one you both point at — hand deploy the materialized merged spec plus the directory that owns the handler, rather than a path — and it needs the registry to be able to hold handler personas in the first place. That is a larger change than this PR, and it touches ground #316 already shipped, so I would rather land it deliberately than patch it here. The P3 on test isolation is also correct; the built-in-id assertion reads ambient cwd and personal config. |
1 participant
agentworkforce deploy <persona-id>resolves through the registry cascade, so an agent kept in.agentworkforce/workforce/agents/<name>/deploys by name from anywhere in the repo. Completes the loop opened by #316, which made those agents discoverable tolist/show/agentbut leftdeploypath-only.Which selectors are paths
Syntax decides, never the filesystem. A path separator, a leading
.///~, or a persona-source extension means a path; anything else is an id. Probing disk instead would let a bareproposal-agentmean the persona in one directory and a same-named folder in another — the same command doing different things depending on where it ran.Handler resolution is unaffected:
onEventresolves against the persona file's directory, and a compiledpersona.jsonsits in the same agent directory asagent.ts, so an id-resolved deploy bundles exactly what a path-resolved one does.Errors
A built-in id resolves but has no file to deploy, and says so:
An unknown id lists what is available, from the registry:
Verification
Exercised against the real
../salescheckout: bare id, explicitpersona.tspath (unchanged), unknown id, and built-in id.deploy-commandandlocal-personassuites pass locally (74).cli.test.tsspawns subprocesses and is being OOM-killed on this machine, so it is left to CI.Semver: minor — new selector form, no change to existing path behavior.
🤖 Generated with Claude Code