C0: wire Palette delegation into run.Executor (skill__/agent__ tools) #8
Reference in New Issue
Block a user
Delete Branch "phase-c0-executor-ports"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
First step of the mort cutover (plan: strangler-fig,
.agent runcanary first, flag + instant fallback, PR-per-phase). This wires the onerun.Portspiece the canary needs that wasn't yet called by the executor: Palette delegation.What
The executor now turns an agent's
SkillPalette/SubAgentPaletteinto delegation tools, so a mort agent that delegates runs throughrun.Executor:run/palette.go:addDelegationToolsbuilds askill__<name>tool (structuredinputs) perSkillPaletteentry and anagent__<name>tool (prompt) perSubAgentPaletteentry. Each invokesrun.Ports.Paletteas a child of the current run (parentRunID = inv.RunID, inheriting caller + channel). A non-ok child status is surfaced to the parent with the partial output.executor.go: called right after the low-level toolbox is built.PaletteSourceor empty palette → no delegation tools (unchanged behavior).Tests
skill__helper→ routed throughPalettewith the right name/caller/inputs/parent run id.Deferred to C0b (the rest of the executor
run.Portswiring)Critic (soft-timeout monitor + deadline binding + steer), Delivery (executor-side output egress, for chatbot/cron surfaces), Checkpointer (needs a majordomo message-history hook to snapshot resumable state). The
.agent runcanary delivers its returnedResult.Outputitself, so none of these are on its critical path.Next after this merges: C1 — mort-side adapters (audit/persona/skill stores + host-tool registration) and routing
.agent runthroughexecutushostbehind a convar flag.🤖 Generated with Claude Code
🪰 Gadfly review —
minimax-m3:cloud(ollama-cloud)Verdict: Minor issues — 3 reviewers: security, correctness, error-handling
🔒 Security — Minor issues
Confirmed — no gate/audit wrapper is applied in
addDelegationTools. The tools are built via rawllm.DefineTooland added directly. Both findings stand.🔒 Security lens
VERDICT: Minor issues
e.cfg.Registry.Build(...)atrun/executor.go:164is gated (CheckGate) and audited via the registry wrappers (seetool/gated_tool.go:144,tool/registry.go:685-690). Theskill__<name>/agent__<name>handlers are built directly viallm.DefineTooland callpalette.InvokeSkill/InvokeAgentwith no gate check and noEmitAudit. Concretely: an LLM can callskill__<name>regardless ofinv.SkillName/ share-safety policy, and the audit recorder (rec.OnTool/EmitAudit) sees nothing for the delegation, only the upstream step observer's emit ofcall.Name/call.Arguments. Fix: route them throughRegistry.Build(register the delegation closures as a tool impl whosePermissioncarries the appropriate gate andSafeForShare), or at minimum calltool.CheckGate(inv)andtool.EmitAudit(inv, ...)inside each closure before/after thepalette.*call.inv.CallerID/inv.ChannelIDare passed straight to the child run. That is the intended "child of current run" semantics, but it means a child can perform actions charged to the parent's caller with no additional authorization step (no separate budget check, no Critic re-bind — both deferred to C0b). Worth a security note for C0b; not blocking here, but flagged because the parent-run model's tool call is the only authorization gate and it now extends the parent's caller identity into a fresh child run.Nothing else in my lane is materially wrong: no SQL/command/path injection (no shell, no templating, no path concatenation from user input), no deserialization, no secret handling, no SSRF surface introduced by this diff. The Toolbox.Add API and
llm.DefineToolsignatures are verified against existing usage intool/registry.go:696andtool/registry_test.go:26.🎯 Correctness — Minor issues
VERDICT: Minor issues
run/palette.go:23 — typed-nil
PaletteSourcewould NPE. Thepalette == nilcheck uses the interface-vs-nil comparison, which does NOT catch a typed-nil concrete value (var p *ConcretePalette; Ports.Palette = p). The Ports doc atrun/ports.go:33-34explicitly promises that a nil Palette leaves palette entries inert, so a host following the docs could reasonably pass a typed-nil and then crash inside the firstInvokeSkill/InvokeAgentcall. Confirmed againstrun/palette.go:23-25andrun/ports.go:32-34.run/palette.go:49 — sub-agent overrides are silently dropped.
InvokeAgentis called with hardcoded"","",nil,nilformodelTierOverride/promptPrepend/toolsSubset/onEvent, even thoughPaletteSource.InvokeAgentaccepts all four (run/ports.go:164-167). TheRunnableAgentDTO has no fields to thread them through (run/agent.go:16-55), so a saved sub-agent that needs any of these overrides cannot be wired up at this layer. Confirmed against the interface signature and DTO definition.run/palette.go:31-37 — skill input schema is not enforced at the wrapper.
skillDelegateArgs.Inputsismap[string]any; the description only says "matching its declared input schema" without enforcement. Any keys/values the model emits are passed straight through. Not a bug for C0 (the skill itself presumably validates downstream), but the PR's "structured inputs" framing is not actually enforced here. Confirmed against the type atrun/palette.go:72-74.run/palette.go:65-69 —
delegationResultmixes status text into the output string. A non-ok child returns prose of the form[skill "x" ended with status "timeout"]\n<output>, which the model will see as plain tool output and may re-feed into subsequent steps. The executor's own step observer atrun/executor.go:214already propagatesr.IsError, so aIsError=trueresult would be a more honest channel for surfacing a partial-status outcome. Confirmed againstrun/palette.go:65-70andrun/executor.go:196-219.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Findings through the error-handling & edge-cases lens:
run/palette.go:31-37, 48-54— Partial output silently discarded on the err path. The doc comment fordelegationResult(lines 63-64) promises a "non-ok child status is surfaced to the parent with the partial output" — but whenInvokeSkill/InvokeAgentreturns a non-nilerr, the callback returnsnil, fmt.Errorf(...)and never touchesout/status. If a battery ever returns(partialOutput, childRunID, "timeout", someTransportErr)(work started, then failed mid-flight), the parent model only sees a bare error and can't react to the partial result. Either gate this behind a clear "err means hard-fail with no usable output" contract (and document it onPaletteSource), or merge the paths sodelegationResultalways runs (e.g. log the partial into the error message).run/palette.go:26, 43— Empty / duplicate palette names not pre-validated.ra.SkillPalette: []string{""}produces the tool nameskill__, and a duplicate entry produces a duplicate tool name. Both surface as an opaquebox.Adderror (add skill__: %w/add skill__foo: %w) far from the real cause. An upfront check that rejects empty names with a clear message (and either dedupes or reports duplicates) before callingbox.Addwould turn a latent landmine into a fast-fail.run/palette.go:65-69— Header with trailing blank line whenout == ""and status != ok. If the child produced no output but ended non-ok,delegationResultreturns"[skill \"x\" ended with status \"timeout\"]\n"followed by the emptyoutformatted via%s, producing a header line followed by a blank line. Not wrong, just noisy. Anif out == "" { return header }branch (orstrings.TrimRight) would clean it up. Minor.Nil-safe behavior verified. Confirmed by reading
run/palette.go:23-25andrun/palette_test.go(TestNoPaletteNoDelegationTools): a nilPaletteSourceshort-circuits before any tool is added, and an empty palette (both slices nil) yields zero iterations. The executor error path atrun/executor.go:173-177mirrors the existingBuildfailure path (sameres.Err+finishAudit("error", …)shape).Loop capture verified.
name := name(palette.go:27, 44) correctly shadows the loop variable — no cross-iteration aliasing if the toolbox dispatches these concurrently later.invvalue-capture verified. The closures capturetool.Invocation(struct) by value, so the executor's reuse of the localinvafter this point can't race with the closure. The audit-stamping write atexecutor.go:160(inv.RunState = stateAcc) happens beforeaddDelegationToolsis called at line 173, so the captured value is the post-audit-mutation one —inv.RunStatepropagates to delegates too, which appears intentional.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 44s
🪰 Gadfly review —
deepseek-v4-flash:cloud(ollama-cloud)Verdict: No material issues found — 3 reviewers: security, correctness, error-handling
🔒 Security — No material issues found
🔒 Security Review
Verdict: No material issues found
I reviewed the change through the security lens, tracing tainted input paths and checking for authn/authz gaps, injection, secret leakage, and unsafe defaults. No material security issues were found.
Analysis:
Authn/authz continuity is preserved. The delegation tools correctly pass
inv.CallerIDandinv.ChannelIDfrom the parent invocation through topalette.InvokeSkill/palette.InvokeAgent. The parent run's identity is inherited, so the palette implementation can perform its own authorization against the same caller. Verified by readingrun/palette.go:32,49andrun/ports.go:160-167.No injection vector in tool names. Tool names are constructed as
"skill__"+name/"agent__"+namefrom palette entries. These are registered withllm.DefineTool— they are identifiers in the LLM tool registry, not executed as commands or interpolated into any shell/path/query context. An attacker controlling palette entry names could not achieve code execution through this path.LLM-generated input reaches the palette without sanitization (
args.Inputsmap andargs.Promptstring atrun/palette.go:32,49). This is by design — thePaletteSourceinterface is the security boundary, and the palette implementation is expected to validate its own inputs. The delegation layer is a thin bridge, not a sanitization layer. No finding here.No secret/credential leakage. The
delegationResultfunction (run/palette.go:65-69) returns partial child output even on non-ok status, but this is documented intentional behavior ("A non-ok child status is surfaced to the parent with the partial output"). The parent agent already has access to the child's output through the tool result; this is a functional design choice, not a leak.No SSRF, unsafe deserialization, or insecure defaults. The code does not make network calls, deserialize untrusted data, or introduce new insecure defaults. The nil-safe palette check (
run/palette.go:23-25) is correct — a nilPaletteSourceproduces no delegation tools, matching the pre-existing behavior.Summary: This change is clean from a security standpoint. The delegation layer correctly threads caller identity through to the palette, uses no dangerous operations, and introduces no new attack surface beyond what the
PaletteSourceinterface already defines.🎯 Correctness — No material issues found
🎯 Correctness — Verdict: No material issues found
I verified the following against the actual checked-out code:
PaletteSourceinterface (run/ports.go:158-167): TheInvokeSkillandInvokeAgentsignatures match the calls inpalette.goexactly — argument count, types, and order all line up. TheparentRunIDparameter is correctly passedinv.RunIDin both cases. ✓RunnableAgentstruct (run/agent.go:16-55): HasSkillPalette []stringandSubAgentPalette []stringfields as consumed. ✓llm.DefineToolcall form: The same package (run/submit.go:76) already callsllm.DefineTool[submitArgs](...)with an explicit type parameter. The calls inpalette.goomit the explicit type parameter, relying on Go's type inference from the closure argument — this is valid Go and consistent with usage intool/registry_test.go:26. ✓delegationResultlogic: The status check (status != "" && status != "ok") correctly surfaces non-ok child statuses (timeout, cancelled, budget_exceeded) to the parent while still passing partial output. ✓name := namecorrectly shadows the loop variable for each closure.invandpaletteare function parameters, not loop variables, so they are already properly captured. ✓palette == nilchecks the interface value; the testTestNoPaletteNoDelegationToolsconfirms the nil-interface case works. ✓fmt.Errorf("... %w", ...), preserving the error chain. ✓No logic bugs, no signature mismatches, no incorrect data flow. The change is semantically correct for its stated purpose.
🧯 Error handling & edge cases — No material issues found
🧯 Error handling & edge cases — Verdict: No material issues found
After reading the full source files (
run/palette.go,run/executor.go,run/ports.go,run/agent.go) and cross-referencing thePaletteSourceinterface andRunnableAgentstruct, I find no error-handling or edge-case defects in this change.What I verified:
addDelegationToolsreturnsnilimmediately (line 23–25). The executor calls it unconditionally; the nil-guard makes this safe.SkillPalette/SubAgentPalette→ thefor rangeloops simply don't execute. No delegation tools are added, which is the documented behavior.box.Addfailure (e.g. duplicate tool name) → the error is propagated up, the executor catches it, setsres.Err, callsfinishAuditwith"error"status, and returns. No resource leak or partial state.InvokeSkill/InvokeAgentreturning an error → the closures return(nil, fmt.Errorf(...)), which the tool framework treats as a tool error. The model sees the failure."ok"status with empty output →delegationResultstill produces a meaningful string like[skill "x" ended with status "timeout"]\n(the%sverb on an empty string is safe).name := namecapture inside the loop is correct (explicit shadow, safe for all Go versions).InvokeAgentcall passesnil, nilfortoolsSubsetandonEvent— both are valid zero values for slice and func types respectively; the interface contract permits them.args.Inputscan be nil if the model omits the field —map[string]anyaccepts nil without issue.No test coverage for error paths (e.g.
InvokeSkillreturning an error, or a non-ok status) — but this is a test-coverage gap, not a code defect. The production code handles these paths correctly.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 1s
🪰 Gadfly review —
glm-5.2:cloud(ollama-cloud)Verdict: No material issues found — 3 reviewers: security, correctness, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens, I traced the trust model and tainted inputs in
run/palette.go:"skill__"+name,"agent__"+name) are built fromra.SkillPalette/ra.SubAgentPalette— these are entries onRunnableAgent, i.e. host/agent config, not model- or caller-controlled input. The model only selects which pre-registered tool to call; it cannot inject arbitrary names at dispatch time. Duplicate-name collisions are caught bybox.Add(t)(which errors and aborts the run). VerifiedRunnableAgentfields atrun/agent.go:44-46.args.Inputs map[string]any,args.Prompt string) are model-produced JSON/strings, which is the standard executor threat model — the agent loop already trusts the model to author tool arguments. They're forwarded verbatim topalette.InvokeSkill/palette.InvokeAgent(verified signatures atrun/ports.go:158-167), which own their own input validation. No extra validation is expected at this thin wiring layer.inv.CallerID/inv.ChannelID/inv.RunID(as parentRunID) directly from the current invocation — there's no privilege change or escalation; the child runs under the same caller identity. Consistent with the doc comment.agent__delegation passes empty overrides andnilfortoolsSubset, so the sub-agent uses its own default tool set — a config choice, not a security gap (no blanket host-tool exposure beyond what the sub-agent is already configured with).No authn/authz gap, injection vector, SSRF, or secret leakage introduced by this change. The one thing I could not confirm is whether
llm.DefineTool/Toolbox.Addrejects names containing characters that would break the tool namespace (e.g. embedded__or control chars), but since the names are config-sourced rather than attacker-controlled, that would at most be a robustness nit, not a material security issue — and I'll leave it to the correctness lens rather than assert it unverified.🎯 Correctness — No material issues found
VERDICT: No material issues found
Verified against the checked-out code:
run/palette.govsrun/ports.go:158-167):InvokeSkill(ctx, callerID, channelID, name, inputs, parentRunID) (output, runID, status, err)— the palette.go call (palette.InvokeSkill(ctx, inv.CallerID, inv.ChannelID, name, args.Inputs, inv.RunID)returningout, _, status, err) matches positionally.InvokeAgent's 10-arg call (ctx, inv.CallerID, inv.ChannelID, name, args.Prompt, inv.RunID, "", "", nil, nil) matches the declared signature order (callerID, channelID, name, prompt, parentRunID, modelTierOverride, promptPrepend, toolsSubset, onEvent). ✓run/executor.go:105):inv.RunIDis the current run's id (Result{RunID: inv.RunID}), so passing it asparentRunIDto the child delegation is correct per the design ("child of the current run"). ✓invis a value parameter (not a pointer), andname := namerebinds the loop var — no aliasing across iterations. The tools are added before the run context exists; the closures reference the stableinv/palette/name, not the loop variable. ✓addDelegationToolsreturns nil immediately whenpalette == nil(run/palette.go:23); emptySkillPalette/SubAgentPaletteslices simply skip both loops. Matches theTestNoPaletteNoDelegationToolsexpectation. ✓delegationResultstatus handling (run/palette.go:65): non-ok (and non-empty) status wraps the partial output so the parent agent can react; ok/empty returns rawout. Behavior is as documented. ✓Unverified (not pursued): the exact declaration of
llm.DefineTool— grep did not locate it in the vendored dep, but the call form matches the existing, working usage inrun/submit.go:76(llm.DefineTool[submitArgs]), so compile-correctness is consistent with established usage.No correctness bug found in this lens.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling & edge-cases lens, this change is clean:
run/palette.go:23): earlyreturn nilwhenpalette == nil, so the no-PaletteSource/ empty-palette paths produce no tools and no error. Verified the executor callsite (run/executor.go:173) passese.cfg.Ports.Palettedirectly and treats the returned error as fatal — consistent with the other fatal build failures above it (build toolbox).run/palette.go:32-34,49-51): whenInvokeSkill/InvokeAgentreturns a non-nilerr, it is wrapped (with%w) and returned to the loop as the tool's(nil, err)result — not swallowed. The non-error path surfaces a non-okchild status viadelegationResultwhile still passing partial output (run/palette.go:65-69), so timeout/cancel/budget-stop children are visible to the parent agent rather than silently dropped.run/palette.go:66):status == ""is treated as the "returnoutunchanged" branch (same as"ok"), so a child that reports an empty status doesn't get mislabeled as a failure. Correct edge-case handling.args.Inputsis a plainmap[string]any; if the model omitsinputs, it's nil and passed through toInvokeSkill— no nil-deref here. Whether the palette tolerates nil inputs is that layer's contract, not this one's, so no defect inrun/.inv/palette; they allocate no resources requiringdefer/rollback. The toolbox/runCtxlifecycle is already managed by the executor.Nothing material to flag in this lens.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 0s
Pull request closed