From 372ef8ff40991644ec2654c61328f31779f4ad21 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 17 Aug 2026 23:35:36 -0600 Subject: Validation-pass fixes; DESIGN.md describes the shipped seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit jobs: a child whose wake pipe cannot be armed is refused up front instead of started into a state nothing can wake (the luv-less drain loop survives only for hosts without a loop); a closing child keeps its concurrency slot and its id's exclusivity until the pump actually exits, so teardown can no longer over-admit new children or let two turns share one session file. Resume-metadata reads honor the plain-error contract on a malformed store. workflow: the built-in schema subset validator is the only validator — the jsonschema probe made behavior depend on an undeclared rock (see rockspec: that dependency is deliberately rejected); dead exports and the unreachable half of the structured-output guard are gone, keeping the empty-arguments provider case. DESIGN.md's seam sections now describe the shipped division: binding-level async jobs and tool control, host-level resolve_model/ExtHost/turn events/ component handles, rock-level policy; protocol bodies run to completion on the loop thread and cannot yield, cancellation is scoped to the stream that opened it, and a child's compaction leaves protocol sessions alone. --- subagents/jobs.lua | 69 ++++++++++++++++++++++++++++++++------------------ subagents/luatool.lua | 3 --- subagents/progress.lua | 10 +++----- subagents/spawn.lua | 10 +++----- subagents/workflow.lua | 47 ++++++++++++---------------------- 5 files changed, 69 insertions(+), 70 deletions(-) (limited to 'subagents') diff --git a/subagents/jobs.lua b/subagents/jobs.lua index bab8f30..3f9a8af 100644 --- a/subagents/jobs.lua +++ b/subagents/jobs.lua @@ -14,10 +14,15 @@ -- Waiting parks the CALLING coroutine — the tool handler's — and a poll -- callback resumes exactly that coroutine. subagents/workflow.lua's rule that -- a workflow callback must never run on a nested coroutine follows from this: --- the parked thread is the one the resume goes to. Where there is no coroutine --- to park (a plain script) or a job with no wake pipe, the identical drain --- runs in a loop instead — same gate, same settle bookkeeping, only the wait --- differs. +-- the parked thread is the one the resume goes to. +-- +-- The wake pipe is therefore mandatory wherever luv is: a started job whose +-- pipe or poll could not be armed is a start failure, not a degraded job. +-- The drain-and-sleep fallback below only ever serves a luv-less host or a +-- caller with no coroutine to park (a plain script), because it cannot serve a +-- child that dispatches tools: those tool batches are posted to this very +-- thread, so a loop that never returns to uv would wait for work only it can +-- do. A visible "could not be armed" beats an unrecoverable hang. -- -- A settled result is read and cached the moment it appears, because -- `job:close()` frees it. Jobs are otherwise left open until close_all() ends @@ -127,7 +132,9 @@ local function settle(handle, raw) end end handle.settled = result - if handle.state == "running" then + -- A closing job still holds its slot: close_all could not join its pump, so + -- the child is still up. It gives the slot back here, once it really exits. + if handle.state == "running" or handle.state == "closing" then running = running - 1 end handle.state = "settled" @@ -142,6 +149,21 @@ end local function launch(handle) handle.fds = open_pipe() + + -- Arm the wake before the child exists, so a host that cannot give us one + -- never leaves a pump running with nothing to drain it. + if ok_uv then + if handle.fds == nil then + return false, "the subagent wake pipe could not be armed" + end + local armed, poll = pcall(uv.new_poll, handle.fds.read) + if not armed or not poll then + close_pipe(handle) + return false, "the subagent wake pipe could not be armed" + end + handle.poll = poll + end + local ok, job, err = pcall(handle.spec.build, handle.fds and handle.fds.write or nil) if not ok then close_pipe(handle) @@ -156,15 +178,11 @@ local function launch(handle) handle.state = "running" running = running + 1 - if handle.fds and ok_uv then - local armed, poll = pcall(uv.new_poll, handle.fds.read) - if armed and poll then - handle.poll = poll - poll:start("r", function() - drain(handle) - wake() - end) - end + if handle.poll then + handle.poll:start("r", function() + drain(handle) + wake() + end) end return true end @@ -265,7 +283,7 @@ end -- -- startspec = { -- build = function(wake_fd) -> job | nil, err -- calls agent:run_async --- label = string?, id = string?, one_shot = boolean? +-- id = string? -- on_event = function(event)? -- one call per drained run_async event -- shape = function(raw) -> result? -- maps the settled run_async result -- onto the caller's result table @@ -279,9 +297,7 @@ function M.start(spec) local handle = setmetatable({ spec = spec, - label = spec.label, id = spec.id, - one_shot = spec.one_shot == true, state = "queued", }, handle_mt) live[#live + 1] = handle @@ -306,15 +322,16 @@ function M.start(spec) return handle end --- True while a child with this id has a turn in flight. A child cancelled by --- close_all is not one: its turn belongs to the turn that ended, and the next --- one must not be refused because that pump has not finished exiting yet. +-- True while a child with this id has a turn in flight, including one that +-- close_all cancelled but whose pump has not exited yet: it still owns that +-- child's session file, and a second writer over the same file loses data. The +-- refusal is transient — the pump's settle clears it on the next drain. function M.active(id) if id == nil then return false end for _, handle in ipairs(live) do - if handle.id == id and handle.settled == nil and handle.state ~= "closing" then + if handle.id == id and handle.settled == nil then return true end end @@ -439,8 +456,10 @@ function M.close_all() end end - -- Whatever is still unsettled stays live so its poll (and any fallback - -- drain) still reaches it; the next close_all sweeps up what settled since. + -- Whatever is still unsettled stays live with its wake poll still armed — + -- start() refuses a job that has none — so the byte its pump writes on the + -- way out still drives the settle that closes it and frees its pipe. The + -- next close_all sweeps up whatever settled since. local closing = {} for _, handle in ipairs(live) do if handle.job and handle.settled == nil then @@ -453,7 +472,9 @@ function M.close_all() end end live, queued, waiters = closing, {}, {} - running = 0 + -- Those children are still running against the same bound; each releases + -- its slot in settle() when its pump finally exits. + running = #closing end return M diff --git a/subagents/luatool.lua b/subagents/luatool.lua index 089c67b..8aaa5c9 100644 --- a/subagents/luatool.lua +++ b/subagents/luatool.lua @@ -54,7 +54,6 @@ local CHUNK_NAME = "subagents.lua" local M = {} M.max_jobs = MAX_JOBS -M.instruction_budget = INSTRUCTION_BUDGET local function shallow_copy(source, skip) local copy = {} @@ -139,8 +138,6 @@ local function format_return(value) return workflow.json_encode(value) end -M.format_return = format_return - -- Tool handler for `subagents.lua`. `profiles` is the discovered profile set -- from activation; when omitted the workflow API discovers it lazily. function M.handle(input, profiles) diff --git a/subagents/progress.lua b/subagents/progress.lua index 274e2a8..087a3c3 100644 --- a/subagents/progress.lua +++ b/subagents/progress.lua @@ -193,12 +193,10 @@ end -- `tool_call_complete` for one of our tools: claim that entry's component so -- the cards the handler is about to raise have somewhere to render. function M.claim(event) - -- The event is host userdata; a host that predates any of these fields - -- answers nil, and one that predates the whole object cannot be indexed. - local ok, name = pcall(function() - return event.tool_name - end) - if not ok or type(name) ~= "string" or name:sub(1, #TOOL_PREFIX) ~= TOOL_PREFIX then + -- The event is host userdata; an unknown field answers nil rather than + -- raising, and activation already refused a host too old to have these. + local name = event.tool_name + if type(name) ~= "string" or name:sub(1, #TOOL_PREFIX) ~= TOOL_PREFIX then return end if type(event.set_component) ~= "function" then diff --git a/subagents/spawn.lua b/subagents/spawn.lua index a83e442..b355d9a 100644 --- a/subagents/spawn.lua +++ b/subagents/spawn.lua @@ -230,8 +230,8 @@ local function read_stored(conv) local manifest for index = 1, #messages do if messages[index].role == "system" then - local metadata = conv:message_metadata(index) - if type(metadata) == "table" then + local got, metadata = try(conv.message_metadata, conv, index) + if got and type(metadata) == "table" then manifest = metadata break end @@ -241,8 +241,8 @@ local function read_stored(conv) local defaults = {} for index = #messages, 1, -1 do if messages[index].role == "user" then - local metadata = conv:message_metadata(index) - local mine = type(metadata) == "table" and metadata.subagents or nil + local got, metadata = try(conv.message_metadata, conv, index) + local mine = got and type(metadata) == "table" and metadata.subagents or nil if type(mine) == "table" then defaults.model = nonempty(mine.model) defaults.reasoning = nonempty(mine.reasoning) @@ -488,9 +488,7 @@ function M.spawn(spec) end local handle, start_err = jobs.start { - label = spec.label, id = id, - one_shot = one_shot, build = function(wake_fd) local job, err = agent:run_async { prompt = spec.prompt, diff --git a/subagents/workflow.lua b/subagents/workflow.lua index b1d3e46..3cf58d9 100644 --- a/subagents/workflow.lua +++ b/subagents/workflow.lua @@ -40,12 +40,14 @@ -- * Handles the callback never awaited are awaited ("all") after it returns, -- purely so no child is orphaned; those results are discarded. -- * Structured output is decoded from result.structured_json and validated --- against output.schema. Validation prefers the `jsonschema` rock and falls --- back to the small built-in subset validator below when it is absent -- --- that rock pulls in lrexlib-pcre, which needs a system PCRE and fails to --- build on stock macOS, and a failed rock install would otherwise take the --- whole extension down silently. A validation failure turns the result into --- status "failed"; it is never reported as a successful structured result. +-- against output.schema by the one validator below: the JSON Schema subset +-- a child's output tool actually uses, ignoring keywords it does not know. +-- There is deliberately no second, rock-dependent path -- `jsonschema` needs +-- lrexlib-pcre and a system PCRE that stock macOS lacks, so it is not a +-- declared dependency, and a validator picked by whether a rock happens to +-- be installed would make the same output pass here and fail there. A +-- validation failure turns the result into status "failed"; it is never +-- reported as a successful structured result. -- * The host seam is reached through `require("panto").ext` at call time, not -- aliased at load time, matching subagents/spawn.lua so a test can install a -- fake `panto` module before the first call rather than before the require. @@ -58,8 +60,6 @@ local M = {} local workflow_mt = { __name = "subagents.workflow" } -M.workflow_mt = workflow_mt - -- --------------------------------------------------------------------------- -- Host seam access -- --------------------------------------------------------------------------- @@ -117,16 +117,15 @@ local function json_encode(value) return tostring(value) end -M.json_decode = json_decode M.json_encode = json_encode -- --------------------------------------------------------------------------- -- Schema validation -- --------------------------------------------------------------------------- --- Built-in fallback validator: the JSON Schema subset that structured child --- output actually uses. Anything it does not understand is ignored rather than --- rejected, so an unrecognized keyword never fails a legitimate result. +-- The validator: the JSON Schema subset that structured child output actually +-- uses. Anything it does not understand is ignored rather than rejected, so an +-- unrecognized keyword never fails a legitimate result. local function is_array_like(value) local count = 0 for key in pairs(value) do @@ -259,23 +258,6 @@ local function check_schema(value, schema, path) return true end -local function validator_for(schema) - local ok, jsonschema = pcall(require, "jsonschema") - if ok and type(jsonschema) == "table" and jsonschema.generate_validator then - local generated_ok, generated = pcall(jsonschema.generate_validator, schema) - if generated_ok and type(generated) == "function" then - return generated - end - end - return function(value) - return check_schema(value, schema, "output") - end -end - -M.validate = function(value, schema) - return validator_for(schema)(value) -end - -- --------------------------------------------------------------------------- -- Result shaping -- --------------------------------------------------------------------------- @@ -312,8 +294,11 @@ local function shape_result(result, handle) return shaped end + -- An empty tool input is a real provider case (a chat-style provider + -- finalizes an argument-less call with ""), and it is not a validation + -- failure: nothing was produced to validate. local raw = shaped.structured_json - if type(raw) ~= "string" or raw == "" then + if raw == nil or raw == "" then return fail(shaped, "structured output missing: the child produced no structured result") end @@ -322,7 +307,7 @@ local function shape_result(result, handle) return fail(shaped, "structured output failed validation: " .. tostring(decoded)) end - local valid, message = validator_for(schema)(decoded) + local valid, message = check_schema(decoded, schema, "output") if not valid then return fail(shaped, "structured output failed validation: " .. tostring(message or "schema mismatch")) end -- cgit v1.3