refactor(mdcode)!: remove entity lookup tools from agent tool derivation - #458
Conversation
libei
left a comment
There was a problem hiding this comment.
The deletion itself is clean. No dangling symbols, distinct()'s comment correctly repointed from the lookup/action collision to the action/action one, and "withheld for one of four reasons" matches whyRefusedWithoutRunning (three) plus noStore (one) exactly. The SKILL.md self-contradiction this set out to fix is genuinely fixed.
I rebuilt the payments model from the YAML blocks on the page and ran kcmd agent-tools against it: the listing pasted into actions.md §7 is byte-identical to the live run. Build clean, 1105 tests pass (68 + 997 + 40).
What's left is orphaned prose — the same failure mode as the reading-judge removal.
The demo still advertises the deleted tool
demo/semantic-model/skill/catalog/EntryGroups/commerce_demo/commerce.yaml isn't in this diff, so these can't be inline:
:156—IssueCredit'sai_context.instructionsstill says "Look the order up first if you were given a customer name rather than a number." I rankcmd agent-toolsindemo/semantic-model/skill/and that sentence comes back at line 10 of the output, so it also lands in the generatedreferences/issue-credit.md. The demo instructs the agent to use a tool this PR deletes.:53— the comment explaining why the model'sai_contextstays business-only cites "How to use a lookup tool, and what to do when a write is refused, are properties of the tools ... so the derivation supplies those." The derivation no longer supplies the first half.
Worth noting why these survive a grep: lookup tool at :53 is wrapped across two comment lines, so a line-based search can't match it, and :156 is a paraphrase rather than the term.
Minor
tests/libts/semantic/runtime/agent_tools.test.ts:57-59—queryStatus,queryMessageandsessionThrowsonFakeStoreare now unreachable; nothing assigns them since the lookup describes went. (executeQueryitself may still be needed to satisfy the client shape.) Dead scaffolding reads as live coverage to the next person.boundFieldsstays exported but has no production consumer outsideagent_tools.ts, wherereadableEntitiescalls it at :572. The description says it's kept "forskills.tsschema generation," butskills.tsimports onlyreadableEntities— so it could go module-private on a change whose point is shrinking this surface.
Pre-existing, not this PR
actions.md's listing says store semantic_skill_demo while the profile block at :264 says database bank. Identical at HEAD~1.
Nothing here is a correctness bug in the runtime. The demo instruction is the one I'd fix before merging, since it ships a live pointer to a tool that no longer exists.
| tool at all, so an agent walks a relationship by looking up each end itself, and | ||
| nothing totals anything on its behalf. | ||
| agent only through an action that guards on it. Entities, relationships and | ||
| metrics get no tool here, so an agent reads or totals through its own read |
There was a problem hiding this comment.
This now tells the agent it totals through its own read surface, but the derived instruction quoted 39 lines below still says "Never compute a total or a balance yourself; the tools do that." With the lookups gone, no derived tool totals anything.
The old wording ("nothing totals anything on its behalf") was merely in tension with that; this version affirmatively directs the agent to do what the shipped instruction forbids. An agent asked what an order is owed reads the lines, then refuses to sum them and waits on a tool that doesn't exist.
One of the two sentences has to move — either drop "or totals" here, or drop the totals clause from instructionFor.
| // copied into every adapter, drifting in each one. | ||
| // The second part is about the tools rather than the business -- 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. |
There was a problem hiding this comment.
This comment says the derivation owes the agent "what a refused write or a warning means" — but the instruction below also still opens with "Never invent an identifier" and no longer says where to get one, since "find it with the lookup tools" went with the tools.
skills.ts patches that hole for SKILL.md via ## Finding a record ("ask the caller, or read the store directly"). The library path — callableTools(modelTools({runtime})), which §7 "Calling it from code" documents as the ADK/LangChain/MCP route — gets only the bare prohibition, so an agent handed a customer name is told never to invent a key and never told where one comes from.
Carrying the skill's one clause into instructionFor would give both consumers the same answer.
| 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('Never invent an identifier'); |
There was a problem hiding this comment.
This assertion no longer tests the thing the describe is named for. Never invent an identifier is in the always-emitted instruction, not in the read-side section, so this line passes even if readSideSection is deleted outright.
The two assertions below it (## Finding a record, the key has to come from somewhere else) do still cover it, so nothing is actually unguarded — but this line is now carrying no weight.
| > Catalog, which takes it under any profile. | ||
| > | ||
| > An action's statements and an agent's lookups run, against an operational | ||
| > An action's statements run against an operational |
There was a problem hiding this comment.
Paragraph wasn't rewrapped after "and an agent's lookups" came out — this line is 51 chars and the next runs to 82, inside an 80-col blockquote.
|
@libei Addressed all review comments in
|
Why
entityTools(find_<entity>/lookup_<entity>) was introduced when action parameters were entity references (e.g.Account) and we shipped a standalone ADK agent demo that had no other way to inspect the store.Since then:
{concept: Account, field: accountId}) resolve at load time to a primitive type (Integer) and description; the runtime performs no entity resolution.SKILL.mdfiles emit the physical schema mapping (readableEntities()/boundFields()) so agents can query the store through their native read surface rather than a narrow single-table exact-matchSELECT ... LIMIT 51tool.entityToolscausedSKILL.mdto contradict itself:instructionFor()told the agent to "find it with the lookup tools rather than asking for it", while the very next section (## Finding a record) had to walk that back ("This skill offers writes, not reads...").What changed
src/libts/semantic/runtime/agent_tools.ts):EntityTool,EntityRows,EntityToolOptions,entityTools(),lookupFor(),whyUnreadable(), andrunLookup().ModelToolsto{actions, instruction}andCallableToolsto{callable, withheld, instruction}."find it with the lookup tools"sentence frominstructionFor().boundFields()andreadableEntities()forskills.tsschema generation.src/tool/commands.ts):printLookupTool()anddescribedPart()fromkcmd agent-tools.tests/libts/semantic/runtime/agent_tools.test.ts,tests/libts/semantic/skills.test.ts,*.skill.golden.md):entityToolsunit tests and updateSKILL.mdgolden files.docs/semantic-model/actions.md,skills.md,reference.md,profiles.md):actions.mdand command references to describe the derived write tools and instruction.