[codex] Fix workspace.yaml collision detection#1165
Merged
Conversation
Contributor
|
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 relocates OpenSpec's workspace view state from ChangesWorkspace View State Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![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: 1
🤖 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 `@src/core/workspace/foundation.ts`:
- Around line 79-81: Tests and registry currently ignore legacy root-level
workspace.yaml, so implement an explicit migration that detects a root-level
"workspace.yaml" and backfills it into the managed metadata directory used by
getWorkspaceViewStatePath/getWorkspaceMetadataDir (or otherwise imports it into
the same shape read by isWorkspaceRoot and readWorkspaceViewState*), and ensure
registry enumeration in registry.ts picks up those migrated workspaces; add a
startup migration function (e.g., migrateLegacyRootWorkspaceYaml) that runs
before workspace enumeration and moves/parses root-level workspace.yaml into
.openspec-workspace/workspace.yaml (or writes the normalized view.yaml) for
known managed/registry workspaces, and update
isWorkspaceRoot/readWorkspaceViewState*/registry.ts to treat the migrated file
as authoritative.
🪄 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: 02808ee6-cd17-4340-9083-bb3b3cdf6d39
📒 Files selected for processing (14)
.changeset/fuzzy-dagster-workspaces.mddocs/concepts.mddocs/workspaces-beta/user-guide.mdopenspec/specs/workspace-foundation/spec.mdsrc/commands/workspace/context-status.tssrc/commands/workspace/open-view.tssrc/commands/workspace/operations.tssrc/core/workspace/foundation.tssrc/core/workspace/legacy-state.tssrc/core/workspace/open-surface.tssrc/core/workspace/state-io.tstest/core/planning-home.test.tstest/core/workspace/foundation.test.tstest/core/workspace/legacy-state.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/workspaces-beta/user-guide.md
- src/core/workspace/open-surface.ts
- .changeset/fuzzy-dagster-workspaces.md
- src/commands/workspace/operations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/planning-home.test.ts
Comment on lines
79
to
81
| export function getWorkspaceViewStatePath(workspaceRoot: string): string { | ||
| return joinWorkspacePath(workspaceRoot, WORKSPACE_VIEW_STATE_FILE_NAME); | ||
| return joinWorkspacePath(getWorkspaceMetadataDir(workspaceRoot), WORKSPACE_VIEW_STATE_FILE_NAME); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Any code still reading a root-level workspace.yaml (outside .openspec-workspace/)?
rg -nP -C3 "workspace\.yaml" --type=ts -g '!**/*.test.ts'
# 2) Any migration/backfill for relocating legacy root state?
rg -nP -C4 '(migrat|backfill|legacyRoot|rootViewState|relocat)' --type=ts -g 'src/**'
# 3) How getWorkspaceViewStatePath / legacy shared path are consumed across src.
rg -nP -C3 '(getWorkspaceViewStatePath|getWorkspaceLegacySharedStatePath)' --type=ts -g 'src/**'Repository: Fission-AI/OpenSpec
Length of output: 22057
🏁 Script executed:
#!/bin/bash
# Find any root-level filesystem probing for "workspace.yaml" (not under .openspec-workspace)
rg -nP --type=ts '(workspaceRoot\s*,\s*["'\'']workspace\.yaml["'\'']|joinWorkspacePath\([^)]*workspaceRoot[^)]*["'\'']workspace\.yaml["'\'']|joinPath\([^)]*workspaceRoot[^)]*["'\'']workspace\.yaml["'\'']|path.*workspace\.yaml)' src test
# Inspect workspace root detection/read path logic around view + legacy workspace.yaml
rg -nP --type=ts 'function readWorkspaceViewState|function isWorkspaceRoot|getWorkspaceLegacySharedStatePath|getWorkspaceViewStatePath' src/core/workspace/state-io.ts src/core/workspace/foundation.ts src/core/workspace/legacy-state.ts
# Locate the test mentioned in the original comment
rg -nP 'ignores unmarked root view state' test --type=ts
rg -nP 'unmarked root view state' test --type=ts
rg -nP 'root view state' test --type=tsRepository: Fission-AI/OpenSpec
Length of output: 3112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search in src for any reads/writes/migrations involving root-level workspace.yaml and view.yaml
rg -nP --type=ts 'workspace\.yaml' src/core src/commands | head -n 200
rg -nP --type=ts 'view\.yaml|WORKSPACE_VIEW_STATE_FILE_NAME|WORKSPACE_LEGACY_SHARED_STATE_FILE_NAME|getWorkspaceViewStatePath|getWorkspaceLegacySharedStatePath' src/core src/commands | head -n 200
# Look specifically for "migration" or "backfill" that mentions workspace state.
rg -nP --type=ts '(migrat|backfill|legacyRoot|root view state|relocat|orphan)' src/core/workspace src/core src/commands | head -n 200
# Inspect the relevant test file around the "ignores unmarked root view state" case.
sed -n '200,340p' test/core/workspace/foundation.test.tsRepository: Fission-AI/OpenSpec
Length of output: 10675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant constants + functions for workspace metadata dir + view state path
sed -n '1,140p' src/core/workspace/foundation.ts
echo '---'
sed -n '1,120p' src/core/workspace/legacy-state.ts
echo '---'
sed -n '1,230p' src/core/workspace/state-io.ts
echo '---'
# Search for any code that might copy/migrate root-level workspace.yaml into .openspec-workspace
rg -nP --type=ts "workspaceRoot.*workspace\.yaml|path\.join\([^)]*workspaceRoot[^)]*workspace\.yaml|joinWorkspacePath\([^)]*workspaceRoot[^)]*workspace\.yaml|\.openspec-workspace'.workspace\.yaml|copyFile.*workspace\.yaml|rename.*workspace\.yaml|move.*workspace\.yaml|relocat|migrat" src/core src/commands
echo '---'
# Search for init/update referencing workspace state migration
rg -nP --type=ts "writeWorkspaceViewState|readWorkspaceViewState|readWorkspaceViewStateSync|workspace\.yaml|view\.yaml|relink|repair .*view\.yaml|orphan" src/core src/commandsRepository: Fission-AI/OpenSpec
Length of output: 20247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Any non-constant usage of root-level workspace.yaml inside src (not tests)
rg -nP --type=ts "workspace\.yaml" src/core/workspace src/commands/workspace src/core/planning-home.ts src/core/registry* 2>/dev/null || true
# 2) Any direct mentions of root-level state backfill/migration in workspace area
rg -nP --type=ts "(migrat|backfill|relocat|legacy).*(workspace|view|state)" src/core/workspace src/commands/workspace || true
# 3) Any code that reads/writes legacy shared state file or workspace view state beyond state-io
rg -nP --type=ts "(getWorkspaceLegacySharedStatePath|readWorkspaceViewState|writeWorkspaceViewState|workspaceStatePartsToViewState|WORKSPACE_LEGACY_SHARED_STATE_FILE_NAME|getWorkspaceViewStatePath)" src/core/workspace src/commands/workspace || true
# 4) Inspect registry + managed workspace root detection
sed -n '1,260p' src/core/workspace/registry.ts || true
sed -n '260,520p' src/core/workspace/registry.ts || trueRepository: Fission-AI/OpenSpec
Length of output: 12045
Add/confirm migration/backfill for legacy root-level workspace.yaml so managed/registry workspaces aren’t orphaned
isWorkspaceRoot/readWorkspaceViewState*only probe.openspec-workspace/view.yamland.openspec-workspace/workspace.yaml(not<root>/workspace.yaml), and managed workspace enumeration (registry.ts) relies on that same detection.- The test suite enforces the behavior that root-level
workspace.yamlis ignored even when its contents look OpenSpec-shaped. - Please ensure there’s an explicit backfill/migration for known managed/registry workspaces (or document the behavior) so existing beta users don’t silently lose their workspace state.
🤖 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/foundation.ts` around lines 79 - 81, Tests and registry
currently ignore legacy root-level workspace.yaml, so implement an explicit
migration that detects a root-level "workspace.yaml" and backfills it into the
managed metadata directory used by
getWorkspaceViewStatePath/getWorkspaceMetadataDir (or otherwise imports it into
the same shape read by isWorkspaceRoot and readWorkspaceViewState*), and ensure
registry enumeration in registry.ts picks up those migrated workspaces; add a
startup migration function (e.g., migrateLegacyRootWorkspaceYaml) that runs
before workspace enumeration and moves/parses root-level workspace.yaml into
.openspec-workspace/workspace.yaml (or writes the normalized view.yaml) for
known managed/registry workspaces, and update
isWorkspaceRoot/readWorkspaceViewState*/registry.ts to treat the migrated file
as authoritative.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/cli/index.ts`:
- Around line 169-175: The current guard uses findWorkspaceRoot(resolvedPath)
and always throws for any path under a workspace, which blocks repo-local
projects; instead, first detect whether the resolvedPath itself contains a
repo-local OpenSpec project (use the existing repo-local detection helper or the
same logic UpdateCommand.execute uses to identify repo-local projects for the
target path). Only if no repo-local project exists should you call
findWorkspaceRoot(resolvedPath) and then throw the workspace-specific error;
additionally ensure you only treat the workspace as blocking when the
resolvedPath is a planning-home path without its own repo-local project (i.e.,
defer the workspaceRoot check until after repo-local detection).
🪄 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: c63661e9-29b4-4c4e-975a-c4164a16bedd
📒 Files selected for processing (6)
.changeset/fuzzy-dagster-workspaces.mddocs/cli.mdopenspec/specs/cli-update/spec.mdsrc/cli/index.tssrc/commands/workspace.tstest/commands/workspace.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fuzzy-dagster-workspaces.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/cli/index.ts`:
- Around line 104-105: The current catch block that unconditionally returns
false should be narrowed to only swallow "not found" filesystem errors; change
the anonymous catch to catch (err) and if (err?.code === 'ENOENT' || err?.code
=== 'ENOTDIR') return false, otherwise rethrow the error so permission/IO
failures bubble up; update the catch around the filesystem check (the try/catch
that currently uses `} catch { return false;`) to implement this conditional
handling.
🪄 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: 3fa31471-4146-4a6f-901f-dc9ce2b926ae
📒 Files selected for processing (4)
docs/concepts.mdopenspec/specs/cli-update/spec.mdsrc/cli/index.tstest/commands/workspace.test.ts
✅ Files skipped from review due to trivial changes (2)
- openspec/specs/cli-update/spec.md
- docs/concepts.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/commands/workspace.test.ts
alfred-openspec
previously approved these changes
Jun 3, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the workspace/update split and Dagster-style workspace.yaml regression coverage. The change keeps repo-local update behavior intact, moves workspace view state under .openspec-workspace/view.yaml, avoids mutating workspace state from top-level openspec update, and CI is green. Looks good to merge.
alfred-openspec
approved these changes
Jun 3, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the workspace/update split after the latest fixes. The Dagster-style workspace.yaml path now stays repo-local without side effects, actual workspace roots give explicit workspace-update guidance, CodeRabbit's catch-block issue is fixed, and CI plus local build/lint/targeted tests are green.
linjinze999
pushed a commit
to linjinze999/OpenSpec
that referenced
this pull request
Jun 4, 2026
* Fix workspace.yaml collision detection * Store workspace view state under metadata * Keep top-level update out of workspace updates * Remove unused workspace root selector * Allow repo updates below workspace roots * Propagate repo state probe errors * Generalize workspace yaml collision coverage
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
2 participants
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
Fixes #1164.
This moves beta workspace view state out of the repository root and into OpenSpec-owned metadata. A project-level
workspace.yamlis now always ignored by workspace discovery, so Dagster's requiredworkspace.yamlno longer redirectsopenspec updateinto the beta workspace update path.It also removes the top-level
openspec updateauto-route into workspace update.openspec updatemay detect a real OpenSpec workspace and print guidance, but it no longer mutates workspace-local guidance, skills, view state, links, registry state, or linked repos. Workspace-local refreshes stay behind the explicitopenspec workspace updatecommand.Root Cause
The stable
openspec updatepath checkedfindWorkspaceRoot()before running the normal repo-local update. The beta workspace detector treated any ancestorworkspace.yamlas OpenSpec-owned, so Dagster projects with their own requiredworkspace.yamlwere redirected into workspace update and failed strict OpenSpec workspace-state parsing.What Changed
.openspec-workspace/view.yaml.workspace.yamlis no longer treated as OpenSpec current state or legacy state..openspec-workspace/workspace.yamlplus.openspec-workspace/local.yaml.allowUnmarkedViewState; workspace-root detection is uniformly strict for current and already-known paths.openspec updatefrom invoking workspace update; actual workspace roots now fail withopenspec workspace updateguidance and no workspace mutation.workspace.yamlfiles in workspace detection, planning-home resolution, andopenspec update.Validation
pnpm run buildpnpm run lintpnpm exec vitest run test/core/workspace/foundation.test.ts test/core/workspace/legacy-state.test.ts test/core/planning-home.test.ts test/commands/workspace.test.ts test/commands/workspace-initiative-open.test.ts test/commands/workspace.interactive.test.ts test/commands/config-profile.test.tspnpm exec vitest run test/commands/workspace.test.ts test/core/workspace/foundation.test.ts test/core/planning-home.test.ts test/core/update.test.ts test/cli-e2e/basic.test.ts test/commands/config-profile.test.tspnpm test(rerun after settled build: 89 files, 1660 tests)node dist/cli/index.js validate cli-update --specsworkspace.yamlexits 0 through normal update, remains unchanged, and no.openspec-workspace,changes, or.code-workspaceartifacts are created..openspec-workspace/view.yamlworkspace root exits 1 withopenspec workspace updateguidance and leaves view state unchanged.Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes
Documentation
Tests