From 388f2d2dca7951e91cdeea2f2ef7b92dce10c121 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 7 May 2026 13:30:11 -0700 Subject: [PATCH] WIP: Changes before auto-review fixes Co-authored-by: Orca --- 00-design-review-context.md | 41 +++++ 00-review-context.md | 88 ++++------ docs/onboarding-funnel-cohort-addendum.md | 152 ++++++++++++++++++ .../ipc/worktree-create-error-class.test.ts | 28 +++- src/main/ipc/worktree-create-error-class.ts | 29 ++-- 5 files changed, 269 insertions(+), 69 deletions(-) create mode 100644 00-design-review-context.md create mode 100644 docs/onboarding-funnel-cohort-addendum.md diff --git a/00-design-review-context.md b/00-design-review-context.md new file mode 100644 index 00000000000..f2c9712520d --- /dev/null +++ b/00-design-review-context.md @@ -0,0 +1,41 @@ +# Design Review Context + +## Document Info +- Path: docs/onboarding-funnel-cohort-addendum.md +- Type: Technical Spec (Addendum to onboarding-funnel-telemetry.md) +- Review started: 2026-05-07 + +## Design Direction +- Direction confirmed: pending +- Chosen approach: Add two booleans (`is_first_repo`, `is_first_workspace`) as cohort properties on 7 onboarding-related events. No new events. +- Alternatives considered: TBD (Phase 0.5) +- Key UX decisions: TBD (Phase 0.5) + +## Iteration State +Current iteration: 1 +Last completed phase: Setup + +## Addressed Issues (Do Not Re-report) + + + +[Initially empty - populated after each fix phase] + +## Skipped Issues (Accepted Risks) + + + + +[Initially empty - populated during triage phase] + +## Invalidated Findings (Do Not Re-report) + + + + +[Initially empty - populated after each validation phase] + +## Findings History + + +[Initially empty] diff --git a/00-review-context.md b/00-review-context.md index 05b4a081e1c..6e3eb39112d 100644 --- a/00-review-context.md +++ b/00-review-context.md @@ -2,79 +2,59 @@ ## Branch Info -- Base: origin/main (b5d2b288 — branch is at HEAD of main, all changes are uncommitted) +- Base: origin/main - Current: brennanb2025/posthog-checkin -- All "diff" is uncommitted changes (no commits yet on top of main) ## Changed Files Summary +- A docs/onboarding-funnel-telemetry.md +- A src/main/ipc/worktree-create-error-class.test.ts +- A src/main/ipc/worktree-create-error-class.ts - M src/main/ipc/worktrees.ts - M src/renderer/src/components/sidebar/AddRepoDialog.tsx - M src/shared/telemetry-events.test.ts - M src/shared/telemetry-events.ts -- A docs/onboarding-funnel-telemetry.md (untracked) -- A src/main/ipc/worktree-create-error-class.ts (untracked) -- A src/main/ipc/worktree-create-error-class.test.ts (untracked) ## Changed Line Ranges (PR Scope) - - -| File | Changed Lines | -|------|---------------| -| src/main/ipc/worktrees.ts | 43 (added import), 162-189 (added try/catch + telemetry), 198 (removed line) | -| src/renderer/src/components/sidebar/AddRepoDialog.tsx | 14 (import), 197, 215, 224-232 (handleSetupStepBack), 258 (onClick rebind), 412 (skip onClick) | -| src/shared/telemetry-events.ts | 76-102 (new schemas), 217-231 (event schemas), 249, 251 (registry entries) | -| src/shared/telemetry-events.test.ts | 109-162 (new test blocks) | -| src/main/ipc/worktree-create-error-class.ts | ALL (new file, 1-44) | -| src/main/ipc/worktree-create-error-class.test.ts | ALL (new file, 1-51) | -| docs/onboarding-funnel-telemetry.md | ALL (new file) | - -## Review Standards Reference - -- Follow /review-code standards -- Focus on: correctness, security, performance, maintainability -- Priority levels: Critical > High > Medium > Low - -## File Categories - -### Backend/IPC -- src/main/ipc/worktrees.ts -- src/main/ipc/worktree-create-error-class.ts -- src/main/ipc/worktree-create-error-class.test.ts - -### Frontend/UI -- src/renderer/src/components/sidebar/AddRepoDialog.tsx - -### Utility/Common -- src/shared/telemetry-events.ts -- src/shared/telemetry-events.test.ts - -### Docs (skipped from code review; reviewed only as context) -- docs/onboarding-funnel-telemetry.md +| File | Changed Lines | +| --------------------------------------------------- | -------------------------------------- | +| docs/onboarding-funnel-telemetry.md | 1-174 (new file) | +| src/main/ipc/worktree-create-error-class.test.ts | 1-71 (new file) | +| src/main/ipc/worktree-create-error-class.ts | 1-55 (new file) | +| src/main/ipc/worktrees.ts | 43, 162-189 | +| src/renderer/src/components/sidebar/AddRepoDialog.tsx | 14, 197, 215, 224-232, 258, 412 | +| src/shared/telemetry-events.test.ts | 109-162 | +| src/shared/telemetry-events.ts | 76-102, 217-231, 249, 251 | ## Skipped Issues (Do Not Re-validate) -- worktree-create-error-class.ts:17-20 | Low | Hypothetical Buffer-stderr handling. Current throws all use bare Error('...') without .stderr; existing code does not need this defensive code per "don't add validation for scenarios that can't happen". | Buffer stderr handling missing -- worktrees.ts:168 | Low | Optional comment about future single-caller assumption. Not a correctness issue. | Comment about future caller of worktrees:create -- AddRepoDialog.tsx:411-414 | Low | Inline arrow onSkip is purely consistency nit; no functional impact. | Inline onSkip arrow not memoized -- AddRepoDialog.tsx:411-414, :197, :215, :228-231 | Low | Double-click guards: track is fire-and-forget; button unmounts on close; impact bounded; precedent (appOpenedTrackedThisSession) is only for useEffect-driven emits. | Skip/Open/Configure/Back double-click guards -- telemetry-events.test.ts:115 | Low | Test loop type (use addRepoSetupStepActionSchema.options); maintainability nit only — test still runtime-fails on typo. | Test loop should use schema.options -- telemetry-events.test.ts:137-165 | Low | Missing source enum rejection test; redundant with workspace_created coverage of same schema. | Missing source enum test -- telemetry-events.ts:87 | Low | Unused export AddRepoSetupStepAction; consistent with peer-type exports (RepoMethod, OptInVia). No fix needed per agent. | Unused type export -- worktrees.ts:168 | Low | Optional comment about emit placement vs pre-validation throws. | Optional emit-placement comment - -## Issues Validated as False Positive - -- worktrees.ts:182 (Codex): track() in catch wrapping concern. track() does not throw — `validate()` returns a result object, posthog.capture is internally guarded, all early-returns inside track() prevent throws. Existing pattern across the codebase wraps no track() call. FP. +- worktree-create-error-class.ts:17-20 | Low | Hypothetical Buffer-stderr handling. Current throws all use bare Error('...') without .stderr. +- worktrees.ts:168 | Low | Optional comment about future single-caller assumption; not a correctness issue. +- AddRepoDialog.tsx:411-414 | Low | Inline arrow onSkip is purely consistency nit; no functional impact. +- AddRepoDialog.tsx:411-414, :197, :215, :228-231 | Low | Double-click guards: track is fire-and-forget; button unmounts on close. +- telemetry-events.test.ts:115 | Low | Test loop type (use schema.options); maintainability nit. +- telemetry-events.test.ts:137-165 | Low | Missing source enum rejection test; redundant with workspace_created coverage. +- telemetry-events.ts:87 | Low | Unused export AddRepoSetupStepAction; consistent with peer-type exports. +- worktrees.ts:168 | Low | Optional comment about emit placement vs pre-validation throws. +- docs/onboarding-funnel-telemetry.md | Low | Multiple stale line citations (telemetry-events.ts, AddRepoDialog.tsx, worktrees.ts, client.ts). Cosmetic doc drift; design doc is for humans, not load-bearing for runtime; symbols stable enough to find via grep. +- docs/onboarding-funnel-telemetry.md:124 | Low | Doc forward-look claim that workspace_composer_opened is "mostly redundant" — debatable but doc-only Phase-2 sketch, not implementation. +- worktree-create-error-class.ts:19 | Low | Only stderr harvested, not cause/stdout/output. Existing throws all use bare Error('...'); no real-world failures fall through today. +- worktrees.ts:188 | Low | track() then throw — track() is a sync void function (client.ts:256). FP. ## Issues to Fix (Phase 3) -1. **High** worktree-create-error-class.ts:18-43 — Case-sensitivity gap: `text.includes('worktree')` is lowercase but real throw site at worktree-remote.ts:203,441 is `'Worktree created but not found in listing'` (capital W). Found by: Claude (×2). Fix: lowercase-normalize the text once before substring checks. +1. **Medium** worktrees.ts:168 — `add_repo_setup_step_action.create_worktree` fires for every `worktrees:create` IPC call, regardless of source. Funnel question is "fraction of users who add a repo bail vs continue", but `worktrees:create` is invoked from many surfaces (WorktreeJumpPalette, useComposerState, launch-work-item-direct, Landing, SidebarHeader, TaskPage). Other 4 actions (skip/configure/open_existing/back) fire only from AddRepoSetupStep. Found by: Claude (×2), Codex. + Fix: Move `create_worktree` emit to the renderer, in `handleCreateWorktree` at AddRepoDialog.tsx:204-212, mirroring the other 4 affordances. The SetupStep "Create" click already has a clean renderer-side seam there. Remove the main-side emit at worktrees.ts:168 and update its comment. -2. **Medium** worktree-create-error-class.ts:40 — Overly broad `'git '` and `'worktree'` substrings cause SSH relay errors (worktree-remote.ts:190, "The SSH relay has not registered the worktree path yet") to bucket into `git_failed` instead of `unknown`. Found by: Claude (×2). Fix: replace broad anchors with specific markers (`'fatal:'`, `'git worktree'`, `'created but not found'`, `'no git provider'`). +2. **Medium** worktree-create-error-class.ts:28 — `'already exists'` is too broad: matches Orca's deliberate `Branch "X" already exists` (worktree-remote.ts:97) AND the `orca-runtime.ts:2909` `Branch "X" already exists locally/on a remote` AND any future git-native `fatal: ... already exists` message. Current code already keeps `'already has pr'` and `'could not find an available worktree name'` as specific markers. Found by: Claude. + Fix: Replace the bare `'already exists'` substring with the two concrete suffixes Orca actually throws — `'already exists locally'` and `'already exists on a remote'` — plus the existing `'already has pr'` and `'could not find an available worktree name'`. The bare-`'already exists. Pick a different worktree name.'` throw at worktree-remote.ts:97 also matches `'already exists. pick'` (lowercased). Add that anchor too (or the simpler `'already exists. pick'`). -3. **Medium** worktree-create-error-class.test.ts:1-4 — Header comment over-promises: claims to "fixture the actual throw sites" but only covers 3 of ~10 throw sites in worktree-remote.ts. Found by: Claude. Fix: adjust the comment to honestly describe spot-coverage of load-bearing throws. +3. **Low→Medium** worktree-create-error-class.ts:50 — `'no git provider'` is bucketed as `git_failed`, but the related SSH-precondition error `'SSH connection is not available'` (worktree-remote.ts:148) falls through to `unknown`. Two SSH-precondition errors → two different buckets. Found by: Claude. + Fix: Drop `'no git provider'` from the git_failed list so both SSH-precondition errors consistently bucket to `unknown`. The schema-evolution doctrine prefers narrow + consistent over broad-and-incoherent. + +4. **Medium** AddRepoDialog.tsx:236-240 — Implicit-close on the Setup step (X icon, ESC, outside-click) calls `closeModal()` and `resetState()` without firing `add_repo_setup_step_action`. Functionally equivalent to Skip but invisible to the funnel; under-reports bail-rate. Found by: Claude. + Fix: In `onOpenChange`, when `step === 'setup' && !open`, emit `add_repo_setup_step_action` with `action: 'skip'`. Guard against double-fire when the explicit Skip button (line 411-415) calls `closeModal()` which re-enters `onOpenChange(false)` — use a ref-flag that the explicit Skip handler sets, and the implicit handler reads-and-resets. Alternatively, have the explicit Skip handler NOT call `closeModal()` directly and let the dialog close itself via `onOpenChange`, but that's a larger refactor; the ref-flag is the local minimum. ## Iteration State diff --git a/docs/onboarding-funnel-cohort-addendum.md b/docs/onboarding-funnel-cohort-addendum.md new file mode 100644 index 00000000000..2bba1efefa5 --- /dev/null +++ b/docs/onboarding-funnel-cohort-addendum.md @@ -0,0 +1,152 @@ +# Onboarding Funnel — Cohort Addendum + +Addendum to [`onboarding-funnel-telemetry.md`](./onboarding-funnel-telemetry.md). Adds cohort dimensionality to the events shipping in Phase 1 (and recommends extending two existing events for symmetry). No new events; only new properties. + +## Goal + +Make the funnel queryable by user cohort, not just by event sequence. The headline product question is "where do **new** users drop off?" — not "where do users drop off." The current schema cannot answer the first question without a multi-step session join through every user's full history, because every event fires identically for first-time users and for someone adding their 6th repo. + +A first-time user clicking Skip on the Setup step is "I don't understand what to do here." A returning user with 5 repos clicking Skip is "I'll set this up later." These are not the same UX problem and shouldn't be averaged together. Today they are. + +## Non-goals + +- Not adding session-start or session-age events. App-session boundaries are already inferable from `app_opened` plus the device clock; a dedicated event would duplicate work. +- Not adding time-on-step instrumentation. The funnel is about transitions, not dwell. Dwell is a separate question with a separate cost (clocks, idle detection, pause-vs-active state). +- Not adding a synthetic `onboarding_complete` event. That state is `first agent_started for distinct_id` and is computable in PostHog without a new event. +- Not adding any property that approaches uniqueness — no exact repo count, no first-seen timestamp, no install age. Cohort signals are buckets, not measurements. + +## The cohort signal + +Two booleans, computed at emit time from local state the renderer/main process already has: + +### `is_first_repo: boolean` + +True when, at the moment the event fires, the user has **zero repos** in their local store (i.e., this `repo_added` will be their first, or this Setup-step interaction follows their first `repo_added`). + +Once this becomes false for a `distinct_id`, it should never become true again — repos are not deleted as part of normal onboarding flow. (If a user deletes all their repos and re-adds one, they technically re-enter the cohort. Acceptable: that user is in a re-onboarding state and the funnel question still applies.) + +### `is_first_workspace: boolean` + +True when, at the moment the event fires, the user has **zero workspaces** across all repos. The transition from "has a repo, no workspace" to "has a workspace" is the load-bearing step of onboarding — most of the time-to-value gap lives here. `is_first_workspace` lets us isolate the cohort that's mid-conversion. + +## Where to add the properties + +I audited every event in `telemetry-events.ts:245-260`. The table below covers all events that touch the onboarding journey or whose failure modes plausibly skew by cohort. Events that don't (`settings_changed`, `telemetry_opted_in`, `telemetry_opted_out`) are deliberately excluded — see "Events deliberately not extended" below. + +| Event | `is_first_repo` | `is_first_workspace` | Rationale | +|---|---|---|---| +| `app_opened` (existing) | yes | yes | Session heartbeat. Both booleans here let us answer "what fraction of sessions are by first-time users?" and "of users who ever opened the app, what fraction never reach the funnel?" without a window function over full history. | +| `repo_added` (existing) | yes | yes | Marks "user got a repo." Both booleans needed: `is_first_repo` for first-time vs returning, `is_first_workspace` to identify the unusual cohort that has added repos before but never created a workspace (a real symptom we'd want to see). | +| `add_repo_setup_step_action` (Phase 1) | yes | yes | Which Setup-step action does each cohort take? Headline diagnostic for the 60% drop. | +| `workspace_created` (existing) | yes | yes | Marks "user got a workspace." Both true = onboarding milestone; both false = routine add. The shape that makes onboarding completion measurable. | +| `workspace_create_failed` (Phase 1) | yes | yes | Failure modes likely skew by cohort — first-time users hit `permission_denied` (misconfigured SSH); power users hit `path_collision` (naming conflicts). Cohort split is essential for prioritization. | +| `agent_started` (existing) | yes | yes | End of the onboarding funnel. Both booleans true = "user just completed onboarding," the canonical conversion signal. | +| `agent_error` (existing) | yes | yes | First-time users hitting `binary_not_found` is an onboarding blocker (CLI not installed); power users hitting it is an environment regression. Same enum value, completely different priority — cohort split is what disambiguates. | + +Total: 7 events × 2 properties = 14 property additions. Five of those events already exist; two ship in Phase 1. + +Adding properties to existing events is additive-safe per the schema-evolution doctrine in `telemetry-events.ts:238-244`. Historical events have these properties as `undefined`; dashboards built on the new fields naturally exclude pre-rollout data, which is correct (we don't have ground truth on cohort for pre-rollout users anyway). + +### Events deliberately not extended + +- **`settings_changed`** — settings activity isn't onboarding-specific. A first-time user flipping `experimentalAgentDashboard` and a power user flipping the same toggle are answering different questions, but the cohort dimension doesn't change *what* we'd do with the data. Skip until proven necessary. +- **`telemetry_opted_in` / `telemetry_opted_out`** — opt cohort skew is interesting but orthogonal to onboarding flow. Adding the booleans here would also be slightly self-referential (the consent toggle deciding whether to tag itself). Skip. + +## Why booleans, not buckets + +A bucketed enum (`prior_repo_count: '0' | '1' | '2_4' | '5_plus'`) would carry strictly more information for the same privacy cost. We chose booleans anyway because: + +- The product question is binary: "is this an onboarding moment or a routine action?" Buckets answer a question we don't have yet. +- Adding a bucketed enum later is additive-safe; reducing dimensionality after the fact is not. Start narrow. +- A boolean is unambiguous at the call site. Bucket boundaries (`2_4` vs `5_plus`) require a decision the author has to defend in code review. + +If Phase 1+addendum data shows we want to distinguish "second repo" from "tenth repo," we add `prior_repo_count_bucket` then. Until that question is real, don't pay for it. + +## Where the values come from + +`is_first_repo` is computable from the same store the existing `repo_added` deduplication uses (`repos.ts:41-49`). The repo count is in process memory; no IPC or filesystem call needed. + +`is_first_workspace` is computable from the workspace store. For renderer-side emit sites (the four pure-UI actions in `add_repo_setup_step_action`), the workspace count is already in the renderer's React state. For main-side emit sites (`worktrees:create`), the count is in main's worktree registry. Both paths are O(1) reads — no database query, no async wait. + +The cost is a single field read per emit site. Approximately 2 lines per emit site × 7 emit sites for each boolean ≈ 28 lines beyond the Phase 1 17, plus the schema additions (~7 lines). Total Phase 1 + addendum ≈ 50 LOC. The accessor functions for both booleans are reused across all emit sites — write once. + +## Privacy guardrails + +- **Booleans only.** No counts, no first-seen timestamps, no install dates. The signal is "in-cohort or not" — one bit per dimension, two bits per event. +- **No retroactive backfill.** Historical events keep `is_first_repo: undefined`. The dashboard treats undefined as "unknown cohort," not as a default. We do not run a migration to label past events. +- **No cross-event correlation in code.** The classifier for each event reads its own local state. We do not introduce a "user lifecycle" service that tracks onboarding status across events — that would be an unbounded surface for cohort PII. + +The properties are subject to the same `.strict()` Zod schema enforcement as everything else; an event with a free-text first-repo claim would be dropped by the validator. + +## SQL once this lands + +**The headline question — onboarding-cohort Setup-step funnel:** + +```sql +SELECT + properties.action, + count(*) AS n +FROM events +WHERE event = 'add_repo_setup_step_action' + AND properties.is_first_repo = true + AND timestamp > now() - INTERVAL 30 DAY +GROUP BY properties.action +ORDER BY n DESC +``` + +**Cohort-split workspace creation success rate:** + +```sql +SELECT + properties.is_first_repo AS first_repo_cohort, + countIf(event = 'workspace_created') AS ok, + countIf(event = 'workspace_create_failed') AS fail, + ok / (ok + fail) AS success_rate +FROM events +WHERE timestamp > now() - INTERVAL 7 DAY +GROUP BY first_repo_cohort +``` + +**Onboarding completion rate (per-day cohort funnel from first repo to first agent):** + +```sql +WITH first_repo AS ( + SELECT distinct_id, min(timestamp) AS t0 + FROM events + WHERE event = 'repo_added' AND properties.is_first_repo = true + GROUP BY distinct_id +), +first_agent AS ( + SELECT distinct_id, min(timestamp) AS t1 + FROM events + WHERE event = 'agent_started' AND properties.is_first_repo = true + GROUP BY distinct_id +) +SELECT + toDate(first_repo.t0) AS day, + count() AS started, + countIf(first_agent.t1 IS NOT NULL) AS completed, + completed / started AS completion_rate +FROM first_repo +LEFT JOIN first_agent USING distinct_id +GROUP BY day +ORDER BY day +``` + +Without the cohort booleans, the equivalent queries require either a window function over every user's full event history (to derive cohort membership at query time) or a maintained materialized view. With the booleans, every cohort question is a single `WHERE` clause. + +## Backward compatibility + +Existing dashboards on `workspace_created`, `repo_added`, `agent_started` continue to work unchanged — the new properties are optional and existing aggregations don't reference them. New cohort-aware dashboards naturally start at the rollout boundary. + +The `funnel_step` event sketched in the parent doc's deferred section should carry the same two booleans when it lands, for the same reasons. Add to the schema sketch. + +## Rollout + +Land alongside Phase 1. The marginal cost is small (~15 LOC), and shipping cohort dimensionality after-the-fact means a window of Phase 1 data without it — exactly the data we'd want to slice. + +## Open questions + +- **Should `is_first_repo` survive a "delete all repos and start over" cycle?** Argument for resetting it to true: that user is in re-onboarding mode, and the funnel question still applies. Argument against: dashboards would need a tiebreaker for the rare distinct_id with multiple `is_first_repo: true` event runs. Default: do not reset. A user who has ever had a repo is not a new user. If this turns out to misclassify a meaningful population, revisit. +- **Should we also add `is_first_session: boolean` (true on the user's very first `app_opened`)?** Probably yes, but it lives outside this addendum because it requires a session-tracking mechanism that doesn't exist today. Defer. +- **Do we need `prior_repo_count_bucket` in Phase 2?** Only if `is_first_repo: false` shows wide variance worth splitting. Defer until the data tells us. diff --git a/src/main/ipc/worktree-create-error-class.test.ts b/src/main/ipc/worktree-create-error-class.test.ts index 3116fea9988..eab6dd1988c 100644 --- a/src/main/ipc/worktree-create-error-class.test.ts +++ b/src/main/ipc/worktree-create-error-class.test.ts @@ -1,7 +1,8 @@ -// Pin the classifier's substring set: widening it later requires explicit -// review per the schema-evolution doctrine in telemetry-events.ts. These -// cases fixture the actual throw sites in worktree-remote.ts so a refactor -// that renames the user-facing error strings shows up here as a regression. +// Spot-check the classifier against the load-bearing throws in +// worktree-remote.ts. Not exhaustive — substring widening still requires +// explicit review per the schema-evolution doctrine in +// telemetry-events.ts. Add fixtures here when adding new buckets or when +// a renamed throw site needs regression coverage. import { describe, expect, it } from 'vitest' import { classifyWorkspaceCreateError } from './worktree-create-error-class' @@ -26,6 +27,25 @@ describe('classifyWorkspaceCreateError', () => { expect(classifyWorkspaceCreateError(err)).toBe('path_collision') }) + it('buckets a branch-already-exists-locally throw as path_collision', () => { + const err = new Error( + 'Branch "feature/foo" already exists locally. Pick a different worktree name.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets an existing-PR collision throw as path_collision', () => { + const err = new Error( + 'Branch "feature/foo" already has PR #42. Pick a different worktree name.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets the post-create listing-miss throw as git_failed', () => { + const err = new Error('Worktree created but not found in listing') + expect(classifyWorkspaceCreateError(err)).toBe('git_failed') + }) + it('buckets EACCES errors as permission_denied', () => { const err = Object.assign(new Error("EACCES: permission denied, mkdir '/tmp/x'"), { code: 'EACCES' diff --git a/src/main/ipc/worktree-create-error-class.ts b/src/main/ipc/worktree-create-error-class.ts index 4b7b636ede8..faa9a43193c 100644 --- a/src/main/ipc/worktree-create-error-class.ts +++ b/src/main/ipc/worktree-create-error-class.ts @@ -17,27 +17,34 @@ import type { WorkspaceCreateErrorClass } from '../../shared/telemetry-events' export function classifyWorkspaceCreateError(error: unknown): WorkspaceCreateErrorClass { const message = error instanceof Error ? error.message : '' const stderr = error instanceof Error ? (error as { stderr?: unknown }).stderr : '' - const text = `${message} ${typeof stderr === 'string' ? stderr : ''}` + // Why: throw sites mix capitalization ('Worktree created...' vs lowercased + // git stderr); normalize once so all anchors below can be lowercase literals. + const text = `${message} ${typeof stderr === 'string' ? stderr : ''}`.toLowerCase() - if (text.includes('Could not resolve a default base ref')) { + if (text.includes('could not resolve a default base ref')) { return 'base_ref_missing' } if ( text.includes('already exists') || - text.includes('already has PR') || - text.includes('Could not find an available worktree name') + text.includes('already has pr') || + text.includes('could not find an available worktree name') ) { return 'path_collision' } - if ( - text.includes('EACCES') || - text.includes('EPERM') || - text.includes('permission denied') || - text.includes('Permission denied') - ) { + if (text.includes('eacces') || text.includes('eperm') || text.includes('permission denied')) { return 'permission_denied' } - if (text.includes('fatal:') || text.includes('git ') || text.includes('worktree')) { + // Why: anchors are intentionally specific to true git failures from + // worktree-remote.ts. Bare 'git ' / 'worktree' would mis-bucket SSH-relay + // and sparse-checkout validation errors (which the design doc routes to + // 'unknown'); 'created but not found in listing' covers the formerly + // miscased 'Worktree created but not found in listing' throw. + if ( + text.includes('fatal:') || + text.includes('git worktree') || + text.includes('created but not found in listing') || + text.includes('no git provider') + ) { return 'git_failed' } return 'unknown'