fix(persona-kit): tags optional + free-form per cloud#553 schema#114
Conversation
parseTags rejected free-form strings (only accepted intent-enum values) AND
required the field to be present + non-empty. Both are wrong per the schema
in cloud#553 (`tags text[]` — denorm metadata, not a closed enum).
This broke the canonical reference persona (examples/notion-essay-pr) whose
tags are ['proactive', 'notion', 'github'] — none of which are in the intent
enum. First customer (proactive-agents) hit it deploying today.
- parseTags returns undefined when tags is missing/null/empty
- accepts any string[] with 1..64-char entries, deduped+sorted
- removes the closed-enum constraint
- widens PersonaSpec.tags to readonly string[] (optional)
- regenerates persona.schema.json with the new shape
(tags items: { type: string, minLength: 1, maxLength: 64 }; tags
dropped from `required`; PersonaTag enum definition removed)
- emit-schema.mjs gains a post-process for tags-items bounds matching
the existing tightenWorkspaceServiceAccountName pattern
- cli list/show + local-personas accept any string tag; --filter-tag is
free-form with built-in tags surfaced as a hint
- tests cover all the optional/free-form/validation paths
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates persona tags from a closed enum to optional free-form strings with trimming, deduplication, sorting, and 1–64 character validation; updates types, parser, JSON schema/emitter, local-personas merging, CLI list/show/picker behavior, and tests. ChangesFree-form persona tags migration
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant LocalPersonas
participant Parser
participant Schema
User->>CLI: run agentworkforce list --filter-tag " custom "
CLI->>CLI: parseListArgs() trim & reject empty/whitespace
CLI->>LocalPersonas: load/build persona specs
LocalPersonas->>Parser: parseTags(raw.tags)
Parser->>Parser: trim, validate (1–64), dedupe, sort
Parser-->>LocalPersonas: readonly string[] | undefined
LocalPersonas->>CLI: specs with tags? (omit when undefined)
CLI->>CLI: collectPersonaRows() uses spec.tags ?? []
CLI->>CLI: formatPersonaShow() shows tags or "(none)"
CLI-->>User: rendered persona list/show
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 parseOverride stores runtime null for tags when input is null, violating the declared type
When raw.tags is null, the guard at local-personas.ts:425 (raw.tags !== undefined && raw.tags !== null) skips validation, but line 492 stores tags: raw.tags as PersonaTag[] | undefined — casting null to PersonaTag[] | undefined. The LocalPersonaOverride interface declares tags?: PersonaTag[] (no null). While downstream usage happens to work (both ?? and truthiness checks treat null correctly), this type unsoundness is fragile — any future code that tests override.tags === undefined to detect "not provided" will incorrectly treat an explicit null override as "tags present" rather than "tags absent".
(Refers to line 492)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Addressed in 29dcdf6. parseOverride now omits the tags key entirely from its return value when no tags were provided (using a conditional spread, same pattern as harness/model/harnessSettings). Combined with the existing raw.tags === null guard at line 426, LocalPersonaOverride.tags is now sound at runtime: either a trimmed/deduped/sorted PersonaTag[], or the property is absent. Never null, never [], never an explicit-undefined key, so override.tags === undefined / "tags" in override both correctly detect "not provided".
| } | ||
| const tags = new Set<string>(); | ||
| for (const [idx, tag] of raw.tags.entries()) { | ||
| if (!PERSONA_TAGS.includes(tag as PersonaTag)) { | ||
| throw new Error( | ||
| `${context}.tags[${idx}] must be one of: ${PERSONA_TAGS.join(', ')}` | ||
| ); | ||
| if (typeof tag !== 'string' || !tag.trim()) { | ||
| throw new Error(`${context}.tags[${idx}] must be a non-empty string`); | ||
| } | ||
| const trimmed = tag.trim(); | ||
| if (trimmed.length > 64) { |
There was a problem hiding this comment.
🟡 local-personas.ts parseOverride stores untrimmed tags while parse.ts parseTags trims them
The parseOverride function in local-personas.ts:429-434 validates that each tag's trimmed form is non-empty and ≤64 chars, but stores the original untrimmed value at local-personas.ts:492 (tags: raw.tags as PersonaTag[]). Meanwhile, parseTags in parse.ts:102-111 trims each entry before storing it (and also dedupes and sorts). This means a local persona override declaring tags: [" documentation "] passes validation but stores the whitespace-padded string. Downstream, --filter-tag documentation at cli.ts:2174 (r.tags.includes(filterTag)) won't match " documentation ", and tags: ["review", "review"] won't be deduped, unlike the same input through parseTags.
| } | |
| const tags = new Set<string>(); | |
| for (const [idx, tag] of raw.tags.entries()) { | |
| if (!PERSONA_TAGS.includes(tag as PersonaTag)) { | |
| throw new Error( | |
| `${context}.tags[${idx}] must be one of: ${PERSONA_TAGS.join(', ')}` | |
| ); | |
| if (typeof tag !== 'string' || !tag.trim()) { | |
| throw new Error(`${context}.tags[${idx}] must be a non-empty string`); | |
| } | |
| const trimmed = tag.trim(); | |
| if (trimmed.length > 64) { | |
| const seen = new Set<string>(); | |
| for (const [idx, tag] of raw.tags.entries()) { | |
| if (typeof tag !== 'string' || !tag.trim()) { | |
| throw new Error(`${context}.tags[${idx}] must be a non-empty string`); | |
| } | |
| const trimmed = tag.trim(); | |
| if (trimmed.length > 64) { | |
| throw new Error(`${context}.tags[${idx}] must be ≤64 characters`); | |
| } | |
| seen.add(trimmed); | |
| } | |
| raw.tags = [...seen].sort(); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Addressed by 431c403, which lands trim + dedupe + sort + collapse-empty for parseOverride in packages/cli/src/local-personas.ts (lines 425-444). Verified the current code matches your suggested patch — tags is stored only when at least one trimmed entry survives, deduped via Set and sorted. Closing.
…parseOverride null path
- packages/deploy/src/modes/cloud.test.ts: route the PersonaSpec<->{cloud:{deployUrl}} casts through `unknown` at the construct site and at all three deployedUrl(...) call sites so TS no longer rejects the bidirectional conversion (CI typecheck blocker).
- packages/cli/src/local-personas.ts: omit `tags` from the parseOverride return object when no tags were provided, instead of returning `tags: undefined`. Combined with the existing `raw.tags === null` guard, this guarantees `LocalPersonaOverride.tags` is either a trimmed/deduped/sorted PersonaTag[] or absent — never null at runtime, never an empty array, never an explicit-undefined key. Addresses Devin BUG_0002.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/cli/src/local-personas.ts (1)
814-815:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse length checks when conditionally emitting
tags.Line 815 and Line 1024 currently use truthiness checks; empty arrays are truthy and can leak
tags: []into resolved specs. Use?.lengthto preserve omit-when-empty semantics consistently.Suggested patch
- ...(override.tags ? { tags: override.tags } : {}), + ...(override.tags?.length ? { tags: override.tags } : {}), @@ - ...(mergedTags ? { tags: mergedTags } : {}), + ...(mergedTags?.length ? { tags: mergedTags } : {}),Also applies to: 1020-1024
🤖 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 814 - 815, The current conditional spreads use truthiness checks on override.tags which will still include tags: [] for empty arrays; change those checks to use length checks (e.g., override.tags?.length ? { tags: override.tags } : {}) so tags is omitted when empty. Update every occurrence where override.tags is conditionally emitted (the spread expression that builds the resolved spec) to use ?.length instead of a plain truthiness check so empty arrays are not leaked into the resolved specs.
🤖 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.ts`:
- Around line 814-815: The current conditional spreads use truthiness checks on
override.tags which will still include tags: [] for empty arrays; change those
checks to use length checks (e.g., override.tags?.length ? { tags: override.tags
} : {}) so tags is omitted when empty. Update every occurrence where
override.tags is conditionally emitted (the spread expression that builds the
resolved spec) to use ?.length instead of a plain truthiness check so empty
arrays are not leaked into the resolved specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3114a675-4274-4573-b080-4fc39a5d327c
📒 Files selected for processing (2)
packages/cli/src/local-personas.tspackages/deploy/src/modes/cloud.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/deploy/src/modes/cloud.test.ts
2 participants
Summary
@agentworkforce/persona-kit'sparseTagshad two bugs that blocked the canonical reference persona (examples/notion-essay-pr) and the first customer (proactive-agents) from deploying today.Bug 1 — Required when it should be optional
Omitting
tags, or settingtags: null, threwpersona[<intent>].tags must be a non-empty array of tags. Per cloud#553 the column istags text[]— denormalized catalog metadata, not a required field.Bug 2 — Validated against the wrong enum
tagswas checked against the closedintentenum (planning | implementation | review | testing | debugging | documentation | release | discovery | analytics).intentis the closed enum;tagsis free-form for catalog filtering. The reference persona's["proactive", "notion", "github"]could never pass.Real-world repro (today, prod)
After this PR,
examples/notion-essay-pr/persona.jsonparses cleanly with its existing free-form tags (no fixture changes needed).Changes
parseTagsrewritten:undefined/null/[]→undefined(omitted)string[], each entry trimmed + non-empty + ≤64 charsPersonaSpec.tagswidened fromPersonaTag[](required closed enum) toreadonly string[](optional).PersonaTagtype widened tostringso existing imports keep compiling without forcing every consumer to migrate.schemas/persona.schema.jsonregenerated:tags.itemswidened from$ref: PersonaTagto{ type: 'string', minLength: 1, maxLength: 64 }tagsremoved fromrequired[]PersonaTagenum definition removedemit-schema.mjsgains a tags-items post-process mirroring the existingtightenWorkspaceServiceAccountNamepattern so the parser's 1..64-char bounds are reflected in the schema (the generator can't infer them from the TS type).list/show/--filter-tag/ persona-picker) handles optionalspec.tags, lets--filter-tag <any-string>through, and surfaces the built-in tag tuple as a hint instead of a hard constraint.local-personas.tsdrops the closed-enum check for overlay/standalone tags and no longer requirestagson standalone files.parseTags accepts an array of arbitrary string tagsparseTags returns undefined when tags is missingparseTags returns undefined when tags is nullparseTags returns undefined when tags is an empty arrayparseTags rejects non-string entriesparseTags rejects empty / whitespace-only stringsparseTags rejects entries over 64 charactersparseTags rejects non-array inputparsePersonaSpec omits tags from the spec when the field is missingpackages/deploy/src/modes/cloud.test.ts— fixture cast changed fromas PersonaSpectoas unknown as PersonaSpecbecause TS no longer narrows['documentation']to the (now-removed) closed enum.Untouched (by design)
examples/notion-essay-pr/persona.jsonkeepstags: ["proactive", "notion", "github"]— the canonical example now proves the fix.packages/personas-core/personas/*.jsonand all 6 fixtures inpackages/persona-kit/src/__fixtures__/personas/*.jsonstill parse unchanged — they happened to be using built-in-tuple values, which are still valid string tags..agentworkforce/notion-essay-pr/persona.jsonwithtags: ["documentation"]) was not reverted — that file belongs to the user.Test plan
corepack pnpm -F @agentworkforce/persona-kit typecheckgreencorepack pnpm -F @agentworkforce/persona-kit test→ 170 pass (was 162; net +8)corepack pnpm run checkgreen (lint + typecheck + tests across the workspace)node packages/persona-kit/scripts/emit-schema.mjsproduces a no-diff schema after the initial regenerationexamples/notion-essay-pr/persona.jsonsucceeds — returnstags: ['github', 'notion', 'proactive'](deduped + sorted)🤖 Generated with Claude Code