adopt's comment said replacing c.entries "would discard entries an earlier
failed Store kept in memory on purpose", which reads as a general guarantee.
It is not one: maybeReload still replaces c.entries wholesale, so the first
sibling write drops an unpersisted entry anyway, and for a key that is on
disk the value read under the lock wins over the newer in-memory one.
Both behaviours are the right call for a cache — converging on the state the
lock protected, and overwriting by key as Store does — but the comment should
not let a reader treat unpersisted entries as durable.
Also notes on the package doc that Store adopts the state it read under the
lock, since the doc previously attributed cross-process visibility to Lookup
alone.
The per-user cache (lock file dir and JSON cache dir) does not need to be world-readable. The lock file and JSON entries (via os.CreateTemp) are already 0o600.
Assisted-By: docker-agent
- Fix TOCTOU race condition in Cache.maybeReload by re-stating the file
under the write lock. The previous implementation stat'd the file before
acquiring the lock, then used the stale FileInfo after the lock, which
could lead to storing an outdated mtime that doesn't match the loaded
content. This could cause subsequent Lookups to miss updates in
high-concurrency scenarios.
- Clarify Windows lock semantics: the mandatory LockFileEx lock is on the
separate .lock file (not the data file), so both Unix and Windows achieve
the same advisory-lock effect for the cache operations.
- Document mtimeOf zero-value semantics: the zero time.Time{} is a sentinel
for 'file not found' and is safe to compare with time.Time.Equal.
- Add comment explaining Store's update-after-persist ordering: the in-memory
map is updated after the disk write so that persistence failures still keep
the entry in memory for the current process.
- Document resolveCachePath's lexical-only path traversal check: symlinks are
not resolved, which is acceptable for the threat model (cache paths come
from agent configs, not untrusted user input).
All tests pass with -race. No functional changes to the cache behavior.
Assisted-By: docker-agent
Three small simplifications in pkg/cache/cache.go that trim the file by
~10% and reduce conceptual surface, with no behavior change:
- Replace the fileLock struct + acquireFileLock + (l).release method
trio with a single `lockFile(path) (func(), error)` that returns a
release closure. The struct existed only to carry an *os.File and
expose a method that called Close — exactly what a closure does.
Saves the type declaration, two separate exports of the same thing,
and the awkward "lock, err := acquireFileLock(...); defer
lock.release()" dance.
- Inline the free-function persistEntry into a *Cache method
persistToDisk that updates c.mtime directly. The old shape had
persistEntry returning (time.Time, error) just so the caller could
do `if !mtime.IsZero() { c.mtime = mtime }`; the IsZero gymnastics
was only there because errors and a missing-stat shared the same
zero return. Method form lets us simply assign and return on error.
- Extract a tiny `mtimeOf(path) time.Time` helper for the three places
that want "current mtime, zero if unavailable" (New, persistToDisk's
dedup branch, persistToDisk's post-write branch). Three identical
three-line stat-or-zero blocks become one one-liner.
- Move the `c.path != ""` short-circuit into Cache.maybeReload itself
so Lookup's body no longer needs to know about the file/in-memory
distinction.
Validation: cross-platform builds (linux/windows amd64) clean, full
mise lint clean, mise test green, go test -race ./pkg/cache/... clean.
Assisted-By: docker-agent
Two processes (or two runtimes) sharing the same cache.json could
silently lose entries: each held its own in-memory map, snapshotted
under its own mutex, and wrote the file independently — last writer
won, the other side's just-stored entries vanished from disk.
Add cross-process serialization via an advisory lock on a sibling
`<path>.lock` file (POSIX flock(2) on Unix, LockFileEx on Windows),
and surface external writes through mtime-based reload on Lookup.
- pkg/cache/lock_unix.go, pkg/cache/lock_windows.go: thin wrappers
around unix.Flock and windows.LockFileEx (both already in our
golang.org/x/sys dep tree, no new packages).
- pkg/cache/cache.go: rework Cache.Store from "snapshot under c.mu,
write" to "lock <path>.lock, reload disk, merge our entry, write
atomically". The lock window is short — one read, one
fsync+rename — and serializes the read-modify-write across every
process holding the same path. Add Cache.maybeReload (called by
Lookup) to pick up external writes when the file mtime has
advanced. The persist callback is gone; path/mtime now live on
the struct.
- The lock file is a long-lived sentinel — never renamed or
deleted. Removing it would let two processes lock different
inodes and lose mutual exclusion.
- pkg/cache/cache_test.go: three new tests (and the leftover-files
test learned about the .lock sentinel):
* TestFileCache_crossProcessConcurrentStoresPreserveAllEntries
— two *Cache instances on the same path, 50 distinct keys
each, asserts all 100 entries are on disk after the race.
Without the lock this test loses ~50% of entries.
* TestFileCache_lookupReloadsAfterExternalWrite — one cache
writes, the other's Lookup picks up the change after the
mtime advances.
* TestFileCache_lockFileNeverDeleted — pins the invariant.
- docs/configuration/agents/index.md and examples/cached_responses.yaml
now describe the cross-process semantics so users know they can
point multiple agents (or multiple processes) at the same path.
In-memory dedup is preserved as a cheap short-circuit: when our local
view already matches, Store skips the file lock + disk hit entirely.
After a recent Lookup our view is fresh, so this catches the common
"store the answer we just replayed" path of cache_response.
Validation: cross-platform builds (linux/windows amd64) clean,
mise lint clean, mise test green, go test -race ./pkg/cache/...
./pkg/runtime/ ./pkg/teamloader/ clean.
Assisted-By: docker-agent
Targeted follow-ups from a second-pass review. No behavior change for
existing scenarios; the only functional delta is that
after_llm_call hooks now also receive AgentName / LastUserMessage,
matching what the field's doc comment already promised.
Bug fixes / contract tightening:
- pkg/runtime/runtime.go: populate AgentName and LastUserMessage in
executeAfterLLMCallHooks, not just executeStopHooks. The Input doc
comment listed both events; this finishes the implementation.
- pkg/runtime/cache.go: log a Debug line in cacheResponseBuiltin when
team.Agent(in.AgentName) fails so a misconfigured team is at least
visible in --debug output instead of silently dropping the write.
Comments to prevent future regressions:
- pkg/cache/cache.go: Store now explicitly documents WHY the persist
callback runs under the write lock. Releasing the lock between
cloning the snapshot and writing it would let two concurrent stores
persist out-of-order and silently overwrite a newer entry with an
older snapshot on the next restart — exactly the lost-write race
fixed in 73d806943. The comment makes the trade-off (brief read
latency vs. strict serialization) the deliberate choice it is.
- pkg/runtime/cache.go: explain why tryReplayCachedResponse treats an
empty cached value as a miss instead of replaying it.
- pkg/cache/cache.go: document the trim-then-lowercase order in
keyNormalizer so a future change doesn't accidentally swap them.
Tests:
- pkg/cache/cache_test.go::TestFileCache_persistenceFailureKeepsInMemory
pins the invariant that a write failure (here: cache directory
stripped of write permission after New) does not break Store —
the in-memory map still receives the entry, only the disk mirror
is skipped. Skipped when running as root.
- pkg/runtime/cache_test.go::TestCache_EmptyResponseIsNotCached pins
the empty-response semantics: an empty assistant content is dropped
from the cache and the model is consulted on every repeat ask.
Validation: mise lint clean, full mise test green,
go test -race ./pkg/cache/... ./pkg/runtime/ ./pkg/teamloader/ clean.
Assisted-By: docker-agent
Three small simplifications, no feature change.
- pkg/cache: collapse the single-implementation Cache interface into an
exported *Cache struct (Go's "accept interfaces, return structs"
guideline). New now returns *cache.Cache, callers use a pointer
field/parameter, and the unexported `cache` type goes away. Net: one
fewer indirection per call site, ~10 fewer lines.
- pkg/cache: push the "skip rewrite when (q, response) already match"
optimization down into Cache.Store. The cache itself is the right
place for it, and any future caller benefits — the runtime
cache_response builtin no longer needs an extra Lookup before Store.
Add a unit test (TestFileCache_dedupSkipsRedundantWrite) that
asserts the file's mtime is preserved on identical writes.
- pkg/runtime/cache.go: as a follow-up to the above, cacheResponseBuiltin
now collapses to a 3-line happy path (resolve agent, get cache,
Store) — the dedup branch is gone.
- pkg/teamloader: extract the 24-line inline cache wiring (path
resolution, traversal check, cache.New) into a dedicated
buildAgentCache helper in a new cache.go file alongside the loader.
The agent loop body now reads as 5 lines. Add unit tests for the
helper covering disabled config, in-memory, relative path,
absolute path, and the path-traversal rejection.
Validation: mise lint clean, full mise test green (incl. existing
runtime cache tests, new pkg/cache and pkg/teamloader unit tests, and
race detector on pkg/cache/runtime/teamloader).
Assisted-By: docker-agent
- Fix race condition in cache.Store: hold lock during persist to ensure
snapshot consistency with disk writes
- Fix resource leak in syncDir: use defer to ensure directory fd is closed
- Add path traversal validation in teamloader: prevent cache paths from
escaping parent directory using filepath.Clean and prefix check
- Add test for path traversal protection
Assisted-By: docker-agent
Same feature, less code. The cache package and the runtime integration
each had visible duplication that made the moving parts harder to
follow than they needed to be.
- pkg/cache: merge `memoryCache` and `fileCache` into a single `cache`
struct with an optional `persist func(map[string]string)` callback.
The in-memory backend leaves persist nil; the file-backed backend
sets it to a closure that writes the snapshot via the existing
atomic-rename helper. Lookup, Store, and the locking pattern now
exist in one place instead of two.
- pkg/cache: tidy `writeJSON` (drop the dir != "." special case since
filepath.Dir always returns a usable directory; pull the directory
fsync into a small `syncDir` helper) and use `maps.Clone` for the
snapshot.
- pkg/runtime: move the cache hit/store logic out of the long
`RunStream` body into two named helpers in a new `cache.go` file:
`tryReplayCachedResponse` (returns the question + a replayed flag)
and `cacheTurnResponse` (stores then clears the question pointer).
The main loop now reads as a two-line cache check and a one-line
store call.
Net change: -45 lines across the two files. No behavior change; lint,
existing cache unit tests, and runtime cache tests all pass unchanged.
Assisted-By: docker-agent
Add a "Response Cache" section to the agent configuration reference and
strengthen the file-backed cache so writes are not just atomic but also
durable.
- docs/configuration/agents/index.md: extend the schema example and
property table with the new `cache` field, and add a dedicated
"Response Cache" section describing the matching options
(case_sensitive, trim_spaces), the storage backends (in-memory vs
JSON file), and the atomicity guarantees of the file-backed store.
- pkg/cache: fsync the temp file before rename and fsync the parent
directory after rename so the rename itself is persisted across an
OS crash. Concurrent readers are still guaranteed to see either the
previous or the new content in full, never a torn write.
- pkg/cache: add tests asserting that the success path leaves no
leftover temp files and that a parallel reader never observes a
partially written cache file under sustained concurrent stores.
Assisted-By: docker-agent
Add a per-agent response cache that maps a normalized user question to
its previous assistant answer, replaying it verbatim on a hit and
skipping the model entirely.
- New pkg/cache package exposing a Cache interface backed by either an
in-memory map (default) or a JSON file that is loaded on startup and
rewritten atomically on every store.
- Two normalization options: case_sensitive (default false) and
trim_spaces (default false), so "Hello" and " hello " can map to
the same cached answer when configured.
- New CacheConfig in pkg/config/latest with matching agent-schema.json
definition, plus an examples/cached_responses.yaml demonstrating the
feature.
- Agent gains a Cache() accessor and a WithCache option; teamloader
builds the cache from config and resolves a relative path against
the agent config directory.
- Runtime loop looks up the latest user message at the start of
RunStream and replays the cached answer (with AgentInfo,
MessageAdded, and stop hooks) on a hit, then stores the final
response on the first stop so subsequent identical turns short-
circuit the model.
- Unit tests for both cache backends and runtime-level tests covering
hit/miss, case-insensitive matching, whitespace trimming, and the
no-cache path.
Assisted-By: docker-agent
David Gageot
Sayt-0
Dwin Gharibi
Trung Nguyen