Repository navigation
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
This brings back the hidden sessions in the remote-added case. I ran the issue's reproduction from source on the base and with this PR: before, only the post-remote session is listed; with the PR, both are. The new store test passes and tsc -b is clean. The public API is unchanged.
It does introduce a leak, though (see inline). Matching every project row that shares a worktree also pulls in the global row, whose worktree is just the first commitless git repo that resolved to it. Repro: git init two folders a/ and b/ without committing, create a session in each, then make the first commit in a/. session list in a/ now also shows the session from b/. On the base it shows only a's session. Any directory that was once an empty repo would end up listing every global session from anywhere.
On the approach: the codebase already has sessions follow a directory's new identity. The Worktree.Event.Resolved handler in session/projector.ts moves sessions from previous to the new project ID. The remote-added case slips through because previous comes from an in-memory cache, and #58668 fixes it at that layer. Matching rows at query time instead leaves sessions whose projectID differs from the project they're listed under. It also leaves other readers that filter by one project ID (e.g. session/stats.ts) out of step with the list. Fixing it in the migration step, or at least never merging the global row, seems more robust.
| const rows = yield* db | ||
| .select({ id: ProjectTable.id }) | ||
| .from(ProjectTable) | ||
| .where(eq(ProjectTable.worktree, row.worktree)) |
There was a problem hiding this comment.
Matching on worktree alone also merges the global row. Its worktree is whatever commitless git repo first resolved to global (it is never re-pointed while that folder exists). Once that folder gets a first commit, its real project shares the worktree with global, and this list returns every global session, including ones in unrelated directories. I reproduced this with two uncommitted repos a/ and b/: after committing in a/, session list in a/ shows b's session. At minimum, global (and probably any row whose sessions aren't all under this worktree) shouldn't be merged here.
|
You are right about the leak, and I reproduced it before changing anything: with The exclusion is at both ends, because the On query-time matching versus the migration step: I agree the projector is the right place for sessions that move, and #58668 covers that case. What it does not cover is rows that already diverged before it lands — the sessions filed under the old row are already recorded with the old
|
There was a problem hiding this comment.
The global row leak is fixed. I re-ran the case with two commitless repos and a first commit in one: session list there now shows only that folder's sessions, the same as on the base. The remote-added reproduction still lists both sessions. The store tests and tsc -b pass, and the public API is unchanged.
Nothing new to flag in this commit.
A worktree's project identity changes with its git fingerprint, so adding a remote leaves a second project row and every earlier session keeps the old project_id. A project-scoped session list matched one row and hid the rest. Filter on all project rows that share the worktree instead. Refs anomalyco#59371
Matching every project row that shares a worktree also matched the global row, whose worktree is whichever commitless repository resolved to it first. A folder that had just gained its first commit therefore listed sessions from unrelated commitless folders. A global caller has no folder identity to expand either, so it stays scoped to its own row.
874420a to
7f38866
Compare
There was a problem hiding this comment.
This push is a rebase onto the latest dev; the change itself is the same as before. I re-ran the remote-added reproduction on the new base and with this PR. The base still hides the earlier session, and the PR lists both. The global row case is still fixed. The store tests and tsc -b pass, and the public API is unchanged.
Nothing new to flag.
|
The shared-worktree expansion also preserves the existing filters and cursor ordering in synthetic integration coverage. A test-only follow-up based on this PR is available at matt8s@4b0e87c. It covers parent selection, archived status, workspace, subpath, search, two-page traversal across identities, exact-directory exclusion and the reverse global-project query. The test blob matches the successful Linux integration validation: full Core and CLI suites, canonical checks and official-compiler checks for both changed packages. Ordinary top-level list defaults and child access are unchanged. No duplicate implementation PR was opened. |
|
Thanks @matt8s — I did not just read your commit ( Applied on top of our head The check that matters more is the reverse one. Reverting only the source change from this PR and keeping your test in place gives 4 pass / 2 fail:
The second one fails on the pagination assertion — Your commit touches only the test file, so it changes no behaviour on its own and nothing in this PR needs to change for it to go in. For anyone reproducing this later: the commit object is not reachable by SHA from the fork ( |
Issue for this PR
Closes #59371
Type of change
What does this PR do?
session listresolves one project for the current directory and lists sessions for that project id. A worktree can own more than oneprojectrow — the row's identity includes the worktree's git fingerprint, so re-creating a remote, or opening the folder before and after that, leaves a second row behind (the report and #57985 both arrive this way). When that happens, sessions recorded under the other row are invisible: the caller is in the same folder, but the query only asks about one of the folder's rows.packages/core/src/session/store.tschanges the project-scoped branch oflistto cover everyprojectrow that shares the worktree, instead of only the row it was handed. The worktree is what a caller means by "this project"; the rows are an implementation detail of how that worktree was recorded.Sessions therefore reappear for anyone already in this state, not only for worktrees created after the fix. Rows on a different worktree stay out of the result, and the exact-
directoryfilter, the cursor and the returned session rows are unchanged.A second commit keeps the
globalproject row out of the match. That row'sworktreeis whichever commitless repository resolved to it first rather than the folder its sessions belong to, so a folder that had just gained its first commit listed sessions from unrelated commitless folders; aglobalcaller has no folder identity to expand either, so it stays scoped to its own row. Its test fails without the fix and passes with it.Part B of the report —
/api/locationreturning the service's start directory rather than the caller's cwd — is a separate defect in a different layer and is deliberately not part of this change.How did you verify your code works?
Red before green, at the layer the defect lives in.
packages/core/test/session-store.test.tsadds a case with twoprojectrows on one worktree and one on a different worktree, each with a session:dev(bc175f675c) without the change, listing with the newer project's id returns only that project's session; the other row's session does not appear.store.tsafter committing makes the new test fail again.Suite, in
packages/core:dev: 6812 pass, 43 skip, 86 fail, 1 error.tsc -binpackages/coreexits 0;prettier --checkandoxlintare clean on the two changed files.Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #57985, #50979