[codex] Add workspace change planning workflow#1089
Open
TabishB wants to merge 11 commits into
Open
Conversation
Contributor
|
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 (2)
📝 WalkthroughWalkthroughAdds a workspace-planning schema and templates; introduces planning-home resolution; updates CLI (workspace setup/update, new-change with goal/areas, update redirection); enriches status/instructions JSON; implements workspace agent-skill generation/update; updates workflow templates; and expands docs and tests. ChangesWorkspace planning, skills, and planning-home integration
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant PlanningHome
participant WorkspaceSkills
participant FS
User->>CLI: openspec update (in workspace)
CLI->>PlanningHome: resolveCurrentPlanningHomeSync()
PlanningHome-->>CLI: { kind: workspace, root, changesDir }
CLI->>WorkspaceSkills: updateWorkspaceAgentSkills(root, selection)
WorkspaceSkills->>FS: write/remove SKILL.md dirs
WorkspaceSkills-->>CLI: installation report (JSON/text)
CLI-->>User: workspace update summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/core/planning-home.test.ts (1)
10-28: ⚡ Quick winConsider adding a POSIX path test case for cross-platform coverage.
The test currently only validates Windows-style paths (
D:\repos\...). Adding a parallel test case with POSIX paths would ensure the functions work correctly on all platforms.♻️ Suggested additional test case
+ it('builds workspace change paths with POSIX path style', () => { + const workspacePlanningHome: PlanningHome = { + kind: 'workspace', + root: '/repos/platform-workspace', + changesDir: '/repos/platform-workspace/changes', + defaultSchema: 'workspace-planning', + workspace: { + name: 'platform', + links: ['api', 'web'], + }, + }; + + expect(getChangeDir(workspacePlanningHome, 'cross-repo-login')).toBe( + '/repos/platform-workspace/changes/cross-repo-login' + ); + expect(formatChangeLocation(workspacePlanningHome, 'cross-repo-login')).toBe( + 'changes/cross-repo-login' + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/planning-home.test.ts` around lines 10 - 28, The test only covers Windows-style paths; add a parallel POSIX-style test case to validate cross-platform behavior by creating a PlanningHome object with POSIX paths (e.g., root: '/home/repos/platform-workspace', changesDir: '/home/repos/platform-workspace/changes') and assert getChangeDir(workspacePlanningHomePosix, 'cross-repo-login') returns '/home/repos/platform-workspace/changes/cross-repo-login' and formatChangeLocation(workspacePlanningHomePosix, 'cross-repo-login') returns 'changes/cross-repo-login'; update or add a new it(...) that mirrors the existing Windows test but uses the PlanningHome type and the getChangeDir and formatChangeLocation functions for POSIX validation.src/core/workspace/skills.ts (1)
348-349: ⚡ Quick winHardcoded tool IDs for command transformation.
The logic hardcodes
'opencode'and'pi'as tool IDs requiring hyphen command transformation. If new tools with similar requirements are added, this code will need manual updates.♻️ Consider adding a tool property
Add a
requiresHyphenTransform?: booleanproperty to the tool definition inAI_TOOLSand check it here:- const transformer = - tool.value === 'opencode' || tool.value === 'pi' ? transformToHyphenCommands : undefined; + const transformer = tool.requiresHyphenTransform ? transformToHyphenCommands : undefined;This would centralize tool-specific configuration and make it easier to maintain.
Also applies to: 460-461
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/workspace/skills.ts` around lines 348 - 349, The code currently sets transformer based on hardcoded tool IDs ('opencode' and 'pi') using tool.value and transformToHyphenCommands; instead add a tool-level boolean (e.g., requiresHyphenTransform?: boolean) to each entry in AI_TOOLS and update the assignment to check tool.requiresHyphenTransform (or AI_TOOLS[tool.value].requiresHyphenTransform) instead of comparing strings; apply the same change to the other occurrence that currently checks 'opencode'/'pi' so future tools can enable the hyphen transform via configuration rather than code changes.schemas/workspace-planning/templates/spec.md (1)
1-10: 💤 Low valueConsider adding examples for all delta operation types.
The schema instruction (lines 33-38 in
schema.yaml) defines four delta operation types: ADDED, MODIFIED, REMOVED, and RENAMED requirements. The template currently shows only the ADDED section. While the schema instructions provide text guidance for the other types, including template examples for MODIFIED, REMOVED, and RENAMED sections would make the template more helpful and reduce ambiguity for users creating workspace specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schemas/workspace-planning/templates/spec.md` around lines 1 - 10, The template only includes an "ADDED Requirements" example; add analogous example sections for MODIFIED, REMOVED, and RENAMED requirement deltas to match the schema.yaml delta types. For each new section (e.g., "MODIFIED Requirements", "REMOVED Requirements", "RENAMED Requirements") provide a short requirement header (like "Requirement: <workspace requirement name>") and a Scenario block using the GIVEN/WHEN/THEN format similar to the ADDED example so authors have concrete templates for each delta operation type.openspec/changes/workspace-change-planning/specs/workspace-links/spec.md (1)
1-164: 💤 Low valueConsider renaming this spec file to better reflect its content.
The file is named
workspace-links/spec.mdbut the requirements focus on workspace agent skills installation and update workflows, not workspace links as a concept. While the scenarios reference linked repos/folders, the primary subject matter is agent skill management during setup and update operations. A name likeworkspace-skills/spec.md,workspace-setup/spec.md, orcli-workspace-skills/spec.mdwould more accurately reflect the content and make the spec easier to locate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/workspace-change-planning/specs/workspace-links/spec.md` around lines 1 - 164, This spec file's name (workspace-links/spec.md) doesn't match its content (workspace agent skill installation/update), so rename the file to a clearer name (e.g., workspace-skills/spec.md or cli-workspace-skills/spec.md) and update any references to it in docs or test harnesses; ensure the new name reflects the primary subject (agent skill setup/update) while keeping the existing content and preserving links to any mentions of "linked repos/folders" within the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openspec/changes/workspace-change-planning/review-candidate-requirements.html`:
- Around line 729-742: The template assigns to card.innerHTML and injects
s.userComment into a textarea via string replacement, which allows XSS if the
comment contains HTML like </textarea>; fix by building the DOM nodes
programmatically instead of using innerHTML: create the container elements (the
sug-head, sug-text, sug-actions, textarea with class "sug-comment") with
document.createElement, set attributes and classes (including data-comment-for
using s.id), set the textarea's value or textContent to s.userComment (not via
string concatenation), and append the pieces to card so content is safely
escaped; ensure the approve/reject/reset buttons and their is-active logic
(based on s.status) and visibility logic (based on state.activeId === s.id) are
preserved when converting the innerHTML block.
In `@src/cli/index.ts`:
- Around line 168-171: The redirect-to-workspace code is dropping the parsed
--force option by calling runWorkspaceUpdateForRoot(workspaceRoot, {}); change
this to propagate the parsed force flag (e.g., pass { force } or { force:
options.force } depending on your option variable) so runWorkspaceUpdateForRoot
receives the same force semantics as the top-level command; update the call in
the branch that uses workspaceRoot (and ensure the option variable name in
scope—such as force or options.force—is referenced).
In `@src/core/workspace/skills.ts`:
- Around line 101-107: The arraysEqual function currently does an
order-sensitive element-by-element comparison causing false positives when
workflow ID order changes; update arraysEqual (and any callers like
last_applied_workflow_ids checks) to perform an order-insensitive set
comparison: treat undefined as empty, if lengths differ return false, convert
both arrays to Sets and ensure every element in one Set exists in the other (and
sizes match) so identical ID sets in different orders are considered equal.
---
Nitpick comments:
In `@openspec/changes/workspace-change-planning/specs/workspace-links/spec.md`:
- Around line 1-164: This spec file's name (workspace-links/spec.md) doesn't
match its content (workspace agent skill installation/update), so rename the
file to a clearer name (e.g., workspace-skills/spec.md or
cli-workspace-skills/spec.md) and update any references to it in docs or test
harnesses; ensure the new name reflects the primary subject (agent skill
setup/update) while keeping the existing content and preserving links to any
mentions of "linked repos/folders" within the spec.
In `@schemas/workspace-planning/templates/spec.md`:
- Around line 1-10: The template only includes an "ADDED Requirements" example;
add analogous example sections for MODIFIED, REMOVED, and RENAMED requirement
deltas to match the schema.yaml delta types. For each new section (e.g.,
"MODIFIED Requirements", "REMOVED Requirements", "RENAMED Requirements") provide
a short requirement header (like "Requirement: <workspace requirement name>")
and a Scenario block using the GIVEN/WHEN/THEN format similar to the ADDED
example so authors have concrete templates for each delta operation type.
In `@src/core/workspace/skills.ts`:
- Around line 348-349: The code currently sets transformer based on hardcoded
tool IDs ('opencode' and 'pi') using tool.value and transformToHyphenCommands;
instead add a tool-level boolean (e.g., requiresHyphenTransform?: boolean) to
each entry in AI_TOOLS and update the assignment to check
tool.requiresHyphenTransform (or AI_TOOLS[tool.value].requiresHyphenTransform)
instead of comparing strings; apply the same change to the other occurrence that
currently checks 'opencode'/'pi' so future tools can enable the hyphen transform
via configuration rather than code changes.
In `@test/core/planning-home.test.ts`:
- Around line 10-28: The test only covers Windows-style paths; add a parallel
POSIX-style test case to validate cross-platform behavior by creating a
PlanningHome object with POSIX paths (e.g., root:
'/home/repos/platform-workspace', changesDir:
'/home/repos/platform-workspace/changes') and assert
getChangeDir(workspacePlanningHomePosix, 'cross-repo-login') returns
'/home/repos/platform-workspace/changes/cross-repo-login' and
formatChangeLocation(workspacePlanningHomePosix, 'cross-repo-login') returns
'changes/cross-repo-login'; update or add a new it(...) that mirrors the
existing Windows test but uses the PlanningHome type and the getChangeDir and
formatChangeLocation functions for POSIX validation.
🪄 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: 77aa2c5c-4555-4464-b8f2-bbe4deade43d
📒 Files selected for processing (64)
docs/cli.mddocs/concepts.mdopenspec/changes/workspace-change-planning/candidate-requirements.mdopenspec/changes/workspace-change-planning/deep-dive-requirements.htmlopenspec/changes/workspace-change-planning/design.mdopenspec/changes/workspace-change-planning/exploration-summary.mdopenspec/changes/workspace-change-planning/manual-acceptance.mdopenspec/changes/workspace-change-planning/prd.mdopenspec/changes/workspace-change-planning/proposal.mdopenspec/changes/workspace-change-planning/review-candidate-requirements.htmlopenspec/changes/workspace-change-planning/specs/artifact-graph/spec.mdopenspec/changes/workspace-change-planning/specs/change-creation/spec.mdopenspec/changes/workspace-change-planning/specs/cli-artifact-workflow/spec.mdopenspec/changes/workspace-change-planning/specs/cli-config/spec.mdopenspec/changes/workspace-change-planning/specs/cli-update/spec.mdopenspec/changes/workspace-change-planning/specs/openspec-conventions/spec.mdopenspec/changes/workspace-change-planning/specs/schema-resolution/spec.mdopenspec/changes/workspace-change-planning/specs/workspace-change-planning/spec.mdopenspec/changes/workspace-change-planning/specs/workspace-links/spec.mdopenspec/changes/workspace-change-planning/tasks.mdschemas/workspace-planning/schema.yamlschemas/workspace-planning/templates/design.mdschemas/workspace-planning/templates/proposal.mdschemas/workspace-planning/templates/spec.mdschemas/workspace-planning/templates/tasks.mdsrc/cli/index.tssrc/commands/config.tssrc/commands/workflow/instructions.tssrc/commands/workflow/new-change.tssrc/commands/workflow/shared.tssrc/commands/workflow/status.tssrc/commands/workspace.tssrc/commands/workspace/operations.tssrc/commands/workspace/selection.tssrc/commands/workspace/types.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/types.tssrc/core/completions/command-registry.tssrc/core/index.tssrc/core/planning-home.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tssrc/core/templates/workflows/continue-change.tssrc/core/templates/workflows/explore.tssrc/core/templates/workflows/ff-change.tssrc/core/templates/workflows/new-change.tssrc/core/templates/workflows/onboard.tssrc/core/templates/workflows/propose.tssrc/core/templates/workflows/sync-specs.tssrc/core/templates/workflows/verify-change.tssrc/core/workspace/foundation.tssrc/core/workspace/index.tssrc/core/workspace/skills.tssrc/utils/change-metadata.tssrc/utils/change-utils.tstest/commands/artifact-workflow.test.tstest/commands/config-profile.test.tstest/commands/workspace.interactive.test.tstest/commands/workspace.test.tstest/core/planning-home.test.tstest/core/templates/skill-templates-parity.test.tstest/core/workspace/skills.test.ts
Comment on lines
+168
to
+171
| const workspaceRoot = await findWorkspaceRoot(resolvedPath); | ||
| if (workspaceRoot) { | ||
| await runWorkspaceUpdateForRoot(workspaceRoot, {}); | ||
| return; |
Contributor
There was a problem hiding this comment.
Propagate --force when redirecting to workspace update.
Line 170 drops the parsed --force option by passing {}. That makes openspec update --force behave inconsistently inside a workspace.
Suggested fix
- await runWorkspaceUpdateForRoot(workspaceRoot, {});
+ await runWorkspaceUpdateForRoot(workspaceRoot, { force: options?.force });📝 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.
Suggested change
| const workspaceRoot = await findWorkspaceRoot(resolvedPath); | |
| if (workspaceRoot) { | |
| await runWorkspaceUpdateForRoot(workspaceRoot, {}); | |
| return; | |
| const workspaceRoot = await findWorkspaceRoot(resolvedPath); | |
| if (workspaceRoot) { | |
| await runWorkspaceUpdateForRoot(workspaceRoot, { force: options?.force }); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 168 - 171, The redirect-to-workspace code is
dropping the parsed --force option by calling
runWorkspaceUpdateForRoot(workspaceRoot, {}); change this to propagate the
parsed force flag (e.g., pass { force } or { force: options.force } depending on
your option variable) so runWorkspaceUpdateForRoot receives the same force
semantics as the top-level command; update the call in the branch that uses
workspaceRoot (and ensure the option variable name in scope—such as force or
options.force—is referenced).
Comment on lines
+101
to
+107
| function arraysEqual(left: readonly string[] | undefined, right: readonly string[]): boolean { | ||
| if (!left || left.length !== right.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return left.every((value, index) => value === right[index]); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential issue: Array comparison doesn't check element order.
The arraysEqual helper compares arrays element-by-element using index position. If the workflow IDs are stored in different orders between last_applied_workflow_ids and the current profile, this will report drift even when the sets are identical. This could cause unnecessary skill regeneration.
🔧 Proposed fix to compare sets instead of ordered arrays
-function arraysEqual(left: readonly string[] | undefined, right: readonly string[]): boolean {
- if (!left || left.length !== right.length) {
- return false;
- }
-
- return left.every((value, index) => value === right[index]);
+function arraysEqual(left: readonly string[] | undefined, right: readonly string[]): boolean {
+ if (!left || left.length !== right.length) {
+ return false;
+ }
+
+ const leftSet = new Set(left);
+ const rightSet = new Set(right);
+ return left.every((value) => rightSet.has(value)) && right.every((value) => leftSet.has(value));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/workspace/skills.ts` around lines 101 - 107, The arraysEqual
function currently does an order-sensitive element-by-element comparison causing
false positives when workflow ID order changes; update arraysEqual (and any
callers like last_applied_workflow_ids checks) to perform an order-insensitive
set comparison: treat undefined as empty, if lengths differ return false,
convert both arrays to Sets and ensure every element in one Set exists in the
other (and sizes match) so identical ID sets in different orders are considered
equal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openspec update, unmanaged skill preservation, failed skill-state recording, and template whitespace.Impact
This gives workspace users a cleaner path for planning and implementing change work with generated workflow skills that match the workspace context. It also tightens update safety so local user-authored skill directories are preserved and failed updates are not recorded as successfully applied.
Validation
pnpm exec eslint src/commands/workspace.ts src/commands/workspace/selection.ts src/core/workspace/skills.ts src/cli/index.tsgit diff --checkpnpm run buildpnpm exec vitest run test/commands/workspace.test.ts test/core/workspace/skills.test.tspnpm testnode dist/cli/index.js validate workspace-change-planning --strictSummary by CodeRabbit
Release Notes
New Features
openspec workspace updatecommand to refresh agent skills in workspaces with--toolsoption for selecting specific agents.Enhancements