fix: unify tool command invocation rendering across generation paths#740
fix: unify tool command invocation rendering across generation paths#740
Conversation
|
Task completed. PR Review: fix: unify tool command invocation rendering across generation pathsOverall AssessmentThis is a well-structured PR that addresses a real consistency problem (issue #727). The approach of centralizing command invocation style handling into explicit tool metadata is sound and sets up a clean path toward the future Architecture & Design (Positive)
Issues & Suggestions1. Getting-started column alignment will break for longer command names
console.log(` ${formatCommandInvocation('new', commandStyle)} Start a new change`);
console.log(` ${formatCommandInvocation('continue', commandStyle)} Create the next artifact`);
console.log(` ${formatCommandInvocation('apply', commandStyle)} Implement tasks`);For const commands = [
{ id: 'new', label: 'Start a new change' },
{ id: 'continue', label: 'Create the next artifact' },
{ id: 'apply', label: 'Implement tasks' },
];
const formatted = commands.map(c => formatCommandInvocation(c.id, commandStyle));
const maxLen = Math.max(...formatted.map(s => s.length));
commands.forEach((c, i) => {
console.log(` ${formatted[i].padEnd(maxLen + 2)}${c.label}`);
});The same issue applies to 2. Regex uses global flag on a module-level
|
📝 WalkthroughWalkthroughThis pull request introduces explicit tool-specific rendering metadata to the template generation pipeline, including command surface patterns, terminology definitions, and verification flags. It extends the transformation pipeline with token-aware rendering and adds fallback behavior for unverified tool command surfaces, updating implementation tasks and validation test plans accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 SummaryCentralizes command invocation style handling by moving transformation logic from adapter-specific code into a shared generation pipeline. Key Changes:
Impact: Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Workflow Templates] --> B[Generate Skill Content]
A --> C[Generate Command Content]
B --> D{Tool-specific<br/>transformation}
C --> D
D --> E[command-invocation-style.ts]
E --> F{Get Tool Style}
F --> G[opsx-colon<br/>Claude]
F --> H[opsx-hyphen<br/>Codex, Cursor, etc]
F --> I[openspec-hyphen<br/>Trae]
G --> J[No Transform]
H --> K[Transform References<br/>/opsx:X → /opsx-X]
I --> L[Transform References<br/>/opsx:X → /openspec-X-Y]
J --> M[Tool Adapter]
K --> M
L --> M
M --> N[Write Skill Files]
M --> O[Write Command Files]
style E fill:#e1f5ff
style F fill:#fff4e1
style M fill:#e8f5e9
Last reviewed commit: c9bc915 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/config.ts (1)
17-19:commandReferenceStylefield lacks a doc comment explaining the default.
skillsDirdocuments itself inline;commandReferenceStyledoes not. The implicit default (opsx-colonwhen absent) is buried incommand-invocation-style.ts. A brief comment would make the field self-documenting:✏️ Suggested doc comment
skillsDir?: string; // e.g., '.claude' - /skills suffix per Agent Skills spec + /** Command reference style for this tool. Defaults to 'opsx-colon' when absent. */ commandReferenceStyle?: 'opsx-colon' | 'opsx-hyphen' | 'openspec-hyphen';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/config.ts` around lines 17 - 19, Add a brief doc comment to the commandReferenceStyle field in the Config type explaining what the options mean and that the default behavior is 'opsx-colon' when the field is omitted; update the comment adjacent to the commandReferenceStyle declaration in src/core/config.ts (the Config interface/type) and, if helpful, mention that the implicit default is implemented in command-invocation-style.ts so readers can find the runtime behavior.test/core/command-generation/adapters.test.ts (1)
458-481: New tests correctly verify the adapter's pass-through contract; minor clarity nit.The tests accurately document that
opencodeAdapter.formatFileno longer transforms command references internally. One thing that could confuse future readers: in production, content reaching this adapter is already inopsx-hyphenformat (transformed upstream), but the tests pass colon-style input. A brief inline comment clarifying "transformation happens upstream; adapter is format-agnostic" would remove that ambiguity.💬 Suggested clarifying comment
describe('opencodeAdapter', () => { + // NOTE: In production, content is pre-transformed to opsx-hyphen style before + // reaching this adapter. These tests use colon-style input to verify the adapter + // itself is format-agnostic (no internal rewriting). it('should have correct toolId', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/command-generation/adapters.test.ts` around lines 458 - 481, Add a short inline comment above the tests that exercise opencodeAdapter.formatFile (the two it blocks that use CommandContent and sampleContent) clarifying that command-format transformation (colon -> hyphen, e.g., opsx:new -> opsx-new) is performed upstream and that the adapter intentionally preserves input verbatim; reference opencodeAdapter.formatFile and the test cases so future readers understand these are pass-through behavior tests, not a failure to transform.openspec/changes/unify-template-generation-pipeline/design.md (1)
57-65:commandReferenceStyleis required inToolProfilebut optional inAIToolOption— reconcile before implementing task 2.2.When
ToolProfileRegistryis implemented, all tool entries must supply an explicitcommandReferenceStyle. Tools currently without the field inAI_TOOLS(claude, codebuddy, crush, gemini, qoder) rely on the implicitopsx-colondefault incommand-invocation-style.ts. Consider either:
- Making the field optional in
ToolProfilewith a documented default, or- Explicitly enumerating
opsx-colonfor those tools when building the registry entries.Either choice is fine, but the mismatch should be a deliberate decision documented in the registry implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/unify-template-generation-pipeline/design.md` around lines 57 - 65, ToolProfile declares commandReferenceStyle as required while AIToolOption treats it as optional, causing a mismatch; update the registry implementation (ToolProfileRegistry / AI_TOOLS builder) to reconcile this by either making ToolProfile.commandReferenceStyle optional and documenting the default 'opsx-colon' (referenced in command-invocation-style.ts) or by explicitly adding commandReferenceStyle: 'opsx-colon' to every tool entry missing it (e.g., claude, codebuddy, crush, gemini, qoder) when constructing AI_TOOLS; apply one consistent approach and add a short comment in the registry explaining the chosen decision.src/core/update.ts (1)
195-203: Hoisttransformeroutside the innerskillTemplatesloop.
getToolCommandReferenceTransformer(tool.value)is invariant across templates — it only depends ontool.value. Moving it before theforloop avoids redundant lookups per template.The same pattern applies at Line 594 in
upgradeLegacyTools.Suggested diff
if (shouldGenerateSkills) { + const transformer = getToolCommandReferenceTransformer(tool.value); for (const { template, dirName } of skillTemplates) { const skillDir = path.join(skillsDir, dirName); const skillFile = path.join(skillDir, 'SKILL.md'); - const transformer = getToolCommandReferenceTransformer(tool.value); const skillContent = generateSkillContent(template, OPENSPEC_VERSION, transformer); await FileSystemUtils.writeFile(skillFile, skillContent); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 195 - 203, The code repeatedly calls getToolCommandReferenceTransformer(tool.value) inside the skillTemplates loop; hoist this call into the outer shouldGenerateSkills branch (e.g., compute const transformer = getToolCommandReferenceTransformer(tool.value) before iterating skillTemplates) and then pass that transformer into generateSkillContent; apply the same refactor in the upgradeLegacyTools function where getToolCommandReferenceTransformer is called per-template so you compute it once per tool and reuse it for all template iterations (references: shouldGenerateSkills, skillTemplates, getToolCommandReferenceTransformer, generateSkillContent, upgradeLegacyTools).test/core/command-invocation-style.test.ts (1)
1-49: Tests cover the core paths well.The four tests validate tool→style resolution, per-tool body transformation for both
opsx-hyphenandopenspec-hyphen, and the consensus logic. Assertions align with the config data and transformation logic.Consider adding edge-case tests for an empty tool array and an unknown tool ID to fully cover the fallback behavior of
getDisplayCommandReferenceStyleandgetToolCommandReferenceStyle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/command-invocation-style.test.ts` around lines 1 - 49, Add two tests covering fallback behavior: one calling getDisplayCommandReferenceStyle with an empty array and asserting it returns the default display style (verify expected default value), and one calling getToolCommandReferenceStyle with an unknown tool id (e.g., 'unknown-tool') and asserting it returns the configured fallback style; place both tests alongside existing ones in command-invocation-style.test.ts to ensure the functions' fallback logic is exercised.src/core/init.ts (1)
545-555: Same transformer hoisting opportunity as noted inupdate.ts.
getToolCommandReferenceTransformer(tool.value)at Line 550 is invariant within theskillTemplatesloop and could be computed once before the loop.Suggested diff
if (shouldGenerateSkills) { // Use tool-specific skillsDir const skillsDir = path.join(projectPath, tool.skillsDir, 'skills'); // Create skill directories and SKILL.md files + const transformer = getToolCommandReferenceTransformer(tool.value); for (const { template, dirName } of skillTemplates) { const skillDir = path.join(skillsDir, dirName); const skillFile = path.join(skillDir, 'SKILL.md'); // Generate SKILL.md content with YAML frontmatter including generatedBy - const transformer = getToolCommandReferenceTransformer(tool.value); const skillContent = generateSkillContent(template, OPENSPEC_VERSION, transformer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/init.ts` around lines 545 - 555, The call to getToolCommandReferenceTransformer(tool.value) is invariant inside the loop over skillTemplates and should be hoisted out to avoid repeated computation; compute const transformer = getToolCommandReferenceTransformer(tool.value) once before the for (const { template, dirName } of skillTemplates) loop and then use that transformer variable inside the loop when calling generateSkillContent(template, OPENSPEC_VERSION, transformer), keeping the rest of the SKILL.md generation logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/core/init.test.ts`:
- Around line 358-367: The test for InitCommand's Trae rewrite is implicitly
coupled to the openspec-propose template content so it can break silently;
update the test (the it block that instantiates InitCommand and reads SKILL.md)
to either (a) load/use a controlled fixture/template that contains a known
'/opsx:apply' reference and assert it becomes '/openspec-apply-change', or (b)
add a clear comment and explicitly write a minimal SKILL.md with '/opsx:apply'
into the testDir before running InitCommand so the assertion on
content('/openspec-apply-change') and notContains('/opsx:apply') targets the
rewrite behavior of InitCommand.execute rather than relying on openspec-propose
template content. Ensure you reference InitCommand.execute and the skillFile
path logic in your change.
---
Nitpick comments:
In `@openspec/changes/unify-template-generation-pipeline/design.md`:
- Around line 57-65: ToolProfile declares commandReferenceStyle as required
while AIToolOption treats it as optional, causing a mismatch; update the
registry implementation (ToolProfileRegistry / AI_TOOLS builder) to reconcile
this by either making ToolProfile.commandReferenceStyle optional and documenting
the default 'opsx-colon' (referenced in command-invocation-style.ts) or by
explicitly adding commandReferenceStyle: 'opsx-colon' to every tool entry
missing it (e.g., claude, codebuddy, crush, gemini, qoder) when constructing
AI_TOOLS; apply one consistent approach and add a short comment in the registry
explaining the chosen decision.
In `@src/core/config.ts`:
- Around line 17-19: Add a brief doc comment to the commandReferenceStyle field
in the Config type explaining what the options mean and that the default
behavior is 'opsx-colon' when the field is omitted; update the comment adjacent
to the commandReferenceStyle declaration in src/core/config.ts (the Config
interface/type) and, if helpful, mention that the implicit default is
implemented in command-invocation-style.ts so readers can find the runtime
behavior.
In `@src/core/init.ts`:
- Around line 545-555: The call to
getToolCommandReferenceTransformer(tool.value) is invariant inside the loop over
skillTemplates and should be hoisted out to avoid repeated computation; compute
const transformer = getToolCommandReferenceTransformer(tool.value) once before
the for (const { template, dirName } of skillTemplates) loop and then use that
transformer variable inside the loop when calling generateSkillContent(template,
OPENSPEC_VERSION, transformer), keeping the rest of the SKILL.md generation
logic unchanged.
In `@src/core/update.ts`:
- Around line 195-203: The code repeatedly calls
getToolCommandReferenceTransformer(tool.value) inside the skillTemplates loop;
hoist this call into the outer shouldGenerateSkills branch (e.g., compute const
transformer = getToolCommandReferenceTransformer(tool.value) before iterating
skillTemplates) and then pass that transformer into generateSkillContent; apply
the same refactor in the upgradeLegacyTools function where
getToolCommandReferenceTransformer is called per-template so you compute it once
per tool and reuse it for all template iterations (references:
shouldGenerateSkills, skillTemplates, getToolCommandReferenceTransformer,
generateSkillContent, upgradeLegacyTools).
In `@test/core/command-generation/adapters.test.ts`:
- Around line 458-481: Add a short inline comment above the tests that exercise
opencodeAdapter.formatFile (the two it blocks that use CommandContent and
sampleContent) clarifying that command-format transformation (colon -> hyphen,
e.g., opsx:new -> opsx-new) is performed upstream and that the adapter
intentionally preserves input verbatim; reference opencodeAdapter.formatFile and
the test cases so future readers understand these are pass-through behavior
tests, not a failure to transform.
In `@test/core/command-invocation-style.test.ts`:
- Around line 1-49: Add two tests covering fallback behavior: one calling
getDisplayCommandReferenceStyle with an empty array and asserting it returns the
default display style (verify expected default value), and one calling
getToolCommandReferenceStyle with an unknown tool id (e.g., 'unknown-tool') and
asserting it returns the configured fallback style; place both tests alongside
existing ones in command-invocation-style.test.ts to ensure the functions'
fallback logic is exercised.
test/core/init.test.ts
Outdated
| it('should rewrite skill command references for Trae invocation style', async () => { | ||
| const initCommand = new InitCommand({ tools: 'trae', force: true }); | ||
| await initCommand.execute(testDir); | ||
|
|
||
| const skillFile = path.join(testDir, '.trae', 'skills', 'openspec-propose', 'SKILL.md'); | ||
| const content = await fs.readFile(skillFile, 'utf-8'); | ||
|
|
||
| expect(content).toContain('/openspec-apply-change'); | ||
| expect(content).not.toContain('/opsx:apply'); | ||
| }); |
There was a problem hiding this comment.
Trae test has an implicit coupling to openspec-propose template content.
expect(content).toContain('/openspec-apply-change') will only pass if the openspec-propose skill template body actually contains a /opsx:apply cross-reference (which gets rewritten). If the template is later refactored to remove or rename that reference, this test fails with a misleading message that looks like an invocation-style regression rather than a template content change.
A brief comment or a targeted fixture body would make the intent explicit and guard against silent test-passes:
it('should rewrite skill command references for Trae invocation style', async () => {
const initCommand = new InitCommand({ tools: 'trae', force: true });
await initCommand.execute(testDir);
const skillFile = path.join(testDir, '.trae', 'skills', 'openspec-propose', 'SKILL.md');
const content = await fs.readFile(skillFile, 'utf-8');
+ // openspec-propose body cross-references /opsx:apply; Trae's openspec-hyphen style
+ // rewrites it to /openspec-apply-change. This assertion will fail if that
+ // cross-reference is ever removed from the template.
expect(content).toContain('/openspec-apply-change');
expect(content).not.toContain('/opsx:apply');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should rewrite skill command references for Trae invocation style', async () => { | |
| const initCommand = new InitCommand({ tools: 'trae', force: true }); | |
| await initCommand.execute(testDir); | |
| const skillFile = path.join(testDir, '.trae', 'skills', 'openspec-propose', 'SKILL.md'); | |
| const content = await fs.readFile(skillFile, 'utf-8'); | |
| expect(content).toContain('/openspec-apply-change'); | |
| expect(content).not.toContain('/opsx:apply'); | |
| }); | |
| it('should rewrite skill command references for Trae invocation style', async () => { | |
| const initCommand = new InitCommand({ tools: 'trae', force: true }); | |
| await initCommand.execute(testDir); | |
| const skillFile = path.join(testDir, '.trae', 'skills', 'openspec-propose', 'SKILL.md'); | |
| const content = await fs.readFile(skillFile, 'utf-8'); | |
| // openspec-propose body cross-references /opsx:apply; Trae's openspec-hyphen style | |
| // rewrites it to /openspec-apply-change. This assertion will fail if that | |
| // cross-reference is ever removed from the template. | |
| expect(content).toContain('/openspec-apply-change'); | |
| expect(content).not.toContain('/opsx:apply'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/core/init.test.ts` around lines 358 - 367, The test for InitCommand's
Trae rewrite is implicitly coupled to the openspec-propose template content so
it can break silently; update the test (the it block that instantiates
InitCommand and reads SKILL.md) to either (a) load/use a controlled
fixture/template that contains a known '/opsx:apply' reference and assert it
becomes '/openspec-apply-change', or (b) add a clear comment and explicitly
write a minimal SKILL.md with '/opsx:apply' into the testDir before running
InitCommand so the assertion on content('/openspec-apply-change') and
notContains('/opsx:apply') targets the rewrite behavior of InitCommand.execute
rather than relying on openspec-propose template content. Ensure you reference
InitCommand.execute and the skillFile path logic in your change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openspec/changes/unify-template-generation-pipeline/design.md (2)
70-74:commandSurface.aliasespurpose is unexplained.The
aliases?: string[]field has no rationale or example in the design. It's unclear whether this is for:
- Alternative invocation patterns the tool also accepts (e.g., a tool that supports both
/opsx:applyand/opsx-apply)- Display names for the command surface
- Something else entirely
A brief clarifying comment or rationale entry (matching the style of the other
commandSurfacedecisions) would prevent incorrect implementations during task 2.4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/unify-template-generation-pipeline/design.md` around lines 70 - 74, The comment points out that commandSurface.aliases is undocumented and ambiguous; update the design.md entry for the commandSurface block to explain the purpose and give examples: state whether aliases are alternative invocation patterns (e.g., '/opsx:apply' vs '/opsx-apply'), display names, or other, and include at least one concrete example and expected behavior (how parsers should treat aliases and whether they require verification via the verified flag). Specifically update the commandSurface.aliases description and add a short rationale line matching other decisions so implementers know how to use commandSurface.aliases when implementing command parsing/validation.
98-114:ArtifactTransform.phasehas no value for the token-render step — design gap.The execution order places token rendering as step 2, explicitly before
preAdaptertransforms. However, theArtifactTransforminterface only definesphase: 'preAdapter' | 'postAdapter'. If the token renderer is implemented as anArtifactTransform(as task 3.3 implies), neither phase is semantically correct and ordering becomes ambiguous.Two approaches to resolve this:
- Add a dedicated phase — extend the union to include a
'tokenRender'phase and document it as the built-in first pass:♻️ Option A – add `tokenRender` phase
interface ArtifactTransform { id: string; scope: 'skill' | 'command' | 'both'; - phase: 'preAdapter' | 'postAdapter'; + phase: 'tokenRender' | 'preAdapter' | 'postAdapter'; priority: number; applies(ctx: GenerationContext): boolean; transform(content: string, ctx: GenerationContext): string; }
- Make it a built-in step — document that token rendering is a first-class, non-configurable pipeline step rather than an
ArtifactTransformplug-in, and reserve the interface for developer-defined transforms only.Whichever option is chosen, the execution order comment (steps 1–6) should reference it explicitly so implementers of task 3.3 aren't left guessing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/unify-template-generation-pipeline/design.md` around lines 98 - 114, The ArtifactTransform interface lacks a phase value for the token-render step, causing ambiguity between the token renderer and preAdapter/postAdapter transforms; update the design by either (A) adding a dedicated phase string like 'tokenRender' to ArtifactTransform.phase (so implementations of ArtifactTransform can declare phase: 'tokenRender' and the execution order 1–6 explicitly references this phase), or (B) document token rendering as a built-in, non-pluggable pipeline step outside ArtifactTransform (and update the doc to state that ArtifactTransform implementations only use 'preAdapter'|'postAdapter'); reference ArtifactTransform, its phase field, and the token-render step in GenerationContext/docs so task 3.3 implementers know the correct placement and ordering.openspec/changes/unify-template-generation-pipeline/tasks.md (1)
15-15: Task 2.6 is misplaced under Tool Profile Layer.
SKILL_NAMESis a manifest-derived projection, not a tool profile concern. The implementation approach in design.md (step 3: "RewiregetSkillTemplates/getCommandContentsto derive from manifest") places this squarely in Section 1 (Manifest Foundation). Keeping it under Section 2 may cause implementers to add it as a profile dependency when it only needs the manifest.♻️ Suggested move
## 1. Manifest Foundation - [ ] 1.1 Create canonical workflow manifest registry under `src/core/templates/` - [ ] 1.2 Define shared manifest types for workflow IDs, skill metadata, and optional command descriptors - [ ] 1.3 Migrate existing workflow registration (`getSkillTemplates`, `getCommandTemplates`, `getCommandContents`) to derive from the manifest - [ ] 1.4 Preserve existing external exports/API compatibility for `src/core/templates/skill-templates.ts` +- [ ] 1.5 Replace hardcoded detection arrays (for example `SKILL_NAMES`) with manifest-derived values ## 2. Tool Profile Layer - [ ] 2.1 Add `ToolProfile` types and `ToolProfileRegistry` - [ ] 2.2 Map all currently supported tools to explicit profile entries - [ ] 2.3 Wire profile lookups to command adapter resolution and skills path resolution - [ ] 2.4 Add per-tool `commandSurface` metadata (pattern, aliases, `verified` flag) - [ ] 2.5 Add per-tool terminology metadata (for example change/workflow/command labels) -- [ ] 2.6 Replace hardcoded detection arrays (for example `SKILL_NAMES`) with manifest-derived values🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/unify-template-generation-pipeline/tasks.md` at line 15, Task 2.6 is in the wrong section: move the item about replacing hardcoded detection arrays (e.g., SKILL_NAMES) out of the Tool Profile Layer and into the Manifest Foundation (Section 1) because SKILL_NAMES is a manifest-derived projection; update the tasks list so the checklist entry "- [ ] 2.6 Replace hardcoded detection arrays (for example `SKILL_NAMES`) with manifest-derived values" is relocated to Section 1 and add a short note linking it to the redesign step that rewires getSkillTemplates/getCommandContents to derive values from the manifest.
🤖 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/unify-template-generation-pipeline/design.md`:
- Around line 47-51: The example token {{cmd.continue.withArg}} is ambiguous;
either define the third-level modifier contract here or replace the example with
a supported two-level token like {{cmd.continue}}. Update the "Examples in
canonical workflow text" block to (a) explicitly document what a modifier such
as .withArg means (e.g., produces a placeholder argument, toggles inclusion, or
is invalid), (b) enumerate allowed modifier names and whether multiple modifiers
are permitted, and (c) describe how the renderer resolves a three-part path
(lookup order and fallback behavior). Make these edits so the example tokens
({{cmd.apply}}, {{term.change}}, and the revised {{cmd.continue}} or the newly
specified {{cmd.continue.withArg}} semantics) match the resolver behavior
expected by tasks 3.3 and 5.3.
---
Nitpick comments:
In `@openspec/changes/unify-template-generation-pipeline/design.md`:
- Around line 70-74: The comment points out that commandSurface.aliases is
undocumented and ambiguous; update the design.md entry for the commandSurface
block to explain the purpose and give examples: state whether aliases are
alternative invocation patterns (e.g., '/opsx:apply' vs '/opsx-apply'), display
names, or other, and include at least one concrete example and expected behavior
(how parsers should treat aliases and whether they require verification via the
verified flag). Specifically update the commandSurface.aliases description and
add a short rationale line matching other decisions so implementers know how to
use commandSurface.aliases when implementing command parsing/validation.
- Around line 98-114: The ArtifactTransform interface lacks a phase value for
the token-render step, causing ambiguity between the token renderer and
preAdapter/postAdapter transforms; update the design by either (A) adding a
dedicated phase string like 'tokenRender' to ArtifactTransform.phase (so
implementations of ArtifactTransform can declare phase: 'tokenRender' and the
execution order 1–6 explicitly references this phase), or (B) document token
rendering as a built-in, non-pluggable pipeline step outside ArtifactTransform
(and update the doc to state that ArtifactTransform implementations only use
'preAdapter'|'postAdapter'); reference ArtifactTransform, its phase field, and
the token-render step in GenerationContext/docs so task 3.3 implementers know
the correct placement and ordering.
In `@openspec/changes/unify-template-generation-pipeline/tasks.md`:
- Line 15: Task 2.6 is in the wrong section: move the item about replacing
hardcoded detection arrays (e.g., SKILL_NAMES) out of the Tool Profile Layer and
into the Manifest Foundation (Section 1) because SKILL_NAMES is a
manifest-derived projection; update the tasks list so the checklist entry "- [ ]
2.6 Replace hardcoded detection arrays (for example `SKILL_NAMES`) with
manifest-derived values" is relocated to Section 1 and add a short note linking
it to the redesign step that rewires getSkillTemplates/getCommandContents to
derive values from the manifest.
| // Examples in canonical workflow text: | ||
| // - {{cmd.apply}} | ||
| // - {{cmd.continue.withArg}} | ||
| // - {{term.change}} | ||
| // - {{term.workflow}} |
There was a problem hiding this comment.
{{cmd.continue.withArg}} token format is undefined — implementation ambiguity.
The two-level tokens {{cmd.apply}} and {{term.change}} are self-explanatory, but {{cmd.continue.withArg}} introduces a three-level path (namespace · command name · modifier) without defining:
- What
.withArgmeans (a variant that includes an argument placeholder? a conditional rendering flag?) - Whether additional modifiers are valid (e.g.,
{{cmd.apply.withArg}}) - How the token renderer resolves the third level
This will create inconsistent implementations across tasks 3.3 and 5.3. Recommend either defining the modifier contract inline here, or simplifying the example to a form that matches what the resolver will actually support (e.g., {{cmd.continue}}).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/unify-template-generation-pipeline/design.md` around lines
47 - 51, The example token {{cmd.continue.withArg}} is ambiguous; either define
the third-level modifier contract here or replace the example with a supported
two-level token like {{cmd.continue}}. Update the "Examples in canonical
workflow text" block to (a) explicitly document what a modifier such as .withArg
means (e.g., produces a placeholder argument, toggles inclusion, or is invalid),
(b) enumerate allowed modifier names and whether multiple modifiers are
permitted, and (c) describe how the renderer resolves a three-part path (lookup
order and fallback behavior). Make these edits so the example tokens
({{cmd.apply}}, {{term.change}}, and the revised {{cmd.continue}} or the newly
specified {{cmd.continue.withArg}} semantics) match the resolver behavior
expected by tasks 3.3 and 5.3.
1 participant
Summary
opsx-colon,opsx-hyphen,openspec-hyphen)initandupdateunify-template-generation-pipelineWhy
Issue #727 reported stale command references after command naming updates (especially for Codex). The prior implementation only rewrote OpenCode content and left other tools with mismatched references.
Verification
pnpm run buildnpx vitest run test/core/command-invocation-style.test.ts test/utils/command-references.test.ts test/core/command-generation/adapters.test.ts test/core/init.test.ts test/core/update.test.tsNotes
unify-template-generation-pipelinearchitecture, rather than introducing another one-off path.Closes #727
Summary by CodeRabbit