fix: protect installs and artifact routing#164
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughThis PR adds unmanaged-directory protection before replacing a skills directory with a symlink, refines classic hook guard routing for docs/superpowers artifact writes to match the governing change (with a runtime script mirror), updates related tests, and revises the changelog and website subproject pointer. ChangesSymlink Install Safety
Superpowers Artifact Routing Fix
Release Metadata
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant classic-hook-guard
participant SuperpowersResolver
participant ClassicState
classic-hook-guard->>classic-hook-guard: isSuperpowersArtifactPath(relativePath)
alt is superpowers artifact
classic-hook-guard->>SuperpowersResolver: resolve governing change
SuperpowersResolver->>ClassicState: match recorded design/plan/verification paths
alt exact match found
ClassicState-->>SuperpowersResolver: matching change
else no exact match
SuperpowersResolver->>SuperpowersResolver: infer change from filename stem
end
SuperpowersResolver-->>classic-hook-guard: governing change (matched/unmatched)
else not superpowers artifact
classic-hook-guard->>classic-hook-guard: repoSourceGoverningChange(projectRoot)
end
classic-hook-guard->>classic-hook-guard: allow if phase design/build/verify and matched, else block
sequenceDiagram
participant Installer as copyCometSkillsForPlatform
participant CreateSymlink as createSymlink
participant FS as Filesystem
Installer->>CreateSymlink: createSymlink(target, linkPath, managedEntries)
CreateSymlink->>FS: attempt unlink existing path
FS-->>CreateSymlink: unlink fails (directory)
CreateSymlink->>FS: collect directory entry paths
FS-->>CreateSymlink: entry paths
CreateSymlink->>CreateSymlink: check entries against managedEntries
alt unmanaged entries found
CreateSymlink-->>Installer: throw error, directory preserved
else all entries managed
CreateSymlink->>FS: remove directory and create symlink
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's GuideProtects symlink-mode platform skill installs from deleting unmanaged skills directories, and adjusts Classic hook governance so docs/superpowers artifacts are associated with the appropriate design/build/verify change, along with version/changelog updates. Sequence diagram for routing docs/superpowers artifacts to the correct governing changesequenceDiagram
participant GitHook as classicHookGuardCommand
participant Guard as governingChange
participant Super as superpowersArtifactGoverningChange
participant Active as activeChanges
GitHook->>Guard: governingChange(relativePath, projectRoot)
Guard->>Guard: isSuperpowersArtifactPath(relativePath)
alt isSuperpowersArtifactPath
Guard->>Super: superpowersArtifactGoverningChange(relativePath, projectRoot)
Super->>Active: activeChanges(projectRoot)
Active-->>Super: active
Super->>Super: matchesRecordedSuperpowersArtifact(relativePath, governing)
alt recorded match
Super-->>Guard: recorded governing
else no recorded match
Super->>Super: allowsSuperpowersArtifacts(governing)
Super->>Super: governingChangeName(governing)
alt name in relativePath
Super-->>Guard: named governing
else no named governing
Super-->>Guard: first eligible governing or null
end
end
Guard->>Guard: repoSourceGoverningChange(projectRoot) (fallback)
else not Superpowers path
Guard->>Guard: repoSourceGoverningChange(projectRoot)
end
Guard-->>GitHook: governing
GitHook->>GitHook: isSuperpowersArtifactPath(relativePath)
GitHook->>GitHook: allowsSuperpowersArtifacts(governing)
alt allowed
GitHook-->>GitHook: allowed(relativePath (superpowers))
else blocked
GitHook-->>GitHook: existing phase guards apply
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was
linked to
issues
Jul 7, 2026
benym
marked this pull request as ready for review
July 7, 2026 14:29
Greptile SummaryThis PR protects skill installs and fixes Classic Superpowers artifact routing.
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "chore: merge master into artifact routin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 found 4 issues, and left some high level feedback:
- In
superpowersArtifactGoverningChangeandrepoSourceGoverningChange,activeChanges(projectRoot)is invoked independently; consider threading a shared active-changes list through these call sites to avoid redundant filesystem calls when resolving the governing change for superpowers artifacts. - The unmanaged entries error in
assertDirectoryContainsOnlyManagedEntriescurrently only shows the first few offending paths; it may be useful to include the manifest-derivedmanagedEntriesroot (e.g., the skills directory) in the message to make it clearer to users what directory tree is considered managed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `superpowersArtifactGoverningChange` and `repoSourceGoverningChange`, `activeChanges(projectRoot)` is invoked independently; consider threading a shared active-changes list through these call sites to avoid redundant filesystem calls when resolving the governing change for superpowers artifacts.
- The unmanaged entries error in `assertDirectoryContainsOnlyManagedEntries` currently only shows the first few offending paths; it may be useful to include the manifest-derived `managedEntries` root (e.g., the skills directory) in the message to make it clearer to users what directory tree is considered managed.
## Individual Comments
### Comment 1
<location path="domains/skill/platform-install.ts" line_range="97" />
<code_context>
+ managedEntries: Set<string>,
+): Promise<void> {
+ const entries = await collectDirectoryEntryPaths(dirPath);
+ const unmanagedEntries = entries.filter((entry) => !managedEntries.has(entry));
+ if (unmanagedEntries.length === 0) return;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Managed entries check treats all nested files/dirs under a managed skill as unmanaged.
Because `collectDirectoryEntryPaths` returns full relative paths (e.g. `a/b/c.txt`) while `managedEntries` only contains prefix paths (e.g. `a`, `a/b`), the `managedEntries.has(entry)` check treats all nested files/dirs under a managed skill as unmanaged. As a result, directories that only contain manifest-managed skills can still be rejected.
To treat entries as managed when they are under a managed prefix, you could instead check whether any prefix of the path is in `managedEntries`:
```ts
const unmanagedEntries = entries.filter((entry) => {
const parts = entry.split('/');
for (let depth = 1; depth <= parts.length; depth++) {
if (managedEntries.has(parts.slice(0, depth).join('/'))) return false;
}
return true;
});
```
This preserves the guard against unrelated top-level entries while not flagging contents of managed skill paths as unmanaged.
</issue_to_address>
### Comment 2
<location path="domains/comet-classic/classic-hook-guard.ts" line_range="223-226" />
<code_context>
return { changeDir, phase: 'open', classic: null, archived: false };
}
}
+ if (isSuperpowersArtifactPath(relativePath)) {
+ return (
+ (await superpowersArtifactGoverningChange(relativePath, projectRoot)) ??
+ repoSourceGoverningChange(projectRoot)
+ );
+ }
</code_context>
<issue_to_address>
**issue:** Mixed awaited and non-awaited operands in `??` expression can lead to type/clarity issues.
Here the left side resolves to `GoverningChange | null`, while the right side is `Promise<GoverningChange | null>`, so the full expression’s type becomes `GoverningChange | null | Promise<GoverningChange | null>`. That can cause awkward unions under strict typing.
To keep the return type consistent, consider awaiting both branches, e.g.:
```ts
const superpowers = await superpowersArtifactGoverningChange(relativePath, projectRoot);
if (superpowers) return superpowers;
return await repoSourceGoverningChange(projectRoot);
```
or:
```ts
return (
(await superpowersArtifactGoverningChange(relativePath, projectRoot)) ??
(await repoSourceGoverningChange(projectRoot))
);
```
</issue_to_address>
### Comment 3
<location path="domains/comet-classic/classic-hook-guard.ts" line_range="155-156" />
<code_context>
+ return relativePath.startsWith('docs/superpowers/');
+}
+
+function allowsSuperpowersArtifacts(governing: GoverningChange): boolean {
+ return (
+ governing.phase === 'design' || governing.phase === 'build' || governing.phase === 'verify'
+ );
</code_context>
<issue_to_address>
**suggestion:** Superpowers phase logic is duplicated instead of reusing the new helper.
In `classicHookGuardCommand`, the superpowers phase check is still duplicated:
```ts
if (
isSuperpowersArtifactPath(relativePath) &&
(phase === 'design' || phase === 'build' || phase === 'verify')
) {
…
}
```
Please reuse the new helper instead:
```ts
if (isSuperpowersArtifactPath(relativePath) && allowsSuperpowersArtifacts(governing)) {
…
}
```
This keeps phase rules in one place and avoids future divergence between command logic and governing-change logic.
Suggested implementation:
```typescript
);
}
function isSuperpowersArtifactPath(relativePath: string): boolean {
return relativePath.startsWith('docs/superpowers/');
}
function allowsSuperpowersArtifacts(governing: GoverningChange): boolean {
return (
governing.phase === 'design' || governing.phase === 'build' || governing.phase === 'verify'
);
}
```
You should also update the superpowers phase logic inside `classicHookGuardCommand` to reuse the new helper. Look for the condition that currently looks like:
```ts
if (
isSuperpowersArtifactPath(relativePath) &&
(phase === 'design' || phase === 'build' || phase === 'verify')
) {
…
}
```
and change it to:
```ts
if (isSuperpowersArtifactPath(relativePath) && allowsSuperpowersArtifacts(governing)) {
…
}
```
This assumes `governing` is already available in that scope (as suggested by your comment). If it is not, you will need to ensure `governing` is passed into or computed within `classicHookGuardCommand` so that the helper can be used.
</issue_to_address>
### Comment 4
<location path="test/domains/comet-classic/comet-scripts.test.ts" line_range="4987-5007" />
<code_context>
expect(result.status).toBe(0);
}, 20_000);
+ it('allows docs/superpowers writes for the matching design change when another active change is open', async () => {
+ await createChange(
+ tmpDir,
+ 'cert-signature-auth',
+ ['workflow: full', 'phase: open', 'archived: false', ''].join('\n'),
+ );
+ await createChange(
+ tmpDir,
+ 'env-issue-ledger',
+ ['workflow: full', 'phase: design', 'archived: false', ''].join('\n'),
+ );
+
+ const docsDir = path.join(tmpDir, 'docs', 'superpowers', 'specs');
+ await fs.mkdir(docsDir, { recursive: true });
+ const targetFile = path.join(docsDir, 'env-issue-ledger-design.md');
+
+ const result = runHookGuard(tmpDir, hookGuardScript, hookStdin(targetFile));
+
+ expect(result.status).toBe(0);
+ expect(result.stderr).toContain('phase: design, superpowers');
+ }, 20_000);
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that covers matching via recorded Superpowers artifacts (designDoc/plan/verificationReport), not just filename-based matching.
To exercise `superpowersArtifactGoverningChange` more fully, please add a test where the governing change is selected via recorded artifacts (e.g. `classic.designDoc` or `classic.plan`), not just by inferring from the filename.
For instance:
- Create two active changes and set `classic.designDoc` (or `plan`) on one to `docs/superpowers/specs/foo-design.md`,
- Write to that path, and
- Assert that the hook guard allows the write and that stderr reports the governing change/phase for the change with the recorded artifact.
This would explicitly cover the `matchesRecordedSuperpowersArtifact` path and help prevent regressions in artifact-based routing.
```suggestion
it('allows docs/superpowers writes for the matching design change when another active change is open', async () => {
await createChange(
tmpDir,
'cert-signature-auth',
['workflow: full', 'phase: open', 'archived: false', ''].join('\n'),
);
await createChange(
tmpDir,
'env-issue-ledger',
['workflow: full', 'phase: design', 'archived: false', ''].join('\n'),
);
const docsDir = path.join(tmpDir, 'docs', 'superpowers', 'specs');
await fs.mkdir(docsDir, { recursive: true });
const targetFile = path.join(docsDir, 'env-issue-ledger-design.md');
const result = runHookGuard(tmpDir, hookGuardScript, hookStdin(targetFile));
expect(result.status).toBe(0);
expect(result.stderr).toContain('phase: design, superpowers');
}, 20_000);
it('allows docs/superpowers writes when governing change is selected via recorded designDoc artifact', async () => {
// Create an open change without any recorded superpowers artifacts.
await createChange(
tmpDir,
'cert-signature-auth',
['workflow: full', 'phase: open', 'archived: false', ''].join('\n'),
);
// Create a design-phase change that records a classic.designDoc superpowers artifact.
await createChange(
tmpDir,
'env-issue-ledger',
[
'workflow: full',
'phase: design',
'archived: false',
'classic.designDoc: docs/superpowers/specs/env-issue-ledger-design.md',
'',
].join('\n'),
);
const docsDir = path.join(tmpDir, 'docs', 'superpowers', 'specs');
await fs.mkdir(docsDir, { recursive: true });
const targetFile = path.join(docsDir, 'env-issue-ledger-design.md');
const result = runHookGuard(tmpDir, hookGuardScript, hookStdin(targetFile));
expect(result.status).toBe(0);
expect(result.stderr).toContain('phase: design, superpowers');
}, 20_000);
```
</issue_to_address>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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
assets/skills/comet/scripts/comet-runtime.mjs (1)
11300-11313: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame substring name-matching risk as the TS source.
This mirrors
superpowersArtifactGoverningChangeindomains/comet-classic/classic-hook-guard.ts, including the boundary-unsaferelativePath2.includes(name)fallback at Lines 11307-11310. Since this is a generated/compiled artifact, the fix belongs upstream in the TS source and should propagate here via regeneration rather than being patched directly.🤖 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/scripts/comet-runtime.mjs` around lines 11300 - 11313, The fallback name matching in superpowersArtifactGoverningChange uses an unsafe substring check that can incorrectly match unrelated paths. Fix this upstream in the TypeScript source version of superpowersArtifactGoverningChange in domains/comet-classic/classic-hook-guard.ts by replacing the boundary-unsafe relativePath2.includes(name) logic with a safer path-boundary check, then regenerate the compiled comet-runtime.mjs artifact so the change propagates here rather than editing this generated file directly.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
5-11: 📐 Maintainability & Code Quality | 🔵 TrivialAlign the changelog block with the documented release-note format.
The entry header is fine, but the section markup (
### Fixed) does not match the repository rule forCHANGELOG.mdrelease notes. Please use the prescribed section structure so the changelog stays machine- and human-consistent. As per coding guidelines:CHANGELOG.mdentries must use## What's Changed [x.y.z] - YYYY-MM-DDwith sections for Added/Changed/Fixed/Tests/Removed/Security, each entry starting with- **keyword**: description.🤖 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 `@CHANGELOG.md` around lines 5 - 11, The changelog entry for the 0.4.0-beta.2 release needs to follow the repository’s required release-note structure. Update the `CHANGELOG.md` section formatting so it uses the prescribed section headings (Added/Changed/Fixed/Tests/Removed/Security) and keep each bullet in the `- **keyword**: description` style, adjusting the existing `Fixed` items to fit that format without changing the release header.Source: Coding guidelines
test/domains/comet-classic/comet-scripts.test.ts (1)
4987-5008: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage for the primary concurrent-change scenario. Consider also adding a case with overlapping/substring change names (e.g.
authvsauth-v2) to cover the name-inclusion fallback's boundary behavior, tied to the matching concern raised inclassic-hook-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 4987 - 5008, The current test covers the main concurrent-change path, but it should also exercise the name-matching fallback in classic-hook-guard.ts for overlapping change names. Add a test in comet-scripts.test.ts that uses two active changes with substring/overlapping names like auth and auth-v2, then verify the docs/superpowers write is allowed only for the correctly matching design change. Make sure the assertions prove the boundary behavior of the matching logic in the hook guard.
🤖 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 `@domains/comet-classic/classic-hook-guard.ts`:
- Around line 165-197: The fallback matching in
superpowersArtifactGoverningChange is too permissive because
relativePath.includes(name) can select the wrong GoverningChange for overlapping
or substring-only names. Tighten the name-based fallback in
superpowersArtifactGoverningChange (and, if needed, governingChangeName usage)
to use a boundary-safe match such as checking path segments or preferring the
most specific/longest eligible name, while keeping
matchesRecordedSuperpowersArtifact as the exact-path fast path.
---
Duplicate comments:
In `@assets/skills/comet/scripts/comet-runtime.mjs`:
- Around line 11300-11313: The fallback name matching in
superpowersArtifactGoverningChange uses an unsafe substring check that can
incorrectly match unrelated paths. Fix this upstream in the TypeScript source
version of superpowersArtifactGoverningChange in
domains/comet-classic/classic-hook-guard.ts by replacing the boundary-unsafe
relativePath2.includes(name) logic with a safer path-boundary check, then
regenerate the compiled comet-runtime.mjs artifact so the change propagates here
rather than editing this generated file directly.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 5-11: The changelog entry for the 0.4.0-beta.2 release needs to
follow the repository’s required release-note structure. Update the
`CHANGELOG.md` section formatting so it uses the prescribed section headings
(Added/Changed/Fixed/Tests/Removed/Security) and keep each bullet in the `-
**keyword**: description` style, adjusting the existing `Fixed` items to fit
that format without changing the release header.
In `@test/domains/comet-classic/comet-scripts.test.ts`:
- Around line 4987-5008: The current test covers the main concurrent-change
path, but it should also exercise the name-matching fallback in
classic-hook-guard.ts for overlapping change names. Add a test in
comet-scripts.test.ts that uses two active changes with substring/overlapping
names like auth and auth-v2, then verify the docs/superpowers write is allowed
only for the correctly matching design change. Make sure the assertions prove
the boundary behavior of the matching logic in the hook guard.
🪄 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: b0da2dc6-da1a-4129-a454-935d48e154b3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CHANGELOG.mdassets/skills/comet/scripts/comet-runtime.mjsdomains/comet-classic/classic-hook-guard.tsdomains/skill/platform-install.tspackage.jsontest/domains/comet-classic/comet-scripts.test.tstest/domains/skill/symlink-install.test.tswebsite
This was referenced Jul 11, 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
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
skills/directory when it contains unmanaged files, preserving local or third-party Skills (fix: comet init 里面的 Symlink安装模式会覆盖删除用户原本的文件数据 #159).docs/superpowers/writes to the matching design/build/verify change so an unrelated active change no longer blocks shared Superpowers artifacts (fix: 并行任务时comet 的门卫脚本（hook）找错了 change #160).0.4.0-beta.2and sync the website changelog/version entries through thewebsitesubmodule pointer.Tests
npx vitest run test/domains/comet-classic/comet-scripts.test.ts test/domains/comet-classic/classic-hook-guard.test.ts test/domains/skill/symlink-install.test.ts test/domains/skill/skills.test.tsgit diff --checkgit -C website diff --checkSummary by Sourcery
Protect symlink-based skill installs and adjust Classic superpowers artifact routing while preparing the 0.4.0-beta.2 release.
Bug Fixes:
Enhancements:
Build:
Summary by CodeRabbit
Bug Fixes
docs/superpowerswrites so the correct active change is used, and unmatched files are now blocked more clearly.Tests
Documentation