Delete the skill command - #100
Merged
Merged
Conversation
added 5 commits
August 4, 2026 19:40
- rm cmd/skill.go, skill_register.go, skill_load.go, skill_run.go, skill_payload.go, skill_materialize.go - rm matching _test.go files
- rm internal/skill package (types.go, client.go, service.go, client_test.go) - settings.go: drop skill import + GetSkillService, reword agent-only comments
- root.go, transport.go, internal/transport: drop "and skill" from comments - internal/agent/service.go: note frameworkSkill kept for backward compat
- drop conductor skill from intro + Conductor Management group - delete Skill Commands section, skill list bullet
v1r3n
approved these changes
Aug 5, 2026
Contributor
|
I'm merging this after #94. |
mp-orkes
marked this pull request as draft
August 5, 2026 15:41
This was referenced Aug 5, 2026
Open
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>
mp-orkes
marked this pull request as ready for review
August 5, 2026 19:14
mp-orkes
approved these changes
Aug 5, 2026
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
3 participants
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.
Pull Request type
NOTE: Please remember to run
./gradlew spotlessApplyto fix any format violations.Changes in this PR
skillcommand tree:register/load/run/serve/list/get/pull/deletecmd/skill*.go,internal/skill/,internal/skillworker/, testsinternal/settings.go: dropGetSkillService+internal/skillimportcmd/root.go,cmd/transport.go,internal/transport/transport.go→ "agent" (types are shared, now agent-only)CLAUDE.md: remove### Skill Commandssection + all otherconductor skillmentionsinternal/agent/service.go'sframeworkSkill/detectFramework— backward compat soagent run --name <x>still works for agents deployed via oldskill load/registeryaml.v3/antihax/optional/conductor-goall used elsewhere toogo.mod/go.sum, install scripts, CI workflows,test/e2e/*.batsMigration note
skill load/registerstill run viaagent run --name <name>skill run/serve(local tool-worker hosting), registering new skills — no replacementIssue #
Alternatives considered
CHANGELOG.md,no
Deprecatedcobra field used anywhere), pre-1.0 (v0.1.4)frameworkSkill/detectFramework— rejected, breaksagent runforalready-deployed skill agents for zero benefit (13 LOC, no dependency on deleted code)