refactor: split skill templates into workflow modules#698
Conversation
|
Task completed. PR Review: refactor: split skill templates into workflow modules (#698)Overall AssessmentThis is a clean decomposition of a ~3,500 line monolithic file into 11 focused workflow modules. The approach is sound — keeping Issue: Missing exports in
|
| Category | Finding |
|---|---|
| Bug | index.ts missing 3 exports + type re-exports vs the facade |
| Suggestion | Replace generic boilerplate comments with file-specific ones |
| Suggestion | Consider workflows/index.ts barrel file |
| Suggestion | Add hash regeneration instructions to parity test |
| Good | onboard.ts shared helper avoids duplication |
| Good | Parity test with stable hashing is solid |
| Good | Facade pattern preserves backward compatibility |
Powered by 1Code
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces core template types and splits a monolithic templates module into many workflow-specific template files, adding per-workflow SkillTemplate/CommandTemplate factory functions, switching the templates facade to a wildcard re-export, and adding parity tests and OpenSpec design/docs for a unified template-generation pipeline. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Greptile OverviewGreptile SummaryThis PR splits the monolithic Key changes
Issues found
Benefits
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Consumer
participant index.ts
participant skill-templates.ts
participant workflows/
Note over Consumer,workflows/: Before: Monolithic File
Consumer->>skill-templates.ts: import { getExploreSkillTemplate }
skill-templates.ts-->>Consumer: return template (3500+ lines)
Note over Consumer,workflows/: After: Modular Architecture
Consumer->>skill-templates.ts: import { getExploreSkillTemplate }
skill-templates.ts->>workflows/: re-export from workflows/explore.js
workflows/-->>skill-templates.ts: template function
skill-templates.ts-->>Consumer: return template
Note over Consumer,index.ts: Alternative Import Path
Consumer->>index.ts: import { getExploreSkillTemplate }
index.ts->>skill-templates.ts: re-export
skill-templates.ts->>workflows/: re-export
workflows/-->>Consumer: template function
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/core/templates/workflows/feedback.ts`:
- Around line 9-111: The getFeedbackSkillTemplate SkillTemplate factory is
missing the standard fields present in other templates (license, compatibility,
metadata); update the object returned by getFeedbackSkillTemplate to include
license: 'MIT', compatibility: 'Requires openspec CLI.', and metadata: { author:
'openspec', version: '1.0' } so it matches
getOnboardSkillTemplate/getArchiveChangeSkillTemplate/getFfChangeSkillTemplate
conventions and remember this will change the
EXPECTED_FUNCTION_HASHES.getFeedbackSkillTemplate parity test.
In `@src/core/templates/workflows/ff-change.ts`:
- Around line 189-201: The command template's "Artifact Creation Guidelines" in
ff-change.ts is missing the guardrail that `context` and `rules` are constraints
and must not be copied into artifact files; add the same sentence used in the
skill template ("`context` and `rules` are constraints for YOU, not content for
the file — Do NOT copy `<context>`, `<rules>`, `<project_context>` blocks into
the artifact.") into the Artifact Creation Guidelines section of the command
template so it matches the guardrail present in continue-change.ts and the skill
template.
In `@test/core/templates/skill-templates-parity.test.ts`:
- Around line 120-142: Add a brief inline comment above the skillFactories array
explaining that getFeedbackSkillTemplate is intentionally excluded from this
generated-content parity test because feedback is not deployed as a generated
skill file (it is covered in the function-payload parity test); mention the
symbols involved (getFeedbackSkillTemplate, skillFactories,
generateSkillContent) so future readers understand the deliberate omission.
🧹 Nitpick comments (3)
src/core/templates/workflows/explore.ts (1)
298-472: Substantial content duplication betweengetExploreSkillTemplateandgetOpsxExploreCommandTemplate.The
contentfield in the command template is ~90% identical to theinstructionsfield in the skill template. Theonboard.tsmodule demonstrates a cleaner pattern using a sharedgetOnboardInstructions()helper to avoid this. Consider extracting shared prose into a helper and interpolating the small differences (e.g., the additional "Input" section in the command template).This isn't blocking since the parity test locks the output, but it increases maintenance burden for future content changes across all workflow modules that follow this pattern.
src/core/templates/workflows/continue-change.ts (2)
1-6: Nit: Header comment is misleading for ongoing maintenance.The comment says "This file is generated by splitting the legacy monolithic templates file" — this reads as if it's auto-generated. If these modules will be hand-maintained going forward, consider rewording to avoid confusion (e.g., "Extracted from the legacy monolithic templates file"). This same comment appears in all workflow files.
9-239: Significant content duplication betweenSkillTemplate.instructionsandCommandTemplate.content.The
instructionsfield (Lines 13–119) and thecontentfield (Lines 132–238) are ~95% identical — roughly 100+ lines of duplicated markdown with only minor differences (input format mentioning/opsx:continue, output prompt wording, and the "all complete" suggestion). This pattern repeats across every workflow file in this PR.Consider extracting the shared instruction body into a helper and composing the small per-variant deltas. For example:
♻️ Sketch of a shared-content approach
// In a shared helper, e.g., src/core/templates/workflows/_continue-change-content.ts function baseContinueInstructions(opts: { inputLine: string; allCompleteMsg: string; outputPrompt: string; }): string { return `Continue working on a change by creating the next artifact. ${opts.inputLine} **Steps** ...shared steps... **If all artifacts are complete (\`isComplete: true\`)**: ... - Suggest: "${opts.allCompleteMsg}" ... **Output** ... - Prompt: "${opts.outputPrompt}" ...`; }This would make future instruction edits less error-prone and reduce the risk of the two variants drifting apart. The same pattern applies to every other workflow module in this PR.
|
Hey @TabishB, great refactoring work here. I wanted to raise something that connects this PR with the discussion happening on #701 (lifecycle hooks). Right now, all 10 skill templates generate identical instructions regardless of the target agent. This works today, but it breaks down when you consider that each agent defines its own way of handling hooks, tools, and workflows. Claude Code has PreToolUse/PostToolUse/Stop, Cursor has its own system, Windsurf another, and many agents have no hook system at all. There's no shared capability model — each agent is its own world. Proposal: Agent Skill Profiles Would you consider designing these workflow modules with an agent-based profile system in mind? The idea: The current set of workflow modules becomes the default profile — the universal, agent-agnostic baseline that works for any agent. Specific agents can define their own agent profile that overrides or extends individual workflow modules to leverage agent-specific features (native hooks, custom tool integrations, etc.). Resolution is simple: agent-specific module if it exists, otherwise fall back to default Concretely: Each agent 's config would map to a profile ID (or 'default'), and the skill generation in update.ts would resolve templates through the profile chain before generating content. This doesn't need to be implemented now — I'm just suggesting that the module boundaries you're creating here are the perfect foundation for this pattern. The factory functions per workflow module are exactly the right seam. If designed with this extensibility in mind (e.g., factory functions that accept an optional agent context), it would make the evolution much smoother. What do you think? |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/core/templates/skill-templates-parity.test.ts (2)
67-81:stableStringifyreturnsundefined(not a string) forundefinedvalues.
JSON.stringify(undefined)returns JSundefined, not the string"undefined". If a template property ever holdsundefined, the hash would still be computed (via implicit coercion in string concatenation), but the representation wouldn't be distinguishable from the literal stringundefinedor from a missing key scenario. Low risk for current template shapes, but worth a defensive guard.🛡️ Optional defensive fix
return JSON.stringify(value);could become:
+ if (value === undefined) return '"__undefined__"'; return JSON.stringify(value);
30-65: Consider adding a brief comment explaining how to update hashes after intentional template changes.When a developer intentionally modifies a template, these tests will fail with opaque SHA-256 mismatches. A short comment (e.g., "Run the test with
--updateor copy actual hashes from the failure output") would reduce friction and avoid confusion about whether the failure signals a regression or an expected change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/templates/skill-templates-parity.test.ts (1)
30-52: Consider adding a safeguard against silently missing new templates.If a new template factory is added in the future but not included in
functionFactories, the parity test will still pass — it just won't cover the new template. A complementary assertion that the count of exported factories matchesfunctionFactorieslength would catch this.Example assertion
+import * as allTemplates from '../../../src/core/templates/skill-templates.js'; + +// Ensure no factory is silently omitted from parity checks +const exportedFactories = Object.entries(allTemplates).filter( + ([key, val]) => typeof val === 'function' && key.startsWith('get') +); +expect(Object.keys(functionFactories).length).toBe(exportedFactories.length);
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openspec/changes/unify-template-generation-pipeline/specs/template-artifact-pipeline/spec.md (1)
24-24: Optional: Consider more concise wording.The phrase "in a consistent way" could be simplified to "consistently" for brevity, though the current wording is clear and acceptable.
✍️ Suggested simplification
- - **AND** generation SHALL use those values or explicit defaults in a consistent way for all workflows + - **AND** generation SHALL use those values or explicit defaults consistently for all workflows
|
Agent-specific behavior is definitely on our radar. This refactor was partly about creating the right module boundaries for exactly this kind of extensibility. We're exploring a pattern that uses a transform pipeline + tool profile registry rather than full template copies per agent, keeps the maintenance burden manageable while still allowing agent-specific optimizations. Early days, but the foundation is here now. |
2 participants
Summary
Validation
Summary by CodeRabbit
New Features
Tests
Documentation
Chores