feat: bbolt-backed command store (daemon-owned, feature-gated)#284
Conversation
Introduce a CommandStore interface that abstracts local command buffering, with two implementations: - fileStore: wraps the existing append-only txt files (pre/post/cursor), the always-available fallback. Behavior unchanged. - boltStore: a bbolt-backed store intended to be owned by the single long-lived daemon (bbolt holds an exclusive file lock). Pre commands go in the "active" bucket, post in "archived", and the sync cursor in "meta", matching the pre-existing activeBucket/archivedBucket constants. Keys are time-ordered (8-byte big-endian UnixNano + sequence) so iteration is chronological and identical timestamps never collide. Add a feature-gated config field (storage.engine: file|bolt, default file) selected via NewCommandStore. Nothing is wired into the track hot path or daemon yet; this is an additive, behavior-neutral first step.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CommandStore interface to abstract command persistence, adding a new bbolt-based storage engine (boltStore) alongside the existing text file implementation (fileStore). Feedback on the new boltStore implementation focuses on safety and transaction efficiency. Specifically, the reviewer recommends refactoring the Prune method to load post commands within the write transaction to prevent race conditions, adding defensive nil checks on bbolt buckets to avoid nil pointer dereferences, and implementing a length check in decodeKeyNano to prevent potential out-of-bounds panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| postCommands, err := s.GetPostCommands(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cursorNano := cursor.UnixNano() | ||
|
|
||
| return s.db.Update(func(tx *bolt.Tx) error { | ||
| archived := tx.Bucket([]byte(archivedBucket)) | ||
| var delArchived [][]byte | ||
| if err := archived.ForEach(func(k, v []byte) error { | ||
| if decodeKeyNano(k) <= cursorNano { | ||
| delArchived = append(delArchived, append([]byte(nil), k...)) | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| for _, k := range delArchived { | ||
| if err := archived.Delete(k); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| active := tx.Bucket([]byte(activeBucket)) |
There was a problem hiding this comment.
Refactor Prune to load postCommands inside the write transaction. This provides several benefits:
- Consistency: Eliminates potential race conditions where post commands are modified between the read and write transactions.
- Efficiency: Reduces the number of database transactions from 2 to 1, and allows populating
postCommandsand identifying keys to delete in a single pass over the archived bucket. - Safety: Adds defensive nil checks for both
archivedandactivebuckets to prevent nil pointer dereference panics.
cursorNano := cursor.UnixNano()
return s.db.Update(func(tx *bolt.Tx) error {
archived := tx.Bucket([]byte(archivedBucket))
if archived == nil {
return fmt.Errorf("bucket %s not found", archivedBucket)
}
var postCommands []*Command
var delArchived [][]byte
if err := archived.ForEach(func(k, v []byte) error {
cmd := new(Command)
if err := json.Unmarshal(v, cmd); err == nil {
cmd.RecordingTime = time.Unix(0, decodeKeyNano(k))
postCommands = append(postCommands, cmd)
}
if decodeKeyNano(k) <= cursorNano {
delArchived = append(delArchived, append([]byte(nil), k...))
}
return nil
}); err != nil {
return err
}
for _, k := range delArchived {
if err := archived.Delete(k); err != nil {
return err
}
}
active := tx.Bucket([]byte(activeBucket))
if active == nil {
return fmt.Errorf("bucket %s not found", activeBucket)
}| func decodeKeyNano(key []byte) int64 { | ||
| return int64(binary.BigEndian.Uint64(key[0:8])) | ||
| } |
There was a problem hiding this comment.
To prevent potential out-of-bounds panics if a key is corrupted or unexpectedly short, add a length check before accessing the slice in decodeKeyNano.
func decodeKeyNano(key []byte) int64 {
if len(key) < 8 {
return 0
}
return int64(binary.BigEndian.Uint64(key[0:8]))
}| b := tx.Bucket([]byte(bucket)) | ||
| seq, err := b.NextSequence() |
There was a problem hiding this comment.
In bbolt, tx.Bucket returns nil if the bucket does not exist. Although buckets are initialized during store creation, performing a nil check is a best practice to prevent nil pointer dereference panics if the database is modified externally or corrupted.
| b := tx.Bucket([]byte(bucket)) | |
| seq, err := b.NextSequence() | |
| b := tx.Bucket([]byte(bucket)) | |
| if b == nil { | |
| return fmt.Errorf("bucket %s not found", bucket) | |
| } | |
| seq, err := b.NextSequence() |
| b := tx.Bucket([]byte(bucket)) | ||
| return b.ForEach(func(k, v []byte) error { |
There was a problem hiding this comment.
Add a nil check for the bucket returned by tx.Bucket to prevent potential nil pointer dereference panics.
b := tx.Bucket([]byte(bucket))
if b == nil {
return fmt.Errorf("bucket %s not found", bucket)
}
return b.ForEach(func(k, v []byte) error {| v := tx.Bucket([]byte(metaBucket)).Get([]byte(cursorKey)) | ||
| if v == nil { | ||
| noCursorExist = true | ||
| cursorTime = time.Time{} | ||
| return nil | ||
| } | ||
| cursorTime = time.Unix(0, int64(binary.BigEndian.Uint64(v))) |
There was a problem hiding this comment.
Add a nil check for the metaBucket to prevent potential nil pointer dereference panics when calling .Get.
b := tx.Bucket([]byte(metaBucket))
if b == nil {
return fmt.Errorf("bucket %s not found", metaBucket)
}
v := b.Get([]byte(cursorKey))
if v == nil {
noCursorExist = true
cursorTime = time.Time{}
return nil
}
cursorTime = time.Unix(0, int64(binary.BigEndian.Uint64(v)))| buf := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(buf, uint64(cursor.UnixNano())) | ||
| return s.db.Update(func(tx *bolt.Tx) error { | ||
| return tx.Bucket([]byte(metaBucket)).Put([]byte(cursorKey), buf) |
There was a problem hiding this comment.
Add a nil check for the metaBucket to prevent potential nil pointer dereference panics when calling .Put.
b := tx.Bucket([]byte(metaBucket))
if b == nil {
return fmt.Errorf("bucket %s not found", metaBucket)
}
return b.Put([]byte(cursorKey), buf)![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de7b8c5fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nil | ||
| } | ||
| cmd.RecordingTime = time.Unix(0, nano) | ||
| closest := cmd.FindClosestCommand(postCommands, true) |
There was a problem hiding this comment.
When pruning a normal completed command where the post hook runs after the pre hook, this calls FindClosestCommand on the pre command. That helper only accepts candidates with receiver.Time - candidate.Time >= 0, so a later post command is never considered a match and the active/pre row survives every prune; in bolt mode this leaves synced pre commands accumulating indefinitely and keeps stale rows in future pre-tree lookups. The same directionality issue is copied into fileStore.Prune, so the abstraction should match posts from the post side or otherwise compare times explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed and fixed in 771e371. You're right that pre.FindClosestCommand(posts) can't match a later post (it only accepts candidates at/before the receiver's time), so synced pre rows never got pruned. Prune now matches from the post side via a shared preHasSyncedPost helper (same unique key + post.Time >= pre.Time + post synced at/before the cursor), applied to both the bolt and file stores, with a regression test for the post.Time > pre.Time case.
Generated by Claude Code
Wire the daemon to own the bbolt CommandStore and accept raw command events over the socket when the bolt storage engine is enabled. - Add track_pre / track_post socket message types and TrackEventPayload; route them through the existing pub/sub topic to new handlers. - handlePubSubTrackPre persists to the active bucket; handlePubSubTrackPost persists to archived, then runs the flush/sync/cursor/prune cycle against the store (the daemon-side equivalent of trySyncLocalToServer). On send failure it leaves data in bolt and does not advance the cursor. - Extract the server-send path (terminal resolution, daemon source, encryption, circuit breaker) from handlePubSubSync into a reusable sendTrackArgsToServer, shared by the sync and track handlers. - Extract the pre/post -> []TrackingData assembly into model.BuildTrackingData so both the CLI fallback and the daemon build payloads from one path. - The daemon constructs and owns the bolt store for its lifetime in cmd/daemon/main.go (gated on storage.engine == bolt). - Make CommandStore.GetPreTree return map[string][]*Command so the interface is implementable/fakeable outside the model package. Adds self-contained track handler tests (fake store + config, no mockery). Nothing in the CLI hot path emits these events yet; that follows next.
When storage.engine == bolt and the daemon socket is ready, the track hook sends the raw pre/post command event to the daemon (fire-and-forget) instead of writing the txt files locally; the daemon owns persistence, sync, and pruning. When bolt is disabled or the daemon is down, the CLI falls back to the existing txt file store and local sync, unchanged. Also dedupe the sync assembly: trySyncLocalToServer now builds its payload via model.BuildTrackingData against an explicit file store (NewFileStore) and advances the cursor through the store interface, removing the duplicated inline assembly and updateCursorToFile. Add daemon.SendTrackEvent and model.NewFileStore.
In bolt mode the daemon owns the exclusively-locked DB, so the CLI cannot read it directly. Add a list_commands request/response socket message: `ls` queries the daemon when bolt is enabled and the socket is ready, and falls back to reading the txt file store otherwise. Extract the pre/post pairing for display into model.BuildListedCommands (returning model.ListedCommand) so the daemon and the CLI fallback render identical output, and replace ls.go's inline anonymous struct with it.
In bolt mode the daemon prunes synced commands from the DB after each sync, and post.txt typically does not exist, so cleanCommandFiles would error when opening it. Skip the txt compaction when storage.engine == bolt; log cleanup still runs.
AnnatarHe
changed the title
|
@claude review it |
|
Claude finished @AnnatarHe's task in 3m 49s —— View job Code Review: feat: bbolt-backed command store
The architecture is sound — delegating the exclusive-lock DB to the daemon, keeping txt as fallback, and routing via the existing socket. The implementation is clean. Several correctness and safety issues need attention before merge. Critical1. Nil pointer panics in
All four need a nil check with a descriptive 2. func decodeKeyNano(key []byte) int64 {
return int64(binary.BigEndian.Uint64(key[0:8])) // panics on corrupted key < 8 bytes
}Add 3.
postCommands, err := s.GetPostCommands(ctx) // View tx #1 — can be stale
// ...
return s.db.Update(func(tx *bolt.Tx) error { // Write tx #2
// uses stale postCommands for active-bucket matchingNew post commands can be inserted between the two transactions by a concurrent goroutine. Gemini's suggestion of collapsing both into a single 4. Circuit breaker bypassed on bolt track-post path —
if syncCircuitBreaker != nil && syncCircuitBreaker.IsOpen() { /* save for retry */ }
High5. Silent empty } else {
commands, err = model.BuildListedCommands(ctx, model.NewFileStore())If 6. No write deadline on
7. Config re-read on every post command — cfg, err := stConfig.ReadConfigFile(ctx)This reads and parses TOML from disk on every Medium8. Lost in-flight events on daemon shutdown — daemon.InitCommandStore(store)
defer store.Close()On SIGTERM, 9.
10. Two separate instance := &model.Command{ Time: time.Now() } // L93
// ...
now := time.Now() // L110 — different timestamp
return daemon.SendTrackEvent(ctx, ..., *instance, now)
Low11. Missing test for happy path of The test covers: nil store, pre persistence, and below-flush-threshold. Missing: the case where 12. commands, err := model.BuildListedCommands(context.Background(), commandStore)The connection's context (and any cancellation if the client disconnects) isn't threaded through. Minor but inconsistent with how other handlers could propagate cancellation. 13.
TestsThe bolt store tests are well-structured and cover the important scenarios. The Coverage gaps from Codecov ( Summary: The design and overall implementation quality is good. Issues #1–4 (nil panics, bounds check, transaction race, circuit breaker bypass) should be fixed before merge. #5 (silent empty |
Prune matched pre rows via pre.FindClosestCommand(posts), but that helper only accepts candidates at or before the pre's time. A post always runs after its pre, so it was never matched and synced pre commands were never pruned — the bolt active bucket (and the pre.txt fallback) grew unbounded. Match from the post side instead: a pre is finished when a synced post (recording time <= cursor) with the same unique key ran at or after it. Extract this as model.preHasSyncedPost, shared by the bolt and file stores. Also: run boltStore.Prune in a single write transaction (collect posts and the keys to delete in one pass over the archived bucket) and add defensive nil checks on tx.Bucket plus a length guard in decodeKeyNano, per review feedback. Adds a regression test covering the realistic case where post.Time > pre.Time.
Raise patch coverage on the new code with high-value unit tests: - model.BuildTrackingData (assembly, exclude filtering, empty case) - fileStore round trip (save/read/cursor/prune) over a temp HOME - daemon handlePubSubTrackPost happy path (flush -> sync -> cursor -> prune) via an httptest server, plus the no-store error branch - a socket round trip exercising list_commands (RequestListCommands / handleListCommands) and SendTrackEvent routing
- commands/ls.go file-store path (previously 0% covered) via a temp-HOME fixture exercising both json and table output - handlePubSubTrackPre/Post parse-error branches
The `track` command runs inside the shell hook on every command and is spawned as a fresh process each time, so the in-memory config cache never helps and reading config.toml/config.local.toml adds filesystem latency to every command. Stop reading config on the fast path: if a daemon is up on the default socket, forward the raw pre/post event and return. Config is only read in the daemon-less fallback (which may point at a custom socket or drive the direct HTTP sync). The daemon now owns the storage-engine decision and exclude filtering. It already reads config (cached, long-lived process), so the bolt-vs-txt choice and exclude rules live there. When the bolt engine is disabled the track handlers fall back to the txt file store (both satisfy CommandStore) instead of erroring, so default (file engine) users keep working through the daemon.
|
@claude ultrareview |
|
Claude finished @AnnatarHe's task in 6m 16s —— View job Ultra Code Review: feat: bbolt-backed command store
Previous Critical Issues — StatusAll four criticals from the first pass are confirmed fixed:
Still Open from First Pass1. Circuit breaker bypass on bolt track-post path —
|
| # | Finding | Severity | Status |
|---|---|---|---|
| 1 | Circuit breaker bypass in track_post | Critical | Open |
| 2 | Silent empty ls (bolt + daemon down) |
High | Open |
| 3 | No write deadline on SendTrackEvent |
High | Open |
| 7 | ls ignores DefaultSocketPath |
High | New |
| 4 | Double config read per flush | Medium | Open |
| 5 | In-flight message loss on shutdown | Medium | Open |
| 8 | cleanCommandFiles still broken pruning |
Medium | New |
| 6 | context.Background() in handleListCommands |
Low | Open |
| 9 | Identity no-op CursorID round-trip | Low | New |
| 10 | Silent bolt→file fallback at startup | Low | New |
The four previous criticals are all fixed. Issue #1 (circuit breaker) is the remaining blocker. Issues #2, #3, #7 are UX correctness problems worth resolving before users hit them. Issue #8 is a latent correctness bug in the file-mode gc path.
UltrareviewFull pass over the diff (storage abstraction, bolt/file stores, daemon track handlers, CLI routing). Build is green and the store/handler unit tests pass. Findings below, roughly in priority order. 1. Behavior change worth a conscious sign-off: file-engine persistence now flows through the daemonAfter the latest refactor,
Net: cleaner and correct, but it changes the default path for all existing users, not just bolt adopters. Flagging so it's deliberate rather than incidental. 2. Low-probability ordering hazard:
|
2 participants
Why
The CLI self-maintains three append-only text files (
pre.txt,post.txt,cursor.txt) in~/.shelltime/commands/. They're written synchronously from the shell hook (shelltime track ... &> /dev/null, no trailing&), so the write sits on the prompt's critical path — which is why fast, lock-free appends are used.This PR adds
go.etcd.io/bboltas a cleaner, more maintainable store. The key constraint: bbolt takes an exclusive OS file lock onOpen(single writer process), so opening +fsync-committing on every hook invocation in the CLI would be slower than the current append and would serialize concurrent terminals. So the daemon owns the bbolt DB, the hook becomes a fire-and-forget socket write, and the txt files remain the no-daemon fallback. Feature-gated via config; clean cutover (no migration of existing txt data).What (by commit)
feat(model)—CommandStoreinterface + factory (storage.engine:filedefault |bolt);fileStore(wraps today's txt logic) andboltStore(bbolt,active/archived/metabuckets, time-ordered collision-free keys). Reuses the pre-existingactiveBucket/archivedBucketconstants.feat(daemon)— daemon owns the bolt store for its lifetime; newtrack_pre/track_postsocket messages + handlers persist to bolt and run the flush/sync/cursor/prune cycle. ExtractedsendTrackArgsToServer(shared send path) andmodel.BuildTrackingData(shared payload assembly).feat(cli)— the track hook routes events to the daemon when bolt is enabled and the socket is ready, falling back to the txt store otherwise;trySyncLocalToServernow reusesBuildTrackingDataagainst an explicit file store.feat(cli)—shelltime lsqueries the daemon (newlist_commandsrequest/response) in bolt mode, since the CLI can't open the locked DB; sharedmodel.BuildListedCommandskeeps output identical across both paths.fix(cli)—gcskips txt command-file compaction in bolt mode (the daemon prunes;post.txtmay not exist).Verification
go vet+go test ./model/ ./commands/ ./daemon/pass (the pre-existing, environment-dependentTestGitInfoTestSuitefailure is unrelated — it fails onmaintoo in this sandbox). New unit tests cover the bolt store (save/read, time-ordering, equal-timestamp collisions, cursor, prune), the track handlers (fake store + config, no mockery), andBuildListedCommands.source: 1) → cursor advance → prune; sub-threshold commands stay buffered and are returned bylsvia the daemon; with the daemon down,trackfalls back topre.txt/post.txtand syncs via direct HTTP, writingcursor.txt.Notes
storageconfig the txt file store is used everywhere.https://claude.ai/code/session_01TePAhXoF7CR7aXRi6nTMsd