feat(skills): Phase 3a skill activation runtime

[josephfung]

Summary

Closes #1495 (Phase 3a of epic #1436). Builds on Phase 2 (#1489) by shipping the deferred activation runtime from design §6.

• Unified discovery — toolSearch returns kind:"skill" for non-synthetic bundles and promotes member-atom matches to their owning skill (so task-create surfaces as tasks, not a bare tool without instructions).
• skill-activate tool — discovery-enabled agents get it alongside tool-registry; activation expands member tools into the working toolkit and injects the SKILL.md body mid-turn.
• Tiered lookup — Tier 0 pinned (eager, unchanged) → Tier 1 progress.activeSkills (wake re-load with relevance filter + cap 5; no-op writes skipped) → Tier 2 discovery when allow_discovery.
• Authority unchanged — activation never widens allowed_callers / action_risk; disallowed members land in skippedTools. Persistence uses bound task only (no LLM task_id).
• Docs (spec 03 + 19) and CHANGELOG [Unreleased].

Design review follow-ups (ee0c076)

• Skip wake activeSkills write when name set unchanged; preserve activatedAt
• Remove LLM-facing task_id from skill-activate
• Runtime tests for mid-turn splice/expansion + wake no-op write
• Tighten relevance to whole-token filter; align spec 03

Test plan

• pnpm run typecheck (+ console typecheck)
• pnpm run lint
• Focused unit/runtime tests for activation + wake short-circuit
• pnpm test with DATABASE_URL on initial implementation (5579 passed)

  

[josephfung]

Design review (Phase 3a skill activation)

Overall the architecture is right and matches design §6. The "activation never widens authority" invariant is real and tested (resolveSkillActivation re-checks allowed_callers per member tool; admin-bundle → tools: [], skippedTools: ['secret-admin']). Additive, no migration — activeSkills rides existing tasks.progress. Per-task isolation and the skillRegistry-unset fallback are handled well.

skill-activate as its own tool is the correct call. It mirrors the existing tool-registry discovery tool and the two-step discover → activate flow is deliberate: discovery can return several candidate skills, and you only want to pay the SKILL.md token cost for the one bundle the LLM actually commits to. Inlining instructions into tool-registry results would speculatively bloat context. Keep the two tools. (Vocabulary is consistent too: skill-activate is registered as a tool / atom that activates skill bundles, per ADR-031.)

Changes to make before ready-for-review (ranked)

1. [blocking] Wake re-load persists unconditionally — self-inflicted write + bus event on every wake.
In src/agents/runtime.ts, when wakeSkills.length > 0 we always call replaceActiveSkillsBlock → setActiveSkillsBlock, which bumps updated_at and publishes a task-updated bus event — even when the re-selected set is identical to what's already stored. replaceActiveSkillsBlock also restamps every activatedAt to now(), so the content churns each wake by design.
This is the exact failure shape that's bitten this system before: the BacklogHeartbeat wake-flood (#1410) and the drift-detector false-positives on task-wake (#1064). A no-op DB write + bus event on every wake of any task that has ever activated a skill will compound.
→ Short-circuit when the re-selected name set equals the stored set: skip both the write and the bus publish. Only persist when the set actually changed. Avoid restamping activatedAt for skills that were already present.

2. [blocking] skill-activate accepts an LLM-supplied task_id and writes to it with no ownership check.
In skills/skill-activate/handler.ts we validate the UUID format, then getTask(task_id) → setActiveSkillsBlock(task_id, …) for whatever id the model passes. Nothing verifies the calling agent owns that task, so an agent can write into another task's activeSkills. Blast radius is low (it's a hint list; authority still isn't widened), but it's a cross-task write driven by model input.
→ Drop the LLM-facing task_id param entirely and rely solely on the bound task from taskMetadata (trustworthy). If a param is genuinely needed, gate it on an ownership check.

3. [should-fix] No runtime-level test coverage for the behavior this PR is actually about.
The helpers (skill-activation.ts, active-skills-progress.ts) are well tested, but nothing exercises AgentRuntime: the messages.splice system-message injection, mid-turn workingToolDefs expansion, or the wake re-load path. The unchecked "Manual: coordinator → tool-registry → skill-activate → member tools callable" box is the most likely thing to break.
→ Add a runtime-level test asserting a skill-activate result splices the instruction block and appends the member tool defs, and run + check the manual end-to-end path.

4. [minor] "Relevance re-check" oversells what the code does.
selectActiveSkillsForWake scores all stored skills, sorts relevant-first, then slices to cap. With cap 5 and ≤5 stored, the relevance signal never drops anything — it's a tiebreaker, not a filter. The token substring match is also loose (relevance.includes("task") fires on "multitask"). The spec wording in docs/specs/03-tools-and-execution.md ("re-checked for relevance against the current step") reads stronger than the implementation.
→ Either tighten the impl (make relevance actually filter) or soften the spec wording so the two match.

Notes (no action required):

• Declaring the skillRegistry capability silently also injects toolRegistry (the "accompanies skillRegistry" coupling in execution.ts). Works, but it's a surprising implicit grant — worth a one-line note near VALID_CAPABILITIES in loader.ts.
• Context budget: the cap-5 comment estimates ~5k tokens per SKILL.md, so up to ~25k tokens of instructions are re-appended to the prompt on every wake (up to cap, for as long as skills stay in the MRU). That's designed-in, but it compounds with chore: add project README, architecture specs, and repo structure #1 — heavier prompts and needless writes both scale with wake count. The right lever is prompt-cache stability (keep the blocks in a stable prefix), which is exactly what the activatedAt restamping in chore: add project README, architecture specs, and repo structure #1 undermines. Worth sanity-checking a couple of real SKILL.md sizes against the assumed 5k to confirm cap 5 is right.

[cursor]

Addressed in ee0c076:

1. Wake write short-circuit — compare re-selected vs stored name sets; skip setActiveSkillsBlock (and its updated_at / task-updated) when unchanged. reconcileActiveSkillsBlock preserves activatedAt for survivors instead of restamping.

2. Dropped LLM task_id — skill-activate persists only via bound taskMetadata; hallucinated task_id input is ignored (test covers this).

3. Runtime coverage — AgentRuntime skill-activate (#1495) asserts instruction splice + workingToolDefs expansion, and wake re-load with no write when the set is unchanged.

4. Relevance — whole-token overlap (≥3 chars); irrelevant skills are dropped when any match remains; MRU fallback only when none match. Spec 03 wording updated to match.

Also noted the skillRegistry → toolRegistry implicit inject near VALID_CAPABILITIES.

[josephfung]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds unified tool and skill discovery with typed kind results, a skill-activate tool, per-tool authorization filtering, and runtime instruction injection. It persists capped active skills in task progress, reloads relevant skills on wake, and wires SkillRegistry through execution and agent runtime construction. Tool manifests, defaults, specifications, changelog entries, and unit tests are updated accordingly.

Possibly related issues

• Phase 3: imported NL + asset skills #1490 — Imported skills consume this activation runtime as a prerequisite.
• Epic: tools/skills granularity rework + Claude-skill import #1436 — The PR implements the epic’s tiered discovery, activation, persistence, and authority-containment objectives.

Possibly related PRs

• josephfung/curia#1493 — Provides the Phase 2 skill-bundle model and registry behaviour used by this runtime.
• josephfung/curia#293 — Overlaps the runtime path that expands discovered skill tools into callable task tools.
• josephfung/curia#1254 — Also modifies AgentRuntime to inject dynamic guidance and skill tool definitions.

Suggested labels: enhancement, size:XXL, orchestration

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately names the main change: Phase 3a skill activation runtime.
Description check	✅ Passed	The description is clearly about the same skill-activation runtime work, so it passes the smell test.
Linked Issues check	✅ Passed	The changes align with #1495 by adding unified discovery, skill activation, wake persistence, and the requested docs.
Out of Scope Changes check	✅ Passed	No clearly unrelated changes stand out; the code, docs, config, and tests all serve the stated skill-activation work.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)

config/registry-defaults.yaml

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

skills/skill-activate/tool.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

skills/tool-registry/tool.json

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'

---

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

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

src/skills/skill-activation.ts (1)

89-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two copies of "may this agent call this tool" logic — pick one and be done with it.

unifiedToolSearch reimplements the exact allowed_callers check that agentMayCallTool already performs a few lines below. Two implementations of the same rule is exactly the sort of thing that quietly diverges after the next edit, and nobody notices until production. Riveting.

♻️ Proposed dedup

-    const allowed = tool.manifest.allowed_callers;
-    if (allowed && allowed.length > 0 && !allowed.includes(agentId)) continue;
+    if (!agentMayCallTool(toolRegistry, name, agentId)) continue;

Also applies to: 112-122

🤖 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/skill-activation.ts` around lines 89 - 90, Remove the inline
allowed_callers filtering from unifiedToolSearch and reuse agentMayCallTool as
the single authorization check for each candidate tool. Ensure both the primary
and alternate search paths apply agentMayCallTool consistently, preserving the
existing behavior for permitted and denied agents.

🤖 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 `@skills/skill-activate/tool.json`:
- Line 4: Update the version field in the new skill manifest from 0.1.1 to
0.1.0, preserving the rest of the manifest unchanged.

In `@src/db/active-skills-progress.ts`:
- Around line 67-74: Update activeSkillNameSetsEqual to perform true set
equality without relying on callers to deduplicate: compare the unique values
represented by both arrays, ensuring duplicate entries cannot make different
sets appear equal. Preserve the boolean result for identical sets and the
existing function signature.

In `@src/skills/execution.test.ts`:
- Line 2182: Remove the duplicate const searchResults declaration within the
same it() test block in execution.test.ts, leaving one declaration with its
existing type and behavior unchanged.

In `@src/skills/types.ts`:
- Around line 228-236: Remove the obsolete duplicate doc comment above the
toolSearch field, leaving a single accurate documentation block for toolSearch
and preserving the field definition and its type unchanged.

---

Nitpick comments:
In `@src/skills/skill-activation.ts`:
- Around line 89-90: Remove the inline allowed_callers filtering from
unifiedToolSearch and reuse agentMayCallTool as the single authorization check
for each candidate tool. Ensure both the primary and alternate search paths
apply agentMayCallTool consistently, preserving the existing behavior for
permitted and denied agents.

🪄 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: ca464a21-b9d3-4317-9935-12c354bd53ca

📥 Commits

Reviewing files that changed from the base of the PR and between 7c0cca3 and 3f40a52.

📒 Files selected for processing (21)

• CHANGELOG.md
• config/registry-defaults.yaml
• docs/specs/03-tools-and-execution.md
• docs/specs/19-tasks-and-backlog.md
• skills/skill-activate/handler.test.ts
• skills/skill-activate/handler.ts
• skills/skill-activate/tool.json
• skills/tool-registry/handler.ts
• skills/tool-registry/tool.json
• src/agents/runtime.ts
• src/db/active-skills-progress.ts
• src/db/task-repo.ts
• src/index.ts
• src/skills/execution.test.ts
• src/skills/execution.ts
• src/skills/loader.ts
• src/skills/skill-activation.ts
• src/skills/types.ts
• tests/unit/agents/runtime.test.ts
• tests/unit/skills/active-skills-progress.test.ts
• tests/unit/skills/skill-activation.test.ts