Conversation
When skill directories are symlinks (e.g. ~/.claude/skills/comet -> ~/.agents/skills/comet), copyFile and ensureDir wrote to the literal path instead of following the symlink target. Broken symlinks caused silent copy failures. Added resolveSymlinkPath() to file-system.ts that walks up the path tree and follows readlink targets for broken symlinks. Applied to ensureDir, copyFile, and writeFile. Fixes #85
Two changes to fix issue #84: 1. ensureOpenSpecCli now always installs/upgrades openspec to the latest version, even if an older version is already present. This ensures users get the --profile support and other improvements. 2. Added fallback logic: if openspec init fails with 'unknown option --profile' in stderr, retry without the flag. This handles edge cases where the upgrade fails but an older openspec remains. The --profile flag is redundant since XDG_CONFIG_HOME config already sets profile to 'custom', but it helps newer openspec versions. Fixes #84
…n macOS When npm packs the project on Windows, bin/comet.js shebang line gets CRLF line endings, causing macOS to interpret '#!/usr/bin/env node\r' instead of '#!/usr/bin/env node', resulting in 'command not found'. Add explicit eol=lf rules for all text file extensions and binary markers for image files in .gitattributes. Fixes #82
- Extract inline subagent-driven-development protocol from comet-build/SKILL.md into standalone comet/reference/subagent-dispatch.md (ZH + EN) - Sync English comet-build/SKILL.md with Chinese optimizations: simplified subagent instructions, TDD constraints reference, context recovery guidance - Add task-checkoff subcommand to comet-state.sh for targeted task verification - Add phase guard recovery steps for subagent build mode after compaction - Update tests: phase-guard assertions, task-checkoff edge cases, English SKILL.md assertions - Update CHANGELOG.md for v0.3.8
Extract four reference documents from inline skill content to enable on-demand loading and reduce per-invocation token cost: - auto-transition.md: shared auto-transition protocol (7 sub-skills) - context-recovery.md: context compression recovery steps (4 sub-skills) - comet-yaml-fields.md: .comet.yaml field table (main SKILL.md) - file-structure.md: directory structure reference (main SKILL.md) Both Chinese and English versions updated. Key commands and state machine hard constraints retained inline; full protocol details moved to reference. Also fixes comet-tweak missing systematic-debugging handling (both langs), and syncs phase guard rule with bilingual recovery references. Estimated savings: 600-1500 tokens per skill invocation, ~4100 tokens across a full workflow.
CodeGraph's `codegraph install` auto-detects and configures all installed agents (Claude Code, Cursor, Codex CLI, etc.). Passing `--target` and `--location` manually caused Codex CLI to be skipped with "does not support --location=local" because Codex has no project-local config concept. Simplify to `codegraph install --yes` and let CodeGraph handle platform detection itself. Remove now-unused filterSupportedPlatforms and CODEGRAPH_SUPPORTED_TARGETS. Closes #98
…loop for Comet workflow
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds uninstall tooling, Pi command-extension support, version checks, unified hook merging, many Comet reference docs, a task-checkoff script, symlink-aware filesystem helpers, CI/docs updates, and comprehensive tests. ChangesRelease & Repo config
CLI, Uninstall, and Core removal
Filesystem helpers
Task-checkoff script
Pi platform & hook management
Version, OpenSpec, CodeGraph, Init/Update
Reference documentation refactor
Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
This was
linked to
issues
Jun 13, 2026
Closed
This was
linked to
issues
Jun 13, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/update.ts (1)
240-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn a stable JSON shape on the no-targets path.
This early-return payload omits
codegraph, while the normal JSON branch includes it. CLI consumers using--jsonnow have to special-case the "no installed targets" branch even thoughcodegraphStatusalready defaults to'skipped'.Proposed fix
{ npm: { scope: options.skipNpm ? 'skipped' : packageScope, status: npmStatus, command: options.skipNpm ? null : formatNpmUpdateCommand(packageScope), }, skills: { totalCopied: 0, targets: [] }, rules: { totalCopied: 0 }, hooks: { totalInstalled: 0 }, + codegraph: 'skipped', },🤖 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/update.ts` around lines 240 - 257, The early-return JSON for the no-targets path omits the codegraph field; update the JSON emitted in the if (targets.length === 0 && options.json) branch to include the same codegraph entry used in the normal JSON branch (e.g., add "codegraph": { status: codegraphStatus, ... } with the same keys/shape the regular branch uses, and include the command/null logic if present) so the --json output has a stable shape.
🤖 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 `@assets/manifest.json`:
- Line 2: The manifest "version" field is out of sync with the release; update
the "version" key in the manifest from "0.3.3" to "0.3.8" so it matches
package.json (or alternatively document the intentional discrepancy), ensuring
the manifest's version string is exactly "0.3.8".
In `@assets/skills-zh/comet-design/SKILL.md`:
- Line 138: Update the decision-point wording so the skill's specific
prohibitions are explicitly mapped to the reference protocol: change the
sentence that currently lists "不得...创建最终 Design Doc、写入 `design_doc`、运行 design
guard，或进入 `/comet-build`" to say these actions are examples of the reference's
generic "write state / execute chosen branch / auto-continue" prohibitions
(keeping the existing examples `design_doc`, "final Design Doc", "design guard",
and `/comet-build` for clarity) and add a short parenthetical note linking the
term "暂停并等待用户明确确认设计方案" to the reference protocol phrase "pause for explicit user
choice" so readers understand they are the same rule. Ensure the decision-point
phrase "暂停并等待用户明确确认设计方案" remains unchanged in intent but now explicitly refers
to the reference protocol's generic actions to avoid ambiguity.
In `@assets/skills-zh/comet-open/SKILL.md`:
- Line 49: 在三个用户决策点（“PRD 拆分建议”决策点、“需求澄清完成确认”决策点、以及“最终用户复核确认”决策点）确保完全遵守
comet/reference/decision-point.md
协议：在每个点由流程暂停并等待用户明确选择，展示互斥且清晰的选项文本，并且不在用户选择前自动执行任何后续动作；具体改动：在“PRD
拆分建议”保持当前三选项表述并显式将流程置为待选中断；在“需求澄清完成确认”增加明确互斥选项（例如“需要继续澄清”/“无需更多澄清，继续”或“需调整后再确认”）并把确认动作移入用户选择分支；在“最终用户复核确认”确保存在显式重申与重试路径（“确认，继续下一阶段”/“需要调整”）且在“确认”前不触发任何自动提交或工件创建。
In `@assets/skills-zh/comet/reference/comet-yaml-fields.md`:
- Around line 31-50: The field reference tables are missing the .comet.yaml
fields handoff_hash and handoff_context which are referenced by comet-verify;
add entries for these fields (mark as required/optional per actual usage) to
both assets/skills-zh/comet/reference/comet-yaml-fields.md (lines 31-50) and
assets/skills/comet/reference/comet-yaml-fields.md (lines 31-50), and then
ensure the implementation sites enforce them: update comet-state.sh cmd_set
whitelist and enum validation to include handoff_hash and handoff_context, add
them to comet-yaml-validate.sh KNOWN_KEYS/schema rules, and update
test/ts/comet-scripts.test.ts examples to cover these fields; alternatively, if
they are internal-only, add a clarifying note in both field tables and modify
assets/skills-zh/comet-verify/SKILL.md (lines ~98-99) to remove or clarify the
references.
In `@assets/skills-zh/comet/reference/context-recovery.md`:
- Around line 1-35: The context-recovery doc references brainstorm-summary.md
ambiguously; update the reference in context-recovery.md so the artifact path is
explicit and consistent with other docs by changing the mention of
"brainstorm-summary.md" to
"openspec/changes/<name>/.comet/handoff/brainstorm-summary.md", and do not
change the existing reference to
"openspec/changes/<name>/.comet/subagent-progress.md" (leave
subagent-progress.md as-is); ensure the text in context-recovery.md and any
nearby mentions match the exact filename used in subagent-dispatch.md.
In `@assets/skills-zh/comet/reference/file-structure.md`:
- Around line 7-28: The fenced code block that begins with the line containing
"openspec/" is missing a language tag; replace the opening backticks (```) with
a language-tagged fence (```yaml) so the block is ```yaml ... ``` to satisfy
Markdown linting and enable proper YAML highlighting.
In `@assets/skills/comet/reference/file-structure.md`:
- Line 7: The fenced code block starting at the shown block (the triple-backtick
fence in file-structure.md) is missing a language tag which triggers MD040;
update the opening fence from ``` to ```text so the block begins with "```text"
and keep the rest of the code block unchanged.
In `@assets/skills/comet/reference/subagent-dispatch.md`:
- Around line 60-62: Add a language tag to the fenced code block that contains
the TDD sentence ("You MUST follow TDD: write a failing test first...") so the
block is like ```text (or ```text\n...```) instead of an unlabeled ```; this
fixes the markdown lint MD040 by specifying the code-fence language.
In `@src/commands/init.ts`:
- Line 207: The call to printVersionInfo is awaited unconditionally, causing
version-registry network I/O to block JSON-mode runs; change the call site so
init's JSON output isn't delayed: if JSON mode is active (check flags.json or
the function/variable used to detect JSON), skip awaiting/logging by either (a)
only call await printVersionInfo(log) when flags.json is false, or (b) invoke
printVersionInfo(log) without awaiting and attach a .catch(()=>{}) to swallow
errors so it runs asynchronously and cannot block emitting JSON; update the call
where you currently have await printVersionInfo(log) accordingly.
In `@src/commands/update.ts`:
- Around line 184-207: The npm update helper currently always spawns npm with
stdio: 'inherit' which breaks JSON-mode output; change updateCometNpmPackage to
accept a boolean flag (e.g., keepJsonOutput or jsonMode) and use stdio: jsonMode
? 'pipe' : 'inherit' when calling spawn inside updateCometNpmPackage, and update
every call site to pass options.json ?? false (so the call-site change
referenced in the review is applied); keep the rest of the function behavior the
same (error/exit handlers) but do not write npm stdout/stderr to the parent
process when jsonMode is true so the final JSON remains parseable.
- Around line 85-99: The loop currently returns 'zh' on the first SKILL.md it
finds (which can be from a legacy dir), so change the logic to respect the
ordering from getInstalledCometSkillsDirs(baseDir, platform, scope): build the
union of skill entry names (use readDir on each skillsDir), then for each entry
iterate skillsDir in the order returned by getInstalledCometSkillsDirs and pick
the first existing SKILL.md (use fileExists(path.join(skillsDir, entry,
'SKILL.md'))); read that file with fs.readFile and decide language
(/[\u3400-\u9fff]/u.test(content)) for that entry, stopping on the first
matching file per entry and returning 'zh' only if the chosen (first-existing)
SKILL.md is Chinese; keep the catch behavior for unreadable files but treat
unreadable as missing so you continue to the next directory.
In `@src/core/skills.ts`:
- Around line 509-513: The code assumes hook groups are arrays and calls
.flatMap/.filter on values that may be malformed; before calling mergeHookGroups
or performing .flatMap/.filter on existingPreToolUse, existingBeforeTool, and
other hook-group variables, validate and coerce them to arrays (e.g., if
(!Array.isArray(x)) x = []; or wrap with Array.isArray(...) ? x : []) so
mergeHookGroups receives only arrays and subsequent .flatMap/.filter calls
cannot throw; update the call sites that pass existingPreToolUse,
existingBeforeTool, etc., and any local uses of .flatMap/.filter to use these
safe-guarded/coerced arrays instead of raw cast values.
In `@src/utils/file-system.ts`:
- Around line 104-108: removeFile/removeDir currently call resolveSymlinkPath()
(which uses fs.realpath) and end up deleting symlink targets instead of the
symlink entries, and they swallow all errors so failures are silent; update
removeFile and removeDir to operate on the original path when the path is a
symlink: use fs.lstat on the incoming filePath, and if lstat.isSymbolicLink()
then call fs.unlink(filePath) or fs.rm(filePath, { recursive: true, force: true
}) directly (do NOT pass the resolved realpath); only fall back to the resolved
path for non-symlink entries. Also stop swallowing errors silently: let the
error propagate or return false but ensure callers handle it—update callers such
as the uninstall flow to detect and count failures (increment failed or surface
the error) instead of assuming success, and adjust isDirEmpty to not return true
on errors but to propagate or report the error so deletions aren’t silently
ignored.
- Around line 104-111: removeFile and removeDir currently swallow all errors and
return false, hiding permission/IO failures; change their catch-handling so only
"not found" is treated as a non-error: in removeFile (and similarly in
removeDir) catch the thrown error from resolveSymlinkPath/fs.unlink (or
fs.rmdir/fs.rm) and if error.code === 'ENOENT' (or other platform-equivalent
"not found") return false, otherwise rethrow the error (or propagate it) so
callers see permission/IO failures; keep resolveSymlinkPath use and only adjust
the catch block to discriminate error.code and not suppress all exceptions.
In `@test/ts/skills.test.ts`:
- Around line 1036-1042: The three assertions are checking English text but
reference enCometRule, which in this test was incorrectly loaded from the
non-English file; update the test so enCometRule is assigned the English rule
file (load 'comet-phase-guard.en.md' instead of 'comet-phase-guard.md') or,
alternatively, change these assertions to use the variable that actually holds
the non-English content—locate the enCometRule declaration (around the existing
rule-variable block near line ~810) and correct its source to the English file
so the expectations around the assertions at lines 1036–1042 validate the
English rule text.
---
Outside diff comments:
In `@src/commands/update.ts`:
- Around line 240-257: The early-return JSON for the no-targets path omits the
codegraph field; update the JSON emitted in the if (targets.length === 0 &&
options.json) branch to include the same codegraph entry used in the normal JSON
branch (e.g., add "codegraph": { status: codegraphStatus, ... } with the same
keys/shape the regular branch uses, and include the command/null logic if
present) so the --json output has a stable shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c215b087-1518-41d3-b9f8-042ebdc847fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (66)
.claude/settings.local.json.gitattributes.github/workflows/ci.yml.gitignoreAGENTS.mdCHANGELOG.mdCLAUDE.mdREADME-zh.mdREADME.mdassets/manifest.jsonassets/skills-zh/comet-archive/SKILL.mdassets/skills-zh/comet-build/SKILL.mdassets/skills-zh/comet-design/SKILL.mdassets/skills-zh/comet-hotfix/SKILL.mdassets/skills-zh/comet-open/SKILL.mdassets/skills-zh/comet-tweak/SKILL.mdassets/skills-zh/comet-verify/SKILL.mdassets/skills-zh/comet/SKILL.mdassets/skills-zh/comet/reference/auto-transition.mdassets/skills-zh/comet/reference/comet-yaml-fields.mdassets/skills-zh/comet/reference/context-recovery.mdassets/skills-zh/comet/reference/debug-gate.mdassets/skills-zh/comet/reference/decision-point.mdassets/skills-zh/comet/reference/file-structure.mdassets/skills-zh/comet/reference/subagent-dispatch.mdassets/skills/comet-archive/SKILL.mdassets/skills/comet-build/SKILL.mdassets/skills/comet-design/SKILL.mdassets/skills/comet-hotfix/SKILL.mdassets/skills/comet-open/SKILL.mdassets/skills/comet-tweak/SKILL.mdassets/skills/comet-verify/SKILL.mdassets/skills/comet/SKILL.mdassets/skills/comet/reference/auto-transition.mdassets/skills/comet/reference/comet-yaml-fields.mdassets/skills/comet/reference/context-recovery.mdassets/skills/comet/reference/debug-gate.mdassets/skills/comet/reference/decision-point.mdassets/skills/comet/reference/file-structure.mdassets/skills/comet/reference/subagent-dispatch.mdassets/skills/comet/rules/comet-phase-guard.en.mdassets/skills/comet/rules/comet-phase-guard.mdassets/skills/comet/scripts/comet-state.shpackage.jsonsrc/cli/index.tssrc/commands/init.tssrc/commands/uninstall.tssrc/commands/update.tssrc/core/codegraph.tssrc/core/detect.tssrc/core/openspec.tssrc/core/platforms.tssrc/core/skills.tssrc/core/uninstall.tssrc/core/version.tssrc/utils/file-system.tstest/ts/comet-scripts.test.tstest/ts/detect.test.tstest/ts/file-system.test.tstest/ts/init-e2e.test.tstest/ts/init.test.tstest/ts/openspec.test.tstest/ts/skills.test.tstest/ts/uninstall.test.tstest/ts/update.test.tstest/ts/version.test.ts
The init-e2e workflow's Pi settings verification interpolated a Windows $RUNNER_TEMP path (with backslashes) into a node -e require() JS string literal, where \a and \_ were parsed as escape characters and mangled the path (D:\a\_temp -> D:a_temp), failing the init-e2e (windows-latest) runners on Node 20 and 22. Pass the path via process.env so it never enters a JS string literal. Linux and macOS were unaffected. Also reformat src/core/openspec.ts to satisfy prettier --check, which was failing the format:check CI step.
Add a husky + lint-staged pre-commit hook that runs prettier --write on staged source files under src/ at every git commit (scope aligned with CI format:check). Editor-agnostic, so it enforces formatting for all contributors regardless of IDE or agent, preventing the prettier formatting issues that broke CI from recurring. prepare now installs the hook on pnpm install; .husky/ is excluded from the published package via the files whitelist. Document the pre-commit workflow in CLAUDE.md and AGENTS.md.
- file-system: symlink-safe removal during uninstall (removeFile/removeDir no longer resolve symlinks before deleting, so a symlinked dir's target is never recursively deleted); isDirEmpty no longer treats unreadable dirs as empty - update: discard npm stdio in --json mode to avoid corrupting output; add codegraph field to the no-targets JSON branch for a stable shape; skip the npm-registry version check in JSON mode - skills: coerce parsed hook groups to arrays before merge/filter so malformed hand-edited settings cannot throw during init/update - init: skip the version check in JSON mode - manifest: bump version 0.3.3 -> 0.3.8 to match package.json - docs: add text fence language tags (MD040) in file-structure.md and subagent-dispatch.md, Chinese and English
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/file-system.ts (1)
141-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
falsewhen the directory is already missing.
fs.lstat()throwsENOENTfor an absent path, soreturn isNotFoundError(error)makes a no-op removal look successful. That contradicts the contract on Line 127 and can over-report removals.Suggested fix
} catch (error) { - return isNotFoundError(error); + if (isNotFoundError(error)) { + return false; + } + throw error; } }🤖 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/utils/file-system.ts` around lines 141 - 142, The catch block that currently returns isNotFoundError(error) should instead return false when the error indicates a missing path and rethrow other errors: replace "return isNotFoundError(error)" with "if (isNotFoundError(error)) return false; throw error;" so that an ENOENT from fs.lstat yields false (no-op removal) and other errors propagate.
🤖 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.
Duplicate comments:
In `@src/utils/file-system.ts`:
- Around line 141-142: The catch block that currently returns
isNotFoundError(error) should instead return false when the error indicates a
missing path and rethrow other errors: replace "return isNotFoundError(error)"
with "if (isNotFoundError(error)) return false; throw error;" so that an ENOENT
from fs.lstat yields false (no-op removal) and other errors propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 308d6960-f462-40d2-a43b-28ceb5363125
📒 Files selected for processing (10)
CHANGELOG.mdassets/manifest.jsonassets/skills-zh/comet/reference/file-structure.mdassets/skills-zh/comet/reference/subagent-dispatch.mdassets/skills/comet/reference/file-structure.mdassets/skills/comet/reference/subagent-dispatch.mdsrc/commands/init.tssrc/commands/update.tssrc/core/skills.tssrc/utils/file-system.ts
✅ Files skipped from review due to trivial changes (3)
- assets/skills/comet/reference/file-structure.md
- assets/skills-zh/comet/reference/file-structure.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- assets/skills/comet/reference/subagent-dispatch.md
- assets/skills-zh/comet/reference/subagent-dispatch.md
- src/commands/update.ts
- src/commands/init.ts
- src/core/skills.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.
✨ Summary
🎯 Scope
init,status,doctor,update)assets/skills/,assets/skills-zh/)assets/skills/comet/scripts/)🧪 Testing
pnpm buildpnpm lintpnpm format:checkpnpm testpnpm test -- test/ts/comet-scripts.test.tspnpm test:shell✅ Checklist
fix: handle project-scope initREADME.md,README-zh.md, orCONTRIBUTING.mdCHANGELOG.mdis updated when behavior changesassets/manifest.jsonand relevant tests👀 Notes for Reviewers
Summary by CodeRabbit
New Features
Documentation
Tests
Chores