Support authenticated Agent Plugin installation from private repositories - #56505
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add workflow test integration test for each agentic engine that supports plugins |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added |
|
@copilot ensure the generate code issues the correct token for each plugin, add tests |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
… tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Found and fixed a real issue (cb15d74): when the same plugin path is declared twice (e.g. main workflow + import) and the higher-version declaration wins during merge, its credential previously replaced the whole entry — silently dropping a github-token/github-app set on the other declaration, so the checkout step fell back to the default token instead of the configured one. Now the credential is preserved across the version merge, and if both declarations set different credentials, compilation fails with a clear error instead of silently picking one. Added regression tests verifying: multiple plugins (public + github-token + github-app) each get the correct, isolated token on their own checkout step; multiple github-app plugins each mint and reference their own distinct token step; credential preserved when a higher/lower version wins the merge; conflicting credentials on duplicate declarations are rejected. |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
This adds the right primitives for private plugin auth, but the merge/dedup path still drops auth metadata for duplicate plugin declarations, which makes the feature order-dependent and can compile a private plugin checkout without the intended credential.
The blocking theme
- Duplicate
plugins:entries with the same plugin/ref but different auth are currently treated as harmless duplicates instead of a conflict or an intentional override, so the surviving checkout token depends on declaration order.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 46.3 AIC · ⌖ 7.44 AIC · ⊞ 7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/plugins_frontmatter.go:111
Per-plugin auth is silently discarded when the same plugin path is declared twice with different credentials but the same ref. mergeValidatedPluginRefs deduplicates only by repoPath and returns early on existing.ref == parsed.ref, so a main workflow entry can be overridden by an imported/public entry (or vice versa) without any error and the surviving checkout will use whichever credential appeared first. That makes private plugin installs fragile and order-dependent instead of determinis…
|
Warning The 🏗️ ADR required — draft added for this implementation changeI enforced the design-decision gate for this PR because the prefetch summary shows 773 added lines in business-logic directories, which exceeds the default ADR threshold, even though the PR does not have the Evidence reviewed
Gate resultNo existing Michael Nygard ADR was present in the PR body, linked issue, or current Action takenI added a draft ADR at:
Next action for the authorPlease review and refine that ADR, especially the decision rationale and trade-offs, before merging this PR. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
pkg/workflow/plugin_installation.go:169: shrink: custom linesToActionSteps duplicates collapseYAMLLinesIntoSteps in checkout_step_generator.go. Reuse the existing helper or make the plugin path return the same []string form.
net: -12 lines possible.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comapi.github.comchatgpt.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #56505 · codex · mai10 · 9.41 AIC · ⌖ 0.503 AIC · ⊞ 12.8K
Comment /ponytail to run again
| return steps | ||
| } | ||
|
|
||
| // linesToActionSteps groups newline-terminated YAML step lines (as produced by |
There was a problem hiding this comment.
pkg/workflow/plugin_installation.go:169: shrink: custom linesToActionSteps duplicates collapseYAMLLinesIntoSteps in checkout_step_generator.go. Reuse the existing helper or make the plugin path return the same []string form.
There was a problem hiding this comment.
Resolved in c1f9c39: the bespoke YAML step splitter was removed; the mint-step output is emitted as one action step.
There was a problem hiding this comment.
Review: Support authenticated Agent Plugin installation from private repositories
Overall the approach is well-structured and mirrors the existing SkillReference pattern closely. Two blocking issues were found:
🔴 Blocking
-
Credential loss during semver deduplication (
plugins_frontmatter.gomergeValidatedPluginRefs): when two declarations for the same repo use different semver tags and the winning (higher) one has no credential while the losing one does, the credential is silently dropped. See inline comment. -
Fragile YAML-line splitting (
plugin_installation.golinesToActionSteps): step boundaries are detected by a hardcoded 6-space" - "prefix. Any indentation change silently produces a corrupt lock file. See inline comment.
✅ Positives
- Schema updated with
oneOfto allow both string and object entries, with properadditionalProperties: false. - Mutual exclusivity of
github-tokenandgithub-appis validated early (frontmatter validation stage). validatePluginSupportstill gates the feature behind engine capability, so unsupported engines get a clear error.- Integration test covers all four engines (copilot, claude, codex, custom behavior-defined).
pluginTokenExpressiongracefully handles theshouldIgnoreMissingKeycase with token fallback.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 39 AIC · ⌖ 10.8 AIC · ⊞ 6.2K
Comments that could not be inline-anchored
pkg/workflow/plugins_frontmatter.go:113
Credential loss during semver deduplication — when the same plugin repo is declared twice with different semver tags (e.g. v1 with a github-token and v2 without one), the higher-version ref entirely replaces the lower-version entry:
if semverutil.Compare(parsed.ref, existing.ref) > 0 {
merged[index] = ref // ref may have no credentials
}If the lower-version declaration carried the credential and the higher-version one did not, the token is silently dropped and t…
pkg/workflow/plugin_installation.go:175
Fragile YAML-line prefix in linesToActionSteps — this function detects step boundaries by matching the hardcoded string " - " (6 spaces + - ). This will silently mis-split or merge steps if the YAML indentation ever changes (e.g. a different job-level indent), producing a corrupted lock file without any error.
The function is only ever called with output from buildGitHubAppTokenMintStepWithMeta; consider either:
- asserting/documenting the exact indent contract, or
- passing t…
There was a problem hiding this comment.
Pull request overview
Adds per-plugin authentication for private Agent Plugin checkouts.
Changes:
- Introduces structured plugin references with token or GitHub App credentials.
- Generates authenticated checkout and GitHub App token steps.
- Extends validation, schema, documentation, and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores structured plugin references. |
pkg/workflow/workflow_builder.go |
Builds merged plugin references. |
pkg/workflow/workflow_builder_test.go |
Updates builder tests. |
pkg/workflow/workflow_builder_frontmatter_extract.go |
Extracts and merges structured entries. |
pkg/workflow/plugins_test.go |
Tests authenticated compilation. |
pkg/workflow/plugins_ref_resolution.go |
Keeps references synchronized after pinning. |
pkg/workflow/plugins_frontmatter.go |
Adds parsing, validation, and deduplication. |
pkg/workflow/plugins_frontmatter_test.go |
Tests object-form frontmatter. |
pkg/workflow/plugins_engine_auth_integration_test.go |
Tests authentication across engines. |
pkg/workflow/plugin_installation.go |
Generates token and authenticated checkout steps. |
pkg/workflow/frontmatter_types.go |
Supports heterogeneous plugin entries. |
pkg/workflow/frontmatter_parsing.go |
Parses structured plugin references. |
pkg/workflow/compiler_yaml_ai_execution.go |
Emits plugin authentication steps. |
pkg/workflow/compiler_orchestrator_frontmatter.go |
Invokes plugin validation. |
pkg/parser/schemas/main_workflow_schema.json |
Adds object-form schema support. |
docs/src/content/docs/reference/frontmatter.md |
Documents private plugin authentication. |
.github/workflows/agentic_commands.yml |
Contains unrelated command-routing drift. |
Review details
Suppressed comments (1)
pkg/workflow/plugins_frontmatter.go:118
- When an authenticated main-workflow plugin is merged with a higher compatible version from an import,
refis the import's auth-free reference and this replacement discards the main workflow credential. The resulting private checkout falls back togithub.token. Inherit the existing credential whenever the winning higher-version reference has no credential of its own.
func mergePluginRefAuth(target *PluginReference, incoming PluginReference) error {
if !pluginRefHasAuth(incoming) {
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Balanced
| var steps []GitHubActionStep | ||
| for i, ref := range workflowData.PluginReferences { | ||
| if ref.GitHubApp == nil { | ||
| continue | ||
| } | ||
| lines := c.buildGitHubAppTokenMintStepWithMeta( | ||
| ref.GitHubApp, | ||
| nil, | ||
| "", | ||
| "", | ||
| fmt.Sprintf("Generate GitHub App token for agent plugin %d", i+1), | ||
| pluginAppTokenStepID(i), | ||
| ) | ||
| steps = append(steps, linesToActionSteps(lines)...) | ||
| } |
There was a problem hiding this comment.
Fixed in 6addd0f: GitHub App token minting now derives the plugin owner and repository name, while explicit configuration still takes precedence.
| existing := parseSkillRefSpec(merged[index].Plugin) | ||
| if existing.ref == parsed.ref { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Resolved in cb15d74: duplicate plugin declarations preserve a lone credential and reject conflicting credentials.
| { | ||
| "type": "object", | ||
| "description": "Object-form plugin reference with per-plugin authentication.", | ||
| "required": ["plugin"], | ||
| "additionalProperties": false, |
There was a problem hiding this comment.
Fixed in 6addd0f: imported object-form plugin entries now retain their structured credentials through ImportsResult and workflow assembly, with regression coverage.
| # /tidy -> tidy [pull_request_comment] reaction=eyes | ||
| # /unbloat -> unbloat-docs [pull_request_comment] reaction=eyes | ||
| # /wiki -> wiki [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes | ||
| # /windows -> windows [discussion,discussion_comment,issue_comment,issues,pull_request,pull_request_comment,pull_request_review_comment] reaction=eyes |
There was a problem hiding this comment.
Resolved in c1f9c39: the unrelated generated command-routing drift was reverted.
🧪 Test Quality Sentinel AnalysisPR: Support authenticated Agent Plugin installation from private repositories (#56505)SummaryTest Quality Score: 88/100 ✅ Excellent This PR introduces well-structured tests for authenticated plugin installation across multiple agentic engines. The test suite demonstrates strong design coverage with proper error-path validation and multi-engine integration testing. Key Metrics
Behavioral Test BreakdownNew Test Functions (Classified)Integration Tests (
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues and two test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
-
Fragile YAML line-splitting (
linesToActionSteps): the" - "prefix heuristic is a bespoke sub-parser that could mis-split steps if awith:value contains that prefix. The root cause is thatbuildGitHubAppTokenMintStepWithMetareturns[]stringrather than[]GitHubActionStep. -
Silent credential drop in dedup (
mergeValidatedPluginRefs): when the same plugin+ref is declared twice with differentgithub-tokenvalues, the second credential is silently discarded. This is a correctness and potential security hazard — the author may believe their override is active when it isn't. -
Implicit positional coupling between
workflowData.PluginsandworkflowData.PluginReferences:generatePluginInstallationStepsiterates the string slice and looks up credentials by the same integer index into the references slice. The invariant is maintained byvalidatePlugins, but it's an implicit contract that future callers can easily break. -
Integration test assertions are weak:
assert.Contains(t, lockText, "token: ...")confirms the token string exists in the lock file but not that it's on the correct step.
Positive Highlights
- ✅ Excellent parity with the existing
SkillReferencepattern — the PR is consistent and well-modelled. - ✅ Good backward compatibility: string-only plugins still work without any configuration change.
- ✅ Strong unit test coverage for
validateFrontmatterPluginsandpluginTokenExpression. - ✅ Engine-agnostic auth injection is a clean design decision — no per-engine changes needed.
- ✅
pluginReferencesOrFallbackis a good defensive helper for callers that only populatePlugins.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 65 AIC · ⌖ 11 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/plugin_installation.go:177
[/codebase-design] linesToActionSteps splits a []string of YAML lines by detecting " - " as a step boundary. This is a fragile heuristic: any with: value containing that exact prefix would incorrectly start a new step. All other step-builders in this file return GitHubActionStep directly.
<details>
<summary>💡 Suggested fix</summary>
If buildGitHubAppTokenMintStepWithMeta always returns one logical step, simply wrap it:
steps = append(steps, GitHubActionStep(lines)…
</details>
<details><summary>pkg/workflow/plugins_frontmatter.go:108</summary>
**[/codebase-design]** When `mergeValidatedPluginRefs` deduplicates two entries with **the same ref** (`existing.ref == parsed.ref`) but **different `github-token` values**, the second credential is silently dropped — the first one wins. There's no warning to the author that a declared credential was ignored.
<details>
<summary>💡 Suggested fix</summary>
Either error when credentials conflict:
```go
if existing.ref == parsed.ref {
if merged[index].GitHubToken != ref.GitHubToken || !appCo…
</details>
<details><summary>pkg/workflow/plugin_installation.go:202</summary>
**[/codebase-design]** `generatePluginInstallationSteps` iterates `workflowData.Plugins` (the plain-string slice) at line 202 but looks up credentials via `pluginTokenExpression(workflowData, i)` which indexes into `workflowData.PluginReferences` by the same integer `i`. This positional coupling means the two slices **must stay in sync** — and `validatePlugins` arranges that, but the contract is implicit.
<details>
<summary>💡 Suggested fix</summary>
Iterate `PluginReferences` directly (since…
</details>
<details><summary>pkg/workflow/plugins_engine_auth_integration_test.go:76</summary>
**[/tdd]** The integration test asserts that `token: ${{ secrets.PRIVATE_PLUGIN_TOKEN }}` appears *somewhere* in the lock file, but doesn't verify it appears in the correct checkout step. If the token were accidentally emitted in a different step (e.g., a staging command), the test would still pass.
<details>
<summary>💡 Suggested fix</summary>
Assert on adjacent lines to confirm placement:
```go
assert.Regexp(t, `(?s)name: Checkout agent plugin octo-org/private-plugin.*?token: \$\{\{ secret…
</details>
<details><summary>pkg/workflow/plugins_frontmatter.go:108</summary>
**[/tdd]** There are no unit tests for `mergeValidatedPluginRefs` covering the case where the same plugin is listed twice with **different credentials** but the same ref. The `continue` path at line 108 silently drops the second credential — this edge case should have an explicit test to document (and, once the bug above is addressed, to enforce) the intended behavior.
<details>
<summary>💡 Suggested test skeleton</summary>
```go
t.Run("errors when same plugin ref is declared twice with diffe…
</details>
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Remaining review feedback still needs resolution on this PR. Please address the unresolved items, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the listed feedback and ran the PR-finisher validation. The latest commit is 6addd0f. |
Private Agent Plugin marketplaces are common in enterprises, but
plugins:had no way to authenticate against private repositories — the checkout step always used the workflow's defaultgithub.token. This adds opt-in per-plugin credentials, mirroring the existingskills:feature.Frontmatter
plugins:entries alongside the existing string form:plugin,github-token,github-app(mutually exclusive).Compilation
PluginReferencetype carries per-entry auth through validation, merging/deduping, and SHA-pinning, mirroringSkillReference.github-appcredentials are minted into a short-lived installation token by the compiler before engine-specific plugin install steps run, then injected astoken:on that plugin'sactions/checkoutstep. This is engine-agnostic: checkout always happens ahead of any engine-specific install command, so no per-engine changes were needed (this also addresses the concern raised in review that plugin support is implemented separately by each engine).github-token/github-app, behavior is unchanged: notoken:override, checkout falls back to the defaultgithub.token.plugins:entries.Docs
reference/frontmatter.mdupdated to document object-formplugins:entries and drop the now-inaccurate "plugins can't be private" note.