feat(persona-registry): warn when an agent's persona.json is stale - #321
feat(persona-registry): warn when an agent's persona.json is stale#321willwashburn wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughNested persona loading now compares authored source and compiled artifact modification times. Stale compiled personas remain available with a warning. Missing compiled personas remain skipped. CLI tests cover stale and current artifacts. ChangesPersona mtime loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds a warning for stale persona artifacts while preserving existing loading behavior; no actionable merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946f52af9e
ℹ️ 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".
| const authored = NESTED_PERSONA_SOURCE_FILENAMES.map((file) => join(sourceDir, file)).find( | ||
| (candidate) => fileMtimeMs(candidate) !== undefined | ||
| ); |
There was a problem hiding this comment.
Inspect every authoring candidate for staleness
When an agent directory contains multiple supported authoring files—for example, an old persona.ts remains after development moves to persona.js—this fixed-order find() always selects the TypeScript file. If persona.json is newer than that old file but older than the actively edited JavaScript file, the stale persona loads without any warning. Compare all recognized sources and use the newest modification time (and its path) for the diagnostic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
An agent driven by its `onEvent` entry has no interactive launch to configure, so `harness`, `model`, and `systemPrompt` — already optional on `PersonaSpec` — are optional here too. Requiring them kept exactly the agents the `agents/` directory was added for out of the registry, reporting a valid deployable persona as malformed. `onEvent` and `cloud` now survive parse and merge. An overlay that tweaks env no longer strips the handler entry that makes its base deployable, so the merged spec is a complete agent rather than a partial one. `harnessSettings` stays required: `PersonaSpec` types it non-optional, and `reasoning`/`timeoutSeconds` have no defensible default to invent on a persona's behalf. `onEvent` is validated as a relative path that cannot escape the agent directory, matching the sidecar rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Hand-rolling the guard drifted from persona-kit twice over. It validated the raw string and stored a trimmed copy, so `" ../x/agent.ts "` cleared the `..` check as the segment `" .."` and escaped the agent directory once trimmed. And it never checked the handler extension, so `onEvent: "README.md"` counted as a handler and skipped the interactive fields the persona never declared. `parseOnEvent` owns both rules and returns the exact string it validated, so the stored value cannot differ from the one that passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d77be7d to
7303cc5
Compare
|
Rebased onto #323 and retargeted there, so this shows only its own diff and merges without a conflict. Merge order: #318 → #323 → #321 → #322. GitHub retargets each to Worth knowing why: left on Full stack verified: 65 tests in |
Order matters in both directions. Validating the raw string and storing a trimmed copy let `" ../x/agent.ts "` clear the `..` check as the segment `" .."` and escape once trimmed. Validating without trimming stored `" ./agent.ts"`, which passes every check and then resolves against a directory named `" ."` at deploy. Trimming before `parseOnEvent` makes the validated value and the stored value the same string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An agent directory whose `persona.json` is older than its `persona.ts` loads the compiled spec with none of the edits sitting in the authoring file, and nothing about the result looks wrong. #316 warned about a persona that was never compiled; this covers the quieter case where it was compiled once. The persona is still served. Dropping it would turn a forgotten compile into a missing persona, which is a worse failure than an out-of-date one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… file A directory that still carries an abandoned `persona.ts` after development moved to `persona.js` was measured against the file nobody edits, so a `persona.json` newer than the dead source but older than the live one read as fresh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7303cc5 to
3425fa1
Compare
An agent directory whose
persona.jsonis older than itspersona.tsloads the compiled spec with none of the edits sitting in the authoring file. Nothing about the result looks wrong — the persona appears inlist, resolves inagent, deploys — it is simply the previous version.#316 warned about a persona that was never compiled. This covers the quieter case: one that was compiled once and has drifted.
The persona is still served. Dropping it would turn a forgotten compile into a missing persona — a worse failure than an out-of-date one, and one that would break
deployandagentrather than just informing them.Verification
Exercised against the real
../salescheckout: warns after touchingpersona.ts, clears afterpersona compile. Two new tests backdate the artifact withutimesSyncrather than sleeping, covering both orderings.local-personassuite passes (53).Semver: patch — a new diagnostic on an existing path, no resolution behavior changes.
🤖 Generated with Claude Code