fix(agent): inject skills index per-turn, not in the frozen snapshot (#46)#48
Merged
Conversation
Session mode snapshots the static system prompt once per session so the cacheable prefix stays stable. The skills index was baked into that snapshot, so a skill added mid-session (e.g. via skill-creator) stayed invisible to the model until /new reset the snapshot — the same staleness bug #41 fixed for memories/reflections. Move the skills index into the per-turn preamble (already an uncached seam carrying live date/time + memory + reflections + plan), gated by a new include_skills flag on build_prompt_sections. The index is now rebuilt fresh every turn from the local skills DB (cheap, like memory), scoped to the active persona's allowlist. Cost is only the block's own tokens on the new turn, not a cache miss on the static prefix.
Adversarial review caught three doc passages that still said the skills index lives in the system prompt, now contradicting the fix: the skills.mdx 'How it works' step, its 'System prompt integration' example, and the architecture.mdx flow diagram. Align all three with per-turn injection.
The per-turn skills index is cheap (summaries only) but, in session mode, the preamble is persisted into the growing history — so re-sending the index every turn accumulated one identical copy per turn. Gate re-injection on a per-session hash of the rendered index: send it on the first turn and whenever it changes (a skill added/edited/removed), skip it otherwise. Between changes the model still sees the prior copy in the replayed history, so freshness (#46) is preserved while unchanged turns cost nothing extra. The hash is cleared on /new and on compaction — both can drop the old copy from view — forcing a fresh re-send. Injection mode and tests pass no session key, so the index is always included there (stateless window, nothing to gate).
Adversarial review of the previous commit's per-session hash gate found two real ways it could leave the model blind to skills: - the REPL's own /clear handler clears history directly, bypassing the hash invalidation in process_message, so the stale hash suppressed the index with no copy left in history; - a concurrent same-chat turn could read the hash set by an in-flight turn before that turn's index-bearing message was persisted, then skip with no visible copy. Both stem from a side cache that must stay in lockstep with every history mutation AND with message persistence. Drop it. Gate instead on ground truth: skip the <available_skills> block only when that exact block is already in the replayed session history. Correct by construction — /new, compaction, persona rebind and concurrent turns that drop or change the block simply won't find it, and the failure direction is a harmless re-send, never a blind turn. Removes the dict, both invalidation hooks and the fixture plumbing; the test now drives the real clear path.
The gate skips the index when the exact block is still in replayed history and re-sends after /new, compaction, or a skill change — not the earlier 'changed since the previous turn' hash framing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #46.
Problem
In session mode the static system prompt is snapshotted once per session so the cacheable prefix stays stable. The skills index was baked into that snapshot, so a skill added mid-session (e.g. via the skill-creator skill) stayed invisible to the model until
/newreset the snapshot — the same staleness bug #41 fixed for memories/reflections.Fix
Mirror the merged PR #43 pattern (memory/reflections → per-turn preamble) for the skills index:
_build_system_promptno longer computes or includes the skills index — it passesskills_index=""and a newinclude_skills=Falsetobuild_prompt_sections. The snapshot is now purely static (identity + tools)._turn_preamblerebuilds the skills index fresh every turn from the local skills DB and injects it as<available_skills>at the head of the user message, scoped to the active persona's allowlist (get_index_block(allow=persona.skills)).build_prompt_sectionsgains aninclude_skillsgate, parallel to the existinginclude_memories/include_reflections.A skill created mid-session is now advertised on the very next turn — no
/new, no snapshot rebuild.Cost
The index is fetched from a local SQLite DB (
skills.get_index_block), cheap to call per turn like memory. The per-turn cost is just the index block's own tokens (on the new, already-uncached user turn), not a cache miss on the static prefix — the cacheable prefix is unchanged across turns.Tests
test_mid_session_skill_visible_next_turn_without_new: asserts the snapshot excludes the skills index, a skill upserted after the snapshot appears in the next turn's preamble, and the snapshot stays frozen (cache intact).Docs
architecture.mdx: static prompt = identity/tools only; skills index + memories injected per-turn (steps 2–3 and the flow diagram).skills.mdx: "How it works" + the prompt-integration section now reflect per-turn injection.Review
Ran a multi-lens adversarial review (correctness, regression/completeness, cost/cache, tests/docs) with per-finding verification. Code change got a clean sign-off (all consumers traced, no missed callers, mirrors #43); the review caught three doc passages that still claimed skills live in the system prompt — all fixed here.