From e64563fda8d56bd91e0b49e2a3b28411357f5cf1 Mon Sep 17 00:00:00 2001 From: pallyoung Date: Wed, 6 May 2026 23:43:49 +0800 Subject: [PATCH 1/6] docs: add windows child process hardening plan --- ...6-05-06-windows-child-process-hardening.md | 411 ++++++++++++++++++ 1 file changed, 411 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-06-windows-child-process-hardening.md diff --git a/docs/superpowers/plans/2026-05-06-windows-child-process-hardening.md b/docs/superpowers/plans/2026-05-06-windows-child-process-hardening.md new file mode 100644 index 000000000..1f855fbf4 --- /dev/null +++ b/docs/superpowers/plans/2026-05-06-windows-child-process-hardening.md @@ -0,0 +1,411 @@ +# Windows Child Process Hardening Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Eliminate avoidable Windows console popups from non-PTY child-process calls while keeping provider/session PTY startup behavior unchanged. + +**Architecture:** Do not add Windows-specific branching to provider session command construction; `claude`/`codex` should continue to be launched the same way across platforms through `node-pty`. Instead, harden the separate non-PTY paths that currently use `spawn()`/`execFile()` directly by always setting `windowsHide: true`, then add tests and a Windows CI lane so the distinction stays enforced. + +**Tech Stack:** TypeScript, Node.js `child_process`, node-pty, Vitest, GitHub Actions. + +--- + +## File Structure + +- Modify: `packages/server/src/supervisor/evaluator.ts` - add `windowsHide: true` to the headless evaluator `spawn()` path. +- Modify: `packages/server/src/provider-runtime/command-check.ts` - extend command lookup execution so `execFile()` can receive `windowsHide: true`. +- Modify: `packages/server/src/provider-runtime/install-manager.ts` - thread `windowsHide: true` through provider auto-install `execFile()` calls. +- Modify: `packages/server/src/workspace/runtime-check.ts` - thread `windowsHide: true` through git/node runtime checks. +- Modify: `packages/server/src/git/cli.ts` - add `windowsHide: true` to git subprocess execution. +- Modify: `packages/cli/src/browser.ts` - add `windowsHide: true` to the browser-launch `spawn()` path on Windows. +- Modify: `packages/server/src/__tests__/provider-runtime/command-check.test.ts` - assert Windows lookup uses `where` and passes `windowsHide: true`. +- Modify: `packages/server/src/__tests__/provider-runtime/install-manager.test.ts` - assert install steps pass `windowsHide: true` into the injected executor. +- Modify: `packages/server/src/__tests__/workspace/runtime-check.test.ts` - assert runtime checks pass `windowsHide: true` into the injected executor. +- Modify: `packages/server/src/__tests__/git/cli.test.ts` - assert git subprocesses use `windowsHide: true`. +- Create: `packages/server/src/supervisor/evaluator.windows.test.ts` - isolate the `spawn()` call and assert `windowsHide: true` is present. +- Create: `packages/cli/src/browser.test.ts` - isolate the browser-launch `spawn()` call and assert `windowsHide: true` is present. +- Modify: `.github/workflows/ci.yml` - add a `windows-latest` job for targeted tests/build verification. + +## Task 1: Lock The Scope With Failing Windows-Option Tests + +**Files:** +- Modify: `packages/server/src/__tests__/provider-runtime/command-check.test.ts` +- Modify: `packages/server/src/__tests__/provider-runtime/install-manager.test.ts` +- Modify: `packages/server/src/__tests__/workspace/runtime-check.test.ts` +- Modify: `packages/server/src/__tests__/git/cli.test.ts` +- Create: `packages/server/src/supervisor/evaluator.windows.test.ts` +- Create: `packages/cli/src/browser.test.ts` + +- [ ] **Step 1: Extend the command-check test to expect `windowsHide: true`** + +Update `packages/server/src/__tests__/provider-runtime/command-check.test.ts` so the injected executor records the options object: + +```ts +it("passes windowsHide when checking commands on Windows", async () => { + const execFile = vi.fn(async () => ({ stdout: "C:\\bin\\claude.cmd\n", stderr: "" })); + + await expect( + checkCommandAvailable("claude", { platform: "win32", execFile }) + ).resolves.toBe(true); + + expect(execFile).toHaveBeenCalledWith("where", ["claude"], { + windowsHide: true, + }); +}); +``` + +- [ ] **Step 2: Extend install-manager and runtime-check tests to expect `windowsHide: true`** + +Update the injected executor signatures in `packages/server/src/__tests__/provider-runtime/install-manager.test.ts` and `packages/server/src/__tests__/workspace/runtime-check.test.ts` so they assert the third argument: + +```ts +const execFile = vi.fn(async (_file: string, _args: string[], options?: { windowsHide?: boolean }) => { + expect(options).toEqual({ windowsHide: true }); + return { stdout: "", stderr: "" }; +}); +``` + +- [ ] **Step 3: Add a git executor test that proves `runGit()` hides Windows windows** + +Add a targeted mock-based test in `packages/server/src/__tests__/git/cli.test.ts`: + +```ts +it("passes windowsHide to execFile for git commands", async () => { + const execFileMock = vi.fn((_file, _args, options, callback) => { + callback(null, "ok", ""); + return { stdin: null } as never; + }); + + vi.doMock("child_process", () => ({ execFile: execFileMock })); + const { runGit } = await import("../../git/cli.js"); + + await expect(runGit("/repo", ["status"])).resolves.toEqual({ stdout: "ok", stderr: "" }); + expect(execFileMock).toHaveBeenCalledWith( + "git", + ["status"], + expect.objectContaining({ windowsHide: true }), + expect.any(Function) + ); +}); +``` + +- [ ] **Step 4: Add isolated spawn tests for the supervisor evaluator and browser opener** + +Create `packages/server/src/supervisor/evaluator.windows.test.ts` and `packages/cli/src/browser.test.ts` with hoisted `vi.mock("node:child_process")` setup: + +```ts +const { spawn } = vi.hoisted(() => ({ + spawn: vi.fn(() => ({ + stdout: { on: vi.fn() }, + stderr: { on: vi.fn() }, + on: vi.fn((event, cb) => { + if (event === "exit") cb(0); + }), + once: vi.fn((event, cb) => { + if (event === "spawn") cb(); + }), + unref: vi.fn(), + pid: 123, + })), +})); + +vi.mock("node:child_process", () => ({ spawn })); +``` + +Assert both call sites include `windowsHide: true`: + +```ts +expect(spawn).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ windowsHide: true }) +); +``` + +- [ ] **Step 5: Run the targeted tests to verify they fail before production changes** + +Run: + +```bash +pnpm --filter @coder-studio/server vitest run \ + src/__tests__/provider-runtime/command-check.test.ts \ + src/__tests__/provider-runtime/install-manager.test.ts \ + src/__tests__/workspace/runtime-check.test.ts \ + src/__tests__/git/cli.test.ts \ + src/supervisor/evaluator.windows.test.ts + +pnpm --filter @spencer-kit/coder-studio vitest run src/browser.test.ts +``` + +Expected: FAIL because the current implementation does not pass `windowsHide: true` and the injected executor signatures do not yet accept an options object. + +- [ ] **Step 6: Commit the failing-test scaffold** + +```bash +git add \ + packages/server/src/__tests__/provider-runtime/command-check.test.ts \ + packages/server/src/__tests__/provider-runtime/install-manager.test.ts \ + packages/server/src/__tests__/workspace/runtime-check.test.ts \ + packages/server/src/__tests__/git/cli.test.ts \ + packages/server/src/supervisor/evaluator.windows.test.ts \ + packages/cli/src/browser.test.ts +git commit -m "test: cover windows child process options" +``` + +## Task 2: Add `windowsHide: true` To Every Non-PTY Runtime Subprocess Path + +**Files:** +- Modify: `packages/server/src/supervisor/evaluator.ts` +- Modify: `packages/server/src/provider-runtime/command-check.ts` +- Modify: `packages/server/src/provider-runtime/install-manager.ts` +- Modify: `packages/server/src/workspace/runtime-check.ts` +- Modify: `packages/server/src/git/cli.ts` +- Modify: `packages/cli/src/browser.ts` + +- [ ] **Step 1: Update the shared `execFile` dependency signatures to accept options** + +Change the server-side dependency types from: + +```ts +execFile?: (file: string, args: string[]) => Promise<{ stdout: string; stderr: string }>; +``` + +to: + +```ts +execFile?: ( + file: string, + args: string[], + options?: { windowsHide?: boolean } +) => Promise<{ stdout: string; stderr: string }>; +``` + +Apply this in: + +- `packages/server/src/provider-runtime/command-check.ts` +- `packages/server/src/provider-runtime/install-manager.ts` +- `packages/server/src/workspace/runtime-check.ts` + +- [ ] **Step 2: Pass `windowsHide: true` through all server `execFile()` wrappers** + +Update each real executor to forward the option into Node: + +```ts +const execFile = deps.execFile ?? ((file: string, args: string[], options) => + execFileAsync(file, args, options) +); +``` + +and each invocation to use: + +```ts +await execFile(lookup, [command], { windowsHide: true }); +``` + +Apply the same pattern to provider install and runtime check calls. + +- [ ] **Step 3: Add `windowsHide: true` to direct `spawn()`/`execFile()` runtime call sites** + +Update: + +- `packages/server/src/supervisor/evaluator.ts` + +```ts +const child = spawn(command.argv[0]!, command.argv.slice(1), { + cwd: command.cwd, + detached: process.platform !== "win32", + env: { ...process.env, ...command.env }, + stdio: ["ignore", "pipe", "pipe"], + windowsHide: true, +}); +``` + +- `packages/server/src/git/cli.ts` + +```ts +const child = execFile( + "git", + gitArgs, + { + cwd, + env: { + ...process.env, + GIT_TERMINAL_PROMPT: "0", + LC_ALL: "C", + LANG: "C", + ...options.env, + }, + maxBuffer: 10 * 1024 * 1024, + timeout: options.timeoutMs, + windowsHide: true, + }, + callback +); +``` + +- `packages/cli/src/browser.ts` + +```ts +const child = spawn(command, args, { + detached: true, + stdio: "ignore", + windowsHide: true, +}); +``` + +- [ ] **Step 4: Re-run the targeted tests to verify they pass** + +Run: + +```bash +pnpm --filter @coder-studio/server vitest run \ + src/__tests__/provider-runtime/command-check.test.ts \ + src/__tests__/provider-runtime/install-manager.test.ts \ + src/__tests__/workspace/runtime-check.test.ts \ + src/__tests__/git/cli.test.ts \ + src/supervisor/evaluator.windows.test.ts + +pnpm --filter @spencer-kit/coder-studio vitest run src/browser.test.ts +``` + +Expected: PASS with every non-PTY child process now explicitly hiding its console window on Windows. + +- [ ] **Step 5: Commit the runtime hardening changes** + +```bash +git add \ + packages/server/src/supervisor/evaluator.ts \ + packages/server/src/provider-runtime/command-check.ts \ + packages/server/src/provider-runtime/install-manager.ts \ + packages/server/src/workspace/runtime-check.ts \ + packages/server/src/git/cli.ts \ + packages/cli/src/browser.ts +git commit -m "fix: hide windows console windows for background subprocesses" +``` + +## Task 3: Prove Provider/PTy Startup Semantics Stay Unchanged + +**Files:** +- Verify: `packages/providers/src/claude/definition.ts` +- Verify: `packages/providers/src/codex/definition.ts` +- Verify: `packages/server/src/session/manager.ts` +- Verify: `packages/server/src/terminal/pty-host.ts` + +- [ ] **Step 1: Re-read the provider/PTy launch chain and confirm no Windows-specific provider branching is introduced** + +Confirm these invariants remain true: + +```ts +// packages/providers/src/claude/definition.ts +argv: ["claude", ...modelArg, ...(cfg.additionalArgs ?? [])] + +// packages/providers/src/codex/definition.ts +argv: ["codex", ...cfg.additionalArgs] + +// packages/server/src/session/manager.ts +const terminalSpec: TerminalSpec = { + kind: "agent", + argv: cmd.argv, + cwd: cmd.cwd, + env: { ...cmd.env, CODER_STUDIO_SESSION_ID: sessionId }, +}; +``` + +- [ ] **Step 2: Run existing provider and session tests to keep that contract locked** + +Run: + +```bash +pnpm --filter @coder-studio/providers vitest run src/claude/definition.test.ts src/codex/definition.test.ts +pnpm --filter @coder-studio/server vitest run src/__tests__/session-integration.test.ts src/__tests__/session-commands.test.ts +``` + +Expected: PASS with no changes to `claude`/`codex` argv construction and no new Windows-only provider branching. + +- [ ] **Step 3: Commit only if a regression guard needed adjustment** + +If no source changes were needed, skip committing in this task. If any expectation text was tightened, use: + +```bash +git add packages/providers/src/claude/definition.test.ts packages/providers/src/codex/definition.test.ts packages/server/src/__tests__/session-integration.test.ts packages/server/src/__tests__/session-commands.test.ts +git commit -m "test: lock provider launch parity across platforms" +``` + +## Task 4: Add Windows CI Coverage And Document What It Can And Cannot Prove + +**Files:** +- Modify: `.github/workflows/ci.yml` + +- [ ] **Step 1: Add a Windows job that runs targeted runtime tests and builds** + +Update `.github/workflows/ci.yml` to keep the existing Ubuntu job and add a focused Windows lane: + +```yaml + windows-runtime: + name: Windows runtime verification + runs-on: windows-latest + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Setup pnpm + uses: pnpm/action-setup@v4 + with: + version: 10.33.2 + run_install: false + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: "24" + cache: "pnpm" + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Run targeted Windows tests + run: | + pnpm --filter @coder-studio/providers test -- --run src/claude/definition.test.ts src/codex/definition.test.ts + pnpm --filter @coder-studio/server test -- --run src/__tests__/provider-runtime/command-check.test.ts src/__tests__/provider-runtime/install-manager.test.ts src/__tests__/workspace/runtime-check.test.ts src/__tests__/git/cli.test.ts src/supervisor/evaluator.windows.test.ts src/__tests__/session-commands.test.ts src/__tests__/session-integration.test.ts + pnpm --filter @spencer-kit/coder-studio test -- --run src/browser.test.ts src/bin.test.ts src/pm2-control.test.ts src/server-control.test.ts + + - name: Build server and cli packages + run: | + pnpm --filter @coder-studio/server build + pnpm --filter @spencer-kit/coder-studio build +``` + +- [ ] **Step 2: Run local workflow validation for YAML syntax** + +Run: + +```bash +pnpm exec biome check .github/workflows/ci.yml +``` + +Expected: PASS with no YAML/formatting issues. + +- [ ] **Step 3: State the verification boundary explicitly in the PR/final summary** + +Use this exact language in the close-out: + +```md +Windows CI now proves that our non-PTY subprocess paths pass `windowsHide: true` and that provider/session PTY startup still works with the existing cross-platform argv contract. It does not visually prove the absence of a transient desktop console flash from node-pty itself; that still requires a manual smoke check on a real Windows desktop. +``` + +- [ ] **Step 4: Commit the CI coverage update** + +```bash +git add .github/workflows/ci.yml +git commit -m "ci: add windows runtime verification" +``` + +## Self-Review + +- [ ] Provider session startup is explicitly kept cross-platform and unchanged; the plan only hardens non-PTY subprocesses. +- [ ] `execFile` is retained for one-shot commands; the plan does not switch to `exec`, because `exec` would introduce an unnecessary shell layer. +- [ ] `spawn` call sites are only used where streaming/detached behavior is already needed, and the plan adds `windowsHide: true` there as well. +- [ ] The plan distinguishes what code inspection and CI can prove from what still requires manual Windows desktop validation. + From f03f30808357849af9c3a1ea6a149e05aa94a62e Mon Sep 17 00:00:00 2001 From: pallyoung Date: Wed, 6 May 2026 23:44:26 +0800 Subject: [PATCH 2/6] docs: add ui component library bootstrap plan --- ...nt-library-phase-a-bootstrap-and-button.md | 1067 +++++++++++++++++ 1 file changed, 1067 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-06-ui-component-library-phase-a-bootstrap-and-button.md diff --git a/docs/superpowers/plans/2026-05-06-ui-component-library-phase-a-bootstrap-and-button.md b/docs/superpowers/plans/2026-05-06-ui-component-library-phase-a-bootstrap-and-button.md new file mode 100644 index 000000000..7b61f85a5 --- /dev/null +++ b/docs/superpowers/plans/2026-05-06-ui-component-library-phase-a-bootstrap-and-button.md @@ -0,0 +1,1067 @@ +# UI Component Library Phase A Bootstrap + Button Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Bootstrap `packages/web/src/components/ui/` as the new home for shared UI primitives, centralize `useViewport()` into a single source of truth, and land `Button` as the first production component with one real caller migrated. + +**Architecture:** Keep this first plan intentionally narrow. Do not attempt Tier 0 in bulk. Create the UI library skeleton (`README.md`, `MIGRATION.md`), move viewport logic into `components/ui/_internal/use-viewport.ts` with a compatibility re-export from `src/hooks/use-viewport.ts`, then add `Button` with CSS Modules and `clsx`. Migrate exactly one simple caller (`features/auth/index.tsx`) so the pattern is proven before the rest of Tier 0. + +**Tech Stack:** React 19, TypeScript 6, Vite, Vitest + Testing Library, Jotai, vanilla CSS Modules, existing `tokens.css`, new `clsx` dependency. + +**Spec reference:** `docs/superpowers/specs/2026-05-06-ui-component-library-design.md` §2, §3, §4, §5, §6, §7, §8. + +**Out of scope for this plan (deferred):** +- `IconButton`, `Input`, `Textarea`, `Tag`, `Badge`, `Pill`, `StatusDot`, `Kbd`, `Spinner`, `Switch` +- All Tier 1 and Tier 2 components +- `portal.tsx`, `focus-trap.ts`, `dismiss.ts`, `render-with-viewport.tsx` +- Deleting the legacy `.btn` block from `styles/components.css` +- Migrating any Button caller other than `features/auth/index.tsx` +- Adding `@floating-ui/react` + +--- + +## File Structure + +**New files:** +- `packages/web/src/components/ui/README.md` — project-level rules for the new UI library +- `packages/web/src/components/ui/MIGRATION.md` — 24-row migration inventory; source of truth for caller counts and deletion timing +- `packages/web/src/components/ui/_internal/use-viewport.ts` — single source of truth for `(max-width: 899px), (pointer: coarse)` viewport detection +- `packages/web/src/components/ui/_internal/use-viewport.test.tsx` — tests the combined media query and live updates +- `packages/web/src/components/ui/button/index.tsx` — first shared primitive; variant/size/loading API +- `packages/web/src/components/ui/button/index.module.css` — Button styles copied from the legacy `.btn` block, plus local hashed classes and temporary `:global()` aliases +- `packages/web/src/components/ui/button/index.test.tsx` — Button unit tests +- `packages/web/src/components/ui/button/README.md` — Button usage contract +- `packages/web/src/components/ui/index.ts` — public barrel export for `Button` + +**Modified files:** +- `packages/web/package.json` — add `clsx` +- `pnpm-lock.yaml` — lockfile update for `clsx` +- `packages/web/src/hooks/use-viewport.ts` — compatibility re-export from the new `_internal` hook +- `packages/web/src/app.test.tsx` — coarse-pointer expectation flips from desktop to mobile +- `packages/web/src/features/auth/index.tsx` — replace raw submit `); + + const button = screen.getByRole("button", { name: "Save" }); + expect(button).toHaveAttribute("type", "button"); + expect(button).toBeEnabled(); + }); + + it("supports the four visual variants", () => { + render( + <> + + + + + + ); + + expect(screen.getByRole("button", { name: "Primary" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Secondary" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Ghost" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Danger" })).toBeInTheDocument(); + }); + + it("supports the three size options", () => { + render( + <> + + + + + ); + + expect(screen.getByRole("button", { name: "Small" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Medium" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Large" })).toBeInTheDocument(); + }); + + it("disables click handlers while loading", async () => { + const user = userEvent.setup(); + const onClick = vi.fn(); + + render( + + ); + + const button = screen.getByRole("button", { name: "Create" }); + expect(button).toBeDisabled(); + expect(button).toHaveAttribute("aria-busy", "true"); + + await user.click(button); + expect(onClick).not.toHaveBeenCalled(); + }); + + it("renders leading and trailing icons", () => { + render( + + ); + + expect(screen.getByTestId("leading-icon")).toBeInTheDocument(); + expect(screen.getByTestId("trailing-icon")).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Open" })).toBeInTheDocument(); + }); + + it("renders as an anchor when as='a'", () => { + render( + + ); + + expect(screen.getByRole("link", { name: "Settings" })).toHaveAttribute( + "href", + "/settings" + ); + }); +}); +``` + +- [ ] **Step 2: Run the Button tests and confirm they fail** + +```bash +pnpm --filter @coder-studio/web exec vitest run src/components/ui/button/index.test.tsx +``` + +Expected: FAIL with `Failed to resolve import "."` from `src/components/ui/button/index.test.tsx`, because `index.tsx` does not exist yet. + +- [ ] **Step 3: Add `clsx`** + +Run: +```bash +pnpm --filter @coder-studio/web add clsx +``` + +Expected: `packages/web/package.json` gains `"clsx"`, and `pnpm-lock.yaml` updates. + +The dependency section in `packages/web/package.json` should now include this exact line: + +```json +"clsx": "^2.1.1" +``` + +- [ ] **Step 4: Implement `Button`, its CSS Module, README, and the public barrel export** + +Create `packages/web/src/components/ui/button/index.tsx`: + +```tsx +import clsx from "clsx"; +import type { AnchorHTMLAttributes, ButtonHTMLAttributes, ReactNode } from "react"; +import styles from "./index.module.css"; + +export type ButtonVariant = "primary" | "secondary" | "ghost" | "danger"; +export type ButtonSize = "sm" | "md" | "lg"; + +interface ButtonBaseProps { + readonly children: ReactNode; + readonly className?: string; + readonly variant?: ButtonVariant; + readonly size?: ButtonSize; + readonly loading?: boolean; + readonly leadingIcon?: ReactNode; + readonly trailingIcon?: ReactNode; +} + +type ButtonElementProps = ButtonBaseProps & + Omit, keyof ButtonBaseProps> & { + readonly as?: "button"; + }; + +type AnchorElementProps = ButtonBaseProps & + Omit, keyof ButtonBaseProps> & { + readonly as: "a"; + }; + +export type ButtonProps = ButtonElementProps | AnchorElementProps; + +const variantClassMap: Record = { + primary: styles.primary, + secondary: styles.secondary, + ghost: styles.ghost, + danger: styles.danger, +}; + +const sizeClassMap: Record = { + sm: styles.sm, + md: undefined, + lg: styles.lg, +}; + +const ButtonContent = ({ + children, + leadingIcon, + loading, + trailingIcon, +}: Pick) => { + return ( + <> + {loading ?