Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions packages/cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ Commands:
--filter-harness <harness> only show this harness
(${HARNESS_VALUES.join(' | ')})
--filter-tag <tag> only show personas carrying this tag
(${PERSONA_TAGS.join(' | ')})
(free-form; common built-in tags: ${PERSONA_TAGS.join(' | ')})
--no-display-description hide the DESCRIPTION column
show <persona> Print the fully-resolved spec for a single persona,
including which cascade layer defined it (cwd, user,
Expand Down Expand Up @@ -2010,7 +2010,7 @@ interface PersonaListRow {
harness: string;
model: string;
intent: string;
tags: PersonaTag[];
tags: readonly PersonaTag[];
description: string;
}

Expand All @@ -2023,7 +2023,7 @@ function collectPersonaRows(): PersonaListRow[] {
harness: spec.harness,
model: spec.model,
intent: spec.intent,
tags: spec.tags,
tags: spec.tags ?? [],
description: spec.description
});
};
Expand Down Expand Up @@ -2146,10 +2146,11 @@ function parseListArgs(args: readonly string[]): {
filterHarness = v as Harness;
} else if (arg === '--filter-tag') {
const v = valueOf(i++, arg);
if (!(PERSONA_TAGS as readonly string[]).includes(v)) {
die(`list: invalid --filter-tag "${v}". Must be one of: ${PERSONA_TAGS.join(', ')}`);
const trimmed = v.trim();
if (!trimmed) {
die('list: --filter-tag requires a non-empty value.');
}
filterTag = v as PersonaTag;
filterTag = trimmed as PersonaTag;
} else if (arg === '--display-description') {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
display.description = true;
} else if (arg === '--no-display-description') {
Expand Down Expand Up @@ -2254,7 +2255,7 @@ function formatPersonaShow(spec: PersonaSpec, source: PersonaSource): string {
lines.push(`PERSONA ${spec.id}`);
lines.push(`SOURCE ${source}`);
lines.push(`INTENT ${spec.intent}`);
lines.push(`TAGS ${spec.tags.length ? spec.tags.join(', ') : '(none)'}`);
lines.push(`TAGS ${spec.tags && spec.tags.length ? spec.tags.join(', ') : '(none)'}`);
lines.push(`DESCRIPTION ${spec.description}`);

lines.push('');
Expand Down Expand Up @@ -3593,15 +3594,15 @@ export function buildPickCandidates(): PickCandidate[] {
byId.set(spec.id, {
id: spec.id,
intent: spec.intent,
tags: [...spec.tags],
tags: spec.tags ? [...spec.tags] : [],
description: spec.description
});
}
for (const [id, spec] of local.byId.entries()) {
byId.set(id, {
id,
intent: spec.intent,
tags: [...spec.tags],
tags: spec.tags ? [...spec.tags] : [],
description: spec.description
});
}
Expand Down
31 changes: 20 additions & 11 deletions packages/cli/src/local-personas.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
CODEX_APPROVAL_POLICIES,
CODEX_SANDBOX_MODES,
HARNESS_VALUES,
PERSONA_TAGS,
SIDECAR_MD_MODES,
type CodexApprovalPolicy,
type CodexSandboxMode,
Expand Down Expand Up @@ -423,16 +422,24 @@ function parseOverride(value: unknown, context: string): LocalPersonaOverride {
if (raw.description !== undefined && typeof raw.description !== 'string') {
throw new Error(`${context}.description must be a string if provided`);
}
if (raw.tags !== undefined) {
if (!Array.isArray(raw.tags) || raw.tags.length === 0) {
throw new Error(`${context}.tags must be a non-empty array of tags if provided`);
let normalizedTags: PersonaTag[] | undefined;
if (raw.tags !== undefined && raw.tags !== null) {
if (!Array.isArray(raw.tags)) {
throw new Error(`${context}.tags must be an array of strings if 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) {
Comment on lines 429 to +436
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Suggested change
}
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();
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new Error(`${context}.tags[${idx}] must be ≤64 characters`);
}
tags.add(trimmed);
}
if (tags.size > 0) {
normalizedTags = Array.from(tags).sort() as PersonaTag[];
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Expand Down Expand Up @@ -489,7 +496,7 @@ function parseOverride(value: unknown, context: string): LocalPersonaOverride {
id: raw.id,
extends: raw.extends as string | undefined,
intent: raw.intent as string | undefined,
tags: raw.tags as PersonaTag[] | undefined,
...(normalizedTags !== undefined ? { tags: normalizedTags } : {}),
description: raw.description as string | undefined,
skills: raw.skills as PersonaSpec['skills'] | undefined,
inputs,
Expand Down Expand Up @@ -804,7 +811,8 @@ function standaloneSpecFromOverride(
return {
id: override.id,
intent: override.intent,
tags: requireStandaloneField(override.tags, `${context}.tags`),
// Tags are optional per cloud#553 (denormalized catalog metadata).
...(override.tags ? { tags: override.tags } : {}),
description: requireStandaloneField(override.description, `${context}.description`),
skills: override.skills ?? [],
...(inputs ? { inputs } : {}),
Expand Down Expand Up @@ -1009,10 +1017,11 @@ function mergeOverride(
const claudeMdMode = override.claudeMdMode ?? base.claudeMdMode;
const agentsMdMode = override.agentsMdMode ?? base.agentsMdMode;

const mergedTags = override.tags ?? base.tags;
return {
id: override.id,
intent: base.intent,
tags: override.tags ?? base.tags,
...(mergedTags ? { tags: mergedTags } : {}),
description: override.description ?? base.description,
skills: override.skills ?? base.skills,
...(inputs ? { inputs } : {}),
Expand Down
16 changes: 9 additions & 7 deletions packages/deploy/src/modes/cloud.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ const ENV_KEYS = [
'WORKFORCE_DEPLOY_RETRY_BACKOFF_MS'
] as const;

function persona(overrides: Record<string, unknown> = {}): PersonaSpec {
function persona(overrides: Partial<PersonaSpec> = {}): PersonaSpec {
return {
id: 'demo',
intent: 'documentation',
tags: ['documentation'],
tags: ['documentation'] as const,
description: 'test persona',
skills: [],
harness: 'codex',
model: 'openai-codex/test',
systemPrompt: 'help',
Expand All @@ -45,7 +46,7 @@ function persona(overrides: Record<string, unknown> = {}): PersonaSpec {
schedules: [{ name: 'daily', cron: '0 9 * * *' }],
onEvent: './agent.ts',
...overrides
} as PersonaSpec;
};
}

async function withBundle(): Promise<{ bundle: BundleResult; cleanup: () => Promise<void> }> {
Expand Down Expand Up @@ -200,20 +201,21 @@ test('cloud URL precedence is flag env, cloud env, persona deployUrl, then defau
return calls.find((call) => call.url.endsWith('/deployments'))?.url;
}

const personaWithUrl = persona({ cloud: { deployUrl: 'https://persona.example.test/' } as unknown });
const personaWithUrl = persona() as unknown as Omit<PersonaSpec, 'cloud'> & { cloud: { deployUrl: string } };
personaWithUrl.cloud = { deployUrl: 'https://persona.example.test/' };
assert.equal(
await deployedUrl({
WORKFORCE_DEPLOY_CLOUD_URL: 'https://flag.example.test/',
WORKFORCE_CLOUD_URL: 'https://env.example.test/'
}, personaWithUrl),
}, personaWithUrl as unknown as PersonaSpec),
'https://flag.example.test/api/v1/workspaces/ws-test/deployments'
);
assert.equal(
await deployedUrl({ WORKFORCE_CLOUD_URL: 'https://env.example.test/' }, personaWithUrl),
await deployedUrl({ WORKFORCE_CLOUD_URL: 'https://env.example.test/' }, personaWithUrl as unknown as PersonaSpec),
'https://env.example.test/api/v1/workspaces/ws-test/deployments'
);
assert.equal(
await deployedUrl({}, personaWithUrl),
await deployedUrl({}, personaWithUrl as unknown as PersonaSpec),
'https://persona.example.test/api/v1/workspaces/ws-test/deployments'
);
assert.equal(
Expand Down
21 changes: 4 additions & 17 deletions packages/persona-kit/schemas/persona.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
"tags": {
"type": "array",
"items": {
"$ref": "#/definitions/PersonaTag"
"type": "string",
"minLength": 1,
"maxLength": 64
},
"description": "Free-form classification labels (from {@link PERSONA_TAGS } ). Every persona has at least one; a persona may carry multiple tags when it spans concerns (e.g. `['testing', 'implementation']`)."
"description": "Free-form catalog labels mirroring `tags text[]` in cloud#553. Tags are denormalized metadata for catalog filtering — they are NOT a closed enum and do NOT overlap with {@link intent } . Authors are free to label personas with provider names, project codes, or workflow shapes (e.g. `['proactive', 'notion', 'github']`). Optional; omitted, `null`, and empty-array values are treated identically."
},
"description": {
"type": "string"
Expand Down Expand Up @@ -130,7 +132,6 @@
"required": [
"id",
"intent",
"tags",
"description",
"skills",
"harness",
Expand Down Expand Up @@ -160,20 +161,6 @@
}
]
},
"PersonaTag": {
"type": "string",
"enum": [
"planning",
"implementation",
"review",
"testing",
"debugging",
"documentation",
"release",
"discovery",
"analytics"
]
},
"PersonaSkill": {
"type": "object",
"properties": {
Expand Down
20 changes: 20 additions & 0 deletions packages/persona-kit/scripts/emit-schema.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,26 @@ function tightenWorkspaceServiceAccountName(node) {
}
tightenWorkspaceServiceAccountName(schema);

// Post-process: tighten the persona `tags[]` items to match parseTags
// (non-empty, ≤64 chars). The TS type widens to `readonly string[]` after
// cloud#553 — the generator emits a bare `{ "type": "string" }` because
// the bounds live in the parser. Without this, the schema accepts empty
// or 1MB tag strings that the parser would then reject. Match the
// `tightenWorkspaceServiceAccountName` post-process pattern above.
const PERSONA_TAG_MAX_LEN = 64;
const personaSpecForTags = schema.definitions?.PersonaSpec;
if (
personaSpecForTags &&
personaSpecForTags.properties?.tags?.type === 'array' &&
personaSpecForTags.properties.tags.items?.type === 'string'
) {
personaSpecForTags.properties.tags.items = {
...personaSpecForTags.properties.tags.items,
minLength: 1,
maxLength: PERSONA_TAG_MAX_LEN
};
}

const serialized = `${JSON.stringify(schema, null, 2)}\n`;
await mkdir(dirname(schemaPath), { recursive: true });

Expand Down
5 changes: 5 additions & 0 deletions packages/persona-kit/src/constants.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import type { Harness, HarnessSkillTarget } from './types.js';

export const HARNESS_VALUES = ['opencode', 'codex', 'claude'] as const;
/**
* Suggested persona tags used by built-in personas. Tags are free-form
* (`tags text[]` in cloud#553) — this tuple is kept only as a UX hint for
* built-in catalog filtering. New personas may use any string.
*/
export const PERSONA_TAGS = [
'planning',
'implementation',
Expand Down
67 changes: 64 additions & 3 deletions packages/persona-kit/src/parse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,70 @@ test('parseHarnessSettings rejects non-boolean dangerouslyBypassApprovalsAndSand
);
});

test('parseTags rejects empty arrays and unknown tags', () => {
assert.throws(() => parseTags([], 'tags'), /must be a non-empty array/);
assert.throws(() => parseTags(['nonsense-tag'], 'tags'), /tags\[0\] must be one of:/);
test('parseTags accepts an array of arbitrary string tags', () => {
// Free-form per cloud#553 `tags text[]` — no closed-enum check.
// Output is deduped and sorted for stable serialization.
assert.deepEqual(
parseTags(['proactive', 'notion', 'github'], 'tags'),
['github', 'notion', 'proactive']
);
});

test('parseTags returns undefined when tags is missing', () => {
assert.equal(parseTags(undefined, 'tags'), undefined);
});

test('parseTags returns undefined when tags is null', () => {
assert.equal(parseTags(null, 'tags'), undefined);
});

test('parseTags returns undefined when tags is an empty array', () => {
// Empty array and "no tags" are semantically identical — both collapse
// to undefined so the field stays omitted from the parsed spec.
assert.equal(parseTags([], 'tags'), undefined);
});

test('parseTags rejects non-string entries', () => {
assert.throws(
() => parseTags(['proactive', 123], 'tags'),
/tags\[1\] must be a string/
);
});

test('parseTags rejects empty / whitespace-only strings', () => {
assert.throws(
() => parseTags(['proactive', ' '], 'tags'),
/tags\[1\] must be a non-empty string/
);
assert.throws(
() => parseTags([''], 'tags'),
/tags\[0\] must be a non-empty string/
);
});

test('parseTags rejects entries over 64 characters', () => {
const tooLong = 'x'.repeat(65);
assert.throws(
() => parseTags(['proactive', tooLong], 'tags'),
/tags\[1\] must be ≤64 characters/
);
});

test('parseTags rejects non-array input', () => {
assert.throws(
() => parseTags('proactive', 'tags'),
/tags must be an array of strings if provided/
);
});

test('parsePersonaSpec omits tags from the spec when the field is missing', () => {
// Tags are optional; the parsed spec should not synthesize an empty array
// when the input omits the field — the schema treats absence and `[]`
// identically per cloud#553's denormalized `tags text[]`.
const raw = { ...validSpec() } as Record<string, unknown>;
delete raw.tags;
const spec = parsePersonaSpec(raw, 'documentation');
assert.equal(spec.tags, undefined);
});

test('parseSkills returns [] for undefined, validates shape per entry', () => {
Expand Down
Loading
Loading