Skip to content

Commit 760863d

Browse files
committed
fix(consent): blocking modal for the first-run hooks prompt
1 parent a4902c6 commit 760863d

9 files changed

Lines changed: 283 additions & 143 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,7 @@ Supporting: `wall-tile-editor.html` (wall sprite editing), `jsonl-viewer.html` (
628628
- **Inline esbuild problem matcher** (no extra extension needed).
629629
- **`erasableSyntaxOnly`** in webview forbids `enum` — use `as const` objects.
630630
- **Server always starts** regardless of hooks toggle. Only hook installation is gated by the setting.
631-
- **Consent before any FIRST `~/.claude/settings.json` write**, one shared `hooksConsentGiven` flag across surfaces. **Exactly one population is prompted: the one with nothing of ours installed.** With our hooks already present but no recorded consent (a pre-consent version installed them silently), consent is granted and the install runs with **no prompt at all** — deliberate zero friction, since that install only ever removes events, never adds them. The accepted cost: consent is read off artifacts our own pre-consent code wrote, and afterwards the **only** removal route is the Settings toggle the disclosure names (pinned in `consent.spec.ts`). The branch is chosen by `areHooksInstalled` = **ANY of our commands on ANY event** — an all-or-nothing reading made a partial install look like "nothing installed", so live hooks were presented as absent and "Don't Ask Again" persisted hooks-off without removing them. The first-run prompt discloses scope, where payloads go, and how to undo (`consentCopy.ts`); VS Code renders it as a **non-modal notification** (`Install Hooks` / `Not Now` / `Don't Ask Again`) carrying the full disclosure in its message — a notification with buttons is permanently expanded with no line clamp and truncates only at 1000 chars, pinned by `consentCopy.test.ts`; standalone asks `[Y/n/never]` on a TTY and installs nothing without one. An unanswered prompt auto-hides to the bell, writes nothing, and asks again next startup.
631+
- **Consent before any FIRST `~/.claude/settings.json` write**, one shared `hooksConsentGiven` flag across surfaces. **Exactly one population is prompted: the one with nothing of ours installed.** With our hooks already present but no recorded consent (a pre-consent version installed them silently), consent is granted and the install runs with **no prompt at all** — deliberate zero friction, since that install only ever removes events, never adds them. The accepted cost: consent is read off artifacts our own pre-consent code wrote, and afterwards the **only** removal route is the Settings toggle the disclosure names (pinned in `consent.spec.ts`). The branch is chosen by `areHooksInstalled` = **ANY of our commands on ANY event** — an all-or-nothing reading made a partial install look like "nothing installed", so live hooks were presented as absent and "Don't Ask Again" persisted hooks-off without removing them. The first-run prompt discloses scope, where payloads go, and how to undo (`consentCopy.ts`); VS Code renders it as a **blocking modal** (`Install Hooks` / `Not Now` / `Don't Ask Again`) — headline as the message, `CONSENT_DISCLOSURE` as `detail`, the slot VS Code renders only for a modal — because this consent must be unmissable, not merely available; standalone asks `[Y/n/never]` on a TTY and installs nothing without one. **Exactly three buttons**: `Not Now` is passed as a `MessageItem` with `isCloseAffordance`, so VS Code uses it AS the cancel affordance instead of synthesizing a fourth `Cancel` that did precisely what `Not Now` did. The overloads are homogeneous (`T extends string` | `T extends MessageItem`), so marking one item forces all three, and the outcome routing compares by object IDENTITY — a string comparison against a `MessageItem` return is the silent failure this shape rules out. **Only `Don't Ask Again` writes anything on a decline**: `Not Now`, Escape, and the close `x` all write nothing and ask again next startup. Escape/close no longer resolve to `undefined` — VS Code routes them through the dialog's `cancelId` to the close-affordance ITEM — so the handler is fail-closed on identity: only an exact match writes, and every other value (both dismissal forms included) falls through to the no-write path. The e2e specs can see the modal only because `launch.ts` seeds `window.dialogStyle: custom` (a native dialog is invisible to Playwright), and `arrangeReviewLayout` bails out while one is open (the blocking overlay eats the sash drag) with `openSettingsModal` repairing the layout on demand.
632632
- **`hooksStatus` is install state, `hooksEnabled` is preference.** The Settings checkbox binds to `hooksInstalled` and toggles the _displayed_ state with no optimistic local update, so it can't read "on" over an untouched settings.json and lands correct rather than flickering when an install fails. Every failure path re-derives and broadcasts the truth (standalone via `clientMessageHandler`, VS Code via `reportHooksStatus`). The hook script is copied BEFORE the entries are written; a failed copy aborts the install (entries pointing at a missing script spawn a dead `node` per event). **Hooks-off is persisted only AFTER a successful uninstall** — flipping it first strands the user: the entries keep firing while the persisted preference makes the next start skip the gate entirely.
633633
- **Never rewrite a shape we did not author.** The unparseable-file abort generalizes: a non-object `hooks`, a non-array `hooks.<Event>`, and junk entries inside an event array are all refused or passed through, never replaced. An array `hooks` was the sharp case — string keys assigned onto it vanish from `JSON.stringify`, so the write committed and reported `installed: true` over a file with no hooks in it. Emptied event keys are deleted only when _our_ removal emptied them. **Internal sentinels must not be values user JSON can hold**: `null` marked "this entry is now empty", so a user-authored `null` inside a hooks array was silently deleted (a file with no Pixel Agents command anywhere came back rewritten and logged as "Hooks removed") — it is a `Symbol` now.
634634
- **Hook identity is anchored at both ends, not a substring.** `includes('claude-hook.js') && includes('.pixel-agents')` claimed — and `uninstallHooks` then DELETED — a `.backup` copy of our script, a shell comment naming our path, a wrapper passing it as an argument, `/opt/evil.pixel-agents/hooks/claude-hook.js`, and `my-pixel-agents-hook.js`. Ours = the `/.pixel-agents/hooks/claude-hook.js` suffix, ending the command's FIRST token, matched **case-insensitively** (the token is normalized to lower case). Case-sensitive matching is what shipped, and on the case-insensitive volumes this runs on (macOS, Windows) a differently-cased path is the SAME INODE as our script and genuinely firing: reinstall appended a duplicate and uninstall left the cased entry as an orphan our own `areHooksInstalled` could no longer see — a live hook with no removal route. The folding is unconditional (no filesystem case-sensitivity probe), so the accepted trade is a Linux-only false positive that is **not** a mere dedup: on a case-sensitive volume `~/.Pixel-Agents/hooks/claude-hook.js` is a genuinely DIFFERENT file, we classify it as ours, and uninstall **deletes** it (`claudeHookInstaller.test.ts` pins that removal). Nothing creates that path, and the trade is deliberate — the alternative is a guaranteed unremovable live hook on the two platforms this actually ships to. A symlink alias to our script is deliberately _not_ recognized — the cost is one duplicate entry, versus deleting a stranger's hook if we resolved paths.

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ pixel-agents --help
108108

109109
The default bind address is `127.0.0.1`. Binding to `0.0.0.0` exposes the UI and WebSocket to the local network; do this only on a trusted network.
110110

111-
Open the URL the CLI prints it carries a `?token=` for this session. Any browser can watch the office without it, but installing or removing hooks (which edits your agent tool's own settings file) is only offered to a session that has the token, so an untokened client on the network cannot approve it. Open the bare address instead and the hooks toggle in Settings is refused, and reports the actual install state rather than appearing to work.
111+
Open the URL the CLI prints - it carries a `?token=` for this session. Any browser can watch the office without it, but installing or removing hooks (which edits your agent tool's own settings file, like the `~/.claude/settings.json`) is only offered to a session that has the token, so an untokened client on the network cannot approve it. Open the bare address instead and the hooks toggle in Settings is refused, and reports the actual install state rather than appearing to work.
112112

113113
Treat that URL as a secret: the token is a bearer capability, not proof of being local. Whoever holds it can approve the hook install from anywhere the server is reachable — so don't paste the URL into a shared channel, and note that it also lands in your browser history and (unredacted) in the server's own request log.
114114

adapters/vscode/PixelAgentsViewProvider.ts

Lines changed: 45 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@ import {
3434
writeLayoutToFile,
3535
} from '../../server/src/layoutPersistence.js';
3636
import { PathSet } from '../../server/src/pathKey.js';
37-
import { CONSENT_INSTALL_MESSAGE } from '../../server/src/providers/hook/claude/consentCopy.js';
37+
import {
38+
CONSENT_DISCLOSURE,
39+
CONSENT_INSTALL_HEADLINE,
40+
} from '../../server/src/providers/hook/claude/consentCopy.js';
3841
import { claudeProvider, copyHookScript } from '../../server/src/providers/index.js';
3942
import { PixelAgentsServer } from '../../server/src/server.js';
4043
import {
@@ -330,34 +333,56 @@ export class PixelAgentsViewProvider implements vscode.WebviewViewProvider {
330333
* nothing they do not already have. (This is deliberately NOT the general
331334
* rule: consent for a fresh install is still asked for, in full, below.)
332335
*
333-
* A non-modal notification, carrying the FULL disclosure in its message. A
334-
* startup consent gate must not block the workbench, and it does not have to:
335-
* a notification with buttons renders permanently expanded (`canCollapse` is
336-
* `!hasActions`) with no line clamp, and truncates only at 1000 characters —
337-
* which the composed message stays under, pinned by consentCopy.test.ts. The
338-
* facts therefore live in the message itself; a "Details" affordance would
339-
* put the disclosure one click away, which is the gap this gate exists to
340-
* close.
336+
* A BLOCKING MODAL, not a notification: this is the one decision that must
337+
* not be missable. A notification can be ignored or auto-hidden to the bell
338+
* and the write it authorizes never questioned again; a modal cannot. The
339+
* headline is the first argument and the full disclosure is `detail` —
340+
* VS Code renders `detail` only for modal messages (MessageOptions), so the
341+
* facts reach the decision surface as the modal's body.
342+
*
343+
* THREE buttons, because "Not Now" is also the close affordance. VS Code
344+
* synthesizes its own Cancel button only when no item claims that role
345+
* (MainThreadMessageService._showModalMessage), and passing three bare
346+
* strings did exactly that: the synthesized Cancel resolved to `undefined`
347+
* and took the Not Now path — a fourth button that did, precisely, what the
348+
* third one did. Marking "Not Now" as the close affordance makes VS Code use
349+
* it AS the cancel button instead of inventing a duplicate.
341350
*
342-
* The Info toast auto-hides after ~10 s WITHOUT closing the notification: it
343-
* parks in the notification bell with its buttons intact and this promise
344-
* still pending. So an unanswered or dismissed prompt writes nothing and
345-
* asks again next startup; only the explicit "Don't Ask Again" persists
346-
* hooks-off. */
351+
* Dismissal still writes nothing, but it no longer arrives as `undefined`.
352+
* Escape resolves through the dialog's cancelId to the close-affordance
353+
* ITEM, so a dismissal now returns the "Not Now" object; `undefined` remains
354+
* reachable (a dialog torn down without an answer). Both are handled by the
355+
* same fail-closed shape below: only an exact match writes, so anything
356+
* else — either dismissal form included — falls through to writing NOTHING
357+
* and asking again next startup. Only "Don't Ask Again" persists hooks-off. */
347358
private async installHooksWithConsent(port: number, token: string): Promise<void> {
348359
if (!readConfig().hooksConsentGiven) {
349360
if (await claudeProvider.areHooksInstalled()) {
350361
// Already installed and already firing: grant and migrate silently.
351362
grantHooksConsent();
352363
} else {
364+
// One MessageItem per button, matched by REFERENCE below. The overloads
365+
// are homogeneous (`T extends string` | `T extends MessageItem`), so
366+
// marking one item forces all three — and identity matching keeps each
367+
// title a single literal, with no second copy to drift out of sync.
368+
const install = { title: 'Install Hooks' };
369+
// `satisfies` is the typo guard on the one property carrying the whole
370+
// point of this shape: inference would accept `isCloseAffordence` as
371+
// just another field and silently restore the duplicate button.
372+
const notNow = {
373+
title: 'Not Now',
374+
isCloseAffordance: true,
375+
} satisfies vscode.MessageItem;
376+
const dontAskAgain = { title: "Don't Ask Again" };
353377
const choice = await vscode.window.showInformationMessage(
354-
CONSENT_INSTALL_MESSAGE,
355-
'Install Hooks',
356-
'Not Now',
357-
"Don't Ask Again",
378+
CONSENT_INSTALL_HEADLINE,
379+
{ modal: true, detail: CONSENT_DISCLOSURE },
380+
install,
381+
notNow,
382+
dontAskAgain,
358383
);
359-
if (choice !== 'Install Hooks') {
360-
if (choice === "Don't Ask Again") {
384+
if (choice !== install) {
385+
if (choice === dontAskAgain) {
361386
this.adapter.setSetting(GLOBAL_KEY_HOOKS_ENABLED, false);
362387
this.runtime.hooksEnabled.current = false;
363388
}

e2e/README.md

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ This section is auto-generated. Do not edit between the markers; CI fails on dri
173173

174174
<!-- BEGIN:E2E-INVENTORY -->
175175

176-
84 tests total. Generated by `scripts/generate-e2e-inventory.mjs`. Re-run after adding or removing tests.
176+
86 tests total. Generated by `scripts/generate-e2e-inventory.mjs`. Re-run after adding or removing tests.
177177

178178
### `@area:spawn` (2 tests)
179179

@@ -205,17 +205,19 @@ This section is auto-generated. Do not edit between the markers; CI fails on dri
205205
- `e2e/claude/hooks-on/lifecycle.spec.ts:877` — rapid /clear then new tool within 500ms lands on the reassigned agent (Hooks ON / lifecycle)
206206
- `e2e/claude/hooks-on/lifecycle.spec.ts:935` — close via X prevents re-adoption of old JSONL during dismissal cooldown (Hooks ON / lifecycle)
207207

208-
### `@area:cross-cutting` (19 tests)
208+
### `@area:cross-cutting` (21 tests)
209209

210210
- `e2e/claude/hooks-off/lifecycle.spec.ts:735` — agentToolsClear fires at turn end via turn_duration JSONL record (Hooks OFF / lifecycle)
211211
- `e2e/claude/hooks-off/lifecycle.spec.ts:800` — heuristic permission timer is cancelled when an agent is closed via overlay (Hooks OFF / lifecycle)
212212
- `e2e/claude/hooks-off/lifecycle.spec.ts:879` — sub-agent permission bubble fires on stalled non-exempt sub-tool via heuristic timer (Hooks OFF / lifecycle)
213-
- `e2e/claude/hooks-on/consent.spec.ts:141` — fresh install: the prompt discloses scope and Install writes the hooks (Hooks consent gate)
214-
- `e2e/claude/hooks-on/consent.spec.ts:182` — Not Now writes nothing and leaves consent ungranted (Hooks consent gate)
215-
- `e2e/claude/hooks-on/consent.spec.ts:206` — Don't Ask Again writes nothing and persists hooks off (Hooks consent gate)
216-
- `e2e/claude/hooks-on/consent.spec.ts:240` — a pre-consent 14-event install migrates to 12 with no prompt (Hooks consent gate / pre-consent install)
217-
- `e2e/claude/hooks-on/consent.spec.ts:284` — Settings toggle removes the migrated hooks and keeps third-party entries (Hooks consent gate / pre-consent install)
218-
- `e2e/claude/hooks-on/consent.spec.ts:330` — a failed uninstall does not persist hooks-off (Hooks consent gate / toggle-off failure)
213+
- `e2e/claude/hooks-on/consent.spec.ts:150` — fresh install: the prompt discloses scope and Install writes the hooks (Hooks consent gate)
214+
- `e2e/claude/hooks-on/consent.spec.ts:218` — Not Now writes nothing and leaves consent ungranted (Hooks consent gate)
215+
- `e2e/claude/hooks-on/consent.spec.ts:242` — Don't Ask Again writes nothing and persists hooks off (Hooks consent gate)
216+
- `e2e/claude/hooks-on/consent.spec.ts:271` — dismissing the modal writes nothing, exactly like Not Now (Hooks consent gate)
217+
- `e2e/claude/hooks-on/consent.spec.ts:298` — the close x writes nothing, exactly like Not Now (Hooks consent gate)
218+
- `e2e/claude/hooks-on/consent.spec.ts:333` — a pre-consent 14-event install migrates to 12 with no prompt (Hooks consent gate / pre-consent install)
219+
- `e2e/claude/hooks-on/consent.spec.ts:374` — Settings toggle removes the migrated hooks and keeps third-party entries (Hooks consent gate / pre-consent install)
220+
- `e2e/claude/hooks-on/consent.spec.ts:420` — a failed uninstall does not persist hooks-off (Hooks consent gate / toggle-off failure)
219221
- `e2e/claude/hooks-on/lifecycle.spec.ts:1034` — done sound chime fires on agentStatus waiting (Hooks ON / lifecycle)
220222
- `e2e/claude/hooks-on/lifecycle.spec.ts:1127` — restored agents skip the matrix spawn animation (Hooks ON / lifecycle)
221223
- `e2e/claude/hooks-on/lifecycle.spec.ts:1211` — tool status text matches every PreToolUse tool name (Hooks ON / lifecycle)

e2e/helpers/launch.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,9 @@ export async function launchVSCode(
9393
const paDir = path.join(tmpHome, '.pixel-agents');
9494
fs.mkdirSync(paDir, { recursive: true });
9595
// hooksConsentGiven is part of the baseline: without it the first-run consent
96-
// prompt gates hook installation and every hooks-on spec would stall on a
97-
// native notification no test answers.
96+
// prompt gates hook installation and every hooks-on spec would stall behind a
97+
// blocking modal no test answers. The consent specs opt out via
98+
// opts.seedConfig — they are the only ones that want the prompt.
9899
const seedConfig = opts.seedConfig ?? {
99100
vscode: { alwaysShowLabels: true },
100101
standalone: { alwaysShowLabels: true },
@@ -256,6 +257,11 @@ export async function launchVSCode(
256257
'workbench.editor.empty.hint': 'hidden',
257258
'workbench.secondarySideBar.defaultVisibility': 'hidden',
258259
'chat.commandCenter.enabled': false,
260+
// Render modal dialogs (showInformationMessage({ modal: true })) in the DOM
261+
// instead of as a native OS dialog, so Playwright can read and click them.
262+
// Only the consent specs need it; harmless everywhere else, where the
263+
// seeded hooksConsentGiven means no modal is ever raised.
264+
'window.dialogStyle': 'custom',
259265
};
260266
if (process.platform === 'darwin') {
261267
userSettings['terminal.integrated.profiles.osx'] = {

0 commit comments

Comments
 (0)