feat(config): add project-scoped profile resolution and scope-aware u…#1012
Open
kaze-droid wants to merge 5 commits intoFission-AI:mainfrom
Open
feat(config): add project-scoped profile resolution and scope-aware u…#1012kaze-droid wants to merge 5 commits intoFission-AI:mainfrom
kaze-droid wants to merge 5 commits intoFission-AI:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds project-scoped config support for Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Cmd as Config/Update Command
participant Resolver as resolveEffectiveProfileSettings
participant ProjectCfg as Project Config (openspec/config.yaml)
participant GlobalCfg as Global Config (XDG JSON)
participant Defaults as Built-in Defaults
User->>CLI: run command (may include CLI overrides / --scope)
CLI->>Cmd: invoke with parsed options
Cmd->>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 == 'project'
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->>Cmd: return EffectiveProfileSettings (with sources)
Cmd->>User: apply/display result (e.g., perform update or write project config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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
…-unknown across project commands
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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