Skip to content

Extend spool projects <query> resolution to match project display paths and session cwd#411

Merged
graydawnc merged 2 commits into
paperboytm:mainfrom
AmagiDDmxh:feat/delimiter-safe-project-paths
Jul 13, 2026
Merged

Extend spool projects <query> resolution to match project display paths and session cwd#411
graydawnc merged 2 commits into
paperboytm:mainfrom
AmagiDDmxh:feat/delimiter-safe-project-paths

Conversation

@AmagiDDmxh

@AmagiDDmxh AmagiDDmxh commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extend spool projects <query> resolution to match project display paths and session cwd values in addition to display name and identity key.
  • Preserve arbitrary filesystem paths containing commas by aggregating display_path and cwd as SQLite JSON arrays instead of comma-delimited GROUP_CONCAT strings.
  • Add coverage for path/cwd query matching and comma-containing display paths/cwds.

Notes

The a,b path was also valid because SQLite GROUP_CONCAT uses comma as its default delimiter, so a valid path like /Users/me/foo,bar would be split into /Users/me/foo and bar. The implementation now uses JSON_GROUP_ARRAY(DISTINCT ...) FILTER (...), then parses the JSON array in TypeScript, so commas and other normal path characters stay intact.

JSON.parse should not fail for values produced by SQLite JSON_GROUP_ARRAY; it would only fail if the SQL expression stopped returning JSON or the DB engine lacked JSON aggregate support. The current migration/test path already exercises the function through better-sqlite3.

Verification

  • pnpm --filter @spool-lab/core exec vitest run src/projects/groups.test.ts
  • pnpm --filter @spool-lab/core run build
  • pnpm --filter @spool-lab/core test
  • pnpm --filter @spool-lab/cli test

@AmagiDDmxh
AmagiDDmxh force-pushed the feat/delimiter-safe-project-paths branch 2 times, most recently from 3d40cc1 to 704b242 Compare July 9, 2026 22:24
@AmagiDDmxh
AmagiDDmxh force-pushed the feat/delimiter-safe-project-paths branch from 704b242 to 6664245 Compare July 9, 2026 22:34
…Group lean

Address review feedback on the path/cwd matching change:

- project_groups_v stays the single definition of cross-source project
  grouping. Instead of freezing it at its v6 migration-time definition
  (and duplicating the SQL inline in groups.ts), the view is dropped and
  recreated at the end of runMigrations on every launch, now including
  the JSON_GROUP_ARRAY path/cwd aggregates. Old and new installs always
  run the same definition, and listProjectGroups reads the view again.
- ProjectGroup goes back to its previous shape; the new displayPaths and
  cwds fields move to ProjectGroupWithPaths, returned only by
  listProjectGroups(db, { withPaths: true }). The app's four renderer
  surfaces fetch groups over IPC on every sidebar refresh and never use
  the paths, so the payload stays as it was; only the CLI query resolver
  opts in.
- Add a regression test pinning the intentional resolution change: an
  exact project-path basename now preempts what used to be a unique
  display-name substring match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@graydawnc

Copy link
Copy Markdown
Collaborator

Thanks @AmagiDDmxh — the core of this PR is solid. The JSON_GROUP_ARRAY approach to the comma-splitting problem is correct (verified the DISTINCT ... FILTER combination down to SQLite 3.41, older than what our better-sqlite3 bundles), and the regression test for comma paths is exactly the kind of coverage we want.

I've pushed a follow-up commit (721f392) directly to this branch addressing three review points, so you can see the reasoning in code rather than a back-and-forth:

  1. project_groups_v stays the single source of truth. Inlining the query in groups.ts left the view defined in the schema (and asserted by tests) while production ran a diverging copy of the same semantics. I get why you inlined — the view was frozen at its v6 migration-time definition, and changing it in place would have meant old and new installs run different definitions. The fix keeps your SQL but moves it into the view itself, which is now dropped and recreated at the end of runMigrations on every launch (same pattern as our FTS rebuild maintenance). Views hold no data, so this is free, and every install always runs the current definition.

  2. ProjectGroup stays lean over IPC. The app fetches project groups on every sidebar refresh across four renderer surfaces, and none of them use paths. The new displayPaths/cwds fields now live on ProjectGroupWithPaths, returned only by listProjectGroups(db, { withPaths: true }), which the CLI resolver opts into. This also keeps spool projects --json output unchanged for existing consumers.

  3. Pinned the intentional behavior change. The new exact-basename tier doesn't just resolve previously-ambiguous queries — it can also flip a query that used to resolve uniquely by name substring (e.g. api matching gateway-api-service) to a project whose directory is literally named api. We're accepting that as the better semantics, but it deserved a test documenting it's deliberate, so I added one.

Verification: @spool-lab/core build + full suite (406 passed, including the migration/view tests, which now assert the live definition again) and the full CLI suite (60 passed, including the spawned-binary projects tests exercising the recreated view end to end).

With that, this looks good to merge from my side.

@graydawnc
graydawnc added this pull request to the merge queue Jul 13, 2026
Merged via the queue into paperboytm:main with commit d71da78 Jul 13, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants