Skip to content

Add GitHub Copilot parser and fix incremental timestamps for resumed sessions - #41

Merged
barisozbas merged 5 commits into
uber:mainfrom
lx39214:add-copilot-parser-and-fix-incremental-timestamps
Sep 12, 2026
Merged

barisozbas merged 5 commits into
uber:mainfrom
lx39214:add-copilot-parser-and-fix-incremental-timestamps

Conversation

@zzzLi-56

@zzzLi-56 zzzLi-56 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Intent

  • Onboard GitHub Copilot CLI session-state logs into the ADR Sensor.
  • Keep resumed sessions incrementally exportable as one stable snapshot per session.

Changes

  • Add CopilotParser for ~/.copilot/session-state/<session-id>/events.jsonl, with optional workspace.yaml and vscode.metadata.json enrichment.
  • Honor COPILOT_HOME and document the equivalent macOS, Linux, and Windows session paths.
  • Clarify that the source reads Copilot CLI session state, not VS Code Copilot Chat extension storage.
  • Normalize chat messages, tool calls and results, permissions, skills, subagents, and session metadata into AgentEvent.
  • Register the copilot source in AgentObserver and expose it through adr-sensor --source copilot.
  • Apply the Sensor's 14-day lookback by default and honor --all-history using the events.jsonl modification time.
  • Use stable start-time filenames plus content-aware, atomic snapshot replacement for resumable Codex and Copilot sessions.
  • Preserve current main behavior for Codex discovery, lookback filtering, tool records, token usage, and SQLite catalogs.

Test Plan

  • uv run ruff check passes for every Python file changed by this PR.
  • uv run pytest tests/ -q: 231 passed on Python 3.11.
  • Manual follow-up: create and resume a Copilot CLI session, then validate adr-sensor --source copilot --no-save and adr-sensor --source copilot --save-sessions output.

Revert Plan

Revert this PR to remove Copilot ingestion and restore the previous per-session export behavior.

Jira Issues

None.

@CLAassistant

CLAassistant commented Aug 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@pengyuzhang pengyuzhang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Second-pass review at head 2e889da, focused on the three highest-impact findings; posted inline below. Summary of where they land:

  1. copilot_parser.py:134 — the new parser's timestamp precedence reintroduces the resumed-session staleness bug this PR fixes elsewhere, and the new test asserts the wrong precedence. Verified by execution.
  2. claude_parser.py:130 — the min→max change interacts with the filename-based exporter: snapshots accumulate unboundedly per active session, nothing prunes.
  3. codex_parser.py:98 — the same change causes a one-time fleet-wide mass re-export on deploy, plus a same-second truncation edge that permanently drops a session's tail messages.

The timestamp fixes themselves are correct in isolation, and the parser wiring (--source copilot, dispatch, schema) is sound — the issues are in the new Copilot parser's own timestamp handling and in unhandled interactions with the incremental exporter, not in the fix's intent.

Comment on lines +134 to +143
timestamp = (
self._normalize_optional_timestamp(workspace_meta.get("updated_at"))
or self._normalize_optional_timestamp(vscode_meta.get("modified"))
or self._normalize_optional_timestamp(session_data["last_event_at"])
or session_data["timestamp"]
or self._normalize_optional_timestamp(workspace_meta.get("created_at"))
or self._normalize_optional_timestamp(vscode_meta.get("created"))
or self._normalize_optional_timestamp(session_data["first_event_at"])
or datetime.now(timezone.utc)
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The timestamp fallback chain prefers stale sidecar metadata over the newest event — reintroducing for Copilot the exact resumed-session bug this PR fixes for Claude and Codex.

workspace.yaml:updated_at and vscode.metadata.json:modified sit ahead of last_event_at, so a session whose events.jsonl is newer than its side-channel metadata gets the older timestamp. That's precisely the resumed-session case: events append at T2 while the sidecar files still read T1 (vscode.metadata.json written once at creation, or workspace.yaml not rewritten on resume).

Verified by executing the parser with stale metadata (modified=2026-07-31, events through 2026-08-10): the emitted AgentEvent.timestamp was 2026-07-31. filter_entries_by_existing_files (observer.py:320) only re-exports when entry_ts > existing_ts, so all post-resume activity — including any malicious tool use ADR exists to catch — is silently never exported. Same defect, third parser.

Suggested change
timestamp = (
self._normalize_optional_timestamp(workspace_meta.get("updated_at"))
or self._normalize_optional_timestamp(vscode_meta.get("modified"))
or self._normalize_optional_timestamp(session_data["last_event_at"])
or session_data["timestamp"]
or self._normalize_optional_timestamp(workspace_meta.get("created_at"))
or self._normalize_optional_timestamp(vscode_meta.get("created"))
or self._normalize_optional_timestamp(session_data["first_event_at"])
or datetime.now(timezone.utc)
)
timestamp = (
self._normalize_optional_timestamp(session_data["last_event_at"])
or self._normalize_optional_timestamp(workspace_meta.get("updated_at"))
or self._normalize_optional_timestamp(vscode_meta.get("modified"))
or session_data["timestamp"]
or self._normalize_optional_timestamp(workspace_meta.get("created_at"))
or self._normalize_optional_timestamp(vscode_meta.get("created"))
or self._normalize_optional_timestamp(session_data["first_event_at"])
or datetime.now(timezone.utc)
)

Note the new test in test_parsers.py asserts the current (wrong) precedence — it expects the updated_at value — so it locks the bug in and needs its expected value updated along with this.

try:
ts = normalize_timestamp(obj["timestamp"])
if sessions[session_id]["timestamp"] is None or ts < sessions[session_id]["timestamp"]:
if sessions[session_id]["timestamp"] is None or ts > sessions[session_id]["timestamp"]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With latest-event timestamps in the export filename, every active session now accumulates one full-history snapshot per run — and nothing prunes the old ones. (Applies equally to the same change in codex_parser.py:98; the mechanism lives in observer.py:289.)

Before this change the embedded timestamp was the stable session start, so one session mapped to one file and each run overwrote it in place. Now the timestamp moves forward whenever the session has activity, save_sessions_to_individual_files opens the new path with 'w', and no code deletes prior snapshots. A session used daily leaves one full-chat-history file per day:

adr.sess-abc.20260810_174501.json   ← Monday's full history
adr.sess-abc.20260811_183212.json   ← Tuesday's (superset of Monday's)
adr.sess-abc.20260812_091544.json   ← ...

The output dir grows without bound, and any consumer watching it re-ingests the session's entire history each time — every earlier message duplicated downstream unless the consumer dedups by session_id.

Together with the upgrade-time re-export (comment on codex_parser.py:98), this suggests the moving timestamp shouldn't be part of the file's identity: either keep a stable filename per session and track the latest-exported-event time in the file contents or a sidecar index, or prune a session's older snapshots when writing the new one.


return AgentEvent(
timestamp=session_data["timestamp"] or datetime.now(timezone.utc),
timestamp=session_data["last_event_timestamp"] or session_data["timestamp"] or datetime.now(timezone.utc),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One-time mass re-export on upgrade: the first run after deploying this change re-exports essentially every session in the retention window, even with zero new activity. (Same for the claude_parser.py:130 min→max flip; the comparison is observer.py:320.)

The incremental filter compares the freshly parsed timestamp against the one embedded in the existing file's name, and skips only when entry_ts > existing_ts fails. On the first post-deploy run, every existing file still carries the old start-semantics timestamp while every freshly parsed entry carries its latest-event timestamp — and latest > start for any session longer than a second. So the check passes for all of them, and every host re-exports a duplicate full-history snapshot of every session at once: a fleet-wide burst of duplicates into the pipeline on deploy day.

Related edge in the same comparison: both sides are truncated to whole seconds (%Y%m%d_%H%M%S filenames; .replace(microsecond=0)) with a strict >, so a session whose final events land within the already-exported second is permanently skipped — for an ended session, those tail messages are never exported. A >= won't fix that (it would re-export forever); it needs sub-second precision or a content-based change check.

Worth handling the transition explicitly (e.g. migrate existing filenames once, or dedup by session_id downstream) rather than letting the burst happen.

@zzzLi-56
zzzLi-56 force-pushed the add-copilot-parser-and-fix-incremental-timestamps branch from 2e889da to 30d2982 Compare September 1, 2026 10:16
@zzzLi-56

zzzLi-56 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for calling out the timestamp and incremental-export edge cases. I have updated this PR accordingly.

First, I have intentionally removed the Claude changes from this PR. I do not currently have representative real Claude session data to validate resumed-session behavior, timestamp semantics, and migration safety to the same standard as Codex and Copilot. Claude therefore remains aligned with upstream for now.

For Codex and Copilot, I reworked the incremental path around the original ADR model of one stable snapshot per session:

  • The exported filename now keeps the session start time as its stable identity instead of moving with the last activity time.
  • Codex/Copilot snapshots use complete exported-content comparison, excluding only rewrite identity fields (timestamp and uuid). This catches same-second tool-result updates that a filename timestamp comparison can miss.
  • Copilot now derives the snapshot identity from session.start.startTime (with creation/event fallbacks), rather than potentially stale workspace.yaml.updated_at or VS Code metadata.
  • Copilot preserves error-only failed tool executions, rather than dropping failures that have no result.
  • Snapshot replacement is atomic: write a same-directory temporary file, flush/fsync it, then os.replace() it. On POSIX, the output directory is also synced before stale snapshots are removed.
  • Legacy moving-timestamp snapshots are removed only after a successful replacement, only for Codex/Copilot, and only after re-reading the candidate and confirming its stored session_id exactly matches. Unreadable, malformed, colliding, or deletion-blocked files are preserved.
  • Lossy filename sanitization is collision-safe, including the edge case where a generated hash-suffixed token collides with another literal session ID.
  • Per-session advisory locks and monotonic revision checks prevent an older parser result from overwriting a newer snapshot.
  • Same-last_event_at concurrency boundary. Both append-only parsers now provide event_count as a secondary revision value, so an older prefix parse cannot overwrite a newer snapshot even when timestamps are equal or unavailable.

For Codex/Copilot only:

  1. it compares the complete export
  2. it atomically replaces the stable snapshot

The external behavior remains the same intended ADR shape: one JSON snapshot per session, with a stable filename. The additional safeguards are necessary because Codex/Copilot sessions can resume and append to the same underlying log for long periods.

Validation included synthetic migration, collision, deletion, same-second tool-result, write-failure, permission, lock, and stale-writer tests.

I also tested copied real local sessions:

  • Codex: 49.8 MB, 8,181 events
  • Copilot: 542.9 MB, 127,452 events

Considering the performance impact of verifying the full content, I tested the processing duration. Earlier performance measurements on comparable maximum samples were approximately:

  • Codex, 47.53 MiB: parse ~0.57 s; atomic save ~0.07 s
  • Copilot, 517.74 MiB: parse ~7.12 s; atomic save ~4.19 s; unchanged-content filtering ~2.08 s

Thank you again for identifying the original timestamp, filename, and same-second update concerns. They led to a more conservative implementation while keeping the original stable per-session snapshot design.

@barisozbas barisozbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing the earlier timestamp and incremental-export concerns. The stable snapshot and atomic replacement approach looks much safer.

Before merging, please address the following:

  1. CopilotParser currently scans every session directory and does not accept or enforce max_age_days. This means the default CLI path includes all Copilot history, despite --all-history being the opt-in for data older than 14 days. Given the reported 542.9 MB Copilot dataset, this can add substantial default processing cost. Please add the same age-limit behavior used by the other parsers and propagate AgentObserver.max_age_days to CopilotParser, with tests for both the default cutoff and --all-history.
  2. Please rebase onto the latest main and resolve the current Sensor conflicts carefully, preserving the newer Codex discovery, age filtering, tool-record, token-usage, and SQLite changes from main while retaining this PR's revision metadata.
  3. Please fix the Ruff import-order failure in adr_sensor/parsers/__init__.py and run the full Sensor test and lint suite on the rebased result.

Once these are addressed and CI is green, this should be ready for another pass.

@barisozbas barisozbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Made required changes. LGTM

Thanks for expanding the observability coverage with Copilot @zzzLi-56 !

@barisozbas
barisozbas merged commit 9b09ead into uber:main Sep 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants