Conversation
benym
marked this pull request as ready for review
July 12, 2026 07:12
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
domains/comet-classic/classic-hook-guard.ts (2)
223-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant non-null assertion followed by
.filter(Boolean).Line 250 uses
governingChangeName(governing)!(non-null assertion) but then.filter(Boolean)removes nullish values. The!is misleading sincegoverningChangeNamecan returnnull, and.filter(Boolean)already handles that. Drop the!for correctness clarity.♻️ Remove redundant non-null assertion
- active.map((governing) => governingChangeName(governing)!).filter(Boolean), + active.map((governing) => governingChangeName(governing)).filter(Boolean),🤖 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-hook-guard.ts` around lines 223 - 253, In repoSourceGoverningChange, remove the redundant non-null assertion from governingChangeName(governing) in the active.map(...).filter(Boolean) expression, leaving the existing nullish filtering behavior unchanged.
277-280: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
activeChangescalled twice for the same project root.
superpowersArtifactGoverningChange(line 203) already callsactiveChanges(projectRoot), and line 277 calls it again as a fallback. Consider passing the already-fetched active changes list to avoid a redundant async I/O call on the hot path.🤖 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-hook-guard.ts` around lines 277 - 280, Update superpowersArtifactGoverningChange and its surrounding call flow to reuse the active changes list already fetched for the same projectRoot instead of calling activeChanges(projectRoot) again in the fallback. Pass the existing list into the fallback logic and preserve the current first-entry and null behavior.
🤖 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/skills/comet/SKILL.md`:
- Line 25: Update the existing-change language lookup in the Output Language
Rule of SKILL.md to use the stable `comet state get <name> language` command
form, matching comet-verify/SKILL.md. Preserve the existing fallback order and
explicit language propagation requirements.
---
Nitpick comments:
In `@domains/comet-classic/classic-hook-guard.ts`:
- Around line 223-253: In repoSourceGoverningChange, remove the redundant
non-null assertion from governingChangeName(governing) in the
active.map(...).filter(Boolean) expression, leaving the existing nullish
filtering behavior unchanged.
- Around line 277-280: Update superpowersArtifactGoverningChange and its
surrounding call flow to reuse the active changes list already fetched for the
same projectRoot instead of calling activeChanges(projectRoot) again in the
fallback. Pass the existing list into the fallback logic and preserve the
current first-entry and null behavior.
🪄 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: 022c0b23-cf02-4611-a663-0e4cb69b0720
📒 Files selected for processing (52)
.github/workflows/ci.ymlCHANGELOG.mdapp/commands/init.tsapp/commands/update.tsassets/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/comet-yaml-fields.mdassets/skills-zh/comet/reference/scripts.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/comet-yaml-fields.mdassets/skills/comet/reference/scripts.mdassets/skills/comet/rules/comet-phase-guard.en.mdassets/skills/comet/rules/comet-phase-guard.mdassets/skills/comet/scripts/comet-runtime.mjsdocs/superpowers/plans/2026-07-12-current-change-hook-binding.mddocs/superpowers/specs/2026-07-12-current-change-hook-binding-design.mddomains/comet-classic/classic-archive.tsdomains/comet-classic/classic-current-change.tsdomains/comet-classic/classic-guard.tsdomains/comet-classic/classic-hook-guard.tsdomains/comet-classic/classic-runtime-run.tsdomains/comet-classic/classic-state-command.tsdomains/comet-classic/index.tsdomains/dashboard/web/src/main.jsxdomains/integrations/openspec.tsdomains/skill/platform-install.tstest/app/init-e2e.test.tstest/app/update.test.tstest/domains/comet-classic/classic-archive.test.tstest/domains/comet-classic/classic-current-change.test.tstest/domains/comet-classic/classic-hook-guard.test.tstest/domains/comet-classic/comet-scripts-guard.test.tstest/domains/comet-classic/comet-scripts.test.tstest/domains/dashboard/web-source.test.tstest/domains/integrations/openspec.test.tstest/domains/skill/internal-skills.test.tstest/domains/skill/skills.test.tstest/repository/ci-workflows.test.tstest/scripts/prepublish-check.test.ts
✅ Files skipped from review due to trivial changes (3)
- assets/skills-zh/comet-design/SKILL.md
- docs/superpowers/specs/2026-07-12-current-change-hook-binding-design.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (17)
- assets/skills-zh/comet/reference/comet-yaml-fields.md
- assets/skills/comet/reference/comet-yaml-fields.md
- assets/skills/comet-archive/SKILL.md
- app/commands/init.ts
- assets/skills-zh/comet/reference/scripts.md
- test/domains/comet-classic/comet-scripts-guard.test.ts
- test/domains/integrations/openspec.test.ts
- assets/skills-zh/comet-archive/SKILL.md
- domains/comet-classic/classic-archive.ts
- assets/skills/comet/reference/scripts.md
- domains/dashboard/web/src/main.jsx
- domains/skill/platform-install.ts
- test/app/init-e2e.test.ts
- assets/skills-zh/comet-verify/SKILL.md
- assets/skills/comet-build/SKILL.md
- assets/skills-zh/comet-build/SKILL.md
- app/commands/update.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
domains/comet-classic/classic-hook-guard.ts (2)
223-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant non-null assertion followed by
.filter(Boolean).Line 250 uses
governingChangeName(governing)!(non-null assertion) but then.filter(Boolean)removes nullish values. The!is misleading sincegoverningChangeNamecan returnnull, and.filter(Boolean)already handles that. Drop the!for correctness clarity.♻️ Remove redundant non-null assertion
- active.map((governing) => governingChangeName(governing)!).filter(Boolean), + active.map((governing) => governingChangeName(governing)).filter(Boolean),🤖 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-hook-guard.ts` around lines 223 - 253, In repoSourceGoverningChange, remove the redundant non-null assertion from governingChangeName(governing) in the active.map(...).filter(Boolean) expression, leaving the existing nullish filtering behavior unchanged.
277-280: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
activeChangescalled twice for the same project root.
superpowersArtifactGoverningChange(line 203) already callsactiveChanges(projectRoot), and line 277 calls it again as a fallback. Consider passing the already-fetched active changes list to avoid a redundant async I/O call on the hot path.🤖 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-hook-guard.ts` around lines 277 - 280, Update superpowersArtifactGoverningChange and its surrounding call flow to reuse the active changes list already fetched for the same projectRoot instead of calling activeChanges(projectRoot) again in the fallback. Pass the existing list into the fallback logic and preserve the current first-entry and null behavior.
🤖 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/skills/comet/SKILL.md`:
- Line 25: Update the existing-change language lookup in the Output Language
Rule of SKILL.md to use the stable `comet state get <name> language` command
form, matching comet-verify/SKILL.md. Preserve the existing fallback order and
explicit language propagation requirements.
---
Nitpick comments:
In `@domains/comet-classic/classic-hook-guard.ts`:
- Around line 223-253: In repoSourceGoverningChange, remove the redundant
non-null assertion from governingChangeName(governing) in the
active.map(...).filter(Boolean) expression, leaving the existing nullish
filtering behavior unchanged.
- Around line 277-280: Update superpowersArtifactGoverningChange and its
surrounding call flow to reuse the active changes list already fetched for the
same projectRoot instead of calling activeChanges(projectRoot) again in the
fallback. Pass the existing list into the fallback logic and preserve the
current first-entry and null behavior.
🪄 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: 022c0b23-cf02-4611-a663-0e4cb69b0720
📒 Files selected for processing (52)
.github/workflows/ci.ymlCHANGELOG.mdapp/commands/init.tsapp/commands/update.tsassets/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/comet-yaml-fields.mdassets/skills-zh/comet/reference/scripts.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/comet-yaml-fields.mdassets/skills/comet/reference/scripts.mdassets/skills/comet/rules/comet-phase-guard.en.mdassets/skills/comet/rules/comet-phase-guard.mdassets/skills/comet/scripts/comet-runtime.mjsdocs/superpowers/plans/2026-07-12-current-change-hook-binding.mddocs/superpowers/specs/2026-07-12-current-change-hook-binding-design.mddomains/comet-classic/classic-archive.tsdomains/comet-classic/classic-current-change.tsdomains/comet-classic/classic-guard.tsdomains/comet-classic/classic-hook-guard.tsdomains/comet-classic/classic-runtime-run.tsdomains/comet-classic/classic-state-command.tsdomains/comet-classic/index.tsdomains/dashboard/web/src/main.jsxdomains/integrations/openspec.tsdomains/skill/platform-install.tstest/app/init-e2e.test.tstest/app/update.test.tstest/domains/comet-classic/classic-archive.test.tstest/domains/comet-classic/classic-current-change.test.tstest/domains/comet-classic/classic-hook-guard.test.tstest/domains/comet-classic/comet-scripts-guard.test.tstest/domains/comet-classic/comet-scripts.test.tstest/domains/dashboard/web-source.test.tstest/domains/integrations/openspec.test.tstest/domains/skill/internal-skills.test.tstest/domains/skill/skills.test.tstest/repository/ci-workflows.test.tstest/scripts/prepublish-check.test.ts
✅ Files skipped from review due to trivial changes (3)
- assets/skills-zh/comet-design/SKILL.md
- docs/superpowers/specs/2026-07-12-current-change-hook-binding-design.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (17)
- assets/skills-zh/comet/reference/comet-yaml-fields.md
- assets/skills/comet/reference/comet-yaml-fields.md
- assets/skills/comet-archive/SKILL.md
- app/commands/init.ts
- assets/skills-zh/comet/reference/scripts.md
- test/domains/comet-classic/comet-scripts-guard.test.ts
- test/domains/integrations/openspec.test.ts
- assets/skills-zh/comet-archive/SKILL.md
- domains/comet-classic/classic-archive.ts
- assets/skills/comet/reference/scripts.md
- domains/dashboard/web/src/main.jsx
- domains/skill/platform-install.ts
- test/app/init-e2e.test.ts
- assets/skills-zh/comet-verify/SKILL.md
- assets/skills/comet-build/SKILL.md
- assets/skills-zh/comet-build/SKILL.md
- app/commands/update.ts
🛑 Comments failed to post (1)
assets/skills/comet/SKILL.md (1)
25-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent CLI form in Output Language Rule.
Line 25 uses
"$COMET_BASH" "$COMET_STATE" get <name> languagewhilecomet-verify/SKILL.mdline 17 uses the stablecomet state get <name> languageform. Since this PR's objective includes "stable Classic command facades," consider updating this reference for consistency.♻️ Proposed fix
- Use the configured Comet artifact language as the output language for every OpenSpec and Superpowers artifact. The configured value is a normalized language id, `en` or `zh-CN`. For an existing change, read `language` from `openspec/changes/<name>/.comet.yaml` using `"$COMET_BASH" "$COMET_STATE" get <name> language`. + Use the configured Comet artifact language as the output language for every OpenSpec and Superpowers artifact. The configured value is a normalized language id, `en` or `zh-CN`. For an existing change, read `language` from `openspec/changes/<name>/.comet.yaml` using `comet state get <name> language`.📝 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.Use the configured Comet artifact language as the output language for every OpenSpec and Superpowers artifact. The configured value is a normalized language id, `en` or `zh-CN`. For an existing change, read `language` from `openspec/changes/<name>/.comet.yaml` using `comet state get <name> language`. Before `.comet.yaml` exists, read `language` from project `.comet/config.yaml`, then fall back to global `~/.comet/config.yaml`; if neither exists, fall back to the current user request language. Include the resolved language explicitly in every prompt or ARGUMENTS passed to external OpenSpec/Superpowers skills.🧰 Tools
🪛 SkillSpector (2.3.7)
[warning] 201: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
🤖 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 `@assets/skills/comet/SKILL.md` at line 25, Update the existing-change language lookup in the Output Language Rule of SKILL.md to use the stable `comet state get <name> language` command form, matching comet-verify/SKILL.md. Preserve the existing fallback order and explicit language propagation requirements.
This was referenced Jul 13, 2026
This was referenced Jul 23, 2026
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
🎯 Scope
init,status,doctor,update)assets/skills/,assets/skills-zh/)assets/skills/comet/scripts/)🧪 Testing
pnpm buildpnpm lintpnpm run lint:architecturepnpm format:checkpnpm testpnpm test -- test/domains/comet-classic/comet-scripts.test.ts✅ 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
comet resume-probefor read-only Ambient Resume probing, including managed project instruction injection.comet state record-checkand improved stable Classic top-level commands.comet update/uninstallplus new scope prompts.comet statusnow clearly distinguishes Comet vs OpenSpec and surfaces archive readiness/command-check details.comet evalharness/draft-hash resolution, archive confirmation gating, and multiple update/uninstall/init edge cases.