Converge worker runtimes: one poll loop, one handler interface - #94
Merged
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
The poll→execute→update loop was hand-copied four times in cmd/worker.go, none
of the copies having backoff, context cancellation, or tests. This adds the
shared implementation the flavours will move onto.
The loop:
- backs off between empty or failed polls, interruptibly. Without this the
four cmd/worker.go loops spin: a test that removes the backoff records 36
million polls in 220ms against 10 with it.
- fails a task individually when it cannot be converted, rather than
discarding its whole batch
- reports results on a context detached from the loop, so work finishing
during Ctrl-C is not abandoned
- turns a handler panic into a failed task instead of a dead worker
Status is an open string type rather than an enum: JavaScript workers forward
whatever their script returns, and FAILED_WITH_TERMINAL_ERROR is documented and
in use. Handler returns only a Result, with no error alongside it, because the
flavours shape failures differently and workflows can observe the difference.
ToTaskResult is a pure function so the mapping most at risk of drifting during
the port can be table-tested against all three pre-existing result shapes.
StdioHandler and GojaHandler carry the two execution models over unchanged,
including the quirk where an unrecognised stdio status reports
"invalid status from worker: FAILED" rather than naming the offending value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deletes the four duplicated poll loops in favour of taskworker.Worker. Each
flavour is now just a handler plus poll options:
worker stdio → StdioHandler
worker js → GojaHandler
worker remote (PYTHON) → StdioHandler
worker remote (NODEJS) → GojaHandler
Behaviour preserved: the result shape of each flavour, --count batch semantics
including the next poll waiting on the slowest task, the credential environment
handed to child processes, stdout/stderr echoing, and --verbose banners.
Deliberate changes:
- the four loops gain backoff, so a poll error no longer spins the CPU
- Ctrl-C and SIGTERM now shut a worker down and exit 0, where the loops were
previously for{} bodies that only died when the process was killed
- in-flight child processes are cancelled on shutdown, since the exec context
now derives from the loop's
worker js and worker remote gain --poll-timeout and --exec-timeout, matching
worker stdio. --timeout stays as a hidden deprecated alias for --poll-timeout.
This resolves #91: worker remote fed one --timeout value to both the poll
(milliseconds) and the execution budget (seconds), so --timeout 100 meant 100ms
and 100s at the same time. --exec-timeout defaults to 100s on remote to keep a
hanging worker bounded as it was before.
injectUtilities, httpRequest and the two result-shape tests move to
internal/taskworker with the code they cover.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces skillworker's own loop, TaskRunner and conductorRunner with an adapter
onto taskworker, leaving this package owning just the tool logic and the
{"result": …} envelope.
ToolHandler is kept rather than collapsed into taskworker.Handler. Seven
constructors return it, three functions pass it as map[string]ToolHandler, and
ten tests assert against it; adapting instead is one line at startSkillWorkers
and avoids duplicating the result wrap across every tool. It is a cohesive local
shape — tool logic in, raw JSON out — and stays this package's vocabulary.
Preserved: the {skillName}__{tool} task type, the conductor-cli worker id, one
task per poll, and wrapResult's fallbacks — non-JSON output carried through as a
string, and nil output yielding {"result": null}.
The loop and runner tests are deleted rather than kept: they covered the code
being removed, and taskworker's tests cover its replacement. handlers_test.go is
untouched and remains the regression guard for the tool logic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Skill workers were undocumented: "skill" appeared zero times in README.md and
CLAUDE.md, the agentspan docs have no skills section, and conductor-skills
documents SDK-based workers instead. WORKER_SKILL.md covers the directory layout,
the {skillName}__{tool} task types, the argv/stdout tool contract, and using a
skill tool from a plain workflow with no agent involved.
CLAUDE.md had no worker or skill commands at all, so an assistant working in this
repo could not discover either. Both now have command tables, and the three
result contracts are written down side by side.
Flag corrections: worker js and worker remote now document --poll-timeout and
--exec-timeout, with --timeout noted as deprecated. WORKER_STDIO.md's comparison
table still called the command "Generic Workers (exec)", a name that predates the
rename to stdio.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three real regressions in the convergence, plus the gaps that let them through.
Ctrl-C could be pressed once and then never again. signal.NotifyContext keeps its
channel registered after firing, so the default disposition never returns and
every later signal is swallowed. Combined with handlers that cannot be
interrupted — a goja script has no vm.Interrupt wired, and a subprocess whose
grandchild holds the captured pipes open blocks cmd.Run — a worker could become
unkillable by anything short of SIGQUIT. Verified against a reproduction:
NotifyContext survives two SIGTERMs, the replacement exits on the second. The
first signal now asks the loop to drain and the second force-exits.
Poll errors were silently swallowed. The four deleted loops each logged them; the
shared loop discarded err, so a worker with a bad token printed its banner and
then nothing, at any log level. The backoff had traded a noisy hot loop for a
silent one. Restored, along with "Polled N task(s)" and the no-tasks debug line.
The two places behaviour actually changed had no tests. GojaHandler and
gojaResultToResult had none at all, and wrapResult lost the coverage it had when
skillworker's runner test was deleted — the spec had named those as the guard for
the undocumented skill contract. Added tests for the js result shapes (including
an unknown status passing through and the output.error failure shape), concurrent
Handle safety for both handlers, and the {"result": ...} envelope with its
non-JSON and nil fallbacks.
Smaller fixes:
- worker js advertised --exec-timeout in three docs and discarded it; goja has
no timeout mechanism, so the flag is gone rather than silently ignored
- worker stdio accepted the hidden --timeout alias and ignored it, because
execWorker read flags directly instead of going through workerPollFlags
- --verbose printed Go field names and post-normalisation values, so a worker
returning an unrecognised status could not see what it had actually sent
- WORKER_SKILL.md named a task type that is never polled: the tool is
list_workspace_files, not list_workspace
- WORKER_SKILL.md implied stderr is separate; executeScript gives the script
one buffer for both, so stderr lands inside {"result": ...}
- README still claimed two worker types, and the new Skill Workers section had
been inserted in front of the JavaScript quick example, orphaning it
- dropped the now-unused TaskResult type from cmd/worker.go
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebase onto main brought in #99's CLAUDE.md, which documents workers and skills independently of the sections this branch added — leaving two "Worker Commands" and two "Skill Commands" sections. Keeps #99's versions, which cover more commands and flags, and folds in what only this branch had: the shared-loop note, the three result contracts side by side, the shutdown and child-environment behaviour, the {skillName}__{tool} task types with their argv/stdout contract, and the WORKER_SKILL.md links. Also corrects #99's flag documentation to match this branch: worker js and worker remote take --poll-timeout rather than --timeout, worker js has no --exec-timeout because a goja script cannot be interrupted, and remote's --exec-timeout defaults to 100s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mp-orkes
force-pushed
the
fix/converge-worker-runtimes
branch
from
August 5, 2026 18:15
e5b04fb to
e4d21db
Compare
Second review found that graceful shutdown was reporting failures the worker
inflicted on itself. StdioHandler derived the child's context from the loop's, so
the first Ctrl-C SIGKILLed every running child; runAndParse turned that into
FAILED / "worker execution failed: signal: killed", and because results are
delivered on a context that outlives cancellation, the server received it.
That is worse than the behaviour it replaced. Previously Ctrl-C killed the CLI
before any update, leaving the task IN_PROGRESS for the server to requeue after
responseTimeoutSeconds. Now a clean shutdown consumed one of the task's retries,
and failed the workflow outright when retryCount was 0. It also contradicted both
the loop's own doc comment and CLAUDE.md, which promise that Run returns once the
in-flight batch finishes.
The child is now detached from the loop context, so a running task completes and
reports its real result. Its execution timeout still applies. A task that will
never finish is covered by the second interrupt, which exits the process.
Verified end to end: SIGTERM while a 3s task is in flight now yields COMPLETED
with the worker's own output, where it previously yielded FAILED.
Also from the same review:
- skill run and skill serve still used signal.NotifyContext, the pattern this
branch replaced precisely because it swallows every signal after the first.
Both now use interruptWithEscalation, so a stream or tool script that ignores
cancellation cannot leave the process unkillable.
- workerPollFlags had no tests, though it is where the #91 fix lives. Added
coverage for the --timeout alias precedence, the exec-timeout default that
preserves remote's old effective behaviour, and the flag surface each command
exposes.
- "Polled N task(s)" moves to Debug. skill run starts one loop per tool type and
streams agent output to the same terminal, so an Info line per poll buried the
stream. Poll errors stay at Error — a silently idle worker is the failure that
logging exists to surface.
- printResultBanner took a status parameter that duplicated Result.Status.
- Corrected the addPollTimeoutFlags comment: the alias is registered on every
worker subcommand, including stdio, which never shipped with --timeout.
- Documented that InputData yields "null" for an empty-but-non-nil input map,
where marshalling the map directly gave "{}".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mp-orkes
added a commit
that referenced
this pull request
Aug 5, 2026
Resolves the conflicts introduced by #94 (worker runtime convergence), which landed after this branch was cut and touched the same skill files this branch deletes. Conflicts, all resolved in favour of the deletion: - cmd/skill_run.go and internal/skillworker/worker.go — modified by #94, deleted here - CLAUDE.md — #94 rewrote the Skill Commands section that this branch removes #94 also added files this branch could not have known about, so the deletion had to be extended: - internal/skillworker/adapter.go and adapter_test.go — the adapter that put skill tools on the shared poll loop - WORKER_SKILL.md — documents skill workers - the Skill Workers section, TOC entry and WORKER_SKILL.md links in README.md, WORKER_JS.md and WORKER_STDIO.md, which this branch did not touch - the skill row in CLAUDE.md's result-contract table, leaving the two remaining flavours internal/taskworker stays: it is the shared poll loop behind worker stdio, js and remote, and does not depend on skills. One piece of it does go, though — Task.InputData existed only so the skill adapter could hand a tool its inputData rather than the whole task. Both remaining handlers use Raw, so it and its tests are removed rather than left as an API with no caller. Verified: build and go vet clean, full suite green, `conductor skill` reports unknown command, worker js/stdio/remote/list-remote intact, and a stdio worker still completes a task end to end against a local server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
ambiorix2099
added a commit
that referenced
this pull request
Aug 5, 2026
…ected suites and a local-OSS-server CI job (#106) * test: add E2E coverage for agent, deploy, doctor, config and server; tag all suites by tier Adds 48 E2E tests (129 -> 177) covering five commands that previously had no E2E coverage at all, and introduces a bats tag scheme so the suite can be selected by tier and by server venue. New suites: - agent.bats (17) definition scaffolding, CRUD, execution search, live runs - config.bats (9) profile save/list/delete and --profile precedence - doctor.bats (7) runtime, server and AI provider reporting - deploy.bats (7) Python agent discovery and deployment - server.bats (8) local server status/logs and mutual-exclusion guards Tagging uses negative selection: suites carry a tier, and only the exceptions are marked. bats file_tags can be added to by test_tags but never subtracted, so tagging every test positively would have been unmaintainable. tier:pr | tier:nightly when it runs orkes-only requires Orkes/Enterprise oss-only requires a local OSS server needs:llm requires a provider credential needs:agentspan requires the agentspan Python package needs:timeout requires GNU timeout(1) Selections: tier:pr,!orkes-only 109 OSS venue tier:pr,!oss-only 157 Enterprise venue tier:nightly 12 5 needs:llm + 7 needs:agentspan Correctly classifies tests that were previously venue-naive. auth.bats is Orkes-only by design: it asserts that unauthenticated calls fail, which is not true against OSS where anonymous access is legitimate. task.bats tests 9, 10, 13 and 14 exercise task signal/signal-sync, which OSS rejects outright. Known-broken behaviour is encoded as a test of the *correct* behaviour plus a skip naming the issue, so the gap is executable documentation and un-skipping is a one-line change: #96 (agent compile), #98 (default profile), #101 (schedule pause/resume on OSS), #103 (stale model strings). #97 and #102 are covered the same way in the nightly tier. server.bats deliberately omits the start/stop lifecycle: the CLI tracks a single instance in server-state.json, so starting or stopping there would clobber the server the rest of the run depends on. It asserts the read-only commands and the guards instead, which also avoids triggering a 435 MB download. LLM assertions are structural only -- execution ids and terminal status, never generated text -- so provider variance cannot cause flakes. Verified against server 3.32.0-rc.23 on local OSS: 109 passed, 0 failed, 6 skipped, every skip carrying a reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: add local-OSS-server and nightly E2E jobs; recover orphaned api_gateway tests Replaces the single e2e-test job with three tag-selected jobs, so the suite is chosen by tier and server venue rather than by a hand-maintained file list. e2e-enterprise PR remote Enterprise; Orkes-only surface e2e-local-server PR pinned OSS jar; OSS paths, `server`, and the RC server e2e-nightly cron live LLM runs + agentspan deploy tests Per-PR test executions go from 111 to 266. Recovers api_gateway.bats. Its 18 tests existed but appeared in no job's file list, so they ran nowhere; tag selection picks them up automatically, and future suites will be too. e2e-local-server is the job that closes the structural gaps the Enterprise job cannot: it exercises the OSS code paths (CONDUCTOR_SERVER_TYPE was pinned to Enterprise everywhere, so the OSS schedule support added in #86 was untested), covers the `server` command, and validates the CLI against the pinned release-candidate server rather than a remote of unknown version. Details worth noting: - The server is started from a scratch directory, not the repo, because `server start` writes its SQLite database relative to the working directory with no flag to override (see #104). - The 435 MB jar is cached on the pinned version, so only the first run per bump pays the download. - Agent executions run inside the server, not the CLI, so ANTHROPIC_API_KEY is set on the server-start step in the nightly job. Setting it only on the bats step would leave the server unable to reach the provider. - The `secrets` context is unavailable in step-level `if`, so the LLM gate reduces the secret to a boolean in job-level env and tests that. Missing credentials produce a warning annotation, not a failure, so forks do not report spurious red. - auth.bats gains an `unauthenticated` tag. It needs a secured server reached *without* credentials, so it must be excluded from the authenticated run or its "should fail" assertions fail for the opposite reason. - Both server-backed jobs assert that bats supports --filter-tags (>= 1.8.0) before running, since silently ignoring the filter would run the wrong tests. - upload-artifact does not expand '~', so the server log is copied into the workspace before upload. workflow_dispatch inputs toggle each job independently; the nightly tier is off by default on manual runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: add CONTEXT.md glossary, testing-strategy ADR, and an E2E README Creates the repo's domain glossary and records the testing decisions taken while building out E2E coverage. CONTEXT.md pins vocabulary that was previously used loosely, including two distinctions that caused real confusion in this work: - Definition vs Execution — the CLI separates registered templates from their running instances, and the command surface mirrors that split. - Agent vs Worker — an Agent's turns are executed by the *server*, a Worker runs on the machine that polls. This is why the server, not the CLI, needs the model provider credential; getting it backwards produces provider auth failures that look like CLI bugs. It also fixes the testing terms coined here: tier (when a test runs), venue (which distribution it is valid against), OSS-safe, and known-broken guard. Venue is deliberately defined as validity rather than location — a test asserting that anonymous access is refused is not merely misplaced against OSS, it is wrong. ADR-0001 records why bats with tag selection, and why not the alternatives. k6 was explicitly proposed and is rejected on capability, not taste: its JS runtime has no subprocess API, so it cannot invoke a CLI at all. Worth recording because it will otherwise be suggested again. Also records why tagging is negative rather than positive — bats file_tags can be added to but never subtracted, so tagging every test positively would have been unmaintainable. test/e2e/README.md gives the runnable form: prerequisites, the exact selection commands per venue and tier, the tag table, and the conventions for adding suites. All four selection counts in it were verified against the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: do not schedule the nightly E2E tier yet; keep it manual-only Removes the cron trigger. The tier:nightly suites spend model tokens on every run, and an unattended schedule reporting to nobody is worse than no coverage — it accrues cost and trains people to ignore a red job. The job now runs only on an explicit workflow_dispatch with run_nightly=true (default false). The cron line is kept as a comment alongside the one other change needed to enable it, so turning it on later is a two-line edit rather than archaeology. Keeps the tag name tier:nightly. It states the intended cadence, and renaming it to tier:manual would churn 12 tests and three documents to describe a temporary state. CONTEXT.md, ADR-0001 and the E2E README now say explicitly that the name is intent rather than current wiring, so the gap between the two is documented rather than misleading. Nothing about test selection changes: 109 OSS, 12 nightly, unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: build the Conductor server from conductor-oss main instead of a pinned RC Releases are cut from conductor-oss/conductor main, so the CLI should be validated against main rather than a tagged release candidate. No artifact is published from main — Maven Central carries only tagged RCs and the S3 'latest' jar has not moved since 3 June — so the E2E jobs now check out the server repo and run `:conductor-server:bootJar`. Verified locally before switching: built main (8 commits ahead of v3.32.0-rc.23) and re-ran the OSS selection against it. 109 passed, 0 failed. The one CLI-facing change in those 8 commits, aggregate token usage on the execution payload, does not affect CLI parsing. Also re-verified every filed defect against a main build. None is resolved by main: #101 schedule pause/resume 405; probe confirms GET->405, PUT->200 #96 agent compile 400 agentConfig is required #97 execution --since returns nothing while --name returns the row #98 config save default profile name is required #102 agent stream still running at 20s That is worth recording because main *does* contain "fix: 404 on SSE stream for nonexistent execution IDs", which reads like #102 but is already in rc.23 and addresses nonexistent rather than terminal executions. The scheduler is untouched between rc.23 and main — zero files — and SchedulerResource still declares @PutMapping, so #101 cannot have been fixed there. Consequence to be aware of: `conductor server start` can only download published versions, so a source-built jar must be launched with `java -jar`. There is then no CLI-managed pid file and the six server-dependent tests in server.bats skip rather than run. They skip with a stated reason, and the guard behaving this way is why the run stayed green rather than reporting six false failures. The costs of building — a Gradle build per PR run, and this repo's CI becoming sensitive to the server repo's build health — are tracked in #105 along with the server.bats gap and the apparently stalled S3 'latest' publish. The env var is now a git ref rather than a version, with a TODO pointing at that issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: drop `conductor code` from CLAUDE.md after its removal in #95 PR #95 removed the `code` command but left its documentation behind, so CLAUDE.md still described a command that now returns `unknown command "code"`. Removes the table rows and the flag list. Also corrects the help-group listing, which is now verified against `--help` output rather than asserted: `worker` belongs to Development only, not Conductor Management, and `skill` is gone. The Conductor Management line had carried `worker` incorrectly since the section was written. Noticed while rebasing onto main after #94, #95 and #100 merged. Small and adjacent to this branch's other doc work, but a separable concern — happy to split it out if preferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: skip agent and api-gateway suites where the server lacks those capabilities First CI run surfaced 20 failures in the Enterprise job, all from two deployment capability gaps rather than CLI defects: agent (5) "Agents API is not available on this Conductor server" api_gateway (15) 404 "No static resource api/gateway/config/auth" The Enterprise server CI targets has neither the Agents API nor API Gateway enabled. The CLI reports both clearly and exits non-zero, which is correct behaviour; the tests were simply assuming capabilities that deployment does not have. Adds capability guards that skip with a stated reason, matching the pattern already used by require_running_server, require_agentspan and require_llm. Both were verified not to over-skip: on the local OSS server, which has both capabilities, all 109 tests still run and pass. api_gateway's guard goes in setup() so it covers all 18 tests in one place. agent's is per test, so the three offline `agent init` tests keep running even where the server has no Agents API. This also corrects an assumption in the previous commit. api_gateway.bats was described as orphaned by oversight; it now looks likely it was dropped from the CI list deliberately, because it fails against this server. Recovering it into a tag-selected run reintroduced those failures. The guard is the right fix either way — the suite now runs wherever API Gateway exists and skips loudly where it does not, rather than being silently absent from every job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: add ADR-0002 recording why E2E builds the server from source Captures the decision @mp-orkes raised in review of #106: why the E2E jobs build conductor-oss/conductor from main rather than pinning a published version with `conductor server start --version <x>`. States plainly that pinning was the preferred option and the original plan, and that it was not taken for one narrow factual reason: no artifact is published from main. Maven Central carries tagged releases only, and the S3 'latest' jar has not moved since 3 June, so a pinned version cannot validate the code the release is cut from. Also records that in this instance pinning would have been adequate — main was 8 commits ahead of rc.23, none CLI-facing — but that this is a property of that particular gap rather than a guarantee, and only knowable after the fact. Consequences are listed without softening: a Gradle build per PR, CI becoming sensitive to the server repo's build health, non-reproducible runs, and six server.bats tests skipping because `server start` cannot launch a source-built jar. Includes explicit triggers for reverting to a pin, and notes the earlier revision of #106 already implemented the pinned form, so reverting is recoverable from history rather than a redesign. ADR-0001 now points here instead of summarising the trade-off inline. Tracked for revisit in #105. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #92. Closes #91.
Collapses the four hand-copied poll loops in
cmd/worker.goand the fifth ininternal/skillworkerinto one shared loop ininternal/taskworker. Each worker flavour is now aHandlerplus poll options.What changed
worker stdiocmd/worker.go:507StdioHandlerworker js:200GojaHandlerworker remotePYTHON:1171StdioHandlerworker remoteNODEJS:1030GojaHandlerskillworker.AsTaskHandlerOnly
skillworker's loop had backoff, cancellation, or tests; the four incmd/worker.gohad none.Behaviour changes (deliberate)
continued on poll errors with no sleep. A test that removes the backoff records 36 million polls in 220ms against ≤10 with it — that was the CPU burn on a server outage.for {}bodies that only died when the process was killed, so the exit code changes from 130 to 0.FAILED / signal: killed— a failure it inflicted on itself, which the server counts against the task's retries.worker remotegains a real--exec-timeout. Fixes worker remote (PYTHON): single --timeout flag used as both poll timeout (ms) and exec timeout (s) #91: one--timeoutvalue was fed to both the poll (ms) and the execution budget (s), so--timeout 100meant 100ms and 100s.--exec-timeoutdefaults to 100s onremoteso a hanging worker stays bounded as before.Everything else is preserved: each flavour's result shape,
--countbatch semantics, credential forwarding, stdout/stderr echoing,--verbosebanners.Preserving the three result contracts
Unifying them is breaking, so it is not done here — workflows can observe the differences.
ToTaskResultis a pure function table-tested against all three:jsoutput.error, empty reasonstdio--worker-idreasonForIncompletion+ logsconductor-clireasonForIncompletionTwo related decisions fell out of that:
Statusis an open string type, because js forwards whatever its script returns andFAILED_WITH_TERMINAL_ERRORis documented and in use; andHandlerreturns only aResult, with noerroralongside it, so there is exactly one way to express failure.Review notes
ToolHandleris kept, not collapsed intotaskworker.Handler. Seven constructors return it, three functions pass it asmap[string]ToolHandler, and ten tests assert against it — adapting instead is one line atstartSkillWorkersand avoids duplicating the{"result": …}wrap seven times.invalid status from worker: FAILEDrather than naming the offending value, becauseStatusis overwritten before the message is formatted. A test pins the current wording; fixing it is a one-liner best kept out of a refactor.skillworker's loop/runner tests are deleted, not ported — they covered the removed code.handlers_test.gois untouched and remains the guard for tool logic.cmd/task.go'sBatchPollis untouched —task pollis a one-shot primitive, not a loop.StdioHandlerandskillworker'sexecuteScriptremain two subprocess spawners: one pipes JSON on stdin, the other passes argv and reads bare stdout. Merging them needs the contract question settled first.Found while testing
--exec-timeoutdoes not bound a process tree. It signals the direct child, thenRunwaits for the captured pipes to close, so a worker that forks a long-running subprocess keeps them open. Measured: 5.02s against a 200ms timeout. This is pre-existing, not introduced here — bounding a tree needs process groups and is platform-specific. Documented with a skipped test atstdio_test.go; worth its own issue.How to test
go test ./... -raceNew tests: 22 loop/mapping cases (backoff, mid-backoff cancellation, per-task conversion failure not discarding batch peers, panic isolation, update-after-cancel), 13 stdio handler cases, plus the goja and result-shape tests moved from
cmd/worker_test.go.Verified end to end against local OSS Conductor:
TASK_IDreaching the childreasonForIncompletion: deliberate failure, logs retrievable at/tasks/{id}/log,outputDataempty (noerrorkey — the stdio shape, not the js one)worker jswith$.taskintact, driven through the deprecated--timeoutalias, which warns and still worksskill servefrom a plain workflow →{"result": "Hello SharedLoop\n"}worker stdioandskill servecleanlyNot included
Contract unification, merging the two subprocess spawners, long-lived worker processes, a worker pool,
vm.Interruptfor goja, and container images (#64). The docs stage coversWORKER_SKILL.md(new — skill workers were undocumented everywhere) and worker/skill sections inCLAUDE.md, which had neither.🤖 Generated with Claude Code