fix(desktop): search fixture-seeded transcripts for content hits - #3069
Conversation
Fixture windows write transcripts into the workspace store. Thread search listed those sessions from the Host catalog but read messages through openSession/loadTranscript, so a Host miss became a title-only hit with no turnId. Give search the same store read the fixture wrote. Fixes apache#2305 Generated-by: Grok
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummaryThis PR fixes desktop thread search for fixture-seeded transcripts. When The change extends the existing fixture workspace store. It does not create a parallel transcript source for production. The fixture reader is injected only in fixture mode. This is the smallest coherent solution because it preserves the production path and adds only the fallback required by the fixture environment. The new Validation
Review-relevant risksThe change affects desktop search behavior in fixture mode and adds the exported No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughFixture-backed thread search now reads seeded session messages from SQLite during E2E runs. Runtime search retains Host transcript fallback and cleanup behavior. Unit and Playwright tests cover result metadata, failures, and Chinese content queries. ChangesFixture-backed thread search
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change restores fixture-backed transcript content search while preserving production transcript behavior, and no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ThreadSearch
participant RuntimeHostSearchIPC
participant FixtureMessageReader
participant OperationalStateSQLite
participant HostTranscript
ThreadSearch->>RuntimeHostSearchIPC: Search thread
RuntimeHostSearchIPC->>FixtureMessageReader: Read fixture messages
FixtureMessageReader->>OperationalStateSQLite: Read session messages
OperationalStateSQLite-->>FixtureMessageReader: Return StoredMessage[]
FixtureMessageReader-->>RuntimeHostSearchIPC: Return fixture messages
RuntimeHostSearchIPC->>HostTranscript: Read transcript when no fixture reader
HostTranscript-->>RuntimeHostSearchIPC: Return transcript or null
RuntimeHostSearchIPC-->>ThreadSearch: Return search results
Possibly related issues
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 907abb43-7fc8-4802-a060-bc662b3907c0
📒 Files selected for processing (6)
apps/desktop/e2e/fixture-thread-search.spec.tsapps/desktop/src/main/__tests__/runtime-host-search-ipc-main.test.tsapps/desktop/src/main/e2e-fixture.tsapps/desktop/src/main/e2e-fixture/seed-helpers.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/main/runtime-host-search-ipc-main.ts
A bound fixture reader used to skip the Host transcript entirely. Treat a null store read as a miss so a later Host-written session in the same fixture window is still searchable. Generated-by: Grok
|
Thanks for digging into the fixture search gap — the direction is right and the test scaffolding is solid. The review surfaced two issues worth resolving before merge. Conclusion: needs work — two P1s. P1-1 — the two new tests pin behavior that
The PR's base ( P1-2 — the premise of the fix may not hold: the Host already serves fixture-seeded transcripts. Suggested verification: revert the 4 production files, keep only Optional nits (P3): tests 3/4 in the new suite exercise unchanged paths; AI-assisted review disclosure: this review was produced with AI assistance (pi review subagents on 中文摘要（AI 辅助审查）结论：FAIL，需要返工。两个 P1：① 两个新测试钉死了 main 已改掉的行为——单测断言 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The intended regression lock is useful, and the Host reader keeps correct session cleanup. Production still defaults to the Host path and the focused old-base checks are green.
Against current main, however, the new assertions are stale (desktopSessionKey projection and 任务标题), so the merged test result fails even though GitHub can merge the text cleanly. More importantly, the fixture and Runtime Host use the same runtime.sqlite; the Host transcript reader already reads that store, and a current-main 120-turn fixture is fully visible through loadTranscript(). From first principles, Runtime Host must remain the sole transcript authority. The smallest solution is to keep the E2E assertion, delete the new Desktop-main SQLite reader/wiring, and fix any actual Host readiness issue only if the E2E still fails.
Reviewed with Codex using two independent reviewer agents; I verified the latest head, current-main contracts, shared store/Host reader path, active-overlay behavior, existing discussion, and live CI.
中文
这条回归锁本身有价值，Host reader 的 session cleanup 也正确；生产默认仍走 Host，旧 base 上的聚焦检查全绿。
但相对当前 main，新增断言已经过期（desktopSessionKey 投影和 任务标题），所以即使 GitHub 文本上可干净合并，合并后的测试仍会失败。更重要的是，fixture 与 Runtime Host 使用同一个 runtime.sqlite，Host transcript reader 本就读取该 store；在 current main 上，120-turn fixture 可完整通过 loadTranscript() 读取。按第一性原理，Runtime Host 必须保持唯一 transcript authority。最小方案是保留 E2E 断言，删除新增的 Desktop-main SQLite reader/wiring；只有 E2E 仍失败时再修真实的 Host readiness 根因。
本次由 Codex 配合两个独立 reviewer agent 审查；我核验了最新 head、current-main 契约、共享 store/Host reader 路径、active overlay 行为、已有讨论和实时 CI。
|
/agentic_review |
Code Review by Qodo
1. Fixture errors bypass Host fallback
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks @1625567290 — re-reviewed at exact head 10d0597c441cb357eb59d53334b8f432850fe40f. The earlier round was against bbcf749c; both points raised there are addressed at this head. No findings; approving.
Previously raised, now resolved
- The E2E assertion no longer uses a bare
sessionIdor the old会话标题wording; it goes throughdesktopSessionKey({hostId, sessionId})and任务标题, matching the current identity contract. - The second fixture-reading branch is gone.
readMessagesnow has a single path —openSession()→loadTranscript()throughreadWithFallback, withclosein afinally— and a failed read returns null rather than falling through to a direct fixture read. Nothing bypasses the active-overlay path.
On the risk specific to a tests-only change
A PR that only touches test files can weaken coverage while appearing to strengthen it, so that was checked directly rather than assumed. This change is purely additive — no existing assertion was modified or removed — and the new assertions are strict rather than permissive: the E2E case uses toBe on the summary and toEqual on the full target object, and the unit cases use assert.deepEqual on the whole target plus a closed === 2 count that pins resource cleanup.
The other failure mode for a fixture-backed search test is that it asserts a string the fixture itself hard-codes, which would prove only that the seed round-trips. That is not the case here: the seeded turns are generated as real conversation text by scenarios-chat.ts, the asserted substring is part of that generated user message, and the search runs through the real IPC path into Host listSessions / openSession / loadTranscript. The seed builds no index and installs no bridge shortcut, so a hit can only be produced by the actual search chain.
The two files do not overlap: the E2E case drives a real fixture-seeded window, the unit case drives the IPC handler against a mock client, including the Host-failure-yields-no-hits path.
This review was AI-assisted. It is not a substitute for independent human review by a committer.
2 participants
Summary
Fixture windows seed transcripts into the workspace store. After the Runtime Host migration,
search:threadstill listed those sessions from the Host catalog (so a title query could hit) but read messages throughopenSession/loadTranscript. A Host miss is swallowed asnull, so content search returned no hits and noturnId— which is what blocked e2e from driving search → navigate-to-a-specific-turn against a seeded transcript (#2305).The original
sessions:readMessagesfixture bypass is gone. This restores the same contract on the current IPC: whenMAKA_E2E_FIXTUREis set, search reads the storewriteSessionwrote. Production still reads the Host transcript.long-transcriptis gone; the lock uses the existingchat-prompt-railseed (第 3 个问题→turn-prompt-rail-3).Fixes #2305
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/desktop run build:mainnode --test apps/desktop/dist/main/__tests__/runtime-host-search-ipc-main.test.js apps/desktop/dist/main/__tests__/thread-search.test.js— 15/15turnIdfixture-thread-search.spec.tsis the window-level lock; CI should run it)AI use
Select exactly one:
Tool(s) and scope:
Grok authored the implementation, tests, and this PR description.
Checklist
Does this PR entail a change in behavior?