fix(omp): parse title slot session transcripts - #120
Conversation
Walkthrough本次变更为 OmpAdapter 的扫描结果附加解析器版本标记，并在 Pi/OMP JSONL 会话解析中引入更细粒度的首行识别逻辑：新增行类型判定、title 槽位严格校验、advisor 标签归一化，以及跳过合法 title 首行的解析主循环调整，附带三条新测试。 ChangesOMP 标题槽位与解析器版本
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Reader as parse_pi_format_file
participant Parser as parse_pi_header_line
participant Slot as OmpTitleSlot 校验
Reader->>Parser: 解析首行 JSONL
Parser->>Slot: 校验 entry_type/v/updated_at
Slot-->>Parser: 校验结果
alt 有效 title 且客户端为 omp
Parser-->>Reader: TitleSlot
Reader->>Reader: 设置标记并跳过该行
else 有效 session
Parser-->>Reader: Session
else 无效
Parser-->>Reader: Invalid
Reader-->>Reader: 返回空 Vec
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/juya review |
![juya-review-bot[bot]](https://avatars.githubusercontent.com/in/4074416?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/tokscale-core/src/sessions/pi.rs (2)
265-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value三个 buffer 重填辅助函数存在重复样板代码。
parse_pi_line_kind、parse_pi_session_header_line、parse_omp_title_slot_line都重复了buffer.clear(); buffer.extend_from_slice(trimmed.as_bytes());两行逻辑，可以抽取一个小helper（例如fn refill_buffer(buffer: &mut Vec<u8>, trimmed: &str)）以减少重复。🤖 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 `@crates/tokscale-core/src/sessions/pi.rs` around lines 265 - 286, The three parsing helpers duplicate the same buffer refill steps, so extract the shared `buffer.clear()` plus `buffer.extend_from_slice(trimmed.as_bytes())` logic into a small helper and call it from `parse_pi_line_kind`, `parse_pi_session_header_line`, and `parse_omp_title_slot_line` to reduce repetition while keeping their existing parse behavior unchanged.
546-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议补充"重复 title 槽位"场景的测试。
现有测试覆盖了合法 title、非法 title、advisor 文件命名三种场景，但没有覆盖文件中出现两个连续
"type":"title"行的情况（根据saw_omp_title_slot逻辑，第二个 title 行会因allow_omp_title_slot变为false而被判定为Invalid，导致整份文件被丢弃）。这是 Line 369 状态机新增的边界条件，建议补充一条回归测试。🤖 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 `@crates/tokscale-core/src/sessions/pi.rs` around lines 546 - 591, Add a regression test for the duplicate OMP title-slot edge case in parse_omp_file/saw_omp_title_slot handling: create an input with two consecutive "type":"title" lines followed by a valid session/message, and assert the file is rejected (messages is empty) because allow_omp_title_slot becomes false on the second title. Place the test alongside test_parse_omp_jsonl_skips_title_slot, test_parse_omp_rejects_invalid_title_slot, and test_parse_omp_advisor_transcript_sets_agent_label so the new state-machine behavior is covered.crates/tokscale-core/src/adapters/omp.rs (1)
12-20: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win建议补充对新 ParserVersion 标记的测试覆盖。
本次变更的核心目的是让旧缓存因解析器版本号变化而失效，但现有测试（
omp_adapter_discovers_default_and_extra_jsonl）只校验了fingerprint_policy，没有断言unit.parser_version确实等于ParserVersion::new(ParserId::Omp, OMP_TITLE_SLOT_REVISION)。由于该机制正是本 PR 修复 bug 的关键路径（避免旧缓存掩盖新解析行为），建议补充一条直接断言，防止未来重构时该行为被静默破坏。Also applies to: 32-36
🤖 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 `@crates/tokscale-core/src/adapters/omp.rs` around lines 12 - 20, The OmpAdapter test coverage is missing a direct assertion that the parser version changes with the new revision, so the current test only checks fingerprint_policy and could miss regressions. Update the OmpAdapter test around omp_adapter_discovers_default_and_extra_jsonl to also assert unit.parser_version equals ParserVersion::new(ParserId::Omp, OMP_TITLE_SLOT_REVISION), using the existing OmpAdapter and OMP_TITLE_SLOT_REVISION symbols to locate the behavior. This should verify the cache-busting version path directly and protect against future refactors silently breaking it.
🤖 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 `@crates/tokscale-core/src/sessions/pi.rs`:
- Around line 27-45: Update the Pi session parser in Pi-related deserialization
so that a bad title-slot record does not invalidate the entire file;
specifically, adjust the `OmpTitleSlot`/initial-line parsing path in `pi.rs` so
malformed title metadata is skipped or treated as non-fatal, allowing the parser
to continue scanning for the next valid `session` or `message` entry instead of
returning `Invalid` for the whole session file.
---
Nitpick comments:
In `@crates/tokscale-core/src/adapters/omp.rs`:
- Around line 12-20: The OmpAdapter test coverage is missing a direct assertion
that the parser version changes with the new revision, so the current test only
checks fingerprint_policy and could miss regressions. Update the OmpAdapter test
around omp_adapter_discovers_default_and_extra_jsonl to also assert
unit.parser_version equals ParserVersion::new(ParserId::Omp,
OMP_TITLE_SLOT_REVISION), using the existing OmpAdapter and
OMP_TITLE_SLOT_REVISION symbols to locate the behavior. This should verify the
cache-busting version path directly and protect against future refactors
silently breaking it.
In `@crates/tokscale-core/src/sessions/pi.rs`:
- Around line 265-286: The three parsing helpers duplicate the same buffer
refill steps, so extract the shared `buffer.clear()` plus
`buffer.extend_from_slice(trimmed.as_bytes())` logic into a small helper and
call it from `parse_pi_line_kind`, `parse_pi_session_header_line`, and
`parse_omp_title_slot_line` to reduce repetition while keeping their existing
parse behavior unchanged.
- Around line 546-591: Add a regression test for the duplicate OMP title-slot
edge case in parse_omp_file/saw_omp_title_slot handling: create an input with
two consecutive "type":"title" lines followed by a valid session/message, and
assert the file is rejected (messages is empty) because allow_omp_title_slot
becomes false on the second title. Place the test alongside
test_parse_omp_jsonl_skips_title_slot,
test_parse_omp_rejects_invalid_title_slot, and
test_parse_omp_advisor_transcript_sets_agent_label so the new state-machine
behavior is covered.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9117957c-3ec4-405d-87e9-67e2ccf14ca8
📒 Files selected for processing (2)
crates/tokscale-core/src/adapters/omp.rscrates/tokscale-core/src/sessions/pi.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Reviewed. No blocking findings.
The change is narrowly scoped and matches the stated intent: OMP discovery now bumps only the OMP parser revision, title-slot handling is restricted to OMP before the session header, malformed or duplicate title slots are treated as hard parse rejection, and advisor transcript labeling stays under the OMP client. That strict rejection behavior is consistent with this fork's preference for clean parser semantics over silent recovery.
CI on the PR head is green for Core CI, Test & Coverage, Build Native, and CodSpeed.
|
Handled the latest review comment. No code change is needed for the I also re-checked local real data using the fixed 2026-07-04 window across the default OMP root and the configured extra scan root. Raw JSONL totals matched the installed |
1 participant
Summary
Root Cause
OMP 16.3.x writes a fixed title slot as the first JSONL record. Tokscale required the first non-empty record to be type=session, so affected OMP transcripts, including __advisor.jsonl, parsed as empty files even when later assistant rows contained usage tokens.
Validation
Summary by cubic
Fix OMP session parsing by skipping the new leading title slot and correctly labeling advisor transcripts. Also versions the OMP parser on all OMP units to avoid stale cache masking.
__advisor.jsonland__advisor.*.jsonl) as "OMP Advisor" while keeping them under the OMP client.Written for commit cdfcbc9. Summary will update on new commits.
Summary by CodeRabbit
新功能
__advisor类文件会统一识别为 OMP Advisor。Bug 修复