Let agents call tools via a new toolRefs field and callTool() SDK method

[k5s-bot]

Summary

Closes #87. Adds tool-calling for agents, abstracted through the SDK as requested:

• Agent.spec.toolRefs []string (Go CRD) — names Tool CRs a sub-agent's own internal loop may call, mirroring the existing Skill.spec.toolRefs pattern. Validated by AgentReconciler (new ToolRefsMissing condition reason), same static-config-sanity-check discipline as every other refs field in this codebase.
• tool_call/tool_result protocol pair (packages/messaging) — same request/response shape as the existing opencode_request/opencode_response pair, correlated by callId.
• AgentSession.callTool(name, input): Promise<unknown> (packages/agent-runtime) — the actual SDK ask: an agent author writes await session.callTool("kubectl-readonly", "get pods -n default") and never sees the NATS round-trip. Throws ToolCallError on failure, rejects with the existing CancelledError on run cancellation, supports multiple concurrent calls (unlike the single-slot ask()).
• Orchestrator dispatch (apps/agent-orchestrator) — AgentDescriptor.toolRefs, a new onToolCall/resolveToolCall pair on AgentOrchestratorChannel, and a dispatchResolvedTool/makeSubAgentToolCallHandler helper (new agent/dispatch-tool.ts) wired at the two direct agent-delegation call sites in graph.ts (checkActiveAgentRun, delegateToAgent). Resolves the requested tool via a new non-RBAC toolCatalog lookup (same toolsById map ADR 0020 already builds) rather than the caller-scoped vectorStore, since an Agent's toolRefs is a static operator-declared allowlist, not a per-caller retrieval filter.

Full design rationale, including a couple of deliberate v1 scope cuts (no agent-backed-tool recursion; not wired at the Skill-mediated agent-backed-Tool runTool branch), is in docs/adr/0028-agent-tool-refs-sub-agent-tool-calls.md. The ADR's "Consequences" section also now documents two deliberate dispatch asymmetries surfaced in review (no onJobProgress subscription in dispatchResolvedTool; the shared awaitReply idle-timeout budget).

Synced with main (merged, not rebased — matching this repo's merge-commit history). The one conflict was in graph.ts, where main's remote-control-url progress-listener enhancement landed on the same two awaitReply call sites this PR adds onToolCall to; both were combined so each site now forwards remote-control URLs and wires sub-agent tool calls.

Test plan

• make generate && make manifests (Go controller-gen) regenerated deepcopy + CRD YAML cleanly, with zero diff beyond what's committed here
• go build ./... and go vet ./... clean
• go test ./... (envtest/ginkgo) — all specs pass, including 2 new toolRefs cases in agent_controller_test.go
• packages/messaging vitest — 14/14 pass (new tool_call/tool_result schema cases)
• packages/agent-runtime vitest — 9/9 pass (new callTool success/failure/concurrency/cancellation cases)
• apps/agent-orchestrator vitest — 356/356 pass post-merge (new nats-agent-channel.test.ts, dispatch-tool.test.ts, plus updated crd-agent-registry.test.ts; the extra cases over the original 339 come from merging main)
• tsc --noEmit clean across all three TS packages/apps touched (after building packages/messaging + packages/agent-runtime first)

---

A maintainer can apply the ai-review label to this PR to request an automated code review.

[k5s-bot]

Starting work on this now. Track progress or send follow-up prompts at https://gateway.kurpuis.com/sessions/C9VMUlrll7NdBW295_qmlvgBXOx_sNZ_Mjs4nxdICVI

[k5s-bot]

Summary

Reviewed by checking out feature/agent-toolrefs-callTool (4adab87) and verifying the PR's claims directly rather than just reading the diff:

• npm install (workspace, all three touched packages) → npm run build/typecheck clean on packages/messaging, packages/agent-runtime, apps/agent-orchestrator.
• packages/messaging vitest: 14/14 pass. packages/agent-runtime vitest: 9/9 pass. apps/agent-orchestrator vitest: 339/339 pass (35 files) — matches the PR description exactly.
• Go: go build ./... and go vet ./... clean. make generate && make manifests produces zero diff against what's committed (deepcopy, CRD YAML, RBAC role all already in sync). make test (envtest/ginkgo): internal/controller package ok, 64.2% coverage, including the two new toolRefs specs.

Overall this is a clean, well-scoped addition that follows the codebase's existing conventions closely (mirrors Skill.spec.toolRefs/SkillReconciler for the CRD side, mirrors ask()/pendingAsk for the SDK side, mirrors opencode_request/opencode_response for the protocol side). The ADR (0028) is thorough and its documented v1 scope cuts are exactly what's implemented — I verified both:

• The agent-backed-Tool branch of runTool (graph.ts:1322, the third awaitReply call site) is indeed left without onToolCall wiring, as documented.
• dispatchResolvedTool (dispatch-tool.ts:44) returns a clean {ok:false} for tool.agentRunTemplate rather than recursing, as documented.

Test coverage is genuinely good: schema round-trips, concurrent/out-of-order callId correlation, cancellation-while-pending, and — nice touch — this is the first unit test file for NatsAgentChannel (nats-agent-channel.test.ts didn't exist before this PR), adding real coverage for awaitReply's message loop beyond just the new tool_call case.

Findings

1. Operational gotcha worth calling out explicitly (not a bug — already disclosed in the ADR)

If an operator gives an Agent both spec.toolRefs and wraps that same Agent as an agent-backed Tool (Tool.spec.agentRef) that some Skill calls, a sub-agent reached via that Skill-mediated path that calls session.callTool() gets no reply at all — its tool_call up-message falls into awaitReply's default: break case (nats-agent-channel.ts:174) since onToolCall isn't wired at that third call site (graph.ts:1322). The caller only finds out via the full run timing out (agentAwaitReplyTimeoutMs, up to DEFAULT_TIMEOUT_MS = 30 min if unset). The ADR calls this out under "Wired at two of the three awaitReply call sites, not all three" — so this is a known, deliberate v1 cut, not an oversight. Flagging mainly so it's tracked as a real operational trap (a misconfigured/overlapping Agent+Tool setup fails silently-slow rather than fast) rather than lost in the ADR prose — might be worth a one-line runtime guard (e.g., reject toolRefs at admission if the same Agent is also referenced by an agentRef-Tool) in a follow-up.

2. Minor: no progress-listener parity between dispatchResolvedTool and runTool's inline dispatch

dispatch-tool.ts:56-77 (the container-Job branch) never calls deps.jobResultReceiver.onJobProgress, unlike the equivalent branch in graph.ts's runTool (graph.ts:1398-1400,1424), which registers/unsubscribes a progress listener around the same awaitJob. Since AgentSession.callTool() doesn't expose a progress channel in its type signature (runtime.ts:44), there's nowhere to route progress events today even if they were subscribed — so this isn't a functional gap, just a documented-nowhere asymmetry between the two now-duplicated dispatch paths. Worth a one-line comment (or fixing when dispatchResolvedTool/runTool eventually converge, per the ADR's "Consequences" section) so the omission reads as intentional rather than copy-paste drift.

3. Minor: idle-timeout interaction with long tool calls

awaitReply's NATS subscription timeout (nats-agent-channel.ts:107,145, default 30 min) is the same idle-timeout budget a tool_call now shares with everything else the sub-agent might do. A long-running container tool (e.g. a slow kubectl job) that emits no progress/warning up-messages while dispatchResolvedTool awaits its callback could, in principle, run past that idle window with no other traffic to keep the subscription alive, surfacing as a generic AgentTurnTimeoutError rather than a clear "tool X took too long" error. This mirrors a pre-existing risk in the agent-backed-Tool runTool path (same dispatch shape, same timeout), so it isn't new, but sub-agent tool calls make hitting it more likely in practice since there's no equivalent of a human pausing thoughtfully — worth keeping in mind if kubectl-readonly-style tools graduate to genuinely slow operations.

4. Nit: nats-agent-channel.test.ts bypasses a private constructor via a type cast

nats-agent-channel.test.ts:25-27 constructs NatsAgentChannel directly by casting around its private constructor (nc: unknown cast to the real signature) since esbuild doesn't enforce private at runtime. It's clearly commented and this is the first test file for this class, so there's no prior convention being broken, but it does mean the "channel must be built via .connect()" invariant already has one deliberate hole. A trivial alternative (an internal-only static forTest() factory, or just dropping private since TS privacy is compile-time-only anyway) would avoid the cast, but this is purely stylistic — not blocking.

Verdict

No correctness, security, or test-coverage issues found. The design is sound (the RBAC-vs-static-allowlist reasoning for the new toolCatalog port, in particular, is well-argued and consistent with how AgentReconciler already treats toolRefs/skillRefs). Findings above are informational/follow-up material, not blockers.

[k5s-bot]

Review posted at #108 (review).

What I did: Checked out feature/agent-toolrefs-callTool and independently verified every claim in the test plan rather than just reading the diff — ran npm install/build/typecheck and vitest for all three touched TS packages (14/14, 9/9, 339/339 all passed, matching the PR description), and ran go build, go vet, make generate && make manifests (zero diff) and make test (envtest/ginkgo, controller package ok, 64.2% coverage) for the Go side.

Verdict: No correctness, security, or test-coverage blockers. Posted as a COMMENT review (not approve/request-changes) with 4 inline notes:

1. A real operational gotcha (already disclosed in the ADR) where an Agent with both toolRefs and an agent-backed-Tool wrapper can silently hang a sub-agent's callTool() until the whole run times out — flagged as worth a follow-up admission-time guard.
2. Minor asymmetry: dispatchResolvedTool doesn't wire job-progress reporting the way runTool's inline dispatch does (not a functional gap since the SDK has no progress channel, just worth a comment).
3. Minor: long-running tool calls share the same idle-timeout budget as the rest of awaitReply, which could surface as a generic timeout error.
4. Style nit: a new test bypasses a private constructor via a type cast.

---

✅ Opened pull request: imaustink/agent-controller#108

[k5s-bot]

🤖 Starting work on this now. Watch live or take over the session here: https://claude.ai/code/session_01EQ88HCtuCGZXv8ed591ccn

[k5s-bot]

Picked back up via ai-triage — synced with main and addressed the review

Synced the branch (this was the actual blocker)

The PR was CONFLICTING/DIRTY against main. Merged origin/main in (merge, not rebase — matching this repo's merge-commit history; no force-push). The single conflict was in apps/agent-orchestrator/src/agent/graph.ts: main had added a remote-control-url progress-listener branch to the same two awaitReply call sites (checkActiveAgentRun, delegateToAgent) that this PR adds onToolCall to. I combined both so each site now forwards remote-control URLs and wires sub-agent tool calls. The toolCatalog/claudeRemoteGateway dep additions (both auto-merged in the AgentGraphDeps interface and index.ts) were kept side by side. PR is now MERGEABLE.

Verification after the merge

• Go: go build ./..., go vet ./... clean; make manifests generate → zero diff (deepcopy/CRD/RBAC all in sync); internal/controller envtest suite ok (64.0%), including the two toolRefs specs.
• TS: tsc --noEmit clean on all three touched packages/apps; packages/messaging 14/14, packages/agent-runtime 9/9, apps/agent-orchestrator 356/356 (up from 339 — the extra cases come from main).

Addressed each review note (all were non-blocking)

1. Progress-listener asymmetry (dispatch-tool.ts) — added a comment that skipping onJobProgress is intentional (callTool() exposes no progress channel), + an ADR 0028 "Consequences" bullet. ✅ resolved
2. Idle-timeout budget (nats-agent-channel.ts:172) — commented that a tool_call shares awaitReply's idle-timeout window and can surface as a generic AgentTurnTimeoutError, + an ADR bullet. Kept as documentation, not a behavior change, since it isn't a new risk. ✅ resolved
3. Private-constructor cast in test (nats-agent-channel.test.ts) — took the suggested route: added an internal NatsAgentChannel.forTest() factory and switched the test to it, so the private constructor invariant stays intact. ✅ resolved
4. Operational gotcha / admission-check follow-up (graph.ts) — deliberately not implemented here, replied on the thread with the reasoning: a reconcile-time check in AgentReconciler would be racy (an Agent isn't re-reconciled when a Tool later starts referencing it via agentRef), so a correct fast-fail needs a validating admission webhook — genuinely a follow-up, and already tracked in ADR 0028's "Wired at two of the three" section. Happy to file a dedicated issue if you'd rather track it outside the ADR. ✅ resolved (as a reasoned deferral)

Deliberately not done

• The admission-time guard for the Agent-toolRefs + agent-backed-Tool overlap (see Stream opencode child stdout live instead of buffering to job exit #4) — deferred as a documented v1 cut.
• No change to the third (runTool) awaitReply call site — still the documented v1 scope cut.

Only the validate check + this run's checks are outstanding as CI re-runs on the pushed merge; nothing was failing on the code.

[k5s-bot]

The PR is now MERGEABLE / CLEAN and the validate check passed on the pushed merge commit. Done.

Summary

I picked up PR #108 ("Let agents call tools via a new toolRefs field and callTool() SDK method") and got it ready to merge.

The actual blocker was a merge conflict. The PR was CONFLICTING/DIRTY against main. I merged origin/main in (merge, not rebase — matching this repo's merge-commit history; no force-push). The one conflict was in apps/agent-orchestrator/src/agent/graph.ts: main had added a remote-control-url progress-listener branch to the same two awaitReply call sites this PR adds onToolCall to. I combined both so each site now forwards remote-control URLs and wires sub-agent tool calls. PR is now MERGEABLE/CLEAN.

Verification after merge: Go build/vet clean; make manifests generate → zero diff; controller envtest suite passes (incl. the 2 toolRefs specs). TS: tsc --noEmit clean; messaging 14/14, agent-runtime 9/9, orchestrator 356/356.

Review feedback (all 4 non-blocking notes from the k5s-bot review), each replied to and thread resolved:

1. Progress-listener asymmetry → added an intentional-omission comment + ADR bullet.
2. Idle-timeout budget → added a comment + ADR bullet (documentation, not a behavior change).
3. Private-constructor cast in the test → added a NatsAgentChannel.forTest() factory and switched the test to it.
4. Admission-check follow-up for the Agent-`toolR…

---

✅ Opened pull request: imaustink/agent-controller#108