Uh oh!
There was an error while loading. Please reload this page.
feat(plugins): resolve plugins from declared git sources (#380) - #398
Merged
Jason Robert (jrob5756) merged 2 commits intoAug 10, 2026
Merged
Conversation
`runtime.plugins` resolved entries against machine state — an installed plugin name, or a path — so a shared workflow still needed "first install these plugins" in a README, and a teammate who skipped that step got a hard error rather than a working run. `runtime.plugin_sources` maps a marketplace name to a git or local source, and `plugins:` references it as `prs@acme`. The split between acquisition and activation follows the Copilot CLI's own settings, which separate `extraKnownMarketplaces` from `enabledPlugins`: eleven plugins commonly come from one repository, so inlining a URL per entry would either clone it eleven times or silently pick one of eleven refs. The load-bearing property is that `prs@acme` means the same thing whether the marketplace was declared, installed via a CLI, or is a local directory. A declared source registers into the same resolution table the installed roots populate, so git feeds resolution rather than adding a second code path. It also gives the ambiguity error from #378 a second remedy: qualify `git@acme` instead of falling back to a path. There is no lockfile — the YAML is the lock. A full 40-character SHA is pinned and fetched once; a tag, branch, or absent ref floats and is re-resolved every run, matching how workflow registries already behave. `conductor run` acquires sources up front and in parallel; `conductor plugin fetch` primes the cache as its own step, which is what keeps `conductor validate` off the network entirely; `conductor plugin list` reports what a run would load. An unreachable remote with a warm cache warns and reuses the checkout, so offline runs keep working. Three details are load-bearing and easy to get wrong: - Source classification cannot reuse `is_path_entry`, which returns True for anything containing '/' and would read every `owner/repo` as a relative path. A source is local by prefix instead. - The two marketplace catalog conventions anchor their per-plugin `source` differently — `.claude-plugin` at the repo root, `.github/plugin` at `pluginRoot` — verified against a real repository shipping both, so both anchors are tried. - `git ls-remote` only emits the dereferenced `^{}` line when the pattern asks for it, so an annotated tag otherwise resolves to the tag object, a SHA no checkout ever equals. Security: a '.' or '..' reaching a cache key would escape the plugin cache root into the sibling registry cache, and is refused; `ext::sh -c` is rejected at the parser and `protocol.ext.allow=never` is pinned on every git call, since `git-remote-ext` runs its argument as a shell command; credentials embedded in a source URL are redacted from every message, including git's own echoed stderr. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)force-pushed
the
feature/380-git-backed-plugin-sources
branch
from
August 10, 2026 19:17
9ef01c6 to
5ddcc76CompareReview follow-ups. The theme running through the blocking findings is the same mistake in two modules: the code raises a precise error type and then collapses the taxonomy back to its base class at the catch site. `_resolve_declared_sources` caught `PluginError` — the base of everything in `plugins/errors.py` — and discarded the whole marketplace table. Two consequences. A source that was merely unfetched took every healthy source down with it, so a local directory sitting on disk was reported as "has not been acquired" and the user was sent to a command that could never help it. And a source that was genuinely *wrong* — a path that does not exist, a `path:` escaping the checkout — surfaced as a yellow warning with exit 0, blaming the network for the author's typo. With the table emptied, every entry became a deferred check and the per-agent MCP-clash and dropped-component reporting was skipped, which is the silent divergence issue #378 exists to remove. Sources now resolve one at a time. `PluginFetchError` is deferrable and warns; `PluginSourceError` and `OSError` are errors. The same per-item treatment is applied to the entry loop in `cli/validate.py`, where one unresolvable entry erased the component counts for every other plugin. Also fixed, each found by review: - `_run_git` reported git's *last* stderr line, which for an unreachable remote is "and the repository exists." — naming no cause and prescribing no remedy. It now selects the first `fatal:`/`error:` line, and carries the full redacted output on the exception so `_clone` can classify a shallow refusal against everything git printed rather than one summary. - `_clone`'s fallback treated every failure as "remote refuses a bare SHA", discarding the original error. It now matches strings taken from the git binary itself and re-raises anything else, chaining both on failure. - The shadow warning documented on `runtime.plugin_sources` did not exist. A declared source replacing a same-named installed marketplace can ship different subagents or a different MCP server, so it is now reported. - `plugin:` could not select a catalog entry in a repository that is both a catalog and a plugin, so the remedy the ambiguity error recommends worked in only one of its two directions. - `fetch_sources` reported only the first failure, dropped duplicate futures, and blocked on `shutdown(wait=True)`. - Cache keys and ref-pointer slugs are lossy: `group/subgroup/repo` and `group_subgroup/repo` shared a directory, and `release/1.x` shared a pointer file with `release_1.x`. Both now carry a digest, so an offline run cannot be served a different repository's checkout. - `conductor plugin list` skipped `for_each` inline agents — the one agent that starts N copies of a plugin's MCP server. - `PluginSource` gained the `__post_init__` invariant its two sibling types already have, and `host`/`owner`/`repo` are required rather than defaulted, since every construction site passes them. - `conductor plugin fetch` no longer reports a green tick for a checkout it could not verify. Tests: `tests/test_plugins/conftest.py` isolates `HOME` package-wide. That was not hygiene — `test_a_sourced_plugin_is_unreachable_without_the_declaration` asserts a reference *fails* without its source declared, and an ambient marketplace on the developer's machine satisfied it, so the test guarding the regression passed vacuously. New coverage for single-provider engine mode, the sub-workflow merge in both directions, pinned-plus-cache-only, catalog narrowing, manifest precedence, ref-slug and cache-key disambiguation, the shallow-refusal classifier, git message selection, and `conductor validate`'s own plugin summary, which no test had invoked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 10, 2026 21:38
Uh oh!
There was an error while loading. Please reload this page.
Jason Robert (jrob5756)
deleted the
feature/380-git-backed-plugin-sources
branch
August 10, 2026 21:39
This was referenced Aug 11, 2026
Jason Robert (jrob5756) added a commit
that referenced
this pull request
Aug 11, 2026
* fix(cli): stop parsing runtime data as rich console markup Rich parses `[...]` in a plain str as markup, and conductor interpolated runtime data straight into those strings. A bracketed token is a tag when its first character is lowercase, `#`, `/` or `@`, so `[0]` renders fine, `[task1]` is silently deleted, and `[/etc/x]` raises MarkupError out of the print call. `style=` does not disable parsing. `conductor validate` died with an unhandled traceback on a workflow named `probe [/bold] name`, and printed `probe name` for `[dim]`. Two further consequences were already shipping: every for-each iteration's verbose panel read the same, because the engine qualifies a member's name as `<agent>[<key>]` and a `key_by:` key of `task1` erased exactly the identity that name exists to carry -- while a key starting with `/`, which `key_by:` over paths produces, killed the run from a logging call. That needed no flags; verbose and full mode both default on. `conductor status` (#389) and `conductor plugin list` (#398) were written against the same unfixed pattern in files #387 never touched, and #398 made these strings third-party rather than the author's own YAML. This is the third occurrence: #382 was the original, #387 fixed cli/run.py and still left `title=` in the function it changed. So invert the default rather than escape ~450 call sites. Every console is built by the new `conductor.console.make_console()` with `markup=False`, making a plain string literal unless it asks to be styled -- which fixes 119 sites with no code churn -- and conductor's own styling goes through `styled("<template>", value)`, which parses the template but inserts values verbatim and byte-exact. Panel titles and Prompt prompts are handled separately: rich calls `Text.from_markup` on those unconditionally, so the console setting never reaches them. That is the trap that left #387 incomplete one line from the code it changed. `rich.markup.escape` is dropped everywhere, since it cannot round-trip a value containing a backslash before a bracket. Rendering is byte-identical to main, ANSI codes included, across ten commands: rich highlights a plain str but not a Text, so the console also re-applies the ReprHighlighter to keep this a pure safety change. Five AST guards read src/conductor and fail with file:line when a new call site reintroduces any of these shapes, each with negative controls -- a source scan that quietly matches nothing reports "all clear" forever. The convention is documented in AGENTS.md rather than only in comments. Closes#406 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): restore dropped styling and close the remaining markup gaps Review follow-ups to the markup-safety change. Two real defects. The dry-run plan built its loop-target marker as a `Text` and then interpolated it into an f-string, which renders a `Text` as its plain form -- so `conductor run --dry-run` lost the yellow that distinguishes a loop target from the agent names around it. That is the fourth time this exact shape has shipped in this change, so it is now a guard rule rather than a fixed call site. And the dialog test asserted `str(title)` on a mocked console, which stays true when a regressed f-string title deletes the agent name from the rendered output; it now renders for real and requires the name in both the body and the title. `markup` is no longer overridable per call. Rich lets `print(..., markup=True)` override the instance setting, so one line could reopen both original failure modes -- and it is the obvious-looking fix for the visible `[green]` that forgetting `styled` now produces. The refusal moved onto the class so subclasses inherit it, and `MarkupFreeConsole` is public because `cli/run.py` subclasses it and AGENTS.md documents doing so. `Console.input` forwards `markup=` to `print` unconditionally, so the refusal is of a *truthy* value only and `input` forces it off; otherwise every `Prompt.ask(console=...)` would raise. Typer renders help through its own rich console, so the previous "outside this convention" carve-out was wrong: `[@registry][@Version]` was being deleted from `conductor run/resume/validate/show --help`, and that syntax appears nowhere else in the help output. The strings are escaped and rule G now enforces it. Guard corrections. Rules F (a `Text` in an f-string), G (typer help) and H (`rich.markup.escape`) are new -- the last two were documented as rules while nothing checked them. `", ".join(...)` was being treated as a `Text` producer, and `str | Text` as a Text-bearing annotation, which between them blinded rule B across a whole module; name resolution is now per-function with closure inheritance. Every rule is a shared predicate called by both the source scan and its negative control, which had already drifted apart. `styled` now refuses rather than silently loses: a format spec or conversion on a `Text` flattened away the styling the caller passed a `Text` to keep, and a field inside a tag raised `'\x00' is not in list`, naming an internal detail. Field resolution goes through `Formatter.get_field`, so dotted and indexed fields work. Rendering stays byte-identical to main across ten commands, now including a dry-run with a loop target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jason Robert <jasonrobert@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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 freeto 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#380. Follow-up to #378, which deliberately sequenced git second.
The problem
runtime.pluginsresolves entries against machine state — an installed plugin name, or a path. So a shared workflow still needs "first install these plugins" in a README, and a teammate who skips that step gets a hard error rather than a working run.The shape
Two concerns, two keys:
plugin_sourcesis acquisition,pluginsis activation. That split is not invented here — the Copilot CLI's ownsettings.jsonseparatesextraKnownMarketplacesfromenabledPlugins, and the reason is structural: 11 of 13 plugins on an ordinary machine come from one repository, so a URL per entry would either clone it 11 times or silently dedupe 11 refs to one.The load-bearing property:
prs@acmemeans the same thing whether the marketplace was declared here, installed via a CLI, or is a local directory. A declared source registers into the same resolution table the installed roots populate, so git is a source feeding resolution rather than a second code path. It also gives #378's ambiguity error a second remedy — qualifygit@acmeinstead of falling back to a path.Design decisions
No lockfile — the YAML is the lock. The issue sketched a
conductor.lockwith resolved SHAs and component counts. Instead:This matches how
registry/version_resolver.pyalready treats workflow registries, and drops--frozenandconductor plugin updatealong with the lockfile. The tradeoff is real and worth naming: an unpinned source can gain a subagent or an MCP server between two runs with nothing to diff, and an MCP server is a subprocess launched with your credentials. Pinning is a one-character edit andconductor plugin listprints the counts on demand.Network posture.
conductor runacquires up front and in parallel;conductor plugin fetchprimes the cache as its own step, which is the entire reasonconductor validatecan stay off the network;conductor plugin listreads the cache. Offline with a warm cache warns and reuses the checkout, so offline runs keep working.At validate time an unfetched source is a warning, not an error — the workflow is not wrong, the machine simply has not fetched, and
runheals it. But the warning names the checks it had to skip, since reporting "valid" when whole categories never ran would be the worse lie. An uninstalled plugin name stays an error, because only the user can fix that.Cache at
$CONDUCTOR_HOME/cache/plugins/<host>/<owner>/<repo>/<sha>/, following this repo's existing convention rather than the$XDG_CACHE_HOMEthe issue sketched. Cloning shells out togit clone --depth 1, so existing SSH keys, credential helpers and self-hosted forges work.Three things that were easy to get wrong
is_path_entry. It returnsTruefor anything containing/, so everyowner/repowould have become a relative directory lookup. A source is local by prefix instead..claude-plugin/marketplace.jsonis repo-root-relative (./dist/claude/ado),.github/plugin/marketplace.jsonispluginRoot-relative (./ado). Verified against a real marketplace repository shipping both, so both anchors are tried.git ls-remoteonly emits the dereferenced^{}line when the pattern asks for it. Without it an annotated tag resolves to the tag object — a SHA no checkout ever equals, so every run would refetch.Also caught by the load-bearing integration test: a directly-constructed
WorkflowEnginenever resolved its own sources, making the feature silently CLI-only.Security
Found in review and fixed, each with a regression test:
.or..reaching a derived cache key would escape the plugin cache root — into the sibling registry cache. Refused.ext::sh -c '...'parsed as an scp-style remote, andgit-remote-extruns its argument as a shell command. Refused at the parser andprotocol.ext.allow=neverpinned on every git call.Unchanged
Plugins are still never discovered. Resolving a declared source is resolution — the author wrote it down, a miss is a hard error — not discovery.
enable_config_discoverystays off on Copilot andsetting_sources=[]on claude-agent-sdk.Trust follows #378: declaring a source is the consent, no prompt and no allowlist. But the docs say plainly that a git source is executable content the reviewer does not have in their tree.
Testing
make checkclean; 5302 tests passtest_fetch.pyruns real git againstfile://repositories — mockingsubprocesswould test the mock, since annotated-tag dereferencing, shallow SHA fetch, and the unreachable-remote fallback are all properties of git itselfmicrosoft/conductor#main): fetch → cache → list → validatemake validate-examplesclean, including the newexamples/plugin-sources.yamlOne pre-existing failure (
test_parallel_agents_execute_concurrently, a timing assertion of< 0.18s) fails identically on a clean tree.Review notes
Worth a look: the no-lockfile call and its lost review signal, the validate-warns-rather-than-errors call on a cold cache, and whether refusing
ext::at the parser plus pinningprotocol.ext.allowis the right belt-and-braces.🤖 Generated with Copilot CLI