Skip to content

Fall back to isogit store when git is absent - #576

Closed
TheGreatAxios wants to merge 1 commit into
mainfrom
fallback-context-store-when-git-missing
Closed

Fall back to isogit store when git is absent#576
TheGreatAxios wants to merge 1 commit into
mainfrom
fallback-context-store-when-git-missing

Conversation

@TheGreatAxios

Copy link
Copy Markdown
Collaborator

Summary

  • When native git is missing from PATH, createOptimizedContextStore returns the base isomorphic-git store instead of wrapping commit/readAt with Bun.spawn(["git", ...]).
  • Harbor and other minimal containers can complete reactor cycle commits without a git binary.

Verification

  • bun run typecheck passes
  • bun test src/session/optimized-context-store.test.ts — 22 pass (includes no-git fallback + native-git still wraps)

Harbor and minimal containers have no git binary; the optimized
wrapper shells out via Bun.spawn and fails the first cycle commit.
@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Review verdict: regression risk — needs a human decision before this can merge. Flagging rather than blocking outright, since the portability need behind it is real.

Detection is sound. Bun.which("git") === null is a PATH lookup, not a spawn, so it cannot misfire on a lock file, a permissions blip, or a slow spawn. Evaluated once per store creation, so a session is pinned to one mode and cannot flap mid-run. No false-positive concern.

The problem is that the fallback silently truncates history. The wrapper writes turns as rolling segments (turns.jsonl, turns-0001.jsonl, …) and its load() recovers the tail via readExtraSegmentTexts. The bare isogit base store only ever reads turns.jsonl — it has no knowledge of numbered segments.

The reviewer verified this directly: wrote 3 turns through createSegmentedJSONLWriter with a small segment size to force rollover, then opened the same directory with createIsogitStore(dir).load()it returned 1 of 3 turns, with no error, no warning, no signal at all.

The scenario that hits it is exactly the one this PR exists to serve: a session dir written under wrapper mode (git present), later resumed where git is absent — the same ~/.corbits/projects/<key>/<session-id>/ mounted into a container without a git binary. The reactor's load() then replays a truncated conversation. The code's own docstring says history "must never" be dropped because "history is the live conversation state," citing CL-5935. This is worse than CL-5935, which at least errored.

Second, independent gap: the base store's parseTurns hard-throws on any malformed line — no torn-tail tolerance, no null-byte stripping, no soft metadata recovery. All of that resilience is wrapper-only and was the CL-5935 fix. A crash mid-write under fallback has no recovery path; the same crash under the wrapper heals itself.

And it is silent. No log.warn when the fallback engages, though this file logs every other degraded path. An operator sees "the agent forgot earlier context" with nothing pointing at the cause.

Suggested reshape, from the reviewer: keep the segmented layout and the torn-tail/orphan resilience in both modes, and swap only the commit mechanism (native git spawn vs isomorphic-git) — push the git-presence check down into commit() rather than up into whole-store selection. That gets the portability without a second storage format.

Not stale: merges clean, typecheck and the 22 store tests pass on current main. Note the two added tests only read back through the same mode they wrote with, so no cross-mode test would have caught this.

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Closing rather than reshaping — the implementation is unsafe and the simplest fix is probably not code at all.

Why it cannot merge as written: the fallback swaps the entire storage implementation, not just the commit transport. The wrapper writes turns as rolling segments (turns.jsonl, turns-0001.jsonl, …); the base isogit store's load() reads only turns.jsonl and knows nothing of numbered segments. A session that rolled over, resumed without git, comes back with a partial history and no warning.

Worse than an under-read: the base store also rewrites turns.jsonl on commit. So a truncated resume, followed by more work, leaves a rewritten first file plus orphaned turns-000N.jsonl beside it — and the next resume with git reads both, producing duplicated, corrupted history on disk. Nothing should be able to do that to a session.

The requirement is real — Harbor and minimal containers, per CL-6923 — so it is filed separately rather than dropped.

Three ways to satisfy it, cheapest first:

  1. Install git in the container. A few MB in an image versus a second storage path in the product. If Harbor's base image can carry it, this needs no code and no review.
  2. Swap only the commit transport. Keep the segmented layout and the torn-tail/orphan resilience in every mode, and push the git-presence check down into commit() so it chooses native git vs isomorphic-git. One format, one read path, nothing to be incompatible with.
  3. Anything that keeps two storage formats — not this.

Note the two tests here only read back through the same mode they wrote with, so no cross-mode test would have caught the truncation. Whatever lands next needs a test that writes under one mode and reads under the other.

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