Address review feedback from @alexhoshina and Codex:
- Log sendLoading errors in ticker goroutine instead of discarding
- Only start typing indicator when PlaceholderRecorder is available
to avoid wasted API calls and unnecessary goroutine creation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(skills): add retry mechanism for HTTP requests
Implement a retry mechanism with exponential backoff for HTTP requests in the skill installer. This improves reliability when fetching skills from GitHub by automatically retrying failed requests up to 3 times.
Add comprehensive tests to verify retry behavior under different scenarios including success on different attempts and proper delay between retries.
* fix: improve http request retry logic with status code checks
Add shouldRetry helper function to determine retryable status codes.
Close response body between retry attempts and break early for non-retryable status codes.
* refactor: remove unused BuiltinSkill struct
The struct was not being used anywhere in the codebase, so it's safe to remove it to reduce clutter and improve maintainability.
* refactor(http): move retry logic to utils package
Extract HTTP retry functionality from skills package to utils for better reusability
Add context-aware sleep function and comprehensive tests
* refactor(http): extract retry delay unit to variable
Extract hardcoded retry delay unit to a variable for better testability and flexibility. Update tests to use milliseconds for faster execution while maintaining the same behavior.
* test(http_retry): remove t.Parallel from test cases
* test(http_retry): remove redundant test cases for retry success
The removed test cases for success on second and third attempts were redundant since the retry logic is already covered by other tests. This simplifies the test suite while maintaining coverage.
Change the default value of session.dm_scope from "main" to
"per-channel-peer" to provide better conversation isolation by
default. This prevents context leakage between different users
and channels.
MigrateFromJSON previously called AddFullMessage in a loop, then
renamed the .json file to .json.migrated. If the process crashed
after appending some messages but before the rename, a retry would
re-read the same .json and append all messages again — duplicating
whatever was written before the crash.
Switch to SetHistory which atomically replaces the session contents.
A retry after crash overwrites the partial data instead of appending.
In SetHistory and Compact, the JSONL file was rewritten before updating
the meta file. If the process crashed between the two writes, the meta
still had a large Skip value pointing past the now-shorter JSONL file,
causing GetHistory to return empty — effectively data loss.
Reverse the order: write meta (with Skip=0) first, then rewrite JSONL.
On crash between the two writes, the old uncompacted file is still
intact and GetHistory reads from line 1, returning stale-but-complete
data. The next operation self-corrects.
A crash between the JSONL append and the meta update in addMsg can
leave meta.Count stale (e.g. file has 101 lines but meta says 100).
The previous code only reconciled when Count==0, so a nonzero stale
count was silently trusted, causing keepLast/skip to be calculated
against the wrong total.
Now TruncateHistory always counts the actual lines on disk. This is
cheap (scan without unmarshal) and TruncateHistory is not a hot path.
Changed `go generate ./cmd/picoclaw` to `go generate ./cmd/picoclaw/...`
so that the workspace embed in cmd/picoclaw/internal/onboard is correctly
generated before building.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Implement TypingCapable interface for LINE channel using the
loading animation API (1:1 chats only, no group support).
- Add StartTyping() with 50s periodic refresh and context-based stop
- Integrate PlaceholderRecorder.RecordTypingStop in processEvent
- Skip RecordPlaceholder (LINE has no message edit API)
- Change sendLoading to accept context and return error
- Relax callAPI status check from 200 to 2xx range
Design consulted with Codex (GPT-5.2).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address feedback from @yinwm for long-running daemon use:
- Replace sync.Map with a fixed-size sharded lock array (64 mutexes).
Keys are mapped via FNV hash, so memory is O(1) regardless of how
many sessions are created over the process lifetime.
- Increase scanner buffer cap from 1 MB to 10 MB. Tool results
(read_file on large files, web search responses) can easily exceed
1 MB. The scanner still starts at 64 KB and only grows as needed.
Address Codex (GPT-5.2) review feedback:
- Start: guard against Interval<=0 or MaxAge<=0 to prevent
time.NewTicker panic on misconfiguration
- ReleaseAll: split into two phases (collect under lock, delete
after unlock) matching CleanExpired pattern
- ReleaseAll: log file removal errors
- Add TestStartZeroIntervalNoPanic and TestStartZeroMaxAgeNoPanic
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CleanExpired: split into two phases — collect expired entries under
lock, then delete files after releasing the lock to minimize contention
- CleanExpired: guard against zero MaxAge (no-op if unconfigured)
- CleanExpired: log file removal errors instead of silently ignoring
- Start: protect with startOnce to prevent multiple goroutines
- Stop: rename once -> stopOnce for clarity
- cmd_gateway: call mediaStore.Stop() on error path after Start()
- Add TestCleanExpiredZeroMaxAge and double-Start test
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback from @Zhaoyikaiii:
- Replace map[string]*sync.Mutex + separate mu with sync.Map.LoadOrStore
for simpler, lock-free session lock management.
- Add skip parameter to readMessages so callers (GetHistory, Compact)
can skip truncated lines without paying the json.Unmarshal cost.
- Add countLines helper for TruncateHistory's count reconciliation,
avoiding full deserialization when only the line count is needed.
Address file growth concern from #711 review: logical truncation via
skip offset is fast but leaves dead lines on disk indefinitely.
Compact() rewrites the JSONL file keeping only active messages, using
the same temp+rename pattern for crash safety. No-op when skip == 0.
The caller (lifecycle manager or agent loop) decides when to trigger
compaction — e.g. when skipped lines exceed active lines.
Read existing sessions/*.json files, convert to JSONL format, and
rename originals to .json.migrated as backup. The migration is
idempotent — second runs skip already-migrated files.
Session keys are read from JSON content (not filenames) so that
sanitized names like telegram_123 correctly map back to telegram:123.
Cover all Store interface methods plus edge cases:
- Basic roundtrip, ordering, empty session, tool calls
- Logical truncation (keep last N, keep zero, keep more than exist)
- SetHistory replacing all + resetting skip offset
- Crash recovery with partial JSON lines
- Persistence across store instances
- Concurrent add+read (10 goroutines x 20 msgs)
- Simulated #704 race (summarizer vs main loop)
- Benchmarks for AddMessage and GetHistory (100/1000 msgs)
Add JSONLStore that persists sessions as .jsonl files (one message per
line) plus .meta.json for summary and truncation offset.
Key design decisions:
- Append-only writes — no full-file rewrites on AddMessage
- Logical truncation via skip offset instead of physical deletion
- Per-session mutex for safe concurrent access
- Crash recovery: malformed trailing lines are silently skipped
- Atomic metadata writes using temp+rename
Zero new dependencies — pure stdlib.
Refs #711
Introduce a backend-agnostic Store interface in pkg/memory/ that maps
one-to-one with the current SessionManager API. Each method is atomic
— no separate Save() call needed.
Refs #711
* fix: remove redundant tools definitions from system prompt
Tools are already provided to the LLM via JSON schema through
ToProviderDefs(), so the text-based tools section in the system
prompt is redundant.
This removes the buildToolsSection() logic and the tools field
from ContextBuilder, reducing system prompt length while maintaining
the "ALWAYS use tools" rule reminder.
Fixes#731
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix: correct spelling 'initialized' (was 'initialised')
---------
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Whitespace is a linter that checks for unnecessary newlines at the start and end of functions, if, for, etc.
Signed-off-by: Kai Xia <kaix+github@fastmail.com>
