feat(agentsessions): import other coding agents' local sessions and continue them in Zero - #878
feat(agentsessions): import other coding agents' local sessions and continue them in Zero#878gnanam1990 wants to merge 3 commits into
Conversation
Adds internal/agentsessions, which reads the sessions Claude Code, Codex,
Factory Droid and Pi leave on the local disk and translates them into Zero
session events.
Four agents, two parsers. Claude Code, Factory Droid and Pi independently
arrived at the same layout — one JSONL file per session under a directory
named after the working directory, with text/thinking/tool_use/tool_result
content blocks — so one family-1 parser serves all three. Codex differs
enough to need its own: date-partitioned directories and every record
wrapped in a "payload" object.
Three rules hold throughout, each enforced by a test rather than left to
care:
1. Read-only. Nothing here writes to, moves or locks another agent's
store.
2. Path-exact globs, never a directory walk. Every one of these agents
keeps live credentials in the same tree as its transcripts —
~/.codex/auth.json, ~/.grok/auth.json, ~/.factory/auth.v2.key, and
most pointedly ~/.pi/agent/auth.json, the direct sibling of
~/.pi/agent/sessions/. Discovery uses fixed-depth globs pinned to one
extension, rejects symlinks by Lstat, and resolves session ids by
comparing glob results rather than joining an id onto a root.
3. Imported text is untrusted input and passes through
internal/redaction at a single chokepoint before reaching the event
log.
Discovery is a bounded head read (64 lines / 2 MiB), never a full parse:
the corpus this was built against is 439 MB across 1,266 files with a
single 73 MB transcript in it. The byte budget is sized from measurement —
three real sessions open with a ~334 KB record, and a 256 KiB budget was
spent before reaching the record carrying cwd, dropping those sessions from
discovery with no error anywhere.
The slugged directory name is treated as a hint only. It is lossy —
"-Users-x-dev-zero" is what both /Users/x/dev/zero and /Users/x/dev-zero
produce — so it narrows the search and the cwd recorded inside the
transcript decides.
Also emits an activity summary as EventCompaction events, because
sessions.promptContextEvents passes messages but not tool events: without
this the model continuing the work sees none of the files read, commands
run or errors hit. Zero's own compaction cannot substitute, since
toolPayloadPreview allow-lists id/name/toolName/status and drops the
arguments and output this needs. Each summary event stays under the
digest's 500-character per-event budget, and a call whose result failed
withdraws its claim so a Read of a nonexistent path is never reported as a
file that was read.
Two subcommands on the existing `zero sessions` dispatcher: zero sessions discover sessions from other agents, this workspace zero sessions import <agent>:<id> copy one into Zero, then --resume it discover scopes to the current workspace by default (--all widens it, --agent filters), because an unscoped list on a machine holding a thousand transcripts is not a list anyone can use. import creates an ordinary Zero session, after which every existing verb — resume, fork, rewind, compact, the picker — works on it with no further change. The provenance tag records both the agent and the source session id, which is what lets a caller tell an already-imported session from one still only on the other agent's disk. An adapter that fails is reported as a warning and does not fail the command: these are undocumented formats belonging to other products, and one vendor shipping a new layout must not deny the user the other three. Nothing here reads another agent's credentials, launches its binary, or uses its subscription. The imported session runs on Zero's own provider with the user's own key.
The picker gains a tab strip — All, then one tab per agent that actually has sessions, busiest first. All lists everything with the source agent on each row; Tab narrows to one agent and wraps back to All. Typing to search still works and the query survives a tab change, since switching agents is a narrowing rather than a reset. Tabs are built from what is present, so an agent never used gets no tab and a single-source store gets no strip at all — "All | zero" is chrome that says nothing. The picker also lists sessions from other agents that have NOT been imported yet; choosing one imports it and resumes in a single step. Without this the strip was honest but nearly empty: in a workspace with 137 discoverable Claude Code sessions it offered the one that had been imported by hand. `/resume <agent>:<id>` accepts the same reference from the command line. Zero session ids cannot contain a colon (sessions.ValidSessionID), so the form is unambiguous. Discovery is memoised per workspace for ten seconds and invalidated on import. Opening the picker costs a bounded read of every transcript across four stores; paying that on each keypress made /resume hitch every time it was opened and dismissed. The tab filter is applied before the query ranks results, so an empty search box shows one agent's sessions rather than every agent's — the exact moment the strip has to be trusted. model_test.go's session-picker assertion moves from "Meta must be empty" to "Meta must not contain the session id, and must name the source agent". That check has always been about keeping the raw id out of the row, which consumed half the picker and truncated the title; empty-string was a proxy for it.
WalkthroughThis change adds cross-agent session discovery and import support. It reads supported transcript formats, translates and redacts events, exposes CLI commands, caches discovery results, and adds agent tabs and import handling to ChangesForeign session storage and adapters
Translation and import flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Adapter
participant ForeignStore
participant ZeroStore
CLI->>Adapter: discover or read session reference
Adapter->>ForeignStore: scan or stream transcript
ForeignStore-->>Adapter: session metadata and transcript records
Adapter-->>CLI: redacted translated events
CLI->>ZeroStore: create imported session and append events
ZeroStore-->>CLI: imported session result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (15)
internal/agentsessions/registry.go (2)
134-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment states an import tag format that the code no longer produces.
Line 138 says the tag is
"imported:claude-code".ImportTagat line 91 produces"imported:claude-code:<foreign session id>". Update the comment.As per coding guidelines: "Ensure PR descriptions, help text, and comments match shipped behavior".
📝 Proposed comment fix
-// Provenance lives in the tag ("imported:claude-code") and in the title. +// Provenance lives in the tag ("imported:claude-code:<foreign session id>") +// and in the title.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/registry.go` around lines 134 - 140, Update the provenance comment near ImportTag to describe the shipped tag format, including the foreign session ID suffix (for example, “imported:claude-code:<foreign session id>”), without changing the import behavior.Source: Coding guidelines
141-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Importindexes the whole foreign store twice for one session.
describecallsadapter.Discover(""), which head-reads every transcript in the store. The file comments report 1,266 files and 439 MB on one real machine.adapter.Readthen globs the same store again to resolve the id. A single import therefore pays a full index plus a second directory scan, only to obtain the title, cwd, and model.This is acceptable for a one-shot CLI import. It is worth reconsidering if the TUI picker imports on selection. Consider adding a
Describe(id string) (ForeignSession, bool)method toAdapterso both the lookup and the read resolve the path once.Also applies to: 175-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/registry.go` around lines 141 - 152, The Import flow currently scans the foreign store twice by calling describe and then adapter.Read. Add an Adapter-level Describe(id string) (ForeignSession, bool) lookup that resolves the session path once, update Import to use it for metadata and pass the resolved path or session to the read operation, and preserve the existing missing-session and read-error behavior.internal/agentsessions/family1_test.go (1)
248-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe 85% ratio assertion depends on a developer's private corpus.
TestTheRealCorpusStillParsesfails when a contributor's real store contains a higher share of stubs than the store this threshold was measured on. The failure is not caused by the change under test. Consider reporting the ratio witht.Logfand keeping only a lower, clearly-broken bound, for exampleratio == 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/family1_test.go` around lines 248 - 257, The ratio assertion in TestTheRealCorpusStillParses is tied to a private corpus and should not require 85% coverage. Replace the 0.85 failure threshold with only a clearly broken zero-result check, while retaining the existing ratio reporting via t.Logf and diagnostic context.internal/agentsessions/translate_test.go (2)
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe doc comment for
TestPayloadKeysMatchWhatTheTUIReadsis attached toconversationEvents.Lines 51-55 describe the test. Lines 56-58 describe
conversationEvents. The whole block sits aboveconversationEvents, so godoc reports the TUI-tripwire explanation as documentation for the helper. Move lines 51-55 above the test at line 70.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/translate_test.go` around lines 51 - 59, Move the TUI payload-key tripwire documentation so it directly precedes TestPayloadKeysMatchWhatTheTUIReads, and leave the conversationEvents-specific explanation immediately above conversationEvents. Ensure each comment block documents only its corresponding symbol.
259-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact counts in the trim note.
The test checks only that the summary contains "not imported". The reported number is therefore unverified, and it is currently wrong by one. Add assertions for both numbers, and add a case for
MaxEvents: 1, which yields a note and zero conversation events.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/translate_test.go` around lines 259 - 266, The trim-note test around the existing event-type and summary assertions only checks wording; assert both reported event counts and correct the expected count. Add a separate case covering MaxEvents: 1, verifying it emits the trim note followed by zero conversation events, so the boundary behavior is regression-tested.Source: Coding guidelines
internal/agentsessions/cache_test.go (1)
81-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the
problemsslice too.The test asserts the aliasing property for
sessionsonly.DiscoverAllCachedcopiessessionsbut returnsentry.problemsby reference atinternal/agentsessions/cache.goLine 45. A caller that appends to or sorts that slice reaches the next caller's results. Either copyproblemsincache.goand extend this test, or state in the comment that onlysessionsis protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/cache_test.go` around lines 81 - 107, Extend TestCallersCannotReorderEachOthersResults to mutate the returned problems slice and verify a subsequent DiscoverAllCached call is unaffected; also update the cache implementation to return a copied problems slice alongside the existing sessions copy, using the relevant entry.problems handling in DiscoverAllCached.internal/agentsessions/paths_test.go (1)
78-147: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a case for a symlinked project directory.
This test plants decoys at the wrong depth and credential files at three levels. It does not cover an intermediate component that is a symlink.
globTranscriptsonlyLstats the final match, so a symlinked project directory under the sessions root escapes the store and the test still passes. Add a case wheresessions/<slug>is a symlink to a directory outside the store, and assert that no transcript under it is returned.The coding guidelines state: "Every behavior or security-boundary change requires a regression test, including failure paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/paths_test.go` around lines 78 - 147, The test TestDiscoveryGlobsNeverMatchACredentialFile must cover symlink traversal through the project-directory component. Create an external directory containing a transcript, add a sessions/<slug> symlink pointing to it, invoke globTranscripts, and assert the external transcript is not returned while preserving the existing valid-transcript assertion.Source: Coding guidelines
internal/agentsessions/cache.go (2)
42-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKey the memo by the normalized workspace path.
The map key is the raw
cwdstring.paths.godefinesnormalizeDirfor exactly this problem:/tmp/proj,/tmp/proj/, and/private/tmp/projare the same workspace, andsameDirtreats them as equal. Here they produce three separate entries and three separate 300ms discoveries, andInvalidateDiscoveryis the only thing that ever bounds the map size. Normalize the key once at entry.♻️ Proposed fix
func DiscoverAllCached(env Env, cwd string) ([]ForeignSession, []error) { + key := normalizeDir(cwd) discoveryMu.Lock() defer discoveryMu.Unlock() - if entry, ok := discoveryCache[cwd]; ok && discoveryNow().Sub(entry.at) < discoveryTTL { + if entry, ok := discoveryCache[key]; ok && discoveryNow().Sub(entry.at) < discoveryTTL { // Copy: callers sort and filter the slice they are handed, and a shared // backing array would let one caller reorder another's results. return append([]ForeignSession{}, entry.sessions...), entry.problems } found, problems := DiscoverAll(env, cwd) - discoveryCache[cwd] = discoveryEntry{sessions: found, problems: problems, at: discoveryNow()} + discoveryCache[key] = discoveryEntry{sessions: found, problems: problems, at: discoveryNow()} return append([]ForeignSession{}, found...), problems }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/cache.go` around lines 42 - 49, Normalize cwd once at the entry point using normalizeDir, then use that normalized workspace path consistently as the discoveryCache key for lookup and storage in the surrounding discovery function. Preserve the existing cache-copy, discovery, and problem-handling behavior, and ensure InvalidateDiscovery receives or matches the same normalized key.
27-33: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
discoveryNowis mutated by tests outside the mutex.
withFakeClockininternal/agentsessions/cache_test.goassignsdiscoveryNowwhileDiscoverAllCachedreads it underdiscoveryMu. No test in this package callst.Parallel, so the race detector stays quiet today. The moment one does,go test -racereports a data race on a package-level variable. Move the clock into the guarded state, or read and write it underdiscoveryMu.The coding guidelines state: "run affected concurrent code under the race detector."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/cache.go` around lines 27 - 33, Protect discoveryNow consistently with discoveryMu: update withFakeClock’s test assignment and restoration to hold the mutex, and ensure DiscoverAllCached reads the clock while holding the same lock. Prefer moving the clock into the mutex-guarded discovery state if that fits the existing design, while preserving test-controlled TTL behavior.Source: Coding guidelines
internal/agentsessions/jsonl_test.go (2)
142-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
streamLinescase for an over-long record.
TestALineTooLongToKeepIsSkippedNotFatalcoversscanHeadonly.streamLinesis the function used for the full import read, so an over-long record there decides whether an imported transcript loses a message or fails outright. Add a case that feedsstreamLinesa record longer than its limit and assert the following records are still visited.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/jsonl_test.go` around lines 142 - 173, Add a focused test for streamLines where one record exceeds the configured size limit, asserting streamLines returns no error and still invokes the callback for subsequent records. Reuse the existing temporary-file and callback-counting patterns from TestStreamLinesReadsEverything and TestStreamLinesToleratesAMissingTrailingNewline.
16-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShrink the 40 MB fixture.
The loop writes 200 lines of 200 KiB each, so this test creates roughly 40 MB on disk on every run, including race-detector runs. The property under test is a ratio: bytes read must stay under
defaultHeadLimit.MaxBytesand well under the file size. Size the fixture fromdefaultHeadLimit.MaxBytesinstead of a fixed 32 MB floor. A file of a few megabytes proves the same property and keeps the suite fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agentsessions/jsonl_test.go` around lines 16 - 46, Reduce the fixture size in TestScanHeadReadsFarLessThanTheWholeFile by deriving the bulk content or number of lines from defaultHeadLimit.MaxBytes rather than writing 200 fixed 200 KiB lines. Keep the file several times larger than the head budget so the existing read-limit and file-size ratio assertions still verify the intended behavior without creating a roughly 40 MB fixture.internal/cli/sessions_import.go (2)
138-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTake
nowas a parameter instead of callingtime.Now()in the loop.
describeAgealready accepts a clock.formatDiscoveredSessionsdefeats that seam by callingtime.Now()per session, so a table test cannot pin the "today" / "Jan _2" / date branches. The redundantIsZerocheck also disappears, becausedescribeAgealready returns""for a zero time.♻️ Proposed change
-func formatDiscoveredSessions(found []agentsessions.ForeignSession, cwd string) string { +func formatDiscoveredSessions(found []agentsessions.ForeignSession, cwd string, now time.Time) string { if len(found) == 0 {for _, session := range found { - age := "" - if !session.UpdatedAt.IsZero() { - age = describeAge(session.UpdatedAt, time.Now()) - } + age := describeAge(session.UpdatedAt, now) header := session.Agent + ":" + session.IDThen update the call site on line 42 to pass
time.Now().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sessions_import.go` around lines 138 - 142, Update formatDiscoveredSessions to accept a now time parameter and pass that value to describeAge for every session, removing the per-session time.Now() call and redundant UpdatedAt.IsZero() check. Update its caller to provide time.Now().
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
--agentagainst the known adapter names.A misspelled agent name silently yields an empty result.
agentsessions.ParseRefrejects an unknown agent forimport, sodiscoverbehaves differently for the same input. The empty-state text does list the readable agents, so this is a polish item, not a bug.♻️ Optional: reject an unknown agent name up front
found, problems := agentsessions.DiscoverAll(agentsessions.OSEnv(), cwd) + if wanted := strings.TrimSpace(options.agent); wanted != "" { + known := agentsessions.AdapterNames(agentsessions.OSEnv()) + if !containsFold(known, wanted) { + return writeExecUsageError(stderr, "unknown agent "+wanted+"; known agents: "+strings.Join(known, ", ")) + } + } found = filterDiscoveredByAgent(found, options.agent)
containsFoldwould be a small helper usingstrings.EqualFold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/sessions_import.go` around lines 33 - 34, Validate options.agent against the known adapter names before calling filterDiscoveredByAgent in the discover flow, using case-insensitive matching consistent with agentsessions.ParseRef and the existing readable-agent list. Reject unknown non-empty agent names up front instead of allowing them to produce an empty result, while preserving discovery for valid names and omitted filters.internal/tui/model.go (1)
1806-1812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire Shift+Tab to
cycleTab(-1), or drop the backward path.
cycleTabaccepts a negative delta, andTestCyclingBackwardsWrapsexercises it, but no key binding reaches it. The Shift+Tab branch at line 1659 has no tabbed-picker case, so it falls tom.noBlockingModal(), which an open picker makes false. Shift+Tab therefore does nothing while the/resumestrip is up.Forward-only cycling works with three tabs. It stops being reasonable if a user has sessions from all four supported agents plus Zero, where reaching the previous tab costs four presses.
♻️ Proposed addition in the Shift+Tab branch
case keyIs(msg, tea.KeyTab) && keyShift(msg): if m.transcriptDetailed { return m, nil } if m.pendingPermission != nil { return m.movePermissionCursor(-1), nil } if m.pendingAskUser != nil { return m.moveAskUserTab(-1), nil } + if m.picker != nil && m.picker.hasTabs() { + m.picker.cycleTab(-1) + return m, nil + }If you keep forward-only cycling, remove
TestCyclingBackwardsWrapsor restate it as a unit test ofcycleTabrather than of user-reachable behavior.As per coding guidelines: "wire advertised entry points or narrow the claim".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 1806 - 1812, Update the Shift+Tab handling branch in the model’s key-processing logic to detect an open tabbed picker, call m.picker.cycleTab(-1), and return before the noBlockingModal fallback. Alternatively, remove or narrow TestCyclingBackwardsWraps so it only verifies the cycleTab method rather than user-reachable behavior; preserve the existing forward Tab handling.Source: Coding guidelines
internal/tui/session_picker_tabs_test.go (1)
69-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the imported-session dedup rule; this test cannot fail.
Two points.
TestAnAgentWithNoSessionsGetsNoTabbuilds a picker fromzeroandcodexrows, then asserts that no tab is namedfactoryorpi.sessionPickerTabsderives every tab from the items it receives, so the assertion holds by construction. The test documents intent but detects no regression.More important is what is missing.
foreignSessionItemsskips any discovered session whose<agent>:<id>already appears as an import tag on a local session. That rule is what stops/resumefrom listing the same conversation twice — once as itself and once as its copy. No test in this file covers it, because every test here constructspickerItemvalues directly and never exercisesforeignSessionItems.A table test over
ParseImportTaginputs plus a fake discovery result would cover it. That needs the injectableagentsessions.Envdiscussed oninternal/tui/model_test.go, so the two are worth doing together.As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/session_picker_tabs_test.go` around lines 69 - 76, Replace the construction-only assertions in TestAnAgentWithNoSessionsGetsNoTab with regression coverage for foreignSessionItems: use an injectable agentsessions.Env and fake discovery results to verify sessions whose <agent>:<id> matches a local session’s ParseImportTag are excluded, while non-matching imported sessions remain. Add table cases covering matching, non-matching, and malformed import tags, reusing the test injection pattern from model_test.go.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agentsessions/activity.go`:
- Around line 258-312: Update activityLog.summaryEvents to apply
maxSummaryEventChars to the fully assembled headline after adding the
toolBreakdown text, rather than relying on toolBreakdown’s independent
truncation. Preserve the existing count and breakdown content while ensuring the
emitted headline stays within the event budget, and extend the relevant summary
test with many unrecognised tool names to cover this case.
- Around line 89-118: Change activityLog deduplication to track claim counts
rather than booleans: update newActivityLog to initialize seen as
map[string]int, increment the bucket/value key in add, and decrement it in
withdraw. Remove the list entry and delete the key only when its count reaches
zero, preserving entries still referenced by other calls.
In `@internal/agentsessions/cache.go`:
- Around line 38-51: Update DiscoverAllCached so the discoveryMu lock is held
only while checking the cache and storing results, not while calling the slow
DiscoverAll operation. Unlock before discovery, allow concurrent misses
(including different workspaces) to proceed independently, then re-acquire the
lock to write the discovered entry and return the copied sessions and problems.
In `@internal/agentsessions/codex_test.go`:
- Around line 150-189: Gate TestTheRealCodexCorpusStillParses behind an explicit
opt-in environment variable, returning via t.Skip before accessing codexRoot,
OSEnv, or the developer’s transcripts when the variable is unset. Preserve the
existing live-corpus assertions for opted-in runs, and keep path-sensitive
behavior covered through a hermetic or non-Linux test rather than relying on
this live test.
In `@internal/agentsessions/paths.go`:
- Around line 97-117: Update globTranscripts in internal/agentsessions/paths.go
(lines 97-117) to reject matches with symlinked parent components and enforce
containment at open time using a rooted or handle-relative no-follow API,
including platform reparse-point protections; final-component Lstat alone is
insufficient. In internal/agentsessions/paths_test.go (lines 78-147), add
coverage where sessions/<slug> symlinks to a directory outside the store and
assert no transcript beneath it is returned.
- Around line 48-60: Update claudeCodeRoot and codexRoot so configured
CLAUDE_CONFIG_DIR or CODEX_HOME values are used only when absolute; treat
relative values like unset configuration and fall back to env.underHome with the
existing default subpaths.
- Around line 195-203: Update sameDir to compare normalized paths
case-insensitively when runtime.GOOS is Windows, while preserving the existing
case-sensitive comparison on other platforms. Add the runtime dependency to the
import block and keep the current empty-path rejection unchanged.
In `@internal/agentsessions/registry.go`:
- Around line 154-167: Update the import flow around store.Create and
store.AppendEvents to delete the newly created session via the sessions store’s
existing delete/remove operation when AppendEvents fails. Preserve the original
append error, but return a combined error if cleanup also fails; never delete
pre-existing sessions or report success after unsuccessful cleanup.
In `@internal/agentsessions/translate.go`:
- Around line 91-97: Full-read translators silently discard records truncated by
the 64 KiB stream limit; make truncation observable and emit a noteEvent for
each skipped truncated record. In internal/agentsessions/translate.go lines
91-97, update streamLines/readBoundedLine signaling and translateFamily1 to
distinguish truncation from ordinary unmarshal failures. Apply the same handling
in internal/agentsessions/codex.go lines 195-199 within translateCodex, while
preserving silent skipping for unrecognised or non-response records.
- Around line 189-201: Update capEvents so the omitted-event count includes
kept[0], using len(events)-(max-1) or the equivalent count, and pass that
corrected value to plural. Adjust the note text to use singular/plural verb
agreement, producing “was not imported” for one omitted event and “were not
imported” otherwise.
In `@internal/cli/sessions_import.go`:
- Around line 230-236: Replace the lexical filepath.Clean comparison in the
sessions import workspace check with the shared sessionMatchesWorkspace
predicate. Promote sessionMatchesWorkspace from internal/tui/session.go to an
appropriate shared package, update both callers to use it, and preserve the
existing empty-string behavior when the workspaces match or the current
directory cannot be determined.
- Around line 1-14: Add regression tests for the sessions discover and import
command flows, covering agent filtering, JSON output, failure exit codes, and
importWorkspaceWarning behavior. Include a non-Linux case that verifies
workspace path normalization, and use the command handlers and existing
session-test helpers to assert results and errors without changing production
behavior.
In `@internal/tui/model_test.go`:
- Around line 908-917: Thread an agentsessions.Env through the model and
session-picker construction so newSessionPicker and foreignSessionItems use the
injected environment instead of agentsessions.OSEnv(). In
internal/tui/model_test.go lines 908-917, build the model with a t.TempDir()
home to isolate discovery. In internal/tui/session_picker_tabs_test.go lines
69-76, use the same injected Env, add coverage for imported-session
deduplication in foreignSessionItems, and strengthen
TestAnAgentWithNoSessionsGetsNoTab so it genuinely verifies the no-tab behavior.
In `@internal/tui/session.go`:
- Around line 434-444: Update newSessionPicker to retain each session’s raw
update time on pickerItem, including items from both local assembly and
foreignSessionItems, then sort the merged items by recency before building the
picker. Add or reuse sortPickerItemsByRecency so sorting uses time.Time rather
than the formatted Label, while preserving per-agent item behavior.
- Around line 515-518: Guard session.UpdatedAt.IsZero() before formatting it, so
zero timestamps do not reach sessionWhen or sessionPickerLabel and produce a
year-1 date. Update the surrounding label logic in the session row path,
preferably by reusing or adding a typed time.Time variant of sessionWhen to
avoid converting the timestamp through RFC3339 text while preserving existing
behavior for populated timestamps.
- Around line 473-479: Move the synchronous agentsessions.Import call out of the
Bubble Tea Update path into a tea.Cmd that performs the import asynchronously
and returns a result message containing the session or error, then handle that
message in the Update flow while preserving agentsessions.InvalidateDiscovery
before rebuilding the picker. Review whether the import should set an explicit
MaxEvents limit instead of using uncapped ReadOptions{}.
---
Nitpick comments:
In `@internal/agentsessions/cache_test.go`:
- Around line 81-107: Extend TestCallersCannotReorderEachOthersResults to mutate
the returned problems slice and verify a subsequent DiscoverAllCached call is
unaffected; also update the cache implementation to return a copied problems
slice alongside the existing sessions copy, using the relevant entry.problems
handling in DiscoverAllCached.
In `@internal/agentsessions/cache.go`:
- Around line 42-49: Normalize cwd once at the entry point using normalizeDir,
then use that normalized workspace path consistently as the discoveryCache key
for lookup and storage in the surrounding discovery function. Preserve the
existing cache-copy, discovery, and problem-handling behavior, and ensure
InvalidateDiscovery receives or matches the same normalized key.
- Around line 27-33: Protect discoveryNow consistently with discoveryMu: update
withFakeClock’s test assignment and restoration to hold the mutex, and ensure
DiscoverAllCached reads the clock while holding the same lock. Prefer moving the
clock into the mutex-guarded discovery state if that fits the existing design,
while preserving test-controlled TTL behavior.
In `@internal/agentsessions/family1_test.go`:
- Around line 248-257: The ratio assertion in TestTheRealCorpusStillParses is
tied to a private corpus and should not require 85% coverage. Replace the 0.85
failure threshold with only a clearly broken zero-result check, while retaining
the existing ratio reporting via t.Logf and diagnostic context.
In `@internal/agentsessions/jsonl_test.go`:
- Around line 142-173: Add a focused test for streamLines where one record
exceeds the configured size limit, asserting streamLines returns no error and
still invokes the callback for subsequent records. Reuse the existing
temporary-file and callback-counting patterns from
TestStreamLinesReadsEverything and
TestStreamLinesToleratesAMissingTrailingNewline.
- Around line 16-46: Reduce the fixture size in
TestScanHeadReadsFarLessThanTheWholeFile by deriving the bulk content or number
of lines from defaultHeadLimit.MaxBytes rather than writing 200 fixed 200 KiB
lines. Keep the file several times larger than the head budget so the existing
read-limit and file-size ratio assertions still verify the intended behavior
without creating a roughly 40 MB fixture.
In `@internal/agentsessions/paths_test.go`:
- Around line 78-147: The test TestDiscoveryGlobsNeverMatchACredentialFile must
cover symlink traversal through the project-directory component. Create an
external directory containing a transcript, add a sessions/<slug> symlink
pointing to it, invoke globTranscripts, and assert the external transcript is
not returned while preserving the existing valid-transcript assertion.
In `@internal/agentsessions/registry.go`:
- Around line 134-140: Update the provenance comment near ImportTag to describe
the shipped tag format, including the foreign session ID suffix (for example,
“imported:claude-code:<foreign session id>”), without changing the import
behavior.
- Around line 141-152: The Import flow currently scans the foreign store twice
by calling describe and then adapter.Read. Add an Adapter-level Describe(id
string) (ForeignSession, bool) lookup that resolves the session path once,
update Import to use it for metadata and pass the resolved path or session to
the read operation, and preserve the existing missing-session and read-error
behavior.
In `@internal/agentsessions/translate_test.go`:
- Around line 51-59: Move the TUI payload-key tripwire documentation so it
directly precedes TestPayloadKeysMatchWhatTheTUIReads, and leave the
conversationEvents-specific explanation immediately above conversationEvents.
Ensure each comment block documents only its corresponding symbol.
- Around line 259-266: The trim-note test around the existing event-type and
summary assertions only checks wording; assert both reported event counts and
correct the expected count. Add a separate case covering MaxEvents: 1, verifying
it emits the trim note followed by zero conversation events, so the boundary
behavior is regression-tested.
In `@internal/cli/sessions_import.go`:
- Around line 138-142: Update formatDiscoveredSessions to accept a now time
parameter and pass that value to describeAge for every session, removing the
per-session time.Now() call and redundant UpdatedAt.IsZero() check. Update its
caller to provide time.Now().
- Around line 33-34: Validate options.agent against the known adapter names
before calling filterDiscoveredByAgent in the discover flow, using
case-insensitive matching consistent with agentsessions.ParseRef and the
existing readable-agent list. Reject unknown non-empty agent names up front
instead of allowing them to produce an empty result, while preserving discovery
for valid names and omitted filters.
In `@internal/tui/model.go`:
- Around line 1806-1812: Update the Shift+Tab handling branch in the model’s
key-processing logic to detect an open tabbed picker, call
m.picker.cycleTab(-1), and return before the noBlockingModal fallback.
Alternatively, remove or narrow TestCyclingBackwardsWraps so it only verifies
the cycleTab method rather than user-reachable behavior; preserve the existing
forward Tab handling.
In `@internal/tui/session_picker_tabs_test.go`:
- Around line 69-76: Replace the construction-only assertions in
TestAnAgentWithNoSessionsGetsNoTab with regression coverage for
foreignSessionItems: use an injectable agentsessions.Env and fake discovery
results to verify sessions whose <agent>:<id> matches a local session’s
ParseImportTag are excluded, while non-matching imported sessions remain. Add
table cases covering matching, non-matching, and malformed import tags, reusing
the test injection pattern from model_test.go.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c1df6e0-d321-4254-bc75-bca5c98723d3
📒 Files selected for processing (25)
internal/agentsessions/activity.gointernal/agentsessions/activity_test.gointernal/agentsessions/cache.gointernal/agentsessions/cache_test.gointernal/agentsessions/codex.gointernal/agentsessions/codex_test.gointernal/agentsessions/family1.gointernal/agentsessions/family1_test.gointernal/agentsessions/import_resume_test.gointernal/agentsessions/jsonl.gointernal/agentsessions/jsonl_test.gointernal/agentsessions/paths.gointernal/agentsessions/paths_test.gointernal/agentsessions/registry.gointernal/agentsessions/translate.gointernal/agentsessions/translate_test.gointernal/agentsessions/types.gointernal/cli/sessions.gointernal/cli/sessions_import.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/picker.gointernal/tui/session.gointernal/tui/session_picker_tabs_test.gointernal/tui/view.go
| // withdraw removes a value a failed call had contributed. | ||
| func (log *activityLog) withdraw(claim pathClaim) { | ||
| list := &log.read | ||
| if claim.bucket == "changed" { | ||
| list = &log.changed | ||
| } | ||
| for index, value := range *list { | ||
| if value == claim.value { | ||
| *list = append((*list)[:index], (*list)[index+1:]...) | ||
| break | ||
| } | ||
| } | ||
| delete(log.seen, claim.bucket+"\x00"+claim.value) | ||
| } | ||
|
|
||
| // add appends value to list unless an equal value is already recorded under | ||
| // bucket. Deduplicated because agents re-read the same file repeatedly and a | ||
| // list of forty identical paths tells the reader nothing. | ||
| func (log *activityLog) add(bucket string, list *[]string, value string) { | ||
| trimmed := strings.TrimSpace(value) | ||
| if trimmed == "" { | ||
| return | ||
| } | ||
| key := bucket + "\x00" + trimmed | ||
| if log.seen[key] { | ||
| return | ||
| } | ||
| log.seen[key] = true | ||
| *list = append(*list, trimmed) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A failed call can withdraw a value another call recorded successfully.
add deduplicates by bucket + value, so two calls that touch the same path produce one entry. withdraw removes that single entry unconditionally. If call t1 reads parser.go and succeeds, and call t2 reads parser.go and fails, the successful read disappears from "Files read". The summary then understates the prior work, which is the failure mode this file exists to prevent.
Count the claims per key and withdraw only when the count reaches zero.
🐛 Proposed fix
- seen map[string]bool
+ // seen counts how many calls contributed each bucket/value pair, so a
+ // failed call cannot withdraw a value another call recorded successfully.
+ seen map[string]int
} func (log *activityLog) withdraw(claim pathClaim) {
+ key := claim.bucket + "\x00" + claim.value
+ if log.seen[key] > 1 {
+ log.seen[key]--
+ return
+ }
list := &log.read
if claim.bucket == "changed" {
list = &log.changed
}
for index, value := range *list {
if value == claim.value {
*list = append((*list)[:index], (*list)[index+1:]...)
break
}
}
- delete(log.seen, claim.bucket+"\x00"+claim.value)
+ delete(log.seen, key)
}
func (log *activityLog) add(bucket string, list *[]string, value string) {
trimmed := strings.TrimSpace(value)
if trimmed == "" {
return
}
key := bucket + "\x00" + trimmed
- if log.seen[key] {
+ if log.seen[key] > 0 {
+ log.seen[key]++
return
}
- log.seen[key] = true
+ log.seen[key] = 1
*list = append(*list, trimmed)
}Update newActivityLog to build seen: map[string]int{}.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentsessions/activity.go` around lines 89 - 118, Change activityLog
deduplication to track claim counts rather than booleans: update newActivityLog
to initialize seen as map[string]int, increment the bucket/value key in add, and
decrement it in withdraw. Remove the list entry and delete the key only when its
count reaches zero, preserving entries still referenced by other calls.
| func (log *activityLog) summaryEvents() []sessions.AppendEventInput { | ||
| if log == nil || log.calls == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| events := []sessions.AppendEventInput{} | ||
| headline := "Prior session activity: " + countPhrase(log.calls, "tool call") | ||
| if log.failed > 0 { | ||
| headline += ", " + countPhrase(log.failed, "failure") | ||
| } | ||
| headline += "." | ||
| if extra := log.toolBreakdown(); extra != "" { | ||
| headline += " " + extra | ||
| } | ||
| events = append(events, noteEvent(headline)) | ||
|
|
||
| for _, section := range []struct { | ||
| label string | ||
| items []string | ||
| }{ | ||
| {"Files read", log.read}, | ||
| {"Files changed", log.changed}, | ||
| {"Commands run", log.commands}, | ||
| {"Searched for", log.searches}, | ||
| {"Failures", log.failures}, | ||
| } { | ||
| if line := summaryLine(section.label, section.items); line != "" { | ||
| events = append(events, noteEvent(line)) | ||
| } | ||
| } | ||
| return events | ||
| } | ||
|
|
||
| // toolBreakdown names tools whose arguments yielded nothing, so an unrecognised | ||
| // schema degrades to "Also: exec x4" rather than to silence. | ||
| func (log *activityLog) toolBreakdown() string { | ||
| if len(log.toolCounts) == 0 { | ||
| return "" | ||
| } | ||
| names := make([]string, 0, len(log.toolCounts)) | ||
| for name := range log.toolCounts { | ||
| names = append(names, name) | ||
| } | ||
| sort.Strings(names) | ||
| parts := make([]string, 0, len(names)) | ||
| for _, name := range names { | ||
| count := log.toolCounts[name] | ||
| if count > 1 { | ||
| parts = append(parts, name+" x"+itoaEvents(count)) | ||
| continue | ||
| } | ||
| parts = append(parts, name) | ||
| } | ||
| return truncateToBudget("Also: "+strings.Join(parts, ", "), maxSummaryEventChars) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The headline event can exceed its own character budget.
toolBreakdown truncates to maxSummaryEventChars (460). The result is then appended to a headline that already holds "Prior session activity: N tool calls, M failures." So the combined string reaches about 510 characters. That is over the 460 budget and over the 500-character digest limit this file exists to respect, so the headline is cut mid-sentence by sessions.summarizePayload.
TestEverySummaryEventSurvivesTheDigestIntact does not catch this, because its fixture uses only Read calls and leaves toolCounts empty. Truncate the assembled headline, and add a case with many unrecognised tool names.
🐛 Proposed fix
if extra := log.toolBreakdown(); extra != "" {
headline += " " + extra
}
- events = append(events, noteEvent(headline))
+ events = append(events, noteEvent(truncateToBudget(headline, maxSummaryEventChars)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (log *activityLog) summaryEvents() []sessions.AppendEventInput { | |
| if log == nil || log.calls == 0 { | |
| return nil | |
| } | |
| events := []sessions.AppendEventInput{} | |
| headline := "Prior session activity: " + countPhrase(log.calls, "tool call") | |
| if log.failed > 0 { | |
| headline += ", " + countPhrase(log.failed, "failure") | |
| } | |
| headline += "." | |
| if extra := log.toolBreakdown(); extra != "" { | |
| headline += " " + extra | |
| } | |
| events = append(events, noteEvent(headline)) | |
| for _, section := range []struct { | |
| label string | |
| items []string | |
| }{ | |
| {"Files read", log.read}, | |
| {"Files changed", log.changed}, | |
| {"Commands run", log.commands}, | |
| {"Searched for", log.searches}, | |
| {"Failures", log.failures}, | |
| } { | |
| if line := summaryLine(section.label, section.items); line != "" { | |
| events = append(events, noteEvent(line)) | |
| } | |
| } | |
| return events | |
| } | |
| // toolBreakdown names tools whose arguments yielded nothing, so an unrecognised | |
| // schema degrades to "Also: exec x4" rather than to silence. | |
| func (log *activityLog) toolBreakdown() string { | |
| if len(log.toolCounts) == 0 { | |
| return "" | |
| } | |
| names := make([]string, 0, len(log.toolCounts)) | |
| for name := range log.toolCounts { | |
| names = append(names, name) | |
| } | |
| sort.Strings(names) | |
| parts := make([]string, 0, len(names)) | |
| for _, name := range names { | |
| count := log.toolCounts[name] | |
| if count > 1 { | |
| parts = append(parts, name+" x"+itoaEvents(count)) | |
| continue | |
| } | |
| parts = append(parts, name) | |
| } | |
| return truncateToBudget("Also: "+strings.Join(parts, ", "), maxSummaryEventChars) | |
| } | |
| func (log *activityLog) summaryEvents() []sessions.AppendEventInput { | |
| if log == nil || log.calls == 0 { | |
| return nil | |
| } | |
| events := []sessions.AppendEventInput{} | |
| headline := "Prior session activity: " + countPhrase(log.calls, "tool call") | |
| if log.failed > 0 { | |
| headline += ", " + countPhrase(log.failed, "failure") | |
| } | |
| headline += "." | |
| if extra := log.toolBreakdown(); extra != "" { | |
| headline += " " + extra | |
| } | |
| events = append(events, noteEvent(truncateToBudget(headline, maxSummaryEventChars))) | |
| for _, section := range []struct { | |
| label string | |
| items []string | |
| }{ | |
| {"Files read", log.read}, | |
| {"Files changed", log.changed}, | |
| {"Commands run", log.commands}, | |
| {"Searched for", log.searches}, | |
| {"Failures", log.failures}, | |
| } { | |
| if line := summaryLine(section.label, section.items); line != "" { | |
| events = append(events, noteEvent(line)) | |
| } | |
| } | |
| return events | |
| } | |
| // toolBreakdown names tools whose arguments yielded nothing, so an unrecognised | |
| // schema degrades to "Also: exec x4" rather than to silence. | |
| func (log *activityLog) toolBreakdown() string { | |
| if len(log.toolCounts) == 0 { | |
| return "" | |
| } | |
| names := make([]string, 0, len(log.toolCounts)) | |
| for name := range log.toolCounts { | |
| names = append(names, name) | |
| } | |
| sort.Strings(names) | |
| parts := make([]string, 0, len(names)) | |
| for _, name := range names { | |
| count := log.toolCounts[name] | |
| if count > 1 { | |
| parts = append(parts, name+" x"+itoaEvents(count)) | |
| continue | |
| } | |
| parts = append(parts, name) | |
| } | |
| return truncateToBudget("Also: "+strings.Join(parts, ", "), maxSummaryEventChars) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentsessions/activity.go` around lines 258 - 312, Update
activityLog.summaryEvents to apply maxSummaryEventChars to the fully assembled
headline after adding the toolBreakdown text, rather than relying on
toolBreakdown’s independent truncation. Preserve the existing count and
breakdown content while ensuring the emitted headline stays within the event
budget, and extend the relevant summary test with many unrecognised tool names
to cover this case.
| func DiscoverAllCached(env Env, cwd string) ([]ForeignSession, []error) { | ||
| discoveryMu.Lock() | ||
| defer discoveryMu.Unlock() | ||
|
|
||
| if entry, ok := discoveryCache[cwd]; ok && discoveryNow().Sub(entry.at) < discoveryTTL { | ||
| // Copy: callers sort and filter the slice they are handed, and a shared | ||
| // backing array would let one caller reorder another's results. | ||
| return append([]ForeignSession{}, entry.sessions...), entry.problems | ||
| } | ||
|
|
||
| found, problems := DiscoverAll(env, cwd) | ||
| discoveryCache[cwd] = discoveryEntry{sessions: found, problems: problems, at: discoveryNow()} | ||
| return append([]ForeignSession{}, found...), problems | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Discovery runs while the global lock is held.
DiscoverAll reads four transcript stores and costs 50-300ms per the comment at Lines 10-13. That call happens at Line 48 with discoveryMu held. Every other caller of DiscoverAllCached blocks for the full duration, including callers for a different workspace that already have a valid memo. The comment states the goal is to stop the TUI hitching; a global lock around the slow path reintroduces the hitch on a concurrent open.
Release the lock while discovering, then re-acquire it to store the entry. Accept that two concurrent misses may both discover; that is cheaper than serializing every caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentsessions/cache.go` around lines 38 - 51, Update
DiscoverAllCached so the discoveryMu lock is held only while checking the cache
and storing results, not while calling the slow DiscoverAll operation. Unlock
before discovery, allow concurrent misses (including different workspaces) to
proceed independently, then re-acquire the lock to write the discovered entry
and return the copied sessions and problems.
| func TestTheRealCodexCorpusStillParses(t *testing.T) { | ||
| env := OSEnv() | ||
| root := codexRoot(env) | ||
| if root == "" { | ||
| t.Skip("no home directory") | ||
| } | ||
| if _, err := os.Stat(root); err != nil { | ||
| t.Skip("no Codex store on this machine") | ||
| } | ||
| adapter := Codex(env) | ||
| found, err := adapter.Discover("") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| total := len(adapter.(codex).transcripts()) | ||
| if total == 0 { | ||
| t.Skip("store exists but holds no rollouts") | ||
| } | ||
| titled, modelled := 0, 0 | ||
| for _, session := range found { | ||
| if session.Title != "" && session.Title != "untitled" && !isCodexContextInjection(session.Title) { | ||
| titled++ | ||
| } | ||
| if session.ModelID != "" { | ||
| modelled++ | ||
| } | ||
| } | ||
| t.Logf("indexed %d of %d rollouts; %d titled, %d with a model", len(found), total, titled, modelled) | ||
| if len(found) == 0 { | ||
| t.Fatal("no Codex sessions indexed from a non-empty store") | ||
| } | ||
| // Both of these were zero before the fixes above; a regression takes them | ||
| // back to zero rather than to some slightly-lower number. | ||
| if titled == 0 { | ||
| t.Error("no session got a real title — the context-injection filter has stopped working") | ||
| } | ||
| if modelled == 0 { | ||
| t.Error("no session got a model — turn_context is being discarded again") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Gate the live-corpus test behind an explicit opt-in.
This test reads the developer's real ~/.codex store during a normal go test ./.... Two consequences follow. First, the result depends on private local data, so the same commit passes on one machine and fails on another; the t.Error calls at Lines 183-188 report a defect that no change in this PR caused. Second, the test opens the contributor's real transcripts, and t.Logf reports counts derived from them.
Require an opt-in environment variable, so the default suite stays hermetic.
The coding guidelines state: "path-sensitive logic must include a non-Linux case or a hermetic equivalent exercising the same normalization."
♻️ Proposed fix
func TestTheRealCodexCorpusStillParses(t *testing.T) {
+ if os.Getenv("ZERO_TEST_LIVE_CORPUS") == "" {
+ t.Skip("set ZERO_TEST_LIVE_CORPUS=1 to run against the local Codex store")
+ }
env := OSEnv()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestTheRealCodexCorpusStillParses(t *testing.T) { | |
| env := OSEnv() | |
| root := codexRoot(env) | |
| if root == "" { | |
| t.Skip("no home directory") | |
| } | |
| if _, err := os.Stat(root); err != nil { | |
| t.Skip("no Codex store on this machine") | |
| } | |
| adapter := Codex(env) | |
| found, err := adapter.Discover("") | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| total := len(adapter.(codex).transcripts()) | |
| if total == 0 { | |
| t.Skip("store exists but holds no rollouts") | |
| } | |
| titled, modelled := 0, 0 | |
| for _, session := range found { | |
| if session.Title != "" && session.Title != "untitled" && !isCodexContextInjection(session.Title) { | |
| titled++ | |
| } | |
| if session.ModelID != "" { | |
| modelled++ | |
| } | |
| } | |
| t.Logf("indexed %d of %d rollouts; %d titled, %d with a model", len(found), total, titled, modelled) | |
| if len(found) == 0 { | |
| t.Fatal("no Codex sessions indexed from a non-empty store") | |
| } | |
| // Both of these were zero before the fixes above; a regression takes them | |
| // back to zero rather than to some slightly-lower number. | |
| if titled == 0 { | |
| t.Error("no session got a real title — the context-injection filter has stopped working") | |
| } | |
| if modelled == 0 { | |
| t.Error("no session got a model — turn_context is being discarded again") | |
| } | |
| } | |
| func TestTheRealCodexCorpusStillParses(t *testing.T) { | |
| if os.Getenv("ZERO_TEST_LIVE_CORPUS") == "" { | |
| t.Skip("set ZERO_TEST_LIVE_CORPUS=1 to run against the local Codex store") | |
| } | |
| env := OSEnv() | |
| root := codexRoot(env) | |
| if root == "" { | |
| t.Skip("no home directory") | |
| } | |
| if _, err := os.Stat(root); err != nil { | |
| t.Skip("no Codex store on this machine") | |
| } | |
| adapter := Codex(env) | |
| found, err := adapter.Discover("") | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| total := len(adapter.(codex).transcripts()) | |
| if total == 0 { | |
| t.Skip("store exists but holds no rollouts") | |
| } | |
| titled, modelled := 0, 0 | |
| for _, session := range found { | |
| if session.Title != "" && session.Title != "untitled" && !isCodexContextInjection(session.Title) { | |
| titled++ | |
| } | |
| if session.ModelID != "" { | |
| modelled++ | |
| } | |
| } | |
| t.Logf("indexed %d of %d rollouts; %d titled, %d with a model", len(found), total, titled, modelled) | |
| if len(found) == 0 { | |
| t.Fatal("no Codex sessions indexed from a non-empty store") | |
| } | |
| // Both of these were zero before the fixes above; a regression takes them | |
| // back to zero rather than to some slightly-lower number. | |
| if titled == 0 { | |
| t.Error("no session got a real title — the context-injection filter has stopped working") | |
| } | |
| if modelled == 0 { | |
| t.Error("no session got a model — turn_context is being discarded again") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentsessions/codex_test.go` around lines 150 - 189, Gate
TestTheRealCodexCorpusStillParses behind an explicit opt-in environment
variable, returning via t.Skip before accessing codexRoot, OSEnv, or the
developer’s transcripts when the variable is unset. Preserve the existing
live-corpus assertions for opted-in runs, and keep path-sensitive behavior
covered through a hermetic or non-Linux test rather than relying on this live
test.
Source: Coding guidelines
| func claudeCodeRoot(env Env) string { | ||
| if dir := env.lookup("CLAUDE_CONFIG_DIR"); dir != "" { | ||
| return filepath.Join(dir, "projects") | ||
| } | ||
| return env.underHome(".claude", "projects") | ||
| } | ||
|
|
||
| func codexRoot(env Env) string { | ||
| if dir := env.lookup("CODEX_HOME"); dir != "" { | ||
| return filepath.Join(dir, "sessions") | ||
| } | ||
| return env.underHome(".codex", "sessions") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Reject a relative redirect value instead of probing the working directory.
env.underHome returns "" when home is unknown, so discovery never probes a relative path. The redirect branches do not apply the same rule. If CLAUDE_CONFIG_DIR=.config/claude is set, claudeCodeRoot returns .config/claude/projects and discovery reads relative to the process working directory. Require an absolute path in both branches.
🛡️ Proposed fix
func claudeCodeRoot(env Env) string {
- if dir := env.lookup("CLAUDE_CONFIG_DIR"); dir != "" {
+ if dir := env.lookup("CLAUDE_CONFIG_DIR"); filepath.IsAbs(dir) {
return filepath.Join(dir, "projects")
}
return env.underHome(".claude", "projects")
}
func codexRoot(env Env) string {
- if dir := env.lookup("CODEX_HOME"); dir != "" {
+ if dir := env.lookup("CODEX_HOME"); filepath.IsAbs(dir) {
return filepath.Join(dir, "sessions")
}
return env.underHome(".codex", "sessions")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func claudeCodeRoot(env Env) string { | |
| if dir := env.lookup("CLAUDE_CONFIG_DIR"); dir != "" { | |
| return filepath.Join(dir, "projects") | |
| } | |
| return env.underHome(".claude", "projects") | |
| } | |
| func codexRoot(env Env) string { | |
| if dir := env.lookup("CODEX_HOME"); dir != "" { | |
| return filepath.Join(dir, "sessions") | |
| } | |
| return env.underHome(".codex", "sessions") | |
| } | |
| func claudeCodeRoot(env Env) string { | |
| if dir := env.lookup("CLAUDE_CONFIG_DIR"); filepath.IsAbs(dir) { | |
| return filepath.Join(dir, "projects") | |
| } | |
| return env.underHome(".claude", "projects") | |
| } | |
| func codexRoot(env Env) string { | |
| if dir := env.lookup("CODEX_HOME"); filepath.IsAbs(dir) { | |
| return filepath.Join(dir, "sessions") | |
| } | |
| return env.underHome(".codex", "sessions") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentsessions/paths.go` around lines 48 - 60, Update claudeCodeRoot
and codexRoot so configured CLAUDE_CONFIG_DIR or CODEX_HOME values are used only
when absolute; treat relative values like unset configuration and fall back to
env.underHome with the existing default subpaths.
| working, err := os.Getwd() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| if filepath.Clean(working) == filepath.Clean(recorded) { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
filepath.Clean is not enough to compare two workspace paths.
Clean only performs lexical tidying. It does not resolve symlinks, and it does not fold case. Two consequences:
- On macOS,
os.Getwdfrequently returns a/private/var/...path while the recorded cwd holds/var/...for the same directory. The paths differ lexically, so the warning fires for a session that ran right here. - On Windows, the comparison is case-sensitive against a case-insensitive filesystem.
C:\Work\repoandC:\work\repocompare unequal and produce the same spurious warning.
The failure mode is a misleading advisory line, not data loss. It is still worth fixing, because the note exists precisely to prevent confusion.
internal/tui/session.go line 483 answers this same question with sessionMatchesWorkspace. Reuse that predicate here rather than maintaining a second, weaker comparison.
🐛 Proposed fix
working, err := os.Getwd()
if err != nil {
return ""
}
- if filepath.Clean(working) == filepath.Clean(recorded) {
+ if sessionMatchesWorkspace(recorded, working) {
return ""
}sessionMatchesWorkspace currently lives in internal/tui. Promote it to a shared package so both callers use one implementation.
As per coding guidelines: "Code and tests must pass on Linux, macOS, and Windows; canonicalize paths before comparison and avoid asserting raw temporary-directory spellings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/sessions_import.go` around lines 230 - 236, Replace the lexical
filepath.Clean comparison in the sessions import workspace check with the shared
sessionMatchesWorkspace predicate. Promote sessionMatchesWorkspace from
internal/tui/session.go to an appropriate shared package, update both callers to
use it, and preserve the existing empty-string behavior when the workspaces
match or the current directory cannot be determined.
Source: Coding guidelines
| // Meta now carries the source agent ("zero", "codex", …) so the picker's | ||
| // All tab says where each session came from. What it must never carry is | ||
| // the raw session id, which is what this check has always been about: | ||
| // rendering the id consumed half the picker and truncated the title. | ||
| if strings.Contains(item.Meta, want.id) { | ||
| t.Fatalf("picker %q exposes the raw session id in metadata: %q", want.title, item.Meta) | ||
| } | ||
| if item.Meta != "zero" { | ||
| t.Fatalf("picker %q metadata = %q, want the source agent", want.title, item.Meta) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The session picker resolves its agent environment at the leaf, which makes TUI tests host-dependent and blocks the missing dedup test. newSessionPicker → foreignSessionItems calls agentsessions.DiscoverAllCached(agentsessions.OSEnv(), m.cwd), and OSEnv reads the real os.UserHomeDir. One root cause produces both problems below: existing picker tests observe whatever transcripts the host happens to have, and no test can supply a controlled discovery result. Thread an agentsessions.Env through the model so both are fixable.
internal/tui/model_test.go#L908-L917: this test passes only on a machine with no Claude Code, Codex, Factory Droid, or Pi transcripts. Extra discovered rows break the"1 / 2"assertion on line 921. Build the model with an injectedEnvpointing at at.TempDir()home.internal/tui/session_picker_tabs_test.go#L69-L76: add a test for the imported-session dedup rule inforeignSessionItems, which currently has no coverage, and strengthenTestAnAgentWithNoSessionsGetsNoTab, which cannot fail as written. Both need the same injectedEnv.
As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths".
📍 Affects 2 files
internal/tui/model_test.go#L908-L917(this comment)internal/tui/session_picker_tabs_test.go#L69-L76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/model_test.go` around lines 908 - 917, Thread an
agentsessions.Env through the model and session-picker construction so
newSessionPicker and foreignSessionItems use the injected environment instead of
agentsessions.OSEnv(). In internal/tui/model_test.go lines 908-917, build the
model with a t.TempDir() home to isolate discovery. In
internal/tui/session_picker_tabs_test.go lines 69-76, use the same injected Env,
add coverage for imported-session deduplication in foreignSessionItems, and
strengthen TestAnAgentWithNoSessionsGetsNoTab so it genuinely verifies the
no-tab behavior.
Source: Coding guidelines
| agent := sessionAgentName(meta.Tag) | ||
| items = append(items, pickerItem{ | ||
| Label: label, | ||
| Value: meta.SessionID, | ||
| // Shown on the right of the row, so the "All" tab says at a glance | ||
| // which agent each session came from. | ||
| Meta: agent, | ||
| Tab: agent, | ||
| }) | ||
| } | ||
| items = append(items, m.foreignSessionItems(metas, now)...) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The "All" tab is not sorted by recency once foreign sessions are appended.
Local items arrive in ListResumable order. foreignSessionItems returns its own recency-ordered list. Line 444 concatenates the second after the first, so the combined list is two sorted runs, not one. Each row leads with a timestamp, which makes the discontinuity visible: a Codex session from ten minutes ago sits below a Zero session from last March.
The per-agent tabs are unaffected, because each tab shows one run. Only "All" — the default tab, and the one this feature exists for — reads as unordered.
Sort the merged slice before building the picker. That needs a sortable timestamp on pickerItem, since Label is already formatted for display.
🐛 Sketch of the fix
Carry the update time alongside each item while assembling, then sort once:
- items = append(items, m.foreignSessionItems(metas, now)...)
+ items = append(items, m.foreignSessionItems(metas, now)...)
+ sortPickerItemsByRecency(items)sortPickerItemsByRecency needs the raw time.Time per item. Add an unexported field to pickerItem, or build a parallel []struct{ item pickerItem; at time.Time } inside newSessionPicker and emit the sorted items from it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent := sessionAgentName(meta.Tag) | |
| items = append(items, pickerItem{ | |
| Label: label, | |
| Value: meta.SessionID, | |
| // Shown on the right of the row, so the "All" tab says at a glance | |
| // which agent each session came from. | |
| Meta: agent, | |
| Tab: agent, | |
| }) | |
| } | |
| items = append(items, m.foreignSessionItems(metas, now)...) | |
| items = append(items, m.foreignSessionItems(metas, now)...) | |
| sortPickerItemsByRecency(items) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/session.go` around lines 434 - 444, Update newSessionPicker to
retain each session’s raw update time on pickerItem, including items from both
local assembly and foreignSessionItems, then sort the merged items by recency
before building the picker. Add or reuse sortPickerItemsByRecency so sorting
uses time.Time rather than the formatted Label, while preserving per-agent item
behavior.
| result, err := agentsessions.Import(m.sessionStore, adapter, id, agentsessions.ReadOptions{}) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| // This session is no longer un-imported, so the memo that says otherwise | ||
| // must go before the picker is rebuilt. | ||
| agentsessions.InvalidateDiscovery() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check transcript read bounds and the model's async command pattern.
set -euo pipefail
echo "=== MaxEvents handling in the read path ==="
rg -n -C4 'MaxEvents' internal/agentsessions --glob '!*_test.go'
echo "=== whole-file vs bounded reads ==="
rg -n -C3 'io.ReadAll|bufio.Scanner|os.Open' internal/agentsessions --glob '!*_test.go'
echo "=== existing tea.Cmd dispatch for slow work ==="
rg -n -C3 'func .*Cmd\(\) tea\.Cmd' internal/tui | head -60Repository: Gitlawb/zero
Length of output: 7803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== importForeignSession and its callers ==="
sed -n '430,510p' internal/tui/session.go
rg -n -C8 'handleResumeCommand|importForeignSession|type .*Msg|Import\(' internal/tui internal/agentsessions --glob '*.go'
echo "=== Import implementation ==="
rg -n -C10 'func Import|func \(.*\) AppendEvents|func .*Read\(' internal/agentsessions --glob '*.go'
echo "=== Update return paths around resume handling ==="
rg -n -C12 'handleResumeCommand|resume' internal/tui/model.go internal/tui/session.go --glob '*.go' | head -240Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== handleResumeCommand and command dispatch ==="
sed -n '205,270p' internal/tui/session.go
rg -n -C10 'handle.*Command|resumeText\(\)|command.*resume|/resume' internal/tui/model.go internal/tui/*.go --glob '*.go' | grep -E 'handleResumeCommand|resumeText|/resume|command' | head -180
echo "=== Import call graph ==="
sed -n '120,205p' internal/agentsessions/registry.go
rg -n -C8 'func \(.*\) Read|func .*Read\(id string|capEvents|streamLines|scanHead' internal/agentsessions --glob '*.go' | head -240
echo "=== relevant async command shape ==="
sed -n '1,100p' internal/tui/command_bash.go
sed -n '40,105p' internal/tui/files_git_sweep.goRepository: Gitlawb/zero
Length of output: 15983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Import implementation ==="
sed -n '135,190p' internal/agentsessions/registry.go
sed -n '50,180p' internal/agentsessions/jsonl.go
echo "=== adapter Read implementations ==="
rg -n '^func .*Read\(.*ReadOptions|^func \(.*\) Read\(' internal/agentsessions --glob '*.go'
for f in $(rg -l '^func .*Read\(.*ReadOptions|^func \(.*\) Read\(' internal/agentsessions --glob '*.go'); do
echo "--- $f"
grep -n -A55 -B5 'func .*Read(' "$f" | head -80
done
echo "=== command dispatch references ==="
rg -n -C6 'handleResumeCommand\(' internal/tui --glob '*.go'
rg -n -C6 'Update\(.*tea.Msg|func \(m model\) Update|parseCommand' internal/tui/model.go internal/tui/*.go --glob '*.go' | head -220Repository: Gitlawb/zero
Length of output: 30533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
session = Path("internal/tui/session.go").read_text()
registry = Path("internal/agentsessions/registry.go").read_text()
jsonl = Path("internal/agentsessions/jsonl.go").read_text()
types = Path("internal/agentsessions/types.go").read_text()
checks = {
"TUI passes zero-value ReadOptions": "agentsessions.Import(m.sessionStore, adapter, id, agentsessions.ReadOptions{})" in session,
"zero MaxEvents means no cap": "Zero means no cap." in types,
"Import reads before store append": registry.index("events, err := adapter.Read(id, options)") < registry.index("store.AppendEvents(created.SessionID, events)"),
"full-read path is explicitly unbounded": "streamLines calls visit with every line of path, without bounding the total." in jsonl,
"full-read loop continues until EOF": "for {" in jsonl[jsonl.index("func streamLines"):jsonl.index("func readBoundedLine")],
}
for name, result in checks.items():
print(f"{name}: {result}")
if not all(checks.values()):
raise SystemExit(1)
PY
echo "=== exact Update call sites ==="
sed -n '4238,4262p' internal/tui/model.go
sed -n '4538,4565p' internal/tui/model.goRepository: Gitlawb/zero
Length of output: 2749
Move foreign-session import off the Bubble Tea Update loop.
agentsessions.Import synchronously reads the transcript to EOF, translates all events, creates the session, and appends the events. ReadOptions{} sets no MaxEvents cap. A large transcript can therefore block rendering and input handling, including Ctrl+C, until the import completes.
Return a tea.Cmd that performs the import and sends a result message. Consider an explicit MaxEvents cap if full imports are not required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/session.go` around lines 473 - 479, Move the synchronous
agentsessions.Import call out of the Bubble Tea Update path into a tea.Cmd that
performs the import asynchronously and returns a result message containing the
session or error, then handle that message in the Update flow while preserving
agentsessions.InvalidateDiscovery before rebuilding the picker. Review whether
the import should set an explicit MaxEvents limit instead of using uncapped
ReadOptions{}.
| label := displayValue(session.Title, "untitled") | ||
| if when := sessionWhen(session.UpdatedAt.Format(time.RFC3339), now); when != "" { | ||
| label = sessionPickerLabel(when, label) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the zero UpdatedAt before formatting it.
session.UpdatedAt.Format(time.RFC3339) on a zero time.Time yields "0001-01-01T00:00:00Z". sessionWhen then parses a valid timestamp and returns a stamp for the year 1, which sessionPickerLabel prints. The CLI path guards this case explicitly — formatDiscoveredTime returns "" for a zero time.
ForeignSession.UpdatedAt falls back to the file modification time, so it is normally populated. An adapter that cannot stat the file leaves it zero, and that row then renders a January year-1 date.
The round trip through a string is also avoidable if sessionWhen gains a time.Time variant, since the value here is already typed.
🐛 Minimal fix
label := displayValue(session.Title, "untitled")
- if when := sessionWhen(session.UpdatedAt.Format(time.RFC3339), now); when != "" {
- label = sessionPickerLabel(when, label)
+ if !session.UpdatedAt.IsZero() {
+ if when := sessionWhen(session.UpdatedAt.Format(time.RFC3339), now); when != "" {
+ label = sessionPickerLabel(when, label)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label := displayValue(session.Title, "untitled") | |
| if when := sessionWhen(session.UpdatedAt.Format(time.RFC3339), now); when != "" { | |
| label = sessionPickerLabel(when, label) | |
| } | |
| label := displayValue(session.Title, "untitled") | |
| if !session.UpdatedAt.IsZero() { | |
| if when := sessionWhen(session.UpdatedAt.Format(time.RFC3339), now); when != "" { | |
| label = sessionPickerLabel(when, label) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/session.go` around lines 515 - 518, Guard
session.UpdatedAt.IsZero() before formatting it, so zero timestamps do not reach
sessionWhen or sessionPickerLabel and produce a year-1 date. Update the
surrounding label logic in the session row path, preferably by reusing or adding
a typed time.Time variant of sessionWhen to avoid converting the timestamp
through RFC3339 text while preserving existing behavior for populated
timestamps.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed as a draft, so this is findings rather than a verdict. The design question you actually asked about is above my pay grade and needs @kevincodex1; what follows is whether the code does what it says.
The credential-safety work is the strongest part and it mostly holds. I checked the claims rather than taking them: the extension pin is case-insensitive, globTranscripts rejects symlinks because IsRegular() is false for them, and I confirmed by probe that a junction is rejected too. Two adversarial passes tried to turn the reparse-point gap into an escape and could not: creating a link under ~/.codex/sessions already requires write access to ~/.codex/sessions, and writing a transcript there directly reaches the same outcome with no link at all. The .jsonl pin plus the rollout-* pin plus Discover gating Import close the residual.
Two blocking, though.
The activity summary is emitted as EventCompaction, whose payload contract it does not satisfy. RehydrateEvents (replay.go:240) scans backwards for the last EventCompaction and restructures the transcript around it. A real CompactionPayload carries PreserveLast, CompactableEvents, PreservedEvents and CompactedThroughSequence — the bookkeeping saying which events the summary replaces. noteEvent writes {"summary": ...} and nothing else, so every one of those is zero, and rehydration reorders the imported transcript around a boundary that describes nothing. It decodes cleanly because the only validated field is Summary. Verified end to end through Import → ReadRehydratedEvents → PrepareExec. You picked the type because promptContextEvents already passes it; the same type has a second contract on the replay side.
Imported text carries control bytes into the terminal. The redaction chokepoint scrubs secrets, not control characters. Probed directly: "innocent title\x1b[2J\x1b[1;1H FORGED ROW \x00 tail" comes back byte-identical, ESC and NUL intact, and that string becomes a picker row and a transcript line. We have shipped this exact class twice in a fortnight: #835, where an MCP failure reason forged a row, and #876, where a copied NUL panicked the whole TUI. An imported title is strictly more attacker-influenced than either. sanitizeCardText already exists.
Two worth fixing before it leaves draft.
TestTheRealCodexCorpusStillParses and TestTheRealCorpusStillParses discover against the real ~/.codex and ~/.claude of whoever runs go test, and assert on what they find. The first fails at your head on this machine (indexed 2 of 2 rollouts; 2 titled, 0 with a model) because these rollouts carry turn_context past the 64-line head budget, which no change to the adapter can fix. CI passes only because the runner has no store to find. That inverts the usual bargain: green on CI, red for contributors. Worth a fixture.
The activity summary collapses successful and failed calls into one bucket per path. A successful Write /p/config.yaml followed by a failed Edit of the same path withdraws the claim entirely, so the summary reports no files changed although the file was rewritten. The withdraw logic is right in principle; it is keyed too coarsely.
Smaller: the family-1 slug fast path skips globSessionDirs, so the picker can list a session Import then refuses; name, toolCallId and role skip redact() while content and arguments get it, so the chokepoint comment is not literally true; capEvents understates the drop by one, and the note is the only thing telling the reader the import is partial; a tool call with no matching result keeps its claim, so an interrupted write reports as a file changed.
Two things I checked and am NOT raising, so you do not chase them. The Title field skipping redaction is real but pre-existing: createSessionTitle on main writes a raw prompt into metadata.json for native sessions too, and zero sessions list redacts at display. Your translate.go redaction is above baseline, not below it. And the reparse-point discovery gap is a documentation inaccuracy rather than a boundary crossing, for the reason above.
The engineering standard here is high: mutation-testing the glob and the redaction, exercising against 302 real sessions, and documenting the two pre-existing main failures instead of claiming a clean run. The two blocking items are both "this type/string has a second contract elsewhere", which is the hardest class to see from inside the change.
|
One correction to the blocking item above, since the sentence ran together: a real The cheapest fix is probably a distinct event type rather than filling in the payload, since the import is not a compaction and pretending otherwise will keep colliding with replay, rewind and lineage. If |
|
Tested the latest head with real local session data. Discovery, CLI import, source tabs, and importing/resuming a selected session all work on the normal path. I found three blockers:
There is also a smaller UX concern: importing a 2,692-event session synchronously blocked the UI for about 0.86s on this machine. Please fix at least the first three before merge. |
3 participants
Summary
Zero can now read the sessions other coding agents leave on the local disk — Claude Code, Codex, Factory Droid and Pi — list them, and continue that work in Zero.
In the TUI,
/resumegains a tab strip (All · zero · claude-code · codex · factory · pi) and lists un-imported sessions directly — choosing one imports and resumes in a single step.Draft, and deliberately so. There is no parent issue yet. Opening this to make the design concrete before asking for one, because the neighbourhood is sensitive — see Scope below.
Scope: how this differs from #399
#399 (
internal/agentcli) was closed on a deliberate design line: Zero talks to model APIs directly, does not wrap other vendors' CLIs, and does not reuse another product's subscription login. That closure invited "a narrow, self-contained slice… with no subprocess harness and no borrowed-identity tokens".This is that slice:
claude/codexbinariesImport is strictly one-way: nothing is written to, moved in, or locked in another agent's store.
Why it is small
sessions.FormatExecPrompt— behind bothzero exec --resumeand the TUI's/resume— renders the event log to a text digest rather than rehydrating a provider-native conversation. So an importer never has to reconstructtool_use/tool_resultpairs into Anthropic- or OpenAI-shaped messages. It only has to emit ZeroEventrecords, after which resume, fork, rewind, compaction, lineage and the picker all work unchanged.Four agents cost two parsers: Claude Code, Factory Droid and Pi independently converged on the same layout, so one family-1 parser serves all three. Codex needs its own (date-partitioned, payload-wrapped).
Credential safety
Every one of the surveyed agents keeps live credentials in the same tree as its transcripts —
~/.codex/auth.json(OPENAI_API_KEY + OAuth),~/.gemini/oauth_creds.json,~/.claude/.credentials.json,~/.grok/auth.json,~/.factory/auth.v2.key, and~/.pi/agent/auth.json, which is the direct sibling of~/.pi/agent/sessions/.So discovery is fixed-depth globs pinned to one extension, never
filepath.WalkDir; symlinks are rejected byLstat(a link namedx.jsonlpointing atauth.jsonotherwise passes the extension check); and a session id is resolved by comparing glob results, never by joining the id onto a root, so../../authmatches nothing.Imported text is untrusted input and passes through
internal/redactionat a single chokepoint.Both properties are mutation-tested: swapping the glob for a walk, or gutting the redaction call, each fail a test.
Tool work reaching the model
sessions.promptContextEventspasses messages but notEventToolCall/EventToolResult. Without help, a 22-event import gave the continuing model 2 messages and ~1,155 characters — no knowledge that any file had been touched.Zero's own compaction cannot substitute:
toolPayloadPreviewallow-listsid/name/toolName/statusand dropsargumentsandoutput, so a summariser learns that a Read failed but never which file or why. Those values are still in hand at translation time.So the translator emits an activity summary as
EventCompaction— a type the filter already passes — one event per category, each under the digest's 500-character per-event budget.promptContextEventsis untouched; native resumes are unaffected.A call whose result failed withdraws its claim, so a Read of a path that does not exist is never reported as a file that was read.
Behaviour changes to existing code
internal/tui/model_test.go: the session-picker assertion moves fromMeta == ""to "Meta must not contain the session id, and must name the source agent". That check has always been about keeping the raw id out of the row; empty-string was a proxy for it.applyQuerygains a tab filter that is a no-op for every picker without a tab strip (covered by a test).Verification
make fmt-check,go vet ./...,go build ./...,git diff HEAD --check— cleango test ./...— all packages pass except two pre-existing failures onmain:TestRunDoctorFormatsRedactedProviderDiagnosticsandTestRunDoctorConnectivityProbesProvider. Both reproduce on a pristineorigin/mainworktree with no changes from this branch.go test -race ./internal/agentsessions/— cleangolangci-lint(unused,ineffassign,staticcheck) — no findings in the new codebridge-sessionstubs), 14/14 Codex rollouts. Import →--resumeverified end to end.applyQuery, removed failed-call withdrawal, oversized summary events, summaries emitted before the conversation — each fails its test.Not included
Cursor, Cline, Roo, Windsurf, Continue, Aider, Grok, opencode and Gemini. Cursor and the VS Code family store chats in undocumented
state.vscdbblobs with no stability guarantee, and none were installed on the machine this was built against — there is no fixture to test them against, so shipping them would be guesswork.Known limits
Resume continues the work, not the process: the conversation, tool activity, cwd, branch and last state in flight are recoverable; the other agent's in-memory context, prompt cache and half-executed tool call are not. The activity summary is an activity log, not comprehension — it says what was done, never why.
Every one of these formats is a private, undocumented implementation detail of another product and will drift. That recurring maintenance, not the initial build, is the real cost — hence one small adapter per agent, each independently skippable, each pinned to checked-in fixtures so a format change fails a test rather than a user's import.
Summary by CodeRabbit
New Features
discoverandimportcommands with workspace, agent, output, event-limit, and reasoning options.Bug Fixes