feat(core): add first-class OMP session indexing - #82
Conversation
c01f800 to
12b4697
Compare
tommy0103
left a comment
There was a problem hiding this comment.
Reviewed the current head (12b4697). The OMP provider itself looks good — the main feedback is about scope, plus one query-layer bug.
Suggest splitting into two PRs
This PR bundles two separable changes:
- Commits 1–3: the OMP provider (the actual #81 work) — clean, well-tested, ready to merge on its own.
- Commit 4 (
fix(query)): provider-agnostic query enhancements (projectPath,excludeInvoking,contextLimit, CLI--compact/--limit/--context/--snippet-length). Useful, but does not depend on OMP.
All open issues below live in commit 4, so the OMP provider is currently blocked by unrelated work. Commit 4 touches almost disjoint files; the split should be a trivial cherry-pick, and the two PRs can land in either order.
Blocking (query commit): excludeInvoking drops unattributed memories
buildWhere generates mem.session_id != ?. remember() stores session_id as NULL when omitted, and NULL != x evaluates to NULL in SQL, so memories({ excludeInvoking: true }) silently filters out every memory without a source session — verified against node:sqlite. Suggest (mem.session_id IS NULL OR mem.session_id != ?) scoped to memories (the other tables' session_id is non-null in practice). The new tests don't catch this because every fixture memory carries a session_id.
Conflict with main
main has moved to #120; skill-doc/SKILL.md now conflicts on the --search example line (main changed the nonce example; this PR adds the compact flags to the same line). Needs a rebase.
Docs / language consistency
- Commit 4's message is in Chinese; the repo convention is English conventional commits.
- New Chinese prose in
SKILL.md, theapi-reference.mdtables, andpackages/cli/README.md("紧凑检索" section) inside otherwise-English docs. Please keep docs English (or confirm mixed language is intentional). - Commit 4's message mentions fixing the read-only SQL keyword scan, but that landed on main via #110 — this commit doesn't touch that code.
Minor
PiSessionUnitMeta.kindwas widened tostring; a template-literal type would keep some checking.- Low-level error messages still say "Pi" for OMP files (
Malformed Pi session header in ...). parseLinesassumes the OMP title record is always at line 0; a mid-file title record would fail entry validation. Worth a comment documenting the invariant (or skippingtype === 'title'in the entry loop).
Verified locally
Built the PR CLI and ran the related suites: 107 pass / 0 fail. On a real ~1 GB index, bare --search returned 68.8 KB / 140 messages; with --compact --limit 8 --context 0 --snippet-length 240 it returned 5.1 KB / 8 hits. The flags work as advertised.
One observation from real usage data (a follow-up thought, not a request for this PR): in indexed agent history, --query scripts outnumber --search roughly 10:1, and agents already truncate output with head -c, producing broken JSON. If bounding output matters, defaults (or a total-size guard) may deserve a separate discussion rather than opt-in flags only.
12b4697 to
96202ef
Compare
|
Split the provider-agnostic query work into #127. This PR is now rebased onto current |
What and why
Closes #81.
OMP (Oh My Pi) stores Pi-compatible JSONL v3 sessions under
~/.omp/agent/sessions, independently from Pi's~/.pi/agent/sessions. Reusing the Pi source identity would lose provenance and prevent both histories from being indexed together. OMP also writes a mutable title record before the canonical session header.This change adds a dedicated
ompprovider and parameterizes the existing Pi projection internally. OMP therefore reuses the established tree, branch, compaction, tool, usage, visibility, cursor, and raw-evidence semantics without copying the parser. The OMP-only configuration consumes the title prelude as metadata while preserving the composite session identity. Pi behavior and storage identity remain unchanged.The provider is registered in the built-in catalog, watcher roots, Core package exports, README, skill, and schema/API references.
Verification
npm test— 468 pass / 0 failnpm run typecheck— 0 errors (root + app)npm run lint— 0 errors / 4 pre-existing warningsnpm run test:electron:all— not required; noapp/source changed.github/workflows/) — the current workflows only run the CLI subset; the repository-wide suite was run locallyupstream/mainAdditional evidence:
discover()is exercised against OMP's project-directory layout.~/.omp/agent/sessionscorpus discovered 85 sessions and verified OMP provenance, title metadata, tool calls, and raw evidence.Deliberately out of scope
Provider adapter
discover()against the supported directory layoutindexVersionMarkervisibility