diff --git a/toolbox/mdcode/demo/semantic-model/skill/README.md b/toolbox/mdcode/demo/semantic-model/skill/README.md index 0c5bd017..657b291b 100644 --- a/toolbox/mdcode/demo/semantic-model/skill/README.md +++ b/toolbox/mdcode/demo/semantic-model/skill/README.md @@ -274,7 +274,7 @@ gcloud spanner databases execute-sql "$DATABASE" \ That leaves order 12345 at $165.85 over four line items — including a $30 shipping fee, which is what the first request below is about — 12346 at $18.00, -and 12347 at $200.00. The two extra orders give the lookups something to tell +and 12347 at $200.00. The two extra orders give a query something to tell 12345 apart from. Every number quoted in this README came from a run against exactly this seed. To diff --git a/toolbox/mdcode/demo/semantic-model/skill/catalog/EntryGroups/commerce_demo/commerce.yaml b/toolbox/mdcode/demo/semantic-model/skill/catalog/EntryGroups/commerce_demo/commerce.yaml index fa6cead7..92eaa22d 100644 --- a/toolbox/mdcode/demo/semantic-model/skill/catalog/EntryGroups/commerce_demo/commerce.yaml +++ b/toolbox/mdcode/demo/semantic-model/skill/catalog/EntryGroups/commerce_demo/commerce.yaml @@ -50,10 +50,10 @@ semantic_model: # keeps privately is a rule that can be changed without the people who own # the model finding out. Agents come and go; the model stays. # - # Only what is specific to this business belongs here. How to use a lookup - # tool, and what to do when a write is refused, are properties of the tools - # rather than of commerce, so the derivation supplies those and this does - # not repeat them. + # Only what is specific to this business belongs here. Where a key has to + # come from, and what to do when a write is refused, are properties of the + # tools rather than of commerce, so the derivation supplies those and this + # does not repeat them. ai_context: instructions: >- You are working an internal operations desk for this business, fixing @@ -153,13 +153,12 @@ semantic_model: ai_context: instructions: >- Give the order as its number, the amount in dollars, and a memo - saying why. Look the order up first if you were given a customer - name rather than a number: an Order is identified by its key alone. - Say in the memo what actually went wrong on the order -- a late - delivery, a damaged item, a shipping charge applied in error -- - and, if this credit is part of a larger amount owed, say that and - give the total. A rule reads the memo, so a fact left out of it is - a fact the rule cannot weigh. + saying why: an Order is identified by its key alone. Say in the + memo what actually went wrong on the order -- a late delivery, a + damaged item, a shipping charge applied in error -- and, if this + credit is part of a larger amount owed, say that and give the + total. A rule reads the memo, so a fact left out of it is a fact + the rule cannot weigh. # The rules, settled by a judge reading the attempted call and, where the # rule is about something on record, the record. All four are what diff --git a/toolbox/mdcode/docs/semantic-model/actions.md b/toolbox/mdcode/docs/semantic-model/actions.md index 09605a20..f7763d81 100644 --- a/toolbox/mdcode/docs/semantic-model/actions.md +++ b/toolbox/mdcode/docs/semantic-model/actions.md @@ -29,7 +29,7 @@ graph LR P["a binding profile
tables, columns, executor, target"] RT(["one model, bound
ready to run"]) ST["the store
where a write lands"] - AG["what an agent is handed
write tools, lookup tools, instruction"] + AG["what an agent is handed
write tools, instruction"] M --> RT P --> RT @@ -399,9 +399,8 @@ That rule is the honest shape of a balance check and it is also one nothing here enforces: the balance is a row, a judge is shown the arguments and no more, and a rule it cannot settle it reports as not holding — which under `reject` refuses every call. Declared this way it is a policy on record and a schema constraint -waiting to be written, not a control. Keep reading with that in mind; [a rule -the judge can't settle](#a-rule-the-judge-cant-settle) is where it is worked -through. +waiting to be written, not a control. Keep reading with that in mind; [what a +guard can see](#what-a-guard-can-see) explains why. Whatever dispatches the call is what checks its guards, and it checks every one of them before the call, with the arguments bound and before any transaction @@ -464,8 +463,7 @@ A rule that turns on a stored value is the second. Nothing fetches that value for the judge, so the sentence gets settled against a figure the model supplied itself or refused for want of one. Write the rule down anyway if the model is where your policy lives, and enforce it in your schema — see -[a rule the judge can't settle](#a-rule-the-judge-cant-settle) for what you get -if you don't. +[what a guard can see](#what-a-guard-can-see). Every constraint must state `on_violation`, and any of the three words will do. Omit it and the push fails, because an unmarked constraint would reject, and @@ -482,9 +480,10 @@ reads. Nothing settles a rule about a stored value the call doesn't carry. A judge is given the rule, the action and the arguments, and goes nowhere for anything else, so *the credit must not exceed the total of the order* has no total to -compare against — see -[a rule the judge can't settle](#a-rule-the-judge-cant-settle). Put that rule in -your schema, where the store enforces it inside the transaction. +compare against. The judge is instructed to treat a rule it cannot settle from +the arguments as one that does not hold and to say in its reason what was +missing — a safe failure, not a working check. Put that rule in your schema, +where the store enforces it inside the transaction. Nothing settles a rule about the state a write *leaves behind*, either. "An order's total equals the sum of its lines" has nothing to look at when the guard @@ -525,20 +524,8 @@ consistent: constraint. All five are about wording. Claim no more in the wording than a judge can -settle. - -A judge settles a guard from the call's arguments and nothing else, so a -sentence about stored data is a rule it has no evidence for. The instructions -tell it to refuse in that case and say what's missing, but that instruction -binds a model rather than the runtime, so the rule can come back held instead — -a guard that never fires and never says why. Phrase the condition around the -arguments the call carries, and try every guard against a case it ought to -refuse. - -A rule that does need a stored row — comparing a credit against the order total, -say — has no binding point here at all. Put it in your schema, where the store -enforces it inside the transaction. See -[a rule the judge can't settle](#a-rule-the-judge-cant-settle). +settle: phrase the condition around the arguments the call carries, and put any +rule that depends on a stored row into your schema. ## A credit policy, worked through @@ -549,7 +536,7 @@ rules bear on whether they may: ``` the business rule a breach settled ────────────────────────────────────── ──────── ──────────────────────── - 1 no credit above the order's total escalate judgment, reading a row + 1 no credit above the order's total escalate judgment (policy; schema enforces) 3 the memo names a service failure warn judgment 4 not one credit split to evade review reject judgment ────────────────────────────────────── ──────── ──────────────────────── @@ -638,7 +625,7 @@ else to put them but a policy document nothing links to. the amount argument against a number that lives in the database, and a judge sees the arguments only, so the rule is recorded in the model and enforced in your schema — see -[a rule the judge can't settle](#a-rule-the-judge-cant-settle). Rule 2 is the +[what a guard can see](#what-a-guard-can-see). Rule 2 is the same comparison against a literal rather than a row, and that difference is the whole reason it stays out of the model. Rule 4 reads on the credits already sitting on the order, so it is the same case as rule 1. @@ -954,33 +941,25 @@ model survive that trip and which don't. ## 7. Hand it to an agent -You don't write the tools an agent calls. You point an agent at your model, and -what it can read, what it can change and what gates the change are all derived -from the model. +You don't hand-write the write tools an agent calls. Point `kcmd` or the +runtime library at your bound model, and it derives one **write tool** per +action and one model-level **instruction**. -The **derivation** is the step that turns a bound model into the set an agent is -handed. The library runs it, and `kcmd` calls the library, so a service that -embeds the library hands its agents the same set. It produces a **write -tool** for every action, a **lookup tool** for every entity, and one -**instruction** from your model's `ai_context`. +Entities, relationships, and metrics get no tool here — an agent reads through +its own read surface and calls these write tools to change the store. A +constraint reaches an agent only through an action that names it in `guards`. -Nothing else in your model becomes a tool of its own. A constraint reaches an -agent only through an action that guards on it. Relationships and metrics get no -tool at all, so an agent walks a relationship by looking up each end itself, and -nothing totals anything on its behalf. +### The set an agent is handed (`kcmd agent-tools`) -### The set an agent is handed - -`kcmd agent-tools` prints every tool the derivation produces, with the -instruction they arrive with. It reads your model under the profile you name and -needs the store that profile binds, because what an agent can call depends on -it. The command opens no connection and runs nothing: +`kcmd agent-tools` prints every write tool the derivation produces along with +the model's instruction. It reads your model under the profile you name and +opens no connection: ```bash kcmd agent-tools ``` -For the model built up on this page, that set is: +For the `payments` model built up on this page, that output is: ``` Model 'payments' (payments_eg), profile 'operational': @@ -994,49 +973,24 @@ Model 'payments' (payments_eg), profile 'operational': This call is gated by TransferWithinAvailableBalance: - TransferWithinAvailableBalance: The amount argument of this call must not exceed Account.balance on the source account. That balance is on - record rather than stated in the arguments, so read it before - answering. A transfer cannot move more than the source account holds. - Lower the amount, or choose another account. + record rather than stated in the arguments, and nothing puts it in + front of you. A transfer cannot move more than the source account + holds. Lower the amount, or choose another account. source: integer -- The account the money leaves. target: integer -- The account the money goes to. amount: number -- How much money to move. - lookup find_account (Account) - A customer's money at this bank. - - Returns accountId, name, balance, minimumBalance, status. Every argument - is an exact match and every one is optional; giving none returns the - first rows. This tool cannot join, compare ranges, or total anything. - accountId: integer - name: string - balance: number - minimumBalance: number - status: string -- open, frozen or closed. - - lookup find_transfer (Transfer) - One movement of money between two accounts. - - Returns transferId, amount, debitedId. Every argument is an exact match - and every one is optional; giving none returns the first rows. This tool - cannot join, compare ranges, or total anything. - transferId: string - amount: number - debitedId: integer - instruction: Never move money between two accounts held by the same customer without saying so in your answer. - Never invent an identifier. When you are given a name or a description - instead of one, find it with the lookup tools rather than asking for it - -- that is what they are for, and asking wastes the caller's time. Never - compute a total or a balance yourself; the tools do that. When a tool - reports that a write did not happen, read the reason it gives and repeat - it plainly; if it says a person has to decide, say so and stop, because - you cannot approve it yourself. When a write did happen and the tool - returns warnings, the change landed and a rule still went unmet or - unchecked: report both, because nobody else will. Finish by saying what - you changed. + Never invent an identifier. Never compute a total or a balance yourself; + the tools do that. When a tool reports that a write did not happen, read + the reason it gives and repeat it plainly; if it says a person has to + decide, say so and stop, because you cannot approve it yourself. When a + write did happen and the tool returns warnings, the change landed and a + rule still went unmet or unchecked: report both, because nobody else + will. Finish by saying what you changed. ``` `transfer_funds` is offered, guard and all. The rule it is gated by is in the @@ -1088,16 +1042,6 @@ line of output per key: description: How much to move. money to move. - entities: - - name: Account ───▶ lookup find_account - description: A customer's… ───▶ A customer's money at this bank. - fields: - - name: accountId - datatype: Integer ───▶ accountId: integer - - name: status - description: open,… ───▶ status: string -- open, frozen - or closed. - ai_context: instructions: Never move… ───▶ instruction: Never move money between two @@ -1106,8 +1050,6 @@ line of output per key: the binding profile what the agent is handed ───────────────────────────────── ─────────────────────────────────── deployment_target ───▶ store: // - entities[].source ───▶ the table a lookup reads - fields[].expression ───▶ the column it filters on actions[].executor ───▶ what the write tool runs ``` @@ -1129,16 +1071,16 @@ belongs to the model because an agent carrying the same rule in its own source is a place someone can change that rule without the people who own the model finding out. Agents get replaced when frameworks change; your model doesn't. -The other part is about the tools rather than the business: what a lookup is -for, and what a refused write means. The derivation owes that part, because it -describes a contract this module defines and your model never stated. Write it -into each agent instead and you copy the same paragraph into every adapter, -where it drifts in each one. +The other part is about the tools rather than the business: where a key has to +come from, and what a refused write or a warning means. The derivation owes that +part, because it describes a contract this module defines and your model never +stated. Write it into each agent instead and you copy the same paragraph into +every adapter, where it drifts in each one. So an agent that appends a persona of its own is saying something your model did not. Put it in the model. -### What a write tool and a lookup tool do +### What a write tool does A **write tool** runs the action. Calling `transfer_funds` puts every guard the action names to whatever judge the runtime behind it holds, binds every argument @@ -1155,22 +1097,9 @@ timeout or a 5xx where your store may have applied the write and lost the response, which is reported as unknown rather than as a rollback so a caller does not retry and apply the write twice. -A **lookup tool** reads one entity: exact match on any bound field, combined -with AND, capped at 50 rows. It can't join, compare ranges, aggregate or order. -That's enough to turn `"Alice Checking"` into the account id your write tool -needs, and it keeps the generated SQL checkable by eye. Table and column names -come from your binding and every filter value is a bound parameter, so no caller -text reaches the SQL. - -A lookup is named for its entity, and an action keeps its own name when the two -collide. An entity named `Account` and an action named `FindAccount` both derive -`find_account`. The action takes that name, because you wrote it, and the lookup -becomes `lookup_account`. Deriving the write tools and the lookups together is -what makes the collision visible at all. - ### What a withheld tool is waiting on -A **write tool** is withheld for one of three reasons: +A **write tool** is withheld for one of four reasons: - This binding supplies no executor, because the model declared none or a profile withdrew it with `executor: null`. @@ -1178,25 +1107,19 @@ A **write tool** is withheld for one of three reasons: to perform the write. - It names a guard this runtime cannot settle — a judgment with no judge to ask, or one with no words in it. +- The runtime has no store, because a call needs somewhere to land. How an entity is keyed is not among them. Every parameter is a scalar, so an action taking the three key fields of a three-part key is as callable as one taking a single id. -A **lookup** is withheld for reasons of its own: - -- The entity is abstract, so it groups its subtypes and has no table to read. -- No field of it is bound to a plain column, so there is nothing to select. -- Its `source` is empty, or reads as a query rather than a table. - -Either kind is withheld when the runtime has no store, because a call needs -somewhere to land. The derivation asks the runtime for every one of these -verdicts instead of working them out again, so the two can't drift: a tool -advertised as runnable that refuses each call spends your agent's turn, and one -withheld that would have worked is never tried. (`kcmd agent-tools` on the -command line lists a guarded action as runnable even so, because a command-line -listing describes the model rather than a particular caller's judge; -`modelTools({runtime})` in code checks the `judge` on the runtime you hand in.) +The derivation asks the runtime for every one of these verdicts instead of +working them out again, so the two can't drift: a tool advertised as runnable +that refuses each call spends your agent's turn, and one withheld that would +have worked is never tried. (`kcmd agent-tools` on the command line lists a +guarded action as runnable even so, because a command-line listing describes +the model rather than a particular caller's judge; `modelTools({runtime})` in +code checks the `judge` on the runtime you hand in.) ### When a rule stops the call @@ -1310,10 +1233,11 @@ means the number 7, bound to `@source` wherever the statement puts it, as the type `Account.accountId` declares. So a caller holding a name rather than an id has to turn one into the other -first, with a lookup tool (`find_account`) before the call. That's deliberate: -finding the right row can be a search with several plausible answers, and the -place to settle which one is in front of whoever is asking — not inside a write -transaction, which would have to pick one silently and commit to it. +first, by reading the store or asking for the key before the call. That's +deliberate: finding the right row can be a search with several plausible +answers, and the place to settle which one is in front of whoever is asking — +not inside a write transaction, which would have to pick one silently and commit +to it. **Targeting the write.** Your statement's `WHERE` clause decides how many rows it lands on, and nothing would stop one that hits every dormant account. That is @@ -1347,13 +1271,12 @@ if (!runtime.store) throw new Error(runtime.storeError); const {callable, withheld, instruction} = callableTools(modelTools({runtime})); ``` -`modelTools` returns `{lookups, actions, instruction}` — the three things the -listing printed. `callableTools` then sorts the lookups and the actions into the -ones this binding can serve and the ones it can't, which is a split every -adapter has to make and the same split every time. Each tool carries a -`runnable` flag, and `unavailable` carries the reason. Offer `callable` to your -agent, and report `withheld` instead of hiding it. `actionTools` and -`entityTools` are exported for a caller that wants one kind. +`modelTools` returns `{actions, instruction}` — the two things the listing +printed. `callableTools` then sorts the actions into the ones this binding can +serve and the ones it can't, which is a split every adapter has to make and the +same split every time. Each tool carries a `runnable` flag, and `unavailable` +carries the reason. Offer `callable` to your agent, and report `withheld` +instead of hiding it. Each tool is a name, a description, typed parameters and `invoke(args)`, so binding one to ADK, to LangChain or to an MCP server is a short adapter over @@ -1394,12 +1317,6 @@ all still gets a runtime, with `storeError` saying why. Its tools are still derived, each marked unavailable for that reason, so your agent is told what the model offers and why it can't reach it. -Go through `createSemanticRuntimes` instead of building a client yourself. It -also checks that every entity is bound to a table in the store your profile -targets. Without it, a model could be bound to some other system, and a lookup -derived from that model would read whatever table of that name your target store -happens to hold. - ### The commerce demo, worked through `demo/semantic-model/skill/` runs this against a live operational store, with no diff --git a/toolbox/mdcode/docs/semantic-model/profiles.md b/toolbox/mdcode/docs/semantic-model/profiles.md index 5876a43b..6c89286f 100644 --- a/toolbox/mdcode/docs/semantic-model/profiles.md +++ b/toolbox/mdcode/docs/semantic-model/profiles.md @@ -7,10 +7,10 @@ > names — BigQuery Graph or Spanner Graph — and the logical model to Knowledge > Catalog, which takes it under any profile. > -> An action's statements and an agent's lookups run, against an operational -> store: **Spanner or AlloyDB**. An AlloyDB target runs and does not push, because -> AlloyDB has no property-graph DDL for a push to deploy; a BigQuery target -> pushes and does not run. A Spanner target does both. +> An action's statements run against an operational store: **Spanner or +> AlloyDB**. An AlloyDB target runs and does not push, because AlloyDB has no +> property-graph DDL for a push to deploy; a BigQuery target pushes and does +> not run. A Spanner target does both. > > A profile may still bind an entity to any other store (a lake table, a > partner's schema). `kcmd` merges it and reports its availability, and neither diff --git a/toolbox/mdcode/docs/semantic-model/reference.md b/toolbox/mdcode/docs/semantic-model/reference.md index c7a575de..f7e0353d 100644 --- a/toolbox/mdcode/docs/semantic-model/reference.md +++ b/toolbox/mdcode/docs/semantic-model/reference.md @@ -94,11 +94,11 @@ IFS=/ read -r PROJECT INSTANCE DATABASE <<<"$(kcmd profiles --print-store)" kcmd agent-tools ``` -Prints what an agent would be handed for the models in the scope: one lookup -tool per entity, and one tool per action. Each carries its description, the -calling guidance the model wrote, and one line per parameter giving that field's -own description. Reading the model is all this does — it touches no store and -runs nothing. +Prints what an agent would be handed for the models in the scope: one write +tool per action, and the model's instruction. Each tool carries its description, +the calling guidance the model wrote, and one line per parameter giving that +parameter's own description. Reading the model is all this does — it touches no +store and runs nothing. A tool the runtime cannot call is listed and marked `[NOT RUNNABLE]` rather than dropped, with the reason in its description, so a refusal is visible before any diff --git a/toolbox/mdcode/docs/semantic-model/skills.md b/toolbox/mdcode/docs/semantic-model/skills.md index 0e21c736..1d4d1444 100644 --- a/toolbox/mdcode/docs/semantic-model/skills.md +++ b/toolbox/mdcode/docs/semantic-model/skills.md @@ -284,13 +284,11 @@ when it cannot settle one the model requires. ## What it doesn't generate yet -* **Reads.** A skill describes the writes. The lookups a model derives are a - read path nothing on the command line calls, so rather than pointing an agent - at a tool that isn't there, the skill says where the key has to come from and, - for a Spanner store, gives the `gcloud` line that reads it and the schema to - write against. A model bound to AlloyDB gets neither, so an agent handed a - name under that profile has nothing in the skill telling it how to reach a - key. +* **Reads.** A skill describes the writes. Rather than generating read tools, + the skill says where a key has to come from and, for a Spanner store, gives + the `gcloud` line that reads it and the schema to write against. A model bound + to AlloyDB gets neither, so an agent handed a name under that profile has + nothing in the skill telling it how to reach a key. * **Metrics.** A metric reaches BigQuery as a `MEASURE`; nothing lowers one into a skill. * **A plugin.** An [Agent Plugin](https://agent-plugins.org/) bundles skills with diff --git a/toolbox/mdcode/src/libts/semantic/runtime/agent_tools.ts b/toolbox/mdcode/src/libts/semantic/runtime/agent_tools.ts index 227e7ccb..6cbc7181 100644 --- a/toolbox/mdcode/src/libts/semantic/runtime/agent_tools.ts +++ b/toolbox/mdcode/src/libts/semantic/runtime/agent_tools.ts @@ -1,15 +1,11 @@ /** * Turning a model into agent tools. * - * An agent needs two things from a model: a way to look at what is there, and - * a way to change it. Both are already declared. - * * An action declares everything a write tool needs: a name, a description of * what it does, its typed parameters, the guidance an AI caller should follow - * (`ai_context.instructions`), and the rules that gate it. An entity plus its - * binding profile declares everything a read tool needs: the fields, and the - * table and columns they resolve to. This module reads both out and hands back - * plain descriptions of the tools, each with a function that runs it. + * (`ai_context.instructions`), and the rules that gate it. This module reads + * that out and hands back plain descriptions of the tools, each with a + * function that runs it. * * The description is framework-neutral on purpose. Nothing here imports an * agent framework, so binding these to Google ADK, to LangChain, or to an MCP @@ -32,12 +28,12 @@ * what the model declares. */ -import {boundTable, spannerTable} from '../binding'; +import {boundTable} from '../binding'; import {Action, ActionParameter, Constraint, Entity, fieldBinding, SemanticModel} from '../ir'; -import {dialectFor, SqlDialect} from './dialect'; +import {SqlDialect} from './dialect'; import {Judge} from './judge'; -import {ActionHandler, ActionOutcome, bindScalar, isParameterRequired, runAction, sentence, whyRefusedWithoutRunning,} from './run_action'; +import {ActionHandler, ActionOutcome, isParameterRequired, runAction, sentence, whyRefusedWithoutRunning,} from './run_action'; import {runtimeClient, SemanticRuntime} from './runtime'; @@ -146,16 +142,6 @@ export interface ActionToolOptions { * refused mid-call. */ judge?: Judge; - /** - * Derive the tools as a caller that will not check the guards at all: the - * write happens and every rule the model states goes unenforced. For trying - * a model out where no judge is configured, which is otherwise a model whose - * every guarded action is unofferable. - * - * Passed to the call as well as to the derivation, for the reason `judge` is: - * a tool derived one way and called the other is advertised wrongly. - */ - skipGuards?: boolean; } @@ -187,8 +173,8 @@ function toolFor(action: Action, opts: ActionToolOptions): ActionTool { // model, then the runtime having no store, which is the same sentence on // every tool and says nothing about this one. const model = opts.runtime.model; - const blocked = whyRefusedWithoutRunning( - model, action, handler, opts.judge, opts.skipGuards) ?? + const blocked = + whyRefusedWithoutRunning(model, action, handler, opts.judge) ?? noStore(opts.runtime) ?? undefined; const tool: ActionTool = { name: snakeCase(action.name), @@ -203,7 +189,6 @@ function toolFor(action: Action, opts: ActionToolOptions): ActionTool { args, handler, judge: opts.judge, - skipGuards: opts.skipGuards, }); return describeOutcome(outcome); }, @@ -404,86 +389,14 @@ function joinNames(names: string[]): string { } -// --------------------------------------------------------------------------- -// The read side: one lookup tool per entity. -// --------------------------------------------------------------------------- - -/** - * Rows a lookup tool returned, with the field names they line up with. - * - * Values are strings because that is what the store returns over REST, and - * because a tool result is read by a language model rather than by arithmetic. - */ -export interface EntityRows { - entity: string; - fields: string[]; - /** `null` where the column is NULL: a CAST of NULL is still NULL. */ - rows: Array>; - /** True when the row cap cut the answer short, so a caller can narrow it. */ - truncated: boolean; - /** Set instead of rows when the entity cannot be read, saying why. */ - problem?: string; -} - - -/** One lookup tool, derived from one entity and its binding. */ -export interface EntityTool { - name: string; - entityName: string; - description: string; - /** One optional exact-match filter per bound field. */ - parameters: ToolParameter[]; - /** - * Whether calling this would reach the store, on the same terms as - * `ActionTool.runnable`: false when the answer is already in the model, so - * that an adapter can decline to offer a tool that returns a problem however - * it is called. `invoke` still works and still reports the problem. - */ - runnable: boolean; - /** Why `runnable` is false, in words a caller can report. */ - unavailable?: string; - invoke(args: Record): Promise; -} - - -export interface EntityToolOptions { - /** The model to derive lookups from, and the store they would read. */ - runtime: SemanticRuntime; - /** Most rows one call returns. Defaults to 50. */ - rowLimit?: number; -} - - -const DEFAULT_ROW_LIMIT = 50; - - -/** - * One lookup tool per entity the model declares, in declaration order. - * - * What these tools can express is deliberately narrow: exact match on any - * bound field, combined with AND, capped at `rowLimit` rows. No joins, no - * ranges, no aggregation and no ordering. That is enough for an agent to find - * the object an action needs, which is the job here, and it keeps the derived - * SQL something a reader can check by eye. A model that needs richer questions - * answered wants a metric or a query surface declared in the model, rather - * than a more clever generator over this one. - * - * Table and column names come from the model's binding, never from an - * argument; filter values are bound parameters. So no caller-supplied text - * reaches the SQL text. - */ -export function entityTools(opts: EntityToolOptions): EntityTool[] { - return (opts.runtime.model.entities ?? []) - .map(entity => lookupFor(entity, opts)); -} - - /** Everything a model offers an agent, in one name space. */ export interface ModelTools { - /** One lookup per entity: how the agent finds the object to act on. */ - lookups: EntityTool[]; /** One write per action. */ actions: ActionTool[]; + /** The ones a call would actually reach the store through. */ + callable: ActionTool[]; + /** The rest. Each carries `unavailable`, saying why. */ + withheld: ActionTool[]; /** * What to tell an agent holding these tools: the model's own * `ai_context.instructions` followed by how the tools are meant to be used. @@ -495,48 +408,31 @@ export interface ModelTools { /** * Every tool a model offers, with the names guaranteed distinct. * - * `actionTools` and `entityTools` each name their own tools, and neither can - * see the other -- so a model with an entity `Account` and an action - * `FindAccount` derives two tools called `find_account`. An adapter registering - * both either errors or silently keeps one, and which one it keeps is the - * framework's business rather than the model's. Deriving them together is the - * only place that can be noticed, so it is the place that settles it. - * - * An action's tool name is the author's own -- the action is called that in the - * model, in the catalog and on the command line -- so it keeps it, and a lookup - * that wanted the same name takes its longer form instead. + * Two actions can snake-case alike (`IssueCredit` and `issue-credit`), and + * there is no principled winner between two author names, so the later one is + * numbered. */ -export function modelTools(opts: ActionToolOptions&EntityToolOptions): - ModelTools { +export function modelTools(opts: ActionToolOptions): ModelTools { const actions = actionTools(opts); - const lookups = entityTools(opts); - const taken = new Set(); for (const tool of actions) { - // Two actions can still collide with each other -- `IssueCredit` and - // `issue-credit` snake-case alike -- and there is no principled winner - // between two author names, so the later one is numbered. tool.name = distinct(tool.name, taken); } - for (const tool of lookups) { - tool.name = taken.has(tool.name) ? - distinct(`lookup_${snakeCase(tool.entityName)}`, taken) : - distinct(tool.name, taken); - } - return {lookups, actions, instruction: instructionFor(opts.runtime.model)}; + return { + actions, + callable: actions.filter(tool => tool.runnable), + withheld: actions.filter(tool => !tool.runnable), + instruction: instructionFor(opts.runtime.model), + }; } -/** A tool derived from a model, whichever half it came from. */ -export type DerivedTool = EntityTool|ActionTool; - - /** Derived tools sorted by whether this binding can serve them. */ export interface CallableTools { /** The ones a call would actually reach the store through. */ - callable: DerivedTool[]; + callable: ActionTool[]; /** The rest. Each carries `unavailable`, saying why. */ - withheld: DerivedTool[]; + withheld: ActionTool[]; /** `ModelTools.instruction`, carried through unchanged. */ instruction: string; } @@ -545,22 +441,11 @@ export interface CallableTools { /** * Sort the derived tools into the ones this binding can serve and the ones it * cannot. - * - * Every adapter has to make this split, and it is the same split every time. A - * tool the runtime cannot run is still declared, still published and still - * worth naming -- but offering it as callable spends a turn on a call that - * cannot succeed and teaches the agent nothing it can act on. Lookups and - * actions answer `runnable` on the same terms, so they are sorted together - * rather than twice, and they keep the order `modelTools` gave them. - * - * What to do about `withheld` stays the caller's: print it, log it, refuse to - * start. Dropping it in silence is the one thing this does not make easy. */ export function callableTools(tools: ModelTools): CallableTools { - const derived: DerivedTool[] = [...tools.lookups, ...tools.actions]; return { - callable: derived.filter(tool => tool.runnable), - withheld: derived.filter(tool => !tool.runnable), + callable: tools.callable, + withheld: tools.withheld, instruction: tools.instruction, }; } @@ -574,11 +459,11 @@ export function callableTools(tools: ModelTools): CallableTools { // agent that carries it in its own source is a place the rule can be changed // without anyone who owns the model noticing. // -// The second part is about the tools rather than the business -- what a lookup -// is for, and what a refused write means. That half is owed by whoever derived -// the tools, because it describes a contract this file defines and the model -// never stated. Written into each agent instead, it is the same paragraph -// copied into every adapter, drifting in each one. +// The second part is about the tools rather than the business -- where a key +// has to come from, and what a refused write or a warning means. That half is +// owed by whoever derived the tools, because it describes a contract this file +// defines and the model never stated. Written into each agent instead, it is +// the same paragraph copied into every adapter, drifting in each one. // // So neither half is the agent's to write, and an agent that appends its own // is saying something the model did not. @@ -588,10 +473,8 @@ function instructionFor(model: SemanticModel): string { if (stated) parts.push(stated); parts.push( 'Never invent an identifier. When you are given a name or a ' + - 'description instead of one, find it with the lookup tools rather than ' + - 'asking for it -- that is what they are for, and asking wastes the ' + - 'caller\'s time. Never compute a total or a balance yourself; the ' + - 'tools do that. When a tool reports that a write did not happen, read ' + + 'description where an action wants a key, ask the caller or read the ' + + 'store directly. When a tool reports that a write did not happen, read ' + 'the reason it gives and repeat it plainly; if it says a person has to ' + 'decide, say so and stop, because you cannot approve it yourself. When ' + 'a write did happen and the tool returns warnings, the change landed ' + @@ -610,28 +493,6 @@ function distinct(base: string, taken: Set): string { } -function lookupFor(entity: Entity, opts: EntityToolOptions): EntityTool { - const bound = boundFields(entity); - const unavailable = whyUnreadable(entity, bound) ?? noStore(opts.runtime); - return { - name: `find_${snakeCase(entity.name)}`, - entityName: entity.name, - description: lookupDescription(entity, bound), - runnable: !unavailable, - ...(unavailable ? {unavailable} : {}), - parameters: bound.map(f => ({ - name: f.name, - type: jsonType(f.type), - description: filterDescription(entity, f), - required: false, - })), - async invoke(args: Record): Promise { - return await runLookup(entity, bound, args, opts); - }, - }; -} - - // Why nothing derived from this runtime can be called, or null when it can // be. Both halves of a runtime are needed to make a call: a model says what to // do and a store is where it happens, and a runtime carrying only the first is @@ -642,35 +503,9 @@ function noStore(runtime: SemanticRuntime): string|null { } -// Why no call to this entity's lookup could return rows, or null if one -// could. Every answer is in the model, which is what makes it answerable -// before the tool is offered rather than after a caller has spent a turn on -// it -- the same bargain `ActionTool.runnable` strikes on the write side. -// `runLookup` asks these again where it would read, because that is where the -// failure has to be reported; here they decide whether to bother the caller. -function whyUnreadable(entity: Entity, bound: BoundField[]): string|null { - if (entity.abstract) { - return `${entity.name} is abstract: it groups its subtypes and has no ` + - `table of its own. Look up one of the subtypes instead.`; - } - if (!bound.length) { - return `No field of ${entity.name} is bound to a plain column, so there ` + - `is nothing to read. Push the model with a binding profile.`; - } - const warnings: string[] = []; - spannerTable(entity.dataSource, warnings, `entity '${entity.name}'`); - return warnings.length ? warnings.join('; ') : null; -} - - // A field is readable when the profile bound it to a plain column. One bound -// to an expression is skipped rather than guessed at, and an entity with no -// plain-column fields yields a tool that reports the problem when called. -// -// Exported because a skill's own account of what there is to read has to match -// what the lookup tools will actually return. Two readers deciding separately -// what counts as readable is how a skill comes to name a column no tool serves. -export interface BoundField { +// to an expression is skipped rather than guessed at. +interface BoundField { name: string; type: string; column: string; @@ -679,7 +514,7 @@ export interface BoundField { } -export function boundFields(entity: Entity): BoundField[] { +function boundFields(entity: Entity): BoundField[] { const bound: BoundField[] = []; for (const field of entity.fields) { const expr = (fieldBinding(field) ?? '').trim(); @@ -712,10 +547,9 @@ export interface ReadableEntity { * What there is to read under this runtime: one entry per entity the model * declares, the profile binds to a table, and a statement can name. * - * The same test the lookup tools apply, for the same reason: an abstract - * entity has no table, a field bound to an expression is not a column, and a - * data source that is not a table reference cannot be read from. An entity - * this leaves out is one nothing here can point a reader at. + * An abstract entity has no table, a field bound to an expression is not a + * column, and a data source that is not a table reference cannot be read from. + * An entity this leaves out is one nothing here can point a reader at. */ export function readableEntities( runtime: SemanticRuntime, dialect: SqlDialect): ReadableEntity[] { @@ -732,129 +566,3 @@ export function readableEntities( } return readable; } - - -// What the model says the field holds, then how the filter treats it. The -// first half is the only written-down source for the values a coded field -// accepts, so a caller that does not get it has to guess one and spend a turn -// learning it was wrong. -function filterDescription(entity: Entity, field: BoundField): string { - const match = `Match ${entity.name}.${ - field.name} exactly. Omit to leave it unfiltered.`; - return field.description ? `${field.description} ${match}` : match; -} - - -function lookupDescription(entity: Entity, bound: BoundField[]): string { - const parts: string[] = []; - parts.push(entity.description?.trim() || `Look up ${entity.name} records.`); - if (bound.length) { - parts.push( - `Returns ${bound.map(f => f.name).join(', ')}. Every argument is an ` + - `exact match and every one is optional; giving none returns the ` + - `first rows. This tool cannot join, compare ranges, or total ` + - `anything.`); - } - const instructions = entity.aiContext?.instructions?.trim(); - if (instructions) parts.push(instructions); - return parts.join('\n\n'); -} - - -async function runLookup( - entity: Entity, bound: BoundField[], args: Record, - opts: EntityToolOptions): Promise { - const empty = {entity: entity.name, fields: [], rows: [], truncated: false}; - // Asked of the same function the tool's `unavailable` was asked of, so the - // reason a call reports and the reason the tool advertises are one text. - const unreadable = whyUnreadable(entity, bound); - if (unreadable) return {...empty, problem: unreadable}; - - // Written for whichever store this runtime is bound to. The shape of the - // statement is the same either way; what differs is how a name is quoted - // and how a column is rendered as text. - const dialect = dialectFor(opts.runtime.store); - const warnings: string[] = []; - const table = boundTable( - entity.dataSource, warnings, `entity '${entity.name}'`, dialect.quote); - - // Only field names the model declares reach the SQL, and every value is - // bound. An argument naming an unknown field is a caller error worth - // reporting rather than ignoring. - const predicates: string[] = []; - const params: Record = {}; - const paramTypes: Record = {}; - for (const [name, value] of Object.entries(args)) { - // Absent means unfiltered; empty means the empty string. `bindScalar` - // draws the line in the same place for the write path, and a caller that - // sends '' for an unset filter gets no rows rather than the first fifty - // of the table read back as though they matched. - if (value === undefined || value === null) continue; - const field = bound.find(f => f.name === name); - if (!field) { - return { - ...empty, - problem: `${entity.name} has no readable field '${name}'. It has ${ - bound.map(f => f.name).join(', ')}.`, - }; - } - // Compared as ITSELF, against the input parsed to the type the field - // declares -- the same discipline the write path's parameter binding - // follows, for the same - // reason. `CAST(col AS STRING) = @f` would let one predicate shape serve - // every column type, and no index can answer it: a lookup on a primary key - // would scan the table. - const value_ = bindScalar({name: field.name, type: field.type}, value); - if ('error' in value_) { - return { - ...empty, - problem: - `${value_.error} No ${entity.name} has ${field.name} = '${value}'.`, - }; - } - const bind = `f_${predicates.length}`; - predicates.push(`${dialect.quote(field.column)} = @${bind}`); - params[bind] = value_.value; - paramTypes[bind] = {code: value_.code}; - } - - const limit = opts.rowLimit ?? DEFAULT_ROW_LIMIT; - const columns = - bound.map(f => dialect.castToText(dialect.quote(f.column))).join(', '); - const where = predicates.length ? ` WHERE ${predicates.join(' AND ')}` : ''; - // One row over the cap, so "there are more" can be told from "that is all". - const sql = `SELECT ${columns} FROM ${table}${where} LIMIT ${limit + 1}`; - - // An unbound entity, a bad source and an unknown filter all come back as a - // `problem` the caller can read out. A store that refuses the read -- no - // permission, no such table, no session to be had -- is not a different kind - // of thing, and throwing would reach an adapter as a crashed tool call - // rather than as something the agent can report and work around. - const client = runtimeClient(opts.runtime); - if ('error' in client) return {...empty, problem: client.error}; - - let rows: Array>; - try { - rows = await client.withSession(async sessionName => { - const res = - await client.executeQuery(sessionName, {sql, params, paramTypes}); - if (res.status < 200 || res.status >= 300) { - throw new Error(res.message ?? `${res.status}`); - } - return res.result?.rows ?? []; - }); - } catch (err) { - return { - ...empty, - problem: `Could not read ${entity.name}: ${ - err instanceof Error ? err.message : String(err)}`, - }; - } - - return { - entity: entity.name, - fields: bound.map(f => f.name), - rows: rows.slice(0, limit), - truncated: rows.length > limit, - }; -} diff --git a/toolbox/mdcode/src/libts/semantic/runtime/judge.ts b/toolbox/mdcode/src/libts/semantic/runtime/judge.ts index 90e9eee8..b81e8bad 100644 --- a/toolbox/mdcode/src/libts/semantic/runtime/judge.ts +++ b/toolbox/mdcode/src/libts/semantic/runtime/judge.ts @@ -61,3 +61,15 @@ export interface Judge { readonly name: string; decide(request: JudgeRequest): Promise; } + +/** + * Placeholder judge used when inspecting a model's derived tools (`kcmd + * agent-tools` and `kcmd skills-generate`) without invoking them. + */ +export const ASSUMED_JUDGE: Judge = { + name: 'the judge the runtime supplies', + decide: () => { + throw new Error('static tool derivation must not call a judge'); + }, +}; + diff --git a/toolbox/mdcode/src/libts/semantic/runtime/run_action.ts b/toolbox/mdcode/src/libts/semantic/runtime/run_action.ts index 33aa0e7a..1cec62a0 100644 --- a/toolbox/mdcode/src/libts/semantic/runtime/run_action.ts +++ b/toolbox/mdcode/src/libts/semantic/runtime/run_action.ts @@ -121,18 +121,8 @@ export interface RunActionOptions { handler?: ActionHandler; // Settles the guards this model states in words. Omitting it does not mean // "run those unjudged": an action with a guard to settle is refused, because - // a judgment is the only body a constraint has and only a judge settles one. + // a judgment is the one body a constraint has and only a judge settles one. judge?: Judge; - // Runs the action without checking its guards at all. Not a weaker check -- - // no check: every refusal a guard would have produced is skipped and the - // write happens. It exists because the refusals above are total. An author - // trying a model out locally, against their own database, has no judge to - // supply and would find every guarded action unrunnable; the alternative is - // deleting the guards to test the write, which is worse. The outcome names - // every guard the run passed over, in `warnings`, so nothing that reads the - // outcome -- a command line, or an agent handed the result of a tool call -- - // is ever told the write passed rules nothing consulted. - skipGuards?: boolean; } @@ -158,8 +148,8 @@ export async function runAction(opts: RunActionOptions): } // Decided BEFORE touching the store, so an action this runtime will not run // fails without having opened a transaction at all. - const refusal = whyRefusedWithoutRunning( - model, action, opts.handler, opts.judge, opts.skipGuards); + const refusal = + whyRefusedWithoutRunning(model, action, opts.handler, opts.judge); if (refusal) return {status: 'error', message: refusal}; // Checked before the judge as well. A judge is asked whether a rule holds @@ -183,13 +173,7 @@ export async function runAction(opts: RunActionOptions): // never the state the write produced, which means a rule about the RESULT of // a write is out of reach here and belongs in the schema. const warnings: string[] = []; - // `skipGuards` means nobody is asked -- the whole point of it -- so it - // stands the asking down too, not just the refusal for want of a judge - // and the unsettled-guard warnings. A caller that passed both used to - // reach the judge with those warnings suppressed, so a guard whose - // judgment states nothing, or one that threw while being asked, committed - // with no line about it anywhere. - if (opts.judge && !opts.skipGuards) { + if (opts.judge) { const judged = judgedGuards(model, action); if (judged.length) { // Returned rather than thrown. A throw from here reaches the catch at @@ -204,27 +188,8 @@ export async function runAction(opts: RunActionOptions): // caller shown no line for it reads the write as having passed every rule the // model states. Every one reaching here is advisory, because anything // stricter was refused above. - // - // This travels with the outcome rather than being left to whoever called, - // including under `skipGuards`. A caller that asked for the skip does know it - // asked, but it is not the only one reading the result: `describeOutcome` - // hands these warnings to an agent as the tool's own answer, and an agent - // told only `applied: true` has been told the write met every rule the model - // states, which is the one thing it did not. - // - // One line for the whole skip rather than one per rule. The rules were not - // checked for one reason, and repeating it four times buries the outcome of - // the write under a list that says the same thing each time. - const skipped = skippedGuards(action, opts.skipGuards); - if (skipped.length) { - warnings.push( - `guards were not checked: ${skipped.join(', ')} -- this run was ` + - `told to skip them, and the write was made anyway`); - } else { - for (const {constraint, why} of unsettledGuards( - model, action, opts.judge)) { - warnings.push(`${citation(constraint)} was not checked: ${why}`); - } + for (const {constraint, why} of unsettledGuards(model, action, opts.judge)) { + warnings.push(`${citation(constraint)} was not checked: ${why}`); } // Whether a transaction was ever opened. A session that could not be @@ -490,7 +455,7 @@ const DEFINITELY_NOT_COMMITTED = new Set([400, 401, 403, 404, 409, 412]); */ export function whyRefusedWithoutRunning( model: SemanticModel, action: Action, handler?: ActionHandler, - judge?: Judge, skipGuards?: boolean): string|null { + judge?: Judge): string|null { // No executor at all is a binding outcome, not a broken model: the executor // is a physical facet, so an action can be declared here and performable // only somewhere else. Say which it is, because the fix is in the profile @@ -515,7 +480,7 @@ export function whyRefusedWithoutRunning( // parameter is a scalar and binds as one, so a key with three parts is three // ordinary parameters and there is no shape of key this runtime cannot pass // to a statement. What is left is the guards. - return unsafeToRunUnchecked(model, action, judge, skipGuards); + return unsafeToRunUnchecked(model, action, judge); } @@ -533,8 +498,7 @@ export function whyRefusedWithoutRunning( // does is the author's, which is what `affects` describes and what the // evaluator will check against the statements once it exists. function unsafeToRunUnchecked( - model: SemanticModel, action: Action, judge?: Judge, - skipGuards?: boolean): string|null { + model: SemanticModel, action: Action, judge?: Judge): string|null { // A guard names a constraint the author says is checked before the call. // One whose `onViolation` is `warn` reports rather than refuses, so an // evaluator would let the write through, and refusing here would make a @@ -578,14 +542,7 @@ function unsafeToRunUnchecked( // Refusing it HERE is what keeps this function and `runAction` in agreement: // a tool advertised as runnable and then refused mid-call spends the // caller's turn and teaches it nothing. - // - // `skipGuards` is the caller saying nobody will be asked, so this refusal - // stands down. Only this one: the two above are the model being wrong about - // its own rules -- a guard that quotes nothing, a guard that names nothing -- - // and not asking repairs neither. They are also what a push refuses, so - // standing them down here would make this runtime disagree with the - // validation that gates publishing the model. - if (!skipGuards && guards.length && !judge) { + if (guards.length && !judge) { return `Action '${action.name}' is guarded by ${quoteList(guards)}, ` + `which ${guards.length === 1 ? 'is' : 'are'} settled by reading the ` + `call, and this runtime was given no judge to ask. Running it would ` + @@ -715,15 +672,6 @@ function citation(constraint: Constraint): string { // `unsafeToRunUnchecked` has refused everything stricter, so what turns up // here is advisory: it did not stop the write, and it still has to be // reported rather than left to read as a rule that passed. -// The guards a `skipGuards` run passed over, in the order the action names -// them. Reads the action rather than the model's constraints, because a guard -// naming a constraint that does not exist was refused before this point and a -// run that reaches here names only real ones. -function skippedGuards( - action: Action, skipGuards?: boolean): readonly string[] { - if (!skipGuards) return []; - return action.guards ?? []; -} function unsettledGuards(model: SemanticModel, action: Action, judge?: Judge): diff --git a/toolbox/mdcode/src/libts/semantic/skills.ts b/toolbox/mdcode/src/libts/semantic/skills.ts index 6482629d..f4252234 100644 --- a/toolbox/mdcode/src/libts/semantic/skills.ts +++ b/toolbox/mdcode/src/libts/semantic/skills.ts @@ -45,7 +45,7 @@ import {Action, AffectedConcept, Constraint, SemanticModel} from './ir'; import {ActionTool, modelTools, readableEntities} from './runtime/agent_tools'; import {dialectFor} from './runtime/dialect'; -import {Judge} from './runtime/judge'; +import {ASSUMED_JUDGE} from './runtime/judge'; import {SemanticRuntime} from './runtime/runtime'; import {storeLine} from './runtime/store'; @@ -84,26 +84,6 @@ export interface GenerateSkillOptions { name?: string; } -/** - * Stands in for the judge the runtime supplies. - * - * A rule stated in words is settled by asking a judge, and the runtime asks it - * before the transaction opens -- never the agent making the call, which would - * be the constrained thing certifying itself. So a guarded action only ever - * runs against a runtime that has one, and that is the runtime a skill - * describes. Whether whoever ran `skills-generate` had a judge configured is a - * fact about that invocation and about nothing the document is read against. - * - * `modelTools` asks only whether a judge is there. Generating a skill settles - * no rule, so this throws if anything reaches it. - */ -const ASSUMED_JUDGE: Judge = { - name: 'the judge the runtime supplies', - decide: () => { - throw new Error('generating a skill must not ask a judge'); - }, -}; - /** * The skill for one model, as files a caller writes out. @@ -371,11 +351,9 @@ const KEY_MATCHES_NOTHING = // What this skill does not offer, said once rather than discovered per call. // -// The instruction above tells an agent to look a record up rather than invent -// an identifier, which is right, and this skill has no way to do it: the -// derived lookups are a read path nothing on the command line calls. Leaving -// that out would leave an agent following an instruction to use a tool that is -// not here, so it is named, along with what does work. +// The instruction above tells an agent never to invent an identifier, and this +// skill offers only the write side -- so where a key has to come from is named +// here, along with how to read the store directly when one is bound. function readSideSection( runtime: SemanticRuntime, actions: ActionTool[]): string[] { // Every sentence below is about supplying a key to an action, so a model @@ -388,9 +366,9 @@ function readSideSection( out.push(''); // The last sentence is a promise about how a write fails, and only the // `sql` kind is executed by this runtime and can be promised. It is dropped - // rather than the section with it: the instruction above still sends an - // agent to lookup tools this skill does not have, and saying where a key - // comes from is the answer to that whichever kind performs the write. + // rather than the section with it: the instruction above still tells an + // agent never to invent an identifier, and saying where a key comes from is + // the answer to that whichever kind performs the write. const performedHere = distinctKinds(runtime.model.actions ?? []).includes('sql'); out.push( @@ -422,8 +400,7 @@ function readSideSection( // INFORMATION_SCHEMA -- which it did, twice, before reading a row. The model // already holds the answer: the binding profile says which table each entity // is and which column each field is, and `readableEntities` derives both from -// the same place the lookup tools do, so what this page says is readable and -// what a tool will actually serve cannot drift apart. +// the profile. // // Both names appear, and which is which is spelled out rather than implied. // The rest of the skill is written in the model's names and a statement has to diff --git a/toolbox/mdcode/src/tool/commands.ts b/toolbox/mdcode/src/tool/commands.ts index 981711e7..70ad5edc 100644 --- a/toolbox/mdcode/src/tool/commands.ts +++ b/toolbox/mdcode/src/tool/commands.ts @@ -20,7 +20,8 @@ import {LoadedModel, loadSemanticModels} from '../libts/semantic/loader'; import {serializeModel} from '../libts/semantic/osi_converter'; import {pullKnowledgeCatalog} from '../libts/semantic/pull_kc'; import {AvailabilityReport, DEFAULT_PROFILE, mergeProfileOntoDoc, pruneUnavailable,} from '../libts/semantic/resolve_profiles'; -import {ActionTool, EntityTool, modelTools} from '../libts/semantic/runtime/agent_tools'; +import {ActionTool, modelTools} from '../libts/semantic/runtime/agent_tools'; +import {ASSUMED_JUDGE} from '../libts/semantic/runtime/judge'; import {createSemanticRuntimes} from '../libts/semantic/runtime/runtime'; import {dataClientFor, storeLine} from '../libts/semantic/runtime/store'; import {generateSkill, SkillPackage} from '../libts/semantic/skills'; @@ -1279,16 +1280,14 @@ export interface AgentOptions { // // kcmd agent-tools // -// Two halves and an instruction, all three derived: one lookup per entity, one -// write per action, and what to tell the agent about using them. Nothing here -// is written for a particular agent, which is the property worth being able to +// One write tool per action and an instruction, both derived. Nothing here is +// written for a particular agent, which is the property worth being able to // see -- the listing is the same whether the caller is ADK, LangChain or a // person reading it to decide whether the model says enough. // // A tool the runtime cannot run today is listed and marked rather than // dropped. The model declares it; what it is waiting on is the useful thing to -// print. The read half is a SELECT the tool would issue, and `gcloud spanner -// databases execute-sql` will run it. +// print. // // A guard is not what decides whether a tool is listed. Who settles one // belongs to the application that embeds the runtime, and this command cannot @@ -1335,10 +1334,9 @@ export async function agentTools(options: AgentOptions = {}): Promise { console.log(` store: ${storeLine(store)}`); console.log(); - const {lookups, actions, instruction} = - modelTools({runtime, skipGuards: true}); + const {actions, instruction} = + modelTools({runtime, judge: ASSUMED_JUDGE}); for (const tool of actions) printActionTool(tool); - for (const tool of lookups) printLookupTool(tool); console.log(' instruction:'); console.log(indentBlock(instruction)); console.log(); @@ -1506,37 +1504,6 @@ function printActionTool(tool: ActionTool): void { } -function printLookupTool(tool: EntityTool): void { - console.log(` lookup ${tool.name} (${tool.entityName})${ - tool.runnable ? '' : ' [NOT READABLE]'}`); - console.log(indentBlock(tool.description)); - // One line per filter, like an action's parameters: the point of this - // command is that it shows what the agent gets, and a bare list of names - // hides the half of it the model wrote. - for (const p of tool.parameters) { - const said = describedPart(p.description); - console.log(wrapTo( - `${p.name}: ${p.type}${said ? ` -- ${said}` : ''}`, BODY_INDENT, - PARAM_CONTINUATION)); - } - if (!tool.runnable) { - console.log(indentBlock(`NOT READABLE: ${tool.unavailable}`)); - } - console.log(); -} - - -// The model's half of a filter description, without the sentence the -// derivation appends to every one of them. Printing that sentence once per -// filter would bury what is actually worth reading. -function describedPart(description: string): string { - // From the end: the derivation appends its sentence last, and a model is - // free to use the word in its own. - const boilerplate = description.lastIndexOf('Match '); - return boilerplate <= 0 ? '' : description.slice(0, boilerplate).trim(); -} - - // A description or an instruction, indented under the line that introduces it. // Both arrive as prose the model's author wrote and wrapped where they liked, // so each line is re-indented rather than the block as a whole. diff --git a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.mcp.skill.golden.md b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.mcp.skill.golden.md index 66e75831..ed4cb35d 100644 --- a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.mcp.skill.golden.md +++ b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.mcp.skill.golden.md @@ -17,7 +17,7 @@ Each action below has a reference page with the arguments it takes, the rules th ## How this model wants to be used -Never invent an identifier. When you are given a name or a description instead of one, find it with the lookup tools rather than asking for it -- that is what they are for, and asking wastes the caller's time. Never compute a total or a balance yourself; the tools do that. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. +Never invent an identifier. When you are given a name or a description where an action wants a key, ask the caller or read the store directly. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. ## Finding a record diff --git a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_bound.skill.golden.md b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_bound.skill.golden.md index f54c4515..8c929c41 100644 --- a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_bound.skill.golden.md +++ b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_bound.skill.golden.md @@ -17,7 +17,7 @@ Each action below has a reference page with the arguments it takes, the rules th ## How this model wants to be used -Never invent an identifier. When you are given a name or a description instead of one, find it with the lookup tools rather than asking for it -- that is what they are for, and asking wastes the caller's time. Never compute a total or a balance yourself; the tools do that. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. +Never invent an identifier. When you are given a name or a description where an action wants a key, ask the caller or read the store directly. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. ## Finding a record diff --git a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_unbound.skill.golden.md b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_unbound.skill.golden.md index d6c28ed7..eb1e7c33 100644 --- a/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_unbound.skill.golden.md +++ b/toolbox/mdcode/tests/libts/semantic/fixtures/actions_place_order.sql_unbound.skill.golden.md @@ -17,7 +17,7 @@ Each action below has a reference page with the arguments it takes, the rules th ## How this model wants to be used -Never invent an identifier. When you are given a name or a description instead of one, find it with the lookup tools rather than asking for it -- that is what they are for, and asking wastes the caller's time. Never compute a total or a balance yourself; the tools do that. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. +Never invent an identifier. When you are given a name or a description where an action wants a key, ask the caller or read the store directly. When a tool reports that a write did not happen, read the reason it gives and repeat it plainly; if it says a person has to decide, say so and stop, because you cannot approve it yourself. When a write did happen and the tool returns warnings, the change landed and a rule still went unmet or unchecked: report both, because nobody else will. Finish by saying what you changed. ## Finding a record diff --git a/toolbox/mdcode/tests/libts/semantic/runtime/agent_tools.test.ts b/toolbox/mdcode/tests/libts/semantic/runtime/agent_tools.test.ts index 32a3171b..90754c3b 100644 --- a/toolbox/mdcode/tests/libts/semantic/runtime/agent_tools.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/runtime/agent_tools.test.ts @@ -16,7 +16,8 @@ import * as path from 'node:path'; import * as spanner from '../../../../src/libts/gcp/spanner'; import {Action, Constraint, Entity, SemanticModel} from '../../../../src/libts/semantic/ir'; import {loadModels} from '../../../../src/libts/semantic/loader'; -import {actionTools, callableTools, describeOutcome, entityTools, modelTools} from '../../../../src/libts/semantic/runtime/agent_tools'; +import {actionTools, callableTools, describeOutcome, modelTools, readableEntities} from '../../../../src/libts/semantic/runtime/agent_tools'; +import {dialectFor} from '../../../../src/libts/semantic/runtime/dialect'; import {Judge} from '../../../../src/libts/semantic/runtime/judge'; import {SemanticRuntime} from '../../../../src/libts/semantic/runtime/runtime'; @@ -54,14 +55,8 @@ function rt( class FakeStore { readonly database = 'projects/p/instances/i/databases/d'; readonly statements: spanner.Statement[] = []; - queryStatus = 200; - queryMessage: string|undefined = undefined; - sessionThrows = false; async withSession(fn: (s: string) => Promise): Promise { - if (this.sessionThrows) { - throw new Error('could not create a session on d (403).'); - } return await fn('sessions/1'); } async beginReadWrite() { @@ -73,9 +68,6 @@ class FakeStore { } async executeQuery(_s: string, stmt: spanner.Statement) { this.statements.push(stmt); - if (this.queryStatus !== 200) { - return {status: this.queryStatus, message: this.queryMessage}; - } return {status: 200, result: {rows: []}}; } async commit() { @@ -517,49 +509,6 @@ describe('what a tool says it is gated by', () => { }); -// The read side owes the same answer the write side owes, for the same reason. -describe('a lookup that could not return a row says so up front', () => { - const model = loadFixtureModel('actions_place_order.yaml'); - - function lookupFor(entities: Entity[], name: string) { - return entityTools({runtime: rt({...model, entities})}) - .find(t => t.entityName === name)!; - } - - test('a bound entity is runnable', () => { - const tool = lookupFor(model.entities, 'customer'); - expect(tool.runnable).toBe(true); - expect(tool.unavailable).toBeUndefined(); - }); - - test('an abstract entity has no table to read', () => { - const entities = model.entities.map( - e => e.name === 'customer' ? {...e, abstract: true} : e); - const tool = lookupFor(entities, 'customer'); - expect(tool.runnable).toBe(false); - expect(tool.unavailable).toContain('abstract'); - }); - - test('an entity with no field bound to a column has nothing to read', () => { - const entities = model.entities.map( - e => e.name === 'customer' ? - {...e, fields: e.fields.map(f => ({...f, expression: undefined}))} : - e); - const tool = lookupFor(entities, 'customer'); - expect(tool.runnable).toBe(false); - expect(tool.unavailable).toContain('binding profile'); - }); - - test( - 'the reason a call reports is the reason the tool advertised', - async () => { - const entities = model.entities.map( - e => e.name === 'customer' ? {...e, abstract: true} : e); - const tool = lookupFor(entities, 'customer'); - const rows = await tool.invoke({}); - expect(rows.problem).toBe(tool.unavailable); - }); -}); // A `sql` executor's claim is that what runs is what the catalog published. A @@ -601,241 +550,9 @@ describe('a handler does not displace an action\'s own statements', () => { }); -// A tool derived with `skipGuards` is the only way a guarded action is offered -// as callable at all, so what it says when it commits is the whole of what the -// agent learns about the rules. -describe('a skipped guard reaches the agent, not just the caller', () => { - const model = loadFixtureModel('actions_place_order.yaml'); - - test('the tool result names the guard the run passed over', async () => { - // The run used to come back `applied: true` and nothing else. The - // suppression was justified by the caller already knowing it asked for the - // skip -- true of the caller, and irrelevant to the agent reading the - // tool's result, which never saw the call that built the tool. An agent - // told only that the write applied has been told it met every rule the - // model states. - const store = new FakeStore(); - const [tool] = actionTools({ - runtime: - rt(withExecutor(model, {executor: RUNNABLE.executor}), store.client), - skipGuards: true, - }); - expect(tool.runnable).toBe(true); - const result = await tool.invoke({customer: 1, quantity: 2}); - expect(result.applied).toBe(true); - expect(result.warnings ?? []).toHaveLength(1); - expect((result.warnings ?? [])[0]) - .toContain('guards were not checked: OrderWithinCustomerCredit'); - }); -}); - - -describe('entity tools', () => { - const model = loadFixtureModel('actions_place_order.yaml'); - const tools = entityTools({runtime: rt(model)}); - - test('one lookup tool per entity', () => { - expect(tools.map(t => t.name)) - .toEqual(model.entities.map( - e => `find_${ - e.name.replace(/([a-z0-9])([A-Z])/g, '$1_$2').toLowerCase()}`)); - }); - - test('the filters are the fields the profile bound to columns', () => { - const orders = tools.find(t => t.entityName === 'orders')!; - expect(orders.parameters.map(p => p.name)) - .toEqual(model.entities.find(e => e.name === 'orders')!.fields.map( - f => f.name)); - }); - - test('every filter is optional', () => { - // Giving none returns the first rows, which is how an agent starts looking. - for (const tool of tools) { - expect(tool.parameters.every(p => !p.required)).toBe(true); - } - }); - - test('the description states what the tool cannot do', () => { - // An agent that knows the limits asks a question the tool can answer. - const orders = tools.find(t => t.entityName === 'orders')!; - expect(orders.description).toContain('exact match'); - expect(orders.description).toContain('cannot join'); - }); -}); - - -// A coded field's allowed values are written in exactly one place -- the -// field's description -- and a caller that does not get them has to guess one. -// The action side already passes a parameter's description through; this is the -// read side agreeing. -describe('what a lookup filter says it matches', () => { - const model = loadModels(`version: "0.2.0.dev0/google" -semantic_model: - - name: m - entities: - - name: LineItem - primary_key: [lineItemId] - source: //spanner.googleapis.com/projects/p/instances/i/databases/d/tables/LineItem - fields: - - {name: lineItemId, datatype: String, expression: line_item_id} - - name: type - datatype: String - description: item, tax, fee, or credit. - expression: type - - {name: amount, datatype: Decimal, expression: amount} -`).models[0]; - const [lineItem] = entityTools({runtime: rt(model)}); - - test('the field\'s own description leads', () => { - const type = lineItem.parameters.find(p => p.name === 'type')!; - expect(type.description.startsWith('item, tax, fee, or credit.')) - .toBe(true); - }); - - test('how the filter behaves is still said, after it', () => { - // The two halves are owed by different authors: what the field holds is - // the model's, that the match is exact is the derivation's. - const type = lineItem.parameters.find(p => p.name === 'type')!; - expect(type.description).toContain('Match LineItem.type exactly'); - expect(type.description).toContain('Omit to leave it unfiltered'); - }); - - test('a field the model says nothing about gets only the behavior', () => { - const amount = lineItem.parameters.find(p => p.name === 'amount')!; - expect(amount.description) - .toBe('Match LineItem.amount exactly. Omit to leave it unfiltered.'); - }); -}); - - -// A filter has to be bound as the type its field declares. Casting the column -// to STRING would let one predicate shape serve every type, and no index can -// answer it -- a lookup on a primary key would scan the table. -describe('how a lookup filter reaches the store', () => { - const model = loadFixtureModel('actions_place_order.yaml'); - - // The fixture declares no field types, so every field travels as text. This - // one gives `orders` a typed key, which is the case worth checking. - function typedOrders(): SemanticModel { - const orders = model.entities.find(e => e.name === 'orders')!; - const typed: Entity = { - ...orders, - dataSource: - '//spanner.googleapis.com/projects/p/instances/i/databases/d/tables/Orders', - fields: orders.fields.map( - f => f.name === 'o_orderkey' ? {...f, type: 'Integer'} : f), - }; - return {...model, entities: [typed]}; - } - - test('the column is compared as itself, not cast to text', async () => { - const store = new FakeStore(); - const [tool] = entityTools({runtime: rt(typedOrders(), store.client)}); - await tool.invoke({o_orderkey: '12345'}); - - const [stmt] = store.statements; - expect(stmt.sql).toContain('WHERE o_orderkey = @f_0'); - expect(stmt.sql).not.toContain('CAST(o_orderkey AS STRING) ='); - expect(stmt.paramTypes!['f_0']).toEqual({code: 'INT64'}); - }); - - test( - 'a value the field\'s type has no room for is reported, not matched', - async () => { - const store = new FakeStore(); - const [tool] = entityTools({runtime: rt(typedOrders(), store.client)}); - const rows = await tool.invoke({o_orderkey: 'not-a-number'}); - expect(rows.problem).toContain('Integer'); - expect(rows.problem).toContain('No orders has o_orderkey'); - // Nothing was asked of the store: there is no row it could mean. - expect(store.statements).toHaveLength(0); - }); - - test('an empty string filters on the empty string', async () => { - // The dangerous reading is the other one. Dropping '' as though the filter - // were absent turns a lookup meant to find one row into an unfiltered read - // of the first rows, which comes back to the agent looking like an answer. - // `bindScalar` draws the line in the same place for the write path. - const store = new FakeStore(); - const [tool] = entityTools({runtime: rt(model, store.client)}); - await tool.invoke({o_orderkey: ''}); - - const [stmt] = store.statements; - expect(stmt.sql).toContain('WHERE o_orderkey = @f_0'); - expect(stmt.params!['f_0']).toBe(''); - }); - - test('an omitted filter is not a filter', async () => { - const store = new FakeStore(); - const [tool] = entityTools({runtime: rt(model, store.client)}); - await tool.invoke({o_orderkey: undefined}); - - const [stmt] = store.statements; - expect(stmt.sql).not.toContain('WHERE'); - }); -}); - - -// Every other failure in a lookup comes back as something the agent can read -// out. A store that refuses the read is not a different kind of thing, and a -// thrown error reaches an adapter as a crashed tool call instead. -describe('a lookup the store will not answer', () => { - const model = loadFixtureModel('actions_place_order.yaml'); - - function bound(): SemanticModel { - const orders = model.entities.find(e => e.name === 'orders')!; - return { - ...model, - entities: [{ - ...orders, - dataSource: - '//spanner.googleapis.com/projects/p/instances/i/databases/d/tables/Orders', - }], - }; - } - - test('a refused read is reported rather than thrown', async () => { - const store = new FakeStore(); - store.queryStatus = 403; - store.queryMessage = 'caller lacks spanner.databases.select'; - const [tool] = entityTools({runtime: rt(bound(), store.client)}); - const rows = await tool.invoke({}); - expect(rows.problem).toContain('Could not read orders'); - expect(rows.problem).toContain('spanner.databases.select'); - expect(rows.rows).toEqual([]); - }); - - test('a session that cannot be opened is reported too', async () => { - const store = new FakeStore(); - store.sessionThrows = true; - const [tool] = entityTools({runtime: rt(bound(), store.client)}); - const rows = await tool.invoke({}); - expect(rows.problem).toContain('Could not read orders'); - expect(rows.problem).toContain('403'); - }); -}); - - -// Both halves land in one name space at the adapter, and a duplicate name -// there is the framework's to resolve however it likes. Deriving them together -// is the only place that can see the collision at all. describe('one name space for everything a model offers', () => { const model = loadFixtureModel('actions_place_order.yaml'); - test('an action keeps its name and the lookup takes the longer form', () => { - // `customer` the entity yields `find_customer`; an action named - // `FindCustomer` wants the same tool name, and it is the author's own. - const clashing = { - ...model, - actions: [{...model.actions![0], name: 'FindCustomer'}], - }; - const {lookups, actions} = modelTools({runtime: rt(clashing)}); - expect(actions.map(t => t.name)).toEqual(['find_customer']); - expect(lookups.map(t => t.name)).toEqual([ - 'find_orders', 'lookup_customer' - ]); - }); - test('two actions that snake-case alike are still told apart', () => { const twins = { ...model, @@ -851,9 +568,8 @@ describe('one name space for everything a model offers', () => { }); test('nothing is renamed when nothing collides', () => { - const {lookups, actions} = modelTools({runtime: rt(model)}); + const {actions} = modelTools({runtime: rt(model)}); expect(actions.map(t => t.name)).toEqual(['place_order']); - expect(lookups.map(t => t.name)).toEqual(['find_orders', 'find_customer']); }); }); @@ -862,16 +578,13 @@ describe('sorting the tools an adapter can actually offer', () => { const model = loadFixtureModel('actions_place_order.yaml'); const runnable = withExecutor(model, RUNNABLE); - test('everything runnable is offered, lookups before actions', () => { - const {callable, withheld} = - callableTools(modelTools({runtime: rt(runnable)})); - expect(callable.map(t => t.name)).toEqual([ - 'find_orders', 'find_customer', 'place_order' - ]); + test('every runnable action is offered directly by modelTools', () => { + const {callable, withheld} = modelTools({runtime: rt(runnable)}); + expect(callable.map(t => t.name)).toEqual(['place_order']); expect(withheld).toEqual([]); }); - test('a guarded action is withheld, and says why', () => { + test('a guarded action without a judge is withheld, and says why', () => { // A guard is the case that matters: the model says this write must be // checked, no checker exists, so the tool must not be offered as callable. const guarded = { @@ -882,9 +595,8 @@ describe('sorting the tools an adapter can actually offer', () => { onViolation: 'escalate', }] as Constraint[], }; - const {callable, withheld} = - callableTools(modelTools({runtime: rt(guarded)})); - expect(callable.map(t => t.name)).toEqual(['find_orders', 'find_customer']); + const {callable, withheld} = modelTools({runtime: rt(guarded)}); + expect(callable).toEqual([]); expect(withheld.map(t => t.name)).toEqual(['place_order']); expect(withheld[0].unavailable).toContain('UnderReview'); }); @@ -918,7 +630,6 @@ describe('the instruction an agent is given comes from the model', () => { const {instruction} = modelTools({runtime: rt(model)}); expect(model.aiContext?.instructions).toBeUndefined(); expect(instruction).toContain('Never invent an identifier'); - expect(instruction).toContain('lookup tools'); expect(instruction).toContain('did not happen'); }); @@ -1005,10 +716,8 @@ describe('what a caller is told about an outcome', () => { // `fieldBinding` is ir.ts's stated single source of truth for whether a field // is bound, and a field awaiting transpilation carries only the vendor // expression it was imported with. `createSemanticRuntimes` transpiles nothing, -// so that is exactly the state a vendor-imported model reaches these tools in. -// Consulting `expression` alone reported every one of its entities as having -// no column to read, and told the caller to push the model with a binding -// profile -- which is not the problem and would not fix it. +// so that is exactly the state a vendor-imported model reaches +// `readableEntities` in. describe('an entity whose fields await transpilation', () => { const model = loadFixtureModel('actions_place_order.yaml'); @@ -1027,25 +736,15 @@ describe('an entity whose fields await transpilation', () => { }); } - test( - 'is readable, because the imported expression names a real column', - () => { - const tool = - entityTools({ - runtime: rt({...model, entities: untranspiled('customer')}) - }).find(t => t.entityName === 'customer')!; - expect(tool.runnable).toBe(true); - expect(tool.unavailable).toBeUndefined(); - }); - - test('offers the same filters it would after transpilation', () => { - const before = entityTools({ - runtime: rt(model) - }).find(t => t.entityName === 'customer')!; - const after = entityTools({ - runtime: rt({...model, entities: untranspiled('customer')}) - }).find(t => t.entityName === 'customer')!; - expect(after.parameters.map(p => p.name)) - .toEqual(before.parameters.map(p => p.name)); + test('yields the same readable schema it would after transpilation', () => { + const baseRuntime = rt(model); + const dialect = dialectFor(baseRuntime.store); + const before = readableEntities(baseRuntime, dialect) + .find(r => r.entity.name === 'customer')!; + const after = + readableEntities( + rt({...model, entities: untranspiled('customer')}), dialect) + .find(r => r.entity.name === 'customer')!; + expect(after.fields).toEqual(before.fields); }); }); diff --git a/toolbox/mdcode/tests/libts/semantic/runtime/run_action.test.ts b/toolbox/mdcode/tests/libts/semantic/runtime/run_action.test.ts index 1745248b..539a2212 100644 --- a/toolbox/mdcode/tests/libts/semantic/runtime/run_action.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/runtime/run_action.test.ts @@ -1223,92 +1223,6 @@ describe('a guard settled by judgment', () => { expect(whyRefusedWithoutRunning(guarded, action, undefined, holds())) .toBeNull(); }); - - test('skipGuards clears the refusal for want of a judge', async () => { - // What the caller is saying is that nobody will be asked. A guarded action - // is otherwise unrunnable without a judge, and that refusal is total, so - // an author with no judge configured could not exercise their own write at - // all without deleting the guard -- which loses the guard and tests a - // different model. - const judged = guarding([justified]); - expect(whyRefusedWithoutRunning(judged, judged.actions![0])) - .toContain('no judge to ask'); - expect(whyRefusedWithoutRunning( - judged, judged.actions![0], undefined, undefined, true)) - .toBeNull(); - }); - - test( - 'skipGuards names every guard it passed over, on the outcome', - async () => { - // One line for the whole skip rather than one per rule, since they went - // unchecked for one reason -- but a line, not silence. This used to - // return no warnings at all, on the reasoning that the caller asking - // for the skip already knew. The caller is not the only reader: - // `describeOutcome` hands these warnings to an agent as a tool's own - // answer, and an agent told only `applied: true` has been told the - // write met every rule the model states. - const fake = fakeStore(); - const outcome = await act({ - model: guarding([advisory]), - actionName: 'Credit', - args: {account: 'A1', amount: 100}, - client: fake.client, - skipGuards: true, - }); - if (outcome.status !== 'committed') throw new Error(outcome.message); - expect(fake.committed).toBe(true); - expect(outcome.warnings ?? []).toHaveLength(1); - expect((outcome.warnings ?? [])[0]) - .toContain('guards were not checked: CreditIsJustified'); - expect((outcome.warnings ?? [])[0]) - .toContain('the write was made anyway'); - }); - - test('skipGuards asks no judge, even when one is handed over', async () => { - // `skipGuards` means nobody is asked. A caller that passed both used to - // reach the judge anyway while the unsettled-guard warnings stayed - // suppressed -- the worst of both, since a judge that threw or returned - // nothing then committed with no line about it anywhere. - const fake = fakeStore(); - let asked = 0; - const outcome = await act({ - model: guarding([justified]), - actionName: 'Credit', - args: {account: 'A1', amount: 100}, - client: fake.client, - skipGuards: true, - judge: { - name: 'a judge nobody should reach', - decide: async () => { - asked++; - return {holds: false, reason: 'refused'}; - }, - }, - }); - expect(asked).toBe(0); - if (outcome.status !== 'committed') throw new Error(outcome.message); - expect(fake.committed).toBe(true); - }); - - test('skipGuards does not clear a guard that names nothing', async () => { - // Not checking the guards is not the same as not reading them. A guard - // naming a rule the model never declares is the model being wrong about - // itself -- a push refuses it on the same grounds -- and it refuses with a - // judge in hand, so standing the judge down was never what was wrong. The - // write it would apply is one the author believes is gated by something - // that does not exist, and the repair is a spelling, not a flag. - const undeclared = creditModel({ - actions: [{...credit, guards: ['NoSuchRule']}], - constraints: [], - }); - const action = undeclared.actions![0]; - expect(whyRefusedWithoutRunning(undeclared, action, undefined, holds())) - .toContain('not declared'); - expect(whyRefusedWithoutRunning( - undeclared, action, undefined, undefined, true)) - .toContain('not declared'); - }); }); diff --git a/toolbox/mdcode/tests/libts/semantic/skills.test.ts b/toolbox/mdcode/tests/libts/semantic/skills.test.ts index f02d9ff1..0f721e13 100644 --- a/toolbox/mdcode/tests/libts/semantic/skills.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/skills.test.ts @@ -402,9 +402,9 @@ describe('when the runtime would refuse the call', () => { describe('what a key matching nothing costs', () => { const model = loadFixtureModel('actions_place_order.yaml'); - // The model-level instruction sends an agent to lookup tools, and this skill - // has none. Saying where a key comes from instead is the answer to that, and - // it does not depend on the model declaring any entities. + // The model-level instruction tells an agent never to invent an identifier, + // and this skill offers only the write side. Saying where a key comes from + // instead does not depend on the model declaring any entities. const noEntities: SemanticModel = { ...model, entities: [], @@ -413,8 +413,8 @@ describe('what a key matching nothing costs', () => { test('still says where a key has to come from', () => { const out = generate(rt(noEntities)).files['SKILL.md']; - expect(out).toContain('lookup tools'); expect(out).toContain('## Finding a record'); + expect(out).toContain('This skill offers writes, not reads.'); expect(out).toContain('the key has to come from somewhere else'); });