feat: Add pflow skill command to publish workflows as AI agent skills

[spinje]

Summary

Implements Task 119 - publish saved workflows as AI agent skills for Claude Code, Cursor, Codex, and Copilot.

Skills are symlinks from tool-specific directories to saved workflows in ~/.pflow/workflows/. The workflow file is enriched with a ## Usage section and frontmatter metadata that AI tools can read.

Task

119

Changes

New CLI commands:

• pflow skill save <name> [--personal] [--cursor/--codex/--copilot] - Publish workflow as skill
• pflow skill list - List all pflow-managed skills across tools
• pflow skill remove <name> [--personal] [--cursor/--codex/--copilot] - Remove skill symlinks
• pflow workflow history <name> - Show execution history and last used inputs

Core features:

• Symlink-based architecture (single source of truth)
• Multi-target support (Claude Code, Cursor, Codex, Copilot)
• Idempotent operations (no --force needed)
• Workflow enrichment with ## Usage section and frontmatter metadata
• Re-save detection hook to restore enrichment after workflow save --force
• Reserved name "skill" to prevent workflow naming conflicts

Explanation

The design uses symlinks rather than copies so that changes to the saved workflow automatically propagate to all tools. The ## Usage section is injected into the workflow file itself (before ## Steps) and is silently ignored by the markdown parser during execution.

Multi-target support was added to make pflow useful beyond Claude Code. The same symlink pattern works for all tools - only the directory paths differ.

The --force flag was intentionally removed from the spec because symlink creation is naturally idempotent (pointing to the same file), and enrichment replaces rather than duplicates the ## Usage section.

Created Docs

• .taskmaster/tasks/task_119/task-119.md - Task specification
• .taskmaster/tasks/task_119/task-review.md - Implementation review
• .taskmaster/tasks/task_119/implementation/progress-log.md - Development log

Testing

• 46 new tests added (24 service + 14 CLI + 5 formatter + 3 workflow manager)
• All 3663 tests pass
• All lint/type checks pass

Run make test to verify.

[claude]

Code Review: Task 119 - Publish Workflows as Claude Code Skills

This is a solid implementation with excellent test coverage and clean separation of concerns. The code follows project conventions well and the symlink-based architecture is elegant.

---

✅ Strengths

1. Clean Architecture: Excellent separation between business logic (skill_service.py) and CLI (skills.py)
2. Comprehensive Tests: 46 new tests with good coverage of edge cases, including broken symlinks, multi-target support, and re-enrichment
3. Type Safety: All functions properly typed with modern Python type hints
4. Idempotent Design: Smart removal of --force flag since symlink operations are naturally idempotent
5. Multi-Tool Support: Well-designed SKILL_TARGETS config makes supporting Cursor/Codex/Copilot trivial
6. Security: Proper atomic file writes with temp files, symlink validation
7. Documentation: Clear docstrings, inline comments explain workarounds (e.g., Claude Code description bug)

---

⚠️ Warnings — Should be addressed

1. Import Organization (skill_service.py:285)

import contextlib

with contextlib.suppress(OSError):
    skill_dir.rmdir()

Issue: Import inside function is unconventional and hurts readability.

Fix: Move to top-level imports:

# At top of file
import contextlib

# In function
with contextlib.suppress(OSError):
    skill_dir.rmdir()

2. Error Handling Gaps

skill_service.py:174-186 — enrich_workflow():

• Generic except Exception catches too much
• If write succeeds but os.replace() fails, temp file lingers
• No specific error messages for common failures

Suggestion:

try:
    with os.fdopen(temp_fd, 'w', encoding='utf-8') as f:
        f.write(new_content)
    os.replace(temp_path, workflow_path)
except OSError as e:
    Path(temp_path).unlink(missing_ok=True)
    raise OSError(f"Failed to write enriched workflow to {workflow_path}: {e}") from e
except Exception:
    Path(temp_path).unlink(missing_ok=True)
    raise

skills.py:172-183 — save_skill():

• Generic except Exception loses error context
• User gets str(e) which may be cryptic

Suggestion: Catch specific exceptions (FileExistsError, OSError, ValueError) and provide actionable messages.

3. Potential Race Condition (skill_service.py:244-252)

if symlink_path.exists() or symlink_path.is_symlink():
    raise FileExistsError(...)

skill_dir.mkdir(parents=True, exist_ok=True)
symlink_path.symlink_to(workflow_path)

Issue: Between the existence check and symlink creation, another process could create the same symlink (TOCTOU).

Fix: Wrap in try/except FileExistsError instead:

skill_dir.mkdir(parents=True, exist_ok=True)
try:
    symlink_path.symlink_to(workflow_path)
except FileExistsError:
    raise FileExistsError(f"Skill '{skill_name}' already exists at {symlink_path}")

4. Inconsistent String Formatting (skills.py:106)

Mix of f-strings and concatenation:

lines.append(f"  {label:<22} {project_subdir + '/':<18} ~/{personal_subdir}/")

Better:

lines.append(f"  {label:<22} {project_subdir+'/':<18} ~/{personal_subdir}/")

Minor, but consistent f-string usage is cleaner.

5. Missing Validation (skill_service.py:217-254)

create_skill_symlink() doesn't validate:

• workflow_path exists before creating symlink
• workflow_path is actually a file (not directory)
• skill_name is valid (could contain path traversal like "../../../evil")

Add:

if not workflow_path.exists() or not workflow_path.is_file():
    raise ValueError(f"Workflow file not found: {workflow_path}")
if ".." in skill_name or "/" in skill_name:
    raise ValueError(f"Invalid skill name: {skill_name}")

---

💡 Suggestions — Optional improvements

1. Deduplicate Target Label Logic

Both _format_target_label() (skills.py:80) and _get_target_help() (skills.py:31) build similar strings from SKILL_TARGETS. Consider extracting a shared helper.

2. Test Quality: Redundant Assertions

Several tests assert the same thing multiple ways:

assert result.exit_code == 0
assert "Published 'test-workflow' to Claude Code" in result.output

The output check implies success. Consider removing redundant exit_code checks when output confirms success.

3. Magic Strings

"## Usage" appears in multiple places. Consider a constant:

USAGE_SECTION_HEADER = "## Usage"

4. Logging Consistency

Some functions log (e.g., enrich_workflow:183), others don't (e.g., _inject_or_replace_usage). Consider consistent logging for debuggability.

5. Path Resolution Clarity (skill_service.py:296-300)

target = Path(os.readlink(symlink_path))
return (symlink_path.parent / target).resolve() if not target.is_absolute() else target.resolve()

This is correct but dense. Add a comment:

# Resolve relative symlinks relative to symlink's directory

---

🔒 Security Review

✅ Good:

• Atomic file writes prevent corruption
• Symlink detection in workflow_save_service.py:401 prevents deletion of skills
• Path traversal protection via is_relative_to()

⚠️ Consider: Validate skill_name doesn't contain path traversal ("..") — see Warning #5.

---

📊 Test Coverage Assessment

Excellent coverage (46 tests):

• ✅ Unit tests for all public functions
• ✅ Edge cases (broken links, missing workflows, idempotent operations)
• ✅ End-to-end integration test
• ✅ Multi-target support

Missing tests:

• Error paths for enrich_workflow() (write failures, permission errors)
• Path traversal attempts in skill_name
• Concurrent symlink creation (race condition)

---

🎯 Overall Assessment

This is high-quality work that's ready to merge with minor fixes. The architecture is sound, tests are comprehensive, and the feature delivers real value.

Priority fixes before merge:

1. Fix import location (Warning docs: Set up Mintlify documentation (Task 93) #1)
2. Add symlink race condition handling (Warning feat: Smart output display for registry run #3)
3. Validate skill_name for path traversal (Warning Shell node errors show generic 'Workflow failed with action: error' without details #5)

Nice to have:

• Improve error handling specificity (Warning feat: unify LLM usage via Simon Willison's llm library (Task 95) #2)
• Add missing error path tests

Great job on the multi-tool support expansion and the re-enrichment hook — these demonstrate excellent forward-thinking design. 🎉

[spinje]

Response to Code Review

Thanks for the thorough review! After careful analysis, I've concluded that no changes are needed. Here's my reasoning:

---

Warning 1: Import Organization (contextlib inside function)

Decision: Won't fix

This is a lazy import pattern used elsewhere in pflow (e.g., skills.py:237 has from collections import defaultdict inside list_skills()). It's intentional, works correctly, and moving it to top-level provides zero functional benefit.

---

Warning 2: Error Handling Gaps

Decision: Won't fix

The current pattern in enrich_workflow() is correct:

except Exception:
    Path(temp_path).unlink(missing_ok=True)
    raise  # Re-raises original exception with full context

This is the standard atomic write cleanup pattern. The suggested fix would lose the original exception context.

In save_skill(), the broad except Exception is intentional for multi-target resilience — we want to continue with other targets if one fails.

---

Warning 3: Potential Race Condition (TOCTOU)

Decision: Won't fix

The pre-check exists for better error messages, not atomicity:

if symlink_path.exists() or symlink_path.is_symlink():
    raise FileExistsError(f"Skill '{skill_name}' already exists at {symlink_path}")

This is a single-user CLI tool. The theoretical race condition is irrelevant in practice. If it did occur, symlink_to() would fail anyway with a less informative error.

---

Warning 4: Inconsistent String Formatting

Decision: Won't fix

The code {project_subdir + '/':<18} is correct — the + '/' is needed because format specs apply to the whole expression. The reviewer's suggested "fix" is functionally identical.

---

Warning 5: Missing Validation (Path Traversal)

Decision: Won't fix — already mitigated

skill_name comes from validated workflow names. WorkflowManager._validate_workflow_name() enforces:

re.match(r"^[a-z0-9]+(?:-[a-z0-9]+)*$", name)

This pattern cannot contain /, .., or any path characters. The workflow must exist (and thus have a validated name) before skill save can be called.

---

Suggestions 1-5

All declined as over-engineering or marginal value for this codebase.

---

Summary: The review raised valid theoretical concerns, but the current implementation is correct for a single-user CLI tool. The patterns used are intentional and consistent with the rest of the codebase.