Repository navigation
fix(session): name archive repo directories with ~XX so GitHub and GitLab can open them - #1576
Conversation
…tLab can open them Repo directories under sessions/repos/ were escaped as %XX (github.com%2Forg%2Frepo). GitHub and GitLab URL-decode a path once, so %2F turned back into a separator and the directory 404ed in the web UI and the contents API. The escape is now ~XX per UTF-8 byte. An archive still under its %XX name is read there and moved to the new name by the next save, which records each file HEAD holds under the old name so the commit stages the rename, and a retry after a failed commit finds it again. `session pull --all` leaves %XX directories for that save to move, since its own rename would stay uncommitted.
The PR description documents sufficient testing: a build followed by a representative real-CLI push verifies migration against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
…ead of publishing them
|
Fixed in c4a6518. A |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Thanks for the quick fix in c4a6518. I reproduced the earlier P1 at d007db3, where pushing a new session published a local edit to an archived session and the deletion of another. At c4a6518 the move is skipped while %XX has uncommitted changes: the commit holds only the index and the new session, and the next clean push moves the directory to ~XX.
One issue remains: teammates on an older CLI can push into %XX after the move.
[P1 blocking] After the move, sessions a teammate pushes with an older CLI stay hidden, and pushing them again creates duplicates. repoDir prefers ~XX whenever it exists (src/session/sync.ts:517-519). moveRepoDirToCurrentName returns early when ~XX exists (:542, :547), so it never moves files out of a %XX directory created later. I reproduced this with a local bare remote, an archive clone per member, and the merge-base and c4a6518 builds:
- Alice (new CLI) pushes S2. The remote now holds only
github.com~2Facme~2Fapp/. - Carol (old CLI) runs
session pull, then pushes S3. The push succeeds and S3 goes into a newgithub.com%2Facme%2Fapp/with its own_index.json. - Alice's
session listandlist --allshow S1 and S2, andsession search "Session 3" --alldoes not return S3. Onlypull --allwarns: "both hold ... leaving github.com%2Facme%2Fapp in place". - Carol upgrades and pushes S5. S3 is still hidden: her own
session listdoes not show it. No later push moves it. - Bob (old CLI) pushes S6 into
%2F, then upgrades and pushes the same workspace. Dedup reads only the~XXindex, so S6 lands a second time under~2F. The remote now holds S6 in both directories.
Teams often run mixed CLI versions during a rollout, so this will happen. Suggested fix, inside the function this PR adds: when both directories exist and %XX is clean, git mv each committed file under %XX whose ~XX path does not exist yet, then rebuild the ~XX index. With the dirty check from c4a6518, this only moves committed content. A test that pushes into %XX after the move, then pushes with the new CLI and expects one directory and a listed session, covers both cases.
[P3 nit] Add a CHANGELOG line. The archive layout on the remote changes, so a line under [Unreleased] telling teams to upgrade together would help, as recent fixes do (for example #1554).
No doc changes are needed. The encoding is reversible (a literal ~ encodes as ~7E), and the names are valid on Windows.
… after the move A teammate still on an older teamai writes under the %XX name after the archive moved to ~XX, so their sessions stayed hidden and pushing them again with a newer teamai stored them twice. The next save now folds that directory into the current one session by session, keyed by platform, author and source session id: a session the current directory holds keeps the later push, and any other one moves under a free name, since the file name does not identify a session. The fold waits while either directory holds local changes, staged or not, so it never publishes or overwrites them.
|
Both addressed in cab5b6f. [P1] Older CLI pushing into
The Test: As in your scenario, S3 is hidden until that next newer-CLI push. [P3] CHANGELOG: added under |
The description documents sufficient testing: a build followed by a representative real-CLI push against a bare remote. The older tested commit is a note, not a blocker; additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
Re-checked at cab5b6f with the real CLI (local bare remote, the merge-base build as the older CLI). Resolved:
The CI review's two findings both reproduce, so this still needs changes before merge: [P1 blocking] A local deletion under
[P1 blocking] A retry after a rejected commit drops a session the fold renamed. When a One change can fix both: on a run or a retry, count a HEAD path under |
…ecover a fold's renamed paths on retry
|
Both fixed in 2136038. Local deletions: any staged or unstaged change under Retry after a fold that renamed a session: a failed commit leaves the move staged under both names. When both have staged paths, the retry records exactly those paths, including the conflict-resolved name, and does not move again. Test: |
The previously reported deletion and collision-renamed retry scenarios are resolved in the current diff. The description documents sufficient testing: a build followed by representative real-CLI pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
Re-checked at 2136038 with the real CLI (local bare remote, the merge-base build as the older CLI). Resolved:
[P1 blocking] Staged changes in both directories are taken as a leftover move and published. I agree with the CI review and reproduced it. When both
The second row is the deletion case from last round, coming back through the retry branch. Every variant over the last three rounds came from inferring "this is my own unfinished move" from the index or HEAD. A narrower design closes the whole class:
Then nothing a member staged or deleted can be mistaken for the move. The alternative, keeping the retry but accepting only exact renames (same blob under both names), needs special cases for fold drops and the failed push's own new session, so it is easier to get wrong. [P3 nit] The dirty-directory test dropped its |
… name holds removals alone
|
Fixed in ac81a39. Staged paths now count as a move to retry only when everything staged under |
The earlier single-directory edit/deletion cases, collision-renamed retry case, and staged-edit-in-both-directories scenario are resolved. The description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
…old the moved content and nothing changed since
|
Both fixed in f8eb1cd. A retry is now accepted only when the staged state is the move's own:
Anything else falls through to the local-changes check, and the directory is left alone. Tests, both red before the fix:
Both scenarios need someone to edit or stage files in the session archive by hand. That makes them at most |
The specific scenarios covered by the earlier findings are resolved; the remaining retry gaps are described above. The description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
…mmit fails, instead of recovering a staged move
|
Both addressed in b4e47f9 by removing retry recovery instead of tightening it again. The second finding was a real regression: a genuine retry was rejected. The move is now a commit of its own (
Both directories are still left alone while either has a local change to a committed file, staged or not. Tests:
|
All earlier findings are resolved by the current rollback-based migration. The description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. The older tested commit is a note, not a blocker; additional agents/providers are deferred to CI. Review was read-only; no PR code was run, built, or installed. |
…index first, and fold committed files only
|
All three fixed in 319b494.
|
The previously reported findings are resolved in the current diff. The description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
… it only as local files
|
Fixed in f50aac2. A held session counts as the dedup winner only when its files are committed under |
The previously reported scenarios are resolved in the current diff. The description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only; no PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at f50aac2. Thanks for moving to the separate move commit; it closes the class of retry bugs from the earlier rounds instead of one case at a time.
Checked with the real CLI at 319b494 (local bare remote, the merge-base build as the older CLI), and the f50aac2 interdiff read on top:
- A member's local edits and deletions under
%XX(unstaged, staged, or the whole directory removed) stay local. The directory is left in place until it is clean. - A move commit rejected by a hook is undone on disk and in the index; the next push moves the directory and nothing is lost or published early.
- Sessions an older CLI pushes under
%XXafter the move fold into~XXand are listed; a re-pushed session is stored once. A stale old-CLI clone that pushes after the move stops on git's rename conflict and succeeds after a pull, which is acceptable for a rollout and is covered by the CHANGELOG line. - A missing or corrupt
~XXindex is repaired before the fold, and f50aac2 keeps a%XXsession in place when~XXholds it only as local files.
The CI review at f50aac2 has two [P3 nit] items, both needing untracked files placed in the archive by hand (a held session with a committed .meta.json but an untracked .jsonl; untracked session files picked up by the index rebuild). I agree they are nits; worth a follow-up if you want the fold to ignore untracked files entirely.
This merges once CI is green at this head.
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-approving at ba89818: the one-line change requires both files of a held ~XX session to be committed before it wins the fold, which closes the first [P3 nit] from the last CI review. Merges once CI is green at this head.
The earlier destination-pair deduplication finding is resolved; the other previously resolved findings remain resolved. The PR description documents sufficient testing: a build followed by representative real-CLI migration and mixed-version pushes against a bare remote. Additional agents/providers are explicitly deferred to CI. Review was read-only and limited to the specified diff; no PR code was run, built, or installed. |
|
Leaving this one as is. The fold rebuilds the The precondition is also narrow. A session pair can sit untracked under |
Summary
teamai session pusharchives a repo's sessions undersessions/repos/<escaped remote>/, and the escape was%XX. For a repo atgithub.com/acme/appthat givessessions/repos/github.com%2Facme%2Fapp/. GitHub and GitLab URL-decode a path once, so%2Fbecomes/again, and the directory returns 404 in the web UI and the contents API (gh api .../contents/...). Git itself handles it, which is why teamai never noticed.The escape is now
~XXper UTF-8 byte.~is unreserved in URLs, so nothing decodes it, and it is valid on Windows. An archive still under the%XXname keeps being read there, and the next push moves it:Because the move is its own commit, and is undone when that commit fails:
When the session's own commit fails afterwards, it stays staged as before, and a later push on a clean archive moves the directory.
session pull --allno longer renames%XXdirectories itself, because that rename would stay uncommitted. It still moves legacy_-folded names as before.Type of Change
Test Plan
Tested commit
ba898188on macOS, Node v24.21.0.npx tsc --noEmitpassesnpm run lintpassesnpx vitest run: all pass except request-deadline tests ingitlab-mr-timeout,gitlab-mr-create-timeoutandgitlab-org-timeout, which do not importsession-flowand fail a different subset each run.session-sync.test.ts), each seen failing first:leaves no % in the namewas red withgithub.com%2Forg%2Frepomoves a %-escaped repository directory to its current name in the next pushwas red:%paths stayed on the remote, and an untracked file in the directory would be publishedundoes a move whose commit is rejected, and moves the archive on a later push, leaving unrelated files out(HEAD and disk unchanged after the rejected commit)folds sessions an older CLI pushed under the %-escaped name after the move into the current directory, once eachwas red: those sessions stayed hidden, a session re-pushed under a new title was listed twice, and a distinct session with the same file name was lostundoes a fold whose commit is rejected while a push updates a held session, and folds on a later pushrestores the percent-escaped directory when a rejected fold had only untracked files under the current namewas red: the session was gone from diskfolds a session held under the current name once, even when that directory has no indexwas red: an_1duplicatekeeps a session under the percent-escaped name when the current name holds it only as untracked fileswas red: the session left HEADleaves sessions an older CLI pushed under the percent-escaped name in place while the current directory holds local editswas red: the local edit was overwrittenpublishes none of <change> under a percent-escaped repository directory, for an unstaged or staged edit, an unstaged or staged deletion, and the whole directory deleted. All but the unstaged deletion were red at some point.publishes none of the edits staged under both repository directory names,publishes neither a staged deletion under the percent-escaped name nor a staged edit under the current one,publishes no edit made between a rejected move and the next pushleaves a %-escaped repository directory for the next push to move when rebuilding every indexwas red: the checkout had an uncommittedD sessions/repos/github.com%2F...Real CLI (
npm run build, thennode dist/index.js session push --source claude-code --repo-root <archive> --scrub -y --limit 1), with a bare remote and a workspace whose origin ishttps://github.com/acme/app.git.An archive under the old name moves on the next push:
A mixed-version team, with the published
teamai-cli@0.28.1as the older CLI:Only the
claude-codesource and a plaingitremote were run locally; the other agents and providers are left to CI. I checked that GitHub decodes%2F(the contents API returns 404 for the%2Fpath and the listing for%252F), but did not push a~2Fdirectory to a live GitHub or GitLab remote.Related Issues
Notes for Reviewers
%XXstay hidden from newer clients until the next newer-teamai push for that repo folds them in. The CHANGELOG line asks teams to upgrade together.decodeRepoIdentityreads both forms. The%XXform left~literal, so a name that contains%is decoded as-is.