fix(adapters): project native skills and harden spawn state#95
fix(adapters): project native skills and harden spawn state#95
Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds native skill directory support for Droid and PI CLI adapters, implements spawn state lifecycle management with failure cleanup, and ensures agent state consistency through pre-RPC reconciliation. Package version is bumped to 0.3.6 to match the release tag. ChangesNative Skills and Spawn Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/spawn.test.ts (1)
112-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTwo existing tests assert the
spawningstate (call index[0]) instead of the final state (call index[1])After this PR,
setAgentis called twice per spawn:
calls[0]— initial registration withstatus: 'spawning'calls[1]— final update withstatus: undefinedThe first test (
uses matching preset...) was correctly updated to usecalls[1](line 105). The other two still referencecalls[0]. Tests pass becausemodelis the same in both calls, but any future assertion onstatusorstatusAtagainst these indices will silently target the wrong state.🛠️ Proposed fix
// "uses explicit --preset over name when both match" - const [, agentState] = mockSetAgent.mock.calls[0] as [string, Record<string, unknown>] + const [, agentState] = mockSetAgent.mock.calls[1] as [string, Record<string, unknown>] expect(agentState.model).toBe('sonnet') // "uses default behavior when no preset matches name" - const [, agentState] = mockSetAgent.mock.calls[0] as [string, Record<string, unknown>] + const [, agentState] = mockSetAgent.mock.calls[1] as [string, Record<string, unknown>] expect(agentState.model).toBe('haiku')🤖 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 `@tests/unit/spawn.test.ts` around lines 112 - 139, The tests 'uses default behavior when no preset matches name' and the other remaining test still assert the agent final state using mockSetAgent.mock.calls[0]; update them to assert the final agent state using mockSetAgent.mock.calls[1] (the second call) so they inspect the post-spawn update rather than the initial spawning registration; locate the assertions that extract [, agentState] from mockSetAgent.mock.calls and change the index to 1 for those tests (the same pattern used in the already-updated 'uses matching preset...' test) to ensure future assertions on status/statusAt target the final state.
🧹 Nitpick comments (1)
src/skills.ts (1)
303-316: ⚡ Quick win
cleanupSkillsshould gate instruction-file cleanup on!adapter.skillDirto matchprojectSkillsrouting
projectSkillsnow routes exclusively based onadapter.skillDir/adapter-name, butcleanupSkillsstill uses only hardcoded adapter-name checks. Adapters withskillDirset (droid, pi) currently trigger the instruction-file cleanup path—they just find no markers and return early. As new adapters withskillDirare added, the intent is obscured and the file read is wasted.♻️ Proposed fix
- if (cliName !== 'claude-code' && cliName !== 'opencode' && adapter.instructionFile) { + if (!adapter.skillDir && cliName !== 'claude-code' && cliName !== 'opencode' && adapter.instructionFile) {🤖 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 `@src/skills.ts` around lines 303 - 316, The cleanup logic currently runs for adapters based only on adapter.name and adapter.instructionFile; change it to also require that adapter.skillDir is falsy (i.e., only run when !adapter.skillDir) so it matches projectSkills' routing; locate the block using cliName/adapter.instructionFile and SKILLS_MARKER_START/SKILLS_MARKER_END and wrap or extend the condition (cliName !== 'claude-code' && cliName !== 'opencode' && adapter.instructionFile) to also check !adapter.skillDir before reading/writing the instruction file and performing the regex replace.
🤖 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 `@src/tui/app.ts`:
- Around line 1030-1037: This block assumes the controller has flushed agent
state to disk before returning so poll() (which calls allAgents()) will see the
new agent; add a concise clarifying comment above this .then(...) explaining the
implicit ordering dependency: that poll() reads state.json synchronously via
allAgents(), that reconcileAgents() (in the controller) must finish
writing/flush state before the RPC response, and that if that invariant is
relaxed the "no agent state" branch may fire spuriously; reference the
functions/fields poll(), allAgents(), reconcileAgents(), this.state.agents, and
setBanner in the comment so future maintainers know the race condition and where
to fix it.
---
Outside diff comments:
In `@tests/unit/spawn.test.ts`:
- Around line 112-139: The tests 'uses default behavior when no preset matches
name' and the other remaining test still assert the agent final state using
mockSetAgent.mock.calls[0]; update them to assert the final agent state using
mockSetAgent.mock.calls[1] (the second call) so they inspect the post-spawn
update rather than the initial spawning registration; locate the assertions that
extract [, agentState] from mockSetAgent.mock.calls and change the index to 1
for those tests (the same pattern used in the already-updated 'uses matching
preset...' test) to ensure future assertions on status/statusAt target the final
state.
---
Nitpick comments:
In `@src/skills.ts`:
- Around line 303-316: The cleanup logic currently runs for adapters based only
on adapter.name and adapter.instructionFile; change it to also require that
adapter.skillDir is falsy (i.e., only run when !adapter.skillDir) so it matches
projectSkills' routing; locate the block using cliName/adapter.instructionFile
and SKILLS_MARKER_START/SKILLS_MARKER_END and wrap or extend the condition
(cliName !== 'claude-code' && cliName !== 'opencode' && adapter.instructionFile)
to also check !adapter.skillDir before reading/writing the instruction file and
performing the regex replace.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: df0ed9ec-c46d-41a7-a8a5-ff84772673d3
📒 Files selected for processing (16)
package.jsonsrc/adapters/droid.tssrc/adapters/pi.tssrc/adapters/types.tssrc/commands/spawn.tssrc/controller/server.tssrc/instructions.tssrc/skills.tssrc/tui/app.tstemplates/system-block-root.mdtemplates/system-block-subagent.mdtemplates/workflow-block.mdtests/unit/instructions.test.tstests/unit/skills.test.tstests/unit/spawn-bootstrap-uses-deliver.test.tstests/unit/spawn.test.ts
| .then(() => { | ||
| clearTimeout(staleTimer) | ||
| this.setBanner(`Spawned ${name}`, 'green', 3000) | ||
| this.poll() | ||
| if (this.state.agents.some(agent => agent.name === name)) { | ||
| this.setBanner(`Spawned ${name}`, 'green', 3000) | ||
| } else { | ||
| this.setBanner(`Spawn ${name} returned but no agent state was found`, 'red', 8000) | ||
| } |
There was a problem hiding this comment.
Implicit ordering assumption between controller state flush and TUI poll() read.
The "no agent state" banner fires when this.state.agents doesn't yet contain name after this.poll(). poll() reads state.json synchronously via allAgents(); if the controller writes agent state asynchronously or the OS has not yet made the write visible at the time allAgents() runs, the read can race with the write and the error banner fires spuriously even though spawn succeeded.
The AI summary states the controller calls reconcileAgents() before returning the RPC response, so this ordering is currently enforced by design. If that invariant is ever relaxed (e.g., fire-and-forget reconcile), this check will produce false negatives silently. Consider adding a comment here documenting the dependency:
📝 Suggested clarifying comment
spawn({ ... })
.then(() => {
clearTimeout(staleTimer)
+ // poll() reads state.json synchronously; safe because the controller
+ // calls reconcileAgents() before resolving the spawn RPC, so the new
+ // agent record is guaranteed to be on disk by this point.
this.poll()
if (this.state.agents.some(agent => agent.name === name)) {📝 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.
| .then(() => { | |
| clearTimeout(staleTimer) | |
| this.setBanner(`Spawned ${name}`, 'green', 3000) | |
| this.poll() | |
| if (this.state.agents.some(agent => agent.name === name)) { | |
| this.setBanner(`Spawned ${name}`, 'green', 3000) | |
| } else { | |
| this.setBanner(`Spawn ${name} returned but no agent state was found`, 'red', 8000) | |
| } | |
| .then(() => { | |
| clearTimeout(staleTimer) | |
| // poll() reads state.json synchronously; safe because the controller | |
| // calls reconcileAgents() before resolving the spawn RPC, so the new | |
| // agent record is guaranteed to be on disk by this point. | |
| this.poll() | |
| if (this.state.agents.some(agent => agent.name === name)) { | |
| this.setBanner(`Spawned ${name}`, 'green', 3000) | |
| } else { | |
| this.setBanner(`Spawn ${name} returned but no agent state was found`, 'red', 8000) | |
| } |
🤖 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 `@src/tui/app.ts` around lines 1030 - 1037, This block assumes the controller
has flushed agent state to disk before returning so poll() (which calls
allAgents()) will see the new agent; add a concise clarifying comment above this
.then(...) explaining the implicit ordering dependency: that poll() reads
state.json synchronously via allAgents(), that reconcileAgents() (in the
controller) must finish writing/flush state before the RPC response, and that if
that invariant is relaxed the "no agent state" branch may fire spuriously;
reference the functions/fields poll(), allAgents(), reconcileAgents(),
this.state.agents, and setBanner in the comment so future maintainers know the
race condition and where to fix it.
1 participant
Summary
Validation
Closes #94
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests