Add first-run onboarding and diagnostics - #13
Conversation
Sessions are fetched per time window, so an empty list no longer means an empty machine. Decide first run from the diagnostics transcript count instead, and keep the window check as the fallback while diagnostics are still loading.
calvintvu
left a comment
There was a problem hiding this comment.
Over-engineering review (complexity only — no correctness/security pass)
+1300 / −35 for a diagnostics panel. Roughly a third of it is machinery the feature does not need yet. Priorities below: P1 = cut before merge, P2 = cut, P3 = nit.
P1 — structural, cut before merge
collector/cmd/coslash/diagnostics.goL1-40: delete the whole file. A TTL cache + mutex +?refresh=1protocol for two directory walks on localhost, single user. Replace withmux.HandleFunc("GET /api/diagnostics", func(w, r) { writeJSON(w, diagnostics.Collect(r.Context(), diagnostics.Default(version))) })inmain.go. This also removes therefresh=1branch inuse-diagnostics.tsL11.collector/cmd/coslash/diagnostics.goL17-21: yagninewDiagnosticsHandlerWithCollectis an injection seam with one caller and no test that uses it. Falls out with the cache.collector/internal/diagnostics/snapshot.goL105-139: yagni the 13-fieldDepsstruct.LookPath,CLIVersion,Home,UserHome,GOOS,GOARCH,Noware all injectable with exactly one injector (Default) and zero tests injecting anything. Callexec.LookPath/runtime.GOOS/time.Nowdirectly and make the signatureCollect(ctx, version). ~45 lines.frontend/.../components/SourceCoverageBanner.tsxL1-30: delete. Third surface for the same facts — the Diagnostics button's status dot already signals warn/fail and the dialog already lists them. Removes the banner,uncoveredSources,sourceCoverageMessage,coverageGaps, the flex-wrapper rework inCoslashPage, and 2 of the 4diagnostics.test.tscases. ~75 lines.frontend/.../components/FirstRunOnboarding.tsxL1-48: shrink. DuplicatesDiagnosticsDialog's loadFailed / loading / checklist / re-run block verbatim, only the chrome differs. One<DiagnosticsPanel diagnostics isLoading loadFailed onRefresh />used by both.frontend/.../lib/page-copy.tsL7-16: shrink. The{kind:'first-run'} | {kind:'copy',…}union rewrites three return sites so one branch can return a sentinel. Keep the function returning copy and branch onnoTranscriptsinCoslashContent, which already rendersFirstRunOnboarding. Drops ~30 of the 76 new test lines with it.
P2 — cut
snapshot.goL45, L153: deleteSettings any, always nil; frontend types itsettings: null. Nothing reads it.snapshot.goL172-178: shrink synthesis-CLI presence is found by scanningsnapshot.Sourcesfor a matchingCLI.Name._, err := exec.LookPath(synthCLI)is one line and stays correct when the synthesis CLI is not an agent source.snapshot.goL273: nativefilepath.Join(home, "summaries")—synthesis.SummariesDir()already exists atcollector/internal/synthesis/paths.go:14.snapshot.goL274-282: delete the summaries count. It feeds no check; it is a decorative number in two UIs.snapshot.goL20 + L229: deletemaxSkippedPaths = 10. It re-slices a listvendors.Scanalready capped at 10 viamaxRecordedSkippedPaths.snapshot.goL85-88, L199-234: shrinkdiagnostics.SkippedPathis a field-for-field copy ofvendors.SkippedPathplus a re-map loop. Reuse the vendor type and map only the display paths.diagnostics/paths.goL1-29: shrink two path-redaction functions wheredisplayErroris already a bluntstrings.ReplaceAll(s, home, "~"). Keep that one, drop the 14-linedisplayPath. ~20 lines.collector/internal/collector/collector.goL3, L27, L33-40: shrink theroot func()field and its error branch exist only to report the root string. PutRootonvendors.SourceScan;SourceHealthcollapses to{Agent, Scan, Err}.vendors/claude/discovery.goL10-21: shrinkFiles()andScan()both do root → scan → filter.func Files() ([]string, error) { s, err := Scan(); if err != nil { return nil, err }; return s.Files, nil }.frontend/.../components/DiagnosticsButton.tsxL1-27: yagni a whole file for<Button>+ icon + status dot, one caller. Inline it inDiagnosticsDialog'sDialogTrigger.CoslashPage.tsxL161, L175, L292: delete thenoTranscriptsprop threaded through two components whilediagnosticsis passed right alongside it. Derive at the use site.
P3 — nits
diagnostics/versions.goL9, L19-22: delete the 64-char truncation of a--versionline. Nothing needs the cap.doctor.goL53-60: shrinkdoctorExitCode→slices.ContainsFunc(snapshot.Checks, func(c diagnostics.Check) bool { return c.Status == diagnostics.StatusFail }), inline at the return.doctor.goL28-31: shrinkmarker := string(check.Status); if fail { marker = "FAIL" }exists to upcase one of three statuses.strings.ToUpper(string(check.Status)).doctor.goL12-15: delete thelog.SetOutput(io.Discard)/ restore dance. The noise it hides is theJSONLFilesUnderskip log, whichScannow returns as data anyway.diagnostics/checks.goL4-30: shrink two separaterange snapshot.Sourcesloops (source check, then CLI check) into one.vendors/claude/discovery.goL27: shrink thefilepath.Separator+filepath.Joinrebuild of"/subagents/workflows/". Release targets are darwin/arm64 and darwin/amd64, andterminalLaunchSupportedis darwin-only — keep the literal.
net: −450 lines possible.
|
Thanks—applied the reductions with clear behavior and maintenance benefits in 1129419. Diagnostics now runs fresh per request, no longer carries cache/refresh or unused dependency machinery, probes the selected synthesis CLI directly, and omits the duplicate coverage banner and unused summary facts. I kept the first-run and dialog renderers separate because their actions and facts differ; combining them would add mode-specific indirection. I also retained diagnostic-owned skipped-path/redaction handling and doctor log suppression to preserve API boundaries, safe path display, and clean JSON output. Validation: |
calvintvu
left a comment
There was a problem hiding this comment.
Correctness + security follow-up (reviewed at 1129419)
Two notes before the findings.
My earlier complexity comment was reviewed against 4f5acf0, and one item in it was wrong. I flagged the diagnostics TTL cache as unnecessary state before tracing what Collect costs. It was acting as a rate limiter on the most expensive operation in the app. 1129419 removed it on my advice, so GET /api/diagnostics now runs a full collect per request with no cache and no mutex. Retracting that recommendation — but the fix is not to restore the cache. It is to make the collect cheap enough that no rate limiter is needed. Plan in the last section.
Everything else in that comment that landed in 1129419 was a good call, and the result is a smaller, clearer diff.
Cost of one Collect at 1129419
| Step | Cost |
|---|---|
probeStorage |
3 syscalls |
collector.Sources() → vendors.Scan ×2 |
2 WalkDirs, stat-only |
SessionCountsByAgent() → List(0) |
2 more WalkDirs over the same roots, then parse() on every transcript ever recorded (8 workers, reads every byte), then probeEnvironment git subprocesses per distinct (cwd, branch) |
commandVersion ×2 |
2 process spawns, ≤2s each |
Roughly all of it is one call that exists to produce a map[string]int. GET /api/diagnostics is also a CORS-simple request and nothing on /api checks Origin or Sec-Fetch-Site, so any page the user visits can trigger it cross-origin in a loop. The response is opaque to the attacker, but the full-machine scan and the git process fan-out still run, now unserialized.
High
1. derive() reports "No sessions found on this machine" when collection fails — with the wrong remediation. internal/diagnostics/checks.go
noSessions is computed from source.Sessions > 0, and counts is set to an empty map whenever SessionCounts() errors. So on a machine with thousands of transcripts where collection failed, or where transcripts exist but fail to parse, the user gets a correct per-source FAIL and sources.none FAIL telling them to "Run claude or codex in a repo for one turn". Two contradictory FAILs, and coslash doctor exits 1 with the wrong diagnosis in the exact scenario the tool exists for.
The frontend already has the right predicate — sources.every(s => s.transcripts === 0). Item A below removes this branch entirely.
2. Unauthenticated cross-origin GET triggers the full-machine scan. See the cost table. Fix is A and E below.
Medium
3. Path redaction fails open. Collect — userHome, _ := os.UserHomeDir()
The error is discarded. When os.UserHomeDir() fails, userHome is "" and both displayPath and displayError short-circuit to identity, so every path in the JSON and in the copy bundle becomes absolute. Narrow window, but a privacy control that silently becomes a no-op deserves the error checked.
4. The copy bundle leaks project paths that home redaction cannot reach. lib/diagnostics.ts formatDiagnosticsForCopy + collectSource
Claude transcript paths are ~/.claude/projects/<cwd-slug>/…, where the slug encodes the absolute working directory with dashes — this repo appears as -Users-<user>-centauri-coslash. displayPath/displayError replace /Users/<user>, not -Users-<user>. So skipped[].path carries the full path of every repo with an unreadable transcript into a bundle whose tooltip promises "never transcript content or session names", and which users paste into public issues.
Either redact the slug form too, or omit skipped[].path from the copy payload and keep it in the local UI. The existing "no session content" test won't catch this — it only asserts that fields the formatter never reads are absent.
5. Synthesis check can report a false OK, and hides the fault it should surface. Collect + checks.go
Enabled is derived from settings on disk, but in main.go synthesis is actually off when NewRunner returns an error (discarded via runner, _ =) or when EnsureDirs fails (mgr.SetRunner(nil)). In those cases doctor reports "Enabled with <model>" while no debrief will ever run.
Conversely, when settings are invalid the check says "Disabled; coSlash will show deterministic transcript details only" and never surfaces state.Error — yet per the README, invalid settings also block terminal launches. That is precisely what a user runs doctor to find.
(The exec.LookPath(synthesisCLI) change in 1129419 is the right fix for the CLI half of this.)
6. Empty-root advice. sourceCheck — when Root() fails, Root stays "", producing Could not fully scan : <err> and Fix: Run ls -la and check ownership.
Low
coslash doctor --helpexits 2.flag.ContinueOnErrorreturnsErrHelpandrunDoctormaps every parse error to 2.main()in the same binary treatsErrHelpas success.version[:maxVersionLength]slices bytes, so a 64-byte cut can split a UTF-8 rune and put U+FFFD in the JSON.probeStoragemutates during a read-only check —MkdirAllcreates~/.coslashas a side effect ofdoctor, and a crash betweenCreateTempandRemoveleaves.diagnostics-*files behind.- "N sessions from M transcripts" reads as a fault on healthy machines — for Claude,
Transcriptscounts subagent files that collapse into parents, so a normal user sees e.g. "3 sessions from 47 transcripts". Fixed for free by item A below. copyStatenever resets toidle— the button reads "Copied" until unmount.navigator.clipboardundefined throws synchronously, not caught by the.catch. Low:127.0.0.1is a secure context.
Making the collect cheap instead of cached
A. Drop List(0); count from the walk already done. The Scan in Sources() already holds every path, and root vs child is path-derivable for both vendors using helpers that exist:
- Claude —
claude.ParentIDFromPath(file) == ""means root (pure string split, already used byFilesSince). - Codex —
codex.SessionIDFromRollout(file) != ""means a rollout file (regex on the basename).
Zero file reads, zero subprocesses. Three things fall out: the countsError branch disappears (no second collection left to fail), finding #1 disappears with it, and the "3 sessions from 47 transcripts" wording is fixed because you report root transcripts rather than every subagent file.
The orphaned-subagent check survives in a better form: roots == 0 && files > 0 means every transcript is a child with no parent — exactly what the current Fix text describes, now path-only.
Worth naming the field sessionFiles rather than sessions: this counts transcript files, not sessions after subagent grouping and excludeSynthesisRuns, so coSlash's own synthesis runs are included.
B. One walk, not four. After A, Sources()'s Scan is the only walk. Today each root is walked twice per request — once by Sources(), once by Files() inside List(0).
C. Move --version off the HTTP path. LookPath is a PATH stat and already gives found + path; the dialog's Facts line falls back to cli.path on its own. commandVersion is two process spawns with 2s timeouts, and claude --version alone pays Node startup. doctor should run them — the user asked and latency is irrelevant there. One bool parameter on Collect splits it.
D. Don't fetch on mount. useDiagnostics fires an unconditional useEffect, so every page load pays for the endpoint even though the panel is behind a button. It is needed in exactly two cases, both already known from useSessions: the dialog is open, or sessions.length === 0 (first-run onboarding). Gate the effect on enabled and pass diagnosticsOpen || sessions.length === 0. For the common user the endpoint is never hit. That is a trigger condition, not stored state.
E. Reject cross-origin on /api. Stateless, and worth doing independent of cost: if Sec-Fetch-Site is present and is neither same-origin nor none, return 403. This closes the drive-by vector rather than relying on the handler staying cheap, and it also covers POST /api/launch on the same mux.
After A and C a collect is 2 WalkDirs + 2 LookPaths + 3 storage syscalls — directory-listing cost, nothing worth caching. With D it rarely runs at all.
Tests
3089455 ("chore: defer Go diagnostics tests") deletes diagnostics_test.go and doctor_test.go, so the Go side of this feature ships with no tests. The handler test is legitimately obsolete now that the cache is gone, but renderDoctor/doctorExitCode isn't, and derive()/sourceCheck() are pure functions over a Snapshot — a table test there is ~15 lines and catches finding #1 directly. displayPath/displayError are pure too, and they're a privacy control.
Frontend coverage is reasonable; page-copy.test.ts covers the new branches well.
Minimum before merge
|
Addressed in |
Context
coSlash currently gives little guidance when no sessions are available or one agent source cannot be scanned. This adds actionable first-run states and support diagnostics while keeping healthy sources usable.
Changes
coslash doctortext and JSON output with safe paths, counts, versions, storage checks, and failure-aware exit codes.Test
cd collector && gofmt -l ./cmd ./internal && go vet ./... && go test ./...— passedcd frontend && npm run lint && npm test && npm run format:check && npm run build— passedScreenshots