fix: persist and resolve global Comet artifact language#191
Merged
Conversation
Global `comet init`/`comet update` now write the selected artifact language to `~/.comet/config.yaml`, and Classic workflows resolve project config before falling back to that global default. Globally installed Chinese Skills now create and validate OpenSpec/Superpowers artifacts in zh-CN instead of always falling back to English. Also fixes several correctness issues found in code review of the above change before it reached master: - mergeProjectConfig silently ignored a re-selected language once `.comet/config.yaml` already had a `language` key (init/update did nothing on a second run with a different --language). - The shared config reader threw on any unrelated YAML syntax error in config.yaml, blocking Classic guard language checks that had nothing to do with the broken field, and surfacing as an inconsistent exit code (70 instead of 1) from comet-state. - An explicit empty string config value was silently treated as "unset" instead of failing validation. - comet update picked an arbitrary platform's language when multiple platforms were installed at the same scope with different languages. Fixes rpamis#174 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuidePersist and correctly resolve Comet artifact language across global and project scopes by introducing a shared Classic config reader, updating init/update workflows to write language defaults, and tightening validation/merging logic so language and other fields behave predictably even with malformed config files or conflicting platform installs. Sequence diagram for language persistence in updateCommandsequenceDiagram
participant User
participant updateCommand
participant getBaseDir
participant mergeProjectConfig
participant LANGUAGES
User->>updateCommand: updateCommand(projectPath, options)
updateCommand->>updateCommand: resolveTargetLanguage(options.language, target.language)
loop scopes (project, global)
updateCommand->>updateCommand: scopeTargets = targets.filter(scope)
alt explicit --language
updateCommand->>updateCommand: languageId = resolveTargetLanguage(options.language, scopeTargets[0].language)
else platforms agree
updateCommand->>updateCommand: agreedLanguage = scopeTargets[0].language
updateCommand->>updateCommand: languageId = agreedLanguage
else conflicting platforms
updateCommand->>updateCommand: languageId = null
end
updateCommand->>getBaseDir: getBaseDir(scope, projectPath)
getBaseDir-->>updateCommand: configRoot
alt languageId
updateCommand->>LANGUAGES: languageToArtifactLanguage(languageId)
LANGUAGES-->>updateCommand: artifactLanguage
updateCommand->>mergeProjectConfig: mergeProjectConfig(configRoot, artifactLanguage)
else no definitive language
updateCommand->>mergeProjectConfig: mergeProjectConfig(configRoot, null)
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughChangesArtifact language persistence and fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InitUpdate
participant ConfigYaml
participant ClassicWorkflow
participant Artifacts
User->>InitUpdate: select or pass skill language
InitUpdate->>ConfigYaml: persist normalized artifact language
ClassicWorkflow->>ConfigYaml: read project config, then global config
ConfigYaml-->>ClassicWorkflow: resolved language
ClassicWorkflow->>Artifacts: generate and validate artifacts using resolved language
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonOops! Something went wrong! :( ESLint: 10.4.0 TypeError: Error while loading rule 'no-irregular-whitespace': sourceCode.getAllComments is not a function 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
updateCommand,languageToArtifactLanguageuses a non-null assertion onLANGUAGES.find(...); consider handling the case where a language id is missing (e.g. by throwing a clear error) instead of relying on!to avoid an opaque runtime crash. - In
readClassicConfigValue,parseDocumenterrors are always ignored; you might want to surface a structured error when the parsed document has errors and the requested field is present, so users see a clear message instead of silently falling back to defaults in the case of a fully malformed config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updateCommand`, `languageToArtifactLanguage` uses a non-null assertion on `LANGUAGES.find(...)`; consider handling the case where a language id is missing (e.g. by throwing a clear error) instead of relying on `!` to avoid an opaque runtime crash.
- In `readClassicConfigValue`, `parseDocument` errors are always ignored; you might want to surface a structured error when the parsed document has errors and the requested field is present, so users see a clear message instead of silently falling back to defaults in the case of a fully malformed config.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
domains/comet-classic/classic-guard.ts (1)
136-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmpty
languagestill skips validation in the guard.projectConfigValue()collapses a missing config value and an explicit empty string to'', andconfiguredLanguage()still treats any falsy value as unset. That means.comet/config.yamlwithlanguage: ""falls back toenhere instead of raising the invalid-language error thatclassic-state-command.tsnow uses.🤖 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 `@domains/comet-classic/classic-guard.ts` around lines 136 - 147, Update projectConfigValue and configuredLanguage to preserve the distinction between a missing language and an explicitly empty string. Ensure configuredLanguage defaults to en only when no value is configured, while an explicit empty string proceeds to validation and throws the invalid-language error, matching classic-state-command.ts behavior.
🧹 Nitpick comments (1)
test/domains/comet-classic/comet-scripts.test.ts (1)
1055-1088: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a guard test for explicit empty
languagein.comet/config.yaml.The existing tests cover malformed-field tolerance and global-language fallback for the guard, but none exercise
language: ""(explicit empty) against the guard'sconfiguredLanguage, which is the scenario affected by the issue raised inclassic-guard.ts.🤖 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/domains/comet-classic/comet-scripts.test.ts` around lines 1055 - 1088, Add a guard test alongside the existing malformed-config test that writes `.comet/config.yaml` with an explicit empty `language: ""`, creates a change whose `configuredLanguage` is populated, and invokes `guardScript` for that change. Assert the guard succeeds, covering the empty-language path in `classic-guard.ts` without relying on global-language fallback.
🤖 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.
Outside diff comments:
In `@domains/comet-classic/classic-guard.ts`:
- Around line 136-147: Update projectConfigValue and configuredLanguage to
preserve the distinction between a missing language and an explicitly empty
string. Ensure configuredLanguage defaults to en only when no value is
configured, while an explicit empty string proceeds to validation and throws the
invalid-language error, matching classic-state-command.ts behavior.
---
Nitpick comments:
In `@test/domains/comet-classic/comet-scripts.test.ts`:
- Around line 1055-1088: Add a guard test alongside the existing
malformed-config test that writes `.comet/config.yaml` with an explicit empty
`language: ""`, creates a change whose `configuredLanguage` is populated, and
invokes `guardScript` for that change. Assert the guard succeeds, covering the
empty-language path in `classic-guard.ts` without relying on global-language
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a702968d-b49c-4334-ae6e-54128661f855
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
CHANGELOG.mdapp/commands/init.tsapp/commands/update.tsassets/manifest.jsonassets/skills-zh/comet-hotfix/SKILL.mdassets/skills-zh/comet-open/SKILL.mdassets/skills-zh/comet-tweak/SKILL.mdassets/skills-zh/comet/SKILL.mdassets/skills-zh/comet/reference/comet-yaml-fields.mdassets/skills/comet-hotfix/SKILL.mdassets/skills/comet-open/SKILL.mdassets/skills/comet-tweak/SKILL.mdassets/skills/comet/SKILL.mdassets/skills/comet/reference/comet-yaml-fields.mdassets/skills/comet/scripts/comet-runtime.mjsdomains/comet-classic/classic-guard.tsdomains/comet-classic/classic-project-config.tsdomains/comet-classic/classic-state-command.tsdomains/skill/platform-install.tspackage.jsontest/app/init-e2e.test.tstest/app/update.test.tstest/domains/comet-classic/comet-scripts.test.tstest/domains/skill/skills.test.ts
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
comet init/comet updatenow persist the selected artifact language to~/.comet/config.yaml, and Classic workflows resolve project config before falling back to that global default — globally installed Chinese Skills now create/validate OpenSpec and Superpowers artifacts inzh-CNinstead of silently falling back to English.mergeProjectConfigsilently ignoring a re-selected language on a config file that already had one; the shared config reader throwing on unrelated YAML syntax errors and blocking guard language checks; an explicit empty config value being treated as unset; andcomet updatepicking an arbitrary platform's language when multiple platforms disagree at the same scope.Fixes #174
Test plan
pnpm buildpnpm lintpnpm format:checkpnpm test(full suite, excluding one pre-existing environment-specific failure inbundle-command.test.tsunrelated to this change — it scans the real~/.claude/skillsdirectory and trips on a locally installed personal skill with malformed frontmatter)Summary by Sourcery
Persist and correctly resolve Comet’s global artifact language across init/update workflows and Classic guards, with project-over-global precedence and robust YAML config handling.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
comet initandcomet updatenow save the selected artifact language.zh-CN.Documentation