From 9b49217490bf4cfbfc72aa4066cc595e6a8518cf Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:26:13 -0400 Subject: [PATCH 1/9] fix(windows): stop a benign cleanup kill from failing a passing smoke `Stop-LaunchedApp` ran `taskkill.exe` in the smoke script's `finally` block without inspecting or resetting `$LASTEXITCODE`. When the launched app (or a child in its tree) had already exited, taskkill printed "There is no running instance of the task" and returned nonzero. `Write-Output` is a cmdlet and does not reset `$LASTEXITCODE`, and GitHub's pwsh step wrapper ends with `exit $LASTEXITCODE` - so the step failed with code 1 immediately after the script printed that the smoke had passed, blocking the v1.2.62 Windows release. Every `taskkill.exe` call in the smoke script now goes through `Invoke-TaskKill`, which returns the exit code to the caller and always leaves `$LASTEXITCODE` at 0. `Stop-LaunchedApp` discards it (cleanup: "already gone" is success); `Stop-InstalledProductProcesses` keeps its load-bearing checks and still throws when it cannot stop a channel-owned supervisor or product process before repair. Audited the other Windows scripts for the same leak on the success path: - windows-uninstall-cleanup.ps1 had it - its lone native command is a best-effort supervisor kill and the script has no trailing `exit`, so a supervisor that had already stopped made the cleanup exit nonzero. Reset there. - windows-firewall-rules.ps1 ends every path with an explicit `exit 0`/`exit 1`. - windows-install-setup.ps1 checks `$LASTEXITCODE` after every native call and its success path ends on a call asserted to be 0. Co-Authored-By: Claude Opus 5 --- .../windows-installed-product-smoke.ps1 | 25 +++++++++++++++---- .../scripts/windows-uninstall-cleanup.ps1 | 6 +++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/apps/desktop/scripts/windows-installed-product-smoke.ps1 b/apps/desktop/scripts/windows-installed-product-smoke.ps1 index 0267a589b..83241e76f 100644 --- a/apps/desktop/scripts/windows-installed-product-smoke.ps1 +++ b/apps/desktop/scripts/windows-installed-product-smoke.ps1 @@ -48,9 +48,26 @@ function Invoke-Uninstaller([bool]$BestEffort = $false) { } } +# Native executables set `$LASTEXITCODE`, and a `pwsh` GitHub Actions step ends +# with `exit $LASTEXITCODE` - so whichever native command this script happened to +# run last decides the step result, no matter what the script itself concluded. +# `taskkill.exe` is the only native command here, and it exits nonzero for the +# benign "there is no running instance of the task" case, which is exactly the +# state a cleanup kill wants. Every call therefore goes through this helper: it +# hands the exit code to the caller that cares and always leaves `$LASTEXITCODE` +# at 0, so a passing smoke cannot be failed by its own teardown. +function Invoke-TaskKill([string]$TargetProcessId) { + & taskkill.exe /PID $TargetProcessId /T /F | Out-Null + $exitCode = $LASTEXITCODE + $global:LASTEXITCODE = 0 + return $exitCode +} + function Stop-LaunchedApp { if ($launchedApp -and -not $launchedApp.HasExited) { - & taskkill.exe /PID $launchedApp.Id /T /F | Out-Null + # Cleanup only: the intent is "it is not running", so a kill that fails + # because the process (or a child in its tree) already exited is success. + [void](Invoke-TaskKill ([string]$launchedApp.Id)) } $script:launchedApp = $null } @@ -65,8 +82,7 @@ function Stop-InstalledProductProcesses { ([string]$_.CommandLine).IndexOf($launcherPrefix, [StringComparison]::OrdinalIgnoreCase) -ge 0 }) foreach ($supervisor in $supervisors) { - & taskkill.exe /PID ([string]$supervisor.ProcessId) /T /F | Out-Null - if ($LASTEXITCODE -ne 0) { + if ((Invoke-TaskKill ([string]$supervisor.ProcessId)) -ne 0) { throw "Could not stop channel-owned ADE supervisor $($supervisor.ProcessId) before repair." } } @@ -81,8 +97,7 @@ function Stop-InstalledProductProcesses { } catch { $false } }) foreach ($process in $processes) { - & taskkill.exe /PID ([string]$process.ProcessId) /T /F | Out-Null - if ($LASTEXITCODE -ne 0) { + if ((Invoke-TaskKill ([string]$process.ProcessId)) -ne 0) { $remaining = Get-CimInstance Win32_Process -Filter "ProcessId = $($process.ProcessId)" -ErrorAction SilentlyContinue if ($remaining) { throw "Could not stop channel-owned ADE process $($process.ProcessId) before repair." diff --git a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 index 9920afa6b..d6536010c 100644 --- a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 +++ b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 @@ -205,7 +205,13 @@ function Remove-ChannelStartupWithoutPackagedCli( if ($supervisorPid -gt 0) { $process = Get-CimInstance Win32_Process -Filter "ProcessId = $supervisorPid" -ErrorAction SilentlyContinue if ($process -and ([string]$process.CommandLine).IndexOf($launcherPath, [StringComparison]::OrdinalIgnoreCase) -ge 0) { + # Best effort: the supervisor may already be gone, and taskkill exits + # nonzero when it is. This is the only native command in the script and + # nothing after it resets `$LASTEXITCODE`, so leaving it set would make + # the uninstall cleanup report failure for a process that is already + # stopped - exactly the state it wanted. & taskkill.exe /PID $supervisorPid /T /F | Out-Null + $global:LASTEXITCODE = 0 } } } catch { From 7f4fe829536514c3bab498a9f0820e147049df00 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:29:34 -0400 Subject: [PATCH 2/9] fix(desktop): install the ADE CLI once at startup, guarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `ade` command was reaching users unreliably: a real user's every `ade` diagnostic answered "no such file or directory" while his app worked fine. Installing the app is the opt-in, exactly as `curl … install.sh | sh` is, but the DMG has no install-time hook and this app has no onboarding flow, so app startup is the only opportunity. `installAdeCliForTerminalInBackground` already existed but was unguarded: it called `installForUser()` on every launch, on the startup critical path, with no check for an existing install and no memory of having run. That re-ran the packaged installer each launch and could clobber an `ade` owned by Homebrew or `install.sh`. The guards now live in `runAdeCliAutoInstall`: - Skips entirely when `ade` already resolves on the user's real shell PATH from ANY source (`status.terminalInstalled` is computed from the host PATH snapshot taken before ADE augments it). We never shadow an install we do not own. - Once ever, not once per launch: an `adeCliAutoInstall` marker in `ade-state.json` (the existing main-process global state store) records the outcome. Deleting the binary or stripping the PATH line afterwards is a deliberate act and is not silently undone. - A build that cannot install (no packaged installer) and a failed install leave no marker, so an app update self-heals instead of stranding the user. - Never throws; failure is a single `ade_cli.auto_install_failed` warn. Runs in `setImmediate(...).unref()`, off the path to the first window, matching the deferral pattern used elsewhere in main.ts and in adeCliService itself. - A process-wide latch keeps the project-open and dormant startup paths from both attempting it. Surface: none added. The Settings card already reports Terminal readiness, the resolved command path, and the install target, and stays the way to repair or reinstall. A startup toast for something the user did not ask for would be a nag, and the honest state is already one click away. Co-Authored-By: Claude Opus 5 --- apps/desktop/src/main/main.ts | 38 ++-- .../services/cli/adeCliAutoInstall.test.ts | 166 ++++++++++++++++++ .../main/services/cli/adeCliAutoInstall.ts | 111 ++++++++++++ .../src/main/services/state/globalState.ts | 15 ++ 4 files changed, 317 insertions(+), 13 deletions(-) create mode 100644 apps/desktop/src/main/services/cli/adeCliAutoInstall.test.ts create mode 100644 apps/desktop/src/main/services/cli/adeCliAutoInstall.ts diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 5d73ba8c8..78d233a4d 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -238,6 +238,7 @@ import { import { createKeybindingsService } from "./services/keybindings/keybindingsService"; import { createAgentToolsService } from "./services/agentTools/agentToolsService"; import { createAdeCliService } from "./services/cli/adeCliService"; +import { runAdeCliAutoInstall } from "./services/cli/adeCliAutoInstall"; import { createDevToolsService } from "./services/devTools/devToolsService"; import { createOnboardingService } from "./services/onboarding/onboardingService"; import { createAutomationService } from "./services/automations/automationService"; @@ -460,25 +461,36 @@ function fixElectronShellPath(): void { // Must run before any service or child process is created. fixElectronShellPath(); +let adeCliAutoInstallScheduled = false; + +/** + * Makes `ade` exist for a user who never opens Settings. Every guard that + * matters — already on PATH, already settled once, silent failure — lives in + * `runAdeCliAutoInstall`; this only schedules it. Both project-open and dormant + * startup reach here, so the process-wide latch keeps it to a single attempt. + */ function installAdeCliForTerminalInBackground( adeCliService: ReturnType, logger: Logger, + globalStatePath: string, ): void { - if (process.env.ADE_DISABLE_CLI_AUTO_INSTALL === "1") return; - void adeCliService.installForUser() - .then((result) => { - logger.info("ade_cli.auto_install", { - ok: result.ok, - command: result.status.command, - installTargetPath: result.status.installTargetPath, - message: result.message, - }); - }) - .catch((error) => { + if (adeCliAutoInstallScheduled) return; + adeCliAutoInstallScheduled = true; + // A convenience, not startup work: it can spawn the packaged installer and + // append to a shell profile, so it stays off the path to the first window. + const task = setImmediate(() => { + void runAdeCliAutoInstall({ + adeCli: adeCliService, + logger, + readState: () => readGlobalState(globalStatePath), + writeState: (state) => writeGlobalState(globalStatePath, state), + }).catch((error) => { logger.warn("ade_cli.auto_install_failed", { error: error instanceof Error ? error.message : String(error), }); }); + }); + task.unref?.(); } const disableHardwareAcceleration = @@ -2841,7 +2853,7 @@ app.whenReady().then(async () => { logger, }); adeCliService.applyToProcessEnv(); - installAdeCliForTerminalInBackground(adeCliService, logger); + installAdeCliForTerminalInBackground(adeCliService, logger, globalStatePath); const devToolsService = createDevToolsService({ logger }); const project = toProjectInfo(projectRoot, baseRef); @@ -5193,7 +5205,7 @@ app.whenReady().then(async () => { logger, }); adeCliService.applyToProcessEnv(); - installAdeCliForTerminalInBackground(adeCliService, logger); + installAdeCliForTerminalInBackground(adeCliService, logger, globalStatePath); const externalOnlyLaneService: FileServiceLaneAdapter = { getFilesWorkspaces: () => [], resolveWorkspaceById: (workspaceId: string) => { diff --git a/apps/desktop/src/main/services/cli/adeCliAutoInstall.test.ts b/apps/desktop/src/main/services/cli/adeCliAutoInstall.test.ts new file mode 100644 index 000000000..4f8096a52 --- /dev/null +++ b/apps/desktop/src/main/services/cli/adeCliAutoInstall.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it, vi } from "vitest"; +import { runAdeCliAutoInstall } from "./adeCliAutoInstall"; +import type { GlobalState } from "../state/globalState"; +import type { AdeCliInstallResult, AdeCliStatus } from "../../../shared/types"; + +function logger() { + return { debug: vi.fn(), error: vi.fn(), info: vi.fn(), warn: vi.fn() }; +} + +function status(overrides: Partial = {}): AdeCliStatus { + return { + command: "ade", + platform: "darwin", + isPackaged: true, + bundledAvailable: true, + bundledBinDir: "/Applications/ADE.app/Contents/Resources/ade-cli/bin", + bundledCommandPath: "/Applications/ADE.app/Contents/Resources/ade-cli/bin/ade", + installerPath: "/Applications/ADE.app/Contents/Resources/ade-cli/install.sh", + agentPathReady: true, + terminalInstalled: false, + terminalCommandPath: null, + installAvailable: true, + installTargetPath: "/Users/someone/.local/bin/ade", + installTargetDirOnPath: false, + message: "", + nextAction: null, + ...overrides, + }; +} + +function harness(args: { + state?: GlobalState; + getStatus?: () => Promise; + installForUser?: () => Promise; + env?: NodeJS.ProcessEnv; +}) { + let state: GlobalState = args.state ?? {}; + const installForUser = vi.fn( + args.installForUser + ?? (async () => ({ ok: true, message: "Installed ade.", status: status({ terminalInstalled: true }) })), + ); + const getStatus = vi.fn(args.getStatus ?? (async () => status())); + const log = logger(); + return { + installForUser, + getStatus, + log, + readState: () => state, + run: () => + runAdeCliAutoInstall({ + adeCli: { getStatus, installForUser }, + logger: log, + readState: () => state, + writeState: (next) => { + state = next; + }, + env: args.env ?? {}, + now: () => new Date("2026-08-19T00:00:00.000Z"), + }), + }; +} + +describe("runAdeCliAutoInstall", () => { + it("does nothing when ade already resolves on the user's PATH", async () => { + // Homebrew, install.sh's ~/.ade/bin/ade, or a hand-made symlink: ADE must + // never clobber or shadow an install it does not own. + const h = harness({ + getStatus: async () => + status({ terminalInstalled: true, terminalCommandPath: "/opt/homebrew/bin/ade" }), + }); + + await expect(h.run()).resolves.toBe("already-available"); + expect(h.installForUser).not.toHaveBeenCalled(); + expect(h.readState().adeCliAutoInstall).toEqual({ + completedAt: "2026-08-19T00:00:00.000Z", + outcome: "already-available", + command: "ade", + }); + }); + + it("does not run again once a previous launch settled the machine", async () => { + // The user deleting the binary or stripping the PATH line is deliberate. + const h = harness({ + state: { + adeCliAutoInstall: { + completedAt: "2026-08-01T00:00:00.000Z", + outcome: "installed", + command: "ade", + }, + }, + }); + + await expect(h.run()).resolves.toBe("already-settled"); + expect(h.getStatus).not.toHaveBeenCalled(); + expect(h.installForUser).not.toHaveBeenCalled(); + }); + + it("installs once and records the marker when ade is missing", async () => { + const h = harness({}); + + await expect(h.run()).resolves.toBe("installed"); + expect(h.installForUser).toHaveBeenCalledTimes(1); + expect(h.readState().adeCliAutoInstall).toEqual({ + completedAt: "2026-08-19T00:00:00.000Z", + outcome: "installed", + command: "ade", + }); + + // Second launch: the marker alone stops it. + await expect(h.run()).resolves.toBe("already-settled"); + expect(h.installForUser).toHaveBeenCalledTimes(1); + }); + + it("preserves unrelated global state when it records the marker", async () => { + const h = harness({ state: { lastProjectRoot: "/repo" } }); + + await h.run(); + + expect(h.readState().lastProjectRoot).toBe("/repo"); + }); + + it("reports a failed install without marking the machine settled", async () => { + // No marker: a build that shipped a broken installer must self-heal on the + // next update rather than leave the user without the command forever. + const h = harness({ + installForUser: async () => ({ + ok: false, + message: "The ADE CLI installer is missing from this app build.", + status: status(), + }), + }); + + await expect(h.run()).resolves.toBe("failed"); + expect(h.readState().adeCliAutoInstall).toBeUndefined(); + expect(h.log.warn).toHaveBeenCalledWith("ade_cli.auto_install_failed", expect.anything()); + }); + + it("never throws when the installer rejects, so startup is unaffected", async () => { + const h = harness({ + installForUser: async () => { + throw new Error("spawn EACCES"); + }, + }); + + await expect(h.run()).resolves.toBe("failed"); + expect(h.readState().adeCliAutoInstall).toBeUndefined(); + expect(h.log.warn).toHaveBeenCalledWith("ade_cli.auto_install_failed", { + error: "spawn EACCES", + }); + }); + + it("skips builds that cannot install without marking them settled", async () => { + const h = harness({ getStatus: async () => status({ installAvailable: false }) }); + + await expect(h.run()).resolves.toBe("unavailable"); + expect(h.installForUser).not.toHaveBeenCalled(); + expect(h.readState().adeCliAutoInstall).toBeUndefined(); + }); + + it("honours ADE_DISABLE_CLI_AUTO_INSTALL", async () => { + const h = harness({ env: { ADE_DISABLE_CLI_AUTO_INSTALL: "1" } }); + + await expect(h.run()).resolves.toBe("disabled"); + expect(h.getStatus).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/desktop/src/main/services/cli/adeCliAutoInstall.ts b/apps/desktop/src/main/services/cli/adeCliAutoInstall.ts new file mode 100644 index 000000000..40c9c9ebc --- /dev/null +++ b/apps/desktop/src/main/services/cli/adeCliAutoInstall.ts @@ -0,0 +1,111 @@ +import type { Logger } from "../logging/logger"; +import type { AdeCliAutoInstall, GlobalState } from "../state/globalState"; +import type { createAdeCliService } from "./adeCliService"; + +/** + * Installing the app is the opt-in for the `ade` command, exactly as running + * `curl -fsSL https://ade-app.dev/install.sh | sh` is. The DMG has no + * install-time hook (drag-and-drop copies a bundle; no code runs) and this app + * has no onboarding flow, so app startup is the only chance to make the command + * exist for a user who never opens Settings and never learns it is there. + * + * It writes to the user's shell profile, so it is deliberately timid: + * + * - It never runs when `ade` already resolves on the user's PATH. Homebrew, + * `install.sh`'s `~/.ade/bin/ade`, and a hand-made symlink all count; ADE must + * not shadow or clobber an install it does not own. + * - It runs once ever, not once per launch, via a marker in the global state + * file. Deleting the binary or stripping the PATH line afterwards is a + * deliberate act and must not be silently undone on the next launch. + * - It only marks a machine settled once it knows the outcome. A build that + * shipped without the installer leaves no marker, so an update self-heals. + * + * Failure is silent and non-fatal: this is a convenience and must never block + * or error app startup. The Settings card remains the surface a user reads and + * the way to repair or reinstall. + */ + +export type AdeCliAutoInstallOutcome = + /** Turned off for this process (used by tests and packaging smokes). */ + | "disabled" + /** A previous launch already settled this machine. */ + | "already-settled" + /** Some other install already owns `ade` on the user's PATH. */ + | "already-available" + /** This build cannot install it (no packaged installer / no local build). */ + | "unavailable" + | "installed" + | "failed"; + +type AdeCliAutoInstallArgs = { + adeCli: Pick, "getStatus" | "installForUser">; + logger: Logger; + readState: () => GlobalState; + writeState: (state: GlobalState) => void; + env?: NodeJS.ProcessEnv; + now?: () => Date; +}; + +export async function runAdeCliAutoInstall( + args: AdeCliAutoInstallArgs, +): Promise { + try { + return await attemptAdeCliAutoInstall(args); + } catch (error) { + // Nothing here is allowed to reach startup. `installForUser` already + // resolves its own failures, so a throw means something unexpected — say so + // once, at warn, and leave the machine unsettled so an update can retry. + args.logger.warn("ade_cli.auto_install_failed", { + error: error instanceof Error ? error.message : String(error), + }); + return "failed"; + } +} + +async function attemptAdeCliAutoInstall( + args: AdeCliAutoInstallArgs, +): Promise { + const env = args.env ?? process.env; + if (env.ADE_DISABLE_CLI_AUTO_INSTALL === "1") return "disabled"; + + const state = args.readState(); + if (state.adeCliAutoInstall) return "already-settled"; + + const settle = (marker: AdeCliAutoInstall): void => { + // Re-read: the state file is shared with the rest of main, and this task is + // deliberately deferred, so anything could have written it meanwhile. + args.writeState({ ...args.readState(), adeCliAutoInstall: marker }); + }; + const completedAt = (args.now ?? (() => new Date()))().toISOString(); + + const status = await args.adeCli.getStatus(); + if (status.terminalInstalled) { + settle({ completedAt, outcome: "already-available", command: status.command }); + return "already-available"; + } + if (!status.installAvailable) { + args.logger.debug("ade_cli.auto_install_skipped", { + command: status.command, + reason: "installer_unavailable", + }); + return "unavailable"; + } + + const result = await args.adeCli.installForUser(); + if (!result.ok) { + args.logger.warn("ade_cli.auto_install_failed", { + command: result.status.command, + installTargetPath: result.status.installTargetPath, + message: result.message, + }); + return "failed"; + } + settle({ completedAt, outcome: "installed", command: result.status.command }); + args.logger.info("ade_cli.auto_install", { + ok: true, + command: result.status.command, + installTargetPath: result.status.installTargetPath, + message: result.message, + }); + return "installed"; +} diff --git a/apps/desktop/src/main/services/state/globalState.ts b/apps/desktop/src/main/services/state/globalState.ts index 4db88e026..771fcea1c 100644 --- a/apps/desktop/src/main/services/state/globalState.ts +++ b/apps/desktop/src/main/services/state/globalState.ts @@ -57,6 +57,19 @@ export type FailedInstallAttempts = { lastFailedAt: string; }; +/** + * Records that startup has already settled Terminal access to the `ade` + * command on this machine, so it is never attempted a second time. Removing the + * binary or the shell PATH line afterwards is a deliberate act, and this marker + * is what stops the next launch from silently undoing it. + */ +export type AdeCliAutoInstall = { + completedAt: string; + /** `installed` = ADE placed it; `already-available` = another install owns it. */ + outcome: "installed" | "already-available"; + command: string; +}; + export type GlobalState = { lastProjectRoot?: string; lastRemoteProjectBinding?: Extract & { @@ -70,6 +83,8 @@ export type GlobalState = { /** Whether ADE may hold this machine awake while agents run. Default: never. */ keepAwakePreferences?: KeepAwakePreferences; welcomeVideo?: AppWelcomeVideoState; + /** Set once Terminal access to `ade` has been settled; see `AdeCliAutoInstall`. */ + adeCliAutoInstall?: AdeCliAutoInstall; }; export function readGlobalState(filePath: string): GlobalState { From aca79497cd0520a0c5ed508e88db7ec3997e939f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:31:18 -0400 Subject: [PATCH 3/9] fix(account-directory): preflight all 8 required deploy values, not 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guarded preflight for `deploy:production` asserted only DIRECTORY_AUTH_SECRET and PUSH_RELAY_URL. A production deploy missing a Clerk secret passed it, `/health` returned green, and every authenticated route answered 503 — precisely the 2026-08-06 incident shape the preflight exists to prevent, half-prevented. Verified the true required set against the Worker rather than assuming it: Hard requirements (no code default, fail closed): - secrets: DIRECTORY_AUTH_SECRET, CLERK_JWKS_URL, CLERK_ISSUER, CLERK_OAUTH_CLIENT_ID. `resolveCallerToken` (src/callerToken.ts:152-159) throws "authentication unavailable" when any of the trio is blank, mapped to 503 in directory.ts and diagnostics.ts, and the whole /device/* OAuth flow fails the same way. None of the three is declared in wrangler.jsonc, so all three must be secret bindings. - vars: PUSH_RELAY_URL, WEB_CLIENT_ORIGIN. WEB_CLIENT_ORIGIN has no default: `trustedWebClientOrigin` returns null and no access-control-allow-origin is emitted, so the browser client at app.ade-app.dev is blocked outright. Warn, do not block: - ONLINE_WINDOW_MS and DIAGNOSTICS_DAILY_GLOBAL_LIMIT both have code defaults (DEFAULT_ONLINE_WINDOW_MS = 90_000, DEFAULT_DIAGNOSTICS_DAILY_GLOBAL_LIMIT = 400) equal to the committed values, so their absence changes no behavior — the diagnostics cost ceiling still applies at 400/day. Failing a deploy on them would be a false gate. They also warn when set to something Number() cannot parse, because the Worker silently falls back to the default while the config reads as configured. Secrets are still checked by NAME ONLY via `wrangler secret list`; no value is ever read or printed. Production checks still target `--env production` because wrangler environments inherit neither vars nor secrets. Tests: 10 new cases (each Clerk secret individually, all-at-once message, WEB_CLIENT_ORIGIN, defaulted-var warnings, unparseable defaulted var, committed wrangler.jsonc warning-free). Suite: 169 passed. Co-Authored-By: Claude Opus 5 --- .../scripts/verify-deployment-config.d.mts | 3 +- .../scripts/verify-deployment-config.mjs | 81 ++++++++++++++--- .../test/verifyDeploymentConfig.test.ts | 86 ++++++++++++++++--- 3 files changed, 143 insertions(+), 27 deletions(-) diff --git a/apps/account-directory/scripts/verify-deployment-config.d.mts b/apps/account-directory/scripts/verify-deployment-config.d.mts index 8d050908a..4c80177b9 100644 --- a/apps/account-directory/scripts/verify-deployment-config.d.mts +++ b/apps/account-directory/scripts/verify-deployment-config.d.mts @@ -1,5 +1,6 @@ export declare const REQUIRED_SECRETS: readonly string[]; export declare const REQUIRED_VARS: readonly string[]; +export declare const DEFAULTED_VARS: readonly { name: string; codeDefault: string }[]; export declare const ENVIRONMENTS: readonly string[]; export declare class DeploymentConfigError extends Error {} @@ -10,7 +11,7 @@ export declare function verifyDirectoryDeploymentConfig(args: { environments?: readonly string[]; listSecretNames: (environment: string) => Iterable; readConfig: () => unknown; -}): void; +}): { warnings: string[] }; export declare function wranglerSecretListInvocation( environment: string, diff --git a/apps/account-directory/scripts/verify-deployment-config.mjs b/apps/account-directory/scripts/verify-deployment-config.mjs index 1f298f1ed..5149a96e3 100644 --- a/apps/account-directory/scripts/verify-deployment-config.mjs +++ b/apps/account-directory/scripts/verify-deployment-config.mjs @@ -15,16 +15,44 @@ import { dirname, resolve } from "node:path"; * relay hand-off has no URL to call, and an `ACTIVITY_RELAY` service binding * cannot supply one (it is only the transport). * - * `DIRECTORY_AUTH_SECRET` is a wrangler SECRET, so it is read from - * `wrangler secret list`; `PUSH_RELAY_URL` is a plain var, so it is read from - * the committed `wrangler.jsonc`. The deploy entry point passes the exact - * environment it is about to publish: wrangler environments do not inherit - * secrets, and a production deploy must not be blocked by an unrelated local - * development environment that is intentionally unconfigured. + * The Clerk trio is checked for exactly the same reason, one failure shape + * further in: `resolveCallerToken` throws "authentication unavailable" when any + * of `CLERK_JWKS_URL` / `CLERK_ISSUER` / `CLERK_OAUTH_CLIENT_ID` is blank, so a + * deploy missing one answers 503 on EVERY authenticated route and on the whole + * `/device/*` sign-in flow — while `/health` stays green, which is precisely the + * shape of the 2026-08-06 incident this preflight exists to prevent. + * `WEB_CLIENT_ORIGIN` earns a hard failure too: it has no code default, and + * without it no `access-control-allow-origin` is emitted, so the browser client + * at app.ade-app.dev is blocked outright. + * + * `ONLINE_WINDOW_MS` and `DIAGNOSTICS_DAILY_GLOBAL_LIMIT` are deliberately NOT + * hard requirements: both have code defaults equal to the committed values + * (`DEFAULT_ONLINE_WINDOW_MS` = 90_000, `DEFAULT_DIAGNOSTICS_DAILY_GLOBAL_LIMIT` + * = 400), so their absence changes no behavior — the diagnostics cost ceiling + * still applies at 400/day. Blocking a deploy on them would be a false gate. + * They warn instead, including when they are set to something the Worker cannot + * parse, because that silently falls back to the default rather than erroring. + * + * Secrets are read from `wrangler secret list` BY NAME ONLY — this never reads, + * prints, or logs a secret value. Vars are read from the committed + * `wrangler.jsonc`. The deploy entry point passes the exact environment it is + * about to publish: wrangler environments inherit neither vars nor secrets, and + * a production deploy must not be blocked by an unrelated local development + * environment that is intentionally unconfigured. */ -export const REQUIRED_SECRETS = ["DIRECTORY_AUTH_SECRET"]; -export const REQUIRED_VARS = ["PUSH_RELAY_URL"]; +export const REQUIRED_SECRETS = [ + "DIRECTORY_AUTH_SECRET", + "CLERK_JWKS_URL", + "CLERK_ISSUER", + "CLERK_OAUTH_CLIENT_ID", +]; +export const REQUIRED_VARS = ["PUSH_RELAY_URL", "WEB_CLIENT_ORIGIN"]; +/** Have code defaults: missing (or unparseable) is a warning, never a failure. */ +export const DEFAULTED_VARS = [ + { name: "ONLINE_WINDOW_MS", codeDefault: "90000" }, + { name: "DIAGNOSTICS_DAILY_GLOBAL_LIMIT", codeDefault: "400" }, +]; export const ENVIRONMENTS = ["default", "production"]; export class DeploymentConfigError extends Error { @@ -83,6 +111,7 @@ function varsForEnvironment(config, environment) { * @param {object} args * @param {(environment: string) => Iterable} args.listSecretNames * @param {() => object} args.readConfig + * @returns {{ warnings: string[] }} Non-blocking notes about defaulted vars. */ export function verifyDirectoryDeploymentConfig(args) { const environments = args.environments ?? ENVIRONMENTS; @@ -104,17 +133,39 @@ export function verifyDirectoryDeploymentConfig(args) { } } const config = args.readConfig(); + const warnings = []; for (const environment of environments) { const vars = varsForEnvironment(config, environment); - const missingVars = REQUIRED_VARS.filter( - (name) => typeof vars[name] !== "string" || !vars[name].trim(), - ); + const missingVars = REQUIRED_VARS.filter((name) => !isConfiguredVar(vars[name])); if (missingVars.length > 0) { throw new DeploymentConfigError( `missing Worker vars for the ${environment} environment: ${missingVars.join(", ")}`, ); } + for (const { name, codeDefault } of DEFAULTED_VARS) { + if (!isConfiguredVar(vars[name])) { + warnings.push( + `${name} is not set for the ${environment} environment; the Worker will use its code default of ${codeDefault}.`, + ); + } else if (!isNonNegativeNumber(vars[name])) { + // The Worker parses these with Number() and silently falls back, so a + // typo here reads as "configured" while doing nothing. + warnings.push( + `${name} for the ${environment} environment is not a non-negative number; the Worker will ignore it and use ${codeDefault}.`, + ); + } + } } + return { warnings }; +} + +function isConfiguredVar(value) { + return typeof value === "string" && Boolean(value.trim()); +} + +function isNonNegativeNumber(value) { + const parsed = Number(value); + return Number.isFinite(parsed) && parsed >= 0; } /** @@ -192,12 +243,13 @@ function main() { "..", "wrangler.jsonc", ); + let warnings = []; try { - verifyDirectoryDeploymentConfig({ + ({ warnings } = verifyDirectoryDeploymentConfig({ environments, listSecretNames: wranglerSecretNames, readConfig: () => parseJsonc(readFileSync(configPath, "utf8")), - }); + })); } catch (error) { console.error( `Account directory deployment preflight failed: ${ @@ -206,8 +258,9 @@ function main() { ); process.exit(1); } + for (const warning of warnings) console.warn(`Account directory deployment preflight: ${warning}`); console.log( - `Account directory relay hand-off configuration is complete for the ${environments.join(" and ")} environment${environments.length === 1 ? "" : "s"}.`, + `Account directory authentication and relay hand-off configuration is complete for the ${environments.join(" and ")} environment${environments.length === 1 ? "" : "s"}.`, ); } diff --git a/apps/account-directory/test/verifyDeploymentConfig.test.ts b/apps/account-directory/test/verifyDeploymentConfig.test.ts index 70d7c00c6..5bc722385 100644 --- a/apps/account-directory/test/verifyDeploymentConfig.test.ts +++ b/apps/account-directory/test/verifyDeploymentConfig.test.ts @@ -5,6 +5,7 @@ import { dirname, resolve } from "node:path"; import { describe, expect, it, vi } from "vitest"; import { DeploymentConfigError, + REQUIRED_SECRETS, parseJsonc, verifyDirectoryDeploymentConfig, wranglerSecretListInvocation, @@ -17,20 +18,27 @@ const wranglerConfigPath = resolve( "wrangler.jsonc", ); +const completeVars = { + PUSH_RELAY_URL: "https://relay.test", + WEB_CLIENT_ORIGIN: "https://app.test", + ONLINE_WINDOW_MS: "90000", + DIAGNOSTICS_DAILY_GLOBAL_LIMIT: "400", +}; + const completeConfig = { - vars: { PUSH_RELAY_URL: "https://relay.test" }, - env: { production: { vars: { PUSH_RELAY_URL: "https://relay.test" } } }, + vars: completeVars, + env: { production: { vars: completeVars } }, }; function verify(args: { environments?: string[]; secretsByEnvironment?: Record; config?: unknown; -}): void { - verifyDirectoryDeploymentConfig({ +}): { warnings: string[] } { + return verifyDirectoryDeploymentConfig({ environments: args.environments, listSecretNames: (environment) => - args.secretsByEnvironment?.[environment] ?? ["DIRECTORY_AUTH_SECRET"], + args.secretsByEnvironment?.[environment] ?? [...REQUIRED_SECRETS], readConfig: () => args.config ?? completeConfig, }); } @@ -55,7 +63,7 @@ describe("account directory deployment preflight", () => { expect(() => verify({ secretsByEnvironment: { - default: ["DIRECTORY_AUTH_SECRET"], + default: [...REQUIRED_SECRETS], production: ["CLERK_ISSUER"], }, }) @@ -65,10 +73,28 @@ describe("account directory deployment preflight", () => { it("does not require the unused default environment for a production deploy", () => { expect(() => verify({ environments: ["production"], - secretsByEnvironment: { default: [], production: ["DIRECTORY_AUTH_SECRET"] }, + secretsByEnvironment: { default: [], production: [...REQUIRED_SECRETS] }, })).not.toThrow(); }); + it.each(["CLERK_JWKS_URL", "CLERK_ISSUER", "CLERK_OAUTH_CLIENT_ID"])( + "fails when %s is not bound", + (missing) => { + // `resolveCallerToken` throws "authentication unavailable" when any of the + // trio is blank: every authenticated route answers 503 and the whole + // /device/* sign-in flow breaks, while /health stays green. + const secrets = REQUIRED_SECRETS.filter((name) => name !== missing); + expect(() => + verify({ secretsByEnvironment: { default: secrets, production: secrets } }) + ).toThrow(new RegExp(`default environment: ${missing}`)); + }, + ); + + it("names every missing secret at once", () => { + expect(() => verify({ secretsByEnvironment: { default: ["DIRECTORY_AUTH_SECRET"] } })) + .toThrow(/CLERK_JWKS_URL, CLERK_ISSUER, CLERK_OAUTH_CLIENT_ID/); + }); + it("rejects an unknown deployment environment", () => { expect(() => verify({ environments: ["staging"] })) .toThrow(/unknown Worker environment\(s\): staging/); @@ -77,26 +103,62 @@ describe("account directory deployment preflight", () => { it.each([ [ "the default environment", - { vars: {}, env: { production: { vars: { PUSH_RELAY_URL: "https://relay.test" } } } }, + { vars: {}, env: { production: { vars: completeVars } } }, /default environment: PUSH_RELAY_URL/, ], [ "the production environment", - { vars: { PUSH_RELAY_URL: "https://relay.test" }, env: {} }, + { vars: completeVars, env: {} }, /production environment: PUSH_RELAY_URL/, ], [ "an empty value", - { vars: { PUSH_RELAY_URL: " " }, env: { production: { vars: { PUSH_RELAY_URL: "https://relay.test" } } } }, + { vars: { ...completeVars, PUSH_RELAY_URL: " " }, env: { production: { vars: completeVars } } }, /default environment: PUSH_RELAY_URL/, ], ])("fails when PUSH_RELAY_URL is missing from %s", (_label, config, expected) => { expect(() => verify({ config })).toThrow(expected as RegExp); }); - it("accepts the committed wrangler.jsonc", () => { + it("fails when WEB_CLIENT_ORIGIN is missing", () => { + // No code default: without it the Worker emits no + // access-control-allow-origin, so the browser client is blocked outright. + const vars = { ...completeVars, WEB_CLIENT_ORIGIN: "" }; + expect(() => verify({ config: { vars, env: { production: { vars } } } })) + .toThrow(/default environment: WEB_CLIENT_ORIGIN/); + }); + + it("warns instead of failing for vars that have code defaults", () => { + // DIAGNOSTICS_DAILY_GLOBAL_LIMIT falls back to 400 and ONLINE_WINDOW_MS to + // 90000, both equal to the committed values, so their absence changes no + // behavior — the diagnostics cost ceiling still applies. Blocking a deploy + // on them would be a false gate. + const vars = { + PUSH_RELAY_URL: "https://relay.test", + WEB_CLIENT_ORIGIN: "https://app.test", + }; + const result = verify({ config: { vars, env: { production: { vars } } } }); + expect(result.warnings).toEqual([ + expect.stringContaining("ONLINE_WINDOW_MS is not set for the default environment"), + expect.stringContaining("DIAGNOSTICS_DAILY_GLOBAL_LIMIT is not set for the default environment"), + expect.stringContaining("ONLINE_WINDOW_MS is not set for the production environment"), + expect.stringContaining("DIAGNOSTICS_DAILY_GLOBAL_LIMIT is not set for the production environment"), + ]); + }); + + it("warns when a defaulted var is set to something the Worker cannot parse", () => { + // Number("unlimited") is NaN, so the Worker silently uses 400 while the + // config reads as configured. + const vars = { ...completeVars, DIAGNOSTICS_DAILY_GLOBAL_LIMIT: "unlimited" }; + const result = verify({ environments: ["production"], config: { vars, env: { production: { vars } } } }); + expect(result.warnings).toEqual([ + expect.stringContaining("DIAGNOSTICS_DAILY_GLOBAL_LIMIT for the production environment is not a non-negative number"), + ]); + }); + + it("accepts the committed wrangler.jsonc with no warnings", () => { const config = parseJsonc(readFileSync(wranglerConfigPath, "utf8")); - expect(() => verify({ config })).not.toThrow(); + expect(verify({ config })).toEqual({ warnings: [] }); }); }); From 0a2bc17c7cd1594bf3fd4730bdccd832faee45e8 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:15:43 -0400 Subject: [PATCH 4/9] fix(windows): reconcile a failed taskkill with what is actually running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit was right about one of the three sites it flagged. The supervisor loop in the installed-product smoke threw on any nonzero `taskkill`, but its process list is a snapshot: a supervisor that exits on its own between the snapshot and the kill is the state the loop wanted, and failing the smoke for it is the same spurious failure this branch already fixed once. It now checks whether the PID is still there AND still the channel-owned supervisor — the same post-check the loop directly below it has always had — and throws only then. The uninstall cleanup keeps its best-effort kill (an uninstall may not refuse to finish over a process it could not stop) but no longer says nothing about it: a supervisor still running after the kill now warns, because the user is about to be told the product was removed. Rejected: `Stop-LaunchedApp` is deliberately best-effort. Its whole intent is "this is not running", so a kill that fails because the process already exited is success, and there is nothing to reconcile. Co-Authored-By: Claude Opus 5 --- .../scripts/windows-installed-product-smoke.ps1 | 9 ++++++++- apps/desktop/scripts/windows-uninstall-cleanup.ps1 | 12 ++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/apps/desktop/scripts/windows-installed-product-smoke.ps1 b/apps/desktop/scripts/windows-installed-product-smoke.ps1 index 83241e76f..c0376ac9c 100644 --- a/apps/desktop/scripts/windows-installed-product-smoke.ps1 +++ b/apps/desktop/scripts/windows-installed-product-smoke.ps1 @@ -83,7 +83,14 @@ function Stop-InstalledProductProcesses { }) foreach ($supervisor in $supervisors) { if ((Invoke-TaskKill ([string]$supervisor.ProcessId)) -ne 0) { - throw "Could not stop channel-owned ADE supervisor $($supervisor.ProcessId) before repair." + # The process list is a snapshot, so a supervisor can exit on its own + # between the snapshot and the kill - which is the state we wanted. Only + # a PID that is still there AND still the channel-owned supervisor is a + # real failure. + $remaining = Get-CimInstance Win32_Process -Filter "ProcessId = $($supervisor.ProcessId)" -ErrorAction SilentlyContinue + if ($remaining -and ([string]$remaining.CommandLine).IndexOf($launcherPrefix, [StringComparison]::OrdinalIgnoreCase) -ge 0) { + throw "Could not stop channel-owned ADE supervisor $($supervisor.ProcessId) before repair." + } } } $processes = @($allProcesses | Where-Object { diff --git a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 index d6536010c..9b67fe677 100644 --- a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 +++ b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 @@ -211,7 +211,19 @@ function Remove-ChannelStartupWithoutPackagedCli( # the uninstall cleanup report failure for a process that is already # stopped - exactly the state it wanted. & taskkill.exe /PID $supervisorPid /T /F | Out-Null + $killExitCode = $LASTEXITCODE $global:LASTEXITCODE = 0 + # Best effort is not the same as unsaid. An uninstall must not refuse + # to finish over a process it could not stop — the PID record and the + # launcher are removed either way — but a supervisor that is STILL + # running after the kill is the one case worth a line, because the + # user is about to be told the product was removed. + if ($killExitCode -ne 0) { + $survivor = Get-CimInstance Win32_Process -Filter "ProcessId = $supervisorPid" -ErrorAction SilentlyContinue + if ($survivor -and ([string]$survivor.CommandLine).IndexOf($launcherPath, [StringComparison]::OrdinalIgnoreCase) -ge 0) { + Write-Warning "ADE could not stop its background service (PID $supervisorPid). Sign out or restart to finish removing it." + } + } } } } catch { From ceb3238fe40b824490a31448696d3ef1705c805f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:15:56 -0400 Subject: [PATCH 5/9] feat(diagnostics): a way to send a report when nothing has visibly broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every "Report issue" button lives on a screen that already failed — a crash boundary, the recovery screen, a failed repair, the connections list. A user whose app merely feels wrong has nowhere to press. Worse, the Diagnostics sharing settings section told them ADE sends "the same report the Report issue button makes", pointing at a button that may not exist anywhere on screen. A real user hit exactly that. That section now carries "Send a report to ADE". It goes through main (`IPC.diagnosticsSendManual` -> `autoDiagnosticsService.sendManual()`) and reuses the existing collector, redaction and uploader rather than duplicating any of it; the only thing that differs from an automatic send is who decided and what happens afterwards. `auto: false` and surface `settings_manual` keep these separable server-side from reports nobody chose to file. Two budgets, one file. Manual sends get their own daily cap — five per install per 24h, deliberately the server's own per-identity daily quota, so the client guard never refuses a report the account directory would still have accepted — counted apart from the automatic three via a new `kind` on each ledger entry. Neither can spend the other: pressing the button cannot silence the automatic reports that explain a crash, and a crash loop that has burned its three automatic sends cannot lock a user out of asking for help. Same file, same lock, same fail-closed rules; only the counters are separate. A manual send is allowed with the toggle off. That toggle governs what ADE does BY ITSELF; a deliberate click about a report the user can read first is not that, and refusing it would leave anyone who turned off background reporting unable to report anything at all. It is never silent about it: with the toggle off the card says the click sends one report now and does not turn automatic reports back on, and nothing here writes `enabled`. Refusals are three sentences because they are three situations — the local cap, the account directory's per-caller 429, and its fleet-wide 429/503. The route answers the two 429s with distinct bodies precisely so a client can tell them apart, so `uploadDiagnosticReport` now reads the body and maps the fleet one to `unavailable` instead of blaming the user for it. No status code reaches the screen; on success the line names the reference and offers View, the same affordance the auto-send toast has. Co-Authored-By: Claude Opus 5 --- apps/ade-cli/src/commands/doctor.ts | 21 ++- .../autoDiagnosticsService.test.ts | 119 +++++++++++++- .../diagnostics/autoDiagnosticsService.ts | 146 ++++++++++++++++- .../diagnostics/autoDiagnosticsStore.test.ts | 98 ++++++++++++ .../diagnostics/autoDiagnosticsStore.ts | 127 ++++++++++++++- .../src/main/services/ipc/registerIpc.ts | 33 +++- apps/desktop/src/preload/global.d.ts | 9 ++ apps/desktop/src/preload/preload.ts | 3 + .../DiagnosticsSharingSection.test.tsx | 138 ++++++++++++++++ .../settings/DiagnosticsSharingSection.tsx | 150 +++++++++++++++++- .../components/settings/settingsManifest.ts | 2 +- .../components/settings/settingsSectionUi.tsx | 19 +++ .../src/shared/diagnosticsUpload.test.ts | 22 +++ apps/desktop/src/shared/diagnosticsUpload.ts | 32 +++- apps/desktop/src/shared/ipc.ts | 7 + apps/desktop/src/shared/types/diagnostics.ts | 41 +++++ .../onboarding-and-settings/README.md | 9 +- docs/features/storage-and-recovery/README.md | 36 ++++- 18 files changed, 990 insertions(+), 22 deletions(-) create mode 100644 apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx diff --git a/apps/ade-cli/src/commands/doctor.ts b/apps/ade-cli/src/commands/doctor.ts index 9b25cdd15..070fcee0e 100644 --- a/apps/ade-cli/src/commands/doctor.ts +++ b/apps/ade-cli/src/commands/doctor.ts @@ -112,8 +112,18 @@ export type DoctorInput = { diagnostics?: DoctorDiagnosticsSharing | null; }; -/** What the shared auto-diagnostics ledger says, verbatim. */ -export type DoctorDiagnosticsSharing = ReturnType; +/** + * What the shared auto-diagnostics ledger says about AUTOMATIC sharing. + * + * Narrowed to the three fields this row reads rather than the ledger's whole + * view: the ledger also reports the separate manual-send budget, and `ade + * doctor` is a health check for what the machine does on its own — a report a + * person deliberately asked for is not a diagnostic about the machine. + */ +export type DoctorDiagnosticsSharing = Pick< + ReturnType, + "enabled" | "sendsInWindow" | "limit" +>; export type DoctorCommandOptions = { role: "cto" | "orchestrator" | "agent" | "external" | "evaluator"; @@ -895,7 +905,12 @@ export function readAutoDiagnosticsSharingForDoctor( env: NodeJS.ProcessEnv = process.env, ): DoctorDiagnosticsSharing | null { try { - return readAutoDiagnosticsState(resolveAutoDiagnosticsStateFile(adeDir, env)); + // Projected down to the automatic budget rather than handed over whole: + // the ledger also tracks the separate manual-send budget, and this row is + // about what the machine does on its own. + const { enabled, sendsInWindow, limit } = + readAutoDiagnosticsState(resolveAutoDiagnosticsStateFile(adeDir, env)); + return { enabled, sendsInWindow, limit }; } catch { return null; } diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.test.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.test.ts index b5dda0a82..e58ff9487 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.test.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.test.ts @@ -218,7 +218,13 @@ describe("createAutoDiagnosticsService", () => { const { service } = harness(); expect(service.isEnabled()).toBe(true); expect(service.setEnabled(false)).toBe(false); - expect(service.getStatus()).toEqual({ enabled: false, sendsInWindow: 0, limit: 3 }); + expect(service.getStatus()).toEqual({ + enabled: false, + sendsInWindow: 0, + limit: 3, + manualSendsInWindow: 0, + manualLimit: 5, + }); }); it("drops a failure code the server would refuse without touching the budget", async () => { @@ -258,3 +264,114 @@ describe("createAutoDiagnosticsService", () => { expect(service.getStatus().sendsInWindow).toBe(1); }); }); + +describe("createAutoDiagnosticsService.sendManual", () => { + it("sends the same report the failure screens send, tagged as not automatic", async () => { + const { service, upload, writeReportFile, onSent } = harness(); + + await expect(service.sendManual()).resolves.toEqual({ + ok: true, + reference: "abcd1234", + reportPath: "/tmp/reports/report.md", + }); + + expect(writeReportFile).toHaveBeenCalledWith("/tmp/reports/report.md", "# report for user_requested"); + // `auto: false` is what keeps these separable server-side from the reports + // nobody chose to file. + expect(upload.mock.calls[0]?.[0]).toMatchObject({ + auto: false, + failureCode: "user_requested", + report: "# report for user_requested", + installId: "install-1", + }); + // No toast: the person who pressed the button is already reading the answer. + expect(onSent).not.toHaveBeenCalled(); + expect(service.listPendingNotices()).toEqual([]); + }); + + it("refuses past the fifth send of the day, in the user's own words", async () => { + const { service, upload } = harness(); + + for (let i = 0; i < 5; i += 1) { + await expect(service.sendManual()).resolves.toMatchObject({ ok: true }); + } + await expect(service.sendManual()).resolves.toEqual({ + ok: false, + reason: "local_limit", + limit: 5, + }); + // Refused HERE, so the request never reaches the account directory and + // never spends one of the caller's server-side slots. + expect(upload).toHaveBeenCalledTimes(5); + }); + + it("keeps the manual budget and the automatic one out of each other's way", async () => { + const { service } = harness(); + + for (let i = 0; i < 5; i += 1) await service.sendManual(); + expect((await service.sendManual()).ok).toBe(false); + + // A user asking for help must not silence the reports that explain the + // failure they are asking about. + await expect(service.report({ failureCode: "disk_full", surface: "project_recovery" })) + .resolves.toBe("completed"); + expect(service.getStatus()).toMatchObject({ + sendsInWindow: 1, + limit: 3, + manualSendsInWindow: 5, + manualLimit: 5, + }); + }); + + it("still sends when automatic sharing is off, and does not turn it back on", async () => { + const { service, filePath, upload } = harness(); + setAutoDiagnosticsEnabled(filePath, false, { now: () => T0 }); + + // The toggle is about what ADE does BY ITSELF. Refusing a deliberate click + // would leave this user with no way to report anything at all. + await expect(service.sendManual()).resolves.toMatchObject({ ok: true }); + expect(upload).toHaveBeenCalledTimes(1); + expect(service.isEnabled()).toBe(false); + + // The automatic path stays refused, so consent is honoured where it applies. + await expect(service.report({ failureCode: "disk_full", surface: "project_recovery" })) + .resolves.toBe("skipped_disabled"); + }); + + it("carries each refusal through as its own reason, never a status code", async () => { + for ( + const [uploadReason, reason] of [ + ["rate_limited", "rate_limited"], + ["unavailable", "unavailable"], + ["too_large", "too_large"], + ["network", "failed"], + ["rejected", "failed"], + ] as const + ) { + const { service } = harness({ + upload: async () => ({ ok: false as const, reason: uploadReason }), + }); + await expect(service.sendManual()).resolves.toEqual({ + ok: false, + reason, + // The local copy still exists, so the surface can offer to show it. + reportPath: "/tmp/reports/report.md", + }); + } + }); + + it("keeps the reservation when the report cannot even be built", async () => { + const { service, upload } = harness({ + buildReport: async () => { + throw new Error("collector wedged"); + }, + }); + + await expect(service.sendManual()).resolves.toEqual({ ok: false, reason: "failed" }); + expect(upload).not.toHaveBeenCalled(); + // Spent on purpose: what the budget bounds is how often this computer + // tries, not how often it wins. A collector that wedges every time would + // otherwise be an unbounded retry loop behind a button. + expect(service.getStatus().manualSendsInWindow).toBe(1); + }); +}); diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts index 16d03d072..708661ff6 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts @@ -14,12 +14,17 @@ import { } from "./autoDiagnosticsSend"; import { ackAutoDiagnosticsNotices, + claimManualDiagnosticsSend, + completeAutoDiagnosticsSend, isAutoDiagnosticsEnabled, listPendingAutoDiagnosticsNotices, readAutoDiagnosticsState, setAutoDiagnosticsEnabled, + MANUAL_DIAGNOSTICS_FAILURE_CODE, + MAX_MANUAL_DIAGNOSTICS_PER_WINDOW, type AutoDiagnosticsNotice, } from "./autoDiagnosticsStore"; +import type { DiagnosticsManualSendResult } from "../../../shared/types/diagnostics"; /** * Sends the diagnostic report nobody was ever going to press the button for. @@ -43,6 +48,8 @@ import { * built, that it uploads anonymously, and the toggle the settings pane reads. */ +type ManualSendFailure = Extract["reason"]; + export type AutoDiagnosticsRequest = { /** Short machine code, e.g. `brain_crash_looping`. Never free text. */ failureCode: string; @@ -88,9 +95,20 @@ export type AutoDiagnosticsServiceDeps = { export type AutoDiagnosticsService = { /** One call per failure point. Never throws and never rejects. */ report: (request: AutoDiagnosticsRequest) => Promise; + /** + * One report, because a person asked for it. Never throws and never rejects; + * every outcome comes back named so the surface can say what happened. + */ + sendManual: () => Promise; isEnabled: () => boolean; setEnabled: (enabled: boolean) => boolean; - getStatus: () => { enabled: boolean; sendsInWindow: number; limit: number }; + getStatus: () => { + enabled: boolean; + sendsInWindow: number; + limit: number; + manualSendsInWindow: number; + manualLimit: number; + }; /** * The sends no renderer has acknowledged showing yet — the brain's, and any * this process made while no window was listening. Read when a renderer @@ -145,8 +163,134 @@ export function createAutoDiagnosticsService( } }; + /** + * "Send a report to ADE", pressed by hand in Settings. + * + * Same collector, same redaction, same uploader as every other send — the + * only thing that differs is who decided and what happens afterwards. + * + * Consent is deliberately NOT checked. The setting the pane offers is about + * reports ADE files BY ITSELF when something breaks; this is a person asking, + * about a report they can open and read. Refusing it would mean anyone who + * turned off background reporting has no way to send anything at all — and + * that is exactly the user this control exists for. The pane says as much + * next to the button when the toggle is off, so the click can never be + * mistaken for turning automatic sending back on. Nothing here writes + * `enabled`. + * + * It is also NOT `runAutoDiagnosticsSend`: that policy is silence on failure + * and a pending toast, both correct for a send nobody asked for and both + * wrong here. A user watching a button has to be told the answer, and telling + * them twice — inline and again in a toast — is worse than once. + */ + const sendManual = async (): Promise => { + // A collection already running is not a budget event: nothing is claimed, + // so a retry a moment later costs the user nothing. + if (inFlight) return { ok: false, reason: "failed" }; + inFlight = true; + try { + const claim = claimManualDiagnosticsSend({ + filePath: deps.stateFilePath, + source: "desktop", + now, + }); + if (!claim.allowed) { + deps.logger?.info?.("diagnostics.manual_send_skipped", { reason: claim.reason }); + // A ledger that cannot be read or locked fails closed, exactly as the + // automatic claim does — but the user gets told, rather than nothing. + return claim.reason === "daily_limit" + ? { ok: false, reason: "local_limit", limit: MAX_MANUAL_DIAGNOSTICS_PER_WINDOW } + : { ok: false, reason: "failed" }; + } + + let built: AutoDiagnosticsReport; + try { + built = await deps.buildReport({ + failureCode: MANUAL_DIAGNOSTICS_FAILURE_CODE, + surface: "settings_manual", + }); + } catch (error) { + deps.logger?.warn?.("diagnostics.manual_send_build_failed", { + error: error instanceof Error ? error.message : String(error), + }); + // The reservation stays spent, for the automatic sender's reason: what + // it bounds is how often this computer tries, not how often it wins. + completeAutoDiagnosticsSend({ + filePath: deps.stateFilePath, + failureCode: MANUAL_DIAGNOSTICS_FAILURE_CODE, + atMs: claim.atMs, + reportPath: null, + reference: null, + pending: false, + kind: "manual", + now, + }); + return { ok: false, reason: "failed" }; + } + + let reportPath: string | null; + try { + reportPath = writeReportFile(built.filePath, built.report) ? built.filePath : null; + } catch { + reportPath = null; + } + + let result: AutoDiagnosticsUploadResult; + try { + result = await upload({ + baseUrl: resolveDiagnosticsUploadBaseUrl(env.ADE_ACCOUNT_DIRECTORY_URL), + report: built.report, + installId: built.installId === "unknown" ? null : built.installId, + appVersion: deps.appVersion, + // `auto: false` is the whole point of the flag: server-side these have + // to stay separable from the reports nobody chose to file. + auto: false, + failureCode: MANUAL_DIAGNOSTICS_FAILURE_CODE, + }); + } catch { + result = { ok: false, reason: "network" }; + } + + completeAutoDiagnosticsSend({ + filePath: deps.stateFilePath, + failureCode: MANUAL_DIAGNOSTICS_FAILURE_CODE, + atMs: claim.atMs, + reportPath, + reference: result.ok ? result.reference : null, + // Never pending: the person who pressed the button is looking at the + // answer. A toast on top of it would be the same news twice. + pending: false, + kind: "manual", + now, + }); + + if (!result.ok) { + deps.logger?.warn?.("diagnostics.manual_send_failed", { reason: result.reason }); + // The uploader tells the two 429s apart for us: `rate_limited` is this + // caller's own daily allowance, `unavailable` is ADE not taking reports + // from anyone. Those are different sentences to a user, so they stay + // different reasons all the way to the screen. + const reason: ManualSendFailure = + result.reason === "rate_limited" + ? "rate_limited" + : result.reason === "unavailable" + ? "unavailable" + : result.reason === "too_large" + ? "too_large" + : "failed"; + return { ok: false, reason, ...(reportPath ? { reportPath } : {}) }; + } + + deps.logger?.info?.("diagnostics.manual_sent", { reference: result.reference }); + return { ok: true, reference: result.reference, reportPath: reportPath ?? "" }; + } finally { + inFlight = false; + } + }; + return { report, + sendManual, isEnabled: () => isAutoDiagnosticsEnabled(deps.stateFilePath), setEnabled: (enabled) => setAutoDiagnosticsEnabled(deps.stateFilePath, enabled, { now }), getStatus: () => readAutoDiagnosticsState(deps.stateFilePath, { now }), diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts index 3ca777d53..e260dc6c3 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts @@ -6,6 +6,7 @@ import { ackAutoDiagnosticsNotices, AUTO_DIAGNOSTICS_WINDOW_MS, claimAutoDiagnosticsSend, + claimManualDiagnosticsSend, completeAutoDiagnosticsSend, isAutoDiagnosticsEnabled, listPendingAutoDiagnosticsNotices, @@ -37,6 +38,8 @@ describe("auto diagnostics budget", () => { enabled: true, sendsInWindow: 0, limit: 3, + manualSendsInWindow: 0, + manualLimit: 5, }); }); @@ -151,6 +154,8 @@ describe("auto diagnostics budget", () => { enabled: false, sendsInWindow: 1, limit: 3, + manualSendsInWindow: 0, + manualLimit: 5, }); }); @@ -263,6 +268,99 @@ describe("auto diagnostics budget", () => { }); }); +describe("manual diagnostics budget", () => { + const claimManual = (filePath: string, atMs: number) => + claimManualDiagnosticsSend({ filePath, source: "desktop", now: () => atMs }); + const claimAuto = (filePath: string, failureCode: string, atMs: number) => + claimAutoDiagnosticsSend({ filePath, failureCode, source: "desktop", now: () => atMs }); + + it("allows five sends a day and then refuses, in the user's own words", () => { + const filePath = stateFile(); + for (let i = 0; i < 5; i += 1) { + expect(claimManual(filePath, T0 + i * 60_000).allowed).toBe(true); + } + expect(claimManual(filePath, T0 + 300_000)).toEqual({ allowed: false, reason: "daily_limit" }); + // And it comes back when the window rolls, rather than being spent forever. + expect(claimManual(filePath, T0 + AUTO_DIAGNOSTICS_WINDOW_MS + 1).allowed).toBe(true); + }); + + it("does not spend the automatic budget, and is not spent by it", () => { + const filePath = stateFile(); + + // A user pressing the button five times must not silence the automatic + // reports that would explain the failure they are reporting. + for (let i = 0; i < 5; i += 1) claimManual(filePath, T0 + i * 60_000); + expect(claimAuto(filePath, "disk_full", T0 + 400_000).allowed).toBe(true); + expect(claimAuto(filePath, "db_integrity", T0 + 410_000).allowed).toBe(true); + expect(claimAuto(filePath, "renderer_crash", T0 + 420_000).allowed).toBe(true); + expect(claimAuto(filePath, "update_service", T0 + 430_000)) + .toEqual({ allowed: false, reason: "daily_limit" }); + + // And the reverse: a machine that has burned its three automatic sends must + // not lock the user out of asking for help. + const other = stateFile(); + claimAuto(other, "disk_full", T0); + claimAuto(other, "db_integrity", T0 + 1_000); + claimAuto(other, "renderer_crash", T0 + 2_000); + expect(claimAuto(other, "update_service", T0 + 3_000)) + .toEqual({ allowed: false, reason: "daily_limit" }); + expect(claimManual(other, T0 + 4_000).allowed).toBe(true); + }); + + it("counts the two budgets apart in the settings view", () => { + const filePath = stateFile(); + claimAuto(filePath, "disk_full", T0); + claimManual(filePath, T0 + 1_000); + claimManual(filePath, T0 + 2_000); + + expect(readAutoDiagnosticsState(filePath, { now: () => T0 + 3_000 })).toEqual({ + enabled: true, + sendsInWindow: 1, + limit: 3, + manualSendsInWindow: 2, + manualLimit: 5, + }); + }); + + it("sends even when automatic sharing is switched off", () => { + const filePath = stateFile(); + setAutoDiagnosticsEnabled(filePath, false, { now: () => T0 }); + + // The toggle governs what ADE does BY ITSELF. A deliberate click is not + // that, and refusing it would leave this user unable to report anything. + expect(claimManual(filePath, T0 + 1_000).allowed).toBe(true); + expect(claimAuto(filePath, "disk_full", T0 + 2_000)) + .toEqual({ allowed: false, reason: "disabled" }); + // ...and asking never flips the setting back on. + expect(isAutoDiagnosticsEnabled(filePath)).toBe(false); + }); + + it("only annotates an entry of the kind that was claimed", () => { + const filePath = stateFile(); + expect(claimManual(filePath, T0).allowed).toBe(true); + const complete = (kind: "auto" | "manual") => + completeAutoDiagnosticsSend({ + filePath, + failureCode: "user_requested", + atMs: T0, + reportPath: "/tmp/report.md", + reference: "abcd1234", + pending: true, + kind, + now: () => T0, + }); + + // The default kind is `auto`, so an automatic completion must not land on a + // manual reservation that happens to share its code and timestamp. + complete("auto"); + expect(listPendingAutoDiagnosticsNotices(filePath)).toEqual([]); + complete("manual"); + expect(listPendingAutoDiagnosticsNotices(filePath)).toEqual([ + { failureCode: "user_requested", reportPath: "/tmp/report.md", reference: "abcd1234" }, + ]); + }); +}); + describe("normalizeAutoDiagnosticsFailureCode", () => { it("passes the codes ADE actually produces", () => { for (const code of ["disk_full", "brain_crash_looping", "snapshot_failed", "update_service"]) { diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts index 9df690806..e746e0d64 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts @@ -36,6 +36,32 @@ export const AUTO_DIAGNOSTICS_WINDOW_MS = 24 * 60 * 60 * 1_000; export const MAX_AUTO_DIAGNOSTICS_PER_CODE = 1; /** At most this many automatic reports in total per window, per install. */ export const MAX_AUTO_DIAGNOSTICS_PER_WINDOW = 3; +/** + * At most this many MANUAL reports per window, per install. + * + * Deliberately the same number as `MAX_DIAGNOSTIC_UPLOADS_PER_DAY` in + * `apps/account-directory/src/diagnostics.ts`, the server's per-identity daily + * quota. That is the point: this guard exists so a person leaning on the button + * gets one honest sentence instead of a server refusal, and matching the + * server's number means it never refuses a report the user was still entitled + * to send. The server stays the authority — its quota counts stored objects, + * keyed on the caller address, and this cannot weaken it. + * + * More generous than the automatic budget (3 a day, one per failure class), and + * for a plain reason: an automatic send is one nobody chose, so its ceiling is a + * promise about what the computer does on its own. A manual send is a person + * asking for help about a report they can read first. + */ +export const MAX_MANUAL_DIAGNOSTICS_PER_WINDOW = 5; + +/** + * The ledger code every manual send is filed under. + * + * Manual sends have no failure class to dedupe on — the whole point is that + * nothing classified itself — so they share one code and are bounded only by + * the window total above. + */ +export const MANUAL_DIAGNOSTICS_FAILURE_CODE = "user_requested"; /** * Shape the server accepts for `failureCode`. Checked here so a caller that @@ -63,11 +89,27 @@ export function normalizeAutoDiagnosticsFailureCode(value: string | null | undef export type AutoDiagnosticsSource = "desktop" | "brain"; +/** + * Who decided to send: ADE, or the person using it. + * + * TWO BUDGETS, ONE FILE. They are counted apart on purpose and neither can + * borrow from the other: a user pressing "Send a report" must not use up the + * automatic reports that explain the crash they are reporting, and a machine in + * a crash loop that has burned its three automatic sends must not lock the user + * out of asking for help. Sharing the file is still right — the lock, the + * window and the retention cap are one mechanism — but the counters are not. + * + * Absent in entries written before manual sends existed, and those were all + * automatic, so the default is `auto`. + */ +export type AutoDiagnosticsKind = "auto" | "manual"; + /** One spent send. Codes and timestamps only — never a report or its text. */ export type AutoDiagnosticsSend = { code: string; atMs: number; source: AutoDiagnosticsSource; + kind: AutoDiagnosticsKind; /** Local path of the saved `.md`, so the toast's "View" can reveal it. */ reportPath: string | null; /** Short upload handle, present once the upload succeeded. */ @@ -116,6 +158,7 @@ function readSend(value: unknown): AutoDiagnosticsSend | null { code, atMs, source: record.source === "brain" ? "brain" : "desktop", + kind: record.kind === "manual" ? "manual" : "auto", reportPath: typeof record.reportPath === "string" && record.reportPath.trim() ? record.reportPath : null, @@ -411,16 +454,20 @@ export function claimAutoDiagnosticsSend(args: { } const nowMs = now(); const recent = withinWindow(state.sends, nowMs); - if (recent.filter((entry) => entry.code === code).length >= MAX_AUTO_DIAGNOSTICS_PER_CODE) { + // Only automatic entries count against the automatic budget; a user who + // pressed "Send a report" has not spent any of it. + const automatic = recent.filter((entry) => entry.kind === "auto"); + if (automatic.filter((entry) => entry.code === code).length >= MAX_AUTO_DIAGNOSTICS_PER_CODE) { return { state: null, result: { allowed: false, reason: "code_limit" } }; } - if (recent.length >= MAX_AUTO_DIAGNOSTICS_PER_WINDOW) { + if (automatic.length >= MAX_AUTO_DIAGNOSTICS_PER_WINDOW) { return { state: null, result: { allowed: false, reason: "daily_limit" } }; } const entry: AutoDiagnosticsSend = { code, atMs: nowMs, source: args.source, + kind: "auto", reportPath: null, reference: null, pending: false, @@ -436,6 +483,54 @@ export function claimAutoDiagnosticsSend(args: { ); } +/** + * Reserves one MANUAL send, or explains why there is none to reserve. + * + * Same shape and the same fail-closed rules as the automatic claim, counting a + * separate budget (see `AutoDiagnosticsKind`). Two differences, both deliberate: + * + * - No consent check. The setting governs reports ADE sends BY ITSELF; a + * person pressing a button is not that, and refusing them would leave a + * user who turned off background reporting with no way to ask for help. + * The surface that offers the button says so plainly instead. + * - No per-code limit. Manual sends have no failure class to dedupe on. + */ +export function claimManualDiagnosticsSend(args: { + filePath: string; + source: AutoDiagnosticsSource; + now?: () => number; +}): AutoDiagnosticsClaim { + const now = args.now ?? Date.now; + return mutate( + args.filePath, + now, + (state, readable) => { + if (!readable) { + return { state: null, result: { allowed: false, reason: "state_unavailable" } }; + } + const nowMs = now(); + const recent = withinWindow(state.sends, nowMs); + if (recent.filter((entry) => entry.kind === "manual").length >= MAX_MANUAL_DIAGNOSTICS_PER_WINDOW) { + return { state: null, result: { allowed: false, reason: "daily_limit" } }; + } + const entry: AutoDiagnosticsSend = { + code: MANUAL_DIAGNOSTICS_FAILURE_CODE, + atMs: nowMs, + source: args.source, + kind: "manual", + reportPath: null, + reference: null, + pending: false, + }; + return { + state: { ...state, sends: [...recent, entry].slice(-MAX_RETAINED_SENDS) }, + result: { allowed: true, atMs: nowMs }, + }; + }, + () => ({ allowed: false, reason: "state_unavailable" }), + ); +} + /** * Records the result of a claimed send. * @@ -458,18 +553,22 @@ export function completeAutoDiagnosticsSend(args: { reportPath: string | null; reference: string | null; pending: boolean; + /** Which budget the reservation came out of. Defaults to the automatic one. */ + kind?: AutoDiagnosticsKind; now?: () => number; }): void { const now = args.now ?? Date.now; const code = normalizeAutoDiagnosticsFailureCode(args.failureCode); if (!code) return; + const kind = args.kind ?? "auto"; const reference = args.reference?.trim() || null; const pending = args.pending && reference != null; mutate( args.filePath, now, (state) => { - const index = state.sends.findIndex((entry) => entry.code === code && entry.atMs === args.atMs); + const index = state.sends.findIndex((entry) => + entry.code === code && entry.atMs === args.atMs && entry.kind === kind); if (index < 0) return { state: null, result: undefined }; const sends = [...state.sends]; sends[index] = { @@ -555,16 +654,32 @@ export function ackAutoDiagnosticsNotices( ); } -/** Read-only view for tests and for the settings pane's spend line. */ +/** + * Read-only view for tests and for the settings pane's spend line. + * + * `sendsInWindow` counts AUTOMATIC sends only, because that is the number the + * pane's footnote is about ("at most three a day, one per problem"). Manual + * sends are reported separately for the same reason they are counted + * separately. + */ export function readAutoDiagnosticsState( filePath: string, deps: { now?: () => number } = {}, -): { enabled: boolean; sendsInWindow: number; limit: number } { +): { + enabled: boolean; + sendsInWindow: number; + limit: number; + manualSendsInWindow: number; + manualLimit: number; +} { const now = deps.now ?? Date.now; const { state } = readState(filePath); + const recent = withinWindow(state.sends, now()); return { enabled: state.enabled, - sendsInWindow: withinWindow(state.sends, now()).length, + sendsInWindow: recent.filter((entry) => entry.kind === "auto").length, limit: MAX_AUTO_DIAGNOSTICS_PER_WINDOW, + manualSendsInWindow: recent.filter((entry) => entry.kind === "manual").length, + manualLimit: MAX_MANUAL_DIAGNOSTICS_PER_WINDOW, }; } diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 732ecf3af..9184aa982 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -817,11 +817,15 @@ import { writeDiagnosticReportFile, } from "../diagnostics/diagnosticReportService"; import type { AutoDiagnosticsService } from "../diagnostics/autoDiagnosticsService"; -import { MAX_AUTO_DIAGNOSTICS_PER_WINDOW } from "../diagnostics/autoDiagnosticsStore"; +import { + MAX_AUTO_DIAGNOSTICS_PER_WINDOW, + MAX_MANUAL_DIAGNOSTICS_PER_WINDOW, +} from "../diagnostics/autoDiagnosticsStore"; import type { DiagnosticReportPayload, DiagnosticReportRequestPayload, DiagnosticsAutoSentPayload, + DiagnosticsManualSendResult, DiagnosticsSharingStatus, } from "../../../shared/types/diagnostics"; @@ -4824,9 +4828,34 @@ export function registerIpc({ }, ); + /** + * "Send a report to ADE" from the Diagnostics sharing settings section. + * + * Takes no argument on purpose. Every other report carries a surface and a + * context from the screen that failed; this one is about nothing in + * particular, so main names the surface itself (`settings_manual`) and uses + * the project it already has open. A renderer choosing either would be a + * renderer choosing whose logs go in the report. + * + * `null` rather than a throw when the service is absent (a runtime mode + * without it): the caller renders "unavailable right now", which is true, + * instead of an exception it would have to translate. + */ + ipcMain.handle( + IPC.diagnosticsSendManual, + async (): Promise => + (await autoDiagnosticsService?.sendManual()) ?? { ok: false, reason: "failed" }, + ); + const diagnosticsSharingStatus = (): DiagnosticsSharingStatus => autoDiagnosticsService?.getStatus() - ?? { enabled: true, sendsInWindow: 0, limit: MAX_AUTO_DIAGNOSTICS_PER_WINDOW }; + ?? { + enabled: true, + sendsInWindow: 0, + limit: MAX_AUTO_DIAGNOSTICS_PER_WINDOW, + manualSendsInWindow: 0, + manualLimit: MAX_MANUAL_DIAGNOSTICS_PER_WINDOW, + }; ipcMain.handle(IPC.diagnosticsGetSharing, async (): Promise => diagnosticsSharingStatus()); diff --git a/apps/desktop/src/preload/global.d.ts b/apps/desktop/src/preload/global.d.ts index e69340df1..87ac7ca56 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -729,6 +729,7 @@ import type { DiagnosticReportPayload, DiagnosticReportRequestPayload, DiagnosticsAutoSentPayload, + DiagnosticsManualSendResult, DiagnosticsSharingStatus, } from "../shared/types/diagnostics"; import type { AppPackageChannel } from "../shared/packageChannel"; @@ -919,6 +920,14 @@ declare global { * not an instruction, and its answer is deliberately uninteresting. */ autoReport: (context: DiagnosticReportRequestPayload) => Promise; + /** + * The one member here that IS individually optional, and the exception + * proves the group's rule: `diagnostics` shipped before this existed, + * so a preload that exposes the group need not expose this. The + * settings control checks for it and hides itself rather than offering + * a button that cannot work. + */ + sendManual?: () => Promise; getSharing: () => Promise; setSharing: (enabled: boolean) => Promise; revealReport: (reportPath: string) => Promise; diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 12d58b149..5bb0ce13c 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -32,6 +32,7 @@ import type { DiagnosticReportPayload, DiagnosticReportRequestPayload, DiagnosticsAutoSentPayload, + DiagnosticsManualSendResult, DiagnosticsSharingStatus, } from "../shared/types/diagnostics"; import type { @@ -4113,6 +4114,8 @@ const adeBridge = { ipcRenderer.invoke(IPC.diagnosticsOpenIssue, context), autoReport: (context: DiagnosticReportRequestPayload): Promise => ipcRenderer.invoke(IPC.diagnosticsAutoReport, context), + sendManual: (): Promise => + ipcRenderer.invoke(IPC.diagnosticsSendManual), getSharing: (): Promise => ipcRenderer.invoke(IPC.diagnosticsGetSharing), setSharing: (enabled: boolean): Promise => diff --git a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx new file mode 100644 index 000000000..e86cd2d21 --- /dev/null +++ b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx @@ -0,0 +1,138 @@ +/* @vitest-environment jsdom */ + +import React from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import type { + DiagnosticsManualSendResult, + DiagnosticsSharingStatus, +} from "../../../shared/types/diagnostics"; +import { DiagnosticsSharingSection } from "./DiagnosticsSharingSection"; + +/** + * The always-available way to send a report. + * + * Every other "Report issue" button in the app sits on a screen that has + * already broken, so a user whose app merely feels wrong had nowhere to press — + * while this very section told them ADE sends "the same report the Report issue + * button makes". What is tested here is that the promise is now true and that + * every refusal says something a person can act on. + */ + +const SHARING_ON: DiagnosticsSharingStatus = { + enabled: true, + sendsInWindow: 0, + limit: 3, + manualSendsInWindow: 0, + manualLimit: 5, +}; + +function mountSection(overrides: { + sharing?: DiagnosticsSharingStatus; + sendManual?: () => Promise; + revealReport?: (reportPath: string) => Promise; + omitSendManual?: boolean; +}) { + const sendManual = overrides.sendManual + ?? (async () => ({ ok: true as const, reference: "abcd1234", reportPath: "/tmp/report.md" })); + const revealReport = overrides.revealReport ?? (async () => undefined); + (window as unknown as { ade?: unknown }).ade = { + diagnostics: { + getSharing: async () => overrides.sharing ?? SHARING_ON, + setSharing: async () => overrides.sharing ?? SHARING_ON, + revealReport, + ...(overrides.omitSendManual ? {} : { sendManual }), + }, + }; + return { ...render(), sendManual, revealReport }; +} + +async function pressSend() { + const button = await screen.findByRole("button", { name: "Send a report to ADE" }); + fireEvent.click(button); +} + +beforeEach(() => { + delete (window as unknown as { ade?: unknown }).ade; +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + delete (window as unknown as { ade?: unknown }).ade; +}); + +describe("DiagnosticsSharingSection manual send", () => { + it("sends, names the reference, and offers to show exactly what was sent", async () => { + const revealReport = vi.fn(async () => undefined); + mountSection({ revealReport }); + + await pressSend(); + + const status = await screen.findByRole("status"); + expect(status.textContent).toContain("Report sent. Reference abcd1234"); + // Same affordance the auto-send toast offers: the user can read the bytes + // that left their computer. + fireEvent.click(screen.getByRole("button", { name: "View report" })); + expect(revealReport).toHaveBeenCalledWith("/tmp/report.md"); + }); + + it("says something different for each refusal, and never a status code", async () => { + const cases: Array<[DiagnosticsManualSendResult, string]> = [ + [ + { ok: false, reason: "local_limit", limit: 5 }, + "You've already sent 5 reports from this computer today. Try again tomorrow.", + ], + // The two 429s the account directory answers with are deliberately + // distinguishable, and they are two different situations to a user: one + // is about them, the other is not. + [ + { ok: false, reason: "rate_limited" }, + "You've already sent several reports today. Try again tomorrow.", + ], + [ + { ok: false, reason: "unavailable" }, + "ADE isn't accepting reports right now. Try again later.", + ], + [ + { ok: false, reason: "failed" }, + "ADE couldn't send the report. Check your connection and try again.", + ], + ]; + + for (const [result, copy] of cases) { + mountSection({ sendManual: async () => result }); + await pressSend(); + await waitFor(() => { + expect(screen.getByRole("status").textContent).toContain(copy); + }); + for (const text of ["429", "503", "status"]) { + expect(screen.getByRole("status").textContent).not.toContain(text); + } + cleanup(); + } + }); + + it("says out loud that a click does not turn automatic reports back on", async () => { + mountSection({ sharing: { ...SHARING_ON, enabled: false } }); + + // The send is still allowed — the toggle governs what ADE does BY ITSELF, + // and refusing here would leave this user unable to report anything — but + // it must never read as re-enabling background sharing. + expect(await screen.findByText(/Automatic reports are off/)).toBeTruthy(); + expect(screen.getByText(/does not turn\s+automatic reports back on/)).toBeTruthy(); + + await pressSend(); + await waitFor(() => { + expect(screen.getByRole("status").textContent).toContain("Report sent"); + }); + }); + + it("hides the control on a preload that predates it rather than offering a dead button", async () => { + mountSection({ omitSendManual: true }); + + // The toggle still renders; only the action it cannot perform is absent. + await screen.findByRole("switch"); + expect(screen.queryByRole("button", { name: "Send a report to ADE" })).toBeNull(); + }); +}); diff --git a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx index 4d833ca44..3bc5a292c 100644 --- a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx +++ b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx @@ -1,14 +1,26 @@ -import React from "react"; +import React, { useState } from "react"; import { Lifebuoy } from "@phosphor-icons/react"; -import type { DiagnosticsSharingStatus } from "../../../shared/types/diagnostics"; +import type { + DiagnosticsManualSendResult, + DiagnosticsSharingStatus, +} from "../../../shared/types/diagnostics"; +import { COLORS, SANS_FONT, outlineButton } from "../lanes/laneDesignTokens"; import { ConsentToggleSection } from "./settingsSectionUi"; /** - * The off switch for automatic diagnostic reports. + * The off switch for automatic diagnostic reports — and the one place a user + * can send one on purpose. * * Same shape as the analytics section next to it, and now literally the same * component: this is a consent control, so it reads the real persisted state * rather than assuming, and it says plainly what gets sent and how often. + * + * The manual send is here because the copy below already promised it. Every + * other "Report issue" button in the app lives on a screen that has already + * broken — a crash boundary, a recovery screen, a failed repair — so a user + * whose app merely FEELS wrong had nowhere to press, while this section told + * them ADE sends "the same report the Report issue button makes". That button + * has to exist somewhere they can always reach. */ export function DiagnosticsSharingSection() { const bridge = window.ade?.diagnostics; @@ -27,6 +39,136 @@ export function DiagnosticsSharingSection() { write={bridge ? (enabled) => bridge.setSharing(enabled) : undefined} readErrorMessage="This setting is unavailable right now." writeErrorMessage="ADE could not save this setting." - /> + > + {(status) => } + + ); +} + +const NOTE_STYLE: React.CSSProperties = { + margin: "8px 0 0", + color: COLORS.textMuted, + fontFamily: SANS_FONT, + fontSize: 11, + lineHeight: 1.5, +}; + +const LINK_STYLE: React.CSSProperties = { + background: "none", + border: "none", + padding: 0, + color: COLORS.textSecondary, + cursor: "pointer", + font: "inherit", + textDecoration: "underline", +}; + +/** + * One short sentence per outcome, and never a status code. + * + * The two refusals a person can act on differently are deliberately worded + * differently: `rate_limited` is the account directory saying THIS computer has + * stored its allowance today, `unavailable` is it saying it is not taking + * reports from anyone right now. The route answers those as two distinct 429 + * bodies precisely so a client can tell them apart, and telling someone to come + * back tomorrow when the truth is "ADE is full" would waste their time. + */ +function describeManualSendFailure(result: Extract): string { + switch (result.reason) { + case "local_limit": + return `You've already sent ${result.limit ?? 5} reports from this computer today. Try again tomorrow.`; + case "rate_limited": + return "You've already sent several reports today. Try again tomorrow."; + case "unavailable": + return "ADE isn't accepting reports right now. Try again later."; + case "too_large": + return "This report is too big to send. It's saved on this computer — open it and attach it to a GitHub issue."; + default: + return "ADE couldn't send the report. Check your connection and try again."; + } +} + +/** + * "Send a report to ADE", for when nothing is visibly broken. + * + * Everything that matters happens in the main process — building the report, + * redacting it, the per-device manual budget, the upload — because a renderer + * cannot be trusted with any of it and because this must be the SAME report the + * error screens send. This component only presses the button and reads the + * answer back honestly. + */ +function ManualDiagnosticsSend({ sharingEnabled }: { sharingEnabled: boolean }) { + const bridge = window.ade?.diagnostics; + const [sending, setSending] = useState(false); + const [result, setResult] = useState(null); + + // An older preload has no `sendManual`; offering a button that cannot work is + // worse than offering none. + if (!bridge?.sendManual) return null; + + const send = async () => { + if (sending) return; + setSending(true); + setResult(null); + try { + setResult(await bridge.sendManual!()); + } catch { + setResult({ ok: false, reason: "failed" }); + } finally { + setSending(false); + } + }; + + const reportPath = result?.ok ? result.reportPath : result?.reportPath ?? ""; + + return ( +
+
+ + + Something feels wrong but nothing has broken? Send one now. + +
+ + {/* + Consent, said out loud rather than quietly contradicted. A deliberate + click sends whether or not automatic sharing is on — the toggle is about + what ADE does BY ITSELF, and a user who turned it off still deserves a + way to ask for help — but they must never be able to mistake this click + for switching background reporting back on. + */} + {sharingEnabled ? null : ( +

+ Automatic reports are off. This sends one report, now. It does not turn + automatic reports back on. +

+ )} + + {result ? ( +

+ {result.ok + ? `Report sent. Reference ${result.reference} — quote it if you get in touch.` + : describeManualSendFailure(result)} + {reportPath ? ( + <> + {" "} + + + ) : null} +

+ ) : null} +
); } diff --git a/apps/desktop/src/renderer/components/settings/settingsManifest.ts b/apps/desktop/src/renderer/components/settings/settingsManifest.ts index 594584062..52dc49786 100644 --- a/apps/desktop/src/renderer/components/settings/settingsManifest.ts +++ b/apps/desktop/src/renderer/components/settings/settingsManifest.ts @@ -167,7 +167,7 @@ export const SETTINGS_ENTRIES: readonly SettingEntry[] = [ // and two identically named search hits pointing at different tabs is a // coin flip for whoever is looking for the off switch. label: "Diagnostics sharing", - keywords: ["diagnostics", "crash", "report", "privacy", "error", "send", "opt out"], + keywords: ["diagnostics", "crash", "report", "report issue", "privacy", "error", "send", "opt out"], tab: "general", anchor: "diagnostics-sharing", scope: "machine", diff --git a/apps/desktop/src/renderer/components/settings/settingsSectionUi.tsx b/apps/desktop/src/renderer/components/settings/settingsSectionUi.tsx index fb3df58c0..92b7ac6c6 100644 --- a/apps/desktop/src/renderer/components/settings/settingsSectionUi.tsx +++ b/apps/desktop/src/renderer/components/settings/settingsSectionUi.tsx @@ -105,6 +105,7 @@ export function ConsentToggleSection({ write, readErrorMessage, writeErrorMessage, + children, }: { id: string; title: string; @@ -121,6 +122,13 @@ export function ConsentToggleSection({ write: ((enabled: boolean) => Promise) | undefined; readErrorMessage: string; writeErrorMessage: string; + /** + * An action that belongs to this consent, rendered below the switch and its + * copy — the diagnostics section's "Send a report to ADE". Given the live + * status, because whether the toggle is on changes what the action has to + * say for itself. + */ + children?: (status: TStatus | null) => React.ReactNode; }) { const toggleId = useId(); const [status, setStatus] = useState(null); @@ -202,6 +210,17 @@ export function ConsentToggleSection({ onChange={(enabled) => void setEnabled(enabled)} /> + {children ? ( +
+ {children(status)} +
+ ) : null} ); diff --git a/apps/desktop/src/shared/diagnosticsUpload.test.ts b/apps/desktop/src/shared/diagnosticsUpload.test.ts index 3e5c14ef7..6b33a7921 100644 --- a/apps/desktop/src/shared/diagnosticsUpload.test.ts +++ b/apps/desktop/src/shared/diagnosticsUpload.test.ts @@ -154,6 +154,28 @@ describe("uploadDiagnosticReport", () => { .resolves.toEqual({ ok: false, reason: "network" }); }); + it("tells the two 429s apart, because they are two different sentences", async () => { + // The route answers a per-caller 429 and a fleet-wide 429 with distinct + // bodies on purpose. Reading them as one would tell a user "you've sent + // several today" when the truth is that ADE stopped taking reports from + // everyone — advice that cannot help them. + const perCaller = capture(new Response(JSON.stringify({ error: "rate limited" }), { status: 429 })); + await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl: perCaller.fetchImpl })) + .resolves.toEqual({ ok: false, reason: "rate_limited" }); + + const fleet = capture( + new Response(JSON.stringify({ error: "daily diagnostics budget exhausted" }), { status: 429 }), + ); + await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl: fleet.fetchImpl })) + .resolves.toEqual({ ok: false, reason: "unavailable" }); + + // A body that cannot be read falls back to the caller-scoped reading: the + // conservative one, since it only ever asks the user to wait. + const unreadable = capture(new Response(null, { status: 429 })); + await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl: unreadable.fetchImpl })) + .resolves.toEqual({ ok: false, reason: "rate_limited" }); + }); + it("treats an unusable success body as a failure rather than inventing a reference", async () => { const { fetchImpl } = capture(new Response("not json", { status: 200 })); await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl })) diff --git a/apps/desktop/src/shared/diagnosticsUpload.ts b/apps/desktop/src/shared/diagnosticsUpload.ts index a727fc971..05b8b2bac 100644 --- a/apps/desktop/src/shared/diagnosticsUpload.ts +++ b/apps/desktop/src/shared/diagnosticsUpload.ts @@ -54,11 +54,27 @@ const DEFAULT_UPLOAD_TIMEOUT_MS = 20_000; */ export type DiagnosticUploadFailure = | "too_large" + /** THIS caller has stored its allowance of reports today. */ | "rate_limited" + /** Nothing to do with this caller: the fleet's day is spent, or the route is down. */ | "unavailable" | "rejected" | "network"; +/** + * The fleet-wide 429's body, mirrored from `apps/account-directory/src/diagnostics.ts`. + * + * The route answers two DISTINCT 429s on purpose — one about the caller's own + * quota, one about the whole fleet's daily ceiling — because only the first is + * something the caller can do anything about. Telling someone "you have sent + * several today" when in fact ADE stopped taking reports from everyone is a + * lie, and the wire already carries enough to avoid it. + * + * A literal rather than an import: the Worker is a separate deploy unit and this + * module is loaded by the renderer, main and the CLI. Change one, change both. + */ +const FLEET_BUDGET_EXHAUSTED_ERROR = "daily diagnostics budget exhausted"; + export type DiagnosticUploadResult = | { ok: true; id: string; reference: string } | { ok: false; reason: DiagnosticUploadFailure }; @@ -182,7 +198,21 @@ export async function uploadDiagnosticReport( } if (response.status === 413) return { ok: false, reason: "too_large" }; - if (response.status === 429) return { ok: false, reason: "rate_limited" }; + if (response.status === 429) { + // Which 429 is it? A body we cannot read falls back to the caller-scoped + // reading, which is what this returned before there were two of them — + // the conservative answer, since it only ever asks the user to wait. + let body = ""; + try { + body = await response.text(); + } catch { + body = ""; + } + return { + ok: false, + reason: body.includes(FLEET_BUDGET_EXHAUSTED_ERROR) ? "unavailable" : "rate_limited", + }; + } if (response.status === 503) return { ok: false, reason: "unavailable" }; if (!response.ok) return { ok: false, reason: "rejected" }; diff --git a/apps/desktop/src/shared/ipc.ts b/apps/desktop/src/shared/ipc.ts index 8fe118bfc..b4b375ce1 100644 --- a/apps/desktop/src/shared/ipc.ts +++ b/apps/desktop/src/shared/ipc.ts @@ -83,6 +83,13 @@ export const IPC = { diagnosticsOpenIssue: "ade.diagnostics.openIssue", /** Renderer-detected failure asking main to consider one automatic send. */ diagnosticsAutoReport: "ade.diagnostics.autoReport", + /** + * "Send a report to ADE", pressed by hand in Settings, with nothing visibly + * broken. Main owns the report, the per-device manual budget and the upload; + * unlike the automatic path it answers with what happened, because someone is + * watching. + */ + diagnosticsSendManual: "ade.diagnostics.sendManual", /** Read the "share diagnostics automatically" setting. */ diagnosticsGetSharing: "ade.diagnostics.getSharing", /** Flip that setting; also what the toast's "Turn off" action calls. */ diff --git a/apps/desktop/src/shared/types/diagnostics.ts b/apps/desktop/src/shared/types/diagnostics.ts index f83817ed3..167187222 100644 --- a/apps/desktop/src/shared/types/diagnostics.ts +++ b/apps/desktop/src/shared/types/diagnostics.ts @@ -14,6 +14,8 @@ export type DiagnosticSurface = | "update_transaction" | "brain_repair" | "connections" + /** A person pressed "Send a report" in Settings, with nothing visibly broken. */ + | "settings_manual" | (string & {}); export type DiagnosticReportRequestPayload = { @@ -39,8 +41,47 @@ export type DiagnosticsSharingStatus = { sendsInWindow: number; /** The daily ceiling those are counted against. */ limit: number; + /** Reports the user asked for by hand in the last 24 hours. Separate budget. */ + manualSendsInWindow?: number; + /** The daily ceiling THOSE are counted against. */ + manualLimit?: number; }; +/** + * The answer to "Send a report" in Settings. + * + * A manual send is the one diagnostics path that must never fail silently: the + * user asked for it and is watching. So every outcome is named, and the surface + * turns each into one plain sentence — never a status code. + * + * `local_limit` is this computer's own guard (see + * `MAX_MANUAL_DIAGNOSTICS_PER_WINDOW`); `rate_limited` is the server saying + * this caller has stored its allowance; `unavailable` is the server saying it + * is not taking reports from anyone right now. They are three different + * sentences because they are three different situations. + */ +export type DiagnosticsManualSendResult = + | { + ok: true; + /** Short handle to read back to support. */ + reference: string; + /** Saved report path; empty when the local copy could not be written. */ + reportPath: string; + } + | { + ok: false; + reason: + | "local_limit" + | "rate_limited" + | "unavailable" + | "too_large" + | "failed"; + /** The local ceiling, so the refusal can name the real number. */ + limit?: number; + /** Saved report path when the report was built but not sent. */ + reportPath?: string; + }; + /** Main → renderer, once per automatic send. Codes and handles only. */ export type DiagnosticsAutoSentPayload = { failureCode: string; diff --git a/docs/features/onboarding-and-settings/README.md b/docs/features/onboarding-and-settings/README.md index fbab89434..d2a45afdf 100644 --- a/docs/features/onboarding-and-settings/README.md +++ b/docs/features/onboarding-and-settings/README.md @@ -394,7 +394,14 @@ Renderer — settings: send. `web: "hidden"` in the manifest, because the consent lives in `~/.ade/secrets/diagnostics-autosend.json` and a browser has no such file. Analytics consent and diagnostics consent are deliberately separate flags, so - turning one off never silently turns off the other. See + turning one off never silently turns off the other. The same card also carries + **Send a report to ADE** — the only report control that does not require + something to have already visibly broken, which is what makes this section's + own copy (*"the same report the Report issue button makes"*) true for a user + whose app merely feels wrong. It has its own daily cap (5 per install per 24h, + counted apart from the automatic 3), sends whether or not the toggle is on + because a deliberate click is not something ADE did by itself, and says so in + the card when the toggle is off. See [storage and recovery → Auto-send](../storage-and-recovery/README.md#auto-send). - `apps/desktop/src/renderer/components/settings/GitHubIntegrationSection.tsx` and `GitHubSection.tsx` — ADE GitHub App / environment / GitHub CLI / PAT diff --git a/docs/features/storage-and-recovery/README.md b/docs/features/storage-and-recovery/README.md index 904a171bf..9217ade54 100644 --- a/docs/features/storage-and-recovery/README.md +++ b/docs/features/storage-and-recovery/README.md @@ -602,19 +602,51 @@ carries only a short stub: GitHub rejects issue URLs somewhere north of 8 KB, so characters) and falls back to a title-and-stub URL past it. The full report rides the clipboard. +Those surfaces all share one property: something has already visibly broken. A +user whose app merely *feels* wrong has nothing to press, so **General → Privacy +→ Diagnostics sharing** also carries a **Send a report to ADE** button +(`renderer/components/settings/DiagnosticsSharingSection.tsx` → +`IPC.diagnosticsSendManual` → `autoDiagnosticsService.sendManual()`). It builds +and uploads the same redacted report the error screens do, under surface +`settings_manual` and `auto: false`, so a report somebody asked for stays +separable server-side from one nobody chose to file. It does not open GitHub: +the point is the send, and the result line offers **View report** for the saved +copy. + +Two things it deliberately does not share with the automatic sender: + +- **Its own budget.** Manual sends are capped at + `MAX_MANUAL_DIAGNOSTICS_PER_WINDOW` (5 per 24h per install), counted apart + from the automatic 3 — matching the account directory's per-identity daily + quota so the client never refuses a report the server would still have + accepted. Neither budget can spend the other: pressing the button cannot + silence the automatic reports that explain a crash, and a crash loop that has + burned its three automatic sends cannot lock a user out of asking for help. +- **Consent.** The toggle governs what ADE sends *by itself*; a deliberate click + is not that, so a manual send is allowed with the toggle off — and when it is + off, the pane says so next to the button, and the click never flips it back on. + +Refusals are three separate sentences, because they are three separate +situations: the local cap ("you've already sent 5 from this computer today"), +the account directory's per-caller 429 ("you've already sent several today"), +and its fleet-wide 429 or 503 ("ADE isn't accepting reports right now"). The +route answers the two 429s with distinct bodies precisely so a client can tell +them apart; `uploadDiagnosticReport` reads the body and maps the fleet one to +`unavailable`. No status code ever reaches the screen. + | Piece | Where | | --- | --- | | Pure builder + redactor | `apps/ade-cli/src/services/diagnostics/diagnosticReport.ts` | | Shared collection (logs, disk, notes, redaction context) | `apps/ade-cli/src/services/diagnostics/diagnosticSources.ts` (`collectMachineDiagnosticSources`) | | Desktop-only extras (its own jsonl logs, runtime status, recovery diagnosis, typed last-failure store) | `apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts` | -| IPC | `IPC.diagnosticsOpenIssue` | +| IPC | `IPC.diagnosticsOpenIssue`; manual send from Settings: `IPC.diagnosticsSendManual` | | Saved report | `/diagnostic-reports/-.md`, mode `0600` | | Headless equivalent | `ade report-issue [--open] [--send]` | | Headless state check | `ade doctor` → the **Diagnostics sharing** row (consent + today's spend) | | Settings toggle | `general.diagnostics-sharing` (General → Privacy, `#diagnostics-sharing`, default **on**, hidden on hosted web) | | Automatic sending (desktop) | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts` | | Automatic sending (brain) | `apps/ade-cli/src/services/diagnostics/autoDiagnosticsSender.ts` | -| Consent flag + shared daily budget | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts` → `~/.ade/secrets/diagnostics-autosend.json` | +| Consent flag + the two daily budgets (automatic and manual) | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts` → `~/.ade/secrets/diagnostics-autosend.json` | | Upload (opt-in) | `POST /diagnostics/upload` on the account directory Worker (`apps/account-directory/src/diagnostics.ts`); one client for both senders — the renderer button and the CLI — in `apps/desktop/src/shared/diagnosticsUpload.ts` | `ade report-issue` and the desktop button read the same machine sources through From 93de1e6d1552d2bb9c332fe3166d9b3b6b2f65b3 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:46:27 -0400 Subject: [PATCH 6/9] fix(diagnostics): collect the evidence that was already on the user's disk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user's app was misbehaving. We asked repeatedly for diagnostics and got nothing usable, then he pasted two log lines by hand that turned out to be decisive — and those lines were in a file the collector does not read. Every single thing we needed was already on his machine. Three sources were missing, all of them best-effort and all of them shared, so the desktop button and `ade report-issue --send` produce the same document. stdout. The collector read `launchd.err.log` only. Early-startup lines — `deeplink.scheme_claimed`, `deeplink.single_instance.lock_lost` — are written with `console.log` before the structured logger exists, so they land in `launchd.out.log` and nowhere else. Both streams are now collected, and the other two platforms are branched honestly rather than asked for a macOS path: Windows has one merged supervisor log by construction, and Linux keeps its output in journald, queried through an injectable runner and only when the systemd unit is actually installed. The service definition. Nothing recorded what the runtime was told to BE. A plist written without `ELECTRON_RUN_AS_NODE=1` boots the whole desktop app as the background service, which then claims the `ade://` scheme and fights the GUI for the single-instance lock — a failure with no signature in any log. The launchd plist, the systemd unit, and the Windows launcher plus its scheduled task XML now get their own section, read from the front (a plist states its Label and environment first) and capped at 8 KB. `main.jsonl` required an open project. It lives under the project root, so the machine-level error screens — the ones a person reaches when nothing will open — silently had no `main.jsonl` at all, and with it went the `ade_cli.auto_install` outcome. Both project logs are now collected for the open project, or for the most recently opened one when there is none, with a note saying which. The registry is read directly rather than through `ProjectRegistry`, which migrates a v1 file by writing it back and throws on a version it does not know; a collector running on a damaged machine may do neither. `ade report-issue --send` needs no arguments, no project and no cwd inside one. It saves the exact bytes it sends under `~/.ade/diagnostic-reports/` BEFORE attempting the upload, then prints the reference id and that path on success, or the reason in plain words and that path on failure — so a failed send leaves the user holding a file to attach instead of a sentence about a service they cannot reach. Size stays inside the 512 KB upload cap: the two project logs take a compact 80-line/16 KB tail rather than the full one, which is what buys room for stdout and the definition. A real report is ~92 KB; the theoretical worst case is ~208 KB of tails. Co-Authored-By: Claude Opus 5 --- apps/ade-cli/src/cli.ts | 9 +- apps/ade-cli/src/commands/reportIssue.test.ts | 88 ++++ apps/ade-cli/src/commands/reportIssue.ts | 87 +++- .../src/serviceManager/installLaunchd.ts | 14 +- .../src/serviceManager/installSystemd.ts | 8 +- .../src/serviceManager/installWindows.ts | 20 + .../services/diagnostics/diagnosticReport.ts | 83 +++- .../diagnostics/diagnosticSources.test.ts | 439 ++++++++++++++++++ .../services/diagnostics/diagnosticSources.ts | 378 ++++++++++++++- apps/desktop/src/main/main.ts | 1 - .../diagnosticReportService.test.ts | 47 ++ .../diagnostics/diagnosticReportService.ts | 22 +- .../src/main/services/ipc/registerIpc.ts | 2 - docs/features/storage-and-recovery/README.md | 55 ++- docs/logging.md | 2 + 15 files changed, 1170 insertions(+), 85 deletions(-) create mode 100644 apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts diff --git a/apps/ade-cli/src/cli.ts b/apps/ade-cli/src/cli.ts index 078f46f47..acef7ecd7 100644 --- a/apps/ade-cli/src/cli.ts +++ b/apps/ade-cli/src/cli.ts @@ -38,6 +38,7 @@ import { buildReportIssuePayload, describeDiagnosticUpload, openDiagnosticIssue, + saveDiagnosticReportCopy, sendDiagnosticReport, } from "./commands/reportIssue"; import { redactDiagnosticText } from "./services/diagnostics/diagnosticReport"; @@ -22623,12 +22624,16 @@ async function runCli( // opening it without copying first sends them to a form with nothing to // paste. Both steps are best effort and the report is printed regardless. const openedIssue = plan.open ? await openDiagnosticIssue(built) : null; + // Saved BEFORE the upload is attempted, so the path printed under a + // failure is a file that already exists rather than one written after we + // knew we needed it. + const savedPath = plan.send ? saveDiagnosticReportCopy(built, { surface: "cli" }) : null; // Sending is opt-in and never blocks the printed report: a failed upload // still leaves the user holding everything they need to file by hand. const sent = plan.send ? await sendDiagnosticReport(built) : null; if (parsed.options.text) { const clipboardNote = openedIssue?.copied ? "\n(the report is on your clipboard)" : ""; - const sendNote = sent ? `\n${describeDiagnosticUpload(sent)}` : ""; + const sendNote = sent ? `\n${describeDiagnosticUpload(sent, savedPath)}` : ""; return { output: `${built.report}\nFile the issue at:\n${built.issueUrl}${clipboardNote}${sendNote}\n`, exitCode: 0, @@ -22636,7 +22641,7 @@ async function runCli( } return { output: formatOutput( - buildReportIssuePayload(built, openedIssue, sent), + buildReportIssuePayload(built, openedIssue, sent, savedPath), parsed.options, undefined, ), diff --git a/apps/ade-cli/src/commands/reportIssue.test.ts b/apps/ade-cli/src/commands/reportIssue.test.ts index 4e447f8e2..9a2306e1b 100644 --- a/apps/ade-cli/src/commands/reportIssue.test.ts +++ b/apps/ade-cli/src/commands/reportIssue.test.ts @@ -2,11 +2,13 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { MAX_DIAGNOSTIC_UPLOAD_BYTES } from "../../../desktop/src/shared/diagnosticsUpload"; import { buildCliDiagnosticReport, buildReportIssuePayload, describeDiagnosticUpload, openDiagnosticIssue, + saveDiagnosticReportCopy, sendDiagnosticReport, } from "./reportIssue"; @@ -228,4 +230,90 @@ describe("sendDiagnosticReport", () => { expect(describeDiagnosticUpload({ ok: false, reason: "network" })) .toContain("couldn't reach"); }); + + // A send that failed used to end at "File it on GitHub instead", leaving the + // user with a wall of Markdown in a terminal and nothing to attach. Both + // outcomes now end somewhere they can go. + it("names the saved file under both a success and a failure", () => { + const saved = "/tmp/reports/2026-08-19-cli.md"; + + const sent = describeDiagnosticUpload({ ok: true, id: "abcdef1234", reference: "abcdef12" }, saved); + expect(sent).toContain("reference abcdef12"); + expect(sent).toContain(saved); + expect(sent).toContain("Exactly what was sent"); + + const failed = describeDiagnosticUpload({ ok: false, reason: "network" }, saved); + expect(failed).toContain("couldn't reach"); + expect(failed).toContain(saved); + + // And when even the local write failed, it says where the report *is* + // rather than pointing at a file that does not exist. + expect(describeDiagnosticUpload({ ok: false, reason: "network" }, null)) + .toContain("the full report is above"); + }); +}); + +describe("saveDiagnosticReportCopy", () => { + it("writes the exact report bytes owner-only, next to the automatic ones", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-report-save-")); + tempDirs.push(dir); + const reportsDir = path.join(dir, "diagnostic-reports"); + + const saved = saveDiagnosticReportCopy( + { report: "REPORT BODY", reportsDir }, + { surface: "cli", at: new Date("2026-08-19T12:00:00.000Z") }, + ); + + expect(saved).toBe(path.join(reportsDir, "2026-08-19T12-00-00-000Z-cli.md")); + expect(fs.readFileSync(saved!, "utf8")).toBe("REPORT BODY"); + if (process.platform !== "win32") { + expect(fs.statSync(saved!).mode & 0o777).toBe(0o600); + } + }); + + it("returns null rather than throwing when the report cannot be written", () => { + // Reporting a bug must never become a second bug: a read-only or full disk + // still leaves the printed report and the issue URL intact. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-report-save-")); + tempDirs.push(dir); + const blocked = path.join(dir, "blocked"); + fs.writeFileSync(blocked, "not a directory\n", "utf8"); + + expect(saveDiagnosticReportCopy({ report: "REPORT BODY", reportsDir: blocked })).toBeNull(); + }); +}); + +describe("buildCliDiagnosticReport — completeness with no project open", () => { + // The whole point of the change: `ade report-issue --send` with no arguments, + // no project open and a cwd outside any project has to produce a COMPLETE + // report, because that is the state the machine is in when ADE will not start. + it("still carries the service definition and the last project's logs", () => { + const home = adeHome({ anonymousId: "anon-complete" }); + const projectRoot = path.join(home, "workspace", "photon"); + const logsDir = path.join(projectRoot, ".ade", "transcripts", "logs"); + fs.mkdirSync(logsDir, { recursive: true }); + fs.writeFileSync(path.join(logsDir, "main.jsonl"), '{"event":"ade_cli.auto_install"}\n', "utf8"); + fs.mkdirSync(path.join(home, "runtime"), { recursive: true }); + fs.writeFileSync( + path.join(home, "projects.json"), + JSON.stringify({ + version: 2, + projects: [{ rootPath: projectRoot, lastOpenedAt: 42, catalogVisibility: "recent" }], + }), + "utf8", + ); + + const built = buildCliDiagnosticReport({ env: { ADE_HOME: home }, projectRoot: null }); + + // No project was open... + expect(built.report).toContain("- Project: none"); + // ...and the report still has the machine-level events and the definition + // section, plus the note that explains where the project logs came from. + expect(built.report).toContain("ade_cli.auto_install"); + expect(built.report).toContain("## Background service definition"); + expect(built.report).toContain("no project was open"); + // `--send` posts this document; it has to fit through the upload cap. + expect(new TextEncoder().encode(built.report).byteLength) + .toBeLessThan(MAX_DIAGNOSTIC_UPLOAD_BYTES); + }); }); diff --git a/apps/ade-cli/src/commands/reportIssue.ts b/apps/ade-cli/src/commands/reportIssue.ts index 33cfb1393..a5d735bb4 100644 --- a/apps/ade-cli/src/commands/reportIssue.ts +++ b/apps/ade-cli/src/commands/reportIssue.ts @@ -4,6 +4,8 @@ import path from "node:path"; import { buildDiagnosticIssueUrl, buildDiagnosticReport, + diagnosticReportFilePath, + writeDiagnosticReportFile, } from "../services/diagnostics/diagnosticReport"; import { collectMachineDiagnosticSources, @@ -52,6 +54,11 @@ export type ReportIssueResult = { appVersion: string | null; /** Where this machine's account session lives, so `--send` can read a token. */ secretsDir: string; + /** + * `~/.ade/diagnostic-reports` — the same directory the brain's automatic + * sender saves to, so the desktop toast's "View" reaches a CLI report too. + */ + reportsDir: string; }; /** @@ -85,11 +92,7 @@ export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): Repo const projectRoot = options.projectRoot?.trim() || null; const surface = options.surface?.trim() || "cli"; - const sources = collectMachineDiagnosticSources({ - env, - projectRoot, - includeProjectCliLog: true, - }); + const sources = collectMachineDiagnosticSources({ env, projectRoot }); const installId = readInstallId(sources.layout.secretsDir) ?? "unknown"; const report = buildDiagnosticReport({ @@ -115,6 +118,7 @@ export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): Repo state: sources.state, storage: sources.storage, logs: sources.logs, + serviceDefinition: sources.serviceDefinition, notes: sources.notes, redaction: sources.redaction, }); @@ -124,6 +128,7 @@ export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): Repo installId, appVersion: options.cliVersion ?? null, secretsDir: sources.layout.secretsDir, + reportsDir: path.join(sources.layout.adeDir, "diagnostic-reports"), issueUrl: buildDiagnosticIssueUrl({ surface, headline: options.headline?.trim().slice(0, 300) || null, @@ -235,19 +240,62 @@ export async function sendDiagnosticReport( }); } -/** One short line for `--text`, in the same register as the rest of the command. */ -export function describeDiagnosticUpload(result: DiagnosticUploadResult): string { - if (result.ok) return `Sent to ADE — reference ${result.reference}`; - switch (result.reason) { - case "rate_limited": - return "Not sent: you've already sent several reports today. Try again tomorrow."; - case "too_large": - return "Not sent: this report is too big to send. File it on GitHub instead."; - case "unavailable": - return "Not sent: ADE can't take reports right now. File it on GitHub instead."; - default: - return "Not sent: ADE couldn't reach the report service. File it on GitHub instead."; +/** + * Saves the exact bytes that were (or would have been) uploaded, next to the + * brain's automatic reports. + * + * `--send` writes one unconditionally, because both outcomes need a file: a + * successful send needs somewhere to point when the user asks "what did you + * just take from my machine", and a failed one needs to leave them holding + * something concrete instead of a sentence about a service they cannot reach. + * Best effort — a read-only or full disk must not turn reporting a bug into a + * second bug. + */ +export function saveDiagnosticReportCopy( + built: Pick, + args: { surface?: string; at?: Date } = {}, +): string | null { + const filePath = diagnosticReportFilePath( + built.reportsDir, + args.surface?.trim() || "cli", + args.at ?? new Date(), + ); + return writeDiagnosticReportFile(filePath, built.report) ? filePath : null; +} + +/** + * What `--send` prints, in the same register as the rest of the command. + * + * Both branches end somewhere the user can go. A success names the reference + * support will ask for AND the local copy of what was sent; a failure says why + * in plain words and names the file, because "couldn't send" with nothing + * attached is how a support thread turns into four more round trips. + */ +export function describeDiagnosticUpload( + result: DiagnosticUploadResult, + savedPath?: string | null, +): string { + if (result.ok) { + const sent = `Sent to ADE — reference ${result.reference}`; + return savedPath ? `${sent}\nExactly what was sent is saved at ${savedPath}` : sent; } + const reason = (() => { + switch (result.reason) { + case "rate_limited": + return "Not sent: you've already sent several reports today. Try again tomorrow."; + case "too_large": + return "Not sent: this report is too big to send."; + case "unavailable": + return "Not sent: ADE can't take reports right now."; + case "rejected": + return "Not sent: the report service refused this report."; + default: + return "Not sent: ADE couldn't reach the report service."; + } + })(); + return savedPath + ? `${reason}\nThe report is saved at ${savedPath} — attach that file to a GitHub issue.` + : `${reason} File it on GitHub instead (the full report is above).`; } /** @@ -262,12 +310,14 @@ export function buildReportIssuePayload( built: Pick, side: { copied: boolean } | null, sent?: DiagnosticUploadResult | null, + savedPath?: string | null, ): { ok: true; installId: string; issueUrl: string; copied: boolean; report: string; + reportPath?: string; sent?: { ok: boolean; reference?: string; reason?: string }; } { return { @@ -276,6 +326,9 @@ export function buildReportIssuePayload( issueUrl: built.issueUrl, copied: side?.copied ?? false, report: built.report, + // The same file the text output names, so a script that wraps `--send` + // can attach it without re-serializing the report itself. + ...(savedPath ? { reportPath: savedPath } : {}), // Omitted entirely without `--send`, so a script can tell "not asked for" // from "asked for and failed". ...(sent diff --git a/apps/ade-cli/src/serviceManager/installLaunchd.ts b/apps/ade-cli/src/serviceManager/installLaunchd.ts index 28d27166b..0dbeec46a 100644 --- a/apps/ade-cli/src/serviceManager/installLaunchd.ts +++ b/apps/ade-cli/src/serviceManager/installLaunchd.ts @@ -95,8 +95,18 @@ function launchdPrintOutputText(result: ReturnType): st return ""; } -export function launchAgentPath(homeDir = os.homedir()): string { - return path.join(homeDir, "Library", "LaunchAgents", `${ADE_RUNTIME_SERVICE_NAME}.plist`); +/** + * `serviceName` is a parameter rather than only the module constant because + * `ADE_RUNTIME_SERVICE_NAME` is frozen from `process.env` at import time. + * Callers that resolve a channel from an environment they were handed — the + * diagnostic collector, and every test that points ADE at a temp home — would + * otherwise silently read the stable channel's plist. + */ +export function launchAgentPath( + homeDir = os.homedir(), + serviceName: string = ADE_RUNTIME_SERVICE_NAME, +): string { + return path.join(homeDir, "Library", "LaunchAgents", `${serviceName}.plist`); } export function isLaunchdPrintRunning(output: string): boolean { diff --git a/apps/ade-cli/src/serviceManager/installSystemd.ts b/apps/ade-cli/src/serviceManager/installSystemd.ts index 6dd4c2c82..66af0f379 100644 --- a/apps/ade-cli/src/serviceManager/installSystemd.ts +++ b/apps/ade-cli/src/serviceManager/installSystemd.ts @@ -44,8 +44,12 @@ type SystemdServiceManagerDeps = { sleep?: (ms: number) => Promise; }; -export function servicePath(homeDir = os.homedir()): string { - return path.join(homeDir, ".config", "systemd", "user", `${ADE_RUNTIME_SERVICE_NAME}.service`); +/** See `launchAgentPath` for why `serviceName` is a parameter. */ +export function servicePath( + homeDir = os.homedir(), + serviceName: string = ADE_RUNTIME_SERVICE_NAME, +): string { + return path.join(homeDir, ".config", "systemd", "user", `${serviceName}.service`); } function serviceUnitName(): string { diff --git a/apps/ade-cli/src/serviceManager/installWindows.ts b/apps/ade-cli/src/serviceManager/installWindows.ts index 606e7d340..60a4d8534 100644 --- a/apps/ade-cli/src/serviceManager/installWindows.ts +++ b/apps/ade-cli/src/serviceManager/installWindows.ts @@ -267,6 +267,26 @@ export function buildWindowsQueryTaskArgs( return ["-NoProfile", "-NonInteractive", "-Command", query]; } +/** + * The task's whole XML definition, for a diagnostic report. + * + * `schtasks /Query /XML` writes UTF-16 to stdout, which a UTF-8 read turns into + * NUL-interleaved garbage. `Export-ScheduledTask` through `[Console]::Out` + * emits ordinary text, the same way every other task query in this file does. + */ +export function buildWindowsExportTaskArgs( + taskName = resolveWindowsTaskName(), +): string[] { + const taskNameLiteral = powerShellSingleQuotedLiteral(taskName); + const query = [ + "$ErrorActionPreference = 'Stop'", + `try { $xml = Export-ScheduledTask -TaskPath '\\' -TaskName ${taskNameLiteral} -ErrorAction Stop } catch { [Console]::Error.Write($_.Exception.Message); exit ${TASK_NOT_FOUND_EXIT_CODE} }`, + `if ($null -eq $xml) { exit ${TASK_NOT_FOUND_EXIT_CODE} }`, + "[Console]::Out.Write($xml)", + ].join("; "); + return ["-NoProfile", "-NonInteractive", "-Command", query]; +} + /** Delimits the Execute/Arguments fields emitted by the task action query. */ export const WINDOWS_TASK_ACTION_FIELD_SEPARATOR = "\u001f"; diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts b/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts index 107287627..b38ce53ad 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts @@ -23,6 +23,34 @@ export const ISSUE_URL_MAX_LENGTH = 6_000; export const LOG_TAIL_MAX_LINES = 120; export const LOG_TAIL_MAX_BYTES = 32 * 1024; +/** + * The tighter cap for the SUPPORTING log tails — the project's `main.jsonl` + * and `ade-cli.jsonl`. + * + * Every source added here is weighed against `MAX_DIAGNOSTIC_UPLOAD_BYTES` + * (512 KB for the whole serialized upload, `apps/desktop/src/shared/diagnosticsUpload.ts`), + * because a report that grows past it is not sent at all — the exact failure + * this collector exists to prevent. At the full cap the desktop's seven tails + * alone would be 224 KB before a single JSON state blob; at this one the two + * project logs cost 32 KB instead of 64 KB, which is what buys room for the + * launchd stdout stream and the service definition. The machine-level + * streams — the service's own stdout/stderr and `brain.jsonl` — keep the full + * cap, because they are the ones that explain a startup that never got far + * enough to write anything else. + */ +export const LOG_TAIL_COMPACT_MAX_LINES = 80; +export const LOG_TAIL_COMPACT_MAX_BYTES = 16 * 1024; + +/** + * Service definitions are read from the FRONT, not tailed: a launchd plist + * states its `Label`, `ProgramArguments` and `EnvironmentVariables` at the top, + * and a Windows launcher script sets its environment and command before + * anything else. A tail of either would keep the part nobody needs. Real + * definitions are 1-2 KB; the cap only bounds the generated PowerShell + * launcher, whose first 8 KB still carry the whole environment block. + */ +export const SERVICE_DEFINITION_MAX_BYTES = 8 * 1024; + export type DiagnosticRedactionContext = { homeDir?: string | null; username?: string | null; @@ -90,6 +118,19 @@ export type DiagnosticReportInput = { }; storage?: readonly DiagnosticVolumeSpace[]; logs?: readonly DiagnosticLogTail[]; + /** + * How this machine's background service is DEFINED — the launchd plist, the + * systemd unit, the Windows launcher script and scheduled task. + * + * Its own section rather than another log, because it answers a different + * question: not "what did the runtime say" but "what was the runtime told to + * be". A plist written by an older install without `ELECTRON_RUN_AS_NODE=1` + * boots the whole desktop app as the background service, which then claims + * the `ade://` scheme and fights the real GUI for the single-instance lock — + * a failure with no signature in any log, and invisible until someone asks + * the user to read the file out loud. + */ + serviceDefinition?: readonly DiagnosticLogTail[]; /** Free-form operational notes, e.g. "doctor: not run". */ notes?: readonly string[]; redaction?: DiagnosticRedactionContext; @@ -392,6 +433,26 @@ function section(title: string, body: string | null): string | null { return `## ${title}\n\n${body.trim()}`; } +/** + * One captured file, rendered so an ABSENCE reads as loudly as a presence. + * + * A file that could not be read still gets its heading and its path with the + * reason underneath, because "we looked here and found nothing" and "we never + * looked" are different facts and only one of them is the reader's problem. + */ +function fileEntryBlock(entry: DiagnosticLogTail): string { + const heading = `### ${entry.label}\n\n\`${entry.path}\``; + if (entry.error) return `${heading}\n\n${entry.error}`; + const text = (entry.text ?? "").trim(); + if (!text) return `${heading}\n\n(empty)`; + return `${heading}\n\n${fence(text)}`; +} + +function fileEntriesSection(title: string, entries: readonly DiagnosticLogTail[]): string | null { + if (entries.length === 0) return null; + return section(title, entries.map(fileEntryBlock).join("\n\n")); +} + /** * Renders the Markdown report, then redacts the whole thing. Redaction runs on * the assembled text rather than per-field on purpose: a future section cannot @@ -469,23 +530,11 @@ export function buildDiagnosticReport(input: DiagnosticReportInput): string { ), ); - const logs = input.logs ?? []; - parts.push( - section( - "Logs", - logs.length - ? logs - .map((log) => { - const heading = `### ${log.label}\n\n\`${log.path}\``; - if (log.error) return `${heading}\n\n${log.error}`; - const text = (log.text ?? "").trim(); - if (!text) return `${heading}\n\n(empty)`; - return `${heading}\n\n${fence(text)}`; - }) - .join("\n\n") - : null, - ), - ); + // Before the logs: what the service was told to be explains what the logs + // then show, and it is short enough to read first. + parts.push(fileEntriesSection("Background service definition", input.serviceDefinition ?? [])); + + parts.push(fileEntriesSection("Logs", input.logs ?? [])); parts.push( section("Notes", (input.notes ?? []).length ? (input.notes ?? []).map((note) => `- ${note}`).join("\n") : null), diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts new file mode 100644 index 000000000..85b3e8dca --- /dev/null +++ b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts @@ -0,0 +1,439 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { buildDiagnosticReport } from "./diagnosticReport"; +import { + collectMachineDiagnosticSources, + readFileHead, + resolveMostRecentProjectRoot, + type DiagnosticCommandRunner, +} from "./diagnosticSources"; + +/** + * These cover the sources a report was missing when a user's decisive evidence + * turned out to be two lines in a file the collector never read: the background + * service's *stdout* stream, the service definition that says what the runtime + * was told to be, and the project logs — which used to require a project to be + * open on a machine where nothing opens. + */ + +const tempDirs: string[] = []; + +function tempDir(prefix: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +/** A machine home with a runtime directory, and nothing else by default. */ +function machineHome(): { home: string; adeDir: string; runtimeDir: string } { + const home = tempDir("ade-diag-home-"); + const adeDir = path.join(home, ".ade"); + const runtimeDir = path.join(adeDir, "runtime"); + fs.mkdirSync(runtimeDir, { recursive: true }); + return { home, adeDir, runtimeDir }; +} + +/** A project with the two logs a report cares about. */ +function project(home: string, name: string, logs: Record = {}): string { + const root = path.join(home, name); + const logsDir = path.join(root, ".ade", "transcripts", "logs"); + fs.mkdirSync(logsDir, { recursive: true }); + for (const [file, contents] of Object.entries(logs)) { + fs.writeFileSync(path.join(logsDir, file), contents, "utf8"); + } + return root; +} + +/** The unit file whose presence gates the journald query. */ +function writeSystemdUnit(home: string, contents = "[Service]\nExecStart=/usr/bin/ade serve\n"): string { + const unitDir = path.join(home, ".config", "systemd", "user"); + fs.mkdirSync(unitDir, { recursive: true }); + const unitPath = path.join(unitDir, "com.ade.runtime.service"); + fs.writeFileSync(unitPath, contents, "utf8"); + return unitPath; +} + +function writeRegistry(adeDir: string, projects: unknown[]): void { + fs.mkdirSync(adeDir, { recursive: true }); + fs.writeFileSync( + path.join(adeDir, "projects.json"), + JSON.stringify({ version: 2, projects }, null, 2), + "utf8", + ); +} + +/** Refuses to run anything: proves a path does not depend on a subprocess. */ +const noCommands: DiagnosticCommandRunner = () => null; + +function collect(args: { + home: string; + platform?: NodeJS.Platform; + projectRoot?: string | null; + runCommand?: DiagnosticCommandRunner; +}) { + return collectMachineDiagnosticSources({ + env: { ADE_HOME: path.join(args.home, ".ade") }, + homeDir: args.home, + platform: args.platform ?? "darwin", + projectRoot: args.projectRoot ?? null, + runCommand: args.runCommand ?? noCommands, + // statfs on a temp dir is real and slow-ish; the figures are not under test. + readVolume: () => null, + }); +} + +describe("collectMachineDiagnosticSources — service output streams", () => { + it("collects launchd stdout as well as stderr", () => { + // The regression this exists for: the early-startup lines that name which + // process ADE actually booted are written with `console.log` before the + // structured logger exists, so they land in stdout and NOWHERE else. + const { home, runtimeDir } = machineHome(); + fs.writeFileSync(path.join(runtimeDir, "launchd.err.log"), "boom\n", "utf8"); + fs.writeFileSync( + path.join(runtimeDir, "launchd.out.log"), + "[main] deeplink.scheme_claimed\n[main] deeplink.single_instance.lock_lost\n", + "utf8", + ); + + const { logs } = collect({ home }); + const stdout = logs.find((log) => log.label === "Background service (stdout)"); + + expect(stdout?.text).toContain("deeplink.single_instance.lock_lost"); + expect(logs.find((log) => log.label === "Background service (stderr)")?.text).toContain("boom"); + }); + + it("notes an absent stdout stream instead of failing", () => { + const { home, runtimeDir } = machineHome(); + fs.writeFileSync(path.join(runtimeDir, "launchd.err.log"), "boom\n", "utf8"); + + const stdout = collect({ home }).logs.find( + (log) => log.label === "Background service (stdout)", + ); + + expect(stdout?.error).toBe("(not present)"); + expect(stdout?.text).toBeUndefined(); + }); + + it("reads the Windows supervisor log, which already merges both streams", () => { + const { home } = machineHome(); + + const { logs } = collect({ home, platform: "win32" }); + + expect(logs.map((log) => log.label)).toContain("Background service supervisor"); + // No launchd paths on a platform that has no launchd: a report claiming to + // have looked at `launchd.err.log` on Windows is worse than one that did not. + expect(logs.some((log) => log.path.includes("launchd"))).toBe(false); + }); + + it("asks journald for the service's output on Linux, where there is no file", () => { + const { home } = machineHome(); + writeSystemdUnit(home); + const run = vi.fn((_command: string, _args: readonly string[]) => ({ + status: 0, + stdout: "Aug 19 12:00:00 host ade[1]: brain.started\n", + })); + + const journal = collect({ home, platform: "linux", runCommand: run }).logs.find( + (log) => log.label === "Background service (journal)", + ); + + expect(journal?.text).toContain("brain.started"); + expect(run).toHaveBeenCalledWith( + "journalctl", + expect.arrayContaining(["--user-unit", "com.ade.runtime.service", "--no-pager"]), + ); + }); + + it("degrades to a noted absence when journalctl is not installed", () => { + const { home } = machineHome(); + writeSystemdUnit(home); + + const journal = collect({ home, platform: "linux", runCommand: () => null }).logs.find( + (log) => log.label === "Background service (journal)", + ); + + expect(journal?.error).toBe("(could not be read)"); + }); + + it("does not spawn journalctl at all when no unit is installed", () => { + const { home } = machineHome(); + const run = vi.fn((_command: string, _args: readonly string[]) => ({ status: 0, stdout: "" })); + + const journal = collect({ home, platform: "linux", runCommand: run }).logs.find( + (log) => log.label === "Background service (journal)", + ); + + expect(run).not.toHaveBeenCalled(); + expect(journal?.error).toBe("(not present)"); + }); +}); + +describe("collectMachineDiagnosticSources — service definition", () => { + it("collects the launchd plist, from the front", () => { + const { home } = machineHome(); + const agents = path.join(home, "Library", "LaunchAgents"); + fs.mkdirSync(agents, { recursive: true }); + fs.writeFileSync( + path.join(agents, "com.ade.runtime.plist"), + "ELECTRON_RUN_AS_NODE1\n", + "utf8", + ); + + const definition = collect({ home }).serviceDefinition; + + expect(definition).toHaveLength(1); + expect(definition[0]?.label).toBe("launchd agent"); + // The whole reason this section exists: a plist written without it boots the + // desktop app as the background service. + expect(definition[0]?.text).toContain("ELECTRON_RUN_AS_NODE"); + }); + + it("follows the channel's service name rather than the frozen default", () => { + const { home } = machineHome(); + const agents = path.join(home, "Library", "LaunchAgents"); + fs.mkdirSync(agents, { recursive: true }); + fs.writeFileSync(path.join(agents, "com.ade.runtime.beta.plist"), "\n", "utf8"); + + const definition = collectMachineDiagnosticSources({ + env: { ADE_HOME: path.join(home, ".ade"), ADE_PACKAGE_CHANNEL: "beta" }, + homeDir: home, + platform: "darwin", + runCommand: noCommands, + readVolume: () => null, + }).serviceDefinition; + + expect(definition[0]?.path).toContain("com.ade.runtime.beta.plist"); + expect(definition[0]?.text).toContain(""); + }); + + it("notes a missing plist rather than throwing", () => { + const { home } = machineHome(); + + const definition = collect({ home }).serviceDefinition; + + expect(definition[0]?.error).toBe("(not present)"); + }); + + it("reads the systemd unit on Linux", () => { + const { home } = machineHome(); + writeSystemdUnit(home); + + const definition = collect({ home, platform: "linux" }).serviceDefinition; + + expect(definition[0]?.label).toBe("systemd user unit"); + expect(definition[0]?.text).toContain("ExecStart="); + }); + + it("reads both halves of the Windows definition — launcher and scheduled task", () => { + const { home } = machineHome(); + const run = vi.fn((_command: string, _args: readonly string[]) => ({ + status: 0, + stdout: "powershell.exe", + })); + + const definition = collect({ home, platform: "win32", runCommand: run }).serviceDefinition; + + expect(definition.map((entry) => entry.label)).toEqual([ + "Background service launcher", + "Scheduled task", + ]); + expect(definition[1]?.text).toContain("powershell.exe"); + }); + + it("notes an unreadable scheduled task without failing the report", () => { + const { home } = machineHome(); + + const definition = collect({ home, platform: "win32", runCommand: () => null }).serviceDefinition; + + expect(definition[1]?.error).toBe("(could not be read)"); + expect(definition[0]?.error).toBe("(not present)"); + }); + + it("marks a definition that was truncated, so a short read is not read as a short file", () => { + const dir = tempDir("ade-diag-head-"); + const file = path.join(dir, "big.ps1"); + fs.writeFileSync(file, "x".repeat(200), "utf8"); + + const entry = readFileHead("launcher", file, 50); + + expect(entry.text?.startsWith("x".repeat(50))).toBe(true); + expect(entry.text).toContain("truncated: 200 bytes on disk"); + }); +}); + +describe("collectMachineDiagnosticSources — project logs with no project open", () => { + it("falls back to the most recently opened project and says that it did", () => { + const { home, adeDir } = machineHome(); + const stale = project(home, "stale", { "main.jsonl": '{"event":"old"}\n' }); + const recent = project(home, "recent", { + "main.jsonl": '{"event":"ade_cli.auto_install"}\n', + "ade-cli.jsonl": '{"event":"cli.started"}\n', + }); + writeRegistry(adeDir, [ + { rootPath: stale, lastOpenedAt: 1_000, catalogVisibility: "recent" }, + { rootPath: recent, lastOpenedAt: 9_000, catalogVisibility: "recent" }, + ]); + + const sources = collect({ home, projectRoot: null }); + + expect(sources.projectRoot).toBe(recent); + expect(sources.projectRootIsFallback).toBe(true); + // The machine-level event that was unreachable without a project open. + expect(sources.logs.find((log) => log.label === "Desktop main")?.text).toContain( + "ade_cli.auto_install", + ); + expect(sources.logs.find((log) => log.label === "ADE CLI")?.text).toContain("cli.started"); + // An absence the reader was never told about is worse than no fallback. + expect(sources.notes.join("\n")).toContain("no project was open"); + }); + + it("prefers the open project and does not mark it as a fallback", () => { + const { home, adeDir } = machineHome(); + const other = project(home, "other", { "main.jsonl": '{"event":"other"}\n' }); + const open = project(home, "open", { "main.jsonl": '{"event":"open"}\n' }); + writeRegistry(adeDir, [{ rootPath: other, lastOpenedAt: 9_000, catalogVisibility: "recent" }]); + + const sources = collect({ home, projectRoot: open }); + + expect(sources.projectRoot).toBe(open); + expect(sources.projectRootIsFallback).toBe(false); + expect(sources.logs.find((log) => log.label === "Desktop main")?.text).toContain('"open"'); + expect(sources.notes.join("\n")).not.toContain("no project was open"); + }); + + it("skips a registered project whose directory is gone", () => { + const { home, adeDir } = machineHome(); + const alive = project(home, "alive", { "main.jsonl": '{"event":"alive"}\n' }); + writeRegistry(adeDir, [ + { rootPath: path.join(home, "deleted"), lastOpenedAt: 9_999, catalogVisibility: "recent" }, + { rootPath: alive, lastOpenedAt: 1, catalogVisibility: "recent" }, + ]); + + expect(collect({ home }).projectRoot).toBe(alive); + }); + + it("says so when the machine has no project at all", () => { + const { home } = machineHome(); + + const sources = collect({ home }); + + expect(sources.projectRoot).toBeNull(); + expect(sources.notes.join("\n")).toContain("no project is registered"); + expect(sources.logs.map((log) => log.label)).not.toContain("Desktop main"); + }); + + it("never writes to or throws on a registry it cannot understand", () => { + // `ProjectRegistry` migrates a v1 file by writing it back and throws on an + // unknown version. A collector that runs on a damaged machine may do + // neither: the report is the last thing that still works there. + const { home, adeDir } = machineHome(); + const registryPath = path.join(adeDir, "projects.json"); + fs.mkdirSync(adeDir, { recursive: true }); + fs.writeFileSync(registryPath, '{"version":99,"projects":[', "utf8"); + const before = fs.readFileSync(registryPath, "utf8"); + + expect(() => collect({ home })).not.toThrow(); + expect(collect({ home }).projectRoot).toBeNull(); + expect(fs.readFileSync(registryPath, "utf8")).toBe(before); + }); + + it("treats a legacy v1 record with no visibility as usable", () => { + const { home, adeDir } = machineHome(); + const root = project(home, "legacy", { "main.jsonl": '{"event":"legacy"}\n' }); + fs.writeFileSync( + path.join(adeDir, "projects.json"), + JSON.stringify({ version: 1, projects: [{ rootPath: root }] }), + "utf8", + ); + + expect(resolveMostRecentProjectRoot(path.join(adeDir, "projects.json"))).toBe(root); + }); + + it("prefers a recent entry over a system one even when the system one is newer", () => { + const { home, adeDir } = machineHome(); + const system = project(home, "system", {}); + const recent = project(home, "recent", {}); + writeRegistry(adeDir, [ + { rootPath: system, lastOpenedAt: 9_999, catalogVisibility: "system" }, + { rootPath: recent, lastOpenedAt: 1, catalogVisibility: "recent" }, + ]); + + expect(resolveMostRecentProjectRoot(path.join(adeDir, "projects.json"))).toBe(recent); + }); +}); + +describe("the new sources go through redaction", () => { + it("redacts a token and the user's home out of the service definition", () => { + const { home } = machineHome(); + const agents = path.join(home, "Library", "LaunchAgents"); + fs.mkdirSync(agents, { recursive: true }); + // Assembled from segments so the working tree never carries a + // secret-shaped literal (same convention as diagnosticReport.test.ts). + const fakeKey = ["sk", "live", "abcdefghijklmnopqrstuvwxyz012345"].join("-"); + fs.writeFileSync( + path.join(agents, "com.ade.runtime.plist"), + [ + "", + "ANTHROPIC_API_KEY", + `${fakeKey}`, + "ADE_HOME", + `${path.join(home, ".ade")}`, + "", + ].join("\n"), + "utf8", + ); + const sources = collect({ home }); + + const report = buildDiagnosticReport({ + generatedAt: "2026-08-19T00:00:00.000Z", + app: { version: "1.2.61", platform: "darwin", arch: "arm64" }, + identity: { installId: "ade_test" }, + context: { surface: "cli" }, + serviceDefinition: sources.serviceDefinition, + logs: sources.logs, + notes: sources.notes, + redaction: { ...sources.redaction, homeDir: home, username: "ada" }, + }); + + expect(report).toContain("## Background service definition"); + expect(report).not.toContain(fakeKey); + expect(report).toContain(""); + expect(report).not.toContain(home); + }); + + it("redacts the fallback project's path out of the log tails it added", () => { + const { home, adeDir } = machineHome(); + const root = project(home, "photon", { + "main.jsonl": '{"event":"open_failed","projectRoot":"__ROOT__"}\n', + }); + fs.writeFileSync( + path.join(root, ".ade", "transcripts", "logs", "main.jsonl"), + `{"event":"open_failed","projectRoot":"${root}"}\n`, + "utf8", + ); + writeRegistry(adeDir, [{ rootPath: root, lastOpenedAt: 5, catalogVisibility: "recent" }]); + const sources = collect({ home }); + + const report = buildDiagnosticReport({ + generatedAt: "2026-08-19T00:00:00.000Z", + app: { version: "1.2.61", platform: "darwin", arch: "arm64" }, + identity: { installId: "ade_test" }, + // No project open: the fallback root reaches the report only through the + // collector's redaction context, so if that is not wired the project's + // absolute path ships in a document the user pastes into a public issue. + context: { surface: "cli", projectRoot: null }, + logs: sources.logs, + redaction: { ...sources.redaction, homeDir: home }, + }); + + expect(report).toContain("open_failed"); + expect(report).not.toContain(root); + expect(report).toContain(" readBytes; + return { + label, + path: filePath, + text: truncated ? `${text}\n… (truncated: ${stat.size} bytes on disk)` : text, + }; } finally { fs.closeSync(handle); } } catch (error) { - const code = error && typeof error === "object" && "code" in error - ? String((error as { code?: unknown }).code) - : ""; - return { label, path: filePath, error: code === "ENOENT" ? "(not present)" : "(could not be read)" }; + return { label, path: filePath, error: unreadableReason(error) }; } } +/** + * Reads a definition that is not a file — journald's copy of the service's + * output, a Windows scheduled task. Injected in tests; production shells out. + * + * Bounded and non-interactive by construction. This collector's whole premise + * is that it works on a machine where ADE does not, so a command that could + * block would defeat it. + */ +export type DiagnosticCommandRunner = ( + command: string, + args: readonly string[], +) => { status: number | null; stdout: string } | null; + +export function runDiagnosticCommand( + command: string, + args: readonly string[], +): { status: number | null; stdout: string } | null { + try { + const result = spawnSync(command, [...args], { + encoding: "utf8", + timeout: 4_000, + windowsHide: true, + stdio: ["ignore", "pipe", "pipe"], + maxBuffer: 2 * 1024 * 1024, + }); + if (result.error) return null; + return { status: result.status, stdout: typeof result.stdout === "string" ? result.stdout : "" }; + } catch { + return null; + } +} + +/** A command's output as a report entry, with the same absence semantics. */ +function readCommandOutput( + label: string, + display: string, + command: string, + args: readonly string[], + run: DiagnosticCommandRunner, + limits: { maxLines?: number; maxBytes?: number } = {}, +): DiagnosticLogTail { + const result = run(command, args); + if (!result) return { label, path: display, error: "(could not be read)" }; + if (result.status !== 0) return { label, path: display, error: "(not present)" }; + return { label, path: display, text: tailLogText(result.stdout, limits) }; +} + /** Parses a JSON file, or null for anything that is missing or malformed. */ export function readDiagnosticJsonFile(filePath: string): unknown { try { @@ -53,6 +164,61 @@ export function readDiagnosticJsonFile(filePath: string): unknown { } } +/** + * The most recently opened project on this machine, read straight out of + * `~/.ade/projects.json`. + * + * Deliberately NOT through `ProjectRegistry`: that class migrates a legacy v1 + * file by writing it back and throws on a version it does not know, and a + * diagnostic collector may do neither. It runs on a machine whose state is + * already suspect, it must never be the thing that mutates it, and a registry + * it cannot parse has to degrade to "no project" rather than take the report + * down with it. + * + * Entries are considered newest-first and the first one that still has a + * `.ade` directory wins, so a project the user deleted does not shadow the one + * they are actually working in. `recent` entries are preferred over the + * `system` ones ADE registers for itself, but a machine that only has `system` + * entries still gets project logs rather than none. + */ +export function resolveMostRecentProjectRoot(projectsPath: string): string | null { + const parsed = readDiagnosticJsonFile(projectsPath); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; + const projects = (parsed as { projects?: unknown }).projects; + if (!Array.isArray(projects)) return null; + + const candidates: { root: string; lastOpenedAt: number; recent: boolean }[] = []; + for (const entry of projects) { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) continue; + const record = entry as Record; + const root = typeof record.rootPath === "string" ? record.rootPath.trim() : ""; + if (!root) continue; + const lastOpenedAt = typeof record.lastOpenedAt === "number" && Number.isFinite(record.lastOpenedAt) + ? record.lastOpenedAt + : 0; + candidates.push({ + root, + lastOpenedAt, + // A v1 registry has no `catalogVisibility` at all; treating those as + // recent keeps a machine that has not been migrated from looking empty. + recent: record.catalogVisibility !== "system", + }); + } + candidates.sort((left, right) => { + if (left.recent !== right.recent) return left.recent ? -1 : 1; + return right.lastOpenedAt - left.lastOpenedAt; + }); + + for (const candidate of candidates) { + try { + if (fs.statSync(path.join(candidate.root, ".ade")).isDirectory()) return candidate.root; + } catch { + // Deleted, unmounted, or unreadable: try the next one. + } + } + return null; +} + /** Free space via `statfs`. The desktop passes its own Electron-aware reader. */ export function readVolumeViaStatfs(label: string, dirPath: string): DiagnosticVolumeSpace | null { try { @@ -100,14 +266,141 @@ export function diagnosticRedactionContext( }; } +/** Tail caps for the two supporting project logs. */ +const PROJECT_LOG_LIMITS = { + maxLines: LOG_TAIL_COMPACT_MAX_LINES, + maxBytes: LOG_TAIL_COMPACT_MAX_BYTES, +} as const; + +/** The project's own `.ade/transcripts/logs`, per `resolveAdeLayout`. */ +function projectLogsDir(projectRoot: string): string { + return path.join(projectRoot, ".ade", "transcripts", "logs"); +} + +/** + * How this machine's background service is defined, per platform. + * + * macOS and Linux keep it in one file. Windows keeps it in two — the generated + * PowerShell launcher holds the environment and the command, and the scheduled + * task holds what runs the launcher and as whom — so both are collected: a task + * pointing at a launcher from an older channel is exactly the kind of drift + * this section exists to make visible. + */ +function collectServiceDefinition(args: { + env: NodeJS.ProcessEnv; + platform: NodeJS.Platform; + homeDir: string; + run: DiagnosticCommandRunner; +}): DiagnosticLogTail[] { + const serviceName = resolveRuntimeServiceName(args.env); + + if (args.platform === "darwin") { + return [readFileHead("launchd agent", launchAgentPath(args.homeDir, serviceName))]; + } + if (args.platform === "win32") { + const entries = [ + readFileHead( + "Background service launcher", + resolveWindowsServiceLauncherPath({ env: args.env, serviceName }), + ), + ]; + // Both the task name (which folds in the Windows user) and locating + // PowerShell can throw on a broken box; neither may take the report with + // it, and the reader still has to be told we looked. + const task = (() => { + try { + const taskName = resolveWindowsTaskName({ serviceName }); + return { + taskName, + shell: windowsPowerShellCommand(), + args: buildWindowsExportTaskArgs(taskName), + }; + } catch { + return null; + } + })(); + entries.push( + task + ? readCommandOutput( + "Scheduled task", + `Export-ScheduledTask -TaskName "${task.taskName}"`, + task.shell, + task.args, + args.run, + { maxBytes: SERVICE_DEFINITION_MAX_BYTES }, + ) + : { + label: "Scheduled task", + path: "Export-ScheduledTask", + error: "(could not be read)", + }, + ); + return entries; + } + return [readFileHead("systemd user unit", systemdUnitPath(args.homeDir, serviceName))]; +} + +/** + * The service's own stdout and stderr — BOTH of them, wherever the platform + * keeps them. + * + * Collecting only stderr is how a user ended up reading two decisive lines off + * his own disk to us by hand: the early-startup lines that name the process + * ADE actually booted (`deeplink.scheme_claimed`, + * `deeplink.single_instance.lock_lost`) are written with `console.log` before + * the structured logger exists, so they land in stdout and nowhere else. + * + * The three platforms keep those streams in three different places: launchd + * splits them into two files, the Windows supervisor merges its own into one, + * and systemd hands them to journald, which is not a file at all. + */ +function collectServiceOutputLogs(args: { + env: NodeJS.ProcessEnv; + platform: NodeJS.Platform; + homeDir: string; + runtimeDir: string; + run: DiagnosticCommandRunner; +}): DiagnosticLogTail[] { + if (args.platform === "win32") { + // One merged stream by construction: the supervisor appends its own lines + // and the brain it starts inherits no redirection. + return [ + readLogTail("Background service supervisor", resolveWindowsSupervisorLogPath({ env: args.env })), + ]; + } + if (args.platform === "darwin") { + return [ + readLogTail("Background service (stderr)", path.join(args.runtimeDir, "launchd.err.log")), + readLogTail("Background service (stdout)", path.join(args.runtimeDir, "launchd.out.log")), + ]; + } + const serviceName = resolveRuntimeServiceName(args.env); + const unit = `${serviceName}.service`; + const display = `journalctl --user-unit ${unit}`; + // Gated on the unit file, so a machine that never installed the service is + // never charged a subprocess to be told nothing — the definition section + // already reports the missing unit, which is the more useful fact anyway. + if (!fs.existsSync(systemdUnitPath(args.homeDir, serviceName))) { + return [{ label: "Background service (journal)", path: display, error: "(not present)" }]; + } + return [ + readCommandOutput( + "Background service (journal)", + display, + "journalctl", + ["--user-unit", unit, "--no-pager", "--lines", "200"], + args.run, + ), + ]; +} + export type MachineDiagnosticSourceOptions = { env?: NodeJS.ProcessEnv; projectRoot?: string | null; - /** - * The CLI's own transcript log. The desktop has richer logs of its own and - * does not write this one. - */ - includeProjectCliLog?: boolean; + /** Test seams; production reads the real machine. */ + homeDir?: string; + platform?: NodeJS.Platform; + runCommand?: DiagnosticCommandRunner; /** Overridden by the desktop, which reads volumes through Electron's helper. */ readVolume?: (label: string, dirPath: string) => DiagnosticVolumeSpace | null; }; @@ -115,12 +408,23 @@ export type MachineDiagnosticSourceOptions = { export type MachineDiagnosticSources = { layout: ReturnType; logs: DiagnosticLogTail[]; + /** The launchd plist / systemd unit / Windows launcher + scheduled task. */ + serviceDefinition: DiagnosticLogTail[]; storage: DiagnosticVolumeSpace[]; state: { machineLastFailure: unknown; projectLastFailure: unknown; lastWedge: unknown; }; + /** + * The project whose logs and state this report carries: the caller's open + * project, else the most recently opened one on the machine. Callers that + * layer their own project-scoped sources on top must key them off THIS, or a + * report with no project open silently mixes two projects. + */ + projectRoot: string | null; + /** True when {@link projectRoot} is the fallback rather than an open project. */ + projectRootIsFallback: boolean; notes: string[]; redaction: DiagnosticRedactionContext; }; @@ -129,19 +433,31 @@ export function collectMachineDiagnosticSources( options: MachineDiagnosticSourceOptions = {}, ): MachineDiagnosticSources { const env = options.env ?? process.env; - const projectRoot = options.projectRoot?.trim() || null; + const platform = options.platform ?? process.platform; + const homeDir = options.homeDir ?? os.homedir(); + const run = options.runCommand ?? runDiagnosticCommand; + const openProjectRoot = options.projectRoot?.trim() || null; const layout = resolveMachineAdeLayout(env); const readVolume = options.readVolume ?? readVolumeViaStatfs; - const logs: DiagnosticLogTail[] = []; - if (process.platform === "win32") { - logs.push(readLogTail("Background service supervisor", resolveWindowsSupervisorLogPath({ env }))); - } else { - logs.push(readLogTail("Background service (stderr)", path.join(layout.runtimeDir, "launchd.err.log"))); - } - logs.push(readLogTail("Brain", path.join(layout.runtimeDir, "brain.jsonl"))); - if (options.includeProjectCliLog && projectRoot) { - logs.push(readLogTail("ADE CLI", path.join(projectRoot, ".ade", "transcripts", "logs", "ade-cli.jsonl"))); + // A report is worth least on the machine where nothing is open, which is + // exactly the machine that cannot open anything. `main.jsonl` carries the + // machine-level events — the CLI auto-install outcome among them — so a + // report that omits it because no window happens to be showing a project is + // missing evidence that was on disk the whole time. + const fallbackProjectRoot = openProjectRoot + ? null + : resolveMostRecentProjectRoot(layout.projectsPath); + const projectRoot = openProjectRoot ?? fallbackProjectRoot; + + const logs: DiagnosticLogTail[] = [ + ...collectServiceOutputLogs({ env, platform, homeDir, runtimeDir: layout.runtimeDir, run }), + readLogTail("Brain", path.join(layout.runtimeDir, "brain.jsonl")), + ]; + if (projectRoot) { + const logsDir = projectLogsDir(projectRoot); + logs.push(readLogTail("Desktop main", path.join(logsDir, "main.jsonl"), PROJECT_LOG_LIMITS)); + logs.push(readLogTail("ADE CLI", path.join(logsDir, "ade-cli.jsonl"), PROJECT_LOG_LIMITS)); } const storage = [ @@ -149,9 +465,19 @@ export function collectMachineDiagnosticSources( projectRoot ? readVolume("Project", projectRoot) : null, ].filter((entry): entry is DiagnosticVolumeSpace => entry != null); + const notes: string[] = [...DIAGNOSTIC_COLLECTION_NOTES]; + if (fallbackProjectRoot) { + notes.push( + `project logs: no project was open, so the most recently opened project on this machine was used (${projectPathLabel(fallbackProjectRoot)})`, + ); + } else if (!projectRoot) { + notes.push("project logs: no project is registered on this machine, so none were collected"); + } + return { layout, logs, + serviceDefinition: collectServiceDefinition({ env, platform, homeDir, run }), storage, state: { machineLastFailure: readDiagnosticJsonFile(path.join(layout.runtimeDir, "last-failure.json")), @@ -160,7 +486,9 @@ export function collectMachineDiagnosticSources( : null, lastWedge: readDiagnosticJsonFile(path.join(layout.runtimeDir, "last-wedge.json")), }, - notes: [...DIAGNOSTIC_COLLECTION_NOTES], + projectRoot, + projectRootIsFallback: fallbackProjectRoot != null, + notes, redaction: diagnosticRedactionContext(projectRoot), }; } diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 78d233a4d..dcbad71a9 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -2603,7 +2603,6 @@ app.whenReady().then(async () => { reportsDir: path.join(app.getPath("userData"), "diagnostic-reports"), installId: productAnalyticsService.getDistinctId(), accountUserId: readAccountOwnerId(), - projectLogsDir: projectRoot ? resolveAdeLayout(projectRoot).logsDir : null, getLocalRuntimeStatus: () => localRuntimePool.getStatus(), // Deliberately no `diagnoseProject`. The recovery diagnosis is itself // one of the triggers, so asking for a fresh one while building the diff --git a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts index 1e595d856..1a730ee67 100644 --- a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts +++ b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts @@ -139,6 +139,53 @@ describe("collectDiagnosticReport", () => { expect(report).not.toContain("requested project root was not recognised"); }); + + // A machine-scoped report is the one a user reaches when nothing will open, + // and it used to be the one missing `main.jsonl` — the desktop appended it + // only when a project happened to be open. The evidence was on disk the + // whole time. + it("carries the last project's main.jsonl when no project is open", async () => { + const adeHome = fs.mkdtempSync(path.join(tempRoot, "adeHome-")); + const projectRoot = fs.mkdtempSync(path.join(tempRoot, "photon-")); + const logsDir = path.join(projectRoot, ".ade", "transcripts", "logs"); + fs.mkdirSync(logsDir, { recursive: true }); + fs.writeFileSync(path.join(logsDir, "main.jsonl"), '{"event":"deeplink.scheme_claimed"}\n', "utf8"); + fs.writeFileSync(path.join(logsDir, "ade-cli.jsonl"), '{"event":"cli.started"}\n', "utf8"); + fs.writeFileSync( + path.join(adeHome, "projects.json"), + JSON.stringify({ + version: 2, + projects: [{ rootPath: projectRoot, lastOpenedAt: 7, catalogVisibility: "recent" }], + }), + "utf8", + ); + + const { report } = await collectDiagnosticReport( + { ...deps(), env: { ADE_HOME: adeHome } }, + { surface: "project_recovery", projectRoot: null }, + ); + + expect(report).toContain("### Desktop main"); + expect(report).toContain("deeplink.scheme_claimed"); + expect(report).toContain("### ADE CLI"); + expect(report).toContain("no project was open"); + // The project's absolute path must not ride along with its logs. + expect(report).not.toContain(projectRoot); + }); + + // The desktop and `ade report-issue` are meant to produce the same document; + // the section that says what the background service was told to be is part + // of it, present-or-noted, on every platform. + it("always carries a background service definition section", async () => { + const adeHome = fs.mkdtempSync(path.join(tempRoot, "adeHome-")); + + const { report } = await collectDiagnosticReport( + { ...deps(), env: { ADE_HOME: adeHome } }, + { surface: "project_recovery", projectRoot: null }, + ); + + expect(report).toContain("## Background service definition"); + }); }); describe("resolveRevealableDiagnosticReport", () => { diff --git a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts index 789fc4ccd..e69964164 100644 --- a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts +++ b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts @@ -88,8 +88,6 @@ export type DiagnosticReportDeps = { installId: string | null; /** Raw account user id; hashed here and never stored or sent verbatim. */ accountUserId?: string | null; - /** Project `.ade/logs` directory for the open project, when there is one. */ - projectLogsDir?: string | null; getLocalRuntimeStatus?: () => Promise | unknown; diagnoseProject?: (projectRoot: string) => Promise; /** Deadline for each optional step above. Test seam; defaults to 8s. */ @@ -196,12 +194,16 @@ export async function collectDiagnosticReport( : Promise.resolve(null), ]); + // Only what lives under Electron's userData is added here. The project's + // `main.jsonl` used to be appended at this point from an `app.getPath`-derived + // directory, which meant it was collected ONLY when a project was open — so a + // report from the machine-level error screens, the ones a person actually + // reaches when nothing will open, silently had no `main.jsonl` at all. + // `collectMachineDiagnosticSources` now owns it, for the open project or for + // the most recently opened one, and the CLI gets the same file. const logs: DiagnosticLogTail[] = [...sources.logs]; logs.push(readLogTail("Desktop local runtime", path.join(deps.userDataPath, "local-runtime.jsonl"))); logs.push(readLogTail("Desktop updates", path.join(deps.userDataPath, "ade-update.jsonl"))); - if (deps.projectLogsDir) { - logs.push(readLogTail("Desktop main", path.join(deps.projectLogsDir, "main.jsonl"))); - } // The typed store rather than the raw file the CLI falls back to: main owns // the writer, so it can read the record's real shape. @@ -212,10 +214,15 @@ export async function collectDiagnosticReport( return null; } })(); - const projectLastFailure = projectRoot + // Keyed off the root the shared collector actually used, not the request's: + // with no project open those differ, and reading the typed store for a root + // the logs above did not come from would attribute one project's last failure + // to another's evidence. + const collectedProjectRoot = sources.projectRoot; + const projectLastFailure = collectedProjectRoot ? (() => { try { - return readLastFailure({ kind: "project", projectRoot }); + return readLastFailure({ kind: "project", projectRoot: collectedProjectRoot }); } catch { return null; } @@ -261,6 +268,7 @@ export async function collectDiagnosticReport( }, storage: sources.storage, logs, + serviceDefinition: sources.serviceDefinition, notes: [...sources.notes, ...(request.extraNotes ?? [])], redaction, }); diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 9184aa982..a474fc536 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -809,7 +809,6 @@ import { quoteWindowsCmdArg } from "../shared/processExecution"; import { probeLocalhostPort } from "../probeLocalhostPort"; import type { ProcessRegistryService } from "../runtime/processRegistryService"; import { openExternalUrl } from "../shared/externalLinks"; -import { resolveAdeLayout } from "../../../shared/adeLayout"; import { collectDiagnosticReport, diagnosticReportRoots, @@ -4742,7 +4741,6 @@ export function registerIpc({ reportsDir: path.join(app.getPath("userData"), "diagnostic-reports"), installId: productAnalyticsService?.getDistinctId() ?? null, accountUserId: getCurrentAccountOwnerId?.() ?? null, - projectLogsDir: projectRoot ? resolveAdeLayout(projectRoot).logsDir : null, getLocalRuntimeStatus: () => localRuntimeConnectionPool?.getStatus() ?? null, diagnoseProject: projectRecoveryService ? (root: string) => projectRecoveryService.diagnose(root) diff --git a/docs/features/storage-and-recovery/README.md b/docs/features/storage-and-recovery/README.md index 9217ade54..fbc11412e 100644 --- a/docs/features/storage-and-recovery/README.md +++ b/docs/features/storage-and-recovery/README.md @@ -33,10 +33,10 @@ | `apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsx` | Full-project recovery surface for typed open failures, diagnosis, repair progress, next action, and technical details. `ProjectTabHost` in `App.tsx` renders it full-viewport whenever `projectTransitionError` carries a `code` and `rootPath`. | | `apps/desktop/src/renderer/components/app/ProjectTransitionErrorAlert.tsx` | Fallback dismissible banner for project open/switch failures that lack a code/rootPath (un-coded string errors); it renders nothing once a coded error hands the surface to `ProjectRecoveryScreen`. | | `apps/desktop/src/main/services/ipc/knownProjectRoots.ts` | Validation for renderer-supplied project roots on the recovery and diagnostics channels. A renderer may only name the open project, a local recent-projects entry, or a root main itself recently attempted to open; `AttemptedProjectRoots` is that last, bounded and expiring, single-writer registry, recorded only after the repo path resolves. Comparison goes through `pathsEqual` (case folding) and falls back to `path.resolve` when a root has no realpath, so a project on an unmounted volume is not refused. | -| `apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts` | Desktop half of **Report issue**: shared machine sources plus the desktop's own jsonl logs, local runtime status, the recovery diagnosis for the open project, the typed last-failure store, and an Electron-aware volume reader. Saves the report `0600`, copies it, and opens a prefilled GitHub issue. | +| `apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts` | Desktop half of **Report issue**: shared machine sources plus what only Electron can reach — its `userData` jsonl logs, local runtime status, the recovery diagnosis for the open project, the typed last-failure store, and an Electron-aware volume reader. Saves the report `0600`, copies it, and opens a prefilled GitHub issue. The typed last-failure store is keyed off the root the shared collector actually used, not the request's, so a report with no project open cannot attribute one project's failure to another's logs. | | `apps/ade-cli/src/services/diagnostics/diagnosticReport.ts` | The pure report builder, the redactor (`redactDiagnosticText`), and `buildDiagnosticIssueUrl`. No I/O, so both the desktop and the CLI produce byte-identical documents from the same sources. | -| `apps/ade-cli/src/services/diagnostics/diagnosticSources.ts` | `collectMachineDiagnosticSources` — the machine-level logs, layout, disk figures and redaction context both surfaces read, so a log added for one appears in both. | -| `apps/ade-cli/src/commands/reportIssue.ts` | `ade report-issue [--open] [--send]`, the headless equivalent. `--send` posts the same redacted report to ADE (Clerk token when the machine is signed in, anonymous otherwise) and prints a short reference id. Local files only: it never starts or contacts the brain, so it still works where ADE will not come up and on hosts with no error screen to press. | +| `apps/ade-cli/src/services/diagnostics/diagnosticSources.ts` | `collectMachineDiagnosticSources` — everything a headless box can read: both of the background service's output streams, the service definition (`readFileHead`), the project logs of the open **or most recently opened** project (`resolveMostRecentProjectRoot`), layout, disk figures and the redaction context. Both surfaces read it, so a source added for one appears in both. Command-backed sources (journald, `schtasks /XML`) go through the injectable `DiagnosticCommandRunner`, bounded and non-interactive. | +| `apps/ade-cli/src/commands/reportIssue.ts` | `ade report-issue [--open] [--send]`, the headless equivalent. `--send` needs no project and no arguments: it posts the same redacted report to ADE (Clerk token when the machine is signed in, anonymous otherwise), saves a copy under `~/.ade/diagnostic-reports/` *before* attempting the upload, and prints the reference id or the plain-words failure alongside that path. Local files only: it never starts or contacts the brain, so it still works where ADE will not come up and on hosts with no error screen to press. | | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts` | The consent flag **and** the spend ledger for automatic uploads, in one file (`/secrets/diagnostics-autosend.json`) that both senders open — the desktop main process and the brain — because "three a day from this computer" is a property of the install, not of a process, and two private ledgers would quietly mean six. Deliberately dependency-free (`node:fs`, no Electron, no logger) for exactly that reason. Owns `AUTO_DIAGNOSTICS_WINDOW_MS` (24 h), `MAX_AUTO_DIAGNOSTICS_PER_CODE` (1), `MAX_AUTO_DIAGNOSTICS_PER_WINDOW` (3), `normalizeAutoDiagnosticsFailureCode` (coerced to the Worker's `FAILURE_CODE_PATTERN`, re-exported rather than rewritten), the mkdir lock — whose `isLockContention` names the Windows delete-pending `EPERM`/`EACCES`/`EBUSY` window as well as `EEXIST` — and the pending-notice queue the toast acknowledgement retires. Consent defaults **on**; an unreadable or locked ledger fails closed. | | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsSend.ts` | `runAutoDiagnosticsSend` — the policy every automatic send obeys, written once. The two senders differ in exactly three things (what they build, how they upload, which analytics surface they report as) and bring those as structural seams; consent, the pre-request reservation, the local copy, silence on failure, the pending flag, the log lines and the analytics dedupe key live here. Also owns the `AutoDiagnosticsOutcome` vocabulary (`completed`, `skipped_disabled`, `skipped_budget`, `skipped_ineligible`, `failed`) and `AUTO_DIAGNOSTICS_ANALYTICS_DEDUPE_MS` (1 h). | | `apps/desktop/src/main/services/diagnostics/autoDiagnosticsService.ts` | The desktop sender: what is specific to this process — how a report gets built (no `diagnoseProject`, since a diagnosis is itself a trigger), that it uploads anonymously, the `onSent` fast path for an open window, and the getter/setter the Settings toggle reads and writes. | @@ -650,22 +650,57 @@ them apart; `uploadDiagnosticReport` reads the body and maps the fleet one to | Upload (opt-in) | `POST /diagnostics/upload` on the account directory Worker (`apps/account-directory/src/diagnostics.ts`); one client for both senders — the renderer button and the CLI — in `apps/desktop/src/shared/diagnosticsUpload.ts` | `ade report-issue` and the desktop button read the same machine sources through -`collectMachineDiagnosticSources`, so a log added for one appears in both; the -CLI adds the project's `ade-cli.jsonl` and the desktop adds its own jsonl logs -and Electron-aware volume reader on top. +`collectMachineDiagnosticSources`, so a source added for one appears in both. +The desktop adds only what lives under Electron's `userData` — its own +`local-runtime.jsonl` and `ade-update.jsonl`, the typed last-failure store, and +an Electron-aware volume reader. + +`ade report-issue --send` takes **no arguments and needs no project**: with +nothing open and a cwd outside every project it still produces a complete report +and uploads it. It also saves the exact bytes it sent to +`~/.ade/diagnostic-reports/`, and prints where. A successful send prints the +reference id plus that path ("exactly what was sent"); a failed one prints the +reason in plain words plus that path, so the user is left holding a file to +attach rather than a sentence about a service they cannot reach. Both appear in +`--json` as `reportPath` and `sent`. The report contains: app version/channel/packaging, platform, arch, OS release (plus `sw_vers -productVersion` on macOS), Electron/Node/Chrome versions, timezone offset, the surface and recovery code the user hit, the technical detail that screen already showed, the local runtime status snapshot, the machine and project `last-failure.json`, `last-wedge.json`, the recovery -diagnosis for the open project, free disk for the ADE home and the project, and -a bounded tail (120 lines / 32 KB each) of the background-service log -(`launchd.err.log`, or the Windows supervisor log), `brain.jsonl`, -`local-runtime.jsonl`, `ade-update.jsonl` and the project's `main.jsonl`. The +diagnosis for the open project, free disk for the ADE home and the project, the +background-service definition, and bounded tails of the logs below. The `ade doctor` checks are **not** run: the report must be collectable on a machine whose brain will not start. +**What is collected, and why each one.** Everything a headless box can read is +collected by `collectMachineDiagnosticSources`, so the desktop button and +`ade report-issue --send` produce the same document. + +| Source | Cap | Why | +| --- | --- | --- | +| Background service, **both** streams — `launchd.err.log` **and** `launchd.out.log` on macOS, the supervisor log on Windows (one merged stream by construction), `journalctl --user-unit` on Linux when the unit exists | 120 lines / 32 KB | Early-startup lines are written with `console.log` before the structured logger exists, so `deeplink.scheme_claimed` and `deeplink.single_instance.lock_lost` land in **stdout and nowhere else**. Collecting only stderr is how a user once had to read the decisive two lines off his own disk by hand. | +| `brain.jsonl` | 120 lines / 32 KB | The brain's own structured log. | +| `local-runtime.jsonl`, `ade-update.jsonl` (desktop only) | 120 lines / 32 KB | Under Electron's `userData`; the CLI does not write them. | +| The project's `main.jsonl` and `ade-cli.jsonl` | 80 lines / 16 KB | Machine-level events (the `ade_cli.auto_install` outcome among them) live here. Collected for the open project, or — when no project is open — for the most recently opened project in `~/.ade/projects.json`, with a note in the report saying which. | +| **Service definition**: the launchd plist, the systemd user unit, or the Windows launcher script plus its scheduled task XML | first 8 KB | Configuration, not logs, and read from the front because a plist states its `Label`, `ProgramArguments` and `EnvironmentVariables` first. A plist written without `ELECTRON_RUN_AS_NODE=1` boots the whole desktop app as the background service, which then claims the `ade://` scheme and fights the GUI for the single-instance lock — a failure with no signature in any log. | + +Two properties hold for every one of them. **Absence is a fact, not an error**: +a missing or unreadable source becomes `(not present)` / `(could not be read)` +under its own heading, never a thrown collector and never a failed upload — the +machine this runs on is by definition damaged. And the total stays inside +`MAX_DIAGNOSTIC_UPLOAD_BYTES` (512 KB for the serialized upload): at the caps +above a desktop report's tails are ~208 KB at their theoretical worst and a real +one is well under 100 KB, which is why the two project logs take the smaller cap +and the service definition is capped at all. + +`resolveMostRecentProjectRoot` reads `~/.ade/projects.json` directly rather than +through `ProjectRegistry`, which migrates a legacy v1 file by writing it back and +throws on a version it does not know. A diagnostic collector may do neither: it +runs on a machine whose state is already suspect, and a registry it cannot parse +has to degrade to "no project" rather than take the report down with it. + **Redaction guarantees.** `redactDiagnosticText` runs over the whole assembled document as the last step — not per field — so a section added later cannot leak by forgetting to opt in. It removes, in order: project roots (collapsed to diff --git a/docs/logging.md b/docs/logging.md index 166ba34ff..0673b9d0f 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -24,6 +24,8 @@ Operational logs use ADE's local logging services and may include bounded diagno The machine brain writes the same `{ts, level, event, meta}` JSONL format as the desktop logger to `~/.ade/runtime/brain.jsonl`, honoring `ADE_LOG_LEVEL` (default `info`). The file rotates at 10 MiB to `brain.1.jsonl`; warnings and errors are also mirrored to stderr with an ISO-8601 timestamp and uppercase level for launchd diagnostics. +Not everything reaches a structured logger. Lines written before one exists — `console.log("[main] …")` during early startup, anything the runtime prints on its way up — go to the background service's **stdout**, which on macOS is `~/.ade/runtime/launchd.out.log` and not the `launchd.err.log` that carries stderr. Both streams are bounded by `runtimeLogMaintenance.ts` and **both** are collected into a diagnostic report; see [Diagnostic reports](features/storage-and-recovery/README.md#diagnostic-reports-report-issue). A `console.log` at that stage is therefore diagnosable, but it is still a last resort: prefer a structured event as soon as the logger is available. + Writes are batched onto an async stream, so a caller that is about to end the process (`app.exit`, a force quit, an install handoff escalation) must call `logger.flushSync()` immediately after the line that matters — while it is still queued — or the records explaining the exit die with the process. `flushSync` drains only what is still queued (a batch already handed to an in-flight async flush is not duplicated), skips rotation deliberately, and, like every other log write, never throws. Not every operational log belongs to the active project. `createFileLogger` also backs machine-scoped sinks for facts that outlive or fall outside a project: `accountBridge` writes `account.local_machines_removed` to `/runtime/account-trust.jsonl`, because dropping a paired machine credential is a machine-level mutation and the project logger follows the active project — on a remote-bound project it would ship the record to the other machine and leave nothing on the machine that actually lost its trust. Account-directory publish outcomes record only bounded per-leg durations, the failing leg, and coarse failure codes such as `token_timeout` or `http_timeout`; they never include bearer tokens or response bodies. These high-frequency health events remain local operational logs and are not product analytics. From c45ff4e30741c5ba0bf35749f6658d340fe2e788 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:05:23 -0400 Subject: [PATCH 7/9] fix(logging): give the main process a log that exists before a project does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The desktop main process had no durable log until a project opened. Both `createFileLogger(main.jsonl)` calls live inside project-open paths and write to a PROJECT-scoped directory, so everything before that had nowhere to go — which is why the early-startup lines were `console.log("[main] …")`, a choice whose own comment said the structured logger may not be ready yet. Those lines survive only as process stdout, which exists for a launchd-spawned runtime and vanishes entirely for a Finder-launched app. Machine-level facts got filed under whichever project happened to open: whether this computer ever got the `ade` command was recorded per project, and on the dormant path it went to a `userData` log no report collects at all. A user whose app fails before opening a project produced a diagnostic report with no main-process log — which is what happened, and why we asked for evidence four times that did not exist. The previous commit made the collector fall back to the most recently opened project's `main.jsonl`. That is a mitigation: it still needs some project to have been opened, and to guess the right one. `machineLogger.ts` writes `~/.ade/runtime/desktop-main.jsonl` — resolved with `resolveMachineAdeLayout`, the same resolver `ade report-issue` uses, so a headless report on a machine where the desktop will not start finds it by construction and each channel's ADE_HOME keeps its own. `app.getPath("userData")` would be a per-platform, per-productName directory the CLI must guess, which is why `local-runtime.jsonl` and `ade-update.jsonl` stay desktop-only sources. It is opened in main.ts's first executable statement, before the `ade://` claim and the single-instance lock, and it reuses `createFileLogger` so the 10 MiB `.1` rotation that bounds `brain.jsonl` bounds this too. Moved to it, by subject rather than by wholesale migration — the computer, not a repository: `desktop.main_started`, the deeplink scheme/single-instance events, `app_navigation.queued_before_dispatcher_ready`, `app.hardware_acceleration`, `machine_trust_reset.failed`, and the CLI auto-install outcome. Project-subject events stay in the project log untouched. Auto-update events were already machine-scoped in `ade-update.jsonl`. Console output is kept as a second copy, not dropped: a terminal-launched app still shows these, and the pathological plist that boots the desktop app as the background service still routes them into `launchd.out.log`. The one branch that quits immediately flushes first, so `deeplink.single_instance.lock_lost` survives the exit. The shared collector picks the file up, so the desktop button, `ade report-issue --send` and the brain's automatic send all carry it. Full tail cap like the other machine-level streams: worst-case desktop tails go 208 KB → 240 KB against the 512 KB upload cap, and a real report measures ~92 KB. Co-Authored-By: Claude Opus 5 --- .../services/diagnostics/diagnosticReport.ts | 10 +- .../diagnostics/diagnosticSources.test.ts | 36 +++- .../services/diagnostics/diagnosticSources.ts | 28 ++- apps/desktop/src/main/main.ts | 73 +++++--- .../services/deeplinks/protocolHandler.ts | 8 + .../diagnosticReportService.test.ts | 26 ++- .../services/logging/machineLogger.test.ts | 161 ++++++++++++++++++ .../main/services/logging/machineLogger.ts | 128 ++++++++++++++ docs/ARCHITECTURE.md | 18 +- docs/features/deeplinks/README.md | 6 +- docs/features/storage-and-recovery/README.md | 10 +- docs/logging.md | 8 +- 12 files changed, 456 insertions(+), 56 deletions(-) create mode 100644 apps/desktop/src/main/services/logging/machineLogger.test.ts create mode 100644 apps/desktop/src/main/services/logging/machineLogger.ts diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts b/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts index b38ce53ad..ab93b6fb5 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticReport.ts @@ -30,13 +30,13 @@ export const LOG_TAIL_MAX_BYTES = 32 * 1024; * Every source added here is weighed against `MAX_DIAGNOSTIC_UPLOAD_BYTES` * (512 KB for the whole serialized upload, `apps/desktop/src/shared/diagnosticsUpload.ts`), * because a report that grows past it is not sent at all — the exact failure - * this collector exists to prevent. At the full cap the desktop's seven tails - * alone would be 224 KB before a single JSON state blob; at this one the two + * this collector exists to prevent. At the full cap the desktop's eight tails + * alone would be 256 KB before a single JSON state blob; at this one the two * project logs cost 32 KB instead of 64 KB, which is what buys room for the * launchd stdout stream and the service definition. The machine-level - * streams — the service's own stdout/stderr and `brain.jsonl` — keep the full - * cap, because they are the ones that explain a startup that never got far - * enough to write anything else. + * streams — the service's own stdout/stderr, `brain.jsonl`, and the desktop + * main process's `desktop-main.jsonl` — keep the full cap, because they are the + * ones that explain a startup that never got far enough to write anything else. */ export const LOG_TAIL_COMPACT_MAX_LINES = 80; export const LOG_TAIL_COMPACT_MAX_BYTES = 16 * 1024; diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts index 85b3e8dca..1cf38269b 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts @@ -285,7 +285,7 @@ describe("collectMachineDiagnosticSources — project logs with no project open" expect(sources.projectRoot).toBe(recent); expect(sources.projectRootIsFallback).toBe(true); // The machine-level event that was unreachable without a project open. - expect(sources.logs.find((log) => log.label === "Desktop main")?.text).toContain( + expect(sources.logs.find((log) => log.label === "Desktop main (project)")?.text).toContain( "ade_cli.auto_install", ); expect(sources.logs.find((log) => log.label === "ADE CLI")?.text).toContain("cli.started"); @@ -303,7 +303,7 @@ describe("collectMachineDiagnosticSources — project logs with no project open" expect(sources.projectRoot).toBe(open); expect(sources.projectRootIsFallback).toBe(false); - expect(sources.logs.find((log) => log.label === "Desktop main")?.text).toContain('"open"'); + expect(sources.logs.find((log) => log.label === "Desktop main (project)")?.text).toContain('"open"'); expect(sources.notes.join("\n")).not.toContain("no project was open"); }); @@ -325,7 +325,37 @@ describe("collectMachineDiagnosticSources — project logs with no project open" expect(sources.projectRoot).toBeNull(); expect(sources.notes.join("\n")).toContain("no project is registered"); - expect(sources.logs.map((log) => log.label)).not.toContain("Desktop main"); + expect(sources.logs.map((log) => log.label)).not.toContain("Desktop main (project)"); + }); + + // The fallback above is a mitigation: it still needs SOME project to have + // been opened, and to guess the right one. The machine log needs neither. + it("carries the desktop's machine log with no project on the machine at all", () => { + const { home, runtimeDir } = machineHome(); + fs.writeFileSync( + path.join(runtimeDir, "desktop-main.jsonl"), + '{"event":"desktop.main_started"}\n{"event":"ade_cli.auto_install"}\n', + "utf8", + ); + + const sources = collect({ home }); + + expect(sources.projectRoot).toBeNull(); + const machineLog = sources.logs.find((log) => log.label === "Desktop main (machine)"); + expect(machineLog?.text).toContain("desktop.main_started"); + // The machine-scoped fact that used to be filed under whichever project + // happened to open, and was therefore unreachable from a report like this. + expect(machineLog?.text).toContain("ade_cli.auto_install"); + }); + + it("notes an absent machine log rather than omitting the section", () => { + const { home } = machineHome(); + + const machineLog = collect({ home }).logs.find( + (log) => log.label === "Desktop main (machine)", + ); + + expect(machineLog?.error).toBe("(not present)"); }); it("never writes to or throws on a registry it cannot understand", () => { diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts b/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts index 9ffb1305a..4b61dac11 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts @@ -13,6 +13,7 @@ import { type DiagnosticVolumeSpace, } from "./diagnosticReport"; import { resolveMachineAdeLayout } from "../projects/machineLayout"; +import { MACHINE_MAIN_LOG_FILE_NAME } from "../../../../desktop/src/main/services/logging/machineLogger"; import { resolveRuntimeServiceName } from "../../serviceManager/common"; import { launchAgentPath } from "../../serviceManager/installLaunchd"; import { servicePath as systemdUnitPath } from "../../serviceManager/installSystemd"; @@ -35,9 +36,10 @@ import { * Electron-only inputs (`readVolumeSpace`, the desktop's own userData jsonl * logs, the typed last-failure store) stay in the desktop service and are * layered on top. Everything a headless box can read — both of the background - * service's output streams, its service definition, and the project logs of - * whichever project this machine used last — belongs HERE, so the desktop's - * button and `ade report-issue --send` produce the same document. + * service's output streams, its service definition, the desktop main process's + * machine log, and the project logs of whichever project this machine used + * last — belongs HERE, so the desktop's button and `ade report-issue --send` + * produce the same document. */ /** The reason a source is absent, in the two words the report renders. */ @@ -441,10 +443,11 @@ export function collectMachineDiagnosticSources( const readVolume = options.readVolume ?? readVolumeViaStatfs; // A report is worth least on the machine where nothing is open, which is - // exactly the machine that cannot open anything. `main.jsonl` carries the - // machine-level events — the CLI auto-install outcome among them — so a - // report that omits it because no window happens to be showing a project is - // missing evidence that was on disk the whole time. + // exactly the machine that cannot open anything. The project's `main.jsonl` + // is still collected — for the open project, or the most recently opened one + // — because a project that failed to open leaves its story there. But it can + // only ever be a fallback: it requires SOME project to have been opened at + // some point, and to guess the right one. const fallbackProjectRoot = openProjectRoot ? null : resolveMostRecentProjectRoot(layout.projectsPath); @@ -452,11 +455,20 @@ export function collectMachineDiagnosticSources( const logs: DiagnosticLogTail[] = [ ...collectServiceOutputLogs({ env, platform, homeDir, runtimeDir: layout.runtimeDir, run }), + // The desktop main process's own machine-scoped log: the launch marker, the + // `ade://` scheme claim, which process won the single-instance lock, the CLI + // auto-install outcome. Full tail cap, like the other machine-level streams + // beside it — these are the ones that explain a startup which never got far + // enough to write anything else, and on a machine where no project has ever + // been opened it is the ONLY main-process log there is. + readLogTail("Desktop main (machine)", path.join(layout.runtimeDir, MACHINE_MAIN_LOG_FILE_NAME)), readLogTail("Brain", path.join(layout.runtimeDir, "brain.jsonl")), ]; if (projectRoot) { const logsDir = projectLogsDir(projectRoot); - logs.push(readLogTail("Desktop main", path.join(logsDir, "main.jsonl"), PROJECT_LOG_LIMITS)); + logs.push( + readLogTail("Desktop main (project)", path.join(logsDir, "main.jsonl"), PROJECT_LOG_LIMITS), + ); logs.push(readLogTail("ADE CLI", path.join(logsDir, "ade-cli.jsonl"), PROJECT_LOG_LIMITS)); } diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index dcbad71a9..69a235315 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -17,6 +17,21 @@ for (const stream of [process.stdout, process.stderr]) { }); } +// The first durable line of the launch, and the earliest statement in this file +// that can write one: everything below — the `ade://` claim, the single-instance +// lock, the whole of `whenReady` — used to happen before any structured logger +// existed, because the only one was built inside the project-open paths. A +// report from a machine where no project ever opened had no main-process log at +// all. Machine-scoped by construction (`~/.ade/runtime/desktop-main.jsonl`), so +// a headless `ade report-issue` finds it too. See `machineLogger.ts`. +logMachineEvent("info", "desktop.main_started", { + pid: process.pid, + version: app.getVersion(), + isPackaged: app.isPackaged, + packageChannel: process.env.ADE_PACKAGE_CHANNEL ?? null, + platform: process.platform, +}); + import { AsyncLocalStorage } from "node:async_hooks"; import os from "node:os"; import path from "node:path"; @@ -49,6 +64,11 @@ import { import { registerIpc } from "./services/ipc/registerIpc"; import { AttemptedProjectRoots } from "./services/ipc/knownProjectRoots"; import { createFileLogger } from "./services/logging/logger"; +import { + flushMachineMainLog, + getMachineMainLogger, + logMachineEvent, +} from "./services/logging/machineLogger"; import { createProductAnalyticsService, defaultProductAnalyticsStateFile, @@ -468,14 +488,20 @@ let adeCliAutoInstallScheduled = false; * matters — already on PATH, already settled once, silent failure — lives in * `runAdeCliAutoInstall`; this only schedules it. Both project-open and dormant * startup reach here, so the process-wide latch keeps it to a single attempt. + * + * Its outcome goes to the MACHINE log, not the caller's. Whether this computer + * ever got the `ade` command is a fact about the computer: filed per project it + * landed wherever the latch happened to win, and on the dormant path it landed + * in a `userData` log no report collects at all — so "did the auto-install + * run?" was unanswerable from the one document meant to answer it. */ function installAdeCliForTerminalInBackground( adeCliService: ReturnType, - logger: Logger, globalStatePath: string, ): void { if (adeCliAutoInstallScheduled) return; adeCliAutoInstallScheduled = true; + const logger = getMachineMainLogger(); // A convenience, not startup work: it can spawn the packaged installer and // append to a shell profile, so it stays off the path to the first window. const task = setImmediate(() => { @@ -1104,16 +1130,14 @@ const dispatchOrQueueAppNavigationRequest = (request: AppNavigationRequest): voi pendingAppNavigationRequests.length - MAX_PENDING_APP_NAVIGATION_REQUESTS, ); } - try { - // The structured logger is not up this early; console keeps the signal. - console.warn("[main] app_navigation.queued_before_dispatcher_ready", { - target: request.target.kind, - source: request.source, - queued: pendingAppNavigationRequests.length, - }); - } catch { - // A missing console must never break navigation queueing. - } + // Durable in the machine log and still on stdout; `logMachineEvent` owns + // both, and swallows a failure of either — a missing console must never + // break navigation queueing. + logMachineEvent("warn", "app_navigation.queued_before_dispatcher_ready", { + target: request.target.kind, + source: request.source, + queued: pendingAppNavigationRequests.length, + }); return; } dispatchAppNavigationRequest(request); @@ -1125,14 +1149,12 @@ const dispatchOrQueueAppNavigationRequest = (request: AppNavigationRequest): voi registerAdeProtocolHandler({ claimAsDefault: deeplinkClaimAsDefault, dispatch: dispatchOrQueueAppNavigationRequest, - log: (event, fields) => { - // Avoid throwing if console is gone; structured logger may not be ready yet. - try { - console.log(`[main] ${event}`, fields); - } catch { - // ignore - } - }, + // Which scheme this build claimed, and which process won the single-instance + // lock, are facts about the computer, not about any project — and they are + // decided before one can be open. They were `console.log` only because no + // structured logger existed this early; the machine log does. + log: (event, fields) => logMachineEvent("info", event, fields), + flushLog: flushMachineMainLog, }); let pendingProjectOpenFiles: string[] = []; @@ -1367,7 +1389,8 @@ app.whenReady().then(async () => { return new Response("Not found", { status: 404 }); } }); - console.log("[info] app.hardware_acceleration", { + // What this computer's GPU was told to do, decided before any project opens. + logMachineEvent("info", "app.hardware_acceleration", { enabled: !disableHardwareAcceleration, reason: disableHardwareAcceleration ? process.env.ADE_DISABLE_HARDWARE_ACCEL === "1" @@ -1477,8 +1500,10 @@ app.whenReady().then(async () => { machineTrustResetRestartRequired = runMachineTrustResetMigration(machineAdeLayout).restartRequired; } catch (error) { // Leave the reset incomplete so the next launch retries. A filesystem - // permission problem must not prevent ADE itself from starting. - console.warn("[warn] machine_trust_reset.failed", { + // permission problem must not prevent ADE itself from starting. The + // subject is this machine's saved machine list, so it belongs in the + // machine log — and it fires before a project logger could exist. + logMachineEvent("warn", "machine_trust_reset.failed", { error: error instanceof Error ? error.message : String(error), }); } @@ -2852,7 +2877,7 @@ app.whenReady().then(async () => { logger, }); adeCliService.applyToProcessEnv(); - installAdeCliForTerminalInBackground(adeCliService, logger, globalStatePath); + installAdeCliForTerminalInBackground(adeCliService, globalStatePath); const devToolsService = createDevToolsService({ logger }); const project = toProjectInfo(projectRoot, baseRef); @@ -5204,7 +5229,7 @@ app.whenReady().then(async () => { logger, }); adeCliService.applyToProcessEnv(); - installAdeCliForTerminalInBackground(adeCliService, logger, globalStatePath); + installAdeCliForTerminalInBackground(adeCliService, globalStatePath); const externalOnlyLaneService: FileServiceLaneAdapter = { getFilesWorkspaces: () => [], resolveWorkspaceById: (workspaceId: string) => { diff --git a/apps/desktop/src/main/services/deeplinks/protocolHandler.ts b/apps/desktop/src/main/services/deeplinks/protocolHandler.ts index 7e1c3592a..8197f4de1 100644 --- a/apps/desktop/src/main/services/deeplinks/protocolHandler.ts +++ b/apps/desktop/src/main/services/deeplinks/protocolHandler.ts @@ -47,6 +47,13 @@ export function registerAdeProtocolHandler(options: { dispatch: DeeplinkDispatcher; /** Optional structured log hook. */ log?: (event: string, fields: Record) => void; + /** + * Drains {@link log} to disk. Called only on the branch that quits this + * process immediately, where a batched write would die with it — and that + * branch's line (`deeplink.single_instance.lock_lost`) is the one that says + * which process ADE actually booted. + */ + flushLog?: () => void; /** * When true, ask the OS to make this app the default `ade://` handler. When * false the single-instance lock and `open-url` / `second-instance` @@ -114,6 +121,7 @@ export function registerAdeProtocolHandler(options: { shouldForwardToLockHolder, }); if (shouldForwardToLockHolder) { + options.flushLog?.(); app.quit(); return; } diff --git a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts index 1a730ee67..03164f093 100644 --- a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts +++ b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.test.ts @@ -165,7 +165,7 @@ describe("collectDiagnosticReport", () => { { surface: "project_recovery", projectRoot: null }, ); - expect(report).toContain("### Desktop main"); + expect(report).toContain("### Desktop main (project)"); expect(report).toContain("deeplink.scheme_claimed"); expect(report).toContain("### ADE CLI"); expect(report).toContain("no project was open"); @@ -173,6 +173,30 @@ describe("collectDiagnosticReport", () => { expect(report).not.toContain(projectRoot); }); + // The fallback above still needs a project to have been opened once, and to + // guess the right one. On a machine where none ever was, the main process's + // own machine log is the only main-process evidence there is — and a report + // from that machine used to contain none at all. + it("carries the desktop's machine log with no project on the machine", async () => { + const adeHome = fs.mkdtempSync(path.join(tempRoot, "adeHome-")); + fs.mkdirSync(path.join(adeHome, "runtime"), { recursive: true }); + fs.writeFileSync( + path.join(adeHome, "runtime", "desktop-main.jsonl"), + '{"event":"desktop.main_started"}\n{"event":"deeplink.single_instance.lock_lost"}\n', + "utf8", + ); + + const { report } = await collectDiagnosticReport( + { ...deps(), env: { ADE_HOME: adeHome } }, + { surface: "project_recovery", projectRoot: null }, + ); + + expect(report).toContain("### Desktop main (machine)"); + expect(report).toContain("desktop.main_started"); + expect(report).toContain("deeplink.single_instance.lock_lost"); + expect(report).toContain("no project is registered"); + }); + // The desktop and `ade report-issue` are meant to produce the same document; // the section that says what the background service was told to be is part // of it, present-or-noted, on every platform. diff --git a/apps/desktop/src/main/services/logging/machineLogger.test.ts b/apps/desktop/src/main/services/logging/machineLogger.test.ts new file mode 100644 index 000000000..3b807f2b7 --- /dev/null +++ b/apps/desktop/src/main/services/logging/machineLogger.test.ts @@ -0,0 +1,161 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + createMachineMainLogger, + flushMachineMainLog, + getMachineMainLogger, + logMachineEvent, + machineMainLogPath, + resetMachineMainLoggerForTests, +} from "./machineLogger"; + +/** + * The main process had no durable log until a project opened, so everything + * before that — the `ade://` claim, the single-instance lock, the CLI + * auto-install outcome — either vanished or was filed under whichever project + * happened to open. These cover the properties that fix depends on. + */ + +const tempDirs: string[] = []; + +function tempHome(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-machine-log-")); + tempDirs.push(dir); + return dir; +} + +const ORIGINAL_ADE_HOME = process.env.ADE_HOME; + +beforeEach(() => { + resetMachineMainLoggerForTests(); +}); + +afterEach(() => { + vi.restoreAllMocks(); + resetMachineMainLoggerForTests(); + if (ORIGINAL_ADE_HOME === undefined) delete process.env.ADE_HOME; + else process.env.ADE_HOME = ORIGINAL_ADE_HOME; + for (const dir of tempDirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +describe("machineMainLogPath", () => { + // The whole point of the location: a headless `ade report-issue` resolves it + // with the same `resolveMachineAdeLayout` the collector already uses, on a + // machine where no project has ever been opened. An `app.getPath("userData")` + // path would be a per-platform, per-productName directory the CLI must guess. + it("sits in the machine's runtime dir, beside brain.jsonl", () => { + const adeHome = path.join(tempHome(), ".ade"); + + expect(machineMainLogPath({ ADE_HOME: adeHome })).toBe( + path.join(adeHome, "runtime", "desktop-main.jsonl"), + ); + }); + + // Each packaged channel gets its own ADE home; two of them must not append to + // one file and read as a single machine's story. + it("follows ADE_HOME, so channels do not share one log", () => { + const stable = path.join(tempHome(), ".ade"); + const beta = path.join(tempHome(), ".ade-beta"); + + expect(machineMainLogPath({ ADE_HOME: stable })).not.toBe( + machineMainLogPath({ ADE_HOME: beta }), + ); + }); +}); + +describe("logMachineEvent", () => { + it("writes machine events to the machine log and not to any project", () => { + const home = tempHome(); + process.env.ADE_HOME = path.join(home, ".ade"); + const projectLogsDir = path.join(home, "project", ".ade", "transcripts", "logs"); + fs.mkdirSync(projectLogsDir, { recursive: true }); + vi.spyOn(console, "log").mockImplementation(() => {}); + + logMachineEvent("info", "ade_cli.auto_install", { ok: true }); + flushMachineMainLog(); + + const written = fs.readFileSync(machineMainLogPath(), "utf8"); + expect(JSON.parse(written.trim())).toMatchObject({ + level: "info", + event: "ade_cli.auto_install", + meta: { ok: true }, + }); + expect(fs.existsSync(path.join(projectLogsDir, "main.jsonl"))).toBe(false); + }); + + // Not vestigial: a terminal-launched app shows it to whoever is watching, and + // an old plist that boots the desktop app as the background service routes + // main's stdout into launchd.out.log, which a report also collects. + it("still mirrors to the console", () => { + process.env.ADE_HOME = path.join(tempHome(), ".ade"); + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + logMachineEvent("warn", "deeplink.single_instance.lock_lost", { claimAsDefault: true }); + + expect(consoleSpy).toHaveBeenCalledWith( + "[main] deeplink.single_instance.lock_lost", + { claimAsDefault: true }, + ); + }); + + // This runs before the app exists. A log sink that can throw would turn a + // broken `~/.ade` into an app that will not start at all. + it("never throws, even when the log cannot be written", () => { + // A file where the runtime DIRECTORY has to be: mkdir fails for every write. + const home = tempHome(); + const adeHome = path.join(home, ".ade"); + fs.mkdirSync(adeHome, { recursive: true }); + fs.writeFileSync(path.join(adeHome, "runtime"), "not a directory", "utf8"); + process.env.ADE_HOME = adeHome; + vi.spyOn(console, "log").mockImplementation(() => {}); + + expect(() => { + logMachineEvent("info", "desktop.main_started", { pid: 1 }); + flushMachineMainLog(); + }).not.toThrow(); + }); + + // A detached GUI launch can leave the process with no usable stdout at all. + it("survives a console that throws", () => { + process.env.ADE_HOME = path.join(tempHome(), ".ade"); + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => { + throw new Error("no console"); + }); + + expect(() => logMachineEvent("error", "desktop.main_started")).not.toThrow(); + expect(consoleErrorSpy).toHaveBeenCalled(); + flushMachineMainLog(); + expect(fs.readFileSync(machineMainLogPath(), "utf8")).toContain("desktop.main_started"); + }); + + it("reuses one logger across calls", () => { + process.env.ADE_HOME = path.join(tempHome(), ".ade"); + + expect(getMachineMainLogger()).toBe(getMachineMainLogger()); + }); +}); + +describe("rotation", () => { + // Bounded by the shared file logger's scheme, the same one that bounds + // brain.jsonl: at the cap the live file is renamed to `.1.jsonl`. A log that + // exists from process start on every launch may not grow without limit. + it("rotates to desktop-main.1.jsonl at the size cap", async () => { + const adeHome = path.join(tempHome(), ".ade"); + const logger = createMachineMainLogger({ + env: { ADE_HOME: adeHome }, + fileLogger: { maxFileBytes: 512, rotationCheckWriteInterval: 1, flushIntervalMs: 1 }, + }); + const logPath = machineMainLogPath({ ADE_HOME: adeHome }); + + for (let index = 0; index < 40; index += 1) { + logger.info("desktop.main_started", { index, padding: "x".repeat(64) }); + await new Promise((resolve) => setTimeout(resolve, 2)); + } + + expect(fs.existsSync(path.join(path.dirname(logPath), "desktop-main.1.jsonl"))).toBe(true); + expect(fs.statSync(logPath).size).toBeLessThanOrEqual(512); + }); +}); diff --git a/apps/desktop/src/main/services/logging/machineLogger.ts b/apps/desktop/src/main/services/logging/machineLogger.ts new file mode 100644 index 000000000..f8754db3a --- /dev/null +++ b/apps/desktop/src/main/services/logging/machineLogger.ts @@ -0,0 +1,128 @@ +import path from "node:path"; + +import { resolveMachineAdeLayout } from "../../../../../ade-cli/src/services/projects/machineLayout"; +import { createFileLogger, type FileLoggerOptions, type Logger } from "./logger"; + +/** + * The desktop main process's MACHINE-scoped log. + * + * The only structured sink main had was `createFileLogger(/.ade/ + * transcripts/logs/main.jsonl)`, built inside the project-open paths. Two + * consequences, both observed on a real machine: everything before a project + * opened had nowhere durable to go (which is why the early-startup lines were + * `console.log("[main] …")` — the comment there said the structured logger may + * not be ready yet, and it was right), and machine-level facts like the `ade` + * CLI auto-install outcome were filed under whichever project happened to open, + * so the same computer told a different story depending on what was open. A + * user whose app failed before opening a project produced a diagnostic report + * with no main-process log at all. + * + * It lives in the machine's `~/.ade/runtime`, next to `brain.jsonl` and + * `account-trust.jsonl`, and NOT under `app.getPath("userData")`. The deciding + * question is who can read it: `resolveMachineAdeLayout` is the same resolver + * `ade report-issue` uses, so a headless report — collected on the machine + * where the desktop will not start, which is the whole point — finds this file + * by construction and honours the same `ADE_HOME`, so each channel keeps its + * own. Electron's `userData` is a per-platform, per-productName directory the + * CLI would have to guess at, which is why `local-runtime.jsonl` and + * `ade-update.jsonl` are still desktop-only sources in a report. + * + * Size is the shared file logger's problem, deliberately: same 10 MiB rotation + * to `desktop-main.1.jsonl` that bounds `brain.jsonl`. + */ +export const MACHINE_MAIN_LOG_FILE_NAME = "desktop-main.jsonl"; + +export function machineMainLogPath(env: NodeJS.ProcessEnv = process.env): string { + return path.join(resolveMachineAdeLayout(env).runtimeDir, MACHINE_MAIN_LOG_FILE_NAME); +} + +/** + * Never `null`, so no caller has to branch on "did the log exist" on the path + * that runs before the app does. A machine whose `~/.ade` cannot even be + * resolved still starts ADE. + */ +const NOOP_LOGGER: Logger = { + debug: () => {}, + info: () => {}, + warn: () => {}, + error: () => {}, + flushSync: () => {}, +}; + +export function createMachineMainLogger( + options: { env?: NodeJS.ProcessEnv; fileLogger?: FileLoggerOptions } = {}, +): Logger { + try { + return createFileLogger(machineMainLogPath(options.env), options.fileLogger); + } catch { + // `resolveMachineAdeLayout` reads the environment and, on Windows, the + // user identity; a box where that throws must still boot. Writes are + // already best-effort inside the file logger itself. + return NOOP_LOGGER; + } +} + +let sharedMachineMainLogger: Logger | null = null; + +export function getMachineMainLogger(): Logger { + if (!sharedMachineMainLogger) sharedMachineMainLogger = createMachineMainLogger(); + return sharedMachineMainLogger; +} + +/** Test seam: the shared logger caches the path it resolved at first use. */ +export function resetMachineMainLoggerForTests(): void { + sharedMachineMainLogger = null; +} + +type MachineLogLevel = "debug" | "info" | "warn" | "error"; + +const CONSOLE_METHOD_BY_LEVEL: Record = { + debug: "debug", + info: "log", + warn: "warn", + error: "error", +}; + +/** + * One machine-scoped event: durable in `desktop-main.jsonl` AND on stdout. + * + * The console half is not vestigial. A terminal-launched app shows it to the + * person watching, and the pathological case this log exists to diagnose — an + * old plist that boots the whole desktop app as the background service — routes + * main's stdout into `~/.ade/runtime/launchd.out.log`, which a report also + * collects. Two independent copies of a startup line cost nothing and the + * incident that motivated this file turned on exactly such a line. + */ +export function logMachineEvent( + level: MachineLogLevel, + event: string, + fields?: Record, +): void { + try { + getMachineMainLogger()[level](event, fields); + } catch { + // A log sink is never worth a failed launch. + } + try { + // Resolved per call, not captured at module load: this runs before the app + // does, and whatever `console` is at that moment is what gets the line. + console[CONSOLE_METHOD_BY_LEVEL[level]](`[main] ${event}`, fields ?? ""); + } catch { + // stdout can be gone entirely (detached GUI launch, closed terminal). + } +} + +/** + * Drains still-queued lines to disk. Writes batch onto an async stream, so a + * caller about to end the process must call this or the records explaining the + * exit die with it — see `Logger.flushSync`. Deliberately NOT called after + * every line: `flushSync` skips rotation by design, so a logger that only ever + * flushed that way would never rotate. + */ +export function flushMachineMainLog(): void { + try { + getMachineMainLogger().flushSync?.(); + } catch { + // Same contract as the writes. + } +} diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6140b7eb5..1fdaa8fef 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -567,13 +567,14 @@ Types for these tables are split into domain modules under `apps/desktop/src/sha ├── personal-chats/ │ ├── state/ # Hidden chat runtime DB, transcripts, attachments │ └── workspaces/ # Separate provider cwd + personal terminal scratch - ├── runtime/ - │ ├── brain.jsonl # Brain log stream (10 MiB rotation) - │ ├── heartbeat.json # Brain liveness beat read by the external watchdog - │ ├── event-loop-wedge.json / last-wedge.json # Wedge breadcrumb, promoted on next boot - │ ├── spawns/ # Detached-brain spawn records (0600) - │ └── updates/ # Staged `ade brain update` payloads - └── logs/ # Main-process structured logs + └── runtime/ + ├── brain.jsonl # Brain log stream (10 MiB rotation) + ├── desktop-main.jsonl # Desktop main-process machine log (10 MiB rotation) + ├── account-trust.jsonl # Machine-scoped paired-credential mutations + ├── heartbeat.json # Brain liveness beat read by the external watchdog + ├── event-loop-wedge.json / last-wedge.json # Wedge breadcrumb, promoted on next boot + ├── spawns/ # Detached-brain spawn records (0600) + └── updates/ # Staged `ade brain update` payloads ``` **Portability buckets** (intentionally distinct): @@ -1828,7 +1829,8 @@ Post-packaging hardening (`apps/desktop/scripts/`): ### 15.1 Logging -- **Main-process logger** — `apps/desktop/src/main/services/logging/logger.ts` (`createFileLogger`). Writes structured JSONL to `~/.ade/logs//ade-main.log`. Categories: `ipc.*`, `project.startup_task_*`, `renderer.*`, per-service telemetry. +- **Main-process project logger** — `apps/desktop/src/main/services/logging/logger.ts` (`createFileLogger`). Writes structured JSONL to `/.ade/transcripts/logs/main.jsonl`, created when a project opens. Categories: `ipc.*`, `project.startup_task_*`, `renderer.*`, per-service telemetry. +- **Main-process machine logger** — `apps/desktop/src/main/services/logging/machineLogger.ts` reuses `createFileLogger` to write `~/.ade/runtime/desktop-main.jsonl` with the same 10 MiB `.1` rotation, opened in `main.ts`'s first executable statement so events that precede (or never reach) a project are durable. Categories: `desktop.main_started`, `deeplink.*`, `app_navigation.*`, `app.hardware_acceleration`, `machine_trust_reset.*`, `ade_cli.auto_install*`. See [logging.md](./logging.md) for the machine-versus-project rule. - **Machine-brain logger** — the headless runtime reuses `createFileLogger` through `apps/ade-cli/src/services/runtime/brainLogger.ts`, writes `~/.ade/runtime/brain.jsonl` with 10 MiB `.1` rotation, and mirrors timestamped warnings/errors to stderr. - **Redaction** — all log writes pass through `redactSecrets()` / `sanitizeStructuredData()`. - **Retention** — local, indefinite until user clears. diff --git a/docs/features/deeplinks/README.md b/docs/features/deeplinks/README.md index daa4d3920..c23865403 100644 --- a/docs/features/deeplinks/README.md +++ b/docs/features/deeplinks/README.md @@ -123,7 +123,11 @@ Desktop main process — protocol handler: caller-supplied dispatcher. `main.ts` wires the dispatcher to focus the most-suitable `BrowserWindow` and `webContents.send(IPC.appNavigate, …)`. `handleDeeplinkUrl` is also re-used by the iOS Send-to-Mac sync command - (`syncRemoteCommandService.ts`'s `deeplinks.open`). + (`syncRemoteCommandService.ts`'s `deeplinks.open`). Its `log` hook is wired + to the machine logger (`~/.ade/runtime/desktop-main.jsonl`), not a project + one — all of this happens before a project can be open — and the branch that + loses the single-instance lock and quits calls `flushLog` first, so + `deeplink.single_instance.lock_lost` survives the exit. - `apps/desktop/src/main/services/deeplinks/projectNavigationWindowSelection.ts` — pure selection helper for project-scoped navigation. It first prefers a window whose active project already matches the target root, then a window diff --git a/docs/features/storage-and-recovery/README.md b/docs/features/storage-and-recovery/README.md index fbc11412e..efb788173 100644 --- a/docs/features/storage-and-recovery/README.md +++ b/docs/features/storage-and-recovery/README.md @@ -17,6 +17,7 @@ | `apps/ade-cli/src/services/runtime/brainFreshnessMonitor.ts` | The running brain stats its own CLI entrypoint every 5 min (`ADE_BRAIN_FRESHNESS_INTERVAL_MS`), hashes only after the stat changes, and — when the on-disk hash no longer matches the baked runtime hash — waits for the brain to go idle (bounded) before triggering the brain-update service restart so an in-place upgrade takes effect without interrupting active work. Disable with `ADE_DISABLE_BRAIN_FRESHNESS=1`. | | `apps/ade-cli/src/services/runtime/runtimeBuildIdentity.ts` | `computeRuntimeBuildHash` / `computeRuntimeBuildHashAsync` — the SHA-256 of the CLI entrypoint used as the brain build identity by the freshness monitor and the desktop compatibility handshake. | | `apps/ade-cli/src/services/runtime/brainLogger.ts` | The machine-brain logger: reuses the desktop `createFileLogger` to write `~/.ade/runtime/brain.jsonl` (10 MiB `.1` rotation) and additionally mirrors timestamped `warn`/`error` lines to stderr so launchd captures them. | +| `apps/desktop/src/main/services/logging/machineLogger.ts` | The desktop main process's **machine-scoped** logger: same `createFileLogger` and rotation, writing `~/.ade/runtime/desktop-main.jsonl`. Opened in `main.ts`'s first executable statement, so the events that happen before (or without) a project — the launch marker, the `ade://` claim, the single-instance outcome, the CLI auto-install result — are durable rather than `console.log` into a stdout a Finder-launched app does not have. `logMachineEvent` writes the structured line and mirrors it to `console`; `flushMachineMainLog` drains it on the branch that quits immediately. See [logging.md](../../logging.md). | | `apps/ade-cli/src/commands/doctor.ts` | `ade doctor [--online]` — connects to the brain over the local socket and prints one `ok`/`warn`/`fail` row per subsystem (App version, Brain, Wedge history, Sync port, Publish health, Relay, Account, Diagnostics sharing); exits non-zero on any `fail`. The **Diagnostics sharing** row reads the shared auto-send ledger through `readAutoDiagnosticsState` — never a second parser — and is always `ok`: consent is a preference, not a fault, so it reports `on · N of 3 automatic reports sent today` or `off · no automatic reports are sent` and never colours a healthy machine. `evaluateDoctorRows` is pure and dependency-injected — every row's inputs are read at the edge (`runDoctorCommand`) and handed in — so the verdict is testable without a machine and a second surface can reuse it. Today the CLI is its only caller: the desktop's **Connection doctor** card (`remoteTargets/ConnectionDoctorPanel.tsx` → `remoteRuntime.runDoctor`) is a different check about reaching a *remote* machine, not this one. | | `apps/desktop/src/shared/adeRuntimeProtocol.ts` | Shared runtime-protocol contract: `RUNTIME_COMPAT_LEVEL` + `isRuntimeProtocolCompatible` (the integer compatibility-window check), and the tolerant parsers `parseRuntimePublishHealth` / `parseRuntimeLastWedge` that decode `runtimeInfo.publishHealth` and `runtimeInfo.lastWedge` for the connection pool, the doctor, and the desktop status surfaces. | | `apps/desktop/src/main/services/runtime/projectRecoveryService.ts` | Brain-independent diagnosis and ordered repair: space, ownership, database validation, migration recovery, service restart, endpoint/project verification, and chat reconciliation. Also owns `restartBrain()` — the machine-scoped restart behind the Connections **Repair** button — which shares one `restartServiceAndWait()` sequence (install → wait ≤90 s for the endpoint → `ping`) with `repair()`'s restart_service/verify_endpoint steps. The two are mutually exclusive: `restartBrain()` rejects while a `repair()` is in flight, because repair stops the service and then does exclusive database work that a reinstall would put a second writer on top of. A forced restart also treats a *skipped* install as a failure ("A newer ADE runtime is already running — quit and reopen ADE instead."), where `repair()` tolerates one, since a protocol-compatible brain that is already running satisfies its step. `main.ts` constructs exactly one of these and shares it with `registerIpc`, so the mutual exclusion actually holds — the post-update transaction's `restart` step (see [desktop auto-update](../onboarding-and-settings/desktop-auto-update.md#applying-an-update-is-one-transaction)) binds to the same instance rather than a second one that could run alongside a repair. | @@ -680,10 +681,11 @@ collected by `collectMachineDiagnosticSources`, so the desktop button and | Source | Cap | Why | | --- | --- | --- | -| Background service, **both** streams — `launchd.err.log` **and** `launchd.out.log` on macOS, the supervisor log on Windows (one merged stream by construction), `journalctl --user-unit` on Linux when the unit exists | 120 lines / 32 KB | Early-startup lines are written with `console.log` before the structured logger exists, so `deeplink.scheme_claimed` and `deeplink.single_instance.lock_lost` land in **stdout and nowhere else**. Collecting only stderr is how a user once had to read the decisive two lines off his own disk by hand. | +| Background service, **both** streams — `launchd.err.log` **and** `launchd.out.log` on macOS, the supervisor log on Windows (one merged stream by construction), `journalctl --user-unit` on Linux when the unit exists | 120 lines / 32 KB | Main's early-startup events mirror to `console`, so on a machine whose plist boots the desktop app as the background service they land in **stdout**. Collecting only stderr is how a user once had to read the decisive two lines off his own disk by hand. | +| `desktop-main.jsonl` — "Desktop main (machine)" | 120 lines / 32 KB | The desktop main process's machine-scoped log, written from process start. The only main-process log on a machine where no project has ever been opened, and where the launch marker, the `ade://` claim, the single-instance outcome and the `ade_cli.auto_install` result live. Full cap, like the other machine-level streams beside it. | | `brain.jsonl` | 120 lines / 32 KB | The brain's own structured log. | -| `local-runtime.jsonl`, `ade-update.jsonl` (desktop only) | 120 lines / 32 KB | Under Electron's `userData`; the CLI does not write them. | -| The project's `main.jsonl` and `ade-cli.jsonl` | 80 lines / 16 KB | Machine-level events (the `ade_cli.auto_install` outcome among them) live here. Collected for the open project, or — when no project is open — for the most recently opened project in `~/.ade/projects.json`, with a note in the report saying which. | +| `local-runtime.jsonl`, `ade-update.jsonl` (desktop only) | 120 lines / 32 KB | Under Electron's `userData`, a per-platform per-productName directory; the CLI does not write them and does not guess at them. | +| The project's `main.jsonl` and `ade-cli.jsonl` — "Desktop main (project)" | 80 lines / 16 KB | What the *project* did. Collected for the open project, or — when no project is open — for the most recently opened project in `~/.ade/projects.json`, with a note in the report saying which. That fallback is a mitigation: it needs some project to have been opened, and to guess the right one, which is why the machine log above exists. | | **Service definition**: the launchd plist, the systemd user unit, or the Windows launcher script plus its scheduled task XML | first 8 KB | Configuration, not logs, and read from the front because a plist states its `Label`, `ProgramArguments` and `EnvironmentVariables` first. A plist written without `ELECTRON_RUN_AS_NODE=1` boots the whole desktop app as the background service, which then claims the `ade://` scheme and fights the GUI for the single-instance lock — a failure with no signature in any log. | Two properties hold for every one of them. **Absence is a fact, not an error**: @@ -691,7 +693,7 @@ a missing or unreadable source becomes `(not present)` / `(could not be read)` under its own heading, never a thrown collector and never a failed upload — the machine this runs on is by definition damaged. And the total stays inside `MAX_DIAGNOSTIC_UPLOAD_BYTES` (512 KB for the serialized upload): at the caps -above a desktop report's tails are ~208 KB at their theoretical worst and a real +above a desktop report's tails are ~240 KB at their theoretical worst and a real one is well under 100 KB, which is why the two project logs take the smaller cap and the service definition is capped at all. diff --git a/docs/logging.md b/docs/logging.md index 0673b9d0f..c07aa3fa0 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -24,11 +24,15 @@ Operational logs use ADE's local logging services and may include bounded diagno The machine brain writes the same `{ts, level, event, meta}` JSONL format as the desktop logger to `~/.ade/runtime/brain.jsonl`, honoring `ADE_LOG_LEVEL` (default `info`). The file rotates at 10 MiB to `brain.1.jsonl`; warnings and errors are also mirrored to stderr with an ISO-8601 timestamp and uppercase level for launchd diagnostics. -Not everything reaches a structured logger. Lines written before one exists — `console.log("[main] …")` during early startup, anything the runtime prints on its way up — go to the background service's **stdout**, which on macOS is `~/.ade/runtime/launchd.out.log` and not the `launchd.err.log` that carries stderr. Both streams are bounded by `runtimeLogMaintenance.ts` and **both** are collected into a diagnostic report; see [Diagnostic reports](features/storage-and-recovery/README.md#diagnostic-reports-report-issue). A `console.log` at that stage is therefore diagnosable, but it is still a last resort: prefer a structured event as soon as the logger is available. +The desktop main process has a structured logger **from process start**: `apps/desktop/src/main/services/logging/machineLogger.ts` writes `~/.ade/runtime/desktop-main.jsonl` (same format, same `createFileLogger`, same 10 MiB rotation to `desktop-main.1.jsonl`), and `main.ts` opens it in its first executable statement, before the `ade://` claim and the single-instance lock. Its location is chosen for who can *read* it: `resolveMachineAdeLayout` is the resolver `ade report-issue` uses, so a headless report on a machine where the desktop will not start finds the file by construction, and each channel's `ADE_HOME` keeps its own. Electron's `userData` — where `local-runtime.jsonl` and `ade-update.jsonl` still live — is a per-platform, per-productName directory the CLI would have to guess, which is why those two remain desktop-only sources in a report. + +Not everything reaches a structured logger. Lines the runtime prints on its way up go to the background service's **stdout**, which on macOS is `~/.ade/runtime/launchd.out.log` and not the `launchd.err.log` that carries stderr. Both streams are bounded by `runtimeLogMaintenance.ts` and **both** are collected into a diagnostic report; see [Diagnostic reports](features/storage-and-recovery/README.md#diagnostic-reports-report-issue). Early main-process events additionally mirror to `console` through `logMachineEvent`, so a terminal-launched app still shows them and the pathological case — an old plist that boots the whole desktop app as the background service — still leaves them in `launchd.out.log`. That mirror is a second copy, not the record: a bare `console.log` for something the machine log could carry is no longer acceptable, because it is invisible to a Finder-launched app. Writes are batched onto an async stream, so a caller that is about to end the process (`app.exit`, a force quit, an install handoff escalation) must call `logger.flushSync()` immediately after the line that matters — while it is still queued — or the records explaining the exit die with the process. `flushSync` drains only what is still queued (a batch already handed to an in-flight async flush is not duplicated), skips rotation deliberately, and, like every other log write, never throws. -Not every operational log belongs to the active project. `createFileLogger` also backs machine-scoped sinks for facts that outlive or fall outside a project: `accountBridge` writes `account.local_machines_removed` to `/runtime/account-trust.jsonl`, because dropping a paired machine credential is a machine-level mutation and the project logger follows the active project — on a remote-bound project it would ship the record to the other machine and leave nothing on the machine that actually lost its trust. Account-directory publish outcomes record only bounded per-leg durations, the failing leg, and coarse failure codes such as `token_timeout` or `http_timeout`; they never include bearer tokens or response bodies. These high-frequency health events remain local operational logs and are not product analytics. +Not every operational log belongs to the active project. The rule is the **subject** of the event: if it is the computer, it goes to `desktop-main.jsonl`; if it is a repository, it goes to that project's `main.jsonl`. Machine-subject events include the launch marker `desktop.main_started`, the deeplink scheme claim and single-instance outcome (`deeplink.*`, including `deeplink.single_instance.lock_lost`), `app_navigation.queued_before_dispatcher_ready`, `app.hardware_acceleration`, `machine_trust_reset.failed`, and the `ade` CLI auto-install outcome (`ade_cli.auto_install`, `ade_cli.auto_install_failed`, `ade_cli.auto_install_skipped`) — whether this computer ever got the `ade` command is a fact about the computer, and filed per project it landed wherever the startup latch happened to win. Project-subject events (`project.init`, `ipc.*`, per-service telemetry) stay in the project log, unchanged. Auto-update events were already machine-scoped in `/ade-update.jsonl` and stay there. + +`createFileLogger` backs other machine-scoped sinks for the same reason: `accountBridge` writes `account.local_machines_removed` to `/runtime/account-trust.jsonl`, because dropping a paired machine credential is a machine-level mutation and the project logger follows the active project — on a remote-bound project it would ship the record to the other machine and leave nothing on the machine that actually lost its trust. Account-directory publish outcomes record only bounded per-leg durations, the failing leg, and coarse failure codes such as `token_timeout` or `http_timeout`; they never include bearer tokens or response bodies. These high-frequency health events remain local operational logs and are not product analytics. Claude compaction observations use the local structured line `agent_chat.claude_context_compaction_observed` with `sessionId`, `trigger` From 7ccd2da2c4c68a0f9672e5b3d946bb77b9fde221 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:39:33 -0400 Subject: [PATCH 8/9] fix(diagnostics): stop a report from freezing the app, and from claiming a file that isn't there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven CodeRabbit findings triaged against the code. Four were real. The desktop collected its diagnostic report on Electron's main process with `spawnSync`, before the first `await`. On Windows that command is a PowerShell `Export-ScheduledTask` and on Linux a `journalctl`, each bounded only by the 4s cap — so every window, menu and IPC call froze for the duration, most often for an automatic report nobody asked for. The commands are now planned and run ahead of the collection and the collector reads their answers, so the sources are identical and the wait is not on the main thread. The plan and the collector share one set of command builders, because a prefetch that decided for itself would eventually run a different command than the report asked for and call a perfectly readable source unreadable. The headless `ade report-issue` keeps the synchronous path: a one-shot CLI has nothing to hold up. An oversized manual send told the user "It's saved on this computer — open it" even when the local copy could not be written and there was no "View report" button to press. The main process already answers that case without a path; the sentence now depends on it. The installed-product smoke re-checked a failed `taskkill` on the app process by asking only whether SOMETHING still held the PID. Windows hands a freed PID to the next process that asks, so a recycled number failed a smoke that had actually passed — the same bug the supervisor loop above it was fixed for. Both loops now re-check with the same ownership test that selected the process, and both treat a process table they could not read as "still there" rather than as success: an unverified kill may not pass. Neither script can be run off Windows, so the CI job that already parses the standalone installer now parses these two as well, and asserts the second ownership check is still there. Manual reservations all carry `user_requested`, so (code, atMs, kind) — the triple a completion finds its entry by — was not unique for them. Two claimed in the same millisecond would have been one reservation as far as completion is concerned. The claim now steps past a timestamp already taken, which costs nothing against a 24-hour window and needs no ledger schema change (the brain and the desktop share this file across versions). Rejected, with grounds: - Removing the em dashes rather than adding a UTF-8 BOM. Every other `.ps1` in the repo is pure ASCII and none carries a BOM, and the two characters were in comments; matching the convention closes the Windows PowerShell decoding question without making one file different from its five siblings. - "Do not remove the PID record after a failed supervisor query" in the uninstall cleanup. An uninstall must not refuse to finish over a process it could not stop, and leaving a PID record pointing at a launcher it also removes is worse than removing both; the failure is already recorded, as a warning naming the PID, which is what the user can act on. - The earlier duplicate of the taskkill finding (posted against an older head) was already fixed in 0a2bc17, as CodeRabbit itself noted on it. Also adds the rejected-body 429 test the review asked for: `new Response(null)` has an EMPTY body whose `text()` resolves to "", so it never entered the catch it was written to cover. Co-Authored-By: Claude Opus 5 --- .../diagnostics/diagnosticSources.test.ts | 93 +++++++ .../services/diagnostics/diagnosticSources.ts | 237 +++++++++++++++--- .../windows-installed-product-smoke.ps1 | 80 ++++-- .../scripts/windows-uninstall-cleanup.ps1 | 4 +- .../windows-uninstall-cleanup.test.mjs | 66 +++++ .../diagnostics/autoDiagnosticsStore.test.ts | 38 +++ .../diagnostics/autoDiagnosticsStore.ts | 27 +- .../diagnostics/diagnosticReportService.ts | 22 +- .../DiagnosticsSharingSection.test.tsx | 28 +++ .../settings/DiagnosticsSharingSection.tsx | 9 +- .../src/shared/diagnosticsUpload.test.ts | 22 +- 11 files changed, 563 insertions(+), 63 deletions(-) diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts index 1cf38269b..fd91c5d51 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticSources.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { buildDiagnosticReport } from "./diagnosticReport"; import { collectMachineDiagnosticSources, + collectMachineDiagnosticSourcesAsync, readFileHead, resolveMostRecentProjectRoot, type DiagnosticCommandRunner, @@ -174,6 +175,98 @@ describe("collectMachineDiagnosticSources — service output streams", () => { }); }); +/** + * The same report, gathered without stopping the process that asks for it. + * + * These commands are the only part of the collection that can take seconds — a + * PowerShell `Export-ScheduledTask`, a `journalctl` — and the desktop runs the + * collection on Electron's main process, where a synchronous spawn freezes + * every window and every IPC call for the duration. + */ +describe("collectMachineDiagnosticSourcesAsync", () => { + function collectAsync(args: { + home: string; + platform?: NodeJS.Platform; + runCommandAsync?: ( + command: string, + commandArgs: readonly string[], + ) => Promise<{ status: number | null; stdout: string } | null>; + }) { + return collectMachineDiagnosticSourcesAsync({ + env: { ADE_HOME: path.join(args.home, ".ade") }, + homeDir: args.home, + platform: args.platform ?? "darwin", + projectRoot: null, + readVolume: () => null, + runCommandAsync: args.runCommandAsync + ?? (async () => { + throw new Error("no command was expected"); + }), + }); + } + + it("renders a command's output from the answer it awaited, not a blocking spawn", async () => { + const { home } = machineHome(); + writeSystemdUnit(home); + const run = vi.fn(async (_command: string, _args: readonly string[]) => ({ + status: 0, + stdout: "Aug 19 12:00:00 host ade[1]: brain.started\n", + })); + + const sources = await collectAsync({ home, platform: "linux", runCommandAsync: run }); + const journal = sources.logs.find((log) => log.label === "Background service (journal)"); + + // The prefetch and the collector have to agree on the exact command, or the + // report would say "(could not be read)" about a source that read fine. + expect(run).toHaveBeenCalledTimes(1); + expect(run).toHaveBeenCalledWith( + "journalctl", + ["--user-unit", "com.ade.runtime.service", "--no-pager", "--lines", "200"], + ); + expect(journal?.text).toContain("brain.started"); + }); + + it("runs the Windows scheduled-task export ahead of the collection", async () => { + const { home } = machineHome(); + const run = vi.fn(async (_command: string, _args: readonly string[]) => ({ + status: 0, + stdout: "powershell.exe", + })); + + const sources = await collectAsync({ home, platform: "win32", runCommandAsync: run }); + + expect(run).toHaveBeenCalledTimes(1); + expect(sources.serviceDefinition[1]?.text).toContain("powershell.exe"); + }); + + it("spawns nothing on a platform or a machine with no command to run", async () => { + // macOS keeps every source in a file, and a Linux box with no unit + // installed has nothing to ask journald about. Either way the collection + // must not pay for a subprocess — the seam above throws if one is started. + const darwin = machineHome(); + await expect(collectAsync({ home: darwin.home })).resolves.toBeTruthy(); + + const linux = machineHome(); + const sources = await collectAsync({ home: linux.home, platform: "linux" }); + expect(sources.logs.find((log) => log.label === "Background service (journal)")?.error) + .toBe("(not present)"); + }); + + it("degrades to a noted absence when the prefetched command could not run", async () => { + const { home } = machineHome(); + writeSystemdUnit(home); + + const sources = await collectAsync({ + home, + platform: "linux", + runCommandAsync: async () => null, + }); + + expect(sources.logs.find((log) => log.label === "Background service (journal)")?.error) + .toBe("(could not be read)"); + }); +}); + describe("collectMachineDiagnosticSources — service definition", () => { it("collects the launchd plist, from the front", () => { const { home } = machineHome(); diff --git a/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts b/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts index 4b61dac11..92140eeba 100644 --- a/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts +++ b/apps/ade-cli/src/services/diagnostics/diagnosticSources.ts @@ -1,4 +1,4 @@ -import { spawnSync } from "node:child_process"; +import { execFile, spawnSync } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -123,6 +123,10 @@ export type DiagnosticCommandRunner = ( args: readonly string[], ) => { status: number | null; stdout: string } | null; +/** Every diagnostic command runs under the same bounds, sync or async. */ +const DIAGNOSTIC_COMMAND_TIMEOUT_MS = 4_000; +const DIAGNOSTIC_COMMAND_MAX_BUFFER = 2 * 1024 * 1024; + export function runDiagnosticCommand( command: string, args: readonly string[], @@ -130,10 +134,10 @@ export function runDiagnosticCommand( try { const result = spawnSync(command, [...args], { encoding: "utf8", - timeout: 4_000, + timeout: DIAGNOSTIC_COMMAND_TIMEOUT_MS, windowsHide: true, stdio: ["ignore", "pipe", "pipe"], - maxBuffer: 2 * 1024 * 1024, + maxBuffer: DIAGNOSTIC_COMMAND_MAX_BUFFER, }); if (result.error) return null; return { status: result.status, stdout: typeof result.stdout === "string" ? result.stdout : "" }; @@ -142,6 +146,46 @@ export function runDiagnosticCommand( } } +/** + * The same command, the same bounds, off the event loop. + * + * Same answers as {@link runDiagnosticCommand}, deliberately: a nonzero exit is + * an answer the report renders as "(not present)", while a spawn failure, a + * timeout or an overrun buffer is `null` — "(could not be read)". `execFile` + * reports all four through one `error`, so the exit code is what tells them + * apart (it is a number only when the process actually ran and exited). + */ +export function runDiagnosticCommandAsync( + command: string, + args: readonly string[], +): Promise<{ status: number | null; stdout: string } | null> { + return new Promise((resolve) => { + try { + execFile( + command, + [...args], + { + encoding: "utf8", + timeout: DIAGNOSTIC_COMMAND_TIMEOUT_MS, + windowsHide: true, + maxBuffer: DIAGNOSTIC_COMMAND_MAX_BUFFER, + }, + (error, stdout) => { + const text = typeof stdout === "string" ? stdout : ""; + if (!error) { + resolve({ status: 0, stdout: text }); + return; + } + const code = (error as NodeJS.ErrnoException & { code?: unknown }).code; + resolve(typeof code === "number" ? { status: code, stdout: text } : null); + }, + ); + } catch { + resolve(null); + } + }); +} + /** A command's output as a report entry, with the same absence semantics. */ function readCommandOutput( label: string, @@ -279,6 +323,126 @@ function projectLogsDir(projectRoot: string): string { return path.join(projectRoot, ".ade", "transcripts", "logs"); } +/** + * One external command the collector may need, named once. + * + * Both command sources are described HERE rather than at the site that renders + * them, because the desktop pre-runs them asynchronously (see + * {@link collectMachineDiagnosticSourcesAsync}) and a prefetch that decided on + * its own which commands to run would eventually run a different set than the + * collector asks for — and answer "(could not be read)" for a source that was + * perfectly readable. + */ +type DiagnosticCommandPlan = { + /** How the report names the source it could not read. */ + display: string; + command: string; + args: string[]; +}; + +/** + * The Windows scheduled task export, or null when we cannot even name it. + * + * Both the task name (which folds in the Windows user) and locating PowerShell + * can throw on a broken box; neither may take the report with it, and the + * reader still has to be told we looked. + */ +function windowsScheduledTaskPlan(env: NodeJS.ProcessEnv): DiagnosticCommandPlan | null { + try { + const taskName = resolveWindowsTaskName({ serviceName: resolveRuntimeServiceName(env) }); + return { + display: `Export-ScheduledTask -TaskName "${taskName}"`, + command: windowsPowerShellCommand(), + args: buildWindowsExportTaskArgs(taskName), + }; + } catch { + return null; + } +} + +/** + * The journald read, and whether the unit that would justify it is installed. + * + * The display name exists either way: a machine that never installed the + * service is never charged a subprocess to be told nothing, but the report + * still names the source it skipped. + */ +function journalPlan( + env: NodeJS.ProcessEnv, + homeDir: string, +): DiagnosticCommandPlan & { installed: boolean } { + const serviceName = resolveRuntimeServiceName(env); + const unit = `${serviceName}.service`; + return { + display: `journalctl --user-unit ${unit}`, + command: "journalctl", + args: ["--user-unit", unit, "--no-pager", "--lines", "200"], + installed: fs.existsSync(systemdUnitPath(homeDir, serviceName)), + }; +} + +/** Every command this platform's report would run. At most one, today. */ +function planMachineDiagnosticCommands(args: { + env: NodeJS.ProcessEnv; + platform: NodeJS.Platform; + homeDir: string; +}): DiagnosticCommandPlan[] { + if (args.platform === "darwin") return []; + if (args.platform === "win32") { + const task = windowsScheduledTaskPlan(args.env); + return task ? [task] : []; + } + const journal = journalPlan(args.env, args.homeDir); + return journal.installed ? [journal] : []; +} + +function diagnosticCommandKey(command: string, args: readonly string[]): string { + return JSON.stringify([command, ...args]); +} + +/** + * Runs this machine's diagnostic commands up front and hands back a runner that + * answers from what they returned. + * + * The point is WHERE the waiting happens. The collector is synchronous by + * design — it is the one thing that has to work on a machine where nothing else + * does, and a headless `ade report-issue` wants it simple — but `spawnSync` + * inside it stops the whole process for as long as the command takes, and on + * Windows that command is a PowerShell `Export-ScheduledTask` bounded only by + * the 4s cap. In the desktop that process is Electron's main process, which + * means every IPC call, every window and every menu freezes with it, for a + * report the user very often did not ask for. + */ +export async function prefetchDiagnosticCommands(args: { + env: NodeJS.ProcessEnv; + platform: NodeJS.Platform; + homeDir: string; + /** Test seam; production spawns for real. */ + run?: ( + command: string, + args: readonly string[], + ) => Promise<{ status: number | null; stdout: string } | null>; +}): Promise { + const run = args.run ?? runDiagnosticCommandAsync; + const answers = new Map(); + await Promise.all( + planMachineDiagnosticCommands(args).map(async (plan) => { + let answer: { status: number | null; stdout: string } | null = null; + try { + answer = await run(plan.command, plan.args); + } catch { + answer = null; + } + answers.set(diagnosticCommandKey(plan.command, plan.args), answer); + }), + ); + // A command that was not planned reads as unreadable rather than silently + // falling back to a blocking spawn, which would give back the freeze this + // exists to remove. The plan and the collector share the builders above, so + // there is nothing to miss. + return (command, commandArgs) => answers.get(diagnosticCommandKey(command, commandArgs)) ?? null; +} + /** * How this machine's background service is defined, per platform. * @@ -306,27 +470,13 @@ function collectServiceDefinition(args: { resolveWindowsServiceLauncherPath({ env: args.env, serviceName }), ), ]; - // Both the task name (which folds in the Windows user) and locating - // PowerShell can throw on a broken box; neither may take the report with - // it, and the reader still has to be told we looked. - const task = (() => { - try { - const taskName = resolveWindowsTaskName({ serviceName }); - return { - taskName, - shell: windowsPowerShellCommand(), - args: buildWindowsExportTaskArgs(taskName), - }; - } catch { - return null; - } - })(); + const task = windowsScheduledTaskPlan(args.env); entries.push( task ? readCommandOutput( "Scheduled task", - `Export-ScheduledTask -TaskName "${task.taskName}"`, - task.shell, + task.display, + task.command, task.args, args.run, { maxBytes: SERVICE_DEFINITION_MAX_BYTES }, @@ -376,21 +526,19 @@ function collectServiceOutputLogs(args: { readLogTail("Background service (stdout)", path.join(args.runtimeDir, "launchd.out.log")), ]; } - const serviceName = resolveRuntimeServiceName(args.env); - const unit = `${serviceName}.service`; - const display = `journalctl --user-unit ${unit}`; + const journal = journalPlan(args.env, args.homeDir); // Gated on the unit file, so a machine that never installed the service is // never charged a subprocess to be told nothing — the definition section // already reports the missing unit, which is the more useful fact anyway. - if (!fs.existsSync(systemdUnitPath(args.homeDir, serviceName))) { - return [{ label: "Background service (journal)", path: display, error: "(not present)" }]; + if (!journal.installed) { + return [{ label: "Background service (journal)", path: journal.display, error: "(not present)" }]; } return [ readCommandOutput( "Background service (journal)", - display, - "journalctl", - ["--user-unit", unit, "--no-pager", "--lines", "200"], + journal.display, + journal.command, + journal.args, args.run, ), ]; @@ -504,3 +652,36 @@ export function collectMachineDiagnosticSources( redaction: diagnosticRedactionContext(projectRoot), }; } + +/** + * The same sources, without stopping the process to gather them. + * + * Identical output to {@link collectMachineDiagnosticSources} — the file reads + * are the same synchronous ones, and they are measured in milliseconds — but + * the external commands are run first and awaited, so nothing here blocks for + * the seconds a PowerShell `Export-ScheduledTask` or a `journalctl` can take. + * This is the entry point for a process that is serving something else while a + * report is built — today Electron's main process, where the block froze every + * window and every IPC call. A one-shot `ade report-issue` has nothing to hold + * up and keeps the plain synchronous one. + * + * The platform inputs are resolved here, once, and handed to both halves, so + * the commands that are pre-run are exactly the commands that get asked for. + */ +export async function collectMachineDiagnosticSourcesAsync( + options: MachineDiagnosticSourceOptions & { + /** Test seam for the prefetch; production spawns for real. */ + runCommandAsync?: ( + command: string, + args: readonly string[], + ) => Promise<{ status: number | null; stdout: string } | null>; + } = {}, +): Promise { + const env = options.env ?? process.env; + const platform = options.platform ?? process.platform; + const homeDir = options.homeDir ?? os.homedir(); + const runCommand = options.runCommand + ? options.runCommand + : await prefetchDiagnosticCommands({ env, platform, homeDir, run: options.runCommandAsync }); + return collectMachineDiagnosticSources({ ...options, env, platform, homeDir, runCommand }); +} diff --git a/apps/desktop/scripts/windows-installed-product-smoke.ps1 b/apps/desktop/scripts/windows-installed-product-smoke.ps1 index c0376ac9c..12452b289 100644 --- a/apps/desktop/scripts/windows-installed-product-smoke.ps1 +++ b/apps/desktop/scripts/windows-installed-product-smoke.ps1 @@ -72,41 +72,85 @@ function Stop-LaunchedApp { $script:launchedApp = $null } +# Is this process the installed product's own executable? +# +# A function rather than an inline filter because the question is asked TWICE +# about the same PID - once to decide what to kill, and again to decide whether +# a failed kill actually left something behind - and two spellings of "is this +# ours" is how the second one drifts into accepting anything. +function Test-IsInstalledAppProcess($Process, [string]$NormalizedAppExe) { + $executablePath = [string]$Process.ExecutablePath + if ([string]::IsNullOrWhiteSpace($executablePath)) { return $false } + try { + return [string]::Equals( + [IO.Path]::GetFullPath($executablePath), + $NormalizedAppExe, + [StringComparison]::OrdinalIgnoreCase + ) + } catch { + # A path this cannot even normalize is not the one we installed. + return $false + } +} + +# Same question for the channel's brain supervisor, which is identified by the +# launcher it was started with rather than by its image path. +function Test-IsChannelSupervisorProcess($Process, [string]$LauncherPrefix) { + if (([string]$Process.Name) -notmatch '^powershell(?:\.exe)?$') { return $false } + return ([string]$Process.CommandLine).IndexOf($LauncherPrefix, [StringComparison]::OrdinalIgnoreCase) -ge 0 +} + +# Re-reads one PID after a `taskkill` that reported failure. +# +# `Readable` is the part that matters: an empty answer and an unreadable process +# table are the same value out of `Get-CimInstance`, and treating "we could not +# look" as "it is gone" would let a kill this script never verified pass as a +# success. So the two are reported apart, and the caller fails closed on the +# second. The process itself comes back rather than a verdict because Windows +# hands a freed PID to whoever asks next: only the caller's own ownership test +# can say whether the thing wearing the number is the thing it tried to kill. +function Get-ProcessAfterKill([string]$TargetProcessId) { + try { + $found = @(Get-CimInstance Win32_Process -Filter "ProcessId = $TargetProcessId" -ErrorAction Stop) + } catch { + return @{ Readable = $false; Process = $null } + } + return @{ Readable = $true; Process = $(if ($found.Count -gt 0) { $found[0] } else { $null }) } +} + function Stop-InstalledProductProcesses { $normalizedAppExe = [IO.Path]::GetFullPath($appExe) $channelAdeHome = Join-Path ([Environment]::GetFolderPath("UserProfile")) $homeName $launcherPrefix = Join-Path $channelAdeHome "runtime\brain-service-" $allProcesses = @(Get-CimInstance Win32_Process -ErrorAction Stop) - $supervisors = @($allProcesses | Where-Object { - $_.Name -match '^powershell(?:\.exe)?$' -and - ([string]$_.CommandLine).IndexOf($launcherPrefix, [StringComparison]::OrdinalIgnoreCase) -ge 0 - }) + $supervisors = @($allProcesses | Where-Object { Test-IsChannelSupervisorProcess $_ $launcherPrefix }) foreach ($supervisor in $supervisors) { if ((Invoke-TaskKill ([string]$supervisor.ProcessId)) -ne 0) { # The process list is a snapshot, so a supervisor can exit on its own # between the snapshot and the kill - which is the state we wanted. Only # a PID that is still there AND still the channel-owned supervisor is a # real failure. - $remaining = Get-CimInstance Win32_Process -Filter "ProcessId = $($supervisor.ProcessId)" -ErrorAction SilentlyContinue - if ($remaining -and ([string]$remaining.CommandLine).IndexOf($launcherPrefix, [StringComparison]::OrdinalIgnoreCase) -ge 0) { + $after = Get-ProcessAfterKill ([string]$supervisor.ProcessId) + if (-not $after.Readable) { + throw "Could not verify that ADE supervisor $($supervisor.ProcessId) stopped before repair." + } + if ($after.Process -and (Test-IsChannelSupervisorProcess $after.Process $launcherPrefix)) { throw "Could not stop channel-owned ADE supervisor $($supervisor.ProcessId) before repair." } } } - $processes = @($allProcesses | Where-Object { - try { - -not [string]::IsNullOrWhiteSpace($_.ExecutablePath) -and - [string]::Equals( - [IO.Path]::GetFullPath([string]$_.ExecutablePath), - $normalizedAppExe, - [StringComparison]::OrdinalIgnoreCase - ) - } catch { $false } - }) + $processes = @($allProcesses | Where-Object { Test-IsInstalledAppProcess $_ $normalizedAppExe }) foreach ($process in $processes) { if ((Invoke-TaskKill ([string]$process.ProcessId)) -ne 0) { - $remaining = Get-CimInstance Win32_Process -Filter "ProcessId = $($process.ProcessId)" -ErrorAction SilentlyContinue - if ($remaining) { + # Same snapshot race, and the same rule as the loop above: "some process + # has this PID" is not "our process is still running", because the PID may + # have been recycled the moment it exited. Only the installed product's + # own executable is a real failure here. + $after = Get-ProcessAfterKill ([string]$process.ProcessId) + if (-not $after.Readable) { + throw "Could not verify that ADE process $($process.ProcessId) stopped before repair." + } + if ($after.Process -and (Test-IsInstalledAppProcess $after.Process $normalizedAppExe)) { throw "Could not stop channel-owned ADE process $($process.ProcessId) before repair." } } diff --git a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 index 9b67fe677..da62e1e92 100644 --- a/apps/desktop/scripts/windows-uninstall-cleanup.ps1 +++ b/apps/desktop/scripts/windows-uninstall-cleanup.ps1 @@ -214,8 +214,8 @@ function Remove-ChannelStartupWithoutPackagedCli( $killExitCode = $LASTEXITCODE $global:LASTEXITCODE = 0 # Best effort is not the same as unsaid. An uninstall must not refuse - # to finish over a process it could not stop — the PID record and the - # launcher are removed either way — but a supervisor that is STILL + # to finish over a process it could not stop - the PID record and the + # launcher are removed either way - but a supervisor that is STILL # running after the kill is the one case worth a line, because the # user is about to be told the product was removed. if ($killExitCode -ne 0) { diff --git a/apps/desktop/scripts/windows-uninstall-cleanup.test.mjs b/apps/desktop/scripts/windows-uninstall-cleanup.test.mjs index 5529ee8a9..a2944c6f7 100644 --- a/apps/desktop/scripts/windows-uninstall-cleanup.test.mjs +++ b/apps/desktop/scripts/windows-uninstall-cleanup.test.mjs @@ -6,6 +6,7 @@ import test from "node:test"; import { spawnSync } from "node:child_process"; const cleanupScript = path.resolve("scripts", "windows-uninstall-cleanup.ps1"); +const installedProductSmokeScript = path.resolve("scripts", "windows-installed-product-smoke.ps1"); const cliWrapperScript = path.resolve("scripts", "ade-cli-windows-wrapper.cmd"); const installSetupScript = path.resolve("scripts", "windows-install-setup.ps1"); const standaloneInstallerScript = path.resolve("..", "ade-cli", "scripts", "install-runtime.ps1"); @@ -111,6 +112,71 @@ test("Windows standalone installer path normalizer is executable PowerShell", { assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); }); +/** + * The two desktop-owned PowerShell scripts, parsed on a real host. + * + * Neither has a harness anywhere else: the uninstall cleanup only runs from an + * NSIS uninstaller and the installed-product smoke only runs on a packaging + * runner, so a syntax error in either surfaces during an uninstall or a release + * rather than in CI. Parsing is not execution, so this costs a process and + * proves the file is at least PowerShell. + */ +test("Windows desktop PowerShell scripts parse", { + skip: process.platform !== "win32", +}, () => { + for (const script of [cleanupScript, installedProductSmokeScript]) { + const probe = [ + "$tokens = $null", + "$errors = $null", + "$null = [Management.Automation.Language.Parser]::ParseFile($env:ADE_TEST_SCRIPT, [ref]$tokens, [ref]$errors)", + "if ($errors.Count -ne 0) { $errors | ForEach-Object { Write-Output $_.Message }; exit 2 }", + ].join("; "); + const result = spawnSync("powershell.exe", [ + "-NoProfile", + "-NonInteractive", + "-Command", + probe, + ], { + encoding: "utf8", + env: { ...process.env, ADE_TEST_SCRIPT: script }, + }); + assert.equal(result.status, 0, `${script}\n${result.stdout}\n${result.stderr}`); + } +}); + +test("Windows installed-product smoke re-checks a failed kill with the test that selected it", { + skip: process.platform !== "win32", +}, () => { + // `taskkill` exits nonzero both when the process is already gone and when it + // could not be stopped, so the smoke re-reads the PID. Windows hands a freed + // PID straight to the next process that asks, which makes "something answers + // to this number" worthless on its own: the re-read has to apply the SAME + // ownership test that chose the process, or a recycled PID fails a smoke that + // actually passed. Both helpers are therefore called twice - once to select, + // once to verify - and this counts the calls so a future edit cannot quietly + // drop the second one. + const probe = [ + "$tokens = $null", + "$errors = $null", + "$ast = [Management.Automation.Language.Parser]::ParseFile($env:ADE_TEST_SCRIPT, [ref]$tokens, [ref]$errors)", + "if ($errors.Count -ne 0) { exit 2 }", + "$names = @($ast.FindAll({ param($node) $node -is [Management.Automation.Language.CommandAst] }, $true) | ForEach-Object { $_.GetCommandName() })", + "if (@($names | Where-Object { $_ -eq 'Test-IsInstalledAppProcess' }).Count -lt 2) { exit 3 }", + "if (@($names | Where-Object { $_ -eq 'Test-IsChannelSupervisorProcess' }).Count -lt 2) { exit 4 }", + "if (@($names | Where-Object { $_ -eq 'Get-ProcessAfterKill' }).Count -lt 2) { exit 5 }", + ].join("; "); + const result = spawnSync("powershell.exe", [ + "-NoProfile", + "-NonInteractive", + "-Command", + probe, + ], { + encoding: "utf8", + env: { ...process.env, ADE_TEST_SCRIPT: installedProductSmokeScript }, + }); + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); +}); + test("Windows uninstall cleanup removes only CLI shims owned by this installation", { skip: process.platform !== "win32", }, (t) => { diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts index e260dc6c3..366e6c483 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.test.ts @@ -335,6 +335,44 @@ describe("manual diagnostics budget", () => { expect(isAutoDiagnosticsEnabled(filePath)).toBe(false); }); + it("completes distinct manual reservations claimed in the same millisecond", () => { + const filePath = stateFile(); + + // Manual sends have no failure class, so they ALL carry `user_requested` + // and the timestamp is the only thing left to tell two of them apart. Two + // that shared one would be a single reservation as far as completion is + // concerned: the second annotation would land on the first one's entry, + // overwriting its path and reference and leaving its own blank. + const first = claimManual(filePath, T0); + const second = claimManual(filePath, T0); + expect(first.allowed).toBe(true); + expect(second.allowed).toBe(true); + if (!first.allowed || !second.allowed) return; + expect(second.atMs).not.toBe(first.atMs); + + const complete = (atMs: number, reportPath: string, reference: string) => + completeAutoDiagnosticsSend({ + filePath, + failureCode: "user_requested", + atMs, + reportPath, + reference, + // Manual sends are never pending in production — the person is looking + // at the answer. It is set here only because the pending list is the + // one readback for what a completion actually recorded. + pending: true, + kind: "manual", + now: () => T0, + }); + complete(first.atMs, "/tmp/first.md", "aaaa1111"); + complete(second.atMs, "/tmp/second.md", "bbbb2222"); + + expect(listPendingAutoDiagnosticsNotices(filePath)).toEqual([ + { failureCode: "user_requested", reportPath: "/tmp/first.md", reference: "aaaa1111" }, + { failureCode: "user_requested", reportPath: "/tmp/second.md", reference: "bbbb2222" }, + ]); + }); + it("only annotates an entry of the kind that was claimed", () => { const filePath = stateFile(); expect(claimManual(filePath, T0).allowed).toBe(true); diff --git a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts index e746e0d64..0bb304952 100644 --- a/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts +++ b/apps/desktop/src/main/services/diagnostics/autoDiagnosticsStore.ts @@ -513,9 +513,26 @@ export function claimManualDiagnosticsSend(args: { if (recent.filter((entry) => entry.kind === "manual").length >= MAX_MANUAL_DIAGNOSTICS_PER_WINDOW) { return { state: null, result: { allowed: false, reason: "daily_limit" } }; } + // A reservation is identified by (code, atMs, kind) — that triple is what + // `completeAutoDiagnosticsSend` finds its entry by. Automatic sends are + // unique in it by construction, since at most one per code exists in a + // window. Manual ones are not: they ALL carry `user_requested`, so two + // claimed in the same millisecond would be indistinguishable, and the + // second completion would land on the first one's entry — overwriting its + // path and reference and leaving its own blank. Stepping past a collision + // costs nothing against a 24-hour window and keeps every reservation + // individually completable. + let atMs = nowMs; + while (recent.some((entry) => + entry.kind === "manual" + && entry.code === MANUAL_DIAGNOSTICS_FAILURE_CODE + && entry.atMs === atMs) + ) { + atMs += 1; + } const entry: AutoDiagnosticsSend = { code: MANUAL_DIAGNOSTICS_FAILURE_CODE, - atMs: nowMs, + atMs, source: args.source, kind: "manual", reportPath: null, @@ -524,7 +541,7 @@ export function claimManualDiagnosticsSend(args: { }; return { state: { ...state, sends: [...recent, entry].slice(-MAX_RETAINED_SENDS) }, - result: { allowed: true, atMs: nowMs }, + result: { allowed: true, atMs }, }; }, () => ({ allowed: false, reason: "state_unavailable" }), @@ -534,6 +551,12 @@ export function claimManualDiagnosticsSend(args: { /** * Records the result of a claimed send. * + * The reservation is found by (code, atMs, kind) — the triple both claims mint + * uniquely, the automatic one because a code gets a single slot per window and + * the manual one because it steps past a timestamp already taken. Nothing here + * can restore that uniqueness after the fact, which is why it is established + * where the entry is written rather than guessed at here. + * * `pending` is how a send reaches the user's screen at all. The brain has no * renderer and the desktop cannot tell whether one received its notice, so a * successful send is left pending either way, offered to the next renderer that diff --git a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts index e69964164..f1a54b02e 100644 --- a/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts +++ b/apps/desktop/src/main/services/diagnostics/diagnosticReportService.ts @@ -11,7 +11,7 @@ import { type DiagnosticVolumeSpace, } from "../../../../../ade-cli/src/services/diagnostics/diagnosticReport"; import { - collectMachineDiagnosticSources, + collectMachineDiagnosticSourcesAsync, readLogTail, } from "../../../../../ade-cli/src/services/diagnostics/diagnosticSources"; import { readVolumeSpace } from "../storage/volume"; @@ -171,14 +171,6 @@ export async function collectDiagnosticReport( const env = deps.env ?? process.env; const at = deps.now?.() ?? new Date(); const projectRoot = request.projectRoot?.trim() || null; - // Logs, volumes, notes and the redaction context are the same set the - // headless `ade report-issue` collects; the Electron-only extras below are - // the only thing this report adds. - const sources = collectMachineDiagnosticSources({ - env, - projectRoot, - readVolume: volumeEntry, - }); // Both optional steps ask the very subsystem the user is reporting as broken // -- the local runtime, and a recovery diagnosis that probes the brain's @@ -186,7 +178,17 @@ export async function collectDiagnosticReport( // leave the "Report issue" button spinning, which is exactly the outcome // this collector promises can never happen. A synchronous throw out of // either one is caught here for the same reason. - const [osProductVersion, localRuntimeStatus, recoveryDiagnosis] = await Promise.all([ + // + // The machine sources go in the same batch, and through the ASYNC collector: + // logs, volumes, notes and the redaction context are the same set the + // headless `ade report-issue` collects, but this is Electron's main process, + // so the platform commands behind them (a PowerShell `Export-ScheduledTask`, + // a `journalctl`) may not be spawned synchronously — they would freeze every + // window and every IPC call for as long as they take, on a report the user + // often did not ask for. The Electron-only extras below are the only thing + // this report adds. + const [sources, osProductVersion, localRuntimeStatus, recoveryDiagnosis] = await Promise.all([ + collectMachineDiagnosticSourcesAsync({ env, projectRoot, readVolume: volumeEntry }), readMacProductVersion().catch(() => null), bestEffortStep(() => deps.getLocalRuntimeStatus?.() ?? null, deps.stepTimeoutMs), projectRoot && deps.diagnoseProject diff --git a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx index e86cd2d21..191359f47 100644 --- a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx +++ b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.test.tsx @@ -113,6 +113,34 @@ describe("DiagnosticsSharingSection manual send", () => { } }); + it("does not claim a local report copy exists when an oversized report cannot be saved", async () => { + // The local copy is written before the upload is attempted, so a `too_large` + // refusal usually comes back with a path — and then the sentence about + // opening it is true and the button that does so is there. + mountSection({ + sendManual: async () => ({ ok: false, reason: "too_large", reportPath: "/tmp/report.md" }), + }); + await pressSend(); + await waitFor(() => { + expect(screen.getByRole("status").textContent).toContain("It's saved on this computer"); + }); + expect(screen.getByRole("button", { name: "View report" })).toBeTruthy(); + cleanup(); + + // When the copy could not be written the main process answers without one, + // and telling this user to open a file sends them looking for something + // that is not there. + mountSection({ sendManual: async () => ({ ok: false, reason: "too_large" }) }); + await pressSend(); + await waitFor(() => { + expect(screen.getByRole("status").textContent).toContain( + "This report is too big to send, and ADE couldn't save a copy on this computer.", + ); + }); + expect(screen.getByRole("status").textContent).not.toContain("It's saved on this computer"); + expect(screen.queryByRole("button", { name: "View report" })).toBeNull(); + }); + it("says out loud that a click does not turn automatic reports back on", async () => { mountSection({ sharing: { ...SHARING_ON, enabled: false } }); diff --git a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx index 3bc5a292c..3fa8acab6 100644 --- a/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx +++ b/apps/desktop/src/renderer/components/settings/DiagnosticsSharingSection.tsx @@ -82,7 +82,14 @@ function describeManualSendFailure(result: Extract { await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl: fleet.fetchImpl })) .resolves.toEqual({ ok: false, reason: "unavailable" }); - // A body that cannot be read falls back to the caller-scoped reading: the - // conservative one, since it only ever asks the user to wait. + // An EMPTY body falls back to the caller-scoped reading: the conservative + // one, since it only ever asks the user to wait. const unreadable = capture(new Response(null, { status: 429 })); await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl: unreadable.fetchImpl })) .resolves.toEqual({ ok: false, reason: "rate_limited" }); }); + it("falls back to the caller's own limit when the 429 body throws instead of arriving", async () => { + // `new Response(null, …)` above is an empty body, not an unreadable one: + // its `text()` resolves to "" and never enters the catch. A stream that + // aborts mid-read does, and it has to reach the same conservative answer + // rather than an exception on a screen that is already showing a failure. + const rejectingBody = { + status: 429, + ok: false, + text: async () => { + throw new Error("aborted"); + }, + } as unknown as Response; + const { fetchImpl } = capture(rejectingBody); + + await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl })) + .resolves.toEqual({ ok: false, reason: "rate_limited" }); + }); + it("treats an unusable success body as a failure rather than inventing a reference", async () => { const { fetchImpl } = capture(new Response("not json", { status: 200 })); await expect(uploadDiagnosticReport({ report: REPORT, baseUrl: BASE_URL, fetchImpl })) From dcb32714728c211239be6da83c235436c52ba778 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:42:49 -0400 Subject: [PATCH 9/9] fix(diagnostics): stop the brain freezing while it builds a report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collector shells out — journalctl on Linux, Export-ScheduledTask on Windows — with a 4s cap per command. The desktop was moved off the synchronous path in the previous commit, but the brain's automatic sender still called buildCliDiagnosticReport synchronously, so it froze its own event loop mid-RPC for up to 4s to build a report nobody asked for. runAutoDiagnosticsSend already awaited `build`, so the sender only needed an async builder to default to. Everything after collection is shared between the two builders rather than duplicated, and a parity test pins them to the same output — a report whose contents depend on which process sent it would defeat the point of collecting it. Co-Authored-By: Claude Opus 5 --- apps/ade-cli/src/commands/reportIssue.test.ts | 22 +++++++ apps/ade-cli/src/commands/reportIssue.ts | 57 +++++++++++++++++-- .../diagnostics/autoDiagnosticsSender.ts | 4 +- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/apps/ade-cli/src/commands/reportIssue.test.ts b/apps/ade-cli/src/commands/reportIssue.test.ts index 9a2306e1b..b6814b8d3 100644 --- a/apps/ade-cli/src/commands/reportIssue.test.ts +++ b/apps/ade-cli/src/commands/reportIssue.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { MAX_DIAGNOSTIC_UPLOAD_BYTES } from "../../../desktop/src/shared/diagnosticsUpload"; import { buildCliDiagnosticReport, + buildCliDiagnosticReportAsync, buildReportIssuePayload, describeDiagnosticUpload, openDiagnosticIssue, @@ -32,6 +33,27 @@ afterEach(() => { for (const dir of tempDirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); }); +describe("buildCliDiagnosticReportAsync", () => { + // Two builders exist because the brain must not block its event loop on the + // collector's subprocesses, while the one-shot CLI has nothing to hold up. + // The only thing that keeps that split honest is that they produce the same + // report; a difference here is a report whose contents depend on which + // process sent it. + it("produces the same report as the synchronous builder", async () => { + const env = { ADE_HOME: adeHome({ identifiedUserHash: "hash-parity" }) }; + const at = () => new Date("2026-08-19T22:30:00.000Z"); + + const sync = buildCliDiagnosticReport({ env, cliVersion: "1.2.62", now: at }); + const async_ = await buildCliDiagnosticReportAsync({ env, cliVersion: "1.2.62", now: at }); + + expect(async_.report).toBe(sync.report); + expect(async_.installId).toBe(sync.installId); + expect(async_.issueUrl).toBe(sync.issueUrl); + expect(async_.reportsDir).toBe(sync.reportsDir); + expect(async_.secretsDir).toBe(sync.secretsDir); + }); +}); + describe("buildCliDiagnosticReport", () => { it("reports the PostHog distinct_id and a prefilled issue URL", () => { const built = buildCliDiagnosticReport({ diff --git a/apps/ade-cli/src/commands/reportIssue.ts b/apps/ade-cli/src/commands/reportIssue.ts index a5d735bb4..4a7f671fa 100644 --- a/apps/ade-cli/src/commands/reportIssue.ts +++ b/apps/ade-cli/src/commands/reportIssue.ts @@ -9,6 +9,7 @@ import { } from "../services/diagnostics/diagnosticReport"; import { collectMachineDiagnosticSources, + collectMachineDiagnosticSourcesAsync, readDiagnosticJsonFile, } from "../services/diagnostics/diagnosticSources"; import { @@ -86,13 +87,20 @@ function readInstallId(secretsDir: string): string | null { return null; } -export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): ReportIssueResult { +/** + * The two builders below differ only in how they collect: a one-shot + * `ade report-issue` has nothing to hold up and stays synchronous, while a + * long-lived process must not block its event loop on the collector's + * subprocesses. Everything after collection is identical, so it lives here + * rather than being duplicated and drifting. + */ +function finishCliDiagnosticReport( + options: ReportIssueOptions, + inputs: { at: Date; projectRoot: string | null; surface: string }, + sources: ReturnType, +): ReportIssueResult { + const { at, projectRoot, surface } = inputs; const env = options.env ?? process.env; - const at = options.now?.() ?? new Date(); - const projectRoot = options.projectRoot?.trim() || null; - const surface = options.surface?.trim() || "cli"; - - const sources = collectMachineDiagnosticSources({ env, projectRoot }); const installId = readInstallId(sources.layout.secretsDir) ?? "unknown"; const report = buildDiagnosticReport({ @@ -142,6 +150,43 @@ export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): Repo }; } +function resolveCliReportInputs(options: ReportIssueOptions): { + at: Date; + projectRoot: string | null; + surface: string; +} { + return { + at: options.now?.() ?? new Date(), + projectRoot: options.projectRoot?.trim() || null, + surface: options.surface?.trim() || "cli", + }; +} + +export function buildCliDiagnosticReport(options: ReportIssueOptions = {}): ReportIssueResult { + const env = options.env ?? process.env; + const inputs = resolveCliReportInputs(options); + const sources = collectMachineDiagnosticSources({ env, projectRoot: inputs.projectRoot }); + return finishCliDiagnosticReport(options, inputs, sources); +} + +/** + * The builder for processes that stay alive. The brain sends automatic reports + * while it is serving RPC, and the collector shells out (`journalctl` on Linux, + * `Export-ScheduledTask` on Windows) with a 4s cap per command — synchronously, + * that is 4s of a frozen event loop for a report nobody asked for. + */ +export async function buildCliDiagnosticReportAsync( + options: ReportIssueOptions = {}, +): Promise { + const env = options.env ?? process.env; + const inputs = resolveCliReportInputs(options); + const sources = await collectMachineDiagnosticSourcesAsync({ + env, + projectRoot: inputs.projectRoot, + }); + return finishCliDiagnosticReport(options, inputs, sources); +} + /** * The side effects of `ade report-issue --open`, in the order the desktop * "Report issue" button does them: the report goes on the clipboard, *then* diff --git a/apps/ade-cli/src/services/diagnostics/autoDiagnosticsSender.ts b/apps/ade-cli/src/services/diagnostics/autoDiagnosticsSender.ts index 9f1fa9242..7f52e254d 100644 --- a/apps/ade-cli/src/services/diagnostics/autoDiagnosticsSender.ts +++ b/apps/ade-cli/src/services/diagnostics/autoDiagnosticsSender.ts @@ -8,7 +8,7 @@ import { import { resolveAutoDiagnosticsStateFile } from "../../../../desktop/src/main/services/diagnostics/autoDiagnosticsStore"; import type { ProductAnalyticsCapture } from "../../../../desktop/src/shared/types/productAnalytics"; import { - buildCliDiagnosticReport, + buildCliDiagnosticReportAsync, sendDiagnosticReport, type ReportIssueResult, } from "../../commands/reportIssue"; @@ -101,7 +101,7 @@ export function createBrainAutoDiagnostics( const layout = resolveMachineAdeLayout(env); const stateFilePath = deps.stateFilePath ?? resolveAutoDiagnosticsStateFile(layout.adeDir, env); const reportsDir = deps.reportsDir ?? path.join(layout.adeDir, "diagnostic-reports"); - const build = deps.build ?? buildCliDiagnosticReport; + const build = deps.build ?? buildCliDiagnosticReportAsync; const send = deps.send ?? sendDiagnosticReport; const writeReportFile = deps.writeReportFile ?? writeDiagnosticReportFile; let inFlight = false;