fix(T-994): Preserve real session timestamps when resolving file-backed transcripts

[ArjenSchwarz]

Fixes Transit ticket T-994: Preserve real session timestamps when resolving file-backed transcripts

[claude]

Code Review — PR #154: Preserve real session timestamps when resolving file-backed transcripts

Overview

This PR fixes a consistency gap between Lister and Resolver: the lister already extracted source-specific timestamps (transcript first-entry, session_meta, workspace.yaml) for CreatedAt, but the resolver fell back to info.ModTime() unconditionally. The fix introduces resolveFileCreatedAt() that mirrors the lister's logic, plus regression tests that prove file mtime is no longer used when real metadata is available.

The change is well-scoped, clearly motivated, and the tests are properly structured.

---

Issues

Inaccurate comment on resolveFileCreatedAt

The doc comment says:

For Claude and Codex, the file is seeked back to the start after parsing.

This is incorrect. Only Claude seeks f back. For SourceCodex, getCodexSessionTimestamp(path) opens an independent file handle from the path string — f is never read, so no seek is needed. The comment should say "For Claude, the file is seeked back to the start after parsing."

SourceKiroIDE / SourceKiro not handled in the switch

The switch covers SourceClaude, SourceCodex, and SourceCopilot, falling back to modTime for anything else. Looking at lister.go, Kiro IDE uses time.UnixMilli(c.startTime) from a JSON header when available. While this means Kiro IDE behaviour is unchanged from before this PR (both old and new code fall back to modTime for Kiro), it leaves a silent gap: if resolveFileCreatedAt is later used for Kiro IDE sessions, it will silently return a less accurate timestamp without any obvious indication that Kiro wasn't handled. A // TODO or explicit SourceKiroIDE case that returns modTime with a comment would make the intent clear.

getCodexSessionTimestamp re-opens the file for Codex

For SourceCodex, resolveFileCreatedAt calls getCodexSessionTimestamp(path), which opens path again internally. The already-open f *os.File is never used. This is correct (Codex metadata is read from path, not from the caller's reader), but it's a slight inefficiency — two file handles open simultaneously on the same path. This is minor and acceptable for now, but worth noting.

---

Test Coverage

Tests are well-written and follow the project's testing guidelines:

• Each source type gets a dedicated subtest
• File mtime is explicitly set to the far future (2099-12-31) to prove the regression doesn't re-appear
• Proper use of defer resolved.Reader.Close()

Suggestions for additional tests (non-blocking):

• A subtest verifying fallback to modTime when transcript parsing fails (e.g., empty Claude JSONL file). This would protect against regressions in the fallback path.
• A subtest for SourceKiroIDE confirming it returns modTime — even if the behaviour is intentional, it documents the decision.

---

Minor

• The fmt import is used in tests (fmt.Sprintf). Make sure this is present; the diff doesn't show import blocks so it's likely already there.
• The seek calls _, _ = f.Seek(0, io.SeekStart) in both the success and error branches for Claude are correct and symmetrical — good.

---

Summary

The fix is correct and the approach is sound. The main actionable item is updating the misleading comment ("For Claude and Codex" → "For Claude"). The Kiro IDE gap is worth a comment or explicit case so future contributors don't have to reconstruct this reasoning. Everything else is minor.

[claude]

Code Review: fix(T-994) — Preserve real session timestamps in file-backed resolver

Overview

Good, focused fix for a real bug: openFileSession was unconditionally using info.ModTime() as CreatedAt, which drifts from the actual session start time (e.g., after a sync, copy, or Git checkout). The new resolveFileCreatedAt method mirrors the lister's source-specific timestamp resolution and falls back to modTime on failure — which is the right pattern.

---

What works well

• Logic parity with the lister is correct. The three source paths (ParseFirstTimestamp for Claude, getCodexSessionTimestamp for Codex, parseCopilotWorkspace for Copilot) match what lister.go does, so the lister and resolver now return consistent timestamps for the same session.
• Defensive seek reset for Claude. Resetting the file position in both the success and error branches is correct — ParseFirstTimestamp advances the read position regardless of outcome.
• Kiro limitation is documented. The comment explaining why Kiro IDE falls through to modTime for now is helpful; it signals intent rather than an oversight.
• Regression tests are well-designed. Setting mtime to year 2099 is a reliable way to prove the code is using transcript content rather than file metadata.

---

Issues and suggestions

1. Tests ignore json.Marshal errors — use the existing mustMarshal helper

The new subtests use data, _ := json.Marshal(entry) (ignoring errors), while the test file already defines mustMarshal at line 738 for exactly this purpose. This pattern is inconsistent within the file.

// Current (new tests):
data, _ := json.Marshal(entry)

// Should be (consistent with mustMarshal helper already in file):
data := mustMarshal(t, entry)

This matters because a silently malformed fixture won't produce the right transcript content, causing the test to pass for the wrong reason (falling back to modTime).

2. Missing test: fallback-to-modTime path

There's no test verifying that when source-specific resolution fails (e.g., a Claude file with no parseable timestamp), CreatedAt falls back to modTime. Without this, a future regression that skips the fallback entirely would go undetected.

t.Run("claude_fallback_to_modtime", func(t *testing.T) {
    // Write a JSONL file with no timestamp field
    // Set mtime to a known time
    // Assert resolved.Metadata.CreatedAt == mtime
})

3. Minor: seek error is silently discarded

_, _ = f.Seek(0, io.SeekStart)

Discarding seek errors is a common and usually acceptable pattern for in-process files, but if the seek fails, the subsequent transcript read will return garbled data. A log.Printf or returning modTime on seek failure would make failures more observable. This is low-priority given how rarely seeks fail on regular files.

---

Summary

The core fix is correct and well-structured. The main actionable items are:

1. Switch the new tests from data, _ := json.Marshal(...) to mustMarshal(t, ...) for consistency and safety.
2. Add a fallback-to-modTime subtest to complete coverage of resolveFileCreatedAt.

Everything else is clean and ready to merge after those two items are addressed.

[claude]

Code Review: PR #154 — Preserve real session timestamps for file-backed transcripts

Overview

This PR fixes T-994: openFileSession was unconditionally setting CreatedAt to the file's modification time (info.ModTime()), causing the resolver to return inaccurate session timestamps compared to what the lister produced for the same sessions. The fix adds a resolveFileCreatedAt helper that mirrors the source-specific timestamp logic already present in the lister.

---

What's Good

• Clean symmetry with the lister. Using the same source-specific parsing paths (transcript first-entry for Claude, session_meta for Codex, workspace.yaml for Copilot) ensures the resolver and lister agree on CreatedAt, eliminating the mismatch that caused T-994.
• Safe fallback. resolveFileCreatedAt falls back to modTime on any parsing error rather than propagating failures, keeping the overall behaviour non-breaking.
• File position handled correctly. The Claude branch seeks back to offset 0 in both the success and error paths, so the caller's subsequent reads on f are unaffected.
• Regression tests. All three source-specific paths are covered with tests that explicitly prove the transcript timestamp takes precedence over a future-dated mtime.

---

Suggestions / Minor Concerns

1. Kiro IDE timestamp not extracted (known limitation, but worth tracking)

The comment says:

// Kiro IDE embeds startTime in JSON but requires full parsing; fall
// through to modTime for now.

This is fine as a pragmatic choice, but it means Kiro IDE sessions resolved by file path will still show incorrect timestamps. Consider opening a follow-up ticket so it doesn't get forgotten.

2. Seek errors silently dropped

_, _ = f.Seek(0, io.SeekStart)

For a regular file this is safe and the pattern is common in this codebase, so it's not a blocking concern. A log line at debug level would help diagnose edge cases on networked or virtual filesystems, but this is a very minor nit.

3. No test for the modTime fallback

The three subtests prove the happy paths. There's no test verifying that when transcript parsing fails (e.g. empty or corrupt file), CreatedAt falls back to modTime. Given the fallback is the safety net, a small negative-case test would add confidence:

t.Run("claude_fallback_to_modtime", func(t *testing.T) {
    // write an empty or non-JSONL file, set a known mtime
    // resolve and assert CreatedAt == known mtime
})

4. Copilot test drives through events.jsonl rather than directly calling openFileSession

The test goes through the full Resolve path (good for integration confidence), but it also means the test is sensitive to Copilot session-discovery logic. If discovery changes, the timestamp test could break for unrelated reasons. This is a very minor observation — the test is still useful.

---

Summary

This is a well-scoped, low-risk bug fix with good test coverage of the primary code paths. The implementation is consistent with existing patterns and the fallback behaviour is correct. The only meaningful gap is the Kiro IDE path, which is explicitly acknowledged. Happy to approve once a follow-up ticket is filed for the Kiro IDE case.

Review generated by Claude (claude-sonnet-4-6)