Skip to content

Commit db2bbc5

Browse files
Merge pull request #158 from laststance/feature/fallow-dupes-health
chore: gate fallow dupes and health
2 parents c4f9e6e + 3e4d421 commit db2bbc5

16 files changed

Lines changed: 452 additions & 317 deletions

.fallowrc.jsonc

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
{
22
// Pin the schema to the same fallow tag this repo runs (`fallow --version`
3-
// currently reports 2.65.0). Keeping `main` here would let upstream schema
3+
// currently reports 2.69.0). Keeping `main` here would let upstream schema
44
// changes silently break editor validation between fallow upgrades — bump
55
// this URL together with the package install when bumping fallow.
6-
"$schema": "https://raw.githubusercontent.com/fallow-rs/fallow/v2.65.0/schema.json",
6+
"$schema": "https://raw.githubusercontent.com/fallow-rs/fallow/v2.69.0/schema.json",
77
// electron-vite uses two HTML entries declared in electron.vite.config.ts:
88
// src/renderer/index.html → loads ./src/main.tsx (main window)
99
// src/renderer/settings/index.html → loads ./main.tsx (Settings window, see PR125)
@@ -47,29 +47,37 @@
4747
// (specs + helpers + fixtures) so `fallow dupes` only flags duplication
4848
// in production code worth refactoring.
4949
//
50-
// The 5 remaining app-code detections are deliberate — DO NOT extract:
51-
// 1. trashService.ts:287/557 (25 lines): `scanOrphanSymlinkPaths`
52-
// uses warn+continue while `moveToTrash` throws on the same
53-
// validatePath/lstat scaffold. Sharing would force the caller to
54-
// pass an error-policy callback and obscure the intent.
55-
// 2. ipc/skills.ts:78/131 (9 lines): bulk-unlink REFUSES local-skill
56-
// directories; single-unlink rm -rf's them (single has confirm UX,
57-
// bulk does not). Both inline `match({isSymlink, isDirectory})`.
58-
// 3. trashService.ts:546/781 (7 lines): `moveSourceBackedToTrash` and
59-
// `moveLocalOnlyToTrash` share return type + TRASH_DIR mkdir +
60-
// buildEntryName but diverge on `entrySourceDir` vs
61-
// `localCopiesRoot`. A shared init helper would only save 6 lines.
62-
// 4. skillScanner.ts:248/353 (7 lines): orphan vs local skill object
63-
// initializers differ in 4 fields (status, isLocal, isOrphan,
64-
// skillMdSymlinkTarget). Sharing needs 4+ parameters.
65-
// 5. CopyToAgentsModal/SkillItem (5 lines): identical Redux selector
66-
// reads — natural shape of `useAppSelector` calls per component.
50+
// The remaining trashService detections are deliberately suppressed inline
51+
// at the narrow blocks that differ only in error policy or entry layout.
52+
// Keep this ignore list broad for tests/fixtures only so new production
53+
// duplication in trashService still gets reported.
6754
"ignore": [
6855
"**/*.test.ts",
6956
"**/*.test.tsx",
7057
"**/*.spec.ts",
7158
"**/*.spec.tsx",
7259
"e2e/**"
7360
]
61+
},
62+
"health": {
63+
// Health runs without Istanbul coverage input in local validate/CI, so CRAP
64+
// would otherwise treat every unmeasured branch as risky. Keep the gate on
65+
// structural complexity and set CRAP as a current-state regression ceiling.
66+
"maxCyclomatic": 40,
67+
"maxCognitive": 40,
68+
"maxCrap": 120,
69+
// Test, E2E, Storybook, and one-off build scripts intentionally optimize
70+
// for scenario clarity over low branch count; they are covered by their own
71+
// runner gates rather than the production health budget.
72+
"ignore": [
73+
"**/*.test.ts",
74+
"**/*.test.tsx",
75+
"**/*.spec.ts",
76+
"**/*.spec.tsx",
77+
"e2e/**",
78+
".storybook/**",
79+
"scripts/**"
80+
],
81+
"suggestInlineSuppression": true
7482
}
7583
}

.github/workflows/fallow.yml

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,22 @@ on:
77
branches: [main]
88

99
jobs:
10-
dead-code:
10+
fallow:
11+
name: ${{ matrix.name }}
1112
runs-on: ubuntu-latest
13+
strategy:
14+
fail-fast: false
15+
matrix:
16+
include:
17+
- name: dead-code
18+
command: dead-code
19+
args: ''
20+
- name: dupes
21+
command: dupes
22+
args: ''
23+
- name: health
24+
command: health
25+
args: '--complexity'
1226

1327
steps:
1428
- name: Checkout
@@ -17,5 +31,5 @@ jobs:
1731
- name: Prepare
1832
uses: ./.github/actions/prepare
1933

20-
- name: Fallow dead-code
21-
run: npx fallow dead-code --fail-on-issues --format compact
34+
- name: Fallow ${{ matrix.name }}
35+
run: pnpm exec fallow ${{ matrix.command }} ${{ matrix.args }} --fail-on-issues --format compact

e2e/spec/unlink-agent.e2e.ts

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -219,18 +219,17 @@ test('unlinkFromAgent removes one valid azure-ai symlink without touching the so
219219
* handler dispatches on `lstat` kind via ts-pattern (skills.ts:130-145):
220220
*
221221
* - symlink → fs.unlink, success: true
222-
* - directory (local skill) → fs.rm -rf, success: true
222+
* - directory (local skill) → confirmed shell.trashItem, success: true
223223
* - regular file (none of above) → otherwise, success: false with copy
224-
* - lstat throws (e.g. ENOENT) → catch, success: false with extracted msg
224+
* - lstat throws ENOENT → idempotent no-op, success: true
225225
*
226-
* The two tests below pin the bottom two rows. Without them, a regression
227-
* that flipped the `.otherwise()` branch to `fs.rm` would silently delete
228-
* any regular file the renderer happens to pass — and a regression that
229-
* stopped catching lstat errors would surface as an uncaught IPC rejection
230-
* in the renderer (worse UX than a structured failure row).
226+
* The two tests below pin the bottom two rows. Without them, double-clicks
227+
* could surface stale-path errors and a regression that flipped the
228+
* `.otherwise()` branch to a destructive path would silently delete any
229+
* regular file the renderer happens to pass.
231230
*/
232231

233-
test('unlinkFromAgent returns structured failure when linkPath does not exist', async ({
232+
test('unlinkFromAgent treats a missing linkPath as an idempotent success', async ({
234233
appWindow,
235234
isolatedHome,
236235
}) => {
@@ -244,8 +243,8 @@ test('unlinkFromAgent returns structured failure when linkPath does not exist',
244243
const missingLinkPath = join(claudeAgentPath, 'never-existed')
245244

246245
// Sanity — confirm the path is genuinely absent before the IPC fires. A
247-
// false positive here would be silent: the handler's success path also
248-
// swallows ENOENT, so we'd think the failure-branch fired when it didn't.
246+
// false positive here would silently test the normal unlink path instead of
247+
// the idempotent missing-path branch.
249248
expect(existsSync(missingLinkPath)).toBe(false)
250249

251250
const ipcResult = await appWindow.evaluate(
@@ -258,13 +257,7 @@ test('unlinkFromAgent returns structured failure when linkPath does not exist',
258257
},
259258
)
260259

261-
expect(ipcResult.success).toBe(false)
262-
// ENOENT is the surface form on macOS + Linux; pinning the substring
263-
// keeps a `extractErrorMessage` rewrite from silently passing this test.
264-
// `UnlinkResult` is { success: boolean, error?: string } (not a discriminated
265-
// union) — toMatch on undefined would throw, so this also covers
266-
// "error must actually be populated when success is false".
267-
expect(ipcResult.error).toMatch(/ENOENT|no such file or directory/i)
260+
expect(ipcResult).toEqual({ success: true })
268261
})
269262

270263
test('unlinkFromAgent refuses a regular file with the structured kind-mismatch error', async ({

package.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@
2323
"typecheck": "tsc --noEmit",
2424
"lint": "eslint . --max-warnings 0",
2525
"lint:fix": "eslint . --fix",
26-
"fallow:dead-code": "pnpm exec fallow dead-code",
27-
"validate": "run-p lint test typecheck fallow:dead-code storybook:build",
26+
"fallow:dead-code": "pnpm exec fallow dead-code --fail-on-issues",
27+
"fallow:dupes": "pnpm exec fallow dupes --fail-on-issues",
28+
"fallow:health": "pnpm exec fallow health --complexity --fail-on-issues",
29+
"validate": "run-p lint test typecheck fallow:dead-code fallow:dupes fallow:health storybook:build",
2830
"prepare": "husky",
2931
"prettier": "prettier --ignore-unknown --write .",
3032
"clean": "rm -rf dist build .vite node_modules pnpm-lock.yaml"

src/main/ipc/ipc-schemas.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ export const IPC_ARG_SCHEMAS: Partial<Record<IpcInvokeChannel, z.ZodTuple>> = {
183183
skillName: skillNameString,
184184
agentId: nonEmptyString,
185185
linkPath: nonEmptyString,
186+
confirmedLocalDirectoryDelete: z.boolean().optional(),
186187
}),
187188
]),
188189
'skills:removeAllFromAgent': z.tuple([

src/main/ipc/skills.ts

Lines changed: 70 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,47 @@ import type {
2727
import { typedHandle } from './typedHandle'
2828
import { typedSend } from './typedSend'
2929

30+
type AgentPathRemovalResult =
31+
| { success: true }
32+
| { success: false; error: string; code?: string }
33+
34+
/**
35+
* Remove an agent symlink path after the caller has already validated and
36+
* lstat'd it. Directories are refused here so destructive local-folder removal
37+
* remains isolated to the single-unlink confirmation path.
38+
* @param linkPath - Validated path inside an agent skills directory
39+
* @param stats - `lstat` result for linkPath
40+
* @returns Structured IPC result for renderer toast handling
41+
* @example
42+
* removeLinkPathByKind('/Users/me/.cursor/skills/task', stats)
43+
*/
44+
async function removeLinkPathByKind(
45+
linkPath: AbsolutePath,
46+
stats: Stats,
47+
): Promise<AgentPathRemovalResult> {
48+
return match({
49+
isSymlink: stats.isSymbolicLink(),
50+
isDirectory: stats.isDirectory(),
51+
})
52+
.with({ isSymlink: true }, async () => {
53+
await fs.unlink(linkPath)
54+
return { success: true } as const
55+
})
56+
.with(
57+
{ isSymlink: false, isDirectory: true },
58+
async () =>
59+
({
60+
success: false as const,
61+
error:
62+
'Cannot unlink a local skill. Use Delete to move it to trash instead.',
63+
}) satisfies AgentPathRemovalResult,
64+
)
65+
.otherwise(async () => ({
66+
success: false as const,
67+
error: 'Cannot remove: path is neither a symlink nor a directory',
68+
}))
69+
}
70+
3071
/**
3172
* Unlink or remove a single link-path inside an agent directory. Shared by the
3273
* single-unlink handler and the batch-unlink handler so both behave identically.
@@ -72,27 +113,7 @@ async function removeFromAgent(
72113
}
73114

74115
try {
75-
// Discriminate on file-stat kind: symlink → unlink, local directory →
76-
// refuse (bulk unlink is non-destructive; user must go through bulk
77-
// Delete), everything else (files, sockets, etc.) → refuse as unsupported.
78-
const kindResult = await match({
79-
isSymlink: stats.isSymbolicLink(),
80-
isDirectory: stats.isDirectory(),
81-
})
82-
.with({ isSymlink: true }, async () => {
83-
await fs.unlink(linkPath)
84-
return { success: true } as const
85-
})
86-
.with({ isSymlink: false, isDirectory: true }, async () => ({
87-
success: false as const,
88-
error:
89-
'Cannot unlink a local skill. Use Delete to move it to trash instead.',
90-
}))
91-
.otherwise(async () => ({
92-
success: false as const,
93-
error: 'Cannot remove: path is neither a symlink nor a directory',
94-
}))
95-
return kindResult
116+
return await removeLinkPathByKind(linkPath, stats)
96117
} catch (error) {
97118
return {
98119
success: false,
@@ -116,36 +137,42 @@ export function registerSkillsHandlers(): void {
116137
* @returns UnlinkResult with success status and optional error
117138
*/
118139
typedHandle(IPC_CHANNELS.SKILLS_UNLINK_FROM_AGENT, async (_, options) => {
119-
const { linkPath } = options
140+
const { linkPath, confirmedLocalDirectoryDelete = false } = options
120141

121142
try {
122143
// Allow agent dirs (for local skills) AND SOURCE_DIR (for symlinked skills).
123144
// validatePath calls realpathSync, which follows the symlink to its source
124145
// in ~/.agents/skills/. Without SOURCE_DIR in the allowed bases, every
125146
// symlinked-skill unlink fails with "Path traversal attempt detected".
126147
validatePath(linkPath, getAllowedBases())
127-
const stats = await fs.lstat(linkPath)
128-
// Discriminate on file-stat kind: symlink → unlink, directory → rm -rf
129-
// (destructive is OK here because the single-unlink handler has its own
130-
// confirmation UX in the renderer), else → refuse.
131-
const unlinkResult = await match({
132-
isSymlink: stats.isSymbolicLink(),
133-
isDirectory: stats.isDirectory(),
134-
})
135-
.with({ isSymlink: true }, async () => {
136-
await fs.unlink(linkPath)
137-
return { success: true } as const
138-
})
139-
.with({ isSymlink: false, isDirectory: true }, async () => {
140-
await fs.rm(linkPath, { recursive: true, force: true })
141-
return { success: true } as const
142-
})
143-
.otherwise(async () => ({
144-
success: false as const,
145-
error: 'Cannot remove: path is neither a symlink nor a directory',
146-
}))
148+
let stats: Stats
149+
try {
150+
stats = await fs.lstat(linkPath)
151+
} catch (error) {
152+
if (errorCode(error) === 'ENOENT') {
153+
// Already gone — match bulk unlink's idempotent no-op behavior.
154+
return { success: true }
155+
}
156+
throw error
157+
}
158+
159+
if (stats.isDirectory() && !stats.isSymbolicLink()) {
160+
if (!confirmedLocalDirectoryDelete) {
161+
return {
162+
success: false,
163+
error:
164+
'Refusing to delete a local skill without explicit confirmation.',
165+
}
166+
}
167+
168+
// Local skill deletion arrives from UnlinkDialog's destructive confirm
169+
// action. Move to OS Trash instead of recursively deleting bytes so a
170+
// mistaken confirmation is still recoverable at the filesystem level.
171+
await shell.trashItem(linkPath)
172+
return { success: true }
173+
}
147174

148-
return unlinkResult
175+
return await removeLinkPathByKind(linkPath, stats)
149176
} catch (error) {
150177
return { success: false, error: extractErrorMessage(error) }
151178
}

0 commit comments

Comments
 (0)