Skill lifecycle phase 2: skill_manage with origin-scoped rails

[jamiepine]

Phase 2 of docs/design-docs/skill-lifecycle.md: the mutation surface. Agents (and users, through the same tool) can now create, patch, and retire skills — with the write origin deciding what's allowed.

skill_manage

One action-dispatched tool: create, patch, edit, delete, write_file, remove_file.

skill_manage(action="patch", name="discord-formatting",
             old_string="Post the full log.",
             new_string="Post a summary; link the full log.")

WriteOrigin is set by the process constructing the tool server — the model never supplies it. Regular branches carry User origin; the Phase 3 reflection branch and Phase 4 curator will carry Agent, which activates the tighter rails:

• Agent writes are limited to workspace skills — never installed (created_by = 'installed'), pinned, or instance-level ones.
• Agent must have read_skill'd the target this session before patch/edit (a shared per-session read tracker between the two tools — a mechanical rail against patching imagined content).
• Agent delete archives to {workspace}/skills/.archive/ (recoverable) instead of removing; User delete removes.
• Pin blocks delete for User and every mutation for Agent.
• delete with absorbed_into requires the absorbing skill to exist on disk and differ from the target — consolidation fails closed on unverified destruction.

Validation on all origins: name charset/length, frontmatter parses with a description within the 80-char budget, non-empty body, 100KB SKILL.md / 1MiB support file caps, and path rails on support files (must live under references|templates|scripts|assets, no .., canonicalized inside the skill dir, symlinks refused). Every successful mutation reloads the live SkillSet directly — no watcher reliance — and records to skill_usage (record_created carries conversation provenance for agent-created skills, record_patch bumps counters).

skills_list

Companion read tool: every loaded skill with source, provenance, lifecycle state, pin flag, and usage counters — the picture the prompt index compresses away. Branches get both tools; workers keep read_skill only.

Category discovery

create accepts an optional category, and the loader now scans one level deeper: both skills/{name}/SKILL.md and skills/{category}/{name}/SKILL.md load.

Lifecycle controls

• API: POST /agents/skills/{pin,adopt,archive,restore}. adopt flips created_by to 'agent' — the explicit act of handing a skill to curation; it is never automatic.
• CLI: spacebot skill pin|unpin|adopt|archive|restore, plus provenance/usage in spacebot skill info. The CLI opens only the agent's SQLite (not the full Db bundle), so it doesn't contend for the redb lock a running daemon holds.
• Archive keeps every prior copy ({name}-1, -2, ... suffixes); restore takes the most recent.

Testing

15 tests on the tool: name/content validation, path escape + symlink refusal, and the full rail matrix exercised through real tool calls against a temp workspace and in-memory SQLite — user create/provenance, agent conversation-origin recording, read-before-write denial then success, installed/pinned refusals for agents, archive-vs-remove delete semantics, pinned user-delete block, absorbed_into fail-closed, support file write + linked_files reload, category discovery.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ef38f2de-46a4-44ef-8539-4022b9ec673d

📥 Commits

Reviewing files that changed from the base of the PR and between 0a4a175 and 19228fb.

📒 Files selected for processing (10)

• prompts/en/tools/skills_list_description.md.j2
• src/api/skills.rs
• src/main.rs
• src/prompts/text.rs
• src/skills.rs
• src/skills/usage.rs
• src/tools.rs
• src/tools/read_skill.rs
• src/tools/skill_manage.rs
• src/tools/skills_list.rs

🚧 Files skipped from review as they are similar to previous changes (9)

• src/prompts/text.rs
• src/tools/read_skill.rs
• src/tools/skills_list.rs
• src/skills.rs
• src/main.rs
• src/api/skills.rs
• src/tools.rs
• src/tools/skill_manage.rs
• src/skills/usage.rs

---

Walkthrough

This change adds persistent skill management across storage, usage tracking, tools, CLI commands, and API routes. It supports creation, modification, deletion, pinning, adoption, archiving, restoration, metadata listing, and read tracking.

Changes

Persistent skill lifecycle

Layer / File(s)	Summary
Skill storage and lifecycle state src/skills.rs, src/skills/usage.rs, src/config/types.rs	Skills support archive and restore operations, category discovery, shared loading, and lifecycle usage metadata.
Skill management tool and validation src/tools/skill_manage.rs	SkillManageTool adds validated skill and support-file mutations with origin-specific permissions, reloads, provenance, and tests.
Tool registration and skill metadata src/tools.rs, src/tools/read_skill.rs, src/tools/skills_list.rs, src/prompts/text.rs, prompts/en/tools/*	The tool registry exposes skill management and listing tools. Skill reads update a shared tracker. Prompt templates describe skill management and listing behavior.
CLI and API lifecycle interfaces src/main.rs, src/api/skills.rs, src/api/server.rs	CLI commands and API routes expose pinning, adoption, archiving, and restoration. Skill output includes usage and lifecycle metadata.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• spacedriveapp/spacebot#621: Adds the skill lifecycle and SkillUsageStore foundations extended by this change.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description directly explains the skill lifecycle mutation surface, origin-scoped permissions, lifecycle controls, validation, and testing covered by the changes.
Title check	✅ Passed	The title clearly identifies Phase 2 and the main skill_manage feature with origin-scoped safeguards.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jamiepine/skill-manage

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/tools.rs (1)

893-917: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Consider WriteOrigin::Agent for the branch tool server.

This branch server is model-driven, but SkillManageTool is registered with WriteOrigin::User. For that origin, edits can target instance-level skills and delete performs permanent remove_dir_all deletion. If branches should keep the autonomous write guards, use WriteOrigin::Agent; otherwise keep this as an explicit intentional policy decision.

🤖 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/tools.rs` around lines 893 - 917, Review the branch tool server
construction and decide the intended write policy for SkillManageTool. If branch
operations are autonomous, change the SkillManageTool registration in the server
built by the surrounding function to use WriteOrigin::Agent, preserving the
existing read tracker; otherwise document or explicitly retain the User origin
as an intentional policy choice.

🧹 Nitpick comments (3)

src/prompts/text.rs (1)

99-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the arm into the "Tool Descriptions" block.

The arm sits between the fragment arms. All other tools/* keys are grouped from Line 153. Move it there to keep the grouping intact.

🤖 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/prompts/text.rs` around lines 99 - 101, Move the ("en",
"tools/skill_manage") match arm from its current position between the fragment
arms into the existing "Tool Descriptions" block with the other tools/* entries,
preserving its include_str! mapping unchanged.

src/api/skills.rs (1)

537-542: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Restore maps every failure to 404.

restore_skill_dir fails for two distinct reasons: no archived copy exists, and an active skill directory already occupies the name. The second case is a conflict, not a missing resource. A client cannot tell the two apart. Return 409 CONFLICT for the occupied-name case, and document it in the utoipa::path responses.

🤖 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/api/skills.rs` around lines 537 - 542, Update the restore flow around
restore_skill_dir so an existing active skill directory is mapped to
StatusCode::CONFLICT while an absent archived copy remains
StatusCode::NOT_FOUND. Distinguish these failure cases using the returned error,
preserve the warning context, and document both 404 and 409 responses in the
associated utoipa::path definition.

src/skills.rs (1)

449-463: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

The suffix scan stops at the first gap.

The loop breaks when {name}-{suffix} is missing. If an intermediate archive copy is removed, later copies are not seen. Example: .archive/deploy and .archive/deploy-2 exist, deploy-1 does not. The scan then selects deploy, which is the oldest copy, not the newest.

Read the archive directory once and select the entry with the highest suffix instead.

🤖 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.rs` around lines 449 - 463, Replace the sequential suffix loop in
the archive source-selection logic with a single read of archive_root, then
inspect matching entries for name and name-N forms and select the highest
numeric suffix. Preserve the plain name as the base candidate and ensure gaps do
not prevent later archive copies from being selected.

🤖 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/api/skills.rs`:
- Around line 481-503: Update archive_skill to read the skill record from the
already-resolved store and return StatusCode::FORBIDDEN when its pinned flag is
true, before calling archive_skill_dir. Preserve the existing workspace-source
validation and archiving behavior for unpinned skills.

In `@src/main.rs`:
- Around line 1296-1298: Update the skill info usage-store flow around
open_skill_usage_store and store.get to handle both Err cases instead of
silently discarding them. Print a concise note explaining the usage section is
unavailable, including the relevant error, while preserving the existing output
for successful store opening and record retrieval.
- Around line 1348-1374: Open the usage store before moving the directory in
both SkillCommand::Archive and SkillCommand::Restore. Move the
open_skill_usage_store call ahead of archive_skill_dir or its restore
equivalent, while keeping the subsequent set_archived or restore state update
behavior unchanged.

In `@src/skills.rs`:
- Around line 403-483: Update archive_skill_dir and restore_skill_dir to
preserve the skill’s workspace-relative category path instead of using only name
under the workspace root. Archive into the corresponding relative location
beneath .archive, retain suffix collision handling, and restore to that same
relative workspace path while checking for an existing destination. Ensure
required category/archive directories are created and no category component is
dropped.

In `@src/skills/usage.rs`:
- Around line 256-266: Update SkillUsage::set_restored to upsert the skill_usage
record instead of relying on a bare UPDATE, creating a missing row with active
state and a null archived_at while preserving the existing restoration behavior
for rows that already exist. Reuse the lifecycle upsert pattern used by the
other methods in this file.

In `@src/tools/skill_manage.rs`:
- Around line 96-106: The poisoned SkillReadTracker mutex must be recoverable
instead of panicking. In src/tools/skill_manage.rs lines 96-106, update has_read
to recover the poisoned guard with into_inner; apply the same change in
src/tools/read_skill.rs lines 106-111 before inserting the lowercased skill
name, replacing the expect-based lock handling at both sites.
- Around line 143-148: Extend the read-before-write check in the skill
modification rail around action matching to include write_file and remove_file
alongside patch and edit. Preserve the existing has_read check and error
response so all mutations of skill content require a prior read_skill.

In `@src/tools/skills_list.rs`:
- Around line 68-81: Move the inline description from ToolDefinition::definition
into a new prompts/en/tools/skills_list_description.md.j2 file, preserving its
text. Register the tools/skills_list prompt key in src/prompts/text.rs and
update skills_list::definition to load the description through
crate::prompts::text::get, matching the existing skill_manage pattern.

---

Outside diff comments:
In `@src/tools.rs`:
- Around line 893-917: Review the branch tool server construction and decide the
intended write policy for SkillManageTool. If branch operations are autonomous,
change the SkillManageTool registration in the server built by the surrounding
function to use WriteOrigin::Agent, preserving the existing read tracker;
otherwise document or explicitly retain the User origin as an intentional policy
choice.

---

Nitpick comments:
In `@src/api/skills.rs`:
- Around line 537-542: Update the restore flow around restore_skill_dir so an
existing active skill directory is mapped to StatusCode::CONFLICT while an
absent archived copy remains StatusCode::NOT_FOUND. Distinguish these failure
cases using the returned error, preserve the warning context, and document both
404 and 409 responses in the associated utoipa::path definition.

In `@src/prompts/text.rs`:
- Around line 99-101: Move the ("en", "tools/skill_manage") match arm from its
current position between the fragment arms into the existing "Tool Descriptions"
block with the other tools/* entries, preserving its include_str! mapping
unchanged.

In `@src/skills.rs`:
- Around line 449-463: Replace the sequential suffix loop in the archive
source-selection logic with a single read of archive_root, then inspect matching
entries for name and name-N forms and select the highest numeric suffix.
Preserve the plain name as the base candidate and ensure gaps do not prevent
later archive copies from being selected.

🪄 Autofix

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: 6ea06518-bc85-401b-9cfb-7b2e3821dfca

📥 Commits

Reviewing files that changed from the base of the PR and between 3eb8b25 and 0a4a175.

📒 Files selected for processing (12)

• prompts/en/tools/skill_manage_description.md.j2
• src/api/server.rs
• src/api/skills.rs
• src/config/types.rs
• src/main.rs
• src/prompts/text.rs
• src/skills.rs
• src/skills/usage.rs
• src/tools.rs
• src/tools/read_skill.rs
• src/tools/skill_manage.rs
• src/tools/skills_list.rs

[jamiepine]

On the outside-diff comment about WriteOrigin::User for the branch server: conversation branches keep User origin deliberately — they act inside a live conversation where the user is present and directing, which is the consent axis the origins encode (same reason Hermes lets foreground deletes remove but autonomous deletes archive). The real gap the comment surfaced was that ingestion and memory-persistence branches share this constructor and are fully autonomous: fixed in 19228fb by registering the skill tools only for the Default (conversation) branch profile, so pipeline branches get no skill mutation surface at all. The reflection branch lands in phase 3 with its own WriteOrigin::Agent server.