feat(cli): add persona create mode#42
Conversation
📝 WalkthroughWalkthroughThis PR adds prompt-visible persona inputs, implements input resolution and placeholder rendering, extends persona specs and local config with inputs and defaultCreateTarget, introduces an interactive ChangesPersona Input System and Create Command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)packages/cli/src/local-personas.test.tsMicrosoft Presidio Analyzer failed to scan this file 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: 4b21bf9859
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/workload-router/src/index.test.ts (1)
200-211: ⚡ Quick winAdd a complementary
resolvePersonacoverage test for inputs propagation.The existing propagation tests follow a consistent two-test pattern — one for
resolvePersonaand one forresolvePersonaByTier(see lines 167–198 formcpServers/permissions). This new test only covers theresolvePersonaByTier(legacy override) path, leaving the standard routing resolution path for inputs untested.♻️ Suggested complementary test
+test('resolvePersona propagates persona input declarations', () => { + const selection = resolvePersona('persona-authoring'); + assert.ok(selection.inputs, 'inputs should be propagated through resolvePersona'); + assert.equal(selection.inputs.TARGET_DIR?.default, 'personas'); + assert.equal(selection.inputs.CREATE_MODE?.default, 'built-in'); + assert.match( + selection.runtime.systemPrompt, + /\$TARGET_DIR\/<id>\.json/ + ); +}); + test('resolvePersonaByTier propagates persona input declarations', () => {🤖 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/workload-router/src/index.test.ts` around lines 200 - 211, Add a complementary unit test for the standard routing path by creating a test named like "resolvePersona propagates persona input declarations" that mirrors the existing resolvePersonaByTier test: call resolvePersona('persona-authoring', 'best'), assert selection.inputs?.TARGET_DIR?.default === 'personas' and selection.inputs?.CREATE_MODE?.default === 'built-in', and assert selection.runtime.systemPrompt matches /\$TARGET_DIR\/<id>\.json/; place it alongside the resolvePersonaByTier test to ensure inputs propagation is covered for both resolvePersona and resolvePersonaByTier.packages/cli/src/local-personas.ts (1)
365-371: 💤 Low valueDuplicated
INPUT_NAME_REandassertInputNameacross packages.The
INPUT_NAME_REregex andassertInputNamefunction are duplicated in bothpackages/cli/src/local-personas.ts(lines 365-371) andpackages/workload-router/src/index.ts(lines 817-823). Consider extracting these into a shared utility in@agentworkforce/workload-routerand re-exporting them, since the router already defines the canonicalPersonaInputSpectype.🤖 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 365 - 371, Extract the duplicated INPUT_NAME_RE and assertInputName into a shared utility module in `@agentworkforce/workload-router` (e.g., export from a new utils file) and replace the local definitions in packages/cli/src/local-personas.ts with imports; update packages/workload-router/src/index.ts to export the canonical PersonaInputSpec and the new INPUT_NAME_RE and assertInputName so CLI can re-use them, ensuring all references to INPUT_NAME_RE and assertInputName are updated to the imported symbols.
🤖 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/local-personas.test.ts`:
- Around line 376-377: The assertions in local-personas.test.ts directly access
.default on possibly undefined entries (spec?.inputs?.TARGET_DIR.default and
spec?.inputs?.CREATE_MODE.default), which can raise a TypeError if the merge
failed; update both checks to use optional chaining on the property access
(e.g., spec?.inputs?.TARGET_DIR?.default and spec?.inputs?.CREATE_MODE?.default)
so failures produce AssertionErrors from assert.equal instead of uncaught
TypeErrors and make the test error messages meaningful; locate these in the test
assertions around the TARGET_DIR and CREATE_MODE checks.
---
Nitpick comments:
In `@packages/cli/src/local-personas.ts`:
- Around line 365-371: Extract the duplicated INPUT_NAME_RE and assertInputName
into a shared utility module in `@agentworkforce/workload-router` (e.g., export
from a new utils file) and replace the local definitions in
packages/cli/src/local-personas.ts with imports; update
packages/workload-router/src/index.ts to export the canonical PersonaInputSpec
and the new INPUT_NAME_RE and assertInputName so CLI can re-use them, ensuring
all references to INPUT_NAME_RE and assertInputName are updated to the imported
symbols.
In `@packages/workload-router/src/index.test.ts`:
- Around line 200-211: Add a complementary unit test for the standard routing
path by creating a test named like "resolvePersona propagates persona input
declarations" that mirrors the existing resolvePersonaByTier test: call
resolvePersona('persona-authoring', 'best'), assert
selection.inputs?.TARGET_DIR?.default === 'personas' and
selection.inputs?.CREATE_MODE?.default === 'built-in', and assert
selection.runtime.systemPrompt matches /\$TARGET_DIR\/<id>\.json/; place it
alongside the resolvePersonaByTier test to ensure inputs propagation is covered
for both resolvePersona and resolvePersonaByTier.
🪄 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: 00840d3a-d310-47ab-8cc2-b28e3516beae
⛔ Files ignored due to path filters (1)
packages/workload-router/src/generated/personas.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
README.mdpackages/agentworkforce/CHANGELOG.mdpackages/agentworkforce/README.mdpackages/cli/CHANGELOG.mdpackages/cli/README.mdpackages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/local-personas.test.tspackages/cli/src/local-personas.tspackages/harness-kit/CHANGELOG.mdpackages/harness-kit/README.mdpackages/harness-kit/src/index.tspackages/harness-kit/src/inputs.test.tspackages/harness-kit/src/inputs.tspackages/harness-kit/src/runner.test.tspackages/harness-kit/src/runner.tspackages/workload-router/CHANGELOG.mdpackages/workload-router/README.mdpackages/workload-router/src/index.test.tspackages/workload-router/src/index.tspersonas/persona-maker.json
| assert.equal(spec?.inputs?.TARGET_DIR.default, '/tmp/user-personas'); | ||
| assert.equal(spec?.inputs?.CREATE_MODE.default, 'local'); |
There was a problem hiding this comment.
Missing ?. before .default will throw TypeError on merge failure instead of an AssertionError.
If either input key is absent from the merged spec (e.g., the cascade merge has a bug), spec?.inputs?.TARGET_DIR evaluates to undefined and the bare .default dereference throws a TypeError rather than an AssertionError. The failure message becomes Cannot read properties of undefined (reading 'default') instead of a meaningful diff.
🛡️ Proposed fix
- assert.equal(spec?.inputs?.TARGET_DIR.default, '/tmp/user-personas');
- assert.equal(spec?.inputs?.CREATE_MODE.default, 'local');
+ assert.equal(spec?.inputs?.TARGET_DIR?.default, '/tmp/user-personas');
+ assert.equal(spec?.inputs?.CREATE_MODE?.default, 'local');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.equal(spec?.inputs?.TARGET_DIR.default, '/tmp/user-personas'); | |
| assert.equal(spec?.inputs?.CREATE_MODE.default, 'local'); | |
| assert.equal(spec?.inputs?.TARGET_DIR?.default, '/tmp/user-personas'); | |
| assert.equal(spec?.inputs?.CREATE_MODE?.default, 'local'); |
🤖 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.test.ts` around lines 376 - 377, The
assertions in local-personas.test.ts directly access .default on possibly
undefined entries (spec?.inputs?.TARGET_DIR.default and
spec?.inputs?.CREATE_MODE.default), which can raise a TypeError if the merge
failed; update both checks to use optional chaining on the property access
(e.g., spec?.inputs?.TARGET_DIR?.default and spec?.inputs?.CREATE_MODE?.default)
so failures produce AssertionErrors from assert.equal instead of uncaught
TypeErrors and make the test error messages meaningful; locate these in the test
assertions around the TARGET_DIR and CREATE_MODE checks.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/cli/src/local-personas.test.ts (1)
376-377:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
.defaultreads with optional chaining.On Line 376, Line 377, and Line 417, a missing merged input key will throw
TypeErrorbefore assertion output, which obscures failures.Suggested fix
- assert.equal(spec?.inputs?.TARGET_DIR.default, '/tmp/user-personas'); - assert.equal(spec?.inputs?.CREATE_MODE.default, 'local'); + assert.equal(spec?.inputs?.TARGET_DIR?.default, '/tmp/user-personas'); + assert.equal(spec?.inputs?.CREATE_MODE?.default, 'local'); ... - assert.equal(spec?.inputs?.TARGET_DIR.default, '/tmp/reviews'); + assert.equal(spec?.inputs?.TARGET_DIR?.default, '/tmp/reviews');Also applies to: 417-417
🤖 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.test.ts` around lines 376 - 377, The assertions read the .default property directly and may throw if an input key is missing; update the checks to safely guard the .default access (e.g., change uses of spec?.inputs?.TARGET_DIR.default and spec?.inputs?.CREATE_MODE.default and the similar occurrence around line 417 to use optional chaining on the property itself such as spec?.inputs?.TARGET_DIR?.default and spec?.inputs?.CREATE_MODE?.default) so a missing key yields a test assertion failure instead of a TypeError.
🤖 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.
Duplicate comments:
In `@packages/cli/src/local-personas.test.ts`:
- Around line 376-377: The assertions read the .default property directly and
may throw if an input key is missing; update the checks to safely guard the
.default access (e.g., change uses of spec?.inputs?.TARGET_DIR.default and
spec?.inputs?.CREATE_MODE.default and the similar occurrence around line 417 to
use optional chaining on the property itself such as
spec?.inputs?.TARGET_DIR?.default and spec?.inputs?.CREATE_MODE?.default) so a
missing key yields a test assertion failure instead of a TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54d7418c-ff5c-4f7c-826b-c6cac679deaa
📒 Files selected for processing (2)
packages/cli/src/local-personas.test.tspackages/cli/src/local-personas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/local-personas.ts
1 participant
Summary
Testing