refactor(core): DI, persistence, command routing, and classification registry (#0001-#0004)

[tonythethompson]

Summary

• Workspace directory independence #1 — DI composition root, QuickShellServices host facade, Core service abstractions
• feat: workspaces with directory-owned persistence #2 — IAtomicFileWriter, shortcuts v1 envelope, WorkspacesChanged on repository
• ci: add GitHub Actions workflow #3 — Typed command routing (CommandKind, CommandIdParser, handler registry)
• refactor: extract ShortcutLaunchExecutor #4 Phase 1 — IProjectClassifier registry, ProjectAnalysisService, Run plugin DI
• refactor: extract ShortcutLaunchExecutor #4 Phase 2 — Expanded ProjectLayout, nine stack classifiers, companion/dev-server detectors, ProjectAnalysisAccessor for Core static call sites, UI migration, companion auto-suggest in workspace seeds

Test plan

• dotnet build QuickShell.sln -c Release -p:Platform=x64
• dotnet test QuickShell.Core.Tests --filter FullyQualifiedName~ProjectAnalysis|FullyQualifiedName~WorkspaceUtility|FullyQualifiedName~QuickShellCompositionRoot|FullyQualifiedName~ProjectAnalysisAccessor
• Manual: discover-git-repo create flow
• Manual: shortcut form directory change (dev server URL auto-fill)
• Manual: Run workspace editor directory picker

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[cubic-dev-ai]

Running ultrareview automatically — This large-scale refactor extracts core services into a DI composition root, introducing a new versioned persistence envelope for shortcuts, hardened command ID parsing, and project classification infrastructure, making any subtle bug in data migration or command routing high-risk for production.. I'll post findings when complete.

[cubic-dev-ai]

I can't run this ultrareview because you've reached your trial's review limit. Trial plans have lower review limits than paid plans. Upgrade now to resume reviews.

To help optimise your usage, you can tune cubic to get the most out of your usage limits:

• Stop reviewing test files
• Turn off incremental reviews
• Use the cubic CLI locally
• Skip non-default base branches

Learn more →

[tonythethompson]

@claude do PR review

[claude]

Claude finished @tonythethompson's task in 4m 44s —— View job

---

PR Review

Todo list

• Gather context (PR description, changed files, comments)
• Read repo CLAUDE.md for conventions (none found in repo)
• Review DI composition root / QuickShell.Core registration
• Review project classification split (Builder/Classifiers/Pipeline)
• Review command routing / shortcut persistence (already on master, see note below)
• Review Windows arg escaping fix
• Post final review feedback

Important note on scope

origin/master (cc965f3, merged ~2h before this review via PR #58 "foundational DI, persistence, and command routing (#1-#3)") already contains the composition root, ICommandRouter, CommandIdParser, versioned shortcut envelope, and IAtomicFileWriter work this PR's description talks about. Diffing this branch against current origin/master shows only 32 files genuinely changed here (not the 100 listed in the PR metadata, which reflects the diff against an older base). The real content of this review is:

• the project-classification split (phase 1 of docs/architecture/0004-service-consolidation-registry-pattern.md)
• the Windows argument-escaping fix
• QuickShell.Run wiring onto IShortcutRepository/DI
• logo asset exports

Worth double-checking the PR is rebased/synced with master so the GitHub diff view matches what's actually being merged.

---

Findings

1. QuickShell.Run/QuickShellRunSettingsPanel.cs — file got mangled with a blank line after every line
The file went from 95 lines (on master) to 191 lines, and the only functional change is ShortcutRepository → IShortcutRepository. Every single line — using statements, {, }, field declarations — now has an extra blank line inserted after it (confirmed by inspecting the raw file, e.g. lines 1-20). This looks like an artifact from whatever tool/edit performed the interface-type swap (possibly a line-ending/formatting mishap) rather than an intentional change. It doesn't break functionality, but it's pure diff noise, will make git blame for this file useless going forward, and doesn't match this repo's formatting elsewhere. Worth reformatting back to normal before merge.
Fix this →

2. QuickShell.Raycast/src/lib/windows-launch.ts — new escaping algorithm has no test coverage for the cases it fixes
escapeWindowsArgument (lines ~120-140) was rewritten to follow proper CommandLineToArgvW backslash-doubling rules (looks correct to me — matches the standard 2n+1/2n backslash rule). But QuickShell.Raycast/src/__tests__/windows-launch.test.ts still only has the single pre-existing test case (C:\Projects\My App, no embedded quotes or trailing backslashes), which doesn't exercise the actual scenarios this "hardening" fix targets — e.g. a path/arg containing a literal ", or a value ending in backslashes right before the closing quote. Since this is flagged as a security-relevant fix (quote/backslash injection), it'd be good to add cases like foo\"bar, C:\Some Path\, and a\\"b to lock in the new behavior.
Fix this →

3. Minor — QuickShell.Run/Main.cs:72-79: preload silently no-ops if the DI-resolved type ever changes

if (_shortcuts is ShortcutRepository repository)
{
    BeginShortcutPreload(repository);
}

_shortcuts is now resolved as IShortcutRepository from the DI container, and BeginShortcutPreload/PreloadAsync only exist on the concrete ShortcutRepository. Today this always succeeds since AddQuickShellCore only ever registers ShortcutRepository, but the pattern match means if that registration ever changes (test double, decorator, etc.), shortcut preload would silently stop happening with no error or log. Not urgent, but consider exposing a PreloadAsync on IShortcutRepository (or an explicit capability interface) instead of a runtime type-check.

4. Minor/nit — redundant classifier sorting
QuickShell.Core/Classification/ProjectAnalysisService.cs:14 sorts classifiers by Priority in the constructor, and ProjectClassificationPipeline.Classify (QuickShell.Core/Classification/ProjectClassificationPipeline.cs:825) sorts again by Priority before iterating. Harmless (stable, idempotent) but redundant — could drop one of the two OrderByDescending calls.

What looks good

• The classifier split (IProjectClassifier, ProjectLayoutAnalyzer, ProjectClassificationBuilder) is a clean, behavior-preserving extraction — verified the moved logic in ProjectClassificationBuilder.cs is byte-for-byte the same as the old ProjectClassifier.Builder, just re-homed. ProjectAnalysisServiceTests.cs directly asserts parity against the legacy static ProjectClassifier.Classify for both Node and .NET layouts, which is a good regression guard for this exact refactor.
• DI wiring in QuickShellServiceCollectionExtensions.AddQuickShellCore and the QuickShellServices facade update are consistent and match the new interface.
• The phased approach matches docs/architecture/0004-service-consolidation-registry-pattern.md — Phase 1 intentionally leaves internal QuickShell.Core call sites (WorkspaceSeedFactory, GitRepoDiscovery, TaskTypeCommandSuggestion, etc.) on the static ProjectClassifier.Classify facade rather than IProjectAnalysisService; that's documented as expected for this phase, not an oversight.
• Removing the /g flag from the regex .test() call in escapeWindowsArgument's guard is a good cleanup (stateful-regex footgun), even though it wasn't actually causing a bug here since the literal creates a fresh RegExp per call.

[tonythethompson]

Addressed both findings from the review:

1. QuickShellRunSettingsPanel.cs mangled formatting — reformatted back to the clean 95-line version (was 191 lines with a stray blank line after every line), keeping the ShortcutRepository → IShortcutRepository swap. (fee0baa)
2. escapeWindowsArgument missing test coverage — added cases for embedded quotes (foo\"bar), a trailing backslash before the closing quote (C:\Some Path\), and backslash-then-quote sequences (a\"b). All pass against the new CommandLineToArgvW-style escaping. (fee0baa)

Also rebased/merged onto current master to resolve the branch's merge conflicts (this branch's history predates the PR #58 squash-merge).

Left as-is (minor, non-blocking):

• Main.cs pattern-match on ShortcutRepository for preload — valid point, but out of scope for this PR; will track separately if it becomes a real gap.
• Redundant OrderByDescending(Priority) in both ProjectAnalysisService and ProjectClassificationPipeline — harmless duplication, low priority cleanup.

🤖 Addressed by Claude Code

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[tonythethompson]

CodeQL review comment flags 20+ items, but these are pre-existing code-quality warnings in unchanged lines (verified e.g. CommandItemHandlers.cs:221 isn't touched by this diff), same category as the 735 already triaged/dismissed against master after PR #58. The CodeQL check run itself reports success (non-blocking). No inline comment threads exist to reply to (this review has no per-line comment_ids). No action needed.

🤖 Addressed by Claude Code