persona-kit: add parse/mount/sidecars/skills test suites (#70)#81
Merged
Conversation
Closes #70 in part. The existing tests in execute.test.ts, plan.test.ts and index.test.ts already cover the cross-harness round-trips and skill materialization paths called out in the issue; this commit fills in the four file-scoped suites the issue listed by name and were still missing. - parse.test.ts: schema validation for parsePersonaSpec and the helpers it delegates to (parseHarnessSettings, parseInputs, parsePermissions, parseMount, parseMcpServers, parseTags, parseSkills, etc.). Covers the malformed-tier field-path message, the deferred-source contract for parseSkills (typos do not throw at parse time), the env-var input-name rule, and the optional+default conflict. - mount.test.ts: applyPersonaMount no-op path (undefined policy) plus the two error paths for a declared policy missing mountDir or personaId. The relayfile-backed branch is exercised end-to-end by the workforce CLI smoke test (#67) and is intentionally not unit-tested here. - sidecars.test.ts: extend-vs-overwrite restore semantics, sourcePath absoluteness check, LIFO dispose, and a buildPersonaSpawnPlan -> writePersonaSidecars round-trip on a path-backed sidecar. - skills.test.ts: cleanupOnDispose true/false for both per-install cleanupPaths and the session-install-root path, plus the empty-plan no-op path. All 119 tests in @agentworkforce/persona-kit pass locally. https://claude.ai/code/session_01UPVSZTvpB4yq4CFWD36Uwy
📝 WalkthroughWalkthroughAdds four Node.js test suites for persona-kit covering parsing/validation, mount policy application, sidecar file writing, and skill installation, including happy paths, error cases, and dispose/cleanup behaviors. ChangesPersona-Kit Test Coverage
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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
🤖 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/persona-kit/src/sidecars.test.ts`:
- Around line 99-115: The test currently only checks both files are removed and
doesn't prove LIFO; change it to use the same filename with dependent writes so
wrong order is detectable: create an initial file with content 'original', call
writePersonaSidecars twice for the same filename (e.g. 'CLAUDE.md') with
contents 'first' then 'second' (mode: 'overwrite'), assert the file now contains
'second', then call handle.dispose() and assert the file content was restored to
'original' (use writePersonaSidecars and handle.dispose to locate where to
change the test).
🪄 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: ba04d4f6-78ea-4e7d-a1fa-adb8ef4acfdd
📒 Files selected for processing (4)
packages/persona-kit/src/mount.test.tspackages/persona-kit/src/parse.test.tspackages/persona-kit/src/sidecars.test.tspackages/persona-kit/src/skills.test.ts
The previous test wrote two different filenames and asserted both were removed on dispose — FIFO would have passed too. Switch to two writes to the same filename in one call, with the file pre-seeded as "original". Only LIFO dispose restores back through "first" to "original"; FIFO would leave the file at "first". Addresses CodeRabbit review feedback on PR #81. https://claude.ai/code/session_01UPVSZTvpB4yq4CFWD36Uwy
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/persona-kit/src/sidecars.test.ts (1)
133-133: ⚡ Quick winAvoid brittle positional assertion on sidecars.
Line 133 assumes a stable array order (
plan.sidecars[0]). Prefer asserting membership so the test remains stable if ordering changes but behavior is still correct.Suggested change
- assert.equal(plan.sidecars[0]?.sourcePath, sourcePath); + assert.equal( + plan.sidecars.some((s) => s.sourcePath === sourcePath && s.filename === 'CLAUDE.md'), + true + );🤖 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/persona-kit/src/sidecars.test.ts` at line 133, The test currently uses a brittle positional assertion assert.equal(plan.sidecars[0]?.sourcePath, sourcePath); — change it to assert membership instead: verify that plan.sidecars contains an entry whose sourcePath equals sourcePath (e.g., use Array.prototype.some or find on plan.sidecars to assert presence). Update the assertion in sidecars.test.ts to check that plan.sidecars.some(s => s.sourcePath === sourcePath) (or assert.ok(find) / assert.exists) so the test passes regardless of array ordering while still validating the expected sidecar exists.
🤖 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.
Nitpick comments:
In `@packages/persona-kit/src/sidecars.test.ts`:
- Line 133: The test currently uses a brittle positional assertion
assert.equal(plan.sidecars[0]?.sourcePath, sourcePath); — change it to assert
membership instead: verify that plan.sidecars contains an entry whose sourcePath
equals sourcePath (e.g., use Array.prototype.some or find on plan.sidecars to
assert presence). Update the assertion in sidecars.test.ts to check that
plan.sidecars.some(s => s.sourcePath === sourcePath) (or assert.ok(find) /
assert.exists) so the test passes regardless of array ordering while still
validating the expected sidecar exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9df68912-de0e-4acd-a478-1785ce3092e9
📒 Files selected for processing (1)
packages/persona-kit/src/sidecars.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Partial completion of #70. The issue lists eight test files. The code on
mainalready covers the cross-harness round-trips, env-binding precedence, JSON serialization, mount-policy threading, sidecar resolution, and the skill-install spawn / cleanup contract via the existingplan.test.ts,execute.test.ts,index.test.ts,inputs.test.ts,interactive-spec.test.ts, andenv-refs.test.ts. This PR adds the four file-scoped suites the issue called out by name that were still missing frompackages/persona-kit/src/.parse.test.ts— schema validation forparsePersonaSpecand the helpers it delegates to (parseHarnessSettings,parseInputs,parsePermissions,parseMount,parseMcpServers,parseTags,parseSkills, etc.). Includes the issue's specific contracts: malformedtiersthrows with a precise field path, malformedskills[i].sourcedoes not throw at parse time (deferred to plan time), input names must match[A-Z_][A-Z0-9_]*, andoptional: truecannot be combined withdefault.mount.test.ts—applyPersonaMountno-op path (undefined policy) plus the two error paths for a declared policy missingmountDirorpersonaId. The@relayfile/local-mount-backed branch is exercised end-to-end by the workforce CLI smoke test ([persona-kit 4/8] Migrate workforce CLI to use @agentworkforce/persona-kit #67) and is intentionally not unit-tested here, matching the issue's "mock in unit tests; real integration in CLI smoke" guidance.sidecars.test.ts— extend-vs-overwrite restore semantics,sourcePathabsoluteness check, LIFO dispose, and abuildPersonaSpawnPlan→writePersonaSidecarsround-trip on a path-backed sidecar.skills.test.ts—cleanupOnDisposetrue/false coverage for both per-installcleanupPathsand the session-install-root path, plus the empty-plan no-op path.What was deliberately not added
load.test.ts—persona-kitdoes not export aloadPersonascascade today; the cascade lives in the workforce CLI, which is out of scope for this package's test suite.plan.test.tsalready iterates['claude', 'codex', 'opencode']and asserts the per-harness behavior the snapshots would freeze. Adding redundant fixture JSON would inflate the suite without catching anything the explicit tests don't already.These can be addressed in a follow-up PR if the team wants the fixture-bound snapshots and property-test layer; the existing assertions cover the same surface area imperatively.
Test plan
pnpm --filter @agentworkforce/persona-kit test— 119/119 pass locallypnpm --filter @agentworkforce/persona-kit typecheckpnpm --filter @agentworkforce/persona-kit lintCloses #70 (in part — leaving the issue open is fine if the team wants the fixture/property-test layer separately).
https://claude.ai/code/session_01UPVSZTvpB4yq4CFWD36Uwy
Generated by Claude Code