Uh oh!
There was an error while loading. Please reload this page.
feat(cli): add persona pack install command - #40
Conversation
📝 WalkthroughWalkthroughImplements a shadcn-style agentworkforce install flow: CLI subcommand to copy persona JSONs from npm packages or local paths into /.agentworkforce/workforce/personas/, with npm pack extraction, persona-id filtering, filename flattening, conflict detection, optional overwrite, temp cleanup, tests, and docs. Adds standalone local persona support via an optional intent field. ChangesPersona Install + Local Persona Standalone
Sequence DiagramsequenceDiagram
actor User
participant CLI as "agentworkforce CLI"
participant Parser as "parseInstallArgs"
participant Handler as "runPersonaInstall"
participant Installer as "installPersonas"
participant NPM as "npm (external)"
participant FS as "File System"
participant Loader as "loadLocalPersonas"
User->>CLI: agentworkforce install <pkg|path> --persona id --overwrite
CLI->>Parser: parseInstallArgs(args)
Parser-->>CLI: PersonaInstallArgs{source, personaIds, overwrite}
CLI->>Handler: runPersonaInstall(rest)
Handler->>Installer: installPersonas({source, personaIds, overwrite, cwd})
alt source is npm package
Installer->>NPM: npm pack <pkg> --json (temp dir)
NPM-->>Installer: <pkg>.tgz
Installer->>FS: Extract tarball -> /tmp/extracted/package/
else source is local path
Installer->>FS: Resolve/expand local path -> ./local-personas/
end
Installer->>FS: Read package.json (agentworkforce.personas?) FS-->>Installer: personaDir path
Installer->>FS: Scan personaDir for *.json
FS-->>Installer: List of persona files
Installer->>Installer: Parse each JSON, validate id, dedupe/filter by personaIds
Installer->>FS: Copy selected files to .agentworkforce/workforce/personas/ (flattened)
FS-->>Installer: Result {installed, conflicts}
Installer-->>Handler: PersonaInstallResult
Handler->>FS: Write stdout (installed summary)
Handler->>FS: Write stderr (conflicts)
Handler->>Loader: loadLocalPersonas(cwd)
Loader-->>Handler: Installed personas discoverable
Handler-->>User: Exit status (0 if no conflicts, non-zero if conflicts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:bb8ff10f01
ℹ️ 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".
| export function isLocalInstallSource(source: string): boolean { | ||
| return ( | ||
| source.startsWith('.') || | ||
| source.startsWith('/') || | ||
| source.startsWith('~/') || | ||
| source === '~' | ||
| ); |
There was a problem hiding this comment.
isLocalInstallSource only treats POSIX-style prefixes (., /, ~/, ~) as local paths, so valid Windows local paths like C:\\packs\\personas or ~\\personas are misclassified as npm specs. In that case installPersonas takes the npm branch and calls npm pack on a filesystem path, causing agentworkforce install <absolute-path> to fail on Windows even though absolute local paths are documented as supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8acf7e4: isLocalInstallSource() now treats Windows drive-letter paths (C:\\...) and UNC paths (\\\\server\\...) as local sources. The ~\\... home-path case was already fixed in 7223a8d and remains covered by tests.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/local-personas.ts (1)
282-315:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
intenton overlays instead of silently ignoring it.If a file sets both
extendsandintent, the standalone path never runs and the merged spec keepsbase.intent, so the authoredintentis dropped without any warning. Failing fast here would make that misconfiguration obvious.Suggested fix
if (raw.extends !== undefined && (typeof raw.extends !== 'string' || !raw.extends.trim())) { throw new Error(`${context}.extends must be a non-empty string if provided`); } + if (raw.intent !== undefined && raw.extends !== undefined) {+ throw new Error(+ `${context}.intent cannot be combined with .extends; omit extends for standalone personas`+ );+ } if (raw.intent !== undefined && !PERSONA_INTENTS.includes(raw.intent as PersonaIntent)) { throw new Error(`${context}.intent must be one of: ${PERSONA_INTENTS.join(', ')}`); }🤖 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/cli/src/local-personas.ts` around lines 282 - 315, The validator currently allows an overlay to specify raw.intent which then gets ignored when extends is present; add a failing check that throws when both raw.extends and raw.intent are provided (e.g., before the existing intent validation block), such as: if (raw.extends !== undefined && raw.intent !== undefined) throw new Error(`${context}.intent cannot be set on overlays that extend another persona`); update the validation near the raw.intent checks (referencing raw.intent, raw.extends, and PERSONA_INTENTS) so overlays must not declare intent and the error surfaces immediately.
🤖 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/cli/README.md`:
- Around line 76-78: Update the two fenced code blocks in the README that show
the install usage and the file mapping examples so they use the "text" fence
language instead of unlabeled fences; specifically replace the opening backticks
for the block containing "agentworkforce install <pkg|path> [--persona <id> ...]
[--overwrite]" and the block containing
"package/personas/nested/code-reviewer.json ->
.agentworkforce/workforce/personas/code-reviewer.json" with ```text (leave the
contents unchanged) to satisfy markdownlint.
In `@packages/cli/src/persona-install.ts`:
- Around line 69-86: The local install detection and expansion don't handle
Windows-style home paths (e.g. "~\\personas"); update isLocalInstallSource to
also treat strings starting with "~\\" as local, and update expandHomePath to
handle inputs starting with "~\\" by joining homedir() with input.slice(2) (same
as the "~/” case), keeping the existing "~" and "/"-prefixed logic; ensure
resolveLocalSource continues to call expandHomePath and use
isAbsolute/resolvePath as before.
In `@README.md`:
- Around line 140-146: Update the fenced code block that shows the directory
tree starting with "@acme/personas/" to include a language tag by changing the
opening backticks to "```text" so the block becomes a labeled plaintext block;
locate the block containing the lines "@acme/personas/", "├── package.json", and
"└── personas/" and add the "text" language specifier to the opening fence.
---
Outside diff comments:
In `@packages/cli/src/local-personas.ts`:
- Around line 282-315: The validator currently allows an overlay to specify
raw.intent which then gets ignored when extends is present; add a failing check
that throws when both raw.extends and raw.intent are provided (e.g., before the
existing intent validation block), such as: if (raw.extends !== undefined &&
raw.intent !== undefined) throw new Error(`${context}.intent cannot be set on
overlays that extend another persona`); update the validation near the
raw.intent checks (referencing raw.intent, raw.extends, and PERSONA_INTENTS) so
overlays must not declare intent and the error surfaces immediately.
🪄 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: 8de86882-c3c3-4769-8770-d90f90443dfc
📒 Files selected for processing (7)
README.mdpackages/cli/README.mdpackages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/local-personas.tspackages/cli/src/persona-install.test.tspackages/cli/src/persona-install.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli/src/persona-install.ts (1)
69-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat absolute Windows paths as local sources too.
This still routes valid Windows local paths like
C:\packs\fooor\\server\share\foointo the npm-package branch, because the detection only checks string prefixes. That makes local installs fail on Windows unless the path happens to start with.or~.Suggested fix
export function isLocalInstallSource(source: string): boolean { + const trimmed = source.trim(); return ( - source.startsWith('.') ||- source.startsWith('/') ||- source.startsWith('~/') ||- source.startsWith('~\\') ||- source === '~'+ trimmed.startsWith('.') ||+ trimmed === '~' ||+ trimmed.startsWith('~/') ||+ trimmed.startsWith('~\\') ||+ isAbsolute(trimmed) ); }🤖 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/cli/src/persona-install.ts` around lines 69 - 77, The isLocalInstallSource function incorrectly treats Windows absolute paths as non-local; update isLocalInstallSource to also return true for Windows drive-letter absolute paths (e.g., matches /^[A-Za-z]:[\\/]/) and UNC paths (e.g., starts with '\\\\'), in addition to the existing checks for '.', '/', '~' and '~/'; modify the function to test these regex/prefix conditions (drive-letter and UNC) and return true when any of them match.
🤖 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/cli/src/persona-install.ts`:
- Around line 331-343: The temp directory is created (tempDir =
mkdtempSync(...)) only inside the npm-resolution branch before the try, but
cleanup lives in the finally after try so exceptions from resolver or
assertDirectory leak the temp dir; move the temp-dir lifecycle so it is created
before entering the try/finally that performs cleanup: when
!isLocalInstallSource(source) call mkdtempSync and set tempDir, then call
resolver (options.resolveNpmPackage ?? defaultResolveNpmPackage) and
assertDirectory inside the try block so the finally can always remove tempDir;
reference tempDir, mkdtempSync,
resolver/resolveNpmPackage/defaultResolveNpmPackage, packageRoot and
assertDirectory to locate the changes.
- Around line 285-289: The spawnSync call that sets packed currently invokes
'npm' with shell: false, which fails on Windows because the npm shim is
'npm.cmd'; change the command argument in that spawnSync (the one assigning to
packed) to choose the correct executable like: const npmExe = process.platform
=== 'win32' ? 'npm.cmd' : 'npm'; then call spawnSync(npmExe, ['pack', spec,
'--pack-destination', packDir, '--json'], { encoding: 'utf8', shell: false });
alternatively use cross-spawn to abstract this; update the spawnSync invocation
that creates packed (and any nearby npm spawn calls) to use npmExe or
cross-spawn so Windows uses the npm shim.
---
Duplicate comments:
In `@packages/cli/src/persona-install.ts`:
- Around line 69-77: The isLocalInstallSource function incorrectly treats
Windows absolute paths as non-local; update isLocalInstallSource to also return
true for Windows drive-letter absolute paths (e.g., matches /^[A-Za-z]:[\\/]/)
and UNC paths (e.g., starts with '\\\\'), in addition to the existing checks for
'.', '/', '~' and '~/'; modify the function to test these regex/prefix
conditions (drive-letter and UNC) and return true when any of them match.
🪄 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: 54b1d3e1-7ca4-4955-8cb6-f64929447549
📒 Files selected for processing (2)
packages/cli/src/persona-install.test.tspackages/cli/src/persona-install.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/persona-install.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
willwashburn
commented
May 7, 2026
Review follow-up pushed in Fixed still-valid items:
Skipped: CodeRabbit docstring coverage warning is advisory and not enforced by repo checks; adding broad docstrings would be unrelated to the review findings. Validated with |
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/cli/src/persona-install.ts`:
- Around line 292-296: The npm pack invocation in persona-install.ts (the
spawnSync call that sets packed) is vulnerable because git+ specs can run
lifecycle scripts; add the '--ignore-scripts' flag to the array of arguments
passed to npmExecutable() in that spawnSync call so npm pack is executed with
scripts disabled, and keep the change scoped to the same spawnSync invocation
that produces the packed variable (consider documenting the remaining caveat
about old npm versions and .npmrc re-enabling scripts elsewhere if desired).
🪄 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: 506abb40-d5b6-4406-b83b-9fadd5dcce94
📒 Files selected for processing (6)
README.mdpackages/cli/README.mdpackages/cli/src/local-personas.test.tspackages/cli/src/local-personas.tspackages/cli/src/persona-install.test.tspackages/cli/src/persona-install.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/README.md
- packages/cli/src/persona-install.test.ts
Uh oh!
There was an error while loading. Please reload this page.
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/cli/src/persona-install.ts`:
- Around line 69-79: The isLocalInstallSource function currently misses Windows
root-relative paths like "\personas"; update isLocalInstallSource to treat any
single leading backslash as a local path by adding a check for
trimmed.startsWith('\\') (or a regex that matches a single leading backslash) so
those inputs are routed to resolveLocalSource as local installs (you can replace
the existing '\\\\' check with a broader '\\' check to still cover UNC paths).
🪄 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: f491083a-dcdf-411e-be18-f58cb49f0ca1
📒 Files selected for processing (1)
packages/cli/src/persona-install.ts
| export function isLocalInstallSource(source: string): boolean { | ||
| const trimmed = source.trim(); | ||
| return ( | ||
| trimmed.startsWith('.') || | ||
| trimmed.startsWith('/') || | ||
| trimmed.startsWith('~/') || | ||
| trimmed.startsWith('~\\') || | ||
| trimmed === '~' || | ||
| /^[A-Za-z]:[\\/]/.test(trimmed) || | ||
| trimmed.startsWith('\\\\') | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js path handling, is a Windows path like "\personas" considered absolute (e.g., path.win32.isAbsolute("\\personas"))?
💡 Result:
Yes, in Node.js, a Windows path like "\personas" (which is the string "\personas" due to JavaScript escaping) is considered absolute by path.win32.isAbsolute. This returns true because it starts with a path separator (), making it a drive-relative path from the root of the current drive, consistent with Windows PathIsRelative API behavior [1][2][3]. Official Node.js documentation shows path.win32.isAbsolute('\foo') effectively returns true, as single-backslash paths are treated as absolute (equivalent to forward slash '/' examples which also return true) [4][5][6][3]. Note that "\personas" (four characters, two backslashes) would be an incomplete UNC path but still absolute per the logic checking initial separators [4]. This behavior has remained unchanged through Node.js v26 [5].
Citations:
- 1:
path.win32.isAbsoluteconsiders relative paths absolute nodejs/node#56766 - 2: Should path.win32.isAbsolute be false for drive-relative paths? nodejs/node#2656
- 3: nodejs/node@1c40079
- 4: https://nodejs.org/docs/latest-v19.x/api/path.html
- 5: https://nodejs.org/api/path.html
- 6: https://r2.nodejs.org/docs/v21.0.0/api/path.html
🏁 Script executed:
cat -n packages/cli/src/persona-install.ts | head -150Repository: AgentWorkforce/workforce
Length of output: 5311
🏁 Script executed:
rg -n "isLocalInstallSource" packages/cli/src/persona-install.tsRepository: AgentWorkforce/workforce
Length of output: 179
🏁 Script executed:
cat -n packages/cli/src/persona-install.ts | sed -n '330,380p'Repository: AgentWorkforce/workforce
Length of output: 2105
Handle Windows root-relative paths (\foo) as local install sources.
isLocalInstallSource misses valid Windows root-relative absolute paths. On Windows, \personas should resolve as a local path via resolveLocalSource, but this check returns false and routes it to npm package resolution instead, causing an error.
Suggested fix
export function isLocalInstallSource(source: string): boolean {
const trimmed = source.trim();
return (
trimmed.startsWith('.') ||
trimmed.startsWith('/') ||
+ trimmed.startsWith('\\') ||
trimmed.startsWith('~/') ||
trimmed.startsWith('~\\') ||
trimmed === '~' ||
/^[A-Za-z]:[\\/]/.test(trimmed) ||
trimmed.startsWith('\\\\')
);
}🤖 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/cli/src/persona-install.ts` around lines 69 - 79, The
isLocalInstallSource function currently misses Windows root-relative paths like
"\personas"; update isLocalInstallSource to treat any single leading backslash
as a local path by adding a check for trimmed.startsWith('\\') (or a regex that
matches a single leading backslash) so those inputs are routed to
resolveLocalSource as local installs (you can replace the existing '\\\\' check
with a broader '\\' check to still cover UNC paths).
Uh oh!
There was an error while loading. Please reload this page.
Closes#36
Summary
agentworkforce installfor npm and local persona packs.agentworkforce/workforce/personas/with subset selection, flattened filenames, conflict reporting, and--overwriteTests
npm run check