Uh oh!
There was an error while loading. Please reload this page.
feat(skills): let the panel delete user-scope skills - #1517
Merged
Conversation
The Skills panel only ever showed a delete button for skills physically
under `{workspaceRoot}/skills/`, because `deleteSkill` could only resolve
that one path. Everything discovered under `~/.maka/skills` or
`~/.agents/skills` — i.e. most of what a user installs — silently lost the
button with no explanation, which read as "the app's own skills are
deletable but mine aren't".
Address the backend limit rather than the symptom: deletion is now addressed
by scope-aware `ref` instead of a bare workspace id.
`deleteSkillByRef` takes only the ref string and re-derives the directory
from a fresh discovery scan — a renderer-supplied path is never trusted. The
target must then clear four checks before the recursive rm:
1. its scope is deletable (`isManageableSkill`),
2. it is a real directory, not a symlink,
3. its parent is one of the discovery dirs the scan actually enumerated,
4. it is still inside its discovery root after realpath resolution.
(3) is the load-bearing one: the containment root for user scope is the whole
home dir, so anchoring to the enumerated scan dirs is what keeps a delete
pinned to `~/.maka/skills` / `~/.agents/skills`.
Project scope stays undeletable and now returns a distinct `blocked_scope`
rather than being invisible: `{cwd}/.maka/skills` and `{cwd}/.agents/skills`
live in the user's repo and are normally tracked by git, so removing them
belongs to the project, not to this panel. Keeping the reason separate from
`blocked_path` stops a policy refusal from reading as a traversal block.
The panel now keys the armed-delete state on ref too — the same skill id can
be discovered in several scopes, and an id-keyed confirm armed every copy at
once.
Deletion stays a permanent recursive rm behind the existing two-step confirm,
matching how workspace-scope deletion already behaved.
Tests: user-scope delete by ref, project-scope refusal leaving the repo file
on disk, symlinked user skill (discovery skips it, so it is not_found before
the symlink guard is even reached), and forged refs that resolve to nothing.jackwenerforce-pushed
the
claude/skill-delete-user-scope
branch
from
July 27, 2026 09:05
a245274 to
98262f2CompareUh oh!
There was an error while loading. Please reload this page.
jackwener added a commit
that referenced
this pull request
Jul 27, 2026
#1517 let the Skills panel delete from `~/.maka/skills` and `~/.agents/skills`. `buildE2eEnv` never overrode HOME, so an E2E run enumerated the developer's real user-scope skills — and a spec that exercised the delete button would have removed one of them for real. That is why #1517 shipped without an e2e for its own destructive path. Set HOME (and USERPROFILE) to a directory inside the throwaway userData dir that teardown already removes. Overriding the home dir sandboxes every consumer at once, rather than threading a `homeDir` option through each skills API and hoping none is missed — if list and delete disagreed about which directories they mean, the bug would be invisible and destructive. userData is pinned separately via app.setPath, so this does not move the app's data dir. `e2eHomeDir()` exposes the sandbox to specs. It is a plain accessor read from the test body, deliberately not a fixture: a fixture would have no declared dependency on the window fixture, so Playwright could resolve it first and hand back a stale path. The invocable-skills seeder gains one user-scope skill under the sandbox, which is what makes the journey assertable. New spec covers what the unit tests cannot — the renderer sending a scope-aware ref through IPC and the list agreeing afterwards: - a user-scope skill is removed from disk and drops out of the list, with the disk still intact after the FIRST of the two confirm clicks; - a project-scope skill offers no delete button at all. Verified the disk assertion is not vacuous: inverting it to expect 'present' fails with `Received: "gone"`. Verified the sandbox introduces no new failures. Full e2e: 69 passed, 3 failed — `settings.spec.ts:141` and `sidebar-navigation.spec.ts:10` fail identically with this change stashed (pre-existing on this machine, green in CI), and `sidebar-navigation.spec.ts:30` is flaky, passing on the stashed baseline in the same session. Gates: desktop test + typecheck exit 0, format:check and knip clean.
Astro-Han added a commit
that referenced
this pull request
Jul 30, 2026
desktop-real-window-smoke was the last launcher still spreading process.env wholesale: a developer with npm run dev open smoked the dev server instead of the build the script just made, and the run touched the real $HOME (#1517's skills-deletion surface). It now launches through buildFixtureEnv with an explicit visible window. The dock contract drops its duplicated fourth case and pins the wiring instead - the regression this module exists for was the call site re-deriving startHidden, which no pure-function test can observe. The reveal contract follows fixture-env's pure shape (the ambient CI read now lives in isCiLinuxDisplay, composed at each call site).
Astro-Han added a commit
that referenced
this pull request
Jul 30, 2026
* fix(desktop): launch the real-window smoke with a visible window The real-window smoke launches the app with MAKA_E2E_FIXTURE, which makes main.ts compute startHidden and main-window.ts keep the window hidden for its whole lifecycle. The gate therefore had nothing to look at: on a clean main its own programmatic-window-visible check reports visible=false and exits 1, and every human check it prompts for — dragging window edges and corners to resize, dragging the titlebar to move, Tab/Shift+Tab traversal inside the modal — is unrunnable against an invisible window. Set MAKA_E2E_SHOW_WINDOW on the launch env, which is exactly the escape hatch main.ts already provides for runs where there is no focus to steal. Capture runs and CI e2e do not set it and keep their hidden-window behavior unchanged. * fix(desktop): keep the dock icon for windows asked to be visible Hiding the dock icon makes the run an accessory app. Contrary to what the migration tracker assumed, such a window is fully clickable — measured on darwin: with MAKA_E2E_SHOW_WINDOW set, the fixture window reports isVisible/isFocused true and takes clicks, drags, and keyboard input normally. What it does not have is a Dock tile or a Cmd+Tab entry, so a reviewer doing manual visual comparison who switches to another app has no way back to it. Key the dock icon off the same startHidden that decides whether the window shows at all, instead of re-deriving the condition from MAKA_E2E_FIXTURE. The two can no longer drift, and MAKA_E2E_SHOW_WINDOW now opts out of both halves of staying out of sight. Capture runs and CI e2e leave the variable unset, so they keep hiding the window and the dock exactly as before. Lock it with a dock diagnostic and a programmatic-dock-visible check in the real-window smoke, which reported dockVisible=false before the change and true after. * feat(desktop): add a launch-only fixture mode to the real-window smoke A migration that must stay visually neutral needs to look at the running app constantly, but the only way to boot a route with fixture data was the smoke gate, which insists on walking a reviewer through twelve checklist prompts and filing a report every time. Add --manual: same launch path, then stop. The window stays up until it is closed or interrupted. Reusing the gate's launcher keeps the window a reviewer inspects identical to the window the gate checks, and leaves one place that knows how to boot a fixture instance. Expose it as `launch:fixture`, and lift the build chain the smoke scripts repeated verbatim into `build:with-deps` rather than copying it a third time. * feat(desktop): add the Astryx migration computed-style contract #1565 PR 0. The mega-branch produced regressions on screens nobody edited, and slicing alone does not localise them: the signal has to come from a baseline that says what the app looked like before. This captures one. check:visual-contract walks the five representative fixture routes in light and dark, records every visible element's box, paint, and alignment properties, and diffs the result against committed JSON. The alignment set (align-items/justify-content/place-items/gap/grid-area) is there because flex and grid misalignment is exactly the class a paint-only property set cannot see. It gates no-change, not correctness: a pre-existing bug on main stays out of scope, and zero diff means the migration did not move this element, never that the element is right. The boot sequence audit-alignment.mjs already worked out — spawn Electron with MAKA_E2E_FIXTURE, wait for a CDP page target, evaluate in the renderer — moves to fixture-cdp.mjs so both scripts share one launcher instead of growing a second. It also gains what a contract harness needs and an audit could do without: animations and webfont reflow frozen before capture, an evaluate deadline so a wedged renderer fails loudly instead of hanging, and process-group teardown so Electron's helpers do not pile up and starve later windows. Also carries #1565's third PR 0 item, salvaging the mega-branch's late regression fixes. Six of the eight land on properties this snapshot already records — a wrapped titlebar row, a timestamp stacking above its title, a missing gap, a column painting over the settings rail all move a rect or a recorded property, so the diff catches them without bespoke rules. What bespoke rules do add is noticing when the harness stops watching, so each one is pinned to an anchor that must keep appearing in the baseline. The remaining two fix files the mega-branch itself created, which do not exist on main; they are recorded in the PR body instead. Determinism was verified by capturing the same build twice. The first attempt was not clean, and both causes are excluded rather than tuned around: element labels fell back to el.id, which Base UI regenerates every render, and OverlayScrollbars' chrome fades with pointer activity, so its visibility was a coin flip. Baselines stay at 1.5MB rather than 4MB because properties equal to their initial value, or inherited unchanged from the parent, are omitted and read back as unchanged. Migration-only: deliberately outside CI, following check:chat-visual, and removed in PR 14 with the maka.legacy layer. * feat(desktop): add the Astryx migration hit-test contract #1565 PR 0. The mega-branch's most expensive regressions were controls that render perfectly and refuse clicks. A computed-style snapshot cannot see that, so it gets a contract of its own: every visible interactive element on the five routes must be reachable at its centre, must not sit under a pointer-events or visibility trap, must not be transparent through an ancestor, and must not fall inside an -webkit-app-region: drag area. Three parts of #1565's proposal did not survive contact with a clean main, and each is replaced by something answering the same question: The drag-region probe was specified as a real click plus a side effect. Fixture windows are hidden, and an unmapped window does not route synthesized input through the browser's hit-testing path, so every target read as swallowed. Making the window visible to fix that turned the probe into something that fires real product actions — one opened a native file dialog and hung the run. Reading -webkit-app-region off the stylesheet rules instead has no side effects: the property is missing from getComputedStyle, but not from the rules that declare it. All five probe points were specified as required. On a clean main that is unreachable: sibling chrome legitimately covers a couple of edge pixels, so a workbar resize handle would indict a control nobody struggles to click. The centre is now required and corners corroborate — three lost corners means something covers the control. The overlay guard keyed on [role=dialog][open], which a React-rendered div[role=dialog] never has, so the settings surface went undetected and every control behind it was reported unhittable. It now keys on the product's own data-modal="true". Geometry probes do need a visible window — elementFromPoint disagrees with getBoundingClientRect against a hidden one — so this check steals focus for a few seconds per route. Memoising style and rect lookups took the densest route from a 60s timeout to 3s, and routes are spaced so one window is fully gone before the next boots. All five routes are clean on ae43cb2. * refactor(scripts): make the migration contract harness trustworthy Review found the harness could report `ok` for a window it never launched, could not see the class of change the migration actually makes, and shipped its only product change with no automated coverage. Launch through the shared E2E seam. The harness rolled its own Electron launcher — fixed debug ports, a /json/list poll, a raw WebSocket, a process-group kill, `...process.env` — all of it already solved next door in `apps/desktop/e2e/fixtures.ts`, and each hand-rolled version worse. It never verified the CDP target belonged to the child it spawned, and the port allocator restarted at 14600 in every process, so a leftover Electron on that port got captured instead. Reproduced: two concurrent runs requesting different scenarios both read the same window. When the leftover is the same route from an older build — the shape you get by re-running one route after an edit — the contract reports `ok` for a stale window. `detached: true` with no signal handler supplied the leftovers; this machine had 270 stale user-data dirs, none ever removed. `buildE2eEnv` moves to `scripts/fixture-env.mjs`, shared by both callers; its own comment already described the bug the harness had, since inheriting the environment leaks `VITE_DEV_SERVER_URL` and loads the dev server instead of the build under test. Launch, readiness and teardown now go through Playwright's Electron support: fixed sleeps become per-route readiness selectors, and teardown removes its temp dir. 235 lines become 139, and the hit-test retry goes with them, per the sibling harness's rule that flakes should fail loudly. Stop committing baselines. They encode the capturing host — font metrics and the macOS traffic-light inset — so one machine's baseline reads as thousands of diffs on another. Capture on the branch you compare against, apply the slice, compare: the two captures that matter always come from the same host. The timezone is pinned through the fixture's IANA override for the same reason. This removes 70k lines from review and makes the orphan-baseline check moot. Record what a cascade flip actually changes. `boxShadow`, `textAlign`, `flexDirection`, `flexWrap` and `outline` were missing, so a utility that starts beating a product rule could change elevation or text placement with an identical rect and be reported clean. Three `INITIAL_VALUES` entries guessed at values Chromium does not serialise and never matched once, leaving constants on all 3,964 records; `findDeadOmissionRules` now fails the run when a rule stops matching — it caught a fourth such entry immediately. Inherited values were compared against the parent rather than the nearest recorded ancestor, so a zero-box wrapper's color could change, repaint every visible child, and leave the capture byte-identical: 129 masked values on the chat route alone. Delete the second drag-region resolver. It parsed stylesheet rules in source order, ignoring specificity, `!important` and `@layer` — which PR 1 and PR 2 introduce — on the premise that the property is absent from computed style. It is not: `getComputedStyle(el).webkitAppRegion` returns `drag`, and `e2e/window-titlebar.spec.ts` has read it that way in CI all along, against rendered geometry and document order. The hit-test keeps what that spec does not cover, and now reports how many elements it actually probed rather than how many the selector matched. Pin the product change. `resolveDockPresentation` is a pure function with tests; the branch it replaced could only be exercised by launching Electron, which is why it had none. The programmatic smoke checks are now pinned by the contract test that exists to pin checks — including the new dock one, which could previously be deleted with CI green — and `e2e` shares `build:with-deps` instead of repeating the chain verbatim. * fix(scripts): keep buildFixtureEnv pure and restore the audit settle budget buildFixtureEnv read process.env.CI inline, so the fixture-env test asserting a hidden run stays hidden passed on every laptop and failed on the Linux CI runner. The ambient read now lives in isCiLinuxDisplay for callers to compose explicitly. audit-alignment gets its 2500ms settle back (the shared-launcher refactor had silently dropped it to 1000ms against static-markup readiness) plus a real ready selector per fixture, and its header now states the deliberate environment change from the raw-spawn launcher it replaced. * fix(scripts): bound fixture teardown with the shared Electron close withFixtureWindow tore down with a bare app.close(), which has no deadline — one wedged launch turned a capture run (or the CI alignment audit, which has no job timeout) into an infinite hang, and the temp userData dir leaked because rm was sequenced after the close. The bounded close the E2E suite already owned moves to scripts/electron-lifecycle.mjs — the same shared-home precedent as fixture-env.mjs, so a bare-node harness and the Playwright suite stop splitting one launch concern across two directories. Renderer evaluate gets a deadline too; teardown then reclaims the process either way. * fix(scripts): capture only what paints and inherit textAlign Two measurement-semantics gaps in the visual capture, both instances of a class the harness had already been bitten by: - textAlign was in PROPERTIES but not INHERITED_PROPERTIES, so 79-85% of records re-recorded an ancestor's value and one container change printed as N descendant lines. It moves to the inherited omission (and out of INITIAL_VALUES — absent must mean exactly one thing). A static table test now pins every CSS-inherited property to that route, which is the guard findDeadOmissionRules could never provide. - Descendants of an opacity:0 element report opacity 1 and a non-zero rect, so the collapsed session panel put 105 invisible records into the chat capture; a change under it would diff on pixels that never paint. The walk now prunes zero-opacity subtrees. The prune exposed that two salvaged anchors were matching those phantom records: the chat fixture never shows the session rows at all. They now point at mcp-hub, whose fixture opens the sidebar for real. * feat(scripts): compare both sides in one run and cover the win32 cascade Replaces the two-step gitignored-baseline workflow. The checker now builds the base ref in a cached temp worktree (node_modules shared; the compare refuses to run across a package-lock change), builds the working tree, captures every route x theme x platform from both builds, and diffs in memory. Every step of the old workflow a human could skip - rebuild after switching, recapture after rebasing, stash on a committed slice - produced a zero-diff pass that had measured the same binary twice; now the instrument owns the whole measurement and there is no baseline file to trust or to stale. The matrix gains the win32 column #1565 asked for: the fixture platform override drives the production app:info -> data-os path, so the per-OS cascade is captured on any host - no Windows runner involved. * fix(scripts): route the smoke gate through the shared launch env desktop-real-window-smoke was the last launcher still spreading process.env wholesale: a developer with npm run dev open smoked the dev server instead of the build the script just made, and the run touched the real $HOME (#1517's skills-deletion surface). It now launches through buildFixtureEnv with an explicit visible window. The dock contract drops its duplicated fourth case and pins the wiring instead - the regression this module exists for was the call site re-deriving startHidden, which no pure-function test can observe. The reveal contract follows fixture-env's pure shape (the ambient CI read now lives in isCiLinuxDisplay, composed at each call site). * fix(scripts): isolate the base build and make captures prove their identity Round-3 review found the compare's base was not a base: the borrowed node_modules resolved @maka/* through workspace links straight back to the working tree, so the base bundled and loaded the candidate's own packages — a slice changing @maka/core would have compared itself against itself. The base now runs its own npm ci against its own lockfile in the cached worktree (atomic lock against concurrent builds; the marker name encodes the scheme so old symlink-era caches rebuild). The lockfile-parity gate and the symlink list existed only to justify the sharing; both are deleted, and a slice may now legitimately change dependencies. The same round showed the instrument asserting nothing about what it measured, so now it proves it: route/theme/platform are closed sets; every capture waits for the requested data-os and theme on the live document before reading a style; painted ::before/::after get paint signatures (47 pseudo rules paint in this renderer, led by the body::after film grain, and a cascade flip on any of them was invisible); mixBlendMode joins the property set; and dead omission rules are counted at the raw sample where the border/outline gates cannot shadow them. * fix(scripts): bound the smoke gate's stop paths The smoke gate still had two unbounded exits: --manual waited forever on 'exit' after one SIGTERM, and the report path fired SIGTERM and called process.exit without waiting or escalating — plus a failed report write skipped the kill entirely. All paths now stop through the shared bounded close (SIGTERM as the graceful phase, 5s grace, SIGKILL the tree), inside a finally. electron-lifecycle's import of @maka/runtime now fails with an error that says what to build; on a fresh clone the raw ERR_MODULE_NOT_FOUND surfaced from four files deep in the import chain. * fix(scripts): land the SIGKILL escalation on a non-group-leader Electron The shared teardown's force path kills by process group. The smoke gate spawned Electron without detached, so the group signal hit ESRCH, the terminator read that as "tree already gone", and a visible window that had ignored SIGTERM survived its own SIGKILL stage. Spawn the smoke child as a group leader like every runtime consumer does, and back the group kill with a direct kill on the root so a future non-leader child still dies inside the 2s exit deadline. * fix(scripts): capture the paint-only channels with animation time frozen filter, backdrop-filter, clip-path and transform move pixels without moving the rect — grayscale() on disabled provider rows, the glass theme's backdrop blur, clip-path as visually-hidden, rotate() on square chevrons — so losing one was invisible to every property the contract sampled. Capture all four (plus transform-origin, gated on a transform actually painting), and stop the renderer's clock first: a running spinner serialises a different matrix at every phase, which would diff time rather than the cascade. Verified with a negative control (an injected filter rule fails the compare) and a clean 20/20 run. Also correct the header, which still described the rejected node_modules-sharing base build, and compare the direct-run guard through pathToFileURL so a percent-encoding checkout path cannot turn the instrument into a silent exit-0 no-op. * fix(scripts): fail closed on unknown hit-test routes --route accepted any value and only failed when nothing matched, so `--route chat --route chatt` dropped the typo and exited 0 as "1 route(s) clean" — silently shrinking the requested coverage. Validate each value against the closed route set (as the visual contract already does), guard the entry point through pathToFileURL, and pin the CLI's exit codes with subprocess tests. * test(desktop): cover the direct-kill backstop in the bounded close The ESRCH-swallowing group-kill path was only proven against a live window; the fake now exercises it too — a terminator that returns without killing must be followed by a direct SIGKILL on the root.
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 freeto 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.
What
The Skills panel now deletes user-scope skills (
~/.maka/skills,~/.agents/skills), not just skills under{workspaceRoot}/skills/.Why
deleteSkillcould only resolvejoin(root, 'skills', id), so the UI hid the button for anything else viamanageable. The hidden-button behaviour was correct — the backend genuinely couldn't delete those — but the effect was that the app's own bundled skill showed 删除 while all 25 of the user's own installs did not, with nothing explaining why. So this fixes the backend limit rather than the UI symptom.How
Deletion is addressed by scope-aware
refinstead of a bare workspace id.deleteSkillByRefreceives only the ref string and re-derives the directory from a fresh discovery scan — a renderer-supplied path is never trusted. The target must then clear four checks before the recursiverm:isManageableSkill),realpathresolution.(3) is the load-bearing check. The containment root for user scope is the entire home dir, so
isPathInside(home, …)alone would be nearly meaningless; anchoring to the enumerated scan dirs is what keeps a delete pinned to~/.maka/skills/~/.agents/skills.The panel also keys the armed-delete state on ref rather than id — the same id can be discovered in several scopes, and an id-keyed confirm armed every copy at once.
Deliberate carve-out: project scope
{cwd}/.maka/skillsand{cwd}/.agents/skillsstay undeletable, but now return a distinctblocked_scopewith an explanatory toast instead of just missing a button. Those files live in the user's repo and are normally tracked by git; removing them belongs to the project, not to this panel. Keeping the reason separate fromblocked_pathstops a policy refusal from reading as a path-traversal block in logs.Easy to reverse — it is one branch in
isManageableSkill.Behaviour note
Deletion remains a permanent recursive
rm(no trash) behind the existing two-step, 4-second confirm. That is unchanged from how workspace-scope deletion already worked, and was an explicit call —shell.trashItemwas considered and rejected in favour of matching existing behaviour.Tests
Four new cases in
skills.test.ts, all against a real temp filesystem:blocked_scopeand the repo file is still on disk afterwards;not_found— discovery skips symlinked dir entries, so it never reaches the symlink guard (which stays as defence in depth), and the link target survives;user:agents:../../../etc, wrong-scope refs, …) resolve to nothing.Updated the existing
manageablecontract:user-helper→true,project-helper→false.Verification
node --test dist/main/**/*.test.js— 2879/2879 pass.npm run typecheck— clean. It caught a miss:bridge-contract.d.tsis the authoritative renderer-facing API declaration, and without updating it the newblocked_scopereason would have been swallowed by the type system.Two things to know
mainis currently red oncheck-console, independent of this PR —permission-overlay-main.tsfrom feat(permissions): drag-to-grant onboarding for macOS TCC (Stage 1) #1515 has three ungatedconsole.warnsites not in the ALLOW map. That blocksnpm run testlocally and will likely fail CI here too. Not introduced by this branch; happy to send a separate one-line unblock.HOME(buildE2eEnvleaves it alone). So there is no e2e for user-scope deletion — writing one would have the suite delete real skills out of the developer's home dir. Adding a HOME sandbox to the fixture is the prerequisite and is left out of scope. The destructive path is covered by the unit tests above; the UI wiring is pinned by the source-contract assertions.