feat(skills): plan template support for skill-driven planning [Phase 4.1]#67541
feat(skills): plan template support for skill-driven planning [Phase 4.1]#67541100yenadmin wants to merge 9 commits into
Conversation
Phase 4.1 of the GPT 5.4 parity sprint. Extends the skill metadata
schema with an optional planTemplate field that auto-populates
update_plan when the skill is activated.
## Schema extension (types.ts)
- New SkillPlanTemplateStep type: { step: string, activeForm?: string }
- New planTemplate?: SkillPlanTemplateStep[] field on OpenClawSkillMetadata
- Parsed from YAML frontmatter `plan-template` in SKILL.md
## Skill planner (skill-planner.ts)
- buildPlanTemplatePayload(): converts template to update_plan args
(all steps start as "pending")
- hasSkillPlanTemplate(): quick check for non-empty templates
- 7 unit tests
## Example frontmatter
```yaml
plan-template:
- step: "Run test suite"
activeForm: "Running test suite"
- step: "Build artifacts"
activeForm: "Building artifacts"
```
Note: the runtime wiring in skills-runtime.ts to auto-call
update_plan on activation follows once #67514 (task-system parity)
merges with the extended update_plan schema.
Tracking: #66345, #67522.
There was a problem hiding this comment.
Pull request overview
Adds initial scaffolding for skill-driven plan templates by introducing metadata/types and helper functions to build an update_plan payload from a skill-provided template.
Changes:
- Added
SkillPlanTemplateStepandplanTemplateto skill metadata types. - Introduced
skill-planner.tswith helpers to detect and build a plan-templateupdate_planpayload. - Added unit tests for the new planner helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/agents/skills/types.ts |
Extends skill metadata types to include an optional plan template field. |
src/agents/skills/skill-planner.ts |
Adds template-to-update_plan payload builder and a helper predicate. |
src/agents/skills/skill-planner.test.ts |
Adds Vitest coverage for the new planner helpers. |
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — all remaining findings are P2 (misleading docstring, dead null-guard, undocumented activeForm drop in plan events). The PR has been through multiple hardening iterations; all prior P0/P1 issues are resolved. The three remaining issues are documentation/style concerns that don't affect runtime behavior. src/agents/pi-embedded-runner/skills-runtime.ts — three P2 style/doc issues noted inline. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/skills-runtime.ts
Line: 105-112
Comment:
**Docstring contradicts implementation — function does sort internally**
The JSDoc says "Candidates MUST be alpha-sorted by `skillName` if the caller wants deterministic collision behavior; this function does not re-sort." But the implementation immediately calls `.toSorted()` on the filtered list, so the pre-sort requirement is unnecessary and the "does not re-sort" claim is incorrect. The snapshot-backed call at line 209 passes `resolvedPlanTemplates` (insertion-ordered, not pre-sorted), and it still produces a deterministic result because the internal sort handles it.
```suggestion
/**
* Lower-level resolver that operates on the snapshot's
* `resolvedPlanTemplates` shape — name + template list, without the
* full SkillEntry. Used in the snapshot-backed run path where
* `resolveEmbeddedRunSkillEntries` returns no entries.
*
* Sorts candidates alpha-ascending by `skillName` before resolving,
* so insertion order does not affect the collision winner.
*/
export function resolveSkillPlanTemplateFromCandidates(
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/skills-runtime.ts
Line: 81-93
Comment:
**`winnerTemplate` is computed and checked but never used**
`winnerTemplate` is extracted from `candidates[0]`, guarded for null, but then never passed to `resolveSkillPlanTemplateFromCandidates` — the function maps all candidates independently. Because `candidates` has already been filtered through `hasSkillPlanTemplate`, `winner.metadata?.planTemplate` is always a non-empty array at this point, so the guard can never fire. The three intermediate lines are dead code that can be removed.
```suggestion
return resolveSkillPlanTemplateFromCandidates(
candidates.map((c) => ({
skillName: c.skill.name,
planTemplate: c.metadata?.planTemplate ?? [],
})),
config,
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/skills-runtime.ts
Line: 236-243
Comment:
**`activeForm` values are silently dropped when building the plan event**
`payload.plan` entries carry `step`, `status`, and `activeForm?`, but when building `planEventData` only `.step` is mapped — `activeForm` is never included. `AgentPlanEventData.steps` is `string[]`, so there is currently no slot for it. Consumers of `onAgentEvent` and `emitAgentPlanEvent` (including the auto-reply pipeline) will never see the `activeForm` values from the skill template.
If `AgentPlanEventData` is intended to stay as `string[]` steps (with `activeForm` conveyed only via the future `update_plan` tool call), a brief comment here stating that would prevent future readers from believing the field is accidentally dropped.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(skills): #67541 iter-2 — camelCase p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add parsePlanTemplate() helper in frontmatter.ts to parse the plan-template YAML field into SkillPlanTemplateStep[] - Include planTemplate in resolveOpenClawMetadata output - Add resolveSkillPlanTemplate() in skills-runtime.ts that scans activated skill entries and returns an update_plan payload via buildPlanTemplatePayload when a plan template is found
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4e31dc5f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Merge order: 7/8. See #66345 for the full merge sequence. |
Review finding: resolveSkillPlanTemplate() scans loaded entries (SkillEntry[]), not activated entries. The docstring was misleading.
|
@copilot please re-review — fix pushed: docstring fix activated→loaded (adbb4e3). |
Plan-mode rollout series — status updateThis PR is part of a 10-PR plan-mode rollout. The cumulative state ships locally as Series overview + landing order: see #68441 (latest comment). This PR's role in the series: see the original PR description above. Live test status: covered by the cumulative install — all behaviors from this PR are verified in production-like usage on Telegram + webchat. No regressions detected since the live install (2026-04-18). |
📋 Architecture & Status — single source of truth (commit
|
|
To use Codex here, create an environment for this repo. |
| /** | ||
| * Builds an `update_plan` payload from a skill's plan template. | ||
| * | ||
| * Normalizes the template by: | ||
| * - Dropping entries with duplicate `step` text (first wins). | ||
| * - Truncating to `maxSteps` (default 50, configurable). | ||
| * | ||
| * Diagnostic fields (`droppedDuplicates`, `truncated`, `maxSteps`) on the | ||
| * returned payload let the caller emit per-skill warning events without | ||
| * needing access to the original template. | ||
| * | ||
| * @param skillName - The name of the skill being activated | ||
| * @param template - The plan template steps from skill metadata | ||
| * @param options - Optional limits/overrides | ||
| * @returns A payload suitable for passing to the `update_plan` tool, | ||
| * or `null` if the (post-normalize) template is empty | ||
| */ |
There was a problem hiding this comment.
buildPlanTemplatePayload() is documented/typed as producing an update_plan tool payload, but the returned object includes droppedDuplicates, truncated, and maxSteps, which are not part of the update_plan tool’s top-level input schema (src/agents/tools/update-plan-tool.ts). If this object is ever passed directly to the tool, schema validation may reject it. Consider separating diagnostics from the tool args (e.g. { toolArgs: { plan, explanation }, diagnostics: ... }) or renaming the type/function so callers don’t treat it as a valid tool input.
|
Closing in favor of consolidated PR #68939. Rationale: this branch was 734 commits behind upstream/main and the The full iteration history + decision log lives in |
TL;DR
Skills can now define a `planTemplate` in their frontmatter that auto-populates `update_plan` on activation, giving users a pre-filled checklist when they invoke a skill.
Tracking
What this PR does
Example skill frontmatter
```yaml
planTemplate:
activeForm: "Reading implementation"
activeForm: "Analyzing code"
activeForm: "Applying changes"
```
Files changed
Dependencies
What follows