feat(skill-guidance): add-skill-guidance#964
feat(skill-guidance): add-skill-guidance#964duizendnegen wants to merge 3 commits intoFission-AI:mainfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a proposal for "skill guidance": a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "openspec CLI"
participant Config as "project config.yaml"
participant Guidance as "guidance subcommand"
participant Workflow as "Seeded Workflow Step"
User->>CLI: run workflow (e.g., explore)
CLI->>Config: load project context and `skills` map
CLI->>Guidance: execute `openspec guidance <skill> --json`
Guidance-->>CLI: return JSON (context + instructions or null)
CLI->>Workflow: provide guidance output
alt guidance returned and parsed
Workflow->>Workflow: apply non-null guidance parts
else guidance missing/unparseable/error
Workflow->>Workflow: emit warning and continue without guidance
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
openspec/changes/add-skill-guidance/proposal.md (2)
7-7: Clarify theskillfield in the return structure.The command returns
{ skill, context, instructions }, but it's not clear what theskillfield contains. Is it an echo of the input skill name, a validated/normalized name, or something else?Consider adding a brief clarification to make the contract more explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/add-skill-guidance/proposal.md` at line 7, Clarify the "skill" field in the "openspec guidance" command contract: specify that the returned object { skill, context, instructions } should contain a canonical, validated skill identifier (not just a raw echo), e.g., normalized name/slug derived from the input (or null/empty if no matching skill exists), and document how unknown inputs are handled (error vs fallback). Update the proposal text for the `openspec guidance <skill-name> [--json]` command to state this exact behavior and the normalization/validation rules so callers know whether `skill` is an echo, a normalized name, or absent.
18-20: Consider elaborating on thecontextvsinstructionsdistinction.You flagged this as a design decision needing alignment in the PR objectives. The proposal mentions:
context= binding project constraints applied throughout the session (not written to documents)instructions= workflow-specific overrides/extensionsWhile the distinction is stated, the proposal could benefit from examples showing when teams would use each field:
- context: Perhaps project-wide constraints like "Always use TypeScript strict mode" or "Follow Google Style Guide"
- instructions: Perhaps skill-specific overrides like "In explore mode, always create a decision log" or "In propose mode, include cost estimates"
Adding examples would help users understand the intended usage pattern and make the design decision clearer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/add-skill-guidance/proposal.md` around lines 18 - 20, Update the proposal text for `skill-guidance-command`, `explore-skill-workflow`, and `propose-skill-workflow` to clearly illustrate the `context` vs `instructions` distinction by adding concrete examples: show `context` as persistent, project-level constraints (e.g., "Always use TypeScript strict mode", "Follow Google Style Guide") and `instructions` as session/workflow overrides (e.g., for `explore-skill-workflow`: "always create a decision log"; for `propose-skill-workflow`: "include cost estimates" and "apply OpenSpec branding in templates"); ensure you mention that `context` is not written to exploration documents while `instructions` can modify per-session behavior and include at least one example for each of the three symbols (`skill-guidance-command`, `explore-skill-workflow`, `propose-skill-workflow`) to make usage patterns explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/add-skill-guidance/proposal.md`:
- Line 9: Update the guidance Step 1 behavior for the seeded workflow skills
(explore, propose, apply, archive, verify, onboard, continue, ff-change) that
call "openspec guidance <skill-name> --json": treat explicit failures (non-zero
exit, stdout parse/JSON error, missing file/command not found) as logged
warnings including error details and proceed without applying any guidance; when
the command succeeds but returns null for one of the fields, apply only the
non-null field(s) (i.e., merge returned "context" into project bindings if
present and apply/override "instructions" only if present) and log an
informational message noting which field(s) were applied versus ignored; ensure
the logging includes the command, skill name, and error/parse details to aid
debugging.
- Line 20: The proposal's mention of "requirements for OpenSpec branding in
generated templates" is likely unrelated to the `propose-skill-workflow`
guidance change and should be removed or clarified; update the
`propose-skill-workflow` description to either delete the branding clause or
explicitly state the rationale and linkage to guidance (e.g., why
CLI/display-name/skill-metadata branding is required by the guidance step), and
ensure the "What Changes" section and any capability docs for
`propose-skill-workflow` consistently reflect that decision.
- Line 8: The proposal currently accepts the `skills:` map in config.yaml
without validation so misspelled keys silently drop; change the config
loading/validation logic (where `skills:` is parsed—e.g., the config loader or
parseConfig/loadConfig function) to check each key against an allowlist of known
skill names and emit a non-fatal warning for any unrecognized key rather than
rejecting it; use the recommended known names (`verify-change`, `sync-specs`,
`propose`, `onboard`, `new-change`, `explore`, `continue-change`,
`bulk-archive-change`, `ff-change`, `archive-change`, `apply-change`,
`feedback`) in the allowlist, but still accept unknown keys (no error) so
forward compatibility is preserved.
- Around line 41-44: The proposal is inconsistent about which workflow specs are
being updated; update openspec/changes/add-skill-guidance/proposal.md to
explicitly state whether the other six workflows (apply, archive, verify,
onboard, continue, ff-change) have existing specs that require no change or
require new/delta specs. Either (A) add the missing spec entries under "Updated
specs" and Impact to include openspec/specs/apply-skill-workflow/spec.md,
openspec/specs/archive-skill-workflow/spec.md,
openspec/specs/verify-skill-workflow/spec.md,
openspec/specs/onboard-skill-workflow/spec.md,
openspec/specs/continue-skill-workflow/spec.md,
openspec/specs/ff-change-skill-workflow/spec.md (mark new or delta
appropriately), or (B) add a brief clarifying note referencing those six files
explaining they already conform and need no changes; ensure the file
openspec/specs/config-loading/spec.md and the two listed new specs still appear
as shown.
---
Nitpick comments:
In `@openspec/changes/add-skill-guidance/proposal.md`:
- Line 7: Clarify the "skill" field in the "openspec guidance" command contract:
specify that the returned object { skill, context, instructions } should contain
a canonical, validated skill identifier (not just a raw echo), e.g., normalized
name/slug derived from the input (or null/empty if no matching skill exists),
and document how unknown inputs are handled (error vs fallback). Update the
proposal text for the `openspec guidance <skill-name> [--json]` command to state
this exact behavior and the normalization/validation rules so callers know
whether `skill` is an echo, a normalized name, or absent.
- Around line 18-20: Update the proposal text for `skill-guidance-command`,
`explore-skill-workflow`, and `propose-skill-workflow` to clearly illustrate the
`context` vs `instructions` distinction by adding concrete examples: show
`context` as persistent, project-level constraints (e.g., "Always use TypeScript
strict mode", "Follow Google Style Guide") and `instructions` as
session/workflow overrides (e.g., for `explore-skill-workflow`: "always create a
decision log"; for `propose-skill-workflow`: "include cost estimates" and "apply
OpenSpec branding in templates"); ensure you mention that `context` is not
written to exploration documents while `instructions` can modify per-session
behavior and include at least one example for each of the three symbols
(`skill-guidance-command`, `explore-skill-workflow`, `propose-skill-workflow`)
to make usage patterns explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee318d82-9f00-4f7c-902c-6c9fcb7e2290
📒 Files selected for processing (2)
openspec/changes/add-skill-guidance/.openspec.yamlopenspec/changes/add-skill-guidance/proposal.md
- Clarify failure/null handling: explicit failures log a warning, partial null fields silently apply non-null fields only - Remove allowlist warning for unknown skills: keys - Drop branding requirements from propose-skill-workflow capability - Use short skill names (apply, archive, verify, continue, ff-change) - Add concrete examples for context vs instructions distinction - Clarify skill field is an echo of the input argument Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All 8 workflow skills get guidance specs: new specs for apply, continue, and ff-change; delta specs for archive, verify, and onboard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
duizendnegen
changed the title
|
I like this idea, hope it will be implemented. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing this. The user need is real, but I would not merge the proposal in its current shape.
The main issue is architectural: this creates a second config-to-prompt injection path next to the existing project config plus instructions flow. Right now context and rules already enter the system through shared loaders and instruction generation. Adding openspec guidance <skill> plus a mandatory Step 1 shell-out in every seeded workflow spreads the behavior across prompt text, JSON parsing, and template drift instead of keeping it in one load-bearing place. With how duplication-heavy core/templates already is, that blast radius worries me more than the new config key itself.
I think the reusable idea here is workflow-level project overrides, but I would route it through the existing instructions / instruction-loader surface rather than a new prompt-local command. In practice that probably means one shared loader in core that can return project context plus workflow-specific overlays, then letting explore/propose/apply consume that through the same instruction contract the rest of the system already leans on.
A few specific concerns I would want resolved before this moves forward:
skills:as a free-form string map is fine as an escape hatch, but it is a weak long-term API for something this central. If we want this, I would prefer naming it around workflows or overlays rather than overloadingskills.- Echoing the raw
skillinput back from the command feels like a shaky contract. Callers usually need canonical workflow identity, not just the same string they passed in. - Embedding a new Step 1 in every seeded template makes update churn and prompt drift worse. I would rather keep as much of this as possible in shared code and generated instruction payloads.
So my vote is: good problem, wrong primary mechanism. I would reframe this around extending the existing project-config/instructions pipeline with workflow-specific overlays, then only add a new CLI surface if that shared path still needs a thin public wrapper.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for iterating on this. My view is still that the problem is real, but the primary mechanism is wrong for OpenSpec's current architecture.
Today project context already flows through shared code via openspec/config.yaml + readProjectConfig() + instruction-loader, and that keeps the config-to-instruction path centralized. Adding openspec guidance <skill> --json plus a new Step 1 shell-out in every seeded workflow would create a second injection path that lives partly in prompt text, partly in CLI parsing, and partly in config loading. That increases template drift for a feature that really wants one load-bearing implementation point.
If we want skill or workflow-specific overrides, I'd rather extend the existing project-config / instruction-loader surface with workflow overlays, then let the templates consume the same shared instruction contract they already depend on. A thin public guidance command could still exist later as a wrapper, but I would not make prompt-local shell execution the core design.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Good problem, but I still think this proposal picks the wrong load-bearing mechanism for the current codebase. I would keep workflow-specific overrides inside the existing project-config and instruction-loader path, then add a thin public wrapper later if needed, rather than hardwiring a new prompt-local openspec guidance Step 1 into every seeded workflow.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
I re-read this against the current config and template architecture, and my core concern is still the same: the problem is real, but openspec guidance <skill> --json plus a new Step 1 shell-out in every seeded workflow is the wrong load-bearing mechanism.
OpenSpec already has one centralized config-to-instruction path through project config parsing and the shared instruction-loading layer. This proposal adds a second injection path that lives partly in CLI plumbing and partly in prompt text, which makes template drift worse in the exact part of the codebase that is already duplication-heavy.
If we want workflow-specific project overrides, I would route them through the existing project-config / instruction-loader surface first, then add a thin public wrapper later only if that shared path still needs it. My recommendation is still comment, not merge as written.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing on this. I re-read it against the current config and workflow architecture, and my take is still: real user problem, wrong load-bearing mechanism.
Today project-level guidance already has one centralized path through openspec/config.yaml parsing plus the shared instruction-loading layer. This proposal would add a second config-to-prompt injection path via openspec guidance <skill> --json and a new Step 1 shell-out inside every seeded workflow. That spreads the behavior across CLI plumbing, prompt text, and template regeneration in exactly the part of the codebase that is already most duplication-heavy.
If we want project-specific workflow overrides, I would rather extend the existing project-config / instruction-loader contract with workflow overlays, then let explore / propose / apply consume that shared payload the same way they already depend on shared instructions today. A thin public guidance command could still exist later as a wrapper, but I would not make prompt-local shell execution the core design.
So my recommendation is still comment, not merge as written.
3 participants
This PR submits a change proposal for skill-level project guidance — a mechanism for teams to inject project-specific conventions into OpenSpec workflow skill sessions (explore, propose, apply, etc.) via
openspec/config.yaml, without editing seeded files that get overwritten onopenspec update.The proposal covers:
openspec guidance <skill-name> [--json]CLI commandskills:key inopenspec/config.yamlmapping skill names to instruction stringsSeeking for alignment on the approach. Especially the
contextvsinstructionsrole distinction and the decision to embed the guidance step directly in the seeded templates.Summary by CodeRabbit
New Features
Documentation
Chores