feat: replace procedure architecture with skills-based approach (CYPACK-996)

[cyrusagent]

Assignee: Payton Webber

Summary

Replaces the rigid procedure-based agent session architecture with a flexible skills-based approach. Procedures (ProcedureAnalyzer, subroutine sequencing, validation loop) are removed in favor of SKILL.md files delivered via the Claude Agent SDK plugin system. This gives agents more natural control over their workflow while allowing users to customize and add their own skills.

Key Changes

• Skills architecture: Five core skills (debug, implementation, investigate, summarize, verify-and-ship) delivered as .claude-plugin packages via the SDK plugins parameter
• DefaultSkillsDeployer: On startup, copies bundled skills to ~/.cyrus/cyrus-skills-plugin/ so users can customize them. Idempotent — only deploys if the directory doesn't exist
• SkillsPluginResolver: Resolves internal + user plugins at runtime, discovers available skills dynamically, and builds skills guidance for the system prompt
• Stop hook: Ensures PRs are always created before sessions end, using a stop_hook_active guard to prevent infinite loops
• Platform-agnostic skills: verify-and-ship handles both GitHub PRs and GitLab MRs based on repository context
• Agent context injection: Bot usernames (GITHUB_BOT_USERNAME, GITLAB_BOT_USERNAME) injected into system prompt via <agent_context> block so skills can reference them dynamically
• User skills support: Users can add custom skills at ~/.cyrus/user-skills-plugin/ with automatic manifest scaffolding and conflict detection

Removed

• ProcedureAnalyzer and all procedure routing logic
• Subroutine sequencing and validation loop (~7000 lines)
• Label-based procedure rerouting
• 5 test files covering removed procedure functionality

Testing

• All 542 package tests passing
• TypeScript typecheck clean
• Biome lint clean
• DefaultSkillsDeployer unit tests (3 tests)
• Prompt assembly tests updated for new skills guidance format

Linear Issue

CYPACK-996

---

Tip: I will respond to comments that @ mention @cyrusagent on this PR. You can also submit a "changes requested" review with all your feedback at once, and I will automatically wake up to address each comment.

[cyrusagent]

PR Review — SOLID Principles Analysis

Overall, this is a strong refactor that removes ~7,000 lines of rigid procedure/subroutine/validation code in favor of a simpler, more flexible skills-based approach. The architecture is significantly easier to reason about. Below are observations organized by SOLID principle.

---

✅ Single Responsibility Principle (SRP) — Mostly Good

Positives:

• RunnerConfigBuilder has a clear, focused responsibility: assembling runner configs for both chat and issue sessions.
• ToolPermissionResolver cleanly owns tool permission logic without procedure concerns.
• AgentSessionManager is significantly simplified — it no longer handles procedure routing, validation loops, or subroutine transitions. It's back to its core job: managing session lifecycle and activity posting.
• Skills (SKILL.md files) each encapsulate a single workflow concern (implement, verify, debug, etc.).

Concern:

• EdgeWorker.deploySkillsToWorkspace() (line ~4478) is a filesystem operation that doesn't really belong in the EdgeWorker class. EdgeWorker is already a god-class with many responsibilities (webhook handling, session orchestration, prompt assembly, workspace setup, runner management, etc.). Consider extracting this into a SkillDeploymentService or at minimum into the GitService/workspace setup layer. This would keep the EdgeWorker focused on orchestration rather than file copying.

---

✅ Open/Closed Principle (OCP) — Strong Improvement

Positives:

• The skills-based architecture is inherently open for extension. Adding a new skill is just adding a new skills/<name>/SKILL.md directory — no code changes needed. Compare this to the old system where adding a subroutine required modifying the procedure registry, analyzer, and often the EdgeWorker itself.
• The stop hook in RunnerConfigBuilder.buildStopHook() provides a clean extension point via the hook pattern rather than hardcoded control flow.

Concern:

• The stop hook's guidance text is hardcoded as a string literal (lines 343-346 in RunnerConfigBuilder.ts). If different session types need different stop guidance in the future, this would need to be parameterized. Not a problem today, but worth noting.

---

✅ Liskov Substitution Principle (LSP) — No Issues

• The IMcpConfigProvider, IChatToolResolver, and IRunnerSelector interfaces in RunnerConfigBuilder.ts are well-designed. Any implementation satisfying these contracts would work correctly as a substitute.
• AgentSessionManager constructor no longer takes optional ProcedureAnalyzer and SharedApplicationServer params that changed its behavior based on whether they were provided. The simplified constructor produces consistent behavior regardless of context — this is a LSP improvement.

---

✅ Interface Segregation Principle (ISP) — Good

Positives:

• IMcpConfigProvider, IChatToolResolver, and IRunnerSelector are narrow, focused interfaces that expose only what RunnerConfigBuilder needs. This is textbook ISP.
• Removing ProcedureAnalyzer from AgentSessionManager's constructor eliminates a dependency that forced the class to know about procedure concepts even when they weren't applicable (e.g., Slack chat sessions passed undefined).

---

⚠️ Dependency Inversion Principle (DIP) — Partial

Positives:

• RunnerConfigBuilder depends on abstractions (IMcpConfigProvider, IChatToolResolver, IRunnerSelector) rather than concrete implementations. Solid DIP.

Concerns:

• EdgeWorker.deploySkillsToWorkspace() directly calls readdir, readFile, writeFile, existsSync, and mkdir (lines 4479-4500). These are concrete filesystem dependencies that make the method untestable in isolation. Injecting a filesystem abstraction (or extracting to a service) would improve testability and follow DIP.
• The stop hook inline-imports no external dependencies, which is fine, but the StopHookInput type is cast with as (line 331) rather than being typed through the hook system generics. This is a minor type-safety concern.

---

Additional Observations

1. Dead comment: Line 113 in EdgeWorker.ts — // Procedures removed — skills-based architecture (CYPACK-996) is a tombstone comment. The git history already captures what was removed and why. Consider removing it.

2. Runner type switch chain (lines 210-229 in RunnerConfigBuilder.ts): The if/else if chain for runner type fallback when labels change mid-session is a potential OCP violation. If a new runner type is added, this chain must be extended. A map-based lookup (runnerSessionIdMap) would be more extensible:

   const sessionRunnerMap: Record<string, RunnerType> = {
     claude: session.claudeSessionId,
     gemini: session.geminiSessionId,
     codex: session.codexSessionId,
     cursor: session.cursorSessionId,
   };
   // Find existing runner type
   const existingRunner = Object.entries(sessionRunnerMap)
     .find(([_, id]) => id && _ !== runnerType)?.[0] as RunnerType | undefined;

3. Skill deployment timing: deploySkillsToWorkspace runs during session creation (line 2615). If skills are updated while a session is active, the session won't see the changes. This is probably fine for now since sessions are typically short-lived, but worth documenting.

4. Unused _log parameter: buildStopHook takes _log: ILogger but doesn't use it (line 323). Either use it for debug logging in the hook callback or remove the parameter.

---

Summary

Principle	Assessment
SRP	✅ Strong — major improvement via procedure removal. Minor concern: deploySkillsToWorkspace in EdgeWorker
OCP	✅ Strong — skills are inherently extensible without code changes
LSP	✅ Clean — no behavioral surprises from substitution
ISP	✅ Good — narrow, focused interfaces for RunnerConfigBuilder deps
DIP	⚠️ Partial — RunnerConfigBuilder is good, but deploySkillsToWorkspace has concrete FS deps

Overall verdict: This is a well-executed simplification that significantly improves the codebase's adherence to SOLID principles. The 18:1 deletion-to-addition ratio speaks for itself. The concerns raised are minor and none are blocking — they represent opportunities for future improvement rather than current defects.

[cyrusagent]

Follow-up Review: Skills Availability for External Repositories

After tracing the full skill deployment path, there is a critical gap — skills are only available when Cyrus works on the Cyrus repo itself. They are NOT available when working on external repos.

---

The Problem

deploySkillsToWorkspace() reads from join(this.cyrusHome, "skills") which resolves to ~/.cyrus/skills/:

// EdgeWorker.ts:4478-4501
private async deploySkillsToWorkspace(workspacePath: string): Promise<void> {
    const sourceDir = join(this.cyrusHome, "skills");  // ~/.cyrus/skills/
    if (!existsSync(sourceDir)) {
        this.logger.debug("No skills directory found at", sourceDir);
        return;  // ← SILENTLY SKIPS DEPLOYMENT
    }
    // ...copies SKILL.md files to workspace...
}

~/.cyrus/skills/ does not exist. The actual skills live in the Cyrus repo at skills/. There's no build step, install hook, or copy mechanism to get them to ~/.cyrus/skills/.

Why it appears to work for the Cyrus repo

The Cyrus repo has symlinks: .claude/skills/debug -> ../../skills/debug. When Cyrus works on its own repo, the symlinks already resolve to the skills. But for any other repository, the workspace .claude/skills/ directory is empty because deploySkillsToWorkspace() has nothing to copy.

What needs to happen

Option A — Ship skills with the CLI package: Add a postinstall script (or startup-time init in the CLI) that copies the canonical skills to ~/.cyrus/skills/. Also include the skills/ directory in the npm package's files array (apps/cli/package.json currently only ships dist and README.md).

Option B — Resolve skills from the package at runtime: Instead of reading from cyrusHome, resolve the skills directory relative to the edge-worker package (or a new cyrus-skills package) using import.meta.url or __dirname. This way skills are always available from the installed package.

Option C — Embed skills in the build: Bundle the SKILL.md contents into the compiled JS (e.g., as string constants or via the existing copy-prompts build script in edge-worker/package.json).

---

Secondary Issue: Workflow guidance duplicates skill content

The system prompt hardcodes workflow steps (lines 4350-4357):

systemPrompt +=
    "\n\n## Workflow\n\n" +
    "Follow this workflow to completion:\n\n" +
    "1. **Implement** — Implement the requested changes...\n" +
    "2. **Verify & Ship** — Run all quality checks...\n" +
    "3. **Summarize** — Post a concise summary...\n\n" +
    "Do NOT skip steps. Complete each phase before moving to the next.";

This duplicates what's in implementation/SKILL.md, verify-and-ship/SKILL.md, and summarize/SKILL.md. If the skills are properly deployed, the agent discovers them at runtime and this hardcoded guidance becomes redundant (or worse, could drift out of sync). Consider referencing the skills by name instead of duplicating their content in the prompt.

---

Issue Requirement: "added to the runner config as one of its params"

The issue description states: "These skills should then be added to the runner config as one of its params (see the claude agent sdk docs for more context)". Currently:

• AgentRunnerConfig in cyrus-core has no skills parameter
• Skills are deployed purely via filesystem (deploySkillsToWorkspace())
• The Claude Agent SDK may support a skills config option that would be the proper way to pass skills to the runner

If the SDK supports it, adding skills to the runner config would be more reliable than filesystem deployment and would solve the cross-repo problem entirely.

[cyrusagent]

SOLID Principles Review — Final Pass

Reviewed the full PR diff (27 files, ~7000 lines removed, skills-based architecture added). Here's the assessment against each SOLID principle:

S — Single Responsibility ✅

Good:

• RunnerConfigBuilder owns all runner config assembly (hooks, MCP, model selection, plugins). Clean separation from EdgeWorker.
• AgentSessionManager is significantly simplified — removed ~487 lines of procedure routing, validation loops, and subroutine completion logic. It now focuses on session lifecycle and activity posting.
• resolveSkillsPlugins() is a focused method with one job: resolve the plugin path from user dir or bundled package.
• The stop hook in RunnerConfigBuilder.buildStopHook() has a single concern: ensure PR/summary before session ends.

Minor observation (non-blocking):

• The skills guidance text (lines 4355-4364 of EdgeWorker.ts) is inlined as a string literal inside the prompt assembly method. If this grows, consider extracting it to a constant or template file. For now, the scope is small enough that it's fine.

O — Open/Closed ✅

Good:

• The plugin architecture is inherently open for extension — users can add custom skills to ~/.cyrus/cyrus-skills-plugin/ or ~/.cyrus/skills/ without modifying source code.
• resolveSkillsPlugins() has a clean priority chain (user plugin → bundled plugin) that can be extended with additional resolution strategies without changing existing logic.
• Skills are discovered at runtime by the SDK, meaning new skills can be added by simply creating new SKILL.md files — no code changes needed.

L — Liskov Substitution ✅

Good:

• IssueRunnerConfigInput.plugins is optional, so callers that don't provide plugins still work identically.
• The plugins field flows through AgentRunnerConfig → ClaudeRunnerConfig → SDK query() with consistent optional semantics at every layer.
• ChatSessionHandler correctly constructs AgentSessionManager without the removed procedure parameters — the constructor signature change is backward-compatible.

I — Interface Segregation ✅

Good:

• RunnerConfigBuilder depends on narrow interfaces (IMcpConfigProvider, IChatToolResolver, IRunnerSelector) rather than the full EdgeWorker — this was already well-designed and remains clean.
• SdkPluginConfig is a focused type re-exported from the SDK — no fat interface.
• The plugins field is scoped to Claude runner only in buildIssueConfig() (line 278: runnerType === "claude" && input.plugins?.length), not leaking Claude-specific concerns to other runners.

D — Dependency Inversion ✅

Good:

• EdgeWorker depends on the SdkPluginConfig type abstraction (from the SDK), not on concrete plugin implementations.
• Plugin resolution is encapsulated in resolveSkillsPlugins() which returns the abstract SdkPluginConfig[] — the SDK handles the concrete plugin loading.
• The constructor interfaces in RunnerConfigBuilder ensure it depends on abstractions, not concretions.

---

One finding — dead type in CyrusAgentSession (non-blocking)

packages/core/src/CyrusAgentSession.ts lines 106-131 still contain the procedure metadata type definition (with procedureName, currentSubroutineIndex, subroutineHistory, and validationLoop). Since all procedure logic has been removed from AgentSessionManager and EdgeWorker, this type is now dead code. It won't cause runtime issues but is unnecessary weight.

Recommendation: Remove the procedure field from CyrusAgentSession.metadata in a follow-up cleanup — it's low priority since it's just a type definition and doesn't affect behavior.

---

Summary

The PR cleanly follows all five SOLID principles. The architecture shift from rigid procedures to plugin-delivered skills is well-executed:

• Responsibilities are clearly separated across EdgeWorker, RunnerConfigBuilder, and AgentSessionManager
• The plugin system is open for extension without modification
• Interface boundaries are narrow and focused
• Dependencies point toward abstractions

Verdict: Approved ✅