feat(config): add project-scoped profile resolution and scope-aware u…#1012
Open
kaze-droid wants to merge 9 commits into
Open
feat(config): add project-scoped profile resolution and scope-aware u…#1012kaze-droid wants to merge 9 commits into
kaze-droid wants to merge 9 commits into
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:
📝 WalkthroughWalkthroughAdds project-scoped config support and key-by-key effective profile resolution (CLI → project → global → defaults), ChangesScoped Config & Profile Resolution
Workspace foundation & CLI
Completions & Generators
Docs, tests, and infra
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant ConfigCmd as Config/Update Cmd
participant Resolver as resolveEffectiveProfileSettings
participant ProjectCfg as Project Config (openspec/config.yaml/.yml)
participant GlobalCfg as Global Config (XDG JSON)
participant Defaults as Built-in Defaults
User->>CLI: run command (with optional CLI overrides / --scope)
CLI->>ConfigCmd: invoke with parsed options
ConfigCmd->>Resolver: request effective settings (pass CLI overrides + scope)
rect rgba(100,150,200,0.5)
Note over Resolver: per-key resolution (profile, delivery, workflows)
alt CLI provided value
Resolver->>Resolver: use CLI value (source: cli)
else if scope != 'global' and project config present
Resolver->>ProjectCfg: read project YAML
alt project has key
Resolver->>Resolver: use project value (source: project)
else
Resolver->>GlobalCfg: read global config
alt global has key
Resolver->>Resolver: use global value (source: global)
else
Resolver->>Defaults: use default (source: default)
end
end
else
Resolver->>GlobalCfg: read global config
alt global has key
Resolver->>Resolver: use global value (source: global)
else
Resolver->>Defaults: use default (source: default)
end
end
end
Resolver->>ConfigCmd: return EffectiveProfileSettings (with sources)
ConfigCmd->>CLI: apply/display result (perform update or write project config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (5)
src/core/project-config.ts (1)
61-65: Consider derivingProjectProfileConfigfromProjectConfigto avoid type drift.
ProjectProfileConfigredeclares the three new fields withProfile/Deliveryfromglobal-config.ts, but the source of truth for validation isProjectConfigSchemaabove (which uses inlinedz.enumliterals). If the globalProfile/Deliverytypes ever broaden, the validator and this type can silently diverge. APick-based derivation keeps them in sync:♻️ Suggested refactor
-export type ProjectProfileConfig = { - profile?: Profile; - delivery?: Delivery; - workflows?: string[]; -}; +export type ProjectProfileConfig = Pick<ProjectConfig, 'profile' | 'delivery' | 'workflows'>;The unused
Profile/Deliveryimport can then be dropped (or asserted via a compile-timesatisfiescheck against the schema enums).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/project-config.ts` around lines 61 - 65, ProjectProfileConfig currently redeclares profile/delivery/workflows and can drift from ProjectConfig/ProjectConfigSchema; change its definition to derive from ProjectConfig (e.g., use Pick<ProjectConfig, 'profile' | 'delivery' | 'workflows'>) so the types stay in sync with ProjectConfigSchema, remove the now-unused Profile/Delivery imports, and optionally add a satisfies or assertion check against ProjectConfigSchema if you want compile-time confirmation.docs/customization.md (1)
34-42: Minor: example may confuse new users.The example sets
profile: custombut lists exactly the four core workflows. Sinceprofile: corealready implies those four workflows, readers may wonder whatcustomadds here. Consider either using a non-core workflow (e.g., addingverifyornew) to demonstrate the override, or switchingprofile: custom→profile: coreand documenting thatworkflowsis only honored undercustom.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/customization.md` around lines 34 - 42, The example shows profile: custom but lists the four core workflows, which is confusing; update the example in docs to either change profile: custom to profile: core (and add a note that workflows is only honored for custom profiles) or keep profile: custom but replace or extend the workflows list with a non-core workflow (e.g., add "verify" or "new") to demonstrate how custom overrides work; edit the block that defines profile and workflows to reflect one of these two options and ensure the surrounding text clarifies that "workflows" is only applied when profile is set to custom.test/core/project-config.test.ts (1)
148-192: Optional: add invaliddeliverytest for symmetry.You cover invalid
profile(lines 148-168) and invalidworkflows(lines 170-192), but there is no equivalent test for invaliddelivery(e.g.,delivery: nonsense) verifying that the warning fires and sibling fields are preserved. Adding it would close the symmetry gap and guard against regressions in the delivery branch ofreadProjectConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/project-config.test.ts` around lines 148 - 192, Add a new unit test in test/core/project-config.test.ts that mirrors the existing cases for invalid 'profile' and 'workflows': create an openspec/config.yaml with schema: spec-driven, profile: custom (or valid), delivery: nonsense (invalid), and optionally workflows: valid; then call readProjectConfig(tempDir) and assert the returned config preserves valid sibling fields (e.g., schema and profile) and does not include the invalid delivery, and assert consoleWarnSpy was called with a message containing "Invalid 'delivery' field". Reference the existing test blocks named "should keep valid sibling profile fields when profile is invalid" and "should keep valid profile and delivery when workflows is invalid" to copy setup/teardown patterns and use readProjectConfig and consoleWarnSpy for consistency.test/core/profile-resolution.test.ts (1)
1-147: Optional: add ascopeOverride: 'project'test for completeness.The suite covers default/project/user/cli/scopeOverride='user' and the core-workflow special case, but there is no explicit test for
scopeOverride: 'project'. Today it behaves identically to "no override" (project wins, user is fallback), but documenting that contract via a test would prevent future regressions if the resolver is later tightened to e.g. suppress user fallback under explicit project scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/profile-resolution.test.ts` around lines 1 - 147, Add a new test case in profile-resolution.test.ts that calls resolveEffectiveProfileSettings with scopeOverride: 'project' and with globalConfig (profile/delivery/workflows), projectConfig (some override values or partial), and optional featureFlags; assert that projectConfig values take precedence over global (and user fallback still applies key-by-key), and that sources map those keys to 'project' (or 'user' where fallback applied). Name the test something like "honors scopeOverride='project' (project wins, user fallback preserved)" and reference resolveEffectiveProfileSettings and the scopeOverride field when constructing the input.src/commands/config.ts (1)
132-178: Confusing naming: localreadProjectConfigFilevs importedreadProjectConfig.This file uses both
readProjectConfigFile(local, returns raw YAML record) andreadProjectConfig(imported from../core/project-config.js, returns validated profile fields). They have nearly identical names but very different return shapes/semantics, and both are referenced within a few lines of each other (e.g., lines 455 vs 462 inconfig list). Consider renaming the local helper to something likereadRawProjectConfigFileorloadProjectConfigYamlto make the distinction obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 132 - 178, The helper readProjectConfigFile conflicts in name with the imported readProjectConfig; rename the local function to a clearer name such as readRawProjectConfigFile (or loadProjectConfigYaml) and update all local references (e.g., ensureProjectConfigForWrite and any other callers within this file) to use the new name; keep the same behavior and signature, and leave the imported readProjectConfig untouched so callers and imports remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/config.ts`:
- Around line 586-593: The project-scope key check is asymmetric: set honors the
allowUnknown flag but get/unset reject unknown keys; update the get and unset
handlers to accept the same --allow-unknown behavior as set by reusing the
existing check (isSupportedProjectProfileKey(key) and PROJECT_PROFILE_KEYS) and
only error when allowUnknown is false. Locate the validation in the get/unset
command blocks and change it to mirror the set logic (use allowUnknown variable,
emit the same error text and set process.exitCode = 1 only when allowUnknown is
false) so keys created with set --allow-unknown can be retrieved or removed.
- Around line 221-246: coerceProjectScopedValue currently falls back to
comma-splitting when a bracket-prefixed string fails JSON.parse, which lets
malformed names like "[propose" persist; update coerceProjectScopedValue to
treat a JSON.parse failure for inputs that start with '[' as a hard validation
error (throw or return an Error) rather than falling back to comma-splitting, so
malformed bracketed input is rejected early; reference the
coerceProjectScopedValue function and ensure validateProjectProfileValue or the
caller surface the error (or validate against ALL_WORKFLOWS in update.ts) so
invalid workflow names are not written to config.yaml.
---
Nitpick comments:
In `@docs/customization.md`:
- Around line 34-42: The example shows profile: custom but lists the four core
workflows, which is confusing; update the example in docs to either change
profile: custom to profile: core (and add a note that workflows is only honored
for custom profiles) or keep profile: custom but replace or extend the workflows
list with a non-core workflow (e.g., add "verify" or "new") to demonstrate how
custom overrides work; edit the block that defines profile and workflows to
reflect one of these two options and ensure the surrounding text clarifies that
"workflows" is only applied when profile is set to custom.
In `@src/commands/config.ts`:
- Around line 132-178: The helper readProjectConfigFile conflicts in name with
the imported readProjectConfig; rename the local function to a clearer name such
as readRawProjectConfigFile (or loadProjectConfigYaml) and update all local
references (e.g., ensureProjectConfigForWrite and any other callers within this
file) to use the new name; keep the same behavior and signature, and leave the
imported readProjectConfig untouched so callers and imports remain correct.
In `@src/core/project-config.ts`:
- Around line 61-65: ProjectProfileConfig currently redeclares
profile/delivery/workflows and can drift from ProjectConfig/ProjectConfigSchema;
change its definition to derive from ProjectConfig (e.g., use
Pick<ProjectConfig, 'profile' | 'delivery' | 'workflows'>) so the types stay in
sync with ProjectConfigSchema, remove the now-unused Profile/Delivery imports,
and optionally add a satisfies or assertion check against ProjectConfigSchema if
you want compile-time confirmation.
In `@test/core/profile-resolution.test.ts`:
- Around line 1-147: Add a new test case in profile-resolution.test.ts that
calls resolveEffectiveProfileSettings with scopeOverride: 'project' and with
globalConfig (profile/delivery/workflows), projectConfig (some override values
or partial), and optional featureFlags; assert that projectConfig values take
precedence over global (and user fallback still applies key-by-key), and that
sources map those keys to 'project' (or 'user' where fallback applied). Name the
test something like "honors scopeOverride='project' (project wins, user fallback
preserved)" and reference resolveEffectiveProfileSettings and the scopeOverride
field when constructing the input.
In `@test/core/project-config.test.ts`:
- Around line 148-192: Add a new unit test in test/core/project-config.test.ts
that mirrors the existing cases for invalid 'profile' and 'workflows': create an
openspec/config.yaml with schema: spec-driven, profile: custom (or valid),
delivery: nonsense (invalid), and optionally workflows: valid; then call
readProjectConfig(tempDir) and assert the returned config preserves valid
sibling fields (e.g., schema and profile) and does not include the invalid
delivery, and assert consoleWarnSpy was called with a message containing
"Invalid 'delivery' field". Reference the existing test blocks named "should
keep valid sibling profile fields when profile is invalid" and "should keep
valid profile and delivery when workflows is invalid" to copy setup/teardown
patterns and use readProjectConfig and consoleWarnSpy for consistency.
🪄 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: 5cfc2f53-b567-4c33-9105-a5d369d21067
📒 Files selected for processing (13)
docs/cli.mddocs/customization.mdsrc/cli/index.tssrc/commands/config.tssrc/core/completions/command-registry.tssrc/core/profile-resolution.tssrc/core/project-config.tssrc/core/update.tstest/commands/config-profile.test.tstest/commands/config.test.tstest/core/profile-resolution.test.tstest/core/project-config.test.tstest/core/update.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/config.ts (1)
359-377:⚠️ Potential issue | 🟡 MinorDrift warning text can be inaccurate when scope is
projectbut no project config exists.
maybeWarnConfigDriftselects wording purely on the passedscope. In the interactiveprofileflow,scopeis the user-supplied CLI scope, so runningopenspec config --scope project profilein a repo withoutopenspec/config.yamlwill surface "Project config is not applied to this project." even though the effective state was actually resolved from the user/global config. Consider gating the "Project config" wording on the actual presence of a project config (or oneffective.sources.*indicating a project-sourced value) so the message reflects reality.🛡️ Possible adjustment
function maybeWarnConfigDrift( projectDir: string, state: ProfileState, colorize: (message: string) => string, - scope: ConfigScope = 'user' + scope: ConfigScope = 'user' ): void { const openspecDir = path.join(projectDir, OPENSPEC_DIR_NAME); if (!fs.existsSync(openspecDir)) { return; } if (!hasProjectConfigDrift(projectDir, state.workflows, state.delivery)) { return; } + const projectConfigPresent = resolveProjectConfigFilePath(projectDir).exists; const message = - scope === 'project' + scope === 'project' && projectConfigPresent ? 'Warning: Project config is not applied to this project. Run `openspec update` to sync.' : 'Warning: User config is not applied to this project. Run `openspec update` to sync.'; console.log(colorize(message)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 359 - 377, maybeWarnConfigDrift currently chooses the "Project config" wording solely from the passed scope; change it so when scope === 'project' you first verify an actual project config exists or that the effective state indicates project-sourced values before using the project-specific message. Concretely, in maybeWarnConfigDrift check for the presence of the concrete project config file (e.g. path.join(openspecDir, 'config.yaml')) or consult state.effective.sources (or similar indicator) to determine if any values are sourced from the project, and fall back to the user-scoped message when no project config/source is present; keep the existing hasProjectConfigDrift check for drift detection and only adjust the message selection logic in maybeWarnConfigDrift.
🧹 Nitpick comments (3)
src/commands/config.ts (3)
117-126: Silent precedence betweenconfig.yamlandconfig.ymlcan confuse users.When both files exist in
openspec/,.yamlis preferred and.ymlis silently ignored — including byset/unset/profilewrites, which will only update the.yamlvariant while the.ymlfile becomes stale and unreferenced. Consider warning (or erroring) when both exist so the user can resolve the ambiguity instead of silently editing one and leaving the other to drift.🛡️ Suggested guardrail
function resolveProjectConfigFilePath(projectDir: string): { path: string; exists: boolean } { const { yamlPath, ymlPath } = getProjectConfigFilePaths(projectDir); - if (fs.existsSync(yamlPath)) { - return { path: yamlPath, exists: true }; - } - if (fs.existsSync(ymlPath)) { - return { path: ymlPath, exists: true }; - } - return { path: yamlPath, exists: false }; + const yamlExists = fs.existsSync(yamlPath); + const ymlExists = fs.existsSync(ymlPath); + if (yamlExists && ymlExists) { + throw new Error( + `Both ${path.relative(projectDir, yamlPath)} and ${path.relative(projectDir, ymlPath)} exist. Remove one to disambiguate the project config.` + ); + } + if (yamlExists) return { path: yamlPath, exists: true }; + if (ymlExists) return { path: ymlPath, exists: true }; + return { path: yamlPath, exists: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 117 - 126, resolveProjectConfigFilePath currently prefers config.yaml over config.yml silently which can lead to stale unedited files; update resolveProjectConfigFilePath (and the write paths used by set/unset/profile) to detect when both getProjectConfigFilePaths() returns existing yamlPath and ymlPath, and in that case either throw an explicit error or emit a clear warning asking the user to remove/merge one before proceeding so writes are not ambiguous; ensure the error/warning is surfaced from resolveProjectConfigFilePath and the write-handling functions (e.g., the set/unset/profile handlers) so no writes occur without the user resolving the ambiguity.
459-494:config list --scope project --jsonis asymmetric with--scope user --json.For user scope,
--jsonprints the resolvedgetGlobalConfig()(which is effectively the runtime config). For project scope,--jsonprints only the raw on-disk YAML object — without any of the effective values, source attribution, or fallbacks shown in the plain-text branch. Anyone scripting against--scope project --json(e.g. CI tooling that readsdelivery/workflows) will get nothing useful when the project file is empty or only sets a subset of keys.Consider emitting a structured payload that includes both raw and effective views, e.g.:
♻️ Possible shape
if (options.json) { - console.log(JSON.stringify(projectFile.content, null, 2)); + console.log( + JSON.stringify( + { + raw: projectFile.content, + effective: { + profile: effective.profile, + delivery: effective.delivery, + workflows: effective.workflows, + sources: effective.sources, + }, + }, + null, + 2 + ) + ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 459 - 494, The current --scope project --json branch prints only the raw on-disk object (projectFile.content); change it to emit a structured JSON payload containing both the raw file and the resolved effective configuration so scripts can read fallbacks and source attribution. Replace the JSON branch (the options.json check that currently does console.log(JSON.stringify(projectFile.content,...))) to output something like JSON.stringify({ raw: projectFile.content, effective }, null, 2), where effective is the value produced by resolveEffectiveProfileSettings({ projectConfig, globalConfig: getGlobalConfig() }) and includes effective.profile, effective.delivery, effective.workflows and effective.sources (the same object used for the plain-text branch and formatSource/CORE_WORKFLOWS logic).
165-170: Preserve YAML comments and key ordering inwriteProjectConfigFile.
parseYaml→ mutate plain object →stringifyYamldiscards YAML comments, custom key ordering, and formatting preferences. For a human-editable config file in the user's repo, this means any comments or organization inopenspec/config.yamlare silently lost on first write viaopenspec config --scope project set …or the interactiveprofileflow.Use
yaml's Document API instead:Suggested implementation using parseDocument
import { parseDocument } from 'yaml'; // read const doc = parseDocument(fileContent); // mutate via doc.setIn(['profile'], 'core'), doc.deleteIn(['workflows']), etc. // write fs.writeFileSync(file.path, doc.toString(), 'utf-8');This preserves comments, ordering, and formatting on round-trip. Requires refactoring
setNestedValueanddeleteNestedValueto work with the Document API's YAMLMap/YAMLSeq types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 165 - 170, The current writeProjectConfigFile uses parseYaml/stringifyYaml which loses YAML comments, ordering and formatting; switch to yaml's Document API: read the file with parseDocument, perform mutations on the Document (use doc.setIn and doc.deleteIn or adapt setNestedValue/deleteNestedValue to operate on YAMLMap/YAMLSeq and Document), then write back using doc.toString() in writeProjectConfigFile so comments and key order are preserved; update any callers that rely on plain objects (e.g., functions named setNestedValue and deleteNestedValue) to accept and modify the YAML Document/YAMLMap types accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/commands/config.ts`:
- Around line 359-377: maybeWarnConfigDrift currently chooses the "Project
config" wording solely from the passed scope; change it so when scope ===
'project' you first verify an actual project config exists or that the effective
state indicates project-sourced values before using the project-specific
message. Concretely, in maybeWarnConfigDrift check for the presence of the
concrete project config file (e.g. path.join(openspecDir, 'config.yaml')) or
consult state.effective.sources (or similar indicator) to determine if any
values are sourced from the project, and fall back to the user-scoped message
when no project config/source is present; keep the existing
hasProjectConfigDrift check for drift detection and only adjust the message
selection logic in maybeWarnConfigDrift.
---
Nitpick comments:
In `@src/commands/config.ts`:
- Around line 117-126: resolveProjectConfigFilePath currently prefers
config.yaml over config.yml silently which can lead to stale unedited files;
update resolveProjectConfigFilePath (and the write paths used by
set/unset/profile) to detect when both getProjectConfigFilePaths() returns
existing yamlPath and ymlPath, and in that case either throw an explicit error
or emit a clear warning asking the user to remove/merge one before proceeding so
writes are not ambiguous; ensure the error/warning is surfaced from
resolveProjectConfigFilePath and the write-handling functions (e.g., the
set/unset/profile handlers) so no writes occur without the user resolving the
ambiguity.
- Around line 459-494: The current --scope project --json branch prints only the
raw on-disk object (projectFile.content); change it to emit a structured JSON
payload containing both the raw file and the resolved effective configuration so
scripts can read fallbacks and source attribution. Replace the JSON branch (the
options.json check that currently does
console.log(JSON.stringify(projectFile.content,...))) to output something like
JSON.stringify({ raw: projectFile.content, effective }, null, 2), where
effective is the value produced by resolveEffectiveProfileSettings({
projectConfig, globalConfig: getGlobalConfig() }) and includes
effective.profile, effective.delivery, effective.workflows and effective.sources
(the same object used for the plain-text branch and formatSource/CORE_WORKFLOWS
logic).
- Around line 165-170: The current writeProjectConfigFile uses
parseYaml/stringifyYaml which loses YAML comments, ordering and formatting;
switch to yaml's Document API: read the file with parseDocument, perform
mutations on the Document (use doc.setIn and doc.deleteIn or adapt
setNestedValue/deleteNestedValue to operate on YAMLMap/YAMLSeq and Document),
then write back using doc.toString() in writeProjectConfigFile so comments and
key order are preserved; update any callers that rely on plain objects (e.g.,
functions named setNestedValue and deleteNestedValue) to accept and modify the
YAML Document/YAMLMap types accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d62c9d1-3b42-4a71-ad06-f12b297b330c
📒 Files selected for processing (8)
docs/customization.mdsrc/commands/config.tssrc/core/completions/command-registry.tssrc/core/project-config.tstest/commands/config-profile.test.tstest/commands/config.test.tstest/core/profile-resolution.test.tstest/core/project-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/core/profile-resolution.test.ts
- docs/customization.md
- src/core/project-config.ts
- test/core/project-config.test.ts
- test/commands/config.test.ts
- test/commands/config-profile.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/project-config.ts (1)
92-114:⚠️ Potential issue | 🟡 MinorWarning/error messages hardcode
config.yamleven when reading.yml.
configPathmay resolve toopenspec/config.yml, but the warnings at line 112 ("openspec/config.yaml is not a valid YAML object") and line 216 ("Failed to parse openspec/config.yaml") always reference the.yamlfilename. This will mislead users who only have a.ymlvariant and hit a parse error.🛠️ Suggested fix
- if (!raw || typeof raw !== 'object') { - console.warn(`openspec/config.yaml is not a valid YAML object`); + const relPath = path.relative(projectRoot, configPath); + if (!raw || typeof raw !== 'object') { + console.warn(`${relPath} is not a valid YAML object`); return null; }And similarly on line 216:
- console.warn(`Failed to parse openspec/config.yaml:`, error); + console.warn(`Failed to parse ${path.relative(projectRoot, configPath)}:`, error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/project-config.ts` around lines 92 - 114, The warning/error messages in readProjectConfig always mention "openspec/config.yaml" even when configPath points to openspec/config.yml; update the messages to use the actual filename by deriving the basename from configPath (e.g., via path.basename(configPath)) and replace the hardcoded strings in readProjectConfig (the console.warn about invalid YAML object and the error log for parse failure) so they reference the computed filename instead of "config.yaml".
🧹 Nitpick comments (1)
test/commands/config.test.ts (1)
416-428: Win32 path assertion is largely tautological.Because the test runs on POSIX in CI but normalizes both sides through
path.win32.normalize/path.win32.join, this test would pass regardless of whether the implementation usespath.join,path.posix.join,path.win32.join, or simple string concatenation with/— any combination yields equivalent strings afterwin32.normalize. The test name promises "cross-platform join semantics" but the assertion only proves the result is parseable as a Windows path.Consider asserting the literal expected segments without renormalizing the actual value, or driving the test on actual Windows in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/config.test.ts` around lines 416 - 428, The current test for getProjectConfigFilePaths is tautological because it normalizes both actual and expected with path.win32, so it cannot verify cross-platform join behavior; change the assertions to compare the raw returned paths to a literal Windows-style expected string or to examine path segments instead of normalizing both sides: call getProjectConfigFilePaths(windowsLikeRoot) and assert paths.yamlPath and paths.ymlPath strictly equal a constructed Windows-style string (e.g., use path.win32.join to build only the expected value and do NOT normalize the actual), or split the returned path with path.win32.sep and assert the expected segments ['C:\\repo\\sample-project','openspec','config.yaml'] are present; update the test case around getProjectConfigFilePaths to use this non-normalizing comparison so the test actually verifies Windows join semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/project-config.ts`:
- Around line 92-114: The warning/error messages in readProjectConfig always
mention "openspec/config.yaml" even when configPath points to
openspec/config.yml; update the messages to use the actual filename by deriving
the basename from configPath (e.g., via path.basename(configPath)) and replace
the hardcoded strings in readProjectConfig (the console.warn about invalid YAML
object and the error log for parse failure) so they reference the computed
filename instead of "config.yaml".
---
Nitpick comments:
In `@test/commands/config.test.ts`:
- Around line 416-428: The current test for getProjectConfigFilePaths is
tautological because it normalizes both actual and expected with path.win32, so
it cannot verify cross-platform join behavior; change the assertions to compare
the raw returned paths to a literal Windows-style expected string or to examine
path segments instead of normalizing both sides: call
getProjectConfigFilePaths(windowsLikeRoot) and assert paths.yamlPath and
paths.ymlPath strictly equal a constructed Windows-style string (e.g., use
path.win32.join to build only the expected value and do NOT normalize the
actual), or split the returned path with path.win32.sep and assert the expected
segments ['C:\\repo\\sample-project','openspec','config.yaml'] are present;
update the test case around getProjectConfigFilePaths to use this
non-normalizing comparison so the test actually verifies Windows join semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2863400f-be56-4d12-a01a-a777b138a2fc
📒 Files selected for processing (13)
docs/cli.mddocs/customization.mdsrc/cli/index.tssrc/commands/config.tssrc/core/completions/command-registry.tssrc/core/profile-resolution.tssrc/core/project-config.tssrc/core/update.tstest/commands/config-profile.test.tstest/commands/config.test.tstest/core/profile-resolution.test.tstest/core/project-config.test.tstest/core/update.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/core/profile-resolution.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cli/index.ts
- docs/customization.md
- src/core/completions/command-registry.ts
- test/core/update.test.ts
- src/core/update.ts
- src/core/profile-resolution.ts
- src/commands/config.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/project-config.ts (2)
205-212: Optional: filter empty string entries inworkflowsfor consistency with rules parsing.The
rulesparser at line 162 strips empty strings out of arrays and warns;workflowsaccepts them as-is. An accidentalworkflows: ["propose", "", "explore"]will silently keep the empty entry, which downstream profile resolution likely treats as a no-op or invalid workflow id. Consider mirroring the rules behavior.♻️ Proposed change
if (raw.workflows !== undefined) { const workflowsResult = z.array(z.string()).safeParse(raw.workflows); if (workflowsResult.success) { - config.workflows = workflowsResult.data; + const validWorkflows = workflowsResult.data.filter((w) => w.length > 0); + if (validWorkflows.length < workflowsResult.data.length) { + console.warn(`Some 'workflows' entries are empty strings, ignoring them`); + } + if (validWorkflows.length > 0) { + config.workflows = validWorkflows; + } } else { console.warn(`Invalid 'workflows' field in config (must be an array of strings)`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/project-config.ts` around lines 205 - 212, The workflows handling accepts empty strings but should mirror the rules parser by filtering them out and warning when any are removed: after validating raw.workflows with workflowsResult (from z.array(z.string()).safeParse), set config.workflows to workflowsResult.data.filter(s => s.trim() !== '') and if the filtered length is smaller than workflowsResult.data.length call console.warn (or the same warning pattern used for rules) to indicate empty workflow entries were removed; update the logic around raw.workflows, workflowsResult, and config.workflows accordingly.
187-212: DRY: reuse the schema's per-field shapes for resilient parsing.The enum members for
profile/deliveryand thez.array(z.string())shape forworkflowsare now defined twice — once inProjectConfigSchema(lines 41–56) and again here as inlinesafeParseschemas. A future addition (e.g. a new profile value) only needs to be updated in one place to silently diverge. Either extract module-level constants and reference them from both the schema and the resilient parsers, or pull them off the schema itself viaProjectConfigSchema.shape.profile.unwrap()etc.♻️ Example refactor sketch
+const PROFILE_FIELD = z.enum(['core', 'custom']); +const DELIVERY_FIELD = z.enum(['both', 'skills', 'commands']); +const WORKFLOWS_FIELD = z.array(z.string()); + export const ProjectConfigSchema = z.object({ ... - profile: z.enum(['core', 'custom']).optional().describe(...), - delivery: z.enum(['both', 'skills', 'commands']).optional().describe(...), - workflows: z.array(z.string()).optional().describe(...), + profile: PROFILE_FIELD.optional().describe(...), + delivery: DELIVERY_FIELD.optional().describe(...), + workflows: WORKFLOWS_FIELD.optional().describe(...), }); ... - const profileResult = z.enum(['core', 'custom']).safeParse(raw.profile); + const profileResult = PROFILE_FIELD.safeParse(raw.profile); ... - const deliveryResult = z.enum(['both', 'skills', 'commands']).safeParse(raw.delivery); + const deliveryResult = DELIVERY_FIELD.safeParse(raw.delivery); ... - const workflowsResult = z.array(z.string()).safeParse(raw.workflows); + const workflowsResult = WORKFLOWS_FIELD.safeParse(raw.workflows);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/project-config.ts` around lines 187 - 212, The parsing blocks in ProjectConfigSchema (profile/delivery/workflows) duplicate the per-field shapes used in the resilient safeParse checks; update the safeParse calls to reuse the schema's actual shapes instead of re-declaring them — e.g., get the enum/array shapes from ProjectConfigSchema.shape.profile / .delivery / .workflows (or extract module-level constants like PROFILE_SHAPE, DELIVERY_SHAPE, WORKFLOWS_SHAPE and use those in both the ProjectConfigSchema and the safeParse calls), then replace the inline z.enum(...) and z.array(z.string()) usages in the safeParse blocks with those referenced shapes so future enum/shape changes remain single-source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/project-config.ts`:
- Around line 205-212: The workflows handling accepts empty strings but should
mirror the rules parser by filtering them out and warning when any are removed:
after validating raw.workflows with workflowsResult (from
z.array(z.string()).safeParse), set config.workflows to
workflowsResult.data.filter(s => s.trim() !== '') and if the filtered length is
smaller than workflowsResult.data.length call console.warn (or the same warning
pattern used for rules) to indicate empty workflow entries were removed; update
the logic around raw.workflows, workflowsResult, and config.workflows
accordingly.
- Around line 187-212: The parsing blocks in ProjectConfigSchema
(profile/delivery/workflows) duplicate the per-field shapes used in the
resilient safeParse checks; update the safeParse calls to reuse the schema's
actual shapes instead of re-declaring them — e.g., get the enum/array shapes
from ProjectConfigSchema.shape.profile / .delivery / .workflows (or extract
module-level constants like PROFILE_SHAPE, DELIVERY_SHAPE, WORKFLOWS_SHAPE and
use those in both the ProjectConfigSchema and the safeParse calls), then replace
the inline z.enum(...) and z.array(z.string()) usages in the safeParse blocks
with those referenced shapes so future enum/shape changes remain single-source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2073b38-9907-476c-8bd0-7ec44ca73024
📒 Files selected for processing (3)
src/core/project-config.tstest/commands/config.test.tstest/core/project-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/project-config.test.ts
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This is the right direction for #914. Project-scoped profile/workflow config is the correct fix for shared repos where global config causes drift.
A few things I’d want tightened before this is mergeable:
- This is currently merge-conflicting, and main has moved around profile/workflow defaults since this PR was opened. I’d rebase first, then review against current main.
- The key-by-key fallback means
profile: customin project config can silently inherit the developer’s globalworkflowsif the project does not set them. For a feature meant to make repo behavior deterministic, that feels risky. I’d either require/warn when projectcustomhas no projectworkflows, or document that inheritance very explicitly. config.yamlvsconfig.ymlpath/ambiguity handling is duplicated betweencommands/config.tsandcore/project-config.ts. I’d rather have one exported project-config helper so CLI writes and runtime reads cannot drift.- The new “both config.yaml and config.yml is an error” behavior is probably right long-term, but it is a compatibility change from “prefer config.yaml”. If we keep it, it needs a clear error path for
openspec updateand a short docs/changelog note. - Please add one test that checks generated output, not just resolved config: global has one workflow set, project has another, plain
openspec updatefollows project, andopenspec update --scope globalignores project.
Net: good product shape, comment-level review for now. I would not approve until it is rebased and the project custom fallback semantics are explicit.
kaze-droid
force-pushed
the
feat/scope-aware-profile-resolution
branch
from
297dd08 to
9169b45
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
src/core/parsers/spec-structure.ts (1)
4-4:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRegex pattern inconsistency - root cause.
This regex uses
\s+(requiring at least one space after###), whilerequirement-blocks.tsline 19 andspecs-apply.tsline 290 both use\s*(allowing zero spaces). This inconsistency means###Requirement: Fooheaders will be handled differently across parsers.See the detailed comment on
specs-apply.ts:290for the full impact and fix.🤖 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/parsers/spec-structure.ts` at line 4, The REQUIREMENT_HEADER regex in spec-structure.ts uses `\s+` after `###` which requires at least one space and is inconsistent with other parsers; update the pattern for the constant REQUIREMENT_HEADER to use `\s*` (allowing zero or more spaces) so headers like `###Requirement: Foo` are parsed the same as in requirement-blocks.ts and specs-apply.ts (refer to REQUIREMENT_HEADER to locate the constant).
🧹 Nitpick comments (4)
test/core/parsers/requirement-blocks.test.ts (1)
1-47: ⚡ Quick winLGTM - Good regression coverage for case-insensitive and no-space variants.
The test suite properly validates:
- Canonical
### Requirement:headers- Mixed-case variants (
### requirement:,### REQUIREMENT:)- No-space variant (
###Requirement:)- Multiple blocks with mixed styles
- Delta spec parsing with no-space headers
💡 Optional: Add test for section boundary behavior
Consider adding a test case to verify that requirement parsing stops at the next top-level
##section (the boundary check added at requirement-blocks.ts:79 and :187):it('stops parsing requirement body at next top-level section', () => { const content = `## Requirements ### Requirement: First The system SHALL first. ## Purpose This should not be part of First requirement.`; const result = extractRequirementsSection(content); expect(result.bodyBlocks.length).toBe(1); expect(result.bodyBlocks[0].raw).not.toContain('Purpose'); expect(result.after).toContain('## Purpose'); });This would document and protect the boundary-detection behavior introduced in this PR.
🤖 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/parsers/requirement-blocks.test.ts` around lines 1 - 47, Add a test to verify requirement parsing stops at the next top-level "## " section: in test/core/parsers/requirement-blocks.test.ts add an it block that calls extractRequirementsSection with content containing "## Requirements", a "### Requirement: First" block, and then a following "## Purpose" section; assert result.bodyBlocks.length === 1, assert result.bodyBlocks[0].raw does not contain "Purpose" (or that the requirement's text ends before the "## Purpose" marker), and assert result.after contains "## Purpose" (or the remainder starts with that marker); use extractRequirementsSection and parseDeltaSpec symbols from the file to locate the parser behavior to protect.test/core/update.test.ts (1)
1560-1588: ⚡ Quick winDrop the duplicated scope-precedence cases.
These two blocks repeat the exact scenarios already covered at Line 1430 and Line 1491. Keeping both copies just doubles the runtime and makes future failures harder to triage.
Also applies to: 1723-1755
🤖 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/update.test.ts` around lines 1560 - 1588, Remove the duplicated test cases that repeat the same scope-precedence scenarios: find and keep only one instance of the "should let project profile settings override global settings by default" it block (the one that sets profile: custom in openspec/config.yaml and asserts openspec-explore SKILL.md exists while openspec-propose does not), and delete the other identical copies (including the similar duplicate group later referenced). Ensure only a single test with that exact setup and assertions remains so the behavior is still covered once.src/core/workspace/open-surface.ts (1)
60-90: 💤 Low valueMarker replacement only handles a single guidance block — minor edge case.
indexOfreturns the first occurrence, so ifAGENTS.mdaccidentally ends up with more than one START/END pair (e.g., a botched merge or manual edit), only the first range is replaced and the trailing orphan markers remain in place forever. The current behavior is safe (no data loss), just non-self-healing. Consider scanning all occurrences and rejecting / collapsing duplicates if you want to defend against that.🤖 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/open-surface.ts` around lines 60 - 90, applyWorkspaceGuidanceBlock currently uses indexOf to find only the first WORKSPACE_GUIDANCE_START_MARKER/WORKSPACE_GUIDANCE_END_MARKER pair, so duplicate/orphan markers are left behind; update applyWorkspaceGuidanceBlock (and continue to use buildWorkspaceGuidanceBlock) to scan the entire existingContent for all occurrences of WORKSPACE_GUIDANCE_START_MARKER and WORKSPACE_GUIDANCE_END_MARKER (e.g., with a global search or loop using indexOf) and then either (a) validate that start/end counts match and there is exactly one pair and throw on mismatches, or (b) collapse multiple pairs by removing all existing marker sections and inserting a single block — implement one of these behaviors, ensuring you reference WORKSPACE_GUIDANCE_START_MARKER and WORKSPACE_GUIDANCE_END_MARKER when locating/removing ranges and return the content with exactly one block inserted.src/core/workspace/foundation.ts (1)
242-263: 💤 Low valuePrefer
z.strictObject()over chained.strict()for Zod 4.In Zod 4.0.17 (your pinned version), chained
.strict()/.passthrough()/.strip()methods remain available for backwards compatibility but are considered legacy. The recommended approach is the top-levelz.strictObject({...})(andz.looseObject({...})). The refactor is functionally identical and non-breaking, but aligns your schemas with Zod 4 idiom.♻️ Proposed refactor
-const SharedStateSchema = z.object({ +const SharedStateSchema = z.strictObject({ version: z.literal(1), name: z.string(), links: z.record(z.string(), PlainObjectSchema), -}).strict(); +}); -const LocalStateSchema = z.object({ +const LocalStateSchema = z.strictObject({ version: z.literal(1), paths: z.record(z.string(), z.string()), preferred_opener: z - .object({ + .strictObject({ kind: z.enum(['agent', 'editor']), id: z.string(), }) - .strict() .optional(), -}).strict(); +}); -const RegistryStateSchema = z.object({ +const RegistryStateSchema = z.strictObject({ version: z.literal(1), workspaces: z.record(z.string(), z.string()), -}).strict(); +});🤖 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 242 - 263, The Zod schemas SharedStateSchema, LocalStateSchema, and RegistryStateSchema use the legacy chained .strict() pattern; change each to use z.strictObject({...}) (and preserve inner .strict() where needed for nested objects such as the preferred_opener object by replacing that nested .object({...}).strict() with z.strictObject({...})). Update SharedStateSchema, LocalStateSchema (including its preferred_opener), and RegistryStateSchema to be defined with z.strictObject to match Zod 4 idiomatic usage while keeping the same shape and types.
🤖 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 `@docs/customization.md`:
- Line 304: Replace the Linux-specific path in the sentence that mentions global
schemas (the line containing "~/.local/share/openspec/schemas/") with a
platform-neutral placeholder such as "<global-data-dir>/schemas/" or provide
OS-specific examples (e.g., Linux: "~/.local/share/openspec/schemas/", macOS:
"~/Library/Application Support/openspec/schemas/", Windows:
"%APPDATA%\\openspec\\schemas\\") so the docs are cross-platform; update the
surrounding sentence to recommend project-level schemas while referencing the
neutral placeholder or the listed OS examples.
In `@README.md`:
- Line 120: Replace the lowercase repository reference "github/spec-kit" in the
README text with the correctly capitalized "GitHub/spec-kit" so user-facing docs
use the proper platform name; search for the exact string "github/spec-kit" (the
reference inside the sentence mentioning the spec-kit community extension
catalog) and update it to "GitHub/spec-kit".
In `@src/commands/config.ts`:
- Around line 221-244: The validateProjectProfileValue function currently only
verifies that "workflows" is an array of strings; update it to also validate
each workflow ID against the canonical set of allowed workflow IDs (e.g., a
knownWorkflows array or a validateWorkflowId helper) and return invalid with a
clear error if any id is unknown or malformed. Specifically, inside the key ===
'workflows' branch (function validateProjectProfileValue) iterate the items,
call the existing/added validator (e.g., validateWorkflowId or check membership
in knownWorkflows) and if any fail return { valid: false, error: 'workflows
contains unknown or invalid workflow id: <id>' } otherwise return { valid: true
}.
In `@src/commands/schema.ts`:
- Around line 871-888: Call resolveProjectConfigPath(projectRoot) and validate
the result before any schema files/directories are created, and if it reports an
ambiguous/missing config path, abort early to avoid leaving partial state;
replace the current parseYaml/stringifyYaml2 usage with YAML's Document API (use
parseDocument and document.toString()) so existing comments and key order are
preserved when updating config.schema, and use the same stringifyYaml writer
path (stringifyYaml) consistently; ensure read/write still uses
fs.readFileSync/fs.writeFileSync but update the in-memory handling to work with
YAML.Document and bail out on resolveProjectConfigPath errors instead of
proceeding to create files.
In `@src/commands/workspace.ts`:
- Around line 432-459: localState.preferred_opener is returned blindly even if
its binary is no longer on PATH; change the logic in workspace open to validate
the stored opener against currently available choices (use
listWorkspaceOpenerChoices() and check choice.value or name matches
localState.preferred_opener) and only return it if still available. If it's
missing and resolveNoInteractive(options) is false and isInteractive(options) is
true, call promptPreferredOpener('Open with:', openerChoices) to let the user
pick a new one; if non-interactive, throw a WorkspaceCliError (same shape as
existing errors) explaining the preferred opener is unset/unavailable. Ensure
the checks integrate with the existing branches around
resolveNoInteractive/options and reuse listWorkspaceOpenerChoices(),
promptPreferredOpener, and WorkspaceCliError.
In `@src/commands/workspace/operations.ts`:
- Around line 178-190: The catch in validateWorkspaceNameForSetup swallows the
original validation message from validateWorkspaceName and always throws a
generic kebab-case error; change the catch to capture the thrown error (e.g.,
catch (err)) and rethrow a WorkspaceCliError that preserves the original message
(use err.message or String(err) as the message) while keeping the same error
code 'invalid_workspace_name' and target 'workspace.name' so callers still get
structured metadata but users see the precise validation reason; ensure you
handle non-Error values by falling back to a sensible default string.
In `@src/commands/workspace/selection.ts`:
- Around line 16-19: The comparison currently canonicalizes only the
registered/registry root (normalizeRegistryRootForComparison) but not the
currentWorkspaceRoot, causing misclassification on Windows; update the logic so
both paths are normalized before comparison: call
FileSystemUtils.canonicalizeExistingPath (or a new helper that wraps it safely)
for both the registered root and currentWorkspaceRoot (handling failures by
falling back to the original path or returning a non-fatal sentinel) and then
compare the canonicalized values so casing/realpath differences don't
misclassify or throw (adjust any code paths that expect thrown errors so stale
registry paths trigger the warning path instead).
In `@src/core/project-config.ts`:
- Around line 229-235: The workflows parsing currently accepts blank strings in
raw.workflows (seen in raw.workflows, workflowsResult, config.workflows); change
the validation to reject or strip-and-reject blank entries by tightening the Zod
schema or post-filtering: either replace z.array(z.string()) with a schema that
trims and enforces non-empty strings (e.g., transform/trim + nonempty/refine)
or, after workflowsResult.success, map/trim and filter out empty entries and
warn/reject if any were blank; then assign the cleaned array to config.workflows
(and log a warning when blank entries are found).
In `@src/core/specs-apply.ts`:
- Line 290: The regex in spec-structure.ts (used by findMainSpecStructureIssues)
requires a space after the "###" (`\s+`) causing inconsistency with
requirement-blocks.ts and specs-apply.ts which accept zero or more spaces;
update the pattern in spec-structure.ts to use `\s*` so it accepts both `###
Requirement:` and `###Requirement:` variants, ensuring all three parsers
(requirement-blocks, specs-apply, and spec-structure) use the same
case-insensitive, optional-space regex.
In `@test/commands/completion.test.ts`:
- Around line 247-253: The test currently asserts an exact output string for
command.complete({ type: 'schemas' }) which is brittle; update the assertion so
it checks that the consoleLogSpy received output that includes the expected
entry rather than matching the whole payload — for example, assert that
consoleLogSpy was called with a value containing 'spec-driven\tschema' (or that
some call to consoleLogSpy includes that substring) instead of expecting exact
equality; modify the test around the call to command.complete and the
expect(consoleLogSpy) assertion so it verifies inclusion using consoleLogSpy,
command.complete, and the existing test description.
---
Duplicate comments:
In `@src/core/parsers/spec-structure.ts`:
- Line 4: The REQUIREMENT_HEADER regex in spec-structure.ts uses `\s+` after
`###` which requires at least one space and is inconsistent with other parsers;
update the pattern for the constant REQUIREMENT_HEADER to use `\s*` (allowing
zero or more spaces) so headers like `###Requirement: Foo` are parsed the same
as in requirement-blocks.ts and specs-apply.ts (refer to REQUIREMENT_HEADER to
locate the constant).
---
Nitpick comments:
In `@src/core/workspace/foundation.ts`:
- Around line 242-263: The Zod schemas SharedStateSchema, LocalStateSchema, and
RegistryStateSchema use the legacy chained .strict() pattern; change each to use
z.strictObject({...}) (and preserve inner .strict() where needed for nested
objects such as the preferred_opener object by replacing that nested
.object({...}).strict() with z.strictObject({...})). Update SharedStateSchema,
LocalStateSchema (including its preferred_opener), and RegistryStateSchema to be
defined with z.strictObject to match Zod 4 idiomatic usage while keeping the
same shape and types.
In `@src/core/workspace/open-surface.ts`:
- Around line 60-90: applyWorkspaceGuidanceBlock currently uses indexOf to find
only the first WORKSPACE_GUIDANCE_START_MARKER/WORKSPACE_GUIDANCE_END_MARKER
pair, so duplicate/orphan markers are left behind; update
applyWorkspaceGuidanceBlock (and continue to use buildWorkspaceGuidanceBlock) to
scan the entire existingContent for all occurrences of
WORKSPACE_GUIDANCE_START_MARKER and WORKSPACE_GUIDANCE_END_MARKER (e.g., with a
global search or loop using indexOf) and then either (a) validate that start/end
counts match and there is exactly one pair and throw on mismatches, or (b)
collapse multiple pairs by removing all existing marker sections and inserting a
single block — implement one of these behaviors, ensuring you reference
WORKSPACE_GUIDANCE_START_MARKER and WORKSPACE_GUIDANCE_END_MARKER when
locating/removing ranges and return the content with exactly one block inserted.
In `@test/core/parsers/requirement-blocks.test.ts`:
- Around line 1-47: Add a test to verify requirement parsing stops at the next
top-level "## " section: in test/core/parsers/requirement-blocks.test.ts add an
it block that calls extractRequirementsSection with content containing "##
Requirements", a "### Requirement: First" block, and then a following "##
Purpose" section; assert result.bodyBlocks.length === 1, assert
result.bodyBlocks[0].raw does not contain "Purpose" (or that the requirement's
text ends before the "## Purpose" marker), and assert result.after contains "##
Purpose" (or the remainder starts with that marker); use
extractRequirementsSection and parseDeltaSpec symbols from the file to locate
the parser behavior to protect.
In `@test/core/update.test.ts`:
- Around line 1560-1588: Remove the duplicated test cases that repeat the same
scope-precedence scenarios: find and keep only one instance of the "should let
project profile settings override global settings by default" it block (the one
that sets profile: custom in openspec/config.yaml and asserts openspec-explore
SKILL.md exists while openspec-propose does not), and delete the other identical
copies (including the similar duplicate group later referenced). Ensure only a
single test with that exact setup and assertions remains so the behavior is
still covered once.
🪄 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: 022f467a-ce86-4a3c-928f-a55c13948792
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (115)
.changeset/clarify-bun-node-runtime.md.changeset/kind-rings-notice.md.changeset/sync-default-core.mdCHANGELOG.mdREADME.mdWORKSPACE_REIMPLEMENTATION_DIRECTION.mdWORKSPACE_REIMPLEMENTATION_START_HERE.mddocs/cli.mddocs/commands.mddocs/concepts.mddocs/customization.mddocs/getting-started.mddocs/installation.mddocs/migration-guide.mddocs/opsx.mddocs/supported-tools.mddocs/workflows.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/.openspec.yamlopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/README.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/design.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/proposal.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/specs/ai-tool-paths/spec.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/specs/cli-init/spec.mdopenspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/tasks.mdopenspec/changes/archive/2026-05-04-workspace-foundation/design.mdopenspec/changes/archive/2026-05-04-workspace-foundation/proposal.mdopenspec/changes/archive/2026-05-04-workspace-foundation/specs/openspec-conventions/spec.mdopenspec/changes/archive/2026-05-04-workspace-foundation/specs/workspace-foundation/spec.mdopenspec/changes/archive/2026-05-04-workspace-foundation/tasks.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/design.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/proposal.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/specs/cli-artifact-workflow/spec.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/specs/workspace-foundation/spec.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/specs/workspace-links/spec.mdopenspec/changes/archive/2026-05-06-workspace-create-and-register-repos/tasks.mdopenspec/changes/archive/2026-05-06-workspace-open-agent-context/design.mdopenspec/changes/archive/2026-05-06-workspace-open-agent-context/proposal.mdopenspec/changes/archive/2026-05-06-workspace-open-agent-context/specs/workspace-foundation/spec.mdopenspec/changes/archive/2026-05-06-workspace-open-agent-context/specs/workspace-open/spec.mdopenspec/changes/archive/2026-05-06-workspace-open-agent-context/tasks.mdopenspec/changes/workspace-apply-repo-slice/proposal.mdopenspec/changes/workspace-change-planning/proposal.mdopenspec/changes/workspace-reimplementation-roadmap/POC_REFERENCE_GUIDE.mdopenspec/changes/workspace-reimplementation-roadmap/README.mdopenspec/changes/workspace-reimplementation-roadmap/proposal.mdopenspec/changes/workspace-verify-and-archive/proposal.mdopenspec/config.yamlopenspec/explorations/workspace-architecture.mdopenspec/specs/ai-tool-paths/spec.mdopenspec/specs/cli-artifact-workflow/spec.mdopenspec/specs/cli-init/spec.mdopenspec/specs/openspec-conventions/spec.mdopenspec/specs/workspace-foundation/spec.mdopenspec/specs/workspace-links/spec.mdopenspec/specs/workspace-open/spec.mdpackage.jsonsrc/cli/index.tssrc/commands/completion.tssrc/commands/config.tssrc/commands/schema.tssrc/commands/workspace.tssrc/commands/workspace/open.tssrc/commands/workspace/operations.tssrc/commands/workspace/selection.tssrc/commands/workspace/types.tssrc/core/completions/command-registry.tssrc/core/completions/completion-provider.tssrc/core/completions/generators/bash-generator.tssrc/core/completions/generators/fish-generator.tssrc/core/completions/generators/powershell-generator.tssrc/core/completions/generators/zsh-generator.tssrc/core/completions/installers/zsh-installer.tssrc/core/completions/templates/bash-templates.tssrc/core/completions/templates/fish-templates.tssrc/core/completions/templates/powershell-templates.tssrc/core/completions/templates/zsh-templates.tssrc/core/completions/types.tssrc/core/config.tssrc/core/global-config.tssrc/core/index.tssrc/core/parsers/requirement-blocks.tssrc/core/parsers/spec-structure.tssrc/core/profile-resolution.tssrc/core/profiles.tssrc/core/project-config.tssrc/core/specs-apply.tssrc/core/update.tssrc/core/workspace/foundation.tssrc/core/workspace/index.tssrc/core/workspace/link-input.tssrc/core/workspace/open-surface.tssrc/core/workspace/openers.tssrc/utils/change-metadata.tstest/cli-e2e/basic.test.tstest/commands/completion.test.tstest/commands/config-profile.test.tstest/commands/config.test.tstest/commands/schema.test.tstest/commands/workspace-open.test.tstest/commands/workspace.interactive.test.tstest/commands/workspace.test.tstest/core/completions/generators/bash-generator.test.tstest/core/completions/generators/fish-generator.test.tstest/core/completions/generators/powershell-generator.test.tstest/core/completions/generators/zsh-generator.test.tstest/core/completions/installers/zsh-installer.test.tstest/core/global-config.test.tstest/core/init.test.tstest/core/parsers/requirement-blocks.test.tstest/core/profile-resolution.test.tstest/core/profile-sync-drift.test.tstest/core/profiles.test.tstest/core/project-config.test.tstest/core/update.test.tstest/core/workspace/foundation.test.ts
✅ Files skipped from review due to trivial changes (33)
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/README.md
- .changeset/sync-default-core.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/specs/ai-tool-paths/spec.md
- .changeset/kind-rings-notice.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/.openspec.yaml
- .changeset/clarify-bun-node-runtime.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/design.md
- openspec/specs/cli-artifact-workflow/spec.md
- CHANGELOG.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/tasks.md
- openspec/explorations/workspace-architecture.md
- openspec/changes/archive/2026-05-06-workspace-create-and-register-repos/tasks.md
- openspec/changes/workspace-change-planning/proposal.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/specs/cli-init/spec.md
- openspec/specs/openspec-conventions/spec.md
- WORKSPACE_REIMPLEMENTATION_START_HERE.md
- openspec/changes/archive/2026-05-06-workspace-create-and-register-repos/specs/cli-artifact-workflow/spec.md
- openspec/changes/archive/2026-04-23-add-kimi-cli-skills-only-support/proposal.md
- openspec/changes/workspace-verify-and-archive/proposal.md
- openspec/changes/archive/2026-05-04-workspace-foundation/tasks.md
- openspec/changes/workspace-reimplementation-roadmap/README.md
- openspec/changes/archive/2026-05-06-workspace-open-agent-context/proposal.md
- src/utils/change-metadata.ts
- docs/concepts.md
- openspec/changes/archive/2026-05-06-workspace-open-agent-context/specs/workspace-open/spec.md
- openspec/changes/archive/2026-05-06-workspace-open-agent-context/specs/workspace-foundation/spec.md
- openspec/changes/workspace-apply-repo-slice/proposal.md
- docs/commands.md
- openspec/changes/archive/2026-05-04-workspace-foundation/specs/workspace-foundation/spec.md
- openspec/changes/archive/2026-05-06-workspace-open-agent-context/tasks.md
- docs/getting-started.md
- test/core/profile-resolution.test.ts
- openspec/specs/workspace-foundation/spec.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/profile-resolution.ts
- src/core/update.ts
- test/core/project-config.test.ts
- test/commands/config.test.ts
- test/commands/config-profile.test.ts
| --- | ||
|
|
||
| > **Note:** OpenSpec also supports user-level schemas at `~/.local/share/openspec/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code. | ||
| > **Note:** OpenSpec also supports global schemas at `~/.local/share/openspec/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code. |
Contributor
There was a problem hiding this comment.
Use a platform-neutral global schema location in docs.
Line 304 hardcodes a Linux-specific path (~/.local/share/openspec/schemas/). In cross-platform docs, this can send users to the wrong location on macOS/Windows. Prefer the neutral <global-data-dir>/schemas/ wording (or list OS-specific examples).
✏️ Suggested doc tweak
-> **Note:** OpenSpec also supports global schemas at `~/.local/share/openspec/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code.
+> **Note:** OpenSpec also supports global schemas at `<global-data-dir>/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code.📝 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
| > **Note:** OpenSpec also supports global schemas at `~/.local/share/openspec/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code. | |
| > **Note:** OpenSpec also supports global schemas at `<global-data-dir>/schemas/` for sharing across projects, but project-level schemas in `openspec/schemas/` are recommended since they're version-controlled with your code. |
🤖 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 `@docs/customization.md` at line 304, Replace the Linux-specific path in the
sentence that mentions global schemas (the line containing
"~/.local/share/openspec/schemas/") with a platform-neutral placeholder such as
"<global-data-dir>/schemas/" or provide OS-specific examples (e.g., Linux:
"~/.local/share/openspec/schemas/", macOS: "~/Library/Application
Support/openspec/schemas/", Windows: "%APPDATA%\\openspec\\schemas\\") so the
docs are cross-platform; update the surrounding sentence to recommend
project-level schemas while referencing the neutral placeholder or the listed OS
examples.
|
|
||
| ## Community schemas | ||
|
|
||
| Third-party schema bundles distributed via standalone repositories — these provide opinionated workflows that integrate OpenSpec with other tools, similar to how [github/spec-kit's community extension catalog](https://github.com/github/spec-kit/tree/main/extensions) handles tool integrations. |
Contributor
There was a problem hiding this comment.
Capitalize “GitHub” in the user-facing docs.
Line 120 uses github/spec-kit; this should be GitHub/spec-kit for correct platform naming in published documentation.
Suggested edit
-Third-party schema bundles distributed via standalone repositories — these provide opinionated workflows that integrate OpenSpec with other tools, similar to how [github/spec-kit's community extension catalog](https://github.com/github/spec-kit/tree/main/extensions) handles tool integrations.
+Third-party schema bundles distributed via standalone repositories — these provide opinionated workflows that integrate OpenSpec with other tools, similar to how [GitHub/spec-kit's community extension catalog](https://github.com/github/spec-kit/tree/main/extensions) handles tool integrations.📝 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
| Third-party schema bundles distributed via standalone repositories — these provide opinionated workflows that integrate OpenSpec with other tools, similar to how [github/spec-kit's community extension catalog](https://github.com/github/spec-kit/tree/main/extensions) handles tool integrations. | |
| Third-party schema bundles distributed via standalone repositories — these provide opinionated workflows that integrate OpenSpec with other tools, similar to how [GitHub/spec-kit's community extension catalog](https://github.com/github/spec-kit/tree/main/extensions) handles tool integrations. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~120-~120: The official name of this software platform is spelled with a capital “H”.
Context: ...nSpec with other tools, similar to how [github/spec-kit's community extension catalog]...
(GITHUB)
🤖 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 `@README.md` at line 120, Replace the lowercase repository reference
"github/spec-kit" in the README text with the correctly capitalized
"GitHub/spec-kit" so user-facing docs use the proper platform name; search for
the exact string "github/spec-kit" (the reference inside the sentence mentioning
the spec-kit community extension catalog) and update it to "GitHub/spec-kit".
Comment on lines
+221
to
+244
| function validateProjectProfileValue(key: string, value: unknown): { valid: boolean; error?: string } { | ||
| if (key === 'profile') { | ||
| if (value === 'core' || value === 'custom') { | ||
| return { valid: true }; | ||
| } | ||
| return { valid: false, error: 'profile must be "core" or "custom"' }; | ||
| } | ||
|
|
||
| if (key === 'delivery') { | ||
| if (value === 'both' || value === 'skills' || value === 'commands') { | ||
| return { valid: true }; | ||
| } | ||
| return { valid: false, error: 'delivery must be "both", "skills", or "commands"' }; | ||
| } | ||
|
|
||
| if (key === 'workflows') { | ||
| if (Array.isArray(value) && value.every((item) => typeof item === 'string')) { | ||
| return { valid: true }; | ||
| } | ||
| return { valid: false, error: 'workflows must be an array of strings' }; | ||
| } | ||
|
|
||
| return { valid: false, error: `Unsupported project config key "${key}"` }; | ||
| } |
Contributor
There was a problem hiding this comment.
Reject unknown workflow IDs at write time.
The workflows branch only checks for “array of strings”, so openspec config --scope project set workflows foo,bar will happily write an invalid profile override. That pushes a typo into project config and leaves later commands to ignore or fail on it instead of surfacing the mistake here.
Suggested fix
if (key === 'workflows') {
- if (Array.isArray(value) && value.every((item) => typeof item === 'string')) {
- return { valid: true };
+ if (!Array.isArray(value) || !value.every((item) => typeof item === 'string')) {
+ return { valid: false, error: 'workflows must be an array of strings' };
}
- return { valid: false, error: 'workflows must be an array of strings' };
+
+ const unknown = value.filter(
+ (item) => !ALL_WORKFLOWS.includes(item as (typeof ALL_WORKFLOWS)[number])
+ );
+ if (unknown.length > 0) {
+ return {
+ valid: false,
+ error: `unknown workflow(s): ${unknown.join(', ')}`,
+ };
+ }
+
+ return { valid: true };
}📝 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
| function validateProjectProfileValue(key: string, value: unknown): { valid: boolean; error?: string } { | |
| if (key === 'profile') { | |
| if (value === 'core' || value === 'custom') { | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: 'profile must be "core" or "custom"' }; | |
| } | |
| if (key === 'delivery') { | |
| if (value === 'both' || value === 'skills' || value === 'commands') { | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: 'delivery must be "both", "skills", or "commands"' }; | |
| } | |
| if (key === 'workflows') { | |
| if (Array.isArray(value) && value.every((item) => typeof item === 'string')) { | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: 'workflows must be an array of strings' }; | |
| } | |
| return { valid: false, error: `Unsupported project config key "${key}"` }; | |
| } | |
| function validateProjectProfileValue(key: string, value: unknown): { valid: boolean; error?: string } { | |
| if (key === 'profile') { | |
| if (value === 'core' || value === 'custom') { | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: 'profile must be "core" or "custom"' }; | |
| } | |
| if (key === 'delivery') { | |
| if (value === 'both' || value === 'skills' || value === 'commands') { | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: 'delivery must be "both", "skills", or "commands"' }; | |
| } | |
| if (key === 'workflows') { | |
| if (!Array.isArray(value) || !value.every((item) => typeof item === 'string')) { | |
| return { valid: false, error: 'workflows must be an array of strings' }; | |
| } | |
| const unknown = value.filter( | |
| (item) => !ALL_WORKFLOWS.includes(item as (typeof ALL_WORKFLOWS)[number]) | |
| ); | |
| if (unknown.length > 0) { | |
| return { | |
| valid: false, | |
| error: `unknown workflow(s): ${unknown.join(', ')}`, | |
| }; | |
| } | |
| return { valid: true }; | |
| } | |
| return { valid: false, error: `Unsupported project config key "${key}"` }; | |
| } |
🤖 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/commands/config.ts` around lines 221 - 244, The
validateProjectProfileValue function currently only verifies that "workflows" is
an array of strings; update it to also validate each workflow ID against the
canonical set of allowed workflow IDs (e.g., a knownWorkflows array or a
validateWorkflowId helper) and return invalid with a clear error if any id is
unknown or malformed. Specifically, inside the key === 'workflows' branch
(function validateProjectProfileValue) iterate the items, call the
existing/added validator (e.g., validateWorkflowId or check membership in
knownWorkflows) and if any fail return { valid: false, error: 'workflows
contains unknown or invalid workflow id: <id>' } otherwise return { valid: true
}.
Comment on lines
871
to
888
| if (options?.default) { | ||
| const configPath = path.join(projectRoot, 'openspec', 'config.yaml'); | ||
| const resolvedConfigPath = resolveProjectConfigPath(projectRoot); | ||
| const configPath = resolvedConfigPath.path; | ||
|
|
||
| if (fs.existsSync(configPath)) { | ||
| if (resolvedConfigPath.exists) { | ||
| const { parse: parseYaml, stringify: stringifyYaml2 } = await import('yaml'); | ||
| const configContent = fs.readFileSync(configPath, 'utf-8'); | ||
| const config = parseYaml(configContent) || {}; | ||
| config.defaultSchema = name; | ||
| config.schema = name; | ||
| fs.writeFileSync(configPath, stringifyYaml2(config)); | ||
| } else { | ||
| // Create config file | ||
| const configDir = path.dirname(configPath); | ||
| if (!fs.existsSync(configDir)) { | ||
| fs.mkdirSync(configDir, { recursive: true }); | ||
| } | ||
| fs.writeFileSync(configPath, stringifyYaml({ defaultSchema: name })); | ||
| fs.writeFileSync(configPath, stringifyYaml({ schema: name })); | ||
| } |
Contributor
There was a problem hiding this comment.
Resolve and validate the project config before creating schema files.
If Line 872 throws for the config.yaml/config.yml ambiguity, schema init --default has already created the new schema directory and templates earlier in the action, so the command reports failure while still leaving partial state behind. This block also reparses into a plain object and stringifies it back, which drops existing YAML comments/key ordering instead of preserving the document like the other project-scope writers do.
Suggested direction
+ let resolvedConfigPath:
+ | ReturnType<typeof resolveProjectConfigPath>
+ | undefined;
+ if (options?.default) {
+ resolvedConfigPath = resolveProjectConfigPath(projectRoot);
+ }
+
// Create schema directory
if (spinner) spinner.start(`Creating schema '${name}'...`);
fs.mkdirSync(schemaDir, { recursive: true });
...
// Update config if --default
- if (options?.default) {
- const resolvedConfigPath = resolveProjectConfigPath(projectRoot);
- const configPath = resolvedConfigPath.path;
-
- if (resolvedConfigPath.exists) {
- const { parse: parseYaml, stringify: stringifyYaml2 } = await import('yaml');
- const configContent = fs.readFileSync(configPath, 'utf-8');
- const config = parseYaml(configContent) || {};
- config.schema = name;
- fs.writeFileSync(configPath, stringifyYaml2(config));
- } else {
- // Create config file
- const configDir = path.dirname(configPath);
- if (!fs.existsSync(configDir)) {
- fs.mkdirSync(configDir, { recursive: true });
- }
- fs.writeFileSync(configPath, stringifyYaml({ schema: name }));
- }
+ if (options?.default && resolvedConfigPath) {
+ const { parseDocument } = await import('yaml');
+ const configPath = resolvedConfigPath.path;
+ const document = resolvedConfigPath.exists
+ ? parseDocument(fs.readFileSync(configPath, 'utf-8'))
+ : parseDocument('');
+
+ document.set('schema', name);
+
+ const configDir = path.dirname(configPath);
+ if (!fs.existsSync(configDir)) {
+ fs.mkdirSync(configDir, { recursive: true });
+ }
+ fs.writeFileSync(configPath, String(document));
}🤖 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/commands/schema.ts` around lines 871 - 888, Call
resolveProjectConfigPath(projectRoot) and validate the result before any schema
files/directories are created, and if it reports an ambiguous/missing config
path, abort early to avoid leaving partial state; replace the current
parseYaml/stringifyYaml2 usage with YAML's Document API (use parseDocument and
document.toString()) so existing comments and key order are preserved when
updating config.schema, and use the same stringifyYaml writer path
(stringifyYaml) consistently; ensure read/write still uses
fs.readFileSync/fs.writeFileSync but update the in-memory handling to work with
YAML.Document and bail out on resolveProjectConfigPath errors instead of
proceeding to create files.
Comment on lines
+432
to
+459
| if (localState.preferred_opener) { | ||
| return localState.preferred_opener; | ||
| } | ||
|
|
||
| if (!resolveNoInteractive(options) && isInteractive(options)) { | ||
| const openerChoices = listWorkspaceOpenerChoices().filter((choice) => choice.available); | ||
| if (openerChoices.length === 0) { | ||
| throw new WorkspaceCliError( | ||
| 'No supported workspace opener is available on PATH.', | ||
| 'workspace_no_available_openers', | ||
| { | ||
| target: 'workspace.opener', | ||
| fix: "Install VS Code ('code'), Codex ('codex'), or Claude ('claude'), then retry.", | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| return promptPreferredOpener('Open with:', openerChoices); | ||
| } | ||
|
|
||
| throw new WorkspaceCliError( | ||
| 'This workspace does not have a preferred opener yet.', | ||
| 'workspace_opener_unset', | ||
| { | ||
| target: 'workspace.opener', | ||
| fix: 'Pass --agent <tool> or --editor, or run workspace setup interactively to choose a default opener.', | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Fall back when the saved opener is no longer available.
Line 432 returns the stored preference before any availability check. If that binary later disappears from PATH, workspace open hard-fails even in an interactive terminal where another opener is available. This makes the saved preference brittle across PATH changes and uninstall/reinstall cycles.
🩹 Suggested fallback
if (localState.preferred_opener) {
- return localState.preferred_opener;
+ const savedValue =
+ localState.preferred_opener.kind === 'editor'
+ ? 'editor'
+ : localState.preferred_opener.id;
+ const savedChoice = listWorkspaceOpenerChoices().find(
+ (choice) => choice.value === savedValue
+ );
+
+ if (savedChoice?.available) {
+ return localState.preferred_opener;
+ }
}
if (!resolveNoInteractive(options) && isInteractive(options)) {
const openerChoices = listWorkspaceOpenerChoices().filter((choice) => choice.available);
if (openerChoices.length === 0) {
@@
- return promptPreferredOpener('Open with:', openerChoices);
+ return promptPreferredOpener(
+ localState.preferred_opener ? 'Saved opener is unavailable. Open with:' : 'Open with:',
+ openerChoices
+ );
}🤖 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/commands/workspace.ts` around lines 432 - 459,
localState.preferred_opener is returned blindly even if its binary is no longer
on PATH; change the logic in workspace open to validate the stored opener
against currently available choices (use listWorkspaceOpenerChoices() and check
choice.value or name matches localState.preferred_opener) and only return it if
still available. If it's missing and resolveNoInteractive(options) is false and
isInteractive(options) is true, call promptPreferredOpener('Open with:',
openerChoices) to let the user pick a new one; if non-interactive, throw a
WorkspaceCliError (same shape as existing errors) explaining the preferred
opener is unset/unavailable. Ensure the checks integrate with the existing
branches around resolveNoInteractive/options and reuse
listWorkspaceOpenerChoices(), promptPreferredOpener, and WorkspaceCliError.
Comment on lines
+178
to
+190
| export function validateWorkspaceNameForSetup(name: string): string { | ||
| try { | ||
| return validateWorkspaceName(name); | ||
| } catch { | ||
| throw new WorkspaceCliError( | ||
| 'Workspace name must be kebab-case with lowercase letters, numbers, and single hyphen separators.', | ||
| 'invalid_workspace_name', | ||
| { | ||
| target: 'workspace.name', | ||
| } | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
validateWorkspaceNameForSetup reports the wrong reason for non-kebab failures.
validateWorkspaceName distinguishes several failure modes (empty, ./.., contains path separators, not kebab-case) and surfaces a precise message per case. The current catch {} discards that message and always returns "must be kebab-case … hyphen separators", so a user who passes e.g. an empty name, .., or foo/bar gets a misleading hint. Preserve the underlying reason while keeping the structured invalid_workspace_name code/target.
🛠️ Proposed fix to preserve the underlying validation reason
export function validateWorkspaceNameForSetup(name: string): string {
try {
return validateWorkspaceName(name);
- } catch {
- throw new WorkspaceCliError(
- 'Workspace name must be kebab-case with lowercase letters, numbers, and single hyphen separators.',
- 'invalid_workspace_name',
- {
- target: 'workspace.name',
- }
- );
+ } catch (error) {
+ throw new WorkspaceCliError(asErrorMessage(error), 'invalid_workspace_name', {
+ target: 'workspace.name',
+ fix: 'Use kebab-case with lowercase letters, numbers, and single hyphen separators.',
+ });
}
}Note: the existing test at lines 502–512 only asserts the message stringContaining('kebab-case'), which still holds for the Bad_Name case because that branch in validateWorkspaceName produces the kebab-case message itself.
🤖 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/commands/workspace/operations.ts` around lines 178 - 190, The catch in
validateWorkspaceNameForSetup swallows the original validation message from
validateWorkspaceName and always throws a generic kebab-case error; change the
catch to capture the thrown error (e.g., catch (err)) and rethrow a
WorkspaceCliError that preserves the original message (use err.message or
String(err) as the message) while keeping the same error code
'invalid_workspace_name' and target 'workspace.name' so callers still get
structured metadata but users see the precise validation reason; ensure you
handle non-Error values by falling back to a sensible default string.
Comment on lines
+16
to
+19
| function normalizeRegistryRootForComparison(workspaceRoot: string): string { | ||
| return process.platform === 'win32' | ||
| ? FileSystemUtils.canonicalizeExistingPath(workspaceRoot) | ||
| : workspaceRoot; |
Contributor
There was a problem hiding this comment.
Normalize both roots before comparing current-vs-registered workspace paths.
Line 59 compares a canonicalized registry root against the raw currentWorkspaceRoot. On Windows that can misclassify a registered workspace as workspace_not_in_local_registry when casing/realpath normalization differs, and a stale registry path can throw here instead of falling back to the warning path.
🪟 Suggested fix
function normalizeRegistryRootForComparison(workspaceRoot: string): string {
- return process.platform === 'win32'
- ? FileSystemUtils.canonicalizeExistingPath(workspaceRoot)
- : workspaceRoot;
+ try {
+ return process.platform === 'win32'
+ ? FileSystemUtils.canonicalizeExistingPath(workspaceRoot)
+ : workspaceRoot;
+ } catch {
+ return workspaceRoot;
+ }
}
@@
const isRegistered =
registeredRoot !== undefined &&
- normalizeRegistryRootForComparison(registeredRoot) === currentWorkspaceRoot;
+ normalizeRegistryRootForComparison(registeredRoot) ===
+ normalizeRegistryRootForComparison(currentWorkspaceRoot);Also applies to: 57-59
🤖 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/commands/workspace/selection.ts` around lines 16 - 19, The comparison
currently canonicalizes only the registered/registry root
(normalizeRegistryRootForComparison) but not the currentWorkspaceRoot, causing
misclassification on Windows; update the logic so both paths are normalized
before comparison: call FileSystemUtils.canonicalizeExistingPath (or a new
helper that wraps it safely) for both the registered root and
currentWorkspaceRoot (handling failures by falling back to the original path or
returning a non-fatal sentinel) and then compare the canonicalized values so
casing/realpath differences don't misclassify or throw (adjust any code paths
that expect thrown errors so stale registry paths trigger the warning path
instead).
Comment on lines
+229
to
+235
| if (raw.workflows !== undefined) { | ||
| const workflowsResult = z.array(z.string()).safeParse(raw.workflows); | ||
| if (workflowsResult.success) { | ||
| config.workflows = workflowsResult.data; | ||
| } else { | ||
| console.warn(`Invalid 'workflows' field in config (must be an array of strings)`); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject blank workflow entries during parsing.
Line 229 currently accepts workflows entries like "" or " ". That can mark workflows as “configured” while still being invalid for resolution/execution. Sanitize or reject blank entries at parse time.
🛠️ Suggested hardening
if (raw.workflows !== undefined) {
const workflowsResult = z.array(z.string()).safeParse(raw.workflows);
if (workflowsResult.success) {
- config.workflows = workflowsResult.data;
+ const cleaned = workflowsResult.data
+ .map((workflow) => workflow.trim())
+ .filter((workflow) => workflow.length > 0);
+
+ if (cleaned.length > 0) {
+ config.workflows = cleaned;
+ } else {
+ console.warn(`Invalid 'workflows' field in config (must contain at least one non-empty workflow id)`);
+ }
+
+ if (cleaned.length < workflowsResult.data.length) {
+ console.warn(`Some 'workflows' entries are empty, ignoring them`);
+ }
} else {
console.warn(`Invalid 'workflows' field in config (must be an array of strings)`);
}
}📝 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
| if (raw.workflows !== undefined) { | |
| const workflowsResult = z.array(z.string()).safeParse(raw.workflows); | |
| if (workflowsResult.success) { | |
| config.workflows = workflowsResult.data; | |
| } else { | |
| console.warn(`Invalid 'workflows' field in config (must be an array of strings)`); | |
| } | |
| if (raw.workflows !== undefined) { | |
| const workflowsResult = z.array(z.string()).safeParse(raw.workflows); | |
| if (workflowsResult.success) { | |
| const cleaned = workflowsResult.data | |
| .map((workflow) => workflow.trim()) | |
| .filter((workflow) => workflow.length > 0); | |
| if (cleaned.length > 0) { | |
| config.workflows = cleaned; | |
| } else { | |
| console.warn(`Invalid 'workflows' field in config (must contain at least one non-empty workflow id)`); | |
| } | |
| if (cleaned.length < workflowsResult.data.length) { | |
| console.warn(`Some 'workflows' entries are empty, ignoring them`); | |
| } | |
| } else { | |
| console.warn(`Invalid 'workflows' field in config (must be an array of strings)`); | |
| } | |
| } |
🤖 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/project-config.ts` around lines 229 - 235, The workflows parsing
currently accepts blank strings in raw.workflows (seen in raw.workflows,
workflowsResult, config.workflows); change the validation to reject or
strip-and-reject blank entries by tightening the Zod schema or post-filtering:
either replace z.array(z.string()) with a schema that trims and enforces
non-empty strings (e.g., transform/trim + nonempty/refine) or, after
workflowsResult.success, map/trim and filter out empty entries and warn/reject
if any were blank; then assign the cleaned array to config.workflows (and log a
warning when blank entries are found).
Comment on lines
+247
to
+253
| describe('dynamic completion data', () => { | ||
| it('should output schema names for shell completion', async () => { | ||
| await command.complete({ type: 'schemas' }); | ||
|
|
||
| expect(consoleLogSpy).toHaveBeenCalledWith('spec-driven\tschema'); | ||
| expect(process.exitCode).toBe(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert on inclusion, not the entire schema completion payload.
complete({ type: 'schemas' }) can validly return more than one schema, so the exact-string check here will start failing as soon as another built-in schema is added or the output ordering changes.
Suggested fix
- expect(consoleLogSpy).toHaveBeenCalledWith('spec-driven\tschema');
+ expect(consoleLogSpy).toHaveBeenCalledWith(
+ expect.stringContaining('spec-driven\tschema')
+ );🤖 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/commands/completion.test.ts` around lines 247 - 253, The test currently
asserts an exact output string for command.complete({ type: 'schemas' }) which
is brittle; update the assertion so it checks that the consoleLogSpy received
output that includes the expected entry rather than matching the whole payload —
for example, assert that consoleLogSpy was called with a value containing
'spec-driven\tschema' (or that some call to consoleLogSpy includes that
substring) instead of expecting exact equality; modify the test around the call
to command.complete and the expect(consoleLogSpy) assertion so it verifies
inclusion using consoleLogSpy, command.complete, and the existing test
description.
* Include sync in core workflow defaults * Add old core custom profile sync hint * Update workflows sync default docs
…-unknown across project commands
kaze-droid
force-pushed
the
feat/scope-aware-profile-resolution
branch
from
9169b45 to
26fe85b
Compare
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
3 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.
Closes #914
openspec config --scope <scope>looked like it supported real scoping, but behavior was still effectively global-only.That made per-repo profile setup awkward.
This fixes that by separating:
What changed
projectscope support inopenspec configfor profile-related keys:profiledeliveryworkflowsglobal|projectacross config/update CLI, completions, and docs.--scope global: global config -> defaults--scope project): project config -> global config -> defaultsopenspec updateto use scope-aware effective resolution.openspec update --scope global|projectfor explicit run-time override.openspec/config.yamlandopenspec/config.ymlexist.config list --scope project --jsonto include bothrawandeffective.How it was tested
Added and expanded tests covering:
updatebehavior with default/project precedence and explicit--scope global|projectprofile,delivery,workflowsconfigcommand project-scope integration (path/list/get/set/unset/profile) and guardrails--scope global|projectconfig.yaml+config.ymlfail-fast behaviorSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests