Skip to content

refactor(session): centralize migration results and archive ownership - #1302

Merged
SaulMoro merged 2 commits into
Tencent:mainfrom
nateEc:refactor/1140-session-archive
Oct 23, 2026
Merged

SaulMoro merged 2 commits into
Tencent:mainfrom
nateEc:refactor/1140-session-archive

Conversation

@nateEc

@nateEc nateEc commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

session migrate now computes a result for one final text/JSON renderer. Archive writers own their index lock, so a new caller cannot accidentally save, delete, or rebuild without it. Session platform facts are declared once beside the adapter registry and shared by fidelity, ID, Windows registration, and tool alias readers.

This completes the remaining parts of #1140. MigrationEngine was already replaced by previewMigration on main, so this PR does not repeat that change.

Type of Change

  • Refactor / internal cleanup

Test Plan

  • npm run typecheck (tsc --noEmit)
  • npm run lint
  • 325 focused tests across ten session suites
  • npm run build
  • npm run test:e2e -- session-archive-lock: one real-CLI test passed using an isolated HOME and a local bare remote. It previewed a Claude fixture as JSON, migrated it to CodeBuddy, scrubbed and pushed the archive, repeated the copy without overwriting it, then pulled/rebuilt the archive index and verified _index.json on the remote.
  • Full unit suite: npm test -- --maxWorkers=2: 12,177 passed, 2 failed, 36 skipped across 495 files. The only failures are the two existing submodule-checkout.test.ts assertions that expect hook.teamai-post-checkout.command. Both were independently reproduced on untouched origin/main at 97f1e7b. No new unit failure remains.

The regression for an unwrapped saveSession first showed it writing through a held lock, and now verifies it waits. Existing migration text/JSON, archive deduplication, conflict repair, and adapter tests remain covered. No live model or service credentials were needed for the CLI record.

Related Issues

Fixes #1140

Notes for Reviewers

The archive's shared state is session files plus _index.json. Its public writers are saveSession, deleteSession, rebuildIndex, rebuildAllIndexes, and commitRebuiltIndexes; they all enter withSessionIndexLock. Private deduplication/upsert/remove and repair helpers run only inside those synchronous locked mutations. List/load readers remain read-only. Network pull/push stays outside the archive lock. Existing command lock wrappers can reenter only while their ownership context is active; that context expires before releasing the outer lock.

The CLI options and public JSON envelope shape are unchanged. The internal SyncManager writer methods now return promises, and all production/test callers were audited with typecheck. Platform facts preserve the current behavior, including both claude and claude-code tool lookup forms. Cross-machine Git conflicts still use the existing repair path; this is a local archive lock.

Review follow-up (681fd2a)

Fixed the text batch archive-error identity noted in automated review: busy/save errors now name the currently failing copied session. The regression first reproduced the last-session misidentification and now passes; all 73 session command tests, typecheck, lint and the build-first archive CLI E2E passed.

- Return migration outcomes to one text/JSON renderer while preserving single-session JSON envelopes, batch progress, previews, reuse, and archive diagnostics.
- Make archive save, delete, and index repair methods asynchronous lock-owning entry points; keep synchronous mutations private and expire reentrant ownership after the outer operation ends.
- Declare fidelity, native tool, ID, Windows registration, and tool alias facts beside the adapter registry, and verify every registered platform supplies them.
- Verify 325 focused tests, lint, typecheck, build, and a real-CLI Claude-to-CodeBuddy archive/pull round trip; the full suite passed 12,177 tests with only two independently reproduced baseline submodule failures.
@github-actions

Copy link
Copy Markdown
  • [P2 non-blocking] src/session-flow/session-migrate-cmd.ts:480: Text-mode archive failures now use the JSON-only archiveFailed message. In an --all --push batch, copied refers to the last migrated session, so a save failure on an earlier session incorrectly identifies the last session as “not archived.” Preserve the previous text-mode error or pass the currently failing session’s ID and cwd.

The PR description documents sufficient testing, including a representative real-CLI run after the build. I reviewed only the specified diff; I did not run, build, or install PR code.

- Pass the currently archived target id and workspace into busy/save failure
  results, instead of naming the last session migrated in an --all batch.
- Add a held-lock regression covering three copied sessions and the first
  failed archive; the regression fails before this change and passes after it.
- Verify 73 session command tests, typecheck, lint and a build-first real CLI
  migration/archive round trip against an isolated home and local bare remote.
@github-actions

Copy link
Copy Markdown

No new findings in the specified diff.

  • Resolved: The earlier [P2 non-blocking] archive-error finding is fixed: save failures and busy-lock errors now pass the currently failing session’s ID and cwd. A regression test covers the text batch case.
  • Testing: The PR description documents sufficient testing, including a representative real-CLI run after the build. No additional provider × agent matrix is required.

I reviewed only the specified diff and did not run, build, or install PR code.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

@nateEc Thanks, approving. This does what #1140 asks: one migration result and renderer, archive locking enforced in the archive itself, and one set of platform facts.

Two review rounds checked it against the base 97f1e7b4, with the built CLI and local bare remotes:

  • Parity. The 12 session steps (list, migrate, pull, push, archive) and 20 more session migrate cases give the same exit codes and output on both builds. The exceptions are the three messages below.
  • Locking. Two session push runs into the same archive, one held mid-write, keep both sessions and both index entries. A session pull --all started while a push holds the lock exits 0 with both sessions indexed. Nested and expired lock owners behave as intended.
  • Failure paths. JSON busy, the symlink refusal, a failure after a partial write and its retry, a push failure, and a two-machine conflict repair under a busy lock all match the base. That includes the interrupted repair and its recovery.
  • Static checks. tsc, lint and the 141 tests in the five changed test files pass. The archive-lock e2e passes.

Optional:

  • [P3 nit] Three text-mode messages change: not-found, archive busy, and archive write refused. The body covers the two archive ones. The not-found wording is new.
  • [P3 nit] Six callers still wrap the self-locking archive methods in withSessionIndexLock (session-cmd.ts:215, :285, :309, :315, session-migrate-cmd.ts:477, session-cmd-shared.ts:252). So busy surfaces two ways, as a {status:'busy'} result and as a thrown error. No path gives a wrong exit or message today. Dropping the wrappers later would also remove the need for the AsyncLocalStorage re-entry.
  • [P3 nit] platformForTool('constructor') (and toString, __proto__) returns the key instead of undefined, because sessionPlatformFacts reads inherited properties (adapters/platform-facts.ts:127). The one production caller validates the tool first. An Object.hasOwn check would close it.

This head conflicts textually with #1280, #1297 and #1298. Whichever lands second needs a rebase.

@SaulMoro
SaulMoro merged commit 6c57c3b into Tencent:main Oct 23, 2026
18 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.

[refactor] session-flow: one renderer for migrate output, an enforced index lock, and one record of platform facts

3 participants