diff --git a/.cursor/rules/model-skills.mdc b/.cursor/rules/model-skills.mdc index da1cacd..fff2f82 100644 --- a/.cursor/rules/model-skills.mdc +++ b/.cursor/rules/model-skills.mdc @@ -15,8 +15,12 @@ reading now. Read `.cursor/skills/write-model-skill/SKILL.md` before changing on - Routing is three stages — triggers, then a keyword search, then carry-over onto a follow-up — and all three run in code. The catalogue never enters the prompt, so an installed skill costs the model nothing until it routes. -- `keywords` match as contiguous phrases over the words as written. Prefer a phrase to a word, and - write it the way a user would type it. +- `keywords` match as contiguous phrases over the words as written. Prefer a phrase to a word, write + it the way a user would type it, and never use one an ordinary sentence might contain: `work out` + once routed "I work out every morning" to the calculator. A keyword its own triggers already match + is rejected by a test. +- `carry: false` when a skill needs something a follow-up fragment cannot contain, as `summarize-url` + needs a URL. - Only the skill that routed is materialised. Do not reach for `SkillEntry.load()` anywhere routing can see it; that is what makes the library free to grow. - A new or changed skill needs routing cases in `route.test.ts` and a scenario in diff --git a/.cursor/skills/debug-model-output/SKILL.md b/.cursor/skills/debug-model-output/SKILL.md index 9d4515d..c30d78e 100644 --- a/.cursor/skills/debug-model-output/SKILL.md +++ b/.cursor/skills/debug-model-output/SKILL.md @@ -59,7 +59,12 @@ Three things follow from that, and all three are easy to undo by accident: and trains the user to ignore the label. `review.test.ts` pins the shy cases — a clarifying question, a rounded decimal, a source carried over from an earlier turn — and they are the point. - **Only successful tool results become evidence.** A failed fetch has nothing to check against, and - demanding a citation for a page that never loaded is worse than saying nothing. + demanding a citation for a page that never loaded is worse than saying nothing. For the same + reason both citation checks stand down when nothing was fetched at all: asked for a website, the + URL is the answer rather than a source for one. +- **The evidence comes from the real conversation, passed in as `options.evidence`.** The turns the + model is sent start with a skill's exemplars, and letting their URLs count as evidence would excuse + the citation this model is likeliest to get wrong — the one it copied out of the example. `pnpm test` covers all of it: the checks are pure functions and `loop.test.ts` drives the correction round with a scripted client, so none of this needs a GPU. diff --git a/.cursor/skills/write-model-skill/SKILL.md b/.cursor/skills/write-model-skill/SKILL.md index 5c3c569..442ab29 100644 --- a/.cursor/skills/write-model-skill/SKILL.md +++ b/.cursor/skills/write-model-skill/SKILL.md @@ -61,10 +61,18 @@ Call `calculator` for the arithmetic. Do not work it out yourself. spends exactly the capacity the skill exists to conserve. - **`keywords` are the second stage of routing**, searched by `retrieve.ts` when no trigger fires. They match as phrases, over the words as written, so write them the way people write them: - `fasse zusammen` never matches "fasse mir die Seite zusammen". Prefer a phrase to a word — - `temperature outside`, not `temperature`, which also means the one water boils at. This is where a - skill gets reach into German, since every trigger in the library is English. A keyword made only - of stopwords is rejected at load. + `fasse zusammen` never matches "fasse mir die Seite zusammen". A keyword made only of stopwords is + rejected at load, and a test rejects any keyword the skill's own triggers already match — it could + never be reached and it still dilutes the idf of the ones that can. Two rules beyond that: + - **Prefer a phrase to a word.** `temperature outside`, not `temperature`, which also means the one + water boils at. + - **Never a word a sentence contains by accident.** `work out` routed "I work out every morning" to + the calculator; `summary` routed "in summary, the trip was a success" to the page reader; bare + `heute` routed "heute war ein schöner Tag" to the clock. Add the counter-example to the + routes-to-nothing corpus in `route.test.ts` when you are unsure. +- **`carry: false`** stops a skill being carried onto a follow-up. Set it when the skill's job needs + something a fragment cannot contain — `summarize-url` needs a URL, and carried onto "and tomorrow?" + it offers the model a page reader and no page. - **`tools` narrows what the model sees**, because accuracy falls as the visible tool list grows. An empty or absent list means no restriction. Names must match real tools or they are dropped silently. diff --git a/README.md b/README.md index 481f7a8..de40936 100644 --- a/README.md +++ b/README.md @@ -287,6 +287,11 @@ Retrieval is lexical rather than semantic on purpose. RAG-MCP shows semantic ret **What is searched is curated, and that is not a detail.** Retrieving over the `description` is the obvious move and a trap: `temperature` appears in the weather description, so a bag-of-words match fires the weather skill on _what temperature does water boil at_. Keywords are written to be matched instead, as phrases, over the words as written — dropping stopwords first would quietly turn `how warm` into `warm` and fire the weather skill on a bowl of soup. A skill that declares no keywords falls back to its description and needs two terms to match, because prose nobody wrote for a router is weaker evidence. +Two rules keep the index sharp, and both are enforced rather than advised: + +- **A keyword may not be something a trigger already matches.** It could never be reached, and it would still dilute the inverse document frequency of the terms that can — so a redundant keyword does not merely sit there, it makes the index worse. A test walks every shipped skill's keywords past its own triggers. +- **A keyword may not be a word a sentence contains by accident.** This is not hypothetical: `work out` routed _I work out every morning_ to the calculator, `summary` routed _in summary, the trip was a success_ to the page reader, bare `heute` routed _heute war ein schöner Tag_ to the clock, and `rechne` routed _ich rechne damit, dass es klappt_ to the calculator. All four are now in the corpus of messages that must route to nothing. + ### Removing what is not needed A skill that keeps applying to turns it has nothing to do with is worse than no skill: it spends context and narrows the tool list on a request that needed neither. So carry-over is deliberately hard to enter and easy to leave. @@ -294,6 +299,7 @@ A skill that keeps applying to turns it has nothing to do with is worse than no - A continuation has to either **say so** (`and`, `und`, `what about`) or be **too short to be asking anything of its own**. Length alone is not enough, and this is where the mechanism would turn harmful: _what is the capital of France?_ is six words, and answering it with the weather skill's exemplars resident would send the model searching for a fact it already knows. - It survives **two turns** on carry-over alone. Past that it has stopped being a continuation and become a default. - It is dropped the moment another skill matches, the message asks something fresh, the turn closes the exchange (_thanks_), or a new chat starts. +- **A skill can refuse to be carried at all** with `carry: false`. `summarize-url` does, because its job is a URL the follow-up does not contain: carried onto _and tomorrow?_ it would hand the model a page reader and no page. The resident skill is read back off the transcript rather than kept in a counter of its own, so rerunning a reply rewinds it too — a counter held to one side would still be carrying the turn it just discarded. @@ -317,6 +323,8 @@ A failed check costs one further generation. The model is handed its own draft a They are also deliberately shy. A clarifying question is asked for no citation; a long decimal quoted to fewer places counts as the calculator's number; citing the site when a page on it was read is close enough; a URL from an earlier reply is not an invention. Every check would rather miss a mistake than invent one, because a check that fires on a correct answer costs a generation and teaches you to ignore the whole mechanism. +Both citation checks need something to have actually been fetched. Without that, a URL in an answer is not a citation at all — asked _what is Anthropic's website_, the URL **is** the answer — so neither its presence nor its absence says anything went wrong. What the checks compare against is the real conversation rather than the turns the model was sent: those begin with a skill's worked examples, and repeating an exemplar's URL instead of the one that came back is precisely the mistake a 0.8B model makes. + The interface says what happened rather than quietly rewriting the reply. While the corrected answer streams in it is labelled with what is being fixed, and afterwards it carries `corrected` — claimed only for an answer that now passes every check — or `flagged`, naming what is still wrong with the text on screen. An answer half fixed and advertised as corrected would be worse than no check at all. ## Measuring changes diff --git a/src/agent/loop.test.ts b/src/agent/loop.test.ts index 3998e64..3c681a9 100644 --- a/src/agent/loop.test.ts +++ b/src/agent/loop.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { LlmClient } from '@/llm/client' import { MAX_TOOL_ROUNDS } from '@/llm/config' import { defineTool } from '@/tools/types' +import { collectEvidence } from './review' import { runAgent } from './loop' import type { AgentCallbacks } from './loop' @@ -230,6 +231,30 @@ describe('checking the answer before returning it', () => { expect(hooks.onCorrection).toHaveBeenCalledWith(['missing-source']) }) + it('treats a URL copied out of a worked example as an invention', async () => { + // The turns a skill composes begin with its exemplars, and repeating their + // URLs instead of citing what was fetched is a failure a model this size + // makes. So the evidence comes from the real conversation, passed in. + const exemplar = 'Ama Osei.\n\nSource: https://exemplar.example/leadership' + const composed = [ + { role: 'user' as const, content: 'Who is the chief executive of Fictional Airways?' }, + { role: 'assistant' as const, content: exemplar }, + ...turns, + ] + const client = fakeClient([ + toolCall('web_search', 'query', 'Fictional Airways chief executive'), + 'copying the exampleAma Osei.\n\nSource: https://exemplar.example/leadership', + 'using what came backAma Osei.\n\nSource: https://fictionalairways.example/leadership', + ]) + + const result = await runAgent(client, composed, [search], callbacks(), { + evidence: collectEvidence(turns), + }) + + expect(result.review).toEqual({ found: ['invented-source'], corrected: true }) + expect(result.content).toContain('https://fictionalairways.example/leadership') + }) + it('can be switched off so the eval can measure what it is worth', async () => { const client = fakeClient([ toolCall('web_search', 'query', 'Fictional Airways chief executive'), diff --git a/src/agent/loop.ts b/src/agent/loop.ts index e797790..38d1279 100644 --- a/src/agent/loop.ts +++ b/src/agent/loop.ts @@ -9,6 +9,7 @@ import { correctionPrompt, reviewAnswer, type ReviewCheck, + type ReviewEvidence, type ReviewOutcome, } from './review' @@ -39,6 +40,14 @@ export interface AgentOptions { strategy?: GenerationStrategy /** Check the answer before returning it. On unless the eval turns it off. */ review?: boolean + /** + * What the answer may be checked against, gathered from the real conversation. + * + * Worth passing: `turns` starts with a skill's worked examples, and letting an + * exemplar's URLs count as evidence excuses the citation a small model is most + * likely to get wrong — the one it copied out of the example. + */ + evidence?: ReviewEvidence } /** @@ -104,7 +113,7 @@ export async function runAgent( const conversation = [...turns] const strategy = options.strategy ?? DEFAULT_STRATEGY const checking = options.review ?? true - const evidence = collectEvidence(turns) + const evidence = options.evidence ?? collectEvidence(turns) let last: AgentResult = { content: '', diff --git a/src/agent/review.test.ts b/src/agent/review.test.ts index b7c6654..ec7e374 100644 --- a/src/agent/review.test.ts +++ b/src/agent/review.test.ts @@ -51,6 +51,21 @@ describe('reviewAnswer', () => { expect(checks('About 0.333.', third)).toEqual([]) }) + it('accepts a negative written with the minus sign prose uses', () => { + const owed = evidence({ toolResults: [{ tool: 'calculator', result: '3 - 8 = -5' }] }) + + // U+2212, which a model writing prose reaches for and `Number` never does. + expect(checks('The result is \u22125.', owed)).toEqual([]) + }) + + it('checks nothing when the value only exists in exponent form', () => { + const huge = evidence({ toolResults: [{ tool: 'calculator', result: '10 ^ 21 = 1e+21' }] }) + + // `String` and `toFixed` both keep the `e`, so an answer that writes the + // number out in full would read as the wrong number. + expect(checks('That is 1,000,000,000,000,000,000,000.', huge)).toEqual([]) + }) + it('leaves a failed calculation alone', () => { const failed = evidence({ toolResults: [ @@ -132,6 +147,12 @@ describe('reviewAnswer', () => { expect(checks('It is 2026.', timed)).toEqual([]) }) + it('does not treat a URL as a citation when nothing was fetched', () => { + // *What is Anthropic's website* is answered with a URL, and that URL is + // the answer rather than a source for one. + expect(checks('It is https://anthropic.com.', evidence())).toEqual([]) + }) + it('accepts a URL the user supplied but no tool returned', () => { const failed = evidence({ knownUrls: ['https://example.com/pricing'], @@ -173,13 +194,21 @@ describe('collectEvidence', () => { { role: 'tool', content: 'https://exemplar.example/page' }, ]) - // The system turn and the exemplar tool turns are not evidence: a URL from a - // worked example is exactly the kind of thing the model should not cite. + // The system turn and any tool turn already in the history are not evidence. expect(collected).toEqual({ toolResults: [], knownUrls: ['https://example.com/pricing', 'https://example.com/old'], }) }) + + it('carries over the URLs an earlier reply cited', () => { + const history = [ + { role: 'user' as const, content: 'Who runs it?' }, + { role: 'assistant' as const, content: 'Ama Osei.\n\nSource: https://fictionalairways.example' }, + ] + + expect(collectEvidence(history).knownUrls).toEqual(['https://fictionalairways.example']) + }) }) describe('correctionPrompt', () => { diff --git a/src/agent/review.ts b/src/agent/review.ts index 1e8d4fe..1919637 100644 --- a/src/agent/review.ts +++ b/src/agent/review.ts @@ -48,11 +48,18 @@ function findUrls(text: string): string[] { return [...text.matchAll(URL_IN_TEXT)].map((match) => match[0].replace(TRAILING_PUNCTUATION, '')) } -/** Evidence as it stands before the first tool has run. */ -export function collectEvidence(turns: ChatTurn[]): ReviewEvidence { +/** + * Evidence as it stands before the first tool has run. + * + * Hand this the real conversation, not the turns the model is about to be sent: + * those begin with a skill's worked examples, and an exemplar's URLs are exactly + * what a model this size is most likely to repeat instead of citing what it + * fetched. A URL that only ever appeared in an example is an invention. + */ +export function collectEvidence(history: ChatTurn[]): ReviewEvidence { return { toolResults: [], - knownUrls: turns + knownUrls: history .filter((turn) => turn.role === 'user' || turn.role === 'assistant') .flatMap((turn) => findUrls(turn.content)), } @@ -104,6 +111,9 @@ function calculations(evidence: ReviewEvidence): { expression: string; value: nu /** Model output puts thousands separators in unpredictable places. */ const SEPARATORS = /[,\s_'’]/g +/** A model writing prose reaches for the typographic minus, `Number` does not. */ +const MINUS = /[\u2212\u2012\u2013\u2014]/g + /** * Every rendering of a number the answer may use. * @@ -114,7 +124,10 @@ const SEPARATORS = /[,\s_'’]/g */ function renderings(value: number): string[] { const exact = String(value) - if (exact.includes('e')) return [exact] + // Exponent form has no plain-decimal rendering to look for — `toFixed` keeps + // the `e` too — so a correct answer written out in full digits would read as + // a wrong one. Nothing to check rather than something to get wrong. + if (exact.includes('e')) return [] const all = new Set([exact]) if (!Number.isInteger(value)) { @@ -124,8 +137,11 @@ function renderings(value: number): string[] { } function statesNumber(answer: string, value: number): boolean { - const digits = answer.replace(SEPARATORS, '') - return renderings(value).some((rendering) => digits.includes(rendering)) + const renderable = renderings(value) + if (renderable.length === 0) return true + + const digits = answer.replace(MINUS, '-').replace(SEPARATORS, '') + return renderable.some((rendering) => digits.includes(rendering)) } /** A question back to the user, and a plain "I could not find it", cite nothing. */ @@ -161,24 +177,29 @@ export function reviewAnswer(answer: string, evidence: ReviewEvidence): ReviewFi }) } + // Both citation checks need something to have been fetched. Without that, a + // URL in an answer is not a citation of anything — it is the answer, as in + // *what is Anthropic's website* — and neither its absence nor its presence + // says the model got something wrong. const source = preferredSource(evidence) + if (!source) return findings + const known = [...evidence.knownUrls, ...evidence.toolResults.flatMap(({ result }) => findUrls(result))] .map(locate) .filter((entry): entry is Located => entry !== null) - const invented = findUrls(draft).find((url) => { - const cited = locate(url) - return cited !== null && !isGrounded(cited, known) + const cited = findUrls(draft) + const invented = cited.find((url) => { + const located = locate(url) + return located !== null && !isGrounded(located, known) }) if (invented) { findings.push({ check: 'invented-source', - instruction: source - ? `Nothing returned ${invented}. The source is ${source} — cite that one instead.` - : `Nothing returned ${invented}. Drop that link; no source was fetched.`, + instruction: `Nothing returned ${invented}. The source is ${source} — cite that one instead.`, }) - } else if (source && findUrls(draft).length === 0 && !NOTHING_TO_CITE.test(draft)) { + } else if (cited.length === 0 && !NOTHING_TO_CITE.test(draft)) { findings.push({ check: 'missing-source', instruction: `The answer cites no source. End it with "Source: ${source}".`, diff --git a/src/eval/runner.ts b/src/eval/runner.ts index 7a2cb30..75c2503 100644 --- a/src/eval/runner.ts +++ b/src/eval/runner.ts @@ -1,5 +1,5 @@ import { runAgent } from '@/agent/loop' -import type { ReviewCheck } from '@/agent/review' +import { collectEvidence, type ReviewCheck } from '@/agent/review' import type { LlmClient } from '@/llm/client' import type { GenerationStrategy } from '@/llm/config' import type { ChatTurn } from '@/llm/protocol' @@ -103,7 +103,11 @@ async function runAttempt( onToolEnd: () => {}, onRoundEnd: () => {}, }, - { strategy: activation?.strategy ?? arm.strategy, review: arm.review ?? true }, + { + strategy: activation?.strategy ?? arm.strategy, + review: arm.review ?? true, + evidence: collectEvidence(history(scenario)), + }, ) const names = calls.map((call) => call.name) diff --git a/src/skills/arithmetic/SKILL.md b/src/skills/arithmetic/SKILL.md index 31ab193..8fe30e2 100644 --- a/src/skills/arithmetic/SKILL.md +++ b/src/skills/arithmetic/SKILL.md @@ -5,13 +5,12 @@ jarvis: priority: 30 tools: - calculator + # Only what the triggers miss, and nothing a sentence might contain by + # accident: bare `rechne` also starts "ich rechne damit, dass …". keywords: - - calculate - - work out - how much is - - square root - - percent of - - rechne + - rechne mir + - rechne aus - berechne - wie viel ist - quadratwurzel @@ -21,7 +20,8 @@ jarvis: - '\d+(\.\d+)?\s*(percent|per cent|%)\s*(of|off)' - '\b(times|multiplied by|divided by|plus|minus)\b.*\d' - '\b(square root|sqrt|to the power of|squared|cubed)\b' - - '\b(calculate|work out|compute)\b' + # A number has to be in the message, or this is "I work out every morning". + - '\b(calculate|work out|compute)\b.*\d' exemplars: - user: What is 6748 * 9? steps: diff --git a/src/skills/current-date/SKILL.md b/src/skills/current-date/SKILL.md index c9cc9f4..0916a85 100644 --- a/src/skills/current-date/SKILL.md +++ b/src/skills/current-date/SKILL.md @@ -5,16 +5,14 @@ jarvis: priority: 25 tools: - current_time + # Bare `heute` is in every second German sentence, so it is not here. keywords: - - date today - what day - - current year - - time right now - datum - welcher tag - welches jahr - uhrzeit - - heute + - wie spät triggers: - '\b(today|tonight|right now|at the moment|currently)\b' - "\\bwhat('s| is)? the (date|time|day)\\b" diff --git a/src/skills/load.test.ts b/src/skills/load.test.ts index 40527b4..73a3f0f 100644 --- a/src/skills/load.test.ts +++ b/src/skills/load.test.ts @@ -152,6 +152,21 @@ Body.` expect(parseSkill(source, 'example/SKILL.md').keywords).toEqual(['wetter', 'temperature outside']) }) + it('lets a skill refuse to be carried onto a follow-up', () => { + const source = `--- +name: example +description: A description. +jarvis: + carry: false +--- +Body.` + + expect(parseSkill(source, 'example/SKILL.md').carry).toBe(false) + // Carrying is the useful default: a follow-up usually wants the skill that + // answered the question before it. + expect(parseSkill(MINIMAL, 'example/SKILL.md').carry).toBe(true) + }) + it('rejects a keyword made only of stopwords', () => { const source = `--- name: example @@ -271,6 +286,21 @@ describe('the shipped skills', () => { }, ) + it.each(skills.map((skill) => [skill.name, skill] as const))( + '%s keeps no keyword its own triggers already match', + (_name, skill) => { + // Keywords are the second stage of routing and exist for what the first + // stage misses. One that a trigger already matches can never be reached, + // and it still dilutes the inverse document frequency of the terms that + // can — so it is not merely redundant, it makes the index worse. + for (const keyword of skill.keywords) { + expect( + skill.triggers.filter((trigger) => trigger.test(keyword)).map((trigger) => trigger.source), + ).toEqual([]) + } + }, + ) + it.each(skills.map((skill) => [skill.name, skill] as const))( '%s fits the context budget without being trimmed', (_name, skill) => { diff --git a/src/skills/load.ts b/src/skills/load.ts index 8990b1e..99237e2 100644 --- a/src/skills/load.ts +++ b/src/skills/load.ts @@ -129,6 +129,7 @@ interface SkillMetadata { keywords: string[] triggers: RegExp[] priority: number + carry: boolean jarvis: Record body: string } @@ -160,12 +161,18 @@ function parseMetadata(source: string, path: string): SkillMetadata { throw new Error(`${path}: "priority" must be a number`) } + const carry = jarvis.carry + if (carry !== undefined && typeof carry !== 'boolean') { + throw new Error(`${path}: "carry" must be a boolean`) + } + return { name: requireString(frontmatter.name, path, 'name'), description: requireString(frontmatter.description, path, 'description'), keywords: checkKeywords(stringArray(jarvis.keywords, path, 'keywords'), path), triggers: compileTriggers(stringArray(jarvis.triggers, path, 'triggers'), path), priority: priority ?? 0, + carry: carry ?? true, jarvis, body, } @@ -199,7 +206,7 @@ export function parseSkill(source: string, path: string): Skill { * prompt is kept in code, where it costs the model nothing. */ export function parseSkillEntry(source: string, path: string): SkillEntry { - const { name, description, keywords, triggers, priority } = parseMetadata(source, path) + const { name, description, keywords, triggers, priority, carry } = parseMetadata(source, path) let materialised: Skill | null = null return { @@ -208,6 +215,7 @@ export function parseSkillEntry(source: string, path: string): SkillEntry { keywords, triggers, priority, + carry, load: () => (materialised ??= parseSkill(source, path)), } } diff --git a/src/skills/research-question/SKILL.md b/src/skills/research-question/SKILL.md index 85929bb..c186f76 100644 --- a/src/skills/research-question/SKILL.md +++ b/src/skills/research-question/SKILL.md @@ -8,11 +8,9 @@ jarvis: - read_page keywords: - look it up - - find out - search the web - - who won - suche im netz - - schau nach + - schau im netz - finde heraus - wer hat gewonnen - aktuelle nachrichten @@ -20,7 +18,9 @@ jarvis: - '\b(latest|current|recent|news|today.s)\b' - '\bwho (is|was|are|won)\b' - '\b(20[2-9]\d)\b' - - '\b(look up|search for|find out|google)\b' + # `find out` only as a request. "My sister will find out" is not one. + - '\b(look up|search for|google)\b' + - '(? ({ +interface Index { + entries: Indexed[] + /** How many entries each term appears in, for the idf. */ + documentFrequency: Map +} + +/** + * Built once per catalogue rather than once per message. + * + * The catalogue is a module-level constant, so this is a cache with exactly one + * live key in practice; the WeakMap is what keeps the eval's throwaway + * catalogues from accumulating. + */ +const INDEXES = new WeakMap() + +function index(catalog: SkillEntry[]): Index { + const cached = INDEXES.get(catalog) + if (cached) return cached + + const entries: Indexed[] = catalog.map((entry) => ({ entry, phrases: entry.keywords .map((keyword) => ({ @@ -159,6 +177,16 @@ function index(catalog: SkillEntry[]): Indexed[] { .filter((phrase) => phrase.words.length > 0), fallback: entry.keywords.length === 0 ? [...new Set(contentTerms(entry.description))] : [], })) + + const documentFrequency = new Map() + for (const { phrases, fallback } of entries) { + const terms = new Set([...phrases.flatMap((phrase) => phrase.scored), ...fallback]) + for (const term of terms) documentFrequency.set(term, (documentFrequency.get(term) ?? 0) + 1) + } + + const built = { entries, documentFrequency } + INDEXES.set(catalog, built) + return built } /** @@ -181,14 +209,9 @@ export function search(message: string, catalog: SkillEntry[]): Retrieved[] { if (words.length === 0) return [] const query = new Set(contentTerms(message)) - const indexed = index(catalog) - const documentFrequency = new Map() - for (const { phrases, fallback } of indexed) { - const terms = new Set([...phrases.flatMap((phrase) => phrase.scored), ...fallback]) - for (const term of terms) documentFrequency.set(term, (documentFrequency.get(term) ?? 0) + 1) - } + const { entries, documentFrequency } = index(catalog) - const scored = indexed.map(({ entry, phrases, fallback }) => { + const scored = entries.map(({ entry, phrases, fallback }) => { const hits = phrases .filter((phrase) => phraseMatches(phrase.words, words)) .map((phrase) => ({ @@ -196,7 +219,7 @@ export function search(message: string, catalog: SkillEntry[]): Retrieved[] { // A longer phrase matching is a stronger signal than a shorter one, and // summing its terms says so without a separate length bonus. score: phrase.scored.reduce( - (total, word) => total + inverseFrequency(word, documentFrequency, indexed.length), + (total, word) => total + inverseFrequency(word, documentFrequency, entries.length), 0, ), })) @@ -208,7 +231,7 @@ export function search(message: string, catalog: SkillEntry[]): Retrieved[] { hits.push({ source: term, // Halved: the description was written to be read, not matched. - score: inverseFrequency(term, documentFrequency, indexed.length) / 2, + score: inverseFrequency(term, documentFrequency, entries.length) / 2, }) } } diff --git a/src/skills/route.test.ts b/src/skills/route.test.ts index 5317b46..e816e65 100644 --- a/src/skills/route.test.ts +++ b/src/skills/route.test.ts @@ -106,6 +106,12 @@ describe('routing nothing at all', () => { // Physics, not this afternoon: the word alone must not pull in the weather. 'What temperature does water boil at?', 'Erzähl mir einen Witz', + // Every one of these is a keyword doing something its author did not mean. + 'I work out every morning before breakfast', + 'In summary, the trip was a success', + 'Heute war ein wirklich schöner Tag', + 'Ich rechne damit, dass es morgen klappt', + 'My sister will find out sooner or later', ])('leaves %j to the model', (message) => { // Firing a tool-shaped skill on plain conversation is the failure mode that // makes a small model reach for tools it does not need. @@ -160,6 +166,12 @@ describe('keeping a skill across a follow-up', () => { it('forgets a skill that is no longer installed', () => { expect(routed('and in Lisbon?', { name: 'removed-skill', carried: 0 })).toBeNull() }) + + it('never carries a skill whose job needs something the follow-up lacks', () => { + // `summarize-url` declares carry: false. Carried onto a bare follow-up it + // would offer the model a page reader and no page. + expect(routed('and this one?', { name: 'summarize-url', carried: 0 })).toBeNull() + }) }) describe('isFollowUp', () => { diff --git a/src/skills/route.ts b/src/skills/route.ts index ff942d6..388ae2b 100644 --- a/src/skills/route.ts +++ b/src/skills/route.ts @@ -110,7 +110,7 @@ export function route(message: string, catalog: SkillEntry[], memory: SkillMemor } const resident = memory ? catalog.find((entry) => entry.name === memory.name) : undefined - if (resident && memory && memory.carried < MAX_CARRIED_TURNS && isFollowUp(message)) { + if (resident?.carry && memory && memory.carried < MAX_CARRIED_TURNS && isFollowUp(message)) { return { route: { entry: resident, reason: 'carried-over', matched: [] }, memory: { name: resident.name, carried: memory.carried + 1 }, diff --git a/src/skills/summarize-url/SKILL.md b/src/skills/summarize-url/SKILL.md index d380f21..63517cd 100644 --- a/src/skills/summarize-url/SKILL.md +++ b/src/skills/summarize-url/SKILL.md @@ -3,14 +3,15 @@ name: summarize-url description: Reads a page the user has linked to and summarises it. Use when the message contains a URL, or asks what a page says. jarvis: priority: 20 + # Never carried onto a follow-up: without a URL in the message this skill + # offers the model a page reader and no page. + carry: false tools: - read_page # Written as people write them: a keyword only matches contiguously, so - # "fasse zusammen" would miss "fasse mir die Seite zusammen". + # "fasse zusammen" would miss "fasse mir die Seite zusammen". Bare `summary` + # is not here either — it also begins "in summary, the trip was a success". keywords: - - summarise - - summarize - - summary - read this page - zusammenfassen - zusammenfassung diff --git a/src/skills/types.ts b/src/skills/types.ts index 839aaec..7189ffb 100644 --- a/src/skills/types.ts +++ b/src/skills/types.ts @@ -51,6 +51,15 @@ export interface Skill { exemplars: SkillExemplar[] /** Higher wins when several skills match. */ priority: number + /** + * Whether this skill may be carried onto a follow-up that matches nothing. + * + * Most should be: *and in Lisbon?* wants the skill that answered the question + * before it. Some cannot be, because their whole job depends on something the + * follow-up does not contain — `summarize-url` carried onto *and tomorrow?* + * offers the model a page reader and no page. + */ + carry: boolean /** Optional per-skill reasoning budget override. */ strategy?: StrategyId } @@ -69,6 +78,7 @@ export interface SkillEntry { keywords: string[] triggers: RegExp[] priority: number + carry: boolean /** Materialises the body and exemplars. Memoised, so calling it twice is free. */ load: () => Skill } diff --git a/src/store/chat.ts b/src/store/chat.ts index b2e9d65..50ee827 100644 --- a/src/store/chat.ts +++ b/src/store/chat.ts @@ -1,5 +1,6 @@ import { create } from 'zustand' import { runAgent } from '@/agent/loop' +import { collectEvidence } from '@/agent/review' import { LlmClient } from '@/llm/client' import type { ChatTurn, LoadProgress } from '@/llm/protocol' import { MODEL_ID } from '@/llm/config' @@ -172,10 +173,12 @@ export const useChatStore = create((set, get) => { patch((message) => ({ ...message, skill: { name: skill.name, reason, matched } })) } + const conversation = toHistory(history) + try { const result = await runAgent( getClient(), - composeTurns(toHistory(history), activation), + composeTurns(conversation, activation), activation?.tools ?? get().tools, { onPartial: ({ content, reasoning }) => patch((message) => ({ ...message, content, reasoning })), @@ -204,7 +207,12 @@ export const useChatStore = create((set, get) => { // record what was wrong with it before the tokens start replacing it. onCorrection: (found) => patch((message) => ({ ...message, review: { found, corrected: false } })), }, - activation?.strategy ? { strategy: activation.strategy } : {}, + { + // The real conversation, not the composed turns: a skill's exemplars + // carry URLs, and copying one out of an example is not a citation. + evidence: collectEvidence(conversation), + ...(activation?.strategy ? { strategy: activation.strategy } : {}), + }, ) patch((message) => ({