Skip to content

feat(extensions): add OnLLMUsage, SetState, enriched AgentEndEvent (#53)#54

Merged
ezynda3 merged 3 commits into
masterfrom
feat/53-llmusage-state-agentend
Jun 9, 2026
Merged

feat(extensions): add OnLLMUsage, SetState, enriched AgentEndEvent (#53)#54
ezynda3 merged 3 commits into
masterfrom
feat/53-llmusage-state-agentend

Conversation

@ezynda3

@ezynda3 ezynda3 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Description

Adds three independent, additive primitives to the extension API surface that issue #53 called out as missing, all delivered together because they share an implementation theme (give extensions accurate per-turn signals and a lightweight place to put state):

  1. OnLLMUsage event — fires after every LLM provider round-trip with per-call token + cost deltas attributed to the specific model/provider used. Derived from the existing SDK StepFinishEvent in the extension bridge; cost is computed from the models registry (zero when pricing is unknown or OAuth credentials are in use). This unblocks budget enforcement that needs to react between calls within a turn, not only at turn boundaries.
  2. ctx.SetState / GetState / DeleteState / ListState — session-scoped, last-write-wins key-value store backed by a sidecar file (<session>.ext-state.json) outside the conversation tree. Reads are O(1), writes don't grow the session JSONL, and the store is not duplicated when the conversation forks. State is invisible to the LLM, survives session resume, and is preserved across extension hot-reloads. Use this for snapshot state ("current value of X"); use AppendEntry for audit logs.
  3. Enriched AgentEndEvent — adds ToolCallCount, ToolNames, LLMCallCount, InputTokensDelta, OutputTokensDelta, CacheReadTokensDelta, CacheWriteTokensDelta, CostDelta, and DurationMs. Populated by a per-turn aggregator hooked into TurnStartEvent / ToolResultEvent / StepFinishEvent. Existing handlers reading only Response / StopReason are unaffected.

A new example extension usage-budget.go demonstrates all three primitives together as a soft-cost-cap with a per-turn report.

Fixes #53

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (godoc on every new exported symbol)
  • I have made corresponding changes to the documentation (README, docs site, skill guides)
  • My changes generate no new warnings (go vet and golangci-lint run both clean)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (go test -race ./...)
  • Docs site builds cleanly (npx tome build in www/)

Additional Information

New files

  • internal/extensions/state_test.go — 7 tests for the state store (CRUD, saver hook, save/load round-trip, malformed JSON, missing file, concurrent writes, no-op defaults).
  • internal/extensions/llmusage_test.go — 3 tests for LLMUsageEvent registration and enriched AgentEndEvent field round-trip.
  • pkg/kit/extensions_bridge_test.go — 6 tests for turnAggregator, llmUsageMeta nil-safety, and extStateSidecarPath.
  • examples/extensions/usage-budget.go — demo extension wiring all three primitives.

Notable changes

  • internal/extensions/api.go — new LLMUsageEvent struct, four new Context function fields, enriched AgentEndEvent, new OnLLMUsage registrar.
  • internal/extensions/runner.go — state store (SetState/GetState/DeleteState/ListState/SnapshotState/LoadStateFromFile/SaveStateToFile/SetStateSaver) with sync.RWMutex protection. State preserved across Reload.
  • internal/extensions/{events,loader,test_api,symbols}.go — wired LLMUsage event type, handler registration, and Yaegi export.
  • pkg/kit/extensions_bridge.goturnAggregator subscribes to turn/tool/step events; emits LLMUsageEvent derived from StepFinishEvent; populates enriched fields on AgentEndEvent.
  • pkg/kit/extension_api.go — exposes the state methods on ExtensionAPI. New InitStatePersistence() loads any existing sidecar and installs a saver hook.
  • cmd/{extension_context,root}.go — wires the four Context callbacks and calls InitStatePersistence before EmitSessionStart.
  • internal/extensions/events_test.go — bumps the expected count from 32 → 33.

Backwards compatibility

Fully additive. All previously valid handler signatures continue to compile and run unchanged. Extensions that read only Response/StopReason on AgentEndEvent see the same values they always have; the new fields default to zero values. The state store is opt-in — no behaviour change for extensions that don't call SetState.

Documentation

  • README.md — added OnLLMUsage to lifecycle list, added Session State bullet, added usage-budget.go to examples list, added a paragraph explaining the enriched event.
  • www/pages/extensions/{overview,capabilities,examples}.md — capability table, field tables for both new events, new Session state section with a "when to use which" comparison vs AppendEntry.
  • skills/kit-extensions/SKILL.md and skills/kit-sdk/SKILL.md — updated agent skill guides.

Summary by CodeRabbit

  • New Features
    • Persist session-scoped key/value state with last-write-wins semantics and optional per-session persistence/init.
    • New OnLLMUsage event: fires after each LLM call with per-call token & cost deltas.
    • OnAgentEnd enriched with per-turn aggregates: tool & LLM call counts, token/cost deltas, duration, and tool names.
  • Documentation
    • Expanded lifecycle, session-state, and examples (including a usage-budget example demonstrating per-call usage and state persistence).

Three additive primitives to the extension API:

- OnLLMUsage event: per-LLM-call token + cost deltas attributed to the
  specific model/provider used for each round-trip. Derived from the SDK
  StepFinishEvent in the extension bridge. Enables accurate budget
  enforcement between calls instead of only at turn boundaries.

- ctx.SetState / GetState / DeleteState / ListState: session-scoped,
  last-write-wins key-value store backed by a sidecar file
  (<session>.ext-state.json) outside the conversation tree. Reads are
  O(1), writes don't grow the JSONL, and the store is not duplicated on
  fork. State is preserved across hot-reloads.

- Enriched AgentEndEvent: ToolCallCount, ToolNames, LLMCallCount, token
  deltas (input/output/cache-read/cache-write), CostDelta, and
  DurationMs populated by a per-turn aggregator. Existing handlers
  reading only Response/StopReason are unaffected.

Includes unit tests for the state store, LLMUsage registration,
enriched AgentEndEvent, turn aggregator, llmUsageMeta, and sidecar path
derivation. Adds examples/extensions/usage-budget.go demoing all three
primitives together. Documents the additions in README, the docs site
(extensions overview, capabilities, examples), and the kit-extensions
and kit-sdk skill guides.

Fixes #53
@mark-iii-labs-huly

Copy link
Copy Markdown

Connected to Huly®: KIT-55

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 86a81660-ff68-41d0-99bb-8d4d3c7095a3

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7e422 and fbf074b.

📒 Files selected for processing (2)
  • internal/extensions/runner.go
  • internal/extensions/state_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/extensions/state_test.go
  • internal/extensions/runner.go

📝 Walkthrough

Walkthrough

Adds per-LLM-call usage events (OnLLMUsage), session-scoped key/value state APIs with optional per-session sidecar persistence, enriched AgentEndEvent per-turn aggregates, runtime turn aggregation and LLM cost/meta bridging, startup/context wiring, tests, examples, and documentation.

Changes

Extension API Contracts

Layer / File(s) Summary
Event types and lifecycle constants
internal/extensions/events.go, internal/extensions/api.go, internal/extensions/events_test.go
Adds LLMUsage event type; introduces LLMUsageEvent and expands AgentEndEvent with per-turn aggregates.
Context and API method signatures
internal/extensions/api.go
Adds Context state function fields (SetState, GetState, DeleteState, ListState) and API.OnLLMUsage registration surface; clarifies AppendEntry docs.
Yaegi exports and loader/test helpers
internal/extensions/symbols.go, internal/extensions/loader.go, internal/extensions/test_api.go
Exports LLMUsageEvent to the extension symbol table; wires onLLMUsage into loader and test API.

Runner state and persistence

Layer / File(s) Summary
Runner state store and methods
internal/extensions/runner.go, internal/extensions/state_test.go
Introduces thread-safe in-memory state map, saver mutex, SetState/GetState/DeleteState/ListState, SnapshotState, LoadStateFromFile, SaveStateToFile, SetStateSaver; tests for semantics, persistence, concurrency, and saver panic behavior.
Public extension API persistence initializer
pkg/kit/extension_api.go
Extends ExtensionAPI with state methods and InitStatePersistence() which computes per-session sidecar path, loads state, and installs a saver hook to persist updates atomically.
Interactive context wiring and startup
cmd/extension_context.go, cmd/root.go
Wires state callbacks into interactive Context and calls InitStatePersistence() on startup, logging warnings on init failure instead of aborting.

Turn aggregation and LLM usage bridge

Layer / File(s) Summary
Turn aggregator and emission
pkg/kit/extensions_bridge.go
Adds turnAggregator to collect tool/LLM counts, token/cost deltas, and duration per turn; consumes snapshot to enrich AgentEndEvent.
Per-step LLM usage meta and emission
pkg/kit/extensions_bridge.go
When handlers registered, subscribes StepFinishEvent to emit LLMUsageEvent with computed model/provider metadata and cost (with Anthropic OAuth suppression).
Bridge tests and helpers
pkg/kit/extensions_bridge_test.go
Unit tests for aggregator lifecycle, duration, llmUsageMeta nil safety, provider checks, and sidecar path derivation.

Tests and validation

Layer / File(s) Summary
LLMUsage and AgentEnd tests
internal/extensions/llmusage_test.go
Tests verifying LLMUsageEvent emission/field propagation, OnLLMUsage registration via NewTestAPI, and preservation of enriched AgentEndEvent fields.
Runner state tests
internal/extensions/state_test.go
Expanded tests for Set/Get/Delete/List semantics, saver firing behavior, save/load round-trip, missing/empty/malformed file behavior, concurrency, context no-ops, and saver panic release.

Documentation and example

Layer / File(s) Summary
Usage-budget example and docs
examples/extensions/usage-budget.go, examples/extensions/README.md, README.md, www/pages/extensions/*, skills/*
Adds usage-budget.go example demonstrating OnLLMUsage and session state; updates capability, overview, examples pages and skill docs to document OnLLMUsage, enriched AgentEndEvent fields, and session state semantics.

Sequence Diagram(s)

sequenceDiagram
  participant TurnStartEvent
  participant turnAggregator
  participant StepFinishEvent
  participant Runner
  participant StateSidecarFile
  participant ExtensionHandler
  TurnStartEvent->>turnAggregator: start()
  StepFinishEvent->>turnAggregator: record step/tool usage
  StepFinishEvent->>Runner: emit LLMUsageEvent
  Runner->>ExtensionHandler: call OnLLMUsage handlers
  Runner->>StateSidecarFile: stateSaver -> SaveStateToFile()
  turnAggregator->>Runner: consume() -> AgentEndEvent enriched
  Runner->>ExtensionHandler: call OnAgentEnd handlers
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Possibly related PRs:
    • mark3labs/kit#24: Overlaps with buildInteractiveExtensionContext wiring changes for extension context state handlers.

"I hopped through code with eager paws,
counting tokens, costs, and clause.
Per-call whispers, sidecar keeps,
turns report while daylight sleeps. 🐇"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely summarizes the main changes: adding OnLLMUsage event, SetState session state API, and enriched AgentEndEvent fields.
Linked Issues check ✅ Passed All three primary requirements from issue #53 are fully implemented: OnLLMUsage event with per-call usage callbacks, session-scoped SetState/GetState/DeleteState/ListState APIs, and enriched AgentEndEvent with per-turn aggregates including tool counts, token deltas, cost delta, and duration.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #53 requirements. Documentation updates, new example extension, tests, and implementation are all in scope for delivering the three new extension primitives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/53-llmusage-state-agentend

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
internal/extensions/events.go (1)

137-147: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Include ToolOutput in AllEventTypes().

ToolOutput is declared as a supported event above, but this slice still skips it, so EventType.IsValid() returns false for a real lifecycle event.

🐛 Proposed fix
 	return []EventType{
 		ToolCall, ToolCallInputStart, ToolCallInputDelta, ToolCallInputEnd,
-		ToolExecutionStart, ToolExecutionEnd, ToolResult,
+		ToolExecutionStart, ToolExecutionEnd, ToolOutput, ToolResult,
 		Input, BeforeAgentStart, AgentStart, AgentEnd,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extensions/events.go` around lines 137 - 147, AllEventTypes()
currently returns a slice missing the ToolOutput EventType, causing
EventType.IsValid() to reject real lifecycle events; update the slice returned
by AllEventTypes (in internal/extensions/events.go) to include ToolOutput among
the other EventType entries (e.g., alongside ToolResult/ToolExecutionEnd) so
ToolOutput is recognized as a valid event.
cmd/root.go (1)

1197-1204: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reinitialize extension state when switching sessions.

This callback swaps the tree session but leaves the extension state map and saver bound to the previous session. After a /new or resume into a different JSONL, GetState will still read the old session's keys and SetState will keep writing the old sidecar.

🐛 Proposed fix
 func switchSessionForUI(path string) error {
 	ts, err := kit.OpenTreeSession(path)
 	if err != nil {
 		return fmt.Errorf("failed to open session: %w", err)
 	}
 	kitInstance.SetTreeSession(ts)
 	appInstance.SwitchTreeSession(ts)
+	if err := kitInstance.Extensions().InitStatePersistence(); err != nil {
+		return fmt.Errorf("reinitializing extension state: %w", err)
+	}
 	return nil
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/root.go` around lines 1197 - 1204, The switchSessionForUI callback
currently replaces the TreeSession but leaves extension state and its saver
bound to the old session; update switchSessionForUI (after
kitInstance.SetTreeSession and appInstance.SwitchTreeSession) to reinitialize
the extension state map and recreate/rebind the extension state saver for the
newly opened session so GetState/SetState operate on the new session's
keys/sidecar (use the TreeSession/ts identifier or path from kit.OpenTreeSession
to create the new saver and an empty state map, replacing the previous
extStateMap/extStateSaver used by GetState/SetState).
🧹 Nitpick comments (2)
pkg/kit/extension_api.go (1)

5-6: ⚡ Quick win

Use structured logging here.

Please route this warning through github.com/charmbracelet/log instead of log.Printf so it stays consistent with the repo's logging contract.

As per coding guidelines, **/*.go: Use github.com/charmbracelet/log for structured logging.

♻️ Proposed fix
-import (
-	"fmt"
-	"log"
-	"strings"
+import (
+	"fmt"
+	"strings"
 
+	"github.com/charmbracelet/log"
 	"github.com/mark3labs/kit/internal/extensions"
 	"github.com/mark3labs/kit/internal/message"
 	"github.com/mark3labs/kit/internal/session"
 )
@@
 	runner := e.kit.extRunner
 	runner.SetStateSaver(func() {
 		if err := runner.SaveStateToFile(path); err != nil {
-			log.Printf("WARN extension state save failed: path=%s err=%v", path, err)
+			log.Warn("extension state save failed", "path", path, "err", err)
 		}
 	})

Also applies to: 394-397

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/kit/extension_api.go` around lines 5 - 6, Replace usages of the standard
library logger with the project's structured logger: remove the "log" import and
import "github.com/charmbracelet/log" instead, then replace log.Printf calls
that emit warnings with the charmbracelet logger equivalents (e.g., log.Warn or
log.Warnf) so messages are structured; update every occurrence in this file
(including the warnings around the earlier occurrence and the ones at the other
locations noted around lines 394-397) to use log.Warn/ log.Warnf and preserve
the original message and formatting arguments.

Source: Coding guidelines

cmd/root.go (1)

934-935: ⚡ Quick win

Prefer structured logging for the startup state warning.

This new warning uses stdlib logging instead of the repository's structured logger.

As per coding guidelines, **/*.go: Use github.com/charmbracelet/log for structured logging.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/root.go` around lines 934 - 935, The call to log.Printf in the
kitInstance.Extensions().InitStatePersistence() error path uses stdlib logging;
replace it with the repository's structured logger from
github.com/charmbracelet/log (e.g., call logger.Warn or log.Warn with
field-style context) and include the error as a field (error=err) and a clear
message like "extension state init failed" so the InitStatePersistence() failure
is recorded using structured logging instead of fmt-based Printf.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/extensions/runner.go`:
- Around line 773-783: Concurrent callers to Runner.SetState (and DeleteState)
can race because saver() is invoked after releasing stateMu, allowing
out-of-order snapshot writes to path+".tmp"; fix by serializing saver execution:
either call r.stateSaver() while still holding r.stateMu (move the saver
invocation inside the critical section in Runner.SetState and similarly in
DeleteState) or add a dedicated mutex (e.g., r.saverMu) and wrap saver() calls
with that mutex while ensuring the saver reads state under r.stateMu so
snapshots are consistent; update SetState, DeleteState and any other places
noted (lines ~797-807, 877-899) to use the same approach to prevent overlapping
saver runs and temp-file races.
- Around line 852-869: LoadStateFromFile currently returns early on missing file
or empty data and leaves r.state unchanged, causing stale keys to persist;
modify Runner.LoadStateFromFile so that when os.IsNotExist(err) is true or when
len(data)==0 it locks r.stateMu and replaces r.state with an empty map (e.g.,
map[string]string{}), then unlocks, ensuring the in-memory store is cleared for
a fresh session; keep the rest of the function behavior (JSON unmarshal path)
unchanged.

In `@pkg/kit/extensions_bridge.go`:
- Around line 332-356: The llmUsageMeta call is producing non-zero cost even for
OAuth-backed sessions; update the LLM usage path to detect OAuth credentials and
force Cost=0 when OAuth is in use. Either extend llmUsageMeta to accept the
session/credentials (or return an explicit oauth-backed boolean) and have
m.Subscribe handlers for StepFinishEvent use that to set Cost/Cos tDelta to 0
before emitting extensions.LLMUsageEvent, or check ev (e.g., ev.Session /
ev.Credentials) in the subscriber and override the computed cost to zero when
OAuth is present; apply the same change to the other LLM-usage emitter block
around the later section (the second occurrence referenced in the comment).
- Around line 24-37: turnAggregator is currently a single shared accumulator
(created as turnAgg := &turnAggregator{kit: m}) that is reset on TurnStartEvent
and snapshotted on TurnEndEvent, which allows interleaved/concurrent turns to
clobber each other's tool/step/usage counts; fix by making aggregation per-turn
(e.g., change turnAggregator to maintain a map[keyed by turn ID] and use the
event TurnStartEvent/TurnEndEvent IDs to start/stop/snapshot via
recordTool/recordStep into the per-turn entry) or alternatively enforce single
in-flight turn by adding mutual exclusion around Kit.runTurn/Prompt so events
cannot overlap. Also address the llmUsageMeta comment mismatch: either update
the comment on llmUsageMeta to accurately state when cost is zero (i.e., when
model/pricing info is missing/unparseable) or implement the intended OAuth
branch (detect OAuth credentials and set cost to zero) inside llmUsageMeta so
the behavior matches the comment. Ensure references to turnAggregator,
TurnStartEvent, TurnEndEvent, recordTool, recordStep, Kit.runTurn/Prompt, and
llmUsageMeta are used to locate the changes.

---

Outside diff comments:
In `@cmd/root.go`:
- Around line 1197-1204: The switchSessionForUI callback currently replaces the
TreeSession but leaves extension state and its saver bound to the old session;
update switchSessionForUI (after kitInstance.SetTreeSession and
appInstance.SwitchTreeSession) to reinitialize the extension state map and
recreate/rebind the extension state saver for the newly opened session so
GetState/SetState operate on the new session's keys/sidecar (use the
TreeSession/ts identifier or path from kit.OpenTreeSession to create the new
saver and an empty state map, replacing the previous extStateMap/extStateSaver
used by GetState/SetState).

In `@internal/extensions/events.go`:
- Around line 137-147: AllEventTypes() currently returns a slice missing the
ToolOutput EventType, causing EventType.IsValid() to reject real lifecycle
events; update the slice returned by AllEventTypes (in
internal/extensions/events.go) to include ToolOutput among the other EventType
entries (e.g., alongside ToolResult/ToolExecutionEnd) so ToolOutput is
recognized as a valid event.

---

Nitpick comments:
In `@cmd/root.go`:
- Around line 934-935: The call to log.Printf in the
kitInstance.Extensions().InitStatePersistence() error path uses stdlib logging;
replace it with the repository's structured logger from
github.com/charmbracelet/log (e.g., call logger.Warn or log.Warn with
field-style context) and include the error as a field (error=err) and a clear
message like "extension state init failed" so the InitStatePersistence() failure
is recorded using structured logging instead of fmt-based Printf.

In `@pkg/kit/extension_api.go`:
- Around line 5-6: Replace usages of the standard library logger with the
project's structured logger: remove the "log" import and import
"github.com/charmbracelet/log" instead, then replace log.Printf calls that emit
warnings with the charmbracelet logger equivalents (e.g., log.Warn or log.Warnf)
so messages are structured; update every occurrence in this file (including the
warnings around the earlier occurrence and the ones at the other locations noted
around lines 394-397) to use log.Warn/ log.Warnf and preserve the original
message and formatting arguments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2291a81-b571-4208-9da0-99195b039ae6

📥 Commits

Reviewing files that changed from the base of the PR and between febdc53 and 8b442d2.

📒 Files selected for processing (22)
  • README.md
  • cmd/extension_context.go
  • cmd/root.go
  • examples/extensions/README.md
  • examples/extensions/usage-budget.go
  • internal/extensions/api.go
  • internal/extensions/events.go
  • internal/extensions/events_test.go
  • internal/extensions/llmusage_test.go
  • internal/extensions/loader.go
  • internal/extensions/runner.go
  • internal/extensions/state_test.go
  • internal/extensions/symbols.go
  • internal/extensions/test_api.go
  • pkg/kit/extension_api.go
  • pkg/kit/extensions_bridge.go
  • pkg/kit/extensions_bridge_test.go
  • skills/kit-extensions/SKILL.md
  • skills/kit-sdk/SKILL.md
  • www/pages/extensions/capabilities.md
  • www/pages/extensions/examples.md
  • www/pages/extensions/overview.md

Comment thread internal/extensions/runner.go Outdated
Comment thread internal/extensions/runner.go
Comment thread pkg/kit/extensions_bridge.go
Comment thread pkg/kit/extensions_bridge.go
- Serialize SetState/DeleteState saver invocations through a new saverMu
  so overlapping atomic-rename writes can no longer race on the shared
  .tmp file and persist an older snapshot after a newer one.
- LoadStateFromFile now clears the in-memory store when the sidecar is
  missing or empty, matching the documented "replace … with its
  contents" contract. This makes session-switching safe by preventing
  keys from a prior session leaking into a new one. Tests updated to
  cover both the missing-file and empty-file cases.
- llmUsageMeta now detects Anthropic OAuth credentials and returns
  Cost=0, matching the comment and the existing usage_tracker behavior
  for OAuth users. Mirrors the OAuth detection already used in
  cmd/extension_context.go.
- Document the single-in-flight-turn assumption baked into the
  per-turn aggregator with a clear migration path (per-turn ID) for if
  concurrent turns ever become a supported use case.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/extensions/runner.go (1)

896-916: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

SaveStateToFile still races for direct callers.

The new saverMu only protects the callback path in SetState/DeleteState. Two goroutines can still call SaveStateToFile(path) concurrently, both write to path + ".tmp", and one rename/remove sequence can clobber the other's temp file or return a spurious error. That breaks the method's "Thread-safe" contract for persistence itself.

🔧 Suggested direction
 func (r *Runner) SaveStateToFile(path string) error {
+	r.saverMu.Lock()
+	defer r.saverMu.Unlock()
+
 	snap := r.SnapshotState()
 	if snap == nil {
 		snap = map[string]string{}
 	}
@@
-	tmp := path + ".tmp"
-	if err := os.WriteFile(tmp, data, 0o644); err != nil {
+	tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp")
+	if err != nil {
+		return fmt.Errorf("creating temp state file: %w", err)
+	}
+	tmp := tmpFile.Name()
+	if _, err := tmpFile.Write(data); err != nil {
+		_ = tmpFile.Close()
+		_ = os.Remove(tmp)
+		return fmt.Errorf("writing extension state: %w", err)
+	}
+	if err := tmpFile.Close(); err != nil {
+		_ = os.Remove(tmp)
+		return fmt.Errorf("closing extension state temp file: %w", err)
+	}
-		return fmt.Errorf("writing extension state: %w", err)
-	}
 	if err := os.Rename(tmp, path); err != nil {
 		_ = os.Remove(tmp)
 		return fmt.Errorf("renaming extension state: %w", err)
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extensions/runner.go` around lines 896 - 916, SaveStateToFile can
race when multiple goroutines write the same tmp path; fix by serializing
persistence: acquire the existing r.saverMu (or a new file-specific mutex on
Runner) at the start of SaveStateToFile and defer its unlock so only one caller
writes/renames/removes the temp file at a time, and also switch to creating a
unique temp file per call (e.g., using os.CreateTemp/os.TempFile in the target
directory) before renaming to the final path to avoid tmp-name collisions;
update SaveStateToFile to use these changes (referencing SaveStateToFile and
r.saverMu).
🧹 Nitpick comments (1)
internal/extensions/state_test.go (1)

174-193: ⚡ Quick win

Exercise the saver path in the concurrency test.

This only stresses the in-memory map, and every goroutine writes the same "v", so it will not catch a regression in the serialized SaveStateToFile path that this PR is actually hardening. A saver-backed concurrent mutation test would lock that behavior in much better.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/extensions/state_test.go` around lines 174 - 193, The concurrency
test TestRunner_State_ConcurrentSet only exercises the in-memory map; modify it
to exercise the saver-backed path by constructing the Runner with a file-backed
saver (or the existing SaveStateToFile implementation) instead of
NewRunner(nil). Create a temporary file or temp dir, initialize the saver used
by SaveStateToFile, instantiate the runner with that saver (e.g., via
NewRunnerWithSaver or by setting runner.saver/savePath before starting
goroutines), then spawn goroutines that call Runner.SetState("k", value) with
varying values (or at least simultaneous writes) so the saver path is exercised;
clean up the temp file/dir after the test and assert final state as before.
Ensure you reference TestRunner_State_ConcurrentSet, NewRunner / runner.saver /
SaveStateToFile, and Runner.SetState when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/extensions/runner.go`:
- Around line 787-790: The mutex saverMu is locked then unlocked around a call
to an arbitrary callback (stateSaver) in SetState and DeleteState, risking a
permanent lock if the callback panics; change the pattern to unlock with defer
immediately after locking (e.g., lock saverMu, defer unlock) so the mutex is
always released, or extract a small helper (runStateSaver) that acquires
saverMu, defers unlock, and invokes the stateSaver callback to avoid duplication
and ensure safety in both SetState and DeleteState.

---

Outside diff comments:
In `@internal/extensions/runner.go`:
- Around line 896-916: SaveStateToFile can race when multiple goroutines write
the same tmp path; fix by serializing persistence: acquire the existing
r.saverMu (or a new file-specific mutex on Runner) at the start of
SaveStateToFile and defer its unlock so only one caller writes/renames/removes
the temp file at a time, and also switch to creating a unique temp file per call
(e.g., using os.CreateTemp/os.TempFile in the target directory) before renaming
to the final path to avoid tmp-name collisions; update SaveStateToFile to use
these changes (referencing SaveStateToFile and r.saverMu).

---

Nitpick comments:
In `@internal/extensions/state_test.go`:
- Around line 174-193: The concurrency test TestRunner_State_ConcurrentSet only
exercises the in-memory map; modify it to exercise the saver-backed path by
constructing the Runner with a file-backed saver (or the existing
SaveStateToFile implementation) instead of NewRunner(nil). Create a temporary
file or temp dir, initialize the saver used by SaveStateToFile, instantiate the
runner with that saver (e.g., via NewRunnerWithSaver or by setting
runner.saver/savePath before starting goroutines), then spawn goroutines that
call Runner.SetState("k", value) with varying values (or at least simultaneous
writes) so the saver path is exercised; clean up the temp file/dir after the
test and assert final state as before. Ensure you reference
TestRunner_State_ConcurrentSet, NewRunner / runner.saver / SaveStateToFile, and
Runner.SetState when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3ea24ef0-459b-448d-b3c3-871b14fc0b29

📥 Commits

Reviewing files that changed from the base of the PR and between 8b442d2 and 4a7e422.

📒 Files selected for processing (4)
  • internal/extensions/runner.go
  • internal/extensions/state_test.go
  • pkg/kit/extensions_bridge.go
  • pkg/kit/extensions_bridge_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/kit/extensions_bridge_test.go

Comment thread internal/extensions/runner.go Outdated
Extract a runSaver helper that locks saverMu and defers Unlock before
invoking the persistence callback. Without the deferred Unlock, a panic
inside the saver (e.g. disk full mid-write) would leave saverMu held
forever and deadlock the next SetState/DeleteState. Both SetState and
DeleteState now route through the helper. New TestRunner_State_Saver
PanicReleasesSaverMu reproduces the deadlock window with a 2s deadline
and proves the mutex is released after a panic.
@ezynda3 ezynda3 merged commit 49f8b48 into master Jun 9, 2026
3 checks passed
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.

feat: OnLLMUsage event, ctx.SetState/GetState, and enriched AgentEndEvent

1 participant