|
| 1 | +# deepwork-jobs Expert Review |
| 2 | + |
| 3 | +**PR**: #192 - feat: Add experts system for auto-improving domain knowledge |
| 4 | +**Date**: 2026-02-01 |
| 5 | +**Reviewer**: deepwork-jobs expert |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +The experts system is a well-designed addition to DeepWork that follows established patterns from the jobs system. The architecture cleanly separates concerns across schema definition (`expert_schema.py`), parsing (`experts_parser.py`), generation (`experts_generator.py`), and CLI (`experts.py`). The use of dynamic command embedding (`$(deepwork topics ...)`) in the generated agent template is an elegant solution for keeping expert agents up-to-date without regenerating them. |
| 10 | + |
| 11 | +From a DeepWork Jobs perspective, this integration is solid. The experts system complements jobs by providing domain knowledge that can be leveraged during job execution. The sync command properly handles both jobs and experts, and the naming conventions (`dwe_` prefix for expert agents) avoid conflicts with job-generated skills. |
| 12 | + |
| 13 | +## Issues Found |
| 14 | + |
| 15 | +### Issue 1 |
| 16 | +- **File**: `src/deepwork/schemas/expert_schema.py` |
| 17 | +- **Line(s)**: 22, 50, 75 |
| 18 | +- **Severity**: Minor |
| 19 | +- **Issue**: The `additionalProperties: False` constraint on all three schemas is strict and may cause friction when evolving the schema. For example, if users want to add custom metadata fields to topics or learnings (like `author`, `tags`, or `priority`), they would get validation errors. |
| 20 | +- **Suggestion**: Consider whether this strictness is intentional. If extensibility is desired, either remove `additionalProperties: False` or document clearly that custom fields are not supported. The jobs schema (`JOB_SCHEMA`) also uses `additionalProperties: False`, so this is consistent with existing patterns. |
| 21 | + |
| 22 | +### Issue 2 |
| 23 | +- **File**: `src/deepwork/core/experts_parser.py` |
| 24 | +- **Line(s)**: 377-379, 387-389 |
| 25 | +- **Severity**: Minor |
| 26 | +- **Issue**: The exception handling for topic/learning parsing re-raises `ExpertParseError` with a bare `raise`. This works but loses context about which file caused the error. The error message is already set in `parse_topic_file`/`parse_learning_file`, but the pattern is unusual. |
| 27 | +- **Suggestion**: Consider either removing the try/except entirely (letting errors propagate naturally) or adding file context to the error message. |
| 28 | + |
| 29 | +### Issue 3 |
| 30 | +- **File**: `src/deepwork/core/experts_generator.py` |
| 31 | +- **Line(s)**: 77-78 |
| 32 | +- **Severity**: Suggestion |
| 33 | +- **Issue**: The `_build_expert_context` method includes `topics_count` and `learnings_count` but these are not used in the template (`agent-expert.md.jinja`). This is dead code that could confuse future maintainers. |
| 34 | +- **Suggestion**: Either remove these unused context variables or add them to the template if there is a future use case (e.g., showing counts in agent description). |
| 35 | + |
| 36 | +### Issue 4 |
| 37 | +- **File**: `src/deepwork/templates/claude/agent-expert.md.jinja` |
| 38 | +- **Line(s)**: 14 |
| 39 | +- **Severity**: Minor |
| 40 | +- **Issue**: The `truncate(200)` filter is applied after `replace('\n', ' ')`, which means the 200-character limit includes any spaces that replaced newlines. A multiline description might get truncated unexpectedly short if it has many newlines. |
| 41 | +- **Suggestion**: This is likely acceptable behavior, but consider if `truncate(200, killwords=False, end='...')` would be better for cleaner truncation at word boundaries. |
| 42 | + |
| 43 | +### Issue 5 |
| 44 | +- **File**: `src/deepwork/cli/sync.py` |
| 45 | +- **Line(s)**: (expert agent generation section) |
| 46 | +- **Severity**: Minor |
| 47 | +- **Issue**: Expert agent generation is hardcoded to only run for Claude (`adapter.name == "claude"`). The comment says "agents live in .claude/agents/" but this should be abstracted through the adapter if/when other platforms support agents. |
| 48 | +- **Suggestion**: Consider adding an `adapter.supports_agents` property or similar abstraction so this check is not a string comparison. However, this is acceptable for now given Claude is the only platform with agent support. |
| 49 | + |
| 50 | +### Issue 6 |
| 51 | +- **File**: `src/deepwork/core/experts_parser.py` |
| 52 | +- **Line(s)**: 61 |
| 53 | +- **Severity**: Minor |
| 54 | +- **Issue**: The frontmatter regex pattern allows optional trailing content after the closing `---` on the same line, but then requires the closing `---` to be at the start of a line. This could cause parsing issues with edge cases. |
| 55 | +- **Suggestion**: The current pattern works correctly for well-formed files. Consider adding a test case for edge cases like trailing whitespace after the closing `---`. |
| 56 | + |
| 57 | +### Issue 7 |
| 58 | +- **File**: `src/deepwork/standard/experts/deepwork_jobs/expert.yml` |
| 59 | +- **Line(s)**: 151 |
| 60 | +- **Severity**: Suggestion |
| 61 | +- **Issue**: The expert documentation mentions "Claude Code currently only supports script hooks. Prompt hooks are parsed but not executed (documented limitation)." This is valuable but might become stale if prompt hooks are implemented. |
| 62 | +- **Suggestion**: The learning file `prompt_hooks_not_executed.md` already exists in learnings/ which is the right approach. Consider referencing it or adding a note about checking current platform capabilities. |
| 63 | + |
| 64 | +## Code Suggestions |
| 65 | + |
| 66 | +### Suggestion 1: Remove unused context variables |
| 67 | + |
| 68 | +**File**: `src/deepwork/core/experts_generator.py` |
| 69 | + |
| 70 | +Before: |
| 71 | +```python |
| 72 | +def _build_expert_context(self, expert: ExpertDefinition) -> dict: |
| 73 | + return { |
| 74 | + "expert_name": expert.name, |
| 75 | + "discovery_description": expert.discovery_description, |
| 76 | + "full_expertise": expert.full_expertise, |
| 77 | + "topics_count": len(expert.topics), |
| 78 | + "learnings_count": len(expert.learnings), |
| 79 | + } |
| 80 | +``` |
| 81 | + |
| 82 | +After: |
| 83 | +```python |
| 84 | +def _build_expert_context(self, expert: ExpertDefinition) -> dict: |
| 85 | + return { |
| 86 | + "expert_name": expert.name, |
| 87 | + "discovery_description": expert.discovery_description, |
| 88 | + "full_expertise": expert.full_expertise, |
| 89 | + } |
| 90 | +``` |
| 91 | + |
| 92 | +### Suggestion 2: Simplify redundant exception handling |
| 93 | + |
| 94 | +**File**: `src/deepwork/core/experts_parser.py` |
| 95 | + |
| 96 | +Before: |
| 97 | +```python |
| 98 | +for topic_file in topics_dir.glob("*.md"): |
| 99 | + try: |
| 100 | + topic = parse_topic_file(topic_file) |
| 101 | + topics.append(topic) |
| 102 | + except ExpertParseError: |
| 103 | + raise |
| 104 | +``` |
| 105 | + |
| 106 | +After: |
| 107 | +```python |
| 108 | +for topic_file in topics_dir.glob("*.md"): |
| 109 | + topic = parse_topic_file(topic_file) |
| 110 | + topics.append(topic) |
| 111 | +``` |
| 112 | + |
| 113 | +The try/except here adds no value since it just re-raises. The `parse_topic_file` function already includes the filename in its error messages. |
| 114 | + |
| 115 | +### Suggestion 3: Add platform abstraction for agent support |
| 116 | + |
| 117 | +**File**: `src/deepwork/core/adapters.py` (add property to `AgentAdapter` base class): |
| 118 | + |
| 119 | +```python |
| 120 | +@property |
| 121 | +def supports_agents(self) -> bool: |
| 122 | + """Whether this platform supports expert agents.""" |
| 123 | + return False |
| 124 | + |
| 125 | +# In ClaudeAdapter: |
| 126 | +@property |
| 127 | +def supports_agents(self) -> bool: |
| 128 | + return True |
| 129 | +``` |
| 130 | + |
| 131 | +Then in `sync.py`: |
| 132 | +```python |
| 133 | +if experts and adapter.supports_agents: |
| 134 | +``` |
| 135 | + |
| 136 | +## Approval Status |
| 137 | + |
| 138 | +**APPROVED**: No blocking issues |
| 139 | + |
| 140 | +The experts system is well-implemented and follows DeepWork's established patterns. The issues identified are minor improvements that do not block merging. The system correctly: |
| 141 | + |
| 142 | +1. Defines clear schemas for expert, topic, and learning validation |
| 143 | +2. Parses expert definitions including nested topics and learnings |
| 144 | +3. Generates agent files with dynamic command embedding for up-to-date content |
| 145 | +4. Integrates with the sync command to generate expert agents alongside job skills |
| 146 | +5. Provides CLI commands (`deepwork topics`, `deepwork learnings`) for dynamic content retrieval |
| 147 | +6. Has comprehensive test coverage across unit and integration tests |
| 148 | + |
| 149 | +The architecture cleanly separates the experts system from jobs while allowing them to coexist and complement each other. |
0 commit comments