Skip to content

fix(indexer): keep session indexing incremental on append - #176

Open
HengYangDS wants to merge 2 commits into
obra:mainfrom
HengYangDS:fix/index-session-high-water
Open

HengYangDS wants to merge 2 commits into
obra:mainfrom
HengYangDS:fix/index-session-high-water

Conversation

@HengYangDS

@HengYangDS HengYangDS commented Sep 26, 2026 •

Copy link
Copy Markdown

Problem

indexSession re-embeds and replaces every previously indexed exchange when a transcript grows. Its archive copy is also only created once, so a newly indexed exchange can point to a stale archive that does not contain the appended lines.

Change

  • Resume session indexing after the database's MAX(line_end) high-water mark, matching the existing incremental-indexing path.
  • Reuse the atomic archive copy helper and refresh when source size changes even if filesystem timestamps tie.
  • Leave summary generation and the existing append-only transcript assumption unchanged.

Verification

  • Added a regression that failed on the upstream base because last_indexed of an existing row was replaced. It now verifies the old row is preserved, the appended exchange is indexed, and the archive contains that exchange even when source and archive mtimes match.
  • Full local suite: 351 tests passed across 63 files. TypeScript compilation, esbuild bundling, bundle syntax checks, and a redacted secret scan of this commit passed.
  • Local tests used already-installed development dependencies and a pre-existing local model cache. A fresh local npm install did not finish within a 300-second bound; this is not a cold-install claim. The fork PR's Node 22/24 CI is the cold-install gate.

Scope and risks

The change applies to the session-specific indexing path; it does not invoke repair or re-summarize historical conversations. It relies on the existing append-only transcript contract and does not reconcile rewritten or truncated sessions. The source-level change is in src/indexer.ts and src/sync.ts; neither module is reachable from the MCP entrypoint. The unrelated generated MCP bundle drift has been removed, leaving that bundle byte-exact to the upstream base.

Related to the high-water fixes discussed in #84 and #152; both issues are already closed, so this PR does not claim to close them.

Additional local Node matrix (2026-09-26)

On macOS arm64, this current PR head passed npm run build, node --check dist/mcp-server.js, and the complete test suite on Node 22.23.3 and Node 24.21.0 (351/351 tests on each). Each run used a fresh source snapshot and the corresponding per-Node native dependency tree, with a pre-seeded local embedding-model cache and an isolated OMP_HOME. The dependency trees were installed from PR #179 (all five PRs have the same package.json); this is not a separate empty-cache install for each PR, an Ubuntu result, or an upstream CI pass.

Minimal-scope revision (2026-10-11)

Exact follow-up head: 28657ccee261cf173812597c2839c788094b1420; parent: b2d106963e41dcb56af129c34b64f80c8ddd2691.

  • Restored dist/mcp-server.js to the exact upstream 7e06519357777badd7a115d2014a7ef845904310 blob. All ten product-source sections in the submitted bundle already matched upstream, and the original esbuild input graph excludes the indexing/sync modules. This removes unrelated transitive-dependency behavior/layout changes rather than introducing a different MCP implementation.
  • Every other tracked file and executable mode is preserved (211 files). The resulting PR diff is six focused source/dist/test files; it is not a dependency upgrade.
  • Fresh native qualification on macOS arm64, existing workspace Node 24.19.0/npm 12.2.0: native npm ci, npm run build, original bundle syntax, 351/351 across 63 files, zero skips, including the three actual packaged MCP tests, pass. The build records its input graph, then retains the exact unchanged upstream bundle rather than presenting fresh resolver output as reproducible original bytes.
  • This isolated installation reuses the previously captured resolver lock (SHA-256 f4ac20b63ee3980db1711ee636f8b79b56fdd6664e5c6cd50809d85722af9398) and a byte-verified pre-existing embedding cache. It is not an official committed lock, a cold-model claim, or a hosted/Windows result. The App's newly bundled Node 24.21.0 refused the native library under its signing boundary; that failed receipt is retained and no signature or runtime policy was changed.
  • Native allowScripts, existing trusted-isolated-dev-tool risk limits, provider selection, summaries, user sessions and installed plugin remain unchanged. Temporary test state has been retired and exact absence verified.

Maintainer workflow approval/hosted CI, merge, stable-release inclusion and installed-consumer adoption remain separately unverified. No duplicate approval ping accompanies this factual revision.

@HengYangDS

Copy link
Copy Markdown
Author

Author follow-up for #176: the exact PR head is still mergeable, but its CI run remains action_required and has not executed. The consolidated current request on #180 covers this contribution as well: please approve the fork workflow and review its narrow source behavior for merge. I will address resulting failures and keep generated output, portability and generality qualified; no hosted pass or installation update is claimed.

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.

1 participant