fix(jcode): prefer journal token_usage over stale snapshot + parse tz-less timestamps - #754
Conversation
…-less timestamps parse_jcode_file emitted snapshot messages before journal messages, and the downstream dedup (should_keep_deduped_message) keeps the FIRST occurrence per dedup_key. So when the append-only journal carried an UPDATED token_usage for a message_id already present in the snapshot (the snapshot is only rewritten at the next checkpoint), the corrected value was silently discarded and stale counts were reported. Now the parser merges in place: a journal entry repeating a snapshotted dedup_key overwrites that entry (journal wins) while preserving the snapshot's turn-start flag, so each message_id still collapses to exactly one entry with no double-counting. Also: naive ISO-8601 datetimes without a Z/offset (e.g. "2026-06-16T12:00:00", space-separated, or with fractional seconds) failed parse_from_rfc3339 and collapsed to the file mtime, scattering messages into the wrong day/month bucket. parse_timestamp_str now parses these as UTC. Nit: jcode_journal_path was duplicated verbatim in jcode.rs and message_cache.rs; extracted a single pub(crate) source of truth in sessions::jcode (the no-filename fallback now uses suffix-append semantics, matching the previous cache behavior). Tests: journal-update-wins (single entry, journal value survives downstream dedup), tz-less timestamp parsing (unit + end-to-end vs mtime fallback). Confidence: high Scope-risk: narrow Not-tested: leap-second / sub-millisecond fractional precision beyond 3 digits
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df72d343ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::iter::once((".journal.jsonl".to_string(), jcode_journal_path(path))); | ||
| let related_paths = std::iter::once(( | ||
| ".journal.jsonl".to_string(), | ||
| crate::sessions::jcode::jcode_journal_path(path), |
There was a problem hiding this comment.
This parser fix is hidden for users who already have a schema-22 source-message-cache.bin entry for the same Jcode snapshot/journal contents: load_or_parse_source_with_fingerprint returns cached.messages whenever the fingerprint matches, so the new journal-wins merge and timezone-less timestamp parsing never run until the files change or the cache is manually cleared. Since CACHE_SCHEMA_VERSION is still 22 in this commit, upgrading can continue reporting the stale snapshot token usage this change is meant to fix; please bump the cache schema (or otherwise invalidate Jcode entries) with the parser output change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6007. Bumped CACHE_SCHEMA_VERSION 22 -> 23 in message_cache.rs so existing schema-22 source-message-cache.bin entries are invalidated and reparsed. Without this, load_or_parse_source_with_fingerprint would keep returning the cached stale snapshot token_usage on a fingerprint match, hiding the journal-wins merge and timezone-less timestamp parsing changes. Valid catch.
| let mut index_by_dedup_key: std::collections::HashMap<String, usize> = parsed | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(idx, message)| message.dedup_key.clone().map(|key| (key, idx))) | ||
| .collect(); |
There was a problem hiding this comment.
When a snapshot already contains replayed duplicate message IDs (a supported case covered by crates/tokscale-core/tests/jcode.rs), this collect() keeps the last index for a duplicate dedup_key. A journal update for that ID then replaces the later snapshot row, but the downstream Jcode dedup is first-wins, so the stale first snapshot row survives and the corrected journal token_usage is discarded. Track the first index or collapse/replace all existing duplicates before appending journal messages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39d6007. The merge index now uses HashMap::entry(key).or_insert(idx) instead of collect(), so it keeps the FIRST index for a repeated dedup_key. That matches the downstream first-wins dedup: a journal update for a replayed snapshot id now overwrites the surviving (first) row rather than a later row that dedup discards, so the corrected journal token_usage is preserved. Added regression test test_jcode_journal_corrects_replayed_snapshot_duplicate (snapshot replays an id twice, journal corrects it to 999). Valid catch.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…schema Address automated review on #754: - jcode parser built the dedup-merge index with `collect()`, which keeps the LAST index for a repeated dedup_key. Downstream dedup is first-wins, so a journal update overwrote a row that was later discarded, preserving stale snapshot token_usage for replayed message ids. Build the index with `entry().or_insert()` so the journal correction targets the surviving (first) occurrence. - Bump CACHE_SCHEMA_VERSION 22 -> 23 so existing source-message caches reparse and pick up the journal-wins merge + timezone-less timestamp fix instead of returning cached stale messages. Add regression test covering a snapshot with a replayed duplicate id plus a journal correction for that id. Constraint: downstream jcode dedup is first-wins (filter on first occurrence) Confidence: high Scope-risk: narrow
…-less timestamps (junhoyeo#754) * fix(jcode): prefer journal token_usage over stale snapshot + parse tz-less timestamps parse_jcode_file emitted snapshot messages before journal messages, and the downstream dedup (should_keep_deduped_message) keeps the FIRST occurrence per dedup_key. So when the append-only journal carried an UPDATED token_usage for a message_id already present in the snapshot (the snapshot is only rewritten at the next checkpoint), the corrected value was silently discarded and stale counts were reported. Now the parser merges in place: a journal entry repeating a snapshotted dedup_key overwrites that entry (journal wins) while preserving the snapshot's turn-start flag, so each message_id still collapses to exactly one entry with no double-counting. Also: naive ISO-8601 datetimes without a Z/offset (e.g. "2026-06-16T12:00:00", space-separated, or with fractional seconds) failed parse_from_rfc3339 and collapsed to the file mtime, scattering messages into the wrong day/month bucket. parse_timestamp_str now parses these as UTC. Nit: jcode_journal_path was duplicated verbatim in jcode.rs and message_cache.rs; extracted a single pub(crate) source of truth in sessions::jcode (the no-filename fallback now uses suffix-append semantics, matching the previous cache behavior). Tests: journal-update-wins (single entry, journal value survives downstream dedup), tz-less timestamp parsing (unit + end-to-end vs mtime fallback). Confidence: high Scope-risk: narrow Not-tested: leap-second / sub-millisecond fractional precision beyond 3 digits * fix(jcode): target first-win duplicate on journal merge + bump cache schema Address automated review on junhoyeo#754: - jcode parser built the dedup-merge index with `collect()`, which keeps the LAST index for a repeated dedup_key. Downstream dedup is first-wins, so a journal update overwrote a row that was later discarded, preserving stale snapshot token_usage for replayed message ids. Build the index with `entry().or_insert()` so the journal correction targets the surviving (first) occurrence. - Bump CACHE_SCHEMA_VERSION 22 -> 23 so existing source-message caches reparse and pick up the journal-wins merge + timezone-less timestamp fix instead of returning cached stale messages. Add regression test covering a snapshot with a replayed duplicate id plus a journal correction for that id. Constraint: downstream jcode dedup is first-wins (filter on first occurrence) Confidence: high Scope-risk: narrow
1 participant
Problem
In
crates/tokscale-core/src/sessions/jcode.rs,parse_jcode_filereturns snapshot messages first, then journal messages. The downstream dedup (should_keep_deduped_message, lib.rs ~1102-1108) keeps the first occurrence perdedup_key(jcode:{session_id}:{message_id}).Three confirmed bugs (PR #718):
token_usagefor amessage_idalready present in the snapshot. Because the snapshot was emitted first, the corrected journal value was silently discarded and stale token counts were reported.Z/offset (e.g.2026-06-16T12:00:00) failedparse_from_rfc3339and collapsed to the file mtime, scattering messages into the wrong day/month bucket.jcode_journal_pathwas duplicated verbatim injcode.rsandmessage_cache.rs.Fix
parse_jcode_filenow merges in place: a journal entry repeating a snapshotteddedup_keyoverwrites that entry (journal wins) while preserving the snapshot's turn-start flag. Eachmessage_idstill collapses to exactly one entry — no double-counting, and the journal value survives downstream dedup.parse_timestamp_str(sessions/utils.rs) gains a naive-datetime fallback (T or space separator, optional fractional seconds) interpreted as UTC.pub(crate)jcode_journal_pathinsessions::jcode;message_cache.rsreuses it. The no-filename fallback uses suffix-append semantics (matching the previous cache behavior).Tests
journal_update_for_snapshotted_id_wins_and_collapses_to_one_entry— one entry, journal value wins.journal_update_replaces_value_after_downstream_dedup— journal value survives the lib.rs first-wins dedup contract.parses_timezone_less_timestamps_instead_of_falling_back_to_mtime— end-to-end vs a forced-distinct mtime.parse_timestamp_str_accepts_timezone_less_datetimes_as_utc— unit coverage (T/space/fractional/offset variants).All
cargo test -p tokscale-corepass;cargo clippy -p tokscale-core --testsis clean.Residual concerns
The cache schema version (already bumped to 22 in
message_cache.rs, note #19 references #718) ensures stale jcode caches reparse. Fractional precision beyond 3 digits and leap seconds are untested.🤖 Generated with Claude Code
Summary by cubic
Fixes stale token counts and timestamp parsing in Jcode sessions by merging journal updates into the parsed snapshot and treating timezone-less timestamps as UTC. Also targets the first-win duplicate during merge and bumps the source-message cache schema to 23 to drop stale caches.
Bug Fixes
dedup_key(journal wins), preserve the snapshot’s turn-start flag, and keep one entry permessage_id. The merge index now targets the first occurrence so the journal correction survives downstream first-wins dedup, even if the snapshot replayed the id.parse_timestamp_straccepts ISO datetimes without aZ/offset (T or space, optional fractional) and interprets them as UTC to avoid mtime fallback and mis-bucketing.Refactors
jcode_journal_pathinto a singlepub(crate)helper insessions::jcode;message_cache.rsreuses it with suffix-append fallback semantics. Also bumpedCACHE_SCHEMA_VERSIONto 23 to force reparse.Written for commit 39d6007. Summary will update on new commits.